Re: [PATCH net-next 3/5] net: pcs: add new PCS driver for altera TSE PCS

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hello Russell,

On Fri, 26 Aug 2022 17:45:13 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Fri, Aug 26, 2022 at 03:54:49PM +0200, Maxime Chevallier wrote:
> > +
> > +/* SGMII PCS register addresses
> > + */
> > +#define SGMII_PCS_SCRATCH	0x10
> > +#define SGMII_PCS_REV		0x11
> > +#define SGMII_PCS_LINK_TIMER_0	0x12
> > +#define   SGMII_PCS_LINK_TIMER_REG(x)		(0x12 + (x))
> > +#define SGMII_PCS_LINK_TIMER_1	0x13
> > +#define SGMII_PCS_IF_MODE	0x14
> > +#define   PCS_IF_MODE_SGMII_ENA		BIT(0)
> > +#define   PCS_IF_MODE_USE_SGMII_AN	BIT(1)
> > +#define   PCS_IF_MODE_SGMI_SPEED_MASK	GENMASK(3, 2)
> > +#define   PCS_IF_MODE_SGMI_SPEED_10	(0 << 2)
> > +#define   PCS_IF_MODE_SGMI_SPEED_100	(1 << 2)
> > +#define   PCS_IF_MODE_SGMI_SPEED_1000	(2 << 2)
> > +#define   PCS_IF_MODE_SGMI_HALF_DUPLEX	BIT(4)
> > +#define   PCS_IF_MODE_SGMI_PHY_AN	BIT(5)  
> 
> This looks very similar to pcs-lynx's register layout. I wonder if
> it's the same underlying hardware.

I've also looked, and indeed the layout is very smiliar. The key
differences would be that the TSE PCS is limited to 1G max and 8/10bit
encoding, whereas the lynx seems to support BaseR and higher speeds.

> > +static int alt_tse_pcs_config(struct phylink_pcs *pcs, unsigned
> > int mode,
> > +			      phy_interface_t interface,
> > +			      const unsigned long *advertising,
> > +			      bool permit_pause_to_mac)
> > +{
> > +	struct altera_tse_pcs *tse_pcs =
> > phylink_pcs_to_tse_pcs(pcs);
> > +	u32 ctrl, if_mode;
> > +
> > +	if (interface != PHY_INTERFACE_MODE_SGMII &&
> > +	    interface != PHY_INTERFACE_MODE_1000BASEX)
> > +		return 0;  
> 
> I would suggest doing this check in .pcs_validate() to catch anyone
> attaching the PCS with an unsupported interface mode.

I'll add it, thanks.

> > +static void alt_tse_pcs_an_restart(struct phylink_pcs *pcs)
> > +{
> > +	struct altera_tse_pcs *tse_pcs =
> > phylink_pcs_to_tse_pcs(pcs);
> > +	u16 bmcr;
> > +
> > +	bmcr = tse_pcs_read(tse_pcs, MII_BMCR);
> > +	bmcr |= BMCR_ANRESTART;
> > +	tse_pcs_write(tse_pcs, MII_BMCR, bmcr);
> > +
> > +	tse_pcs_reset(tse_pcs);  
> 
> Any ideas why a reset is necessary after setting BMCR_ANRESTART?
> Normally, this is not required.

From my tests, this is something this block needs :/ A soft reset on
this PCS will reset the comma detection and encoding logic, and the
testing I've done needs that for proper autoneg, especially when we
switch back and forth between SGMII/1000BaseX.

The TSE PCS support in dwmac-socfpga also does it, and this was
developped independently to this driver, so it looks like folks who
worked on that also found the same behaviour...

This might be worth adding a comment then :)

> > diff --git a/include/linux/pcs-altera-tse.h
> > b/include/linux/pcs-altera-tse.h new file mode 100644
> > index 000000000000..9c85e7c8ef70
> > --- /dev/null
> > +++ b/include/linux/pcs-altera-tse.h
> > @@ -0,0 +1,17 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2022 Bootlin
> > + *
> > + * Maxime Chevallier <maxime.chevallier@bootlin.com>
> > + */
> > +
> > +#ifndef __LINUX_PCS_ALTERA_TSE_H
> > +#define __LINUX_PCS_ALTERA_TSE_H
> > +
> > +struct phylink;  
> 
> Don't you want "struct phylink_pcs;" here?

Oh yes indeed, I this this whole line isn't necessary at all as a
matter of fact :/

Thanks for the review,

Maxime