On 1/9/19 1:15 PM, BALATON Zoltan wrote:
> On Wed, 9 Jan 2019, Philippe Mathieu-Daudé wrote:
>> On 1/3/19 5:27 PM, BALATON Zoltan wrote:
>>> There are several boards with SPD EEPROMs that are now using
>>> duplicated or slightly different hard coded data. Add a helper to
>>> generate SPD data for a memory module of given type and size that
>>> could be used by these boards (either as is or with further changes if
>>> needed) which should help cleaning this up and avoid further
>>> duplication.
>>>
>>> Signed-off-by: BALATON Zoltan <bala...@eik.bme.hu>
>>> ---
>>> v3: Fixed a tab indent
>>> v2: Added errp parameter to pass errors back to caller
>>>
>>> hw/i2c/smbus_eeprom.c | 130
>>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>> include/hw/i2c/smbus.h | 3 ++
>>> 2 files changed, 133 insertions(+)
>>>
>>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>>> index f18aa3de35..bef24a1ca4 100644
>>> --- a/hw/i2c/smbus_eeprom.c
>>> +++ b/hw/i2c/smbus_eeprom.c
> [...]
>>> +
>>> + spd = g_malloc0(256);
>>
>> I think this buffer is eeprom-dependant, not SPD related.
>
> This function is called spd_data_generate(). It specifically generates
> SPD EEPROM data and nothing else. as you see below data is hardcoded so
> would not work for other EEPROMs (the first two bytes even specify
> EEPROM size, hence I don't think size needs to be passed as a parameter.
Well this is why worried me at first, because you alloc 256 bytes ...
>
>> Wouldn't it be cleaner to pass the (previously created) buffer as
>> argument such:
>>
>> /* return true on success */
>> bool spd_data_fill(void *buf, size_t bufsize,
>> enum sdram_type type, ram_addr_t ram_size,
>> Error **errp);
>
> It could take a previously created buffer but it's simpler this way and
> one less error to handle by the caller so I don't like adding two more
> parameters for this.
>
>> or return something else like ssize_t.
>
> Again, the current version is simpler IMO so while this aims to be
> generic to be used by other boards but still not completely generic for
> all kinds of EEPROMs. Just for SPD EEPROMs commonly found on SDR, DDR
> and DDR2 memory modules. Anything else (even DDR3) is too dissimilar so
> those will need another function not this one.
>
> Regards,
> BALATON Zoltan
>
>>
>>> + spd[0] = 128; /* data bytes in EEPROM */
... for a 128 bytes EEPROM.
Maybe we can find a compromise at a quick fix with:
/* no other size currently supported */
static const size_t spd_eeprom_size = 128;
spd = g_malloc0(spd_eeprom_size);
...
spd[0] = spd_eeprom_size;
spd[1] = 1 + ctzl(spd_eeprom_size);
>>> + spd[1] = 8; /* log2 size of EEPROM */
>>> + spd[2] = type;
>>> + spd[3] = 13; /* row address bits */
>>> + spd[4] = 10; /* column address bits */
>>> + spd[5] = (type == DDR2 ? nbanks - 1 : nbanks);
>>> + spd[6] = 64; /* module data width */
>>> + /* reserved / data width high */
>>> + spd[8] = 4; /* interface voltage level */
>>> + spd[9] = 0x25; /* highest CAS latency */
>>> + spd[10] = 1; /* access time */
>>> + /* DIMM configuration 0 = non-ECC */
>>> + spd[12] = 0x82; /* refresh requirements */
>>> + spd[13] = 8; /* primary SDRAM width */
>>> + /* ECC SDRAM width */
>>> + spd[15] = (type == DDR2 ? 0 : 1); /* reserved / delay for random
>>> col rd */
>>> + spd[16] = 12; /* burst lengths supported */
>>> + spd[17] = 4; /* banks per SDRAM device */
>>> + spd[18] = 12; /* ~CAS latencies supported */
>>> + spd[19] = (type == DDR2 ? 0 : 1); /* reserved / ~CS latencies
>>> supported */
>>> + spd[20] = 2; /* DIMM type / ~WE latencies */
>>> + /* module features */
>>> + /* memory chip features */
>>> + spd[23] = 0x12; /* clock cycle time @ medium CAS latency */
>>> + /* data access time */
>>> + /* clock cycle time @ short CAS latency */
>>> + /* data access time */
>>> + spd[27] = 20; /* min. row precharge time */
>>> + spd[28] = 15; /* min. row active row delay */
>>> + spd[29] = 20; /* min. ~RAS to ~CAS delay */
>>> + spd[30] = 45; /* min. active to precharge time */
>>> + spd[31] = density;
>>> + spd[32] = 20; /* addr/cmd setup time */
>>> + spd[33] = 8; /* addr/cmd hold time */
>>> + spd[34] = 20; /* data input setup time */
>>> + spd[35] = 8; /* data input hold time */
>>> +
>>> + /* checksum */
>>> + for (i = 0; i < 63; i++) {
>>> + spd[63] += spd[i];
>>> + }
>>> + return spd;
>>> +}
>>> diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h
>>> index d8b1b9ee81..d3dd0fcb14 100644
>>> --- a/include/hw/i2c/smbus.h
>>> +++ b/include/hw/i2c/smbus.h
>>> @@ -93,4 +93,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t
>>> address, uint8_t *eeprom_buf);
>>> void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
>>> const uint8_t *eeprom_spd, int size);
>>>
>>> +enum sdram_type { SDR = 0x4, DDR = 0x7, DDR2 = 0x8 };
>>> +uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size,
>>> Error **errp);
>>> +
>>> #endif
>>>
>>
>>
