[PATCH net-next 1/2] net: dsa: realtek: support reset controller

[PATCH net-next 1/2] 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.

This is an example of how to use the reset controller:

        switch {
                compatible = "realtek,rtl8366rb";

                resets = <&rst 8>;
                reset-names = "switch";

		...
	}

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-mdio.c | 36 +++++++++++++++++++++-----
 drivers/net/dsa/realtek/realtek-smi.c  | 34 +++++++++++++++++++-----
 drivers/net/dsa/realtek/realtek.h      |  6 +++++
 3 files changed, 63 insertions(+), 13 deletions(-)

diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index 292e6d087e8b..600124c58c00 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -140,6 +140,23 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = {
 	.disable_locking = true,
 };
 
+static int realtek_mdio_hwreset(struct realtek_priv *priv, bool active)
+{
+#ifdef CONFIG_RESET_CONTROLLER
+	if (priv->reset_ctl) {
+		if (active)
+			return reset_control_assert(priv->reset_ctl);
+		else
+			return reset_control_deassert(priv->reset_ctl);
+	}
+#endif
+
+	if (priv->reset)
+		gpiod_set_value(priv->reset, active);
+
+	return 0;
+}
+
 static int realtek_mdio_probe(struct mdio_device *mdiodev)
 {
 	struct realtek_priv *priv;
@@ -194,20 +211,26 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
 
 	dev_set_drvdata(dev, priv);
 
-	/* TODO: if power is software controlled, set up any regulators here */
 	priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds");
 
+#ifdef CONFIG_RESET_CONTROLLER
+	priv->reset_ctl = devm_reset_control_get(dev, "switch");
+	if (IS_ERR(priv->reset_ctl)) {
+		dev_err(dev, "failed to get switch reset control\n");
+		return PTR_ERR(priv->reset_ctl);
+	}
+#endif
+
 	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");
 		return PTR_ERR(priv->reset);
 	}
-
-	if (priv->reset) {
-		gpiod_set_value(priv->reset, 1);
+	if (priv->reset_ctl || priv->reset) {
+		realtek_mdio_hwreset(priv, 1);
 		dev_dbg(dev, "asserted RESET\n");
 		msleep(REALTEK_HW_STOP_DELAY);
-		gpiod_set_value(priv->reset, 0);
+		realtek_mdio_hwreset(priv, 0);
 		msleep(REALTEK_HW_START_DELAY);
 		dev_dbg(dev, "deasserted RESET\n");
 	}
@@ -246,8 +269,7 @@ static void realtek_mdio_remove(struct mdio_device *mdiodev)
 	dsa_unregister_switch(priv->ds);
 
 	/* leave the device reset asserted */
-	if (priv->reset)
-		gpiod_set_value(priv->reset, 1);
+	realtek_mdio_hwreset(priv, 1);
 }
 
 static void realtek_mdio_shutdown(struct mdio_device *mdiodev)
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index bfd11591faf4..751159d71223 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -408,6 +408,23 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
 	return ret;
 }
 
+static int realtek_smi_hwreset(struct realtek_priv *priv, bool active)
+{
+#ifdef CONFIG_RESET_CONTROLLER
+	if (priv->reset_ctl) {
+		if (active)
+			return reset_control_assert(priv->reset_ctl);
+		else
+			return reset_control_deassert(priv->reset_ctl);
+	}
+#endif
+
+	if (priv->reset)
+		gpiod_set_value(priv->reset, active);
+
+	return 0;
+}
+
 static int realtek_smi_probe(struct platform_device *pdev)
 {
 	const struct realtek_variant *var;
@@ -457,18 +474,24 @@ static int realtek_smi_probe(struct platform_device *pdev)
 	dev_set_drvdata(dev, priv);
 	spin_lock_init(&priv->lock);
 
-	/* TODO: if power is software controlled, set up any regulators here */
+#ifdef CONFIG_RESET_CONTROLLER
+	priv->reset_ctl = devm_reset_control_get(dev, "switch");
+	if (IS_ERR(priv->reset_ctl)) {
+		dev_err(dev, "failed to get switch reset control\n");
+		return PTR_ERR(priv->reset_ctl);
+	}
+#endif
 
 	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");
 		return PTR_ERR(priv->reset);
 	}
-	if (priv->reset) {
-		gpiod_set_value(priv->reset, 1);
+	if (priv->reset_ctl || priv->reset) {
+		realtek_smi_hwreset(priv, 1);
 		dev_dbg(dev, "asserted RESET\n");
 		msleep(REALTEK_HW_STOP_DELAY);
-		gpiod_set_value(priv->reset, 0);
+		realtek_smi_hwreset(priv, 0);
 		msleep(REALTEK_HW_START_DELAY);
 		dev_dbg(dev, "deasserted RESET\n");
 	}
@@ -518,8 +541,7 @@ static void realtek_smi_remove(struct platform_device *pdev)
 		of_node_put(priv->slave_mii_bus->dev.of_node);
 
 	/* leave the device reset asserted */
-	if (priv->reset)
-		gpiod_set_value(priv->reset, 1);
+	realtek_smi_hwreset(priv, 1);
 }
 
 static void realtek_smi_shutdown(struct platform_device *pdev)
diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
index 4fa7c6ba874a..ad61e5c13f96 100644
--- a/drivers/net/dsa/realtek/realtek.h
+++ b/drivers/net/dsa/realtek/realtek.h
@@ -12,6 +12,9 @@
 #include <linux/platform_device.h>
 #include <linux/gpio/consumer.h>
 #include <net/dsa.h>
+#ifdef CONFIG_RESET_CONTROLLER
+#include <linux/reset.h>
+#endif
 
 #define REALTEK_HW_STOP_DELAY		25	/* msecs */
 #define REALTEK_HW_START_DELAY		100	/* msecs */
@@ -48,6 +51,9 @@ struct rtl8366_vlan_4k {
 
 struct realtek_priv {
 	struct device		*dev;
+#ifdef CONFIG_RESET_CONTROLLER
+	struct reset_control    *reset_ctl;
+#endif
 	struct gpio_desc	*reset;
 	struct gpio_desc	*mdc;
 	struct gpio_desc	*mdio;
-- 
2.42.0

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

[Luiz Angelo Daros de Luca]

Realtek switches can now be reset using a reset controller.

The 'reset-gpios' were never mandatory for the driver, although they
are 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
---
 Documentation/devicetree/bindings/net/dsa/realtek.yaml | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
index cce692f57b08..070821eae2a7 100644
--- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
@@ -59,6 +59,12 @@ properties:
     description: GPIO to be used to reset the whole device
     maxItems: 1
 
+  resets:
+    maxItems: 1
+
+  reset-names:
+    const: switch
+
   realtek,disable-leds:
     type: boolean
     description: |
@@ -127,7 +133,6 @@ else:
     - mdc-gpios
     - mdio-gpios
     - mdio
-    - reset-gpios
 
 required:
   - compatible
-- 
2.42.0

Re: [PATCH net-next 1/2] net: dsa: realtek: support reset controller

[Florian Fainelli]

On 10/24/23 13:58, Luiz Angelo Daros de Luca 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.
> 
> This is an example of how to use the reset controller:
> 
>          switch {
>                  compatible = "realtek,rtl8366rb";
> 
>                  resets = <&rst 8>;
>                  reset-names = "switch";
> 
> 		...
> 	}
> 
> 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-mdio.c | 36 +++++++++++++++++++++-----
>   drivers/net/dsa/realtek/realtek-smi.c  | 34 +++++++++++++++++++-----
>   drivers/net/dsa/realtek/realtek.h      |  6 +++++
>   3 files changed, 63 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index 292e6d087e8b..600124c58c00 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -140,6 +140,23 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = {
>   	.disable_locking = true,
>   };
>   
> +static int realtek_mdio_hwreset(struct realtek_priv *priv, bool active)
> +{
> +#ifdef CONFIG_RESET_CONTROLLER
> +	if (priv->reset_ctl) {
> +		if (active)
> +			return reset_control_assert(priv->reset_ctl);
> +		else
> +			return reset_control_deassert(priv->reset_ctl);
> +	}
> +#endif

Empty stubs are provided when CONFIG_RESET_CONTROLLER is disabled, and 
if you switch to using devm_reset_control_get() then you will get a NULL 
reset_control reference which will be a no-op for all of those operations.
-- 
Florian

Re: [PATCH net-next 1/2] net: dsa: realtek: support reset controller

[Luiz Angelo Daros de Luca]

> Empty stubs are provided when CONFIG_RESET_CONTROLLER is disabled

Nice! I'll drop the "#ifdef"s.

> if you switch to using devm_reset_control_get() then you will get a NULL
> reset_control reference which will be a no-op for all of those operations.

I'm already using devm_reset_control_get(). Maybe you copied the wrong
name? Did you mean devm_reset_control_get_optional()?
It is, indeed, what I needed. Thanks.

Regards,

Luiz

Re: [PATCH net-next 1/2] net: dsa: realtek: support reset controller

[Florian Fainelli]

On 10/24/23 15:02, Luiz Angelo Daros de Luca wrote:
>> Empty stubs are provided when CONFIG_RESET_CONTROLLER is disabled
> 
> Nice! I'll drop the "#ifdef"s.
> 
>> if you switch to using devm_reset_control_get() then you will get a NULL
>> reset_control reference which will be a no-op for all of those operations.
> 
> I'm already using devm_reset_control_get(). Maybe you copied the wrong
> name? Did you mean devm_reset_control_get_optional()?
> It is, indeed, what I needed. Thanks.

Yes I copy/pasted what I had just been searching for and instead 
intended to mention that devm_reset_control_get_optional() would do what 
you need. Thanks!
-- 
Florian

Re: [PATCH net-next 1/2] net: dsa: realtek: support reset controller

[Vladimir Oltean]

On Tue, Oct 24, 2023 at 07:02:33PM -0300, Luiz Angelo Daros de Luca wrote:
> > Empty stubs are provided when CONFIG_RESET_CONTROLLER is disabled
> 
> Nice! I'll drop the "#ifdef"s.
> 
> > if you switch to using devm_reset_control_get() then you will get a NULL
> > reset_control reference which will be a no-op for all of those operations.
> 
> I'm already using devm_reset_control_get(). Maybe you copied the wrong
> name? Did you mean devm_reset_control_get_optional()?
> It is, indeed, what I needed. Thanks.

Please also wait for a review on the device tree binding change before
posting a v2.

Re: [PATCH net-next 1/2] net: dsa: realtek: support reset controller

[Luiz Angelo Daros de Luca]

Hi Linus,

> -       /* TODO: if power is software controlled, set up any regulators here */
> +#ifdef CONFIG_RESET_CONTROLLER
> +       priv->reset_ctl = devm_reset_control_get(dev, "switch");
> +       if (IS_ERR(priv->reset_ctl)) {
> +               dev_err(dev, "failed to get switch reset control\n");
> +               return PTR_ERR(priv->reset_ctl);
> +       }
> +#endif

I'm dropping this TODO as I think it means something like this reset
control, right?

Regards,

Luiz

Re: [PATCH net-next 1/2] net: dsa: realtek: support reset controller

[Andrew Lunn]

On Tue, Oct 24, 2023 at 07:17:10PM -0300, Luiz Angelo Daros de Luca wrote:
> Hi Linus,
> 
> > -       /* TODO: if power is software controlled, set up any regulators here */
> > +#ifdef CONFIG_RESET_CONTROLLER
> > +       priv->reset_ctl = devm_reset_control_get(dev, "switch");
> > +       if (IS_ERR(priv->reset_ctl)) {
> > +               dev_err(dev, "failed to get switch reset control\n");
> > +               return PTR_ERR(priv->reset_ctl);
> > +       }
> > +#endif
> 
> I'm dropping this TODO as I think it means something like this reset
> control, right?

No, a regulator is a different thing to a reset controller. A
regulator is used to control the power to the device.

	Andrew

Re: [PATCH net-next 1/2] net: dsa: realtek: support reset controller

[Vladimir Oltean]

On Tue, Oct 24, 2023 at 05:58:04PM -0300, Luiz Angelo Daros de Luca 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.
> 
> This is an example of how to use the reset controller:
> 
>         switch {
>                 compatible = "realtek,rtl8366rb";
> 
>                 resets = <&rst 8>;
>                 reset-names = "switch";
> 
> 		...
> 	}

Mix of tabs and spaces here.
Also, examples belong to the dt-schema.

> 
> 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-mdio.c | 36 +++++++++++++++++++++-----
>  drivers/net/dsa/realtek/realtek-smi.c  | 34 +++++++++++++++++++-----
>  drivers/net/dsa/realtek/realtek.h      |  6 +++++
>  3 files changed, 63 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index 292e6d087e8b..600124c58c00 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -140,6 +140,23 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = {
>  	.disable_locking = true,
>  };
>  
> +static int realtek_mdio_hwreset(struct realtek_priv *priv, bool active)
> +{
> +#ifdef CONFIG_RESET_CONTROLLER
> +	if (priv->reset_ctl) {
> +		if (active)
> +			return reset_control_assert(priv->reset_ctl);
> +		else
> +			return reset_control_deassert(priv->reset_ctl);
> +	}
> +#endif
> +
> +	if (priv->reset)
> +		gpiod_set_value(priv->reset, active);
> +
> +	return 0;
> +}
> +

This "bool active" artificially unifies two discrete code paths in the
same function, where the callers are not the same and the implementation
is not the same (given a priv->reset_ctl presence), separated by an "if".

Would it make more sense to have discrete functions, each with its
unique caller, like this?

static int realtek_reset_assert(struct realtek_priv *priv)
{
	if (priv->reset_ctl)
		return reset_control_assert(priv->reset_ctl);

	if (priv->reset)
		gpiod_set_value(priv->reset, 1);

	return 0;
}

static int realtek_reset_deassert(struct realtek_priv *priv)
{
	if (priv->reset_ctl)
		return reset_control_deassert(priv->reset_ctl);

	if (priv->reset)
		gpiod_set_value(priv->reset, 0);

	return 0;
}

Also, you return int but ignore error values everywhere. I guess it
would make more sense to return void, but print warnings within the
reset functions if the calls to the reset control fail.

>  static int realtek_mdio_probe(struct mdio_device *mdiodev)
>  {
>  	struct realtek_priv *priv;
> @@ -194,20 +211,26 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
>  
>  	dev_set_drvdata(dev, priv);
>  
> -	/* TODO: if power is software controlled, set up any regulators here */

I'm not sure if "power" and "reset" are the same thing...

>  	priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds");
>  
> +#ifdef CONFIG_RESET_CONTROLLER
> +	priv->reset_ctl = devm_reset_control_get(dev, "switch");
> +	if (IS_ERR(priv->reset_ctl)) {
> +		dev_err(dev, "failed to get switch reset control\n");
> +		return PTR_ERR(priv->reset_ctl);

		ret = PTR_ERR(priv->reset_ctl);
		return dev_err_probe(dev, err, "failed to get reset control\n");

This suppresses -EPROBE_DEFER prints.

> +	}
> +#endif
> +
>  	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");
>  		return PTR_ERR(priv->reset);
>  	}
> -
> -	if (priv->reset) {
> -		gpiod_set_value(priv->reset, 1);
> +	if (priv->reset_ctl || priv->reset) {
> +		realtek_mdio_hwreset(priv, 1);
>  		dev_dbg(dev, "asserted RESET\n");
>  		msleep(REALTEK_HW_STOP_DELAY);
> -		gpiod_set_value(priv->reset, 0);
> +		realtek_mdio_hwreset(priv, 0);
>  		msleep(REALTEK_HW_START_DELAY);
>  		dev_dbg(dev, "deasserted RESET\n");
>  	}
> @@ -246,8 +269,7 @@ static void realtek_mdio_remove(struct mdio_device *mdiodev)
>  	dsa_unregister_switch(priv->ds);
>  
>  	/* leave the device reset asserted */
> -	if (priv->reset)
> -		gpiod_set_value(priv->reset, 1);
> +	realtek_mdio_hwreset(priv, 1);

nitpick: "bool" arguments should take "true" or "false".

>  }
>  
>  static void realtek_mdio_shutdown(struct mdio_device *mdiodev)
> diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> index bfd11591faf4..751159d71223 100644
> --- a/drivers/net/dsa/realtek/realtek-smi.c
> +++ b/drivers/net/dsa/realtek/realtek-smi.c
> @@ -408,6 +408,23 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
>  	return ret;
>  }
>  
> +static int realtek_smi_hwreset(struct realtek_priv *priv, bool active)
> +{
> +#ifdef CONFIG_RESET_CONTROLLER
> +	if (priv->reset_ctl) {
> +		if (active)
> +			return reset_control_assert(priv->reset_ctl);
> +		else
> +			return reset_control_deassert(priv->reset_ctl);
> +	}
> +#endif
> +
> +	if (priv->reset)
> +		gpiod_set_value(priv->reset, active);
> +
> +	return 0;
> +}

What is the reason for duplicating realtek_mdio_hwreset()?

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

[Vladimir Oltean]

On Tue, Oct 24, 2023 at 05:58:06PM -0300, Luiz Angelo Daros de Luca wrote:
> Realtek switches can now be reset using a reset controller.
> 
> The 'reset-gpios' were never mandatory for the driver, although they
> are 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
> ---

I believe there was a rule that device tree binding patches should come
before the user of those bindings.

>  Documentation/devicetree/bindings/net/dsa/realtek.yaml | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> index cce692f57b08..070821eae2a7 100644
> --- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> @@ -59,6 +59,12 @@ properties:
>      description: GPIO to be used to reset the whole device
>      maxItems: 1
>  
> +  resets:
> +    maxItems: 1
> +
> +  reset-names:
> +    const: switch
> +
>    realtek,disable-leds:
>      type: boolean
>      description: |
> @@ -127,7 +133,6 @@ else:
>      - mdc-gpios
>      - mdio-gpios
>      - mdio
> -    - reset-gpios

Ideally, the change that makes the reset-gpios optional should not be
named "add reset controller", unless it is actually the addition of the
reset controller which makes it optional. Which you say is not the case.
So, I think it should be a separate change.

>  
>  required:
>    - compatible
> -- 
> 2.42.0
> 

I also commented this on the other change: please move the example here.

Re: [PATCH net-next 1/2] net: dsa: realtek: support reset controller

[Luiz Angelo Daros de Luca]

> On Tue, Oct 24, 2023 at 05:58:04PM -0300, Luiz Angelo Daros de Luca 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.
> >
> > This is an example of how to use the reset controller:
> >
> >         switch {
> >                 compatible = "realtek,rtl8366rb";
> >
> >                 resets = <&rst 8>;
> >                 reset-names = "switch";
> >
> >               ...
> >       }
>
> Mix of tabs and spaces here.
> Also, examples belong to the dt-schema.

OK

>
> >
> > 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-mdio.c | 36 +++++++++++++++++++++-----
> >  drivers/net/dsa/realtek/realtek-smi.c  | 34 +++++++++++++++++++-----
> >  drivers/net/dsa/realtek/realtek.h      |  6 +++++
> >  3 files changed, 63 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> > index 292e6d087e8b..600124c58c00 100644
> > --- a/drivers/net/dsa/realtek/realtek-mdio.c
> > +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> > @@ -140,6 +140,23 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = {
> >       .disable_locking = true,
> >  };
> >
> > +static int realtek_mdio_hwreset(struct realtek_priv *priv, bool active)
> > +{
> > +#ifdef CONFIG_RESET_CONTROLLER
> > +     if (priv->reset_ctl) {
> > +             if (active)
> > +                     return reset_control_assert(priv->reset_ctl);
> > +             else
> > +                     return reset_control_deassert(priv->reset_ctl);
> > +     }
> > +#endif
> > +
> > +     if (priv->reset)
> > +             gpiod_set_value(priv->reset, active);
> > +
> > +     return 0;
> > +}
> > +
>
> This "bool active" artificially unifies two discrete code paths in the
> same function, where the callers are not the same and the implementation
> is not the same (given a priv->reset_ctl presence), separated by an "if".
>
> Would it make more sense to have discrete functions, each with its
> unique caller, like this?
>
> static int realtek_reset_assert(struct realtek_priv *priv)
> {
>         if (priv->reset_ctl)
>                 return reset_control_assert(priv->reset_ctl);
>
>         if (priv->reset)
>                 gpiod_set_value(priv->reset, 1);
>
>         return 0;
> }
>
> static int realtek_reset_deassert(struct realtek_priv *priv)
> {
>         if (priv->reset_ctl)
>                 return reset_control_deassert(priv->reset_ctl);
>
>         if (priv->reset)
>                 gpiod_set_value(priv->reset, 0);
>
>         return 0;
> }

Sure. It is better.

> Also, you return int but ignore error values everywhere. I guess it
> would make more sense to return void, but print warnings within the
> reset functions if the calls to the reset control fail.
>
> >  static int realtek_mdio_probe(struct mdio_device *mdiodev)
> >  {
> >       struct realtek_priv *priv;
> > @@ -194,20 +211,26 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
> >
> >       dev_set_drvdata(dev, priv);
> >
> > -     /* TODO: if power is software controlled, set up any regulators here */
>
> I'm not sure if "power" and "reset" are the same thing...
>
> >       priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds");
> >
> > +#ifdef CONFIG_RESET_CONTROLLER
> > +     priv->reset_ctl = devm_reset_control_get(dev, "switch");
> > +     if (IS_ERR(priv->reset_ctl)) {
> > +             dev_err(dev, "failed to get switch reset control\n");
> > +             return PTR_ERR(priv->reset_ctl);
>
>                 ret = PTR_ERR(priv->reset_ctl);
>                 return dev_err_probe(dev, err, "failed to get reset control\n");
>
> This suppresses -EPROBE_DEFER prints.

OK

>
> > +     }
> > +#endif
> > +
> >       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");
> >               return PTR_ERR(priv->reset);
> >       }
> > -
> > -     if (priv->reset) {
> > -             gpiod_set_value(priv->reset, 1);
> > +     if (priv->reset_ctl || priv->reset) {
> > +             realtek_mdio_hwreset(priv, 1);
> >               dev_dbg(dev, "asserted RESET\n");
> >               msleep(REALTEK_HW_STOP_DELAY);
> > -             gpiod_set_value(priv->reset, 0);
> > +             realtek_mdio_hwreset(priv, 0);
> >               msleep(REALTEK_HW_START_DELAY);
> >               dev_dbg(dev, "deasserted RESET\n");
> >       }
> > @@ -246,8 +269,7 @@ static void realtek_mdio_remove(struct mdio_device *mdiodev)
> >       dsa_unregister_switch(priv->ds);
> >
> >       /* leave the device reset asserted */
> > -     if (priv->reset)
> > -             gpiod_set_value(priv->reset, 1);
> > +     realtek_mdio_hwreset(priv, 1);
>
> nitpick: "bool" arguments should take "true" or "false".

OK

>
> >  }
> >
> >  static void realtek_mdio_shutdown(struct mdio_device *mdiodev)
> > diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> > index bfd11591faf4..751159d71223 100644
> > --- a/drivers/net/dsa/realtek/realtek-smi.c
> > +++ b/drivers/net/dsa/realtek/realtek-smi.c
> > @@ -408,6 +408,23 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
> >       return ret;
> >  }
> >
> > +static int realtek_smi_hwreset(struct realtek_priv *priv, bool active)
> > +{
> > +#ifdef CONFIG_RESET_CONTROLLER
> > +     if (priv->reset_ctl) {
> > +             if (active)
> > +                     return reset_control_assert(priv->reset_ctl);
> > +             else
> > +                     return reset_control_deassert(priv->reset_ctl);
> > +     }
> > +#endif
> > +
> > +     if (priv->reset)
> > +             gpiod_set_value(priv->reset, active);
> > +
> > +     return 0;
> > +}
>
> What is the reason for duplicating realtek_mdio_hwreset()?

Both interface modules, realtek-smi and realtek-mdio, do not share
code, except for the realtek.h header file. I don't know if it is
worth it to put the code in a new shared module. What is the best
practice here? Create a realtek_common.c linked to both modules?

Regards,

Luiz

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

[Arınç ÜNAL]

Nice work Luiz.

On 24.10.2023 23:58, Luiz Angelo Daros de Luca wrote:
> Realtek switches can now be reset using a reset controller.

The switch could always be reset using a reset controller. The fact that
the Linux driver lacked the ability to do so is irrelevant here. The
abilities or the features of the hardware had never changed. You should get
rid of the "now" above.

> 
> The 'reset-gpios' were never mandatory for the driver, although they
> are required for some devices if the switch reset was left asserted by
> a previous driver, such as the bootloader.

dt-bindings are for documenting hardware. The Linux driver details are
irrelevant here. Also, from what I read above, I deduce that for the switch
to be properly controlled in all possible states that it would be found in,
the switch must be reset.

So instead of above I'd say:

Resetting the switch is mandatory. Resetting the switch with reset-gpios is
not mandatory. Therefore require one of reset-gpios or resets and
reset-names.

For dt-bindings changes, I'd remove reset-gpios from else of
if:required:reg as you already do with this patch, and add below to the
root of the schema.

oneOf:
   - required:
     - reset-gpios
   - required:
     - resets
     - reset-names

And, like Vladimir said, this should be a separate patch.

Also, please put the dt-bindings patches first in the patch series order.

Arınç

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

[Luiz Angelo Daros de Luca]

> On 24.10.2023 23:58, Luiz Angelo Daros de Luca wrote:
> > Realtek switches can now be reset using a reset controller.
>
> The switch could always be reset using a reset controller. The fact that
> the Linux driver lacked the ability to do so is irrelevant here. The
> abilities or the features of the hardware had never changed. You should get
> rid of the "now" above.

Yes, I need to avoid thinking about where it will be used.

> > The 'reset-gpios' were never mandatory for the driver, although they
> > are required for some devices if the switch reset was left asserted by
> > a previous driver, such as the bootloader.
>
> dt-bindings are for documenting hardware. The Linux driver details are
> irrelevant here. Also, from what I read above, I deduce that for the switch
> to be properly controlled in all possible states that it would be found in,
> the switch must be reset.

I don't believe the switch must be reset by the HW. It will only be
necessary if the reset was kept asserted by the previous driver or its
initial state. And even in that case, we would only need to deassert
the reset, not assert it.
The driver will sw reset the switch during setup, leaving it in a
pristine state.

This is the current code flow:

realtek-smi/mdio probe()
   HW reset assert
   sleep
   HW reset deassert
   sleep
   rtl8365mb/rtl8366rb detect()
      SW reset

In fact, if we could make sure the hw reset was actually performed,
like checking a switch uptime register, we could avoid resetting it
again.

If you must not consider how the driver was implemented, I must assume
that an advanced driver might be able to configure every aspect of the
switch without a reset. Reset is the easiest solution but I believe
there is a narrow window between the reset and the switch is properly
configured where the switch might act like a dump switch, forwarding
some packets to all ports and possibly leaking traffic.

> So instead of above I'd say:
>
> Resetting the switch is mandatory. Resetting the switch with reset-gpios is
> not mandatory. Therefore require one of reset-gpios or resets and
> reset-names.
>
> For dt-bindings changes, I'd remove reset-gpios from else of
> if:required:reg as you already do with this patch, and add below to the
> root of the schema.
>
> oneOf:
>    - required:
>      - reset-gpios
>    - required:
>      - resets
>      - reset-names

As I said, I don't believe a way to HW reset is mandatory.

> And, like Vladimir said, this should be a separate patch.

OK. I'm just waiting some days for a v2.

>
> Also, please put the dt-bindings patches first in the patch series order.

OK

>
> Arınç

Regards,

Luiz

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

[Rob Herring]

On Tue, Oct 24, 2023 at 05:58:06PM -0300, Luiz Angelo Daros de Luca wrote:
> Realtek switches can now be reset using a reset controller.
> 
> The 'reset-gpios' were never mandatory for the driver, although they
> are 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
> ---
>  Documentation/devicetree/bindings/net/dsa/realtek.yaml | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> index cce692f57b08..070821eae2a7 100644
> --- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> @@ -59,6 +59,12 @@ properties:
>      description: GPIO to be used to reset the whole device
>      maxItems: 1
>  
> +  resets:
> +    maxItems: 1
> +
> +  reset-names:
> +    const: switch

$block-name is not really a useful name for resources. Generally, you   
don't need -names if there's only 1 entry.

> +
>    realtek,disable-leds:
>      type: boolean
>      description: |
> @@ -127,7 +133,6 @@ else:
>      - mdc-gpios
>      - mdio-gpios
>      - mdio
> -    - reset-gpios
>  
>  required:
>    - compatible
> -- 
> 2.42.0
> 

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

[Luiz Angelo Daros de Luca]

> > +  resets:
> > +    maxItems: 1
> > +
> > +  reset-names:
> > +    const: switch
>
> $block-name is not really a useful name for resources. Generally, you
> don't need -names if there's only 1 entry.

I didn't know the reset-control name was optional. Yes, it is not
useful. I'll get rid of it.
It looks like there are more bindings where it is not necessary.

Regards,

Luiz