Message-ID: <20250429193732.GQ3339421@horms.kernel.org>
Date: Tue, 29 Apr 2025 20:37:32 +0100
From: Simon Horman <horms@...nel.org>
To: Raju Rangoju <Raju.Rangoju@....com>
Cc: andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, Shyam-sundar.S-k@....com,
	Sudheesh Mavila <sudheesh.mavila@....com>
Subject: Re: [PATCH net-next v2 3/5] amd-xgbe: add support for new XPCS
 routines

On Mon, Apr 28, 2025 at 08:32:33PM +0530, 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.
> 
> Some of the ethernet add-in-cards have dual PHY but share a single MDIO
> line (between the ports). In such cases, link inconsistencies are
> noticed during the heavy traffic and during reboot stress tests. Using
> smn calls helps avoid such race conditions.
> 
> Suggested-by: Sudheesh Mavila <sudheesh.mavila@....com>
> Signed-off-by: Raju Rangoju <Raju.Rangoju@....com>
> ---
> - PCI config accesses can race with other drivers performing SMN accesses
>   so, fall back to AMD SMN API to avoid race.
> 
>  drivers/net/ethernet/amd/xgbe/xgbe-dev.c | 81 ++++++++++++++++++++++++
>  drivers/net/ethernet/amd/xgbe/xgbe-smn.h | 30 +++++++++
>  drivers/net/ethernet/amd/xgbe/xgbe.h     |  6 ++
>  3 files changed, 117 insertions(+)
>  create mode 100644 drivers/net/ethernet/amd/xgbe/xgbe-smn.h
> 
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> index 765f20b24722..5f367922e705 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> @@ -14,6 +14,7 @@

Hi Raju,

I think you need the following about here:

#include <linux/pci.h>

To make sure that pci_err(), which is used elsewhere in this patch,
is always defined. Building allmodconfig for arm and arm64 shows
that, without this change, pci_err is not defined.

Alternatively, perhaps netdev_err can be used instead of pci_err().

>  
>  #include "xgbe.h"
>  #include "xgbe-common.h"
> +#include "xgbe-smn.h"
>  
>  static inline unsigned int xgbe_get_max_frame(struct xgbe_prv_data *pdata)
>  {
> @@ -1066,6 +1067,80 @@ static void xgbe_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;
> +	int mmd_data;
> +	int ret;
> +
> +	mmd_address = xgbe_get_mmd_address(pdata, mmd_reg);
> +
> +	xgbe_get_pcs_index_and_offset(pdata, mmd_address, &index, &offset);
> +
> +	ret = amd_smn_write(0, (pdata->smn_base + pdata->xpcs_window_sel_reg), index);

nit: Please line wrap to 80 columns wide or less, as is still preferred for
     Networking code.  Likewise elsewhere in this patch.

     Flagged by checkpatch.pl --max-line-length=80

     Also, the inner parentheses seem to be unnecessary.

> +	if (ret)
> +		return ret;
> +
> +	ret = amd_smn_read(0, pdata->smn_base + offset, &mmd_data);
> +	if (ret)
> +		return ret;
> +
> +	mmd_data = (offset % 4) ? FIELD_GET(XGBE_GEN_HI_MASK, mmd_data) :
> +				  FIELD_GET(XGBE_GEN_LO_MASK, mmd_data);
> +
> +	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 *dev;
> +	int ret;
> +
> +	dev = pdata->pcidev;
> +	mmd_address = xgbe_get_mmd_address(pdata, mmd_reg);
> +
> +	xgbe_get_pcs_index_and_offset(pdata, mmd_address, &index, &offset);
> +
> +	ret = amd_smn_write(0, (pdata->smn_base + pdata->xpcs_window_sel_reg), index);
> +	if (ret) {
> +		pci_err(dev, "Failed to write data 0x%x\n", index);
> +		return;
> +	}
> +
> +	ret = amd_smn_read(0, pdata->smn_base + offset, &pci_mmd_data);
> +	if (ret) {
> +		pci_err(dev, "Failed to read data\n");
> +		return;
> +	}
> +
> +	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;
> +
> +	ret = amd_smn_write(0, (pdata->smn_base + pdata->xpcs_window_sel_reg), index);
> +	if (ret) {
> +		pci_err(dev, "Failed to write data 0x%x\n", index);
> +		return;
> +	}
> +
> +	ret = amd_smn_write(0, (pdata->smn_base + offset), pci_mmd_data);
> +	if (ret) {
> +		pci_err(dev, "Failed to write data 0x%x\n", pci_mmd_data);
> +		return;
> +	}
> +}

...

> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-smn.h b/drivers/net/ethernet/amd/xgbe/xgbe-smn.h
> new file mode 100644
> index 000000000000..a1763aa648bd
> --- /dev/null
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-smn.h
> @@ -0,0 +1,30 @@
> +// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-3-Clause)

Checkpatch says this should be:

/* SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-3-Clause) */


> +/*
> + * Copyright (c) 2014-2025, Advanced Micro Devices, Inc.
> + * Copyright (c) 2014, Synopsys, Inc.
> + * All rights reserved
> + *
> + * Author: Raju Rangoju <Raju.Rangoju@....com>
> + */
> +
> +#ifndef __SMN_H__
> +#define __SMN_H__
> +
> +#ifdef CONFIG_AMD_NB
> +
> +#include <asm/amd_nb.h>
> +
> +#else
> +
> +static inline int amd_smn_write(u16 node, u32 address, u32 value)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline int amd_smn_read(u16 node, u32 address, u32 *value)
> +{
> +	return -ENODEV;
> +}
> +
> +#endif
> +#endif

It feels a little odd to provide these dummy implementation here,
rather than where the real implementations are declared. But I do
see where you are coming from with this approach. And I guess it
is fine so long as this is the only user of this mechanism.

...

Powered by blists - more mailing lists