[PATCH 1/3] net: dsa: ksz9477: always toggle reset gpio if available

[PATCH 1/3] net: dsa: ksz9477: always toggle reset gpio if available

[Ahmad Fatoum]

The barebox driver currently only ensures that the switch is out of
reset, but doesn't actually trigger a reset pulse.

The Linux driver, on the other hand, holds reset active for 10ms and
then waits a whopping 100ms after reset deassertion.

This seems excessive by a thousandfold for at least KSZ9893R[1], whose
datasheet states:

  After the de-assertion of reset, it is recommended to wait a minimum of
  100µs before starting to program the device through any interface.

Therefore, let's pulse the reset like Linux does and give the switch 100
microseconds before continuing with the driver probe.

[1]: DS00002420E-page 176

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/net/ksz9477.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ksz9477.c b/drivers/net/ksz9477.c
index 1abea9d04017..5c5f4ec8d995 100644
--- a/drivers/net/ksz9477.c
+++ b/drivers/net/ksz9477.c
@@ -433,8 +433,10 @@ static int microchip_switch_probe(struct device *dev)
 	if (IS_ERR(gpio)) {
 		dev_warn(dev, "Failed to get 'reset' GPIO (ignored)\n");
 	} else if (gpio) {
-		mdelay(1);
+		gpiod_set_value(gpio, true);
+		mdelay(10);
 		gpiod_set_value(gpio, false);
+		udelay(100);
 	}
 
 	ksz_reset_switch(dev->priv);
-- 
2.39.2

[PATCH 2/3] net: dsa: ksz9477: report 0 as value when returning error

[Ahmad Fatoum]

Most register accesses via the ksz_read* functions don't expect that
they can fail and don't check for an error. This assumption is wrong if
there is a problem with the underlying transport, e.g. an I2C bus.

In that case, the functions would return an error code that mostly goes
unchecked and *val is set to an uninitialized value.

The original Linux driver suffers from a similar problem: There, *val is
only set on success, but if the read fails, there's a warning printed
to log, but still the uninitialized variable in the caller function will
be used instead.

This likely goes unnoticed, because GCC builds on Linux have
-Wno-maybe-uninitialized, but we have implicit -W-maybe-uninitialized in
barebox.

As we sync the code from Linux, one should probably fix this there
first, but let's improve the situation a bit by not reporting an
uninitialized value, but a deterministic zero.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/net/ksz_common.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ksz_common.h b/drivers/net/ksz_common.h
index 291488fe3485..44b5055ee397 100644
--- a/drivers/net/ksz_common.h
+++ b/drivers/net/ksz_common.h
@@ -20,7 +20,7 @@ struct ksz_switch {
 
 static inline int ksz_read8(struct ksz_switch *priv, u32 reg, u8 *val)
 {
-	unsigned int value;
+	unsigned int value = 0;
 	int ret = regmap_read(priv->regmap[0], reg, &value);
 
 	*val = value;
@@ -29,7 +29,7 @@ static inline int ksz_read8(struct ksz_switch *priv, u32 reg, u8 *val)
 
 static inline int ksz_read16(struct ksz_switch *priv, u32 reg, u16 *val)
 {
-	unsigned int value;
+	unsigned int value = 0;
 	int ret = regmap_read(priv->regmap[1], reg, &value);
 
 	*val = value;
@@ -38,7 +38,7 @@ static inline int ksz_read16(struct ksz_switch *priv, u32 reg, u16 *val)
 
 static inline int ksz_read32(struct ksz_switch *priv, u32 reg, u32 *val)
 {
-	unsigned int value;
+	unsigned int value = 0;
 	int ret = regmap_read(priv->regmap[2], reg, &value);
 
 	*val = value;
-- 
2.39.2

[PATCH 3/3] net: dsa: ksz9477: return negative error codes on PHY access failures

[Ahmad Fatoum]

The ksz register accessors return a negative error code if the
underlying transport, e.g. i2c, fails. We need to propagate this error
code, instead of reporting incorrect data.

For the case a PHY doesn't exist, reads on a physical bus return 0xFFFF
due to the pull-ups on the MDIO line, so we leave that as is.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/net/ksz9477.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ksz9477.c b/drivers/net/ksz9477.c
index 5c5f4ec8d995..950eb89c09c2 100644
--- a/drivers/net/ksz9477.c
+++ b/drivers/net/ksz9477.c
@@ -29,12 +29,15 @@ static int ksz9477_phy_read16(struct dsa_switch *ds, int addr, int reg)
 {
 	struct device *dev = ds->dev;
 	struct ksz_switch *priv = dev_get_priv(dev);
-	u16 val = 0xffff;
+	int ret;
+	u16 val;
 
 	if (addr >= priv->phy_port_cnt)
-		return val;
+		return 0xffff;
 
-	ksz_pread16(priv, addr, 0x100 + (reg << 1), &val);
+	ret = ksz_pread16(priv, addr, 0x100 + (reg << 1), &val);
+	if (ret)
+		return ret;
 
 	return val;
 }
@@ -52,9 +55,8 @@ static int ksz9477_phy_write16(struct dsa_switch *ds, int addr, int reg,
 	/* No gigabit support.  Do not write to this register. */
 	if (!(priv->features & GBIT_SUPPORT) && reg == MII_CTRL1000)
 		return 0;
-	ksz_pwrite16(priv, addr, 0x100 + (reg << 1), val);
 
-	return 0;
+	return ksz_pwrite16(priv, addr, 0x100 + (reg << 1), val);
 }
 
 static int ksz9477_switch_detect(struct ksz_switch *priv)
-- 
2.39.2

Re: [PATCH 1/3] net: dsa: ksz9477: always toggle reset gpio if available

[Ahmad Fatoum]

On 02.05.24 17:17, Ahmad Fatoum wrote:
> The barebox driver currently only ensures that the switch is out of
> reset, but doesn't actually trigger a reset pulse.
> 
> The Linux driver, on the other hand, holds reset active for 10ms and
> then waits a whopping 100ms after reset deassertion.
> 
> This seems excessive by a thousandfold for at least KSZ9893R[1], whose
> datasheet states:
> 
>   After the de-assertion of reset, it is recommended to wait a minimum of
>   100µs before starting to program the device through any interface.
> 
> Therefore, let's pulse the reset like Linux does and give the switch 100
> microseconds before continuing with the driver probe.
> 
> [1]: DS00002420E-page 176
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

I forgot to include the patch prefix, but IMO all three patches are
master material.

Cheers,
Ahmad

> ---
>  drivers/net/ksz9477.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ksz9477.c b/drivers/net/ksz9477.c
> index 1abea9d04017..5c5f4ec8d995 100644
> --- a/drivers/net/ksz9477.c
> +++ b/drivers/net/ksz9477.c
> @@ -433,8 +433,10 @@ static int microchip_switch_probe(struct device *dev)
>  	if (IS_ERR(gpio)) {
>  		dev_warn(dev, "Failed to get 'reset' GPIO (ignored)\n");
>  	} else if (gpio) {
> -		mdelay(1);
> +		gpiod_set_value(gpio, true);
> +		mdelay(10);
>  		gpiod_set_value(gpio, false);
> +		udelay(100);
>  	}
>  
>  	ksz_reset_switch(dev->priv);

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/3] net: dsa: ksz9477: always toggle reset gpio if available

[Sascha Hauer]

On Thu, 02 May 2024 17:17:55 +0200, Ahmad Fatoum wrote:
> The barebox driver currently only ensures that the switch is out of
> reset, but doesn't actually trigger a reset pulse.
> 
> The Linux driver, on the other hand, holds reset active for 10ms and
> then waits a whopping 100ms after reset deassertion.
> 
> This seems excessive by a thousandfold for at least KSZ9893R[1], whose
> datasheet states:
> 
> [...]

Applied, thanks!

[1/3] net: dsa: ksz9477: always toggle reset gpio if available
      https://git.pengutronix.de/cgit/barebox/commit/?id=372d21e79e30 (link may not be stable)
[2/3] net: dsa: ksz9477: report 0 as value when returning error
      https://git.pengutronix.de/cgit/barebox/commit/?id=6d42b2827243 (link may not be stable)
[3/3] net: dsa: ksz9477: return negative error codes on PHY access failures
      https://git.pengutronix.de/cgit/barebox/commit/?id=eefd990b4cfd (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>