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@xxxxxxxxxxxxxxx
> Cc: linux-gpio@xxxxxxxxxxxxxxx
> Cc: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
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.
