Message-ID: <a97eb87d-bc36-cb31-b887-1feef40c4d34@arinc9.com>
Date:   Tue, 28 Mar 2023 00:40:08 +0300
From:   Arınç ÜNAL <arinc.unal@...nc9.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>, netdev <netdev@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@...ts.infradead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Thibaut <hacks@...shdirt.org>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Sean Wang <sean.wang@...iatek.com>,
        Landen Chao <Landen.Chao@...iatek.com>,
        DENG Qingfang <dqfext@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Sergio Paracuellos <sergio.paracuellos@...il.com>
Subject: Re: Move MT7530 phy muxing from DSA to PHY driver

On 27.03.2023 21:38, Vladimir Oltean wrote:
> On Sun, Mar 26, 2023 at 07:52:12PM +0300, Arınç ÜNAL wrote:
>> I'm currently working on the mt7530 driver and I think I found a way
>> that takes the least effort, won't break the ABI, and most importantly,
>> will work.
> 
> This sounds promising....
> 
>> As we acknowledged, the registers are in the switch address space. This
>> must also mean that the switch must be properly probed before doing
>> anything with the registers.
>>
>> I'm not sure how exactly making a tiny driver would work in this case.
> 
> I'm not sure how it would work, either. It sounds like the driver for
> the mdio-bus address @1f should have been a parent driver (MFD or not)
> with 2 (platform device) children, one for the switch and another for
> the HWTRAP registers and whatever else might be needed for the PHY
> multiplexing. The parent (mdio_device) driver deals with the chip-wide
> reset, resources, and manages the registers, giving them regmaps.
> The driver with the mux probably just exports a symbol representing a
> function that gets called by the "mediatek,eth-mac" driver and/or the
> switch driver.
> 
> BTW, I have something vaguely similar to this in a stalled WIP branch
> for sja1105, but things like this get really complicated really quickly
> if the DSA driver's DT bindings weren't designed from day one to not
> cover the entire switch's register map.
> 
>> I figured we can just run the phy muxing code before the DSA driver
>> exits because there are no (CPU) ports defined on the devicetree. Right
>> after probing is done on mt7530_probe, before dsa_register_switch() is run.
> 
> Aren't there timing issues, though? When is the earliest moment that the
> "mediatek,eth-mac" driver needs the HWTRAP muxing to be changed?
> The operation of changing that from the "mediatek,mt7530" driver is
> completely asynchronous to the probing of "mediatek,eth-mac".
> What's the worst that will happen with incorrect (not yet updated) GMII
> signal muxing? "Just" some lost packets?

We're not doing any changes to the MediaTek SoC's MAC if that's what 
you're asking. The phy muxing on the mt7530 DSA driver merely muxes PHY4 
of the switch to GMAC5 of the switch. Whatever MAC connected to GMAC5 
can access the muxed PHY.

It's just the current ABI that requires the MAC to be mediatek,eth-mac. 
PHY muxing can still be perfectly done with a simple property like 
mediatek,mt7530-muxphy = <0>;.

properties:
   mediatek,mt7530-muxphy:
     description:
       Set the PHY to mux to GMAC5. Only PHY0 or PHY4 can be muxed.
     enum:
       - 0
       - 4
     maxItems: 1

Whether or not PHY muxing will work with non-MediaTek MACs is still 
unknown as there is no known hardware that combines a standalone MT7530 
with a non-MediaTek SoC. Though in theory, it should work.

> 
>>
>> For proof of concept, I've moved some necessary switch probing code from
>> mt7530_setup() to mt7530_probe(). After the switch is properly reset,
>> phy4 is muxed, before dsa_register_switch() is run.
> 
> This is fragile because someone eager for some optimizations could move
> the code back the way it was, and say: "the switch initialization costs
> X ms and is done X times, because dsa_register_switch() -> ... ->
> of_find_net_device_by_node() returns -EPROBE_DEFER the first X-1 times.
> If we move the switch initialization to ds->ops->setup(), it will run
> only once, after the handle to the DSA master has been obtained, and
> this gives us a boost in kernel startup time."
> 
> It's even more fragile because currently (neither before nor after your change),
> mt7530_remove() does not do the mirror opposite of mt7530_probe(), and somebody
> eager from the future will notice this, and add an error handling path for
> dsa_register_switch(), which calls the opposite of regulator_enable(),
> regulator_disable(), saying "hey, there's no reason to let the regulators
> on if the switch failed to probe, it consumes power for nothing!".
> 
> It's an open question whether that regulator is needed for anything after
> the HWMUX registers has been changed, or if it can indeed be turned off.
> Not knowing this, it's hard to say if the change is okay or not.
> It seems that there's a high probability it will work for a while,
> by coincidence.

Let's just say that since it's needed for probing, it's best it stays 
on. This should prevent mt7530_remove() from going further if phy muxing 
is enabled.

> @@ -3167,6 +3173,9 @@ mt7530_remove(struct mdio_device *mdiodev)
>  	if (!priv)
>  		return;
>  
> +	if (priv->p5_intf_sel == (P5_INTF_SEL_PHY_P0 || P5_INTF_SEL_PHY_P4))
> +		return;
> +
>  	ret = regulator_disable(priv->core_pwr);
>  	if (ret < 0)
>  		dev_err(priv->dev,

Is mt7530_remove(), or to be more inclusive, the .remove operation of 
mdio_driver, run only when the switch fails to probe? If not, with the 
diff above, the switch won't be disabled in both of the cases (phy 
muxing only, phy muxing + normal DSA ports) if mt7530_remove() is run 
for some other reason.

> 
>>
>> [    0.650721] mt7530 mdio-bus:1f: MT7530 adapts as multi-chip module
>> [    0.660285] mt7530 mdio-bus:1f: muxing phy4 to gmac5
>> [    0.665284] mt7530 mdio-bus:1f: no ports child node found
>> [    0.670688] mt7530: probe of mdio-bus:1f failed with error -22
>> [    0.679118] mtk_soc_eth 1e100000.ethernet: generated random MAC address b6:9c:4d:eb:1f:8e
>> [    0.688922] mtk_soc_eth 1e100000.ethernet eth0: mediatek frame engine at 0xbe100000, irq 15
>>
>> ---
>>
>> # ifup eth0
>> [   30.674595] mtk_soc_eth 1e100000.ethernet eth0: configuring for fixed/rgmii link mode
>> [   30.683451] mtk_soc_eth 1e100000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
>> # ping 192.168.2.2
>> PING 192.168.2.2 (192.168.2.2): 56 data bytes
>> 64 bytes from 192.168.2.2: seq=0 ttl=64 time=0.688 ms
>> 64 bytes from 192.168.2.2: seq=1 ttl=64 time=0.375 ms
>> 64 bytes from 192.168.2.2: seq=2 ttl=64 time=0.357 ms
>> 64 bytes from 192.168.2.2: seq=3 ttl=64 time=0.323 ms
>>
>> ---
>>
>> # ip a
>> 1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1000
>>      link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
>>      inet 127.0.0.1/8 scope host lo
>>         valid_lft forever preferred_lft forever
>> 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP group default qlen 1000
>>      link/ether b6:9c:4d:eb:1f:8e brd ff:ff:ff:ff:ff:ff
>>      inet 192.168.2.1/24 scope global eth0
>>         valid_lft forever preferred_lft forever
>>
>> There is a lot to do, such as fixing the method to read from the
>> devicetree as it relies on the mac node the CPU port is connected to but
>> when this is finalised, we should be able to use it like this:
>>
>> mac@1 {
>> 	compatible = "mediatek,eth-mac";
>> 	reg = <1>;
>> 	phy-mode = "rgmii";
>> 	phy-handle = <&ethphy0>;
>> };
>>
>> mdio-bus {
>> 	#address-cells = <1>;
>> 	#size-cells = <0>;
>>
>> 	ethphy0: ethernet-phy@0 {
>> 		reg = <0>;
>> 	};
>>
>> 	switch@1f {
>> 		compatible = "mediatek,mt7530";
>> 		reg = <0x1f>;
>> 		reset-gpios = <&pio 33 0>;
>> 		core-supply = <&mt6323_vpa_reg>;
>> 		io-supply = <&mt6323_vemc3v3_reg>;
>> 	};
>> };
> 
> And this is fragile because the "mediatek,eth-mac" driver only works
> because of the side effects of a driver that began to probe, and failed.
> Someone, seeing that "mediatek,mt7530" fails to probe, and knowing that
> the switch ports are not needed/used on that board, could put a
> status = "disabled"; property under the switch@1f node.

This should help.

> 	if (priv->p5_intf_sel == (P5_INTF_SEL_PHY_P0 || P5_INTF_SEL_PHY_P4)) {
> 		dev_info(priv->dev,
> 			 "PHY muxing is enabled, gracefully exiting\n");
> 		return;
> 	}

Arınç

Powered by blists - more mailing lists