linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] drm/msm: Use nvmem_cell_read_variable_le_u32() to read speed bin
@ 2021-05-21 20:45 Douglas Anderson
  2021-05-21 22:02 ` Stephen Boyd
  0 siblings, 1 reply; 4+ messages in thread
From: Douglas Anderson @ 2021-05-21 20:45 UTC (permalink / raw)
  To: Rob Clark
  Cc: John Stultz, YongQin Liu, swboyd, Jordan Crouse, linux-arm-msm,
	Akhil P Oommen, Douglas Anderson, Daniel Vetter, David Airlie,
	Eric Anholt, Jonathan Marek, Sai Prakash Ranjan, Sean Paul,
	Sharat Masetty, dri-devel, freedreno, linux-kernel

Let's use the newly-added nvmem_cell_read_variable_le_u32() to future
proof ourselves a little bit.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
The patch that this depends on is now in mainline so it can be merged
at will. I'm just sending this as a singleton patch to make it obvious
that there are no dependencies now.

Changes in v2:
- Rebased

 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index b4d8e1b01ee4..a07214157ad3 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1403,10 +1403,10 @@ static int a6xx_set_supported_hw(struct device *dev, struct a6xx_gpu *a6xx_gpu,
 {
 	struct opp_table *opp_table;
 	u32 supp_hw = UINT_MAX;
-	u16 speedbin;
+	u32 speedbin;
 	int ret;
 
-	ret = nvmem_cell_read_u16(dev, "speed_bin", &speedbin);
+	ret = nvmem_cell_read_variable_le_u32(dev, "speed_bin", &speedbin);
 	/*
 	 * -ENOENT means that the platform doesn't support speedbin which is
 	 * fine
@@ -1419,7 +1419,6 @@ static int a6xx_set_supported_hw(struct device *dev, struct a6xx_gpu *a6xx_gpu,
 			      ret);
 		goto done;
 	}
-	speedbin = le16_to_cpu(speedbin);
 
 	supp_hw = fuse_to_supp_hw(dev, revn, speedbin);
 
-- 
2.31.1.818.g46aad6cb9e-goog


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] drm/msm: Use nvmem_cell_read_variable_le_u32() to read speed bin
  2021-05-21 20:45 [PATCH v2] drm/msm: Use nvmem_cell_read_variable_le_u32() to read speed bin Douglas Anderson
@ 2021-05-21 22:02 ` Stephen Boyd
  2021-05-21 22:35   ` Doug Anderson
  0 siblings, 1 reply; 4+ messages in thread
From: Stephen Boyd @ 2021-05-21 22:02 UTC (permalink / raw)
  To: Douglas Anderson, Rob Clark
  Cc: John Stultz, YongQin Liu, Jordan Crouse, linux-arm-msm,
	Akhil P Oommen, Daniel Vetter, David Airlie, Eric Anholt,
	Jonathan Marek, Sai Prakash Ranjan, Sean Paul, Sharat Masetty,
	dri-devel, freedreno, linux-kernel

Quoting Douglas Anderson (2021-05-21 13:45:50)
> Let's use the newly-added nvmem_cell_read_variable_le_u32() to future
> proof ourselves a little bit.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> The patch that this depends on is now in mainline so it can be merged
> at will. I'm just sending this as a singleton patch to make it obvious
> that there are no dependencies now.
>
> Changes in v2:
> - Rebased
>
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index b4d8e1b01ee4..a07214157ad3 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -1403,10 +1403,10 @@ static int a6xx_set_supported_hw(struct device *dev, struct a6xx_gpu *a6xx_gpu,
>  {
>         struct opp_table *opp_table;
>         u32 supp_hw = UINT_MAX;
> -       u16 speedbin;
> +       u32 speedbin;
>         int ret;
>
> -       ret = nvmem_cell_read_u16(dev, "speed_bin", &speedbin);
> +       ret = nvmem_cell_read_variable_le_u32(dev, "speed_bin", &speedbin);

I missed the review of this API, sorry. I wonder why it doesn't return
the value into an __le32 pointer. Then the caller could use
le32_to_cpu() like other places in the kernel and we know that code is
properly converting the little endian value to CPU native order. Right
now the API doesn't express the endianess of the bits in the return
value because it uses u32, so from a static checker perspective (sparse)
those bits are CPU native order, not little endian.

>         /*
>          * -ENOENT means that the platform doesn't support speedbin which is
>          * fine

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] drm/msm: Use nvmem_cell_read_variable_le_u32() to read speed bin
  2021-05-21 22:02 ` Stephen Boyd
@ 2021-05-21 22:35   ` Doug Anderson
  2021-05-21 22:54     ` Stephen Boyd
  0 siblings, 1 reply; 4+ messages in thread
From: Doug Anderson @ 2021-05-21 22:35 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Rob Clark, John Stultz, YongQin Liu, Jordan Crouse,
	linux-arm-msm, Akhil P Oommen, Daniel Vetter, David Airlie,
	Eric Anholt, Jonathan Marek, Sai Prakash Ranjan, Sean Paul,
	Sharat Masetty, dri-devel, freedreno, LKML

Hi,

On Fri, May 21, 2021 at 3:02 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Douglas Anderson (2021-05-21 13:45:50)
> > Let's use the newly-added nvmem_cell_read_variable_le_u32() to future
> > proof ourselves a little bit.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > The patch that this depends on is now in mainline so it can be merged
> > at will. I'm just sending this as a singleton patch to make it obvious
> > that there are no dependencies now.
> >
> > Changes in v2:
> > - Rebased
> >
> >  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > index b4d8e1b01ee4..a07214157ad3 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > @@ -1403,10 +1403,10 @@ static int a6xx_set_supported_hw(struct device *dev, struct a6xx_gpu *a6xx_gpu,
> >  {
> >         struct opp_table *opp_table;
> >         u32 supp_hw = UINT_MAX;
> > -       u16 speedbin;
> > +       u32 speedbin;
> >         int ret;
> >
> > -       ret = nvmem_cell_read_u16(dev, "speed_bin", &speedbin);
> > +       ret = nvmem_cell_read_variable_le_u32(dev, "speed_bin", &speedbin);
>
> I missed the review of this API, sorry.

You commented on the patch that added it, though? Oddly I can't find
your commit on lore.kernel.org (?), but it's in my inbox...


> I wonder why it doesn't return
> the value into an __le32 pointer. Then the caller could use
> le32_to_cpu() like other places in the kernel and we know that code is
> properly converting the little endian value to CPU native order. Right
> now the API doesn't express the endianess of the bits in the return
> value because it uses u32, so from a static checker perspective (sparse)
> those bits are CPU native order, not little endian.

I think it's backwards of what you're saying? This function is for
when the value is stored in nvram in little endian but returned to the
caller in CPU native order. It would be really awkward _not_ to
convert this value from LE to native order in the
nvmem_cell_read_variable_le_u32() function because that functions
handles the fact that the cell could be specified as several different
sizes (as long as it's less than 32-bits).

-Doug


-Doug

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] drm/msm: Use nvmem_cell_read_variable_le_u32() to read speed bin
  2021-05-21 22:35   ` Doug Anderson
@ 2021-05-21 22:54     ` Stephen Boyd
  0 siblings, 0 replies; 4+ messages in thread
From: Stephen Boyd @ 2021-05-21 22:54 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Rob Clark, John Stultz, YongQin Liu, Jordan Crouse,
	linux-arm-msm, Akhil P Oommen, Daniel Vetter, David Airlie,
	Eric Anholt, Jonathan Marek, Sai Prakash Ranjan, Sean Paul,
	dri-devel, freedreno, LKML

Quoting Doug Anderson (2021-05-21 15:35:33)
> Hi,
>
> On Fri, May 21, 2021 at 3:02 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Douglas Anderson (2021-05-21 13:45:50)
> > > Let's use the newly-added nvmem_cell_read_variable_le_u32() to future
> > > proof ourselves a little bit.
> > >
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > ---
> > > The patch that this depends on is now in mainline so it can be merged
> > > at will. I'm just sending this as a singleton patch to make it obvious
> > > that there are no dependencies now.
> > >
> > > Changes in v2:
> > > - Rebased
> > >
> > >  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > > index b4d8e1b01ee4..a07214157ad3 100644
> > > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > > @@ -1403,10 +1403,10 @@ static int a6xx_set_supported_hw(struct device *dev, struct a6xx_gpu *a6xx_gpu,
> > >  {
> > >         struct opp_table *opp_table;
> > >         u32 supp_hw = UINT_MAX;
> > > -       u16 speedbin;
> > > +       u32 speedbin;
> > >         int ret;
> > >
> > > -       ret = nvmem_cell_read_u16(dev, "speed_bin", &speedbin);
> > > +       ret = nvmem_cell_read_variable_le_u32(dev, "speed_bin", &speedbin);
> >
> > I missed the review of this API, sorry.
>
> You commented on the patch that added it, though? Oddly I can't find
> your commit on lore.kernel.org (?), but it's in my inbox...

Must be brain fog on my end!

>
>
> > I wonder why it doesn't return
> > the value into an __le32 pointer. Then the caller could use
> > le32_to_cpu() like other places in the kernel and we know that code is
> > properly converting the little endian value to CPU native order. Right
> > now the API doesn't express the endianess of the bits in the return
> > value because it uses u32, so from a static checker perspective (sparse)
> > those bits are CPU native order, not little endian.
>
> I think it's backwards of what you're saying? This function is for
> when the value is stored in nvram in little endian but returned to the
> caller in CPU native order. It would be really awkward _not_ to
> convert this value from LE to native order in the
> nvmem_cell_read_variable_le_u32() function because that functions
> handles the fact that the cell could be specified as several different
> sizes (as long as it's less than 32-bits).
>

Ah ok. I was looking at the name of the API and thinking it was an le32;
happily glossing over that _u between le and 32. So it's "nvmem cell read
variable little endian to cpu u32"?

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-05-21 22:54 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-21 20:45 [PATCH v2] drm/msm: Use nvmem_cell_read_variable_le_u32() to read speed bin Douglas Anderson
2021-05-21 22:02 ` Stephen Boyd
2021-05-21 22:35   ` Doug Anderson
2021-05-21 22:54     ` Stephen Boyd

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).