Re: [Freedreno] [PATCH v2 3/4] drm/msm: get rid of msm_iomap_size

abhinavk@xxxxxxxxxxxxxx · Tue, 27 Apr 2021 15:12:33 -0700

On 2021-04-27 13:32, Dmitry Baryshkov wrote:
On Tue, 27 Apr 2021 at 22:30, <abhinavk@xxxxxxxxxxxxxx> wrote:

Hi Dmitry

On 2021-04-26 17:18, Dmitry Baryshkov wrote:
> Instead of looping throught the resources each time to get the DSI CTRL
> area size, get it at the ioremap time.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
We will have to call into the individual modules anyway everytime we
take a snapshot as only they have access to the required clocks and 
the
base address.

So even though there is nothing wrong with this change, it still adds 
a
size member
which can be avoided because we have to call into the module anyway.

Any strong preference to store the size as opposed to just getting it
when we take
the snapshot?

Locality. We ioremap the resource from one piece of code and then we
get it's length from a completely different piece of code. If we ever
change e.g. the ioremap'ed resource name, we'd have to also update
snapshot users.
With this patch these changes are done in a transparent way. Whichever
we do with the regions in future, there is always a valid base + size
combo.

Alright, no further concerns from my side on this:

Reviewed-by: Abhinav Kumar <abhinavk@xxxxxxxxxxxxxx>

> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c |  5 +++--
>  drivers/gpu/drm/msm/msm_drv.c      | 27 +++++++++------------------
>  drivers/gpu/drm/msm/msm_drv.h      |  3 ++-
>  3 files changed, 14 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 1a63368c3912..b3ee5c0bce12 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -102,6 +102,7 @@ struct msm_dsi_host {
>       int id;
>
>       void __iomem *ctrl_base;
> +     phys_addr_t ctrl_size;
>       struct regulator_bulk_data supplies[DSI_DEV_REGULATOR_MAX];
>
>       struct clk *bus_clks[DSI_BUS_CLK_MAX];
> @@ -1839,7 +1840,7 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
>               goto fail;
>       }
>
> -     msm_host->ctrl_base = msm_ioremap(pdev, "dsi_ctrl", "DSI CTRL");
> +     msm_host->ctrl_base = msm_ioremap_size(pdev, "dsi_ctrl", "DSI CTRL",
> &msm_host->ctrl_size);
>       if (IS_ERR(msm_host->ctrl_base)) {
>               pr_err("%s: unable to map Dsi ctrl base\n", __func__);
>               ret = PTR_ERR(msm_host->ctrl_base);
> @@ -2494,7 +2495,7 @@ void msm_dsi_host_snapshot(struct msm_disp_state
> *disp_state, struct mipi_dsi_ho
>
>       pm_runtime_get_sync(&msm_host->pdev->dev);
>
> -     msm_disp_snapshot_add_block(disp_state,
> msm_iomap_size(msm_host->pdev, "dsi_ctrl"),
> +     msm_disp_snapshot_add_block(disp_state, msm_host->ctrl_size,
>                       msm_host->ctrl_base, "dsi%d_ctrl", msm_host->id);
>
>       pm_runtime_put_sync(&msm_host->pdev->dev);
> diff --git a/drivers/gpu/drm/msm/msm_drv.c
> b/drivers/gpu/drm/msm/msm_drv.c
> index 92fe844b517b..be578fc4e54f 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -124,7 +124,7 @@ struct clk *msm_clk_get(struct platform_device
> *pdev, const char *name)
>  }
>
>  static void __iomem *_msm_ioremap(struct platform_device *pdev, const
> char *name,
> -                               const char *dbgname, bool quiet)
> +                               const char *dbgname, bool quiet, phys_addr_t *psize)
>  {
>       struct resource *res;
>       unsigned long size;
> @@ -153,37 +153,28 @@ static void __iomem *_msm_ioremap(struct
> platform_device *pdev, const char *name
>       if (reglog)
>               printk(KERN_DEBUG "IO:region %s %p %08lx\n", dbgname, ptr, size);
>
> +     if (psize)
> +             *psize = size;
> +
>       return ptr;
>  }
>
>  void __iomem *msm_ioremap(struct platform_device *pdev, const char
> *name,
>                         const char *dbgname)
>  {
> -     return _msm_ioremap(pdev, name, dbgname, false);
> +     return _msm_ioremap(pdev, name, dbgname, false, NULL);
>  }
>
>  void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const
> char *name,
>                               const char *dbgname)
>  {
> -     return _msm_ioremap(pdev, name, dbgname, true);
> +     return _msm_ioremap(pdev, name, dbgname, true, NULL);
>  }
>
> -unsigned long msm_iomap_size(struct platform_device *pdev, const char
> *name)
> +void __iomem *msm_ioremap_size(struct platform_device *pdev, const
> char *name,
> +                       const char *dbgname, phys_addr_t *psize)
>  {
> -     struct resource *res;
> -
> -     if (name)
> -             res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
> -     else
> -             res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -
> -     if (!res) {
> -             dev_dbg(&pdev->dev, "failed to get memory resource: %s\n",
> -                             name);
> -             return 0;
> -     }
> -
> -     return resource_size(res);
> +     return _msm_ioremap(pdev, name, dbgname, false, psize);
>  }
>
>  void msm_writel(u32 data, void __iomem *addr)
> diff --git a/drivers/gpu/drm/msm/msm_drv.h
> b/drivers/gpu/drm/msm/msm_drv.h
> index 15cb34451ded..c33fc1293789 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -450,9 +450,10 @@ struct clk *msm_clk_bulk_get_clock(struct
> clk_bulk_data *bulk, int count,
>       const char *name);
>  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,
> +             const char *dbgname, phys_addr_t *size);
>  void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const
> char *name,
>               const char *dbgname);
> -unsigned long msm_iomap_size(struct platform_device *pdev, const char
> *name);
>  void msm_writel(u32 data, void __iomem *addr);
>  u32 msm_readl(const void __iomem *addr);
>  void msm_rmw(void __iomem *addr, u32 mask, u32 or);