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

[Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>]

Hi Linus,

On Sun, Dec 30, 2018 at 8:50 PM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>
> Hi Martin,
>
> I'm sorry if I broke your "cd-inverted" boards :(
it's not like the world is going to end because of this... so don't
worry, we'll sort it out!

> 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.
it seems to me that mmc_gpio_request_cd was removed in commit
a622bb0a1e1f62 ("mmc: slot-gpio: Delete legacy GPIO handling").
mmc_gpio_get_cd is using gpiod

> mmc_gpiod_request_ro() is called from the generic DT
> parser in host.c and the override_cd_active_level is
> set to false.
mmc_gpiod_request_ro seems to contain some dead code: as far as I can
see all callers are passing override_active_level = false
however, mmc_gpiod_request_cd is a problem - see below

> 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.
there are two callers of mmc_gpiod_request_cd which are passing
override_active_level = true:
1) mmc_of_parse - looking back at this call site you're right that we
could remove the "inversion" here
2) sdhci_pci_probe_slot (drivers/mmc/host/sdhci-pci-core.c) uses
override_active_level = true for some Intel BayTrail devices...

so for 4.21 I propose the following fix:
change mmc_of_parse to pass "false" as 4th parameter to mmc_gpiod_request_cd.
I have tested this on one of my affected boards and it brings
GPIO_ACTIVE_HIGH + cd-inverted back to life.

for 4.22 someone who knows the SDHCI driver (and preferably has some
Intel BayTrail hardware) checks whether we need that
override_active_level = true (because I can't see anything that flips
MMC_CAP2_CD_ACTIVE_HIGH on, so I *assume* override_active_level is a
no-op in this case anyways).
then we can go and remove all override_active_level handling from the
CD and RO logic in drivers/mmc/{core,host}

> (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.)
if you want I can send a patch for 4.21 changing only mmc_of_parse.
I don't have any Intel BayTrail hardware so I'm probably not the right
person for patching sdhci_pci_probe_slot.
after that we can do the final cleanup by removing the
override_active_level (which at this point is always false) in
drivers/mmc/{core,host}

> > 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...
agreed


Regards
Martin