The 11/24/2021 10:20, Russell King (Oracle) wrote:
>
> Hi,
Hi Russel,
>
> On Wed, Nov 24, 2021 at 09:39:12AM +0100, Horatiu Vultur wrote:
> > +static int lan966x_port_open(struct net_device *dev)
> > +{
> > + struct lan966x_port *port = netdev_priv(dev);
> > + struct lan966x *lan966x = port->lan966x;
> > + int err;
> > +
> > + if (port->serdes) {
> > + err = phy_set_mode_ext(port->serdes, PHY_MODE_ETHERNET,
> > + port->config.phy_mode);
> > + if (err) {
> > + netdev_err(dev, "Could not set mode of SerDes\n");
> > + return err;
> > + }
> > + }
>
> This could be done in the mac_prepare() method.
Yes, I will move this in mac_prepare()
>
> > +static void lan966x_cleanup_ports(struct lan966x *lan966x)
> > +{
> > + struct lan966x_port *port;
> > + int portno;
> > +
> > + for (portno = 0; portno < lan966x->num_phys_ports; portno++) {
> > + port = lan966x->ports[portno];
> > + if (!port)
> > + continue;
> > +
> > + if (port->phylink) {
> > + rtnl_lock();
> > + lan966x_port_stop(port->dev);
> > + rtnl_unlock();
> > + phylink_destroy(port->phylink);
> > + port->phylink = NULL;
> > + }
> > +
> > + if (port->fwnode)
> > + fwnode_handle_put(port->fwnode);
> > +
> > + if (port->dev)
> > + unregister_netdev(port->dev);
>
> This doesn't look like the correct sequence to me. Shouldn't the net
> device be unregistered first, which will take the port down by doing
> so and make it unavailable to userspace to further manipulate. Then
> we should start tearing other stuff down such as destroying phylink
> and disabling interrupts (in the caller of this.)
I can change the order as you suggested.
Regarding the interrupts, shouldn't they be first disable and then do
all the teardown?
>
> Don't you need to free the netdev as well at some point?
It is not needed because they are allocated using devm_alloc_etherdev_mqs
>
> > static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
> > - phy_interface_t phy_mode)
> > + phy_interface_t phy_mode,
> > + struct fwnode_handle *portnp)
> > {
> ...
> > + port->phylink_config.dev = &port->dev->dev;
> > + port->phylink_config.type = PHYLINK_NETDEV;
> > + port->phylink_config.pcs_poll = true;
> > + port->phylink_pcs.poll = true;
>
> You don't need to set both of these - please omit
> port->phylink_config.pcs_poll.
I will remove it.
>
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > index 7a1ff9d19fbf..ce2798db0449 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> ...
> > @@ -44,15 +58,48 @@ struct lan966x {
> > void __iomem *regs[NUM_TARGETS];
> >
> > int shared_queue_sz;
> > +
> > + /* interrupts */
> > + int xtr_irq;
> > +};
> > +
> > +struct lan966x_port_config {
> > + phy_interface_t portmode;
> > + phy_interface_t phy_mode;
>
> What is the difference between "portmode" and "phy_mode"? Does it matter
> if port->config.phy_mode get zeroed when lan966x_port_pcs_set() is
> called from lan966x_pcs_config()? It looks to me like the first call
> will clear phy_mode, setting it to PHY_INTERFACE_MODE_NA from that point
> on.
The purpose was to use portmode to configure the MAC and the phy_mode
to configure the serdes. There are small issues regarding this which
will be fix in the next series also I will add some comments just to
make it clear.
Actually, port->config.phy_mode will not get zeroed. Because right after
the memset it follows: 'config = port->config'.
>
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_port.c b/drivers/net/ethernet/microchip/lan966x/lan966x_port.c
> > new file mode 100644
> > index 000000000000..ca1b0c8d1bf5
> > --- /dev/null
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_port.c
> > @@ -0,0 +1,422 @@
> ...
> > +void lan966x_port_status_get(struct lan966x_port *port,
> > + struct phylink_link_state *state)
> > +{
> > + struct lan966x *lan966x = port->lan966x;
> > + u16 lp_adv, ld_adv;
> > + bool link_down;
> > + u16 bmsr = 0;
> > + u32 val;
> > +
> > + val = lan_rd(lan966x, DEV_PCS1G_STICKY(port->chip_port));
> > + link_down = DEV_PCS1G_STICKY_LINK_DOWN_STICKY_GET(val);
> > + if (link_down)
> > + lan_wr(val, lan966x, DEV_PCS1G_STICKY(port->chip_port));
> > +
> > + /* Get both current Link and Sync status */
> > + val = lan_rd(lan966x, DEV_PCS1G_LINK_STATUS(port->chip_port));
> > + state->link = DEV_PCS1G_LINK_STATUS_LINK_STATUS_GET(val) &&
> > + DEV_PCS1G_LINK_STATUS_SYNC_STATUS_GET(val);
> > + state->link &= !link_down;
> > +
> > + if (port->config.portmode == PHY_INTERFACE_MODE_1000BASEX)
> > + state->speed = SPEED_1000;
> > + else if (port->config.portmode == PHY_INTERFACE_MODE_2500BASEX)
> > + state->speed = SPEED_2500;
>
> Why not use state->interface? state->interface will be the currently
> configured interface mode (which should be the same as your
> port->config.portmode.)
I will use state->interface.
>
> > +
> > + state->duplex = DUPLEX_FULL;
>
> Also, what is the purpose of initialising state->speed and state->duplex
> here? phylink_mii_c22_pcs_decode_state() will do that for you when
> decoding the advertisements.
>
> If it's to deal with autoneg disabled, then it ought to be conditional on
> autoneg being disabled and the link being up.
It was the case for autoneg disabled.
>
> > +
> > + /* Get PCS ANEG status register */
> > + val = lan_rd(lan966x, DEV_PCS1G_ANEG_STATUS(port->chip_port));
> > +
> > + /* Aneg complete provides more information */
> > + if (DEV_PCS1G_ANEG_STATUS_ANEG_COMPLETE_GET(val)) {
> > + lp_adv = DEV_PCS1G_ANEG_STATUS_LP_ADV_GET(val);
> > + state->an_complete = true;
> > +
> > + bmsr |= state->link ? BMSR_LSTATUS : 0;
> > + bmsr |= state->an_complete;
>
> Shouldn't this be setting BMSR_ANEGCOMPLETE?
That was a silly mistake from my side.
>
> > +
> > + if (port->config.portmode == PHY_INTERFACE_MODE_SGMII) {
> > + phylink_mii_c22_pcs_decode_state(state, bmsr, lp_adv);
> > + } else {
> > + val = lan_rd(lan966x, DEV_PCS1G_ANEG_CFG(port->chip_port));
> > + ld_adv = DEV_PCS1G_ANEG_CFG_ADV_ABILITY_GET(val);
> > + phylink_mii_c22_pcs_decode_state(state, bmsr, ld_adv);
> > + }
>
> This looks like it can be improved:
>
> if (DEV_PCS1G_ANEG_STATUS_ANEG_COMPLETE_GET(val)) {
> state->an_complete = true;
>
> bmsr |= state->link ? BMSR_LSTATUS : 0;
> bmsr |= BMSR_ANEGCOMPLETE;
>
> if (state->interface == PHY_INTERFACE_MODE_SGMII) {
> lp_adv = DEV_PCS1G_ANEG_STATUS_LP_ADV_GET(val);
> } else {
> val = lan_rd(lan966x, DEV_PCS1G_ANEG_CFG(port->chip_port));
> lp_adv = DEV_PCS1G_ANEG_CFG_ADV_ABILITY_GET(val);
> }
>
> phylink_mii_c22_pcs_decode_state(state, bmsr, lp_adv);
> }
>
> I'm not sure that the non-SGMII code is actually correct though. Which
> advertisement are you extracting by reading the DEV_PCS1G_ANEG_CFG
> register and extracting DEV_PCS1G_ANEG_CFG_ADV_ABILITY_GET ? From the
> code in lan966x_port_pcs_set(), it suggests this is our advertisement,
> but it's supposed to always be the link partner's advertisement being
> passed to phylink_mii_c22_pcs_decode_state().
Actually, like you mentioned it needs to be link partner's advertisement
so that code can be simplified more:
if (DEV_PCS1G_ANEG_STATUS_ANEG_COMPLETE_GET(val)) {
state->an_complete = true;
bmsr |= state->link ? BMSR_LSTATUS : 0;
bmsr |= BMSR_ANEGCOMPLETE;
lp_adv = DEV_PCS1G_ANEG_STATUS_LP_ADV_GET(val);
phylink_mii_c22_pcs_decode_state(state, bmsr, lp_adv);
}
Because inside phylink_mii_c22_pcs_decode_state, more precisely in
phylink_decode_c37_work, state->advertising will have the local
advertising.
>
> > +int lan966x_port_pcs_set(struct lan966x_port *port,
> > + struct lan966x_port_config *config)
> > +{
> ...
> > + port->config = *config;
>
> As mentioned elsewhere, "config" won't have phy_mode set, so this clears
> port->config.phymode to PHY_INTERFACE_MODE_NA, which I think will cause
> e.g. lan966x_port_link_up() not to behave as intended.
Actually, the "config" will still have the phy_mode because config will
get teh value of port->config and after that some fields are changed.
>
> Thanks.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
--
/Horatiu
