[Skiboot] [PATCH 3/7] hdat: handle ISDIMM SPD blobs

[Oliver O'Halloran]

On Fri, Jan 13, 2017 at 8:40 PM, Vasant Hegde
<hegdevasant at linux.vnet.ibm.com> wrote:
> On 01/13/2017 12:26 PM, Oliver O'Halloran wrote:
>>
>> Previously the HDAT format was only ever used with IBM hardware so it
>> would store vital product data (VPD) blobs in the IBM ASCII Keyword VPD
>> format. With P9 HDAT is used on OpenPower machines which use Industry
>> Standard DIMMs that provide their product data through a "Serial Present
>> Detect" EEPROM mounted on the DIMM.
>>
>> The SPD blob has a different format and is exported in the device-tree
>> under the "spd" property rather than the "ibm,vpd" property. This patch
>> adds support for recognising these blobs and placing them in the
>> appropriate DT property.
>>
>> Signed-off-by: Oliver O'Halloran <oohall at gmail.com>
>> ---
>>   hdata/memory.c | 37 ++++++++++++++++++++++++++++---------
>>   hdata/vpd.c    |  3 ++-
>>   2 files changed, 30 insertions(+), 10 deletions(-)
>>
>> diff --git a/hdata/memory.c b/hdata/memory.c
>> index 2b23aeaff03d..f95e67e2f2b8 100644
>> --- a/hdata/memory.c
>> +++ b/hdata/memory.c
>> @@ -269,6 +269,7 @@ static void vpd_add_ram_area(const struct
>> HDIF_common_hdr *msarea)
>>         const struct HDIF_ram_area_id *ram_id;
>>         struct dt_node *ram_node;
>>         u32 chip_id;
>> +       const void *vpd_blob;
>>
>>         ramptr = HDIF_child_arr(msarea, 0);
>>         if (!CHECK_SPPTR(ramptr)) {
>> @@ -285,15 +286,33 @@ static void vpd_add_ram_area(const struct
>> HDIF_common_hdr *msarea)
>>                 if (!CHECK_SPPTR(ram_id))
>>                         continue;
>>
>> -               if ((be16_to_cpu(ram_id->flags) & RAM_AREA_INSTALLED) &&
>> -                   (be16_to_cpu(ram_id->flags) & RAM_AREA_FUNCTIONAL)) {
>> -                       ram_node = dt_add_vpd_node(ramarea, 0, 1);
>> -                       if (ram_node) {
>> -                               chip_id = add_chip_id_to_ram_area(msarea,
>> -
>> ram_node);
>> -                               add_bus_freq_to_ram_area(ram_node,
>> chip_id);
>> -                               add_size_to_ram_area(ram_node, ramarea,
>> 1);
>> -                       }
>> +               /* Don't add VPD for non-existent RAM */
>> +               if (!(be16_to_cpu(ram_id->flags) & RAM_AREA_INSTALLED))
>
>
> Not sure why you removed RAM_AREA_FUNCTIONAL checking. I think we should
> retain that check.

Eh, I thought the original logic was kinda broken. If you have a DIMM
installed it makes sense to provide the VPD for it even if the DIMM is
non-functional. It might be a good idea to add another property to
indicate that the FRU is defective.

>
>> +                       continue;
>> +
>> +               ram_node = dt_add_vpd_node(ramarea, 0, 1);
>> +               if (!ram_node)
>> +                       continue;
>> +
>> +               chip_id = add_chip_id_to_ram_area(msarea, ram_node);
>> +               add_bus_freq_to_ram_area(ram_node, chip_id);
>> +
>> +               vpd_blob = HDIF_get_idata(ramarea, 1, &ram_sz);
>
>
> This works fine on FSP based system. I haven't got chance to play with
> hostboot generated HDAT. Hopefully above logic works on Open Power system.

I've successfully tested it on a Witherspoon and a Zaius. The original
motivation for this patch was the VPD parser spamming the skiboot log
with complaints about the VPD blobs being badly formatted since
they're not actually VPD blobs.

Oliver