IronPort-SDR: LPRhALNPyeBcxVcVAqU8wnDhlrHHoUIz7xMWn7oiC1BEh0hEmPjmIH62nQNBP2yJI9s1z//E1u
 m39YbqNXLADrEeBqjUNhwLv3EqNoAc8N+NJ0SKYsLjGBfaE8cOIlVaXoKy3AjIjMBb/WNAKBmo
 mFdyJrIAj1eJ/MsEatXBCnCjhaKoMMdGUNmqBYaB38Tbd69IRV8Hp6dXmndFH89fFoLHz4CQ91
 jmLyz+IFnKqacmgVuaBxXQkvMvCyjTfJQIcwTXIpfDRi6Tr8Dxct8Tyc2e9irLXmIZ4r0PSX4P
 lbuNtQg0Gym2d3AkDSinzIVe
Date:   Wed, 24 Nov 2021 16:43:23 +0100
From:   Horatiu Vultur <horatiu.vultur@microchip.com>
To:     "Russell King (Oracle)" <linux@armlinux.org.uk>
CC:     <davem@davemloft.net>, <kuba@kernel.org>, <robh+dt@kernel.org>,
        <UNGLinuxDriver@microchip.com>, <p.zabel@pengutronix.de>,
        <netdev@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net-next v3 3/6] net: lan966x: add port module support
Message-ID: <20211124154323.44liimrwzthsh547@soft-dev3-1.localhost>
References: <20211124083915.2223065-1-horatiu.vultur@microchip.com>
 <20211124083915.2223065-4-horatiu.vultur@microchip.com>
 <YZ4SB/wX6UT3zrEV@shell.armlinux.org.uk>
 <20211124145800.my4niep3sifqpg55@soft-dev3-1.localhost>
 <YZ5UXdiNNf011skU@shell.armlinux.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <YZ5UXdiNNf011skU@shell.armlinux.org.uk>
Precedence: bulk

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