Re: 89a5e15bcba87d ("gpio/mmc/of: Respect polarity in the device tree")

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi Martin,

I'm sorry if I broke your "cd-inverted" boards :(

On Sat, Dec 29, 2018 at 3:26 PM Martin Blumenstingl
<martin.blumenstingl@xxxxxxxxxxxxxx> wrote:

> Today I noticed that my Odroid-C1 doesn't see it's SD card anymore.
> My board is not the only one, Odroid-C1 on Kernel CI suffers from the
> same problem:
> v4.20-6428-g00c569b567c7 still sees the SD card (mmc0): [1]
> v4.20-8955-g903b77c63167 doesn't see the SD card (mmc0) anymore: [2]
>
> Other affected boards:
> - Meson8b EC-100 (which I own)
> - Odroid-C2 on Kernel CI, see [3] and [4] (compared to Odroid-C1 it's
> mmc1 instead of mmc0. mmc0 on Odroid-C1 is the eMMC)
> - (I believe that all boards using "cd-inverted" are affected)
>
> My series from [0] is meant as an additional improvement for the
> 32-bit Amlogic Meson .dts files. We still need a fix for all other
> boards.
>
> I'm not sure how the MMC core / GPIO subsystems are supposed to handle
> "cd-inverted" since I believe this property is now evaluated twice:
> - first in of_gpio_flags_quirks
> - but also in mmc_gpio_get_cd (using ctx->override_cd_active_level)
> doesn't this essentially make "cd-inverted" a no-op?

This latter instance is now gone from the kernel if you
look in the mainline treee, we only have mmc_gpiod_request_cd()
nowadays.

mmc_gpiod_request_ro() is called from the generic DT
parser in host.c and the override_cd_active_level is
set to false.

I wonder if there was some confusion that development happened
in the GPIO and MMC tree in parallel? (Sorry about that.)

> If you want I can work on a patch which tries to bring everything back
> into a "working" state.

Yes please :)

> My understanding is that the old behavior was:
> - mmc_gpiod_request_cd / mmc_gpiod_request_ro used
> devm_gpiod_get_index which takes the polarity into account
> - check for "active low" != cd-inverted in drivers/mmc/core/host.c
> together with mmc_gpio_get_cd:
> -- active low, no cd-inverted: low means card is detected
> -- active low, cd-inverted: high means card is detected
> -- active high, no cd-inverted: low means card is detected
> -- active high, cd-inverted: high means card is detected
> - ACPI and OF based systems were treated the same
>
> My initial idea would was to remove the cd-inverted handling from the
> MMC core. But I'm not sure whether this would break ACPI based systems
> (which "somehow" set cd-inverted).

I think you should just do this. The XOR between the GPIO
descriptor flag and "cd-inverted" happens in gpiolib-of.c
now. The code in host.c should not worry about it.
As all GPIOs are now "active high" as seen from the outside,
it should just hammer down:
host->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
when calling mmc_of_parse().

My mistake to not kill off the same code from
host.c I guess. "cd-inverted" is not working because of
being handled in two places which ends up in mishmash.
Sorry ....

ACPI does not use this AFAICT, mmc_of_parse() is indeed
just using device properties, but grep mmc_of_parse
only yields device tree-based driver files.

(Tell me if you want me to patch it, it's better if you do
it since you seem to have some devices that see
problems.)

> My second idea is to revert the following two commits, but I'm not
> sure if that would break another case:
> - 89a5e15bcba87d ("gpio/mmc/of: Respect polarity in the device tree")
> - 81c85ec15a1946 ("gpio: OF: Parse MMC-specific CD and WP properties")

I would strongly prefer to fix the problem at its root instead,
so we have clean code that is easy to read and understand.
Currently that is our biggest problem aside from non-booting
systems...

Yours,
Linus Walleij