Re: [PATCH v3 3/7] clk: qcom: gdsc: enable optional power domain support
From: Dmitry Baryshkov
Date: Fri Jul 09 2021 - 17:29:01 EST
On 09/07/2021 21:38, Bjorn Andersson wrote:
On Fri 09 Jul 12:31 CDT 2021, Dmitry Baryshkov wrote:
On sm8250 dispcc and videocc registers are powered up by the MMCX power
domain. Currently we use a regulator to enable this domain on demand,
however this has some consequences, as genpd code is not reentrant.
Teach Qualcomm clock controller code about setting up runtime PM and
using specified for gdsc powerup.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
---
drivers/clk/qcom/common.c | 37 +++++++++++++++++++++++++++++++------
drivers/clk/qcom/gdsc.c | 4 ++++
2 files changed, 35 insertions(+), 6 deletions(-)
diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index 60d2a78d1395..43d8f8feeb3c 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -10,6 +10,7 @@
#include <linux/clk-provider.h>
#include <linux/reset-controller.h>
#include <linux/of.h>
+#include <linux/pm_runtime.h>
#include "common.h"
#include "clk-rcg.h"
@@ -224,6 +225,11 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec,
return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
}
+static void qcom_cc_pm_runtime_disable(void *data)
+{
+ pm_runtime_disable(data);
+}
+
int qcom_cc_really_probe(struct platform_device *pdev,
const struct qcom_cc_desc *desc, struct regmap *regmap)
{
@@ -241,6 +247,18 @@ int qcom_cc_really_probe(struct platform_device *pdev,
if (!cc)
return -ENOMEM;
+ pm_runtime_enable(dev);
In turingcc-qcs404.c I'm using pm_runtime to have the clock framework
ensure that the iface clock is enabled during clock operations, so this
will result in a "unbalanced enable" warning.
And later I register the disabler:
ret = devm_add_action_or_reset(dev, qcom_cc_pm_runtime_disable, dev);
You might want to add this to qcs404 code.
+ ret = pm_runtime_get_sync(dev);
I don't think you should wrap the entire initialization in a
pm_runtime_get_sync()/put() region. Instead follow the clock framework
and wrap gdsc initialization that needs to touch the hardware in:
Init should be wrapped in the pm_runtime_get/put calls, so that the MMCX
domain is on through the fall init sequence. Otherwise it can get turned
off during it, boom, failed register access and reboot.
if (pm_runtime_enabled())
pm_runtime_get_sync();
I don't think it's worth doing that. Having single lock for the whole
init sequence is safer (and cleaner).
If you check other pm-enabled drivers, they would either call
pm_runtime_enable at the end of the probe or get_sync in the beginning
of the probe and put_FOO in the end. In this driver calling
pm_runtime_enable() at the end of the probe function will not work,
since this way clk subsystem will not pick up the device for runtime
power management (as pm_runtime_enabled() would return false).
I do however think that as of this patch, when probe returns MMCX might
very well be turned off, as the only user (this driver) has pm_runtime
enabled and it's idle. So I think you should introduce the
pm_runtime_get()/put() in the gdsc functions before this patch.
Maybe I'd just squash them together.
To summarize, I think you should rely on the individual clock drivers to
pm_runtime_enable()/disable().
+ if (ret < 0) {
+ pm_runtime_put(dev);
+ pm_runtime_disable(dev);
+ return ret;
+ }
+
+ ret = devm_add_action_or_reset(dev, qcom_cc_pm_runtime_disable, dev);
+ if (ret)
+ goto err;
+
reset = &cc->reset;
reset->rcdev.of_node = dev->of_node;
reset->rcdev.ops = &qcom_reset_ops;
@@ -251,7 +269,7 @@ int qcom_cc_really_probe(struct platform_device *pdev,
ret = devm_reset_controller_register(dev, &reset->rcdev);
if (ret)
- return ret;
+ goto err;
if (desc->gdscs && desc->num_gdscs) {
scd = devm_kzalloc(dev, sizeof(*scd), GFP_KERNEL);
@@ -262,11 +280,11 @@ int qcom_cc_really_probe(struct platform_device *pdev,
scd->num = desc->num_gdscs;
ret = gdsc_register(scd, &reset->rcdev, regmap);
if (ret)
- return ret;
+ goto err;
ret = devm_add_action_or_reset(dev, qcom_cc_gdsc_unregister,
scd);
if (ret)
- return ret;
+ goto err;
}
cc->rclks = rclks;
@@ -277,7 +295,7 @@ int qcom_cc_really_probe(struct platform_device *pdev,
for (i = 0; i < num_clk_hws; i++) {
ret = devm_clk_hw_register(dev, clk_hws[i]);
if (ret)
- return ret;
+ goto err;
}
for (i = 0; i < num_clks; i++) {
@@ -286,14 +304,21 @@ int qcom_cc_really_probe(struct platform_device *pdev,
ret = devm_clk_register_regmap(dev, rclks[i]);
if (ret)
- return ret;
+ goto err;
}
ret = devm_of_clk_add_hw_provider(dev, qcom_cc_clk_hw_get, cc);
if (ret)
- return ret;
+ goto err;
+
+ pm_runtime_put(dev);
return 0;
+
+err:
+ pm_runtime_put(dev);
+
+ return ret;
}
EXPORT_SYMBOL_GPL(qcom_cc_really_probe);
diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 51ed640e527b..ccd36617d067 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -439,6 +439,8 @@ int gdsc_register(struct gdsc_desc *desc,
continue;
if (scs[i]->parent)
pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd);
+ else if (!IS_ERR_OR_NULL(dev->pm_domain))
+ pm_genpd_add_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd);
Nice, I didn't know that we could fish it out of the dev.
Regards,
Bjorn
}
return of_genpd_add_provider_onecell(dev->of_node, data);
@@ -457,6 +459,8 @@ void gdsc_unregister(struct gdsc_desc *desc)
continue;
if (scs[i]->parent)
pm_genpd_remove_subdomain(scs[i]->parent, &scs[i]->pd);
+ else if (!IS_ERR_OR_NULL(dev->pm_domain))
+ pm_genpd_remove_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd);
}
of_genpd_del_provider(dev->of_node);
}
--
2.30.2
--
With best wishes
Dmitry