Message-ID: <20211124145800.my4niep3sifqpg55@soft-dev3-1.localhost>
Date:   Wed, 24 Nov 2021 15:58:00 +0100
From:   Horatiu Vultur <horatiu.vultur@...rochip.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
CC:     <davem@...emloft.net>, <kuba@...nel.org>, <robh+dt@...nel.org>,
        <UNGLinuxDriver@...rochip.com>, <p.zabel@...gutronix.de>,
        <netdev@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v3 3/6] net: lan966x: add port module support

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

Powered by blists - more mailing lists