| Message ID | 86ae7d0a8914a2e3c21009b4e9eb8f3c92e343fb.1379414181.git.marcel@ziswiler.com |
|---|---|
| State | Superseded |
| Delegated to: | Albert ARIBAUD |
| Headers | show |
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.
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.
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;
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(-)