Re: [PATCH net-next v12 09/15] net: stmmac: dwmac-loongson: Add phy_interface for Loongson GMAC

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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 (it
does get used by a truck load of platform specific code though
which I haven't looked at to answer this.)

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.

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!