[RFC,next,v1,0/5] stmmac: honor the GPIO flags for the PHY reset GPIO
mbox series

Message ID 20190609180621.7607-1-martin.blumenstingl@googlemail.com
Headers show
Series
  • stmmac: honor the GPIO flags for the PHY reset GPIO
Related show

Message

Martin Blumenstingl June 9, 2019, 6:06 p.m. UTC
Recent Amlogic SoCs (G12A which includes S905X2 and S905D2 as well as
G12B which includes S922X) use GPIOZ_14 or GPIOZ_15 for the PHY reset
line. These GPIOs are special because they are marked as "3.3V input
tolerant open drain (OD) pins" which means they can only drive the pin
output LOW (to reset the PHY) or to switch to input mode (to take the
PHY out of reset).
The GPIO subsystem already supports this with the GPIO_OPEN_DRAIN and
GPIO_OPEN_SOURCE flags in the devicetree bindings.

The goal of this series to add support for these special GPIOs in
stmmac.

Patch #2 prepares gpiolib-of for the switch from (legacy) GPIO numbers
to GPIO descriptors in stmmac. This requires the gpiolib-of to take
care of the "snps,reset-active-low" property.

Patch #3 switches stmmac from (legacy) GPIO numbers to GPIO descriptors
because this enables tracking of the GPIO flags which are passed via
devicetree. In other words: GPIO_OPEN_DRAIN and GPIO_OPEN_SOURCE are
now honored correctly, which is exactly what is needed for these
Amlogic platforms.

Patch #1 and #4 are minor cleanups which follow the boyscout rule:
"Always leave the campground cleaner than you found it."

Patch #5 is included here to show how this new functionality is used.

My test-cases were:
- X96 Max: snps,reset-gpio = <&gpio GPIOZ_15 0> with and without
           snps,reset-active-low before these patches. The PHY was
           not detected.
- X96 Max: snps,reset-gpio = <&gpio GPIOZ_15 GPIO_OPEN_SOURCE>. The
           PHY is now detected correctly
- Meson8b EC100: snps,reset-gpio = <&gpio GPIOH_4 0> with
                 snps,reset-active-low. Before and after these
                 patches the PHY is detected correctly.
- Meson8b EC100: snps,reset-gpio = <&gpio GPIOH_4 0> without
                 snps,reset-active-low. Before and after these
                 patches the PHY is not detected (this is expected
                 because we need to set the output LOW to take the
                 PHY out of reset).
- Meson8b EC100: snps,reset-gpio = <&gpio GPIOH_4 GPIO_ACTIVE_LOW>
                 but without snps,reset-active-low. Before these
                 patches the PHY was not detected. With these patches
                 the PHY is now detected correctly.

I am sending this as RFC because I'm not very familiar with the GPIO
subsystem. What I came up with seems fine to me, but I'm not sure so
I don't want this to be applied before Linus W. is happy with it. I
am also looking for suggestions how to handle these cross-tree changes
(patch #2 belongs to the linux-gpio tree, patches #1, 3 and #4 should
go through the net-next tree. I will re-send patch #5 separately as
this should go through Kevin's linux-amlogic tree).


Martin Blumenstingl (5):
  net: stmmac: drop redundant check in stmmac_mdio_reset
  gpio: of: parse stmmac PHY reset line specific active-low property
  net: stmmac: use GPIO descriptors in stmmac_mdio_reset
  net: stmmac: use device_property_read_u32_array to read the reset
    delays
  arm64: dts: meson: g12a: x96-max: fix the Ethernet PHY reset line

 .../boot/dts/amlogic/meson-g12a-x96-max.dts   |  3 +-
 drivers/gpio/gpiolib-of.c                     |  6 +++
 .../net/ethernet/stmicro/stmmac/stmmac_mdio.c | 43 ++++++++-----------
 3 files changed, 26 insertions(+), 26 deletions(-)

Comments

Andrew Lunn June 9, 2019, 8:45 p.m. UTC | #1
> Patch #1 and #4 are minor cleanups which follow the boyscout rule:
> "Always leave the campground cleaner than you found it."

> I
> am also looking for suggestions how to handle these cross-tree changes
> (patch #2 belongs to the linux-gpio tree, patches #1, 3 and #4 should
> go through the net-next tree. I will re-send patch #5 separately as
> this should go through Kevin's linux-amlogic tree).

Hi Martin

Patches 1 and 4 don't seem to have and dependencies. So i would
suggest splitting them out and submitting them to netdev for merging
independent of the rest.

Linus can probably create a stable branch with the GPIO changes, which
David can pull into net-next, and then apply the stmmac changes on
top.

	Andrew
Linus Walleij June 9, 2019, 9:52 p.m. UTC | #2
On Sun, Jun 9, 2019 at 10:45 PM Andrew Lunn <andrew@lunn.ch> wrote:

> Linus can probably create a stable branch with the GPIO changes, which
> David can pull into net-next, and then apply the stmmac changes on
> top.

Sure thing, just tell me what to queue and I'll create an immutable
branch for this that David can pull.

Yours,
Linus Walleij
Martin Blumenstingl June 9, 2019, 10:32 p.m. UTC | #3
Hi Andrew,

On Sun, Jun 9, 2019 at 10:45 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > Patch #1 and #4 are minor cleanups which follow the boyscout rule:
> > "Always leave the campground cleaner than you found it."
>
> > I
> > am also looking for suggestions how to handle these cross-tree changes
> > (patch #2 belongs to the linux-gpio tree, patches #1, 3 and #4 should
> > go through the net-next tree. I will re-send patch #5 separately as
> > this should go through Kevin's linux-amlogic tree).
>
> Hi Martin
>
> Patches 1 and 4 don't seem to have and dependencies. So i would
> suggest splitting them out and submitting them to netdev for merging
> independent of the rest.
OK, I will do that but after the GPIO changes are applied because only
then I can get rid of that "np" variable

> Linus can probably create a stable branch with the GPIO changes, which
> David can pull into net-next, and then apply the stmmac changes on
> top.
let's go this way since Linus is happy with that route also.
I'll re-spin v2 tomorrow


Martin
Maxime Ripard June 10, 2019, 11:47 a.m. UTC | #4
Hi Andrew,

On Sun, Jun 09, 2019 at 10:45:10PM +0200, Andrew Lunn wrote:
> > Patch #1 and #4 are minor cleanups which follow the boyscout rule:
> > "Always leave the campground cleaner than you found it."
>
> > I
> > am also looking for suggestions how to handle these cross-tree changes
> > (patch #2 belongs to the linux-gpio tree, patches #1, 3 and #4 should
> > go through the net-next tree. I will re-send patch #5 separately as
> > this should go through Kevin's linux-amlogic tree).
>
> Patches 1 and 4 don't seem to have and dependencies. So i would
> suggest splitting them out and submitting them to netdev for merging
> independent of the rest.

Jumping on the occasion of that series. These properties have been
defined to deal with phy reset, while it seems that the PHY core can
now handle that pretty easily through generic properties.

Wouldn't it make more sense to just move to that generic properties
that already deals with the flags properly?

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Martin Blumenstingl June 10, 2019, 12:31 p.m. UTC | #5
Hi Maxime,

On Mon, Jun 10, 2019 at 1:47 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> Hi Andrew,
>
> On Sun, Jun 09, 2019 at 10:45:10PM +0200, Andrew Lunn wrote:
> > > Patch #1 and #4 are minor cleanups which follow the boyscout rule:
> > > "Always leave the campground cleaner than you found it."
> >
> > > I
> > > am also looking for suggestions how to handle these cross-tree changes
> > > (patch #2 belongs to the linux-gpio tree, patches #1, 3 and #4 should
> > > go through the net-next tree. I will re-send patch #5 separately as
> > > this should go through Kevin's linux-amlogic tree).
> >
> > Patches 1 and 4 don't seem to have and dependencies. So i would
> > suggest splitting them out and submitting them to netdev for merging
> > independent of the rest.
>
> Jumping on the occasion of that series. These properties have been
> defined to deal with phy reset, while it seems that the PHY core can
> now handle that pretty easily through generic properties.
>
> Wouldn't it make more sense to just move to that generic properties
> that already deals with the flags properly?
thank you for bringing this up!
if anyone else (just like me) doesn't know about it, there are generic
bindings defined here: [0]

I just tested this on my X96 Max by defining the following properties
inside the PHY node:
  reset-delay-us = <10000>;
  reset-assert-us = <10000>;
  reset-deassert-us = <10000>;
  reset-gpios = <&gpio GPIOZ_15 (GPIO_ACTIVE_LOW | GPIO_OPEN_DRAIN)>;

that means I don't need any stmmac patches which seems nice.
instead I can submit a patch to mark the snps,reset-gpio properties in
the dt-bindings deprecated (and refer to the generic bindings instead)
what do you think?


Martin


[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/phy.txt?id=b54dd90cab00f5b64ed8ce533991c20bf781a3cd#n58
Andrew Lunn June 10, 2019, 1:25 p.m. UTC | #6
> if anyone else (just like me) doesn't know about it, there are generic
> bindings defined here: [0]
> 
> I just tested this on my X96 Max by defining the following properties
> inside the PHY node:
>   reset-delay-us = <10000>;
>   reset-assert-us = <10000>;
>   reset-deassert-us = <10000>;
>   reset-gpios = <&gpio GPIOZ_15 (GPIO_ACTIVE_LOW | GPIO_OPEN_DRAIN)>;
> 
> that means I don't need any stmmac patches which seems nice.
> instead I can submit a patch to mark the snps,reset-gpio properties in
> the dt-bindings deprecated (and refer to the generic bindings instead)
> what do you think?

Hi Martin

I know Linus wants to replace all users of old GPIO numbers with gpio
descriptors. So your patches have value, even if you don't need them.

One other things to watch out for. We have generic code at two
levels. Either the GPIO is per PHY, and the properties should be in
the PHY node, or the reset is for all PHYs of an MDIO bus, and then
the properties should be in the MDIO node.

    Andrew
Maxime Ripard June 10, 2019, 1:51 p.m. UTC | #7
Hi Martin,

On Mon, Jun 10, 2019 at 02:31:17PM +0200, Martin Blumenstingl wrote:
> On Mon, Jun 10, 2019 at 1:47 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > Hi Andrew,
> >
> > On Sun, Jun 09, 2019 at 10:45:10PM +0200, Andrew Lunn wrote:
> > > > Patch #1 and #4 are minor cleanups which follow the boyscout rule:
> > > > "Always leave the campground cleaner than you found it."
> > >
> > > > I
> > > > am also looking for suggestions how to handle these cross-tree changes
> > > > (patch #2 belongs to the linux-gpio tree, patches #1, 3 and #4 should
> > > > go through the net-next tree. I will re-send patch #5 separately as
> > > > this should go through Kevin's linux-amlogic tree).
> > >
> > > Patches 1 and 4 don't seem to have and dependencies. So i would
> > > suggest splitting them out and submitting them to netdev for merging
> > > independent of the rest.
> >
> > Jumping on the occasion of that series. These properties have been
> > defined to deal with phy reset, while it seems that the PHY core can
> > now handle that pretty easily through generic properties.
> >
> > Wouldn't it make more sense to just move to that generic properties
> > that already deals with the flags properly?
> thank you for bringing this up!
> if anyone else (just like me) doesn't know about it, there are generic
> bindings defined here: [0]
>
> I just tested this on my X96 Max by defining the following properties
> inside the PHY node:
>   reset-delay-us = <10000>;
>   reset-assert-us = <10000>;
>   reset-deassert-us = <10000>;
>   reset-gpios = <&gpio GPIOZ_15 (GPIO_ACTIVE_LOW | GPIO_OPEN_DRAIN)>;
>
> that means I don't need any stmmac patches which seems nice.

I'm glad it works for you :)

> instead I can submit a patch to mark the snps,reset-gpio properties in
> the dt-bindings deprecated (and refer to the generic bindings instead)
> what do you think?

I already did as part of the binding reworks I did earlier today:
http://lists.infradead.org/pipermail/linux-arm-kernel/2019-June/658427.html

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Martin Blumenstingl June 10, 2019, 3:51 p.m. UTC | #8
On Mon, Jun 10, 2019 at 3:51 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> Hi Martin,
>
> On Mon, Jun 10, 2019 at 02:31:17PM +0200, Martin Blumenstingl wrote:
> > On Mon, Jun 10, 2019 at 1:47 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > Hi Andrew,
> > >
> > > On Sun, Jun 09, 2019 at 10:45:10PM +0200, Andrew Lunn wrote:
> > > > > Patch #1 and #4 are minor cleanups which follow the boyscout rule:
> > > > > "Always leave the campground cleaner than you found it."
> > > >
> > > > > I
> > > > > am also looking for suggestions how to handle these cross-tree changes
> > > > > (patch #2 belongs to the linux-gpio tree, patches #1, 3 and #4 should
> > > > > go through the net-next tree. I will re-send patch #5 separately as
> > > > > this should go through Kevin's linux-amlogic tree).
> > > >
> > > > Patches 1 and 4 don't seem to have and dependencies. So i would
> > > > suggest splitting them out and submitting them to netdev for merging
> > > > independent of the rest.
> > >
> > > Jumping on the occasion of that series. These properties have been
> > > defined to deal with phy reset, while it seems that the PHY core can
> > > now handle that pretty easily through generic properties.
> > >
> > > Wouldn't it make more sense to just move to that generic properties
> > > that already deals with the flags properly?
> > thank you for bringing this up!
> > if anyone else (just like me) doesn't know about it, there are generic
> > bindings defined here: [0]
> >
> > I just tested this on my X96 Max by defining the following properties
> > inside the PHY node:
> >   reset-delay-us = <10000>;
> >   reset-assert-us = <10000>;
> >   reset-deassert-us = <10000>;
> >   reset-gpios = <&gpio GPIOZ_15 (GPIO_ACTIVE_LOW | GPIO_OPEN_DRAIN)>;
> >
> > that means I don't need any stmmac patches which seems nice.
>
> I'm glad it works for you :)
>
> > instead I can submit a patch to mark the snps,reset-gpio properties in
> > the dt-bindings deprecated (and refer to the generic bindings instead)
> > what do you think?
>
> I already did as part of the binding reworks I did earlier today:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2019-June/658427.html
great, thank you - you have my Reviewed-by!
Martin Blumenstingl June 10, 2019, 3:52 p.m. UTC | #9
Hi Andrew,

On Mon, Jun 10, 2019 at 3:25 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > if anyone else (just like me) doesn't know about it, there are generic
> > bindings defined here: [0]
> >
> > I just tested this on my X96 Max by defining the following properties
> > inside the PHY node:
> >   reset-delay-us = <10000>;
> >   reset-assert-us = <10000>;
> >   reset-deassert-us = <10000>;
> >   reset-gpios = <&gpio GPIOZ_15 (GPIO_ACTIVE_LOW | GPIO_OPEN_DRAIN)>;
> >
> > that means I don't need any stmmac patches which seems nice.
> > instead I can submit a patch to mark the snps,reset-gpio properties in
> > the dt-bindings deprecated (and refer to the generic bindings instead)
> > what do you think?
>
> Hi Martin
>
> I know Linus wants to replace all users of old GPIO numbers with gpio
> descriptors. So your patches have value, even if you don't need them.
OK, then I will send my patches anyways

> One other things to watch out for. We have generic code at two
> levels. Either the GPIO is per PHY, and the properties should be in
> the PHY node, or the reset is for all PHYs of an MDIO bus, and then
> the properties should be in the MDIO node.
our Amlogic boards only have one PHY and all schematics I'm aware of
route the SoC's GPIO line directly to the PHY's reset line.
so in my opinion defining the resets for the PHY is the right thing to do


Martin