Message-ID: <ZlolU6+lUaXQSQID@shell.armlinux.org.uk>
Date: Fri, 31 May 2024 20:30:27 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Serge Semin <fancer.lancer@...il.com>
Cc: 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>,
	Byungho An <bh74.an@...sung.com>,
	Giuseppe CAVALLARO <peppe.cavallaro@...com>,
	Alexei Starovoitov <ast@...nel.org>,
	Daniel Borkmann <daniel@...earbox.net>,
	Jesper Dangaard Brouer <hawk@...nel.org>,
	John Fastabend <john.fastabend@...il.com>, bpf@...r.kernel.org,
	netdev@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC net-next 2/3] net: stmmac: Activate Inband/PCS flag
 based on the selected iface

Hi Serge,

Thanks for the reply. I've attempted to deal with most of these in my
v2 posting, but maybe not in the best way yet.

On Fri, May 31, 2024 at 08:13:49PM +0300, Serge Semin wrote:
> > Does this
> > mean it is true that these cores will never be used with an external
> > PCS?
> 
> Sorry, I was wrong to suggest the (priv->plat.has_gmac ||
> priv->plat.has_gmac4)-based statement. Indeed there is a case of having DW
> QoS Eth and DW XPCS synthesized together with the SGMII/1000Base-X
> downstream interface. Not sure why it was needed to implement that way
> seeing DW QoS Eth IP-core supports optional SGMII PHY interface out of
> box, but AFAICS Intel mGBE is that case. Anyway the correct way to
> detect the internal PCS support is to check the PCSSEL flag set in the
> HWFEATURE register (preserved in the stmmac_priv::dma_cap::pcs field).

We can only wonder why!

> > Please can you confirm that if an external PCS (e.g. xpcs, lynx PCS)
> > is being used, the internal PCS will not have been synthesized, and
> > thus priv->dma_cap.pcs will be false?
> 
> Alas I can't confirm that. priv->dma_cap.pcs only indicates the
> internal PCS availability. External PCS is an independent entity from
> the DW *MAC IP-core point of view. So the DW GMAC/QoS Eth/XGMAC
> controllers aren't aware of its existence. It's the low-level platform
> driver/code responsibility to somehow detect it being available
> ("pcs-handle" property, plat->mdio_bus_data->has_xpcs flag, etc).
> 
> Regarding the internal PCS, as long as the DW GMAC or DW QoS Eth is
> synthesized with the SGMII/TBI/RTBI PHY interface support
> priv->dma_cap.pcs will get to be true. Note the device can be
> synthesized with several PHY interfaces supported. As long as
> SGMII/TBI/RTBI PHY interface is any of them, the flag will be set
> irrespective from the PHY interface activated at runtime. 

I've been debating about this, and given your response, I'm wondering
whether we should change stmmac_mac_select_pcs() to instead do:

static struct phylink_pcs *stmmac_mac_select_pcs(struct phylink_config *config,
						 phy_interface_t interface)
{
	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
	struct phylink_pcs *pcs;

	if (priv->plat->select_pcs) {
		pcs = priv->plat->select_pcs(priv, interface);
		if (!IS_ERR(pcs))
			return pcs;
	}

	return stmmac_mac_phylink_select_pcs(priv, interface);
}

and push the problem of whether to provide a PCS that overrides
the MAC internal PCS into platform code. That would mean Intel mGBE
would be able to override with XPCS. rzn1 and socfpga can then do
their own thing as well.

I'm trying hard not to go down another rabbit hole... I've just
spotted that socfpga sets mac_interface to PHY_INTERFACE_MODE_SGMII.
That's another reason for pushing this down into platform drivers -
if platform drivers are doing weird stuff, then we can contain their
weirdness in the platform drivers moving it out of the core code.

> You can extend the priv->dma_cap.pcs flag semantics. So it could
> be indicating three types of the PCS'es:
> RGMII, SGMII, XPCS (or TBI/RTBI in future).

If TBI/RTBI gets supported, then this would have to be extended, but I
get the impression that this isn't popular.

> I guess the DW XPCS implementation might be more preferable. From one
> side DW XPCS SGMII can support up to 2.5Gbps speed, while the DW
> GMAC/QoS Eth SGMII can work with up to 1Gbps speed only. On the other
> hand the DW XPCS might be available over the MDIO-bus, which is slower
> to access than the internal PCS CSRs available in the DW GMAC/QoS Eth
> CSRs space. So the more performant link speed seems more useful
> feature over the faster device setup process.

I think which should be used would depend on how the hardware is wired
up. This brings us back to platform specifics again, which points
towards moving the decision making into platform code as per the above.

> One thing I am not sure about is that there is a real case of having
> the DW GMAC/QoS Eth synthesized with the native SGMII/TBI/RTBI PHY
> interface support and being attached to the DW XPCS controller, which
> would have the SGMII downstream PHY interface. DW XPCS has only the
> XGMII or GMII/MII upstream interfaces over which the MAC can be
> attached.

That gives us another possibility, but needs platforms to be doing
the right thing. If mac_interface were set to XGMII or GMII/MII, then
that would exclude the internal MAC PCS.

> So DW GMAC/QoS Eth and DW XPCS can be connected via the
> GMII/MII interface only. Regarding Intel mGBE, it likely is having a
> setup like this:
> 
> +------------+          +---------+
> |            | GMII/MII |         |   SGMII
> | DW QoS Eth +----------+ DW XPCS +------------
> |            |          |         | 1000Base-X
> +------------+          +---------+


So as an alternative, 

     mac_interface            phy_interface

     XGMII/GMII/MII           SGMII/1000Base-X
MAC ---------------- DW XPCS ------------------

     INTERNAL                SGMII/TBI/RTBI
MAC ---------- Internal PCS ----------------

     INTERNAL                  RGMII
MAC ---------- Internal "PCS" --------------

One of the problems here, though, is socfpga. It uses mac_interface
with RGMII*, MII, GMII, SGMII and RMII. I think it's confusing
mac_interface for phy_interface, but I haven't read through enough
of it to be certain.

So that again leads me back to my proposal above for
stmmac_mac_select_pcs() as the least likely to break proposition -
at least given how things are at the moment.

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