[PATCH net-next v3 0/3] net: dsa: realtek: support reset controller and update docs

[PATCH net-next v3 0/3] net: dsa: realtek: support reset controller and update docs

[Luiz Angelo Daros de Luca]

The driver previously supported reset pins using GPIO, but it lacked
support for reset controllers. Although a reset method is generally not
required, the driver fails to detect the switch if the reset was kept
asserted by a previous driver.

This series adds support to reset a Realtek switch using a reset
controller. It also updates the binding documentation to remove the
requirement of a reset method and to add the new reset controller
property.

It was tested on a TL-WR1043ND v1 router (rtl8366rb via SMI).

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---

Changes in v3:
- Rebased on the Realtek DSA driver refactoring (08f627164126)
- Dropped the reset controller example in bindings
- Used %pe in error printing
- Linked to v2: https://lore.kernel.org/r/20231027190910.27044-1-luizluca@gmail.com/

Changes in v2:
- Introduced a dedicated commit for removing the reset-gpios requirement
- Placed binding patches before code changes
- Removed the 'reset-names' property
- Moved the example from the commit message to realtek.yaml
- Split the reset function into _assert/_deassert variants
- Modified reset functions to return a warning instead of a value
- Utilized devm_reset_control_get_optional to prevent failure when the
  reset control is missing
- Used 'true' and 'false' for boolean values
- Removed the CONFIG_RESET_CONTROLLER check as stub methods are
  sufficient when undefined
- Linked to v1: https://lore.kernel.org/r/20231024205805.19314-1-luizluca@gmail.com/

---
Luiz Angelo Daros de Luca (3):
      dt-bindings: net: dsa: realtek: reset-gpios is not required
      dt-bindings: net: dsa: realtek: add reset controller
      net: dsa: realtek: support reset controller

 .../devicetree/bindings/net/dsa/realtek.yaml       |  4 +-
 drivers/net/dsa/realtek/realtek.h                  |  2 +
 drivers/net/dsa/realtek/rtl83xx.c                  | 52 +++++++++++++++++++---
 drivers/net/dsa/realtek/rtl83xx.h                  |  2 +
 4 files changed, 54 insertions(+), 6 deletions(-)
---
base-commit: 0f37666d87d2dea42ec21776c3d562b7cbd71612
change-id: 20240212-realtek-reset-88a0bf25bb22

Best regards,
-- 
Luiz Angelo Daros de Luca <luizluca@gmail.com>

[PATCH net-next v3 1/3] dt-bindings: net: dsa: realtek: reset-gpios is not required

[Luiz Angelo Daros de Luca]

The 'reset-gpios' should not be mandatory. although they might be
required for some devices if the switch reset was left asserted by a
previous driver, such as the bootloader.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
Cc: devicetree@vger.kernel.org
Acked-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Acked-by: Rob Herring <robh@kernel.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
 Documentation/devicetree/bindings/net/dsa/realtek.yaml | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
index cce692f57b08..46e113df77c8 100644
--- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
@@ -127,7 +127,6 @@ else:
     - mdc-gpios
     - mdio-gpios
     - mdio
-    - reset-gpios
 
 required:
   - compatible

-- 
2.43.0

[PATCH net-next v3 2/3] dt-bindings: net: dsa: realtek: add reset controller

[Luiz Angelo Daros de Luca]

Realtek switches can use a reset controller instead of reset-gpios.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
Cc: devicetree@vger.kernel.org
Acked-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/net/dsa/realtek.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
index 46e113df77c8..70b6bda3cf98 100644
--- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
@@ -59,6 +59,9 @@ properties:
     description: GPIO to be used to reset the whole device
     maxItems: 1
 
+  resets:
+    maxItems: 1
+
   realtek,disable-leds:
     type: boolean
     description: |

-- 
2.43.0

[PATCH net-next v3 3/3] net: dsa: realtek: support reset controller

[Luiz Angelo Daros de Luca]

The 'reset-gpios' will not work when the switch reset is controlled by a
reset controller.

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

The reset controller will take precedence over the reset GPIO.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/realtek.h |  2 ++
 drivers/net/dsa/realtek/rtl83xx.c | 52 +++++++++++++++++++++++++++++++++++----
 drivers/net/dsa/realtek/rtl83xx.h |  2 ++
 3 files changed, 51 insertions(+), 5 deletions(-)

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..3d1f541936b8 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,46 @@ 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;
+
+	if (priv->reset_ctl) {
+		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));
+	}
+
+	if (priv->reset)
+		gpiod_set_value(priv->reset, true);
+}
+
+void rtl83xx_reset_deassert(struct realtek_priv *priv)
+{
+	int ret;
+
+	if (priv->reset_ctl) {
+		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));
+	}
+
+	if (priv->reset)
+		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 */

-- 
2.43.0

Re: [PATCH net-next v3 3/3] net: dsa: realtek: support reset controller

[Linus Walleij]

Hi Luiz,

thanks for your patch!

On Wed, Feb 14, 2024 at 1:54 AM Luiz Angelo Daros de Luca
<luizluca@gmail.com> wrote:

> The 'reset-gpios' will not work when the switch reset is controlled by a
> reset controller.
>
> Although the reset is optional and the driver performs a soft reset
> during setup, if the initial reset state was asserted, the driver will
> not detect it.
>
> The reset controller will take precedence over the reset GPIO.
>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
(...)
> +void rtl83xx_reset_assert(struct realtek_priv *priv)
> +{
> +       int ret;
> +
> +       if (priv->reset_ctl) {
> +               ret = reset_control_assert(priv->reset_ctl);

Actually, reset_control_assert() is NULL-tolerand (as well as the
stubs) so you can just issue this unconditionally. If priv->reset_ctl
is NULL it will just return 0.

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

Same here! Also NULL-tolerant. You do not need to check priv->reset.
Just issue it.

> +void rtl83xx_reset_deassert(struct realtek_priv *priv)

Same comments for deassert.

With this fixed (the rest looks just fine):
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH net-next v3 3/3] net: dsa: realtek: support reset controller

[Luiz Angelo Daros de Luca]

> Hi Luiz,

Hi Linus,

> thanks for your patch!

I'm glad to help ;-)

> On Wed, Feb 14, 2024 at 1:54 AM Luiz Angelo Daros de Luca
> <luizluca@gmail.com> wrote:
>
> > The 'reset-gpios' will not work when the switch reset is controlled by a
> > reset controller.
> >
> > Although the reset is optional and the driver performs a soft reset
> > during setup, if the initial reset state was asserted, the driver will
> > not detect it.
> >
> > The reset controller will take precedence over the reset GPIO.
> >
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> (...)
> > +void rtl83xx_reset_assert(struct realtek_priv *priv)
> > +{
> > +       int ret;
> > +
> > +       if (priv->reset_ctl) {
> > +               ret = reset_control_assert(priv->reset_ctl);
>
> Actually, reset_control_assert() is NULL-tolerand (as well as the
> stubs) so you can just issue this unconditionally. If priv->reset_ctl
> is NULL it will just return 0.

The idea was to avoid gpiod_set_value if the reset_control_assert was
configured and worked. However, I don't see a big issue in calling
them both as you don't expect both to be configured.
I'll remove the "ifs not null" and let both be called.

>
> > +               if (!ret)
> > +                       return;
> > +
> > +               dev_warn(priv->dev,
> > +                        "Failed to assert the switch reset control: %pe\n",
> > +                        ERR_PTR(ret));
> > +       }
> > +
> > +       if (priv->reset)
> > +               gpiod_set_value(priv->reset, true);
>
> Same here! Also NULL-tolerant. You do not need to check priv->reset.
> Just issue it.
>
> > +void rtl83xx_reset_deassert(struct realtek_priv *priv)
>
> Same comments for deassert.
>
> With this fixed (the rest looks just fine):
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> Yours,
> Linus Walleij

Re: [PATCH net-next v3 3/3] net: dsa: realtek: support reset controller

[Vladimir Oltean]

On Wed, Feb 14, 2024 at 07:17:55PM -0300, Luiz Angelo Daros de Luca wrote:
> > On Wed, Feb 14, 2024 at 1:54 AM Luiz Angelo Daros de Luca
> > <luizluca@gmail.com> wrote:
> >
> > > The 'reset-gpios' will not work when the switch reset is controlled by a
> > > reset controller.
> > >
> > > Although the reset is optional and the driver performs a soft reset
> > > during setup, if the initial reset state was asserted, the driver will
> > > not detect it.
> > >
> > > The reset controller will take precedence over the reset GPIO.
> > >
> > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> > (...)
> > > +void rtl83xx_reset_assert(struct realtek_priv *priv)
> > > +{
> > > +       int ret;
> > > +
> > > +       if (priv->reset_ctl) {
> > > +               ret = reset_control_assert(priv->reset_ctl);
> >
> > Actually, reset_control_assert() is NULL-tolerand (as well as the
> > stubs) so you can just issue this unconditionally. If priv->reset_ctl
> > is NULL it will just return 0.
> 
> The idea was to avoid gpiod_set_value if the reset_control_assert was
> configured and worked. However, I don't see a big issue in calling
> them both as you don't expect both to be configured.
> I'll remove the "ifs not null" and let both be called.

In the defense of Linus' comment, your proposed code did not do what you
seem to have intended anyway. If priv->reset_ctl was non-NULL, it fell
through and potentially ran gpiod_set_value() anyway - there was no
early "return". So, the simplification comment was predictable.

Re: [PATCH net-next v3 3/3] net: dsa: realtek: support reset controller

[Vladimir Oltean]

On Wed, Feb 14, 2024 at 07:17:55PM -0300, Luiz Angelo Daros de Luca wrote:
> > > The reset controller will take precedence over the reset GPIO.
> 
> I'll remove the "ifs not null" and let both be called.

If you do, be sure to update the commit message about it (see first line).