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.
> > The whole point of
> > these quirks is also to make sure that existing device trees continue to
> > work.
> >
> > Also, checking for only the enable-gpio property is wrong in this case.
> > enable-gpio is only specified for regulator-gpio. The standard GPIO for
> > regulator-fixed is "gpio", so the quirks must be applied to that
> > property in order to ensure backwards-compatibility.
> >
> >> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> >> index 03ec3ddaba60..1b4c741e0635 100644
> >> --- a/drivers/gpio/gpiolib-of.c
> >> +++ b/drivers/gpio/gpiolib-of.c
> >> @@ -84,6 +84,7 @@ static void of_gpio_flags_quirks(struct device_node *np,
> >> * Note that active low is the default.
> >> */
> >> if (IS_ENABLED(CONFIG_REGULATOR) &&
> >> + !strcmp(propname, "enable-gpio") &&
> >> (of_device_is_compatible(np, "regulator-fixed") ||
> >> of_device_is_compatible(np, "reg-fixed-voltage") ||
> >> of_device_is_compatible(np, "regulator-gpio"))) {
> >
> > I think this would need to be something like the below to reflect what
> > you were trying to achieve, while keeping backwards compatibility:
> >
> > --- >8 ---
> > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> > index 1b4c741e0635..bddfc6102a50 100644
> > --- a/drivers/gpio/gpiolib-of.c
> > +++ b/drivers/gpio/gpiolib-of.c
> > @@ -84,10 +84,10 @@ static void of_gpio_flags_quirks(struct device_node *np,
> > * Note that active low is the default.
> > */
> > if (IS_ENABLED(CONFIG_REGULATOR) &&
> > - !strcmp(propname, "enable-gpio") &&
> > (of_device_is_compatible(np, "regulator-fixed") ||
> > of_device_is_compatible(np, "reg-fixed-voltage") ||
> > - of_device_is_compatible(np, "regulator-gpio"))) {
> > + (of_device_is_compatible(np, "regulator-gpio") &&
> > + strcmp(propname, "enable-gpio") == 0))) {
> > /*
> > * The regulator GPIO handles are specified such that the
> > * presence or absence of "enable-active-high" solely controls
> > --- >8 ---
> >
> > That applied on top of today's linux-next fixes the regression for me
> > without a need to modify the device tree. Feel free to integrate that
> > into your patch.
>
> This one works on R-Car Gen3 too, so if we can add that on top of this
> patch, fine by me . Can you send a formal patch ?
Linus,
can you squash this into Marek's original commit? I'd rather not make
that two patches because that would potentially cause bisectability
issues.
Thierry
Attachment:
signature.asc
Description: PGP signature
