Message ID | 20231201142453.324697-1-heiko@sntech.de |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:bcd1:0:b0:403:3b70:6f57 with SMTP id r17csp1157992vqy; Fri, 1 Dec 2023 06:25:23 -0800 (PST) X-Google-Smtp-Source: AGHT+IFARhOVojRIhucZzLKi2gCpteIbAovO/OkZPRU+GXG2q/lOyUHyBAYfETWvHA+3Z2YUdkLZ X-Received: by 2002:a05:6358:260c:b0:16d:edaa:921c with SMTP id l12-20020a056358260c00b0016dedaa921cmr27982431rwc.12.1701440723213; Fri, 01 Dec 2023 06:25:23 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1701440723; cv=none; d=google.com; s=arc-20160816; b=W69m2Ot0uiAkGM9+3OI7MYlk140fxu36TZ3erfYRxodABorpaX7cQxWQR8EhBsvYCM dNLNjdWXeIXIEqum9ljvXT/Oy3qWRBKXBWnlQ8pvi3CU+8IjMiE57xDmyc2iWkh+5+Xe fS37g51W+Rs1q67uCeCA2ekaZhbC4Fxcn0f6PQaCdzjOOmctZuml0Uusz0okpXeOxwwJ xCGlOdiJPtmZ0sAUrL6LzNrXNq18DxPwYOQgqccPuPxkzEmF/m97us0jEC/nrTkSO+0B korfMMLWJp07W/kDtyZqfIVOpNiY+u54LJCrEXTuhvM5eiDEYO0+HkywcI+UcpaiN6Ok dAYw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from; bh=VNoTqj4jw9QAvG++X6P0FY9oSAe739z2m4VaBDKPNjk=; fh=sFSLDUmuJYY2JlgyzfaSoPOTudHNb2Ag+XKlsmzXB60=; b=knYOJ3wzNS7oALuu6OwN+vzYU2F0I4QrcxSFphStkoFRkfxsXkdWWg8IpE08KwjRJu CpiA0DAqbsYY4V6zEo9S/HUEVXkaf6oTkR6n6fD85ARjX2s5Vx6pbiZGjt/C7nQdUcoJ wusqUww+ZO4MYf15+gUyVBo7tgyhrdeG0QpUWEriuTcC1zP0T5CVeOigWSpHebGP2tMP 7UrKKOjefzS3f5yz0hphYzfhZdxQe/ocuVzf+GG8GBPD+/I9v5PmC2PpzQ85GN0l+3ZO M0+WGK2yJftYpcjSVD2f3NOmcxbOXHe2OUeUk15d6oCeaXBuh7O2cRNmIsHaq061BFm0 Wlog== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=sntech.de Received: from howler.vger.email (howler.vger.email. [23.128.96.34]) by mx.google.com with ESMTPS id y16-20020a656c10000000b005c24fb6927csi3770216pgu.656.2023.12.01.06.25.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 01 Dec 2023 06:25:23 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=sntech.de Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by howler.vger.email (Postfix) with ESMTP id 95D6884C29B0; Fri, 1 Dec 2023 06:25:13 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at howler.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1379161AbjLAOZF (ORCPT <rfc822;ruipengqi7@gmail.com> + 99 others); Fri, 1 Dec 2023 09:25:05 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52558 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1379143AbjLAOZD (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 1 Dec 2023 09:25:03 -0500 Received: from gloria.sntech.de (gloria.sntech.de [185.11.138.130]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 911E610F4; Fri, 1 Dec 2023 06:25:08 -0800 (PST) Received: from i53875b61.versanet.de ([83.135.91.97] helo=phil.lan) by gloria.sntech.de with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from <heiko@sntech.de>) id 1r94Rs-0000PA-CQ; Fri, 01 Dec 2023 15:24:56 +0100 From: Heiko Stuebner <heiko@sntech.de> To: andrew@lunn.ch, hkallweit1@gmail.com Cc: linux@armlinux.org.uk, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, quentin.schulz@theobroma-systems.com, heiko@sntech.de, Heiko Stuebner <heiko.stuebner@cherry.de> Subject: [PATCH] net: mdio: enable optional clock when registering a phy from devicetree Date: Fri, 1 Dec 2023 15:24:53 +0100 Message-Id: <20231201142453.324697-1-heiko@sntech.de> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.8 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on howler.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Fri, 01 Dec 2023 06:25:13 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1784089907765095485 X-GMAIL-MSGID: 1784089907765095485 |
Series |
net: mdio: enable optional clock when registering a phy from devicetree
|
|
Commit Message
Heiko Stübner
Dec. 1, 2023, 2:24 p.m. UTC
From: Heiko Stuebner <heiko.stuebner@cherry.de> The ethernet-phy binding (now) specifys that phys can declare a clock supply. Phy driver itself will handle this when probing the phy-driver. But there is a gap when trying to detect phys, because the mdio-bus needs to talk to the phy to get its phy-id. Using actual phy-ids in the dt like compatible = "ethernet-phy-id0022.1640", "ethernet-phy-ieee802.3-c22"; of course circumvents this, but in turn hard-codes the phy. With boards often having multiple phy options and the mdio-bus being able to actually probe devices, this feels like a step back. So check for the existence of a phy-clock per the -dtbinding in the of_mdiobus_register_phy() and enable the clock around the fwnode_mdiobus_register_phy() call which tries to determine the phy-id. Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de> --- drivers/net/mdio/of_mdio.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-)
Comments
On Fri, Dec 01, 2023 at 03:24:53PM +0100, Heiko Stuebner wrote: > From: Heiko Stuebner <heiko.stuebner@cherry.de> > > The ethernet-phy binding (now) specifys that phys can declare a clock > supply. Phy driver itself will handle this when probing the phy-driver. > > But there is a gap when trying to detect phys, because the mdio-bus needs > to talk to the phy to get its phy-id. Using actual phy-ids in the dt like > compatible = "ethernet-phy-id0022.1640", > "ethernet-phy-ieee802.3-c22"; > of course circumvents this, but in turn hard-codes the phy. > > With boards often having multiple phy options and the mdio-bus being able > to actually probe devices, this feels like a step back. > > So check for the existence of a phy-clock per the -dtbinding in the > of_mdiobus_register_phy() and enable the clock around the > fwnode_mdiobus_register_phy() call which tries to determine the phy-id. Why handle this separately to the reset GPIO and the reset controller? Andrew
On 12/1/23 06:24, Heiko Stuebner wrote: > From: Heiko Stuebner <heiko.stuebner@cherry.de> > > The ethernet-phy binding (now) specifys that phys can declare a clock > supply. Phy driver itself will handle this when probing the phy-driver. > > But there is a gap when trying to detect phys, because the mdio-bus needs > to talk to the phy to get its phy-id. Using actual phy-ids in the dt like > compatible = "ethernet-phy-id0022.1640", > "ethernet-phy-ieee802.3-c22"; > of course circumvents this, but in turn hard-codes the phy. But it is the established practice for situations like those where you need specific resources to be available in order to identify the device you are trying to probe/register. You can get away here with the clock API because it can operate on device_node, and you might be able with a bunch of other "resources" subsystems, but for instance with regulators, that won't work, we need a "struct device" which won't be created because that is exactly what we are trying to do. Also this only works for OF, not for ACPI or other yet to come firmware interface. Sorry but NACK. I am sympathetic to the idea that if you have multiple boards and you may have multiple PHY vendors this may not really scale, but in 2023 you have boot loaders aware of the Device Tree which can do all sorts of live DTB patching to provide the kernel with a "perfect" view of the world.
Hi Andrew, On 12/1/23 23:15, Andrew Lunn wrote: > [You don't often get email from andrew@lunn.ch. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > On Fri, Dec 01, 2023 at 03:24:53PM +0100, Heiko Stuebner wrote: >> From: Heiko Stuebner <heiko.stuebner@cherry.de> >> >> The ethernet-phy binding (now) specifys that phys can declare a clock >> supply. Phy driver itself will handle this when probing the phy-driver. >> >> But there is a gap when trying to detect phys, because the mdio-bus needs >> to talk to the phy to get its phy-id. Using actual phy-ids in the dt like >> compatible = "ethernet-phy-id0022.1640", >> "ethernet-phy-ieee802.3-c22"; >> of course circumvents this, but in turn hard-codes the phy. >> >> With boards often having multiple phy options and the mdio-bus being able >> to actually probe devices, this feels like a step back. >> >> So check for the existence of a phy-clock per the -dtbinding in the >> of_mdiobus_register_phy() and enable the clock around the >> fwnode_mdiobus_register_phy() call which tries to determine the phy-id. > > Why handle this separately to the reset GPIO and the reset controller? > I was just wondering about this as well. Right now, we put the reset on the MAC controller device tree node for our board (c.f. https://lore.kernel.org/linux-arm-kernel/20231201191103.343097-1-heiko@sntech.de/) because otherwise it doesn't work. I assume this is because the phy net subsystem is not handling the reset at the proper time (it does so before probing the PHY driver, which is too late because the auto-detection mechanism has already run before and the MAC couldn't find the PHY ID since the PHY wasn't reset properly at that time). I think essentially we would need to have both the reset assert/deassert and clock enabling/disabling in the same location as this patch is suggesting to make all of this work. I'll not investigate this, because Florian NACK'ed the whole thing. I do not personally have an interest in fixing only the reset, because we need the clock to be enabled for the auto-detection mechanism to work. Cheers, Quentin
Hi Florian, Heiko, On 12/1/23 23:41, Florian Fainelli wrote: > On 12/1/23 06:24, Heiko Stuebner wrote: >> From: Heiko Stuebner <heiko.stuebner@cherry.de> >> >> The ethernet-phy binding (now) specifys that phys can declare a clock >> supply. Phy driver itself will handle this when probing the phy-driver. >> >> But there is a gap when trying to detect phys, because the mdio-bus needs >> to talk to the phy to get its phy-id. Using actual phy-ids in the dt like >> compatible = "ethernet-phy-id0022.1640", >> "ethernet-phy-ieee802.3-c22"; >> of course circumvents this, but in turn hard-codes the phy. > > But it is the established practice for situations like those where you > need specific resources to be available in order to identify the device > you are trying to probe/register. > > You can get away here with the clock API because it can operate on > device_node, and you might be able with a bunch of other "resources" > subsystems, but for instance with regulators, that won't work, we need a > "struct device" which won't be created because that is exactly what we > are trying to do. > > Also this only works for OF, not for ACPI or other yet to come firmware > interface. > > Sorry but NACK. > > I am sympathetic to the idea that if you have multiple boards and you > may have multiple PHY vendors this may not really scale, but in 2023 you > have boot loaders aware of the Device Tree which can do all sorts of > live DTB patching to provide the kernel with a "perfect" view of the world. There's a strong push towards unifying the device tree across all pieces of SW involved, sometimes going as far as only having one binary passed between SW stages (e.g. U-Boot passes its own DT to TF-A, and then to the Linux kernel without actually loading anything aside from the Linux kernel Image binary) if I remember correctly (haven't really followed tbh). So, this is kinda a step backward for this effort. I don't like relying on bootloader to make the kernel work, this is usually not a great thing. I understand the reasons but am still a bit sad to not see this done in the kernel. Heiko, I would personally put the ID of the PHY to be the most likely encountered in the Linux kernel Device Tree so that if we somehow have a broken bootloader, there's a chance some devices still work properly. HW department said ksz9131 so we can go forward with this. In U-Boot DT, we would need a -u-boot.dtsi we change to the auto-detection compatible and we do the magic the Linux kernel doesn't want to do and hope it's fine for U-Boot maintainers. Once properly detected, we fixup the DT before booting the kernel. Cheers, Quentin
Am Montag, 4. Dezember 2023, 11:14:12 CET schrieb Quentin Schulz: > Hi Florian, Heiko, > > On 12/1/23 23:41, Florian Fainelli wrote: > > On 12/1/23 06:24, Heiko Stuebner wrote: > >> From: Heiko Stuebner <heiko.stuebner@cherry.de> > >> > >> The ethernet-phy binding (now) specifys that phys can declare a clock > >> supply. Phy driver itself will handle this when probing the phy-driver. > >> > >> But there is a gap when trying to detect phys, because the mdio-bus needs > >> to talk to the phy to get its phy-id. Using actual phy-ids in the dt like > >> compatible = "ethernet-phy-id0022.1640", > >> "ethernet-phy-ieee802.3-c22"; > >> of course circumvents this, but in turn hard-codes the phy. > > > > But it is the established practice for situations like those where you > > need specific resources to be available in order to identify the device > > you are trying to probe/register. > > > > You can get away here with the clock API because it can operate on > > device_node, and you might be able with a bunch of other "resources" > > subsystems, but for instance with regulators, that won't work, we need a > > "struct device" which won't be created because that is exactly what we > > are trying to do. > > > > Also this only works for OF, not for ACPI or other yet to come firmware > > interface. > > > > Sorry but NACK. > > > > I am sympathetic to the idea that if you have multiple boards and you > > may have multiple PHY vendors this may not really scale, but in 2023 you > > have boot loaders aware of the Device Tree which can do all sorts of > > live DTB patching to provide the kernel with a "perfect" view of the world. > > There's a strong push towards unifying the device tree across all pieces > of SW involved, sometimes going as far as only having one binary passed > between SW stages (e.g. U-Boot passes its own DT to TF-A, and then to > the Linux kernel without actually loading anything aside from the Linux > kernel Image binary) if I remember correctly (haven't really followed > tbh). So, this is kinda a step backward for this effort. I don't like > relying on bootloader to make the kernel work, this is usually not a > great thing. I understand the reasons but am still a bit sad to not see > this done in the kernel. > > Heiko, I would personally put the ID of the PHY to be the most likely > encountered in the Linux kernel Device Tree so that if we somehow have a > broken bootloader, there's a chance some devices still work properly. HW > department said ksz9131 so we can go forward with this. hmm, I was more of the mind of having either all or none work ;-) [i.e. keeping the c.22 compatible in the main dt and having firmware add the phy-id] I.e. a bootloader doing the correct detection and fixup would insert the matching phy-id and a broken bootloader would not do this. Having some boards work that by chance have the right phy and others break would possibly create a wild goose chase if the bootloader support for phy-id-handling breaks somewhere down the line. Heiko > In U-Boot DT, we > would need a -u-boot.dtsi we change to the auto-detection compatible and > we do the magic the Linux kernel doesn't want to do and hope it's fine > for U-Boot maintainers. Once properly detected, we fixup the DT before > booting the kernel.
> I was just wondering about this as well. Right now, we put the reset on the > MAC controller device tree node for our board (c.f. https://lore.kernel.org/linux-arm-kernel/20231201191103.343097-1-heiko@sntech.de/) > because otherwise it doesn't work. > > I assume this is because the phy net subsystem is not handling the reset at > the proper time (it does so before probing the PHY driver, which is too late > because the auto-detection mechanism has already run before and the MAC > couldn't find the PHY ID since the PHY wasn't reset properly at that time). > > I think essentially we would need to have both the reset assert/deassert and > clock enabling/disabling in the same location as this patch is suggesting to > make all of this work. > > I'll not investigate this, because Florian NACK'ed the whole thing. I do not > personally have an interest in fixing only the reset, because we need the > clock to be enabled for the auto-detection mechanism to work. There was a couple of discussions at LPC this year about resources needed to be enabled before bus discovered would work. I missed the first talk, but was in the second. That concentrated more on PCI, despite it being a generic problem. Ideally we want some way to list all the resources and ordering and delays etc, so that a generic block of code could get the device into an enumerable state. But there was a general push back on this idea from GregKH and the PM Maintainer, but i think the MMC Maintainer was for it, since the MMC subsystem has something which could be made generic. The outcome of the session was a PCI only solution. I still don't think we should be solving this in phylib, so for the moment, we need to keep with ID values in DT, so the driver gets probed. Anything we add to phylib is just going to make it harder to adopt a generic solution, if it ever gets agreed on. Andrew
diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c index 64ebcb6d235c..895b12849b23 100644 --- a/drivers/net/mdio/of_mdio.c +++ b/drivers/net/mdio/of_mdio.c @@ -8,6 +8,7 @@ * out of the OpenFirmware device tree and using it to populate an mii_bus. */ +#include <linux/clk.h> #include <linux/device.h> #include <linux/err.h> #include <linux/fwnode_mdio.h> @@ -15,6 +16,7 @@ #include <linux/module.h> #include <linux/netdevice.h> #include <linux/of.h> +#include <linux/of_clk.h> #include <linux/of_irq.h> #include <linux/of_mdio.h> #include <linux/of_net.h> @@ -46,7 +48,37 @@ EXPORT_SYMBOL(of_mdiobus_phy_device_register); static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *child, u32 addr) { - return fwnode_mdiobus_register_phy(mdio, of_fwnode_handle(child), addr); + struct clk *clk = NULL; + int ret; + + /* ethernet-phy binding specifies a maximum of 1 clock */ + if (of_clk_get_parent_count(child) == 1) { + clk = of_clk_get(child, 0); + if (IS_ERR(clk)) { + if (PTR_ERR(clk) != -ENOENT) + return dev_err_probe(&mdio->dev, PTR_ERR(clk), + "Could not get defined clock for MDIO device at address %u\n", + addr); + + clk = NULL; + } + } + + ret = clk_prepare_enable(clk); + if (ret < 0) { + clk_put(clk); + dev_err(&mdio->dev, + "Could not enable clock for MDIO device at address %u: %d\n", + addr, ret); + return ret; + } + + ret = fwnode_mdiobus_register_phy(mdio, of_fwnode_handle(child), addr); + + clk_disable_unprepare(clk); + clk_put(clk); + + return ret; } static int of_mdiobus_register_device(struct mii_bus *mdio,