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

[Tudor Ambarus]

On 1/14/25 12:57 PM, Alexander Stein wrote:
> Hello everyone,

Hi,

> 
> Am Dienstag, 12. November 2024, 08:52:42 CET schrieb Cheng Ming Lin:
>> From: Cheng Ming Lin <chengminglin@xxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxx
>> Reviewd-by: Pratyush Yadav <pratyush@xxxxxxxxxx>
>> Signed-off-by: Cheng Ming Lin <chengminglin@xxxxxxxxxxx>
>> ---
>>  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?
> 

My guess is that 1S-1S-4S is stripped out in
spi_nor_spimem_adjust_hwcaps(). Maybe the controller has some limitation
in nxp_fspi_supports_op(). Would you add some prints, and check these
chunks of code?

Cheers,
ta