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

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)