Message ID | 20231201142453.324697-1-heiko@sntech.de (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | net: mdio: enable optional clock when registering a phy from devicetree | expand |
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,