[PATCH 0/4] nvmem: Support non-stride-aligned NVMEM cell data

[PATCH 0/4] nvmem: Support non-stride-aligned NVMEM cell data

[Samuel Holland]

The first half of this series fixes a bug in the sunxi SID driver,
emphasizing that it really does have a hardware-level stride of 4 bytes.

The remainder of the series tries to answer the question:

    How can I use nvmem_cell_read_u8() to read byte 0x2a of a NVMEM
    device that has .stride == 4?

The NVMEM cell may be at a different offset in future SoCs, so I think
it would be wrong to use nvmem_cell_read_u32() and extract the single
relevant byte in the consumer driver.

I can think of three solutions:
 1) Change the NVMEM provider driver to use .stride == 1, and fix the
    alignment inside that driver. Some other NVMEM implementations have
    taken this path. This is not ideal because it requires allocating
    an extra bounce buffer inside the driver.
 2) Extend nvmem_shift_read_buffer_in_place() to handle larger bit
    offsets. Specify a stride-aligned "reg" in the devicetree, and use
    "bits" to provide the sub-stride offset. This adds a minimal amount
    of new code, and is generic across all drivers.
 3) Do the same as #2, but also remove the alignment checks from
    nvmem_cell_info_to_nvmem_cell_entry_nodup() and have it convert
    non-stride-aligned "reg" properties to the equivalent bit_offset
    and nbits fields (and use that from nvmem_add_cells_from_of()).

Since option #3 has larger impacts on the NVMEM core, and is backward-
compatible with option #2, I have implemented option #2 in this series.


Samuel Holland (4):
  nvmem: sunxi_sid: Always use 32-bit MMIO reads
  nvmem: sunxi_sid: Drop the workaround on A64
  dt-bindings: nvmem: Allow bit offsets greater than a byte
  nvmem: core: Support reading cells with >= 8 bit offsets

 .../devicetree/bindings/nvmem/nvmem.yaml      |  2 +-
 drivers/nvmem/core.c                          | 43 +++++++++++--------
 drivers/nvmem/sunxi_sid.c                     | 23 ++++++----
 3 files changed, 40 insertions(+), 28 deletions(-)

-- 
2.35.1

[PATCH 1/4] nvmem: sunxi_sid: Always use 32-bit MMIO reads

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

 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

[PATCH 2/4] nvmem: sunxi_sid: Drop the workaround on A64

[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.

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

[PATCH 3/4] dt-bindings: nvmem: Allow bit offsets greater than a byte

[Samuel Holland]

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

[PATCH 4/4] nvmem: core: Support reading cells with >= 8 bit offsets

[Samuel Holland]

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

Re: [PATCH 2/4] nvmem: sunxi_sid: Drop the workaround on A64

[Icenowy Zheng]

在 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 },

Re: [PATCH 2/4] nvmem: sunxi_sid: Drop the workaround on A64

[Samuel Holland]

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

Re: [PATCH 1/4] nvmem: sunxi_sid: Always use 32-bit MMIO reads

[Heiko Stübner]

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>

Re: [PATCH 3/4] dt-bindings: nvmem: Allow bit offsets greater than a byte

[Rob Herring]

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

Re: [PATCH 3/4] dt-bindings: nvmem: Allow bit offsets greater than a byte

[Samuel Holland]

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

Re: [PATCH 1/4] nvmem: sunxi_sid: Always use 32-bit MMIO reads

[Srinivas Kandagatla]

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

Re: [PATCH 3/4] dt-bindings: nvmem: Allow bit offsets greater than a byte

[Samuel Holland]

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

Re: [PATCH 1/4] nvmem: sunxi_sid: Always use 32-bit MMIO reads

[Jernej Škrabec]

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