On Feb 1, 2011, at 4:08 AM, Timur Tabi wrote: > On Mon, Jan 31, 2011 at 11:28 PM, Kumar Gala <ga...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot