Re: [PATCH net-next v3] net: dsa: mv88e6xxx: Add erratum 3.14 for 88E6390X and 88E6190X

[Ante Knezic]

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?

> > >  
> > >  	err = mv88e639x_pcs_setup_irq(mpcs, chip, port);
> > >  	if (err)
> > > @@ -873,6 +914,7 @@ static int mv88e6393x_pcs_init(struct mv88e6xxx_chip *chip, int port)
> > >  	mpcs->xg_pcs.ops = &mv88e6393x_xg_pcs_ops;
> > >  	mpcs->xg_pcs.neg_mode = true;
> > >  	mpcs->supports_5g = true;
> > > +	mpcs->chip = chip;
> 
> Presumably the 6393x isn't affected by this, so this is not necessary
> with the above changes.

This was done merely for consistency, besides the memory is already reserved, why not point
it to something? In case of bool replacement it will not matter anymore.

Thanks,
	Ante