Message-ID: <aBsadO2IB_je91Jx@shell.armlinux.org.uk>
Date: Wed, 7 May 2025 09:31:48 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc: Tristram.Ha@...rochip.com, Andrew Lunn <andrew@...n.ch>,
	Woojung Huh <woojung.huh@...rochip.com>,
	Vladimir Oltean <olteanv@...il.com>,
	Heiner Kallweit <hkallweit1@...il.com>,
	"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

On Wed, May 07, 2025 at 09:44:49AM +0200, Maxime Chevallier wrote:
> Hi Tristram,
> 
> On Tue, 6 May 2025 17:09:11 -0700
> <Tristram.Ha@...rochip.com> wrote:
> 
> > From: Tristram Ha <tristram.ha@...rochip.com>
> > 
> > The KSZ9477 switch driver uses the XPCS driver to operate its SGMII
> > port.  However there are some hardware bugs in the KSZ9477 SGMII
> > module so workarounds are needed.  There was a proposal to update the
> > XPCS driver to accommodate KSZ9477, but the new code is not generic
> > enough to be used by other vendors.  It is better to do all these
> > workarounds inside the KSZ9477 driver instead of modifying the XPCS
> > driver.
> > 
> > There are 3 hardware issues.  The first is the MII_ADVERTISE register
> > needs to be write once after reset for the correct code word to be
> > sent.  The XPCS driver disables auto-negotiation first before
> > configuring the SGMII/1000BASE-X mode and then enables it back.  The
> > KSZ9477 driver then writes the MII_ADVERTISE register before enabling
> > auto-negotiation.  In 1000BASE-X mode the MII_ADVERTISE register will
> > be set, so KSZ9477 driver does not need to write it.
> > 
> > The second issue is the MII_BMCR register needs to set the exact speed
> > and duplex mode when running in SGMII mode.  During link polling the
> > KSZ9477 will check the speed and duplex mode are different from
> > previous ones and update the MII_BMCR register accordingly.
> > 
> > The last issue is 1000BASE-X mode does not work with auto-negotiation
> > on.  The cause is the local port hardware does not know the link is up
> > and so network traffic is not forwarded.  The workaround is to write 2
> > additional bits when 1000BASE-X mode is configured.
> > 
> > Note the SGMII interrupt in the port cannot be masked.  As that
> > interrupt is not handled in the KSZ9477 driver the SGMII interrupt bit
> > will not be set even when the XPCS driver sets it.
> >
> > Signed-off-by: Tristram Ha <tristram.ha@...rochip.com>
> 
> [...]
> 
> > +
> > +static int ksz9477_pcs_read(struct mii_bus *bus, int phy, int mmd, int reg)
> > +{
> > +	struct ksz_device *dev = bus->priv;
> > +	int port = ksz_get_sgmii_port(dev);
> > +	u16 val;
> > +
> > +	port_sgmii_r(dev, port, mmd, reg, &val);
> > +
> > +	/* Simulate a value to activate special code in the XPCS driver if
> > +	 * supported.
> > +	 */
> > +	if (mmd == MDIO_MMD_PMAPMD) {
> > +		if (reg == MDIO_DEVID1)
> > +			val = 0x9477;
> > +		else if (reg == MDIO_DEVID2)
> > +			val = 0x22 << 10;
> > +	} else if (mmd == MDIO_MMD_VEND2) {
> > +		struct ksz_port *p = &dev->ports[port];
> > +
> > +		/* Need to update MII_BMCR register with the exact speed and
> > +		 * duplex mode when running in SGMII mode and this register is
> > +		 * used to detect connected speed in that mode.
> > +		 */
> > +		if (reg == MMD_SR_MII_AUTO_NEG_STATUS) {
> > +			int duplex, speed;
> > +
> > +			if (val & SR_MII_STAT_LINK_UP) {
> > +				speed = (val >> SR_MII_STAT_S) & SR_MII_STAT_M;
> > +				if (speed == SR_MII_STAT_1000_MBPS)
> > +					speed = SPEED_1000;
> > +				else if (speed == SR_MII_STAT_100_MBPS)
> > +					speed = SPEED_100;
> > +				else
> > +					speed = SPEED_10;
> > +
> > +				if (val & SR_MII_STAT_FULL_DUPLEX)
> > +					duplex = DUPLEX_FULL;
> > +				else
> > +					duplex = DUPLEX_HALF;
> > +
> > +				if (!p->phydev.link ||
> > +				    p->phydev.speed != speed ||
> > +				    p->phydev.duplex != duplex) {
> > +					u16 ctrl;
> > +
> > +					p->phydev.link = 1;
> > +					p->phydev.speed = speed;
> > +					p->phydev.duplex = duplex;
> > +					port_sgmii_r(dev, port, mmd, MII_BMCR,
> > +						     &ctrl);
> > +					ctrl &= BMCR_ANENABLE;
> > +					ctrl |= mii_bmcr_encode_fixed(speed,
> > +								      duplex);
> > +					port_sgmii_w(dev, port, mmd, MII_BMCR,
> > +						     ctrl);
> > +				}
> > +			} else {
> > +				p->phydev.link = 0;
> > +			}
> > +		} else if (reg == MII_BMSR) {
> > +			p->phydev.link = (val & BMSR_LSTATUS);
> > +		}
> > +	}
> > +	return val;
> > +}
> > +
> > +static int ksz9477_pcs_write(struct mii_bus *bus, int phy, int mmd, int reg,
> > +			     u16 val)
> > +{
> > +	struct ksz_device *dev = bus->priv;
> > +	int port = ksz_get_sgmii_port(dev);
> > +
> > +	if (mmd == MDIO_MMD_VEND2) {
> > +		struct ksz_port *p = &dev->ports[port];
> > +
> > +		if (reg == MMD_SR_MII_AUTO_NEG_CTRL) {
> > +			u16 sgmii_mode = SR_MII_PCS_SGMII << SR_MII_PCS_MODE_S;
> > +
> > +			/* Need these bits for 1000BASE-X mode to work with
> > +			 * AN on.
> > +			 */
> > +			if (!(val & sgmii_mode))
> > +				val |= SR_MII_SGMII_LINK_UP |
> > +				       SR_MII_TX_CFG_PHY_MASTER;
> > +
> > +			/* SGMII interrupt in the port cannot be masked, so
> > +			 * make sure interrupt is not enabled as it is not
> > +			 * handled.
> > +			 */
> > +			val &= ~SR_MII_AUTO_NEG_COMPLETE_INTR;
> > +		} else if (reg == MII_BMCR) {
> > +			/* The MII_ADVERTISE register needs to write once
> > +			 * before doing auto-negotiation for the correct
> > +			 * config_word to be sent out after reset.
> > +			 */
> > +			if ((val & BMCR_ANENABLE) && !p->sgmii_adv_write) {
> > +				u16 adv;
> > +
> > +				/* The SGMII port cannot disable flow contrl
> > +				 * so it is better to just advertise symmetric
> > +				 * pause.
> > +				 */
> > +				port_sgmii_r(dev, port, mmd, MII_ADVERTISE,
> > +					     &adv);
> > +				adv |= ADVERTISE_1000XPAUSE;
> > +				adv &= ~ADVERTISE_1000XPSE_ASYM;
> > +				port_sgmii_w(dev, port, mmd, MII_ADVERTISE,
> > +					     adv);
> > +				p->sgmii_adv_write = 1;
> > +			} else if (val & BMCR_RESET) {
> > +				p->sgmii_adv_write = 0;
> > +			}
> > +		} else if (reg == MII_ADVERTISE) {
> > +			/* XPCS driver writes to this register so there is no
> > +			 * need to update it for the errata.
> > +			 */
> > +			p->sgmii_adv_write = 1;
> > +		}
> > +	}
> > +	port_sgmii_w(dev, port, mmd, reg, val);
> > +	return 0;
> > +}
> 
> I'm a bit confused here, are you intercepting r/w ops that are supposed
> to be handled by xpcs ?
> 
> Russell has sent a series [1] (not merged yet, I think we were waiting
> on some feedback from Synopsys folks ?) to properly support the XPCS
> version that's in KSZ9477, and you also had a patchset that didn't
> require all this sgmii_r/w snooping [2].
> 
> I've been running your previous patchset on top of Russell's for a few
> months, if works fine with SGMII as well as 1000BaseX :)
> 
> Can we maybe focus on getting pcs-xpcs to properly support this version
> of the IP instead of these 2 R/W functions ? Or did I miss something in
> the previous discussions ?

Honestly, I don't think Tristram is doing anything unreasonable here,
given what Vladimir has been saying. Essentially, we've been blocking
a way forward on the pcs-xpcs driver. We've had statements from the
hardware designers from Microchip. We've had statements from Synopsys.
The two don't quite agree, but that's not atypical. Yet, we're still
demanding why the Microchip version of XPCS is different.

So what's left for Tristram to do other than to hack around the blockage
we're causing by intercepting the read/write ops and bodging them.

As I understand the situation, this is Jose's response having asked
internally at my request:

https://lore.kernel.org/netdev/DM4PR12MB5088BA650B164D5CEC33CA08D3E82@DM4PR12MB5088.namprd12.prod.outlook.com/

To put it another way, as far as Synopsys can tell us, they are unaware
of the Microchip behaviour, but customers can modify the Synopsys IP.

Maybe Microchip's version is based on an old Synopsys version, but
which was modified by Microchip a long time ago and those engineers
have moved on, and no one really knows anymore. I doubt that we are
ever going to get to the bottom of the different behaviour.

So, what do we do now? Do we continue playing hardball and basically
saying "no" to changing the XPCS driver, demanding information that
doesn't seem to exist anymore? Or do we try to come up with an
approach that works.

I draw attention to the last sentence in Jose's quote in his reply.
As far as the Synopsys folk are concerned, setting these bits to 1
should have no effect provided there aren't customer modifications to
the IP that depend on these being set to zero.

That last bit is where I think the sticking point between Vladimir and
myself is - I'm in favour of keeping things simple and just setting
the bits. Vladimir feels it would be safer to make it conditional,
which leads to more complicated code.

I didn't progress my series because I decided it was a waste of time
to try and progress this any further - I'd dug up the SJA1105 docs to
see what they said, I'd reached out to Synopsys and got a statement
back, and still Vladimir wasn't happy.

With Vladimir continuing to demand information from Tristram that just
didn't exist, I saw that the

[rest of the email got deleted because Linux / X11 / KDE got confused
about the state the backspace key and decided it was going to be
continuously pressed and doing nothing except shutting the laptop
down would stop it.]

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists