Re: [PATCH v6 28/28] sfc: support pio mapping based on cxl

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

On 12/3/24 14:52, Martin Habets wrote:
> Hi Alejandro,
>
> On Mon, Dec 02, 2024 at 05:12:22PM +0000, 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>
>> ---
>>   drivers/net/ethernet/sfc/ef10.c       | 49 +++++++++++++++++++++++----
>>   drivers/net/ethernet/sfc/efx_cxl.c    | 19 ++++++++++-
>>   drivers/net/ethernet/sfc/net_driver.h |  2 ++
>>   drivers/net/ethernet/sfc/nic.h        |  3 ++
>>   4 files changed, 66 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
>> index 452009ed7a43..f2aeffc323c6 100644
>> --- a/drivers/net/ethernet/sfc/ef10.c
>> +++ b/drivers/net/ethernet/sfc/ef10.c
>> @@ -24,6 +24,7 @@
>>   #include <linux/wait.h>
>>   #include <linux/workqueue.h>
>>   #include <net/udp_tunnel.h>
>> +#include "efx_cxl.h"
>>   
>>   /* Hardware control for EF10 architecture including 'Huntington'. */
>>   
>> @@ -177,6 +178,12 @@ static int efx_ef10_init_datapath_caps(struct efx_nic *efx)
>>   			  efx->num_mac_stats);
>>   	}
>>   
>> +	if (outlen < MC_CMD_GET_CAPABILITIES_V7_OUT_LEN)
>> +		nic_data->datapath_caps3 = 0;
>> +	else
>> +		nic_data->datapath_caps3 = MCDI_DWORD(outbuf,
>> +						      GET_CAPABILITIES_V7_OUT_FLAGS3);
>> +
>>   	return 0;
>>   }
>>   
>> @@ -919,6 +926,9 @@ static void efx_ef10_forget_old_piobufs(struct efx_nic *efx)
>>   static void efx_ef10_remove(struct efx_nic *efx)
>>   {
>>   	struct efx_ef10_nic_data *nic_data = efx->nic_data;
>> +#ifdef CONFIG_SFC_CXL
>> +	struct efx_probe_data *probe_data;
>> +#endif
>>   	int rc;
>>   
>>   #ifdef CONFIG_SFC_SRIOV
>> @@ -949,7 +959,12 @@ static void efx_ef10_remove(struct efx_nic *efx)
>>   
>>   	efx_mcdi_rx_free_indir_table(efx);
>>   
>> +#ifdef CONFIG_SFC_CXL
>> +	probe_data = container_of(efx, struct efx_probe_data, efx);
>> +	if (nic_data->wc_membase && !probe_data->cxl_pio_in_use)
>> +#else
>>   	if (nic_data->wc_membase)
>> +#endif
>>   		iounmap(nic_data->wc_membase);
>>   
>>   	rc = efx_mcdi_free_vis(efx);
>> @@ -1140,6 +1155,9 @@ static int efx_ef10_dimension_resources(struct efx_nic *efx)
>>   	unsigned int channel_vis, pio_write_vi_base, max_vis;
>>   	struct efx_ef10_nic_data *nic_data = efx->nic_data;
>>   	unsigned int uc_mem_map_size, wc_mem_map_size;
>> +#ifdef CONFIG_SFC_CXL
>> +	struct efx_probe_data *probe_data;
>> +#endif
>>   	void __iomem *membase;
>>   	int rc;
>>   
>> @@ -1263,8 +1281,27 @@ static int efx_ef10_dimension_resources(struct efx_nic *efx)
>>   	iounmap(efx->membase);
>>   	efx->membase = membase;
>>   
>> -	/* Set up the WC mapping if needed */
>> -	if (wc_mem_map_size) {
>> +	if (!wc_mem_map_size) {
>> +		netif_dbg(efx, probe, efx->net_dev, "wc_mem_map_size is 0\n");
> Please still print the details of the memory BAR that the netif_dbg has below.
> It is useful for debugging.
>

I guess just a goto there setting rc and removing this debug comment here.

It makes sense. I'll change it.


>> +		return 0;
>> +	}
>> +
>> +	/* Set up the WC mapping */
>> +
>> +#ifdef CONFIG_SFC_CXL
>> +	probe_data = container_of(efx, struct efx_probe_data, efx);
>> +	if ((nic_data->datapath_caps3 &
>> +	    (1 << MC_CMD_GET_CAPABILITIES_V7_OUT_CXL_CONFIG_ENABLE_LBN)) &&
>> +	    probe_data->cxl_pio_initialised) {
>> +		/* Using PIO through CXL mapping? */
>> +		nic_data->pio_write_base = probe_data->cxl->ctpio_cxl +
>> +					   (pio_write_vi_base * efx->vi_stride +
>> +					    ER_DZ_TX_PIOBUF - uc_mem_map_size);
>> +		probe_data->cxl_pio_in_use = true;
>> +	} else
>> +#endif
>> +	{
>> +		/* Using legacy PIO BAR mapping */
>>   		nic_data->wc_membase = ioremap_wc(efx->membase_phys +
>>   						  uc_mem_map_size,
>>   						  wc_mem_map_size);
>> @@ -1279,12 +1316,12 @@ static int efx_ef10_dimension_resources(struct efx_nic *efx)
>>   			nic_data->wc_membase +
>>   			(pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF -
>>   			 uc_mem_map_size);
>> -
>> -		rc = efx_ef10_link_piobufs(efx);
>> -		if (rc)
>> -			efx_ef10_free_piobufs(efx);
>>   	}
>>   
>> +	rc = efx_ef10_link_piobufs(efx);
>> +	if (rc)
>> +		efx_ef10_free_piobufs(efx);
>> +
>>   	netif_dbg(efx, probe, efx->net_dev,
>>   		  "memory BAR at %pa (virtual %p+%x UC, %p+%x WC)\n",
>>   		  &efx->membase_phys, efx->membase, uc_mem_map_size,
>> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
>> index 71b32fc48ca7..78eb8aa9702a 100644
>> --- a/drivers/net/ethernet/sfc/efx_cxl.c
>> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
>> @@ -24,9 +24,10 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
>>   	DECLARE_BITMAP(expected, CXL_MAX_CAPS);
>>   	DECLARE_BITMAP(found, CXL_MAX_CAPS);
>>   	struct pci_dev *pci_dev;
>> +	resource_size_t max;
>>   	struct efx_cxl *cxl;
>>   	struct resource res;
>> -	resource_size_t max;
>> +	struct range range;
>>   	u16 dvsec;
>>   	int rc;
>>   
>> @@ -135,10 +136,25 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
>>   		goto err_region;
>>   	}
>>   
>> +	rc = cxl_get_region_range(cxl->efx_region, &range);
>> +	if (rc) {
>> +		pci_err(pci_dev, "CXL getting regions params failed");
>> +		goto err_region_params;
>> +	}
>> +
>> +	cxl->ctpio_cxl = ioremap(range.start, range.end - range.start);
>> +	if (!cxl->ctpio_cxl) {
>> +		pci_err(pci_dev, "CXL ioremap region failed");
> This error will be more useful if you print out the start & size. Users can
> the check that against /proc/iomem.


I'll do.


>> +		goto err_region_params;
>> +	}
>> +
>>   	probe_data->cxl = cxl;
>> +	probe_data->cxl_pio_initialised = true;
>>   
>>   	return 0;
>>   
>> +err_region_params:
>> +	cxl_accel_region_detach(cxl->cxled);
>>   err_region:
>>   	cxl_dpa_free(cxl->cxled);
>>   err3:
>> @@ -153,6 +169,7 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
>>   void efx_cxl_exit(struct efx_probe_data *probe_data)
>>   {
>>   	if (probe_data->cxl) {
>> +		iounmap(probe_data->cxl->ctpio_cxl);
>>   		cxl_accel_region_detach(probe_data->cxl->cxled);
>>   		cxl_dpa_free(probe_data->cxl->cxled);
>>   		cxl_release_resource(probe_data->cxl->cxlds, CXL_RES_RAM);
>> diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
>> index 7f11ff200c25..79b0e6663f23 100644
>> --- a/drivers/net/ethernet/sfc/net_driver.h
>> +++ b/drivers/net/ethernet/sfc/net_driver.h
>> @@ -1209,6 +1209,7 @@ struct efx_cxl;
>>    * @efx: Efx NIC details
>>    * @cxl: details of related cxl objects
>>    * @cxl_pio_initialised: cxl initialization outcome.
>> ++ * @cxl_pio_in_use: PIO using CXL mapping
> Extra + sign here isn't right. Please build kdoc, I expect it would have caught this.


OK. I'll fix it.

Thanks!


> Martin
>
>>    */
>>   struct efx_probe_data {
>>   	struct pci_dev *pci_dev;
>> @@ -1216,6 +1217,7 @@ struct efx_probe_data {
>>   #ifdef CONFIG_SFC_CXL
>>   	struct efx_cxl *cxl;
>>   	bool cxl_pio_initialised;
>> +	bool cxl_pio_in_use;
>>   #endif
>>   };
>>   
>> diff --git a/drivers/net/ethernet/sfc/nic.h b/drivers/net/ethernet/sfc/nic.h
>> index 9fa5c4c713ab..c87cc9214690 100644
>> --- a/drivers/net/ethernet/sfc/nic.h
>> +++ b/drivers/net/ethernet/sfc/nic.h
>> @@ -152,6 +152,8 @@ enum {
>>    *	%MC_CMD_GET_CAPABILITIES response)
>>    * @datapath_caps2: Further Capabilities of datapath firmware (FLAGS2 field of
>>    * %MC_CMD_GET_CAPABILITIES response)
>> + * @datapath_caps3: Further Capabilities of datapath firmware (FLAGS3 field of
>> + * %MC_CMD_GET_CAPABILITIES response)
>>    * @rx_dpcpu_fw_id: Firmware ID of the RxDPCPU
>>    * @tx_dpcpu_fw_id: Firmware ID of the TxDPCPU
>>    * @must_probe_vswitching: Flag: vswitching has yet to be setup after MC reboot
>> @@ -186,6 +188,7 @@ struct efx_ef10_nic_data {
>>   	bool must_check_datapath_caps;
>>   	u32 datapath_caps;
>>   	u32 datapath_caps2;
>> +	u32 datapath_caps3;
>>   	unsigned int rx_dpcpu_fw_id;
>>   	unsigned int tx_dpcpu_fw_id;
>>   	bool must_probe_vswitching;
>> -- 
>> 2.17.1
>>
>>