diff mbox series

[net-next] net: pcs: tse: port to pcs-lynx

Message ID 20230210190949.1115836-1-maxime.chevallier@bootlin.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next] net: pcs: tse: port to pcs-lynx | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 0 this patch: 5
netdev/cc_maintainers warning 1 maintainers not CCed: joyce.ooi@intel.com
netdev/build_clang fail Errors and warnings before: 0 this patch: 5
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 0 this patch: 5
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Maxime Chevallier Feb. 10, 2023, 7:09 p.m. UTC 
When submitting the initial driver for the Altera TSE PCS, Russell King
noted that the register layout for the TSE PCS is very similar to the
Lynx PCS. The main difference being that TSE PCS's register space is
memory-mapped, whereas Lynx's is exposed over MDIO.

Convert the TSE PCS to reuse the whole logic from Lynx, by allowing
the creation of a dummy MDIO bus, and a dummy MDIO device located at
address 0 on that bus. The MAC driver that uses this PCS must provide
callbacks to read/write the MMIO.

Also convert the Altera TSE MAC driver to this new way of using the TSE
PCS.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/ethernet/altera/altera_tse.h      |   2 +-
 drivers/net/ethernet/altera/altera_tse_main.c |  50 ++++-
 drivers/net/pcs/Kconfig                       |   4 +
 drivers/net/pcs/pcs-altera-tse.c              | 194 +++++++-----------
 include/linux/pcs-altera-tse.h                |  22 +-
 5 files changed, 142 insertions(+), 130 deletions(-)

Comments

Vladimir Oltean Feb. 10, 2023, 7:31 p.m. UTC | #1 
On Fri, Feb 10, 2023 at 08:09:49PM +0100, Maxime Chevallier wrote:
> When submitting the initial driver for the Altera TSE PCS, Russell King
> noted that the register layout for the TSE PCS is very similar to the
> Lynx PCS. The main difference being that TSE PCS's register space is
> memory-mapped, whereas Lynx's is exposed over MDIO.
> 
> Convert the TSE PCS to reuse the whole logic from Lynx, by allowing
> the creation of a dummy MDIO bus, and a dummy MDIO device located at
> address 0 on that bus. The MAC driver that uses this PCS must provide
> callbacks to read/write the MMIO.
> 
> Also convert the Altera TSE MAC driver to this new way of using the TSE
> PCS.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
>  drivers/net/ethernet/altera/altera_tse.h      |   2 +-
>  drivers/net/ethernet/altera/altera_tse_main.c |  50 ++++-
>  drivers/net/pcs/Kconfig                       |   4 +
>  drivers/net/pcs/pcs-altera-tse.c              | 194 +++++++-----------
>  include/linux/pcs-altera-tse.h                |  22 +-
>  5 files changed, 142 insertions(+), 130 deletions(-)

The glue layer is larger than the duplicated PCS code? :(

> 
> diff --git a/drivers/net/ethernet/altera/altera_tse.h b/drivers/net/ethernet/altera/altera_tse.h
> index db5eed06e92d..f4e3fddb639a 100644
> --- a/drivers/net/ethernet/altera/altera_tse.h
> +++ b/drivers/net/ethernet/altera/altera_tse.h
> @@ -476,7 +476,7 @@ struct altera_tse_private {
>  
>  	struct phylink *phylink;
>  	struct phylink_config phylink_config;
> -	struct phylink_pcs *pcs;
> +	struct altera_tse_pcs *pcs;
>  };
>  
>  /* Function prototypes
> diff --git a/drivers/net/ethernet/altera/altera_tse_main.c b/drivers/net/ethernet/altera/altera_tse_main.c
> index 66e3af73ec41..109b7ed90c6e 100644
> --- a/drivers/net/ethernet/altera/altera_tse_main.c
> +++ b/drivers/net/ethernet/altera/altera_tse_main.c
> @@ -87,6 +87,36 @@ static inline u32 tse_tx_avail(struct altera_tse_private *priv)
>  	return priv->tx_cons + priv->tx_ring_size - priv->tx_prod - 1;
>  }
>  
> +static int altera_tse_pcs_read(void *priv, int regnum)
> +{
> +	struct altera_tse_private *tse = priv;
> +
> +	if (tse->pcs_base)
> +		return readw(tse->pcs_base + (regnum * 2));
> +	else
> +		return readl(tse->mac_dev + tse_csroffs(mdio_phy0) +
> +			     (regnum * 4));
> +	return 0;

code after return

Usual practice to avoid this is

	if (tse->pcs_base)
		return readw(tse->pcs_base + regnum * 2);

	return readl(tse->mac_dev + tse_csroffs(mdio_phy0) + regnum * 4);

also, multiplication has higher operator precedence over addition, so ()
not needed.

> +}
> +
> +static int altera_tse_pcs_write(void *priv, int regnum, u16 value)
> +{
> +	struct altera_tse_private *tse = priv;
> +
> +	if (tse->pcs_base)
> +		writew(value, tse->pcs_base + (regnum * 2));
> +	else
> +		writel(value, tse->mac_dev + tse_csroffs(mdio_phy0) +
> +			(regnum * 4));
> +
> +	return 0;
> +}
> +
> +static struct altera_tse_pcs_ops tse_ops = {
> +	.read = altera_tse_pcs_read,
> +	.write = altera_tse_pcs_write,
> +};
> +
>  /* MDIO specific functions
>   */
>  static int altera_tse_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
> @@ -1090,7 +1120,7 @@ static struct phylink_pcs *alt_tse_select_pcs(struct phylink_config *config,
>  
>  	if (interface == PHY_INTERFACE_MODE_SGMII ||
>  	    interface == PHY_INTERFACE_MODE_1000BASEX)
> -		return priv->pcs;
> +		return altera_tse_pcs_to_phylink_pcs(priv->pcs);
>  	else
>  		return NULL;
>  }
> @@ -1143,7 +1173,6 @@ static int altera_tse_probe(struct platform_device *pdev)
>  	struct resource *pcs_res;
>  	struct net_device *ndev;
>  	void __iomem *descmap;
> -	int pcs_reg_width = 2;
>  	int ret = -ENODEV;
>  
>  	ndev = alloc_etherdev(sizeof(struct altera_tse_private));
> @@ -1262,10 +1291,6 @@ static int altera_tse_probe(struct platform_device *pdev)
>  	 */
>  	ret = request_and_map(pdev, "pcs", &pcs_res,
>  			      &priv->pcs_base);
> -	if (ret) {
> -		priv->pcs_base = priv->mac_dev + tse_csroffs(mdio_phy0);
> -		pcs_reg_width = 4;
> -	}
>  
>  	/* Rx IRQ */
>  	priv->rx_irq = platform_get_irq_byname(pdev, "rx_irq");
> @@ -1389,7 +1414,11 @@ static int altera_tse_probe(struct platform_device *pdev)
>  			 (unsigned long) control_port->start, priv->rx_irq,
>  			 priv->tx_irq);
>  
> -	priv->pcs = alt_tse_pcs_create(ndev, priv->pcs_base, pcs_reg_width);
> +	priv->pcs = alt_tse_pcs_create(ndev, &tse_ops, priv);
> +	if (!priv->pcs) {
> +		ret = -ENODEV;
> +		goto err_init_phy;
> +	}
>  
>  	priv->phylink_config.dev = &ndev->dev;
>  	priv->phylink_config.type = PHYLINK_NETDEV;
> @@ -1412,11 +1441,12 @@ static int altera_tse_probe(struct platform_device *pdev)
>  	if (IS_ERR(priv->phylink)) {
>  		dev_err(&pdev->dev, "failed to create phylink\n");
>  		ret = PTR_ERR(priv->phylink);
> -		goto err_init_phy;
> +		goto err_pcs;
>  	}
>  
>  	return 0;
> -
> +err_pcs:
> +	alt_tse_pcs_destroy(priv->pcs);
>  err_init_phy:
>  	unregister_netdev(ndev);
>  err_register_netdev:
> @@ -1438,6 +1468,8 @@ static int altera_tse_remove(struct platform_device *pdev)
>  	altera_tse_mdio_destroy(ndev);
>  	unregister_netdev(ndev);
>  	phylink_destroy(priv->phylink);
> +	alt_tse_pcs_destroy(priv->pcs);
> +
>  	free_netdev(ndev);
>  
>  	return 0;
> diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
> index 6e7e6c346a3e..768e8cefe17c 100644
> --- a/drivers/net/pcs/Kconfig
> +++ b/drivers/net/pcs/Kconfig
> @@ -28,8 +28,12 @@ config PCS_RZN1_MIIC
>  
>  config PCS_ALTERA_TSE
>  	tristate
> +	select PCS_LYNX
>  	help
>  	  This module provides helper functions for the Altera Triple Speed
>  	  Ethernet SGMII PCS, that can be found on the Intel Socfpga family.
> +	  This PCS appears to be a Lynx PCS exposed over mmio instead of a
> +	  mdio device, so the core logic from Lynx PCS is re-used and wrapped
> +	  around a virtual mii bus and mdio device.
>  
>  endmenu
> diff --git a/drivers/net/pcs/pcs-altera-tse.c b/drivers/net/pcs/pcs-altera-tse.c
> index d616749761f4..3adf6b1c0823 100644
> --- a/drivers/net/pcs/pcs-altera-tse.c
> +++ b/drivers/net/pcs/pcs-altera-tse.c
> @@ -9,151 +9,109 @@
>  #include <linux/phy.h>
>  #include <linux/phylink.h>
>  #include <linux/pcs-altera-tse.h>
> +#include <linux/pcs-lynx.h>
>  
> -/* SGMII PCS register addresses
> - */
> -#define SGMII_PCS_LINK_TIMER_0	0x12
> -#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_HALF_DUPLEX	BIT(4)
> -#define   PCS_IF_MODE_SGMI_PHY_AN	BIT(5)
> -#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)
> +static int altera_tse_pcs_mdio_write(struct mii_bus *bus, int mii_id, int regnum,

Confusing name "mii_id" (may suggest a connection to MII_PHYSID1/MII_PHYSID2
when there is none). Would suggest something more conventional, like "addr"
or "phyad".

> +				     u16 value)
>  {
> -	return container_of(pcs, struct altera_tse_pcs, pcs);
> -}
> +	struct altera_tse_pcs *tse_pcs = bus->priv;
>  
> -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);
> +	return tse_pcs->ops->write(tse_pcs->priv, regnum, value);
>  }
>  
> -static void tse_pcs_write(struct altera_tse_pcs *tse_pcs, int regnum,
> -			  u16 value)
> +static int altera_tse_pcs_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>  {
> -	if (tse_pcs->reg_width == 4)
> -		writel(value, tse_pcs->base + regnum * 4);
> -	else
> -		writew(value, tse_pcs->base + regnum * 2);
> -}
> +	struct altera_tse_pcs *tse_pcs = bus->priv;
>  
> -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);
> +	if (mii_id != 0)
> +		return 0;

This doesn't look well at all in the patch delta, but it makes altera_tse_pcs_mdio_read()
return 0 for reads from unknown (not emulated) PHY address. Would -ENODEV make more sense?
It would accelerate the failure (not sure when/if the lynx pcs driver would fail if
all its registers returned 0; I suppose it would be non-obvious).

>  
> -	return read_poll_timeout(tse_pcs_read, bmcr, (bmcr & BMCR_RESET),
> -				 10, SGMII_PCS_SW_RESET_TIMEOUT, 1,
> -				 tse_pcs, MII_BMCR);
> +	return tse_pcs->ops->read(tse_pcs->priv, regnum);
>  }
>  
> -static int alt_tse_pcs_validate(struct phylink_pcs *pcs,
> -				unsigned long *supported,
> -				const struct phylink_link_state *state)
> +static struct altera_tse_pcs *
> +altera_tse_pcs_mdio_create(struct net_device *dev,
> +			   struct altera_tse_pcs_ops *ops,
> +			   void *priv)
>  {
> -	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);
> +	struct altera_tse_pcs *tse_pcs;
> +	struct mii_bus *mii_bus;
> +	int ret;
> +
> +	tse_pcs = kzalloc(sizeof(*tse_pcs), GFP_KERNEL);
> +	if (IS_ERR(tse_pcs))
> +		return NULL;
> +
> +	tse_pcs->ops = ops;
> +	tse_pcs->priv = priv;
> +
> +	mii_bus = mdiobus_alloc();
> +	if (!mii_bus)
> +		goto out_free_pcs;
> +
> +	mii_bus->name = "Altera TSE PCS MDIO";
> +	mii_bus->read = &altera_tse_pcs_mdio_read;
> +	mii_bus->write = &altera_tse_pcs_mdio_write;
> +	snprintf(mii_bus->id, MII_BUS_ID_SIZE, "%s-%s", mii_bus->name,
> +		 dev->name);
> +
> +	mii_bus->priv = tse_pcs;
> +	mii_bus->parent = &dev->dev;
> +
> +	ret = mdiobus_register(mii_bus);
> +	if (ret)
> +		goto out_free_mdio;
> +
> +	tse_pcs->mii_bus = mii_bus;
> +	tse_pcs->mdiodev = mdio_device_create(mii_bus, 0);

Maybe a #define TSE_PCS_PHYAD 0, to make it clear that this 0 is the
same as the other 0?

> +	if (IS_ERR(tse_pcs->mdiodev)) {
> +		ret = PTR_ERR(tse_pcs->mdiodev);
> +		goto out_unregister_mdio;
>  	}
>  
> -	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;
>  
> -	return tse_pcs_reset(tse_pcs);
> +out_unregister_mdio:
> +	mdiobus_unregister(mii_bus);
> +out_free_mdio:
> +	mdiobus_free(mii_bus);
> +out_free_pcs:
> +	kfree(tse_pcs);
> +	return NULL;
>  }
>  
> -static void alt_tse_pcs_get_state(struct phylink_pcs *pcs,
> -				  struct phylink_link_state *state)
> +void alt_tse_pcs_destroy(struct altera_tse_pcs *tse_pcs)
>  {
> -	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);
> +	mdio_device_free(tse_pcs->mdiodev);
> +	mdiobus_unregister(tse_pcs->mii_bus);
> +	mdiobus_free(tse_pcs->mii_bus);
> +	kfree(tse_pcs);
>  }
>  
> -static void alt_tse_pcs_an_restart(struct phylink_pcs *pcs)
> +struct altera_tse_pcs *alt_tse_pcs_create(struct net_device *dev,
> +					  struct altera_tse_pcs_ops *ops,
> +					  void *priv)
>  {
> -	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);
> -}
> +	struct altera_tse_pcs *tse_pcs;
> +	struct phylink_pcs *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,
> -};
> +	tse_pcs = altera_tse_pcs_mdio_create(dev, ops, priv);
> +	if (!tse_pcs)
> +		return NULL;
>  
> -struct phylink_pcs *alt_tse_pcs_create(struct net_device *ndev,
> -				       void __iomem *pcs_base, int reg_width)
> -{
> -	struct altera_tse_pcs *tse_pcs;
> +	pcs = lynx_pcs_create(tse_pcs->mdiodev);
> +	if (!pcs)
> +		goto out_free_mdio;
>  
> -	if (reg_width != 4 && reg_width != 2)
> -		return ERR_PTR(-EINVAL);
> +	tse_pcs->pcs = pcs;
>  
> -	tse_pcs = devm_kzalloc(&ndev->dev, sizeof(*tse_pcs), GFP_KERNEL);
> -	if (!tse_pcs)
> -		return ERR_PTR(-ENOMEM);
> +	return tse_pcs;
>  
> -	tse_pcs->pcs.ops = &alt_tse_pcs_ops;
> -	tse_pcs->base = pcs_base;
> -	tse_pcs->reg_width = reg_width;
> +out_free_mdio:
> +	alt_tse_pcs_destroy(tse_pcs);
>  
> -	return &tse_pcs->pcs;
> +	return NULL;
>  }
> -EXPORT_SYMBOL_GPL(alt_tse_pcs_create);
>  
>  MODULE_LICENSE("GPL");
>  MODULE_DESCRIPTION("Altera TSE PCS driver");
> diff --git a/include/linux/pcs-altera-tse.h b/include/linux/pcs-altera-tse.h
> index 92ab9f08e835..67be242a468e 100644
> --- a/include/linux/pcs-altera-tse.h
> +++ b/include/linux/pcs-altera-tse.h
> @@ -11,7 +11,25 @@
>  struct phylink_pcs;
>  struct net_device;
>  
> -struct phylink_pcs *alt_tse_pcs_create(struct net_device *ndev,
> -				       void __iomem *pcs_base, int reg_width);
> +struct altera_tse_pcs_ops {
> +	int (*read)(void *priv, int regnum);
> +	int (*write)(void *priv, int regnum, u16 value);
> +};
> +
> +struct altera_tse_pcs {
> +	struct phylink_pcs *pcs;
> +	struct altera_tse_pcs_ops *ops;
> +	struct mii_bus *mii_bus;
> +	struct mdio_device *mdiodev;
> +	void *priv;
> +};

Don't expose struct altera_tse_pcs to include/linux/pcs-altera-tse.h if
only drivers/net/pcs/pcs-altera-tse.c is going to access it. Put in in
pcs-altera-tse.c.

> +
> +#define altera_tse_pcs_to_phylink_pcs(tse_pcs)	((tse_pcs)->pcs)
> +
> +struct altera_tse_pcs *alt_tse_pcs_create(struct net_device *ndev,
> +					  struct altera_tse_pcs_ops *ops,
> +					  void *priv);

Also, it seems trivial for this to return the more generic struct phylink_pcs
type.

> +
> +void alt_tse_pcs_destroy(struct altera_tse_pcs *tse_pcs);
>  
>  #endif /* __LINUX_PCS_ALTERA_TSE_H */
> -- 
> 2.39.1
>
Andrew Lunn Feb. 10, 2023, 8:02 p.m. UTC | #2 
On Fri, Feb 10, 2023 at 09:31:59PM +0200, Vladimir Oltean wrote:
> On Fri, Feb 10, 2023 at 08:09:49PM +0100, Maxime Chevallier wrote:
> > When submitting the initial driver for the Altera TSE PCS, Russell King
> > noted that the register layout for the TSE PCS is very similar to the
> > Lynx PCS. The main difference being that TSE PCS's register space is
> > memory-mapped, whereas Lynx's is exposed over MDIO.
> > 
> > Convert the TSE PCS to reuse the whole logic from Lynx, by allowing
> > the creation of a dummy MDIO bus, and a dummy MDIO device located at
> > address 0 on that bus. The MAC driver that uses this PCS must provide
> > callbacks to read/write the MMIO.
> > 
> > Also convert the Altera TSE MAC driver to this new way of using the TSE
> > PCS.
> > 
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > ---
> >  drivers/net/ethernet/altera/altera_tse.h      |   2 +-
> >  drivers/net/ethernet/altera/altera_tse_main.c |  50 ++++-
> >  drivers/net/pcs/Kconfig                       |   4 +
> >  drivers/net/pcs/pcs-altera-tse.c              | 194 +++++++-----------
> >  include/linux/pcs-altera-tse.h                |  22 +-
> >  5 files changed, 142 insertions(+), 130 deletions(-)
> 
> The glue layer is larger than the duplicated PCS code? :(

I was wondering if the glue could actually be made generic. The kernel
has a number of reasonably generic MMIO device drivers, which are just
given an address range and assume a logical mapping.

Could this be made into a generic MDIO MMIO bus driver, which just
gets configured with a base address, and maybe a stride between
registers?

	Andrew
kernel test robot Feb. 11, 2023, 6:05 a.m. UTC | #3 
Hi Maxime,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Maxime-Chevallier/net-pcs-tse-port-to-pcs-lynx/20230211-021544
patch link:    https://lore.kernel.org/r/20230210190949.1115836-1-maxime.chevallier%40bootlin.com
patch subject: [PATCH net-next] net: pcs: tse: port to pcs-lynx
config: i386-randconfig-a013 (https://download.01.org/0day-ci/archive/20230211/202302111310.IO2xvRNi-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/fc34f36dac37aedc9928f351a233c6f610fd5a68
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Maxime-Chevallier/net-pcs-tse-port-to-pcs-lynx/20230211-021544
        git checkout fc34f36dac37aedc9928f351a233c6f610fd5a68
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302111310.IO2xvRNi-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "alt_tse_pcs_create" [drivers/net/ethernet/altera/altera_tse.ko] undefined!
>> ERROR: modpost: "alt_tse_pcs_destroy" [drivers/net/ethernet/altera/altera_tse.ko] undefined!
Andrew Lunn Feb. 11, 2023, 3:32 p.m. UTC | #4 
On Fri, Feb 10, 2023 at 09:31:59PM +0200, Vladimir Oltean wrote:
> On Fri, Feb 10, 2023 at 08:09:49PM +0100, Maxime Chevallier wrote:
> > When submitting the initial driver for the Altera TSE PCS, Russell King
> > noted that the register layout for the TSE PCS is very similar to the
> > Lynx PCS. The main difference being that TSE PCS's register space is
> > memory-mapped, whereas Lynx's is exposed over MDIO.
> > 
> > Convert the TSE PCS to reuse the whole logic from Lynx, by allowing
> > the creation of a dummy MDIO bus, and a dummy MDIO device located at
> > address 0 on that bus. The MAC driver that uses this PCS must provide
> > callbacks to read/write the MMIO.
> > 
> > Also convert the Altera TSE MAC driver to this new way of using the TSE
> > PCS.
> > 
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > ---
> >  drivers/net/ethernet/altera/altera_tse.h      |   2 +-
> >  drivers/net/ethernet/altera/altera_tse_main.c |  50 ++++-
> >  drivers/net/pcs/Kconfig                       |   4 +
> >  drivers/net/pcs/pcs-altera-tse.c              | 194 +++++++-----------
> >  include/linux/pcs-altera-tse.h                |  22 +-
> >  5 files changed, 142 insertions(+), 130 deletions(-)
> 
> The glue layer is larger than the duplicated PCS code? :(

Another option might be regmap. There are both regmap-mdio.c and
regmap-mmio.c.

	Andrew
Vladimir Oltean Feb. 11, 2023, 9:45 p.m. UTC | #5 
On Sat, Feb 11, 2023 at 04:32:49PM +0100, Andrew Lunn wrote:
> Another option might be regmap. There are both regmap-mdio.c and
> regmap-mmio.c.

Maxime had originally considered regmap too, but I thought it would
involve too many changes in the lynx-pcs driver and I wasn't sure
that those changes were for the better, since it would then need
to use the more low-level variants of phylink helpers (for example
phylink_mii_c22_pcs_decode_state() instead of phylink_mii_c22_pcs_get_state()).
Vladimir Oltean Feb. 11, 2023, 9:52 p.m. UTC | #6 
On Fri, Feb 10, 2023 at 09:02:39PM +0100, Andrew Lunn wrote:
> I was wondering if the glue could actually be made generic. The kernel
> has a number of reasonably generic MMIO device drivers, which are just
> given an address range and assume a logical mapping.
> 
> Could this be made into a generic MDIO MMIO bus driver, which just
> gets configured with a base address, and maybe a stride between
> registers?

This sounds interesting to me because I also have at least one other
potential use for it. The "nxp,sja1110-base-tx-mdio" driver does basically
just that, except it's SPI instead of MMIO. So if the generic driver was a
platform device driver and it was aware of dev_get_regmap(), it could
get reused.

What I'm not sure of is the spacing between MDIO registers. For the
SJA1110 CBTX PHY, the registers are 32-bit wide (but contain 16-bit
values). So MII_BMCR is at offset 0x0, MII_BMSR at 0x4 etc. I'd imagine
that other MDIO buses might have MII_BMSR at 0x2.
Andrew Lunn Feb. 11, 2023, 10:23 p.m. UTC | #7 
On Sat, Feb 11, 2023 at 11:52:29PM +0200, Vladimir Oltean wrote:
> On Fri, Feb 10, 2023 at 09:02:39PM +0100, Andrew Lunn wrote:
> > I was wondering if the glue could actually be made generic. The kernel
> > has a number of reasonably generic MMIO device drivers, which are just
> > given an address range and assume a logical mapping.
> > 
> > Could this be made into a generic MDIO MMIO bus driver, which just
> > gets configured with a base address, and maybe a stride between
> > registers?
> 
> This sounds interesting to me because I also have at least one other
> potential use for it. The "nxp,sja1110-base-tx-mdio" driver does basically
> just that, except it's SPI instead of MMIO. So if the generic driver was a
> platform device driver and it was aware of dev_get_regmap(), it could
> get reused.
> 
> What I'm not sure of is the spacing between MDIO registers. For the
> SJA1110 CBTX PHY, the registers are 32-bit wide (but contain 16-bit
> values). So MII_BMCR is at offset 0x0, MII_BMSR at 0x4 etc. I'd imagine
> that other MDIO buses might have MII_BMSR at 0x2.

This is what i meant by stride. The distance between registers. As you
say, it could be 2 bytes, but also 4 bytes. It should be a
configuration parameter when instantiating such a generic driver.

	Andrew
Maxime Chevallier Feb. 21, 2023, 7:34 a.m. UTC | #8 
Hi Vlad, Andrew,

On Fri, 10 Feb 2023 21:02:39 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> On Fri, Feb 10, 2023 at 09:31:59PM +0200, Vladimir Oltean wrote:
> > On Fri, Feb 10, 2023 at 08:09:49PM +0100, Maxime Chevallier wrote:  
> > > When submitting the initial driver for the Altera TSE PCS,
> > > Russell King noted that the register layout for the TSE PCS is
> > > very similar to the Lynx PCS. The main difference being that TSE
> > > PCS's register space is memory-mapped, whereas Lynx's is exposed
> > > over MDIO.
> > > 
> > > Convert the TSE PCS to reuse the whole logic from Lynx, by
> > > allowing the creation of a dummy MDIO bus, and a dummy MDIO
> > > device located at address 0 on that bus. The MAC driver that uses
> > > this PCS must provide callbacks to read/write the MMIO.
> > > 
> > > Also convert the Altera TSE MAC driver to this new way of using
> > > the TSE PCS.
> > > 
> > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > ---
> > >  drivers/net/ethernet/altera/altera_tse.h      |   2 +-
> > >  drivers/net/ethernet/altera/altera_tse_main.c |  50 ++++-
> > >  drivers/net/pcs/Kconfig                       |   4 +
> > >  drivers/net/pcs/pcs-altera-tse.c              | 194
> > > +++++++----------- include/linux/pcs-altera-tse.h
> > > |  22 +- 5 files changed, 142 insertions(+), 130 deletions(-)  
> > 
> > The glue layer is larger than the duplicated PCS code? :(  
> 
> I was wondering if the glue could actually be made generic. The kernel
> has a number of reasonably generic MMIO device drivers, which are just
> given an address range and assume a logical mapping.
> 
> Could this be made into a generic MDIO MMIO bus driver, which just
> gets configured with a base address, and maybe a stride between
> registers?

That would be ideal, I'll spin a new series prorotyping this, indeed
that can be interesting for other devices.

Thanks for the review,

Maxime

> 	Andrew
Sean Anderson Feb. 23, 2023, 5:13 p.m. UTC | #9 
On 2/10/23 15:02, Andrew Lunn wrote:
> On Fri, Feb 10, 2023 at 09:31:59PM +0200, Vladimir Oltean wrote:
>> On Fri, Feb 10, 2023 at 08:09:49PM +0100, Maxime Chevallier wrote:
>> > When submitting the initial driver for the Altera TSE PCS, Russell King
>> > noted that the register layout for the TSE PCS is very similar to the
>> > Lynx PCS. The main difference being that TSE PCS's register space is
>> > memory-mapped, whereas Lynx's is exposed over MDIO.
>> > 
>> > Convert the TSE PCS to reuse the whole logic from Lynx, by allowing
>> > the creation of a dummy MDIO bus, and a dummy MDIO device located at
>> > address 0 on that bus. The MAC driver that uses this PCS must provide
>> > callbacks to read/write the MMIO.
>> > 
>> > Also convert the Altera TSE MAC driver to this new way of using the TSE
>> > PCS.
>> > 
>> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
>> > ---
>> >  drivers/net/ethernet/altera/altera_tse.h      |   2 +-
>> >  drivers/net/ethernet/altera/altera_tse_main.c |  50 ++++-
>> >  drivers/net/pcs/Kconfig                       |   4 +
>> >  drivers/net/pcs/pcs-altera-tse.c              | 194 +++++++-----------
>> >  include/linux/pcs-altera-tse.h                |  22 +-
>> >  5 files changed, 142 insertions(+), 130 deletions(-)
>> 
>> The glue layer is larger than the duplicated PCS code? :(
> 
> I was wondering if the glue could actually be made generic. The kernel
> has a number of reasonably generic MMIO device drivers, which are just
> given an address range and assume a logical mapping.
> 
> Could this be made into a generic MDIO MMIO bus driver, which just
> gets configured with a base address, and maybe a stride between
> registers?

That's what I originally considered for MACB [1], but I ended up creating
the various encode/decode functions because the PCS didn't have an MDIO
counterpart [2].

--Sean

[1] https://lore.kernel.org/netdev/20211004191527.1610759-11-sean.anderson@seco.com/
    This patch doesn't have a mmap-mdio, as I dropped it before submission.
[2] https://lore.kernel.org/netdev/20211004191527.1610759-8-sean.anderson@seco.com/
diff mbox series

Patch

diff --git a/drivers/net/ethernet/altera/altera_tse.h b/drivers/net/ethernet/altera/altera_tse.h
index db5eed06e92d..f4e3fddb639a 100644
--- a/drivers/net/ethernet/altera/altera_tse.h
+++ b/drivers/net/ethernet/altera/altera_tse.h
@@ -476,7 +476,7 @@  struct altera_tse_private {
 
 	struct phylink *phylink;
 	struct phylink_config phylink_config;
-	struct phylink_pcs *pcs;
+	struct altera_tse_pcs *pcs;
 };
 
 /* Function prototypes
diff --git a/drivers/net/ethernet/altera/altera_tse_main.c b/drivers/net/ethernet/altera/altera_tse_main.c
index 66e3af73ec41..109b7ed90c6e 100644
--- a/drivers/net/ethernet/altera/altera_tse_main.c
+++ b/drivers/net/ethernet/altera/altera_tse_main.c
@@ -87,6 +87,36 @@  static inline u32 tse_tx_avail(struct altera_tse_private *priv)
 	return priv->tx_cons + priv->tx_ring_size - priv->tx_prod - 1;
 }
 
+static int altera_tse_pcs_read(void *priv, int regnum)
+{
+	struct altera_tse_private *tse = priv;
+
+	if (tse->pcs_base)
+		return readw(tse->pcs_base + (regnum * 2));
+	else
+		return readl(tse->mac_dev + tse_csroffs(mdio_phy0) +
+			     (regnum * 4));
+	return 0;
+}
+
+static int altera_tse_pcs_write(void *priv, int regnum, u16 value)
+{
+	struct altera_tse_private *tse = priv;
+
+	if (tse->pcs_base)
+		writew(value, tse->pcs_base + (regnum * 2));
+	else
+		writel(value, tse->mac_dev + tse_csroffs(mdio_phy0) +
+			(regnum * 4));
+
+	return 0;
+}
+
+static struct altera_tse_pcs_ops tse_ops = {
+	.read = altera_tse_pcs_read,
+	.write = altera_tse_pcs_write,
+};
+
 /* MDIO specific functions
  */
 static int altera_tse_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
@@ -1090,7 +1120,7 @@  static struct phylink_pcs *alt_tse_select_pcs(struct phylink_config *config,
 
 	if (interface == PHY_INTERFACE_MODE_SGMII ||
 	    interface == PHY_INTERFACE_MODE_1000BASEX)
-		return priv->pcs;
+		return altera_tse_pcs_to_phylink_pcs(priv->pcs);
 	else
 		return NULL;
 }
@@ -1143,7 +1173,6 @@  static int altera_tse_probe(struct platform_device *pdev)
 	struct resource *pcs_res;
 	struct net_device *ndev;
 	void __iomem *descmap;
-	int pcs_reg_width = 2;
 	int ret = -ENODEV;
 
 	ndev = alloc_etherdev(sizeof(struct altera_tse_private));
@@ -1262,10 +1291,6 @@  static int altera_tse_probe(struct platform_device *pdev)
 	 */
 	ret = request_and_map(pdev, "pcs", &pcs_res,
 			      &priv->pcs_base);
-	if (ret) {
-		priv->pcs_base = priv->mac_dev + tse_csroffs(mdio_phy0);
-		pcs_reg_width = 4;
-	}
 
 	/* Rx IRQ */
 	priv->rx_irq = platform_get_irq_byname(pdev, "rx_irq");
@@ -1389,7 +1414,11 @@  static int altera_tse_probe(struct platform_device *pdev)
 			 (unsigned long) control_port->start, priv->rx_irq,
 			 priv->tx_irq);
 
-	priv->pcs = alt_tse_pcs_create(ndev, priv->pcs_base, pcs_reg_width);
+	priv->pcs = alt_tse_pcs_create(ndev, &tse_ops, priv);
+	if (!priv->pcs) {
+		ret = -ENODEV;
+		goto err_init_phy;
+	}
 
 	priv->phylink_config.dev = &ndev->dev;
 	priv->phylink_config.type = PHYLINK_NETDEV;
@@ -1412,11 +1441,12 @@  static int altera_tse_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->phylink)) {
 		dev_err(&pdev->dev, "failed to create phylink\n");
 		ret = PTR_ERR(priv->phylink);
-		goto err_init_phy;
+		goto err_pcs;
 	}
 
 	return 0;
-
+err_pcs:
+	alt_tse_pcs_destroy(priv->pcs);
 err_init_phy:
 	unregister_netdev(ndev);
 err_register_netdev:
@@ -1438,6 +1468,8 @@  static int altera_tse_remove(struct platform_device *pdev)
 	altera_tse_mdio_destroy(ndev);
 	unregister_netdev(ndev);
 	phylink_destroy(priv->phylink);
+	alt_tse_pcs_destroy(priv->pcs);
+
 	free_netdev(ndev);
 
 	return 0;
diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index 6e7e6c346a3e..768e8cefe17c 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -28,8 +28,12 @@  config PCS_RZN1_MIIC
 
 config PCS_ALTERA_TSE
 	tristate
+	select PCS_LYNX
 	help
 	  This module provides helper functions for the Altera Triple Speed
 	  Ethernet SGMII PCS, that can be found on the Intel Socfpga family.
+	  This PCS appears to be a Lynx PCS exposed over mmio instead of a
+	  mdio device, so the core logic from Lynx PCS is re-used and wrapped
+	  around a virtual mii bus and mdio device.
 
 endmenu
diff --git a/drivers/net/pcs/pcs-altera-tse.c b/drivers/net/pcs/pcs-altera-tse.c
index d616749761f4..3adf6b1c0823 100644
--- a/drivers/net/pcs/pcs-altera-tse.c
+++ b/drivers/net/pcs/pcs-altera-tse.c
@@ -9,151 +9,109 @@ 
 #include <linux/phy.h>
 #include <linux/phylink.h>
 #include <linux/pcs-altera-tse.h>
+#include <linux/pcs-lynx.h>
 
-/* SGMII PCS register addresses
- */
-#define SGMII_PCS_LINK_TIMER_0	0x12
-#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_HALF_DUPLEX	BIT(4)
-#define   PCS_IF_MODE_SGMI_PHY_AN	BIT(5)
-#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)
+static int altera_tse_pcs_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
+				     u16 value)
 {
-	return container_of(pcs, struct altera_tse_pcs, pcs);
-}
+	struct altera_tse_pcs *tse_pcs = bus->priv;
 
-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);
+	return tse_pcs->ops->write(tse_pcs->priv, regnum, value);
 }
 
-static void tse_pcs_write(struct altera_tse_pcs *tse_pcs, int regnum,
-			  u16 value)
+static int altera_tse_pcs_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 {
-	if (tse_pcs->reg_width == 4)
-		writel(value, tse_pcs->base + regnum * 4);
-	else
-		writew(value, tse_pcs->base + regnum * 2);
-}
+	struct altera_tse_pcs *tse_pcs = bus->priv;
 
-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);
+	if (mii_id != 0)
+		return 0;
 
-	return read_poll_timeout(tse_pcs_read, bmcr, (bmcr & BMCR_RESET),
-				 10, SGMII_PCS_SW_RESET_TIMEOUT, 1,
-				 tse_pcs, MII_BMCR);
+	return tse_pcs->ops->read(tse_pcs->priv, regnum);
 }
 
-static int alt_tse_pcs_validate(struct phylink_pcs *pcs,
-				unsigned long *supported,
-				const struct phylink_link_state *state)
+static struct altera_tse_pcs *
+altera_tse_pcs_mdio_create(struct net_device *dev,
+			   struct altera_tse_pcs_ops *ops,
+			   void *priv)
 {
-	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);
+	struct altera_tse_pcs *tse_pcs;
+	struct mii_bus *mii_bus;
+	int ret;
+
+	tse_pcs = kzalloc(sizeof(*tse_pcs), GFP_KERNEL);
+	if (IS_ERR(tse_pcs))
+		return NULL;
+
+	tse_pcs->ops = ops;
+	tse_pcs->priv = priv;
+
+	mii_bus = mdiobus_alloc();
+	if (!mii_bus)
+		goto out_free_pcs;
+
+	mii_bus->name = "Altera TSE PCS MDIO";
+	mii_bus->read = &altera_tse_pcs_mdio_read;
+	mii_bus->write = &altera_tse_pcs_mdio_write;
+	snprintf(mii_bus->id, MII_BUS_ID_SIZE, "%s-%s", mii_bus->name,
+		 dev->name);
+
+	mii_bus->priv = tse_pcs;
+	mii_bus->parent = &dev->dev;
+
+	ret = mdiobus_register(mii_bus);
+	if (ret)
+		goto out_free_mdio;
+
+	tse_pcs->mii_bus = mii_bus;
+	tse_pcs->mdiodev = mdio_device_create(mii_bus, 0);
+	if (IS_ERR(tse_pcs->mdiodev)) {
+		ret = PTR_ERR(tse_pcs->mdiodev);
+		goto out_unregister_mdio;
 	}
 
-	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;
 
-	return tse_pcs_reset(tse_pcs);
+out_unregister_mdio:
+	mdiobus_unregister(mii_bus);
+out_free_mdio:
+	mdiobus_free(mii_bus);
+out_free_pcs:
+	kfree(tse_pcs);
+	return NULL;
 }
 
-static void alt_tse_pcs_get_state(struct phylink_pcs *pcs,
-				  struct phylink_link_state *state)
+void alt_tse_pcs_destroy(struct altera_tse_pcs *tse_pcs)
 {
-	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);
+	mdio_device_free(tse_pcs->mdiodev);
+	mdiobus_unregister(tse_pcs->mii_bus);
+	mdiobus_free(tse_pcs->mii_bus);
+	kfree(tse_pcs);
 }
 
-static void alt_tse_pcs_an_restart(struct phylink_pcs *pcs)
+struct altera_tse_pcs *alt_tse_pcs_create(struct net_device *dev,
+					  struct altera_tse_pcs_ops *ops,
+					  void *priv)
 {
-	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);
-}
+	struct altera_tse_pcs *tse_pcs;
+	struct phylink_pcs *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,
-};
+	tse_pcs = altera_tse_pcs_mdio_create(dev, ops, priv);
+	if (!tse_pcs)
+		return NULL;
 
-struct phylink_pcs *alt_tse_pcs_create(struct net_device *ndev,
-				       void __iomem *pcs_base, int reg_width)
-{
-	struct altera_tse_pcs *tse_pcs;
+	pcs = lynx_pcs_create(tse_pcs->mdiodev);
+	if (!pcs)
+		goto out_free_mdio;
 
-	if (reg_width != 4 && reg_width != 2)
-		return ERR_PTR(-EINVAL);
+	tse_pcs->pcs = pcs;
 
-	tse_pcs = devm_kzalloc(&ndev->dev, sizeof(*tse_pcs), GFP_KERNEL);
-	if (!tse_pcs)
-		return ERR_PTR(-ENOMEM);
+	return tse_pcs;
 
-	tse_pcs->pcs.ops = &alt_tse_pcs_ops;
-	tse_pcs->base = pcs_base;
-	tse_pcs->reg_width = reg_width;
+out_free_mdio:
+	alt_tse_pcs_destroy(tse_pcs);
 
-	return &tse_pcs->pcs;
+	return NULL;
 }
-EXPORT_SYMBOL_GPL(alt_tse_pcs_create);
 
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("Altera TSE PCS driver");
diff --git a/include/linux/pcs-altera-tse.h b/include/linux/pcs-altera-tse.h
index 92ab9f08e835..67be242a468e 100644
--- a/include/linux/pcs-altera-tse.h
+++ b/include/linux/pcs-altera-tse.h
@@ -11,7 +11,25 @@ 
 struct phylink_pcs;
 struct net_device;
 
-struct phylink_pcs *alt_tse_pcs_create(struct net_device *ndev,
-				       void __iomem *pcs_base, int reg_width);
+struct altera_tse_pcs_ops {
+	int (*read)(void *priv, int regnum);
+	int (*write)(void *priv, int regnum, u16 value);
+};
+
+struct altera_tse_pcs {
+	struct phylink_pcs *pcs;
+	struct altera_tse_pcs_ops *ops;
+	struct mii_bus *mii_bus;
+	struct mdio_device *mdiodev;
+	void *priv;
+};
+
+#define altera_tse_pcs_to_phylink_pcs(tse_pcs)	((tse_pcs)->pcs)
+
+struct altera_tse_pcs *alt_tse_pcs_create(struct net_device *ndev,
+					  struct altera_tse_pcs_ops *ops,
+					  void *priv);
+
+void alt_tse_pcs_destroy(struct altera_tse_pcs *tse_pcs);
 
 #endif /* __LINUX_PCS_ALTERA_TSE_H */