* [PATCH 1/4] nvmem: sunxi_sid: Always use 32-bit MMIO reads
2022-08-14 17:36 [PATCH 0/4] nvmem: Support non-stride-aligned NVMEM cell data Samuel Holland
@ 2022-08-14 17:36 ` Samuel Holland
2022-08-25 12:05 ` Heiko Stübner
` (2 more replies)
2022-08-14 17:36 ` [PATCH 2/4] nvmem: sunxi_sid: Drop the workaround on A64 Samuel Holland
` (2 subsequent siblings)
3 siblings, 3 replies; 13+ messages in thread
From: Samuel Holland @ 2022-08-14 17:36 UTC (permalink / raw)
To: Srinivas Kandagatla, Chen-Yu Tsai, Jernej Skrabec
Cc: Samuel Holland, Greg Kroah-Hartman, Krzysztof Kozlowski,
Rob Herring, devicetree, linux-arm-kernel, linux-kernel,
linux-sunxi
The SID SRAM on at least some SoCs (A64 and D1) returns different values
when read with bus cycles narrower than 32 bits. This is not immediately
obvious, because memcpy_fromio() uses word-size accesses as long as
enough data is being copied.
The vendor driver always uses 32-bit MMIO reads, so do the same here.
This is faster than the register-based method, which is currently used
as a workaround on A64. And it fixes the values returned on D1, where
the SRAM method was being used.
The special case for the last word is needed to maintain .word_size == 1
for sysfs ABI compatibility, as noted previously in commit de2a3eaea552
("nvmem: sunxi_sid: Optimize register read-out method").
Fixes: 07ae4fde9efa ("nvmem: sunxi_sid: Add support for D1 variant")
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
drivers/nvmem/sunxi_sid.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
index 5750e1f4bcdb..92dfe4cb10e3 100644
--- a/drivers/nvmem/sunxi_sid.c
+++ b/drivers/nvmem/sunxi_sid.c
@@ -41,8 +41,21 @@ static int sunxi_sid_read(void *context, unsigned int offset,
void *val, size_t bytes)
{
struct sunxi_sid *sid = context;
+ u32 word;
+
+ /* .stride = 4 so offset is guaranteed to be aligned */
+ __ioread32_copy(val, sid->base + sid->value_offset + offset, bytes / 4);
- memcpy_fromio(val, sid->base + sid->value_offset + offset, bytes);
+ val += round_down(bytes, 4);
+ offset += round_down(bytes, 4);
+ bytes = bytes % 4;
+
+ if (!bytes)
+ return 0;
+
+ /* Handle any trailing bytes */
+ word = readl_relaxed(sid->base + sid->value_offset + offset);
+ memcpy(val, &word, bytes);
return 0;
}
--
2.35.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] nvmem: sunxi_sid: Always use 32-bit MMIO reads
2022-08-14 17:36 ` [PATCH 1/4] nvmem: sunxi_sid: Always use 32-bit MMIO reads Samuel Holland
@ 2022-08-25 12:05 ` Heiko Stübner
2022-09-09 8:48 ` Srinivas Kandagatla
2023-01-08 20:50 ` Jernej Škrabec
2 siblings, 0 replies; 13+ messages in thread
From: Heiko Stübner @ 2022-08-25 12:05 UTC (permalink / raw)
To: Srinivas Kandagatla, Chen-Yu Tsai, Jernej Skrabec, linux-arm-kernel
Cc: Samuel Holland, Greg Kroah-Hartman, Krzysztof Kozlowski,
Rob Herring, devicetree, linux-arm-kernel, linux-kernel,
linux-sunxi, Samuel Holland
Am Sonntag, 14. August 2022, 19:36:52 CEST schrieb Samuel Holland:
> The SID SRAM on at least some SoCs (A64 and D1) returns different values
> when read with bus cycles narrower than 32 bits. This is not immediately
> obvious, because memcpy_fromio() uses word-size accesses as long as
> enough data is being copied.
>
> The vendor driver always uses 32-bit MMIO reads, so do the same here.
> This is faster than the register-based method, which is currently used
> as a workaround on A64. And it fixes the values returned on D1, where
> the SRAM method was being used.
>
> The special case for the last word is needed to maintain .word_size == 1
> for sysfs ABI compatibility, as noted previously in commit de2a3eaea552
> ("nvmem: sunxi_sid: Optimize register read-out method").
>
> Fixes: 07ae4fde9efa ("nvmem: sunxi_sid: Add support for D1 variant")
> Signed-off-by: Samuel Holland <samuel@sholland.org>
On a D1-Nezha:
Tested-by: Heiko Stuebner <heiko@sntech.de>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] nvmem: sunxi_sid: Always use 32-bit MMIO reads
2022-08-14 17:36 ` [PATCH 1/4] nvmem: sunxi_sid: Always use 32-bit MMIO reads Samuel Holland
2022-08-25 12:05 ` Heiko Stübner
@ 2022-09-09 8:48 ` Srinivas Kandagatla
2023-01-08 20:50 ` Jernej Škrabec
2 siblings, 0 replies; 13+ messages in thread
From: Srinivas Kandagatla @ 2022-09-09 8:48 UTC (permalink / raw)
To: Samuel Holland, Chen-Yu Tsai, Jernej Skrabec
Cc: Greg Kroah-Hartman, Krzysztof Kozlowski, Rob Herring, devicetree,
linux-arm-kernel, linux-kernel, linux-sunxi
On 14/08/2022 18:36, Samuel Holland wrote:
> The SID SRAM on at least some SoCs (A64 and D1) returns different values
> when read with bus cycles narrower than 32 bits. This is not immediately
> obvious, because memcpy_fromio() uses word-size accesses as long as
> enough data is being copied.
>
> The vendor driver always uses 32-bit MMIO reads, so do the same here.
> This is faster than the register-based method, which is currently used
> as a workaround on A64. And it fixes the values returned on D1, where
> the SRAM method was being used.
>
> The special case for the last word is needed to maintain .word_size == 1
> for sysfs ABI compatibility, as noted previously in commit de2a3eaea552
> ("nvmem: sunxi_sid: Optimize register read-out method").
>
Missing Cc stable..
--srini
> Fixes: 07ae4fde9efa ("nvmem: sunxi_sid: Add support for D1 variant")
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>
> drivers/nvmem/sunxi_sid.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
> index 5750e1f4bcdb..92dfe4cb10e3 100644
> --- a/drivers/nvmem/sunxi_sid.c
> +++ b/drivers/nvmem/sunxi_sid.c
> @@ -41,8 +41,21 @@ static int sunxi_sid_read(void *context, unsigned int offset,
> void *val, size_t bytes)
> {
> struct sunxi_sid *sid = context;
> + u32 word;
> +
> + /* .stride = 4 so offset is guaranteed to be aligned */
> + __ioread32_copy(val, sid->base + sid->value_offset + offset, bytes / 4);
>
> - memcpy_fromio(val, sid->base + sid->value_offset + offset, bytes);
> + val += round_down(bytes, 4);
> + offset += round_down(bytes, 4);
> + bytes = bytes % 4;
> +
> + if (!bytes)
> + return 0;
> +
> + /* Handle any trailing bytes */
> + word = readl_relaxed(sid->base + sid->value_offset + offset);
> + memcpy(val, &word, bytes);
>
> return 0;
> }
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] nvmem: sunxi_sid: Always use 32-bit MMIO reads
2022-08-14 17:36 ` [PATCH 1/4] nvmem: sunxi_sid: Always use 32-bit MMIO reads Samuel Holland
2022-08-25 12:05 ` Heiko Stübner
2022-09-09 8:48 ` Srinivas Kandagatla
@ 2023-01-08 20:50 ` Jernej Škrabec
2 siblings, 0 replies; 13+ messages in thread
From: Jernej Škrabec @ 2023-01-08 20:50 UTC (permalink / raw)
To: Srinivas Kandagatla, Chen-Yu Tsai, Samuel Holland
Cc: Samuel Holland, Greg Kroah-Hartman, Krzysztof Kozlowski,
Rob Herring, devicetree, linux-arm-kernel, linux-kernel,
linux-sunxi
Dne nedelja, 14. avgust 2022 ob 19:36:52 CET je Samuel Holland napisal(a):
> The SID SRAM on at least some SoCs (A64 and D1) returns different values
> when read with bus cycles narrower than 32 bits. This is not immediately
> obvious, because memcpy_fromio() uses word-size accesses as long as
> enough data is being copied.
>
> The vendor driver always uses 32-bit MMIO reads, so do the same here.
> This is faster than the register-based method, which is currently used
> as a workaround on A64. And it fixes the values returned on D1, where
> the SRAM method was being used.
>
> The special case for the last word is needed to maintain .word_size == 1
> for sysfs ABI compatibility, as noted previously in commit de2a3eaea552
> ("nvmem: sunxi_sid: Optimize register read-out method").
>
> Fixes: 07ae4fde9efa ("nvmem: sunxi_sid: Add support for D1 variant")
> Signed-off-by: Samuel Holland <samuel@sholland.org>
Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>
Best regards,
Jernej
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/4] nvmem: sunxi_sid: Drop the workaround on A64
2022-08-14 17:36 [PATCH 0/4] nvmem: Support non-stride-aligned NVMEM cell data Samuel Holland
2022-08-14 17:36 ` [PATCH 1/4] nvmem: sunxi_sid: Always use 32-bit MMIO reads Samuel Holland
@ 2022-08-14 17:36 ` Samuel Holland
2022-08-15 8:37 ` Icenowy Zheng
2022-08-14 17:36 ` [PATCH 3/4] dt-bindings: nvmem: Allow bit offsets greater than a byte Samuel Holland
2022-08-14 17:36 ` [PATCH 4/4] nvmem: core: Support reading cells with >= 8 bit offsets Samuel Holland
3 siblings, 1 reply; 13+ messages in thread
From: Samuel Holland @ 2022-08-14 17:36 UTC (permalink / raw)
To: Srinivas Kandagatla, Chen-Yu Tsai, Jernej Skrabec
Cc: Samuel Holland, Greg Kroah-Hartman, Krzysztof Kozlowski,
Rob Herring, devicetree, linux-arm-kernel, linux-kernel,
linux-sunxi
Now that the SRAM readout code is fixed by using 32-bit accesses, it
always returns the same values as register readout, so the A64 variant
no longer needs the workaround. This makes the D1 variant structure
redundant, so remove it.
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
drivers/nvmem/sunxi_sid.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
index 92dfe4cb10e3..a970f1741cc6 100644
--- a/drivers/nvmem/sunxi_sid.c
+++ b/drivers/nvmem/sunxi_sid.c
@@ -197,15 +197,9 @@ static const struct sunxi_sid_cfg sun8i_h3_cfg = {
.need_register_readout = true,
};
-static const struct sunxi_sid_cfg sun20i_d1_cfg = {
- .value_offset = 0x200,
- .size = 0x100,
-};
-
static const struct sunxi_sid_cfg sun50i_a64_cfg = {
.value_offset = 0x200,
.size = 0x100,
- .need_register_readout = true,
};
static const struct sunxi_sid_cfg sun50i_h6_cfg = {
@@ -218,7 +212,7 @@ static const struct of_device_id sunxi_sid_of_match[] = {
{ .compatible = "allwinner,sun7i-a20-sid", .data = &sun7i_a20_cfg },
{ .compatible = "allwinner,sun8i-a83t-sid", .data = &sun50i_a64_cfg },
{ .compatible = "allwinner,sun8i-h3-sid", .data = &sun8i_h3_cfg },
- { .compatible = "allwinner,sun20i-d1-sid", .data = &sun20i_d1_cfg },
+ { .compatible = "allwinner,sun20i-d1-sid", .data = &sun50i_a64_cfg },
{ .compatible = "allwinner,sun50i-a64-sid", .data = &sun50i_a64_cfg },
{ .compatible = "allwinner,sun50i-h5-sid", .data = &sun50i_a64_cfg },
{ .compatible = "allwinner,sun50i-h6-sid", .data = &sun50i_h6_cfg },
--
2.35.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] nvmem: sunxi_sid: Drop the workaround on A64
2022-08-14 17:36 ` [PATCH 2/4] nvmem: sunxi_sid: Drop the workaround on A64 Samuel Holland
@ 2022-08-15 8:37 ` Icenowy Zheng
2022-08-16 0:16 ` Samuel Holland
0 siblings, 1 reply; 13+ messages in thread
From: Icenowy Zheng @ 2022-08-15 8:37 UTC (permalink / raw)
To: Samuel Holland, Srinivas Kandagatla, Chen-Yu Tsai, Jernej Skrabec
Cc: Greg Kroah-Hartman, Krzysztof Kozlowski, Rob Herring, devicetree,
linux-arm-kernel, linux-kernel, linux-sunxi
在 2022-08-14星期日的 12:36 -0500,Samuel Holland写道:
> Now that the SRAM readout code is fixed by using 32-bit accesses, it
> always returns the same values as register readout, so the A64
> variant
> no longer needs the workaround. This makes the D1 variant structure
> redundant, so remove it.
Is this really tested on A64?
>
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>
> drivers/nvmem/sunxi_sid.c | 8 +-------
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
> index 92dfe4cb10e3..a970f1741cc6 100644
> --- a/drivers/nvmem/sunxi_sid.c
> +++ b/drivers/nvmem/sunxi_sid.c
> @@ -197,15 +197,9 @@ static const struct sunxi_sid_cfg sun8i_h3_cfg =
> {
> .need_register_readout = true,
> };
>
> -static const struct sunxi_sid_cfg sun20i_d1_cfg = {
> - .value_offset = 0x200,
> - .size = 0x100,
> -};
> -
> static const struct sunxi_sid_cfg sun50i_a64_cfg = {
> .value_offset = 0x200,
> .size = 0x100,
> - .need_register_readout = true,
> };
>
> static const struct sunxi_sid_cfg sun50i_h6_cfg = {
> @@ -218,7 +212,7 @@ static const struct of_device_id
> sunxi_sid_of_match[] = {
> { .compatible = "allwinner,sun7i-a20-sid", .data =
> &sun7i_a20_cfg },
> { .compatible = "allwinner,sun8i-a83t-sid", .data =
> &sun50i_a64_cfg },
> { .compatible = "allwinner,sun8i-h3-sid", .data =
> &sun8i_h3_cfg },
> - { .compatible = "allwinner,sun20i-d1-sid", .data =
> &sun20i_d1_cfg },
> + { .compatible = "allwinner,sun20i-d1-sid", .data =
> &sun50i_a64_cfg },
> { .compatible = "allwinner,sun50i-a64-sid", .data =
> &sun50i_a64_cfg },
> { .compatible = "allwinner,sun50i-h5-sid", .data =
> &sun50i_a64_cfg },
> { .compatible = "allwinner,sun50i-h6-sid", .data =
> &sun50i_h6_cfg },
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] nvmem: sunxi_sid: Drop the workaround on A64
2022-08-15 8:37 ` Icenowy Zheng
@ 2022-08-16 0:16 ` Samuel Holland
0 siblings, 0 replies; 13+ messages in thread
From: Samuel Holland @ 2022-08-16 0:16 UTC (permalink / raw)
To: Icenowy Zheng, Srinivas Kandagatla, Chen-Yu Tsai, Jernej Skrabec
Cc: Greg Kroah-Hartman, Krzysztof Kozlowski, Rob Herring, devicetree,
linux-arm-kernel, linux-kernel, linux-sunxi
Hi Icenowy,
On 8/15/22 3:37 AM, Icenowy Zheng wrote:
> 在 2022-08-14星期日的 12:36 -0500,Samuel Holland写道:
>> Now that the SRAM readout code is fixed by using 32-bit accesses, it
>> always returns the same values as register readout, so the A64
>> variant
>> no longer needs the workaround. This makes the D1 variant structure
>> redundant, so remove it.
>
> Is this really tested on A64?
Yes, I tested this on a Pine A64-LTS. You can see the difference easily with
md.{b,w,l,q} in the U-Boot shell.
I verified that all three of the following are identical:
- NVMEM exported to sysfs with the register readout method
- NVMEM exported to sysfs with this patch series applied
- SRAM dump made with md.l in the U-Boot shell
Regards,
Samuel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/4] dt-bindings: nvmem: Allow bit offsets greater than a byte
2022-08-14 17:36 [PATCH 0/4] nvmem: Support non-stride-aligned NVMEM cell data Samuel Holland
2022-08-14 17:36 ` [PATCH 1/4] nvmem: sunxi_sid: Always use 32-bit MMIO reads Samuel Holland
2022-08-14 17:36 ` [PATCH 2/4] nvmem: sunxi_sid: Drop the workaround on A64 Samuel Holland
@ 2022-08-14 17:36 ` Samuel Holland
2022-08-25 21:02 ` Rob Herring
2022-08-14 17:36 ` [PATCH 4/4] nvmem: core: Support reading cells with >= 8 bit offsets Samuel Holland
3 siblings, 1 reply; 13+ messages in thread
From: Samuel Holland @ 2022-08-14 17:36 UTC (permalink / raw)
To: Srinivas Kandagatla, Chen-Yu Tsai, Jernej Skrabec
Cc: Samuel Holland, Greg Kroah-Hartman, Krzysztof Kozlowski,
Rob Herring, devicetree, linux-arm-kernel, linux-kernel,
linux-sunxi
Some NVMEM devices contain cells which do not start at a multiple of the
device's stride. However, the "reg" property of a cell must be aligned
to its provider device's stride.
These cells can be represented in the DT using the "bits" property if
that property allows offsets up to the full stride. 63 is chosen
assuming that NVMEM devices will not have strides larger than 8 bytes.
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
Documentation/devicetree/bindings/nvmem/nvmem.yaml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.yaml b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
index 3bb349c634cb..4f440ab6a13c 100644
--- a/Documentation/devicetree/bindings/nvmem/nvmem.yaml
+++ b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
@@ -53,7 +53,7 @@ patternProperties:
$ref: /schemas/types.yaml#/definitions/uint32-array
items:
- minimum: 0
- maximum: 7
+ maximum: 63
description:
Offset in bit within the address range specified by reg.
- minimum: 1
--
2.35.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/4] dt-bindings: nvmem: Allow bit offsets greater than a byte
2022-08-14 17:36 ` [PATCH 3/4] dt-bindings: nvmem: Allow bit offsets greater than a byte Samuel Holland
@ 2022-08-25 21:02 ` Rob Herring
2022-09-09 3:29 ` Samuel Holland
0 siblings, 1 reply; 13+ messages in thread
From: Rob Herring @ 2022-08-25 21:02 UTC (permalink / raw)
To: Samuel Holland
Cc: Srinivas Kandagatla, Chen-Yu Tsai, Jernej Skrabec,
Greg Kroah-Hartman, Krzysztof Kozlowski, devicetree,
linux-arm-kernel, linux-kernel, linux-sunxi
On Sun, Aug 14, 2022 at 12:36:54PM -0500, Samuel Holland wrote:
> Some NVMEM devices contain cells which do not start at a multiple of the
> device's stride. However, the "reg" property of a cell must be aligned
> to its provider device's stride.
How is a DT author supposed to know this?
I would lean toward it's the OS's problem to deal with (your option 1 I
guess). I worry that one client could expect it one way and another
client the other. Or folks making DT changes to 'fix' things.
>
> These cells can be represented in the DT using the "bits" property if
> that property allows offsets up to the full stride. 63 is chosen
> assuming that NVMEM devices will not have strides larger than 8 bytes.
>
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>
> Documentation/devicetree/bindings/nvmem/nvmem.yaml | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.yaml b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
> index 3bb349c634cb..4f440ab6a13c 100644
> --- a/Documentation/devicetree/bindings/nvmem/nvmem.yaml
> +++ b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
> @@ -53,7 +53,7 @@ patternProperties:
> $ref: /schemas/types.yaml#/definitions/uint32-array
> items:
> - minimum: 0
> - maximum: 7
> + maximum: 63
> description:
> Offset in bit within the address range specified by reg.
> - minimum: 1
> --
> 2.35.1
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/4] dt-bindings: nvmem: Allow bit offsets greater than a byte
2022-08-25 21:02 ` Rob Herring
@ 2022-09-09 3:29 ` Samuel Holland
2023-01-01 18:59 ` Samuel Holland
0 siblings, 1 reply; 13+ messages in thread
From: Samuel Holland @ 2022-09-09 3:29 UTC (permalink / raw)
To: Rob Herring
Cc: Srinivas Kandagatla, Chen-Yu Tsai, Jernej Skrabec,
Greg Kroah-Hartman, Krzysztof Kozlowski, devicetree,
linux-arm-kernel, linux-kernel, linux-sunxi
On 8/25/22 4:02 PM, Rob Herring wrote:
> On Sun, Aug 14, 2022 at 12:36:54PM -0500, Samuel Holland wrote:
>> Some NVMEM devices contain cells which do not start at a multiple of the
>> device's stride. However, the "reg" property of a cell must be aligned
>> to its provider device's stride.
>
> How is a DT author supposed to know this?
To know the stride? They know the compatible string of the NVMEM provider
device, and the stride is a property of the hardware.
To know that alignment to the stride is required? That's not documented in the
binding. I can add that to the "reg" description in this file.
> I would lean toward it's the OS's problem to deal with (your option 1 I
> guess). I worry that one client could expect it one way and another
> client the other. Or folks making DT changes to 'fix' things.
After this binding change, the meaning of
reg = <0x2a 1>;
bits = <0 8>; // optional
and
reg = <0x28 4>;
bits = <16 8>;
would be identical.
With option 1, only the first representation would be valid in the DT.
With this series (option 2), either representation would validate in the DT, but
only the second representation would be accepted by Linux for the driver in
question (sunxi-sid).
With option 3, either representation would work in the DT and Linux.
Note that there is no restriction on the bit length, so the following are
already equivalent today:
reg = <0x28 1>;
bits = <0 8>; // optional
and
reg = <0x28 2>;
bits = <0 8>;
So there are already multiple possible representations. I don't really think
that is a problem, since the meaning is still unambiguous.
Regards,
Samuel
>> These cells can be represented in the DT using the "bits" property if
>> that property allows offsets up to the full stride. 63 is chosen
>> assuming that NVMEM devices will not have strides larger than 8 bytes.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>> Documentation/devicetree/bindings/nvmem/nvmem.yaml | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.yaml b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
>> index 3bb349c634cb..4f440ab6a13c 100644
>> --- a/Documentation/devicetree/bindings/nvmem/nvmem.yaml
>> +++ b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
>> @@ -53,7 +53,7 @@ patternProperties:
>> $ref: /schemas/types.yaml#/definitions/uint32-array
>> items:
>> - minimum: 0
>> - maximum: 7
>> + maximum: 63
>> description:
>> Offset in bit within the address range specified by reg.
>> - minimum: 1
>> --
>> 2.35.1
>>
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/4] dt-bindings: nvmem: Allow bit offsets greater than a byte
2022-09-09 3:29 ` Samuel Holland
@ 2023-01-01 18:59 ` Samuel Holland
0 siblings, 0 replies; 13+ messages in thread
From: Samuel Holland @ 2023-01-01 18:59 UTC (permalink / raw)
To: Srinivas Kandagatla, Rob Herring
Cc: Chen-Yu Tsai, Jernej Skrabec, Greg Kroah-Hartman,
Krzysztof Kozlowski, devicetree, linux-arm-kernel, linux-kernel,
linux-sunxi
On 9/8/22 22:29, Samuel Holland wrote:
> On 8/25/22 4:02 PM, Rob Herring wrote:
>> On Sun, Aug 14, 2022 at 12:36:54PM -0500, Samuel Holland wrote:
>>> Some NVMEM devices contain cells which do not start at a multiple of the
>>> device's stride. However, the "reg" property of a cell must be aligned
>>> to its provider device's stride.
>>
>> How is a DT author supposed to know this?
>
> To know the stride? They know the compatible string of the NVMEM provider
> device, and the stride is a property of the hardware.
>
> To know that alignment to the stride is required? That's not documented in the
> binding. I can add that to the "reg" description in this file.
>
>> I would lean toward it's the OS's problem to deal with (your option 1 I
>> guess). I worry that one client could expect it one way and another
>> client the other. Or folks making DT changes to 'fix' things.
>
> After this binding change, the meaning of
>
> reg = <0x2a 1>;
> bits = <0 8>; // optional
>
> and
>
> reg = <0x28 4>;
> bits = <16 8>;
>
> would be identical.
>
> With option 1, only the first representation would be valid in the DT.
>
> With this series (option 2), either representation would validate in the DT, but
> only the second representation would be accepted by Linux for the driver in
> question (sunxi-sid).
>
> With option 3, either representation would work in the DT and Linux.
>
> Note that there is no restriction on the bit length, so the following are
> already equivalent today:
>
> reg = <0x28 1>;
> bits = <0 8>; // optional
>
> and
>
> reg = <0x28 2>;
> bits = <0 8>;
>
> So there are already multiple possible representations. I don't really think
> that is a problem, since the meaning is still unambiguous.
Does anyone have further thoughts on this?
From Rob's comment on the alignment being "the OS's problem", it sounds
like I should implement option 3 -- have the NVMEM core transform the
cell to match the driver's alignment/size requirements. Before I start
implementing it, does that sound workable?
Regards,
Samuel
>>> These cells can be represented in the DT using the "bits" property if
>>> that property allows offsets up to the full stride. 63 is chosen
>>> assuming that NVMEM devices will not have strides larger than 8 bytes.
>>>
>>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>>> ---
>>>
>>> Documentation/devicetree/bindings/nvmem/nvmem.yaml | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.yaml b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
>>> index 3bb349c634cb..4f440ab6a13c 100644
>>> --- a/Documentation/devicetree/bindings/nvmem/nvmem.yaml
>>> +++ b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
>>> @@ -53,7 +53,7 @@ patternProperties:
>>> $ref: /schemas/types.yaml#/definitions/uint32-array
>>> items:
>>> - minimum: 0
>>> - maximum: 7
>>> + maximum: 63
>>> description:
>>> Offset in bit within the address range specified by reg.
>>> - minimum: 1
>>> --
>>> 2.35.1
>>>
>>>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/4] nvmem: core: Support reading cells with >= 8 bit offsets
2022-08-14 17:36 [PATCH 0/4] nvmem: Support non-stride-aligned NVMEM cell data Samuel Holland
` (2 preceding siblings ...)
2022-08-14 17:36 ` [PATCH 3/4] dt-bindings: nvmem: Allow bit offsets greater than a byte Samuel Holland
@ 2022-08-14 17:36 ` Samuel Holland
3 siblings, 0 replies; 13+ messages in thread
From: Samuel Holland @ 2022-08-14 17:36 UTC (permalink / raw)
To: Srinivas Kandagatla, Chen-Yu Tsai, Jernej Skrabec
Cc: Samuel Holland, Greg Kroah-Hartman, Krzysztof Kozlowski,
Rob Herring, devicetree, linux-arm-kernel, linux-kernel,
linux-sunxi
For NVMEM devices with .stride > 1, some cell values may not be aligned
to the device's stride. In this case, it is necessary to use bit_offset
to access the cell. For example, to access the third byte of an NVMEM
device with .stride == 4, we need "bits = <16 8>;" in the devicetree.
Implement this on the read side. The write side implementation would be
more complicated, and it is not necessary for read-only NVMEM devices.
For now, reject writes for these cells to avoid any incorrect behavior.
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
drivers/nvmem/core.c | 43 ++++++++++++++++++++++++-------------------
1 file changed, 24 insertions(+), 19 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 1e3c754efd0d..309beba8c9f0 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -1373,63 +1373,67 @@ void nvmem_cell_put(struct nvmem_cell *cell)
}
EXPORT_SYMBOL_GPL(nvmem_cell_put);
-static void nvmem_shift_read_buffer_in_place(struct nvmem_cell_entry *cell, void *buf)
+static int nvmem_shift_read_buffer_in_place(struct nvmem_cell_entry *cell, void *buf)
{
+ int bit_offset = cell->bit_offset, bytes, i;
u8 *p, *b;
- int i, extra, bit_offset = cell->bit_offset;
p = b = buf;
if (bit_offset) {
+ int byte_offset = bit_offset / BITS_PER_BYTE;
+
+ b += byte_offset;
+ bit_offset %= BITS_PER_BYTE;
+ bytes = cell->bytes - byte_offset;
+
/* First shift */
- *b++ >>= bit_offset;
+ *p = *b++ >> bit_offset;
/* setup rest of the bytes if any */
- for (i = 1; i < cell->bytes; i++) {
+ for (i = 1; i < bytes; i++) {
/* Get bits from next byte and shift them towards msb */
- *p |= *b << (BITS_PER_BYTE - bit_offset);
-
- p = b;
- *b++ >>= bit_offset;
+ *p++ |= *b << (BITS_PER_BYTE - bit_offset);
+ *p = *b++ >> bit_offset;
}
- } else {
- /* point to the msb */
- p += cell->bytes - 1;
}
/* result fits in less bytes */
- extra = cell->bytes - DIV_ROUND_UP(cell->nbits, BITS_PER_BYTE);
- while (--extra >= 0)
- *p-- = 0;
+ bytes = DIV_ROUND_UP(cell->nbits, BITS_PER_BYTE);
+ p = buf + bytes;
+ memset(p, 0, cell->bytes - bytes);
/* clear msb bits if any leftover in the last byte */
if (cell->nbits % BITS_PER_BYTE)
- *p &= GENMASK((cell->nbits % BITS_PER_BYTE) - 1, 0);
+ p[-1] &= GENMASK((cell->nbits % BITS_PER_BYTE) - 1, 0);
+
+ return bytes;
}
static int __nvmem_cell_read(struct nvmem_device *nvmem,
struct nvmem_cell_entry *cell,
void *buf, size_t *len, const char *id)
{
+ int bytes = cell->bytes;
int rc;
- rc = nvmem_reg_read(nvmem, cell->offset, buf, cell->bytes);
+ rc = nvmem_reg_read(nvmem, cell->offset, buf, bytes);
if (rc)
return rc;
/* shift bits in-place */
if (cell->bit_offset || cell->nbits)
- nvmem_shift_read_buffer_in_place(cell, buf);
+ bytes = nvmem_shift_read_buffer_in_place(cell, buf);
if (nvmem->cell_post_process) {
rc = nvmem->cell_post_process(nvmem->priv, id,
- cell->offset, buf, cell->bytes);
+ cell->offset, buf, bytes);
if (rc)
return rc;
}
if (len)
- *len = cell->bytes;
+ *len = bytes;
return 0;
}
@@ -1526,6 +1530,7 @@ static int __nvmem_cell_entry_write(struct nvmem_cell_entry *cell, void *buf, si
int rc;
if (!nvmem || nvmem->read_only ||
+ cell->bit_offset >= BITS_PER_BYTE ||
(cell->bit_offset == 0 && len != cell->bytes))
return -EINVAL;
--
2.35.1
^ permalink raw reply related [flat|nested] 13+ messages in thread