[RFT][PATCH V2 00/10] net: dsa: microchip: Convert to regmap

[RFT][PATCH V2 00/10] net: dsa: microchip: Convert to regmap

[Marek Vasut]

This patchset converts KSZ9477 switch driver to regmap.

This was tested with extra patches on KSZ8795 and compile-tested
for KSZ9477, as I don't own a device with KSZ9477.

Tristram, I hope this helps you get started with the regmap.
Please test on the KSZ9477 and let me know how it works, or
ideally provide fixes :)

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Tristram Ha <Tristram.Ha@microchip.com>
Cc: Woojung Huh <Woojung.Huh@microchip.com>

Marek Vasut (10):
  net: dsa: microchip: Remove ksz_{read,write}24()
  net: dsa: microchip: Remove ksz_{get,set}()
  net: dsa: microchip: Inline ksz_spi.h
  net: dsa: microchip: Move ksz_cfg and ksz_port_cfg to ksz9477.c
  net: dsa: microchip: Use PORT_CTRL_ADDR() instead of indirect function
    call
  net: dsa: microchip: Factor out register access opcode generation
  net: dsa: microchip: Initial SPI regmap support
  net: dsa: microchip: Dispose of ksz_io_ops
  net: dsa: microchip: Factor out regmap config generation into common
    header
  net: dsa: microchip: Replace ad-hoc bit manipulation with regmap

 drivers/net/dsa/microchip/Kconfig       |   1 +
 drivers/net/dsa/microchip/ksz9477.c     |  35 +++---
 drivers/net/dsa/microchip/ksz9477_spi.c | 112 +++--------------
 drivers/net/dsa/microchip/ksz_common.c  |   6 +-
 drivers/net/dsa/microchip/ksz_common.h  | 153 +++++++-----------------
 drivers/net/dsa/microchip/ksz_priv.h    |  23 +---
 drivers/net/dsa/microchip/ksz_spi.h     |  69 -----------
 7 files changed, 79 insertions(+), 320 deletions(-)
 delete mode 100644 drivers/net/dsa/microchip/ksz_spi.h

-- 
2.19.2

[RFT][PATCH V2 01/10] net: dsa: microchip: Remove ksz_{read,write}24()

[Marek Vasut]

These functions and callbacks are never used, remove them.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Tristram Ha <Tristram.Ha@microchip.com>
Cc: Woojung Huh <Woojung.Huh@microchip.com>
---
V2: No change
---
 drivers/net/dsa/microchip/ksz9477_spi.c | 25 -------------------------
 drivers/net/dsa/microchip/ksz_common.h  | 22 ----------------------
 drivers/net/dsa/microchip/ksz_priv.h    |  2 --
 3 files changed, 49 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477_spi.c b/drivers/net/dsa/microchip/ksz9477_spi.c
index d757ba151cb1..695def784933 100644
--- a/drivers/net/dsa/microchip/ksz9477_spi.c
+++ b/drivers/net/dsa/microchip/ksz9477_spi.c
@@ -73,37 +73,12 @@ static int ksz_spi_write(struct ksz_device *dev, u32 reg, void *data,
 	return ksz9477_spi_write_reg(spi, reg, dev->txbuf, len);
 }
 
-static int ksz_spi_read24(struct ksz_device *dev, u32 reg, u32 *val)
-{
-	int ret;
-
-	*val = 0;
-	ret = ksz_spi_read(dev, reg, (u8 *)val, 3);
-	if (!ret) {
-		*val = be32_to_cpu(*val);
-		/* convert to 24bit */
-		*val >>= 8;
-	}
-
-	return ret;
-}
-
-static int ksz_spi_write24(struct ksz_device *dev, u32 reg, u32 value)
-{
-	/* make it to big endian 24bit from MSB */
-	value <<= 8;
-	value = cpu_to_be32(value);
-	return ksz_spi_write(dev, reg, &value, 3);
-}
-
 static const struct ksz_io_ops ksz9477_spi_ops = {
 	.read8 = ksz_spi_read8,
 	.read16 = ksz_spi_read16,
-	.read24 = ksz_spi_read24,
 	.read32 = ksz_spi_read32,
 	.write8 = ksz_spi_write8,
 	.write16 = ksz_spi_write16,
-	.write24 = ksz_spi_write24,
 	.write32 = ksz_spi_write32,
 	.get = ksz_spi_get,
 	.set = ksz_spi_set,
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 2dd832de0d52..5f2206a9b3e0 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -56,17 +56,6 @@ static inline int ksz_read16(struct ksz_device *dev, u32 reg, u16 *val)
 	return ret;
 }
 
-static inline int ksz_read24(struct ksz_device *dev, u32 reg, u32 *val)
-{
-	int ret;
-
-	mutex_lock(&dev->reg_mutex);
-	ret = dev->ops->read24(dev, reg, val);
-	mutex_unlock(&dev->reg_mutex);
-
-	return ret;
-}
-
 static inline int ksz_read32(struct ksz_device *dev, u32 reg, u32 *val)
 {
 	int ret;
@@ -100,17 +89,6 @@ static inline int ksz_write16(struct ksz_device *dev, u32 reg, u16 value)
 	return ret;
 }
 
-static inline int ksz_write24(struct ksz_device *dev, u32 reg, u32 value)
-{
-	int ret;
-
-	mutex_lock(&dev->reg_mutex);
-	ret = dev->ops->write24(dev, reg, value);
-	mutex_unlock(&dev->reg_mutex);
-
-	return ret;
-}
-
 static inline int ksz_write32(struct ksz_device *dev, u32 reg, u32 value)
 {
 	int ret;
diff --git a/drivers/net/dsa/microchip/ksz_priv.h b/drivers/net/dsa/microchip/ksz_priv.h
index 60b49010904b..db16002b6b6a 100644
--- a/drivers/net/dsa/microchip/ksz_priv.h
+++ b/drivers/net/dsa/microchip/ksz_priv.h
@@ -104,11 +104,9 @@ struct ksz_device {
 struct ksz_io_ops {
 	int (*read8)(struct ksz_device *dev, u32 reg, u8 *value);
 	int (*read16)(struct ksz_device *dev, u32 reg, u16 *value);
-	int (*read24)(struct ksz_device *dev, u32 reg, u32 *value);
 	int (*read32)(struct ksz_device *dev, u32 reg, u32 *value);
 	int (*write8)(struct ksz_device *dev, u32 reg, u8 value);
 	int (*write16)(struct ksz_device *dev, u32 reg, u16 value);
-	int (*write24)(struct ksz_device *dev, u32 reg, u32 value);
 	int (*write32)(struct ksz_device *dev, u32 reg, u32 value);
 	int (*get)(struct ksz_device *dev, u32 reg, void *data, size_t len);
 	int (*set)(struct ksz_device *dev, u32 reg, void *data, size_t len);
-- 
2.19.2

[RFT][PATCH V2 02/10] net: dsa: microchip: Remove ksz_{get,set}()

[Marek Vasut]

These functions and callbacks are never used, remove them.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Tristram Ha <Tristram.Ha@microchip.com>
Cc: Woojung Huh <Woojung.Huh@microchip.com>
---
V2: No change
---
 drivers/net/dsa/microchip/ksz9477_spi.c |  2 --
 drivers/net/dsa/microchip/ksz_common.h  | 24 ------------------------
 drivers/net/dsa/microchip/ksz_priv.h    |  2 --
 drivers/net/dsa/microchip/ksz_spi.h     | 10 ----------
 4 files changed, 38 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477_spi.c b/drivers/net/dsa/microchip/ksz9477_spi.c
index 695def784933..2d5262b01c77 100644
--- a/drivers/net/dsa/microchip/ksz9477_spi.c
+++ b/drivers/net/dsa/microchip/ksz9477_spi.c
@@ -80,8 +80,6 @@ static const struct ksz_io_ops ksz9477_spi_ops = {
 	.write8 = ksz_spi_write8,
 	.write16 = ksz_spi_write16,
 	.write32 = ksz_spi_write32,
-	.get = ksz_spi_get,
-	.set = ksz_spi_set,
 };
 
 static int ksz9477_spi_probe(struct spi_device *spi)
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 5f2206a9b3e0..8611122a51d6 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -100,30 +100,6 @@ static inline int ksz_write32(struct ksz_device *dev, u32 reg, u32 value)
 	return ret;
 }
 
-static inline int ksz_get(struct ksz_device *dev, u32 reg, void *data,
-			  size_t len)
-{
-	int ret;
-
-	mutex_lock(&dev->reg_mutex);
-	ret = dev->ops->get(dev, reg, data, len);
-	mutex_unlock(&dev->reg_mutex);
-
-	return ret;
-}
-
-static inline int ksz_set(struct ksz_device *dev, u32 reg, void *data,
-			  size_t len)
-{
-	int ret;
-
-	mutex_lock(&dev->reg_mutex);
-	ret = dev->ops->set(dev, reg, data, len);
-	mutex_unlock(&dev->reg_mutex);
-
-	return ret;
-}
-
 static inline void ksz_pread8(struct ksz_device *dev, int port, int offset,
 			      u8 *data)
 {
diff --git a/drivers/net/dsa/microchip/ksz_priv.h b/drivers/net/dsa/microchip/ksz_priv.h
index db16002b6b6a..c3a272505af1 100644
--- a/drivers/net/dsa/microchip/ksz_priv.h
+++ b/drivers/net/dsa/microchip/ksz_priv.h
@@ -108,8 +108,6 @@ struct ksz_io_ops {
 	int (*write8)(struct ksz_device *dev, u32 reg, u8 value);
 	int (*write16)(struct ksz_device *dev, u32 reg, u16 value);
 	int (*write32)(struct ksz_device *dev, u32 reg, u32 value);
-	int (*get)(struct ksz_device *dev, u32 reg, void *data, size_t len);
-	int (*set)(struct ksz_device *dev, u32 reg, void *data, size_t len);
 };
 
 struct alu_struct {
diff --git a/drivers/net/dsa/microchip/ksz_spi.h b/drivers/net/dsa/microchip/ksz_spi.h
index 427811bd60b3..976bace31f37 100644
--- a/drivers/net/dsa/microchip/ksz_spi.h
+++ b/drivers/net/dsa/microchip/ksz_spi.h
@@ -56,14 +56,4 @@ static int ksz_spi_write32(struct ksz_device *dev, u32 reg, u32 value)
 	return ksz_spi_write(dev, reg, &value, 4);
 }
 
-static int ksz_spi_get(struct ksz_device *dev, u32 reg, void *data, size_t len)
-{
-	return ksz_spi_read(dev, reg, data, len);
-}
-
-static int ksz_spi_set(struct ksz_device *dev, u32 reg, void *data, size_t len)
-{
-	return ksz_spi_write(dev, reg, data, len);
-}
-
 #endif
-- 
2.19.2

[RFT][PATCH V2 03/10] net: dsa: microchip: Inline ksz_spi.h

[Marek Vasut]

The functions in the header file are static, and the header file is
included from single C file, just inline the code into the C file.
The bonus is that it's easier to spot further content to clean up.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Tristram Ha <Tristram.Ha@microchip.com>
Cc: Woojung Huh <Woojung.Huh@microchip.com>
---
V2: No change
---
 drivers/net/dsa/microchip/ksz9477_spi.c | 43 +++++++++++++++++-
 drivers/net/dsa/microchip/ksz_spi.h     | 59 -------------------------
 2 files changed, 42 insertions(+), 60 deletions(-)
 delete mode 100644 drivers/net/dsa/microchip/ksz_spi.h

diff --git a/drivers/net/dsa/microchip/ksz9477_spi.c b/drivers/net/dsa/microchip/ksz9477_spi.c
index 2d5262b01c77..9ca150a472ea 100644
--- a/drivers/net/dsa/microchip/ksz9477_spi.c
+++ b/drivers/net/dsa/microchip/ksz9477_spi.c
@@ -13,7 +13,6 @@
 #include <linux/spi/spi.h>
 
 #include "ksz_priv.h"
-#include "ksz_spi.h"
 
 /* SPI frame opcodes */
 #define KS_SPIOP_RD			3
@@ -73,6 +72,48 @@ static int ksz_spi_write(struct ksz_device *dev, u32 reg, void *data,
 	return ksz9477_spi_write_reg(spi, reg, dev->txbuf, len);
 }
 
+static int ksz_spi_read8(struct ksz_device *dev, u32 reg, u8 *val)
+{
+	return ksz_spi_read(dev, reg, val, 1);
+}
+
+static int ksz_spi_read16(struct ksz_device *dev, u32 reg, u16 *val)
+{
+	int ret = ksz_spi_read(dev, reg, (u8 *)val, 2);
+
+	if (!ret)
+		*val = be16_to_cpu(*val);
+
+	return ret;
+}
+
+static int ksz_spi_read32(struct ksz_device *dev, u32 reg, u32 *val)
+{
+	int ret = ksz_spi_read(dev, reg, (u8 *)val, 4);
+
+	if (!ret)
+		*val = be32_to_cpu(*val);
+
+	return ret;
+}
+
+static int ksz_spi_write8(struct ksz_device *dev, u32 reg, u8 value)
+{
+	return ksz_spi_write(dev, reg, &value, 1);
+}
+
+static int ksz_spi_write16(struct ksz_device *dev, u32 reg, u16 value)
+{
+	value = cpu_to_be16(value);
+	return ksz_spi_write(dev, reg, &value, 2);
+}
+
+static int ksz_spi_write32(struct ksz_device *dev, u32 reg, u32 value)
+{
+	value = cpu_to_be32(value);
+	return ksz_spi_write(dev, reg, &value, 4);
+}
+
 static const struct ksz_io_ops ksz9477_spi_ops = {
 	.read8 = ksz_spi_read8,
 	.read16 = ksz_spi_read16,
diff --git a/drivers/net/dsa/microchip/ksz_spi.h b/drivers/net/dsa/microchip/ksz_spi.h
deleted file mode 100644
index 976bace31f37..000000000000
--- a/drivers/net/dsa/microchip/ksz_spi.h
+++ /dev/null
@@ -1,59 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0
- * Microchip KSZ series SPI access common header
- *
- * Copyright (C) 2017-2018 Microchip Technology Inc.
- *	Tristram Ha <Tristram.Ha@microchip.com>
- */
-
-#ifndef __KSZ_SPI_H
-#define __KSZ_SPI_H
-
-/* Chip dependent SPI access */
-static int ksz_spi_read(struct ksz_device *dev, u32 reg, u8 *data,
-			unsigned int len);
-static int ksz_spi_write(struct ksz_device *dev, u32 reg, void *data,
-			 unsigned int len);
-
-static int ksz_spi_read8(struct ksz_device *dev, u32 reg, u8 *val)
-{
-	return ksz_spi_read(dev, reg, val, 1);
-}
-
-static int ksz_spi_read16(struct ksz_device *dev, u32 reg, u16 *val)
-{
-	int ret = ksz_spi_read(dev, reg, (u8 *)val, 2);
-
-	if (!ret)
-		*val = be16_to_cpu(*val);
-
-	return ret;
-}
-
-static int ksz_spi_read32(struct ksz_device *dev, u32 reg, u32 *val)
-{
-	int ret = ksz_spi_read(dev, reg, (u8 *)val, 4);
-
-	if (!ret)
-		*val = be32_to_cpu(*val);
-
-	return ret;
-}
-
-static int ksz_spi_write8(struct ksz_device *dev, u32 reg, u8 value)
-{
-	return ksz_spi_write(dev, reg, &value, 1);
-}
-
-static int ksz_spi_write16(struct ksz_device *dev, u32 reg, u16 value)
-{
-	value = cpu_to_be16(value);
-	return ksz_spi_write(dev, reg, &value, 2);
-}
-
-static int ksz_spi_write32(struct ksz_device *dev, u32 reg, u32 value)
-{
-	value = cpu_to_be32(value);
-	return ksz_spi_write(dev, reg, &value, 4);
-}
-
-#endif
-- 
2.19.2

[RFT][PATCH V2 04/10] net: dsa: microchip: Move ksz_cfg and ksz_port_cfg to ksz9477.c

[Marek Vasut]

These functions are only used by the KSZ9477 code, move them from
the header into that code. Note that these functions will be soon
replaced by regmap equivalents.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Tristram Ha <Tristram.Ha@microchip.com>
Cc: Woojung Huh <Woojung.Huh@microchip.com>
---
V2: New patch
---
 drivers/net/dsa/microchip/ksz9477.c    | 29 ++++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz_common.h | 29 --------------------------
 2 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 57a146a0dd4a..017d26c03b42 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -63,6 +63,35 @@ static const struct {
 	{ 0x83, "tx_discards" },
 };
 
+static void ksz_cfg(struct ksz_device *dev, u32 addr, u8 bits, bool set)
+{
+	u8 data;
+
+	ksz_read8(dev, addr, &data);
+	if (set)
+		data |= bits;
+	else
+		data &= ~bits;
+	ksz_write8(dev, addr, data);
+}
+
+static void ksz_port_cfg(struct ksz_device *dev, int port, int offset, u8 bits,
+			 bool set)
+{
+	u32 addr;
+	u8 data;
+
+	addr = dev->dev_ops->get_port_addr(port, offset);
+	ksz_read8(dev, addr, &data);
+
+	if (set)
+		data |= bits;
+	else
+		data &= ~bits;
+
+	ksz_write8(dev, addr, data);
+}
+
 static void ksz9477_cfg32(struct ksz_device *dev, u32 addr, u32 bits, bool set)
 {
 	u32 data;
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 8611122a51d6..9ce91fb03efe 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -136,33 +136,4 @@ static inline void ksz_pwrite32(struct ksz_device *dev, int port, int offset,
 	ksz_write32(dev, dev->dev_ops->get_port_addr(port, offset), data);
 }
 
-static void ksz_cfg(struct ksz_device *dev, u32 addr, u8 bits, bool set)
-{
-	u8 data;
-
-	ksz_read8(dev, addr, &data);
-	if (set)
-		data |= bits;
-	else
-		data &= ~bits;
-	ksz_write8(dev, addr, data);
-}
-
-static void ksz_port_cfg(struct ksz_device *dev, int port, int offset, u8 bits,
-			 bool set)
-{
-	u32 addr;
-	u8 data;
-
-	addr = dev->dev_ops->get_port_addr(port, offset);
-	ksz_read8(dev, addr, &data);
-
-	if (set)
-		data |= bits;
-	else
-		data &= ~bits;
-
-	ksz_write8(dev, addr, data);
-}
-
 #endif
-- 
2.19.2

[RFT][PATCH V2 05/10] net: dsa: microchip: Use PORT_CTRL_ADDR() instead of indirect function call

[Marek Vasut]

The indirect function call to dev->dev_ops->get_port_addr() is expensive
especially if called for every single register access, and only returns
the value of PORT_CTRL_ADDR() macro. Use PORT_CTRL_ADDR() macro directly
instead.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Tristram Ha <Tristram.Ha@microchip.com>
Cc: Woojung Huh <Woojung.Huh@microchip.com>
---
V2: New patch
---
 drivers/net/dsa/microchip/ksz9477.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 017d26c03b42..5e720a9e3659 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -81,7 +81,7 @@ static void ksz_port_cfg(struct ksz_device *dev, int port, int offset, u8 bits,
 	u32 addr;
 	u8 data;
 
-	addr = dev->dev_ops->get_port_addr(port, offset);
+	addr = PORT_CTRL_ADDR(port, offset);
 	ksz_read8(dev, addr, &data);
 
 	if (set)
-- 
2.19.2

[RFT][PATCH V2 06/10] net: dsa: microchip: Factor out register access opcode generation

[Marek Vasut]

Factor out the code which sends out the register read/write opcodes
to the switch, since the code differs in single bit between read and
write.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Tristram Ha <Tristram.Ha@microchip.com>
Cc: Woojung Huh <Woojung.Huh@microchip.com>
---
V2: New patch
---
 drivers/net/dsa/microchip/ksz9477_spi.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477_spi.c b/drivers/net/dsa/microchip/ksz9477_spi.c
index 9ca150a472ea..3eee397f8bb4 100644
--- a/drivers/net/dsa/microchip/ksz9477_spi.c
+++ b/drivers/net/dsa/microchip/ksz9477_spi.c
@@ -25,19 +25,24 @@
 /* Enough to read all switch port registers. */
 #define SPI_TX_BUF_LEN			0x100
 
-static int ksz9477_spi_read_reg(struct spi_device *spi, u32 reg, u8 *val,
-				unsigned int len)
+static u32 ksz9477_spi_cmd(u32 reg, bool read)
 {
 	u32 txbuf;
-	int ret;
 
 	txbuf = reg & SPI_ADDR_MASK;
-	txbuf |= KS_SPIOP_RD << SPI_ADDR_SHIFT;
+	txbuf |= (read ? KS_SPIOP_RD : KS_SPIOP_WR) << SPI_ADDR_SHIFT;
 	txbuf <<= SPI_TURNAROUND_SHIFT;
 	txbuf = cpu_to_be32(txbuf);
 
-	ret = spi_write_then_read(spi, &txbuf, 4, val, len);
-	return ret;
+	return txbuf;
+}
+
+static int ksz9477_spi_read_reg(struct spi_device *spi, u32 reg, u8 *val,
+				unsigned int len)
+{
+	u32 txbuf = ksz9477_spi_cmd(reg, true);
+
+	return spi_write_then_read(spi, &txbuf, 4, val, len);
 }
 
 static int ksz9477_spi_write_reg(struct spi_device *spi, u32 reg, u8 *val,
@@ -45,10 +50,7 @@ static int ksz9477_spi_write_reg(struct spi_device *spi, u32 reg, u8 *val,
 {
 	u32 *txbuf = (u32 *)val;
 
-	*txbuf = reg & SPI_ADDR_MASK;
-	*txbuf |= (KS_SPIOP_WR << SPI_ADDR_SHIFT);
-	*txbuf <<= SPI_TURNAROUND_SHIFT;
-	*txbuf = cpu_to_be32(*txbuf);
+	*txbuf = ksz9477_spi_cmd(reg, false);
 
 	return spi_write(spi, txbuf, 4 + len);
 }
-- 
2.19.2

[RFT][PATCH V2 07/10] net: dsa: microchip: Initial SPI regmap support

[Marek Vasut]

Add basic SPI regmap support into the driver.

Previous patches unconver that ksz_spi_write() is always ever called
with len = 1, 2 or 4. We can thus drop the if (len > SPI_TX_BUF_LEN)
check and we can also drop the allocation of the txbuf which is part
of the driver data and wastes 256 bytes for no reason. Regmap covers
the whole thing now.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Tristram Ha <Tristram.Ha@microchip.com>
Cc: Woojung Huh <Woojung.Huh@microchip.com>
---
V2: - Squash with "net: dsa: microchip: Remove dev->txbuf"
    - Use separate regmaps for 8/16/32bit registers
    - Increase the regmap size to 0xd00 to cover the entire register area
---
 drivers/net/dsa/microchip/Kconfig       |   1 +
 drivers/net/dsa/microchip/ksz9477_spi.c | 113 ++++++++++--------------
 drivers/net/dsa/microchip/ksz_priv.h    |   3 +-
 3 files changed, 51 insertions(+), 66 deletions(-)

diff --git a/drivers/net/dsa/microchip/Kconfig b/drivers/net/dsa/microchip/Kconfig
index bea29fde9f3d..3a00cb1b372a 100644
--- a/drivers/net/dsa/microchip/Kconfig
+++ b/drivers/net/dsa/microchip/Kconfig
@@ -1,4 +1,5 @@
 config NET_DSA_MICROCHIP_KSZ_COMMON
+	select REGMAP_SPI
 	tristate
 
 menuconfig NET_DSA_MICROCHIP_KSZ9477
diff --git a/drivers/net/dsa/microchip/ksz9477_spi.c b/drivers/net/dsa/microchip/ksz9477_spi.c
index 3eee397f8bb4..b624c496993e 100644
--- a/drivers/net/dsa/microchip/ksz9477_spi.c
+++ b/drivers/net/dsa/microchip/ksz9477_spi.c
@@ -10,78 +10,53 @@
 #include <linux/delay.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/regmap.h>
 #include <linux/spi/spi.h>
 
 #include "ksz_priv.h"
 
-/* SPI frame opcodes */
-#define KS_SPIOP_RD			3
-#define KS_SPIOP_WR			2
-
 #define SPI_ADDR_SHIFT			24
-#define SPI_ADDR_MASK			(BIT(SPI_ADDR_SHIFT) - 1)
 #define SPI_TURNAROUND_SHIFT		5
 
-/* Enough to read all switch port registers. */
-#define SPI_TX_BUF_LEN			0x100
-
-static u32 ksz9477_spi_cmd(u32 reg, bool read)
-{
-	u32 txbuf;
-
-	txbuf = reg & SPI_ADDR_MASK;
-	txbuf |= (read ? KS_SPIOP_RD : KS_SPIOP_WR) << SPI_ADDR_SHIFT;
-	txbuf <<= SPI_TURNAROUND_SHIFT;
-	txbuf = cpu_to_be32(txbuf);
-
-	return txbuf;
-}
-
-static int ksz9477_spi_read_reg(struct spi_device *spi, u32 reg, u8 *val,
-				unsigned int len)
-{
-	u32 txbuf = ksz9477_spi_cmd(reg, true);
-
-	return spi_write_then_read(spi, &txbuf, 4, val, len);
-}
-
-static int ksz9477_spi_write_reg(struct spi_device *spi, u32 reg, u8 *val,
-				 unsigned int len)
-{
-	u32 *txbuf = (u32 *)val;
-
-	*txbuf = ksz9477_spi_cmd(reg, false);
-
-	return spi_write(spi, txbuf, 4 + len);
-}
-
-static int ksz_spi_read(struct ksz_device *dev, u32 reg, u8 *data,
-			unsigned int len)
-{
-	struct spi_device *spi = dev->priv;
-
-	return ksz9477_spi_read_reg(spi, reg, data, len);
-}
-
-static int ksz_spi_write(struct ksz_device *dev, u32 reg, void *data,
-			 unsigned int len)
-{
-	struct spi_device *spi = dev->priv;
+/* SPI frame opcodes */
+#define KS_SPIOP_RD			3
+#define KS_SPIOP_WR			2
 
-	if (len > SPI_TX_BUF_LEN)
-		len = SPI_TX_BUF_LEN;
-	memcpy(&dev->txbuf[4], data, len);
-	return ksz9477_spi_write_reg(spi, reg, dev->txbuf, len);
-}
+#define KS_SPIOP_FLAG_MASK(opcode)		\
+	cpu_to_be16((opcode) << (SPI_ADDR_SHIFT + SPI_TURNAROUND_SHIFT))
+
+#define KSZ_REGMAP_COMMON(width)					\
+	{								\
+		.val_bits = (width),					\
+		.reg_stride = (width) / 8,				\
+		.reg_bits = SPI_ADDR_SHIFT,				\
+		.pad_bits = SPI_TURNAROUND_SHIFT,			\
+		.max_register = 0xF00,					\
+		.cache_type = REGCACHE_NONE,				\
+		.read_flag_mask = KS_SPIOP_FLAG_MASK(KS_SPIOP_RD),	\
+		.write_flag_mask = KS_SPIOP_FLAG_MASK(KS_SPIOP_WR),	\
+		.reg_format_endian = REGMAP_ENDIAN_BIG,			\
+		.val_format_endian = REGMAP_ENDIAN_BIG			\
+	}
+
+static const struct regmap_config ksz9477_regmap_config[] = {
+	KSZ_REGMAP_COMMON(8),
+	KSZ_REGMAP_COMMON(16),
+	KSZ_REGMAP_COMMON(32),
+};
 
 static int ksz_spi_read8(struct ksz_device *dev, u32 reg, u8 *val)
 {
-	return ksz_spi_read(dev, reg, val, 1);
+	unsigned int value;
+	int ret = regmap_read(dev->regmap, reg, &value);
+
+	*val = value;
+	return ret;
 }
 
 static int ksz_spi_read16(struct ksz_device *dev, u32 reg, u16 *val)
 {
-	int ret = ksz_spi_read(dev, reg, (u8 *)val, 2);
+	int ret = regmap_bulk_read(dev->regmap, reg, val, 2);
 
 	if (!ret)
 		*val = be16_to_cpu(*val);
@@ -91,7 +66,7 @@ static int ksz_spi_read16(struct ksz_device *dev, u32 reg, u16 *val)
 
 static int ksz_spi_read32(struct ksz_device *dev, u32 reg, u32 *val)
 {
-	int ret = ksz_spi_read(dev, reg, (u8 *)val, 4);
+	int ret = regmap_bulk_read(dev->regmap, reg, val, 4);
 
 	if (!ret)
 		*val = be32_to_cpu(*val);
@@ -101,19 +76,19 @@ static int ksz_spi_read32(struct ksz_device *dev, u32 reg, u32 *val)
 
 static int ksz_spi_write8(struct ksz_device *dev, u32 reg, u8 value)
 {
-	return ksz_spi_write(dev, reg, &value, 1);
+	return regmap_write(dev->regmap, reg, value);
 }
 
 static int ksz_spi_write16(struct ksz_device *dev, u32 reg, u16 value)
 {
 	value = cpu_to_be16(value);
-	return ksz_spi_write(dev, reg, &value, 2);
+	return regmap_bulk_write(dev->regmap, reg, &value, 2);
 }
 
 static int ksz_spi_write32(struct ksz_device *dev, u32 reg, u32 value)
 {
 	value = cpu_to_be32(value);
-	return ksz_spi_write(dev, reg, &value, 4);
+	return regmap_bulk_write(dev->regmap, reg, &value, 4);
 }
 
 static const struct ksz_io_ops ksz9477_spi_ops = {
@@ -128,17 +103,27 @@ static const struct ksz_io_ops ksz9477_spi_ops = {
 static int ksz9477_spi_probe(struct spi_device *spi)
 {
 	struct ksz_device *dev;
-	int ret;
+	int i, ret;
 
 	dev = ksz_switch_alloc(&spi->dev, &ksz9477_spi_ops, spi);
 	if (!dev)
 		return -ENOMEM;
 
+	for (i = 0; i < ARRAY_SIZE(ksz9477_regmap_config); i++) {
+		dev->regmap[i] = devm_regmap_init_spi(spi,
+					&ksz9477_regmap_config[i]);
+		if (IS_ERR(dev->regmap[i])) {
+			ret = PTR_ERR(dev->regmap[i]);
+			dev_err(&spi->dev,
+				"Failed to initialize regmap%i: %d\n",
+				ksz9477_regmap_config[i].val_bits, ret);
+			return ret;
+		}
+	}
+
 	if (spi->dev.platform_data)
 		dev->pdata = spi->dev.platform_data;
 
-	dev->txbuf = devm_kzalloc(dev->dev, 4 + SPI_TX_BUF_LEN, GFP_KERNEL);
-
 	ret = ksz9477_switch_register(dev);
 
 	/* Main DSA driver may not be started yet. */
diff --git a/drivers/net/dsa/microchip/ksz_priv.h b/drivers/net/dsa/microchip/ksz_priv.h
index c3a272505af1..883458fd0c6b 100644
--- a/drivers/net/dsa/microchip/ksz_priv.h
+++ b/drivers/net/dsa/microchip/ksz_priv.h
@@ -56,6 +56,7 @@ struct ksz_device {
 	const struct ksz_dev_ops *dev_ops;
 
 	struct device *dev;
+	struct regmap *regmap[3];
 
 	void *priv;
 
@@ -81,8 +82,6 @@ struct ksz_device {
 
 	u64 mib_value[TOTAL_SWITCH_COUNTER_NUM];
 
-	u8 *txbuf;
-
 	struct ksz_port *ports;
 	struct timer_list mib_read_timer;
 	struct work_struct mib_read;
-- 
2.19.2

[RFT][PATCH V2 08/10] net: dsa: microchip: Dispose of ksz_io_ops

[Marek Vasut]

Since the driver now uses regmap , get rid of ad-hoc ksz_io_ops
abstraction, which no longer has any meaning. Moreover, since regmap
has it's own locking, get rid of the register access mutex.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Tristram Ha <Tristram.Ha@microchip.com>
Cc: Woojung Huh <Woojung.Huh@microchip.com>
---
V2: Use separate regmaps for 8/16/32bit registers
---
 drivers/net/dsa/microchip/ksz9477_spi.c | 57 +------------------------
 drivers/net/dsa/microchip/ksz_common.c  |  6 +--
 drivers/net/dsa/microchip/ksz_common.h  | 50 ++++++----------------
 drivers/net/dsa/microchip/ksz_priv.h    | 16 +------
 4 files changed, 17 insertions(+), 112 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477_spi.c b/drivers/net/dsa/microchip/ksz9477_spi.c
index b624c496993e..b440641f4898 100644
--- a/drivers/net/dsa/microchip/ksz9477_spi.c
+++ b/drivers/net/dsa/microchip/ksz9477_spi.c
@@ -45,67 +45,12 @@ static const struct regmap_config ksz9477_regmap_config[] = {
 	KSZ_REGMAP_COMMON(32),
 };
 
-static int ksz_spi_read8(struct ksz_device *dev, u32 reg, u8 *val)
-{
-	unsigned int value;
-	int ret = regmap_read(dev->regmap, reg, &value);
-
-	*val = value;
-	return ret;
-}
-
-static int ksz_spi_read16(struct ksz_device *dev, u32 reg, u16 *val)
-{
-	int ret = regmap_bulk_read(dev->regmap, reg, val, 2);
-
-	if (!ret)
-		*val = be16_to_cpu(*val);
-
-	return ret;
-}
-
-static int ksz_spi_read32(struct ksz_device *dev, u32 reg, u32 *val)
-{
-	int ret = regmap_bulk_read(dev->regmap, reg, val, 4);
-
-	if (!ret)
-		*val = be32_to_cpu(*val);
-
-	return ret;
-}
-
-static int ksz_spi_write8(struct ksz_device *dev, u32 reg, u8 value)
-{
-	return regmap_write(dev->regmap, reg, value);
-}
-
-static int ksz_spi_write16(struct ksz_device *dev, u32 reg, u16 value)
-{
-	value = cpu_to_be16(value);
-	return regmap_bulk_write(dev->regmap, reg, &value, 2);
-}
-
-static int ksz_spi_write32(struct ksz_device *dev, u32 reg, u32 value)
-{
-	value = cpu_to_be32(value);
-	return regmap_bulk_write(dev->regmap, reg, &value, 4);
-}
-
-static const struct ksz_io_ops ksz9477_spi_ops = {
-	.read8 = ksz_spi_read8,
-	.read16 = ksz_spi_read16,
-	.read32 = ksz_spi_read32,
-	.write8 = ksz_spi_write8,
-	.write16 = ksz_spi_write16,
-	.write32 = ksz_spi_write32,
-};
-
 static int ksz9477_spi_probe(struct spi_device *spi)
 {
 	struct ksz_device *dev;
 	int i, ret;
 
-	dev = ksz_switch_alloc(&spi->dev, &ksz9477_spi_ops, spi);
+	dev = ksz_switch_alloc(&spi->dev, spi);
 	if (!dev)
 		return -ENOMEM;
 
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 3b12e2dcff31..c78afd5e3d4b 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -262,9 +262,7 @@ void ksz_disable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
 }
 EXPORT_SYMBOL_GPL(ksz_disable_port);
 
-struct ksz_device *ksz_switch_alloc(struct device *base,
-				    const struct ksz_io_ops *ops,
-				    void *priv)
+struct ksz_device *ksz_switch_alloc(struct device *base, void *priv)
 {
 	struct dsa_switch *ds;
 	struct ksz_device *swdev;
@@ -282,7 +280,6 @@ struct ksz_device *ksz_switch_alloc(struct device *base,
 
 	swdev->ds = ds;
 	swdev->priv = priv;
-	swdev->ops = ops;
 
 	return swdev;
 }
@@ -307,7 +304,6 @@ int ksz_switch_register(struct ksz_device *dev,
 		gpiod_set_value(dev->reset_gpio, 0);
 	}
 
-	mutex_init(&dev->reg_mutex);
 	mutex_init(&dev->stats_mutex);
 	mutex_init(&dev->alu_mutex);
 	mutex_init(&dev->vlan_mutex);
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 9ce91fb03efe..903e3e39bfd4 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -7,6 +7,8 @@
 #ifndef __KSZ_COMMON_H
 #define __KSZ_COMMON_H
 
+#include <linux/regmap.h>
+
 void ksz_update_port_member(struct ksz_device *dev, int port);
 
 /* Common DSA access functions */
@@ -36,68 +38,44 @@ void ksz_disable_port(struct dsa_switch *ds, int port, struct phy_device *phy);
 
 static inline int ksz_read8(struct ksz_device *dev, u32 reg, u8 *val)
 {
-	int ret;
-
-	mutex_lock(&dev->reg_mutex);
-	ret = dev->ops->read8(dev, reg, val);
-	mutex_unlock(&dev->reg_mutex);
+	unsigned int value;
+	int ret = regmap_read(dev->regmap[0], reg, &value);
 
+	*val = value;
 	return ret;
 }
 
 static inline int ksz_read16(struct ksz_device *dev, u32 reg, u16 *val)
 {
-	int ret;
-
-	mutex_lock(&dev->reg_mutex);
-	ret = dev->ops->read16(dev, reg, val);
-	mutex_unlock(&dev->reg_mutex);
+	unsigned int value;
+	int ret = regmap_read(dev->regmap[1], reg, &value);
 
+	*val = value;
 	return ret;
 }
 
 static inline int ksz_read32(struct ksz_device *dev, u32 reg, u32 *val)
 {
-	int ret;
-
-	mutex_lock(&dev->reg_mutex);
-	ret = dev->ops->read32(dev, reg, val);
-	mutex_unlock(&dev->reg_mutex);
+	unsigned int value;
+	int ret = regmap_read(dev->regmap[2], reg, &value);
 
+	*val = value;
 	return ret;
 }
 
 static inline int ksz_write8(struct ksz_device *dev, u32 reg, u8 value)
 {
-	int ret;
-
-	mutex_lock(&dev->reg_mutex);
-	ret = dev->ops->write8(dev, reg, value);
-	mutex_unlock(&dev->reg_mutex);
-
-	return ret;
+	return regmap_write(dev->regmap[0], reg, value);
 }
 
 static inline int ksz_write16(struct ksz_device *dev, u32 reg, u16 value)
 {
-	int ret;
-
-	mutex_lock(&dev->reg_mutex);
-	ret = dev->ops->write16(dev, reg, value);
-	mutex_unlock(&dev->reg_mutex);
-
-	return ret;
+	return regmap_write(dev->regmap[1], reg, value);
 }
 
 static inline int ksz_write32(struct ksz_device *dev, u32 reg, u32 value)
 {
-	int ret;
-
-	mutex_lock(&dev->reg_mutex);
-	ret = dev->ops->write32(dev, reg, value);
-	mutex_unlock(&dev->reg_mutex);
-
-	return ret;
+	return regmap_write(dev->regmap[2], reg, value);
 }
 
 static inline void ksz_pread8(struct ksz_device *dev, int port, int offset,
diff --git a/drivers/net/dsa/microchip/ksz_priv.h b/drivers/net/dsa/microchip/ksz_priv.h
index 883458fd0c6b..cfd5a5b6ae95 100644
--- a/drivers/net/dsa/microchip/ksz_priv.h
+++ b/drivers/net/dsa/microchip/ksz_priv.h
@@ -16,8 +16,6 @@
 
 #include "ksz9477_reg.h"
 
-struct ksz_io_ops;
-
 struct vlan_table {
 	u32 table[3];
 };
@@ -48,11 +46,9 @@ struct ksz_device {
 	struct ksz_platform_data *pdata;
 	const char *name;
 
-	struct mutex reg_mutex;		/* register access */
 	struct mutex stats_mutex;	/* status access */
 	struct mutex alu_mutex;		/* ALU access */
 	struct mutex vlan_mutex;	/* vlan access */
-	const struct ksz_io_ops *ops;
 	const struct ksz_dev_ops *dev_ops;
 
 	struct device *dev;
@@ -100,15 +96,6 @@ struct ksz_device {
 	u16 port_mask;
 };
 
-struct ksz_io_ops {
-	int (*read8)(struct ksz_device *dev, u32 reg, u8 *value);
-	int (*read16)(struct ksz_device *dev, u32 reg, u16 *value);
-	int (*read32)(struct ksz_device *dev, u32 reg, u32 *value);
-	int (*write8)(struct ksz_device *dev, u32 reg, u8 value);
-	int (*write16)(struct ksz_device *dev, u32 reg, u16 value);
-	int (*write32)(struct ksz_device *dev, u32 reg, u32 value);
-};
-
 struct alu_struct {
 	/* entry 1 */
 	u8	is_static:1;
@@ -153,8 +140,7 @@ struct ksz_dev_ops {
 	void (*exit)(struct ksz_device *dev);
 };
 
-struct ksz_device *ksz_switch_alloc(struct device *base,
-				    const struct ksz_io_ops *ops, void *priv);
+struct ksz_device *ksz_switch_alloc(struct device *base, void *priv);
 int ksz_switch_register(struct ksz_device *dev,
 			const struct ksz_dev_ops *ops);
 void ksz_switch_remove(struct ksz_device *dev);
-- 
2.19.2

[RFT][PATCH V2 09/10] net: dsa: microchip: Factor out regmap config generation into common header

[Marek Vasut]

The regmap config tables are rather similar for various generations of
the KSZ8xxx/KSZ9xxx switches. Introduce a macro which allows generating
those tables without duplication. Note that $regalign parameter is not
used right now, but will be used in KSZ87xx series switches.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Tristram Ha <Tristram.Ha@microchip.com>
Cc: Woojung Huh <Woojung.Huh@microchip.com>
---
V2: New patch
---
 drivers/net/dsa/microchip/ksz9477_spi.c | 28 ++---------------------
 drivers/net/dsa/microchip/ksz_common.h  | 30 +++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 26 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477_spi.c b/drivers/net/dsa/microchip/ksz9477_spi.c
index b440641f4898..29b51524cae9 100644
--- a/drivers/net/dsa/microchip/ksz9477_spi.c
+++ b/drivers/net/dsa/microchip/ksz9477_spi.c
@@ -14,36 +14,12 @@
 #include <linux/spi/spi.h>
 
 #include "ksz_priv.h"
+#include "ksz_common.h"
 
 #define SPI_ADDR_SHIFT			24
 #define SPI_TURNAROUND_SHIFT		5
 
-/* SPI frame opcodes */
-#define KS_SPIOP_RD			3
-#define KS_SPIOP_WR			2
-
-#define KS_SPIOP_FLAG_MASK(opcode)		\
-	cpu_to_be16((opcode) << (SPI_ADDR_SHIFT + SPI_TURNAROUND_SHIFT))
-
-#define KSZ_REGMAP_COMMON(width)					\
-	{								\
-		.val_bits = (width),					\
-		.reg_stride = (width) / 8,				\
-		.reg_bits = SPI_ADDR_SHIFT,				\
-		.pad_bits = SPI_TURNAROUND_SHIFT,			\
-		.max_register = 0xF00,					\
-		.cache_type = REGCACHE_NONE,				\
-		.read_flag_mask = KS_SPIOP_FLAG_MASK(KS_SPIOP_RD),	\
-		.write_flag_mask = KS_SPIOP_FLAG_MASK(KS_SPIOP_WR),	\
-		.reg_format_endian = REGMAP_ENDIAN_BIG,			\
-		.val_format_endian = REGMAP_ENDIAN_BIG			\
-	}
-
-static const struct regmap_config ksz9477_regmap_config[] = {
-	KSZ_REGMAP_COMMON(8),
-	KSZ_REGMAP_COMMON(16),
-	KSZ_REGMAP_COMMON(32),
-};
+KSZ_REGMAP_TABLE(ksz9477, SPI_ADDR_SHIFT, SPI_TURNAROUND_SHIFT, 0);
 
 static int ksz9477_spi_probe(struct spi_device *spi)
 {
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 903e3e39bfd4..4d30a67c14a3 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -114,4 +114,34 @@ static inline void ksz_pwrite32(struct ksz_device *dev, int port, int offset,
 	ksz_write32(dev, dev->dev_ops->get_port_addr(port, offset), data);
 }
 
+/* Regmap tables generation */
+#define KSZ_SPI_OP_RD		3
+#define KSZ_SPI_OP_WR		2
+
+#define KSZ_SPI_OP_FLAG_MASK(opcode, regbits, regpad)			\
+	cpu_to_be16((opcode) << ((regbits) + (regpad)))
+
+#define KSZ_REGMAP_ENTRY(width, regbits, regpad, regalign)		\
+	{								\
+		.val_bits = (width),					\
+		.reg_stride = (width) / 8,				\
+		.reg_bits = (regbits) + (regalign),			\
+		.pad_bits = (regpad),					\
+		.max_register = 0xF00,					\
+		.cache_type = REGCACHE_NONE,				\
+		.read_flag_mask =					\
+			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_RD, regbits, regpad), \
+		.write_flag_mask =					\
+			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_WR, regbits, regpad), \
+		.reg_format_endian = REGMAP_ENDIAN_BIG,			\
+		.val_format_endian = REGMAP_ENDIAN_BIG			\
+	}
+
+#define KSZ_REGMAP_TABLE(ksz, regbits, regpad, regalign)		\
+	static const struct regmap_config ksz##_regmap_config[] = {	\
+		KSZ_REGMAP_ENTRY(8, (regbits), (regpad), (regalign)),	\
+		KSZ_REGMAP_ENTRY(16, (regbits), (regpad), (regalign)),	\
+		KSZ_REGMAP_ENTRY(32, (regbits), (regpad), (regalign)),	\
+	}
+
 #endif
-- 
2.19.2

[RFT][PATCH V2 10/10] net: dsa: microchip: Replace ad-hoc bit manipulation with regmap

[Marek Vasut]

Regmap provides bit manipulation functions to set/clear bits, use those
insted of reimplementing them.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Tristram Ha <Tristram.Ha@microchip.com>
Cc: Woojung Huh <Woojung.Huh@microchip.com>
---
V2: New patch
---
 drivers/net/dsa/microchip/ksz9477.c | 46 ++++-------------------------
 1 file changed, 6 insertions(+), 40 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 5e720a9e3659..ba3371f2ab0c 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -65,60 +65,26 @@ static const struct {
 
 static void ksz_cfg(struct ksz_device *dev, u32 addr, u8 bits, bool set)
 {
-	u8 data;
-
-	ksz_read8(dev, addr, &data);
-	if (set)
-		data |= bits;
-	else
-		data &= ~bits;
-	ksz_write8(dev, addr, data);
+	regmap_update_bits(dev->regmap[0], addr, bits, set ? bits : 0);
 }
 
 static void ksz_port_cfg(struct ksz_device *dev, int port, int offset, u8 bits,
 			 bool set)
 {
-	u32 addr;
-	u8 data;
-
-	addr = PORT_CTRL_ADDR(port, offset);
-	ksz_read8(dev, addr, &data);
-
-	if (set)
-		data |= bits;
-	else
-		data &= ~bits;
-
-	ksz_write8(dev, addr, data);
+	regmap_update_bits(dev->regmap[0], PORT_CTRL_ADDR(port, offset),
+			   bits, set ? bits : 0);
 }
 
 static void ksz9477_cfg32(struct ksz_device *dev, u32 addr, u32 bits, bool set)
 {
-	u32 data;
-
-	ksz_read32(dev, addr, &data);
-	if (set)
-		data |= bits;
-	else
-		data &= ~bits;
-	ksz_write32(dev, addr, data);
+	regmap_update_bits(dev->regmap[2], addr, bits, set ? bits : 0);
 }
 
 static void ksz9477_port_cfg32(struct ksz_device *dev, int port, int offset,
 			       u32 bits, bool set)
 {
-	u32 addr;
-	u32 data;
-
-	addr = PORT_CTRL_ADDR(port, offset);
-	ksz_read32(dev, addr, &data);
-
-	if (set)
-		data |= bits;
-	else
-		data &= ~bits;
-
-	ksz_write32(dev, addr, data);
+	regmap_update_bits(dev->regmap[2], PORT_CTRL_ADDR(port, offset),
+			   bits, set ? bits : 0);
 }
 
 static int ksz9477_wait_vlan_ctrl_ready(struct ksz_device *dev, u32 waiton,
-- 
2.19.2

RE: [RFT][PATCH V2 09/10] net: dsa: microchip: Factor out regmap config generation into common header

[Tristram.Ha]

> +	{								\
> +		.val_bits = (width),					\
> +		.reg_stride = (width) / 8,				\
> +		.reg_bits = (regbits) + (regalign),			\
> +		.pad_bits = (regpad),					\
> +		.max_register = 0xF00,					\
> +		.cache_type = REGCACHE_NONE,
> 	\
> +		.read_flag_mask =					\
> +			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_RD, regbits,
> regpad), \
> +		.write_flag_mask =					\
> +			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_WR, regbits,
> regpad), \
> +		.reg_format_endian = REGMAP_ENDIAN_BIG,
> 	\
> +		.val_format_endian = REGMAP_ENDIAN_BIG
> 	\
> +	}

max_registers for KSZ9477 should be 0x8000.

I found that the SPI access works with these settings:
reg_bits = 32 - 5, val_bits = 8, pad_bits = 5, read_flag_mask = KS_SPIOP_RD << 5.

I am using this in another driver running in 4.9.  Somehow the 8-bit write causes the
hardware to be in a wrong state.  This is a quick change so there may be bugs in the
driver.

As expected the access speed is a few microseconds slower than before.

Re: [RFT][PATCH V2 09/10] net: dsa: microchip: Factor out regmap config generation into common header

[Marek Vasut]

On 12/21/2018 05:16 AM, Tristram.Ha@microchip.com wrote:
>> +	{								\
>> +		.val_bits = (width),					\
>> +		.reg_stride = (width) / 8,				\
>> +		.reg_bits = (regbits) + (regalign),			\
>> +		.pad_bits = (regpad),					\
>> +		.max_register = 0xF00,					\
>> +		.cache_type = REGCACHE_NONE,
>> 	\
>> +		.read_flag_mask =					\
>> +			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_RD, regbits,
>> regpad), \
>> +		.write_flag_mask =					\
>> +			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_WR, regbits,
>> regpad), \
>> +		.reg_format_endian = REGMAP_ENDIAN_BIG,
>> 	\
>> +		.val_format_endian = REGMAP_ENDIAN_BIG
>> 	\
>> +	}
> 
> max_registers for KSZ9477 should be 0x8000.
> 
> I found that the SPI access works with these settings:
> reg_bits = 32 - 5, val_bits = 8, pad_bits = 5, read_flag_mask = KS_SPIOP_RD << 5.

This is wrong, val_bits must match the register width (8 for 8bit
regmap, 16 for 16bit regmap etc). Anyway, can you prepare a short diff
to give me an idea what needs to be changed ?

> I am using this in another driver running in 4.9.  Somehow the 8-bit write causes the
> hardware to be in a wrong state.  This is a quick change so there may be bugs in the
> driver.

Can you try this as-is on linux-next or net-next ?

> As expected the access speed is a few microseconds slower than before.

Why is it expected ?

-- 
Best regards,
Marek Vasut

Re: [RFT][PATCH V2 09/10] net: dsa: microchip: Factor out regmap config generation into common header

[Marek Vasut]

On 12/21/18 6:23 AM, Marek Vasut wrote:
> On 12/21/2018 05:16 AM, Tristram.Ha@microchip.com wrote:
>>> +	{								\
>>> +		.val_bits = (width),					\
>>> +		.reg_stride = (width) / 8,				\
>>> +		.reg_bits = (regbits) + (regalign),			\
>>> +		.pad_bits = (regpad),					\
>>> +		.max_register = 0xF00,					\
>>> +		.cache_type = REGCACHE_NONE,
>>> 	\
>>> +		.read_flag_mask =					\
>>> +			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_RD, regbits,
>>> regpad), \
>>> +		.write_flag_mask =					\
>>> +			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_WR, regbits,
>>> regpad), \
>>> +		.reg_format_endian = REGMAP_ENDIAN_BIG,
>>> 	\
>>> +		.val_format_endian = REGMAP_ENDIAN_BIG
>>> 	\
>>> +	}
>>
>> max_registers for KSZ9477 should be 0x8000.
>>
>> I found that the SPI access works with these settings:
>> reg_bits = 32 - 5, val_bits = 8, pad_bits = 5, read_flag_mask = KS_SPIOP_RD << 5.
> 
> This is wrong, val_bits must match the register width (8 for 8bit
> regmap, 16 for 16bit regmap etc). Anyway, can you prepare a short diff
> to give me an idea what needs to be changed ?
> 
>> I am using this in another driver running in 4.9.  Somehow the 8-bit write causes the
>> hardware to be in a wrong state.  This is a quick change so there may be bugs in the
>> driver.
> 
> Can you try this as-is on linux-next or net-next ?
> 
>> As expected the access speed is a few microseconds slower than before.
> 
> Why is it expected ?

Bump ?

-- 
Best regards,
Marek Vasut

RE: [RFT][PATCH V2 09/10] net: dsa: microchip: Factor out regmap config generation into common header

[Tristram.Ha]

> >>> +	{								\
> >>> +		.val_bits = (width),					\
> >>> +		.reg_stride = (width) / 8,				\
> >>> +		.reg_bits = (regbits) + (regalign),			\
> >>> +		.pad_bits = (regpad),					\
> >>> +		.max_register = 0xF00,					\
> >>> +		.cache_type = REGCACHE_NONE,
> >>> 	\
> >>> +		.read_flag_mask =					\
> >>> +			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_RD, regbits,
> >>> regpad), \
> >>> +		.write_flag_mask =					\
> >>> +			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_WR, regbits,
> >>> regpad), \
> >>> +		.reg_format_endian = REGMAP_ENDIAN_BIG,
> >>> 	\
> >>> +		.val_format_endian = REGMAP_ENDIAN_BIG
> >>> 	\
> >>> +	}
> >>
> >> max_registers for KSZ9477 should be 0x8000.
> >>
> >> I found that the SPI access works with these settings:
> >> reg_bits = 32 - 5, val_bits = 8, pad_bits = 5, read_flag_mask =
> KS_SPIOP_RD << 5.
> >
> > This is wrong, val_bits must match the register width (8 for 8bit
> > regmap, 16 for 16bit regmap etc). Anyway, can you prepare a short diff
> > to give me an idea what needs to be changed ?
> >
> 
> Bump ?
> 

This is the regmap_config I used in Linux 4.9:

	.reg_bits		= SPI_REGMAP_REG,
	.val_bits		= SPI_REGMAP_VAL,
	.pad_bits		= SPI_REGMAP_PAD,
	.read_flag_mask	= KS_SPIOP_RD << SPI_REGMAP_MASK_S,
	.write_flag_mask	= KS_SPIOP_WR << SPI_REGMAP_MASK_S,
	.reg_format_endian	= REGMAP_ENDIAN_BIG,
	.val_format_endian	= REGMAP_ENDIAN_BIG,

For KSZ9477:

SPI_CMD_LEN		4
SPI_REGMAP_PAD	SPI_TURNAROUND_SHIFT
SPI_REGMAP_VAL	8
SPI_REGMAP_REG	\
	(SPI_CMD_LEN * SPI_REGMAP_VAL - SPI_TURNAROUND_SHIFT)
SPI_REGMAP_MASK_S	\
	(SPI_ADDR_SHIFT + SPI_TURNAROUND_SHIFT - \
	(SPI_CMD_LEN * SPI_REGMAP_VAL - 8))

For KSZ8795:

SPI_CMD_LEN		2
SPI_REGMAP_PAD	SPI_TURNAROUND_S
SPI_REGMAP_VAL	8
SPI_REGMAP_REG	\
	(SPI_CMD_LEN * SPI_REGMAP_VAL - SPI_TURNAROUND_S)
SPI_REGMAP_MASK_S	\
	(SPI_ADDR_S + SPI_TURNAROUND_S - \
	(SPI_CMD_LEN * SPI_REGMAP_VAL - 8))

So the differences between KSZ9477 and KSZ8795 are SPI_CMD_LEN, SPI_ADDR_S, and SPI_TURNAROUND_S.

KSZ9477:

.reg_bits = 32 - 5 = 27
.val_bits = 8
.pad_bits = 5
.read_flag_mask = KS_SPIOP_RD << 5,

KSZ8795:

.reg_bits = 16 - 1 = 15
.val_bits = 8
.pad_bits = 1
.read_flag_mask = KS_SPIOP_RD << 5,

The regmap.c code uses reg_bits + reg_shift (which comes from pad_bits) to decide whether the register space is 16-bit or 32-bit.  The value space is always 8-bit.

The shift for _flag_mask turns out to be the same for both KSZ9477 and KSZ8795.

RE: [RFT][PATCH V2 09/10] net: dsa: microchip: Factor out regmap config generation into common header

[Tristram.Ha]

> This is the regmap_config I used in Linux 4.9:
> 
> 	.reg_bits		= SPI_REGMAP_REG,
> 	.val_bits		= SPI_REGMAP_VAL,
> 	.pad_bits		= SPI_REGMAP_PAD,
> 	.read_flag_mask	= KS_SPIOP_RD << SPI_REGMAP_MASK_S,
> 	.write_flag_mask	= KS_SPIOP_WR << SPI_REGMAP_MASK_S,
> 	.reg_format_endian	= REGMAP_ENDIAN_BIG,
> 	.val_format_endian	= REGMAP_ENDIAN_BIG,
> 
> For KSZ9477:
> 
> SPI_CMD_LEN		4
> SPI_REGMAP_PAD	SPI_TURNAROUND_SHIFT
> SPI_REGMAP_VAL	8
> SPI_REGMAP_REG	\
> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - SPI_TURNAROUND_SHIFT)
> SPI_REGMAP_MASK_S	\
> 	(SPI_ADDR_SHIFT + SPI_TURNAROUND_SHIFT - \
> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - 8))
> 
> For KSZ8795:
> 
> SPI_CMD_LEN		2
> SPI_REGMAP_PAD	SPI_TURNAROUND_S
> SPI_REGMAP_VAL	8
> SPI_REGMAP_REG	\
> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - SPI_TURNAROUND_S)
> SPI_REGMAP_MASK_S	\
> 	(SPI_ADDR_S + SPI_TURNAROUND_S - \
> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - 8))
> 
> So the differences between KSZ9477 and KSZ8795 are SPI_CMD_LEN,
> SPI_ADDR_S, and SPI_TURNAROUND_S.
> 
> KSZ9477:
> 
> .reg_bits = 32 - 5 = 27
> .val_bits = 8
> .pad_bits = 5
> .read_flag_mask = KS_SPIOP_RD << 5,
> 
> KSZ8795:
> 
> .reg_bits = 16 - 1 = 15
> .val_bits = 8
> .pad_bits = 1
> .read_flag_mask = KS_SPIOP_RD << 5,
> 
> The regmap.c code uses reg_bits + reg_shift (which comes from pad_bits) to
> decide whether the register space is 16-bit or 32-bit.  The value space is
> always 8-bit.
> 
> The shift for _flag_mask turns out to be the same for both KSZ9477 and
> KSZ8795.

I just looked at your regmap code and you use 3 regmap pointers for specific 8-bit, 16-bit, and 32-bit accesses.  The switch access is always 8-bit.  It has automatic register increment so that you can access arbitrary length of registers.  The use of 16-bit and 32-bit accesses makes access efficient if it makes sense.

Most older switches define registers in 8-bit.  Exceptions are the default VID and indirect access.

A specific switch mostly defines registers in 16-bit because it shares the core design with an Ethernet controller.

KSZ9477 is the newer designed switch and it gets some designs from older switches and that is why it has a mix of 8-bit, 16-bit, and 32-bit register definitions.

In my code I just use regmap_bulk_read and regmap_bulk_write and still use the old spi access functions for specific 8-bit, 16-bit, and 32-bit accesses.

We can combine the logic of ksz_spi_read8 and others into ksz_read8 and so they can be used for both SPI and I2C accesses.

Re: [RFT][PATCH V2 09/10] net: dsa: microchip: Factor out regmap config generation into common header

[Marek Vasut]

On 1/9/19 8:08 PM, Tristram.Ha@microchip.com wrote:
>>>>> +	{								\
>>>>> +		.val_bits = (width),					\
>>>>> +		.reg_stride = (width) / 8,				\
>>>>> +		.reg_bits = (regbits) + (regalign),			\
>>>>> +		.pad_bits = (regpad),					\
>>>>> +		.max_register = 0xF00,					\
>>>>> +		.cache_type = REGCACHE_NONE,
>>>>> 	\
>>>>> +		.read_flag_mask =					\
>>>>> +			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_RD, regbits,
>>>>> regpad), \
>>>>> +		.write_flag_mask =					\
>>>>> +			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_WR, regbits,
>>>>> regpad), \
>>>>> +		.reg_format_endian = REGMAP_ENDIAN_BIG,
>>>>> 	\
>>>>> +		.val_format_endian = REGMAP_ENDIAN_BIG
>>>>> 	\
>>>>> +	}
>>>>
>>>> max_registers for KSZ9477 should be 0x8000.
>>>>
>>>> I found that the SPI access works with these settings:
>>>> reg_bits = 32 - 5, val_bits = 8, pad_bits = 5, read_flag_mask =
>> KS_SPIOP_RD << 5.
>>>
>>> This is wrong, val_bits must match the register width (8 for 8bit
>>> regmap, 16 for 16bit regmap etc). Anyway, can you prepare a short diff
>>> to give me an idea what needs to be changed ?
>>>
>>
>> Bump ?
>>
> 
> This is the regmap_config I used in Linux 4.9:
> 
> 	.reg_bits		= SPI_REGMAP_REG,
> 	.val_bits		= SPI_REGMAP_VAL,
> 	.pad_bits		= SPI_REGMAP_PAD,
> 	.read_flag_mask	= KS_SPIOP_RD << SPI_REGMAP_MASK_S,
> 	.write_flag_mask	= KS_SPIOP_WR << SPI_REGMAP_MASK_S,
> 	.reg_format_endian	= REGMAP_ENDIAN_BIG,
> 	.val_format_endian	= REGMAP_ENDIAN_BIG,
> 
> For KSZ9477:
> 
> SPI_CMD_LEN		4
> SPI_REGMAP_PAD	SPI_TURNAROUND_SHIFT
> SPI_REGMAP_VAL	8
> SPI_REGMAP_REG	\
> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - SPI_TURNAROUND_SHIFT)
> SPI_REGMAP_MASK_S	\
> 	(SPI_ADDR_SHIFT + SPI_TURNAROUND_SHIFT - \
> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - 8))
> 
> For KSZ8795:
> 
> SPI_CMD_LEN		2
> SPI_REGMAP_PAD	SPI_TURNAROUND_S
> SPI_REGMAP_VAL	8
> SPI_REGMAP_REG	\
> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - SPI_TURNAROUND_S)
> SPI_REGMAP_MASK_S	\
> 	(SPI_ADDR_S + SPI_TURNAROUND_S - \
> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - 8))
> 
> So the differences between KSZ9477 and KSZ8795 are SPI_CMD_LEN, SPI_ADDR_S, and SPI_TURNAROUND_S.
> 
> KSZ9477:
> 
> .reg_bits = 32 - 5 = 27
> .val_bits = 8
> .pad_bits = 5
> .read_flag_mask = KS_SPIOP_RD << 5,
> 
> KSZ8795:
> 
> .reg_bits = 16 - 1 = 15
> .val_bits = 8
> .pad_bits = 1
> .read_flag_mask = KS_SPIOP_RD << 5,
> 
> The regmap.c code uses reg_bits + reg_shift (which comes from pad_bits) to decide whether the register space is 16-bit or 32-bit.  The value space is always 8-bit.
> 
> The shift for _flag_mask turns out to be the same for both KSZ9477 and KSZ8795.

Can you prepare a patch ? One which can apply on top of this series , so
we get the authorship information and stuff ?

Also, can you test this on net-next or next ?

-- 
Best regards,
Marek Vasut

Re: [RFT][PATCH V2 09/10] net: dsa: microchip: Factor out regmap config generation into common header

[Marek Vasut]

On 1/9/19 8:58 PM, Tristram.Ha@microchip.com wrote:
>> This is the regmap_config I used in Linux 4.9:
>>
>> 	.reg_bits		= SPI_REGMAP_REG,
>> 	.val_bits		= SPI_REGMAP_VAL,
>> 	.pad_bits		= SPI_REGMAP_PAD,
>> 	.read_flag_mask	= KS_SPIOP_RD << SPI_REGMAP_MASK_S,
>> 	.write_flag_mask	= KS_SPIOP_WR << SPI_REGMAP_MASK_S,
>> 	.reg_format_endian	= REGMAP_ENDIAN_BIG,
>> 	.val_format_endian	= REGMAP_ENDIAN_BIG,
>>
>> For KSZ9477:
>>
>> SPI_CMD_LEN		4
>> SPI_REGMAP_PAD	SPI_TURNAROUND_SHIFT
>> SPI_REGMAP_VAL	8
>> SPI_REGMAP_REG	\
>> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - SPI_TURNAROUND_SHIFT)
>> SPI_REGMAP_MASK_S	\
>> 	(SPI_ADDR_SHIFT + SPI_TURNAROUND_SHIFT - \
>> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - 8))
>>
>> For KSZ8795:
>>
>> SPI_CMD_LEN		2
>> SPI_REGMAP_PAD	SPI_TURNAROUND_S
>> SPI_REGMAP_VAL	8
>> SPI_REGMAP_REG	\
>> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - SPI_TURNAROUND_S)
>> SPI_REGMAP_MASK_S	\
>> 	(SPI_ADDR_S + SPI_TURNAROUND_S - \
>> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - 8))
>>
>> So the differences between KSZ9477 and KSZ8795 are SPI_CMD_LEN,
>> SPI_ADDR_S, and SPI_TURNAROUND_S.
>>
>> KSZ9477:
>>
>> .reg_bits = 32 - 5 = 27
>> .val_bits = 8
>> .pad_bits = 5
>> .read_flag_mask = KS_SPIOP_RD << 5,
>>
>> KSZ8795:
>>
>> .reg_bits = 16 - 1 = 15
>> .val_bits = 8
>> .pad_bits = 1
>> .read_flag_mask = KS_SPIOP_RD << 5,
>>
>> The regmap.c code uses reg_bits + reg_shift (which comes from pad_bits) to
>> decide whether the register space is 16-bit or 32-bit.  The value space is
>> always 8-bit.
>>
>> The shift for _flag_mask turns out to be the same for both KSZ9477 and
>> KSZ8795.
> 
> I just looked at your regmap code and you use 3 regmap pointers for specific 8-bit, 16-bit, and 32-bit accesses.  The switch access is always 8-bit.  It has automatic register increment so that you can access arbitrary length of registers.  The use of 16-bit and 32-bit accesses makes access efficient if it makes sense.

Right, that's what happens here.

> Most older switches define registers in 8-bit.  Exceptions are the default VID and indirect access.
> 
> A specific switch mostly defines registers in 16-bit because it shares the core design with an Ethernet controller.
> 
> KSZ9477 is the newer designed switch and it gets some designs from older switches and that is why it has a mix of 8-bit, 16-bit, and 32-bit register definitions.

Right, that's quite horrible.

> In my code I just use regmap_bulk_read and regmap_bulk_write and still use the old spi access functions for specific 8-bit, 16-bit, and 32-bit accesses.

Let's not mix regmap and non-regmap accesses, that'd be a mess. Let's
stick to one, regmap.

> We can combine the logic of ksz_spi_read8 and others into ksz_read8 and so they can be used for both SPI and I2C accesses.

You can just use regmap_*() accessors and regmap will deal with i2c/spi
abstraction for you, that's the idea.

-- 
Best regards,
Marek Vasut

RE: [RFT][PATCH V2 09/10] net: dsa: microchip: Factor out regmap config generation into common header

[Tristram.Ha]

> > I just looked at your regmap code and you use 3 regmap pointers for
> specific 8-bit, 16-bit, and 32-bit accesses.  The switch access is always 8-bit.  It
> has automatic register increment so that you can access arbitrary length of
> registers.  The use of 16-bit and 32-bit accesses makes access efficient if it
> makes sense.
> 
> Right, that's what happens here.
> 
> > Most older switches define registers in 8-bit.  Exceptions are the default
> VID and indirect access.
> >
> > A specific switch mostly defines registers in 16-bit because it shares the
> core design with an Ethernet controller.
> >
> > KSZ9477 is the newer designed switch and it gets some designs from older
> switches and that is why it has a mix of 8-bit, 16-bit, and 32-bit register
> definitions.
> 
> Right, that's quite horrible.
> 
> > In my code I just use regmap_bulk_read and regmap_bulk_write and still
> use the old spi access functions for specific 8-bit, 16-bit, and 32-bit accesses.
> 
> Let's not mix regmap and non-regmap accesses, that'd be a mess. Let's
> stick to one, regmap.
> 
> > We can combine the logic of ksz_spi_read8 and others into ksz_read8 and
> so they can be used for both SPI and I2C accesses.
> 
> You can just use regmap_*() accessors and regmap will deal with i2c/spi
> abstraction for you, that's the idea.
> 

What I meant is I use bulk_read as a base and modify it to access 16-bit and 32-bit instead of using regmap[1] and regmap[2].  We can keep regmap[2] for 32-bit access just for the regmap_update_bits function.

I intend to keep the 3 regmap pointers as a specific switch actually requires those specific accesses as it does not have automatic register increment.

Re: [RFT][PATCH V2 09/10] net: dsa: microchip: Factor out regmap config generation into common header

[Marek Vasut]

On 1/10/19 3:10 AM, Tristram.Ha@microchip.com wrote:
>>> I just looked at your regmap code and you use 3 regmap pointers for
>> specific 8-bit, 16-bit, and 32-bit accesses.  The switch access is always 8-bit.  It
>> has automatic register increment so that you can access arbitrary length of
>> registers.  The use of 16-bit and 32-bit accesses makes access efficient if it
>> makes sense.
>>
>> Right, that's what happens here.
>>
>>> Most older switches define registers in 8-bit.  Exceptions are the default
>> VID and indirect access.
>>>
>>> A specific switch mostly defines registers in 16-bit because it shares the
>> core design with an Ethernet controller.
>>>
>>> KSZ9477 is the newer designed switch and it gets some designs from older
>> switches and that is why it has a mix of 8-bit, 16-bit, and 32-bit register
>> definitions.
>>
>> Right, that's quite horrible.
>>
>>> In my code I just use regmap_bulk_read and regmap_bulk_write and still
>> use the old spi access functions for specific 8-bit, 16-bit, and 32-bit accesses.
>>
>> Let's not mix regmap and non-regmap accesses, that'd be a mess. Let's
>> stick to one, regmap.
>>
>>> We can combine the logic of ksz_spi_read8 and others into ksz_read8 and
>> so they can be used for both SPI and I2C accesses.
>>
>> You can just use regmap_*() accessors and regmap will deal with i2c/spi
>> abstraction for you, that's the idea.
>>
> 
> What I meant is I use bulk_read as a base and modify it to access 16-bit and 32-bit instead of using regmap[1] and regmap[2].  We can keep regmap[2] for 32-bit access just for the regmap_update_bits function.
> 
> I intend to keep the 3 regmap pointers as a specific switch actually requires those specific accesses as it does not have automatic register increment.

I don't think the bulk read is a good idea due to register alignment.
You see, with bulk read, the user might try to read two bytes from the
middle of 4 byte register and I'm not sure how the HW would like that.
If we have 32bit regmap for 32bit registers, all behaves as expected.

-- 
Best regards,
Marek Vasut

RE: [RFT][PATCH V2 09/10] net: dsa: microchip: Factor out regmap config generation into common header

[Tristram.Ha]

> On 1/10/19 3:10 AM, Tristram.Ha@microchip.com wrote:
> >>> I just looked at your regmap code and you use 3 regmap pointers for
> >> specific 8-bit, 16-bit, and 32-bit accesses.  The switch access is always 8-bit.
> It
> >> has automatic register increment so that you can access arbitrary length of
> >> registers.  The use of 16-bit and 32-bit accesses makes access efficient if it
> >> makes sense.
> >>
> >> Right, that's what happens here.
> >>
> >>> Most older switches define registers in 8-bit.  Exceptions are the default
> >> VID and indirect access.
> >>>
> >>> A specific switch mostly defines registers in 16-bit because it shares the
> >> core design with an Ethernet controller.
> >>>
> >>> KSZ9477 is the newer designed switch and it gets some designs from
> older
> >> switches and that is why it has a mix of 8-bit, 16-bit, and 32-bit register
> >> definitions.
> >>
> >> Right, that's quite horrible.
> >>
> >>> In my code I just use regmap_bulk_read and regmap_bulk_write and
> still
> >> use the old spi access functions for specific 8-bit, 16-bit, and 32-bit
> accesses.
> >>
> >> Let's not mix regmap and non-regmap accesses, that'd be a mess. Let's
> >> stick to one, regmap.
> >>
> >>> We can combine the logic of ksz_spi_read8 and others into ksz_read8
> and
> >> so they can be used for both SPI and I2C accesses.
> >>
> >> You can just use regmap_*() accessors and regmap will deal with i2c/spi
> >> abstraction for you, that's the idea.
> >>
> >
> > What I meant is I use bulk_read as a base and modify it to access 16-bit and
> 32-bit instead of using regmap[1] and regmap[2].  We can keep regmap[2]
> for 32-bit access just for the regmap_update_bits function.
> >
> > I intend to keep the 3 regmap pointers as a specific switch actually requires
> those specific accesses as it does not have automatic register increment.
> 
> I don't think the bulk read is a good idea due to register alignment.
> You see, with bulk read, the user might try to read two bytes from the
> middle of 4 byte register and I'm not sure how the HW would like that.
> If we have 32bit regmap for 32bit registers, all behaves as expected.

I just changed bulk_read to raw_read as it is more correct.

The switch access is 8-bit and so there is no restriction on accessing registers.
Of course some registers like PHY registers are better accessed in 16-bit, but you can write the high byte or low byte without problem.

Actually the hardware has some bugs that requires writing in 32-bit for some PHY related registers.  The driver has to make sure to write in the correct way to have the right result.

My point is the driver is the only one who is using these functions to write, so the developer does not try to write the register in the wrong way.

It turns out the switch that requires exact 8-bit, 16-bit, and 32-bit access functions does not work using the regmap mechanism without additional register manipulation, so we do not really need 3 regmap pointers.

One benefit of having 32-bit access is the register dump from debugfs can be much shorter than using 8-bit.

Re: [RFT][PATCH V2 09/10] net: dsa: microchip: Factor out regmap config generation into common header

[Marek Vasut]

On 1/11/19 5:04 AM, Tristram.Ha@microchip.com wrote:
>> On 1/10/19 3:10 AM, Tristram.Ha@microchip.com wrote:
>>>>> I just looked at your regmap code and you use 3 regmap pointers for
>>>> specific 8-bit, 16-bit, and 32-bit accesses.  The switch access is always 8-bit.
>> It
>>>> has automatic register increment so that you can access arbitrary length of
>>>> registers.  The use of 16-bit and 32-bit accesses makes access efficient if it
>>>> makes sense.
>>>>
>>>> Right, that's what happens here.
>>>>
>>>>> Most older switches define registers in 8-bit.  Exceptions are the default
>>>> VID and indirect access.
>>>>>
>>>>> A specific switch mostly defines registers in 16-bit because it shares the
>>>> core design with an Ethernet controller.
>>>>>
>>>>> KSZ9477 is the newer designed switch and it gets some designs from
>> older
>>>> switches and that is why it has a mix of 8-bit, 16-bit, and 32-bit register
>>>> definitions.
>>>>
>>>> Right, that's quite horrible.
>>>>
>>>>> In my code I just use regmap_bulk_read and regmap_bulk_write and
>> still
>>>> use the old spi access functions for specific 8-bit, 16-bit, and 32-bit
>> accesses.
>>>>
>>>> Let's not mix regmap and non-regmap accesses, that'd be a mess. Let's
>>>> stick to one, regmap.
>>>>
>>>>> We can combine the logic of ksz_spi_read8 and others into ksz_read8
>> and
>>>> so they can be used for both SPI and I2C accesses.
>>>>
>>>> You can just use regmap_*() accessors and regmap will deal with i2c/spi
>>>> abstraction for you, that's the idea.
>>>>
>>>
>>> What I meant is I use bulk_read as a base and modify it to access 16-bit and
>> 32-bit instead of using regmap[1] and regmap[2].  We can keep regmap[2]
>> for 32-bit access just for the regmap_update_bits function.
>>>
>>> I intend to keep the 3 regmap pointers as a specific switch actually requires
>> those specific accesses as it does not have automatic register increment.
>>
>> I don't think the bulk read is a good idea due to register alignment.
>> You see, with bulk read, the user might try to read two bytes from the
>> middle of 4 byte register and I'm not sure how the HW would like that.
>> If we have 32bit regmap for 32bit registers, all behaves as expected.
> 
> I just changed bulk_read to raw_read as it is more correct.
> 
> The switch access is 8-bit and so there is no restriction on accessing registers.
> Of course some registers like PHY registers are better accessed in 16-bit, but you can write the high byte or low byte without problem.
> 
> Actually the hardware has some bugs that requires writing in 32-bit for some PHY related registers.  The driver has to make sure to write in the correct way to have the right result.

OK, so there are clearly restrictions to what can be written and how.

> My point is the driver is the only one who is using these functions to write, so the developer does not try to write the register in the wrong way.
> 
> It turns out the switch that requires exact 8-bit, 16-bit, and 32-bit access functions does not work using the regmap mechanism without additional register manipulation, so we do not really need 3 regmap pointers.

Can you elaborate on this ?

> One benefit of having 32-bit access is the register dump from debugfs can be much shorter than using 8-bit.

You can constraint each regmap definition and have it allow only certain
types of accesses to a selected set of registers, so then your debugfs
regmap dump would match the register list in the datasheet. I didn't add
it into this initial series for simplicity's sake though.

-- 
Best regards,
Marek Vasut

RE: [RFT][PATCH V2 09/10] net: dsa: microchip: Factor out regmap config generation into common header

[Tristram.Ha]

> OK, so there are clearly restrictions to what can be written and how.
>

It is hardware bug.  You need to read those high PHY registers in 32-bit and modify them and write them back even though they are 16-bit.  The regular low PHY registers are not affected.

Another hardware bug with I2C access is an interrupt will be triggered whenever the PHY register write does not end at 32-bit boundary.  Right now that interrupt is not enabled, and this problem can be easily avoided by disabling a function.

These problems are for KSZ9477 only.
 
> > My point is the driver is the only one who is using these functions to write,
> so the developer does not try to write the register in the wrong way.
> >
> > It turns out the switch that requires exact 8-bit, 16-bit, and 32-bit access
> functions does not work using the regmap mechanism without additional
> register manipulation, so we do not really need 3 regmap pointers.
> 
> Can you elaborate on this ?
> 

This switch shares design with an Ethernet controller, and the register access uses byte enable.

There are 4 bits of byte enable indicating whether 1 byte, 2 bytes, 3 bytes, or 4 bytes are accessed.  Normally the 3 bytes option is not used.

The register address is then shifted right by 2.

0x40.1 -> 0x101
0x41.1 -> 0x102
0x42.1 -> 0x104
0x43.1 -> 0x108
0x40.2 -> 0x103
0x42.2 -> 0x10c
0x40.4 -> 0x10f
0x44.4 -> 0x11f

So the only option that works well with the regmap mechanism is 32-bit.

Problem is the register definitions are mostly 16-bit, while the switch also shares another switch design which uses 8-bit.

Re: [RFT][PATCH V2 09/10] net: dsa: microchip: Factor out regmap config generation into common header

[Marek Vasut]

On 1/11/19 7:56 PM, Tristram.Ha@microchip.com wrote:
>> OK, so there are clearly restrictions to what can be written and how.
>>
> 
> It is hardware bug.  You need to read those high PHY registers in 32-bit and modify them and write them back even though they are 16-bit.  The regular low PHY registers are not affected.
> 
> Another hardware bug with I2C access is an interrupt will be triggered whenever the PHY register write does not end at 32-bit boundary.  Right now that interrupt is not enabled, and this problem can be easily avoided by disabling a function.
> 
> These problems are for KSZ9477 only.

The regmap constraints can easily deal with this :-)

>>> My point is the driver is the only one who is using these functions to write,
>> so the developer does not try to write the register in the wrong way.
>>>
>>> It turns out the switch that requires exact 8-bit, 16-bit, and 32-bit access
>> functions does not work using the regmap mechanism without additional
>> register manipulation, so we do not really need 3 regmap pointers.
>>
>> Can you elaborate on this ?
>>
> 
> This switch shares design with an Ethernet controller, and the register access uses byte enable.
> 
> There are 4 bits of byte enable indicating whether 1 byte, 2 bytes, 3 bytes, or 4 bytes are accessed.  Normally the 3 bytes option is not used.
> 
> The register address is then shifted right by 2.
> 
> 0x40.1 -> 0x101
> 0x41.1 -> 0x102
> 0x42.1 -> 0x104
> 0x43.1 -> 0x108
> 0x40.2 -> 0x103
> 0x42.2 -> 0x10c
> 0x40.4 -> 0x10f
> 0x44.4 -> 0x11f
> 
> So the only option that works well with the regmap mechanism is 32-bit.
> 
> Problem is the register definitions are mostly 16-bit, while the switch also shares another switch design which uses 8-bit.

So we can have a regmap for each "chunk" of the address space, which as
the correct width, that's fine.

Can you try this series on net-next on the KSZ9477 , fix it up where
needed to make it work with that switch and send out the changes that
were needed ?

Thanks !

-- 
Best regards,
Marek Vasut