[U-Boot] [RFC] mpc83xx: add config options to spd_sdram

[U-Boot] [RFC] mpc83xx: add config options to spd_sdram

[Andre Schwarz]

Kim,

I have made some mods to spd_sdram.c for various reason:

1.
use SPD setup also for soldered RAM.
This allows DDR mounting options without U-Boot change because SPD data
is written during in-circuit/boundary-scan testing.

2.
read SPD data also from extended adressing EEPROMS used for e.g. HRCW 
storage.
Due to HRCW being being located at offset 0 we need an SPD data offset.

3.
for optimized signal integrity and power consumption we need more 
influence on
the on-die termination. Although the assumed default values are working they
are far from ideal.

4.
CPO values depend on internal bond wire length which has significantly
high variance on MPC837x, i.e. this value also should be board specific.


I have taken care not to increase code size and not to break existing 
boards.

Any comments from your side ?
Is this something you might ACK ?


Regards,
Andr?


diff --git a/arch/powerpc/cpu/mpc83xx/spd_sdram.c 
b/arch/powerpc/cpu/mpc83xx/spd_sdram.c
index 44aaa9a..7d10c8c 100644
--- a/arch/powerpc/cpu/mpc83xx/spd_sdram.c
+++ b/arch/powerpc/cpu/mpc83xx/spd_sdram.c
@@ -68,6 +68,12 @@ void board_add_ram_info(int use_default)
  #ifndef    CONFIG_SYS_READ_SPD
  #define CONFIG_SYS_READ_SPD    i2c_read
  #endif
+#ifndef SPD_EEPROM_OFFSET
+#define SPD_EEPROM_OFFSET    0
+#endif
+#ifndef SPD_EEPROM_ADDR_LEN
+#define SPD_EEPROM_ADDR_LEN     1
+#endif

  /*
   * Convert picoseconds into clock cycles (rounding up if needed).
@@ -160,7 +166,8 @@ long int spd_sdram()
      isync();

      /* Read SPD parameters with I2C */
-    CONFIG_SYS_READ_SPD(SPD_EEPROM_ADDRESS, 0, 1, (uchar *) & spd, 
sizeof (spd));
+    CONFIG_SYS_READ_SPD(SPD_EEPROM_ADDRESS, SPD_EEPROM_OFFSET,
+        SPD_EEPROM_ADDR_LEN, (uchar *) & spd, sizeof (spd));
  #ifdef SPD_DEBUG
      spd_debug(&spd);
  #endif
@@ -562,6 +569,9 @@ long int spd_sdram()
       * Empirically, 0x3 == 6/8 clock delay is suggested for DDR I 266.
       */
      wr_data_delay = 2;
+#ifdef CONFIG_SYS_DDR_WRITE_DATA_DELAY
+    wr_data_delay = CONFIG_SYS_DDR_WRITE_DATA_DELAY;
+#endif

      /*
       * Write Latency
@@ -601,6 +611,9 @@ long int spd_sdram()
       */
      cpo = 0;
      if (spd.mem_type == SPD_MEMTYPE_DDR2) {
+#ifdef CONFIG_SYS_DDR_CPO
+        cpo = CONFIG_SYS_DDR_CPO;
+#else
          if (effective_data_rate == 266) {
              cpo = 0x4;        /* READ_LAT + 1/2 */
          } else if (effective_data_rate == 333) {
@@ -611,6 +624,7 @@ long int spd_sdram()
              /* Automatic calibration */
              cpo = 0x1f;
          }
+#endif
      }

      ddr->timing_cfg_2 = (0
@@ -673,12 +687,23 @@ long int spd_sdram()
               * Bit 2 == 0x04 == 75 Ohm, with 2 DIMM modules.
               * Bit 6 == 0x40 == 150 Ohm, with 1 DIMM module.
               */
+#if defined(CONFIG_SYS_DDR_MODE_ODT_OFF)
+            mode_odt_enable = 0x00;         /* disabled */
+#elif defined(CONFIG_SYS_DDR_MODE_ODT_50)
+            mode_odt_enable = 0x44;         /*  50 Ohm */
+#elif defined(CONFIG_SYS_DDR_MODE_ODT_75)
+            mode_odt_enable = 0x04;         /*  75 Ohm */
+#else
              mode_odt_enable = 0x40;         /* 150 Ohm */
+#endif
          }

          ddr->sdram_mode =
              (0
               | (1 << (16 + 10))             /* DQS Differential disable */
+#ifdef CONFIG_SYS_DDR_MODE_WEAK
+             | (1 << (16 + 1))        /* weak driver (~60%) */
+#endif
               | (add_lat << (16 + 3))        /* Additive Latency in 
EMRS1 */
               | (mode_odt_enable << 16)      /* ODT Enable in EMRS1 */
               | ((twr_clk - 1) << 9)         /* Write Recovery Autopre */
@@ -740,6 +765,9 @@ long int spd_sdram()
          odt_cfg = 0x2;        /* ODT to IOs during reads */
      }
  #endif
+#ifdef CONFIG_ALWAYS_ASSERT_ODT_TO_CPU
+    odt_cfg = 0x3;        /* ODT always on */
+#endif
      if (spd.mem_type == SPD_MEMTYPE_DDR2) {
          ddr->sdram_cfg2 = (0
                  | (0 << 26)    /* True DQS */
@@ -779,10 +807,12 @@ long int spd_sdram()
          sdram_type = SDRAM_CFG_SDRAM_TYPE_DDR2;

      sdram_cfg = (0
-             | SDRAM_CFG_MEM_EN        /* DDR enable */
               | SDRAM_CFG_SREN        /* Self refresh */
               | sdram_type        /* SDRAM type */
               );
+#ifdef CONFIG_DDR_HALF_STRENGTH
+    sdram_cfg |= SDRAM_CFG_HSE;
+#endif

      /* sdram_cfg[3] = RD_EN - registered DIMM enable */
      if (spd.mod_attr & 0x02)
@@ -822,6 +852,7 @@ long int spd_sdram()
       */
      sdram_cfg |= SDRAM_CFG_2T_EN;
  #endif
+    sdram_cfg |= SDRAM_CFG_MEM_EN;
      /* Enable controller, and GO! */
      ddr->sdram_cfg = sdram_cfg;
      asm("sync;isync");

MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

[U-Boot] [RFC] mpc83xx: add config options to spd_sdram

[Kim Phillips]

On Tue, 5 Apr 2011 11:49:49 +0200
Andre Schwarz <andre.schwarz@matrix-vision.de> wrote:

Cc'ing York Sun, who knows a little more about this stuff than I do.

> I have made some mods to spd_sdram.c for various reason:
> 
> 1.
> use SPD setup also for soldered RAM.
> This allows DDR mounting options without U-Boot change because SPD data
> is written during in-circuit/boundary-scan testing.

not sure I understand this - board with soldered RAM can't physically
get the SPD data from RAM, yet SPD data were somehow acquired and
written into some ROM, so spd_sdram() is still needed to parse &
program the controller without requiring a new u-boot binary?

this isn't included in the below diff, right?

> 2.
> read SPD data also from extended adressing EEPROMS used for e.g. HRCW 
> storage.
> Due to HRCW being being located at offset 0 we need an SPD data offset.

and this is the SPD_EEPROM_OFFSET stuff below?  hmm..

so both 1 & 2 are coagulated in my mind - ideally, this would be
implemented with the new ddr code in
arch/powerpc/cpu/mpc8xxx/ddr/main.c - see fsl_ddr_compute(), which has
a GET_SPD step that would need to be either skipped (i.e, start_step =
COMPUTE_DIMM_PARMS), or changed to be able to be overridden, or,
ideally, all DIMM computations would be precomputed and stored in ROM
and a direct start_step to e.g., STEP_ASSIGN_ADDRESSES were made.

> 3.
> for optimized signal integrity and power consumption we need more 
> influence on
> the on-die termination. Although the assumed default values are working they
> are far from ideal.

board specific things like this are perfectly acceptable, of course.  

however, it should not be being done by glittering old-83xx/spd_sdram
with an extra #ifdef for every new parameter that is needed, especially
when this is all fixed in the new ddr code.  So, in preparation for
83xx to eventually migrate to the new ddr code, I'd like this to be
done in such a way so as to start to conform to the new code by at least
defining ODT, etc. parameters in a board-specific
ddr.c:fsl_ddr_board_options().  See board/freescale/{p1022ds,*}/ddr.c
for example.

> 4.
> CPO values depend on internal bond wire length which has significantly
> high variance on MPC837x, i.e. this value also should be board specific.

again, to be defined as popts->cpo_override in the board-specific
ddr.c:fsl_ddr_board_options().

> I have taken care not to increase code size and not to break existing 
> boards.
> 
> Any comments from your side ?
> Is this something you might ACK ?

not as-is - see above, and let me know what you think.

Kim

[U-Boot] [RFC] mpc83xx: add config options to spd_sdram

[York Sun]

On Tue, 2011-04-05 at 16:52 -0500, Kim Phillips wrote:
> On Tue, 5 Apr 2011 11:49:49 +0200
> Andre Schwarz <andre.schwarz@matrix-vision.de> wrote:
> 
> Cc'ing York Sun, who knows a little more about this stuff than I do.
> 
> > I have made some mods to spd_sdram.c for various reason:
> > 
> > 1.
> > use SPD setup also for soldered RAM.
> > This allows DDR mounting options without U-Boot change because SPD data
> > is written during in-circuit/boundary-scan testing.
> 
> not sure I understand this - board with soldered RAM can't physically
> get the SPD data from RAM, yet SPD data were somehow acquired and
> written into some ROM, so spd_sdram() is still needed to parse &
> program the controller without requiring a new u-boot binary?
> 
> this isn't included in the below diff, right?

Actually I have code for this already, pending for more broader testing.
I use raw timing data from datasheet of DDR chips. We have an internal
discussion to recommend using EEPROM to mimic SPD structure.

> 
> > 2.
> > read SPD data also from extended adressing EEPROMS used for e.g. HRCW 
> > storage.
> > Due to HRCW being being located at offset 0 we need an SPD data offset.
> 
> and this is the SPD_EEPROM_OFFSET stuff below?  hmm..
> 
> so both 1 & 2 are coagulated in my mind - ideally, this would be
> implemented with the new ddr code in
> arch/powerpc/cpu/mpc8xxx/ddr/main.c - see fsl_ddr_compute(), which has
> a GET_SPD step that would need to be either skipped (i.e, start_step =
> COMPUTE_DIMM_PARMS), or changed to be able to be overridden, or,
> ideally, all DIMM computations would be precomputed and stored in ROM
> and a direct start_step to e.g., STEP_ASSIGN_ADDRESSES were made.

I don't 100% agree with the idea to put SPD-like data in HRCW. How can
you be sure the data matches DDR? It would be equivalent to using fixed
raw timing data, wouldn't it?

> 
> > 3.
> > for optimized signal integrity and power consumption we need more 
> > influence on
> > the on-die termination. Although the assumed default values are working they
> > are far from ideal.
> 
> board specific things like this are perfectly acceptable, of course.  
> 
> however, it should not be being done by glittering old-83xx/spd_sdram
> with an extra #ifdef for every new parameter that is needed, especially
> when this is all fixed in the new ddr code.  So, in preparation for
> 83xx to eventually migrate to the new ddr code, I'd like this to be
> done in such a way so as to start to conform to the new code by at least
> defining ODT, etc. parameters in a board-specific
> ddr.c:fsl_ddr_board_options().  See board/freescale/{p1022ds,*}/ddr.c
> for example.

The ODT table is implemented in options.c and it can be override by
board functions. Look for the last function in options.c and  ddr.c
under the board folder.

> 
> > 4.
> > CPO values depend on internal bond wire length which has significantly
> > high variance on MPC837x, i.e. this value also should be board specific.
> 
> again, to be defined as popts->cpo_override in the board-specific
> ddr.c:fsl_ddr_board_options().

I recommend to use auto calibration. If for some reason, you want to
override, it belongs to board specific file, same as the above.

York

[U-Boot] [RFC] mpc83xx: add config options to spd_sdram

[Andre Schwarz]

Kim, York,

>> I have made some mods to spd_sdram.c for various reason:
>>
>> 1.
>> use SPD setup also for soldered RAM.
>> This allows DDR mounting options without U-Boot change because SPD data
>> is written during in-circuit/boundary-scan testing.
> not sure I understand this - board with soldered RAM can't physically
> get the SPD data from RAM, yet SPD data were somehow acquired and
> written into some ROM, so spd_sdram() is still needed to parse&
> program the controller without requiring a new u-boot binary?
SPD data is nothing more than an I2C-EEPROM soldered on each memory module
containing the physical details of the memory devices.
Since I already have an I2C-EEPROM on the board I can use it for SPD 
data, i.e.
the board's memory is configured using normal detection routines.

During production (exactly: board testing) the EEPROM will be programmed 
with an SPD
table matching the soldered memory devices. This gives some flexibility 
regarding size and
speed grades ... some devices have pretty short life-cycles.

> this isn't included in the below diff, right?
yes it is - all I need is to provide the EEPROM's I2C adress and the 
offset of the SPD table.
>> 2.
>> read SPD data also from extended adressing EEPROMS used for e.g. HRCW
>> storage.
>> Due to HRCW being being located at offset 0 we need an SPD data offset.
> and this is the SPD_EEPROM_OFFSET stuff below?  hmm..
yes - since the start of EEPROM memory is already occupied by HRCW.

> so both 1&  2 are coagulated in my mind - ideally, this would be
> implemented with the new ddr code in
> arch/powerpc/cpu/mpc8xxx/ddr/main.c - see fsl_ddr_compute(), which has
> a GET_SPD step that would need to be either skipped (i.e, start_step =
> COMPUTE_DIMM_PARMS), or changed to be able to be overridden, or,
> ideally, all DIMM computations would be precomputed and stored in ROM
> and a direct start_step to e.g., STEP_ASSIGN_ADDRESSES were made.

Currently the spd_sdram is doing *exactly* what I need.
 From my point of view there's no need to move to new code.

>> 3.
>> for optimized signal integrity and power consumption we need more
>> influence on
>> the on-die termination. Although the assumed default values are working they
>> are far from ideal.
> board specific things like this are perfectly acceptable, of course.
ok.
> however, it should not be being done by glittering old-83xx/spd_sdram
> with an extra #ifdef for every new parameter

ok - what about this :

#if !defined(CONFIG_SYS_DDR_MODE_ODT_VALUE)
#define CONFIG_SYS_DDR_MODE_ODT_VALUE 0x40
#endif

mode_odt_enable = CONFIG_SYS_DDR_MODE_ODT_VALUE;


I really can't think of this change being a problem for anybody.

>   that is needed, especially
> when this is all fixed in the new ddr code.  So, in preparation for
> 83xx to eventually migrate to the new ddr code,
understood - but the effort needed to dig through the new code and
get it up and running is hardly justified by the sole need to set a 
proper ODT value.

I did not even come to speak of an increased memory footprint.
I'll have to provide a patch these days for a 8343 based board removing 
features
due binary size constantly growing ...

>   I'd like this to be
> done in such a way so as to start to conform to the new code by at least
> defining ODT, etc. parameters in a board-specific
> ddr.c:fsl_ddr_board_options().  See board/freescale/{p1022ds,*}/ddr.c
> for example.
ok - understood.
I'm willing to do it but not all at once.

>> 4.
>> CPO values depend on internal bond wire length which has significantly
>> high variance on MPC837x, i.e. this value also should be board specific.
> again, to be defined as popts->cpo_override in the board-specific
> ddr.c:fsl_ddr_board_options().
ok.
>> I have taken care not to increase code size and not to break existing
>> boards.
>>
>> Any comments from your side ?
>> Is this something you might ACK ?
> not as-is - see above, and let me know what you think.

I fully understand that you want the new code to be used and I'll use it 
asap.
But right now the spd_sdram is working fine and I need this flexibility 
using
the local EEPROM.

IMHO this patch is not breaking anything and completely transparent.
Not a single byte of code increase either ....




Regards,
Andr?


MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

[U-Boot] [RFC] mpc83xx: add config options to spd_sdram

[Andre Schwarz]

York,
>>> I have made some mods to spd_sdram.c for various reason:
>>>
>>> 1.
>>> use SPD setup also for soldered RAM.
>>> This allows DDR mounting options without U-Boot change because SPD data
>>> is written during in-circuit/boundary-scan testing.
>> not sure I understand this - board with soldered RAM can't physically
>> get the SPD data from RAM, yet SPD data were somehow acquired and
>> written into some ROM, so spd_sdram() is still needed to parse&
>> program the controller without requiring a new u-boot binary?
>>
>> this isn't included in the below diff, right?
> Actually I have code for this already, pending for more broader testing.
> I use raw timing data from datasheet of DDR chips. We have an internal
> discussion to recommend using EEPROM to mimic SPD structure.
sorry - I don't understand this.
Why do you want to create something new if there's a standard
like SPD layout ? It is working perfectly fine and the data can be
obtained from the manufacturer without further modifications.

>>> 2.
>>> read SPD data also from extended adressing EEPROMS used for e.g. HRCW
>>> storage.
>>> Due to HRCW being being located at offset 0 we need an SPD data offset.
>> and this is the SPD_EEPROM_OFFSET stuff below?  hmm..
>>
>> so both 1&  2 are coagulated in my mind - ideally, this would be
>> implemented with the new ddr code in
>> arch/powerpc/cpu/mpc8xxx/ddr/main.c - see fsl_ddr_compute(), which has
>> a GET_SPD step that would need to be either skipped (i.e, start_step =
>> COMPUTE_DIMM_PARMS), or changed to be able to be overridden, or,
>> ideally, all DIMM computations would be precomputed and stored in ROM
>> and a direct start_step to e.g., STEP_ASSIGN_ADDRESSES were made.
> I don't 100% agree with the idea to put SPD-like data in HRCW.
The SPD data has nothing to do with HRCW - it's just stored in the same 
PROM.
>   How can you be sure the data matches DDR?
Because the SPD data gets programmed during production.
We can use any SPD table *exactly* matching the soldered parts.
> It would be equivalent to using fixed raw timing data, wouldn't it?
no - it isn't.
I want the *same* U-Boot binary running on *different* memory 
configurations, i.e.
*no* fixed timing at all.

>>> 3.
>>> for optimized signal integrity and power consumption we need more
>>> influence on
>>> the on-die termination. Although the assumed default values are working they
>>> are far from ideal.
>> board specific things like this are perfectly acceptable, of course.
>>
>> however, it should not be being done by glittering old-83xx/spd_sdram
>> with an extra #ifdef for every new parameter that is needed, especially
>> when this is all fixed in the new ddr code.  So, in preparation for
>> 83xx to eventually migrate to the new ddr code, I'd like this to be
>> done in such a way so as to start to conform to the new code by at least
>> defining ODT, etc. parameters in a board-specific
>> ddr.c:fsl_ddr_board_options().  See board/freescale/{p1022ds,*}/ddr.c
>> for example.
> The ODT table is implemented in options.c and it can be override by
> board functions. Look for the last function in options.c and  ddr.c
> under the board folder.

To me it is a fundamental design error to rely on environment data to set
up the hardware correctly. Especially something that critical as memory
configuration.

Do you expect U-Boot to work with a corrupted environment, i.e. hwconfig
value unavailable ?

Of course this might never happen on a reference design board - but 
that's not real life.

>>> 4.
>>> CPO values depend on internal bond wire length which has significantly
>>> high variance on MPC837x, i.e. this value also should be board specific.
>> again, to be defined as popts->cpo_override in the board-specific
>> ddr.c:fsl_ddr_board_options().
> I recommend to use auto calibration. If for some reason, you want to
> override, it belongs to board specific file, same as the above.
>
no - auto calibration is far from ideal regarding emc and power consumption.


Regards,
Andr?

MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

[U-Boot] [RFC] mpc83xx: add config options to spd_sdram

[York Sun]

On Wed, 2011-04-06 at 10:42 +0200, Andre Schwarz wrote:
> York,
> >>> I have made some mods to spd_sdram.c for various reason:
> >>>
> >>> 1.
> >>> use SPD setup also for soldered RAM.
> >>> This allows DDR mounting options without U-Boot change because SPD data
> >>> is written during in-circuit/boundary-scan testing.
> >> not sure I understand this - board with soldered RAM can't physically
> >> get the SPD data from RAM, yet SPD data were somehow acquired and
> >> written into some ROM, so spd_sdram() is still needed to parse&
> >> program the controller without requiring a new u-boot binary?
> >>
> >> this isn't included in the below diff, right?
> > Actually I have code for this already, pending for more broader testing.
> > I use raw timing data from datasheet of DDR chips. We have an internal
> > discussion to recommend using EEPROM to mimic SPD structure.
> sorry - I don't understand this.
> Why do you want to create something new if there's a standard
> like SPD layout ? It is working perfectly fine and the data can be
> obtained from the manufacturer without further modifications.


I am not creating anything new. For some platforms with soldered DDR
components, there is no SPD on board. We need to either create the
SPD-like structure, or use the raw timing data from datasheet.

> 
> >>> 2.
> >>> read SPD data also from extended adressing EEPROMS used for e.g. HRCW
> >>> storage.
> >>> Due to HRCW being being located at offset 0 we need an SPD data offset.
> >> and this is the SPD_EEPROM_OFFSET stuff below?  hmm..
> >>
> >> so both 1&  2 are coagulated in my mind - ideally, this would be
> >> implemented with the new ddr code in
> >> arch/powerpc/cpu/mpc8xxx/ddr/main.c - see fsl_ddr_compute(), which has
> >> a GET_SPD step that would need to be either skipped (i.e, start_step =
> >> COMPUTE_DIMM_PARMS), or changed to be able to be overridden, or,
> >> ideally, all DIMM computations would be precomputed and stored in ROM
> >> and a direct start_step to e.g., STEP_ASSIGN_ADDRESSES were made.
> > I don't 100% agree with the idea to put SPD-like data in HRCW.
> The SPD data has nothing to do with HRCW - it's just stored in the same 
> PROM.
> >   How can you be sure the data matches DDR?
> Because the SPD data gets programmed during production.
> We can use any SPD table *exactly* matching the soldered parts.

Agreed as far as the SPD data is not easily erased by mistake.

> > It would be equivalent to using fixed raw timing data, wouldn't it?
> no - it isn't.
> I want the *same* U-Boot binary running on *different* memory 
> configurations, i.e.
> *no* fixed timing at all.

As far as the SPD data is persistent.

> 
> >>> 3.
> >>> for optimized signal integrity and power consumption we need more
> >>> influence on
> >>> the on-die termination. Although the assumed default values are working they
> >>> are far from ideal.
> >> board specific things like this are perfectly acceptable, of course.
> >>
> >> however, it should not be being done by glittering old-83xx/spd_sdram
> >> with an extra #ifdef for every new parameter that is needed, especially
> >> when this is all fixed in the new ddr code.  So, in preparation for
> >> 83xx to eventually migrate to the new ddr code, I'd like this to be
> >> done in such a way so as to start to conform to the new code by at least
> >> defining ODT, etc. parameters in a board-specific
> >> ddr.c:fsl_ddr_board_options().  See board/freescale/{p1022ds,*}/ddr.c
> >> for example.
> > The ODT table is implemented in options.c and it can be override by
> > board functions. Look for the last function in options.c and  ddr.c
> > under the board folder.
> 
> To me it is a fundamental design error to rely on environment data to set
> up the hardware correctly. Especially something that critical as memory
> configuration.
> 
> Do you expect U-Boot to work with a corrupted environment, i.e. hwconfig
> value unavailable ?
> 
> Of course this might never happen on a reference design board - but 
> that's not real life.

It is not in environmental variable.

> 
> >>> 4.
> >>> CPO values depend on internal bond wire length which has significantly
> >>> high variance on MPC837x, i.e. this value also should be board specific.
> >> again, to be defined as popts->cpo_override in the board-specific
> >> ddr.c:fsl_ddr_board_options().
> > I recommend to use auto calibration. If for some reason, you want to
> > override, it belongs to board specific file, same as the above.
> >
> no - auto calibration is far from ideal regarding emc and power consumption.
> 

Then just use whatever parameter fits. Again, it is in the board ddr
file.


York

[U-Boot] [RFC] mpc83xx: add config options to spd_sdram

[York Sun]

On Wed, 2011-04-06 at 10:18 +0200, Andre Schwarz wrote:
> Kim, York,
> 
> >> I have made some mods to spd_sdram.c for various reason:
> >>
> >> 1.
> >> use SPD setup also for soldered RAM.
> >> This allows DDR mounting options without U-Boot change because SPD data
> >> is written during in-circuit/boundary-scan testing.
> > not sure I understand this - board with soldered RAM can't physically
> > get the SPD data from RAM, yet SPD data were somehow acquired and
> > written into some ROM, so spd_sdram() is still needed to parse&
> > program the controller without requiring a new u-boot binary?
> SPD data is nothing more than an I2C-EEPROM soldered on each memory module
> containing the physical details of the memory devices.
> Since I already have an I2C-EEPROM on the board I can use it for SPD 
> data, i.e.
> the board's memory is configured using normal detection routines.
> 
> During production (exactly: board testing) the EEPROM will be programmed 
> with an SPD
> table matching the soldered memory devices. This gives some flexibility 
> regarding size and
> speed grades ... some devices have pretty short life-cycles.

That's right. If you have the SPD in I2C-EEPROM, you have a real SPD as
far as software concerns. Just provide the I2C address.

> >> 3.
> >> for optimized signal integrity and power consumption we need more
> >> influence on
> >> the on-die termination. Although the assumed default values are working they
> >> are far from ideal.
> > board specific things like this are perfectly acceptable, of course.
> ok.
> > however, it should not be being done by glittering old-83xx/spd_sdram
> > with an extra #ifdef for every new parameter
> 
> ok - what about this :
> 
> #if !defined(CONFIG_SYS_DDR_MODE_ODT_VALUE)
> #define CONFIG_SYS_DDR_MODE_ODT_VALUE 0x40
> #endif
> 
> mode_odt_enable = CONFIG_SYS_DDR_MODE_ODT_VALUE;
> 
> 
> I really can't think of this change being a problem for anybody.
> 

I would suggest to put it in fsl_ddr_board_options() in ddr.c under the
board directory. There you can override any options, including 

popts->cs_local_opts[i].odt_rd_cfg
popts->cs_local_opts[i].odt_wr_cfg
popts->cpo_override



York

[U-Boot] [RFC] mpc83xx: add config options to spd_sdram

[Schwarz, Andre]

York,
?
ok - will give it a try tomorrow.
?
Cheers,
Andr?
?

York Sun <yorksun@freescale.com> hat am 6. April 2011 um 18:37 geschrieben:

> On Wed, 2011-04-06 at 10:18 +0200, Andre Schwarz wrote:
> > Kim, York,
> >
> > >> I have made some mods to spd_sdram.c for various reason:
> > >>
> > >> 1.
> > >> use SPD setup also for soldered RAM.
> > >> This allows DDR mounting options without U-Boot change because SPD data
> > >> is written during in-circuit/boundary-scan testing.
> > > not sure I understand this - board with soldered RAM can't physically
> > > get the SPD data from RAM, yet SPD data were somehow acquired and
> > > written into some ROM, so spd_sdram() is still needed to parse&
> > > program the controller without requiring a new u-boot binary?
> > SPD data is nothing more than an I2C-EEPROM soldered on each memory module
> > containing the physical details of the memory devices.
> > Since I already have an I2C-EEPROM on the board I can use it for SPD
> > data, i.e.
> > the board's memory is configured using normal detection routines.
> >
> > During production (exactly: board testing) the EEPROM will be programmed
> > with an SPD
> > table matching the soldered memory devices. This gives some flexibility
> > regarding size and
> > speed grades ... some devices have pretty short life-cycles.
>
> That's right. If you have the SPD in I2C-EEPROM, you have a real SPD as
> far as software concerns. Just provide the I2C address.
>
> > >> 3.
> > >> for optimized signal integrity and power consumption we need more
> > >> influence on
> > >> the on-die termination. Although the assumed default values are working
> > >> they
> > >> are far from ideal.
> > > board specific things like this are perfectly acceptable, of course.
> > ok.
> > > however, it should not be being done by glittering old-83xx/spd_sdram
> > > with an extra #ifdef for every new parameter
> >
> > ok - what about this :
> >
> > #if !defined(CONFIG_SYS_DDR_MODE_ODT_VALUE)
> > #define CONFIG_SYS_DDR_MODE_ODT_VALUE 0x40
> > #endif
> >
> > mode_odt_enable = CONFIG_SYS_DDR_MODE_ODT_VALUE;
> >
> >
> > I really can't think of this change being a problem for anybody.
> >
>
> I would suggest to put it in fsl_ddr_board_options() in ddr.c under the
> board directory. There you can override any options, including
>
> popts->cs_local_opts[i].odt_rd_cfg
> popts->cs_local_opts[i].odt_wr_cfg
> popts->cpo_override
>
>
>
> York
>
>
>--
Gru?,
Andr? Schwarz

MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

[U-Boot] [RFC] mpc83xx: add config options to spd_sdram

[Schwarz, Andre]

York,
?


"Schwarz,Andre" <andre.schwarz@matrix-vision.de> hat am 6. April 2011 um 20:42
geschrieben:


> York,
> ?
> ok - will give it a try tomorrow.
> ?
> 
?
hmm - having a look at the Makefile it looks like I need CONFIG_FSL_DDR2.
This seems to pull in the "new" code ... without omitting the "old" one?
in arch/powerpc/cpu/mpc83xx/spd_sdram.c
?
The Makefile further uses ctrl_regs.c ... which fails.
?
having a look arch/powerpc/cpu/mpc8xxx/ddr/ctrl_regs.c?gives :
?
#ifdef CONFIG_MPC85xx
??#define _DDR_ADDR CONFIG_SYS_MPC85xx_DDR_ADDR
#elif defined(CONFIG_MPC86xx)
??#define _DDR_ADDR CONFIG_SYS_MPC86xx_DDR_ADDR
#else?? ? ? ?
??#error "Undefined _DDR_ADDR"
#endif?
?
There's not a single sign of any 83xx within this code.
Grepping through the board configs only show 85xx and 86xx based boards using
it.
?
Sorry, but I'm feeling like an idiot.
?
Are you playing some game with me or am I simply unable to understand the code ?
?
Please shed some light on this.
?
Regards,
Andr?
?


> ?
> 
> York Sun <yorksun@freescale.com> hat am 6. April 2011 um 18:37 geschrieben:
> 
> > On Wed, 2011-04-06 at 10:18 +0200, Andre Schwarz wrote:
> > > Kim, York,
> > >
> > > >> I have made some mods to spd_sdram.c for various reason:
> > > >>
> > > >> 1.
> > > >> use SPD setup also for soldered RAM.
> > > >> This allows DDR mounting options without U-Boot change because SPD data
> > > >> is written during in-circuit/boundary-scan testing.
> > > > not sure I understand this - board with soldered RAM can't physically
> > > > get the SPD data from RAM, yet SPD data were somehow acquired and
> > > > written into some ROM, so spd_sdram() is still needed to parse&
> > > > program the controller without requiring a new u-boot binary?
> > > SPD data is nothing more than an I2C-EEPROM soldered on each memory module
> > > containing the physical details of the memory devices.
> > > Since I already have an I2C-EEPROM on the board I can use it for SPD
> > > data, i.e.
> > > the board's memory is configured using normal detection routines.
> > >
> > > During production (exactly: board testing) the EEPROM will be programmed
> > > with an SPD
> > > table matching the soldered memory devices. This gives some flexibility
> > > regarding size and
> > > speed grades ... some devices have pretty short life-cycles.
> >
> > That's right. If you have the SPD in I2C-EEPROM, you have a real SPD as
> > far as software concerns. Just provide the I2C address.
> >
> > > >> 3.
> > > >> for optimized signal integrity and power consumption we need more
> > > >> influence on
> > > >> the on-die termination. Although the assumed default values are working
> > > >> they
> > > >> are far from ideal.
> > > > board specific things like this are perfectly acceptable, of course.
> > > ok.
> > > > however, it should not be being done by glittering old-83xx/spd_sdram
> > > > with an extra #ifdef for every new parameter
> > >
> > > ok - what about this :
> > >
> > > #if !defined(CONFIG_SYS_DDR_MODE_ODT_VALUE)
> > > #define CONFIG_SYS_DDR_MODE_ODT_VALUE 0x40
> > > #endif
> > >
> > > mode_odt_enable = CONFIG_SYS_DDR_MODE_ODT_VALUE;
> > >
> > >
> > > I really can't think of this change being a problem for anybody.
> > >
> >
> > I would suggest to put it in fsl_ddr_board_options() in ddr.c under the
> > board directory. There you can override any options, including
> >
> > popts->cs_local_opts[i].odt_rd_cfg
> > popts->cs_local_opts[i].odt_wr_cfg
> > popts->cpo_override
> >
> >
> >
> > York
> >
> >
> >--
> Gru?,
> Andr? Schwarz
>

MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

[U-Boot] [RFC] mpc83xx: add config options to spd_sdram

[York Sun]

On Thu, 2011-04-07 at 22:42 +0200, Schwarz,Andre wrote:
> York,
> 
>  
> 
> 
> "Schwarz,Andre" <andre.schwarz@matrix-vision.de> hat am 6. April 2011
> um 20:42 geschrieben:
> 
> > York,
> > 
> >  
> > 
> > ok - will give it a try tomorrow.
> > 
> >  
> > 
>  
> 
> hmm - having a look at the Makefile it looks like I need
> CONFIG_FSL_DDR2.
> 
> This seems to pull in the "new" code ... without omitting the "old"
> one 
> 
> in arch/powerpc/cpu/mpc83xx/spd_sdram.c
> 
>  
> 
> The Makefile further uses ctrl_regs.c ... which fails.
> 
>  
> 
> having a look arch/powerpc/cpu/mpc8xxx/ddr/ctrl_regs.c gives :
> 
>  
> 
> #ifdef CONFIG_MPC85xx
> 
>   #define _DDR_ADDR CONFIG_SYS_MPC85xx_DDR_ADDR
> 
> #elif defined(CONFIG_MPC86xx)
> 
>   #define _DDR_ADDR CONFIG_SYS_MPC86xx_DDR_ADDR
> 
> #else        
> 
>   #error "Undefined _DDR_ADDR"
> 
> #endif 
> 
>  
> 
> There's not a single sign of any 83xx within this code.
> 
> Grepping through the board configs only show 85xx and 86xx based
> boards using it.
> 
>  
> 
> Sorry, but I'm feeling like an idiot.
> 
>  
> 
> Are you playing some game with me or am I simply unable to understand
> the code ?
> 
>  
> 
> Please shed some light on this.
> 
>  
Andre,

I am sorry I totally ignored the subject with mpc83xx. I was thinking of
mpc85xx.

You are right the old code is in spd_sdram.c and it is still in use for
mpc83xx. Unless to adjust the code, there is no board specific file.

York

[U-Boot] [RFC] mpc83xx: add config options to spd_sdram

[Andre Schwarz]

Kim,


>>> York,
>>>
>>>
>>>
>>> ok - will give it a try tomorrow.
>>>
>>>
>>>
>>
>>
>> hmm - having a look at the Makefile it looks like I need
>> CONFIG_FSL_DDR2.
>>
>> This seems to pull in the "new" code ... without omitting the "old"
>> one
>>
>> in arch/powerpc/cpu/mpc83xx/spd_sdram.c
>>
>>
>>
>> The Makefile further uses ctrl_regs.c ... which fails.
>>
>>
>>
>> having a look arch/powerpc/cpu/mpc8xxx/ddr/ctrl_regs.c gives :
>>
>>
>>
>> #ifdef CONFIG_MPC85xx
>>
>>    #define _DDR_ADDR CONFIG_SYS_MPC85xx_DDR_ADDR
>>
>> #elif defined(CONFIG_MPC86xx)
>>
>>    #define _DDR_ADDR CONFIG_SYS_MPC86xx_DDR_ADDR
>>
>> #else
>>
>>    #error "Undefined _DDR_ADDR"
>>
>> #endif
>>
>>
>>
>> There's not a single sign of any 83xx within this code.
>>
>> Grepping through the board configs only show 85xx and 86xx based
>> boards using it.
>>
>>
>>
>> Sorry, but I'm feeling like an idiot.
>>
>>
>>
>> Are you playing some game with me or am I simply unable to understand
>> the code ?
>>
>>
>>
>> Please shed some light on this.
>>
>>
> Andre,
>
> I am sorry I totally ignored the subject with mpc83xx. I was thinking of
> mpc85xx.
ok - no "new" code for 83xx then.
> You are right the old code is in spd_sdram.c and it is still in use for
> mpc83xx. Unless to adjust the code, there is no board specific file.
>
Are you willing to accept some changes to spd_sdram.c or will I have 
another out-of-tree board  ?

What we need is :

- more configurable settings like driver strength, cpo and odt values.
- Reading SPD data from an arbitrary i2c eeprom.

What's so bad about my proposal ?
Please let me know so that I'm able to improve.


Regards,
Andr?

MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

[U-Boot] [RFC] mpc83xx: add config options to spd_sdram

[York Sun]

On Fri, 2011-04-08 at 09:41 +0200, Andre Schwarz wrote:
> > I am sorry I totally ignored the subject with mpc83xx. I was thinking of
> > mpc85xx.
> ok - no "new" code for 83xx then.
> > You are right the old code is in spd_sdram.c and it is still in use for
> > mpc83xx. Unless to adjust the code, there is no board specific file.
> >
> Are you willing to accept some changes to spd_sdram.c or will I have 
> another out-of-tree board  ?
> 
> What we need is :
> 
> - more configurable settings like driver strength, cpo and odt values.
> - Reading SPD data from an arbitrary i2c eeprom.
> 
> What's so bad about my proposal ?
> Please let me know so that I'm able to improve.
> 

I welcome the change to use the new code since the feature you asked is
already there.

York

[U-Boot] [RFC] mpc83xx: add config options to spd_sdram

[Andre Schwarz]

York,
> On Fri, 2011-04-08 at 09:41 +0200, Andre Schwarz wrote:
>>> I am sorry I totally ignored the subject with mpc83xx. I was thinking of
>>> mpc85xx.
>> ok - no "new" code for 83xx then.
>>> You are right the old code is in spd_sdram.c and it is still in use for
>>> mpc83xx. Unless to adjust the code, there is no board specific file.
>>>
>> Are you willing to accept some changes to spd_sdram.c or will I have
>> another out-of-tree board  ?
>>
>> What we need is :
>>
>> - more configurable settings like driver strength, cpo and odt values.
>> - Reading SPD data from an arbitrary i2c eeprom.
>>
>> What's so bad about my proposal ?
>> Please let me know so that I'm able to improve.
>>
> I welcome the change to use the new code since the feature you asked is
> already there.
sorry, but I'm not sure if I understand this correctly.

What I need is some minor non-intrusive changes to 83xx-specific file 
spd_sdram.c because it is the most straightforward solution with *no* 
impact on code size or other existing boards. So: The feature I ask for 
is *not* already there.

What I definitely won't do is use the "new" fsl ddr code in mpc8xxx/ddr 
because it completely lacks 83xx support.
So: The feature I ask for is there but my CPU family is not supported.
IMHO the "new" code is total overkill for 83xx.



Cheers,
Andr?

MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

[U-Boot] [RFC] mpc83xx: add config options to spd_sdram

[York Sun]

On Fri, 2011-04-08 at 18:29 +0200, Andre Schwarz wrote:
> York,
> > On Fri, 2011-04-08 at 09:41 +0200, Andre Schwarz wrote:
> >>> I am sorry I totally ignored the subject with mpc83xx. I was thinking of
> >>> mpc85xx.
> >> ok - no "new" code for 83xx then.
> >>> You are right the old code is in spd_sdram.c and it is still in use for
> >>> mpc83xx. Unless to adjust the code, there is no board specific file.
> >>>
> >> Are you willing to accept some changes to spd_sdram.c or will I have
> >> another out-of-tree board  ?
> >>
> >> What we need is :
> >>
> >> - more configurable settings like driver strength, cpo and odt values.
> >> - Reading SPD data from an arbitrary i2c eeprom.
> >>
> >> What's so bad about my proposal ?
> >> Please let me know so that I'm able to improve.
> >>
> > I welcome the change to use the new code since the feature you asked is
> > already there.
> sorry, but I'm not sure if I understand this correctly.
> 
> What I need is some minor non-intrusive changes to 83xx-specific file 
> spd_sdram.c because it is the most straightforward solution with *no* 
> impact on code size or other existing boards. So: The feature I ask for 
> is *not* already there.
> 
> What I definitely won't do is use the "new" fsl ddr code in mpc8xxx/ddr 
> because it completely lacks 83xx support.
> So: The feature I ask for is there but my CPU family is not supported.
> IMHO the "new" code is total overkill for 83xx.
> 

It would be easier to maintain one set of code. But if the change you
need is relatively small to the old code and doesn't have negative
impact on other boards, go ahead to post the patch.

York

[U-Boot] [RFC] mpc83xx: add config options to spd_sdram

[Andre Schwarz]

On 04/08/2011 06:36 PM, York Sun wrote:
> On Fri, 2011-04-08 at 18:29 +0200, Andre Schwarz wrote:
>> York,
>>> On Fri, 2011-04-08 at 09:41 +0200, Andre Schwarz wrote:
>>>>> I am sorry I totally ignored the subject with mpc83xx. I was thinking of
>>>>> mpc85xx.
>>>> ok - no "new" code for 83xx then.
>>>>> You are right the old code is in spd_sdram.c and it is still in use for
>>>>> mpc83xx. Unless to adjust the code, there is no board specific file.
>>>>>
>>>> Are you willing to accept some changes to spd_sdram.c or will I have
>>>> another out-of-tree board  ?
>>>>
>>>> What we need is :
>>>>
>>>> - more configurable settings like driver strength, cpo and odt values.
>>>> - Reading SPD data from an arbitrary i2c eeprom.
>>>>
>>>> What's so bad about my proposal ?
>>>> Please let me know so that I'm able to improve.
>>>>
>>> I welcome the change to use the new code since the feature you asked is
>>> already there.
>> sorry, but I'm not sure if I understand this correctly.
>>
>> What I need is some minor non-intrusive changes to 83xx-specific file
>> spd_sdram.c because it is the most straightforward solution with *no*
>> impact on code size or other existing boards. So: The feature I ask for
>> is *not* already there.
>>
>> What I definitely won't do is use the "new" fsl ddr code in mpc8xxx/ddr
>> because it completely lacks 83xx support.
>> So: The feature I ask for is there but my CPU family is not supported.
>> IMHO the "new" code is total overkill for 83xx.
>>
> It would be easier to maintain one set of code. But if the change you
> need is relatively small to the old code and doesn't have negative
> impact on other boards, go ahead to post the patch.
yes - that's what I've been asking for.
So far Kim's NAK is still valid - let's see what his favorite solution 
will look like.

Thank you for sharing your thoughts.


Regards,
Andr?

MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner