[RFC 0/7] Introduce a generic regmap-based MDIO driver

[RFC 0/7] Introduce a generic regmap-based MDIO driver

[Maxime Chevallier]

Hello everyone,

When the Altera TSE PCS driver was initially introduced, there were
comments by Russell that the register layout looked very familiar to the
existing Lynx PCS driver, the only difference being that the TSE PCS
driver is memory-mapped whereas the Lynx PCS driver sits on an MDIO bus.

Since then, I've sent a followup to create a wrapper around Lynx, that
would create a virtual MDIO bus driver that would translate the mdio
operations to mmio operations [1].

Discussion ensued, and we agreed that we could make this more generic,
the idea being that this PCS isn't the only example of such a device,
that memory-maps an mdio-like register layout

Vladimir mentionned that some ocelot devices have the same kind of
problematic, but this time the devices sits on a SPI bus, exposing MDIO
registers.

This series therefore introduces a new "virtual" mdio driver, that would
translate MDIO accesses to an underlying regmap. That way, we can use
the whole phylink and phylib logic to configure the device (as phylink
interacts with mdiodevice's).

One issue encountered is that the MDIO registers have a stride of 1,
and we need to translate that to a higher stride for memory-mapped
devices. This is what the first 3 patches of this series do.

Then, patch 4 addresses a small inconsistency found in the only user of
regmap's reg_downshift.

Patch 5 introduces the new MDIO driver, while patch 6 makes use of it
by porting altera_tse to the Lynx PCS.

Finally patch 7 drops the TSE PCS driver, as it is no longer needed.

This series is a RFC as is still has a few shortcomings, but due to
various subsystems being involved, I'm sending it as a whole so that
reviewers can get a clear view of the big picture, the end-goal and the
various problems faces.

The existing shortcomings are :
 - The virtual MDIO bus driver can only have 1 mdio-device on it. If we
   have multiple ones that are memory-mapped onto the same range (with
   an offset, for example), we can't address them and we would have to
   create one such virtual bus driver per device. I don't know as of
   today if the problem will show-up for some users

 - With this series, we can also convert dwmac_socfpga to Lynx, but I've
   left this out for now

 - The renaming of regmap.format.reg_downshift to regmap.format.reg_shift
   can be confusing, as regmap.reg_shift also exists and has another
   meaning. I'm very bad at naming, so any suggestion is welcome.

Thanks,

Maxime

[1] : https://lore.kernel.org/all/20230210190949.1115836-1-maxime.chevallier@bootlin.com/

Maxime Chevallier (7):
  regmap: add a helper to translate the register address
  regmap: check for alignment on translated register addresses
  regmap: allow upshifting register addresses before performing
    operations
  mfd: ocelot-spi: Change the regmap stride to reflect the real one
  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

 MAINTAINERS                                   |  14 +-
 drivers/base/regmap/internal.h                |   2 +-
 drivers/base/regmap/regmap.c                  |  55 +++---
 drivers/mfd/ocelot-spi.c                      |   4 +-
 drivers/net/ethernet/altera/altera_tse.h      |   1 +
 drivers/net/ethernet/altera/altera_tse_main.c |  54 +++++-
 drivers/net/mdio/Kconfig                      |  11 ++
 drivers/net/mdio/Makefile                     |   1 +
 drivers/net/mdio/mdio-regmap.c                |  85 ++++++++++
 drivers/net/pcs/Kconfig                       |   6 -
 drivers/net/pcs/Makefile                      |   1 -
 drivers/net/pcs/pcs-altera-tse.c              | 160 ------------------
 include/linux/mdio/mdio-regmap.h              |  25 +++
 include/linux/pcs-altera-tse.h                |  17 --
 include/linux/regmap.h                        |  15 +-
 15 files changed, 223 insertions(+), 228 deletions(-)
 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.39.2

[RFC 1/7] regmap: add a helper to translate the register address

[Maxime Chevallier]

Register addresses passed to regmap operations can be offset with
regmap.reg_base and downshifted with regmap.reg_downshift.

Add a helper to apply both these operations and return the translated
address, that we can then use to perform the actual register operation
ont the underlying bus.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/base/regmap/regmap.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index d2a54eb0efd9..a4e4367648bf 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1676,6 +1676,12 @@ static void regmap_set_work_buf_flag_mask(struct regmap *map, int max_bytes,
 		buf[i] |= (mask >> (8 * i)) & 0xff;
 }
 
+static unsigned int regmap_reg_addr(struct regmap *map, unsigned int reg)
+{
+	reg += map->reg_base;
+	return reg >> map->format.reg_downshift;
+}
+
 static int _regmap_raw_write_impl(struct regmap *map, unsigned int reg,
 				  const void *val, size_t val_len, bool noinc)
 {
@@ -1753,8 +1759,7 @@ static int _regmap_raw_write_impl(struct regmap *map, unsigned int reg,
 			return ret;
 	}
 
-	reg += map->reg_base;
-	reg >>= map->format.reg_downshift;
+	reg = regmap_reg_addr(map, reg);
 	map->format.format_reg(map->work_buf, reg, map->reg_shift);
 	regmap_set_work_buf_flag_mask(map, map->format.reg_bytes,
 				      map->write_flag_mask);
@@ -1924,8 +1929,7 @@ static int _regmap_bus_formatted_write(void *context, unsigned int reg,
 			return ret;
 	}
 
-	reg += map->reg_base;
-	reg >>= map->format.reg_downshift;
+	reg = regmap_reg_addr(map, reg);
 	map->format.format_write(map, reg, val);
 
 	trace_regmap_hw_write_start(map, reg, 1);
@@ -1942,8 +1946,7 @@ static int _regmap_bus_reg_write(void *context, unsigned int reg,
 {
 	struct regmap *map = context;
 
-	reg += map->reg_base;
-	reg >>= map->format.reg_downshift;
+	reg = regmap_reg_addr(map, reg);
 	return map->bus->reg_write(map->bus_context, reg, val);
 }
 
@@ -2494,8 +2497,7 @@ static int _regmap_raw_multi_reg_write(struct regmap *map,
 		unsigned int reg = regs[i].reg;
 		unsigned int val = regs[i].def;
 		trace_regmap_hw_write_start(map, reg, 1);
-		reg += map->reg_base;
-		reg >>= map->format.reg_downshift;
+		reg = regmap_reg_addr(map, reg);
 		map->format.format_reg(u8, reg, map->reg_shift);
 		u8 += reg_bytes + pad_bytes;
 		map->format.format_val(u8, val, 0);
@@ -2821,8 +2823,7 @@ static int _regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
 			return ret;
 	}
 
-	reg += map->reg_base;
-	reg >>= map->format.reg_downshift;
+	reg = regmap_reg_addr(map, reg);
 	map->format.format_reg(map->work_buf, reg, map->reg_shift);
 	regmap_set_work_buf_flag_mask(map, map->format.reg_bytes,
 				      map->read_flag_mask);
@@ -2842,8 +2843,7 @@ static int _regmap_bus_reg_read(void *context, unsigned int reg,
 {
 	struct regmap *map = context;
 
-	reg += map->reg_base;
-	reg >>= map->format.reg_downshift;
+	reg = regmap_reg_addr(map, reg);
 	return map->bus->reg_read(map->bus_context, reg, val);
 }
 
@@ -3235,8 +3235,7 @@ static int _regmap_update_bits(struct regmap *map, unsigned int reg,
 		*change = false;
 
 	if (regmap_volatile(map, reg) && map->reg_update_bits) {
-		reg += map->reg_base;
-		reg >>= map->format.reg_downshift;
+		reg = regmap_reg_addr(map, reg);
 		ret = map->reg_update_bits(map->bus_context, reg, mask, val);
 		if (ret == 0 && change)
 			*change = true;
-- 
2.39.2

[RFC 2/7] regmap: check for alignment on translated register addresses

[Maxime Chevallier]

With regmap->reg_base and regmap->reg_downshift, the actual register
address that is going to be used for the next operation might not be the
same as the one we have as an input. Addresses can be offset with
reg_base and shifted, which will affect alignment.

Check for alignment on the real register address.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/base/regmap/regmap.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index a4e4367648bf..726f59612fd6 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -2016,7 +2016,7 @@ int regmap_write(struct regmap *map, unsigned int reg, unsigned int val)
 {
 	int ret;
 
-	if (!IS_ALIGNED(reg, map->reg_stride))
+	if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride))
 		return -EINVAL;
 
 	map->lock(map->lock_arg);
@@ -2043,7 +2043,7 @@ int regmap_write_async(struct regmap *map, unsigned int reg, unsigned int val)
 {
 	int ret;
 
-	if (!IS_ALIGNED(reg, map->reg_stride))
+	if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride))
 		return -EINVAL;
 
 	map->lock(map->lock_arg);
@@ -2258,7 +2258,7 @@ int regmap_noinc_write(struct regmap *map, unsigned int reg,
 		return -EINVAL;
 	if (val_len % map->format.val_bytes)
 		return -EINVAL;
-	if (!IS_ALIGNED(reg, map->reg_stride))
+	if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride))
 		return -EINVAL;
 	if (val_len == 0)
 		return -EINVAL;
@@ -2399,7 +2399,7 @@ int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val,
 	int ret = 0, i;
 	size_t val_bytes = map->format.val_bytes;
 
-	if (!IS_ALIGNED(reg, map->reg_stride))
+	if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride))
 		return -EINVAL;
 
 	/*
@@ -2638,7 +2638,7 @@ static int _regmap_multi_reg_write(struct regmap *map,
 			int reg = regs[i].reg;
 			if (!map->writeable_reg(map->dev, reg))
 				return -EINVAL;
-			if (!IS_ALIGNED(reg, map->reg_stride))
+			if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride))
 				return -EINVAL;
 		}
 
@@ -2789,7 +2789,7 @@ int regmap_raw_write_async(struct regmap *map, unsigned int reg,
 
 	if (val_len % map->format.val_bytes)
 		return -EINVAL;
-	if (!IS_ALIGNED(reg, map->reg_stride))
+	if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride))
 		return -EINVAL;
 
 	map->lock(map->lock_arg);
@@ -2911,7 +2911,7 @@ int regmap_read(struct regmap *map, unsigned int reg, unsigned int *val)
 {
 	int ret;
 
-	if (!IS_ALIGNED(reg, map->reg_stride))
+	if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride))
 		return -EINVAL;
 
 	map->lock(map->lock_arg);
@@ -2945,7 +2945,7 @@ int regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
 
 	if (val_len % map->format.val_bytes)
 		return -EINVAL;
-	if (!IS_ALIGNED(reg, map->reg_stride))
+	if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride))
 		return -EINVAL;
 	if (val_count == 0)
 		return -EINVAL;
@@ -3040,7 +3040,7 @@ int regmap_noinc_read(struct regmap *map, unsigned int reg,
 
 	if (val_len % map->format.val_bytes)
 		return -EINVAL;
-	if (!IS_ALIGNED(reg, map->reg_stride))
+	if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride))
 		return -EINVAL;
 	if (val_len == 0)
 		return -EINVAL;
@@ -3162,7 +3162,7 @@ int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val,
 	size_t val_bytes = map->format.val_bytes;
 	bool vol = regmap_volatile_range(map, reg, val_count);
 
-	if (!IS_ALIGNED(reg, map->reg_stride))
+	if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride))
 		return -EINVAL;
 	if (val_count == 0)
 		return -EINVAL;
-- 
2.39.2

[RFC 3/7] regmap: allow upshifting register addresses before performing operations

[Maxime Chevallier]

Similar to the existing reg_downshift mechanism, that is used to
translate register addresses on busses that have a smaller address
stride, it's also possible to want to upshift register addresses.

Such a case was encountered when network PHYs and PCS that usually sit
on a MDIO bus (16-bits register with a stride of 1) are integrated
directly as memory-mapped devices. Here, the same register layout
defined in 802.3 is used, but the register now have a larger stride.

Introduce a mechanism to also allow upshifting register addresses.
Re-purpose reg_downshift into a more generic, signed reg_shift, whose
sign indicates the direction of the shift. To avoid confusion, also
introduce macros to explicitly indicate if we want to downshift or
upshift.

For bisectability, change any use of reg_downshift to use reg_shift.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/base/regmap/internal.h |  2 +-
 drivers/base/regmap/regmap.c   | 10 ++++++++--
 drivers/mfd/ocelot-spi.c       |  2 +-
 include/linux/regmap.h         | 15 ++++++++++++---
 4 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index da8996e7a1f1..2ef53936652b 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -31,7 +31,7 @@ struct regmap_format {
 	size_t buf_size;
 	size_t reg_bytes;
 	size_t pad_bytes;
-	size_t reg_downshift;
+	int reg_shift;
 	size_t val_bytes;
 	void (*format_write)(struct regmap *map,
 			     unsigned int reg, unsigned int val);
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 726f59612fd6..c4cde4f45b05 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -814,7 +814,7 @@ struct regmap *__regmap_init(struct device *dev,
 
 	map->format.reg_bytes = DIV_ROUND_UP(config->reg_bits, 8);
 	map->format.pad_bytes = config->pad_bits / 8;
-	map->format.reg_downshift = config->reg_downshift;
+	map->format.reg_shift = config->reg_shift;
 	map->format.val_bytes = DIV_ROUND_UP(config->val_bits, 8);
 	map->format.buf_size = DIV_ROUND_UP(config->reg_bits +
 			config->val_bits + config->pad_bits, 8);
@@ -1679,7 +1679,13 @@ static void regmap_set_work_buf_flag_mask(struct regmap *map, int max_bytes,
 static unsigned int regmap_reg_addr(struct regmap *map, unsigned int reg)
 {
 	reg += map->reg_base;
-	return reg >> map->format.reg_downshift;
+
+	if (map->format.reg_shift > 0)
+		reg >>= map->format.reg_shift;
+	else if (map->format.reg_shift < 0)
+		reg <<= -(map->format.reg_shift);
+
+	return reg;
 }
 
 static int _regmap_raw_write_impl(struct regmap *map, unsigned int reg,
diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
index 2ecd271de2fb..2d1349a10ca9 100644
--- a/drivers/mfd/ocelot-spi.c
+++ b/drivers/mfd/ocelot-spi.c
@@ -125,7 +125,7 @@ static int ocelot_spi_initialize(struct device *dev)
 static const struct regmap_config ocelot_spi_regmap_config = {
 	.reg_bits = 24,
 	.reg_stride = 4,
-	.reg_downshift = 2,
+	.reg_shift = REGMAP_DOWNSHIFT(2),
 	.val_bits = 32,
 
 	.write_flag_mask = 0x80,
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 4d10790adeb0..f02c3857b023 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -46,6 +46,14 @@ struct sdw_slave;
 #define REGMAP_MDIO_C45_DEVAD_MASK	GENMASK(20, 16)
 #define REGMAP_MDIO_C45_REGNUM_MASK	GENMASK(15, 0)
 
+/*
+ * regmap.reg_shift indicates by how much we must shift registers prior to
+ * performing any operation. It's a signed value, positive numbers means
+ * downshifting the register's address, while negative numbers means upshifting.
+ */
+#define REGMAP_UPSHIFT(s)	(-(s))
+#define REGMAP_DOWNSHIFT(s)	(s)
+
 /* An enum of all the supported cache types */
 enum regcache_type {
 	REGCACHE_NONE,
@@ -246,8 +254,9 @@ typedef void (*regmap_unlock)(void *);
  * @reg_stride: The register address stride. Valid register addresses are a
  *              multiple of this value. If set to 0, a value of 1 will be
  *              used.
- * @reg_downshift: The number of bits to downshift the register before
- *		   performing any operations.
+ * @reg_shift: The number of bits to shift the register before performing any
+ *	       operations. Any positive number will be downshifted, and negative
+ *	       values will be upshifted
  * @reg_base: Value to be added to every register address before performing any
  *	      operation.
  * @pad_bits: Number of bits of padding between register and value.
@@ -381,7 +390,7 @@ struct regmap_config {
 
 	int reg_bits;
 	int reg_stride;
-	int reg_downshift;
+	int reg_shift;
 	unsigned int reg_base;
 	int pad_bits;
 	int val_bits;
-- 
2.39.2

[RFC 4/7] mfd: ocelot-spi: Change the regmap stride to reflect the real one

[Maxime Chevallier]

When used over SPI, the register addresses needs to be translated,
compared to when used over MMIO. The translation consists in applying an
offset with reg_base, then downshifting the registers by 2. This
actually changes the register stride from 4 to 1.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/mfd/ocelot-spi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
index 2d1349a10ca9..107cda0544aa 100644
--- a/drivers/mfd/ocelot-spi.c
+++ b/drivers/mfd/ocelot-spi.c
@@ -124,7 +124,7 @@ static int ocelot_spi_initialize(struct device *dev)
 
 static const struct regmap_config ocelot_spi_regmap_config = {
 	.reg_bits = 24,
-	.reg_stride = 4,
+	.reg_stride = 1,
 	.reg_shift = REGMAP_DOWNSHIFT(2),
 	.val_bits = 32,
 
-- 
2.39.2

[RFC 5/7] net: mdio: Introduce a regmap-based mdio driver

[Maxime Chevallier]

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>
---
 MAINTAINERS                      |  7 +++
 drivers/net/mdio/Kconfig         | 11 +++++
 drivers/net/mdio/Makefile        |  1 +
 drivers/net/mdio/mdio-regmap.c   | 85 ++++++++++++++++++++++++++++++++
 include/linux/mdio/mdio-regmap.h | 25 ++++++++++
 5 files changed, 129 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 8d5bc223f305..10b3a1800e0d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12751,6 +12751,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/mdio/Kconfig b/drivers/net/mdio/Kconfig
index 90309980686e..671e4bb82e3e 100644
--- a/drivers/net/mdio/Kconfig
+++ b/drivers/net/mdio/Kconfig
@@ -182,6 +182,17 @@ config MDIO_IPQ8064
 	  This driver supports the MDIO interface found in the network
 	  interface units of the IPQ8064 SoC
 
+config MDIO_REGMAP
+	tristate "Regmap-based virtual MDIO bus driver"
+	depends on REGMAP
+	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..c85d62c2f55c
--- /dev/null
+++ b/drivers/net/mdio/mdio-regmap.c
@@ -0,0 +1,85 @@
+// 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 & BIT(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 & BIT(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);
+
+	if (!config->valid_addr)
+		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->parent = config->parent;
+	mrc->valid_addr = config->valid_addr;
+
+	mii->name = DRV_NAME;
+	strncpy(mii->id, config->name, MII_BUS_ID_SIZE);
+	mii->parent = config->parent;
+	mii->read = mdio_regmap_read_c22;
+	mii->write = mdio_regmap_write_c22;
+
+	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;
+}
+
diff --git a/include/linux/mdio/mdio-regmap.h b/include/linux/mdio/mdio-regmap.h
new file mode 100644
index 000000000000..ea428e5a2913
--- /dev/null
+++ b/include/linux/mdio/mdio-regmap.h
@@ -0,0 +1,25 @@
+/* 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
+
+#define MDIO_REGMAP_NAME 63
+
+struct device;
+struct regmap;
+
+struct mdio_regmap_config {
+	struct device *parent;
+	struct regmap *regmap;
+	char name[MDIO_REGMAP_NAME];
+	u32 valid_addr;
+};
+
+struct mii_bus *devm_mdio_regmap_register(struct device *dev,
+					  const struct mdio_regmap_config *config);
+
+#endif
-- 
2.39.2

[RFC 6/7] net: ethernet: altera-tse: Convert to mdio-regmap and use PCS Lynx

[Maxime Chevallier]

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>
---
 drivers/net/ethernet/altera/altera_tse.h      |  1 +
 drivers/net/ethernet/altera/altera_tse_main.c | 54 ++++++++++++++++---
 2 files changed, 49 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 66e3af73ec41..c5f4b5e24376 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>
 
@@ -1139,13 +1141,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 = 0x1,
+	};
+
 	ndev = alloc_etherdev(sizeof(struct altera_tse_private));
 	if (!ndev) {
 		dev_err(&pdev->dev, "Could not allocate network device\n");
@@ -1263,9 +1273,30 @@ 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_stride = 4;
+		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_stride = 2;
+		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");
@@ -1389,7 +1420,15 @@ 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);
+	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;
@@ -1412,11 +1451,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:
@@ -1438,6 +1478,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;
-- 
2.39.2

[RFC 7/7] net: pcs: Drop the TSE PCS driver

[Maxime Chevallier]

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>
---
 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 10b3a1800e0d..e2e648a46e3f 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 6e7e6c346a3e..6289b7c765f1 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -26,10 +26,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 4c780d8f2e98..0ff5388fcdea 100644
--- a/drivers/net/pcs/Makefile
+++ b/drivers/net/pcs/Makefile
@@ -6,4 +6,3 @@ pcs_xpcs-$(CONFIG_PCS_XPCS)	:= pcs-xpcs.o pcs-xpcs-nxp.o
 obj-$(CONFIG_PCS_XPCS)		+= pcs_xpcs.o
 obj-$(CONFIG_PCS_LYNX)		+= pcs-lynx.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.39.2

Re: [RFC 3/7] regmap: allow upshifting register addresses before performing operations

[Andrew Lunn]

> index da8996e7a1f1..2ef53936652b 100644
> --- a/drivers/base/regmap/internal.h
> +++ b/drivers/base/regmap/internal.h
> @@ -31,7 +31,7 @@ struct regmap_format {
>  	size_t buf_size;
>  	size_t reg_bytes;
>  	size_t pad_bytes;
> -	size_t reg_downshift;
> +	int reg_shift;

Maybe ssize_t to keep with the pattern of using size_t. However,
ssize_t is somewhat over sized for a bit shift. So maybe just s8?

>  	size_t val_bytes;
>  	void (*format_write)(struct regmap *map,
>  			     unsigned int reg, unsigned int val);
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index 726f59612fd6..c4cde4f45b05 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -814,7 +814,7 @@ struct regmap *__regmap_init(struct device *dev,
>  
>  	map->format.reg_bytes = DIV_ROUND_UP(config->reg_bits, 8);
>  	map->format.pad_bytes = config->pad_bits / 8;
> -	map->format.reg_downshift = config->reg_downshift;
> +	map->format.reg_shift = config->reg_shift;
>  	map->format.val_bytes = DIV_ROUND_UP(config->val_bits, 8);
>  	map->format.buf_size = DIV_ROUND_UP(config->reg_bits +
>  			config->val_bits + config->pad_bits, 8);
> @@ -1679,7 +1679,13 @@ static void regmap_set_work_buf_flag_mask(struct regmap *map, int max_bytes,
>  static unsigned int regmap_reg_addr(struct regmap *map, unsigned int reg)
>  {
>  	reg += map->reg_base;
> -	return reg >> map->format.reg_downshift;
> +
> +	if (map->format.reg_shift > 0)
> +		reg >>= map->format.reg_shift;
> +	else if (map->format.reg_shift < 0)
> +		reg <<= -(map->format.reg_shift);

I was wondering about negative shifts. It is apparently undefined
behaviour. So this construct is required.

	   Andrew

Re: [RFC 4/7] mfd: ocelot-spi: Change the regmap stride to reflect the real one

[Andrew Lunn]

On Fri, Mar 24, 2023 at 10:36:41AM +0100, Maxime Chevallier wrote:
> When used over SPI, the register addresses needs to be translated,
> compared to when used over MMIO. The translation consists in applying an
> offset with reg_base, then downshifting the registers by 2. This
> actually changes the register stride from 4 to 1.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
>  drivers/mfd/ocelot-spi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
> index 2d1349a10ca9..107cda0544aa 100644
> --- a/drivers/mfd/ocelot-spi.c
> +++ b/drivers/mfd/ocelot-spi.c
> @@ -124,7 +124,7 @@ static int ocelot_spi_initialize(struct device *dev)
>  
>  static const struct regmap_config ocelot_spi_regmap_config = {
>  	.reg_bits = 24,
> -	.reg_stride = 4,
> +	.reg_stride = 1,
>  	.reg_shift = REGMAP_DOWNSHIFT(2),
>  	.val_bits = 32,

This does not look like a bisectable change? Or did it never work
before?

	Andrew

Re: [RFC 4/7] mfd: ocelot-spi: Change the regmap stride to reflect the real one

[Maxime Chevallier]

Hello Andrew,

On Fri, 24 Mar 2023 13:11:07 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> On Fri, Mar 24, 2023 at 10:36:41AM +0100, Maxime Chevallier wrote:
> > When used over SPI, the register addresses needs to be translated,
> > compared to when used over MMIO. The translation consists in
> > applying an offset with reg_base, then downshifting the registers
> > by 2. This actually changes the register stride from 4 to 1.
> > 
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > ---
> >  drivers/mfd/ocelot-spi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
> > index 2d1349a10ca9..107cda0544aa 100644
> > --- a/drivers/mfd/ocelot-spi.c
> > +++ b/drivers/mfd/ocelot-spi.c
> > @@ -124,7 +124,7 @@ static int ocelot_spi_initialize(struct device
> > *dev) 
> >  static const struct regmap_config ocelot_spi_regmap_config = {
> >  	.reg_bits = 24,
> > -	.reg_stride = 4,
> > +	.reg_stride = 1,
> >  	.reg_shift = REGMAP_DOWNSHIFT(2),
> >  	.val_bits = 32,  
> 
> This does not look like a bisectable change? Or did it never work
> before?

Actually this works in all cases because of "regmap: check for alignment
on translated register addresses" in this series. Before this series,
I think using a stride of 1 would have worked too, as any 4-byte-aligned
accesses are also 1-byte aligned.

But that's also why I need review on this, my understanding is that
reg_stride is used just as a check for alignment, and I couldn't test
this ocelot-related patch on the real HW, so please take it with a
grain of salt :(

Thanks,

Maxime

> 	Andrew

Re: [RFC 5/7] net: mdio: Introduce a regmap-based mdio driver

[kernel test robot]

Hi Maxime,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on broonie-regmap/for-next]
[also build test WARNING on lee-mfd/for-mfd-next driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus linus/master v6.3-rc3 next-20230324]
[cannot apply to lee-mfd/for-mfd-fixes]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Maxime-Chevallier/regmap-add-a-helper-to-translate-the-register-address/20230324-173909
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next
patch link:    https://lore.kernel.org/r/20230324093644.464704-6-maxime.chevallier%40bootlin.com
patch subject: [RFC 5/7] net: mdio: Introduce a regmap-based mdio driver
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230324/202303242009.BW09N8SV-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 12.1.0
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/cb259fbfba2e078cfe013cb986496b6af0e55d21
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Maxime-Chevallier/regmap-add-a-helper-to-translate-the-register-address/20230324-173909
        git checkout cb259fbfba2e078cfe013cb986496b6af0e55d21
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash drivers/net/mdio/

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/202303242009.BW09N8SV-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/net/mdio/mdio-regmap.c: In function 'devm_mdio_regmap_register':
>> drivers/net/mdio/mdio-regmap.c:72:9: warning: 'strncpy' output may be truncated copying 61 bytes from a string of length 62 [-Wstringop-truncation]
      72 |         strncpy(mii->id, config->name, MII_BUS_ID_SIZE);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/strncpy +72 drivers/net/mdio/mdio-regmap.c

    46	
    47	struct mii_bus *devm_mdio_regmap_register(struct device *dev,
    48						  const struct mdio_regmap_config *config)
    49	{
    50		struct mdio_regmap_config *mrc;
    51		struct mii_bus *mii;
    52		int rc;
    53	
    54		if (!config->parent)
    55			return ERR_PTR(-EINVAL);
    56	
    57		if (!config->valid_addr)
    58			return ERR_PTR(-EINVAL);
    59	
    60		mii = devm_mdiobus_alloc_size(config->parent, sizeof(*mrc));
    61		if (!mii)
    62			return ERR_PTR(-ENOMEM);
    63	
    64		mrc = mii->priv;
    65		memcpy(mrc, config, sizeof(*mrc));
    66	
    67		mrc->regmap = config->regmap;
    68		mrc->parent = config->parent;
    69		mrc->valid_addr = config->valid_addr;
    70	
    71		mii->name = DRV_NAME;
  > 72		strncpy(mii->id, config->name, MII_BUS_ID_SIZE);
    73		mii->parent = config->parent;
    74		mii->read = mdio_regmap_read_c22;
    75		mii->write = mdio_regmap_write_c22;
    76	
    77		rc = devm_mdiobus_register(dev, mii);
    78		if (rc) {
    79			dev_err(config->parent, "Cannot register MDIO bus![%s] (%d)\n", mii->id, rc);
    80			return ERR_PTR(rc);
    81		}
    82	
    83		return mii;
    84	}
    85	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [RFC 4/7] mfd: ocelot-spi: Change the regmap stride to reflect the real one

[Colin Foster]

Hi Maxime,

On Fri, Mar 24, 2023 at 01:48:17PM +0100, Maxime Chevallier wrote:
> Hello Andrew,
> 
> On Fri, 24 Mar 2023 13:11:07 +0100
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > On Fri, Mar 24, 2023 at 10:36:41AM +0100, Maxime Chevallier wrote:
> > > When used over SPI, the register addresses needs to be translated,
> > > compared to when used over MMIO. The translation consists in
> > > applying an offset with reg_base, then downshifting the registers
> > > by 2. This actually changes the register stride from 4 to 1.
> > > 
> > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > ---
> > >  drivers/mfd/ocelot-spi.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
> > > index 2d1349a10ca9..107cda0544aa 100644
> > > --- a/drivers/mfd/ocelot-spi.c
> > > +++ b/drivers/mfd/ocelot-spi.c
> > > @@ -124,7 +124,7 @@ static int ocelot_spi_initialize(struct device
> > > *dev) 
> > >  static const struct regmap_config ocelot_spi_regmap_config = {
> > >  	.reg_bits = 24,
> > > -	.reg_stride = 4,
> > > +	.reg_stride = 1,
> > >  	.reg_shift = REGMAP_DOWNSHIFT(2),
> > >  	.val_bits = 32,  
> > 
> > This does not look like a bisectable change? Or did it never work
> > before?
> 
> Actually this works in all cases because of "regmap: check for alignment
> on translated register addresses" in this series. Before this series,
> I think using a stride of 1 would have worked too, as any 4-byte-aligned
> accesses are also 1-byte aligned.
> 
> But that's also why I need review on this, my understanding is that
> reg_stride is used just as a check for alignment, and I couldn't test
> this ocelot-related patch on the real HW, so please take it with a
> grain of salt :(

You're exactly right. reg_stride wasn't used anywhere in the
ocelot-spi path before this patch series. When I build against patch 3
("regmap: allow upshifting register addresses before performing
operations") ocelot-spi breaks.

[    3.207711] ocelot-soc spi0.0: error -EINVAL: Error initializing SPI bus

When I build against the whole series, or even just up to patch 4 ("mfd:
ocelot-spi: Change the regmap stride to reflect the real one")
functionality returns.

If you keep patch 4 and apply it before patch 2, everything should
work.

Sorry for the bug. Thanks for the fix. And I'm glad I'm not the only one
taking advantage of the "reg_shift" regmap operation! I thought I'd be
the only one.


Let me know if you want me to take any action on this fix.


Colin

Re: [RFC 6/7] net: ethernet: altera-tse: Convert to mdio-regmap and use PCS Lynx

[kernel test robot]

Hi Maxime,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on broonie-regmap/for-next]
[also build test ERROR on lee-mfd/for-mfd-next driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus linus/master v6.3-rc3 next-20230324]
[cannot apply to lee-mfd/for-mfd-fixes]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Maxime-Chevallier/regmap-add-a-helper-to-translate-the-register-address/20230324-173909
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next
patch link:    https://lore.kernel.org/r/20230324093644.464704-7-maxime.chevallier%40bootlin.com
patch subject: [RFC 6/7] net: ethernet: altera-tse: Convert to mdio-regmap and use PCS Lynx
config: nios2-defconfig (https://download.01.org/0day-ci/archive/20230325/202303250021.BjLa7zxR-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 12.1.0
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/38972c069ffca67815b706f76938d49e7a8a48a6
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Maxime-Chevallier/regmap-add-a-helper-to-translate-the-register-address/20230324-173909
        git checkout 38972c069ffca67815b706f76938d49e7a8a48a6
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=nios2 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=nios2 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/202303250021.BjLa7zxR-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/net/ethernet/altera/altera_tse_main.c: In function 'altera_tse_probe':
>> drivers/net/ethernet/altera/altera_tse_main.c:1152:30: error: storage size of 'pcs_regmap_cfg' isn't known
    1152 |         struct regmap_config pcs_regmap_cfg;
         |                              ^~~~~~~~~~~~~~
>> drivers/net/ethernet/altera/altera_tse_main.c:1293:22: error: implicit declaration of function 'devm_regmap_init_mmio' [-Werror=implicit-function-declaration]
    1293 |         pcs_regmap = devm_regmap_init_mmio(&pdev->dev, priv->pcs_base,
         |                      ^~~~~~~~~~~~~~~~~~~~~
   drivers/net/ethernet/altera/altera_tse_main.c:1152:30: warning: unused variable 'pcs_regmap_cfg' [-Wunused-variable]
    1152 |         struct regmap_config pcs_regmap_cfg;
         |                              ^~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +1152 drivers/net/ethernet/altera/altera_tse_main.c

  1136	
  1137	/* Probe Altera TSE MAC device
  1138	 */
  1139	static int altera_tse_probe(struct platform_device *pdev)
  1140	{
  1141		const struct of_device_id *of_id = NULL;
  1142		struct altera_tse_private *priv;
  1143		struct resource *control_port;
  1144		struct regmap *pcs_regmap;
  1145		struct resource *dma_res;
  1146		struct resource *pcs_res;
  1147		struct mii_bus *pcs_bus;
  1148		struct net_device *ndev;
  1149		void __iomem *descmap;
  1150		int ret = -ENODEV;
  1151	
> 1152		struct regmap_config pcs_regmap_cfg;
  1153	
  1154		struct mdio_regmap_config mrc = {
  1155			.parent = &pdev->dev,
  1156			.valid_addr = 0x1,
  1157		};
  1158	
  1159		ndev = alloc_etherdev(sizeof(struct altera_tse_private));
  1160		if (!ndev) {
  1161			dev_err(&pdev->dev, "Could not allocate network device\n");
  1162			return -ENODEV;
  1163		}
  1164	
  1165		SET_NETDEV_DEV(ndev, &pdev->dev);
  1166	
  1167		priv = netdev_priv(ndev);
  1168		priv->device = &pdev->dev;
  1169		priv->dev = ndev;
  1170		priv->msg_enable = netif_msg_init(debug, default_msg_level);
  1171	
  1172		of_id = of_match_device(altera_tse_ids, &pdev->dev);
  1173	
  1174		if (of_id)
  1175			priv->dmaops = (struct altera_dmaops *)of_id->data;
  1176	
  1177	
  1178		if (priv->dmaops &&
  1179		    priv->dmaops->altera_dtype == ALTERA_DTYPE_SGDMA) {
  1180			/* Get the mapped address to the SGDMA descriptor memory */
  1181			ret = request_and_map(pdev, "s1", &dma_res, &descmap);
  1182			if (ret)
  1183				goto err_free_netdev;
  1184	
  1185			/* Start of that memory is for transmit descriptors */
  1186			priv->tx_dma_desc = descmap;
  1187	
  1188			/* First half is for tx descriptors, other half for tx */
  1189			priv->txdescmem = resource_size(dma_res)/2;
  1190	
  1191			priv->txdescmem_busaddr = (dma_addr_t)dma_res->start;
  1192	
  1193			priv->rx_dma_desc = (void __iomem *)((uintptr_t)(descmap +
  1194							     priv->txdescmem));
  1195			priv->rxdescmem = resource_size(dma_res)/2;
  1196			priv->rxdescmem_busaddr = dma_res->start;
  1197			priv->rxdescmem_busaddr += priv->txdescmem;
  1198	
  1199			if (upper_32_bits(priv->rxdescmem_busaddr)) {
  1200				dev_dbg(priv->device,
  1201					"SGDMA bus addresses greater than 32-bits\n");
  1202				ret = -EINVAL;
  1203				goto err_free_netdev;
  1204			}
  1205			if (upper_32_bits(priv->txdescmem_busaddr)) {
  1206				dev_dbg(priv->device,
  1207					"SGDMA bus addresses greater than 32-bits\n");
  1208				ret = -EINVAL;
  1209				goto err_free_netdev;
  1210			}
  1211		} else if (priv->dmaops &&
  1212			   priv->dmaops->altera_dtype == ALTERA_DTYPE_MSGDMA) {
  1213			ret = request_and_map(pdev, "rx_resp", &dma_res,
  1214					      &priv->rx_dma_resp);
  1215			if (ret)
  1216				goto err_free_netdev;
  1217	
  1218			ret = request_and_map(pdev, "tx_desc", &dma_res,
  1219					      &priv->tx_dma_desc);
  1220			if (ret)
  1221				goto err_free_netdev;
  1222	
  1223			priv->txdescmem = resource_size(dma_res);
  1224			priv->txdescmem_busaddr = dma_res->start;
  1225	
  1226			ret = request_and_map(pdev, "rx_desc", &dma_res,
  1227					      &priv->rx_dma_desc);
  1228			if (ret)
  1229				goto err_free_netdev;
  1230	
  1231			priv->rxdescmem = resource_size(dma_res);
  1232			priv->rxdescmem_busaddr = dma_res->start;
  1233	
  1234		} else {
  1235			ret = -ENODEV;
  1236			goto err_free_netdev;
  1237		}
  1238	
  1239		if (!dma_set_mask(priv->device, DMA_BIT_MASK(priv->dmaops->dmamask))) {
  1240			dma_set_coherent_mask(priv->device,
  1241					      DMA_BIT_MASK(priv->dmaops->dmamask));
  1242		} else if (!dma_set_mask(priv->device, DMA_BIT_MASK(32))) {
  1243			dma_set_coherent_mask(priv->device, DMA_BIT_MASK(32));
  1244		} else {
  1245			ret = -EIO;
  1246			goto err_free_netdev;
  1247		}
  1248	
  1249		/* MAC address space */
  1250		ret = request_and_map(pdev, "control_port", &control_port,
  1251				      (void __iomem **)&priv->mac_dev);
  1252		if (ret)
  1253			goto err_free_netdev;
  1254	
  1255		/* xSGDMA Rx Dispatcher address space */
  1256		ret = request_and_map(pdev, "rx_csr", &dma_res,
  1257				      &priv->rx_dma_csr);
  1258		if (ret)
  1259			goto err_free_netdev;
  1260	
  1261	
  1262		/* xSGDMA Tx Dispatcher address space */
  1263		ret = request_and_map(pdev, "tx_csr", &dma_res,
  1264				      &priv->tx_dma_csr);
  1265		if (ret)
  1266			goto err_free_netdev;
  1267	
  1268		/* SGMII PCS address space. The location can vary depending on how the
  1269		 * IP is integrated. We can have a resource dedicated to it at a specific
  1270		 * address space, but if it's not the case, we fallback to the mdiophy0
  1271		 * from the MAC's address space
  1272		 */
  1273		ret = request_and_map(pdev, "pcs", &pcs_res,
  1274				      &priv->pcs_base);
  1275		if (ret) {
  1276			/* If we can't find a dedicated resource for the PCS, fallback
  1277			 * to the internal PCS, that has a different address stride
  1278			 */
  1279			priv->pcs_base = priv->mac_dev + tse_csroffs(mdio_phy0);
  1280			pcs_regmap_cfg.reg_bits = 32;
  1281			/* Values are MDIO-like values, on 16 bits */
  1282			pcs_regmap_cfg.val_bits = 16;
  1283			pcs_regmap_cfg.reg_stride = 4;
  1284			pcs_regmap_cfg.reg_shift = REGMAP_UPSHIFT(2);
  1285		} else {
  1286			pcs_regmap_cfg.reg_bits = 16;
  1287			pcs_regmap_cfg.val_bits = 16;
  1288			pcs_regmap_cfg.reg_stride = 2;
  1289			pcs_regmap_cfg.reg_shift = REGMAP_UPSHIFT(1);
  1290		}
  1291	
  1292		/* Create a regmap for the PCS so that it can be used by the PCS driver */
> 1293		pcs_regmap = devm_regmap_init_mmio(&pdev->dev, priv->pcs_base,
  1294						   &pcs_regmap_cfg);
  1295		if (IS_ERR(pcs_regmap)) {
  1296			ret = PTR_ERR(pcs_regmap);
  1297			goto err_free_netdev;
  1298		}
  1299		mrc.regmap = pcs_regmap;
  1300	
  1301		/* Rx IRQ */
  1302		priv->rx_irq = platform_get_irq_byname(pdev, "rx_irq");
  1303		if (priv->rx_irq == -ENXIO) {
  1304			dev_err(&pdev->dev, "cannot obtain Rx IRQ\n");
  1305			ret = -ENXIO;
  1306			goto err_free_netdev;
  1307		}
  1308	
  1309		/* Tx IRQ */
  1310		priv->tx_irq = platform_get_irq_byname(pdev, "tx_irq");
  1311		if (priv->tx_irq == -ENXIO) {
  1312			dev_err(&pdev->dev, "cannot obtain Tx IRQ\n");
  1313			ret = -ENXIO;
  1314			goto err_free_netdev;
  1315		}
  1316	
  1317		/* get FIFO depths from device tree */
  1318		if (of_property_read_u32(pdev->dev.of_node, "rx-fifo-depth",
  1319					 &priv->rx_fifo_depth)) {
  1320			dev_err(&pdev->dev, "cannot obtain rx-fifo-depth\n");
  1321			ret = -ENXIO;
  1322			goto err_free_netdev;
  1323		}
  1324	
  1325		if (of_property_read_u32(pdev->dev.of_node, "tx-fifo-depth",
  1326					 &priv->tx_fifo_depth)) {
  1327			dev_err(&pdev->dev, "cannot obtain tx-fifo-depth\n");
  1328			ret = -ENXIO;
  1329			goto err_free_netdev;
  1330		}
  1331	
  1332		/* get hash filter settings for this instance */
  1333		priv->hash_filter =
  1334			of_property_read_bool(pdev->dev.of_node,
  1335					      "altr,has-hash-multicast-filter");
  1336	
  1337		/* Set hash filter to not set for now until the
  1338		 * multicast filter receive issue is debugged
  1339		 */
  1340		priv->hash_filter = 0;
  1341	
  1342		/* get supplemental address settings for this instance */
  1343		priv->added_unicast =
  1344			of_property_read_bool(pdev->dev.of_node,
  1345					      "altr,has-supplementary-unicast");
  1346	
  1347		priv->dev->min_mtu = ETH_ZLEN + ETH_FCS_LEN;
  1348		/* Max MTU is 1500, ETH_DATA_LEN */
  1349		priv->dev->max_mtu = ETH_DATA_LEN;
  1350	
  1351		/* Get the max mtu from the device tree. Note that the
  1352		 * "max-frame-size" parameter is actually max mtu. Definition
  1353		 * in the ePAPR v1.1 spec and usage differ, so go with usage.
  1354		 */
  1355		of_property_read_u32(pdev->dev.of_node, "max-frame-size",
  1356				     &priv->dev->max_mtu);
  1357	
  1358		/* The DMA buffer size already accounts for an alignment bias
  1359		 * to avoid unaligned access exceptions for the NIOS processor,
  1360		 */
  1361		priv->rx_dma_buf_sz = ALTERA_RXDMABUFFER_SIZE;
  1362	
  1363		/* get default MAC address from device tree */
  1364		ret = of_get_ethdev_address(pdev->dev.of_node, ndev);
  1365		if (ret)
  1366			eth_hw_addr_random(ndev);
  1367	
  1368		/* get phy addr and create mdio */
  1369		ret = altera_tse_phy_get_addr_mdio_create(ndev);
  1370	
  1371		if (ret)
  1372			goto err_free_netdev;
  1373	
  1374		/* initialize netdev */
  1375		ndev->mem_start = control_port->start;
  1376		ndev->mem_end = control_port->end;
  1377		ndev->netdev_ops = &altera_tse_netdev_ops;
  1378		altera_tse_set_ethtool_ops(ndev);
  1379	
  1380		altera_tse_netdev_ops.ndo_set_rx_mode = tse_set_rx_mode;
  1381	
  1382		if (priv->hash_filter)
  1383			altera_tse_netdev_ops.ndo_set_rx_mode =
  1384				tse_set_rx_mode_hashfilter;
  1385	
  1386		/* Scatter/gather IO is not supported,
  1387		 * so it is turned off
  1388		 */
  1389		ndev->hw_features &= ~NETIF_F_SG;
  1390		ndev->features |= ndev->hw_features | NETIF_F_HIGHDMA;
  1391	
  1392		/* VLAN offloading of tagging, stripping and filtering is not
  1393		 * supported by hardware, but driver will accommodate the
  1394		 * extra 4-byte VLAN tag for processing by upper layers
  1395		 */
  1396		ndev->features |= NETIF_F_HW_VLAN_CTAG_RX;
  1397	
  1398		/* setup NAPI interface */
  1399		netif_napi_add(ndev, &priv->napi, tse_poll);
  1400	
  1401		spin_lock_init(&priv->mac_cfg_lock);
  1402		spin_lock_init(&priv->tx_lock);
  1403		spin_lock_init(&priv->rxdma_irq_lock);
  1404	
  1405		netif_carrier_off(ndev);
  1406		ret = register_netdev(ndev);
  1407		if (ret) {
  1408			dev_err(&pdev->dev, "failed to register TSE net device\n");
  1409			goto err_register_netdev;
  1410		}
  1411	
  1412		platform_set_drvdata(pdev, ndev);
  1413	
  1414		priv->revision = ioread32(&priv->mac_dev->megacore_revision);
  1415	
  1416		if (netif_msg_probe(priv))
  1417			dev_info(&pdev->dev, "Altera TSE MAC version %d.%d at 0x%08lx irq %d/%d\n",
  1418				 (priv->revision >> 8) & 0xff,
  1419				 priv->revision & 0xff,
  1420				 (unsigned long) control_port->start, priv->rx_irq,
  1421				 priv->tx_irq);
  1422	
  1423		snprintf(mrc.name, MII_BUS_ID_SIZE, "%s-pcs-mii", ndev->name);
  1424		pcs_bus = devm_mdio_regmap_register(&pdev->dev, &mrc);
  1425		priv->pcs_mdiodev = mdio_device_create(pcs_bus, 0);
  1426	
  1427		priv->pcs = lynx_pcs_create(priv->pcs_mdiodev);
  1428		if (!priv->pcs) {
  1429			ret = -ENODEV;
  1430			goto err_init_phy;
  1431		}
  1432	
  1433		priv->phylink_config.dev = &ndev->dev;
  1434		priv->phylink_config.type = PHYLINK_NETDEV;
  1435		priv->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_10 |
  1436							MAC_100 | MAC_1000FD;
  1437	
  1438		phy_interface_set_rgmii(priv->phylink_config.supported_interfaces);
  1439		__set_bit(PHY_INTERFACE_MODE_MII,
  1440			  priv->phylink_config.supported_interfaces);
  1441		__set_bit(PHY_INTERFACE_MODE_GMII,
  1442			  priv->phylink_config.supported_interfaces);
  1443		__set_bit(PHY_INTERFACE_MODE_SGMII,
  1444			  priv->phylink_config.supported_interfaces);
  1445		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
  1446			  priv->phylink_config.supported_interfaces);
  1447	
  1448		priv->phylink = phylink_create(&priv->phylink_config,
  1449					       of_fwnode_handle(priv->device->of_node),
  1450					       priv->phy_iface, &alt_tse_phylink_ops);
  1451		if (IS_ERR(priv->phylink)) {
  1452			dev_err(&pdev->dev, "failed to create phylink\n");
  1453			ret = PTR_ERR(priv->phylink);
  1454			goto err_pcs;
  1455		}
  1456	
  1457		return 0;
  1458	err_pcs:
  1459		mdio_device_free(priv->pcs_mdiodev);
  1460	err_init_phy:
  1461		unregister_netdev(ndev);
  1462	err_register_netdev:
  1463		netif_napi_del(&priv->napi);
  1464		altera_tse_mdio_destroy(ndev);
  1465	err_free_netdev:
  1466		free_netdev(ndev);
  1467		return ret;
  1468	}
  1469	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [RFC 6/7] net: ethernet: altera-tse: Convert to mdio-regmap and use PCS Lynx

[kernel test robot]

Hi Maxime,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on broonie-regmap/for-next]
[also build test ERROR on lee-mfd/for-mfd-next driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus linus/master v6.3-rc3 next-20230324]
[cannot apply to lee-mfd/for-mfd-fixes]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Maxime-Chevallier/regmap-add-a-helper-to-translate-the-register-address/20230324-173909
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next
patch link:    https://lore.kernel.org/r/20230324093644.464704-7-maxime.chevallier%40bootlin.com
patch subject: [RFC 6/7] net: ethernet: altera-tse: Convert to mdio-regmap and use PCS Lynx
config: x86_64-randconfig-a002 (https://download.01.org/0day-ci/archive/20230325/202303250021.qYlsLPIN-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/38972c069ffca67815b706f76938d49e7a8a48a6
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Maxime-Chevallier/regmap-add-a-helper-to-translate-the-register-address/20230324-173909
        git checkout 38972c069ffca67815b706f76938d49e7a8a48a6
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 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/202303250021.qYlsLPIN-lkp@intel.com/

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

>> ERROR: modpost: "lynx_pcs_create" [drivers/net/ethernet/altera/altera_tse.ko] undefined!
>> ERROR: modpost: "devm_mdio_regmap_register" [drivers/net/ethernet/altera/altera_tse.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [RFC 4/7] mfd: ocelot-spi: Change the regmap stride to reflect the real one

[Colin Foster]

On Fri, Mar 24, 2023 at 08:48:18AM -0700, Colin Foster wrote:
> Hi Maxime,
> 
> On Fri, Mar 24, 2023 at 01:48:17PM +0100, Maxime Chevallier wrote:
> > Hello Andrew,
> > 
> > On Fri, 24 Mar 2023 13:11:07 +0100
> > Andrew Lunn <andrew@lunn.ch> wrote:
> > 
> > > >  	.reg_bits = 24,
> > > > -	.reg_stride = 4,
> > > > +	.reg_stride = 1,
> > > >  	.reg_shift = REGMAP_DOWNSHIFT(2),
> > > >  	.val_bits = 32,  
> > > 
> > > This does not look like a bisectable change? Or did it never work
> > > before?
> > 
> > Actually this works in all cases because of "regmap: check for alignment
> > on translated register addresses" in this series. Before this series,
> > I think using a stride of 1 would have worked too, as any 4-byte-aligned
> > accesses are also 1-byte aligned.
> > 
> > But that's also why I need review on this, my understanding is that
> > reg_stride is used just as a check for alignment, and I couldn't test
> > this ocelot-related patch on the real HW, so please take it with a
> > grain of salt :(
> 
> You're exactly right. reg_stride wasn't used anywhere in the
> ocelot-spi path before this patch series. When I build against patch 3
> ("regmap: allow upshifting register addresses before performing
> operations") ocelot-spi breaks.
> 
> [    3.207711] ocelot-soc spi0.0: error -EINVAL: Error initializing SPI bus
> 
> When I build against the whole series, or even just up to patch 4 ("mfd:
> ocelot-spi: Change the regmap stride to reflect the real one")
> functionality returns.
> 
> If you keep patch 4 and apply it before patch 2, everything should
> work.

I replied too soon, before looking more into patch 2.

Some context from that patch:

--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -2016,7 +2016,7 @@ int regmap_write(struct regmap *map, unsigned int reg, unsigned int val)
 {
        int ret;

-       if (!IS_ALIGNED(reg, map->reg_stride))
+       if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride))
                return -EINVAL;

        map->lock(map->lock_arg);


I don't know whether checking IS_ALIGNED before or after the shift is
the right thing to do. My initial intention was to perform the shift at
the last possible moment before calling into the read / write routines.
That way it wouldn't interfere with any underlying regcache mechanisms
(which aren't used by ocelot-spi)

But to me it seems like patch 2 changes this expected behavior, so the
two patches should be squashed.


... Thinking more about it ...


In ocelot-spi, at the driver layer, we're accessing two registers.
They'd be at address 0x71070000 and 0x71070004. The driver uses those
addresses, so there's a stride of 4. I can't access 0x71070001.

The fact that the translation from "address" to "bits that go out the
SPI bus" shifts out the last two bits and hacks off a couple of the MSBs
doesn't seem like it should affect the 'reg_stride'.


So maybe patches 2 and 4 should be dropped, and your patch 6
alterra_tse_main should use a reg_stride of 1? That has a subtle benefit
of not needing an additional operation or two from regmap_reg_addr().

Would that cause any issues? Hopefully there isn't something I'm
missing.


(Aside: I'm now curious how the compiler will optimize
regmap_reg_addr())


Colin

Re: [RFC 2/7] regmap: check for alignment on translated register addresses

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 618 bytes --]

On Fri, Mar 24, 2023 at 10:36:39AM +0100, Maxime Chevallier wrote:
> With regmap->reg_base and regmap->reg_downshift, the actual register
> address that is going to be used for the next operation might not be the
> same as the one we have as an input. Addresses can be offset with
> reg_base and shifted, which will affect alignment.
> 
> Check for alignment on the real register address.

It is not at all clear to me that the combination of stride and
downshift particularly makes sense, and especially not that the
stride should be applied after downshifting rather than to what
the user is passing in.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: (subset) [RFC 0/7] Introduce a generic regmap-based MDIO driver

[Mark Brown]

On Fri, 24 Mar 2023 10:36:37 +0100, Maxime Chevallier wrote:
> When the Altera TSE PCS driver was initially introduced, there were
> comments by Russell that the register layout looked very familiar to the
> existing Lynx PCS driver, the only difference being that the TSE PCS
> driver is memory-mapped whereas the Lynx PCS driver sits on an MDIO bus.
> 
> Since then, I've sent a followup to create a wrapper around Lynx, that
> would create a virtual MDIO bus driver that would translate the mdio
> operations to mmio operations [1].
> 
> [...]

Applied to

   broonie/regmap.git for-next

Thanks!

[1/7] regmap: add a helper to translate the register address
      commit: 3f58f6dc4d92ed6fae4a4da0d5b091e00ec10fa8

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [RFC 4/7] mfd: ocelot-spi: Change the regmap stride to reflect the real one

[Andrew Lunn]

> > >  static const struct regmap_config ocelot_spi_regmap_config = {
> > >  	.reg_bits = 24,
> > > -	.reg_stride = 4,
> > > +	.reg_stride = 1,
> > >  	.reg_shift = REGMAP_DOWNSHIFT(2),
> > >  	.val_bits = 32,  
> > 
> > This does not look like a bisectable change? Or did it never work
> > before?
> 
> Actually this works in all cases because of "regmap: check for alignment
> on translated register addresses" in this series. Before this series,
> I think using a stride of 1 would have worked too, as any 4-byte-aligned
> accesses are also 1-byte aligned.

This is the sort of think which is good to explain in the commit
message. That is the place to answer questions reviewers are likely to
ask for things which are not obvious from just the patch.

    Andrew

Re: [RFC 2/7] regmap: check for alignment on translated register addresses

[Maxime Chevallier]

Hello Mark,

On Fri, 24 Mar 2023 18:51:19 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Fri, Mar 24, 2023 at 10:36:39AM +0100, Maxime Chevallier wrote:
> > With regmap->reg_base and regmap->reg_downshift, the actual register
> > address that is going to be used for the next operation might not
> > be the same as the one we have as an input. Addresses can be offset
> > with reg_base and shifted, which will affect alignment.
> > 
> > Check for alignment on the real register address.  
> 
> It is not at all clear to me that the combination of stride and
> downshift particularly makes sense, and especially not that the
> stride should be applied after downshifting rather than to what
> the user is passing in.

I agree on the part where the ordering of "adding and offset, then
down/upshifting" isn't natural. This is the order in which operations
are done today, and from what I could gather, only the ocelot-spi MFD
driver uses both of these operations.

It would indeed make sense to first shift the register to have the
proper spacing between register addresses, then adding the offset.

So maybe we should address that in ocelot-spi in the next iteration,
Colin, would you agree ?

Thanks,

Maxime

Re: [RFC 4/7] mfd: ocelot-spi: Change the regmap stride to reflect the real one

[Maxime Chevallier]

Hello Andrew,

On Mon, 27 Mar 2023 02:02:55 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > > >  static const struct regmap_config ocelot_spi_regmap_config = {
> > > >  	.reg_bits = 24,
> > > > -	.reg_stride = 4,
> > > > +	.reg_stride = 1,
> > > >  	.reg_shift = REGMAP_DOWNSHIFT(2),
> > > >  	.val_bits = 32,    
> > > 
> > > This does not look like a bisectable change? Or did it never work
> > > before?  
> > 
> > Actually this works in all cases because of "regmap: check for
> > alignment on translated register addresses" in this series. Before
> > this series, I think using a stride of 1 would have worked too, as
> > any 4-byte-aligned accesses are also 1-byte aligned.  
> 
> This is the sort of think which is good to explain in the commit
> message. That is the place to answer questions reviewers are likely to
> ask for things which are not obvious from just the patch.

That's right, I will include this explanation in the next iteration.
Thanks for the review,

Maxime

>     Andrew

Re: [RFC 4/7] mfd: ocelot-spi: Change the regmap stride to reflect the real one

[Maxime Chevallier]

On Fri, 24 Mar 2023 10:56:05 -0700
Colin Foster <colin.foster@in-advantage.com> wrote:

> On Fri, Mar 24, 2023 at 08:48:18AM -0700, Colin Foster wrote:
> > Hi Maxime,
> > 
> > On Fri, Mar 24, 2023 at 01:48:17PM +0100, Maxime Chevallier wrote:  
> > > Hello Andrew,
> > > 
> > > On Fri, 24 Mar 2023 13:11:07 +0100
> > > Andrew Lunn <andrew@lunn.ch> wrote:
> > >   
> > > > >  	.reg_bits = 24,
> > > > > -	.reg_stride = 4,
> > > > > +	.reg_stride = 1,
> > > > >  	.reg_shift = REGMAP_DOWNSHIFT(2),
> > > > >  	.val_bits = 32,    
> > > > 
> > > > This does not look like a bisectable change? Or did it never
> > > > work before?  
> > > 
> > > Actually this works in all cases because of "regmap: check for
> > > alignment on translated register addresses" in this series.
> > > Before this series, I think using a stride of 1 would have worked
> > > too, as any 4-byte-aligned accesses are also 1-byte aligned.
> > > 
> > > But that's also why I need review on this, my understanding is
> > > that reg_stride is used just as a check for alignment, and I
> > > couldn't test this ocelot-related patch on the real HW, so please
> > > take it with a grain of salt :(  
> > 
> > You're exactly right. reg_stride wasn't used anywhere in the
> > ocelot-spi path before this patch series. When I build against
> > patch 3 ("regmap: allow upshifting register addresses before
> > performing operations") ocelot-spi breaks.
> > 
> > [    3.207711] ocelot-soc spi0.0: error -EINVAL: Error initializing
> > SPI bus
> > 
> > When I build against the whole series, or even just up to patch 4
> > ("mfd: ocelot-spi: Change the regmap stride to reflect the real
> > one") functionality returns.
> > 
> > If you keep patch 4 and apply it before patch 2, everything should
> > work.  
> 
> I replied too soon, before looking more into patch 2.
> 
> Some context from that patch:
> 
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -2016,7 +2016,7 @@ int regmap_write(struct regmap *map, unsigned
> int reg, unsigned int val) {
>         int ret;
> 
> -       if (!IS_ALIGNED(reg, map->reg_stride))
> +       if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride))
>                 return -EINVAL;
> 
>         map->lock(map->lock_arg);
> 
> 
> I don't know whether checking IS_ALIGNED before or after the shift is
> the right thing to do. My initial intention was to perform the shift
> at the last possible moment before calling into the read / write
> routines. That way it wouldn't interfere with any underlying regcache
> mechanisms (which aren't used by ocelot-spi)
> 
> But to me it seems like patch 2 changes this expected behavior, so the
> two patches should be squashed.
> 
> 
> ... Thinking more about it ...
> 
> 
> In ocelot-spi, at the driver layer, we're accessing two registers.
> They'd be at address 0x71070000 and 0x71070004. The driver uses those
> addresses, so there's a stride of 4. I can't access 0x71070001.
>
> The fact that the translation from "address" to "bits that go out the
> SPI bus" shifts out the last two bits and hacks off a couple of the
> MSBs doesn't seem like it should affect the 'reg_stride'.
> 
> 
> So maybe patches 2 and 4 should be dropped, and your patch 6
> alterra_tse_main should use a reg_stride of 1? That has a subtle
> benefit of not needing an additional operation or two from
> regmap_reg_addr().
> 
> Would that cause any issues? Hopefully there isn't something I'm
> missing.

Well here I guess it's also about the semantic of reg_stride. Should it
represent the alignment constraints of the register address we feed as
an input to a regmap_read/regmap_write operation, or the alignment
constraints of the underlying bus ? This is kind of a new concern, as
we are now translating register addresses.

I asked myself the same question, so I'm very open for discussion, but
my gut feeling is that the reg_stride is there to make sure we don't
perform an access whose alignment won't work with the bus we are using,
so using a stride of 1 on a memory-mapped device with 2 or 4 byte
register alignment is a bit counter-intuitive.

Thanks a lot for the review, suggestions and tests !

Best regards,

Maxime

> 
> (Aside: I'm now curious how the compiler will optimize
> regmap_reg_addr())
> 
> 
> Colin

Re: [RFC 2/7] regmap: check for alignment on translated register addresses

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 767 bytes --]

On Thu, Mar 30, 2023 at 11:45:46AM +0200, Maxime Chevallier wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > It is not at all clear to me that the combination of stride and
> > downshift particularly makes sense, and especially not that the
> > stride should be applied after downshifting rather than to what
> > the user is passing in.

> I agree on the part where the ordering of "adding and offset, then
> down/upshifting" isn't natural. This is the order in which operations
> are done today, and from what I could gather, only the ocelot-spi MFD
> driver uses both of these operations.

Right.  I don't think the ordering is particularly intentional,
like I say it's not clear that combinding the two makes sense so
I think it just wasn't considered at all.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 2/7] regmap: check for alignment on translated register addresses

[Colin Foster]

Hi Maxime,

On Thu, Mar 30, 2023 at 11:45:46AM +0200, Maxime Chevallier wrote:
> Hello Mark,
> 
> On Fri, 24 Mar 2023 18:51:19 +0000
> Mark Brown <broonie@kernel.org> wrote:
> 
> > On Fri, Mar 24, 2023 at 10:36:39AM +0100, Maxime Chevallier wrote:
> > > With regmap->reg_base and regmap->reg_downshift, the actual register
> > > address that is going to be used for the next operation might not
> > > be the same as the one we have as an input. Addresses can be offset
> > > with reg_base and shifted, which will affect alignment.
> > > 
> > > Check for alignment on the real register address.  
> > 
> > It is not at all clear to me that the combination of stride and
> > downshift particularly makes sense, and especially not that the
> > stride should be applied after downshifting rather than to what
> > the user is passing in.
> 
> I agree on the part where the ordering of "adding and offset, then
> down/upshifting" isn't natural. This is the order in which operations
> are done today, and from what I could gather, only the ocelot-spi MFD
> driver uses both of these operations.
> 
> It would indeed make sense to first shift the register to have the
> proper spacing between register addresses, then adding the offset.
> 
> So maybe we should address that in ocelot-spi in the next iteration,
> Colin, would you agree ?

I'm curious what you mean by "proper spacing". The proper spacing of the
VSC7512 (ocelot-spi) is 4, and that's what the reg_stride is. All
registers can be accessed in many ways, as I described.

Consider the case I brought up, where we're trying to access a register
at 0x71070004. If you're on the processor internal to ocelot, this would
be

uint32 val = *(uint32 *)0x71070004;

Accesses to 0x71070001, 0x71070002, and 0x71070003 are not possible.
(Well maybe they are _possible_ on some architectures... you get the
point though)


Translate this to accessing the same register via SPI, where everything
is shifted down two bits. We're still accessing 0x71070004, but the way
this gets sent on the bus is 0x71070004 >> 2 = 0x1c41c001.

This patch set would have allowed accesses to 0x71070001 in the
ocelot-spi scenario, but not in the MMIO scenario. That wouldn't be
desired.


The result of all this is a consistent driver interface. "Give me
register 0x4 from a base address of 0x71070000" works in both MMIO and
SPI. Everything in /sys/kernel/debug/regmap should be the same in both
scenarios as well. All of these scenarios were the motivation behind
reg_shift.


Colin Foster

Re: [RFC 5/7] net: mdio: Introduce a regmap-based mdio driver

[Vladimir Oltean]

Hi Maxime,

On Fri, Mar 24, 2023 at 10:36:42AM +0100, 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>
> ---
>  MAINTAINERS                      |  7 +++
>  drivers/net/mdio/Kconfig         | 11 +++++
>  drivers/net/mdio/Makefile        |  1 +
>  drivers/net/mdio/mdio-regmap.c   | 85 ++++++++++++++++++++++++++++++++
>  include/linux/mdio/mdio-regmap.h | 25 ++++++++++
>  5 files changed, 129 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 8d5bc223f305..10b3a1800e0d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12751,6 +12751,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/mdio/Kconfig b/drivers/net/mdio/Kconfig
> index 90309980686e..671e4bb82e3e 100644
> --- a/drivers/net/mdio/Kconfig
> +++ b/drivers/net/mdio/Kconfig
> @@ -182,6 +182,17 @@ config MDIO_IPQ8064
>  	  This driver supports the MDIO interface found in the network
>  	  interface units of the IPQ8064 SoC
>  
> +config MDIO_REGMAP
> +	tristate "Regmap-based virtual MDIO bus driver"

IMO this shouldn't have the text visible to the Kconfig user (just
"tristate"); it should only be selectable by the driver which makes use
of the exported symbol.

AFAIK, Kconfig options which are selectable should not depend on
anything. The dependency should transfer to the options which selects
it (here REGMAP). Optionally there can be a comment specifying that the

> +	depends on REGMAP
> +	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..c85d62c2f55c
> --- /dev/null
> +++ b/drivers/net/mdio/mdio-regmap.c
> @@ -0,0 +1,85 @@
> +// 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 & BIT(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 & BIT(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);
> +
> +	if (!config->valid_addr)
> +		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->parent = config->parent;

mrc->parent doesn't seem to be used

> +	mrc->valid_addr = config->valid_addr;
> +
> +	mii->name = DRV_NAME;
> +	strncpy(mii->id, config->name, MII_BUS_ID_SIZE);
> +	mii->parent = config->parent;
> +	mii->read = mdio_regmap_read_c22;
> +	mii->write = mdio_regmap_write_c22;
> +
> +	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;
> +}
> +
> diff --git a/include/linux/mdio/mdio-regmap.h b/include/linux/mdio/mdio-regmap.h
> new file mode 100644
> index 000000000000..ea428e5a2913
> --- /dev/null
> +++ b/include/linux/mdio/mdio-regmap.h
> @@ -0,0 +1,25 @@
> +/* 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
> +
> +#define MDIO_REGMAP_NAME 63

hmm.. NAME_LEN would be less confusing for a name. Although... why not
MII_BUS_ID_SIZE directly, seeing that this is how it is actually used?

> +
> +struct device;
> +struct regmap;
> +
> +struct mdio_regmap_config {
> +	struct device *parent;
> +	struct regmap *regmap;
> +	char name[MDIO_REGMAP_NAME];
> +	u32 valid_addr;

Would it be better to name this valid_addr_mask, since that is how it is
used? Or is the "ctx->valid_addr & BIT(addr)" check wrong (should have
been "ctx->valid_addr == addr")? How is the driver supposed to work with
multiple valid addresses?

> +};
> +
> +struct mii_bus *devm_mdio_regmap_register(struct device *dev,
> +					  const struct mdio_regmap_config *config);
> +
> +#endif
> -- 
> 2.39.2
>

Re: [RFC 7/7] net: pcs: Drop the TSE PCS driver

[Vladimir Oltean]

Hi Maxime,

On Fri, Mar 24, 2023 at 10:36:44AM +0100, Maxime Chevallier wrote:
> 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>
> ---
> -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);
> -}

I just noticed this difference between the Lynx PCS and Altera TSE PCS
drivers. The Lynx driver doesn't reset the PCS, and if it did, it would
wait until the opposite condition from yours would be true: BMCR_RESET
should clear: "!(bmcr & BMCR_RESET)".

Is your reset procedure correct, I wonder?

Re: [RFC 7/7] net: pcs: Drop the TSE PCS driver

[Maxime Chevallier]

Hello Vlad,

On Mon, 15 May 2023 14:48:09 +0300
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> Hi Maxime,
> 
> On Fri, Mar 24, 2023 at 10:36:44AM +0100, Maxime Chevallier wrote:
> > 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>
> > ---
> > -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);
> > -}  
> 
> I just noticed this difference between the Lynx PCS and Altera TSE PCS
> drivers. The Lynx driver doesn't reset the PCS, and if it did, it
> would wait until the opposite condition from yours would be true:
> BMCR_RESET should clear: "!(bmcr & BMCR_RESET)".
> 
> Is your reset procedure correct, I wonder?

I also wonder... My understanding from the documentation I had is that
the PCS needed to be reset every time a major configuration was
changed, as the reset would put the internal state machines back to
their init condition, but without resetting the whole IP.
When performing my initial tests, this was indeed necessary. However
when porting to Lynx and running tests, it pretty much worked out of the
box, and I'm quite happy with it being that way. So I didn't dig much
deeper on why the reset was needed with the TSE PCS in the first place.

On a side note, the final dependencies for the final removal of TSE
were merged into the regmap tree, I'll therefore send a new version of
that series shortly.

Thanks for the review Vlad,

Best Regards,

Maxime