On Fri, Jan 25, 2019 at 11:00:46AM -0800, Florian Fainelli wrote: > > +++ b/drivers/net/phy/phylink.c > > @@ -409,7 +409,8 @@ static void phylink_resolve(struct work_struct *w) > > > > case MLO_AN_FIXED: > > phylink_get_fixed_state(pl, &link_state); > > - phylink_mac_config(pl, &link_state); > > + if (link_state.link != netif_carrier_ok(ndev)) > > + phylink_mac_config(pl, &link_state); > > With that change though, I don't think we would ever be able to disable > the MAC upon link down which could be sucking additional power, so maybe > we should be comparing link_state.link with the previous link_state > (prior to calling phylink_get_fixed_state(), so something like that: > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > index e7becc7379d7..88e0045ecf79 100644 > --- a/drivers/net/phy/phylink.c > +++ b/drivers/net/phy/phylink.c > @@ -409,7 +409,8 @@ static void phylink_resolve(struct work_struct *w) > > case MLO_AN_FIXED: > phylink_get_fixed_state(pl, &link_state); > - phylink_mac_config(pl, &link_state); > + if (pl->mac_link_dropped != link_state.link) > + phylink_mac_config(pl, &link_state); > break; > > case MLO_AN_INBAND: > > > It seems to me that the problem might very well be mv88e6xxx specific in > that the function mv88e6xxx_port_setup_mac() calls into multiple other > functions in order to configure the port's MAC and that is not atomic > from the HW's perspective, therefore there is a chance for the HW to > latch in some register writes (even if they are the same values) trigger > a MAC reconfiguration upon that write, which is responsible for causing > some packets to be dropped.
The mac_config is expected to _update_ the MAC, not reprogram it in full each time it's called. Consider a SGMII link with a PHY on a SFP module. The SGMII link must be placed in "negotiation" mode (so must use in-band negotiation) but there is no mechanism on the link to convey the negotiated flow control settings. So, mac_config() must be called whenever the link changes to update the MAC with the flow control settings read from the PHY. However, the MAC still needs negotiation enabled on the link, and must not disturb that, because it could cause the PHY to then report link-down. That would throw phylink into a continual loop of the link bouncing up and down. This is how I've implemented mac_config() in both mvneta and the out of tree mvpp2x driver - in mvneta, I read the current register settings, modify them according to the requested parameters, then compare them, and only write the new register settings back as required taking account of hardware restrictions (eg, port needs to be temporarily disabled if certain settings are changed.) If mac_config() is implemented as per that, then the above patch is unnecessary. I know that detail is not documented, it's something that I need to update in the header file and my .rst documentation (also something else that needs submitting.) I also have a patch for phylink that allows us to use the GPIO interrupt rather than polling with a 1s timer to check the state of the fixed-link GPIO. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
