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

On 2021-04-27 13:32, Dmitry Baryshkov wrote:

On Tue, 27 Apr 2021 at 22:30, abhinavk@codeaurora.org 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@linaro.org

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@codeaurora.org

...

...

---

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);