diff mbox series

[v2,15/15] efx: support pio mapping based on cxl

Message ID 20240715172835.24757-16-alejandro.lucero-palau@amd.com (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series cxl: add Type2 device support | expand

Commit Message

Lucero Palau, Alejandro July 15, 2024, 5:28 p.m. UTC
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      | 25 +++++++++++++++++++++----
 drivers/net/ethernet/sfc/efx_cxl.c   | 12 +++++++++++-
 drivers/net/ethernet/sfc/mcdi_pcol.h |  3 +++
 drivers/net/ethernet/sfc/nic.h       |  1 +
 4 files changed, 36 insertions(+), 5 deletions(-)

Comments

Jonathan Cameron Aug. 4, 2024, 6:13 p.m. UTC | #1
On Mon, 15 Jul 2024 18:28:35 +0100
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.

This explains why you weren't worried about any step of the CXL
code failing and why that wasn't a 'bug' as such.

I'd argue that you should still have the cxl intialization return
an error code and cleanup any state it if hits an error.

Then the top level driver can of course elect to use an alternative
path given that failure.  Logically it belongs there rather than relying
on a buffer being mapped or not.

> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> ---
>  drivers/net/ethernet/sfc/ef10.c      | 25 +++++++++++++++++++++----
>  drivers/net/ethernet/sfc/efx_cxl.c   | 12 +++++++++++-
>  drivers/net/ethernet/sfc/mcdi_pcol.h |  3 +++
>  drivers/net/ethernet/sfc/nic.h       |  1 +
>  4 files changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
> index 8fa6c0e9195b..3924076d2628 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;
>  }
>  
> @@ -1275,10 +1282,20 @@ static int efx_ef10_dimension_resources(struct efx_nic *efx)
>  			return -ENOMEM;
>  		}
>  		nic_data->pio_write_vi_base = pio_write_vi_base;
> -		nic_data->pio_write_base =
> -			nic_data->wc_membase +
> -			(pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF -
> -			 uc_mem_map_size);
> +
> +		if ((nic_data->datapath_caps3 &
> +		    (1 << MC_CMD_GET_CAPABILITIES_V10_OUT_CXL_CONFIG_ENABLE_LBN)) &&
> +		    efx->cxl->ctpio_cxl)
As per comment at the top, I'd prefer to see some clean handling of the an
error passed up to the caller of the cxl init that then sets a flag that
we can clearly see is all about whether we have CXL or not.

Using this buffer mapping is a it too much of a detail in my opinion.

> +		{
> +			nic_data->pio_write_base =
> +				efx->cxl->ctpio_cxl +
> +				(pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF -
> +				 uc_mem_map_size);
> +		} else {
> +			nic_data->pio_write_base =nic_data->wc_membase +
> +				(pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF -
> +				 uc_mem_map_size);
> +		}
Alejandro Lucero Palau Aug. 19, 2024, 4:28 p.m. UTC | #2
On 8/4/24 19:13, Jonathan Cameron wrote:
> On Mon, 15 Jul 2024 18:28:35 +0100
> 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.
> This explains why you weren't worried about any step of the CXL
> code failing and why that wasn't a 'bug' as such.
>
> I'd argue that you should still have the cxl intialization return
> an error code and cleanup any state it if hits an error.


Ideally, but with devm* being used, this is not easy to do if the error 
is not fatal.


> Then the top level driver can of course elect to use an alternative
> path given that failure.  Logically it belongs there rather than relying
> on a buffer being mapped or not.
>

Same driver needs to support same functionality which relies on those 
specific hardware buffers.

The functionality is expected to be there with or without CXL. If the 
hardware has no CXL, the system or the device, the functionality will be 
there with legacy PCIe BAR regions. The green light for CXL use comes 
from two sources: the firmware and the kernel. Both need to give the 
thumbs up. If not, legacy PCIe BAR regions will be used.


>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/net/ethernet/sfc/ef10.c      | 25 +++++++++++++++++++++----
>>   drivers/net/ethernet/sfc/efx_cxl.c   | 12 +++++++++++-
>>   drivers/net/ethernet/sfc/mcdi_pcol.h |  3 +++
>>   drivers/net/ethernet/sfc/nic.h       |  1 +
>>   4 files changed, 36 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
>> index 8fa6c0e9195b..3924076d2628 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;
>>   }
>>   
>> @@ -1275,10 +1282,20 @@ static int efx_ef10_dimension_resources(struct efx_nic *efx)
>>   			return -ENOMEM;
>>   		}
>>   		nic_data->pio_write_vi_base = pio_write_vi_base;
>> -		nic_data->pio_write_base =
>> -			nic_data->wc_membase +
>> -			(pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF -
>> -			 uc_mem_map_size);
>> +
>> +		if ((nic_data->datapath_caps3 &
>> +		    (1 << MC_CMD_GET_CAPABILITIES_V10_OUT_CXL_CONFIG_ENABLE_LBN)) &&
>> +		    efx->cxl->ctpio_cxl)
> As per comment at the top, I'd prefer to see some clean handling of the an
> error passed up to the caller of the cxl init that then sets a flag that
> we can clearly see is all about whether we have CXL or not.
>
> Using this buffer mapping is a it too much of a detail in my opinion.


Yes, maybe that is clearer than relying on the pointer from the CXL 
mapping.

I will do it.

Thanks!


>> +		{
>> +			nic_data->pio_write_base =
>> +				efx->cxl->ctpio_cxl +
>> +				(pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF -
>> +				 uc_mem_map_size);
>> +		} else {
>> +			nic_data->pio_write_base =nic_data->wc_membase +
>> +				(pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF -
>> +				 uc_mem_map_size);
>> +		}
>
Jonathan Cameron Aug. 27, 2024, 3:23 p.m. UTC | #3
On Mon, 19 Aug 2024 17:28:46 +0100
Alejandro Lucero Palau <alucerop@amd.com> wrote:

> On 8/4/24 19:13, Jonathan Cameron wrote:
> > On Mon, 15 Jul 2024 18:28:35 +0100
> > 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.  
> > This explains why you weren't worried about any step of the CXL
> > code failing and why that wasn't a 'bug' as such.
> >
> > I'd argue that you should still have the cxl intialization return
> > an error code and cleanup any state it if hits an error.  
> 
> 
> Ideally, but with devm* being used, this is not easy to do if the error 
> is not fatal.

That's usually a strong argument that you shouldn't use devm at that
level of abstraction.  

> 
> 
> > Then the top level driver can of course elect to use an alternative
> > path given that failure.  Logically it belongs there rather than relying
> > on a buffer being mapped or not.
> >  
> 
> Same driver needs to support same functionality which relies on those 
> specific hardware buffers.
> 
> The functionality is expected to be there with or without CXL. If the 
> hardware has no CXL, the system or the device, the functionality will be 
> there with legacy PCIe BAR regions. The green light for CXL use comes 
> from two sources: the firmware and the kernel. Both need to give the 
> thumbs up. If not, legacy PCIe BAR regions will be used.

Rather than going through full setup, see if you can figure out a minimal
(state free) check on whether it should work.

If a system is broken, then it's very different from a legacy system
with no support for CXL and we can maybe just handle the broken system
with errors (or quirks if it's a shipping system).

Jonathan
  
>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
index 8fa6c0e9195b..3924076d2628 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;
 }
 
@@ -1275,10 +1282,20 @@  static int efx_ef10_dimension_resources(struct efx_nic *efx)
 			return -ENOMEM;
 		}
 		nic_data->pio_write_vi_base = pio_write_vi_base;
-		nic_data->pio_write_base =
-			nic_data->wc_membase +
-			(pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF -
-			 uc_mem_map_size);
+
+		if ((nic_data->datapath_caps3 &
+		    (1 << MC_CMD_GET_CAPABILITIES_V10_OUT_CXL_CONFIG_ENABLE_LBN)) &&
+		    efx->cxl->ctpio_cxl)
+		{
+			nic_data->pio_write_base =
+				efx->cxl->ctpio_cxl +
+				(pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF -
+				 uc_mem_map_size);
+		} else {
+			nic_data->pio_write_base =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)
diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
index 4012e3faa298..8e65ef42a572 100644
--- a/drivers/net/ethernet/sfc/efx_cxl.c
+++ b/drivers/net/ethernet/sfc/efx_cxl.c
@@ -21,8 +21,8 @@ 
 void efx_cxl_init(struct efx_nic *efx)
 {
 	struct pci_dev *pci_dev = efx->pci_dev;
+	resource_size_t start, end, max = 0;
 	struct efx_cxl *cxl = efx->cxl;
-	resource_size_t max = 0;
 	struct resource res;
 	u16 dvsec;
 
@@ -104,6 +104,13 @@  void efx_cxl_init(struct efx_nic *efx)
 		return;
 	}
 
+	cxl_accel_get_region_params(cxl->efx_region, &start, &end);
+
+	cxl->ctpio_cxl = ioremap(start, end - start);
+	if (!cxl->ctpio_cxl) {
+		pci_info(pci_dev, "CXL accel create region failed");
+		cxl_dpa_free(cxl->cxled);
+	}
 out:
 	cxl_release_endpoint(cxl->cxlmd, cxl->endpoint);
 }
@@ -112,6 +119,9 @@  void efx_cxl_exit(struct efx_nic *efx)
 {
 	struct efx_cxl *cxl = efx->cxl;
 
+	if (cxl->ctpio_cxl)
+		iounmap(cxl->ctpio_cxl);
+
 	if (cxl->efx_region)
 		cxl_region_detach(cxl->cxled);
 
diff --git a/drivers/net/ethernet/sfc/mcdi_pcol.h b/drivers/net/ethernet/sfc/mcdi_pcol.h
index cd297e19cddc..05fd5e021142 100644
--- a/drivers/net/ethernet/sfc/mcdi_pcol.h
+++ b/drivers/net/ethernet/sfc/mcdi_pcol.h
@@ -18374,6 +18374,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 16
+#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.
diff --git a/drivers/net/ethernet/sfc/nic.h b/drivers/net/ethernet/sfc/nic.h
index 1db64fc6e909..cd635f4f7f94 100644
--- a/drivers/net/ethernet/sfc/nic.h
+++ b/drivers/net/ethernet/sfc/nic.h
@@ -186,6 +186,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;