| Message ID | 20200108093903.57620-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: sdhci-acpi: Introduce device specific quirks, fix issues on 2 device models | expand |
On 8/01/20 11:39 am, Hans de Goede wrote: > Based on a sample of 7 DSDTs from Cherry Trail devices using an AXP288 > PMIC depending on the design one of 2 possible LDOs on the PMIC is used > for the MMC signalling voltage, either DLDO3 or GPIO1LDO (GPIO1 pin in > low noise LDO mode). > > The Lenovo Miix 320-10ICR uses GPIO1LDO in the SHC1 ACPI device's DSM > methods to set 3.3 or 1.8 signalling voltage and this appears to work > as advertised, so presumably the device is actually using GPIO1LDO for > the external microSD signalling voltage. > > But this device has a bug in the _PS0 method of the SHC1 ACPI device, > the DSM remembers the last set signalling voltage and the _PS0 restores > this after a (runtime) suspend-resume cycle, but it "restores" the voltage > on DLDO3 instead of setting it on GPIO1LDO as the DSM method does. DLDO3 > is used for the LCD and setting it to 1.8V causes the LCD to go black. > > This issue can be worked around by setting the SDHCI_QUIRK2_NO_1_8_V > quirk on the sdhci_host so that the DSM never gets used to program the > signalling voltage to 1.8V. Could you instead call the 3.3V DSM at runtime suspend time, then the _PS0 would not "restore" the 1.8V value? That should allow you to use 1.8V UHS-I speed modes with SD cards that support them. > > So far we have mostly been able to avoid using device specific quirks in > the sdhci-acpi code, but given that this issue is specific to this one > model and we certainly do not want to disable 1.8V modes everywhere I > see no other option. > > This commit adds a new mechanism for setting sdhci-acpi specific quirks > and a matching sdhci-acpi.quirks module parameter to make testing quirks / > similar issues on other devices easier. > > The first quirk supported by this mechanism is SDHCI_ACPI_QUIRK_SD_NO_1_8V, > which when set causes any slots with the SDHCI_ACPI_SD_CD flag to get the > SDHCI_QUIRK2_NO_1_8_V quirk set on their sdhci_host. > > This commit also adds a DMI table for specifying default quirks for some > models and adds an entry for the Lenovo Miix 320-10ICR which enables the > SDHCI_QUIRK2_NO_1_8_V by default on this model, fixing the LCD going black > when the external microSD slot is used. > > BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=111294 > BugLink: https://gitlab.freedesktop.org/drm/intel/issues/355 > Reported-by: russianneuromancer <russianneuromancer@ya.ru> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/mmc/host/sdhci-acpi.c | 39 +++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c > index 105e73d4a3b9..9f150c73e958 100644 > --- a/drivers/mmc/host/sdhci-acpi.c > +++ b/drivers/mmc/host/sdhci-acpi.c > @@ -23,6 +23,7 @@ > #include <linux/pm.h> > #include <linux/pm_runtime.h> > #include <linux/delay.h> > +#include <linux/dmi.h> > > #include <linux/mmc/host.h> > #include <linux/mmc/pm.h> > @@ -75,6 +76,14 @@ struct sdhci_acpi_host { > unsigned long private[0] ____cacheline_aligned; > }; > > +enum { > + SDHCI_ACPI_QUIRK_SD_NO_1_8V = BIT(0), > +}; > + > +static int quirks = -1; > +module_param(quirks, int, 0444); > +MODULE_PARM_DESC(quirks, "Override sdhci-acpi specific quirks"); Why is a module parameter needed? > + > static inline void *sdhci_acpi_priv(struct sdhci_acpi_host *c) > { > return (void *)c->private; > @@ -647,6 +656,24 @@ static const struct acpi_device_id sdhci_acpi_ids[] = { > }; > MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids); > > +static const struct dmi_system_id sdhci_acpi_quirks[] = { > + { > + /* > + * The Lenovo Miix 320-10ICR has a bug in the _PS0 method of > + * the SHC1 ACPI device, this bug causes it to reprogram the > + * wrong LDO (DLDO3) to 1.8V if 1.8V modes are used and the > + * card is (runtime) suspended + resumed. DLDO3 is used for > + * the LCD and setting it to 1.8V causes the LCD to go black. > + */ > + .matches = { > + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), > + DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo MIIX 320-10ICR"), > + }, > + .driver_data = (void *)SDHCI_ACPI_QUIRK_SD_NO_1_8V, > + }, > + {} /* Terminating entry */ > +}; > + > static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(struct acpi_device *adev) > { > const struct sdhci_acpi_uid_slot *u; > @@ -663,6 +690,7 @@ static int sdhci_acpi_probe(struct platform_device *pdev) > struct device *dev = &pdev->dev; > const struct sdhci_acpi_slot *slot; > struct acpi_device *device, *child; > + const struct dmi_system_id *id; > struct sdhci_acpi_host *c; > struct sdhci_host *host; > struct resource *iomem; > @@ -670,6 +698,14 @@ static int sdhci_acpi_probe(struct platform_device *pdev) > size_t priv_size; > int err; > > + if (quirks == -1) { > + id = dmi_first_match(sdhci_acpi_quirks); > + if (id) > + quirks = (long)id->driver_data; > + else > + quirks = 0; > + } > + > device = ACPI_COMPANION(dev); > if (!device) > return -ENODEV; > @@ -759,6 +795,9 @@ static int sdhci_acpi_probe(struct platform_device *pdev) > dev_warn(dev, "failed to setup card detect gpio\n"); > c->use_runtime_pm = false; > } > + > + if (quirks & SDHCI_ACPI_QUIRK_SD_NO_1_8V) > + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; > } > > err = sdhci_setup_host(host); >
Hi, On 15-01-2020 13:57, Adrian Hunter wrote: > On 8/01/20 11:39 am, Hans de Goede wrote: >> Based on a sample of 7 DSDTs from Cherry Trail devices using an AXP288 >> PMIC depending on the design one of 2 possible LDOs on the PMIC is used >> for the MMC signalling voltage, either DLDO3 or GPIO1LDO (GPIO1 pin in >> low noise LDO mode). >> >> The Lenovo Miix 320-10ICR uses GPIO1LDO in the SHC1 ACPI device's DSM >> methods to set 3.3 or 1.8 signalling voltage and this appears to work >> as advertised, so presumably the device is actually using GPIO1LDO for >> the external microSD signalling voltage. >> >> But this device has a bug in the _PS0 method of the SHC1 ACPI device, >> the DSM remembers the last set signalling voltage and the _PS0 restores >> this after a (runtime) suspend-resume cycle, but it "restores" the voltage >> on DLDO3 instead of setting it on GPIO1LDO as the DSM method does. DLDO3 >> is used for the LCD and setting it to 1.8V causes the LCD to go black. >> >> This issue can be worked around by setting the SDHCI_QUIRK2_NO_1_8_V >> quirk on the sdhci_host so that the DSM never gets used to program the >> signalling voltage to 1.8V. > > Could you instead call the 3.3V DSM at runtime suspend time, then the _PS0 > would not "restore" the 1.8V value? That should allow you to use 1.8V UHS-I > speed modes with SD cards that support them. I have considered doing this, but this means reprogramming the signal voltage to 3.3V at a time when the card does not expect this, is this ok? We would then also need to recall the DSM to put the voltage back to 1.8V from resume. I have a feeling that this is probably what Windows does (I guess it moves the entire card back to a more safe IOS mode before suspend), accidentally avoiding the bug. I assume you want to only call the DSM to set the voltage to 3.3V on the affected model, or do you want to do this on all machines ? Adding this does seem to introduce more complexity then simply disabling 1.8V modes and given that it is just a single model which is affected I went with the more simple option of just disabling the 1.8V modes. Ideally we would not need any quirks, but if we do we should at least make the work-around as simple as possible. So I've a slight preference for just sticking with DHCI_QUIRK2_NO_1_8_V ... Note that the suspend/resume handling is broken also in the sense that it does not disable the signal voltage during suspend. >> So far we have mostly been able to avoid using device specific quirks in >> the sdhci-acpi code, but given that this issue is specific to this one >> model and we certainly do not want to disable 1.8V modes everywhere I >> see no other option. >> >> This commit adds a new mechanism for setting sdhci-acpi specific quirks >> and a matching sdhci-acpi.quirks module parameter to make testing quirks / >> similar issues on other devices easier. >> >> The first quirk supported by this mechanism is SDHCI_ACPI_QUIRK_SD_NO_1_8V, >> which when set causes any slots with the SDHCI_ACPI_SD_CD flag to get the >> SDHCI_QUIRK2_NO_1_8_V quirk set on their sdhci_host. >> >> This commit also adds a DMI table for specifying default quirks for some >> models and adds an entry for the Lenovo Miix 320-10ICR which enables the >> SDHCI_QUIRK2_NO_1_8_V by default on this model, fixing the LCD going black >> when the external microSD slot is used. >> >> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=111294 >> BugLink: https://gitlab.freedesktop.org/drm/intel/issues/355 >> Reported-by: russianneuromancer <russianneuromancer@ya.ru> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/mmc/host/sdhci-acpi.c | 39 +++++++++++++++++++++++++++++++++++ >> 1 file changed, 39 insertions(+) >> >> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c >> index 105e73d4a3b9..9f150c73e958 100644 >> --- a/drivers/mmc/host/sdhci-acpi.c >> +++ b/drivers/mmc/host/sdhci-acpi.c >> @@ -23,6 +23,7 @@ >> #include <linux/pm.h> >> #include <linux/pm_runtime.h> >> #include <linux/delay.h> >> +#include <linux/dmi.h> >> >> #include <linux/mmc/host.h> >> #include <linux/mmc/pm.h> >> @@ -75,6 +76,14 @@ struct sdhci_acpi_host { >> unsigned long private[0] ____cacheline_aligned; >> }; >> >> +enum { >> + SDHCI_ACPI_QUIRK_SD_NO_1_8V = BIT(0), >> +}; >> + >> +static int quirks = -1; >> +module_param(quirks, int, 0444); >> +MODULE_PARM_DESC(quirks, "Override sdhci-acpi specific quirks"); > > Why is a module parameter needed? The module parameter is purely to make testing if the same quirk(s) help on other devices easier. Like the debug_quirks[2] params in sdhci.c Regards, Hans >> + >> static inline void *sdhci_acpi_priv(struct sdhci_acpi_host *c) >> { >> return (void *)c->private; >> @@ -647,6 +656,24 @@ static const struct acpi_device_id sdhci_acpi_ids[] = { >> }; >> MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids); >> >> +static const struct dmi_system_id sdhci_acpi_quirks[] = { >> + { >> + /* >> + * The Lenovo Miix 320-10ICR has a bug in the _PS0 method of >> + * the SHC1 ACPI device, this bug causes it to reprogram the >> + * wrong LDO (DLDO3) to 1.8V if 1.8V modes are used and the >> + * card is (runtime) suspended + resumed. DLDO3 is used for >> + * the LCD and setting it to 1.8V causes the LCD to go black. >> + */ >> + .matches = { >> + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), >> + DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo MIIX 320-10ICR"), >> + }, >> + .driver_data = (void *)SDHCI_ACPI_QUIRK_SD_NO_1_8V, >> + }, >> + {} /* Terminating entry */ >> +}; >> + >> static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(struct acpi_device *adev) >> { >> const struct sdhci_acpi_uid_slot *u; >> @@ -663,6 +690,7 @@ static int sdhci_acpi_probe(struct platform_device *pdev) >> struct device *dev = &pdev->dev; >> const struct sdhci_acpi_slot *slot; >> struct acpi_device *device, *child; >> + const struct dmi_system_id *id; >> struct sdhci_acpi_host *c; >> struct sdhci_host *host; >> struct resource *iomem; >> @@ -670,6 +698,14 @@ static int sdhci_acpi_probe(struct platform_device *pdev) >> size_t priv_size; >> int err; >> >> + if (quirks == -1) { >> + id = dmi_first_match(sdhci_acpi_quirks); >> + if (id) >> + quirks = (long)id->driver_data; >> + else >> + quirks = 0; >> + } >> + >> device = ACPI_COMPANION(dev); >> if (!device) >> return -ENODEV; >> @@ -759,6 +795,9 @@ static int sdhci_acpi_probe(struct platform_device *pdev) >> dev_warn(dev, "failed to setup card detect gpio\n"); >> c->use_runtime_pm = false; >> } >> + >> + if (quirks & SDHCI_ACPI_QUIRK_SD_NO_1_8V) >> + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; >> } >> >> err = sdhci_setup_host(host); >> >
On 15/01/20 3:31 pm, Hans de Goede wrote: > Hi, > > On 15-01-2020 13:57, Adrian Hunter wrote: >> On 8/01/20 11:39 am, Hans de Goede wrote: >>> Based on a sample of 7 DSDTs from Cherry Trail devices using an AXP288 >>> PMIC depending on the design one of 2 possible LDOs on the PMIC is used >>> for the MMC signalling voltage, either DLDO3 or GPIO1LDO (GPIO1 pin in >>> low noise LDO mode). >>> >>> The Lenovo Miix 320-10ICR uses GPIO1LDO in the SHC1 ACPI device's DSM >>> methods to set 3.3 or 1.8 signalling voltage and this appears to work >>> as advertised, so presumably the device is actually using GPIO1LDO for >>> the external microSD signalling voltage. >>> >>> But this device has a bug in the _PS0 method of the SHC1 ACPI device, >>> the DSM remembers the last set signalling voltage and the _PS0 restores >>> this after a (runtime) suspend-resume cycle, but it "restores" the voltage >>> on DLDO3 instead of setting it on GPIO1LDO as the DSM method does. DLDO3 >>> is used for the LCD and setting it to 1.8V causes the LCD to go black. >>> >>> This issue can be worked around by setting the SDHCI_QUIRK2_NO_1_8_V >>> quirk on the sdhci_host so that the DSM never gets used to program the >>> signalling voltage to 1.8V. >> >> Could you instead call the 3.3V DSM at runtime suspend time, then the _PS0 >> would not "restore" the 1.8V value? That should allow you to use 1.8V UHS-I >> speed modes with SD cards that support them. > > I have considered doing this, but this means reprogramming the signal > voltage to 3.3V at a time when the card does not expect this, is this ok? The host controller does not runtime suspend unless the card is runtime suspended, so the bus power should be off. > > We would then also need to recall the DSM to put the voltage back to 1.8V > from resume. No, the card will be reinitialized at 3.3V and switch back to 1.8V. > I have a feeling that this is probably what Windows does > (I guess it moves the entire card back to a more safe IOS mode before > suspend), accidentally avoiding the bug. > > I assume you want to only call the DSM to set the voltage to 3.3V on > the affected model, or do you want to do this on all machines ? I would stick with the specific machine for now. > > Adding this does seem to introduce more complexity then simply disabling > 1.8V modes and given that it is just a single model which is affected > I went with the more simple option of just disabling the 1.8V modes. > > Ideally we would not need any quirks, but if we do we should at least > make the work-around as simple as possible. So I've a slight preference > for just sticking with DHCI_QUIRK2_NO_1_8_V ... > > Note that the suspend/resume handling is broken also in the sense that > it does not disable the signal voltage during suspend. The bus power gets switched off if the card is runtime suspended. The host controller should go to D3cold which means everything off. > >>> So far we have mostly been able to avoid using device specific quirks in >>> the sdhci-acpi code, but given that this issue is specific to this one >>> model and we certainly do not want to disable 1.8V modes everywhere I >>> see no other option. >>> >>> This commit adds a new mechanism for setting sdhci-acpi specific quirks >>> and a matching sdhci-acpi.quirks module parameter to make testing quirks / >>> similar issues on other devices easier. >>> >>> The first quirk supported by this mechanism is SDHCI_ACPI_QUIRK_SD_NO_1_8V, >>> which when set causes any slots with the SDHCI_ACPI_SD_CD flag to get the >>> SDHCI_QUIRK2_NO_1_8_V quirk set on their sdhci_host. >>> >>> This commit also adds a DMI table for specifying default quirks for some >>> models and adds an entry for the Lenovo Miix 320-10ICR which enables the >>> SDHCI_QUIRK2_NO_1_8_V by default on this model, fixing the LCD going black >>> when the external microSD slot is used. >>> >>> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=111294 >>> BugLink: https://gitlab.freedesktop.org/drm/intel/issues/355 >>> Reported-by: russianneuromancer <russianneuromancer@ya.ru> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> drivers/mmc/host/sdhci-acpi.c | 39 +++++++++++++++++++++++++++++++++++ >>> 1 file changed, 39 insertions(+) >>> >>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c >>> index 105e73d4a3b9..9f150c73e958 100644 >>> --- a/drivers/mmc/host/sdhci-acpi.c >>> +++ b/drivers/mmc/host/sdhci-acpi.c >>> @@ -23,6 +23,7 @@ >>> #include <linux/pm.h> >>> #include <linux/pm_runtime.h> >>> #include <linux/delay.h> >>> +#include <linux/dmi.h> >>> #include <linux/mmc/host.h> >>> #include <linux/mmc/pm.h> >>> @@ -75,6 +76,14 @@ struct sdhci_acpi_host { >>> unsigned long private[0] ____cacheline_aligned; >>> }; >>> +enum { >>> + SDHCI_ACPI_QUIRK_SD_NO_1_8V = BIT(0), >>> +}; >>> + >>> +static int quirks = -1; >>> +module_param(quirks, int, 0444); >>> +MODULE_PARM_DESC(quirks, "Override sdhci-acpi specific quirks"); >> >> Why is a module parameter needed? > > The module parameter is purely to make testing if the same quirk(s) > help on other devices easier. Like the debug_quirks[2] params in sdhci.c Mmm, but we already have SDHCI_QUIRK2_NO_1_8_V > > Regards, > > Hans > > > >>> + >>> static inline void *sdhci_acpi_priv(struct sdhci_acpi_host *c) >>> { >>> return (void *)c->private; >>> @@ -647,6 +656,24 @@ static const struct acpi_device_id sdhci_acpi_ids[] = { >>> }; >>> MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids); >>> +static const struct dmi_system_id sdhci_acpi_quirks[] = { >>> + { >>> + /* >>> + * The Lenovo Miix 320-10ICR has a bug in the _PS0 method of >>> + * the SHC1 ACPI device, this bug causes it to reprogram the >>> + * wrong LDO (DLDO3) to 1.8V if 1.8V modes are used and the >>> + * card is (runtime) suspended + resumed. DLDO3 is used for >>> + * the LCD and setting it to 1.8V causes the LCD to go black. >>> + */ >>> + .matches = { >>> + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), >>> + DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo MIIX 320-10ICR"), >>> + }, >>> + .driver_data = (void *)SDHCI_ACPI_QUIRK_SD_NO_1_8V, >>> + }, >>> + {} /* Terminating entry */ >>> +}; >>> + >>> static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(struct >>> acpi_device *adev) >>> { >>> const struct sdhci_acpi_uid_slot *u; >>> @@ -663,6 +690,7 @@ static int sdhci_acpi_probe(struct platform_device >>> *pdev) >>> struct device *dev = &pdev->dev; >>> const struct sdhci_acpi_slot *slot; >>> struct acpi_device *device, *child; >>> + const struct dmi_system_id *id; >>> struct sdhci_acpi_host *c; >>> struct sdhci_host *host; >>> struct resource *iomem; >>> @@ -670,6 +698,14 @@ static int sdhci_acpi_probe(struct platform_device >>> *pdev) >>> size_t priv_size; >>> int err; >>> + if (quirks == -1) { >>> + id = dmi_first_match(sdhci_acpi_quirks); >>> + if (id) >>> + quirks = (long)id->driver_data; >>> + else >>> + quirks = 0; >>> + } >>> + >>> device = ACPI_COMPANION(dev); >>> if (!device) >>> return -ENODEV; >>> @@ -759,6 +795,9 @@ static int sdhci_acpi_probe(struct platform_device >>> *pdev) >>> dev_warn(dev, "failed to setup card detect gpio\n"); >>> c->use_runtime_pm = false; >>> } >>> + >>> + if (quirks & SDHCI_ACPI_QUIRK_SD_NO_1_8V) >>> + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; >>> } >>> err = sdhci_setup_host(host); >>> >> >
Hi, On 15-01-2020 14:48, Adrian Hunter wrote: > On 15/01/20 3:31 pm, Hans de Goede wrote: >> Hi, >> >> On 15-01-2020 13:57, Adrian Hunter wrote: >>> On 8/01/20 11:39 am, Hans de Goede wrote: >>>> Based on a sample of 7 DSDTs from Cherry Trail devices using an AXP288 >>>> PMIC depending on the design one of 2 possible LDOs on the PMIC is used >>>> for the MMC signalling voltage, either DLDO3 or GPIO1LDO (GPIO1 pin in >>>> low noise LDO mode). >>>> >>>> The Lenovo Miix 320-10ICR uses GPIO1LDO in the SHC1 ACPI device's DSM >>>> methods to set 3.3 or 1.8 signalling voltage and this appears to work >>>> as advertised, so presumably the device is actually using GPIO1LDO for >>>> the external microSD signalling voltage. >>>> >>>> But this device has a bug in the _PS0 method of the SHC1 ACPI device, >>>> the DSM remembers the last set signalling voltage and the _PS0 restores >>>> this after a (runtime) suspend-resume cycle, but it "restores" the voltage >>>> on DLDO3 instead of setting it on GPIO1LDO as the DSM method does. DLDO3 >>>> is used for the LCD and setting it to 1.8V causes the LCD to go black. >>>> >>>> This issue can be worked around by setting the SDHCI_QUIRK2_NO_1_8_V >>>> quirk on the sdhci_host so that the DSM never gets used to program the >>>> signalling voltage to 1.8V. >>> >>> Could you instead call the 3.3V DSM at runtime suspend time, then the _PS0 >>> would not "restore" the 1.8V value? That should allow you to use 1.8V UHS-I >>> speed modes with SD cards that support them. >> >> I have considered doing this, but this means reprogramming the signal >> voltage to 3.3V at a time when the card does not expect this, is this ok? > > The host controller does not runtime suspend unless the card is runtime > suspended, so the bus power should be off. > >> >> We would then also need to recall the DSM to put the voltage back to 1.8V >> from resume. > > No, the card will be reinitialized at 3.3V and switch back to 1.8V. > >> I have a feeling that this is probably what Windows does >> (I guess it moves the entire card back to a more safe IOS mode before >> suspend), accidentally avoiding the bug. >> >> I assume you want to only call the DSM to set the voltage to 3.3V on >> the affected model, or do you want to do this on all machines ? > > I would stick with the specific machine for now. Ok I will give this a try. Hopefully I can make some time for this the coming days. >> Adding this does seem to introduce more complexity then simply disabling >> 1.8V modes and given that it is just a single model which is affected >> I went with the more simple option of just disabling the 1.8V modes. >> >> Ideally we would not need any quirks, but if we do we should at least >> make the work-around as simple as possible. So I've a slight preference >> for just sticking with DHCI_QUIRK2_NO_1_8_V ... >> >> Note that the suspend/resume handling is broken also in the sense that >> it does not disable the signal voltage during suspend. > > The bus power gets switched off if the card is runtime suspended. The host > controller should go to D3cold which means everything off. Right, what I mean is that the _PS3 method is broken in that it does not turn off the voltage-regulator providing the signal voltage, as it does do on other machines with a non buggy implementation. >>>> So far we have mostly been able to avoid using device specific quirks in >>>> the sdhci-acpi code, but given that this issue is specific to this one >>>> model and we certainly do not want to disable 1.8V modes everywhere I >>>> see no other option. >>>> >>>> This commit adds a new mechanism for setting sdhci-acpi specific quirks >>>> and a matching sdhci-acpi.quirks module parameter to make testing quirks / >>>> similar issues on other devices easier. >>>> >>>> The first quirk supported by this mechanism is SDHCI_ACPI_QUIRK_SD_NO_1_8V, >>>> which when set causes any slots with the SDHCI_ACPI_SD_CD flag to get the >>>> SDHCI_QUIRK2_NO_1_8_V quirk set on their sdhci_host. >>>> >>>> This commit also adds a DMI table for specifying default quirks for some >>>> models and adds an entry for the Lenovo Miix 320-10ICR which enables the >>>> SDHCI_QUIRK2_NO_1_8_V by default on this model, fixing the LCD going black >>>> when the external microSD slot is used. >>>> >>>> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=111294 >>>> BugLink: https://gitlab.freedesktop.org/drm/intel/issues/355 >>>> Reported-by: russianneuromancer <russianneuromancer@ya.ru> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> drivers/mmc/host/sdhci-acpi.c | 39 +++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 39 insertions(+) >>>> >>>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c >>>> index 105e73d4a3b9..9f150c73e958 100644 >>>> --- a/drivers/mmc/host/sdhci-acpi.c >>>> +++ b/drivers/mmc/host/sdhci-acpi.c >>>> @@ -23,6 +23,7 @@ >>>> #include <linux/pm.h> >>>> #include <linux/pm_runtime.h> >>>> #include <linux/delay.h> >>>> +#include <linux/dmi.h> >>>> #include <linux/mmc/host.h> >>>> #include <linux/mmc/pm.h> >>>> @@ -75,6 +76,14 @@ struct sdhci_acpi_host { >>>> unsigned long private[0] ____cacheline_aligned; >>>> }; >>>> +enum { >>>> + SDHCI_ACPI_QUIRK_SD_NO_1_8V = BIT(0), >>>> +}; >>>> + >>>> +static int quirks = -1; >>>> +module_param(quirks, int, 0444); >>>> +MODULE_PARM_DESC(quirks, "Override sdhci-acpi specific quirks"); >>> >>> Why is a module parameter needed? >> >> The module parameter is purely to make testing if the same quirk(s) >> help on other devices easier. Like the debug_quirks[2] params in sdhci.c > > Mmm, but we already have SDHCI_QUIRK2_NO_1_8_V True, but this only applies to the sdcard slot and not to the eMMC, also you are asking for this to be changed to: SDHCI_ACPI_QUIRK_SD_SET_SIGNAL_3_3V_ON_SUSPEND Which is not duplicate. Anyways if you dislike the module parameter bits I can drop them and make this only available through the DMI quirks. Regards, Hans > >> >> Regards, >> >> Hans >> >> >> >>>> + >>>> static inline void *sdhci_acpi_priv(struct sdhci_acpi_host *c) >>>> { >>>> return (void *)c->private; >>>> @@ -647,6 +656,24 @@ static const struct acpi_device_id sdhci_acpi_ids[] = { >>>> }; >>>> MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids); >>>> +static const struct dmi_system_id sdhci_acpi_quirks[] = { >>>> + { >>>> + /* >>>> + * The Lenovo Miix 320-10ICR has a bug in the _PS0 method of >>>> + * the SHC1 ACPI device, this bug causes it to reprogram the >>>> + * wrong LDO (DLDO3) to 1.8V if 1.8V modes are used and the >>>> + * card is (runtime) suspended + resumed. DLDO3 is used for >>>> + * the LCD and setting it to 1.8V causes the LCD to go black. >>>> + */ >>>> + .matches = { >>>> + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), >>>> + DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo MIIX 320-10ICR"), >>>> + }, >>>> + .driver_data = (void *)SDHCI_ACPI_QUIRK_SD_NO_1_8V, >>>> + }, >>>> + {} /* Terminating entry */ >>>> +}; >>>> + >>>> static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(struct >>>> acpi_device *adev) >>>> { >>>> const struct sdhci_acpi_uid_slot *u; >>>> @@ -663,6 +690,7 @@ static int sdhci_acpi_probe(struct platform_device >>>> *pdev) >>>> struct device *dev = &pdev->dev; >>>> const struct sdhci_acpi_slot *slot; >>>> struct acpi_device *device, *child; >>>> + const struct dmi_system_id *id; >>>> struct sdhci_acpi_host *c; >>>> struct sdhci_host *host; >>>> struct resource *iomem; >>>> @@ -670,6 +698,14 @@ static int sdhci_acpi_probe(struct platform_device >>>> *pdev) >>>> size_t priv_size; >>>> int err; >>>> + if (quirks == -1) { >>>> + id = dmi_first_match(sdhci_acpi_quirks); >>>> + if (id) >>>> + quirks = (long)id->driver_data; >>>> + else >>>> + quirks = 0; >>>> + } >>>> + >>>> device = ACPI_COMPANION(dev); >>>> if (!device) >>>> return -ENODEV; >>>> @@ -759,6 +795,9 @@ static int sdhci_acpi_probe(struct platform_device >>>> *pdev) >>>> dev_warn(dev, "failed to setup card detect gpio\n"); >>>> c->use_runtime_pm = false; >>>> } >>>> + >>>> + if (quirks & SDHCI_ACPI_QUIRK_SD_NO_1_8V) >>>> + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; >>>> } >>>> err = sdhci_setup_host(host); >>>> >>> >> >
On 15/01/20 5:31 pm, Hans de Goede wrote: > Hi, > > On 15-01-2020 14:48, Adrian Hunter wrote: >> On 15/01/20 3:31 pm, Hans de Goede wrote: >>> Hi, >>> >>> On 15-01-2020 13:57, Adrian Hunter wrote: >>>> On 8/01/20 11:39 am, Hans de Goede wrote: >>>>> Based on a sample of 7 DSDTs from Cherry Trail devices using an AXP288 >>>>> PMIC depending on the design one of 2 possible LDOs on the PMIC is used >>>>> for the MMC signalling voltage, either DLDO3 or GPIO1LDO (GPIO1 pin in >>>>> low noise LDO mode). >>>>> >>>>> The Lenovo Miix 320-10ICR uses GPIO1LDO in the SHC1 ACPI device's DSM >>>>> methods to set 3.3 or 1.8 signalling voltage and this appears to work >>>>> as advertised, so presumably the device is actually using GPIO1LDO for >>>>> the external microSD signalling voltage. >>>>> >>>>> But this device has a bug in the _PS0 method of the SHC1 ACPI device, >>>>> the DSM remembers the last set signalling voltage and the _PS0 restores >>>>> this after a (runtime) suspend-resume cycle, but it "restores" the voltage >>>>> on DLDO3 instead of setting it on GPIO1LDO as the DSM method does. DLDO3 >>>>> is used for the LCD and setting it to 1.8V causes the LCD to go black. >>>>> >>>>> This issue can be worked around by setting the SDHCI_QUIRK2_NO_1_8_V >>>>> quirk on the sdhci_host so that the DSM never gets used to program the >>>>> signalling voltage to 1.8V. >>>> >>>> Could you instead call the 3.3V DSM at runtime suspend time, then the _PS0 >>>> would not "restore" the 1.8V value? That should allow you to use 1.8V >>>> UHS-I >>>> speed modes with SD cards that support them. >>> >>> I have considered doing this, but this means reprogramming the signal >>> voltage to 3.3V at a time when the card does not expect this, is this ok? >> >> The host controller does not runtime suspend unless the card is runtime >> suspended, so the bus power should be off. >> >>> >>> We would then also need to recall the DSM to put the voltage back to 1.8V >>> from resume. >> >> No, the card will be reinitialized at 3.3V and switch back to 1.8V. >> >>> I have a feeling that this is probably what Windows does >>> (I guess it moves the entire card back to a more safe IOS mode before >>> suspend), accidentally avoiding the bug. >>> >>> I assume you want to only call the DSM to set the voltage to 3.3V on >>> the affected model, or do you want to do this on all machines ? >> >> I would stick with the specific machine for now. > > Ok I will give this a try. Hopefully I can make some time for this the > coming days. > >>> Adding this does seem to introduce more complexity then simply disabling >>> 1.8V modes and given that it is just a single model which is affected >>> I went with the more simple option of just disabling the 1.8V modes. >>> >>> Ideally we would not need any quirks, but if we do we should at least >>> make the work-around as simple as possible. So I've a slight preference >>> for just sticking with DHCI_QUIRK2_NO_1_8_V ... >>> >>> Note that the suspend/resume handling is broken also in the sense that >>> it does not disable the signal voltage during suspend. >> >> The bus power gets switched off if the card is runtime suspended. The host >> controller should go to D3cold which means everything off. > > Right, what I mean is that the _PS3 method is broken in that it does > not turn off the voltage-regulator providing the signal voltage, as > it does do on other machines with a non buggy implementation. Is that different to what you would get with Windows? Also, you could possibly build a custom DSDT and fix the _PS0 and _PS3 yourself. That requires building it into a custom kernel also though. > >>>>> So far we have mostly been able to avoid using device specific quirks in >>>>> the sdhci-acpi code, but given that this issue is specific to this one >>>>> model and we certainly do not want to disable 1.8V modes everywhere I >>>>> see no other option. >>>>> >>>>> This commit adds a new mechanism for setting sdhci-acpi specific quirks >>>>> and a matching sdhci-acpi.quirks module parameter to make testing quirks / >>>>> similar issues on other devices easier. >>>>> >>>>> The first quirk supported by this mechanism is >>>>> SDHCI_ACPI_QUIRK_SD_NO_1_8V, >>>>> which when set causes any slots with the SDHCI_ACPI_SD_CD flag to get the >>>>> SDHCI_QUIRK2_NO_1_8_V quirk set on their sdhci_host. >>>>> >>>>> This commit also adds a DMI table for specifying default quirks for some >>>>> models and adds an entry for the Lenovo Miix 320-10ICR which enables the >>>>> SDHCI_QUIRK2_NO_1_8_V by default on this model, fixing the LCD going black >>>>> when the external microSD slot is used. >>>>> >>>>> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=111294 >>>>> BugLink: https://gitlab.freedesktop.org/drm/intel/issues/355 >>>>> Reported-by: russianneuromancer <russianneuromancer@ya.ru> >>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>> --- >>>>> drivers/mmc/host/sdhci-acpi.c | 39 +++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 39 insertions(+) >>>>> >>>>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c >>>>> index 105e73d4a3b9..9f150c73e958 100644 >>>>> --- a/drivers/mmc/host/sdhci-acpi.c >>>>> +++ b/drivers/mmc/host/sdhci-acpi.c >>>>> @@ -23,6 +23,7 @@ >>>>> #include <linux/pm.h> >>>>> #include <linux/pm_runtime.h> >>>>> #include <linux/delay.h> >>>>> +#include <linux/dmi.h> >>>>> #include <linux/mmc/host.h> >>>>> #include <linux/mmc/pm.h> >>>>> @@ -75,6 +76,14 @@ struct sdhci_acpi_host { >>>>> unsigned long private[0] ____cacheline_aligned; >>>>> }; >>>>> +enum { >>>>> + SDHCI_ACPI_QUIRK_SD_NO_1_8V = BIT(0), >>>>> +}; >>>>> + >>>>> +static int quirks = -1; >>>>> +module_param(quirks, int, 0444); >>>>> +MODULE_PARM_DESC(quirks, "Override sdhci-acpi specific quirks"); >>>> >>>> Why is a module parameter needed? >>> >>> The module parameter is purely to make testing if the same quirk(s) >>> help on other devices easier. Like the debug_quirks[2] params in sdhci.c >> >> Mmm, but we already have SDHCI_QUIRK2_NO_1_8_V > > True, but this only applies to the sdcard slot and not to the eMMC, > also you are asking for this to be changed to: > > SDHCI_ACPI_QUIRK_SD_SET_SIGNAL_3_3V_ON_SUSPEND > > Which is not duplicate. Anyways if you dislike the module parameter > bits I can drop them and make this only available through the DMI quirks. > It isn't dislike, it is whether it will ever be needed. >>> >>>>> + >>>>> static inline void *sdhci_acpi_priv(struct sdhci_acpi_host *c) >>>>> { >>>>> return (void *)c->private; >>>>> @@ -647,6 +656,24 @@ static const struct acpi_device_id >>>>> sdhci_acpi_ids[] = { >>>>> }; >>>>> MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids); >>>>> +static const struct dmi_system_id sdhci_acpi_quirks[] = { >>>>> + { >>>>> + /* >>>>> + * The Lenovo Miix 320-10ICR has a bug in the _PS0 method of >>>>> + * the SHC1 ACPI device, this bug causes it to reprogram the >>>>> + * wrong LDO (DLDO3) to 1.8V if 1.8V modes are used and the >>>>> + * card is (runtime) suspended + resumed. DLDO3 is used for >>>>> + * the LCD and setting it to 1.8V causes the LCD to go black. >>>>> + */ >>>>> + .matches = { >>>>> + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), >>>>> + DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo MIIX 320-10ICR"), >>>>> + }, >>>>> + .driver_data = (void *)SDHCI_ACPI_QUIRK_SD_NO_1_8V, >>>>> + }, >>>>> + {} /* Terminating entry */ >>>>> +}; >>>>> + >>>>> static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(struct >>>>> acpi_device *adev) >>>>> { >>>>> const struct sdhci_acpi_uid_slot *u; >>>>> @@ -663,6 +690,7 @@ static int sdhci_acpi_probe(struct platform_device >>>>> *pdev) >>>>> struct device *dev = &pdev->dev; >>>>> const struct sdhci_acpi_slot *slot; >>>>> struct acpi_device *device, *child; >>>>> + const struct dmi_system_id *id; >>>>> struct sdhci_acpi_host *c; >>>>> struct sdhci_host *host; >>>>> struct resource *iomem; >>>>> @@ -670,6 +698,14 @@ static int sdhci_acpi_probe(struct platform_device >>>>> *pdev) >>>>> size_t priv_size; >>>>> int err; >>>>> + if (quirks == -1) { >>>>> + id = dmi_first_match(sdhci_acpi_quirks); >>>>> + if (id) >>>>> + quirks = (long)id->driver_data; >>>>> + else >>>>> + quirks = 0; >>>>> + } >>>>> + >>>>> device = ACPI_COMPANION(dev); >>>>> if (!device) >>>>> return -ENODEV; >>>>> @@ -759,6 +795,9 @@ static int sdhci_acpi_probe(struct platform_device >>>>> *pdev) >>>>> dev_warn(dev, "failed to setup card detect gpio\n"); >>>>> c->use_runtime_pm = false; >>>>> } >>>>> + >>>>> + if (quirks & SDHCI_ACPI_QUIRK_SD_NO_1_8V) >>>>> + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; >>>>> } >>>>> err = sdhci_setup_host(host); >>>>> >>>> >>> >> >
HI, On 16-01-2020 08:59, Adrian Hunter wrote: > On 15/01/20 5:31 pm, Hans de Goede wrote: <snip> >>>> Note that the suspend/resume handling is broken also in the sense that >>>> it does not disable the signal voltage during suspend. >>> >>> The bus power gets switched off if the card is runtime suspended. The host >>> controller should go to D3cold which means everything off. >> >> Right, what I mean is that the _PS3 method is broken in that it does >> not turn off the voltage-regulator providing the signal voltage, as >> it does do on other machines with a non buggy implementation. > > Is that different to what you would get with Windows? No Windows has the same problem. > Also, you could possibly build a custom DSDT and fix the _PS0 and _PS3 > yourself. That requires building it into a custom kernel also though. I have not tried, but yes that should work, but until we get some generic mechanism (*) in Linux / distro-s to provide DSDT overrides, that is not helpful for regular Linux users. *) which also has copyright issues, so the chances of this ever happening are slim <snip> >>>>>> +static int quirks = -1; >>>>>> +module_param(quirks, int, 0444); >>>>>> +MODULE_PARM_DESC(quirks, "Override sdhci-acpi specific quirks"); >>>>> >>>>> Why is a module parameter needed? >>>> >>>> The module parameter is purely to make testing if the same quirk(s) >>>> help on other devices easier. Like the debug_quirks[2] params in sdhci.c >>> >>> Mmm, but we already have SDHCI_QUIRK2_NO_1_8_V >> >> True, but this only applies to the sdcard slot and not to the eMMC, >> also you are asking for this to be changed to: >> >> SDHCI_ACPI_QUIRK_SD_SET_SIGNAL_3_3V_ON_SUSPEND >> >> Which is not duplicate. Anyways if you dislike the module parameter >> bits I can drop them and make this only available through the DMI quirks. >> > > It isn't dislike, it is whether it will ever be needed. For this specific issue, chances are not that big we will need it on another device. The quirk added by the second patch, to disable (broken) read-only detection OTOH might very well be useful on some other devices. And adding the option to override the quirks from the kernel commandline requires very little extra code. Anyways, it is your call. Please let me know if you want to drop the module parameter for v2, or if you are ok with keeping it. Regards, Hans
On 16/01/20 3:26 pm, Hans de Goede wrote: > HI, > > On 16-01-2020 08:59, Adrian Hunter wrote: >> On 15/01/20 5:31 pm, Hans de Goede wrote: > > <snip> > >>>>> Note that the suspend/resume handling is broken also in the sense that >>>>> it does not disable the signal voltage during suspend. >>>> >>>> The bus power gets switched off if the card is runtime suspended. The host >>>> controller should go to D3cold which means everything off. >>> >>> Right, what I mean is that the _PS3 method is broken in that it does >>> not turn off the voltage-regulator providing the signal voltage, as >>> it does do on other machines with a non buggy implementation. >> >> Is that different to what you would get with Windows? > > No Windows has the same problem. > >> Also, you could possibly build a custom DSDT and fix the _PS0 and _PS3 >> yourself. That requires building it into a custom kernel also though. > > I have not tried, but yes that should work, but until we get some generic > mechanism (*) in Linux / distro-s to provide DSDT overrides, that is not > helpful for regular Linux users. > > *) which also has copyright issues, so the chances of this ever happening > are slim > > <snip> > >>>>>>> +static int quirks = -1; >>>>>>> +module_param(quirks, int, 0444); >>>>>>> +MODULE_PARM_DESC(quirks, "Override sdhci-acpi specific quirks"); >>>>>> >>>>>> Why is a module parameter needed? >>>>> >>>>> The module parameter is purely to make testing if the same quirk(s) >>>>> help on other devices easier. Like the debug_quirks[2] params in sdhci.c >>>> >>>> Mmm, but we already have SDHCI_QUIRK2_NO_1_8_V >>> >>> True, but this only applies to the sdcard slot and not to the eMMC, >>> also you are asking for this to be changed to: >>> >>> SDHCI_ACPI_QUIRK_SD_SET_SIGNAL_3_3V_ON_SUSPEND >>> >>> Which is not duplicate. Anyways if you dislike the module parameter >>> bits I can drop them and make this only available through the DMI quirks. >>> >> >> It isn't dislike, it is whether it will ever be needed. > > For this specific issue, chances are not that big we will need it > on another device. The quirk added by the second patch, to disable > (broken) read-only detection OTOH might very well be useful on some > other devices. > > And adding the option to override the quirks from the kernel commandline > requires very little extra code. > > Anyways, it is your call. Please let me know if you want to drop the > module parameter for v2, or if you are ok with keeping it. I would be more interested in something like below (completely untested). Thoughts? From: Adrian Hunter <adrian.hunter@intel.com> Date: Fri, 17 Jan 2020 11:01:31 +0200 Subject: [PATCH] mmc: sdhci: Add module param debug_spec End users can have linux compatibility issues. SDHCI provides 2 module params to help debug such issues: debug_quirks and debug_quirks2. Those have 3 limitations: - they apply to all devices instead of a specific device - they overwrite all quirks instead of specific bits - they do not cover capabilities Add a new module parameter debug_spec which addresses all 3 limitations. Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> --- drivers/mmc/host/sdhci.c | 41 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 6e22a800bded..096cec1afe8e 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -46,6 +46,7 @@ static unsigned int debug_quirks = 0; static unsigned int debug_quirks2; +static char *debug_spec; static void sdhci_finish_data(struct sdhci_host *); @@ -3860,6 +3861,34 @@ void __sdhci_read_caps(struct sdhci_host *host, const u16 *ver, } EXPORT_SYMBOL_GPL(__sdhci_read_caps); +static void sdhci_update_debug_quirks_and_caps(struct sdhci_host *host) +{ + struct mmc_host *mmc = host->mmc; + unsigned int *vars[] = {&debug_quirks, &debug_quirks2, &host->caps, + &host->caps1, &mmc->caps, &mmc->caps2}; + unsigned int mask, val, old, new; + const char *p = debug_spec; + int ret, var, n; + char id[32]; + + while (1) { + ret = sscanf(p, "%31s %d %x %x %n", id, &var, &mask, &val, &n); + if (ret != 4) + break; + p += n; + if (var >= ARRAY_SIZE(vars) || + strcmp(id, dev_name(mmc_dev(mmc)))) + continue; + old = *vars[var]; + new = (old & ~mask) | val; + if (old != new) { + *vars[var] = new; + DBG("debug_spec: updating var %d %#x -> %#x\n", + var, old, new); + } + } +} + static void sdhci_allocate_bounce_buffer(struct sdhci_host *host) { struct mmc_host *mmc = host->mmc; @@ -3967,6 +3996,8 @@ int sdhci_setup_host(struct sdhci_host *host) sdhci_read_caps(host); + sdhci_update_debug_quirks_and_caps(host); + override_timeout_clk = host->timeout_clk; if (host->version > SDHCI_SPEC_420) { @@ -4513,6 +4544,8 @@ int __sdhci_add_host(struct sdhci_host *host) struct mmc_host *mmc = host->mmc; int ret; + sdhci_update_debug_quirks_and_caps(host); + host->complete_wq = alloc_workqueue("sdhci", flags, 0); if (!host->complete_wq) return -ENOMEM; @@ -4676,6 +4709,7 @@ module_exit(sdhci_drv_exit); module_param(debug_quirks, uint, 0444); module_param(debug_quirks2, uint, 0444); +module_param(debug_spec, charp, 0444); MODULE_AUTHOR("Pierre Ossman <pierre@ossman.eu>"); MODULE_DESCRIPTION("Secure Digital Host Controller Interface core driver"); @@ -4683,3 +4717,10 @@ MODULE_LICENSE("GPL"); MODULE_PARM_DESC(debug_quirks, "Force certain quirks."); MODULE_PARM_DESC(debug_quirks2, "Force certain other quirks."); +MODULE_PARM_DESC(debug_spec, + "Alter quirks, sdhci capabilities and MMC capabilities. The " + "string contains one or more: <device name> <variable> <mask> " + "<value> where <variable> is 0 (debug_quirks), " + "1 (debug_quirks2), 2 (sdhci->caps), 3 (sdhci->caps1), " + "4 (mmc->caps) or 5 (mmc->caps2), <mask> is a hex bit mask of " + "bits to change, and <value> is the hex value.");
Hi, On 1/17/20 10:16 AM, Adrian Hunter wrote: > On 16/01/20 3:26 pm, Hans de Goede wrote: >> HI, >> >> On 16-01-2020 08:59, Adrian Hunter wrote: >>> On 15/01/20 5:31 pm, Hans de Goede wrote: >> >> <snip> >> >>>>>> Note that the suspend/resume handling is broken also in the sense that >>>>>> it does not disable the signal voltage during suspend. >>>>> >>>>> The bus power gets switched off if the card is runtime suspended. The host >>>>> controller should go to D3cold which means everything off. >>>> >>>> Right, what I mean is that the _PS3 method is broken in that it does >>>> not turn off the voltage-regulator providing the signal voltage, as >>>> it does do on other machines with a non buggy implementation. >>> >>> Is that different to what you would get with Windows? >> >> No Windows has the same problem. >> >>> Also, you could possibly build a custom DSDT and fix the _PS0 and _PS3 >>> yourself. That requires building it into a custom kernel also though. >> >> I have not tried, but yes that should work, but until we get some generic >> mechanism (*) in Linux / distro-s to provide DSDT overrides, that is not >> helpful for regular Linux users. >> >> *) which also has copyright issues, so the chances of this ever happening >> are slim >> >> <snip> >> >>>>>>>> +static int quirks = -1; >>>>>>>> +module_param(quirks, int, 0444); >>>>>>>> +MODULE_PARM_DESC(quirks, "Override sdhci-acpi specific quirks"); >>>>>>> >>>>>>> Why is a module parameter needed? >>>>>> >>>>>> The module parameter is purely to make testing if the same quirk(s) >>>>>> help on other devices easier. Like the debug_quirks[2] params in sdhci.c >>>>> >>>>> Mmm, but we already have SDHCI_QUIRK2_NO_1_8_V >>>> >>>> True, but this only applies to the sdcard slot and not to the eMMC, >>>> also you are asking for this to be changed to: >>>> >>>> SDHCI_ACPI_QUIRK_SD_SET_SIGNAL_3_3V_ON_SUSPEND >>>> >>>> Which is not duplicate. Anyways if you dislike the module parameter >>>> bits I can drop them and make this only available through the DMI quirks. >>>> >>> >>> It isn't dislike, it is whether it will ever be needed. >> >> For this specific issue, chances are not that big we will need it >> on another device. The quirk added by the second patch, to disable >> (broken) read-only detection OTOH might very well be useful on some >> other devices. >> >> And adding the option to override the quirks from the kernel commandline >> requires very little extra code. >> >> Anyways, it is your call. Please let me know if you want to drop the >> module parameter for v2, or if you are ok with keeping it. > > I would be more interested in something like below (completely untested). > Thoughts? First of all, sorry that I've been quite slow to respond to this. I think your proposal is interesting, although I'm not sure it is immediately useful for the sdhci-acpi issues from my patch-series as those are sdhci-acpi / Intel DSM specific. I guess we could still use quirk flags at the shared sdhci level for them despite this, but I'm not sure if that is the right approach. For now for the v2 of this series which I'm preparing I've just removed the module option. If we feel the need for something like this later we can always add it later. Regards, Hans > > > From: Adrian Hunter <adrian.hunter@intel.com> > Date: Fri, 17 Jan 2020 11:01:31 +0200 > Subject: [PATCH] mmc: sdhci: Add module param debug_spec > > End users can have linux compatibility issues. SDHCI provides 2 module > params to help debug such issues: debug_quirks and debug_quirks2. Those > have 3 limitations: > - they apply to all devices instead of a specific device > - they overwrite all quirks instead of specific bits > - they do not cover capabilities > Add a new module parameter debug_spec which addresses all 3 limitations. > > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/mmc/host/sdhci.c | 41 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index 6e22a800bded..096cec1afe8e 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -46,6 +46,7 @@ > > static unsigned int debug_quirks = 0; > static unsigned int debug_quirks2; > +static char *debug_spec; > > static void sdhci_finish_data(struct sdhci_host *); > > @@ -3860,6 +3861,34 @@ void __sdhci_read_caps(struct sdhci_host *host, const u16 *ver, > } > EXPORT_SYMBOL_GPL(__sdhci_read_caps); > > +static void sdhci_update_debug_quirks_and_caps(struct sdhci_host *host) > +{ > + struct mmc_host *mmc = host->mmc; > + unsigned int *vars[] = {&debug_quirks, &debug_quirks2, &host->caps, > + &host->caps1, &mmc->caps, &mmc->caps2}; > + unsigned int mask, val, old, new; > + const char *p = debug_spec; > + int ret, var, n; > + char id[32]; > + > + while (1) { > + ret = sscanf(p, "%31s %d %x %x %n", id, &var, &mask, &val, &n); > + if (ret != 4) > + break; > + p += n; > + if (var >= ARRAY_SIZE(vars) || > + strcmp(id, dev_name(mmc_dev(mmc)))) > + continue; > + old = *vars[var]; > + new = (old & ~mask) | val; > + if (old != new) { > + *vars[var] = new; > + DBG("debug_spec: updating var %d %#x -> %#x\n", > + var, old, new); > + } > + } > +} > + > static void sdhci_allocate_bounce_buffer(struct sdhci_host *host) > { > struct mmc_host *mmc = host->mmc; > @@ -3967,6 +3996,8 @@ int sdhci_setup_host(struct sdhci_host *host) > > sdhci_read_caps(host); > > + sdhci_update_debug_quirks_and_caps(host); > + > override_timeout_clk = host->timeout_clk; > > if (host->version > SDHCI_SPEC_420) { > @@ -4513,6 +4544,8 @@ int __sdhci_add_host(struct sdhci_host *host) > struct mmc_host *mmc = host->mmc; > int ret; > > + sdhci_update_debug_quirks_and_caps(host); > + > host->complete_wq = alloc_workqueue("sdhci", flags, 0); > if (!host->complete_wq) > return -ENOMEM; > @@ -4676,6 +4709,7 @@ module_exit(sdhci_drv_exit); > > module_param(debug_quirks, uint, 0444); > module_param(debug_quirks2, uint, 0444); > +module_param(debug_spec, charp, 0444); > > MODULE_AUTHOR("Pierre Ossman <pierre@ossman.eu>"); > MODULE_DESCRIPTION("Secure Digital Host Controller Interface core driver"); > @@ -4683,3 +4717,10 @@ MODULE_LICENSE("GPL"); > > MODULE_PARM_DESC(debug_quirks, "Force certain quirks."); > MODULE_PARM_DESC(debug_quirks2, "Force certain other quirks."); > +MODULE_PARM_DESC(debug_spec, > + "Alter quirks, sdhci capabilities and MMC capabilities. The " > + "string contains one or more: <device name> <variable> <mask> " > + "<value> where <variable> is 0 (debug_quirks), " > + "1 (debug_quirks2), 2 (sdhci->caps), 3 (sdhci->caps1), " > + "4 (mmc->caps) or 5 (mmc->caps2), <mask> is a hex bit mask of " > + "bits to change, and <value> is the hex value."); >
Hi, On 1/15/20 2:48 PM, Adrian Hunter wrote: > On 15/01/20 3:31 pm, Hans de Goede wrote: >> Hi, >> >> On 15-01-2020 13:57, Adrian Hunter wrote: >>> On 8/01/20 11:39 am, Hans de Goede wrote: >>>> Based on a sample of 7 DSDTs from Cherry Trail devices using an AXP288 >>>> PMIC depending on the design one of 2 possible LDOs on the PMIC is used >>>> for the MMC signalling voltage, either DLDO3 or GPIO1LDO (GPIO1 pin in >>>> low noise LDO mode). >>>> >>>> The Lenovo Miix 320-10ICR uses GPIO1LDO in the SHC1 ACPI device's DSM >>>> methods to set 3.3 or 1.8 signalling voltage and this appears to work >>>> as advertised, so presumably the device is actually using GPIO1LDO for >>>> the external microSD signalling voltage. >>>> >>>> But this device has a bug in the _PS0 method of the SHC1 ACPI device, >>>> the DSM remembers the last set signalling voltage and the _PS0 restores >>>> this after a (runtime) suspend-resume cycle, but it "restores" the voltage >>>> on DLDO3 instead of setting it on GPIO1LDO as the DSM method does. DLDO3 >>>> is used for the LCD and setting it to 1.8V causes the LCD to go black. >>>> >>>> This issue can be worked around by setting the SDHCI_QUIRK2_NO_1_8_V >>>> quirk on the sdhci_host so that the DSM never gets used to program the >>>> signalling voltage to 1.8V. >>> >>> Could you instead call the 3.3V DSM at runtime suspend time, then the _PS0 >>> would not "restore" the 1.8V value? That should allow you to use 1.8V UHS-I >>> speed modes with SD cards that support them. >> >> I have considered doing this, but this means reprogramming the signal >> voltage to 3.3V at a time when the card does not expect this, is this ok? > > The host controller does not runtime suspend unless the card is runtime > suspended, so the bus power should be off. > >> >> We would then also need to recall the DSM to put the voltage back to 1.8V >> from resume. > > No, the card will be reinitialized at 3.3V and switch back to 1.8V. > >> I have a feeling that this is probably what Windows does >> (I guess it moves the entire card back to a more safe IOS mode before >> suspend), accidentally avoiding the bug. >> >> I assume you want to only call the DSM to set the voltage to 3.3V on >> the affected model, or do you want to do this on all machines ? > > I would stick with the specific machine for now. Ok I've prepared a new version of the patch for this, which resets the signal voltage to 3.3V at the end of the (runtime)suspend handler using the Intel DSM and this is only done on the affected model. I can confirm that this workaround fixes the LCD going black, while keeping UHS modes working. I'll send out v2 of this patch-series soon. Regards, Hans > >> >> Adding this does seem to introduce more complexity then simply disabling >> 1.8V modes and given that it is just a single model which is affected >> I went with the more simple option of just disabling the 1.8V modes. >> >> Ideally we would not need any quirks, but if we do we should at least >> make the work-around as simple as possible. So I've a slight preference >> for just sticking with DHCI_QUIRK2_NO_1_8_V ... >> >> Note that the suspend/resume handling is broken also in the sense that >> it does not disable the signal voltage during suspend. > > The bus power gets switched off if the card is runtime suspended. The host > controller should go to D3cold which means everything off. > >> >>>> So far we have mostly been able to avoid using device specific quirks in >>>> the sdhci-acpi code, but given that this issue is specific to this one >>>> model and we certainly do not want to disable 1.8V modes everywhere I >>>> see no other option. >>>> >>>> This commit adds a new mechanism for setting sdhci-acpi specific quirks >>>> and a matching sdhci-acpi.quirks module parameter to make testing quirks / >>>> similar issues on other devices easier. >>>> >>>> The first quirk supported by this mechanism is SDHCI_ACPI_QUIRK_SD_NO_1_8V, >>>> which when set causes any slots with the SDHCI_ACPI_SD_CD flag to get the >>>> SDHCI_QUIRK2_NO_1_8_V quirk set on their sdhci_host. >>>> >>>> This commit also adds a DMI table for specifying default quirks for some >>>> models and adds an entry for the Lenovo Miix 320-10ICR which enables the >>>> SDHCI_QUIRK2_NO_1_8_V by default on this model, fixing the LCD going black >>>> when the external microSD slot is used. >>>> >>>> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=111294 >>>> BugLink: https://gitlab.freedesktop.org/drm/intel/issues/355 >>>> Reported-by: russianneuromancer <russianneuromancer@ya.ru> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> drivers/mmc/host/sdhci-acpi.c | 39 +++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 39 insertions(+) >>>> >>>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c >>>> index 105e73d4a3b9..9f150c73e958 100644 >>>> --- a/drivers/mmc/host/sdhci-acpi.c >>>> +++ b/drivers/mmc/host/sdhci-acpi.c >>>> @@ -23,6 +23,7 @@ >>>> #include <linux/pm.h> >>>> #include <linux/pm_runtime.h> >>>> #include <linux/delay.h> >>>> +#include <linux/dmi.h> >>>> #include <linux/mmc/host.h> >>>> #include <linux/mmc/pm.h> >>>> @@ -75,6 +76,14 @@ struct sdhci_acpi_host { >>>> unsigned long private[0] ____cacheline_aligned; >>>> }; >>>> +enum { >>>> + SDHCI_ACPI_QUIRK_SD_NO_1_8V = BIT(0), >>>> +}; >>>> + >>>> +static int quirks = -1; >>>> +module_param(quirks, int, 0444); >>>> +MODULE_PARM_DESC(quirks, "Override sdhci-acpi specific quirks"); >>> >>> Why is a module parameter needed? >> >> The module parameter is purely to make testing if the same quirk(s) >> help on other devices easier. Like the debug_quirks[2] params in sdhci.c > > Mmm, but we already have SDHCI_QUIRK2_NO_1_8_V > >> >> Regards, >> >> Hans >> >> >> >>>> + >>>> static inline void *sdhci_acpi_priv(struct sdhci_acpi_host *c) >>>> { >>>> return (void *)c->private; >>>> @@ -647,6 +656,24 @@ static const struct acpi_device_id sdhci_acpi_ids[] = { >>>> }; >>>> MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids); >>>> +static const struct dmi_system_id sdhci_acpi_quirks[] = { >>>> + { >>>> + /* >>>> + * The Lenovo Miix 320-10ICR has a bug in the _PS0 method of >>>> + * the SHC1 ACPI device, this bug causes it to reprogram the >>>> + * wrong LDO (DLDO3) to 1.8V if 1.8V modes are used and the >>>> + * card is (runtime) suspended + resumed. DLDO3 is used for >>>> + * the LCD and setting it to 1.8V causes the LCD to go black. >>>> + */ >>>> + .matches = { >>>> + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), >>>> + DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo MIIX 320-10ICR"), >>>> + }, >>>> + .driver_data = (void *)SDHCI_ACPI_QUIRK_SD_NO_1_8V, >>>> + }, >>>> + {} /* Terminating entry */ >>>> +}; >>>> + >>>> static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(struct >>>> acpi_device *adev) >>>> { >>>> const struct sdhci_acpi_uid_slot *u; >>>> @@ -663,6 +690,7 @@ static int sdhci_acpi_probe(struct platform_device >>>> *pdev) >>>> struct device *dev = &pdev->dev; >>>> const struct sdhci_acpi_slot *slot; >>>> struct acpi_device *device, *child; >>>> + const struct dmi_system_id *id; >>>> struct sdhci_acpi_host *c; >>>> struct sdhci_host *host; >>>> struct resource *iomem; >>>> @@ -670,6 +698,14 @@ static int sdhci_acpi_probe(struct platform_device >>>> *pdev) >>>> size_t priv_size; >>>> int err; >>>> + if (quirks == -1) { >>>> + id = dmi_first_match(sdhci_acpi_quirks); >>>> + if (id) >>>> + quirks = (long)id->driver_data; >>>> + else >>>> + quirks = 0; >>>> + } >>>> + >>>> device = ACPI_COMPANION(dev); >>>> if (!device) >>>> return -ENODEV; >>>> @@ -759,6 +795,9 @@ static int sdhci_acpi_probe(struct platform_device >>>> *pdev) >>>> dev_warn(dev, "failed to setup card detect gpio\n"); >>>> c->use_runtime_pm = false; >>>> } >>>> + >>>> + if (quirks & SDHCI_ACPI_QUIRK_SD_NO_1_8V) >>>> + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; >>>> } >>>> err = sdhci_setup_host(host); >>>> >>> >> >
diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c index 105e73d4a3b9..9f150c73e958 100644 --- a/drivers/mmc/host/sdhci-acpi.c +++ b/drivers/mmc/host/sdhci-acpi.c @@ -23,6 +23,7 @@ #include <linux/pm.h> #include <linux/pm_runtime.h> #include <linux/delay.h> +#include <linux/dmi.h> #include <linux/mmc/host.h> #include <linux/mmc/pm.h> @@ -75,6 +76,14 @@ struct sdhci_acpi_host { unsigned long private[0] ____cacheline_aligned; }; +enum { + SDHCI_ACPI_QUIRK_SD_NO_1_8V = BIT(0), +}; + +static int quirks = -1; +module_param(quirks, int, 0444); +MODULE_PARM_DESC(quirks, "Override sdhci-acpi specific quirks"); + static inline void *sdhci_acpi_priv(struct sdhci_acpi_host *c) { return (void *)c->private; @@ -647,6 +656,24 @@ static const struct acpi_device_id sdhci_acpi_ids[] = { }; MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids); +static const struct dmi_system_id sdhci_acpi_quirks[] = { + { + /* + * The Lenovo Miix 320-10ICR has a bug in the _PS0 method of + * the SHC1 ACPI device, this bug causes it to reprogram the + * wrong LDO (DLDO3) to 1.8V if 1.8V modes are used and the + * card is (runtime) suspended + resumed. DLDO3 is used for + * the LCD and setting it to 1.8V causes the LCD to go black. + */ + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), + DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo MIIX 320-10ICR"), + }, + .driver_data = (void *)SDHCI_ACPI_QUIRK_SD_NO_1_8V, + }, + {} /* Terminating entry */ +}; + static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(struct acpi_device *adev) { const struct sdhci_acpi_uid_slot *u; @@ -663,6 +690,7 @@ static int sdhci_acpi_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; const struct sdhci_acpi_slot *slot; struct acpi_device *device, *child; + const struct dmi_system_id *id; struct sdhci_acpi_host *c; struct sdhci_host *host; struct resource *iomem; @@ -670,6 +698,14 @@ static int sdhci_acpi_probe(struct platform_device *pdev) size_t priv_size; int err; + if (quirks == -1) { + id = dmi_first_match(sdhci_acpi_quirks); + if (id) + quirks = (long)id->driver_data; + else + quirks = 0; + } + device = ACPI_COMPANION(dev); if (!device) return -ENODEV; @@ -759,6 +795,9 @@ static int sdhci_acpi_probe(struct platform_device *pdev) dev_warn(dev, "failed to setup card detect gpio\n"); c->use_runtime_pm = false; } + + if (quirks & SDHCI_ACPI_QUIRK_SD_NO_1_8V) + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; } err = sdhci_setup_host(host);
Based on a sample of 7 DSDTs from Cherry Trail devices using an AXP288 PMIC depending on the design one of 2 possible LDOs on the PMIC is used for the MMC signalling voltage, either DLDO3 or GPIO1LDO (GPIO1 pin in low noise LDO mode). The Lenovo Miix 320-10ICR uses GPIO1LDO in the SHC1 ACPI device's DSM methods to set 3.3 or 1.8 signalling voltage and this appears to work as advertised, so presumably the device is actually using GPIO1LDO for the external microSD signalling voltage. But this device has a bug in the _PS0 method of the SHC1 ACPI device, the DSM remembers the last set signalling voltage and the _PS0 restores this after a (runtime) suspend-resume cycle, but it "restores" the voltage on DLDO3 instead of setting it on GPIO1LDO as the DSM method does. DLDO3 is used for the LCD and setting it to 1.8V causes the LCD to go black. This issue can be worked around by setting the SDHCI_QUIRK2_NO_1_8_V quirk on the sdhci_host so that the DSM never gets used to program the signalling voltage to 1.8V. So far we have mostly been able to avoid using device specific quirks in the sdhci-acpi code, but given that this issue is specific to this one model and we certainly do not want to disable 1.8V modes everywhere I see no other option. This commit adds a new mechanism for setting sdhci-acpi specific quirks and a matching sdhci-acpi.quirks module parameter to make testing quirks / similar issues on other devices easier. The first quirk supported by this mechanism is SDHCI_ACPI_QUIRK_SD_NO_1_8V, which when set causes any slots with the SDHCI_ACPI_SD_CD flag to get the SDHCI_QUIRK2_NO_1_8_V quirk set on their sdhci_host. This commit also adds a DMI table for specifying default quirks for some models and adds an entry for the Lenovo Miix 320-10ICR which enables the SDHCI_QUIRK2_NO_1_8_V by default on this model, fixing the LCD going black when the external microSD slot is used. BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=111294 BugLink: https://gitlab.freedesktop.org/drm/intel/issues/355 Reported-by: russianneuromancer <russianneuromancer@ya.ru> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/mmc/host/sdhci-acpi.c | 39 +++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)