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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 16/02/2022 05:35, Stephen Boyd wrote:
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.

The clocks are stored in three different structures, because dp_power enables and disables them separately. See dp_power_clk_enable() and dp_ctrl.c. This devm_clk_bulk_get_all is out of question.

On the other hand, we can call set_rate on disabled clocks. Let me see if we can get this into manageble state.



         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.

Yep. Tried to keep the origin code here.


+                               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;
  }



--
With best wishes
Dmitry



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux