| Message ID | 20220814173656.11856-1-samuel@sholland.org |
|---|---|
| Headers | show |
| Series | nvmem: Support non-stride-aligned NVMEM cell data | expand |
在 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 },
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>
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; > }
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
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(-)