| Message ID | 20220211051403.3952-1-luizluca@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: dsa: realtek: realtek-mdio: reset before setup | expand |
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
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 */
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
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
> 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
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.
> > > > + /* 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.
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 */
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(-)