Messages in this thread | | | Date | Fri, 26 Nov 2021 11:04:25 +0000 | From | "Russell King (Oracle)" <> | Subject | Re: [PATCH net-next v4 3/6] net: lan966x: add port module support |
| |
On Fri, Nov 26, 2021 at 10:05:37AM +0100, Horatiu Vultur wrote: > This patch adds support for netdev and phylink in the switch. The > injection + extraction is register based. This will be replaced with DMA > accees. > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
This looks mostly good now, thanks. There's one remaining issue:
> +int lan966x_port_pcs_set(struct lan966x_port *port, > + struct lan966x_port_config *config) > +{ > + struct lan966x *lan966x = port->lan966x; > + bool inband_aneg = false; > + bool outband; > + int err; > + > + lan966x_port_link_down(port);
This looks like something the MAC layer should be doing. Phylink won't change the interface mode by just calling the PCS - it will do this sequence, known as a major reconfiguration:
mac_link_down() (if the link was previously up) mac_prepare() mac_config() if (pcs_config() > 0) pcs_an_restart() mac_finish()
pcs_config() will also be called thusly:
if (pcs_config() > 0) pcs_an_restart()
to change the ethtool advertising mask which changes the inband advert or the Autoneg bit, which has an effect only on your DEV_PCS1G_ANEG_CFG() register, and this may be called with the link up or down.
Also, pcs_config() is supposed to return 0 if the inband advert has not changed, or positive if it has (so pcs_an_restart() is called to cause in-band negotiation to be restarted.)
Note also that pcs_an_restart() may also be called when ethtool requests negotiation restart when we're operating in 802.3z modes.
So, my question is - do you need to be so heavy weight with the call to lan966x_port_link_down() to take everything down when pcs_config() is called, and is it really in the right place through the sequence for a major reconfiguration?
Thanks.
-- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
| |