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

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

Hello Sean,

On Sun, 9 Oct 2022 01:38:15 -0400
Sean Anderson <seanga2@gmail.com> wrote:


> > +#define   SGMII_PCS_LINK_TIMER_REG(x)		(0x12 + (x))  
> 
> Not used.

Right, I'll remove that in a followup patch

> > +#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)  
> 
> You can use FIELD_PREP if you're so inclined. I assume SGMI is from
> the datasheet.

Will do ! thanks :)

> > +#define   PCS_IF_MODE_SGMI_HALF_DUPLEX	BIT(4)
> > +#define   PCS_IF_MODE_SGMI_PHY_AN	BIT(5)
> > +#define SGMII_PCS_DIS_READ_TO	0x15
> > +#define SGMII_PCS_READ_TO	0x16
> > +#define SGMII_PCS_SW_RESET_TIMEOUT 100 /* usecs */
> > +
> > +struct altera_tse_pcs {
> > +	struct phylink_pcs pcs;
> > +	void __iomem *base;
> > +	int reg_width;
> > +};
> > +
> > +static struct altera_tse_pcs *phylink_pcs_to_tse_pcs(struct
> > phylink_pcs *pcs) +{
> > +	return container_of(pcs, struct altera_tse_pcs, pcs);
> > +}
> > +
> > +static u16 tse_pcs_read(struct altera_tse_pcs *tse_pcs, int regnum)
> > +{
> > +	if (tse_pcs->reg_width == 4)
> > +		return readl(tse_pcs->base + regnum * 4);
> > +	else
> > +		return readw(tse_pcs->base + regnum * 2);
> > +}
> > +
> > +static void tse_pcs_write(struct altera_tse_pcs *tse_pcs, int
> > regnum,
> > +			  u16 value)
> > +{
> > +	if (tse_pcs->reg_width == 4)
> > +		writel(value, tse_pcs->base + regnum * 4);
> > +	else
> > +		writew(value, tse_pcs->base + regnum * 2);
> > +}
> > +
> > +static int tse_pcs_reset(struct altera_tse_pcs *tse_pcs)
> > +{
> > +	int i = 0;
> > +	u16 bmcr;
> > +
> > +	/* Reset PCS block */
> > +	bmcr = tse_pcs_read(tse_pcs, MII_BMCR);
> > +	bmcr |= BMCR_RESET;
> > +	tse_pcs_write(tse_pcs, MII_BMCR, bmcr);
> > +
> > +	for (i = 0; i < SGMII_PCS_SW_RESET_TIMEOUT; i++) {
> > +		if (!(tse_pcs_read(tse_pcs, MII_BMCR) &
> > BMCR_RESET))
> > +			return 0;
> > +		udelay(1);
> > +	}  
> 
> read_poll_timeout?

Oh yeah definitely, I didn't know about this helper.

> > +
> > +	return -ETIMEDOUT;
> > +}
> > +
> > +static int alt_tse_pcs_validate(struct phylink_pcs *pcs,
> > +				unsigned long *supported,
> > +				const struct phylink_link_state
> > *state) +{
> > +	if (state->interface == PHY_INTERFACE_MODE_SGMII ||
> > +	    state->interface == PHY_INTERFACE_MODE_1000BASEX)
> > +		return 1;
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +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;
> > +
> > +	ctrl = tse_pcs_read(tse_pcs, MII_BMCR);
> > +	if_mode = tse_pcs_read(tse_pcs, SGMII_PCS_IF_MODE);
> > +
> > +	/* Set link timer to 1.6ms, as per the MegaCore Function
> > User Guide */
> > +	tse_pcs_write(tse_pcs, SGMII_PCS_LINK_TIMER_0, 0x0D40);
> > +	tse_pcs_write(tse_pcs, SGMII_PCS_LINK_TIMER_1, 0x03);  
> 
> Shouldn't this be different for SGMII vs 1000BASE-X?

I've dug a bit and indeed you're right. The value of 1.6ms works for
SGMII, but for 1000BaseX it should be set to 10ms. I'll send a fix for
this too.

> > +
> > +	if (interface == PHY_INTERFACE_MODE_SGMII) {
> > +		if_mode |= PCS_IF_MODE_USE_SGMII_AN |
> > PCS_IF_MODE_SGMII_ENA;  
> 
> I think PCS_IF_MODE_USE_SGMII_AN should be cleared if
> mode=MLO_AN_FIXED.

Correct.

> > +	} else if (interface == PHY_INTERFACE_MODE_1000BASEX) {
> > +		if_mode &= ~(PCS_IF_MODE_USE_SGMII_AN |
> > PCS_IF_MODE_SGMII_ENA);
> > +		if_mode |= PCS_IF_MODE_SGMI_SPEED_1000;  
> 
> I don't think you need to set this for 1000BASE-X.

You're correct too.

> > +	}
> > +
> > +	ctrl |= (BMCR_SPEED1000 | BMCR_FULLDPLX | BMCR_ANENABLE);  
> 
> BMCR_FULLDPLX is read-only, so you don't have to set it. Same for the
> speed.

Thanks, that's true

> > +
> > +	tse_pcs_write(tse_pcs, MII_BMCR, ctrl);
> > +	tse_pcs_write(tse_pcs, SGMII_PCS_IF_MODE, if_mode);
> > +
> > +	return tse_pcs_reset(tse_pcs);
> > +}
> > +
> > +static void alt_tse_pcs_get_state(struct phylink_pcs *pcs,
> > +				  struct phylink_link_state *state)
> > +{
> > +	struct altera_tse_pcs *tse_pcs =
> > phylink_pcs_to_tse_pcs(pcs);
> > +	u16 bmsr, lpa;
> > +
> > +	bmsr = tse_pcs_read(tse_pcs, MII_BMSR);
> > +	lpa = tse_pcs_read(tse_pcs, MII_LPA);
> > +
> > +	phylink_mii_c22_pcs_decode_state(state, bmsr, lpa);
> > +}
> > +
> > +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);
> > +
> > +	/* This PCS seems to require a soft reset to re-sync the
> > AN logic */
> > +	tse_pcs_reset(tse_pcs);  
> 
> This is kinda strange since c22 phys are supposed to reset the other
> registers to default values when BMCR_RESET is written. Good thing
> this is a PCS...

Indeed. This soft reset will not affect the register configuration, it
will only reset all internal state machines.

The datasheet actually recommends performing a reset after any
configuration change...

That's one thing with this IP, it tries to re-use the C22 register
layout but it's not fully consistent with it...

> > +}
> > +
> > +static const struct phylink_pcs_ops alt_tse_pcs_ops = {
> > +	.pcs_validate = alt_tse_pcs_validate,
> > +	.pcs_get_state = alt_tse_pcs_get_state,
> > +	.pcs_config = alt_tse_pcs_config,
> > +	.pcs_an_restart = alt_tse_pcs_an_restart,
> > +};  
> 
> Don't you need link_up to set the speed/duplex for MLO_AN_FIXED?

I'll give it a test and confirm it

> > +
> > +struct phylink_pcs *alt_tse_pcs_create(struct net_device *ndev,
> > +				       void __iomem *pcs_base, int
> > reg_width) +{
> > +	struct altera_tse_pcs *tse_pcs;
> > +
> > +	if (reg_width != 4 && reg_width != 2)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	tse_pcs = devm_kzalloc(&ndev->dev, sizeof(*tse_pcs),
> > GFP_KERNEL);
> > +	if (!tse_pcs)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	tse_pcs->pcs.ops = &alt_tse_pcs_ops;
> > +	tse_pcs->base = pcs_base;
> > +	tse_pcs->reg_width = reg_width;
> > +
> > +	return &tse_pcs->pcs;
> > +}
> > +EXPORT_SYMBOL_GPL(alt_tse_pcs_create);
> > diff --git a/include/linux/pcs-altera-tse.h
> > b/include/linux/pcs-altera-tse.h new file mode 100644
> > index 000000000000..92ab9f08e835
> > --- /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_pcs;
> > +struct net_device;
> > +
> > +struct phylink_pcs *alt_tse_pcs_create(struct net_device *ndev,
> > +				       void __iomem *pcs_base, int
> > reg_width); +
> > +#endif /* __LINUX_PCS_ALTERA_TSE_H */  
> 
> --Sean

Thanks a lot for the review ! I'll do a round of tests with the
comments and send follow-up patches.

Best regards,

Maxime