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

[Jae Hyun Yoo <quic_jaehyoo@quicinc.com>]

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?

Thanks,
Jae