diff mbox series

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

Message ID 20210727045222.905056-4-tudor.ambarus@microchip.com (mailing list archive)
State New, archived
Headers show
Series mtd: spi-nor: Handle ID collisions and clean params init | expand

Commit Message

Tudor Ambarus July 27, 2021, 4:51 a.m. UTC 
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
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(-)

Comments

Michael Walle Aug. 23, 2021, 10:42 p.m. UTC | #1 
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.

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

-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) },
Tudor Ambarus Oct. 1, 2021, 8:41 a.m. UTC | #2 
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) },
diff mbox series

Patch

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 },
 	{ "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) },