* [PATCH] of_mdio: Fix broken PHY IRQ in case of probe deferral
@ 2017-05-18 12:59 Geert Uytterhoeven
[not found] ` <1495112345-24795-1-git-send-email-geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Geert Uytterhoeven @ 2017-05-18 12:59 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Rob Herring, Frank Rowand
Cc: Thomas Petazzoni, Sergei Shtylyov, netdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Geert Uytterhoeven
If an Ethernet PHY is initialized before the interrupt controller it is
connected to, a message like the following is printed:
irq: no irq domain found for /interrupt-controller@e61c0000 !
However, the actual error is ignored, leading to a non-functional (-1)
PHY interrupt later:
Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY driver [Micrel KSZ8041RNLI] (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=-1)
Depending on whether the PHY driver will fall back to polling, Ethernet
may or may not work.
To fix this:
1. Switch of_mdiobus_register_phy() from irq_of_parse_and_map() to
of_irq_get().
Unlike the former, the latter returns -EPROBE_DEFER if the
interrupt controller is not yet available, so this condition can be
detected.
Other errors are handled the same as before, i.e. use the passed
mdio->irq[addr] as interrupt.
2. Propagate and handle errors from of_mdiobus_register_phy() and
of_mdiobus_register_device().
Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
---
Seen on r8a7791/koelsch when using the new CPG/MSSR clock driver.
I assume it always happened on RZ/G1 in mainline.
---
drivers/of/of_mdio.c | 39 +++++++++++++++++++++++++++------------
1 file changed, 27 insertions(+), 12 deletions(-)
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 7e4c80f9b6cda0d3..f9ac2893f56184be 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -44,7 +44,7 @@ static int of_get_phy_id(struct device_node *device, u32 *phy_id)
return -EINVAL;
}
-static void of_mdiobus_register_phy(struct mii_bus *mdio,
+static int of_mdiobus_register_phy(struct mii_bus *mdio,
struct device_node *child, u32 addr)
{
struct phy_device *phy;
@@ -60,9 +60,13 @@ static void of_mdiobus_register_phy(struct mii_bus *mdio,
else
phy = get_phy_device(mdio, addr, is_c45);
if (IS_ERR(phy))
- return;
+ return PTR_ERR(phy);
- rc = irq_of_parse_and_map(child, 0);
+ rc = of_irq_get(child, 0);
+ if (rc == -EPROBE_DEFER) {
+ phy_device_free(phy);
+ return rc;
+ }
if (rc > 0) {
phy->irq = rc;
mdio->irq[addr] = rc;
@@ -84,22 +88,23 @@ static void of_mdiobus_register_phy(struct mii_bus *mdio,
if (rc) {
phy_device_free(phy);
of_node_put(child);
- return;
+ return rc;
}
dev_dbg(&mdio->dev, "registered phy %s at address %i\n",
child->name, addr);
+ return 0;
}
-static void of_mdiobus_register_device(struct mii_bus *mdio,
- struct device_node *child, u32 addr)
+static int of_mdiobus_register_device(struct mii_bus *mdio,
+ struct device_node *child, u32 addr)
{
struct mdio_device *mdiodev;
int rc;
mdiodev = mdio_device_create(mdio, addr);
if (IS_ERR(mdiodev))
- return;
+ return PTR_ERR(mdiodev);
/* Associate the OF node with the device structure so it
* can be looked up later.
@@ -112,11 +117,12 @@ static void of_mdiobus_register_device(struct mii_bus *mdio,
if (rc) {
mdio_device_free(mdiodev);
of_node_put(child);
- return;
+ return rc;
}
dev_dbg(&mdio->dev, "registered mdio device %s at address %i\n",
child->name, addr);
+ return 0;
}
int of_mdio_parse_addr(struct device *dev, const struct device_node *np)
@@ -242,9 +248,11 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
}
if (of_mdiobus_child_is_phy(child))
- of_mdiobus_register_phy(mdio, child, addr);
+ rc = of_mdiobus_register_phy(mdio, child, addr);
else
- of_mdiobus_register_device(mdio, child, addr);
+ rc = of_mdiobus_register_device(mdio, child, addr);
+ if (rc)
+ goto unregister;
}
if (!scanphys)
@@ -265,12 +273,19 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
dev_info(&mdio->dev, "scan phy %s at address %i\n",
child->name, addr);
- if (of_mdiobus_child_is_phy(child))
- of_mdiobus_register_phy(mdio, child, addr);
+ if (of_mdiobus_child_is_phy(child)) {
+ rc = of_mdiobus_register_phy(mdio, child, addr);
+ if (rc)
+ goto unregister;
+ }
}
}
return 0;
+
+unregister:
+ mdiobus_unregister(mdio);
+ return rc;
}
EXPORT_SYMBOL(of_mdiobus_register);
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 17+ messages in thread
[parent not found: <1495112345-24795-1-git-send-email-geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>]
* Re: [PATCH] of_mdio: Fix broken PHY IRQ in case of probe deferral
[not found] ` <1495112345-24795-1-git-send-email-geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
@ 2017-05-18 15:21 ` David Miller
0 siblings, 0 replies; 17+ messages in thread
From: David Miller @ 2017-05-18 15:21 UTC (permalink / raw)
To: geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ
Cc: andrew-g2DYL2Zd6BY, f.fainelli-Re5JQEeQqe8AvxtiuMwx3w,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
frowand.list-Re5JQEeQqe8AvxtiuMwx3w,
thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8,
netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
From: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
Date: Thu, 18 May 2017 14:59:05 +0200
> If an Ethernet PHY is initialized before the interrupt controller it is
> connected to, a message like the following is printed:
>
> irq: no irq domain found for /interrupt-controller@e61c0000 !
>
> However, the actual error is ignored, leading to a non-functional (-1)
> PHY interrupt later:
>
> Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY driver [Micrel KSZ8041RNLI] (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=-1)
>
> Depending on whether the PHY driver will fall back to polling, Ethernet
> may or may not work.
>
> To fix this:
> 1. Switch of_mdiobus_register_phy() from irq_of_parse_and_map() to
> of_irq_get().
> Unlike the former, the latter returns -EPROBE_DEFER if the
> interrupt controller is not yet available, so this condition can be
> detected.
> Other errors are handled the same as before, i.e. use the passed
> mdio->irq[addr] as interrupt.
> 2. Propagate and handle errors from of_mdiobus_register_phy() and
> of_mdiobus_register_device().
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
Florian or someone similarly knowledgable, please review.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] of_mdio: Fix broken PHY IRQ in case of probe deferral
2017-05-18 12:59 [PATCH] of_mdio: Fix broken PHY IRQ in case of probe deferral Geert Uytterhoeven
[not found] ` <1495112345-24795-1-git-send-email-geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
@ 2017-05-18 16:09 ` Andrew Lunn
2017-05-18 16:13 ` Geert Uytterhoeven
2017-05-18 18:25 ` Florian Fainelli
2 siblings, 1 reply; 17+ messages in thread
From: Andrew Lunn @ 2017-05-18 16:09 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Florian Fainelli, Rob Herring, Frank Rowand, Thomas Petazzoni,
Sergei Shtylyov, netdev, devicetree, linux-renesas-soc,
linux-kernel
On Thu, May 18, 2017 at 02:59:05PM +0200, Geert Uytterhoeven wrote:
> If an Ethernet PHY is initialized before the interrupt controller it is
> connected to, a message like the following is printed:
>
> irq: no irq domain found for /interrupt-controller@e61c0000 !
>
> However, the actual error is ignored, leading to a non-functional (-1)
> PHY interrupt later:
>
> Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY driver [Micrel KSZ8041RNLI] (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=-1)
>
> Depending on whether the PHY driver will fall back to polling, Ethernet
> may or may not work.
>
> To fix this:
> 1. Switch of_mdiobus_register_phy() from irq_of_parse_and_map() to
> of_irq_get().
> Unlike the former, the latter returns -EPROBE_DEFER if the
> interrupt controller is not yet available, so this condition can be
> detected.
> Other errors are handled the same as before, i.e. use the passed
> mdio->irq[addr] as interrupt.
> 2. Propagate and handle errors from of_mdiobus_register_phy() and
> of_mdiobus_register_device().
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Seen on r8a7791/koelsch when using the new CPG/MSSR clock driver.
> I assume it always happened on RZ/G1 in mainline.
> ---
> drivers/of/of_mdio.c | 39 +++++++++++++++++++++++++++------------
> 1 file changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index 7e4c80f9b6cda0d3..f9ac2893f56184be 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -44,7 +44,7 @@ static int of_get_phy_id(struct device_node *device, u32 *phy_id)
> return -EINVAL;
> }
>
> -static void of_mdiobus_register_phy(struct mii_bus *mdio,
> +static int of_mdiobus_register_phy(struct mii_bus *mdio,
> struct device_node *child, u32 addr)
> {
> struct phy_device *phy;
> @@ -60,9 +60,13 @@ static void of_mdiobus_register_phy(struct mii_bus *mdio,
> else
> phy = get_phy_device(mdio, addr, is_c45);
> if (IS_ERR(phy))
> - return;
> + return PTR_ERR(phy);
>
> - rc = irq_of_parse_and_map(child, 0);
> + rc = of_irq_get(child, 0);
> + if (rc == -EPROBE_DEFER) {
> + phy_device_free(phy);
> + return rc;
> + }
Maybe this should be consistent. All other places there is an error,
you return it. Here however, you only return the error if it is
EPROBE_DEFER.
Andrew
> if (rc > 0) {
> phy->irq = rc;
> mdio->irq[addr] = rc;
> @@ -84,22 +88,23 @@ static void of_mdiobus_register_phy(struct mii_bus *mdio,
> if (rc) {
> phy_device_free(phy);
> of_node_put(child);
> - return;
> + return rc;
> }
>
> dev_dbg(&mdio->dev, "registered phy %s at address %i\n",
> child->name, addr);
> + return 0;
> }
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] of_mdio: Fix broken PHY IRQ in case of probe deferral
2017-05-18 16:09 ` Andrew Lunn
@ 2017-05-18 16:13 ` Geert Uytterhoeven
[not found] ` <CAMuHMdW_ZSn_nXSNRTHQwFioRD5UTotxKgm1eMqAP06n+cWv+g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 17+ messages in thread
From: Geert Uytterhoeven @ 2017-05-18 16:13 UTC (permalink / raw)
To: Andrew Lunn
Cc: Geert Uytterhoeven, Florian Fainelli, Rob Herring, Frank Rowand,
Thomas Petazzoni, Sergei Shtylyov, netdev@vger.kernel.org,
devicetree@vger.kernel.org, Linux-Renesas,
linux-kernel@vger.kernel.org
Hi Andrew,
On Thu, May 18, 2017 at 6:09 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> On Thu, May 18, 2017 at 02:59:05PM +0200, Geert Uytterhoeven wrote:
>> If an Ethernet PHY is initialized before the interrupt controller it is
>> connected to, a message like the following is printed:
>>
>> irq: no irq domain found for /interrupt-controller@e61c0000 !
>>
>> However, the actual error is ignored, leading to a non-functional (-1)
>> PHY interrupt later:
>>
>> Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY driver [Micrel KSZ8041RNLI] (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=-1)
>>
>> Depending on whether the PHY driver will fall back to polling, Ethernet
>> may or may not work.
>>
>> To fix this:
>> 1. Switch of_mdiobus_register_phy() from irq_of_parse_and_map() to
>> of_irq_get().
>> Unlike the former, the latter returns -EPROBE_DEFER if the
>> interrupt controller is not yet available, so this condition can be
>> detected.
>> Other errors are handled the same as before, i.e. use the passed
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> mdio->irq[addr] as interrupt.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> 2. Propagate and handle errors from of_mdiobus_register_phy() and
>> of_mdiobus_register_device().
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> Seen on r8a7791/koelsch when using the new CPG/MSSR clock driver.
>> I assume it always happened on RZ/G1 in mainline.
>> ---
>> drivers/of/of_mdio.c | 39 +++++++++++++++++++++++++++------------
>> 1 file changed, 27 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
>> index 7e4c80f9b6cda0d3..f9ac2893f56184be 100644
>> --- a/drivers/of/of_mdio.c
>> +++ b/drivers/of/of_mdio.c
>> @@ -44,7 +44,7 @@ static int of_get_phy_id(struct device_node *device, u32 *phy_id)
>> return -EINVAL;
>> }
>>
>> -static void of_mdiobus_register_phy(struct mii_bus *mdio,
>> +static int of_mdiobus_register_phy(struct mii_bus *mdio,
>> struct device_node *child, u32 addr)
>> {
>> struct phy_device *phy;
>> @@ -60,9 +60,13 @@ static void of_mdiobus_register_phy(struct mii_bus *mdio,
>> else
>> phy = get_phy_device(mdio, addr, is_c45);
>> if (IS_ERR(phy))
>> - return;
>> + return PTR_ERR(phy);
>>
>> - rc = irq_of_parse_and_map(child, 0);
>> + rc = of_irq_get(child, 0);
>> + if (rc == -EPROBE_DEFER) {
>> + phy_device_free(phy);
>> + return rc;
>> + }
>
> Maybe this should be consistent. All other places there is an error,
> you return it. Here however, you only return the error if it is
> EPROBE_DEFER.
That's because of the "else" branch in the code below:
if (rc > 0) {
phy->irq = rc;
mdio->irq[addr] = rc;
} else {
phy->irq = mdio->irq[addr];
}
cfr. the marked part of the patch description.
I didn't want to change that behavior, as it's not clear to me why it's handled
that way.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] of_mdio: Fix broken PHY IRQ in case of probe deferral
2017-05-18 12:59 [PATCH] of_mdio: Fix broken PHY IRQ in case of probe deferral Geert Uytterhoeven
[not found] ` <1495112345-24795-1-git-send-email-geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
2017-05-18 16:09 ` Andrew Lunn
@ 2017-05-18 18:25 ` Florian Fainelli
2017-05-18 18:48 ` Geert Uytterhoeven
2 siblings, 1 reply; 17+ messages in thread
From: Florian Fainelli @ 2017-05-18 18:25 UTC (permalink / raw)
To: Geert Uytterhoeven, Andrew Lunn, Rob Herring, Frank Rowand
Cc: Thomas Petazzoni, Sergei Shtylyov, netdev, devicetree,
linux-renesas-soc, linux-kernel
On 05/18/2017 05:59 AM, Geert Uytterhoeven wrote:
> If an Ethernet PHY is initialized before the interrupt controller it is
> connected to, a message like the following is printed:
>
> irq: no irq domain found for /interrupt-controller@e61c0000 !
>
> However, the actual error is ignored, leading to a non-functional (-1)
> PHY interrupt later:
>
> Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY driver [Micrel KSZ8041RNLI] (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=-1)
>
> Depending on whether the PHY driver will fall back to polling, Ethernet
> may or may not work.
>
> To fix this:
> 1. Switch of_mdiobus_register_phy() from irq_of_parse_and_map() to
> of_irq_get().
> Unlike the former, the latter returns -EPROBE_DEFER if the
> interrupt controller is not yet available, so this condition can be
> detected.
> Other errors are handled the same as before, i.e. use the passed
> mdio->irq[addr] as interrupt.
> 2. Propagate and handle errors from of_mdiobus_register_phy() and
> of_mdiobus_register_device().
This most certainly works fine in the simple case where you have one PHY
hanging off the MDIO bus, now what happens if you have several?
Presumably, the first PHY that returns EPROBE_DEFER will make the entire
bus registration return EPROB_DEFER as well, and so on, and so forth,
but I am not sure if we will be properly unwinding the successful
registration of PHYs that either don't have an interrupt, or did not
return EPROBE_DEFER.
It should be possible to mimic this behavior by using the fixed PHY, and
possibly the dsa_loop.c driver which would create 4 ports, expecting 4
fixed PHYs to be present.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Seen on r8a7791/koelsch when using the new CPG/MSSR clock driver.
> I assume it always happened on RZ/G1 in mainline.
> ---
> drivers/of/of_mdio.c | 39 +++++++++++++++++++++++++++------------
> 1 file changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index 7e4c80f9b6cda0d3..f9ac2893f56184be 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -44,7 +44,7 @@ static int of_get_phy_id(struct device_node *device, u32 *phy_id)
> return -EINVAL;
> }
>
> -static void of_mdiobus_register_phy(struct mii_bus *mdio,
> +static int of_mdiobus_register_phy(struct mii_bus *mdio,
> struct device_node *child, u32 addr)
> {
> struct phy_device *phy;
> @@ -60,9 +60,13 @@ static void of_mdiobus_register_phy(struct mii_bus *mdio,
> else
> phy = get_phy_device(mdio, addr, is_c45);
> if (IS_ERR(phy))
> - return;
> + return PTR_ERR(phy);
>
> - rc = irq_of_parse_and_map(child, 0);
> + rc = of_irq_get(child, 0);
> + if (rc == -EPROBE_DEFER) {
> + phy_device_free(phy);
> + return rc;
> + }
> if (rc > 0) {
> phy->irq = rc;
> mdio->irq[addr] = rc;
> @@ -84,22 +88,23 @@ static void of_mdiobus_register_phy(struct mii_bus *mdio,
> if (rc) {
> phy_device_free(phy);
> of_node_put(child);
> - return;
> + return rc;
> }
>
> dev_dbg(&mdio->dev, "registered phy %s at address %i\n",
> child->name, addr);
> + return 0;
> }
>
> -static void of_mdiobus_register_device(struct mii_bus *mdio,
> - struct device_node *child, u32 addr)
> +static int of_mdiobus_register_device(struct mii_bus *mdio,
> + struct device_node *child, u32 addr)
> {
> struct mdio_device *mdiodev;
> int rc;
>
> mdiodev = mdio_device_create(mdio, addr);
> if (IS_ERR(mdiodev))
> - return;
> + return PTR_ERR(mdiodev);
>
> /* Associate the OF node with the device structure so it
> * can be looked up later.
> @@ -112,11 +117,12 @@ static void of_mdiobus_register_device(struct mii_bus *mdio,
> if (rc) {
> mdio_device_free(mdiodev);
> of_node_put(child);
> - return;
> + return rc;
> }
>
> dev_dbg(&mdio->dev, "registered mdio device %s at address %i\n",
> child->name, addr);
> + return 0;
> }
>
> int of_mdio_parse_addr(struct device *dev, const struct device_node *np)
> @@ -242,9 +248,11 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
> }
>
> if (of_mdiobus_child_is_phy(child))
> - of_mdiobus_register_phy(mdio, child, addr);
> + rc = of_mdiobus_register_phy(mdio, child, addr);
> else
> - of_mdiobus_register_device(mdio, child, addr);
> + rc = of_mdiobus_register_device(mdio, child, addr);
> + if (rc)
> + goto unregister;
> }
>
> if (!scanphys)
> @@ -265,12 +273,19 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
> dev_info(&mdio->dev, "scan phy %s at address %i\n",
> child->name, addr);
>
> - if (of_mdiobus_child_is_phy(child))
> - of_mdiobus_register_phy(mdio, child, addr);
> + if (of_mdiobus_child_is_phy(child)) {
> + rc = of_mdiobus_register_phy(mdio, child, addr);
> + if (rc)
> + goto unregister;
> + }
> }
> }
>
> return 0;
> +
> +unregister:
> + mdiobus_unregister(mdio);
> + return rc;
> }
> EXPORT_SYMBOL(of_mdiobus_register);
>
>
--
Florian
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] of_mdio: Fix broken PHY IRQ in case of probe deferral
2017-05-18 18:25 ` Florian Fainelli
@ 2017-05-18 18:48 ` Geert Uytterhoeven
2017-05-18 19:34 ` Andrew Lunn
0 siblings, 1 reply; 17+ messages in thread
From: Geert Uytterhoeven @ 2017-05-18 18:48 UTC (permalink / raw)
To: Florian Fainelli
Cc: Geert Uytterhoeven, Andrew Lunn, Rob Herring, Frank Rowand,
Thomas Petazzoni, Sergei Shtylyov, netdev@vger.kernel.org,
devicetree@vger.kernel.org, Linux-Renesas,
linux-kernel@vger.kernel.org
Hi Florian,
On Thu, May 18, 2017 at 8:25 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 05/18/2017 05:59 AM, Geert Uytterhoeven wrote:
>> If an Ethernet PHY is initialized before the interrupt controller it is
>> connected to, a message like the following is printed:
>>
>> irq: no irq domain found for /interrupt-controller@e61c0000 !
>>
>> However, the actual error is ignored, leading to a non-functional (-1)
>> PHY interrupt later:
>>
>> Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY driver [Micrel KSZ8041RNLI] (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=-1)
>>
>> Depending on whether the PHY driver will fall back to polling, Ethernet
>> may or may not work.
>>
>> To fix this:
>> 1. Switch of_mdiobus_register_phy() from irq_of_parse_and_map() to
>> of_irq_get().
>> Unlike the former, the latter returns -EPROBE_DEFER if the
>> interrupt controller is not yet available, so this condition can be
>> detected.
>> Other errors are handled the same as before, i.e. use the passed
>> mdio->irq[addr] as interrupt.
>> 2. Propagate and handle errors from of_mdiobus_register_phy() and
>> of_mdiobus_register_device().
>
> This most certainly works fine in the simple case where you have one PHY
> hanging off the MDIO bus, now what happens if you have several?
>
> Presumably, the first PHY that returns EPROBE_DEFER will make the entire
> bus registration return EPROB_DEFER as well, and so on, and so forth,
> but I am not sure if we will be properly unwinding the successful
> registration of PHYs that either don't have an interrupt, or did not
> return EPROBE_DEFER.
>
> It should be possible to mimic this behavior by using the fixed PHY, and
> possibly the dsa_loop.c driver which would create 4 ports, expecting 4
> fixed PHYs to be present.
mdiobus_unregister(), called from of_mdiobus_register() on failure,
should do the unwinding, right?
And when the driver is reprobed, all PHYs are reprobed, until they all
succeed.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] of_mdio: Fix broken PHY IRQ in case of probe deferral
2017-05-18 18:48 ` Geert Uytterhoeven
@ 2017-05-18 19:34 ` Andrew Lunn
2017-05-18 20:36 ` Geert Uytterhoeven
0 siblings, 1 reply; 17+ messages in thread
From: Andrew Lunn @ 2017-05-18 19:34 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Florian Fainelli, Geert Uytterhoeven, Rob Herring, Frank Rowand,
Thomas Petazzoni, Sergei Shtylyov, netdev@vger.kernel.org,
devicetree@vger.kernel.org, Linux-Renesas,
linux-kernel@vger.kernel.org
> > This most certainly works fine in the simple case where you have one PHY
> > hanging off the MDIO bus, now what happens if you have several?
> >
> > Presumably, the first PHY that returns EPROBE_DEFER will make the entire
> > bus registration return EPROB_DEFER as well, and so on, and so forth,
> > but I am not sure if we will be properly unwinding the successful
> > registration of PHYs that either don't have an interrupt, or did not
> > return EPROBE_DEFER.
> >
> > It should be possible to mimic this behavior by using the fixed PHY, and
> > possibly the dsa_loop.c driver which would create 4 ports, expecting 4
> > fixed PHYs to be present.
>
> mdiobus_unregister(), called from of_mdiobus_register() on failure,
> should do the unwinding, right?
>
> And when the driver is reprobed, all PHYs are reprobed, until they all
> succeed.
That is the theory. I looked at that while reviewing the patch. But
this has probably not been tested in anger. It would be good to test
this properly, with not just the first PHY returning -EPROBE_DEFER, to
really test the unwind.
Andrew
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] of_mdio: Fix broken PHY IRQ in case of probe deferral
2017-05-18 19:34 ` Andrew Lunn
@ 2017-05-18 20:36 ` Geert Uytterhoeven
2017-05-18 22:21 ` Florian Fainelli
0 siblings, 1 reply; 17+ messages in thread
From: Geert Uytterhoeven @ 2017-05-18 20:36 UTC (permalink / raw)
To: Andrew Lunn
Cc: Florian Fainelli, Geert Uytterhoeven, Rob Herring, Frank Rowand,
Thomas Petazzoni, Sergei Shtylyov, netdev@vger.kernel.org,
devicetree@vger.kernel.org, Linux-Renesas,
linux-kernel@vger.kernel.org
Hi Andrew,
On Thu, May 18, 2017 at 9:34 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> > This most certainly works fine in the simple case where you have one PHY
>> > hanging off the MDIO bus, now what happens if you have several?
>> >
>> > Presumably, the first PHY that returns EPROBE_DEFER will make the entire
>> > bus registration return EPROB_DEFER as well, and so on, and so forth,
>> > but I am not sure if we will be properly unwinding the successful
>> > registration of PHYs that either don't have an interrupt, or did not
>> > return EPROBE_DEFER.
>> >
>> > It should be possible to mimic this behavior by using the fixed PHY, and
>> > possibly the dsa_loop.c driver which would create 4 ports, expecting 4
>> > fixed PHYs to be present.
>>
>> mdiobus_unregister(), called from of_mdiobus_register() on failure,
>> should do the unwinding, right?
>>
>> And when the driver is reprobed, all PHYs are reprobed, until they all
>> succeed.
>
> That is the theory. I looked at that while reviewing the patch. But
> this has probably not been tested in anger. It would be good to test
> this properly, with not just the first PHY returning -EPROBE_DEFER, to
> really test the unwind.
Unfortunately I don't have a board with multiple PHYs, so I cannot test
that case.
Does unbinding/rebinding a network driver with multiple PHYs currently
work? Or module unload/reload?
That should exercise a similar code path.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] of_mdio: Fix broken PHY IRQ in case of probe deferral
2017-05-18 20:36 ` Geert Uytterhoeven
@ 2017-05-18 22:21 ` Florian Fainelli
[not found] ` <08a89dd5-b707-8b8f-b8e1-e20b9ed630b7-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 17+ messages in thread
From: Florian Fainelli @ 2017-05-18 22:21 UTC (permalink / raw)
To: Geert Uytterhoeven, Andrew Lunn
Cc: Geert Uytterhoeven, Rob Herring, Frank Rowand, Thomas Petazzoni,
Sergei Shtylyov, netdev@vger.kernel.org,
devicetree@vger.kernel.org, Linux-Renesas,
linux-kernel@vger.kernel.org
On 05/18/2017 01:36 PM, Geert Uytterhoeven wrote:
> Hi Andrew,
>
> On Thu, May 18, 2017 at 9:34 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>>>> This most certainly works fine in the simple case where you have one PHY
>>>> hanging off the MDIO bus, now what happens if you have several?
>>>>
>>>> Presumably, the first PHY that returns EPROBE_DEFER will make the entire
>>>> bus registration return EPROB_DEFER as well, and so on, and so forth,
>>>> but I am not sure if we will be properly unwinding the successful
>>>> registration of PHYs that either don't have an interrupt, or did not
>>>> return EPROBE_DEFER.
>>>>
>>>> It should be possible to mimic this behavior by using the fixed PHY, and
>>>> possibly the dsa_loop.c driver which would create 4 ports, expecting 4
>>>> fixed PHYs to be present.
>>>
>>> mdiobus_unregister(), called from of_mdiobus_register() on failure,
>>> should do the unwinding, right?
>>>
>>> And when the driver is reprobed, all PHYs are reprobed, until they all
>>> succeed.
>>
>> That is the theory. I looked at that while reviewing the patch. But
>> this has probably not been tested in anger. It would be good to test
>> this properly, with not just the first PHY returning -EPROBE_DEFER, to
>> really test the unwind.
>
> Unfortunately I don't have a board with multiple PHYs, so I cannot test
> that case.
>
> Does unbinding/rebinding a network driver with multiple PHYs currently
> work? Or module unload/reload?
Usually there is a strict 1:1 mapping between a network device (not
driver) and a PHY device, switch drivers however, would have multiple
PHYs (one per port, aka net_deice).
NB: binding and unbinding of PHYs is pretty broken at the moment though,
because there is a complete disconnect between what the Ethernet MAC
expects, and the state in which the PHY is. I had some patches to fix
that, but this turned out to be playing whack-a-mole which I typically
suck at.
--
Florian
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-07-09 19:38 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-18 12:59 [PATCH] of_mdio: Fix broken PHY IRQ in case of probe deferral Geert Uytterhoeven
[not found] ` <1495112345-24795-1-git-send-email-geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
2017-05-18 15:21 ` David Miller
2017-05-18 16:09 ` Andrew Lunn
2017-05-18 16:13 ` Geert Uytterhoeven
[not found] ` <CAMuHMdW_ZSn_nXSNRTHQwFioRD5UTotxKgm1eMqAP06n+cWv+g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-05-18 16:33 ` Andrew Lunn
2017-05-18 17:38 ` Geert Uytterhoeven
2017-05-18 18:25 ` Florian Fainelli
2017-05-18 18:48 ` Geert Uytterhoeven
2017-05-18 19:34 ` Andrew Lunn
2017-05-18 20:36 ` Geert Uytterhoeven
2017-05-18 22:21 ` Florian Fainelli
[not found] ` <08a89dd5-b707-8b8f-b8e1-e20b9ed630b7-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-05-23 9:36 ` Geert Uytterhoeven
2017-06-06 9:43 ` Geert Uytterhoeven
[not found] ` <CAMuHMdUt6yOQ9=02z_7WG58YGxFYD=_zn7SeLEqGmfTrNBFJXw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-07-02 20:37 ` Geert Uytterhoeven
2017-07-09 16:49 ` Florian Fainelli
2017-07-09 17:28 ` Andrew Lunn
2017-07-09 19:38 ` Geert Uytterhoeven
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).