bcm63xx_enet: fix internal phy IRQ assignment

PATCH Superseded Daniel González Cabanelas 4 participants 22 messages Feb 24, 2021 – Feb 26, 2021

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Daniel González Cabanelas

author

2021-02-24 16:05 UTC #0 lore raw

PATCH v2 1/1 bcm63xx_enet: fix internal phy IRQ assignment

The current bcm63xx_enet driver doesn't asign the internal phy IRQ. As a result of this it works in polling mode.

Fix it using the phy_device structure to assign the platform IRQ.

Tested under a BCM6348 board. Kernel dmesg before the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom

              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)

After the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom

              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=17)

Pluging and uplugging the ethernet cable now generates interrupts and the PHY goes up and down as expected.

Signed-off-by Daniel González Cabanelas <[email protected]>

---
changes in V2:
- snippet moved after the mdiobus registration
- added missing brackets

 drivers/net/ethernet/broadcom/bcm63xx_enet.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

drivers/net/ethernet/broadcom/bcm63xx_enet.c +11 -2

index fd876721316..dd218722560 100644
@@ -1818,10 +1818,19 @@ static int bcm_enet_probe(struct platform_device *pdev)
 		 * if a slave is not present on hw */
 		bus->phy_mask = ~(1 << priv->phy_id);
 
-		if (priv->has_phy_interrupt)
+		ret = mdiobus_register(bus);
+
+		if (priv->has_phy_interrupt) {
+			phydev = mdiobus_get_phy(bus, priv->phy_id);
+			if (!phydev) {
+				dev_err(&dev->dev, "no PHY found\n");
+				goto out_unregister_mdio;
+			}
+
 			bus->irq[priv->phy_id] = priv->phy_interrupt;
+			phydev->irq = priv->phy_interrupt;
+		}
 
-		ret = mdiobus_register(bus);
 		if (ret) {
 			dev_err(&pdev->dev, "unable to register mdio bus\n");
 			goto out_free_mdio;

Signature · 1 line

2.30.1

Heiner Kallweit

2021-02-24 21:45 UTC #1 lore raw

Quoted 38 lines · Daniel González Cabanelas

The current bcm63xx_enet driver doesn't asign the internal phy IRQ. As a result of this it works in polling mode.
Fix it using the phy_device structure to assign the platform IRQ.
Tested under a BCM6348 board. Kernel dmesg before the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
After the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=17)
Pluging and uplugging the ethernet cable now generates interrupts and the PHY goes up and down as expected.
Signed-off-by Daniel González Cabanelas <[email protected]>
---
changes in V2:
- snippet moved after the mdiobus registration
- added missing brackets
 drivers/net/ethernet/broadcom/bcm63xx_enet.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/net/ethernet/broadcom/bcm63xx_enet.c +4 -1
index fd876721316..dd218722560 100644
@@ -1818,10 +1818,19 @@ static int bcm_enet_probe(struct platform_device *pdev)
 		 * if a slave is not present on hw */
 		bus->phy_mask = ~(1 << priv->phy_id);
 
-		if (priv->has_phy_interrupt)
+		ret = mdiobus_register(bus);
+
+		if (priv->has_phy_interrupt) {
+			phydev = mdiobus_get_phy(bus, priv->phy_id);

drivers/net/ethernet/broadcom/bcm63xx_enet.c static int bcm_enet_probe(struct platform_device *pdev) · line 1824

Quoted 10 lines

drivers/net/ethernet/broadcom/bcm63xx_enet.c +7 -1

+			if (!phydev) {
+				dev_err(&dev->dev, "no PHY found\n");
+				goto out_unregister_mdio;
+			}
+
 			bus->irq[priv->phy_id] = priv->phy_interrupt;
+			phydev->irq = priv->phy_interrupt;
+		}
 
-		ret = mdiobus_register(bus);

You shouldn't have to set phydev->irq, this is done by phy_device_create(). For this to work bus->irq[] needs to be set before calling mdiobus_register().

Quoted 4 lines

drivers/net/ethernet/broadcom/bcm63xx_enet.c +0 -0

 		if (ret) {
 			dev_err(&pdev->dev, "unable to register mdio bus\n");
 			goto out_free_mdio;

Florian Fainelli

2021-02-24 22:02 UTC #2 lore raw

Quoted 48 lines · Heiner Kallweit

Quoted 47 lines · Daniel González Cabanelas
The current bcm63xx_enet driver doesn't asign the internal phy IRQ. As a result of this it works in polling mode.
Fix it using the phy_device structure to assign the platform IRQ.
Tested under a BCM6348 board. Kernel dmesg before the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
After the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=17)
Pluging and uplugging the ethernet cable now generates interrupts and the PHY goes up and down as expected.
Signed-off-by Daniel González Cabanelas <[email protected]>
---
changes in V2:
- snippet moved after the mdiobus registration
- added missing brackets
 drivers/net/ethernet/broadcom/bcm63xx_enet.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/net/ethernet/broadcom/bcm63xx_enet.c +11 -1
index fd876721316..dd218722560 100644
@@ -1818,10 +1818,19 @@ static int bcm_enet_probe(struct platform_device *pdev)
 		 * if a slave is not present on hw */
 		bus->phy_mask = ~(1 << priv->phy_id);
 
-		if (priv->has_phy_interrupt)
+		ret = mdiobus_register(bus);
+
+		if (priv->has_phy_interrupt) {
+			phydev = mdiobus_get_phy(bus, priv->phy_id);
+			if (!phydev) {
+				dev_err(&dev->dev, "no PHY found\n");
+				goto out_unregister_mdio;
+			}
+
 			bus->irq[priv->phy_id] = priv->phy_interrupt;
+			phydev->irq = priv->phy_interrupt;
+		}
 

Quoted 4 lines

drivers/net/ethernet/broadcom/bcm63xx_enet.c static int bcm_enet_probe(struct platform_device *pdev) · line 1824
Quoted 1 line
drivers/net/ethernet/broadcom/bcm63xx_enet.c +0 -1
-		ret = mdiobus_register(bus);
You shouldn't have to set phydev->irq, this is done by phy_device_create(). For this to work bus->irq[] needs to be set before calling mdiobus_register().

Yes good point, and that is what the unchanged code does actually. Daniel, any idea why that is not working?

Signature · 1 line

Florian

Daniel González Cabanelas

author

2021-02-24 23:56 UTC #3 lore raw

Quoted 52 lines · Florian Fainelli

Quoted 48 lines · Heiner Kallweit
Quoted 47 lines · Daniel González Cabanelas
The current bcm63xx_enet driver doesn't asign the internal phy IRQ. As a result of this it works in polling mode.
Fix it using the phy_device structure to assign the platform IRQ.
Tested under a BCM6348 board. Kernel dmesg before the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
After the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=17)
Pluging and uplugging the ethernet cable now generates interrupts and the PHY goes up and down as expected.
Signed-off-by Daniel González Cabanelas <[email protected]>
---
changes in V2:
- snippet moved after the mdiobus registration
- added missing brackets
 drivers/net/ethernet/broadcom/bcm63xx_enet.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/net/ethernet/broadcom/bcm63xx_enet.c +11 -1
index fd876721316..dd218722560 100644
@@ -1818,10 +1818,19 @@ static int bcm_enet_probe(struct platform_device *pdev)
              * if a slave is not present on hw */
             bus->phy_mask = ~(1 << priv->phy_id);
 
-            if (priv->has_phy_interrupt)
+            ret = mdiobus_register(bus);
+
+            if (priv->has_phy_interrupt) {
+                    phydev = mdiobus_get_phy(bus, priv->phy_id);
+                    if (!phydev) {
+                            dev_err(&dev->dev, "no PHY found\n");
+                            goto out_unregister_mdio;
+                    }
+
                     bus->irq[priv->phy_id] = priv->phy_interrupt;
+                    phydev->irq = priv->phy_interrupt;
+            }
 

Quoted 7 lines

Quoted 4 lines
drivers/net/ethernet/broadcom/bcm63xx_enet.c static int bcm_enet_probe(struct platform_device *pdev) · line 1824
Quoted 1 line
drivers/net/ethernet/broadcom/bcm63xx_enet.c +0 -1
-            ret = mdiobus_register(bus);
You shouldn't have to set phydev->irq, this is done by phy_device_create(). For this to work bus->irq[] needs to be set before calling mdiobus_register().
Yes good point, and that is what the unchanged code does actually. Daniel, any idea why that is not working?

Hi Florian, I don't know. bus->irq[] has no effect, only assigning the IRQ through phydev->irq works.

I can resend the patch without the bus->irq[] line since it's pointless in this scenario.

Regards

Quoted 2 lines

--
Florian

Heiner Kallweit

2021-02-25 07:23 UTC #4 lore raw

Quoted 59 lines · Daniel González Cabanelas

Quoted 57 lines · Florian Fainelli
Quoted 52 lines · Heiner Kallweit
Quoted 38 lines · Daniel González Cabanelas
The current bcm63xx_enet driver doesn't asign the internal phy IRQ. As a result of this it works in polling mode.
Fix it using the phy_device structure to assign the platform IRQ.
Tested under a BCM6348 board. Kernel dmesg before the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
After the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=17)
Pluging and uplugging the ethernet cable now generates interrupts and the PHY goes up and down as expected.
Signed-off-by Daniel González Cabanelas <[email protected]>
---
changes in V2:
- snippet moved after the mdiobus registration
- added missing brackets
 drivers/net/ethernet/broadcom/bcm63xx_enet.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/net/ethernet/broadcom/bcm63xx_enet.c +4 -1
index fd876721316..dd218722560 100644
@@ -1818,10 +1818,19 @@ static int bcm_enet_probe(struct platform_device *pdev)
              * if a slave is not present on hw */
             bus->phy_mask = ~(1 << priv->phy_id);
 
-            if (priv->has_phy_interrupt)
+            ret = mdiobus_register(bus);
+
+            if (priv->has_phy_interrupt) {
+                    phydev = mdiobus_get_phy(bus, priv->phy_id);
drivers/net/ethernet/broadcom/bcm63xx_enet.c static int bcm_enet_probe(struct platform_device *pdev) · line 1824
Quoted 10 lines
drivers/net/ethernet/broadcom/bcm63xx_enet.c +7 -1
+                    if (!phydev) {
+                            dev_err(&dev->dev, "no PHY found\n");
+                            goto out_unregister_mdio;
+                    }
+
                     bus->irq[priv->phy_id] = priv->phy_interrupt;
+                    phydev->irq = priv->phy_interrupt;
+            }
 
-            ret = mdiobus_register(bus);
You shouldn't have to set phydev->irq, this is done by phy_device_create(). For this to work bus->irq[] needs to be set before calling mdiobus_register().

Quoted 9 lines

Quoted 2 lines
Yes good point, and that is what the unchanged code does actually. Daniel, any idea why that is not working?
Hi Florian, I don't know. bus->irq[] has no effect, only assigning the IRQ through phydev->irq works.
I can resend the patch without the bus->irq[] line since it's pointless in this scenario.

It's still an ugly workaround and a proper root cause analysis should be done first. I can only imagine that phydev->irq is overwritten in phy_probe() because phy_drv_supports_irq() is false. Can you please check whether phydev->irq is properly set in phy_device_create(), and if yes, whether it's reset to PHY_POLL in phy_probe()?.

On which kernel version do you face this problem?

Quoted 3 lines

Regards
Quoted 2 lines
--
Florian

Daniel González Cabanelas

author

2021-02-25 16:38 UTC #5 lore raw

Quoted 71 lines · Heiner Kallweit

Quoted 59 lines · Daniel González Cabanelas
Quoted 57 lines · Florian Fainelli
Quoted 52 lines · Heiner Kallweit
Quoted 38 lines · Daniel González Cabanelas
The current bcm63xx_enet driver doesn't asign the internal phy IRQ. As a result of this it works in polling mode.
Fix it using the phy_device structure to assign the platform IRQ.
Tested under a BCM6348 board. Kernel dmesg before the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
After the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=17)
Pluging and uplugging the ethernet cable now generates interrupts and the PHY goes up and down as expected.
Signed-off-by Daniel González Cabanelas <[email protected]>
---
changes in V2:
- snippet moved after the mdiobus registration
- added missing brackets
 drivers/net/ethernet/broadcom/bcm63xx_enet.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/net/ethernet/broadcom/bcm63xx_enet.c +4 -1
index fd876721316..dd218722560 100644
@@ -1818,10 +1818,19 @@ static int bcm_enet_probe(struct platform_device *pdev)
              * if a slave is not present on hw */
             bus->phy_mask = ~(1 << priv->phy_id);
 
-            if (priv->has_phy_interrupt)
+            ret = mdiobus_register(bus);
+
+            if (priv->has_phy_interrupt) {
+                    phydev = mdiobus_get_phy(bus, priv->phy_id);
drivers/net/ethernet/broadcom/bcm63xx_enet.c static int bcm_enet_probe(struct platform_device *pdev) · line 1824
Quoted 10 lines
drivers/net/ethernet/broadcom/bcm63xx_enet.c +7 -1
+                    if (!phydev) {
+                            dev_err(&dev->dev, "no PHY found\n");
+                            goto out_unregister_mdio;
+                    }
+
                     bus->irq[priv->phy_id] = priv->phy_interrupt;
+                    phydev->irq = priv->phy_interrupt;
+            }
 
-            ret = mdiobus_register(bus);
You shouldn't have to set phydev->irq, this is done by phy_device_create(). For this to work bus->irq[] needs to be set before calling mdiobus_register().
Quoted 9 lines
Quoted 2 lines
Yes good point, and that is what the unchanged code does actually. Daniel, any idea why that is not working?
Hi Florian, I don't know. bus->irq[] has no effect, only assigning the IRQ through phydev->irq works.
I can resend the patch without the bus->irq[] line since it's pointless in this scenario.

Quoted 6 lines

It's still an ugly workaround and a proper root cause analysis should be done first. I can only imagine that phydev->irq is overwritten in phy_probe() because phy_drv_supports_irq() is false. Can you please check whether phydev->irq is properly set in phy_device_create(), and if yes, whether it's reset to PHY_POLL in phy_probe()?.

Hi Heiner, I added some kernel prints:

[    2.712519] libphy: Fixed MDIO Bus: probed
[    2.721969] =======phy_device_create===========
[    2.726841] phy_device_create: dev->irq = 17
[    2.726841]
[    2.832620] =======phy_probe===========
[    2.836846] phy_probe: phydev->irq = 17
[    2.840950] phy_probe: phy_drv_supports_irq = 0, phy_interrupt_is_valid = 1
[    2.848267] phy_probe: phydev->irq = -1
[    2.848267]
[    2.854059] =======phy_probe===========
[    2.858174] phy_probe: phydev->irq = -1
[    2.862253] phy_probe: phydev->irq = -1
[    2.862253]
[    2.868121] libphy: bcm63xx_enet MII bus: probed
[    2.873320] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY

driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)

Currently using kernel 5.4.99. I still have no idea what's going on.

Quoted 2 lines

On which kernel version do you face this problem?

The kernel version 4.4 works ok. The minimum version where I found the problem were the kernel 4.9.111, now using 5.4. And 5.10 also tested.

Regards
Daniel

Quoted 4 lines

Quoted 3 lines
Regards
Quoted 2 lines
--
Florian

Heiner Kallweit

2021-02-25 20:06 UTC #6 lore raw

Quoted 97 lines · Daniel González Cabanelas

Quoted 71 lines · Heiner Kallweit
Quoted 59 lines · Daniel González Cabanelas
Quoted 57 lines · Florian Fainelli
Quoted 52 lines · Heiner Kallweit
Quoted 38 lines · Daniel González Cabanelas
The current bcm63xx_enet driver doesn't asign the internal phy IRQ. As a result of this it works in polling mode.
Fix it using the phy_device structure to assign the platform IRQ.
Tested under a BCM6348 board. Kernel dmesg before the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
After the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=17)
Pluging and uplugging the ethernet cable now generates interrupts and the PHY goes up and down as expected.
Signed-off-by Daniel González Cabanelas <[email protected]>
---
changes in V2:
- snippet moved after the mdiobus registration
- added missing brackets
 drivers/net/ethernet/broadcom/bcm63xx_enet.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/net/ethernet/broadcom/bcm63xx_enet.c +4 -1
index fd876721316..dd218722560 100644
@@ -1818,10 +1818,19 @@ static int bcm_enet_probe(struct platform_device *pdev)
              * if a slave is not present on hw */
             bus->phy_mask = ~(1 << priv->phy_id);
 
-            if (priv->has_phy_interrupt)
+            ret = mdiobus_register(bus);
+
+            if (priv->has_phy_interrupt) {
+                    phydev = mdiobus_get_phy(bus, priv->phy_id);
drivers/net/ethernet/broadcom/bcm63xx_enet.c static int bcm_enet_probe(struct platform_device *pdev) · line 1824
Quoted 10 lines
drivers/net/ethernet/broadcom/bcm63xx_enet.c +7 -1
+                    if (!phydev) {
+                            dev_err(&dev->dev, "no PHY found\n");
+                            goto out_unregister_mdio;
+                    }
+
                     bus->irq[priv->phy_id] = priv->phy_interrupt;
+                    phydev->irq = priv->phy_interrupt;
+            }
 
-            ret = mdiobus_register(bus);
You shouldn't have to set phydev->irq, this is done by phy_device_create(). For this to work bus->irq[] needs to be set before calling mdiobus_register().
Quoted 9 lines
Quoted 2 lines
Yes good point, and that is what the unchanged code does actually. Daniel, any idea why that is not working?
Hi Florian, I don't know. bus->irq[] has no effect, only assigning the IRQ through phydev->irq works.
I can resend the patch without the bus->irq[] line since it's pointless in this scenario.
Quoted 6 lines
It's still an ugly workaround and a proper root cause analysis should be done first. I can only imagine that phydev->irq is overwritten in phy_probe() because phy_drv_supports_irq() is false. Can you please check whether phydev->irq is properly set in phy_device_create(), and if yes, whether it's reset to PHY_POLL in phy_probe()?.
Hi Heiner, I added some kernel prints:
[    2.712519] libphy: Fixed MDIO Bus: probed
[    2.721969] =======phy_device_create===========
[    2.726841] phy_device_create: dev->irq = 17
[    2.726841]
[    2.832620] =======phy_probe===========
[    2.836846] phy_probe: phydev->irq = 17
[    2.840950] phy_probe: phy_drv_supports_irq = 0, phy_interrupt_is_valid = 1
[    2.848267] phy_probe: phydev->irq = -1
[    2.848267]
[    2.854059] =======phy_probe===========
[    2.858174] phy_probe: phydev->irq = -1
[    2.862253] phy_probe: phydev->irq = -1
[    2.862253]
[    2.868121] libphy: bcm63xx_enet MII bus: probed
[    2.873320] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY

Quoted 5 lines

driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
Currently using kernel 5.4.99. I still have no idea what's going on.

Thanks for debugging. This confirms my assumption that the interrupt is overwritten in phy_probe(). I'm just scratching my head how phy_drv_supports_irq() can return 0. In 5.4.99 it's defined as:

static bool phy_drv_supports_irq(struct phy_driver *phydrv)

{
	return phydrv->config_intr && phydrv->ack_interrupt;
}

And that's the PHY driver:

static struct phy_driver bcm63xx_driver[] = {

{
	.phy_id		= 0x00406000,
	.phy_id_mask	= 0xfffffc00,
	.name		= "Broadcom BCM63XX (1)",
	/* PHY_BASIC_FEATURES */
	.flags		= PHY_IS_INTERNAL,
	.config_init	= bcm63xx_config_init,
	.ack_interrupt	= bcm_phy_ack_intr,
	.config_intr	= bcm63xx_config_intr,
}

So both callbacks are set. Can you extend your debugging and check in phy_drv_supports_irq() which of the callbacks is missing?

Last but not least: Do you use a mainline kernel, or is it maybe a modified downstream kernel? In the latter case, please check in your kernel sources whether both callbacks are set.

Quoted 12 lines

Quoted 2 lines
On which kernel version do you face this problem?
The kernel version 4.4 works ok. The minimum version where I found the problem were the kernel 4.9.111, now using 5.4. And 5.10 also tested.
Regards
Daniel
Quoted 4 lines
Quoted 3 lines
Regards
Quoted 2 lines
--
Florian

Daniel González Cabanelas

author

2021-02-25 22:29 UTC #7 lore raw

Quoted 127 lines · Heiner Kallweit

Quoted 97 lines · Daniel González Cabanelas
Quoted 71 lines · Heiner Kallweit
Quoted 59 lines · Daniel González Cabanelas
Quoted 57 lines · Florian Fainelli
Quoted 52 lines · Heiner Kallweit
Quoted 38 lines · Daniel González Cabanelas
The current bcm63xx_enet driver doesn't asign the internal phy IRQ. As a result of this it works in polling mode.
Fix it using the phy_device structure to assign the platform IRQ.
Tested under a BCM6348 board. Kernel dmesg before the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
After the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=17)
Pluging and uplugging the ethernet cable now generates interrupts and the PHY goes up and down as expected.
Signed-off-by Daniel González Cabanelas <[email protected]>
---
changes in V2:
- snippet moved after the mdiobus registration
- added missing brackets
 drivers/net/ethernet/broadcom/bcm63xx_enet.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/net/ethernet/broadcom/bcm63xx_enet.c +4 -1
index fd876721316..dd218722560 100644
@@ -1818,10 +1818,19 @@ static int bcm_enet_probe(struct platform_device *pdev)
              * if a slave is not present on hw */
             bus->phy_mask = ~(1 << priv->phy_id);
 
-            if (priv->has_phy_interrupt)
+            ret = mdiobus_register(bus);
+
+            if (priv->has_phy_interrupt) {
+                    phydev = mdiobus_get_phy(bus, priv->phy_id);
drivers/net/ethernet/broadcom/bcm63xx_enet.c static int bcm_enet_probe(struct platform_device *pdev) · line 1824
Quoted 10 lines
drivers/net/ethernet/broadcom/bcm63xx_enet.c +7 -1
+                    if (!phydev) {
+                            dev_err(&dev->dev, "no PHY found\n");
+                            goto out_unregister_mdio;
+                    }
+
                     bus->irq[priv->phy_id] = priv->phy_interrupt;
+                    phydev->irq = priv->phy_interrupt;
+            }
 
-            ret = mdiobus_register(bus);
You shouldn't have to set phydev->irq, this is done by phy_device_create(). For this to work bus->irq[] needs to be set before calling mdiobus_register().
Quoted 9 lines
Quoted 2 lines
Yes good point, and that is what the unchanged code does actually. Daniel, any idea why that is not working?
Hi Florian, I don't know. bus->irq[] has no effect, only assigning the IRQ through phydev->irq works.
I can resend the patch without the bus->irq[] line since it's pointless in this scenario.
Quoted 6 lines
It's still an ugly workaround and a proper root cause analysis should be done first. I can only imagine that phydev->irq is overwritten in phy_probe() because phy_drv_supports_irq() is false. Can you please check whether phydev->irq is properly set in phy_device_create(), and if yes, whether it's reset to PHY_POLL in phy_probe()?.
Hi Heiner, I added some kernel prints:
[    2.712519] libphy: Fixed MDIO Bus: probed
[    2.721969] =======phy_device_create===========
[    2.726841] phy_device_create: dev->irq = 17
[    2.726841]
[    2.832620] =======phy_probe===========
[    2.836846] phy_probe: phydev->irq = 17
[    2.840950] phy_probe: phy_drv_supports_irq = 0, phy_interrupt_is_valid = 1
[    2.848267] phy_probe: phydev->irq = -1
[    2.848267]
[    2.854059] =======phy_probe===========
[    2.858174] phy_probe: phydev->irq = -1
[    2.862253] phy_probe: phydev->irq = -1
[    2.862253]
[    2.868121] libphy: bcm63xx_enet MII bus: probed
[    2.873320] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
Quoted 5 lines
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
Currently using kernel 5.4.99. I still have no idea what's going on.
Thanks for debugging. This confirms my assumption that the interrupt is overwritten in phy_probe(). I'm just scratching my head how phy_drv_supports_irq() can return 0. In 5.4.99 it's defined as:
static bool phy_drv_supports_irq(struct phy_driver *phydrv)
{
        return phydrv->config_intr && phydrv->ack_interrupt;
}
And that's the PHY driver:
static struct phy_driver bcm63xx_driver[] = {
{
        .phy_id         = 0x00406000,
        .phy_id_mask    = 0xfffffc00,
        .name           = "Broadcom BCM63XX (1)",
/* PHY_BASIC_FEATURES */
.flags = PHY_IS_INTERNAL,
.config_init = bcm63xx_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm63xx_config_intr,
}

Quoted 3 lines

So both callbacks are set. Can you extend your debugging and check in phy_drv_supports_irq() which of the callbacks is missing?

Hi, both callbacks are missing on the first check. However on the next calls they're there.

[    2.263909] libphy: Fixed MDIO Bus: probed
[    2.273026] =======phy_device_create===========
[    2.277908] phy_device_create: dev->irq = 17
[    2.277908]
[    2.373104] =======phy_probe===========
[    2.377336] phy_probe: phydev->irq = 17
[    2.381445] phy_drv_supports_irq: phydrv->config_intr = 0,
phydrv->ack_interrupt = 0
[    2.389554] phydev->irq = PHY_POLL;
[    2.393186] phy_probe: phydev->irq = -1
[    2.393186]
[    2.398987] =======phy_probe===========
[    2.403108] phy_probe: phydev->irq = -1
[    2.407195] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.415314] phy_probe: phydev->irq = -1
[    2.415314]
[    2.421189] libphy: bcm63xx_enet MII bus: probed
[    2.426129] =======phy_connect===========
[    2.430410] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.438537] phy_connect: phy_drv_supports_irq = 1
[    2.438537]
[    2.445284] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY

driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)

I also added the prints to phy_connect.

Quoted 4 lines

Last but not least: Do you use a mainline kernel, or is it maybe a modified downstream kernel? In the latter case, please check in your kernel sources whether both callbacks are set.

It's a modified kernel, and the the callbacks are set. BTW I also tested the kernel with no patches concerning to the ethernet driver.

Regards,
Daniel

Quoted 15 lines

Quoted 14 lines
Quoted 4 lines
On which kernel version do you face this problem?
The kernel version 4.4 works ok. The minimum version where I found the problem were the kernel 4.9.111, now using 5.4. And 5.10 also tested.
Regards
Daniel
Quoted 4 lines
Quoted 3 lines
Regards
Quoted 2 lines
--
Florian

Heiner Kallweit

2021-02-25 22:57 UTC #8 lore raw

Quoted 129 lines · Daniel González Cabanelas

Quoted 127 lines · Heiner Kallweit
Quoted 97 lines · Daniel González Cabanelas
Quoted 71 lines · Heiner Kallweit
Quoted 59 lines · Daniel González Cabanelas
Quoted 57 lines · Florian Fainelli
Quoted 52 lines · Heiner Kallweit
Quoted 38 lines · Daniel González Cabanelas
The current bcm63xx_enet driver doesn't asign the internal phy IRQ. As a result of this it works in polling mode.
Fix it using the phy_device structure to assign the platform IRQ.
Tested under a BCM6348 board. Kernel dmesg before the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
After the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=17)
Pluging and uplugging the ethernet cable now generates interrupts and the PHY goes up and down as expected.
Signed-off-by Daniel González Cabanelas <[email protected]>
---
changes in V2:
- snippet moved after the mdiobus registration
- added missing brackets
 drivers/net/ethernet/broadcom/bcm63xx_enet.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/net/ethernet/broadcom/bcm63xx_enet.c +4 -1
index fd876721316..dd218722560 100644
@@ -1818,10 +1818,19 @@ static int bcm_enet_probe(struct platform_device *pdev)
              * if a slave is not present on hw */
             bus->phy_mask = ~(1 << priv->phy_id);
 
-            if (priv->has_phy_interrupt)
+            ret = mdiobus_register(bus);
+
+            if (priv->has_phy_interrupt) {
+                    phydev = mdiobus_get_phy(bus, priv->phy_id);
drivers/net/ethernet/broadcom/bcm63xx_enet.c static int bcm_enet_probe(struct platform_device *pdev) · line 1824
Quoted 10 lines
drivers/net/ethernet/broadcom/bcm63xx_enet.c +7 -1
+                    if (!phydev) {
+                            dev_err(&dev->dev, "no PHY found\n");
+                            goto out_unregister_mdio;
+                    }
+
                     bus->irq[priv->phy_id] = priv->phy_interrupt;
+                    phydev->irq = priv->phy_interrupt;
+            }
 
-            ret = mdiobus_register(bus);
You shouldn't have to set phydev->irq, this is done by phy_device_create(). For this to work bus->irq[] needs to be set before calling mdiobus_register().
Quoted 9 lines
Quoted 2 lines
Yes good point, and that is what the unchanged code does actually. Daniel, any idea why that is not working?
Hi Florian, I don't know. bus->irq[] has no effect, only assigning the IRQ through phydev->irq works.
I can resend the patch without the bus->irq[] line since it's pointless in this scenario.
Quoted 6 lines
It's still an ugly workaround and a proper root cause analysis should be done first. I can only imagine that phydev->irq is overwritten in phy_probe() because phy_drv_supports_irq() is false. Can you please check whether phydev->irq is properly set in phy_device_create(), and if yes, whether it's reset to PHY_POLL in phy_probe()?.
Hi Heiner, I added some kernel prints:
[    2.712519] libphy: Fixed MDIO Bus: probed
[    2.721969] =======phy_device_create===========
[    2.726841] phy_device_create: dev->irq = 17
[    2.726841]
[    2.832620] =======phy_probe===========
[    2.836846] phy_probe: phydev->irq = 17
[    2.840950] phy_probe: phy_drv_supports_irq = 0, phy_interrupt_is_valid = 1
[    2.848267] phy_probe: phydev->irq = -1
[    2.848267]
[    2.854059] =======phy_probe===========
[    2.858174] phy_probe: phydev->irq = -1
[    2.862253] phy_probe: phydev->irq = -1
[    2.862253]
[    2.868121] libphy: bcm63xx_enet MII bus: probed
[    2.873320] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
Quoted 5 lines
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
Currently using kernel 5.4.99. I still have no idea what's going on.
Thanks for debugging. This confirms my assumption that the interrupt is overwritten in phy_probe(). I'm just scratching my head how phy_drv_supports_irq() can return 0. In 5.4.99 it's defined as:
static bool phy_drv_supports_irq(struct phy_driver *phydrv)
{
        return phydrv->config_intr && phydrv->ack_interrupt;
}
And that's the PHY driver:
static struct phy_driver bcm63xx_driver[] = {
{
        .phy_id         = 0x00406000,
        .phy_id_mask    = 0xfffffc00,
        .name           = "Broadcom BCM63XX (1)",
/* PHY_BASIC_FEATURES */
.flags = PHY_IS_INTERNAL,
.config_init = bcm63xx_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm63xx_config_intr,
}

Quoted 8 lines

Quoted 3 lines
So both callbacks are set. Can you extend your debugging and check in phy_drv_supports_irq() which of the callbacks is missing?
Hi, both callbacks are missing on the first check. However on the next calls they're there.
[    2.263909] libphy: Fixed MDIO Bus: probed

This is weird. The phy_device seems to show up on both MDIO buses, the fixed one *and* the bcm63xx_enet bus.
I assume that if you build your kernel w/o CONFIG_FIXED_PHY the issue should be gone. But that's not really a solution, we need to check further.

Quoted 54 lines

[    2.273026] =======phy_device_create===========
[    2.277908] phy_device_create: dev->irq = 17
[    2.277908]
[    2.373104] =======phy_probe===========
[    2.377336] phy_probe: phydev->irq = 17
[    2.381445] phy_drv_supports_irq: phydrv->config_intr = 0,
phydrv->ack_interrupt = 0
[    2.389554] phydev->irq = PHY_POLL;
[    2.393186] phy_probe: phydev->irq = -1
[    2.393186]
[    2.398987] =======phy_probe===========
[    2.403108] phy_probe: phydev->irq = -1
[    2.407195] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.415314] phy_probe: phydev->irq = -1
[    2.415314]
[    2.421189] libphy: bcm63xx_enet MII bus: probed
[    2.426129] =======phy_connect===========
[    2.430410] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.438537] phy_connect: phy_drv_supports_irq = 1
[    2.438537]
[    2.445284] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
I also added the prints to phy_connect.
Quoted 4 lines
Last but not least: Do you use a mainline kernel, or is it maybe a modified downstream kernel? In the latter case, please check in your kernel sources whether both callbacks are set.
It's a modified kernel, and the the callbacks are set. BTW I also tested the kernel with no patches concerning to the ethernet driver.
Regards,
Daniel
Quoted 15 lines
Quoted 14 lines
Quoted 4 lines
On which kernel version do you face this problem?
The kernel version 4.4 works ok. The minimum version where I found the problem were the kernel 4.9.111, now using 5.4. And 5.10 also tested.
Regards
Daniel
Quoted 4 lines
Quoted 3 lines
Regards
Quoted 2 lines
--
Florian

Florian Fainelli

2021-02-26 04:13 UTC #9 lore raw

Quoted 56 lines · Heiner Kallweit

Quoted 56 lines
Quoted 55 lines
Quoted 24 lines
Quoted 6 lines
It's still an ugly workaround and a proper root cause analysis should be done first. I can only imagine that phydev->irq is overwritten in phy_probe() because phy_drv_supports_irq() is false. Can you please check whether phydev->irq is properly set in phy_device_create(), and if yes, whether it's reset to PHY_POLL in phy_probe()?.
Hi Heiner, I added some kernel prints:
[    2.712519] libphy: Fixed MDIO Bus: probed
[    2.721969] =======phy_device_create===========
[    2.726841] phy_device_create: dev->irq = 17
[    2.726841]
[    2.832620] =======phy_probe===========
[    2.836846] phy_probe: phydev->irq = 17
[    2.840950] phy_probe: phy_drv_supports_irq = 0, phy_interrupt_is_valid = 1
[    2.848267] phy_probe: phydev->irq = -1
[    2.848267]
[    2.854059] =======phy_probe===========
[    2.858174] phy_probe: phydev->irq = -1
[    2.862253] phy_probe: phydev->irq = -1
[    2.862253]
[    2.868121] libphy: bcm63xx_enet MII bus: probed
[    2.873320] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
Quoted 5 lines
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
Currently using kernel 5.4.99. I still have no idea what's going on.
Thanks for debugging. This confirms my assumption that the interrupt is overwritten in phy_probe(). I'm just scratching my head how phy_drv_supports_irq() can return 0. In 5.4.99 it's defined as:
static bool phy_drv_supports_irq(struct phy_driver *phydrv)
{
        return phydrv->config_intr && phydrv->ack_interrupt;
}
And that's the PHY driver:
static struct phy_driver bcm63xx_driver[] = {
{
        .phy_id         = 0x00406000,
        .phy_id_mask    = 0xfffffc00,
        .name           = "Broadcom BCM63XX (1)",
/* PHY_BASIC_FEATURES */
.flags = PHY_IS_INTERNAL,
.config_init = bcm63xx_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm63xx_config_intr,
}
So both callbacks are set. Can you extend your debugging and check in phy_drv_supports_irq() which of the callbacks is missing?

Quoted 7 lines

Quoted 4 lines
Hi, both callbacks are missing on the first check. However on the next calls they're there.
[    2.263909] libphy: Fixed MDIO Bus: probed
This is weird. The phy_device seems to show up on both MDIO buses, the fixed one *and* the bcm63xx_enet bus.

Yes that does not make sense to me at all, but maybe something broke at some point for non-Device Tree systems and we are just catching it now. The largest rework that occurred during v4.4 and v4.9 was the introduction of mdio_device.

Signature · 1 line

Florian

Heiner Kallweit

2021-02-26 07:15 UTC #10 lore raw

In reply to Daniel González Cabanelas

Quoted 160 lines · Daniel González Cabanelas

Quoted 127 lines · Heiner Kallweit
Quoted 97 lines · Daniel González Cabanelas
Quoted 71 lines · Heiner Kallweit
Quoted 59 lines · Daniel González Cabanelas
Quoted 57 lines · Florian Fainelli
Quoted 52 lines · Heiner Kallweit
Quoted 38 lines · Daniel González Cabanelas
The current bcm63xx_enet driver doesn't asign the internal phy IRQ. As a result of this it works in polling mode.
Fix it using the phy_device structure to assign the platform IRQ.
Tested under a BCM6348 board. Kernel dmesg before the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
After the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=17)
Pluging and uplugging the ethernet cable now generates interrupts and the PHY goes up and down as expected.
Signed-off-by Daniel González Cabanelas <[email protected]>
---
changes in V2:
- snippet moved after the mdiobus registration
- added missing brackets
 drivers/net/ethernet/broadcom/bcm63xx_enet.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/net/ethernet/broadcom/bcm63xx_enet.c +4 -1
index fd876721316..dd218722560 100644
@@ -1818,10 +1818,19 @@ static int bcm_enet_probe(struct platform_device *pdev)
              * if a slave is not present on hw */
             bus->phy_mask = ~(1 << priv->phy_id);
 
-            if (priv->has_phy_interrupt)
+            ret = mdiobus_register(bus);
+
+            if (priv->has_phy_interrupt) {
+                    phydev = mdiobus_get_phy(bus, priv->phy_id);
drivers/net/ethernet/broadcom/bcm63xx_enet.c static int bcm_enet_probe(struct platform_device *pdev) · line 1824
Quoted 10 lines
drivers/net/ethernet/broadcom/bcm63xx_enet.c +7 -1
+                    if (!phydev) {
+                            dev_err(&dev->dev, "no PHY found\n");
+                            goto out_unregister_mdio;
+                    }
+
                     bus->irq[priv->phy_id] = priv->phy_interrupt;
+                    phydev->irq = priv->phy_interrupt;
+            }
 
-            ret = mdiobus_register(bus);
You shouldn't have to set phydev->irq, this is done by phy_device_create(). For this to work bus->irq[] needs to be set before calling mdiobus_register().
Quoted 9 lines
Quoted 2 lines
Yes good point, and that is what the unchanged code does actually. Daniel, any idea why that is not working?
Hi Florian, I don't know. bus->irq[] has no effect, only assigning the IRQ through phydev->irq works.
I can resend the patch without the bus->irq[] line since it's pointless in this scenario.
Quoted 6 lines
It's still an ugly workaround and a proper root cause analysis should be done first. I can only imagine that phydev->irq is overwritten in phy_probe() because phy_drv_supports_irq() is false. Can you please check whether phydev->irq is properly set in phy_device_create(), and if yes, whether it's reset to PHY_POLL in phy_probe()?.
Hi Heiner, I added some kernel prints:
[    2.712519] libphy: Fixed MDIO Bus: probed
[    2.721969] =======phy_device_create===========
[    2.726841] phy_device_create: dev->irq = 17
[    2.726841]
[    2.832620] =======phy_probe===========
[    2.836846] phy_probe: phydev->irq = 17
[    2.840950] phy_probe: phy_drv_supports_irq = 0, phy_interrupt_is_valid = 1
[    2.848267] phy_probe: phydev->irq = -1
[    2.848267]
[    2.854059] =======phy_probe===========
[    2.858174] phy_probe: phydev->irq = -1
[    2.862253] phy_probe: phydev->irq = -1
[    2.862253]
[    2.868121] libphy: bcm63xx_enet MII bus: probed
[    2.873320] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
Quoted 5 lines
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
Currently using kernel 5.4.99. I still have no idea what's going on.
Thanks for debugging. This confirms my assumption that the interrupt is overwritten in phy_probe(). I'm just scratching my head how phy_drv_supports_irq() can return 0. In 5.4.99 it's defined as:
static bool phy_drv_supports_irq(struct phy_driver *phydrv)
{
        return phydrv->config_intr && phydrv->ack_interrupt;
}
And that's the PHY driver:
static struct phy_driver bcm63xx_driver[] = {
{
        .phy_id         = 0x00406000,
        .phy_id_mask    = 0xfffffc00,
        .name           = "Broadcom BCM63XX (1)",
/* PHY_BASIC_FEATURES */
.flags = PHY_IS_INTERNAL,
.config_init = bcm63xx_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm63xx_config_intr,
}
Quoted 3 lines
So both callbacks are set. Can you extend your debugging and check in phy_drv_supports_irq() which of the callbacks is missing?
Hi, both callbacks are missing on the first check. However on the next calls they're there.
[    2.263909] libphy: Fixed MDIO Bus: probed
[    2.273026] =======phy_device_create===========
[    2.277908] phy_device_create: dev->irq = 17
[    2.277908]
[    2.373104] =======phy_probe===========
[    2.377336] phy_probe: phydev->irq = 17
[    2.381445] phy_drv_supports_irq: phydrv->config_intr = 0,
phydrv->ack_interrupt = 0
[    2.389554] phydev->irq = PHY_POLL;
[    2.393186] phy_probe: phydev->irq = -1
[    2.393186]
[    2.398987] =======phy_probe===========
[    2.403108] phy_probe: phydev->irq = -1
[    2.407195] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.415314] phy_probe: phydev->irq = -1
[    2.415314]
[    2.421189] libphy: bcm63xx_enet MII bus: probed
[    2.426129] =======phy_connect===========
[    2.430410] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.438537] phy_connect: phy_drv_supports_irq = 1
[    2.438537]
[    2.445284] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY

Quoted 3 lines

driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)

I'd like to understand why the phy_device is probed twice, with which drivers it's probed.
Could you please add printing phydrv->name to phy_probe() ?

Quoted 28 lines

I also added the prints to phy_connect.
Quoted 4 lines
Last but not least: Do you use a mainline kernel, or is it maybe a modified downstream kernel? In the latter case, please check in your kernel sources whether both callbacks are set.
It's a modified kernel, and the the callbacks are set. BTW I also tested the kernel with no patches concerning to the ethernet driver.
Regards,
Daniel
Quoted 15 lines
Quoted 14 lines
Quoted 4 lines
On which kernel version do you face this problem?
The kernel version 4.4 works ok. The minimum version where I found the problem were the kernel 4.9.111, now using 5.4. And 5.10 also tested.
Regards
Daniel
Quoted 4 lines
Quoted 3 lines
Regards
Quoted 2 lines
--
Florian

Daniel González Cabanelas

author

2021-02-26 09:12 UTC #11 lore raw

Quoted 166 lines · Heiner Kallweit

Quoted 160 lines · Daniel González Cabanelas
Quoted 127 lines · Heiner Kallweit
Quoted 97 lines · Daniel González Cabanelas
Quoted 71 lines · Heiner Kallweit
Quoted 59 lines · Daniel González Cabanelas
Quoted 57 lines · Florian Fainelli
Quoted 52 lines · Heiner Kallweit
Quoted 38 lines · Daniel González Cabanelas
The current bcm63xx_enet driver doesn't asign the internal phy IRQ. As a result of this it works in polling mode.
Fix it using the phy_device structure to assign the platform IRQ.
Tested under a BCM6348 board. Kernel dmesg before the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
After the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=17)
Pluging and uplugging the ethernet cable now generates interrupts and the PHY goes up and down as expected.
Signed-off-by Daniel González Cabanelas <[email protected]>
---
changes in V2:
- snippet moved after the mdiobus registration
- added missing brackets
 drivers/net/ethernet/broadcom/bcm63xx_enet.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/net/ethernet/broadcom/bcm63xx_enet.c +4 -1
index fd876721316..dd218722560 100644
@@ -1818,10 +1818,19 @@ static int bcm_enet_probe(struct platform_device *pdev)
              * if a slave is not present on hw */
             bus->phy_mask = ~(1 << priv->phy_id);
 
-            if (priv->has_phy_interrupt)
+            ret = mdiobus_register(bus);
+
+            if (priv->has_phy_interrupt) {
+                    phydev = mdiobus_get_phy(bus, priv->phy_id);
drivers/net/ethernet/broadcom/bcm63xx_enet.c static int bcm_enet_probe(struct platform_device *pdev) · line 1824
Quoted 10 lines
drivers/net/ethernet/broadcom/bcm63xx_enet.c +7 -1
+                    if (!phydev) {
+                            dev_err(&dev->dev, "no PHY found\n");
+                            goto out_unregister_mdio;
+                    }
+
                     bus->irq[priv->phy_id] = priv->phy_interrupt;
+                    phydev->irq = priv->phy_interrupt;
+            }
 
-            ret = mdiobus_register(bus);
You shouldn't have to set phydev->irq, this is done by phy_device_create(). For this to work bus->irq[] needs to be set before calling mdiobus_register().
Quoted 9 lines
Quoted 2 lines
Yes good point, and that is what the unchanged code does actually. Daniel, any idea why that is not working?
Hi Florian, I don't know. bus->irq[] has no effect, only assigning the IRQ through phydev->irq works.
I can resend the patch without the bus->irq[] line since it's pointless in this scenario.
Quoted 6 lines
It's still an ugly workaround and a proper root cause analysis should be done first. I can only imagine that phydev->irq is overwritten in phy_probe() because phy_drv_supports_irq() is false. Can you please check whether phydev->irq is properly set in phy_device_create(), and if yes, whether it's reset to PHY_POLL in phy_probe()?.
Hi Heiner, I added some kernel prints:
[    2.712519] libphy: Fixed MDIO Bus: probed
[    2.721969] =======phy_device_create===========
[    2.726841] phy_device_create: dev->irq = 17
[    2.726841]
[    2.832620] =======phy_probe===========
[    2.836846] phy_probe: phydev->irq = 17
[    2.840950] phy_probe: phy_drv_supports_irq = 0, phy_interrupt_is_valid = 1
[    2.848267] phy_probe: phydev->irq = -1
[    2.848267]
[    2.854059] =======phy_probe===========
[    2.858174] phy_probe: phydev->irq = -1
[    2.862253] phy_probe: phydev->irq = -1
[    2.862253]
[    2.868121] libphy: bcm63xx_enet MII bus: probed
[    2.873320] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
Quoted 5 lines
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
Currently using kernel 5.4.99. I still have no idea what's going on.
Thanks for debugging. This confirms my assumption that the interrupt is overwritten in phy_probe(). I'm just scratching my head how phy_drv_supports_irq() can return 0. In 5.4.99 it's defined as:
static bool phy_drv_supports_irq(struct phy_driver *phydrv)
{
        return phydrv->config_intr && phydrv->ack_interrupt;
}
And that's the PHY driver:
static struct phy_driver bcm63xx_driver[] = {
{
        .phy_id         = 0x00406000,
        .phy_id_mask    = 0xfffffc00,
        .name           = "Broadcom BCM63XX (1)",
/* PHY_BASIC_FEATURES */
.flags = PHY_IS_INTERNAL,
.config_init = bcm63xx_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm63xx_config_intr,
}
Quoted 3 lines
So both callbacks are set. Can you extend your debugging and check in phy_drv_supports_irq() which of the callbacks is missing?
Hi, both callbacks are missing on the first check. However on the next calls they're there.
[    2.263909] libphy: Fixed MDIO Bus: probed
[    2.273026] =======phy_device_create===========
[    2.277908] phy_device_create: dev->irq = 17
[    2.277908]
[    2.373104] =======phy_probe===========
[    2.377336] phy_probe: phydev->irq = 17
[    2.381445] phy_drv_supports_irq: phydrv->config_intr = 0,
phydrv->ack_interrupt = 0
[    2.389554] phydev->irq = PHY_POLL;
[    2.393186] phy_probe: phydev->irq = -1
[    2.393186]
[    2.398987] =======phy_probe===========
[    2.403108] phy_probe: phydev->irq = -1
[    2.407195] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.415314] phy_probe: phydev->irq = -1
[    2.415314]
[    2.421189] libphy: bcm63xx_enet MII bus: probed
[    2.426129] =======phy_connect===========
[    2.430410] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.438537] phy_connect: phy_drv_supports_irq = 1
[    2.438537]
[    2.445284] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
Quoted 3 lines
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)

Quoted 4 lines

I'd like to understand why the phy_device is probed twice, with which drivers it's probed.
Could you please add printing phydrv->name to phy_probe() ?

Hi Heiner, indeed there are two different probed devices. The B53 switch driver is causing this issue.

[    2.269595] libphy: Fixed MDIO Bus: probed
[    2.278706] =======phy_device_create===========
[    2.283594] phy_device_create: dev->irq = 17
[    2.283594]
[    2.379554] =======phy_probe===========
[    2.383780] phy_probe: phydrv->name = Broadcom B53 (3)
[    2.389235] phy_probe: phydev->irq = 17
[    2.393332] phy_drv_supports_irq: phydrv->config_intr = 0,
phydrv->ack_interrupt = 0
[    2.401445] phydev->irq = PHY_POLL
[    2.405080] phy_probe: phydev->irq = -1
[    2.405080]
[    2.410878] =======phy_probe===========
[    2.414996] phy_probe: phydrv->name = Broadcom BCM63XX (1)
[    2.420791] phy_probe: phydev->irq = -1
[    2.424876] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.432994] phy_probe: phydev->irq = -1
[    2.432994]
[    2.438862] libphy: bcm63xx_enet MII bus: probed
[    2.443809] =======phy_connect===========
[    2.448092] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.456215] phy_connect: phy_drv_supports_irq = 1
[    2.456215]
[    2.462961] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY

driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)

The board has no switch, it's a driver for other boards in OpenWrt. I forgot it wasn't upstreamed:
https://github.com/openwrt/openwrt/tree/master/target/linux/generic/files/drivers/net/phy/b53

I tested a kernel compiled without this driver, now the IRQ is detected as it should be:

[    2.270707] libphy: Fixed MDIO Bus: probed
[    2.279715] =======phy_device_create===========
[    2.284600] phy_device_create: dev->irq = 17
[    2.284600]
[    2.373763] =======phy_probe===========
[    2.377989] phy_probe: phydrv->name = Broadcom BCM63XX (1)
[    2.383803] phy_probe: phydev->irq = 17
[    2.387888] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.396007] phy_probe: phydev->irq = 17
[    2.396007]
[    2.401877] libphy: bcm63xx_enet MII bus: probed
[    2.406820] =======phy_connect===========
[    2.411099] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.419226] phy_connect: phy_drv_supports_irq = 1
[    2.419226]
[    2.429857] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY

driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=17)

Then, maybe this is an OpenWrt bug itself?

Regards
Daniel

Quoted 30 lines

Quoted 29 lines
I also added the prints to phy_connect.
Quoted 4 lines
Last but not least: Do you use a mainline kernel, or is it maybe a modified downstream kernel? In the latter case, please check in your kernel sources whether both callbacks are set.
It's a modified kernel, and the the callbacks are set. BTW I also tested the kernel with no patches concerning to the ethernet driver.
Regards,
Daniel
Quoted 15 lines
Quoted 14 lines
Quoted 4 lines
On which kernel version do you face this problem?
The kernel version 4.4 works ok. The minimum version where I found the problem were the kernel 4.9.111, now using 5.4. And 5.10 also tested.
Regards
Daniel
Quoted 4 lines
Quoted 3 lines
Regards
Quoted 2 lines
--
Florian

Heiner Kallweit

2021-02-26 09:34 UTC #12 lore raw

Quoted 176 lines · Daniel González Cabanelas

Quoted 166 lines · Heiner Kallweit
Quoted 160 lines · Daniel González Cabanelas
Quoted 127 lines · Heiner Kallweit
Quoted 97 lines · Daniel González Cabanelas
Quoted 71 lines · Heiner Kallweit
Quoted 59 lines · Daniel González Cabanelas
Quoted 57 lines · Florian Fainelli
Quoted 52 lines · Heiner Kallweit
Quoted 38 lines · Daniel González Cabanelas
The current bcm63xx_enet driver doesn't asign the internal phy IRQ. As a result of this it works in polling mode.
Fix it using the phy_device structure to assign the platform IRQ.
Tested under a BCM6348 board. Kernel dmesg before the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
After the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=17)
Pluging and uplugging the ethernet cable now generates interrupts and the PHY goes up and down as expected.
Signed-off-by Daniel González Cabanelas <[email protected]>
---
changes in V2:
- snippet moved after the mdiobus registration
- added missing brackets
 drivers/net/ethernet/broadcom/bcm63xx_enet.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/net/ethernet/broadcom/bcm63xx_enet.c +4 -1
index fd876721316..dd218722560 100644
@@ -1818,10 +1818,19 @@ static int bcm_enet_probe(struct platform_device *pdev)
              * if a slave is not present on hw */
             bus->phy_mask = ~(1 << priv->phy_id);
 
-            if (priv->has_phy_interrupt)
+            ret = mdiobus_register(bus);
+
+            if (priv->has_phy_interrupt) {
+                    phydev = mdiobus_get_phy(bus, priv->phy_id);
drivers/net/ethernet/broadcom/bcm63xx_enet.c static int bcm_enet_probe(struct platform_device *pdev) · line 1824
Quoted 10 lines
drivers/net/ethernet/broadcom/bcm63xx_enet.c +7 -1
+                    if (!phydev) {
+                            dev_err(&dev->dev, "no PHY found\n");
+                            goto out_unregister_mdio;
+                    }
+
                     bus->irq[priv->phy_id] = priv->phy_interrupt;
+                    phydev->irq = priv->phy_interrupt;
+            }
 
-            ret = mdiobus_register(bus);
You shouldn't have to set phydev->irq, this is done by phy_device_create(). For this to work bus->irq[] needs to be set before calling mdiobus_register().
Quoted 9 lines
Quoted 2 lines
Yes good point, and that is what the unchanged code does actually. Daniel, any idea why that is not working?
Hi Florian, I don't know. bus->irq[] has no effect, only assigning the IRQ through phydev->irq works.
I can resend the patch without the bus->irq[] line since it's pointless in this scenario.
Quoted 6 lines
It's still an ugly workaround and a proper root cause analysis should be done first. I can only imagine that phydev->irq is overwritten in phy_probe() because phy_drv_supports_irq() is false. Can you please check whether phydev->irq is properly set in phy_device_create(), and if yes, whether it's reset to PHY_POLL in phy_probe()?.
Hi Heiner, I added some kernel prints:
[    2.712519] libphy: Fixed MDIO Bus: probed
[    2.721969] =======phy_device_create===========
[    2.726841] phy_device_create: dev->irq = 17
[    2.726841]
[    2.832620] =======phy_probe===========
[    2.836846] phy_probe: phydev->irq = 17
[    2.840950] phy_probe: phy_drv_supports_irq = 0, phy_interrupt_is_valid = 1
[    2.848267] phy_probe: phydev->irq = -1
[    2.848267]
[    2.854059] =======phy_probe===========
[    2.858174] phy_probe: phydev->irq = -1
[    2.862253] phy_probe: phydev->irq = -1
[    2.862253]
[    2.868121] libphy: bcm63xx_enet MII bus: probed
[    2.873320] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
Quoted 5 lines
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
Currently using kernel 5.4.99. I still have no idea what's going on.
Thanks for debugging. This confirms my assumption that the interrupt is overwritten in phy_probe(). I'm just scratching my head how phy_drv_supports_irq() can return 0. In 5.4.99 it's defined as:
static bool phy_drv_supports_irq(struct phy_driver *phydrv)
{
        return phydrv->config_intr && phydrv->ack_interrupt;
}
And that's the PHY driver:
static struct phy_driver bcm63xx_driver[] = {
{
        .phy_id         = 0x00406000,
        .phy_id_mask    = 0xfffffc00,
        .name           = "Broadcom BCM63XX (1)",
/* PHY_BASIC_FEATURES */
.flags = PHY_IS_INTERNAL,
.config_init = bcm63xx_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm63xx_config_intr,
}
Quoted 3 lines
So both callbacks are set. Can you extend your debugging and check in phy_drv_supports_irq() which of the callbacks is missing?
Hi, both callbacks are missing on the first check. However on the next calls they're there.
[    2.263909] libphy: Fixed MDIO Bus: probed
[    2.273026] =======phy_device_create===========
[    2.277908] phy_device_create: dev->irq = 17
[    2.277908]
[    2.373104] =======phy_probe===========
[    2.377336] phy_probe: phydev->irq = 17
[    2.381445] phy_drv_supports_irq: phydrv->config_intr = 0,
phydrv->ack_interrupt = 0
[    2.389554] phydev->irq = PHY_POLL;
[    2.393186] phy_probe: phydev->irq = -1
[    2.393186]
[    2.398987] =======phy_probe===========
[    2.403108] phy_probe: phydev->irq = -1
[    2.407195] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.415314] phy_probe: phydev->irq = -1
[    2.415314]
[    2.421189] libphy: bcm63xx_enet MII bus: probed
[    2.426129] =======phy_connect===========
[    2.430410] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.438537] phy_connect: phy_drv_supports_irq = 1
[    2.438537]
[    2.445284] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
Quoted 3 lines
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
Quoted 4 lines
I'd like to understand why the phy_device is probed twice, with which drivers it's probed.
Could you please add printing phydrv->name to phy_probe() ?
Hi Heiner, indeed there are two different probed devices. The B53 switch driver is causing this issue.

Quoted 6 lines

[    2.269595] libphy: Fixed MDIO Bus: probed
[    2.278706] =======phy_device_create===========
[    2.283594] phy_device_create: dev->irq = 17
[    2.283594]
[    2.379554] =======phy_probe===========
[    2.383780] phy_probe: phydrv->name = Broadcom B53 (3)

Is this an out-of-tree driver? I can't find this string in any DSA or PHY driver.

Quoted 86 lines

[    2.389235] phy_probe: phydev->irq = 17
[    2.393332] phy_drv_supports_irq: phydrv->config_intr = 0,
phydrv->ack_interrupt = 0
[    2.401445] phydev->irq = PHY_POLL
[    2.405080] phy_probe: phydev->irq = -1
[    2.405080]
[    2.410878] =======phy_probe===========
[    2.414996] phy_probe: phydrv->name = Broadcom BCM63XX (1)
[    2.420791] phy_probe: phydev->irq = -1
[    2.424876] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.432994] phy_probe: phydev->irq = -1
[    2.432994]
[    2.438862] libphy: bcm63xx_enet MII bus: probed
[    2.443809] =======phy_connect===========
[    2.448092] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.456215] phy_connect: phy_drv_supports_irq = 1
[    2.456215]
[    2.462961] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
The board has no switch, it's a driver for other boards in OpenWrt. I forgot it wasn't upstreamed:
https://github.com/openwrt/openwrt/tree/master/target/linux/generic/files/drivers/net/phy/b53
I tested a kernel compiled without this driver, now the IRQ is detected as it should be:
[    2.270707] libphy: Fixed MDIO Bus: probed
[    2.279715] =======phy_device_create===========
[    2.284600] phy_device_create: dev->irq = 17
[    2.284600]
[    2.373763] =======phy_probe===========
[    2.377989] phy_probe: phydrv->name = Broadcom BCM63XX (1)
[    2.383803] phy_probe: phydev->irq = 17
[    2.387888] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.396007] phy_probe: phydev->irq = 17
[    2.396007]
[    2.401877] libphy: bcm63xx_enet MII bus: probed
[    2.406820] =======phy_connect===========
[    2.411099] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.419226] phy_connect: phy_drv_supports_irq = 1
[    2.419226]
[    2.429857] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=17)
Then, maybe this is an OpenWrt bug itself?
Regards
Daniel
Quoted 30 lines
Quoted 29 lines
I also added the prints to phy_connect.
Quoted 4 lines
Last but not least: Do you use a mainline kernel, or is it maybe a modified downstream kernel? In the latter case, please check in your kernel sources whether both callbacks are set.
It's a modified kernel, and the the callbacks are set. BTW I also tested the kernel with no patches concerning to the ethernet driver.
Regards,
Daniel
Quoted 15 lines
Quoted 14 lines
Quoted 4 lines
On which kernel version do you face this problem?
The kernel version 4.4 works ok. The minimum version where I found the problem were the kernel 4.9.111, now using 5.4. And 5.10 also tested.
Regards
Daniel
Quoted 4 lines
Quoted 3 lines
Regards
Quoted 2 lines
--
Florian

Heiner Kallweit

2021-02-26 09:40 UTC #13 lore raw

Quoted 184 lines · Heiner Kallweit

Quoted 176 lines · Daniel González Cabanelas
Quoted 166 lines · Heiner Kallweit
Quoted 160 lines · Daniel González Cabanelas
Quoted 127 lines · Heiner Kallweit
Quoted 97 lines · Daniel González Cabanelas
Quoted 71 lines · Heiner Kallweit
Quoted 59 lines · Daniel González Cabanelas
Quoted 57 lines · Florian Fainelli
Quoted 52 lines · Heiner Kallweit
Quoted 38 lines · Daniel González Cabanelas
The current bcm63xx_enet driver doesn't asign the internal phy IRQ. As a result of this it works in polling mode.
Fix it using the phy_device structure to assign the platform IRQ.
Tested under a BCM6348 board. Kernel dmesg before the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
After the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=17)
Pluging and uplugging the ethernet cable now generates interrupts and the PHY goes up and down as expected.
Signed-off-by Daniel González Cabanelas <[email protected]>
---
changes in V2:
- snippet moved after the mdiobus registration
- added missing brackets
 drivers/net/ethernet/broadcom/bcm63xx_enet.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/net/ethernet/broadcom/bcm63xx_enet.c +4 -1
index fd876721316..dd218722560 100644
@@ -1818,10 +1818,19 @@ static int bcm_enet_probe(struct platform_device *pdev)
              * if a slave is not present on hw */
             bus->phy_mask = ~(1 << priv->phy_id);
 
-            if (priv->has_phy_interrupt)
+            ret = mdiobus_register(bus);
+
+            if (priv->has_phy_interrupt) {
+                    phydev = mdiobus_get_phy(bus, priv->phy_id);
drivers/net/ethernet/broadcom/bcm63xx_enet.c static int bcm_enet_probe(struct platform_device *pdev) · line 1824
Quoted 10 lines
drivers/net/ethernet/broadcom/bcm63xx_enet.c +7 -1
+                    if (!phydev) {
+                            dev_err(&dev->dev, "no PHY found\n");
+                            goto out_unregister_mdio;
+                    }
+
                     bus->irq[priv->phy_id] = priv->phy_interrupt;
+                    phydev->irq = priv->phy_interrupt;
+            }
 
-            ret = mdiobus_register(bus);
You shouldn't have to set phydev->irq, this is done by phy_device_create(). For this to work bus->irq[] needs to be set before calling mdiobus_register().
Quoted 9 lines
Quoted 2 lines
Yes good point, and that is what the unchanged code does actually. Daniel, any idea why that is not working?
Hi Florian, I don't know. bus->irq[] has no effect, only assigning the IRQ through phydev->irq works.
I can resend the patch without the bus->irq[] line since it's pointless in this scenario.
Quoted 6 lines
It's still an ugly workaround and a proper root cause analysis should be done first. I can only imagine that phydev->irq is overwritten in phy_probe() because phy_drv_supports_irq() is false. Can you please check whether phydev->irq is properly set in phy_device_create(), and if yes, whether it's reset to PHY_POLL in phy_probe()?.
Hi Heiner, I added some kernel prints:
[    2.712519] libphy: Fixed MDIO Bus: probed
[    2.721969] =======phy_device_create===========
[    2.726841] phy_device_create: dev->irq = 17
[    2.726841]
[    2.832620] =======phy_probe===========
[    2.836846] phy_probe: phydev->irq = 17
[    2.840950] phy_probe: phy_drv_supports_irq = 0, phy_interrupt_is_valid = 1
[    2.848267] phy_probe: phydev->irq = -1
[    2.848267]
[    2.854059] =======phy_probe===========
[    2.858174] phy_probe: phydev->irq = -1
[    2.862253] phy_probe: phydev->irq = -1
[    2.862253]
[    2.868121] libphy: bcm63xx_enet MII bus: probed
[    2.873320] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
Quoted 5 lines
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
Currently using kernel 5.4.99. I still have no idea what's going on.
Thanks for debugging. This confirms my assumption that the interrupt is overwritten in phy_probe(). I'm just scratching my head how phy_drv_supports_irq() can return 0. In 5.4.99 it's defined as:
static bool phy_drv_supports_irq(struct phy_driver *phydrv)
{
        return phydrv->config_intr && phydrv->ack_interrupt;
}
And that's the PHY driver:
static struct phy_driver bcm63xx_driver[] = {
{
        .phy_id         = 0x00406000,
        .phy_id_mask    = 0xfffffc00,
        .name           = "Broadcom BCM63XX (1)",
/* PHY_BASIC_FEATURES */
.flags = PHY_IS_INTERNAL,
.config_init = bcm63xx_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm63xx_config_intr,
}
Quoted 3 lines
So both callbacks are set. Can you extend your debugging and check in phy_drv_supports_irq() which of the callbacks is missing?
Hi, both callbacks are missing on the first check. However on the next calls they're there.
[    2.263909] libphy: Fixed MDIO Bus: probed
[    2.273026] =======phy_device_create===========
[    2.277908] phy_device_create: dev->irq = 17
[    2.277908]
[    2.373104] =======phy_probe===========
[    2.377336] phy_probe: phydev->irq = 17
[    2.381445] phy_drv_supports_irq: phydrv->config_intr = 0,
phydrv->ack_interrupt = 0
[    2.389554] phydev->irq = PHY_POLL;
[    2.393186] phy_probe: phydev->irq = -1
[    2.393186]
[    2.398987] =======phy_probe===========
[    2.403108] phy_probe: phydev->irq = -1
[    2.407195] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.415314] phy_probe: phydev->irq = -1
[    2.415314]
[    2.421189] libphy: bcm63xx_enet MII bus: probed
[    2.426129] =======phy_connect===========
[    2.430410] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.438537] phy_connect: phy_drv_supports_irq = 1
[    2.438537]
[    2.445284] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
Quoted 3 lines
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
Quoted 4 lines
I'd like to understand why the phy_device is probed twice, with which drivers it's probed.
Could you please add printing phydrv->name to phy_probe() ?
Hi Heiner, indeed there are two different probed devices. The B53 switch driver is causing this issue.
Quoted 6 lines
[    2.269595] libphy: Fixed MDIO Bus: probed
[    2.278706] =======phy_device_create===========
[    2.283594] phy_device_create: dev->irq = 17
[    2.283594]
[    2.379554] =======phy_probe===========
[    2.383780] phy_probe: phydrv->name = Broadcom B53 (3)

Quoted 3 lines

Is this an out-of-tree driver? I can't find this string in any DSA or PHY driver.

I found this PHY driver name in an older patch set that obviously didn't make it into mainline:
https://patchwork.ozlabs.org/project/netdev/patch/[email protected]/

Any yes, if you use this patch set then you have conflicting PHY drivers. How about using the in-tree B53 DSA driver?

Quoted 88 lines

Quoted 87 lines
[    2.389235] phy_probe: phydev->irq = 17
[    2.393332] phy_drv_supports_irq: phydrv->config_intr = 0,
phydrv->ack_interrupt = 0
[    2.401445] phydev->irq = PHY_POLL
[    2.405080] phy_probe: phydev->irq = -1
[    2.405080]
[    2.410878] =======phy_probe===========
[    2.414996] phy_probe: phydrv->name = Broadcom BCM63XX (1)
[    2.420791] phy_probe: phydev->irq = -1
[    2.424876] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.432994] phy_probe: phydev->irq = -1
[    2.432994]
[    2.438862] libphy: bcm63xx_enet MII bus: probed
[    2.443809] =======phy_connect===========
[    2.448092] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.456215] phy_connect: phy_drv_supports_irq = 1
[    2.456215]
[    2.462961] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
The board has no switch, it's a driver for other boards in OpenWrt. I forgot it wasn't upstreamed:
https://github.com/openwrt/openwrt/tree/master/target/linux/generic/files/drivers/net/phy/b53
I tested a kernel compiled without this driver, now the IRQ is detected as it should be:
[    2.270707] libphy: Fixed MDIO Bus: probed
[    2.279715] =======phy_device_create===========
[    2.284600] phy_device_create: dev->irq = 17
[    2.284600]
[    2.373763] =======phy_probe===========
[    2.377989] phy_probe: phydrv->name = Broadcom BCM63XX (1)
[    2.383803] phy_probe: phydev->irq = 17
[    2.387888] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.396007] phy_probe: phydev->irq = 17
[    2.396007]
[    2.401877] libphy: bcm63xx_enet MII bus: probed
[    2.406820] =======phy_connect===========
[    2.411099] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.419226] phy_connect: phy_drv_supports_irq = 1
[    2.419226]
[    2.429857] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=17)
Then, maybe this is an OpenWrt bug itself?
Regards
Daniel
Quoted 30 lines
Quoted 29 lines
I also added the prints to phy_connect.
Quoted 4 lines
Last but not least: Do you use a mainline kernel, or is it maybe a modified downstream kernel? In the latter case, please check in your kernel sources whether both callbacks are set.
It's a modified kernel, and the the callbacks are set. BTW I also tested the kernel with no patches concerning to the ethernet driver.
Regards,
Daniel
Quoted 15 lines
Quoted 14 lines
Quoted 4 lines
On which kernel version do you face this problem?
The kernel version 4.4 works ok. The minimum version where I found the problem were the kernel 4.9.111, now using 5.4. And 5.10 also tested.
Regards
Daniel
Quoted 4 lines
Quoted 3 lines
Regards
Quoted 2 lines
--
Florian

Daniel González Cabanelas

author

2021-02-26 09:51 UTC #14 lore raw

In reply to Heiner Kallweit

Quoted 185 lines · Heiner Kallweit

Quoted 176 lines · Daniel González Cabanelas
Quoted 166 lines · Heiner Kallweit
Quoted 160 lines · Daniel González Cabanelas
Quoted 127 lines · Heiner Kallweit
Quoted 97 lines · Daniel González Cabanelas
Quoted 71 lines · Heiner Kallweit
Quoted 59 lines · Daniel González Cabanelas
Quoted 57 lines · Florian Fainelli
Quoted 52 lines · Heiner Kallweit
Quoted 38 lines · Daniel González Cabanelas
The current bcm63xx_enet driver doesn't asign the internal phy IRQ. As a result of this it works in polling mode.
Fix it using the phy_device structure to assign the platform IRQ.
Tested under a BCM6348 board. Kernel dmesg before the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
After the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=17)
Pluging and uplugging the ethernet cable now generates interrupts and the PHY goes up and down as expected.
Signed-off-by Daniel González Cabanelas <[email protected]>
---
changes in V2:
- snippet moved after the mdiobus registration
- added missing brackets
 drivers/net/ethernet/broadcom/bcm63xx_enet.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/net/ethernet/broadcom/bcm63xx_enet.c +4 -1
index fd876721316..dd218722560 100644
@@ -1818,10 +1818,19 @@ static int bcm_enet_probe(struct platform_device *pdev)
              * if a slave is not present on hw */
             bus->phy_mask = ~(1 << priv->phy_id);
 
-            if (priv->has_phy_interrupt)
+            ret = mdiobus_register(bus);
+
+            if (priv->has_phy_interrupt) {
+                    phydev = mdiobus_get_phy(bus, priv->phy_id);
drivers/net/ethernet/broadcom/bcm63xx_enet.c static int bcm_enet_probe(struct platform_device *pdev) · line 1824
Quoted 10 lines
drivers/net/ethernet/broadcom/bcm63xx_enet.c +7 -1
+                    if (!phydev) {
+                            dev_err(&dev->dev, "no PHY found\n");
+                            goto out_unregister_mdio;
+                    }
+
                     bus->irq[priv->phy_id] = priv->phy_interrupt;
+                    phydev->irq = priv->phy_interrupt;
+            }
 
-            ret = mdiobus_register(bus);
You shouldn't have to set phydev->irq, this is done by phy_device_create(). For this to work bus->irq[] needs to be set before calling mdiobus_register().
Quoted 9 lines
Quoted 2 lines
Yes good point, and that is what the unchanged code does actually. Daniel, any idea why that is not working?
Hi Florian, I don't know. bus->irq[] has no effect, only assigning the IRQ through phydev->irq works.
I can resend the patch without the bus->irq[] line since it's pointless in this scenario.
Quoted 6 lines
It's still an ugly workaround and a proper root cause analysis should be done first. I can only imagine that phydev->irq is overwritten in phy_probe() because phy_drv_supports_irq() is false. Can you please check whether phydev->irq is properly set in phy_device_create(), and if yes, whether it's reset to PHY_POLL in phy_probe()?.
Hi Heiner, I added some kernel prints:
[    2.712519] libphy: Fixed MDIO Bus: probed
[    2.721969] =======phy_device_create===========
[    2.726841] phy_device_create: dev->irq = 17
[    2.726841]
[    2.832620] =======phy_probe===========
[    2.836846] phy_probe: phydev->irq = 17
[    2.840950] phy_probe: phy_drv_supports_irq = 0, phy_interrupt_is_valid = 1
[    2.848267] phy_probe: phydev->irq = -1
[    2.848267]
[    2.854059] =======phy_probe===========
[    2.858174] phy_probe: phydev->irq = -1
[    2.862253] phy_probe: phydev->irq = -1
[    2.862253]
[    2.868121] libphy: bcm63xx_enet MII bus: probed
[    2.873320] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
Quoted 5 lines
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
Currently using kernel 5.4.99. I still have no idea what's going on.
Thanks for debugging. This confirms my assumption that the interrupt is overwritten in phy_probe(). I'm just scratching my head how phy_drv_supports_irq() can return 0. In 5.4.99 it's defined as:
static bool phy_drv_supports_irq(struct phy_driver *phydrv)
{
        return phydrv->config_intr && phydrv->ack_interrupt;
}
And that's the PHY driver:
static struct phy_driver bcm63xx_driver[] = {
{
        .phy_id         = 0x00406000,
        .phy_id_mask    = 0xfffffc00,
        .name           = "Broadcom BCM63XX (1)",
/* PHY_BASIC_FEATURES */
.flags = PHY_IS_INTERNAL,
.config_init = bcm63xx_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm63xx_config_intr,
}
Quoted 3 lines
So both callbacks are set. Can you extend your debugging and check in phy_drv_supports_irq() which of the callbacks is missing?
Hi, both callbacks are missing on the first check. However on the next calls they're there.
[    2.263909] libphy: Fixed MDIO Bus: probed
[    2.273026] =======phy_device_create===========
[    2.277908] phy_device_create: dev->irq = 17
[    2.277908]
[    2.373104] =======phy_probe===========
[    2.377336] phy_probe: phydev->irq = 17
[    2.381445] phy_drv_supports_irq: phydrv->config_intr = 0,
phydrv->ack_interrupt = 0
[    2.389554] phydev->irq = PHY_POLL;
[    2.393186] phy_probe: phydev->irq = -1
[    2.393186]
[    2.398987] =======phy_probe===========
[    2.403108] phy_probe: phydev->irq = -1
[    2.407195] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.415314] phy_probe: phydev->irq = -1
[    2.415314]
[    2.421189] libphy: bcm63xx_enet MII bus: probed
[    2.426129] =======phy_connect===========
[    2.430410] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.438537] phy_connect: phy_drv_supports_irq = 1
[    2.438537]
[    2.445284] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
Quoted 3 lines
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
Quoted 4 lines
I'd like to understand why the phy_device is probed twice, with which drivers it's probed.
Could you please add printing phydrv->name to phy_probe() ?
Hi Heiner, indeed there are two different probed devices. The B53 switch driver is causing this issue.
Quoted 6 lines
[    2.269595] libphy: Fixed MDIO Bus: probed
[    2.278706] =======phy_device_create===========
[    2.283594] phy_device_create: dev->irq = 17
[    2.283594]
[    2.379554] =======phy_probe===========
[    2.383780] phy_probe: phydrv->name = Broadcom B53 (3)

Quoted 3 lines

Is this an out-of-tree driver? I can't find this string in any DSA or PHY driver.

Yes it is.
https://github.com/openwrt/openwrt/blob/master/target/linux/generic/files/drivers/net/phy/b53/b53_mdio.c#L421

Quoted 88 lines

Quoted 87 lines
[    2.389235] phy_probe: phydev->irq = 17
[    2.393332] phy_drv_supports_irq: phydrv->config_intr = 0,
phydrv->ack_interrupt = 0
[    2.401445] phydev->irq = PHY_POLL
[    2.405080] phy_probe: phydev->irq = -1
[    2.405080]
[    2.410878] =======phy_probe===========
[    2.414996] phy_probe: phydrv->name = Broadcom BCM63XX (1)
[    2.420791] phy_probe: phydev->irq = -1
[    2.424876] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.432994] phy_probe: phydev->irq = -1
[    2.432994]
[    2.438862] libphy: bcm63xx_enet MII bus: probed
[    2.443809] =======phy_connect===========
[    2.448092] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.456215] phy_connect: phy_drv_supports_irq = 1
[    2.456215]
[    2.462961] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
The board has no switch, it's a driver for other boards in OpenWrt. I forgot it wasn't upstreamed:
https://github.com/openwrt/openwrt/tree/master/target/linux/generic/files/drivers/net/phy/b53
I tested a kernel compiled without this driver, now the IRQ is detected as it should be:
[    2.270707] libphy: Fixed MDIO Bus: probed
[    2.279715] =======phy_device_create===========
[    2.284600] phy_device_create: dev->irq = 17
[    2.284600]
[    2.373763] =======phy_probe===========
[    2.377989] phy_probe: phydrv->name = Broadcom BCM63XX (1)
[    2.383803] phy_probe: phydev->irq = 17
[    2.387888] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.396007] phy_probe: phydev->irq = 17
[    2.396007]
[    2.401877] libphy: bcm63xx_enet MII bus: probed
[    2.406820] =======phy_connect===========
[    2.411099] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.419226] phy_connect: phy_drv_supports_irq = 1
[    2.419226]
[    2.429857] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=17)
Then, maybe this is an OpenWrt bug itself?
Regards
Daniel
Quoted 30 lines
Quoted 29 lines
I also added the prints to phy_connect.
Quoted 4 lines
Last but not least: Do you use a mainline kernel, or is it maybe a modified downstream kernel? In the latter case, please check in your kernel sources whether both callbacks are set.
It's a modified kernel, and the the callbacks are set. BTW I also tested the kernel with no patches concerning to the ethernet driver.
Regards,
Daniel
Quoted 15 lines
Quoted 14 lines
Quoted 4 lines
On which kernel version do you face this problem?
The kernel version 4.4 works ok. The minimum version where I found the problem were the kernel 4.9.111, now using 5.4. And 5.10 also tested.
Regards
Daniel
Quoted 4 lines
Quoted 3 lines
Regards
Quoted 2 lines
--
Florian

Heiner Kallweit

2021-02-26 10:11 UTC #15 lore raw

Quoted 187 lines · Daniel González Cabanelas

Quoted 185 lines · Heiner Kallweit
Quoted 182 lines · Daniel González Cabanelas
Quoted 166 lines · Heiner Kallweit
Quoted 160 lines · Daniel González Cabanelas
Quoted 127 lines · Heiner Kallweit
Quoted 97 lines · Daniel González Cabanelas
Quoted 71 lines · Heiner Kallweit
Quoted 59 lines · Daniel González Cabanelas
Quoted 57 lines · Florian Fainelli
Quoted 52 lines · Heiner Kallweit
Quoted 38 lines · Daniel González Cabanelas
The current bcm63xx_enet driver doesn't asign the internal phy IRQ. As a result of this it works in polling mode.
Fix it using the phy_device structure to assign the platform IRQ.
Tested under a BCM6348 board. Kernel dmesg before the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
After the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=17)
Pluging and uplugging the ethernet cable now generates interrupts and the PHY goes up and down as expected.
Signed-off-by Daniel González Cabanelas <[email protected]>
---
changes in V2:
- snippet moved after the mdiobus registration
- added missing brackets
 drivers/net/ethernet/broadcom/bcm63xx_enet.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/net/ethernet/broadcom/bcm63xx_enet.c +4 -1
index fd876721316..dd218722560 100644
@@ -1818,10 +1818,19 @@ static int bcm_enet_probe(struct platform_device *pdev)
              * if a slave is not present on hw */
             bus->phy_mask = ~(1 << priv->phy_id);
 
-            if (priv->has_phy_interrupt)
+            ret = mdiobus_register(bus);
+
+            if (priv->has_phy_interrupt) {
+                    phydev = mdiobus_get_phy(bus, priv->phy_id);
drivers/net/ethernet/broadcom/bcm63xx_enet.c static int bcm_enet_probe(struct platform_device *pdev) · line 1824
Quoted 10 lines
drivers/net/ethernet/broadcom/bcm63xx_enet.c +7 -1
+                    if (!phydev) {
+                            dev_err(&dev->dev, "no PHY found\n");
+                            goto out_unregister_mdio;
+                    }
+
                     bus->irq[priv->phy_id] = priv->phy_interrupt;
+                    phydev->irq = priv->phy_interrupt;
+            }
 
-            ret = mdiobus_register(bus);
You shouldn't have to set phydev->irq, this is done by phy_device_create(). For this to work bus->irq[] needs to be set before calling mdiobus_register().
Quoted 9 lines
Quoted 2 lines
Yes good point, and that is what the unchanged code does actually. Daniel, any idea why that is not working?
Hi Florian, I don't know. bus->irq[] has no effect, only assigning the IRQ through phydev->irq works.
I can resend the patch without the bus->irq[] line since it's pointless in this scenario.
Quoted 6 lines
It's still an ugly workaround and a proper root cause analysis should be done first. I can only imagine that phydev->irq is overwritten in phy_probe() because phy_drv_supports_irq() is false. Can you please check whether phydev->irq is properly set in phy_device_create(), and if yes, whether it's reset to PHY_POLL in phy_probe()?.
Hi Heiner, I added some kernel prints:
[    2.712519] libphy: Fixed MDIO Bus: probed
[    2.721969] =======phy_device_create===========
[    2.726841] phy_device_create: dev->irq = 17
[    2.726841]
[    2.832620] =======phy_probe===========
[    2.836846] phy_probe: phydev->irq = 17
[    2.840950] phy_probe: phy_drv_supports_irq = 0, phy_interrupt_is_valid = 1
[    2.848267] phy_probe: phydev->irq = -1
[    2.848267]
[    2.854059] =======phy_probe===========
[    2.858174] phy_probe: phydev->irq = -1
[    2.862253] phy_probe: phydev->irq = -1
[    2.862253]
[    2.868121] libphy: bcm63xx_enet MII bus: probed
[    2.873320] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
Quoted 5 lines
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
Currently using kernel 5.4.99. I still have no idea what's going on.
Thanks for debugging. This confirms my assumption that the interrupt is overwritten in phy_probe(). I'm just scratching my head how phy_drv_supports_irq() can return 0. In 5.4.99 it's defined as:
static bool phy_drv_supports_irq(struct phy_driver *phydrv)
{
        return phydrv->config_intr && phydrv->ack_interrupt;
}
And that's the PHY driver:
static struct phy_driver bcm63xx_driver[] = {
{
        .phy_id         = 0x00406000,
        .phy_id_mask    = 0xfffffc00,
        .name           = "Broadcom BCM63XX (1)",
/* PHY_BASIC_FEATURES */
.flags = PHY_IS_INTERNAL,
.config_init = bcm63xx_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm63xx_config_intr,
}
Quoted 3 lines
So both callbacks are set. Can you extend your debugging and check in phy_drv_supports_irq() which of the callbacks is missing?
Hi, both callbacks are missing on the first check. However on the next calls they're there.
[    2.263909] libphy: Fixed MDIO Bus: probed
[    2.273026] =======phy_device_create===========
[    2.277908] phy_device_create: dev->irq = 17
[    2.277908]
[    2.373104] =======phy_probe===========
[    2.377336] phy_probe: phydev->irq = 17
[    2.381445] phy_drv_supports_irq: phydrv->config_intr = 0,
phydrv->ack_interrupt = 0
[    2.389554] phydev->irq = PHY_POLL;
[    2.393186] phy_probe: phydev->irq = -1
[    2.393186]
[    2.398987] =======phy_probe===========
[    2.403108] phy_probe: phydev->irq = -1
[    2.407195] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.415314] phy_probe: phydev->irq = -1
[    2.415314]
[    2.421189] libphy: bcm63xx_enet MII bus: probed
[    2.426129] =======phy_connect===========
[    2.430410] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.438537] phy_connect: phy_drv_supports_irq = 1
[    2.438537]
[    2.445284] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
Quoted 3 lines
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
Quoted 4 lines
I'd like to understand why the phy_device is probed twice, with which drivers it's probed.
Could you please add printing phydrv->name to phy_probe() ?
Hi Heiner, indeed there are two different probed devices. The B53 switch driver is causing this issue.
[    2.269595] libphy: Fixed MDIO Bus: probed
[    2.278706] =======phy_device_create===========
[    2.283594] phy_device_create: dev->irq = 17
[    2.283594]
[    2.379554] =======phy_probe===========
[    2.383780] phy_probe: phydrv->name = Broadcom B53 (3)

Quoted 7 lines

Quoted 3 lines
Is this an out-of-tree driver? I can't find this string in any DSA or PHY driver.
Yes it is.
https://github.com/openwrt/openwrt/blob/master/target/linux/generic/files/drivers/net/phy/b53/b53_mdio.c#L421

OK, I see. Then there's no reason to complain upstream. Either use the mainline B53 DSA driver of fix interrupt mode downstream.

Quoted 88 lines

Quoted 88 lines
Quoted 87 lines
[    2.389235] phy_probe: phydev->irq = 17
[    2.393332] phy_drv_supports_irq: phydrv->config_intr = 0,
phydrv->ack_interrupt = 0
[    2.401445] phydev->irq = PHY_POLL
[    2.405080] phy_probe: phydev->irq = -1
[    2.405080]
[    2.410878] =======phy_probe===========
[    2.414996] phy_probe: phydrv->name = Broadcom BCM63XX (1)
[    2.420791] phy_probe: phydev->irq = -1
[    2.424876] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.432994] phy_probe: phydev->irq = -1
[    2.432994]
[    2.438862] libphy: bcm63xx_enet MII bus: probed
[    2.443809] =======phy_connect===========
[    2.448092] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.456215] phy_connect: phy_drv_supports_irq = 1
[    2.456215]
[    2.462961] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
The board has no switch, it's a driver for other boards in OpenWrt. I forgot it wasn't upstreamed:
https://github.com/openwrt/openwrt/tree/master/target/linux/generic/files/drivers/net/phy/b53
I tested a kernel compiled without this driver, now the IRQ is detected as it should be:
[    2.270707] libphy: Fixed MDIO Bus: probed
[    2.279715] =======phy_device_create===========
[    2.284600] phy_device_create: dev->irq = 17
[    2.284600]
[    2.373763] =======phy_probe===========
[    2.377989] phy_probe: phydrv->name = Broadcom BCM63XX (1)
[    2.383803] phy_probe: phydev->irq = 17
[    2.387888] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.396007] phy_probe: phydev->irq = 17
[    2.396007]
[    2.401877] libphy: bcm63xx_enet MII bus: probed
[    2.406820] =======phy_connect===========
[    2.411099] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.419226] phy_connect: phy_drv_supports_irq = 1
[    2.419226]
[    2.429857] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=17)
Then, maybe this is an OpenWrt bug itself?
Regards
Daniel
Quoted 30 lines
Quoted 29 lines
I also added the prints to phy_connect.
Quoted 4 lines
Last but not least: Do you use a mainline kernel, or is it maybe a modified downstream kernel? In the latter case, please check in your kernel sources whether both callbacks are set.
It's a modified kernel, and the the callbacks are set. BTW I also tested the kernel with no patches concerning to the ethernet driver.
Regards,
Daniel
Quoted 15 lines
Quoted 14 lines
Quoted 4 lines
On which kernel version do you face this problem?
The kernel version 4.4 works ok. The minimum version where I found the problem were the kernel 4.9.111, now using 5.4. And 5.10 also tested.
Regards
Daniel
Quoted 4 lines
Quoted 3 lines
Regards
Quoted 2 lines
--
Florian

Daniel González Cabanelas

author

2021-02-26 10:20 UTC #16 lore raw

Quoted 193 lines · Heiner Kallweit

Quoted 191 lines · Daniel González Cabanelas
Quoted 188 lines · Heiner Kallweit
Quoted 176 lines · Daniel González Cabanelas
Quoted 166 lines · Heiner Kallweit
Quoted 160 lines · Daniel González Cabanelas
Quoted 127 lines · Heiner Kallweit
Quoted 97 lines · Daniel González Cabanelas
Quoted 71 lines · Heiner Kallweit
Quoted 59 lines · Daniel González Cabanelas
Quoted 57 lines · Florian Fainelli
Quoted 52 lines · Heiner Kallweit
Quoted 48 lines · Daniel González Cabanelas
The current bcm63xx_enet driver doesn't asign the internal phy IRQ. As a
result of this it works in polling mode.
Fix it using the phy_device structure to assign the platform IRQ.
Tested under a BCM6348 board. Kernel dmesg before the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
After the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=17)
Pluging and uplugging the ethernet cable now generates interrupts and the
PHY goes up and down as expected.
Signed-off-by: Daniel González Cabanelas <[email protected]>
---
changes in V2:
- snippet moved after the mdiobus registration
- added missing brackets
drivers/net/ethernet/broadcom/bcm63xx_enet.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bcm63xx_enet.c b/drivers/net/ethernet/broadcom/bcm63xx_enet.c
index fd876721316..dd218722560 100644
--- a/drivers/net/ethernet/broadcom/bcm63xx_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm63xx_enet.c
@@ -1818,10 +1818,19 @@ static int bcm_enet_probe(struct platform_device *pdev)
* if a slave is not present on hw */
bus->phy_mask = ~(1 << priv->phy_id);
- if (priv->has_phy_interrupt)
+ ret = mdiobus_register(bus);
+
+ if (priv->has_phy_interrupt) {
+ phydev = mdiobus_get_phy(bus, priv->phy_id);
+ if (!phydev) {
+ dev_err(&dev->dev, "no PHY found\n");
+ goto out_unregister_mdio;
+ }
+
bus->irq[priv->phy_id] = priv->phy_interrupt;
+ phydev->irq = priv->phy_interrupt;
+ }
- ret = mdiobus_register(bus);
You shouldn't have to set phydev->irq, this is done by phy_device_create(). For this to work bus->irq[] needs to be set before calling mdiobus_register().
Quoted 9 lines
Quoted 2 lines
Yes good point, and that is what the unchanged code does actually. Daniel, any idea why that is not working?
Hi Florian, I don't know. bus->irq[] has no effect, only assigning the IRQ through phydev->irq works.
I can resend the patch without the bus->irq[] line since it's pointless in this scenario.
Quoted 6 lines
It's still an ugly workaround and a proper root cause analysis should be done first. I can only imagine that phydev->irq is overwritten in phy_probe() because phy_drv_supports_irq() is false. Can you please check whether phydev->irq is properly set in phy_device_create(), and if yes, whether it's reset to PHY_POLL in phy_probe()?.
Hi Heiner, I added some kernel prints:
[    2.712519] libphy: Fixed MDIO Bus: probed
[    2.721969] =======phy_device_create===========
[    2.726841] phy_device_create: dev->irq = 17
[    2.726841]
[    2.832620] =======phy_probe===========
[    2.836846] phy_probe: phydev->irq = 17
[    2.840950] phy_probe: phy_drv_supports_irq = 0, phy_interrupt_is_valid = 1
[    2.848267] phy_probe: phydev->irq = -1
[    2.848267]
[    2.854059] =======phy_probe===========
[    2.858174] phy_probe: phydev->irq = -1
[    2.862253] phy_probe: phydev->irq = -1
[    2.862253]
[    2.868121] libphy: bcm63xx_enet MII bus: probed
[    2.873320] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
Quoted 5 lines
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
Currently using kernel 5.4.99. I still have no idea what's going on.
Thanks for debugging. This confirms my assumption that the interrupt is overwritten in phy_probe(). I'm just scratching my head how phy_drv_supports_irq() can return 0. In 5.4.99 it's defined as:
static bool phy_drv_supports_irq(struct phy_driver *phydrv)
{
        return phydrv->config_intr && phydrv->ack_interrupt;
}
And that's the PHY driver:
static struct phy_driver bcm63xx_driver[] = {
{
        .phy_id         = 0x00406000,
        .phy_id_mask    = 0xfffffc00,
        .name           = "Broadcom BCM63XX (1)",
/* PHY_BASIC_FEATURES */
.flags = PHY_IS_INTERNAL,
.config_init = bcm63xx_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm63xx_config_intr,
}
Quoted 3 lines
So both callbacks are set. Can you extend your debugging and check in phy_drv_supports_irq() which of the callbacks is missing?
Hi, both callbacks are missing on the first check. However on the next calls they're there.
[    2.263909] libphy: Fixed MDIO Bus: probed
[    2.273026] =======phy_device_create===========
[    2.277908] phy_device_create: dev->irq = 17
[    2.277908]
[    2.373104] =======phy_probe===========
[    2.377336] phy_probe: phydev->irq = 17
[    2.381445] phy_drv_supports_irq: phydrv->config_intr = 0,
phydrv->ack_interrupt = 0
[    2.389554] phydev->irq = PHY_POLL;
[    2.393186] phy_probe: phydev->irq = -1
[    2.393186]
[    2.398987] =======phy_probe===========
[    2.403108] phy_probe: phydev->irq = -1
[    2.407195] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.415314] phy_probe: phydev->irq = -1
[    2.415314]
[    2.421189] libphy: bcm63xx_enet MII bus: probed
[    2.426129] =======phy_connect===========
[    2.430410] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.438537] phy_connect: phy_drv_supports_irq = 1
[    2.438537]
[    2.445284] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
Quoted 3 lines
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
Quoted 4 lines
I'd like to understand why the phy_device is probed twice, with which drivers it's probed.
Could you please add printing phydrv->name to phy_probe() ?
Hi Heiner, indeed there are two different probed devices. The B53 switch driver is causing this issue.
Quoted 6 lines
[    2.269595] libphy: Fixed MDIO Bus: probed
[    2.278706] =======phy_device_create===========
[    2.283594] phy_device_create: dev->irq = 17
[    2.283594]
[    2.379554] =======phy_probe===========
[    2.383780] phy_probe: phydrv->name = Broadcom B53 (3)
Is this an out-of-tree driver? I can't find this string in any DSA or PHY driver.

Quoted 7 lines

Quoted 3 lines
Yes it is.
https://github.com/openwrt/openwrt/blob/master/target/linux/generic/files/drivers/net/phy/b53/b53_mdio.c#L421
OK, I see. Then there's no reason to complain upstream. Either use the mainline B53 DSA driver of fix interrupt mode downstream.

I agree.

This b53 driver has one PHY with the same BCM63XX phy_id, causing a double probe. I'll send the original patch to the OpenWrt project.

Thank you very much.
Daniel

Quoted 90 lines

Quoted 89 lines
Quoted 89 lines
Quoted 88 lines
[    2.389235] phy_probe: phydev->irq = 17
[    2.393332] phy_drv_supports_irq: phydrv->config_intr = 0,
phydrv->ack_interrupt = 0
[    2.401445] phydev->irq = PHY_POLL
[    2.405080] phy_probe: phydev->irq = -1
[    2.405080]
[    2.410878] =======phy_probe===========
[    2.414996] phy_probe: phydrv->name = Broadcom BCM63XX (1)
[    2.420791] phy_probe: phydev->irq = -1
[    2.424876] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.432994] phy_probe: phydev->irq = -1
[    2.432994]
[    2.438862] libphy: bcm63xx_enet MII bus: probed
[    2.443809] =======phy_connect===========
[    2.448092] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.456215] phy_connect: phy_drv_supports_irq = 1
[    2.456215]
[    2.462961] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
The board has no switch, it's a driver for other boards in OpenWrt. I forgot it wasn't upstreamed:
https://github.com/openwrt/openwrt/tree/master/target/linux/generic/files/drivers/net/phy/b53
I tested a kernel compiled without this driver, now the IRQ is detected as it should be:
[    2.270707] libphy: Fixed MDIO Bus: probed
[    2.279715] =======phy_device_create===========
[    2.284600] phy_device_create: dev->irq = 17
[    2.284600]
[    2.373763] =======phy_probe===========
[    2.377989] phy_probe: phydrv->name = Broadcom BCM63XX (1)
[    2.383803] phy_probe: phydev->irq = 17
[    2.387888] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.396007] phy_probe: phydev->irq = 17
[    2.396007]
[    2.401877] libphy: bcm63xx_enet MII bus: probed
[    2.406820] =======phy_connect===========
[    2.411099] phy_drv_supports_irq: phydrv->config_intr = 1,
phydrv->ack_interrupt = 1
[    2.419226] phy_connect: phy_drv_supports_irq = 1
[    2.419226]
[    2.429857] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=17)
Then, maybe this is an OpenWrt bug itself?
Regards
Daniel
Quoted 30 lines
Quoted 29 lines
I also added the prints to phy_connect.
Quoted 4 lines
Last but not least: Do you use a mainline kernel, or is it maybe a modified downstream kernel? In the latter case, please check in your kernel sources whether both callbacks are set.
It's a modified kernel, and the the callbacks are set. BTW I also tested the kernel with no patches concerning to the ethernet driver.
Regards,
Daniel
Quoted 15 lines
Quoted 14 lines
Quoted 4 lines
On which kernel version do you face this problem?
The kernel version 4.4 works ok. The minimum version where I found the problem were the kernel 4.9.111, now using 5.4. And 5.10 also tested.
Regards
Daniel
Quoted 4 lines
Quoted 3 lines
Regards
Quoted 2 lines
--
Florian

Andrew Lunn

2021-02-26 14:17 UTC #17 lore raw

Quoted 8 lines

Quoted 3 lines
OK, I see. Then there's no reason to complain upstream. Either use the mainline B53 DSA driver of fix interrupt mode downstream.
I agree.
This b53 driver has one PHY with the same BCM63XX phy_id, causing a double probe. I'll send the original patch to the OpenWrt project.

Hi Daniel

There is a bit of a disconnect between OpenWRT and Mainline. They have a lot of fixes that don't make it upstream. So it is good to see somebody trying to fix mainline first, and then backport to OpenWRT. But please do test mainline and confirm it is actually broken before submitting patches.

When you do submit to OpenWRT, please make it clear this is an OpenWRT problem so somebody does not try to push it to mainline again....

And if you have an itch to scratch, try adding mainline support for this board. We can guide you.

Andrew

Heiner Kallweit

2021-02-26 14:29 UTC #18 lore raw

Quoted 17 lines · Andrew Lunn

Quoted 8 lines
Quoted 3 lines
OK, I see. Then there's no reason to complain upstream. Either use the mainline B53 DSA driver of fix interrupt mode downstream.
I agree.
This b53 driver has one PHY with the same BCM63XX phy_id, causing a double probe. I'll send the original patch to the OpenWrt project.
Hi Daniel
There is a bit of a disconnect between OpenWRT and Mainline. They have a lot of fixes that don't make it upstream. So it is good to see somebody trying to fix mainline first, and then backport to OpenWRT. But please do test mainline and confirm it is actually broken before submitting patches.

Quoted 6 lines

When you do submit to OpenWRT, please make it clear this is an OpenWRT problem so somebody does not try to push it to mainline again....
And if you have an itch to scratch, try adding mainline support for this board. We can guide you.

Daniel has two conflicting PHY drivers for bcm63xx, the one from mainline, and one in the OpenWRT downstream b53 driver. Removing the mainline PHY driver would resolve the conflict, but the OpenWRT PHY driver has no IRQ support so Daniel would gain nothing.
I think best would be to remove the duplicated PHY driver from the OpenWRT b53 driver. Daniel could try to remove b53_phy_driver_id3 and re-test.

Quoted 2 lines

Andrew

Heiner

Daniel González Cabanelas

author

2021-02-26 16:15 UTC #19 lore raw

I could update the BCM5365 phy_id in the downstream B53 driver to fix it and avoid any kind of future conflicts if the driver is upstreamed. Accordingly to documentation the whole BCM5365 UID (not masked) is 0x00406370.
PHYID HIGH[15:0] = OUI[21:6]
PHYID LOW[15:0] = OUI[5:0] + MODEL[5:0] + REV[3:0]

Right now the used mask is 0x1ffffc00. But if I understood correctly it is only required to mask the last 3 bits. This would reflect in the B53 driver:
---snip---
/* BCM5365 */
static struct phy_driver b53_phy_driver_id3 = {
.phy_id = 0x00406370,
.name = "Broadcom B53 (3)",
.phy_id_mask = 0xfffffff8,,
----snip---

For the tested board, BCM6348, the UID is 0x00406240 (read by the kernel). But in this case its driver involves more SoCs/PHYs, maybe with different UIDs.

Regards

Quoted 36 lines · Heiner Kallweit

Quoted 17 lines · Andrew Lunn
Quoted 8 lines
Quoted 3 lines
OK, I see. Then there's no reason to complain upstream. Either use the mainline B53 DSA driver of fix interrupt mode downstream.
I agree.
This b53 driver has one PHY with the same BCM63XX phy_id, causing a double probe. I'll send the original patch to the OpenWrt project.
Hi Daniel
There is a bit of a disconnect between OpenWRT and Mainline. They have a lot of fixes that don't make it upstream. So it is good to see somebody trying to fix mainline first, and then backport to OpenWRT. But please do test mainline and confirm it is actually broken before submitting patches.
Quoted 6 lines
When you do submit to OpenWRT, please make it clear this is an OpenWRT problem so somebody does not try to push it to mainline again....
And if you have an itch to scratch, try adding mainline support for this board. We can guide you.
Daniel has two conflicting PHY drivers for bcm63xx, the one from mainline, and one in the OpenWRT downstream b53 driver. Removing the mainline PHY driver would resolve the conflict, but the OpenWRT PHY driver has no IRQ support so Daniel would gain nothing.
I think best would be to remove the duplicated PHY driver from the OpenWRT b53 driver. Daniel could try to remove b53_phy_driver_id3 and re-test.
Quoted 2 lines
Andrew
Heiner

Florian Fainelli

2021-02-26 16:55 UTC #20 lore raw

Quoted 18 lines · Daniel González Cabanelas

I could update the BCM5365 phy_id in the downstream B53 driver to fix it and avoid any kind of future conflicts if the driver is upstreamed. Accordingly to documentation the whole BCM5365 UID (not masked) is 0x00406370.
PHYID HIGH[15:0] = OUI[21:6]
PHYID LOW[15:0] = OUI[5:0] + MODEL[5:0] + REV[3:0]
Right now the used mask is 0x1ffffc00. But if I understood correctly it is only required to mask the last 3 bits. This would reflect in the B53 driver:
---snip---
/* BCM5365 */
static struct phy_driver b53_phy_driver_id3 = {
.phy_id = 0x00406370,
.name = "Broadcom B53 (3)",
.phy_id_mask = 0xfffffff8,,
----snip---

Quoted 3 lines

For the tested board, BCM6348, the UID is 0x00406240 (read by the kernel). But in this case its driver involves more SoCs/PHYs, maybe with different UIDs.

Or another way to solve this entirely is to move to the upstream DSA driver for b53 under drivers/net/dsa/b53 and register the switch as a mdio_device instead of as a phy_device.

Signature · 1 line

Florian

Andrew Lunn

2021-02-25 13:54 UTC #21 lore raw

In reply to Daniel González Cabanelas

Quoted 13 lines · Daniel González Cabanelas

The current bcm63xx_enet driver doesn't asign the internal phy IRQ. As a result of this it works in polling mode.
Fix it using the phy_device structure to assign the platform IRQ.
Tested under a BCM6348 board. Kernel dmesg before the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=POLL)
After the patch:
Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY driver [Broadcom
              BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01, irq=17)

Quoted 8 lines

Pluging and uplugging the ethernet cable now generates interrupts and the PHY goes up and down as expected.
Signed-off-by Daniel González Cabanelas <[email protected]>
---
changes in V2:
- snippet moved after the mdiobus registration
- added missing brackets

Hi Daniel

It is a good idea to wait at least a day between posting versions. If you post too frequently, people tend to review the old version, since it is first in there mailbox.

Andrew