| Message ID | 20181126141714.18399-1-linus.walleij@linaro.org |
|---|---|
| State | Accepted |
| Commit | 81c85ec15a1946f2e347ec0bf66936121eb97ce7 |
| Headers | show |
| Series | gpio: OF: Parse MMC-specific CD and WP properties | expand |
On Mon, Nov 26, 2018 at 03:17:14PM +0100, Linus Walleij wrote: > When retrieveing CD (card detect) and WP (write protect) > GPIO handles from the device tree, make sure to assign > them active low by default unless the "cd-inverted" or > "wp-inverted" properties are set. These properties mean > that respective signal is active HIGH since the SDHCI > specification stipulates that this kind of signals > should be treated as active LOW. > > If the twocell GPIO flag is also specified as active > low, well that's nice and we will silently ignore the > tautological specification. > > If however the GPIO line is specified as active low > in the GPIO flasg cell and "cd-inverted" or "wp-inverted" > is also specified, the latter takes precedence and we > print a warning. > > The current effect on the MMC slot-gpio core are as > follows: > > For CD GPIOs: no effect. The current code in > mmc/core/host.c calls mmc_gpiod_request_cd() with > the "override_active_level" argument set to true, > which means that whatever the GPIO descriptor > thinks about active low/high will be ignored, the > core will use the MMC_CAP2_CD_ACTIVE_HIGH to keep > track of this and reads the raw value from the > GPIO descriptor, totally bypassing gpiolibs inversion > semantics. I plan to clean this up at a later point > passing the handling of inversion semantics over > to gpiolib, so this patch prepares the ground for > that. > > Fow WP GPIOs: this is probably fixing a bug, because > the code in mmc/core/host.c calls mmc_gpiod_request_ro() > with the "override_active_level" argument set to false, > which means it will respect the inversion semantics of > the gpiolib and ignore the MMC_CAP2_RO_ACTIVE_HIGH > flag for everyone using this through device tree. > However the code in host.c confusingly goes to great > lengths setting up the MMC_CAP2_RO_ACTIVE_HIGH flag > from the GPIO descriptor and by reading the "wp-inverted" > property of the node. As far as I can tell this is all > in vain and the inversion is broken: device trees that > use "wp-inverted" do not work as intended, instead the > only way to actually get inversion on a line is by > setting the second cell flag to GPIO_ACTIVE_HIGH (which > will be the default) or GPIO_ACTIVE_LOW if they want > the proper MMC semantics. Presumably all device trees do > this right but we need to parse and handle this properly. > > Cc: linux-mmc@vger.kernel.org > Cc: linux-gpio@vger.kernel.org > Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> At least for qemu this doeesn't work: It causes boots from mmc to fail for vexpress-a9 and vexpress-a15 machine types. Reason is that OF_GPIO_ACTIVE_LOW was so far not set for those machine types, but it is now set by the code below. This may also affect other machines - hard to say, since there are lots of boot failures in -next right now, and the symptoms are often similar. Since the lack of {cd,wp}-inverted now always sets OF_GPIO_ACTIVE_LOW, I suspect that all active-high configurations which do not also explicitly specify {cd,wp}-inverted in their devicetree files are now broken. Guenter > --- > drivers/gpio/gpiolib-of.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c > index 7f1260c78270..50a390251410 100644 > --- a/drivers/gpio/gpiolib-of.c > +++ b/drivers/gpio/gpiolib-of.c > @@ -57,6 +57,45 @@ static void of_gpio_flags_quirks(struct device_node *np, > enum of_gpio_flags *flags, > int index) > { > + /* > + * Handle MMC "cd-inverted" and "wp-inverted" semantics. > + */ > + if (IS_ENABLED(CONFIG_MMC)) { > + if (of_property_read_bool(np, "cd-gpios")) { > + if (of_property_read_bool(np, "cd-inverted")) { > + if (*flags & OF_GPIO_ACTIVE_LOW) { > + /* "cd-inverted" takes precedence */ > + *flags &= ~OF_GPIO_ACTIVE_LOW; > + pr_warn("%s GPIO handle specifies CD active low - ignored\n", > + of_node_full_name(np)); > + } > + } else { > + /* > + * Active low is the default according to the > + * SDHCI specification. If the GPIO handle > + * specifies the same thing - good. > + */ > + *flags |= OF_GPIO_ACTIVE_LOW; > + } > + } > + if (of_property_read_bool(np, "wp-gpios")) { > + if (of_property_read_bool(np, "wp-inverted")) { > + /* "wp-inverted" takes precedence */ > + if (*flags & OF_GPIO_ACTIVE_LOW) { > + *flags &= ~OF_GPIO_ACTIVE_LOW; > + pr_warn("%s GPIO handle specifies WP active low - ignored\n", > + of_node_full_name(np)); > + } > + } else { > + /* > + * Active low is the default according to the > + * SDHCI specification. If the GPIO handle > + * specifies the same thing - good. > + */ > + *flags |= OF_GPIO_ACTIVE_LOW; > + } > + } > + } > /* > * Some GPIO fixed regulator quirks. > * Note that active low is the default.
On 12/17/18 6:10 AM, Linus Walleij wrote: > On Sun, Dec 16, 2018 at 8:43 PM Guenter Roeck <linux@roeck-us.net> wrote: > >> At least for qemu this doeesn't work: It causes boots from mmc to >> fail for vexpress-a9 and vexpress-a15 machine types. > > Oh not good. How do I reproduce this? > > My qemu command line looks like this: > qemu-system-arm -M vexpress-a9 -no-reboot -smp cpus=4 -kernel > ${HOME}/zImage -dtb ${HOME}/vexpress-v2p-ca9.dtb -hda hda.img -append > "root=/dev/sda init=init console=ttyAMA0" -serial stdio > I use: /opt/buildbot/qemu-install/v3.0/bin/qemu-system-arm -M vexpress-a9 \ -kernel arch/arm/boot/zImage -no-reboot \ -snapshot -drive file=rootfs-armv5.ext2,format=raw,if=sd \ -append 'root=/dev/mmcblk0 rootwait rw console=ttyAMA0,115200 console=tty1' \ -nographic Works fine for me, and always did (up to now, that is). > I can't figure out how to make QEMU Vexpress boot from MMC, is it > some option that is not documented in man qemu-system-arm? > >> Reason is that >> OF_GPIO_ACTIVE_LOW was so far not set for those machine types, but >> it is now set by the code below. This may also affect other machines - >> hard to say, since there are lots of boot failures in -next right now, >> and the symptoms are often similar. >> >> Since the lack of {cd,wp}-inverted now always sets OF_GPIO_ACTIVE_LOW, >> I suspect that all active-high configurations which do not also explicitly >> specify {cd,wp}-inverted in their devicetree files are now broken. > > Indeed this can be the problem. > > The device tree node (arch/arm/boot/dts/vexpress-v2m.dtsi) looks > like this: > > mmci@5000 { > compatible = "arm,pl180", > "arm,primecell"; > reg = <0x05000 0x1000>; > interrupts = <9 10>; > cd-gpios = <&v2m_mmc_gpios 0 0>; > wp-gpios = <&v2m_mmc_gpios 1 0>; > max-frequency = <12000000>; > vmmc-supply = <&v2m_fixed_3v3>; > clocks = <&v2m_clk24mhz>, <&smbclk>; > clock-names = "mclk", "apb_pclk"; > }; > > According to the bindings, this means that the GPIOs are active > low, because all CD/WP lines are active low unless > "[cd|ro]-inverted" is specified, despite the fact that the second > cell on the gpios here is 0 meaning they should be active high. > > But since all ARM reference designs I have seen actually have > active low GPIOs for this I am a bit confused. > I can't comment on that. I only see that qemu no longer works after your patch has been applied, and it used to work with all older kernels I ever tested. Either case, your patch does introduce a semantic change which affects all systems with active high cd and wp pins (and, frankly, I don't see the point of the {cd,wp}-invert properties; they seem redundant and ask for trouble). Thanks, Guenter > I suspect it's rather some bug making them active high. But I > would need to reproduce and look closer. > > Yours, > Linus Walleij >
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index 7f1260c78270..50a390251410 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -57,6 +57,45 @@ static void of_gpio_flags_quirks(struct device_node *np, enum of_gpio_flags *flags, int index) { + /* + * Handle MMC "cd-inverted" and "wp-inverted" semantics. + */ + if (IS_ENABLED(CONFIG_MMC)) { + if (of_property_read_bool(np, "cd-gpios")) { + if (of_property_read_bool(np, "cd-inverted")) { + if (*flags & OF_GPIO_ACTIVE_LOW) { + /* "cd-inverted" takes precedence */ + *flags &= ~OF_GPIO_ACTIVE_LOW; + pr_warn("%s GPIO handle specifies CD active low - ignored\n", + of_node_full_name(np)); + } + } else { + /* + * Active low is the default according to the + * SDHCI specification. If the GPIO handle + * specifies the same thing - good. + */ + *flags |= OF_GPIO_ACTIVE_LOW; + } + } + if (of_property_read_bool(np, "wp-gpios")) { + if (of_property_read_bool(np, "wp-inverted")) { + /* "wp-inverted" takes precedence */ + if (*flags & OF_GPIO_ACTIVE_LOW) { + *flags &= ~OF_GPIO_ACTIVE_LOW; + pr_warn("%s GPIO handle specifies WP active low - ignored\n", + of_node_full_name(np)); + } + } else { + /* + * Active low is the default according to the + * SDHCI specification. If the GPIO handle + * specifies the same thing - good. + */ + *flags |= OF_GPIO_ACTIVE_LOW; + } + } + } /* * Some GPIO fixed regulator quirks. * Note that active low is the default.
When retrieveing CD (card detect) and WP (write protect) GPIO handles from the device tree, make sure to assign them active low by default unless the "cd-inverted" or "wp-inverted" properties are set. These properties mean that respective signal is active HIGH since the SDHCI specification stipulates that this kind of signals should be treated as active LOW. If the twocell GPIO flag is also specified as active low, well that's nice and we will silently ignore the tautological specification. If however the GPIO line is specified as active low in the GPIO flasg cell and "cd-inverted" or "wp-inverted" is also specified, the latter takes precedence and we print a warning. The current effect on the MMC slot-gpio core are as follows: For CD GPIOs: no effect. The current code in mmc/core/host.c calls mmc_gpiod_request_cd() with the "override_active_level" argument set to true, which means that whatever the GPIO descriptor thinks about active low/high will be ignored, the core will use the MMC_CAP2_CD_ACTIVE_HIGH to keep track of this and reads the raw value from the GPIO descriptor, totally bypassing gpiolibs inversion semantics. I plan to clean this up at a later point passing the handling of inversion semantics over to gpiolib, so this patch prepares the ground for that. Fow WP GPIOs: this is probably fixing a bug, because the code in mmc/core/host.c calls mmc_gpiod_request_ro() with the "override_active_level" argument set to false, which means it will respect the inversion semantics of the gpiolib and ignore the MMC_CAP2_RO_ACTIVE_HIGH flag for everyone using this through device tree. However the code in host.c confusingly goes to great lengths setting up the MMC_CAP2_RO_ACTIVE_HIGH flag from the GPIO descriptor and by reading the "wp-inverted" property of the node. As far as I can tell this is all in vain and the inversion is broken: device trees that use "wp-inverted" do not work as intended, instead the only way to actually get inversion on a line is by setting the second cell flag to GPIO_ACTIVE_HIGH (which will be the default) or GPIO_ACTIVE_LOW if they want the proper MMC semantics. Presumably all device trees do this right but we need to parse and handle this properly. Cc: linux-mmc@vger.kernel.org Cc: linux-gpio@vger.kernel.org Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/gpio/gpiolib-of.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) -- 2.19.1