Hello Russell,
On Fri, 26 Aug 2022 17:45:13 +0100
"Russell King (Oracle)" <linux@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxx>
> > + */
> > +
> > +#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
