[PATCH net-next] net: dsa: realtek: realtek-mdio: reset before setup

[PATCH net-next] net: dsa: realtek: realtek-mdio: reset before setup

[Luiz Angelo Daros de Luca]

Some devices, like the switch in Banana Pi BPI R64 only starts to answer
after a HW reset. It is the same reset code from realtek-smi.

Reported-by: Frank Wunderlich <frank-w@public-files.de>
Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/realtek-mdio.c | 19 +++++++++++++++++++
 drivers/net/dsa/realtek/realtek-smi.c  |  6 ++----
 drivers/net/dsa/realtek/realtek.h      |  9 ++++++---
 3 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index e6e3c1769166..78b419a6cb01 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -152,6 +152,21 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
 	/* TODO: if power is software controlled, set up any regulators here */
 	priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds");
 
+	/* Assert then deassert RESET */
+	priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(priv->reset)) {
+		dev_err(dev, "failed to get RESET GPIO\n");
+		return PTR_ERR(priv->reset);
+	}
+
+	if (priv->reset) {
+		dev_info(dev, "asserted RESET\n");
+		msleep(REALTEK_HW_STOP_DELAY);
+		gpiod_set_value(priv->reset, 0);
+		msleep(REALTEK_HW_START_DELAY);
+		dev_info(dev, "deasserted RESET\n");
+	}
+
 	ret = priv->ops->detect(priv);
 	if (ret) {
 		dev_err(dev, "unable to detect switch\n");
@@ -183,6 +198,10 @@ static void realtek_mdio_remove(struct mdio_device *mdiodev)
 	if (!priv)
 		return;
 
+	/* leave the device reset asserted */
+	if (priv->reset)
+		gpiod_set_value(priv->reset, 1);
+
 	dsa_unregister_switch(priv->ds);
 
 	dev_set_drvdata(&mdiodev->dev, NULL);
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index a849b5cbb4e4..cada5386f6a2 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -43,8 +43,6 @@
 #include "realtek.h"
 
 #define REALTEK_SMI_ACK_RETRY_COUNT		5
-#define REALTEK_SMI_HW_STOP_DELAY		25	/* msecs */
-#define REALTEK_SMI_HW_START_DELAY		100	/* msecs */
 
 static inline void realtek_smi_clk_delay(struct realtek_priv *priv)
 {
@@ -426,9 +424,9 @@ static int realtek_smi_probe(struct platform_device *pdev)
 		dev_err(dev, "failed to get RESET GPIO\n");
 		return PTR_ERR(priv->reset);
 	}
-	msleep(REALTEK_SMI_HW_STOP_DELAY);
+	msleep(REALTEK_HW_STOP_DELAY);
 	gpiod_set_value(priv->reset, 0);
-	msleep(REALTEK_SMI_HW_START_DELAY);
+	msleep(REALTEK_HW_START_DELAY);
 	dev_info(dev, "deasserted RESET\n");
 
 	/* Fetch MDIO pins */
diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
index ed5abf6cb3d6..e7d3e1bcf8b8 100644
--- a/drivers/net/dsa/realtek/realtek.h
+++ b/drivers/net/dsa/realtek/realtek.h
@@ -5,14 +5,17 @@
  * Copyright (C) 2009-2010 Gabor Juhos <juhosg@openwrt.org>
  */
 
-#ifndef _REALTEK_SMI_H
-#define _REALTEK_SMI_H
+#ifndef _REALTEK_H
+#define _REALTEK_H
 
 #include <linux/phy.h>
 #include <linux/platform_device.h>
 #include <linux/gpio/consumer.h>
 #include <net/dsa.h>
 
+#define REALTEK_HW_STOP_DELAY		25	/* msecs */
+#define REALTEK_HW_START_DELAY		100	/* msecs */
+
 struct realtek_ops;
 struct dentry;
 struct inode;
@@ -142,4 +145,4 @@ void rtl8366_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data);
 extern const struct realtek_variant rtl8366rb_variant;
 extern const struct realtek_variant rtl8365mb_variant;
 
-#endif /*  _REALTEK_SMI_H */
+#endif /*  _REALTEK_H */
-- 
2.35.1

Re: [PATCH net-next] net: dsa: realtek: realtek-mdio: reset before setup

[Frank Wunderlich]

Am 11. Februar 2022 06:14:04 MEZ schrieb Luiz Angelo Daros de Luca <luizluca@gmail.com>:
>Some devices, like the switch in Banana Pi BPI R64 only starts to
>answer
>after a HW reset. It is the same reset code from realtek-smi.
>
>Reported-by: Frank Wunderlich <frank-w@public-files.de>
>Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
>---
> drivers/net/dsa/realtek/realtek-mdio.c | 19 +++++++++++++++++++
> drivers/net/dsa/realtek/realtek-smi.c  |  6 ++----
> drivers/net/dsa/realtek/realtek.h      |  9 ++++++---
> 3 files changed, 27 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/net/dsa/realtek/realtek-mdio.c
>b/drivers/net/dsa/realtek/realtek-mdio.c
>index e6e3c1769166..78b419a6cb01 100644
>--- a/drivers/net/dsa/realtek/realtek-mdio.c
>+++ b/drivers/net/dsa/realtek/realtek-mdio.c
>@@ -152,6 +152,21 @@ static int realtek_mdio_probe(struct mdio_device
>*mdiodev)
>	/* TODO: if power is software controlled, set up any regulators here
>*/
>	priv->leds_disabled = of_property_read_bool(np,
>"realtek,disable-leds");
> 
>+	/* Assert then deassert RESET */
>+	priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
>+	if (IS_ERR(priv->reset)) {
>+		dev_err(dev, "failed to get RESET GPIO\n");
>+		return PTR_ERR(priv->reset);
>+	}
>+
>+	if (priv->reset) {
>+		dev_info(dev, "asserted RESET\n");
>+		msleep(REALTEK_HW_STOP_DELAY);
>+		gpiod_set_value(priv->reset, 0);
>+		msleep(REALTEK_HW_START_DELAY);
>+		dev_info(dev, "deasserted RESET\n");
>+	}
>+
> 	ret = priv->ops->detect(priv);
> 	if (ret) {
> 		dev_err(dev, "unable to detect switch\n");
>@@ -183,6 +198,10 @@ static void realtek_mdio_remove(struct mdio_device
>*mdiodev)
> 	if (!priv)
> 		return;
> 
>+	/* leave the device reset asserted */
>+	if (priv->reset)
>+		gpiod_set_value(priv->reset, 1);
>+
> 	dsa_unregister_switch(priv->ds);
> 
> 	dev_set_drvdata(&mdiodev->dev, NULL);
>diff --git a/drivers/net/dsa/realtek/realtek-smi.c
>b/drivers/net/dsa/realtek/realtek-smi.c
>index a849b5cbb4e4..cada5386f6a2 100644
>--- a/drivers/net/dsa/realtek/realtek-smi.c
>+++ b/drivers/net/dsa/realtek/realtek-smi.c
>@@ -43,8 +43,6 @@
> #include "realtek.h"
> 
> #define REALTEK_SMI_ACK_RETRY_COUNT		5
>-#define REALTEK_SMI_HW_STOP_DELAY		25	/* msecs */
>-#define REALTEK_SMI_HW_START_DELAY		100	/* msecs */
> 
> static inline void realtek_smi_clk_delay(struct realtek_priv *priv)
> {
>@@ -426,9 +424,9 @@ static int realtek_smi_probe(struct platform_device
>*pdev)
> 		dev_err(dev, "failed to get RESET GPIO\n");
> 		return PTR_ERR(priv->reset);
> 	}
>-	msleep(REALTEK_SMI_HW_STOP_DELAY);
>+	msleep(REALTEK_HW_STOP_DELAY);
> 	gpiod_set_value(priv->reset, 0);
>-	msleep(REALTEK_SMI_HW_START_DELAY);
>+	msleep(REALTEK_HW_START_DELAY);
> 	dev_info(dev, "deasserted RESET\n");
> 
> 	/* Fetch MDIO pins */
>diff --git a/drivers/net/dsa/realtek/realtek.h
>b/drivers/net/dsa/realtek/realtek.h
>index ed5abf6cb3d6..e7d3e1bcf8b8 100644
>--- a/drivers/net/dsa/realtek/realtek.h
>+++ b/drivers/net/dsa/realtek/realtek.h
>@@ -5,14 +5,17 @@
>  * Copyright (C) 2009-2010 Gabor Juhos <juhosg@openwrt.org>
>  */
> 
>-#ifndef _REALTEK_SMI_H
>-#define _REALTEK_SMI_H
>+#ifndef _REALTEK_H
>+#define _REALTEK_H
> 
> #include <linux/phy.h>
> #include <linux/platform_device.h>
> #include <linux/gpio/consumer.h>
> #include <net/dsa.h>
> 
>+#define REALTEK_HW_STOP_DELAY		25	/* msecs */
>+#define REALTEK_HW_START_DELAY		100	/* msecs */
>+
> struct realtek_ops;
> struct dentry;
> struct inode;
>@@ -142,4 +145,4 @@ void rtl8366_get_ethtool_stats(struct dsa_switch
>*ds, int port, uint64_t *data);
> extern const struct realtek_variant rtl8366rb_variant;
> extern const struct realtek_variant rtl8365mb_variant;
> 
>-#endif /*  _REALTEK_SMI_H */
>+#endif /*  _REALTEK_H */

Tested on Bpi-r64 v0.1. After this reset switch gets recognized. Before mdio-read is always 0.

Tested-by: Frank Wunderlich <frank-w@public-files.de>
regards Frank

Re: [PATCH net-next] net: dsa: realtek: realtek-mdio: reset before setup

[Alvin Šipraga]

Luiz Angelo Daros de Luca <luizluca@gmail.com> writes:

> Some devices, like the switch in Banana Pi BPI R64 only starts to answer
> after a HW reset. It is the same reset code from realtek-smi.
>
> Reported-by: Frank Wunderlich <frank-w@public-files.de>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---
>  drivers/net/dsa/realtek/realtek-mdio.c | 19 +++++++++++++++++++
>  drivers/net/dsa/realtek/realtek-smi.c  |  6 ++----
>  drivers/net/dsa/realtek/realtek.h      |  9 ++++++---
>  3 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index e6e3c1769166..78b419a6cb01 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -152,6 +152,21 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
>  	/* TODO: if power is software controlled, set up any regulators here */
>  	priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds");
>  
> +	/* Assert then deassert RESET */
> +	priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(priv->reset)) {
> +		dev_err(dev, "failed to get RESET GPIO\n");
> +		return PTR_ERR(priv->reset);
> +	}
> +
> +	if (priv->reset) {

gpiod_set_value seems tolerant of a NULL gpio_desc pointer, but perhaps
you would like to add the same if statement to realtek-smi so that it
doesn't pretend to reset the chip when the reset GPIO is absent?

> +		dev_info(dev, "asserted RESET\n");
> +		msleep(REALTEK_HW_STOP_DELAY);
> +		gpiod_set_value(priv->reset, 0);
> +		msleep(REALTEK_HW_START_DELAY);
> +		dev_info(dev, "deasserted RESET\n");
> +	}
> +
>  	ret = priv->ops->detect(priv);
>  	if (ret) {
>  		dev_err(dev, "unable to detect switch\n");
> @@ -183,6 +198,10 @@ static void realtek_mdio_remove(struct mdio_device *mdiodev)
>  	if (!priv)
>  		return;
>  
> +	/* leave the device reset asserted */
> +	if (priv->reset)
> +		gpiod_set_value(priv->reset, 1);
> +
>  	dsa_unregister_switch(priv->ds);

Wouldn't you prefer to reset the chip after dsa_unregister_switch()?

Otherwise:

Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

>  
>  	dev_set_drvdata(&mdiodev->dev, NULL);
> diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> index a849b5cbb4e4..cada5386f6a2 100644
> --- a/drivers/net/dsa/realtek/realtek-smi.c
> +++ b/drivers/net/dsa/realtek/realtek-smi.c
> @@ -43,8 +43,6 @@
>  #include "realtek.h"
>  
>  #define REALTEK_SMI_ACK_RETRY_COUNT		5
> -#define REALTEK_SMI_HW_STOP_DELAY		25	/* msecs */
> -#define REALTEK_SMI_HW_START_DELAY		100	/* msecs */
>  
>  static inline void realtek_smi_clk_delay(struct realtek_priv *priv)
>  {
> @@ -426,9 +424,9 @@ static int realtek_smi_probe(struct platform_device *pdev)
>  		dev_err(dev, "failed to get RESET GPIO\n");
>  		return PTR_ERR(priv->reset);
>  	}
> -	msleep(REALTEK_SMI_HW_STOP_DELAY);
> +	msleep(REALTEK_HW_STOP_DELAY);
>  	gpiod_set_value(priv->reset, 0);
> -	msleep(REALTEK_SMI_HW_START_DELAY);
> +	msleep(REALTEK_HW_START_DELAY);
>  	dev_info(dev, "deasserted RESET\n");
>  
>  	/* Fetch MDIO pins */
> diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
> index ed5abf6cb3d6..e7d3e1bcf8b8 100644
> --- a/drivers/net/dsa/realtek/realtek.h
> +++ b/drivers/net/dsa/realtek/realtek.h
> @@ -5,14 +5,17 @@
>   * Copyright (C) 2009-2010 Gabor Juhos <juhosg@openwrt.org>
>   */
>  
> -#ifndef _REALTEK_SMI_H
> -#define _REALTEK_SMI_H
> +#ifndef _REALTEK_H
> +#define _REALTEK_H
>  
>  #include <linux/phy.h>
>  #include <linux/platform_device.h>
>  #include <linux/gpio/consumer.h>
>  #include <net/dsa.h>
>  
> +#define REALTEK_HW_STOP_DELAY		25	/* msecs */
> +#define REALTEK_HW_START_DELAY		100	/* msecs */
> +
>  struct realtek_ops;
>  struct dentry;
>  struct inode;
> @@ -142,4 +145,4 @@ void rtl8366_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data);
>  extern const struct realtek_variant rtl8366rb_variant;
>  extern const struct realtek_variant rtl8365mb_variant;
>  
> -#endif /*  _REALTEK_SMI_H */
> +#endif /*  _REALTEK_H */

Re: [PATCH net-next] net: dsa: realtek: realtek-mdio: reset before setup

[Linus Walleij]

On Fri, Feb 11, 2022 at 6:14 AM Luiz Angelo Daros de Luca
<luizluca@gmail.com> wrote:

> Some devices, like the switch in Banana Pi BPI R64 only starts to answer
> after a HW reset. It is the same reset code from realtek-smi.
>
> Reported-by: Frank Wunderlich <frank-w@public-files.de>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>

Looks good to me!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH net-next] net: dsa: realtek: realtek-mdio: reset before setup

[Linus Walleij]

On Fri, Feb 11, 2022 at 10:54 AM Alvin Šipraga <ALSI@bang-olufsen.dk> wrote:
> Luiz Angelo Daros de Luca <luizluca@gmail.com> writes:

> > +     if (priv->reset) {
>
> gpiod_set_value seems tolerant of a NULL gpio_desc pointer, but perhaps
> you would like to add the same if statement to realtek-smi so that it
> doesn't pretend to reset the chip when the reset GPIO is absent?

That is fine by me and you can keep my review tag if you also
add this.

Yours,
Linus Walleij

Re: [PATCH net-next] net: dsa: realtek: realtek-mdio: reset before setup

[Andrew Lunn]

> diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
> index ed5abf6cb3d6..e7d3e1bcf8b8 100644
> --- a/drivers/net/dsa/realtek/realtek.h
> +++ b/drivers/net/dsa/realtek/realtek.h
> @@ -5,14 +5,17 @@
>   * Copyright (C) 2009-2010 Gabor Juhos <juhosg@openwrt.org>
>   */
>  
> -#ifndef _REALTEK_SMI_H
> -#define _REALTEK_SMI_H
> +#ifndef _REALTEK_H
> +#define _REALTEK_H
  
 
> -#endif /*  _REALTEK_SMI_H */
> +#endif /*  _REALTEK_H */

These two hunks should probably be in a separate patch.  At minimum,
please mention it is in the commit message.

       Andrew

Re: [PATCH net-next] net: dsa: realtek: realtek-mdio: reset before setup

[Florian Fainelli]

On 2/10/22 9:14 PM, Luiz Angelo Daros de Luca wrote:
> Some devices, like the switch in Banana Pi BPI R64 only starts to answer
> after a HW reset. It is the same reset code from realtek-smi.
> 
> Reported-by: Frank Wunderlich <frank-w@public-files.de>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---

[snip]

>  	ret = priv->ops->detect(priv);
>  	if (ret) {
>  		dev_err(dev, "unable to detect switch\n");
> @@ -183,6 +198,10 @@ static void realtek_mdio_remove(struct mdio_device *mdiodev)
>  	if (!priv)
>  		return;
>  
> +	/* leave the device reset asserted */
> +	if (priv->reset)
> +		gpiod_set_value(priv->reset, 1);
> +
>  	dsa_unregister_switch(priv->ds);
>  
>  	dev_set_drvdata(&mdiodev->dev, NULL);
> diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> index a849b5cbb4e4..cada5386f6a2 100644
> --- a/drivers/net/dsa/realtek/realtek-smi.c
> +++ b/drivers/net/dsa/realtek/realtek-smi.c
> @@ -43,8 +43,6 @@
>  #include "realtek.h"
>  
>  #define REALTEK_SMI_ACK_RETRY_COUNT		5
> -#define REALTEK_SMI_HW_STOP_DELAY		25	/* msecs */
> -#define REALTEK_SMI_HW_START_DELAY		100	/* msecs */
>  
>  static inline void realtek_smi_clk_delay(struct realtek_priv *priv)
>  {
> @@ -426,9 +424,9 @@ static int realtek_smi_probe(struct platform_device *pdev)
>  		dev_err(dev, "failed to get RESET GPIO\n");
>  		return PTR_ERR(priv->reset);
>  	}
> -	msleep(REALTEK_SMI_HW_STOP_DELAY);
> +	msleep(REALTEK_HW_STOP_DELAY);
>  	gpiod_set_value(priv->reset, 0);
> -	msleep(REALTEK_SMI_HW_START_DELAY);
> +	msleep(REALTEK_HW_START_DELAY);
>  	dev_info(dev, "deasserted RESET\n");

Maybe demote these to debug prints since they would show up every time
you load/unload the driver.
-- 
Florian

Re: [PATCH net-next] net: dsa: realtek: realtek-mdio: reset before setup

[Luiz Angelo Daros de Luca]

> >
> > +     /* leave the device reset asserted */
> > +     if (priv->reset)
> > +             gpiod_set_value(priv->reset, 1);
> > +
> >       dsa_unregister_switch(priv->ds);
>
> Wouldn't you prefer to reset the chip after dsa_unregister_switch()?

Thanks Alvin, you are right. Maybe a thread might ask something after
reset/before unregisters.