[U-Boot] [PATCH V3 28/32] mx6q_4x_mt41j128.cfg: add mx6 solo/duallite support

[Troy Kisky <troy.kisky@boundarydevices.com>]

On 10/8/2012 11:46 AM, Eric Nelson wrote:
> Hi Troy,
>
> This seems to be the patch where the rubber meets the road in
> much of this series.
>
> On 10/03/2012 06:47 PM, Troy Kisky wrote:
>> Signed-off-by: Troy Kisky<troy.kisky@boundarydevices.com>
>> ---
>>   board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg |  120 
>> +++++++++++++++++++-------
>>   1 file changed, 87 insertions(+), 33 deletions(-)
>>
>> diff --git a/board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg 
>> b/board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg
>> index 9e20db0..f45f93e 100644
>> --- a/board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg
>> +++ b/board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg
>> @@ -49,6 +49,15 @@ BOOT_FROM      sd
>>    *      Address   absolute address of the register
>>    *      value     value to be stored in the register
>>    */
>> +/*
>> + * DDR3 settings
>> + * MX6Q    ddr is limited to 1066 Mhz, currently 1056 MHz(528 MHz 
>> clock),
>> + *       memory bus width: 64 bits, x16/x32/x64
>> + * MX6DL   ddr is limited to 800 MHz(400 MHz clock)
>> + *       memory bus width: 64 bits, x16/x32/x64
>> + * MX6SOLO ddr is limited to 800 MHz(400 MHz clock)
>> + *       memory bus width: 32 bits, x16/x32
>> + */
>
> This comment seems to be critical to understanding some of
> what's below, since now three **different** memory configurations
> are represented in this file.
>
> At a minimum, the file name should be changed to get rid of
> '6q' and '4x' since those don't necessarily apply.

Right, my preference is to put at back in 
board/freescale/mx6qsabrelite/imximage.cfg
since MPDGCTRL0/1 DQS GATE registers and MMDC_MPRDDQBYnDL should be very 
board specific,
but that may be meet by objections from Fabio.


>
>>   WRITE_ENTRY1(IOM_DRAM_SDQS0, 0x00000030)
>>   WRITE_ENTRY1(IOM_DRAM_SDQS1, 0x00000030)
>>   WRITE_ENTRY1(IOM_DRAM_SDQS2, 0x00000030)
>> @@ -90,6 +99,7 @@ WRITE_ENTRY1(IOM_GRP_B6DS, 0x00000030)
>>   WRITE_ENTRY1(IOM_GRP_B7DS, 0x00000030)
>>
>>   WRITE_ENTRY1(IOM_GRP_ADDDS, 0x00000030)
>> +
>>   /* (differential input) */
>>   WRITE_ENTRY1(IOM_DDRMODE_CTL, 0x00020000)
>>   /* disable ddr pullups */
>> @@ -119,48 +129,92 @@ WRITE_ENTRY1(MMDC_P0_MDMISC, 0x00081740)
>>    * MDSCR, con_req
>>    */
>>   WRITE_ENTRY1(MMDC_P0_MDSCR, 0x00008000)
>> +
>>   /*
>> - * MDCFG0, tRFC=0x56 clocks, tXS=0x5b clocks
>> - * tXP=4 clocks, tXPDLL=13 clocks
>> - * tFAW=24 clocks, cas=8 cycles
>> + * MDCFG0,
>> + * MX6Q:
>> + *    tRFC=0x56 clocks, tXS=0x5b clocks, tXP=4 clocks, tXPDLL=13 clocks
>> + *    tFAW=24 clocks, cas=8 cycles
>> + * MX6DL/SOLO:
>> + *    tRFC=0x6a clocks, tXS=0x6e clocks, tXP=3 clocks, tXPDLL=10 clocks
>> + *     tFAW=19 clocks, cas=6 cycles
>>    */
>> -WRITE_ENTRY1(MMDC_P0_MDCFG0, 0x555A7975)
>> +WRITE_ENTRY2(MMDC_P0_MDCFG0, 0x555A7975, 0x696D5323)
>> +
>
> Here's where I start to get lost in the macro-fu.
>
> The WRITE_ENTRY1 macros make some sense to me in that they
> always write a single value to an offset whose address
> changes based on the processor type.
>
> In order to understand WRITE_ENTRY2, you really have to
> know that the dual, solo, and sololite share many
> characteristics.

RIght, ENTRY2 is used when there are differences between
mx6q, and mx6dl/solo. In the case above, the value
0x555A7975 is written if a mx6quad, and the value 0x696D5323
is written if a mx6duallite or mx6solo.

>
> It's kinda hard to infer that knowledge from the code
> and I wonder if this structure will hold up under future
> revisions.
>
>>   /*
>> - * MDCFG1, tRDC=8, tRP=8, tRC=27,tRAS=20,tRPA=tRP+1,tWR=8
>> - * tMRD=4, tCWL=6
>> + * MDCFG1,
>> + * MX6Q:
>> + *    tRDC=8, tRP=8, tRC=27, tRAS=20, tRPA=tRP+1, tWR=8, tMRD=4, tCWL=6
>> + * MX6DL/SOLO:
>> + *    tRDC=6, tRP=6, tRC=20, tRAS=15, tRPA=tRP+1, tWR=7, tMRD=4, tCWL=5
>>    */
>> -WRITE_ENTRY1(MMDC_P0_MDCFG1, 0xFF538E64)
>> +WRITE_ENTRY2(MMDC_P0_MDCFG1, 0xFF538E64, 0xB66E8C63)
>> +
>>   /*
>>    * MDCFG2,tDLLK=512,tRTP=4,tWTR=4,tRRD=4
>>    */
>>   WRITE_ENTRY1(MMDC_P0_MDCFG2, 0x01FF00DB)
>>   WRITE_ENTRY1(MMDC_P0_MDRWD, 0x000026D2)
>> -
>>   WRITE_ENTRY1(MMDC_P0_MDOR, 0x005B0E21)
>> -WRITE_ENTRY1(MMDC_P0_MDOTC, 0x09444040)
>> -WRITE_ENTRY1(MMDC_P0_MDPDC, 0x00025576)
>>
>>   /*
>> - * Mx6Q - 64 bit wide ddr
>> + * MMDC_MDOTC,
>> + * MX6Q:
>> + *    tAOFPD=2 cycles, tAONPD=2, tANPD=5, tAXPD=5, tODTLon=5, 
>> tODT_idle_off=5
>> + * MX6DL/SOLO:
>> + *    tAOFPD=1 cycles, tAONPD=1, tANPD=4, tAXPD=4, tODTLon=4, 
>> tODT_idle_off=4
>> + */
>> +WRITE_ENTRY2(MMDC_P0_MDOTC, 0x09444040, 0x00333030)
>> +
>> +/*
>> + * MDPDC - [17:16](2) =>  CKE pulse width = 3 cycles.
>> + * [15:12](5) =>  PWDT_1 = 256 cycles
>> + * [11:8](5) =>PWDR_0 = 256 cycles
>> + * MX6Q:       [2:0](6) =>  CKSRE = 6 cycles, [5:3](6) =>  CKSRX = 6 
>> cycles
>> + * MX6DL/SOLO: [2:0](5) =>  CKSRE = 5 cycles, [5:3](5) =>  CKSRX = 5 
>> cycles
>> + */
>> +WRITE_ENTRY2(MMDC_P0_MDPDC, 0x00025576, 0x0002556D)
>> +
>> +/*
>> + * MX6Q/DL - 64 bit wide ddr
>>    * last address is  (1<<28 (base) + 1<<30  - 1)  / (1<<25) =
>>    *     1<<3 + 1<<5 - 1 = 8 + 0x20 -1 = 0x27
>>    */
>> +/*
>> + * MX6SOLO - 32 bit wide ddr
>> + * last address is  (1<<28 (base) + 1<<29  - 1)  / (1<<25) =
>> + *    1<<3 + 1<<4 - 1 = 8 + 0x10 -1 = 0x17
>> + */
>>   /* MDASP, CS0_END */
>> -WRITE_ENTRY1(MMDC_P0_MDASP, 0x00000027)
>> +WRITE_ENTRY3(MMDC_P0_MDASP, 0x00000027, 0x00000027, 0x00000017)
>
> Is it unreasonable to think there's a use case for 32-bit wide
> memory on a dual or quad?

Sure, they just would not use this file.

>
> What happens when we populate 256Mx16 DDR chips?
>
> Sabre Auto is already doing this.

Again, definitely a new file would be needed.

>
>>   /*
>> - * MDCTL, CS0 enable, CS1 disabled, row=14, col=10, burst=8, 
>> width=64/32bit
>> - * mx6q   : row+col+bank+width=14+10+3+3=30 = 1G
>> + * MDCTL, CS0 enable, CS1 disabled, row=14, col=10, burst=8
>> + * MX6Q/DL: width=64bit row+col+bank+width=14+10+3+3=30 = 1G
>> + * MX6SOLO: width=32bit row+col+bank+width=14+10+3+2=29 = 512M
>>    */
>> -WRITE_ENTRY1(MMDC_P0_MDCTL, 0x831A0000)
>> +WRITE_ENTRY3(MMDC_P0_MDCTL, 0x831A0000, 0x831A0000, 0x83190000)
>>
>> -/* MDSCR, con_req, LOAD MR2, CS0, A3,A10 set (CAS Write=6), RZQ/2 */
>> -WRITE_ENTRY1(MMDC_P0_MDSCR, 0x04088032)
>> +/*
>> + * LOAD MR2: MDSCR, con_req,  CS0, A10 set - RZQ/2
>> + * MX6Q:    A3 set(CAS Write=6)
>> + * MX6DL/SOLO: (CAS Write=5)
>> + */
>> +WRITE_ENTRY2(MMDC_P0_MDSCR, 0x04088032, 0x04008032)
>>   /* LOAD MR3, CS0 */
>>   WRITE_ENTRY1(MMDC_P0_MDSCR, 0x00008033)
>> -/* LOAD MR1, CS0, A1,A6 set Rtt=RZQ/2, ODI=RZQ/7 */
>> -WRITE_ENTRY1(MMDC_P0_MDSCR, 0x00428031)
>> -/* LOAD MR0, CS0, A6,A8,A11 set CAS=8, WR=8, DLL reset */
>> -WRITE_ENTRY1(MMDC_P0_MDSCR, 0x09408030)
>> +
>> +/*
>> + * LOAD MR1, CS0
>> + * MX6Q: A6 set: Rtt=RZQ/2, A1 set: ODI=RZQ/7
>> + * MX6DL/SOLO: A2 set: Rtt=RZQ/4, ODI=RZQ/6
>> + */
>> +WRITE_ENTRY2(MMDC_P0_MDSCR, 0x00428031, 0x00048031)
>> +
>> +/* LOAD MR0, CS0 A8 set: DLL Reset
>> + * MX6Q: A6 set: CAS=8 A11 set: WR=8
>> + * MX6DL/SOLO: A4 set: CAS=5, A9,A10 set: WR=7
>> + */
>> +WRITE_ENTRY2(MMDC_P0_MDSCR, 0x09408030, 0x07208030)
>>
>>   /* ZQ calibrate, CS0 */
>>   WRITE_ENTRY1(MMDC_P0_MDSCR, 0x04008040)
>> @@ -173,18 +227,18 @@ WRITE_ENTRY1(MMDC_P0_MPODTCTRL, 0x00022227)
>>   WRITE_ENTRY1(MMDC_P1_MPODTCTRL, 0x00022227)
>>
>>   /* MPDGCTRL0/1 DQS GATE*/
>> -WRITE_ENTRY1(MMDC_P0_MPDGCTRL0, 0x434B0350)
>> -WRITE_ENTRY1(MMDC_P0_MPDGCTRL1, 0x034C0359)
>> -WRITE_ENTRY1(MMDC_P1_MPDGCTRL0, 0x434B0350)
>> -WRITE_ENTRY1(MMDC_P1_MPDGCTRL1, 0x03650348)
>> -WRITE_ENTRY1(MMDC_P0_MPRDDLCTL, 0x4436383B)
>> -WRITE_ENTRY1(MMDC_P1_MPRDDLCTL, 0x39393341)
>> -WRITE_ENTRY1(MMDC_P0_MPWRDLCTL, 0x35373933)
>> -WRITE_ENTRY1(MMDC_P1_MPWRDLCTL, 0x48254A36)
>> -WRITE_ENTRY1(MMDC_P0_MPWLDECTRL0, 0x001F001F)
>> -WRITE_ENTRY1(MMDC_P0_MPWLDECTRL1, 0x001F001F)
>> -WRITE_ENTRY1(MMDC_P1_MPWLDECTRL0, 0x00440044)
>> -WRITE_ENTRY1(MMDC_P1_MPWLDECTRL1, 0x00440044)
>> +WRITE_ENTRY2(MMDC_P0_MPDGCTRL0, 0x434B0350, 0x42350231)
>> +WRITE_ENTRY2(MMDC_P0_MPDGCTRL1, 0x034C0359, 0x021A0218)
>> +WRITE_ENTRY2(MMDC_P1_MPDGCTRL0, 0x434B0350, 0x42350231)
>> +WRITE_ENTRY2(MMDC_P1_MPDGCTRL1, 0x03650348, 0x021A0218)
>> +WRITE_ENTRY2(MMDC_P0_MPRDDLCTL, 0x4436383B, 0x4B4B4E49)
>> +WRITE_ENTRY2(MMDC_P1_MPRDDLCTL, 0x39393341, 0x4B4B4E49)
>> +WRITE_ENTRY2(MMDC_P0_MPWRDLCTL, 0x35373933, 0x3F3F3035)
>> +WRITE_ENTRY2(MMDC_P1_MPWRDLCTL, 0x48254A36, 0x3F3F3035)
>> +WRITE_ENTRY2(MMDC_P0_MPWLDECTRL0, 0x001F001F, 0x0040003C)
>> +WRITE_ENTRY2(MMDC_P0_MPWLDECTRL1, 0x001F001F, 0x0032003E)
>> +WRITE_ENTRY2(MMDC_P1_MPWLDECTRL0, 0x00440044, 0x0040003C)
>> +WRITE_ENTRY2(MMDC_P1_MPWLDECTRL1, 0x00440044, 0x0032003E)
>>
>>   /* MPMUR0 - Complete calibration by forced measurement */
>>   WRITE_ENTRY1(MMDC_P0_MPMUR0, 0x00000800)
>
> It appears that only the memory configuration registers use
> WRITE_ENTRY2 or WRITE_ENTRY3 macros, and if so, I'd much
> rather see three separate files expressing the values, i.e.:
>
>     mx6q_4x_mt41j128.cfg
>     mx6dl_4x_mt41j128.cfg
>     mx6s_2x_mt41j128.cfg

That is definitely a valid option. But you lose the common settings
of the iomux registers. Cut-n-pasting the files isn't so bad I guess,
but updates such as
     mx6q_4x_mt41j128.cfg: use ddr3 mode for reset

     Bits 19-18 of IOMUXC_IOMUXC_SW_PAD_CTL_PAD_DRAM_RESET
     should be 3 for DDR3 mode. The current value of 0 is
     reserved in TRM.

     Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>

diff --git a/board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg 
b/board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg
index b859e2f..9c622c8 100644
--- a/board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg
+++ b/board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg
@@ -72,7 +72,7 @@ WRITE_ENTRY1(IOM_DRAM_RAS, 0x00020030)
  WRITE_ENTRY1(IOM_DRAM_SDCLK_0, 0x00020030)
  WRITE_ENTRY1(IOM_DRAM_SDCLK_1, 0x00020030)

-WRITE_ENTRY1(IOM_DRAM_RESET, 0x00020030)
+WRITE_ENTRY1(IOM_DRAM_RESET, 0x000e0030)
  WRITE_ENTRY1(IOM_DRAM_SDCKE0, 0x00003000)
  WRITE_ENTRY1(IOM_DRAM_SDCKE1, 0x00003000)
  WRITE_ENTRY1(IOM_DRAM_SDBA2, 0x00000000)



Would then need to be applied to all three files.

>
> I'm not certain the processor part of the name is the
> right designator though. If I understand correctly,
> the differences in values between 6q and 6dl are mostly
> based on the memory speed.
>
> Is that right?

Correct...

>
> It also seems valid that a board would want to use an
> i.mx6quad with a lower memory bus speed.

Especially if there is a layout problem.

>
> If the address differences are taken care of by the
> preprocessor, it seems that a set like this would
> be more appropriate (and processor independent):
>
>     mx6_4x_mt41j128_1066.cfg
>     mx6_4x_mt41j128_800.cfg
>     mx6_2x_mt41j128_800.cfg

Right, this is definitely a valid option. And if different
boards calibrated their DDR to the same settings
(DQS and byte delays) it would make more sense.

But I still see this file as board specific and not so much
memory type specific.


>
> By splitting up the files, we can still check for
> differences between them using 'diff', and it will
> be easier to extend the set.
>
> Note that meaningful diffs will require the use of
> symbolic names rather than varying addresses for
> registers.
>
> It's also conceivable that calibration on a given
> layout will require per-board tweaks, but that's
> not clear at the moment.
>
> You've clearly been through this in more detail than
> I have. Please let me know your thoughts.
>
> Regards,
>
>
> Eric
>