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

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

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