Message-ID: <20250115111042.2bf22b61@fedora.home>
Date: Wed, 15 Jan 2025 11:10:42 +0100
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Tristram.Ha@...rochip.com, Woojung Huh <woojung.huh@...rochip.com>,
 Andrew Lunn <andrew@...n.ch>, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo
 Abeni <pabeni@...hat.com>, UNGLinuxDriver@...rochip.com,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support
 to KSZ9477 switch

Hello Vlad, Tristram,

I'm replying to Vlad's review as he correctly points that this looks
very much like XPCS :)

On Tue, 14 Jan 2025 18:09:08 +0200
Vladimir Oltean <olteanv@...il.com> wrote:

> On Mon, Jan 13, 2025 at 06:47:04PM -0800, Tristram.Ha@...rochip.com wrote:
> > From: Tristram Ha <tristram.ha@...rochip.com>
> > 
> > The SGMII module of KSZ9477 switch can be setup in 3 ways: direct connect
> > without using any SFP, SGMII mode with 10/100/1000Base-T SFP, and SerDes
> > mode with 1000BaseX SFP, which can be fiber or copper.  Note some
> > 1000Base-T copper SFPs advertise themselves as SGMII but start in
> > 1000BaseX mode, and the PHY driver of the PHY inside will change it to
> > SGMII mode.
> > 
> > The SGMII module can only support basic link status of the SFP, so the
> > port can be simulated as having a regular internal PHY when SFP cage
> > logic is not present.  The original KSZ9477 evaluation board operates in
> > this way and so requires the simulation code.  
> 
> I don't follow what you are saing here. What is the basic link status of
> the SFP? It is expected of a SGMII PCS to be able to report:
> - the "link up" indication
> - the "autoneg complete" indication
> - for SGMII: the speed and duplex communicated by the PHY, if in-band
>   mode is selected and enabled
> - for 1000Base-X: the duplex and pause bits communicated by the link
>   partner, if in-band mode is selected and enabled.
> 
> What, out of these, is missing? I'm mostly confused about the reference
> to the SFP here. The SGMII PCS shouldn't care whether the link goes
> through an SFP module or not. It just reports the above things. Higher
> layer code (the SFP bus driver) determines if the SFP module wants to
> use SGMII or 1000Base-X, based on its EEPROM contents.
> 
> Essentially I don't understand the justification for simulating an
> internal phylib phy. Why would the SFP cage logic (I assume you mean
> CONFIG_SFP) be missing? If you have a phylink-based driver, you have to
> have that enabled if you have SFP cages on your board.
> 
> > A PCS driver for the SGMII port is provided to support the SFP cage
> > logic used in the phylink code.  It is used to confirm the link is up
> > and process the SGMII interrupt.
> > 
> > One issue for the 1000BaseX SFP is there is no link down interrupt, so
> > the driver has to use polling to detect link off when the link is up.
> > 
> > Note the SGMII interrupt cannot be masked in hardware.  Also the module
> > is not reset when the switch is reset.  It is important to reset the
> > module properly to make sure interrupt is not triggered prematurely.
> > 
> > One side effect is the SGMII interrupt is triggered when an internal PHY
> > is powered down and powered up.  This happens when a port using internal
> > PHY is turned off and then turned on.
> > 
> > Signed-off-by: Tristram Ha <tristram.ha@...rochip.com>
> > ---
> > v2
> >  - use standard MDIO names when programming MMD registers
> >  - use pcs_config API to setup SGMII module
> >  - remove the KSZ9477 device tree example as it was deemed unnecessary
> > 
> >  drivers/net/dsa/microchip/ksz9477.c     | 455 +++++++++++++++++++++++-
> >  drivers/net/dsa/microchip/ksz9477.h     |   9 +-
> >  drivers/net/dsa/microchip/ksz9477_reg.h |   1 +
> >  drivers/net/dsa/microchip/ksz_common.c  | 111 +++++-
> >  drivers/net/dsa/microchip/ksz_common.h  |  23 +-
> >  5 files changed, 588 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> > index 29fe79ea74cd..3613eea1e3fb 100644
> > --- a/drivers/net/dsa/microchip/ksz9477.c
> > +++ b/drivers/net/dsa/microchip/ksz9477.c
> > @@ -2,7 +2,7 @@
> >  /*
> >   * Microchip KSZ9477 switch driver main logic
> >   *
> > - * Copyright (C) 2017-2024 Microchip Technology Inc.
> > + * Copyright (C) 2017-2025 Microchip Technology Inc.
> >   */
> >  
> >  #include <linux/kernel.h>
> > @@ -12,6 +12,8 @@
> >  #include <linux/phy.h>
> >  #include <linux/if_bridge.h>
> >  #include <linux/if_vlan.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/phylink.h>
> >  #include <net/dsa.h>
> >  #include <net/switchdev.h>
> >  
> > @@ -161,6 +163,415 @@ static int ksz9477_wait_alu_sta_ready(struct ksz_device *dev)
> >  					10, 1000);
> >  }
> >  
> > +static void port_sgmii_s(struct ksz_device *dev, uint port, u16 devid, u16 reg,
> > +			 u16 len)
> > +{
> > +	u32 data;
> > +
> > +	data = (devid & MII_MMD_CTRL_DEVAD_MASK) << 16;
> > +	data |= reg;
> > +	if (len > 1)
> > +		data |= PORT_SGMII_AUTO_INCR;
> > +	ksz_pwrite32(dev, port, REG_PORT_SGMII_ADDR__4, data);
> > +}
> > +
> > +static void port_sgmii_r(struct ksz_device *dev, uint port, u16 devid, u16 reg,
> > +			 u16 *buf, u16 len)
> > +{
> > +	u32 data;
> > +
> > +	mutex_lock(&dev->sgmii_mutex);
> > +	port_sgmii_s(dev, port, devid, reg, len);
> > +	while (len) {
> > +		ksz_pread32(dev, port, REG_PORT_SGMII_DATA__4, &data);
> > +		*buf++ = (u16)data;
> > +		len--;
> > +	}
> > +	mutex_unlock(&dev->sgmii_mutex);
> > +}
> > +
> > +static void port_sgmii_w(struct ksz_device *dev, uint port, u16 devid, u16 reg,
> > +			 u16 *buf, u16 len)
> > +{
> > +	u32 data;
> > +
> > +	mutex_lock(&dev->sgmii_mutex);
> > +	port_sgmii_s(dev, port, devid, reg, len);
> > +	while (len) {
> > +		data = *buf++;
> > +		ksz_pwrite32(dev, port, REG_PORT_SGMII_DATA__4, data);
> > +		len--;
> > +	}
> > +	mutex_unlock(&dev->sgmii_mutex);
> > +}

These helpers can be converted into mii_bus read_c45/write_c45 mdio
accessors, which would be the first step towards using xpcs here.

[...]
  
> > +static void ksz_parse_sgmii(struct ksz_device *dev, int port,
> > +			    struct device_node *dn)
> > +{
> > +	const char *managed;
> > +
> > +	if (dev->info->sgmii_port != port + 1)
> > +		return;
> > +	/* SGMII port can be used without using SFP.
> > +	 * The sfp declaration is returned as being a fixed link so need to
> > +	 * check the managed string to know the port is not using sfp.
> > +	 */
> > +	if (of_phy_is_fixed_link(dn) &&
> > +	    of_property_read_string(dn, "managed", &managed))
> > +		dev->ports[port].interface = PHY_INTERFACE_MODE_INTERNAL;
> > +}  
> 
> There is way too much that seems unjustifiably complex at this stage,
> including this. I would like to see a v3 using xpcs + the remaining
> required delta for ksz9477, ideally with no internal PHY simulation.
> Then we can go from there.
> 
> Also please make sure to keep linux@...linux.org.uk in cc for future
> submissions of this feature.

I mentionned on the previous iteration that there's indeed a DW XPCS in
there :
https://lore.kernel.org/netdev/20241129135919.57d59c90@fedora.home/

I have access to a platform with a KSZ9477, and indeed the PHY id
register for the PCS mdio device show the DW XPCS id.

I've been able to get this serdes port working with the XPCS driver
(although on 6.1 due to project constraints), although I couldn't get
1000BaseX autoneg to work.

So all in all I agree with Vlad's comments here, there's a lot of logic
in this series to detect the phy_interface_mode, detect SFP or not,
most of which isn't needed.

The logic should boil down to :

 - Create some helpers to access the PCS through a virtual mdio bus
(basically the current port_sgmii_w/r)

 - Register a virtual mdio bus to access the PCS, hooked in
ksz9477_port_setup() for the serdes port. That would look something
like this :

+       bus = devm_mdiobus_alloc(ds->dev);
(...)
+       bus->read_c45 = ksz9477_sgmii_read;
+       bus->write_c45 = ksz9477_sgmii_write;
(...)
+       ret = devm_mdiobus_register(ds->dev, bus);
+       if (ret)
+               (...)
+
+       port->xpcs = xpcs_create_mdiodev(bus, 0, <iface>);

- Make sure that .phylink_select_pcs() returns a ref to that xpcs

- Write the necessary ksz9477-specific glue logic (adjust the phylink capabilities,
make sure the virual MDIO registers are un the regmap area, etc.)

I will be happy to test any further iterations :)

Thanks,

Maxime

Powered by blists - more mailing lists