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:   Tue, 25 Jul 2023 20:23:43 +0300
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Ante Knezic <ante.knezic@helmholz.de>
Cc:     netdev@vger.kernel.org, andrew@lunn.ch, f.fainelli@gmail.com,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, linux-kernel@vger.kernel.org,
        Russell King <linux@armlinux.org.uk>
Subject: Re: [PATCH net-next v3] net: dsa: mv88e6xxx: Add erratum 3.14 for
 88E6390X and 88E6190X
Message-ID: <20230725172343.qcqmcoygyhcgunmh@skbuf>
References: <20230721102618.13408-1-ante.knezic@helmholz.de>
 <20230721102618.13408-1-ante.knezic@helmholz.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230721102618.13408-1-ante.knezic@helmholz.de>
 <20230721102618.13408-1-ante.knezic@helmholz.de>
Precedence: bulk

On Fri, Jul 21, 2023 at 12:26:18PM +0200, Ante Knezic wrote:
> Fixes XAUI/RXAUI lane alignment errors.
> Issue causes dropped packets when trying to communicate over
> fiber via SERDES lanes of port 9 and 10.
> Errata document applies only to 88E6190X and 88E6390X devices.
> Requires poking in undocumented registers.
> 
> Signed-off-by: Ante Knezic <ante.knezic@helmholz.de>
> ---
> V3 : Rework to fit the new phylink_pcs infrastructure
> V2 : Rework as suggested by Andrew Lunn <andrew@lun.ch> 
>  * make int lanes[] const 
>  * reorder prod_nums
>  * update commit message to indicate we are dealing with
>    undocumented Marvell registers and magic values
> ---
>  drivers/net/dsa/mv88e6xxx/pcs-639x.c | 42 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> 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 supports_5g;
>  	phy_interface_t interface;
>  	unsigned int irq;
> @@ -205,13 +206,52 @@ static void mv88e639x_sgmii_pcs_pre_config(struct phylink_pcs *pcs,
>  	mv88e639x_sgmii_pcs_control_pwr(mpcs, false);
>  }
>  
> +static int mv88e6390_erratum_3_14(struct mv88e639x_pcs *mpcs)
> +{
> +	const int lanes[] = { MV88E6390_PORT9_LANE0, MV88E6390_PORT9_LANE1,
> +		MV88E6390_PORT9_LANE2, MV88E6390_PORT9_LANE3,
> +		MV88E6390_PORT10_LANE0, MV88E6390_PORT10_LANE1,
> +		MV88E6390_PORT10_LANE2, MV88E6390_PORT10_LANE3 };
> +	struct mdio_device mdio;
> +	int err, i;
> +
> +	/* 88e6190x and 88e6390x errata 3.14:
> +	 * After chip reset, SERDES reconfiguration or SERDES core
> +	 * Software Reset, the SERDES lanes may not be properly aligned
> +	 * resulting in CRC errors
> +	 */
> +
> +	mdio.bus = mpcs->mdio.bus;
> +
> +	for (i = 0; i < ARRAY_SIZE(lanes); i++) {
> +		mdio.addr = lanes[i];
> +
> +		err = mdiodev_c45_write(&mdio, MDIO_MMD_PHYXS,
> +					0xf054, 0x400C);
> +		if (err)
> +			return err;
> +
> +		err = mdiodev_c45_write(&mdio, MDIO_MMD_PHYXS,
> +					0xf054, 0x4000);
> +		if (err)
> +			return err;

I'm not sure which way is preferred by PHY maintainers, but it seems to
be a useless complication to simulate that you have a struct mdio_device
for the other lanes when you don't. It appears more appropriate to just
use mdiobus_c45_write(mpcs->mdio.bus, lanes[i]).

There's also the locking question (with the big caveat that we don't
know what the register writes do!). There's locking at the bus level,
but the MDIO device isn't locked. So phylink on those other PCSes can
still do stuff, even in-between the first and the second write to
undocumented register 0xf054.

I can speculate that writing 0x400c -> 0x4000 is something like: set
RX_RESET | TX_RESET followed by clear RX_RESET | TX_RESET. Is it ok if
stuff happens in between these writes - will it stick, or does this
logically interact with anything else in any other way? I guess we won't
know. I might be a bit closer to being okay with it if you could confirm
that some other (unrelated) register write to the PCS does make it
through (and can be read back) in between the 2 erratum writes.

> +	}
> +
> +	return 0;
> +}
> +
>  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);

You could at least print an error if a write failure occurred, so that
it doesn't go completely unnoticed.

> +
>  	return 0;
>  }
>  
> @@ -523,6 +563,7 @@ static int mv88e6390_pcs_init(struct mv88e6xxx_chip *chip, int port)
>  	mpcs->sgmii_pcs.neg_mode = true;
>  	mpcs->xg_pcs.ops = &mv88e6390_xg_pcs_ops;
>  	mpcs->xg_pcs.neg_mode = true;
> +	mpcs->chip = chip;
>  
>  	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;
>  
>  	err = mv88e6393x_erratum_4_6(mpcs);
>  	if (err)
> -- 
> 2.11.0
>