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

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

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/