diff mbox series

[net-next,v4,3/3] net: dsa: realtek: support reset controller

Message ID 20240219-realtek-reset-v4-3-858b82a29503@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: dsa: realtek: support reset controller and update docs | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 940 this patch: 940
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 10 of 10 maintainers
netdev/build_clang success Errors and warnings before: 957 this patch: 957
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 957 this patch: 957
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 90 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-02-22--00-00 (tests: 1455)

Commit Message

Luiz Angelo Daros de Luca Feb. 19, 2024, 11:44 p.m. UTC 
Add support for resetting the device using a reset controller,
complementing the existing GPIO reset functionality (reset-gpios).

Although the reset is optional and the driver performs a soft reset
during setup, if the initial reset pin state was asserted, the driver
will not detect the device until the reset is deasserted.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/net/dsa/realtek/realtek.h |  2 ++
 drivers/net/dsa/realtek/rtl83xx.c | 46 ++++++++++++++++++++++++++++++++++-----
 drivers/net/dsa/realtek/rtl83xx.h |  2 ++
 3 files changed, 45 insertions(+), 5 deletions(-)

Comments

Alvin Šipraga Feb. 20, 2024, 10:26 a.m. UTC | #1 
On Mon, Feb 19, 2024 at 08:44:42PM -0300, Luiz Angelo Daros de Luca wrote:
> +void rtl83xx_reset_assert(struct realtek_priv *priv)
> +{
> +	int ret;
> +
> +	ret = reset_control_assert(priv->reset_ctl);
> +	if (!ret)
> +		return;

If priv->reset_ctl is NULL - i.e. if no DT property is specified - then
this will always return early and the GPIO will not be asserted.

> +
> +	dev_warn(priv->dev,
> +		 "Failed to assert the switch reset control: %pe\n",
> +		 ERR_PTR(ret));

You only log an error if the reset controller assert fails, but not if
the GPIO assert fails. Why the unequal treatment?

I suggest keeping it simple:

void rtl83xx_reset_assert(struct realtek_priv *priv)
{
  int ret;

  ret = reset_control_assert(priv->reset_ctl);
  if (ret)
    dev_warn(priv->dev, "failed to assert reset control: %d\n", ret);

  ret = gpiod_set_value(priv->reset, false);
  if (ret)
    dev_warn(priv->dev, "failed to assert reset GPIO: %d\n", ret);
}

or even drop the warnings altogether.

> +
> +	gpiod_set_value(priv->reset, true);
> +}
> +
> +void rtl83xx_reset_deassert(struct realtek_priv *priv)
> +{
> +	int ret;
> +
> +	ret = reset_control_deassert(priv->reset_ctl);
> +	if (!ret)
> +		return;
> +
> +	dev_warn(priv->dev,
> +		 "Failed to deassert the switch reset control: %pe\n",
> +		 ERR_PTR(ret));
> +
> +	gpiod_set_value(priv->reset, false);
> +}

Same comments apply to this function. Just deassert both.

> +
>  MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>");
>  MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
>  MODULE_DESCRIPTION("Realtek DSA switches common module");
> diff --git a/drivers/net/dsa/realtek/rtl83xx.h b/drivers/net/dsa/realtek/rtl83xx.h
> index 0ddff33df6b0..c8a0ff8fd75e 100644
> --- a/drivers/net/dsa/realtek/rtl83xx.h
> +++ b/drivers/net/dsa/realtek/rtl83xx.h
> @@ -18,5 +18,7 @@ int rtl83xx_register_switch(struct realtek_priv *priv);
>  void rtl83xx_unregister_switch(struct realtek_priv *priv);
>  void rtl83xx_shutdown(struct realtek_priv *priv);
>  void rtl83xx_remove(struct realtek_priv *priv);
> +void rtl83xx_reset_assert(struct realtek_priv *priv);
> +void rtl83xx_reset_deassert(struct realtek_priv *priv);
>  
>  #endif /* _RTL83XX_H */
> 
> -- 
> 2.43.0
>
Luiz Angelo Daros de Luca Feb. 20, 2024, 12:22 p.m. UTC | #2 
Hi Alvin,

> On Mon, Feb 19, 2024 at 08:44:42PM -0300, Luiz Angelo Daros de Luca wrote:
> > +void rtl83xx_reset_assert(struct realtek_priv *priv)
> > +{
> > +     int ret;
> > +
> > +     ret = reset_control_assert(priv->reset_ctl);
> > +     if (!ret)
> > +             return;
>
> If priv->reset_ctl is NULL - i.e. if no DT property is specified - then
> this will always return early and the GPIO will not be asserted.

I made a mistake. I should be

if (ret) {
          dev_warn...
}

not returning on error (as you suggested below).

I was sure I was doing just that... I was surprised to see it as it
is.  I'll recheck my branch with all the integrated changes. It passed
my tests as when reset is missed, it normally does not matter. Thanks
for the catch.

>
> > +
> > +     dev_warn(priv->dev,
> > +              "Failed to assert the switch reset control: %pe\n",
> > +              ERR_PTR(ret));
>
> You only log an error if the reset controller assert fails, but not if
> the GPIO assert fails. Why the unequal treatment?

Because it does not return a value. There is no way to tell if it failed.

>
> I suggest keeping it simple:
>
> void rtl83xx_reset_assert(struct realtek_priv *priv)
> {
>   int ret;
>
>   ret = reset_control_assert(priv->reset_ctl);
>   if (ret)
>     dev_warn(priv->dev, "failed to assert reset control: %d\n", ret);
>
>   ret = gpiod_set_value(priv->reset, false);
>   if (ret)
>     dev_warn(priv->dev, "failed to assert reset GPIO: %d\n", ret);
> }
>
> or even drop the warnings altogether.
>
> > +
> > +     gpiod_set_value(priv->reset, true);
> > +}
> > +
> > +void rtl83xx_reset_deassert(struct realtek_priv *priv)
> > +{
> > +     int ret;
> > +
> > +     ret = reset_control_deassert(priv->reset_ctl);
> > +     if (!ret)
> > +             return;
> > +
> > +     dev_warn(priv->dev,
> > +              "Failed to deassert the switch reset control: %pe\n",
> > +              ERR_PTR(ret));
> > +
> > +     gpiod_set_value(priv->reset, false);
> > +}
>
> Same comments apply to this function. Just deassert both.
>
> > +
> >  MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>");
> >  MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
> >  MODULE_DESCRIPTION("Realtek DSA switches common module");
> > diff --git a/drivers/net/dsa/realtek/rtl83xx.h b/drivers/net/dsa/realtek/rtl83xx.h
> > index 0ddff33df6b0..c8a0ff8fd75e 100644
> > --- a/drivers/net/dsa/realtek/rtl83xx.h
> > +++ b/drivers/net/dsa/realtek/rtl83xx.h
> > @@ -18,5 +18,7 @@ int rtl83xx_register_switch(struct realtek_priv *priv);
> >  void rtl83xx_unregister_switch(struct realtek_priv *priv);
> >  void rtl83xx_shutdown(struct realtek_priv *priv);
> >  void rtl83xx_remove(struct realtek_priv *priv);
> > +void rtl83xx_reset_assert(struct realtek_priv *priv);
> > +void rtl83xx_reset_deassert(struct realtek_priv *priv);
> >
> >  #endif /* _RTL83XX_H */
> >
> > --
> > 2.43.0
> >

Regards,

Luiz
Alvin Šipraga Feb. 20, 2024, 1:30 p.m. UTC | #3 
On Tue, Feb 20, 2024 at 09:22:44AM -0300, Luiz Angelo Daros de Luca wrote:
> Hi Alvin,
> 
> > On Mon, Feb 19, 2024 at 08:44:42PM -0300, Luiz Angelo Daros de Luca wrote:
> > > +void rtl83xx_reset_assert(struct realtek_priv *priv)
> > > +{
> > > +     int ret;
> > > +
> > > +     ret = reset_control_assert(priv->reset_ctl);
> > > +     if (!ret)
> > > +             return;
> >
> > If priv->reset_ctl is NULL - i.e. if no DT property is specified - then
> > this will always return early and the GPIO will not be asserted.
> 
> I made a mistake. I should be
> 
> if (ret) {
>           dev_warn...
> }
> 
> not returning on error (as you suggested below).
> 
> I was sure I was doing just that... I was surprised to see it as it
> is.  I'll recheck my branch with all the integrated changes. It passed
> my tests as when reset is missed, it normally does not matter. Thanks
> for the catch.
> 
> >
> > > +
> > > +     dev_warn(priv->dev,
> > > +              "Failed to assert the switch reset control: %pe\n",
> > > +              ERR_PTR(ret));
> >
> > You only log an error if the reset controller assert fails, but not if
> > the GPIO assert fails. Why the unequal treatment?
> 
> Because it does not return a value. There is no way to tell if it failed.

Ah ok, nevermind that part then.

BTW, please use gpiod_set_value_cansleep(). With that I think this is good.
Alvin Šipraga Feb. 20, 2024, 1:42 p.m. UTC | #4 
On Tue, Feb 20, 2024 at 01:30:33PM +0000, Alvin Šipraga wrote:
> On Tue, Feb 20, 2024 at 09:22:44AM -0300, Luiz Angelo Daros de Luca wrote:
> > Hi Alvin,
> > 
> > > On Mon, Feb 19, 2024 at 08:44:42PM -0300, Luiz Angelo Daros de Luca wrote:
> > > > +void rtl83xx_reset_assert(struct realtek_priv *priv)
> > > > +{
> > > > +     int ret;
> > > > +
> > > > +     ret = reset_control_assert(priv->reset_ctl);
> > > > +     if (!ret)
> > > > +             return;
> > >
> > > If priv->reset_ctl is NULL - i.e. if no DT property is specified - then
> > > this will always return early and the GPIO will not be asserted.
> > 
> > I made a mistake. I should be
> > 
> > if (ret) {
> >           dev_warn...
> > }
> > 
> > not returning on error (as you suggested below).
> > 
> > I was sure I was doing just that... I was surprised to see it as it
> > is.  I'll recheck my branch with all the integrated changes. It passed
> > my tests as when reset is missed, it normally does not matter. Thanks
> > for the catch.
> > 
> > >
> > > > +
> > > > +     dev_warn(priv->dev,
> > > > +              "Failed to assert the switch reset control: %pe\n",
> > > > +              ERR_PTR(ret));
> > >
> > > You only log an error if the reset controller assert fails, but not if
> > > the GPIO assert fails. Why the unequal treatment?
> > 
> > Because it does not return a value. There is no way to tell if it failed.
> 
> Ah ok, nevermind that part then.
> 
> BTW, please use gpiod_set_value_cansleep(). With that I think this is good.

OK, actually the original code wasn't doing that, so not crucial for this
change. It can be done in a follow-up.
diff mbox series

Patch

diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
index b80bfde1ad04..e0b1aa01337b 100644
--- a/drivers/net/dsa/realtek/realtek.h
+++ b/drivers/net/dsa/realtek/realtek.h
@@ -12,6 +12,7 @@ 
 #include <linux/platform_device.h>
 #include <linux/gpio/consumer.h>
 #include <net/dsa.h>
+#include <linux/reset.h>
 
 #define REALTEK_HW_STOP_DELAY		25	/* msecs */
 #define REALTEK_HW_START_DELAY		100	/* msecs */
@@ -48,6 +49,7 @@  struct rtl8366_vlan_4k {
 
 struct realtek_priv {
 	struct device		*dev;
+	struct reset_control    *reset_ctl;
 	struct gpio_desc	*reset;
 	struct gpio_desc	*mdc;
 	struct gpio_desc	*mdio;
diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c
index 801873754df2..8262ec5032a4 100644
--- a/drivers/net/dsa/realtek/rtl83xx.c
+++ b/drivers/net/dsa/realtek/rtl83xx.c
@@ -184,6 +184,13 @@  rtl83xx_probe(struct device *dev,
 						    "realtek,disable-leds");
 
 	/* TODO: if power is software controlled, set up any regulators here */
+	priv->reset_ctl = devm_reset_control_get_optional(dev, NULL);
+	if (IS_ERR(priv->reset_ctl)) {
+		ret = PTR_ERR(priv->reset_ctl);
+		dev_err_probe(dev, ret, "failed to get reset control\n");
+		return ERR_CAST(priv->reset_ctl);
+	}
+
 	priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(priv->reset)) {
 		dev_err(dev, "failed to get RESET GPIO\n");
@@ -192,11 +199,11 @@  rtl83xx_probe(struct device *dev,
 
 	dev_set_drvdata(dev, priv);
 
-	if (priv->reset) {
-		gpiod_set_value(priv->reset, 1);
+	if (priv->reset_ctl || priv->reset) {
+		rtl83xx_reset_assert(priv);
 		dev_dbg(dev, "asserted RESET\n");
 		msleep(REALTEK_HW_STOP_DELAY);
-		gpiod_set_value(priv->reset, 0);
+		rtl83xx_reset_deassert(priv);
 		msleep(REALTEK_HW_START_DELAY);
 		dev_dbg(dev, "deasserted RESET\n");
 	}
@@ -292,11 +299,40 @@  EXPORT_SYMBOL_NS_GPL(rtl83xx_shutdown, REALTEK_DSA);
 void rtl83xx_remove(struct realtek_priv *priv)
 {
 	/* leave the device reset asserted */
-	if (priv->reset)
-		gpiod_set_value(priv->reset, 1);
+	rtl83xx_reset_assert(priv);
 }
 EXPORT_SYMBOL_NS_GPL(rtl83xx_remove, REALTEK_DSA);
 
+void rtl83xx_reset_assert(struct realtek_priv *priv)
+{
+	int ret;
+
+	ret = reset_control_assert(priv->reset_ctl);
+	if (!ret)
+		return;
+
+	dev_warn(priv->dev,
+		 "Failed to assert the switch reset control: %pe\n",
+		 ERR_PTR(ret));
+
+	gpiod_set_value(priv->reset, true);
+}
+
+void rtl83xx_reset_deassert(struct realtek_priv *priv)
+{
+	int ret;
+
+	ret = reset_control_deassert(priv->reset_ctl);
+	if (!ret)
+		return;
+
+	dev_warn(priv->dev,
+		 "Failed to deassert the switch reset control: %pe\n",
+		 ERR_PTR(ret));
+
+	gpiod_set_value(priv->reset, false);
+}
+
 MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>");
 MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
 MODULE_DESCRIPTION("Realtek DSA switches common module");
diff --git a/drivers/net/dsa/realtek/rtl83xx.h b/drivers/net/dsa/realtek/rtl83xx.h
index 0ddff33df6b0..c8a0ff8fd75e 100644
--- a/drivers/net/dsa/realtek/rtl83xx.h
+++ b/drivers/net/dsa/realtek/rtl83xx.h
@@ -18,5 +18,7 @@  int rtl83xx_register_switch(struct realtek_priv *priv);
 void rtl83xx_unregister_switch(struct realtek_priv *priv);
 void rtl83xx_shutdown(struct realtek_priv *priv);
 void rtl83xx_remove(struct realtek_priv *priv);
+void rtl83xx_reset_assert(struct realtek_priv *priv);
+void rtl83xx_reset_deassert(struct realtek_priv *priv);
 
 #endif /* _RTL83XX_H */