[PATCH v2 2/2] drm/msm/dp: rewrite dss_module_power to use bulk clock functions
Kuogee Hsieh
quic_khsieh at quicinc.com
Wed Dec 29 22:03:15 UTC 2021
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 at linaro.org>
Tested-by: Kuogee Hsieh <quic_khsieh at quicinc.com>
Reviewed-by: Kuogee Hsieh <quic_khsieh at 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)
>
>
More information about the dri-devel
mailing list