| Message ID | E1mpwSN-00D8Lz-GB@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Allow DSA drivers to set all phylink capabilities | expand |
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 --
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.
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 --
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.
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?
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.
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!
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,
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(-)