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

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

[Cheng Ming Lin]

From: Cheng Ming Lin <chengminglin@mxic.com.tw>

Set the dummy buswidth to match the data buswidth.

v2:
* Add fixes tag and cc to stable.

Cheng Ming Lin (1):
  mtd: spi-nor: core: replace dummy buswidth from addr to data

 drivers/mtd/spi-nor/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Cheng Ming Lin]

From: Cheng Ming Lin <chengminglin@mxic.com.tw>

Set the dummy buswidth to match the data buswidth.

v2:
* Add fixes tag and cc to stable.

Cheng Ming Lin (1):
  mtd: spi-nor: core: replace dummy buswidth from addr to data

 drivers/mtd/spi-nor/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.25.1

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

[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);
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[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);
-- 
2.25.1

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

[Alexander Stein]

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/



______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Alexander Stein]

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/

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@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?
> 

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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@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?
> 

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

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

[Alexander Stein]

Hi Tudor,

Am Dienstag, 14. Januar 2025, 14:26:47 CET schrieb 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@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?
> > 
> 
> 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?

Thanks for the fast response. I was able to track it down.
Eventually the buswidth check in spi_check_buswidth_req fails.
For command 0x3c:
Before revert:
> mode: 0x800, buswidth: 2
After revert
> mode: 0x800, buswidth: 1

The mode is set to SPI_RX_QUAD. Thus the check for dummy buswidth fails
now that data_nbits are used now.

For command 0x6c it's similar but op->dummy.buswidth is 4 now.

It boils down that there are SPI controllers which have
> spi-tx-bus-width = <1>;
> spi-rx-bus-width = <4>;
set in their DT nodes.

So it seems this combination is not supported.

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/



______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Alexander Stein]

Hi Tudor,

Am Dienstag, 14. Januar 2025, 14:26:47 CET schrieb 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@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?
> > 
> 
> 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?

Thanks for the fast response. I was able to track it down.
Eventually the buswidth check in spi_check_buswidth_req fails.
For command 0x3c:
Before revert:
> mode: 0x800, buswidth: 2
After revert
> mode: 0x800, buswidth: 1

The mode is set to SPI_RX_QUAD. Thus the check for dummy buswidth fails
now that data_nbits are used now.

For command 0x6c it's similar but op->dummy.buswidth is 4 now.

It boils down that there are SPI controllers which have
> spi-tx-bus-width = <1>;
> spi-rx-bus-width = <4>;
set in their DT nodes.

So it seems this combination is not supported.

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/

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

[Pratyush Yadav]

On Tue, Jan 14 2025, Alexander Stein wrote:

> Hi Tudor,
>
> Am Dienstag, 14. Januar 2025, 14:26:47 CET schrieb 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@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?
>> > 
>> 
>> 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?
>
> Thanks for the fast response. I was able to track it down.
> Eventually the buswidth check in spi_check_buswidth_req fails.
> For command 0x3c:
> Before revert:
>> mode: 0x800, buswidth: 2
> After revert
>> mode: 0x800, buswidth: 1
>
> The mode is set to SPI_RX_QUAD. Thus the check for dummy buswidth fails
> now that data_nbits are used now.
>
> For command 0x6c it's similar but op->dummy.buswidth is 4 now.
>
> It boils down that there are SPI controllers which have
>> spi-tx-bus-width = <1>;
>> spi-rx-bus-width = <4>;
> set in their DT nodes.
>
> So it seems this combination is not supported.

No, this check is wrong. See
https://lore.kernel.org/linux-mtd/20241112075242.174010-1-linchengming884@gmail.com/T/#m7cc1a5055702f5a42a8f3949c968842d845914d7

-- 
Regards,
Pratyush Yadav

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Pratyush Yadav]

On Tue, Jan 14 2025, Alexander Stein wrote:

> Hi Tudor,
>
> Am Dienstag, 14. Januar 2025, 14:26:47 CET schrieb 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@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?
>> > 
>> 
>> 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?
>
> Thanks for the fast response. I was able to track it down.
> Eventually the buswidth check in spi_check_buswidth_req fails.
> For command 0x3c:
> Before revert:
>> mode: 0x800, buswidth: 2
> After revert
>> mode: 0x800, buswidth: 1
>
> The mode is set to SPI_RX_QUAD. Thus the check for dummy buswidth fails
> now that data_nbits are used now.
>
> For command 0x6c it's similar but op->dummy.buswidth is 4 now.
>
> It boils down that there are SPI controllers which have
>> spi-tx-bus-width = <1>;
>> spi-rx-bus-width = <4>;
> set in their DT nodes.
>
> So it seems this combination is not supported.

No, this check is wrong. See
https://lore.kernel.org/linux-mtd/20241112075242.174010-1-linchengming884@gmail.com/T/#m7cc1a5055702f5a42a8f3949c968842d845914d7

-- 
Regards,
Pratyush Yadav

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

[Pratyush Yadav]

On Tue, Jan 14 2025, Alexander Stein wrote:

> 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?

Thanks for reporting this. I spent some time digging through this, and I
think I know what is going on.

Most controller's supports_op hook call spi_mem_default_supports_op(),
including nxp_fspi_supports_op(). In spi_mem_default_supports_op(),
spi_mem_check_buswidth() is called to check if the buswidths for the op
can actually be supported by the board's wiring. This wiring information
comes from (among other things) the spi-{tx,rx}-bus-width DT properties.
Based on these properties, SPI_TX_* or SPI_RX_* flags are set by
of_spi_parse_dt(). spi_mem_check_buswidth() then uses these flags to
make the decision whether an op can be supported by the board's wiring
(in a way, indirectly checking against spi-{rx,tx}-bus-width).

In arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql.dtsi we have:

	flash0: flash@0 {
		reg = <0>;
		compatible = "jedec,spi-nor";
		spi-max-frequency = <80000000>;
		spi-tx-bus-width = <1>;
		spi-rx-bus-width = <4>;

Now the tricky bit here is we do the below in spi_mem_check_buswidth():

	if (op->dummy.nbytes &&
	    spi_check_buswidth_req(mem, op->dummy.buswidth, true))
		return false;

The "true" parameter here means to "treat the op as TX". Since the board
only supports 1-bit TX, the 4-bit dummy TX is considered as unsupported,
and the op gets rejected. In reality, a dummy phase is neither a RX nor
a TX. We should ideally treat it differently, and only check if it is
one of 1, 2, 4, or 8, and not test it against the board capabilities at
all.

Alexander, can you test my theory by making sure it is indeed the dummy
check in spi_mem_check_buswidth() that fails, and either removing it or
passing "false" instead of "true" to spi_check_buswidth_req() fixes the
bug for you?

I took a quick look and the spi-tx-bus-width == 1 and spi-rx-bus-width >
1 combination seems to be quite common so I think many other boards are
affected by this bug as well.

Since we are quite late in the cycle, and that changing
spi_mem_check_buswidth() might cause all sorts of breakages, I think the
best idea currently would be to revert this patch, and resend it with
the other changes later.

Tudor, Michael, Miquel, what do you think about this? We are at rc7 but
I think we should send out a fixes PR with a revert. If you agree, I
will send out a patch and a PR.

-- 
Regards,
Pratyush Yadav

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Pratyush Yadav]

On Tue, Jan 14 2025, Alexander Stein wrote:

> 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?

Thanks for reporting this. I spent some time digging through this, and I
think I know what is going on.

Most controller's supports_op hook call spi_mem_default_supports_op(),
including nxp_fspi_supports_op(). In spi_mem_default_supports_op(),
spi_mem_check_buswidth() is called to check if the buswidths for the op
can actually be supported by the board's wiring. This wiring information
comes from (among other things) the spi-{tx,rx}-bus-width DT properties.
Based on these properties, SPI_TX_* or SPI_RX_* flags are set by
of_spi_parse_dt(). spi_mem_check_buswidth() then uses these flags to
make the decision whether an op can be supported by the board's wiring
(in a way, indirectly checking against spi-{rx,tx}-bus-width).

In arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql.dtsi we have:

	flash0: flash@0 {
		reg = <0>;
		compatible = "jedec,spi-nor";
		spi-max-frequency = <80000000>;
		spi-tx-bus-width = <1>;
		spi-rx-bus-width = <4>;

Now the tricky bit here is we do the below in spi_mem_check_buswidth():

	if (op->dummy.nbytes &&
	    spi_check_buswidth_req(mem, op->dummy.buswidth, true))
		return false;

The "true" parameter here means to "treat the op as TX". Since the board
only supports 1-bit TX, the 4-bit dummy TX is considered as unsupported,
and the op gets rejected. In reality, a dummy phase is neither a RX nor
a TX. We should ideally treat it differently, and only check if it is
one of 1, 2, 4, or 8, and not test it against the board capabilities at
all.

Alexander, can you test my theory by making sure it is indeed the dummy
check in spi_mem_check_buswidth() that fails, and either removing it or
passing "false" instead of "true" to spi_check_buswidth_req() fixes the
bug for you?

I took a quick look and the spi-tx-bus-width == 1 and spi-rx-bus-width >
1 combination seems to be quite common so I think many other boards are
affected by this bug as well.

Since we are quite late in the cycle, and that changing
spi_mem_check_buswidth() might cause all sorts of breakages, I think the
best idea currently would be to revert this patch, and resend it with
the other changes later.

Tudor, Michael, Miquel, what do you think about this? We are at rc7 but
I think we should send out a fixes PR with a revert. If you agree, I
will send out a patch and a PR.

-- 
Regards,
Pratyush Yadav

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

[Miquel Raynal]

Hello Pratyush,

On 14/01/2025 at 16:15:24 GMT, Pratyush Yadav <pratyush@kernel.org> wrote:

>>> --- 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);

Facing recently a similar issue myself in the SPI NAND world, I believe
we should get rid of the notion of bits when it comes to the dummy
phase. I would appreciate a clarification like "dummy.cycles" which
would typically not require any bus width implications.

...

> Most controller's supports_op hook call spi_mem_default_supports_op(),
> including nxp_fspi_supports_op(). In spi_mem_default_supports_op(),
> spi_mem_check_buswidth() is called to check if the buswidths for the op
> can actually be supported by the board's wiring. This wiring information
> comes from (among other things) the spi-{tx,rx}-bus-width DT properties.
> Based on these properties, SPI_TX_* or SPI_RX_* flags are set by
> of_spi_parse_dt(). spi_mem_check_buswidth() then uses these flags to
> make the decision whether an op can be supported by the board's wiring
> (in a way, indirectly checking against spi-{rx,tx}-bus-width).

Thanks for the whole explanation, it's pretty clear.

> In arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql.dtsi we have:
>
> 	flash0: flash@0 {
> 		reg = <0>;
> 		compatible = "jedec,spi-nor";
> 		spi-max-frequency = <80000000>;
> 		spi-tx-bus-width = <1>;
> 		spi-rx-bus-width = <4>;
>
> Now the tricky bit here is we do the below in spi_mem_check_buswidth():
>
> 	if (op->dummy.nbytes &&
> 	    spi_check_buswidth_req(mem, op->dummy.buswidth, true))
> 		return false;

May I challenge this entire section? Is there *any* reason to check
anything against dummy cycles wrt the width? Maybe a "can handle x
cycles" check would be interesting though, but I'd go for a different
helper, that is specific to the dummy cycles.

> The "true" parameter here means to "treat the op as TX". Since the
> board only supports 1-bit TX, the 4-bit dummy TX is considered as
> unsupported, and the op gets rejected. In reality, a dummy phase is
> neither a RX nor a TX. We should ideally treat it differently, and
> only check if it is one of 1, 2, 4, or 8, and not test it against the
> board capabilities at all.

...

> Since we are quite late in the cycle, and that changing
> spi_mem_check_buswidth() might cause all sorts of breakages, I think the
> best idea currently would be to revert this patch, and resend it with
> the other changes later.
>
> Tudor, Michael, Miquel, what do you think about this? We are at rc7 but
> I think we should send out a fixes PR with a revert. If you agree, I
> will send out a patch and a PR.

Either way I am fine. the -rc cycles are also available for us to
settle. But it's true we can bikeshed a little bit, so feel free to
revert this patch before sending the MR.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Miquel Raynal]

Hello Pratyush,

On 14/01/2025 at 16:15:24 GMT, Pratyush Yadav <pratyush@kernel.org> wrote:

>>> --- 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);

Facing recently a similar issue myself in the SPI NAND world, I believe
we should get rid of the notion of bits when it comes to the dummy
phase. I would appreciate a clarification like "dummy.cycles" which
would typically not require any bus width implications.

...

> Most controller's supports_op hook call spi_mem_default_supports_op(),
> including nxp_fspi_supports_op(). In spi_mem_default_supports_op(),
> spi_mem_check_buswidth() is called to check if the buswidths for the op
> can actually be supported by the board's wiring. This wiring information
> comes from (among other things) the spi-{tx,rx}-bus-width DT properties.
> Based on these properties, SPI_TX_* or SPI_RX_* flags are set by
> of_spi_parse_dt(). spi_mem_check_buswidth() then uses these flags to
> make the decision whether an op can be supported by the board's wiring
> (in a way, indirectly checking against spi-{rx,tx}-bus-width).

Thanks for the whole explanation, it's pretty clear.

> In arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql.dtsi we have:
>
> 	flash0: flash@0 {
> 		reg = <0>;
> 		compatible = "jedec,spi-nor";
> 		spi-max-frequency = <80000000>;
> 		spi-tx-bus-width = <1>;
> 		spi-rx-bus-width = <4>;
>
> Now the tricky bit here is we do the below in spi_mem_check_buswidth():
>
> 	if (op->dummy.nbytes &&
> 	    spi_check_buswidth_req(mem, op->dummy.buswidth, true))
> 		return false;

May I challenge this entire section? Is there *any* reason to check
anything against dummy cycles wrt the width? Maybe a "can handle x
cycles" check would be interesting though, but I'd go for a different
helper, that is specific to the dummy cycles.

> The "true" parameter here means to "treat the op as TX". Since the
> board only supports 1-bit TX, the 4-bit dummy TX is considered as
> unsupported, and the op gets rejected. In reality, a dummy phase is
> neither a RX nor a TX. We should ideally treat it differently, and
> only check if it is one of 1, 2, 4, or 8, and not test it against the
> board capabilities at all.

...

> Since we are quite late in the cycle, and that changing
> spi_mem_check_buswidth() might cause all sorts of breakages, I think the
> best idea currently would be to revert this patch, and resend it with
> the other changes later.
>
> Tudor, Michael, Miquel, what do you think about this? We are at rc7 but
> I think we should send out a fixes PR with a revert. If you agree, I
> will send out a patch and a PR.

Either way I am fine. the -rc cycles are also available for us to
settle. But it's true we can bikeshed a little bit, so feel free to
revert this patch before sending the MR.

Thanks,
Miquèl

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

[Pratyush Yadav]

On Tue, Jan 14 2025, Miquel Raynal wrote:

> Hello Pratyush,
>
> On 14/01/2025 at 16:15:24 GMT, Pratyush Yadav <pratyush@kernel.org> wrote:
>
>>>> --- 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);
>
> Facing recently a similar issue myself in the SPI NAND world, I believe
> we should get rid of the notion of bits when it comes to the dummy
> phase. I would appreciate a clarification like "dummy.cycles" which
> would typically not require any bus width implications.

I agree. All peripheral drivers convert cycles to bytes, and controller
drivers convert them back to cycles. This whole thing should be avoided,
especially since it contains some traps with division truncation.

>
> ...
>
>> Most controller's supports_op hook call spi_mem_default_supports_op(),
>> including nxp_fspi_supports_op(). In spi_mem_default_supports_op(),
>> spi_mem_check_buswidth() is called to check if the buswidths for the op
>> can actually be supported by the board's wiring. This wiring information
>> comes from (among other things) the spi-{tx,rx}-bus-width DT properties.
>> Based on these properties, SPI_TX_* or SPI_RX_* flags are set by
>> of_spi_parse_dt(). spi_mem_check_buswidth() then uses these flags to
>> make the decision whether an op can be supported by the board's wiring
>> (in a way, indirectly checking against spi-{rx,tx}-bus-width).
>
> Thanks for the whole explanation, it's pretty clear.
>
>> In arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql.dtsi we have:
>>
>> 	flash0: flash@0 {
>> 		reg = <0>;
>> 		compatible = "jedec,spi-nor";
>> 		spi-max-frequency = <80000000>;
>> 		spi-tx-bus-width = <1>;
>> 		spi-rx-bus-width = <4>;
>>
>> Now the tricky bit here is we do the below in spi_mem_check_buswidth():
>>
>> 	if (op->dummy.nbytes &&
>> 	    spi_check_buswidth_req(mem, op->dummy.buswidth, true))
>> 		return false;
>
> May I challenge this entire section? Is there *any* reason to check
> anything against dummy cycles wrt the width? Maybe a "can handle x
> cycles" check would be interesting though, but I'd go for a different
> helper, that is specific to the dummy cycles.

I suppose you would want to sanity check that the cycles are at least
between 1, 2, 4, or 8 (or at the very least not 0).

>
>> The "true" parameter here means to "treat the op as TX". Since the
>> board only supports 1-bit TX, the 4-bit dummy TX is considered as
>> unsupported, and the op gets rejected. In reality, a dummy phase is
>> neither a RX nor a TX. We should ideally treat it differently, and
>> only check if it is one of 1, 2, 4, or 8, and not test it against the
>> board capabilities at all.
>
> ...
>
>> Since we are quite late in the cycle, and that changing
>> spi_mem_check_buswidth() might cause all sorts of breakages, I think the
>> best idea currently would be to revert this patch, and resend it with
>> the other changes later.
>>
>> Tudor, Michael, Miquel, what do you think about this? We are at rc7 but
>> I think we should send out a fixes PR with a revert. If you agree, I
>> will send out a patch and a PR.
>
> Either way I am fine. the -rc cycles are also available for us to
> settle. But it's true we can bikeshed a little bit, so feel free to
> revert this patch before sending the MR.

To be clear, since the patch was added in v6.13-rc1 I want to revert it
via a fixes pull request to Linus before he releases v6.13 this week. I
want to fix it in v6.13, not in v6.14.

-- 
Regards,
Pratyush Yadav

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Pratyush Yadav]

On Tue, Jan 14 2025, Miquel Raynal wrote:

> Hello Pratyush,
>
> On 14/01/2025 at 16:15:24 GMT, Pratyush Yadav <pratyush@kernel.org> wrote:
>
>>>> --- 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);
>
> Facing recently a similar issue myself in the SPI NAND world, I believe
> we should get rid of the notion of bits when it comes to the dummy
> phase. I would appreciate a clarification like "dummy.cycles" which
> would typically not require any bus width implications.

I agree. All peripheral drivers convert cycles to bytes, and controller
drivers convert them back to cycles. This whole thing should be avoided,
especially since it contains some traps with division truncation.

>
> ...
>
>> Most controller's supports_op hook call spi_mem_default_supports_op(),
>> including nxp_fspi_supports_op(). In spi_mem_default_supports_op(),
>> spi_mem_check_buswidth() is called to check if the buswidths for the op
>> can actually be supported by the board's wiring. This wiring information
>> comes from (among other things) the spi-{tx,rx}-bus-width DT properties.
>> Based on these properties, SPI_TX_* or SPI_RX_* flags are set by
>> of_spi_parse_dt(). spi_mem_check_buswidth() then uses these flags to
>> make the decision whether an op can be supported by the board's wiring
>> (in a way, indirectly checking against spi-{rx,tx}-bus-width).
>
> Thanks for the whole explanation, it's pretty clear.
>
>> In arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql.dtsi we have:
>>
>> 	flash0: flash@0 {
>> 		reg = <0>;
>> 		compatible = "jedec,spi-nor";
>> 		spi-max-frequency = <80000000>;
>> 		spi-tx-bus-width = <1>;
>> 		spi-rx-bus-width = <4>;
>>
>> Now the tricky bit here is we do the below in spi_mem_check_buswidth():
>>
>> 	if (op->dummy.nbytes &&
>> 	    spi_check_buswidth_req(mem, op->dummy.buswidth, true))
>> 		return false;
>
> May I challenge this entire section? Is there *any* reason to check
> anything against dummy cycles wrt the width? Maybe a "can handle x
> cycles" check would be interesting though, but I'd go for a different
> helper, that is specific to the dummy cycles.

I suppose you would want to sanity check that the cycles are at least
between 1, 2, 4, or 8 (or at the very least not 0).

>
>> The "true" parameter here means to "treat the op as TX". Since the
>> board only supports 1-bit TX, the 4-bit dummy TX is considered as
>> unsupported, and the op gets rejected. In reality, a dummy phase is
>> neither a RX nor a TX. We should ideally treat it differently, and
>> only check if it is one of 1, 2, 4, or 8, and not test it against the
>> board capabilities at all.
>
> ...
>
>> Since we are quite late in the cycle, and that changing
>> spi_mem_check_buswidth() might cause all sorts of breakages, I think the
>> best idea currently would be to revert this patch, and resend it with
>> the other changes later.
>>
>> Tudor, Michael, Miquel, what do you think about this? We are at rc7 but
>> I think we should send out a fixes PR with a revert. If you agree, I
>> will send out a patch and a PR.
>
> Either way I am fine. the -rc cycles are also available for us to
> settle. But it's true we can bikeshed a little bit, so feel free to
> revert this patch before sending the MR.

To be clear, since the patch was added in v6.13-rc1 I want to revert it
via a fixes pull request to Linus before he releases v6.13 this week. I
want to fix it in v6.13, not in v6.14.

-- 
Regards,
Pratyush Yadav

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

[Michael Walle]

[-- Attachment #1.1: Type: text/plain, Size: 2053 bytes --]

Hi,

> >>>> --- 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);
> >
> > Facing recently a similar issue myself in the SPI NAND world, I believe
> > we should get rid of the notion of bits when it comes to the dummy
> > phase. I would appreciate a clarification like "dummy.cycles" which
> > would typically not require any bus width implications.
>
> I agree. All peripheral drivers convert cycles to bytes, and controller
> drivers convert them back to cycles. This whole thing should be avoided,
> especially since it contains some traps with division truncation.

Here is the relevant discussion:
https://lore.kernel.org/linux-mtd/f647e713a65f5d3f0f2e3af95c4d0a89@walle.cc/

TLDR: yes, please use the notion of (clock) cycles. But there are
some problems to solve first.

> >> Since we are quite late in the cycle, and that changing
> >> spi_mem_check_buswidth() might cause all sorts of breakages, I think the
> >> best idea currently would be to revert this patch, and resend it with
> >> the other changes later.
> >>
> >> Tudor, Michael, Miquel, what do you think about this? We are at rc7 but
> >> I think we should send out a fixes PR with a revert. If you agree, I
> >> will send out a patch and a PR.
> >
> > Either way I am fine. the -rc cycles are also available for us to
> > settle. But it's true we can bikeshed a little bit, so feel free to
> > revert this patch before sending the MR.
>
> To be clear, since the patch was added in v6.13-rc1 I want to revert it
> via a fixes pull request to Linus before he releases v6.13 this week. I
> want to fix it in v6.13, not in v6.14.

Since it's clearly a regression, I'd revert it.

-michael

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 2053 bytes --]

Hi,

> >>>> --- 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);
> >
> > Facing recently a similar issue myself in the SPI NAND world, I believe
> > we should get rid of the notion of bits when it comes to the dummy
> > phase. I would appreciate a clarification like "dummy.cycles" which
> > would typically not require any bus width implications.
>
> I agree. All peripheral drivers convert cycles to bytes, and controller
> drivers convert them back to cycles. This whole thing should be avoided,
> especially since it contains some traps with division truncation.

Here is the relevant discussion:
https://lore.kernel.org/linux-mtd/f647e713a65f5d3f0f2e3af95c4d0a89@walle.cc/

TLDR: yes, please use the notion of (clock) cycles. But there are
some problems to solve first.

> >> Since we are quite late in the cycle, and that changing
> >> spi_mem_check_buswidth() might cause all sorts of breakages, I think the
> >> best idea currently would be to revert this patch, and resend it with
> >> the other changes later.
> >>
> >> Tudor, Michael, Miquel, what do you think about this? We are at rc7 but
> >> I think we should send out a fixes PR with a revert. If you agree, I
> >> will send out a patch and a PR.
> >
> > Either way I am fine. the -rc cycles are also available for us to
> > settle. But it's true we can bikeshed a little bit, so feel free to
> > revert this patch before sending the MR.
>
> To be clear, since the patch was added in v6.13-rc1 I want to revert it
> via a fixes pull request to Linus before he releases v6.13 this week. I
> want to fix it in v6.13, not in v6.14.

Since it's clearly a regression, I'd revert it.

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

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

[Tudor Ambarus]

On 1/14/25 5:51 PM, Miquel Raynal wrote:
>> Tudor, Michael, Miquel, what do you think about this? We are at rc7 but
>> I think we should send out a fixes PR with a revert. If you agree, I
>> will send out a patch and a PR.
> Either way I am fine. the -rc cycles are also available for us to

same. You could also temporarily fix nxp driver by tracking RX, this way
having both macronix and nxp happy.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Tudor Ambarus]

On 1/14/25 5:51 PM, Miquel Raynal wrote:
>> Tudor, Michael, Miquel, what do you think about this? We are at rc7 but
>> I think we should send out a fixes PR with a revert. If you agree, I
>> will send out a patch and a PR.
> Either way I am fine. the -rc cycles are also available for us to

same. You could also temporarily fix nxp driver by tracking RX, this way
having both macronix and nxp happy.

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

[Alexander Stein]

Hi Pratyush,

Am Dienstag, 14. Januar 2025, 17:15:24 CET schrieb Pratyush Yadav:
> On Tue, Jan 14 2025, Alexander Stein wrote:
> 
> > 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?
> 
> Thanks for reporting this. I spent some time digging through this, and I
> think I know what is going on.
> 
> Most controller's supports_op hook call spi_mem_default_supports_op(),
> including nxp_fspi_supports_op(). In spi_mem_default_supports_op(),
> spi_mem_check_buswidth() is called to check if the buswidths for the op
> can actually be supported by the board's wiring. This wiring information
> comes from (among other things) the spi-{tx,rx}-bus-width DT properties.
> Based on these properties, SPI_TX_* or SPI_RX_* flags are set by
> of_spi_parse_dt(). spi_mem_check_buswidth() then uses these flags to
> make the decision whether an op can be supported by the board's wiring
> (in a way, indirectly checking against spi-{rx,tx}-bus-width).
> 
> In arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql.dtsi we have:
> 
> 	flash0: flash@0 {
> 		reg = <0>;
> 		compatible = "jedec,spi-nor";
> 		spi-max-frequency = <80000000>;
> 		spi-tx-bus-width = <1>;
> 		spi-rx-bus-width = <4>;
> 
> Now the tricky bit here is we do the below in spi_mem_check_buswidth():
> 
> 	if (op->dummy.nbytes &&
> 	    spi_check_buswidth_req(mem, op->dummy.buswidth, true))
> 		return false;
> 
> The "true" parameter here means to "treat the op as TX". Since the board
> only supports 1-bit TX, the 4-bit dummy TX is considered as unsupported,
> and the op gets rejected. In reality, a dummy phase is neither a RX nor
> a TX. We should ideally treat it differently, and only check if it is
> one of 1, 2, 4, or 8, and not test it against the board capabilities at
> all.
> 
> Alexander, can you test my theory by making sure it is indeed the dummy
> check in spi_mem_check_buswidth() that fails, and either removing it or
> passing "false" instead of "true" to spi_check_buswidth_req() fixes the
> bug for you?

Thanks for the explanation, this matches my observation. I'm using the
following diff
---8<---
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -150,7 +150,7 @@ static bool spi_mem_check_buswidth(struct spi_mem *mem,
                return false;
 
        if (op->dummy.nbytes &&
-           spi_check_buswidth_req(mem, op->dummy.buswidth, true))
+           spi_check_buswidth_req(mem, op->dummy.buswidth, false))
                return false;
 
        if (op->data.dir != SPI_MEM_NO_DATA &&
---8<---
and I'm back at read 1S-1S-4S. So your theory is correct.

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/



______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Alexander Stein]

Hi Pratyush,

Am Dienstag, 14. Januar 2025, 17:15:24 CET schrieb Pratyush Yadav:
> On Tue, Jan 14 2025, Alexander Stein wrote:
> 
> > 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?
> 
> Thanks for reporting this. I spent some time digging through this, and I
> think I know what is going on.
> 
> Most controller's supports_op hook call spi_mem_default_supports_op(),
> including nxp_fspi_supports_op(). In spi_mem_default_supports_op(),
> spi_mem_check_buswidth() is called to check if the buswidths for the op
> can actually be supported by the board's wiring. This wiring information
> comes from (among other things) the spi-{tx,rx}-bus-width DT properties.
> Based on these properties, SPI_TX_* or SPI_RX_* flags are set by
> of_spi_parse_dt(). spi_mem_check_buswidth() then uses these flags to
> make the decision whether an op can be supported by the board's wiring
> (in a way, indirectly checking against spi-{rx,tx}-bus-width).
> 
> In arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql.dtsi we have:
> 
> 	flash0: flash@0 {
> 		reg = <0>;
> 		compatible = "jedec,spi-nor";
> 		spi-max-frequency = <80000000>;
> 		spi-tx-bus-width = <1>;
> 		spi-rx-bus-width = <4>;
> 
> Now the tricky bit here is we do the below in spi_mem_check_buswidth():
> 
> 	if (op->dummy.nbytes &&
> 	    spi_check_buswidth_req(mem, op->dummy.buswidth, true))
> 		return false;
> 
> The "true" parameter here means to "treat the op as TX". Since the board
> only supports 1-bit TX, the 4-bit dummy TX is considered as unsupported,
> and the op gets rejected. In reality, a dummy phase is neither a RX nor
> a TX. We should ideally treat it differently, and only check if it is
> one of 1, 2, 4, or 8, and not test it against the board capabilities at
> all.
> 
> Alexander, can you test my theory by making sure it is indeed the dummy
> check in spi_mem_check_buswidth() that fails, and either removing it or
> passing "false" instead of "true" to spi_check_buswidth_req() fixes the
> bug for you?

Thanks for the explanation, this matches my observation. I'm using the
following diff
---8<---
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -150,7 +150,7 @@ static bool spi_mem_check_buswidth(struct spi_mem *mem,
                return false;
 
        if (op->dummy.nbytes &&
-           spi_check_buswidth_req(mem, op->dummy.buswidth, true))
+           spi_check_buswidth_req(mem, op->dummy.buswidth, false))
                return false;
 
        if (op->data.dir != SPI_MEM_NO_DATA &&
---8<---
and I'm back at read 1S-1S-4S. So your theory is correct.

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/

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

[Tudor Ambarus]

On Tue, 12 Nov 2024 15:52:41 +0800, Cheng Ming Lin wrote:
> Set the dummy buswidth to match the data buswidth.
> 
> v2:
> * Add fixes tag and cc to stable.
> 
> Cheng Ming Lin (1):
>   mtd: spi-nor: core: replace dummy buswidth from addr to data
> 
> [...]

You had some warnings which I fixed while applying:
WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: 0e30f47232ab ("mtd: spi-nor: add support for DTR protocol")'
#19: 
Fixes: 0e30f47232ab5 ("mtd: spi-nor: add support for DTR protocol")

WARNING: Non-standard signature: 'Reviewd-by:' - perhaps 'Reviewed-by:'?
#21: 
Reviewd-by: Pratyush Yadav <pratyush@kernel.org>

Applied to git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git,
spi-nor/next branch. Thanks!

[1/1] mtd: spi-nor: core: replace dummy buswidth from addr to data
      https://git.kernel.org/mtd/c/98d1fb94ce75

Cheers,
-- 
Tudor Ambarus <tudor.ambarus@linaro.org>

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Tudor Ambarus]

On Tue, 12 Nov 2024 15:52:41 +0800, Cheng Ming Lin wrote:
> Set the dummy buswidth to match the data buswidth.
> 
> v2:
> * Add fixes tag and cc to stable.
> 
> Cheng Ming Lin (1):
>   mtd: spi-nor: core: replace dummy buswidth from addr to data
> 
> [...]

You had some warnings which I fixed while applying:
WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: 0e30f47232ab ("mtd: spi-nor: add support for DTR protocol")'
#19: 
Fixes: 0e30f47232ab5 ("mtd: spi-nor: add support for DTR protocol")

WARNING: Non-standard signature: 'Reviewd-by:' - perhaps 'Reviewed-by:'?
#21: 
Reviewd-by: Pratyush Yadav <pratyush@kernel.org>

Applied to git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git,
spi-nor/next branch. Thanks!

[1/1] mtd: spi-nor: core: replace dummy buswidth from addr to data
      https://git.kernel.org/mtd/c/98d1fb94ce75

Cheers,
-- 
Tudor Ambarus <tudor.ambarus@linaro.org>

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

[Cheng Ming Lin]

Hi Tudor,

Tudor Ambarus <tudor.ambarus@linaro.org> 於 2024年11月12日 週二 下午4:36寫道：
>
> On Tue, 12 Nov 2024 15:52:41 +0800, Cheng Ming Lin wrote:
> > Set the dummy buswidth to match the data buswidth.
> >
> > v2:
> > * Add fixes tag and cc to stable.
> >
> > Cheng Ming Lin (1):
> >   mtd: spi-nor: core: replace dummy buswidth from addr to data
> >
> > [...]
>
> You had some warnings which I fixed while applying:

I apologize for overlooking that, and thank you very much for your help.

> WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: 0e30f47232ab ("mtd: spi-nor: add support for DTR protocol")'
> #19:
> Fixes: 0e30f47232ab5 ("mtd: spi-nor: add support for DTR protocol")
>
> WARNING: Non-standard signature: 'Reviewd-by:' - perhaps 'Reviewed-by:'?
> #21:
> Reviewd-by: Pratyush Yadav <pratyush@kernel.org>
>
> Applied to git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git,
> spi-nor/next branch. Thanks!
>
> [1/1] mtd: spi-nor: core: replace dummy buswidth from addr to data
>       https://git.kernel.org/mtd/c/98d1fb94ce75
>
> Cheers,
> --
> Tudor Ambarus <tudor.ambarus@linaro.org>

Thanks,
Cheng Ming Lin

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Cheng Ming Lin]

Hi Tudor,

Tudor Ambarus <tudor.ambarus@linaro.org> 於 2024年11月12日 週二 下午4:36寫道：
>
> On Tue, 12 Nov 2024 15:52:41 +0800, Cheng Ming Lin wrote:
> > Set the dummy buswidth to match the data buswidth.
> >
> > v2:
> > * Add fixes tag and cc to stable.
> >
> > Cheng Ming Lin (1):
> >   mtd: spi-nor: core: replace dummy buswidth from addr to data
> >
> > [...]
>
> You had some warnings which I fixed while applying:

I apologize for overlooking that, and thank you very much for your help.

> WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: 0e30f47232ab ("mtd: spi-nor: add support for DTR protocol")'
> #19:
> Fixes: 0e30f47232ab5 ("mtd: spi-nor: add support for DTR protocol")
>
> WARNING: Non-standard signature: 'Reviewd-by:' - perhaps 'Reviewed-by:'?
> #21:
> Reviewd-by: Pratyush Yadav <pratyush@kernel.org>
>
> Applied to git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git,
> spi-nor/next branch. Thanks!
>
> [1/1] mtd: spi-nor: core: replace dummy buswidth from addr to data
>       https://git.kernel.org/mtd/c/98d1fb94ce75
>
> Cheers,
> --
> Tudor Ambarus <tudor.ambarus@linaro.org>

Thanks,
Cheng Ming Lin