| Message ID | 20220426134924.30372-2-linux@fw-web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add MT7531 switch to BPI-R2Pro Board | expand |
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).
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?
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 --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"); } }