* Re: [PATCH net-next] net: dsa: realtek: realtek-mdio: reset before setup
2022-02-11 5:14 [PATCH net-next] net: dsa: realtek: realtek-mdio: reset before setup Luiz Angelo Daros de Luca
@ 2022-02-11 6:09 ` Frank Wunderlich
2022-02-11 9:54 ` Alvin Šipraga
` (3 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Frank Wunderlich @ 2022-02-11 6:09 UTC (permalink / raw)
To: Luiz Angelo Daros de Luca, netdev
Cc: linus.walleij, andrew, vivien.didelot, f.fainelli, olteanv, davem,
kuba, alsi, arinc.unal
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net-next] net: dsa: realtek: realtek-mdio: reset before setup
2022-02-11 5:14 [PATCH net-next] net: dsa: realtek: realtek-mdio: reset before setup Luiz Angelo Daros de Luca
2022-02-11 6:09 ` Frank Wunderlich
@ 2022-02-11 9:54 ` Alvin Šipraga
2022-02-11 10:30 ` Linus Walleij
2022-02-11 23:12 ` Luiz Angelo Daros de Luca
2022-02-11 10:28 ` Linus Walleij
` (2 subsequent siblings)
4 siblings, 2 replies; 8+ messages in thread
From: Alvin Šipraga @ 2022-02-11 9:54 UTC (permalink / raw)
To: Luiz Angelo Daros de Luca
Cc: netdev@vger.kernel.org, linus.walleij@linaro.org, andrew@lunn.ch,
vivien.didelot@gmail.com, f.fainelli@gmail.com, olteanv@gmail.com,
davem@davemloft.net, kuba@kernel.org, arinc.unal@arinc9.com,
Frank Wunderlich
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 */
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net-next] net: dsa: realtek: realtek-mdio: reset before setup
2022-02-11 9:54 ` Alvin Šipraga
@ 2022-02-11 10:30 ` Linus Walleij
2022-02-11 23:12 ` Luiz Angelo Daros de Luca
1 sibling, 0 replies; 8+ messages in thread
From: Linus Walleij @ 2022-02-11 10:30 UTC (permalink / raw)
To: Alvin Šipraga
Cc: Luiz Angelo Daros de Luca, netdev@vger.kernel.org, andrew@lunn.ch,
vivien.didelot@gmail.com, f.fainelli@gmail.com, olteanv@gmail.com,
davem@davemloft.net, kuba@kernel.org, arinc.unal@arinc9.com,
Frank Wunderlich
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net-next] net: dsa: realtek: realtek-mdio: reset before setup
2022-02-11 9:54 ` Alvin Šipraga
2022-02-11 10:30 ` Linus Walleij
@ 2022-02-11 23:12 ` Luiz Angelo Daros de Luca
1 sibling, 0 replies; 8+ messages in thread
From: Luiz Angelo Daros de Luca @ 2022-02-11 23:12 UTC (permalink / raw)
To: Alvin Šipraga
Cc: netdev@vger.kernel.org, linus.walleij@linaro.org, andrew@lunn.ch,
vivien.didelot@gmail.com, f.fainelli@gmail.com, olteanv@gmail.com,
davem@davemloft.net, kuba@kernel.org, arinc.unal@arinc9.com,
Frank Wunderlich
> >
> > + /* 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] net: dsa: realtek: realtek-mdio: reset before setup
2022-02-11 5:14 [PATCH net-next] net: dsa: realtek: realtek-mdio: reset before setup Luiz Angelo Daros de Luca
2022-02-11 6:09 ` Frank Wunderlich
2022-02-11 9:54 ` Alvin Šipraga
@ 2022-02-11 10:28 ` Linus Walleij
2022-02-11 19:44 ` Andrew Lunn
2022-02-11 21:01 ` Florian Fainelli
4 siblings, 0 replies; 8+ messages in thread
From: Linus Walleij @ 2022-02-11 10:28 UTC (permalink / raw)
To: Luiz Angelo Daros de Luca
Cc: netdev, andrew, vivien.didelot, f.fainelli, olteanv, davem, kuba,
ALSI, arinc.unal, Frank Wunderlich
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] net: dsa: realtek: realtek-mdio: reset before setup
2022-02-11 5:14 [PATCH net-next] net: dsa: realtek: realtek-mdio: reset before setup Luiz Angelo Daros de Luca
` (2 preceding siblings ...)
2022-02-11 10:28 ` Linus Walleij
@ 2022-02-11 19:44 ` Andrew Lunn
2022-02-11 21:01 ` Florian Fainelli
4 siblings, 0 replies; 8+ messages in thread
From: Andrew Lunn @ 2022-02-11 19:44 UTC (permalink / raw)
To: Luiz Angelo Daros de Luca
Cc: netdev, linus.walleij, vivien.didelot, f.fainelli, olteanv, davem,
kuba, alsi, arinc.unal, Frank Wunderlich
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net-next] net: dsa: realtek: realtek-mdio: reset before setup
2022-02-11 5:14 [PATCH net-next] net: dsa: realtek: realtek-mdio: reset before setup Luiz Angelo Daros de Luca
` (3 preceding siblings ...)
2022-02-11 19:44 ` Andrew Lunn
@ 2022-02-11 21:01 ` Florian Fainelli
4 siblings, 0 replies; 8+ messages in thread
From: Florian Fainelli @ 2022-02-11 21:01 UTC (permalink / raw)
To: Luiz Angelo Daros de Luca, netdev
Cc: linus.walleij, andrew, vivien.didelot, olteanv, davem, kuba, alsi,
arinc.unal, Frank Wunderlich
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
^ permalink raw reply [flat|nested] 8+ messages in thread