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

[PATCH 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.

RFC->non-RFC: added Vladimir's Reviewed-by's, fixed the typo in the
commit message of patch 6, and removed the phrase at the end of a
comment as requested.

 drivers/net/dsa/sja1105/sja1105_main.c | 100 ++++++++++++++-------------------
 1 file changed, 42 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 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.

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>                                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 net-next 1/6] net: dsa: sja1105: populate supported_interfaces

[Russell King (Oracle)]

On Fri, Feb 25, 2022 at 11:56:02AM +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.
> 
> Reviewed-by: Vladimir Oltean <olteanv@gmail.com>                                Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Bl"%$y vi!

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

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

[Vladimir Oltean]

On Fri, Feb 25, 2022 at 11:56:02AM +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.
> 
> Reviewed-by: Vladimir Oltean <olteanv@gmail.com>                                Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

These all appear on the same line, can you please fix and resend?

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

[Russell King (Oracle)]

On Fri, Feb 25, 2022 at 01:58:58PM +0200, Vladimir Oltean wrote:
> On Fri, Feb 25, 2022 at 11:56:02AM +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.
> > 
> > Reviewed-by: Vladimir Oltean <olteanv@gmail.com>                                Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> 
> These all appear on the same line, can you please fix and resend?

I hate vi/vim precisely because of this.

How this problem happens. I read the email in mutt under KDE's kconsole
with your attributation. I select the attributation so it can be pasted.
I edit the commit, which starts vi, move to the line containing my
sign-off, hit 'i' and paste it in.

The result is a line that _looks_ in vi as being entirely correct. The
next line follows on as if it is a separate line.

I save the commit message. When I look at it in "git log", again,
everything looks good.

The only times that it can be identified is after sending and looking
at it in mutt, and noticing that the Signed-off-by line appears to have
a# '+' prefix, indicating that mutt wrapped the line - or after it gets
merged into net-next when linux-next identifies the lack of s-o-b, by
which time it's too late to fix.

How do others avoid this problem? Not use vi/vim, but use some other
editor such as emacs or microemacs that doesn't have this crazy way of
dealing with multiple lines?

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

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

[Vladimir Oltean]

On Fri, Feb 25, 2022 at 12:08:02PM +0000, Russell King (Oracle) wrote:
> On Fri, Feb 25, 2022 at 01:58:58PM +0200, Vladimir Oltean wrote:
> > On Fri, Feb 25, 2022 at 11:56:02AM +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.
> > > 
> > > Reviewed-by: Vladimir Oltean <olteanv@gmail.com>                                Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > 
> > These all appear on the same line, can you please fix and resend?
> 
> I hate vi/vim precisely because of this.
> 
> How this problem happens. I read the email in mutt under KDE's kconsole
> with your attributation. I select the attributation so it can be pasted.
> I edit the commit, which starts vi, move to the line containing my
> sign-off, hit 'i' and paste it in.
> 
> The result is a line that _looks_ in vi as being entirely correct. The
> next line follows on as if it is a separate line.
> 
> I save the commit message. When I look at it in "git log", again,
> everything looks good.
> 
> The only times that it can be identified is after sending and looking
> at it in mutt, and noticing that the Signed-off-by line appears to have
> a# '+' prefix, indicating that mutt wrapped the line - or after it gets
> merged into net-next when linux-next identifies the lack of s-o-b, by
> which time it's too late to fix.
> 
> How do others avoid this problem? Not use vi/vim, but use some other
> editor such as emacs or microemacs that doesn't have this crazy way of
> dealing with multiple lines?

I do this in vim all the time and never had this problem.
Maybe you're not realizing it's on the same line because you don't have
line numbers turned on? A long line wrapped by the vim viewer would be
obvious.

:set number

Compare (just a random bit of a commit message)

 20 Fixes: 869f0ec048dc ("arm64: dts: freescale: Fix 'interrupt-map' parent address cells")           │
 21 Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Signed-off-by: Vladimir Oltean <vladimir.│
    oltean@nxp.com>                                                                                   │
 22

with

Fixes: 869f0ec048dc ("arm64: dts: freescale: Fix 'interrupt-map' parent address cells")               │
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Signed-off-by: Vladimir Oltean <vladimir.    │
oltean@nxp.com>                                                                                       │

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

[Russell King (Oracle)]

On Fri, Feb 25, 2022 at 02:15:56PM +0200, Vladimir Oltean wrote:
> On Fri, Feb 25, 2022 at 12:08:02PM +0000, Russell King (Oracle) wrote:
> > On Fri, Feb 25, 2022 at 01:58:58PM +0200, Vladimir Oltean wrote:
> > > On Fri, Feb 25, 2022 at 11:56:02AM +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.
> > > > 
> > > > Reviewed-by: Vladimir Oltean <olteanv@gmail.com>                                Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > 
> > > These all appear on the same line, can you please fix and resend?
> > 
> > I hate vi/vim precisely because of this.
> > 
> > How this problem happens. I read the email in mutt under KDE's kconsole
> > with your attributation. I select the attributation so it can be pasted.
> > I edit the commit, which starts vi, move to the line containing my
> > sign-off, hit 'i' and paste it in.
> > 
> > The result is a line that _looks_ in vi as being entirely correct. The
> > next line follows on as if it is a separate line.
> > 
> > I save the commit message. When I look at it in "git log", again,
> > everything looks good.
> > 
> > The only times that it can be identified is after sending and looking
> > at it in mutt, and noticing that the Signed-off-by line appears to have
> > a# '+' prefix, indicating that mutt wrapped the line - or after it gets
> > merged into net-next when linux-next identifies the lack of s-o-b, by
> > which time it's too late to fix.
> > 
> > How do others avoid this problem? Not use vi/vim, but use some other
> > editor such as emacs or microemacs that doesn't have this crazy way of
> > dealing with multiple lines?
> 
> I do this in vim all the time and never had this problem.
> Maybe you're not realizing it's on the same line because you don't have
> line numbers turned on? A long line wrapped by the vim viewer would be
> obvious.
> 
> :set number

Thanks - Peter Zijlstra suggested "set showbreak=+" which works
wonderfully - gives the same indication that mutt does when reading
email that a line has wrapped and without being too intrusive but
makes the wrap visible.

The issue appears to be caused by a combination effect: not having
vim setup to identify wrapped lines, kconsole's copy-n-paste
behaviour where it attempts to replicate exactly what the program
in the terminal's output (even if it includes whitespace to the end
of the line), and probably the combination of mutt under screen under
mosh, resulting in the terminal thinking that the program wrote
"Reviewed-By: Joe Bloggs <joe@example.com>[large-number-of-space-characters]"

Anyway, "set showbreak=+" seems to be perfect.

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

[PATCH 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.

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>                                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

[PATCH 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.

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>                                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

[PATCH 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.

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>                                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

[PATCH 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.

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>                                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

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

[Russell King (Oracle)]

Vladimir Oltean suggests that sja1105 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 | 27 +++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 5beef06d8ff7..8fc309446e1e 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,27 @@ 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.
+		 */
+		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

[PATCH net-next v2 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.

RFC->non-RFC: added Vladimir's Reviewed-by's, fixed the typo in the
commit message of patch 6, and removed the phrase at the end of a
comment as requested.

v2: fix the fscking vi fsckups when pasting in attributations.

 drivers/net/dsa/sja1105/sja1105_main.c | 100 ++++++++++++++-------------------
 1 file changed, 42 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 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.

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
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