[U-Boot,2/4] mx6: add structs for mmdc and ddr iomux registers

Signed-off-by: Edward Lin <edward.lin@technexion.com>
---
 arch/arm/include/asm/arch-mx6/mx6_ddr_regs.h | 167
+++++++++++++++++++++++++++
 1 file changed, 167 insertions(+)
 create mode 100644 arch/arm/include/asm/arch-mx6/mx6_ddr_regs.h

Hi Edward,

On 11/08/2013 02:35 AM, Edward Lin wrote:
> Signed-off-by: Edward Lin <edward.lin@technexion.com>
> ---
>   arch/arm/include/asm/arch-mx6/mx6_ddr_regs.h | 167
> +++++++++++++++++++++++++++
>   1 file changed, 167 insertions(+)
>   create mode 100644 arch/arm/include/asm/arch-mx6/mx6_ddr_regs.h
>
> diff --git a/arch/arm/include/asm/arch-mx6/mx6_ddr_regs.h
> b/arch/arm/include/asm/arch-mx6/mx6_ddr_regs.h
> new file mode 100644
> index 0000000..dd5e892
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-mx6/mx6_ddr_regs.h
> @@ -0,0 +1,167 @@
> +/*
> + * Copyright (C) 2013 TechNexion Inc.
> + *
> + * Author: Edward Lin <edward.lin@technexion.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +#ifndef __ASM_ARCH_MX6_DDR_REGS_H__
> +#define __ASM_ARCH_MX6_DDR_REGS_H__
> +
> +#define MX6_MMDC_P0_BASE       0x021b0000
> +#define MX6_MMDC_P1_BASE       0x021b4000
> +

These are already declared in
	arch/arm/include/asm/arch-mx6/imx-regs.h

> +/* MMDC P0/P1 Registers */
> +struct mmdc_p_regs {
> +       u32 mdctl;
> +       u32 mdpdc;
> +       u32 mdotc;
> +       u32 mdcfg0;
> +       u32 mdcfg1;
> +       u32 mdcfg2;
> +       u32 mdmisc;
> +       u32 mdscr;
> +       u32 mdref;
> +       u32 res1[2];
> +       u32 mdrwd;
> +       u32 mdor;

Up to this point, this appears to be a mirror of
struct esd_mmdc_regs from arch/arm/imx-common/cpu.c

> +       u32 res2[3];
> +       u32 mdasp;
> +       u32 res3[240];
> +       u32 mapsr;
> +       u32 res4[254];
> +       u32 mpzqhwctrl;
> +       u32 res5[2];
> +       u32 mpwldectrl0;
> +       u32 mpwldectrl1;
> +       u32 res6;
> +       u32 mpodtctrl;
> +       u32 mprddqby0dl;
> +       u32 mprddqby1dl;
> +       u32 mprddqby2dl;
> +       u32 mprddqby3dl;
> +       u32 res7[4];
> +       u32 mpdgctrl0;
> +       u32 mpdgctrl1;
> +       u32 res8;
> +       u32 mprddlctl;
> +       u32 res9;
> +       u32 mpwrdlctl;
> +       u32 res10[25];
> +       u32 mpmur0;
> +}__attribute__((packed, aligned(4)));
> +

I'm not sure where you're headed with these...

It seems arbitrary to treat these as structures,
and I'm guessing that you're going to want tables
of values as is currently done in the DCD and
declared in mx6q-ddr.h/mx6dl-ddr.h.

> +#define MX6Q_IOM_DDR_BASE      0x020e0500
> +struct mx6q_iomux_ddr_regs {
> +       u32 res1[3];
> +       u32 dram_sdqs5;
> +       u32 dram_dqm5;
> +       u32 dram_dqm4;
> +       u32 dram_sdqs4;
> +       u32 dram_sdqs3;
> +       u32 dram_dqm3;
> +       u32 dram_sdqs2;
> +       u32 dram_dqm2;
> +       u32 res2[16];
> +       u32 dram_cas;

...

Regards,


Eric

On Fri, 2013-11-08 at 16:42 -0700, Eric Nelson wrote:
> Hi Edward,
> 
> On 11/08/2013 02:35 AM, Edward Lin wrote:
> > Signed-off-by: Edward Lin <edward.lin@technexion.com>
> > ---
> >   arch/arm/include/asm/arch-mx6/mx6_ddr_regs.h | 167
> > +++++++++++++++++++++++++++
> >   1 file changed, 167 insertions(+)
> >   create mode 100644 arch/arm/include/asm/arch-mx6/mx6_ddr_regs.h
> >
> > diff --git a/arch/arm/include/asm/arch-mx6/mx6_ddr_regs.h
> > b/arch/arm/include/asm/arch-mx6/mx6_ddr_regs.h
> > new file mode 100644
> > index 0000000..dd5e892
> > --- /dev/null
> > +++ b/arch/arm/include/asm/arch-mx6/mx6_ddr_regs.h
> > @@ -0,0 +1,167 @@
> > +/*
> > + * Copyright (C) 2013 TechNexion Inc.
> > + *
> > + * Author: Edward Lin <edward.lin@technexion.com>
> > + *
> > + * SPDX-License-Identifier:    GPL-2.0+
> > + */
> > +#ifndef __ASM_ARCH_MX6_DDR_REGS_H__
> > +#define __ASM_ARCH_MX6_DDR_REGS_H__
> > +
> > +#define MX6_MMDC_P0_BASE       0x021b0000
> > +#define MX6_MMDC_P1_BASE       0x021b4000
> > +
> 
> These are already declared in
> 	arch/arm/include/asm/arch-mx6/imx-regs.h
> 

Thanks for pointing that out, I agree.

> > +/* MMDC P0/P1 Registers */
> > +struct mmdc_p_regs {
> > +       u32 mdctl;
> > +       u32 mdpdc;
> > +       u32 mdotc;
> > +       u32 mdcfg0;
> > +       u32 mdcfg1;
> > +       u32 mdcfg2;
> > +       u32 mdmisc;
> > +       u32 mdscr;
> > +       u32 mdref;
> > +       u32 res1[2];
> > +       u32 mdrwd;
> > +       u32 mdor;
> 
> Up to this point, this appears to be a mirror of
> struct esd_mmdc_regs from arch/arm/imx-common/cpu.c
> 

The struct defined in cpu.c is for all imx cpus, including
imx5 and imx6. For this reason I would like to keep them 
separate. Our struct is for imx6 only.

> > +       u32 res2[3];
> > +       u32 mdasp;
> > +       u32 res3[240];
> > +       u32 mapsr;
> > +       u32 res4[254];
> > +       u32 mpzqhwctrl;
> > +       u32 res5[2];
> > +       u32 mpwldectrl0;
> > +       u32 mpwldectrl1;
> > +       u32 res6;
> > +       u32 mpodtctrl;
> > +       u32 mprddqby0dl;
> > +       u32 mprddqby1dl;
> > +       u32 mprddqby2dl;
> > +       u32 mprddqby3dl;
> > +       u32 res7[4];
> > +       u32 mpdgctrl0;
> > +       u32 mpdgctrl1;
> > +       u32 res8;
> > +       u32 mprddlctl;
> > +       u32 res9;
> > +       u32 mpwrdlctl;
> > +       u32 res10[25];
> > +       u32 mpmur0;
> > +}__attribute__((packed, aligned(4)));
> > +
> 
> I'm not sure where you're headed with these...
> 
> It seems arbitrary to treat these as structures,
> and I'm guessing that you're going to want tables
> of values as is currently done in the DCD and
> declared in mx6q-ddr.h/mx6dl-ddr.h.
> 
We been told by Stefano that use of structures is a strict rule.

> > +#define MX6Q_IOM_DDR_BASE      0x020e0500
> > +struct mx6q_iomux_ddr_regs {
> > +       u32 res1[3];
> > +       u32 dram_sdqs5;
> > +       u32 dram_dqm5;
> > +       u32 dram_dqm4;
> > +       u32 dram_sdqs4;
> > +       u32 dram_sdqs3;
> > +       u32 dram_dqm3;
> > +       u32 dram_sdqs2;
> > +       u32 dram_dqm2;
> > +       u32 res2[16];
> > +       u32 dram_cas;
> 
> ...
> 
> Regards,
> 
> 
> Eric
>

Hi Edward,

On 08/11/2013 10:35, Edward Lin wrote:
> Signed-off-by: Edward Lin <edward.lin@technexion.com>
> ---
>  arch/arm/include/asm/arch-mx6/mx6_ddr_regs.h | 167
> +++++++++++++++++++++++++++
>  1 file changed, 167 insertions(+)
>  create mode 100644 arch/arm/include/asm/arch-mx6/mx6_ddr_regs.h
> 
> diff --git a/arch/arm/include/asm/arch-mx6/mx6_ddr_regs.h
> b/arch/arm/include/asm/arch-mx6/mx6_ddr_regs.h
> new file mode 100644
> index 0000000..dd5e892
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-mx6/mx6_ddr_regs.h
> @@ -0,0 +1,167 @@
> +/*
> + * Copyright (C) 2013 TechNexion Inc.
> + *
> + * Author: Edward Lin <edward.lin@technexion.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +#ifndef __ASM_ARCH_MX6_DDR_REGS_H__
> +#define __ASM_ARCH_MX6_DDR_REGS_H__
> +
> +#define MX6_MMDC_P0_BASE       0x021b0000
> +#define MX6_MMDC_P1_BASE       0x021b4000
> +
> +/* MMDC P0/P1 Registers */
> +struct mmdc_p_regs {
> +       u32 mdctl;
> +       u32 mdpdc;
> +       u32 mdotc;
> +       u32 mdcfg0;
> +       u32 mdcfg1;
> +       u32 mdcfg2;
> +       u32 mdmisc;

Ok - if these structure is to make it available for other components, it
should replace the structure esd_mmdc_regs in cpu.c. We cannot have both.

> +       u32 res10[25];
> +       u32 mpmur0;
> +}__attribute__((packed, aligned(4)));
> +

I am missing why the packed is needed.

> +#define MX6Q_IOM_DDR_BASE      0x020e0500
> +struct mx6q_iomux_ddr_regs {
> +       u32 res1[3];
> +       u32 dram_sdqs5;
> +       u32 dram_dqm5;
> +       u32 dram_dqm4;
> +       u32 dram_sdqs4;
> +       u32 dram_sdqs3;
> +       u32 dram_dqm3;
> +       u32 dram_sdqs2;
> +       u32 dram_dqm2;
> +       u32 res2[16];
> +       u32 dram_cas;
> +       u32 res3[2];
> +       u32 dram_ras;
> +       u32 dram_reset;
> +       u32 res4[2];
> +       u32 dram_sdclk_0;
> +       u32 dram_sdba2;
> +       u32 dram_sdcke0;
> +       u32 dram_sdclk_1;
> +       u32 dram_sdcke1;
> +       u32 dram_sdodt0;
> +       u32 dram_sdodt1;
> +       u32 res5;
> +       u32 dram_sdqs0;
> +       u32 dram_dqm0;
> +       u32 dram_sdqs1;
> +       u32 dram_dqm1;
> +       u32 dram_sdqs6;
> +       u32 dram_dqm6;
> +       u32 dram_sdqs7;
> +       u32 dram_dqm7;
> +}__attribute__((packed, aligned(4)));
> +
> +#define MX6Q_IOM_GRP_BASE      0x020e0700
> +struct mx6q_iomux_grp_regs {
> +       u32 res1[18];
> +       u32 grp_b7ds;
> +       u32 grp_addds;
> +       u32 grp_ddrmode_ctl;
> +       u32 res2;
> +       u32 grp_ddrpke;
> +       u32 res3[6];
> +       u32 grp_ddrmode;
> +       u32 res4[3];
> +       u32 grp_b0ds;
> +       u32 grp_b1ds;
> +       u32 grp_ctlds;
> +       u32 res5;
> +       u32 grp_b2ds;
> +       u32 grp_ddr_type;
> +       u32 grp_b3ds;
> +       u32 grp_b4ds;
> +       u32 grp_b5ds;
> +       u32 grp_b6ds;
> +}__attribute__((packed, aligned(4)));
> +
> +#define MX6DL_IOM_DDR_BASE     0x020e0400
> +struct mx6dl_iomux_ddr_regs {
> +       u32 res1[25];
> +       u32 dram_cas;
> +       u32 res2[2];
> +       u32 dram_dqm0;
> +       u32 dram_dqm1;
> +       u32 dram_dqm2;
> +       u32 dram_dqm3;
> +       u32 dram_dqm4;
> +       u32 dram_dqm5;
> +       u32 dram_dqm6;
> +       u32 dram_dqm7;
> +       u32 dram_ras;
> +       u32 dram_reset;
> +       u32 res3[2];
> +       u32 dram_sdba2;
> +       u32 dram_sdcke0;
> +       u32 dram_sdcke1;
> +       u32 dram_sdclk_0;
> +       u32 dram_sdclk_1;
> +       u32 dram_sdodt0;
> +       u32 dram_sdodt1;
> +       u32 dram_sdqs0;
> +       u32 dram_sdqs1;
> +       u32 dram_sdqs2;
> +       u32 dram_sdqs3;
> +       u32 dram_sdqs4;
> +       u32 dram_sdqs5;
> +       u32 dram_sdqs6;
> +       u32 dram_sdqs7;
> +}__attribute__((packed, aligned(4)));
> +
> +#define MX6DL_IOM_GRP_BASE     0x020e0700
> +struct mx6dl_iomux_grp_regs {
> +       u32 res1[18];
> +       u32 grp_b7ds;
> +       u32 grp_addds;
> +       u32 grp_ddrmode_ctl;
> +       u32 grp_ddrpke;
> +       u32 res2[2];
> +       u32 grp_ddrmode;
> +       u32 grp_b0ds;
> +       u32 res3;
> +       u32 grp_ctlds;
> +       u32 grp_b1ds;
> +       u32 grp_ddr_type;
> +       u32 grp_b2ds;
> +       u32 grp_b3ds;
> +       u32 grp_b4ds;
> +       u32 grp_b5ds;
> +       u32 res4;
> +       u32 grp_b6ds;
> +}__attribute__((packed, aligned(4)));
> +
> +#endif
> 

Best regards,
Stefano Babic

Stefano,

I'll reply to this, since including the __attribute__  was my suggestion.
 
> > +       u32 res10[25];
> > +       u32 mpmur0;
> > +}__attribute__((packed, aligned(4)));
> > +
> 
> I am missing why the packed is needed.
> 

This was discussed before, and I tried to explain it already then.

Short answer: to make a broken design slightly less likely to cause problems.

Long answer: Using structs the way required by u-boot maintainers is invalid C. 
It is a  compiler quirk that it happens to work. This is clear from ANSI C89 
standard: http://www.lysator.liu.se/c/rat/c5.html#3-5-2-1

However people (like Microsoft famously with Office) kept shooting themselves 
in the foot with structure members and padding, this was clarified even 
further in the C99 standard (see pt. 1424 in 
http://c0x.coding-guidelines.com/6.7.2.1.html )

Meanwhile, the gcc people added a compiler hint, __attribute__((packed)), to 
kindly ask the compiler not to add any padding structs. (So you guys certainly
are not the first ones doing this kind of hacks :-).

While on topic, the aligned(4) is needed because of the packed attribute.

There is a drawback with the gcc __attribute__((packed)). The compiler might
no longer be able to assume any alignment of the members (what if the struct 
resides on an odd address?). To remedy this some compilers create complicated 
code for accessing the struct members in a way that no CPU exceptions are 
raised, to make the structure accesses work (and some compilers just let the 
Bus Errors happen). The __attribute__((aligned(4))) tells the compiler that it 
may assume that the struct will always be aligned on a 4-byte boundary.

> > +
> > +/* MMDC P0/P1 Registers */
> > +struct mmdc_p_regs {
> > +       u32 mdctl;
> > +       u32 mdpdc;
> > +       u32 mdotc;
> > +       u32 mdcfg0;
> > +       u32 mdcfg1;
> > +       u32 mdcfg2;
> > +       u32 mdmisc;
> 
> Ok - if these structure is to make it available for other components, it
> should replace the structure esd_mmdc_regs in cpu.c. We cannot have both.
>

So would you suggest we add a union inside the structure to differentiate
imx5 and imx6? Or just make the structure imx6s/dl/q specific? 

regards,

//Tapani

Hi Tapani,

On 13/11/2013 06:50, Tapani wrote:
> 
> Stefano,
> 
> I'll reply to this, since including the __attribute__  was my suggestion.
>  
>>> +       u32 res10[25];
>>> +       u32 mpmur0;
>>> +}__attribute__((packed, aligned(4)));
>>> +
>>
>> I am missing why the packed is needed.
>>
> 
> This was discussed before, and I tried to explain it already then.
> 
> Short answer: to make a broken design slightly less likely to cause problems.
> 
> Long answer: Using structs the way required by u-boot maintainers is invalid C. 
> It is a  compiler quirk that it happens to work. This is clear from ANSI C89 
> standard: http://www.lysator.liu.se/c/rat/c5.html#3-5-2-1
> 
> However people (like Microsoft famously with Office) kept shooting themselves 
> in the foot with structure members and padding, this was clarified even 
> further in the C99 standard (see pt. 1424 in 
> http://c0x.coding-guidelines.com/6.7.2.1.html )
> 
> Meanwhile, the gcc people added a compiler hint, __attribute__((packed)), to 
> kindly ask the compiler not to add any padding structs. (So you guys certainly
> are not the first ones doing this kind of hacks :-).

The specific question should have been: why do we use packed when all
fields in the structure are already u32 ? The compiler does not
introduce padding, at least now : packed will be necessary if there are
16bit or 8 bit registers.

If gcc in the future will generate padding (let's say, all fields will
become 64 bit), this become a general issue and must be globally fixed -
I mean for all structures accessing to internal registers. But using
this only here is not consistent.

> 
> While on topic, the aligned(4) is needed because of the packed attribute.

This is clear, but again, if we needed, the fix should go globally.
Currently all structures in u-boot do not use it.

> 
> There is a drawback with the gcc __attribute__((packed)). The compiler might
> no longer be able to assume any alignment of the members (what if the struct 
> resides on an odd address?).

In u-boot such as structures are not allocated, but there address is set
in code, such as:

struct src *src_regs = (struct src *)SRC_BASE_ADDR;

of course, if SRC_ADDR is not aligned, it crashes - but this is a bug. I
think (not tested) that if you force the compiler to align the address,
and for example SRC_BASE_ADDR is not correct and set on an odd address,
the compiler will force it to the nearest aligned address, that is
presumably wrong and generates unpredictable results.

> To remedy this some compilers create complicated 
> code for accessing the struct members in a way that no CPU exceptions are 
> raised, to make the structure accesses work (and some compilers just let the 
> Bus Errors happen). The __attribute__((aligned(4))) tells the compiler that it 
> may assume that the struct will always be aligned on a 4-byte boundary.

But in the way u-boot uses the structures, a crash or CPU exception must
be raised because it signals a bug.

> 
>>> +
>>> +/* MMDC P0/P1 Registers */
>>> +struct mmdc_p_regs {
>>> +       u32 mdctl;
>>> +       u32 mdpdc;
>>> +       u32 mdotc;
>>> +       u32 mdcfg0;
>>> +       u32 mdcfg1;
>>> +       u32 mdcfg2;
>>> +       u32 mdmisc;
>>
>> Ok - if these structure is to make it available for other components, it
>> should replace the structure esd_mmdc_regs in cpu.c. We cannot have both.
>>
> 
> So would you suggest we add a union inside the structure to differentiate
> imx5 and imx6? Or just make the structure imx6s/dl/q specific? 

The main point is that I disagree on duplicating code. The same
structure with your patch is defined twice, this cannot be.

The specific register layout is defined into the corresponding
imx-regs.h (./arch/arm/include/asm/arch-mx5/imx-regs.h or
./arch/arm/include/asm/arch-mx6/imx-regs.h). If there is the same
structure (let's say, the same controller with different layout) on
different socs, you have to define the structures in the imx-regs.h for
each SOC that uses that structure.

Best regards,
Stefano Babic