| Message ID | 20230525101126.370108-2-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: add a regmap-based mdio driver and drop TSE PCS | expand |
On Thu, May 25, 2023 at 12:11:23PM +0200, Maxime Chevallier wrote: > There exists several examples today of devices that embed an ethernet > PHY or PCS directly inside an SoC. In this situation, either the device > is controlled through a vendor-specific register set, or sometimes > exposes the standard 802.3 registers that are typically accessed over > MDIO. > > As phylib and phylink are designed to use mdiodevices, this driver > allows creating a virtual MDIO bus, that translates mdiodev register > accesses to regmap accesses. > > The reason we use regmap is because there are at least 3 such devices > known today, 2 of them are Altera TSE PCS's, memory-mapped, exposed > with a 4-byte stride in stmmac's dwmac-socfpga variant, and a 2-byte > stride in altera-tse. The other one (nxp,sja1110-base-tx-mdio) is > exposed over SPI. > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> ... > +struct mii_bus *devm_mdio_regmap_register(struct device *dev, > + const struct mdio_regmap_config *config) > +{ > + struct mdio_regmap_config *mrc; > + struct mii_bus *mii; > + int rc; > + > + if (!config->parent) > + return ERR_PTR(-EINVAL); > + > + mii = devm_mdiobus_alloc_size(config->parent, sizeof(*mrc)); > + if (!mii) > + return ERR_PTR(-ENOMEM); > + > + mrc = mii->priv; > + memcpy(mrc, config, sizeof(*mrc)); > + > + mrc->regmap = config->regmap; > + mrc->valid_addr = config->valid_addr; > + > + mii->name = DRV_NAME; > + strscpy(mii->id, config->name, MII_BUS_ID_SIZE); > + mii->parent = config->parent; > + mii->read = mdio_regmap_read_c22; > + mii->write = mdio_regmap_write_c22; > + > + if (config->autoscan) > + mii->phy_mask = ~BIT(config->valid_addr); > + else > + mii->phy_mask = ~0UL; Hi Maxime, phy_mask is a u32. But 0UL may be either 32 or 64 bits wide. I think a better approach would be to use U32_MAX. > + > + rc = devm_mdiobus_register(dev, mii); > + if (rc) { > + dev_err(config->parent, "Cannot register MDIO bus![%s] (%d)\n", mii->id, rc); > + return ERR_PTR(rc); > + } > + > + return mii; > +} > +EXPORT_SYMBOL_GPL(devm_mdio_regmap_register); > + > +MODULE_DESCRIPTION("MDIO API over regmap"); > +MODULE_AUTHOR("Maxime Chevallier <maxime.chevallier@bootlin.com>"); > +MODULE_LICENSE("GPL");
On Thu, May 25, 2023 at 12:11:23PM +0200, Maxime Chevallier wrote: > +struct mii_bus *devm_mdio_regmap_register(struct device *dev, > + const struct mdio_regmap_config *config) > +{ > + struct mdio_regmap_config *mrc; > + struct mii_bus *mii; > + int rc; > + > + if (!config->parent) > + return ERR_PTR(-EINVAL); > + > + mii = devm_mdiobus_alloc_size(config->parent, sizeof(*mrc)); > + if (!mii) > + return ERR_PTR(-ENOMEM); > + > + mrc = mii->priv; > + memcpy(mrc, config, sizeof(*mrc)); > + > + mrc->regmap = config->regmap; > + mrc->valid_addr = config->valid_addr; You have just memcpy'd everything from config into mrc. Doesn't this already include "regmap" and "valid_addr" ? However, these are the only two things used, so does it really make sense to allocate the full mdio_regmap_config structure, or would a smaller data structure (of one pointer and one u8) be more appropriate?
Hello Russell, On Thu, 25 May 2023 12:11:38 +0100 "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > On Thu, May 25, 2023 at 12:11:23PM +0200, Maxime Chevallier wrote: > > +struct mii_bus *devm_mdio_regmap_register(struct device *dev, > > + const struct > > mdio_regmap_config *config) +{ > > + struct mdio_regmap_config *mrc; > > + struct mii_bus *mii; > > + int rc; > > + > > + if (!config->parent) > > + return ERR_PTR(-EINVAL); > > + > > + mii = devm_mdiobus_alloc_size(config->parent, > > sizeof(*mrc)); > > + if (!mii) > > + return ERR_PTR(-ENOMEM); > > + > > + mrc = mii->priv; > > + memcpy(mrc, config, sizeof(*mrc)); > > + > > + mrc->regmap = config->regmap; > > + mrc->valid_addr = config->valid_addr; > > You have just memcpy'd everything from config into mrc. Doesn't this > already include "regmap" and "valid_addr" ? Oh right... good catch, thanks ! > However, these are the only two things used, so does it really make > sense to allocate the full mdio_regmap_config structure, or would a > smaller data structure (of one pointer and one u8) be more > appropriate? > You are correct, other fields are unused so I'll use a new struct for the mii->priv field. Thank you for reviewing, Best regards, Maxime
Hello Simon, On Thu, 25 May 2023 13:02:39 +0200 Simon Horman <simon.horman@corigine.com> wrote: > On Thu, May 25, 2023 at 12:11:23PM +0200, Maxime Chevallier wrote: > > There exists several examples today of devices that embed an > > ethernet PHY or PCS directly inside an SoC. In this situation, > > either the device is controlled through a vendor-specific register > > set, or sometimes exposes the standard 802.3 registers that are > > typically accessed over MDIO. > > > > As phylib and phylink are designed to use mdiodevices, this driver > > allows creating a virtual MDIO bus, that translates mdiodev register > > accesses to regmap accesses. > > > > The reason we use regmap is because there are at least 3 such > > devices known today, 2 of them are Altera TSE PCS's, memory-mapped, > > exposed with a 4-byte stride in stmmac's dwmac-socfpga variant, and > > a 2-byte stride in altera-tse. The other one > > (nxp,sja1110-base-tx-mdio) is exposed over SPI. > > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > > ... > > > +struct mii_bus *devm_mdio_regmap_register(struct device *dev, > > + const struct > > mdio_regmap_config *config) +{ > > + struct mdio_regmap_config *mrc; > > + struct mii_bus *mii; > > + int rc; > > + > > + if (!config->parent) > > + return ERR_PTR(-EINVAL); > > + > > + mii = devm_mdiobus_alloc_size(config->parent, > > sizeof(*mrc)); > > + if (!mii) > > + return ERR_PTR(-ENOMEM); > > + > > + mrc = mii->priv; > > + memcpy(mrc, config, sizeof(*mrc)); > > + > > + mrc->regmap = config->regmap; > > + mrc->valid_addr = config->valid_addr; > > + > > + mii->name = DRV_NAME; > > + strscpy(mii->id, config->name, MII_BUS_ID_SIZE); > > + mii->parent = config->parent; > > + mii->read = mdio_regmap_read_c22; > > + mii->write = mdio_regmap_write_c22; > > + > > + if (config->autoscan) > > + mii->phy_mask = ~BIT(config->valid_addr); > > + else > > + mii->phy_mask = ~0UL; > > Hi Maxime, > > phy_mask is a u32. > But 0UL may be either 32 or 64 bits wide. Right > I think a better approach would be to use U32_MAX. I guess ~0 would also work, and this would also align with what fixed-phy and sfp do for their internal MDIO bus. I'll fix that for next revision Thanks, Maxime > > + > > + rc = devm_mdiobus_register(dev, mii); > > + if (rc) { > > + dev_err(config->parent, "Cannot register MDIO > > bus![%s] (%d)\n", mii->id, rc); > > + return ERR_PTR(rc); > > + } > > + > > + return mii; > > +} > > +EXPORT_SYMBOL_GPL(devm_mdio_regmap_register); > > + > > +MODULE_DESCRIPTION("MDIO API over regmap"); > > +MODULE_AUTHOR("Maxime Chevallier <maxime.chevallier@bootlin.com>"); > > +MODULE_LICENSE("GPL");
On Thu, May 25, 2023 at 02:43:59PM +0200, Maxime Chevallier wrote: > Hello Simon, > > On Thu, 25 May 2023 13:02:39 +0200 > Simon Horman <simon.horman@corigine.com> wrote: > > > On Thu, May 25, 2023 at 12:11:23PM +0200, Maxime Chevallier wrote: > > > There exists several examples today of devices that embed an > > > ethernet PHY or PCS directly inside an SoC. In this situation, > > > either the device is controlled through a vendor-specific register > > > set, or sometimes exposes the standard 802.3 registers that are > > > typically accessed over MDIO. > > > > > > As phylib and phylink are designed to use mdiodevices, this driver > > > allows creating a virtual MDIO bus, that translates mdiodev register > > > accesses to regmap accesses. > > > > > > The reason we use regmap is because there are at least 3 such > > > devices known today, 2 of them are Altera TSE PCS's, memory-mapped, > > > exposed with a 4-byte stride in stmmac's dwmac-socfpga variant, and > > > a 2-byte stride in altera-tse. The other one > > > (nxp,sja1110-base-tx-mdio) is exposed over SPI. > > > > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > > > > ... > > > > > +struct mii_bus *devm_mdio_regmap_register(struct device *dev, > > > + const struct > > > mdio_regmap_config *config) +{ > > > + struct mdio_regmap_config *mrc; > > > + struct mii_bus *mii; > > > + int rc; > > > + > > > + if (!config->parent) > > > + return ERR_PTR(-EINVAL); > > > + > > > + mii = devm_mdiobus_alloc_size(config->parent, > > > sizeof(*mrc)); > > > + if (!mii) > > > + return ERR_PTR(-ENOMEM); > > > + > > > + mrc = mii->priv; > > > + memcpy(mrc, config, sizeof(*mrc)); > > > + > > > + mrc->regmap = config->regmap; > > > + mrc->valid_addr = config->valid_addr; > > > + > > > + mii->name = DRV_NAME; > > > + strscpy(mii->id, config->name, MII_BUS_ID_SIZE); > > > + mii->parent = config->parent; > > > + mii->read = mdio_regmap_read_c22; > > > + mii->write = mdio_regmap_write_c22; > > > + > > > + if (config->autoscan) > > > + mii->phy_mask = ~BIT(config->valid_addr); > > > + else > > > + mii->phy_mask = ~0UL; > > > > Hi Maxime, > > > > phy_mask is a u32. > > But 0UL may be either 32 or 64 bits wide. > > Right > > > I think a better approach would be to use U32_MAX. > > I guess ~0 would also work, and this would also align with what > fixed-phy and sfp do for their internal MDIO bus. Yes, I guess so too. > I'll fix that for next revision Thanks!
diff --git a/MAINTAINERS b/MAINTAINERS index c25172d6471a..ef8362aa93b3 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12840,6 +12840,13 @@ F: Documentation/devicetree/bindings/net/ieee802154/mcr20a.txt F: drivers/net/ieee802154/mcr20a.c F: drivers/net/ieee802154/mcr20a.h +MDIO REGMAP DRIVER +M: Maxime Chevallier <maxime.chevallier@bootlin.com> +L: netdev@vger.kernel.org +S: Maintained +F: drivers/net/mdio/mdio-regmap.c +F: include/linux/mdio/mdio-regmap.h + MEASUREMENT COMPUTING CIO-DAC IIO DRIVER M: William Breathitt Gray <william.gray@linaro.org> L: linux-iio@vger.kernel.org diff --git a/drivers/net/ethernet/altera/Kconfig b/drivers/net/ethernet/altera/Kconfig index dd7fd41ccde5..0a7c0a217536 100644 --- a/drivers/net/ethernet/altera/Kconfig +++ b/drivers/net/ethernet/altera/Kconfig @@ -5,6 +5,8 @@ config ALTERA_TSE select PHYLIB select PHYLINK select PCS_ALTERA_TSE + select MDIO_REGMAP + depends on REGMAP help This driver supports the Altera Triple-Speed (TSE) Ethernet MAC. diff --git a/drivers/net/mdio/Kconfig b/drivers/net/mdio/Kconfig index 9ff2e6f22f3f..aef39c89cf44 100644 --- a/drivers/net/mdio/Kconfig +++ b/drivers/net/mdio/Kconfig @@ -185,6 +185,16 @@ config MDIO_IPQ8064 This driver supports the MDIO interface found in the network interface units of the IPQ8064 SoC +config MDIO_REGMAP + tristate + help + This driver allows using MDIO devices that are not sitting on a + regular MDIO bus, but still exposes the standard 802.3 register + layout. It's regmap-based so that it can be used on integrated, + memory-mapped PHYs, SPI PHYs and so on. A new virtual MDIO bus is + created, and its read/write operations are mapped to the underlying + regmap. + config MDIO_THUNDER tristate "ThunderX SOCs MDIO buses" depends on 64BIT diff --git a/drivers/net/mdio/Makefile b/drivers/net/mdio/Makefile index 7d4cb4c11e4e..1015f0db4531 100644 --- a/drivers/net/mdio/Makefile +++ b/drivers/net/mdio/Makefile @@ -19,6 +19,7 @@ obj-$(CONFIG_MDIO_MOXART) += mdio-moxart.o obj-$(CONFIG_MDIO_MSCC_MIIM) += mdio-mscc-miim.o obj-$(CONFIG_MDIO_MVUSB) += mdio-mvusb.o obj-$(CONFIG_MDIO_OCTEON) += mdio-octeon.o +obj-$(CONFIG_MDIO_REGMAP) += mdio-regmap.o obj-$(CONFIG_MDIO_SUN4I) += mdio-sun4i.o obj-$(CONFIG_MDIO_THUNDER) += mdio-thunder.o obj-$(CONFIG_MDIO_XGENE) += mdio-xgene.o diff --git a/drivers/net/mdio/mdio-regmap.c b/drivers/net/mdio/mdio-regmap.c new file mode 100644 index 000000000000..d7ff946d6088 --- /dev/null +++ b/drivers/net/mdio/mdio-regmap.c @@ -0,0 +1,90 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* Driver for MMIO-Mapped MDIO devices. Some IPs expose internal PHYs or PCS + * within the MMIO-mapped area + * + * Copyright (C) 2023 Maxime Chevallier <maxime.chevallier@bootlin.com> + */ +#include <linux/bitfield.h> +#include <linux/delay.h> +#include <linux/mdio.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_mdio.h> +#include <linux/phy.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/mdio/mdio-regmap.h> + +#define DRV_NAME "mdio-regmap" + +static int mdio_regmap_read_c22(struct mii_bus *bus, int addr, int regnum) +{ + struct mdio_regmap_config *ctx = bus->priv; + unsigned int val; + int ret; + + if (ctx->valid_addr != addr) + return -ENODEV; + + ret = regmap_read(ctx->regmap, regnum, &val); + if (ret < 0) + return ret; + + return val; +} + +static int mdio_regmap_write_c22(struct mii_bus *bus, int addr, int regnum, + u16 val) +{ + struct mdio_regmap_config *ctx = bus->priv; + + if (ctx->valid_addr != addr) + return -ENODEV; + + return regmap_write(ctx->regmap, regnum, val); +} + +struct mii_bus *devm_mdio_regmap_register(struct device *dev, + const struct mdio_regmap_config *config) +{ + struct mdio_regmap_config *mrc; + struct mii_bus *mii; + int rc; + + if (!config->parent) + return ERR_PTR(-EINVAL); + + mii = devm_mdiobus_alloc_size(config->parent, sizeof(*mrc)); + if (!mii) + return ERR_PTR(-ENOMEM); + + mrc = mii->priv; + memcpy(mrc, config, sizeof(*mrc)); + + mrc->regmap = config->regmap; + mrc->valid_addr = config->valid_addr; + + mii->name = DRV_NAME; + strscpy(mii->id, config->name, MII_BUS_ID_SIZE); + mii->parent = config->parent; + mii->read = mdio_regmap_read_c22; + mii->write = mdio_regmap_write_c22; + + if (config->autoscan) + mii->phy_mask = ~BIT(config->valid_addr); + else + mii->phy_mask = ~0UL; + + rc = devm_mdiobus_register(dev, mii); + if (rc) { + dev_err(config->parent, "Cannot register MDIO bus![%s] (%d)\n", mii->id, rc); + return ERR_PTR(rc); + } + + return mii; +} +EXPORT_SYMBOL_GPL(devm_mdio_regmap_register); + +MODULE_DESCRIPTION("MDIO API over regmap"); +MODULE_AUTHOR("Maxime Chevallier <maxime.chevallier@bootlin.com>"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/mdio/mdio-regmap.h b/include/linux/mdio/mdio-regmap.h new file mode 100644 index 000000000000..b8508f152552 --- /dev/null +++ b/include/linux/mdio/mdio-regmap.h @@ -0,0 +1,24 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Driver for MMIO-Mapped MDIO devices. Some IPs expose internal PHYs or PCS + * within the MMIO-mapped area + * + * Copyright (C) 2023 Maxime Chevallier <maxime.chevallier@bootlin.com> + */ +#ifndef MDIO_REGMAP_H +#define MDIO_REGMAP_H + +struct device; +struct regmap; + +struct mdio_regmap_config { + struct device *parent; + struct regmap *regmap; + char name[MII_BUS_ID_SIZE]; + u8 valid_addr; + bool autoscan; +}; + +struct mii_bus *devm_mdio_regmap_register(struct device *dev, + const struct mdio_regmap_config *config); + +#endif
There exists several examples today of devices that embed an ethernet PHY or PCS directly inside an SoC. In this situation, either the device is controlled through a vendor-specific register set, or sometimes exposes the standard 802.3 registers that are typically accessed over MDIO. As phylib and phylink are designed to use mdiodevices, this driver allows creating a virtual MDIO bus, that translates mdiodev register accesses to regmap accesses. The reason we use regmap is because there are at least 3 such devices known today, 2 of them are Altera TSE PCS's, memory-mapped, exposed with a 4-byte stride in stmmac's dwmac-socfpga variant, and a 2-byte stride in altera-tse. The other one (nxp,sja1110-base-tx-mdio) is exposed over SPI. Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> --- V1->V2 : - Use phy_mask to avoid unnecessary scanning, suggested by Andrew - Allow entirely disabling scanning, suggested by Vlad MAINTAINERS | 7 +++ drivers/net/ethernet/altera/Kconfig | 2 + drivers/net/mdio/Kconfig | 10 ++++ drivers/net/mdio/Makefile | 1 + drivers/net/mdio/mdio-regmap.c | 90 +++++++++++++++++++++++++++++ include/linux/mdio/mdio-regmap.h | 24 ++++++++ 6 files changed, 134 insertions(+) create mode 100644 drivers/net/mdio/mdio-regmap.c create mode 100644 include/linux/mdio/mdio-regmap.h