Patchwork [U-Boot,01/10] arm: vf610: fix anadig register struct

login
register
mail settings
Submitter Marcel Ziswiler
Date Sept. 17, 2013, 10:45 a.m.
Message ID <86ae7d0a8914a2e3c21009b4e9eb8f3c92e343fb.1379414181.git.marcel@ziswiler.com>
Download mbox | patch
Permalink /patch/275428/
State Superseded
Delegated to: Albert ARIBAUD
Headers show

Comments

Marcel Ziswiler - Sept. 17, 2013, 10:45 a.m.
The anadig_reg structure started at the wrong offset (fixed by adding
resvA[4]), was missing some reserved field required for alignment
purpose (resvB[3] between pll4_denom and pll6_ctrl) and further
contained too short a reserved field causing further miss-alignment
(resv10[7]).

Discovered and tested by temporarily putting the following debug
instrumentation into board_init():
    struct anadig_reg *anadig = (struct anadig_reg *)ANADIG_BASE_ADDR;
    printf("&anadig->pll3_ctrl=0x%p\n", &anadig->pll3_ctrl);
    printf("&anadig->pll5_ctrl=0x%p\n", &anadig->pll5_ctrl);

Signed-off-by: Marcel Ziswiler <marcel@ziswiler.com>
---
 arch/arm/include/asm/arch-vf610/crm_regs.h |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Otavio Salvador - Sept. 17, 2013, 1:27 p.m.
On Tue, Sep 17, 2013 at 7:45 AM, Marcel Ziswiler <marcel@ziswiler.com> wrote:
> The anadig_reg structure started at the wrong offset (fixed by adding
> resvA[4]), was missing some reserved field required for alignment
> purpose (resvB[3] between pll4_denom and pll6_ctrl) and further
> contained too short a reserved field causing further miss-alignment
> (resv10[7]).
>
> Discovered and tested by temporarily putting the following debug
> instrumentation into board_init():
>     struct anadig_reg *anadig = (struct anadig_reg *)ANADIG_BASE_ADDR;
>     printf("&anadig->pll3_ctrl=0x%p\n", &anadig->pll3_ctrl);
>     printf("&anadig->pll5_ctrl=0x%p\n", &anadig->pll5_ctrl);
>
> Signed-off-by: Marcel Ziswiler <marcel@ziswiler.com>

I agree with the way to fix it but it is a little bit hard to get it
is a 'reserved'; we used reserved_<where> to make it more explicit.

Take a look at http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/include/asm/arch-mxs/regs-power-mx28.h;h=9528e3ce9ad805ec30a1c0595924dbddb296c50f
for an usage example.
Marcel Ziswiler - Sept. 30, 2013, 8:19 a.m.
On 09/17/2013 03:27 PM, Otavio Salvador wrote:
> I agree with the way to fix it but it is a little bit hard to get it 
> is a 'reserved'; we used reserved_<where> to make it more explicit. 
> Take a look at 
> http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/include/asm/arch-mxs/regs-power-mx28.h;h=9528e3ce9ad805ec30a1c0595924dbddb296c50f 
> for an usage example. 

At the end of the day those reserved fields are just memory holes in the 
register set. I don't think they really belong to either the previous 
last nor the first next valid register. Also I don't really see how the 
naming convention in use at above mentioned link is much more consistent.

If we really do want do change the way this is done then probably naming 
it after the actual memory offset would be most helpful (e.g. 
reserved_0x014[3]). If you agree I will cook up a new patch series in 
that respect.

Patch

diff --git a/arch/arm/include/asm/arch-vf610/crm_regs.h b/arch/arm/include/asm/arch-vf610/crm_regs.h
index 85f1fda..57a0242 100644
--- a/arch/arm/include/asm/arch-vf610/crm_regs.h
+++ b/arch/arm/include/asm/arch-vf610/crm_regs.h
@@ -55,6 +55,7 @@  struct ccm_reg {
 
 /* Analog components control digital interface (ANADIG) */
 struct anadig_reg {
+	u32 resvA[4];
 	u32 pll3_ctrl;
 	u32 resv0[3];
 	u32 pll7_ctrl;
@@ -72,12 +73,13 @@  struct anadig_reg {
 	u32 pll4_num;
 	u32 resv7[3];
 	u32 pll4_denom;
+	u32 resvB[3];
 	u32 pll6_ctrl;
 	u32 resv8[3];
 	u32 pll6_num;
 	u32 resv9[3];
 	u32 pll6_denom;
-	u32 resv10[3];
+	u32 resv10[7];
 	u32 pll5_ctrl;
 	u32 resv11[3];
 	u32 pll3_pfd;