Re: [PATCH v16 22/22] sfc: support pio mapping based on cxl

[Dan Williams <dan.j.williams@intel.com>]

alejandro.lucero-palau@ 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.

I was really hoping to get to the end of this patchset and read the
compelling story about why anyone with this device would be clamoring
for the CXL support to be lit up. One of the best ways to get
maintainers to accept complexity is to offset the complexity with
compelling end user impact. So, every time someone dips into
drivers/cxl/ and gets discouraged by "ugh, the complexity" they can
refer to this patch and be reminded "oh, the benefit!".

Maybe that would be more obvious to me if I knew what a "PIO buffer" was
used for currently, but some more words about the why of all this would
help clarify if the design is making the right complexity vs benefit
tradeoffs.

> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> Acked-by: Edward Cree <ecree.xilinx@gmail.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/net/ethernet/sfc/ef10.c       | 50 +++++++++++++++++++++++----
>  drivers/net/ethernet/sfc/efx_cxl.c    | 18 ++++++++++
>  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 47349c148c0c..1a13fdbbc1b3 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'. */
>  
> @@ -106,7 +107,7 @@ static int efx_ef10_get_vf_index(struct efx_nic *efx)
>  
>  static int efx_ef10_init_datapath_caps(struct efx_nic *efx)
>  {
> -	MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CAPABILITIES_V4_OUT_LEN);
> +	MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CAPABILITIES_V7_OUT_LEN);
>  	struct efx_ef10_nic_data *nic_data = efx->nic_data;
>  	size_t outlen;
>  	int rc;
> @@ -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

Not my driver to maintain, but the ifdefery really does not belong here...

>  	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

...in the header do something like:

#ifdef CONFIG_SFC_CXL
static inline void unmap_wc_membase(nic_data)
{
	struct efx_probe_data *probe_data = container_of(efx, struct efx_probe_data, efx);

	 if (nic_data->wc_membase && !probe_data->cxl_pio_in_use)
		iounmap(nic_data->wc_membase);
}
#else
static inline void unmap_wc_membase(nic_data)
{
	 if (nic_data->wc_membase)
		iounmap(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,25 @@ 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)
> +		goto skip_pio;
> +
> +	/* 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

Looks like another static inline helper.