From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id EABCEC3DA6E for ; Fri, 5 Jan 2024 20:45:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=mMAg9jld//C51JoR4LMGUJ8fPsqbBBCWDsSDlVbcZt4=; b=DBO3pXo3u1colFWg7ODkO3Hh9V I65ONJIn8GYor5M937KW4ptGF3l4v6vMOi0tv9pUkos+F6/EfTY4kXDkRprPrciFx1fLgA0gLn624 fOU5J9Bmz95ZXTKQiL5tl1f+/Jju3x/BtuaI3YKa35yKOmq8S6mh34G/hYwwMxi9uVt7kHlgUl5z1 YGrq+62q1ZwySOvUqk4Y1w6Bera7wlBnD67PC4c6A9tOSvbzTLuYjd+keF1ejTOWzlT5rFGzsqN+v F4HxoEAckzrczYMs7zNwJU4k0+N9qmzVid350M9Zr7apvZDQZc0wrnt34O5VfQspfnzBt/AIiLl0V EF2YP47A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1rLr4c-000EYy-37; Fri, 05 Jan 2024 20:45:46 +0000 Received: from relay9-d.mail.gandi.net ([2001:4b98:dc4:8::229]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1rLr4Y-000EW8-1p; Fri, 05 Jan 2024 20:45:44 +0000 Received: by mail.gandi.net (Postfix) with ESMTPSA id 401C8FF805; Fri, 5 Jan 2024 20:45:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=arinc9.com; s=gm1; t=1704487536; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=mMAg9jld//C51JoR4LMGUJ8fPsqbBBCWDsSDlVbcZt4=; b=jDeNszcHy+TaKLR6TCFOLvVAPiRaRJdw+iRQvipfT9sLXuI+Hnll/4icc9lB9aQJ7dcqIV pKCCdNpNC39hBK0M0oABh5XXXImfLcrMRV97tNgfWO40lgsIGJ8RN02bqxKq5yR424Xi1Z KHq4Geyu7CvCkH0knoLqwLslPOyzLRszSyvIxn8O45fQKMUp6vCJyr3oemDPBn+c7yQvJV nKGXRQ0xLAg8i3hC2hwjlTUxuH9jBCfcIfGFLyTy7/7julcomY2Zdx020rbTpsnd1iKbP8 TZpqnV+Zut57nRIHduGgdsmREw5NwN0E4BKvwYmj15VJBmjt7kk2IKiH0S5WHw== Message-ID: Date: Fri, 5 Jan 2024 23:45:29 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH net-next] net: dsa: mt7530: register OF node for internal MDIO bus Content-Language: en-US To: Vladimir Oltean Cc: Daniel Golle , Landen Chao , DENG Qingfang , Sean Wang , Andrew Lunn , Florian Fainelli , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Matthias Brugger , AngeloGioacchino Del Regno , David Bauer , 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 References: <20231220173539.59071-1-arinc.unal@arinc9.com> <20231220173539.59071-1-arinc.unal@arinc9.com> <20231221151607.ujobhh4aet4obxdz@skbuf> <6600c6b1-2230-4963-940c-8b95a01750fd@arinc9.com> <20231227191154.6jkqdlqdxciidpfw@skbuf> <20231227200217.kdltxpmhvlp6z4cd@skbuf> <20240103190246.ctyeehvfmhctpphf@skbuf> From: =?UTF-8?B?QXLEsW7DpyDDnE5BTA==?= In-Reply-To: <20240103190246.ctyeehvfmhctpphf@skbuf> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-GND-Sasl: arinc.unal@arinc9.com X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240105_124542_909436_05AF5D8F X-CRM114-Status: GOOD ( 33.31 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org On 3.01.2024 22:02, Vladimir Oltean wrote: > On Thu, Dec 28, 2023 at 07:58:13PM +0300, Arınç ÜNAL wrote: >> The implementation below follows this logic: >> >> No switch MDIO bus defined: Register the MDIO bus, set the interrupts for >> PHYs if "interrupt-controller" is defined at the switch node. >> >> Switch MDIO bus defined: Register the MDIO bus, set the interrupts for PHYs >> if ["interrupt-controller" is defined at the switch node and "interrupts" >> is defined at the PHY nodes under the switch MDIO bus node]. >> >> I think this approach fits your description so I'd like to agree that this >> should be the way for all DSA subdrivers. Please let me know what you >> think. >> >> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c >> index 391c4dbdff42..bbd230a73ead 100644 >> --- a/drivers/net/dsa/mt7530.c >> +++ b/drivers/net/dsa/mt7530.c >> @@ -2155,15 +2155,21 @@ mt7530_setup_mdio(struct mt7530_priv *priv) >> { >> struct dsa_switch *ds = priv->ds; >> struct device *dev = priv->dev; >> + struct device_node *np, *mnp; >> struct mii_bus *bus; >> static int idx; >> int ret; >> + np = priv->dev->of_node; >> + mnp = of_get_child_by_name(np, "mdio"); >> + > > Empty line between variable declarations and code. Or you can initialize > them as part of their declaration, but you need to stick to the "longest > line first" rule. > > Also, it would be good to also check of_device_is_available(mnp). Will do. > >> + ds->user_mii_bus = bus; >> + >> bus->priv = priv; >> bus->name = KBUILD_MODNAME "-mii"; >> snprintf(bus->id, MII_BUS_ID_SIZE, KBUILD_MODNAME "-%d", idx++); >> @@ -2174,10 +2180,11 @@ mt7530_setup_mdio(struct mt7530_priv *priv) >> bus->parent = dev; >> bus->phy_mask = ~ds->phys_mii_mask; >> - if (priv->irq) >> + if (priv->irq && mnp == NULL) >> mt7530_setup_mdio_irq(priv); >> - ret = devm_mdiobus_register(dev, bus); >> + ret = devm_of_mdiobus_register(dev, bus, mnp); >> + of_node_put(mnp); > > This is going to be interesting. There isn't really a correct way to > manage the reference to "mnp", as far as I can tell. Normally, it should > have been possible to release the reference as you did. But you need > something along the lines of what Luiz/Russell have been discussing > here: > > https://lore.kernel.org/netdev/20231220045228.27079-2-luizluca@gmail.com/ > > In any case, the devres variant of of_mdiobus_register() seems incompatible > with the mt7530 driver owning the "mnp" node for any longer than this, > because it has no hook to call of_node_put() once the MDIO bus is unregistered. I'm not sure what's the step I should take here. I don't know how MDIO registration works and don't intend to spend time studying it at this time. Looking at the conversation, I see that, in the end, this patch is applied: https://lore.kernel.org/netdev/E1rLL6p-00EvAd-Ej@rmk-PC.armlinux.org.uk/ > >> if (ret) { >> dev_err(dev, "failed to register MDIO bus: %d\n", ret); >> if (priv->irq) >> >> With this device tree: >> >> switch { >> interrupt-controller; >> } >> >> [ 1.420534] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=17) >> [ 1.433224] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=18) >> [ 1.445338] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=19) >> [ 1.457472] mt7530-mdio mdio-bus:1f lan4 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=20) >> [ 1.469587] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=21) >> >> With this device tree: >> >> switch { >> interrupt-controller; >> >> mdio { >> phy { >> reg = <0>; >> } >> } >> } >> >> [ 1.413101] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=POLL) >> [ 1.429954] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=POLL) >> [ 1.443704] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=POLL) >> [ 1.455876] mt7530-mdio mdio-bus:1f lan4 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=POLL) >> [ 1.468079] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=POLL) >> >> With this device tree: >> >> switch { >> interrupt-controller; >> >> mdio { >> phy { >> reg = <0>; >> interrupts = <0>; >> } >> } >> } >> >> [ 1.420534] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=17) >> [ 1.433224] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=18) >> [ 1.445338] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=19) >> [ 1.457472] mt7530-mdio mdio-bus:1f lan4 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=20) >> [ 1.469587] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=21) > > Looks sane. > > FWIW, I found Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml > where internal PHYs don't have an 'interrupts' property, yet they are > probably still expected to use interrupts - according to ksz_irq_phy_setup(). This conflicts with the reason we want the subdrivers to use ds->user_mii_bus for. I'd much like to implement what I've done with this patch to this subdriver. I believe it's negligible for the old device trees to have the switch PHYs work with polling until the interrupts are defined on the device tree. > > Anyway, what's done is done, but I still don't see the point of making > the binding much more flexible than it needs to be. I don't see it that way. It's simply about describing the hardware. PHYs without interrupts is still valid hardware. If the PHYs of this specific hardware cannot possibly come without interrupts, then it should be the bindings that ensure that interrupts on the device tree are always defined. Not the other way around: Keep this information out of the device tree and do it on the subdriver. Arınç