[PATCH RFC net-next 0/6] net: dsa: sja1105: phylink updates

[PATCH RFC net-next 0/6] net: dsa: sja1105: phylink updates

[Russell King (Oracle)]

Hi,

This series updates the phylink implementation in sja1105 to use the
supported_interfaces bitmap, convert to the mac_select_pcs() interface,
mark as non-legacy, and get rid of the validation method.

As a final step, enable switching between SGMII and 2500BASE-X as it
is a feature that Vladimir desires.

Specifically, the patches in this series:

1. Populates the supported_interfaces bitmap.
2. As a result of the supported_interfaces bitmap being populated,
   sja1105 no longer needs to check the interface mode as phylink
   will do this.
3. Switch away from using phylink_set_pcs(), using the mac_select_pcs()
   method instead.
4. Mark the driver as not-legacy
5. Fill in mac_capabilities using _exactly_ the same conditions as is
   currently used to decide which link modes to support, and convert
   to use phylink_generic_validate()
6. Add brand new support to permit switching between SGMII and
   2500BASE-X modes of operation as per Vladimir's single patch that
   performs steps 1, 2, 5 and 6 in one go.

There are some additional changes in Vladimir's single patch that I
have not included:

* validation of priv->phy_mode[] in sja1105_phylink_get_caps(). The
  driver has already validated the phy_mode for each port in
  sja1105_init_mii_settings(), and a failure here will prevent the
  driver reaching sja1105_phylink_get_caps().

* Changing the decisions on which mac_capabilities to set. Vladimir's
  patch always sets MAC_10FD | MAC_100FD | MAC_1000FD despite the
  current code clearly making the 1G speed conditional on the
  xmii_mode for the port. The change in decision making may be
  visible when in PHY_INTERFACE_MODE_INTERNAL mode, for which
  the phylink_generic_validate() will pass through all the MAC
  capabilities as ethtool link modes.

  Hence, if we have PHY_INTERFACE_MODE_INTERNAL but supports_rgmii[]
  or supports_sgmii[] is non-zero, currently we do not get 1G speeds.
  With Vladimir's additional change, we will get 1G speeds.

  While it is not clear whether that can happen, I feel changing the
  decision making should be a separate patch.

* The decision for MAC_2500FD is made differently -
  sja1105_init_mii_settings() allows PHY_INTERFACE_MODE_2500BASEX
  when supports_2500basex[] is non-zero, and is not based on any other
  condition such as supports_sgmii[] or supports_rgmii[]. Vladimir's
  patch makes it additionally conditional on those supports_.gmii[]
  settings, which is a functional change that should be made in a
  separate patch - and if desired, then sja1105_init_mii_settings()
  should also be updated at the same time.

Consequently, I believe that my previous objections to Vladimir's
single patch approach are well founded and justified, even through
Vladimir is the maintainer of this driver. I have no objection to
the additional changes, I just don't think they should all be wrapped
up into a single patch that converts the way validation is done _and_
also makes a bunch of other functional changes.

 drivers/net/dsa/sja1105/sja1105_main.c | 101 ++++++++++++++-------------------
 1 file changed, 43 insertions(+), 58 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 1/6] net: dsa: sja1105: populate supported_interfaces

[Russell King (Oracle)]

Populate the supported interfaces bitmap for the SJA1105 DSA switch.

This switch only supports a static model of configuration, so we
restrict the interface modes to the configured setting.

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

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index b513713be610..90e73a982faf 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1412,6 +1412,18 @@ static void sja1105_mac_link_up(struct dsa_switch *ds, int port,
 	sja1105_inhibit_tx(priv, BIT(port), false);
 }
 
+static void sja1105_phylink_get_caps(struct dsa_switch *ds, int port,
+				     struct phylink_config *config)
+{
+	struct sja1105_private *priv = ds->priv;
+
+	/* 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.
+	 */
+	__set_bit(priv->phy_mode[port], config->supported_interfaces);
+}
+
 static void sja1105_phylink_validate(struct dsa_switch *ds, int port,
 				     unsigned long *supported,
 				     struct phylink_link_state *state)
@@ -3152,6 +3164,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_get_caps	= sja1105_phylink_get_caps,
 	.phylink_validate	= sja1105_phylink_validate,
 	.phylink_mac_config	= sja1105_mac_config,
 	.phylink_mac_link_up	= sja1105_mac_link_up,
-- 
2.30.2

Re: [PATCH RFC net-next 1/6] net: dsa: sja1105: populate supported_interfaces

[Vladimir Oltean]

On Thu, Feb 24, 2022 at 04:15:15PM +0000, Russell King (Oracle) wrote:
> Populate the supported interfaces bitmap for the SJA1105 DSA switch.
> 
> This switch only supports a static model of configuration, so we
> restrict the interface modes to the configured setting.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

[PATCH RFC net-next 2/6] net: dsa: sja1105: remove interface checks

[Russell King (Oracle)]

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

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

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 90e73a982faf..e278bd86e3c6 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1358,19 +1358,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)
@@ -1379,12 +1366,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)
@@ -1438,16 +1419,6 @@ static void sja1105_phylink_validate(struct dsa_switch *ds, int port,
 
 	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);
-		return;
-	}
-
 	/* The MAC does not support pause frames, and also doesn't
 	 * support half-duplex traffic modes.
 	 */
-- 
2.30.2

Re: [PATCH RFC net-next 2/6] net: dsa: sja1105: remove interface checks

[Vladimir Oltean]

On Thu, Feb 24, 2022 at 04:15:21PM +0000, Russell King (Oracle) wrote:
> When the supported interfaces bitmap is populated, phylink will itself
> check that the interface mode is present in this bitmap. Drivers no
> longer need to perform this check themselves. Remove these checks.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

[PATCH RFC net-next 3/6] net: dsa: sja1105: use .mac_select_pcs() interface

[Russell King (Oracle)]

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

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

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index e278bd86e3c6..b5c36f808df1 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1358,18 +1358,16 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port,
 	return sja1105_clocking_setup_port(priv, port);
 }
 
-static void sja1105_mac_config(struct dsa_switch *ds, int port,
-			       unsigned int mode,
-			       const struct phylink_link_state *state)
+static struct phylink_pcs *
+sja1105_mac_select_pcs(struct dsa_switch *ds, int port, phy_interface_t iface)
 {
-	struct dsa_port *dp = dsa_to_port(ds, port);
 	struct sja1105_private *priv = ds->priv;
-	struct dw_xpcs *xpcs;
-
-	xpcs = priv->xpcs[port];
+	struct dw_xpcs *xpcs = priv->xpcs[port];
 
 	if (xpcs)
-		phylink_set_pcs(dp->pl, &xpcs->pcs);
+		return &xpcs->pcs;
+
+	return NULL;
 }
 
 static void sja1105_mac_link_down(struct dsa_switch *ds, int port,
@@ -3137,7 +3135,7 @@ static const struct dsa_switch_ops sja1105_switch_ops = {
 	.port_max_mtu		= sja1105_get_max_mtu,
 	.phylink_get_caps	= sja1105_phylink_get_caps,
 	.phylink_validate	= sja1105_phylink_validate,
-	.phylink_mac_config	= sja1105_mac_config,
+	.phylink_mac_select_pcs	= sja1105_mac_select_pcs,
 	.phylink_mac_link_up	= sja1105_mac_link_up,
 	.phylink_mac_link_down	= sja1105_mac_link_down,
 	.get_strings		= sja1105_get_strings,
-- 
2.30.2

Re: [PATCH RFC net-next 3/6] net: dsa: sja1105: use .mac_select_pcs() interface

[Vladimir Oltean]

On Thu, Feb 24, 2022 at 04:15:26PM +0000, Russell King (Oracle) wrote:
> Convert the PCS selection to use mac_select_pcs, which allows the PCS
> to perform any validation it needs, and removes the need to set the PCS
> in the mac_config() callback, delving into the higher DSA levels to do
> so.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

>  drivers/net/dsa/sja1105/sja1105_main.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
> index e278bd86e3c6..b5c36f808df1 100644
> --- a/drivers/net/dsa/sja1105/sja1105_main.c
> +++ b/drivers/net/dsa/sja1105/sja1105_main.c
> @@ -1358,18 +1358,16 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port,
>  	return sja1105_clocking_setup_port(priv, port);
>  }
>  
> -static void sja1105_mac_config(struct dsa_switch *ds, int port,
> -			       unsigned int mode,
> -			       const struct phylink_link_state *state)
> +static struct phylink_pcs *
> +sja1105_mac_select_pcs(struct dsa_switch *ds, int port, phy_interface_t iface)
>  {
> -	struct dsa_port *dp = dsa_to_port(ds, port);
>  	struct sja1105_private *priv = ds->priv;
> -	struct dw_xpcs *xpcs;
> -
> -	xpcs = priv->xpcs[port];
> +	struct dw_xpcs *xpcs = priv->xpcs[port];
>  
>  	if (xpcs)
> -		phylink_set_pcs(dp->pl, &xpcs->pcs);
> +		return &xpcs->pcs;
> +
> +	return NULL;
>  }
>  
>  static void sja1105_mac_link_down(struct dsa_switch *ds, int port,
> @@ -3137,7 +3135,7 @@ static const struct dsa_switch_ops sja1105_switch_ops = {
>  	.port_max_mtu		= sja1105_get_max_mtu,
>  	.phylink_get_caps	= sja1105_phylink_get_caps,
>  	.phylink_validate	= sja1105_phylink_validate,
> -	.phylink_mac_config	= sja1105_mac_config,

Deleting sja1105_mac_config() here is safe not because
phylink_mac_config() stops calling pl->mac_ops->mac_config(), but
because dsa_port_phylink_mac_config() first checks whether
ds->ops->phylink_mac_config is implemented, and that is purely an
artefact of providing a phylib-style ds->ops->adjust_link, right?

Maybe it's worth mentioning.

> +	.phylink_mac_select_pcs	= sja1105_mac_select_pcs,
>  	.phylink_mac_link_up	= sja1105_mac_link_up,
>  	.phylink_mac_link_down	= sja1105_mac_link_down,
>  	.get_strings		= sja1105_get_strings,
> -- 
> 2.30.2
> 

Re: [PATCH RFC net-next 3/6] net: dsa: sja1105: use .mac_select_pcs() interface

[Russell King (Oracle)]

On Fri, Feb 25, 2022 at 12:39:13PM +0200, Vladimir Oltean wrote:
> On Thu, Feb 24, 2022 at 04:15:26PM +0000, Russell King (Oracle) wrote:
> > Convert the PCS selection to use mac_select_pcs, which allows the PCS
> > to perform any validation it needs, and removes the need to set the PCS
> > in the mac_config() callback, delving into the higher DSA levels to do
> > so.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> 
> Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Thanks.

> > -	.phylink_mac_config	= sja1105_mac_config,
> 
> Deleting sja1105_mac_config() here is safe not because
> phylink_mac_config() stops calling pl->mac_ops->mac_config(), but
> because dsa_port_phylink_mac_config() first checks whether
> ds->ops->phylink_mac_config is implemented, and that is purely an
> artefact of providing a phylib-style ds->ops->adjust_link, right?

Yes and no.

We already have a several DSA drivers that have NULL phylink_mac_config
and that don't provide an adjust_link function. Even if adjust_link was
eventually killed off, the test in dsa_port_phylink_mac_config() would
still be necessary unless all these DSA drivers are updated with a stub
function for it.

Consequently, I view phylink_mac_config in DSA as entirely optional and
that optionality is already very much a part of the DSA interface, even
though that is not the case with the corresponding phylink_mac_ops
.mac_config method.

Moreover, this optionality is a common theme in DSA switch operations
methods.

> Maybe it's worth mentioning.

Given that .phylink_mac_config is already established as being optional
in DSA, does the addition of one more instance need to be explicitly
mentioned?

-- 
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 3/6] net: dsa: sja1105: use .mac_select_pcs() interface

[Vladimir Oltean]

On Fri, Feb 25, 2022 at 10:57:40AM +0000, Russell King (Oracle) wrote:
> > > -	.phylink_mac_config	= sja1105_mac_config,
> > 
> > Deleting sja1105_mac_config() here is safe not because
> > phylink_mac_config() stops calling pl->mac_ops->mac_config(), but
> > because dsa_port_phylink_mac_config() first checks whether
> > ds->ops->phylink_mac_config is implemented, and that is purely an
> > artefact of providing a phylib-style ds->ops->adjust_link, right?
> 
> Yes and no.
> 
> We already have a several DSA drivers that have NULL phylink_mac_config
> and that don't provide an adjust_link function. Even if adjust_link was
> eventually killed off, the test in dsa_port_phylink_mac_config() would
> still be necessary unless all these DSA drivers are updated with a stub
> function for it.
> 
> Consequently, I view phylink_mac_config in DSA as entirely optional and
> that optionality is already very much a part of the DSA interface, even
> though that is not the case with the corresponding phylink_mac_ops
> .mac_config method.
> 
> Moreover, this optionality is a common theme in DSA switch operations
> methods.
> 
> > Maybe it's worth mentioning.
> 
> Given that .phylink_mac_config is already established as being optional
> in DSA, does the addition of one more instance need to be explicitly
> mentioned?

I am aware of that, I am just pointing out that this is an unintended
side effect of the existence of adjust_link, and non-DSA phylink drivers
still need the mac_config stub. When going back and forth between a DSA
and a non-DSA driver, this is not obvious until you take into account
the dsa_port_phylink_mac_config() trampoline. Anyway, don't mention it
if you don't think it is necessary.

[PATCH RFC net-next 4/6] net: dsa: sja1105: mark as non-legacy

[Russell King (Oracle)]

The sja1105 DSA driver does not have a phylink_mac_config() method
implementation, it is safe to mark this as a non-legacy driver.

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

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index b5c36f808df1..8f061cce77f0 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1396,6 +1396,12 @@ static void sja1105_phylink_get_caps(struct dsa_switch *ds, int port,
 {
 	struct sja1105_private *priv = ds->priv;
 
+	/* This driver does not make use of the speed, duplex, pause or the
+	 * advertisement in its mac_config, so it is safe to mark this driver
+	 * as non-legacy.
+	 */
+	config->legacy_pre_march2020 = false;
+
 	/* 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.
-- 
2.30.2

Re: [PATCH RFC net-next 4/6] net: dsa: sja1105: mark as non-legacy

[Vladimir Oltean]

On Thu, Feb 24, 2022 at 04:15:31PM +0000, Russell King (Oracle) wrote:
> The sja1105 DSA driver does not have a phylink_mac_config() method
> implementation, it is safe to mark this as a non-legacy driver.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

>  drivers/net/dsa/sja1105/sja1105_main.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
> index b5c36f808df1..8f061cce77f0 100644
> --- a/drivers/net/dsa/sja1105/sja1105_main.c
> +++ b/drivers/net/dsa/sja1105/sja1105_main.c
> @@ -1396,6 +1396,12 @@ static void sja1105_phylink_get_caps(struct dsa_switch *ds, int port,
>  {
>  	struct sja1105_private *priv = ds->priv;
>  
> +	/* This driver does not make use of the speed, duplex, pause or the
> +	 * advertisement in its mac_config, so it is safe to mark this driver
> +	 * as non-legacy.
> +	 */
> +	config->legacy_pre_march2020 = false;
> +
>  	/* 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.
> -- 
> 2.30.2
> 

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

[Russell King (Oracle)]

Populate the MAC capabilities for the SJA1105 DSA switch using the same
decision making which sja1105_phylink_validate() uses. Remove the now
obsolete sja1105_phylink_validate() implementation to allow DSA to use
phylink_generic_validate() for this switch driver.

As noted by Vladimir, this fixes an inconsequential bug which allowed
gigabit and lower interface modes to be indicated when operating in
2500base-X mode.

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

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 8f061cce77f0..5beef06d8ff7 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1395,6 +1395,7 @@ static void sja1105_phylink_get_caps(struct dsa_switch *ds, int port,
 				     struct phylink_config *config)
 {
 	struct sja1105_private *priv = ds->priv;
+	struct sja1105_xmii_params_entry *mii;
 
 	/* This driver does not make use of the speed, duplex, pause or the
 	 * advertisement in its mac_config, so it is safe to mark this driver
@@ -1407,40 +1408,19 @@ static void sja1105_phylink_get_caps(struct dsa_switch *ds, int port,
 	 * we have to program that early.
 	 */
 	__set_bit(priv->phy_mode[port], config->supported_interfaces);
-}
-
-static void sja1105_phylink_validate(struct dsa_switch *ds, int port,
-				     unsigned long *supported,
-				     struct phylink_link_state *state)
-{
-	/* 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;
 
 	/* 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);
+	config->mac_capabilities = MAC_10FD | MAC_100FD;
+
+	mii = priv->static_config.tables[BLK_IDX_XMII_PARAMS].entries;
 	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
@@ -3140,7 +3120,6 @@ static const struct dsa_switch_ops sja1105_switch_ops = {
 	.port_change_mtu	= sja1105_change_mtu,
 	.port_max_mtu		= sja1105_get_max_mtu,
 	.phylink_get_caps	= sja1105_phylink_get_caps,
-	.phylink_validate	= sja1105_phylink_validate,
 	.phylink_mac_select_pcs	= sja1105_mac_select_pcs,
 	.phylink_mac_link_up	= sja1105_mac_link_up,
 	.phylink_mac_link_down	= sja1105_mac_link_down,
-- 
2.30.2

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

[Vladimir Oltean]

On Thu, Feb 24, 2022 at 04:15:36PM +0000, Russell King (Oracle) wrote:
> Populate the MAC capabilities for the SJA1105 DSA switch using the same
> decision making which sja1105_phylink_validate() uses. Remove the now
> obsolete sja1105_phylink_validate() implementation to allow DSA to use
> phylink_generic_validate() for this switch driver.
> 
> As noted by Vladimir, this fixes an inconsequential bug which allowed
> gigabit and lower interface modes to be indicated when operating in
> 2500base-X mode.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

[PATCH RFC net-next 6/6] net: dsa: sja1105: support switching between SGMII and 2500BASE-X

[Russell King (Oracle)]

Vladimir Oltean suggests that sla1105 can support switching between
SGMII and 2500BASE-X modes. Augment sja1105_phylink_get_caps() to
fill in both interface modes if they can be supported.

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

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 5beef06d8ff7..36001b1d7968 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1396,6 +1396,7 @@ static void sja1105_phylink_get_caps(struct dsa_switch *ds, int port,
 {
 	struct sja1105_private *priv = ds->priv;
 	struct sja1105_xmii_params_entry *mii;
+	phy_interface_t phy_mode;
 
 	/* This driver does not make use of the speed, duplex, pause or the
 	 * advertisement in its mac_config, so it is safe to mark this driver
@@ -1403,11 +1404,28 @@ static void sja1105_phylink_get_caps(struct dsa_switch *ds, int port,
 	 */
 	config->legacy_pre_march2020 = false;
 
-	/* 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.
-	 */
-	__set_bit(priv->phy_mode[port], config->supported_interfaces);
+	phy_mode = priv->phy_mode[port];
+	if (phy_mode == PHY_INTERFACE_MODE_SGMII ||
+	    phy_mode == 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);
+	} else {
+		/* 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.
+		 */
+		__set_bit(phy_mode, config->supported_interfaces);
+	}
 
 	/* The MAC does not support pause frames, and also doesn't
 	 * support half-duplex traffic modes.
-- 
2.30.2

Re: [PATCH RFC net-next 6/6] net: dsa: sja1105: support switching between SGMII and 2500BASE-X

[Vladimir Oltean]

On Thu, Feb 24, 2022 at 04:15:41PM +0000, Russell King (Oracle) wrote:
> Vladimir Oltean suggests that sla1105 can support switching between

s/sla1105/sja1105/

> SGMII and 2500BASE-X modes. Augment sja1105_phylink_get_caps() to
> fill in both interface modes if they can be supported.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/dsa/sja1105/sja1105_main.c | 28 +++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
> index 5beef06d8ff7..36001b1d7968 100644
> --- a/drivers/net/dsa/sja1105/sja1105_main.c
> +++ b/drivers/net/dsa/sja1105/sja1105_main.c
> @@ -1396,6 +1396,7 @@ static void sja1105_phylink_get_caps(struct dsa_switch *ds, int port,
>  {
>  	struct sja1105_private *priv = ds->priv;
>  	struct sja1105_xmii_params_entry *mii;
> +	phy_interface_t phy_mode;
>  
>  	/* This driver does not make use of the speed, duplex, pause or the
>  	 * advertisement in its mac_config, so it is safe to mark this driver
> @@ -1403,11 +1404,28 @@ static void sja1105_phylink_get_caps(struct dsa_switch *ds, int port,
>  	 */
>  	config->legacy_pre_march2020 = false;
>  
> -	/* 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.
> -	 */
> -	__set_bit(priv->phy_mode[port], config->supported_interfaces);
> +	phy_mode = priv->phy_mode[port];
> +	if (phy_mode == PHY_INTERFACE_MODE_SGMII ||
> +	    phy_mode == 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).
> +		 */

It is actually known (or so I think).
Bits 2:1 (PCS_MODE) of register VR_MII_AN_CTRL (MMD 0x1f, address 0x8001)
of the XPCS, as instantiated in SJA1105R/S, says:
00: Clause 37 auto-negotiation for 1000BASE-X mode
    *Not supported*
10: Clause 37 auto-negotiation for SGMII mode

When I look at the XPCS documentation for SJA1110, it doesn't say
"Not supported", however I don't have the setup to try it.
If it's anything like the XPCS instantiation from SJA1105 though, this
is possibly a documentation glitch and I wouldn't say it was implemented
just because the documentation doesn't say it isn't.

On the other hand, disabling SGMII in-band autoneg is possible, and the
resulting mode is electrically compatible with 1000Base-X without
in-band autoneg. Interpret this as you wish.

> +		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);
> +	} else {
> +		/* 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.
> +		 */
> +		__set_bit(phy_mode, config->supported_interfaces);
> +	}
>  
>  	/* The MAC does not support pause frames, and also doesn't
>  	 * support half-duplex traffic modes.
> -- 
> 2.30.2
> 

Re: [PATCH RFC net-next 6/6] net: dsa: sja1105: support switching between SGMII and 2500BASE-X

[Russell King (Oracle)]

On Fri, Feb 25, 2022 at 01:16:49PM +0200, Vladimir Oltean wrote:
> On Thu, Feb 24, 2022 at 04:15:41PM +0000, Russell King (Oracle) wrote:
> > Vladimir Oltean suggests that sla1105 can support switching between
> 
> s/sla1105/sja1105/

Thanks for catching that.

> > SGMII and 2500BASE-X modes. Augment sja1105_phylink_get_caps() to
> > fill in both interface modes if they can be supported.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/dsa/sja1105/sja1105_main.c | 28 +++++++++++++++++++++-----
> >  1 file changed, 23 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
> > index 5beef06d8ff7..36001b1d7968 100644
> > --- a/drivers/net/dsa/sja1105/sja1105_main.c
> > +++ b/drivers/net/dsa/sja1105/sja1105_main.c
> > @@ -1396,6 +1396,7 @@ static void sja1105_phylink_get_caps(struct dsa_switch *ds, int port,
> >  {
> >  	struct sja1105_private *priv = ds->priv;
> >  	struct sja1105_xmii_params_entry *mii;
> > +	phy_interface_t phy_mode;
> >  
> >  	/* This driver does not make use of the speed, duplex, pause or the
> >  	 * advertisement in its mac_config, so it is safe to mark this driver
> > @@ -1403,11 +1404,28 @@ static void sja1105_phylink_get_caps(struct dsa_switch *ds, int port,
> >  	 */
> >  	config->legacy_pre_march2020 = false;
> >  
> > -	/* 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.
> > -	 */
> > -	__set_bit(priv->phy_mode[port], config->supported_interfaces);
> > +	phy_mode = priv->phy_mode[port];
> > +	if (phy_mode == PHY_INTERFACE_MODE_SGMII ||
> > +	    phy_mode == 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).
> > +		 */
> 
> It is actually known (or so I think).
> Bits 2:1 (PCS_MODE) of register VR_MII_AN_CTRL (MMD 0x1f, address 0x8001)
> of the XPCS, as instantiated in SJA1105R/S, says:
> 00: Clause 37 auto-negotiation for 1000BASE-X mode
>     *Not supported*
> 10: Clause 37 auto-negotiation for SGMII mode
> 
> When I look at the XPCS documentation for SJA1110, it doesn't say
> "Not supported", however I don't have the setup to try it.
> If it's anything like the XPCS instantiation from SJA1105 though, this
> is possibly a documentation glitch and I wouldn't say it was implemented
> just because the documentation doesn't say it isn't.
> 
> On the other hand, disabling SGMII in-band autoneg is possible, and the
> resulting mode is electrically compatible with 1000Base-X without
> in-band autoneg. Interpret this as you wish.

The comment above comes directly from your patch back in November.
Are you suggesting you aren't happy with your own comment? If you
would like to update it, please let me have a suitable replacement
for it.

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 6/6] net: dsa: sja1105: support switching between SGMII and 2500BASE-X

[Vladimir Oltean]

On Fri, Feb 25, 2022 at 11:23:20AM +0000, Russell King (Oracle) wrote:
> The comment above comes directly from your patch back in November.
> Are you suggesting you aren't happy with your own comment? If you
> would like to update it, please let me have a suitable replacement
> for it.

Yes, I'm not happy with my own comment, just remove the text between the
parentheses. Thanks.