| Message ID | 20210817012754.8710-21-digetx@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | NVIDIA Tegra power management patches for 5.16 | expand |
On Tue, Aug 17, 2021 at 04:27:40AM +0300, Dmitry Osipenko wrote: > The SDHCI on Tegra belongs to the core power domain and we're going to > enable GENPD support for the core domain. Now SDHCI must be resumed using > runtime PM API in order to initialize the SDHCI power state. The SDHCI > clock rate must be changed using OPP API that will reconfigure the power > domain performance state in accordance to the rate. Add runtime PM and OPP > support to the SDHCI driver. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/mmc/host/sdhci-tegra.c | 146 ++++++++++++++++++++++++--------- > 1 file changed, 105 insertions(+), 41 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c > index 387ce9cdbd7c..a3583359c972 100644 > --- a/drivers/mmc/host/sdhci-tegra.c > +++ b/drivers/mmc/host/sdhci-tegra.c > @@ -15,6 +15,8 @@ > #include <linux/of.h> > #include <linux/of_device.h> > #include <linux/pinctrl/consumer.h> > +#include <linux/pm_opp.h> > +#include <linux/pm_runtime.h> > #include <linux/regulator/consumer.h> > #include <linux/reset.h> > #include <linux/mmc/card.h> > @@ -24,6 +26,8 @@ > #include <linux/gpio/consumer.h> > #include <linux/ktime.h> > > +#include <soc/tegra/common.h> > + > #include "sdhci-pltfm.h" > #include "cqhci.h" > > @@ -123,6 +127,12 @@ > SDHCI_TRNS_BLK_CNT_EN | \ > SDHCI_TRNS_DMA) > > +enum { > + TEGRA_CLK_BULK_SDHCI, > + TEGRA_CLK_BULK_TMCLK, > + TEGRA_CLK_BULK_NUM, > +}; > + > struct sdhci_tegra_soc_data { > const struct sdhci_pltfm_data *pdata; > u64 dma_mask; > @@ -171,6 +181,8 @@ struct sdhci_tegra { > bool enable_hwcq; > unsigned long curr_clk_rate; > u8 tuned_tap_delay; > + > + struct clk_bulk_data clocks[TEGRA_CLK_BULK_NUM]; This doesn't seem worth it to me. There's a lot of churn in this driver that's only needed to convert this to the clk_bulk API and it makes the code a lot more difficult to read, in my opinion. It looks like the only benefit that this gives us is that runtime suspend and resume become a few lines shorter. Thierry
19.08.2021 20:03, Thierry Reding пишет: > On Tue, Aug 17, 2021 at 04:27:40AM +0300, Dmitry Osipenko wrote: >> The SDHCI on Tegra belongs to the core power domain and we're going to >> enable GENPD support for the core domain. Now SDHCI must be resumed using >> runtime PM API in order to initialize the SDHCI power state. The SDHCI >> clock rate must be changed using OPP API that will reconfigure the power >> domain performance state in accordance to the rate. Add runtime PM and OPP >> support to the SDHCI driver. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/mmc/host/sdhci-tegra.c | 146 ++++++++++++++++++++++++--------- >> 1 file changed, 105 insertions(+), 41 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c >> index 387ce9cdbd7c..a3583359c972 100644 >> --- a/drivers/mmc/host/sdhci-tegra.c >> +++ b/drivers/mmc/host/sdhci-tegra.c >> @@ -15,6 +15,8 @@ >> #include <linux/of.h> >> #include <linux/of_device.h> >> #include <linux/pinctrl/consumer.h> >> +#include <linux/pm_opp.h> >> +#include <linux/pm_runtime.h> >> #include <linux/regulator/consumer.h> >> #include <linux/reset.h> >> #include <linux/mmc/card.h> >> @@ -24,6 +26,8 @@ >> #include <linux/gpio/consumer.h> >> #include <linux/ktime.h> >> >> +#include <soc/tegra/common.h> >> + >> #include "sdhci-pltfm.h" >> #include "cqhci.h" >> >> @@ -123,6 +127,12 @@ >> SDHCI_TRNS_BLK_CNT_EN | \ >> SDHCI_TRNS_DMA) >> >> +enum { >> + TEGRA_CLK_BULK_SDHCI, >> + TEGRA_CLK_BULK_TMCLK, >> + TEGRA_CLK_BULK_NUM, >> +}; >> + >> struct sdhci_tegra_soc_data { >> const struct sdhci_pltfm_data *pdata; >> u64 dma_mask; >> @@ -171,6 +181,8 @@ struct sdhci_tegra { >> bool enable_hwcq; >> unsigned long curr_clk_rate; >> u8 tuned_tap_delay; >> + >> + struct clk_bulk_data clocks[TEGRA_CLK_BULK_NUM]; > > This doesn't seem worth it to me. There's a lot of churn in this driver > that's only needed to convert this to the clk_bulk API and it makes the > code a lot more difficult to read, in my opinion. > > It looks like the only benefit that this gives us is that runtime > suspend and resume become a few lines shorter. The driver probe code looks cleaner with that. You should be looking at the final result and not at the patch to see it.
On Fri, Aug 20, 2021 at 01:37:13AM +0300, Dmitry Osipenko wrote: > 19.08.2021 20:03, Thierry Reding пишет: > > On Tue, Aug 17, 2021 at 04:27:40AM +0300, Dmitry Osipenko wrote: > >> The SDHCI on Tegra belongs to the core power domain and we're going to > >> enable GENPD support for the core domain. Now SDHCI must be resumed using > >> runtime PM API in order to initialize the SDHCI power state. The SDHCI > >> clock rate must be changed using OPP API that will reconfigure the power > >> domain performance state in accordance to the rate. Add runtime PM and OPP > >> support to the SDHCI driver. > >> > >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > >> --- > >> drivers/mmc/host/sdhci-tegra.c | 146 ++++++++++++++++++++++++--------- > >> 1 file changed, 105 insertions(+), 41 deletions(-) > >> > >> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c > >> index 387ce9cdbd7c..a3583359c972 100644 > >> --- a/drivers/mmc/host/sdhci-tegra.c > >> +++ b/drivers/mmc/host/sdhci-tegra.c > >> @@ -15,6 +15,8 @@ > >> #include <linux/of.h> > >> #include <linux/of_device.h> > >> #include <linux/pinctrl/consumer.h> > >> +#include <linux/pm_opp.h> > >> +#include <linux/pm_runtime.h> > >> #include <linux/regulator/consumer.h> > >> #include <linux/reset.h> > >> #include <linux/mmc/card.h> > >> @@ -24,6 +26,8 @@ > >> #include <linux/gpio/consumer.h> > >> #include <linux/ktime.h> > >> > >> +#include <soc/tegra/common.h> > >> + > >> #include "sdhci-pltfm.h" > >> #include "cqhci.h" > >> > >> @@ -123,6 +127,12 @@ > >> SDHCI_TRNS_BLK_CNT_EN | \ > >> SDHCI_TRNS_DMA) > >> > >> +enum { > >> + TEGRA_CLK_BULK_SDHCI, > >> + TEGRA_CLK_BULK_TMCLK, > >> + TEGRA_CLK_BULK_NUM, > >> +}; > >> + > >> struct sdhci_tegra_soc_data { > >> const struct sdhci_pltfm_data *pdata; > >> u64 dma_mask; > >> @@ -171,6 +181,8 @@ struct sdhci_tegra { > >> bool enable_hwcq; > >> unsigned long curr_clk_rate; > >> u8 tuned_tap_delay; > >> + > >> + struct clk_bulk_data clocks[TEGRA_CLK_BULK_NUM]; > > > > This doesn't seem worth it to me. There's a lot of churn in this driver > > that's only needed to convert this to the clk_bulk API and it makes the > > code a lot more difficult to read, in my opinion. > > > > It looks like the only benefit that this gives us is that runtime > > suspend and resume become a few lines shorter. > > The driver probe code looks cleaner with that. You should be looking at > the final result and not at the patch to see it. I did look at the final result and didn't find it cleaner at all. =) Thierry
20.08.2021 14:35, Thierry Reding пишет: > On Fri, Aug 20, 2021 at 01:37:13AM +0300, Dmitry Osipenko wrote: >> 19.08.2021 20:03, Thierry Reding пишет: >>> On Tue, Aug 17, 2021 at 04:27:40AM +0300, Dmitry Osipenko wrote: >>>> The SDHCI on Tegra belongs to the core power domain and we're going to >>>> enable GENPD support for the core domain. Now SDHCI must be resumed using >>>> runtime PM API in order to initialize the SDHCI power state. The SDHCI >>>> clock rate must be changed using OPP API that will reconfigure the power >>>> domain performance state in accordance to the rate. Add runtime PM and OPP >>>> support to the SDHCI driver. >>>> >>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>> --- >>>> drivers/mmc/host/sdhci-tegra.c | 146 ++++++++++++++++++++++++--------- >>>> 1 file changed, 105 insertions(+), 41 deletions(-) >>>> >>>> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c >>>> index 387ce9cdbd7c..a3583359c972 100644 >>>> --- a/drivers/mmc/host/sdhci-tegra.c >>>> +++ b/drivers/mmc/host/sdhci-tegra.c >>>> @@ -15,6 +15,8 @@ >>>> #include <linux/of.h> >>>> #include <linux/of_device.h> >>>> #include <linux/pinctrl/consumer.h> >>>> +#include <linux/pm_opp.h> >>>> +#include <linux/pm_runtime.h> >>>> #include <linux/regulator/consumer.h> >>>> #include <linux/reset.h> >>>> #include <linux/mmc/card.h> >>>> @@ -24,6 +26,8 @@ >>>> #include <linux/gpio/consumer.h> >>>> #include <linux/ktime.h> >>>> >>>> +#include <soc/tegra/common.h> >>>> + >>>> #include "sdhci-pltfm.h" >>>> #include "cqhci.h" >>>> >>>> @@ -123,6 +127,12 @@ >>>> SDHCI_TRNS_BLK_CNT_EN | \ >>>> SDHCI_TRNS_DMA) >>>> >>>> +enum { >>>> + TEGRA_CLK_BULK_SDHCI, >>>> + TEGRA_CLK_BULK_TMCLK, >>>> + TEGRA_CLK_BULK_NUM, >>>> +}; >>>> + >>>> struct sdhci_tegra_soc_data { >>>> const struct sdhci_pltfm_data *pdata; >>>> u64 dma_mask; >>>> @@ -171,6 +181,8 @@ struct sdhci_tegra { >>>> bool enable_hwcq; >>>> unsigned long curr_clk_rate; >>>> u8 tuned_tap_delay; >>>> + >>>> + struct clk_bulk_data clocks[TEGRA_CLK_BULK_NUM]; >>> >>> This doesn't seem worth it to me. There's a lot of churn in this driver >>> that's only needed to convert this to the clk_bulk API and it makes the >>> code a lot more difficult to read, in my opinion. >>> >>> It looks like the only benefit that this gives us is that runtime >>> suspend and resume become a few lines shorter. >> >> The driver probe code looks cleaner with that. You should be looking at >> the final result and not at the patch to see it. > > I did look at the final result and didn't find it cleaner at all. =) There is UAF bug in this patch that was spotted by kasan. The sdhci_tegra_soc_data isn't resource-managed, but clk_bulk_data is. I'm now thinking that it should be okay to keep tmclk always-on, so I'll replace the bulk clks back with the sdhci clk in v9.
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c index 387ce9cdbd7c..a3583359c972 100644 --- a/drivers/mmc/host/sdhci-tegra.c +++ b/drivers/mmc/host/sdhci-tegra.c @@ -15,6 +15,8 @@ #include <linux/of.h> #include <linux/of_device.h> #include <linux/pinctrl/consumer.h> +#include <linux/pm_opp.h> +#include <linux/pm_runtime.h> #include <linux/regulator/consumer.h> #include <linux/reset.h> #include <linux/mmc/card.h> @@ -24,6 +26,8 @@ #include <linux/gpio/consumer.h> #include <linux/ktime.h> +#include <soc/tegra/common.h> + #include "sdhci-pltfm.h" #include "cqhci.h" @@ -123,6 +127,12 @@ SDHCI_TRNS_BLK_CNT_EN | \ SDHCI_TRNS_DMA) +enum { + TEGRA_CLK_BULK_SDHCI, + TEGRA_CLK_BULK_TMCLK, + TEGRA_CLK_BULK_NUM, +}; + struct sdhci_tegra_soc_data { const struct sdhci_pltfm_data *pdata; u64 dma_mask; @@ -171,6 +181,8 @@ struct sdhci_tegra { bool enable_hwcq; unsigned long curr_clk_rate; u8 tuned_tap_delay; + + struct clk_bulk_data clocks[TEGRA_CLK_BULK_NUM]; }; static u16 tegra_sdhci_readw(struct sdhci_host *host, int reg) @@ -758,10 +770,15 @@ static void tegra_sdhci_set_clock(struct sdhci_host *host, unsigned int clock) { struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host); + struct device *dev = mmc_dev(host->mmc); unsigned long host_clk; + int err; - if (!clock) - return sdhci_set_clock(host, clock); + if (!clock) { + sdhci_set_clock(host, clock); + dev_pm_opp_set_rate(dev, clock); + return; + } /* * In DDR50/52 modes the Tegra SDHCI controllers require the SDHCI @@ -776,7 +793,12 @@ static void tegra_sdhci_set_clock(struct sdhci_host *host, unsigned int clock) * from clk_get_rate() is used. */ host_clk = tegra_host->ddr_signaling ? clock * 2 : clock; - clk_set_rate(pltfm_host->clk, host_clk); + + err = dev_pm_opp_set_rate(dev, host_clk); + if (err) + dev_err(dev, "failed to set clk rate to %luHz: %d\n", + host_clk, err); + tegra_host->curr_clk_rate = host_clk; if (tegra_host->ddr_signaling) host->max_clk = host_clk; @@ -1653,6 +1675,21 @@ static int sdhci_tegra_probe(struct platform_device *pdev) goto err_power_req; } + tegra_host->clocks[TEGRA_CLK_BULK_SDHCI].id = NULL; + tegra_host->clocks[TEGRA_CLK_BULK_TMCLK].id = "tmclk"; + + rc = devm_clk_bulk_get_optional(&pdev->dev, TEGRA_CLK_BULK_NUM, + tegra_host->clocks); + + /* controller clock is mandatory */ + if (!rc && !tegra_host->clocks[TEGRA_CLK_BULK_SDHCI].clk) + rc = -ENOENT; + + if (rc) { + dev_err_probe(&pdev->dev, rc, "failed to get clock\n"); + goto err_power_req; + } + /* * Tegra210 has a separate SDMMC_LEGACY_TM clock used for host * timeout clock and SW can choose TMCLK or SDCLK for hardware @@ -1669,34 +1706,19 @@ static int sdhci_tegra_probe(struct platform_device *pdev) */ if (soc_data->nvquirks & NVQUIRK_HAS_TMCLK) { - clk = devm_clk_get(&pdev->dev, "tmclk"); - if (IS_ERR(clk)) { - rc = PTR_ERR(clk); - if (rc == -EPROBE_DEFER) - goto err_power_req; - - dev_warn(&pdev->dev, "failed to get tmclk: %d\n", rc); - clk = NULL; - } + clk = tegra_host->clocks[TEGRA_CLK_BULK_TMCLK].clk; + if (!clk) + dev_warn(&pdev->dev, "failed to get tmclk"); clk_set_rate(clk, 12000000); - rc = clk_prepare_enable(clk); - if (rc) { - dev_err(&pdev->dev, - "failed to enable tmclk: %d\n", rc); - goto err_power_req; - } - - tegra_host->tmclk = clk; } - clk = devm_clk_get(mmc_dev(host->mmc), NULL); - if (IS_ERR(clk)) { - rc = dev_err_probe(&pdev->dev, PTR_ERR(clk), - "failed to get clock\n"); - goto err_clk_get; + clk = tegra_host->clocks[TEGRA_CLK_BULK_SDHCI].clk; + if (!clk) { + dev_err(&pdev->dev, "failed to get sdhci clock\n"); + rc = -ENOENT; + goto err_power_req; } - clk_prepare_enable(clk); pltfm_host->clk = clk; tegra_host->rst = devm_reset_control_get_exclusive(&pdev->dev, @@ -1704,9 +1726,18 @@ static int sdhci_tegra_probe(struct platform_device *pdev) if (IS_ERR(tegra_host->rst)) { rc = PTR_ERR(tegra_host->rst); dev_err(&pdev->dev, "failed to get reset control: %d\n", rc); - goto err_rst_get; + goto err_power_req; } + rc = devm_tegra_core_dev_init_opp_table_simple(&pdev->dev); + if (rc) + goto err_power_req; + + pm_runtime_enable(&pdev->dev); + rc = pm_runtime_resume_and_get(&pdev->dev); + if (rc) + goto err_pm_get; + rc = reset_control_assert(tegra_host->rst); if (rc) goto err_rst_get; @@ -1728,9 +1759,9 @@ static int sdhci_tegra_probe(struct platform_device *pdev) err_add_host: reset_control_assert(tegra_host->rst); err_rst_get: - clk_disable_unprepare(pltfm_host->clk); -err_clk_get: - clk_disable_unprepare(tegra_host->tmclk); + pm_runtime_put(&pdev->dev); +err_pm_get: + pm_runtime_disable(&pdev->dev); err_power_req: err_parse_dt: sdhci_pltfm_free(pdev); @@ -1747,19 +1778,44 @@ static int sdhci_tegra_remove(struct platform_device *pdev) reset_control_assert(tegra_host->rst); usleep_range(2000, 4000); - clk_disable_unprepare(pltfm_host->clk); - clk_disable_unprepare(tegra_host->tmclk); + + pm_runtime_put(&pdev->dev); + pm_runtime_disable(&pdev->dev); sdhci_pltfm_free(pdev); return 0; } -#ifdef CONFIG_PM_SLEEP -static int __maybe_unused sdhci_tegra_suspend(struct device *dev) +static int __maybe_unused sdhci_tegra_runtime_suspend(struct device *dev) +{ + struct sdhci_host *host = dev_get_drvdata(dev); + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host); + + clk_bulk_disable_unprepare(TEGRA_CLK_BULK_NUM, tegra_host->clocks); + + return 0; +} + +static int __maybe_unused sdhci_tegra_runtime_resume(struct device *dev) { struct sdhci_host *host = dev_get_drvdata(dev); struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host); + int err; + + err = dev_pm_opp_sync(dev); + if (err) + return err; + + return clk_bulk_prepare_enable(TEGRA_CLK_BULK_NUM, tegra_host->clocks); +} + +#ifdef CONFIG_PM_SLEEP +static int sdhci_tegra_suspend(struct device *dev) +{ + struct sdhci_host *host = dev_get_drvdata(dev); int ret; if (host->mmc->caps2 & MMC_CAP2_CQE) { @@ -1774,17 +1830,22 @@ static int __maybe_unused sdhci_tegra_suspend(struct device *dev) return ret; } - clk_disable_unprepare(pltfm_host->clk); + ret = pm_runtime_force_suspend(dev); + if (ret) { + sdhci_resume_host(host); + cqhci_resume(host->mmc); + return ret; + } + return 0; } -static int __maybe_unused sdhci_tegra_resume(struct device *dev) +static int sdhci_tegra_resume(struct device *dev) { struct sdhci_host *host = dev_get_drvdata(dev); - struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); int ret; - ret = clk_prepare_enable(pltfm_host->clk); + ret = pm_runtime_force_resume(dev); if (ret) return ret; @@ -1803,13 +1864,16 @@ static int __maybe_unused sdhci_tegra_resume(struct device *dev) suspend_host: sdhci_suspend_host(host); disable_clk: - clk_disable_unprepare(pltfm_host->clk); + pm_runtime_force_suspend(dev); return ret; } #endif -static SIMPLE_DEV_PM_OPS(sdhci_tegra_dev_pm_ops, sdhci_tegra_suspend, - sdhci_tegra_resume); +static const struct dev_pm_ops sdhci_tegra_dev_pm_ops = { + SET_RUNTIME_PM_OPS(sdhci_tegra_runtime_suspend, sdhci_tegra_runtime_resume, + NULL) + SET_SYSTEM_SLEEP_PM_OPS(sdhci_tegra_suspend, sdhci_tegra_resume) +}; static struct platform_driver sdhci_tegra_driver = { .driver = {
The SDHCI on Tegra belongs to the core power domain and we're going to enable GENPD support for the core domain. Now SDHCI must be resumed using runtime PM API in order to initialize the SDHCI power state. The SDHCI clock rate must be changed using OPP API that will reconfigure the power domain performance state in accordance to the rate. Add runtime PM and OPP support to the SDHCI driver. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/mmc/host/sdhci-tegra.c | 146 ++++++++++++++++++++++++--------- 1 file changed, 105 insertions(+), 41 deletions(-)