Message-ID: <tukyfritbypmq3cf2mkasoaqq7lbjf6owaltghosx37df4cg3b@4mpglxfda25a>
Date: Mon, 3 Jun 2024 16:39:11 -0500
From: Andrew Halaney <ahalaney@...hat.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Serge Semin <fancer.lancer@...il.com>, 
	Alexandre Torgue <alexandre.torgue@...s.st.com>, Alexei Starovoitov <ast@...nel.org>, bpf@...r.kernel.org, 
	Daniel Borkmann <daniel@...earbox.net>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Jesper Dangaard Brouer <hawk@...nel.org>, John Fastabend <john.fastabend@...il.com>, 
	Jose Abreu <joabreu@...opsys.com>, linux-arm-kernel@...ts.infradead.org, 
	linux-stm32@...md-mailman.stormreply.com, Maxime Coquelin <mcoquelin.stm32@...il.com>, 
	netdev@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH RFC v2 0/8] net: stmmac: convert stmmac "pcs" to phylink

On Fri, May 31, 2024 at 12:25:47PM GMT, Russell King (Oracle) wrote:
> Hi,
> 
> This is version 2 of the series switching stmmac to use phylink PCS
> instead of going behind phylink's back.
> 
> Changes since version 1:
> - Addition of patches from Serge Semin to allow RGMII to use the
>   "PCS" code even if priv->dma_cap.pcs is not set (including tweaks
>   by me.)
> - Restructuring of the patch set to be a more logical split.
> - Leave the pcs_ctrl_ane methods until we've worked out what to do
>   with the qcom-ethqos driver (this series may still end up breaking
>   it, but at least we will now successfully compile.)
> 
> A reminder that what I want to hear from this patch set are the results
> of testing - and thanks to Serge, the RGMII paths were exercised, but
> I have not had any results for the SGMII side of this.

I took this for a brief spin on the sa8775p-ride eval board from
Qualcomm, as a reminder here's the dts to show the setup:

    https://elixir.bootlin.com/linux/v6.9-rc3/source/arch/arm64/boot/dts/qcom/sa8775p-ride.dts#L288

I didn't notice any issues with traffic on either interface (there's two
stmmac device instances, both using SGMII to a marvell 88ea1512 phy).
Did some basic link up / down tests, iperf3, etc.

If there's anything in specific you'd like exercised, please let me
know. I'll try and find time to look over the patches more carefully
tomorrow for review purposes, but I only know what I know from reading
the driver some, so I can't answer any of the open questions with any
official documentation.

Tested-by: Andrew Halaney <ahalaney@...hat.com>

> 
> There are still a bunch of outstanding questions:
> 
> - whether we should be using two separate PCS instances, one for
>   RGMII and another for SGMII. If the PCS hardware is not present,
>   but are using RGMII mode, then we probably don't want to be
>   accessing the registers that would've been there for SGMII.
> - what the three interrupts associated with the PCS code actually
>   mean when they fire.
> - which block's status we're reading in the pcs_get_state() method,
>   and whether we should be reading that for both RGMII and SGMII.
> - whether we need to activate phylink's inband mode in more cases
>   (so that the PCS/MAC status gets read and used for the link.)
> 
> There's probably more questions to be asked... but really the critical
> thing is to shake out any breakage from making this conversion. Bear
> in mind that I have little knowledge of this hardware, so this
> conversion has been done somewhat blind using only what I can observe
> from the current driver.
> 
> Original blurb below.
> 
> As I noted recently in a thread (and was ignored) stmmac sucks. (I
> won't hide my distain for drivers that make my life as phylink
> maintainer more difficult!)
> 
> One of the contract conditions for using phylink is that the driver
> will _not_ mess with the netif carrier. stmmac developers/maintainers
> clearly didn't read that, because stmmac messes with the netif
> carrier, which destroys phylink's guarantee that it'll make certain
> calls in a particular order (e.g. it won't call mac_link_up() twice
> in a row without an intervening mac_link_down().) This is clearly
> stated in the phylink documentation.
> 
> Thus, this patch set attempts to fix this. Why does it mess with the
> netif carrier? It has its own independent PCS implementation that
> completely bypasses phylink _while_ phylink is still being used.
> This is not acceptable. Either the driver uses phylink, or it doesn't
> use phylink. There is no half-way house about this. Therefore, this
> driver needs to either be fixed, or needs to stop using phylink.
> 
> Since I was ignored when I brought this up, I've hacked together the
> following patch set - and it is hacky at the moment. It's also broken
> because of recentl changes involving dwmac-qcom-ethqos.c - but there
> isn't sufficient information in the driver for me to fix this. The
> driver appears to use SGMII at 2500Mbps, which simply does not exist.
> What interface mode (and neg_mode) does phylink pass to pcs_config()
> in each of the speeds that dwmac-qcom-ethqos.c is interested in.
> Without this information, I can't do that conversion. So for the
> purposes of this, I've just ignored dwmac-qcom-ethqos.c (which means
> it will fail to build.)
> 
> The patch splitup is not ideal, but that's not what I'm interested in
> here. What I want to hear is the results of testing - does this switch
> of the RGMII/SGMII "pcs" stuff to a phylink_pcs work for this driver?
> 
> Please don't review the patches, but you are welcome to send fixes to
> them. Once we know that the overall implementation works, then I'll
> look at how best to split the patches. In the mean time, the present
> form is more convenient for making changes and fixing things.
> 
> There is still more improvement that's needed here.
> 
> Thanks.
> 
>  drivers/net/ethernet/stmicro/stmmac/Makefile       |   2 +-
>  drivers/net/ethernet/stmicro/stmmac/common.h       |  12 +-
>  .../net/ethernet/stmicro/stmmac/dwmac1000_core.c   | 146 ++++++++++++++-------
>  drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c  | 131 +++++++++++++-----
>  drivers/net/ethernet/stmicro/stmmac/hwif.h         |  19 ++-
>  .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c   | 111 +---------------
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  |  33 ++---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c   |  58 ++++++++
>  drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h   |  34 +----
>  9 files changed, 298 insertions(+), 248 deletions(-)
>  create mode 100644 drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c
> 
> -- 
> 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