[net,v4] net: dsa: lan9303: ensure chip reset and wait for READY status

Commit Message

A. Sverdlin Oct. 4, 2024, 11:36 a.m. UTC

From: Anatolij Gustschin <[e-mail]>

Accessing device registers seems to be not reliable, the chip
revision is sometimes detected wrongly (0 instead of expected 1).

Ensure that the chip reset is performed via reset GPIO and then
wait for 'Device Ready' status in HW_CFG register before doing
any register initializations.

Signed-off-by: Anatolij Gustschin <[e-mail]>
[alex: reworked using read_poll_timeout()]
Signed-off-by: Alexander Sverdlin <[e-mail]>
---
Changelog:
v4: return read_poll_timeout() error unmodified
v3: comment style, use "!ret" in stop condition, user-readable error code
v2: use read_poll_timeout()

 drivers/net/dsa/lan9303-core.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

Comments

Vladimir Oltean Oct. 4, 2024, 11:57 a.m. UTC | #1

On Fri, Oct 04, 2024 at 01:36:54PM +0200, A. Sverdlin wrote:
> From: Anatolij Gustschin <[e-mail]>
> 
> Accessing device registers seems to be not reliable, the chip
> revision is sometimes detected wrongly (0 instead of expected 1).
> 
> Ensure that the chip reset is performed via reset GPIO and then
> wait for 'Device Ready' status in HW_CFG register before doing
> any register initializations.
> 
> Signed-off-by: Anatolij Gustschin <[e-mail]>
> [alex: reworked using read_poll_timeout()]
> Signed-off-by: Alexander Sverdlin <[e-mail]>
> ---

Reviewed-by: Vladimir Oltean <[e-mail]>

I see you're missing a Fixes: tag (meaning in this case: backport down
to all stable tree containing this commit). I think you can just post it
as a reply to this email, without resending, and it should get picked up
by maintainers through the same mechanism as Reviewed-by: tags.

A. Sverdlin Oct. 4, 2024, 12:07 p.m. UTC | #2

Hi Vladimir!

On Fri, 2024-10-04 at 14:57 +0300, Vladimir Oltean wrote:
> On Fri, Oct 04, 2024 at 01:36:54PM +0200, A. Sverdlin wrote:
> > From: Anatolij Gustschin <[e-mail]>
> > 
> > Accessing device registers seems to be not reliable, the chip
> > revision is sometimes detected wrongly (0 instead of expected 1).
> > 
> > Ensure that the chip reset is performed via reset GPIO and then
> > wait for 'Device Ready' status in HW_CFG register before doing
> > any register initializations.
> > 
> > Signed-off-by: Anatolij Gustschin <[e-mail]>
> > [alex: reworked using read_poll_timeout()]
> > Signed-off-by: Alexander Sverdlin <[e-mail]>
> > ---
> 
> Reviewed-by: Vladimir Oltean <[e-mail]>
> 
> I see you're missing a Fixes: tag (meaning in this case: backport down
> to all stable tree containing this commit). I think you can just post it
> as a reply to this email, without resending, and it should get picked up
> by maintainers through the same mechanism as Reviewed-by: tags.

Well, the only meaningful would be the very first commit for lan9303:

Fixes: a1292595e006 ("net: dsa: add new DSA switch driver for the SMSC-LAN9303")
Cc: <[e-mail]>

[e-mail] Oct. 8, 2024, midnight UTC | #3

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <[e-mail]>:

On Fri,  4 Oct 2024 13:36:54 +0200 you wrote:
> From: Anatolij Gustschin <[e-mail]>
> 
> Accessing device registers seems to be not reliable, the chip
> revision is sometimes detected wrongly (0 instead of expected 1).
> 
> Ensure that the chip reset is performed via reset GPIO and then
> wait for 'Device Ready' status in HW_CFG register before doing
> any register initializations.
> 
> [...]

Here is the summary with links:
  - [net,v4] net: dsa: lan9303: ensure chip reset and wait for READY status
    https://git.kernel.org/netdev/net/c/5c14e51d2d7d

You are awesome, thank you!

Patch

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 268949939636a..d246f95d57ecf 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -6,6 +6,7 @@ 
 #include <linux/module.h>
 #include <linux/gpio/consumer.h>
 #include <linux/regmap.h>
+#include <linux/iopoll.h>
 #include <linux/mutex.h>
 #include <linux/mii.h>
 #include <linux/of.h>
@@ -839,6 +840,8 @@  static void lan9303_handle_reset(struct lan9303 *chip)
 	if (!chip->reset_gpio)
 		return;
 
+	gpiod_set_value_cansleep(chip->reset_gpio, 1);
+
 	if (chip->reset_duration != 0)
 		msleep(chip->reset_duration);
 
@@ -864,8 +867,34 @@  static int lan9303_disable_processing(struct lan9303 *chip)
 static int lan9303_check_device(struct lan9303 *chip)
 {
 	int ret;
+	int err;
 	u32 reg;
 
+	/* In I2C-managed configurations this polling loop will clash with
+	 * switch's reading of EEPROM right after reset and this behaviour is
+	 * not configurable. While lan9303_read() already has quite long retry
+	 * timeout, seems not all cases are being detected as arbitration error.
+	 *
+	 * According to datasheet, EEPROM loader has 30ms timeout (in case of
+	 * missing EEPROM).
+	 *
+	 * Loading of the largest supported EEPROM is expected to take at least
+	 * 5.9s.
+	 */
+	err = read_poll_timeout(lan9303_read, ret,
+				!ret && reg & LAN9303_HW_CFG_READY,
+				20000, 6000000, false,
+				chip->regmap, LAN9303_HW_CFG, &reg);
+	if (ret) {
+		dev_err(chip->dev, "failed to read HW_CFG reg: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+	if (err) {
+		dev_err(chip->dev, "HW_CFG not ready: 0x%08x\n", reg);
+		return err;
+	}
+
 	ret = lan9303_read(chip->regmap, LAN9303_CHIP_REV, &reg);
 	if (ret) {
 		dev_err(chip->dev, "failed to read chip revision register: %d\n",