[PATCH 0/2] mmc: sdhci-acpi: Introduce device specific quirks, fix issues on 2 device models

[PATCH 0/2] mmc: sdhci-acpi: Introduce device specific quirks, fix issues on 2 device models

[Hans de Goede]

Hi Adrian,

I know you have sofar mostly resisted adding device specific (DMI based)
quirks to the sdhci-acpi.c driver and I agree with you that whenever
possible, those should be avoided.

But yesterday I was debugging an issue where using the microSD slot
causes the LCD panel of a tablet to go black. This turns out to be a
bug in the DSDT which gets triggered when using 1.8V modes, see the first
patch for details. In this case I really so no other option then disabling
1.8V modes and only doing so only on the affected device model.

Another issue which I had on my TODO list of things to fix is the Acer
SW5-012 version of the Acer Switch 10 models always reporting the microSD
as being write-protected. Here too I see no other option then a model
specific quirk, since some BYT devices may use a normal SD slot with
actual write-protect capabilities and we do not want to disable
write-protect checking everywhere just because it is broken on one model.
The workaround for this is the second patch in this series.

Regards,

Hans

[PATCH 1/2] mmc: sdhci-acpi: Disable 1.8V modes on external microSD on Lenovo Miix 320

[Hans de Goede]

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(+)

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);
-- 
2.24.1

Re: [PATCH 1/2] mmc: sdhci-acpi: Disable 1.8V modes on external microSD on Lenovo Miix 320

[Adrian Hunter]

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);
> 

Re: [PATCH 1/2] mmc: sdhci-acpi: Disable 1.8V modes on external microSD on Lenovo Miix 320

[Hans de Goede]

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);
>>
> 

Re: [PATCH 1/2] mmc: sdhci-acpi: Disable 1.8V modes on external microSD on Lenovo Miix 320

[Adrian Hunter]

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);
>>>
>>
> 

Re: [PATCH 1/2] mmc: sdhci-acpi: Disable 1.8V modes on external microSD on Lenovo Miix 320

[Hans de Goede]

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);
>>>>
>>>
>>
> 

Re: [PATCH 1/2] mmc: sdhci-acpi: Disable 1.8V modes on external microSD on Lenovo Miix 320

[Adrian Hunter]

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);
>>>>>
>>>>
>>>
>>
> 

[FSL P5020 P5040 PPC] Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates

[Christian Zigotzky]

[-- Attachment #1: Type: text/plain, Size: 295 bytes --]

Hi All,

We still need the attached patch for our onboard SD card interface 
[1,2]. Could you please add this patch to the tree?

Thanks,
Christian

[1] https://www.spinics.net/lists/linux-mmc/msg56211.html
[2] 
http://forum.hyperion-entertainment.com/viewtopic.php?f=58&t=4349&start=20#p49012


[-- Attachment #2: coherent_cache-v3.patch --]
[-- Type: text/x-patch, Size: 1860 bytes --]

diff -rupN a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
--- a/arch/powerpc/kernel/setup-common.c	2019-12-03 18:05:52.436070217 +0100
+++ b/arch/powerpc/kernel/setup-common.c	2019-12-03 18:03:20.316629696 +0100
@@ -780,6 +780,22 @@ static int __init check_cache_coherency(
 late_initcall(check_cache_coherency);
 #endif /* CONFIG_CHECK_CACHE_COHERENCY */
 
+#ifndef CONFIG_NOT_COHERENT_CACHE
+/*
+ * For historical reasons powerpc kernels are built with hard wired knowledge of
+ * whether or not DMA accesses are cache coherent. Additionally device trees on
+ * powerpc do not typically support the dma-coherent property.
+ *
+ * So when we know that DMA is coherent, override arch_of_dma_is_coherent() to
+ * tell the drivers/of code that all devices are coherent regardless of whether
+ * they have a dma-coherent property.
+ */
+bool arch_of_dma_is_coherent(struct device_node *np)
+{
+	return true;
+}
+#endif
+
 #ifdef CONFIG_DEBUG_FS
 struct dentry *powerpc_debugfs_root;
 EXPORT_SYMBOL(powerpc_debugfs_root);
diff -rupN a/drivers/of/address.c b/drivers/of/address.c
--- a/drivers/of/address.c	2019-12-03 18:05:57.332052212 +0100
+++ b/drivers/of/address.c	2019-12-03 18:03:20.320629681 +0100
@@ -990,6 +990,14 @@ out:
 	return ret;
 }
 
+/*
+ * arch_of_dma_is_coherent - Arch hook to determine if device is coherent for DMA
+ */
+bool __weak arch_of_dma_is_coherent(struct device_node *np)
+{
+	return false;
+}
+
 /**
  * of_dma_is_coherent - Check if device is coherent
  * @np:	device node
@@ -999,8 +1007,12 @@ out:
  */
 bool of_dma_is_coherent(struct device_node *np)
 {
-	struct device_node *node = of_node_get(np);
+	struct device_node *node;
+
+	if (arch_of_dma_is_coherent(np))
+		return true;
 
+	np = of_node_get(np);
 	while (node) {
 		if (of_property_read_bool(node, "dma-coherent")) {
 			of_node_put(node);

Re: [FSL P5020 P5040 PPC] Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates

[Ulf Hansson]

On Thu, 16 Jan 2020 at 12:18, Christian Zigotzky <chzigotzky@xenosoft.de> wrote:
>
> Hi All,
>
> We still need the attached patch for our onboard SD card interface
> [1,2]. Could you please add this patch to the tree?

No, because according to previous discussion that isn't the correct
solution and more importantly it will break other archs (if I recall
correctly).

Looks like someone from the ppc community needs to pick up the ball.

>
> Thanks,
> Christian
>
> [1] https://www.spinics.net/lists/linux-mmc/msg56211.html

I think this discussion even suggested some viable solutions, so it
just be a matter of sending a patch :-)

> [2]
> http://forum.hyperion-entertainment.com/viewtopic.php?f=58&t=4349&start=20#p49012
>

Kind regards
Uffe

Re: [FSL P5020 P5040 PPC] Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates

[Ulf Hansson]

On Thu, 16 Jan 2020 at 12:18, Christian Zigotzky <chzigotzky@xenosoft.de> wrote:
>
> Hi All,
>
> We still need the attached patch for our onboard SD card interface
> [1,2]. Could you please add this patch to the tree?

No, because according to previous discussion that isn't the correct
solution and more importantly it will break other archs (if I recall
correctly).

Looks like someone from the ppc community needs to pick up the ball.

>
> Thanks,
> Christian
>
> [1] https://www.spinics.net/lists/linux-mmc/msg56211.html

I think this discussion even suggested some viable solutions, so it
just be a matter of sending a patch :-)

> [2]
> http://forum.hyperion-entertainment.com/viewtopic.php?f=58&t=4349&start=20#p49012
>

Kind regards
Uffe

[FSL P5020 P5040 PPC] Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates

[Christian Zigotzky]

Am 16.01.20 um 16:46 schrieb Ulf Hansson:
> On Thu, 16 Jan 2020 at 12:18, Christian Zigotzky <chzigotzky@xenosoft.de> wrote:
>> Hi All,
>>
>> We still need the attached patch for our onboard SD card interface
>> [1,2]. Could you please add this patch to the tree?
> No, because according to previous discussion that isn't the correct
> solution and more importantly it will break other archs (if I recall
> correctly).
>
> Looks like someone from the ppc community needs to pick up the ball.
I am not sure if the ppc community have to fix this issue because your 
updates (mmc-v5.4-2) are responsible for this issue. If nobody wants to 
fix this issue then we will lost the onboard SD card support in the 
future. PLEASE check the 'mmc-v5.4-2' updates again.
>
>> Thanks,
>> Christian
>>
>> [1] https://www.spinics.net/lists/linux-mmc/msg56211.html
> I think this discussion even suggested some viable solutions, so it
> just be a matter of sending a patch :-)
>
>> [2]
>> http://forum.hyperion-entertainment.com/viewtopic.php?f=58&t=4349&start=20#p49012
>>
> Kind regards
> Uffe

[FSL P5020 P5040 PPC] Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates

[Christian Zigotzky]

Am 16.01.20 um 16:46 schrieb Ulf Hansson:
> On Thu, 16 Jan 2020 at 12:18, Christian Zigotzky <chzigotzky@xenosoft.de> wrote:
>> Hi All,
>>
>> We still need the attached patch for our onboard SD card interface
>> [1,2]. Could you please add this patch to the tree?
> No, because according to previous discussion that isn't the correct
> solution and more importantly it will break other archs (if I recall
> correctly).
>
> Looks like someone from the ppc community needs to pick up the ball.
I am not sure if the ppc community have to fix this issue because your 
updates (mmc-v5.4-2) are responsible for this issue. If nobody wants to 
fix this issue then we will lost the onboard SD card support in the 
future. PLEASE check the 'mmc-v5.4-2' updates again.
>
>> Thanks,
>> Christian
>>
>> [1] https://www.spinics.net/lists/linux-mmc/msg56211.html
> I think this discussion even suggested some viable solutions, so it
> just be a matter of sending a patch :-)
>
>> [2]
>> http://forum.hyperion-entertainment.com/viewtopic.php?f=58&t=4349&start=20#p49012
>>
> Kind regards
> Uffe

Re: [FSL P5020 P5040 PPC] Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates

[Ulf Hansson]

On Mon, 20 Jan 2020 at 10:17, Christian Zigotzky <chzigotzky@xenosoft.de> wrote:
>
> Am 16.01.20 um 16:46 schrieb Ulf Hansson:
> > On Thu, 16 Jan 2020 at 12:18, Christian Zigotzky <chzigotzky@xenosoft.de> wrote:
> >> Hi All,
> >>
> >> We still need the attached patch for our onboard SD card interface
> >> [1,2]. Could you please add this patch to the tree?
> > No, because according to previous discussion that isn't the correct
> > solution and more importantly it will break other archs (if I recall
> > correctly).
> >
> > Looks like someone from the ppc community needs to pick up the ball.
> I am not sure if the ppc community have to fix this issue because your
> updates (mmc-v5.4-2) are responsible for this issue. If nobody wants to
> fix this issue then we will lost the onboard SD card support in the
> future. PLEASE check the 'mmc-v5.4-2' updates again.

Applying your suggested fix breaks other archs/boards. It's really not
a good situation, but I will not take a step back when it's quite easy
to take a step forward instead.

Someone just need to care and send a patch, it doesn't look that hard
to me, but maybe I am wrong.

Apologies if this isn't the answer you wanted, but that's all I can do
for now, sorry.

Kind regards
Uffe

Re: [FSL P5020 P5040 PPC] Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates

[Ulf Hansson]

On Mon, 20 Jan 2020 at 10:17, Christian Zigotzky <chzigotzky@xenosoft.de> wrote:
>
> Am 16.01.20 um 16:46 schrieb Ulf Hansson:
> > On Thu, 16 Jan 2020 at 12:18, Christian Zigotzky <chzigotzky@xenosoft.de> wrote:
> >> Hi All,
> >>
> >> We still need the attached patch for our onboard SD card interface
> >> [1,2]. Could you please add this patch to the tree?
> > No, because according to previous discussion that isn't the correct
> > solution and more importantly it will break other archs (if I recall
> > correctly).
> >
> > Looks like someone from the ppc community needs to pick up the ball.
> I am not sure if the ppc community have to fix this issue because your
> updates (mmc-v5.4-2) are responsible for this issue. If nobody wants to
> fix this issue then we will lost the onboard SD card support in the
> future. PLEASE check the 'mmc-v5.4-2' updates again.

Applying your suggested fix breaks other archs/boards. It's really not
a good situation, but I will not take a step back when it's quite easy
to take a step forward instead.

Someone just need to care and send a patch, it doesn't look that hard
to me, but maybe I am wrong.

Apologies if this isn't the answer you wanted, but that's all I can do
for now, sorry.

Kind regards
Uffe

Re: [FSL P5020 P5040 PPC] Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates

[Michael Ellerman]

Ulf Hansson <ulf.hansson@linaro.org> writes:
> On Thu, 16 Jan 2020 at 12:18, Christian Zigotzky <chzigotzky@xenosoft.de> wrote:
>>
>> Hi All,
>>
>> We still need the attached patch for our onboard SD card interface
>> [1,2]. Could you please add this patch to the tree?
>
> No, because according to previous discussion that isn't the correct
> solution and more importantly it will break other archs (if I recall
> correctly).
>
> Looks like someone from the ppc community needs to pick up the ball.

That's a pretty small community these days :) :/

Christian can you test this please? I think I got the polarity of all
the tests right, but it's Friday night so maybe I'm wrong :)

cheers


From 975ba6e8b52d6f5358e93c1f5a47adc4a0b5fb70 Mon Sep 17 00:00:00 2001
From: Michael Ellerman <mpe@ellerman.id.au>
Date: Fri, 24 Jan 2020 22:26:59 +1100
Subject: [PATCH] of: Add OF_DMA_DEFAULT_COHERENT & select it on powerpc

There's an OF helper called of_dma_is_coherent(), which checks if a
device has a "dma-coherent" property to see if the device is coherent
for DMA.

But on some platforms devices are coherent by default, and on some
platforms it's not possible to update existing device trees to add the
"dma-coherent" property.

So add a Kconfig symbol to allow arch code to tell
of_dma_is_coherent() that devices are coherent by default, regardless
of the presence of the property.

Select that symbol on powerpc when NOT_COHERENT_CACHE is not set, ie.
when the system has a coherent cache.

Fixes: 92ea637edea3 ("of: introduce of_dma_is_coherent() helper")
Cc: stable@vger.kernel.org # v3.16+
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/Kconfig | 1 +
 drivers/of/Kconfig   | 4 ++++
 drivers/of/address.c | 6 +++++-
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 62752c3bfabf..460678ab2375 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -235,6 +235,7 @@ config PPC
 	select NEED_DMA_MAP_STATE		if PPC64 || NOT_COHERENT_CACHE
 	select NEED_SG_DMA_LENGTH
 	select OF
+	select OF_DMA_DEFAULT_COHERENT		if !NOT_COHERENT_CACHE
 	select OF_EARLY_FLATTREE
 	select OLD_SIGACTION			if PPC32
 	select OLD_SIGSUSPEND
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 37c2ccbefecd..d91618641be6 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -103,4 +103,8 @@ config OF_OVERLAY
 config OF_NUMA
 	bool
 
+config OF_DMA_DEFAULT_COHERENT
+	# arches should select this if DMA is coherent by default for OF devices
+	bool
+
 endif # OF
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 99c1b8058559..e8a39c3ec4d4 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -995,12 +995,16 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz
  * @np:	device node
  *
  * It returns true if "dma-coherent" property was found
- * for this device in DT.
+ * for this device in the DT, or if DMA is coherent by
+ * default for OF devices on the current platform.
  */
 bool of_dma_is_coherent(struct device_node *np)
 {
 	struct device_node *node = of_node_get(np);
 
+	if (IS_ENABLED(CONFIG_OF_DMA_DEFAULT_COHERENT))
+		return true;
+
 	while (node) {
 		if (of_property_read_bool(node, "dma-coherent")) {
 			of_node_put(node);
-- 
2.21.1

Re: [FSL P5020 P5040 PPC] Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates

[Michael Ellerman]

Ulf Hansson <ulf.hansson@linaro.org> writes:
> On Thu, 16 Jan 2020 at 12:18, Christian Zigotzky <chzigotzky@xenosoft.de> wrote:
>>
>> Hi All,
>>
>> We still need the attached patch for our onboard SD card interface
>> [1,2]. Could you please add this patch to the tree?
>
> No, because according to previous discussion that isn't the correct
> solution and more importantly it will break other archs (if I recall
> correctly).
>
> Looks like someone from the ppc community needs to pick up the ball.

That's a pretty small community these days :) :/

Christian can you test this please? I think I got the polarity of all
the tests right, but it's Friday night so maybe I'm wrong :)

cheers


From 975ba6e8b52d6f5358e93c1f5a47adc4a0b5fb70 Mon Sep 17 00:00:00 2001
From: Michael Ellerman <mpe@ellerman.id.au>
Date: Fri, 24 Jan 2020 22:26:59 +1100
Subject: [PATCH] of: Add OF_DMA_DEFAULT_COHERENT & select it on powerpc

There's an OF helper called of_dma_is_coherent(), which checks if a
device has a "dma-coherent" property to see if the device is coherent
for DMA.

But on some platforms devices are coherent by default, and on some
platforms it's not possible to update existing device trees to add the
"dma-coherent" property.

So add a Kconfig symbol to allow arch code to tell
of_dma_is_coherent() that devices are coherent by default, regardless
of the presence of the property.

Select that symbol on powerpc when NOT_COHERENT_CACHE is not set, ie.
when the system has a coherent cache.

Fixes: 92ea637edea3 ("of: introduce of_dma_is_coherent() helper")
Cc: stable@vger.kernel.org # v3.16+
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/Kconfig | 1 +
 drivers/of/Kconfig   | 4 ++++
 drivers/of/address.c | 6 +++++-
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 62752c3bfabf..460678ab2375 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -235,6 +235,7 @@ config PPC
 	select NEED_DMA_MAP_STATE		if PPC64 || NOT_COHERENT_CACHE
 	select NEED_SG_DMA_LENGTH
 	select OF
+	select OF_DMA_DEFAULT_COHERENT		if !NOT_COHERENT_CACHE
 	select OF_EARLY_FLATTREE
 	select OLD_SIGACTION			if PPC32
 	select OLD_SIGSUSPEND
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 37c2ccbefecd..d91618641be6 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -103,4 +103,8 @@ config OF_OVERLAY
 config OF_NUMA
 	bool
 
+config OF_DMA_DEFAULT_COHERENT
+	# arches should select this if DMA is coherent by default for OF devices
+	bool
+
 endif # OF
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 99c1b8058559..e8a39c3ec4d4 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -995,12 +995,16 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz
  * @np:	device node
  *
  * It returns true if "dma-coherent" property was found
- * for this device in DT.
+ * for this device in the DT, or if DMA is coherent by
+ * default for OF devices on the current platform.
  */
 bool of_dma_is_coherent(struct device_node *np)
 {
 	struct device_node *node = of_node_get(np);
 
+	if (IS_ENABLED(CONFIG_OF_DMA_DEFAULT_COHERENT))
+		return true;
+
 	while (node) {
 		if (of_property_read_bool(node, "dma-coherent")) {
 			of_node_put(node);
-- 
2.21.1

Re: [FSL P5020 P5040 PPC] Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates

[Christian Zigotzky]

On 24 January 2020 at 12:42 pm, Michael Ellerman wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:
>> On Thu, 16 Jan 2020 at 12:18, Christian Zigotzky <chzigotzky@xenosoft.de> wrote:
>>> Hi All,
>>>
>>> We still need the attached patch for our onboard SD card interface
>>> [1,2]. Could you please add this patch to the tree?
>> No, because according to previous discussion that isn't the correct
>> solution and more importantly it will break other archs (if I recall
>> correctly).
>>
>> Looks like someone from the ppc community needs to pick up the ball.
> That's a pretty small community these days :) :/
>
> Christian can you test this please? I think I got the polarity of all
> the tests right, but it's Friday night so maybe I'm wrong :)
>
> cheers
Michael,

Thanks a lot for the new patch! I compiled the RC7 of kernel 5.5 with 
your patch again yesterday and the kernel works without any problems 
with our onboard SD cards. [1]

Cheers,
Christian

[1] 
http://forum.hyperion-entertainment.com/viewtopic.php?f=58&t=4384&p=49697#p49693
>
> >From 975ba6e8b52d6f5358e93c1f5a47adc4a0b5fb70 Mon Sep 17 00:00:00 2001
> From: Michael Ellerman <mpe@ellerman.id.au>
> Date: Fri, 24 Jan 2020 22:26:59 +1100
> Subject: [PATCH] of: Add OF_DMA_DEFAULT_COHERENT & select it on powerpc
>
> There's an OF helper called of_dma_is_coherent(), which checks if a
> device has a "dma-coherent" property to see if the device is coherent
> for DMA.
>
> But on some platforms devices are coherent by default, and on some
> platforms it's not possible to update existing device trees to add the
> "dma-coherent" property.
>
> So add a Kconfig symbol to allow arch code to tell
> of_dma_is_coherent() that devices are coherent by default, regardless
> of the presence of the property.
>
> Select that symbol on powerpc when NOT_COHERENT_CACHE is not set, ie.
> when the system has a coherent cache.
>
> Fixes: 92ea637edea3 ("of: introduce of_dma_is_coherent() helper")
> Cc: stable@vger.kernel.org # v3.16+
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>   arch/powerpc/Kconfig | 1 +
>   drivers/of/Kconfig   | 4 ++++
>   drivers/of/address.c | 6 +++++-
>   3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 62752c3bfabf..460678ab2375 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -235,6 +235,7 @@ config PPC
>   	select NEED_DMA_MAP_STATE		if PPC64 || NOT_COHERENT_CACHE
>   	select NEED_SG_DMA_LENGTH
>   	select OF
> +	select OF_DMA_DEFAULT_COHERENT		if !NOT_COHERENT_CACHE
>   	select OF_EARLY_FLATTREE
>   	select OLD_SIGACTION			if PPC32
>   	select OLD_SIGSUSPEND
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 37c2ccbefecd..d91618641be6 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -103,4 +103,8 @@ config OF_OVERLAY
>   config OF_NUMA
>   	bool
>   
> +config OF_DMA_DEFAULT_COHERENT
> +	# arches should select this if DMA is coherent by default for OF devices
> +	bool
> +
>   endif # OF
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 99c1b8058559..e8a39c3ec4d4 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -995,12 +995,16 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz
>    * @np:	device node
>    *
>    * It returns true if "dma-coherent" property was found
> - * for this device in DT.
> + * for this device in the DT, or if DMA is coherent by
> + * default for OF devices on the current platform.
>    */
>   bool of_dma_is_coherent(struct device_node *np)
>   {
>   	struct device_node *node = of_node_get(np);
>   
> +	if (IS_ENABLED(CONFIG_OF_DMA_DEFAULT_COHERENT))
> +		return true;
> +
>   	while (node) {
>   		if (of_property_read_bool(node, "dma-coherent")) {
>   			of_node_put(node);

Re: [FSL P5020 P5040 PPC] Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates

[Christian Zigotzky]

On 24 January 2020 at 12:42 pm, Michael Ellerman wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:
>> On Thu, 16 Jan 2020 at 12:18, Christian Zigotzky <chzigotzky@xenosoft.de> wrote:
>>> Hi All,
>>>
>>> We still need the attached patch for our onboard SD card interface
>>> [1,2]. Could you please add this patch to the tree?
>> No, because according to previous discussion that isn't the correct
>> solution and more importantly it will break other archs (if I recall
>> correctly).
>>
>> Looks like someone from the ppc community needs to pick up the ball.
> That's a pretty small community these days :) :/
>
> Christian can you test this please? I think I got the polarity of all
> the tests right, but it's Friday night so maybe I'm wrong :)
>
> cheers
Michael,

Thanks a lot for the new patch! I compiled the RC7 of kernel 5.5 with 
your patch again yesterday and the kernel works without any problems 
with our onboard SD cards. [1]

Cheers,
Christian

[1] 
http://forum.hyperion-entertainment.com/viewtopic.php?f=58&t=4384&p=49697#p49693
>
> >From 975ba6e8b52d6f5358e93c1f5a47adc4a0b5fb70 Mon Sep 17 00:00:00 2001
> From: Michael Ellerman <mpe@ellerman.id.au>
> Date: Fri, 24 Jan 2020 22:26:59 +1100
> Subject: [PATCH] of: Add OF_DMA_DEFAULT_COHERENT & select it on powerpc
>
> There's an OF helper called of_dma_is_coherent(), which checks if a
> device has a "dma-coherent" property to see if the device is coherent
> for DMA.
>
> But on some platforms devices are coherent by default, and on some
> platforms it's not possible to update existing device trees to add the
> "dma-coherent" property.
>
> So add a Kconfig symbol to allow arch code to tell
> of_dma_is_coherent() that devices are coherent by default, regardless
> of the presence of the property.
>
> Select that symbol on powerpc when NOT_COHERENT_CACHE is not set, ie.
> when the system has a coherent cache.
>
> Fixes: 92ea637edea3 ("of: introduce of_dma_is_coherent() helper")
> Cc: stable@vger.kernel.org # v3.16+
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>   arch/powerpc/Kconfig | 1 +
>   drivers/of/Kconfig   | 4 ++++
>   drivers/of/address.c | 6 +++++-
>   3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 62752c3bfabf..460678ab2375 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -235,6 +235,7 @@ config PPC
>   	select NEED_DMA_MAP_STATE		if PPC64 || NOT_COHERENT_CACHE
>   	select NEED_SG_DMA_LENGTH
>   	select OF
> +	select OF_DMA_DEFAULT_COHERENT		if !NOT_COHERENT_CACHE
>   	select OF_EARLY_FLATTREE
>   	select OLD_SIGACTION			if PPC32
>   	select OLD_SIGSUSPEND
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 37c2ccbefecd..d91618641be6 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -103,4 +103,8 @@ config OF_OVERLAY
>   config OF_NUMA
>   	bool
>   
> +config OF_DMA_DEFAULT_COHERENT
> +	# arches should select this if DMA is coherent by default for OF devices
> +	bool
> +
>   endif # OF
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 99c1b8058559..e8a39c3ec4d4 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -995,12 +995,16 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz
>    * @np:	device node
>    *
>    * It returns true if "dma-coherent" property was found
> - * for this device in DT.
> + * for this device in the DT, or if DMA is coherent by
> + * default for OF devices on the current platform.
>    */
>   bool of_dma_is_coherent(struct device_node *np)
>   {
>   	struct device_node *node = of_node_get(np);
>   
> +	if (IS_ENABLED(CONFIG_OF_DMA_DEFAULT_COHERENT))
> +		return true;
> +
>   	while (node) {
>   		if (of_property_read_bool(node, "dma-coherent")) {
>   			of_node_put(node);

Re: [FSL P5020 P5040 PPC] Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates

[Michael Ellerman]

Christian Zigotzky <chzigotzky@xenosoft.de> writes:
> On 24 January 2020 at 12:42 pm, Michael Ellerman wrote:
>> Ulf Hansson <ulf.hansson@linaro.org> writes:
>>> On Thu, 16 Jan 2020 at 12:18, Christian Zigotzky <chzigotzky@xenosoft.de> wrote:
>>>> Hi All,
>>>>
>>>> We still need the attached patch for our onboard SD card interface
>>>> [1,2]. Could you please add this patch to the tree?
>>> No, because according to previous discussion that isn't the correct
>>> solution and more importantly it will break other archs (if I recall
>>> correctly).
>>>
>>> Looks like someone from the ppc community needs to pick up the ball.
>> That's a pretty small community these days :) :/
>>
>> Christian can you test this please? I think I got the polarity of all
>> the tests right, but it's Friday night so maybe I'm wrong :)
>>
>> cheers
> Michael,
>
> Thanks a lot for the new patch! I compiled the RC7 of kernel 5.5 with 
> your patch again yesterday and the kernel works without any problems 
> with our onboard SD cards. [1]

Thanks for testing.

cheers

Re: [FSL P5020 P5040 PPC] Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates

[Michael Ellerman]

Christian Zigotzky <chzigotzky@xenosoft.de> writes:
> On 24 January 2020 at 12:42 pm, Michael Ellerman wrote:
>> Ulf Hansson <ulf.hansson@linaro.org> writes:
>>> On Thu, 16 Jan 2020 at 12:18, Christian Zigotzky <chzigotzky@xenosoft.de> wrote:
>>>> Hi All,
>>>>
>>>> We still need the attached patch for our onboard SD card interface
>>>> [1,2]. Could you please add this patch to the tree?
>>> No, because according to previous discussion that isn't the correct
>>> solution and more importantly it will break other archs (if I recall
>>> correctly).
>>>
>>> Looks like someone from the ppc community needs to pick up the ball.
>> That's a pretty small community these days :) :/
>>
>> Christian can you test this please? I think I got the polarity of all
>> the tests right, but it's Friday night so maybe I'm wrong :)
>>
>> cheers
> Michael,
>
> Thanks a lot for the new patch! I compiled the RC7 of kernel 5.5 with 
> your patch again yesterday and the kernel works without any problems 
> with our onboard SD cards. [1]

Thanks for testing.

cheers

[PASEMI PA6T PPC] Onboard CF card device with new SanDisk High (>8G) CF cards

[Christian Zigotzky]

Hi All,

Which mailing list is responsible for the pata_pcmcia driver? We are 
using new SanDisk High (>8G) CF cards with this driver [1] and we need 
the following line in the file "drivers/ata/pata_pcmcia.c".

+    PCMCIA_DEVICE_MANF_CARD(0x00f1, 0x0101),        /* SanDisk High 
(>8G) CFA */

Thanks,
Christian

[1] https://forum.hyperion-entertainment.com/viewtopic.php?f=35&t=4282

Re: [PASEMI PA6T PPC] Onboard CF card device with new SanDisk High (>8G) CF cards

[Christoph Hellwig]

On Tue, Jan 28, 2020 at 08:58:29AM +0100, Christian Zigotzky wrote:
> Hi All,
> 
> Which mailing list is responsible for the pata_pcmcia driver? We are using
> new SanDisk High (>8G) CF cards with this driver [1] and we need the
> following line in the file "drivers/ata/pata_pcmcia.c".
> 
> +    PCMCIA_DEVICE_MANF_CARD(0x00f1, 0x0101),        /* SanDisk High (>8G)
> CFA */

Please send a formal patch for it to linux-ide@vger.kernel.org

Re: [PASEMI PA6T PPC] Onboard CF card device with new SanDisk High (>8G) CF cards

[Christoph Hellwig]

On Tue, Jan 28, 2020 at 08:58:29AM +0100, Christian Zigotzky wrote:
> Hi All,
> 
> Which mailing list is responsible for the pata_pcmcia driver? We are using
> new SanDisk High (>8G) CF cards with this driver [1] and we need the
> following line in the file "drivers/ata/pata_pcmcia.c".
> 
> +    PCMCIA_DEVICE_MANF_CARD(0x00f1, 0x0101),        /* SanDisk High (>8G)
> CFA */

Please send a formal patch for it to linux-ide@vger.kernel.org

Re: [PASEMI PA6T PPC] Onboard CF card device with new SanDisk High (>8G) CF cards

[Rob Herring]

On Tue, Jan 28, 2020 at 2:01 AM Christian Zigotzky
<chzigotzky@xenosoft.de> wrote:
>
> Hi All,
>
> Which mailing list is responsible for the pata_pcmcia driver? We are
> using new SanDisk High (>8G) CF cards with this driver [1] and we need
> the following line in the file "drivers/ata/pata_pcmcia.c".
>
> +    PCMCIA_DEVICE_MANF_CARD(0x00f1, 0x0101),        /* SanDisk High
> (>8G) CFA */

Run get_maintainers.pl and it will answer that for you:

$ scripts/get_maintainer.pl -f drivers/ata/pata_pcmcia.c
Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
(maintainer:LIBATA PATA DRIVERS)
Jens Axboe <axboe@kernel.dk> (maintainer:LIBATA PATA DRIVERS)
linux-ide@vger.kernel.org (open list:LIBATA PATA DRIVERS)
linux-kernel@vger.kernel.org (open list)

Re: [PASEMI PA6T PPC] Onboard CF card device with new SanDisk High (>8G) CF cards

[Rob Herring]

On Tue, Jan 28, 2020 at 2:01 AM Christian Zigotzky
<chzigotzky@xenosoft.de> wrote:
>
> Hi All,
>
> Which mailing list is responsible for the pata_pcmcia driver? We are
> using new SanDisk High (>8G) CF cards with this driver [1] and we need
> the following line in the file "drivers/ata/pata_pcmcia.c".
>
> +    PCMCIA_DEVICE_MANF_CARD(0x00f1, 0x0101),        /* SanDisk High
> (>8G) CFA */

Run get_maintainers.pl and it will answer that for you:

$ scripts/get_maintainer.pl -f drivers/ata/pata_pcmcia.c
Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
(maintainer:LIBATA PATA DRIVERS)
Jens Axboe <axboe@kernel.dk> (maintainer:LIBATA PATA DRIVERS)
linux-ide@vger.kernel.org (open list:LIBATA PATA DRIVERS)
linux-kernel@vger.kernel.org (open list)

Re: [PASEMI PA6T PPC] Onboard CF card device with new SanDisk High (>8G) CF cards

[Christian Zigotzky]

On 28 January 2020 at 3:16 pm, Rob Herring wrote:
> On Tue, Jan 28, 2020 at 2:01 AM Christian Zigotzky
> <chzigotzky@xenosoft.de> wrote:
>> Hi All,
>>
>> Which mailing list is responsible for the pata_pcmcia driver? We are
>> using new SanDisk High (>8G) CF cards with this driver [1] and we need
>> the following line in the file "drivers/ata/pata_pcmcia.c".
>>
>> +    PCMCIA_DEVICE_MANF_CARD(0x00f1, 0x0101),        /* SanDisk High
>> (>8G) CFA */
> Run get_maintainers.pl and it will answer that for you:
>
> $ scripts/get_maintainer.pl -f drivers/ata/pata_pcmcia.c
> Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> (maintainer:LIBATA PATA DRIVERS)
> Jens Axboe <axboe@kernel.dk> (maintainer:LIBATA PATA DRIVERS)
> linux-ide@vger.kernel.org (open list:LIBATA PATA DRIVERS)
> linux-kernel@vger.kernel.org (open list)
Thank you!

Re: [PASEMI PA6T PPC] Onboard CF card device with new SanDisk High (>8G) CF cards

[Christian Zigotzky]

On 28 January 2020 at 3:16 pm, Rob Herring wrote:
> On Tue, Jan 28, 2020 at 2:01 AM Christian Zigotzky
> <chzigotzky@xenosoft.de> wrote:
>> Hi All,
>>
>> Which mailing list is responsible for the pata_pcmcia driver? We are
>> using new SanDisk High (>8G) CF cards with this driver [1] and we need
>> the following line in the file "drivers/ata/pata_pcmcia.c".
>>
>> +    PCMCIA_DEVICE_MANF_CARD(0x00f1, 0x0101),        /* SanDisk High
>> (>8G) CFA */
> Run get_maintainers.pl and it will answer that for you:
>
> $ scripts/get_maintainer.pl -f drivers/ata/pata_pcmcia.c
> Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> (maintainer:LIBATA PATA DRIVERS)
> Jens Axboe <axboe@kernel.dk> (maintainer:LIBATA PATA DRIVERS)
> linux-ide@vger.kernel.org (open list:LIBATA PATA DRIVERS)
> linux-kernel@vger.kernel.org (open list)
Thank you!

Re: [PATCH 1/2] mmc: sdhci-acpi: Disable 1.8V modes on external microSD on Lenovo Miix 320

[Hans de Goede]

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

Re: [PATCH 1/2] mmc: sdhci-acpi: Disable 1.8V modes on external microSD on Lenovo Miix 320

[Adrian Hunter]

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.");
-- 
2.17.1

Re: [PATCH 1/2] mmc: sdhci-acpi: Disable 1.8V modes on external microSD on Lenovo Miix 320

[Hans de Goede]

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.");
> 

Re: [PATCH 1/2] mmc: sdhci-acpi: Disable 1.8V modes on external microSD on Lenovo Miix 320

[Hans de Goede]

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);
>>>>
>>>
>>
> 

[PATCH 2/2] mmc: sdhci-acpi: Disable write protect detection on Acer Aspire Switch 10 (SW5-012)

[Hans de Goede]

On the Acer Aspire Switch 10 (SW5-012) microSD slot always reports the card
being write-protected even though microSD cards do not have a write-protect
switch at all.

Add a new SDHCI_ACPI_QUIRK_SD_NO_WRITE_PROTECT quirk which when set sets
the MMC_CAP2_NO_WRITE_PROTECT flag on the controller for the external SD
slot, and add a DMI quirk which enables this new quirk by default on the
Acer SW5-012.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/mmc/host/sdhci-acpi.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index 9f150c73e958..69485d29b6bc 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -78,6 +78,7 @@ struct sdhci_acpi_host {
 
 enum {
 	SDHCI_ACPI_QUIRK_SD_NO_1_8V			= BIT(0),
+	SDHCI_ACPI_QUIRK_SD_NO_WRITE_PROTECT		= BIT(1),
 };
 
 static int quirks = -1;
@@ -671,6 +672,18 @@ static const struct dmi_system_id sdhci_acpi_quirks[] = {
 		},
 		.driver_data = (void *)SDHCI_ACPI_QUIRK_SD_NO_1_8V,
 	},
+	{
+		/*
+		 * The Acer Aspire Switch 10 (SW5-012) microSD slot always
+		 * reports the card being write-protected even though microSD
+		 * cards do not have a write-protect switch at all.
+		 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Aspire SW5-012"),
+		},
+		.driver_data = (void *)SDHCI_ACPI_QUIRK_SD_NO_WRITE_PROTECT,
+	},
 	{} /* Terminating entry */
 };
 
@@ -798,6 +811,9 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 
 		if (quirks & SDHCI_ACPI_QUIRK_SD_NO_1_8V)
 			host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
+
+		if (quirks & SDHCI_ACPI_QUIRK_SD_NO_WRITE_PROTECT)
+			host->mmc->caps2 |= MMC_CAP2_NO_WRITE_PROTECT;
 	}
 
 	err = sdhci_setup_host(host);
-- 
2.24.1

Re: [PATCH 0/2] mmc: sdhci-acpi: Introduce device specific quirks, fix issues on 2 device models

[Adrian Hunter]

On 8/01/20 11:39 am, Hans de Goede wrote:
> Hi Adrian,
> 
> I know you have sofar mostly resisted adding device specific (DMI based)
> quirks to the sdhci-acpi.c driver and I agree with you that whenever
> possible, those should be avoided.

Not exactly.  I objected to matching against the string "UNKNOWN" because it
is insufficiently specific.

Matching specific DMI strings to identify board issues is fine.

> 
> But yesterday I was debugging an issue where using the microSD slot
> causes the LCD panel of a tablet to go black. This turns out to be a
> bug in the DSDT which gets triggered when using 1.8V modes, see the first
> patch for details. In this case I really so no other option then disabling
> 1.8V modes and only doing so only on the affected device model.
> 
> Another issue which I had on my TODO list of things to fix is the Acer
> SW5-012 version of the Acer Switch 10 models always reporting the microSD
> as being write-protected. Here too I see no other option then a model
> specific quirk, since some BYT devices may use a normal SD slot with
> actual write-protect capabilities and we do not want to disable
> write-protect checking everywhere just because it is broken on one model.
> The workaround for this is the second patch in this series.
> 
> Regards,
> 
> Hans
>