* [PATCH 0/2] clk: qcom: move pm_clk handling to common code
@ 2021-07-10 14:01 Dmitry Baryshkov
2021-07-10 14:01 ` [PATCH 1/2] clk: qcom: use common code for qcom_cc_probe_by_index Dmitry Baryshkov
2021-07-10 14:01 ` [PATCH 2/2] clk: qcom: move pm_clk functionality into common code Dmitry Baryshkov
0 siblings, 2 replies; 6+ messages in thread
From: Dmitry Baryshkov @ 2021-07-10 14:01 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Stephen Boyd, Michael Turquette, Taniya Das
Cc: linux-arm-msm, linux-clk, Ulf Hansson
Several Qualcomm clock controller drivers use pm_clk functionality.
Instead of having common code in all the drivers, move the pm_clk
handling to the qcom_cc_map/qcom_cc_probe.
Note, this was compile tested only.
----------------------------------------------------------------
Dmitry Baryshkov (2):
clk: qcom: use common code for qcom_cc_probe_by_index
clk: qcom: move pm_clk functionality into common code
drivers/clk/qcom/camcc-sc7180.c | 44 ++---------
drivers/clk/qcom/common.c | 134 ++++++++++++++++++++++++++++------
drivers/clk/qcom/common.h | 6 ++
drivers/clk/qcom/lpasscorecc-sc7180.c | 56 +++-----------
drivers/clk/qcom/mss-sc7180.c | 40 ++--------
drivers/clk/qcom/q6sstop-qcs404.c | 36 ++-------
drivers/clk/qcom/turingcc-qcs404.c | 34 ++-------
7 files changed, 152 insertions(+), 198 deletions(-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] clk: qcom: use common code for qcom_cc_probe_by_index
2021-07-10 14:01 [PATCH 0/2] clk: qcom: move pm_clk handling to common code Dmitry Baryshkov
@ 2021-07-10 14:01 ` Dmitry Baryshkov
2021-07-16 23:29 ` Bjorn Andersson
2021-07-10 14:01 ` [PATCH 2/2] clk: qcom: move pm_clk functionality into common code Dmitry Baryshkov
1 sibling, 1 reply; 6+ messages in thread
From: Dmitry Baryshkov @ 2021-07-10 14:01 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Stephen Boyd, Michael Turquette, Taniya Das
Cc: linux-arm-msm, linux-clk, Ulf Hansson
Rewrite qcom_cc_map and qcom_cc_probe_by_index to use (new)
qcom_cc_map_by_index, removing code duplication and opening a way to
have more common code in qcom_cc_map*.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/clk/qcom/common.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index 60d2a78d1395..ed7c516a597a 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -69,20 +69,26 @@ int qcom_find_src_index(struct clk_hw *hw, const struct parent_map *map, u8 src)
}
EXPORT_SYMBOL_GPL(qcom_find_src_index);
-struct regmap *
-qcom_cc_map(struct platform_device *pdev, const struct qcom_cc_desc *desc)
+static struct regmap *
+qcom_cc_map_by_index(struct platform_device *pdev, const struct qcom_cc_desc *desc, int index)
{
void __iomem *base;
struct resource *res;
struct device *dev = &pdev->dev;
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ res = platform_get_resource(pdev, IORESOURCE_MEM, index);
base = devm_ioremap_resource(dev, res);
if (IS_ERR(base))
return ERR_CAST(base);
return devm_regmap_init_mmio(dev, base, desc->config);
}
+
+struct regmap *
+qcom_cc_map(struct platform_device *pdev, const struct qcom_cc_desc *desc)
+{
+ return qcom_cc_map_by_index(pdev, desc, 0);
+}
EXPORT_SYMBOL_GPL(qcom_cc_map);
void
@@ -313,15 +319,8 @@ int qcom_cc_probe_by_index(struct platform_device *pdev, int index,
const struct qcom_cc_desc *desc)
{
struct regmap *regmap;
- struct resource *res;
- void __iomem *base;
-
- res = platform_get_resource(pdev, IORESOURCE_MEM, index);
- base = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(base))
- return -ENOMEM;
- regmap = devm_regmap_init_mmio(&pdev->dev, base, desc->config);
+ regmap = qcom_cc_map_by_index(pdev, desc, index);
if (IS_ERR(regmap))
return PTR_ERR(regmap);
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] clk: qcom: move pm_clk functionality into common code
2021-07-10 14:01 [PATCH 0/2] clk: qcom: move pm_clk handling to common code Dmitry Baryshkov
2021-07-10 14:01 ` [PATCH 1/2] clk: qcom: use common code for qcom_cc_probe_by_index Dmitry Baryshkov
@ 2021-07-10 14:01 ` Dmitry Baryshkov
2021-07-16 23:26 ` Bjorn Andersson
1 sibling, 1 reply; 6+ messages in thread
From: Dmitry Baryshkov @ 2021-07-10 14:01 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Stephen Boyd, Michael Turquette, Taniya Das
Cc: linux-arm-msm, linux-clk, Ulf Hansson
Several Qualcomm clock controller drivers use pm_clk functionality.
Instead of having common code in all the drivers, move the pm_clk
handling to the qcom_cc_map/qcom_cc_probe.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/clk/qcom/camcc-sc7180.c | 44 ++--------
drivers/clk/qcom/common.c | 113 +++++++++++++++++++++++---
drivers/clk/qcom/common.h | 6 ++
drivers/clk/qcom/lpasscorecc-sc7180.c | 56 +++----------
drivers/clk/qcom/mss-sc7180.c | 40 ++-------
drivers/clk/qcom/q6sstop-qcs404.c | 36 ++------
drivers/clk/qcom/turingcc-qcs404.c | 34 ++------
7 files changed, 142 insertions(+), 187 deletions(-)
diff --git a/drivers/clk/qcom/camcc-sc7180.c b/drivers/clk/qcom/camcc-sc7180.c
index 9bcf2f8ed4de..1c6c1b7fab51 100644
--- a/drivers/clk/qcom/camcc-sc7180.c
+++ b/drivers/clk/qcom/camcc-sc7180.c
@@ -9,7 +9,6 @@
#include <linux/of.h>
#include <linux/of_device.h>
#include <linux/pm_clock.h>
-#include <linux/pm_runtime.h>
#include <linux/regmap.h>
#include <dt-bindings/clock/qcom,camcc-sc7180.h>
@@ -1631,8 +1630,12 @@ static const struct regmap_config cam_cc_sc7180_regmap_config = {
.fast_io = true,
};
+static const char * const cam_cc_sc7180_pm_clks[] = { "xo", "iface" };
+
static const struct qcom_cc_desc cam_cc_sc7180_desc = {
.config = &cam_cc_sc7180_regmap_config,
+ .pm_clks = cam_cc_sc7180_pm_clks,
+ .num_pm_clks = ARRAY_SIZE(cam_cc_sc7180_pm_clks),
.clk_hws = cam_cc_sc7180_hws,
.num_clk_hws = ARRAY_SIZE(cam_cc_sc7180_hws),
.clks = cam_cc_sc7180_clocks,
@@ -1652,33 +1655,9 @@ static int cam_cc_sc7180_probe(struct platform_device *pdev)
struct regmap *regmap;
int ret;
- pm_runtime_enable(&pdev->dev);
- ret = pm_clk_create(&pdev->dev);
- if (ret < 0)
- return ret;
-
- ret = pm_clk_add(&pdev->dev, "xo");
- if (ret < 0) {
- dev_err(&pdev->dev, "Failed to acquire XO clock\n");
- goto disable_pm_runtime;
- }
-
- ret = pm_clk_add(&pdev->dev, "iface");
- if (ret < 0) {
- dev_err(&pdev->dev, "Failed to acquire iface clock\n");
- goto disable_pm_runtime;
- }
-
- ret = pm_runtime_get(&pdev->dev);
- if (ret)
- goto destroy_pm_clk;
-
regmap = qcom_cc_map(pdev, &cam_cc_sc7180_desc);
- if (IS_ERR(regmap)) {
- ret = PTR_ERR(regmap);
- pm_runtime_put(&pdev->dev);
- goto destroy_pm_clk;
- }
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
clk_fabia_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
clk_fabia_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
@@ -1686,21 +1665,12 @@ static int cam_cc_sc7180_probe(struct platform_device *pdev)
clk_fabia_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
ret = qcom_cc_really_probe(pdev, &cam_cc_sc7180_desc, regmap);
- pm_runtime_put(&pdev->dev);
if (ret < 0) {
dev_err(&pdev->dev, "Failed to register CAM CC clocks\n");
- goto destroy_pm_clk;
+ return 0;
}
return 0;
-
-destroy_pm_clk:
- pm_clk_destroy(&pdev->dev);
-
-disable_pm_runtime:
- pm_runtime_disable(&pdev->dev);
-
- return ret;
}
static const struct dev_pm_ops cam_cc_pm_ops = {
diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index ed7c516a597a..e1d34561cab7 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -7,6 +7,8 @@
#include <linux/module.h>
#include <linux/regmap.h>
#include <linux/platform_device.h>
+#include <linux/pm_clock.h>
+#include <linux/pm_runtime.h>
#include <linux/clk-provider.h>
#include <linux/reset-controller.h>
#include <linux/of.h>
@@ -69,12 +71,86 @@ int qcom_find_src_index(struct clk_hw *hw, const struct parent_map *map, u8 src)
}
EXPORT_SYMBOL_GPL(qcom_find_src_index);
+static void qcom_cc_pm_runtime_disable(void *data)
+{
+ pm_runtime_disable(data);
+}
+
+static void qcom_cc_pm_clk_destroy(void *data)
+{
+ pm_clk_destroy(data);
+}
+
+static int
+qcom_cc_add_pm_clks(struct platform_device *pdev, const struct qcom_cc_desc *desc)
+{
+ struct device *dev = &pdev->dev;
+ int ret;
+ int i;
+
+ if (!desc->num_pm_clks)
+ return 0;
+
+ ret = pm_clk_create(dev);
+ if (ret < 0)
+ return ret;
+ ret = devm_add_action_or_reset(dev, qcom_cc_pm_clk_destroy, dev);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < desc->num_pm_clks; i++) {
+ ret = pm_clk_add(dev, desc->pm_clks[i]);
+ if (ret < 0) {
+ dev_err(dev, "Failed to acquire %s pm clk\n", desc->pm_clks[i]);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static int
+qcom_cc_manage_pm(struct platform_device *pdev, const struct qcom_cc_desc *desc)
+{
+ struct device *dev = &pdev->dev;
+ int ret;
+
+ /* For now enable runtime PM only if we have PM clocks in use */
+ if (desc->num_pm_clks && !pm_runtime_enabled(dev)) {
+ pm_runtime_enable(dev);
+
+ ret = devm_add_action_or_reset(dev, qcom_cc_pm_runtime_disable, dev);
+ if (ret)
+ return ret;
+ }
+
+ ret = qcom_cc_add_pm_clks(pdev, desc);
+ if (ret)
+ return ret;
+
+ /* Other code might have enabled runtime PM, resume device here */
+ if (pm_runtime_enabled(dev)) {
+ ret = pm_runtime_get_sync(dev);
+ if (ret) {
+ pm_runtime_put_noidle(dev);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
static struct regmap *
qcom_cc_map_by_index(struct platform_device *pdev, const struct qcom_cc_desc *desc, int index)
{
void __iomem *base;
struct resource *res;
struct device *dev = &pdev->dev;
+ int ret;
+
+ ret = qcom_cc_manage_pm(pdev, desc);
+ if (ret)
+ return ERR_PTR(ret);
res = platform_get_resource(pdev, IORESOURCE_MEM, index);
base = devm_ioremap_resource(dev, res);
@@ -244,8 +320,10 @@ int qcom_cc_really_probe(struct platform_device *pdev,
struct clk_hw **clk_hws = desc->clk_hws;
cc = devm_kzalloc(dev, sizeof(*cc), GFP_KERNEL);
- if (!cc)
- return -ENOMEM;
+ if (!cc) {
+ ret = -ENOMEM;
+ goto err;
+ }
reset = &cc->reset;
reset->rcdev.of_node = dev->of_node;
@@ -257,22 +335,25 @@ 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);
- if (!scd)
- return -ENOMEM;
+ if (!scd) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
scd->dev = dev;
scd->scs = desc->gdscs;
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;
@@ -283,7 +364,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++) {
@@ -292,14 +373,26 @@ 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;
+
+ if (pm_runtime_enabled(dev)) {
+ /* for the LPASS on sc7180, which uses autosuspend */
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put(dev);
+ }
return 0;
+
+err:
+ if (pm_runtime_enabled(dev))
+ pm_runtime_put(dev);
+
+ return ret;
}
EXPORT_SYMBOL_GPL(qcom_cc_really_probe);
diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
index bb39a7e106d8..5b45e2033a92 100644
--- a/drivers/clk/qcom/common.h
+++ b/drivers/clk/qcom/common.h
@@ -19,8 +19,14 @@ struct clk_hw;
#define PLL_VOTE_FSM_ENA BIT(20)
#define PLL_VOTE_FSM_RESET BIT(21)
+/*
+ * Note: if pm_clks are used, pm_clk_suspend/resume should be called manually
+ * from runtime pm callbacks (or just passed to SET_RUNTIME_PM_OPS).
+ */
struct qcom_cc_desc {
const struct regmap_config *config;
+ const char *const *pm_clks;
+ size_t num_pm_clks;
struct clk_regmap **clks;
size_t num_clks;
const struct qcom_reset_map *resets;
diff --git a/drivers/clk/qcom/lpasscorecc-sc7180.c b/drivers/clk/qcom/lpasscorecc-sc7180.c
index 2e0ecc38efdd..5c7faa36305a 100644
--- a/drivers/clk/qcom/lpasscorecc-sc7180.c
+++ b/drivers/clk/qcom/lpasscorecc-sc7180.c
@@ -338,8 +338,12 @@ static struct regmap_config lpass_core_cc_sc7180_regmap_config = {
.fast_io = true,
};
+static const char * const lpass_sc7180_pm_clks[] = { "iface" };
+
static const struct qcom_cc_desc lpass_core_hm_sc7180_desc = {
.config = &lpass_core_cc_sc7180_regmap_config,
+ .pm_clks = lpass_sc7180_pm_clks,
+ .num_pm_clks = ARRAY_SIZE(lpass_sc7180_pm_clks),
.gdscs = lpass_core_hm_sc7180_gdscs,
.num_gdscs = ARRAY_SIZE(lpass_core_hm_sc7180_gdscs),
};
@@ -352,55 +356,20 @@ static const struct qcom_cc_desc lpass_core_cc_sc7180_desc = {
static const struct qcom_cc_desc lpass_audio_hm_sc7180_desc = {
.config = &lpass_core_cc_sc7180_regmap_config,
+ .pm_clks = lpass_sc7180_pm_clks,
+ .num_pm_clks = ARRAY_SIZE(lpass_sc7180_pm_clks),
.gdscs = lpass_audio_hm_sc7180_gdscs,
.num_gdscs = ARRAY_SIZE(lpass_audio_hm_sc7180_gdscs),
};
-static void lpass_pm_runtime_disable(void *data)
-{
- pm_runtime_disable(data);
-}
-
-static void lpass_pm_clk_destroy(void *data)
-{
- pm_clk_destroy(data);
-}
-
-static int lpass_create_pm_clks(struct platform_device *pdev)
-{
- int ret;
-
- pm_runtime_use_autosuspend(&pdev->dev);
- pm_runtime_set_autosuspend_delay(&pdev->dev, 500);
- pm_runtime_enable(&pdev->dev);
-
- ret = devm_add_action_or_reset(&pdev->dev, lpass_pm_runtime_disable, &pdev->dev);
- if (ret)
- return ret;
-
- ret = pm_clk_create(&pdev->dev);
- if (ret)
- return ret;
- ret = devm_add_action_or_reset(&pdev->dev, lpass_pm_clk_destroy, &pdev->dev);
- if (ret)
- return ret;
-
- ret = pm_clk_add(&pdev->dev, "iface");
- if (ret < 0)
- dev_err(&pdev->dev, "failed to acquire iface clock\n");
-
- return ret;
-}
-
static int lpass_core_cc_sc7180_probe(struct platform_device *pdev)
{
const struct qcom_cc_desc *desc;
struct regmap *regmap;
int ret;
- ret = lpass_create_pm_clks(pdev);
- if (ret)
- return ret;
+ pm_runtime_use_autosuspend(&pdev->dev);
+ pm_runtime_set_autosuspend_delay(&pdev->dev, 500);
lpass_core_cc_sc7180_regmap_config.name = "lpass_audio_cc";
desc = &lpass_audio_hm_sc7180_desc;
@@ -428,20 +397,15 @@ static int lpass_core_cc_sc7180_probe(struct platform_device *pdev)
ret = qcom_cc_really_probe(pdev, &lpass_core_cc_sc7180_desc, regmap);
- pm_runtime_mark_last_busy(&pdev->dev);
- pm_runtime_put_autosuspend(&pdev->dev);
-
return ret;
}
static int lpass_hm_core_probe(struct platform_device *pdev)
{
const struct qcom_cc_desc *desc;
- int ret;
- ret = lpass_create_pm_clks(pdev);
- if (ret)
- return ret;
+ pm_runtime_use_autosuspend(&pdev->dev);
+ pm_runtime_set_autosuspend_delay(&pdev->dev, 500);
lpass_core_cc_sc7180_regmap_config.name = "lpass_hm_core";
desc = &lpass_core_hm_sc7180_desc;
diff --git a/drivers/clk/qcom/mss-sc7180.c b/drivers/clk/qcom/mss-sc7180.c
index 673fa1a4f734..41b448a0f5ff 100644
--- a/drivers/clk/qcom/mss-sc7180.c
+++ b/drivers/clk/qcom/mss-sc7180.c
@@ -63,48 +63,19 @@ static struct clk_regmap *mss_sc7180_clocks[] = {
[MSS_AXI_NAV_CLK] = &mss_axi_nav_clk.clkr,
};
+static const char * const mss_sc7180_pm_clks[] = { "cfg_ahb" };
+
static const struct qcom_cc_desc mss_sc7180_desc = {
.config = &mss_regmap_config,
+ .pm_clks = mss_sc7180_pm_clks,
+ .num_pm_clks = ARRAY_SIZE(mss_sc7180_pm_clks),
.clks = mss_sc7180_clocks,
.num_clks = ARRAY_SIZE(mss_sc7180_clocks),
};
static int mss_sc7180_probe(struct platform_device *pdev)
{
- int ret;
-
- pm_runtime_enable(&pdev->dev);
- ret = pm_clk_create(&pdev->dev);
- if (ret)
- goto disable_pm_runtime;
-
- ret = pm_clk_add(&pdev->dev, "cfg_ahb");
- if (ret < 0) {
- dev_err(&pdev->dev, "failed to acquire iface clock\n");
- goto destroy_pm_clk;
- }
-
- ret = qcom_cc_probe(pdev, &mss_sc7180_desc);
- if (ret < 0)
- goto destroy_pm_clk;
-
- return 0;
-
-destroy_pm_clk:
- pm_clk_destroy(&pdev->dev);
-
-disable_pm_runtime:
- pm_runtime_disable(&pdev->dev);
-
- return ret;
-}
-
-static int mss_sc7180_remove(struct platform_device *pdev)
-{
- pm_clk_destroy(&pdev->dev);
- pm_runtime_disable(&pdev->dev);
-
- return 0;
+ return qcom_cc_probe(pdev, &mss_sc7180_desc);
}
static const struct dev_pm_ops mss_sc7180_pm_ops = {
@@ -119,7 +90,6 @@ MODULE_DEVICE_TABLE(of, mss_sc7180_match_table);
static struct platform_driver mss_sc7180_driver = {
.probe = mss_sc7180_probe,
- .remove = mss_sc7180_remove,
.driver = {
.name = "sc7180-mss",
.of_match_table = mss_sc7180_match_table,
diff --git a/drivers/clk/qcom/q6sstop-qcs404.c b/drivers/clk/qcom/q6sstop-qcs404.c
index 723f932fbf7d..398c237417f8 100644
--- a/drivers/clk/qcom/q6sstop-qcs404.c
+++ b/drivers/clk/qcom/q6sstop-qcs404.c
@@ -117,6 +117,8 @@ static struct regmap_config q6sstop_regmap_config = {
.fast_io = true,
};
+static const char * const qcs404_pm_clks[] = { NULL };
+
static struct clk_regmap *q6sstop_qcs404_clocks[] = {
[LCC_AHBFABRIC_CBC_CLK] = &lcc_ahbfabric_cbc_clk.clkr,
[LCC_Q6SS_AHBS_CBC_CLK] = &lcc_q6ss_ahbs_cbc_clk.clkr,
@@ -144,6 +146,8 @@ static struct clk_regmap *tcsr_qcs404_clocks[] = {
static const struct qcom_cc_desc tcsr_qcs404_desc = {
.config = &q6sstop_regmap_config,
+ .pm_clks = qcs404_pm_clks,
+ .num_pm_clks = ARRAY_SIZE(qcs404_pm_clks),
.clks = tcsr_qcs404_clocks,
.num_clks = ARRAY_SIZE(tcsr_qcs404_clocks),
};
@@ -159,46 +163,19 @@ static int q6sstopcc_qcs404_probe(struct platform_device *pdev)
const struct qcom_cc_desc *desc;
int ret;
- pm_runtime_enable(&pdev->dev);
- ret = pm_clk_create(&pdev->dev);
- if (ret)
- goto disable_pm_runtime;
-
- ret = pm_clk_add(&pdev->dev, NULL);
- if (ret < 0) {
- dev_err(&pdev->dev, "failed to acquire iface clock\n");
- goto destroy_pm_clk;
- }
-
q6sstop_regmap_config.name = "q6sstop_tcsr";
desc = &tcsr_qcs404_desc;
ret = qcom_cc_probe_by_index(pdev, 1, desc);
if (ret)
- goto destroy_pm_clk;
+ return ret;
q6sstop_regmap_config.name = "q6sstop_cc";
desc = &q6sstop_qcs404_desc;
ret = qcom_cc_probe_by_index(pdev, 0, desc);
if (ret)
- goto destroy_pm_clk;
-
- return 0;
-
-destroy_pm_clk:
- pm_clk_destroy(&pdev->dev);
-
-disable_pm_runtime:
- pm_runtime_disable(&pdev->dev);
-
- return ret;
-}
-
-static int q6sstopcc_qcs404_remove(struct platform_device *pdev)
-{
- pm_clk_destroy(&pdev->dev);
- pm_runtime_disable(&pdev->dev);
+ return ret;
return 0;
}
@@ -209,7 +186,6 @@ static const struct dev_pm_ops q6sstopcc_pm_ops = {
static struct platform_driver q6sstopcc_qcs404_driver = {
.probe = q6sstopcc_qcs404_probe,
- .remove = q6sstopcc_qcs404_remove,
.driver = {
.name = "qcs404-q6sstopcc",
.of_match_table = q6sstopcc_qcs404_match_table,
diff --git a/drivers/clk/qcom/turingcc-qcs404.c b/drivers/clk/qcom/turingcc-qcs404.c
index 4cfbbf5bf4d9..ba283812ef7c 100644
--- a/drivers/clk/qcom/turingcc-qcs404.c
+++ b/drivers/clk/qcom/turingcc-qcs404.c
@@ -100,8 +100,12 @@ static const struct regmap_config turingcc_regmap_config = {
.fast_io = true,
};
+static const char * const turingcc_pm_clks[] = { NULL };
+
static const struct qcom_cc_desc turingcc_desc = {
.config = &turingcc_regmap_config,
+ .pm_clks = turingcc_pm_clks,
+ .num_pm_clks = ARRAY_SIZE(turingcc_pm_clks),
.clks = turingcc_clocks,
.num_clks = ARRAY_SIZE(turingcc_clocks),
};
@@ -110,36 +114,9 @@ static int turingcc_probe(struct platform_device *pdev)
{
int ret;
- pm_runtime_enable(&pdev->dev);
- ret = pm_clk_create(&pdev->dev);
- if (ret)
- goto disable_pm_runtime;
-
- ret = pm_clk_add(&pdev->dev, NULL);
- if (ret < 0) {
- dev_err(&pdev->dev, "failed to acquire iface clock\n");
- goto destroy_pm_clk;
- }
-
ret = qcom_cc_probe(pdev, &turingcc_desc);
if (ret < 0)
- goto destroy_pm_clk;
-
- return 0;
-
-destroy_pm_clk:
- pm_clk_destroy(&pdev->dev);
-
-disable_pm_runtime:
- pm_runtime_disable(&pdev->dev);
-
- return ret;
-}
-
-static int turingcc_remove(struct platform_device *pdev)
-{
- pm_clk_destroy(&pdev->dev);
- pm_runtime_disable(&pdev->dev);
+ return ret;
return 0;
}
@@ -156,7 +133,6 @@ MODULE_DEVICE_TABLE(of, turingcc_match_table);
static struct platform_driver turingcc_driver = {
.probe = turingcc_probe,
- .remove = turingcc_remove,
.driver = {
.name = "qcs404-turingcc",
.of_match_table = turingcc_match_table,
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] clk: qcom: move pm_clk functionality into common code
2021-07-10 14:01 ` [PATCH 2/2] clk: qcom: move pm_clk functionality into common code Dmitry Baryshkov
@ 2021-07-16 23:26 ` Bjorn Andersson
2021-07-17 7:33 ` Dmitry Baryshkov
0 siblings, 1 reply; 6+ messages in thread
From: Bjorn Andersson @ 2021-07-16 23:26 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Andy Gross, Stephen Boyd, Michael Turquette, Taniya Das,
linux-arm-msm, linux-clk, Ulf Hansson
On Sat 10 Jul 09:01 CDT 2021, Dmitry Baryshkov wrote:
> Several Qualcomm clock controller drivers use pm_clk functionality.
> Instead of having common code in all the drivers, move the pm_clk
> handling to the qcom_cc_map/qcom_cc_probe.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/clk/qcom/camcc-sc7180.c | 44 ++--------
> drivers/clk/qcom/common.c | 113 +++++++++++++++++++++++---
> drivers/clk/qcom/common.h | 6 ++
> drivers/clk/qcom/lpasscorecc-sc7180.c | 56 +++----------
> drivers/clk/qcom/mss-sc7180.c | 40 ++-------
> drivers/clk/qcom/q6sstop-qcs404.c | 36 ++------
> drivers/clk/qcom/turingcc-qcs404.c | 34 ++------
> 7 files changed, 142 insertions(+), 187 deletions(-)
>
> diff --git a/drivers/clk/qcom/camcc-sc7180.c b/drivers/clk/qcom/camcc-sc7180.c
> index 9bcf2f8ed4de..1c6c1b7fab51 100644
> --- a/drivers/clk/qcom/camcc-sc7180.c
> +++ b/drivers/clk/qcom/camcc-sc7180.c
> @@ -9,7 +9,6 @@
> #include <linux/of.h>
> #include <linux/of_device.h>
> #include <linux/pm_clock.h>
> -#include <linux/pm_runtime.h>
> #include <linux/regmap.h>
>
> #include <dt-bindings/clock/qcom,camcc-sc7180.h>
> @@ -1631,8 +1630,12 @@ static const struct regmap_config cam_cc_sc7180_regmap_config = {
> .fast_io = true,
> };
>
> +static const char * const cam_cc_sc7180_pm_clks[] = { "xo", "iface" };
> +
> static const struct qcom_cc_desc cam_cc_sc7180_desc = {
> .config = &cam_cc_sc7180_regmap_config,
> + .pm_clks = cam_cc_sc7180_pm_clks,
> + .num_pm_clks = ARRAY_SIZE(cam_cc_sc7180_pm_clks),
> .clk_hws = cam_cc_sc7180_hws,
> .num_clk_hws = ARRAY_SIZE(cam_cc_sc7180_hws),
> .clks = cam_cc_sc7180_clocks,
> @@ -1652,33 +1655,9 @@ static int cam_cc_sc7180_probe(struct platform_device *pdev)
> struct regmap *regmap;
> int ret;
>
> - pm_runtime_enable(&pdev->dev);
> - ret = pm_clk_create(&pdev->dev);
> - if (ret < 0)
> - return ret;
> -
> - ret = pm_clk_add(&pdev->dev, "xo");
> - if (ret < 0) {
> - dev_err(&pdev->dev, "Failed to acquire XO clock\n");
> - goto disable_pm_runtime;
> - }
> -
> - ret = pm_clk_add(&pdev->dev, "iface");
> - if (ret < 0) {
> - dev_err(&pdev->dev, "Failed to acquire iface clock\n");
> - goto disable_pm_runtime;
> - }
> -
> - ret = pm_runtime_get(&pdev->dev);
> - if (ret)
> - goto destroy_pm_clk;
> -
> regmap = qcom_cc_map(pdev, &cam_cc_sc7180_desc);
> - if (IS_ERR(regmap)) {
> - ret = PTR_ERR(regmap);
> - pm_runtime_put(&pdev->dev);
> - goto destroy_pm_clk;
> - }
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
>
> clk_fabia_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
> clk_fabia_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
> @@ -1686,21 +1665,12 @@ static int cam_cc_sc7180_probe(struct platform_device *pdev)
> clk_fabia_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
>
> ret = qcom_cc_really_probe(pdev, &cam_cc_sc7180_desc, regmap);
> - pm_runtime_put(&pdev->dev);
> if (ret < 0) {
> dev_err(&pdev->dev, "Failed to register CAM CC clocks\n");
> - goto destroy_pm_clk;
> + return 0;
> }
>
> return 0;
> -
> -destroy_pm_clk:
> - pm_clk_destroy(&pdev->dev);
> -
> -disable_pm_runtime:
> - pm_runtime_disable(&pdev->dev);
> -
> - return ret;
> }
>
> static const struct dev_pm_ops cam_cc_pm_ops = {
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index ed7c516a597a..e1d34561cab7 100644
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c
> @@ -7,6 +7,8 @@
> #include <linux/module.h>
> #include <linux/regmap.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_clock.h>
> +#include <linux/pm_runtime.h>
> #include <linux/clk-provider.h>
> #include <linux/reset-controller.h>
> #include <linux/of.h>
> @@ -69,12 +71,86 @@ int qcom_find_src_index(struct clk_hw *hw, const struct parent_map *map, u8 src)
> }
> EXPORT_SYMBOL_GPL(qcom_find_src_index);
>
> +static void qcom_cc_pm_runtime_disable(void *data)
> +{
> + pm_runtime_disable(data);
> +}
> +
> +static void qcom_cc_pm_clk_destroy(void *data)
> +{
> + pm_clk_destroy(data);
> +}
> +
> +static int
> +qcom_cc_add_pm_clks(struct platform_device *pdev, const struct qcom_cc_desc *desc)
> +{
> + struct device *dev = &pdev->dev;
> + int ret;
> + int i;
> +
> + if (!desc->num_pm_clks)
> + return 0;
> +
> + ret = pm_clk_create(dev);
> + if (ret < 0)
> + return ret;
> + ret = devm_add_action_or_reset(dev, qcom_cc_pm_clk_destroy, dev);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < desc->num_pm_clks; i++) {
> + ret = pm_clk_add(dev, desc->pm_clks[i]);
> + if (ret < 0) {
> + dev_err(dev, "Failed to acquire %s pm clk\n", desc->pm_clks[i]);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int
> +qcom_cc_manage_pm(struct platform_device *pdev, const struct qcom_cc_desc *desc)
> +{
> + struct device *dev = &pdev->dev;
> + int ret;
> +
> + /* For now enable runtime PM only if we have PM clocks in use */
> + if (desc->num_pm_clks && !pm_runtime_enabled(dev)) {
> + pm_runtime_enable(dev);
> +
> + ret = devm_add_action_or_reset(dev, qcom_cc_pm_runtime_disable, dev);
> + if (ret)
> + return ret;
> + }
> +
> + ret = qcom_cc_add_pm_clks(pdev, desc);
> + if (ret)
> + return ret;
> +
> + /* Other code might have enabled runtime PM, resume device here */
> + if (pm_runtime_enabled(dev)) {
> + ret = pm_runtime_get_sync(dev);
As I said previously, don't do this. Look at how clk_pm_runtime_get()
and clk_pm_runtime_put() are invoked throughout the clock framework.
At best this would be an optimization to ensure that the pm_runtime
state isn't toggled back and forth between every operation, but this is
typically not how we deal with that and this is certainly unrelated to
the rest of what the patch does.
> + if (ret) {
> + pm_runtime_put_noidle(dev);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> static struct regmap *
> qcom_cc_map_by_index(struct platform_device *pdev, const struct qcom_cc_desc *desc, int index)
I really don't like the idea of having a function with a name that
indicates that it's mapping hardware blocks to under the hood also play
pm_runtime.
Afaict the reason for this patch is to avoid having to sprinkle
pm_runtime_enable(), pm_clk_create(), pm_clk_destroy() and
pm_runtime_disable() in the various clock drivers that needs this.
But as I said previously, there's a lot of drivers in the kernel that
does exactly and only that, so there's definitely motivation to create
devm_pm_runtime_enable() and devm_pm_clk_create() and then go back and
clean up these and a lot of other drivers.
Perhaps I'm overly optimistic about getting those interfaces landed, if
that's the case I would like to have some explicit
devres-pm_runtime_enable/pm_clk_create/pm_clk_add added in the common
code, rather than piggy backing the existing map function.
But either way, I would much rather see you land the subdomain setup in
gdsc_register(), the pm_runtime_enable/disable we need in the
dispcc-sm8250 and the pm_runtime_get()/put() we need in gdsc_init(),
gdsc_enable() and gdsc_disable() - before refactoring all this.
> {
> void __iomem *base;
> struct resource *res;
> struct device *dev = &pdev->dev;
> + int ret;
> +
> + ret = qcom_cc_manage_pm(pdev, desc);
> + if (ret)
> + return ERR_PTR(ret);
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> base = devm_ioremap_resource(dev, res);
> @@ -244,8 +320,10 @@ int qcom_cc_really_probe(struct platform_device *pdev,
> struct clk_hw **clk_hws = desc->clk_hws;
>
> cc = devm_kzalloc(dev, sizeof(*cc), GFP_KERNEL);
> - if (!cc)
> - return -ENOMEM;
> + if (!cc) {
> + ret = -ENOMEM;
> + goto err;
> + }
>
> reset = &cc->reset;
> reset->rcdev.of_node = dev->of_node;
> @@ -257,22 +335,25 @@ 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);
> - if (!scd)
> - return -ENOMEM;
> + if (!scd) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> scd->dev = dev;
> scd->scs = desc->gdscs;
> 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;
> @@ -283,7 +364,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++) {
> @@ -292,14 +373,26 @@ 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;
> +
> + if (pm_runtime_enabled(dev)) {
> + /* for the LPASS on sc7180, which uses autosuspend */
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put(dev);
> + }
>
> return 0;
> +
> +err:
> + if (pm_runtime_enabled(dev))
> + pm_runtime_put(dev);
> +
> + return ret;
> }
> EXPORT_SYMBOL_GPL(qcom_cc_really_probe);
>
> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
> index bb39a7e106d8..5b45e2033a92 100644
> --- a/drivers/clk/qcom/common.h
> +++ b/drivers/clk/qcom/common.h
> @@ -19,8 +19,14 @@ struct clk_hw;
> #define PLL_VOTE_FSM_ENA BIT(20)
> #define PLL_VOTE_FSM_RESET BIT(21)
>
> +/*
> + * Note: if pm_clks are used, pm_clk_suspend/resume should be called manually
> + * from runtime pm callbacks (or just passed to SET_RUNTIME_PM_OPS).
> + */
I don't like the fact that you're hiding most of the pm_runtime boiler
plate code, but each driver still needs to do this.
Perhaps there is merit to have a qcom_cc_pm_enable_and_add_clocks() and
qcom_cc_pm_ops exposed by common.c, but please let's add the pieces we
need first.
Regards,
Bjorn
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] clk: qcom: use common code for qcom_cc_probe_by_index
2021-07-10 14:01 ` [PATCH 1/2] clk: qcom: use common code for qcom_cc_probe_by_index Dmitry Baryshkov
@ 2021-07-16 23:29 ` Bjorn Andersson
0 siblings, 0 replies; 6+ messages in thread
From: Bjorn Andersson @ 2021-07-16 23:29 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Andy Gross, Stephen Boyd, Michael Turquette, Taniya Das,
linux-arm-msm, linux-clk, Ulf Hansson
On Sat 10 Jul 09:01 CDT 2021, Dmitry Baryshkov wrote:
> Rewrite qcom_cc_map and qcom_cc_probe_by_index to use (new)
> qcom_cc_map_by_index, removing code duplication and opening a way to
> have more common code in qcom_cc_map*.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Regards,
Bjorn
> ---
> drivers/clk/qcom/common.c | 21 ++++++++++-----------
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index 60d2a78d1395..ed7c516a597a 100644
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c
> @@ -69,20 +69,26 @@ int qcom_find_src_index(struct clk_hw *hw, const struct parent_map *map, u8 src)
> }
> EXPORT_SYMBOL_GPL(qcom_find_src_index);
>
> -struct regmap *
> -qcom_cc_map(struct platform_device *pdev, const struct qcom_cc_desc *desc)
> +static struct regmap *
> +qcom_cc_map_by_index(struct platform_device *pdev, const struct qcom_cc_desc *desc, int index)
> {
> void __iomem *base;
> struct resource *res;
> struct device *dev = &pdev->dev;
>
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> base = devm_ioremap_resource(dev, res);
> if (IS_ERR(base))
> return ERR_CAST(base);
>
> return devm_regmap_init_mmio(dev, base, desc->config);
> }
> +
> +struct regmap *
> +qcom_cc_map(struct platform_device *pdev, const struct qcom_cc_desc *desc)
> +{
> + return qcom_cc_map_by_index(pdev, desc, 0);
> +}
> EXPORT_SYMBOL_GPL(qcom_cc_map);
>
> void
> @@ -313,15 +319,8 @@ int qcom_cc_probe_by_index(struct platform_device *pdev, int index,
> const struct qcom_cc_desc *desc)
> {
> struct regmap *regmap;
> - struct resource *res;
> - void __iomem *base;
> -
> - res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> - base = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(base))
> - return -ENOMEM;
>
> - regmap = devm_regmap_init_mmio(&pdev->dev, base, desc->config);
> + regmap = qcom_cc_map_by_index(pdev, desc, index);
> if (IS_ERR(regmap))
> return PTR_ERR(regmap);
>
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] clk: qcom: move pm_clk functionality into common code
2021-07-16 23:26 ` Bjorn Andersson
@ 2021-07-17 7:33 ` Dmitry Baryshkov
0 siblings, 0 replies; 6+ messages in thread
From: Dmitry Baryshkov @ 2021-07-17 7:33 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Andy Gross, Stephen Boyd, Michael Turquette, Taniya Das,
open list:DRM DRIVER FOR MSM ADRENO GPU,
open list:COMMON CLK FRAMEWORK, Ulf Hansson
On Sat, 17 Jul 2021 at 02:27, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Sat 10 Jul 09:01 CDT 2021, Dmitry Baryshkov wrote:
>
> > Several Qualcomm clock controller drivers use pm_clk functionality.
> > Instead of having common code in all the drivers, move the pm_clk
> > handling to the qcom_cc_map/qcom_cc_probe.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> > drivers/clk/qcom/camcc-sc7180.c | 44 ++--------
> > drivers/clk/qcom/common.c | 113 +++++++++++++++++++++++---
> > drivers/clk/qcom/common.h | 6 ++
> > drivers/clk/qcom/lpasscorecc-sc7180.c | 56 +++----------
> > drivers/clk/qcom/mss-sc7180.c | 40 ++-------
> > drivers/clk/qcom/q6sstop-qcs404.c | 36 ++------
> > drivers/clk/qcom/turingcc-qcs404.c | 34 ++------
> > 7 files changed, 142 insertions(+), 187 deletions(-)
> >
> > diff --git a/drivers/clk/qcom/camcc-sc7180.c b/drivers/clk/qcom/camcc-sc7180.c
> > index 9bcf2f8ed4de..1c6c1b7fab51 100644
> > --- a/drivers/clk/qcom/camcc-sc7180.c
> > +++ b/drivers/clk/qcom/camcc-sc7180.c
> > @@ -9,7 +9,6 @@
> > #include <linux/of.h>
> > #include <linux/of_device.h>
> > #include <linux/pm_clock.h>
> > -#include <linux/pm_runtime.h>
> > #include <linux/regmap.h>
> >
> > #include <dt-bindings/clock/qcom,camcc-sc7180.h>
> > @@ -1631,8 +1630,12 @@ static const struct regmap_config cam_cc_sc7180_regmap_config = {
> > .fast_io = true,
> > };
> >
> > +static const char * const cam_cc_sc7180_pm_clks[] = { "xo", "iface" };
> > +
> > static const struct qcom_cc_desc cam_cc_sc7180_desc = {
> > .config = &cam_cc_sc7180_regmap_config,
> > + .pm_clks = cam_cc_sc7180_pm_clks,
> > + .num_pm_clks = ARRAY_SIZE(cam_cc_sc7180_pm_clks),
> > .clk_hws = cam_cc_sc7180_hws,
> > .num_clk_hws = ARRAY_SIZE(cam_cc_sc7180_hws),
> > .clks = cam_cc_sc7180_clocks,
> > @@ -1652,33 +1655,9 @@ static int cam_cc_sc7180_probe(struct platform_device *pdev)
> > struct regmap *regmap;
> > int ret;
> >
> > - pm_runtime_enable(&pdev->dev);
> > - ret = pm_clk_create(&pdev->dev);
> > - if (ret < 0)
> > - return ret;
> > -
> > - ret = pm_clk_add(&pdev->dev, "xo");
> > - if (ret < 0) {
> > - dev_err(&pdev->dev, "Failed to acquire XO clock\n");
> > - goto disable_pm_runtime;
> > - }
> > -
> > - ret = pm_clk_add(&pdev->dev, "iface");
> > - if (ret < 0) {
> > - dev_err(&pdev->dev, "Failed to acquire iface clock\n");
> > - goto disable_pm_runtime;
> > - }
> > -
> > - ret = pm_runtime_get(&pdev->dev);
> > - if (ret)
> > - goto destroy_pm_clk;
> > -
> > regmap = qcom_cc_map(pdev, &cam_cc_sc7180_desc);
> > - if (IS_ERR(regmap)) {
> > - ret = PTR_ERR(regmap);
> > - pm_runtime_put(&pdev->dev);
> > - goto destroy_pm_clk;
> > - }
> > + if (IS_ERR(regmap))
> > + return PTR_ERR(regmap);
> >
> > clk_fabia_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
> > clk_fabia_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
> > @@ -1686,21 +1665,12 @@ static int cam_cc_sc7180_probe(struct platform_device *pdev)
> > clk_fabia_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
> >
> > ret = qcom_cc_really_probe(pdev, &cam_cc_sc7180_desc, regmap);
> > - pm_runtime_put(&pdev->dev);
> > if (ret < 0) {
> > dev_err(&pdev->dev, "Failed to register CAM CC clocks\n");
> > - goto destroy_pm_clk;
> > + return 0;
> > }
> >
> > return 0;
> > -
> > -destroy_pm_clk:
> > - pm_clk_destroy(&pdev->dev);
> > -
> > -disable_pm_runtime:
> > - pm_runtime_disable(&pdev->dev);
> > -
> > - return ret;
> > }
> >
> > static const struct dev_pm_ops cam_cc_pm_ops = {
> > diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> > index ed7c516a597a..e1d34561cab7 100644
> > --- a/drivers/clk/qcom/common.c
> > +++ b/drivers/clk/qcom/common.c
> > @@ -7,6 +7,8 @@
> > #include <linux/module.h>
> > #include <linux/regmap.h>
> > #include <linux/platform_device.h>
> > +#include <linux/pm_clock.h>
> > +#include <linux/pm_runtime.h>
> > #include <linux/clk-provider.h>
> > #include <linux/reset-controller.h>
> > #include <linux/of.h>
> > @@ -69,12 +71,86 @@ int qcom_find_src_index(struct clk_hw *hw, const struct parent_map *map, u8 src)
> > }
> > EXPORT_SYMBOL_GPL(qcom_find_src_index);
> >
> > +static void qcom_cc_pm_runtime_disable(void *data)
> > +{
> > + pm_runtime_disable(data);
> > +}
> > +
> > +static void qcom_cc_pm_clk_destroy(void *data)
> > +{
> > + pm_clk_destroy(data);
> > +}
> > +
> > +static int
> > +qcom_cc_add_pm_clks(struct platform_device *pdev, const struct qcom_cc_desc *desc)
> > +{
> > + struct device *dev = &pdev->dev;
> > + int ret;
> > + int i;
> > +
> > + if (!desc->num_pm_clks)
> > + return 0;
> > +
> > + ret = pm_clk_create(dev);
> > + if (ret < 0)
> > + return ret;
> > + ret = devm_add_action_or_reset(dev, qcom_cc_pm_clk_destroy, dev);
> > + if (ret)
> > + return ret;
> > +
> > + for (i = 0; i < desc->num_pm_clks; i++) {
> > + ret = pm_clk_add(dev, desc->pm_clks[i]);
> > + if (ret < 0) {
> > + dev_err(dev, "Failed to acquire %s pm clk\n", desc->pm_clks[i]);
> > + return ret;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +qcom_cc_manage_pm(struct platform_device *pdev, const struct qcom_cc_desc *desc)
> > +{
> > + struct device *dev = &pdev->dev;
> > + int ret;
> > +
> > + /* For now enable runtime PM only if we have PM clocks in use */
> > + if (desc->num_pm_clks && !pm_runtime_enabled(dev)) {
> > + pm_runtime_enable(dev);
> > +
> > + ret = devm_add_action_or_reset(dev, qcom_cc_pm_runtime_disable, dev);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + ret = qcom_cc_add_pm_clks(pdev, desc);
> > + if (ret)
> > + return ret;
> > +
> > + /* Other code might have enabled runtime PM, resume device here */
> > + if (pm_runtime_enabled(dev)) {
> > + ret = pm_runtime_get_sync(dev);
>
> As I said previously, don't do this. Look at how clk_pm_runtime_get()
> and clk_pm_runtime_put() are invoked throughout the clock framework.
Please, check what I'm changing here. sc7180/qcs404 Do call
pm_runtime_get/_put in the _probe function. Like most other drivers
do. Do you dislike the pm_runtime_enabled() call? Or the idea of
calling pm_runtime_get_foo()/_put_foo()? I don't think we can get w/o
the latter.
> At best this would be an optimization to ensure that the pm_runtime
> state isn't toggled back and forth between every operation, but this is
> typically not how we deal with that and this is certainly unrelated to
> the rest of what the patch does.
>
> > + if (ret) {
> > + pm_runtime_put_noidle(dev);
> > + return ret;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static struct regmap *
> > qcom_cc_map_by_index(struct platform_device *pdev, const struct qcom_cc_desc *desc, int index)
>
> I really don't like the idea of having a function with a name that
> indicates that it's mapping hardware blocks to under the hood also play
> pm_runtime.
>
> Afaict the reason for this patch is to avoid having to sprinkle
> pm_runtime_enable(), pm_clk_create(), pm_clk_destroy() and
> pm_runtime_disable() in the various clock drivers that needs this.
>
> But as I said previously, there's a lot of drivers in the kernel that
> does exactly and only that, so there's definitely motivation to create
> devm_pm_runtime_enable() and devm_pm_clk_create() and then go back and
> clean up these and a lot of other drivers.
I like the idea of these helpers. Will try adding the devres-managed
helpers and using them from the qcom cc code. I will remove it from
the qcom_cc_map().
>
> Perhaps I'm overly optimistic about getting those interfaces landed, if
> that's the case I would like to have some explicit
> devres-pm_runtime_enable/pm_clk_create/pm_clk_add added in the common
> code, rather than piggy backing the existing map function.
>
> But either way, I would much rather see you land the subdomain setup in
> gdsc_register(), the pm_runtime_enable/disable we need in the
> dispcc-sm8250 and the pm_runtime_get()/put() we need in gdsc_init(),
> gdsc_enable() and gdsc_disable() - before refactoring all this.
Well, after you pointed me to the turingcc driver(and other pm-enabled
qcom cc drivers), it was quite natural to rework this to come up with
the code that would be common to qcs404, sc7180 and sm8250 in terms of
setting up runtime pm. Otherwise we can land the sm8250 code and then
have to rewrite it again to support both gdsc-subdomains and pm_clk
usage.
>
> > {
> > void __iomem *base;
> > struct resource *res;
> > struct device *dev = &pdev->dev;
> > + int ret;
> > +
> > + ret = qcom_cc_manage_pm(pdev, desc);
> > + if (ret)
> > + return ERR_PTR(ret);
> >
> > res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> > base = devm_ioremap_resource(dev, res);
> > @@ -244,8 +320,10 @@ int qcom_cc_really_probe(struct platform_device *pdev,
> > struct clk_hw **clk_hws = desc->clk_hws;
> >
> > cc = devm_kzalloc(dev, sizeof(*cc), GFP_KERNEL);
> > - if (!cc)
> > - return -ENOMEM;
> > + if (!cc) {
> > + ret = -ENOMEM;
> > + goto err;
> > + }
> >
> > reset = &cc->reset;
> > reset->rcdev.of_node = dev->of_node;
> > @@ -257,22 +335,25 @@ 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);
> > - if (!scd)
> > - return -ENOMEM;
> > + if (!scd) {
> > + ret = -ENOMEM;
> > + goto err;
> > + }
> > +
> > scd->dev = dev;
> > scd->scs = desc->gdscs;
> > 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;
> > @@ -283,7 +364,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++) {
> > @@ -292,14 +373,26 @@ 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;
> > +
> > + if (pm_runtime_enabled(dev)) {
> > + /* for the LPASS on sc7180, which uses autosuspend */
> > + pm_runtime_mark_last_busy(dev);
> > + pm_runtime_put(dev);
> > + }
> >
> > return 0;
> > +
> > +err:
> > + if (pm_runtime_enabled(dev))
> > + pm_runtime_put(dev);
> > +
> > + return ret;
> > }
> > EXPORT_SYMBOL_GPL(qcom_cc_really_probe);
> >
> > diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
> > index bb39a7e106d8..5b45e2033a92 100644
> > --- a/drivers/clk/qcom/common.h
> > +++ b/drivers/clk/qcom/common.h
> > @@ -19,8 +19,14 @@ struct clk_hw;
> > #define PLL_VOTE_FSM_ENA BIT(20)
> > #define PLL_VOTE_FSM_RESET BIT(21)
> >
> > +/*
> > + * Note: if pm_clks are used, pm_clk_suspend/resume should be called manually
> > + * from runtime pm callbacks (or just passed to SET_RUNTIME_PM_OPS).
> > + */
>
> I don't like the fact that you're hiding most of the pm_runtime boiler
> plate code, but each driver still needs to do this.
I did not like that too. I have some ideas (like using device_type),
but this can wait.
>
> Perhaps there is merit to have a qcom_cc_pm_enable_and_add_clocks() and
> qcom_cc_pm_ops exposed by common.c, but please let's add the pieces we
> need first.
Ack.
>
> Regards,
> Bjorn
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-07-17 7:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-10 14:01 [PATCH 0/2] clk: qcom: move pm_clk handling to common code Dmitry Baryshkov
2021-07-10 14:01 ` [PATCH 1/2] clk: qcom: use common code for qcom_cc_probe_by_index Dmitry Baryshkov
2021-07-16 23:29 ` Bjorn Andersson
2021-07-10 14:01 ` [PATCH 2/2] clk: qcom: move pm_clk functionality into common code Dmitry Baryshkov
2021-07-16 23:26 ` Bjorn Andersson
2021-07-17 7:33 ` Dmitry Baryshkov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).