[PATCH v3 2/2] mtd: spi-nor: micron-st: Add support for mt25qu01g

[Tudor Ambarus]

Hi, Fabio,

The entire patch looks very suspicious to me. I'd like some more tests
please, see my reasoning below.

On 26.10.2023 14:23, Fabio Estevam wrote:
> From: Fabio Estevam <festevam at denx.de>
> 
> Add support for the MT25QU01G 128MB Micron Serial NOR Flash Memory
> model.
> 
> Datasheet:
> https://www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/mt25q/die-rev-b/mt25q_qlkt_u_01g_bbb_0.pdf
> 
> Tested on a i.MX8MP based board:
> 
> ~# cat /sys/devices/platform/soc at 0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0.0/spi-nor/jedec_id
> 20bb21104400
> 
> ~# cat /sys/devices/platform/soc at 0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0.0/spi-nor/manufacturer
> st
> 
> ~# cat /sys/devices/platform/soc at 0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0.0/spi-nor/partname
> mt25qu01g
> 
> ~# xxd -p  /sys/devices/platform/soc at 0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0.0/spi-nor/sfdp
> 53464450060101ff00060110300000ff84000102800000ffffffffffffff
> ffffffffffffffffffffffffffffffffffffe520fbffffffff3f29eb276b
> 273b27bbffffffffffff27bbffff29eb0c2010d80f520000244a99008b8e
> 03e1ac0127387a757a75fbbdd55c4a0f82ff81bd3d36ffffffffffffffff
> ffffffffffffffffffe7ffff21dcffff
> 
> ~# md5sum  /sys/devices/platform/soc at 0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0.0/spi-nor/sfdp
> 9d28d1b11de8b15ba9152644219d9a78 /sys/devices/platform/soc at 0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0.0/spi-nor/sfdp
> 
> ~# dd if=/dev/urandom of=spi_test bs=1M count=128
> 128+0 records in
> 128+0 records out
> 134217728 bytes (134 MB, 128 MiB) copied, 4.77424 s, 28.1 MB/s
> 
> ~# mtd_debug write /dev/mtd0 0 134217728 spi_test
> Copied 134217728 bytes from spi_test to address 0x00000000 in flash
> 
> ~# mtd_debug read /dev/mtd0 0 134217728 spi_read
> Copied 134217728 bytes from address 0x00000000 in flash to spi_read
> 
> ~# sha1sum spi_test spi_read
> 6778615a7b7db2d3c3fa122721d940082eb67039  spi_test
> 6778615a7b7db2d3c3fa122721d940082eb67039  spi_read
> 
> ~# flash_erase /dev/mtd0 0 0
> Erasing 131072 Kibyte @ 0 -- 100 % complete 
> 
> ~# hexdump -C /dev/mtd0
> 00000000  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
> *
> 08000000

This doesn't prove the flash works because if the entire flash is
already erased and all the ops are noops, the test passes.

Would you please do the following tests instead?

1/ dd if=/dev/urandom of=spi_test bs=1M count=128
2/ mtd_debug write /dev/mtd0 0 134217728 spi_test
3/ just make sure that we wrote something and the flash does not
contains just 0xff data. Read 246 bytes from both dies and dump them.
mtd_debug read /dev/mtd0 0 256 first-die
cat first-die
mtd_debug read /dev/mtd0 67108864 256 second-die
cat second-die
4/ now that we are sure we have some data in it and not just 0xff, erase
the entire flash and make sure we get just 0xff when reading
flash_erase /dev/mtd0 0 0
hexdump -C /dev/mtd0
6/ now do the write-readback-verify test
mtd_debug write /dev/mtd0 0 134217728 spi_test
mtd_debug read /dev/mtd0 0 134217728 spi_read
sha1sum spi_test spi_read

> 
> Signed-off-by: Fabio Estevam <festevam at denx.de>
> ---
> Changes since v2:
> - Added fixups to report the correct n_dice information.
> - Removed .flags = NO_CHIP_ERASE (Michael)
> - Removed .mfr_flags = USE_FSR (Michael)

Not a good idea to remove USE_FSR. FSR is used to determine page program
and erase errors, please keep it.
> 
>  drivers/mtd/spi-nor/micron-st.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
> index 8920547c12bf..31839d6bc3ac 100644
> --- a/drivers/mtd/spi-nor/micron-st.c
> +++ b/drivers/mtd/spi-nor/micron-st.c
> @@ -192,6 +192,20 @@ static struct spi_nor_fixups mt25qu512a_fixups = {
>  	.post_bfpt = mt25qu512a_post_bfpt_fixup,
>  };
>  
> +static int mt25qu01g_post_sfdp_fixup(struct spi_nor *nor)
> +{
> +	struct spi_nor_flash_parameter *params = nor->params;
> +
> +	/* MT25QU01G does not define the SCCR entry, so pass n_dice manually. */
> +	params->n_dice = 2;
> +

I wonder why this is needed if it's not used anywhere. Do you set this
just so that based on it to set the no-chip-erase flag? If yes, set the
no chip erase flag directly and drop the entire fixup hook. Would be
good to introduce the die erase capability if you care about erase speed.

Skimming over the datasheet you pointed out didn't help me remove my
concerns.
See "Table 24: 4-BYTE READ MEMORY Operations", it says "a die can be
read with a single command", but we don't consider this anywhere in the
code.

If the flash works with the current code, why does it work?
Please enable dev_dbg, we have one in spi_nor_read(), we shall see
what's going on.

Cheers,
ta