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

Quoting Dmitry Baryshkov (2022-01-31 13:05:13)

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..318e1f8629cb 100644 --- a/drivers/gpu/drm/msm/dp/dp_power.c +++ b/drivers/gpu/drm/msm/dp/dp_power.c @@ -105,59 +105,40 @@ 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);
  

Could we go further and devm_clk_bulk_get_all() and then either have some new clk API that goes through the bulk list and finds the one named something and hands over a pointer to it, think "clk_get() on top of clk_bulk_data", or just get the clk again that you want to set a rate on and have two pointers but otherwise it's a don't care. Then we wouldn't need to do much at all here to store the rate settable clk and find it in a loop.

    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) -{

•   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];
  

•   if (!core || !ctrl || !stream) {
  

•           DRM_ERROR("invalid power_data\n");
  

•           return -EINVAL;
  

•   }
  

•   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;
  

-}

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 *mp, int enable)
  

{

•   struct dss_clk *clk_arry = mp->clk_config;
  

•   int num_clk = mp->num_clk;
    u32 rate;
    int i, rc = 0;
  
    for (i = 0; i < num_clk; i++) {
  

•           if (clk_arry[i].clk) {
  

•           if (mp->clocks[i].clk) {
                    if (clk_arry[i].type == DSS_CLK_PCLK) {
                            if (enable)
                                    rate = clk_arry[i].rate;
  

@@ -168,9 +149,49 @@ static int dp_power_clk_set_link_rate(struct dp_power_private *power, if (rc) break; }

•           } else {
  

•                   DRM_ERROR("%pS->%s: '%s' is not available\n",
  

•                           __builtin_return_address(0), __func__,
  

•                           mp->clocks[i].id);
  

•                   rc = -EPERM;
  

•                   break;
  

•           }
  

•   }
  

•   return rc;
  

+}

+static int dp_clk_set_rate(struct dss_module_power *mp) +{

•   struct dss_clk *clk_arry = mp->clk_config;
  

•   int num_clk = mp->num_clk;
  

•   int i, rc = 0;
  
  

•   for (i = 0; i < num_clk; i++) {
  

•           if (mp->clocks[i].clk) {
  

•                   if (clk_arry[i].type != DSS_CLK_AHB) {
  

This loops is gross.

•                           DRM_DEBUG("%pS->%s: '%s' rate %ld\n",
  

•                                   __builtin_return_address(0), __func__,
  

•                                   mp->clocks[i].id,
  

•                                   clk_arry[i].rate);
  

•                           rc = clk_set_rate(mp->clocks[i].clk,
  

•                                   clk_arry[i].rate);
  

•                           if (rc) {
  

•                                   DRM_ERROR("%pS->%s: %s failed. rc=%d\n",
  

•                                           __builtin_return_address(0),
  

•                                           __func__,
  

•                                           mp->clocks[i].id, rc);
  

•                                   break;
  

•                           }
  

•                   }
  

•           } else {
  

•                   DRM_ERROR("%pS->%s: '%s' is not available\n",
  

•                           __builtin_return_address(0), __func__,
  

•                           mp->clocks[i].id);
  

•                   rc = -EPERM;
  

•                   break;
            }
    }
  

•   return rc;
  

}