net: dsa: sja1105: phylink updates
cover net: dsa: sja1105: phylink updates
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(-)
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
PATCH 1/6 net: dsa: sja1105: populate supported_interfaces
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.
--- drivers/net/dsa/sja1105/sja1105_main.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
drivers/net/dsa/sja1105/sja1105_main.c +13 -0
@@ -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,
Signature · 1 line
2.30.2
PATCH 2/6 net: dsa: sja1105: remove interface checks
In reply to 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.
--- drivers/net/dsa/sja1105/sja1105_main.c | 29 -------------------------- 1 file changed, 29 deletions(-)
drivers/net/dsa/sja1105/sja1105_main.c +0 -29
@@ -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. */
Signature · 1 line
2.30.2
PATCH 3/6 net: dsa: sja1105: use .mac_select_pcs() interface
In reply to 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.
--- drivers/net/dsa/sja1105/sja1105_main.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)
drivers/net/dsa/sja1105/sja1105_main.c +7 -9
@@ -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,
Signature · 1 line
2.30.2
PATCH 4/6 net: dsa: sja1105: mark as non-legacy
In reply to 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.
--- drivers/net/dsa/sja1105/sja1105_main.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/net/dsa/sja1105/sja1105_main.c +6 -0
@@ -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.
Signature · 1 line
2.30.2
PATCH 5/6 net: dsa: sja1105: convert to phylink_generic_validate()
In reply to 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.
--- drivers/net/dsa/sja1105/sja1105_main.c | 35 ++++++-------------------- 1 file changed, 7 insertions(+), 28 deletions(-)
drivers/net/dsa/sja1105/sja1105_main.c +7 -28
@@ -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,
Signature · 1 line
2.30.2
PATCH 6/6 net: dsa: sja1105: support switching between SGMII and 2500BASE-X
In reply to 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.
--- drivers/net/dsa/sja1105/sja1105_main.c | 27 +++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)
drivers/net/dsa/sja1105/sja1105_main.c +22 -5
@@ -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.
Signature · 1 line
2.30.2
Re: [PATCH net-next 1/6] net: dsa: sja1105: populate supported_interfaces
In reply to Russell King (Oracle)Quoted 6 lines · 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.
Bl"%$y vi!
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Vladimir Oltean
maintainerRe: [PATCH net-next 1/6] net: dsa: sja1105: populate supported_interfaces
In reply to Russell King (Oracle)Quoted 6 lines · 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.
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
Quoted 9 lines · Vladimir Oltean
Quoted 6 lines · 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.
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?
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Vladimir Oltean
maintainerRe: [PATCH net-next 1/6] net: dsa: sja1105: populate supported_interfaces
Quoted 30 lines · Russell King (Oracle)
Quoted 9 lines · Vladimir Oltean
Quoted 6 lines · 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.
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.
Quoted 3 lines
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 <[email protected]> Signed-off-by: Vladimir Oltean <vladimir.│
[email protected]> │22
with
Fixes: 869f0ec048dc ("arm64: dts: freescale: Fix 'interrupt-map' parent address cells") │
Signed-off-by: Vladimir Oltean <[email protected]> Signed-off-by: Vladimir Oltean <vladimir. │
[email protected]> │Re: [PATCH net-next 1/6] net: dsa: sja1105: populate supported_interfaces
Quoted 35 lines · Vladimir Oltean
Quoted 30 lines · Russell King (Oracle)
Quoted 9 lines · Vladimir Oltean
Quoted 6 lines · 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.
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.
Quoted 3 lines
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?
Quoted 6 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 <[email protected]>[large-number-of-space-characters]"
Anyway, "set showbreak=+" seems to be perfect.
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!