Re: [RFC 7/7] net: pcs: Drop the TSE PCS driver

From: Maxime Chevallier <maxime.chevallier@bootlin.com>
To: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: Mark Brown <broonie@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	rafael@kernel.org, Colin Foster <colin.foster@in-advantage.com>,
	Lee Jones <lee@kernel.org>,
	davem@davemloft.net, Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Russell King <linux@armlinux.org.uk>,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	thomas.petazzoni@bootlin.com
Subject: Re: [RFC 7/7] net: pcs: Drop the TSE PCS driver
Date: Mon, 22 May 2023 08:26:29 +0200	[thread overview]
Message-ID: <20230522082629.7fe93534@pc-7.home> (raw)
In-Reply-To: <20230515114809.d4jzhiazymfqajbj@skbuf>

Hello Vlad,

On Mon, 15 May 2023 14:48:09 +0300
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> Hi Maxime,
> 
> On Fri, Mar 24, 2023 at 10:36:44AM +0100, Maxime Chevallier wrote:
> > Now that we can easily create a mdio-device that represents a
> > memory-mapped device that exposes an MDIO-like register layout, we
> > don't need the Altera TSE PCS anymore, since we can use the Lynx
> > PCS instead.
> > 
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > ---
> > -static int tse_pcs_reset(struct altera_tse_pcs *tse_pcs)
> > -{
> > -	u16 bmcr;
> > -
> > -	/* Reset PCS block */
> > -	bmcr = tse_pcs_read(tse_pcs, MII_BMCR);
> > -	bmcr |= BMCR_RESET;
> > -	tse_pcs_write(tse_pcs, MII_BMCR, bmcr);
> > -
> > -	return read_poll_timeout(tse_pcs_read, bmcr, (bmcr &
> > BMCR_RESET),
> > -				 10, SGMII_PCS_SW_RESET_TIMEOUT, 1,
> > -				 tse_pcs, MII_BMCR);
> > -}  
> 
> I just noticed this difference between the Lynx PCS and Altera TSE PCS
> drivers. The Lynx driver doesn't reset the PCS, and if it did, it
> would wait until the opposite condition from yours would be true:
> BMCR_RESET should clear: "!(bmcr & BMCR_RESET)".
> 
> Is your reset procedure correct, I wonder?

I also wonder... My understanding from the documentation I had is that
the PCS needed to be reset every time a major configuration was
changed, as the reset would put the internal state machines back to
their init condition, but without resetting the whole IP.
When performing my initial tests, this was indeed necessary. However
when porting to Lynx and running tests, it pretty much worked out of the
box, and I'm quite happy with it being that way. So I didn't dig much
deeper on why the reset was needed with the TSE PCS in the first place.

On a side note, the final dependencies for the final removal of TSE
were merged into the regmap tree, I'll therefore send a new version of
that series shortly.

Thanks for the review Vlad,

Best Regards,

Maxime