Re: [PATCH net-next v2] net: dsa: mt7530: detect PHY muxing when PHY is defined on switch MDIO bus

From: Daniel Golle <daniel@makrotopia.org>
To: arinc.unal@arinc9.com
Cc: DENG Qingfang <dqfext@gmail.com>,
	Sean Wang <sean.wang@mediatek.com>, Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Vladimir Oltean <olteanv@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	AngeloGioacchino Del Regno
	<angelogioacchino.delregno@collabora.com>,
	Bartel Eerdekens <bartel.eerdekens@constell8.be>,
	mithat.guner@xeront.com, erkin.bozoglu@xeront.com,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org
Subject: Re: [PATCH net-next v2] net: dsa: mt7530: detect PHY muxing when PHY is defined on switch MDIO bus
Date: Mon, 6 May 2024 12:24:17 +0100	[thread overview]
Message-ID: <Zji94d4yfEBaHlzt@makrotopia.org> (raw)
In-Reply-To: <20240430-b4-for-netnext-mt7530-use-switch-mdio-bus-for-phy-muxing-v2-1-9104d886d0db@arinc9.com>

On Tue, Apr 30, 2024 at 08:01:33AM +0300, Arınç ÜNAL via B4 Relay wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Currently, the MT7530 DSA subdriver configures the MT7530 switch to provide
> direct access to switch PHYs, meaning, the switch PHYs listen on the MDIO
> bus the switch listens on. The PHY muxing feature makes use of this.
> 
> This is problematic as the PHY may be attached before the switch is
> initialised, in which case, the PHY will fail to be attached.
> 
> Since commit 91374ba537bd ("net: dsa: mt7530: support OF-based registration
> of switch MDIO bus"), we can describe the switch PHYs on the MDIO bus of
> the switch on the device tree. Extend the check to detect PHY muxing when
> the PHY is defined on the MDIO bus of the switch on the device tree.
> 
> When the PHY is described this way, the switch will be initialised first,
> then the switch MDIO bus will be registered. Only after these steps, the
> PHY will be attached.
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>

Reviewed-by: Daniel Golle <daniel@makrotopia.org>

> ---
> Changes in v2:
> - Address the terminology on the patch log.
> - Link to v1: https://lore.kernel.org/r/20240429-b4-for-netnext-mt7530-use-switch-mdio-bus-for-phy-muxing-v1-1-1f775983e155@arinc9.com
> ---
>  drivers/net/dsa/mt7530.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 2b9f904a98f0..6cf21c9d523b 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2483,7 +2483,8 @@ mt7530_setup(struct dsa_switch *ds)
>  			if (!phy_node)
>  				continue;
>  
> -			if (phy_node->parent == priv->dev->of_node->parent) {
> +			if (phy_node->parent == priv->dev->of_node->parent ||
> +			    phy_node->parent->parent == priv->dev->of_node) {

I had some concerns about missing check for phy_node->parent != NULL,
but it's impossible in practise. If phy_node exists, it will have a parent
node as well.

To be super extra safe, maybe doing
phy_node->parent && phy_node->parent->parent == priv->dev->of_node
would be better.

>  				ret = of_get_phy_mode(mac_np, &interface);
>  				if (ret && ret != -ENODEV) {
>  					of_node_put(mac_np);
> 
> ---
> base-commit: 5c4c0edca68a5841a8d53ccd49596fe199c8334c
> change-id: 20240429-b4-for-netnext-mt7530-use-switch-mdio-bus-for-phy-muxing-586269371c55
> 
> Best regards,
> -- 
> Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> 
> 