diff mbox series

[v2,3/4] drm/msm: get rid of msm_iomap_size

Message ID 20210427001828.2375555-4-dmitry.baryshkov@linaro.org
State Accepted
Commit bac2c6a62ed91ba4f6c7c14a6a40b7c696b35645
Headers show
Series drm/msm: improve register snapshotting | expand

Commit Message

Dmitry Baryshkov April 27, 2021, 12:18 a.m. UTC
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>

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

-- 
2.30.2

Comments

Abhinav Kumar April 27, 2021, 7:29 p.m. UTC | #1
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?

> ---
>  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);
Dmitry Baryshkov April 27, 2021, 8:32 p.m. UTC | #2
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.

>
> > ---
> >  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);
Abhinav Kumar April 27, 2021, 10:12 p.m. UTC | #3
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);
Bjorn Andersson April 28, 2021, 2:47 a.m. UTC | #4
On Mon 26 Apr 19:18 CDT 2021, Dmitry Baryshkov wrote:
[..]
> 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)


size_t sounds like a better fit for psize...

Regards,
Bjorn
Dmitry Baryshkov April 28, 2021, 1:41 p.m. UTC | #5
On 28/04/2021 05:47, Bjorn Andersson wrote:
> On Mon 26 Apr 19:18 CDT 2021, Dmitry Baryshkov wrote:

> [..]

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

> 

> size_t sounds like a better fit for psize...


I was trying to select between size_t and phys_addr_t, settling on the 
latter one because it is used for resource size.


-- 
With best wishes
Dmitry
Bjorn Andersson April 28, 2021, 1:59 p.m. UTC | #6
On Wed 28 Apr 08:41 CDT 2021, Dmitry Baryshkov wrote:

> On 28/04/2021 05:47, Bjorn Andersson wrote:

> > On Mon 26 Apr 19:18 CDT 2021, Dmitry Baryshkov wrote:

> > [..]

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

> > 

> > size_t sounds like a better fit for psize...

> 

> I was trying to select between size_t and phys_addr_t, settling on the

> latter one because it is used for resource size.

> 


I always thought resource_size_t was an alias for size_t, now I know :)

That said, I still think that size_t (in line with resource_size_t)
gives a better hint about what the parameter represents...

Regards,
Bjorn
Dmitry Baryshkov April 28, 2021, 2:03 p.m. UTC | #7
On 28/04/2021 16:59, Bjorn Andersson wrote:
> On Wed 28 Apr 08:41 CDT 2021, Dmitry Baryshkov wrote:

> 

>> On 28/04/2021 05:47, Bjorn Andersson wrote:

>>> On Mon 26 Apr 19:18 CDT 2021, Dmitry Baryshkov wrote:

>>> [..]

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

>>>

>>> size_t sounds like a better fit for psize...

>>

>> I was trying to select between size_t and phys_addr_t, settling on the

>> latter one because it is used for resource size.

>>

> 

> I always thought resource_size_t was an alias for size_t, now I know :)

> 

> That said, I still think that size_t (in line with resource_size_t)

> gives a better hint about what the parameter represents...


Indeed, I'll change that in the next version.

> 

> Regards,

> Bjorn

> 



-- 
With best wishes
Dmitry
diff mbox series

Patch

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