Subject: Re: [PATCH net-next 2/2] amd-xgbe: Add support for AMD Crater ethernet device

From: Simon Horman <[email-masked]>
Date: Thu, 19 Oct 2023 13:03:10 +0200

On Wed, Oct 18, 2023 at 08:14:50PM +0530, Raju Rangoju wrote:
> The AMD Crater device has new window settings for the XPCS access, add
> support to adopt to the new window settings. There is a hardware bug
> where in the BAR1 registers cannot be accessed directly. As a fallback
> mechanism, access these PCS registers through indirect access via SMN.
> 
> Co-developed-by: Sudheesh Mavila <[email-masked]>
> Signed-off-by: Sudheesh Mavila <[email-masked]>
> Acked-by: Shyam Sundar S K <[email-masked]>
> Signed-off-by: Raju Rangoju <[email-masked]>

Hi Sudheesh,

some minor feedback from my side.

> ---
>  drivers/net/ethernet/amd/xgbe/xgbe-common.h |  5 ++++
>  drivers/net/ethernet/amd/xgbe/xgbe-dev.c    | 33 +++++++++++++++++----
>  drivers/net/ethernet/amd/xgbe/xgbe-pci.c    | 32 +++++++++++++++-----
>  drivers/net/ethernet/amd/xgbe/xgbe.h        |  6 ++++
>  4 files changed, 63 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-common.h b/drivers/net/ethernet/amd/xgbe/xgbe-common.h
> index 3b70f6737633..e1f70f0528ef 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-common.h
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-common.h
> @@ -900,6 +900,11 @@
>  #define PCS_V2_RV_WINDOW_SELECT		0x1064
>  #define PCS_V2_YC_WINDOW_DEF		0x18060
>  #define PCS_V2_YC_WINDOW_SELECT		0x18064
> +#define PCS_V2_RN_WINDOW_DEF		0xF8078
> +#define PCS_V2_RN_WINDOW_SELECT		0xF807c
> +
> +#define PCS_RN_SMN_BASE_ADDR		0x11E00000
> +#define PCS_RN_PORT_ADDR_SIZE		0x100000
>  
>  /* PCS register entry bit positions and sizes */
>  #define PCS_V2_WINDOW_DEF_OFFSET_INDEX	6
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> index f393228d41c7..da8ec218282f 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> @@ -1176,8 +1176,17 @@ static int xgbe_read_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad,
>  	offset = pdata->xpcs_window + (mmd_address & pdata->xpcs_window_mask);
>  
>  	spin_lock_irqsave(&pdata->xpcs_lock, flags);
> -	XPCS32_IOWRITE(pdata, pdata->xpcs_window_sel_reg, index);
> -	mmd_data = XPCS16_IOREAD(pdata, offset);
> +	if (pdata->vdata->is_crater) {
> +		amd_smn_write(0,
> +			      (pdata->xphy_base + pdata->xpcs_window_sel_reg),
> +			      index);
> +		amd_smn_read(0, pdata->xphy_base + offset, &mmd_data);
> +		mmd_data = (offset % ALIGNMENT_VAL) ?
> +			   ((mmd_data >> 16) & 0xffff) : (mmd_data & 0xffff);

I wonder if it would be nice to use FIELD_GET() here...

> +	} else {
> +		XPCS32_IOWRITE(pdata, pdata->xpcs_window_sel_reg, index);
> +		mmd_data = XPCS16_IOREAD(pdata, offset);
> +	}
>  	spin_unlock_irqrestore(&pdata->xpcs_lock, flags);
>  
>  	return mmd_data;
> @@ -1186,8 +1195,8 @@ static int xgbe_read_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad,
>  static void xgbe_write_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad,
>  				   int mmd_reg, int mmd_data)
>  {
> +	unsigned int mmd_address, index, offset, crtr_mmd_data;
>  	unsigned long flags;
> -	unsigned int mmd_address, index, offset;
>  
>  	if (mmd_reg & XGBE_ADDR_C45)
>  		mmd_address = mmd_reg & ~XGBE_ADDR_C45;
> @@ -1208,8 +1217,22 @@ static void xgbe_write_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad,
>  	offset = pdata->xpcs_window + (mmd_address & pdata->xpcs_window_mask);
>  
>  	spin_lock_irqsave(&pdata->xpcs_lock, flags);
> -	XPCS32_IOWRITE(pdata, pdata->xpcs_window_sel_reg, index);
> -	XPCS16_IOWRITE(pdata, offset, mmd_data);
> +	if (pdata->vdata->is_crater) {
> +		amd_smn_write(0, (pdata->xphy_base + pdata->xpcs_window_sel_reg), index);
> +		amd_smn_read(0, pdata->xphy_base + offset, &crtr_mmd_data);
> +		if (offset % ALIGNMENT_VAL) {
> +			crtr_mmd_data &= ~GENMASK(31, 16);
> +			crtr_mmd_data |=  (mmd_data << 16);
> +		} else {
> +			crtr_mmd_data &= ~GENMASK(15, 0);
> +			crtr_mmd_data |=  (mmd_data);
> +		}

... and FIELD_PREP() here.

> +		amd_smn_write(0, (pdata->xphy_base + pdata->xpcs_window_sel_reg), index);
> +		amd_smn_write(0, (pdata->xphy_base + offset), crtr_mmd_data);
> +	} else {
> +		XPCS32_IOWRITE(pdata, pdata->xpcs_window_sel_reg, index);
> +		XPCS16_IOWRITE(pdata, offset, mmd_data);
> +	}
>  	spin_unlock_irqrestore(&pdata->xpcs_lock, flags);
>  }
>  
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-pci.c b/drivers/net/ethernet/amd/xgbe/xgbe-pci.c
> index a17359d43b45..90ad520d3c29 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-pci.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-pci.c
> @@ -279,15 +279,21 @@ static int xgbe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		pdata->xpcs_window_def_reg = PCS_V2_RV_WINDOW_DEF;
>  		pdata->xpcs_window_sel_reg = PCS_V2_RV_WINDOW_SELECT;
>  	} else if (rdev && (rdev->vendor == PCI_VENDOR_ID_AMD) &&
> -		   (rdev->device == 0x14b5)) {
> -		pdata->xpcs_window_def_reg = PCS_V2_YC_WINDOW_DEF;
> -		pdata->xpcs_window_sel_reg = PCS_V2_YC_WINDOW_SELECT;
> -
> -		/* Yellow Carp devices do not need cdr workaround */
> +		   ((rdev->device == 0x14b5) || (rdev->device == 0x1630))) {
> +		/* Yellow Carp and Crater devices
> +		 * do not need cdr workaround and RRC
> +		 */
>  		pdata->vdata->an_cdr_workaround = 0;
> -
> -		/* Yellow Carp devices do not need rrc */
>  		pdata->vdata->enable_rrc = 0;
> +
> +		if (rdev->device == 0x1630) {

Not strictly related to this patch, but I am wondering if we could create
#defines for magic numbers like this one.

> +			pdata->xpcs_window_def_reg = PCS_V2_RN_WINDOW_DEF;
> +			pdata->xpcs_window_sel_reg = PCS_V2_RN_WINDOW_SELECT;
> +			pdata->vdata->is_crater = true;

Is 'is_crater' necessary?
pdata has a pointer to the pci_dev, AFAICT.

> +		} else {
> +			pdata->xpcs_window_def_reg = PCS_V2_YC_WINDOW_DEF;
> +			pdata->xpcs_window_sel_reg = PCS_V2_YC_WINDOW_SELECT;
> +		}
>  	} else {
>  		pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF;
>  		pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT;
> @@ -295,7 +301,17 @@ static int xgbe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	pci_dev_put(rdev);
>  
>  	/* Configure the PCS indirect addressing support */
> -	reg = XPCS32_IOREAD(pdata, pdata->xpcs_window_def_reg);
> +	if (pdata->vdata->is_crater) {
> +		reg = XP_IOREAD(pdata, XP_PROP_0);
> +		pdata->xphy_base = PCS_RN_SMN_BASE_ADDR +
> +				   (PCS_RN_PORT_ADDR_SIZE *
> +				    XP_GET_BITS(reg, XP_PROP_0, PORT_ID));
> +		if (netif_msg_probe(pdata))
> +			dev_dbg(dev, "xphy_base = %#08x\n", pdata->xphy_base);
> +		amd_smn_read(0, pdata->xphy_base + (pdata->xpcs_window_def_reg), &reg);
> +	} else {
> +		reg = XPCS32_IOREAD(pdata, pdata->xpcs_window_def_reg);
> +	}
>  	pdata->xpcs_window = XPCS_GET_BITS(reg, PCS_V2_WINDOW_DEF, OFFSET);
>  	pdata->xpcs_window <<= 6;
>  	pdata->xpcs_window_size = XPCS_GET_BITS(reg, PCS_V2_WINDOW_DEF, SIZE);
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe.h b/drivers/net/ethernet/amd/xgbe/xgbe.h
> index ad136ed493ed..a161fac35643 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe.h
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe.h
> @@ -133,6 +133,7 @@
>  #include <linux/dcache.h>
>  #include <linux/ethtool.h>
>  #include <linux/list.h>
> +#include <asm/amd_nb.h>
>  
>  #define XGBE_DRV_NAME		"amd-xgbe"
>  #define XGBE_DRV_DESC		"AMD 10 Gigabit Ethernet Driver"
> @@ -305,6 +306,9 @@
>  /* MDIO port types */
>  #define XGMAC_MAX_C22_PORT		3
>  
> + /* offset alignment */
> +#define ALIGNMENT_VAL			4
> +
>  /* Link mode bit operations */
>  #define XGBE_ZERO_SUP(_ls)		\
>  	ethtool_link_ksettings_zero_link_mode((_ls), supported)
> @@ -1046,6 +1050,7 @@ struct xgbe_version_data {
>  	unsigned int rx_desc_prefetch;
>  	unsigned int an_cdr_workaround;
>  	unsigned int enable_rrc;
> +	bool is_crater;
>  };
>  
>  struct xgbe_prv_data {
> @@ -1056,6 +1061,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;
> -- 
> 2.25.1
> 
>