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

[Florian Fainelli <f.fainelli@gmail.com>]

On 2/25/2021 2:56 PM, Heiner Kallweit wrote:
>>>>> 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