| Message ID | 20181018215101.169847-1-rajatja@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: sdhci-pci: Try "cd" for card-detect lookup before using NULL | expand |
On Fri, Oct 19, 2018 at 12:53 AM Rajat Jain <rajatja@google.com> wrote: > > Problem: > > The card detect IRQ does not work with modern BIOS (that want > to use DSD to provide the card detect GPIO to the driver). > > Details: > > (Discussion: https://lkml.org/lkml/2018/9/25/1113) We have a Link tag for such references. > The mmc core provides the mmc_gpiod_request_cd() API to let host drivers > request the gpio descriptor for the "card detect" (or carrier detect?) pin. card detect is a right term. > This pin is specified in the ACPI for the SDHC device: > > * Either as a resource using _CRS. This is a method used by legacy BIOS. > (The driver needs to tell which resource index). > > * Or as a named property ("cd-gpio") in DSD (which may internally point cd-gpios (gpio suffix is a legacy). > to an entry in _CRS). This way, the driver can lookup using a string. > This is what modern BIOS prefer to use. > > This API finally results in a call to the following code: > > struct gpio_desc *acpi_find_gpio(..., const char *con_id,...) > { > ... > /* Lookup gpio (using "<con_id>-gpio") in the _DSD */ > ... > if (!acpi_can_fallback_to_crs(adev, con_id)) > return ERR_PTR(-ENOENT); > ... > /* Falling back to _CRS is allowed, Lookup gpio in the _CRS */ > ... > } > > Note that this means that if the ACPI has _DSD properties, the kernel > will never use _CRS for the lookup (Because acpi_can_fallback_to_crs() > will always be false for any device hat has _DSD entries). > > The SDHCI driver is thus currently broken on a modern BIOS > (even if > BIOS provides both _CRS and DSD entries, either of which could be used for _DSD > a successful lookup). This is incorrect. _DSD for GPIOs without any accompanying _CRS doesn't make any sense. > Ironically, none of these will be used for the > lookup currently because: > > * Since the con_id is NULL, acpi_find_gpio() does not find a matching > entry in DSDT. (The DSDT entry has the property name = "cd-gpio") cd-gpios > > * Because ACPI contains DSDT entries, thus acpi_can_fallback_to_crs() > returns false (because device properties have been populated from > DSD), thus the _CRS is never used for the lookup. _DSD > > Fix: > > Try "cd" for lookup in the _DSD before falling back to using NULL so > as to try looking up in the _CRS. > > I've tested this patch successfully with both Legacy BIOS (that > provide only _CRS method) as well as modern BIOS (that provide both > _CRS and DSD). Also the use of "cd" also appears to be farly consistent _DSD fairly > across other users of this API (other MMC host controller drivers). > if (slot->cd_idx >= 0) { > - ret = mmc_gpiod_request_cd(host->mmc, NULL, slot->cd_idx, > + ret = mmc_gpiod_request_cd(host->mmc, "cd", slot->cd_idx, > slot->cd_override_level, 0, NULL); Yes. > + if (ret && ret != -EPROBE_DEFER) > + ret = mmc_gpiod_request_cd(host->mmc, NULL, > + slot->cd_idx, > + slot->cd_override_level, > + 0, NULL); And no. Instead of this part you need to provide an ACPI GPIO mapping table. See examples, like net/rfkill/rfkill-gpio.c (look for acpi_rfkill_default_gpios) > if (ret == -EPROBE_DEFER) > goto remove;
Hi Andy, Thanks for your review. On Fri, Oct 19, 2018 at 2:13 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Oct 19, 2018 at 12:53 AM Rajat Jain <rajatja@google.com> wrote: > > > > Problem: > > > > The card detect IRQ does not work with modern BIOS (that want > > to use DSD to provide the card detect GPIO to the driver). > > > > Details: > > > > > (Discussion: https://lkml.org/lkml/2018/9/25/1113) > > We have a Link tag for such references. > > > The mmc core provides the mmc_gpiod_request_cd() API to let host drivers > > request the gpio descriptor for the "card detect" (or carrier detect?) pin. > > card detect is a right term. > > > This pin is specified in the ACPI for the SDHC device: > > > > * Either as a resource using _CRS. This is a method used by legacy BIOS. > > (The driver needs to tell which resource index). > > > > * Or as a named property ("cd-gpio") in DSD (which may internally point > > cd-gpios (gpio suffix is a legacy). > > > to an entry in _CRS). This way, the driver can lookup using a string. > > This is what modern BIOS prefer to use. > > > > This API finally results in a call to the following code: > > > > struct gpio_desc *acpi_find_gpio(..., const char *con_id,...) > > { > > ... > > /* Lookup gpio (using "<con_id>-gpio") in the _DSD */ > > ... > > if (!acpi_can_fallback_to_crs(adev, con_id)) > > return ERR_PTR(-ENOENT); > > ... > > /* Falling back to _CRS is allowed, Lookup gpio in the _CRS */ > > ... > > } > > > > Note that this means that if the ACPI has _DSD properties, the kernel > > will never use _CRS for the lookup (Because acpi_can_fallback_to_crs() > > will always be false for any device hat has _DSD entries). > > > > The SDHCI driver is thus currently broken on a modern BIOS > > > (even if > > BIOS provides both _CRS and DSD entries, either of which could be used for > > _DSD > > > a successful lookup). > > This is incorrect. _DSD for GPIOs without any accompanying _CRS > doesn't make any sense. > > > Ironically, none of these will be used for the > > lookup currently because: > > > > * Since the con_id is NULL, acpi_find_gpio() does not find a matching > > entry in DSDT. (The DSDT entry has the property name = "cd-gpio") > > cd-gpios > > > > > * Because ACPI contains DSDT entries, thus acpi_can_fallback_to_crs() > > returns false (because device properties have been populated from > > DSD), thus the _CRS is never used for the lookup. > > _DSD > > > > > Fix: > > > > Try "cd" for lookup in the _DSD before falling back to using NULL so > > as to try looking up in the _CRS. > > > > I've tested this patch successfully with both Legacy BIOS (that > > provide only _CRS method) as well as modern BIOS (that provide both > > _CRS and DSD). Also the use of "cd" also appears to be farly consistent > > _DSD > fairly I can fix the commit log to take care of all your review comments. > > > across other users of this API (other MMC host controller drivers). > > > if (slot->cd_idx >= 0) { > > - ret = mmc_gpiod_request_cd(host->mmc, NULL, slot->cd_idx, > > + ret = mmc_gpiod_request_cd(host->mmc, "cd", slot->cd_idx, > > slot->cd_override_level, 0, NULL); > > Yes. > > > + if (ret && ret != -EPROBE_DEFER) > > + ret = mmc_gpiod_request_cd(host->mmc, NULL, > > + slot->cd_idx, > > + slot->cd_override_level, > > + 0, NULL); > > And no. Instead of this part you need to provide an ACPI GPIO mapping table. Sure, I am willing to do so, and I tried earlier too. However, certain doubts arose in my mind when I tried that and I posted my questions earlier (https://lkml.org/lkml/2018/9/28/507) but couldn't elicit any response. Unfortunately I still do not have answers. My primary questions are: 1) - It seems that 1 SDHCI device may support multiple slots (looking at the code). It is not clear to me if they could share card detect interrupts, or should have separate ones? Also, the driver may not really know? So should I add 1 or two pins using the devm_acpi_dev_add_driver_gpios(). Is some one familiar with SDHC driver can answer these questions, it shall be great. 2) I'm not really sure what should I set "active_low" to? Isn't this something that should be specified by platform / ACPI too, and driver should just be able to say say choose whatever the ACPI says? struct acpi_gpio_params { unsigned int crs_entry_index; unsigned int line_index; bool active_low; }; Since I do not understand the above two issues, and thus I chose the safest path and not disturb the current code so as not to cause any regressions. Please let me know, and I'm happy to re-spin my patch. Thanks, Rajat > > See examples, like > net/rfkill/rfkill-gpio.c > > (look for acpi_rfkill_default_gpios) > > > if (ret == -EPROBE_DEFER) > > goto remove; > > -- > With Best Regards, > Andy Shevchenko
On Mon, Oct 22, 2018 at 04:34:55PM -0700, Rajat Jain wrote: > On Fri, Oct 19, 2018 at 2:13 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Fri, Oct 19, 2018 at 12:53 AM Rajat Jain <rajatja@google.com> wrote: > > > across other users of this API (other MMC host controller drivers). > > > > > if (slot->cd_idx >= 0) { > > > - ret = mmc_gpiod_request_cd(host->mmc, NULL, slot->cd_idx, > > > + ret = mmc_gpiod_request_cd(host->mmc, "cd", slot->cd_idx, > > > slot->cd_override_level, 0, NULL); > > > > Yes. > > > > > + if (ret && ret != -EPROBE_DEFER) > > > + ret = mmc_gpiod_request_cd(host->mmc, NULL, > > > + slot->cd_idx, > > > + slot->cd_override_level, > > > + 0, NULL); > > > > And no. Instead of this part you need to provide an ACPI GPIO mapping table. > > Sure, I am willing to do so, and I tried earlier too. However, certain > doubts arose in my mind when I tried that and I posted my questions > earlier (https://lkml.org/lkml/2018/9/28/507) but couldn't elicit any > response. Unfortunately I still do not have answers. My primary > questions are: > > 1) - It seems that 1 SDHCI device may support multiple slots (looking > at the code). It is not clear to me if they could share card detect > interrupts, or should have separate ones? This is more likely question to HW engineers of your platform with a caveat that there should be a way to distinguish exact slot in which card is being inserted. > Also, the driver may not > really know? I think in such case the bug in HW design and / or driver. > So should I add 1 or two pins using the > devm_acpi_dev_add_driver_gpios(). This depends on the above, e.g. HW design, ACPI tables. > Is some one familiar with SDHC > driver can answer these questions, it shall be great. Actually above questions better to ask in linux-mmc mailing list, which by the fact is in Cc list already. So, wait for someone to clarify. > 2) I'm not really sure what should I set "active_low" to? Isn't this > something that should be specified by platform / ACPI too, and driver > should just be able to say say choose whatever the ACPI says? > > struct acpi_gpio_params { > unsigned int crs_entry_index; > unsigned int line_index; > bool active_low; > }; ACPI specification misses this property, that's why we have it in the structure. In your case it should be provided by _DSD and thus be consistent with the hardcoded values. > Since I do not understand the above two issues, and thus I chose the > safest path and not disturb the current code so as not to cause any > regressions. As far as I can see in the above it is disturbing the current code more than needed. > > Please let me know, and I'm happy to re-spin my patch.
Hi Andy, On Wed, Oct 24, 2018 at 3:02 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Oct 22, 2018 at 04:34:55PM -0700, Rajat Jain wrote: > > On Fri, Oct 19, 2018 at 2:13 AM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Fri, Oct 19, 2018 at 12:53 AM Rajat Jain <rajatja@google.com> wrote: > > > > > across other users of this API (other MMC host controller drivers). > > > > > > > if (slot->cd_idx >= 0) { > > > > - ret = mmc_gpiod_request_cd(host->mmc, NULL, slot->cd_idx, > > > > + ret = mmc_gpiod_request_cd(host->mmc, "cd", slot->cd_idx, > > > > slot->cd_override_level, 0, NULL); > > > > > > Yes. > > > > > > > + if (ret && ret != -EPROBE_DEFER) > > > > + ret = mmc_gpiod_request_cd(host->mmc, NULL, > > > > + slot->cd_idx, > > > > + slot->cd_override_level, > > > > + 0, NULL); > > > > > > And no. Instead of this part you need to provide an ACPI GPIO mapping table. > > > > Sure, I am willing to do so, and I tried earlier too. However, certain > > doubts arose in my mind when I tried that and I posted my questions > > earlier (https://lkml.org/lkml/2018/9/28/507) but couldn't elicit any > > response. Unfortunately I still do not have answers. My primary > > questions are: > > > > 1) - It seems that 1 SDHCI device may support multiple slots (looking > > at the code). It is not clear to me if they could share card detect > > interrupts, or should have separate ones? > > This is more likely question to HW engineers of your platform with a caveat > that there should be a way to distinguish exact slot in which card is being > inserted. > > > Also, the driver may not > > really know? > > I think in such case the bug in HW design and / or driver. Why? You can have a shared or dedicated interrupt and the driver does not really need to know if it can poll the status. > > > So should I add 1 or two pins using the > > devm_acpi_dev_add_driver_gpios(). > > This depends on the above, e.g. HW design, ACPI tables. Yes, it depends on the HW design and that is exactly why the approach with devm_acpi_dev_add_driver_gpios() does not work well here: this is a generic driver used on many platforms and you are trying to put the platform knowledge into the driver. Here we are lucky I guess as I do not believe anyone is using more than one slot, so we can have a tavle with a single entry, but actually doing the fallback the way Rajat was proposing is more correct. Or you have a table with N entries, where N is hopefully sufficiently large. > > > > Is some one familiar with SDHC > > driver can answer these questions, it shall be great. > > Actually above questions better to ask in linux-mmc mailing list, which by the > fact is in Cc list already. So, wait for someone to clarify. > > > > 2) I'm not really sure what should I set "active_low" to? Isn't this > > something that should be specified by platform / ACPI too, and driver > > should just be able to say say choose whatever the ACPI says? > > > > struct acpi_gpio_params { > > unsigned int crs_entry_index; > > unsigned int line_index; > > bool active_low; > > }; > > > ACPI specification misses this property, that's why we have it in the > structure. In your case it should be provided by _DSD and thus be consistent > with the hardcoded values. Again, you think as if the driver was platform specific; it is not. I have 1000s of systems with different ACPI tables. Let's say half of them use one polarity, and half another. Which polarity do you propose to use? Thanks.
On Wed, Oct 24, 2018 at 9:03 PM Dmitry Torokhov <dtor@google.com> wrote: > On Wed, Oct 24, 2018 at 3:02 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Mon, Oct 22, 2018 at 04:34:55PM -0700, Rajat Jain wrote: > > > On Fri, Oct 19, 2018 at 2:13 AM Andy Shevchenko > > > <andy.shevchenko@gmail.com> wrote: > > > > On Fri, Oct 19, 2018 at 12:53 AM Rajat Jain <rajatja@google.com> wrote: > > > > > > > across other users of this API (other MMC host controller drivers). > > > > > > > > > if (slot->cd_idx >= 0) { > > > > > - ret = mmc_gpiod_request_cd(host->mmc, NULL, slot->cd_idx, > > > > > + ret = mmc_gpiod_request_cd(host->mmc, "cd", slot->cd_idx, > > > > > slot->cd_override_level, 0, NULL); > > > > > > > > Yes. > > > > > > > > > + if (ret && ret != -EPROBE_DEFER) > > > > > + ret = mmc_gpiod_request_cd(host->mmc, NULL, > > > > > + slot->cd_idx, > > > > > + slot->cd_override_level, > > > > > + 0, NULL); > > > > > > > > And no. Instead of this part you need to provide an ACPI GPIO mapping table. > > > > > > Sure, I am willing to do so, and I tried earlier too. However, certain > > > doubts arose in my mind when I tried that and I posted my questions > > > earlier (https://lkml.org/lkml/2018/9/28/507) but couldn't elicit any > > > response. Unfortunately I still do not have answers. My primary > > > questions are: > > > > > > 1) - It seems that 1 SDHCI device may support multiple slots (looking > > > at the code). It is not clear to me if they could share card detect > > > interrupts, or should have separate ones? > > > > This is more likely question to HW engineers of your platform with a caveat > > that there should be a way to distinguish exact slot in which card is being > > inserted. > > > > > Also, the driver may not > > > really know? > > > > I think in such case the bug in HW design and / or driver. > > Why? You can have a shared or dedicated interrupt and the driver does > not really need to know if it can poll the status. Yes, that's my point either we get 1:1 mapping between slot and GPIOs or have a possibility to read back from some register(s) the actual status of all of them, otherwise it's a bad design. Sorry if I wasn't clear about it. > > > So should I add 1 or two pins using the > > > devm_acpi_dev_add_driver_gpios(). > > > > This depends on the above, e.g. HW design, ACPI tables. > > Yes, it depends on the HW design and that is exactly why the approach > with devm_acpi_dev_add_driver_gpios() does not work well here: this is > a generic driver used on many platforms and you are trying to put the > platform knowledge into the driver. Here we are lucky I guess as I do > not believe anyone is using more than one slot, so we can have a tavle > with a single entry, but actually doing the fallback the way Rajat was > proposing is more correct. Or you have a table with N entries, where N > is hopefully sufficiently large. Yes, unfortunately this is the case. We need to keep somewhere the list to support old firmwares (see hci_bcm.c as an example how BIOS can screw things up). Soonish we start _DSD in BIOSes in a correct way (ha-ha), better for everyone. > > > Is some one familiar with SDHC > > > driver can answer these questions, it shall be great. > > > > Actually above questions better to ask in linux-mmc mailing list, which by the > > fact is in Cc list already. So, wait for someone to clarify. > > > > > > > 2) I'm not really sure what should I set "active_low" to? Isn't this > > > something that should be specified by platform / ACPI too, and driver > > > should just be able to say say choose whatever the ACPI says? > > > > > > struct acpi_gpio_params { > > > unsigned int crs_entry_index; > > > unsigned int line_index; > > > bool active_low; > > > }; > > > > > > ACPI specification misses this property, that's why we have it in the > > structure. In your case it should be provided by _DSD and thus be consistent > > with the hardcoded values. > > Again, you think as if the driver was platform specific; it is not. I > have 1000s of systems with different ACPI tables. Let's say half of > them use one polarity, and half another. Which polarity do you propose > to use? Use one table for one half and another for the rest.
On Mon, Oct 29, 2018 at 8:23 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Wed, Oct 24, 2018 at 9:03 PM Dmitry Torokhov <dtor@google.com> wrote: > > On Wed, Oct 24, 2018 at 3:02 AM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Mon, Oct 22, 2018 at 04:34:55PM -0700, Rajat Jain wrote: > > > > On Fri, Oct 19, 2018 at 2:13 AM Andy Shevchenko > > > > <andy.shevchenko@gmail.com> wrote: > > > > > On Fri, Oct 19, 2018 at 12:53 AM Rajat Jain <rajatja@google.com> wrote: > > > > > > > > > across other users of this API (other MMC host controller drivers). > > > > > > > > > > > if (slot->cd_idx >= 0) { > > > > > > - ret = mmc_gpiod_request_cd(host->mmc, NULL, slot->cd_idx, > > > > > > + ret = mmc_gpiod_request_cd(host->mmc, "cd", slot->cd_idx, > > > > > > slot->cd_override_level, 0, NULL); > > > > > > > > > > Yes. > > > > > > > > > > > + if (ret && ret != -EPROBE_DEFER) > > > > > > + ret = mmc_gpiod_request_cd(host->mmc, NULL, > > > > > > + slot->cd_idx, > > > > > > + slot->cd_override_level, > > > > > > + 0, NULL); > > > > > > > > > > And no. Instead of this part you need to provide an ACPI GPIO mapping table. > > > > > > > > Sure, I am willing to do so, and I tried earlier too. However, certain > > > > doubts arose in my mind when I tried that and I posted my questions > > > > earlier (https://lkml.org/lkml/2018/9/28/507) but couldn't elicit any > > > > response. Unfortunately I still do not have answers. My primary > > > > questions are: > > > > > > > > 1) - It seems that 1 SDHCI device may support multiple slots (looking > > > > at the code). It is not clear to me if they could share card detect > > > > interrupts, or should have separate ones? > > > > > > This is more likely question to HW engineers of your platform with a caveat > > > that there should be a way to distinguish exact slot in which card is being > > > inserted. > > > > > > > Also, the driver may not > > > > really know? > > > > > > I think in such case the bug in HW design and / or driver. > > > > Why? You can have a shared or dedicated interrupt and the driver does > > not really need to know if it can poll the status. > > Yes, that's my point either we get 1:1 mapping between slot and GPIOs > or have a possibility to read back from some register(s) the actual > status of all of them, otherwise it's a bad design. No, AFAIU, the driver only should only be able to read the status of *the* interrupt that was fired? (as opposite to the ability to read *all of them* when an interrupt fires). > Sorry if I wasn't > clear about it. > > > > > So should I add 1 or two pins using the > > > > devm_acpi_dev_add_driver_gpios(). > > > > > > This depends on the above, e.g. HW design, ACPI tables. > > > > Yes, it depends on the HW design and that is exactly why the approach > > with devm_acpi_dev_add_driver_gpios() does not work well here: this is > > a generic driver used on many platforms and you are trying to put the > > platform knowledge into the driver. Here we are lucky I guess as I do > > not believe anyone is using more than one slot, so we can have a tavle > > with a single entry, but actually doing the fallback the way Rajat was > > proposing is more correct. Or you have a table with N entries, where N > > is hopefully sufficiently large. > > Yes, unfortunately this is the case. We need to keep somewhere the > list to support old firmwares (see hci_bcm.c as an example how BIOS > can screw things up). > Soonish we start _DSD in BIOSes in a correct way (ha-ha), better for everyone. > > > > > Is some one familiar with SDHC > > > > driver can answer these questions, it shall be great. > > > > > > Actually above questions better to ask in linux-mmc mailing list, which by the > > > fact is in Cc list already. So, wait for someone to clarify. > > > > > > > > > > 2) I'm not really sure what should I set "active_low" to? Isn't this > > > > something that should be specified by platform / ACPI too, and driver > > > > should just be able to say say choose whatever the ACPI says? > > > > > > > > struct acpi_gpio_params { > > > > unsigned int crs_entry_index; > > > > unsigned int line_index; > > > > bool active_low; > > > > }; > > > > > > > > > ACPI specification misses this property, that's why we have it in the > > > structure. In your case it should be provided by _DSD and thus be consistent > > > with the hardcoded values. > > > > Again, you think as if the driver was platform specific; it is not. I > > have 1000s of systems with different ACPI tables. Let's say half of > > them use one polarity, and half another. Which polarity do you propose > > to use? > > Use one table for one half and another for the rest. But how does driver determine which table to use for which platform? (Currently the driver is platform independent). Thanks, Rajat > > -- > With Best Regards, > Andy Shevchenko
On Mon, Oct 29, 2018 at 10:22:02AM -0700, Rajat Jain wrote: > On Mon, Oct 29, 2018 at 8:23 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Wed, Oct 24, 2018 at 9:03 PM Dmitry Torokhov <dtor@google.com> wrote: > > > On Wed, Oct 24, 2018 at 3:02 AM Andy Shevchenko > > > <andy.shevchenko@gmail.com> wrote: > > > > On Mon, Oct 22, 2018 at 04:34:55PM -0700, Rajat Jain wrote: > > > > > On Fri, Oct 19, 2018 at 2:13 AM Andy Shevchenko > > > > > <andy.shevchenko@gmail.com> wrote: > > > > > Also, the driver may not > > > > > really know? > > > > > > > > I think in such case the bug in HW design and / or driver. > > > > > > Why? You can have a shared or dedicated interrupt and the driver does > > > not really need to know if it can poll the status. > > > > Yes, that's my point either we get 1:1 mapping between slot and GPIOs > > or have a possibility to read back from some register(s) the actual > > status of all of them, otherwise it's a bad design. > > No, AFAIU, the driver only should only be able to read the status of > *the* interrupt that was fired? (as opposite to the ability to read > *all of them* when an interrupt fires). I can't be sure in the details of this (sdhci) driver, I'm not a maintainer of that one. So, my above conclusions are purely generic. > > > > > 2) I'm not really sure what should I set "active_low" to? Isn't this > > > > > something that should be specified by platform / ACPI too, and driver > > > > > should just be able to say say choose whatever the ACPI says? > > > > > > > > > > struct acpi_gpio_params { > > > > > unsigned int crs_entry_index; > > > > > unsigned int line_index; > > > > > bool active_low; > > > > > }; > > > > ACPI specification misses this property, that's why we have it in the > > > > structure. In your case it should be provided by _DSD and thus be consistent > > > > with the hardcoded values. > > > > > > Again, you think as if the driver was platform specific; it is not. I > > > have 1000s of systems with different ACPI tables. Let's say half of > > > them use one polarity, and half another. Which polarity do you propose > > > to use? > > > > Use one table for one half and another for the rest. > > But how does driver determine which table to use for which platform? > (Currently the driver is platform independent). Based on vendor and device IDs in any form of it.
On Mon, Oct 29, 2018 at 10:44 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Oct 29, 2018 at 10:22:02AM -0700, Rajat Jain wrote: > > On Mon, Oct 29, 2018 at 8:23 AM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Wed, Oct 24, 2018 at 9:03 PM Dmitry Torokhov <dtor@google.com> wrote: > > > > On Wed, Oct 24, 2018 at 3:02 AM Andy Shevchenko > > > > <andy.shevchenko@gmail.com> wrote: > > > > > On Mon, Oct 22, 2018 at 04:34:55PM -0700, Rajat Jain wrote: > > > > > > On Fri, Oct 19, 2018 at 2:13 AM Andy Shevchenko > > > > > > <andy.shevchenko@gmail.com> wrote: > > > > > > > Also, the driver may not > > > > > > really know? > > > > > > > > > > I think in such case the bug in HW design and / or driver. > > > > > > > > Why? You can have a shared or dedicated interrupt and the driver does > > > > not really need to know if it can poll the status. > > > > > > Yes, that's my point either we get 1:1 mapping between slot and GPIOs > > > or have a possibility to read back from some register(s) the actual > > > status of all of them, otherwise it's a bad design. > > > > No, AFAIU, the driver only should only be able to read the status of > > *the* interrupt that was fired? (as opposite to the ability to read > > *all of them* when an interrupt fires). > > I can't be sure in the details of this (sdhci) driver, I'm not a maintainer of > that one. So, my above conclusions are purely generic. > > > > > > > 2) I'm not really sure what should I set "active_low" to? Isn't this > > > > > > something that should be specified by platform / ACPI too, and driver > > > > > > should just be able to say say choose whatever the ACPI says? > > > > > > > > > > > > struct acpi_gpio_params { > > > > > > unsigned int crs_entry_index; > > > > > > unsigned int line_index; > > > > > > bool active_low; > > > > > > }; > > > > > > > ACPI specification misses this property, that's why we have it in the > > > > > structure. In your case it should be provided by _DSD and thus be consistent > > > > > with the hardcoded values. > > > > > > > > Again, you think as if the driver was platform specific; it is not. I > > > > have 1000s of systems with different ACPI tables. Let's say half of > > > > them use one polarity, and half another. Which polarity do you propose > > > > to use? > > > > > > Use one table for one half and another for the rest. > > > > But how does driver determine which table to use for which platform? > > (Currently the driver is platform independent). > > Based on vendor and device IDs in any form of it. The vendor ID and device ID here would mean building a table of platforms ids in this (currently platform independent) driver. I'm not sure if my original patch introduces any problems that are worth solving using such a table. I would thus prefer to solve it using my original patch, I would respin it taking care of your other review comments. Thanks and best regards, Rajat > > -- > With Best Regards, > Andy Shevchenko > >
diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c index 7bfd366d970d..e53333c695b3 100644 --- a/drivers/mmc/host/sdhci-pci-core.c +++ b/drivers/mmc/host/sdhci-pci-core.c @@ -1762,8 +1762,13 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot( device_init_wakeup(&pdev->dev, true); if (slot->cd_idx >= 0) { - ret = mmc_gpiod_request_cd(host->mmc, NULL, slot->cd_idx, + ret = mmc_gpiod_request_cd(host->mmc, "cd", slot->cd_idx, slot->cd_override_level, 0, NULL); + if (ret && ret != -EPROBE_DEFER) + ret = mmc_gpiod_request_cd(host->mmc, NULL, + slot->cd_idx, + slot->cd_override_level, + 0, NULL); if (ret == -EPROBE_DEFER) goto remove;
Problem: The card detect IRQ does not work with modern BIOS (that want to use DSD to provide the card detect GPIO to the driver). Details: (Discussion: https://lkml.org/lkml/2018/9/25/1113) The mmc core provides the mmc_gpiod_request_cd() API to let host drivers request the gpio descriptor for the "card detect" (or carrier detect?) pin. This pin is specified in the ACPI for the SDHC device: * Either as a resource using _CRS. This is a method used by legacy BIOS. (The driver needs to tell which resource index). * Or as a named property ("cd-gpio") in DSD (which may internally point to an entry in _CRS). This way, the driver can lookup using a string. This is what modern BIOS prefer to use. This API finally results in a call to the following code: struct gpio_desc *acpi_find_gpio(..., const char *con_id,...) { ... /* Lookup gpio (using "<con_id>-gpio") in the _DSD */ ... if (!acpi_can_fallback_to_crs(adev, con_id)) return ERR_PTR(-ENOENT); ... /* Falling back to _CRS is allowed, Lookup gpio in the _CRS */ ... } Note that this means that if the ACPI has _DSD properties, the kernel will never use _CRS for the lookup (Because acpi_can_fallback_to_crs() will always be false for any device hat has _DSD entries). The SDHCI driver is thus currently broken on a modern BIOS (even if BIOS provides both _CRS and DSD entries, either of which could be used for a successful lookup). Ironically, none of these will be used for the lookup currently because: * Since the con_id is NULL, acpi_find_gpio() does not find a matching entry in DSDT. (The DSDT entry has the property name = "cd-gpio") * Because ACPI contains DSDT entries, thus acpi_can_fallback_to_crs() returns false (because device properties have been populated from DSD), thus the _CRS is never used for the lookup. Fix: Try "cd" for lookup in the _DSD before falling back to using NULL so as to try looking up in the _CRS. I've tested this patch successfully with both Legacy BIOS (that provide only _CRS method) as well as modern BIOS (that provide both _CRS and DSD). Also the use of "cd" also appears to be farly consistent across other users of this API (other MMC host controller drivers). Signed-off-by: Rajat Jain <rajatja@google.com> --- drivers/mmc/host/sdhci-pci-core.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)