diff mbox series

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

Message ID 20241002171230.1502325-1-alexander.sverdlin@siemens.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net,v2] net: dsa: lan9303: ensure chip reset and wait for READY status | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present fail Series targets non-next tree, but doesn't contain any Fixes tags
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 9 this patch: 9
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 3 maintainers not CCed: pabeni@redhat.com kuba@kernel.org edumazet@google.com
netdev/build_clang success Errors and warnings before: 9 this patch: 9
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 44 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-10-03--18-00 (tests: 772)

Commit Message

A. Sverdlin Oct. 2, 2024, 5:12 p.m. UTC 
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(+)

Comments

Vladimir Oltean Oct. 3, 2024, 9:15 p.m. UTC | #1 
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
>
A. Sverdlin Oct. 4, 2024, 7:26 a.m. UTC | #2 
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?
Vladimir Oltean Oct. 4, 2024, 8:16 a.m. UTC | #3 
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.
diff mbox series

Patch

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",