Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <87005399-4cef-c705-24ee-11ed49071ddc@linaro.org>
Date:   Fri, 20 Jan 2023 18:52:12 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.0
Subject: Re: [PATCH] memory: renesas-rpc-if: Remove redundant division of
 dummy
Content-Language: en-US
To:     Wolfram Sang <wsa+renesas@sang-engineering.com>,
        linux-renesas-soc@vger.kernel.org
Cc:     Mark Brown <broonie@kernel.org>,
        Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>,
        linux-kernel@vger.kernel.org
References: <20230112090655.43367-1-wsa+renesas@sang-engineering.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230112090655.43367-1-wsa+renesas@sang-engineering.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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