Message-ID: <20230210193159.qmbtvwtx6kqagvxy@skbuf>
Date:   Fri, 10 Feb 2023 21:31:59 +0200
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com,
        Andrew Lunn <andrew@...n.ch>, Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        linux-arm-kernel@...ts.infradead.org,
        Ioana Ciornei <ioana.ciornei@....com>
Subject: Re: [PATCH net-next] net: pcs: tse: port to pcs-lynx

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@...tlin.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
>

Powered by blists - more mailing lists