Re: [PATCH v4 net-next 2/2] amd-xgbe: add support for Crater ethernet device

[Simon Horman <horms@kernel.org>]

On Mon, Feb 12, 2024 at 05:27:32PM +0530, Raju Rangoju wrote:
> 
> 
> On 2/8/2024 12:39 AM, Simon Horman wrote:
> > On Tue, Feb 06, 2024 at 02:19:00AM +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@amd.com>
> > > Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com>
> > > ---
> > >   drivers/net/ethernet/amd/xgbe/xgbe-common.h |   5 +
> > >   drivers/net/ethernet/amd/xgbe/xgbe-dev.c    |  57 ++++++++
> > >   drivers/net/ethernet/amd/xgbe/xgbe-pci.c    |  33 ++++-
> > >   drivers/net/ethernet/amd/xgbe/xgbe-smn.h    | 139 ++++++++++++++++++++
> > >   drivers/net/ethernet/amd/xgbe/xgbe.h        |   7 +
> > >   5 files changed, 240 insertions(+), 1 deletion(-)
> > >   create mode 100644 drivers/net/ethernet/amd/xgbe/xgbe-smn.h
> > 
> > Hi Raju,
> > 
> > This patch seems to be doing a lot.
> > 
> > * Add support for XGBE_RN_PCI_DEVICE_ID to xgbe_pci_probe()
> > * Add fallback implementations of amd_smn_(write|read)()
> > * Add XGBE_XPCS_ACCESS_V3 support to xgbe_(read|write)_mmd_regs()
> > * Add XGBE_XPCS_ACCESS_V3 support to xgbe_pci_probe()
> > * Add support for PCI_VDEVICE(AMD, 0x1641)
> > 
> > So a similar theme to my comment on patch 1/1,
> > I wonder if it could be broken up into separate patches.
> 
> Hi Simon,
> 
> In my v2[*] series I had initially split pci_id patch to separate patch.
> But, I had received a comment from you about "W=1 allmodconfig builds on
> x86_64 with gcc-13 and clang-16 flag that xgbe_v3 us defined but not used."
> In this series, I had ensured warnings are taken care.
> 
> However, based on your new comments I will further try to separate the
> patches taking care of warnings.
> 
> [*] "[PATCH v2 net-next 2/4] amd-xgbe: add support for Crater ethernet
> device"

Thanks, I understand your point that I have provided somewhat conflicting
advice there. Sorry about that.

> > I will also say that I am surprised to see this driver using
> > full licence preambles rather than SPDX headers. I assume that
> > is due to direction from legal. And if so, I accept that you may not
> > be in a position to change this. But my comment stands.
> 
> This is done to ensure xgbe-smn.h license match with license in all the
> other files.

Yes, understood.

I think that it owuld be ideal if, as an activity separate to this
patch-set, some work was done to use SPDX for this driver.