Hi,
With commit f10e4bf6632b5b ("gpio: acpi: Even more tighten up ACPI
GPIO lookups"), the gpio lookups were tightened up such that _CRS
method will *only* be tried for lookup, if the ACPI node for the
device does not contain any _DSD properties, AND con_id=NULL:
bool acpi_can_fallback_to_crs(struct acpi_device *adev, const char *con_id)
{
/* Never allow fallback if the device has properties */
if (adev->data.properties || adev->driver_gpios)
return false;
return con_id == NULL;
}
From the commit log of the said commit, the following types of cases
were identified:
Case 1: (I think this represents the modern BIOS, because this is
what we want everyone to use i.e. we want to be able to lookup using
_DSD preferably:)
Device (DEVX)
{
...
Name (_CRS, ResourceTemplate ()
{
GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
"\\_SB.GPO0", 0, ResourceConsumer) {15}
})
Name (_DSD, Package ()
{
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package ()
{
Package () {"some-gpios", Package() {^DEVX, 0, 0, 0 }},
}
})
}
Case 2: (I think this represents the Legacy BIOS, because this is
what has been in use historically, i.e. need to lookup via the _CRS):
Device (DEVX)
{
...
Name (_CRS, ResourceTemplate ()
{
GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
"\\_SB.GPO0", 0, ResourceConsumer) {27}
})
}
Ideally a driver should be able to support both legacy and modern BIOS
(i.e. both the above cases). But it seems to me that as per the
current code, the driver has to be aware of what kind of BIOS it is,
because it needs to deal with them differently:
* Use con_id=NULL if it is dealing with a legacy BIOS (i.e. no _DSD
properties in the ACPI).
* Use con_id=<actual string> if it is dealing with a modern BIOS (i.e.
which provides _DSD for the <string> property)
Questions:
1) Is the above a correct understanding of what the gpiolib expects?
So it seems to be that _CRS is not really a "fallback option" anymore
at the gpiolib end anymore (i.e. if a driver wants to support both the
cases (legacy and modern), then it would need to do the fallback
explicitly?)
2) If so, it seems that the the sdhci driver is broken for modern BIOS
as it uses con_id = NULL when calling mmc_gpiod_request_cd(). We can
possibly fix this by either relaxing.the rules to allow _CRS fallback
in gpiolib, or change the sdhci driver to try first using "cd" and
then NULL as the con_id. What do you think?
This is what I see on my system (running 4.19-rc3'ish):
sdhci-pci 0000:00:14.5: SDHCI controller found [8086:34f8] (rev 0)
sdhci-pci 0000:00:14.5: GPIO lookup for consumer (null)
sdhci-pci 0000:00:14.5: using ACPI for GPIO lookup
acpi device:03: GPIO: looking up gpios
acpi device:03: GPIO: looking up gpio
sdhci-pci 0000:00:14.5: using lookup tables for GPIO lookup
sdhci-pci 0000:00:14.5: No GPIO consumer (null) found
sdhci-pci 0000:00:14.5: failed to setup card detect gpio
This is how my ACPI looks for the device:
Scope (\_SB.PCI0.SDXC)
{
Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
GpioInt (Edge, ActiveBoth, SharedAndWake, PullNone, 0x2710,
"\\_SB.PCI0.GPIO", 0x00, ResourceConsumer, ,
)
{ // Pin list
0x0005
}
})
Name (_DSD, Package (0x02) // _DSD: Device-Specific Data
{
ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device
Properties for _DSD */,
Package (0x01)
{
Package (0x02)
{
"cd-gpio",
Package (0x04)
{
\_SB.PCI0.SDXC,
Zero,
Zero,
One
}
}
}
})
}
Thanks,
Rajat
Hi,
On Tue, Sep 25, 2018 at 01:54:57PM -0700, Rajat Jain wrote:
> * Use con_id=NULL if it is dealing with a legacy BIOS (i.e. no _DSD
> properties in the ACPI).
> * Use con_id=<actual string> if it is dealing with a modern BIOS (i.e.
> which provides _DSD for the <string> property)
Or you can use con_id=<actual string> everywhere and supply
acpi_dev_add_driver_gpios() where needed to cover cases where BIOS does
not provide _DSD. See also Documentation/acpi/gpio-properties.txt for
more information.
In case of SDHCI I think the correct way is to stick using _CRS lookup
only because there typically is just one GpioInt() and I have not seen a
single BIOS yet where they implement _DSD for this besides yours. If
there is not way to change the BIOS implementation then I guess we just
need to amend the driver to call acpi_dev_add_driver_gpios().
On Wed, Sep 26, 2018 at 10:49 AM Mika Westerberg
<[email protected]> wrote:
>
> Hi,
>
> On Tue, Sep 25, 2018 at 01:54:57PM -0700, Rajat Jain wrote:
> > * Use con_id=NULL if it is dealing with a legacy BIOS (i.e. no _DSD
> > properties in the ACPI).
> > * Use con_id=<actual string> if it is dealing with a modern BIOS (i.e.
> > which provides _DSD for the <string> property)
>
> Or you can use con_id=<actual string> everywhere and supply
> acpi_dev_add_driver_gpios() where needed to cover cases where BIOS does
> not provide _DSD. See also Documentation/acpi/gpio-properties.txt for
> more information.
Thanks, Mika. That is exactly the way how I suggested to fix and
actually fixed a lot of drivers already.
Run `git grep -n -w devm_acpi_dev_add_driver_gpios` to find examples.
> In case of SDHCI I think the correct way is to stick using _CRS lookup
> only because there typically is just one GpioInt() and I have not seen a
> single BIOS yet where they implement _DSD for this besides yours. If
> there is not way to change the BIOS implementation then I guess we just
> need to amend the driver to call acpi_dev_add_driver_gpios().
True.
--
With Best Regards,
Andy Shevchenko
Hi,
Thanks Mika and Andy for your inputs.
On Wed, Sep 26, 2018 at 1:42 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Wed, Sep 26, 2018 at 10:49 AM Mika Westerberg
> <[email protected]> wrote:
> >
> > Hi,
> >
> > On Tue, Sep 25, 2018 at 01:54:57PM -0700, Rajat Jain wrote:
> > > * Use con_id=NULL if it is dealing with a legacy BIOS (i.e. no _DSD
> > > properties in the ACPI).
> > > * Use con_id=<actual string> if it is dealing with a modern BIOS (i.e.
> > > which provides _DSD for the <string> property)
> >
> > Or you can use con_id=<actual string> everywhere and supply
> > acpi_dev_add_driver_gpios() where needed to cover cases where BIOS does
> > not provide _DSD.
This sounds like a good idea and I'd like to do this. I have some
questions though:
1) If the BIOS does provide a _DSD entry for "cd-gpio", and
additionally driver also uses devm_acpi_dev_add_driver_gpios() to add
one more entry for the same string "cd-gpio", which one will (should?)
actually be returned by the gpiolib? The one in BIOS or the one that
was added by the driver?
2) Related, I'm trying to understand how can a driver use
devm_acpi_dev_add_driver_gpios(), for *only* the case where the BIOS
does not have a _DSD (Or should it really care)? Does the driver need
to check for _DSD using some other ACPI call?
> See also Documentation/acpi/gpio-properties.txt for
> > more information.
>
> Thanks, Mika. That is exactly the way how I suggested to fix and
> actually fixed a lot of drivers already.
>
> Run `git grep -n -w devm_acpi_dev_add_driver_gpios` to find examples.
>
> > In case of SDHCI I think the correct way is to stick using _CRS lookup
> > only because there typically is just one GpioInt() and I have not seen a
> > single BIOS yet where they implement _DSD for this besides yours. If
> > there is not way to change the BIOS implementation then I guess we just
> > need to amend the driver to call acpi_dev_add_driver_gpios().
Since we shouldn't discourage a BIOS that is trying to do the right
thing by exposing the details in _DST, I think it would be preferable
if we can solve this in the kernel.
Thanks,
Rajat
>
> True.
>
> --
> With Best Regards,
> Andy Shevchenko
On Wed, Sep 26, 2018 at 10:26 PM Rajat Jain <[email protected]> wrote:
> On Wed, Sep 26, 2018 at 1:42 AM Andy Shevchenko
> <[email protected]> wrote:
> > On Wed, Sep 26, 2018 at 10:49 AM Mika Westerberg
> > <[email protected]> wrote:
> > > Or you can use con_id=<actual string> everywhere and supply
> > > acpi_dev_add_driver_gpios() where needed to cover cases where BIOS does
> > > not provide _DSD.
>
> This sounds like a good idea and I'd like to do this. I have some
> questions though:
>
> 1) If the BIOS does provide a _DSD entry for "cd-gpio", and
> additionally driver also uses devm_acpi_dev_add_driver_gpios() to add
> one more entry for the same string "cd-gpio", which one will (should?)
> actually be returned by the gpiolib? The one in BIOS or the one that
> was added by the driver?
Of course the one that BIOS provides. This hardcoded mapping tables is
a fallback for *old* BIOSes which do not have _DSD.
> 2) Related, I'm trying to understand how can a driver use
> devm_acpi_dev_add_driver_gpios(), for *only* the case where the BIOS
> does not have a _DSD (Or should it really care)? Does the driver need
> to check for _DSD using some other ACPI call?
The magic happens internally in ACPI core.
Whenever one calls gpiod_get() with a name on ACPI-enabled platform,
_DSD would be checked first.
> > See also Documentation/acpi/gpio-properties.txt for
> > > more information.
> > > In case of SDHCI I think the correct way is to stick using _CRS lookup
> > > only because there typically is just one GpioInt() and I have not seen a
> > > single BIOS yet where they implement _DSD for this besides yours. If
> > > there is not way to change the BIOS implementation then I guess we just
> > > need to amend the driver to call acpi_dev_add_driver_gpios().
>
> Since we shouldn't discourage a BIOS that is trying to do the right
> thing by exposing the details in _DST, I think it would be preferable
> if we can solve this in the kernel.
Patches are welcome, I think.
Btw, is there any existing hardware on the market with such BIOS?
--
With Best Regards,
Andy Shevchenko
On Thu, Sep 27, 2018 at 12:26 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Wed, Sep 26, 2018 at 10:26 PM Rajat Jain <[email protected]> wrote:
> > On Wed, Sep 26, 2018 at 1:42 AM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Wed, Sep 26, 2018 at 10:49 AM Mika Westerberg
> > > <[email protected]> wrote:
>
> > > > Or you can use con_id=<actual string> everywhere and supply
> > > > acpi_dev_add_driver_gpios() where needed to cover cases where BIOS does
> > > > not provide _DSD.
> >
> > This sounds like a good idea and I'd like to do this. I have some
> > questions though:
> >
> > 1) If the BIOS does provide a _DSD entry for "cd-gpio", and
> > additionally driver also uses devm_acpi_dev_add_driver_gpios() to add
> > one more entry for the same string "cd-gpio", which one will (should?)
> > actually be returned by the gpiolib? The one in BIOS or the one that
> > was added by the driver?
>
> Of course the one that BIOS provides. This hardcoded mapping tables is
> a fallback for *old* BIOSes which do not have _DSD.
>
> > 2) Related, I'm trying to understand how can a driver use
> > devm_acpi_dev_add_driver_gpios(), for *only* the case where the BIOS
> > does not have a _DSD (Or should it really care)? Does the driver need
> > to check for _DSD using some other ACPI call?
>
> The magic happens internally in ACPI core.
> Whenever one calls gpiod_get() with a name on ACPI-enabled platform,
> _DSD would be checked first.
Got it, thanks.
>
> > > See also Documentation/acpi/gpio-properties.txt for
> > > > more information.
>
> > > > In case of SDHCI I think the correct way is to stick using _CRS lookup
> > > > only because there typically is just one GpioInt() and I have not seen a
> > > > single BIOS yet where they implement _DSD for this besides yours. If
> > > > there is not way to change the BIOS implementation then I guess we just
> > > > need to amend the driver to call acpi_dev_add_driver_gpios().
> >
> > Since we shouldn't discourage a BIOS that is trying to do the right
> > thing by exposing the details in _DST, I think it would be preferable
> > if we can solve this in the kernel.
>
> Patches are welcome, I think.
>
> Btw, is there any existing hardware on the market with such BIOS?
Yes, all the chrome books available in the market (at least the ones
released in last 3 years) have same ACPI layout (provide _DSD for
card-detect). They are all working fine today because they use an
older kernel, but if we update them to the latest kernel, this part
will be broken.
Thanks,
Rajat Jain
>
> --
> With Best Regards,
> Andy Shevchenko
On Thu, Sep 27, 2018 at 7:57 PM Rajat Jain <[email protected]> wrote:
> > Patches are welcome, I think.
> >
> > Btw, is there any existing hardware on the market with such BIOS?
>
> Yes, all the chrome books available in the market (at least the ones
> released in last 3 years) have same ACPI layout (provide _DSD for
> card-detect). They are all working fine today because they use an
> older kernel, but if we update them to the latest kernel, this part
> will be broken.
This is not looking good at all.
Andy should we revert the patch or do you have some other
quick fix in mind we could do? It seems reverting the patch
could be bad for the mctrl patches IIUC, but this regression
seems even more serious.
Yours,
Linus Walleij
On Fri, Sep 28, 2018 at 1:42 AM Linus Walleij <[email protected]> wrote:
>
> On Thu, Sep 27, 2018 at 7:57 PM Rajat Jain <[email protected]> wrote:
>
> > > Patches are welcome, I think.
> > >
> > > Btw, is there any existing hardware on the market with such BIOS?
> >
> > Yes, all the chrome books available in the market (at least the ones
> > released in last 3 years) have same ACPI layout (provide _DSD for
> > card-detect). They are all working fine today because they use an
> > older kernel, but if we update them to the latest kernel, this part
> > will be broken.
>
> This is not looking good at all.
>
> Andy should we revert the patch or do you have some other
> quick fix in mind we could do? It seems reverting the patch
> could be bad for the mctrl patches IIUC, but this regression
> seems even more serious.
Sorry, I may have made it sound like more serious than it is. Yes,
what I said is true, but we (Google) does not plan to update those
devices to the latest kernel. That regression will only be seen by any
developers who try to do it on their own.
Also, the commit I mentioned above was put in long time back, and I
think it is more reasonable to put a fix in the sdhci driver instead.
I tried using Andy's suggestion yesterday, but was faced with more
questions that I did not have answers to (with my limited
understanding):
- It seems that 1 SDHCI device may support multiple slots. It was 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().
- I was unsure what should I set the active_low to.
Given these concerns, it seemed the easiest fix to me, if we can call
mmc_gpiod_request_cd() twice, once with "cd" and then with NULL as a
fallback.
Thanks,
Rajat Jain
>
> Yours,
> Linus Walleij
On Fri, Sep 28, 2018 at 2:34 PM Rajat Jain <[email protected]> wrote:
> > This is not looking good at all.
> >
> > Andy should we revert the patch or do you have some other
> > quick fix in mind we could do? It seems reverting the patch
> > could be bad for the mctrl patches IIUC, but this regression
> > seems even more serious.
>
> Sorry, I may have made it sound like more serious than it is. Yes,
> what I said is true, but we (Google) does not plan to update those
> devices to the latest kernel. That regression will only be seen by any
> developers who try to do it on their own.
Given the number of core kernel developers using chromebooks
and relying on them to be working as expected, these are the
users I am most worried about, really.
If only 3 users in the world can't read photos from their SD-cards
that may sound like a small problem, but if those 3 users are
core kernel developers reporting bugs, I as maintainer have a
big problem. So not all users are equal, or how should I put it.
> Also, the commit I mentioned above was put in long time back, and I
> think it is more reasonable to put a fix in the sdhci driver instead.
OK cool.
> - It seems that 1 SDHCI device may support multiple slots. It was 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().
>
> - I was unsure what should I set the active_low to.
>
> Given these concerns, it seemed the easiest fix to me, if we can call
> mmc_gpiod_request_cd() twice, once with "cd" and then with NULL as a
> fallback.
That sounds like a resonable fix to me.
Yours,
Linus Walleij
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 <[email protected]>
---
drivers/mmc/host/sdhci-pci-core.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
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;
--
2.19.1.568.g152ad8e336-goog
On Fri, Oct 19, 2018 at 12:53 AM Rajat Jain <[email protected]> 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;
--
With Best Regards,
Andy Shevchenko
Hi Andy,
Thanks for your review.
On Fri, Oct 19, 2018 at 2:13 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Fri, Oct 19, 2018 at 12:53 AM Rajat Jain <[email protected]> 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
> <[email protected]> wrote:
> > On Fri, Oct 19, 2018 at 12:53 AM Rajat Jain <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Wed, Oct 24, 2018 at 3:02 AM Andy Shevchenko
<[email protected]> 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
> > <[email protected]> wrote:
> > > On Fri, Oct 19, 2018 at 12:53 AM Rajat Jain <[email protected]> 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.
--
Dmitry
On Wed, Oct 24, 2018 at 9:03 PM Dmitry Torokhov <[email protected]> wrote:
> On Wed, Oct 24, 2018 at 3:02 AM Andy Shevchenko
> <[email protected]> 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
> > > <[email protected]> wrote:
> > > > On Fri, Oct 19, 2018 at 12:53 AM Rajat Jain <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
On Mon, Oct 29, 2018 at 8:23 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Wed, Oct 24, 2018 at 9:03 PM Dmitry Torokhov <[email protected]> wrote:
> > On Wed, Oct 24, 2018 at 3:02 AM Andy Shevchenko
> > <[email protected]> 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
> > > > <[email protected]> wrote:
> > > > > On Fri, Oct 19, 2018 at 12:53 AM Rajat Jain <[email protected]> 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
> <[email protected]> wrote:
> > On Wed, Oct 24, 2018 at 9:03 PM Dmitry Torokhov <[email protected]> wrote:
> > > On Wed, Oct 24, 2018 at 3:02 AM Andy Shevchenko
> > > <[email protected]> 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
> > > > > <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
On Mon, Oct 29, 2018 at 10:44 AM Andy Shevchenko
<[email protected]> 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
> > <[email protected]> wrote:
> > > On Wed, Oct 24, 2018 at 9:03 PM Dmitry Torokhov <[email protected]> wrote:
> > > > On Wed, Oct 24, 2018 at 3:02 AM Andy Shevchenko
> > > > <[email protected]> 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
> > > > > > <[email protected]> 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
>
>
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:
The mmc core provides the mmc_gpiod_request_cd() API to let host drivers
request the gpio descriptor for the "card 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-gpios"/"cd-gpio") in _DSD (which internally
points 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 (for index based lookup) and _DSD entries (for
string based 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-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.
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" appears to be fairly consistent
across other users of this API (other MMC host controller drivers).
Link: https://lkml.org/lkml/2018/9/25/1113
Signed-off-by: Rajat Jain <[email protected]>
---
v2: Fix the commit log to take care of Andy's comments.
drivers/mmc/host/sdhci-pci-core.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
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;
--
2.19.1.568.g152ad8e336-goog
On 30/10/18 12:17 AM, Rajat Jain 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:
>
> The mmc core provides the mmc_gpiod_request_cd() API to let host drivers
> request the gpio descriptor for the "card 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-gpios"/"cd-gpio") in _DSD (which internally
> points 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 (for index based lookup) and _DSD entries (for
> string based 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-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.
>
> 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" appears to be fairly consistent
> across other users of this API (other MMC host controller drivers).
>
> Link: https://lkml.org/lkml/2018/9/25/1113
> Signed-off-by: Rajat Jain <[email protected]>
This is good enough for a fix. An ACPI GPIO mappings table can be done later.
Acked-by: Adrian Hunter <[email protected]>
> ---
> v2: Fix the commit log to take care of Andy's comments.
>
> drivers/mmc/host/sdhci-pci-core.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> 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;
>
>
On 29 October 2018 at 23:17, Rajat Jain <[email protected]> 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:
>
> The mmc core provides the mmc_gpiod_request_cd() API to let host drivers
> request the gpio descriptor for the "card 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-gpios"/"cd-gpio") in _DSD (which internally
> points 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 (for index based lookup) and _DSD entries (for
> string based 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-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.
>
> 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" appears to be fairly consistent
> across other users of this API (other MMC host controller drivers).
>
> Link: https://lkml.org/lkml/2018/9/25/1113
> Signed-off-by: Rajat Jain <[email protected]>
Applied for fixes, thanks!
Should I add a stable tag to this as well?
Kind regards
Uffe
> ---
> v2: Fix the commit log to take care of Andy's comments.
>
> drivers/mmc/host/sdhci-pci-core.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> 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;
>
> --
> 2.19.1.568.g152ad8e336-goog
>
On Mon, Nov 12, 2018 at 1:05 PM Ulf Hansson <[email protected]> wrote:
>
> On 29 October 2018 at 23:17, Rajat Jain <[email protected]> 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:
> >
> > The mmc core provides the mmc_gpiod_request_cd() API to let host drivers
> > request the gpio descriptor for the "card 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-gpios"/"cd-gpio") in _DSD (which internally
> > points 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 (for index based lookup) and _DSD entries (for
> > string based 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-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.
> >
> > 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" appears to be fairly consistent
> > across other users of this API (other MMC host controller drivers).
> >
> > Link: https://lkml.org/lkml/2018/9/25/1113
> > Signed-off-by: Rajat Jain <[email protected]>
>
> Applied for fixes, thanks!
>
> Should I add a stable tag to this as well?
If you go with that it might make sense to have Fixes tag against
commit f10e4bf6632b5be11cea875b66ba959833a69258
Author: Andy Shevchenko <[email protected]>
Date: Tue May 23 20:03:19 2017 +0300
gpio: acpi: Even more tighten up ACPI GPIO lookups
--
With Best Regards,
Andy Shevchenko
On Mon, Nov 12, 2018 at 3:26 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Mon, Nov 12, 2018 at 1:05 PM Ulf Hansson <[email protected]> wrote:
> >
> > On 29 October 2018 at 23:17, Rajat Jain <[email protected]> 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:
> > >
> > > The mmc core provides the mmc_gpiod_request_cd() API to let host drivers
> > > request the gpio descriptor for the "card 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-gpios"/"cd-gpio") in _DSD (which internally
> > > points 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 (for index based lookup) and _DSD entries (for
> > > string based 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-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.
> > >
> > > 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" appears to be fairly consistent
> > > across other users of this API (other MMC host controller drivers).
> > >
> > > Link: https://lkml.org/lkml/2018/9/25/1113
> > > Signed-off-by: Rajat Jain <[email protected]>
> >
> > Applied for fixes, thanks!
> >
> > Should I add a stable tag to this as well?
>
> If you go with that it might make sense to have Fixes tag against
>
> commit f10e4bf6632b5be11cea875b66ba959833a69258
> Author: Andy Shevchenko <[email protected]>
> Date: Tue May 23 20:03:19 2017 +0300
>
> gpio: acpi: Even more tighten up ACPI GPIO lookups
>
I agree.
Thanks,
Rajat
> --
> With Best Regards,
> Andy Shevchenko