linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next v2 0/4] net: add a regmap-based mdio driver and drop TSE PCS
@ 2023-05-25 10:11 Maxime Chevallier
  2023-05-25 10:11 ` [PATCH net-next v2 1/4] net: mdio: Introduce a regmap-based mdio driver Maxime Chevallier
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Maxime Chevallier @ 2023-05-25 10:11 UTC (permalink / raw)
  To: Mark Brown, davem
  Cc: Maxime Chevallier, netdev, linux-kernel, alexis.lothore,
	thomas.petazzoni, Andrew Lunn, Jakub Kicinski, Eric Dumazet,
	Paolo Abeni, Florian Fainelli, Heiner Kallweit, Russell King,
	Vladimir Oltean, Ioana Ciornei, linux-stm32, linux-arm-kernel,
	Maxime Coquelin, Jose Abreu, Alexandre Torgue,
	Giuseppe Cavallaro

Hello everyone,

This is the V2 of a series that follows-up on the work [1] aiming to drop the
altera TSE PCS driver, as it turns out to be a version of the Lynx PCS exposed
as a memory-mapped block, instead of living on an MDIO bus.

One step of this removal involved creating a regmap-based mdio driver
that translates MDIO accesses into the actual underlying bus that
exposes the register. The register layout must of course match the
standard MDIO layout, but we can now account for differences in stride
with recent work on the regmap subsystem [2].

Mark, Net maintainers, this series depends on the patch e12ff2876493 that was
recently merged into the regmap tree [3].

For this series to be usable in net-next, this patch must be applied
beforehand. Should Mark create a tag that would then be merged into
net-next ?

This series introduces a new MDIO driver, and uses it to convert Altera
TSE from the actual TSE PCS driver to Lynx PCS.

Since it turns out dwmac_socfpga also uses a TSE PCS block, port that
driver to Lynx as well.

Changes in V2 :
 - Use phy_mask to avoid unnecessarily scanning the whole mdio bus
 - Go one step further and completely disable scanning if users
   set the .autoscan flag to false, in case the mdiodevice isn't an
   actual PHY (a PCS for example).

Thanks,

Maxime

[1] : https://lore.kernel.org/all/20230324093644.464704-1-maxime.chevallier@bootlin.com/
[2] : https://lore.kernel.org/all/20230407152604.105467-1-maxime.chevallier@bootlin.com/#t
[3] : https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git/commit/?id=e12ff28764937dd58c8613f16065da60da149048



Maxime Chevallier (4):
  net: mdio: Introduce a regmap-based mdio driver
  net: ethernet: altera-tse: Convert to mdio-regmap and use PCS Lynx
  net: pcs: Drop the TSE PCS driver
  net: stmmac: dwmac-sogfpga: use the lynx pcs driver

 MAINTAINERS                                   |  14 +-
 drivers/net/ethernet/altera/Kconfig           |   2 +
 drivers/net/ethernet/altera/altera_tse.h      |   1 +
 drivers/net/ethernet/altera/altera_tse_main.c |  57 +++-
 drivers/net/ethernet/stmicro/stmmac/Kconfig   |   1 +
 drivers/net/ethernet/stmicro/stmmac/Makefile  |   2 +-
 .../ethernet/stmicro/stmmac/altr_tse_pcs.c    | 257 ------------------
 .../ethernet/stmicro/stmmac/altr_tse_pcs.h    |  29 --
 drivers/net/ethernet/stmicro/stmmac/common.h  |   1 +
 .../ethernet/stmicro/stmmac/dwmac-socfpga.c   |  90 ++++--
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |  12 +-
 drivers/net/mdio/Kconfig                      |  10 +
 drivers/net/mdio/Makefile                     |   1 +
 drivers/net/mdio/mdio-regmap.c                |  90 ++++++
 drivers/net/pcs/Kconfig                       |   6 -
 drivers/net/pcs/Makefile                      |   1 -
 drivers/net/pcs/pcs-altera-tse.c              | 160 -----------
 include/linux/mdio/mdio-regmap.h              |  26 ++
 include/linux/pcs-altera-tse.h                |  17 --
 19 files changed, 264 insertions(+), 513 deletions(-)
 delete mode 100644 drivers/net/ethernet/stmicro/stmmac/altr_tse_pcs.c
 delete mode 100644 drivers/net/ethernet/stmicro/stmmac/altr_tse_pcs.h
 create mode 100644 drivers/net/mdio/mdio-regmap.c
 delete mode 100644 drivers/net/pcs/pcs-altera-tse.c
 create mode 100644 include/linux/mdio/mdio-regmap.h
 delete mode 100644 include/linux/pcs-altera-tse.h

-- 
2.40.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH net-next v2 1/4] net: mdio: Introduce a regmap-based mdio driver
  2023-05-25 10:11 [PATCH net-next v2 0/4] net: add a regmap-based mdio driver and drop TSE PCS Maxime Chevallier
@ 2023-05-25 10:11 ` Maxime Chevallier
  2023-05-25 11:02   ` Simon Horman
  2023-05-25 11:11   ` Russell King (Oracle)
  2023-05-25 10:11 ` [PATCH net-next v2 2/4] net: ethernet: altera-tse: Convert to mdio-regmap and use PCS Lynx Maxime Chevallier
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 10+ messages in thread
From: Maxime Chevallier @ 2023-05-25 10:11 UTC (permalink / raw)
  To: Mark Brown, davem
  Cc: Maxime Chevallier, netdev, linux-kernel, alexis.lothore,
	thomas.petazzoni, Andrew Lunn, Jakub Kicinski, Eric Dumazet,
	Paolo Abeni, Florian Fainelli, Heiner Kallweit, Russell King,
	Vladimir Oltean, Ioana Ciornei, linux-stm32, linux-arm-kernel,
	Maxime Coquelin, Jose Abreu, Alexandre Torgue,
	Giuseppe Cavallaro

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

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
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net-next v2 2/4] net: ethernet: altera-tse: Convert to mdio-regmap and use PCS Lynx
  2023-05-25 10:11 [PATCH net-next v2 0/4] net: add a regmap-based mdio driver and drop TSE PCS Maxime Chevallier
  2023-05-25 10:11 ` [PATCH net-next v2 1/4] net: mdio: Introduce a regmap-based mdio driver Maxime Chevallier
@ 2023-05-25 10:11 ` Maxime Chevallier
  2023-05-25 10:11 ` [PATCH net-next v2 3/4] net: pcs: Drop the TSE PCS driver Maxime Chevallier
  2023-05-25 10:11 ` [PATCH net-next v2 4/4] net: stmmac: dwmac-sogfpga: use the lynx pcs driver Maxime Chevallier
  3 siblings, 0 replies; 10+ messages in thread
From: Maxime Chevallier @ 2023-05-25 10:11 UTC (permalink / raw)
  To: Mark Brown, davem
  Cc: Maxime Chevallier, netdev, linux-kernel, alexis.lothore,
	thomas.petazzoni, Andrew Lunn, Jakub Kicinski, Eric Dumazet,
	Paolo Abeni, Florian Fainelli, Heiner Kallweit, Russell King,
	Vladimir Oltean, Ioana Ciornei, linux-stm32, linux-arm-kernel,
	Maxime Coquelin, Jose Abreu, Alexandre Torgue,
	Giuseppe Cavallaro

The newly introduced regmap-based MDIO driver allows for an easy mapping
of an mdiodevice onto the memory-mapped TSE PCS, which is actually a
Lynx PCS.

Convert Altera TSE to use this PCS instead of the pcs-altera-tse, which
is nothing more than a memory-mapped Lynx PCS.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V1->V2 : No changes

 drivers/net/ethernet/altera/altera_tse.h      |  1 +
 drivers/net/ethernet/altera/altera_tse_main.c | 57 +++++++++++++++++--
 include/linux/mdio/mdio-regmap.h              |  2 +
 3 files changed, 54 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/altera/altera_tse.h b/drivers/net/ethernet/altera/altera_tse.h
index db5eed06e92d..d50cf440d01b 100644
--- a/drivers/net/ethernet/altera/altera_tse.h
+++ b/drivers/net/ethernet/altera/altera_tse.h
@@ -477,6 +477,7 @@ struct altera_tse_private {
 	struct phylink *phylink;
 	struct phylink_config phylink_config;
 	struct phylink_pcs *pcs;
+	struct mdio_device *pcs_mdiodev;
 };
 
 /* Function prototypes
diff --git a/drivers/net/ethernet/altera/altera_tse_main.c b/drivers/net/ethernet/altera/altera_tse_main.c
index 190ff1bcd94e..66db6a7d0b22 100644
--- a/drivers/net/ethernet/altera/altera_tse_main.c
+++ b/drivers/net/ethernet/altera/altera_tse_main.c
@@ -27,14 +27,16 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/mii.h>
+#include <linux/mdio/mdio-regmap.h>
 #include <linux/netdevice.h>
 #include <linux/of_device.h>
 #include <linux/of_mdio.h>
 #include <linux/of_net.h>
 #include <linux/of_platform.h>
-#include <linux/pcs-altera-tse.h>
+#include <linux/pcs-lynx.h>
 #include <linux/phy.h>
 #include <linux/platform_device.h>
+#include <linux/regmap.h>
 #include <linux/skbuff.h>
 #include <asm/cacheflush.h>
 
@@ -1134,13 +1136,21 @@ static int altera_tse_probe(struct platform_device *pdev)
 	const struct of_device_id *of_id = NULL;
 	struct altera_tse_private *priv;
 	struct resource *control_port;
+	struct regmap *pcs_regmap;
 	struct resource *dma_res;
 	struct resource *pcs_res;
+	struct mii_bus *pcs_bus;
 	struct net_device *ndev;
 	void __iomem *descmap;
-	int pcs_reg_width = 2;
 	int ret = -ENODEV;
 
+	struct regmap_config pcs_regmap_cfg;
+
+	struct mdio_regmap_config mrc = {
+		.parent = &pdev->dev,
+		.valid_addr = 0x0,
+	};
+
 	ndev = alloc_etherdev(sizeof(struct altera_tse_private));
 	if (!ndev) {
 		dev_err(&pdev->dev, "Could not allocate network device\n");
@@ -1258,10 +1268,29 @@ static int altera_tse_probe(struct platform_device *pdev)
 	ret = request_and_map(pdev, "pcs", &pcs_res,
 			      &priv->pcs_base);
 	if (ret) {
+		/* If we can't find a dedicated resource for the PCS, fallback
+		 * to the internal PCS, that has a different address stride
+		 */
 		priv->pcs_base = priv->mac_dev + tse_csroffs(mdio_phy0);
-		pcs_reg_width = 4;
+		pcs_regmap_cfg.reg_bits = 32;
+		/* Values are MDIO-like values, on 16 bits */
+		pcs_regmap_cfg.val_bits = 16;
+		pcs_regmap_cfg.reg_shift = REGMAP_UPSHIFT(2);
+	} else {
+		pcs_regmap_cfg.reg_bits = 16;
+		pcs_regmap_cfg.val_bits = 16;
+		pcs_regmap_cfg.reg_shift = REGMAP_UPSHIFT(1);
 	}
 
+	/* Create a regmap for the PCS so that it can be used by the PCS driver */
+	pcs_regmap = devm_regmap_init_mmio(&pdev->dev, priv->pcs_base,
+					   &pcs_regmap_cfg);
+	if (IS_ERR(pcs_regmap)) {
+		ret = PTR_ERR(pcs_regmap);
+		goto err_free_netdev;
+	}
+	mrc.regmap = pcs_regmap;
+
 	/* Rx IRQ */
 	priv->rx_irq = platform_get_irq_byname(pdev, "rx_irq");
 	if (priv->rx_irq == -ENXIO) {
@@ -1384,7 +1413,20 @@ 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);
+	snprintf(mrc.name, MII_BUS_ID_SIZE, "%s-pcs-mii", ndev->name);
+	pcs_bus = devm_mdio_regmap_register(&pdev->dev, &mrc);
+	if (IS_ERR(pcs_bus)) {
+		ret = PTR_ERR(pcs_bus);
+		goto err_init_phy;
+	}
+
+	priv->pcs_mdiodev = mdio_device_create(pcs_bus, 0);
+
+	priv->pcs = lynx_pcs_create(priv->pcs_mdiodev);
+	if (!priv->pcs) {
+		ret = -ENODEV;
+		goto err_init_phy;
+	}
 
 	priv->phylink_config.dev = &ndev->dev;
 	priv->phylink_config.type = PHYLINK_NETDEV;
@@ -1407,11 +1449,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:
+	mdio_device_free(priv->pcs_mdiodev);
 err_init_phy:
 	unregister_netdev(ndev);
 err_register_netdev:
@@ -1433,6 +1476,8 @@ static int altera_tse_remove(struct platform_device *pdev)
 	altera_tse_mdio_destroy(ndev);
 	unregister_netdev(ndev);
 	phylink_destroy(priv->phylink);
+	mdio_device_free(priv->pcs_mdiodev);
+
 	free_netdev(ndev);
 
 	return 0;
diff --git a/include/linux/mdio/mdio-regmap.h b/include/linux/mdio/mdio-regmap.h
index b8508f152552..679d9069846b 100644
--- a/include/linux/mdio/mdio-regmap.h
+++ b/include/linux/mdio/mdio-regmap.h
@@ -7,6 +7,8 @@
 #ifndef MDIO_REGMAP_H
 #define MDIO_REGMAP_H
 
+#include <linux/phy.h>
+
 struct device;
 struct regmap;
 
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net-next v2 3/4] net: pcs: Drop the TSE PCS driver
  2023-05-25 10:11 [PATCH net-next v2 0/4] net: add a regmap-based mdio driver and drop TSE PCS Maxime Chevallier
  2023-05-25 10:11 ` [PATCH net-next v2 1/4] net: mdio: Introduce a regmap-based mdio driver Maxime Chevallier
  2023-05-25 10:11 ` [PATCH net-next v2 2/4] net: ethernet: altera-tse: Convert to mdio-regmap and use PCS Lynx Maxime Chevallier
@ 2023-05-25 10:11 ` Maxime Chevallier
  2023-05-25 10:11 ` [PATCH net-next v2 4/4] net: stmmac: dwmac-sogfpga: use the lynx pcs driver Maxime Chevallier
  3 siblings, 0 replies; 10+ messages in thread
From: Maxime Chevallier @ 2023-05-25 10:11 UTC (permalink / raw)
  To: Mark Brown, davem
  Cc: Maxime Chevallier, netdev, linux-kernel, alexis.lothore,
	thomas.petazzoni, Andrew Lunn, Jakub Kicinski, Eric Dumazet,
	Paolo Abeni, Florian Fainelli, Heiner Kallweit, Russell King,
	Vladimir Oltean, Ioana Ciornei, linux-stm32, linux-arm-kernel,
	Maxime Coquelin, Jose Abreu, Alexandre Torgue,
	Giuseppe Cavallaro

Now that we can easily create a mdio-device that represents a
memory-mapped device that exposes an MDIO-like register layout, we don't
need the Altera TSE PCS anymore, since we can use the Lynx PCS instead.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V1->V2 : No changes

 MAINTAINERS                      |   7 --
 drivers/net/pcs/Kconfig          |   6 --
 drivers/net/pcs/Makefile         |   1 -
 drivers/net/pcs/pcs-altera-tse.c | 160 -------------------------------
 include/linux/pcs-altera-tse.h   |  17 ----
 5 files changed, 191 deletions(-)
 delete mode 100644 drivers/net/pcs/pcs-altera-tse.c
 delete mode 100644 include/linux/pcs-altera-tse.h

diff --git a/MAINTAINERS b/MAINTAINERS
index ef8362aa93b3..629f18fcac42 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -909,13 +909,6 @@ L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/ethernet/altera/
 
-ALTERA TSE PCS
-M:	Maxime Chevallier <maxime.chevallier@bootlin.com>
-L:	netdev@vger.kernel.org
-S:	Supported
-F:	drivers/net/pcs/pcs-altera-tse.c
-F:	include/linux/pcs-altera-tse.h
-
 ALTERA UART/JTAG UART SERIAL DRIVERS
 M:	Tobias Klauser <tklauser@distanz.ch>
 L:	linux-serial@vger.kernel.org
diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index 7c34fb7cbf7b..87cf308fc6d8 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -33,10 +33,4 @@ config PCS_RZN1_MIIC
 	  on RZ/N1 SoCs. This PCS converts MII to RMII/RGMII or can be set in
 	  pass-through mode for MII.
 
-config PCS_ALTERA_TSE
-	tristate
-	help
-	  This module provides helper functions for the Altera Triple Speed
-	  Ethernet SGMII PCS, that can be found on the Intel Socfpga family.
-
 endmenu
diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
index 9b9afd6b1c22..ea662a7989b2 100644
--- a/drivers/net/pcs/Makefile
+++ b/drivers/net/pcs/Makefile
@@ -7,4 +7,3 @@ obj-$(CONFIG_PCS_XPCS)		+= pcs_xpcs.o
 obj-$(CONFIG_PCS_LYNX)		+= pcs-lynx.o
 obj-$(CONFIG_PCS_MTK_LYNXI)	+= pcs-mtk-lynxi.o
 obj-$(CONFIG_PCS_RZN1_MIIC)	+= pcs-rzn1-miic.o
-obj-$(CONFIG_PCS_ALTERA_TSE)	+= pcs-altera-tse.o
diff --git a/drivers/net/pcs/pcs-altera-tse.c b/drivers/net/pcs/pcs-altera-tse.c
deleted file mode 100644
index d616749761f4..000000000000
--- a/drivers/net/pcs/pcs-altera-tse.c
+++ /dev/null
@@ -1,160 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Copyright (C) 2022 Bootlin
- *
- * Maxime Chevallier <maxime.chevallier@bootlin.com>
- */
-
-#include <linux/netdevice.h>
-#include <linux/phy.h>
-#include <linux/phylink.h>
-#include <linux/pcs-altera-tse.h>
-
-/* SGMII PCS register addresses
- */
-#define SGMII_PCS_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)
-{
-	return container_of(pcs, struct altera_tse_pcs, pcs);
-}
-
-static u16 tse_pcs_read(struct altera_tse_pcs *tse_pcs, int regnum)
-{
-	if (tse_pcs->reg_width == 4)
-		return readl(tse_pcs->base + regnum * 4);
-	else
-		return readw(tse_pcs->base + regnum * 2);
-}
-
-static void tse_pcs_write(struct altera_tse_pcs *tse_pcs, int regnum,
-			  u16 value)
-{
-	if (tse_pcs->reg_width == 4)
-		writel(value, tse_pcs->base + regnum * 4);
-	else
-		writew(value, tse_pcs->base + regnum * 2);
-}
-
-static int tse_pcs_reset(struct altera_tse_pcs *tse_pcs)
-{
-	u16 bmcr;
-
-	/* Reset PCS block */
-	bmcr = tse_pcs_read(tse_pcs, MII_BMCR);
-	bmcr |= BMCR_RESET;
-	tse_pcs_write(tse_pcs, MII_BMCR, bmcr);
-
-	return read_poll_timeout(tse_pcs_read, bmcr, (bmcr & BMCR_RESET),
-				 10, SGMII_PCS_SW_RESET_TIMEOUT, 1,
-				 tse_pcs, MII_BMCR);
-}
-
-static int alt_tse_pcs_validate(struct phylink_pcs *pcs,
-				unsigned long *supported,
-				const struct phylink_link_state *state)
-{
-	if (state->interface == PHY_INTERFACE_MODE_SGMII ||
-	    state->interface == PHY_INTERFACE_MODE_1000BASEX)
-		return 1;
-
-	return -EINVAL;
-}
-
-static int alt_tse_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
-			      phy_interface_t interface,
-			      const unsigned long *advertising,
-			      bool permit_pause_to_mac)
-{
-	struct altera_tse_pcs *tse_pcs = phylink_pcs_to_tse_pcs(pcs);
-	u32 ctrl, if_mode;
-
-	ctrl = tse_pcs_read(tse_pcs, MII_BMCR);
-	if_mode = tse_pcs_read(tse_pcs, SGMII_PCS_IF_MODE);
-
-	/* Set link timer to 1.6ms, as per the MegaCore Function User Guide */
-	tse_pcs_write(tse_pcs, SGMII_PCS_LINK_TIMER_0, 0x0D40);
-	tse_pcs_write(tse_pcs, SGMII_PCS_LINK_TIMER_1, 0x03);
-
-	if (interface == PHY_INTERFACE_MODE_SGMII) {
-		if_mode |= PCS_IF_MODE_USE_SGMII_AN | PCS_IF_MODE_SGMII_ENA;
-	} else if (interface == PHY_INTERFACE_MODE_1000BASEX) {
-		if_mode &= ~(PCS_IF_MODE_USE_SGMII_AN | PCS_IF_MODE_SGMII_ENA);
-	}
-
-	ctrl |= (BMCR_SPEED1000 | BMCR_FULLDPLX | BMCR_ANENABLE);
-
-	tse_pcs_write(tse_pcs, MII_BMCR, ctrl);
-	tse_pcs_write(tse_pcs, SGMII_PCS_IF_MODE, if_mode);
-
-	return tse_pcs_reset(tse_pcs);
-}
-
-static void alt_tse_pcs_get_state(struct phylink_pcs *pcs,
-				  struct phylink_link_state *state)
-{
-	struct altera_tse_pcs *tse_pcs = phylink_pcs_to_tse_pcs(pcs);
-	u16 bmsr, lpa;
-
-	bmsr = tse_pcs_read(tse_pcs, MII_BMSR);
-	lpa = tse_pcs_read(tse_pcs, MII_LPA);
-
-	phylink_mii_c22_pcs_decode_state(state, bmsr, lpa);
-}
-
-static void alt_tse_pcs_an_restart(struct phylink_pcs *pcs)
-{
-	struct altera_tse_pcs *tse_pcs = phylink_pcs_to_tse_pcs(pcs);
-	u16 bmcr;
-
-	bmcr = tse_pcs_read(tse_pcs, MII_BMCR);
-	bmcr |= BMCR_ANRESTART;
-	tse_pcs_write(tse_pcs, MII_BMCR, bmcr);
-
-	/* This PCS seems to require a soft reset to re-sync the AN logic */
-	tse_pcs_reset(tse_pcs);
-}
-
-static const struct phylink_pcs_ops alt_tse_pcs_ops = {
-	.pcs_validate = alt_tse_pcs_validate,
-	.pcs_get_state = alt_tse_pcs_get_state,
-	.pcs_config = alt_tse_pcs_config,
-	.pcs_an_restart = alt_tse_pcs_an_restart,
-};
-
-struct phylink_pcs *alt_tse_pcs_create(struct net_device *ndev,
-				       void __iomem *pcs_base, int reg_width)
-{
-	struct altera_tse_pcs *tse_pcs;
-
-	if (reg_width != 4 && reg_width != 2)
-		return ERR_PTR(-EINVAL);
-
-	tse_pcs = devm_kzalloc(&ndev->dev, sizeof(*tse_pcs), GFP_KERNEL);
-	if (!tse_pcs)
-		return ERR_PTR(-ENOMEM);
-
-	tse_pcs->pcs.ops = &alt_tse_pcs_ops;
-	tse_pcs->base = pcs_base;
-	tse_pcs->reg_width = reg_width;
-
-	return &tse_pcs->pcs;
-}
-EXPORT_SYMBOL_GPL(alt_tse_pcs_create);
-
-MODULE_LICENSE("GPL");
-MODULE_DESCRIPTION("Altera TSE PCS driver");
-MODULE_AUTHOR("Maxime Chevallier <maxime.chevallier@bootlin.com>");
diff --git a/include/linux/pcs-altera-tse.h b/include/linux/pcs-altera-tse.h
deleted file mode 100644
index 92ab9f08e835..000000000000
--- a/include/linux/pcs-altera-tse.h
+++ /dev/null
@@ -1,17 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- * Copyright (C) 2022 Bootlin
- *
- * Maxime Chevallier <maxime.chevallier@bootlin.com>
- */
-
-#ifndef __LINUX_PCS_ALTERA_TSE_H
-#define __LINUX_PCS_ALTERA_TSE_H
-
-struct phylink_pcs;
-struct net_device;
-
-struct phylink_pcs *alt_tse_pcs_create(struct net_device *ndev,
-				       void __iomem *pcs_base, int reg_width);
-
-#endif /* __LINUX_PCS_ALTERA_TSE_H */
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net-next v2 4/4] net: stmmac: dwmac-sogfpga: use the lynx pcs driver
  2023-05-25 10:11 [PATCH net-next v2 0/4] net: add a regmap-based mdio driver and drop TSE PCS Maxime Chevallier
                   ` (2 preceding siblings ...)
  2023-05-25 10:11 ` [PATCH net-next v2 3/4] net: pcs: Drop the TSE PCS driver Maxime Chevallier
@ 2023-05-25 10:11 ` Maxime Chevallier
  3 siblings, 0 replies; 10+ messages in thread
From: Maxime Chevallier @ 2023-05-25 10:11 UTC (permalink / raw)
  To: Mark Brown, davem
  Cc: Maxime Chevallier, netdev, linux-kernel, alexis.lothore,
	thomas.petazzoni, Andrew Lunn, Jakub Kicinski, Eric Dumazet,
	Paolo Abeni, Florian Fainelli, Heiner Kallweit, Russell King,
	Vladimir Oltean, Ioana Ciornei, linux-stm32, linux-arm-kernel,
	Maxime Coquelin, Jose Abreu, Alexandre Torgue,
	Giuseppe Cavallaro

dwmac_socfpga re-implements support for the TSE PCS, which is identical
to the already existing TSE PCS, which in turn is the same as the Lynx
PCS. Drop the existing TSE re-implemenation and use the Lynx PCS
instead, relying on the regmap-mdio driver to translate MDIO accesses
into mmio accesses.

Instead of extending xpcs, allow using a generic phylink_pcs, populated
by lynx_pcs_create(), and use .mac_select_pcs() to return the relevant
PCS to be used.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V1->V2 : No changes

 drivers/net/ethernet/stmicro/stmmac/Kconfig   |   1 +
 drivers/net/ethernet/stmicro/stmmac/Makefile  |   2 +-
 .../ethernet/stmicro/stmmac/altr_tse_pcs.c    | 257 ------------------
 .../ethernet/stmicro/stmmac/altr_tse_pcs.h    |  29 --
 drivers/net/ethernet/stmicro/stmmac/common.h  |   1 +
 .../ethernet/stmicro/stmmac/dwmac-socfpga.c   |  90 ++++--
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |  12 +-
 7 files changed, 76 insertions(+), 316 deletions(-)
 delete mode 100644 drivers/net/ethernet/stmicro/stmmac/altr_tse_pcs.c
 delete mode 100644 drivers/net/ethernet/stmicro/stmmac/altr_tse_pcs.h

diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index 5f5a997f21f3..62b484cca1c3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -158,6 +158,7 @@ config DWMAC_SOCFPGA
 	default ARCH_INTEL_SOCFPGA
 	depends on OF && (ARCH_INTEL_SOCFPGA || COMPILE_TEST)
 	select MFD_SYSCON
+	select PCS_LYNX
 	help
 	  Support for ethernet controller on Altera SOCFPGA
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
index 8738fdbb4b2d..7dd3d388068b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Makefile
+++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
@@ -35,7 +35,7 @@ obj-$(CONFIG_DWMAC_IMX8)	+= dwmac-imx.o
 obj-$(CONFIG_DWMAC_TEGRA)	+= dwmac-tegra.o
 obj-$(CONFIG_DWMAC_VISCONTI)	+= dwmac-visconti.o
 stmmac-platform-objs:= stmmac_platform.o
-dwmac-altr-socfpga-objs := altr_tse_pcs.o dwmac-socfpga.o
+dwmac-altr-socfpga-objs := dwmac-socfpga.o
 
 obj-$(CONFIG_STMMAC_PCI)	+= stmmac-pci.o
 obj-$(CONFIG_DWMAC_INTEL)	+= dwmac-intel.o
diff --git a/drivers/net/ethernet/stmicro/stmmac/altr_tse_pcs.c b/drivers/net/ethernet/stmicro/stmmac/altr_tse_pcs.c
deleted file mode 100644
index 00f6d347eaf7..000000000000
--- a/drivers/net/ethernet/stmicro/stmmac/altr_tse_pcs.c
+++ /dev/null
@@ -1,257 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright Altera Corporation (C) 2016. All rights reserved.
- *
- * Author: Tien Hock Loh <thloh@altera.com>
- */
-
-#include <linux/mfd/syscon.h>
-#include <linux/of.h>
-#include <linux/of_address.h>
-#include <linux/of_net.h>
-#include <linux/phy.h>
-#include <linux/regmap.h>
-#include <linux/reset.h>
-#include <linux/stmmac.h>
-
-#include "stmmac.h"
-#include "stmmac_platform.h"
-#include "altr_tse_pcs.h"
-
-#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII	0
-#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RGMII		BIT(1)
-#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RMII		BIT(2)
-#define SYSMGR_EMACGRP_CTRL_PHYSEL_WIDTH		2
-#define SYSMGR_EMACGRP_CTRL_PHYSEL_MASK			GENMASK(1, 0)
-
-#define TSE_PCS_CONTROL_AN_EN_MASK			BIT(12)
-#define TSE_PCS_CONTROL_REG				0x00
-#define TSE_PCS_CONTROL_RESTART_AN_MASK			BIT(9)
-#define TSE_PCS_CTRL_AUTONEG_SGMII			0x1140
-#define TSE_PCS_IF_MODE_REG				0x28
-#define TSE_PCS_LINK_TIMER_0_REG			0x24
-#define TSE_PCS_LINK_TIMER_1_REG			0x26
-#define TSE_PCS_SIZE					0x40
-#define TSE_PCS_STATUS_AN_COMPLETED_MASK		BIT(5)
-#define TSE_PCS_STATUS_LINK_MASK			0x0004
-#define TSE_PCS_STATUS_REG				0x02
-#define TSE_PCS_SGMII_SPEED_1000			BIT(3)
-#define TSE_PCS_SGMII_SPEED_100				BIT(2)
-#define TSE_PCS_SGMII_SPEED_10				0x0
-#define TSE_PCS_SW_RST_MASK				0x8000
-#define TSE_PCS_PARTNER_ABILITY_REG			0x0A
-#define TSE_PCS_PARTNER_DUPLEX_FULL			0x1000
-#define TSE_PCS_PARTNER_DUPLEX_HALF			0x0000
-#define TSE_PCS_PARTNER_DUPLEX_MASK			0x1000
-#define TSE_PCS_PARTNER_SPEED_MASK			GENMASK(11, 10)
-#define TSE_PCS_PARTNER_SPEED_1000			BIT(11)
-#define TSE_PCS_PARTNER_SPEED_100			BIT(10)
-#define TSE_PCS_PARTNER_SPEED_10			0x0000
-#define TSE_PCS_PARTNER_SPEED_1000			BIT(11)
-#define TSE_PCS_PARTNER_SPEED_100			BIT(10)
-#define TSE_PCS_PARTNER_SPEED_10			0x0000
-#define TSE_PCS_SGMII_SPEED_MASK			GENMASK(3, 2)
-#define TSE_PCS_SGMII_LINK_TIMER_0			0x0D40
-#define TSE_PCS_SGMII_LINK_TIMER_1			0x0003
-#define TSE_PCS_SW_RESET_TIMEOUT			100
-#define TSE_PCS_USE_SGMII_AN_MASK			BIT(1)
-#define TSE_PCS_USE_SGMII_ENA				BIT(0)
-#define TSE_PCS_IF_USE_SGMII				0x03
-
-#define AUTONEGO_LINK_TIMER				20
-
-static int tse_pcs_reset(void __iomem *base, struct tse_pcs *pcs)
-{
-	int counter = 0;
-	u16 val;
-
-	val = readw(base + TSE_PCS_CONTROL_REG);
-	val |= TSE_PCS_SW_RST_MASK;
-	writew(val, base + TSE_PCS_CONTROL_REG);
-
-	while (counter < TSE_PCS_SW_RESET_TIMEOUT) {
-		val = readw(base + TSE_PCS_CONTROL_REG);
-		val &= TSE_PCS_SW_RST_MASK;
-		if (val == 0)
-			break;
-		counter++;
-		udelay(1);
-	}
-	if (counter >= TSE_PCS_SW_RESET_TIMEOUT) {
-		dev_err(pcs->dev, "PCS could not get out of sw reset\n");
-		return -ETIMEDOUT;
-	}
-
-	return 0;
-}
-
-int tse_pcs_init(void __iomem *base, struct tse_pcs *pcs)
-{
-	int ret = 0;
-
-	writew(TSE_PCS_IF_USE_SGMII, base + TSE_PCS_IF_MODE_REG);
-
-	writew(TSE_PCS_CTRL_AUTONEG_SGMII, base + TSE_PCS_CONTROL_REG);
-
-	writew(TSE_PCS_SGMII_LINK_TIMER_0, base + TSE_PCS_LINK_TIMER_0_REG);
-	writew(TSE_PCS_SGMII_LINK_TIMER_1, base + TSE_PCS_LINK_TIMER_1_REG);
-
-	ret = tse_pcs_reset(base, pcs);
-	if (ret == 0)
-		writew(SGMII_ADAPTER_ENABLE,
-		       pcs->sgmii_adapter_base + SGMII_ADAPTER_CTRL_REG);
-
-	return ret;
-}
-
-static void pcs_link_timer_callback(struct tse_pcs *pcs)
-{
-	u16 val = 0;
-	void __iomem *tse_pcs_base = pcs->tse_pcs_base;
-	void __iomem *sgmii_adapter_base = pcs->sgmii_adapter_base;
-
-	val = readw(tse_pcs_base + TSE_PCS_STATUS_REG);
-	val &= TSE_PCS_STATUS_LINK_MASK;
-
-	if (val != 0) {
-		dev_dbg(pcs->dev, "Adapter: Link is established\n");
-		writew(SGMII_ADAPTER_ENABLE,
-		       sgmii_adapter_base + SGMII_ADAPTER_CTRL_REG);
-	} else {
-		mod_timer(&pcs->aneg_link_timer, jiffies +
-			  msecs_to_jiffies(AUTONEGO_LINK_TIMER));
-	}
-}
-
-static void auto_nego_timer_callback(struct tse_pcs *pcs)
-{
-	u16 val = 0;
-	u16 speed = 0;
-	u16 duplex = 0;
-	void __iomem *tse_pcs_base = pcs->tse_pcs_base;
-	void __iomem *sgmii_adapter_base = pcs->sgmii_adapter_base;
-
-	val = readw(tse_pcs_base + TSE_PCS_STATUS_REG);
-	val &= TSE_PCS_STATUS_AN_COMPLETED_MASK;
-
-	if (val != 0) {
-		dev_dbg(pcs->dev, "Adapter: Auto Negotiation is completed\n");
-		val = readw(tse_pcs_base + TSE_PCS_PARTNER_ABILITY_REG);
-		speed = val & TSE_PCS_PARTNER_SPEED_MASK;
-		duplex = val & TSE_PCS_PARTNER_DUPLEX_MASK;
-
-		if (speed == TSE_PCS_PARTNER_SPEED_10 &&
-		    duplex == TSE_PCS_PARTNER_DUPLEX_FULL)
-			dev_dbg(pcs->dev,
-				"Adapter: Link Partner is Up - 10/Full\n");
-		else if (speed == TSE_PCS_PARTNER_SPEED_100 &&
-			 duplex == TSE_PCS_PARTNER_DUPLEX_FULL)
-			dev_dbg(pcs->dev,
-				"Adapter: Link Partner is Up - 100/Full\n");
-		else if (speed == TSE_PCS_PARTNER_SPEED_1000 &&
-			 duplex == TSE_PCS_PARTNER_DUPLEX_FULL)
-			dev_dbg(pcs->dev,
-				"Adapter: Link Partner is Up - 1000/Full\n");
-		else if (speed == TSE_PCS_PARTNER_SPEED_10 &&
-			 duplex == TSE_PCS_PARTNER_DUPLEX_HALF)
-			dev_err(pcs->dev,
-				"Adapter does not support Half Duplex\n");
-		else if (speed == TSE_PCS_PARTNER_SPEED_100 &&
-			 duplex == TSE_PCS_PARTNER_DUPLEX_HALF)
-			dev_err(pcs->dev,
-				"Adapter does not support Half Duplex\n");
-		else if (speed == TSE_PCS_PARTNER_SPEED_1000 &&
-			 duplex == TSE_PCS_PARTNER_DUPLEX_HALF)
-			dev_err(pcs->dev,
-				"Adapter does not support Half Duplex\n");
-		else
-			dev_err(pcs->dev,
-				"Adapter: Invalid Partner Speed and Duplex\n");
-
-		if (duplex == TSE_PCS_PARTNER_DUPLEX_FULL &&
-		    (speed == TSE_PCS_PARTNER_SPEED_10 ||
-		     speed == TSE_PCS_PARTNER_SPEED_100 ||
-		     speed == TSE_PCS_PARTNER_SPEED_1000))
-			writew(SGMII_ADAPTER_ENABLE,
-			       sgmii_adapter_base + SGMII_ADAPTER_CTRL_REG);
-	} else {
-		val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG);
-		val |= TSE_PCS_CONTROL_RESTART_AN_MASK;
-		writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG);
-
-		tse_pcs_reset(tse_pcs_base, pcs);
-		mod_timer(&pcs->aneg_link_timer, jiffies +
-			  msecs_to_jiffies(AUTONEGO_LINK_TIMER));
-	}
-}
-
-static void aneg_link_timer_callback(struct timer_list *t)
-{
-	struct tse_pcs *pcs = from_timer(pcs, t, aneg_link_timer);
-
-	if (pcs->autoneg == AUTONEG_ENABLE)
-		auto_nego_timer_callback(pcs);
-	else if (pcs->autoneg == AUTONEG_DISABLE)
-		pcs_link_timer_callback(pcs);
-}
-
-void tse_pcs_fix_mac_speed(struct tse_pcs *pcs, struct phy_device *phy_dev,
-			   unsigned int speed)
-{
-	void __iomem *tse_pcs_base = pcs->tse_pcs_base;
-	u32 val;
-
-	pcs->autoneg = phy_dev->autoneg;
-
-	if (phy_dev->autoneg == AUTONEG_ENABLE) {
-		val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG);
-		val |= TSE_PCS_CONTROL_AN_EN_MASK;
-		writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG);
-
-		val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG);
-		val |= TSE_PCS_USE_SGMII_AN_MASK;
-		writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG);
-
-		val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG);
-		val |= TSE_PCS_CONTROL_RESTART_AN_MASK;
-
-		tse_pcs_reset(tse_pcs_base, pcs);
-
-		timer_setup(&pcs->aneg_link_timer, aneg_link_timer_callback,
-			    0);
-		mod_timer(&pcs->aneg_link_timer, jiffies +
-			  msecs_to_jiffies(AUTONEGO_LINK_TIMER));
-	} else if (phy_dev->autoneg == AUTONEG_DISABLE) {
-		val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG);
-		val &= ~TSE_PCS_CONTROL_AN_EN_MASK;
-		writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG);
-
-		val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG);
-		val &= ~TSE_PCS_USE_SGMII_AN_MASK;
-		writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG);
-
-		val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG);
-		val &= ~TSE_PCS_SGMII_SPEED_MASK;
-
-		switch (speed) {
-		case 1000:
-			val |= TSE_PCS_SGMII_SPEED_1000;
-			break;
-		case 100:
-			val |= TSE_PCS_SGMII_SPEED_100;
-			break;
-		case 10:
-			val |= TSE_PCS_SGMII_SPEED_10;
-			break;
-		default:
-			return;
-		}
-		writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG);
-
-		tse_pcs_reset(tse_pcs_base, pcs);
-
-		timer_setup(&pcs->aneg_link_timer, aneg_link_timer_callback,
-			    0);
-		mod_timer(&pcs->aneg_link_timer, jiffies +
-			  msecs_to_jiffies(AUTONEGO_LINK_TIMER));
-	}
-}
diff --git a/drivers/net/ethernet/stmicro/stmmac/altr_tse_pcs.h b/drivers/net/ethernet/stmicro/stmmac/altr_tse_pcs.h
deleted file mode 100644
index 694ac25ef426..000000000000
--- a/drivers/net/ethernet/stmicro/stmmac/altr_tse_pcs.h
+++ /dev/null
@@ -1,29 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/* Copyright Altera Corporation (C) 2016. All rights reserved.
- *
- * Author: Tien Hock Loh <thloh@altera.com>
- */
-
-#ifndef __TSE_PCS_H__
-#define __TSE_PCS_H__
-
-#include <linux/phy.h>
-#include <linux/timer.h>
-
-#define SGMII_ADAPTER_CTRL_REG		0x00
-#define SGMII_ADAPTER_ENABLE		0x0000
-#define SGMII_ADAPTER_DISABLE		0x0001
-
-struct tse_pcs {
-	struct device *dev;
-	void __iomem *tse_pcs_base;
-	void __iomem *sgmii_adapter_base;
-	struct timer_list aneg_link_timer;
-	int autoneg;
-};
-
-int tse_pcs_init(void __iomem *base, struct tse_pcs *pcs);
-void tse_pcs_fix_mac_speed(struct tse_pcs *pcs, struct phy_device *phy_dev,
-			   unsigned int speed);
-
-#endif /* __TSE_PCS_H__ */
diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index 4ad692c4116c..34751524775a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -519,6 +519,7 @@ struct mac_device_info {
 	const struct stmmac_tc_ops *tc;
 	const struct stmmac_mmc_ops *mmc;
 	struct dw_xpcs *xpcs;
+	struct phylink_pcs *phylink_pcs; /* Generic external PCS */
 	struct mii_regs mii;	/* MII register Addresses */
 	struct mac_link link;
 	void __iomem *pcsr;     /* vpointer to device CSRs */
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
index 6ee050300b31..5f61b33905fc 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
@@ -10,14 +10,14 @@
 #include <linux/of_net.h>
 #include <linux/phy.h>
 #include <linux/regmap.h>
+#include <linux/mdio/mdio-regmap.h>
 #include <linux/reset.h>
 #include <linux/stmmac.h>
+#include <linux/pcs-lynx.h>
 
 #include "stmmac.h"
 #include "stmmac_platform.h"
 
-#include "altr_tse_pcs.h"
-
 #define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII 0x0
 #define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RGMII 0x1
 #define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RMII 0x2
@@ -37,6 +37,10 @@
 #define EMAC_SPLITTER_CTRL_SPEED_100		0x3
 #define EMAC_SPLITTER_CTRL_SPEED_1000		0x0
 
+#define SGMII_ADAPTER_CTRL_REG		0x00
+#define SGMII_ADAPTER_ENABLE		0x0000
+#define SGMII_ADAPTER_DISABLE		0x0001
+
 struct socfpga_dwmac;
 struct socfpga_dwmac_ops {
 	int (*set_phy_mode)(struct socfpga_dwmac *dwmac_priv);
@@ -50,16 +54,18 @@ struct socfpga_dwmac {
 	struct reset_control *stmmac_rst;
 	struct reset_control *stmmac_ocp_rst;
 	void __iomem *splitter_base;
+	void __iomem *tse_pcs_base;
+	void __iomem *sgmii_adapter_base;
 	bool f2h_ptp_ref_clk;
-	struct tse_pcs pcs;
 	const struct socfpga_dwmac_ops *ops;
+	struct mdio_device *pcs_mdiodev;
 };
 
 static void socfpga_dwmac_fix_mac_speed(void *priv, unsigned int speed)
 {
 	struct socfpga_dwmac *dwmac = (struct socfpga_dwmac *)priv;
 	void __iomem *splitter_base = dwmac->splitter_base;
-	void __iomem *sgmii_adapter_base = dwmac->pcs.sgmii_adapter_base;
+	void __iomem *sgmii_adapter_base = dwmac->sgmii_adapter_base;
 	struct device *dev = dwmac->dev;
 	struct net_device *ndev = dev_get_drvdata(dev);
 	struct phy_device *phy_dev = ndev->phydev;
@@ -89,11 +95,9 @@ static void socfpga_dwmac_fix_mac_speed(void *priv, unsigned int speed)
 		writel(val, splitter_base + EMAC_SPLITTER_CTRL_REG);
 	}
 
-	if (phy_dev && sgmii_adapter_base) {
+	if (phy_dev && sgmii_adapter_base)
 		writew(SGMII_ADAPTER_ENABLE,
 		       sgmii_adapter_base + SGMII_ADAPTER_CTRL_REG);
-		tse_pcs_fix_mac_speed(&dwmac->pcs, phy_dev, speed);
-	}
 }
 
 static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac, struct device *dev)
@@ -183,11 +187,11 @@ static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac, struct device *
 				goto err_node_put;
 			}
 
-			dwmac->pcs.sgmii_adapter_base =
+			dwmac->sgmii_adapter_base =
 			    devm_ioremap_resource(dev, &res_sgmii_adapter);
 
-			if (IS_ERR(dwmac->pcs.sgmii_adapter_base)) {
-				ret = PTR_ERR(dwmac->pcs.sgmii_adapter_base);
+			if (IS_ERR(dwmac->sgmii_adapter_base)) {
+				ret = PTR_ERR(dwmac->sgmii_adapter_base);
 				goto err_node_put;
 			}
 		}
@@ -205,11 +209,11 @@ static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac, struct device *
 				goto err_node_put;
 			}
 
-			dwmac->pcs.tse_pcs_base =
+			dwmac->tse_pcs_base =
 			    devm_ioremap_resource(dev, &res_tse_pcs);
 
-			if (IS_ERR(dwmac->pcs.tse_pcs_base)) {
-				ret = PTR_ERR(dwmac->pcs.tse_pcs_base);
+			if (IS_ERR(dwmac->tse_pcs_base)) {
+				ret = PTR_ERR(dwmac->tse_pcs_base);
 				goto err_node_put;
 			}
 		}
@@ -235,6 +239,13 @@ static int socfpga_get_plat_phymode(struct socfpga_dwmac *dwmac)
 	return priv->plat->interface;
 }
 
+static void socfpga_sgmii_config(struct socfpga_dwmac *dwmac, bool enable)
+{
+	u16 val = enable ? SGMII_ADAPTER_ENABLE : SGMII_ADAPTER_DISABLE;
+
+	writew(val, dwmac->sgmii_adapter_base + SGMII_ADAPTER_CTRL_REG);
+}
+
 static int socfpga_set_phy_mode_common(int phymode, u32 *val)
 {
 	switch (phymode) {
@@ -310,12 +321,8 @@ static int socfpga_gen5_set_phy_mode(struct socfpga_dwmac *dwmac)
 	 */
 	reset_control_deassert(dwmac->stmmac_ocp_rst);
 	reset_control_deassert(dwmac->stmmac_rst);
-	if (phymode == PHY_INTERFACE_MODE_SGMII) {
-		if (tse_pcs_init(dwmac->pcs.tse_pcs_base, &dwmac->pcs) != 0) {
-			dev_err(dwmac->dev, "Unable to initialize TSE PCS");
-			return -EINVAL;
-		}
-	}
+	if (phymode == PHY_INTERFACE_MODE_SGMII)
+		socfpga_sgmii_config(dwmac, true);
 
 	return 0;
 }
@@ -367,12 +374,8 @@ static int socfpga_gen10_set_phy_mode(struct socfpga_dwmac *dwmac)
 	 */
 	reset_control_deassert(dwmac->stmmac_ocp_rst);
 	reset_control_deassert(dwmac->stmmac_rst);
-	if (phymode == PHY_INTERFACE_MODE_SGMII) {
-		if (tse_pcs_init(dwmac->pcs.tse_pcs_base, &dwmac->pcs) != 0) {
-			dev_err(dwmac->dev, "Unable to initialize TSE PCS");
-			return -EINVAL;
-		}
-	}
+	if (phymode == PHY_INTERFACE_MODE_SGMII)
+		socfpga_sgmii_config(dwmac, true);
 	return 0;
 }
 
@@ -386,6 +389,14 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
 	struct net_device	*ndev;
 	struct stmmac_priv	*stpriv;
 	const struct socfpga_dwmac_ops *ops;
+	struct regmap_config pcs_regmap_cfg;
+	struct regmap *pcs_regmap;
+	struct mii_bus *pcs_bus;
+
+	struct mdio_regmap_config mrc = {
+		.parent = &pdev->dev,
+		.valid_addr = 0x0,
+	};
 
 	ops = device_get_match_data(&pdev->dev);
 	if (!ops) {
@@ -443,6 +454,35 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_dvr_remove;
 
+	memset(&pcs_regmap_cfg, 0, sizeof(pcs_regmap_cfg));
+	pcs_regmap_cfg.reg_bits = 16;
+	pcs_regmap_cfg.val_bits = 16;
+	pcs_regmap_cfg.reg_shift = REGMAP_UPSHIFT(1);
+
+	/* Create a regmap for the PCS so that it can be used by the PCS driver,
+	 * if we have such a PCS
+	 */
+	if (dwmac->tse_pcs_base) {
+		pcs_regmap = devm_regmap_init_mmio(&pdev->dev, dwmac->tse_pcs_base,
+						   &pcs_regmap_cfg);
+		if (IS_ERR(pcs_regmap)) {
+			ret = PTR_ERR(pcs_regmap);
+			goto err_dvr_remove;
+		}
+
+		mrc.regmap = pcs_regmap;
+
+		snprintf(mrc.name, MII_BUS_ID_SIZE, "%s-pcs-mii", ndev->name);
+		pcs_bus = devm_mdio_regmap_register(&pdev->dev, &mrc);
+		if (IS_ERR(pcs_bus)) {
+			ret = PTR_ERR(pcs_bus);
+			goto err_dvr_remove;
+		}
+
+		dwmac->pcs_mdiodev = mdio_device_create(pcs_bus, 0);
+		stpriv->hw->phylink_pcs = lynx_pcs_create(dwmac->pcs_mdiodev);
+	}
+
 	return 0;
 
 err_dvr_remove:
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 0fca81507a77..e570a95dd8d0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -937,10 +937,13 @@ static struct phylink_pcs *stmmac_mac_select_pcs(struct phylink_config *config,
 {
 	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
 
-	if (!priv->hw->xpcs)
-		return NULL;
+	if (priv->hw->xpcs)
+		return &priv->hw->xpcs->pcs;
+
+	if (priv->hw->phylink_pcs)
+		return priv->hw->phylink_pcs;
 
-	return &priv->hw->xpcs->pcs;
+	return NULL;
 }
 
 static void stmmac_mac_config(struct phylink_config *config, unsigned int mode,
@@ -3813,7 +3816,8 @@ static int __stmmac_open(struct net_device *dev,
 	if (priv->hw->pcs != STMMAC_PCS_TBI &&
 	    priv->hw->pcs != STMMAC_PCS_RTBI &&
 	    (!priv->hw->xpcs ||
-	     xpcs_get_an_mode(priv->hw->xpcs, mode) != DW_AN_C73)) {
+	     xpcs_get_an_mode(priv->hw->xpcs, mode) != DW_AN_C73) &&
+	    !priv->hw->phylink_pcs) {
 		ret = stmmac_init_phy(dev);
 		if (ret) {
 			netdev_err(priv->dev,
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next v2 1/4] net: mdio: Introduce a regmap-based mdio driver
  2023-05-25 10:11 ` [PATCH net-next v2 1/4] net: mdio: Introduce a regmap-based mdio driver Maxime Chevallier
@ 2023-05-25 11:02   ` Simon Horman
  2023-05-25 12:43     ` Maxime Chevallier
  2023-05-25 11:11   ` Russell King (Oracle)
  1 sibling, 1 reply; 10+ messages in thread
From: Simon Horman @ 2023-05-25 11:02 UTC (permalink / raw)
  To: Maxime Chevallier
  Cc: Mark Brown, davem, netdev, linux-kernel, alexis.lothore,
	thomas.petazzoni, Andrew Lunn, Jakub Kicinski, Eric Dumazet,
	Paolo Abeni, Florian Fainelli, Heiner Kallweit, Russell King,
	Vladimir Oltean, Ioana Ciornei, linux-stm32, linux-arm-kernel,
	Maxime Coquelin, Jose Abreu, Alexandre Torgue,
	Giuseppe Cavallaro

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");

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next v2 1/4] net: mdio: Introduce a regmap-based mdio driver
  2023-05-25 10:11 ` [PATCH net-next v2 1/4] net: mdio: Introduce a regmap-based mdio driver Maxime Chevallier
  2023-05-25 11:02   ` Simon Horman
@ 2023-05-25 11:11   ` Russell King (Oracle)
  2023-05-25 12:41     ` Maxime Chevallier
  1 sibling, 1 reply; 10+ messages in thread
From: Russell King (Oracle) @ 2023-05-25 11:11 UTC (permalink / raw)
  To: Maxime Chevallier
  Cc: Mark Brown, davem, netdev, linux-kernel, alexis.lothore,
	thomas.petazzoni, Andrew Lunn, Jakub Kicinski, Eric Dumazet,
	Paolo Abeni, Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Ioana Ciornei, linux-stm32, linux-arm-kernel, Maxime Coquelin,
	Jose Abreu, Alexandre Torgue, Giuseppe Cavallaro

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?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next v2 1/4] net: mdio: Introduce a regmap-based mdio driver
  2023-05-25 11:11   ` Russell King (Oracle)
@ 2023-05-25 12:41     ` Maxime Chevallier
  0 siblings, 0 replies; 10+ messages in thread
From: Maxime Chevallier @ 2023-05-25 12:41 UTC (permalink / raw)
  To: Russell King (Oracle)
  Cc: Mark Brown, davem, netdev, linux-kernel, alexis.lothore,
	thomas.petazzoni, Andrew Lunn, Jakub Kicinski, Eric Dumazet,
	Paolo Abeni, Florian Fainelli, Heiner Kallweit, Vladimir Oltean,
	Ioana Ciornei, linux-stm32, linux-arm-kernel, Maxime Coquelin,
	Jose Abreu, Alexandre Torgue, Giuseppe Cavallaro

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next v2 1/4] net: mdio: Introduce a regmap-based mdio driver
  2023-05-25 11:02   ` Simon Horman
@ 2023-05-25 12:43     ` Maxime Chevallier
  2023-05-25 15:00       ` Simon Horman
  0 siblings, 1 reply; 10+ messages in thread
From: Maxime Chevallier @ 2023-05-25 12:43 UTC (permalink / raw)
  To: Simon Horman
  Cc: Mark Brown, davem, netdev, linux-kernel, alexis.lothore,
	thomas.petazzoni, Andrew Lunn, Jakub Kicinski, Eric Dumazet,
	Paolo Abeni, Florian Fainelli, Heiner Kallweit, Russell King,
	Vladimir Oltean, Ioana Ciornei, linux-stm32, linux-arm-kernel,
	Maxime Coquelin, Jose Abreu, Alexandre Torgue,
	Giuseppe Cavallaro

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");  


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next v2 1/4] net: mdio: Introduce a regmap-based mdio driver
  2023-05-25 12:43     ` Maxime Chevallier
@ 2023-05-25 15:00       ` Simon Horman
  0 siblings, 0 replies; 10+ messages in thread
From: Simon Horman @ 2023-05-25 15:00 UTC (permalink / raw)
  To: Maxime Chevallier
  Cc: Mark Brown, davem, netdev, linux-kernel, alexis.lothore,
	thomas.petazzoni, Andrew Lunn, Jakub Kicinski, Eric Dumazet,
	Paolo Abeni, Florian Fainelli, Heiner Kallweit, Russell King,
	Vladimir Oltean, Ioana Ciornei, linux-stm32, linux-arm-kernel,
	Maxime Coquelin, Jose Abreu, Alexandre Torgue,
	Giuseppe Cavallaro

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!

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-05-25 15:00 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-25 10:11 [PATCH net-next v2 0/4] net: add a regmap-based mdio driver and drop TSE PCS Maxime Chevallier
2023-05-25 10:11 ` [PATCH net-next v2 1/4] net: mdio: Introduce a regmap-based mdio driver Maxime Chevallier
2023-05-25 11:02   ` Simon Horman
2023-05-25 12:43     ` Maxime Chevallier
2023-05-25 15:00       ` Simon Horman
2023-05-25 11:11   ` Russell King (Oracle)
2023-05-25 12:41     ` Maxime Chevallier
2023-05-25 10:11 ` [PATCH net-next v2 2/4] net: ethernet: altera-tse: Convert to mdio-regmap and use PCS Lynx Maxime Chevallier
2023-05-25 10:11 ` [PATCH net-next v2 3/4] net: pcs: Drop the TSE PCS driver Maxime Chevallier
2023-05-25 10:11 ` [PATCH net-next v2 4/4] net: stmmac: dwmac-sogfpga: use the lynx pcs driver Maxime Chevallier

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).