diff mbox series

[RFC,v1,1/3] net: dsa: mt753x: make reset optional

Message ID 20220426134924.30372-2-linux@fw-web.de (mailing list archive)
State New
Headers show
Series Add MT7531 switch to BPI-R2Pro Board | expand

Commit Message

Frank Wunderlich April 26, 2022, 1:49 p.m. UTC
From: Frank Wunderlich <frank-w@public-files.de>

Currently a reset line is required, but on BPI-R2-Pro board
this reset is shared with the gmac and prevents the switch to
be initialized because mdio is not ready fast enough after
the reset.

So make the reset optional to allow shared reset lines.

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
 drivers/net/dsa/mt7530.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Florian Fainelli April 26, 2022, 3:42 p.m. UTC | #1
On 4/26/22 06:49, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> Currently a reset line is required, but on BPI-R2-Pro board
> this reset is shared with the gmac and prevents the switch to
> be initialized because mdio is not ready fast enough after
> the reset.
> 
> So make the reset optional to allow shared reset lines.
> 
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>

This looks fine however 
Documentation/devicetree/bindings/net/dsa/mt7530.txt still has some 
verbiage that suggests that the 'reset' property is mandatory, so you 
might need to update the binding (and as a separate patch we should make 
it YAML).
Vladimir Oltean April 26, 2022, 11:57 p.m. UTC | #2
On Tue, Apr 26, 2022 at 03:49:22PM +0200, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> Currently a reset line is required, but on BPI-R2-Pro board
> this reset is shared with the gmac and prevents the switch to
> be initialized because mdio is not ready fast enough after
> the reset.
> 
> So make the reset optional to allow shared reset lines.

What does it mean "to allow shared reset lines"? Allow as in "allow them
to sit there, unused"?

> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
>  drivers/net/dsa/mt7530.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 19f0035d4410..ccf4cb944167 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2134,7 +2134,7 @@ mt7530_setup(struct dsa_switch *ds)
>  		reset_control_assert(priv->rstc);
>  		usleep_range(1000, 1100);
>  		reset_control_deassert(priv->rstc);
> -	} else {
> +	} else if (priv->reset) {

I don't really understand this patch. gpiod_set_value_cansleep() can
tolerate NULL GPIO descriptors.

>  		gpiod_set_value_cansleep(priv->reset, 0);
>  		usleep_range(1000, 1100);
>  		gpiod_set_value_cansleep(priv->reset, 1);
> @@ -2276,7 +2276,7 @@ mt7531_setup(struct dsa_switch *ds)
>  		reset_control_assert(priv->rstc);
>  		usleep_range(1000, 1100);
>  		reset_control_deassert(priv->rstc);
> -	} else {
> +	} else if (priv->reset) {
>  		gpiod_set_value_cansleep(priv->reset, 0);
>  		usleep_range(1000, 1100);
>  		gpiod_set_value_cansleep(priv->reset, 1);
> @@ -3272,8 +3272,7 @@ mt7530_probe(struct mdio_device *mdiodev)
>  		priv->reset = devm_gpiod_get_optional(&mdiodev->dev, "reset",
>  						      GPIOD_OUT_LOW);
>  		if (IS_ERR(priv->reset)) {
> -			dev_err(&mdiodev->dev, "Couldn't get our reset line\n");
> -			return PTR_ERR(priv->reset);
> +			dev_warn(&mdiodev->dev, "Couldn't get our reset line\n");

I certainly don't understand why you're suppressing the pointer-encoded
errors here. The function used is devm_gpiod_get_optional(), which
returns NULL for a missing reset-gpios, not IS_ERR(something). The
IS_ERR(something) is actually important to not ignore, maybe it's
IS_ERR(-EPROBE_DEFER). And this change breaks waiting for the descriptor
to become available.

>  		}
>  	}
>  
> -- 
> 2.25.1
> 

So what doesn't work without this patch, exactly?
Frank Wunderlich April 27, 2022, 7:05 a.m. UTC | #3
Hi,

you're right, reset is already optional...

idk why it was failing before the patch...i guess i had always defined the reset in dts on switch-side and dropped it same time with this patch.

Reset on both nodes (gmac+switch) blocks in switch driver because of exclusive (error message "could not get our reset line") and after dropping the reset on gmac-side the mdio-bus does not come up after switch driver resets gmac+switch (in loop with edefer).

> Gesendet: Mittwoch, 27. April 2022 um 01:57 Uhr
> Von: "Vladimir Oltean" <olteanv@gmail.com>
> On Tue, Apr 26, 2022 at 03:49:22PM +0200, Frank Wunderlich wrote:
> > From: Frank Wunderlich <frank-w@public-files.de>
> >
> > Currently a reset line is required, but on BPI-R2-Pro board
> > this reset is shared with the gmac and prevents the switch to
> > be initialized because mdio is not ready fast enough after
> > the reset.
> >
> > So make the reset optional to allow shared reset lines.
>
> What does it mean "to allow shared reset lines"? Allow as in "allow them
> to sit there, unused"?

for switch part unused right, but reset-line is used by gmac. If switch does the reset, it resets the gmac too and so the mdio goes down. It took longer to get up than the wait-poll after the reset and so i got mdio read errors.

> > -	} else {
> > +	} else if (priv->reset) {
>
> I don't really understand this patch. gpiod_set_value_cansleep() can
> tolerate NULL GPIO descriptors.

had not looked for NULL-tolerance, so i precautionary added the check.

> >  		gpiod_set_value_cansleep(priv->reset, 0);
> >  		usleep_range(1000, 1100);
> >  		gpiod_set_value_cansleep(priv->reset, 1);

> > @@ -3272,8 +3272,7 @@ mt7530_probe(struct mdio_device *mdiodev)
> >  		priv->reset = devm_gpiod_get_optional(&mdiodev->dev, "reset",
> >  						      GPIOD_OUT_LOW);
> >  		if (IS_ERR(priv->reset)) {
> > -			dev_err(&mdiodev->dev, "Couldn't get our reset line\n");
> > -			return PTR_ERR(priv->reset);
> > +			dev_warn(&mdiodev->dev, "Couldn't get our reset line\n");
>
> I certainly don't understand why you're suppressing the pointer-encoded
> errors here. The function used is devm_gpiod_get_optional(), which
> returns NULL for a missing reset-gpios, not IS_ERR(something). The
> IS_ERR(something) is actually important to not ignore, maybe it's
> IS_ERR(-EPROBE_DEFER). And this change breaks waiting for the descriptor
> to become available.

you're right...the intention was to not leave the probe function if not reset was defined...but yes, devm_gpiod_get_optional is called so reset is already optional.

> So what doesn't work without this patch, exactly?

reverted the Patch in my repo and it is still working :)

just ignore it. something went wrong during my tests...

sorry for the inconvenience.

regards Frank
diff mbox series

Patch

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 19f0035d4410..ccf4cb944167 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2134,7 +2134,7 @@  mt7530_setup(struct dsa_switch *ds)
 		reset_control_assert(priv->rstc);
 		usleep_range(1000, 1100);
 		reset_control_deassert(priv->rstc);
-	} else {
+	} else if (priv->reset) {
 		gpiod_set_value_cansleep(priv->reset, 0);
 		usleep_range(1000, 1100);
 		gpiod_set_value_cansleep(priv->reset, 1);
@@ -2276,7 +2276,7 @@  mt7531_setup(struct dsa_switch *ds)
 		reset_control_assert(priv->rstc);
 		usleep_range(1000, 1100);
 		reset_control_deassert(priv->rstc);
-	} else {
+	} else if (priv->reset) {
 		gpiod_set_value_cansleep(priv->reset, 0);
 		usleep_range(1000, 1100);
 		gpiod_set_value_cansleep(priv->reset, 1);
@@ -3272,8 +3272,7 @@  mt7530_probe(struct mdio_device *mdiodev)
 		priv->reset = devm_gpiod_get_optional(&mdiodev->dev, "reset",
 						      GPIOD_OUT_LOW);
 		if (IS_ERR(priv->reset)) {
-			dev_err(&mdiodev->dev, "Couldn't get our reset line\n");
-			return PTR_ERR(priv->reset);
+			dev_warn(&mdiodev->dev, "Couldn't get our reset line\n");
 		}
 	}