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@xxxxxxxxxxx>
> >>
> >> The default dummy cycle for Macronix SPI NOR flash in Octal Output
> >> Read Mode(1-1-8) is 20.
> >>
> >> Currently, the dummy buswidth is set according to the address bus width.
> >> In the 1-1-8 mode, this means the dummy buswidth is 1. When converting
> >> dummy cycles to bytes, this results in 20 x 1 / 8 = 2 bytes, causing the
> >> host to read data 4 cycles too early.
> >>
> >> Since the protocol data buswidth is always greater than or equal to the
> >> address buswidth. Setting the dummy buswidth to match the data buswidth
> >> increases the likelihood that the dummy cycle-to-byte conversion will be
> >> divisible, preventing the host from reading data prematurely.
> >>
> >> Fixes: 0e30f47232ab5 ("mtd: spi-nor: add support for DTR protocol")
> >> Cc: stable@xxxxxxxxxxxxxxx
> >> Reviewd-by: Pratyush Yadav <pratyush@xxxxxxxxxx>
> >> Signed-off-by: Cheng Ming Lin <chengminglin@xxxxxxxxxxx>
> >> ---
> >> drivers/mtd/spi-nor/core.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> >> index f9c189ed7353..c7aceaa8a43f 100644
> >> --- a/drivers/mtd/spi-nor/core.c
> >> +++ b/drivers/mtd/spi-nor/core.c
> >> @@ -89,7 +89,7 @@ void spi_nor_spimem_setup_op(const struct spi_nor *nor,
> >> op->addr.buswidth = spi_nor_get_protocol_addr_nbits(proto);
> >>
> >> if (op->dummy.nbytes)
> >> - op->dummy.buswidth = spi_nor_get_protocol_addr_nbits(proto);
> >> + op->dummy.buswidth = spi_nor_get_protocol_data_nbits(proto);
> >>
> >> if (op->data.nbytes)
> >> op->data.buswidth = spi_nor_get_protocol_data_nbits(proto);
> >>
> >
> > I just noticed this commit caused a regression on my i.MX8M Plus based board,
> > detected using git bisect.
> > DT: arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql-mba8mpxl.dts
> > Starting with this patch read is only 1S-1S-1S, before it was
> > 1S-1S-4S.
> >
> > before:
> >> cat /sys/kernel/debug/spi-nor/spi0.0/params
> >> name mt25qu512a
> >> id 20 bb 20 10 44 00
> >> size 64.0 MiB
> >> write size 1
> >> page size 256
> >> address nbytes 4
> >> flags HAS_SR_TB | 4B_OPCODES | HAS_4BAIT | HAS_LOCK | HAS_4BIT_BP
> >> | HAS_SR_BP3_BIT6 | SOFT_RESET
> >>
> >> opcodes
> >>
> >> read 0x6c
> >>
> >> dummy cycles 8
> >>
> >> erase 0xdc
> >> program 0x12
> >> 8D extension none
> >>
> >> protocols
> >>
> >> read 1S-1S-4S
> >> write 1S-1S-1S
> >> register 1S-1S-1S
> >>
> >> erase commands
> >>
> >> 21 (4.00 KiB) [1]
> >> dc (64.0 KiB) [3]
> >> c7 (64.0 MiB)
> >>
> >> sector map
> >>
> >> region (in hex) | erase mask | overlaid
> >> ------------------+------------+----------
> >> 00000000-03ffffff | [ 3] | no
> >
> > after:
> >> cat /sys/kernel/debug/spi-nor/spi0.0/params
> >> name mt25qu512a
> >> id 20 bb 20 10 44 00
> >> size 64.0 MiB
> >> write size 1
> >> page size 256
> >> address nbytes 4
> >> flags HAS_SR_TB | 4B_OPCODES | HAS_4BAIT | HAS_LOCK | HAS_4BIT_BP
> >> | HAS_SR_BP3_BIT6 | SOFT_RESET
> >>
> >> opcodes
> >>
> >> read 0x13
> >>
> >> dummy cycles 0
> >>
> >> erase 0xdc
> >> program 0x12
> >> 8D extension none
> >>
> >> protocols
> >>
> >> read 1S-1S-1S
> >> write 1S-1S-1S
> >> register 1S-1S-1S
> >>
> >> erase commands
> >>
> >> 21 (4.00 KiB) [1]
> >> dc (64.0 KiB) [3]
> >> c7 (64.0 MiB)
> >>
> >> sector map
> >>
> >> region (in hex) | erase mask | overlaid
> >> ------------------+------------+----------
> >> 00000000-03ffffff | [ 3] | no
> >
> > AFAICT the patch seems sane, so it probably just uncovered another
> > problem already lurking somewhere deeper.
> > Given the HW similarity I expect imx8mn and imx8mm based platforms to be
> > affected as well.
> > Reverting this commit make the read to be 1S-1S-4S again.
> > Any ideas ow to tackling down this problem?
>
> 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/
