Message-ID:
 <DM3PR11MB87367ED6C8963A2FEB4C349BEC1B2@DM3PR11MB8736.namprd11.prod.outlook.com>
Date: Fri, 17 Jan 2025 00:51:20 +0000
From: <Tristram.Ha@...rochip.com>
To: <olteanv@...il.com>
CC: <Woojung.Huh@...rochip.com>, <andrew@...n.ch>, <davem@...emloft.net>,
	<edumazet@...gle.com>, <kuba@...nel.org>, <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

> 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.

I explained that the KSZ9477 board that is used to verify DSA driver does
not have SFP cage hardware logic so that the EEPROM can be read through
I2C.  The SGMII hardware implementation is used on other boards that do
not use Linux so it is not always required to have that logic.

I do not know whether customers followed that example and setup the
hardware that way.

Anyway the PHY simulation code will be removed and the code will be kept
for internal use only if it is not desirable.

> > +     port_sgmii_r(dev, p, MDIO_MMD_VEND2, MII_BMSR, &status, 1);
> > +     port_sgmii_r(dev, p, MDIO_MMD_VEND2, MII_BMSR, &status, 1);
> > +     port_sgmii_r(dev, p, MDIO_MMD_VEND2, MMD_SR_MII_AUTO_NEG_STATUS,
> &data,
> > +                  1);
> > +
> > +     /* Typical register values with different SFPs.
> > +      * 10/100/1000: 1f0001 = 01ad  1f0005 = 4000  1f8002 = 0008
> > +      *              1f0001 = 01bd  1f0005 = d000  1f8002 = 001a
> > +      * 1000:        1f0001 = 018d  1f0005 = 0000  1f8002 = 0000
> > +      *              1f0001 = 01ad  1f0005 = 40a0  1f8002 = 0001
> > +      *              1f0001 = 01ad  1f0005 = 41a0  1f8002 = 0001
> > +      * fiber:       1f0001 = 0189  1f0005 = 0000  1f8002 = 0000
> > +      *              1f0001 = 01ad  1f0005 = 41a0  1f8002 = 0001
> 
> Hmm, these registers look extremely similar to the DW XPCS.
> 1f8002 => DW_VR_MII_AN_INTR_STS. Why don't you implement a virtual MDIO
> bus for accessing the XPCS registers at MDIO_MMD_VEND2 (0x1f_0000 +
> register address) and let drivers/net/pcs/pcs-xpcs.c handle the logic?
> 
> It will be better for everybody to understand what is the special
> handling that your hardware integration needs, when it is relative to
> the common driver.
> 
> You can look at sja1105 and its xpcs handling for an example of just this.

The XPCS driver available in the kernel does not work in KSZ9477 as the
SGMII hardware implementation is probably too old and so is not
compatible.  Link detection works but the SGMII port does not pass
traffic for some SFPs.  That is the reason that XPCS driver is not used.

> > +int ksz9477_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
> > +                    phy_interface_t interface,
> > +                    const unsigned long *advertising)
> > +{
> > +     struct ksz_pcs *priv = container_of(pcs, struct ksz_pcs, pcs);
> > +     struct ksz_device *dev = priv->dev;
> > +     struct ksz_port *p = &dev->ports[priv->port];
> > +
> > +     if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
> > +             p->pcs_priv->using_sfp = 1;
> 
> When neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED, it does not mean that
> we are using an SFP. We can have an on-board SGMII PHY which requires
> PHYLINK_PCS_NEG_INBAND_ENABLED as well.
> 
> Generally speaking, this implementation seems to depend on aspects which
> it really shouldn't be concern about.

It is just to confirm whether SFP cage code is used in the phylink
driver or not as the device tree can specify using managed sfp.

> > +
> > +     if (dev->info->sgmii_port == port + 1) {
> 
> Can you use a different representation for "doesn't have an SGMII port"
> than "dev->info->sgmii_port == 0"? You can hide it behind a wrapper like
> ksz_port_is_sgmii(). It is confusing and error-prone to have comparisons
> against port + 1 everywhere, as well as to set 7 in the info structure
> when in reality its index is 6.

Will update that.
The SGMII port is specified in the device info block because it can be a
different port in a different chip like LAN9374.
 
> > +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.

This is used to accommodate direct mode as the SGMII port can be
connected directly with no SFP in between and so will never lose link.

> Also please make sure to keep linux@...linux.org.uk in cc for future
> submissions of this feature.

Noted.

Powered by blists - more mailing lists