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. > > > --- > > 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); -- With best wishes Dmitry _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel