Message-ID: <Zb+9jQUqaha3Idsi@shell.armlinux.org.uk>
Date: Sun, 4 Feb 2024 16:38:37 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Arınç ÜNAL <arinc.unal@...nc9.com>
Cc: Daniel Golle <daniel@...rotopia.org>, DENG Qingfang <dqfext@...il.com>,
	Sean Wang <sean.wang@...iatek.com>, Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	Vladimir Oltean <olteanv@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	mithat.guner@...ont.com, erkin.bozoglu@...ont.com,
	Bartel Eerdekens <bartel.eerdekens@...stell8.be>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH net-next v3 4/7] net: dsa: mt7530: move XTAL check to
 mt7530_setup()

On Sun, Feb 04, 2024 at 06:55:14PM +0300, Arınç ÜNAL wrote:
> On 4.02.2024 17:18, Russell King (Oracle) wrote:
> > On Sun, Feb 04, 2024 at 04:55:40PM +0300, Arınç ÜNAL wrote:
> > > This is not about laziness. This is before patch 2:
> > > 
> > > phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config()
> > > -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config()
> > >     -> mt753x_mac_config()
> > >        -> mt753x_info :: mac_port_config() -> mt7530_mac_config()
> > >           -> mt7530_setup_port5()
> > >     -> mt753x_pad_setup()
> > >        -> mt753x_info :: pad_setup() -> mt7530_pad_clk_setup()
> > > 
> > > This is after:
> > > 
> > > phylink_mac_ops :: mac_config() -> dsa_port_phylink_mac_config()
> > > -> dsa_switch_ops :: phylink_mac_config() -> mt753x_phylink_mac_config()
> > >     -> mt753x_mac_config()
> > >        -> mt753x_info :: mac_port_config() -> mt7530_mac_config()
> > >           -> mt7530_setup_port5()
> > >           -> mt7530_setup_port6()
> > > 
> > > Patch 2 does not move mt7530_setup_port6() to be called from
> > > phylink_mac_ops :: mac_config(), it already is. There is no valid reason to
> > > reorder the patches.
> > > 
> > > My response to Russell should've stated this instead of focusing on his
> > > second sentence.
> > 
> > This patch moves the test for a 20MHz crystal to mt7530_setup(),
> > which is something that is entirely orthogonal to patch 2, which
> > can be done cleanly (I've just applied the patches in the original
> > order and then reordered them:
> > 
> > 98c481f5d706 net: dsa: mt7530: do not clear config->supported_interfaces
> > 93c6b53b17f4 net: dsa: mt7530: correct port capabilities of MT7988
> > c9c6d4c51a1d net: dsa: mt7530: simplify mt7530_setup_port6() and change to void
> > adfa948253e0 net: dsa: mt7530: remove pad_setup function pointer
> > 57e21e6c2fc0 net: dsa: mt7530: call port 6 setup from mt7530_mac_config()
> > 959a0f9323c8 net: dsa: mt7530: move XTAL check to mt7530_setup()
> > 856ab64a22ef net: dsa: mt7530: empty default case on mt7530_setup_port5()
> > 
> > No problems. The end result is identical comparing the git tree at the
> > original "move XTAL" patch with adfa948253e0.
> > 
> > Now, if we look at "net: dsa: mt7530: remove pad_setup function pointer"
> > we can see that yes, the pad_setup() method was called from mac_confing,
> > but this is the exact contents of that patch removing the callsite:
> > 
> > -               mt753x_pad_setup(ds, state);
> > 
> > This returns an integer, which may be an error code, which is ignored.
> > Therefore, if the XTAL frequency check fires, and mt753x_pad_setup()
> > returns an error, it is ignored today.
> > 
> > After "net: dsa: mt7530: call port 6 setup from mt7530_mac_config()"
> > the renamed pad_setup() method is now called from mac_config() thusly:
> > 
> > +               ret = mt7530_setup_port6(priv->ds, interface);
> > +               if (ret)
> > +                       return ret;
> > 
> > So now the error checks cause mt7530_mac_config() to return an error
> > which in turn causes mt753x_mac_config() to fail, and therefore
> > mt753x_phylink_mac_config() has different behaviour.
> > 
> > So, patch 2 changes the driver behaviour in the case of a 20MHz XTAL,
> > which is then changed again by patch 4.
> > 
> > It would be better to have only one change of behaviour by moving
> > patch 4 before patch 2.
> 
> If the idea is to not bring any more error returns to mt753x_mac_config()
> because the return code is actually checked for that, I should do a bit
> more effort and put patch 5 before patch 2 as well, to live up to what you
> originally requested.

I assume you are referring to getting rid of the default case in
mt7530_pad_clk_setup().

In patch "net: dsa: mt7530: call port 6 setup from mt7530_mac_config()"
where you move this to be called from mt7530_mac_config(), you add it
as:

+       } else if (port == 6) {
+               ret = mt7530_setup_port6(priv->ds, interface);
+               if (ret)
+                       return ret;
+       }

So it is only called for port 6. The switch within the called function
deals with PHY_INTERFACE_MODE_RGMII and PHY_INTERFACE_MODE_TRGMII.
Anything else results in the use of the default case, and thus
returning an error.

Since mt7530_mac_port_get_caps() does this for port 6:

                __set_bit(PHY_INTERFACE_MODE_RGMII,
                          config->supported_interfaces);
                __set_bit(PHY_INTERFACE_MODE_TRGMII,
                          config->supported_interfaces);

mt7530_setup_port6() will only ever be called for these two modes,
which means that the default case is unreachable, thus we will never
execute that path, thus whether that path returns an error or not is
completely irrelevant.

The only case in mt7530_setup_port6() / mt7530_pad_clk_setup() which
can today return an error is the XTAL check.

Therefore, my suggestion makes complete sense, and there is no need
to also move patch 5.

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

Powered by blists - more mailing lists