| 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 |
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 >
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
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!
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
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()).
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.
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
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
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 --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 */
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(-)