Message-ID: <eycodyo57suhzb4jgxjn5fmltzxogo6nszgnkvgak6lqarsw72@lz47ughdxy5r>
Date: Sat, 4 May 2024 00:01:42 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>, 
	Yanteng Si <siyanteng@...ngson.cn>
Cc: Yanteng Si <siyanteng@...ngson.cn>, andrew@...n.ch, 
	hkallweit1@...il.com, peppe.cavallaro@...com, alexandre.torgue@...s.st.com, 
	joabreu@...opsys.com, Jose.Abreu@...opsys.com, chenhuacai@...nel.org, 
	guyinggang@...ngson.cn, netdev@...r.kernel.org, chris.chenfeiyang@...il.com, 
	siyanteng01@...il.com
Subject: Re: [PATCH net-next v12 09/15] net: stmmac: dwmac-loongson: Add
 phy_interface for Loongson GMAC

On Fri, Apr 26, 2024 at 12:00:25PM +0100, Russell King (Oracle) wrote:
> On Fri, Apr 26, 2024 at 06:16:42PM +0800, Yanteng Si wrote:
> > Hi Russell,
> > 
> > 在 2024/4/25 22:36, Russell King (Oracle) 写道:
> > > > The mac_interface of gmac is PHY_INTERFACE_MODE_RGMII_ID.
> > > No it isn't!
> > Ok, that's a typo.
> > > 
> > > > +	plat->phy_interface = PHY_INTERFACE_MODE_RGMII_ID;
> > > You don't touch mac_interface here. Please read the big comment I put
> > > in include/linux/stmmac.h above these fields in struct
> > > plat_stmmacenet_data to indicate what the difference between
> > > mac_interface and phy_interface are, and then correct which-ever
> > > of the above needs to be corrected.
> > 
> > Copy your big comment here:
> > 
> >     int phy_addr;
> >     /* MAC ----- optional PCS ----- SerDes ----- optional PHY ----- Media
> >      *       ^                               ^
> >      * mac_interface                   phy_interface
> >      *
> >      * mac_interface is the MAC-side interface, which may be the same
> >      * as phy_interface if there is no intervening PCS. If there is a
> >      * PCS, then mac_interface describes the interface mode between the
> >      * MAC and PCS, and phy_interface describes the interface mode
> >      * between the PCS and PHY.
> >      */
> >     phy_interface_t mac_interface;
> >     /* phy_interface is the PHY-side interface - the interface used by
> >      * an attached PHY.
> >      */
> > 
> > Our hardware engineer said we don't support pcs, and if I understand
> > 
> > your comment correctly, our mac_interface and phy_interface should
> > 
> > be the same, right?
> 

> It only matters to the core code if priv->dma_cap.pcs is set true.
> If it isn't, then mac_interface doesn't seem to be relevent

Absolutely correct. Moreover AFAICS from the databooks the PCS module
is only available for the DW GMACs with the TBI, RTBI and SGMII
interfaces. So the STMMAC_PCS_RGMII PCS flag seems misleading. The only
info about the link state that could be runtime detected on the MAC
side is: link speed, duplex mode and link status. It's done by means
of the RGMII in-band status signals. That's what is implemented in the
dwmac1000_rgsmii() method. I've checked it works as soon as the
GMAC_INT_DISABLE_RGMII IRQ is unmask. But the RGMII-capable DW GMACs
don't support the Auto-negotiation feature, because it has no PCS
inside. Thus what has been implemented for the RGMII case in
stmmac_ethtool_get_link_ksettings() and
stmmac_ethtool_set_link_ksettings() seems incorrect to me.

> (it
> does get used by a truck load of platform specific code though
> which I haven't looked at to answer this.)

IMO the most of them are using the plat_stmmacenet_data::mac_interface
field as just an additional storage of the PHY-interface type. The
only cases which I would consider as validly using the field are the
ones with the SGMII interface support (and TBI/RTBI).

BTW I see the "mac-mode" DT-property support was introduced in 2019 by
the commit 0060c8783330 ("net: stmmac: implement support for passive
mode converters via dt"). The commit didn't touch any of the platform
drivers, but merely changed the plat_stmmacenet_data::mac_interface
semantics to containing the intermediate interface type if the
property was specified. So IMO all the platform drivers which had been
available by the time the change was introduced can be converted to
using the plat_stmmacenet_data::phy_interface field with a small
probability to break things.

I can't help but notice that since that commit there have been no any
DW MAC DT-node introduced with the "mac-mode" property specified. So
all of that makes me thinking that the code implemented around the
mac_interface data has been mainly unused and most likely was needless
overcomplication. Sigh...

> 
> I would suggest that if priv->dma_cap.pcs is false, then setting
> mac_interface to PHY_INTERFACE_MODE_NA to make it explicit that
> it's not used would be a good idea.

I bet it will be false. But Yanteng, could you please double check
that?

If it is could you convert this patch to setting
plat_stmmacenet_data::mac_interface with PHY_INTERFACE_MODE_NA by
default for all your device and setting the
plat_stmmacenet_data::phy_interface with PHY_INTERFACE_MODE_RGMII_ID
for the Loongson GMAC devices?

BTW Yanteng, are you sure it's supposed to
PHY_INTERFACE_MODE_RGMII_ID? AFAICS from the Loongson DTS files
(loongson64-2k1000.dtsi, ls7a-pch.dtsi) the phy-mode is just
PHY_INTERFACE_MODE_RGMII with no internal delays.

-Serge(y)

> 
> While looking at this, however, I've come across the fact that
> stmmac manipulates the netif carrier via netif_carrier_off() and
> netif_carrier_on(), which is a _big_ no no when using phylink.
> Phylink manages the carrier for the driver, and its part of
> phylink's state. Fiddling with the carrier totally breaks the
> guarantee that phylink will make calls to mac_link_down() and
> mac_link_up().
> 
> If a driver wants to fiddle with the netif carrier, it must NOT
> use phylink. If it wants to use phylink, it must NOT fiddle with
> the netif carrier state. The two are mutually exclusive.
> 
> stmmac is quickly becoming a driver I don't care about whether my
> changes to phylink end up breaking it or not because of abuses
> like this.
> 
> -- 
> 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