[PATCH net-next 0/3] net: dsa: remove validate method

[PATCH net-next 0/3] net: dsa: remove validate method

[Russell King (Oracle)]

Hi,

These three patches remove DSA's phylink .validate method which becomes
unnecessary once the last two drivers provide phylink capabilities,
which this patch set adds. Both of these are best guesses.

 drivers/net/dsa/dsa_loop.c             |  9 +++++++++
 drivers/net/dsa/vitesse-vsc73xx-core.c | 26 ++++++++++++++++++++++++++
 net/dsa/port.c                         | 15 ---------------
 3 files changed, 35 insertions(+), 15 deletions(-)

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

[PATCH net-next 1/3] net: dsa: vsc73xx: add phylink capabilities

[Russell King (Oracle)]

Add phylink capabilities for vsc73xx. Although this switch driver does
populates the .adjust_link method, dsa_slave_phy_setup() will still be
used to create phylink instances for the LAN ports, although phylink
won't be used for shared links.

There are two different classes of switch - 5+1 and 8 port. The 5+1
port switches uses port indicies 0-4 for the user interfaces and 6 for
the CPU port. The 8 port is confusing - some comments in the driver
imply that port index 7 is used, but the driver actually still uses 6,
so that is what we go with. Also, there appear to be no DTs in the
kernel tree that are using the 8 port variety.

It also looks like port 5 is always skipped.

The switch supports 10M, 100M and 1G speeds. It is not clear whether
all these speeds are supported on the CPU interface. It also looks like
symmetric pause is supported, whether asymmetric pause is as well is
unclear. However, it looks like the pause configuration is entirely
static, and doesn't depend on negotiation results.

So, let's do the best effort we can based on the information found in
the driver when creating vsc73xx_phylink_get_caps().

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/vitesse-vsc73xx-core.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 4f09e7438f3b..35a846d7e13f 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -1037,6 +1037,31 @@ static int vsc73xx_get_max_mtu(struct dsa_switch *ds, int port)
 	return 9600 - ETH_HLEN - ETH_FCS_LEN;
 }
 
+static void vsc73xx_phylink_get_caps(struct dsa_switch *dsa, int port,
+				     struct phylink_config *config)
+{
+	unsigned long *interfaces = config->supported_interfaces;
+
+	if (port == 5)
+		return;
+
+	if (port == CPU_PORT) {
+		__set_bit(PHY_INTERFACE_MODE_MII, interfaces);
+		__set_bit(PHY_INTERFACE_MODE_REVMII, interfaces);
+		__set_bit(PHY_INTERFACE_MODE_GMII, interfaces);
+		__set_bit(PHY_INTERFACE_MODE_RGMII, interfaces);
+	}
+
+	if (port <= 4) {
+		/* Internal PHYs */
+		__set_bit(PHY_INTERFACE_MODE_INTERNAL, interfaces);
+		/* phylib default */
+		__set_bit(PHY_INTERFACE_MODE_GMII, interfaces);
+	}
+
+	config->mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000;
+}
+
 static const struct dsa_switch_ops vsc73xx_ds_ops = {
 	.get_tag_protocol = vsc73xx_get_tag_protocol,
 	.setup = vsc73xx_setup,
@@ -1050,6 +1075,7 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
 	.port_disable = vsc73xx_port_disable,
 	.port_change_mtu = vsc73xx_change_mtu,
 	.port_max_mtu = vsc73xx_get_max_mtu,
+	.phylink_get_caps = vsc73xx_phylink_get_caps,
 };
 
 static int vsc73xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
-- 
2.30.2

Re: [PATCH net-next 1/3] net: dsa: vsc73xx: add phylink capabilities

[Linus Walleij]

On Mon, Oct 9, 2023 at 12:39 PM Russell King (Oracle)
<rmk+kernel@armlinux.org.uk> wrote:

> There are two different classes of switch - 5+1 and 8 port. The 5+1
> port switches uses port indicies 0-4 for the user interfaces and 6 for
> the CPU port. The 8 port is confusing - some comments in the driver
> imply that port index 7 is used, but the driver actually still uses 6,
> so that is what we go with. Also, there appear to be no DTs in the
> kernel tree that are using the 8 port variety.

This has never been tested, I think the 8 port variant is mostly
used in stand-alone configurations without a CPU, the first user
of a Linux setup will have to deal with it if the need arise.

The patch looks good to me!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH net-next 1/3] net: dsa: vsc73xx: add phylink capabilities

[Florian Fainelli]

On 10/9/23 03:39, Russell King (Oracle) wrote:
> Add phylink capabilities for vsc73xx. Although this switch driver does
> populates the .adjust_link method, dsa_slave_phy_setup() will still be
> used to create phylink instances for the LAN ports, although phylink
> won't be used for shared links.
> 
> There are two different classes of switch - 5+1 and 8 port. The 5+1
> port switches uses port indicies 0-4 for the user interfaces and 6 for
> the CPU port. The 8 port is confusing - some comments in the driver
> imply that port index 7 is used, but the driver actually still uses 6,
> so that is what we go with. Also, there appear to be no DTs in the
> kernel tree that are using the 8 port variety.
> 
> It also looks like port 5 is always skipped.
> 
> The switch supports 10M, 100M and 1G speeds. It is not clear whether
> all these speeds are supported on the CPU interface. It also looks like
> symmetric pause is supported, whether asymmetric pause is as well is
> unclear. However, it looks like the pause configuration is entirely
> static, and doesn't depend on negotiation results.
> 
> So, let's do the best effort we can based on the information found in
> the driver when creating vsc73xx_phylink_get_caps().
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

[PATCH net-next 2/3] net: dsa: dsa_loop: add phylink capabilities

[Russell King (Oracle)]

Add phylink capabilities for dsa_loop, which I believe being a software
construct means that it supports essentially all interface types and
all speeds.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/dsa_loop.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/dsa/dsa_loop.c b/drivers/net/dsa/dsa_loop.c
index 5b139f2206b6..c70ed67cc188 100644
--- a/drivers/net/dsa/dsa_loop.c
+++ b/drivers/net/dsa/dsa_loop.c
@@ -277,6 +277,14 @@ static int dsa_loop_port_max_mtu(struct dsa_switch *ds, int port)
 	return ETH_MAX_MTU;
 }
 
+static void dsa_loop_phylink_get_caps(struct dsa_switch *dsa, int port,
+				      struct phylink_config *config)
+{
+	bitmap_fill(config->supported_interfaces, PHY_INTERFACE_MODE_MAX);
+	__clear_bit(PHY_INTERFACE_MODE_NA, config->supported_interfaces);
+	config->mac_capabilities = ~0;
+}
+
 static const struct dsa_switch_ops dsa_loop_driver = {
 	.get_tag_protocol	= dsa_loop_get_protocol,
 	.setup			= dsa_loop_setup,
@@ -295,6 +303,7 @@ static const struct dsa_switch_ops dsa_loop_driver = {
 	.port_vlan_del		= dsa_loop_port_vlan_del,
 	.port_change_mtu	= dsa_loop_port_change_mtu,
 	.port_max_mtu		= dsa_loop_port_max_mtu,
+	.phylink_get_caps	= dsa_loop_phylink_get_caps,
 };
 
 static int dsa_loop_drv_probe(struct mdio_device *mdiodev)
-- 
2.30.2

Re: [PATCH net-next 2/3] net: dsa: dsa_loop: add phylink capabilities

[Florian Fainelli]

On 10/9/23 03:39, Russell King (Oracle) wrote:
> Add phylink capabilities for dsa_loop, which I believe being a software
> construct means that it supports essentially all interface types and
> all speeds.

That is right!

> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

[PATCH net-next 3/3] net: dsa: remove dsa_port_phylink_validate()

[Russell King (Oracle)]

As all drivers now provide phylink capabilities (including MAC), the
if() condition in dsa_port_phylink_validate() will always be true. We
will always use the generic validator, which phylink will call itself
if the .validate method isn't populated. Thus, there is now no need to
implement the .validate method, so this implementation can be removed.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 net/dsa/port.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/net/dsa/port.c b/net/dsa/port.c
index 5f01bd4f9dec..6e0d000a97c4 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -1554,20 +1554,6 @@ static struct phy_device *dsa_port_get_phy_device(struct dsa_port *dp)
 	return phydev;
 }
 
-static void dsa_port_phylink_validate(struct phylink_config *config,
-				      unsigned long *supported,
-				      struct phylink_link_state *state)
-{
-	/* Skip call for drivers which don't yet set mac_capabilities,
-	 * since validating in that case would mean their PHY will advertise
-	 * nothing. In turn, skipping validation makes them advertise
-	 * everything that the PHY supports, so those drivers should be
-	 * converted ASAP.
-	 */
-	if (config->mac_capabilities)
-		phylink_generic_validate(config, supported, state);
-}
-
 static struct phylink_pcs *
 dsa_port_phylink_mac_select_pcs(struct phylink_config *config,
 				phy_interface_t interface)
@@ -1666,7 +1652,6 @@ static void dsa_port_phylink_mac_link_up(struct phylink_config *config,
 }
 
 static const struct phylink_mac_ops dsa_port_phylink_mac_ops = {
-	.validate = dsa_port_phylink_validate,
 	.mac_select_pcs = dsa_port_phylink_mac_select_pcs,
 	.mac_prepare = dsa_port_phylink_mac_prepare,
 	.mac_config = dsa_port_phylink_mac_config,
-- 
2.30.2

Re: [PATCH net-next 3/3] net: dsa: remove dsa_port_phylink_validate()

[Florian Fainelli]

On 10/9/23 03:39, Russell King (Oracle) wrote:
> As all drivers now provide phylink capabilities (including MAC), the
> if() condition in dsa_port_phylink_validate() will always be true. We
> will always use the generic validator, which phylink will call itself
> if the .validate method isn't populated. Thus, there is now no need to
> implement the .validate method, so this implementation can be removed.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH net-next 0/3] net: dsa: remove validate method

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Mon, 9 Oct 2023 10:56:39 +0100 you wrote:
> Hi,
> 
> These three patches remove DSA's phylink .validate method which becomes
> unnecessary once the last two drivers provide phylink capabilities,
> which this patch set adds. Both of these are best guesses.
> 
>  drivers/net/dsa/dsa_loop.c             |  9 +++++++++
>  drivers/net/dsa/vitesse-vsc73xx-core.c | 26 ++++++++++++++++++++++++++
>  net/dsa/port.c                         | 15 ---------------
>  3 files changed, 35 insertions(+), 15 deletions(-)

Here is the summary with links:
  - [net-next,1/3] net: dsa: vsc73xx: add phylink capabilities
    https://git.kernel.org/netdev/net-next/c/a026809c261b
  - [net-next,2/3] net: dsa: dsa_loop: add phylink capabilities
    https://git.kernel.org/netdev/net-next/c/db2c6d5fc4bd
  - [net-next,3/3] net: dsa: remove dsa_port_phylink_validate()
    https://git.kernel.org/netdev/net-next/c/63b9f7a19ff1

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