| Message ID | 20190609180621.7607-3-martin.blumenstingl@googlemail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | stmmac: honor the GPIO flags for the PHY reset GPIO | expand |
On Sun, Jun 09, 2019 at 08:06:18PM +0200, Martin Blumenstingl wrote: > The stmmac driver currently ignores the GPIO flags which are passed via > devicetree because it operates with legacy GPIO numbers instead of GPIO > descriptors. Hi Martin I don't think this is the reason. I think historically stmmac messed up and ignored the flags. There are a number of device tree blobs which have the incorrect flag value, but since it was always ignored, it did not matter. Then came along a board which really did need the flag, but it was too late, it could not be enabled because too many boards would break. So the hack was made, and snps,reset-active-low was added. Since snps,reset-active-low is a hack, it should not be in the core. Please don't add it to gpiolib-of.c, keep it within stmmac driver. Andrew
Hi Andrew, On Sun, Jun 9, 2019 at 10:38 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Sun, Jun 09, 2019 at 08:06:18PM +0200, Martin Blumenstingl wrote: > > The stmmac driver currently ignores the GPIO flags which are passed via > > devicetree because it operates with legacy GPIO numbers instead of GPIO > > descriptors. > > Hi Martin > > I don't think this is the reason. I think historically stmmac messed > up and ignored the flags. There are a number of device tree blobs > which have the incorrect flag value, but since it was always ignored, > it did not matter. Then came along a board which really did need the > flag, but it was too late, it could not be enabled because too many > boards would break. So the hack was made, and snps,reset-active-low > was added. that seems appropriate. I don't know whether you can fetch the GPIO flags when using legacy GPIO numbers. so it may also be a mix of your explanation and mine. in the end it's the same though: stmmac ignores the GPIO flags > Since snps,reset-active-low is a hack, it should not be in the > core. Please don't add it to gpiolib-of.c, keep it within stmmac > driver. I don't know how to keep backwards compatibility with old .dtb / .dts when moving this into the stmmac driver again. let's assume I put the "snps,reset-active-low" inversion logic back into stmmac. then I need to ignore the flags because some .dts file use the flag GPIO_ACTIVE_LOW *and* set "snps,reset-active-low" at the same time. "snps,reset-active-low" would then invert GPIO_ACTIVE_LOW again, which basically results in GPIO_ACTIVE_HIGH however, I can't ignore the flags because then I'm losing the information I need for the newer Amlogic SoCs like open drain / open source. so the logic that I need is: - use GPIO flags from .dtb / .dts - set GPIO_ACTIVE_LOW in addition to the flags if "snps,reset-active-low" is set (this is different to "always invert the output value") my understanding that of_gpio_flags_quirks (which I'm touching with this patch) is supposed to manage similar quirks to what we have in stmmac (it also contains some regulator and MMC quirks too). however, that's exactly the reason why I decided to mark this as RFC - so I'm eager to hear Linus comments on this Martin
On Sun, Jun 9, 2019 at 11:21 PM Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > my understanding that of_gpio_flags_quirks (which I'm touching with > this patch) is supposed to manage similar quirks to what we have in > stmmac (it also contains some regulator and MMC quirks too). > however, that's exactly the reason why I decided to mark this as RFC - > so I'm eager to hear Linus comments on this The idea with the quirks in gpiolib-of.c is to make device drivers simpler, and phase them over to ignoring quirks for mistakes done in the early days of DT standardization. This feature of the gpiolib API is supposed to make it "narrow and deep": make the generic case simple and handle any hardware description languages (DT or ACPI or board files) and quirks (mostly historical) under the hood. Especially drivers should not need to worry about polarity inversion instead just grab a GPIO descriptor and play away with it, asserting it as 1 and deasserting it as 0 whether that is the right polarity or not, the gpiolib should keep track of polarity no matter how that is described, even with historical weird bools like "snps,active-low" etc. So I think you are probably doing the right thing here. This patch is: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index aec7bd86ae7e..2533f2471821 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -158,6 +158,12 @@ static void of_gpio_flags_quirks(struct device_node *np, } } } + + /* Legacy handling of stmmac's active-low PHY reset line */ + if (IS_ENABLED(CONFIG_STMMAC_ETH) && + !strcmp(propname, "snps,reset-gpio") && + of_property_read_bool(np, "snps,reset-active-low")) + *flags |= OF_GPIO_ACTIVE_LOW; } /**
The stmmac driver currently ignores the GPIO flags which are passed via devicetree because it operates with legacy GPIO numbers instead of GPIO descriptors. stmmac assumes that the GPIO is "active HIGH" by default. This can be overwritten by setting "snps,reset-active-low" to make the reset line "active LOW". 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" 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. Add the stmmac PHY reset line specific active low parsing to gpiolib-of so stmmac can be ported to GPIO descriptors while being backwards compatible with device trees which use the "old" way of specifying the polarity. Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> --- drivers/gpio/gpiolib-of.c | 6 ++++++ 1 file changed, 6 insertions(+)