Add support for Vybrid VF610-based PCM052

imximage header size is 4-byte, not 8-byte aligned. This produces .imx images that a Vybrid cannot boot on.

Fix by adding a "padding" field in header.

Signed-off-by Albert ARIBAUD (3ADEV) <[email protected]>

---

 tools/imximage.h | 1 +
 1 file changed, 1 insertion(+)

tools/imximage.h +0 -0

index 36fe095..a913329 100644

Quoted 23 lines · On 19/06/2015 14:18, Albert ARIBAUD (3ADEV) wrote:

imximage header size is 4-byte, not 8-byte aligned. This produces .imx images that a Vybrid cannot boot on.

Fix by adding a "padding" field in header.

Signed-off-by Albert ARIBAUD (3ADEV) <[email protected]>

---

 tools/imximage.h | 1 +
 1 file changed, 1 insertion(+)

tools/imximage.h +1 -0

index 36fe095..a913329 100644
@@ -129,6 +129,7 @@ typedef struct {
 	ivt_header_t header;
 	write_dcd_command_t write_dcd_command;
 	dcd_addr_data_t addr_data[MAX_HW_CFG_SIZE_V2];
+	uint32_t padding[1]; /* end up on an 8-byte boundary */
 } dcd_v2_t;
 

tools/imximage.h typedef struct { · line 135

Quoted 2 lines

tools/imximage.h +0 -0

 typedef struct {

Applied to u-boot-imx, thanks !

Quoted 2 lines

Applied to u-boot-imx, thanks !

Sorry, just stumbled over this message now.

We discussed exactly this issue already more than a year ago, see:
http://lists.denx.de/pipermail/u-boot/2014-April/177580.html

Back then you asked whether I asked Freescale about it. Earlier this year I tried to get hold of that issue and asked Freescale on Community, see:
https://community.freescale.com/thread/355161

However, no official confirmation or explanation so far.

I think my patch back then solves the issue nicer. The struct dcd_v2_t is not the reason the whole header is not aligned by 8-byte, it is a problem of the boot_data_t header which is 12 bytes long. So inserting the padding just after struct boot_data_t (or inside of struct boot_data_t) seems to be more appropriate.

Quoted 5 lines

I think my patch back then solves the issue nicer. The struct dcd_v2_t is not the reason the whole header is not aligned by 8-byte, it is a problem of the boot_data_t header which is 12 bytes long. So inserting the padding just after struct boot_data_t (or inside of struct boot_data_t) seems to be more appropriate.

I see. Albert, can you test on your board if Stefan's patch solves your issue, too ? I could then revert this one and apply Stefan's.

It remains doubious how the ROMs on Freescale are interpretating the header, but we can only test it.

Best regards,
Stefano

Hello Stefano,

Quoted 6 lines · Stefano Babic

Quoted 5 lines

I think my patch back then solves the issue nicer. The struct dcd_v2_t is not the reason the whole header is not aligned by 8-byte, it is a problem of the boot_data_t header which is 12 bytes long. So inserting the padding just after struct boot_data_t (or inside of struct boot_data_t) seems to be more appropriate.

Quoted 2 lines

I see. Albert, can you test on your board if Stefan's patch solves your issue, too ? I could then revert this one and apply Stefan's.

I can test it, but I'm sure that boot data does not need size alignment since in my case there is no such alignment and Vybrid boots. To me this means aligning boot_data_t size would introduce a constraint that is not really there.

After reading the U-Boot and Freescale discussions, IIUC you have come to the conclusion that the missing 4 bytes were in boot_data_t because it was the only structure in the header which was not 8-bytes-aligned.

However, the available documentation does not specify this constraint, and from a more experimental vewpoint, my patch adds 4 bytes at the end of the overall header, thus leaving boot data at 12 bytes (therefore leaving the dcd_table not 8-byte-aligned) and yet Vybrid can boot.

To me, this proves that the size alignment problem is not with boot_data_t but with the overall header.

If Stefan would test my patch as well, I reckon he would find it to work as well as his.

So we have two patches which fix Vybrid booting:

- one which aligns the boot_data_t /size/ but keeps its /offset/ unaligned;

- one which aligns the the boot_data_t /offset/ but keeps its /size/ unaligned.

Seems to me that the conclusion is that the actual alignment of boot_data_t does not matter and that only the alignment of the whole imx_header_v2_t size (and, consequently, offset) matters.

How about just adding an attribute((align(8))) to imx_header_v2_t?

Quoted 3 lines

It remains doubious how the ROMs on Freescale are interpretating the header, but we can only test it.

See my other answer. We could prove it by disassembling the ROM code. Any volunteer? :)

Hi Albert,

Quoted 11 lines · Albert ARIBAUD

Hello Stefano,

Quoted 6 lines · Stefano Babic

Quoted 5 lines

I think my patch back then solves the issue nicer. The struct dcd_v2_t is not the reason the whole header is not aligned by 8-byte, it is a problem of the boot_data_t header which is 12 bytes long. So inserting the padding just after struct boot_data_t (or inside of struct boot_data_t) seems to be more appropriate.

Quoted 7 lines

Quoted 2 lines

I see. Albert, can you test on your board if Stefan's patch solves your issue, too ? I could then revert this one and apply Stefan's.

I can test it, but I'm sure that boot data does not need size alignment since in my case there is no such alignment and Vybrid boots. To me this means aligning boot_data_t size would introduce a constraint that is not really there.

I quote here from both emails, since its about the same topic.

Quoted 8 lines

After reading the U-Boot and Freescale discussions, IIUC you have come to the conclusion that the missing 4 bytes were in boot_data_t because it was the only structure in the header which was not 8-bytes-aligned.

However, the available documentation does not specify this constraint, and from a more experimental vewpoint, my patch adds 4 bytes at the end of the overall header, thus leaving boot data at 12 bytes (therefore leaving the dcd_table not 8-byte-aligned) and yet Vybrid can boot.

Yes, I agree on that. Also the boot data seems not to have the 8-byte alignment requirement (see the graphics I posted on the Freescale community article).

Quoted 2 lines

To me, this proves that the size alignment problem is not with boot_data_t but with the overall header.

I agree. Still, boot_data_t is the "offender", hence I would prefer that solution over your solution.

Quoted 10 lines

If Stefan would test my patch as well, I reckon he would find it to work as well as his.

So we have two patches which fix Vybrid booting:

- one which aligns the boot_data_t /size/ but keeps its /offset/ unaligned;

- one which aligns the the boot_data_t /offset/ but keeps its /size/ unaligned.

I don't get this classification. The complete header struct shows that dcd_v2_t is _after_ boot_data_t.

typedef struct {
	flash_header_v2_t fhdr;
	boot_data_t boot_data;
	dcd_v2_t dcd_table;
} imx_header_v2_t;

Your patch changes the size of dcd_v2_t, hence compensating the "unaligned" size of boot_data_t.

As far as I see we have these two patches which fix Vybrid booting:

- one which changes the boot_data_t /size/ which keeps the offset of dcd_v2_t and the image aligned.

- one which changes the dcd_v2_t /size/ which compensates the unaligned size of boot_data_t and keeps the image aligned.

Quoted 4 lines

Seems to me that the conclusion is that the actual alignment of boot_data_t does not matter and that only the alignment of the whole imx_header_v2_t size (and, consequently, offset) matters.

Quoted 1 line

How about just adding an attribute((align(8))) to imx_header_v2_t?

Hm this sounds tempting, but it does not seem to work here. I think because it only aligns the beginning of imx_header_v2_t in to 8-byte, however it does not align the size of the whole struct to 8 bytes, I guess? Hence the header size is still "unaligned".

Quoted 5 lines

Quoted 2 lines

It remains doubious how the ROMs on Freescale are interpretating the header, but we can only test it.

See my other answer. We could prove it by disassembling the ROM code. Any volunteer? :)

Hehe, not me :-)

--
Stefan

Hello Stefan (and sorry for the duplicate),

Quoted 57 lines · Stefan Agner

Hi Albert,

Quoted 11 lines · Albert ARIBAUD

Hello Stefano,

Quoted 6 lines · Stefano Babic

Quoted 5 lines

I think my patch back then solves the issue nicer. The struct dcd_v2_t is not the reason the whole header is not aligned by 8-byte, it is a problem of the boot_data_t header which is 12 bytes long. So inserting the padding just after struct boot_data_t (or inside of struct boot_data_t) seems to be more appropriate.

Quoted 7 lines

Quoted 2 lines

I see. Albert, can you test on your board if Stefan's patch solves your issue, too ? I could then revert this one and apply Stefan's.

I can test it, but I'm sure that boot data does not need size alignment since in my case there is no such alignment and Vybrid boots. To me this means aligning boot_data_t size would introduce a constraint that is not really there.

I quote here from both emails, since its about the same topic.

Quoted 8 lines

After reading the U-Boot and Freescale discussions, IIUC you have come to the conclusion that the missing 4 bytes were in boot_data_t because it was the only structure in the header which was not 8-bytes-aligned.

However, the available documentation does not specify this constraint, and from a more experimental vewpoint, my patch adds 4 bytes at the end of the overall header, thus leaving boot data at 12 bytes (therefore leaving the dcd_table not 8-byte-aligned) and yet Vybrid can boot.

Yes, I agree on that. Also the boot data seems not to have the 8-byte alignment requirement (see the graphics I posted on the Freescale community article).

Quoted 2 lines

To me, this proves that the size alignment problem is not with boot_data_t but with the overall header.

I agree. Still, boot_data_t is the "offender", hence I would prefer that solution over your solution.

Quoted 10 lines

If Stefan would test my patch as well, I reckon he would find it to work as well as his.

So we have two patches which fix Vybrid booting:

- one which aligns the boot_data_t /size/ but keeps its /offset/ unaligned;

- one which aligns the the boot_data_t /offset/ but keeps its /size/ unaligned.

I don't get this classification. The complete header struct shows that dcd_v2_t is _after_ boot_data_t.

Quoted 8 lines

typedef struct {
	flash_header_v2_t fhdr;
	boot_data_t boot_data;
	dcd_v2_t dcd_table;
} imx_header_v2_t;

Your patch changes the size of dcd_v2_t, hence compensating the "unaligned" size of boot_data_t.

Compensating the unaligned size of the overall header for sure, since I'm adding a last field in the last structure. But precisely since I'm adding 4 bytes at the very end of the construct, we cannot tell whether I fixed the 'size' of the flash header, boot data or DCD.

What we can tell, though, is that Vybrid will boot as long as we increase the size of the overall header to an 8-byte boundary, and also that it will boot regardless to whether these 4 bytes are added between boot data and DCD or after DCD.

(note, btw, that in the Vybrid RM, the IVT itself, and the DCD, contain their own size explicitly in their tag-length-version headers, but the boot data does not contain its own size, nor is it contained elsewhere. What we call 'the size of the boot data' is 'the difference between the boot data and dcd addresses', but that's a definition for convenience.)

Quoted 14 lines

As far as I see we have these two patches which fix Vybrid booting:

- one which changes the boot_data_t /size/ which keeps the offset of dcd_v2_t and the image aligned.

- one which changes the dcd_v2_t /size/ which compensates the unaligned size of boot_data_t and keeps the image aligned.

Quoted 5 lines

Seems to me that the conclusion is that the actual alignment of boot_data_t does not matter and that only the alignment of the whole imx_header_v2_t size (and, consequently, offset) matters.

How about just adding an attribute((align(8))) to imx_header_v2_t?

Quoted 4 lines

Hm this sounds tempting, but it does not seem to work here. I think because it only aligns the beginning of imx_header_v2_t in to 8-byte, however it does not align the size of the whole struct to 8 bytes, I guess? Hence the header size is still "unaligned".

Correct -- my fault, this aligns the address, not size, and there is no attribute that will control size alignment.

So we really need to add manual padding.

And if we want to not introduce any artificial constaints, I suggest we define the padding as a separate field in the overall structure, i.e.:

typedef struct {
        flash_header_v2_t fhdr;
        boot_data_t boot_data;
        dcd_v2_t dcd_table;
	uint32_t padding; /* make size an 8-byte multiple */
} imx_header_v2_t;

I think this is the 'least inexact' solution: it does not enforce any address or size alignment constraint that is not defined in the RM, and it does show that the constraint is not on the boot data or DCD but on the header as a whole.

Functionally, it is identical to what I did, so I'm pretty sure it works. :)

Quoted 19 lines

Quoted 18 lines

As far as I see we have these two patches which fix Vybrid booting:

- one which changes the boot_data_t /size/ which keeps the offset of dcd_v2_t and the image aligned.

- one which changes the dcd_v2_t /size/ which compensates the unaligned size of boot_data_t and keeps the image aligned.

Quoted 5 lines

Seems to me that the conclusion is that the actual alignment of boot_data_t does not matter and that only the alignment of the whole imx_header_v2_t size (and, consequently, offset) matters.

How about just adding an attribute((align(8))) to imx_header_v2_t?

Hm this sounds tempting, but it does not seem to work here. I think because it only aligns the beginning of imx_header_v2_t in to 8-byte, however it does not align the size of the whole struct to 8 bytes, I guess? Hence the header size is still "unaligned".

Quoted 2 lines

Correct -- my fault, this aligns the address, not size, and there is no attribute that will control size alignment.

Actually I just discovered that "aligned" also changes the size (make sure using the correct syntax "__attribute__((aligned(8)))").

However, imximage.c seems to do some calculations using sizeof(struct imx_header) and sizeof(imx_header_v2_t) which lead to this error message:
./tools/mkimage: header error

Adding __attribute__((aligned(8))) to struct imx_header seems not to alleviate the problem... Any idea?

Quoted 11 lines

So we really need to add manual padding.

And if we want to not introduce any artificial constaints, I suggest we define the padding as a separate field in the overall structure, i.e.:

typedef struct {
        flash_header_v2_t fhdr;
        boot_data_t boot_data;
        dcd_v2_t dcd_table;
	uint32_t padding; /* make size an 8-byte multiple */
} imx_header_v2_t;

That change would also sound good to me.

Quoted 6 lines

I think this is the 'least inexact' solution: it does not enforce any address or size alignment constraint that is not defined in the RM, and it does show that the constraint is not on the boot data or DCD but on the header as a whole.

Quoted 2 lines

Functionally, it is identical to what I did, so I'm pretty sure it works. :)

Agreed.

--
Stefan

Hi Stefano,

Quoted 24 lines · Stefano Babic

Quoted 23 lines · On 19/06/2015 14:18, Albert ARIBAUD (3ADEV) wrote:

imximage header size is 4-byte, not 8-byte aligned. This produces .imx images that a Vybrid cannot boot on.

Fix by adding a "padding" field in header.

Signed-off-by Albert ARIBAUD (3ADEV) <[email protected]>

---

 tools/imximage.h | 1 +
 1 file changed, 1 insertion(+)

tools/imximage.h +1 -0

index 36fe095..a913329 100644
@@ -129,6 +129,7 @@ typedef struct {
 	ivt_header_t header;
 	write_dcd_command_t write_dcd_command;
 	dcd_addr_data_t addr_data[MAX_HW_CFG_SIZE_V2];
+	uint32_t padding[1]; /* end up on an 8-byte boundary */
 } dcd_v2_t;
 

Quoted 4 lines

tools/imximage.h typedef struct { · line 135

Quoted 2 lines

tools/imximage.h +0 -0

 typedef struct {

Applied to u-boot-imx, thanks !

Sorry, just stumbled over this message now.

We discussed exactly this issue already more than a year ago, see:
http://lists.denx.de/pipermail/u-boot/2014-April/177580.html

Back then you asked whether I asked Freescale about it. Earlier this year I tried to get hold of that issue and asked Freescale on Community, see:
https://community.freescale.com/thread/355161

However, no official confirmation or explanation so far.

I think my patch back then solves the issue nicer. The struct dcd_v2_t is not the reason the whole header is not aligned by 8-byte, it is a problem of the boot_data_t header which is 12 bytes long. So inserting the padding just after struct boot_data_t (or inside of struct boot_data_t) seems to be more appropriate.

--
Stefan

Add support in mxc_i2c driver, iomux_v3 and vf610 architecture for the four I2C instances available in VF610.

Signed-off-by Albert ARIBAUD (3ADEV) <[email protected]>

---

 arch/arm/include/asm/arch-vf610/crm_regs.h    |  3 +++
 arch/arm/include/asm/arch-vf610/imx-regs.h    |  3 +++
 arch/arm/include/asm/arch-vf610/iomux-vf610.h | 11 +++++++----
 arch/arm/include/asm/imx-common/iomux-v3.h    |  2 ++
 drivers/i2c/mxc_i2c.c                         |  3 ++-
 5 files changed, 17 insertions(+), 5 deletions(-)

arch/arm/include/asm/arch-vf610/crm_regs.h +3 -0

index fdb45e9..a46e396 100644
@@ -207,6 +207,7 @@ struct anadig_reg {
 #define CCM_CCGR4_CCM_CTRL_MASK			(0x3 << 22)
 #define CCM_CCGR4_GPC_CTRL_MASK			(0x3 << 24)
 #define CCM_CCGR4_I2C0_CTRL_MASK		(0x3 << 12)
+#define CCM_CCGR4_I2C1_CTRL_MASK		(0x3 << 14)
 #define CCM_CCGR6_OCOTP_CTRL_MASK		(0x3 << 10)
 #define CCM_CCGR6_DSPI2_CTRL_MASK		(0x3 << 24)
 #define CCM_CCGR6_DSPI3_CTRL_MASK		(0x3 << 26)

@@ -216,6 +217,8 @@ struct anadig_reg {
 #define CCM_CCGR9_FEC0_CTRL_MASK		0x3
 #define CCM_CCGR9_FEC1_CTRL_MASK		(0x3 << 2)
 #define CCM_CCGR10_NFC_CTRL_MASK		0x3
+#define CCM_CCGR10_I2C2_CTRL_MASK		(0x3 << 12)
+#define CCM_CCGR10_I2C3_CTRL_MASK		(0x3 << 14)
 
 #define ANADIG_PLL7_CTRL_BYPASS         (1 << 16)
 #define ANADIG_PLL7_CTRL_ENABLE         (1 << 13)

arch/arm/include/asm/arch-vf610/imx-regs.h +3 -0

index 7df3b1e..4366985 100644
@@ -75,6 +75,9 @@
 #define ESAI_FIFO_BASE_ADDR	(AIPS0_BASE_ADDR + 0x00063000)
 #define WDOG_BASE_ADDR		(AIPS0_BASE_ADDR + 0x00065000)
 #define I2C1_BASE_ADDR		(AIPS0_BASE_ADDR + 0x00066000)
+#define I2C2_BASE_ADDR		(AIPS0_BASE_ADDR + 0x00067000)
+#define I2C3_BASE_ADDR		(AIPS0_BASE_ADDR + 0x000E6000)
+#define I2C4_BASE_ADDR		(AIPS0_BASE_ADDR + 0x000E7000)
 #define WKUP_BASE_ADDR		(AIPS0_BASE_ADDR + 0x0006A000)
 #define CCM_BASE_ADDR		(AIPS0_BASE_ADDR + 0x0006B000)
 #define GPC_BASE_ADDR		(AIPS0_BASE_ADDR + 0x0006C000)

arch/arm/include/asm/arch-vf610/iomux-vf610.h +7 -4

index 019307b..0e2bd53 100644
@@ -20,7 +20,8 @@
 #define VF610_DDR_PAD_CTRL_1	(PAD_CTL_DSE_25ohm | \
 				PAD_CTL_INPUT_DIFFERENTIAL)
 #define VF610_I2C_PAD_CTRL	(PAD_CTL_PUS_47K_UP | PAD_CTL_DSE_50ohm | \
-				PAD_CTL_SPEED_HIGH | PAD_CTL_OBE_IBE_ENABLE)
+				PAD_CTL_SPEED_HIGH | PAD_CTL_ODE | \
+				PAD_CTL_OBE_IBE_ENABLE)
 #define VF610_NFC_IO_PAD_CTRL	(PAD_CTL_SPEED_MED | PAD_CTL_SRE | \
 				PAD_CTL_DSE_50ohm | PAD_CTL_PUS_47K_UP | \
 				PAD_CTL_OBE_IBE_ENABLE)

@@ -110,6 +111,8 @@ enum {
 	VF610_PAD_PTA29__ESDHC1_DAT3		= IOMUX_PAD(0x004c, 0x004c, 5, __NA_, 0, VF610_SDHC_PAD_CTRL),
 	VF610_PAD_PTB14__I2C0_SCL		= IOMUX_PAD(0x0090, 0x0090, 2, 0x033c, 1, VF610_I2C_PAD_CTRL),
 	VF610_PAD_PTB15__I2C0_SDA		= IOMUX_PAD(0x0094, 0x0094, 2, 0x0340, 1, VF610_I2C_PAD_CTRL),
+	VF610_PAD_PTA22__I2C2_SCL		= IOMUX_PAD(0x0030, 0x0030, 6, 0x034c, 0, VF610_I2C_PAD_CTRL),
+	VF610_PAD_PTA23__I2C2_SDA		= IOMUX_PAD(0x0034, 0x0034, 6, 0x0350, 0, VF610_I2C_PAD_CTRL),
 	VF610_PAD_PTD31__NF_IO15		= IOMUX_PAD(0x00fc, 0x00fc, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
 	VF610_PAD_PTD31__GPIO_63		= IOMUX_PAD(0x00fc, 0x00fc, 0, __NA_, 0, VF610_GPIO_PAD_CTRL),
 	VF610_PAD_PTD30__NF_IO14		= IOMUX_PAD(0x0100, 0x0100, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),

@@ -146,10 +149,10 @@ enum {
 	VF610_PAD_PTD12__GPIO_91		= IOMUX_PAD(0x016c, 0x016c, 0, __NA_, 0, VF610_GPIO_PAD_CTRL),
 	VF610_PAD_PTD13__GPIO_92		= IOMUX_PAD(0x0170, 0x0170, 0, __NA_, 0, VF610_GPIO_PAD_CTRL),
 	VF610_PAD_PTD22__NF_IO6			= IOMUX_PAD(0x0120, 0x0120, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
-	VF610_PAD_PTD21__NF_IO5			= IOMUX_PAD(0x0124, 0x0124, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL), 
-	VF610_PAD_PTD20__NF_IO4			= IOMUX_PAD(0x0128, 0x0128, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL), 
+	VF610_PAD_PTD21__NF_IO5			= IOMUX_PAD(0x0124, 0x0124, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
+	VF610_PAD_PTD20__NF_IO4			= IOMUX_PAD(0x0128, 0x0128, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
 	VF610_PAD_PTD19__NF_IO3			= IOMUX_PAD(0x012c, 0x012c, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
-	VF610_PAD_PTD18__NF_IO2			= IOMUX_PAD(0x0130, 0x0130, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL), 
+	VF610_PAD_PTD18__NF_IO2			= IOMUX_PAD(0x0130, 0x0130, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
 	VF610_PAD_PTD17__NF_IO1			= IOMUX_PAD(0x0134, 0x0134, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
 	VF610_PAD_PTD16__NF_IO0			= IOMUX_PAD(0x0138, 0x0138, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
 	VF610_PAD_PTB24__NF_WE_B		= IOMUX_PAD(0x0178, 0x0178, 5, __NA_, 0, VF610_NFC_CN_PAD_CTRL),

arch/arm/include/asm/imx-common/iomux-v3.h +2 -0

index 2581019..5cde90f 100644
@@ -127,6 +127,8 @@ typedef u64 iomux_v3_cfg_t;
 
 #define PAD_CTL_SRE		(1 << 11)
 
+#define PAD_CTL_ODE		(1 << 10)
+
 #define PAD_CTL_DSE_150ohm	(1 << 6)
 #define PAD_CTL_DSE_50ohm	(3 << 6)
 #define PAD_CTL_DSE_25ohm	(6 << 6)

drivers/i2c/mxc_i2c.c +0 -0

index b3c50aa..f1056e2 100644

The VF610 DDRMC driver code contains settings which are board-specific. Move these out to boards so that new boards can define their own without having to modify the driver.

Signed-off-by Albert ARIBAUD (3ADEV) <[email protected]>

---

 arch/arm/imx-common/ddrmc-vf610.c             | 239 ++++++--------------------
 arch/arm/include/asm/arch-vf610/ddrmc-vf610.h |  60 +------
 board/freescale/vf610twr/vf610twr.c           | 181 +++++++++++++------
 board/toradex/colibri_vf/colibri_vf.c         | 169 +++++++++++++-----
 4 files changed, 312 insertions(+), 337 deletions(-)

arch/arm/imx-common/ddrmc-vf610.c +52 -187

index e462631..44b9a0f 100644
@@ -12,9 +12,9 @@
 #include <asm/arch/iomux-vf610.h>
 #include <asm/arch/ddrmc-vf610.h>
 
-void ddrmc_setup_iomux(void)
+void ddrmc_setup_iomux(const iomux_v3_cfg_t *pads, int pads_count)
 {
-	static const iomux_v3_cfg_t ddr_pads[] = {
+	static const iomux_v3_cfg_t default_pads[] = {
 		VF610_PAD_DDR_A15__DDR_A_15,
 		VF610_PAD_DDR_A14__DDR_A_14,
 		VF610_PAD_DDR_A13__DDR_A_13,

@@ -65,212 +65,77 @@ void ddrmc_setup_iomux(void)
 		VF610_PAD_DDR_RESETB,
 	};
 
-	imx_iomux_v3_setup_multiple_pads(ddr_pads, ARRAY_SIZE(ddr_pads));
-}
+	if ((pads == NULL) || (pads_count == 0)) {
+		pads = default_pads;
+		pads_count = ARRAY_SIZE(default_pads);
+	}
 
-void ddrmc_phy_init(void)
-{
-	struct ddrmr_regs *ddrmr = (struct ddrmr_regs *)DDR_BASE_ADDR;
+	imx_iomux_v3_setup_multiple_pads(pads, pads_count);
+}
 
-	writel(DDRMC_PHY_DQ_TIMING, &ddrmr->phy[0]);
-	writel(DDRMC_PHY_DQ_TIMING, &ddrmr->phy[16]);
-	writel(DDRMC_PHY_DQ_TIMING, &ddrmr->phy[32]);
+static struct ddrmc_reg_setting default_phy_settings[] = {
+	{ DDRMC_PHY_DQ_TIMING,  0 },
+	{ DDRMC_PHY_DQ_TIMING, 16 },
+	{ DDRMC_PHY_DQ_TIMING, 32 },
 
-	writel(DDRMC_PHY_DQS_TIMING, &ddrmr->phy[1]);
-	writel(DDRMC_PHY_DQS_TIMING, &ddrmr->phy[17]);
+	{ DDRMC_PHY_DQS_TIMING,  1 },
+	{ DDRMC_PHY_DQS_TIMING, 17 },
 
-	writel(DDRMC_PHY_CTRL, &ddrmr->phy[2]);
-	writel(DDRMC_PHY_CTRL, &ddrmr->phy[18]);
-	writel(DDRMC_PHY_CTRL, &ddrmr->phy[34]);
+	{ DDRMC_PHY_CTRL,  2 },
+	{ DDRMC_PHY_CTRL, 18 },
+	{ DDRMC_PHY_CTRL, 34 },
 
-	writel(DDRMC_PHY_MASTER_CTRL, &ddrmr->phy[3]);
-	writel(DDRMC_PHY_MASTER_CTRL, &ddrmr->phy[19]);
-	writel(DDRMC_PHY_MASTER_CTRL, &ddrmr->phy[35]);
+	{ DDRMC_PHY_MASTER_CTRL,  3 },
+	{ DDRMC_PHY_MASTER_CTRL, 19 },
+	{ DDRMC_PHY_MASTER_CTRL, 35 },
 
-	writel(DDRMC_PHY_SLAVE_CTRL, &ddrmr->phy[4]);
-	writel(DDRMC_PHY_SLAVE_CTRL, &ddrmr->phy[20]);
-	writel(DDRMC_PHY_SLAVE_CTRL, &ddrmr->phy[36]);
+	{ DDRMC_PHY_SLAVE_CTRL,  4 },
+	{ DDRMC_PHY_SLAVE_CTRL, 20 },
+	{ DDRMC_PHY_SLAVE_CTRL, 36 },
 
 	/* LPDDR2 only parameter */
-	writel(DDRMC_PHY_OFF, &ddrmr->phy[49]);
+	{ DDRMC_PHY_OFF, 49 },
 
-	writel(DDRMC_PHY50_DDR3_MODE |
-		   DDRMC_PHY50_EN_SW_HALF_CYCLE, &ddrmr->phy[50]);
+	{ DDRMC_PHY50_DDR3_MODE | DDRMC_PHY50_EN_SW_HALF_CYCLE, 50 },
 
 	/* Processor Pad ODT settings */
-	writel(DDRMC_PHY_PROC_PAD_ODT, &ddrmr->phy[52]);
-}
+	{ DDRMC_PHY_PROC_PAD_ODT, 52 },
 
-static void ddrmc_ctrl_lvl_init(struct ddrmc_lvl_info *lvl)
-{
-	struct ddrmr_regs *ddrmr = (struct ddrmr_regs *)DDR_BASE_ADDR;
-	u32 cr102 = 0, cr105 = 0, cr106 = 0, cr110 = 0;
+	/* end marker */
+	{ 0, -1 }
+};
 
-	if (lvl->wrlvl_reg_en) {
-		writel(DDRMC_CR97_WRLVL_EN, &ddrmr->cr[97]);
-		writel(DDRMC_CR98_WRLVL_DL_0(lvl->wrlvl_dl_0), &ddrmr->cr[98]);
-		writel(DDRMC_CR99_WRLVL_DL_1(lvl->wrlvl_dl_1), &ddrmr->cr[99]);
-	}
-
-	if (lvl->rdlvl_reg_en) {
-		cr102 |= DDRMC_CR102_RDLVL_REG_EN;
-		cr105 |= DDRMC_CR105_RDLVL_DL_0(lvl->rdlvl_dl_0);
-		cr110 |= DDRMC_CR110_RDLVL_DL_1(lvl->rdlvl_dl_1);
-	}
-
-	if (lvl->rdlvl_gt_reg_en) {
-		cr102 |= DDRMC_CR102_RDLVL_GT_REGEN;
-		cr106 |= DDRMC_CR106_RDLVL_GTDL_0(lvl->rdlvl_gt_dl_0);
-		cr110 |= DDRMC_CR110_RDLVL_GTDL_1(lvl->rdlvl_gt_dl_1);
-	}
-
-	writel(cr102, &ddrmr->cr[102]);
-	writel(cr105, &ddrmr->cr[105]);
-	writel(cr106, &ddrmr->cr[106]);
-	writel(cr110, &ddrmr->cr[110]);
-}
-
-void ddrmc_ctrl_init_ddr3(struct ddr3_jedec_timings const *timings,
-						  struct ddrmc_lvl_info *lvl,
-						  int col_diff, int row_diff)
+void ddrmc_ctrl_init_ddr3(struct ddrmc_reg_setting const *cr_settings,
+	struct ddrmc_reg_setting const *phy_settings)
 {
 	struct ddrmr_regs *ddrmr = (struct ddrmr_regs *)DDR_BASE_ADDR;
+	struct ddrmc_reg_setting const *cr_setting = cr_settings;
+	struct ddrmc_reg_setting const *phy_setting = phy_settings;
 
+	/* configure controller for DDR3 and start init sequence */
 	writel(DDRMC_CR00_DRAM_CLASS_DDR3, &ddrmr->cr[0]);
-	writel(DDRMC_CR02_DRAM_TINIT(timings->tinit), &ddrmr->cr[2]);
-	writel(DDRMC_CR10_TRST_PWRON(timings->trst_pwron), &ddrmr->cr[10]);
-
-	writel(DDRMC_CR11_CKE_INACTIVE(timings->cke_inactive), &ddrmr->cr[11]);
-	writel(DDRMC_CR12_WRLAT(timings->wrlat) |
-		   DDRMC_CR12_CASLAT_LIN(timings->caslat_lin), &ddrmr->cr[12]);
-	writel(DDRMC_CR13_TRC(timings->trc) | DDRMC_CR13_TRRD(timings->trrd) |
-		   DDRMC_CR13_TCCD(timings->tccd), &ddrmr->cr[13]);
-	writel(DDRMC_CR14_TFAW(timings->tfaw) | DDRMC_CR14_TRP(timings->trp) |
-		   DDRMC_CR14_TWTR(timings->twtr) |
-		   DDRMC_CR14_TRAS_MIN(timings->tras_min), &ddrmr->cr[14]);
-	writel(DDRMC_CR16_TMRD(timings->tmrd) |
-		   DDRMC_CR16_TRTP(timings->trtp), &ddrmr->cr[16]);
-	writel(DDRMC_CR17_TRAS_MAX(timings->tras_max) |
-		   DDRMC_CR17_TMOD(timings->tmod), &ddrmr->cr[17]);
-	writel(DDRMC_CR18_TCKESR(timings->tckesr) |
-		   DDRMC_CR18_TCKE(timings->tcke), &ddrmr->cr[18]);
 
-	writel(DDRMC_CR20_AP_EN, &ddrmr->cr[20]);
-	writel(DDRMC_CR21_TRCD_INT(timings->trcd_int) |
-		   DDRMC_CR21_CCMAP_EN, &ddrmr->cr[21]);
-
-	writel(DDRMC_CR22_TDAL(timings->tdal), &ddrmr->cr[22]);
-	writel(DDRMC_CR23_BSTLEN(3) |
-		   DDRMC_CR23_TDLL(timings->tdll), &ddrmr->cr[23]);
-	writel(DDRMC_CR24_TRP_AB(timings->trp_ab), &ddrmr->cr[24]);
-
-	writel(DDRMC_CR25_TREF_EN, &ddrmr->cr[25]);
-	writel(DDRMC_CR26_TREF(timings->tref) |
-		   DDRMC_CR26_TRFC(timings->trfc), &ddrmr->cr[26]);
-	writel(DDRMC_CR28_TREF_INT(0), &ddrmr->cr[28]);
-	writel(DDRMC_CR29_TPDEX(timings->tpdex), &ddrmr->cr[29]);
-
-	writel(DDRMC_CR30_TXPDLL(timings->txpdll), &ddrmr->cr[30]);
-	writel(DDRMC_CR31_TXSNR(timings->txsnr) |
-		   DDRMC_CR31_TXSR(timings->txsr), &ddrmr->cr[31]);
-	writel(DDRMC_CR33_EN_QK_SREF, &ddrmr->cr[33]);
-	writel(DDRMC_CR34_CKSRX(timings->cksrx) |
-		   DDRMC_CR34_CKSRE(timings->cksre), &ddrmr->cr[34]);
-
-	writel(DDRMC_CR38_FREQ_CHG_EN(0), &ddrmr->cr[38]);
-	writel(DDRMC_CR39_PHY_INI_COM(1024) | DDRMC_CR39_PHY_INI_STA(16) |
-		   DDRMC_CR39_FRQ_CH_DLLOFF(2), &ddrmr->cr[39]);
-
-	writel(DDRMC_CR41_PHY_INI_STRT_INI_DIS, &ddrmr->cr[41]);
-	writel(DDRMC_CR48_MR1_DA_0(70) |
-		   DDRMC_CR48_MR0_DA_0(1056), &ddrmr->cr[48]);
-
-	writel(DDRMC_CR66_ZQCL(timings->zqcl) |
-		   DDRMC_CR66_ZQINIT(timings->zqinit), &ddrmr->cr[66]);
-	writel(DDRMC_CR67_ZQCS(timings->zqcs), &ddrmr->cr[67]);
-	writel(DDRMC_CR69_ZQ_ON_SREF_EX(2), &ddrmr->cr[69]);
-
-	writel(DDRMC_CR70_REF_PER_ZQ(timings->ref_per_zq), &ddrmr->cr[70]);
-	writel(DDRMC_CR72_ZQCS_ROTATE(0), &ddrmr->cr[72]);
-
-	writel(DDRMC_CR73_APREBIT(timings->aprebit) |
-		   DDRMC_CR73_COL_DIFF(col_diff) |
-		   DDRMC_CR73_ROW_DIFF(row_diff), &ddrmr->cr[73]);
-	writel(DDRMC_CR74_BANKSPLT_EN | DDRMC_CR74_ADDR_CMP_EN |
-		   DDRMC_CR74_CMD_AGE_CNT(64) | DDRMC_CR74_AGE_CNT(64),
-		   &ddrmr->cr[74]);
-	writel(DDRMC_CR75_RW_PG_EN | DDRMC_CR75_RW_EN | DDRMC_CR75_PRI_EN |
-		   DDRMC_CR75_PLEN, &ddrmr->cr[75]);
-	writel(DDRMC_CR76_NQENT_ACTDIS(3) | DDRMC_CR76_D_RW_G_BKCN(3) |
-		   DDRMC_CR76_W2R_SPLT_EN, &ddrmr->cr[76]);
-	writel(DDRMC_CR77_CS_MAP | DDRMC_CR77_DI_RD_INTLEAVE |
-		   DDRMC_CR77_SWAP_EN, &ddrmr->cr[77]);
-	writel(DDRMC_CR78_Q_FULLNESS(7) |
-		   DDRMC_CR78_BUR_ON_FLY_BIT(12), &ddrmr->cr[78]);
-	writel(DDRMC_CR79_CTLUPD_AREF(0), &ddrmr->cr[79]);
+	/* execute CR sequence (MUST be supplied) */
+	if (cr_setting)
+		while (cr_setting->reg_num >= 0) {
+			writel(cr_setting->setting,
+			       &ddrmr->cr[cr_setting->reg_num]);
+			cr_setting++;
+		}
+
+	/* execute PHY sequence (SHOULD be supplied but can be default) */
+	if (phy_setting == NULL)
+		phy_setting = default_phy_settings;
+
+	while (phy_setting->reg_num >= 0) {
+		writel(phy_setting->setting,
+		       &ddrmr->phy[phy_setting->reg_num]);
+		phy_setting++;
+	}
 
+	/* final config: disable DDR interrupts and finish init squence */
 	writel(DDRMC_CR82_INT_MASK, &ddrmr->cr[82]);
 
-	writel(DDRMC_CR87_ODT_WR_MAPCS0, &ddrmr->cr[87]);
-	writel(DDRMC_CR88_TODTL_CMD(4), &ddrmr->cr[88]);
-	writel(DDRMC_CR89_AODT_RWSMCS(2), &ddrmr->cr[89]);
-
-	writel(DDRMC_CR91_R2W_SMCSDL(2), &ddrmr->cr[91]);
-	writel(DDRMC_CR96_WLMRD(timings->wlmrd) |
-		   DDRMC_CR96_WLDQSEN(timings->wldqsen), &ddrmr->cr[96]);
-
-	if (lvl != NULL)
-		ddrmc_ctrl_lvl_init(lvl);
-
-	writel(DDRMC_CR117_AXI0_W_PRI(0) |
-		   DDRMC_CR117_AXI0_R_PRI(0), &ddrmr->cr[117]);
-	writel(DDRMC_CR118_AXI1_W_PRI(1) |
-		   DDRMC_CR118_AXI1_R_PRI(1), &ddrmr->cr[118]);
-
-	writel(DDRMC_CR120_AXI0_PRI1_RPRI(2) |
-		   DDRMC_CR120_AXI0_PRI0_RPRI(2), &ddrmr->cr[120]);
-	writel(DDRMC_CR121_AXI0_PRI3_RPRI(2) |
-		   DDRMC_CR121_AXI0_PRI2_RPRI(2), &ddrmr->cr[121]);
-	writel(DDRMC_CR122_AXI1_PRI1_RPRI(1) | DDRMC_CR122_AXI1_PRI0_RPRI(1) |
-		   DDRMC_CR122_AXI0_PRIRLX(100), &ddrmr->cr[122]);
-	writel(DDRMC_CR123_AXI1_P_ODR_EN | DDRMC_CR123_AXI1_PRI3_RPRI(1) |
-		   DDRMC_CR123_AXI1_PRI2_RPRI(1), &ddrmr->cr[123]);
-	writel(DDRMC_CR124_AXI1_PRIRLX(100), &ddrmr->cr[124]);
-
-	writel(DDRMC_CR126_PHY_RDLAT(8), &ddrmr->cr[126]);
-	writel(DDRMC_CR132_WRLAT_ADJ(5) |
-		   DDRMC_CR132_RDLAT_ADJ(6), &ddrmr->cr[132]);
-	writel(DDRMC_CR137_PHYCTL_DL(2), &ddrmr->cr[137]);
-	writel(DDRMC_CR138_PHY_WRLV_MXDL(256) |
-		   DDRMC_CR138_PHYDRAM_CK_EN(1), &ddrmr->cr[138]);
-	writel(DDRMC_CR139_PHY_WRLV_RESPLAT(4) | DDRMC_CR139_PHY_WRLV_LOAD(7) |
-		   DDRMC_CR139_PHY_WRLV_DLL(3) |
-		   DDRMC_CR139_PHY_WRLV_EN(3), &ddrmr->cr[139]);
-	writel(DDRMC_CR140_PHY_WRLV_WW(64), &ddrmr->cr[140]);
-	writel(DDRMC_CR143_RDLV_GAT_MXDL(1536) |
-		   DDRMC_CR143_RDLV_MXDL(128), &ddrmr->cr[143]);
-	writel(DDRMC_CR144_PHY_RDLVL_RES(4) | DDRMC_CR144_PHY_RDLV_LOAD(7) |
-		   DDRMC_CR144_PHY_RDLV_DLL(3) |
-		   DDRMC_CR144_PHY_RDLV_EN(3), &ddrmr->cr[144]);
-	writel(DDRMC_CR145_PHY_RDLV_RR(64), &ddrmr->cr[145]);
-	writel(DDRMC_CR146_PHY_RDLVL_RESP(64), &ddrmr->cr[146]);
-	writel(DDRMC_CR147_RDLV_RESP_MASK(983040), &ddrmr->cr[147]);
-	writel(DDRMC_CR148_RDLV_GATE_RESP_MASK(983040), &ddrmr->cr[148]);
-	writel(DDRMC_CR151_RDLV_GAT_DQ_ZERO_CNT(1) |
-		   DDRMC_CR151_RDLVL_DQ_ZERO_CNT(1), &ddrmr->cr[151]);
-
-	writel(DDRMC_CR154_PAD_ZQ_EARLY_CMP_EN_TIMER(13) |
-		   DDRMC_CR154_PAD_ZQ_MODE(1) |
-		   DDRMC_CR154_DDR_SEL_PAD_CONTR(3) |
-		   DDRMC_CR154_PAD_ZQ_HW_FOR(1), &ddrmr->cr[154]);
-	writel(DDRMC_CR155_PAD_ODT_BYTE1(2) |
-		   DDRMC_CR155_PAD_ODT_BYTE0(2), &ddrmr->cr[155]);
-	writel(DDRMC_CR158_TWR(6), &ddrmr->cr[158]);
-	writel(DDRMC_CR161_ODT_EN(1) | DDRMC_CR161_TODTH_RD(2) |
-		   DDRMC_CR161_TODTH_WR(2), &ddrmr->cr[161]);
-
-	ddrmc_phy_init();
-
 	writel(DDRMC_CR00_DRAM_CLASS_DDR3 | DDRMC_CR00_START, &ddrmr->cr[0]);
 
 	while (!(readl(&ddrmr->cr[80]) && 0x100))

arch/arm/include/asm/arch-vf610/ddrmc-vf610.h +6 -54

index 6730cde..bc7f106 100644
@@ -11,62 +11,14 @@
 #ifndef __ASM_ARCH_VF610_DDRMC_H
 #define __ASM_ARCH_VF610_DDRMC_H
 
-struct ddrmc_lvl_info {
-	u16 wrlvl_reg_en;
-	u16 wrlvl_dl_0;
-	u16 wrlvl_dl_1;
-	u16 rdlvl_gt_reg_en;
-	u16 rdlvl_gt_dl_0;
-	u16 rdlvl_gt_dl_1;
-	u16 rdlvl_reg_en;
-	u16 rdlvl_dl_0;
-	u16 rdlvl_dl_1;
+struct ddrmc_reg_setting {
+	u32	setting;
+	int	reg_num; /* CR or PHY ; -1 for last entry */
 };
 
-struct ddr3_jedec_timings {
-	u8 tinit;
-	u32 trst_pwron;
-	u32 cke_inactive;
-	u8 wrlat;
-	u8 caslat_lin;
-	u8 trc;
-	u8 trrd;
-	u8 tccd;
-	u8 tfaw;
-	u8 trp;
-	u8 twtr;
-	u8 tras_min;
-	u8 tmrd;
-	u8 trtp;
-	u32 tras_max;
-	u8 tmod;
-	u8 tckesr;
-	u8 tcke;
-	u8 trcd_int;
-	u8 tdal;
-	u16 tdll;
-	u8 trp_ab;
-	u16 tref;
-	u8 trfc;
-	u8 tpdex;
-	u8 txpdll;
-	u8 txsnr;
-	u16 txsr;
-	u8 cksrx;
-	u8 cksre;
-	u16 zqcl;
-	u16 zqinit;
-	u8 zqcs;
-	u8 ref_per_zq;
-	u8 aprebit;
-	u8 wlmrd;
-	u8 wldqsen;
-};
-
-void ddrmc_setup_iomux(void);
+void ddrmc_setup_iomux(const iomux_v3_cfg_t *pads, int pads_count);
 void ddrmc_phy_init(void);
-void ddrmc_ctrl_init_ddr3(struct ddr3_jedec_timings const *timings,
-						  struct ddrmc_lvl_info *lvl,
-						  int col_diff, int row_diff);
+void ddrmc_ctrl_init_ddr3(struct ddrmc_reg_setting const *cr_settings,
+	struct ddrmc_reg_setting const *phy_settings);
 
 #endif

board/freescale/vf610twr/vf610twr.c +127 -54

index 4160acd..aab20ad 100644
@@ -28,63 +28,136 @@ DECLARE_GLOBAL_DATA_PTR;
 #define ENET_PAD_CTRL	(PAD_CTL_PUS_47K_UP | PAD_CTL_SPEED_HIGH | \
 			PAD_CTL_DSE_50ohm | PAD_CTL_OBE_IBE_ENABLE)
 
+static struct ddrmc_reg_setting vf610twr_cr_settings[] = {
+	{ DDRMC_CR02_DRAM_TINIT(5), 2 },
+	{ DDRMC_CR10_TRST_PWRON(80000), 10 },
+	{ DDRMC_CR11_CKE_INACTIVE(200000), 11 },
+	{ DDRMC_CR12_WRLAT(5) | DDRMC_CR12_CASLAT_LIN(12), 12 },
+	{ DDRMC_CR13_TRC(21) | DDRMC_CR13_TRRD(4) | DDRMC_CR13_TCCD(4) |
+		   DDRMC_CR13_TBST_INT_INTERVAL(0), 13 },
+	{ DDRMC_CR14_TFAW(20) | DDRMC_CR14_TRP(6) | DDRMC_CR14_TWTR(5) |
+		   DDRMC_CR14_TRAS_MIN(15), 14 },
+	{ DDRMC_CR16_TMRD(4) | DDRMC_CR16_TRTP(4), 16 },
+	{ DDRMC_CR17_TRAS_MAX(28080) | DDRMC_CR17_TMOD(12), 17 },
+	{ DDRMC_CR18_TCKESR(4) | DDRMC_CR18_TCKE(3), 18 },
+
+	{ DDRMC_CR20_AP_EN, 20 },
+	{ DDRMC_CR21_TRCD_INT(6) | DDRMC_CR21_CCMAP_EN, 21 },
+
+	{ DDRMC_CR22_TDAL(12), 22 },
+	{ DDRMC_CR23_BSTLEN(3) |
+		   DDRMC_CR23_TDLL(512), 23 },
+	{ DDRMC_CR24_TRP_AB(6), 24 },
+
+	{ DDRMC_CR25_TREF_EN, 25 },
+	{ DDRMC_CR26_TREF(3120) | DDRMC_CR26_TRFC(44), 26 },
+	{ DDRMC_CR28_TREF_INT(0), 28 },
+	{ DDRMC_CR29_TPDEX(3), 29 },
+
+	{ DDRMC_CR30_TXPDLL(10), 30 },
+	{ DDRMC_CR31_TXSNR(48) | DDRMC_CR31_TXSR(468), 31 },
+	{ DDRMC_CR33_EN_QK_SREF, 33 },
+	{ DDRMC_CR34_CKSRX(5) | DDRMC_CR34_CKSRE(5), 34 },
+
+	{ DDRMC_CR38_FREQ_CHG_EN(0), 38 },
+	{ DDRMC_CR39_PHY_INI_COM(1024) | DDRMC_CR39_PHY_INI_STA(16) |
+		   DDRMC_CR39_FRQ_CH_DLLOFF(2), 39 },
+
+	{ DDRMC_CR41_PHY_INI_STRT_INI_DIS, 41 },
+	{ DDRMC_CR48_MR1_DA_0(70) |
+		   DDRMC_CR48_MR0_DA_0(1056), 48 },
+
+	{ DDRMC_CR66_ZQCL(256) | DDRMC_CR66_ZQINIT(512), 66 },
+	{ DDRMC_CR67_ZQCS(64), 67 },
+	{ DDRMC_CR69_ZQ_ON_SREF_EX(2), 69 },
+
+	{ DDRMC_CR70_REF_PER_ZQ(64), 70 },
+	{ DDRMC_CR72_ZQCS_ROTATE(0), 72 },
+
+	{ DDRMC_CR73_APREBIT(10) | DDRMC_CR73_COL_DIFF(1) |
+		   DDRMC_CR73_ROW_DIFF(3), 73 },
+	{ DDRMC_CR74_BANKSPLT_EN | DDRMC_CR74_ADDR_CMP_EN |
+		   DDRMC_CR74_CMD_AGE_CNT(64) |
+		   DDRMC_CR74_AGE_CNT(64), 74 },
+	{ DDRMC_CR75_RW_PG_EN | DDRMC_CR75_RW_EN | DDRMC_CR75_PRI_EN |
+		   DDRMC_CR75_PLEN, 75 },
+	{ DDRMC_CR76_NQENT_ACTDIS(3) | DDRMC_CR76_D_RW_G_BKCN(3) |
+		   DDRMC_CR76_W2R_SPLT_EN, 76 },
+	{ DDRMC_CR77_CS_MAP | DDRMC_CR77_DI_RD_INTLEAVE |
+		   DDRMC_CR77_SWAP_EN, 77 },
+	{ DDRMC_CR78_Q_FULLNESS(7) | DDRMC_CR78_BUR_ON_FLY_BIT(12), 78 },
+	{ DDRMC_CR79_CTLUPD_AREF(0), 79 },
+
+	{ DDRMC_CR87_ODT_WR_MAPCS0, 87 },
+	{ DDRMC_CR88_TODTL_CMD(4), 88 },
+	{ DDRMC_CR89_AODT_RWSMCS(2), 89 },
+
+	{ DDRMC_CR91_R2W_SMCSDL(2), 91 },
+	{ DDRMC_CR96_WLMRD(40) | DDRMC_CR96_WLDQSEN(25), 96 },
+
+	/* levelling */
+	{ DDRMC_CR97_WRLVL_EN, 97 },
+	{ DDRMC_CR98_WRLVL_DL_0(0), 98 },
+	{ DDRMC_CR99_WRLVL_DL_1(0), 99 },
+	{ DDRMC_CR102_RDLVL_REG_EN | DDRMC_CR102_RDLVL_GT_REGEN, 102 },
+	{ DDRMC_CR105_RDLVL_DL_0(0), 105 },
+	{ DDRMC_CR106_RDLVL_GTDL_0(4), 106 },
+	{ DDRMC_CR110_RDLVL_DL_1(0) | DDRMC_CR110_RDLVL_GTDL_1(4), 110 },
+
+	/* AXI */
+	{ DDRMC_CR117_AXI0_W_PRI(0) | DDRMC_CR117_AXI0_R_PRI(0), 117 },
+	{ DDRMC_CR118_AXI1_W_PRI(1) | DDRMC_CR118_AXI1_R_PRI(1), 118 },
+	{ DDRMC_CR120_AXI0_PRI1_RPRI(2) |
+		   DDRMC_CR120_AXI0_PRI0_RPRI(2), 120 },
+	{ DDRMC_CR121_AXI0_PRI3_RPRI(2) |
+		   DDRMC_CR121_AXI0_PRI2_RPRI(2), 121 },
+	{ DDRMC_CR122_AXI1_PRI1_RPRI(1) | DDRMC_CR122_AXI1_PRI0_RPRI(1) |
+		   DDRMC_CR122_AXI0_PRIRLX(100), 122 },
+	{ DDRMC_CR123_AXI1_P_ODR_EN | DDRMC_CR123_AXI1_PRI3_RPRI(1) |
+		   DDRMC_CR123_AXI1_PRI2_RPRI(1), 123 },
+	{ DDRMC_CR124_AXI1_PRIRLX(100), 124 },
+	{ DDRMC_CR126_PHY_RDLAT(8), 126 },
+	{ DDRMC_CR132_WRLAT_ADJ(5) |
+		   DDRMC_CR132_RDLAT_ADJ(6), 132 },
+	{ DDRMC_CR137_PHYCTL_DL(2), 137 },
+	{ DDRMC_CR138_PHY_WRLV_MXDL(256) |
+		   DDRMC_CR138_PHYDRAM_CK_EN(1), 138 },
+	{ DDRMC_CR139_PHY_WRLV_RESPLAT(4) | DDRMC_CR139_PHY_WRLV_LOAD(7) |
+		   DDRMC_CR139_PHY_WRLV_DLL(3) |
+		   DDRMC_CR139_PHY_WRLV_EN(3), 139 },
+	{ DDRMC_CR140_PHY_WRLV_WW(64), 140 },
+	{ DDRMC_CR143_RDLV_GAT_MXDL(1536) |
+		   DDRMC_CR143_RDLV_MXDL(128), 143 },
+	{ DDRMC_CR144_PHY_RDLVL_RES(4) | DDRMC_CR144_PHY_RDLV_LOAD(7) |
+		   DDRMC_CR144_PHY_RDLV_DLL(3) |
+		   DDRMC_CR144_PHY_RDLV_EN(3), 144 },
+	{ DDRMC_CR145_PHY_RDLV_RR(64), 145 },
+	{ DDRMC_CR146_PHY_RDLVL_RESP(64), 146 },
+	{ DDRMC_CR147_RDLV_RESP_MASK(983040), 147 },
+	{ DDRMC_CR148_RDLV_GATE_RESP_MASK(983040), 148 },
+	{ DDRMC_CR151_RDLV_GAT_DQ_ZERO_CNT(1) |
+		   DDRMC_CR151_RDLVL_DQ_ZERO_CNT(1), 151 },
+
+	{ DDRMC_CR154_PAD_ZQ_EARLY_CMP_EN_TIMER(13) |
+		   DDRMC_CR154_PAD_ZQ_MODE(1) |
+		   DDRMC_CR154_DDR_SEL_PAD_CONTR(3) |
+		   DDRMC_CR154_PAD_ZQ_HW_FOR(1), 154 },
+	{ DDRMC_CR155_PAD_ODT_BYTE1(1) | DDRMC_CR155_PAD_ODT_BYTE0(1), 155 },
+	{ DDRMC_CR158_TWR(6), 158 },
+	{ DDRMC_CR161_ODT_EN(1) | DDRMC_CR161_TODTH_RD(2) |
+		   DDRMC_CR161_TODTH_WR(2), 161 },
+
+	/* end marker */
+	{ 0, -1 }
+}
+
 int dram_init(void)
 {
-	struct ddrmc_lvl_info lvl = {
-		.wrlvl_reg_en = 1,
-		.wrlvl_dl_0 = 0,
-		.wrlvl_dl_1 = 0,
-		.rdlvl_gt_reg_en = 1,
-		.rdlvl_gt_dl_0 = 4,
-		.rdlvl_gt_dl_1 = 4,
-		.rdlvl_reg_en = 1,
-		.rdlvl_dl_0 = 0,
-		.rdlvl_dl_1 = 0,
-	};
-
-	static const struct ddr3_jedec_timings timings = {
-		.tinit           = 5,
-		.trst_pwron      = 80000,
-		.cke_inactive    = 200000,
-		.wrlat           = 5,
-		.caslat_lin      = 12,
-		.trc             = 21,
-		.trrd            = 4,
-		.tccd            = 4,
-		.tfaw            = 20,
-		.trp             = 6,
-		.twtr            = 4,
-		.tras_min        = 15,
-		.tmrd            = 4,
-		.trtp            = 4,
-		.tras_max        = 28080,
-		.tmod            = 12,
-		.tckesr          = 4,
-		.tcke            = 3,
-		.trcd_int        = 6,
-		.tdal            = 12,
-		.tdll            = 512,
-		.trp_ab          = 6,
-		.tref            = 3120,
-		.trfc            = 44,
-		.tpdex           = 3,
-		.txpdll          = 10,
-		.txsnr           = 48,
-		.txsr            = 468,
-		.cksrx           = 5,
-		.cksre           = 5,
-		.zqcl            = 256,
-		.zqinit          = 512,
-		.zqcs            = 64,
-		.ref_per_zq      = 64,
-		.aprebit         = 10,
-		.wlmrd           = 40,
-		.wldqsen         = 25,
-	};
-
-	ddrmc_setup_iomux();
+	/* use driver default IOMUX settings */
+	ddrmc_setup_iomux(NULL, 0);
 
-	ddrmc_ctrl_init_ddr3(&timings, &lvl, 1, 3);
+	/* provide own CR setup but use default PHY setup */
+	ddrmc_ctrl_init_ddr3(vf610twr_cr_settings, NULL);
 	gd->ram_size = get_ram_size((void *)PHYS_SDRAM, PHYS_SDRAM_SIZE);
 
 	return 0;

board/toradex/colibri_vf/colibri_vf.c +127 -42

index 8618fd0..01b3106 100644
@@ -39,51 +39,136 @@ static const iomux_v3_cfg_t usb_pads[] = {
 	VF610_PAD_PTD4__GPIO_83,
 };
 
+static struct ddrmc_reg_setting colibri_vf_cr_settings[] = {
+	{ DDRMC_CR02_DRAM_TINIT(5), 2 },
+	{ DDRMC_CR10_TRST_PWRON(80000), 10 },
+	{ DDRMC_CR11_CKE_INACTIVE(200000), 11 },
+	{ DDRMC_CR12_WRLAT(5) | DDRMC_CR12_CASLAT_LIN(12), 12 },
+	{ DDRMC_CR13_TRC(21) | DDRMC_CR13_TRRD(4) | DDRMC_CR13_TCCD(4) |
+		   DDRMC_CR13_TBST_INT_INTERVAL(0), 13 },
+	{ DDRMC_CR14_TFAW(20) | DDRMC_CR14_TRP(6) | DDRMC_CR14_TWTR(5) |
+		   DDRMC_CR14_TRAS_MIN(15), 14 },
+	{ DDRMC_CR16_TMRD(4) | DDRMC_CR16_TRTP(4), 16 },
+	{ DDRMC_CR17_TRAS_MAX(28080) | DDRMC_CR17_TMOD(12), 17 },
+	{ DDRMC_CR18_TCKESR(4) | DDRMC_CR18_TCKE(3), 18 },
+
+	{ DDRMC_CR20_AP_EN, 20 },
+	{ DDRMC_CR21_TRCD_INT(6) | DDRMC_CR21_CCMAP_EN, 21 },
+
+	{ DDRMC_CR22_TDAL(12), 22 },
+	{ DDRMC_CR23_BSTLEN(3) |
+		   DDRMC_CR23_TDLL(512), 23 },
+	{ DDRMC_CR24_TRP_AB(6), 24 },
+
+	{ DDRMC_CR25_TREF_EN, 25 },
+	{ DDRMC_CR26_TREF(3120) | DDRMC_CR26_TRFC(44), 26 },
+	{ DDRMC_CR28_TREF_INT(0), 28 },
+	{ DDRMC_CR29_TPDEX(3), 29 },
+
+	{ DDRMC_CR30_TXPDLL(10), 30 },
+	{ DDRMC_CR31_TXSNR(48) | DDRMC_CR31_TXSR(468), 31 },
+	{ DDRMC_CR33_EN_QK_SREF, 33 },
+	{ DDRMC_CR34_CKSRX(5) | DDRMC_CR34_CKSRE(5), 34 },
+
+	{ DDRMC_CR38_FREQ_CHG_EN(0), 38 },
+	{ DDRMC_CR39_PHY_INI_COM(1024) | DDRMC_CR39_PHY_INI_STA(16) |
+		   DDRMC_CR39_FRQ_CH_DLLOFF(2), 39 },
+
+	{ DDRMC_CR41_PHY_INI_STRT_INI_DIS, 41 },
+	{ DDRMC_CR48_MR1_DA_0(70) |
+		   DDRMC_CR48_MR0_DA_0(1056), 48 },
+
+	{ DDRMC_CR66_ZQCL(256) | DDRMC_CR66_ZQINIT(512), 66 },
+	{ DDRMC_CR67_ZQCS(64), 67 },
+	{ DDRMC_CR69_ZQ_ON_SREF_EX(2), 69 },
+
+	{ DDRMC_CR70_REF_PER_ZQ(64), 70 },
+	{ DDRMC_CR72_ZQCS_ROTATE(0), 72 },
+
+	{ DDRMC_CR73_APREBIT(10) | DDRMC_CR73_COL_DIFF(1) |
+		   DDRMC_CR73_ROW_DIFF(3), 73 },
+	{ DDRMC_CR74_BANKSPLT_EN | DDRMC_CR74_ADDR_CMP_EN |
+		   DDRMC_CR74_CMD_AGE_CNT(64) |
+		   DDRMC_CR74_AGE_CNT(64), 74 },
+	{ DDRMC_CR75_RW_PG_EN | DDRMC_CR75_RW_EN | DDRMC_CR75_PRI_EN |
+		   DDRMC_CR75_PLEN, 75 },
+	{ DDRMC_CR76_NQENT_ACTDIS(3) | DDRMC_CR76_D_RW_G_BKCN(3) |
+		   DDRMC_CR76_W2R_SPLT_EN, 76 },
+	{ DDRMC_CR77_CS_MAP | DDRMC_CR77_DI_RD_INTLEAVE |
+		   DDRMC_CR77_SWAP_EN, 77 },
+	{ DDRMC_CR78_Q_FULLNESS(7) | DDRMC_CR78_BUR_ON_FLY_BIT(12), 78 },
+	{ DDRMC_CR79_CTLUPD_AREF(0), 79 },
+
+	{ DDRMC_CR87_ODT_WR_MAPCS0, 87 },
+	{ DDRMC_CR88_TODTL_CMD(4), 88 },
+	{ DDRMC_CR89_AODT_RWSMCS(2), 89 },
+
+	{ DDRMC_CR91_R2W_SMCSDL(2), 91 },
+	{ DDRMC_CR96_WLMRD(40) | DDRMC_CR96_WLDQSEN(25), 96 },
+
+	/* levelling */
+	{ DDRMC_CR97_WRLVL_EN, 97 },
+	{ DDRMC_CR98_WRLVL_DL_0(0), 98 },
+	{ DDRMC_CR99_WRLVL_DL_1(0), 99 },
+	{ DDRMC_CR102_RDLVL_REG_EN | DDRMC_CR102_RDLVL_GT_REGEN, 102 },
+	{ DDRMC_CR105_RDLVL_DL_0(0), 105 },
+	{ DDRMC_CR106_RDLVL_GTDL_0(4), 106 },
+	{ DDRMC_CR110_RDLVL_DL_1(0) | DDRMC_CR110_RDLVL_GTDL_1(4), 110 },
+
+	/* AXI */
+	{ DDRMC_CR117_AXI0_W_PRI(0) | DDRMC_CR117_AXI0_R_PRI(0), 117 },
+	{ DDRMC_CR118_AXI1_W_PRI(1) | DDRMC_CR118_AXI1_R_PRI(1), 118 },
+	{ DDRMC_CR120_AXI0_PRI1_RPRI(2) |
+		   DDRMC_CR120_AXI0_PRI0_RPRI(2), 120 },
+	{ DDRMC_CR121_AXI0_PRI3_RPRI(2) |
+		   DDRMC_CR121_AXI0_PRI2_RPRI(2), 121 },
+	{ DDRMC_CR122_AXI1_PRI1_RPRI(1) | DDRMC_CR122_AXI1_PRI0_RPRI(1) |
+		   DDRMC_CR122_AXI0_PRIRLX(100), 122 },
+	{ DDRMC_CR123_AXI1_P_ODR_EN | DDRMC_CR123_AXI1_PRI3_RPRI(1) |
+		   DDRMC_CR123_AXI1_PRI2_RPRI(1), 123 },
+	{ DDRMC_CR124_AXI1_PRIRLX(100), 124 },
+	{ DDRMC_CR126_PHY_RDLAT(8), 126 },
+	{ DDRMC_CR132_WRLAT_ADJ(5) |
+		   DDRMC_CR132_RDLAT_ADJ(6), 132 },
+	{ DDRMC_CR137_PHYCTL_DL(2), 137 },
+	{ DDRMC_CR138_PHY_WRLV_MXDL(256) |
+		   DDRMC_CR138_PHYDRAM_CK_EN(1), 138 },
+	{ DDRMC_CR139_PHY_WRLV_RESPLAT(4) | DDRMC_CR139_PHY_WRLV_LOAD(7) |
+		   DDRMC_CR139_PHY_WRLV_DLL(3) |
+		   DDRMC_CR139_PHY_WRLV_EN(3), 139 },
+	{ DDRMC_CR140_PHY_WRLV_WW(64), 140 },
+	{ DDRMC_CR143_RDLV_GAT_MXDL(1536) |
+		   DDRMC_CR143_RDLV_MXDL(128), 143 },
+	{ DDRMC_CR144_PHY_RDLVL_RES(4) | DDRMC_CR144_PHY_RDLV_LOAD(7) |
+		   DDRMC_CR144_PHY_RDLV_DLL(3) |
+		   DDRMC_CR144_PHY_RDLV_EN(3), 144 },
+	{ DDRMC_CR145_PHY_RDLV_RR(64), 145 },
+	{ DDRMC_CR146_PHY_RDLVL_RESP(64), 146 },
+	{ DDRMC_CR147_RDLV_RESP_MASK(983040), 147 },
+	{ DDRMC_CR148_RDLV_GATE_RESP_MASK(983040), 148 },
+	{ DDRMC_CR151_RDLV_GAT_DQ_ZERO_CNT(1) |
+		   DDRMC_CR151_RDLVL_DQ_ZERO_CNT(1), 151 },
+
+	{ DDRMC_CR154_PAD_ZQ_EARLY_CMP_EN_TIMER(13) |
+		   DDRMC_CR154_PAD_ZQ_MODE(1) |
+		   DDRMC_CR154_DDR_SEL_PAD_CONTR(3) |
+		   DDRMC_CR154_PAD_ZQ_HW_FOR(1), 154 },
+	{ DDRMC_CR155_PAD_ODT_BYTE1(1) | DDRMC_CR155_PAD_ODT_BYTE0(1), 155 },
+	{ DDRMC_CR158_TWR(6), 158 },
+	{ DDRMC_CR161_ODT_EN(1) | DDRMC_CR161_TODTH_RD(2) |
+		   DDRMC_CR161_TODTH_WR(2), 161 },
+
+	/* end marker */
+	{ 0, -1 }
+}
+
 int dram_init(void)
 {
-	static const struct ddr3_jedec_timings timings = {
-		.tinit           = 5,
-		.trst_pwron      = 80000,
-		.cke_inactive    = 200000,
-		.wrlat           = 5,
-		.caslat_lin      = 12,
-		.trc             = 21,
-		.trrd            = 4,
-		.tccd            = 4,
-		.tfaw            = 20,
-		.trp             = 6,
-		.twtr            = 4,
-		.tras_min        = 15,
-		.tmrd            = 4,
-		.trtp            = 4,
-		.tras_max        = 28080,
-		.tmod            = 12,
-		.tckesr          = 4,
-		.tcke            = 3,
-		.trcd_int        = 6,
-		.tdal            = 12,
-		.tdll            = 512,
-		.trp_ab          = 6,
-		.tref            = 3120,
-		.trfc            = 64,
-		.tpdex           = 3,
-		.txpdll          = 10,
-		.txsnr           = 48,
-		.txsr            = 468,
-		.cksrx           = 5,
-		.cksre           = 5,
-		.zqcl            = 256,
-		.zqinit          = 512,
-		.zqcs            = 64,
-		.ref_per_zq      = 64,
-		.aprebit         = 10,
-		.wlmrd           = 40,
-		.wldqsen         = 25,
-	};
-
-	ddrmc_setup_iomux();
+	/* use driver default IOMUX settings */
+	ddrmc_setup_iomux(NULL, 0);
 
-	ddrmc_ctrl_init_ddr3(&timings, NULL, 1, 2);
+	/* provide own CR setup but use default PHY setup */
+	ddrmc_ctrl_init_ddr3(&colibri_vf_cr_settings, NULL, 1, 2);
 	gd->ram_size = get_ram_size((void *)PHYS_SDRAM, PHYS_SDRAM_SIZE);
 
 	return 0;

Hi Albert,

Quoted 4 lines · On 19.06.2015 14:18, Albert ARIBAUD (3ADEV) wrote:

The VF610 DDRMC driver code contains settings which are board-specific. Move these out to boards so that new boards can define their own without having to modify the driver.

Quoted 8 lines

Signed-off-by Albert ARIBAUD (3ADEV) <[email protected]>

---

 arch/arm/imx-common/ddrmc-vf610.c             | 239 ++++++--------------------
 arch/arm/include/asm/arch-vf610/ddrmc-vf610.h |  60 +------
 board/freescale/vf610twr/vf610twr.c           | 181 +++++++++++++------
 board/toradex/colibri_vf/colibri_vf.c         | 169 +++++++++++++-----
 4 files changed, 312 insertions(+), 337 deletions(-)

So this goes basically back to setting all DDR registers directly from boards? What we tried to do here is to factor out the memory chip specific data (JEDEC). The idea behind this is it would make it simpler to add new RAM timings if a new RAM vendor is used on a different board revision... With your changes it will be unnecessarily hard to add just a new RAM timing for a new board revision...

--
Stefan

Hi Albert,

Quoted 4 lines · On 19.06.2015 14:18, Albert ARIBAUD (3ADEV) wrote:

The VF610 DDRMC driver code contains settings which are board-specific. Move these out to boards so that new boards can define their own without having to modify the driver.

Quoted 8 lines

Signed-off-by Albert ARIBAUD (3ADEV) <[email protected]>

---

 arch/arm/imx-common/ddrmc-vf610.c             | 239 ++++++--------------------
 arch/arm/include/asm/arch-vf610/ddrmc-vf610.h |  60 +------
 board/freescale/vf610twr/vf610twr.c           | 181 +++++++++++++------
 board/toradex/colibri_vf/colibri_vf.c         | 169 +++++++++++++-----
 4 files changed, 312 insertions(+), 337 deletions(-)

So this goes basically back to setting all DDR registers directly from boards? What we tried to do here is to factor out the memory chip specific data (JEDEC). The idea behind this is it would make it simpler to add new RAM timings if a new RAM vendor is used on a different board revision... With your changes it will be unnecessarily hard to add just a new RAM timing for a new board revision...

--
Stefan

I could probably factor back out the JEDEC settings, but there are still differences in the lists of registers to write between the existing vf610twr/colibri_vf and the new pcm052, especially the PHY regs but elsewhere too, and there are some writes in the driver that the PCM052 does not have.

As I wanted to leave the existing boards strictly unaffected, and as I did not want to start sprinkling '#if defined(some-board)' over the driver code, I went for a fully board-controlled design so that no board could possibly be affected by any future change to the driver.

How about a mix? I could keep the JEDEC and lvl pointers in the DDR controller init call arguments and append "per-boards" CR and PHY arrays. The driver would do the JEDEC writes (thus keeping JEDEC DDR3 additions simple), the LVL writes if not NULL, then the "per-board" CR writes if not NULL, then the current common PHY writes, then the "per-board" PHY writes if not null.

This would keep common parts (JEDEC and minimal settings) in the driver while allowing board their own specific settings -- even overriding the driver settings, since the per-board writes would come last before CR000 is rewritten.

Would that be ok ?

Quoted 1 line

--

Hi Albert, Stefan,

Quoted 11 lines · Albert ARIBAUD

I could probably factor back out the JEDEC settings, but there are still differences in the lists of registers to write between the existing vf610twr/colibri_vf and the new pcm052, especially the PHY regs but elsewhere too, and there are some writes in the driver that the PCM052 does not have.

As I wanted to leave the existing boards strictly unaffected, and as I did not want to start sprinkling '#if defined(some-board)' over the driver code, I went for a fully board-controlled design so that no board could possibly be affected by any future change to the driver.

Quoted 6 lines

How about a mix? I could keep the JEDEC and lvl pointers in the DDR controller init call arguments and append "per-boards" CR and PHY arrays. The driver would do the JEDEC writes (thus keeping JEDEC DDR3 additions simple), the LVL writes if not NULL, then the "per-board" CR writes if not NULL, then the current common PHY writes, then the "per-board" PHY writes if not null.

This matches IMHO what we have already tried to do with most of Frescale's i.MXes, putting general code and setting in the arch/cpu/ (or in the imx_common for MX5 and MX6), but letting the board code to write the board specific part.

Some mix seems to me a goog compromise between flexibility and common code.

Quoted 8 lines

This would keep common parts (JEDEC and minimal settings) in the driver while allowing board their own specific settings -- even overriding the driver settings, since the per-board writes would come last before CR000 is rewritten.

Would that be ok ?

Quoted 1 line

--

Best regards,
Stefano Babic

Hi Albert, Hi Stefano,

Quoted 26 lines · Stefano Babic

Hi Albert, Stefan,

Quoted 11 lines · Albert ARIBAUD

I could probably factor back out the JEDEC settings, but there are still differences in the lists of registers to write between the existing vf610twr/colibri_vf and the new pcm052, especially the PHY regs but elsewhere too, and there are some writes in the driver that the PCM052 does not have.

As I wanted to leave the existing boards strictly unaffected, and as I did not want to start sprinkling '#if defined(some-board)' over the driver code, I went for a fully board-controlled design so that no board could possibly be affected by any future change to the driver.

Quoted 6 lines

How about a mix? I could keep the JEDEC and lvl pointers in the DDR controller init call arguments and append "per-boards" CR and PHY arrays. The driver would do the JEDEC writes (thus keeping JEDEC DDR3 additions simple), the LVL writes if not NULL, then the "per-board" CR writes if not NULL, then the current common PHY writes, then the "per-board" PHY writes if not null.

This matches IMHO what we have already tried to do with most of Frescale's i.MXes, putting general code and setting in the arch/cpu/ (or in the imx_common for MX5 and MX6), but letting the board code to write the board specific part.

Quoted 1 line

Some mix seems to me a goog compromise between flexibility and common code.

As far as I understood Alberts proposition it would mean that we write some registers directly from board code right? I'm not sure if this works out for all registers, since some might have JEDEC and Board specific fields in one register...?

Looking throug some CR registers lead me to belive that most settings are exact the same or already covered by the JEDEC struct... The few remaning ones could be part of a renamed ddrmc_lvl_info struct (e.g. ddrmc_cr_info).

The same for the PHY settings (ddrmc_phy_info). This would lead to a common place where all registers gets written, while having the flexibility to use board specific data... No back and forth between the board file and the common code.

I did not an exact survey how much of the registers are actually different, but I feel that the difference is not that big that moving the whole initialization to the board files is worth the effort...

--
Stefan

Quoted 12 lines

Quoted 8 lines

This would keep common parts (JEDEC and minimal settings) in the driver while allowing board their own specific settings -- even overriding the driver settings, since the per-board writes would come last before CR000 is rewritten.

Would that be ok ?

Quoted 1 line

--

Best regards,
Stefano Babic

This locally defined struct is actually only used once and as an opaque type. Remove it for clarity.

Signed-off-by Albert ARIBAUD (3ADEV) <[email protected]>

---

This locally defined struct is actually only used once and as an opaque type. Remove it for clarity.

Signed-off-by Albert ARIBAUD (3ADEV) <[email protected]>

---

 drivers/net/fec_mxc.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

drivers/net/fec_mxc.c +3 -12

index 9225d37..c5dcbbb 100644
@@ -67,13 +67,6 @@ DECLARE_GLOBAL_DATA_PTR;
 
 #undef DEBUG
 
-struct nbuf {
-	uint8_t data[1500];	/**< actual data */
-	int length;		/**< actual length */
-	int used;		/**< buffer in use or not */
-	uint8_t head[16];	/**< MAC header(6 + 6 + 2) + 2(aligned) */
-};
-
 #ifdef CONFIG_FEC_MXC_SWAP_PACKET
 static void swap_packet(uint32_t *packet, int length)
 {

@@ -775,7 +768,6 @@ static int fec_recv(struct eth_device *dev)
 	struct fec_bd *rbd = &fec->rbd_base[fec->rbd_index];
 	unsigned long ievent;
 	int frame_length, len = 0;
-	struct nbuf *frame;
 	uint16_t bd_status;
 	uint32_t addr, size, end;
 	int i;

@@ -835,12 +827,11 @@ static int fec_recv(struct eth_device *dev)
 			/*
 			 * Get buffer address and size
 			 */
-			frame = (struct nbuf *)readl(&rbd->data_pointer);
+			addr = readl(&rbd->data_pointer);
 			frame_length = readw(&rbd->data_length) - 4;
 			/*
 			 * Invalidate data cache over the buffer
 			 */
-			addr = (uint32_t)frame;
 			end = roundup(addr + frame_length, ARCH_DMA_MINALIGN);
 			addr &= ~(ARCH_DMA_MINALIGN - 1);
 			invalidate_dcache_range(addr, end);

@@ -849,16 +840,15 @@ static int fec_recv(struct eth_device *dev)
 			 *  Fill the buffer and pass it to upper layers
 			 */
 #ifdef CONFIG_FEC_MXC_SWAP_PACKET
-			swap_packet((uint32_t *)frame->data, frame_length);
+			swap_packet((uint32_t *)addr, frame_length);
 #endif
-			memcpy(buff, frame->data, frame_length);
+			memcpy(buff, (char *)addr, frame_length);
 			net_process_received_packet(buff, frame_length);
 			len = frame_length;