Re: [Qemu-ppc] [Qemu-devel] [PATCH v3 1/6] smbus: Add a helper to generate SPD EEPROM data

[Philippe Mathieu-Daudé]

On 1/9/19 7:05 PM, BALATON Zoltan wrote:
> On Wed, 9 Jan 2019, BALATON Zoltan wrote:
>> On Wed, 9 Jan 2019, Philippe Mathieu-Daudé wrote:
>>> 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 <address@hidden>
>>>>>> ---
>>>>>> 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.
>>
>> No, I allocate 256 bytes for a 256 bytes EEPROM of which the first 128
>> bytes are containing SPD data as described in for example:
>>
>> https://en.wikipedia.org/wiki/Serial_presence_detect
>>
>> This also matches the previous code that allocated 256 bytes (and
>> still does, see smbus_eeprom_init() function just above this one).
>>
>>> 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);
>>
>> This does not match static SPD data currently in the code elsewhere
>> which all start with 128, 8,... so I'm not sure some firmware would
>> dislike a non-usual eeprom with 128, 4. My intention was to remove
>> static SPD data that's present elsewhere and replace it with nearly
>> identical data generated by this function. The data also has to match
>> what's normally found on real hardware so maybe try dumping data from
>> some memory modules and check what they have and if your suggestion is
>> common then we could go with that otherwise I'd rather waste 128 bytes
>> (or half a kilobyte for 4 modules) than get compatibility problems due
>> to presenting unusual data to firmwares and other guest software that
>> parse SPD data.
>>
>> Unless someone else also thinks it's a good idea to go with unusual
>> SPD data to save a few bytes.
> 
> Even then it would not work. Whole smbus_eeprom.c seems to have EEPROM
> size == 256 hardcoded all over the place, so...

Yes, this 'device' needs love^H^H^H^Hcleanup.

Thanks for the info you provided.

Regards,

Phil.