Date: Thu, 11 May 2023 22:15:19 -0700
From: Ron Eggler <ron.eggler@...tywest.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: netdev@...r.kernel.org
Subject: Re: PHY VSC8531 MDIO data not reflected in ethernet/ sub-module


On 2023-05-11 16:21, Ron Eggler wrote:
> Hi Andrew,
>
> Thanks for your response:
>
> On 2023-05-11 8:34 a.m., Andrew Lunn wrote:
>>> ***[ 6.728165] DEBUG: in ravb_emac_init_gbeth(), calling
>>> ravb_set_rate_gbeth(), priv->duplex: 0, priv->speed: 0*
>>> ***[ 6.751973] DEBUG: in ravb_set_rate_gbeth() - priv->speed 0*
>>> ***[ 6.831153] DEBUG: in ravb_adjust_link(), phydev->speed -1, 
>>> priv->speed
>>> 0*
>>> ***[ 6.839952] DEBUG: in ravb_adjust_link(), priv->no_avb_link 0,
>>> phydev->link 0*
>> If there is no link, everything else is meaningless. You cannot have
>> speed without link.
> Makes sense!
>>> While I also receive the following using the mii-tool utility:
>>> # mii-tool -vv eth0
>>> Using SIOCGMIIPHY=0x8947
>>> eth0: negotiated 1000baseT-FD flow-control, link ok
>>>    registers for MII PHY 0:
>>>      1040 796d 0007 0572 01e1 cde1 000f 2001
>>>      4006 0300 7800 0000 0000 4002 0000 3000
>>>      0000 f000 0088 0000 0000 0001 3200 1000
>>>      0000 0000 0000 0000 a035 0054 0400 0000
>>>    product info: vendor 00:01:c1, model 23 rev 2
>>>    basic mode:   autonegotiation enabled
>>>    basic status: autonegotiation complete, link ok
>>>    capabilities: 1000baseT-HD 1000baseT-FD 100baseTx-FD 100baseTx-HD
>>> 10baseT-FD 10baseT-HD
>>>    advertising:  1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 
>>> 10baseT-HD
>>>    link partner: 1000baseT-HD 1000baseT-FD 100baseTx-FD 100baseTx-HD
>>> 10baseT-FD 10baseT-HD flow-control
>> Assuming you are not using interrupts, phylib will poll the PHY once a
>> second, calling
>> phy_check_link_status()->
>
> I don't see it being invoked every Seconds but it gets invoked on 
> boot, I added debug logs and see the following:
>
> [    6.669425] DEBUG: in phy_check_link_status(), phy_read_status() 
> ret err 0, phydev->loopback_enabled 0,
> [    6.696237] DEBUG: in phy_check_link_status(), phydev->link 0, 
> phydev->state UP
>
> state = UP which means it's ready to start auto negotiation(in 
> phy_state_machine())  but instead in phy_check_link_status(), 
> phydev->state should be set to  PHY_RUNNING but it only can get set to 
> PHY_RUNNING when phydev->link is 1 (in phy_check_link_status()):
>
>>    phy_read_status()->
>>      phydev->drv->read_status()
>> or
>>      genphy_read_status()
>>
>> Check what these are doing, why do they think the link is down.
>
> Yes, so in phy_read_status, phydev->drv->read_status appears to be set 
> but I cannot figure out where it gets set. (I obviously need to find 
> the function to find why the link isn't read correctly).
> I temporarily set phydev->drv->read_status to 0 to force invocation of 
> genphy_read_status() function to see how that would work.
>
> genphy_update_link(0 is called from genphy_read_status() and I get the 
> below data:
>
> [    6.795480] DEBUG: in genphy_update_link(), after phy_read() bmcr 4160
> [    6.805225] DEBUG: in genphy_update_link(), bmcr 0x1040 & 0x200
> [    6.815730] DEBUG: in genphy_read_status(), genphy_update_link() 0 
> phydev->autoneg 1, phydev->link 0
>
>
> Could it be that the link needs a second to come up when when the 
> network drivers get started and hence I should make sure that the 
> polling once a second works (which currently doesn't appear to be the 
> case)? Am I missing a configuration option?

For now I've tried to add:
phydev->irq = PHY_POLL;
to phy_attached_info_irq() in drivers/net/phy/phy_device.c which turns 
out totally locks-up the system, i.e. I can't even type root to login 
anymore. This is after I removed the log messages now but while I still 
had them in there, they would just fill up my screen, the polling 
appears to use up all the CPU power.

A little cut-out from the logs, looked like this:

...
[  418.485394] DEBUG: in phy_check_link_status(), phy_read_status() err 
0, phydev->loopback_enabled 0,
[  418.503700] DEBUG: in phy_check_link_status(), phydev->link 1, 
phydev->state RUNNING
[  418.511463] DEBUG: in phy_state_machine(), phydev->state RUNNING, 
needs_aneg 0
[  418.518727] DEBUG: in phy_state_machine(), phy_start_aneg() 0, 
needs_aneg 826560768
[  419.361074] DEBUG: in phy_read_status(), phydev->drv->read_status 0
[  419.367474] DEBUG: in genphy_update_link(), after phy_read() bmcr 4160
[  419.375665] DEBUG: in genphy_update_link(), bmcr 0x1040 & 0x200
[  419.382324] DEBUG: in genphy_read_status(), genphy_update_link() 0 
phydev->autoneg 1, phydev->link 0
[  419.382547] DEBUG: in phy_check_link_status(), phy_read_status() err 
0, phydev->loopback_enabled 0,
[  419.400837] DEBUG: in phy_check_link_status(), phydev->link 0, 
phydev->state NOLINK
[  419.408521] DEBUG: in phy_state_machine(), phydev->state NOLINK, 
needs_aneg 0
[  419.415718] DEBUG: in phy_state_machine(), phy_start_aneg() 0, 
needs_aneg 826560768
[  419.553089] DEBUG: in phy_read_status(), phydev->drv->read_status 0
[  419.559486] DEBUG: in genphy_update_link(), after phy_read() bmcr 4160
[  419.566858] DEBUG: in genphy_update_link(), bmcr 0x1040 & 0x200
[  419.573423] DEBUG: in genphy_read_status(), genphy_update_link() 0 
phydev->autoneg 1, phydev->link 1
[  419.573432] DEBUG: in phy_check_link_status(), phy_read_status() err 
0, phydev->loopback_enabled 0,
[  419.591738] DEBUG: in phy_check_link_status(), phydev->link 1, 
phydev->state RUNNING
[  419.599505] DEBUG: in phy_state_machine(), phydev->state RUNNING, 
needs_aneg 0
[  419.606764] DEBUG: in phy_state_machine(), phy_start_aneg() 0, 
needs_aneg 826560768
...

The state toggled between RUNNING & NOLINK and the phydev->link toggeled 
between 1 and 0

The reason why I had interrupts activated is because the device tree 
that's used as a base had a PHY with the interrupt pin hooked up and 
while I override the original phy node in the device tree with 
https://github.com/MistySOM/meta-mistysom/blob/phy-enable/recipes-kernel/linux/smarc-rzg2l/0001-add-vsc8531-userspace-dts.patch 
, the interrupt attribute from the original device tree will persist. I 
will have to create a patch that removes the attrribute in the dtsi file 
where the original phy node is defined. I think that should give me the 
poll functionality

-- 

Ron

Powered by blists - more mailing lists