Date:   Wed, 10 Mar 2021 09:23:54 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Rafał Miłecki <zajec5@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        bcm-kernel-feedback-list@...adcom.com,
        Rafał Miłecki <rafal@...ecki.pl>
Subject: Re: [PATCH] net: dsa: bcm_sf2: enable GPHY for switch probing

On 3/10/21 2:40 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@...ecki.pl>
> 
> GPHY needs to be enabled to successfully probe & setup switch port
> connected to it. Otherwise hardcoding PHY OUI would be required.
> 
> This prevents unimac_mdio_read() from getting MDIO_READ_FAIL.
> 
> Before:
> brcm-sf2 80080000.switch lan4 (uninitialized): PHY [800c05c0.mdio--1:08] driver [Generic PHY] (irq=POLL)
> brcm-sf2 80080000.switch lan3 (uninitialized): PHY [800c05c0.mdio--1:09] driver [Generic PHY] (irq=POLL)
> brcm-sf2 80080000.switch lan2 (uninitialized): PHY [800c05c0.mdio--1:0a] driver [Generic PHY] (irq=POLL)
> brcm-sf2 80080000.switch lan1 (uninitialized): PHY [800c05c0.mdio--1:0b] driver [Generic PHY] (irq=POLL)
> brcm-sf2 80080000.switch wan (uninitialized): error -5 setting up PHY for tree 0, switch 0, port 7
> 
> After:
> brcm-sf2 80080000.switch lan4 (uninitialized): PHY [800c05c0.mdio--1:08] driver [Generic PHY] (irq=POLL)
> brcm-sf2 80080000.switch lan3 (uninitialized): PHY [800c05c0.mdio--1:09] driver [Generic PHY] (irq=POLL)
> brcm-sf2 80080000.switch lan2 (uninitialized): PHY [800c05c0.mdio--1:0a] driver [Generic PHY] (irq=POLL)
> brcm-sf2 80080000.switch lan1 (uninitialized): PHY [800c05c0.mdio--1:0b] driver [Generic PHY] (irq=POLL)
> brcm-sf2 80080000.switch wan (uninitialized): PHY [800c05c0.mdio--1:0c] driver [Generic PHY] (irq=POLL)
> 
> Signed-off-by: Rafał Miłecki <rafal@...ecki.pl>
> ---
> Below you can see backtrace after adding
> WARN(1, "MDIO_READ_FAIL\n");
> to the unimac_mdio_read() for the (cmd & MDIO_READ_FAIL) condition.
> 
> [    0.584058] brcm-sf2 80080000.switch: found switch: BCM4908, rev 0
> [    0.596214] brcm-sf2 80080000.switch lan4 (uninitialized): PHY [800c05c0.mdio--1:08] driver [Generic PHY]
> [    0.612212] brcm-sf2 80080000.switch lan3 (uninitialized): PHY [800c05c0.mdio--1:09] driver [Generic PHY]
> [    0.628216] brcm-sf2 80080000.switch lan2 (uninitialized): PHY [800c05c0.mdio--1:0a] driver [Generic PHY]
> [    0.644215] brcm-sf2 80080000.switch lan1 (uninitialized): PHY [800c05c0.mdio--1:0b] driver [Generic PHY]
> [    0.656212] ------------[ cut here ]------------
> [    0.660884] MDIO_READ_FAIL
> [    0.663685] WARNING: CPU: 0 PID: 128 at unimac_mdio_read+0x98/0xb8
> [    0.670016] Modules linked in:
> [    0.673156] CPU: 0 PID: 128 Comm: kworker/0:2 Not tainted 5.4.99 #0
> [    0.679603] Hardware name: Netgear R8000P (DT)
> [    0.684185] Workqueue: events deferred_probe_work_func
> [    0.689462] pstate: 60400005 (nZCv daif +PAN -UAO)
> [    0.694389] pc : unimac_mdio_read+0x98/0xb8
> [    0.698690] lr : unimac_mdio_read+0x98/0xb8
> [    0.702989] sp : ffffffc0108d3840
> [    0.706394] x29: ffffffc0108d3840 x28: 0000000000000000
> [    0.711860] x27: ffffff801e8a8850 x26: ffffff801e8a8840
> [    0.717325] x25: ffffff801f257000 x24: 0000000000000000
> [    0.722790] x23: 0000000000002001 x22: 0000000000000001
> [    0.728256] x21: ffffff801f257000 x20: 0000000000001000
> [    0.733723] x19: ffffff801f34a080 x18: 000000000000000e
> [    0.739188] x17: 0000000000000001 x16: 0000000000000019
> [    0.744653] x15: 0000000000000033 x14: ffffffc01079daa0
> [    0.750119] x13: 0000000000000000 x12: ffffffc01079d000
> [    0.755584] x11: ffffffc010767000 x10: 0000000000000010
> [    0.761049] x9 : 0000000000000000 x8 : 0000000000000000
> [    0.766516] x7 : 0000000000000007 x6 : 000000000000006e
> [    0.771981] x5 : 0000000000000006 x4 : 0000000000000000
> [    0.777446] x3 : 0000000000000000 x2 : 00000000ffffffff
> [    0.782912] x1 : ffffffc010767158 x0 : 000000000000000e
> [    0.788379] Call trace:
> [    0.790890]  unimac_mdio_read+0x98/0xb8
> [    0.794831]  __mdiobus_read+0x40/0x58
> [    0.798594]  mdiobus_read+0x48/0x70
> [    0.802177]  genphy_read_abilities+0x84/0x158
> [    0.806657]  phy_probe+0x160/0x1d8
> [    0.810153]  phy_attach_direct+0x210/0x2c0
> [    0.814368]  of_phy_attach+0x40/0x80
> [    0.818042]  phylink_of_phy_connect+0x6c/0x120
> [    0.822611]  dsa_slave_create+0x2b8/0x408
> [    0.826728]  dsa_register_switch+0x888/0xa60
> [    0.831120]  b53_switch_register+0x1c4/0x300
> [    0.835510]  bcm_sf2_sw_probe+0x50c/0x640
> [    0.839631]  platform_drv_probe+0x50/0xa0
> [    0.843752]  really_probe+0xcc/0x2e0
> [    0.847425]  driver_probe_device+0x54/0xe8
> [    0.851637]  __device_attach_driver+0x80/0xb8
> [    0.856118]  bus_for_each_drv+0x68/0xa8
> [    0.860059]  __device_attach+0xcc/0x140
> [    0.864001]  device_initial_probe+0x10/0x18
> [    0.868303]  bus_probe_device+0x90/0x98
> [    0.872245]  deferred_probe_work_func+0x6c/0xa0
> [    0.876907]  process_one_work+0x1fc/0x390
> [    0.881026]  worker_thread+0x278/0x4d0
> [    0.884882]  kthread+0x120/0x128
> [    0.888195]  ret_from_fork+0x10/0x1c
> [    0.891868] ---[ end trace 918e8c44c53d6f7b ]---
> ---
>  drivers/net/dsa/bcm_sf2.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
> index 70626547ffb3..6159d0a69870 100644
> --- a/drivers/net/dsa/bcm_sf2.c
> +++ b/drivers/net/dsa/bcm_sf2.c
> @@ -1432,10 +1432,14 @@ static int bcm_sf2_sw_probe(struct platform_device *pdev)
>  	rev = reg_readl(priv, REG_PHY_REVISION);
>  	priv->hw_params.gphy_rev = rev & PHY_REVISION_MASK;
>  
> +	bcm_sf2_gphy_enable_set(priv->dev->ds, true);
> +
>  	ret = b53_switch_register(dev);
>  	if (ret)
>  		goto out_mdio;
>  
> +	bcm_sf2_gphy_enable_set(priv->dev->ds, false);

This change got me thinking some more about how to deal with that, and I
believe that we do have a possible problem with your change. Past
b53_switch_register() we will call dsa_register_switch() which will be
publishing the network devices which makes them immediately visible to
notifiers and user-space for use. This means that we could be racing
with a process opening the port, and the bcm_sf2_gphy_enable_set() call
and have a port with the PHY disabled with no idea why.

The ideal way to solve this would be to have a hook before we call
dsa_slave_create() and one after such that we could turn on the internal
PHY just for probing, and then disable it immediately after in case the
network port was never used so we could continue to save power.

There is the dsa_switch_ops::get_phy_flags() that we could use for that
purpose, but we have no way to balance it.

I have been down the road of trying to solve these chicken and egg
problems with integrated PHYs that need clocks/power to be turned on and
really the best and easiest way was just to do what you have mentioned
which is to use a compatible string with the full OUI included within.
This solves a whole lot of problems for free. Given that you control how
the Device Trees are generated and loaded with the kernel is there a
strong reason not to do that?
-- 
-- 
Florian

Powered by blists - more mailing lists