diff mbox series

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

Message ID 20220421182803.6495-2-kdasu.kdev@gmail.com (mailing list archive)
State New, archived
Headers show
Series mmc: sdhci-brcmstb: host controller clock enhancements | expand

Commit Message

Kamal Dasu April 21, 2022, 6:27 p.m. UTC 
From: Al Cooper <alcooperx@gmail.com>

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>
Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
---
 drivers/mmc/host/sdhci-brcmstb.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Florian Fainelli April 22, 2022, 5:14 p.m. UTC | #1 
On 4/21/22 11:27, Kamal Dasu wrote:
> From: Al Cooper <alcooperx@gmail.com>
> 
> 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>
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Adrian Hunter April 27, 2022, 4:41 a.m. UTC | #2 
On 21/04/22 21:27, Kamal Dasu wrote:
> From: Al Cooper <alcooperx@gmail.com>
> 
> 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>
> Signed-off-by: Kamal Dasu <kdasu.kdev@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);
>  }
>  

I think we fixed that already with the commit below:


commit 66c915d09b942fb3b2b0cb2f56562180901fba17
Author: Ulf Hansson <ulf.hansson@linaro.org>
Date:   Fri Dec 3 15:15:54 2021 +0100

    mmc: core: Disable card detect during shutdown
    
    It's seems prone to problems by allowing card detect and its corresponding
    mmc_rescan() work to run, during platform shutdown. For example, we may end
    up turning off the power while initializing a card, which potentially could
    damage it.
    
    To avoid this scenario, let's add ->shutdown_pre() callback for the mmc host
    class device and then turn of the card detect from there.
    
    Reported-by: Al Cooper <alcooperx@gmail.com>
    Suggested-by: Adrian Hunter <adrian.hunter@intel.com>
    Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
    Cc: stable@vger.kernel.org
    Link: https://lore.kernel.org/r/20211203141555.105351-1-ulf.hansson@linaro.org
diff mbox series

Patch

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