From: Daniel González Cabanelas <hidden> Date: 2021-02-24 16:05:09
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(-)
On 24.02.2021 16:44, Daniel González Cabanelas wrote:
quoted hunk
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(-)
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().
if (ret) {
dev_err(&pdev->dev, "unable to register mdio bus\n");
goto out_free_mdio;
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(-)
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?
--
Florian
From: Daniel González Cabanelas <hidden> Date: 2021-02-24 23:56:08
El mié, 24 feb 2021 a las 23:01, Florian Fainelli
([off-list ref]) escribió:
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(-)
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
On 25.02.2021 00:54, Daniel González Cabanelas wrote:
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(-)
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()?.
On which kernel version do you face this problem?
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-02-25 13:54:34
On Wed, Feb 24, 2021 at 04:44:18PM +0100, Daniel González Cabanelas wrote:
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
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
From: Daniel González Cabanelas <hidden> Date: 2021-02-25 16:38:09
El jue, 25 feb 2021 a las 8:22, Heiner Kallweit
([off-list ref]) escribió:
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(-)
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.
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
On 25.02.2021 17:36, Daniel González Cabanelas wrote:
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(-)
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?
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
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
From: Daniel González Cabanelas <hidden> Date: 2021-02-25 22:29:58
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(-)
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
On 25.02.2021 23:28, Daniel González Cabanelas wrote:
El jue, 25 feb 2021 a las 21:05, Heiner Kallweit
([off-list ref]) escribió:
quoted
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(-)
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
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.
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
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
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
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.
--
Florian
On 25.02.2021 23:28, Daniel González Cabanelas wrote:
El jue, 25 feb 2021 a las 21:05, Heiner Kallweit
([off-list ref]) escribió:
quoted
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(-)
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?
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() ?
I also added the prints to phy_connect.
quoted
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
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
From: Daniel González Cabanelas <hidden> Date: 2021-02-26 09:12:22
El vie, 26 feb 2021 a las 8:13, Heiner Kallweit
([off-list ref]) escribió:
On 25.02.2021 23:28, Daniel González Cabanelas wrote:
quoted
El jue, 25 feb 2021 a las 21:05, Heiner Kallweit
([off-list ref]) escribió:
quoted
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(-)
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?
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
I also added the prints to phy_connect.
quoted
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
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
On 26.02.2021 10:10, Daniel González Cabanelas wrote:
El vie, 26 feb 2021 a las 8:13, Heiner Kallweit
([off-list ref]) escribió:
quoted
On 25.02.2021 23:28, Daniel González Cabanelas wrote:
quoted
El jue, 25 feb 2021 a las 21:05, Heiner Kallweit
([off-list ref]) escribió:
quoted
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(-)
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?
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)
Is this an out-of-tree driver? I can't find this string in any
DSA or PHY driver.
[ 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
quoted
I also added the prints to phy_connect.
quoted
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
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
On 26.02.2021 10:10, Daniel González Cabanelas wrote:
quoted
El vie, 26 feb 2021 a las 8:13, Heiner Kallweit
([off-list ref]) escribió:
quoted
On 25.02.2021 23:28, Daniel González Cabanelas wrote:
quoted
El jue, 25 feb 2021 a las 21:05, Heiner Kallweit
([off-list ref]) escribió:
quoted
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(-)
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?
[ 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
quoted
I also added the prints to phy_connect.
quoted
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
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
From: Daniel González Cabanelas <hidden> Date: 2021-02-26 09:51:24
El vie, 26 feb 2021 a las 10:32, Heiner Kallweit
([off-list ref]) escribió:
On 26.02.2021 10:10, Daniel González Cabanelas wrote:
quoted
El vie, 26 feb 2021 a las 8:13, Heiner Kallweit
([off-list ref]) escribió:
quoted
On 25.02.2021 23:28, Daniel González Cabanelas wrote:
quoted
El jue, 25 feb 2021 a las 21:05, Heiner Kallweit
([off-list ref]) escribió:
quoted
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(-)
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?
[ 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
quoted
I also added the prints to phy_connect.
quoted
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
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
On 26.02.2021 10:49, Daniel González Cabanelas wrote:
El vie, 26 feb 2021 a las 10:32, Heiner Kallweit
([off-list ref]) escribió:
quoted
On 26.02.2021 10:10, Daniel González Cabanelas wrote:
quoted
El vie, 26 feb 2021 a las 8:13, Heiner Kallweit
([off-list ref]) escribió:
quoted
On 25.02.2021 23:28, Daniel González Cabanelas wrote:
quoted
El jue, 25 feb 2021 a las 21:05, Heiner Kallweit
([off-list ref]) escribió:
quoted
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(-)
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?
OK, I see. Then there's no reason to complain upstream.
Either use the mainline B53 DSA driver of fix interrupt mode
downstream.
quoted
quoted
[ 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
quoted
I also added the prints to phy_connect.
quoted
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
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
From: Daniel González Cabanelas <hidden> Date: 2021-02-26 10:20:51
El vie, 26 feb 2021 a las 11:08, Heiner Kallweit
([off-list ref]) escribió:
On 26.02.2021 10:49, Daniel González Cabanelas wrote:
quoted
El vie, 26 feb 2021 a las 10:32, Heiner Kallweit
([off-list ref]) escribió:
quoted
On 26.02.2021 10:10, Daniel González Cabanelas wrote:
quoted
El vie, 26 feb 2021 a las 8:13, Heiner Kallweit
([off-list ref]) escribió:
quoted
On 25.02.2021 23:28, Daniel González Cabanelas wrote:
quoted
El jue, 25 feb 2021 a las 21:05, Heiner Kallweit
([off-list ref]) escribió:
quoted
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(-)
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?
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
quoted
quoted
[ 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
quoted
I also added the prints to phy_connect.
quoted
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
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
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-02-26 14:17:21
quoted
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
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.
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.
From: Daniel González Cabanelas <hidden> Date: 2021-02-26 16:15:11
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
El vie, 26 feb 2021 a las 15:28, Heiner Kallweit
([off-list ref]) escribió:
On 26.02.2021 15:16, Andrew Lunn wrote:
quoted
quoted
quoted
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.
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.
On 2/26/2021 8:14 AM, Daniel González Cabanelas wrote:
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.
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.
--
Florian