[PATCH net v2] net: dsa: lan9303: ensure chip reset and wait for READY status

[PATCH net v2] net: dsa: lan9303: ensure chip reset and wait for READY status

[A. Sverdlin]

From: Anatolij Gustschin <agust@denx.de>

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 <agust@denx.de>
[alex: reworked using read_poll_timeout()]
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
Changelog:
v2: use read_poll_timeout()

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

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 268949939636a..3155ec1ab2517 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);
 
@@ -866,6 +869,29 @@ static int lan9303_check_device(struct lan9303 *chip)
 	int ret;
 	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.
+	 */
+	if (read_poll_timeout(lan9303_read, ret, reg & LAN9303_HW_CFG_READY,
+			      20000, 6000000, false,
+			      chip->regmap, LAN9303_HW_CFG, &reg)) {
+		dev_err(chip->dev, "HW_CFG not ready: 0x%08x\n", reg);
+		return -ENODEV;
+	}
+	if (ret) {
+		dev_err(chip->dev, "failed to read HW_CFG reg: %d\n", ret);
+		return ret;
+	}
+
 	ret = lan9303_read(chip->regmap, LAN9303_CHIP_REV, &reg);
 	if (ret) {
 		dev_err(chip->dev, "failed to read chip revision register: %d\n",
-- 
2.46.2

Re: [PATCH net v2] net: dsa: lan9303: ensure chip reset and wait for READY status

[Vladimir Oltean]

On Wed, Oct 02, 2024 at 07:12:28PM +0200, A. Sverdlin wrote:
> @@ -866,6 +869,29 @@ static int lan9303_check_device(struct lan9303 *chip)
>  	int ret;
>  	u32 reg;
>  
> +	/*
> +	 * In I2C-managed configurations this polling loop will clash with

netdev coding style is with comments like this: /* In I2C managed configurations...

> +	 * 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.

These arbitration errors happen only after reset? So in theory, after
this patch, we could remove the for() loop from lan9303_read()?

> +	 *
> +	 * 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.
> +	 */
> +	if (read_poll_timeout(lan9303_read, ret, reg & LAN9303_HW_CFG_READY,

Isn't "reg" uninitialized if "ret" is non-zero? So shouldn't be "ret"
also part of the break condition somehow?

> +			      20000, 6000000, false,
> +			      chip->regmap, LAN9303_HW_CFG, &reg)) {
> +		dev_err(chip->dev, "HW_CFG not ready: 0x%08x\n", reg);
> +		return -ENODEV;

What point is there to mangle the return code from read_poll_timeout()
(-ETIMEDOUT) into -ENODEV, instead of just propagating that?

> +	}
> +	if (ret) {
> +		dev_err(chip->dev, "failed to read HW_CFG reg: %d\n", ret);

%pe, ERR_PTR(ret) is nicer for the average, non-expert in errno.h user.
I see this driver isn't using it, so maybe there's an argument about
consistency, but there's a beginning for everything..

> +		return ret;
> +	}
> +
>  	ret = lan9303_read(chip->regmap, LAN9303_CHIP_REV, &reg);
>  	if (ret) {
>  		dev_err(chip->dev, "failed to read chip revision register: %d\n",
> -- 
> 2.46.2
> 

Re: [PATCH net v2] net: dsa: lan9303: ensure chip reset and wait for READY status

[Sverdlin, Alexander]

Thanks for the review Vladimir!

On Fri, 2024-10-04 at 00:15 +0300, Vladimir Oltean wrote:
> > +	 * 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.
> 
> These arbitration errors happen only after reset? So in theory, after
> this patch, we could remove the for() loop from lan9303_read()?

This is a good point! Shall I add the removal to a series for net or post the
removal separately for net-next?

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH net v2] net: dsa: lan9303: ensure chip reset and wait for READY status

[Vladimir Oltean]

On Fri, Oct 04, 2024 at 07:26:21AM +0000, Sverdlin, Alexander wrote:
> Thanks for the review Vladimir!
> 
> On Fri, 2024-10-04 at 00:15 +0300, Vladimir Oltean wrote:
> > > +	 * 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.
> > 
> > These arbitration errors happen only after reset? So in theory, after
> > this patch, we could remove the for() loop from lan9303_read()?
> 
> This is a good point! Shall I add the removal to a series for net or post the
> removal separately for net-next?

That would be net-next material, as long as they don't intersect functionally.
What you could do is confirm that this is the case indeed, and that
nothing needs to change in the read_poll_timeout() logic even with the
simplified lan9303_read().

If true, lan9303_read() will always return 0 at the first iteration
after this patch, and after the read_poll_timeout() breaks through.