| Message ID | 20190609180621.7607-1-martin.blumenstingl@googlemail.com |
|---|---|
| Headers | show |
| Series | stmmac: honor the GPIO flags for the PHY reset GPIO | expand |
> 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
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
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
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
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
> 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
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
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!
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
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(-)