Re: [PATCH net-next v3 3/6] net: lan966x: add port module support

[Horatiu Vultur]

The 11/24/2021 15:03, Russell King (Oracle) wrote:
> 
> On Wed, Nov 24, 2021 at 03:58:00PM +0100, Horatiu Vultur wrote:
> > > 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?
> 
> Depends if you need them disabled before you do the teardown. However,
> what would be the effect of disabling interrupts while the user still
> has the ability to interact with the port - that is the main point.
> 
> Generally the teardown should be the reverse of setup - where it's now
> accepted that all setup should be done prior to user publication. So,
> user interfaces should be removed and then teardown should proceed.

Yes, I get your point. I will remove the interface and then I will
disable the interrupts.

> 
> > > 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'.
> 
> Ah, missed that, thanks. However, why should portmode and phy_mode be
> different?

Because the serdes knows only few modes(QSGMII, SGMII, GMII) and this
information will come from DT. So I would like to have one variable that
will configure the serdes ('phy_mode') and one will configure the MAC
('portmode').

> 
> > 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.
> 
> Correct.
> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

-- 
/Horatiu