Subject: Re: [PATCH RFC net-next v2 14/17] net: stmmac: Move internal PCS PHYLINK ops to stmmac_pcs.c

From: "Russell King (Oracle)" <[email-masked]>
Date: Wed, 3 Jul 2024 21:07:22 +0100

On Wed, Jul 03, 2024 at 10:08:16PM +0300, Serge Semin wrote:
> On Fri, Jun 28, 2024 at 04:07:46PM +0100, Russell King (Oracle) wrote:
> > On Mon, Jun 24, 2024 at 04:26:31PM +0300, Serge Semin wrote:
> > > @@ -621,7 +548,6 @@ int dwmac1000_setup(struct stmmac_priv *priv)
> > >  	mac->mii.clk_csr_shift = 2;
> > >  	mac->mii.clk_csr_mask = GENMASK(5, 2);
> > >  
> > > -	mac->mac_pcs.ops = &dwmac1000_mii_pcs_ops;
> > >  	mac->mac_pcs.neg_mode = true;
> > 
> > "mac->mac_pcs.neg_mode = true;" is a property of the "ops" so should
> > move with it.
> > 
> > > @@ -1475,7 +1396,6 @@ int dwmac4_setup(struct stmmac_priv *priv)
> > >  	mac->mii.clk_csr_mask = GENMASK(11, 8);
> > >  	mac->num_vlan = dwmac4_get_num_vlan(priv->ioaddr);
> > >  
> > > -	mac->mac_pcs.ops = &dwmac4_mii_pcs_ops;
> > >  	mac->mac_pcs.neg_mode = true;
> > 
> > Also applies here.
> > 
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > > index 3666893acb69..c42fb2437948 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > > @@ -363,6 +363,7 @@ int stmmac_hwif_init(struct stmmac_priv *priv)
> > >  		mac->tc = mac->tc ? : entry->tc;
> > >  		mac->mmc = mac->mmc ? : entry->mmc;
> > >  		mac->est = mac->est ? : entry->est;
> > > +		mac->mac_pcs.ops = mac->mac_pcs.ops ?: entry->pcs;
> > 
> 
> > Removing both of the above means that mac->mac_pcs.ops won't ever be set
> > prior to this, so this whole thing should just be:
> > 
> > 		mac->mac_pcs.ops = entry->pcs;
> > 		mac->mac_pcs.neg_mode = true;
> 
> Actually, no. mac->mac_pcs.ops can be set by the platform-specific
> plat_stmmacenet_data::setup() method.

mac->mac_pcs is there for the _internal_ MAC only, not for platforms
to fiddle around with (remember, my patch set adds this!)

I think you're thinking of mac->phylink_pcs which platforms can and
do fiddle with.

> > > +	/* TODO Check the PCS_AN_STATUS.Link status here?.. Note the flag is latched-low */
> > > +
> > > +	/* TODO The next is the TBI/RTBI-specific and seems to be valid if PCS_AN_STATUS.ANC */
> > >  	val = readl(priv->pcsaddr + PCS_ANE_LPA);
> > 
> 
> > I thought these registers only existed of dma_cap.pcs is true ?
> 
> Right. The AN-registers are SGMII/TBI/RTBI-specific.

Therefore, I suggest that if state->interface is RGMII, then these
registers should not be accessed.

My idea is to provide two PCS per MAC:

One simple one which covers RGMII which only reads the PHYIF/RGSMIIIS
register, does no configuration, but does implement the .pcs_enable/
.pcs_disable etc. The .pcs_validate method should also be empty for
this because the AutoNeg ethtool capability does not refer to the
inband signalling, but to the media PHY.

Then a more complex PCS implementation that does everything the RGMII
one does, but also the bits for SGMII (and TBI/RTBI).

> > If we
> > start checking PCS_AN_STATUS.Link here, and this register reads as
> > zeros, doesn't it mean that RMGII inband mode won't ever signal link
> > up?
> 
> Right. The PCS_AN_STATUS.Link should be checked for the SGMII (and
> TBI/RTBI) only. The databooks defines the flag as follows:
> 
> DW GMAC v3.73a:
> Link Status   This bit indicates whether the data channel (link) is up or
> R_SS_SC_LLO   down. For the TBI, RTBI or SGMII interfaces, if ANEG is going
>               on, data cannot be transferred across the link and hence the
>               link is given as down.
> 
> DW QoS Eth:
> Link Status   When this bit is set, it indicates that the link is up between
> Read-only     the MAC and the TBI, RTBI, or SGMII interface. When this bit is
>               reset, it indicates that the link is down between the MAC and
>               the TBI, RTBI, or SGMII interface.
> 
> I guess that in fact the flag semantics is the same on both devices.
> But the Access-status for some reason different. Although DW QoS Eth
> databook doesn't define any latched-low CSR. So there is a chance that
> some of the databooks might be wrong in the flag access status.

Yes, it sounds like it.

> > > -	/* TODO Make sure that STMMAC_PCS_PAUSE STMMAC_PCS_ASYM_PAUSE usage is legitimate */
> > > +	/* TODO The databook says the encoding is defined in IEEE 802.3z,
> > > +	 * Section 37.2.1.4. Do we need the STMMAC_PCS_PAUSE and
> > > +	 * STMMAC_PCS_ASYM_PAUSE mask here?
> > > +	 */
> > >  	linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> > >  			 state->lp_advertising,
> > >  			 FIELD_GET(PCS_ANE_PSE, val) & STMMAC_PCS_PAUSE);
> > 
> 
> > If it's 802.3z aka 1000base-X format, then yes, we should be using
> > these bits if we are getting state from this register.
> 
> I meant that should we be using the driver-specific macro in here
> seeing the field encoding is defined by the IEEE 802.3z? Is there any
> ready-to-use macros/constants defined in the network subsystem core
> for the standard Pause encoding (IEEE 802.3z Section 37.2.1.4)?

include/uapi/linux/mii.h:

#define ADVERTISE_1000XFULL     0x0020  /* Try for 1000BASE-X full-duplex */
	/* GMAC_ANE_FD */
#define ADVERTISE_1000XHALF     0x0040  /* Try for 1000BASE-X half-duplex */
	/* GMAC_ANE_HD */
#define ADVERTISE_1000XPAUSE    0x0080  /* Try for 1000BASE-X pause    */
	/* GMAC_ANE_PSE bit 0 */
#define ADVERTISE_1000XPSE_ASYM 0x0100  /* Try for 1000BASE-X asym pause */
	/* GMAC_ANE_PSE bit 1 */
#define ADVERTISE_LPACK         0x4000  /* Ack link partners response  */
	/* GMAC_ANE_ACK */

#define LPA_1000XFULL           0x0020  /* Can do 1000BASE-X full-duplex */
	/* GMAC_ANE_FD */
#define LPA_1000XHALF           0x0040  /* Can do 1000BASE-X half-duplex */
	/* GMAC_ANE_HD */
#define LPA_1000XPAUSE          0x0080  /* Can do 1000BASE-X pause     */
	/* GMAC_ANE_PSE bit 0 */
#define LPA_1000XPAUSE_ASYM     0x0100  /* Can do 1000BASE-X pause asym*/
	/* GMAC_ANE_PSE bit 1 */
#define LPA_RESV                0x1000  /* Unused...                   */
	/* GMAC_ANE_RFE bit 0 */
#define LPA_RFAULT              0x2000  /* Link partner faulted        */
	/* GMAC_ANE_RFE bit 1 */
#define LPA_LPACK               0x4000  /* Link partner acked us       */
	/* GMAC_ANE_ACK */

> > If TBI/RTBI is ever used, rather than trying to shoe-horn it all into
> > these functions, please consider splitting them into separate PCSes,
> > and sharing code between them e.g. using common functions called from
> > the method functions or shared method functions where appropriate.
> 
> Ok. Sounds reasonable.
> 
> I guess your message also means that the patchset re-spinning will be
> on me from now, right?) If so, please note, I can't promise I'll be
> able to do that soonish. I am quite busy at the moment. I'll be
> more-or-less free for that in a month or so.

Not necessarily - some good news today, the high priority issue I was
working on is lower priority at last, which means I've more time to
look at mainline again. Bad news... I need a break after about 2.5
months of frustrations, which could be from this weekend!

Given the fix for the LNKMOD issue, I suspect that won't be merged
into net-next until after the weekend, but I'll see whether I can
sneak a respin of the patch set once that's happened. That said,
given that we'll be at -rc7, it's likely too late to be thinking
about getting the PCS changes queued up for this coming merge
window. In any case, I don't think even if I did post a series, we're
at the point where we have something that would be ready.

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