Re: [PATCH v2] bcm63xx_enet: fix internal phy IRQ assignment

El jue, 25 feb 2021 a las 21:05, Heiner Kallweit
([off-list ref]) escribió:

On 25.02.2021 17:36, Daniel González Cabanelas wrote:

quoted

El jue, 25 feb 2021 a las 8:22, Heiner Kallweit
([off-list ref]) escribió:

quoted

On 25.02.2021 00:54, Daniel González Cabanelas wrote:

quoted

El mié, 24 feb 2021 a las 23:01, Florian Fainelli
([off-list ref]) escribió:

quoted

On 2/24/2021 1:44 PM, Heiner Kallweit wrote:

quoted

On 24.02.2021 16:44, Daniel González Cabanelas wrote:

quoted

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

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

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.

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?

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.

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

quoted

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

quoted

Regards

quoted

--
Florian