[PATCH net-next 0/3] net: dsa: microchip: fix writes to phy registers >= 0x10

[PATCH net-next 0/3] net: dsa: microchip: fix writes to phy registers >= 0x10

[Rasmus Villemoes]

Patch 1 is just a simplification, technically unrelated to the other
two patches. But it would be a bit inconsistent to have the new
ksz_prmw32() introduced in patch 2 use ksz_rmw32() while leaving
ksz_prmw8() as-is.

The actual fix is of course patch 3. I can definitely see some weird
behaviour on our ksz9567 when writing to phy registers 0x1e and 0x1f
(with phytool from userspace), though it does not seem that the effect
is always to write zeroes to the buddy register as the errata sheet
says would be the case. In our case, the switch is connected via i2c;
I hope somebody with other switches and/or the SPI variants can test
this.

Rasmus Villemoes (3):
  net: dsa: microchip: simplify ksz_prmw8()
  net: dsa: microchip: add ksz_prmw32() helper
  net: dsa: microchip: fix writes to phy registers >= 0x10

 drivers/net/dsa/microchip/ksz9477.c    | 18 +++++++++++++++++-
 drivers/net/dsa/microchip/ksz_common.h | 18 ++++++++----------
 2 files changed, 25 insertions(+), 11 deletions(-)

-- 
2.37.2

[PATCH net-next 1/3] net: dsa: microchip: simplify ksz_prmw8()

[Rasmus Villemoes]

Implement ksz_prmw8() in terms of ksz_rmw8(), just as all the other
ksz_pX are implemented in terms of ksz_X. No functional change.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/net/dsa/microchip/ksz_common.h | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index a66b56857ec6..2453c43c48a5 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -578,17 +578,8 @@ static inline int ksz_pwrite32(struct ksz_device *dev, int port, int offset,
 static inline int ksz_prmw8(struct ksz_device *dev, int port, int offset,
 			    u8 mask, u8 val)
 {
-	int ret;
-
-	ret = regmap_update_bits(ksz_regmap_8(dev),
-				 dev->dev_ops->get_port_addr(port, offset),
-				 mask, val);
-	if (ret)
-		dev_err(dev->dev, "can't rmw 8bit reg 0x%x: %pe\n",
-			dev->dev_ops->get_port_addr(port, offset),
-			ERR_PTR(ret));
-
-	return ret;
+	return ksz_rmw8(dev, dev->dev_ops->get_port_addr(port, offset),
+			mask, val);
 }
 
 static inline void ksz_regmap_lock(void *__mtx)
-- 
2.37.2

Re: [PATCH net-next 1/3] net: dsa: microchip: simplify ksz_prmw8()

[Simon Horman]

On Tue, Jun 20, 2023 at 01:38:52PM +0200, Rasmus Villemoes wrote:
> Implement ksz_prmw8() in terms of ksz_rmw8(), just as all the other
> ksz_pX are implemented in terms of ksz_X. No functional change.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next 1/3] net: dsa: microchip: simplify ksz_prmw8()

[Arun.Ramadoss]

On Tue, 2023-06-20 at 13:38 +0200, Rasmus Villemoes wrote:
> [Some people who received this message don't often get email from
> linux@rasmusvillemoes.dk. Learn why this is important at 
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Implement ksz_prmw8() in terms of ksz_rmw8(), just as all the other
> ksz_pX are implemented in terms of ksz_X. No functional change.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Acked-by: Arun Ramadoss <arun.ramadoss@microchip.com>

> 

[PATCH net-next 2/3] net: dsa: microchip: add ksz_prmw32() helper

[Rasmus Villemoes]

This will be used in a subsequent patch fixing an errata for writes to
certain PHY registers.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/net/dsa/microchip/ksz_common.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 2453c43c48a5..28444e5924f9 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -582,6 +582,13 @@ static inline int ksz_prmw8(struct ksz_device *dev, int port, int offset,
 			mask, val);
 }
 
+static inline int ksz_prmw32(struct ksz_device *dev, int port, int offset,
+			     u32 mask, u32 val)
+{
+	return ksz_rmw32(dev, dev->dev_ops->get_port_addr(port, offset),
+			 mask, val);
+}
+
 static inline void ksz_regmap_lock(void *__mtx)
 {
 	struct mutex *mtx = __mtx;
-- 
2.37.2

Re: [PATCH net-next 2/3] net: dsa: microchip: add ksz_prmw32() helper

[Simon Horman]

On Tue, Jun 20, 2023 at 01:38:53PM +0200, Rasmus Villemoes wrote:
> This will be used in a subsequent patch fixing an errata for writes to
> certain PHY registers.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next 2/3] net: dsa: microchip: add ksz_prmw32() helper

[Arun.Ramadoss]

On Tue, 2023-06-20 at 13:38 +0200, Rasmus Villemoes wrote:
> [Some people who received this message don't often get email from
> linux@rasmusvillemoes.dk. Learn why this is important at 
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> This will be used in a subsequent patch fixing an errata for writes
> to
> certain PHY registers.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Acked-by: Arun Ramadoss <arun.ramadoss@microchip.com>

[PATCH net-next 3/3] net: dsa: microchip: fix writes to phy registers >= 0x10

[Rasmus Villemoes]

According to the errata sheets for ksz9477 and ksz9567, writes to the
PHY registers 0x10-0x1f (i.e. those located at addresses 0xN120 to
0xN13f) must be done as a 32 bit write to the 4-byte aligned address
containing the register, hence requires a RMW in order not to change
the adjacent PHY register.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/net/dsa/microchip/ksz9477.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index fc5157a10af5..83b7f2d5c1ea 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -329,11 +329,27 @@ int ksz9477_r_phy(struct ksz_device *dev, u16 addr, u16 reg, u16 *data)
 
 int ksz9477_w_phy(struct ksz_device *dev, u16 addr, u16 reg, u16 val)
 {
+	u32 mask, val32;
+
 	/* No real PHY after this. */
 	if (!dev->info->internal_phy[addr])
 		return 0;
 
-	return ksz_pwrite16(dev, addr, 0x100 + (reg << 1), val);
+	if (reg < 0x10)
+		return ksz_pwrite16(dev, addr, 0x100 + (reg << 1), val);
+
+	/* Errata: When using SPI, I2C, or in-band register access,
+	 * writes to certain PHY registers should be performed as
+	 * 32-bit writes instead of 16-bit writes.
+	 */
+	val32 = val;
+	mask = 0xffff;
+	if ((reg & 1) == 0) {
+		val32 <<= 16;
+		mask <<= 16;
+	}
+	reg &= ~1;
+	return ksz_prmw32(dev, addr, 0x100 + (reg << 1), mask, val32);
 }
 
 void ksz9477_cfg_port_member(struct ksz_device *dev, int port, u8 member)
-- 
2.37.2

Re: [PATCH net-next 3/3] net: dsa: microchip: fix writes to phy registers >= 0x10

[Simon Horman]

On Tue, Jun 20, 2023 at 01:38:54PM +0200, Rasmus Villemoes wrote:
> According to the errata sheets for ksz9477 and ksz9567, writes to the
> PHY registers 0x10-0x1f (i.e. those located at addresses 0xN120 to
> 0xN13f) must be done as a 32 bit write to the 4-byte aligned address
> containing the register, hence requires a RMW in order not to change
> the adjacent PHY register.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next 3/3] net: dsa: microchip: fix writes to phy registers >= 0x10

[Andrew Lunn]

On Tue, Jun 20, 2023 at 01:38:54PM +0200, Rasmus Villemoes wrote:
> According to the errata sheets for ksz9477 and ksz9567, writes to the
> PHY registers 0x10-0x1f (i.e. those located at addresses 0xN120 to
> 0xN13f) must be done as a 32 bit write to the 4-byte aligned address
> containing the register, hence requires a RMW in order not to change
> the adjacent PHY register.

ASIC engineers do see to come up with novel ways to break things.

I assume you have not seen real problems with this, which is why it is
not for net and a Fixes: tag?

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 3/3] net: dsa: microchip: fix writes to phy registers >= 0x10

[Rasmus Villemoes]

On 20/06/2023 21.28, Andrew Lunn wrote:
> On Tue, Jun 20, 2023 at 01:38:54PM +0200, Rasmus Villemoes wrote:
>> According to the errata sheets for ksz9477 and ksz9567, writes to the
>> PHY registers 0x10-0x1f (i.e. those located at addresses 0xN120 to
>> 0xN13f) must be done as a 32 bit write to the 4-byte aligned address
>> containing the register, hence requires a RMW in order not to change
>> the adjacent PHY register.
> 
> ASIC engineers do see to come up with novel ways to break things.
> 
> I assume you have not seen real problems with this, which is why it is
> not for net and a Fixes: tag?

Well, not real problems yet, no. The back story is that I want/need to
implement support for "single LED mode" on the ksz9567, because our
board has two separate simple LEDs for link/activity, and not some
multi-color LED that can indicate speed/link/activity. So that means
writing a 1 to bit 4 of MMD reg 2/0, but due to an errata, _also_
writing a 1 to bit 9 of phy register 0x1e, and when one wants to do
that, this errata applies.

Rasmus

Re: [PATCH net-next 0/3] net: dsa: microchip: fix writes to phy registers >= 0x10

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 20 Jun 2023 13:38:51 +0200 you wrote:
> Patch 1 is just a simplification, technically unrelated to the other
> two patches. But it would be a bit inconsistent to have the new
> ksz_prmw32() introduced in patch 2 use ksz_rmw32() while leaving
> ksz_prmw8() as-is.
> 
> The actual fix is of course patch 3. I can definitely see some weird
> behaviour on our ksz9567 when writing to phy registers 0x1e and 0x1f
> (with phytool from userspace), though it does not seem that the effect
> is always to write zeroes to the buddy register as the errata sheet
> says would be the case. In our case, the switch is connected via i2c;
> I hope somebody with other switches and/or the SPI variants can test
> this.
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] net: dsa: microchip: simplify ksz_prmw8()
    https://git.kernel.org/netdev/net-next/c/3b42fbd59511
  - [net-next,2/3] net: dsa: microchip: add ksz_prmw32() helper
    https://git.kernel.org/netdev/net-next/c/ece28ecbec9f
  - [net-next,3/3] net: dsa: microchip: fix writes to phy registers >= 0x10
    https://git.kernel.org/netdev/net-next/c/5c844d57aa78

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html