Re: [PATCH v3 20/20] efx: support pio mapping based on cxl

[Alejandro Lucero Palau <alucerop@amd.com>]

On 9/13/24 18:45, Edward Cree wrote:
> On 07/09/2024 09:18, alejandro.lucero-palau@amd.com wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> With a device supporting CXL and successfully initialised, use the cxl
>> region to map the memory range and use this mapping for PIO buffers.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> <snip>> diff --git a/drivers/net/ethernet/sfc/mcdi_pcol.h b/drivers/net/ethernet/sfc/mcdi_pcol.h
>> index cd297e19cddc..c158a1e8d01b 100644
>> --- a/drivers/net/ethernet/sfc/mcdi_pcol.h
>> +++ b/drivers/net/ethernet/sfc/mcdi_pcol.h
>> @@ -16799,6 +16799,9 @@
>>   #define        MC_CMD_GET_CAPABILITIES_V7_OUT_DYNAMIC_MPORT_JOURNAL_OFST 148
>>   #define        MC_CMD_GET_CAPABILITIES_V7_OUT_DYNAMIC_MPORT_JOURNAL_LBN 14
>>   #define        MC_CMD_GET_CAPABILITIES_V7_OUT_DYNAMIC_MPORT_JOURNAL_WIDTH 1
>> +#define        MC_CMD_GET_CAPABILITIES_V7_OUT_CXL_CONFIG_ENABLE_OFST 148
>> +#define        MC_CMD_GET_CAPABILITIES_V7_OUT_CXL_CONFIG_ENABLE_LBN 17
>> +#define        MC_CMD_GET_CAPABILITIES_V7_OUT_CXL_CONFIG_ENABLE_WIDTH 1
>>   
>>   /* MC_CMD_GET_CAPABILITIES_V8_OUT msgresponse */
>>   #define    MC_CMD_GET_CAPABILITIES_V8_OUT_LEN 160
>> @@ -17303,6 +17306,9 @@
>>   #define        MC_CMD_GET_CAPABILITIES_V8_OUT_DYNAMIC_MPORT_JOURNAL_OFST 148
>>   #define        MC_CMD_GET_CAPABILITIES_V8_OUT_DYNAMIC_MPORT_JOURNAL_LBN 14
>>   #define        MC_CMD_GET_CAPABILITIES_V8_OUT_DYNAMIC_MPORT_JOURNAL_WIDTH 1
>> +#define        MC_CMD_GET_CAPABILITIES_V8_OUT_CXL_CONFIG_ENABLE_OFST 148
>> +#define        MC_CMD_GET_CAPABILITIES_V8_OUT_CXL_CONFIG_ENABLE_LBN 17
>> +#define        MC_CMD_GET_CAPABILITIES_V8_OUT_CXL_CONFIG_ENABLE_WIDTH 1
>>   /* These bits are reserved for communicating test-specific capabilities to
>>    * host-side test software. All production drivers should treat this field as
>>    * opaque.
>> @@ -17821,6 +17827,9 @@
>>   #define        MC_CMD_GET_CAPABILITIES_V9_OUT_DYNAMIC_MPORT_JOURNAL_OFST 148
>>   #define        MC_CMD_GET_CAPABILITIES_V9_OUT_DYNAMIC_MPORT_JOURNAL_LBN 14
>>   #define        MC_CMD_GET_CAPABILITIES_V9_OUT_DYNAMIC_MPORT_JOURNAL_WIDTH 1
>> +#define        MC_CMD_GET_CAPABILITIES_V9_OUT_CXL_CONFIG_ENABLE_OFST 148
>> +#define        MC_CMD_GET_CAPABILITIES_V9_OUT_CXL_CONFIG_ENABLE_LBN 17
>> +#define        MC_CMD_GET_CAPABILITIES_V9_OUT_CXL_CONFIG_ENABLE_WIDTH 1
>>   /* These bits are reserved for communicating test-specific capabilities to
>>    * host-side test software. All production drivers should treat this field as
>>    * opaque.
>> @@ -18374,6 +18383,9 @@
>>   #define        MC_CMD_GET_CAPABILITIES_V10_OUT_DYNAMIC_MPORT_JOURNAL_OFST 148
>>   #define        MC_CMD_GET_CAPABILITIES_V10_OUT_DYNAMIC_MPORT_JOURNAL_LBN 14
>>   #define        MC_CMD_GET_CAPABILITIES_V10_OUT_DYNAMIC_MPORT_JOURNAL_WIDTH 1
>> +#define        MC_CMD_GET_CAPABILITIES_V10_OUT_CXL_CONFIG_ENABLE_OFST 148
>> +#define        MC_CMD_GET_CAPABILITIES_V10_OUT_CXL_CONFIG_ENABLE_LBN 17
>> +#define        MC_CMD_GET_CAPABILITIES_V10_OUT_CXL_CONFIG_ENABLE_WIDTH 1
>>   /* These bits are reserved for communicating test-specific capabilities to
>>    * host-side test software. All production drivers should treat this field as
>>    * opaque.


Hi Ed,

It is really kind of you to comment here, specifically this kind of comment.


> Please, do not make targeted edits to mcdi_pcol.h.  Our standard
>   process for this file is to regenerate the whole thing from
>   smartnic_registry whenever we want to pick up new additions;
>   this helps us have certainty and traceability that the driver
>   and firmware-side definitions of the MCDI protocol are consistent
>   and limit the risk of copy/paste errors etc. introducing
>   differences that could cause backwards-compatibility headaches
>   later.  Ideally the MCDI update should also be a separate commit.


Thank you for telling me ... I already knew it.

FWIW, I have followed the process. I guess you did not see it in our 
internal driver repo, but it is in the firmware repo which is the one 
that matters here.

You know there are some mcdi_pcol.h definitions not upstreamed, and 
therefore it is not possible to just submit the new generated file 
upstream but only those bits needed in a patch. I wonder how copy & 
paste can be avoided here. Is there a tool we have unbeknownst to me?


BTW, it is almost an standard process those daily team meetings you have 
been missing "lately" ... we could have avoided this public exchange ...


> See previous commits to this file, such as
>      1f17708b47a9 ("sfc: update MCDI protocol headers")
>   for examples of how this should look.
>
> -ed