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

[Serge Semin]

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.

> 
> > +static void dwmac_pcs_get_state(struct phylink_pcs *pcs,
> > +				struct phylink_link_state *state)
> >  {
> > +	struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs);
> >  	struct stmmac_priv *priv = hw->priv;
> >  	u32 val;
> >  
> > +	val = stmmac_pcs_get_config_reg(priv, hw);
> > +
> > +	/* TODO The next is SGMII/RGMII/SMII-specific */
> > +	state->link = !!(val & PCS_CFG_LNKSTS);
> > +	if (!state->link)
> > +		return;
> > +
> > +	switch (FIELD_GET(PCS_CFG_LNKSPEED, val)) {
> > +	case PCS_CFG_LNKSPEED_2_5:
> > +		state->speed = SPEED_10;
> > +		break;
> > +	case PCS_CFG_LNKSPEED_25:
> > +		state->speed = SPEED_100;
> > +		break;
> > +	case PCS_CFG_LNKSPEED_250:
> > +		state->speed = SPEED_1000;
> > +		break;
> > +	default:
> > +		netdev_err(priv->dev, "Unknown speed detected\n");
> > +		break;
> > +	}
> > +
> > +	state->duplex = val & PCS_CFG_LNKMOD ? DUPLEX_FULL : DUPLEX_HALF;
> > +
> > +	/* 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.

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

> 
> >  
> > -	/* 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)?

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

-Serge(y)

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