[RFC/RFT PATCH net-next 0/4] KSZ DSA driver: xMII speed adjustment and partial reg_fields conversion

[RFC/RFT PATCH net-next 0/4] KSZ DSA driver: xMII speed adjustment and partial reg_fields conversion

[Vladimir Oltean]

Hi,

Yesterday I picked up this patch and resubmitted it:
https://patchwork.kernel.org/project/netdevbpf/patch/20230222031738.189025-1-marex@denx.de/
here:
https://patchwork.kernel.org/project/netdevbpf/patch/20230315231916.2998480-1-vladimir.oltean@nxp.com/

and today I'm trying to address the rest of the points brought up in
that conversation, namely:

- commit c476bede4b0f ("net: dsa: microchip: ksz8795: use common xmii
  function") stopped adjusting the xMII port speed on KSZ8795, does it
  still work? No idea. Patch 3/4 deals with that.

- Mapping P_XMII_CTRL_0 and P_XMII_CTRL_1 to the same value on KSZ8795
  raised some eyebrows, and some reading shows that it is also partially
  incorrect (see patch 2/4). This is also where I propose to convert to
  reg_fields.

As it turns out, patch 2/4 is a dependency for patch 3/4, even if 3/4
may be a fix.

Patch 1/4 is a dependency of 2/4.

Patch 4/4 is something I also noticed during review. I left it at the
end so that it won't conflict with something that could reasonably be
submitted as a bug fix.

ABSOLUTELY NO TESTING WAS DONE. I don't have the hardware.

THIS BREAKS EVERYTHING EXCEPT FOR KSZ8795. Any testers should test on
that if possible (due to both patches 2/4, and 3/4).

Vladimir Oltean (4):
  net: dsa: microchip: add an enum for regmap widths
  net: dsa: microchip: partial conversion to regfields API for KSZ8795
    (WIP)
  net: dsa: microchip: allow setting xMII port speed/duplex on
    KSZ8765/KSZ8794/KSZ8795
  net: dsa: microchip: remove unused dev->dev_ops->phylink_mac_config()

 drivers/net/dsa/microchip/ksz8795.c      |  45 ++--
 drivers/net/dsa/microchip/ksz8863_smi.c  |  11 +-
 drivers/net/dsa/microchip/ksz9477.c      |  24 +--
 drivers/net/dsa/microchip/ksz9477_i2c.c  |  11 +-
 drivers/net/dsa/microchip/ksz_common.c   | 256 +++++++++++++----------
 drivers/net/dsa/microchip/ksz_common.h   | 110 +++++++---
 drivers/net/dsa/microchip/ksz_spi.c      |   6 +-
 drivers/net/dsa/microchip/lan937x_main.c |   8 +-
 8 files changed, 299 insertions(+), 172 deletions(-)

-- 
2.34.1

[RFC/RFT PATCH net-next 1/4] net: dsa: microchip: add an enum for regmap widths

[Vladimir Oltean]

It is not immediately obvious that this driver allocates, via the
KSZ_REGMAP_TABLE() macro, 3 regmaps for register access: dev->regmap[0]
for 8-bit access, dev->regmap[1] for 16-bit and dev->regmap[2] for
32-bit access.

In future changes that add support for reg_fields, each field will have
to specify through which of the 3 regmaps it's going to go. Add an enum
now, to denote one of the 3 register access widths, and make the code go
through some wrapper functions for easier review and further
modification.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/microchip/ksz8795.c      |  8 ++--
 drivers/net/dsa/microchip/ksz8863_smi.c  |  2 +-
 drivers/net/dsa/microchip/ksz9477.c      | 24 +++++------
 drivers/net/dsa/microchip/ksz9477_i2c.c  |  2 +-
 drivers/net/dsa/microchip/ksz_common.c   |  6 +--
 drivers/net/dsa/microchip/ksz_common.h   | 53 +++++++++++++++++-------
 drivers/net/dsa/microchip/ksz_spi.c      |  2 +-
 drivers/net/dsa/microchip/lan937x_main.c |  8 ++--
 8 files changed, 63 insertions(+), 42 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 003b0ac2854c..7f9ab6d13952 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -28,13 +28,13 @@
 
 static void ksz_cfg(struct ksz_device *dev, u32 addr, u8 bits, bool set)
 {
-	regmap_update_bits(dev->regmap[0], addr, bits, set ? bits : 0);
+	regmap_update_bits(ksz_regmap_8(dev), addr, bits, set ? bits : 0);
 }
 
 static void ksz_port_cfg(struct ksz_device *dev, int port, int offset, u8 bits,
 			 bool set)
 {
-	regmap_update_bits(dev->regmap[0], PORT_CTRL_ADDR(port, offset),
+	regmap_update_bits(ksz_regmap_8(dev), PORT_CTRL_ADDR(port, offset),
 			   bits, set ? bits : 0);
 }
 
@@ -1375,14 +1375,14 @@ int ksz8_setup(struct dsa_switch *ds)
 	ksz_cfg(dev, S_LINK_AGING_CTRL, SW_LINK_AUTO_AGING, true);
 
 	/* Enable aggressive back off algorithm in half duplex mode. */
-	regmap_update_bits(dev->regmap[0], REG_SW_CTRL_1,
+	regmap_update_bits(ksz_regmap_8(dev), REG_SW_CTRL_1,
 			   SW_AGGR_BACKOFF, SW_AGGR_BACKOFF);
 
 	/*
 	 * Make sure unicast VLAN boundary is set as default and
 	 * enable no excessive collision drop.
 	 */
-	regmap_update_bits(dev->regmap[0], REG_SW_CTRL_2,
+	regmap_update_bits(ksz_regmap_8(dev), REG_SW_CTRL_2,
 			   UNICAST_VLAN_BOUNDARY | NO_EXC_COLLISION_DROP,
 			   UNICAST_VLAN_BOUNDARY | NO_EXC_COLLISION_DROP);
 
diff --git a/drivers/net/dsa/microchip/ksz8863_smi.c b/drivers/net/dsa/microchip/ksz8863_smi.c
index 2f4623f3bd85..5871f27451cb 100644
--- a/drivers/net/dsa/microchip/ksz8863_smi.c
+++ b/drivers/net/dsa/microchip/ksz8863_smi.c
@@ -145,7 +145,7 @@ static int ksz8863_smi_probe(struct mdio_device *mdiodev)
 	if (!dev)
 		return -ENOMEM;
 
-	for (i = 0; i < ARRAY_SIZE(ksz8863_regmap_config); i++) {
+	for (i = 0; i < __KSZ_NUM_REGMAPS; i++) {
 		rc = ksz8863_regmap_config[i];
 		rc.lock_arg = &dev->regmap_mutex;
 		dev->regmap[i] = devm_regmap_init(&mdiodev->dev,
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index bf13d47c26cf..3019f54049fc 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -21,25 +21,25 @@
 
 static void ksz_cfg(struct ksz_device *dev, u32 addr, u8 bits, bool set)
 {
-	regmap_update_bits(dev->regmap[0], addr, bits, set ? bits : 0);
+	regmap_update_bits(ksz_regmap_8(dev), addr, bits, set ? bits : 0);
 }
 
 static void ksz_port_cfg(struct ksz_device *dev, int port, int offset, u8 bits,
 			 bool set)
 {
-	regmap_update_bits(dev->regmap[0], PORT_CTRL_ADDR(port, offset),
+	regmap_update_bits(ksz_regmap_8(dev), PORT_CTRL_ADDR(port, offset),
 			   bits, set ? bits : 0);
 }
 
 static void ksz9477_cfg32(struct ksz_device *dev, u32 addr, u32 bits, bool set)
 {
-	regmap_update_bits(dev->regmap[2], addr, bits, set ? bits : 0);
+	regmap_update_bits(ksz_regmap_32(dev), addr, bits, set ? bits : 0);
 }
 
 static void ksz9477_port_cfg32(struct ksz_device *dev, int port, int offset,
 			       u32 bits, bool set)
 {
-	regmap_update_bits(dev->regmap[2], PORT_CTRL_ADDR(port, offset),
+	regmap_update_bits(ksz_regmap_32(dev), PORT_CTRL_ADDR(port, offset),
 			   bits, set ? bits : 0);
 }
 
@@ -52,7 +52,7 @@ int ksz9477_change_mtu(struct ksz_device *dev, int port, int mtu)
 
 	frame_size = mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
 
-	return regmap_update_bits(dev->regmap[1], REG_SW_MTU__2,
+	return regmap_update_bits(ksz_regmap_16(dev), REG_SW_MTU__2,
 				  REG_SW_MTU_MASK, frame_size);
 }
 
@@ -60,7 +60,7 @@ static int ksz9477_wait_vlan_ctrl_ready(struct ksz_device *dev)
 {
 	unsigned int val;
 
-	return regmap_read_poll_timeout(dev->regmap[0], REG_SW_VLAN_CTRL,
+	return regmap_read_poll_timeout(ksz_regmap_8(dev), REG_SW_VLAN_CTRL,
 					val, !(val & VLAN_START), 10, 1000);
 }
 
@@ -147,7 +147,7 @@ static int ksz9477_wait_alu_ready(struct ksz_device *dev)
 {
 	unsigned int val;
 
-	return regmap_read_poll_timeout(dev->regmap[2], REG_SW_ALU_CTRL__4,
+	return regmap_read_poll_timeout(ksz_regmap_32(dev), REG_SW_ALU_CTRL__4,
 					val, !(val & ALU_START), 10, 1000);
 }
 
@@ -155,7 +155,7 @@ static int ksz9477_wait_alu_sta_ready(struct ksz_device *dev)
 {
 	unsigned int val;
 
-	return regmap_read_poll_timeout(dev->regmap[2],
+	return regmap_read_poll_timeout(ksz_regmap_32(dev),
 					REG_SW_ALU_STAT_CTRL__4,
 					val, !(val & ALU_STAT_START),
 					10, 1000);
@@ -170,7 +170,7 @@ int ksz9477_reset_switch(struct ksz_device *dev)
 	ksz_cfg(dev, REG_SW_OPERATION, SW_RESET, true);
 
 	/* turn off SPI DO Edge select */
-	regmap_update_bits(dev->regmap[0], REG_SW_GLOBAL_SERIAL_CTRL_0,
+	regmap_update_bits(ksz_regmap_8(dev), REG_SW_GLOBAL_SERIAL_CTRL_0,
 			   SPI_AUTO_EDGE_DETECTION, 0);
 
 	/* default configuration */
@@ -213,7 +213,7 @@ void ksz9477_r_mib_cnt(struct ksz_device *dev, int port, u16 addr, u64 *cnt)
 	data |= (addr << MIB_COUNTER_INDEX_S);
 	ksz_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT__4, data);
 
-	ret = regmap_read_poll_timeout(dev->regmap[2],
+	ret = regmap_read_poll_timeout(ksz_regmap_32(dev),
 			PORT_CTRL_ADDR(port, REG_PORT_MIB_CTRL_STAT__4),
 			val, !(val & MIB_COUNTER_READ), 10, 1000);
 	/* failed to read MIB. get out of loop */
@@ -346,7 +346,7 @@ void ksz9477_flush_dyn_mac_table(struct ksz_device *dev, int port)
 	const u16 *regs = dev->info->regs;
 	u8 data;
 
-	regmap_update_bits(dev->regmap[0], REG_SW_LUE_CTRL_2,
+	regmap_update_bits(ksz_regmap_8(dev), REG_SW_LUE_CTRL_2,
 			   SW_FLUSH_OPTION_M << SW_FLUSH_OPTION_S,
 			   SW_FLUSH_OPTION_DYN_MAC << SW_FLUSH_OPTION_S);
 
@@ -1165,7 +1165,7 @@ int ksz9477_setup(struct dsa_switch *ds)
 	ksz_cfg(dev, REG_SW_MAC_CTRL_1, SW_JUMBO_PACKET, true);
 
 	/* Now we can configure default MTU value */
-	ret = regmap_update_bits(dev->regmap[1], REG_SW_MTU__2, REG_SW_MTU_MASK,
+	ret = regmap_update_bits(ksz_regmap_16(dev), REG_SW_MTU__2, REG_SW_MTU_MASK,
 				 VLAN_ETH_FRAME_LEN + ETH_FCS_LEN);
 	if (ret)
 		return ret;
diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c b/drivers/net/dsa/microchip/ksz9477_i2c.c
index 97a317263a2f..497be833f707 100644
--- a/drivers/net/dsa/microchip/ksz9477_i2c.c
+++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
@@ -24,7 +24,7 @@ static int ksz9477_i2c_probe(struct i2c_client *i2c)
 	if (!dev)
 		return -ENOMEM;
 
-	for (i = 0; i < ARRAY_SIZE(ksz9477_regmap_config); i++) {
+	for (i = 0; i < __KSZ_NUM_REGMAPS; i++) {
 		rc = ksz9477_regmap_config[i];
 		rc.lock_arg = &dev->regmap_mutex;
 		dev->regmap[i] = devm_regmap_init_i2c(i2c, &rc);
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 50fd548c72d8..8617aeaa0248 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2093,7 +2093,7 @@ static int ksz_setup(struct dsa_switch *ds)
 	}
 
 	/* set broadcast storm protection 10% rate */
-	regmap_update_bits(dev->regmap[1], regs[S_BROADCAST_CTRL],
+	regmap_update_bits(ksz_regmap_16(dev), regs[S_BROADCAST_CTRL],
 			   BROADCAST_STORM_RATE,
 			   (BROADCAST_STORM_VALUE *
 			   BROADCAST_STORM_PROT_RATE) / 100);
@@ -2104,7 +2104,7 @@ static int ksz_setup(struct dsa_switch *ds)
 
 	ds->num_tx_queues = dev->info->num_tx_queues;
 
-	regmap_update_bits(dev->regmap[0], regs[S_MULTICAST_CTRL],
+	regmap_update_bits(ksz_regmap_8(dev), regs[S_MULTICAST_CTRL],
 			   MULTICAST_STORM_DISABLE, MULTICAST_STORM_DISABLE);
 
 	ksz_init_mib_timer(dev);
@@ -2154,7 +2154,7 @@ static int ksz_setup(struct dsa_switch *ds)
 	}
 
 	/* start switch */
-	regmap_update_bits(dev->regmap[0], regs[S_START_CTRL],
+	regmap_update_bits(ksz_regmap_8(dev), regs[S_START_CTRL],
 			   SW_START, SW_START);
 
 	return 0;
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 8abecaf6089e..ef1643c357a4 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -22,6 +22,13 @@
 struct ksz_device;
 struct ksz_port;
 
+enum ksz_regmap_width {
+	KSZ_REGMAP_8,
+	KSZ_REGMAP_16,
+	KSZ_REGMAP_32,
+	__KSZ_NUM_REGMAPS,
+};
+
 struct vlan_table {
 	u32 table[3];
 };
@@ -137,7 +144,7 @@ struct ksz_device {
 	const struct ksz_dev_ops *dev_ops;
 
 	struct device *dev;
-	struct regmap *regmap[3];
+	struct regmap *regmap[__KSZ_NUM_REGMAPS];
 
 	void *priv;
 	int irq;
@@ -377,11 +384,25 @@ phy_interface_t ksz_get_xmii(struct ksz_device *dev, int port, bool gbit);
 extern const struct ksz_chip_data ksz_switch_chips[];
 
 /* Common register access functions */
+static inline struct regmap *ksz_regmap_8(struct ksz_device *dev)
+{
+	return dev->regmap[KSZ_REGMAP_8];
+}
+
+static inline struct regmap *ksz_regmap_16(struct ksz_device *dev)
+{
+	return dev->regmap[KSZ_REGMAP_16];
+}
+
+static inline struct regmap *ksz_regmap_32(struct ksz_device *dev)
+{
+	return dev->regmap[KSZ_REGMAP_32];
+}
 
 static inline int ksz_read8(struct ksz_device *dev, u32 reg, u8 *val)
 {
 	unsigned int value;
-	int ret = regmap_read(dev->regmap[0], reg, &value);
+	int ret = regmap_read(ksz_regmap_8(dev), reg, &value);
 
 	if (ret)
 		dev_err(dev->dev, "can't read 8bit reg: 0x%x %pe\n", reg,
@@ -394,7 +415,7 @@ static inline int ksz_read8(struct ksz_device *dev, u32 reg, u8 *val)
 static inline int ksz_read16(struct ksz_device *dev, u32 reg, u16 *val)
 {
 	unsigned int value;
-	int ret = regmap_read(dev->regmap[1], reg, &value);
+	int ret = regmap_read(ksz_regmap_16(dev), reg, &value);
 
 	if (ret)
 		dev_err(dev->dev, "can't read 16bit reg: 0x%x %pe\n", reg,
@@ -407,7 +428,7 @@ static inline int ksz_read16(struct ksz_device *dev, u32 reg, u16 *val)
 static inline int ksz_read32(struct ksz_device *dev, u32 reg, u32 *val)
 {
 	unsigned int value;
-	int ret = regmap_read(dev->regmap[2], reg, &value);
+	int ret = regmap_read(ksz_regmap_32(dev), reg, &value);
 
 	if (ret)
 		dev_err(dev->dev, "can't read 32bit reg: 0x%x %pe\n", reg,
@@ -422,7 +443,7 @@ static inline int ksz_read64(struct ksz_device *dev, u32 reg, u64 *val)
 	u32 value[2];
 	int ret;
 
-	ret = regmap_bulk_read(dev->regmap[2], reg, value, 2);
+	ret = regmap_bulk_read(ksz_regmap_32(dev), reg, value, 2);
 	if (ret)
 		dev_err(dev->dev, "can't read 64bit reg: 0x%x %pe\n", reg,
 			ERR_PTR(ret));
@@ -436,7 +457,7 @@ static inline int ksz_write8(struct ksz_device *dev, u32 reg, u8 value)
 {
 	int ret;
 
-	ret = regmap_write(dev->regmap[0], reg, value);
+	ret = regmap_write(ksz_regmap_8(dev), reg, value);
 	if (ret)
 		dev_err(dev->dev, "can't write 8bit reg: 0x%x %pe\n", reg,
 			ERR_PTR(ret));
@@ -448,7 +469,7 @@ static inline int ksz_write16(struct ksz_device *dev, u32 reg, u16 value)
 {
 	int ret;
 
-	ret = regmap_write(dev->regmap[1], reg, value);
+	ret = regmap_write(ksz_regmap_16(dev), reg, value);
 	if (ret)
 		dev_err(dev->dev, "can't write 16bit reg: 0x%x %pe\n", reg,
 			ERR_PTR(ret));
@@ -460,7 +481,7 @@ static inline int ksz_write32(struct ksz_device *dev, u32 reg, u32 value)
 {
 	int ret;
 
-	ret = regmap_write(dev->regmap[2], reg, value);
+	ret = regmap_write(ksz_regmap_32(dev), reg, value);
 	if (ret)
 		dev_err(dev->dev, "can't write 32bit reg: 0x%x %pe\n", reg,
 			ERR_PTR(ret));
@@ -473,7 +494,7 @@ static inline int ksz_rmw16(struct ksz_device *dev, u32 reg, u16 mask,
 {
 	int ret;
 
-	ret = regmap_update_bits(dev->regmap[1], reg, mask, value);
+	ret = regmap_update_bits(ksz_regmap_16(dev), reg, mask, value);
 	if (ret)
 		dev_err(dev->dev, "can't rmw 16bit reg 0x%x: %pe\n", reg,
 			ERR_PTR(ret));
@@ -486,7 +507,7 @@ static inline int ksz_rmw32(struct ksz_device *dev, u32 reg, u32 mask,
 {
 	int ret;
 
-	ret = regmap_update_bits(dev->regmap[2], reg, mask, value);
+	ret = regmap_update_bits(ksz_regmap_32(dev), reg, mask, value);
 	if (ret)
 		dev_err(dev->dev, "can't rmw 32bit reg 0x%x: %pe\n", reg,
 			ERR_PTR(ret));
@@ -503,12 +524,12 @@ static inline int ksz_write64(struct ksz_device *dev, u32 reg, u64 value)
 	val[0] = swab32(value & 0xffffffffULL);
 	val[1] = swab32(value >> 32ULL);
 
-	return regmap_bulk_write(dev->regmap[2], reg, val, 2);
+	return regmap_bulk_write(ksz_regmap_32(dev), reg, val, 2);
 }
 
 static inline int ksz_rmw8(struct ksz_device *dev, int offset, u8 mask, u8 val)
 {
-	return regmap_update_bits(dev->regmap[0], offset, mask, val);
+	return regmap_update_bits(ksz_regmap_8(dev), offset, mask, val);
 }
 
 static inline int ksz_pread8(struct ksz_device *dev, int port, int offset,
@@ -552,7 +573,7 @@ static inline int ksz_pwrite32(struct ksz_device *dev, int port, int offset,
 static inline void ksz_prmw8(struct ksz_device *dev, int port, int offset,
 			     u8 mask, u8 val)
 {
-	regmap_update_bits(dev->regmap[0],
+	regmap_update_bits(ksz_regmap_8(dev),
 			   dev->dev_ops->get_port_addr(port, offset),
 			   mask, val);
 }
@@ -709,9 +730,9 @@ static inline int is_lan937x(struct ksz_device *dev)
 
 #define KSZ_REGMAP_TABLE(ksz, swp, regbits, regpad, regalign)		\
 	static const struct regmap_config ksz##_regmap_config[] = {	\
-		KSZ_REGMAP_ENTRY(8, swp, (regbits), (regpad), (regalign)), \
-		KSZ_REGMAP_ENTRY(16, swp, (regbits), (regpad), (regalign)), \
-		KSZ_REGMAP_ENTRY(32, swp, (regbits), (regpad), (regalign)), \
+		[KSZ_REGMAP_8] = KSZ_REGMAP_ENTRY(8, swp, (regbits), (regpad), (regalign)), \
+		[KSZ_REGMAP_16] = KSZ_REGMAP_ENTRY(16, swp, (regbits), (regpad), (regalign)), \
+		[KSZ_REGMAP_32] = KSZ_REGMAP_ENTRY(32, swp, (regbits), (regpad), (regalign)), \
 	}
 
 #endif
diff --git a/drivers/net/dsa/microchip/ksz_spi.c b/drivers/net/dsa/microchip/ksz_spi.c
index 96c52e8fb51b..279338451621 100644
--- a/drivers/net/dsa/microchip/ksz_spi.c
+++ b/drivers/net/dsa/microchip/ksz_spi.c
@@ -63,7 +63,7 @@ static int ksz_spi_probe(struct spi_device *spi)
 	else
 		regmap_config = ksz9477_regmap_config;
 
-	for (i = 0; i < ARRAY_SIZE(ksz8795_regmap_config); i++) {
+	for (i = 0; i < __KSZ_NUM_REGMAPS; i++) {
 		rc = regmap_config[i];
 		rc.lock_arg = &dev->regmap_mutex;
 		rc.wr_table = chip->wr_table;
diff --git a/drivers/net/dsa/microchip/lan937x_main.c b/drivers/net/dsa/microchip/lan937x_main.c
index 399a3905e6ca..b479a628b1ae 100644
--- a/drivers/net/dsa/microchip/lan937x_main.c
+++ b/drivers/net/dsa/microchip/lan937x_main.c
@@ -20,13 +20,13 @@
 
 static int lan937x_cfg(struct ksz_device *dev, u32 addr, u8 bits, bool set)
 {
-	return regmap_update_bits(dev->regmap[0], addr, bits, set ? bits : 0);
+	return regmap_update_bits(ksz_regmap_8(dev), addr, bits, set ? bits : 0);
 }
 
 static int lan937x_port_cfg(struct ksz_device *dev, int port, int offset,
 			    u8 bits, bool set)
 {
-	return regmap_update_bits(dev->regmap[0], PORT_CTRL_ADDR(port, offset),
+	return regmap_update_bits(ksz_regmap_8(dev), PORT_CTRL_ADDR(port, offset),
 				  bits, set ? bits : 0);
 }
 
@@ -86,7 +86,7 @@ static int lan937x_internal_phy_write(struct ksz_device *dev, int addr, int reg,
 	if (ret < 0)
 		return ret;
 
-	ret = regmap_read_poll_timeout(dev->regmap[1], REG_VPHY_IND_CTRL__2,
+	ret = regmap_read_poll_timeout(ksz_regmap_16(dev), REG_VPHY_IND_CTRL__2,
 				       value, !(value & VPHY_IND_BUSY), 10,
 				       1000);
 	if (ret < 0) {
@@ -116,7 +116,7 @@ static int lan937x_internal_phy_read(struct ksz_device *dev, int addr, int reg,
 	if (ret < 0)
 		return ret;
 
-	ret = regmap_read_poll_timeout(dev->regmap[1], REG_VPHY_IND_CTRL__2,
+	ret = regmap_read_poll_timeout(ksz_regmap_16(dev), REG_VPHY_IND_CTRL__2,
 				       value, !(value & VPHY_IND_BUSY), 10,
 				       1000);
 	if (ret < 0) {
-- 
2.34.1

[RFC/RFT PATCH net-next 2/4] net: dsa: microchip: partial conversion to regfields API for KSZ8795 (WIP)

[Vladimir Oltean]

During review, Russell King has pointed out that the current way in
which the KSZ common driver performs register access is error-prone.

Based on the KSZ9477 software model where we have the XMII Port Control
0 Register (at offset 0xN300) and XMII Port Control 1 Register
(at offset 0xN301), this driver also holds P_XMII_CTRL_0 and P_XMII_CTRL_1.

However, on some switches, fields accessed through P_XMII_CTRL_0 (for
example P_MII_100MBIT_M or P_MII_DUPLEX_M) and fields accessed through
P_XMII_CTRL_1 (for example P_MII_SEL_M) may end up accessing the same
hardware register. Or at least, that was certainly the impression after
commit
https://patchwork.kernel.org/project/netdevbpf/patch/20230315231916.2998480-1-vladimir.oltean@nxp.com/
(sorry, can't reference the sha1sum of an unmerged commit), because for
ksz8795_regs[], P_XMII_CTRL_0 and P_XMII_CTRL_1 now have the same value.

But the reality is far more interesting. Opening a KSZ8795 datasheet, I
see that out of the register fields accessed via P_XMII_CTRL_0:
- what the driver names P_MII_SEL_M *is* actually "GMII/MII Mode Select"
  (bit 2) of the Port 5 Interface Control 6, address 0x56 (all good here)
- what the driver names P_MII_100MBIT_M is actually "Switch SW5-MII/RMII
  Speed" (bit 4) of the Global Control 4 register, address 0x06.

That is a huge problem, because the driver cannot access this register
for KSZ8795 in its current form, even if that register exists. This
creates an even stronger motivation to try to do something to normalize
the way in which this driver abstracts away register field movement from
one switch family to another.

As I had proposed in that thread, reg_fields from regmap propose to
solve exactly this problem. This patch contains a COMPLETELY UNTESTED
rework of the driver, so that accesses done through the following
registers (for demonstration purposes):
- REG_IND_BYTE - a global register
- REG_IND_CTRL_0 - another global register
- P_LOCAL_CTRL - a port register
- P_FORCE_CTRL - another port register
- P_XMII_CTRL_0 and P_XMII_CTRL_1 - either port register, or global
  registers, depending on which manual you read!

are converted to the regfields API.

!! WARNING !! I only attempted to add a ksz_reg_fields structure for
KSZ8795. The other switch families will currently crash!

For easier partial migration, I have renamed the "REG_" or "P_" prefixes
of the enum ksz_regs values into a common "RF_" (for reg field) prefix
for a new enum type: ksz_rf. Eventually, enum ksz_regs, as well as the
masks, should disappear completely, being replaced by reg fields.

Link: https://lore.kernel.org/netdev/Y%2FYPfxg8Ackb8zmW@shell.armlinux.org.uk/
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/microchip/ksz8795.c     |  37 ++---
 drivers/net/dsa/microchip/ksz8863_smi.c |   9 +
 drivers/net/dsa/microchip/ksz9477_i2c.c |   9 +
 drivers/net/dsa/microchip/ksz_common.c  | 209 ++++++++++++++++--------
 drivers/net/dsa/microchip/ksz_common.h  |  49 ++++++
 drivers/net/dsa/microchip/ksz_spi.c     |   4 +
 6 files changed, 227 insertions(+), 90 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 7f9ab6d13952..1abdc45278ad 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -40,18 +40,15 @@ static void ksz_port_cfg(struct ksz_device *dev, int port, int offset, u8 bits,
 
 static int ksz8_ind_write8(struct ksz_device *dev, u8 table, u16 addr, u8 data)
 {
-	const u16 *regs;
 	u16 ctrl_addr;
 	int ret = 0;
 
-	regs = dev->info->regs;
-
 	mutex_lock(&dev->alu_mutex);
 
 	ctrl_addr = IND_ACC_TABLE(table) | addr;
-	ret = ksz_write8(dev, regs[REG_IND_BYTE], data);
+	ret = ksz_regfield_write(dev, RF_IND_BYTE, data);
 	if (!ret)
-		ret = ksz_write16(dev, regs[REG_IND_CTRL_0], ctrl_addr);
+		ret = ksz_regfield_write(dev, RF_IND_CTRL_0, ctrl_addr);
 
 	mutex_unlock(&dev->alu_mutex);
 
@@ -176,7 +173,7 @@ void ksz8_r_mib_cnt(struct ksz_device *dev, int port, u16 addr, u64 *cnt)
 	ctrl_addr |= IND_ACC_TABLE(TABLE_MIB | TABLE_READ);
 
 	mutex_lock(&dev->alu_mutex);
-	ksz_write16(dev, regs[REG_IND_CTRL_0], ctrl_addr);
+	ksz_regfield_write(dev, RF_IND_CTRL_0, ctrl_addr);
 
 	/* It is almost guaranteed to always read the valid bit because of
 	 * slow SPI speed.
@@ -214,7 +211,7 @@ static void ksz8795_r_mib_pkt(struct ksz_device *dev, int port, u16 addr,
 	ctrl_addr |= IND_ACC_TABLE(TABLE_MIB | TABLE_READ);
 
 	mutex_lock(&dev->alu_mutex);
-	ksz_write16(dev, regs[REG_IND_CTRL_0], ctrl_addr);
+	ksz_regfield_write(dev, RF_IND_CTRL_0, ctrl_addr);
 
 	/* It is almost guaranteed to always read the valid bit because of
 	 * slow SPI speed.
@@ -265,7 +262,7 @@ static void ksz8863_r_mib_pkt(struct ksz_device *dev, int port, u16 addr,
 	ctrl_addr |= IND_ACC_TABLE(TABLE_MIB | TABLE_READ);
 
 	mutex_lock(&dev->alu_mutex);
-	ksz_write16(dev, regs[REG_IND_CTRL_0], ctrl_addr);
+	ksz_regfield_write(dev, RF_IND_CTRL_0, ctrl_addr);
 	ksz_read32(dev, regs[REG_IND_DATA_LO], &data);
 	mutex_unlock(&dev->alu_mutex);
 
@@ -346,7 +343,7 @@ static void ksz8_r_table(struct ksz_device *dev, int table, u16 addr, u64 *data)
 	ctrl_addr = IND_ACC_TABLE(table | TABLE_READ) | addr;
 
 	mutex_lock(&dev->alu_mutex);
-	ksz_write16(dev, regs[REG_IND_CTRL_0], ctrl_addr);
+	ksz_regfield_write(dev, RF_IND_CTRL_0, ctrl_addr);
 	ksz_read64(dev, regs[REG_IND_DATA_HI], data);
 	mutex_unlock(&dev->alu_mutex);
 }
@@ -362,7 +359,7 @@ static void ksz8_w_table(struct ksz_device *dev, int table, u16 addr, u64 data)
 
 	mutex_lock(&dev->alu_mutex);
 	ksz_write64(dev, regs[REG_IND_DATA_HI], data);
-	ksz_write16(dev, regs[REG_IND_CTRL_0], ctrl_addr);
+	ksz_regfield_write(dev, RF_IND_CTRL_0, ctrl_addr);
 	mutex_unlock(&dev->alu_mutex);
 }
 
@@ -412,7 +409,7 @@ int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
 	ctrl_addr = IND_ACC_TABLE(TABLE_DYNAMIC_MAC | TABLE_READ) | addr;
 
 	mutex_lock(&dev->alu_mutex);
-	ksz_write16(dev, regs[REG_IND_CTRL_0], ctrl_addr);
+	ksz_regfield_write(dev, RF_IND_CTRL_0, ctrl_addr);
 
 	rc = ksz8_valid_dyn_entry(dev, &data);
 	if (rc == -EAGAIN) {
@@ -605,8 +602,9 @@ static void ksz8_w_vlan_table(struct ksz_device *dev, u16 vid, u16 vlan)
 
 int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
 {
-	u8 restart, speed, ctrl, link;
+	u8 restart, speed, link;
 	int processed = true;
+	unsigned int ctrl;
 	const u16 *regs;
 	u8 val1, val2;
 	u16 data = 0;
@@ -625,7 +623,7 @@ int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
 		if (ret)
 			return ret;
 
-		ret = ksz_pread8(dev, p, regs[P_FORCE_CTRL], &ctrl);
+		ret = ksz_regfields_read(dev, p, RF_FORCE_CTRL, &ctrl);
 		if (ret)
 			return ret;
 
@@ -682,7 +680,7 @@ int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
 			data = KSZ8795_ID_LO;
 		break;
 	case MII_ADVERTISE:
-		ret = ksz_pread8(dev, p, regs[P_LOCAL_CTRL], &ctrl);
+		ret = ksz_regfields_read(dev, p, RF_LOCAL_CTRL, &ctrl);
 		if (ret)
 			return ret;
 
@@ -759,7 +757,8 @@ int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
 
 int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
 {
-	u8 restart, speed, ctrl, data;
+	u8 restart, speed, data;
+	unsigned int ctrl;
 	const u16 *regs;
 	u8 p = phy;
 	int ret;
@@ -788,7 +787,7 @@ int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
 				return ret;
 		}
 
-		ret = ksz_pread8(dev, p, regs[P_FORCE_CTRL], &ctrl);
+		ret = ksz_regfields_read(dev, p, RF_FORCE_CTRL, &ctrl);
 		if (ret)
 			return ret;
 
@@ -819,7 +818,7 @@ int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
 			data &= ~PORT_FORCE_FULL_DUPLEX;
 
 		if (data != ctrl) {
-			ret = ksz_pwrite8(dev, p, regs[P_FORCE_CTRL], data);
+			ret = ksz_regfields_write(dev, p, RF_FORCE_CTRL, data);
 			if (ret)
 				return ret;
 		}
@@ -866,7 +865,7 @@ int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
 		}
 		break;
 	case MII_ADVERTISE:
-		ret = ksz_pread8(dev, p, regs[P_LOCAL_CTRL], &ctrl);
+		ret = ksz_regfields_read(dev, p, RF_LOCAL_CTRL, &ctrl);
 		if (ret)
 			return ret;
 
@@ -888,7 +887,7 @@ int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
 			data |= PORT_AUTO_NEG_10BT;
 
 		if (data != ctrl) {
-			ret = ksz_pwrite8(dev, p, regs[P_LOCAL_CTRL], data);
+			ret = ksz_regfields_write(dev, p, RF_LOCAL_CTRL, data);
 			if (ret)
 				return ret;
 		}
diff --git a/drivers/net/dsa/microchip/ksz8863_smi.c b/drivers/net/dsa/microchip/ksz8863_smi.c
index 5871f27451cb..f9d22a444146 100644
--- a/drivers/net/dsa/microchip/ksz8863_smi.c
+++ b/drivers/net/dsa/microchip/ksz8863_smi.c
@@ -136,11 +136,16 @@ static const struct regmap_config ksz8863_regmap_config[] = {
 
 static int ksz8863_smi_probe(struct mdio_device *mdiodev)
 {
+	const struct ksz_chip_data *chip;
 	struct regmap_config rc;
 	struct ksz_device *dev;
 	int ret;
 	int i;
 
+	chip = device_get_match_data(ddev);
+	if (!chip)
+		return -EINVAL;
+
 	dev = ksz_switch_alloc(&mdiodev->dev, mdiodev);
 	if (!dev)
 		return -ENOMEM;
@@ -159,6 +164,10 @@ static int ksz8863_smi_probe(struct mdio_device *mdiodev)
 		}
 	}
 
+	ret = ksz_regfields_init(dev, chip);
+	if (ret)
+		return ret;
+
 	if (mdiodev->dev.platform_data)
 		dev->pdata = mdiodev->dev.platform_data;
 
diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c b/drivers/net/dsa/microchip/ksz9477_i2c.c
index 497be833f707..2cbd76aed974 100644
--- a/drivers/net/dsa/microchip/ksz9477_i2c.c
+++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
@@ -16,10 +16,15 @@ KSZ_REGMAP_TABLE(ksz9477, not_used, 16, 0, 0);
 
 static int ksz9477_i2c_probe(struct i2c_client *i2c)
 {
+	const struct ksz_chip_data *chip;
 	struct regmap_config rc;
 	struct ksz_device *dev;
 	int i, ret;
 
+	chip = device_get_match_data(ddev);
+	if (!chip)
+		return -EINVAL;
+
 	dev = ksz_switch_alloc(&i2c->dev, i2c);
 	if (!dev)
 		return -ENOMEM;
@@ -35,6 +40,10 @@ static int ksz9477_i2c_probe(struct i2c_client *i2c)
 		}
 	}
 
+	ret = ksz_regfields_init(dev, chip);
+	if (ret)
+		return ret;
+
 	if (i2c->dev.platform_data)
 		dev->pdata = i2c->dev.platform_data;
 
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 8617aeaa0248..5bcbea8d9151 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -295,6 +295,65 @@ static const struct ksz_dev_ops lan937x_dev_ops = {
 	.exit = lan937x_switch_exit,
 };
 
+#define KSZ8_PORT_BASE		0x10
+#define KSZ8_PORT_SPACING	0x10
+#define KSZ9477_PORT_BASE	0x1000
+#define KSZ9477_PORT_SPACING	0x1000
+
+#define KSZ_REG_FIELD_8(_reg, _lsb, _msb) \
+	{ .width = KSZ_REGMAP_8, .regfield = REG_FIELD(_reg, _lsb, _msb) }
+#define KSZ_REG_FIELD_16(_reg, _lsb, _msb) \
+	{ .width = KSZ_REGMAP_16, .regfield = REG_FIELD(_reg, _lsb, _msb) }
+#define KSZ_REG_FIELD_32(_reg, _lsb, _msb) \
+	{ .width = KSZ_REGMAP_32, .regfield = REG_FIELD(_reg, _lsb, _msb) }
+#define KSZ8_PORT_REG_FIELD_8(_reg, _lsb, _msb) \
+	{ \
+		.width = KSZ_REGMAP_8, \
+		.regfield = REG_FIELD_ID(KSZ8_PORT_BASE + (_reg), \
+					 _lsb, _msb, KSZ_MAX_NUM_PORTS, \
+					 KSZ8_PORT_SPACING) \
+	}
+#define KSZ8_PORT_REG_FIELD_16(_reg, _lsb, _msb) \
+	{ \
+		.width = KSZ_REGMAP_16, \
+		.regfield = REG_FIELD_ID(KSZ8_PORT_BASE + (_reg), \
+					 _lsb, _msb, KSZ_MAX_NUM_PORTS, \
+					 KSZ8_PORT_SPACING) \
+	}
+#define KSZ8_PORT_REG_FIELD_32(_reg, _lsb, _msb) \
+	{ \
+		.width = KSZ_REGMAP_32, \
+		.regfield = REG_FIELD_ID(KSZ8_PORT_BASE + (_reg), \
+					 _lsb, _msb, KSZ_MAX_NUM_PORTS, \
+					 KSZ8_PORT_SPACING) \
+	}
+/* Some registers are wacky, in the sense that they should be per port (and are
+ * accessed using per-port regfields by the driver), but on some hardware, they
+ * are defined in the global address space, so they should be accessed via the
+ * global regfield API. We hack these up by using a REG_FIELD_ID() definition
+ * with a spacing of 0, so that accesses to any port access the same (global)
+ * register.
+ */
+#define KSZ_WACKY_REG_FIELD_8(_reg, _lsb, _msb) \
+	{ \
+		.width = KSZ_REGMAP_8, \
+		.regfield = REG_FIELD_ID(_reg, _lsb, _msb, KSZ_MAX_NUM_PORTS, 0) \
+	}
+
+static const struct ksz_reg_field ksz8795_regfields[__KSZ_NUM_REGFIELDS] = {
+	[RF_IND_CTRL_0] = KSZ_REG_FIELD_16(0x6E, 0, 15),
+	[RF_IND_BYTE] = KSZ_REG_FIELD_8(0xA0, 0, 7),
+	[RF_FORCE_CTRL] = KSZ8_PORT_REG_FIELD_8(0x0C, 0, 7),
+	[RF_LOCAL_CTRL] = KSZ8_PORT_REG_FIELD_8(0x07, 0, 7),
+	[RF_GMII_1GBIT] = KSZ8_PORT_REG_FIELD_8(0x06, 6, 6),
+	[RF_MII_SEL] = KSZ8_PORT_REG_FIELD_8(0x06, 2, 2),
+	[RF_RGMII_ID_IG_ENABLE] = KSZ8_PORT_REG_FIELD_8(0x06, 4, 4),
+	[RF_RGMII_ID_EG_ENABLE] = KSZ8_PORT_REG_FIELD_8(0x06, 3, 3),
+	[RF_MII_DUPLEX] = KSZ_WACKY_REG_FIELD_8(0x06, 6, 6), // Global Control 4
+	[RF_MII_TX_FLOW_CTRL] = KSZ_WACKY_REG_FIELD_8(0x06, 5, 5), // Global Control 4
+	[RF_MII_100MBIT] = KSZ_WACKY_REG_FIELD_8(0x06, 4, 4), // Global Control 4
+};
+
 static const u16 ksz8795_regs[] = {
 	[REG_IND_CTRL_0]		= 0x6E,
 	[REG_IND_DATA_8]		= 0x70,
@@ -1121,6 +1180,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 		.regs = ksz8795_regs,
 		.masks = ksz8795_masks,
 		.shifts = ksz8795_shifts,
+		.regfields = ksz8795_regfields,
 		.xmii_ctrl0 = ksz8795_xmii_ctrl0,
 		.xmii_ctrl1 = ksz8795_xmii_ctrl1,
 		.supports_mii = {false, false, false, false, true},
@@ -2747,34 +2807,28 @@ static void ksz_set_xmii(struct ksz_device *dev, int port,
 {
 	const u8 *bitval = dev->info->xmii_ctrl1;
 	struct ksz_port *p = &dev->ports[port];
-	const u16 *regs = dev->info->regs;
-	u8 data8;
-
-	ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8);
-
-	data8 &= ~(P_MII_SEL_M | P_RGMII_ID_IG_ENABLE |
-		   P_RGMII_ID_EG_ENABLE);
+	unsigned int mii_sel;
 
 	switch (interface) {
 	case PHY_INTERFACE_MODE_MII:
-		data8 |= bitval[P_MII_SEL];
+		mii_sel = bitval[P_MII_SEL];
 		break;
 	case PHY_INTERFACE_MODE_RMII:
-		data8 |= bitval[P_RMII_SEL];
+		mii_sel = bitval[P_RMII_SEL];
 		break;
 	case PHY_INTERFACE_MODE_GMII:
-		data8 |= bitval[P_GMII_SEL];
+		mii_sel = bitval[P_GMII_SEL];
 		break;
 	case PHY_INTERFACE_MODE_RGMII:
 	case PHY_INTERFACE_MODE_RGMII_ID:
 	case PHY_INTERFACE_MODE_RGMII_TXID:
 	case PHY_INTERFACE_MODE_RGMII_RXID:
-		data8 |= bitval[P_RGMII_SEL];
+		mii_sel = bitval[P_RGMII_SEL];
 		/* On KSZ9893, disable RGMII in-band status support */
 		if (dev->chip_id == KSZ9893_CHIP_ID ||
 		    dev->chip_id == KSZ8563_CHIP_ID ||
 		    dev->chip_id == KSZ9563_CHIP_ID)
-			data8 &= ~P_MII_MAC_MODE;
+			ksz_regfields_write(dev, port, RF_MII_MAC_MODE, 0);
 		break;
 	default:
 		dev_err(dev->dev, "Unsupported interface '%s' for port %d\n",
@@ -2782,42 +2836,46 @@ static void ksz_set_xmii(struct ksz_device *dev, int port,
 		return;
 	}
 
-	if (p->rgmii_tx_val)
-		data8 |= P_RGMII_ID_EG_ENABLE;
-
-	if (p->rgmii_rx_val)
-		data8 |= P_RGMII_ID_IG_ENABLE;
-
-	/* Write the updated value */
-	ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8);
+	ksz_regfields_write(dev, port, RF_MII_SEL, mii_sel);
+	ksz_regfields_write(dev, port, RF_RGMII_ID_EG_ENABLE,
+			    !!p->rgmii_tx_val);
+	ksz_regfields_write(dev, port, RF_RGMII_ID_IG_ENABLE,
+			    !!p->rgmii_rx_val);
 }
 
 phy_interface_t ksz_get_xmii(struct ksz_device *dev, int port, bool gbit)
 {
 	const u8 *bitval = dev->info->xmii_ctrl1;
-	const u16 *regs = dev->info->regs;
+	unsigned int mii_sel, ig, eg;
 	phy_interface_t interface;
-	u8 data8;
-	u8 val;
-
-	ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8);
+	int ret;
 
-	val = FIELD_GET(P_MII_SEL_M, data8);
+	ret = ksz_regfields_read(dev, port, RF_MII_SEL, &mii_sel);
+	if (WARN_ON(ret))
+		return PHY_INTERFACE_MODE_NA;
 
-	if (val == bitval[P_MII_SEL]) {
+	if (mii_sel == bitval[P_MII_SEL]) {
 		if (gbit)
 			interface = PHY_INTERFACE_MODE_GMII;
 		else
 			interface = PHY_INTERFACE_MODE_MII;
-	} else if (val == bitval[P_RMII_SEL]) {
+	} else if (mii_sel == bitval[P_RMII_SEL]) {
 		interface = PHY_INTERFACE_MODE_RGMII;
 	} else {
+		ret = ksz_regfields_read(dev, port, RF_RGMII_ID_IG_ENABLE, &ig);
+		if (WARN_ON(ret))
+			return PHY_INTERFACE_MODE_NA;
+
+		ret = ksz_regfields_read(dev, port, RF_RGMII_ID_IG_ENABLE, &eg);
+		if (WARN_ON(ret))
+			return PHY_INTERFACE_MODE_NA;
+
 		interface = PHY_INTERFACE_MODE_RGMII;
-		if (data8 & P_RGMII_ID_EG_ENABLE)
+		if (eg)
 			interface = PHY_INTERFACE_MODE_RGMII_TXID;
-		if (data8 & P_RGMII_ID_IG_ENABLE) {
+		if (ig) {
 			interface = PHY_INTERFACE_MODE_RGMII_RXID;
-			if (data8 & P_RGMII_ID_EG_ENABLE)
+			if (eg)
 				interface = PHY_INTERFACE_MODE_RGMII_ID;
 		}
 	}
@@ -2855,14 +2913,13 @@ static void ksz_phylink_mac_config(struct dsa_switch *ds, int port,
 bool ksz_get_gbit(struct ksz_device *dev, int port)
 {
 	const u8 *bitval = dev->info->xmii_ctrl1;
-	const u16 *regs = dev->info->regs;
 	bool gbit = false;
-	u8 data8;
-	bool val;
-
-	ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8);
+	unsigned int val;
+	int ret;
 
-	val = FIELD_GET(P_GMII_1GBIT_M, data8);
+	ret = ksz_regfields_read(dev, port, RF_GMII_1GBIT, &val);
+	if (WARN_ON(ret))
+		return false;
 
 	if (val == bitval[P_GMII_1GBIT])
 		gbit = true;
@@ -2873,39 +2930,27 @@ bool ksz_get_gbit(struct ksz_device *dev, int port)
 static void ksz_set_gbit(struct ksz_device *dev, int port, bool gbit)
 {
 	const u8 *bitval = dev->info->xmii_ctrl1;
-	const u16 *regs = dev->info->regs;
-	u8 data8;
-
-	ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8);
-
-	data8 &= ~P_GMII_1GBIT_M;
+	unsigned int val;
 
 	if (gbit)
-		data8 |= FIELD_PREP(P_GMII_1GBIT_M, bitval[P_GMII_1GBIT]);
+		val = bitval[P_GMII_1GBIT];
 	else
-		data8 |= FIELD_PREP(P_GMII_1GBIT_M, bitval[P_GMII_NOT_1GBIT]);
+		val = bitval[P_GMII_NOT_1GBIT];
 
-	/* Write the updated value */
-	ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8);
+	ksz_regfields_write(dev, port, RF_GMII_1GBIT, val);
 }
 
 static void ksz_set_100_10mbit(struct ksz_device *dev, int port, int speed)
 {
 	const u8 *bitval = dev->info->xmii_ctrl0;
-	const u16 *regs = dev->info->regs;
-	u8 data8;
-
-	ksz_pread8(dev, port, regs[P_XMII_CTRL_0], &data8);
-
-	data8 &= ~P_MII_100MBIT_M;
+	unsigned int val;
 
 	if (speed == SPEED_100)
-		data8 |= FIELD_PREP(P_MII_100MBIT_M, bitval[P_MII_100MBIT]);
+		val = bitval[P_MII_100MBIT];
 	else
-		data8 |= FIELD_PREP(P_MII_100MBIT_M, bitval[P_MII_10MBIT]);
+		val = bitval[P_MII_10MBIT];
 
-	/* Write the updated value */
-	ksz_pwrite8(dev, port, regs[P_XMII_CTRL_0], data8);
+	ksz_regfields_write(dev, port, RF_MII_100MBIT, val);
 }
 
 static void ksz_port_set_xmii_speed(struct ksz_device *dev, int port, int speed)
@@ -2923,26 +2968,19 @@ static void ksz_duplex_flowctrl(struct ksz_device *dev, int port, int duplex,
 				bool tx_pause, bool rx_pause)
 {
 	const u8 *bitval = dev->info->xmii_ctrl0;
-	const u32 *masks = dev->info->masks;
-	const u16 *regs = dev->info->regs;
-	u8 mask;
-	u8 val;
-
-	mask = P_MII_DUPLEX_M | masks[P_MII_TX_FLOW_CTRL] |
-	       masks[P_MII_RX_FLOW_CTRL];
+	unsigned int val;
 
 	if (duplex == DUPLEX_FULL)
-		val = FIELD_PREP(P_MII_DUPLEX_M, bitval[P_MII_FULL_DUPLEX]);
+		val = bitval[P_MII_FULL_DUPLEX];
 	else
-		val = FIELD_PREP(P_MII_DUPLEX_M, bitval[P_MII_HALF_DUPLEX]);
+		val = bitval[P_MII_HALF_DUPLEX];
 
-	if (tx_pause)
-		val |= masks[P_MII_TX_FLOW_CTRL];
+	ksz_regfields_write(dev, port, RF_MII_DUPLEX, val);
 
-	if (rx_pause)
-		val |= masks[P_MII_RX_FLOW_CTRL];
+	ksz_regfields_write(dev, port, RF_MII_TX_FLOW_CTRL, !!tx_pause);
 
-	ksz_prmw8(dev, port, regs[P_XMII_CTRL_0], mask, val);
+	if (dev->regfields[RF_MII_RX_FLOW_CTRL])
+		ksz_regfields_write(dev, port, RF_MII_RX_FLOW_CTRL, !!rx_pause);
 }
 
 static void ksz9477_phylink_mac_link_up(struct ksz_device *dev, int port,
@@ -3420,6 +3458,35 @@ static const struct dsa_switch_ops ksz_switch_ops = {
 	.set_mac_eee		= ksz_set_mac_eee,
 };
 
+int ksz_regfields_init(struct ksz_device *dev, const struct ksz_chip_data *chip)
+{
+	const struct ksz_reg_field *regfields = chip->regfields;
+	struct regmap_field *rf;
+	struct regmap *regmap;
+	enum ksz_rf i;
+
+	dev->regfields = devm_kcalloc(dev->dev, __KSZ_NUM_REGFIELDS,
+				      sizeof(*dev->regfields), GFP_KERNEL);
+	if (!dev->regfields)
+		return -ENOMEM;
+
+	for (i = 0; i < __KSZ_NUM_REGFIELDS; i++) {
+		if (!regfields[i].regfield.reg)
+			continue;
+
+		regmap = dev->regmap[regfields[i].width];
+
+		rf = devm_regmap_field_alloc(dev->dev, regmap,
+					     regfields[i].regfield);
+		if (IS_ERR(rf))
+			return PTR_ERR(rf);
+
+		dev->regfields[i] = rf;
+	}
+
+	return 0;
+}
+
 struct ksz_device *ksz_switch_alloc(struct device *base, void *priv)
 {
 	struct dsa_switch *ds;
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index ef1643c357a4..a92ebf5417b4 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -47,6 +47,11 @@ struct ksz_mib_names {
 	char string[ETH_GSTRING_LEN];
 };
 
+struct ksz_reg_field {
+	enum ksz_regmap_width width;
+	struct reg_field regfield;
+};
+
 struct ksz_chip_data {
 	u32 chip_id;
 	const char *dev_name;
@@ -68,6 +73,7 @@ struct ksz_chip_data {
 	const u16 *regs;
 	const u32 *masks;
 	const u8 *shifts;
+	const struct ksz_reg_field *regfields;
 	const u8 *xmii_ctrl0;
 	const u8 *xmii_ctrl1;
 	int stp_ctrl_reg;
@@ -145,6 +151,7 @@ struct ksz_device {
 
 	struct device *dev;
 	struct regmap *regmap[__KSZ_NUM_REGMAPS];
+	struct regmap_field **regfields;
 
 	void *priv;
 	int irq;
@@ -235,6 +242,23 @@ enum ksz_regs {
 	P_XMII_CTRL_1,
 };
 
+enum ksz_rf {
+	RF_IND_CTRL_0,
+	RF_IND_BYTE,
+	RF_FORCE_CTRL,
+	RF_LOCAL_CTRL,
+	RF_GMII_1GBIT,
+	RF_MII_100MBIT,
+	RF_MII_SEL,
+	RF_MII_DUPLEX,
+	RF_MII_TX_FLOW_CTRL,
+	RF_MII_RX_FLOW_CTRL,
+	RF_RGMII_ID_IG_ENABLE,
+	RF_RGMII_ID_EG_ENABLE,
+	RF_MII_MAC_MODE,
+	__KSZ_NUM_REGFIELDS,
+};
+
 enum ksz_masks {
 	PORT_802_1P_REMAPPING,
 	SW_TAIL_TAG_ENABLE,
@@ -371,6 +395,7 @@ struct ksz_dev_ops {
 	void (*exit)(struct ksz_device *dev);
 };
 
+int ksz_regfields_init(struct ksz_device *dev, const struct ksz_chip_data *chip);
 struct ksz_device *ksz_switch_alloc(struct device *base, void *priv);
 int ksz_switch_register(struct ksz_device *dev);
 void ksz_switch_remove(struct ksz_device *dev);
@@ -578,6 +603,30 @@ static inline void ksz_prmw8(struct ksz_device *dev, int port, int offset,
 			   mask, val);
 }
 
+static inline int ksz_regfield_read(struct ksz_device *dev, enum ksz_rf rf,
+				    unsigned int *val)
+{
+	return regmap_field_read(dev->regfields[rf], val);
+}
+
+static inline int ksz_regfield_write(struct ksz_device *dev, enum ksz_rf rf,
+				     unsigned int val)
+{
+	return regmap_field_write(dev->regfields[rf], val);
+}
+
+static inline int ksz_regfields_read(struct ksz_device *dev, enum ksz_rf rf,
+				     unsigned int port, unsigned int *val)
+{
+	return regmap_fields_read(dev->regfields[rf], port, val);
+}
+
+static inline int ksz_regfields_write(struct ksz_device *dev, enum ksz_rf rf,
+				      unsigned int port, unsigned int val)
+{
+	return regmap_fields_write(dev->regfields[rf], port, val);
+}
+
 static inline void ksz_regmap_lock(void *__mtx)
 {
 	struct mutex *mtx = __mtx;
diff --git a/drivers/net/dsa/microchip/ksz_spi.c b/drivers/net/dsa/microchip/ksz_spi.c
index 279338451621..a64d2c71ef68 100644
--- a/drivers/net/dsa/microchip/ksz_spi.c
+++ b/drivers/net/dsa/microchip/ksz_spi.c
@@ -77,6 +77,10 @@ static int ksz_spi_probe(struct spi_device *spi)
 		}
 	}
 
+	ret = ksz_regfields_init(dev, chip);
+	if (ret)
+		return ret;
+
 	if (spi->dev.platform_data)
 		dev->pdata = spi->dev.platform_data;
 
-- 
2.34.1

Re: [RFC/RFT PATCH net-next 2/4] net: dsa: microchip: partial conversion to regfields API for KSZ8795 (WIP)

[Vladimir Oltean]

On Thu, Mar 16, 2023 at 06:12:48PM +0200, Vladimir Oltean wrote:
> During review, Russell King has pointed out that the current way in
> which the KSZ common driver performs register access is error-prone.
> 
> Based on the KSZ9477 software model where we have the XMII Port Control
> 0 Register (at offset 0xN300) and XMII Port Control 1 Register
> (at offset 0xN301), this driver also holds P_XMII_CTRL_0 and P_XMII_CTRL_1.
> 
> However, on some switches, fields accessed through P_XMII_CTRL_0 (for
> example P_MII_100MBIT_M or P_MII_DUPLEX_M) and fields accessed through
> P_XMII_CTRL_1 (for example P_MII_SEL_M) may end up accessing the same
> hardware register. Or at least, that was certainly the impression after
> commit
> https://patchwork.kernel.org/project/netdevbpf/patch/20230315231916.2998480-1-vladimir.oltean@nxp.com/
> (sorry, can't reference the sha1sum of an unmerged commit), because for
> ksz8795_regs[], P_XMII_CTRL_0 and P_XMII_CTRL_1 now have the same value.
> 
> But the reality is far more interesting. Opening a KSZ8795 datasheet, I
> see that out of the register fields accessed via P_XMII_CTRL_0:
> - what the driver names P_MII_SEL_M *is* actually "GMII/MII Mode Select"
>   (bit 2) of the Port 5 Interface Control 6, address 0x56 (all good here)
> - what the driver names P_MII_100MBIT_M is actually "Switch SW5-MII/RMII
>   Speed" (bit 4) of the Global Control 4 register, address 0x06.
> 
> That is a huge problem, because the driver cannot access this register
> for KSZ8795 in its current form, even if that register exists. This
> creates an even stronger motivation to try to do something to normalize
> the way in which this driver abstracts away register field movement from
> one switch family to another.
> 
> As I had proposed in that thread, reg_fields from regmap propose to
> solve exactly this problem. This patch contains a COMPLETELY UNTESTED
> rework of the driver, so that accesses done through the following
> registers (for demonstration purposes):
> - REG_IND_BYTE - a global register
> - REG_IND_CTRL_0 - another global register
> - P_LOCAL_CTRL - a port register
> - P_FORCE_CTRL - another port register
> - P_XMII_CTRL_0 and P_XMII_CTRL_1 - either port register, or global
>   registers, depending on which manual you read!
> 
> are converted to the regfields API.
> 
> !! WARNING !! I only attempted to add a ksz_reg_fields structure for
> KSZ8795. The other switch families will currently crash!
> 
> For easier partial migration, I have renamed the "REG_" or "P_" prefixes
> of the enum ksz_regs values into a common "RF_" (for reg field) prefix
> for a new enum type: ksz_rf. Eventually, enum ksz_regs, as well as the
> masks, should disappear completely, being replaced by reg fields.
> 
> Link: https://lore.kernel.org/netdev/Y%2FYPfxg8Ackb8zmW@shell.armlinux.org.uk/
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> diff --git a/drivers/net/dsa/microchip/ksz8863_smi.c b/drivers/net/dsa/microchip/ksz8863_smi.c
> index 5871f27451cb..f9d22a444146 100644
> --- a/drivers/net/dsa/microchip/ksz8863_smi.c
> +++ b/drivers/net/dsa/microchip/ksz8863_smi.c
> @@ -136,11 +136,16 @@ static const struct regmap_config ksz8863_regmap_config[] = {
>  
>  static int ksz8863_smi_probe(struct mdio_device *mdiodev)
>  {
> +	const struct ksz_chip_data *chip;
>  	struct regmap_config rc;
>  	struct ksz_device *dev;
>  	int ret;
>  	int i;
>  
> +	chip = device_get_match_data(ddev);

s/ddev/&mdiodev->dev/

> +	if (!chip)
> +		return -EINVAL;
> +
>  	dev = ksz_switch_alloc(&mdiodev->dev, mdiodev);
>  	if (!dev)
>  		return -ENOMEM;
> @@ -159,6 +164,10 @@ static int ksz8863_smi_probe(struct mdio_device *mdiodev)
>  		}
>  	}
>  
> +	ret = ksz_regfields_init(dev, chip);
> +	if (ret)
> +		return ret;
> +
>  	if (mdiodev->dev.platform_data)
>  		dev->pdata = mdiodev->dev.platform_data;
>  
> diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c b/drivers/net/dsa/microchip/ksz9477_i2c.c
> index 497be833f707..2cbd76aed974 100644
> --- a/drivers/net/dsa/microchip/ksz9477_i2c.c
> +++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
> @@ -16,10 +16,15 @@ KSZ_REGMAP_TABLE(ksz9477, not_used, 16, 0, 0);
>  
>  static int ksz9477_i2c_probe(struct i2c_client *i2c)
>  {
> +	const struct ksz_chip_data *chip;
>  	struct regmap_config rc;
>  	struct ksz_device *dev;
>  	int i, ret;
>  
> +	chip = device_get_match_data(ddev);

s/ddev/&i2c->dev/

> +	if (!chip)
> +		return -EINVAL;
> +
>  	dev = ksz_switch_alloc(&i2c->dev, i2c);
>  	if (!dev)
>  		return -ENOMEM;
> @@ -35,6 +40,10 @@ static int ksz9477_i2c_probe(struct i2c_client *i2c)
>  		}
>  	}
>  
> +	ret = ksz_regfields_init(dev, chip);
> +	if (ret)
> +		return ret;
> +
>  	if (i2c->dev.platform_data)
>  		dev->pdata = i2c->dev.platform_data;
>  

Re: [RFC/RFT PATCH net-next 2/4] net: dsa: microchip: partial conversion to regfields API for KSZ8795 (WIP)

[Oleksij Rempel]

On Fri, Mar 17, 2023 at 11:46:29AM +0200, Vladimir Oltean wrote:
> > +++ b/drivers/net/dsa/microchip/ksz8863_smi.c
> > @@ -136,11 +136,16 @@ static const struct regmap_config ksz8863_regmap_config[] = {
> >  
> >  static int ksz8863_smi_probe(struct mdio_device *mdiodev)
> >  {
> > +	const struct ksz_chip_data *chip;
> >  	struct regmap_config rc;
> >  	struct ksz_device *dev;
> >  	int ret;
> >  	int i;
> >  
> > +	chip = device_get_match_data(ddev);
> 
> s/ddev/&mdiodev->dev/

It fails on ksz8873 switch with following trace:

[    2.490822] 8<--- cut here ---
[    2.493907] Unable to handle kernel NULL pointer dereference at virtual address 00000004 when read
[    2.502924] [00000004] *pgd=00000000
[    2.506536] Internal error: Oops: 5 [#1] PREEMPT SMP ARM
[    2.511864] Modules linked in:
[    2.514935] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.3.0-rc1-00519-gd11a10757686-dirty #263
[    2.523562] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[    2.530100] PC is at ksz_regfields_init+0x44/0xa0
[    2.534833] LR is at _raw_spin_unlock_irqrestore+0x44/0x68
[    2.540336] pc : [<8075384c>]    lr : [<80ca2ca0>]    psr: a0000013
[    2.546614] sp : a0835c30  ip : a0835bd0  fp : a0835c5c
[    2.551848] r10: 80dacf48  r9 : 80dacdc0  r8 : 821d82ac
[    2.557083] r7 : 80dad00c  r6 : 00000000  r5 : 821d8240  r4 : 00000000
[    2.563622] r3 : 00000000  r2 : 00000000  r1 : 1ed17000  r0 : 821dac40
...
[    2.936367] Backtrace:                                                                    
[    2.938826]  ksz_regfields_init from ksz8863_smi_probe+0xfc/0x134
[    2.944962]  r6:00000003 r5:820bc800 r4:821d8240
[    2.949588]  ksz8863_smi_probe from mdio_probe+0x38/0x5c
[    2.954948]  r10:811495b8 r9:821da338 r8:00000000 r7:814dbbb8 r6:81468944 r5:820bc800
[    2.962786]  r4:81468944
[    2.965327]  mdio_probe from really_probe+0x1ac/0x3c8
[    2.970414]  r5:00000000 r4:820bc800                                                      
[    2.973997]  really_probe from __driver_probe_device+0x1d0/0x204
[    2.980036]  r8:814cc580 r7:814dbbb8 r6:820bc800 r5:81468944 r4:820bc800
[    2.986744]  __driver_probe_device from driver_probe_device+0x4c/0xc8
[    2.993217]  r9:821da338 r8:814cc580 r7:00000000 r6:820bc800 r5:81468944 r4:8154f4d4
[    3.000967]  driver_probe_device from __driver_attach+0x158/0x17c
[    3.007092]  r7:8067f830 r6:81468944 r5:81468944 r4:820bc800
[    3.012758]  __driver_attach from bus_for_each_dev+0x88/0xc8
[    3.018449]  r7:8067f830 r6:81468944 r5:81b25400 r4:821d0634
[    3.024116]  bus_for_each_dev from driver_attach+0x28/0x30
[    3.029631]  r7:00000000 r6:81b25400 r5:821da300 r4:81468944
[    3.035296]  driver_attach from bus_add_driver+0xdc/0x1f8
[    3.040719]  bus_add_driver from driver_register+0xc8/0x110
[    3.046326]  r9:814cc580 r8:814cc580 r7:00000000 r6:00000006 r5:81235290 r4:81468944
[    3.054076]  driver_register from mdio_driver_register+0x60/0xa0
[    3.060117]  r5:81235290 r4:81468944
[    3.063699]  mdio_driver_register from mdio_module_init+0x1c/0x24
[    3.069827]  r5:81235290 r4:81968940
[    3.073409]  mdio_module_init from do_one_initcall+0xac/0x214
[    3.079183]  do_one_initcall from kernel_init_freeable+0x1f8/0x244
[    3.085399]  r8:8124b858 r7:8124b834 r6:00000006 r5:00000075 r4:8199d580
[    3.092108]  kernel_init_freeable from kernel_init+0x24/0x140
[    3.097888]  r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:80c9d3a8
[    3.105726]  r4:81304f00
[    3.108268]  kernel_init from ret_from_fork+0x14/0x2c
[    3.113344] Exception stack(0xa0835fb0 to 0xa0835ff8)
...

There reason is that ksz8795_regfields[] is assigned only to KSZ8795.
KSZ8794, KSZ8765 and KSZ8830 (KSZ8863/KSZ8873) do not have needed regfields.

Please note, ksz8795_regfields[] is not compatible with KSZ8830 (KSZ8863/KSZ8873)
series.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [RFC/RFT PATCH net-next 2/4] net: dsa: microchip: partial conversion to regfields API for KSZ8795 (WIP)

[Vladimir Oltean]

On Fri, Mar 17, 2023 at 12:46:46PM +0100, Oleksij Rempel wrote:
> There reason is that ksz8795_regfields[] is assigned only to KSZ8795.
> KSZ8794, KSZ8765 and KSZ8830 (KSZ8863/KSZ8873) do not have needed regfields.
> 
> Please note, ksz8795_regfields[] is not compatible with KSZ8830 (KSZ8863/KSZ8873)
> series.

Right... well, it's kind of in the title and in the commit description:

| !! WARNING !! I only attempted to add a ksz_reg_fields structure for
| KSZ8795. The other switch families will currently crash!

If the only device you can test on is KSZ8873, that isn't going to help
me very much at the moment, because it doesn't have an xMII port, but
rather, either MII or RMII depending on part number. AFAIU, ksz_is_ksz88x3()
returns true for your device, and this means that neither phylink_mac_link_up()
nor phylink_mac_config() do nothing for your device. Also, above all,
ksz8863_regs[] does not have either P_XMII_CTRL_0 nor P_XMII_CTRL_1
defined, which are some of the registers I had converted to reg_fields,
in order to see whether it's possible to access a global register via a
port regfield call.

I'm going to let this patch set simmer for a few more days. If no one
volunteers to test on a KSZ8795, IMO the exercise is slightly pointless,
as that's where the problems were, and more and more blind reasoning
about what could be a problem isn't going to get us very far. I'd rather
not spend more time on this problem at this stage. I've copied some more
people who contributed patches to this switch family in the past few
years, in the hope that maybe someone can help.

For context, the cover letter is here:
https://patchwork.kernel.org/project/netdevbpf/cover/20230316161250.3286055-1-vladimir.oltean@nxp.com/

Re: [RFC/RFT PATCH net-next 2/4] net: dsa: microchip: partial conversion to regfields API for KSZ8795 (WIP)

[Oleksij Rempel]

On Fri, Mar 17, 2023 at 02:50:22PM +0200, Vladimir Oltean wrote:
> On Fri, Mar 17, 2023 at 12:46:46PM +0100, Oleksij Rempel wrote:
> > There reason is that ksz8795_regfields[] is assigned only to KSZ8795.
> > KSZ8794, KSZ8765 and KSZ8830 (KSZ8863/KSZ8873) do not have needed regfields.
> > 
> > Please note, ksz8795_regfields[] is not compatible with KSZ8830 (KSZ8863/KSZ8873)
> > series.
> 
> Right... well, it's kind of in the title and in the commit description:
> 
> | !! WARNING !! I only attempted to add a ksz_reg_fields structure for
> | KSZ8795. The other switch families will currently crash!
> 
> If the only device you can test on is KSZ8873, that isn't going to help
> me very much at the moment, because it doesn't have an xMII port, but
> rather, either MII or RMII depending on part number. AFAIU, ksz_is_ksz88x3()
> returns true for your device, and this means that neither phylink_mac_link_up()
> nor phylink_mac_config() do nothing for your device. Also, above all,
> ksz8863_regs[] does not have either P_XMII_CTRL_0 nor P_XMII_CTRL_1
> defined, which are some of the registers I had converted to reg_fields,
> in order to see whether it's possible to access a global register via a
> port regfield call.
> 
> I'm going to let this patch set simmer for a few more days. If no one
> volunteers to test on a KSZ8795, IMO the exercise is slightly pointless,
> as that's where the problems were, and more and more blind reasoning
> about what could be a problem isn't going to get us very far. I'd rather
> not spend more time on this problem at this stage. I've copied some more
> people who contributed patches to this switch family in the past few
> years, in the hope that maybe someone can help.
> 
> For context, the cover letter is here:
> https://patchwork.kernel.org/project/netdevbpf/cover/20230316161250.3286055-1-vladimir.oltean@nxp.com/

If you'll give up, may be i'll be able to take it over.

Thanks!
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [RFC/RFT PATCH net-next 2/4] net: dsa: microchip: partial conversion to regfields API for KSZ8795 (WIP)

[Vladimir Oltean]

On Fri, Mar 17, 2023 at 02:21:40PM +0100, Oleksij Rempel wrote:
> If you'll give up, may be i'll be able to take it over.

I have not given up yet, I just need someone to run a few tests on KSZ8795.

Re: [RFC/RFT PATCH net-next 2/4] net: dsa: microchip: partial conversion to regfields API for KSZ8795 (WIP)

[Vladimir Oltean]

On Fri, Mar 17, 2023 at 04:07:22PM +0200, Vladimir Oltean wrote:
> On Fri, Mar 17, 2023 at 02:21:40PM +0100, Oleksij Rempel wrote:
> > If you'll give up, may be i'll be able to take it over.
> 
> I have not given up yet, I just need someone to run a few tests on KSZ8795.

Seeing that the first line of people (able to test on KSZ8795) has no
opinion, maybe it's time to turn to the second line of people.

Russell, you've expressed that this driver is "vile" for permitting
access to the same hardware register through 2 different ksz8795_regs[]
elements. Does the approach used in this patch set address your concerns,
or in other words, do you believe is it worth extending the conversion
to the KSZ switches that other people can test, such that the entire
driver ultimately benefits from the conversion?

Re: [RFC/RFT PATCH net-next 2/4] net: dsa: microchip: partial conversion to regfields API for KSZ8795 (WIP)

[Russell King (Oracle)]

On Sat, Mar 25, 2023 at 02:16:55PM +0200, Vladimir Oltean wrote:
> Russell, you've expressed that this driver is "vile" for permitting
> access to the same hardware register through 2 different ksz8795_regs[]
> elements. Does the approach used in this patch set address your concerns,
> or in other words, do you believe is it worth extending the conversion
> to the KSZ switches that other people can test, such that the entire
> driver ultimately benefits from the conversion?

Thanks for pointing it out - I'll look at it next week.

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

[RFC/RFT PATCH net-next 3/4] net: dsa: microchip: allow setting xMII port speed/duplex on KSZ8765/KSZ8794/KSZ8795

[Vladimir Oltean]

Prior to commit c476bede4b0f ("net: dsa: microchip: ksz8795: use common
xmii function"), the logic from ksz8795_cpu_interface_select() used to
modify the following fields in REG_PORT_5_CTRL_6, based on the phy-mode
of the CPU port:
- PORT_INTERFACE_TYPE (now P_MII_SEL_M)
- PORT_GMII_1GPS_MODE (now P_GMII_1GBIT_M)
- PORT_RGMII_ID_IN_ENABLE (now P_RGMII_ID_IG_ENABLE)
- PORT_RGMII_ID_OUT_ENABLE (now P_RGMII_ID_EG_ENABLE)

That logic was replaced to a call with ksz_set_xmii(), which only
touches 3 of those 4 fields. It does not touch PORT_GMII_1GPS_MODE
(now P_GMII_1GBIT_M, or RF_GMII_1GBIT, in its reg_fields form).
That is handled by ksz_set_gbit(), which should have been called as
well, for code-wise identical behavior.

The driver has always written to PORT_GMII_1GPS_MODE to enable RGMII
ports at their maximum speed, since the initial commit e66f840c08a2
("net: dsa: ksz: Add Microchip KSZ8795 DSA driver"). Searching in the
KSZ8975 documentation, I see that the Is_1Gbps field (what the driver
calls P_GMII_1GBIT_M) is set to 1 by default, unless pin strapping via
LED1_0 sets it to zero, case in which the RGMII speed is either 10 or
100 Mbps - controlled via P_MII_100MBIT.

I can only imagine this being a problem if there are boards where the
pin strapping is wrong (asking for 100Mbps when the link was capable of
gigabit), and the initial version of the driver was overwriting that.
That may or may not be plausible. For example, this commit does indicate
a (different, but still) incorrect pin strapping setting which does need
to be fixed up by the Linux driver:
https://patchwork.kernel.org/project/netdevbpf/patch/20230315231916.2998480-1-vladimir.oltean@nxp.com/

In any case, it makes sense for Linux to configure the switch to match
the device tree description, and that means calling the missing
ksz_port_set_xmii_speed().

The current position of the ksz_port_set_xmii_speed() call is from the
ksz9477_phylink_mac_link_up() handler, but this is called for all
dev_ops except for ksz8_dev_ops.

Studyint ksz9477_phylink_mac_link_up() a bit, its contents seems to be
more than useful, since it also calls ksz_duplex_flowctrl() and that is
also something that KSZ8765 supports but doesn't call. It seems to be
desirable to move the entirety of ksz9477_phylink_mac_link_up() into the
common ksz_phylink_mac_link_up(), and remove the family-specific
handling.

The KSZ8830 switch is a bit special. It uses ksz8_dev_ops() (so it has
skipped phylink_mac_link_up() so far), but it uses the ksz8863_regs[],
which don't have P_GMII_1GBIT_M defined. This makes sense, since the
KSZ8830 switch comes either in RMII or MII variants, both of which are
fixed speeds (100Mbps). So we still need to skip phylink_mac_link_up()
completely for these, and the test for ksz_is_ksz88x3(), which is also
present in ksz_phylink_mac_config(), achieves that.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 38 ++++++--------------------
 drivers/net/dsa/microchip/ksz_common.h |  5 ----
 2 files changed, 9 insertions(+), 34 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 5bcbea8d9151..9bc26c5da254 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -216,13 +216,6 @@ static const struct ksz_dev_ops ksz8_dev_ops = {
 	.change_mtu = ksz8_change_mtu,
 };
 
-static void ksz9477_phylink_mac_link_up(struct ksz_device *dev, int port,
-					unsigned int mode,
-					phy_interface_t interface,
-					struct phy_device *phydev, int speed,
-					int duplex, bool tx_pause,
-					bool rx_pause);
-
 static const struct ksz_dev_ops ksz9477_dev_ops = {
 	.setup = ksz9477_setup,
 	.get_port_addr = ksz9477_get_port_addr,
@@ -249,7 +242,6 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
 	.mdb_add = ksz9477_mdb_add,
 	.mdb_del = ksz9477_mdb_del,
 	.change_mtu = ksz9477_change_mtu,
-	.phylink_mac_link_up = ksz9477_phylink_mac_link_up,
 	.config_cpu_port = ksz9477_config_cpu_port,
 	.tc_cbs_set_cinc = ksz9477_tc_cbs_set_cinc,
 	.enable_stp_addr = ksz9477_enable_stp_addr,
@@ -286,7 +278,6 @@ static const struct ksz_dev_ops lan937x_dev_ops = {
 	.mdb_add = ksz9477_mdb_add,
 	.mdb_del = ksz9477_mdb_del,
 	.change_mtu = lan937x_change_mtu,
-	.phylink_mac_link_up = ksz9477_phylink_mac_link_up,
 	.config_cpu_port = lan937x_config_cpu_port,
 	.tc_cbs_set_cinc = lan937x_tc_cbs_set_cinc,
 	.enable_stp_addr = ksz9477_enable_stp_addr,
@@ -2983,15 +2974,18 @@ static void ksz_duplex_flowctrl(struct ksz_device *dev, int port, int duplex,
 		ksz_regfields_write(dev, port, RF_MII_RX_FLOW_CTRL, !!rx_pause);
 }
 
-static void ksz9477_phylink_mac_link_up(struct ksz_device *dev, int port,
-					unsigned int mode,
-					phy_interface_t interface,
-					struct phy_device *phydev, int speed,
-					int duplex, bool tx_pause,
-					bool rx_pause)
+static void ksz_phylink_mac_link_up(struct dsa_switch *ds, int port,
+				    unsigned int mode,
+				    phy_interface_t interface,
+				    struct phy_device *phydev, int speed,
+				    int duplex, bool tx_pause, bool rx_pause)
 {
+	struct ksz_device *dev = ds->priv;
 	struct ksz_port *p;
 
+	if (ksz_is_ksz88x3(dev))
+		return;
+
 	p = &dev->ports[port];
 
 	/* Internal PHYs */
@@ -3005,20 +2999,6 @@ static void ksz9477_phylink_mac_link_up(struct ksz_device *dev, int port,
 	ksz_duplex_flowctrl(dev, port, duplex, tx_pause, rx_pause);
 }
 
-static void ksz_phylink_mac_link_up(struct dsa_switch *ds, int port,
-				    unsigned int mode,
-				    phy_interface_t interface,
-				    struct phy_device *phydev, int speed,
-				    int duplex, bool tx_pause, bool rx_pause)
-{
-	struct ksz_device *dev = ds->priv;
-
-	if (dev->dev_ops->phylink_mac_link_up)
-		dev->dev_ops->phylink_mac_link_up(dev, port, mode, interface,
-						  phydev, speed, duplex,
-						  tx_pause, rx_pause);
-}
-
 static int ksz_switch_detect(struct ksz_device *dev)
 {
 	u8 id1, id2, id4;
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index a92ebf5417b4..760e5f21faa1 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -381,11 +381,6 @@ struct ksz_dev_ops {
 	void (*phylink_mac_config)(struct ksz_device *dev, int port,
 				   unsigned int mode,
 				   const struct phylink_link_state *state);
-	void (*phylink_mac_link_up)(struct ksz_device *dev, int port,
-				    unsigned int mode,
-				    phy_interface_t interface,
-				    struct phy_device *phydev, int speed,
-				    int duplex, bool tx_pause, bool rx_pause);
 	void (*setup_rgmii_delay)(struct ksz_device *dev, int port);
 	int (*tc_cbs_set_cinc)(struct ksz_device *dev, int port, u32 val);
 	void (*config_cpu_port)(struct dsa_switch *ds);
-- 
2.34.1

[RFC/RFT PATCH net-next 4/4] net: dsa: microchip: remove unused dev->dev_ops->phylink_mac_config()

[Vladimir Oltean]

The history is that Arun first added a phylink_mac_config()
implementation for lan937x_dev_ops in commit a0cb1aa43825 ("net: dsa:
microchip: lan937x: add phylink_mac_config support"), then in commit
f3d890f5f90e ("net: dsa: microchip: add support for phylink mac
config"), that implementation became common for all switches, but the
dev_ops->phylink_mac_config() function pointer remained there, even
though there is no switch-specific handling anymore.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 3 ---
 drivers/net/dsa/microchip/ksz_common.h | 3 ---
 2 files changed, 6 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 9bc26c5da254..421e1212b210 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2894,9 +2894,6 @@ static void ksz_phylink_mac_config(struct dsa_switch *ds, int port,
 
 	ksz_set_xmii(dev, port, state->interface);
 
-	if (dev->dev_ops->phylink_mac_config)
-		dev->dev_ops->phylink_mac_config(dev, port, mode, state);
-
 	if (dev->dev_ops->setup_rgmii_delay)
 		dev->dev_ops->setup_rgmii_delay(dev, port);
 }
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 760e5f21faa1..618154f3c894 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -378,9 +378,6 @@ struct ksz_dev_ops {
 	int (*change_mtu)(struct ksz_device *dev, int port, int mtu);
 	void (*freeze_mib)(struct ksz_device *dev, int port, bool freeze);
 	void (*port_init_cnt)(struct ksz_device *dev, int port);
-	void (*phylink_mac_config)(struct ksz_device *dev, int port,
-				   unsigned int mode,
-				   const struct phylink_link_state *state);
 	void (*setup_rgmii_delay)(struct ksz_device *dev, int port);
 	int (*tc_cbs_set_cinc)(struct ksz_device *dev, int port, u32 val);
 	void (*config_cpu_port)(struct dsa_switch *ds);
-- 
2.34.1

Re: [RFC/RFT PATCH net-next 0/4] KSZ DSA driver: xMII speed adjustment and partial reg_fields conversion

[Arun.Ramadoss]

Hi Vladimir,
On Thu, 2023-03-16 at 18:12 +0200, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> 
> ABSOLUTELY NO TESTING WAS DONE. I don't have the hardware.
> 
> THIS BREAKS EVERYTHING EXCEPT FOR KSZ8795. Any testers should test on
> that if possible (due to both patches 2/4, and 3/4).

I don't have KSZ87xx and KSZ88xx series of boards. I can able to test
only on KSZ9477 and LAN937x series of boards. I will wait for next
version of patch to do sanity test on this KSZ9477 and LAN937x series
of boards.

> 
> Vladimir Oltean (4):
>   net: dsa: microchip: add an enum for regmap widths
>   net: dsa: microchip: partial conversion to regfields API for
> KSZ8795
>     (WIP)
>   net: dsa: microchip: allow setting xMII port speed/duplex on
>     KSZ8765/KSZ8794/KSZ8795
>   net: dsa: microchip: remove unused dev->dev_ops-
> >phylink_mac_config()
> 
>  drivers/net/dsa/microchip/ksz8795.c      |  45 ++--
>  drivers/net/dsa/microchip/ksz8863_smi.c  |  11 +-
>  drivers/net/dsa/microchip/ksz9477.c      |  24 +--
>  drivers/net/dsa/microchip/ksz9477_i2c.c  |  11 +-
>  drivers/net/dsa/microchip/ksz_common.c   | 256 +++++++++++++------
> ----
>  drivers/net/dsa/microchip/ksz_common.h   | 110 +++++++---
>  drivers/net/dsa/microchip/ksz_spi.c      |   6 +-
>  drivers/net/dsa/microchip/lan937x_main.c |   8 +-
>  8 files changed, 299 insertions(+), 172 deletions(-)
> 
> --
> 2.34.1
> 

Re: [RFC/RFT PATCH net-next 0/4] KSZ DSA driver: xMII speed adjustment and partial reg_fields conversion

[Oleksij Rempel]

Hi Vladimir,

On Thu, Mar 16, 2023 at 06:12:46PM +0200, Vladimir Oltean wrote:
> Hi,
> 
> Yesterday I picked up this patch and resubmitted it:
> https://patchwork.kernel.org/project/netdevbpf/patch/20230222031738.189025-1-marex@denx.de/
> here:
> https://patchwork.kernel.org/project/netdevbpf/patch/20230315231916.2998480-1-vladimir.oltean@nxp.com/
> 
> and today I'm trying to address the rest of the points brought up in
> that conversation, namely:
> 
> - commit c476bede4b0f ("net: dsa: microchip: ksz8795: use common xmii
>   function") stopped adjusting the xMII port speed on KSZ8795, does it
>   still work? No idea. Patch 3/4 deals with that.
> 
> - Mapping P_XMII_CTRL_0 and P_XMII_CTRL_1 to the same value on KSZ8795
>   raised some eyebrows, and some reading shows that it is also partially
>   incorrect (see patch 2/4). This is also where I propose to convert to
>   reg_fields.
> 
> As it turns out, patch 2/4 is a dependency for patch 3/4, even if 3/4
> may be a fix.
> 
> Patch 1/4 is a dependency of 2/4.
> 
> Patch 4/4 is something I also noticed during review. I left it at the
> end so that it won't conflict with something that could reasonably be
> submitted as a bug fix.
> 
> ABSOLUTELY NO TESTING WAS DONE. I don't have the hardware.
> 
> THIS BREAKS EVERYTHING EXCEPT FOR KSZ8795. Any testers should test on
> that if possible (due to both patches 2/4, and 3/4).

I can test it on KSZ8873, but currently it is not compiling on top of net-next.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [RFC/RFT PATCH net-next 0/4] KSZ DSA driver: xMII speed adjustment and partial reg_fields conversion

[Vladimir Oltean]

On Fri, Mar 17, 2023 at 07:02:20AM +0100, Oleksij Rempel wrote:
> I can test it on KSZ8873, but currently it is not compiling on top of net-next.

That's my fault. I reset my defconfig, and looks like that only kept
CONFIG_NET_DSA_MICROCHIP_KSZ_COMMON=y, disabling all the individual
KSZ drivers. I will leave comments inline where changes are necessary.