[RFC,v3,0/14] net: stmmac: convert stmmac "pcs" to phylink

Hi,

This is version 3 of the series switching stmmac to use phylink PCS
isntead of going behind phylink's back.

Changes since version 2:
- Adopted some of Serge's feedback.
- New patch: adding ethqos_pcs_set_inband() for qcom-ethqos so we
  have one place to modify for AN control rather than many.
- New patch: pass the stmmac_priv structure into the pcs_set_ane()
  method.
- New patch: remove pcs_get_adv_lp() early, as this is only for TBI
  and RTBI, support for which we dropped in an already merged patch.
- Provide stmmac_pcs structure to encapsulate the pointer to
  stmmac_priv, PCS MMIO address pointer and phylink_pcs structure.
- Restructure dwmac_pcs_config() so we can eventually share code
  with dwmac_ctrl_ane().
- New patch: move dwmac_ctrl_ane() into stmmac_pcs.c, and share code.
- New patch: pass the stmmac_pcs structure into dwmac_pcs_isr().
- New patch: similar to Serge's patch, rename the PCS registers, but
  use STMMAC_PCS_ as the prefix rather than just PCS_ which is too
  generic.
- New patch: incorporate "net: stmmac: Activate Inband/PCS flag
  based on the selected iface" from Serge.

On the subject of whether we should have two PCS instances, I
experimented with that and have now decided against it. Instead,
dwmac_pcs_config() now tests whether we need to fiddle with the
PCS control register or not.

Note that I prefer not to have multiple layers of indirection, but
instead prefer a library-style approach, which is why I haven't
turned the PCS support into something that's self contained with
a method in the MAC driver to grab the RGSMII status.


Previous cover messages from earlier posts below:

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.

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       |  25 ++--
 .../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    |  13 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac1000.h    |  13 +-
 .../net/ethernet/stmicro/stmmac/dwmac1000_core.c   | 110 +++++++-------
 drivers/net/ethernet/stmicro/stmmac/dwmac4.h       |  13 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c  |  99 +++++++------
 drivers/net/ethernet/stmicro/stmmac/hwif.h         |  24 ++--
 .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c   | 111 +-------------
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  |  30 +---
 drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c   |  63 ++++++++
 drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h   | 159 ++++++++++-----------
 12 files changed, 306 insertions(+), 356 deletions(-)
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c

On Fri, Aug 02, 2024 at 11:46:30AM GMT, Russell King (Oracle) wrote:
> Add ethqos_pcs_set_inband() to improve readability, and to allow future
> changes when phylink PCS support is properly merged.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>

On Fri, Aug 02, 2024 at 11:46:41AM GMT, Russell King (Oracle) wrote:
> Discussing with Serge Semin, it appears that the GMAC_ANE_ADV and
> GMAC_ANE_LPA registers are only available for TBI and RTBI PHY
> interfaces. In commit 482b3c3ba757 ("net: stmmac: Drop TBI/RTBI PCS
> flags") support for these was dropped, and thus it no longer makes
> sense to access these registers.
> 
> Remove the *_get_adv_lp() functions from the stmmac driver.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Clean up seems good, I'll take Serge's word on the IP details here.

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>

On Fri, Aug 02, 2024 at 11:47:32AM GMT, Russell King (Oracle) wrote:
> The pcs_ctrl_ane() method is no longer required as this will be handled
> by the mac_pcs phylink_pcs instance. Remove these methods, their common
> implementation, the pcs_link, pcs_duplex and pcs_speed members of
> struct stmmac_extra_stats, and stmmac_has_mac_phylink_select_pcs().
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

...

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> index 3c8ae3753205..799af80024d2 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> @@ -321,48 +321,6 @@ static int stmmac_ethtool_get_link_ksettings(struct net_device *dev,
>  {
>  	struct stmmac_priv *priv = netdev_priv(dev);
>  
> -	if (!(priv->plat->flags & STMMAC_FLAG_HAS_INTEGRATED_PCS) &&

This change effectively makes the INTEGRATED_PCS flag useless, I think
we should remove it entirely.

Thanks,
Andrew

On Fri, Aug 02, 2024 at 02:02:25PM -0500, Andrew Halaney wrote:
> On Fri, Aug 02, 2024 at 11:47:32AM GMT, Russell King (Oracle) wrote:
> > The pcs_ctrl_ane() method is no longer required as this will be handled
> > by the mac_pcs phylink_pcs instance. Remove these methods, their common
> > implementation, the pcs_link, pcs_duplex and pcs_speed members of
> > struct stmmac_extra_stats, and stmmac_has_mac_phylink_select_pcs().
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> 
> ...
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> > index 3c8ae3753205..799af80024d2 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> > @@ -321,48 +321,6 @@ static int stmmac_ethtool_get_link_ksettings(struct net_device *dev,
> >  {
> >  	struct stmmac_priv *priv = netdev_priv(dev);
> >  
> > -	if (!(priv->plat->flags & STMMAC_FLAG_HAS_INTEGRATED_PCS) &&
> 
> This change effectively makes the INTEGRATED_PCS flag useless, I think
> we should remove it entirely.

I'm hoping the ethqos folk are going to test this patch series and tell
me whether it works for them - specifically Sneh Shah who added

	net: stmmac: dwmac-qcom-ethqos: Add support for 2.5G SGMII

which directly configures the PCS bypassing phylink. Specifically,
if this in stmmac_check_pcs_mode():

	priv->dma_cap.pcs && interface == PHY_INTERFACE_MODE_SGMII

is true for this device, then we may be in for problems. Since
priv->dma_cap.pcs comes from hardware, it's impossible to tell
unless one has that hardware.

On Fri, Aug 02, 2024 at 11:45:21AM GMT, Russell King (Oracle) wrote:
> Hi,
> 
> This is version 3 of the series switching stmmac to use phylink PCS
> isntead of going behind phylink's back.
> 
> Changes since version 2:
> - Adopted some of Serge's feedback.
> - New patch: adding ethqos_pcs_set_inband() for qcom-ethqos so we
>   have one place to modify for AN control rather than many.
> - New patch: pass the stmmac_priv structure into the pcs_set_ane()
>   method.
> - New patch: remove pcs_get_adv_lp() early, as this is only for TBI
>   and RTBI, support for which we dropped in an already merged patch.
> - Provide stmmac_pcs structure to encapsulate the pointer to
>   stmmac_priv, PCS MMIO address pointer and phylink_pcs structure.
> - Restructure dwmac_pcs_config() so we can eventually share code
>   with dwmac_ctrl_ane().
> - New patch: move dwmac_ctrl_ane() into stmmac_pcs.c, and share code.
> - New patch: pass the stmmac_pcs structure into dwmac_pcs_isr().
> - New patch: similar to Serge's patch, rename the PCS registers, but
>   use STMMAC_PCS_ as the prefix rather than just PCS_ which is too
>   generic.
> - New patch: incorporate "net: stmmac: Activate Inband/PCS flag
>   based on the selected iface" from Serge.
> 
> On the subject of whether we should have two PCS instances, I
> experimented with that and have now decided against it. Instead,
> dwmac_pcs_config() now tests whether we need to fiddle with the
> PCS control register or not.
> 
> Note that I prefer not to have multiple layers of indirection, but
> instead prefer a library-style approach, which is why I haven't
> turned the PCS support into something that's self contained with
> a method in the MAC driver to grab the RGSMII status.
> 

Tested-by: Andrew Halaney <ahalaney@redhat.com> # sa8775p-ride

Note, I also tested with setting sa8775p-ride to:

    managed = "in-band-status";

and noticed no issues either when signalling was done in-band. Just
highlighting that since there's some comments referencing the lack of
in-band signalling with dwmac-qcom-ethqos usage in the series, but it
seems that's ok in either case.

I know there's the "sa8775p-ride-r3.dts" that was recently added,
running with "OCSGMII" (hacked up 2.5GHz SGMII IIUC), I can't test that
since I don't have that hardware. I think some of the remaining
interesting bits in the dwmac-qcom-ethqos driver are to handle that
(like the usage of ethqos_pcs_set_inband).

Thanks,
Andrew

On Fri, 2 Aug 2024 11:45:21 +0100 Russell King (Oracle) wrote:
> Subject: [PATCH RFC v3 0/14] net: stmmac: convert stmmac "pcs" to phylink

we have a build error here inside the tasty layered cake that is the op
handling in this driver (from patch 2 to 13, inclusive):

In file included from drivers/net/ethernet/stmicro/stmmac/common.h:26,
                 from drivers/net/ethernet/stmicro/stmmac/stmmac.h:20,
                 from drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c:19:
drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c: In function ‘stmmac_ethtool_set_link_ksettings’:
drivers/net/ethernet/stmicro/stmmac/hwif.h:15:17: error: too many arguments to function ‘priv->hw->mac->pcs_ctrl_ane’
   15 |                 (__priv)->hw->__module->__cname((__arg0), ##__args); \
      |                 ^
drivers/net/ethernet/stmicro/stmmac/hwif.h:485:9: note: in expansion of macro ‘stmmac_do_void_callback’
  485 |         stmmac_do_void_callback(__priv, mac, pcs_ctrl_ane, __priv, __args)
      |         ^~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c:419:17: note: in expansion of macro ‘stmmac_pcs_ctrl_ane’
  419 |                 stmmac_pcs_ctrl_ane(priv, priv->ioaddr, 1, priv->hw->ps, 0);
      |                 ^~~~~~~~~~~~~~~~~~~

On Fri, Aug 2, 2024 at 12:45 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> Hi,
>
> This is version 3 of the series switching stmmac to use phylink PCS
> isntead of going behind phylink's back.
>

Tested-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> #
sa8775p-ride-r3

(The board is a more recent revision of the one Andrew tested this series on)

On Fri, 2 Aug 2024 12:45:21 +0200, "Russell King (Oracle)"
<linux@armlinux.org.uk> said:
> Hi,
>
> This is version 3 of the series switching stmmac to use phylink PCS
> isntead of going behind phylink's back.
>

Sorry for the noise but I had the line wrapping on. Here's the tag once again:

Tested-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> #
sa8775p-ride-r3

(The board is a more recent revision of the one Andrew tested this series on)

On Fri, Aug 02, 2024 at 08:22:42PM GMT, Russell King (Oracle) wrote:
> On Fri, Aug 02, 2024 at 02:02:25PM -0500, Andrew Halaney wrote:
> > On Fri, Aug 02, 2024 at 11:47:32AM GMT, Russell King (Oracle) wrote:
> > > The pcs_ctrl_ane() method is no longer required as this will be handled
> > > by the mac_pcs phylink_pcs instance. Remove these methods, their common
> > > implementation, the pcs_link, pcs_duplex and pcs_speed members of
> > > struct stmmac_extra_stats, and stmmac_has_mac_phylink_select_pcs().
> > >
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> >
> > ...
> >
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> > > index 3c8ae3753205..799af80024d2 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> > > @@ -321,48 +321,6 @@ static int stmmac_ethtool_get_link_ksettings(struct net_device *dev,
> > >  {
> > >  	struct stmmac_priv *priv = netdev_priv(dev);
> > >
> > > -	if (!(priv->plat->flags & STMMAC_FLAG_HAS_INTEGRATED_PCS) &&
> >
> > This change effectively makes the INTEGRATED_PCS flag useless, I think
> > we should remove it entirely.
>
> I'm hoping the ethqos folk are going to test this patch series and tell
> me whether it works for them - specifically Sneh Shah who added
>
> 	net: stmmac: dwmac-qcom-ethqos: Add support for 2.5G SGMII
>
> which directly configures the PCS bypassing phylink. Specifically,
> if this in stmmac_check_pcs_mode():
>
> 	priv->dma_cap.pcs && interface == PHY_INTERFACE_MODE_SGMII
>
> is true for this device, then we may be in for problems. Since
> priv->dma_cap.pcs comes from hardware, it's impossible to tell
> unless one has that hardware.

Hopefully we get a response there. For what its worth I have a
access to the sa8775p-ride.dts board in a remote lab and
dma_cap.pcs is definitely set for this integration of the IP
on sa8775p. The only upstream described boards are:

    1) sa8775p-ride
    2) sa8775p-ride-r3

The difference is that "r3" is the latest spin of the board, with some
Aquantia phys attached to the 2 stmmac MACs on the board instead of the
Marvell 88EA1512 phys on the former. My understanding is that's to
evaluate 2500 Mbps speeds (the 88EA1512 only goes up to 1000 Mbps).

The "r3" board's Aquantia aqr115c's are capable of 2500 Mbps, but are
"overclock SGMII". The "r3" describes the phy interface as 2500base-x,
with no in-band signalling (since the "OCSGMII" is hacked up and doesn't
really do the in-band signalling you've described in the past). That's
all based on Bart's commit message adding support for that in:

    0ebc581f8a4b7 net: phy: aquantia: add support for aqr115c

I think Sneh also had access to a board with the sa8775p in a fixed-link
configuration doing 2500 Mbps, but that's not described upstream at the
moment. I believe that was the board that originally motivated the patch
you highlighted from him.

At the very least Bartosz and I tested this and things didn't break
noticeably for the 2 boards I listed above... so that's good :)

Hope that helps,
Andrew