Re: [PATCH] gpio: of: Apply regulator-gpio quirk only to enable-gpios

[Marek Vasut <marek.vasut@gmail.com>]

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@gmail.com wrote:
>>>>>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>>>
>>>>>> 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@gmail.com>
>>>>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>>>>> Cc: Jan Kotas <jank@cadence.com>
>>>>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>>>>> Cc: Mark Brown <broonie@kernel.org>
>>>>>> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
>>>>>> Cc: linux-renesas-soc@vger.kernel.org
>>>>>> To: linux-gpio@vger.kernel.org
>>>>>> ---
>>>>>>  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