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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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

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

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

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

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

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

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

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

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

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

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

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

Thanks.

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