* [PATCH 2/3] drm/msm: Use nvmem_cell_read_variable_le_u32() to read speed bin
2021-03-06 0:26 [PATCH 1/3] nvmem: core: Add functions to make number reading easy Douglas Anderson
@ 2021-03-06 0:26 ` Douglas Anderson
2021-03-06 0:26 ` [PATCH 3/3] PM: AVS: qcom-cpr: Use nvmem_cell_read_variable_le_u32() Douglas Anderson
2021-03-10 10:37 ` [PATCH 1/3] nvmem: core: Add functions to make number reading easy Srinivas Kandagatla
2 siblings, 0 replies; 6+ messages in thread
From: Douglas Anderson @ 2021-03-06 0:26 UTC (permalink / raw)
To: Rafael J . Wysocki, Rob Clark, Jordan Crouse
Cc: Ulf Hansson, Niklas Cassel, Jorge Ramirez-Ortiz, swboyd,
linux-arm-msm, Bjorn Andersson, 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>
---
This is based on my previous patch ("drm/msm: Fix speed-bin support
not to access outside valid memory") which has already landed in
msm-next. In case it's not obvious, this patch has a strong dependency
on my previous patch and also on patch #1 in this series.
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 0e2024defd79..e34705d17559 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1351,17 +1351,16 @@ 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);
if (ret) {
DRM_DEV_ERROR(dev,
"failed to read speed-bin (%d). Some OPPs may not be supported by hardware",
ret);
goto done;
}
- speedbin = le16_to_cpu(speedbin);
supp_hw = fuse_to_supp_hw(dev, revn, speedbin);
--
2.30.1.766.gb4fecdf3b7-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] PM: AVS: qcom-cpr: Use nvmem_cell_read_variable_le_u32()
2021-03-06 0:26 [PATCH 1/3] nvmem: core: Add functions to make number reading easy Douglas Anderson
2021-03-06 0:26 ` [PATCH 2/3] drm/msm: Use nvmem_cell_read_variable_le_u32() to read speed bin Douglas Anderson
@ 2021-03-06 0:26 ` Douglas Anderson
2021-03-10 10:37 ` [PATCH 1/3] nvmem: core: Add functions to make number reading easy Srinivas Kandagatla
2 siblings, 0 replies; 6+ messages in thread
From: Douglas Anderson @ 2021-03-06 0:26 UTC (permalink / raw)
To: Rafael J . Wysocki, Rob Clark, Jordan Crouse
Cc: Ulf Hansson, Niklas Cassel, Jorge Ramirez-Ortiz, swboyd,
linux-arm-msm, Bjorn Andersson, Akhil P Oommen, Douglas Anderson,
Andy Gross, Niklas Cassel, linux-kernel, linux-pm
Let's delete the private function cpr_read_efuse() since it does the
basically the same thing as the new API call
nvmem_cell_read_variable_le_u32().
Differences between the new API call and the old private function:
* less error printing (I assume this is OK).
* will give an error if the value doesn't fit in 32-bits (the old code
would have truncated silently).
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
I haven't done any more than compile-test this. Mostly I'm just
writing this patch because it helped provide inspiration for the
general API function.
drivers/soc/qcom/cpr.c | 43 +++++-------------------------------------
1 file changed, 5 insertions(+), 38 deletions(-)
diff --git a/drivers/soc/qcom/cpr.c b/drivers/soc/qcom/cpr.c
index b24cc77d1889..4ce8e816154f 100644
--- a/drivers/soc/qcom/cpr.c
+++ b/drivers/soc/qcom/cpr.c
@@ -801,38 +801,6 @@ static int cpr_set_performance_state(struct generic_pm_domain *domain,
return ret;
}
-static int cpr_read_efuse(struct device *dev, const char *cname, u32 *data)
-{
- struct nvmem_cell *cell;
- ssize_t len;
- char *ret;
- int i;
-
- *data = 0;
-
- cell = nvmem_cell_get(dev, cname);
- if (IS_ERR(cell)) {
- if (PTR_ERR(cell) != -EPROBE_DEFER)
- dev_err(dev, "undefined cell %s\n", cname);
- return PTR_ERR(cell);
- }
-
- ret = nvmem_cell_read(cell, &len);
- nvmem_cell_put(cell);
- if (IS_ERR(ret)) {
- dev_err(dev, "can't read cell %s\n", cname);
- return PTR_ERR(ret);
- }
-
- for (i = 0; i < len; i++)
- *data |= ret[i] << (8 * i);
-
- kfree(ret);
- dev_dbg(dev, "efuse read(%s) = %x, bytes %zd\n", cname, *data, len);
-
- return 0;
-}
-
static int
cpr_populate_ring_osc_idx(struct cpr_drv *drv)
{
@@ -843,8 +811,7 @@ cpr_populate_ring_osc_idx(struct cpr_drv *drv)
int ret;
for (; fuse < end; fuse++, fuses++) {
- ret = cpr_read_efuse(drv->dev, fuses->ring_osc,
- &data);
+ ret = nvmem_cell_read_variable_le_u32(drv->dev, fuses->ring_osc, &data);
if (ret)
return ret;
fuse->ring_osc_idx = data;
@@ -863,7 +830,7 @@ static int cpr_read_fuse_uV(const struct cpr_desc *desc,
u32 bits = 0;
int ret;
- ret = cpr_read_efuse(drv->dev, init_v_efuse, &bits);
+ ret = nvmem_cell_read_variable_le_u32(drv->dev, init_v_efuse, &bits);
if (ret)
return ret;
@@ -932,7 +899,7 @@ static int cpr_fuse_corner_init(struct cpr_drv *drv)
}
/* Populate target quotient by scaling */
- ret = cpr_read_efuse(drv->dev, fuses->quotient, &fuse->quot);
+ ret = nvmem_cell_read_variable_le_u32(drv->dev, fuses->quotient, &fuse->quot);
if (ret)
return ret;
@@ -1001,7 +968,7 @@ static int cpr_calculate_scaling(const char *quot_offset,
prev_fuse = fuse - 1;
if (quot_offset) {
- ret = cpr_read_efuse(drv->dev, quot_offset, "_diff);
+ ret = nvmem_cell_read_variable_le_u32(drv->dev, quot_offset, "_diff);
if (ret)
return ret;
@@ -1701,7 +1668,7 @@ static int cpr_probe(struct platform_device *pdev)
* initialized after attaching to the power domain,
* since it depends on the CPU's OPP table.
*/
- ret = cpr_read_efuse(dev, "cpr_fuse_revision", &cpr_rev);
+ ret = nvmem_cell_read_variable_le_u32(dev, "cpr_fuse_revision", &cpr_rev);
if (ret)
return ret;
--
2.30.1.766.gb4fecdf3b7-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] nvmem: core: Add functions to make number reading easy
2021-03-06 0:26 [PATCH 1/3] nvmem: core: Add functions to make number reading easy Douglas Anderson
2021-03-06 0:26 ` [PATCH 2/3] drm/msm: Use nvmem_cell_read_variable_le_u32() to read speed bin Douglas Anderson
2021-03-06 0:26 ` [PATCH 3/3] PM: AVS: qcom-cpr: Use nvmem_cell_read_variable_le_u32() Douglas Anderson
@ 2021-03-10 10:37 ` Srinivas Kandagatla
2021-03-10 15:50 ` Doug Anderson
2 siblings, 1 reply; 6+ messages in thread
From: Srinivas Kandagatla @ 2021-03-10 10:37 UTC (permalink / raw)
To: Douglas Anderson, Rafael J . Wysocki, Rob Clark, Jordan Crouse
Cc: Ulf Hansson, swboyd, linux-arm-msm, Bjorn Andersson,
Akhil P Oommen, linux-kernel
On 06/03/2021 00:26, Douglas Anderson wrote:
> Sometimes the clients of nvmem just want to get a number out of
> nvmem. They don't want to think about exactly how many bytes the nvmem
> cell took up. They just want the number. Let's make it easy.
>
> In general this concept is useful because nvmem space is precious and
> usually the fewest bits are allocated that will hold a given value on
> a given system. However, even though small numbers might be fine on
> one system that doesn't mean that logically the number couldn't be
> bigger. Imagine nvmem containing a max frequency for a component. On
> one system perhaps that fits in 16 bits. On another system it might
> fit in 32 bits. The code reading this number doesn't care--it just
> wants the number.
>
> We'll provide two functions: nvmem_cell_read_variable_le_u32() and
> nvmem_cell_read_variable_le_u64().
>
> Comparing these to the existing functions like nvmem_cell_read_u32():
> * These new functions have no problems if the value was stored in
> nvmem in fewer bytes. It's OK to use these function as long as the
> value stored will fit in 32-bits (or 64-bits).
> * These functions avoid problems that the earlier APIs had with bit
> offsets. For instance, you can't use nvmem_cell_read_u32() to read a
> value has nbits=32 and bit_offset=4 because the nvmem cell must be
> at least 5 bytes big to hold this value. The new API accounts for
> this and works fine.
> * These functions make it very explicit that they assume that the
> number was stored in little endian format. The old functions made
> this assumption whenever bit_offset was non-zero (see
> nvmem_shift_read_buffer_in_place()) but didn't whenever the
> bit_offset was zero.
>
> NOTE: it's assumed that we don't need an 8-bit or 16-bit version of
> this function. The 32-bit version of the function can be used to read
> 8-bit or 16-bit data.
>
> At the moment, I'm only adding the "unsigned" versions of these
> functions, but if it ends up being useful someone could add a "signed"
> version that did 2's complement sign extension.
>
> At the moment, I'm only adding the "little endian" versions of these
> functions. Adding the "big endian" version would require adding "big
> endian" support to nvmem_shift_read_buffer_in_place().
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> This is a logical follow-up to:
> https://lore.kernel.org/r/20210227002603.3260599-1-dianders@chromium.org/
> ...but since it doesn't really share any of the same patches I'm not
> marking it as a v2.
>
> drivers/nvmem/core.c | 95 ++++++++++++++++++++++++++++++++++
> include/linux/nvmem-consumer.h | 4 ++
> 2 files changed, 99 insertions(+)
>
This patch as it is LGTM.
If you plan to take this via other trees, here is
Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
--srini
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index a5ab1e0c74cf..635e3131eb5f 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -1606,6 +1606,101 @@ int nvmem_cell_read_u64(struct device *dev, const char *cell_id, u64 *val)
> }
> EXPORT_SYMBOL_GPL(nvmem_cell_read_u64);
>
> +static void *nvmem_cell_read_variable_common(struct device *dev,
> + const char *cell_id,
> + size_t max_len, size_t *len)
> +{
> + struct nvmem_cell *cell;
> + int nbits;
> + void *buf;
> +
> + cell = nvmem_cell_get(dev, cell_id);
> + if (IS_ERR(cell))
> + return cell;
> +
> + nbits = cell->nbits;
> + buf = nvmem_cell_read(cell, len);
> + nvmem_cell_put(cell);
> + if (IS_ERR(buf))
> + return buf;
> +
> + /*
> + * If nbits is set then nvmem_cell_read() can significantly exaggerate
> + * the length of the real data. Throw away the extra junk.
> + */
> + if (nbits)
> + *len = DIV_ROUND_UP(nbits, 8);
> +
> + if (*len > max_len) {
> + kfree(buf);
> + return ERR_PTR(-ERANGE);
> + }
> +
> + return buf;
> +}
> +
> +/**
> + * nvmem_cell_read_variable_le_u32() - Read up to 32-bits of data as a little endian number.
> + *
> + * @dev: Device that requests the nvmem cell.
> + * @cell_id: Name of nvmem cell to read.
> + * @val: pointer to output value.
> + *
> + * Return: 0 on success or negative errno.
> + */
> +int nvmem_cell_read_variable_le_u32(struct device *dev, const char *cell_id,
> + u32 *val)
> +{
> + size_t len;
> + u8 *buf;
> + int i;
> +
> + buf = nvmem_cell_read_variable_common(dev, cell_id, sizeof(*val), &len);
> + if (IS_ERR(buf))
> + return PTR_ERR(buf);
> +
> + /* Copy w/ implicit endian conversion */
> + *val = 0;
> + for (i = 0; i < len; i++)
> + *val |= buf[i] << (8 * i);
> +
> + kfree(buf);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(nvmem_cell_read_variable_le_u32);
> +
> +/**
> + * nvmem_cell_read_variable_le_u64() - Read up to 64-bits of data as a little endian number.
> + *
> + * @dev: Device that requests the nvmem cell.
> + * @cell_id: Name of nvmem cell to read.
> + * @val: pointer to output value.
> + *
> + * Return: 0 on success or negative errno.
> + */
> +int nvmem_cell_read_variable_le_u64(struct device *dev, const char *cell_id,
> + u64 *val)
> +{
> + size_t len;
> + u8 *buf;
> + int i;
> +
> + buf = nvmem_cell_read_variable_common(dev, cell_id, sizeof(*val), &len);
> + if (IS_ERR(buf))
> + return PTR_ERR(buf);
> +
> + /* Copy w/ implicit endian conversion */
> + *val = 0;
> + for (i = 0; i < len; i++)
> + *val |= buf[i] << (8 * i);
> +
> + kfree(buf);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(nvmem_cell_read_variable_le_u64);
> +
> /**
> * nvmem_device_cell_read() - Read a given nvmem device and cell
> *
> diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
> index 052293f4cbdb..923dada24eb4 100644
> --- a/include/linux/nvmem-consumer.h
> +++ b/include/linux/nvmem-consumer.h
> @@ -65,6 +65,10 @@ int nvmem_cell_read_u8(struct device *dev, const char *cell_id, u8 *val);
> int nvmem_cell_read_u16(struct device *dev, const char *cell_id, u16 *val);
> int nvmem_cell_read_u32(struct device *dev, const char *cell_id, u32 *val);
> int nvmem_cell_read_u64(struct device *dev, const char *cell_id, u64 *val);
> +int nvmem_cell_read_variable_le_u32(struct device *dev, const char *cell_id,
> + u32 *val);
> +int nvmem_cell_read_variable_le_u64(struct device *dev, const char *cell_id,
> + u64 *val);
>
> /* direct nvmem device read/write interface */
> struct nvmem_device *nvmem_device_get(struct device *dev, const char *name);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] nvmem: core: Add functions to make number reading easy
2021-03-10 10:37 ` [PATCH 1/3] nvmem: core: Add functions to make number reading easy Srinivas Kandagatla
@ 2021-03-10 15:50 ` Doug Anderson
2021-03-10 16:19 ` Srinivas Kandagatla
0 siblings, 1 reply; 6+ messages in thread
From: Doug Anderson @ 2021-03-10 15:50 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: Rafael J . Wysocki, Rob Clark, Jordan Crouse, Ulf Hansson,
Stephen Boyd, linux-arm-msm, Bjorn Andersson, Akhil P Oommen,
LKML
Hi,
On Wed, Mar 10, 2021 at 2:37 AM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
>
> On 06/03/2021 00:26, Douglas Anderson wrote:
> > Sometimes the clients of nvmem just want to get a number out of
> > nvmem. They don't want to think about exactly how many bytes the nvmem
> > cell took up. They just want the number. Let's make it easy.
> >
> > In general this concept is useful because nvmem space is precious and
> > usually the fewest bits are allocated that will hold a given value on
> > a given system. However, even though small numbers might be fine on
> > one system that doesn't mean that logically the number couldn't be
> > bigger. Imagine nvmem containing a max frequency for a component. On
> > one system perhaps that fits in 16 bits. On another system it might
> > fit in 32 bits. The code reading this number doesn't care--it just
> > wants the number.
> >
> > We'll provide two functions: nvmem_cell_read_variable_le_u32() and
> > nvmem_cell_read_variable_le_u64().
> >
> > Comparing these to the existing functions like nvmem_cell_read_u32():
> > * These new functions have no problems if the value was stored in
> > nvmem in fewer bytes. It's OK to use these function as long as the
> > value stored will fit in 32-bits (or 64-bits).
> > * These functions avoid problems that the earlier APIs had with bit
> > offsets. For instance, you can't use nvmem_cell_read_u32() to read a
> > value has nbits=32 and bit_offset=4 because the nvmem cell must be
> > at least 5 bytes big to hold this value. The new API accounts for
> > this and works fine.
> > * These functions make it very explicit that they assume that the
> > number was stored in little endian format. The old functions made
> > this assumption whenever bit_offset was non-zero (see
> > nvmem_shift_read_buffer_in_place()) but didn't whenever the
> > bit_offset was zero.
> >
> > NOTE: it's assumed that we don't need an 8-bit or 16-bit version of
> > this function. The 32-bit version of the function can be used to read
> > 8-bit or 16-bit data.
> >
> > At the moment, I'm only adding the "unsigned" versions of these
> > functions, but if it ends up being useful someone could add a "signed"
> > version that did 2's complement sign extension.
> >
> > At the moment, I'm only adding the "little endian" versions of these
> > functions. Adding the "big endian" version would require adding "big
> > endian" support to nvmem_shift_read_buffer_in_place().
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > This is a logical follow-up to:
> > https://lore.kernel.org/r/20210227002603.3260599-1-dianders@chromium.org/
> > ...but since it doesn't really share any of the same patches I'm not
> > marking it as a v2.
> >
> > drivers/nvmem/core.c | 95 ++++++++++++++++++++++++++++++++++
> > include/linux/nvmem-consumer.h | 4 ++
> > 2 files changed, 99 insertions(+)
> >
>
> This patch as it is LGTM.
>
> If you plan to take this via other trees, here is
>
> Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Thanks! I think none of this is terribly urgent, though. Unless
someone has a different opinion, my thought would be:
* This patch lands in your tree for 5.13.
* I'll snooze the email for 2 months and poke patch #2 and #3 once
5.13-rc1 is out.
Does that sound OK to you?
-Doug
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] nvmem: core: Add functions to make number reading easy
2021-03-10 15:50 ` Doug Anderson
@ 2021-03-10 16:19 ` Srinivas Kandagatla
0 siblings, 0 replies; 6+ messages in thread
From: Srinivas Kandagatla @ 2021-03-10 16:19 UTC (permalink / raw)
To: Doug Anderson
Cc: Rafael J . Wysocki, Rob Clark, Jordan Crouse, Ulf Hansson,
Stephen Boyd, linux-arm-msm, Bjorn Andersson, Akhil P Oommen,
LKML
On 10/03/2021 15:50, Doug Anderson wrote:
> Hi,
>
> On Wed, Mar 10, 2021 at 2:37 AM Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>>
>>
>>
>> On 06/03/2021 00:26, Douglas Anderson wrote:
>>> Sometimes the clients of nvmem just want to get a number out of
>>> nvmem. They don't want to think about exactly how many bytes the nvmem
>>> cell took up. They just want the number. Let's make it easy.
>>>
>>> In general this concept is useful because nvmem space is precious and
>>> usually the fewest bits are allocated that will hold a given value on
>>> a given system. However, even though small numbers might be fine on
>>> one system that doesn't mean that logically the number couldn't be
>>> bigger. Imagine nvmem containing a max frequency for a component. On
>>> one system perhaps that fits in 16 bits. On another system it might
>>> fit in 32 bits. The code reading this number doesn't care--it just
>>> wants the number.
>>>
>>> We'll provide two functions: nvmem_cell_read_variable_le_u32() and
>>> nvmem_cell_read_variable_le_u64().
>>>
>>> Comparing these to the existing functions like nvmem_cell_read_u32():
>>> * These new functions have no problems if the value was stored in
>>> nvmem in fewer bytes. It's OK to use these function as long as the
>>> value stored will fit in 32-bits (or 64-bits).
>>> * These functions avoid problems that the earlier APIs had with bit
>>> offsets. For instance, you can't use nvmem_cell_read_u32() to read a
>>> value has nbits=32 and bit_offset=4 because the nvmem cell must be
>>> at least 5 bytes big to hold this value. The new API accounts for
>>> this and works fine.
>>> * These functions make it very explicit that they assume that the
>>> number was stored in little endian format. The old functions made
>>> this assumption whenever bit_offset was non-zero (see
>>> nvmem_shift_read_buffer_in_place()) but didn't whenever the
>>> bit_offset was zero.
>>>
>>> NOTE: it's assumed that we don't need an 8-bit or 16-bit version of
>>> this function. The 32-bit version of the function can be used to read
>>> 8-bit or 16-bit data.
>>>
>>> At the moment, I'm only adding the "unsigned" versions of these
>>> functions, but if it ends up being useful someone could add a "signed"
>>> version that did 2's complement sign extension.
>>>
>>> At the moment, I'm only adding the "little endian" versions of these
>>> functions. Adding the "big endian" version would require adding "big
>>> endian" support to nvmem_shift_read_buffer_in_place().
>>>
>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>> ---
>>> This is a logical follow-up to:
>>> https://lore.kernel.org/r/20210227002603.3260599-1-dianders@chromium.org/
>>> ...but since it doesn't really share any of the same patches I'm not
>>> marking it as a v2.
>>>
>>> drivers/nvmem/core.c | 95 ++++++++++++++++++++++++++++++++++
>>> include/linux/nvmem-consumer.h | 4 ++
>>> 2 files changed, 99 insertions(+)
>>>
>>
>> This patch as it is LGTM.
>>
>> If you plan to take this via other trees, here is
>>
>> Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>
> Thanks! I think none of this is terribly urgent, though. Unless
> someone has a different opinion, my thought would be:
>
> * This patch lands in your tree for 5.13.
>
> * I'll snooze the email for 2 months and poke patch #2 and #3 once
> 5.13-rc1 is out.
>
> Does that sound OK to you?
That works for me!
Applied thanks!
--srini
>
> -Doug
>
^ permalink raw reply [flat|nested] 6+ messages in thread