[PATCH v2 0/2] drm/msm: rework clock handling

[PATCH v2 0/2] drm/msm: rework clock handling

[Dmitry Baryshkov]

msm_dss_clk_*() functions significantly duplicate clk_bulk_* family of
functions. Drop custom code and use bulk clocks directly. This also
removes dependency of DP driver on the DPU driver internals.

Changes since v1:
 - Rebase on top of current tree to fix conflicts

The following changes since commit e4840d537c2c6b1189d4de16ee0f4820e069dcea:

  drm/msm: Do hw_init() before capturing GPU state (2021-11-22 10:45:55 -0800)

are available in the Git repository at:

  https://git.linaro.org/people/dmitry.baryshkov/kernel.git dpu-clocks-cleanup

for you to fetch changes up to d6c3e05f71d0fe131f427c32553a01b6df0bec9d:

  drm/msm/dp: rewrite dss_module_power to use bulk clock functions (2021-11-25 12:20:56 +0300)

----------------------------------------------------------------
Dmitry Baryshkov (2):
      drm/msm/dpu: simplify clocks handling
      drm/msm/dp: rewrite dss_module_power to use bulk clock functions

 drivers/gpu/drm/msm/Makefile                  |   1 -
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c |  24 +---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |   6 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c   | 187 --------------------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h   |  40 ------
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |  46 ++-----
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h       |   4 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c      |  26 ++--
 drivers/gpu/drm/msm/dp/dp_ctrl.c              |  19 ++-
 drivers/gpu/drm/msm/dp/dp_parser.c            |  21 ++-
 drivers/gpu/drm/msm/dp/dp_parser.h            |  17 ++-
 drivers/gpu/drm/msm/dp/dp_power.c             |  81 ++++++-----
 drivers/gpu/drm/msm/msm_drv.c                 |  49 +++++++
 drivers/gpu/drm/msm/msm_drv.h                 |   1 +
 14 files changed, 164 insertions(+), 358 deletions(-)
 delete mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c
 delete mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h

[PATCH v2 1/2] drm/msm/dpu: simplify clocks handling

[Dmitry Baryshkov]

DPU driver contains code to parse clock items from device tree into
special data struct and then enable/disable/set rate for the clocks
using that data struct. However the DPU driver itself uses only parsing
and enabling/disabling part (the rate setting is used by DP driver).

Move this implementation to the DP driver (which actually uses rate
setting) and replace hand-coded enable/disable/get loops in the DPU
with the respective clk_bulk operations. Put operation is removed
completely because, it is handled using devres instead.

DP implementation is unchanged for now.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/Makefile                  |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 24 ++-----
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  6 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       | 46 +++----------
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h       |  4 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c      | 26 +++----
 .../dpu1/dpu_io_util.c => dp/dp_clk_util.c}   | 69 +------------------
 .../dpu1/dpu_io_util.h => dp/dp_clk_util.h}   |  2 -
 drivers/gpu/drm/msm/dp/dp_parser.h            |  2 +-
 drivers/gpu/drm/msm/msm_drv.c                 | 49 +++++++++++++
 drivers/gpu/drm/msm/msm_drv.h                 |  1 +
 11 files changed, 84 insertions(+), 147 deletions(-)
 rename drivers/gpu/drm/msm/{disp/dpu1/dpu_io_util.c => dp/dp_clk_util.c} (61%)
 rename drivers/gpu/drm/msm/{disp/dpu1/dpu_io_util.h => dp/dp_clk_util.h} (92%)

diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index 40577f8856d8..b6637da219b0 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -69,7 +69,6 @@ msm-y := \
 	disp/dpu1/dpu_hw_top.o \
 	disp/dpu1/dpu_hw_util.o \
 	disp/dpu1/dpu_hw_vbif.o \
-	disp/dpu1/dpu_io_util.o \
 	disp/dpu1/dpu_kms.o \
 	disp/dpu1/dpu_mdss.o \
 	disp/dpu1/dpu_plane.o \
@@ -105,6 +104,7 @@ msm-$(CONFIG_DRM_MSM_GPU_STATE)	+= adreno/a6xx_gpu_state.o
 
 msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \
 	dp/dp_catalog.o \
+	dp/dp_clk_util.o \
 	dp/dp_ctrl.o \
 	dp/dp_display.o \
 	dp/dp_drm.o \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 60fe06018581..4d184122d63e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -284,17 +284,6 @@ void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc)
 	}
 }
 
-static int _dpu_core_perf_set_core_clk_rate(struct dpu_kms *kms, u64 rate)
-{
-	struct dss_clk *core_clk = kms->perf.core_clk;
-
-	if (core_clk->max_rate && (rate > core_clk->max_rate))
-		rate = core_clk->max_rate;
-
-	core_clk->rate = rate;
-	return dev_pm_opp_set_rate(&kms->pdev->dev, core_clk->rate);
-}
-
 static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
 {
 	u64 clk_rate = kms->perf.perf_tune.min_core_clk;
@@ -306,7 +295,7 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
 			dpu_cstate = to_dpu_crtc_state(crtc->state);
 			clk_rate = max(dpu_cstate->new_perf.core_clk_rate,
 							clk_rate);
-			clk_rate = clk_round_rate(kms->perf.core_clk->clk,
+			clk_rate = clk_round_rate(kms->perf.core_clk,
 					clk_rate);
 		}
 	}
@@ -405,10 +394,11 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
 
 		trace_dpu_core_perf_update_clk(kms->dev, stop_req, clk_rate);
 
-		ret = _dpu_core_perf_set_core_clk_rate(kms, clk_rate);
+		if (clk_rate > kms->perf.max_core_clk_rate)
+			clk_rate = kms->perf.max_core_clk_rate;
+		ret = dev_pm_opp_set_rate(&kms->pdev->dev, clk_rate);
 		if (ret) {
-			DPU_ERROR("failed to set %s clock rate %llu\n",
-					kms->perf.core_clk->clk_name, clk_rate);
+			DPU_ERROR("failed to set core clock rate %llu\n", clk_rate);
 			return ret;
 		}
 
@@ -529,13 +519,13 @@ void dpu_core_perf_destroy(struct dpu_core_perf *perf)
 int dpu_core_perf_init(struct dpu_core_perf *perf,
 		struct drm_device *dev,
 		struct dpu_mdss_cfg *catalog,
-		struct dss_clk *core_clk)
+		struct clk *core_clk)
 {
 	perf->dev = dev;
 	perf->catalog = catalog;
 	perf->core_clk = core_clk;
 
-	perf->max_core_clk_rate = core_clk->max_rate;
+	perf->max_core_clk_rate = clk_get_rate(core_clk);
 	if (!perf->max_core_clk_rate) {
 		DPU_DEBUG("optional max core clk rate, use default\n");
 		perf->max_core_clk_rate = DPU_PERF_DEFAULT_MAX_CORE_CLK_RATE;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index cf4b9b5964c6..8dfcc6db7176 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -56,7 +56,7 @@ struct dpu_core_perf_tune {
  * @dev: Pointer to drm device
  * @debugfs_root: top level debug folder
  * @catalog: Pointer to catalog configuration
- * @core_clk: Pointer to core clock structure
+ * @core_clk: Pointer to the core clock
  * @core_clk_rate: current core clock rate
  * @max_core_clk_rate: maximum allowable core clock rate
  * @perf_tune: debug control for performance tuning
@@ -69,7 +69,7 @@ struct dpu_core_perf {
 	struct drm_device *dev;
 	struct dentry *debugfs_root;
 	struct dpu_mdss_cfg *catalog;
-	struct dss_clk *core_clk;
+	struct clk *core_clk;
 	u64 core_clk_rate;
 	u64 max_core_clk_rate;
 	struct dpu_core_perf_tune perf_tune;
@@ -120,7 +120,7 @@ void dpu_core_perf_destroy(struct dpu_core_perf *perf);
 int dpu_core_perf_init(struct dpu_core_perf *perf,
 		struct drm_device *dev,
 		struct dpu_mdss_cfg *catalog,
-		struct dss_clk *core_clk);
+		struct clk *core_clk);
 
 struct dpu_kms;
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index a15b26428280..655cbd912309 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -936,29 +936,15 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms)
 	return 0;
 }
 
-static struct dss_clk *_dpu_kms_get_clk(struct dpu_kms *dpu_kms,
-		char *clock_name)
-{
-	struct dss_module_power *mp = &dpu_kms->mp;
-	int i;
-
-	for (i = 0; i < mp->num_clk; i++) {
-		if (!strcmp(mp->clk_config[i].clk_name, clock_name))
-			return &mp->clk_config[i];
-	}
-
-	return NULL;
-}
-
 u64 dpu_kms_get_clk_rate(struct dpu_kms *dpu_kms, char *clock_name)
 {
-	struct dss_clk *clk;
+	struct clk *clk;
 
-	clk = _dpu_kms_get_clk(dpu_kms, clock_name);
+	clk = msm_clk_bulk_get_clock(dpu_kms->clocks, dpu_kms->num_clocks, clock_name);
 	if (!clk)
 		return -EINVAL;
 
-	return clk_get_rate(clk->clk);
+	return clk_get_rate(clk);
 }
 
 static int dpu_kms_hw_init(struct msm_kms *kms)
@@ -1070,7 +1056,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
 	}
 
 	rc = dpu_core_perf_init(&dpu_kms->perf, dev, dpu_kms->catalog,
-			_dpu_kms_get_clk(dpu_kms, "core"));
+			msm_clk_bulk_get_clock(dpu_kms->clocks, dpu_kms->num_clocks, "core"));
 	if (rc) {
 		DPU_ERROR("failed to init perf %d\n", rc);
 		goto perf_err;
@@ -1157,7 +1143,6 @@ static int dpu_bind(struct device *dev, struct device *master, void *data)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct msm_drm_private *priv = ddev->dev_private;
 	struct dpu_kms *dpu_kms;
-	struct dss_module_power *mp;
 	int ret = 0;
 
 	dpu_kms = devm_kzalloc(&pdev->dev, sizeof(*dpu_kms), GFP_KERNEL);
@@ -1174,12 +1159,12 @@ static int dpu_bind(struct device *dev, struct device *master, void *data)
 		return ret;
 	}
 
-	mp = &dpu_kms->mp;
-	ret = msm_dss_parse_clock(pdev, mp);
-	if (ret) {
+	ret = msm_parse_clock(pdev, &dpu_kms->clocks);
+	if (ret < 0) {
 		DPU_ERROR("failed to parse clocks, ret=%d\n", ret);
 		return ret;
 	}
+	dpu_kms->num_clocks = ret;
 
 	platform_set_drvdata(pdev, dpu_kms);
 
@@ -1203,11 +1188,6 @@ static void dpu_unbind(struct device *dev, struct device *master, void *data)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dpu_kms *dpu_kms = platform_get_drvdata(pdev);
-	struct dss_module_power *mp = &dpu_kms->mp;
-
-	msm_dss_put_clk(mp->clk_config, mp->num_clk);
-	devm_kfree(&pdev->dev, mp->clk_config);
-	mp->num_clk = 0;
 
 	if (dpu_kms->rpm_enabled)
 		pm_runtime_disable(&pdev->dev);
@@ -1231,21 +1211,18 @@ static int dpu_dev_remove(struct platform_device *pdev)
 
 static int __maybe_unused dpu_runtime_suspend(struct device *dev)
 {
-	int i, rc = -1;
+	int i;
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dpu_kms *dpu_kms = platform_get_drvdata(pdev);
-	struct dss_module_power *mp = &dpu_kms->mp;
 
 	/* Drop the performance state vote */
 	dev_pm_opp_set_rate(dev, 0);
-	rc = msm_dss_enable_clk(mp->clk_config, mp->num_clk, false);
-	if (rc)
-		DPU_ERROR("clock disable failed rc:%d\n", rc);
+	clk_bulk_disable_unprepare(dpu_kms->num_clocks, dpu_kms->clocks);
 
 	for (i = 0; i < dpu_kms->num_paths; i++)
 		icc_set_bw(dpu_kms->path[i], 0, 0);
 
-	return rc;
+	return 0;
 }
 
 static int __maybe_unused dpu_runtime_resume(struct device *dev)
@@ -1255,7 +1232,6 @@ static int __maybe_unused dpu_runtime_resume(struct device *dev)
 	struct dpu_kms *dpu_kms = platform_get_drvdata(pdev);
 	struct drm_encoder *encoder;
 	struct drm_device *ddev;
-	struct dss_module_power *mp = &dpu_kms->mp;
 	int i;
 
 	ddev = dpu_kms->dev;
@@ -1265,7 +1241,7 @@ static int __maybe_unused dpu_runtime_resume(struct device *dev)
 	for (i = 0; i < dpu_kms->num_paths; i++)
 		icc_set_bw(dpu_kms->path[i], 0, Bps_to_icc(MIN_IB_BW));
 
-	rc = msm_dss_enable_clk(mp->clk_config, mp->num_clk, true);
+	rc = clk_bulk_prepare_enable(dpu_kms->num_clocks, dpu_kms->clocks);
 	if (rc) {
 		DPU_ERROR("clock enable failed rc:%d\n", rc);
 		return rc;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index 775bcbda860f..d366aa359d38 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -21,7 +21,6 @@
 #include "dpu_hw_lm.h"
 #include "dpu_hw_interrupts.h"
 #include "dpu_hw_top.h"
-#include "dpu_io_util.h"
 #include "dpu_rm.h"
 #include "dpu_core_perf.h"
 
@@ -113,7 +112,8 @@ struct dpu_kms {
 	struct platform_device *pdev;
 	bool rpm_enabled;
 
-	struct dss_module_power mp;
+	struct clk_bulk_data *clocks;
+	int num_clocks;
 
 	/* reference count bandwidth requests, so we know when we can
 	 * release bandwidth.  Each atomic update increments, and frame-
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
index b466784d9822..d7faf11a5456 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
@@ -29,7 +29,8 @@ struct dpu_irq_controller {
 struct dpu_mdss {
 	struct msm_mdss base;
 	void __iomem *mmio;
-	struct dss_module_power mp;
+	struct clk_bulk_data *clocks;
+	int num_clocks;
 	struct dpu_irq_controller irq_controller;
 };
 
@@ -136,10 +137,9 @@ static void _dpu_mdss_irq_domain_fini(struct dpu_mdss *dpu_mdss)
 static int dpu_mdss_enable(struct msm_mdss *mdss)
 {
 	struct dpu_mdss *dpu_mdss = to_dpu_mdss(mdss);
-	struct dss_module_power *mp = &dpu_mdss->mp;
 	int ret;
 
-	ret = msm_dss_enable_clk(mp->clk_config, mp->num_clk, true);
+	ret = clk_bulk_prepare_enable(dpu_mdss->num_clocks, dpu_mdss->clocks);
 	if (ret) {
 		DPU_ERROR("clock enable failed, ret:%d\n", ret);
 		return ret;
@@ -174,14 +174,10 @@ static int dpu_mdss_enable(struct msm_mdss *mdss)
 static int dpu_mdss_disable(struct msm_mdss *mdss)
 {
 	struct dpu_mdss *dpu_mdss = to_dpu_mdss(mdss);
-	struct dss_module_power *mp = &dpu_mdss->mp;
-	int ret;
 
-	ret = msm_dss_enable_clk(mp->clk_config, mp->num_clk, false);
-	if (ret)
-		DPU_ERROR("clock disable failed, ret:%d\n", ret);
+	clk_bulk_disable_unprepare(dpu_mdss->num_clocks, dpu_mdss->clocks);
 
-	return ret;
+	return 0;
 }
 
 static void dpu_mdss_destroy(struct drm_device *dev)
@@ -189,7 +185,6 @@ static void dpu_mdss_destroy(struct drm_device *dev)
 	struct platform_device *pdev = to_platform_device(dev->dev);
 	struct msm_drm_private *priv = dev->dev_private;
 	struct dpu_mdss *dpu_mdss = to_dpu_mdss(priv->mdss);
-	struct dss_module_power *mp = &dpu_mdss->mp;
 	int irq;
 
 	pm_runtime_suspend(dev->dev);
@@ -197,8 +192,6 @@ static void dpu_mdss_destroy(struct drm_device *dev)
 	_dpu_mdss_irq_domain_fini(dpu_mdss);
 	irq = platform_get_irq(pdev, 0);
 	irq_set_chained_handler_and_data(irq, NULL, NULL);
-	msm_dss_put_clk(mp->clk_config, mp->num_clk);
-	devm_kfree(&pdev->dev, mp->clk_config);
 
 	if (dpu_mdss->mmio)
 		devm_iounmap(&pdev->dev, dpu_mdss->mmio);
@@ -217,7 +210,6 @@ int dpu_mdss_init(struct drm_device *dev)
 	struct platform_device *pdev = to_platform_device(dev->dev);
 	struct msm_drm_private *priv = dev->dev_private;
 	struct dpu_mdss *dpu_mdss;
-	struct dss_module_power *mp;
 	int ret;
 	int irq;
 
@@ -231,12 +223,12 @@ int dpu_mdss_init(struct drm_device *dev)
 
 	DRM_DEBUG("mapped mdss address space @%pK\n", dpu_mdss->mmio);
 
-	mp = &dpu_mdss->mp;
-	ret = msm_dss_parse_clock(pdev, mp);
-	if (ret) {
+	ret = msm_parse_clock(pdev, &dpu_mdss->clocks);
+	if (ret < 0) {
 		DPU_ERROR("failed to parse clocks, ret=%d\n", ret);
 		goto clk_parse_err;
 	}
+	dpu_mdss->num_clocks = ret;
 
 	dpu_mdss->base.dev = dev;
 	dpu_mdss->base.funcs = &mdss_funcs;
@@ -263,9 +255,7 @@ int dpu_mdss_init(struct drm_device *dev)
 irq_error:
 	_dpu_mdss_irq_domain_fini(dpu_mdss);
 irq_domain_error:
-	msm_dss_put_clk(mp->clk_config, mp->num_clk);
 clk_parse_err:
-	devm_kfree(&pdev->dev, mp->clk_config);
 	if (dpu_mdss->mmio)
 		devm_iounmap(&pdev->dev, dpu_mdss->mmio);
 	dpu_mdss->mmio = NULL;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c b/drivers/gpu/drm/msm/dp/dp_clk_util.c
similarity index 61%
rename from drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c
rename to drivers/gpu/drm/msm/dp/dp_clk_util.c
index 078afc5f5882..44a4fc59ff31 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c
+++ b/drivers/gpu/drm/msm/dp/dp_clk_util.c
@@ -11,7 +11,7 @@
 
 #include <drm/drm_print.h>
 
-#include "dpu_io_util.h"
+#include "dp_clk_util.h"
 
 void msm_dss_put_clk(struct dss_clk *clk_arry, int num_clk)
 {
@@ -118,70 +118,3 @@ int msm_dss_enable_clk(struct dss_clk *clk_arry, int num_clk, int enable)
 
 	return rc;
 }
-
-int msm_dss_parse_clock(struct platform_device *pdev,
-			struct dss_module_power *mp)
-{
-	u32 i, rc = 0;
-	const char *clock_name;
-	int num_clk = 0;
-
-	if (!pdev || !mp)
-		return -EINVAL;
-
-	mp->num_clk = 0;
-	num_clk = of_property_count_strings(pdev->dev.of_node, "clock-names");
-	if (num_clk <= 0) {
-		pr_debug("clocks are not defined\n");
-		return 0;
-	}
-
-	mp->clk_config = devm_kcalloc(&pdev->dev,
-				      num_clk, sizeof(struct dss_clk),
-				      GFP_KERNEL);
-	if (!mp->clk_config)
-		return -ENOMEM;
-
-	for (i = 0; i < num_clk; i++) {
-		rc = of_property_read_string_index(pdev->dev.of_node,
-						   "clock-names", i,
-						   &clock_name);
-		if (rc) {
-			DRM_DEV_ERROR(&pdev->dev, "Failed to get clock name for %d\n",
-				i);
-			break;
-		}
-		strlcpy(mp->clk_config[i].clk_name, clock_name,
-			sizeof(mp->clk_config[i].clk_name));
-
-		mp->clk_config[i].type = DSS_CLK_AHB;
-	}
-
-	rc = msm_dss_get_clk(&pdev->dev, mp->clk_config, num_clk);
-	if (rc) {
-		DRM_DEV_ERROR(&pdev->dev, "Failed to get clock refs %d\n", rc);
-		goto err;
-	}
-
-	rc = of_clk_set_defaults(pdev->dev.of_node, false);
-	if (rc) {
-		DRM_DEV_ERROR(&pdev->dev, "Failed to set clock defaults %d\n", rc);
-		goto err;
-	}
-
-	for (i = 0; i < num_clk; i++) {
-		u32 rate = clk_get_rate(mp->clk_config[i].clk);
-		if (!rate)
-			continue;
-		mp->clk_config[i].rate = rate;
-		mp->clk_config[i].type = DSS_CLK_PCLK;
-		mp->clk_config[i].max_rate = rate;
-	}
-
-	mp->num_clk = num_clk;
-	return 0;
-
-err:
-	msm_dss_put_clk(mp->clk_config, num_clk);
-	return rc;
-}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h b/drivers/gpu/drm/msm/dp/dp_clk_util.h
similarity index 92%
rename from drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h
rename to drivers/gpu/drm/msm/dp/dp_clk_util.h
index e6b5c772fa3b..6288a2833a58 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h
+++ b/drivers/gpu/drm/msm/dp/dp_clk_util.h
@@ -35,6 +35,4 @@ int msm_dss_get_clk(struct device *dev, struct dss_clk *clk_arry, int num_clk);
 void msm_dss_put_clk(struct dss_clk *clk_arry, int num_clk);
 int msm_dss_clk_set_rate(struct dss_clk *clk_arry, int num_clk);
 int msm_dss_enable_clk(struct dss_clk *clk_arry, int num_clk, int enable);
-int msm_dss_parse_clock(struct platform_device *pdev,
-		struct dss_module_power *mp);
 #endif /* __DPU_IO_UTIL_H__ */
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
index 3172da089421..094b39bfed8c 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.h
+++ b/drivers/gpu/drm/msm/dp/dp_parser.h
@@ -10,7 +10,7 @@
 #include <linux/phy/phy.h>
 #include <linux/phy/phy-dp.h>
 
-#include "dpu_io_util.h"
+#include "dp_clk_util.h"
 #include "msm_drv.h"
 
 #define DP_LABEL "MDSS DP DISPLAY"
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 892c04365239..3e90fca33581 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -5,6 +5,7 @@
  * Author: Rob Clark <robdclark@gmail.com>
  */
 
+#include <linux/clk/clk-conf.h>
 #include <linux/dma-mapping.h>
 #include <linux/kthread.h>
 #include <linux/sched/mm.h>
@@ -123,6 +124,54 @@ struct clk *msm_clk_get(struct platform_device *pdev, const char *name)
 	return clk;
 }
 
+int msm_parse_clock(struct platform_device *pdev, struct clk_bulk_data **clocks)
+{
+	u32 i, rc = 0;
+	const char *clock_name;
+	struct clk_bulk_data *bulk;
+	int num_clk = 0;
+
+	if (!pdev)
+		return -EINVAL;
+
+	num_clk = of_property_count_strings(pdev->dev.of_node, "clock-names");
+	if (num_clk <= 0) {
+		pr_debug("clocks are not defined\n");
+		return 0;
+	}
+
+	bulk = devm_kcalloc(&pdev->dev, num_clk, sizeof(struct clk_bulk_data), GFP_KERNEL);
+	if (!bulk)
+		return -ENOMEM;
+
+	for (i = 0; i < num_clk; i++) {
+		rc = of_property_read_string_index(pdev->dev.of_node,
+						   "clock-names", i,
+						   &clock_name);
+		if (rc) {
+			DRM_DEV_ERROR(&pdev->dev, "Failed to get clock name for %d\n", i);
+			return rc;
+		}
+		bulk[i].id = devm_kstrdup(&pdev->dev, clock_name, GFP_KERNEL);
+	}
+
+	rc = devm_clk_bulk_get(&pdev->dev, num_clk, bulk);
+	if (rc) {
+		DRM_DEV_ERROR(&pdev->dev, "Failed to get clock refs %d\n", rc);
+		return rc;
+	}
+
+	rc = of_clk_set_defaults(pdev->dev.of_node, false);
+	if (rc) {
+		DRM_DEV_ERROR(&pdev->dev, "Failed to set clock defaults %d\n", rc);
+		return rc;
+	}
+
+	*clocks = bulk;
+
+	return num_clk;
+}
+
 static void __iomem *_msm_ioremap(struct platform_device *pdev, const char *name,
 				  const char *dbgname, bool quiet, phys_addr_t *psize)
 {
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 69952b239384..cfede901056d 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -477,6 +477,7 @@ struct clk *msm_clk_get(struct platform_device *pdev, const char *name);
 
 struct clk *msm_clk_bulk_get_clock(struct clk_bulk_data *bulk, int count,
 	const char *name);
+int msm_parse_clock(struct platform_device *pdev, struct clk_bulk_data **clocks);
 void __iomem *msm_ioremap(struct platform_device *pdev, const char *name,
 		const char *dbgname);
 void __iomem *msm_ioremap_size(struct platform_device *pdev, const char *name,
-- 
2.33.0

[PATCH v2 2/2] drm/msm/dp: rewrite dss_module_power to use bulk clock functions

[Dmitry Baryshkov]

In order to simplify DP code, drop hand-coded loops over clock arrays,
replacing them with clk_bulk_* functions.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/Makefile         |   1 -
 drivers/gpu/drm/msm/dp/dp_clk_util.c | 120 ---------------------------
 drivers/gpu/drm/msm/dp/dp_clk_util.h |  38 ---------
 drivers/gpu/drm/msm/dp/dp_ctrl.c     |  19 ++---
 drivers/gpu/drm/msm/dp/dp_parser.c   |  21 ++++-
 drivers/gpu/drm/msm/dp/dp_parser.h   |  17 +++-
 drivers/gpu/drm/msm/dp/dp_power.c    |  81 +++++++++---------
 7 files changed, 83 insertions(+), 214 deletions(-)
 delete mode 100644 drivers/gpu/drm/msm/dp/dp_clk_util.c
 delete mode 100644 drivers/gpu/drm/msm/dp/dp_clk_util.h

diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index b6637da219b0..ccacf604881a 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -104,7 +104,6 @@ msm-$(CONFIG_DRM_MSM_GPU_STATE)	+= adreno/a6xx_gpu_state.o
 
 msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \
 	dp/dp_catalog.o \
-	dp/dp_clk_util.o \
 	dp/dp_ctrl.o \
 	dp/dp_display.o \
 	dp/dp_drm.o \
diff --git a/drivers/gpu/drm/msm/dp/dp_clk_util.c b/drivers/gpu/drm/msm/dp/dp_clk_util.c
deleted file mode 100644
index 44a4fc59ff31..000000000000
--- a/drivers/gpu/drm/msm/dp/dp_clk_util.c
+++ /dev/null
@@ -1,120 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2012-2015, 2017-2018, The Linux Foundation.
- * All rights reserved.
- */
-
-#include <linux/clk.h>
-#include <linux/clk/clk-conf.h>
-#include <linux/err.h>
-#include <linux/delay.h>
-#include <linux/of.h>
-
-#include <drm/drm_print.h>
-
-#include "dp_clk_util.h"
-
-void msm_dss_put_clk(struct dss_clk *clk_arry, int num_clk)
-{
-	int i;
-
-	for (i = num_clk - 1; i >= 0; i--) {
-		if (clk_arry[i].clk)
-			clk_put(clk_arry[i].clk);
-		clk_arry[i].clk = NULL;
-	}
-}
-
-int msm_dss_get_clk(struct device *dev, struct dss_clk *clk_arry, int num_clk)
-{
-	int i, rc = 0;
-
-	for (i = 0; i < num_clk; i++) {
-		clk_arry[i].clk = clk_get(dev, clk_arry[i].clk_name);
-		rc = PTR_ERR_OR_ZERO(clk_arry[i].clk);
-		if (rc) {
-			DEV_ERR("%pS->%s: '%s' get failed. rc=%d\n",
-				__builtin_return_address(0), __func__,
-				clk_arry[i].clk_name, rc);
-			goto error;
-		}
-	}
-
-	return rc;
-
-error:
-	for (i--; i >= 0; i--) {
-		if (clk_arry[i].clk)
-			clk_put(clk_arry[i].clk);
-		clk_arry[i].clk = NULL;
-	}
-
-	return rc;
-}
-
-int msm_dss_clk_set_rate(struct dss_clk *clk_arry, int num_clk)
-{
-	int i, rc = 0;
-
-	for (i = 0; i < num_clk; i++) {
-		if (clk_arry[i].clk) {
-			if (clk_arry[i].type != DSS_CLK_AHB) {
-				DEV_DBG("%pS->%s: '%s' rate %ld\n",
-					__builtin_return_address(0), __func__,
-					clk_arry[i].clk_name,
-					clk_arry[i].rate);
-				rc = clk_set_rate(clk_arry[i].clk,
-					clk_arry[i].rate);
-				if (rc) {
-					DEV_ERR("%pS->%s: %s failed. rc=%d\n",
-						__builtin_return_address(0),
-						__func__,
-						clk_arry[i].clk_name, rc);
-					break;
-				}
-			}
-		} else {
-			DEV_ERR("%pS->%s: '%s' is not available\n",
-				__builtin_return_address(0), __func__,
-				clk_arry[i].clk_name);
-			rc = -EPERM;
-			break;
-		}
-	}
-
-	return rc;
-}
-
-int msm_dss_enable_clk(struct dss_clk *clk_arry, int num_clk, int enable)
-{
-	int i, rc = 0;
-
-	if (enable) {
-		for (i = 0; i < num_clk; i++) {
-			DEV_DBG("%pS->%s: enable '%s'\n",
-				__builtin_return_address(0), __func__,
-				clk_arry[i].clk_name);
-			rc = clk_prepare_enable(clk_arry[i].clk);
-			if (rc)
-				DEV_ERR("%pS->%s: %s en fail. rc=%d\n",
-					__builtin_return_address(0),
-					__func__,
-					clk_arry[i].clk_name, rc);
-
-			if (rc && i) {
-				msm_dss_enable_clk(&clk_arry[i - 1],
-					i - 1, false);
-				break;
-			}
-		}
-	} else {
-		for (i = num_clk - 1; i >= 0; i--) {
-			DEV_DBG("%pS->%s: disable '%s'\n",
-				__builtin_return_address(0), __func__,
-				clk_arry[i].clk_name);
-
-			clk_disable_unprepare(clk_arry[i].clk);
-		}
-	}
-
-	return rc;
-}
diff --git a/drivers/gpu/drm/msm/dp/dp_clk_util.h b/drivers/gpu/drm/msm/dp/dp_clk_util.h
deleted file mode 100644
index 6288a2833a58..000000000000
--- a/drivers/gpu/drm/msm/dp/dp_clk_util.h
+++ /dev/null
@@ -1,38 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/* Copyright (c) 2012, 2017-2018, The Linux Foundation. All rights reserved.
- */
-
-#ifndef __DPU_IO_UTIL_H__
-#define __DPU_IO_UTIL_H__
-
-#include <linux/platform_device.h>
-#include <linux/types.h>
-
-#define DEV_DBG(fmt, args...)   pr_debug(fmt, ##args)
-#define DEV_INFO(fmt, args...)  pr_info(fmt, ##args)
-#define DEV_WARN(fmt, args...)  pr_warn(fmt, ##args)
-#define DEV_ERR(fmt, args...)   pr_err(fmt, ##args)
-
-enum dss_clk_type {
-	DSS_CLK_AHB, /* no set rate. rate controlled through rpm */
-	DSS_CLK_PCLK,
-};
-
-struct dss_clk {
-	struct clk *clk; /* clk handle */
-	char clk_name[32];
-	enum dss_clk_type type;
-	unsigned long rate;
-	unsigned long max_rate;
-};
-
-struct dss_module_power {
-	unsigned int num_clk;
-	struct dss_clk *clk_config;
-};
-
-int msm_dss_get_clk(struct device *dev, struct dss_clk *clk_arry, int num_clk);
-void msm_dss_put_clk(struct dss_clk *clk_arry, int num_clk);
-int msm_dss_clk_set_rate(struct dss_clk *clk_arry, int num_clk);
-int msm_dss_enable_clk(struct dss_clk *clk_arry, int num_clk, int enable);
-#endif /* __DPU_IO_UTIL_H__ */
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 62e75dc8afc6..e9a4d6c32f57 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1289,20 +1289,19 @@ static int dp_ctrl_setup_main_link(struct dp_ctrl_private *ctrl,
 static void dp_ctrl_set_clock_rate(struct dp_ctrl_private *ctrl,
 			enum dp_pm_type module, char *name, unsigned long rate)
 {
+	u32 i;
 	u32 num = ctrl->parser->mp[module].num_clk;
-	struct dss_clk *cfg = ctrl->parser->mp[module].clk_config;
-
-	while (num && strcmp(cfg->clk_name, name)) {
-		num--;
-		cfg++;
-	}
 
 	DRM_DEBUG_DP("setting rate=%lu on clk=%s\n", rate, name);
 
-	if (num)
-		cfg->rate = rate;
-	else
-		DRM_ERROR("%s clock doesn't exit to set rate %lu\n",
+	for (i = 0; i < num; i++) {
+		if (!strcmp(ctrl->parser->mp[module].clocks[i].id, name)) {
+			ctrl->parser->mp[module].clk_config[i].rate = rate;
+			return;
+		}
+	}
+
+	DRM_ERROR("%s clock doesn't exit to set rate %lu\n",
 				name, rate);
 }
 
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
index a7acc23f742b..0fe726913b4e 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -162,6 +162,11 @@ static int dp_parser_init_clk_data(struct dp_parser *parser)
 	}
 
 	core_power->num_clk = core_clk_count;
+	core_power->clocks = devm_kcalloc(dev,
+			core_power->num_clk, sizeof(struct clk_bulk_data),
+			GFP_KERNEL);
+	if (!core_power->clocks)
+		return -ENOMEM;
 	core_power->clk_config = devm_kzalloc(dev,
 			sizeof(struct dss_clk) * core_power->num_clk,
 			GFP_KERNEL);
@@ -175,6 +180,11 @@ static int dp_parser_init_clk_data(struct dp_parser *parser)
 	}
 
 	ctrl_power->num_clk = ctrl_clk_count;
+	ctrl_power->clocks = devm_kcalloc(dev,
+			ctrl_power->num_clk, sizeof(struct clk_bulk_data),
+			GFP_KERNEL);
+	if (!ctrl_power->clocks)
+		return -ENOMEM;
 	ctrl_power->clk_config = devm_kzalloc(dev,
 			sizeof(struct dss_clk) * ctrl_power->num_clk,
 			GFP_KERNEL);
@@ -190,6 +200,11 @@ static int dp_parser_init_clk_data(struct dp_parser *parser)
 	}
 
 	stream_power->num_clk = stream_clk_count;
+	stream_power->clocks = devm_kcalloc(dev,
+			stream_power->num_clk, sizeof(struct clk_bulk_data),
+			GFP_KERNEL);
+	if (!stream_power->clocks)
+		return -ENOMEM;
 	stream_power->clk_config = devm_kzalloc(dev,
 			sizeof(struct dss_clk) * stream_power->num_clk,
 			GFP_KERNEL);
@@ -236,21 +251,21 @@ static int dp_parser_clock(struct dp_parser *parser)
 				core_clk_index < core_clk_count) {
 			struct dss_clk *clk =
 				&core_power->clk_config[core_clk_index];
-			strlcpy(clk->clk_name, clk_name, sizeof(clk->clk_name));
+			core_power->clocks[i].id = devm_kstrdup(dev, clk_name, GFP_KERNEL);
 			clk->type = DSS_CLK_AHB;
 			core_clk_index++;
 		} else if (dp_parser_check_prefix("stream", clk_name) &&
 				stream_clk_index < stream_clk_count) {
 			struct dss_clk *clk =
 				&stream_power->clk_config[stream_clk_index];
-			strlcpy(clk->clk_name, clk_name, sizeof(clk->clk_name));
+			stream_power->clocks[i].id = devm_kstrdup(dev, clk_name, GFP_KERNEL);
 			clk->type = DSS_CLK_PCLK;
 			stream_clk_index++;
 		} else if (dp_parser_check_prefix("ctrl", clk_name) &&
 			   ctrl_clk_index < ctrl_clk_count) {
 			struct dss_clk *clk =
 				&ctrl_power->clk_config[ctrl_clk_index];
-			strlcpy(clk->clk_name, clk_name, sizeof(clk->clk_name));
+			ctrl_power->clocks[i].id = devm_kstrdup(dev, clk_name, GFP_KERNEL);
 			ctrl_clk_index++;
 			if (dp_parser_check_prefix("ctrl_link", clk_name) ||
 			    dp_parser_check_prefix("stream_pixel", clk_name))
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
index 094b39bfed8c..f16072f33cdb 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.h
+++ b/drivers/gpu/drm/msm/dp/dp_parser.h
@@ -10,7 +10,6 @@
 #include <linux/phy/phy.h>
 #include <linux/phy/phy-dp.h>
 
-#include "dp_clk_util.h"
 #include "msm_drv.h"
 
 #define DP_LABEL "MDSS DP DISPLAY"
@@ -106,6 +105,22 @@ struct dp_regulator_cfg {
 	struct dp_reg_entry regs[DP_DEV_REGULATOR_MAX];
 };
 
+enum dss_clk_type {
+	DSS_CLK_AHB, /* no set rate. rate controlled through rpm */
+	DSS_CLK_PCLK,
+};
+
+struct dss_clk {
+	enum dss_clk_type type;
+	unsigned long rate;
+};
+
+struct dss_module_power {
+	unsigned int num_clk;
+	struct clk_bulk_data *clocks;
+	struct dss_clk *clk_config;
+};
+
 /**
  * struct dp_parser - DP parser's data exposed to clients
  *
diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
index b48b45e92bfa..87683071868d 100644
--- a/drivers/gpu/drm/msm/dp/dp_power.c
+++ b/drivers/gpu/drm/msm/dp/dp_power.c
@@ -105,72 +105,69 @@ static int dp_power_clk_init(struct dp_power_private *power)
 	ctrl = &power->parser->mp[DP_CTRL_PM];
 	stream = &power->parser->mp[DP_STREAM_PM];
 
-	rc = msm_dss_get_clk(dev, core->clk_config, core->num_clk);
+	rc = devm_clk_bulk_get(dev, core->num_clk, core->clocks);
 	if (rc) {
 		DRM_ERROR("failed to get %s clk. err=%d\n",
 			dp_parser_pm_name(DP_CORE_PM), rc);
 		return rc;
 	}
 
-	rc = msm_dss_get_clk(dev, ctrl->clk_config, ctrl->num_clk);
+	rc = devm_clk_bulk_get(dev, ctrl->num_clk, ctrl->clocks);
 	if (rc) {
 		DRM_ERROR("failed to get %s clk. err=%d\n",
 			dp_parser_pm_name(DP_CTRL_PM), rc);
-		msm_dss_put_clk(core->clk_config, core->num_clk);
 		return -ENODEV;
 	}
 
-	rc = msm_dss_get_clk(dev, stream->clk_config, stream->num_clk);
+	rc = devm_clk_bulk_get(dev, stream->num_clk, stream->clocks);
 	if (rc) {
 		DRM_ERROR("failed to get %s clk. err=%d\n",
 			dp_parser_pm_name(DP_CTRL_PM), rc);
-		msm_dss_put_clk(core->clk_config, core->num_clk);
 		return -ENODEV;
 	}
 
 	return 0;
 }
 
-static int dp_power_clk_deinit(struct dp_power_private *power)
+static int dp_power_clk_set_link_rate(struct dp_power_private *power,
+			struct dss_clk *clk_arry, int num_clk, int enable)
 {
-	struct dss_module_power *core, *ctrl, *stream;
-
-	core = &power->parser->mp[DP_CORE_PM];
-	ctrl = &power->parser->mp[DP_CTRL_PM];
-	stream = &power->parser->mp[DP_STREAM_PM];
+	u32 rate;
+	int i, rc = 0;
 
-	if (!core || !ctrl || !stream) {
-		DRM_ERROR("invalid power_data\n");
-		return -EINVAL;
+	for (i = 0; i < num_clk; i++) {
+		if (clk_arry[i].type == DSS_CLK_PCLK) {
+			if (enable)
+				rate = clk_arry[i].rate;
+			else
+				rate = 0;
+
+			rc = dev_pm_opp_set_rate(power->dev, rate);
+			if (rc)
+				break;
+		}
 	}
-
-	msm_dss_put_clk(ctrl->clk_config, ctrl->num_clk);
-	msm_dss_put_clk(core->clk_config, core->num_clk);
-	msm_dss_put_clk(stream->clk_config, stream->num_clk);
-	return 0;
+	return rc;
 }
 
-static int dp_power_clk_set_link_rate(struct dp_power_private *power,
-			struct dss_clk *clk_arry, int num_clk, int enable)
+static int dp_clk_set_rate(struct dss_module_power *mp)
 {
-	u32 rate;
 	int i, rc = 0;
+	struct dss_clk *clk_arry = mp->clk_config;
 
-	for (i = 0; i < num_clk; i++) {
-		if (clk_arry[i].clk) {
-			if (clk_arry[i].type == DSS_CLK_PCLK) {
-				if (enable)
-					rate = clk_arry[i].rate;
-				else
-					rate = 0;
-
-				rc = dev_pm_opp_set_rate(power->dev, rate);
-				if (rc)
-					break;
+	for (i = 0; i < mp->num_clk; i++) {
+		if (clk_arry[i].type != DSS_CLK_AHB) {
+			rc = clk_set_rate(mp->clocks[i].clk, mp->clk_config[i].rate);
+			if (rc) {
+				DRM_ERROR("%pS->%s: %s failed. rc=%d\n",
+						__builtin_return_address(0),
+						__func__,
+						mp->clocks[i].id, rc);
+				break;
 			}
-
 		}
 	}
+
 	return rc;
 }
 
@@ -189,7 +186,7 @@ static int dp_power_clk_set_rate(struct dp_power_private *power,
 	} else {
 
 		if (enable) {
-			rc = msm_dss_clk_set_rate(mp->clk_config, mp->num_clk);
+			rc = dp_clk_set_rate(mp);
 			if (rc) {
 				DRM_ERROR("failed to set clks rate\n");
 				return rc;
@@ -197,10 +194,14 @@ static int dp_power_clk_set_rate(struct dp_power_private *power,
 		}
 	}
 
-	rc = msm_dss_enable_clk(mp->clk_config, mp->num_clk, enable);
-	if (rc) {
-		DRM_ERROR("failed to %d clks, err: %d\n", enable, rc);
-		return rc;
+	if (enable) {
+		rc = clk_bulk_prepare_enable(mp->num_clk, mp->clocks);
+		if (rc) {
+			DRM_ERROR("failed to enable clks, err: %d\n", rc);
+			return rc;
+		}
+	} else {
+		clk_bulk_disable_unprepare(mp->num_clk, mp->clocks);
 	}
 
 	return 0;
@@ -336,9 +337,7 @@ void dp_power_client_deinit(struct dp_power *dp_power)
 
 	power = container_of(dp_power, struct dp_power_private, dp_power);
 
-	dp_power_clk_deinit(power);
 	pm_runtime_disable(&power->pdev->dev);
-
 }
 
 int dp_power_init(struct dp_power *dp_power, bool flip)
-- 
2.33.0

Re: [PATCH v2 2/2] drm/msm/dp: rewrite dss_module_power to use bulk clock functions

[Dmitry Baryshkov]

Kuogee,

Some time ago you volonteered to check these two patches on a DP hosts. 
Any progress?

On 26/11/2021 05:35, Dmitry Baryshkov wrote:
> In order to simplify DP code, drop hand-coded loops over clock arrays,
> replacing them with clk_bulk_* functions.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/Makefile         |   1 -
>   drivers/gpu/drm/msm/dp/dp_clk_util.c | 120 ---------------------------
>   drivers/gpu/drm/msm/dp/dp_clk_util.h |  38 ---------
>   drivers/gpu/drm/msm/dp/dp_ctrl.c     |  19 ++---
>   drivers/gpu/drm/msm/dp/dp_parser.c   |  21 ++++-
>   drivers/gpu/drm/msm/dp/dp_parser.h   |  17 +++-
>   drivers/gpu/drm/msm/dp/dp_power.c    |  81 +++++++++---------
>   7 files changed, 83 insertions(+), 214 deletions(-)
>   delete mode 100644 drivers/gpu/drm/msm/dp/dp_clk_util.c
>   delete mode 100644 drivers/gpu/drm/msm/dp/dp_clk_util.h
> 
> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> index b6637da219b0..ccacf604881a 100644
> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -104,7 +104,6 @@ msm-$(CONFIG_DRM_MSM_GPU_STATE)	+= adreno/a6xx_gpu_state.o
>   
>   msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \
>   	dp/dp_catalog.o \
> -	dp/dp_clk_util.o \
>   	dp/dp_ctrl.o \
>   	dp/dp_display.o \
>   	dp/dp_drm.o \
> diff --git a/drivers/gpu/drm/msm/dp/dp_clk_util.c b/drivers/gpu/drm/msm/dp/dp_clk_util.c
> deleted file mode 100644
> index 44a4fc59ff31..000000000000
> --- a/drivers/gpu/drm/msm/dp/dp_clk_util.c
> +++ /dev/null
> @@ -1,120 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0-only
> -/* Copyright (c) 2012-2015, 2017-2018, The Linux Foundation.
> - * All rights reserved.
> - */
> -
> -#include <linux/clk.h>
> -#include <linux/clk/clk-conf.h>
> -#include <linux/err.h>
> -#include <linux/delay.h>
> -#include <linux/of.h>
> -
> -#include <drm/drm_print.h>
> -
> -#include "dp_clk_util.h"
> -
> -void msm_dss_put_clk(struct dss_clk *clk_arry, int num_clk)
> -{
> -	int i;
> -
> -	for (i = num_clk - 1; i >= 0; i--) {
> -		if (clk_arry[i].clk)
> -			clk_put(clk_arry[i].clk);
> -		clk_arry[i].clk = NULL;
> -	}
> -}
> -
> -int msm_dss_get_clk(struct device *dev, struct dss_clk *clk_arry, int num_clk)
> -{
> -	int i, rc = 0;
> -
> -	for (i = 0; i < num_clk; i++) {
> -		clk_arry[i].clk = clk_get(dev, clk_arry[i].clk_name);
> -		rc = PTR_ERR_OR_ZERO(clk_arry[i].clk);
> -		if (rc) {
> -			DEV_ERR("%pS->%s: '%s' get failed. rc=%d\n",
> -				__builtin_return_address(0), __func__,
> -				clk_arry[i].clk_name, rc);
> -			goto error;
> -		}
> -	}
> -
> -	return rc;
> -
> -error:
> -	for (i--; i >= 0; i--) {
> -		if (clk_arry[i].clk)
> -			clk_put(clk_arry[i].clk);
> -		clk_arry[i].clk = NULL;
> -	}
> -
> -	return rc;
> -}
> -
> -int msm_dss_clk_set_rate(struct dss_clk *clk_arry, int num_clk)
> -{
> -	int i, rc = 0;
> -
> -	for (i = 0; i < num_clk; i++) {
> -		if (clk_arry[i].clk) {
> -			if (clk_arry[i].type != DSS_CLK_AHB) {
> -				DEV_DBG("%pS->%s: '%s' rate %ld\n",
> -					__builtin_return_address(0), __func__,
> -					clk_arry[i].clk_name,
> -					clk_arry[i].rate);
> -				rc = clk_set_rate(clk_arry[i].clk,
> -					clk_arry[i].rate);
> -				if (rc) {
> -					DEV_ERR("%pS->%s: %s failed. rc=%d\n",
> -						__builtin_return_address(0),
> -						__func__,
> -						clk_arry[i].clk_name, rc);
> -					break;
> -				}
> -			}
> -		} else {
> -			DEV_ERR("%pS->%s: '%s' is not available\n",
> -				__builtin_return_address(0), __func__,
> -				clk_arry[i].clk_name);
> -			rc = -EPERM;
> -			break;
> -		}
> -	}
> -
> -	return rc;
> -}
> -
> -int msm_dss_enable_clk(struct dss_clk *clk_arry, int num_clk, int enable)
> -{
> -	int i, rc = 0;
> -
> -	if (enable) {
> -		for (i = 0; i < num_clk; i++) {
> -			DEV_DBG("%pS->%s: enable '%s'\n",
> -				__builtin_return_address(0), __func__,
> -				clk_arry[i].clk_name);
> -			rc = clk_prepare_enable(clk_arry[i].clk);
> -			if (rc)
> -				DEV_ERR("%pS->%s: %s en fail. rc=%d\n",
> -					__builtin_return_address(0),
> -					__func__,
> -					clk_arry[i].clk_name, rc);
> -
> -			if (rc && i) {
> -				msm_dss_enable_clk(&clk_arry[i - 1],
> -					i - 1, false);
> -				break;
> -			}
> -		}
> -	} else {
> -		for (i = num_clk - 1; i >= 0; i--) {
> -			DEV_DBG("%pS->%s: disable '%s'\n",
> -				__builtin_return_address(0), __func__,
> -				clk_arry[i].clk_name);
> -
> -			clk_disable_unprepare(clk_arry[i].clk);
> -		}
> -	}
> -
> -	return rc;
> -}
> diff --git a/drivers/gpu/drm/msm/dp/dp_clk_util.h b/drivers/gpu/drm/msm/dp/dp_clk_util.h
> deleted file mode 100644
> index 6288a2833a58..000000000000
> --- a/drivers/gpu/drm/msm/dp/dp_clk_util.h
> +++ /dev/null
> @@ -1,38 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-only */
> -/* Copyright (c) 2012, 2017-2018, The Linux Foundation. All rights reserved.
> - */
> -
> -#ifndef __DPU_IO_UTIL_H__
> -#define __DPU_IO_UTIL_H__
> -
> -#include <linux/platform_device.h>
> -#include <linux/types.h>
> -
> -#define DEV_DBG(fmt, args...)   pr_debug(fmt, ##args)
> -#define DEV_INFO(fmt, args...)  pr_info(fmt, ##args)
> -#define DEV_WARN(fmt, args...)  pr_warn(fmt, ##args)
> -#define DEV_ERR(fmt, args...)   pr_err(fmt, ##args)
> -
> -enum dss_clk_type {
> -	DSS_CLK_AHB, /* no set rate. rate controlled through rpm */
> -	DSS_CLK_PCLK,
> -};
> -
> -struct dss_clk {
> -	struct clk *clk; /* clk handle */
> -	char clk_name[32];
> -	enum dss_clk_type type;
> -	unsigned long rate;
> -	unsigned long max_rate;
> -};
> -
> -struct dss_module_power {
> -	unsigned int num_clk;
> -	struct dss_clk *clk_config;
> -};
> -
> -int msm_dss_get_clk(struct device *dev, struct dss_clk *clk_arry, int num_clk);
> -void msm_dss_put_clk(struct dss_clk *clk_arry, int num_clk);
> -int msm_dss_clk_set_rate(struct dss_clk *clk_arry, int num_clk);
> -int msm_dss_enable_clk(struct dss_clk *clk_arry, int num_clk, int enable);
> -#endif /* __DPU_IO_UTIL_H__ */
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 62e75dc8afc6..e9a4d6c32f57 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1289,20 +1289,19 @@ static int dp_ctrl_setup_main_link(struct dp_ctrl_private *ctrl,
>   static void dp_ctrl_set_clock_rate(struct dp_ctrl_private *ctrl,
>   			enum dp_pm_type module, char *name, unsigned long rate)
>   {
> +	u32 i;
>   	u32 num = ctrl->parser->mp[module].num_clk;
> -	struct dss_clk *cfg = ctrl->parser->mp[module].clk_config;
> -
> -	while (num && strcmp(cfg->clk_name, name)) {
> -		num--;
> -		cfg++;
> -	}
>   
>   	DRM_DEBUG_DP("setting rate=%lu on clk=%s\n", rate, name);
>   
> -	if (num)
> -		cfg->rate = rate;
> -	else
> -		DRM_ERROR("%s clock doesn't exit to set rate %lu\n",
> +	for (i = 0; i < num; i++) {
> +		if (!strcmp(ctrl->parser->mp[module].clocks[i].id, name)) {
> +			ctrl->parser->mp[module].clk_config[i].rate = rate;
> +			return;
> +		}
> +	}
> +
> +	DRM_ERROR("%s clock doesn't exit to set rate %lu\n",
>   				name, rate);
>   }
>   
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
> index a7acc23f742b..0fe726913b4e 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> @@ -162,6 +162,11 @@ static int dp_parser_init_clk_data(struct dp_parser *parser)
>   	}
>   
>   	core_power->num_clk = core_clk_count;
> +	core_power->clocks = devm_kcalloc(dev,
> +			core_power->num_clk, sizeof(struct clk_bulk_data),
> +			GFP_KERNEL);
> +	if (!core_power->clocks)
> +		return -ENOMEM;
>   	core_power->clk_config = devm_kzalloc(dev,
>   			sizeof(struct dss_clk) * core_power->num_clk,
>   			GFP_KERNEL);
> @@ -175,6 +180,11 @@ static int dp_parser_init_clk_data(struct dp_parser *parser)
>   	}
>   
>   	ctrl_power->num_clk = ctrl_clk_count;
> +	ctrl_power->clocks = devm_kcalloc(dev,
> +			ctrl_power->num_clk, sizeof(struct clk_bulk_data),
> +			GFP_KERNEL);
> +	if (!ctrl_power->clocks)
> +		return -ENOMEM;
>   	ctrl_power->clk_config = devm_kzalloc(dev,
>   			sizeof(struct dss_clk) * ctrl_power->num_clk,
>   			GFP_KERNEL);
> @@ -190,6 +200,11 @@ static int dp_parser_init_clk_data(struct dp_parser *parser)
>   	}
>   
>   	stream_power->num_clk = stream_clk_count;
> +	stream_power->clocks = devm_kcalloc(dev,
> +			stream_power->num_clk, sizeof(struct clk_bulk_data),
> +			GFP_KERNEL);
> +	if (!stream_power->clocks)
> +		return -ENOMEM;
>   	stream_power->clk_config = devm_kzalloc(dev,
>   			sizeof(struct dss_clk) * stream_power->num_clk,
>   			GFP_KERNEL);
> @@ -236,21 +251,21 @@ static int dp_parser_clock(struct dp_parser *parser)
>   				core_clk_index < core_clk_count) {
>   			struct dss_clk *clk =
>   				&core_power->clk_config[core_clk_index];
> -			strlcpy(clk->clk_name, clk_name, sizeof(clk->clk_name));
> +			core_power->clocks[i].id = devm_kstrdup(dev, clk_name, GFP_KERNEL);
>   			clk->type = DSS_CLK_AHB;
>   			core_clk_index++;
>   		} else if (dp_parser_check_prefix("stream", clk_name) &&
>   				stream_clk_index < stream_clk_count) {
>   			struct dss_clk *clk =
>   				&stream_power->clk_config[stream_clk_index];
> -			strlcpy(clk->clk_name, clk_name, sizeof(clk->clk_name));
> +			stream_power->clocks[i].id = devm_kstrdup(dev, clk_name, GFP_KERNEL);
>   			clk->type = DSS_CLK_PCLK;
>   			stream_clk_index++;
>   		} else if (dp_parser_check_prefix("ctrl", clk_name) &&
>   			   ctrl_clk_index < ctrl_clk_count) {
>   			struct dss_clk *clk =
>   				&ctrl_power->clk_config[ctrl_clk_index];
> -			strlcpy(clk->clk_name, clk_name, sizeof(clk->clk_name));
> +			ctrl_power->clocks[i].id = devm_kstrdup(dev, clk_name, GFP_KERNEL);
>   			ctrl_clk_index++;
>   			if (dp_parser_check_prefix("ctrl_link", clk_name) ||
>   			    dp_parser_check_prefix("stream_pixel", clk_name))
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
> index 094b39bfed8c..f16072f33cdb 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> @@ -10,7 +10,6 @@
>   #include <linux/phy/phy.h>
>   #include <linux/phy/phy-dp.h>
>   
> -#include "dp_clk_util.h"
>   #include "msm_drv.h"
>   
>   #define DP_LABEL "MDSS DP DISPLAY"
> @@ -106,6 +105,22 @@ struct dp_regulator_cfg {
>   	struct dp_reg_entry regs[DP_DEV_REGULATOR_MAX];
>   };
>   
> +enum dss_clk_type {
> +	DSS_CLK_AHB, /* no set rate. rate controlled through rpm */
> +	DSS_CLK_PCLK,
> +};
> +
> +struct dss_clk {
> +	enum dss_clk_type type;
> +	unsigned long rate;
> +};
> +
> +struct dss_module_power {
> +	unsigned int num_clk;
> +	struct clk_bulk_data *clocks;
> +	struct dss_clk *clk_config;
> +};
> +
>   /**
>    * struct dp_parser - DP parser's data exposed to clients
>    *
> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
> index b48b45e92bfa..87683071868d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_power.c
> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
> @@ -105,72 +105,69 @@ static int dp_power_clk_init(struct dp_power_private *power)
>   	ctrl = &power->parser->mp[DP_CTRL_PM];
>   	stream = &power->parser->mp[DP_STREAM_PM];
>   
> -	rc = msm_dss_get_clk(dev, core->clk_config, core->num_clk);
> +	rc = devm_clk_bulk_get(dev, core->num_clk, core->clocks);
>   	if (rc) {
>   		DRM_ERROR("failed to get %s clk. err=%d\n",
>   			dp_parser_pm_name(DP_CORE_PM), rc);
>   		return rc;
>   	}
>   
> -	rc = msm_dss_get_clk(dev, ctrl->clk_config, ctrl->num_clk);
> +	rc = devm_clk_bulk_get(dev, ctrl->num_clk, ctrl->clocks);
>   	if (rc) {
>   		DRM_ERROR("failed to get %s clk. err=%d\n",
>   			dp_parser_pm_name(DP_CTRL_PM), rc);
> -		msm_dss_put_clk(core->clk_config, core->num_clk);
>   		return -ENODEV;
>   	}
>   
> -	rc = msm_dss_get_clk(dev, stream->clk_config, stream->num_clk);
> +	rc = devm_clk_bulk_get(dev, stream->num_clk, stream->clocks);
>   	if (rc) {
>   		DRM_ERROR("failed to get %s clk. err=%d\n",
>   			dp_parser_pm_name(DP_CTRL_PM), rc);
> -		msm_dss_put_clk(core->clk_config, core->num_clk);
>   		return -ENODEV;
>   	}
>   
>   	return 0;
>   }
>   
> -static int dp_power_clk_deinit(struct dp_power_private *power)
> +static int dp_power_clk_set_link_rate(struct dp_power_private *power,
> +			struct dss_clk *clk_arry, int num_clk, int enable)
>   {
> -	struct dss_module_power *core, *ctrl, *stream;
> -
> -	core = &power->parser->mp[DP_CORE_PM];
> -	ctrl = &power->parser->mp[DP_CTRL_PM];
> -	stream = &power->parser->mp[DP_STREAM_PM];
> +	u32 rate;
> +	int i, rc = 0;
>   
> -	if (!core || !ctrl || !stream) {
> -		DRM_ERROR("invalid power_data\n");
> -		return -EINVAL;
> +	for (i = 0; i < num_clk; i++) {
> +		if (clk_arry[i].type == DSS_CLK_PCLK) {
> +			if (enable)
> +				rate = clk_arry[i].rate;
> +			else
> +				rate = 0;
> +
> +			rc = dev_pm_opp_set_rate(power->dev, rate);
> +			if (rc)
> +				break;
> +		}
>   	}
> -
> -	msm_dss_put_clk(ctrl->clk_config, ctrl->num_clk);
> -	msm_dss_put_clk(core->clk_config, core->num_clk);
> -	msm_dss_put_clk(stream->clk_config, stream->num_clk);
> -	return 0;
> +	return rc;
>   }
>   
> -static int dp_power_clk_set_link_rate(struct dp_power_private *power,
> -			struct dss_clk *clk_arry, int num_clk, int enable)
> +static int dp_clk_set_rate(struct dss_module_power *mp)
>   {
> -	u32 rate;
>   	int i, rc = 0;
> +	struct dss_clk *clk_arry = mp->clk_config;
>   
> -	for (i = 0; i < num_clk; i++) {
> -		if (clk_arry[i].clk) {
> -			if (clk_arry[i].type == DSS_CLK_PCLK) {
> -				if (enable)
> -					rate = clk_arry[i].rate;
> -				else
> -					rate = 0;
> -
> -				rc = dev_pm_opp_set_rate(power->dev, rate);
> -				if (rc)
> -					break;
> +	for (i = 0; i < mp->num_clk; i++) {
> +		if (clk_arry[i].type != DSS_CLK_AHB) {
> +			rc = clk_set_rate(mp->clocks[i].clk, mp->clk_config[i].rate);
> +			if (rc) {
> +				DRM_ERROR("%pS->%s: %s failed. rc=%d\n",
> +						__builtin_return_address(0),
> +						__func__,
> +						mp->clocks[i].id, rc);
> +				break;
>   			}
> -
>   		}
>   	}
> +
>   	return rc;
>   }
>   
> @@ -189,7 +186,7 @@ static int dp_power_clk_set_rate(struct dp_power_private *power,
>   	} else {
>   
>   		if (enable) {
> -			rc = msm_dss_clk_set_rate(mp->clk_config, mp->num_clk);
> +			rc = dp_clk_set_rate(mp);
>   			if (rc) {
>   				DRM_ERROR("failed to set clks rate\n");
>   				return rc;
> @@ -197,10 +194,14 @@ static int dp_power_clk_set_rate(struct dp_power_private *power,
>   		}
>   	}
>   
> -	rc = msm_dss_enable_clk(mp->clk_config, mp->num_clk, enable);
> -	if (rc) {
> -		DRM_ERROR("failed to %d clks, err: %d\n", enable, rc);
> -		return rc;
> +	if (enable) {
> +		rc = clk_bulk_prepare_enable(mp->num_clk, mp->clocks);
> +		if (rc) {
> +			DRM_ERROR("failed to enable clks, err: %d\n", rc);
> +			return rc;
> +		}
> +	} else {
> +		clk_bulk_disable_unprepare(mp->num_clk, mp->clocks);
>   	}
>   
>   	return 0;
> @@ -336,9 +337,7 @@ void dp_power_client_deinit(struct dp_power *dp_power)
>   
>   	power = container_of(dp_power, struct dp_power_private, dp_power);
>   
> -	dp_power_clk_deinit(power);
>   	pm_runtime_disable(&power->pdev->dev);
> -
>   }
>   
>   int dp_power_init(struct dp_power *dp_power, bool flip)


-- 
With best wishes
Dmitry

Re: [PATCH v2 2/2] drm/msm/dp: rewrite dss_module_power to use bulk clock functions

[Bjorn Andersson]

On Thu 25 Nov 20:35 CST 2021, Dmitry Baryshkov wrote:

> In order to simplify DP code, drop hand-coded loops over clock arrays,
> replacing them with clk_bulk_* functions.
> 

I've yet to debug this, but applying the two patches and attaching an
HDMI cable to my USB dongle results in the follwing splat on the 8350
HDK.

[   39.658840] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
[   39.667883] Mem abort info:
[   39.670774]   ESR = 0x96000006
[   39.673940]   EC = 0x25: DABT (current EL), IL = 32 bits
[   39.679417]   SET = 0, FnV = 0
[   39.682582]   EA = 0, S1PTW = 0
[   39.685825]   FSC = 0x06: level 2 translation fault
[   39.690851] Data abort info:
[   39.693838]   ISV = 0, ISS = 0x00000006
[   39.697797]   CM = 0, WnR = 0
[   39.700864] user pgtable: 4k pages, 48-bit VAs, pgdp=000000010eb8d000
[   39.707501] [0000000000000000] pgd=080000010f097003, p4d=080000010f097003, pud=080000010ba58003, pmd=0000000000000000
[   39.718425] Internal error: Oops: 96000006 [#1] PREEMPT SMP
[   39.724169] Modules linked in: pmic_glink_altmode qcom_pmic_glink_power cfg80211 rfkill 8021q garp mrp stp llc microchip lan78xx snd_soc_hdmi_codec pmic_glink pdr_interface rpmsg_char qrtr_smd qrtr fsa4480 qcom_q6v5_pas qcom_pil_info i2c_qcom_geni qcom_q6v5 msm qcom_sysmon qcom_stats gpu_sched crct10dif_ce drm_kms_helper qcom_common qcom_glink_smem gpucc_sm8350 phy_qcom_qmp mdt_loader typec ufs_qcom qmi_helpers qcom_rng socinfo qnoc_sm8350 rmtfs_mem fuse drm ipv6
[   39.766330] CPU: 0 PID: 85 Comm: kworker/0:3 Not tainted 5.16.0-rc5-next-20211215-00046-g2f90133452d9 #280
[   39.776256] Hardware name: Qualcomm Technologies, Inc. SM8350 HDK (DT)
[   39.782969] Workqueue: events pmic_glink_altmode_worker [pmic_glink_altmode]
[   39.790235] pstate: 40400005 (nZcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   39.797393] pc : __pi_strcmp+0x1c/0xf0
[   39.801271] lr : dp_ctrl_set_clock_rate+0x8c/0xf0 [msm]
[   39.806737] sp : ffff800008adbbc0
[   39.810153] x29: ffff800008adbbc0 x28: 0000000000000000 x27: 0000000000000000
[   39.817501] x26: ffffb60107f74270 x25: ffff297a49b33c80 x24: 00000000202fbf00
[   39.824846] x23: 0000000000000001 x22: ffff297a4b400580 x21: 0000000000000020
[   39.832188] x20: ffffb600e3d78068 x19: 0000000000000000 x18: ffffffffffffffff
[   39.839541] x17: 6b6c63206e6f2030 x16: ffffb601063476c0 x15: 0720072007200720
[   39.846893] x14: 0720072007200720 x13: ffffb60107c622c8 x12: 0000000000000765
[   39.854229] x11: 0000000000000277 x10: 0101010101010101 x9 : ffffb60107c622c8
[   39.861565] x8 : 00000000ffffefff x7 : 0000000000000000 x6 : ffffb60107cba2c8
[   39.868902] x5 : 0000000000000000 x4 : ffff297cbe8619d8 x3 : 0000000000000000
[   39.876240] x2 : 0000000000000000 x1 : ffffb600e3d78068 x0 : 0000000000000000
[   39.883594] Call trace:
[   39.886124]  __pi_strcmp+0x1c/0xf0
[   39.889638]  dp_ctrl_enable_mainlink_clocks+0x98/0x110 [msm]
[   39.895537]  dp_ctrl_on_link+0x98/0x3f0 [msm]
[   39.900096]  dp_display_process_hpd_high+0xa8/0x100 [msm]
[   39.905731]  dp_display_usbpd_attention_cb+0x164/0x1a4 [msm]
[   39.911629]  dp_hpd_oob_event+0x74/0xa4 [msm]
[   39.916195]  dp_display_oob_hotplug_event+0x1c/0x2c [msm]
[   39.921831]  dp_oob_hotplug_event+0x18/0x24 [msm]
[   39.926756]  drm_connector_oob_hotplug_event+0x40/0x60 [drm]
[   39.932686]  pmic_glink_altmode_worker+0x7c/0x194 [pmic_glink_altmode]
[   39.939398]  process_one_work+0x1d0/0x350
[   39.943526]  worker_thread+0x13c/0x460
[   39.947390]  kthread+0x17c/0x190
[   39.950722]  ret_from_fork+0x10/0x20
[   39.954416] Code: f24008ff 540003e1 f2400807 54000241 (f8408402)
[   39.960684] ---[ end trace 0000000000000000 ]---

Regards,
Bjorn

Re: [PATCH v2 2/2] drm/msm/dp: rewrite dss_module_power to use bulk clock functions

[Kuogee Hsieh]

On 12/28/2021 4:29 PM, Dmitry Baryshkov wrote:
> Kuogee,
>
> Some time ago you volonteered to check these two patches on a DP 
> hosts. Any progress?
>
> On 26/11/2021 05:35, Dmitry Baryshkov wrote:
>> In order to simplify DP code, drop hand-coded loops over clock arrays,
>> replacing them with clk_bulk_* functions.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Tested-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Reviewed-by: Kuogee Hsieh <quic_khsieh@quicinc.com>

>> ---
>>   drivers/gpu/drm/msm/Makefile         |   1 -
>>   drivers/gpu/drm/msm/dp/dp_clk_util.c | 120 ---------------------------
>>   drivers/gpu/drm/msm/dp/dp_clk_util.h |  38 ---------
>>   drivers/gpu/drm/msm/dp/dp_ctrl.c     |  19 ++---
>>   drivers/gpu/drm/msm/dp/dp_parser.c   |  21 ++++-
>>   drivers/gpu/drm/msm/dp/dp_parser.h   |  17 +++-
>>   drivers/gpu/drm/msm/dp/dp_power.c    |  81 +++++++++---------
>>   7 files changed, 83 insertions(+), 214 deletions(-)
>>   delete mode 100644 drivers/gpu/drm/msm/dp/dp_clk_util.c
>>   delete mode 100644 drivers/gpu/drm/msm/dp/dp_clk_util.h
>>
>> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
>> index b6637da219b0..ccacf604881a 100644
>> --- a/drivers/gpu/drm/msm/Makefile
>> +++ b/drivers/gpu/drm/msm/Makefile
>> @@ -104,7 +104,6 @@ msm-$(CONFIG_DRM_MSM_GPU_STATE)    += 
>> adreno/a6xx_gpu_state.o
>>     msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \
>>       dp/dp_catalog.o \
>> -    dp/dp_clk_util.o \
>>       dp/dp_ctrl.o \
>>       dp/dp_display.o \
>>       dp/dp_drm.o \
>> diff --git a/drivers/gpu/drm/msm/dp/dp_clk_util.c 
>> b/drivers/gpu/drm/msm/dp/dp_clk_util.c
>> deleted file mode 100644
>> index 44a4fc59ff31..000000000000
>> --- a/drivers/gpu/drm/msm/dp/dp_clk_util.c
>> +++ /dev/null
>> @@ -1,120 +0,0 @@
>> -// SPDX-License-Identifier: GPL-2.0-only
>> -/* Copyright (c) 2012-2015, 2017-2018, The Linux Foundation.
>> - * All rights reserved.
>> - */
>> -
>> -#include <linux/clk.h>
>> -#include <linux/clk/clk-conf.h>
>> -#include <linux/err.h>
>> -#include <linux/delay.h>
>> -#include <linux/of.h>
>> -
>> -#include <drm/drm_print.h>
>> -
>> -#include "dp_clk_util.h"
>> -
>> -void msm_dss_put_clk(struct dss_clk *clk_arry, int num_clk)
>> -{
>> -    int i;
>> -
>> -    for (i = num_clk - 1; i >= 0; i--) {
>> -        if (clk_arry[i].clk)
>> -            clk_put(clk_arry[i].clk);
>> -        clk_arry[i].clk = NULL;
>> -    }
>> -}
>> -
>> -int msm_dss_get_clk(struct device *dev, struct dss_clk *clk_arry, 
>> int num_clk)
>> -{
>> -    int i, rc = 0;
>> -
>> -    for (i = 0; i < num_clk; i++) {
>> -        clk_arry[i].clk = clk_get(dev, clk_arry[i].clk_name);
>> -        rc = PTR_ERR_OR_ZERO(clk_arry[i].clk);
>> -        if (rc) {
>> -            DEV_ERR("%pS->%s: '%s' get failed. rc=%d\n",
>> -                __builtin_return_address(0), __func__,
>> -                clk_arry[i].clk_name, rc);
>> -            goto error;
>> -        }
>> -    }
>> -
>> -    return rc;
>> -
>> -error:
>> -    for (i--; i >= 0; i--) {
>> -        if (clk_arry[i].clk)
>> -            clk_put(clk_arry[i].clk);
>> -        clk_arry[i].clk = NULL;
>> -    }
>> -
>> -    return rc;
>> -}
>> -
>> -int msm_dss_clk_set_rate(struct dss_clk *clk_arry, int num_clk)
>> -{
>> -    int i, rc = 0;
>> -
>> -    for (i = 0; i < num_clk; i++) {
>> -        if (clk_arry[i].clk) {
>> -            if (clk_arry[i].type != DSS_CLK_AHB) {
>> -                DEV_DBG("%pS->%s: '%s' rate %ld\n",
>> -                    __builtin_return_address(0), __func__,
>> -                    clk_arry[i].clk_name,
>> -                    clk_arry[i].rate);
>> -                rc = clk_set_rate(clk_arry[i].clk,
>> -                    clk_arry[i].rate);
>> -                if (rc) {
>> -                    DEV_ERR("%pS->%s: %s failed. rc=%d\n",
>> -                        __builtin_return_address(0),
>> -                        __func__,
>> -                        clk_arry[i].clk_name, rc);
>> -                    break;
>> -                }
>> -            }
>> -        } else {
>> -            DEV_ERR("%pS->%s: '%s' is not available\n",
>> -                __builtin_return_address(0), __func__,
>> -                clk_arry[i].clk_name);
>> -            rc = -EPERM;
>> -            break;
>> -        }
>> -    }
>> -
>> -    return rc;
>> -}
>> -
>> -int msm_dss_enable_clk(struct dss_clk *clk_arry, int num_clk, int 
>> enable)
>> -{
>> -    int i, rc = 0;
>> -
>> -    if (enable) {
>> -        for (i = 0; i < num_clk; i++) {
>> -            DEV_DBG("%pS->%s: enable '%s'\n",
>> -                __builtin_return_address(0), __func__,
>> -                clk_arry[i].clk_name);
>> -            rc = clk_prepare_enable(clk_arry[i].clk);
>> -            if (rc)
>> -                DEV_ERR("%pS->%s: %s en fail. rc=%d\n",
>> -                    __builtin_return_address(0),
>> -                    __func__,
>> -                    clk_arry[i].clk_name, rc);
>> -
>> -            if (rc && i) {
>> -                msm_dss_enable_clk(&clk_arry[i - 1],
>> -                    i - 1, false);
>> -                break;
>> -            }
>> -        }
>> -    } else {
>> -        for (i = num_clk - 1; i >= 0; i--) {
>> -            DEV_DBG("%pS->%s: disable '%s'\n",
>> -                __builtin_return_address(0), __func__,
>> -                clk_arry[i].clk_name);
>> -
>> -            clk_disable_unprepare(clk_arry[i].clk);
>> -        }
>> -    }
>> -
>> -    return rc;
>> -}
>> diff --git a/drivers/gpu/drm/msm/dp/dp_clk_util.h 
>> b/drivers/gpu/drm/msm/dp/dp_clk_util.h
>> deleted file mode 100644
>> index 6288a2833a58..000000000000
>> --- a/drivers/gpu/drm/msm/dp/dp_clk_util.h
>> +++ /dev/null
>> @@ -1,38 +0,0 @@
>> -/* SPDX-License-Identifier: GPL-2.0-only */
>> -/* Copyright (c) 2012, 2017-2018, The Linux Foundation. All rights 
>> reserved.
>> - */
>> -
>> -#ifndef __DPU_IO_UTIL_H__
>> -#define __DPU_IO_UTIL_H__
>> -
>> -#include <linux/platform_device.h>
>> -#include <linux/types.h>
>> -
>> -#define DEV_DBG(fmt, args...)   pr_debug(fmt, ##args)
>> -#define DEV_INFO(fmt, args...)  pr_info(fmt, ##args)
>> -#define DEV_WARN(fmt, args...)  pr_warn(fmt, ##args)
>> -#define DEV_ERR(fmt, args...)   pr_err(fmt, ##args)
>> -
>> -enum dss_clk_type {
>> -    DSS_CLK_AHB, /* no set rate. rate controlled through rpm */
>> -    DSS_CLK_PCLK,
>> -};
>> -
>> -struct dss_clk {
>> -    struct clk *clk; /* clk handle */
>> -    char clk_name[32];
>> -    enum dss_clk_type type;
>> -    unsigned long rate;
>> -    unsigned long max_rate;
>> -};
>> -
>> -struct dss_module_power {
>> -    unsigned int num_clk;
>> -    struct dss_clk *clk_config;
>> -};
>> -
>> -int msm_dss_get_clk(struct device *dev, struct dss_clk *clk_arry, 
>> int num_clk);
>> -void msm_dss_put_clk(struct dss_clk *clk_arry, int num_clk);
>> -int msm_dss_clk_set_rate(struct dss_clk *clk_arry, int num_clk);
>> -int msm_dss_enable_clk(struct dss_clk *clk_arry, int num_clk, int 
>> enable);
>> -#endif /* __DPU_IO_UTIL_H__ */
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index 62e75dc8afc6..e9a4d6c32f57 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -1289,20 +1289,19 @@ static int dp_ctrl_setup_main_link(struct 
>> dp_ctrl_private *ctrl,
>>   static void dp_ctrl_set_clock_rate(struct dp_ctrl_private *ctrl,
>>               enum dp_pm_type module, char *name, unsigned long rate)
>>   {
>> +    u32 i;
>>       u32 num = ctrl->parser->mp[module].num_clk;
>> -    struct dss_clk *cfg = ctrl->parser->mp[module].clk_config;
>> -
>> -    while (num && strcmp(cfg->clk_name, name)) {
>> -        num--;
>> -        cfg++;
>> -    }
>>         DRM_DEBUG_DP("setting rate=%lu on clk=%s\n", rate, name);
>>   -    if (num)
>> -        cfg->rate = rate;
>> -    else
>> -        DRM_ERROR("%s clock doesn't exit to set rate %lu\n",
>> +    for (i = 0; i < num; i++) {
>> +        if (!strcmp(ctrl->parser->mp[module].clocks[i].id, name)) {
>> +            ctrl->parser->mp[module].clk_config[i].rate = rate;
>> +            return;
>> +        }
>> +    }
>> +
>> +    DRM_ERROR("%s clock doesn't exit to set rate %lu\n",
>>                   name, rate);
>>   }
>>   diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c 
>> b/drivers/gpu/drm/msm/dp/dp_parser.c
>> index a7acc23f742b..0fe726913b4e 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
>> @@ -162,6 +162,11 @@ static int dp_parser_init_clk_data(struct 
>> dp_parser *parser)
>>       }
>>         core_power->num_clk = core_clk_count;
>> +    core_power->clocks = devm_kcalloc(dev,
>> +            core_power->num_clk, sizeof(struct clk_bulk_data),
>> +            GFP_KERNEL);
>> +    if (!core_power->clocks)
>> +        return -ENOMEM;
>>       core_power->clk_config = devm_kzalloc(dev,
>>               sizeof(struct dss_clk) * core_power->num_clk,
>>               GFP_KERNEL);
>> @@ -175,6 +180,11 @@ static int dp_parser_init_clk_data(struct 
>> dp_parser *parser)
>>       }
>>         ctrl_power->num_clk = ctrl_clk_count;
>> +    ctrl_power->clocks = devm_kcalloc(dev,
>> +            ctrl_power->num_clk, sizeof(struct clk_bulk_data),
>> +            GFP_KERNEL);
>> +    if (!ctrl_power->clocks)
>> +        return -ENOMEM;
>>       ctrl_power->clk_config = devm_kzalloc(dev,
>>               sizeof(struct dss_clk) * ctrl_power->num_clk,
>>               GFP_KERNEL);
>> @@ -190,6 +200,11 @@ static int dp_parser_init_clk_data(struct 
>> dp_parser *parser)
>>       }
>>         stream_power->num_clk = stream_clk_count;
>> +    stream_power->clocks = devm_kcalloc(dev,
>> +            stream_power->num_clk, sizeof(struct clk_bulk_data),
>> +            GFP_KERNEL);
>> +    if (!stream_power->clocks)
>> +        return -ENOMEM;
>>       stream_power->clk_config = devm_kzalloc(dev,
>>               sizeof(struct dss_clk) * stream_power->num_clk,
>>               GFP_KERNEL);
>> @@ -236,21 +251,21 @@ static int dp_parser_clock(struct dp_parser 
>> *parser)
>>                   core_clk_index < core_clk_count) {
>>               struct dss_clk *clk =
>> &core_power->clk_config[core_clk_index];
>> -            strlcpy(clk->clk_name, clk_name, sizeof(clk->clk_name));
>> +            core_power->clocks[i].id = devm_kstrdup(dev, clk_name, 
>> GFP_KERNEL);
>>               clk->type = DSS_CLK_AHB;
>>               core_clk_index++;
>>           } else if (dp_parser_check_prefix("stream", clk_name) &&
>>                   stream_clk_index < stream_clk_count) {
>>               struct dss_clk *clk =
>> &stream_power->clk_config[stream_clk_index];
>> -            strlcpy(clk->clk_name, clk_name, sizeof(clk->clk_name));
>> +            stream_power->clocks[i].id = devm_kstrdup(dev, clk_name, 
>> GFP_KERNEL);
>>               clk->type = DSS_CLK_PCLK;
>>               stream_clk_index++;
>>           } else if (dp_parser_check_prefix("ctrl", clk_name) &&
>>                  ctrl_clk_index < ctrl_clk_count) {
>>               struct dss_clk *clk =
>> &ctrl_power->clk_config[ctrl_clk_index];
>> -            strlcpy(clk->clk_name, clk_name, sizeof(clk->clk_name));
>> +            ctrl_power->clocks[i].id = devm_kstrdup(dev, clk_name, 
>> GFP_KERNEL);
>>               ctrl_clk_index++;
>>               if (dp_parser_check_prefix("ctrl_link", clk_name) ||
>>                   dp_parser_check_prefix("stream_pixel", clk_name))
>> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h 
>> b/drivers/gpu/drm/msm/dp/dp_parser.h
>> index 094b39bfed8c..f16072f33cdb 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
>> @@ -10,7 +10,6 @@
>>   #include <linux/phy/phy.h>
>>   #include <linux/phy/phy-dp.h>
>>   -#include "dp_clk_util.h"
>>   #include "msm_drv.h"
>>     #define DP_LABEL "MDSS DP DISPLAY"
>> @@ -106,6 +105,22 @@ struct dp_regulator_cfg {
>>       struct dp_reg_entry regs[DP_DEV_REGULATOR_MAX];
>>   };
>>   +enum dss_clk_type {
>> +    DSS_CLK_AHB, /* no set rate. rate controlled through rpm */
>> +    DSS_CLK_PCLK,
>> +};
>> +
>> +struct dss_clk {
>> +    enum dss_clk_type type;
>> +    unsigned long rate;
>> +};
>> +
>> +struct dss_module_power {
>> +    unsigned int num_clk;
>> +    struct clk_bulk_data *clocks;
>> +    struct dss_clk *clk_config;
>> +};
>> +
>>   /**
>>    * struct dp_parser - DP parser's data exposed to clients
>>    *
>> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c 
>> b/drivers/gpu/drm/msm/dp/dp_power.c
>> index b48b45e92bfa..87683071868d 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_power.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
>> @@ -105,72 +105,69 @@ static int dp_power_clk_init(struct 
>> dp_power_private *power)
>>       ctrl = &power->parser->mp[DP_CTRL_PM];
>>       stream = &power->parser->mp[DP_STREAM_PM];
>>   -    rc = msm_dss_get_clk(dev, core->clk_config, core->num_clk);
>> +    rc = devm_clk_bulk_get(dev, core->num_clk, core->clocks);
>>       if (rc) {
>>           DRM_ERROR("failed to get %s clk. err=%d\n",
>>               dp_parser_pm_name(DP_CORE_PM), rc);
>>           return rc;
>>       }
>>   -    rc = msm_dss_get_clk(dev, ctrl->clk_config, ctrl->num_clk);
>> +    rc = devm_clk_bulk_get(dev, ctrl->num_clk, ctrl->clocks);
>>       if (rc) {
>>           DRM_ERROR("failed to get %s clk. err=%d\n",
>>               dp_parser_pm_name(DP_CTRL_PM), rc);
>> -        msm_dss_put_clk(core->clk_config, core->num_clk);
>>           return -ENODEV;
>>       }
>>   -    rc = msm_dss_get_clk(dev, stream->clk_config, stream->num_clk);
>> +    rc = devm_clk_bulk_get(dev, stream->num_clk, stream->clocks);
>>       if (rc) {
>>           DRM_ERROR("failed to get %s clk. err=%d\n",
>>               dp_parser_pm_name(DP_CTRL_PM), rc);
>> -        msm_dss_put_clk(core->clk_config, core->num_clk);
>>           return -ENODEV;
>>       }
>>         return 0;
>>   }
>>   -static int dp_power_clk_deinit(struct dp_power_private *power)
>> +static int dp_power_clk_set_link_rate(struct dp_power_private *power,
>> +            struct dss_clk *clk_arry, int num_clk, int enable)
>>   {
>> -    struct dss_module_power *core, *ctrl, *stream;
>> -
>> -    core = &power->parser->mp[DP_CORE_PM];
>> -    ctrl = &power->parser->mp[DP_CTRL_PM];
>> -    stream = &power->parser->mp[DP_STREAM_PM];
>> +    u32 rate;
>> +    int i, rc = 0;
>>   -    if (!core || !ctrl || !stream) {
>> -        DRM_ERROR("invalid power_data\n");
>> -        return -EINVAL;
>> +    for (i = 0; i < num_clk; i++) {
>> +        if (clk_arry[i].type == DSS_CLK_PCLK) {
>> +            if (enable)
>> +                rate = clk_arry[i].rate;
>> +            else
>> +                rate = 0;
>> +
>> +            rc = dev_pm_opp_set_rate(power->dev, rate);
>> +            if (rc)
>> +                break;
>> +        }
>>       }
>> -
>> -    msm_dss_put_clk(ctrl->clk_config, ctrl->num_clk);
>> -    msm_dss_put_clk(core->clk_config, core->num_clk);
>> -    msm_dss_put_clk(stream->clk_config, stream->num_clk);
>> -    return 0;
>> +    return rc;
>>   }
>>   -static int dp_power_clk_set_link_rate(struct dp_power_private *power,
>> -            struct dss_clk *clk_arry, int num_clk, int enable)
>> +static int dp_clk_set_rate(struct dss_module_power *mp)
>>   {
>> -    u32 rate;
>>       int i, rc = 0;
>> +    struct dss_clk *clk_arry = mp->clk_config;
>>   -    for (i = 0; i < num_clk; i++) {
>> -        if (clk_arry[i].clk) {
>> -            if (clk_arry[i].type == DSS_CLK_PCLK) {
>> -                if (enable)
>> -                    rate = clk_arry[i].rate;
>> -                else
>> -                    rate = 0;
>> -
>> -                rc = dev_pm_opp_set_rate(power->dev, rate);
>> -                if (rc)
>> -                    break;
>> +    for (i = 0; i < mp->num_clk; i++) {
>> +        if (clk_arry[i].type != DSS_CLK_AHB) {
>> +            rc = clk_set_rate(mp->clocks[i].clk, 
>> mp->clk_config[i].rate);
>> +            if (rc) {
>> +                DRM_ERROR("%pS->%s: %s failed. rc=%d\n",
>> +                        __builtin_return_address(0),
>> +                        __func__,
>> +                        mp->clocks[i].id, rc);
>> +                break;
>>               }
>> -
>>           }
>>       }
>> +
>>       return rc;
>>   }
>>   @@ -189,7 +186,7 @@ static int dp_power_clk_set_rate(struct 
>> dp_power_private *power,
>>       } else {
>>             if (enable) {
>> -            rc = msm_dss_clk_set_rate(mp->clk_config, mp->num_clk);
>> +            rc = dp_clk_set_rate(mp);
>>               if (rc) {
>>                   DRM_ERROR("failed to set clks rate\n");
>>                   return rc;
>> @@ -197,10 +194,14 @@ static int dp_power_clk_set_rate(struct 
>> dp_power_private *power,
>>           }
>>       }
>>   -    rc = msm_dss_enable_clk(mp->clk_config, mp->num_clk, enable);
>> -    if (rc) {
>> -        DRM_ERROR("failed to %d clks, err: %d\n", enable, rc);
>> -        return rc;
>> +    if (enable) {
>> +        rc = clk_bulk_prepare_enable(mp->num_clk, mp->clocks);
>> +        if (rc) {
>> +            DRM_ERROR("failed to enable clks, err: %d\n", rc);
>> +            return rc;
>> +        }
>> +    } else {
>> +        clk_bulk_disable_unprepare(mp->num_clk, mp->clocks);
>>       }
>>         return 0;
>> @@ -336,9 +337,7 @@ void dp_power_client_deinit(struct dp_power 
>> *dp_power)
>>         power = container_of(dp_power, struct dp_power_private, 
>> dp_power);
>>   -    dp_power_clk_deinit(power);
>>       pm_runtime_disable(&power->pdev->dev);
>> -
>>   }
>>     int dp_power_init(struct dp_power *dp_power, bool flip)
>
>

Re: [PATCH v2 2/2] drm/msm/dp: rewrite dss_module_power to use bulk clock functions

[Dmitry Baryshkov]

HI,

On Wed, 29 Dec 2021 at 07:12, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Thu 25 Nov 20:35 CST 2021, Dmitry Baryshkov wrote:
>
> > In order to simplify DP code, drop hand-coded loops over clock arrays,
> > replacing them with clk_bulk_* functions.
> >
>
> I've yet to debug this, but applying the two patches and attaching an
> HDMI cable to my USB dongle results in the follwing splat on the 8350
> HDK.

Intersesting. The only major difference between original code and the
patches code in this function is the removal of `if (clk_arry[i].clk)`
condition in that function. Could yyou please check whether clocks are
properly parsed at the time you receive the hpd event?

If  we can not debug this issue,  I'd then propose to merge first
patch and let somebody else rewrite dp_clk_util to use clk_bulk_data.

>
> [   39.658840] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> [   39.667883] Mem abort info:
> [   39.670774]   ESR = 0x96000006
> [   39.673940]   EC = 0x25: DABT (current EL), IL = 32 bits
> [   39.679417]   SET = 0, FnV = 0
> [   39.682582]   EA = 0, S1PTW = 0
> [   39.685825]   FSC = 0x06: level 2 translation fault
> [   39.690851] Data abort info:
> [   39.693838]   ISV = 0, ISS = 0x00000006
> [   39.697797]   CM = 0, WnR = 0
> [   39.700864] user pgtable: 4k pages, 48-bit VAs, pgdp=000000010eb8d000
> [   39.707501] [0000000000000000] pgd=080000010f097003, p4d=080000010f097003, pud=080000010ba58003, pmd=0000000000000000
> [   39.718425] Internal error: Oops: 96000006 [#1] PREEMPT SMP
> [   39.724169] Modules linked in: pmic_glink_altmode qcom_pmic_glink_power cfg80211 rfkill 8021q garp mrp stp llc microchip lan78xx snd_soc_hdmi_codec pmic_glink pdr_interface rpmsg_char qrtr_smd qrtr fsa4480 qcom_q6v5_pas qcom_pil_info i2c_qcom_geni qcom_q6v5 msm qcom_sysmon qcom_stats gpu_sched crct10dif_ce drm_kms_helper qcom_common qcom_glink_smem gpucc_sm8350 phy_qcom_qmp mdt_loader typec ufs_qcom qmi_helpers qcom_rng socinfo qnoc_sm8350 rmtfs_mem fuse drm ipv6
> [   39.766330] CPU: 0 PID: 85 Comm: kworker/0:3 Not tainted 5.16.0-rc5-next-20211215-00046-g2f90133452d9 #280
> [   39.776256] Hardware name: Qualcomm Technologies, Inc. SM8350 HDK (DT)
> [   39.782969] Workqueue: events pmic_glink_altmode_worker [pmic_glink_altmode]
> [   39.790235] pstate: 40400005 (nZcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [   39.797393] pc : __pi_strcmp+0x1c/0xf0
> [   39.801271] lr : dp_ctrl_set_clock_rate+0x8c/0xf0 [msm]
> [   39.806737] sp : ffff800008adbbc0
> [   39.810153] x29: ffff800008adbbc0 x28: 0000000000000000 x27: 0000000000000000
> [   39.817501] x26: ffffb60107f74270 x25: ffff297a49b33c80 x24: 00000000202fbf00
> [   39.824846] x23: 0000000000000001 x22: ffff297a4b400580 x21: 0000000000000020
> [   39.832188] x20: ffffb600e3d78068 x19: 0000000000000000 x18: ffffffffffffffff
> [   39.839541] x17: 6b6c63206e6f2030 x16: ffffb601063476c0 x15: 0720072007200720
> [   39.846893] x14: 0720072007200720 x13: ffffb60107c622c8 x12: 0000000000000765
> [   39.854229] x11: 0000000000000277 x10: 0101010101010101 x9 : ffffb60107c622c8
> [   39.861565] x8 : 00000000ffffefff x7 : 0000000000000000 x6 : ffffb60107cba2c8
> [   39.868902] x5 : 0000000000000000 x4 : ffff297cbe8619d8 x3 : 0000000000000000
> [   39.876240] x2 : 0000000000000000 x1 : ffffb600e3d78068 x0 : 0000000000000000
> [   39.883594] Call trace:
> [   39.886124]  __pi_strcmp+0x1c/0xf0
> [   39.889638]  dp_ctrl_enable_mainlink_clocks+0x98/0x110 [msm]
> [   39.895537]  dp_ctrl_on_link+0x98/0x3f0 [msm]
> [   39.900096]  dp_display_process_hpd_high+0xa8/0x100 [msm]
> [   39.905731]  dp_display_usbpd_attention_cb+0x164/0x1a4 [msm]
> [   39.911629]  dp_hpd_oob_event+0x74/0xa4 [msm]
> [   39.916195]  dp_display_oob_hotplug_event+0x1c/0x2c [msm]
> [   39.921831]  dp_oob_hotplug_event+0x18/0x24 [msm]
> [   39.926756]  drm_connector_oob_hotplug_event+0x40/0x60 [drm]
> [   39.932686]  pmic_glink_altmode_worker+0x7c/0x194 [pmic_glink_altmode]
> [   39.939398]  process_one_work+0x1d0/0x350
> [   39.943526]  worker_thread+0x13c/0x460
> [   39.947390]  kthread+0x17c/0x190
> [   39.950722]  ret_from_fork+0x10/0x20
> [   39.954416] Code: f24008ff 540003e1 f2400807 54000241 (f8408402)
> [   39.960684] ---[ end trace 0000000000000000 ]---
>
> Regards,
> Bjorn



-- 
With best wishes
Dmitry

Re: [PATCH v2 2/2] drm/msm/dp: rewrite dss_module_power to use bulk clock functions

[Dmitry Baryshkov]

On Fri, 31 Dec 2021 at 07:48, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> HI,
>
> On Wed, 29 Dec 2021 at 07:12, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > On Thu 25 Nov 20:35 CST 2021, Dmitry Baryshkov wrote:
> >
> > > In order to simplify DP code, drop hand-coded loops over clock arrays,
> > > replacing them with clk_bulk_* functions.
> > >
> >
> > I've yet to debug this, but applying the two patches and attaching an
> > HDMI cable to my USB dongle results in the follwing splat on the 8350
> > HDK.
>
> Intersesting. The only major difference between original code and the
> patches code in this function is the removal of `if (clk_arry[i].clk)`
> condition in that function. Could yyou please check whether clocks are
> properly parsed at the time you receive the hpd event?

s/parsed/dp_power_clk_init called/

>
> If  we can not debug this issue,  I'd then propose to merge first
> patch and let somebody else rewrite dp_clk_util to use clk_bulk_data.
>
> >
> > [   39.658840] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> > [   39.667883] Mem abort info:
> > [   39.670774]   ESR = 0x96000006
> > [   39.673940]   EC = 0x25: DABT (current EL), IL = 32 bits
> > [   39.679417]   SET = 0, FnV = 0
> > [   39.682582]   EA = 0, S1PTW = 0
> > [   39.685825]   FSC = 0x06: level 2 translation fault
> > [   39.690851] Data abort info:
> > [   39.693838]   ISV = 0, ISS = 0x00000006
> > [   39.697797]   CM = 0, WnR = 0
> > [   39.700864] user pgtable: 4k pages, 48-bit VAs, pgdp=000000010eb8d000
> > [   39.707501] [0000000000000000] pgd=080000010f097003, p4d=080000010f097003, pud=080000010ba58003, pmd=0000000000000000
> > [   39.718425] Internal error: Oops: 96000006 [#1] PREEMPT SMP
> > [   39.724169] Modules linked in: pmic_glink_altmode qcom_pmic_glink_power cfg80211 rfkill 8021q garp mrp stp llc microchip lan78xx snd_soc_hdmi_codec pmic_glink pdr_interface rpmsg_char qrtr_smd qrtr fsa4480 qcom_q6v5_pas qcom_pil_info i2c_qcom_geni qcom_q6v5 msm qcom_sysmon qcom_stats gpu_sched crct10dif_ce drm_kms_helper qcom_common qcom_glink_smem gpucc_sm8350 phy_qcom_qmp mdt_loader typec ufs_qcom qmi_helpers qcom_rng socinfo qnoc_sm8350 rmtfs_mem fuse drm ipv6
> > [   39.766330] CPU: 0 PID: 85 Comm: kworker/0:3 Not tainted 5.16.0-rc5-next-20211215-00046-g2f90133452d9 #280
> > [   39.776256] Hardware name: Qualcomm Technologies, Inc. SM8350 HDK (DT)
> > [   39.782969] Workqueue: events pmic_glink_altmode_worker [pmic_glink_altmode]
> > [   39.790235] pstate: 40400005 (nZcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > [   39.797393] pc : __pi_strcmp+0x1c/0xf0
> > [   39.801271] lr : dp_ctrl_set_clock_rate+0x8c/0xf0 [msm]
> > [   39.806737] sp : ffff800008adbbc0
> > [   39.810153] x29: ffff800008adbbc0 x28: 0000000000000000 x27: 0000000000000000
> > [   39.817501] x26: ffffb60107f74270 x25: ffff297a49b33c80 x24: 00000000202fbf00
> > [   39.824846] x23: 0000000000000001 x22: ffff297a4b400580 x21: 0000000000000020
> > [   39.832188] x20: ffffb600e3d78068 x19: 0000000000000000 x18: ffffffffffffffff
> > [   39.839541] x17: 6b6c63206e6f2030 x16: ffffb601063476c0 x15: 0720072007200720
> > [   39.846893] x14: 0720072007200720 x13: ffffb60107c622c8 x12: 0000000000000765
> > [   39.854229] x11: 0000000000000277 x10: 0101010101010101 x9 : ffffb60107c622c8
> > [   39.861565] x8 : 00000000ffffefff x7 : 0000000000000000 x6 : ffffb60107cba2c8
> > [   39.868902] x5 : 0000000000000000 x4 : ffff297cbe8619d8 x3 : 0000000000000000
> > [   39.876240] x2 : 0000000000000000 x1 : ffffb600e3d78068 x0 : 0000000000000000
> > [   39.883594] Call trace:
> > [   39.886124]  __pi_strcmp+0x1c/0xf0
> > [   39.889638]  dp_ctrl_enable_mainlink_clocks+0x98/0x110 [msm]
> > [   39.895537]  dp_ctrl_on_link+0x98/0x3f0 [msm]
> > [   39.900096]  dp_display_process_hpd_high+0xa8/0x100 [msm]
> > [   39.905731]  dp_display_usbpd_attention_cb+0x164/0x1a4 [msm]
> > [   39.911629]  dp_hpd_oob_event+0x74/0xa4 [msm]
> > [   39.916195]  dp_display_oob_hotplug_event+0x1c/0x2c [msm]
> > [   39.921831]  dp_oob_hotplug_event+0x18/0x24 [msm]
> > [   39.926756]  drm_connector_oob_hotplug_event+0x40/0x60 [drm]
> > [   39.932686]  pmic_glink_altmode_worker+0x7c/0x194 [pmic_glink_altmode]
> > [   39.939398]  process_one_work+0x1d0/0x350
> > [   39.943526]  worker_thread+0x13c/0x460
> > [   39.947390]  kthread+0x17c/0x190
> > [   39.950722]  ret_from_fork+0x10/0x20
> > [   39.954416] Code: f24008ff 540003e1 f2400807 54000241 (f8408402)
> > [   39.960684] ---[ end trace 0000000000000000 ]---
> >
> > Regards,
> > Bjorn
>
>
>
> --
> With best wishes
> Dmitry



-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH v2 1/2] drm/msm/dpu: simplify clocks handling

[Jessica Zhang]

On 11/25/2021 6:35 PM, Dmitry Baryshkov wrote:
> DPU driver contains code to parse clock items from device tree into
> special data struct and then enable/disable/set rate for the clocks
> using that data struct. However the DPU driver itself uses only parsing
> and enabling/disabling part (the rate setting is used by DP driver).
> 
> Move this implementation to the DP driver (which actually uses rate
> setting) and replace hand-coded enable/disable/get loops in the DPU
> with the respective clk_bulk operations. Put operation is removed
> completely because, it is handled using devres instead.
> 
> DP implementation is unchanged for now.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/Makefile                  |  2 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 24 ++-----
>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  6 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       | 46 +++----------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h       |  4 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c      | 26 +++----
>   .../dpu1/dpu_io_util.c => dp/dp_clk_util.c}   | 69 +------------------
>   .../dpu1/dpu_io_util.h => dp/dp_clk_util.h}   |  2 -
>   drivers/gpu/drm/msm/dp/dp_parser.h            |  2 +-
>   drivers/gpu/drm/msm/msm_drv.c                 | 49 +++++++++++++
>   drivers/gpu/drm/msm/msm_drv.h                 |  1 +
>   11 files changed, 84 insertions(+), 147 deletions(-)
>   rename drivers/gpu/drm/msm/{disp/dpu1/dpu_io_util.c => dp/dp_clk_util.c} (61%)
>   rename drivers/gpu/drm/msm/{disp/dpu1/dpu_io_util.h => dp/dp_clk_util.h} (92%)
> 
> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> index 40577f8856d8..b6637da219b0 100644
> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -69,7 +69,6 @@ msm-y := \
>   	disp/dpu1/dpu_hw_top.o \
>   	disp/dpu1/dpu_hw_util.o \
>   	disp/dpu1/dpu_hw_vbif.o \
> -	disp/dpu1/dpu_io_util.o \
>   	disp/dpu1/dpu_kms.o \
>   	disp/dpu1/dpu_mdss.o \
>   	disp/dpu1/dpu_plane.o \
> @@ -105,6 +104,7 @@ msm-$(CONFIG_DRM_MSM_GPU_STATE)	+= adreno/a6xx_gpu_state.o
>   
>   msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \
>   	dp/dp_catalog.o \
> +	dp/dp_clk_util.o \
>   	dp/dp_ctrl.o \
>   	dp/dp_display.o \
>   	dp/dp_drm.o \
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> index 60fe06018581..4d184122d63e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> @@ -284,17 +284,6 @@ void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc)
>   	}
>   }
>   
> -static int _dpu_core_perf_set_core_clk_rate(struct dpu_kms *kms, u64 rate)
> -{
> -	struct dss_clk *core_clk = kms->perf.core_clk;
> -
> -	if (core_clk->max_rate && (rate > core_clk->max_rate))
> -		rate = core_clk->max_rate;
> -
> -	core_clk->rate = rate;
> -	return dev_pm_opp_set_rate(&kms->pdev->dev, core_clk->rate);
> -}
> -
>   static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
>   {
>   	u64 clk_rate = kms->perf.perf_tune.min_core_clk;
> @@ -306,7 +295,7 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
>   			dpu_cstate = to_dpu_crtc_state(crtc->state);
>   			clk_rate = max(dpu_cstate->new_perf.core_clk_rate,
>   							clk_rate);
> -			clk_rate = clk_round_rate(kms->perf.core_clk->clk,
> +			clk_rate = clk_round_rate(kms->perf.core_clk,
>   					clk_rate);
>   		}
>   	}
> @@ -405,10 +394,11 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>   
>   		trace_dpu_core_perf_update_clk(kms->dev, stop_req, clk_rate);
>   
> -		ret = _dpu_core_perf_set_core_clk_rate(kms, clk_rate);
> +		if (clk_rate > kms->perf.max_core_clk_rate)
> +			clk_rate = kms->perf.max_core_clk_rate;
> +		ret = dev_pm_opp_set_rate(&kms->pdev->dev, clk_rate);
>   		if (ret) {
> -			DPU_ERROR("failed to set %s clock rate %llu\n",
> -					kms->perf.core_clk->clk_name, clk_rate);
> +			DPU_ERROR("failed to set core clock rate %llu\n", clk_rate);
>   			return ret;
>   		}
>   
> @@ -529,13 +519,13 @@ void dpu_core_perf_destroy(struct dpu_core_perf *perf)
>   int dpu_core_perf_init(struct dpu_core_perf *perf,
>   		struct drm_device *dev,
>   		struct dpu_mdss_cfg *catalog,
> -		struct dss_clk *core_clk)
> +		struct clk *core_clk)
>   {
>   	perf->dev = dev;
>   	perf->catalog = catalog;
>   	perf->core_clk = core_clk;
>   
> -	perf->max_core_clk_rate = core_clk->max_rate;
> +	perf->max_core_clk_rate = clk_get_rate(core_clk);
>   	if (!perf->max_core_clk_rate) {
>   		DPU_DEBUG("optional max core clk rate, use default\n");
>   		perf->max_core_clk_rate = DPU_PERF_DEFAULT_MAX_CORE_CLK_RATE;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> index cf4b9b5964c6..8dfcc6db7176 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> @@ -56,7 +56,7 @@ struct dpu_core_perf_tune {
>    * @dev: Pointer to drm device
>    * @debugfs_root: top level debug folder
>    * @catalog: Pointer to catalog configuration
> - * @core_clk: Pointer to core clock structure
> + * @core_clk: Pointer to the core clock
>    * @core_clk_rate: current core clock rate
>    * @max_core_clk_rate: maximum allowable core clock rate
>    * @perf_tune: debug control for performance tuning
> @@ -69,7 +69,7 @@ struct dpu_core_perf {
>   	struct drm_device *dev;
>   	struct dentry *debugfs_root;
>   	struct dpu_mdss_cfg *catalog;
> -	struct dss_clk *core_clk;
> +	struct clk *core_clk;
>   	u64 core_clk_rate;
>   	u64 max_core_clk_rate;
>   	struct dpu_core_perf_tune perf_tune;
> @@ -120,7 +120,7 @@ void dpu_core_perf_destroy(struct dpu_core_perf *perf);
>   int dpu_core_perf_init(struct dpu_core_perf *perf,
>   		struct drm_device *dev,
>   		struct dpu_mdss_cfg *catalog,
> -		struct dss_clk *core_clk);
> +		struct clk *core_clk);
>   
>   struct dpu_kms;
>   
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index a15b26428280..655cbd912309 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -936,29 +936,15 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms)
>   	return 0;
>   }
>   
> -static struct dss_clk *_dpu_kms_get_clk(struct dpu_kms *dpu_kms,
> -		char *clock_name)
> -{
> -	struct dss_module_power *mp = &dpu_kms->mp;
> -	int i;
> -
> -	for (i = 0; i < mp->num_clk; i++) {
> -		if (!strcmp(mp->clk_config[i].clk_name, clock_name))
> -			return &mp->clk_config[i];
> -	}
> -
> -	return NULL;
> -}
> -
>   u64 dpu_kms_get_clk_rate(struct dpu_kms *dpu_kms, char *clock_name)
>   {
> -	struct dss_clk *clk;
> +	struct clk *clk;
>   
> -	clk = _dpu_kms_get_clk(dpu_kms, clock_name);
> +	clk = msm_clk_bulk_get_clock(dpu_kms->clocks, dpu_kms->num_clocks, clock_name);
>   	if (!clk)
>   		return -EINVAL;
>   
> -	return clk_get_rate(clk->clk);
> +	return clk_get_rate(clk);
>   }
>   
>   static int dpu_kms_hw_init(struct msm_kms *kms)
> @@ -1070,7 +1056,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
>   	}
>   
>   	rc = dpu_core_perf_init(&dpu_kms->perf, dev, dpu_kms->catalog,
> -			_dpu_kms_get_clk(dpu_kms, "core"));
> +			msm_clk_bulk_get_clock(dpu_kms->clocks, dpu_kms->num_clocks, "core"));
>   	if (rc) {
>   		DPU_ERROR("failed to init perf %d\n", rc);
>   		goto perf_err;
> @@ -1157,7 +1143,6 @@ static int dpu_bind(struct device *dev, struct device *master, void *data)
>   	struct platform_device *pdev = to_platform_device(dev);
>   	struct msm_drm_private *priv = ddev->dev_private;
>   	struct dpu_kms *dpu_kms;
> -	struct dss_module_power *mp;
>   	int ret = 0;
>   
>   	dpu_kms = devm_kzalloc(&pdev->dev, sizeof(*dpu_kms), GFP_KERNEL);
> @@ -1174,12 +1159,12 @@ static int dpu_bind(struct device *dev, struct device *master, void *data)
>   		return ret;
>   	}
>   
> -	mp = &dpu_kms->mp;
> -	ret = msm_dss_parse_clock(pdev, mp);
> -	if (ret) {
> +	ret = msm_parse_clock(pdev, &dpu_kms->clocks);
> +	if (ret < 0) {
>   		DPU_ERROR("failed to parse clocks, ret=%d\n", ret);
>   		return ret;
>   	}
> +	dpu_kms->num_clocks = ret;
>   
>   	platform_set_drvdata(pdev, dpu_kms);
>   
> @@ -1203,11 +1188,6 @@ static void dpu_unbind(struct device *dev, struct device *master, void *data)
>   {
>   	struct platform_device *pdev = to_platform_device(dev);
>   	struct dpu_kms *dpu_kms = platform_get_drvdata(pdev);
> -	struct dss_module_power *mp = &dpu_kms->mp;
> -
> -	msm_dss_put_clk(mp->clk_config, mp->num_clk);
> -	devm_kfree(&pdev->dev, mp->clk_config);
> -	mp->num_clk = 0;
>   
>   	if (dpu_kms->rpm_enabled)
>   		pm_runtime_disable(&pdev->dev);
> @@ -1231,21 +1211,18 @@ static int dpu_dev_remove(struct platform_device *pdev)
>   
>   static int __maybe_unused dpu_runtime_suspend(struct device *dev)
>   {
> -	int i, rc = -1;
> +	int i;
>   	struct platform_device *pdev = to_platform_device(dev);
>   	struct dpu_kms *dpu_kms = platform_get_drvdata(pdev);
> -	struct dss_module_power *mp = &dpu_kms->mp;
>   
>   	/* Drop the performance state vote */
>   	dev_pm_opp_set_rate(dev, 0);
> -	rc = msm_dss_enable_clk(mp->clk_config, mp->num_clk, false);
> -	if (rc)
> -		DPU_ERROR("clock disable failed rc:%d\n", rc);
> +	clk_bulk_disable_unprepare(dpu_kms->num_clocks, dpu_kms->clocks);
>   
>   	for (i = 0; i < dpu_kms->num_paths; i++)
>   		icc_set_bw(dpu_kms->path[i], 0, 0);
>   
> -	return rc;
> +	return 0;
>   }
>   
>   static int __maybe_unused dpu_runtime_resume(struct device *dev)
> @@ -1255,7 +1232,6 @@ static int __maybe_unused dpu_runtime_resume(struct device *dev)
>   	struct dpu_kms *dpu_kms = platform_get_drvdata(pdev);
>   	struct drm_encoder *encoder;
>   	struct drm_device *ddev;
> -	struct dss_module_power *mp = &dpu_kms->mp;
>   	int i;
>   
>   	ddev = dpu_kms->dev;
> @@ -1265,7 +1241,7 @@ static int __maybe_unused dpu_runtime_resume(struct device *dev)
>   	for (i = 0; i < dpu_kms->num_paths; i++)
>   		icc_set_bw(dpu_kms->path[i], 0, Bps_to_icc(MIN_IB_BW));
>   
> -	rc = msm_dss_enable_clk(mp->clk_config, mp->num_clk, true);
> +	rc = clk_bulk_prepare_enable(dpu_kms->num_clocks, dpu_kms->clocks);
>   	if (rc) {
>   		DPU_ERROR("clock enable failed rc:%d\n", rc);
>   		return rc;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> index 775bcbda860f..d366aa359d38 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> @@ -21,7 +21,6 @@
>   #include "dpu_hw_lm.h"
>   #include "dpu_hw_interrupts.h"
>   #include "dpu_hw_top.h"
> -#include "dpu_io_util.h"
>   #include "dpu_rm.h"
>   #include "dpu_core_perf.h"
>   
> @@ -113,7 +112,8 @@ struct dpu_kms {
>   	struct platform_device *pdev;
>   	bool rpm_enabled;
>   
> -	struct dss_module_power mp;
> +	struct clk_bulk_data *clocks;
> +	int num_clocks;
>   
>   	/* reference count bandwidth requests, so we know when we can
>   	 * release bandwidth.  Each atomic update increments, and frame-
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> index b466784d9822..d7faf11a5456 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> @@ -29,7 +29,8 @@ struct dpu_irq_controller {
>   struct dpu_mdss {
>   	struct msm_mdss base;
>   	void __iomem *mmio;
> -	struct dss_module_power mp;
> +	struct clk_bulk_data *clocks;
> +	int num_clocks;
>   	struct dpu_irq_controller irq_controller;
>   };
>   
> @@ -136,10 +137,9 @@ static void _dpu_mdss_irq_domain_fini(struct dpu_mdss *dpu_mdss)
>   static int dpu_mdss_enable(struct msm_mdss *mdss)
>   {
>   	struct dpu_mdss *dpu_mdss = to_dpu_mdss(mdss);
> -	struct dss_module_power *mp = &dpu_mdss->mp;
>   	int ret;
>   
> -	ret = msm_dss_enable_clk(mp->clk_config, mp->num_clk, true);
> +	ret = clk_bulk_prepare_enable(dpu_mdss->num_clocks, dpu_mdss->clocks);
>   	if (ret) {
>   		DPU_ERROR("clock enable failed, ret:%d\n", ret);
>   		return ret;
> @@ -174,14 +174,10 @@ static int dpu_mdss_enable(struct msm_mdss *mdss)
>   static int dpu_mdss_disable(struct msm_mdss *mdss)
>   {
>   	struct dpu_mdss *dpu_mdss = to_dpu_mdss(mdss);
> -	struct dss_module_power *mp = &dpu_mdss->mp;
> -	int ret;
>   
> -	ret = msm_dss_enable_clk(mp->clk_config, mp->num_clk, false);
> -	if (ret)
> -		DPU_ERROR("clock disable failed, ret:%d\n", ret);
> +	clk_bulk_disable_unprepare(dpu_mdss->num_clocks, dpu_mdss->clocks);
>   
> -	return ret;
> +	return 0;
>   }
>   
>   static void dpu_mdss_destroy(struct drm_device *dev)

Hi Dmitry,

Looks like this is based on some outdated code:
2027e5b3 (drm/msm: Initialize MDSS irq domain at probe time) changes 
`*dev` to `*mdss`

I want to test this patch on some boards (namely RB3 and RB5). Can you 
release a version with your changes rebased on top of the tip of msm-next?

> @@ -189,7 +185,6 @@ static void dpu_mdss_destroy(struct drm_device *dev)
>   	struct platform_device *pdev = to_platform_device(dev->dev);
>   	struct msm_drm_private *priv = dev->dev_private;
>   	struct dpu_mdss *dpu_mdss = to_dpu_mdss(priv->mdss);
> -	struct dss_module_power *mp = &dpu_mdss->mp;
>   	int irq;
>   
>   	pm_runtime_suspend(dev->dev);
> @@ -197,8 +192,6 @@ static void dpu_mdss_destroy(struct drm_device *dev)
>   	_dpu_mdss_irq_domain_fini(dpu_mdss);
>   	irq = platform_get_irq(pdev, 0);
>   	irq_set_chained_handler_and_data(irq, NULL, NULL);
> -	msm_dss_put_clk(mp->clk_config, mp->num_clk);
> -	devm_kfree(&pdev->dev, mp->clk_config);
>   
>   	if (dpu_mdss->mmio)
>   		devm_iounmap(&pdev->dev, dpu_mdss->mmio);
> @@ -217,7 +210,6 @@ int dpu_mdss_init(struct drm_device *dev)
>   	struct platform_device *pdev = to_platform_device(dev->dev);
>   	struct msm_drm_private *priv = dev->dev_private;
>   	struct dpu_mdss *dpu_mdss;
> -	struct dss_module_power *mp;
>   	int ret;
>   	int irq;
>   
> @@ -231,12 +223,12 @@ int dpu_mdss_init(struct drm_device *dev)
>   
>   	DRM_DEBUG("mapped mdss address space @%pK\n", dpu_mdss->mmio);
>   
> -	mp = &dpu_mdss->mp;
> -	ret = msm_dss_parse_clock(pdev, mp);
> -	if (ret) {
> +	ret = msm_parse_clock(pdev, &dpu_mdss->clocks);
> +	if (ret < 0) {
>   		DPU_ERROR("failed to parse clocks, ret=%d\n", ret);
>   		goto clk_parse_err;
>   	}
> +	dpu_mdss->num_clocks = ret;
>   
>   	dpu_mdss->base.dev = dev;
>   	dpu_mdss->base.funcs = &mdss_funcs;
> @@ -263,9 +255,7 @@ int dpu_mdss_init(struct drm_device *dev)
>   irq_error:
>   	_dpu_mdss_irq_domain_fini(dpu_mdss);
>   irq_domain_error:
> -	msm_dss_put_clk(mp->clk_config, mp->num_clk);
>   clk_parse_err:
> -	devm_kfree(&pdev->dev, mp->clk_config);
>   	if (dpu_mdss->mmio)
>   		devm_iounmap(&pdev->dev, dpu_mdss->mmio);
>   	dpu_mdss->mmio = NULL;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c b/drivers/gpu/drm/msm/dp/dp_clk_util.c
> similarity index 61%
> rename from drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c
> rename to drivers/gpu/drm/msm/dp/dp_clk_util.c
> index 078afc5f5882..44a4fc59ff31 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c
> +++ b/drivers/gpu/drm/msm/dp/dp_clk_util.c
> @@ -11,7 +11,7 @@
>   
>   #include <drm/drm_print.h>
>   
> -#include "dpu_io_util.h"
> +#include "dp_clk_util.h"
>   
>   void msm_dss_put_clk(struct dss_clk *clk_arry, int num_clk)
>   {
> @@ -118,70 +118,3 @@ int msm_dss_enable_clk(struct dss_clk *clk_arry, int num_clk, int enable)
>   
>   	return rc;
>   }
> -
> -int msm_dss_parse_clock(struct platform_device *pdev,
> -			struct dss_module_power *mp)
> -{
> -	u32 i, rc = 0;
> -	const char *clock_name;
> -	int num_clk = 0;
> -
> -	if (!pdev || !mp)
> -		return -EINVAL;
> -
> -	mp->num_clk = 0;
> -	num_clk = of_property_count_strings(pdev->dev.of_node, "clock-names");
> -	if (num_clk <= 0) {
> -		pr_debug("clocks are not defined\n");
> -		return 0;
> -	}
> -
> -	mp->clk_config = devm_kcalloc(&pdev->dev,
> -				      num_clk, sizeof(struct dss_clk),
> -				      GFP_KERNEL);
> -	if (!mp->clk_config)
> -		return -ENOMEM;
> -
> -	for (i = 0; i < num_clk; i++) {
> -		rc = of_property_read_string_index(pdev->dev.of_node,
> -						   "clock-names", i,
> -						   &clock_name);
> -		if (rc) {
> -			DRM_DEV_ERROR(&pdev->dev, "Failed to get clock name for %d\n",
> -				i);
> -			break;
> -		}
> -		strlcpy(mp->clk_config[i].clk_name, clock_name,
> -			sizeof(mp->clk_config[i].clk_name));
> -
> -		mp->clk_config[i].type = DSS_CLK_AHB;
> -	}
> -
> -	rc = msm_dss_get_clk(&pdev->dev, mp->clk_config, num_clk);
> -	if (rc) {
> -		DRM_DEV_ERROR(&pdev->dev, "Failed to get clock refs %d\n", rc);
> -		goto err;
> -	}
> -
> -	rc = of_clk_set_defaults(pdev->dev.of_node, false);
> -	if (rc) {
> -		DRM_DEV_ERROR(&pdev->dev, "Failed to set clock defaults %d\n", rc);
> -		goto err;
> -	}
> -
> -	for (i = 0; i < num_clk; i++) {
> -		u32 rate = clk_get_rate(mp->clk_config[i].clk);
> -		if (!rate)
> -			continue;
> -		mp->clk_config[i].rate = rate;
> -		mp->clk_config[i].type = DSS_CLK_PCLK;
> -		mp->clk_config[i].max_rate = rate;
> -	}
> -
> -	mp->num_clk = num_clk;
> -	return 0;
> -
> -err:
> -	msm_dss_put_clk(mp->clk_config, num_clk);
> -	return rc;
> -}
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h b/drivers/gpu/drm/msm/dp/dp_clk_util.h
> similarity index 92%
> rename from drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h
> rename to drivers/gpu/drm/msm/dp/dp_clk_util.h
> index e6b5c772fa3b..6288a2833a58 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h
> +++ b/drivers/gpu/drm/msm/dp/dp_clk_util.h
> @@ -35,6 +35,4 @@ int msm_dss_get_clk(struct device *dev, struct dss_clk *clk_arry, int num_clk);
>   void msm_dss_put_clk(struct dss_clk *clk_arry, int num_clk);
>   int msm_dss_clk_set_rate(struct dss_clk *clk_arry, int num_clk);
>   int msm_dss_enable_clk(struct dss_clk *clk_arry, int num_clk, int enable);
> -int msm_dss_parse_clock(struct platform_device *pdev,
> -		struct dss_module_power *mp);
>   #endif /* __DPU_IO_UTIL_H__ */
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
> index 3172da089421..094b39bfed8c 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> @@ -10,7 +10,7 @@
>   #include <linux/phy/phy.h>
>   #include <linux/phy/phy-dp.h>
>   
> -#include "dpu_io_util.h"
> +#include "dp_clk_util.h"
>   #include "msm_drv.h"
>   
>   #define DP_LABEL "MDSS DP DISPLAY"
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 892c04365239..3e90fca33581 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -5,6 +5,7 @@
>    * Author: Rob Clark <robdclark@gmail.com>
>    */
>   
> +#include <linux/clk/clk-conf.h>
>   #include <linux/dma-mapping.h>
>   #include <linux/kthread.h>
>   #include <linux/sched/mm.h>
> @@ -123,6 +124,54 @@ struct clk *msm_clk_get(struct platform_device *pdev, const char *name)
>   	return clk;
>   }
>   
> +int msm_parse_clock(struct platform_device *pdev, struct clk_bulk_data **clocks)

Can you also move msm_parse_clock and other io helper methods (like 
_msm_ioremap) into a separate msm_io_utils file instead? That would help 
avoid file bloat.

Thanks,

Jessica Zhang

> +{
> +	u32 i, rc = 0;
> +	const char *clock_name;
> +	struct clk_bulk_data *bulk;
> +	int num_clk = 0;
> +
> +	if (!pdev)
> +		return -EINVAL;
> +
> +	num_clk = of_property_count_strings(pdev->dev.of_node, "clock-names");
> +	if (num_clk <= 0) {
> +		pr_debug("clocks are not defined\n");
> +		return 0;
> +	}
> +
> +	bulk = devm_kcalloc(&pdev->dev, num_clk, sizeof(struct clk_bulk_data), GFP_KERNEL);
> +	if (!bulk)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_clk; i++) {
> +		rc = of_property_read_string_index(pdev->dev.of_node,
> +						   "clock-names", i,
> +						   &clock_name);
> +		if (rc) {
> +			DRM_DEV_ERROR(&pdev->dev, "Failed to get clock name for %d\n", i);
> +			return rc;
> +		}
> +		bulk[i].id = devm_kstrdup(&pdev->dev, clock_name, GFP_KERNEL);
> +	}
> +
> +	rc = devm_clk_bulk_get(&pdev->dev, num_clk, bulk);
> +	if (rc) {
> +		DRM_DEV_ERROR(&pdev->dev, "Failed to get clock refs %d\n", rc);
> +		return rc;
> +	}
> +
> +	rc = of_clk_set_defaults(pdev->dev.of_node, false);
> +	if (rc) {
> +		DRM_DEV_ERROR(&pdev->dev, "Failed to set clock defaults %d\n", rc);
> +		return rc;
> +	}
> +
> +	*clocks = bulk;
> +
> +	return num_clk;
> +}
> +
>   static void __iomem *_msm_ioremap(struct platform_device *pdev, const char *name,
>   				  const char *dbgname, bool quiet, phys_addr_t *psize)
>   {
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 69952b239384..cfede901056d 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -477,6 +477,7 @@ struct clk *msm_clk_get(struct platform_device *pdev, const char *name);
>   
>   struct clk *msm_clk_bulk_get_clock(struct clk_bulk_data *bulk, int count,
>   	const char *name);
> +int msm_parse_clock(struct platform_device *pdev, struct clk_bulk_data **clocks);
>   void __iomem *msm_ioremap(struct platform_device *pdev, const char *name,
>   		const char *dbgname);
>   void __iomem *msm_ioremap_size(struct platform_device *pdev, const char *name,
> -- 
> 2.33.0
> 

Re: [Freedreno] [PATCH v2 1/2] drm/msm/dpu: simplify clocks handling

[Dmitry Baryshkov]

On 19/01/2022 05:32, Jessica Zhang wrote:
> On 11/25/2021 6:35 PM, Dmitry Baryshkov wrote:
>> DPU driver contains code to parse clock items from device tree into
>> special data struct and then enable/disable/set rate for the clocks
>> using that data struct. However the DPU driver itself uses only parsing
>> and enabling/disabling part (the rate setting is used by DP driver).
>>
>> Move this implementation to the DP driver (which actually uses rate
>> setting) and replace hand-coded enable/disable/get loops in the DPU
>> with the respective clk_bulk operations. Put operation is removed
>> completely because, it is handled using devres instead.
>>
>> DP implementation is unchanged for now.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/Makefile                  |  2 +-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 24 ++-----
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  6 +-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       | 46 +++----------
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h       |  4 +-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c      | 26 +++----
>>   .../dpu1/dpu_io_util.c => dp/dp_clk_util.c}   | 69 +------------------
>>   .../dpu1/dpu_io_util.h => dp/dp_clk_util.h}   |  2 -
>>   drivers/gpu/drm/msm/dp/dp_parser.h            |  2 +-
>>   drivers/gpu/drm/msm/msm_drv.c                 | 49 +++++++++++++
>>   drivers/gpu/drm/msm/msm_drv.h                 |  1 +
>>   11 files changed, 84 insertions(+), 147 deletions(-)
>>   rename drivers/gpu/drm/msm/{disp/dpu1/dpu_io_util.c => 
>> dp/dp_clk_util.c} (61%)
>>   rename drivers/gpu/drm/msm/{disp/dpu1/dpu_io_util.h => 
>> dp/dp_clk_util.h} (92%)
>>

[skipped]

>> @@ -174,14 +174,10 @@ static int dpu_mdss_enable(struct msm_mdss *mdss)
>>   static int dpu_mdss_disable(struct msm_mdss *mdss)
>>   {
>>       struct dpu_mdss *dpu_mdss = to_dpu_mdss(mdss);
>> -    struct dss_module_power *mp = &dpu_mdss->mp;
>> -    int ret;
>> -    ret = msm_dss_enable_clk(mp->clk_config, mp->num_clk, false);
>> -    if (ret)
>> -        DPU_ERROR("clock disable failed, ret:%d\n", ret);
>> +    clk_bulk_disable_unprepare(dpu_mdss->num_clocks, dpu_mdss->clocks);
>> -    return ret;
>> +    return 0;
>>   }
>>   static void dpu_mdss_destroy(struct drm_device *dev)
> 
> Hi Dmitry,
> 
> Looks like this is based on some outdated code:
> 2027e5b3 (drm/msm: Initialize MDSS irq domain at probe time) changes 
> `*dev` to `*mdss`
> 
> I want to test this patch on some boards (namely RB3 and RB5). Can you 
> release a version with your changes rebased on top of the tip of msm-next?

Sure, I'll post v3.

> 
>> @@ -189,7 +185,6 @@ static void dpu_mdss_destroy(struct drm_device *dev)
>>       struct platform_device *pdev = to_platform_device(dev->dev);
>>       struct msm_drm_private *priv = dev->dev_private;
>>       struct dpu_mdss *dpu_mdss = to_dpu_mdss(priv->mdss);
>> -    struct dss_module_power *mp = &dpu_mdss->mp;
>>       int irq;
>>       pm_runtime_suspend(dev->dev);
>> @@ -197,8 +192,6 @@ static void dpu_mdss_destroy(struct drm_device *dev)
>>       _dpu_mdss_irq_domain_fini(dpu_mdss);
>>       irq = platform_get_irq(pdev, 0);
>>       irq_set_chained_handler_and_data(irq, NULL, NULL);
>> -    msm_dss_put_clk(mp->clk_config, mp->num_clk);
>> -    devm_kfree(&pdev->dev, mp->clk_config);
>>       if (dpu_mdss->mmio)
>>           devm_iounmap(&pdev->dev, dpu_mdss->mmio);
>> @@ -217,7 +210,6 @@ int dpu_mdss_init(struct drm_device *dev)
>>       struct platform_device *pdev = to_platform_device(dev->dev);
>>       struct msm_drm_private *priv = dev->dev_private;
>>       struct dpu_mdss *dpu_mdss;
>> -    struct dss_module_power *mp;
>>       int ret;
>>       int irq;
>> @@ -231,12 +223,12 @@ int dpu_mdss_init(struct drm_device *dev)
>>       DRM_DEBUG("mapped mdss address space @%pK\n", dpu_mdss->mmio);
>> -    mp = &dpu_mdss->mp;
>> -    ret = msm_dss_parse_clock(pdev, mp);
>> -    if (ret) {
>> +    ret = msm_parse_clock(pdev, &dpu_mdss->clocks);
>> +    if (ret < 0) {
>>           DPU_ERROR("failed to parse clocks, ret=%d\n", ret);
>>           goto clk_parse_err;
>>       }
>> +    dpu_mdss->num_clocks = ret;
>>       dpu_mdss->base.dev = dev;
>>       dpu_mdss->base.funcs = &mdss_funcs;
>> @@ -263,9 +255,7 @@ int dpu_mdss_init(struct drm_device *dev)
>>   irq_error:
>>       _dpu_mdss_irq_domain_fini(dpu_mdss);
>>   irq_domain_error:
>> -    msm_dss_put_clk(mp->clk_config, mp->num_clk);
>>   clk_parse_err:
>> -    devm_kfree(&pdev->dev, mp->clk_config);
>>       if (dpu_mdss->mmio)
>>           devm_iounmap(&pdev->dev, dpu_mdss->mmio);
>>       dpu_mdss->mmio = NULL;
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c 
>> b/drivers/gpu/drm/msm/dp/dp_clk_util.c
>> similarity index 61%
>> rename from drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c
>> rename to drivers/gpu/drm/msm/dp/dp_clk_util.c
>> index 078afc5f5882..44a4fc59ff31 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_clk_util.c
>> @@ -11,7 +11,7 @@
>>   #include <drm/drm_print.h>
>> -#include "dpu_io_util.h"
>> +#include "dp_clk_util.h"
>>   void msm_dss_put_clk(struct dss_clk *clk_arry, int num_clk)
>>   {
>> @@ -118,70 +118,3 @@ int msm_dss_enable_clk(struct dss_clk *clk_arry, 
>> int num_clk, int enable)
>>       return rc;
>>   }
>> -
>> -int msm_dss_parse_clock(struct platform_device *pdev,
>> -            struct dss_module_power *mp)
>> -{
>> -    u32 i, rc = 0;
>> -    const char *clock_name;
>> -    int num_clk = 0;
>> -
>> -    if (!pdev || !mp)
>> -        return -EINVAL;
>> -
>> -    mp->num_clk = 0;
>> -    num_clk = of_property_count_strings(pdev->dev.of_node, 
>> "clock-names");
>> -    if (num_clk <= 0) {
>> -        pr_debug("clocks are not defined\n");
>> -        return 0;
>> -    }
>> -
>> -    mp->clk_config = devm_kcalloc(&pdev->dev,
>> -                      num_clk, sizeof(struct dss_clk),
>> -                      GFP_KERNEL);
>> -    if (!mp->clk_config)
>> -        return -ENOMEM;
>> -
>> -    for (i = 0; i < num_clk; i++) {
>> -        rc = of_property_read_string_index(pdev->dev.of_node,
>> -                           "clock-names", i,
>> -                           &clock_name);
>> -        if (rc) {
>> -            DRM_DEV_ERROR(&pdev->dev, "Failed to get clock name for 
>> %d\n",
>> -                i);
>> -            break;
>> -        }
>> -        strlcpy(mp->clk_config[i].clk_name, clock_name,
>> -            sizeof(mp->clk_config[i].clk_name));
>> -
>> -        mp->clk_config[i].type = DSS_CLK_AHB;
>> -    }
>> -
>> -    rc = msm_dss_get_clk(&pdev->dev, mp->clk_config, num_clk);
>> -    if (rc) {
>> -        DRM_DEV_ERROR(&pdev->dev, "Failed to get clock refs %d\n", rc);
>> -        goto err;
>> -    }
>> -
>> -    rc = of_clk_set_defaults(pdev->dev.of_node, false);
>> -    if (rc) {
>> -        DRM_DEV_ERROR(&pdev->dev, "Failed to set clock defaults 
>> %d\n", rc);
>> -        goto err;
>> -    }
>> -
>> -    for (i = 0; i < num_clk; i++) {
>> -        u32 rate = clk_get_rate(mp->clk_config[i].clk);
>> -        if (!rate)
>> -            continue;
>> -        mp->clk_config[i].rate = rate;
>> -        mp->clk_config[i].type = DSS_CLK_PCLK;
>> -        mp->clk_config[i].max_rate = rate;
>> -    }
>> -
>> -    mp->num_clk = num_clk;
>> -    return 0;
>> -
>> -err:
>> -    msm_dss_put_clk(mp->clk_config, num_clk);
>> -    return rc;
>> -}
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h 
>> b/drivers/gpu/drm/msm/dp/dp_clk_util.h
>> similarity index 92%
>> rename from drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h
>> rename to drivers/gpu/drm/msm/dp/dp_clk_util.h
>> index e6b5c772fa3b..6288a2833a58 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_clk_util.h
>> @@ -35,6 +35,4 @@ int msm_dss_get_clk(struct device *dev, struct 
>> dss_clk *clk_arry, int num_clk);
>>   void msm_dss_put_clk(struct dss_clk *clk_arry, int num_clk);
>>   int msm_dss_clk_set_rate(struct dss_clk *clk_arry, int num_clk);
>>   int msm_dss_enable_clk(struct dss_clk *clk_arry, int num_clk, int 
>> enable);
>> -int msm_dss_parse_clock(struct platform_device *pdev,
>> -        struct dss_module_power *mp);
>>   #endif /* __DPU_IO_UTIL_H__ */
>> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h 
>> b/drivers/gpu/drm/msm/dp/dp_parser.h
>> index 3172da089421..094b39bfed8c 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
>> @@ -10,7 +10,7 @@
>>   #include <linux/phy/phy.h>
>>   #include <linux/phy/phy-dp.h>
>> -#include "dpu_io_util.h"
>> +#include "dp_clk_util.h"
>>   #include "msm_drv.h"
>>   #define DP_LABEL "MDSS DP DISPLAY"
>> diff --git a/drivers/gpu/drm/msm/msm_drv.c 
>> b/drivers/gpu/drm/msm/msm_drv.c
>> index 892c04365239..3e90fca33581 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.c
>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>> @@ -5,6 +5,7 @@
>>    * Author: Rob Clark <robdclark@gmail.com>
>>    */
>> +#include <linux/clk/clk-conf.h>
>>   #include <linux/dma-mapping.h>
>>   #include <linux/kthread.h>
>>   #include <linux/sched/mm.h>
>> @@ -123,6 +124,54 @@ struct clk *msm_clk_get(struct platform_device 
>> *pdev, const char *name)
>>       return clk;
>>   }
>> +int msm_parse_clock(struct platform_device *pdev, struct 
>> clk_bulk_data **clocks)
> 
> Can you also move msm_parse_clock and other io helper methods (like 
> _msm_ioremap) into a separate msm_io_utils file instead? That would help 
> avoid file bloat.

Nice idea!

> Thanks,
> 
> Jessica Zhang
> 
>> +{
>> +    u32 i, rc = 0;
>> +    const char *clock_name;
>> +    struct clk_bulk_data *bulk;
>> +    int num_clk = 0;
>> +
>> +    if (!pdev)
>> +        return -EINVAL;
>> +
>> +    num_clk = of_property_count_strings(pdev->dev.of_node, 
>> "clock-names");
>> +    if (num_clk <= 0) {
>> +        pr_debug("clocks are not defined\n");
>> +        return 0;
>> +    }
>> +
>> +    bulk = devm_kcalloc(&pdev->dev, num_clk, sizeof(struct 
>> clk_bulk_data), GFP_KERNEL);
>> +    if (!bulk)
>> +        return -ENOMEM;
>> +
>> +    for (i = 0; i < num_clk; i++) {
>> +        rc = of_property_read_string_index(pdev->dev.of_node,
>> +                           "clock-names", i,
>> +                           &clock_name);
>> +        if (rc) {
>> +            DRM_DEV_ERROR(&pdev->dev, "Failed to get clock name for 
>> %d\n", i);
>> +            return rc;
>> +        }
>> +        bulk[i].id = devm_kstrdup(&pdev->dev, clock_name, GFP_KERNEL);
>> +    }
>> +
>> +    rc = devm_clk_bulk_get(&pdev->dev, num_clk, bulk);
>> +    if (rc) {
>> +        DRM_DEV_ERROR(&pdev->dev, "Failed to get clock refs %d\n", rc);
>> +        return rc;
>> +    }
>> +
>> +    rc = of_clk_set_defaults(pdev->dev.of_node, false);
>> +    if (rc) {
>> +        DRM_DEV_ERROR(&pdev->dev, "Failed to set clock defaults 
>> %d\n", rc);
>> +        return rc;
>> +    }
>> +
>> +    *clocks = bulk;
>> +
>> +    return num_clk;
>> +}
>> +
>>   static void __iomem *_msm_ioremap(struct platform_device *pdev, 
>> const char *name,
>>                     const char *dbgname, bool quiet, phys_addr_t *psize)
>>   {
>> diff --git a/drivers/gpu/drm/msm/msm_drv.h 
>> b/drivers/gpu/drm/msm/msm_drv.h
>> index 69952b239384..cfede901056d 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.h
>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>> @@ -477,6 +477,7 @@ struct clk *msm_clk_get(struct platform_device 
>> *pdev, const char *name);
>>   struct clk *msm_clk_bulk_get_clock(struct clk_bulk_data *bulk, int 
>> count,
>>       const char *name);
>> +int msm_parse_clock(struct platform_device *pdev, struct 
>> clk_bulk_data **clocks);
>>   void __iomem *msm_ioremap(struct platform_device *pdev, const char 
>> *name,
>>           const char *dbgname);
>>   void __iomem *msm_ioremap_size(struct platform_device *pdev, const 
>> char *name,
>> -- 
>> 2.33.0
>>


-- 
With best wishes
Dmitry