| Message ID | 20211126090540.3550913-1-horatiu.vultur@microchip.com |
|---|---|
| Headers | show |
| Series | net: lan966x: Add lan966x switch driver | expand |
Hi Horatiu, On Fri, 2021-11-26 at 10:05 +0100, Horatiu Vultur wrote: [...] > +static int lan966x_reset_switch(struct lan966x *lan966x) > +{ > + struct reset_control *reset; > + int val = 0; > + int ret; > + > + reset = devm_reset_control_get_shared(lan966x->dev, "switch"); > + if (IS_ERR(reset)) > + return dev_err_probe(lan966x->dev, PTR_ERR(reset), > + "Could not obtain switch reset"); > + reset_control_reset(reset); > + > + reset = devm_reset_control_get_shared(lan966x->dev, "phy"); > + if (IS_ERR(reset)) > + return dev_err_probe(lan966x->dev, PTR_ERR(reset), > + "Could not obtain phy reset\n"); > + reset_control_reset(reset); In general, please request all resources before activating the hardware. Here I'd request both reset controls first and only then trigger them. That way a failure to optain the phy reset won't cause an unnecessary switch reset. [...] > +static int lan966x_remove(struct platform_device *pdev) > +{ > + return 0; > +} If there is nothing to do here, this function can be removed altogether. regards Philipp
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.
On Fri, 26 Nov 2021 10:05:37 +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> Clang sees issues here: drivers/net/ethernet/microchip/lan966x/lan966x_main.c:409:8: warning: variable 'sz' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] if (err != 4) ^~~~~~~~ drivers/net/ethernet/microchip/lan966x/lan966x_main.c:469:7: note: uninitialized use occurs here if (sz < 0 || err) ^~ drivers/net/ethernet/microchip/lan966x/lan966x_main.c:409:4: note: remove the 'if' if its condition is always false if (err != 4) ^~~~~~~~~~~~~ drivers/net/ethernet/microchip/lan966x/lan966x_main.c:403:9: note: initialize the variable 'sz' to silence this warning int sz, buf_len; ^ = 0
The 11/26/2021 10:12, Jakub Kicinski wrote: Hi Jakub, > > On Fri, 26 Nov 2021 10:05:37 +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> > > Clang sees issues here: > > drivers/net/ethernet/microchip/lan966x/lan966x_main.c:409:8: warning: variable 'sz' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] > if (err != 4) > ^~~~~~~~ > drivers/net/ethernet/microchip/lan966x/lan966x_main.c:469:7: note: uninitialized use occurs here > if (sz < 0 || err) > ^~ > drivers/net/ethernet/microchip/lan966x/lan966x_main.c:409:4: note: remove the 'if' if its condition is always false > if (err != 4) > ^~~~~~~~~~~~~ > drivers/net/ethernet/microchip/lan966x/lan966x_main.c:403:9: note: initialize the variable 'sz' to silence this warning > int sz, buf_len; > ^ > = 0 Thanks for notification. I should definitely need to start to build the kernel using Clang.
The 11/26/2021 11:04, Russell King (Oracle) wrote: > > 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: Thanks for the help! > > > +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 for the detail explanation. You are right, it doesn't look like it is needed to call lan966x_port_link_down when pcs_config is called. I can put the lan966x_port_link_down() inside the mac_link_down() callback. > > Thanks. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!