| Message ID | 20241004090332.3252564-1-alexander.sverdlin@siemens.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v3] net: dsa: lan9303: ensure chip reset and wait for READY status | expand |
On Fri, Oct 04, 2024 at 11:03:31AM +0200, A. Sverdlin wrote: > 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: > v3: comment style, use "!ret" in stop condition, user-readable error code > v2: use read_poll_timeout() > > drivers/net/dsa/lan9303-core.c | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c > index 268949939636a..f478b55d4d297 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,30 @@ 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, > + !ret && reg & LAN9303_HW_CFG_READY, > + 20000, 6000000, false, > + chip->regmap, LAN9303_HW_CFG, ®)) { > + dev_err(chip->dev, "HW_CFG not ready: 0x%08x\n", reg); > + return -ETIMEDOUT; Please: int ret, err; err = read_poll_timeout(); if (err) ret = err; if (ret) { dev_err(chip->dev, "failed to read HW_CFG reg: %pe\n", ERR_PTR(ret)); return ret; } No hardcoding of -ETIMEDOUT either. > + } > + if (ret) { > + dev_err(chip->dev, "failed to read HW_CFG reg: %pe\n", > + ERR_PTR(ret)); > + return ret; > + } > + > ret = lan9303_read(chip->regmap, LAN9303_CHIP_REV, ®); > if (ret) { > dev_err(chip->dev, "failed to read chip revision register: %d\n", > -- > 2.46.2 >
Hi Vladimir! On Fri, 2024-10-04 at 13:18 +0300, Vladimir Oltean wrote: > > + if (read_poll_timeout(lan9303_read, ret, > > + !ret && reg & LAN9303_HW_CFG_READY, > > + 20000, 6000000, false, > > + chip->regmap, LAN9303_HW_CFG, ®)) { > > + dev_err(chip->dev, "HW_CFG not ready: 0x%08x\n", reg); > > + return -ETIMEDOUT; > > Please: > > int ret, err; > > err = read_poll_timeout(); > if (err) > ret = err; > if (ret) { > dev_err(chip->dev, "failed to read HW_CFG reg: %pe\n", > ERR_PTR(ret)); > return ret; > } > > No hardcoding of -ETIMEDOUT either. I've removed the hardcoded -ETIMEDOUT in the v4, but retained different error messages, because I believe, individual read errors could be more informative than generic -ETIMEDOUT, including, maybe even the actual register value.
diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c index 268949939636a..f478b55d4d297 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,30 @@ 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, + !ret && reg & LAN9303_HW_CFG_READY, + 20000, 6000000, false, + chip->regmap, LAN9303_HW_CFG, ®)) { + dev_err(chip->dev, "HW_CFG not ready: 0x%08x\n", reg); + return -ETIMEDOUT; + } + if (ret) { + dev_err(chip->dev, "failed to read HW_CFG reg: %pe\n", + ERR_PTR(ret)); + return ret; + } + ret = lan9303_read(chip->regmap, LAN9303_CHIP_REV, ®); if (ret) { dev_err(chip->dev, "failed to read chip revision register: %d\n",