[PATCH] net: dsa: mv88e6xxx: Wait for EEPROM done after HW reset

[PATCH] net: dsa: mv88e6xxx: Wait for EEPROM done after HW reset

[Ruslan V. Sushko]

From: Andrew Lunn <andrew@lunn.ch>

When the switch is hardware reset, it reads the contents of the
EEPROM. This can contain instructions for programming values into
registers and to perform waits between such programming. Reading the
EEPROM can take longer than the 100ms mv88e6xxx_hardware_reset() waits
after deasserting the reset GPIO. So poll the EEPROM done bit to
ensure it is complete.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Ruslan Sushko <rus@sushko.dev>
---
 drivers/net/dsa/mv88e6xxx/chip.c    |  2 ++
 drivers/net/dsa/mv88e6xxx/global1.c | 31 +++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/global1.h |  1 +
 3 files changed, 34 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index bd297ae7cf9e..34cca0a4b31c 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2297,6 +2297,8 @@ static void mv88e6xxx_hardware_reset(struct mv88e6xxx_chip *chip)
 		usleep_range(10000, 20000);
 		gpiod_set_value_cansleep(gpiod, 0);
 		usleep_range(10000, 20000);
+
+		mv88e6xxx_g1_wait_eeprom_done(chip);
 	}
 }
 
diff --git a/drivers/net/dsa/mv88e6xxx/global1.c b/drivers/net/dsa/mv88e6xxx/global1.c
index f62aa83ca08d..33d443a37efc 100644
--- a/drivers/net/dsa/mv88e6xxx/global1.c
+++ b/drivers/net/dsa/mv88e6xxx/global1.c
@@ -75,6 +75,37 @@ static int mv88e6xxx_g1_wait_init_ready(struct mv88e6xxx_chip *chip)
 	return mv88e6xxx_g1_wait_bit(chip, MV88E6XXX_G1_STS, bit, 1);
 }
 
+void mv88e6xxx_g1_wait_eeprom_done(struct mv88e6xxx_chip *chip)
+{
+	const unsigned long timeout = jiffies + 1 * HZ;
+	u16 val;
+	int err;
+
+	/* Wait up to 1 second for the switch to finish reading the
+	 * EEPROM.
+	 */
+	while (time_before(jiffies, timeout)) {
+		err = mv88e6xxx_g1_read(chip, MV88E6XXX_G1_STS, &val);
+		if (err) {
+			dev_err(chip->dev, "Error reading status");
+			return;
+		}
+
+		/* If the switch is still resetting, it may not
+		 * respond on the bus, and so MDIO read returns
+		 * 0xffff. Differentiate between that, and waiting for
+		 * the EEPROM to be done by bit 0 being set.
+		 */
+		if (val != 0xffff &&
+		    val & BIT(MV88E6XXX_G1_STS_IRQ_EEPROM_DONE))
+			return;
+
+		usleep_range(1000, 2000);
+	}
+
+	dev_err(chip->dev, "Timeout waiting for EEPROM done");
+}
+
 /* Offset 0x01: Switch MAC Address Register Bytes 0 & 1
  * Offset 0x02: Switch MAC Address Register Bytes 2 & 3
  * Offset 0x03: Switch MAC Address Register Bytes 4 & 5
diff --git a/drivers/net/dsa/mv88e6xxx/global1.h b/drivers/net/dsa/mv88e6xxx/global1.h
index 1e3546f8b072..e05abe61fa11 100644
--- a/drivers/net/dsa/mv88e6xxx/global1.h
+++ b/drivers/net/dsa/mv88e6xxx/global1.h
@@ -278,6 +278,7 @@ int mv88e6xxx_g1_set_switch_mac(struct mv88e6xxx_chip *chip, u8 *addr);
 int mv88e6185_g1_reset(struct mv88e6xxx_chip *chip);
 int mv88e6352_g1_reset(struct mv88e6xxx_chip *chip);
 int mv88e6250_g1_reset(struct mv88e6xxx_chip *chip);
+void mv88e6xxx_g1_wait_eeprom_done(struct mv88e6xxx_chip *chip);
 
 int mv88e6185_g1_ppu_enable(struct mv88e6xxx_chip *chip);
 int mv88e6185_g1_ppu_disable(struct mv88e6xxx_chip *chip);
-- 
2.25.4

Re: [PATCH] net: dsa: mv88e6xxx: Wait for EEPROM done after HW reset

[Jakub Kicinski]

On Mon, 16 Nov 2020 08:43:01 -0800 Ruslan V. Sushko wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> 
> When the switch is hardware reset, it reads the contents of the
> EEPROM. This can contain instructions for programming values into
> registers and to perform waits between such programming. Reading the
> EEPROM can take longer than the 100ms mv88e6xxx_hardware_reset() waits
> after deasserting the reset GPIO. So poll the EEPROM done bit to
> ensure it is complete.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Ruslan Sushko <rus@sushko.dev>

Andrew, do we need this in net?

Re: [PATCH] net: dsa: mv88e6xxx: Wait for EEPROM done after HW reset

[Andrew Lunn]

On Wed, Nov 18, 2020 at 10:52:51AM -0800, Jakub Kicinski wrote:
> On Mon, 16 Nov 2020 08:43:01 -0800 Ruslan V. Sushko wrote:
> > From: Andrew Lunn <andrew@lunn.ch>
> > 
> > When the switch is hardware reset, it reads the contents of the
> > EEPROM. This can contain instructions for programming values into
> > registers and to perform waits between such programming. Reading the
> > EEPROM can take longer than the 100ms mv88e6xxx_hardware_reset() waits
> > after deasserting the reset GPIO. So poll the EEPROM done bit to
> > ensure it is complete.
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > Signed-off-by: Ruslan Sushko <rus@sushko.dev>
> 
> Andrew, do we need this in net?

I was wondering about that. I actually recommend leaving the EEPROM
empty. The driver has no idea what magic the EEPROM has done, and so
will stomp over it, or make assumptions which are not true about
register values.

But Zodiac has valid use cases of putting stuff into the EEPROM, and
they are aware of the danger. And this patch has got lost at least
once, causing lots of head scratching. So getting it into 5.10 makes
sense for them. I don't think it needs to go further back.

Not sure what Fixes: tag to use.

    Andrew

Re: [PATCH] net: dsa: mv88e6xxx: Wait for EEPROM done after HW reset

[Jakub Kicinski]

On Wed, 18 Nov 2020 20:10:17 +0100 Andrew Lunn wrote:
> On Wed, Nov 18, 2020 at 10:52:51AM -0800, Jakub Kicinski wrote:
> > On Mon, 16 Nov 2020 08:43:01 -0800 Ruslan V. Sushko wrote:  
> > > From: Andrew Lunn <andrew@lunn.ch>
> > > 
> > > When the switch is hardware reset, it reads the contents of the
> > > EEPROM. This can contain instructions for programming values into
> > > registers and to perform waits between such programming. Reading the
> > > EEPROM can take longer than the 100ms mv88e6xxx_hardware_reset() waits
> > > after deasserting the reset GPIO. So poll the EEPROM done bit to
> > > ensure it is complete.
> > > 
> > > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > > Signed-off-by: Ruslan Sushko <rus@sushko.dev>  
> > 
> > Andrew, do we need this in net?  
> 
> I was wondering about that. I actually recommend leaving the EEPROM
> empty. The driver has no idea what magic the EEPROM has done, and so
> will stomp over it, or make assumptions which are not true about
> register values.
> 
> But Zodiac has valid use cases of putting stuff into the EEPROM, and
> they are aware of the danger. And this patch has got lost at least
> once, causing lots of head scratching. So getting it into 5.10 makes
> sense for them. I don't think it needs to go further back.
> 
> Not sure what Fixes: tag to use.

Ack, sound reasonable. Applying to net and not queuing, thanks!