Re: [PATCH net-next 3/5] amd-xgbe: add support for new XPCS routines

[Tom Lendacky <thomas.lendacky@amd.com>]

On 4/8/25 13:19, Raju Rangoju wrote:
> Add the necessary support to enable Crater ethernet device. Since the
> BAR1 address cannot be used to access the XPCS registers on Crater, use
> the smn functions.
> 
> Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com>
> ---
>  drivers/net/ethernet/amd/xgbe/xgbe-dev.c | 79 ++++++++++++++++++++++++
>  drivers/net/ethernet/amd/xgbe/xgbe.h     |  6 ++
>  2 files changed, 85 insertions(+)
> 
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> index ae82dc3ac460..d75cf8df272f 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> @@ -11,6 +11,7 @@
>  #include <linux/bitrev.h>
>  #include <linux/crc32.h>
>  #include <linux/crc32poly.h>
> +#include <linux/pci.h>
>  
>  #include "xgbe.h"
>  #include "xgbe-common.h"
> @@ -1066,6 +1067,78 @@ static void get_pcs_index_and_offset(struct xgbe_prv_data *pdata,
>  	*offset = pdata->xpcs_window + (mmd_address & pdata->xpcs_window_mask);
>  }
>  
> +static int xgbe_read_mmd_regs_v3(struct xgbe_prv_data *pdata, int prtad,
> +				 int mmd_reg)
> +{
> +	unsigned int mmd_address, index, offset;
> +	struct pci_dev *rdev;
> +	unsigned long flags;
> +	int mmd_data;
> +
> +	rdev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0, 0));
> +	if (!rdev)
> +		return 0;
> +
> +	mmd_address = get_mmd_address(pdata, mmd_reg);
> +
> +	get_pcs_index_and_offset(pdata, mmd_address, &index, &offset);
> +
> +	spin_lock_irqsave(&pdata->xpcs_lock, flags);

These PCI config accesses can race with other drivers performing SMN
accesses. You'll need to make use of the AMD SMN API (see
arch/x86/kernel/amd_node.c, amd_smn_{read,write}()) to ensure protection.

The AMD SMN API uses a mutex to sync access, if you need to protect
these accesses with a spinlock then you are looking at updating the AMD
SMN API, too.

Thanks,
Tom

> +	pci_write_config_dword(rdev, 0x60, (pdata->xphy_base + pdata->xpcs_window_sel_reg));
> +	pci_write_config_dword(rdev, 0x64, index);
> +	pci_write_config_dword(rdev, 0x60, pdata->xphy_base + offset);
> +	pci_read_config_dword(rdev, 0x64, &mmd_data);
> +	mmd_data = (offset % 4) ? FIELD_GET(XGBE_GEN_HI_MASK, mmd_data) :
> +				  FIELD_GET(XGBE_GEN_LO_MASK, mmd_data);
> +
> +	pci_dev_put(rdev);
> +	spin_unlock_irqrestore(&pdata->xpcs_lock, flags);
> +
> +	return mmd_data;
> +}
> +
> +static void xgbe_write_mmd_regs_v3(struct xgbe_prv_data *pdata, int prtad,
> +				   int mmd_reg, int mmd_data)
> +{
> +	unsigned int pci_mmd_data, hi_mask, lo_mask;
> +	unsigned int mmd_address, index, offset;
> +	struct pci_dev *rdev;
> +	unsigned long flags;
> +
> +	rdev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0, 0));
> +	if (!rdev)
> +		return;
> +
> +	mmd_address = get_mmd_address(pdata, mmd_reg);
> +
> +	get_pcs_index_and_offset(pdata, mmd_address, &index, &offset);
> +
> +	spin_lock_irqsave(&pdata->xpcs_lock, flags);
> +	pci_write_config_dword(rdev, 0x60, (pdata->xphy_base + pdata->xpcs_window_sel_reg));
> +	pci_write_config_dword(rdev, 0x64, index);
> +	pci_write_config_dword(rdev, 0x60, pdata->xphy_base + offset);
> +	pci_read_config_dword(rdev, 0x64, &pci_mmd_data);
> +
> +	if (offset % 4) {
> +		hi_mask = FIELD_PREP(XGBE_GEN_HI_MASK, mmd_data);
> +		lo_mask = FIELD_GET(XGBE_GEN_LO_MASK, pci_mmd_data);
> +	} else {
> +		hi_mask = FIELD_PREP(XGBE_GEN_HI_MASK,
> +				     FIELD_GET(XGBE_GEN_HI_MASK, pci_mmd_data));
> +		lo_mask = FIELD_GET(XGBE_GEN_LO_MASK, mmd_data);
> +	}
> +
> +	pci_mmd_data = hi_mask | lo_mask;
> +
> +	pci_write_config_dword(rdev, 0x60, (pdata->xphy_base + pdata->xpcs_window_sel_reg));
> +	pci_write_config_dword(rdev, 0x64, index);
> +	pci_write_config_dword(rdev, 0x60, (pdata->xphy_base + offset));
> +	pci_write_config_dword(rdev, 0x64, pci_mmd_data);
> +	pci_dev_put(rdev);
> +
> +	spin_unlock_irqrestore(&pdata->xpcs_lock, flags);
> +}
> +
>  static int xgbe_read_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad,
>  				 int mmd_reg)
>  {
> @@ -1160,6 +1233,9 @@ static int xgbe_read_mmd_regs(struct xgbe_prv_data *pdata, int prtad,
>  	case XGBE_XPCS_ACCESS_V2:
>  	default:
>  		return xgbe_read_mmd_regs_v2(pdata, prtad, mmd_reg);
> +
> +	case XGBE_XPCS_ACCESS_V3:
> +		return xgbe_read_mmd_regs_v3(pdata, prtad, mmd_reg);
>  	}
>  }
>  
> @@ -1173,6 +1249,9 @@ static void xgbe_write_mmd_regs(struct xgbe_prv_data *pdata, int prtad,
>  	case XGBE_XPCS_ACCESS_V2:
>  	default:
>  		return xgbe_write_mmd_regs_v2(pdata, prtad, mmd_reg, mmd_data);
> +
> +	case XGBE_XPCS_ACCESS_V3:
> +		return xgbe_write_mmd_regs_v3(pdata, prtad, mmd_reg, mmd_data);
>  	}
>  }
>  
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe.h b/drivers/net/ethernet/amd/xgbe/xgbe.h
> index 2e9b3be44ff8..6c49bf19e537 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe.h
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe.h
> @@ -242,6 +242,10 @@
>  #define XGBE_RV_PCI_DEVICE_ID	0x15d0
>  #define XGBE_YC_PCI_DEVICE_ID	0x14b5
>  
> + /* Generic low and high masks */
> +#define XGBE_GEN_HI_MASK	GENMASK(31, 16)
> +#define XGBE_GEN_LO_MASK	GENMASK(15, 0)
> +
>  struct xgbe_prv_data;
>  
>  struct xgbe_packet_data {
> @@ -460,6 +464,7 @@ enum xgbe_speed {
>  enum xgbe_xpcs_access {
>  	XGBE_XPCS_ACCESS_V1 = 0,
>  	XGBE_XPCS_ACCESS_V2,
> +	XGBE_XPCS_ACCESS_V3,
>  };
>  
>  enum xgbe_an_mode {
> @@ -951,6 +956,7 @@ struct xgbe_prv_data {
>  	struct device *dev;
>  	struct platform_device *phy_platdev;
>  	struct device *phy_dev;
> +	unsigned int xphy_base;
>  
>  	/* Version related data */
>  	struct xgbe_version_data *vdata;