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

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

 



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@xxxxxxxxxx>
---
  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@xxxxxxxxx>
   */
+#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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux