* [PATCH] memory: renesas-rpc-if: Remove redundant division of dummy
@ 2023-01-12 9:06 Wolfram Sang
2023-01-13 8:10 ` Wolfram Sang
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Wolfram Sang @ 2023-01-12 9:06 UTC (permalink / raw)
To: linux-renesas-soc
Cc: Wolfram Sang, Krzysztof Kozlowski, Mark Brown, Sergei Shtylyov,
linux-kernel
From: Cong Dang <cong.dang.xn@renesas.com>
The dummy cycles value was wrongly calculated if dummy.buswidth > 1,
which affects QSPI, OSPI, HyperFlash on various SoCs. We're lucky in
Single SPI case since its dummy.buswidth equals to 1, so the result of
the division is unchanged
This issue can be reproduced using something like the following commands
A. QSPI mode: Mount device with jffs2 format
jffs2: CLEANMARKER node found at 0x00000004, not first node in block (0x00000000)
B. QSPI mode: Write data to mtd10, where mtd10 is a parition on SPI Flash
storage, defined properly in a device tree
[Correct fragment, read from SPI Flash]
root@v3x:~# echo "hello" > /dev/mtd10
root@v3x:~# hexdump -C -n100 /dev/mtd10
00000000 68 65 6c 6c 6f 0a ff ff ff ff ff ff ff ff ff ff |hello...........|
00000010 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
[Incorrect read of the same fragment: see the difference at offsets 0-3]
root@v3x:~# echo "hello" > /dev/mtd10
root@v3x:~# hexdump -C -n100 /dev/mtd10
00000000 00 00 00 00 68 65 6c 6c 6f 0a ff ff ff ff ff ff |....hello.......|
00000010 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
As seen from the result, 4 NULL bytes were inserted before the test data.
Wrong calculation in rpcif_prepare() led to miss of some dummy cycle. A
division by bus width is redundant because it had been performed already
in spi-rpc-if.c::rpcif_spi_mem_prepare()
Fix this by removing the redundant division.
Fixes: ca7d8b980b67 ("memory: add Renesas RPC-IF driver")
Signed-off-by: Cong Dang <cong.dang.xn@renesas.com>
Signed-off-by: Hai Pham <hai.pham.ud@renesas.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
Sadly, I cannot test this patch myself because I don't have access to
hardware which uses a buswidth > 1 for the dummy read. However, from
code review, this patch makes sense. The division by buswidth is done
twice, once in the SPI driver and once in the RPC core. It should stay
only in the SPI driver.
drivers/memory/renesas-rpc-if.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/memory/renesas-rpc-if.c b/drivers/memory/renesas-rpc-if.c
index 09cd4318a83d..c36b407851ff 100644
--- a/drivers/memory/renesas-rpc-if.c
+++ b/drivers/memory/renesas-rpc-if.c
@@ -430,8 +430,7 @@ void rpcif_prepare(struct rpcif *rpc, const struct rpcif_op *op, u64 *offs,
if (op->dummy.buswidth) {
rpc->enable |= RPCIF_SMENR_DME;
- rpc->dummy = RPCIF_SMDMCR_DMCYC(op->dummy.ncycles /
- op->dummy.buswidth);
+ rpc->dummy = RPCIF_SMDMCR_DMCYC(op->dummy.ncycles);
}
if (op->option.buswidth) {
--
2.30.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] memory: renesas-rpc-if: Remove redundant division of dummy
2023-01-12 9:06 [PATCH] memory: renesas-rpc-if: Remove redundant division of dummy Wolfram Sang
@ 2023-01-13 8:10 ` Wolfram Sang
2023-01-20 17:52 ` Krzysztof Kozlowski
2023-01-26 10:15 ` Krzysztof Kozlowski
2 siblings, 0 replies; 4+ messages in thread
From: Wolfram Sang @ 2023-01-13 8:10 UTC (permalink / raw)
To: linux-renesas-soc
Cc: Krzysztof Kozlowski, Mark Brown, Sergey Shtylyov, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2979 bytes --]
Using Sergei's recent email...
On Thu, Jan 12, 2023 at 10:06:55AM +0100, Wolfram Sang wrote:
> From: Cong Dang <cong.dang.xn@renesas.com>
>
> The dummy cycles value was wrongly calculated if dummy.buswidth > 1,
> which affects QSPI, OSPI, HyperFlash on various SoCs. We're lucky in
> Single SPI case since its dummy.buswidth equals to 1, so the result of
> the division is unchanged
>
> This issue can be reproduced using something like the following commands
> A. QSPI mode: Mount device with jffs2 format
> jffs2: CLEANMARKER node found at 0x00000004, not first node in block (0x00000000)
>
> B. QSPI mode: Write data to mtd10, where mtd10 is a parition on SPI Flash
> storage, defined properly in a device tree
>
> [Correct fragment, read from SPI Flash]
>
> root@v3x:~# echo "hello" > /dev/mtd10
> root@v3x:~# hexdump -C -n100 /dev/mtd10
> 00000000 68 65 6c 6c 6f 0a ff ff ff ff ff ff ff ff ff ff |hello...........|
> 00000010 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
>
> [Incorrect read of the same fragment: see the difference at offsets 0-3]
>
> root@v3x:~# echo "hello" > /dev/mtd10
> root@v3x:~# hexdump -C -n100 /dev/mtd10
> 00000000 00 00 00 00 68 65 6c 6c 6f 0a ff ff ff ff ff ff |....hello.......|
> 00000010 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
>
> As seen from the result, 4 NULL bytes were inserted before the test data.
> Wrong calculation in rpcif_prepare() led to miss of some dummy cycle. A
> division by bus width is redundant because it had been performed already
> in spi-rpc-if.c::rpcif_spi_mem_prepare()
>
> Fix this by removing the redundant division.
>
> Fixes: ca7d8b980b67 ("memory: add Renesas RPC-IF driver")
> Signed-off-by: Cong Dang <cong.dang.xn@renesas.com>
> Signed-off-by: Hai Pham <hai.pham.ud@renesas.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> Sadly, I cannot test this patch myself because I don't have access to
> hardware which uses a buswidth > 1 for the dummy read. However, from
> code review, this patch makes sense. The division by buswidth is done
> twice, once in the SPI driver and once in the RPC core. It should stay
> only in the SPI driver.
>
> drivers/memory/renesas-rpc-if.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/memory/renesas-rpc-if.c b/drivers/memory/renesas-rpc-if.c
> index 09cd4318a83d..c36b407851ff 100644
> --- a/drivers/memory/renesas-rpc-if.c
> +++ b/drivers/memory/renesas-rpc-if.c
> @@ -430,8 +430,7 @@ void rpcif_prepare(struct rpcif *rpc, const struct rpcif_op *op, u64 *offs,
>
> if (op->dummy.buswidth) {
> rpc->enable |= RPCIF_SMENR_DME;
> - rpc->dummy = RPCIF_SMDMCR_DMCYC(op->dummy.ncycles /
> - op->dummy.buswidth);
> + rpc->dummy = RPCIF_SMDMCR_DMCYC(op->dummy.ncycles);
> }
>
> if (op->option.buswidth) {
> --
> 2.30.2
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] memory: renesas-rpc-if: Remove redundant division of dummy
2023-01-12 9:06 [PATCH] memory: renesas-rpc-if: Remove redundant division of dummy Wolfram Sang
2023-01-13 8:10 ` Wolfram Sang
@ 2023-01-20 17:52 ` Krzysztof Kozlowski
2023-01-26 10:15 ` Krzysztof Kozlowski
2 siblings, 0 replies; 4+ messages in thread
From: Krzysztof Kozlowski @ 2023-01-20 17:52 UTC (permalink / raw)
To: Wolfram Sang, linux-renesas-soc; +Cc: Mark Brown, Sergei Shtylyov, linux-kernel
On 12/01/2023 10:06, Wolfram Sang wrote:
> From: Cong Dang <cong.dang.xn@renesas.com>
>
> The dummy cycles value was wrongly calculated if dummy.buswidth > 1,
> which affects QSPI, OSPI, HyperFlash on various SoCs. We're lucky in
> Single SPI case since its dummy.buswidth equals to 1, so the result of
> the division is unchanged
>
> This issue can be reproduced using something like the following commands
> A. QSPI mode: Mount device with jffs2 format
> jffs2: CLEANMARKER node found at 0x00000004, not first node in block (0x00000000)
>
> B. QSPI mode: Write data to mtd10, where mtd10 is a parition on SPI Flash
> storage, defined properly in a device tree
>
> [Correct fragment, read from SPI Flash]
>
> root@v3x:~# echo "hello" > /dev/mtd10
> root@v3x:~# hexdump -C -n100 /dev/mtd10
> 00000000 68 65 6c 6c 6f 0a ff ff ff ff ff ff ff ff ff ff |hello...........|
> 00000010 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
>
> [Incorrect read of the same fragment: see the difference at offsets 0-3]
>
> root@v3x:~# echo "hello" > /dev/mtd10
> root@v3x:~# hexdump -C -n100 /dev/mtd10
> 00000000 00 00 00 00 68 65 6c 6c 6f 0a ff ff ff ff ff ff |....hello.......|
> 00000010 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
>
> As seen from the result, 4 NULL bytes were inserted before the test data.
> Wrong calculation in rpcif_prepare() led to miss of some dummy cycle. A
> division by bus width is redundant because it had been performed already
> in spi-rpc-if.c::rpcif_spi_mem_prepare()
>
> Fix this by removing the redundant division.
>
> Fixes: ca7d8b980b67 ("memory: add Renesas RPC-IF driver")
> Signed-off-by: Cong Dang <cong.dang.xn@renesas.com>
> Signed-off-by: Hai Pham <hai.pham.ud@renesas.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> Sadly, I cannot test this patch myself because I don't have access to
> hardware which uses a buswidth > 1 for the dummy read. However, from
> code review, this patch makes sense. The division by buswidth is done
> twice, once in the SPI driver and once in the RPC core. It should stay
> only in the SPI driver.
Any tests or further reviews on this? If not, I'll pick it up in few days.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] memory: renesas-rpc-if: Remove redundant division of dummy
2023-01-12 9:06 [PATCH] memory: renesas-rpc-if: Remove redundant division of dummy Wolfram Sang
2023-01-13 8:10 ` Wolfram Sang
2023-01-20 17:52 ` Krzysztof Kozlowski
@ 2023-01-26 10:15 ` Krzysztof Kozlowski
2 siblings, 0 replies; 4+ messages in thread
From: Krzysztof Kozlowski @ 2023-01-26 10:15 UTC (permalink / raw)
To: Wolfram Sang, linux-renesas-soc
Cc: Krzysztof Kozlowski, Mark Brown, Sergei Shtylyov, linux-kernel
On Thu, 12 Jan 2023 10:06:55 +0100, Wolfram Sang wrote:
> From: Cong Dang <cong.dang.xn@renesas.com>
>
> The dummy cycles value was wrongly calculated if dummy.buswidth > 1,
> which affects QSPI, OSPI, HyperFlash on various SoCs. We're lucky in
> Single SPI case since its dummy.buswidth equals to 1, so the result of
> the division is unchanged
>
> [...]
Applied, thanks!
However it is late in the cycle. I'll try to squeeze it in, but there is a
chance this might miss upcoming merge window. If that happens, I will keep the
patch for the next-next merge window.
[1/1] memory: renesas-rpc-if: Remove redundant division of dummy
https://git.kernel.org/krzk/linux-mem-ctrl/c/cb1ed99d9520892616c6d566efbe2d92a84a576a
Best regards,
--
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-01-26 10:16 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-12 9:06 [PATCH] memory: renesas-rpc-if: Remove redundant division of dummy Wolfram Sang
2023-01-13 8:10 ` Wolfram Sang
2023-01-20 17:52 ` Krzysztof Kozlowski
2023-01-26 10:15 ` Krzysztof Kozlowski
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).