Message ID | 20190811150812.6780-2-marek.behun@nic.cz |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | [net-next,1/2] net: dsa: mv88e6xxx: fix RGMII-ID port setup | expand |
On Sun, Aug 11, 2019 at 05:08:12PM +0200, Marek Behún wrote: > The fixed_phy driver does not set the phydev->is_gigabit_capable member > when the fixed_phy is gigabit capable. Neither does any other PHY driver. It should be possible to tell if a PHY supports 1G by looking at register values. If this does not work for fixed_link, it means we are missing something in the emulation. That is what we should be fixing. Also, this change has nothing to do the lp_advertise, what you previously said the problem was. At the moment, i don't get the feeling you have really dug all the way down and really understand the root causes. Andrew
On Sun, 11 Aug 2019 17:40:01 +0200 Andrew Lunn <andrew@lunn.ch> wrote: > On Sun, Aug 11, 2019 at 05:08:12PM +0200, Marek Behún wrote: > > The fixed_phy driver does not set the phydev->is_gigabit_capable member > > when the fixed_phy is gigabit capable. > > Neither does any other PHY driver. It should be possible to tell if a > PHY supports 1G by looking at register values. If this does not work > for fixed_link, it means we are missing something in the emulation. > That is what we should be fixing. > > Also, this change has nothing to do the lp_advertise, what you > previously said the problem was. At the moment, i don't get the > feeling you have really dug all the way down and really understand the > root causes. > > Andrew Andrew, is_gigabit_capable is otherwise set only in the phy_probe function. This function is not called at all for the DSA cpu port fixed_link phy. Why is that? But I guess it is not important anymore, if CPU and DSA were converted to phylink in net-next. I shall test it and let you know. In any case, sorry for the spam. Marek
On 11.08.2019 18:08, Marek Behun wrote: > On Sun, 11 Aug 2019 17:40:01 +0200 > Andrew Lunn <andrew@lunn.ch> wrote: > >> On Sun, Aug 11, 2019 at 05:08:12PM +0200, Marek Behún wrote: >>> The fixed_phy driver does not set the phydev->is_gigabit_capable member >>> when the fixed_phy is gigabit capable. >> >> Neither does any other PHY driver. It should be possible to tell if a >> PHY supports 1G by looking at register values. If this does not work >> for fixed_link, it means we are missing something in the emulation. >> That is what we should be fixing. >> >> Also, this change has nothing to do the lp_advertise, what you >> previously said the problem was. At the moment, i don't get the >> feeling you have really dug all the way down and really understand the >> root causes. >> >> Andrew > > Andrew, > is_gigabit_capable is otherwise set only in the phy_probe function. > This function is not called at all for the DSA cpu port fixed_link phy. > Why is that? But I guess it is not important anymore, if CPU and DSA > were converted to phylink in net-next. I shall test it and let you know. > In any case, sorry for the spam. > Marek > A fixed phy should be bound to the genphy driver, but that happens later in phy_attach_direct(). Maybe the fixed phy device of a CPU port gets never attached to a net device? This would explain why phy_probe() doesn't get called. Following idea: We could swphy let return PHY ID 0xffffffff (instead of current value 0x00000000). Then the fixed phy device would be bound to the genphy driver immediately at device registration time. As a consequence phy_probe() would call genphy_read_abilities() that sets supported modes properly. This should allow to remove the manual adding of supported modes in dsa_port_fixed_link_register_of(). One more thing regarding genphy_read_abilities() and fixed phys in general: swphy sets bit BMSR_ESTATEN, then genphy_read_abilities() reads MII_ESTATUS to check if and which 1Gbps modes are supported. MII_ESTATUS however isn't defined in swphy. We're just fortunate that therefore the default value 0xffff is returned and both 1Gbps modes seem to be supported. Last but not least I think the calls to genphy_config_init() and genphy_read_status() in dsa_port_fixed_link_register_of() are both not needed and could be removed. Heiner
diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c index 3ffe46df249e..424b02ad7b7b 100644 --- a/drivers/net/phy/fixed_phy.c +++ b/drivers/net/phy/fixed_phy.c @@ -286,6 +286,7 @@ static struct phy_device *__fixed_phy_register(unsigned int irq, phy->supported); linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, phy->supported); + phy->is_gigabit_capable = 1; /* fall through */ case SPEED_100: linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
The fixed_phy driver does not set the phydev->is_gigabit_capable member when the fixed_phy is gigabit capable. This in turn causes phy_device.c:genphy_read_status function to return unknown phy parameters (SPEED_UNKNOWN, DUPLEX_UNKNOWN, ...), if the fixed_phy is created with SPEED_1000. Signed-off-by: Marek Behún <marek.behun@nic.cz> Cc: Heiner Kallweit <hkallweit1@gmail.com> Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk> Cc: Vivien Didelot <vivien.didelot@gmail.com> Cc: Andrew Lunn <andrew@lunn.ch> Cc: Florian Fainelli <f.fainelli@gmail.com> Cc: David S. Miller <davem@davemloft.net> --- drivers/net/phy/fixed_phy.c | 1 + 1 file changed, 1 insertion(+)