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

Commit Message

Maxime Chevallier Sept. 1, 2022, 2:35 p.m. UTC

The Altera Triple Speed Ethernet has a SGMII/1000BaseC PCS that can be
integrated in several ways. It can either be part of the TSE MAC's
address space, accessed through 32 bits accesses on the mapped mdio
device 0, or through a dedicated 16 bits register set.

This driver allows using the TSE PCS outside of altera TSE's driver,
since it can be used standalone by other MACs.

Signed-off-by: Maxime Chevallier <[e-mail]>
---
V2->V3 :
  - No changes
V1->V2 :
  - Added a pcs_validate() callback to filter interface modes
  - Added comments on the need for a soft reset at an_restart

 MAINTAINERS                      |   7 ++
 drivers/net/pcs/Kconfig          |   6 ++
 drivers/net/pcs/Makefile         |   1 +
 drivers/net/pcs/pcs-altera-tse.c | 171 +++++++++++++++++++++++++++++++
 include/linux/pcs-altera-tse.h   |  17 +++
 5 files changed, 202 insertions(+)
 create mode 100644 drivers/net/pcs/pcs-altera-tse.c
 create mode 100644 include/linux/pcs-altera-tse.h

Comments

Sean Anderson Oct. 9, 2022, 5:38 a.m. UTC | #1

On 9/1/22 10:35, Maxime Chevallier wrote:
> The Altera Triple Speed Ethernet has a SGMII/1000BaseC PCS that can be
> integrated in several ways. It can either be part of the TSE MAC's
> address space, accessed through 32 bits accesses on the mapped mdio
> device 0, or through a dedicated 16 bits register set.
> 
> This driver allows using the TSE PCS outside of altera TSE's driver,
> since it can be used standalone by other MACs.
> 
> Signed-off-by: Maxime Chevallier <[e-mail]>
> ---
> V2->V3 :
>    - No changes
> V1->V2 :
>    - Added a pcs_validate() callback to filter interface modes
>    - Added comments on the need for a soft reset at an_restart
> 
>   MAINTAINERS                      |   7 ++
>   drivers/net/pcs/Kconfig          |   6 ++
>   drivers/net/pcs/Makefile         |   1 +
>   drivers/net/pcs/pcs-altera-tse.c | 171 +++++++++++++++++++++++++++++++
>   include/linux/pcs-altera-tse.h   |  17 +++
>   5 files changed, 202 insertions(+)
>   create mode 100644 drivers/net/pcs/pcs-altera-tse.c
>   create mode 100644 include/linux/pcs-altera-tse.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fe484e7d36dc..576c01576a5f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -878,6 +878,13 @@ L:	[e-mail]
>   S:	Maintained
>   F:	drivers/net/ethernet/altera/
>   
> +ALTERA TSE PCS
> +M:	Maxime Chevallier <[e-mail]>
> +L:	[e-mail]
> +S:	Supported
> +F:	drivers/net/pcs/pcs-altera-tse.c
> +F:	include/linux/pcs-altera-tse.h
> +
>   ALTERA UART/JTAG UART SERIAL DRIVERS
>   M:	Tobias Klauser <[e-mail]>
>   L:	[e-mail]
> diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
> index 6289b7c765f1..6e7e6c346a3e 100644
> --- a/drivers/net/pcs/Kconfig
> +++ b/drivers/net/pcs/Kconfig
> @@ -26,4 +26,10 @@ config PCS_RZN1_MIIC
>   	  on RZ/N1 SoCs. This PCS converts MII to RMII/RGMII or can be set in
>   	  pass-through mode for MII.
>   
> +config PCS_ALTERA_TSE
> +	tristate
> +	help
> +	  This module provides helper functions for the Altera Triple Speed
> +	  Ethernet SGMII PCS, that can be found on the Intel Socfpga family.
> +
>   endmenu
> diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
> index 0ff5388fcdea..4c780d8f2e98 100644
> --- a/drivers/net/pcs/Makefile
> +++ b/drivers/net/pcs/Makefile
> @@ -6,3 +6,4 @@ pcs_xpcs-$(CONFIG_PCS_XPCS)	:= pcs-xpcs.o pcs-xpcs-nxp.o
>   obj-$(CONFIG_PCS_XPCS)		+= pcs_xpcs.o
>   obj-$(CONFIG_PCS_LYNX)		+= pcs-lynx.o
>   obj-$(CONFIG_PCS_RZN1_MIIC)	+= pcs-rzn1-miic.o
> +obj-$(CONFIG_PCS_ALTERA_TSE)	+= pcs-altera-tse.o
> diff --git a/drivers/net/pcs/pcs-altera-tse.c b/drivers/net/pcs/pcs-altera-tse.c
> new file mode 100644
> index 000000000000..ae7688ffc4d7
> --- /dev/null
> +++ b/drivers/net/pcs/pcs-altera-tse.c
> @@ -0,0 +1,171 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2022 Bootlin
> + *
> + * Maxime Chevallier <[e-mail]>
> + */
> +
> +#include <linux/netdevice.h>
> +#include <linux/phy.h>
> +#include <linux/phylink.h>
> +#include <linux/pcs-altera-tse.h>
> +
> +/* 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))

Not used.

> +#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.

> +#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?

> +
> +	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?

> +
> +	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.

> +	} 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.

> +	}
> +
> +	ctrl |= (BMCR_SPEED1000 | BMCR_FULLDPLX | BMCR_ANENABLE);

BMCR_FULLDPLX is read-only, so you don't have to set it. Same for the
speed.

> +
> +	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...

> +}
> +
> +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?

> +
> +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 <[e-mail]>
> + */
> +
> +#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

Maxime Chevallier Oct. 26, 2022, 9:37 a.m. UTC | #2

Hello Sean,

On Sun, 9 Oct 2022 01:38:15 -0400
Sean Anderson <[e-mail]> 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 <[e-mail]>
> > + */
> > +
> > +#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

Andrew Lunn Oct. 26, 2022, 12:05 p.m. UTC | #3

> > > +	/* 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...

The Marvell PHYs work like this. Many of its registers won't take
effect until you do a soft reset. I think the thinking behind this is
that changing many registers is disruptive to the link and slow. It
takes over a second to perform auto-neg etc. So ideally you want to
make all your register changes, and then trigger them into operation.
And a soft reset is this trigger.

    Andrew

Russell King (Oracle) Oct. 26, 2022, 12:47 p.m. UTC | #4

On Wed, Oct 26, 2022 at 11:37:11AM +0200, Maxime Chevallier wrote:
> Hello Sean,
> 
> On Sun, 9 Oct 2022 01:38:15 -0400
> Sean Anderson <[e-mail]> 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_ANi	BIT(5)

The definitions up to here look very similar to pcs-lynx.c when it comes
to 1000base-X and SGMII. I wonder whether regmap can help here to
abstract the register access differences and then maybe code can be
shared.

What value is in registers 2 and 3 (the ID registers) for this PCS?

On the link timer value setting, I have a patch to add a phylink helper
which returns the link timer in nanoseconds. May be a good idea if we
get that queued up so drivers can make use of it rather than hard-coding
a register value everywhere.

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index fe484e7d36dc..576c01576a5f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -878,6 +878,13 @@  L:	[e-mail]
 S:	Maintained
 F:	drivers/net/ethernet/altera/
 
+ALTERA TSE PCS
+M:	Maxime Chevallier <[e-mail]>
+L:	[e-mail]
+S:	Supported
+F:	drivers/net/pcs/pcs-altera-tse.c
+F:	include/linux/pcs-altera-tse.h
+
 ALTERA UART/JTAG UART SERIAL DRIVERS
 M:	Tobias Klauser <[e-mail]>
 L:	[e-mail]
diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index 6289b7c765f1..6e7e6c346a3e 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -26,4 +26,10 @@  config PCS_RZN1_MIIC
 	  on RZ/N1 SoCs. This PCS converts MII to RMII/RGMII or can be set in
 	  pass-through mode for MII.
 
+config PCS_ALTERA_TSE
+	tristate
+	help
+	  This module provides helper functions for the Altera Triple Speed
+	  Ethernet SGMII PCS, that can be found on the Intel Socfpga family.
+
 endmenu
diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
index 0ff5388fcdea..4c780d8f2e98 100644
--- a/drivers/net/pcs/Makefile
+++ b/drivers/net/pcs/Makefile
@@ -6,3 +6,4 @@  pcs_xpcs-$(CONFIG_PCS_XPCS)	:= pcs-xpcs.o pcs-xpcs-nxp.o
 obj-$(CONFIG_PCS_XPCS)		+= pcs_xpcs.o
 obj-$(CONFIG_PCS_LYNX)		+= pcs-lynx.o
 obj-$(CONFIG_PCS_RZN1_MIIC)	+= pcs-rzn1-miic.o
+obj-$(CONFIG_PCS_ALTERA_TSE)	+= pcs-altera-tse.o
diff --git a/drivers/net/pcs/pcs-altera-tse.c b/drivers/net/pcs/pcs-altera-tse.c
new file mode 100644
index 000000000000..ae7688ffc4d7
--- /dev/null
+++ b/drivers/net/pcs/pcs-altera-tse.c
@@ -0,0 +1,171 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022 Bootlin
+ *
+ * Maxime Chevallier <[e-mail]>
+ */
+
+#include <linux/netdevice.h>
+#include <linux/phy.h>
+#include <linux/phylink.h>
+#include <linux/pcs-altera-tse.h>
+
+/* 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)
+#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);
+	}
+
+	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);
+
+	if (interface == PHY_INTERFACE_MODE_SGMII) {
+		if_mode |= PCS_IF_MODE_USE_SGMII_AN | PCS_IF_MODE_SGMII_ENA;
+	} 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;
+	}
+
+	ctrl |= (BMCR_SPEED1000 | BMCR_FULLDPLX | BMCR_ANENABLE);
+
+	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);
+}
+
+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,
+};
+
+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 <[e-mail]>
+ */
+
+#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 */