On 11 June 2018 at 11:50, Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> wrote: > Hi Ulf, > > On 2018-06-11 11:35, Ulf Hansson wrote: >> On 11 June 2018 at 08:48, Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> wrote: >>> dw_mci_exynos_resume_noirq() performs DWMMC register access without >>> ensuring that respective clocks are enabled. This might cause external >>> abort on some systems (observed on Exynos5433 based boards). Fix this >>> by adding needed prepare_enable/disable_unprepare calls. >>> >>> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> >>> --- >>> drivers/mmc/host/dw_mmc-exynos.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c >>> index 3164681108ae..6125b68726b0 100644 >>> --- a/drivers/mmc/host/dw_mmc-exynos.c >>> +++ b/drivers/mmc/host/dw_mmc-exynos.c >>> @@ -193,6 +193,9 @@ static int dw_mci_exynos_resume_noirq(struct device *dev) >>> struct dw_mci_exynos_priv_data *priv = host->priv; >>> u32 clksel; >>> >>> + clk_prepare_enable(host->biu_clk); >>> + clk_prepare_enable(host->ciu_clk); >>> + >>> if (priv->ctrl_type == DW_MCI_TYPE_EXYNOS7 || >>> priv->ctrl_type == DW_MCI_TYPE_EXYNOS7_SMU) >>> clksel = mci_readl(host, CLKSEL64); >>> @@ -207,6 +210,9 @@ static int dw_mci_exynos_resume_noirq(struct device *dev) >>> mci_writel(host, CLKSEL, clksel); >>> } >>> >>> + clk_disable_unprepare(host->biu_clk); >>> + clk_disable_unprepare(host->ciu_clk); >>> + >>> return 0; >>> } >>> #else >> I looked a little closer and I am wondering if it wouldn't be possible >> to use SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() instead of >> SET_SYSTEM_SLEEP_PM_OPS()? >> >> Somelike this: >> SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, >> dw_mci_exynos_resume_noirq) >> >> Then from dw_mci_exynos_resume_noirq() call pm_runtime_force_resume(). >> >> I think it would simplify the code a bit, as you can rely on the >> runtime PM callbacks to deal with clk_prepare_enable() and >> clk_disable_unprepare(), unless I am mistaken. > > This will not fix the problem, because mci_writel() calls in > dw_mci_exynos_resume_noirq are done unconditionally, regardless of the > controller's runtime pm state. Since commit 1d9174fbc55e after calling > pm_runtime_force_resume() there is no guarantee that device is in > runtime active state if it was runtime suspended state. Yes, because the runtime PM usage count is greater than 1. (pm_runtime_get_noresume() is called during probe). If you want to make this explicit (not relying on ->probe()), one can add a ->suspend_noirq() callback and call pm_runtime_get_noresume() in it. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
