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

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hello everyone,

Am Dienstag, 12. November 2024, 08:52:42 CET schrieb Cheng Ming Lin:
> 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.
> 
> Fixes: 0e30f47232ab5 ("mtd: spi-nor: add support for DTR protocol")
> Cc: stable@vger.kernel.org
> Reviewd-by: Pratyush Yadav <pratyush@kernel.org>
> 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);
> 

I just noticed this commit caused a regression on my i.MX8M Plus based board,
detected using git bisect.
DT: arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql-mba8mpxl.dts
Starting with this patch read is only 1S-1S-1S, before it was
1S-1S-4S.

before:
> cat /sys/kernel/debug/spi-nor/spi0.0/params
> name            mt25qu512a
> id              20 bb 20 10 44 00
> size            64.0 MiB
> write size      1
> page size       256
> address nbytes  4
> flags           HAS_SR_TB | 4B_OPCODES | HAS_4BAIT | HAS_LOCK | HAS_4BIT_BP
> | HAS_SR_BP3_BIT6 | SOFT_RESET
> 
> opcodes
> 
>  read           0x6c
>  
>   dummy cycles  8
>  
>  erase          0xdc
>  program        0x12
>  8D extension   none
> 
> protocols
> 
>  read           1S-1S-4S
>  write          1S-1S-1S
>  register       1S-1S-1S
> 
> erase commands
> 
>  21 (4.00 KiB) [1]
>  dc (64.0 KiB) [3]
>  c7 (64.0 MiB)
> 
> sector map
> 
>  region (in hex)   | erase mask | overlaid
>  ------------------+------------+----------
>  00000000-03ffffff |     [   3] | no

after:
> cat /sys/kernel/debug/spi-nor/spi0.0/params
> name            mt25qu512a
> id              20 bb 20 10 44 00
> size            64.0 MiB
> write size      1
> page size       256
> address nbytes  4
> flags           HAS_SR_TB | 4B_OPCODES | HAS_4BAIT | HAS_LOCK | HAS_4BIT_BP
> | HAS_SR_BP3_BIT6 | SOFT_RESET
> 
> opcodes
> 
>  read           0x13
>  
>   dummy cycles  0
>  
>  erase          0xdc
>  program        0x12
>  8D extension   none
> 
> protocols
> 
>  read           1S-1S-1S
>  write          1S-1S-1S
>  register       1S-1S-1S
> 
> erase commands
> 
>  21 (4.00 KiB) [1]
>  dc (64.0 KiB) [3]
>  c7 (64.0 MiB)
> 
> sector map
> 
>  region (in hex)   | erase mask | overlaid
>  ------------------+------------+----------
>  00000000-03ffffff |     [   3] | no

AFAICT the patch seems sane, so it probably just uncovered another
problem already lurking somewhere deeper.
Given the HW similarity I expect imx8mn and imx8mm based platforms to be
affected as well.
Reverting this commit make the read to be 1S-1S-4S again.
Any ideas ow to tackling down this problem?

Best regards,
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/