Re: [PATCH v3 4/7] clk: qcom: gdsc: call runtime PM functions for the provider device
From: Bjorn Andersson <hidden>
Date: 2021-07-09 18:54:28
Also in:
linux-arm-msm, linux-clk, lkml
On Fri 09 Jul 12:31 CDT 2021, Dmitry Baryshkov wrote:
quoted hunk ↗ jump to hunk
In order to properly handle runtime PM status of the provider device, call pm_runtime_get/pm_runtime_put on the clock controller device. Signed-off-by: Dmitry Baryshkov <redacted> --- drivers/clk/qcom/gdsc.c | 66 ++++++++++++++++++++++++++++++++++++++--- drivers/clk/qcom/gdsc.h | 2 ++ 2 files changed, 64 insertions(+), 4 deletions(-)diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c index ccd36617d067..6bec31fccb09 100644 --- a/drivers/clk/qcom/gdsc.c +++ b/drivers/clk/qcom/gdsc.c@@ -11,6 +11,7 @@ #include <linux/kernel.h> #include <linux/ktime.h> #include <linux/pm_domain.h> +#include <linux/pm_runtime.h> #include <linux/regmap.h> #include <linux/regulator/consumer.h> #include <linux/reset-controller.h>@@ -50,6 +51,30 @@ enum gdsc_status { GDSC_ON }; +static int gdsc_pm_runtime_get(struct gdsc *sc) +{ + int ret; + + if (!sc->rpm_dev) + return 0; + + ret = pm_runtime_get_sync(sc->rpm_dev); + if (ret < 0) { + pm_runtime_put_noidle(sc->rpm_dev); + return ret; + } + + return 0; +} + +static int gdsc_pm_runtime_put(struct gdsc *sc) +{ + if (!sc->rpm_dev) + return 0; + + return pm_runtime_put_sync(sc->rpm_dev); +} + /* Returns 1 if GDSC status is status, 0 if not, and < 0 on error */ static int gdsc_check_status(struct gdsc *sc, enum gdsc_status status) {@@ -232,9 +257,8 @@ static void gdsc_retain_ff_on(struct gdsc *sc) regmap_update_bits(sc->regmap, sc->gdscr, mask, mask); } -static int gdsc_enable(struct generic_pm_domain *domain) +static int _gdsc_enable(struct gdsc *sc) { - struct gdsc *sc = domain_to_gdsc(domain); int ret; if (sc->pwrsts == PWRSTS_ON)@@ -290,11 +314,28 @@ static int gdsc_enable(struct generic_pm_domain *domain) return 0; } -static int gdsc_disable(struct generic_pm_domain *domain) +static int gdsc_enable(struct generic_pm_domain *domain) { struct gdsc *sc = domain_to_gdsc(domain); int ret; + ret = gdsc_pm_runtime_get(sc); + if (ret) + return ret; + + ret = _gdsc_enable(sc); + if (ret) { + gdsc_pm_runtime_put(sc);
I presume what you do here is to leave the pm_runtime state of dispcc active if we succeeded in enabling the gdsc. But the gdsc is a subdomain of the parent domain, so the framework should take case of its dependency. So the reason for gdsc_pm_runtime_get()/put() in this code path is so that you can access the dispcc registers, i.e. I think you should get()/put() regardless of the return value.
quoted hunk ↗ jump to hunk
+ return ret; + } + + return 0; +} + +static int _gdsc_disable(struct gdsc *sc) +{ + int ret; + if (sc->pwrsts == PWRSTS_ON) return gdsc_assert_reset(sc);@@ -329,6 +370,18 @@ static int gdsc_disable(struct generic_pm_domain *domain) return 0; } +static int gdsc_disable(struct generic_pm_domain *domain) +{ + struct gdsc *sc = domain_to_gdsc(domain); + int ret; +
If the gdsc is found to be on at initialization, the next operation that will happen is gdsc_disable() and as you didn't activate the pm_runtime state in gdsc_init() you would in theory get here with registers unaccessible. In practice though, the active gdsc should through the being a subdomain of the parent domain keep power on for you, so you won't notice this issue. But as above, I think you should wrap _gdsc_disable() in a get()/put() pair.
quoted hunk ↗ jump to hunk
+ ret = _gdsc_disable(sc); + if (ret) + return ret; + + return gdsc_pm_runtime_put(sc); +} + static int gdsc_init(struct gdsc *sc) { u32 mask, val;@@ -425,6 +478,8 @@ int gdsc_register(struct gdsc_desc *desc, for (i = 0; i < num; i++) { if (!scs[i]) continue; + if (pm_runtime_enabled(dev)) + scs[i]->rpm_dev = dev; scs[i]->regmap = regmap; scs[i]->rcdev = rcdev; ret = gdsc_init(scs[i]);@@ -486,7 +541,10 @@ void gdsc_unregister(struct gdsc_desc *desc) */ int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain) { + struct gdsc *sc = domain_to_gdsc(domain); + /* Do nothing but give genpd the impression that we were successful */ - return 0; + /* Get the runtime PM device only */ + return gdsc_pm_runtime_get(sc);
Per above, if you let the framework deal with the gdsc's dependencies on the parent domain and you only get()/put() for the sake of dispcc then you don't need you don't need to do this to keep the subsequent gdsc_disable() in balance.
quoted hunk ↗ jump to hunk
} EXPORT_SYMBOL_GPL(gdsc_gx_do_nothing_enable);diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h index 5bb396b344d1..a82982df0a55 100644 --- a/drivers/clk/qcom/gdsc.h +++ b/drivers/clk/qcom/gdsc.h@@ -25,6 +25,7 @@ struct reset_controller_dev; * @resets: ids of resets associated with this gdsc * @reset_count: number of @resets * @rcdev: reset controller + * @rpm_dev: runtime PM device */ struct gdsc { struct generic_pm_domain pd;@@ -58,6 +59,7 @@ struct gdsc { const char *supply; struct regulator *rsupply; + struct device *rpm_dev;
This isn't just the "runtime pm device", it's the device this gdsc is associated with. So "dev" sounds sufficient to me, but that requires that you have a separate bool rpm_enabled to remember if pm_runtime_enabled() was true during probe. So unless we need "dev" for something else this might be sufficient. Regards, Bjorn
};
struct gdsc_desc {
--
2.30.2