[PATCH net-next v2 0/4] net: dsa: ocelot: phylink updates

Hi,

This series updates the Ocelot DSA driver for some of the recent
phylink changes. Specifically, we fill in the supported_interfaces
fields, convert to mac_select_pcs and mark the driver as non-legacy.
We do not convert to phylink_generic_validate() as Ocelot has
special support for its rate adapting PCS which makes the generic
validate method unsuitable for this driver.

The three changes mentioned above are implemented in their own
separate patches with one additional cleanup:

1) Populate the supported_interfaces bitmap
2) Remove the now unnecessary interface checks in the validate methods
3) Convert from phylink_set_pcs() to .mac_select_pcs.
4) Mark the driver as non-legacy

Thanks.

RFC -> non-RFC: add reviewed-by/tested-by's, update patch 1 to set the
supported_interfaces bitmap in felix.c rather than the sub-drivers as 
requested by Vladimir.

non-RFC -> v2: fix build error introduced in patch 1.

 drivers/net/dsa/ocelot/felix.c           | 30 ++++++++++++++++++++++++------
 drivers/net/dsa/ocelot/felix_vsc9959.c   |  7 -------
 drivers/net/dsa/ocelot/seville_vsc9953.c |  7 -------
 3 files changed, 24 insertions(+), 20 deletions(-)

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

Populate the supported interfaces bitmap for the Ocelot DSA switches.

Since all sub-drivers only support a single interface mode, defined by
ocelot_port->phy_mode, we can handle this in the main driver code
without reference to the sub-driver.

Signed-off-by: Russell King (Oracle) <redacted>
---
 drivers/net/dsa/ocelot/felix.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 9959407fede8..a1a6ace39aab 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c

@@ -778,6 +778,15 @@ static int felix_vlan_del(struct dsa_switch *ds, int port,
 	return ocelot_vlan_del(ocelot, port, vlan->vid);
 }
 
+static void felix_phylink_get_caps(struct dsa_switch *ds, int port,
+				   struct phylink_config *config)
+{
+	struct ocelot *ocelot = ds->priv;
+
+	__set_bit(ocelot->ports[port]->phy_mode,
+		  config->supported_interfaces);
+}
+
 static void felix_phylink_validate(struct dsa_switch *ds, int port,
 				   unsigned long *supported,
 				   struct phylink_link_state *state)

@@ -1587,6 +1596,7 @@ const struct dsa_switch_ops felix_switch_ops = {
 	.get_ethtool_stats		= felix_get_ethtool_stats,
 	.get_sset_count			= felix_get_sset_count,
 	.get_ts_info			= felix_get_ts_info,
+	.phylink_get_caps		= felix_phylink_get_caps,
 	.phylink_validate		= felix_phylink_validate,
 	.phylink_mac_config		= felix_phylink_mac_config,
 	.phylink_mac_link_down		= felix_phylink_mac_link_down,

-- 
2.30.2

When the supported interfaces bitmap is populated, phylink will itself
check that the interface mode is present in this bitmap. Drivers no
longer need to perform this check themselves. Remove these checks.

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Russell King (Oracle) <redacted>
---
 drivers/net/dsa/ocelot/felix_vsc9959.c   | 7 -------
 drivers/net/dsa/ocelot/seville_vsc9953.c | 7 -------
 2 files changed, 14 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 33f0ceae381d..434c7e4f0648 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c

@@ -944,15 +944,8 @@ static void vsc9959_phylink_validate(struct ocelot *ocelot, int port,
 				     unsigned long *supported,
 				     struct phylink_link_state *state)
 {
-	struct ocelot_port *ocelot_port = ocelot->ports[port];
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
 
-	if (state->interface != PHY_INTERFACE_MODE_NA &&
-	    state->interface != ocelot_port->phy_mode) {
-		linkmode_zero(supported);
-		return;
-	}
-
 	phylink_set_port_modes(mask);
 	phylink_set(mask, Autoneg);
 	phylink_set(mask, Pause);

diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
index f2f1608a476c..f12c1a1a3d5c 100644
--- a/drivers/net/dsa/ocelot/seville_vsc9953.c
+++ b/drivers/net/dsa/ocelot/seville_vsc9953.c

@@ -917,15 +917,8 @@ static void vsc9953_phylink_validate(struct ocelot *ocelot, int port,
 				     unsigned long *supported,
 				     struct phylink_link_state *state)
 {
-	struct ocelot_port *ocelot_port = ocelot->ports[port];
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
 
-	if (state->interface != PHY_INTERFACE_MODE_NA &&
-	    state->interface != ocelot_port->phy_mode) {
-		linkmode_zero(supported);
-		return;
-	}
-
 	phylink_set_port_modes(mask);
 	phylink_set(mask, Autoneg);
 	phylink_set(mask, Pause);

-- 
2.30.2

Convert the PCS selection to use mac_select_pcs, which allows the PCS
to perform any validation it needs, and removes the need to set the PCS
in the mac_config() callback, delving into the higher DSA levels to do
so.

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Russell King (Oracle) <redacted>
---
 drivers/net/dsa/ocelot/felix.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index a1a6ace39aab..bfc756c35a2b 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c

@@ -798,16 +798,18 @@ static void felix_phylink_validate(struct dsa_switch *ds, int port,
 		felix->info->phylink_validate(ocelot, port, supported, state);
 }
 
-static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
-				     unsigned int link_an_mode,
-				     const struct phylink_link_state *state)
+static struct phylink_pcs *felix_phylink_mac_select_pcs(struct dsa_switch *ds,
+							int port,
+							phy_interface_t iface)
 {
 	struct ocelot *ocelot = ds->priv;
 	struct felix *felix = ocelot_to_felix(ocelot);
-	struct dsa_port *dp = dsa_to_port(ds, port);
+	struct phylink_pcs *pcs = NULL;
 
 	if (felix->pcs && felix->pcs[port])
-		phylink_set_pcs(dp->pl, felix->pcs[port]);
+		pcs = felix->pcs[port];
+
+	return pcs;
 }
 
 static void felix_phylink_mac_link_down(struct dsa_switch *ds, int port,

@@ -1598,7 +1600,7 @@ const struct dsa_switch_ops felix_switch_ops = {
 	.get_ts_info			= felix_get_ts_info,
 	.phylink_get_caps		= felix_phylink_get_caps,
 	.phylink_validate		= felix_phylink_validate,
-	.phylink_mac_config		= felix_phylink_mac_config,
+	.phylink_mac_select_pcs		= felix_phylink_mac_select_pcs,
 	.phylink_mac_link_down		= felix_phylink_mac_link_down,
 	.phylink_mac_link_up		= felix_phylink_mac_link_up,
 	.port_fast_age			= felix_port_fast_age,

-- 
2.30.2

The ocelot DSA driver does not make use of the speed, duplex, pause or
advertisement in its phylink_mac_config() implementation, so it can be
marked as a non-legacy driver.

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Russell King (Oracle) <redacted>
---
 drivers/net/dsa/ocelot/felix.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index bfc756c35a2b..1d7c5d7970bd 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c

@@ -783,6 +783,12 @@ static void felix_phylink_get_caps(struct dsa_switch *ds, int port,
 {
 	struct ocelot *ocelot = ds->priv;
 
+	/* This driver does not make use of the speed, duplex, pause or the
+	 * advertisement in its mac_config, so it is safe to mark this driver
+	 * as non-legacy.
+	 */
+	config->legacy_pre_march2020 = false;
+
 	__set_bit(ocelot->ports[port]->phy_mode,
 		  config->supported_interfaces);
 }

-- 
2.30.2

On Fri, Feb 25, 2022 at 04:19:25PM +0000, Russell King (Oracle) wrote:

Populate the supported interfaces bitmap for the Ocelot DSA switches.

Since all sub-drivers only support a single interface mode, defined by
ocelot_port->phy_mode, we can handle this in the main driver code
without reference to the sub-driver.

Signed-off-by: Russell King (Oracle) <redacted>
---

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

On Fri, Feb 25, 2022 at 04:25:30PM +0000, Vladimir Oltean wrote:

On Fri, Feb 25, 2022 at 04:19:25PM +0000, Russell King (Oracle) wrote:

quoted

Populate the supported interfaces bitmap for the Ocelot DSA switches.

Since all sub-drivers only support a single interface mode, defined by
ocelot_port->phy_mode, we can handle this in the main driver code
without reference to the sub-driver.

Signed-off-by: Russell King (Oracle) <redacted>
---

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

Brilliant, thanks.

This is the final driver in net-next that was making use of
phylink_set_pcs(), so once this series is merged, that function will
only be used by phylink internally. The next patch I have in the queue
is to remove that function.

Marek Behún will be very happy to see phylink_set_pcs() gone.

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

On Fri, 25 Feb 2022 16:31:52 +0000
"Russell King (Oracle)" [off-list ref] wrote:

On Fri, Feb 25, 2022 at 04:25:30PM +0000, Vladimir Oltean wrote:

quoted

On Fri, Feb 25, 2022 at 04:19:25PM +0000, Russell King (Oracle) wrote:  

quoted

Populate the supported interfaces bitmap for the Ocelot DSA switches.

Since all sub-drivers only support a single interface mode, defined by
ocelot_port->phy_mode, we can handle this in the main driver code
without reference to the sub-driver.

Signed-off-by: Russell King (Oracle) <redacted>
---  

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

Brilliant, thanks.

This is the final driver in net-next that was making use of
phylink_set_pcs(), so once this series is merged, that function will
only be used by phylink internally. The next patch I have in the queue
is to remove that function.

Marek Behún will be very happy to see phylink_set_pcs() gone.

Yes, finally we can convert mv88e6xxx fully :)

On Fri, Feb 25, 2022 at 06:16:53PM +0100, Marek Behún wrote:

On Fri, 25 Feb 2022 16:31:52 +0000
"Russell King (Oracle)" [off-list ref] wrote:

quoted

On Fri, Feb 25, 2022 at 04:25:30PM +0000, Vladimir Oltean wrote:

quoted

On Fri, Feb 25, 2022 at 04:19:25PM +0000, Russell King (Oracle) wrote:  

quoted

Populate the supported interfaces bitmap for the Ocelot DSA switches.

Since all sub-drivers only support a single interface mode, defined by
ocelot_port->phy_mode, we can handle this in the main driver code
without reference to the sub-driver.

Signed-off-by: Russell King (Oracle) <redacted>
---  

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

Brilliant, thanks.

This is the final driver in net-next that was making use of
phylink_set_pcs(), so once this series is merged, that function will
only be used by phylink internally. The next patch I have in the queue
is to remove that function.

Marek Behún will be very happy to see phylink_set_pcs() gone.

Yes, finally we can convert mv88e6xxx fully :)

... changing the subject line to show we've drifted off topic ...

Yes, once we've worked out what the PCS interface should look like in
order to deal with the 88E6393 errata workaround that needs to be run
each time the interface changes or whenever we "power up" the PCS.

I think that needs to be discussed, because I can see no clean and
clear solution that doesn't have some kind of down-side.

The existing pcs_enable/pcs_disable I have in my tree fits well with
the idea of changing a PCS, but not for being called every time we do
a major config when the PCS isn't being changed. To see what I mean,
would someone who didn't have the 6393 issue be happy with this
sequence on every major configuration, even when the PCS isn't being
changed:

	mac_prepare()
	pcs_disable()
	mac_config()
	pcs_enable()
	pcs_config()
	pcs_an_restart()
	mac_finish()

I thought about calling pcs_disable() pcs_prepare() but that then
throws out pcs_enable(), unless we do that after pcs_config() - but
what if pcs_enable() is used to clear the PDOWN bit of the PCS, or
other (possibly external) PCS power control that prevents its registers
being accessed.

I'm also thinking, having had another recent look at
mv88e6xxx_mac_config(), we would need to move this:

        if (mode != MLO_AN_PHY || !mv88e6xxx_phy_is_internal(ds, port)) {
                /* In inband mode, the link may come up at any time while the
                 * link is not forced down. Force the link down while we
                 * reconfigure the interface mode.
                 */
                if (mode == MLO_AN_INBAND &&
                    p->interface != state->interface &&
                    chip->info->ops->port_set_link)
                        chip->info->ops->port_set_link(chip, port,
                                                       LINK_FORCED_DOWN);

into a mac_prepare() callback so it still happens prior to the PCS
being called - which will need to happen between mac_prepare() and
mac_config() for the errata. That means extending the mac_prepare()
and mac_finish() methods into DSA as well.

I do have a patch to add those additional callbacks to DSA, but I
currently have no code that makes use of them (so haven't sent it
yet.) See "net: dsa: add support for new phylink calls".

I think I would prefer at this point - to get the mt7530 changes
settled, which will then allow the phylink_helper_basex_speed()
helper to be removed, do a few further phylink/sfp code cleanups
(using %pe consistently in that code to print errors) and then wait
until the next kernel cycle before tackling mv88e6xxx.

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

... changing the subject line to show we've drifted off topic ...

Yes, once we've worked out what the PCS interface should look like in
order to deal with the 88E6393 errata workaround that needs to be run
each time the interface changes or whenever we "power up" the PCS.

Hi Russell

The erratas are not limited to 6393. For the 6390 there is an errata
where you need to "power up" the PCS before you change cmode,
otherwise TX works, but RX just drops frames rather than pass them to
the MAC.

I've not looked at the details of your proposal, and maybe it is a
none issue, i just wanted to make sure you are aware of this.

   Andrew

On Fri, Feb 25, 2022 at 06:57:33PM +0100, Andrew Lunn wrote:

quoted

... changing the subject line to show we've drifted off topic ...

Yes, once we've worked out what the PCS interface should look like in
order to deal with the 88E6393 errata workaround that needs to be run
each time the interface changes or whenever we "power up" the PCS.

Hi Russell

The erratas are not limited to 6393. For the 6390 there is an errata
where you need to "power up" the PCS before you change cmode,
otherwise TX works, but RX just drops frames rather than pass them to
the MAC.

I've not looked at the details of your proposal, and maybe it is a
none issue, i just wanted to make sure you are aware of this.

Thanks - Marek has been keeping me on the straight and narrow with
these issues, but it's good to know that it's not necessary for the
other Marvell variants. That's been at the back of my mind a bit as
we're getting closer to the point that we need to sort this out.

I should also mention - once the mt7530 and mv88e6xxx drivers are
sorted, I believe we will then be in a position to kill off all the
"legacy_pre_march2020" stuff in DSA since DSA will no longer need the
legacy phylink behaviour.

Sadly, though, that doesn't mean "legacy_pre_march2020" can be removed,
we still have mtk_eth_soc reliant on the old behaviour.

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

Hello:

This series was applied to netdev/net-next.git (master)
by David S. Miller [off-list ref]:

On Fri, 25 Feb 2022 16:19:10 +0000 you wrote:

Hi,

This series updates the Ocelot DSA driver for some of the recent
phylink changes. Specifically, we fill in the supported_interfaces
fields, convert to mac_select_pcs and mark the driver as non-legacy.
We do not convert to phylink_generic_validate() as Ocelot has
special support for its rate adapting PCS which makes the generic
validate method unsuitable for this driver.

[...]

Here is the summary with links:
  - [net-next,1/4] net: dsa: ocelot: populate supported_interfaces
    https://git.kernel.org/netdev/net-next/c/79fda660bdbb
  - [net-next,2/4] net: dsa: ocelot: remove interface checks
    https://git.kernel.org/netdev/net-next/c/e57a15401e82
  - [net-next,3/4] net: dsa: ocelot: convert to mac_select_pcs()
    https://git.kernel.org/netdev/net-next/c/864ba485ac52
  - [net-next,4/4] net: dsa: ocelot: mark as non-legacy
    https://git.kernel.org/netdev/net-next/c/f6f04c02047c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html