[net-next PATCH v2] net: dsa: qca8k: convert to regmap read/write API

[net-next PATCH v2] net: dsa: qca8k: convert to regmap read/write API

[Christian Marangi]

Convert qca8k to regmap read/write bulk API. The mgmt eth can write up
to 16 bytes of data at times. Currently we use a custom function to do
it but regmap now supports declaration of read/write bulk even without a
bus.

Drop the custom function and rework the regmap function to this new
implementation.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca/qca8k-8xxx.c   | 94 +++++++++++++++++++++++-------
 drivers/net/dsa/qca/qca8k-common.c | 49 ++--------------
 drivers/net/dsa/qca/qca8k.h        |  5 +-
 3 files changed, 82 insertions(+), 66 deletions(-)

v2:
- Move out of RFC.
- CC Mark Brown for first implementation of this kind of regmap stuff.

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 1d3e7782a71f..c9ddc4848f58 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -342,16 +342,12 @@ qca8k_regmap_update_bits_eth(struct qca8k_priv *priv, u32 reg, u32 mask, u32 wri
 }
 
 static int
-qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
+qca8k_read_mii(struct qca8k_priv *priv, uint32_t reg, uint32_t *val)
 {
-	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
 	struct mii_bus *bus = priv->bus;
 	u16 r1, r2, page;
 	int ret;
 
-	if (!qca8k_read_eth(priv, reg, val, sizeof(*val)))
-		return 0;
-
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
@@ -368,16 +364,12 @@ qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
 }
 
 static int
-qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
+qca8k_write_mii(struct qca8k_priv *priv, uint32_t reg, uint32_t val)
 {
-	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
 	struct mii_bus *bus = priv->bus;
 	u16 r1, r2, page;
 	int ret;
 
-	if (!qca8k_write_eth(priv, reg, &val, sizeof(val)))
-		return 0;
-
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
@@ -394,17 +386,14 @@ qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
 }
 
 static int
-qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_val)
+qca8k_regmap_update_bits_mii(struct qca8k_priv *priv, uint32_t reg,
+			     uint32_t mask, uint32_t write_val)
 {
-	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
 	struct mii_bus *bus = priv->bus;
 	u16 r1, r2, page;
 	u32 val;
 	int ret;
 
-	if (!qca8k_regmap_update_bits_eth(priv, reg, mask, write_val))
-		return 0;
-
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
@@ -427,17 +416,84 @@ qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_
 	return ret;
 }
 
+static int
+qca8k_bulk_read(void *ctx, const void *reg_buf, size_t reg_len,
+		void *val_buf, size_t val_len)
+{
+	int i, count = val_len / sizeof(u32), ret;
+	struct qca8k_priv *priv = ctx;
+	u32 reg = *(u32 *)reg_buf;
+
+	if (priv->mgmt_master &&
+	    !qca8k_read_eth(priv, reg, val_buf, val_len))
+		return 0;
+
+	/* loop count times and increment reg of 4 */
+	for (i = 0; i < count; i++, reg += sizeof(u32)) {
+		ret = qca8k_read_mii(priv, reg, val_buf + i);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int
+qca8k_bulk_gather_write(void *ctx, const void *reg_buf, size_t reg_len,
+			const void *val_buf, size_t val_len)
+{
+	int i, count = val_len / sizeof(u32), ret;
+	struct qca8k_priv *priv = ctx;
+	u32 *val = (u32 *)val_buf;
+	u32 reg = *(u32 *)reg_buf;
+
+	if (priv->mgmt_master &&
+	    !qca8k_write_eth(priv, reg, val, val_len))
+		return 0;
+
+	/* loop count times, increment reg of 4 and increment val ptr to
+	 * the next value
+	 */
+	for (i = 0; i < count; i++, reg += sizeof(u32), val++) {
+		ret = qca8k_write_mii(priv, reg, *val);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int
+qca8k_bulk_write(void *ctx, const void *data, size_t bytes)
+{
+	return qca8k_bulk_gather_write(ctx, data, sizeof(u32), data + sizeof(u32),
+				       bytes - sizeof(u32));
+}
+
+static int
+qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_val)
+{
+	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
+
+	if (!qca8k_regmap_update_bits_eth(priv, reg, mask, write_val))
+		return 0;
+
+	return qca8k_regmap_update_bits_mii(priv, reg, mask, write_val);
+}
+
 static struct regmap_config qca8k_regmap_config = {
-	.reg_bits = 16,
+	.reg_bits = 32,
 	.val_bits = 32,
 	.reg_stride = 4,
 	.max_register = 0x16ac, /* end MIB - Port6 range */
-	.reg_read = qca8k_regmap_read,
-	.reg_write = qca8k_regmap_write,
+	.read = qca8k_bulk_read,
+	.write = qca8k_bulk_write,
 	.reg_update_bits = qca8k_regmap_update_bits,
 	.rd_table = &qca8k_readable_table,
 	.disable_locking = true, /* Locking is handled by qca8k read/write */
 	.cache_type = REGCACHE_NONE, /* Explicitly disable CACHE */
+	.max_raw_read = 16, /* mgmt eth can read/write up to 4 bytes at times */
+	.max_raw_write = 16,
 };
 
 static int
@@ -2014,8 +2070,6 @@ static SIMPLE_DEV_PM_OPS(qca8k_pm_ops,
 
 static const struct qca8k_info_ops qca8xxx_ops = {
 	.autocast_mib = qca8k_get_ethtool_stats_eth,
-	.read_eth = qca8k_read_eth,
-	.write_eth = qca8k_write_eth,
 };
 
 static const struct qca8k_match_data qca8327 = {
diff --git a/drivers/net/dsa/qca/qca8k-common.c b/drivers/net/dsa/qca/qca8k-common.c
index bba95613e218..a3bcff4bac29 100644
--- a/drivers/net/dsa/qca/qca8k-common.c
+++ b/drivers/net/dsa/qca/qca8k-common.c
@@ -101,45 +101,6 @@ const struct regmap_access_table qca8k_readable_table = {
 	.n_yes_ranges = ARRAY_SIZE(qca8k_readable_ranges),
 };
 
-/* TODO: remove these extra ops when we can support regmap bulk read/write */
-static int qca8k_bulk_read(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
-{
-	int i, count = len / sizeof(u32), ret;
-
-	if (priv->mgmt_master && priv->info->ops->read_eth &&
-	    !priv->info->ops->read_eth(priv, reg, val, len))
-		return 0;
-
-	for (i = 0; i < count; i++) {
-		ret = regmap_read(priv->regmap, reg + (i * 4), val + i);
-		if (ret < 0)
-			return ret;
-	}
-
-	return 0;
-}
-
-/* TODO: remove these extra ops when we can support regmap bulk read/write */
-static int qca8k_bulk_write(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
-{
-	int i, count = len / sizeof(u32), ret;
-	u32 tmp;
-
-	if (priv->mgmt_master && priv->info->ops->write_eth &&
-	    !priv->info->ops->write_eth(priv, reg, val, len))
-		return 0;
-
-	for (i = 0; i < count; i++) {
-		tmp = val[i];
-
-		ret = regmap_write(priv->regmap, reg + (i * 4), tmp);
-		if (ret < 0)
-			return ret;
-	}
-
-	return 0;
-}
-
 static int qca8k_busy_wait(struct qca8k_priv *priv, u32 reg, u32 mask)
 {
 	u32 val;
@@ -150,11 +111,12 @@ static int qca8k_busy_wait(struct qca8k_priv *priv, u32 reg, u32 mask)
 
 static int qca8k_fdb_read(struct qca8k_priv *priv, struct qca8k_fdb *fdb)
 {
-	u32 reg[3];
+	u32 reg[QCA8K_ATU_TABLE_SIZE];
 	int ret;
 
 	/* load the ARL table into an array */
-	ret = qca8k_bulk_read(priv, QCA8K_REG_ATU_DATA0, reg, sizeof(reg));
+	ret = regmap_bulk_read(priv->regmap, QCA8K_REG_ATU_DATA0, reg,
+			       QCA8K_ATU_TABLE_SIZE);
 	if (ret)
 		return ret;
 
@@ -178,7 +140,7 @@ static int qca8k_fdb_read(struct qca8k_priv *priv, struct qca8k_fdb *fdb)
 static void qca8k_fdb_write(struct qca8k_priv *priv, u16 vid, u8 port_mask,
 			    const u8 *mac, u8 aging)
 {
-	u32 reg[3] = { 0 };
+	u32 reg[QCA8K_ATU_TABLE_SIZE] = { 0 };
 
 	/* vid - 83:72 */
 	reg[2] = FIELD_PREP(QCA8K_ATU_VID_MASK, vid);
@@ -195,7 +157,8 @@ static void qca8k_fdb_write(struct qca8k_priv *priv, u16 vid, u8 port_mask,
 	reg[0] |= FIELD_PREP(QCA8K_ATU_ADDR5_MASK, mac[5]);
 
 	/* load the array into the ARL table */
-	qca8k_bulk_write(priv, QCA8K_REG_ATU_DATA0, reg, sizeof(reg));
+	regmap_bulk_write(priv->regmap, QCA8K_REG_ATU_DATA0, reg,
+			  QCA8K_ATU_TABLE_SIZE);
 }
 
 static int qca8k_fdb_access(struct qca8k_priv *priv, enum qca8k_fdb_cmd cmd,
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index e36ecc9777f4..fc5766a40b8a 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -148,6 +148,8 @@
 #define QCA8K_REG_IPV4_PRI_ADDR_MASK			0x474
 
 /* Lookup registers */
+#define QCA8K_ATU_TABLE_SIZE				3 /* 12 bytes wide table / sizeof(u32) */
+
 #define QCA8K_REG_ATU_DATA0				0x600
 #define   QCA8K_ATU_ADDR2_MASK				GENMASK(31, 24)
 #define   QCA8K_ATU_ADDR3_MASK				GENMASK(23, 16)
@@ -328,9 +330,6 @@ struct qca8k_priv;
 
 struct qca8k_info_ops {
 	int (*autocast_mib)(struct dsa_switch *ds, int port, u64 *data);
-	/* TODO: remove these extra ops when we can support regmap bulk read/write */
-	int (*read_eth)(struct qca8k_priv *priv, u32 reg, u32 *val, int len);
-	int (*write_eth)(struct qca8k_priv *priv, u32 reg, u32 *val, int len);
 };
 
 struct qca8k_match_data {
-- 
2.36.1

Re: [net-next PATCH v2] net: dsa: qca8k: convert to regmap read/write API

[Christian Marangi]

On Sat, Aug 27, 2022 at 04:00:40PM +0200, Andrew Lunn wrote:
> >  static struct regmap_config qca8k_regmap_config = {
> > -	.reg_bits = 16,
> > +	.reg_bits = 32,
> 
> Does this change really allow you to access more registers? 
>

I could be confused but I think the value was wrong from the start. (the
driver is a bit old and the regmap config struct was very wrong at
times)
This should declare how wide is each address right?

If this is the case then at times who declared the regmap config was
confused by the fact that the mdio is limited to 16 bits and require
special handling.

This is not problematic for the bits ops but is problematic for the bulk
ops as they base the calculation on these values.

Or I could be totally wrong... Anyway without this change the wrong
address is passed to the bulk ops so it's necessary (and for the said
reason, the value was wrong from the start)

> >  	.val_bits = 32,
> >  	.reg_stride = 4,
> >  	.max_register = 0x16ac, /* end MIB - Port6 range */
> > -	.reg_read = qca8k_regmap_read,
> > -	.reg_write = qca8k_regmap_write,
> > +	.read = qca8k_bulk_read,
> > +	.write = qca8k_bulk_write,
> >  	.reg_update_bits = qca8k_regmap_update_bits,
> >  	.rd_table = &qca8k_readable_table,
> >  	.disable_locking = true, /* Locking is handled by qca8k read/write */
> >  	.cache_type = REGCACHE_NONE, /* Explicitly disable CACHE */
> > +	.max_raw_read = 16, /* mgmt eth can read/write up to 4 bytes at times */
> > +	.max_raw_write = 16,
> 
> I think the word 'bytes' in the comment is wrong. I assume you can
> access 4 registers, each register is one 32-bit work in size.
> 

Yes you are right. Any suggestion on how to improve? 

> >  static int qca8k_fdb_read(struct qca8k_priv *priv, struct qca8k_fdb *fdb)
> >  {
> > -	u32 reg[3];
> > +	u32 reg[QCA8K_ATU_TABLE_SIZE];
> >  	int ret;
> >  
> >  	/* load the ARL table into an array */
> > -	ret = qca8k_bulk_read(priv, QCA8K_REG_ATU_DATA0, reg, sizeof(reg));
> > +	ret = regmap_bulk_read(priv->regmap, QCA8K_REG_ATU_DATA0, reg,
> > +			       QCA8K_ATU_TABLE_SIZE);
> >  	if (ret)
> >  		return ret;
> 
> Please split the 3 -> QCA8K_ATU_TABLE_SIZE change out into a patch of
> its own.
> 
> Ideally you want lots of small, obviously correct patches. A change
> which replaces 3 for QCA8K_ATU_TABLE_SIZE should be small and
> obviously correct, meaning it is quick and easy to review, and makes
> the more complex to review change smaller and also simpler to review.
> 

Will split in v3 hoping I get some feedback from Mark and you.

>     Andrew

-- 
	Ansuel

Re: [net-next PATCH v2] net: dsa: qca8k: convert to regmap read/write API

[Andrew Lunn]

>  static struct regmap_config qca8k_regmap_config = {
> -	.reg_bits = 16,
> +	.reg_bits = 32,

Does this change really allow you to access more registers? 

>  	.val_bits = 32,
>  	.reg_stride = 4,
>  	.max_register = 0x16ac, /* end MIB - Port6 range */
> -	.reg_read = qca8k_regmap_read,
> -	.reg_write = qca8k_regmap_write,
> +	.read = qca8k_bulk_read,
> +	.write = qca8k_bulk_write,
>  	.reg_update_bits = qca8k_regmap_update_bits,
>  	.rd_table = &qca8k_readable_table,
>  	.disable_locking = true, /* Locking is handled by qca8k read/write */
>  	.cache_type = REGCACHE_NONE, /* Explicitly disable CACHE */
> +	.max_raw_read = 16, /* mgmt eth can read/write up to 4 bytes at times */
> +	.max_raw_write = 16,

I think the word 'bytes' in the comment is wrong. I assume you can
access 4 registers, each register is one 32-bit work in size.

>  static int qca8k_fdb_read(struct qca8k_priv *priv, struct qca8k_fdb *fdb)
>  {
> -	u32 reg[3];
> +	u32 reg[QCA8K_ATU_TABLE_SIZE];
>  	int ret;
>  
>  	/* load the ARL table into an array */
> -	ret = qca8k_bulk_read(priv, QCA8K_REG_ATU_DATA0, reg, sizeof(reg));
> +	ret = regmap_bulk_read(priv->regmap, QCA8K_REG_ATU_DATA0, reg,
> +			       QCA8K_ATU_TABLE_SIZE);
>  	if (ret)
>  		return ret;

Please split the 3 -> QCA8K_ATU_TABLE_SIZE change out into a patch of
its own.

Ideally you want lots of small, obviously correct patches. A change
which replaces 3 for QCA8K_ATU_TABLE_SIZE should be small and
obviously correct, meaning it is quick and easy to review, and makes
the more complex to review change smaller and also simpler to review.

    Andrew

Re: [net-next PATCH v2] net: dsa: qca8k: convert to regmap read/write API

[Andrew Lunn]

On Sat, Aug 27, 2022 at 03:48:34PM +0200, Christian Marangi wrote:
> On Sat, Aug 27, 2022 at 04:00:40PM +0200, Andrew Lunn wrote:
> > >  static struct regmap_config qca8k_regmap_config = {
> > > -	.reg_bits = 16,
> > > +	.reg_bits = 32,
> > 
> > Does this change really allow you to access more registers? 
> >
> 
> I could be confused but I think the value was wrong from the start. (the
> driver is a bit old and the regmap config struct was very wrong at
> times)
> This should declare how wide is each address right?
> 
> If this is the case then at times who declared the regmap config was
> confused by the fact that the mdio is limited to 16 bits and require
> special handling.
> 
> This is not problematic for the bits ops but is problematic for the bulk
> ops as they base the calculation on these values.
> 
> Or I could be totally wrong... Anyway without this change the wrong
> address is passed to the bulk ops so it's necessary (and for the said
> reason, the value was wrong from the start)

So please figure it out, maybe use git blame to see who added it, and
ask etc. And then submit a patch which changes just this, including
and explanation why it should be changed.
 
> > >  	.val_bits = 32,
> > >  	.reg_stride = 4,
> > >  	.max_register = 0x16ac, /* end MIB - Port6 range */
> > > -	.reg_read = qca8k_regmap_read,
> > > -	.reg_write = qca8k_regmap_write,
> > > +	.read = qca8k_bulk_read,
> > > +	.write = qca8k_bulk_write,
> > >  	.reg_update_bits = qca8k_regmap_update_bits,
> > >  	.rd_table = &qca8k_readable_table,
> > >  	.disable_locking = true, /* Locking is handled by qca8k read/write */
> > >  	.cache_type = REGCACHE_NONE, /* Explicitly disable CACHE */
> > > +	.max_raw_read = 16, /* mgmt eth can read/write up to 4 bytes at times */
> > > +	.max_raw_write = 16,
> > 
> > I think the word 'bytes' in the comment is wrong. I assume you can
> > access 4 registers, each register is one 32-bit work in size.
> > 
> 
> Yes you are right. Any suggestion on how to improve?

s/bytes/words/

	Andrew

Re: [net-next PATCH v2] net: dsa: qca8k: convert to regmap read/write API

[Christian Marangi]

On Sat, Aug 27, 2022 at 04:00:40PM +0200, Andrew Lunn wrote:
> >  static struct regmap_config qca8k_regmap_config = {
> > -	.reg_bits = 16,
> > +	.reg_bits = 32,
> 
> Does this change really allow you to access more registers? 
> 
> >  	.val_bits = 32,
> >  	.reg_stride = 4,
> >  	.max_register = 0x16ac, /* end MIB - Port6 range */
> > -	.reg_read = qca8k_regmap_read,
> > -	.reg_write = qca8k_regmap_write,
> > +	.read = qca8k_bulk_read,
> > +	.write = qca8k_bulk_write,
> >  	.reg_update_bits = qca8k_regmap_update_bits,
> >  	.rd_table = &qca8k_readable_table,
> >  	.disable_locking = true, /* Locking is handled by qca8k read/write */
> >  	.cache_type = REGCACHE_NONE, /* Explicitly disable CACHE */
> > +	.max_raw_read = 16, /* mgmt eth can read/write up to 4 bytes at times */
> > +	.max_raw_write = 16,
> 
> I think the word 'bytes' in the comment is wrong. I assume you can
> access 4 registers, each register is one 32-bit work in size.
> 
> >  static int qca8k_fdb_read(struct qca8k_priv *priv, struct qca8k_fdb *fdb)
> >  {
> > -	u32 reg[3];
> > +	u32 reg[QCA8K_ATU_TABLE_SIZE];
> >  	int ret;
> >  
> >  	/* load the ARL table into an array */
> > -	ret = qca8k_bulk_read(priv, QCA8K_REG_ATU_DATA0, reg, sizeof(reg));
> > +	ret = regmap_bulk_read(priv->regmap, QCA8K_REG_ATU_DATA0, reg,
> > +			       QCA8K_ATU_TABLE_SIZE);
> >  	if (ret)
> >  		return ret;
> 
> Please split the 3 -> QCA8K_ATU_TABLE_SIZE change out into a patch of
> its own.
> 
> Ideally you want lots of small, obviously correct patches. A change
> which replaces 3 for QCA8K_ATU_TABLE_SIZE should be small and
> obviously correct, meaning it is quick and easy to review, and makes
> the more complex to review change smaller and also simpler to review.
> 
>     Andrew

I'm working on v3.
I manage to make everything work with 16 bit. (It was set as the max reg
for this switch is 0x16ac)

Anyway on second look, this change is necessary and I think split this
in another patch would be redundant and problematic. (for backporting
purpose and to correctly write a sane commit description)

The regmap bulk implementation take as the 4th arg the count of reg to
read/write. Current implementation instead take the actual size of the
array. So I think this has to be changed in the bulk conversion patch.

I can split it but it will be something like

qca8k_bulk_write(priv, QCA8K_REG_ATU_DATA0, reg, 
		 QCA8K_ATU_TABLE_SIZE * sizeof(u32));

and on the bulk conversion commit I have to drop it again to 

regmap_bulk_read(priv, QCA8K_REG_ATU_DATA0, reg,
                 QCA8K_ATU_TABLE_SIZE);

Seems a bad thing to make a change and than fix that change on the next
commit.

Will wait for a response about this and propose v3 with what is the
correct approach.

-- 
	Ansuel