Commit e7b5d63a82fe ("mmc: sdhci-brcmstb: Add shutdown callback")
that added a shutdown callback to the diver, is causing "mmc timeout"
errors on S5 suspend. The problem was that the "remove" was queuing
additional MMC commands after the "shutdown" and these caused
timeouts as the MMC queues were cleaned up for "remove". The
shutdown callback will be changed to calling sdhci-pltfm_suspend
which should get better power savings because the clocks will be
shutdown.
Fixes: e7b5d63a82fe ("mmc: sdhci-brcmstb: Add shutdown callback")
Signed-off-by: Al Cooper <[email protected]>
---
drivers/mmc/host/sdhci-brcmstb.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c
index bbf3496f4495..f9780c65ebe9 100644
--- a/drivers/mmc/host/sdhci-brcmstb.c
+++ b/drivers/mmc/host/sdhci-brcmstb.c
@@ -314,11 +314,7 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
static void sdhci_brcmstb_shutdown(struct platform_device *pdev)
{
- int ret;
-
- ret = sdhci_pltfm_unregister(pdev);
- if (ret)
- dev_err(&pdev->dev, "failed to shutdown\n");
+ sdhci_pltfm_suspend(&pdev->dev);
}
MODULE_DEVICE_TABLE(of, sdhci_brcm_of_match);
--
2.17.1
On 1/7/21 2:15 PM, Al Cooper wrote:
> Commit e7b5d63a82fe ("mmc: sdhci-brcmstb: Add shutdown callback")
> that added a shutdown callback to the diver, is causing "mmc timeout"
> errors on S5 suspend. The problem was that the "remove" was queuing
> additional MMC commands after the "shutdown" and these caused
> timeouts as the MMC queues were cleaned up for "remove". The
> shutdown callback will be changed to calling sdhci-pltfm_suspend
> which should get better power savings because the clocks will be
> shutdown.
>
> Fixes: e7b5d63a82fe ("mmc: sdhci-brcmstb: Add shutdown callback")
> Signed-off-by: Al Cooper <[email protected]>
Acked-by: Florian Fainelli <[email protected]>
--
Florian
On 8/01/21 12:15 am, Al Cooper wrote:
> Commit e7b5d63a82fe ("mmc: sdhci-brcmstb: Add shutdown callback")
> that added a shutdown callback to the diver, is causing "mmc timeout"
> errors on S5 suspend. The problem was that the "remove" was queuing
> additional MMC commands after the "shutdown" and these caused
> timeouts as the MMC queues were cleaned up for "remove". The
> shutdown callback will be changed to calling sdhci-pltfm_suspend
> which should get better power savings because the clocks will be
> shutdown.
>
> Fixes: e7b5d63a82fe ("mmc: sdhci-brcmstb: Add shutdown callback")
> Signed-off-by: Al Cooper <[email protected]>
Acked-by: Adrian Hunter <[email protected]>
> ---
> drivers/mmc/host/sdhci-brcmstb.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c
> index bbf3496f4495..f9780c65ebe9 100644
> --- a/drivers/mmc/host/sdhci-brcmstb.c
> +++ b/drivers/mmc/host/sdhci-brcmstb.c
> @@ -314,11 +314,7 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
>
> static void sdhci_brcmstb_shutdown(struct platform_device *pdev)
> {
> - int ret;
> -
> - ret = sdhci_pltfm_unregister(pdev);
> - if (ret)
> - dev_err(&pdev->dev, "failed to shutdown\n");
> + sdhci_pltfm_suspend(&pdev->dev);
> }
>
> MODULE_DEVICE_TABLE(of, sdhci_brcm_of_match);
>
On Thu, 7 Jan 2021 at 23:15, Al Cooper <[email protected]> wrote:
>
> Commit e7b5d63a82fe ("mmc: sdhci-brcmstb: Add shutdown callback")
> that added a shutdown callback to the diver, is causing "mmc timeout"
> errors on S5 suspend. The problem was that the "remove" was queuing
> additional MMC commands after the "shutdown" and these caused
> timeouts as the MMC queues were cleaned up for "remove". The
> shutdown callback will be changed to calling sdhci-pltfm_suspend
> which should get better power savings because the clocks will be
> shutdown.
>
> Fixes: e7b5d63a82fe ("mmc: sdhci-brcmstb: Add shutdown callback")
> Signed-off-by: Al Cooper <[email protected]>
Applied for fixes and by adding a stable tag, thanks!
Kind regards
Uffe
> ---
> drivers/mmc/host/sdhci-brcmstb.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c
> index bbf3496f4495..f9780c65ebe9 100644
> --- a/drivers/mmc/host/sdhci-brcmstb.c
> +++ b/drivers/mmc/host/sdhci-brcmstb.c
> @@ -314,11 +314,7 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
>
> static void sdhci_brcmstb_shutdown(struct platform_device *pdev)
> {
> - int ret;
> -
> - ret = sdhci_pltfm_unregister(pdev);
> - if (ret)
> - dev_err(&pdev->dev, "failed to shutdown\n");
> + sdhci_pltfm_suspend(&pdev->dev);
> }
>
> MODULE_DEVICE_TABLE(of, sdhci_brcm_of_match);
> --
> 2.17.1
>
On 07/01/2021 23:15, Al Cooper wrote:
> Commit e7b5d63a82fe ("mmc: sdhci-brcmstb: Add shutdown callback")
> that added a shutdown callback to the diver, is causing "mmc timeout"
> errors on S5 suspend. The problem was that the "remove" was queuing
> additional MMC commands after the "shutdown" and these caused
> timeouts as the MMC queues were cleaned up for "remove". The
> shutdown callback will be changed to calling sdhci-pltfm_suspend
> which should get better power savings because the clocks will be
> shutdown.
>
> Fixes: e7b5d63a82fe ("mmc: sdhci-brcmstb: Add shutdown callback")
> Signed-off-by: Al Cooper <[email protected]>
> ---
> drivers/mmc/host/sdhci-brcmstb.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c
> index bbf3496f4495..f9780c65ebe9 100644
> --- a/drivers/mmc/host/sdhci-brcmstb.c
> +++ b/drivers/mmc/host/sdhci-brcmstb.c
> @@ -314,11 +314,7 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
>
> static void sdhci_brcmstb_shutdown(struct platform_device *pdev)
> {
> - int ret;
> -
> - ret = sdhci_pltfm_unregister(pdev);
> - if (ret)
> - dev_err(&pdev->dev, "failed to shutdown\n");
> + sdhci_pltfm_suspend(&pdev->dev);
> }
>
> MODULE_DEVICE_TABLE(of, sdhci_brcm_of_match);
>
Good morning,
Unfortunately this patch will cause link failures when CONFIG_PM_SLEEP is not
set in the kernel config, as in sdhci-pltfm.c, the implementation of
sdhci_pltfm_suspend() is in between #ifdef CONFIG_PM_SLEEP:
/opt/toolchains/aarch64-musl-1.1.21-gcc-8.3.0-binutils-2.32-gdb-7.12.1-2/bin/aarch64-linux-musl-ld:
drivers/mmc/host/sdhci-brcmstb.o: in function `sdhci_brcmstb_shutdown':
sdhci-brcmstb.c:(.text+0x16c): undefined reference to `sdhci_pltfm_suspend'
sdhci-brcmstb.c:(.text+0x16c): relocation truncated to fit: R_AARCH64_CALL26
against undefined symbol `sdhci_pltfm_suspend'
I'm not sure if definiting sdhci_pltfm_suspend() empty stubs in sdhci-pltfm.h
when CONFIG_PM_SLEEP is not set would be prefered over adding #ifdef
CONFIG_PM_SLEEP in sdhci-brcmstb.c, but I can send a patch for either solution.
Regards,
--
Nicolas Schichan