Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 26 Jul 2023 10:53:17 +0100
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Ante Knezic <ante.knezic@helmholz.de>
Cc:     andrew@lunn.ch, davem@davemloft.net, edumazet@google.com,
        f.fainelli@gmail.com, kuba@kernel.org,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        olteanv@gmail.com, pabeni@redhat.com
Subject: Re: [PATCH net-next v3] net: dsa: mv88e6xxx: Add erratum 3.14 for
 88E6390X and 88E6190X
Message-ID: <ZMDtDXG4Xj94F7vw@shell.armlinux.org.uk>
References: <ZMALH03Fbp3wKkO2@shell.armlinux.org.uk>
 <20230726094935.12629-1-ante.knezic@helmholz.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230726094935.12629-1-ante.knezic@helmholz.de>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Wed, Jul 26, 2023 at 11:49:35AM +0200, Ante Knezic wrote:
> On Tue, 25 Jul 2023 18:49:19 +0100 Russell King (Oracle) wrote:
> > Does the errata say that _all_ lanes need this treatment, even when
> > they are not being used as a group (e.g. for XAUI) ?
> 
> No, unfortunatelly errata says very little, I tried applying erratum only on the requested 
> lane of port 9/10 but this did not work out as expected and the issue was still visible.
> I dont have the necessary HW to perform more tests on other lanes unfortunatelly.
> 
> On Tue, 25 Jul 2023 18:49:19 +0100 Russell King (Oracle) wrote:
> > On Tue, Jul 25, 2023 at 08:23:43PM +0300, Vladimir Oltean wrote:
> > > On Fri, Jul 21, 2023 at 12:26:18PM +0200, Ante Knezic wrote:
> > > > diff --git a/drivers/net/dsa/mv88e6xxx/pcs-639x.c b/drivers/net/dsa/mv88e6xxx/pcs-639x.c
> > > > index 98dd49dac421..50b14804c360 100644
> > > > --- a/drivers/net/dsa/mv88e6xxx/pcs-639x.c
> > > > +++ b/drivers/net/dsa/mv88e6xxx/pcs-639x.c
> > > > @@ -20,6 +20,7 @@ struct mv88e639x_pcs {
> > > >  	struct mdio_device mdio;
> > > >  	struct phylink_pcs sgmii_pcs;
> > > >  	struct phylink_pcs xg_pcs;
> > > > +	struct mv88e6xxx_chip *chip;
> > 
> > 	bool erratum_3_14;
> 
> ...
> 
> > > >  static int mv88e639x_sgmii_pcs_post_config(struct phylink_pcs *pcs,
> > > >  					   phy_interface_t interface)
> > > >  {
> > > >  	struct mv88e639x_pcs *mpcs = sgmii_pcs_to_mv88e639x_pcs(pcs);
> > > > +	struct mv88e6xxx_chip *chip = mpcs->chip;
> > > >  
> > > >  	mv88e639x_sgmii_pcs_control_pwr(mpcs, true);
> > > >  
> > > > +	if (chip->info->prod_num == MV88E6XXX_PORT_SWITCH_ID_PROD_6190X ||
> > > > +	    chip->info->prod_num == MV88E6XXX_PORT_SWITCH_ID_PROD_6390X)
> > > > +		mv88e6390_erratum_3_14(mpcs);
> > 
> > 	int err;
> > ...
> > 	if (mpcs->erratum_3_14) {
> > 		err = mv88e6390_erratum_3_14(mpcs);
> > 		if (err)
> > 			dev_err(mpcs->mdio.dev.parent,
> > 				"failed to apply erratum 3.14: %pe\n",
> > 				ERR_PTR(err));
> > 	}
> > 
> 
> So you propose to ditch the chip ptr from the mpcs and add a bool variable instead. But
> isn't this too general - the errata applies only to 6190X and 6390X, other devices
> might (and probably do) have errata 3.14 as something completely different? Possible new changes
> (new errata, fixes etc) in the pcs-xxx.c might benefit from having a chip ptr more than 
> using a bool variable "just" for one errata found on two device types?

As a longer term goal, I would like to move the pcs drivers out of
mv88e6xxx and into drivers/net/pcs, so I want to minimise the use of
the "chip" pointer in the drivers. That's why I coded them the way I
have, as almost entirely stand-alone implementations that make no use
of the hardware accessors provided by the 88e6xxx core.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!