[U-Boot] [PATCH 2/2 v2] powerpc/8xxx: Refactor fsl_ddr_get_spd into common code from board

[Kumar Gala]

On Feb 1, 2011, at 4:08 AM, Timur Tabi wrote:

> On Mon, Jan 31, 2011 at 11:28 PM, Kumar Gala <galak at kernel.crashing.org> wrote:
>> +#if defined(SPD_EEPROM_ADDRESS) || \
>> +    defined(SPD_EEPROM_ADDRESS1) || defined(SPD_EEPROM_ADDRESS2) || \
>> +    defined(SPD_EEPROM_ADDRESS3) || defined(SPD_EEPROM_ADDRESS4)
>> +#if (CONFIG_NUM_DDR_CONTROLLERS == 1) && (CONFIG_DIMM_SLOTS_PER_CTLR == 1)
>> +u8 spd_i2c_addr[CONFIG_NUM_DDR_CONTROLLERS][CONFIG_DIMM_SLOTS_PER_CTLR] = {
>> +       [0][0] = SPD_EEPROM_ADDRESS,
>> +};
>> +#endif
>> +#if (CONFIG_NUM_DDR_CONTROLLERS == 2) && (CONFIG_DIMM_SLOTS_PER_CTLR == 1)
>> +u8 spd_i2c_addr[CONFIG_NUM_DDR_CONTROLLERS][CONFIG_DIMM_SLOTS_PER_CTLR] = {
>> +       [0][0] = SPD_EEPROM_ADDRESS1,   /* controller 1 */
>> +       [1][0] = SPD_EEPROM_ADDRESS2,   /* controller 2 */
>> +};
>> +#endif
>> +#if (CONFIG_NUM_DDR_CONTROLLERS == 2) && (CONFIG_DIMM_SLOTS_PER_CTLR == 2)
>> +u8 spd_i2c_addr[CONFIG_NUM_DDR_CONTROLLERS][CONFIG_DIMM_SLOTS_PER_CTLR] = {
>> +       [0][0] = SPD_EEPROM_ADDRESS1,   /* controller 1 */
>> +       [0][1] = SPD_EEPROM_ADDRESS2,   /* controller 1 */
>> +       [1][0] = SPD_EEPROM_ADDRESS3,   /* controller 2 */
>> +       [1][1] = SPD_EEPROM_ADDRESS4,   /* controller 2 */
>> +};
>> +#endif
> 
> Wouldn't it be easier if we just temporarily defined values for
> SPD_EEPROM_ADDRESSx?  Like this:
> 
> #ifndef SPD_EEPROM_ADDRESS2
> #define SPD_EEPROM_ADDRESS2 0
> #endif
> ...
> 
> u8 spd_i2c_addr[2][2] = {

This is problematic because because if you notice SPD_EEPROM_ADDRESS2 is used for both controller 2 / dimm 1 & controller 1 / dimm 2.

>> +static void __get_spd(generic_spd_eeprom_t *spd, u8 i2c_address)
>> +{
>> +       int ret = i2c_read(i2c_address, 0, 1, (uchar *)spd,
>> +                               sizeof(generic_spd_eeprom_t));
>> +
>> +       if (ret) {
>> +               debug("DDR: failed to read SPD from address %u\n", i2c_address);
> 
> This should probably be a printf() instead.

will change

- k