[PATCH 1/2] mmc: sdhci-iproc: "mmc1: Internal clock never stabilised." seen on 72113

[PATCH 1/2] mmc: sdhci-iproc: "mmc1: Internal clock never stabilised." seen on 72113

[Al Cooper]

The problem is in the .shutdown callback that was added to the
sdhci-iproc and sdhci-brcmstb drivers to save power in S5. The
shutdown callback will just call the sdhci_pltfm_suspend() function
to suspend the lower level driver and then stop the sdhci system
clock. The problem is that in some cases there can be a worker
thread in the "system_freezable_wq" work queue that is scanning
for a device every second. In normal system suspend, this queue
is suspended before the driver suspend is called. In shutdown the
queue is not suspended and the thread my run after we stop the
sdhci clock in the shutdown callback which will cause the "clock
never stabilised" error. The solution will be to have the shutdown
callback cancel the worker thread before calling suspend (and
stopping the sdhci clock).

NOTE: This is only happening on systems with the Legacy RPi SDIO
core because that's the only controller that doesn't have the
presence signal and needs to use a worker thread to do a 1 second
poll loop.

Fixes: 98b5ce4c08ca ("mmc: sdhci-iproc: Add support for the legacy sdhci controller on the BCM7211")
Signed-off-by: Al Cooper <alcooperx@gmail.com>
---
 drivers/mmc/host/sdhci-iproc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
index 032bf852397f..05db768edcfc 100644
--- a/drivers/mmc/host/sdhci-iproc.c
+++ b/drivers/mmc/host/sdhci-iproc.c
@@ -428,6 +428,10 @@ static int sdhci_iproc_probe(struct platform_device *pdev)
 
 static void sdhci_iproc_shutdown(struct platform_device *pdev)
 {
+	struct sdhci_host *host = platform_get_drvdata(pdev);
+
+	/* Cancel possible rescan worker thread */
+	cancel_delayed_work_sync(&host->mmc->detect);
 	sdhci_pltfm_suspend(&pdev->dev);
 }
 

base-commit: 58e1100fdc5990b0cc0d4beaf2562a92e621ac7d
-- 
2.17.1

[PATCH 2/2] mmc: sdhci-brcmstb: "mmc1: Internal clock never stabilised." seen on 72113

[Al Cooper]

The problem is in the .shutdown callback that was added to the
sdhci-iproc and sdhci-brcmstb drivers to save power in S5. The
shutdown callback will just call the sdhci_pltfm_suspend() function
to suspend the lower level driver and then stop the sdhci system
clock. The problem is that in some cases there can be a worker
thread in the "system_freezable_wq" work queue that is scanning
for a device every second. In normal system suspend, this queue
is suspended before the driver suspend is called. In shutdown the
queue is not suspended and the thread my run after we stop the
sdhci clock in the shutdown callback which will cause the "clock
never stabilised" error. The solution will be to have the shutdown
callback cancel the worker thread before calling suspend (and
stopping the sdhci clock).

NOTE: This is only happening on systems with the Legacy RPi SDIO
core because that's the only controller that doesn't have the
presence signal and needs to use a worker thread to do a 1 second
poll loop.

Fixes: 5b191dcba719 ("mmc: sdhci-brcmstb: Fix mmc timeout errors on S5 suspend")
Signed-off-by: Al Cooper <alcooperx@gmail.com>
---
 drivers/mmc/host/sdhci-brcmstb.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c
index f24623aac2db..11037cd14cfa 100644
--- a/drivers/mmc/host/sdhci-brcmstb.c
+++ b/drivers/mmc/host/sdhci-brcmstb.c
@@ -313,6 +313,10 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
 
 static void sdhci_brcmstb_shutdown(struct platform_device *pdev)
 {
+	struct sdhci_host *host = platform_get_drvdata(pdev);
+
+	/* Cancel possible rescan worker thread */
+	cancel_delayed_work_sync(&host->mmc->detect);
 	sdhci_pltfm_suspend(&pdev->dev);
 }
 
-- 
2.17.1

Re: [PATCH 1/2] mmc: sdhci-iproc: "mmc1: Internal clock never stabilised." seen on 72113

[Florian Fainelli]

On 12/2/21 8:30 AM, Al Cooper wrote:
> The problem is in the .shutdown callback that was added to the
> sdhci-iproc and sdhci-brcmstb drivers to save power in S5. The
> shutdown callback will just call the sdhci_pltfm_suspend() function
> to suspend the lower level driver and then stop the sdhci system
> clock. The problem is that in some cases there can be a worker
> thread in the "system_freezable_wq" work queue that is scanning
> for a device every second. In normal system suspend, this queue
> is suspended before the driver suspend is called. In shutdown the
> queue is not suspended and the thread my run after we stop the
> sdhci clock in the shutdown callback which will cause the "clock
> never stabilised" error. The solution will be to have the shutdown
> callback cancel the worker thread before calling suspend (and
> stopping the sdhci clock).
> 
> NOTE: This is only happening on systems with the Legacy RPi SDIO
> core because that's the only controller that doesn't have the
> presence signal and needs to use a worker thread to do a 1 second
> poll loop.
> 
> Fixes: 98b5ce4c08ca ("mmc: sdhci-iproc: Add support for the legacy sdhci controller on the BCM7211")
> Signed-off-by: Al Cooper <alcooperx@gmail.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH 2/2] mmc: sdhci-brcmstb: "mmc1: Internal clock never stabilised." seen on 72113

[Florian Fainelli]

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

On 12/2/21 8:30 AM, Al Cooper wrote:
> The problem is in the .shutdown callback that was added to the
> sdhci-iproc and sdhci-brcmstb drivers to save power in S5. The
> shutdown callback will just call the sdhci_pltfm_suspend() function
> to suspend the lower level driver and then stop the sdhci system
> clock. The problem is that in some cases there can be a worker
> thread in the "system_freezable_wq" work queue that is scanning
> for a device every second. In normal system suspend, this queue
> is suspended before the driver suspend is called. In shutdown the
> queue is not suspended and the thread my run after we stop the
> sdhci clock in the shutdown callback which will cause the "clock
> never stabilised" error. The solution will be to have the shutdown
> callback cancel the worker thread before calling suspend (and
> stopping the sdhci clock).
> 
> NOTE: This is only happening on systems with the Legacy RPi SDIO
> core because that's the only controller that doesn't have the
> presence signal and needs to use a worker thread to do a 1 second
> poll loop.
> 
> Fixes: 5b191dcba719 ("mmc: sdhci-brcmstb: Fix mmc timeout errors on S5 suspend")
> Signed-off-by: Al Cooper <alcooperx@gmail.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH 1/2] mmc: sdhci-iproc: "mmc1: Internal clock never stabilised." seen on 72113

[Adrian Hunter]

On 02/12/2021 18:30, Al Cooper wrote:
> The problem is in the .shutdown callback that was added to the
> sdhci-iproc and sdhci-brcmstb drivers to save power in S5. The
> shutdown callback will just call the sdhci_pltfm_suspend() function
> to suspend the lower level driver and then stop the sdhci system
> clock. The problem is that in some cases there can be a worker
> thread in the "system_freezable_wq" work queue that is scanning
> for a device every second. In normal system suspend, this queue
> is suspended before the driver suspend is called. In shutdown the
> queue is not suspended and the thread my run after we stop the
> sdhci clock in the shutdown callback which will cause the "clock
> never stabilised" error. The solution will be to have the shutdown
> callback cancel the worker thread before calling suspend (and
> stopping the sdhci clock).
> 
> NOTE: This is only happening on systems with the Legacy RPi SDIO
> core because that's the only controller that doesn't have the
> presence signal and needs to use a worker thread to do a 1 second
> poll loop.
> 
> Fixes: 98b5ce4c08ca ("mmc: sdhci-iproc: Add support for the legacy sdhci controller on the BCM7211")
> Signed-off-by: Al Cooper <alcooperx@gmail.com>
> ---
>  drivers/mmc/host/sdhci-iproc.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
> index 032bf852397f..05db768edcfc 100644
> --- a/drivers/mmc/host/sdhci-iproc.c
> +++ b/drivers/mmc/host/sdhci-iproc.c
> @@ -428,6 +428,10 @@ static int sdhci_iproc_probe(struct platform_device *pdev)
>  
>  static void sdhci_iproc_shutdown(struct platform_device *pdev)
>  {
> +	struct sdhci_host *host = platform_get_drvdata(pdev);
> +
> +	/* Cancel possible rescan worker thread */
> +	cancel_delayed_work_sync(&host->mmc->detect);
>  	sdhci_pltfm_suspend(&pdev->dev);
>  }
>  
> 
> base-commit: 58e1100fdc5990b0cc0d4beaf2562a92e621ac7d
> 

I wonder if mmc core should handle this instead. e.g.

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 240c5af793dc..55c509442659 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2264,15 +2264,21 @@ void mmc_start_host(struct mmc_host *host)
 	_mmc_detect_change(host, 0, false);
 }
 
-void mmc_stop_host(struct mmc_host *host)
+void mmc_quiesce_host(struct mmc_host *host)
 {
-	if (host->slot.cd_irq >= 0) {
+	if (host->slot.cd_irq >= 0 && host->slot.irq_enabled) {
 		mmc_gpio_set_cd_wake(host, false);
 		disable_irq(host->slot.cd_irq);
+		host->slot.irq_enabled = false;
 	}
 
 	host->rescan_disable = 1;
 	cancel_delayed_work_sync(&host->detect);
+}
+
+void mmc_stop_host(struct mmc_host *host)
+{
+	mmc_quiesce_host(host);
 
 	/* clear pm flags now and let card drivers set them as needed */
 	host->pm_flags = 0;
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 7931a4f0137d..dcee28eec9a6 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -71,6 +71,7 @@ static inline void mmc_delay(unsigned int ms)
 void mmc_rescan(struct work_struct *work);
 void mmc_start_host(struct mmc_host *host);
 void mmc_stop_host(struct mmc_host *host);
+void mmc_quiesce_host(struct mmc_host *host);
 
 void _mmc_detect_change(struct mmc_host *host, unsigned long delay,
 			bool cd_irq);
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index d4683b1d263f..5b272f938558 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -80,9 +80,18 @@ static void mmc_host_classdev_release(struct device *dev)
 	kfree(host);
 }
 
+static int mmc_host_shutdown_pre(struct device *dev)
+{
+	struct mmc_host *host = cls_dev_to_mmc_host(dev);
+
+	mmc_quiesce_host(host);
+	return 0;
+}
+
 static struct class mmc_host_class = {
 	.name		= "mmc_host",
 	.dev_release	= mmc_host_classdev_release,
+	.shutdown_pre	= mmc_host_shutdown_pre,
 	.pm		= MMC_HOST_CLASS_DEV_PM_OPS,
 };
 
diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
index dd2a4b6ab6ad..4967f0b15806 100644
--- a/drivers/mmc/core/slot-gpio.c
+++ b/drivers/mmc/core/slot-gpio.c
@@ -110,6 +110,8 @@ void mmc_gpiod_request_cd_irq(struct mmc_host *host)
 			ctx->cd_label, host);
 		if (ret < 0)
 			irq = ret;
+		else
+			host->slot.irq_enabled = true;
 	}
 
 	host->slot.cd_irq = irq;
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 7afb57cab00b..d1fd9b05274b 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -263,6 +263,7 @@ struct mmc_async_req {
 struct mmc_slot {
 	int cd_irq;
 	bool cd_wake_enabled;
+	bool irq_enabled;
 	void *handler_priv;
 };