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

[Pratyush Yadav <pratyush@kernel.org>]

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