[U-Boot] mx6: spl: Rename ncs as ranks and move it to mx6_ddr3_cfg

[U-Boot] mx6: spl: Rename ncs as ranks and move it to mx6_ddr3_cfg

[Nikolay Dimitrov]

Hi guys,

I'm working on adding SO-DIMM SPL support on a custom imx6 board, so I'm 
thinking on the idea of which DDR3 settings belong to the controller and 
which belong to the DDR3 memory module/chips.

My proposal is to rename the struct member "ncs" to "ranks" (as per 
JEDEC) and to move it as part of the DDR3 module description (be it an 
external so-dimm module or on-board DDR ICs), and not part of the MMDC 
description. This way the "ranks" value can be loaded from the SO-DIMM 
SPD or hard-coded as in Tim's case for the Ventana board.

The code for my board is out of the official tree, so I'm showing my 
idea on Tim's Ventana SPL code:


diff --git a/arch/arm/cpu/armv7/mx6/ddr.c b/arch/arm/cpu/armv7/mx6/ddr.c
index 0434211..0d3580b 100644
--- a/arch/arm/cpu/armv7/mx6/ddr.c
+++ b/arch/arm/cpu/armv7/mx6/ddr.c
@@ -340,7 +340,7 @@ void mx6_dram_cfg(const struct mx6_ddr_sysinfo *i,
  	debug("trrd=%d\n", trrd);
  	debug("txpr=%d\n", txpr);
  	debug("CS0_END=%d\n", CS0_END);
-	debug("ncs=%d\n", i->ncs);
+	debug("ranks=%d\n", m->ranks);
  	debug("Rtt_wr=%d\n", i->rtt_wr);
  	debug("Rtt_nom=%d\n", i->rtt_nom);
  	debug("SRT=%d\n", m->SRT);
@@ -437,11 +437,11 @@ void mx6_dram_cfg(const struct mx6_ddr_sysinfo *i,
  	/* Step 7: Enable MMDC with desired chip select */
  	reg = mmdc0->mdctl |
  	      (1 << 31) |			/* SDE_0 for CS0 */
-	      ((i->ncs == 2) ? 1 : 0) << 30;	/* SDE_1 for CS1 */
+	      ((m->ranks == 2) ? 1 : 0) << 30;	/* SDE_1 for CS1 */
  	mmdc0->mdctl = reg;

  	/* Step 8: Write Mode Registers to Init DDR3 devices */
-	for (cs = 0; cs < i->ncs; cs++) {
+	for (cs = 0; cs < m->ranks; cs++) {
  		/* MR2 */
  		reg = (i->rtt_wr & 3) << 9 | (m->SRT & 1) << 7 |
  		      ((tcwl - 3) & 3) << 3;
diff --git a/arch/arm/include/asm/arch-mx6/mx6-ddr.h 
b/arch/arm/include/asm/arch-mx6/mx6-ddr.h
index 5ebabfa..2b9649c 100644
--- a/arch/arm/include/asm/arch-mx6/mx6-ddr.h
+++ b/arch/arm/include/asm/arch-mx6/mx6-ddr.h
@@ -191,13 +191,13 @@ struct mx6_ddr3_cfg {
  	u16 trcmin;	/* tRC min (ns*100) */
  	u16 trasmin;	/* tRAS min (ns*100) */
  	u8 SRT;		/* self-refresh temperature: 0=normal, 1=extended */
+	u8 ranks;	/* ranks (chip-selects) of the DDR3 memory (1,2) */
  };

  /* System Information: Varies per board design, layout, and term 
choices */
  struct mx6_ddr_sysinfo {
  	u8 dsize;	/* size of bus (in dwords: 0=16bit,1=32bit,2=64bit) */
  	u8 cs_density;	/* density per chip select (Gb) */
-	u8 ncs;		/* number chip selects used (1|2) */
  	char cs1_mirror;/* enable address mirror (0|1) */
  	char bi_on;	/* Bank interleaving enable */
  	u8 rtt_nom;	/* Rtt_Nom (DDR3_RTT_*) */
diff --git a/board/gateworks/gw_ventana/gw_ventana_spl.c 
b/board/gateworks/gw_ventana/gw_ventana_spl.c
index e943879..93be270 100644
--- a/board/gateworks/gw_ventana/gw_ventana_spl.c
+++ b/board/gateworks/gw_ventana/gw_ventana_spl.c
@@ -199,6 +199,7 @@ static struct mx6_ddr3_cfg mt41k128m16jt_125 = {
  	.trcd = 1375,
  	.trcmin = 4875,
  	.trasmin = 3500,
+	.ranks = 1
  };

  /* GW54xx specific calibration */
@@ -309,8 +310,6 @@ static void spl_dram_init(int width, int size, int 
board_model)
  		.dsize = width/32,
  		/* config for full 4GB range so that get_mem_size() works */
  		.cs_density = 32, /* 32Gb per CS */
-		/* single chip select */
-		.ncs = 1,
  		.cs1_mirror = 0,
  		.rtt_wr = 1 /*DDR3_RTT_60_OHM*/,	/* RTT_Wr = RZQ/4 */
  #ifdef RTT_NOM_120OHM


Please excuse me if this way to show ideas is not the appropriate one, 
just tell me how to do it better and I'll do my best.

Kind regards,
Nikolay

[U-Boot] mx6: spl: Rename ncs as ranks and move it to mx6_ddr3_cfg

[Tim Harvey]

On Sat, Aug 9, 2014 at 7:08 AM, Nikolay Dimitrov <picmaster@mail.bg> wrote:
> Hi guys,
>
> I'm working on adding SO-DIMM SPL support on a custom imx6 board, so I'm
> thinking on the idea of which DDR3 settings belong to the controller and
> which belong to the DDR3 memory module/chips.
>
> My proposal is to rename the struct member "ncs" to "ranks" (as per JEDEC)
> and to move it as part of the DDR3 module description (be it an external
> so-dimm module or on-board DDR ICs), and not part of the MMDC description.
> This way the "ranks" value can be loaded from the SO-DIMM SPD or hard-coded
> as in Tim's case for the Ventana board.
>
> The code for my board is out of the official tree, so I'm showing my idea on
> Tim's Ventana SPL code:
>
>
> diff --git a/arch/arm/cpu/armv7/mx6/ddr.c b/arch/arm/cpu/armv7/mx6/ddr.c
> index 0434211..0d3580b 100644
> --- a/arch/arm/cpu/armv7/mx6/ddr.c
> +++ b/arch/arm/cpu/armv7/mx6/ddr.c
> @@ -340,7 +340,7 @@ void mx6_dram_cfg(const struct mx6_ddr_sysinfo *i,
>         debug("trrd=%d\n", trrd);
>         debug("txpr=%d\n", txpr);
>         debug("CS0_END=%d\n", CS0_END);
> -       debug("ncs=%d\n", i->ncs);
> +       debug("ranks=%d\n", m->ranks);
>         debug("Rtt_wr=%d\n", i->rtt_wr);
>         debug("Rtt_nom=%d\n", i->rtt_nom);
>         debug("SRT=%d\n", m->SRT);
> @@ -437,11 +437,11 @@ void mx6_dram_cfg(const struct mx6_ddr_sysinfo *i,
>         /* Step 7: Enable MMDC with desired chip select */
>         reg = mmdc0->mdctl |
>               (1 << 31) |                       /* SDE_0 for CS0 */
> -             ((i->ncs == 2) ? 1 : 0) << 30;    /* SDE_1 for CS1 */
> +             ((m->ranks == 2) ? 1 : 0) << 30;  /* SDE_1 for CS1 */
>         mmdc0->mdctl = reg;
>
>         /* Step 8: Write Mode Registers to Init DDR3 devices */
> -       for (cs = 0; cs < i->ncs; cs++) {
> +       for (cs = 0; cs < m->ranks; cs++) {
>                 /* MR2 */
>                 reg = (i->rtt_wr & 3) << 9 | (m->SRT & 1) << 7 |
>                       ((tcwl - 3) & 3) << 3;
> diff --git a/arch/arm/include/asm/arch-mx6/mx6-ddr.h
> b/arch/arm/include/asm/arch-mx6/mx6-ddr.h
> index 5ebabfa..2b9649c 100644
> --- a/arch/arm/include/asm/arch-mx6/mx6-ddr.h
> +++ b/arch/arm/include/asm/arch-mx6/mx6-ddr.h
> @@ -191,13 +191,13 @@ struct mx6_ddr3_cfg {
>         u16 trcmin;     /* tRC min (ns*100) */
>         u16 trasmin;    /* tRAS min (ns*100) */
>         u8 SRT;         /* self-refresh temperature: 0=normal, 1=extended */
> +       u8 ranks;       /* ranks (chip-selects) of the DDR3 memory (1,2) */
>  };
>
>  /* System Information: Varies per board design, layout, and term choices */
>  struct mx6_ddr_sysinfo {
>         u8 dsize;       /* size of bus (in dwords: 0=16bit,1=32bit,2=64bit)
> */
>         u8 cs_density;  /* density per chip select (Gb) */
> -       u8 ncs;         /* number chip selects used (1|2) */
>         char cs1_mirror;/* enable address mirror (0|1) */
>         char bi_on;     /* Bank interleaving enable */
>         u8 rtt_nom;     /* Rtt_Nom (DDR3_RTT_*) */
> diff --git a/board/gateworks/gw_ventana/gw_ventana_spl.c
> b/board/gateworks/gw_ventana/gw_ventana_spl.c
> index e943879..93be270 100644
> --- a/board/gateworks/gw_ventana/gw_ventana_spl.c
> +++ b/board/gateworks/gw_ventana/gw_ventana_spl.c
> @@ -199,6 +199,7 @@ static struct mx6_ddr3_cfg mt41k128m16jt_125 = {
>         .trcd = 1375,
>         .trcmin = 4875,
>         .trasmin = 3500,
> +       .ranks = 1
>  };
>
>  /* GW54xx specific calibration */
> @@ -309,8 +310,6 @@ static void spl_dram_init(int width, int size, int
> board_model)
>                 .dsize = width/32,
>                 /* config for full 4GB range so that get_mem_size() works */
>                 .cs_density = 32, /* 32Gb per CS */
> -               /* single chip select */
> -               .ncs = 1,
>                 .cs1_mirror = 0,
>                 .rtt_wr = 1 /*DDR3_RTT_60_OHM*/,        /* RTT_Wr = RZQ/4 */
>  #ifdef RTT_NOM_120OHM
>
>
> Please excuse me if this way to show ideas is not the appropriate one, just
> tell me how to do it better and I'll do my best.
>
> Kind regards,
> Nikolay

Hi Nikolay,

The ncs field is a 'system' property detailing how many chip selects
your board supports. I suppose in your case you are saying you have a
SO-DIMM that you route both chip selects to and the SO-DIMM SPD tells
you whether it uses 1 or 2 of them.

Nikita may have comments on this as he's using board configurations
with multiple chip selects - I haven't needed that myself yet (but
will soon) so I've cc'd him.

Tim

[U-Boot] mx6: spl: Rename ncs as ranks and move it to mx6_ddr3_cfg

[Nikolay Dimitrov]

Hi Tim,

On 08/10/2014 10:47 AM, Tim Harvey wrote:
> On Sat, Aug 9, 2014 at 7:08 AM, Nikolay Dimitrov <picmaster@mail.bg> wrote:
>> Hi guys,
>>
>> I'm working on adding SO-DIMM SPL support on a custom imx6 board, so I'm
>> thinking on the idea of which DDR3 settings belong to the controller and
>> which belong to the DDR3 memory module/chips.
>>
>> My proposal is to rename the struct member "ncs" to "ranks" (as per JEDEC)
>> and to move it as part of the DDR3 module description (be it an external
>> so-dimm module or on-board DDR ICs), and not part of the MMDC description.
>> This way the "ranks" value can be loaded from the SO-DIMM SPD or hard-coded
>> as in Tim's case for the Ventana board.
>>
>> The code for my board is out of the official tree, so I'm showing my idea on
>> Tim's Ventana SPL code:
>>
>>
>> diff --git a/arch/arm/cpu/armv7/mx6/ddr.c b/arch/arm/cpu/armv7/mx6/ddr.c
>> index 0434211..0d3580b 100644
>> --- a/arch/arm/cpu/armv7/mx6/ddr.c
>> +++ b/arch/arm/cpu/armv7/mx6/ddr.c
>> @@ -340,7 +340,7 @@ void mx6_dram_cfg(const struct mx6_ddr_sysinfo *i,
>>          debug("trrd=%d\n", trrd);
>>          debug("txpr=%d\n", txpr);
>>          debug("CS0_END=%d\n", CS0_END);
>> -       debug("ncs=%d\n", i->ncs);
>> +       debug("ranks=%d\n", m->ranks);
>>          debug("Rtt_wr=%d\n", i->rtt_wr);
>>          debug("Rtt_nom=%d\n", i->rtt_nom);
>>          debug("SRT=%d\n", m->SRT);
>> @@ -437,11 +437,11 @@ void mx6_dram_cfg(const struct mx6_ddr_sysinfo *i,
>>          /* Step 7: Enable MMDC with desired chip select */
>>          reg = mmdc0->mdctl |
>>                (1 << 31) |                       /* SDE_0 for CS0 */
>> -             ((i->ncs == 2) ? 1 : 0) << 30;    /* SDE_1 for CS1 */
>> +             ((m->ranks == 2) ? 1 : 0) << 30;  /* SDE_1 for CS1 */
>>          mmdc0->mdctl = reg;
>>
>>          /* Step 8: Write Mode Registers to Init DDR3 devices */
>> -       for (cs = 0; cs < i->ncs; cs++) {
>> +       for (cs = 0; cs < m->ranks; cs++) {
>>                  /* MR2 */
>>                  reg = (i->rtt_wr & 3) << 9 | (m->SRT & 1) << 7 |
>>                        ((tcwl - 3) & 3) << 3;
>> diff --git a/arch/arm/include/asm/arch-mx6/mx6-ddr.h
>> b/arch/arm/include/asm/arch-mx6/mx6-ddr.h
>> index 5ebabfa..2b9649c 100644
>> --- a/arch/arm/include/asm/arch-mx6/mx6-ddr.h
>> +++ b/arch/arm/include/asm/arch-mx6/mx6-ddr.h
>> @@ -191,13 +191,13 @@ struct mx6_ddr3_cfg {
>>          u16 trcmin;     /* tRC min (ns*100) */
>>          u16 trasmin;    /* tRAS min (ns*100) */
>>          u8 SRT;         /* self-refresh temperature: 0=normal, 1=extended */
>> +       u8 ranks;       /* ranks (chip-selects) of the DDR3 memory (1,2) */
>>   };
>>
>>   /* System Information: Varies per board design, layout, and term choices */
>>   struct mx6_ddr_sysinfo {
>>          u8 dsize;       /* size of bus (in dwords: 0=16bit,1=32bit,2=64bit)
>> */
>>          u8 cs_density;  /* density per chip select (Gb) */
>> -       u8 ncs;         /* number chip selects used (1|2) */
>>          char cs1_mirror;/* enable address mirror (0|1) */
>>          char bi_on;     /* Bank interleaving enable */
>>          u8 rtt_nom;     /* Rtt_Nom (DDR3_RTT_*) */
>> diff --git a/board/gateworks/gw_ventana/gw_ventana_spl.c
>> b/board/gateworks/gw_ventana/gw_ventana_spl.c
>> index e943879..93be270 100644
>> --- a/board/gateworks/gw_ventana/gw_ventana_spl.c
>> +++ b/board/gateworks/gw_ventana/gw_ventana_spl.c
>> @@ -199,6 +199,7 @@ static struct mx6_ddr3_cfg mt41k128m16jt_125 = {
>>          .trcd = 1375,
>>          .trcmin = 4875,
>>          .trasmin = 3500,
>> +       .ranks = 1
>>   };
>>
>>   /* GW54xx specific calibration */
>> @@ -309,8 +310,6 @@ static void spl_dram_init(int width, int size, int
>> board_model)
>>                  .dsize = width/32,
>>                  /* config for full 4GB range so that get_mem_size() works */
>>                  .cs_density = 32, /* 32Gb per CS */
>> -               /* single chip select */
>> -               .ncs = 1,
>>                  .cs1_mirror = 0,
>>                  .rtt_wr = 1 /*DDR3_RTT_60_OHM*/,        /* RTT_Wr = RZQ/4 */
>>   #ifdef RTT_NOM_120OHM
>>
>>
>> Please excuse me if this way to show ideas is not the appropriate one, just
>> tell me how to do it better and I'll do my best.
>>
>> Kind regards,
>> Nikolay
>
> Hi Nikolay,
>
> The ncs field is a 'system' property detailing how many chip selects
> your board supports. I suppose in your case you are saying you have a
> SO-DIMM that you route both chip selects to and the SO-DIMM SPD tells
> you whether it uses 1 or 2 of them.
Yes, my board is routed to support all the required SO-DIMM signals, 
including the 2x CS signals.

Actually, thinking more about your explanation, it starts to look like 
keeping "ncs" is actually a good idea. But in the case where the board 
supports 2x CS but the plugged SO-DIMM memory supports only 1x CS 
(single rank), then it also makes sense to have the additional "ranks" 
struct member, so we can describe the capabilities of both memory and 
MMDC, and configure MMDC accordingly.

> Nikita may have comments on this as he's using board configurations
> with multiple chip selects - I haven't needed that myself yet (but
> will soon) so I've cc'd him.
I'm definitely looking for input from more knowledgeable people than me.

Kind regards,
Nikolay