Re: [PATCH v2 03/35] mtd: spi-nor: macronix: Handle ID collision b/w MX25L3233F and MX25L3205D

From: <Tudor.Ambarus@microchip.com>
To: <michael@walle.cc>
Cc: macromorgan@hotmail.com, vigneshr@ti.com, jaimeliao@mxic.com.tw,
	richard@nod.at, esben@geanix.com, linux@rasmusvillemoes.dk,
	knaerzche@gmail.com, linux-mtd@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org, code@reto-schneider.ch,
	miquel.raynal@bootlin.com, heiko.thiery@gmail.com, sr@denx.de,
	figgyc@figgyc.uk, p.yadav@ti.com, mail@david-bauer.net,
	zhengxunli@mxic.com.tw
Subject: Re: [PATCH v2 03/35] mtd: spi-nor: macronix: Handle ID collision b/w MX25L3233F and MX25L3205D
Date: Fri, 1 Oct 2021 08:41:41 +0000	[thread overview]
Message-ID: <970e427a-953d-cd3e-da80-84082a03333a@microchip.com> (raw)
In-Reply-To: <e41e5aa88846c87d041e3c3ea7845202@walle.cc>

On 8/24/21 1:42 AM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2021-07-27 06:51, schrieb Tudor Ambarus:
>> Macronix has a bad habbit of reusing flash IDs. While MX25L3233F
>> supports
>> RDSFDP opcode, MX25L3205D does not support it and does not recommend
>> issuing opcodes that are not supported ("It is not recommended to adopt
>> any other code not in the command definition table, which will
>> potentially
>> enter the hidden mode.").
>>
>> We tested the RDSFDP on the MX25L3205D and the conclusion is that the
>> flash didn't reply anything. Given that it is unlikely that RDSFDP will
>> cause any problems for the old MX25L3205D, differentiate between the
>> two
>> flashes by parsing SFDP.
>>
>> Tested MX25L3233F. Generated a 256 Kbyte random data and did an erase,
>> write, read back and compare test. The flash uses for reads
>> SPINOR_OP_READ_1_4_4 0xeb, for erases SPINOR_OP_BE_4K 0x20, and for
>> writes
>> SPINOR_OP_PP 0x02.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> Acked-by: Pratyush Yadav <p.yadav@ti.com>
>> ---
>> root@sama5d2-xplained:~# find / -iname spi-nor
>> /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor
>> /sys/devices/platform/ahb/ahb:apb/f8000000.spi/spi_master/spi0/spi0.0/spi-nor
>> /sys/bus/spi/drivers/spi-nor
>> root@sama5d2-xplained:~# ls -al
>> /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor
>> total 0
>> drwxr-xr-x 2 root root    0 Mar  9 14:51 .
>> drwxr-xr-x 6 root root    0 Mar  9 14:50 ..
>> -r--r--r-- 1 root root 4096 Mar  9 14:51 jedec_id
>> -r--r--r-- 1 root root 4096 Mar  9 14:51 manufacturer
>> -r--r--r-- 1 root root 4096 Mar  9 14:51 partname
>> -r--r--r-- 1 root root    0 Mar  9 14:51 sfdp
>> root@sama5d2-xplained:~# cat
>> /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/jedec_id
>> c22016
>> root@sama5d2-xplained:~# cat
>> /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/manufacturer
>> macronix
>> root@sama5d2-xplained:~# cat
>> /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/partname
>> mx25l3233f
>> root@sama5d2-xplained:~# cat
>> /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/sfdp
>> > mx25l3233f-sfdp
>> root@sama5d2-xplained:~# hexdump mx25l3233f-sfdp
> 
> use xxd if possible and the sha1sum/md5sum is missing.

ok, will update.

> 
>> 0000000 4653 5044 0100 ff01 0000 0901 0030 ff00
>> 0000010 00c2 0401 0060 ff00 ffff ffff ffff ffff
>> 0000020 ffff ffff ffff ffff ffff ffff ffff ffff
>> 0000030 20e5 fff1 ffff 01ff eb44 6b08 3b08 bb04
>> 0000040 ffee ffff ffff ff00 ffff ff00 200c 520f
>> 0000050 d810 ff00 ffff ffff ffff ffff ffff ffff
>> 0000060 3600 2650 f99c 6477 cffe ffff ffff ffff
>> 0000070
>>
>>  drivers/mtd/spi-nor/macronix.c | 23 ++++++++++++++++++++++-
>>  1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/spi-nor/macronix.c
>> b/drivers/mtd/spi-nor/macronix.c
>> index 27498ed0cc0d..68f6ac060bc6 100644
>> --- a/drivers/mtd/spi-nor/macronix.c
>> +++ b/drivers/mtd/spi-nor/macronix.c
>> @@ -8,6 +8,24 @@
>>
>>  #include "core.h"
>>
>> +static int mx25l3233f_post_bfpt_fixups(struct spi_nor *nor,
>> +                             const struct sfdp_parameter_header *bfpt_header,
>> +                             const struct sfdp_bfpt *bfpt)
>> +{
>> +     /*
>> +      * Macronix has a bad habit of reusing flash IDs: MX25L3233F collides
>> +      * with MX25L3205D. MX25L3233F defines SFDP tables, while the older
>> +      * variant does not.
>> +      */
>> +     nor->name = "mx25l3233f";
>> +
>> +     return 0;
>> +}
>> +
>> +static struct spi_nor_fixups mx25l3233f_fixups = {
>> +     .post_bfpt = mx25l3233f_post_bfpt_fixups,
>> +};
>> +
>>  static int
>>  mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
>>                           const struct sfdp_parameter_header *bfpt_header,
>> @@ -39,7 +57,10 @@ static const struct flash_info macronix_parts[] = {
>>       { "mx25l4005a",  INFO(0xc22013, 0, 64 * 1024,   8, SECT_4K) },
>>       { "mx25l8005",   INFO(0xc22014, 0, 64 * 1024,  16, 0) },
>>       { "mx25l1606e",  INFO(0xc22015, 0, 64 * 1024,  32, SECT_4K) },
>> -     { "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K) },
>> +     { "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, SPI_NOR_PARSE_SFDP
>> |
>> +                           SECT_4K)
>> +             /* ID collision with mx25l3233f. */
>> +             .fixups = &mx25l3233f_fixups },
> 
> Shouldn't we use mx25l3205d_fixups as name here? What if there are more
> flashes with the same id. Using the name of the colliding flash here
> doesn't really scale.

I agree, will change.

Cheers,
ta
> 
> -michael
> 
>>       { "mx25l3255e",  INFO(0xc29e16, 0, 64 * 1024,  64, SECT_4K) },
>>       { "mx25l6405d",  INFO(0xc22017, 0, 64 * 1024, 128, SECT_4K) },
>>       { "mx25u2033e",  INFO(0xc22532, 0, 64 * 1024,   4, SECT_4K) },

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel