[Freedreno] [PATCH v2 1/2] drm/msm/dpu: simplify clocks handling
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Wed Jan 19 20:31:05 UTC 2022
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 at 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 at 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
>>
--
With best wishes
Dmitry
More information about the dri-devel
mailing list