Message-ID: <uw56ervtsa2uoovtsmoidkcfflhnwuv7cgj23qdebag3gry42n@inwwdqlbazg6>
Date: Sat, 26 Aug 2023 22:01:40 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Alexandre Torgue <alexandre.torgue@...s.st.com>, 
	Jose Abreu <joabreu@...opsys.com>, Andrew Lunn <andrew@...n.ch>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Feiyang Chen <chenfeiyang@...ngson.cn>, Heiner Kallweit <hkallweit1@...il.com>, 
	Jakub Kicinski <kuba@...nel.org>, linux-arm-kernel@...ts.infradead.org, 
	linux-stm32@...md-mailman.stormreply.com, Maxime Coquelin <mcoquelin.stm32@...il.com>, 
	netdev@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next 08/10] net: stmmac: move xgmac specific phylink
 caps to dwxgmac2 core

On Sat, Aug 26, 2023 at 03:51:53PM +0100, Russell King (Oracle) wrote:
> On Sat, Aug 26, 2023 at 04:32:15PM +0300, Serge Semin wrote:
> > On Thu, Aug 24, 2023 at 02:38:29PM +0100, Russell King (Oracle) wrote:
> > > Move the xgmac specific phylink capabilities to the dwxgmac2 support
> > > core.
> > > 
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
> > > ---
> > >  drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c | 10 ++++++++++
> > >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c   | 10 ----------
> > >  2 files changed, 10 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > > index 34e1b0c3f346..f352be269deb 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > > @@ -47,6 +47,14 @@ static void dwxgmac2_core_init(struct mac_device_info *hw,
> > >  	writel(XGMAC_INT_DEFAULT_EN, ioaddr + XGMAC_INT_EN);
> > >  }
> > >  
> > > +static void xgmac_phylink_get_caps(struct stmmac_priv *priv)
> > > +{
> > > +	priv->phylink_config.mac_capabilities |= MAC_2500FD | MAC_5000FD |
> > > +						 MAC_10000FD | MAC_25000FD |
> > > +						 MAC_40000FD | MAC_50000FD |
> > > +						 MAC_100000FD;
> > > +}
> > > +
> > >  static void dwxgmac2_set_mac(void __iomem *ioaddr, bool enable)
> > >  {
> > >  	u32 tx = readl(ioaddr + XGMAC_TX_CONFIG);
> > > @@ -1490,6 +1498,7 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, u32 num_txq,
> > >  
> > >  const struct stmmac_ops dwxgmac210_ops = {
> > >  	.core_init = dwxgmac2_core_init,
> > 
> > > +	.phylink_get_caps = xgmac_phylink_get_caps,
> > 
> > This doesn't look correct. DW XGMAC doesn't support 25/40/50/100Gbps
> > speeds.
> 
> So the reason this got added is to keep the code compatible with how
> things work today.
> 
> When priv->plat->has_xgmac is true, the old code in stmmac_phy_setup()
> would enable speeds from 2.5G up to 100G, limiting them if
> priv->plat->max_speed is set non-zero.

Indeed. I didn't consider it has been coded like that long before this
discussion.

> 
> The table in hwif.c matches when:
> 	entry->gmac == priv->plat->has_gmac,
> 	entry->gmac4 == priv->plat->has_gmac4 and
> 	entry->xgmac == priv->plat->has_xgmac
> 
> The entries in the table which patch on has_xgmac = true contain the
> following:
> 
>                 .mac = &dwxgmac210_ops,
>                 .mac = &dwxlgmac2_ops,
> 
> Therefore, to keep things compatible, I've effectively moved this
> initialisation into the new .phylink_get_caps method that is part of
> those two ops, and since they have has_xgmac true, this means that
> all these speeds need to be set.
> 
> We do this without regard to max_speed, which we apply separately,
> after the .phylink_get_caps method has returned.
> 
> So, the code is functionally identical to what happens in the driver,
> even if it is the case that xgmac210 doesn't actually support the
> speeds. If those extra speeds that the hardware doesn't support were
> present before, they're present after. If those extra speeds are
> limited by the max_speed, then they will be similarly limited.

> 
> While it may look odd, since the specifications for Synopsys are all
> behind closed doors, all I can do is transform the code - I can't
> know that such-and-such a core doesn't actually support stuff. So
> my only option is to keep the code bug-compatible.
> 

If I see odd things and have no specs at hand I normally dig into the
git log in order to find a respective commit, then get to finding the
corresponding lkml discussion which may have some clue of possible
oddness justification. In this case the problematic part was added in
the commit 8a880936e902 ("net: stmmac: Add XLGMII support"). So before
having the XLGMAC support added the DW XGMAC had had standard speeds
support. Seeing there were no discussion concerning that part of the
code within the review it was very likely wrong to add the higher
speeds support to the DW XMGAC controller. But of course with no
databook at hand it gets to be still an assumption but with high
probability to be true though.

> I think all I've done here is make it glaringly obvious what the old
> code is doing and you've spotted "but that isn't right!" - which is
> actually a good thing!
> 
> Feel free to submit patches to correct the functionality as bugs in
> the driver become more obvious!

I see your point. Ok I'll add such fix to my extensive collection of
the STMMAC driver bug-fixes.) I'll send it out to the community
eventually when I get to have more spare time for review.

-Serge(y)

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