Re: [PATCH RFC net-next 2/3] net: stmmac: Activate Inband/PCS flag based on the selected iface

[Serge Semin]

On Sun, May 26, 2024 at 07:00:22PM +0100, Russell King (Oracle) wrote:
> On Sun, May 26, 2024 at 05:49:48PM +0100, Russell King (Oracle) wrote:
> > On Sat, May 25, 2024 at 12:02:58AM +0300, Serge Semin wrote:
> > > The HWFEATURE.PCSSEL flag is set if the PCS block has been synthesized
> > > into the DW GMAC controller. It's always done if the controller supports
> > > at least one of the SGMII, TBI, RTBI PHY interfaces. If none of these
> > > interfaces support was activated during the IP-core synthesize the PCS
> > > block won't be activated either and the HWFEATURE.PCSSEL flag won't be
> > > set. Based on that the RGMII in-band status detection procedure
> > > implemented in the driver hasn't been working for the devices with the
> > > RGMII interface support and with none of the SGMII, TBI, RTBI PHY
> > > interfaces available in the device.
> > > 
> > > Fix that just by dropping the dma_cap.pcs flag check from the conditional
> > > statement responsible for the In-band/PCS functionality activation. If the
> > > RGMII interface is supported by the device then the in-band link status
> > > detection will be also supported automatically (it's always embedded into
> > > the RGMII RTL code). If the SGMII interface is supported by the device
> > > then the PCS block will be supported too (it's unconditionally synthesized
> > > into the controller). The later is also correct for the TBI/RTBI PHY
> > > interfaces.
> > > 
> > > Note while at it drop the netdev_dbg() calls since at the moment of the
> > > stmmac_check_pcs_mode() invocation the network device isn't registered. So
> > > the debug prints will be for the unknown/NULL device.
> > 
> > Thanks. As this is a fix, shouldn't it be submitted for the net tree as
> > it seems to be fixing a bug in the driver as it stands today?
> > 
> > Also, a build fix is required here:
> > 
> > > -	if (priv->dma_cap.pcs) {
> > > -		if ((interface == PHY_INTERFACE_MODE_RGMII) ||
> > > -		    (interface == PHY_INTERFACE_MODE_RGMII_ID) ||
> > > -		    (interface == PHY_INTERFACE_MODE_RGMII_RXID) ||
> > > -		    (interface == PHY_INTERFACE_MODE_RGMII_TXID)) {
> > > -			netdev_dbg(priv->dev, "PCS RGMII support enabled\n");
> > > -			priv->hw->pcs = STMMAC_PCS_RGMII;
> > > -		} else if (interface == PHY_INTERFACE_MODE_SGMII) {
> > > -			netdev_dbg(priv->dev, "PCS SGMII support enabled\n");
> > > -			priv->hw->pcs = STMMAC_PCS_SGMII;
> > > -		}
> > > -	}
> > > +	if (phy_interface_mode_is_rgmii(interface))
> > > +		priv->hw.pcs = STMMAC_PCS_RGMII;
> > > +	else if (interface == PHY_INTERFACE_MODE_SGMII)
> > > +		priv->hw.pcs = STMMAC_PCS_SGMII;
> > 
> > Both of these assignments should be priv->hw->pcs not priv->hw.pcs.
> > 
> > I think there's also another bug that needs fixing along with this.
> > See stmmac_ethtool_set_link_ksettings(). Note that this denies the
> > ability to disable autoneg, which (a) doesn't make sense for RGMII
> > with an attached PHY, and (b) this code should be passing the
> > ethtool op to phylink for it to pass on to phylib so the PHY can
> > be appropriately configured for the users desired autoneg and
> > link mode settings.
> > 
> > I also don't think it makes any sense for the STMMAC_PCS_SGMII case
> > given that it means Cisco SGMII - which implies that there is also
> > a PHY (since Cisco SGMII with inband is designed to be coupled with
> > something that looks like a PHY to send the inband signalling
> > necessary to configure e.g. the SGMII link symbol replication.
> > 
> > In both of these cases, even if the user requests autoneg to be
> > disabled, that _shouldn't_ affect internal network driver links.
> > This ethtool op is about configuring the externally visible media
> > side of the network driver, not the internal links.
> 

> I have a concern about this patch. Have you considered dwmac-intel with
> its XPCS support, where the XPCS is used for Cisco SGMII and 1000base-X
> support. Does the dwmac-intel version of the core set
> priv->dma_cap.pcs? If it doesn't, then removing the test on this will
> cause a regression, since in Cisco SGMII mode, we end up setting
> priv->hw->pcs to SYMMAC_PCS_SGMII where we didn't before. As
> priv->flags will not have STMMAC_FLAG_HAS_INTEGRATED_PCS, this will
> enable all the "integrated PCS" code paths despite XPCS clearly
> intending to be used for Cisco SGMII.
> 
> I'm also wondering whether the same applies to the lynx PCS as well,
> or in the general case if we have any kind of external PCS.
> 
> Hence, I think this probably needs to be:
> 
> 	if (phy_interface_mode_is_rgmii(interface))
> 		priv->hw->pcs = STMMAC_PCS_RGMII;
> 	else if (interface == PHY_INTERFACE_MODE_SGMII && priv->dma_cap.pcs)
> 		priv->hw->pcs = STMMAC_PCS_SGMII;
> 
> At least this is what unpicking the awful stmmac code suggests (and I
> do feel that my point about the shocking state of this driver is proven
> as details like this are extremely difficult to unpick, and not
> unpicking them correctly will lead to regressions.) Therefore, I would
> suggest that it would be wise if you also double-checked this.

Double-checked that part. Indeed this is what I forgot to take into
account. (Just realized I had a glimpse thought about checking the DW
xGMAC/XPCS for supporting the SGMII interface, but the thought got
away from my mind forgotten.) DW XPCS can be synthesized with having
the GMII/MII interface connected to the MAC and SGMII downstream
interface over a single 1000Base-X lane.

In anyway AFAICS that case has nothing to do with the PCS embedded
into the DW GMAC or DW QoS Eth synthesized with the SGMII support. DW
XGMAC has no embedded PCS, but could be attached to the separate DW
XPCS device.

About the correct implementation. Right, priv->dma_cap.pcs indicates
that there is an embedded PCS and the flag can be set for DW GMAC or DW
QoS Eth only. Although I would change the order:

       if (phy_interface_mode_is_rgmii(interface))
               priv->hw->pcs = STMMAC_PCS_RGMII;
       else if (priv->dma_cap.pcs && interface == PHY_INTERFACE_MODE_SGMII)
               priv->hw->pcs = STMMAC_PCS_SGMII;

since priv->dma_cap.pcs is a primary flag. If it isn't set the
interface will be irrelevant.

Alternative solution could be to use the has_gmac/has_gmac4 flags
instead. That will emphasize that the embedded PCS is expected to be
specific for the DW GMAC and DW QoS Eth IP-cores:

       if (phy_interface_mode_is_rgmii(interface))
               priv->hw->pcs = STMMAC_PCS_RGMII;
       else if ((priv->plat.has_gmac || priv->plat.has_gmac4) &&
		interface == PHY_INTERFACE_MODE_SGMII)
               priv->hw->pcs = STMMAC_PCS_SGMII;

-Serge(y)

> 
> If my analysis is correct, then my changes to stmmac_mac_select_pcs()
> are also wrong.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!