[v3,2/3] mmc: sdhci-acpi: Add blacklist module option

Commit Message

Adrian Hunter April 13, 2017, 11:50 a.m. UTC

On 12/04/17 21:19, Hans de Goede wrote:
> Commit e5bbf30733f9 ("mmc: sdhci-acpi: Ensure connected devices are
> powered when probing") introduced unconditional calling of
> acpi_device_fix_up_power() on the mmc controller and its children.
> 
> This broke wifi on some systems because acpi_device_fix_up_power()
> was called even for disabled children sometimes leaving gpio-s in
> a state where wifi would not work, this was fixed in
> commit e1d070c3793a ("mmc: sdhci-acpi: Only powered up enabled acpi
> child devices").
> 
> Unfortunately on some devices calling acpi_device_fix_up_power()
> still causes issues. Specifically on the GPD-win mini clam-shell PC
> which has a pci-e wifi module, it causes the wifi module to get
> turned off. This is a BIOS bug and I've tried to get the manufacturer
> to fix this but sofar they have not responded (and even if they do
> then we cannot assume all users will update their BIOS).
> 
> Since the GPD-win uses a pci-e wifi module the sdhci controller for
> sdio cards really should not get initialized on it at all.

What if we move the call to acpi_device_fix_up_power() for the child
devices into the ->init_card() callback? i.e. it won't happen if there
is no card:



--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Hans de Goede April 13, 2017, 12:02 p.m. UTC | #1

Hi,

On 13-04-17 13:50, Adrian Hunter wrote:
> On 12/04/17 21:19, Hans de Goede wrote:
>> Commit e5bbf30733f9 ("mmc: sdhci-acpi: Ensure connected devices are
>> powered when probing") introduced unconditional calling of
>> acpi_device_fix_up_power() on the mmc controller and its children.
>>
>> This broke wifi on some systems because acpi_device_fix_up_power()
>> was called even for disabled children sometimes leaving gpio-s in
>> a state where wifi would not work, this was fixed in
>> commit e1d070c3793a ("mmc: sdhci-acpi: Only powered up enabled acpi
>> child devices").
>>
>> Unfortunately on some devices calling acpi_device_fix_up_power()
>> still causes issues. Specifically on the GPD-win mini clam-shell PC
>> which has a pci-e wifi module, it causes the wifi module to get
>> turned off. This is a BIOS bug and I've tried to get the manufacturer
>> to fix this but sofar they have not responded (and even if they do
>> then we cannot assume all users will update their BIOS).
>>
>> Since the GPD-win uses a pci-e wifi module the sdhci controller for
>> sdio cards really should not get initialized on it at all.
>
> What if we move the call to acpi_device_fix_up_power() for the child
> devices into the ->init_card() callback? i.e. it won't happen if there
> is no card:

Interesting idea, but I think that defeats the original purpose of
calling acpi_device_fix_up_power(), AFAIK sometimes without calling
acpi_device_fix_up_power() the sdio-device will not have power, so
it will not get enumerated / seen and init_card() will not get called.

Either that or since the slot is marked as always-present init_card
gets called always and the patch won't help (I've not tried).

The above may not be entirely correct, but I'm pretty sure that
on some systems the card cannot be enumerated without first making
sure it is powered, so we cannot use card-presence detection to
decide if we should call fixup_power or not. At least I've seen
this on ARM systems (where we have the pwrseq stuff to deal with this)
I'm fine with giving you approach a try, maybe on X86 systems the
sdio-card always has enough power to at least enumerate ? But I'm
afraid it may cause regressions.

Regards,

Hans


>
>
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index c6a9a1bfaa22..dd97978f0361 100644
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -81,6 +81,7 @@ struct sdhci_acpi_host {
>  	const struct sdhci_acpi_slot	*slot;
>  	struct platform_device		*pdev;
>  	bool				use_runtime_pm;
> +	bool				child_fixup_power_done;
>  };
>
>  static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)
> @@ -374,11 +375,29 @@ static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(const char *hid,
>  	return NULL;
>  }
>
> +static void sdhci_acpi_init_card(struct mmc_host *mmc, struct mmc_card *card)
> +{
> +	struct device *dev = mmc_dev(mmc);
> +	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
> +	struct acpi_device *child;
> +
> +	if (c->child_fixup_power_done || !adev)
> +		return;
> +
> +	/* Ensure child devices are powered on */
> +	list_for_each_entry(child, &adev->children, node)
> +		if (child->status.present && child->status.enabled)
> +			acpi_device_fix_up_power(child);
> +
> +	c->child_fixup_power_done = true;
> +}
> +
>  static int sdhci_acpi_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	acpi_handle handle = ACPI_HANDLE(dev);
> -	struct acpi_device *device, *child;
> +	struct acpi_device *device;
>  	struct sdhci_acpi_host *c;
>  	struct sdhci_host *host;
>  	struct resource *iomem;
> @@ -390,11 +409,8 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>  	if (acpi_bus_get_device(handle, &device))
>  		return -ENODEV;
>
> -	/* Power on the SDHCI controller and its children */
> +	/* Power on the SDHCI controller */
>  	acpi_device_fix_up_power(device);
> -	list_for_each_entry(child, &device->children, node)
> -		if (child->status.present && child->status.enabled)
> -			acpi_device_fix_up_power(child);
>
>  	if (acpi_bus_get_status(device) || !device->status.present)
>  		return -ENODEV;
> @@ -432,6 +448,8 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>  	host->ops	= &sdhci_acpi_ops_dflt;
>  	host->irq	= platform_get_irq(pdev, 0);
>
> +	host->mmc_host_ops.init_card = sdhci_acpi_init_card;
> +
>  	host->ioaddr = devm_ioremap_nocache(dev, iomem->start,
>  					    resource_size(iomem));
>  	if (host->ioaddr == NULL) {
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index c6a9a1bfaa22..dd97978f0361 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -81,6 +81,7 @@  struct sdhci_acpi_host {
 	const struct sdhci_acpi_slot	*slot;
 	struct platform_device		*pdev;
 	bool				use_runtime_pm;
+	bool				child_fixup_power_done;
 };
 
 static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)
@@ -374,11 +375,29 @@  static const struct sdhci_acpi_slot *sdhci_acpi_get_slot(const char *hid,
 	return NULL;
 }
 
+static void sdhci_acpi_init_card(struct mmc_host *mmc, struct mmc_card *card)
+{
+	struct device *dev = mmc_dev(mmc);
+	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	struct acpi_device *child;
+
+	if (c->child_fixup_power_done || !adev)
+		return;
+
+	/* Ensure child devices are powered on */
+	list_for_each_entry(child, &adev->children, node)
+		if (child->status.present && child->status.enabled)
+			acpi_device_fix_up_power(child);
+
+	c->child_fixup_power_done = true;
+}
+
 static int sdhci_acpi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	acpi_handle handle = ACPI_HANDLE(dev);
-	struct acpi_device *device, *child;
+	struct acpi_device *device;
 	struct sdhci_acpi_host *c;
 	struct sdhci_host *host;
 	struct resource *iomem;
@@ -390,11 +409,8 @@  static int sdhci_acpi_probe(struct platform_device *pdev)
 	if (acpi_bus_get_device(handle, &device))
 		return -ENODEV;
 
-	/* Power on the SDHCI controller and its children */
+	/* Power on the SDHCI controller */
 	acpi_device_fix_up_power(device);
-	list_for_each_entry(child, &device->children, node)
-		if (child->status.present && child->status.enabled)
-			acpi_device_fix_up_power(child);
 
 	if (acpi_bus_get_status(device) || !device->status.present)
 		return -ENODEV;
@@ -432,6 +448,8 @@  static int sdhci_acpi_probe(struct platform_device *pdev)
 	host->ops	= &sdhci_acpi_ops_dflt;
 	host->irq	= platform_get_irq(pdev, 0);
 
+	host->mmc_host_ops.init_card = sdhci_acpi_init_card;
+
 	host->ioaddr = devm_ioremap_nocache(dev, iomem->start,
 					    resource_size(iomem));
 	if (host->ioaddr == NULL) {