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
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 <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
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 <[email protected]>
Andrew
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
Hello:
This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <[email protected]>:
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