Hi Marek, On Sun, 11 Aug 2019 at 17:06, Marek Behun <marek.be...@nic.cz> wrote: > > OK guys, something is terribly wrong here. > > I bisected to the commit mentioned (88d6272acaaa), looked around at the > genphy functions, tried adding the link=0 workaround and it did work, > so I though this was the issue. > > What I realized now is that before the commit 88d6272acaaa things > worked because of two bugs, which negated each other. This commit caused > one of this bugs not to fire, and thus the second bug was not negated. > > What actually happened before the commit that broke it is this: > - after the fixed_phy is created, the parameters are corrent > - genphy_read_status breaks the parameters: > - first it sets the parameters to unknown (SPEED_UNKNOWN, > DUPLEX_UNKNOWN) > - then read the registers, which are simulated for fixed_phy > - then it uses phy-core.c:phy_resolve_aneg_linkmode function, which > looks for correct settings by bit-anding the ->advertising and > ->lp_advertigins bit arrays. But in fixed_phy, ->lp_advertising > is set to zero, so the parameters are left at SPEED_UNKNOWN, ... > (this is the first bug) > - then adjust_link is called, which then goes to > mv88e6xxx_port_setup_mac, where there is a test if it should change > something: > if (state.link == link && state.speed == speed && > state.duplex == duplex) > return 0; > - since current speed on the switch port (state.speed) is SPEED_1000, > and new speed is SPEED_UNKNOWN, this test fails, and so the rest of > this function is called, which makes the port work > (the if test is the second bug) > > After the commit that broke things: > - after the fixed_phy is created, the parameters are corrent > - genphy_read_status doesn't change them > - mv88e6xxx_port_setup_mac does nothing, since the if condition above > is true > > So, there are two things that are broken: > - the test in mv88e6xxx_port_setup_mac whether there is to be a change > should be more sophisticated > - fixed_phy should also simulate the lp_advertising register > > What do you think of this? >
I don't know. But I think Heiner was asking you what kernel you're on because of what you said here: > Hopefully DSA fixed-link port functionality will be converted to phylink > API soon. The DSA fixed-link port functionality *has* been converted to phylink. See: - https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=0e27921816ad9 - https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=7fb5a711545d7d25fe9726a9ad277474dd83bd06 > Marek > > On Sun, 11 Aug 2019 13:35:20 +0200 > Heiner Kallweit <hkallwe...@gmail.com> wrote: > > > On 11.08.2019 05:39, Andrew Lunn wrote: > > > On Sun, Aug 11, 2019 at 05:18:57AM +0200, Marek Behún wrote: > > >> Commit 88d6272acaaa ("net: phy: avoid unneeded MDIO reads in > > >> genphy_read_status") broke fixed link DSA port registration in > > >> dsa_port_fixed_link_register_of: the genphy_read_status does not do what > > >> it is supposed to and the following adjust_link is given wrong > > >> parameters. > > > > > > Hi Marek > > > > > > Which parameters are incorrect? > > > > > > In fixed_phy.c, __fixed_phy_register() there is: > > > > > > /* propagate the fixed link values to struct phy_device */ > > > phy->link = status->link; > > > if (status->link) { > > > phy->speed = status->speed; > > > phy->duplex = status->duplex; > > > phy->pause = status->pause; > > > phy->asym_pause = status->asym_pause; > > > } > > > > > > Are we not initialising something? Or is the initialisation done here > > > getting reset sometime afterwards? > > > > > In addition to Andrew's question: > > We talk about this DT config: armada-385-turris-omnia.dts ? > > Which kernel version are you using? > > > > > Thanks > > > Andrew > > > > > Heiner > Regards, -Vladimir
