Re: [RFC PATCH 1/2] cmd/fru: cmd/fru: move FRU handling support to common region

[Michal Simek <michal.simek@amd.com>]

Hi,

On 7/29/22 16:38, Jae Hyun Yoo wrote:
> Hello Michal,
> 
> On 7/29/2022 4:13 AM, Michal Simek wrote:
>> You should fix subject.
> 
> Ah, I'll remove one of 'cmd/fru:' prefix in the title.
> 
>> On 7/27/22 01:50, Jae Hyun Yoo wrote:
>>> From: Graeme Gregory <quic_ggregory@quicinc.com>
>>>
>>> The FRU handling was added as a Xilinx board dependent support but it
>>> would be useful for other boards too, so this commit moves the FRU
>>> handling support to the common region to be enabled by CONFIG_CMD_FRU.
>>> Since the Multirecord parsing logic should be implemented on each OEM
>>> board specifically, it defines 'fru_parse_multirec' as a weak function
>>> so that it can be replaced with the board specific implementation.
>>
>> Not entirely. Some multirecords are common and described by spec. But parising 
>> multirecord based on IANA ID is vendor specific.
>> It means boards shouldn't replicate code for hearder checking. Only handle 
>> that IANA specific part.
> 
> Right. I'll change the multirecords parsing logic to call the vendor
> specific parsing function only when record type is 0xc0 - 0xff. All
> other standard type parsing logic and header checking will be placed
> in the generic location.
> 
>>>
>>> Signed-off-by: Graeme Gregory <quic_ggregory@quicinc.com>
>>> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
>>> ---
>>>   board/xilinx/Kconfig                      |  8 ---
>>>   board/xilinx/common/Makefile              |  3 --
>>>   board/xilinx/common/board.c               | 63 +++++++++++++++++++----
>>>   cmd/Kconfig                               |  8 +++
>>>   cmd/Makefile                              |  1 +
>>>   {board/xilinx/common => cmd}/fru.c        |  3 +-
>>>   common/Makefile                           |  2 +
>>>   {board/xilinx/common => common}/fru_ops.c | 37 ++++++-------
>>>   {board/xilinx/common => include}/fru.h    | 15 +-----
>>>   9 files changed, 82 insertions(+), 58 deletions(-)
>>>   rename {board/xilinx/common => cmd}/fru.c (99%)
>>>   rename {board/xilinx/common => common}/fru_ops.c (93%)
>>>   rename {board/xilinx/common => include}/fru.h (85%)
>>
>> The main reason why I didn't added to generic location was that in board field 
>> there are xilinx specific custom fields.
>> With other vendor this won't work.
>> I think this should be solved before this code can be moved to generic location.
>>
>> Also maybe make sense to move and spec that variable creation.
> 
> Yes, I realized that the Xilinx specific customization was added into
> the standard board info area so actually it breaks the spec.
> 
> struct fru_board_data {
>          [....]
> 
>      /* Xilinx custom fields */
>      u8 rev_type_len;
>      u8 rev[FRU_BOARD_MAX_LEN];
>      u8 pcie_type_len;
>      u8 pcie[FRU_BOARD_MAX_LEN];
>      u8 uuid_type_len;
>      u8 uuid[FRU_BOARD_MAX_LEN];
> };
> 
> I think, this type of customization should be added using multirecords
> instead of expanding the common board info structure, and it's the
> reason why the FRU spec provides OEM multirecord types (0xc0 - 0xff).
> Do you have any plan to replace them with OEM multirecords?

Based on
https://www.intel.com/content/dam/www/public/us/en/documents/product-briefs/platform-management-fru-document-rev-1-2-feb-2013.pdf

Table 11-1 has

Additional custom Mfg. Info fields. Defined by manufacturing. Each
field must be preceded by a type/length byte

is clearly defined that it can be custom. Only that fields to FRU are defined by 
the spec.

That's why based on manufacturer name these fields can be defined. And because 
we did it for Xilinx only we didn't need to design it in a generic way. It means 
if there are some additional fields which is quite clear from sizes special 
board manufacturing functions can be called but code change is required.

Thanks,
Michal