Date:   Sun, 16 Apr 2023 16:48:23 +0300
From:   Arınç ÜNAL <arinc.unal@...nc9.com>
To:     Daniel Golle <daniel@...rotopia.org>, netdev@...r.kernel.org,
        linux-mediatek@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Sean Wang <sean.wang@...iatek.com>,
        Landen Chao <Landen.Chao@...iatek.com>,
        DENG Qingfang <dqfext@...il.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>,
        Russell King <linux@...linux.org.uk>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>
Subject: Re: [PATCH net-next v2] net: dsa: mt7530: fix support for MT7531BE

On 16.04.2023 15:08, Daniel Golle wrote:
> There are two variants of the MT7531 switch IC which got different
> features (and pins) regarding port 5:
>   * MT7531AE: SGMII/1000Base-X/2500Base-X SerDes PCS
>   * MT7531BE: RGMII
> 
> Moving the creation of the SerDes PCS from mt753x_setup to mt7530_probe
> with commit 6de285229773 ("net: dsa: mt7530: move SGMII PCS creation
> to mt7530_probe function") works fine for MT7531AE which got two
> instances of mtk-pcs-lynxi, however, MT7531BE requires mt7531_pll_setup
> to setup clocks before the single PCS on port 6 (usually used as CPU
> port) starts to work and hence the PCS creation failed on MT7531BE.
> 
> Fix this by introducing a pointer to mt7531_create_sgmii function in
> struct mt7530_priv and call it again at the end of mt753x_setup like it
> was before commit 6de285229773 ("net: dsa: mt7530: move SGMII PCS
> creation to mt7530_probe function").
> 
> Fixes: 6de285229773 ("net: dsa: mt7530: move SGMII PCS creation to mt7530_probe function")
> Signed-off-by: Daniel Golle <daniel@...rotopia.org>

I'll put my 2 cents about the patch along with responding to your points 
on the other thread here.

> Why don't we use my original solution [1] which has some advantages:
> 
>  * It doesn't requrire additional export of mt7530_regmap_bus
> 
>  * It doesn't move PCS creation to mt7530.c, hence PCS_MTK_LYNXI is
>    only required for MDIO-connected switches
>    (with your patch we would have to move the dependency on PCS_MTK_LYNXI
>    from NET_DSA_MT7530_MDIO to NET_DSA_MT7530)

Maybe this is what should happen. Maybe the PCS creation (and therefore 
mt7530_regmap_bus) should be on the core driver. Both are on the MDIO 
driver for the sole reason of only the devices on the MDIO driver 
currently using it. It's not an MDIO-specific operation as far as I can 
tell. Having it on the core driver would make more sense in the long run.

> 
>  * It doesn't expose the dysfunctional SerDes PCS for port 5 on MT7531BE
>    This will still fail and hence result in probing on MT7531 to exit
>    prematurely, preventing the switch driver from being loaded.
>    Before 9ecc00164dc23 ("net: dsa: mt7530: refactor SGMII PCS creation")
>    the return value of mtk_pcs_lynxi_create was ignored, now it isn't...

Ok, so checking whether port 5 is SGMII or not on the PCS creation code 
should be done on the same patch that fixes this issue.

> 
>  * It changes much less in terms of LoC

I'd rather prefer a better logic than the "least amount of changes 
possible" approach.

Let's analyse what this patch does:

With this patch, mt7531_create_sgmii() is run after mt7530_setup_mdio is 
run, under mt753x_setup(). mt7531_pll_setup() and, as the last 
requirement, mt7530_setup_mdio() must be run to be able to create the 
PCS instances. That also means running mt7530_free_irq_common must be 
avoided since the device uses MDIO so mt7530_free_mdio_irq needs to be 
run too.

While probing the driver, the priv->create_sgmii pointer will be made to 
point to mt7531_create_sgmii, if MT7531 is detected. Why? This pointer 
won't be used for any other devices and sgmii will always be created for 
any MT7531 variants, so it's always going to point to 
mt7531_create_sgmii when priv->id is ID_MT7531. So you're introducing a 
new pointer just to be able to call mt7531_create_sgmii() on 
mt7530-mdio.c from mt7530.c.

On mt753x_setup(), if priv->create_sgmii is pointing to something it 
will now run whatever it points to with two arguments. One being the 
priv table and the other being mt7531_dual_sgmii_supported() which 
returns 1 or 0 by looking at the very same priv table. That looks bad. 
What could be done instead is introduce a new field on the priv table 
that keeps the information of whether port 5 on the MT7531 switch is 
SGMII or not.

A similar logic is already there on the U-Boot MediaTek ethernet driver.

https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L903

So this patch fixes the issue with the only consideration being changing 
as less lines of code as possible. And that's okay. We can make the 
least amount of changes to fix the issue first, then improve the driver. 
But there's nothing new made on the driver after the commit that caused 
this issue, backportability to the stable trees is a non-issue. So why 
not do it properly the first time?

Whatever the outcome with this patch is, on my upcoming patch series, I 
intend to move mt7531_create_sgmii to mt7530.c. Then introduce 
priv->p5_sgmii to get rid of mt7531_dual_sgmii_supported().

Arınç

Powered by blists - more mailing lists