Re: [PATCH] mtd: spi-nor: core: replace dummy buswidth from addr to data

[Tudor Ambarus <tudor.ambarus@linaro.org>]

On 11/7/24 9:30 AM, Cheng Ming Lin wrote:
> From: Cheng Ming Lin <chengminglin@mxic.com.tw>
> 
> The default dummy cycle for Macronix SPI NOR flash in Octal Output
> Read Mode(1-1-8) is 20.
> 
> Currently, the dummy buswidth is set according to the address bus width.
> In the 1-1-8 mode, this means the dummy buswidth is 1. When converting
> dummy cycles to bytes, this results in 20 x 1 / 8 = 2 bytes, causing the
> host to read data 4 cycles too early.
> 
> Since the protocol data buswidth is always greater than or equal to the
> address buswidth. Setting the dummy buswidth to match the data buswidth
> increases the likelihood that the dummy cycle-to-byte conversion will be
> divisible, preventing the host from reading data prematurely.

This is still very wrong and the `fix` is working just by chance.
Consider what happens when one requires 10 dummy cycles. BTW, does this
fix a real problem, or it's just a theoretical fix?

I don't like that we're chasing our tails here. The fix is to pass
directly cycles instead of bytes and update MTD and SPI to use cycles. I
had an attempt to fix this in the past, but I can't allocate time to
respin. Are you interested in taking over and fixing MTD and SPI?

> Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw>
> ---
>  drivers/mtd/spi-nor/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index f9c189ed7353..c7aceaa8a43f 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -89,7 +89,7 @@ void spi_nor_spimem_setup_op(const struct spi_nor *nor,
>  		op->addr.buswidth = spi_nor_get_protocol_addr_nbits(proto);
>  
>  	if (op->dummy.nbytes)
> -		op->dummy.buswidth = spi_nor_get_protocol_addr_nbits(proto);
> +		op->dummy.buswidth = spi_nor_get_protocol_data_nbits(proto);
>  
>  	if (op->data.nbytes)
>  		op->data.buswidth = spi_nor_get_protocol_data_nbits(proto);