Message-ID: <5dsv27tygpfsmaofdtswwlsfjxzxlwnnysdziqgdglkydgoqtg@qwssdirr42bd>
Date: Thu, 4 Jul 2024 15:55:26 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Halaney <ahalaney@...hat.com>, 
	Alexandre Torgue <alexandre.torgue@...s.st.com>, Jose Abreu <joabreu@...opsys.com>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Maxime Coquelin <mcoquelin.stm32@...il.com>, Alexei Starovoitov <ast@...nel.org>, 
	Jesper Dangaard Brouer <hawk@...nel.org>, John Fastabend <john.fastabend@...il.com>, 
	Daniel Borkmann <daniel@...earbox.net>, linux-arm-kernel@...ts.infradead.org, 
	linux-stm32@...md-mailman.stormreply.com, bpf@...r.kernel.org, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC net-next v2 16/17] net: stmmac: Move internal PCS
 init method to stmmac_pcs.c

On Fri, Jun 28, 2024 at 03:36:20PM +0100, Russell King (Oracle) wrote:
> On Mon, Jun 24, 2024 at 04:26:33PM +0300, Serge Semin wrote:
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 72c2d3e2c121..743d356f6d12 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -950,13 +950,16 @@ static struct phylink_pcs *stmmac_mac_select_pcs(struct phylink_config *config,
> >  {
> >  	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
> >  
> > +	if (priv->hw->pcs)
> > +		return &priv->hw->mac_pcs;
> > +
> >  	if (priv->hw->xpcs)
> >  		return &priv->hw->xpcs->pcs;
> >  
> >  	if (priv->hw->phylink_pcs)
> >  		return priv->hw->phylink_pcs;
> >  
> > -	return stmmac_mac_phylink_select_pcs(priv, interface);
> > +	return NULL;
> 
> I really really don't like this due to:
> 

> 1. I spent a long time working out what the priority here should be, and
> you've just thrown all that work away by changing it - to something that
> I believe is incorrect.
> 

Right, the correct precedence would be to use the external PCS if one
available. It's easy to fix anyway.

> 2. I want to eventually see this function checking the interface type
> before just handing out a random PCS,

The only problem is that currently it relies on the
plat_stmmaenet_data::mac_interface field value instead of parsing the
specified interface type.(

> and it was my intention to
> eventually that into the MACs own select_pcs() methods. Getting rid of
> those methods means that the MACs themselves now can't make the
> decision which is where that should be.

Ok. Why not. We can preserve the MAC-own select_pcs() method.
(See my last comment on this email for details.)

> 
> 3. When operating in RGMII "inband" mode, the .pcs_config etc doesn't
> make much sense (we're probably accessing registers that don't exist)

Absolutely right. Current dwmac_pcs_config() implementation is fully
SGMII/TBI/RTBI-specific.

> and I had plans to split this into a RGMII "PCS" which was just a PCS
> that implemented .pcs_get_state(), a stub .pcs_config(), and a separate
> fully-featured "SGMII PCS".

Actually it's a good idea. We should have that implemented in v3.

> 
> So, I would like to eventually see here something like:
> 
> 	if (priv->hw->xpcs)
> 		return &priv->hw->xpcs->pcs;
> 
> 	if (priv->hw->phylink_pcs)
> 		return priv->hw->phylink_pcs;
> 
> 	if (!(priv->plat->flags & STMMAC_FLAG_HAS_INTEGRATED_PCS)) {
> 		if (phy_interface_mode_is_rgmii(priv->plat->mac_interface))
> 			return &priv->hw->mac_rgmii_pcs;
> 
> 		if (priv->dma_cap.pcs &&
> 		    priv->plat->mac_interface == PHY_INTERFACE_MODE_SGMII)
> 			return &priv->hw->mac_sgmii_pcs;
> 	}
> 
> 	return NULL;

So the differences of my and your implementations are:
1. priv->hw->pcs field is utilized to determine the RGMII/SGMII PCS
availability (it's initialized in dwmac_pcs_init()).
2. The order of the PCS selection: internal PCS has precedence over
the external PCS'es.
3. There is a single PHY-link PCS descriptor for both RGMII "inband"
and SGMII PCSes.

There is nothing hard to settle the 2. and 3. notes. The only
problematic part is 1. due to the damn mac_device_info::ps field
implying the fixed-speed semantics for the MAC2MAC case. The field is
initialized in the stmmac_hw_setup() method based on the
mac_device_info::pcs field content. The mac_device_info::ps value is
then utilized in the stmmac_ops::core_init() methods and in
dwmac_pcs_config() to pre-define the link speed. Since I hadn't come
up with a good idea of what to do with that MAC2MAC stuff back then I
decided to preserve the mac_device_info::pcs-based semantics
everywhere.

But now I guess I've got a good idea. We can use the
plat_stmmacenet_data::mac_port_sel_speed field directly where it is
relevant. Like this:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:
static int stmmac_hw_setup(struct net_device *dev, bool ptp_register)
{
	// Drop everything priv->hw.pcs and priv->hw.ps related from here
	// due to the changes suggested further.
}

drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c:
drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c:
static void dwmac*_core_init(...)
{
	...
	// Directly use the plat_stmmacenet_data::mac_port_sel_speed value
	switch (priv->plat->mac_port_sel_speed) {
	case SPEED_1000:
		ps_speed = hw->link.speed1000;
		break;
	case SPEED_100:
		ps_speed = hw->link.speed100;
		break;
	case SPEED_10:
		ps_speed = hw->link.speed10;
		break;
	default:
		dev_warn(priv->device, "Unsupported port speed\n");
		break;
	}

	if (ps_speed) {
		value &= hw->link.speed_mask;
		value |= ps_speed | GMAC_CONFIG_TE;
	}
	...
}

drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:
static void dwmac*_core_init(...)
{
	// There is no internal PCS in DW XGMACes. So we can freely drop
	// the hw->ps clause from here.
}

drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c:
static int dwmac_pcs_config(...)
{
	...
	// Directly use the plat_stmmacenet_data::mac_port_sel_speed value
	if (priv->plat->mac_port_sel_speed)
		val |= PCS_AN_CTRL_SGMRAL;
	...
}

After that we can freely drop the mac_device_info::ps and
mac_device_info::pcs fields. Thoughts?

> 
> > +void dwmac_pcs_init(struct mac_device_info *hw)
> > +{
> > +	struct stmmac_priv *priv = hw->priv;
> > +	int interface = priv->plat->mac_interface;
> > +
> > +	if (priv->plat->flags & STMMAC_FLAG_HAS_INTEGRATED_PCS)
> > +		return;
> > +	else if (phy_interface_mode_is_rgmii(interface))
> > +		hw->pcs = STMMAC_PCS_RGMII;
> > +	else if (priv->dma_cap.pcs && interface == PHY_INTERFACE_MODE_SGMII)
> > +		hw->pcs = STMMAC_PCS_SGMII;
> > +
> > +	hw->mac_pcs.neg_mode = true;
> > +}
> 

> Please move "hw->mac_pcs.neg_mode = true;" to where the PCS method
> functions are implemented - it determines whether the PCS method
> functions take the AN mode or the neg mode, and this is a property of
> their implementations. It should not be split away from them.

Ok.

---

Seeing the series introducing the plat_stmmacenet_data::select_pcs()
method has been recently merged in, let's discuss the entire PCS
selection code a bit more. Taking into account what you said above I
guess we can implement something like this:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:
static struct phylink_pcs *stmmac_mac_select_pcs(struct phylink_config *config,
                                                 phy_interface_t interface)
{
	// Platform-specific PCS selection method implying DW XPCS and
	// Lynx PCS selection (and internal PCS selection if relevant)
        if (priv->plat->select_pcs) {
                pcs = priv->plat->select_pcs(priv, interface);
                if (!IS_ERR(pcs))
                        return pcs;
        }

	// MAC-specific PCS selection method
	pcs = stmmac_mac_select_int_pcs(priv, priv->hw, priv->plat->mac_interface);
        if (!IS_ERR(pcs))
                return pcs;

        return NULL;
}

drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c:
static struct phylink_pcs *dwmac1000_select_pcs(struct mac_device_info *hw,
						phy_interface_t interface)
{
	if (phy_interface_mode_is_rgmii(interface))
		return &hw->mac_rgmii_pcs;
	else if (priv->dma_cap.pcs && interface == PHY_INTERFACE_MODE_SGMII)
		return &hw->mac_sgmii_pcs;

	return NULLL
}

...

const struct stmmac_ops dwmac1000_ops = {
	...
	.select_pcs = dwmac1000_select_pcs,
	...
};

drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c:
// The same changes as in the dwmac1000_core.c file.

drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c:
// Drop my dwmac_pcs_init() implementation if we get to eliminate the 
// mac_device_info::ps and mac_device_info::pcs fields as I suggested
// earlier in this message


So what do you think?

-Serge(y)

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