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@linaro.org
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@gmail.com */ +#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