[PATCH RFC net-next 00/12] Allow DSA drivers to set all phylink capabilities

[PATCH RFC net-next 00/12] Allow DSA drivers to set all phylink capabilities

[Russell King (Oracle)]

Hi,

During the last cycle, I introduced a phylink_get_interfaces() method
for DSA drivers to be able to fill out the supported_interfaces member
of phylink_config. However, further phylink development allowing the
validation hook to be greatly simplified became possible when a bitmask
of MAC capabilities is used along with the supported_interfaces bitmap.

In order to allow DSA drivers to fill out both fields, we either need
to add another method, or change the existing method. As there are no
users of the phylink_get_interfaces() yet, let's take the latter
approach, and pass the phylink_config structure to the DSA driver, so
that it can set both fields. (patch 3)

We also add the capability for DSA drivers to transition to using the
phylink_generic_validate() functionality by filling out the phylink
mac_capabilities field, and removing their .phylink_validate method.
(patch 2)

This series also contains an initial patch that consolidates the logic
in DSA around the call to phylink_create(), meaning that there becomes
a single site which issues the new call, rather than two. (patch 1)

The overall effect will be that, once this series has been applied, it
becomes possible to start eliminating the phylink validation
implementations scattered throughout the DSA drivers. Patches to do
this will follow once this series is merged.

I am including nine DSA drivers that were relatively simple to convert
in this series. The more complex ones will follow later. Please note
that none of these DSA drivers have been tested beyond a build-test,
so should be checked by the DSA switch driver maintainers.

 include/net/dsa.h  |  4 ++--
 net/dsa/dsa_priv.h |  2 +-
 net/dsa/port.c     | 48 +++++++++++++++++++++++++++++++-----------------
 net/dsa/slave.c    | 19 +++----------------
 4 files changed, 37 insertions(+), 36 deletions(-)

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

[PATCH RFC net-next 01/12] net: dsa: consolidate phylink creation

[Russell King (Oracle)]

The code in port.c and slave.c creating the phylink instance is very
similar - let's consolidate this into a single function.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 net/dsa/dsa_priv.h |  2 +-
 net/dsa/port.c     | 44 ++++++++++++++++++++++++++++----------------
 net/dsa/slave.c    | 19 +++----------------
 3 files changed, 32 insertions(+), 33 deletions(-)

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index a5c9bc7b66c6..3fb2c37c9b88 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -258,13 +258,13 @@ int dsa_port_mrp_add_ring_role(const struct dsa_port *dp,
 			       const struct switchdev_obj_ring_role_mrp *mrp);
 int dsa_port_mrp_del_ring_role(const struct dsa_port *dp,
 			       const struct switchdev_obj_ring_role_mrp *mrp);
+int dsa_port_phylink_create(struct dsa_port *dp);
 int dsa_port_link_register_of(struct dsa_port *dp);
 void dsa_port_link_unregister_of(struct dsa_port *dp);
 int dsa_port_hsr_join(struct dsa_port *dp, struct net_device *hsr);
 void dsa_port_hsr_leave(struct dsa_port *dp, struct net_device *hsr);
 int dsa_port_tag_8021q_vlan_add(struct dsa_port *dp, u16 vid, bool broadcast);
 void dsa_port_tag_8021q_vlan_del(struct dsa_port *dp, u16 vid, bool broadcast);
-extern const struct phylink_mac_ops dsa_port_phylink_mac_ops;
 
 static inline bool dsa_port_offloads_bridge_port(struct dsa_port *dp,
 						 const struct net_device *dev)
diff --git a/net/dsa/port.c b/net/dsa/port.c
index f6f12ad2b525..eaa66114924b 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -1072,7 +1072,7 @@ static void dsa_port_phylink_mac_link_up(struct phylink_config *config,
 				     speed, duplex, tx_pause, rx_pause);
 }
 
-const struct phylink_mac_ops dsa_port_phylink_mac_ops = {
+static const struct phylink_mac_ops dsa_port_phylink_mac_ops = {
 	.validate = dsa_port_phylink_validate,
 	.mac_pcs_get_state = dsa_port_phylink_mac_pcs_get_state,
 	.mac_config = dsa_port_phylink_mac_config,
@@ -1081,6 +1081,30 @@ const struct phylink_mac_ops dsa_port_phylink_mac_ops = {
 	.mac_link_up = dsa_port_phylink_mac_link_up,
 };
 
+int dsa_port_phylink_create(struct dsa_port *dp)
+{
+	struct dsa_switch *ds = dp->ds;
+	phy_interface_t mode;
+	int err;
+
+	err = of_get_phy_mode(dp->dn, &mode);
+	if (err)
+		mode = PHY_INTERFACE_MODE_NA;
+
+	if (ds->ops->phylink_get_interfaces)
+		ds->ops->phylink_get_interfaces(ds, dp->index,
+					dp->pl_config.supported_interfaces);
+
+	dp->pl = phylink_create(&dp->pl_config, of_fwnode_handle(dp->dn),
+				mode, &dsa_port_phylink_mac_ops);
+	if (IS_ERR(dp->pl)) {
+		pr_err("error creating PHYLINK: %ld\n", PTR_ERR(dp->pl));
+		return PTR_ERR(dp->pl);
+	}
+
+	return 0;
+}
+
 static int dsa_port_setup_phy_of(struct dsa_port *dp, bool enable)
 {
 	struct dsa_switch *ds = dp->ds;
@@ -1157,27 +1181,15 @@ static int dsa_port_phylink_register(struct dsa_port *dp)
 {
 	struct dsa_switch *ds = dp->ds;
 	struct device_node *port_dn = dp->dn;
-	phy_interface_t mode;
 	int err;
 
-	err = of_get_phy_mode(port_dn, &mode);
-	if (err)
-		mode = PHY_INTERFACE_MODE_NA;
-
 	dp->pl_config.dev = ds->dev;
 	dp->pl_config.type = PHYLINK_DEV;
 	dp->pl_config.pcs_poll = ds->pcs_poll;
 
-	if (ds->ops->phylink_get_interfaces)
-		ds->ops->phylink_get_interfaces(ds, dp->index,
-					dp->pl_config.supported_interfaces);
-
-	dp->pl = phylink_create(&dp->pl_config, of_fwnode_handle(port_dn),
-				mode, &dsa_port_phylink_mac_ops);
-	if (IS_ERR(dp->pl)) {
-		pr_err("error creating PHYLINK: %ld\n", PTR_ERR(dp->pl));
-		return PTR_ERR(dp->pl);
-	}
+	err = dsa_port_phylink_create(dp);
+	if (err)
+		return err;
 
 	err = phylink_of_phy_connect(dp->pl, port_dn, 0);
 	if (err && err != -ENODEV) {
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index ad61f6bc8886..33b54eadc641 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1851,14 +1851,9 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)
 	struct dsa_port *dp = dsa_slave_to_port(slave_dev);
 	struct device_node *port_dn = dp->dn;
 	struct dsa_switch *ds = dp->ds;
-	phy_interface_t mode;
 	u32 phy_flags = 0;
 	int ret;
 
-	ret = of_get_phy_mode(port_dn, &mode);
-	if (ret)
-		mode = PHY_INTERFACE_MODE_NA;
-
 	dp->pl_config.dev = &slave_dev->dev;
 	dp->pl_config.type = PHYLINK_NETDEV;
 
@@ -1871,17 +1866,9 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)
 		dp->pl_config.poll_fixed_state = true;
 	}
 
-	if (ds->ops->phylink_get_interfaces)
-		ds->ops->phylink_get_interfaces(ds, dp->index,
-					dp->pl_config.supported_interfaces);
-
-	dp->pl = phylink_create(&dp->pl_config, of_fwnode_handle(port_dn), mode,
-				&dsa_port_phylink_mac_ops);
-	if (IS_ERR(dp->pl)) {
-		netdev_err(slave_dev,
-			   "error creating PHYLINK: %ld\n", PTR_ERR(dp->pl));
-		return PTR_ERR(dp->pl);
-	}
+	ret = dsa_port_phylink_create(dp);
+	if (ret)
+		return ret;
 
 	if (ds->ops->get_phy_flags)
 		phy_flags = ds->ops->get_phy_flags(ds, dp->index);
-- 
2.30.2

Re: [PATCH RFC net-next 01/12] net: dsa: consolidate phylink creation

[Vladimir Oltean]

On Wed, Nov 24, 2021 at 05:52:28PM +0000, Russell King (Oracle) wrote:
> The code in port.c and slave.c creating the phylink instance is very
> similar - let's consolidate this into a single function.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  net/dsa/dsa_priv.h |  2 +-
>  net/dsa/port.c     | 44 ++++++++++++++++++++++++++++----------------
>  net/dsa/slave.c    | 19 +++----------------
>  3 files changed, 32 insertions(+), 33 deletions(-)
> 
> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
> index a5c9bc7b66c6..3fb2c37c9b88 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -258,13 +258,13 @@ int dsa_port_mrp_add_ring_role(const struct dsa_port *dp,
>  			       const struct switchdev_obj_ring_role_mrp *mrp);
>  int dsa_port_mrp_del_ring_role(const struct dsa_port *dp,
>  			       const struct switchdev_obj_ring_role_mrp *mrp);
> +int dsa_port_phylink_create(struct dsa_port *dp);
>  int dsa_port_link_register_of(struct dsa_port *dp);
>  void dsa_port_link_unregister_of(struct dsa_port *dp);
>  int dsa_port_hsr_join(struct dsa_port *dp, struct net_device *hsr);
>  void dsa_port_hsr_leave(struct dsa_port *dp, struct net_device *hsr);
>  int dsa_port_tag_8021q_vlan_add(struct dsa_port *dp, u16 vid, bool broadcast);
>  void dsa_port_tag_8021q_vlan_del(struct dsa_port *dp, u16 vid, bool broadcast);
> -extern const struct phylink_mac_ops dsa_port_phylink_mac_ops;
>  
>  static inline bool dsa_port_offloads_bridge_port(struct dsa_port *dp,
>  						 const struct net_device *dev)
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index f6f12ad2b525..eaa66114924b 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -1072,7 +1072,7 @@ static void dsa_port_phylink_mac_link_up(struct phylink_config *config,
>  				     speed, duplex, tx_pause, rx_pause);
>  }
>  
> -const struct phylink_mac_ops dsa_port_phylink_mac_ops = {
> +static const struct phylink_mac_ops dsa_port_phylink_mac_ops = {
>  	.validate = dsa_port_phylink_validate,
>  	.mac_pcs_get_state = dsa_port_phylink_mac_pcs_get_state,
>  	.mac_config = dsa_port_phylink_mac_config,
> @@ -1081,6 +1081,30 @@ const struct phylink_mac_ops dsa_port_phylink_mac_ops = {
>  	.mac_link_up = dsa_port_phylink_mac_link_up,
>  };
>  
> +int dsa_port_phylink_create(struct dsa_port *dp)
> +{
> +	struct dsa_switch *ds = dp->ds;
> +	phy_interface_t mode;
> +	int err;
> +
> +	err = of_get_phy_mode(dp->dn, &mode);
> +	if (err)
> +		mode = PHY_INTERFACE_MODE_NA;
> +
> +	if (ds->ops->phylink_get_interfaces)
> +		ds->ops->phylink_get_interfaces(ds, dp->index,
> +					dp->pl_config.supported_interfaces);

Can you please save dp->pl_config to a struct phylink_config *config
temporary variable, and pass that here and to phylink_create() while
preserving the alignment of that argument to the open brace? Thanks.

> +
> +	dp->pl = phylink_create(&dp->pl_config, of_fwnode_handle(dp->dn),
> +				mode, &dsa_port_phylink_mac_ops);
> +	if (IS_ERR(dp->pl)) {
> +		pr_err("error creating PHYLINK: %ld\n", PTR_ERR(dp->pl));
> +		return PTR_ERR(dp->pl);
> +	}
> +
> +	return 0;
> +}
> +
>  static int dsa_port_setup_phy_of(struct dsa_port *dp, bool enable)
>  {
>  	struct dsa_switch *ds = dp->ds;
> @@ -1157,27 +1181,15 @@ static int dsa_port_phylink_register(struct dsa_port *dp)
>  {
>  	struct dsa_switch *ds = dp->ds;
>  	struct device_node *port_dn = dp->dn;
> -	phy_interface_t mode;
>  	int err;
>  
> -	err = of_get_phy_mode(port_dn, &mode);
> -	if (err)
> -		mode = PHY_INTERFACE_MODE_NA;
> -
>  	dp->pl_config.dev = ds->dev;
>  	dp->pl_config.type = PHYLINK_DEV;
>  	dp->pl_config.pcs_poll = ds->pcs_poll;
>  
> -	if (ds->ops->phylink_get_interfaces)
> -		ds->ops->phylink_get_interfaces(ds, dp->index,
> -					dp->pl_config.supported_interfaces);
> -
> -	dp->pl = phylink_create(&dp->pl_config, of_fwnode_handle(port_dn),
> -				mode, &dsa_port_phylink_mac_ops);
> -	if (IS_ERR(dp->pl)) {
> -		pr_err("error creating PHYLINK: %ld\n", PTR_ERR(dp->pl));
> -		return PTR_ERR(dp->pl);
> -	}
> +	err = dsa_port_phylink_create(dp);
> +	if (err)
> +		return err;
>  
>  	err = phylink_of_phy_connect(dp->pl, port_dn, 0);
>  	if (err && err != -ENODEV) {
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index ad61f6bc8886..33b54eadc641 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -1851,14 +1851,9 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)
>  	struct dsa_port *dp = dsa_slave_to_port(slave_dev);
>  	struct device_node *port_dn = dp->dn;
>  	struct dsa_switch *ds = dp->ds;
> -	phy_interface_t mode;
>  	u32 phy_flags = 0;
>  	int ret;
>  
> -	ret = of_get_phy_mode(port_dn, &mode);
> -	if (ret)
> -		mode = PHY_INTERFACE_MODE_NA;
> -
>  	dp->pl_config.dev = &slave_dev->dev;
>  	dp->pl_config.type = PHYLINK_NETDEV;
>  
> @@ -1871,17 +1866,9 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)
>  		dp->pl_config.poll_fixed_state = true;
>  	}
>  
> -	if (ds->ops->phylink_get_interfaces)
> -		ds->ops->phylink_get_interfaces(ds, dp->index,
> -					dp->pl_config.supported_interfaces);
> -
> -	dp->pl = phylink_create(&dp->pl_config, of_fwnode_handle(port_dn), mode,
> -				&dsa_port_phylink_mac_ops);
> -	if (IS_ERR(dp->pl)) {
> -		netdev_err(slave_dev,
> -			   "error creating PHYLINK: %ld\n", PTR_ERR(dp->pl));
> -		return PTR_ERR(dp->pl);
> -	}
> +	ret = dsa_port_phylink_create(dp);
> +	if (ret)
> +		return ret;
>  
>  	if (ds->ops->get_phy_flags)
>  		phy_flags = ds->ops->get_phy_flags(ds, dp->index);
> -- 
> 2.30.2
>

Re: [PATCH RFC net-next 01/12] net: dsa: consolidate phylink creation

[Russell King (Oracle)]

On Wed, Nov 24, 2021 at 06:11:57PM +0000, Vladimir Oltean wrote:
> On Wed, Nov 24, 2021 at 05:52:28PM +0000, Russell King (Oracle) wrote:
> > The code in port.c and slave.c creating the phylink instance is very
> > similar - let's consolidate this into a single function.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  net/dsa/dsa_priv.h |  2 +-
> >  net/dsa/port.c     | 44 ++++++++++++++++++++++++++++----------------
> >  net/dsa/slave.c    | 19 +++----------------
> >  3 files changed, 32 insertions(+), 33 deletions(-)
> > 
> > diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
> > index a5c9bc7b66c6..3fb2c37c9b88 100644
> > --- a/net/dsa/dsa_priv.h
> > +++ b/net/dsa/dsa_priv.h
> > @@ -258,13 +258,13 @@ int dsa_port_mrp_add_ring_role(const struct dsa_port *dp,
> >  			       const struct switchdev_obj_ring_role_mrp *mrp);
> >  int dsa_port_mrp_del_ring_role(const struct dsa_port *dp,
> >  			       const struct switchdev_obj_ring_role_mrp *mrp);
> > +int dsa_port_phylink_create(struct dsa_port *dp);
> >  int dsa_port_link_register_of(struct dsa_port *dp);
> >  void dsa_port_link_unregister_of(struct dsa_port *dp);
> >  int dsa_port_hsr_join(struct dsa_port *dp, struct net_device *hsr);
> >  void dsa_port_hsr_leave(struct dsa_port *dp, struct net_device *hsr);
> >  int dsa_port_tag_8021q_vlan_add(struct dsa_port *dp, u16 vid, bool broadcast);
> >  void dsa_port_tag_8021q_vlan_del(struct dsa_port *dp, u16 vid, bool broadcast);
> > -extern const struct phylink_mac_ops dsa_port_phylink_mac_ops;
> >  
> >  static inline bool dsa_port_offloads_bridge_port(struct dsa_port *dp,
> >  						 const struct net_device *dev)
> > diff --git a/net/dsa/port.c b/net/dsa/port.c
> > index f6f12ad2b525..eaa66114924b 100644
> > --- a/net/dsa/port.c
> > +++ b/net/dsa/port.c
> > @@ -1072,7 +1072,7 @@ static void dsa_port_phylink_mac_link_up(struct phylink_config *config,
> >  				     speed, duplex, tx_pause, rx_pause);
> >  }
> >  
> > -const struct phylink_mac_ops dsa_port_phylink_mac_ops = {
> > +static const struct phylink_mac_ops dsa_port_phylink_mac_ops = {
> >  	.validate = dsa_port_phylink_validate,
> >  	.mac_pcs_get_state = dsa_port_phylink_mac_pcs_get_state,
> >  	.mac_config = dsa_port_phylink_mac_config,
> > @@ -1081,6 +1081,30 @@ const struct phylink_mac_ops dsa_port_phylink_mac_ops = {
> >  	.mac_link_up = dsa_port_phylink_mac_link_up,
> >  };
> >  
> > +int dsa_port_phylink_create(struct dsa_port *dp)
> > +{
> > +	struct dsa_switch *ds = dp->ds;
> > +	phy_interface_t mode;
> > +	int err;
> > +
> > +	err = of_get_phy_mode(dp->dn, &mode);
> > +	if (err)
> > +		mode = PHY_INTERFACE_MODE_NA;
> > +
> > +	if (ds->ops->phylink_get_interfaces)
> > +		ds->ops->phylink_get_interfaces(ds, dp->index,
> > +					dp->pl_config.supported_interfaces);
> 
> Can you please save dp->pl_config to a struct phylink_config *config
> temporary variable, and pass that here and to phylink_create() while
> preserving the alignment of that argument to the open brace? Thanks.

There is no point; first, this is how the original code was formatted
that is moved here, and second, this code is deleted in patch 3.
Making it a local variable, and then deleting it in patch 3 is pointless
churn.

Thanks.

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

[PATCH RFC net-next 02/12] net: dsa: support use of phylink_generic_validate()

[Russell King (Oracle)]

Support the use of phylink_generic_validate() when there is no
phylink_validate method given in the DSA switch operations and
mac_capabilities have been set in the phylink_config structure by the
DSA switch driver.

This gives DSA switch drivers the option to use this if they provide
the supported_interfaces and mac_capabilities, while still giving them
an option to override the default implementation if necessary.

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

diff --git a/net/dsa/port.c b/net/dsa/port.c
index eaa66114924b..d928be884f01 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -981,8 +981,11 @@ static void dsa_port_phylink_validate(struct phylink_config *config,
 	struct dsa_port *dp = container_of(config, struct dsa_port, pl_config);
 	struct dsa_switch *ds = dp->ds;
 
-	if (!ds->ops->phylink_validate)
+	if (!ds->ops->phylink_validate) {
+		if (config->mac_capabilities)
+			phylink_generic_validate(config, supported, state);
 		return;
+	}
 
 	ds->ops->phylink_validate(ds, dp->index, supported, state);
 }
-- 
2.30.2

Re: [PATCH RFC net-next 02/12] net: dsa: support use of phylink_generic_validate()

[Vladimir Oltean]

On Wed, Nov 24, 2021 at 05:52:33PM +0000, Russell King (Oracle) wrote:
> Support the use of phylink_generic_validate() when there is no
> phylink_validate method given in the DSA switch operations and
> mac_capabilities have been set in the phylink_config structure by the
> DSA switch driver.
> 
> This gives DSA switch drivers the option to use this if they provide
> the supported_interfaces and mac_capabilities, while still giving them
> an option to override the default implementation if necessary.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

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

>  net/dsa/port.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index eaa66114924b..d928be884f01 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -981,8 +981,11 @@ static void dsa_port_phylink_validate(struct phylink_config *config,
>  	struct dsa_port *dp = container_of(config, struct dsa_port, pl_config);
>  	struct dsa_switch *ds = dp->ds;
>  
> -	if (!ds->ops->phylink_validate)
> +	if (!ds->ops->phylink_validate) {
> +		if (config->mac_capabilities)
> +			phylink_generic_validate(config, supported, state);
>  		return;
> +	}
>  
>  	ds->ops->phylink_validate(ds, dp->index, supported, state);
>  }
> -- 
> 2.30.2
>

[PATCH RFC net-next 03/12] net: dsa: replace phylink_get_interfaces() with phylink_get_caps()

[Russell King (Oracle)]

Phylink needs slightly more information than phylink_get_interfaces()
allows us to get from the DSA drivers - we need the MAC capabilities.
Replace the phylink_get_interfaces() method with phylink_get_caps() to
allow DSA drivers to fill in the phylink_config MAC capabilities field
as well.

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

diff --git a/include/net/dsa.h b/include/net/dsa.h
index eff5c44ba377..8ca9d50cbbc2 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -645,8 +645,8 @@ struct dsa_switch_ops {
 	/*
 	 * PHYLINK integration
 	 */
-	void	(*phylink_get_interfaces)(struct dsa_switch *ds, int port,
-					  unsigned long *supported_interfaces);
+	void	(*phylink_get_caps)(struct dsa_switch *ds, int port,
+				    struct phylink_config *config);
 	void	(*phylink_validate)(struct dsa_switch *ds, int port,
 				    unsigned long *supported,
 				    struct phylink_link_state *state);
diff --git a/net/dsa/port.c b/net/dsa/port.c
index d928be884f01..6d5ebe61280b 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -1094,9 +1094,8 @@ int dsa_port_phylink_create(struct dsa_port *dp)
 	if (err)
 		mode = PHY_INTERFACE_MODE_NA;
 
-	if (ds->ops->phylink_get_interfaces)
-		ds->ops->phylink_get_interfaces(ds, dp->index,
-					dp->pl_config.supported_interfaces);
+	if (ds->ops->phylink_get_caps)
+		ds->ops->phylink_get_caps(ds, dp->index, &dp->pl_config);
 
 	dp->pl = phylink_create(&dp->pl_config, of_fwnode_handle(dp->dn),
 				mode, &dsa_port_phylink_mac_ops);
-- 
2.30.2

Re: [PATCH RFC net-next 03/12] net: dsa: replace phylink_get_interfaces() with phylink_get_caps()

[Vladimir Oltean]

On Wed, Nov 24, 2021 at 05:52:38PM +0000, Russell King (Oracle) wrote:
> Phylink needs slightly more information than phylink_get_interfaces()
> allows us to get from the DSA drivers - we need the MAC capabilities.
> Replace the phylink_get_interfaces() method with phylink_get_caps() to
> allow DSA drivers to fill in the phylink_config MAC capabilities field
> as well.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

The effects of submitting new API without any user :)

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

>  include/net/dsa.h | 4 ++--
>  net/dsa/port.c    | 5 ++---
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index eff5c44ba377..8ca9d50cbbc2 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -645,8 +645,8 @@ struct dsa_switch_ops {
>  	/*
>  	 * PHYLINK integration
>  	 */
> -	void	(*phylink_get_interfaces)(struct dsa_switch *ds, int port,
> -					  unsigned long *supported_interfaces);
> +	void	(*phylink_get_caps)(struct dsa_switch *ds, int port,
> +				    struct phylink_config *config);
>  	void	(*phylink_validate)(struct dsa_switch *ds, int port,
>  				    unsigned long *supported,
>  				    struct phylink_link_state *state);
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index d928be884f01..6d5ebe61280b 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -1094,9 +1094,8 @@ int dsa_port_phylink_create(struct dsa_port *dp)
>  	if (err)
>  		mode = PHY_INTERFACE_MODE_NA;
>  
> -	if (ds->ops->phylink_get_interfaces)
> -		ds->ops->phylink_get_interfaces(ds, dp->index,
> -					dp->pl_config.supported_interfaces);
> +	if (ds->ops->phylink_get_caps)
> +		ds->ops->phylink_get_caps(ds, dp->index, &dp->pl_config);
>  
>  	dp->pl = phylink_create(&dp->pl_config, of_fwnode_handle(dp->dn),
>  				mode, &dsa_port_phylink_mac_ops);
> -- 
> 2.30.2
>

Re: [PATCH RFC net-next 03/12] net: dsa: replace phylink_get_interfaces() with phylink_get_caps()

[Russell King (Oracle)]

On Wed, Nov 24, 2021 at 06:15:08PM +0000, Vladimir Oltean wrote:
> On Wed, Nov 24, 2021 at 05:52:38PM +0000, Russell King (Oracle) wrote:
> > Phylink needs slightly more information than phylink_get_interfaces()
> > allows us to get from the DSA drivers - we need the MAC capabilities.
> > Replace the phylink_get_interfaces() method with phylink_get_caps() to
> > allow DSA drivers to fill in the phylink_config MAC capabilities field
> > as well.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> 
> The effects of submitting new API without any user :)

It had users at the time, but they were not submitted, and the addition
of MAC capabilities was a future development. Had they been submitted at
the time, then they would have required updating too.

So that's entirely irrelevent.

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

Re: [PATCH RFC net-next 03/12] net: dsa: replace phylink_get_interfaces() with phylink_get_caps()

[Russell King (Oracle)]

On Wed, Nov 24, 2021 at 06:26:48PM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 24, 2021 at 06:15:08PM +0000, Vladimir Oltean wrote:
> > On Wed, Nov 24, 2021 at 05:52:38PM +0000, Russell King (Oracle) wrote:
> > > Phylink needs slightly more information than phylink_get_interfaces()
> > > allows us to get from the DSA drivers - we need the MAC capabilities.
> > > Replace the phylink_get_interfaces() method with phylink_get_caps() to
> > > allow DSA drivers to fill in the phylink_config MAC capabilities field
> > > as well.
> > > 
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > ---
> > 
> > The effects of submitting new API without any user :)
> 
> It had users at the time, but they were not submitted, and the addition
> of MAC capabilities was a future development. Had they been submitted at
> the time, then they would have required updating too.

That was a bit rushed... to explain more fully.

Prior to the merge window, the development work was centered around
only eliminating the PHY_INTERFACE_MODE_xxx checks and the complexity
that the PHY_INTERFACE_MODE_NA technique brought into the many
validation functions. Users of this had already been merged, and
included mvneta and mvpp2. See these commits, which are all in
v5.16-rc1:

b63f1117aefc net: mvpp2: clean up mvpp2_phylink_validate()
76947a635874 net: mvpp2: drop use of phylink_helper_basex_speed()
6c0c4b7ac06f net: mvpp2: remove interface checks in mvpp2_phylink_validate()
8498e17ed4c5 net: mvpp2: populate supported_interfaces member

099cbfa286ab net: mvneta: drop use of phylink_helper_basex_speed()
d9ca72807ecb net: mvneta: remove interface checks in mvneta_validate()
fdedb695e6a8 net: mvneta: populate supported_interfaces member

The original commit adding phylink_get_interfaces() extended this
into DSA, and the intention was to submit at least mv88e6xxx, but
it was too close to the merge window to do so.

Through making that change and eventually eliminating the basex helper
from all drivers that were using it, thereby making the validate()
behaviour much cleaner, it then became clear that it was possible to
push this cleanup further by also introducing a MAC capabilities field
to phylink_config.

The addition of the supported_interfaces member and the addition of the
mac_capabilities member are two entirely separate developments, but I
have now chosen to combine the two after the merge window in order to
reduce the number of patches. They were separate patches in my tree up
until relatively recently, and still are for the mt7530 and b53 drivers
currently.

So no, this is not "The effects of submitting new API without any user".

Thanks.

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

Re: [PATCH RFC net-next 03/12] net: dsa: replace phylink_get_interfaces() with phylink_get_caps()

[Vladimir Oltean]

On Wed, Nov 24, 2021 at 07:10:49PM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 24, 2021 at 06:26:48PM +0000, Russell King (Oracle) wrote:
> > On Wed, Nov 24, 2021 at 06:15:08PM +0000, Vladimir Oltean wrote:
> > > On Wed, Nov 24, 2021 at 05:52:38PM +0000, Russell King (Oracle) wrote:
> > > > Phylink needs slightly more information than phylink_get_interfaces()
> > > > allows us to get from the DSA drivers - we need the MAC capabilities.
> > > > Replace the phylink_get_interfaces() method with phylink_get_caps() to
> > > > allow DSA drivers to fill in the phylink_config MAC capabilities field
> > > > as well.
> > > > 
> > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > > ---
> > > 
> > > The effects of submitting new API without any user :)
> > 
> > It had users at the time, but they were not submitted, and the addition
> > of MAC capabilities was a future development. Had they been submitted at
> > the time, then they would have required updating too.
> 
> That was a bit rushed... to explain more fully.
> 
> Prior to the merge window, the development work was centered around
> only eliminating the PHY_INTERFACE_MODE_xxx checks and the complexity
> that the PHY_INTERFACE_MODE_NA technique brought into the many
> validation functions. Users of this had already been merged, and
> included mvneta and mvpp2. See these commits, which are all in
> v5.16-rc1:
> 
> b63f1117aefc net: mvpp2: clean up mvpp2_phylink_validate()
> 76947a635874 net: mvpp2: drop use of phylink_helper_basex_speed()
> 6c0c4b7ac06f net: mvpp2: remove interface checks in mvpp2_phylink_validate()
> 8498e17ed4c5 net: mvpp2: populate supported_interfaces member
> 
> 099cbfa286ab net: mvneta: drop use of phylink_helper_basex_speed()
> d9ca72807ecb net: mvneta: remove interface checks in mvneta_validate()
> fdedb695e6a8 net: mvneta: populate supported_interfaces member
> 
> The original commit adding phylink_get_interfaces() extended this
> into DSA, and the intention was to submit at least mv88e6xxx, but
> it was too close to the merge window to do so.
> 
> Through making that change and eventually eliminating the basex helper
> from all drivers that were using it, thereby making the validate()
> behaviour much cleaner, it then became clear that it was possible to
> push this cleanup further by also introducing a MAC capabilities field
> to phylink_config.
> 
> The addition of the supported_interfaces member and the addition of the
> mac_capabilities member are two entirely separate developments, but I
> have now chosen to combine the two after the merge window in order to
> reduce the number of patches. They were separate patches in my tree up
> until relatively recently, and still are for the mt7530 and b53 drivers
> currently.
> 
> So no, this is not "The effects of submitting new API without any user".
> 
> Thanks.

Ok, the patch is not the effect of submitting new API without any user.
It is just the effect of more development done to API without any user,
without any causal relationship between the two. My bad.

Re: [PATCH RFC net-next 03/12] net: dsa: replace phylink_get_interfaces() with phylink_get_caps()

[Russell King (Oracle)]

On Wed, Nov 24, 2021 at 08:26:13PM +0000, Vladimir Oltean wrote:
> On Wed, Nov 24, 2021 at 07:10:49PM +0000, Russell King (Oracle) wrote:
> > On Wed, Nov 24, 2021 at 06:26:48PM +0000, Russell King (Oracle) wrote:
> > > On Wed, Nov 24, 2021 at 06:15:08PM +0000, Vladimir Oltean wrote:
> > > > On Wed, Nov 24, 2021 at 05:52:38PM +0000, Russell King (Oracle) wrote:
> > > > > Phylink needs slightly more information than phylink_get_interfaces()
> > > > > allows us to get from the DSA drivers - we need the MAC capabilities.
> > > > > Replace the phylink_get_interfaces() method with phylink_get_caps() to
> > > > > allow DSA drivers to fill in the phylink_config MAC capabilities field
> > > > > as well.
> > > > > 
> > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > > > ---
> > > > 
> > > > The effects of submitting new API without any user :)
> > > 
> > > It had users at the time, but they were not submitted, and the addition
> > > of MAC capabilities was a future development. Had they been submitted at
> > > the time, then they would have required updating too.
> > 
> > That was a bit rushed... to explain more fully.
> > 
> > Prior to the merge window, the development work was centered around
> > only eliminating the PHY_INTERFACE_MODE_xxx checks and the complexity
> > that the PHY_INTERFACE_MODE_NA technique brought into the many
> > validation functions. Users of this had already been merged, and
> > included mvneta and mvpp2. See these commits, which are all in
> > v5.16-rc1:
> > 
> > b63f1117aefc net: mvpp2: clean up mvpp2_phylink_validate()
> > 76947a635874 net: mvpp2: drop use of phylink_helper_basex_speed()
> > 6c0c4b7ac06f net: mvpp2: remove interface checks in mvpp2_phylink_validate()
> > 8498e17ed4c5 net: mvpp2: populate supported_interfaces member
> > 
> > 099cbfa286ab net: mvneta: drop use of phylink_helper_basex_speed()
> > d9ca72807ecb net: mvneta: remove interface checks in mvneta_validate()
> > fdedb695e6a8 net: mvneta: populate supported_interfaces member
> > 
> > The original commit adding phylink_get_interfaces() extended this
> > into DSA, and the intention was to submit at least mv88e6xxx, but
> > it was too close to the merge window to do so.
> > 
> > Through making that change and eventually eliminating the basex helper
> > from all drivers that were using it, thereby making the validate()
> > behaviour much cleaner, it then became clear that it was possible to
> > push this cleanup further by also introducing a MAC capabilities field
> > to phylink_config.
> > 
> > The addition of the supported_interfaces member and the addition of the
> > mac_capabilities member are two entirely separate developments, but I
> > have now chosen to combine the two after the merge window in order to
> > reduce the number of patches. They were separate patches in my tree up
> > until relatively recently, and still are for the mt7530 and b53 drivers
> > currently.
> > 
> > So no, this is not "The effects of submitting new API without any user".
> > 
> > Thanks.
> 
> Ok, the patch is not the effect of submitting new API without any user.
> It is just the effect of more development done to API without any user,
> without any causal relationship between the two. My bad.

I do wonder whether you intentionally missed where I said "It had users
at the time, but they were not submitted". This is why we don't get on
well, you're always so confrontational.

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

Re: [PATCH RFC net-next 03/12] net: dsa: replace phylink_get_interfaces() with phylink_get_caps()

[Vladimir Oltean]

On Wed, Nov 24, 2021 at 08:56:33PM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 24, 2021 at 08:26:13PM +0000, Vladimir Oltean wrote:
> > On Wed, Nov 24, 2021 at 07:10:49PM +0000, Russell King (Oracle) wrote:
> > > On Wed, Nov 24, 2021 at 06:26:48PM +0000, Russell King (Oracle) wrote:
> > > > On Wed, Nov 24, 2021 at 06:15:08PM +0000, Vladimir Oltean wrote:
> > > > > On Wed, Nov 24, 2021 at 05:52:38PM +0000, Russell King (Oracle) wrote:
> > > > > > Phylink needs slightly more information than phylink_get_interfaces()
> > > > > > allows us to get from the DSA drivers - we need the MAC capabilities.
> > > > > > Replace the phylink_get_interfaces() method with phylink_get_caps() to
> > > > > > allow DSA drivers to fill in the phylink_config MAC capabilities field
> > > > > > as well.
> > > > > > 
> > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > > > > ---
> > > > > 
> > > > > The effects of submitting new API without any user :)
> > > > 
> > > > It had users at the time, but they were not submitted, and the addition
> > > > of MAC capabilities was a future development. Had they been submitted at
> > > > the time, then they would have required updating too.
> > > 
> > > That was a bit rushed... to explain more fully.
> > > 
> > > Prior to the merge window, the development work was centered around
> > > only eliminating the PHY_INTERFACE_MODE_xxx checks and the complexity
> > > that the PHY_INTERFACE_MODE_NA technique brought into the many
> > > validation functions. Users of this had already been merged, and
> > > included mvneta and mvpp2. See these commits, which are all in
> > > v5.16-rc1:
> > > 
> > > b63f1117aefc net: mvpp2: clean up mvpp2_phylink_validate()
> > > 76947a635874 net: mvpp2: drop use of phylink_helper_basex_speed()
> > > 6c0c4b7ac06f net: mvpp2: remove interface checks in mvpp2_phylink_validate()
> > > 8498e17ed4c5 net: mvpp2: populate supported_interfaces member
> > > 
> > > 099cbfa286ab net: mvneta: drop use of phylink_helper_basex_speed()
> > > d9ca72807ecb net: mvneta: remove interface checks in mvneta_validate()
> > > fdedb695e6a8 net: mvneta: populate supported_interfaces member
> > > 
> > > The original commit adding phylink_get_interfaces() extended this
> > > into DSA, and the intention was to submit at least mv88e6xxx, but
> > > it was too close to the merge window to do so.
> > > 
> > > Through making that change and eventually eliminating the basex helper
> > > from all drivers that were using it, thereby making the validate()
> > > behaviour much cleaner, it then became clear that it was possible to
> > > push this cleanup further by also introducing a MAC capabilities field
> > > to phylink_config.
> > > 
> > > The addition of the supported_interfaces member and the addition of the
> > > mac_capabilities member are two entirely separate developments, but I
> > > have now chosen to combine the two after the merge window in order to
> > > reduce the number of patches. They were separate patches in my tree up
> > > until relatively recently, and still are for the mt7530 and b53 drivers
> > > currently.
> > > 
> > > So no, this is not "The effects of submitting new API without any user".
> > > 
> > > Thanks.
> > 
> > Ok, the patch is not the effect of submitting new API without any user.
> > It is just the effect of more development done to API without any user,
> > without any causal relationship between the two. My bad.
> 
> I do wonder whether you intentionally missed where I said "It had users
> at the time, but they were not submitted". This is why we don't get on
> well, you're always so confrontational.

David who applied your patch can correct me, but my understanding from
the little time I've spent on netdev is that dead code isn't a candidate
for getting accepted into the tree, even more so in the last few days
before the merge window, from where it got into v5.16-rc1. About the
only exception I know of is when introducing a function which is only to
be called later in the series, and both the caller and the callee are
subject to review. Sure, your hook isn't doing any harm there, save for
a few extra bytes of kernel .text, and I know that your intention was
for Prasanna to use that new callback for the lan937x driver, but the
truth is that there are other ways to achieve what you want, like for
example ask Prasanna to pick your patch and submit it along with his
lan937x driver, or you submit it yourself when the time comes for other
drivers to be converted, whichever comes first.

So yes, I take issue with that as a matter of principle, I very much
expect that a kernel developer of your experience does not set a
precedent and a pretext for people who submit various shady stuff to the
kernel just to make their downstream life easier.

[PATCH RFC net-next 04/12] net: dsa: ar9331: convert to phylink_generic_validate()

[Russell King (Oracle)]

Populate the supported interfaces and MAC capabilities for the AR9331
DSA switch and remove the old validate implementation to allow DSA to
use phylink_generic_validate() for this switch driver.

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

diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
index da0d7e68643a..3bda7015f0c1 100644
--- a/drivers/net/dsa/qca/ar9331.c
+++ b/drivers/net/dsa/qca/ar9331.c
@@ -499,52 +499,27 @@ static enum dsa_tag_protocol ar9331_sw_get_tag_protocol(struct dsa_switch *ds,
 	return DSA_TAG_PROTO_AR9331;
 }
 
-static void ar9331_sw_phylink_validate(struct dsa_switch *ds, int port,
-				       unsigned long *supported,
-				       struct phylink_link_state *state)
+static void ar9331_sw_phylink_get_caps(struct dsa_switch *ds, int port,
+				       struct phylink_config *config)
 {
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
+	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
+		MAC_10 | MAC_100;
 
 	switch (port) {
 	case 0:
-		if (state->interface != PHY_INTERFACE_MODE_GMII)
-			goto unsupported;
-
-		phylink_set(mask, 1000baseT_Full);
-		phylink_set(mask, 1000baseT_Half);
+		__set_bit(PHY_INTERFACE_MODE_GMII,
+			  config->supported_interfaces);
+		config->mac_capabilities |= MAC_1000;
 		break;
 	case 1:
 	case 2:
 	case 3:
 	case 4:
 	case 5:
-		if (state->interface != PHY_INTERFACE_MODE_INTERNAL)
-			goto unsupported;
+		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
+			  config->supported_interfaces);
 		break;
-	default:
-		linkmode_zero(supported);
-		dev_err(ds->dev, "Unsupported port: %i\n", port);
-		return;
 	}
-
-	phylink_set_port_modes(mask);
-	phylink_set(mask, Pause);
-	phylink_set(mask, Asym_Pause);
-
-	phylink_set(mask, 10baseT_Half);
-	phylink_set(mask, 10baseT_Full);
-	phylink_set(mask, 100baseT_Half);
-	phylink_set(mask, 100baseT_Full);
-
-	linkmode_and(supported, supported, mask);
-	linkmode_and(state->advertising, state->advertising, mask);
-
-	return;
-
-unsupported:
-	linkmode_zero(supported);
-	dev_err(ds->dev, "Unsupported interface: %d, port: %d\n",
-		state->interface, port);
 }
 
 static void ar9331_sw_phylink_mac_config(struct dsa_switch *ds, int port,
@@ -697,7 +672,7 @@ static const struct dsa_switch_ops ar9331_sw_ops = {
 	.get_tag_protocol	= ar9331_sw_get_tag_protocol,
 	.setup			= ar9331_sw_setup,
 	.port_disable		= ar9331_sw_port_disable,
-	.phylink_validate	= ar9331_sw_phylink_validate,
+	.phylink_get_caps	= ar9331_sw_phylink_get_caps,
 	.phylink_mac_config	= ar9331_sw_phylink_mac_config,
 	.phylink_mac_link_down	= ar9331_sw_phylink_mac_link_down,
 	.phylink_mac_link_up	= ar9331_sw_phylink_mac_link_up,
-- 
2.30.2

[PATCH RFC net-next 05/12] net: dsa: bcm_sf2: convert to phylink_generic_validate()

[Russell King (Oracle)]

Populate the supported interfaces and MAC capabilities for the bcm_sf2
DSA switch and remove the old validate implementation to allow DSA to
use phylink_generic_validate() for this switch driver.

The exclusion of Gigabit linkmodes for MII and Reverse MII links is
handled within phylink_generic_validate() in phylink, so there is no
need to make them conditional on the interface mode in the driver.

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

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 13aa43b5cffd..d6ef0fb0d943 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -672,49 +672,18 @@ static u32 bcm_sf2_sw_get_phy_flags(struct dsa_switch *ds, int port)
 		       PHY_BRCM_IDDQ_SUSPEND;
 }
 
-static void bcm_sf2_sw_validate(struct dsa_switch *ds, int port,
-				unsigned long *supported,
-				struct phylink_link_state *state)
+static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port,
+				struct phylink_config *config)
 {
-	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
-
-	if (!phy_interface_mode_is_rgmii(state->interface) &&
-	    state->interface != PHY_INTERFACE_MODE_MII &&
-	    state->interface != PHY_INTERFACE_MODE_REVMII &&
-	    state->interface != PHY_INTERFACE_MODE_GMII &&
-	    state->interface != PHY_INTERFACE_MODE_INTERNAL &&
-	    state->interface != PHY_INTERFACE_MODE_MOCA) {
-		linkmode_zero(supported);
-		if (port != core_readl(priv, CORE_IMP0_PRT_ID))
-			dev_err(ds->dev,
-				"Unsupported interface: %d for port %d\n",
-				state->interface, port);
-		return;
-	}
-
-	/* Allow all the expected bits */
-	phylink_set(mask, Autoneg);
-	phylink_set_port_modes(mask);
-	phylink_set(mask, Pause);
-	phylink_set(mask, Asym_Pause);
-
-	/* With the exclusion of MII and Reverse MII, we support Gigabit,
-	 * including Half duplex
-	 */
-	if (state->interface != PHY_INTERFACE_MODE_MII &&
-	    state->interface != PHY_INTERFACE_MODE_REVMII) {
-		phylink_set(mask, 1000baseT_Full);
-		phylink_set(mask, 1000baseT_Half);
-	}
-
-	phylink_set(mask, 10baseT_Half);
-	phylink_set(mask, 10baseT_Full);
-	phylink_set(mask, 100baseT_Half);
-	phylink_set(mask, 100baseT_Full);
-
-	linkmode_and(supported, supported, mask);
-	linkmode_and(state->advertising, state->advertising, mask);
+	__set_bit(PHY_INTERFACE_MODE_MII, config->supported_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_REVMII, config->supported_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_GMII, config->supported_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_INTERNAL, config->supported_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_MOCA, config->supported_interfaces);
+	phy_interface_set_rgmii(config->supported_interfaces);
+
+	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
+		MAC_10 | MAC_100 | MAC_1000;
 }
 
 static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port,
@@ -1181,7 +1150,7 @@ static const struct dsa_switch_ops bcm_sf2_ops = {
 	.get_sset_count		= bcm_sf2_sw_get_sset_count,
 	.get_ethtool_phy_stats	= b53_get_ethtool_phy_stats,
 	.get_phy_flags		= bcm_sf2_sw_get_phy_flags,
-	.phylink_validate	= bcm_sf2_sw_validate,
+	.phylink_get_caps	= bcm_sf2_sw_get_caps,
 	.phylink_mac_config	= bcm_sf2_sw_mac_config,
 	.phylink_mac_link_down	= bcm_sf2_sw_mac_link_down,
 	.phylink_mac_link_up	= bcm_sf2_sw_mac_link_up,
-- 
2.30.2

Re: [PATCH RFC net-next 05/12] net: dsa: bcm_sf2: convert to phylink_generic_validate()

[Florian Fainelli]

On 11/24/21 9:52 AM, Russell King (Oracle) wrote:
> Populate the supported interfaces and MAC capabilities for the bcm_sf2
> DSA switch and remove the old validate implementation to allow DSA to
> use phylink_generic_validate() for this switch driver.
> 
> The exclusion of Gigabit linkmodes for MII and Reverse MII links is
> handled within phylink_generic_validate() in phylink, so there is no
> need to make them conditional on the interface mode in the driver.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Tested-by: Florian Fainelli <f.fainelli@gmail.com>

but it looks like the fixed link ports are reporting some pretty strange
advertisement values one of my two platforms running the same kernel image:

# ethtool rgmii_2
Settings for rgmii_2:
        Supported ports: [ MII ]
        Supported link modes:   1000baseKX/Full
        Supported pause frame use: Symmetric Receive-only
        Supports auto-negotiation: Yes
        Supported FEC modes: Not reported
        Advertised link modes:  1000baseKX/Full
        Advertised pause frame use: Symmetric Receive-only
        Advertised auto-negotiation: Yes
        Advertised FEC modes: Not reported
        Link partner advertised link modes:  1000baseKX/Full
        Link partner advertised pause frame use: No
        Link partner advertised auto-negotiation: No
        Link partner advertised FEC modes: Not reported
        Speed: 1000Mb/s
        Duplex: Full
        Auto-negotiation: on
        Port: MII
        PHYAD: 0
        Transceiver: internal
        Supports Wake-on: gsf
        Wake-on: d
        SecureOn password: 00:00:00:00:00:00
        Link detected: yes
#

These should be 1000BaseT/Full since these are RGMII fixed links:

# ethtool rgmii_2
Settings for rgmii_2:
        Supported ports: [ MII ]
        Supported link modes:   1000baseT/Full
        Supported pause frame use: Symmetric Receive-only
        Supports auto-negotiation: Yes
        Supported FEC modes: Not reported
        Advertised link modes:  1000baseT/Full
        Advertised pause frame use: Symmetric Receive-only
        Advertised auto-negotiation: Yes
        Advertised FEC modes: Not reported
        Link partner advertised link modes:  1000baseT/Full
        Link partner advertised pause frame use: No
        Link partner advertised auto-negotiation: No
        Link partner advertised FEC modes: Not reported
        Speed: 1000Mb/s
        Duplex: Full
        Auto-negotiation: on
        Port: MII
        PHYAD: 0
        Transceiver: internal
        Supports Wake-on: gsf
        Wake-on: d
        SecureOn password: 00:00:00:00:00:00
        Link detected: yes
#

There is no problem with Linus' master branch at
net-5.16-rc4-173-ga51e3ac43ddb, let me see if I can bisect this and/or
fix it in the next days.

Thanks!
-- 
Florian

Re: [PATCH RFC net-next 05/12] net: dsa: bcm_sf2: convert to phylink_generic_validate()

[Florian Fainelli]

On 12/3/21 12:03 PM, Florian Fainelli wrote:
> On 11/24/21 9:52 AM, Russell King (Oracle) wrote:
>> Populate the supported interfaces and MAC capabilities for the bcm_sf2
>> DSA switch and remove the old validate implementation to allow DSA to
>> use phylink_generic_validate() for this switch driver.
>>
>> The exclusion of Gigabit linkmodes for MII and Reverse MII links is
>> handled within phylink_generic_validate() in phylink, so there is no
>> need to make them conditional on the interface mode in the driver.
>>
>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> 
> Tested-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> but it looks like the fixed link ports are reporting some pretty strange
> advertisement values one of my two platforms running the same kernel image:

We would want to amend your patch with something that caters a bit more
towards how the ports have been configured:

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index d6ef0fb0d943..88933c3feddd 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -675,12 +675,18 @@ static u32 bcm_sf2_sw_get_phy_flags(struct
dsa_switch *ds, int port)
  static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port,
                                 struct phylink_config *config)
  {
-       __set_bit(PHY_INTERFACE_MODE_MII, config->supported_interfaces);
-       __set_bit(PHY_INTERFACE_MODE_REVMII, config->supported_interfaces);
-       __set_bit(PHY_INTERFACE_MODE_GMII, config->supported_interfaces);
-       __set_bit(PHY_INTERFACE_MODE_INTERNAL,
config->supported_interfaces);
-       __set_bit(PHY_INTERFACE_MODE_MOCA, config->supported_interfaces);
-       phy_interface_set_rgmii(config->supported_interfaces);
+       struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
+
+       if (priv->int_phy_mask & BIT(port))
+               __set_bit(PHY_INTERFACE_MODE_INTERNAL,
config->supported_interfaces);
+       else if (priv->moca_port == port)
+               __set_bit(PHY_INTERFACE_MODE_MOCA,
config->supported_interfaces);
+       else {
+               __set_bit(PHY_INTERFACE_MODE_MII,
config->supported_interfaces);
+               __set_bit(PHY_INTERFACE_MODE_REVMII,
config->supported_interfaces);
+               __set_bit(PHY_INTERFACE_MODE_GMII,
config->supported_interfaces);
+               phy_interface_set_rgmii(config->supported_interfaces);
+       }

         config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
                 MAC_10 | MAC_100 | MAC_1000;

Now, with respect to the fixed link ports reporting 1000baseKX/Full this 
is introduced by switching to your patch, it works before and it 
"breaks" after.

The first part that is a bit weird is that we seem to be calling 
phylink_generic_validate() twice in a row from the same call site.

For fixed link ports, instead of masking with what the fixed link 
actually supports, we seem to be using a supported mask which is all 1s 
which seems a bit excessive for a fixed link.

This is an excerpt with the internal PHY:

[    4.210890] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized): 
Calling phylink_generic_validate
[    4.220063] before phylink_get_linkmodes: 0000000,00000000,00010fc0
[    4.226357] phylink_get_linkmodes: caps: 0xffffffff mac_capabilities: 
0xff
[    4.233258] after phylink_get_linkmodes: c000018,00000200,00036fff
[    4.239463] before anding supported with mask: 0000000,00000000,000062ff
[    4.246189] after anding supported with mask: 0000000,00000000,000062ff
[    4.252829] before anding advertising with mask: 
c000018,00000200,00036fff
[    4.259729] after anding advertising with mask: c000018,00000200,00036fff
[    4.266546] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized): 
PHY [f0b403c0.mdio--1:05] driver [Broadcom BCM7445] (irq=POLL)

and this is what a fixed link port looks like:

[    4.430765] brcm-sf2 f0b00000.ethernet_switch rgmii_2 
(uninitialized): Calling phylink_generic_validate
[    4.440205] before phylink_get_linkmodes: 0000000,00000000,00010fc0
[    4.446500] phylink_get_linkmodes: caps: 0xff mac_capabilities: 0xff
[    4.452880] after phylink_get_linkmodes: c000018,00000200,00036fff
[    4.459085] before anding supported with mask: fffffff,ffffffff,ffffffff
[    4.465811] after anding supported with mask: c000018,00000200,00036fff
[    4.472450] before anding advertising with mask: 
c000018,00000200,00036fff
[    4.479349] after anding advertising with mask: c000018,00000200,00036fff

or maybe the problem is with phylink_get_ksettings... ran out of time 
tonight to look further into it.
-- 
Florian

Re: [PATCH RFC net-next 05/12] net: dsa: bcm_sf2: convert to phylink_generic_validate()

[Russell King (Oracle)]

On Fri, Dec 03, 2021 at 08:18:22PM -0800, Florian Fainelli wrote:
> On 12/3/21 12:03 PM, Florian Fainelli wrote:
> > On 11/24/21 9:52 AM, Russell King (Oracle) wrote:
> > > Populate the supported interfaces and MAC capabilities for the bcm_sf2
> > > DSA switch and remove the old validate implementation to allow DSA to
> > > use phylink_generic_validate() for this switch driver.
> > > 
> > > The exclusion of Gigabit linkmodes for MII and Reverse MII links is
> > > handled within phylink_generic_validate() in phylink, so there is no
> > > need to make them conditional on the interface mode in the driver.
> > > 
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > 
> > Tested-by: Florian Fainelli <f.fainelli@gmail.com>
> > 
> > but it looks like the fixed link ports are reporting some pretty strange
> > advertisement values one of my two platforms running the same kernel image:
> 
> We would want to amend your patch with something that caters a bit more
> towards how the ports have been configured:
> 
> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
> index d6ef0fb0d943..88933c3feddd 100644
> --- a/drivers/net/dsa/bcm_sf2.c
> +++ b/drivers/net/dsa/bcm_sf2.c
> @@ -675,12 +675,18 @@ static u32 bcm_sf2_sw_get_phy_flags(struct
> dsa_switch *ds, int port)
>  static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port,
>                                 struct phylink_config *config)
>  {
> -       __set_bit(PHY_INTERFACE_MODE_MII, config->supported_interfaces);
> -       __set_bit(PHY_INTERFACE_MODE_REVMII, config->supported_interfaces);
> -       __set_bit(PHY_INTERFACE_MODE_GMII, config->supported_interfaces);
> -       __set_bit(PHY_INTERFACE_MODE_INTERNAL,
> config->supported_interfaces);
> -       __set_bit(PHY_INTERFACE_MODE_MOCA, config->supported_interfaces);
> -       phy_interface_set_rgmii(config->supported_interfaces);
> +       struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
> +
> +       if (priv->int_phy_mask & BIT(port))
> +               __set_bit(PHY_INTERFACE_MODE_INTERNAL,
> config->supported_interfaces);
> +       else if (priv->moca_port == port)
> +               __set_bit(PHY_INTERFACE_MODE_MOCA,
> config->supported_interfaces);
> +       else {
> +               __set_bit(PHY_INTERFACE_MODE_MII,
> config->supported_interfaces);
> +               __set_bit(PHY_INTERFACE_MODE_REVMII,
> config->supported_interfaces);
> +               __set_bit(PHY_INTERFACE_MODE_GMII,
> config->supported_interfaces);
> +               phy_interface_set_rgmii(config->supported_interfaces);
> +       }
> 
>         config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
>                 MAC_10 | MAC_100 | MAC_1000;

That's fine, thanks for the update.

> Now, with respect to the fixed link ports reporting 1000baseKX/Full this is
> introduced by switching to your patch, it works before and it "breaks"
> after.
> 
> The first part that is a bit weird is that we seem to be calling
> phylink_generic_validate() twice in a row from the same call site.
> 
> For fixed link ports, instead of masking with what the fixed link actually
> supports, we seem to be using a supported mask which is all 1s which seems a
> bit excessive for a fixed link.
> 
> This is an excerpt with the internal PHY:
> 
> [    4.210890] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized):
> Calling phylink_generic_validate
> [    4.220063] before phylink_get_linkmodes: 0000000,00000000,00010fc0
> [    4.226357] phylink_get_linkmodes: caps: 0xffffffff mac_capabilities:
> 0xff
> [    4.233258] after phylink_get_linkmodes: c000018,00000200,00036fff
> [    4.239463] before anding supported with mask: 0000000,00000000,000062ff
> [    4.246189] after anding supported with mask: 0000000,00000000,000062ff
> [    4.252829] before anding advertising with mask:
> c000018,00000200,00036fff
> [    4.259729] after anding advertising with mask: c000018,00000200,00036fff
> [    4.266546] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized): PHY
> [f0b403c0.mdio--1:05] driver [Broadcom BCM7445] (irq=POLL)
> 
> and this is what a fixed link port looks like:
> 
> [    4.430765] brcm-sf2 f0b00000.ethernet_switch rgmii_2 (uninitialized):
> Calling phylink_generic_validate
> [    4.440205] before phylink_get_linkmodes: 0000000,00000000,00010fc0
> [    4.446500] phylink_get_linkmodes: caps: 0xff mac_capabilities: 0xff
> [    4.452880] after phylink_get_linkmodes: c000018,00000200,00036fff
> [    4.459085] before anding supported with mask: fffffff,ffffffff,ffffffff
> [    4.465811] after anding supported with mask: c000018,00000200,00036fff
> [    4.472450] before anding advertising with mask:
> c000018,00000200,00036fff
> [    4.479349] after anding advertising with mask: c000018,00000200,00036fff
> 
> or maybe the problem is with phylink_get_ksettings... ran out of time
> tonight to look further into it.

It will be:

        s = phy_lookup_setting(pl->link_config.speed, pl->link_config.duplex,
                               pl->supported, true);
        linkmode_zero(pl->supported);
        phylink_set(pl->supported, MII);
        phylink_set(pl->supported, Pause);
        phylink_set(pl->supported, Asym_Pause);
        phylink_set(pl->supported, Autoneg);
        if (s) {
                __set_bit(s->bit, pl->supported);
                __set_bit(s->bit, pl->link_config.lp_advertising);

Since 1000baseKX_Full is set in the supported mask, phy_lookup_setting()
returns the first entry it finds in the supported table:

        /* 1G */
        PHY_SETTING(   1000, FULL,   1000baseKX_Full            ),
        PHY_SETTING(   1000, FULL,   1000baseT_Full             ),
        PHY_SETTING(   1000, HALF,   1000baseT_Half             ),
        PHY_SETTING(   1000, FULL,   1000baseT1_Full            ),
        PHY_SETTING(   1000, FULL,   1000baseX_Full             ),

Consequently, 1000baseKX_Full is preferred over 1000baseT_Full.

Fixed links don't specify their underlying technology, only the speed
and duplex, so going from speed and duplex to an ethtool link mode is
not easy. I suppose we could drop 1000baseKX_Full from the supported
bitmap in phylink_parse_fixedlink() before the first phylink_validate()
call. Alternatively, the table could be re-ordered. It was supposed to
be grouped by speed and sorted in descending match priority as specified
by the comment above the table. Does it really make sense that
1000baseKX_Full is supposed to be preferred over all the other 1G
speeds? I suppose that's a question for Tom Lendacky
<thomas.lendacky@amd.com>, who introduced this in 3e7077067e80
("phy: Expand phy speed/duplex settings array") back in 2014.

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

Re: [PATCH RFC net-next 05/12] net: dsa: bcm_sf2: convert to phylink_generic_validate()

[Russell King (Oracle)]

On Sat, Dec 04, 2021 at 08:59:12AM +0000, Russell King (Oracle) wrote:
> On Fri, Dec 03, 2021 at 08:18:22PM -0800, Florian Fainelli wrote:
> > On 12/3/21 12:03 PM, Florian Fainelli wrote:
> > > On 11/24/21 9:52 AM, Russell King (Oracle) wrote:
> > > > Populate the supported interfaces and MAC capabilities for the bcm_sf2
> > > > DSA switch and remove the old validate implementation to allow DSA to
> > > > use phylink_generic_validate() for this switch driver.
> > > > 
> > > > The exclusion of Gigabit linkmodes for MII and Reverse MII links is
> > > > handled within phylink_generic_validate() in phylink, so there is no
> > > > need to make them conditional on the interface mode in the driver.
> > > > 
> > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > 
> > > Tested-by: Florian Fainelli <f.fainelli@gmail.com>
> > > 
> > > but it looks like the fixed link ports are reporting some pretty strange
> > > advertisement values one of my two platforms running the same kernel image:
> > 
> > We would want to amend your patch with something that caters a bit more
> > towards how the ports have been configured:
> > 
> > diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
> > index d6ef0fb0d943..88933c3feddd 100644
> > --- a/drivers/net/dsa/bcm_sf2.c
> > +++ b/drivers/net/dsa/bcm_sf2.c
> > @@ -675,12 +675,18 @@ static u32 bcm_sf2_sw_get_phy_flags(struct
> > dsa_switch *ds, int port)
> >  static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port,
> >                                 struct phylink_config *config)
> >  {
> > -       __set_bit(PHY_INTERFACE_MODE_MII, config->supported_interfaces);
> > -       __set_bit(PHY_INTERFACE_MODE_REVMII, config->supported_interfaces);
> > -       __set_bit(PHY_INTERFACE_MODE_GMII, config->supported_interfaces);
> > -       __set_bit(PHY_INTERFACE_MODE_INTERNAL,
> > config->supported_interfaces);
> > -       __set_bit(PHY_INTERFACE_MODE_MOCA, config->supported_interfaces);
> > -       phy_interface_set_rgmii(config->supported_interfaces);
> > +       struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
> > +
> > +       if (priv->int_phy_mask & BIT(port))
> > +               __set_bit(PHY_INTERFACE_MODE_INTERNAL,
> > config->supported_interfaces);
> > +       else if (priv->moca_port == port)
> > +               __set_bit(PHY_INTERFACE_MODE_MOCA,
> > config->supported_interfaces);
> > +       else {
> > +               __set_bit(PHY_INTERFACE_MODE_MII,
> > config->supported_interfaces);
> > +               __set_bit(PHY_INTERFACE_MODE_REVMII,
> > config->supported_interfaces);
> > +               __set_bit(PHY_INTERFACE_MODE_GMII,
> > config->supported_interfaces);
> > +               phy_interface_set_rgmii(config->supported_interfaces);
> > +       }
> > 
> >         config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> >                 MAC_10 | MAC_100 | MAC_1000;
> 
> That's fine, thanks for the update.

Here's the resulting updated patch. I've changed it slightly to avoid
the wrapping, and updated the commit text - please let me know if you'd
like any attributations added. Thanks!

8<===
From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
Subject: [PATCH RFC v2 net-next] net: dsa: bcm_sf2: convert to
 phylink_generic_validate()

Populate the supported interfaces and MAC capabilities for the bcm_sf2
DSA switch and remove the old validate implementation to allow DSA to
use phylink_generic_validate() for this switch driver.

The exclusion of Gigabit linkmodes for MII and Reverse MII links is
handled within phylink_generic_validate() in phylink, so there is no
need to make them conditional on the interface mode in the driver.

Thanks to Florian Fainelli for suggesting how to populate the supported
interfaces.

Link: https://lore.kernel.org/r/3b3fed98-0c82-99e9-dc72-09fe01c2bcf3@gmail.com
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/bcm_sf2.c | 54 +++++++++++----------------------------
 1 file changed, 15 insertions(+), 39 deletions(-)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 13aa43b5cffd..114d4ba7716f 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -672,49 +672,25 @@ static u32 bcm_sf2_sw_get_phy_flags(struct dsa_switch *ds, int port)
 		       PHY_BRCM_IDDQ_SUSPEND;
 }
 
-static void bcm_sf2_sw_validate(struct dsa_switch *ds, int port,
-				unsigned long *supported,
-				struct phylink_link_state *state)
+static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port,
+				struct phylink_config *config)
 {
+	unsigned long *interfaces = config->supported_interfaces;
 	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
-
-	if (!phy_interface_mode_is_rgmii(state->interface) &&
-	    state->interface != PHY_INTERFACE_MODE_MII &&
-	    state->interface != PHY_INTERFACE_MODE_REVMII &&
-	    state->interface != PHY_INTERFACE_MODE_GMII &&
-	    state->interface != PHY_INTERFACE_MODE_INTERNAL &&
-	    state->interface != PHY_INTERFACE_MODE_MOCA) {
-		linkmode_zero(supported);
-		if (port != core_readl(priv, CORE_IMP0_PRT_ID))
-			dev_err(ds->dev,
-				"Unsupported interface: %d for port %d\n",
-				state->interface, port);
-		return;
-	}
-
-	/* Allow all the expected bits */
-	phylink_set(mask, Autoneg);
-	phylink_set_port_modes(mask);
-	phylink_set(mask, Pause);
-	phylink_set(mask, Asym_Pause);
 
-	/* With the exclusion of MII and Reverse MII, we support Gigabit,
-	 * including Half duplex
-	 */
-	if (state->interface != PHY_INTERFACE_MODE_MII &&
-	    state->interface != PHY_INTERFACE_MODE_REVMII) {
-		phylink_set(mask, 1000baseT_Full);
-		phylink_set(mask, 1000baseT_Half);
+	if (priv->int_phy_mask & BIT(port)) {
+		__set_bit(PHY_INTERFACE_MODE_INTERNAL, interfaces);
+	} else if (priv->moca_port == port) {
+		__set_bit(PHY_INTERFACE_MODE_MOCA, interfaces);
+	} else {
+		__set_bit(PHY_INTERFACE_MODE_MII, interfaces);
+		__set_bit(PHY_INTERFACE_MODE_REVMII, interfaces);
+		__set_bit(PHY_INTERFACE_MODE_GMII, interfaces);
+		phy_interface_set_rgmii(interfaces);
 	}
 
-	phylink_set(mask, 10baseT_Half);
-	phylink_set(mask, 10baseT_Full);
-	phylink_set(mask, 100baseT_Half);
-	phylink_set(mask, 100baseT_Full);
-
-	linkmode_and(supported, supported, mask);
-	linkmode_and(state->advertising, state->advertising, mask);
+	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
+		MAC_10 | MAC_100 | MAC_1000;
 }
 
 static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port,
@@ -1181,7 +1157,7 @@ static const struct dsa_switch_ops bcm_sf2_ops = {
 	.get_sset_count		= bcm_sf2_sw_get_sset_count,
 	.get_ethtool_phy_stats	= b53_get_ethtool_phy_stats,
 	.get_phy_flags		= bcm_sf2_sw_get_phy_flags,
-	.phylink_validate	= bcm_sf2_sw_validate,
+	.phylink_get_caps	= bcm_sf2_sw_get_caps,
 	.phylink_mac_config	= bcm_sf2_sw_mac_config,
 	.phylink_mac_link_down	= bcm_sf2_sw_mac_link_down,
 	.phylink_mac_link_up	= bcm_sf2_sw_mac_link_up,
-- 
2.30.2

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

Re: [PATCH RFC net-next 05/12] net: dsa: bcm_sf2: convert to phylink_generic_validate()

[Russell King (Oracle)]

On Sat, Dec 04, 2021 at 08:59:12AM +0000, Russell King (Oracle) wrote:
> On Fri, Dec 03, 2021 at 08:18:22PM -0800, Florian Fainelli wrote:
> > Now, with respect to the fixed link ports reporting 1000baseKX/Full this is
> > introduced by switching to your patch, it works before and it "breaks"
> > after.
> > 
> > The first part that is a bit weird is that we seem to be calling
> > phylink_generic_validate() twice in a row from the same call site.
> > 
> > For fixed link ports, instead of masking with what the fixed link actually
> > supports, we seem to be using a supported mask which is all 1s which seems a
> > bit excessive for a fixed link.
> > 
> > This is an excerpt with the internal PHY:
> > 
> > [    4.210890] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized):
> > Calling phylink_generic_validate
> > [    4.220063] before phylink_get_linkmodes: 0000000,00000000,00010fc0
> > [    4.226357] phylink_get_linkmodes: caps: 0xffffffff mac_capabilities:
> > 0xff
> > [    4.233258] after phylink_get_linkmodes: c000018,00000200,00036fff
> > [    4.239463] before anding supported with mask: 0000000,00000000,000062ff
> > [    4.246189] after anding supported with mask: 0000000,00000000,000062ff
> > [    4.252829] before anding advertising with mask:
> > c000018,00000200,00036fff
> > [    4.259729] after anding advertising with mask: c000018,00000200,00036fff
> > [    4.266546] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized): PHY
> > [f0b403c0.mdio--1:05] driver [Broadcom BCM7445] (irq=POLL)
> > 
> > and this is what a fixed link port looks like:
> > 
> > [    4.430765] brcm-sf2 f0b00000.ethernet_switch rgmii_2 (uninitialized):
> > Calling phylink_generic_validate
> > [    4.440205] before phylink_get_linkmodes: 0000000,00000000,00010fc0
> > [    4.446500] phylink_get_linkmodes: caps: 0xff mac_capabilities: 0xff
> > [    4.452880] after phylink_get_linkmodes: c000018,00000200,00036fff
> > [    4.459085] before anding supported with mask: fffffff,ffffffff,ffffffff
> > [    4.465811] after anding supported with mask: c000018,00000200,00036fff
> > [    4.472450] before anding advertising with mask:
> > c000018,00000200,00036fff
> > [    4.479349] after anding advertising with mask: c000018,00000200,00036fff
> > 
> > or maybe the problem is with phylink_get_ksettings... ran out of time
> > tonight to look further into it.
> 
> It will be:
> 
>         s = phy_lookup_setting(pl->link_config.speed, pl->link_config.duplex,
>                                pl->supported, true);
>         linkmode_zero(pl->supported);
>         phylink_set(pl->supported, MII);
>         phylink_set(pl->supported, Pause);
>         phylink_set(pl->supported, Asym_Pause);
>         phylink_set(pl->supported, Autoneg);
>         if (s) {
>                 __set_bit(s->bit, pl->supported);
>                 __set_bit(s->bit, pl->link_config.lp_advertising);
> 
> Since 1000baseKX_Full is set in the supported mask, phy_lookup_setting()
> returns the first entry it finds in the supported table:
> 
>         /* 1G */
>         PHY_SETTING(   1000, FULL,   1000baseKX_Full            ),
>         PHY_SETTING(   1000, FULL,   1000baseT_Full             ),
>         PHY_SETTING(   1000, HALF,   1000baseT_Half             ),
>         PHY_SETTING(   1000, FULL,   1000baseT1_Full            ),
>         PHY_SETTING(   1000, FULL,   1000baseX_Full             ),
> 
> Consequently, 1000baseKX_Full is preferred over 1000baseT_Full.
> 
> Fixed links don't specify their underlying technology, only the speed
> and duplex, so going from speed and duplex to an ethtool link mode is
> not easy. I suppose we could drop 1000baseKX_Full from the supported
> bitmap in phylink_parse_fixedlink() before the first phylink_validate()
> call. Alternatively, the table could be re-ordered. It was supposed to
> be grouped by speed and sorted in descending match priority as specified
> by the comment above the table. Does it really make sense that
> 1000baseKX_Full is supposed to be preferred over all the other 1G
> speeds? I suppose that's a question for Tom Lendacky
> <thomas.lendacky@amd.com>, who introduced this in 3e7077067e80
> ("phy: Expand phy speed/duplex settings array") back in 2014.

Here's a patch for one of my suggestions above. Tom, I'd appreciate
if you could look at this please. Thanks.

8<===
From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
Subject: [PATCH net-next] net: phy: prefer 1000baseT over 1000baseKX

The PHY settings table is supposed to be sorted by descending match
priority - in other words, earlier entries are preferred over later
entries.

The order of 1000baseKX/Full and 1000baseT/Full is such that we
prefer 1000baseKX/Full over 1000baseT/Full, but 1000baseKX/Full is
a lot rarer than 1000baseT/Full, and thus is much less likely to
be preferred.

This causes phylink problems - it means a fixed link specifying a
speed of 1G and full duplex gets an ethtool linkmode of 1000baseKX/Full
rather than 1000baseT/Full as would be expected - and since we offer
userspace a software emulation of a conventional copper PHY, we want
to offer copper modes in preference to anything else. However, we do
still want to allow the rarer modes as well.

Hence, let's reorder these two modes to prefer copper.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 2870c33b8975..271fc01f7f7f 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -162,11 +162,11 @@ static const struct phy_setting settings[] = {
 	PHY_SETTING(   2500, FULL,   2500baseT_Full		),
 	PHY_SETTING(   2500, FULL,   2500baseX_Full		),
 	/* 1G */
-	PHY_SETTING(   1000, FULL,   1000baseKX_Full		),
 	PHY_SETTING(   1000, FULL,   1000baseT_Full		),
 	PHY_SETTING(   1000, HALF,   1000baseT_Half		),
 	PHY_SETTING(   1000, FULL,   1000baseT1_Full		),
 	PHY_SETTING(   1000, FULL,   1000baseX_Full		),
+	PHY_SETTING(   1000, FULL,   1000baseKX_Full		),
 	/* 100M */
 	PHY_SETTING(    100, FULL,    100baseT_Full		),
 	PHY_SETTING(    100, FULL,    100baseT1_Full		),
-- 
2.30.2

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

Re: [PATCH RFC net-next 05/12] net: dsa: bcm_sf2: convert to phylink_generic_validate()

[Andrew Lunn]

> The order of 1000baseKX/Full and 1000baseT/Full is such that we
> prefer 1000baseKX/Full over 1000baseT/Full, but 1000baseKX/Full is
> a lot rarer than 1000baseT/Full, and thus is much less likely to
> be preferred.
> 
> This causes phylink problems - it means a fixed link specifying a
> speed of 1G and full duplex gets an ethtool linkmode of 1000baseKX/Full
> rather than 1000baseT/Full as would be expected - and since we offer
> userspace a software emulation of a conventional copper PHY, we want
> to offer copper modes in preference to anything else. However, we do
> still want to allow the rarer modes as well.

2.5G already places T before X, so it makes it more uniform with that.

For 10G, T comes last. Maybe we should also consider this case?  Do we
see more 10G copper than fibre/backplane?

    Andrew

Re: [PATCH RFC net-next 05/12] net: dsa: bcm_sf2: convert to phylink_generic_validate()

[Russell King (Oracle)]

On Sat, Dec 04, 2021 at 04:01:32PM +0100, Andrew Lunn wrote:
> > The order of 1000baseKX/Full and 1000baseT/Full is such that we
> > prefer 1000baseKX/Full over 1000baseT/Full, but 1000baseKX/Full is
> > a lot rarer than 1000baseT/Full, and thus is much less likely to
> > be preferred.
> > 
> > This causes phylink problems - it means a fixed link specifying a
> > speed of 1G and full duplex gets an ethtool linkmode of 1000baseKX/Full
> > rather than 1000baseT/Full as would be expected - and since we offer
> > userspace a software emulation of a conventional copper PHY, we want
> > to offer copper modes in preference to anything else. However, we do
> > still want to allow the rarer modes as well.
> 
> 2.5G already places T before X, so it makes it more uniform with that.
> 
> For 10G, T comes last. Maybe we should also consider this case?  Do we
> see more 10G copper than fibre/backplane?

For a fixed link, I'm not sure - but given that speeds higher than 1G
can't be emulated by the C22 PHY emulation, does it really matter?

Looking through the DTs that we have in the kernel tree, we have some
fixed-links at 10G speed, and no one has complained, so I'd suggest
leaving sleeping dogs and all that.

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

Re: [PATCH RFC net-next 05/12] net: dsa: bcm_sf2: convert to phylink_generic_validate()

[Tom Lendacky]

On 12/4/21 8:52 AM, Russell King (Oracle) wrote:
> On Sat, Dec 04, 2021 at 08:59:12AM +0000, Russell King (Oracle) wrote:
>> On Fri, Dec 03, 2021 at 08:18:22PM -0800, Florian Fainelli wrote:

> 
> Here's a patch for one of my suggestions above. Tom, I'd appreciate
> if you could look at this please. Thanks.

I think it's fine to move the setting down. The driver that I was working 
on at the time only advertised 1000baseKX_Full for 1gpbs (which wasn't in 
the array and why I added it), so I don't see an issue with moving it down.

A quick build and test showed that I was able to successfully connect at 1 
gbps. I didn't dive any deeper than that.

Thanks,
Tom

> 
> 8<===
> From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
> Subject: [PATCH net-next] net: phy: prefer 1000baseT over 1000baseKX
> 
> The PHY settings table is supposed to be sorted by descending match
> priority - in other words, earlier entries are preferred over later
> entries.
> 
> The order of 1000baseKX/Full and 1000baseT/Full is such that we
> prefer 1000baseKX/Full over 1000baseT/Full, but 1000baseKX/Full is
> a lot rarer than 1000baseT/Full, and thus is much less likely to
> be preferred.
> 
> This causes phylink problems - it means a fixed link specifying a
> speed of 1G and full duplex gets an ethtool linkmode of 1000baseKX/Full
> rather than 1000baseT/Full as would be expected - and since we offer
> userspace a software emulation of a conventional copper PHY, we want
> to offer copper modes in preference to anything else. However, we do
> still want to allow the rarer modes as well.
> 
> Hence, let's reorder these two modes to prefer copper.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>   drivers/net/phy/phy-core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
> index 2870c33b8975..271fc01f7f7f 100644
> --- a/drivers/net/phy/phy-core.c
> +++ b/drivers/net/phy/phy-core.c
> @@ -162,11 +162,11 @@ static const struct phy_setting settings[] = {
>   	PHY_SETTING(   2500, FULL,   2500baseT_Full		),
>   	PHY_SETTING(   2500, FULL,   2500baseX_Full		),
>   	/* 1G */
> -	PHY_SETTING(   1000, FULL,   1000baseKX_Full		),
>   	PHY_SETTING(   1000, FULL,   1000baseT_Full		),
>   	PHY_SETTING(   1000, HALF,   1000baseT_Half		),
>   	PHY_SETTING(   1000, FULL,   1000baseT1_Full		),
>   	PHY_SETTING(   1000, FULL,   1000baseX_Full		),
> +	PHY_SETTING(   1000, FULL,   1000baseKX_Full		),
>   	/* 100M */
>   	PHY_SETTING(    100, FULL,    100baseT_Full		),
>   	PHY_SETTING(    100, FULL,    100baseT1_Full		),
> 

Re: [PATCH RFC net-next 05/12] net: dsa: bcm_sf2: convert to phylink_generic_validate()

[Russell King (Oracle)]

On Mon, Dec 06, 2021 at 09:59:46AM -0600, Tom Lendacky wrote:
> On 12/4/21 8:52 AM, Russell King (Oracle) wrote:
> > On Sat, Dec 04, 2021 at 08:59:12AM +0000, Russell King (Oracle) wrote:
> > > On Fri, Dec 03, 2021 at 08:18:22PM -0800, Florian Fainelli wrote:
> 
> > 
> > Here's a patch for one of my suggestions above. Tom, I'd appreciate
> > if you could look at this please. Thanks.
> 
> I think it's fine to move the setting down. The driver that I was working on
> at the time only advertised 1000baseKX_Full for 1gpbs (which wasn't in the
> array and why I added it), so I don't see an issue with moving it down.
> 
> A quick build and test showed that I was able to successfully connect at 1
> gbps. I didn't dive any deeper than that.

Thanks Tom! Can I use that to add your tested-by to this change please?

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

Re: [PATCH RFC net-next 05/12] net: dsa: bcm_sf2: convert to phylink_generic_validate()

[Tom Lendacky]

On 12/6/21 10:13 AM, Russell King (Oracle) wrote:
> On Mon, Dec 06, 2021 at 09:59:46AM -0600, Tom Lendacky wrote:
>> On 12/4/21 8:52 AM, Russell King (Oracle) wrote:
>>> On Sat, Dec 04, 2021 at 08:59:12AM +0000, Russell King (Oracle) wrote:
>>>> On Fri, Dec 03, 2021 at 08:18:22PM -0800, Florian Fainelli wrote:
>>
>>>
>>> Here's a patch for one of my suggestions above. Tom, I'd appreciate
>>> if you could look at this please. Thanks.
>>
>> I think it's fine to move the setting down. The driver that I was working on
>> at the time only advertised 1000baseKX_Full for 1gpbs (which wasn't in the
>> array and why I added it), so I don't see an issue with moving it down.
>>
>> A quick build and test showed that I was able to successfully connect at 1
>> gbps. I didn't dive any deeper than that.
> 
> Thanks Tom! Can I use that to add your tested-by to this change please?

Certainly.

Tested-by: Tom Lendacky <thomas.lendacky@amd.com>

> 

Re: [PATCH RFC net-next 05/12] net: dsa: bcm_sf2: convert to phylink_generic_validate()

[Russell King (Oracle)]

On Mon, Dec 06, 2021 at 10:36:24AM -0600, Tom Lendacky wrote:
> On 12/6/21 10:13 AM, Russell King (Oracle) wrote:
> > On Mon, Dec 06, 2021 at 09:59:46AM -0600, Tom Lendacky wrote:
> > > On 12/4/21 8:52 AM, Russell King (Oracle) wrote:
> > > > On Sat, Dec 04, 2021 at 08:59:12AM +0000, Russell King (Oracle) wrote:
> > > > > On Fri, Dec 03, 2021 at 08:18:22PM -0800, Florian Fainelli wrote:
> > > 
> > > > 
> > > > Here's a patch for one of my suggestions above. Tom, I'd appreciate
> > > > if you could look at this please. Thanks.
> > > 
> > > I think it's fine to move the setting down. The driver that I was working on
> > > at the time only advertised 1000baseKX_Full for 1gpbs (which wasn't in the
> > > array and why I added it), so I don't see an issue with moving it down.
> > > 
> > > A quick build and test showed that I was able to successfully connect at 1
> > > gbps. I didn't dive any deeper than that.
> > 
> > Thanks Tom! Can I use that to add your tested-by to this change please?
> 
> Certainly.
> 
> Tested-by: Tom Lendacky <thomas.lendacky@amd.com>

Thanks!

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

Re: [PATCH RFC net-next 05/12] net: dsa: bcm_sf2: convert to phylink_generic_validate()

[Florian Fainelli]

On 12/4/21 6:52 AM, Russell King (Oracle) wrote:
> On Sat, Dec 04, 2021 at 08:59:12AM +0000, Russell King (Oracle) wrote:
>> On Fri, Dec 03, 2021 at 08:18:22PM -0800, Florian Fainelli wrote:
>>> Now, with respect to the fixed link ports reporting 1000baseKX/Full this is
>>> introduced by switching to your patch, it works before and it "breaks"
>>> after.
>>>
>>> The first part that is a bit weird is that we seem to be calling
>>> phylink_generic_validate() twice in a row from the same call site.
>>>
>>> For fixed link ports, instead of masking with what the fixed link actually
>>> supports, we seem to be using a supported mask which is all 1s which seems a
>>> bit excessive for a fixed link.
>>>
>>> This is an excerpt with the internal PHY:
>>>
>>> [    4.210890] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized):
>>> Calling phylink_generic_validate
>>> [    4.220063] before phylink_get_linkmodes: 0000000,00000000,00010fc0
>>> [    4.226357] phylink_get_linkmodes: caps: 0xffffffff mac_capabilities:
>>> 0xff
>>> [    4.233258] after phylink_get_linkmodes: c000018,00000200,00036fff
>>> [    4.239463] before anding supported with mask: 0000000,00000000,000062ff
>>> [    4.246189] after anding supported with mask: 0000000,00000000,000062ff
>>> [    4.252829] before anding advertising with mask:
>>> c000018,00000200,00036fff
>>> [    4.259729] after anding advertising with mask: c000018,00000200,00036fff
>>> [    4.266546] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized): PHY
>>> [f0b403c0.mdio--1:05] driver [Broadcom BCM7445] (irq=POLL)
>>>
>>> and this is what a fixed link port looks like:
>>>
>>> [    4.430765] brcm-sf2 f0b00000.ethernet_switch rgmii_2 (uninitialized):
>>> Calling phylink_generic_validate
>>> [    4.440205] before phylink_get_linkmodes: 0000000,00000000,00010fc0
>>> [    4.446500] phylink_get_linkmodes: caps: 0xff mac_capabilities: 0xff
>>> [    4.452880] after phylink_get_linkmodes: c000018,00000200,00036fff
>>> [    4.459085] before anding supported with mask: fffffff,ffffffff,ffffffff
>>> [    4.465811] after anding supported with mask: c000018,00000200,00036fff
>>> [    4.472450] before anding advertising with mask:
>>> c000018,00000200,00036fff
>>> [    4.479349] after anding advertising with mask: c000018,00000200,00036fff
>>>
>>> or maybe the problem is with phylink_get_ksettings... ran out of time
>>> tonight to look further into it.
>>
>> It will be:
>>
>>         s = phy_lookup_setting(pl->link_config.speed, pl->link_config.duplex,
>>                                pl->supported, true);
>>         linkmode_zero(pl->supported);
>>         phylink_set(pl->supported, MII);
>>         phylink_set(pl->supported, Pause);
>>         phylink_set(pl->supported, Asym_Pause);
>>         phylink_set(pl->supported, Autoneg);
>>         if (s) {
>>                 __set_bit(s->bit, pl->supported);
>>                 __set_bit(s->bit, pl->link_config.lp_advertising);
>>
>> Since 1000baseKX_Full is set in the supported mask, phy_lookup_setting()
>> returns the first entry it finds in the supported table:
>>
>>         /* 1G */
>>         PHY_SETTING(   1000, FULL,   1000baseKX_Full            ),
>>         PHY_SETTING(   1000, FULL,   1000baseT_Full             ),
>>         PHY_SETTING(   1000, HALF,   1000baseT_Half             ),
>>         PHY_SETTING(   1000, FULL,   1000baseT1_Full            ),
>>         PHY_SETTING(   1000, FULL,   1000baseX_Full             ),
>>
>> Consequently, 1000baseKX_Full is preferred over 1000baseT_Full.
>>
>> Fixed links don't specify their underlying technology, only the speed
>> and duplex, so going from speed and duplex to an ethtool link mode is
>> not easy. I suppose we could drop 1000baseKX_Full from the supported
>> bitmap in phylink_parse_fixedlink() before the first phylink_validate()
>> call. Alternatively, the table could be re-ordered. It was supposed to
>> be grouped by speed and sorted in descending match priority as specified
>> by the comment above the table. Does it really make sense that
>> 1000baseKX_Full is supposed to be preferred over all the other 1G
>> speeds? I suppose that's a question for Tom Lendacky
>> <thomas.lendacky@amd.com>, who introduced this in 3e7077067e80
>> ("phy: Expand phy speed/duplex settings array") back in 2014.
> 
> Here's a patch for one of my suggestions above. Tom, I'd appreciate
> if you could look at this please. Thanks.

I don't have objections on the patch per-se, but I am still wary that
this is going to break another driver in terms of what its fixed link
ports are supposed to report, so maybe the generic validation approach
needs to be provided some additional hints as to what port link modes
are supported, or rather, not supported.

So I would suggest we have bcm_sf2 continue to implement
ds->ops->validate which does call phylink_generic_validate() but also
prunes unsupported link modes for its fixed link ports, what do you think?
-- 
Florian

Re: [PATCH RFC net-next 05/12] net: dsa: bcm_sf2: convert to phylink_generic_validate()

[Russell King (Oracle)]

On Mon, Dec 06, 2021 at 09:06:53AM -0800, Florian Fainelli wrote:
> On 12/4/21 6:52 AM, Russell King (Oracle) wrote:
> > On Sat, Dec 04, 2021 at 08:59:12AM +0000, Russell King (Oracle) wrote:
> >> It will be:
> >>
> >>         s = phy_lookup_setting(pl->link_config.speed, pl->link_config.duplex,
> >>                                pl->supported, true);
> >>         linkmode_zero(pl->supported);
> >>         phylink_set(pl->supported, MII);
> >>         phylink_set(pl->supported, Pause);
> >>         phylink_set(pl->supported, Asym_Pause);
> >>         phylink_set(pl->supported, Autoneg);
> >>         if (s) {
> >>                 __set_bit(s->bit, pl->supported);
> >>                 __set_bit(s->bit, pl->link_config.lp_advertising);
> >>
> >> Since 1000baseKX_Full is set in the supported mask, phy_lookup_setting()
> >> returns the first entry it finds in the supported table:
> >>
> >>         /* 1G */
> >>         PHY_SETTING(   1000, FULL,   1000baseKX_Full            ),
> >>         PHY_SETTING(   1000, FULL,   1000baseT_Full             ),
> >>         PHY_SETTING(   1000, HALF,   1000baseT_Half             ),
> >>         PHY_SETTING(   1000, FULL,   1000baseT1_Full            ),
> >>         PHY_SETTING(   1000, FULL,   1000baseX_Full             ),
> >>
> >> Consequently, 1000baseKX_Full is preferred over 1000baseT_Full.
> >>
> >> Fixed links don't specify their underlying technology, only the speed
> >> and duplex, so going from speed and duplex to an ethtool link mode is
> >> not easy. I suppose we could drop 1000baseKX_Full from the supported
> >> bitmap in phylink_parse_fixedlink() before the first phylink_validate()
> >> call. Alternatively, the table could be re-ordered. It was supposed to
> >> be grouped by speed and sorted in descending match priority as specified
> >> by the comment above the table. Does it really make sense that
> >> 1000baseKX_Full is supposed to be preferred over all the other 1G
> >> speeds? I suppose that's a question for Tom Lendacky
> >> <thomas.lendacky@amd.com>, who introduced this in 3e7077067e80
> >> ("phy: Expand phy speed/duplex settings array") back in 2014.
> > 
> > Here's a patch for one of my suggestions above. Tom, I'd appreciate
> > if you could look at this please. Thanks.
> 
> I don't have objections on the patch per-se, but I am still wary that
> this is going to break another driver in terms of what its fixed link
> ports are supposed to report, so maybe the generic validation approach
> needs to be provided some additional hints as to what port link modes
> are supported, or rather, not supported.

Honestly, I'm not sure I'd call this a breakage, when we haven't really
cared that the link modes for fixed links reflect the underlying link
in the past.

Fixed-links started out (and still are for the vast majority of
drivers that use phylib for their fixed links) as being an emulation of
a copper PHY. Thus, they end up with baseT linkmodes no matter what the
actual underlying link is.

Phylink provides a fixed-link implementation that is supposed to be
broadly similar to the original phylib based implementation without
using the emulation of a PHY directly, allowing it greater flexibility
in the link speeds that it can support.

It was never intended for a MAC driver to restrict the linkmode
technologies - and doing so goes completely against the phylink design
principle and also the phylink documentation. Restricting the linkmodes
based on technologoy encodes information about the setup of the world
outside of the MAC block into the MAC driver. This is actually a
problem that needs to be sorted.

Consider a driver which decides to restrict linkmodes based on
technology, such as the Marvell DSA which presently allows only
1000baseX and 1000baseT linkmodes (at the time, there was no 1000baseKX
ethtool linkmode.) Now someone comes along with a board design that
interfaces one of the Marvell DSA ports to a PHY that supports
1000baseKX. They add 1000baseKX to the linkmodes that Marvell DSA now
lets through.

Any fixed link on Marvell DSA now ends up reporting 1000baseKX instead
of 1000baseT as it used to before, even if the underlying link was
actually 1000baseX. (This is why I say, we don't actually care what
technology has historically been reported - it demonstrably is very
much the case.)

Now, going on further, there is the argument that we should be
reporting baseT link modes for fixed links up to 1G speeds, since fixed
links provide emulation of a copper PHY. For non-phylink, that copper
PHY emulation was to allow phylib to be re-used for fixed-links, and
thus you only ever get baseT linkmodes reported (and phylib-based fixed
links only go up to 1G speeds.) If we don't fix this, then converting
to phylink results in 1000baseKX being selected if one is compliant
with the above.

Then there's the argument that we have never really cared for the
actual technology of a fixed link. For example, on the VF610 ZII rev B
board, which uses the Freescale FEC driver (using phylib, not phylink),
the port used to talk to the DSA switches reports:

Settings for eth1:
        Supported ports: [ TP MII ]
        Supported link modes:   100baseT/Full
        Supported pause frame use: Symmetric Receive-only
        Supports auto-negotiation: Yes
        Supported FEC modes: Not reported
        Advertised link modes:  100baseT/Full
...

Even though there is no twisted-pair copper link in sight - it's
actually a RMII link, but we don't have any ethtool link modes to
describe that.

This was also true for Clearfog with its DSA switch connected via
1000BASE-X, except there we used to get 1000baseT/Full with the phylib-
based fixed link prior to phylink.

Then there's fixed links that use 10000/Full - these will end up being
10000baseCR/Full... even though they are not 10000baseCR - and again,
we don't actually have an ethtool linkmode that reports what they are.

So, really, the whole "technology" thing for fixed links is very vague
and we have never actually cared if it actually reports the link.

If MAC drivers restrict the technology to make fixed links "work" as
they expect, they are restricting the technologies that the connection
as a whole can support when not operating in fixed-link mode, and that
is plain wrong.

> So I would suggest we have bcm_sf2 continue to implement
> ds->ops->validate which does call phylink_generic_validate() but also
> prunes unsupported link modes for its fixed link ports, what do you
> think?

I'm not keen as I want to kill off .validate entirely, and not have its
legacy hanging around making future development (e.g. to properly
support rate-adaption) more difficult. I've been looking at this
recently and I just can't come up with a clean way to have a MAC and
PCS split where either the PCS or PHY do rate adaption without the MAC
being fully aware that is going on in its validate() method.

So, rather than keeping this around, if there is a need to specify the
technology, I would rather introduce another field into phylink_config,
misnamed, as "mac_techologies" which indicates whether we wish to
restrict to baseT/baseX/baseKX etc and this _only_ gets used for
fixed-links. It's misnamed because the MAC should have nothing to do
with this, and it's a hack to allow people to have their preferred
ethtool linkmodes.

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

Re: [PATCH RFC net-next 05/12] net: dsa: bcm_sf2: convert to phylink_generic_validate()

[Russell King (Oracle)]

On Mon, Dec 06, 2021 at 07:26:48PM +0000, Russell King (Oracle) wrote:
> On Mon, Dec 06, 2021 at 09:06:53AM -0800, Florian Fainelli wrote:
> > On 12/4/21 6:52 AM, Russell King (Oracle) wrote:
> > > On Sat, Dec 04, 2021 at 08:59:12AM +0000, Russell King (Oracle) wrote:
> > >> It will be:
> > >>
> > >>         s = phy_lookup_setting(pl->link_config.speed, pl->link_config.duplex,
> > >>                                pl->supported, true);
> > >>         linkmode_zero(pl->supported);
> > >>         phylink_set(pl->supported, MII);
> > >>         phylink_set(pl->supported, Pause);
> > >>         phylink_set(pl->supported, Asym_Pause);
> > >>         phylink_set(pl->supported, Autoneg);
> > >>         if (s) {
> > >>                 __set_bit(s->bit, pl->supported);
> > >>                 __set_bit(s->bit, pl->link_config.lp_advertising);
> > >>
> > >> Since 1000baseKX_Full is set in the supported mask, phy_lookup_setting()
> > >> returns the first entry it finds in the supported table:
> > >>
> > >>         /* 1G */
> > >>         PHY_SETTING(   1000, FULL,   1000baseKX_Full            ),
> > >>         PHY_SETTING(   1000, FULL,   1000baseT_Full             ),
> > >>         PHY_SETTING(   1000, HALF,   1000baseT_Half             ),
> > >>         PHY_SETTING(   1000, FULL,   1000baseT1_Full            ),
> > >>         PHY_SETTING(   1000, FULL,   1000baseX_Full             ),
> > >>
> > >> Consequently, 1000baseKX_Full is preferred over 1000baseT_Full.
> > >>
> > >> Fixed links don't specify their underlying technology, only the speed
> > >> and duplex, so going from speed and duplex to an ethtool link mode is
> > >> not easy. I suppose we could drop 1000baseKX_Full from the supported
> > >> bitmap in phylink_parse_fixedlink() before the first phylink_validate()
> > >> call. Alternatively, the table could be re-ordered. It was supposed to
> > >> be grouped by speed and sorted in descending match priority as specified
> > >> by the comment above the table. Does it really make sense that
> > >> 1000baseKX_Full is supposed to be preferred over all the other 1G
> > >> speeds? I suppose that's a question for Tom Lendacky
> > >> <thomas.lendacky@amd.com>, who introduced this in 3e7077067e80
> > >> ("phy: Expand phy speed/duplex settings array") back in 2014.
> > > 
> > > Here's a patch for one of my suggestions above. Tom, I'd appreciate
> > > if you could look at this please. Thanks.
> > 
> > I don't have objections on the patch per-se, but I am still wary that
> > this is going to break another driver in terms of what its fixed link
> > ports are supposed to report, so maybe the generic validation approach
> > needs to be provided some additional hints as to what port link modes
> > are supported, or rather, not supported.
> 
> Honestly, I'm not sure I'd call this a breakage, when we haven't really
> cared that the link modes for fixed links reflect the underlying link
> in the past.
> 
> Fixed-links started out (and still are for the vast majority of
> drivers that use phylib for their fixed links) as being an emulation of
> a copper PHY. Thus, they end up with baseT linkmodes no matter what the
> actual underlying link is.
> 
> Phylink provides a fixed-link implementation that is supposed to be
> broadly similar to the original phylib based implementation without
> using the emulation of a PHY directly, allowing it greater flexibility
> in the link speeds that it can support.
> 
> It was never intended for a MAC driver to restrict the linkmode
> technologies - and doing so goes completely against the phylink design
> principle and also the phylink documentation. Restricting the linkmodes
> based on technologoy encodes information about the setup of the world
> outside of the MAC block into the MAC driver. This is actually a
> problem that needs to be sorted.
> 
> Consider a driver which decides to restrict linkmodes based on
> technology, such as the Marvell DSA which presently allows only
> 1000baseX and 1000baseT linkmodes (at the time, there was no 1000baseKX
> ethtool linkmode.) Now someone comes along with a board design that
> interfaces one of the Marvell DSA ports to a PHY that supports
> 1000baseKX. They add 1000baseKX to the linkmodes that Marvell DSA now
> lets through.
> 
> Any fixed link on Marvell DSA now ends up reporting 1000baseKX instead
> of 1000baseT as it used to before, even if the underlying link was
> actually 1000baseX. (This is why I say, we don't actually care what
> technology has historically been reported - it demonstrably is very
> much the case.)
> 
> Now, going on further, there is the argument that we should be
> reporting baseT link modes for fixed links up to 1G speeds, since fixed
> links provide emulation of a copper PHY. For non-phylink, that copper
> PHY emulation was to allow phylib to be re-used for fixed-links, and
> thus you only ever get baseT linkmodes reported (and phylib-based fixed
> links only go up to 1G speeds.) If we don't fix this, then converting
> to phylink results in 1000baseKX being selected if one is compliant
> with the above.
> 
> Then there's the argument that we have never really cared for the
> actual technology of a fixed link. For example, on the VF610 ZII rev B
> board, which uses the Freescale FEC driver (using phylib, not phylink),
> the port used to talk to the DSA switches reports:
> 
> Settings for eth1:
>         Supported ports: [ TP MII ]
>         Supported link modes:   100baseT/Full
>         Supported pause frame use: Symmetric Receive-only
>         Supports auto-negotiation: Yes
>         Supported FEC modes: Not reported
>         Advertised link modes:  100baseT/Full
> ...
> 
> Even though there is no twisted-pair copper link in sight - it's
> actually a RMII link, but we don't have any ethtool link modes to
> describe that.
> 
> This was also true for Clearfog with its DSA switch connected via
> 1000BASE-X, except there we used to get 1000baseT/Full with the phylib-
> based fixed link prior to phylink.
> 
> Then there's fixed links that use 10000/Full - these will end up being
> 10000baseCR/Full... even though they are not 10000baseCR - and again,
> we don't actually have an ethtool linkmode that reports what they are.
> 
> So, really, the whole "technology" thing for fixed links is very vague
> and we have never actually cared if it actually reports the link.
> 
> If MAC drivers restrict the technology to make fixed links "work" as
> they expect, they are restricting the technologies that the connection
> as a whole can support when not operating in fixed-link mode, and that
> is plain wrong.
> 
> > So I would suggest we have bcm_sf2 continue to implement
> > ds->ops->validate which does call phylink_generic_validate() but also
> > prunes unsupported link modes for its fixed link ports, what do you
> > think?
> 
> I'm not keen as I want to kill off .validate entirely, and not have its
> legacy hanging around making future development (e.g. to properly
> support rate-adaption) more difficult. I've been looking at this
> recently and I just can't come up with a clean way to have a MAC and
> PCS split where either the PCS or PHY do rate adaption without the MAC
> being fully aware that is going on in its validate() method.
> 
> So, rather than keeping this around, if there is a need to specify the
> technology, I would rather introduce another field into phylink_config,
> misnamed, as "mac_techologies" which indicates whether we wish to
> restrict to baseT/baseX/baseKX etc and this _only_ gets used for
> fixed-links. It's misnamed because the MAC should have nothing to do
> with this, and it's a hack to allow people to have their preferred
> ethtool linkmodes.

... and this is not just a problem for bcm_sf2. I was about to send
the Marvell DSA conversion to phylink_generic_validate(), and the
fixed-link I have for lan6 on Clearfog shows the same change to
1000baseKX. The same is going to be true across the board.

I've thought about this more. There is precedent for changing the
order - we've done it already with 1000baseX vs 1000baseT... silently.
See 3c6b59d6f07c ("net: phy: Add more link modes to the settings
table") - it was originally:

ETHTOOL_LINK_MODE_2500baseX_Full_BIT
ETHTOOL_LINK_MODE_1000baseKX_Full_BIT
ETHTOOL_LINK_MODE_1000baseX_Full_BIT
ETHTOOL_LINK_MODE_1000baseT_Full_BIT
ETHTOOL_LINK_MODE_1000baseT_Half_BIT

and became:

ETHTOOL_LINK_MODE_2500baseT_Full_BIT
ETHTOOL_LINK_MODE_2500baseX_Full_BIT
ETHTOOL_LINK_MODE_1000baseKX_Full_BIT
ETHTOOL_LINK_MODE_1000baseT_Full_BIT
ETHTOOL_LINK_MODE_1000baseT_Half_BIT
ETHTOOL_LINK_MODE_1000baseX_Full_BIT

This would have made a fixed-link on a Marvell DSA switch to change
from reporting 1000baseX/Full to reporting 1000baseT/Full... and no one
noticed that change.

So, I doubt anyone will notice if we move 1000baseKX_Full down below
the 1000baseT entries. I suspect also that there are very few who even
care what link modes are reported for a fixed-link.

However, as I said above, we could introduce something like a bitmask
of media technologies to allow the preference for fixed-links to be
specified. That said, if fixed-links were something new today, I think
we'd be saying about having that come from DT/firmware on the grounds
that "DT describes the hardware" and this is very much a hardware
property!

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

[PATCH RFC net-next 06/12] net: dsa: hellcreek: convert to phylink_generic_validate()

[Russell King (Oracle)]

Populate the supported interfaces and MAC capabilities for the
hellcreek DSA switch and remove the old validate implementation to
allow DSA to use phylink_generic_validate() for this switch driver.

The switch actually only supports MII and RGMII, but as phylib defaults
to GMII, we need to include this interface mode to keep existing DT
working.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/hirschmann/hellcreek.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c
index 4e0b53d94b52..86839b43011b 100644
--- a/drivers/net/dsa/hirschmann/hellcreek.c
+++ b/drivers/net/dsa/hirschmann/hellcreek.c
@@ -1384,14 +1384,19 @@ static void hellcreek_teardown(struct dsa_switch *ds)
 	dsa_devlink_resources_unregister(ds);
 }
 
-static void hellcreek_phylink_validate(struct dsa_switch *ds, int port,
-				       unsigned long *supported,
-				       struct phylink_link_state *state)
+static void hellcreek_phylink_get_caps(struct dsa_switch *ds, int port,
+				       struct phylink_config *config)
 {
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
 	struct hellcreek *hellcreek = ds->priv;
 
-	dev_dbg(hellcreek->dev, "Phylink validate for port %d\n", port);
+	__set_bit(PHY_INTERFACE_MODE_MII, config->supported_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_RGMII, config->supported_interfaces);
+
+	/* Include GMII - the hardware does not support this interface
+	 * mode, but it's the default interface mode for phylib, so we
+	 * need it for compatibility with existing DT.
+	 */
+	__set_bit(PHY_INTERFACE_MODE_GMII, config->supported_interfaces);
 
 	/* The MAC settings are a hardware configuration option and cannot be
 	 * changed at run time or by strapping. Therefore the attached PHYs
@@ -1399,12 +1404,9 @@ static void hellcreek_phylink_validate(struct dsa_switch *ds, int port,
 	 * by the hardware.
 	 */
 	if (hellcreek->pdata->is_100_mbits)
-		phylink_set(mask, 100baseT_Full);
+		config->mac_capabilities = MAC_100FD;
 	else
-		phylink_set(mask, 1000baseT_Full);
-
-	linkmode_and(supported, supported, mask);
-	linkmode_and(state->advertising, state->advertising, mask);
+		config->mac_capabilities = MAC_1000FD;
 }
 
 static int
@@ -1755,7 +1757,7 @@ static const struct dsa_switch_ops hellcreek_ds_ops = {
 	.get_strings	       = hellcreek_get_strings,
 	.get_tag_protocol      = hellcreek_get_tag_protocol,
 	.get_ts_info	       = hellcreek_get_ts_info,
-	.phylink_validate      = hellcreek_phylink_validate,
+	.phylink_get_caps      = hellcreek_phylink_get_caps,
 	.port_bridge_flags     = hellcreek_bridge_flags,
 	.port_bridge_join      = hellcreek_port_bridge_join,
 	.port_bridge_leave     = hellcreek_port_bridge_leave,
-- 
2.30.2

Re: [PATCH RFC net-next 06/12] net: dsa: hellcreek: convert to phylink_generic_validate()

[Kurt Kanzenbach]

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

Russell,

On Wed Nov 24 2021, Russell King (Oracle) wrote:
> Populate the supported interfaces and MAC capabilities for the
> hellcreek DSA switch and remove the old validate implementation to
> allow DSA to use phylink_generic_validate() for this switch driver.
>
> The switch actually only supports MII and RGMII, but as phylib defaults
> to GMII, we need to include this interface mode to keep existing DT
> working.
>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Looks good and works.

Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
Tested-by: Kurt Kanzenbach <kurt@linutronix.de>

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

[PATCH RFC net-next 07/12] net: dsa: ksz8795: convert to phylink_generic_validate()

[Russell King (Oracle)]

Populate the supported interfaces and MAC capabilities for the
Microchip KSZ8795 DSA switch and remove the old validate implementation
to allow DSA to use phylink_generic_validate() for this switch driver.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/microchip/ksz8795.c | 45 ++++++++---------------------
 1 file changed, 12 insertions(+), 33 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 43fc3087aeb3..0f1228442d7f 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -1502,27 +1502,22 @@ static int ksz8_setup(struct dsa_switch *ds)
 	return 0;
 }
 
-static void ksz8_validate(struct dsa_switch *ds, int port,
-			  unsigned long *supported,
-			  struct phylink_link_state *state)
+static void ksz8_get_caps(struct dsa_switch *ds, int port,
+			  struct phylink_config *config)
 {
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
 	struct ksz_device *dev = ds->priv;
 
 	if (port == dev->cpu_port) {
-		if (state->interface != PHY_INTERFACE_MODE_RMII &&
-		    state->interface != PHY_INTERFACE_MODE_MII &&
-		    state->interface != PHY_INTERFACE_MODE_NA)
-			goto unsupported;
+		__set_bit(PHY_INTERFACE_MODE_RMII,
+			  config->supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_MII,
+			  config->supported_interfaces);
 	} else {
-		if (state->interface != PHY_INTERFACE_MODE_INTERNAL &&
-		    state->interface != PHY_INTERFACE_MODE_NA)
-			goto unsupported;
+		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
+			  config->supported_interfaces);
 	}
 
-	/* Allow all the expected bits */
-	phylink_set_port_modes(mask);
-	phylink_set(mask, Autoneg);
+	config->mac_capabilities = MAC_10 | MAC_100;
 
 	/* Silicon Errata Sheet (DS80000830A):
 	 * "Port 1 does not respond to received flow control PAUSE frames"
@@ -1530,27 +1525,11 @@ static void ksz8_validate(struct dsa_switch *ds, int port,
 	 * switches.
 	 */
 	if (!ksz_is_ksz88x3(dev) || port)
-		phylink_set(mask, Pause);
+		config->mac_capabilities |= MAC_SYM_PAUSE;
 
 	/* Asym pause is not supported on KSZ8863 and KSZ8873 */
 	if (!ksz_is_ksz88x3(dev))
-		phylink_set(mask, Asym_Pause);
-
-	/* 10M and 100M are only supported */
-	phylink_set(mask, 10baseT_Half);
-	phylink_set(mask, 10baseT_Full);
-	phylink_set(mask, 100baseT_Half);
-	phylink_set(mask, 100baseT_Full);
-
-	linkmode_and(supported, supported, mask);
-	linkmode_and(state->advertising, state->advertising, mask);
-
-	return;
-
-unsupported:
-	linkmode_zero(supported);
-	dev_err(ds->dev, "Unsupported interface: %s, port: %d\n",
-		phy_modes(state->interface), port);
+		config->mac_capabilities |= MAC_ASYM_PAUSE;
 }
 
 static const struct dsa_switch_ops ksz8_switch_ops = {
@@ -1559,7 +1538,7 @@ static const struct dsa_switch_ops ksz8_switch_ops = {
 	.setup			= ksz8_setup,
 	.phy_read		= ksz_phy_read16,
 	.phy_write		= ksz_phy_write16,
-	.phylink_validate	= ksz8_validate,
+	.phylink_get_caps	= ksz8_get_caps,
 	.phylink_mac_link_down	= ksz_mac_link_down,
 	.port_enable		= ksz_enable_port,
 	.get_strings		= ksz8_get_strings,
-- 
2.30.2

[PATCH RFC net-next 08/12] net: dsa: lantiq: convert to phylink_generic_validate()

[Russell King (Oracle)]

Populate the supported interfaces and MAC capabilities for the Lantiq
DSA switches and remove the old validate implementation to allow DSA to
use phylink_generic_validate() for this switch driver.

The exclusion of Gigabit linkmodes for MII, Reverse MII and Reduced MII
links is handled within phylink_generic_validate() in phylink, so there
is no need to make them conditional on the interface mode in the driver.

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

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 7056d98d8177..583af774e1bd 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -1438,114 +1438,70 @@ static int gswip_port_fdb_dump(struct dsa_switch *ds, int port,
 	return 0;
 }
 
-static void gswip_phylink_set_capab(unsigned long *supported,
-				    struct phylink_link_state *state)
-{
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
-
-	/* Allow all the expected bits */
-	phylink_set(mask, Autoneg);
-	phylink_set_port_modes(mask);
-	phylink_set(mask, Pause);
-	phylink_set(mask, Asym_Pause);
-
-	/* With the exclusion of MII, Reverse MII and Reduced MII, we
-	 * support Gigabit, including Half duplex
-	 */
-	if (state->interface != PHY_INTERFACE_MODE_MII &&
-	    state->interface != PHY_INTERFACE_MODE_REVMII &&
-	    state->interface != PHY_INTERFACE_MODE_RMII) {
-		phylink_set(mask, 1000baseT_Full);
-		phylink_set(mask, 1000baseT_Half);
-	}
-
-	phylink_set(mask, 10baseT_Half);
-	phylink_set(mask, 10baseT_Full);
-	phylink_set(mask, 100baseT_Half);
-	phylink_set(mask, 100baseT_Full);
-
-	linkmode_and(supported, supported, mask);
-	linkmode_and(state->advertising, state->advertising, mask);
-}
-
-static void gswip_xrx200_phylink_validate(struct dsa_switch *ds, int port,
-					  unsigned long *supported,
-					  struct phylink_link_state *state)
+static void gswip_xrx200_phylink_get_caps(struct dsa_switch *ds, int port,
+					  struct phylink_config *config)
 {
 	switch (port) {
 	case 0:
 	case 1:
-		if (!phy_interface_mode_is_rgmii(state->interface) &&
-		    state->interface != PHY_INTERFACE_MODE_MII &&
-		    state->interface != PHY_INTERFACE_MODE_REVMII &&
-		    state->interface != PHY_INTERFACE_MODE_RMII)
-			goto unsupported;
+		phy_interface_set_rgmii(config->supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_MII,
+			  config->supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_REVMII,
+			  config->supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_RMII,
+			  config->supported_interfaces);
 		break;
+
 	case 2:
 	case 3:
 	case 4:
-		if (state->interface != PHY_INTERFACE_MODE_INTERNAL)
-			goto unsupported;
+		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
+			  config->supported_interfaces);
 		break;
+
 	case 5:
-		if (!phy_interface_mode_is_rgmii(state->interface) &&
-		    state->interface != PHY_INTERFACE_MODE_INTERNAL)
-			goto unsupported;
+		phy_interface_set_rgmii(config->supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
+			  config->supported_interfaces);
 		break;
-	default:
-		linkmode_zero(supported);
-		dev_err(ds->dev, "Unsupported port: %i\n", port);
-		return;
 	}
 
-	gswip_phylink_set_capab(supported, state);
-
-	return;
-
-unsupported:
-	linkmode_zero(supported);
-	dev_err(ds->dev, "Unsupported interface '%s' for port %d\n",
-		phy_modes(state->interface), port);
+	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
+		MAC_10 | MAC_100 | MAC_1000;
 }
 
-static void gswip_xrx300_phylink_validate(struct dsa_switch *ds, int port,
-					  unsigned long *supported,
-					  struct phylink_link_state *state)
+static void gswip_xrx300_phylink_get_caps(struct dsa_switch *ds, int port,
+					  struct phylink_config *config)
 {
 	switch (port) {
 	case 0:
-		if (!phy_interface_mode_is_rgmii(state->interface) &&
-		    state->interface != PHY_INTERFACE_MODE_GMII &&
-		    state->interface != PHY_INTERFACE_MODE_RMII)
-			goto unsupported;
+		phy_interface_set_rgmii(config->supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_GMII,
+			  config->supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_RMII,
+			  config->supported_interfaces);
 		break;
+
 	case 1:
 	case 2:
 	case 3:
 	case 4:
-		if (state->interface != PHY_INTERFACE_MODE_INTERNAL)
-			goto unsupported;
+		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
+			  config->supported_interfaces);
 		break;
+
 	case 5:
-		if (!phy_interface_mode_is_rgmii(state->interface) &&
-		    state->interface != PHY_INTERFACE_MODE_INTERNAL &&
-		    state->interface != PHY_INTERFACE_MODE_RMII)
-			goto unsupported;
+		phy_interface_set_rgmii(config->supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
+			  config->supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_RMII,
+			  config->supported_interfaces);
 		break;
-	default:
-		linkmode_zero(supported);
-		dev_err(ds->dev, "Unsupported port: %i\n", port);
-		return;
 	}
 
-	gswip_phylink_set_capab(supported, state);
-
-	return;
-
-unsupported:
-	linkmode_zero(supported);
-	dev_err(ds->dev, "Unsupported interface '%s' for port %d\n",
-		phy_modes(state->interface), port);
+	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
+		MAC_10 | MAC_100 | MAC_1000;
 }
 
 static void gswip_port_set_link(struct gswip_priv *priv, int port, bool link)
@@ -1827,7 +1783,7 @@ static const struct dsa_switch_ops gswip_xrx200_switch_ops = {
 	.port_fdb_add		= gswip_port_fdb_add,
 	.port_fdb_del		= gswip_port_fdb_del,
 	.port_fdb_dump		= gswip_port_fdb_dump,
-	.phylink_validate	= gswip_xrx200_phylink_validate,
+	.phylink_get_caps	= gswip_xrx200_phylink_get_caps,
 	.phylink_mac_config	= gswip_phylink_mac_config,
 	.phylink_mac_link_down	= gswip_phylink_mac_link_down,
 	.phylink_mac_link_up	= gswip_phylink_mac_link_up,
@@ -1851,7 +1807,7 @@ static const struct dsa_switch_ops gswip_xrx300_switch_ops = {
 	.port_fdb_add		= gswip_port_fdb_add,
 	.port_fdb_del		= gswip_port_fdb_del,
 	.port_fdb_dump		= gswip_port_fdb_dump,
-	.phylink_validate	= gswip_xrx300_phylink_validate,
+	.phylink_get_caps	= gswip_xrx300_phylink_get_caps,
 	.phylink_mac_config	= gswip_phylink_mac_config,
 	.phylink_mac_link_down	= gswip_phylink_mac_link_down,
 	.phylink_mac_link_up	= gswip_phylink_mac_link_up,
-- 
2.30.2

Re: [PATCH RFC net-next 08/12] net: dsa: lantiq: convert to phylink_generic_validate()

[Hauke Mehrtens]

On 11/24/21 6:53 PM, Russell King (Oracle) wrote:
> Populate the supported interfaces and MAC capabilities for the Lantiq
> DSA switches and remove the old validate implementation to allow DSA to
> use phylink_generic_validate() for this switch driver.
> 
> The exclusion of Gigabit linkmodes for MII, Reverse MII and Reduced MII
> links is handled within phylink_generic_validate() in phylink, so there
> is no need to make them conditional on the interface mode in the driver.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Hauke Mehrtens <hauke@hauke-m.de>

> ---
>   drivers/net/dsa/lantiq_gswip.c | 120 +++++++++++----------------------
>   1 file changed, 38 insertions(+), 82 deletions(-)
> 
> diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
> index 7056d98d8177..583af774e1bd 100644
> --- a/drivers/net/dsa/lantiq_gswip.c
> +++ b/drivers/net/dsa/lantiq_gswip.c
> @@ -1438,114 +1438,70 @@ static int gswip_port_fdb_dump(struct dsa_switch *ds, int port,
>   	return 0;
>   }
>   
> -static void gswip_phylink_set_capab(unsigned long *supported,
> -				    struct phylink_link_state *state)
> -{
> -	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> -
> -	/* Allow all the expected bits */
> -	phylink_set(mask, Autoneg);
> -	phylink_set_port_modes(mask);
> -	phylink_set(mask, Pause);
> -	phylink_set(mask, Asym_Pause);
> -
> -	/* With the exclusion of MII, Reverse MII and Reduced MII, we
> -	 * support Gigabit, including Half duplex
> -	 */
> -	if (state->interface != PHY_INTERFACE_MODE_MII &&
> -	    state->interface != PHY_INTERFACE_MODE_REVMII &&
> -	    state->interface != PHY_INTERFACE_MODE_RMII) {
> -		phylink_set(mask, 1000baseT_Full);
> -		phylink_set(mask, 1000baseT_Half);
> -	}
> -
> -	phylink_set(mask, 10baseT_Half);
> -	phylink_set(mask, 10baseT_Full);
> -	phylink_set(mask, 100baseT_Half);
> -	phylink_set(mask, 100baseT_Full);
> -
> -	linkmode_and(supported, supported, mask);
> -	linkmode_and(state->advertising, state->advertising, mask);
> -}
> -
> -static void gswip_xrx200_phylink_validate(struct dsa_switch *ds, int port,
> -					  unsigned long *supported,
> -					  struct phylink_link_state *state)
> +static void gswip_xrx200_phylink_get_caps(struct dsa_switch *ds, int port,
> +					  struct phylink_config *config)
>   {
>   	switch (port) {
>   	case 0:
>   	case 1:
> -		if (!phy_interface_mode_is_rgmii(state->interface) &&
> -		    state->interface != PHY_INTERFACE_MODE_MII &&
> -		    state->interface != PHY_INTERFACE_MODE_REVMII &&
> -		    state->interface != PHY_INTERFACE_MODE_RMII)
> -			goto unsupported;
> +		phy_interface_set_rgmii(config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_MII,
> +			  config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_REVMII,
> +			  config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_RMII,
> +			  config->supported_interfaces);
>   		break;
> +
>   	case 2:
>   	case 3:
>   	case 4:
> -		if (state->interface != PHY_INTERFACE_MODE_INTERNAL)
> -			goto unsupported;
> +		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> +			  config->supported_interfaces);
>   		break;
> +
>   	case 5:
> -		if (!phy_interface_mode_is_rgmii(state->interface) &&
> -		    state->interface != PHY_INTERFACE_MODE_INTERNAL)
> -			goto unsupported;
> +		phy_interface_set_rgmii(config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> +			  config->supported_interfaces);
>   		break;
> -	default:
> -		linkmode_zero(supported);
> -		dev_err(ds->dev, "Unsupported port: %i\n", port);
> -		return;
>   	}
>   
> -	gswip_phylink_set_capab(supported, state);
> -
> -	return;
> -
> -unsupported:
> -	linkmode_zero(supported);
> -	dev_err(ds->dev, "Unsupported interface '%s' for port %d\n",
> -		phy_modes(state->interface), port);
> +	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> +		MAC_10 | MAC_100 | MAC_1000;
>   }
>   
> -static void gswip_xrx300_phylink_validate(struct dsa_switch *ds, int port,
> -					  unsigned long *supported,
> -					  struct phylink_link_state *state)
> +static void gswip_xrx300_phylink_get_caps(struct dsa_switch *ds, int port,
> +					  struct phylink_config *config)
>   {
>   	switch (port) {
>   	case 0:
> -		if (!phy_interface_mode_is_rgmii(state->interface) &&
> -		    state->interface != PHY_INTERFACE_MODE_GMII &&
> -		    state->interface != PHY_INTERFACE_MODE_RMII)
> -			goto unsupported;
> +		phy_interface_set_rgmii(config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_GMII,
> +			  config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_RMII,
> +			  config->supported_interfaces);
>   		break;
> +
>   	case 1:
>   	case 2:
>   	case 3:
>   	case 4:
> -		if (state->interface != PHY_INTERFACE_MODE_INTERNAL)
> -			goto unsupported;
> +		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> +			  config->supported_interfaces);
>   		break;
> +
>   	case 5:
> -		if (!phy_interface_mode_is_rgmii(state->interface) &&
> -		    state->interface != PHY_INTERFACE_MODE_INTERNAL &&
> -		    state->interface != PHY_INTERFACE_MODE_RMII)
> -			goto unsupported;
> +		phy_interface_set_rgmii(config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> +			  config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_RMII,
> +			  config->supported_interfaces);
>   		break;
> -	default:
> -		linkmode_zero(supported);
> -		dev_err(ds->dev, "Unsupported port: %i\n", port);
> -		return;
>   	}
>   
> -	gswip_phylink_set_capab(supported, state);
> -
> -	return;
> -
> -unsupported:
> -	linkmode_zero(supported);
> -	dev_err(ds->dev, "Unsupported interface '%s' for port %d\n",
> -		phy_modes(state->interface), port);
> +	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> +		MAC_10 | MAC_100 | MAC_1000;
>   }
>   
>   static void gswip_port_set_link(struct gswip_priv *priv, int port, bool link)
> @@ -1827,7 +1783,7 @@ static const struct dsa_switch_ops gswip_xrx200_switch_ops = {
>   	.port_fdb_add		= gswip_port_fdb_add,
>   	.port_fdb_del		= gswip_port_fdb_del,
>   	.port_fdb_dump		= gswip_port_fdb_dump,
> -	.phylink_validate	= gswip_xrx200_phylink_validate,
> +	.phylink_get_caps	= gswip_xrx200_phylink_get_caps,
>   	.phylink_mac_config	= gswip_phylink_mac_config,
>   	.phylink_mac_link_down	= gswip_phylink_mac_link_down,
>   	.phylink_mac_link_up	= gswip_phylink_mac_link_up,
> @@ -1851,7 +1807,7 @@ static const struct dsa_switch_ops gswip_xrx300_switch_ops = {
>   	.port_fdb_add		= gswip_port_fdb_add,
>   	.port_fdb_del		= gswip_port_fdb_del,
>   	.port_fdb_dump		= gswip_port_fdb_dump,
> -	.phylink_validate	= gswip_xrx300_phylink_validate,
> +	.phylink_get_caps	= gswip_xrx300_phylink_get_caps,
>   	.phylink_mac_config	= gswip_phylink_mac_config,
>   	.phylink_mac_link_down	= gswip_phylink_mac_link_down,
>   	.phylink_mac_link_up	= gswip_phylink_mac_link_up,
> 

[PATCH RFC net-next 09/12] net: dsa: ocelot: convert to phylink_generic_validate()

[Russell King (Oracle)]

Populate the supported interfaces and MAC capabilities for the Ocelot
DSA switches and remove the old validate implementation to allow DSA to
use phylink_generic_validate() for this switch driver.

The felix_vsc9959 and seville_vsc9953 sub-drivers only supports a
single interface mode, defined by ocelot_port->phy_mode, so we indicate
only this interface mode to phylink. Since phylink restricts the
ethtool link modes based on interface, we do not need to make the MAC
capabilities dependent on the interface mode.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/ocelot/felix.c           | 11 ++++---
 drivers/net/dsa/ocelot/felix.h           |  5 ++--
 drivers/net/dsa/ocelot/felix_vsc9959.c   | 37 +++++-------------------
 drivers/net/dsa/ocelot/seville_vsc9953.c | 34 +++++-----------------
 4 files changed, 21 insertions(+), 66 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index e487143709da..26529db951fc 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -801,15 +801,14 @@ static int felix_vlan_del(struct dsa_switch *ds, int port,
 	return ocelot_vlan_del(ocelot, port, vlan->vid);
 }
 
-static void felix_phylink_validate(struct dsa_switch *ds, int port,
-				   unsigned long *supported,
-				   struct phylink_link_state *state)
+static void felix_phylink_get_caps(struct dsa_switch *ds, int port,
+				   struct phylink_config *config)
 {
 	struct ocelot *ocelot = ds->priv;
 	struct felix *felix = ocelot_to_felix(ocelot);
 
-	if (felix->info->phylink_validate)
-		felix->info->phylink_validate(ocelot, port, supported, state);
+	if (felix->info->phylink_get_caps)
+		felix->info->phylink_get_caps(ocelot, port, config);
 }
 
 static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
@@ -1644,7 +1643,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_validate		= felix_phylink_validate,
+	.phylink_get_caps		= felix_phylink_get_caps,
 	.phylink_mac_config		= felix_phylink_mac_config,
 	.phylink_mac_link_down		= felix_phylink_mac_link_down,
 	.phylink_mac_link_up		= felix_phylink_mac_link_up,
diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h
index dfe08dddd262..1060add151de 100644
--- a/drivers/net/dsa/ocelot/felix.h
+++ b/drivers/net/dsa/ocelot/felix.h
@@ -43,9 +43,8 @@ struct felix_info {
 
 	int	(*mdio_bus_alloc)(struct ocelot *ocelot);
 	void	(*mdio_bus_free)(struct ocelot *ocelot);
-	void	(*phylink_validate)(struct ocelot *ocelot, int port,
-				    unsigned long *supported,
-				    struct phylink_link_state *state);
+	void	(*phylink_get_caps)(struct ocelot *ocelot, int port,
+				    struct phylink_config *config);
 	int	(*prevalidate_phy_mode)(struct ocelot *ocelot, int port,
 					phy_interface_t phy_mode);
 	int	(*port_setup_tc)(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 42ac1952b39a..091d33a52e41 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -938,39 +938,16 @@ static int vsc9959_reset(struct ocelot *ocelot)
 	return 0;
 }
 
-static void vsc9959_phylink_validate(struct ocelot *ocelot, int port,
-				     unsigned long *supported,
-				     struct phylink_link_state *state)
+static void vsc9959_phylink_get_caps(struct ocelot *ocelot, int port,
+				     struct phylink_config *config)
 {
 	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);
-	phylink_set(mask, Asym_Pause);
-	phylink_set(mask, 10baseT_Half);
-	phylink_set(mask, 10baseT_Full);
-	phylink_set(mask, 100baseT_Half);
-	phylink_set(mask, 100baseT_Full);
-	phylink_set(mask, 1000baseT_Half);
-	phylink_set(mask, 1000baseT_Full);
-
-	if (state->interface == PHY_INTERFACE_MODE_INTERNAL ||
-	    state->interface == PHY_INTERFACE_MODE_2500BASEX ||
-	    state->interface == PHY_INTERFACE_MODE_USXGMII) {
-		phylink_set(mask, 2500baseT_Full);
-		phylink_set(mask, 2500baseX_Full);
-	}
+	__set_bit(ocelot_port->phy_mode,
+		  config->supported_interfaces);
 
-	linkmode_and(supported, supported, mask);
-	linkmode_and(state->advertising, state->advertising, mask);
+	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
+		MAC_10 | MAC_100 | MAC_1000 | MAC_2500FD;
 }
 
 static int vsc9959_prevalidate_phy_mode(struct ocelot *ocelot, int port,
@@ -2161,7 +2138,7 @@ static const struct felix_info felix_info_vsc9959 = {
 	.ptp_caps		= &vsc9959_ptp_caps,
 	.mdio_bus_alloc		= vsc9959_mdio_bus_alloc,
 	.mdio_bus_free		= vsc9959_mdio_bus_free,
-	.phylink_validate	= vsc9959_phylink_validate,
+	.phylink_get_caps	= vsc9959_phylink_get_caps,
 	.prevalidate_phy_mode	= vsc9959_prevalidate_phy_mode,
 	.port_setup_tc		= vsc9959_port_setup_tc,
 	.port_sched_speed_set	= vsc9959_sched_speed_set,
diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
index 899b98193b4a..37759853e1b2 100644
--- a/drivers/net/dsa/ocelot/seville_vsc9953.c
+++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
@@ -994,36 +994,16 @@ static int vsc9953_reset(struct ocelot *ocelot)
 	return 0;
 }
 
-static void vsc9953_phylink_validate(struct ocelot *ocelot, int port,
-				     unsigned long *supported,
-				     struct phylink_link_state *state)
+static void vsc9953_phylink_get_caps(struct ocelot *ocelot, int port,
+				     struct phylink_config *config)
 {
 	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);
-	phylink_set(mask, Asym_Pause);
-	phylink_set(mask, 10baseT_Full);
-	phylink_set(mask, 10baseT_Half);
-	phylink_set(mask, 100baseT_Full);
-	phylink_set(mask, 100baseT_Half);
-	phylink_set(mask, 1000baseT_Full);
-
-	if (state->interface == PHY_INTERFACE_MODE_INTERNAL) {
-		phylink_set(mask, 2500baseT_Full);
-		phylink_set(mask, 2500baseX_Full);
-	}
+	__set_bit(ocelot_port->phy_mode,
+		  config->supported_interfaces);
 
-	linkmode_and(supported, supported, mask);
-	linkmode_and(state->advertising, state->advertising, mask);
+	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
+		MAC_10 | MAC_100 | MAC_1000FD | MAC_2500FD;
 }
 
 static int vsc9953_prevalidate_phy_mode(struct ocelot *ocelot, int port,
@@ -1185,7 +1165,7 @@ static const struct felix_info seville_info_vsc9953 = {
 	.num_tx_queues		= OCELOT_NUM_TC,
 	.mdio_bus_alloc		= vsc9953_mdio_bus_alloc,
 	.mdio_bus_free		= vsc9953_mdio_bus_free,
-	.phylink_validate	= vsc9953_phylink_validate,
+	.phylink_get_caps	= vsc9953_phylink_get_caps,
 	.prevalidate_phy_mode	= vsc9953_prevalidate_phy_mode,
 };
 
-- 
2.30.2

Re: [PATCH RFC net-next 09/12] net: dsa: ocelot: convert to phylink_generic_validate()

[Vladimir Oltean]

On Wed, Nov 24, 2021 at 05:53:09PM +0000, Russell King (Oracle) wrote:
> Populate the supported interfaces and MAC capabilities for the Ocelot
> DSA switches and remove the old validate implementation to allow DSA to
> use phylink_generic_validate() for this switch driver.
> 
> The felix_vsc9959 and seville_vsc9953 sub-drivers only supports a
> single interface mode, defined by ocelot_port->phy_mode, so we indicate
> only this interface mode to phylink. Since phylink restricts the
> ethtool link modes based on interface, we do not need to make the MAC
> capabilities dependent on the interface mode.

Yes, and this driver cannot make use of phylink_generic_validate()
unless something changes in phylink_get_linkmodes(). You've said a
number of times that PHY rate adaptation via PAUSE frames is not
something that is supported, yet it works with 2500base-x and the felix
driver, and we use this functionality on LS1028A-QDS boards and the
AQR412 PHY, and customer boards using LS1028A probably use it too. See
this comment in ocelot_phylink_mac_link_up():

	/* Handle RX pause in all cases, with 2500base-X this is used for rate
	 * adaptation.
	 */
	mac_fc_cfg |= SYS_MAC_FC_CFG_RX_FC_ENA;

The reason why you've said it isn't supported is because "it will
confuse MAC and PCS drivers at the moment", which is something that does
not happen for this particular combination of MAC and PCS (Lynx) drivers
and SERDES protocol (because the speed is fixed, there is no reason to
look at the "speed" argument which represents the media-side link speed).

And what has to change in phylink_get_linkmodes() is this:

void phylink_get_linkmodes(unsigned long *linkmodes, phy_interface_t interface,
			   unsigned long mac_capabilities)
{
	unsigned long caps = MAC_SYM_PAUSE | MAC_ASYM_PAUSE;

	switch (interface) {
(...)
	case PHY_INTERFACE_MODE_2500BASEX:
		caps |= MAC_2500FD;
		break;
(...)
	}

	phylink_caps_to_linkmodes(linkmodes, caps & mac_capabilities);
}

As long as phylink_caps_to_linkmodes() doesn't have additional logic to
not remove the gigabit and lower link modes from the PHY advertisement
and supported masks when MAC_2500FD is set but MAC_1000FD and lower
aren't (and the driver requests rate adaptation via PAUSE frames for
this PHY mode), then the generic validation method is going to introduce
regressions here, which are not acceptable.

Otherwise the patch looks good.

> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/dsa/ocelot/felix.c           | 11 ++++---
>  drivers/net/dsa/ocelot/felix.h           |  5 ++--
>  drivers/net/dsa/ocelot/felix_vsc9959.c   | 37 +++++-------------------
>  drivers/net/dsa/ocelot/seville_vsc9953.c | 34 +++++-----------------
>  4 files changed, 21 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
> index e487143709da..26529db951fc 100644
> --- a/drivers/net/dsa/ocelot/felix.c
> +++ b/drivers/net/dsa/ocelot/felix.c
> @@ -801,15 +801,14 @@ static int felix_vlan_del(struct dsa_switch *ds, int port,
>  	return ocelot_vlan_del(ocelot, port, vlan->vid);
>  }
>  
> -static void felix_phylink_validate(struct dsa_switch *ds, int port,
> -				   unsigned long *supported,
> -				   struct phylink_link_state *state)
> +static void felix_phylink_get_caps(struct dsa_switch *ds, int port,
> +				   struct phylink_config *config)
>  {
>  	struct ocelot *ocelot = ds->priv;
>  	struct felix *felix = ocelot_to_felix(ocelot);
>  
> -	if (felix->info->phylink_validate)
> -		felix->info->phylink_validate(ocelot, port, supported, state);
> +	if (felix->info->phylink_get_caps)
> +		felix->info->phylink_get_caps(ocelot, port, config);
>  }
>  
>  static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
> @@ -1644,7 +1643,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_validate		= felix_phylink_validate,
> +	.phylink_get_caps		= felix_phylink_get_caps,
>  	.phylink_mac_config		= felix_phylink_mac_config,
>  	.phylink_mac_link_down		= felix_phylink_mac_link_down,
>  	.phylink_mac_link_up		= felix_phylink_mac_link_up,
> diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h
> index dfe08dddd262..1060add151de 100644
> --- a/drivers/net/dsa/ocelot/felix.h
> +++ b/drivers/net/dsa/ocelot/felix.h
> @@ -43,9 +43,8 @@ struct felix_info {
>  
>  	int	(*mdio_bus_alloc)(struct ocelot *ocelot);
>  	void	(*mdio_bus_free)(struct ocelot *ocelot);
> -	void	(*phylink_validate)(struct ocelot *ocelot, int port,
> -				    unsigned long *supported,
> -				    struct phylink_link_state *state);
> +	void	(*phylink_get_caps)(struct ocelot *ocelot, int port,
> +				    struct phylink_config *config);
>  	int	(*prevalidate_phy_mode)(struct ocelot *ocelot, int port,
>  					phy_interface_t phy_mode);
>  	int	(*port_setup_tc)(struct dsa_switch *ds, int port,
> diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
> index 42ac1952b39a..091d33a52e41 100644
> --- a/drivers/net/dsa/ocelot/felix_vsc9959.c
> +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
> @@ -938,39 +938,16 @@ static int vsc9959_reset(struct ocelot *ocelot)
>  	return 0;
>  }
>  
> -static void vsc9959_phylink_validate(struct ocelot *ocelot, int port,
> -				     unsigned long *supported,
> -				     struct phylink_link_state *state)
> +static void vsc9959_phylink_get_caps(struct ocelot *ocelot, int port,
> +				     struct phylink_config *config)
>  {
>  	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);
> -	phylink_set(mask, Asym_Pause);
> -	phylink_set(mask, 10baseT_Half);
> -	phylink_set(mask, 10baseT_Full);
> -	phylink_set(mask, 100baseT_Half);
> -	phylink_set(mask, 100baseT_Full);
> -	phylink_set(mask, 1000baseT_Half);
> -	phylink_set(mask, 1000baseT_Full);
> -
> -	if (state->interface == PHY_INTERFACE_MODE_INTERNAL ||
> -	    state->interface == PHY_INTERFACE_MODE_2500BASEX ||
> -	    state->interface == PHY_INTERFACE_MODE_USXGMII) {
> -		phylink_set(mask, 2500baseT_Full);
> -		phylink_set(mask, 2500baseX_Full);
> -	}
> +	__set_bit(ocelot_port->phy_mode,
> +		  config->supported_interfaces);

This fits on a single line.

>  
> -	linkmode_and(supported, supported, mask);
> -	linkmode_and(state->advertising, state->advertising, mask);
> +	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> +		MAC_10 | MAC_100 | MAC_1000 | MAC_2500FD;
>  }
>  
>  static int vsc9959_prevalidate_phy_mode(struct ocelot *ocelot, int port,
> @@ -2161,7 +2138,7 @@ static const struct felix_info felix_info_vsc9959 = {
>  	.ptp_caps		= &vsc9959_ptp_caps,
>  	.mdio_bus_alloc		= vsc9959_mdio_bus_alloc,
>  	.mdio_bus_free		= vsc9959_mdio_bus_free,
> -	.phylink_validate	= vsc9959_phylink_validate,
> +	.phylink_get_caps	= vsc9959_phylink_get_caps,
>  	.prevalidate_phy_mode	= vsc9959_prevalidate_phy_mode,
>  	.port_setup_tc		= vsc9959_port_setup_tc,
>  	.port_sched_speed_set	= vsc9959_sched_speed_set,
> diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
> index 899b98193b4a..37759853e1b2 100644
> --- a/drivers/net/dsa/ocelot/seville_vsc9953.c
> +++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
> @@ -994,36 +994,16 @@ static int vsc9953_reset(struct ocelot *ocelot)
>  	return 0;
>  }
>  
> -static void vsc9953_phylink_validate(struct ocelot *ocelot, int port,
> -				     unsigned long *supported,
> -				     struct phylink_link_state *state)
> +static void vsc9953_phylink_get_caps(struct ocelot *ocelot, int port,
> +				     struct phylink_config *config)
>  {
>  	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);
> -	phylink_set(mask, Asym_Pause);
> -	phylink_set(mask, 10baseT_Full);
> -	phylink_set(mask, 10baseT_Half);
> -	phylink_set(mask, 100baseT_Full);
> -	phylink_set(mask, 100baseT_Half);
> -	phylink_set(mask, 1000baseT_Full);
> -
> -	if (state->interface == PHY_INTERFACE_MODE_INTERNAL) {
> -		phylink_set(mask, 2500baseT_Full);
> -		phylink_set(mask, 2500baseX_Full);
> -	}
> +	__set_bit(ocelot_port->phy_mode,
> +		  config->supported_interfaces);
>  
> -	linkmode_and(supported, supported, mask);
> -	linkmode_and(state->advertising, state->advertising, mask);
> +	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> +		MAC_10 | MAC_100 | MAC_1000FD | MAC_2500FD;

I would prefer if MAC_10 was aligned with MAC_ASYM_PAUSE, if possible. Thanks.

>  }
>  
>  static int vsc9953_prevalidate_phy_mode(struct ocelot *ocelot, int port,
> @@ -1185,7 +1165,7 @@ static const struct felix_info seville_info_vsc9953 = {
>  	.num_tx_queues		= OCELOT_NUM_TC,
>  	.mdio_bus_alloc		= vsc9953_mdio_bus_alloc,
>  	.mdio_bus_free		= vsc9953_mdio_bus_free,
> -	.phylink_validate	= vsc9953_phylink_validate,
> +	.phylink_get_caps	= vsc9953_phylink_get_caps,
>  	.prevalidate_phy_mode	= vsc9953_prevalidate_phy_mode,
>  };
>  
> -- 
> 2.30.2
>

Re: [PATCH RFC net-next 09/12] net: dsa: ocelot: convert to phylink_generic_validate()

[Russell King (Oracle)]

On Wed, Nov 24, 2021 at 08:07:49PM +0000, Vladimir Oltean wrote:
> On Wed, Nov 24, 2021 at 05:53:09PM +0000, Russell King (Oracle) wrote:
> > Populate the supported interfaces and MAC capabilities for the Ocelot
> > DSA switches and remove the old validate implementation to allow DSA to
> > use phylink_generic_validate() for this switch driver.
> > 
> > The felix_vsc9959 and seville_vsc9953 sub-drivers only supports a
> > single interface mode, defined by ocelot_port->phy_mode, so we indicate
> > only this interface mode to phylink. Since phylink restricts the
> > ethtool link modes based on interface, we do not need to make the MAC
> > capabilities dependent on the interface mode.
> 
> Yes, and this driver cannot make use of phylink_generic_validate()
> unless something changes in phylink_get_linkmodes(). You've said a
> number of times that PHY rate adaptation via PAUSE frames is not
> something that is supported, yet it works with 2500base-x and the felix
> driver, and we use this functionality on LS1028A-QDS boards and the
> AQR412 PHY, and customer boards using LS1028A probably use it too. See
> this comment in ocelot_phylink_mac_link_up():

I'll drop this for now, since the issues around rate adaption should
not be handled by phylink_generic_validate(). The point of this
generic helper is to deal with the common case.

We don't get have a way of knowing that the PHY is using rate adaption,
and when rate adaption is in use, it changes the requirements for the
validation path quite substantially. So, we need:

1) phylib to be able to tell us that rate adaption is happening on the
   PHY.
2) change the way we restrict the PHY support/advertisements when
   rate adaption is in use.

I view this as an entirely separate change to this series; it needs to
change in phylink_bringup_phy(), where the restriction is applied to
the PHY, and not in phylink_validate() or below that function.

Thanks.

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

[PATCH RFC net-next 10/12] net: dsa: qca8k: convert to phylink_generic_validate()

[Russell King (Oracle)]

Populate the supported interfaces and MAC capabilities for the QCA8K
DSA switch and remove the old validate implementation to allow DSA to
use phylink_generic_validate() for this switch driver.

In making this change, we bring consistency to the ethtool linkmodes
that phylink's validate step produces, thereby following the expected
behaviour as the phylink documentation has explained. Specifically, the
ethtool 1000baseX_Full capability is now permitted for all interface
modes, as it is a property of the PHY driver whether 1000baseX fiber
connections can be supported.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 6516df08a5d5..a0bdc14997e8 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1527,67 +1527,39 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 	}
 }
 
-static void
-qca8k_phylink_validate(struct dsa_switch *ds, int port,
-		       unsigned long *supported,
-		       struct phylink_link_state *state)
+static void qca8k_phylink_get_caps(struct dsa_switch *ds, int port,
+				   struct phylink_config *config)
 {
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
-
 	switch (port) {
 	case 0: /* 1st CPU port */
-		if (state->interface != PHY_INTERFACE_MODE_NA &&
-		    state->interface != PHY_INTERFACE_MODE_RGMII &&
-		    state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
-		    state->interface != PHY_INTERFACE_MODE_RGMII_TXID &&
-		    state->interface != PHY_INTERFACE_MODE_RGMII_RXID &&
-		    state->interface != PHY_INTERFACE_MODE_SGMII)
-			goto unsupported;
+		phy_interface_set_rgmii(config->supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_SGMII,
+			  config->supported_interfaces);
 		break;
+
 	case 1:
 	case 2:
 	case 3:
 	case 4:
 	case 5:
 		/* Internal PHY */
-		if (state->interface != PHY_INTERFACE_MODE_NA &&
-		    state->interface != PHY_INTERFACE_MODE_GMII &&
-		    state->interface != PHY_INTERFACE_MODE_INTERNAL)
-			goto unsupported;
+		__set_bit(PHY_INTERFACE_MODE_GMII,
+			  config->supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
+			  config->supported_interfaces);
 		break;
+
 	case 6: /* 2nd CPU port / external PHY */
-		if (state->interface != PHY_INTERFACE_MODE_NA &&
-		    state->interface != PHY_INTERFACE_MODE_RGMII &&
-		    state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
-		    state->interface != PHY_INTERFACE_MODE_RGMII_TXID &&
-		    state->interface != PHY_INTERFACE_MODE_RGMII_RXID &&
-		    state->interface != PHY_INTERFACE_MODE_SGMII &&
-		    state->interface != PHY_INTERFACE_MODE_1000BASEX)
-			goto unsupported;
+		phy_interface_set_rgmii(config->supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_SGMII,
+			  config->supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
+			  config->supported_interfaces);
 		break;
-	default:
-unsupported:
-		linkmode_zero(supported);
-		return;
 	}
 
-	phylink_set_port_modes(mask);
-	phylink_set(mask, Autoneg);
-
-	phylink_set(mask, 1000baseT_Full);
-	phylink_set(mask, 10baseT_Half);
-	phylink_set(mask, 10baseT_Full);
-	phylink_set(mask, 100baseT_Half);
-	phylink_set(mask, 100baseT_Full);
-
-	if (state->interface == PHY_INTERFACE_MODE_1000BASEX)
-		phylink_set(mask, 1000baseX_Full);
-
-	phylink_set(mask, Pause);
-	phylink_set(mask, Asym_Pause);
-
-	linkmode_and(supported, supported, mask);
-	linkmode_and(state->advertising, state->advertising, mask);
+	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
+		MAC_10 | MAC_100 | MAC_1000FD;
 }
 
 static int
@@ -2405,7 +2377,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 	.port_vlan_filtering	= qca8k_port_vlan_filtering,
 	.port_vlan_add		= qca8k_port_vlan_add,
 	.port_vlan_del		= qca8k_port_vlan_del,
-	.phylink_validate	= qca8k_phylink_validate,
+	.phylink_get_caps	= qca8k_phylink_get_caps,
 	.phylink_mac_link_state	= qca8k_phylink_mac_link_state,
 	.phylink_mac_config	= qca8k_phylink_mac_config,
 	.phylink_mac_link_down	= qca8k_phylink_mac_link_down,
-- 
2.30.2

[PATCH RFC net-next 11/12] net: dsa: sja1105: convert to phylink_generic_validate()

[Russell King (Oracle)]

Populate the supported interfaces and MAC capabilities for the SJA1105
DSA switch and remove the old validate implementation to allow DSA to
use phylink_generic_validate() for this switch driver.

This switch only supports a static model of configuration, so we
restrict the interface modes to the configured setting, and pass the
MAC capabilities. As it is unclear which interface modes support 1G
speeds, we keep the setting of MAC_1000FD conditional on the configured
interface mode.

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

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index c343effe2e96..6d763f2f63b7 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1411,48 +1411,29 @@ static void sja1105_mac_link_up(struct dsa_switch *ds, int port,
 	sja1105_inhibit_tx(priv, BIT(port), false);
 }
 
-static void sja1105_phylink_validate(struct dsa_switch *ds, int port,
-				     unsigned long *supported,
-				     struct phylink_link_state *state)
+static void sja1105_phylink_get_caps(struct dsa_switch *ds, int port,
+				     struct phylink_config *config)
 {
-	/* Construct a new mask which exhaustively contains all link features
-	 * supported by the MAC, and then apply that (logical AND) to what will
-	 * be sent to the PHY for "marketing".
-	 */
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
 	struct sja1105_private *priv = ds->priv;
 	struct sja1105_xmii_params_entry *mii;
 
 	mii = priv->static_config.tables[BLK_IDX_XMII_PARAMS].entries;
 
-	/* include/linux/phylink.h says:
-	 *     When @state->interface is %PHY_INTERFACE_MODE_NA, phylink
-	 *     expects the MAC driver to return all supported link modes.
+	/* The SJA1105 MAC programming model is through the static config
+	 * (the xMII Mode table cannot be dynamically reconfigured), and
+	 * we have to program that early.
 	 */
-	if (state->interface != PHY_INTERFACE_MODE_NA &&
-	    sja1105_phy_mode_mismatch(priv, port, state->interface)) {
-		linkmode_zero(supported);
-		return;
-	}
+	__set_bit(priv->phy_mode[port], config->supported_interfaces);
+
+	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
+		MAC_10FD | MAC_100FD;
 
-	/* The MAC does not support pause frames, and also doesn't
-	 * support half-duplex traffic modes.
-	 */
-	phylink_set(mask, Autoneg);
-	phylink_set(mask, MII);
-	phylink_set(mask, 10baseT_Full);
-	phylink_set(mask, 100baseT_Full);
-	phylink_set(mask, 100baseT1_Full);
 	if (mii->xmii_mode[port] == XMII_MODE_RGMII ||
 	    mii->xmii_mode[port] == XMII_MODE_SGMII)
-		phylink_set(mask, 1000baseT_Full);
-	if (priv->info->supports_2500basex[port]) {
-		phylink_set(mask, 2500baseT_Full);
-		phylink_set(mask, 2500baseX_Full);
-	}
+		config->mac_capabilities |= MAC_1000FD;
 
-	linkmode_and(supported, supported, mask);
-	linkmode_and(state->advertising, state->advertising, mask);
+	if (priv->info->supports_2500basex[port])
+		config->mac_capabilities |= MAC_2500FD;
 }
 
 static int
@@ -3189,7 +3170,7 @@ static const struct dsa_switch_ops sja1105_switch_ops = {
 	.set_ageing_time	= sja1105_set_ageing_time,
 	.port_change_mtu	= sja1105_change_mtu,
 	.port_max_mtu		= sja1105_get_max_mtu,
-	.phylink_validate	= sja1105_phylink_validate,
+	.phylink_get_caps	= sja1105_phylink_get_caps,
 	.phylink_mac_config	= sja1105_mac_config,
 	.phylink_mac_link_up	= sja1105_mac_link_up,
 	.phylink_mac_link_down	= sja1105_mac_link_down,
-- 
2.30.2

Re: [PATCH RFC net-next 11/12] net: dsa: sja1105: convert to phylink_generic_validate()

[Vladimir Oltean]

On Wed, Nov 24, 2021 at 05:53:19PM +0000, Russell King (Oracle) wrote:
> Populate the supported interfaces and MAC capabilities for the SJA1105
> DSA switch and remove the old validate implementation to allow DSA to
> use phylink_generic_validate() for this switch driver.
> 
> This switch only supports a static model of configuration, so we
> restrict the interface modes to the configured setting, and pass the
> MAC capabilities. As it is unclear which interface modes support 1G
> speeds, we keep the setting of MAC_1000FD conditional on the configured
> interface mode.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Please use this patch for sja1105. Thanks.

-- >8 --
From cc8a64e9aec8afeae5005dd34636fdccde22c1ac Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Wed, 24 Nov 2021 21:02:43 +0200
Subject: [PATCH] net: dsa: sja1105: convert to phylink_generic_validate()

Provide a ->phylink_get_caps() implementation in order to tell phylink
what are the PHY modes between which each port can change, and the MAC
capabilities so it can limit the advertisement and supported masks of
the PHY using the generic validation method.

Now that we populate phylink_config->supported_interfaces, it is
phylink's responsibility to not attempt a PHY mode change towards
something which we do not support, so we can delete the logic from
sja1105_phy_mode_mismatch() and move the essence of it into
sja1105_phylink_get_caps(), which happens much earlier. By removing the
PHY mode mismatch checks, we now effectively allow SERDES protocol
changes between SGMII and 2500base-X, while still denying PHY mode
changes between one xMII kind and another, which did not make sense.

This patch also fixes an inconsequential bug, which was that for ports
which support 2500base-X, we used to keep advertising the gigabit and
lower speeds. We should not have done this, because 2500base-X operates
only at 2500Mbps (and we do not support PAUSE frames in order for the
lower media speeds to work via rate adaptation). Nonetheless, the only
SJA1110 boards which use 2500base-X use it in a SERDES-to-SERDES fixed
link, so there isn't any PHY whose advertisement matters there.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 105 +++++++++++++------------
 1 file changed, 53 insertions(+), 52 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index c343effe2e96..86883291e71d 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1357,19 +1357,6 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port,
 	return sja1105_clocking_setup_port(priv, port);
 }
 
-/* The SJA1105 MAC programming model is through the static config (the xMII
- * Mode table cannot be dynamically reconfigured), and we have to program
- * that early (earlier than PHYLINK calls us, anyway).
- * So just error out in case the connected PHY attempts to change the initial
- * system interface MII protocol from what is defined in the DT, at least for
- * now.
- */
-static bool sja1105_phy_mode_mismatch(struct sja1105_private *priv, int port,
-				      phy_interface_t interface)
-{
-	return priv->phy_mode[port] != interface;
-}
-
 static void sja1105_mac_config(struct dsa_switch *ds, int port,
 			       unsigned int mode,
 			       const struct phylink_link_state *state)
@@ -1378,12 +1365,6 @@ static void sja1105_mac_config(struct dsa_switch *ds, int port,
 	struct sja1105_private *priv = ds->priv;
 	struct dw_xpcs *xpcs;
 
-	if (sja1105_phy_mode_mismatch(priv, port, state->interface)) {
-		dev_err(ds->dev, "Changing PHY mode to %s not supported!\n",
-			phy_modes(state->interface));
-		return;
-	}
-
 	xpcs = priv->xpcs[port];
 
 	if (xpcs)
@@ -1411,48 +1392,68 @@ static void sja1105_mac_link_up(struct dsa_switch *ds, int port,
 	sja1105_inhibit_tx(priv, BIT(port), false);
 }
 
-static void sja1105_phylink_validate(struct dsa_switch *ds, int port,
-				     unsigned long *supported,
-				     struct phylink_link_state *state)
+static void sja1105_phylink_get_caps(struct dsa_switch *ds, int port,
+				     struct phylink_config *config)
 {
-	/* Construct a new mask which exhaustively contains all link features
-	 * supported by the MAC, and then apply that (logical AND) to what will
-	 * be sent to the PHY for "marketing".
-	 */
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
 	struct sja1105_private *priv = ds->priv;
-	struct sja1105_xmii_params_entry *mii;
 
-	mii = priv->static_config.tables[BLK_IDX_XMII_PARAMS].entries;
-
-	/* include/linux/phylink.h says:
-	 *     When @state->interface is %PHY_INTERFACE_MODE_NA, phylink
-	 *     expects the MAC driver to return all supported link modes.
-	 */
-	if (state->interface != PHY_INTERFACE_MODE_NA &&
-	    sja1105_phy_mode_mismatch(priv, port, state->interface)) {
-		linkmode_zero(supported);
+	switch (priv->phy_mode[port]) {
+	case PHY_INTERFACE_MODE_MII:
+	case PHY_INTERFACE_MODE_REVMII:
+	case PHY_INTERFACE_MODE_RMII:
+	case PHY_INTERFACE_MODE_REVRMII:
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+	case PHY_INTERFACE_MODE_INTERNAL:
+		/* Changing the PHY mode of xMII (parallel) ports is possible,
+		 * but requires a switch reset, and on top of that, will never
+		 * be needed in real life. So these ports support only the PHY
+		 * mode declared in the device tree.
+		 */
+		__set_bit(priv->phy_mode[port], config->supported_interfaces);
+		break;
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_2500BASEX:
+		/* Changing the PHY mode on SERDES ports is possible and makes
+		 * sense, because that is done through the XPCS. We allow
+		 * changes between SGMII and 2500base-X (it is unknown whether
+		 * 1000base-X is supported).
+		 */
+		if (priv->info->supports_sgmii[port]) {
+			__set_bit(PHY_INTERFACE_MODE_SGMII,
+				  config->supported_interfaces);
+		}
+		if (priv->info->supports_2500basex[port]) {
+			__set_bit(PHY_INTERFACE_MODE_2500BASEX,
+				  config->supported_interfaces);
+		}
+		break;
+	default:
 		return;
 	}
 
 	/* The MAC does not support pause frames, and also doesn't
 	 * support half-duplex traffic modes.
 	 */
-	phylink_set(mask, Autoneg);
-	phylink_set(mask, MII);
-	phylink_set(mask, 10baseT_Full);
-	phylink_set(mask, 100baseT_Full);
-	phylink_set(mask, 100baseT1_Full);
-	if (mii->xmii_mode[port] == XMII_MODE_RGMII ||
-	    mii->xmii_mode[port] == XMII_MODE_SGMII)
-		phylink_set(mask, 1000baseT_Full);
-	if (priv->info->supports_2500basex[port]) {
-		phylink_set(mask, 2500baseT_Full);
-		phylink_set(mask, 2500baseX_Full);
+	if (priv->info->supports_rgmii[port] ||
+	    priv->info->supports_sgmii[port]) {
+		/* Ports can be gigabit-capable either because they are xMII or
+		 * because they are SERDES ports.
+		 */
+		config->mac_capabilities = MAC_10FD | MAC_100FD | MAC_1000FD;
+		/* Some SERDES ports also support the 2500Mbps speed */
+		if (priv->info->supports_2500basex)
+			config->mac_capabilities |= MAC_2500FD;
+	} else {
+		/* As per the "Port compatibility matrix" chapter in
+		 * Documentation/networking/dsa/sja1105.rst, ports that don't
+		 * support xMII or SERDES go to the internal 100base-T1 or
+		 * 100base-TX ports, which aren't gigabit capable.
+		 */
+		config->mac_capabilities = MAC_10FD | MAC_100FD | MAC_1000FD;
 	}
-
-	linkmode_and(supported, supported, mask);
-	linkmode_and(state->advertising, state->advertising, mask);
 }
 
 static int
@@ -3189,7 +3190,7 @@ static const struct dsa_switch_ops sja1105_switch_ops = {
 	.set_ageing_time	= sja1105_set_ageing_time,
 	.port_change_mtu	= sja1105_change_mtu,
 	.port_max_mtu		= sja1105_get_max_mtu,
-	.phylink_validate	= sja1105_phylink_validate,
+	.phylink_get_caps	= sja1105_phylink_get_caps,
 	.phylink_mac_config	= sja1105_mac_config,
 	.phylink_mac_link_up	= sja1105_mac_link_up,
 	.phylink_mac_link_down	= sja1105_mac_link_down,
-- >8 --

Re: [PATCH RFC net-next 11/12] net: dsa: sja1105: convert to phylink_generic_validate()

[Russell King (Oracle)]

On Wed, Nov 24, 2021 at 07:53:40PM +0000, Vladimir Oltean wrote:
> On Wed, Nov 24, 2021 at 05:53:19PM +0000, Russell King (Oracle) wrote:
> > Populate the supported interfaces and MAC capabilities for the SJA1105
> > DSA switch and remove the old validate implementation to allow DSA to
> > use phylink_generic_validate() for this switch driver.
> > 
> > This switch only supports a static model of configuration, so we
> > restrict the interface modes to the configured setting, and pass the
> > MAC capabilities. As it is unclear which interface modes support 1G
> > speeds, we keep the setting of MAC_1000FD conditional on the configured
> > interface mode.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> 
> Please use this patch for sja1105. Thanks.

Your patch is combining two changes into one patch. Specifically, the
there are two logical changes in your patch:

1) changing the existing behaviour of the validate() function by
allowing switching between PHY_INTERFACE_MODE_SGMII and
PHY_INTERFACE_MODE_2500BASEX, which was not permitted before with the
sja1105_phy_mode_mismatch() check.

2) converting to supported_interfaces / mac_capabilities way of defining
what is supported.

Combining the two changes makes the patch harder to review, and it
becomes less obvious that it is actually correct. I would recommend
changing the existing behaviour prior to the conversion, but ultimately
that is your decision.

For more information please see the "Separate your changes" section in
Documentation/process/submitting-patches.rst

Thanks.

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

Re: [PATCH RFC net-next 11/12] net: dsa: sja1105: convert to phylink_generic_validate()

[Vladimir Oltean]

On Wed, Nov 24, 2021 at 09:08:33PM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 24, 2021 at 07:53:40PM +0000, Vladimir Oltean wrote:
> > On Wed, Nov 24, 2021 at 05:53:19PM +0000, Russell King (Oracle) wrote:
> > > Populate the supported interfaces and MAC capabilities for the SJA1105
> > > DSA switch and remove the old validate implementation to allow DSA to
> > > use phylink_generic_validate() for this switch driver.
> > > 
> > > This switch only supports a static model of configuration, so we
> > > restrict the interface modes to the configured setting, and pass the
> > > MAC capabilities. As it is unclear which interface modes support 1G
> > > speeds, we keep the setting of MAC_1000FD conditional on the configured
> > > interface mode.
> > > 
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > ---
> > 
> > Please use this patch for sja1105. Thanks.
> 
> Your patch is combining two changes into one patch. Specifically, the
> there are two logical changes in your patch:
> 
> 1) changing the existing behaviour of the validate() function by
> allowing switching between PHY_INTERFACE_MODE_SGMII and
> PHY_INTERFACE_MODE_2500BASEX, which was not permitted before with the
> sja1105_phy_mode_mismatch() check.
> 
> 2) converting to supported_interfaces / mac_capabilities way of defining
> what is supported.
> 
> Combining the two changes makes the patch harder to review, and it
> becomes less obvious that it is actually correct. I would recommend
> changing the existing behaviour prior to the conversion, but ultimately
> that is your decision.
> 
> For more information please see the "Separate your changes" section in
> Documentation/process/submitting-patches.rst
> 
> Thanks.

-- >8 --
From febedc56cf0e269556e7483a70a3e6cb8d0d5cc3 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Wed, 24 Nov 2021 21:02:43 +0200
Subject: [PATCH] net: dsa: sja1105: convert to phylink_generic_validate()

Provide a ->phylink_get_caps() implementation in order to tell phylink
what are the PHY modes between which each port can change (none for
now), and the MAC capabilities so it can limit the advertisement and
supported masks of the PHY.

Now that we populate phylink_config->supported_interfaces, it is
phylink's responsibility to not attempt a PHY mode change towards
something which we do not support, so we can delete the logic from
sja1105_phy_mode_mismatch() and move the essence of it into
sja1105_phylink_get_caps(), which happens much earlier.

This patch also fixes an inconsequential bug, which was that for ports
which support 2500base-X, we used to keep advertising the gigabit and
lower speeds. We should not have done this, because 2500base-X operates
only at 2500Mbps (and we do not support PAUSE frames in order for the
lower media speeds to work via rate adaptation). Nonetheless, the only
SJA1110 boards which use 2500base-X use it in a SERDES-to-SERDES fixed
link, so there isn't any PHY whose advertisement matters there.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 78 ++++++++------------------
 1 file changed, 24 insertions(+), 54 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index c343effe2e96..0db590daa3d9 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1357,19 +1357,6 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port,
 	return sja1105_clocking_setup_port(priv, port);
 }
 
-/* The SJA1105 MAC programming model is through the static config (the xMII
- * Mode table cannot be dynamically reconfigured), and we have to program
- * that early (earlier than PHYLINK calls us, anyway).
- * So just error out in case the connected PHY attempts to change the initial
- * system interface MII protocol from what is defined in the DT, at least for
- * now.
- */
-static bool sja1105_phy_mode_mismatch(struct sja1105_private *priv, int port,
-				      phy_interface_t interface)
-{
-	return priv->phy_mode[port] != interface;
-}
-
 static void sja1105_mac_config(struct dsa_switch *ds, int port,
 			       unsigned int mode,
 			       const struct phylink_link_state *state)
@@ -1378,12 +1365,6 @@ static void sja1105_mac_config(struct dsa_switch *ds, int port,
 	struct sja1105_private *priv = ds->priv;
 	struct dw_xpcs *xpcs;
 
-	if (sja1105_phy_mode_mismatch(priv, port, state->interface)) {
-		dev_err(ds->dev, "Changing PHY mode to %s not supported!\n",
-			phy_modes(state->interface));
-		return;
-	}
-
 	xpcs = priv->xpcs[port];
 
 	if (xpcs)
@@ -1411,48 +1392,37 @@ static void sja1105_mac_link_up(struct dsa_switch *ds, int port,
 	sja1105_inhibit_tx(priv, BIT(port), false);
 }
 
-static void sja1105_phylink_validate(struct dsa_switch *ds, int port,
-				     unsigned long *supported,
-				     struct phylink_link_state *state)
+static void sja1105_phylink_get_caps(struct dsa_switch *ds, int port,
+				     struct phylink_config *config)
 {
-	/* Construct a new mask which exhaustively contains all link features
-	 * supported by the MAC, and then apply that (logical AND) to what will
-	 * be sent to the PHY for "marketing".
-	 */
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
 	struct sja1105_private *priv = ds->priv;
-	struct sja1105_xmii_params_entry *mii;
-
-	mii = priv->static_config.tables[BLK_IDX_XMII_PARAMS].entries;
-
-	/* include/linux/phylink.h says:
-	 *     When @state->interface is %PHY_INTERFACE_MODE_NA, phylink
-	 *     expects the MAC driver to return all supported link modes.
+	phy_interface_t phy_mode;
+
+	phy_mode = priv->phy_mode[port];
+
+	/* Changing the PHY mode of xMII (parallel) ports is possible,
+	 * but requires a switch reset, and on top of that, will never
+	 * be needed in real life. So these ports support only the PHY
+	 * mode declared in the device tree.
+	 * On the other hand, changing the PHY mode on SERDES ports is
+	 * possible and makes sense, because that is done through the
+	 * XPCS. We could allow changes between SGMII and 2500base-X
+	 * (it is unknown whether 1000base-X is supported), but that
+	 * is left for a future time.
 	 */
-	if (state->interface != PHY_INTERFACE_MODE_NA &&
-	    sja1105_phy_mode_mismatch(priv, port, state->interface)) {
-		linkmode_zero(supported);
-		return;
-	}
+	__set_bit(phy_mode, config->supported_interfaces);
 
 	/* The MAC does not support pause frames, and also doesn't
 	 * support half-duplex traffic modes.
 	 */
-	phylink_set(mask, Autoneg);
-	phylink_set(mask, MII);
-	phylink_set(mask, 10baseT_Full);
-	phylink_set(mask, 100baseT_Full);
-	phylink_set(mask, 100baseT1_Full);
-	if (mii->xmii_mode[port] == XMII_MODE_RGMII ||
-	    mii->xmii_mode[port] == XMII_MODE_SGMII)
-		phylink_set(mask, 1000baseT_Full);
-	if (priv->info->supports_2500basex[port]) {
-		phylink_set(mask, 2500baseT_Full);
-		phylink_set(mask, 2500baseX_Full);
+	if (phy_mode == PHY_INTERFACE_MODE_2500BASEX) {
+		config->mac_capabilities = MAC_2500FD;
+	} else if (phy_interface_mode_is_rgmii(phy_mode) ||
+		   phy_mode == PHY_INTERFACE_MODE_SGMII) {
+		config->mac_capabilities = MAC_10FD | MAC_100FD | MAC_1000FD;
+	} else {
+		config->mac_capabilities = MAC_10FD | MAC_100FD;
 	}
-
-	linkmode_and(supported, supported, mask);
-	linkmode_and(state->advertising, state->advertising, mask);
 }
 
 static int
@@ -3189,7 +3159,7 @@ static const struct dsa_switch_ops sja1105_switch_ops = {
 	.set_ageing_time	= sja1105_set_ageing_time,
 	.port_change_mtu	= sja1105_change_mtu,
 	.port_max_mtu		= sja1105_get_max_mtu,
-	.phylink_validate	= sja1105_phylink_validate,
+	.phylink_get_caps	= sja1105_phylink_get_caps,
 	.phylink_mac_config	= sja1105_mac_config,
 	.phylink_mac_link_up	= sja1105_mac_link_up,
 	.phylink_mac_link_down	= sja1105_mac_link_down,
-- >8 --

Re: [PATCH RFC net-next 11/12] net: dsa: sja1105: convert to phylink_generic_validate()

[Russell King (Oracle)]

On Wed, Nov 24, 2021 at 10:34:33PM +0000, Vladimir Oltean wrote:
> On Wed, Nov 24, 2021 at 09:08:33PM +0000, Russell King (Oracle) wrote:
> > On Wed, Nov 24, 2021 at 07:53:40PM +0000, Vladimir Oltean wrote:
> > > On Wed, Nov 24, 2021 at 05:53:19PM +0000, Russell King (Oracle) wrote:
> > > > Populate the supported interfaces and MAC capabilities for the SJA1105
> > > > DSA switch and remove the old validate implementation to allow DSA to
> > > > use phylink_generic_validate() for this switch driver.
> > > > 
> > > > This switch only supports a static model of configuration, so we
> > > > restrict the interface modes to the configured setting, and pass the
> > > > MAC capabilities. As it is unclear which interface modes support 1G
> > > > speeds, we keep the setting of MAC_1000FD conditional on the configured
> > > > interface mode.
> > > > 
> > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > > ---
> > > 
> > > Please use this patch for sja1105. Thanks.
> > 
> > Your patch is combining two changes into one patch. Specifically, the
> > there are two logical changes in your patch:
> > 
> > 1) changing the existing behaviour of the validate() function by
> > allowing switching between PHY_INTERFACE_MODE_SGMII and
> > PHY_INTERFACE_MODE_2500BASEX, which was not permitted before with the
> > sja1105_phy_mode_mismatch() check.
> > 
> > 2) converting to supported_interfaces / mac_capabilities way of defining
> > what is supported.
> > 
> > Combining the two changes makes the patch harder to review, and it
> > becomes less obvious that it is actually correct. I would recommend
> > changing the existing behaviour prior to the conversion, but ultimately
> > that is your decision.
> > 
> > For more information please see the "Separate your changes" section in
> > Documentation/process/submitting-patches.rst
> > 
> > Thanks.
> 
> -- >8 --
> From febedc56cf0e269556e7483a70a3e6cb8d0d5cc3 Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Date: Wed, 24 Nov 2021 21:02:43 +0200
> Subject: [PATCH] net: dsa: sja1105: convert to phylink_generic_validate()
> 
> Provide a ->phylink_get_caps() implementation in order to tell phylink
> what are the PHY modes between which each port can change (none for
> now), and the MAC capabilities so it can limit the advertisement and
> supported masks of the PHY.
> 
> Now that we populate phylink_config->supported_interfaces, it is
> phylink's responsibility to not attempt a PHY mode change towards
> something which we do not support, so we can delete the logic from
> sja1105_phy_mode_mismatch() and move the essence of it into
> sja1105_phylink_get_caps(), which happens much earlier.
> 
> This patch also fixes an inconsequential bug, which was that for ports
> which support 2500base-X, we used to keep advertising the gigabit and
> lower speeds. We should not have done this, because 2500base-X operates
> only at 2500Mbps (and we do not support PAUSE frames in order for the
> lower media speeds to work via rate adaptation). Nonetheless, the only
> SJA1110 boards which use 2500base-X use it in a SERDES-to-SERDES fixed
> link, so there isn't any PHY whose advertisement matters there.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Clearly, you have stopped listening to me. This can no longer be
productive.

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

Re: [PATCH RFC net-next 11/12] net: dsa: sja1105: convert to phylink_generic_validate()

[Vladimir Oltean]

On Wed, Nov 24, 2021 at 11:21:35PM +0000, Russell King (Oracle) wrote:
> Clearly, you have stopped listening to me. This can no longer be
> productive.

What is wrong with the second patch? You said I should split the change
that allows the SERDES protocol to be changed, and I did. You also said
I should make the change in behavior be the first patch, but that it's
up to me, and I decided not to make that change now at all.

As for why I prefer to send you a patch that I am testing, it is to make
the conversion process easier to you. For example you removed a comment
that said this MAC doesn't support flow control, and you declared flow
control in mac_capabilities anyway.

So no, I have not stopped listening to you, can you please tell me what
is not right?

Re: [PATCH RFC net-next 11/12] net: dsa: sja1105: convert to phylink_generic_validate()

[Russell King (Oracle)]

On Wed, Nov 24, 2021 at 11:32:00PM +0000, Vladimir Oltean wrote:
> On Wed, Nov 24, 2021 at 11:21:35PM +0000, Russell King (Oracle) wrote:
> > Clearly, you have stopped listening to me. This can no longer be
> > productive.
> 
> What is wrong with the second patch? You said I should split the change
> that allows the SERDES protocol to be changed, and I did. You also said
> I should make the change in behavior be the first patch, but that it's
> up to me, and I decided not to make that change now at all.
> 
> As for why I prefer to send you a patch that I am testing, it is to make
> the conversion process easier to you. For example you removed a comment
> that said this MAC doesn't support flow control, and you declared flow
> control in mac_capabilities anyway.
> 
> So no, I have not stopped listening to you, can you please tell me what
> is not right?

Let's be clear: I find dealing with you extremely difficult and
stressful. I don't find that with other people, such as Marek or
Andrew. I don't know why this is, but every time we interact, it
quickly becomes confrontational. I don't want this, and the only
way I can see to stop this is to stop interacting with you, which
is obviously also detrimental. I don't have a solution to this.

Now, as for your second patch, it didn't contain a changelog to
indicate what had changed, and it looked like it was merely a re-post
of the previously posted patch. Given how noisy the patch is due to
the size of the changes being made, this is hardly surprising. There
is a reason why we ask for changelogs when patches are modified, and
this is *exactly* why.

Having saved out and diffed the two patches, I can now see the
changes you've made. Now:

-       if (priv->info->supports_2500basex[port]) {
-               phylink_set(mask, 2500baseT_Full);
-               phylink_set(mask, 2500baseX_Full);
+       if (phy_mode == PHY_INTERFACE_MODE_2500BASEX) {
+               config->mac_capabilities = MAC_2500FD;
+       } else if (phy_interface_mode_is_rgmii(phy_mode) ||
+                  phy_mode == PHY_INTERFACE_MODE_SGMII) {
+               config->mac_capabilities = MAC_10FD | MAC_100FD | MAC_1000FD;
+       } else {
+               config->mac_capabilities = MAC_10FD | MAC_100FD;
        }

This limitation according to the interface mode is done by the generic
validation, so is unnecessary unless there really is a restriction on
the capabilities of the MAC.

Given that the generic validation will only permit the 2.5G ethtool
link modes, RGMII and SGMII will permit the 10, 100 and 1G ethtool link
modes, and MII/RevMII/RMII/RevRMII will only permit the 10 and 100
ethtool link modes, recoding this in the get_caps is rather pointless.

This also becomes less obvious that it is a correct conversion - one
can't look at the old validate() code and the new get_caps() code and
check that it's making the same decisions. The old validate code
did:

- allow 10 and 100 FD
- if mii->xmii_mode[port] is XMII_MODE_RGMII or XMII_MODE_SGMII
  - allow 1000 FD
- if priv->info->supports_2500basex[port]
  - allow 2500 FD

The new code bases it off the PHY interface mode, and now one has to
refer to the code in sja1105_init_mii_settings() to see what that is
doing to work out whether it is making equivalent decisions.

In other words, it's changing how the decisions are made concerning
which speeds (whether they are the MAC capabilities or ethtool link
modes) _and_ converting to the new way of specifying those speeds.

I've made the decision to drop the sja1105 patch from this series as
well as ocelot. Do whatever you want there, I no longer care, unless
what you do causes me problems for phylink.

Thanks.

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

Re: [PATCH RFC net-next 11/12] net: dsa: sja1105: convert to phylink_generic_validate()

[Vladimir Oltean]

On Thu, Nov 25, 2021 at 12:56:23PM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 24, 2021 at 11:32:00PM +0000, Vladimir Oltean wrote:
> > On Wed, Nov 24, 2021 at 11:21:35PM +0000, Russell King (Oracle) wrote:
> > > Clearly, you have stopped listening to me. This can no longer be
> > > productive.
> > 
> > What is wrong with the second patch? You said I should split the change
> > that allows the SERDES protocol to be changed, and I did. You also said
> > I should make the change in behavior be the first patch, but that it's
> > up to me, and I decided not to make that change now at all.
> > 
> > As for why I prefer to send you a patch that I am testing, it is to make
> > the conversion process easier to you. For example you removed a comment
> > that said this MAC doesn't support flow control, and you declared flow
> > control in mac_capabilities anyway.
> > 
> > So no, I have not stopped listening to you, can you please tell me what
> > is not right?
> 
> Let's be clear: I find dealing with you extremely difficult and
> stressful. I don't find that with other people, such as Marek or
> Andrew. I don't know why this is, but every time we interact, it
> quickly becomes confrontational.

I assure you that I am no more confrontational with you than with others.
If I say something that is unpleasant, it isn't to bother you, it is
because it bothers me and I'd rather let you know. If what I am saying is
wrong, it's because I don't know any better. I'm sure it's the same for you.

> I don't want this, and the only way I can see to stop this is to stop
> interacting with you, which is obviously also detrimental.

You have practically stopped interacting with me already, I have phylink
patches for broken NXP boards that have been waiting for at least 2
months for you to review them:
https://patchwork.kernel.org/project/netdevbpf/cover/20210922181446.2677089-1-vladimir.oltean@nxp.com/

So yes, it is a problem that is blocking me as well. In fact, you are
also known to say that you're "getting to the point of wishing that
phylink did not have users except my own":
https://patchwork.kernel.org/project/linux-mediatek/cover/20200217172242.GZ25745@shell.armlinux.org.uk/#23436579

This is not what maintainers do, or say, and I'm not sure how a phylink
user is supposed to feel about it. Personally I am absolutely dreading
it, since phylink and Russell King the person are one and the same
thing, and if you don't share the same views as Russell King you are
effectively limited in what you can do. It is not fun.

I honestly don't know why you keep fabricating these strawmen that
conversations become confrontational due to me, it makes it appear to
the poor people on CC watching us fight that I am being overly
aggressive and you are out of strategies to defend. Do I need to remind
you how I was drawn into a totally unrelated discussion about dpaa2-eth
having a deadlock due to the rtnl_mutex being held at phylink_create()
time, and I tried to be helpful and understand the phylink and sfp-bus
design, and you ended up calling me flat-out stupid?
Here:
https://patchwork.kernel.org/project/netdevbpf/patch/20210901225053.1205571-2-vladimir.oltean@nxp.com/

And you said: "if you think that's a valid approach, then quite frankly
I don't want you touching my code, because you clearly don't know what
you're doing as you aren't willing to put the necessary effort in to
understanding the code."

So if you treat in this way a person who is trying to help, then what
help do you need, and why do you complain that you aren't getting more
attention with your phylink conversions? You are Russell King, you don't
need any help.

> I don't have a solution to this.

I think a good start would be to reply strictly to actual code, and to
read code before replying, and try to avoid using phrases such as
"I don't want you touching my code", "I can bring a horse to water but
can't make it drink", "please stop being a problem", "you constantly
bitch and moan".

You might notice that things will start to improve.

> Now, as for your second patch, it didn't contain a changelog to
> indicate what had changed, and it looked like it was merely a re-post
> of the previously posted patch. Given how noisy the patch is due to
> the size of the changes being made, this is hardly surprising. There
> is a reason why we ask for changelogs when patches are modified, and
> this is *exactly* why.

Yes, it didn't contain a changelog. A non-confrontational reaction to
that would have been either to read the patch before commenting, or to
ask for a changelog if it was so noisy as you say it was. But instead,
your reaction was "Clearly, you have stopped listening to me. This can
no longer be productive." Talk about me being confrontational.

It is not even the first time you attack me personally on a patch
without reading it:
https://patchwork.ozlabs.org/project/netdev/patch/20200625152331.3784018-5-olteanv@gmail.com/
This time I didn't even tell you that it's annoying as I did last time,
I just asked what is wrong.

> Having saved out and diffed the two patches, I can now see the
> changes you've made. Now:
> 
> -       if (priv->info->supports_2500basex[port]) {
> -               phylink_set(mask, 2500baseT_Full);
> -               phylink_set(mask, 2500baseX_Full);
> +       if (phy_mode == PHY_INTERFACE_MODE_2500BASEX) {
> +               config->mac_capabilities = MAC_2500FD;
> +       } else if (phy_interface_mode_is_rgmii(phy_mode) ||
> +                  phy_mode == PHY_INTERFACE_MODE_SGMII) {
> +               config->mac_capabilities = MAC_10FD | MAC_100FD | MAC_1000FD;
> +       } else {
> +               config->mac_capabilities = MAC_10FD | MAC_100FD;
>         }
> 
> This limitation according to the interface mode is done by the generic
> validation, so is unnecessary unless there really is a restriction on
> the capabilities of the MAC.
> 
> Given that the generic validation will only permit the 2.5G ethtool
> link modes, RGMII and SGMII will permit the 10, 100 and 1G ethtool link
> modes, and MII/RevMII/RMII/RevRMII will only permit the 10 and 100
> ethtool link modes, recoding this in the get_caps is rather pointless.

This is nitpicking. The variable is called "mac_capabilities" and that
is what I am reporting - MAC capabilities. I know what this variable is
being used for right now, which is limiting the PHY advertisement, but I
don't know what it is going to be used for in the future. Clearly there
is nothing wrong in reporting only the actual capabilities supported by
the MAC. I don't care if phylink_get_linkmodes() will automagically
remove MAC_2500FD for an RMII port, since RMII ports really do not
support MAC_2500FD I don't see why I would report it, in fact, as I've
explained to you in the comments to the ocelot patch, I believe that
phylink_get_linkmodes(), plus the way in which mac_capabilities is used
(specifically, the way in which we report it without it being a function
of PHY mode) does gratuitously break PAUSE-based rate adaptation if we
were to use the generic phylink implementation. I therefore find the
generic validation function to be of limited use.

> This also becomes less obvious that it is a correct conversion - one
> can't look at the old validate() code and the new get_caps() code and
> check that it's making the same decisions. The old validate code
> did:
> 
> - allow 10 and 100 FD
> - if mii->xmii_mode[port] is XMII_MODE_RGMII or XMII_MODE_SGMII
>   - allow 1000 FD
> - if priv->info->supports_2500basex[port]
>   - allow 2500 FD
> 
> The new code bases it off the PHY interface mode, and now one has to
> refer to the code in sja1105_init_mii_settings() to see what that is
> doing to work out whether it is making equivalent decisions.

And after looking at sja1105_init_mii_settings(), are the decisions
equivalent or not? My point being that even if I create a separate patch
which changes the decision making process from mii->xmii_mode to
priv->phy_mode, you'd still need to look at that function.

The patch has my sign off on it, and I tested it. Nobody will blame you
for any breakage. If you don't want to carry it, don't carry it, you
don't even need a reason to.

> In other words, it's changing how the decisions are made concerning
> which speeds (whether they are the MAC capabilities or ethtool link
> modes) _and_ converting to the new way of specifying those speeds.
> 
> I've made the decision to drop the sja1105 patch from this series as
> well as ocelot. Do whatever you want there, I no longer care, unless
> what you do causes me problems for phylink.

This is perfectly acceptable to me.

> Thanks.

No, thank you!

[PATCH RFC net-next 12/12] net: dsa: xrs700x: convert to phylink_generic_validate()

[Russell King (Oracle)]

Populate the supported interfaces and MAC capabilities for the xrs700x
family of DSA switches and remove the old validate implementation to
allow DSA to use phylink_generic_validate() for this switch driver.

According to commit ee00b24f32eb ("net: dsa: add Arrow SpeedChips
XRS700x driver") the switch supports one RMII port and up to three
RGMII ports. This commit assumes that port 0 is the RMII port and the
remainder are RGMII.

This commit also results in the Autoneg bit being set in the ethtool
link modes, which wasn't in the original; if this switch supports
RGMII to a 10/100/1G PHY, then surely we want to allow Autoneg on the
PHY.

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

diff --git a/drivers/net/dsa/xrs700x/xrs700x.c b/drivers/net/dsa/xrs700x/xrs700x.c
index 910fcb3b252b..7a6cfa41900f 100644
--- a/drivers/net/dsa/xrs700x/xrs700x.c
+++ b/drivers/net/dsa/xrs700x/xrs700x.c
@@ -441,34 +441,27 @@ static void xrs700x_teardown(struct dsa_switch *ds)
 	cancel_delayed_work_sync(&priv->mib_work);
 }
 
-static void xrs700x_phylink_validate(struct dsa_switch *ds, int port,
-				     unsigned long *supported,
-				     struct phylink_link_state *state)
+static void xrs700x_phylink_get_caps(struct dsa_switch *ds, int port,
+				     struct phylink_config *config)
 {
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
-
 	switch (port) {
 	case 0:
+		__set_bit(PHY_INTERFACE_MODE_RMII,
+			  config->supported_interfaces);
+		config->mac_capabilities = MAC_10FD | MAC_100FD;
 		break;
+
 	case 1:
 	case 2:
 	case 3:
-		phylink_set(mask, 1000baseT_Full);
+		phy_interface_set_rgmii(config->supported_interfaces);
+		config->mac_capabilities = MAC_10FD | MAC_100FD | MAC_1000FD;
 		break;
+
 	default:
-		linkmode_zero(supported);
 		dev_err(ds->dev, "Unsupported port: %i\n", port);
-		return;
+		break;
 	}
-
-	phylink_set_port_modes(mask);
-
-	/* The switch only supports full duplex. */
-	phylink_set(mask, 10baseT_Full);
-	phylink_set(mask, 100baseT_Full);
-
-	linkmode_and(supported, supported, mask);
-	linkmode_and(state->advertising, state->advertising, mask);
 }
 
 static void xrs700x_mac_link_up(struct dsa_switch *ds, int port,
@@ -702,7 +695,7 @@ static const struct dsa_switch_ops xrs700x_ops = {
 	.setup			= xrs700x_setup,
 	.teardown		= xrs700x_teardown,
 	.port_stp_state_set	= xrs700x_port_stp_state_set,
-	.phylink_validate	= xrs700x_phylink_validate,
+	.phylink_get_caps	= xrs700x_phylink_get_caps,
 	.phylink_mac_link_up	= xrs700x_mac_link_up,
 	.get_strings		= xrs700x_get_strings,
 	.get_sset_count		= xrs700x_get_sset_count,
-- 
2.30.2

Re: [PATCH RFC net-next 00/12] Allow DSA drivers to set all phylink capabilities

[Russell King (Oracle)]

On Wed, Nov 24, 2021 at 05:46:01PM +0000, Russell King (Oracle) wrote:
> During the last cycle, I introduced a phylink_get_interfaces() method
> for DSA drivers to be able to fill out the supported_interfaces member
> of phylink_config. However, further phylink development allowing the
> validation hook to be greatly simplified became possible when a bitmask
> of MAC capabilities is used along with the supported_interfaces bitmap.
...

Hi all,

Patches 1 through 3, 6 and 8 have been merged, the rest have not.
Getting patches 4, 5, 7, 10 and 12 tested and reviewed would be great
please. These are ar9331, bcm_sf2, ksz8795, qca8k and xrs700x. Thanks!

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

Re: [PATCH RFC net-next 00/12] Allow DSA drivers to set all phylink capabilities

[Florian Fainelli]

On 12/3/21 8:15 AM, Russell King (Oracle) wrote:
> On Wed, Nov 24, 2021 at 05:46:01PM +0000, Russell King (Oracle) wrote:
>> During the last cycle, I introduced a phylink_get_interfaces() method
>> for DSA drivers to be able to fill out the supported_interfaces member
>> of phylink_config. However, further phylink development allowing the
>> validation hook to be greatly simplified became possible when a bitmask
>> of MAC capabilities is used along with the supported_interfaces bitmap.
> ...
> 
> Hi all,
> 
> Patches 1 through 3, 6 and 8 have been merged, the rest have not.
> Getting patches 4, 5, 7, 10 and 12 tested and reviewed would be great
> please. These are ar9331, bcm_sf2, ksz8795, qca8k and xrs700x. Thanks!

Do you have a re-based version that does not conflict with what is
currently in net-next?
-- 
Florian

Re: [PATCH RFC net-next 00/12] Allow DSA drivers to set all phylink capabilities

[Florian Fainelli]

On 12/3/21 11:28 AM, Florian Fainelli wrote:
> On 12/3/21 8:15 AM, Russell King (Oracle) wrote:
>> On Wed, Nov 24, 2021 at 05:46:01PM +0000, Russell King (Oracle) wrote:
>>> During the last cycle, I introduced a phylink_get_interfaces() method
>>> for DSA drivers to be able to fill out the supported_interfaces member
>>> of phylink_config. However, further phylink development allowing the
>>> validation hook to be greatly simplified became possible when a bitmask
>>> of MAC capabilities is used along with the supported_interfaces bitmap.
>> ...
>>
>> Hi all,
>>
>> Patches 1 through 3, 6 and 8 have been merged, the rest have not.
>> Getting patches 4, 5, 7, 10 and 12 tested and reviewed would be great
>> please. These are ar9331, bcm_sf2, ksz8795, qca8k and xrs700x. Thanks!
> 
> Do you have a re-based version that does not conflict with what is
> currently in net-next?

Nevermind, grabbed the patches of interest and will report back on the
testing.
-- 
Florian