[PATCH RFC 00/10] net: pcs: xpcs: cleanups batch 1

[PATCH RFC 00/10] net: pcs: xpcs: cleanups batch 1

[Russell King (Oracle)]

Hi,

First, sorry for the bland series subject - this is the first in a
number of cleanup series to the XPCS driver. This series has some
functional changes beyond merely cleanups, notably the first patch.

This series starts off with a patch that moves the PCS reset from
the xpcs_create*() family of calls to when phylink first configures
the PHY. The motivation for this change is to get rid of the
interface argument to the xpcs_create*() functions, which I see as
unnecessary complexity. This patch needs testing on SJA1105, Wangxun
and STMMAC drivers.

Patch 2 removes the now unnecessary interface argument from the
internal xpcs_create() and xpcs_init_iface() functions. With this,
xpcs_init_iface() becomes a misnamed function, but patch 3 removes
this function, moving its now meager contents to xpcs_create().

Patch 4 adds xpcs_destroy_pcs() and xpcs_create_pcs_mdiodev()
functions which return and take a phylink_pcs, allowing SJA1105
and Wangxun drivers to be converted to using the phylink_pcs
structure internally.

Patches 5 through 8 convert both these drivers to that end.

Patch 9 drops the interface argument from the remaining xpcs_create*()
functions, addressing the only remaining caller of these functions,
that being the STMMAC driver.

As patch 7 removed the direct calls to the XPCS config/link-up
functions, the last patch makes these functions static.

 drivers/net/dsa/sja1105/sja1105.h                 |  2 +-
 drivers/net/dsa/sja1105/sja1105_main.c            | 83 ++++++++++----------
 drivers/net/dsa/sja1105/sja1105_mdio.c            | 28 ++++---
 drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c |  7 +-
 drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c    | 18 ++---
 drivers/net/ethernet/wangxun/txgbe/txgbe_type.h   |  2 +-
 drivers/net/pcs/pcs-xpcs.c                        | 92 ++++++++++++++---------
 include/linux/pcs/pcs-xpcs.h                      | 14 ++--
 8 files changed, 130 insertions(+), 116 deletions(-)

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

[PATCH RFC net-next 01/10] net: pcs: xpcs: move PCS reset to .pcs_pre_config()

[Russell King (Oracle)]

Move the PCS reset to .pcs_pre_config() rather than at creation time,
which means we call the reset function with the interface that we're
actually going to be using to talk to the downstream device.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/pcs/pcs-xpcs.c   | 39 +++++++++++++++++++++++++++---------
 include/linux/pcs/pcs-xpcs.h |  1 +
 2 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 82463f9d50c8..7c6c40ddf722 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -659,6 +659,30 @@ int xpcs_config_eee(struct dw_xpcs *xpcs, int mult_fact_100ns, int enable)
 }
 EXPORT_SYMBOL_GPL(xpcs_config_eee);
 
+static void xpcs_pre_config(struct phylink_pcs *pcs, phy_interface_t interface)
+{
+	struct dw_xpcs *xpcs = phylink_pcs_to_xpcs(pcs);
+	const struct dw_xpcs_compat *compat;
+	int ret;
+
+	if (!xpcs->need_reset)
+		return;
+
+	compat = xpcs_find_compat(xpcs->desc, interface);
+	if (!compat) {
+		dev_err(&xpcs->mdiodev->dev, "unsupported interface %s\n",
+			phy_modes(interface));
+		return;
+	}
+
+	ret = xpcs_soft_reset(xpcs, compat);
+	if (ret)
+		dev_err(&xpcs->mdiodev->dev, "soft reset failed: %pe\n",
+			ERR_PTR(ret));
+
+	xpcs->need_reset = false;
+}
+
 static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
 				      unsigned int neg_mode)
 {
@@ -1365,6 +1389,7 @@ static const struct dw_xpcs_desc xpcs_desc_list[] = {
 
 static const struct phylink_pcs_ops xpcs_phylink_ops = {
 	.pcs_validate = xpcs_validate,
+	.pcs_pre_config = xpcs_pre_config,
 	.pcs_config = xpcs_config,
 	.pcs_get_state = xpcs_get_state,
 	.pcs_an_restart = xpcs_an_restart,
@@ -1460,18 +1485,12 @@ static int xpcs_init_id(struct dw_xpcs *xpcs)
 
 static int xpcs_init_iface(struct dw_xpcs *xpcs, phy_interface_t interface)
 {
-	const struct dw_xpcs_compat *compat;
-
-	compat = xpcs_find_compat(xpcs->desc, interface);
-	if (!compat)
-		return -EINVAL;
-
-	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
+	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID)
 		xpcs->pcs.poll = false;
-		return 0;
-	}
+	else
+		xpcs->need_reset = true;
 
-	return xpcs_soft_reset(xpcs, compat);
+	return 0;
 }
 
 static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index b4a4eb6c8866..fd75d0605bb6 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -61,6 +61,7 @@ struct dw_xpcs {
 	struct clk_bulk_data clks[DW_XPCS_NUM_CLKS];
 	struct phylink_pcs pcs;
 	phy_interface_t interface;
+	bool need_reset;
 };
 
 int xpcs_get_an_mode(struct dw_xpcs *xpcs, phy_interface_t interface);
-- 
2.30.2

Re: [PATCH RFC net-next 01/10] net: pcs: xpcs: move PCS reset to .pcs_pre_config()

[Vladimir Oltean]

On Mon, Sep 23, 2024 at 03:00:59PM +0100, Russell King (Oracle) wrote:
> Move the PCS reset to .pcs_pre_config() rather than at creation time,
> which means we call the reset function with the interface that we're
> actually going to be using to talk to the downstream device.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com> # sja1105

Re: [PATCH RFC net-next 01/10] net: pcs: xpcs: move PCS reset to .pcs_pre_config()

[Serge Semin]

[-- Attachment #1: Type: text/plain, Size: 3911 bytes --]

Hi Russell

On Mon, Sep 23, 2024 at 03:00:59PM GMT, Russell King (Oracle) wrote:
> Move the PCS reset to .pcs_pre_config() rather than at creation time,
> which means we call the reset function with the interface that we're
> actually going to be using to talk to the downstream device.

The PCS-reset procedure actually can be converted to being independent
from the PHY-interface. Thus you won't need to move the PCS resetting
to the pre_config() method, and get rid from the pointer to
dw_xpcs_compat utilization each time the reset is required.
Please see the attached patch for details.*

* I was going to submit it as a part of a one more XPCS-related series,
but seeing my work interfere with yours I'll hold on with sending my
patch set for until yours is merged in.

> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/pcs/pcs-xpcs.c   | 39 +++++++++++++++++++++++++++---------
>  include/linux/pcs/pcs-xpcs.h |  1 +
>  2 files changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index 82463f9d50c8..7c6c40ddf722 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -659,6 +659,30 @@ int xpcs_config_eee(struct dw_xpcs *xpcs, int mult_fact_100ns, int enable)
>  }
>  EXPORT_SYMBOL_GPL(xpcs_config_eee);
>  
> +static void xpcs_pre_config(struct phylink_pcs *pcs, phy_interface_t interface)
> +{
> +	struct dw_xpcs *xpcs = phylink_pcs_to_xpcs(pcs);
> +	const struct dw_xpcs_compat *compat;
> +	int ret;
> +
> +	if (!xpcs->need_reset)
> +		return;
> +

> +	compat = xpcs_find_compat(xpcs->desc, interface);
> +	if (!compat) {
> +		dev_err(&xpcs->mdiodev->dev, "unsupported interface %s\n",
> +			phy_modes(interface));
> +		return;
> +	}

Please note, it's better to preserve the xpcs_find_compat() call even
if the need_reset flag is false, since it makes sure that the
PHY-interface is actually supported by the PCS.

> +
> +	ret = xpcs_soft_reset(xpcs, compat);
> +	if (ret)
> +		dev_err(&xpcs->mdiodev->dev, "soft reset failed: %pe\n",
> +			ERR_PTR(ret));
> +
> +	xpcs->need_reset = false;
> +}
> +
>  static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
>  				      unsigned int neg_mode)
>  {
> @@ -1365,6 +1389,7 @@ static const struct dw_xpcs_desc xpcs_desc_list[] = {
>  
>  static const struct phylink_pcs_ops xpcs_phylink_ops = {
>  	.pcs_validate = xpcs_validate,
> +	.pcs_pre_config = xpcs_pre_config,
>  	.pcs_config = xpcs_config,
>  	.pcs_get_state = xpcs_get_state,
>  	.pcs_an_restart = xpcs_an_restart,
> @@ -1460,18 +1485,12 @@ static int xpcs_init_id(struct dw_xpcs *xpcs)
>  
>  static int xpcs_init_iface(struct dw_xpcs *xpcs, phy_interface_t interface)
>  {
> -	const struct dw_xpcs_compat *compat;
> -
> -	compat = xpcs_find_compat(xpcs->desc, interface);
> -	if (!compat)
> -		return -EINVAL;
> -
> -	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
> +	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID)
>  		xpcs->pcs.poll = false;
> -		return 0;
> -	}
> +	else
> +		xpcs->need_reset = true;
>  
> -	return xpcs_soft_reset(xpcs, compat);
> +	return 0;
>  }
>  
>  static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
> diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
> index b4a4eb6c8866..fd75d0605bb6 100644
> --- a/include/linux/pcs/pcs-xpcs.h
> +++ b/include/linux/pcs/pcs-xpcs.h
> @@ -61,6 +61,7 @@ struct dw_xpcs {
>  	struct clk_bulk_data clks[DW_XPCS_NUM_CLKS];
>  	struct phylink_pcs pcs;
>  	phy_interface_t interface;

> +	bool need_reset;

If you still prefer the PCS-reset being done in the pre_config()
function, then what about just directly checking the PMA id in there?

	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID)
		return 0;

	return xpcs_soft_reset(xpcs);

-Serge(y)

>  };
>  
>  int xpcs_get_an_mode(struct dw_xpcs *xpcs, phy_interface_t interface);
> -- 
> 2.30.2
> 
> 

[-- Attachment #2: 0001-net-pcs-xpcs-Drop-compat-arg-from-soft-reset-method.patch --]
[-- Type: text/x-patch, Size: 4915 bytes --]

From 7e36cef5d954cc17586194b8e0b3c58fe0dfe592 Mon Sep 17 00:00:00 2001
From: Serge Semin <fancer.lancer@gmail.com>
Date: Tue, 4 Jul 2023 12:39:29 +0300
Subject: [PATCH] net: pcs: xpcs: Drop compat arg from soft-reset method
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

It's very much inconvenient to have the soft-reset method requiring the
xpcs_compat structure instance passed. The later one is found based on the
PHY-interface type which isn't always available. Such design makes an
ordinary reset-method context depended and unnecessary limits its usage
area. Indeed based on [1,2] all Soft-RST flags exported by the PMA/PMD,
PCS, AN or MII MMDs are _shared_. It means it resets all the DWX_xpcs
internal blocks including CSRs, but except the Management Interface (MDIO,
MCI, APB). Thus it doesn't really matter which MMDs soft-reset flag is
set, the result will be the same. So the AN-mode-depended code can be
freely dropped from the soft-reset method. But depending on the DW XPCS
device capabilities (basically it depends on the IP-core synthesize
parameters) it can lack some of the MMDs. In order to solve that
difficulty the Vendor-Specific 1 MMD can be utilized. It is also called as
Control MMD and exports some generic device info about the device
including a list of the available MMDs: PMA/PMD, XS/PCS, AN or MII. This
MMD persists on all the DW XPCS device [3]. Thus it can be freely utilize
to cross-platformly determine actual MMD to perform the soft-reset.

[1] DesignWare® Cores Ethernet PCS, Version 3.11b, June 2015, p.111.
[2] DesignWare® Cores Ethernet PCS, Version 3.11b, June 2015, p.268.
[3] DesignWare® Cores Ethernet PCS, Version 3.11b, June 2015, p.269.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/net/pcs/pcs-xpcs.c   | 31 ++++++++++++++++---------------
 drivers/net/pcs/pcs-xpcs.h   |  7 +++++++
 include/linux/pcs/pcs-xpcs.h |  1 +
 3 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 014ca2b067f4..81c166726636 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -271,24 +271,18 @@ static int xpcs_poll_reset(struct dw_xpcs *xpcs, int dev)
 	return (ret & MDIO_CTRL1_RESET) ? -ETIMEDOUT : 0;
 }
 
-static int xpcs_soft_reset(struct dw_xpcs *xpcs,
-			   const struct dw_xpcs_compat *compat)
+static int xpcs_soft_reset(struct dw_xpcs *xpcs)
 {
 	int ret, dev;
 
-	switch (compat->an_mode) {
-	case DW_AN_C73:
-	case DW_10GBASER:
-		dev = MDIO_MMD_PCS;
-		break;
-	case DW_AN_C37_SGMII:
-	case DW_2500BASEX:
-	case DW_AN_C37_1000BASEX:
+	if (xpcs->mmd_ctrl & DW_SR_CTRL_MII_MMD_EN)
 		dev = MDIO_MMD_VEND2;
-		break;
-	default:
+	else if (xpcs->mmd_ctrl & DW_SR_CTRL_PCS_XS_MMD_EN)
+		dev = MDIO_MMD_PCS;
+	else if (xpcs->mmd_ctrl & DW_SR_CTRL_PMA_MMD_EN)
+		dev = MDIO_MMD_PMAPMD;
+	else
 		return -EINVAL;
-	}
 
 	ret = xpcs_write(xpcs, dev, MDIO_CTRL1, MDIO_CTRL1_RESET);
 	if (ret < 0)
@@ -935,7 +929,7 @@ static int xpcs_get_state_c73(struct dw_xpcs *xpcs,
 	/* ... and then we check the faults. */
 	ret = xpcs_read_fault_c73(xpcs, state, pcs_stat1);
 	if (ret) {
-		ret = xpcs_soft_reset(xpcs, compat);
+		ret = xpcs_soft_reset(xpcs);
 		if (ret)
 			return ret;
 
@@ -1485,17 +1479,24 @@ static int xpcs_init_id(struct dw_xpcs *xpcs)
 static int xpcs_init_iface(struct dw_xpcs *xpcs, phy_interface_t interface)
 {
 	const struct dw_xpcs_compat *compat;
+	int ret;
 
 	compat = xpcs_find_compat(xpcs->desc, interface);
 	if (!compat)
 		return -EINVAL;
 
+	ret = xpcs_read(xpcs, MDIO_MMD_VEND1, DW_SR_CTRL_MMD_CTRL);
+	if (ret < 0)
+		return ret;
+
+	xpcs->mmd_ctrl = ret;
+
 	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
 		xpcs->pcs.poll = false;
 		return 0;
 	}
 
-	return xpcs_soft_reset(xpcs, compat);
+	return xpcs_soft_reset(xpcs);
 }
 
 static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h
index fa05adfae220..774b71801cc0 100644
--- a/drivers/net/pcs/pcs-xpcs.h
+++ b/drivers/net/pcs/pcs-xpcs.h
@@ -52,6 +52,13 @@
 #define DW_C73_2500KX			BIT(0)
 #define DW_C73_5000KR			BIT(1)
 
+/* VR_CTRL_MMD */
+#define DW_SR_CTRL_MMD_CTRL		0x0009
+#define DW_SR_CTRL_AN_MMD_EN		BIT(0)
+#define DW_SR_CTRL_PCS_XS_MMD_EN	BIT(1)
+#define DW_SR_CTRL_MII_MMD_EN		BIT(2)
+#define DW_SR_CTRL_PMA_MMD_EN		BIT(3)
+
 /* Clause 37 Defines */
 /* VR MII MMD registers offsets */
 #define DW_VR_MII_MMD_CTRL		0x0000
diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index b4a4eb6c8866..241a1a959406 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -59,6 +59,7 @@ struct dw_xpcs {
 	const struct dw_xpcs_desc *desc;
 	struct mdio_device *mdiodev;
 	struct clk_bulk_data clks[DW_XPCS_NUM_CLKS];
+	u16 mmd_ctrl;
 	struct phylink_pcs pcs;
 	phy_interface_t interface;
 };
-- 
2.46.1

Re: [PATCH RFC net-next 01/10] net: pcs: xpcs: move PCS reset to .pcs_pre_config()

[Russell King (Oracle)]

On Mon, Sep 30, 2024 at 01:16:57AM +0300, Serge Semin wrote:
> Hi Russell
> 
> On Mon, Sep 23, 2024 at 03:00:59PM GMT, Russell King (Oracle) wrote:
> > +static void xpcs_pre_config(struct phylink_pcs *pcs, phy_interface_t interface)
> > +{
> > +	struct dw_xpcs *xpcs = phylink_pcs_to_xpcs(pcs);
> > +	const struct dw_xpcs_compat *compat;
> > +	int ret;
> > +
> > +	if (!xpcs->need_reset)
> > +		return;
> > +
> 
> > +	compat = xpcs_find_compat(xpcs->desc, interface);
> > +	if (!compat) {
> > +		dev_err(&xpcs->mdiodev->dev, "unsupported interface %s\n",
> > +			phy_modes(interface));
> > +		return;
> > +	}
> 
> Please note, it's better to preserve the xpcs_find_compat() call even
> if the need_reset flag is false, since it makes sure that the
> PHY-interface is actually supported by the PCS.

Sorry, but I strongly disagree. xpcs_validate() will already have dealt
with that, so we can be sure at this point that the interface is always
valid. The NULL check is really only there because it'll stop the
"you've forgotten to check for NULL on this function that can return
NULL" brigade endlessly submitting patches to add something there -
just like xpcs_get_state() and xpcs_do_config().

> > +	bool need_reset;
> 
> If you still prefer the PCS-reset being done in the pre_config()
> function, then what about just directly checking the PMA id in there?
> 
> 	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID)
> 		return 0;
> 
> 	return xpcs_soft_reset(xpcs);

I think you've missed what "need_reset" is doing as you seem to
think it's just to make it conditional on the PMA ID. That's only
part of the story.

In the existing code, the reset only happens _once_ when the create
happens, not every time the PCS is configured. I am preserving this
behaviour, because I do _NOT_ wish to incorporate multiple functional
changes into one patch - and certainly in a cleanup series keep the
number of functional changes to a minimum.

So, all in all, I don't see the need to change anything in my patch.

Thanks for the feedback anyway.

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

Re: [PATCH RFC net-next 01/10] net: pcs: xpcs: move PCS reset to .pcs_pre_config()

[Serge Semin]

On Mon, Sep 30, 2024 at 11:14:15AM GMT, Russell King (Oracle) wrote:
> On Mon, Sep 30, 2024 at 01:16:57AM +0300, Serge Semin wrote:
> > Hi Russell
> > 
> > On Mon, Sep 23, 2024 at 03:00:59PM GMT, Russell King (Oracle) wrote:
> > > +static void xpcs_pre_config(struct phylink_pcs *pcs, phy_interface_t interface)
> > > +{
> > > +	struct dw_xpcs *xpcs = phylink_pcs_to_xpcs(pcs);
> > > +	const struct dw_xpcs_compat *compat;
> > > +	int ret;
> > > +
> > > +	if (!xpcs->need_reset)
> > > +		return;
> > > +
> > 
> > > +	compat = xpcs_find_compat(xpcs->desc, interface);
> > > +	if (!compat) {
> > > +		dev_err(&xpcs->mdiodev->dev, "unsupported interface %s\n",
> > > +			phy_modes(interface));
> > > +		return;
> > > +	}
> > 
> > Please note, it's better to preserve the xpcs_find_compat() call even
> > if the need_reset flag is false, since it makes sure that the
> > PHY-interface is actually supported by the PCS.
>
 
> Sorry, but I strongly disagree. xpcs_validate() will already have dealt
> with that, so we can be sure at this point that the interface is always
> valid. The NULL check is really only there because it'll stop the
> "you've forgotten to check for NULL on this function that can return
> NULL" brigade endlessly submitting patches to add something there -
> just like xpcs_get_state() and xpcs_do_config().

Thanks for the detailed answer. Indeed, I missed the part that the
pcs_validate() already does the interface check.

> 
> > > +	bool need_reset;
> > 
> > If you still prefer the PCS-reset being done in the pre_config()
> > function, then what about just directly checking the PMA id in there?
> > 
> > 	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID)
> > 		return 0;
> > 
> > 	return xpcs_soft_reset(xpcs);
> 
> I think you've missed what "need_reset" is doing as you seem to
> think it's just to make it conditional on the PMA ID. That's only
> part of the story.
> 
> In the existing code, the reset only happens _once_ when the create
> happens, not every time the PCS is configured. I am preserving this
> behaviour, because I do _NOT_ wish to incorporate multiple functional
> changes into one patch - and certainly in a cleanup series keep the
> number of functional changes to a minimum.

Ok. So the goal is to preserve the semantics. Seems reasonable. But... 

> 
> So, all in all, I don't see the need to change anything in my patch.

I'll get back to this patch discussion in the v1 series since you have
already submitted it.

-Serge(y)

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

[PATCH RFC net-next 02/10] net: pcs: xpcs: drop interface argument from internal functions

[Russell King (Oracle)]

Now that we no longer use the "interface" argument when creating the
XPCS sub-driver, remove it from xpcs_create() and xpcs_init_iface().

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/pcs/pcs-xpcs.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 7c6c40ddf722..2d8cc3959b4c 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -1483,7 +1483,7 @@ static int xpcs_init_id(struct dw_xpcs *xpcs)
 	return 0;
 }
 
-static int xpcs_init_iface(struct dw_xpcs *xpcs, phy_interface_t interface)
+static int xpcs_init_iface(struct dw_xpcs *xpcs)
 {
 	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID)
 		xpcs->pcs.poll = false;
@@ -1493,8 +1493,7 @@ static int xpcs_init_iface(struct dw_xpcs *xpcs, phy_interface_t interface)
 	return 0;
 }
 
-static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
-				   phy_interface_t interface)
+static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev)
 {
 	struct dw_xpcs *xpcs;
 	int ret;
@@ -1511,7 +1510,7 @@ static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
 	if (ret)
 		goto out_clear_clks;
 
-	ret = xpcs_init_iface(xpcs, interface);
+	ret = xpcs_init_iface(xpcs);
 	if (ret)
 		goto out_clear_clks;
 
@@ -1546,7 +1545,7 @@ struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr,
 	if (IS_ERR(mdiodev))
 		return ERR_CAST(mdiodev);
 
-	xpcs = xpcs_create(mdiodev, interface);
+	xpcs = xpcs_create(mdiodev);
 
 	/* xpcs_create() has taken a refcount on the mdiodev if it was
 	 * successful. If xpcs_create() fails, this will free the mdio
@@ -1584,7 +1583,7 @@ struct dw_xpcs *xpcs_create_fwnode(struct fwnode_handle *fwnode,
 	if (!mdiodev)
 		return ERR_PTR(-EPROBE_DEFER);
 
-	xpcs = xpcs_create(mdiodev, interface);
+	xpcs = xpcs_create(mdiodev);
 
 	/* xpcs_create() has taken a refcount on the mdiodev if it was
 	 * successful. If xpcs_create() fails, this will free the mdio
-- 
2.30.2

Re: [PATCH RFC net-next 02/10] net: pcs: xpcs: drop interface argument from internal functions

[Vladimir Oltean]

On Mon, Sep 23, 2024 at 03:01:04PM +0100, Russell King (Oracle) wrote:
> Now that we no longer use the "interface" argument when creating the
> XPCS sub-driver, remove it from xpcs_create() and xpcs_init_iface().
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

[PATCH RFC net-next 03/10] net: pcs: xpcs: get rid of xpcs_init_iface()

[Russell King (Oracle)]

xpcs_init_iface() no longer does anything with the interface mode, and
now merely does configuration related to the PMA ID. Move this back
into xpcs_create() as it doesn't warrant being a separate function
anymore.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/pcs/pcs-xpcs.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 2d8cc3959b4c..8765b01c0b5d 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -1483,16 +1483,6 @@ static int xpcs_init_id(struct dw_xpcs *xpcs)
 	return 0;
 }
 
-static int xpcs_init_iface(struct dw_xpcs *xpcs)
-{
-	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID)
-		xpcs->pcs.poll = false;
-	else
-		xpcs->need_reset = true;
-
-	return 0;
-}
-
 static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev)
 {
 	struct dw_xpcs *xpcs;
@@ -1510,9 +1500,10 @@ static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev)
 	if (ret)
 		goto out_clear_clks;
 
-	ret = xpcs_init_iface(xpcs);
-	if (ret)
-		goto out_clear_clks;
+	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID)
+		xpcs->pcs.poll = false;
+	else
+		xpcs->need_reset = true;
 
 	return xpcs;
 
-- 
2.30.2

Re: [PATCH RFC net-next 03/10] net: pcs: xpcs: get rid of xpcs_init_iface()

[Vladimir Oltean]

On Mon, Sep 23, 2024 at 03:01:10PM +0100, Russell King (Oracle) wrote:
> xpcs_init_iface() no longer does anything with the interface mode, and
> now merely does configuration related to the PMA ID. Move this back
> into xpcs_create() as it doesn't warrant being a separate function
> anymore.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

[PATCH RFC net-next 04/10] net: pcs: xpcs: add xpcs_destroy_pcs() and xpcs_create_pcs_mdiodev()

[Russell King (Oracle)]

Provide xpcs create/destroy functions that return and take a phylink_pcs
pointer instead of an xpcs pointer. This will be used by drivers that
have been converted to use phylink_pcs pointers internally, rather than
dw_xpcs pointers.

As xpcs_create_mdiodev() no longer makes use of its interface argument,
pass PHY_INTERFACE_MODE_NA into xpcs_create_mdiodev() until it is
removed later in the series.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/pcs/pcs-xpcs.c   | 18 ++++++++++++++++++
 include/linux/pcs/pcs-xpcs.h |  3 +++
 2 files changed, 21 insertions(+)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 8765b01c0b5d..9b61f97222b9 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -1550,6 +1550,18 @@ struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr,
 }
 EXPORT_SYMBOL_GPL(xpcs_create_mdiodev);
 
+struct phylink_pcs *xpcs_create_pcs_mdiodev(struct mii_bus *bus, int addr)
+{
+	struct dw_xpcs *xpcs;
+
+	xpcs = xpcs_create_mdiodev(bus, addr, PHY_INTERFACE_MODE_NA);
+	if (IS_ERR(xpcs))
+		return ERR_CAST(xpcs);
+
+	return &xpcs->pcs;
+}
+EXPORT_SYMBOL_GPL(xpcs_create_pcs_mdiodev);
+
 /**
  * xpcs_create_fwnode() - Create a DW xPCS instance from @fwnode
  * @fwnode: fwnode handle poining to the DW XPCS device
@@ -1599,5 +1611,11 @@ void xpcs_destroy(struct dw_xpcs *xpcs)
 }
 EXPORT_SYMBOL_GPL(xpcs_destroy);
 
+void xpcs_destroy_pcs(struct phylink_pcs *pcs)
+{
+	xpcs_destroy(phylink_pcs_to_xpcs(pcs));
+}
+EXPORT_SYMBOL_GPL(xpcs_destroy_pcs);
+
 MODULE_DESCRIPTION("Synopsys DesignWare XPCS library");
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index fd75d0605bb6..a4e2243ce647 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -78,4 +78,7 @@ struct dw_xpcs *xpcs_create_fwnode(struct fwnode_handle *fwnode,
 				   phy_interface_t interface);
 void xpcs_destroy(struct dw_xpcs *xpcs);
 
+struct phylink_pcs *xpcs_create_pcs_mdiodev(struct mii_bus *bus, int addr);
+void xpcs_destroy_pcs(struct phylink_pcs *pcs);
+
 #endif /* __LINUX_PCS_XPCS_H */
-- 
2.30.2

Re: [PATCH RFC net-next 04/10] net: pcs: xpcs: add xpcs_destroy_pcs() and xpcs_create_pcs_mdiodev()

[Vladimir Oltean]

On Mon, Sep 23, 2024 at 03:01:15PM +0100, Russell King (Oracle) wrote:
> Provide xpcs create/destroy functions that return and take a phylink_pcs
> pointer instead of an xpcs pointer. This will be used by drivers that
> have been converted to use phylink_pcs pointers internally, rather than
> dw_xpcs pointers.
> 
> As xpcs_create_mdiodev() no longer makes use of its interface argument,
> pass PHY_INTERFACE_MODE_NA into xpcs_create_mdiodev() until it is
> removed later in the series.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

[PATCH RFC net-next 05/10] net: wangxun: txgbe: use phylink_pcs internally

[Russell King (Oracle)]

Use xpcs_create_pcs_mdiodev() to create the XPCS instance, storing
and using the phylink_pcs pointer internally, rather than dw_xpcs.
Use xpcs_destroy_pcs() to destroy the XPCS instance when we've
finished with it.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c | 18 +++++++++---------
 .../net/ethernet/wangxun/txgbe/txgbe_type.h    |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
index 67b61afdde96..3dd89dafe7c7 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
@@ -122,7 +122,7 @@ static int txgbe_pcs_write(struct mii_bus *bus, int addr, int devnum, int regnum
 static int txgbe_mdio_pcs_init(struct txgbe *txgbe)
 {
 	struct mii_bus *mii_bus;
-	struct dw_xpcs *xpcs;
+	struct phylink_pcs *pcs;
 	struct pci_dev *pdev;
 	struct wx *wx;
 	int ret = 0;
@@ -147,11 +147,11 @@ static int txgbe_mdio_pcs_init(struct txgbe *txgbe)
 	if (ret)
 		return ret;
 
-	xpcs = xpcs_create_mdiodev(mii_bus, 0, PHY_INTERFACE_MODE_10GBASER);
-	if (IS_ERR(xpcs))
-		return PTR_ERR(xpcs);
+	pcs = xpcs_create_pcs_mdiodev(mii_bus, 0);
+	if (IS_ERR(pcs))
+		return PTR_ERR(pcs);
 
-	txgbe->xpcs = xpcs;
+	txgbe->pcs = pcs;
 
 	return 0;
 }
@@ -163,7 +163,7 @@ static struct phylink_pcs *txgbe_phylink_mac_select(struct phylink_config *confi
 	struct txgbe *txgbe = wx->priv;
 
 	if (interface == PHY_INTERFACE_MODE_10GBASER)
-		return &txgbe->xpcs->pcs;
+		return txgbe->pcs;
 
 	return NULL;
 }
@@ -302,7 +302,7 @@ irqreturn_t txgbe_link_irq_handler(int irq, void *data)
 	status = rd32(wx, TXGBE_CFG_PORT_ST);
 	up = !!(status & TXGBE_CFG_PORT_ST_LINK_UP);
 
-	phylink_pcs_change(&txgbe->xpcs->pcs, up);
+	phylink_pcs_change(txgbe->pcs, up);
 
 	return IRQ_HANDLED;
 }
@@ -778,7 +778,7 @@ int txgbe_init_phy(struct txgbe *txgbe)
 err_destroy_phylink:
 	phylink_destroy(wx->phylink);
 err_destroy_xpcs:
-	xpcs_destroy(txgbe->xpcs);
+	xpcs_destroy_pcs(txgbe->pcs);
 err_unregister_swnode:
 	software_node_unregister_node_group(txgbe->nodes.group);
 
@@ -798,6 +798,6 @@ void txgbe_remove_phy(struct txgbe *txgbe)
 	clkdev_drop(txgbe->clock);
 	clk_unregister(txgbe->clk);
 	phylink_destroy(txgbe->wx->phylink);
-	xpcs_destroy(txgbe->xpcs);
+	xpcs_destroy_pcs(txgbe->pcs);
 	software_node_unregister_node_group(txgbe->nodes.group);
 }
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
index 959102c4c379..cc3a7b62fe9e 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
@@ -329,7 +329,7 @@ struct txgbe {
 	struct wx *wx;
 	struct txgbe_nodes nodes;
 	struct txgbe_irq misc;
-	struct dw_xpcs *xpcs;
+	struct phylink_pcs *pcs;
 	struct platform_device *sfp_dev;
 	struct platform_device *i2c_dev;
 	struct clk_lookup *clock;
-- 
2.30.2

[PATCH RFC net-next 06/10] net: dsa: sja1105: simplify static configuration reload

[Russell King (Oracle)]

The static configuration reload saves the port speed in the static
configuration tables by first converting it from the internal
respresentation to the SPEED_xxx ethtool representation, and then
converts it back to restore the setting. This is because
sja1105_adjust_port_config() takes the speed as SPEED_xxx.

However, this is unnecessarily complex. If we split
sja1105_adjust_port_config() up, we can simply save and restore the
mac[port].speed member in the static configuration tables.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 63 +++++++++++++-------------
 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index bc7e50dcb57c..b95c64b7e705 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1257,29 +1257,11 @@ static int sja1105_parse_dt(struct sja1105_private *priv)
 	return rc;
 }
 
-/* Convert link speed from SJA1105 to ethtool encoding */
-static int sja1105_port_speed_to_ethtool(struct sja1105_private *priv,
-					 u64 speed)
-{
-	if (speed == priv->info->port_speed[SJA1105_SPEED_10MBPS])
-		return SPEED_10;
-	if (speed == priv->info->port_speed[SJA1105_SPEED_100MBPS])
-		return SPEED_100;
-	if (speed == priv->info->port_speed[SJA1105_SPEED_1000MBPS])
-		return SPEED_1000;
-	if (speed == priv->info->port_speed[SJA1105_SPEED_2500MBPS])
-		return SPEED_2500;
-	return SPEED_UNKNOWN;
-}
-
-/* Set link speed in the MAC configuration for a specific port. */
-static int sja1105_adjust_port_config(struct sja1105_private *priv, int port,
-				      int speed_mbps)
+static int sja1105_set_port_speed(struct sja1105_private *priv, int port,
+				  int speed_mbps)
 {
 	struct sja1105_mac_config_entry *mac;
-	struct device *dev = priv->ds->dev;
 	u64 speed;
-	int rc;
 
 	/* On P/Q/R/S, one can read from the device via the MAC reconfiguration
 	 * tables. On E/T, MAC reconfig tables are not readable, only writable.
@@ -1313,7 +1295,7 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port,
 		speed = priv->info->port_speed[SJA1105_SPEED_2500MBPS];
 		break;
 	default:
-		dev_err(dev, "Invalid speed %iMbps\n", speed_mbps);
+		dev_err(priv->ds->dev, "Invalid speed %iMbps\n", speed_mbps);
 		return -EINVAL;
 	}
 
@@ -1325,11 +1307,29 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port,
 	 * we need to configure the PCS only (if even that).
 	 */
 	if (priv->phy_mode[port] == PHY_INTERFACE_MODE_SGMII)
-		mac[port].speed = priv->info->port_speed[SJA1105_SPEED_1000MBPS];
+		speed = priv->info->port_speed[SJA1105_SPEED_1000MBPS];
 	else if (priv->phy_mode[port] == PHY_INTERFACE_MODE_2500BASEX)
-		mac[port].speed = priv->info->port_speed[SJA1105_SPEED_2500MBPS];
-	else
-		mac[port].speed = speed;
+		speed = priv->info->port_speed[SJA1105_SPEED_2500MBPS];
+
+	mac[port].speed = speed;
+
+	return 0;
+}
+
+/* Set link speed in the MAC configuration for a specific port. */
+static int sja1105_set_port_config(struct sja1105_private *priv, int port)
+{
+	struct sja1105_mac_config_entry *mac;
+	struct device *dev = priv->ds->dev;
+	int rc;
+
+	/* On P/Q/R/S, one can read from the device via the MAC reconfiguration
+	 * tables. On E/T, MAC reconfig tables are not readable, only writable.
+	 * We have to *know* what the MAC looks like.  For the sake of keeping
+	 * the code common, we'll use the static configuration tables as a
+	 * reasonable approximation for both E/T and P/Q/R/S.
+	 */
+	mac = priv->static_config.tables[BLK_IDX_MAC_CONFIG].entries;
 
 	/* Write to the dynamic reconfiguration tables */
 	rc = sja1105_dynamic_config_write(priv, BLK_IDX_MAC_CONFIG, port,
@@ -1390,7 +1390,8 @@ static void sja1105_mac_link_up(struct phylink_config *config,
 	struct sja1105_private *priv = dp->ds->priv;
 	int port = dp->index;
 
-	sja1105_adjust_port_config(priv, port, speed);
+	if (!sja1105_set_port_speed(priv, port, speed))
+		sja1105_set_port_config(priv, port);
 
 	sja1105_inhibit_tx(priv, BIT(port), false);
 }
@@ -2293,7 +2294,7 @@ int sja1105_static_config_reload(struct sja1105_private *priv,
 {
 	struct ptp_system_timestamp ptp_sts_before;
 	struct ptp_system_timestamp ptp_sts_after;
-	int speed_mbps[SJA1105_MAX_NUM_PORTS];
+	u64 mac_speed[SJA1105_MAX_NUM_PORTS];
 	u16 bmcr[SJA1105_MAX_NUM_PORTS] = {0};
 	struct sja1105_mac_config_entry *mac;
 	struct dsa_switch *ds = priv->ds;
@@ -2307,14 +2308,13 @@ int sja1105_static_config_reload(struct sja1105_private *priv,
 
 	mac = priv->static_config.tables[BLK_IDX_MAC_CONFIG].entries;
 
-	/* Back up the dynamic link speed changed by sja1105_adjust_port_config
+	/* Back up the dynamic link speed changed by sja1105_set_port_speed
 	 * in order to temporarily restore it to SJA1105_SPEED_AUTO - which the
 	 * switch wants to see in the static config in order to allow us to
 	 * change it through the dynamic interface later.
 	 */
 	for (i = 0; i < ds->num_ports; i++) {
-		speed_mbps[i] = sja1105_port_speed_to_ethtool(priv,
-							      mac[i].speed);
+		mac_speed[i] = mac[i].speed;
 		mac[i].speed = priv->info->port_speed[SJA1105_SPEED_AUTO];
 
 		if (priv->xpcs[i])
@@ -2377,7 +2377,8 @@ int sja1105_static_config_reload(struct sja1105_private *priv,
 		struct dw_xpcs *xpcs = priv->xpcs[i];
 		unsigned int neg_mode;
 
-		rc = sja1105_adjust_port_config(priv, i, speed_mbps[i]);
+		mac[i].speed = mac_speed[i];
+		rc = sja1105_set_port_config(priv, i);
 		if (rc < 0)
 			goto out;
 
-- 
2.30.2

Re: [PATCH RFC net-next 06/10] net: dsa: sja1105: simplify static configuration reload

[Vladimir Oltean]

On Mon, Sep 23, 2024 at 03:01:25PM +0100, Russell King (Oracle) wrote:
> The static configuration reload saves the port speed in the static
> configuration tables by first converting it from the internal
> respresentation to the SPEED_xxx ethtool representation, and then
> converts it back to restore the setting. This is because
> sja1105_adjust_port_config() takes the speed as SPEED_xxx.
> 
> However, this is unnecessarily complex. If we split
> sja1105_adjust_port_config() up, we can simply save and restore the
> mac[port].speed member in the static configuration tables.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Thanks for the patch, and the idea is good.

>  drivers/net/dsa/sja1105/sja1105_main.c | 63 +++++++++++++-------------
>  1 file changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
> index bc7e50dcb57c..b95c64b7e705 100644
> --- a/drivers/net/dsa/sja1105/sja1105_main.c
> +++ b/drivers/net/dsa/sja1105/sja1105_main.c
> @@ -1257,29 +1257,11 @@ static int sja1105_parse_dt(struct sja1105_private *priv)
>  	return rc;
>  }
>  
> -/* Convert link speed from SJA1105 to ethtool encoding */
> -static int sja1105_port_speed_to_ethtool(struct sja1105_private *priv,
> -					 u64 speed)
> -{
> -	if (speed == priv->info->port_speed[SJA1105_SPEED_10MBPS])
> -		return SPEED_10;
> -	if (speed == priv->info->port_speed[SJA1105_SPEED_100MBPS])
> -		return SPEED_100;
> -	if (speed == priv->info->port_speed[SJA1105_SPEED_1000MBPS])
> -		return SPEED_1000;
> -	if (speed == priv->info->port_speed[SJA1105_SPEED_2500MBPS])
> -		return SPEED_2500;
> -	return SPEED_UNKNOWN;
> -}
> -
> -/* Set link speed in the MAC configuration for a specific port. */
> -static int sja1105_adjust_port_config(struct sja1105_private *priv, int port,
> -				      int speed_mbps)
> +static int sja1105_set_port_speed(struct sja1105_private *priv, int port,
> +				  int speed_mbps)
>  {
>  	struct sja1105_mac_config_entry *mac;
> -	struct device *dev = priv->ds->dev;

I think if you could keep this line in the new sja1105_set_port_speed()
function..

>  	u64 speed;
> -	int rc;
>  
>  	/* On P/Q/R/S, one can read from the device via the MAC reconfiguration
>  	 * tables. On E/T, MAC reconfig tables are not readable, only writable.
> @@ -1313,7 +1295,7 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port,
>  		speed = priv->info->port_speed[SJA1105_SPEED_2500MBPS];
>  		break;
>  	default:
> -		dev_err(dev, "Invalid speed %iMbps\n", speed_mbps);
> +		dev_err(priv->ds->dev, "Invalid speed %iMbps\n", speed_mbps);

you could also get rid of this unnecessary line change.

>  		return -EINVAL;
>  	}

There are 2 more changes which I believe should be made in sja1105_set_port_speed():
- since it isn't called from mac_config() anymore but from mac_link_up()
  (change which happened quite a while ago), it mustn't handle SPEED_UNKNOWN
- we can trust that phylink will not call mac_link_up() with a speed
  outside what we provided in mac_capabilities, so we can remove the
  -EINVAL "default" speed_mbps case, and make this method return void,
  as it can never truly cause an error

But I believe these are incremental changes which should be done after
this patch. I've made a note of them and will create 2 patches on top
when I have the spare time.

>  
> @@ -1325,11 +1307,29 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port,
>  	 * we need to configure the PCS only (if even that).
>  	 */
>  	if (priv->phy_mode[port] == PHY_INTERFACE_MODE_SGMII)
> -		mac[port].speed = priv->info->port_speed[SJA1105_SPEED_1000MBPS];
> +		speed = priv->info->port_speed[SJA1105_SPEED_1000MBPS];
>  	else if (priv->phy_mode[port] == PHY_INTERFACE_MODE_2500BASEX)
> -		mac[port].speed = priv->info->port_speed[SJA1105_SPEED_2500MBPS];
> -	else
> -		mac[port].speed = speed;
> +		speed = priv->info->port_speed[SJA1105_SPEED_2500MBPS];
> +
> +	mac[port].speed = speed;
> +
> +	return 0;
> +}
> +
> +/* Set link speed in the MAC configuration for a specific port. */

Could this comment state this instead? "Write the MAC Configuration
Table entry and, if necessary, the CGU settings, after a link speed
change for this port."

> +static int sja1105_set_port_config(struct sja1105_private *priv, int port)
> +{
> +	struct sja1105_mac_config_entry *mac;
> +	struct device *dev = priv->ds->dev;
> +	int rc;
> +
> +	/* On P/Q/R/S, one can read from the device via the MAC reconfiguration
> +	 * tables. On E/T, MAC reconfig tables are not readable, only writable.
> +	 * We have to *know* what the MAC looks like.  For the sake of keeping
> +	 * the code common, we'll use the static configuration tables as a
> +	 * reasonable approximation for both E/T and P/Q/R/S.
> +	 */
> +	mac = priv->static_config.tables[BLK_IDX_MAC_CONFIG].entries;
>  
>  	/* Write to the dynamic reconfiguration tables */
>  	rc = sja1105_dynamic_config_write(priv, BLK_IDX_MAC_CONFIG, port,
> @@ -1390,7 +1390,8 @@ static void sja1105_mac_link_up(struct phylink_config *config,
>  	struct sja1105_private *priv = dp->ds->priv;
>  	int port = dp->index;
>  
> -	sja1105_adjust_port_config(priv, port, speed);
> +	if (!sja1105_set_port_speed(priv, port, speed))
> +		sja1105_set_port_config(priv, port);
>  
>  	sja1105_inhibit_tx(priv, BIT(port), false);
>  }
> @@ -2293,7 +2294,7 @@ int sja1105_static_config_reload(struct sja1105_private *priv,
>  {
>  	struct ptp_system_timestamp ptp_sts_before;
>  	struct ptp_system_timestamp ptp_sts_after;
> -	int speed_mbps[SJA1105_MAX_NUM_PORTS];
> +	u64 mac_speed[SJA1105_MAX_NUM_PORTS];

Could you move this line lower to preserve the ordering by decreasing line length?

>  	u16 bmcr[SJA1105_MAX_NUM_PORTS] = {0};
>  	struct sja1105_mac_config_entry *mac;
>  	struct dsa_switch *ds = priv->ds;
> @@ -2307,14 +2308,13 @@ int sja1105_static_config_reload(struct sja1105_private *priv,
>  
>  	mac = priv->static_config.tables[BLK_IDX_MAC_CONFIG].entries;
>  
> -	/* Back up the dynamic link speed changed by sja1105_adjust_port_config
> +	/* Back up the dynamic link speed changed by sja1105_set_port_speed

Could you please put () after the function name? I know the original
code did not have it, but my coding style has changed in the past 5 years.

>  	 * in order to temporarily restore it to SJA1105_SPEED_AUTO - which the
>  	 * switch wants to see in the static config in order to allow us to
>  	 * change it through the dynamic interface later.
>  	 */
>  	for (i = 0; i < ds->num_ports; i++) {
> -		speed_mbps[i] = sja1105_port_speed_to_ethtool(priv,
> -							      mac[i].speed);
> +		mac_speed[i] = mac[i].speed;
>  		mac[i].speed = priv->info->port_speed[SJA1105_SPEED_AUTO];
>  
>  		if (priv->xpcs[i])
> @@ -2377,7 +2377,8 @@ int sja1105_static_config_reload(struct sja1105_private *priv,
>  		struct dw_xpcs *xpcs = priv->xpcs[i];
>  		unsigned int neg_mode;
>  
> -		rc = sja1105_adjust_port_config(priv, i, speed_mbps[i]);
> +		mac[i].speed = mac_speed[i];
> +		rc = sja1105_set_port_config(priv, i);
>  		if (rc < 0)
>  			goto out;
>  
> -- 
> 2.30.2
> 

Re: [PATCH RFC net-next 06/10] net: dsa: sja1105: simplify static configuration reload

[Russell King (Oracle)]

On Wed, Sep 25, 2024 at 04:15:17PM +0300, Vladimir Oltean wrote:
> On Mon, Sep 23, 2024 at 03:01:25PM +0100, Russell King (Oracle) wrote:
> > +static int sja1105_set_port_speed(struct sja1105_private *priv, int port,
> > +				  int speed_mbps)
> >  {
> >  	struct sja1105_mac_config_entry *mac;
> > -	struct device *dev = priv->ds->dev;
> 
> I think if you could keep this line in the new sja1105_set_port_speed()
> function..
> 
> >  	u64 speed;
> > -	int rc;
> >  
> >  	/* On P/Q/R/S, one can read from the device via the MAC reconfiguration
> >  	 * tables. On E/T, MAC reconfig tables are not readable, only writable.
> > @@ -1313,7 +1295,7 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port,
> >  		speed = priv->info->port_speed[SJA1105_SPEED_2500MBPS];
> >  		break;
> >  	default:
> > -		dev_err(dev, "Invalid speed %iMbps\n", speed_mbps);
> > +		dev_err(priv->ds->dev, "Invalid speed %iMbps\n", speed_mbps);
> 
> you could also get rid of this unnecessary line change.

Yes, maybe I'll move it to another patch, but the reason I made the
change is that I don't see much point to the local variable existing
for just one user (there were multiple users prior to this patch.)
However...

> >  		return -EINVAL;
> >  	}
> 
> There are 2 more changes which I believe should be made in sja1105_set_port_speed():
> - since it isn't called from mac_config() anymore but from mac_link_up()
>   (change which happened quite a while ago), it mustn't handle SPEED_UNKNOWN
> - we can trust that phylink will not call mac_link_up() with a speed
>   outside what we provided in mac_capabilities, so we can remove the
>   -EINVAL "default" speed_mbps case, and make this method return void,
>   as it can never truly cause an error
> 
> But I believe these are incremental changes which should be done after
> this patch. I've made a note of them and will create 2 patches on top
> when I have the spare time.

... if we were to make those changes prior to this patch, then the
dev_err() will no longer be there and thus this becomes a non-issue.
So I'd suggest a patch prior to this one to make the changes you state
here, thus eliminating the need for this hunk in this patch.

> > +/* Set link speed in the MAC configuration for a specific port. */
> 
> Could this comment state this instead? "Write the MAC Configuration
> Table entry and, if necessary, the CGU settings, after a link speed
> change for this port."

Done.

> > @@ -2293,7 +2294,7 @@ int sja1105_static_config_reload(struct sja1105_private *priv,
> >  {
> >  	struct ptp_system_timestamp ptp_sts_before;
> >  	struct ptp_system_timestamp ptp_sts_after;
> > -	int speed_mbps[SJA1105_MAX_NUM_PORTS];
> > +	u64 mac_speed[SJA1105_MAX_NUM_PORTS];
> 
> Could you move this line lower to preserve the ordering by decreasing line length?
> 
> >  	u16 bmcr[SJA1105_MAX_NUM_PORTS] = {0};

Probably didn't notice that due to not having full clear sight for the
screen yet (a few more weeks to go before that happens!) Thanks for
spotting that.

> > -	/* Back up the dynamic link speed changed by sja1105_adjust_port_config
> > +	/* Back up the dynamic link speed changed by sja1105_set_port_speed
> 
> Could you please put () after the function name? I know the original
> code did not have it, but my coding style has changed in the past 5 years.

Done.

Thanks!

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

Re: [PATCH RFC net-next 06/10] net: dsa: sja1105: simplify static configuration reload

[Vladimir Oltean]

On Wed, Sep 25, 2024 at 08:38:23PM +0100, Russell King (Oracle) wrote:
> > There are 2 more changes which I believe should be made in sja1105_set_port_speed():
> > - since it isn't called from mac_config() anymore but from mac_link_up()
> >   (change which happened quite a while ago), it mustn't handle SPEED_UNKNOWN
> > - we can trust that phylink will not call mac_link_up() with a speed
> >   outside what we provided in mac_capabilities, so we can remove the
> >   -EINVAL "default" speed_mbps case, and make this method return void,
> >   as it can never truly cause an error
> > 
> > But I believe these are incremental changes which should be done after
> > this patch. I've made a note of them and will create 2 patches on top
> > when I have the spare time.
> 
> ... if we were to make those changes prior to this patch, then the
> dev_err() will no longer be there and thus this becomes a non-issue.
> So I'd suggest a patch prior to this one to make the changes you state
> here, thus eliminating the need for this hunk in this patch.

That sounds good. Are you suggesting you will write up such a patch for v2?

Re: [PATCH RFC net-next 06/10] net: dsa: sja1105: simplify static configuration reload

[Russell King (Oracle)]

On Thu, Sep 26, 2024 at 12:16:13AM +0300, Vladimir Oltean wrote:
> On Wed, Sep 25, 2024 at 08:38:23PM +0100, Russell King (Oracle) wrote:
> > > There are 2 more changes which I believe should be made in sja1105_set_port_speed():
> > > - since it isn't called from mac_config() anymore but from mac_link_up()
> > >   (change which happened quite a while ago), it mustn't handle SPEED_UNKNOWN
> > > - we can trust that phylink will not call mac_link_up() with a speed
> > >   outside what we provided in mac_capabilities, so we can remove the
> > >   -EINVAL "default" speed_mbps case, and make this method return void,
> > >   as it can never truly cause an error
> > > 
> > > But I believe these are incremental changes which should be done after
> > > this patch. I've made a note of them and will create 2 patches on top
> > > when I have the spare time.
> > 
> > ... if we were to make those changes prior to this patch, then the
> > dev_err() will no longer be there and thus this becomes a non-issue.
> > So I'd suggest a patch prior to this one to make the changes you state
> > here, thus eliminating the need for this hunk in this patch.
> 
> That sounds good. Are you suggesting you will write up such a patch for v2?

Actually, the three patches become interdependent.

Let's say we want to eliminate SPEED_UNKNOWN. Prior to my patch in this
sub-thread, we have this:

                speed_mbps[i] = sja1105_port_speed_to_ethtool(priv,
                                                              mac[i].speed);
...
                rc = sja1105_adjust_port_config(priv, i, speed_mbps[i]);

sja1105_port_speed_to_ethtool() can return SPEED_UNKNOWN if
mac[i].speed is not one of the four encodings. If we can't guarantee
that it is one of the four encodings, then SPEED_UNKNOWN may be
passed into sja1105_adjust_port_config().

Similarly, as for the default case, we can't simply delete that,
because that'll leave "speed" uninitialised and we'll get a build
warning without my changes. We could change the default case to
simply:

	default:
		return 0;

but that just looks perverse.

So, I think rather than trying to do your suggestion before my patch,
my patch needs to stand as it currently is, and then your suggestion
must happen after it - otherwise we end up introducing more complexity
or weirdness.

Hmm?

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

Re: [PATCH RFC net-next 06/10] net: dsa: sja1105: simplify static configuration reload

[Vladimir Oltean]

On Thu, Sep 26, 2024 at 01:02:35PM +0100, Russell King (Oracle) wrote:
> On Thu, Sep 26, 2024 at 12:16:13AM +0300, Vladimir Oltean wrote:
> > On Wed, Sep 25, 2024 at 08:38:23PM +0100, Russell King (Oracle) wrote:
> > > > There are 2 more changes which I believe should be made in sja1105_set_port_speed():
> > > > - since it isn't called from mac_config() anymore but from mac_link_up()
> > > >   (change which happened quite a while ago), it mustn't handle SPEED_UNKNOWN
> > > > - we can trust that phylink will not call mac_link_up() with a speed
> > > >   outside what we provided in mac_capabilities, so we can remove the
> > > >   -EINVAL "default" speed_mbps case, and make this method return void,
> > > >   as it can never truly cause an error
> > > > 
> > > > But I believe these are incremental changes which should be done after
> > > > this patch. I've made a note of them and will create 2 patches on top
> > > > when I have the spare time.
> > > 
> > > ... if we were to make those changes prior to this patch, then the
> > > dev_err() will no longer be there and thus this becomes a non-issue.
> > > So I'd suggest a patch prior to this one to make the changes you state
> > > here, thus eliminating the need for this hunk in this patch.
> > 
> > That sounds good. Are you suggesting you will write up such a patch for v2?
> 
> Actually, the three patches become interdependent.
> 
> Let's say we want to eliminate SPEED_UNKNOWN. Prior to my patch in this
> sub-thread, we have this:
> 
>                 speed_mbps[i] = sja1105_port_speed_to_ethtool(priv,
>                                                               mac[i].speed);
> ...
>                 rc = sja1105_adjust_port_config(priv, i, speed_mbps[i]);
> 
> sja1105_port_speed_to_ethtool() can return SPEED_UNKNOWN if
> mac[i].speed is not one of the four encodings. If we can't guarantee
> that it is one of the four encodings, then SPEED_UNKNOWN may be
> passed into sja1105_adjust_port_config().
> 
> Similarly, as for the default case, we can't simply delete that,
> because that'll leave "speed" uninitialised and we'll get a build
> warning without my changes. We could change the default case to
> simply:
> 
> 	default:
> 		return 0;
> 
> but that just looks perverse.
> 
> So, I think rather than trying to do your suggestion before my patch,
> my patch needs to stand as it currently is, and then your suggestion
> must happen after it - otherwise we end up introducing more complexity
> or weirdness.
> 
> Hmm?

Ok, if we're back to my original proposal, I'm implicitly okay with that.

[PATCH RFC net-next 07/10] net: dsa: sja1105: call PCS config/link_up via pcs_ops structure

[Russell King (Oracle)]

Call the PCS operations through the ops structure, which avoids needing
to export xpcs internal functions.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index b95c64b7e705..8ef1a1931a33 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -2375,6 +2375,7 @@ int sja1105_static_config_reload(struct sja1105_private *priv,
 
 	for (i = 0; i < ds->num_ports; i++) {
 		struct dw_xpcs *xpcs = priv->xpcs[i];
+		struct phylink_pcs *pcs;
 		unsigned int neg_mode;
 
 		mac[i].speed = mac_speed[i];
@@ -2385,12 +2386,15 @@ int sja1105_static_config_reload(struct sja1105_private *priv,
 		if (!xpcs)
 			continue;
 
+		pcs = &xpcs->pcs;
+
 		if (bmcr[i] & BMCR_ANENABLE)
 			neg_mode = PHYLINK_PCS_NEG_INBAND_ENABLED;
 		else
 			neg_mode = PHYLINK_PCS_NEG_OUTBAND;
 
-		rc = xpcs_do_config(xpcs, priv->phy_mode[i], NULL, neg_mode);
+		rc = pcs->ops->pcs_config(pcs, neg_mode, priv->phy_mode[i],
+					  NULL, true);
 		if (rc < 0)
 			goto out;
 
@@ -2406,8 +2410,8 @@ int sja1105_static_config_reload(struct sja1105_private *priv,
 			else
 				speed = SPEED_10;
 
-			xpcs_link_up(&xpcs->pcs, neg_mode, priv->phy_mode[i],
-				     speed, DUPLEX_FULL);
+			pcs->ops->pcs_link_up(pcs, neg_mode, priv->phy_mode[i],
+					      speed, DUPLEX_FULL);
 		}
 	}
 
-- 
2.30.2

Re: [PATCH RFC net-next 07/10] net: dsa: sja1105: call PCS config/link_up via pcs_ops structure

[Vladimir Oltean]

On Mon, Sep 23, 2024 at 03:01:30PM +0100, Russell King (Oracle) wrote:
> Call the PCS operations through the ops structure, which avoids needing
> to export xpcs internal functions.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>

[PATCH RFC net-next 08/10] net: dsa: sja1105: use phylink_pcs internally

[Russell King (Oracle)]

Use xpcs_create_pcs_mdiodev() to create the XPCS instance, storing
and using the phylink_pcs pointer internally, rather than dw_xpcs.
Use xpcs_destroy_pcs() to destroy the XPCS instance when we've
finished with it.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/sja1105/sja1105.h      |  2 +-
 drivers/net/dsa/sja1105/sja1105_main.c | 16 ++++-----------
 drivers/net/dsa/sja1105/sja1105_mdio.c | 28 ++++++++++++--------------
 3 files changed, 18 insertions(+), 28 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
index 8c66d3bf61f0..dceb96ae9c83 100644
--- a/drivers/net/dsa/sja1105/sja1105.h
+++ b/drivers/net/dsa/sja1105/sja1105.h
@@ -278,7 +278,7 @@ struct sja1105_private {
 	struct mii_bus *mdio_base_t1;
 	struct mii_bus *mdio_base_tx;
 	struct mii_bus *mdio_pcs;
-	struct dw_xpcs *xpcs[SJA1105_MAX_NUM_PORTS];
+	struct phylink_pcs *pcs[SJA1105_MAX_NUM_PORTS];
 	struct sja1105_ptp_data ptp_data;
 	struct sja1105_tas_data tas_data;
 };
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 8ef1a1931a33..c86905c94765 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -15,7 +15,6 @@
 #include <linux/of.h>
 #include <linux/of_net.h>
 #include <linux/of_mdio.h>
-#include <linux/pcs/pcs-xpcs.h>
 #include <linux/netdev_features.h>
 #include <linux/netdevice.h>
 #include <linux/if_bridge.h>
@@ -1356,12 +1355,8 @@ sja1105_mac_select_pcs(struct phylink_config *config, phy_interface_t iface)
 {
 	struct dsa_port *dp = dsa_phylink_to_port(config);
 	struct sja1105_private *priv = dp->ds->priv;
-	struct dw_xpcs *xpcs = priv->xpcs[dp->index];
 
-	if (xpcs)
-		return &xpcs->pcs;
-
-	return NULL;
+	return priv->pcs[dp->index];
 }
 
 static void sja1105_mac_config(struct phylink_config *config,
@@ -2317,7 +2312,7 @@ int sja1105_static_config_reload(struct sja1105_private *priv,
 		mac_speed[i] = mac[i].speed;
 		mac[i].speed = priv->info->port_speed[SJA1105_SPEED_AUTO];
 
-		if (priv->xpcs[i])
+		if (priv->pcs[i])
 			bmcr[i] = mdiobus_c45_read(priv->mdio_pcs, i,
 						   MDIO_MMD_VEND2, MDIO_CTRL1);
 	}
@@ -2374,8 +2369,7 @@ int sja1105_static_config_reload(struct sja1105_private *priv,
 	}
 
 	for (i = 0; i < ds->num_ports; i++) {
-		struct dw_xpcs *xpcs = priv->xpcs[i];
-		struct phylink_pcs *pcs;
+		struct phylink_pcs *pcs = priv->pcs[i];
 		unsigned int neg_mode;
 
 		mac[i].speed = mac_speed[i];
@@ -2383,11 +2377,9 @@ int sja1105_static_config_reload(struct sja1105_private *priv,
 		if (rc < 0)
 			goto out;
 
-		if (!xpcs)
+		if (!pcs)
 			continue;
 
-		pcs = &xpcs->pcs;
-
 		if (bmcr[i] & BMCR_ANENABLE)
 			neg_mode = PHYLINK_PCS_NEG_INBAND_ENABLED;
 		else
diff --git a/drivers/net/dsa/sja1105/sja1105_mdio.c b/drivers/net/dsa/sja1105/sja1105_mdio.c
index 52ddb4ef259e..84b7169f2974 100644
--- a/drivers/net/dsa/sja1105/sja1105_mdio.c
+++ b/drivers/net/dsa/sja1105/sja1105_mdio.c
@@ -400,7 +400,7 @@ static int sja1105_mdiobus_pcs_register(struct sja1105_private *priv)
 	}
 
 	for (port = 0; port < ds->num_ports; port++) {
-		struct dw_xpcs *xpcs;
+		struct phylink_pcs *pcs;
 
 		if (dsa_is_unused_port(ds, port))
 			continue;
@@ -409,13 +409,13 @@ static int sja1105_mdiobus_pcs_register(struct sja1105_private *priv)
 		    priv->phy_mode[port] != PHY_INTERFACE_MODE_2500BASEX)
 			continue;
 
-		xpcs = xpcs_create_mdiodev(bus, port, priv->phy_mode[port]);
-		if (IS_ERR(xpcs)) {
-			rc = PTR_ERR(xpcs);
+		pcs = xpcs_create_pcs_mdiodev(bus, port);
+		if (IS_ERR(pcs)) {
+			rc = PTR_ERR(pcs);
 			goto out_pcs_free;
 		}
 
-		priv->xpcs[port] = xpcs;
+		priv->pcs[port] = pcs;
 	}
 
 	priv->mdio_pcs = bus;
@@ -424,11 +424,10 @@ static int sja1105_mdiobus_pcs_register(struct sja1105_private *priv)
 
 out_pcs_free:
 	for (port = 0; port < ds->num_ports; port++) {
-		if (!priv->xpcs[port])
-			continue;
-
-		xpcs_destroy(priv->xpcs[port]);
-		priv->xpcs[port] = NULL;
+		if (priv->pcs[port]) {
+			xpcs_destroy_pcs(priv->pcs[port]);
+			priv->pcs[port] = NULL;
+		}
 	}
 
 	mdiobus_unregister(bus);
@@ -446,11 +445,10 @@ static void sja1105_mdiobus_pcs_unregister(struct sja1105_private *priv)
 		return;
 
 	for (port = 0; port < ds->num_ports; port++) {
-		if (!priv->xpcs[port])
-			continue;
-
-		xpcs_destroy(priv->xpcs[port]);
-		priv->xpcs[port] = NULL;
+		if (priv->pcs[port]) {
+			xpcs_destroy_pcs(priv->pcs[port]);
+			priv->pcs[port] = NULL;
+		}
 	}
 
 	mdiobus_unregister(priv->mdio_pcs);
-- 
2.30.2

Re: [PATCH RFC net-next 08/10] net: dsa: sja1105: use phylink_pcs internally

[Vladimir Oltean]

On Mon, Sep 23, 2024 at 03:01:35PM +0100, Russell King (Oracle) wrote:
> Use xpcs_create_pcs_mdiodev() to create the XPCS instance, storing
> and using the phylink_pcs pointer internally, rather than dw_xpcs.
> Use xpcs_destroy_pcs() to destroy the XPCS instance when we've
> finished with it.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>

[PATCH RFC net-next 09/10] net: pcs: xpcs: drop interface argument from xpcs_create*()

[Russell King (Oracle)]

The XPCS sub-driver no longer uses the "interface" argument to the
xpcs_create_mdiodev() and xpcs_create_fwnode() functions. Remove
this now unnecessary argument, updating the stmmac driver
appropriately.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c |  7 +++----
 drivers/net/pcs/pcs-xpcs.c                        | 10 +++-------
 include/linux/pcs/pcs-xpcs.h                      |  6 ++----
 3 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index 03f90676b3ad..0c7d81ddd440 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -500,23 +500,22 @@ int stmmac_pcs_setup(struct net_device *ndev)
 	struct fwnode_handle *devnode, *pcsnode;
 	struct dw_xpcs *xpcs = NULL;
 	struct stmmac_priv *priv;
-	int addr, mode, ret;
+	int addr, ret;
 
 	priv = netdev_priv(ndev);
-	mode = priv->plat->phy_interface;
 	devnode = priv->plat->port_node;
 
 	if (priv->plat->pcs_init) {
 		ret = priv->plat->pcs_init(priv);
 	} else if (fwnode_property_present(devnode, "pcs-handle")) {
 		pcsnode = fwnode_find_reference(devnode, "pcs-handle", 0);
-		xpcs = xpcs_create_fwnode(pcsnode, mode);
+		xpcs = xpcs_create_fwnode(pcsnode);
 		fwnode_handle_put(pcsnode);
 		ret = PTR_ERR_OR_ZERO(xpcs);
 	} else if (priv->plat->mdio_bus_data &&
 		   priv->plat->mdio_bus_data->pcs_mask) {
 		addr = ffs(priv->plat->mdio_bus_data->pcs_mask) - 1;
-		xpcs = xpcs_create_mdiodev(priv->mii, addr, mode);
+		xpcs = xpcs_create_mdiodev(priv->mii, addr);
 		ret = PTR_ERR_OR_ZERO(xpcs);
 	} else {
 		return 0;
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 9b61f97222b9..f25e7afdfdf5 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -1520,14 +1520,12 @@ static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev)
  * xpcs_create_mdiodev() - create a DW xPCS instance with the MDIO @addr
  * @bus: pointer to the MDIO-bus descriptor for the device to be looked at
  * @addr: device MDIO-bus ID
- * @interface: requested PHY interface
  *
  * Return: a pointer to the DW XPCS handle if successful, otherwise -ENODEV if
  * the PCS device couldn't be found on the bus and other negative errno related
  * to the data allocation and MDIO-bus communications.
  */
-struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr,
-				    phy_interface_t interface)
+struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr)
 {
 	struct mdio_device *mdiodev;
 	struct dw_xpcs *xpcs;
@@ -1554,7 +1552,7 @@ struct phylink_pcs *xpcs_create_pcs_mdiodev(struct mii_bus *bus, int addr)
 {
 	struct dw_xpcs *xpcs;
 
-	xpcs = xpcs_create_mdiodev(bus, addr, PHY_INTERFACE_MODE_NA);
+	xpcs = xpcs_create_mdiodev(bus, addr);
 	if (IS_ERR(xpcs))
 		return ERR_CAST(xpcs);
 
@@ -1565,7 +1563,6 @@ EXPORT_SYMBOL_GPL(xpcs_create_pcs_mdiodev);
 /**
  * xpcs_create_fwnode() - Create a DW xPCS instance from @fwnode
  * @fwnode: fwnode handle poining to the DW XPCS device
- * @interface: requested PHY interface
  *
  * Return: a pointer to the DW XPCS handle if successful, otherwise -ENODEV if
  * the fwnode device is unavailable or the PCS device couldn't be found on the
@@ -1573,8 +1570,7 @@ EXPORT_SYMBOL_GPL(xpcs_create_pcs_mdiodev);
  * other negative errno related to the data allocations and MDIO-bus
  * communications.
  */
-struct dw_xpcs *xpcs_create_fwnode(struct fwnode_handle *fwnode,
-				   phy_interface_t interface)
+struct dw_xpcs *xpcs_create_fwnode(struct fwnode_handle *fwnode)
 {
 	struct mdio_device *mdiodev;
 	struct dw_xpcs *xpcs;
diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index a4e2243ce647..758daabb76c7 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -72,10 +72,8 @@ int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
 void xpcs_get_interfaces(struct dw_xpcs *xpcs, unsigned long *interfaces);
 int xpcs_config_eee(struct dw_xpcs *xpcs, int mult_fact_100ns,
 		    int enable);
-struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr,
-				    phy_interface_t interface);
-struct dw_xpcs *xpcs_create_fwnode(struct fwnode_handle *fwnode,
-				   phy_interface_t interface);
+struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr);
+struct dw_xpcs *xpcs_create_fwnode(struct fwnode_handle *fwnode);
 void xpcs_destroy(struct dw_xpcs *xpcs);
 
 struct phylink_pcs *xpcs_create_pcs_mdiodev(struct mii_bus *bus, int addr);
-- 
2.30.2

Re: [PATCH RFC net-next 09/10] net: pcs: xpcs: drop interface argument from xpcs_create*()

[Vladimir Oltean]

On Mon, Sep 23, 2024 at 03:01:40PM +0100, Russell King (Oracle) wrote:
> The XPCS sub-driver no longer uses the "interface" argument to the
> xpcs_create_mdiodev() and xpcs_create_fwnode() functions. Remove
> this now unnecessary argument, updating the stmmac driver
> appropriately.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

[PATCH RFC net-next 10/10] net: pcs: xpcs: make xpcs_do_config() and xpcs_link_up() internal

[Russell King (Oracle)]

As nothing outside pcs-xpcs.c calls neither xpcs_do_config() nor
xpcs_link_up(), remove their exports and prototypes.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/pcs/pcs-xpcs.c   | 11 +++++------
 include/linux/pcs/pcs-xpcs.h |  4 ----
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index f25e7afdfdf5..0a01c552f591 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -851,8 +851,9 @@ static int xpcs_config_2500basex(struct dw_xpcs *xpcs)
 	return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, ret);
 }
 
-int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
-		   const unsigned long *advertising, unsigned int neg_mode)
+static int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
+			  const unsigned long *advertising,
+			  unsigned int neg_mode)
 {
 	const struct dw_xpcs_compat *compat;
 	int ret;
@@ -905,7 +906,6 @@ int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(xpcs_do_config);
 
 static int xpcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 		       phy_interface_t interface,
@@ -1207,8 +1207,8 @@ static void xpcs_link_up_1000basex(struct dw_xpcs *xpcs, unsigned int neg_mode,
 		pr_err("%s: xpcs_write returned %pe\n", __func__, ERR_PTR(ret));
 }
 
-void xpcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
-		  phy_interface_t interface, int speed, int duplex)
+static void xpcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
+			 phy_interface_t interface, int speed, int duplex)
 {
 	struct dw_xpcs *xpcs = phylink_pcs_to_xpcs(pcs);
 
@@ -1219,7 +1219,6 @@ void xpcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
 	if (interface == PHY_INTERFACE_MODE_1000BASEX)
 		return xpcs_link_up_1000basex(xpcs, neg_mode, speed, duplex);
 }
-EXPORT_SYMBOL_GPL(xpcs_link_up);
 
 static void xpcs_an_restart(struct phylink_pcs *pcs)
 {
diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index 758daabb76c7..abda475111d1 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -65,10 +65,6 @@ struct dw_xpcs {
 };
 
 int xpcs_get_an_mode(struct dw_xpcs *xpcs, phy_interface_t interface);
-void xpcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
-		  phy_interface_t interface, int speed, int duplex);
-int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
-		   const unsigned long *advertising, unsigned int neg_mode);
 void xpcs_get_interfaces(struct dw_xpcs *xpcs, unsigned long *interfaces);
 int xpcs_config_eee(struct dw_xpcs *xpcs, int mult_fact_100ns,
 		    int enable);
-- 
2.30.2

Re: [PATCH RFC net-next 10/10] net: pcs: xpcs: make xpcs_do_config() and xpcs_link_up() internal

[Vladimir Oltean]

On Mon, Sep 23, 2024 at 03:01:45PM +0100, Russell King (Oracle) wrote:
> As nothing outside pcs-xpcs.c calls neither xpcs_do_config() nor
> xpcs_link_up(), remove their exports and prototypes.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [PATCH RFC 00/10] net: pcs: xpcs: cleanups batch 1

[Maxime Chevallier]

Hi Russell,

On Mon, 23 Sep 2024 15:00:26 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> Hi,
> 
> First, sorry for the bland series subject - this is the first in a
> number of cleanup series to the XPCS driver. This series has some
> functional changes beyond merely cleanups, notably the first patch.

FWIW I've reviewed the whole series, to the best of my knowledge this
looks fine, nice improvements. I don't have any means to test however.

Thanks,

Maxime

Re: [PATCH RFC 00/10] net: pcs: xpcs: cleanups batch 1

[Vladimir Oltean]

Hi Russell,

On Mon, Sep 23, 2024 at 03:00:26PM +0100, Russell King (Oracle) wrote:
> First, sorry for the bland series subject - this is the first in a
> number of cleanup series to the XPCS driver.

I presume you intend to remove the rest of the exported xpcs functions
as well, in further "batches". Could you share in advance some details
about what you plan to do with xpcs_get_an_mode() as used in stmmac?

	if (xpcs_get_an_mode(priv->hw->xpcs, mode) != DW_AN_C73))

I'm interested because I actually have some downstream NXP patches which
introduce an entirely new MLO_AN_C73 negotiating mode in phylink (though
they don't convert XPCS to it, sadly). Just wondering where this is going
in your view.

Re: [PATCH RFC 00/10] net: pcs: xpcs: cleanups batch 1

[Russell King (Oracle)]

On Wed, Sep 25, 2024 at 04:43:37PM +0300, Vladimir Oltean wrote:
> Hi Russell,
> 
> On Mon, Sep 23, 2024 at 03:00:26PM +0100, Russell King (Oracle) wrote:
> > First, sorry for the bland series subject - this is the first in a
> > number of cleanup series to the XPCS driver.
> 
> I presume you intend to remove the rest of the exported xpcs functions
> as well, in further "batches". Could you share in advance some details
> about what you plan to do with xpcs_get_an_mode() as used in stmmac?

I've been concentrating more on the sja1105 and wangxun users with this
cleanup, as changing stmmac is going to be quite painful - so I've left
this as something for the future. stmmac already stores a phylink_pcs
pointer, but we can't re-use that for XPCS because stmmac needs to know
that it's an XPCS vs some other PCS due to the direct calls such as
xpcs_get_an_mode() and xpcs_config_eee().

When I was working on EEE support at phylink level, I did try to figure
out what xpcs_config_eee() is all about, what it's trying to do, why,
and how it would fit into any phylink-based EEE scheme, but I never got
very far with that due to lack of documentation.

So, at the moment I have no plans to touch the prototypes of
xpcs_get_an_mode(), xpcs_config_eee() nor xpcs_get_interfaces(). With
the entire patch series being so large already, I'm in no hurry to add
patches for this - which would need yet more work on stmmac that I'm
no longer willing to do.

> 	if (xpcs_get_an_mode(priv->hw->xpcs, mode) != DW_AN_C73))
> 
> I'm interested because I actually have some downstream NXP patches which
> introduce an entirely new MLO_AN_C73 negotiating mode in phylink (though
> they don't convert XPCS to it, sadly). Just wondering where this is going
> in your view.

To give a flavour of what remains:

net: pcs: xpcs: move Wangxun VR_XS_PCS_DIG_CTRL1 configuration
net: pcs: xpcs: correctly place DW_VR_MII_DIG_CTRL1_2G5_EN
net: pcs: xpcs: use dev_*() to print messages
net: pcs: xpcs: convert to use read_poll_timeout()
net: pcs: xpcs: add _modify() accessors
net: pcs: xpcs: use FIELD_PREP() and FIELD_GET()
net: pcs: xpcs: convert to use linkmode_adv_to_c73()
net: pcs: xpcs: add xpcs_linkmode_supported()
net: mdio: add linkmode_adv_to_c73()
net: pcs: xpcs: move searching ID list out of line
net: pcs: xpcs: rename xpcs_get_id()
net: pcs: xpcs: move definition of struct dw_xpcs to private header
net: pcs: xpcs: provide a helper to get the phylink pcs given xpcs
net: pcs: xpcs: pass xpcs instead of xpcs->id to xpcs_find_compat()
net: pcs: xpcs: don't use array for interface
net: pcs: xpcs: remove dw_xpcs_compat enum

which looks like this on the diffstat:

 drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c |   2 +-
 drivers/net/pcs/pcs-xpcs-nxp.c                    |  24 +-
 drivers/net/pcs/pcs-xpcs-wx.c                     |  51 +--
 drivers/net/pcs/pcs-xpcs.c                        | 521 +++++++++-------------
 drivers/net/pcs/pcs-xpcs.h                        |  42 +-
 include/linux/mdio.h                              |  40 ++
 include/linux/pcs/pcs-xpcs.h                      |  19 +-
 7 files changed, 303 insertions(+), 396 deletions(-)

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

Re: [PATCH RFC 00/10] net: pcs: xpcs: cleanups batch 1

[Vladimir Oltean]

On Thu, Sep 26, 2024 at 12:41:27PM +0100, Russell King (Oracle) wrote:
> On Wed, Sep 25, 2024 at 04:43:37PM +0300, Vladimir Oltean wrote:
> > Hi Russell,
> > 
> > On Mon, Sep 23, 2024 at 03:00:26PM +0100, Russell King (Oracle) wrote:
> > > First, sorry for the bland series subject - this is the first in a
> > > number of cleanup series to the XPCS driver.
> > 
> > I presume you intend to remove the rest of the exported xpcs functions
> > as well, in further "batches". Could you share in advance some details
> > about what you plan to do with xpcs_get_an_mode() as used in stmmac?
> 
> I've been concentrating more on the sja1105 and wangxun users with this
> cleanup, as changing stmmac is going to be quite painful - so I've left
> this as something for the future. stmmac already stores a phylink_pcs
> pointer, but we can't re-use that for XPCS because stmmac needs to know
> that it's an XPCS vs some other PCS due to the direct calls such as
> xpcs_get_an_mode() and xpcs_config_eee().
> 
> When I was working on EEE support at phylink level, I did try to figure
> out what xpcs_config_eee() is all about, what it's trying to do, why,
> and how it would fit into any phylink-based EEE scheme, but I never got
> very far with that due to lack of documentation.
> 
> So, at the moment I have no plans to touch the prototypes of
> xpcs_get_an_mode(), xpcs_config_eee() nor xpcs_get_interfaces(). With
> the entire patch series being so large already, I'm in no hurry to add
> patches for this - which would need yet more work on stmmac that I'm
> no longer willing to do.

Ok, but I guess that the (very) long-term plan still is that direct
calls from the MAC driver into symbols exported by the PCS are no longer
going to be a thing, right?

> > 	if (xpcs_get_an_mode(priv->hw->xpcs, mode) != DW_AN_C73))
> > 
> > I'm interested because I actually have some downstream NXP patches which
> > introduce an entirely new MLO_AN_C73 negotiating mode in phylink (though
> > they don't convert XPCS to it, sadly). Just wondering where this is going
> > in your view.
> 
> To give a flavour of what remains:
> 
> net: pcs: xpcs: move Wangxun VR_XS_PCS_DIG_CTRL1 configuration
> net: pcs: xpcs: correctly place DW_VR_MII_DIG_CTRL1_2G5_EN
> net: pcs: xpcs: use dev_*() to print messages
> net: pcs: xpcs: convert to use read_poll_timeout()
> net: pcs: xpcs: add _modify() accessors
> net: pcs: xpcs: use FIELD_PREP() and FIELD_GET()
> net: pcs: xpcs: convert to use linkmode_adv_to_c73()
> net: pcs: xpcs: add xpcs_linkmode_supported()
> net: mdio: add linkmode_adv_to_c73()
> net: pcs: xpcs: move searching ID list out of line
> net: pcs: xpcs: rename xpcs_get_id()
> net: pcs: xpcs: move definition of struct dw_xpcs to private header
> net: pcs: xpcs: provide a helper to get the phylink pcs given xpcs
> net: pcs: xpcs: pass xpcs instead of xpcs->id to xpcs_find_compat()
> net: pcs: xpcs: don't use array for interface
> net: pcs: xpcs: remove dw_xpcs_compat enum
> 
> which looks like this on the diffstat:
> 
>  drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c |   2 +-
>  drivers/net/pcs/pcs-xpcs-nxp.c                    |  24 +-
>  drivers/net/pcs/pcs-xpcs-wx.c                     |  51 +--
>  drivers/net/pcs/pcs-xpcs.c                        | 521 +++++++++-------------
>  drivers/net/pcs/pcs-xpcs.h                        |  42 +-
>  include/linux/mdio.h                              |  40 ++
>  include/linux/pcs/pcs-xpcs.h                      |  19 +-
>  7 files changed, 303 insertions(+), 396 deletions(-)

Ok, I don't see anything major on the clause 73 autoneg front. Which I
guess is good? (because at least there aren't competing ideas in flight
about phylink's role for this operating mode)

The bad part is that some user-visible functional changes in xpcs will
most likely be in order. So will probably not be able to be converted
without someone with both access to the 10G hardware and the motivation
to do so (and this might make the conversion unreachable for me too).

For example, without having seen the content of your patch list,
I can only assume linkmode_adv_to_c73() is based on the ethtool
link modes that _xpcs_config_aneg_c73(), mii_c73_mod_linkmode() and
phylink_c73_priority_resolution[] treat.

But I'm already objecting that 2500baseX shouldn't be in those tables.
There should have been a new 2500base-KX ethtool mode, which is one
of the amendments to 802.3-2018 called 802.3cb-2018.

I also have other objections to XPCS's implementation of C73, but I
don't think this is the right context to point them all out. The gist
is that at least for this area, I don't think it would be a good idea
at all to base core phylink support based on what XPCS has/does.

I guess what I'm saying is that taking a break from stmmac until the
groundwork in the core has been laid out through some other vector also
seems like the best idea to me.

Would you be interested in seeing an alternative implementation of
clause 73 support (for the Lynx PCS), this time centered around phylink_pcs,
even if it doesn't touch stmmac/xpcs? As a side effect of that work, it
would maybe provide a long-term avenue of avoiding the xpcs_get_interfaces()
and xpcs_get_an_mode() direct calls, as well as a more consolidated
framework for C73 in XPCS to be reimplemented by somebody. (warning,
this implementation will be quite large, so the question is also about
your time/energy availability in the near future).