Bug 177101 - sdhci-acpi: Card detect setup fails on Asus T100HAN (cherry-trail)
Summary: sdhci-acpi: Card detect setup fails on Asus T100HAN (cherry-trail)
Status: RESOLVED CODE_FIX
Alias: None
Product: ACPI
Classification: Unclassified
Component: Other (show other bugs)
Hardware: Intel Linux
: P1 normal
Assignee: Zhang Rui
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-09 06:32 UTC by Jonas Aaberg
Modified: 2016-12-26 02:29 UTC (History)
1 user (show)

See Also:
Kernel Version: 4.8
Tree: Mainline
Regression: No


Attachments
My .config (44.26 KB, application/gzip)
2016-10-09 06:32 UTC, Jonas Aaberg
Details
dmesg output (44.04 KB, text/plain)
2016-10-09 06:32 UTC, Jonas Aaberg
Details
DSDT (45.72 KB, application/x-bzip)
2016-10-09 06:33 UTC, Jonas Aaberg
Details
dsdt with GPO3 described in _DEP (899.82 KB, text/x-dsl)
2016-11-29 08:31 UTC, Zhang Rui
Details
patch to add dependency checking for sdhci-acpi driver (1.41 KB, patch)
2016-12-02 03:38 UTC, Zhang Rui
Details | Diff
patch to support deferred probe if GPIO not ready in sdhci-acpi driver (1021 bytes, patch)
2016-12-07 05:15 UTC, Zhang Rui
Details | Diff

Description Jonas Aaberg 2016-10-09 06:32:37 UTC
Created attachment 241201 [details]
My .config

On my Asus T100HAN, cherry-trail, sdhci-acpi fails to figure out which
gpio that is the card detect pin. From dmesg:
--
# dmesg |grep sdh
[    1.293450] sdhci: Secure Digital Host Controller Interface driver
[    1.295946] sdhci: Copyright(c) Pierre Ossman
[    2.370378] sdhci-acpi 80860F14:03: GPIO lookup for consumer 80860F14:03 cd
[    2.370390] sdhci-acpi 80860F14:03: using ACPI for GPIO lookup
[    2.371059] sdhci-acpi 80860F14:03: lookup for GPIO 80860F14:03 cd failed
[    2.371070] sdhci-acpi 80860F14:03: failed to setup card detect gpio
--

consumer 80860F14:03 works fine, if I boot with an SD card inserted.

Note, the DSDT must be patched according to: 
https://github.com/p-an/T100HA-DSDT/commit/811047e224ac5bcae5922adcb7fdce6570537c96

otherwise neither wlan nor the sd card reader shows up.

In the attached DSDT, search for:
Device (SHC1)
for the configration of the card detect hardware.
Comment 1 Jonas Aaberg 2016-10-09 06:32:55 UTC
Created attachment 241211 [details]
dmesg output
Comment 2 Jonas Aaberg 2016-10-09 06:33:11 UTC
Created attachment 241221 [details]
DSDT
Comment 3 Zhang Rui 2016-10-12 02:47:33 UTC
does the problem still exist if you set CONFIG_PINCTRL_CHERRYVIEW=y?
Comment 4 Jonas Aaberg 2016-10-12 11:19:43 UTC
(In reply to Zhang Rui from comment #3)
> does the problem still exist if you set CONFIG_PINCTRL_CHERRYVIEW=y?

No, with CONFIG_PINCTRL_CHERRYVIEW=y card detect works just fine! Thanks a lot!
Comment 5 Zhang Rui 2016-11-29 06:33:57 UTC
So this is a problem that the GPIO dependency, which exists in _CRS, is not properly described via _DEP method, and results in a racing issue.
If we make GPIO controller driver load before the GPIO consumer driver (force built-in in this case), then problem is gone.

I'm thinking if we should propose a patch to handle the dependency described in _CRS.
Comment 6 Zhang Rui 2016-11-29 08:31:57 UTC
Created attachment 246211 [details]
dsdt with GPO3 described in _DEP

another to confirm this is to compile and apply the new dsdt attached, which has GPIO controller dependency described in _DEP method.
Please check if the problem still exists, even with CONFIG_PINTRL_CHERRYTRAIL=m.
Comment 7 Jonas Aaberg 2016-11-30 08:29:48 UTC
Build and tested with:
CONFIG_PINCTRL_CHERRYVIEW=m
and used your attached dsdt. Card detect doesn't work with that combination.
Comment 8 Zhang Rui 2016-12-02 03:38:24 UTC
Created attachment 246581 [details]
patch to add dependency checking for sdhci-acpi driver

oops, I forgot one thing.
You need to applied this patch as well as the customized DSDT.
If this works, then I will work out another two patches to do the hack in kernel, instead of customized DSDT.
Comment 9 Jonas Aaberg 2016-12-04 07:49:53 UTC
With this patch applied, the eMMC is not detected, and boot ends up in initramfs' shell.
I insmod'ed the pinctrl-cherryview module from an usbdrive, but nothing happen. I rmmod'ed the sdhci, sdhci-acpi and mmc_core.
Then I insmod'ed these modules again. But from dmesg's point of view nothing happened.

I don't get you added a check for mailbox device:
        !strcmp(info->hardware_id.string, "INT33BD"));
I was expecting INT33BB - sd controller.

So I patched your patch changing INT33BD to INT33BB, and tested. But nothing changed.
Comment 10 Zhang Rui 2016-12-05 02:30:20 UTC
        Device (SHC1)
        {
            Name (_HID, "80860F14" /* Intel Baytrail SDIO/MMC Host Controller */)  // _HID: Hardware ID
            Name (_CID, "PNP0D40" /* SDA Standard Compliant SD Host Controller */)  // _CID: Compatible ID
            Name (_DDN, "Intel(R) SD Card Controller - 80862296")  // _DDN: DOS Device Name
            Name (_UID, 0x03)  // _UID: Unique ID
            Name (_HRV, One)  // _HRV: Hardware Revision
            Name (_DEP, Package (0x02)  // _DEP: Dependencies
            {
                MBID,
                I2C7
            })
            ...
         }


This is the description of sdhci-acpi 80860F14:03.
what I'm doing the customized dsdt is to add GPO3 into the _DEP package.
what I'm doing in the patch is to add check for _DEP when SHC1 driver is probed.

With both applied, it should work in this way
1.During acpi initialization, we know that SHC1 has 3 dependencies (MBID, I2C7 and GPO3), and increase the dep_unmet count. The hacks I do in drivers/acpi/scan.c is to ignore the dependency of MBID device, which does not have a driver in Linux.
2.during I2C7 and GPO3 driver probing, it will decrease the dep_unmet count
3.for SHC1, it is probed only if dep_unmet is decreased to 0, which means all the dependencies are solved.
Comment 11 Zhang Rui 2016-12-05 02:31:24 UTC
> I don't get you added a check for mailbox device:
>         !strcmp(info->hardware_id.string, "INT33BD"));
> I was expecting INT33BB - sd controller.
> 
> So I patched your patch changing INT33BD to INT33BB, and tested. But nothing
> changed.

int33bd is the hid for MBID device.
Comment 12 Zhang Rui 2016-12-05 02:43:49 UTC
I think I know what the problem this.
My patches enables dependency checking, and the dependency for SHC1 is resolved.
But, at the same time, dependency checking is enabled for the other 80860F14 devices as well, which has unmet dependency.
PEPD (INT33A4) and PMIC (INT33FD) is referenced in _DEP for other 80860F14 devices.

to make a quick check, can you please modify the patch I attached a little,
in drivers/acpi/scan.c, ignore INT33A4 and INT33FD as well as INT33BD?

if it works, please check if ignoring INT33A4 and INT33BD is sufficient.
Comment 13 Jonas Aaberg 2016-12-05 07:10:54 UTC
Thanks for explaining!

Ignoring INT33A4 and INT33BD is sufficient. The eMMC is present when needed,
the sd-card reader and wifi (over sdio) works once pinctrl-cherryview
is modprobed. Card detect works fine.
Comment 14 Zhang Rui 2016-12-06 00:00:32 UTC
hmmm, can you please try some kernel later than 4.9-rc3?
The below commit seems to fix the order issue, in a different way, please verify if it works for you or not, when CONFIG_PINCTRL_CHERRYVIEW=m.

commit 67bf5156edc4f58241fd7c119ae145c552adddd6
Author: David Arcari <darcari@redhat.com>
Date:   Wed Oct 12 18:40:30 2016 +0200

    gpio / ACPI: fix returned error from acpi_dev_gpio_irq_get()
    
    acpi_dev_gpio_irq_get() currently ignores the error returned
    by acpi_get_gpiod_by_index() and overwrites it with -ENOENT.
    
    Problem is this error can be -EPROBE_DEFER, which just blows
    up some drivers when the module ordering is not correct.
    
    Cc: stable@vger.kernel.org
    Signed-off-by: David Arcari <darcari@redhat.com>
    Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
    Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
    Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Comment 15 Jonas Aaberg 2016-12-06 13:54:09 UTC
Sorry for that I've not mentioned, but I've been testing on 4.9-rc7.

But I'll retest on rc8 without any of you patches, but with PINCTRL_CHERRYVIEW=m
Comment 16 Jonas Aaberg 2016-12-06 19:27:59 UTC
Retested with vanilla 4.9-rc8 + PINCTRL_CHERRYVIEW=m + my dsdt and then with yours. Neither did work.
Comment 17 Zhang Rui 2016-12-07 05:07:43 UTC
here is the code flow for requiring an ACPI GPIO resource in sdhci-acpi driver
sdhci_acpi_probe->mmc_gpiod_request_cd->devm_gpiod_get_index->gpiod_get_index->acpi_find_gpio->acpi_get_gpiod_by_index->acpi_gpio_resource_lookup->acpi_populate_gpio_lookup->acpi_get_gpiod

acpi_get_gpiod returns -EPROBE_DEFER when the GPIO chip is not registered yet.

But, the problem here is that, in sdhci_acpi_probe(), the return value of mmc_gpiod_request_cd() is ignored. Hard to say it is right or not, but if we change the code to something like
        if (sdhci_acpi_flag(c, SDHCI_ACPI_SD_CD)) {
                bool v = sdhci_acpi_flag(c, SDHCI_ACPI_SD_CD_OVERRIDE_LEVEL);

                err = mmc_gpiod_request_cd(host->mmc, NULL, 0, v, 0, NULL);
                if (err) {
                        if (err = -EPROBE_DEFER)
                                 cleanup and return err;
                        dev_warn(dev, "failed to setup card detect gpio\n");
                        c->use_runtime_pm = false;
                }
        }
then nothing needs to be changed in ACPI code, the dependency problem is resolved even if PINCTRL_CHERRYVIEW=m.

Anyway, I'm not sure this is the right behaviour and what cleanup needs to be done before return, let me raise this problem to Adrian Hunter, the sdhci-acpi expert.
Comment 18 Zhang Rui 2016-12-07 05:15:10 UTC
Created attachment 247061 [details]
patch to support deferred probe if GPIO not ready in sdhci-acpi driver

please try this patch on top of 4.9-rc8 kernel, with your dsdt file, without any other patch and see if the problem is gone.
Comment 19 Jonas Aaberg 2016-12-07 10:16:04 UTC
Nope, that patch on 4.9-rc8 with my dtds didn't work :-(

Might be completely unrelated, but it is in the same area. I filed a bug on that on this computer you have to do changes to the dsdt in order to get the sd card reader and wifi (over sdio) to work at all:

https://bugzilla.kernel.org/show_bug.cgi?id=189801

Maybe that DSDT workaround/fix has something to do with this issue.
Comment 20 Zhang Rui 2016-12-08 01:31:44 UTC
(In reply to Jonas Aaberg from comment #0)
> # dmesg |grep sdh
> [    1.293450] sdhci: Secure Digital Host Controller Interface driver
> [    1.295946] sdhci: Copyright(c) Pierre Ossman
> [    2.370378] sdhci-acpi 80860F14:03: GPIO lookup for consumer 80860F14:03
> cd
> [    2.370390] sdhci-acpi 80860F14:03: using ACPI for GPIO lookup
> [    2.371059] sdhci-acpi 80860F14:03: lookup for GPIO 80860F14:03 cd failed
> [    2.371070] sdhci-acpi 80860F14:03: failed to setup card detect gpio

with the patch in comment #18, the symptom is exactly the same as comment #0, except we lose this line "sdhci-acpi 80860F14:03: failed to setup card detect gpio", right?
Comment 21 Jonas Aaberg 2016-12-08 08:08:20 UTC
On the contrary, with your patch on rc8 I get loads of "failed to setup cards detect gpio:"

dmesg |grep sdh

[    2.096840] sdhci: Secure Digital Host Controller Interface driver
[    2.099692] sdhci: Copyright(c) Pierre Ossman
[    3.194645] sdhci-acpi 80860F14:03: GPIO lookup for consumer 80860F14:03 cd
[    3.194657] sdhci-acpi 80860F14:03: using ACPI for GPIO lookup
[    3.195303] sdhci-acpi 80860F14:03: lookup for GPIO 80860F14:03 cd failed
[    3.250903] sdhci-acpi 80860F14:03: GPIO lookup for consumer 80860F14:03 cd
[    3.250908] sdhci-acpi 80860F14:03: using ACPI for GPIO lookup
[    3.251131] sdhci-acpi 80860F14:03: lookup for GPIO 80860F14:03 cd failed
[    3.268194] sdhci-acpi 80860F14:03: GPIO lookup for consumer 80860F14:03 cd
[    3.268202] sdhci-acpi 80860F14:03: using ACPI for GPIO lookup
[    3.268795] sdhci-acpi 80860F14:03: lookup for GPIO 80860F14:03 cd failed
[    3.791110] sdhci-acpi 80860F14:03: GPIO lookup for consumer 80860F14:03 cd
[    3.791115] sdhci-acpi 80860F14:03: using ACPI for GPIO lookup
[    3.791413] sdhci-acpi 80860F14:03: lookup for GPIO 80860F14:03 cd failed
[    4.203745] sdhci-acpi 80860F14:03: GPIO lookup for consumer 80860F14:03 cd
[    4.203785] sdhci-acpi 80860F14:03: using ACPI for GPIO lookup
[    4.203952] sdhci-acpi 80860F14:03: lookup for GPIO 80860F14:03 cd failed
[    4.414246] sdhci-acpi 80860F14:03: GPIO lookup for consumer 80860F14:03 cd
[    4.414250] sdhci-acpi 80860F14:03: using ACPI for GPIO lookup
[    4.414401] sdhci-acpi 80860F14:03: lookup for GPIO 80860F14:03 cd failed
[    4.433935] sdhci-acpi 80860F14:03: GPIO lookup for consumer 80860F14:03 cd
[    4.433938] sdhci-acpi 80860F14:03: using ACPI for GPIO lookup
[    5.930499] sdhci: Timeout waiting for Buffer Read Ready interrupt during tuning procedure, falling back to fixed sampling clock

Otherwise the behavior is as in comment #0
Comment 22 Zhang Rui 2016-12-08 13:14:19 UTC
(In reply to Jonas Aaberg from comment #21)
> On the contrary, with your patch on rc8 I get loads of "failed to setup
> cards detect gpio:"
> 
> dmesg |grep sdh
> 
> [    2.096840] sdhci: Secure Digital Host Controller Interface driver
> [    2.099692] sdhci: Copyright(c) Pierre Ossman
> [    3.194645] sdhci-acpi 80860F14:03: GPIO lookup for consumer 80860F14:03
> cd
> [    3.194657] sdhci-acpi 80860F14:03: using ACPI for GPIO lookup
> [    3.195303] sdhci-acpi 80860F14:03: lookup for GPIO 80860F14:03 cd failed

sdhci-acpi driver probe is deferred here.

> [    3.250903] sdhci-acpi 80860F14:03: GPIO lookup for consumer 80860F14:03
> cd
> [    3.250908] sdhci-acpi 80860F14:03: using ACPI for GPIO lookup
> [    3.251131] sdhci-acpi 80860F14:03: lookup for GPIO 80860F14:03 cd failed

sdhci-acpi driver is rescheduled and re-deferred here.
and same below.

Looks like the patch works, sdhci-acpi driver is deferred because GPIO is not ready. But then the problem is that, why the pinctrl-cherrytrail driver is not loaded in the end? It's built as module, and it should be loaded automatically by udev.
Comment 23 Zhang Rui 2016-12-08 13:20:14 UTC
can you please reboot with boot option "initcall_debug" and then attach the full dmesg output after boot?
please attach the output of "ls -l /sys/bus/platform/devices/INT33FF*/", and "cat /sys/bus/platform/devices/INT33FF*/*" after boot.
Comment 24 Jonas Aaberg 2016-12-12 08:03:27 UTC
When I added initcall_debug, I noticed that 4.9rc8+your patch from comment #18
+ my dsdt did work! I rebuilt to make sure I test on the right build and tested again without initcall debug, it does still work!

A difference from the working case, is that during the none working case I got this print:

--[    5.930499] sdhci: Timeout waiting for Buffer Read Ready interrupt during tuning procedure, falling back to fixed sampling clock
--
Maybe that did stop the card detect from working? unfortunately, I don't have the kernel log left from the none-working cases.

I will try to reproduce the none-working case again.
Comment 25 Zhang Rui 2016-12-12 08:46:25 UTC
(In reply to Jonas Aaberg from comment #24)
> When I added initcall_debug, I noticed that 4.9rc8+your patch from comment
> #18
> + my dsdt did work! I rebuilt to make sure I test on the right build and
> tested again without initcall debug, it does still work!
> 
Good news to me. :)

> A difference from the working case, is that during the none working case I
> got this print:
> 
> --[    5.930499] sdhci: Timeout waiting for Buffer Read Ready interrupt
> during tuning procedure, falling back to fixed sampling clock
> --
> Maybe that did stop the card detect from working? unfortunately, I don't
> have the kernel log left from the none-working cases.
> 
> I will try to reproduce the none-working case again.

yes, please. And if it can be reproduced, it indicates the patch in comment #18 is not sufficient, maybe we need some help from the sdhci experts.
Comment 26 Zhang Rui 2016-12-22 03:00:53 UTC
ping...
if you can not reproduce the problem any more after applying the patch in comment #18, I will send this patch out and close this bug report.
Comment 27 Jonas Aaberg 2016-12-22 07:21:43 UTC
Sorry for my silence. No, I've not been able to reproduce the sdhci time out bug.
However, in any case it should be reported as a new bug, since it is probably unrelated.

Please go ahead and close this bug. Patch in comment #18 is a working fix. Please add me as tested by, if you want to.

Thanks!
Comment 28 Zhang Rui 2016-12-26 02:29:00 UTC
patch sent at https://patchwork.kernel.org/patch/9488397/

Note You need to log in before you can comment on or make changes to this bug.