| Message ID | 20170616072929.3266-1-quentin.schulz@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] mmc: sdhci-of-at91: factor out clks and presets setting | expand |
On Fri, Jun 16, 2017 at 09:29:28AM +0200, Quentin Schulz wrote: > The setting of clocks and presets is currently done in probe only but > once deep PM support is added, it'll be needed in the resume function. > > Let's create a function for this setting. > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com> Thanks > --- > drivers/mmc/host/sdhci-of-at91.c | 147 ++++++++++++++++++++++----------------- > 1 file changed, 82 insertions(+), 65 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c > index 7611fd679f1a..fb8c6011f13d 100644 > --- a/drivers/mmc/host/sdhci-of-at91.c > +++ b/drivers/mmc/host/sdhci-of-at91.c > @@ -128,6 +128,84 @@ static const struct of_device_id sdhci_at91_dt_match[] = { > }; > MODULE_DEVICE_TABLE(of, sdhci_at91_dt_match); > > +static int sdhci_at91_set_clks_presets(struct device *dev) > +{ > + struct sdhci_host *host = dev_get_drvdata(dev); > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host); > + int ret; > + unsigned int caps0, caps1; > + unsigned int clk_base, clk_mul; > + unsigned int gck_rate, real_gck_rate; > + unsigned int preset_div; > + > + /* > + * The mult clock is provided by as a generated clock by the PMC > + * controller. In order to set the rate of gck, we have to get the > + * base clock rate and the clock mult from capabilities. > + */ > + clk_prepare_enable(priv->hclock); > + caps0 = readl(host->ioaddr + SDHCI_CAPABILITIES); > + caps1 = readl(host->ioaddr + SDHCI_CAPABILITIES_1); > + clk_base = (caps0 & SDHCI_CLOCK_V3_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT; > + clk_mul = (caps1 & SDHCI_CLOCK_MUL_MASK) >> SDHCI_CLOCK_MUL_SHIFT; > + gck_rate = clk_base * 1000000 * (clk_mul + 1); > + ret = clk_set_rate(priv->gck, gck_rate); > + if (ret < 0) { > + dev_err(dev, "failed to set gck"); > + clk_disable_unprepare(priv->hclock); > + return ret; > + } > + /* > + * We need to check if we have the requested rate for gck because in > + * some cases this rate could be not supported. If it happens, the rate > + * is the closest one gck can provide. We have to update the value > + * of clk mul. > + */ > + real_gck_rate = clk_get_rate(priv->gck); > + if (real_gck_rate != gck_rate) { > + clk_mul = real_gck_rate / (clk_base * 1000000) - 1; > + caps1 &= (~SDHCI_CLOCK_MUL_MASK); > + caps1 |= ((clk_mul << SDHCI_CLOCK_MUL_SHIFT) & > + SDHCI_CLOCK_MUL_MASK); > + /* Set capabilities in r/w mode. */ > + writel(SDMMC_CACR_KEY | SDMMC_CACR_CAPWREN, > + host->ioaddr + SDMMC_CACR); > + writel(caps1, host->ioaddr + SDHCI_CAPABILITIES_1); > + /* Set capabilities in ro mode. */ > + writel(0, host->ioaddr + SDMMC_CACR); > + dev_info(dev, "update clk mul to %u as gck rate is %u Hz\n", > + clk_mul, real_gck_rate); > + } > + > + /* > + * We have to set preset values because it depends on the clk_mul > + * value. Moreover, SDR104 is supported in a degraded mode since the > + * maximum sd clock value is 120 MHz instead of 208 MHz. For that > + * reason, we need to use presets to support SDR104. > + */ > + preset_div = DIV_ROUND_UP(real_gck_rate, 24000000) - 1; > + writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, > + host->ioaddr + SDHCI_PRESET_FOR_SDR12); > + preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1; > + writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, > + host->ioaddr + SDHCI_PRESET_FOR_SDR25); > + preset_div = DIV_ROUND_UP(real_gck_rate, 100000000) - 1; > + writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, > + host->ioaddr + SDHCI_PRESET_FOR_SDR50); > + preset_div = DIV_ROUND_UP(real_gck_rate, 120000000) - 1; > + writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, > + host->ioaddr + SDHCI_PRESET_FOR_SDR104); > + preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1; > + writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, > + host->ioaddr + SDHCI_PRESET_FOR_DDR50); > + > + clk_prepare_enable(priv->mainck); > + clk_prepare_enable(priv->gck); > + > + return 0; > +} > + > #ifdef CONFIG_PM > static int sdhci_at91_runtime_suspend(struct device *dev) > { > @@ -192,11 +270,7 @@ static int sdhci_at91_probe(struct platform_device *pdev) > struct sdhci_host *host; > struct sdhci_pltfm_host *pltfm_host; > struct sdhci_at91_priv *priv; > - unsigned int caps0, caps1; > - unsigned int clk_base, clk_mul; > - unsigned int gck_rate, real_gck_rate; > int ret; > - unsigned int preset_div; > > match = of_match_device(sdhci_at91_dt_match, &pdev->dev); > if (!match) > @@ -228,66 +302,9 @@ static int sdhci_at91_probe(struct platform_device *pdev) > return PTR_ERR(priv->gck); > } > > - /* > - * The mult clock is provided by as a generated clock by the PMC > - * controller. In order to set the rate of gck, we have to get the > - * base clock rate and the clock mult from capabilities. > - */ > - clk_prepare_enable(priv->hclock); > - caps0 = readl(host->ioaddr + SDHCI_CAPABILITIES); > - caps1 = readl(host->ioaddr + SDHCI_CAPABILITIES_1); > - clk_base = (caps0 & SDHCI_CLOCK_V3_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT; > - clk_mul = (caps1 & SDHCI_CLOCK_MUL_MASK) >> SDHCI_CLOCK_MUL_SHIFT; > - gck_rate = clk_base * 1000000 * (clk_mul + 1); > - ret = clk_set_rate(priv->gck, gck_rate); > - if (ret < 0) { > - dev_err(&pdev->dev, "failed to set gck"); > - goto hclock_disable_unprepare; > - } > - /* > - * We need to check if we have the requested rate for gck because in > - * some cases this rate could be not supported. If it happens, the rate > - * is the closest one gck can provide. We have to update the value > - * of clk mul. > - */ > - real_gck_rate = clk_get_rate(priv->gck); > - if (real_gck_rate != gck_rate) { > - clk_mul = real_gck_rate / (clk_base * 1000000) - 1; > - caps1 &= (~SDHCI_CLOCK_MUL_MASK); > - caps1 |= ((clk_mul << SDHCI_CLOCK_MUL_SHIFT) & SDHCI_CLOCK_MUL_MASK); > - /* Set capabilities in r/w mode. */ > - writel(SDMMC_CACR_KEY | SDMMC_CACR_CAPWREN, host->ioaddr + SDMMC_CACR); > - writel(caps1, host->ioaddr + SDHCI_CAPABILITIES_1); > - /* Set capabilities in ro mode. */ > - writel(0, host->ioaddr + SDMMC_CACR); > - dev_info(&pdev->dev, "update clk mul to %u as gck rate is %u Hz\n", > - clk_mul, real_gck_rate); > - } > - > - /* > - * We have to set preset values because it depends on the clk_mul > - * value. Moreover, SDR104 is supported in a degraded mode since the > - * maximum sd clock value is 120 MHz instead of 208 MHz. For that > - * reason, we need to use presets to support SDR104. > - */ > - preset_div = DIV_ROUND_UP(real_gck_rate, 24000000) - 1; > - writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, > - host->ioaddr + SDHCI_PRESET_FOR_SDR12); > - preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1; > - writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, > - host->ioaddr + SDHCI_PRESET_FOR_SDR25); > - preset_div = DIV_ROUND_UP(real_gck_rate, 100000000) - 1; > - writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, > - host->ioaddr + SDHCI_PRESET_FOR_SDR50); > - preset_div = DIV_ROUND_UP(real_gck_rate, 120000000) - 1; > - writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, > - host->ioaddr + SDHCI_PRESET_FOR_SDR104); > - preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1; > - writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, > - host->ioaddr + SDHCI_PRESET_FOR_DDR50); > - > - clk_prepare_enable(priv->mainck); > - clk_prepare_enable(priv->gck); > + ret = sdhci_at91_set_clks_presets(&pdev->dev); > + if (ret) > + goto sdhci_pltfm_free; > > ret = mmc_of_parse(host->mmc); > if (ret) > @@ -335,8 +352,8 @@ static int sdhci_at91_probe(struct platform_device *pdev) > clocks_disable_unprepare: > clk_disable_unprepare(priv->gck); > clk_disable_unprepare(priv->mainck); > -hclock_disable_unprepare: > clk_disable_unprepare(priv->hclock); > +sdhci_pltfm_free: > sdhci_pltfm_free(pdev); > return ret; > } > -- > 2.11.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 16, 2017 at 09:29:29AM +0200, Quentin Schulz wrote: > This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2 > SoC's SDHCI controller. > > When resuming from deepest state, it is required to restore preset > registers as the registers are lost since VDD core has been shut down > when entering deepest state on the SAMA5D2. The clocks need to be > reconfigured as well. > > The other registers and init process are taken care of by the SDHCI > core. > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com> > --- > drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++-- > 1 file changed, 32 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c > index fb8c6011f13d..300513fc1068 100644 > --- a/drivers/mmc/host/sdhci-of-at91.c > +++ b/drivers/mmc/host/sdhci-of-at91.c > @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev) > } > > #ifdef CONFIG_PM > +static int sdhci_at91_suspend(struct device *dev) > +{ > + struct sdhci_host *host = dev_get_drvdata(dev); > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host); > + int ret; > + > + ret = sdhci_suspend_host(host); > + > + if (host->runtime_suspended) > + return ret; > + > + clk_disable_unprepare(priv->gck); > + clk_disable_unprepare(priv->hclock); > + clk_disable_unprepare(priv->mainck); > + > + return ret; > +} > + > +static int sdhci_at91_resume(struct device *dev) > +{ > + struct sdhci_host *host = dev_get_drvdata(dev); > + int ret; > + > + ret = sdhci_at91_set_clks_presets(dev); > + if (ret) > + return ret; > + > + return sdhci_resume_host(host); > +} > + > static int sdhci_at91_runtime_suspend(struct device *dev) > { > struct sdhci_host *host = dev_get_drvdata(dev); > @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev) > #endif /* CONFIG_PM */ > > static const struct dev_pm_ops sdhci_at91_dev_pm_ops = { > - SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > - pm_runtime_force_resume) > + SET_SYSTEM_SLEEP_PM_OPS(sdhci_at91_suspend, sdhci_at91_resume) > SET_RUNTIME_PM_OPS(sdhci_at91_runtime_suspend, > sdhci_at91_runtime_resume, > NULL) > -- > 2.11.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16/06/17 10:29, Quentin Schulz wrote: > The setting of clocks and presets is currently done in probe only but > once deep PM support is added, it'll be needed in the resume function. > > Let's create a function for this setting. > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> Apart from cosmetic comment below: Acked-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/mmc/host/sdhci-of-at91.c | 147 ++++++++++++++++++++++----------------- > 1 file changed, 82 insertions(+), 65 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c > index 7611fd679f1a..fb8c6011f13d 100644 > --- a/drivers/mmc/host/sdhci-of-at91.c > +++ b/drivers/mmc/host/sdhci-of-at91.c > @@ -128,6 +128,84 @@ static const struct of_device_id sdhci_at91_dt_match[] = { > }; > MODULE_DEVICE_TABLE(of, sdhci_at91_dt_match); > > +static int sdhci_at91_set_clks_presets(struct device *dev) > +{ > + struct sdhci_host *host = dev_get_drvdata(dev); > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host); > + int ret; > + unsigned int caps0, caps1; > + unsigned int clk_base, clk_mul; > + unsigned int gck_rate, real_gck_rate; > + unsigned int preset_div; Too much whitespace. > + > + /* > + * The mult clock is provided by as a generated clock by the PMC > + * controller. In order to set the rate of gck, we have to get the > + * base clock rate and the clock mult from capabilities. > + */ > + clk_prepare_enable(priv->hclock); > + caps0 = readl(host->ioaddr + SDHCI_CAPABILITIES); > + caps1 = readl(host->ioaddr + SDHCI_CAPABILITIES_1); > + clk_base = (caps0 & SDHCI_CLOCK_V3_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT; > + clk_mul = (caps1 & SDHCI_CLOCK_MUL_MASK) >> SDHCI_CLOCK_MUL_SHIFT; > + gck_rate = clk_base * 1000000 * (clk_mul + 1); > + ret = clk_set_rate(priv->gck, gck_rate); > + if (ret < 0) { > + dev_err(dev, "failed to set gck"); > + clk_disable_unprepare(priv->hclock); > + return ret; > + } > + /* > + * We need to check if we have the requested rate for gck because in > + * some cases this rate could be not supported. If it happens, the rate > + * is the closest one gck can provide. We have to update the value > + * of clk mul. > + */ > + real_gck_rate = clk_get_rate(priv->gck); > + if (real_gck_rate != gck_rate) { > + clk_mul = real_gck_rate / (clk_base * 1000000) - 1; > + caps1 &= (~SDHCI_CLOCK_MUL_MASK); > + caps1 |= ((clk_mul << SDHCI_CLOCK_MUL_SHIFT) & > + SDHCI_CLOCK_MUL_MASK); > + /* Set capabilities in r/w mode. */ > + writel(SDMMC_CACR_KEY | SDMMC_CACR_CAPWREN, > + host->ioaddr + SDMMC_CACR); > + writel(caps1, host->ioaddr + SDHCI_CAPABILITIES_1); > + /* Set capabilities in ro mode. */ > + writel(0, host->ioaddr + SDMMC_CACR); > + dev_info(dev, "update clk mul to %u as gck rate is %u Hz\n", > + clk_mul, real_gck_rate); > + } > + > + /* > + * We have to set preset values because it depends on the clk_mul > + * value. Moreover, SDR104 is supported in a degraded mode since the > + * maximum sd clock value is 120 MHz instead of 208 MHz. For that > + * reason, we need to use presets to support SDR104. > + */ > + preset_div = DIV_ROUND_UP(real_gck_rate, 24000000) - 1; > + writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, > + host->ioaddr + SDHCI_PRESET_FOR_SDR12); > + preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1; > + writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, > + host->ioaddr + SDHCI_PRESET_FOR_SDR25); > + preset_div = DIV_ROUND_UP(real_gck_rate, 100000000) - 1; > + writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, > + host->ioaddr + SDHCI_PRESET_FOR_SDR50); > + preset_div = DIV_ROUND_UP(real_gck_rate, 120000000) - 1; > + writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, > + host->ioaddr + SDHCI_PRESET_FOR_SDR104); > + preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1; > + writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, > + host->ioaddr + SDHCI_PRESET_FOR_DDR50); > + > + clk_prepare_enable(priv->mainck); > + clk_prepare_enable(priv->gck); > + > + return 0; > +} > + > #ifdef CONFIG_PM > static int sdhci_at91_runtime_suspend(struct device *dev) > { > @@ -192,11 +270,7 @@ static int sdhci_at91_probe(struct platform_device *pdev) > struct sdhci_host *host; > struct sdhci_pltfm_host *pltfm_host; > struct sdhci_at91_priv *priv; > - unsigned int caps0, caps1; > - unsigned int clk_base, clk_mul; > - unsigned int gck_rate, real_gck_rate; > int ret; > - unsigned int preset_div; > > match = of_match_device(sdhci_at91_dt_match, &pdev->dev); > if (!match) > @@ -228,66 +302,9 @@ static int sdhci_at91_probe(struct platform_device *pdev) > return PTR_ERR(priv->gck); > } > > - /* > - * The mult clock is provided by as a generated clock by the PMC > - * controller. In order to set the rate of gck, we have to get the > - * base clock rate and the clock mult from capabilities. > - */ > - clk_prepare_enable(priv->hclock); > - caps0 = readl(host->ioaddr + SDHCI_CAPABILITIES); > - caps1 = readl(host->ioaddr + SDHCI_CAPABILITIES_1); > - clk_base = (caps0 & SDHCI_CLOCK_V3_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT; > - clk_mul = (caps1 & SDHCI_CLOCK_MUL_MASK) >> SDHCI_CLOCK_MUL_SHIFT; > - gck_rate = clk_base * 1000000 * (clk_mul + 1); > - ret = clk_set_rate(priv->gck, gck_rate); > - if (ret < 0) { > - dev_err(&pdev->dev, "failed to set gck"); > - goto hclock_disable_unprepare; > - } > - /* > - * We need to check if we have the requested rate for gck because in > - * some cases this rate could be not supported. If it happens, the rate > - * is the closest one gck can provide. We have to update the value > - * of clk mul. > - */ > - real_gck_rate = clk_get_rate(priv->gck); > - if (real_gck_rate != gck_rate) { > - clk_mul = real_gck_rate / (clk_base * 1000000) - 1; > - caps1 &= (~SDHCI_CLOCK_MUL_MASK); > - caps1 |= ((clk_mul << SDHCI_CLOCK_MUL_SHIFT) & SDHCI_CLOCK_MUL_MASK); > - /* Set capabilities in r/w mode. */ > - writel(SDMMC_CACR_KEY | SDMMC_CACR_CAPWREN, host->ioaddr + SDMMC_CACR); > - writel(caps1, host->ioaddr + SDHCI_CAPABILITIES_1); > - /* Set capabilities in ro mode. */ > - writel(0, host->ioaddr + SDMMC_CACR); > - dev_info(&pdev->dev, "update clk mul to %u as gck rate is %u Hz\n", > - clk_mul, real_gck_rate); > - } > - > - /* > - * We have to set preset values because it depends on the clk_mul > - * value. Moreover, SDR104 is supported in a degraded mode since the > - * maximum sd clock value is 120 MHz instead of 208 MHz. For that > - * reason, we need to use presets to support SDR104. > - */ > - preset_div = DIV_ROUND_UP(real_gck_rate, 24000000) - 1; > - writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, > - host->ioaddr + SDHCI_PRESET_FOR_SDR12); > - preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1; > - writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, > - host->ioaddr + SDHCI_PRESET_FOR_SDR25); > - preset_div = DIV_ROUND_UP(real_gck_rate, 100000000) - 1; > - writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, > - host->ioaddr + SDHCI_PRESET_FOR_SDR50); > - preset_div = DIV_ROUND_UP(real_gck_rate, 120000000) - 1; > - writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, > - host->ioaddr + SDHCI_PRESET_FOR_SDR104); > - preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1; > - writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, > - host->ioaddr + SDHCI_PRESET_FOR_DDR50); > - > - clk_prepare_enable(priv->mainck); > - clk_prepare_enable(priv->gck); > + ret = sdhci_at91_set_clks_presets(&pdev->dev); > + if (ret) > + goto sdhci_pltfm_free; > > ret = mmc_of_parse(host->mmc); > if (ret) > @@ -335,8 +352,8 @@ static int sdhci_at91_probe(struct platform_device *pdev) > clocks_disable_unprepare: > clk_disable_unprepare(priv->gck); > clk_disable_unprepare(priv->mainck); > -hclock_disable_unprepare: > clk_disable_unprepare(priv->hclock); > +sdhci_pltfm_free: > sdhci_pltfm_free(pdev); > return ret; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16/06/17 10:29, Quentin Schulz wrote: > This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2 > SoC's SDHCI controller. > > When resuming from deepest state, it is required to restore preset > registers as the registers are lost since VDD core has been shut down > when entering deepest state on the SAMA5D2. The clocks need to be > reconfigured as well. > > The other registers and init process are taken care of by the SDHCI > core. > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > --- > drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++-- > 1 file changed, 32 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c > index fb8c6011f13d..300513fc1068 100644 > --- a/drivers/mmc/host/sdhci-of-at91.c > +++ b/drivers/mmc/host/sdhci-of-at91.c > @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev) > } > > #ifdef CONFIG_PM Should be CONFIG_PM_SLEEP for suspend / resume callbacks. > +static int sdhci_at91_suspend(struct device *dev) > +{ > + struct sdhci_host *host = dev_get_drvdata(dev); > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host); > + int ret; > + > + ret = sdhci_suspend_host(host); > + > + if (host->runtime_suspended) > + return ret; Suspending while runtime suspended seems like a bad idea. Have you considered just adding sdhci_at91_set_clks_presets() to sdhci_at91_runtime_resume()? > + > + clk_disable_unprepare(priv->gck); > + clk_disable_unprepare(priv->hclock); > + clk_disable_unprepare(priv->mainck); > + > + return ret; > +} > + > +static int sdhci_at91_resume(struct device *dev) > +{ > + struct sdhci_host *host = dev_get_drvdata(dev); > + int ret; > + > + ret = sdhci_at91_set_clks_presets(dev); > + if (ret) > + return ret; > + > + return sdhci_resume_host(host); > +} > + > static int sdhci_at91_runtime_suspend(struct device *dev) > { > struct sdhci_host *host = dev_get_drvdata(dev); > @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev) > #endif /* CONFIG_PM */ > > static const struct dev_pm_ops sdhci_at91_dev_pm_ops = { > - SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > - pm_runtime_force_resume) > + SET_SYSTEM_SLEEP_PM_OPS(sdhci_at91_suspend, sdhci_at91_resume) > SET_RUNTIME_PM_OPS(sdhci_at91_runtime_suspend, > sdhci_at91_runtime_resume, > NULL) > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Adrian, On 20/06/2017 09:39, Adrian Hunter wrote: > On 16/06/17 10:29, Quentin Schulz wrote: >> This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2 >> SoC's SDHCI controller. >> >> When resuming from deepest state, it is required to restore preset >> registers as the registers are lost since VDD core has been shut down >> when entering deepest state on the SAMA5D2. The clocks need to be >> reconfigured as well. >> >> The other registers and init process are taken care of by the SDHCI >> core. >> >> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> >> --- >> drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++-- >> 1 file changed, 32 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c >> index fb8c6011f13d..300513fc1068 100644 >> --- a/drivers/mmc/host/sdhci-of-at91.c >> +++ b/drivers/mmc/host/sdhci-of-at91.c >> @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev) >> } >> >> #ifdef CONFIG_PM > > Should be CONFIG_PM_SLEEP for suspend / resume callbacks. > So I let this CONFIG_PM around the runtime_suspend/resume but put another CONFIG_PM_SLEEP around the suspend/resume functions? >> +static int sdhci_at91_suspend(struct device *dev) >> +{ >> + struct sdhci_host *host = dev_get_drvdata(dev); >> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> + struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host); >> + int ret; >> + >> + ret = sdhci_suspend_host(host); >> + >> + if (host->runtime_suspended) >> + return ret; > > Suspending while runtime suspended seems like a bad idea. Have you > considered just adding sdhci_at91_set_clks_presets() to > sdhci_at91_runtime_resume()? > Adding sdhci_at91_set_clks_presets() to runtime_resume() seems a bad idea as well. You don't need to recompute the clock rate, set it and set the presets registers each time you do a runtime_resume. As the runtime_pm of sdhci has a quite aggressive policy of activation, this seems like a bad idea on the optimization side. Thanks, Quentin >> + >> + clk_disable_unprepare(priv->gck); >> + clk_disable_unprepare(priv->hclock); >> + clk_disable_unprepare(priv->mainck); >> + >> + return ret; >> +} >> + >> +static int sdhci_at91_resume(struct device *dev) >> +{ >> + struct sdhci_host *host = dev_get_drvdata(dev); >> + int ret; >> + >> + ret = sdhci_at91_set_clks_presets(dev); >> + if (ret) >> + return ret; >> + >> + return sdhci_resume_host(host); >> +} >> + >> static int sdhci_at91_runtime_suspend(struct device *dev) >> { >> struct sdhci_host *host = dev_get_drvdata(dev); >> @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev) >> #endif /* CONFIG_PM */ >> >> static const struct dev_pm_ops sdhci_at91_dev_pm_ops = { >> - SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, >> - pm_runtime_force_resume) >> + SET_SYSTEM_SLEEP_PM_OPS(sdhci_at91_suspend, sdhci_at91_resume) >> SET_RUNTIME_PM_OPS(sdhci_at91_runtime_suspend, >> sdhci_at91_runtime_resume, >> NULL) >> > -- Quentin Schulz, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Adrian, On 20/06/2017 08:36, Adrian Hunter wrote: > On 16/06/17 10:29, Quentin Schulz wrote: >> The setting of clocks and presets is currently done in probe only but >> once deep PM support is added, it'll be needed in the resume function. >> >> Let's create a function for this setting. >> >> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > > Apart from cosmetic comment below: > > Acked-by: Adrian Hunter <adrian.hunter@intel.com> > >> --- >> drivers/mmc/host/sdhci-of-at91.c | 147 ++++++++++++++++++++++----------------- >> 1 file changed, 82 insertions(+), 65 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c >> index 7611fd679f1a..fb8c6011f13d 100644 >> --- a/drivers/mmc/host/sdhci-of-at91.c >> +++ b/drivers/mmc/host/sdhci-of-at91.c >> @@ -128,6 +128,84 @@ static const struct of_device_id sdhci_at91_dt_match[] = { >> }; >> MODULE_DEVICE_TABLE(of, sdhci_at91_dt_match); >> >> +static int sdhci_at91_set_clks_presets(struct device *dev) >> +{ >> + struct sdhci_host *host = dev_get_drvdata(dev); >> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> + struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host); >> + int ret; >> + unsigned int caps0, caps1; >> + unsigned int clk_base, clk_mul; >> + unsigned int gck_rate, real_gck_rate; >> + unsigned int preset_div; > > Too much whitespace. > Simply moved some variables from the original code (see sdhci_at91_probe below). Thanks, Quentin >> + >> + /* >> + * The mult clock is provided by as a generated clock by the PMC >> + * controller. In order to set the rate of gck, we have to get the >> + * base clock rate and the clock mult from capabilities. >> + */ >> + clk_prepare_enable(priv->hclock); >> + caps0 = readl(host->ioaddr + SDHCI_CAPABILITIES); >> + caps1 = readl(host->ioaddr + SDHCI_CAPABILITIES_1); >> + clk_base = (caps0 & SDHCI_CLOCK_V3_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT; >> + clk_mul = (caps1 & SDHCI_CLOCK_MUL_MASK) >> SDHCI_CLOCK_MUL_SHIFT; >> + gck_rate = clk_base * 1000000 * (clk_mul + 1); >> + ret = clk_set_rate(priv->gck, gck_rate); >> + if (ret < 0) { >> + dev_err(dev, "failed to set gck"); >> + clk_disable_unprepare(priv->hclock); >> + return ret; >> + } >> + /* >> + * We need to check if we have the requested rate for gck because in >> + * some cases this rate could be not supported. If it happens, the rate >> + * is the closest one gck can provide. We have to update the value >> + * of clk mul. >> + */ >> + real_gck_rate = clk_get_rate(priv->gck); >> + if (real_gck_rate != gck_rate) { >> + clk_mul = real_gck_rate / (clk_base * 1000000) - 1; >> + caps1 &= (~SDHCI_CLOCK_MUL_MASK); >> + caps1 |= ((clk_mul << SDHCI_CLOCK_MUL_SHIFT) & >> + SDHCI_CLOCK_MUL_MASK); >> + /* Set capabilities in r/w mode. */ >> + writel(SDMMC_CACR_KEY | SDMMC_CACR_CAPWREN, >> + host->ioaddr + SDMMC_CACR); >> + writel(caps1, host->ioaddr + SDHCI_CAPABILITIES_1); >> + /* Set capabilities in ro mode. */ >> + writel(0, host->ioaddr + SDMMC_CACR); >> + dev_info(dev, "update clk mul to %u as gck rate is %u Hz\n", >> + clk_mul, real_gck_rate); >> + } >> + >> + /* >> + * We have to set preset values because it depends on the clk_mul >> + * value. Moreover, SDR104 is supported in a degraded mode since the >> + * maximum sd clock value is 120 MHz instead of 208 MHz. For that >> + * reason, we need to use presets to support SDR104. >> + */ >> + preset_div = DIV_ROUND_UP(real_gck_rate, 24000000) - 1; >> + writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, >> + host->ioaddr + SDHCI_PRESET_FOR_SDR12); >> + preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1; >> + writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, >> + host->ioaddr + SDHCI_PRESET_FOR_SDR25); >> + preset_div = DIV_ROUND_UP(real_gck_rate, 100000000) - 1; >> + writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, >> + host->ioaddr + SDHCI_PRESET_FOR_SDR50); >> + preset_div = DIV_ROUND_UP(real_gck_rate, 120000000) - 1; >> + writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, >> + host->ioaddr + SDHCI_PRESET_FOR_SDR104); >> + preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1; >> + writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, >> + host->ioaddr + SDHCI_PRESET_FOR_DDR50); >> + >> + clk_prepare_enable(priv->mainck); >> + clk_prepare_enable(priv->gck); >> + >> + return 0; >> +} >> + >> #ifdef CONFIG_PM >> static int sdhci_at91_runtime_suspend(struct device *dev) >> { >> @@ -192,11 +270,7 @@ static int sdhci_at91_probe(struct platform_device *pdev) >> struct sdhci_host *host; >> struct sdhci_pltfm_host *pltfm_host; >> struct sdhci_at91_priv *priv; >> - unsigned int caps0, caps1; >> - unsigned int clk_base, clk_mul; >> - unsigned int gck_rate, real_gck_rate; >> int ret; >> - unsigned int preset_div; >> >> match = of_match_device(sdhci_at91_dt_match, &pdev->dev); >> if (!match) >> @@ -228,66 +302,9 @@ static int sdhci_at91_probe(struct platform_device *pdev) >> return PTR_ERR(priv->gck); >> } >> >> - /* >> - * The mult clock is provided by as a generated clock by the PMC >> - * controller. In order to set the rate of gck, we have to get the >> - * base clock rate and the clock mult from capabilities. >> - */ >> - clk_prepare_enable(priv->hclock); >> - caps0 = readl(host->ioaddr + SDHCI_CAPABILITIES); >> - caps1 = readl(host->ioaddr + SDHCI_CAPABILITIES_1); >> - clk_base = (caps0 & SDHCI_CLOCK_V3_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT; >> - clk_mul = (caps1 & SDHCI_CLOCK_MUL_MASK) >> SDHCI_CLOCK_MUL_SHIFT; >> - gck_rate = clk_base * 1000000 * (clk_mul + 1); >> - ret = clk_set_rate(priv->gck, gck_rate); >> - if (ret < 0) { >> - dev_err(&pdev->dev, "failed to set gck"); >> - goto hclock_disable_unprepare; >> - } >> - /* >> - * We need to check if we have the requested rate for gck because in >> - * some cases this rate could be not supported. If it happens, the rate >> - * is the closest one gck can provide. We have to update the value >> - * of clk mul. >> - */ >> - real_gck_rate = clk_get_rate(priv->gck); >> - if (real_gck_rate != gck_rate) { >> - clk_mul = real_gck_rate / (clk_base * 1000000) - 1; >> - caps1 &= (~SDHCI_CLOCK_MUL_MASK); >> - caps1 |= ((clk_mul << SDHCI_CLOCK_MUL_SHIFT) & SDHCI_CLOCK_MUL_MASK); >> - /* Set capabilities in r/w mode. */ >> - writel(SDMMC_CACR_KEY | SDMMC_CACR_CAPWREN, host->ioaddr + SDMMC_CACR); >> - writel(caps1, host->ioaddr + SDHCI_CAPABILITIES_1); >> - /* Set capabilities in ro mode. */ >> - writel(0, host->ioaddr + SDMMC_CACR); >> - dev_info(&pdev->dev, "update clk mul to %u as gck rate is %u Hz\n", >> - clk_mul, real_gck_rate); >> - } >> - >> - /* >> - * We have to set preset values because it depends on the clk_mul >> - * value. Moreover, SDR104 is supported in a degraded mode since the >> - * maximum sd clock value is 120 MHz instead of 208 MHz. For that >> - * reason, we need to use presets to support SDR104. >> - */ >> - preset_div = DIV_ROUND_UP(real_gck_rate, 24000000) - 1; >> - writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, >> - host->ioaddr + SDHCI_PRESET_FOR_SDR12); >> - preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1; >> - writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, >> - host->ioaddr + SDHCI_PRESET_FOR_SDR25); >> - preset_div = DIV_ROUND_UP(real_gck_rate, 100000000) - 1; >> - writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, >> - host->ioaddr + SDHCI_PRESET_FOR_SDR50); >> - preset_div = DIV_ROUND_UP(real_gck_rate, 120000000) - 1; >> - writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, >> - host->ioaddr + SDHCI_PRESET_FOR_SDR104); >> - preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1; >> - writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, >> - host->ioaddr + SDHCI_PRESET_FOR_DDR50); >> - >> - clk_prepare_enable(priv->mainck); >> - clk_prepare_enable(priv->gck); >> + ret = sdhci_at91_set_clks_presets(&pdev->dev); >> + if (ret) >> + goto sdhci_pltfm_free; >> >> ret = mmc_of_parse(host->mmc); >> if (ret) >> @@ -335,8 +352,8 @@ static int sdhci_at91_probe(struct platform_device *pdev) >> clocks_disable_unprepare: >> clk_disable_unprepare(priv->gck); >> clk_disable_unprepare(priv->mainck); >> -hclock_disable_unprepare: >> clk_disable_unprepare(priv->hclock); >> +sdhci_pltfm_free: >> sdhci_pltfm_free(pdev); >> return ret; >> } >> > -- Quentin Schulz, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Adrian and Ludovic, On 20/06/2017 11:49, Ludovic Desroches wrote: > On Tue, Jun 20, 2017 at 10:07:06AM +0200, Quentin Schulz wrote: >> Hi Adrian, >> >> On 20/06/2017 09:39, Adrian Hunter wrote: >>> On 16/06/17 10:29, Quentin Schulz wrote: >>>> This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2 >>>> SoC's SDHCI controller. >>>> >>>> When resuming from deepest state, it is required to restore preset >>>> registers as the registers are lost since VDD core has been shut down >>>> when entering deepest state on the SAMA5D2. The clocks need to be >>>> reconfigured as well. >>>> >>>> The other registers and init process are taken care of by the SDHCI >>>> core. >>>> >>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> >>>> --- >>>> drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++-- >>>> 1 file changed, 32 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c >>>> index fb8c6011f13d..300513fc1068 100644 >>>> --- a/drivers/mmc/host/sdhci-of-at91.c >>>> +++ b/drivers/mmc/host/sdhci-of-at91.c >>>> @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev) >>>> } >>>> >>>> #ifdef CONFIG_PM >>> >>> Should be CONFIG_PM_SLEEP for suspend / resume callbacks. >>> >> >> So I let this CONFIG_PM around the runtime_suspend/resume but put >> another CONFIG_PM_SLEEP around the suspend/resume functions? >> >>>> +static int sdhci_at91_suspend(struct device *dev) >>>> +{ >>>> + struct sdhci_host *host = dev_get_drvdata(dev); >>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>> + struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host); >>>> + int ret; >>>> + >>>> + ret = sdhci_suspend_host(host); >>>> + >>>> + if (host->runtime_suspended) >>>> + return ret; >>> >>> Suspending while runtime suspended seems like a bad idea. Have you >>> considered just adding sdhci_at91_set_clks_presets() to >>> sdhci_at91_runtime_resume()? >>> >> >> Adding sdhci_at91_set_clks_presets() to runtime_resume() seems a bad >> idea as well. You don't need to recompute the clock rate, set it and set >> the presets registers each time you do a runtime_resume. As the >> runtime_pm of sdhci has a quite aggressive policy of activation, this >> seems like a bad idea on the optimization side. > > So maybe increment/decrement the device's usage counter. It should be > safer. > From what I've understood from the runtime_pm documentation[1], it seems that there is no need in my case to test if the system has been runtime suspended before being suspended. So I think we can safely remove the test and leave the rest as is. My understanding is the following: If the system is not runtime suspended before doing suspend, then it just does suspend and then resume. => enable and disable clocks are called once each so it is balanced. If the system is already runtime suspended when suspending, the resume will be called and once the device will be used, the runtime resume will be called. => enable and disable clocks are called twice each (once in runtime and system suspend/resume) so it is balanced. A few quick tests on my sama5d2_xplained seem to be validating those hypothesis. Do we agree on removing the `if (host->runtime_suspended)`? Thanks, Quentin > Ludovic > >> >> Thanks, >> Quentin >> >>>> + >>>> + clk_disable_unprepare(priv->gck); >>>> + clk_disable_unprepare(priv->hclock); >>>> + clk_disable_unprepare(priv->mainck); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int sdhci_at91_resume(struct device *dev) >>>> +{ >>>> + struct sdhci_host *host = dev_get_drvdata(dev); >>>> + int ret; >>>> + >>>> + ret = sdhci_at91_set_clks_presets(dev); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + return sdhci_resume_host(host); >>>> +} >>>> + >>>> static int sdhci_at91_runtime_suspend(struct device *dev) >>>> { >>>> struct sdhci_host *host = dev_get_drvdata(dev); >>>> @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev) >>>> #endif /* CONFIG_PM */ >>>> >>>> static const struct dev_pm_ops sdhci_at91_dev_pm_ops = { >>>> - SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, >>>> - pm_runtime_force_resume) >>>> + SET_SYSTEM_SLEEP_PM_OPS(sdhci_at91_suspend, sdhci_at91_resume) >>>> SET_RUNTIME_PM_OPS(sdhci_at91_runtime_suspend, >>>> sdhci_at91_runtime_resume, >>>> NULL) >>>> >>> >> >> -- >> Quentin Schulz, Free Electrons >> Embedded Linux and Kernel engineering >> http://free-electrons.com -- Quentin Schulz, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Better with the link. On 05/07/2017 08:23, Quentin Schulz wrote: > Hi Adrian and Ludovic, > > On 20/06/2017 11:49, Ludovic Desroches wrote: >> On Tue, Jun 20, 2017 at 10:07:06AM +0200, Quentin Schulz wrote: >>> Hi Adrian, >>> >>> On 20/06/2017 09:39, Adrian Hunter wrote: >>>> On 16/06/17 10:29, Quentin Schulz wrote: >>>>> This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2 >>>>> SoC's SDHCI controller. >>>>> >>>>> When resuming from deepest state, it is required to restore preset >>>>> registers as the registers are lost since VDD core has been shut down >>>>> when entering deepest state on the SAMA5D2. The clocks need to be >>>>> reconfigured as well. >>>>> >>>>> The other registers and init process are taken care of by the SDHCI >>>>> core. >>>>> >>>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> >>>>> --- >>>>> drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++-- >>>>> 1 file changed, 32 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c >>>>> index fb8c6011f13d..300513fc1068 100644 >>>>> --- a/drivers/mmc/host/sdhci-of-at91.c >>>>> +++ b/drivers/mmc/host/sdhci-of-at91.c >>>>> @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev) >>>>> } >>>>> >>>>> #ifdef CONFIG_PM >>>> >>>> Should be CONFIG_PM_SLEEP for suspend / resume callbacks. >>>> >>> >>> So I let this CONFIG_PM around the runtime_suspend/resume but put >>> another CONFIG_PM_SLEEP around the suspend/resume functions? >>> >>>>> +static int sdhci_at91_suspend(struct device *dev) >>>>> +{ >>>>> + struct sdhci_host *host = dev_get_drvdata(dev); >>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>>> + struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host); >>>>> + int ret; >>>>> + >>>>> + ret = sdhci_suspend_host(host); >>>>> + >>>>> + if (host->runtime_suspended) >>>>> + return ret; >>>> >>>> Suspending while runtime suspended seems like a bad idea. Have you >>>> considered just adding sdhci_at91_set_clks_presets() to >>>> sdhci_at91_runtime_resume()? >>>> >>> >>> Adding sdhci_at91_set_clks_presets() to runtime_resume() seems a bad >>> idea as well. You don't need to recompute the clock rate, set it and set >>> the presets registers each time you do a runtime_resume. As the >>> runtime_pm of sdhci has a quite aggressive policy of activation, this >>> seems like a bad idea on the optimization side. >> >> So maybe increment/decrement the device's usage counter. It should be >> safer. >> > > From what I've understood from the runtime_pm documentation[1], it seems > that there is no need in my case to test if the system has been runtime > suspended before being suspended. So I think we can safely remove the > test and leave the rest as is. > > My understanding is the following: > If the system is not runtime suspended before doing suspend, then it > just does suspend and then resume. > => enable and disable clocks are called once each so it is balanced. > > If the system is already runtime suspended when suspending, the resume > will be called and once the device will be used, the runtime resume will > be called. > => enable and disable clocks are called twice each (once in runtime and > system suspend/resume) so it is balanced. > > A few quick tests on my sama5d2_xplained seem to be validating those > hypothesis. > > Do we agree on removing the `if (host->runtime_suspended)`? > [1] http://elixir.free-electrons.com/linux/latest/source/Documentation/power/runtime_pm.txt#L613 > Thanks, > Quentin > >> Ludovic >> >>> >>> Thanks, >>> Quentin >>> >>>>> + >>>>> + clk_disable_unprepare(priv->gck); >>>>> + clk_disable_unprepare(priv->hclock); >>>>> + clk_disable_unprepare(priv->mainck); >>>>> + >>>>> + return ret; >>>>> +} >>>>> + >>>>> +static int sdhci_at91_resume(struct device *dev) >>>>> +{ >>>>> + struct sdhci_host *host = dev_get_drvdata(dev); >>>>> + int ret; >>>>> + >>>>> + ret = sdhci_at91_set_clks_presets(dev); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + return sdhci_resume_host(host); >>>>> +} >>>>> + >>>>> static int sdhci_at91_runtime_suspend(struct device *dev) >>>>> { >>>>> struct sdhci_host *host = dev_get_drvdata(dev); >>>>> @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev) >>>>> #endif /* CONFIG_PM */ >>>>> >>>>> static const struct dev_pm_ops sdhci_at91_dev_pm_ops = { >>>>> - SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, >>>>> - pm_runtime_force_resume) >>>>> + SET_SYSTEM_SLEEP_PM_OPS(sdhci_at91_suspend, sdhci_at91_resume) >>>>> SET_RUNTIME_PM_OPS(sdhci_at91_runtime_suspend, >>>>> sdhci_at91_runtime_resume, >>>>> NULL) >>>>> >>>> >>> >>> -- >>> Quentin Schulz, Free Electrons >>> Embedded Linux and Kernel engineering >>> http://free-electrons.com > -- Quentin Schulz, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16 June 2017 at 09:29, Quentin Schulz <quentin.schulz@free-electrons.com> wrote: > This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2 > SoC's SDHCI controller. > > When resuming from deepest state, it is required to restore preset > registers as the registers are lost since VDD core has been shut down > when entering deepest state on the SAMA5D2. The clocks need to be > reconfigured as well. Right, so compared to runtime resume there is some additional operations that is needed during system resume. Fair enough. However by looking at the changes below, you also change the system suspend operations, as it now calls sdhci_suspend_host(). Is that change needed? Then why? > > The other registers and init process are taken care of by the SDHCI > core. > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > --- > drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++-- > 1 file changed, 32 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c > index fb8c6011f13d..300513fc1068 100644 > --- a/drivers/mmc/host/sdhci-of-at91.c > +++ b/drivers/mmc/host/sdhci-of-at91.c > @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev) > } > > #ifdef CONFIG_PM > +static int sdhci_at91_suspend(struct device *dev) > +{ > + struct sdhci_host *host = dev_get_drvdata(dev); > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host); > + int ret; > + > + ret = sdhci_suspend_host(host); > + This is wrong, you can't call sdhci_suspend_host() unless the device is runtime resumed... > + if (host->runtime_suspended) > + return ret; ... and this is weird... > + > + clk_disable_unprepare(priv->gck); > + clk_disable_unprepare(priv->hclock); > + clk_disable_unprepare(priv->mainck); > + > + return ret; > +} > + > +static int sdhci_at91_resume(struct device *dev) > +{ > + struct sdhci_host *host = dev_get_drvdata(dev); > + int ret; > + > + ret = sdhci_at91_set_clks_presets(dev); > + if (ret) > + return ret; Instead of doing it like this, I suggest you set a new flag to true here, let's call it "restore_needed". In the ->runtime_resume() callback, you check the restore_needed flag and performs the extra operations in that case. When that's done, the ->runtime_resume() callback clears the flag, as to avoid the next runtime resume from unnecessary doing the extra operations. > + > + return sdhci_resume_host(host); Remove this and call pm_runtime_force_resume(). > +} > + > static int sdhci_at91_runtime_suspend(struct device *dev) > { > struct sdhci_host *host = dev_get_drvdata(dev); > @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev) > #endif /* CONFIG_PM */ > > static const struct dev_pm_ops sdhci_at91_dev_pm_ops = { > - SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, Leave the pm_runtime_force_suspend() here, unless you have other reasons not being described in the change log, to change the system suspend operations. > - pm_runtime_force_resume) > + SET_SYSTEM_SLEEP_PM_OPS(sdhci_at91_suspend, sdhci_at91_resume) > SET_RUNTIME_PM_OPS(sdhci_at91_runtime_suspend, > sdhci_at91_runtime_resume, > NULL) > -- > 2.11.0 > Adopting my changes should simplify the code, avoid unnecessary resuming the device but instead deferring that until really needed via runtime PM. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 11, 2017 at 02:42:44PM +0200, Ulf Hansson wrote: > On 16 June 2017 at 09:29, Quentin Schulz > <quentin.schulz@free-electrons.com> wrote: > > This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2 > > SoC's SDHCI controller. > > > > When resuming from deepest state, it is required to restore preset > > registers as the registers are lost since VDD core has been shut down > > when entering deepest state on the SAMA5D2. The clocks need to be > > reconfigured as well. > > Right, so compared to runtime resume there is some additional > operations that is needed during system resume. Fair enough. > > However by looking at the changes below, you also change the system > suspend operations, as it now calls sdhci_suspend_host(). Is that > change needed? Then why? > > > > > The other registers and init process are taken care of by the SDHCI > > core. > > > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > > --- > > drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++-- > > 1 file changed, 32 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c > > index fb8c6011f13d..300513fc1068 100644 > > --- a/drivers/mmc/host/sdhci-of-at91.c > > +++ b/drivers/mmc/host/sdhci-of-at91.c > > @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev) > > } > > > > #ifdef CONFIG_PM > > +static int sdhci_at91_suspend(struct device *dev) > > +{ > > + struct sdhci_host *host = dev_get_drvdata(dev); > > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > > + struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host); > > + int ret; > > + > > + ret = sdhci_suspend_host(host); > > + > > This is wrong, you can't call sdhci_suspend_host() unless the device > is runtime resumed... > > > + if (host->runtime_suspended) > > + return ret; > > ... and this is weird... > > > + > > + clk_disable_unprepare(priv->gck); > > + clk_disable_unprepare(priv->hclock); > > + clk_disable_unprepare(priv->mainck); > > + > > + return ret; > > +} > > + > > +static int sdhci_at91_resume(struct device *dev) > > +{ > > + struct sdhci_host *host = dev_get_drvdata(dev); > > + int ret; > > + > > + ret = sdhci_at91_set_clks_presets(dev); > > + if (ret) > > + return ret; > > Instead of doing it like this, I suggest you set a new flag to true > here, let's call it "restore_needed". > > In the ->runtime_resume() callback, you check the restore_needed flag > and performs the extra operations in that case. When that's done, the > ->runtime_resume() callback clears the flag, as to avoid the next > runtime resume from unnecessary doing the extra operations. > > > + > > + return sdhci_resume_host(host); > > Remove this and call pm_runtime_force_resume(). > > > +} > > + > > static int sdhci_at91_runtime_suspend(struct device *dev) > > { > > struct sdhci_host *host = dev_get_drvdata(dev); > > @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev) > > #endif /* CONFIG_PM */ > > > > static const struct dev_pm_ops sdhci_at91_dev_pm_ops = { > > - SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > > Leave the pm_runtime_force_suspend() here, unless you have other > reasons not being described in the change log, to change the system > suspend operations. I think we need to keep it to be able to set the restore_needed flag, isn't it? Regards Ludovic > > > - pm_runtime_force_resume) > > + SET_SYSTEM_SLEEP_PM_OPS(sdhci_at91_suspend, sdhci_at91_resume) > > SET_RUNTIME_PM_OPS(sdhci_at91_runtime_suspend, > > sdhci_at91_runtime_resume, > > NULL) > > -- > > 2.11.0 > > > > Adopting my changes should simplify the code, avoid unnecessary > resuming the device but instead deferring that until really needed via > runtime PM. > > Kind regards > Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11 July 2017 at 15:33, Ludovic Desroches <ludovic.desroches@microchip.com> wrote: > On Tue, Jul 11, 2017 at 02:42:44PM +0200, Ulf Hansson wrote: >> On 16 June 2017 at 09:29, Quentin Schulz >> <quentin.schulz@free-electrons.com> wrote: >> > This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2 >> > SoC's SDHCI controller. >> > >> > When resuming from deepest state, it is required to restore preset >> > registers as the registers are lost since VDD core has been shut down >> > when entering deepest state on the SAMA5D2. The clocks need to be >> > reconfigured as well. >> >> Right, so compared to runtime resume there is some additional >> operations that is needed during system resume. Fair enough. >> >> However by looking at the changes below, you also change the system >> suspend operations, as it now calls sdhci_suspend_host(). Is that >> change needed? Then why? >> >> > >> > The other registers and init process are taken care of by the SDHCI >> > core. >> > >> > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> >> > --- >> > drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++-- >> > 1 file changed, 32 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c >> > index fb8c6011f13d..300513fc1068 100644 >> > --- a/drivers/mmc/host/sdhci-of-at91.c >> > +++ b/drivers/mmc/host/sdhci-of-at91.c >> > @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev) >> > } >> > >> > #ifdef CONFIG_PM >> > +static int sdhci_at91_suspend(struct device *dev) >> > +{ >> > + struct sdhci_host *host = dev_get_drvdata(dev); >> > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> > + struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host); >> > + int ret; >> > + >> > + ret = sdhci_suspend_host(host); >> > + >> >> This is wrong, you can't call sdhci_suspend_host() unless the device >> is runtime resumed... >> >> > + if (host->runtime_suspended) >> > + return ret; >> >> ... and this is weird... >> >> > + >> > + clk_disable_unprepare(priv->gck); >> > + clk_disable_unprepare(priv->hclock); >> > + clk_disable_unprepare(priv->mainck); >> > + >> > + return ret; >> > +} >> > + >> > +static int sdhci_at91_resume(struct device *dev) >> > +{ >> > + struct sdhci_host *host = dev_get_drvdata(dev); >> > + int ret; >> > + >> > + ret = sdhci_at91_set_clks_presets(dev); >> > + if (ret) >> > + return ret; >> >> Instead of doing it like this, I suggest you set a new flag to true >> here, let's call it "restore_needed". >> >> In the ->runtime_resume() callback, you check the restore_needed flag >> and performs the extra operations in that case. When that's done, the >> ->runtime_resume() callback clears the flag, as to avoid the next >> runtime resume from unnecessary doing the extra operations. >> >> > + >> > + return sdhci_resume_host(host); >> >> Remove this and call pm_runtime_force_resume(). >> >> > +} >> > + >> > static int sdhci_at91_runtime_suspend(struct device *dev) >> > { >> > struct sdhci_host *host = dev_get_drvdata(dev); >> > @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev) >> > #endif /* CONFIG_PM */ >> > >> > static const struct dev_pm_ops sdhci_at91_dev_pm_ops = { >> > - SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, >> >> Leave the pm_runtime_force_suspend() here, unless you have other >> reasons not being described in the change log, to change the system >> suspend operations. > > I think we need to keep it to be able to set the restore_needed flag, isn't it? Yeah, perhaps it's better to set the flag from sdhci_at91_suspend() and instead leave the resume callback being assigned to pm_runtime_force_resume(). I guess that is what you meant? [...] Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 11, 2017 at 03:54:17PM +0200, Ulf Hansson wrote: > On 11 July 2017 at 15:33, Ludovic Desroches > <ludovic.desroches@microchip.com> wrote: > > On Tue, Jul 11, 2017 at 02:42:44PM +0200, Ulf Hansson wrote: > >> On 16 June 2017 at 09:29, Quentin Schulz > >> <quentin.schulz@free-electrons.com> wrote: > >> > This adds deepest (Backup+Self-Refresh) PM support to the ATMEL SAMA5D2 > >> > SoC's SDHCI controller. > >> > > >> > When resuming from deepest state, it is required to restore preset > >> > registers as the registers are lost since VDD core has been shut down > >> > when entering deepest state on the SAMA5D2. The clocks need to be > >> > reconfigured as well. > >> > >> Right, so compared to runtime resume there is some additional > >> operations that is needed during system resume. Fair enough. > >> > >> However by looking at the changes below, you also change the system > >> suspend operations, as it now calls sdhci_suspend_host(). Is that > >> change needed? Then why? > >> > >> > > >> > The other registers and init process are taken care of by the SDHCI > >> > core. > >> > > >> > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > >> > --- > >> > drivers/mmc/host/sdhci-of-at91.c | 34 ++++++++++++++++++++++++++++++++-- > >> > 1 file changed, 32 insertions(+), 2 deletions(-) > >> > > >> > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c > >> > index fb8c6011f13d..300513fc1068 100644 > >> > --- a/drivers/mmc/host/sdhci-of-at91.c > >> > +++ b/drivers/mmc/host/sdhci-of-at91.c > >> > @@ -207,6 +207,37 @@ static int sdhci_at91_set_clks_presets(struct device *dev) > >> > } > >> > > >> > #ifdef CONFIG_PM > >> > +static int sdhci_at91_suspend(struct device *dev) > >> > +{ > >> > + struct sdhci_host *host = dev_get_drvdata(dev); > >> > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > >> > + struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host); > >> > + int ret; > >> > + > >> > + ret = sdhci_suspend_host(host); > >> > + > >> > >> This is wrong, you can't call sdhci_suspend_host() unless the device > >> is runtime resumed... > >> > >> > + if (host->runtime_suspended) > >> > + return ret; > >> > >> ... and this is weird... > >> > >> > + > >> > + clk_disable_unprepare(priv->gck); > >> > + clk_disable_unprepare(priv->hclock); > >> > + clk_disable_unprepare(priv->mainck); > >> > + > >> > + return ret; > >> > +} > >> > + > >> > +static int sdhci_at91_resume(struct device *dev) > >> > +{ > >> > + struct sdhci_host *host = dev_get_drvdata(dev); > >> > + int ret; > >> > + > >> > + ret = sdhci_at91_set_clks_presets(dev); > >> > + if (ret) > >> > + return ret; > >> > >> Instead of doing it like this, I suggest you set a new flag to true > >> here, let's call it "restore_needed". > >> > >> In the ->runtime_resume() callback, you check the restore_needed flag > >> and performs the extra operations in that case. When that's done, the > >> ->runtime_resume() callback clears the flag, as to avoid the next > >> runtime resume from unnecessary doing the extra operations. > >> > >> > + > >> > + return sdhci_resume_host(host); > >> > >> Remove this and call pm_runtime_force_resume(). > >> > >> > +} > >> > + > >> > static int sdhci_at91_runtime_suspend(struct device *dev) > >> > { > >> > struct sdhci_host *host = dev_get_drvdata(dev); > >> > @@ -256,8 +287,7 @@ static int sdhci_at91_runtime_resume(struct device *dev) > >> > #endif /* CONFIG_PM */ > >> > > >> > static const struct dev_pm_ops sdhci_at91_dev_pm_ops = { > >> > - SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > >> > >> Leave the pm_runtime_force_suspend() here, unless you have other > >> reasons not being described in the change log, to change the system > >> suspend operations. > > > > I think we need to keep it to be able to set the restore_needed flag, isn't it? > > Yeah, perhaps it's better to set the flag from sdhci_at91_suspend() > and instead leave the resume callback being assigned to > pm_runtime_force_resume(). > > I guess that is what you meant? Exactly. Thanks for your smart solution! Regards Ludovic -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c index 7611fd679f1a..fb8c6011f13d 100644 --- a/drivers/mmc/host/sdhci-of-at91.c +++ b/drivers/mmc/host/sdhci-of-at91.c @@ -128,6 +128,84 @@ static const struct of_device_id sdhci_at91_dt_match[] = { }; MODULE_DEVICE_TABLE(of, sdhci_at91_dt_match); +static int sdhci_at91_set_clks_presets(struct device *dev) +{ + struct sdhci_host *host = dev_get_drvdata(dev); + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct sdhci_at91_priv *priv = sdhci_pltfm_priv(pltfm_host); + int ret; + unsigned int caps0, caps1; + unsigned int clk_base, clk_mul; + unsigned int gck_rate, real_gck_rate; + unsigned int preset_div; + + /* + * The mult clock is provided by as a generated clock by the PMC + * controller. In order to set the rate of gck, we have to get the + * base clock rate and the clock mult from capabilities. + */ + clk_prepare_enable(priv->hclock); + caps0 = readl(host->ioaddr + SDHCI_CAPABILITIES); + caps1 = readl(host->ioaddr + SDHCI_CAPABILITIES_1); + clk_base = (caps0 & SDHCI_CLOCK_V3_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT; + clk_mul = (caps1 & SDHCI_CLOCK_MUL_MASK) >> SDHCI_CLOCK_MUL_SHIFT; + gck_rate = clk_base * 1000000 * (clk_mul + 1); + ret = clk_set_rate(priv->gck, gck_rate); + if (ret < 0) { + dev_err(dev, "failed to set gck"); + clk_disable_unprepare(priv->hclock); + return ret; + } + /* + * We need to check if we have the requested rate for gck because in + * some cases this rate could be not supported. If it happens, the rate + * is the closest one gck can provide. We have to update the value + * of clk mul. + */ + real_gck_rate = clk_get_rate(priv->gck); + if (real_gck_rate != gck_rate) { + clk_mul = real_gck_rate / (clk_base * 1000000) - 1; + caps1 &= (~SDHCI_CLOCK_MUL_MASK); + caps1 |= ((clk_mul << SDHCI_CLOCK_MUL_SHIFT) & + SDHCI_CLOCK_MUL_MASK); + /* Set capabilities in r/w mode. */ + writel(SDMMC_CACR_KEY | SDMMC_CACR_CAPWREN, + host->ioaddr + SDMMC_CACR); + writel(caps1, host->ioaddr + SDHCI_CAPABILITIES_1); + /* Set capabilities in ro mode. */ + writel(0, host->ioaddr + SDMMC_CACR); + dev_info(dev, "update clk mul to %u as gck rate is %u Hz\n", + clk_mul, real_gck_rate); + } + + /* + * We have to set preset values because it depends on the clk_mul + * value. Moreover, SDR104 is supported in a degraded mode since the + * maximum sd clock value is 120 MHz instead of 208 MHz. For that + * reason, we need to use presets to support SDR104. + */ + preset_div = DIV_ROUND_UP(real_gck_rate, 24000000) - 1; + writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, + host->ioaddr + SDHCI_PRESET_FOR_SDR12); + preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1; + writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, + host->ioaddr + SDHCI_PRESET_FOR_SDR25); + preset_div = DIV_ROUND_UP(real_gck_rate, 100000000) - 1; + writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, + host->ioaddr + SDHCI_PRESET_FOR_SDR50); + preset_div = DIV_ROUND_UP(real_gck_rate, 120000000) - 1; + writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, + host->ioaddr + SDHCI_PRESET_FOR_SDR104); + preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1; + writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, + host->ioaddr + SDHCI_PRESET_FOR_DDR50); + + clk_prepare_enable(priv->mainck); + clk_prepare_enable(priv->gck); + + return 0; +} + #ifdef CONFIG_PM static int sdhci_at91_runtime_suspend(struct device *dev) { @@ -192,11 +270,7 @@ static int sdhci_at91_probe(struct platform_device *pdev) struct sdhci_host *host; struct sdhci_pltfm_host *pltfm_host; struct sdhci_at91_priv *priv; - unsigned int caps0, caps1; - unsigned int clk_base, clk_mul; - unsigned int gck_rate, real_gck_rate; int ret; - unsigned int preset_div; match = of_match_device(sdhci_at91_dt_match, &pdev->dev); if (!match) @@ -228,66 +302,9 @@ static int sdhci_at91_probe(struct platform_device *pdev) return PTR_ERR(priv->gck); } - /* - * The mult clock is provided by as a generated clock by the PMC - * controller. In order to set the rate of gck, we have to get the - * base clock rate and the clock mult from capabilities. - */ - clk_prepare_enable(priv->hclock); - caps0 = readl(host->ioaddr + SDHCI_CAPABILITIES); - caps1 = readl(host->ioaddr + SDHCI_CAPABILITIES_1); - clk_base = (caps0 & SDHCI_CLOCK_V3_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT; - clk_mul = (caps1 & SDHCI_CLOCK_MUL_MASK) >> SDHCI_CLOCK_MUL_SHIFT; - gck_rate = clk_base * 1000000 * (clk_mul + 1); - ret = clk_set_rate(priv->gck, gck_rate); - if (ret < 0) { - dev_err(&pdev->dev, "failed to set gck"); - goto hclock_disable_unprepare; - } - /* - * We need to check if we have the requested rate for gck because in - * some cases this rate could be not supported. If it happens, the rate - * is the closest one gck can provide. We have to update the value - * of clk mul. - */ - real_gck_rate = clk_get_rate(priv->gck); - if (real_gck_rate != gck_rate) { - clk_mul = real_gck_rate / (clk_base * 1000000) - 1; - caps1 &= (~SDHCI_CLOCK_MUL_MASK); - caps1 |= ((clk_mul << SDHCI_CLOCK_MUL_SHIFT) & SDHCI_CLOCK_MUL_MASK); - /* Set capabilities in r/w mode. */ - writel(SDMMC_CACR_KEY | SDMMC_CACR_CAPWREN, host->ioaddr + SDMMC_CACR); - writel(caps1, host->ioaddr + SDHCI_CAPABILITIES_1); - /* Set capabilities in ro mode. */ - writel(0, host->ioaddr + SDMMC_CACR); - dev_info(&pdev->dev, "update clk mul to %u as gck rate is %u Hz\n", - clk_mul, real_gck_rate); - } - - /* - * We have to set preset values because it depends on the clk_mul - * value. Moreover, SDR104 is supported in a degraded mode since the - * maximum sd clock value is 120 MHz instead of 208 MHz. For that - * reason, we need to use presets to support SDR104. - */ - preset_div = DIV_ROUND_UP(real_gck_rate, 24000000) - 1; - writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, - host->ioaddr + SDHCI_PRESET_FOR_SDR12); - preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1; - writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, - host->ioaddr + SDHCI_PRESET_FOR_SDR25); - preset_div = DIV_ROUND_UP(real_gck_rate, 100000000) - 1; - writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, - host->ioaddr + SDHCI_PRESET_FOR_SDR50); - preset_div = DIV_ROUND_UP(real_gck_rate, 120000000) - 1; - writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, - host->ioaddr + SDHCI_PRESET_FOR_SDR104); - preset_div = DIV_ROUND_UP(real_gck_rate, 50000000) - 1; - writew(SDHCI_AT91_PRESET_COMMON_CONF | preset_div, - host->ioaddr + SDHCI_PRESET_FOR_DDR50); - - clk_prepare_enable(priv->mainck); - clk_prepare_enable(priv->gck); + ret = sdhci_at91_set_clks_presets(&pdev->dev); + if (ret) + goto sdhci_pltfm_free; ret = mmc_of_parse(host->mmc); if (ret) @@ -335,8 +352,8 @@ static int sdhci_at91_probe(struct platform_device *pdev) clocks_disable_unprepare: clk_disable_unprepare(priv->gck); clk_disable_unprepare(priv->mainck); -hclock_disable_unprepare: clk_disable_unprepare(priv->hclock); +sdhci_pltfm_free: sdhci_pltfm_free(pdev); return ret; }
The setting of clocks and presets is currently done in probe only but once deep PM support is added, it'll be needed in the resume function. Let's create a function for this setting. Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> --- drivers/mmc/host/sdhci-of-at91.c | 147 ++++++++++++++++++++++----------------- 1 file changed, 82 insertions(+), 65 deletions(-) -- 2.11.0 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html