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