On 2/19/19 6:05 PM, Thierry Reding wrote: > On Tue, Feb 19, 2019 at 05:30:35PM +0100, Marek Vasut wrote: >> On 2/19/19 5:08 PM, Thierry Reding wrote: >>> On Tue, Feb 19, 2019 at 04:25:59PM +0100, Marek Vasut wrote: >>>> On 2/19/19 4:18 PM, Thierry Reding wrote: >>>>> On Sat, Feb 16, 2019 at 02:46:27PM +0100, marek.vasut@xxxxxxxxx wrote: >>>>>> From: Marek Vasut <marek.vasut+renesas@xxxxxxxxx> >>>>>> >>>>>> Since commit d6cd33ad7102 ("regulator: gpio: Convert to use descriptors") >>>>>> the GPIO regulator had inverted the polarity of the control GPIO. This >>>>>> problem manifested itself on systems with DT containing the following >>>>>> description (snippet from salvator-common.dtsi): >>>>>> >>>>>> gpios = <&gpio5 1 GPIO_ACTIVE_HIGH>; >>>>>> gpios-states = <1>; >>>>>> states = <3300000 1 >>>>>> 1800000 0>; >>>>>> >>>>>> Prior to the aforementioned commit, the gpio-regulator code used >>>>>> gpio_request_array() to claim the GPIO(s) specified in the "gpios" >>>>>> DT node, while the commit changed that to devm_gpiod_get_index(). >>>>>> >>>>>> The legacy gpio_request_array() calls gpio_request_one() and then >>>>>> gpiod_request(), which parses the DT flags of the "gpios" node and >>>>>> populates the GPIO descriptor flags field accordingly. >>>>>> >>>>>> The new devm_gpiod_get_index() calls gpiod_get_index(), then >>>>>> of_find_gpio(), of_get_named_gpiod_flags() with flags != NULL, >>>>>> and then of_gpio_flags_quirks(). Since commit a603a2b8d86e >>>>>> ("gpio: of: Add special quirk to parse regulator flags"), >>>>>> of_gpio_flags_quirks() contains a quirk for regulator-gpio >>>>>> which was never triggered by the legacy gpio_request_array() >>>>>> code path, but is triggered by devm_gpiod_get_index() code >>>>>> path. >>>>>> >>>>>> This quirk checks whether a GPIO is associated with a fixed >>>>>> or gpio-regulator and if so, checks two additional conditions. >>>>>> First, whether such GPIO is active-low, and if so, ignores the >>>>>> active-low flag. Second, whether the regulator DT node does >>>>>> have an "enable-active-high" property and if the property is >>>>>> NOT present, sets the GPIO flags as active-low. >>>>>> >>>>>> The second check triggers a problem, since it is applied to all >>>>>> GPIOs associated with a gpio-regulator, rather than only on the >>>>>> "enable" GPIOs, as the old code did. This changes the way the >>>>>> gpio-regulator interprets the DT description of the control >>>>>> GPIOs. >>>>>> >>>>>> The old code using gpio_request_array() explicitly parsed the >>>>>> "enable-active-high" DT property and only applied it to the >>>>>> GPIOs described in the "enable-gpios" DT node, and only if >>>>>> those were present. >>>>>> >>>>>> This patch fixes the quirk code by only applying the quirk >>>>>> to "enable-gpios", thus restoring the old behavior. >>>>>> >>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@xxxxxxxxx> >>>>>> Cc: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> >>>>>> Cc: Jan Kotas <jank@xxxxxxxxxxx> >>>>>> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx> >>>>>> Cc: Mark Brown <broonie@xxxxxxxxxx> >>>>>> Cc: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx> >>>>>> Cc: linux-renesas-soc@xxxxxxxxxxxxxxx >>>>>> To: linux-gpio@xxxxxxxxxxxxxxx >>>>>> --- >>>>>> drivers/gpio/gpiolib-of.c | 1 + >>>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> This causes a runtime regression on Jetson TX1. The symptoms are that >>>>> HDMI monitors are no longer reliably detected as plugged in. The reason >>>>> is because the GPIO polarity for the HDMI +5V regulator is now reversed >>>>> and therefore the HPD signal, which is usually fed by the +5V signal, >>>>> does not reflect the correct value. >>>>> >>>>> Now it's somewhat confusing to me why this wasn't broken before. We have >>>>> this in device tree: >>>>> >>>>> vdd_hdmi: regulator@10 { >>>>> compatible = "regulator-fixed"; >>>>> reg = <10>; >>>>> regulator-name = "VDD_HDMI_5V0"; >>>>> regulator-min-microvolt = <5000000>; >>>>> regulator-max-microvolt = <5000000>; >>>>> gpio = <&exp1 12 GPIO_ACTIVE_LOW>; >>>>> enable-active-high; >>>>> vin-supply = <&vdd_5v0_sys>; >>>>> }; >>>>> >>>>> This is from arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi. The above >>>>> is obviously wrong because it specifies GPIO_ACTIVE_LOW and I do see the >>>>> warning from the GPIO library about how this is being ignored. However, >>>>> the above works fine on today's linux-next with this commit reverted >>>>> because the quirks will cause the GPIO_ACTIVE_LOW flag to be ignored. >>>>> >>>>> However, with this commit applied, the quirk will be skipped for the >>>>> regulator and cause the GPIO_ACTIVE_LOW flag to be used to invert the >>>>> GPIO during enable and disable operations. >>>>> >>>>> Now, this is clearly a bug in the DT and I'm going to send out a patch >>>>> to fix that up, but I think we need to revert this patch for now because >>>>> there may be other cases out there that are broken. >>>> >>>> This patch fixes a breakage on R-Car Gen3 platforms, so reverting this >>>> patch will break those platforms. However, those platforms do not have a >>>> buggy DT, unlike the Jetson. >>> >>> Erm... that's not how we do things. You can't break one platform just to >>> make another work. By all means let's fix breakage on R-Car Gen3 >>> platforms, but let's do it in a way that keeps everyone else happy as >>> well. >> >> Erm... that's not what I suggested. It's just that this regulator rework >> ( d6cd33ad7102 ) changed the behavior of the gpio-regulator because >> there are a lot of obscure, inobvious and undocumented edge-cases in the >> gpio-regulator code. > > Sorry if I misinterpreted what you were saying. Sorry, my English just sucks ;-/ >> If we were to revert this patch, we'd have to revert the d6cd33ad7102 >> too to fix the Gen3. But since d6cd33ad7102 is a nice cleanup and it >> makes sense, I'd rather if we fixed the situation, restored the DT >> handling to the way it was before and moved forward. > > Okay, sounds like we have the same goal, and from what you said earlier > the proposed fixup on top of your patch should fix this for everyone. > Let's hope there aren't any more nasty corner cases. Sounds good! >> Handling broken DTs only adds to the complexity, but I think this cannot >> be helped, since those DTs can be stored in some ROM. > > FWIW, the Jetson TX1 DT isn't technically broken because it used to work > fine. It's just that it relied on the quirks for correctness. So we are > in the lucky situation that the DT is not in a ROM and we can easily > update it, but I agree, others may be in a different situation, so let's > fix this so that things are back to normal for everyone. That doesn't > mean that we shouldn't fix DTs when we can, so I'll still send out that > DT patch for Jetson TX1. Thanks! >> btw if you can, also look at >> [PATCH] regulator: gpio: Reword the binding document >> the binding document could use a once-over. > > Okay, I'll take a look. :) -- Best regards, Marek Vasut