[PATCH] net: mdio: enable optional clock when registering a phy from devicetree

[PATCH] net: mdio: enable optional clock when registering a phy from devicetree

[Heiko Stuebner]

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(-)

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,
-- 
2.39.2

Re: [PATCH] net: mdio: enable optional clock when registering a phy from devicetree

[Andrew Lunn]

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

Re: [PATCH] net: mdio: enable optional clock when registering a phy from devicetree

[Florian Fainelli]

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.
-- 
Florian

Re: [PATCH] net: mdio: enable optional clock when registering a phy from devicetree

[Quentin Schulz]

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

Re: [PATCH] net: mdio: enable optional clock when registering a phy from devicetree

[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. 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

Re: [PATCH] net: mdio: enable optional clock when registering a phy from devicetree

[Heiko Stübner]

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.

Re: [PATCH] net: mdio: enable optional clock when registering a phy from devicetree

[Andrew Lunn]

> 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