Re: [PATCH 3/7] mtd: spi-nor: macronix: Handle ID collision b/w MX25L3233F and MX25L3205D

From: Pratyush Yadav <p.yadav@ti.com>
To: Tudor Ambarus <tudor.ambarus@microchip.com>
Cc: <michael@walle.cc>, <vigneshr@ti.com>, <figgyc@figgyc.uk>,
	<mail@david-bauer.net>, <linux@rasmusvillemoes.dk>,
	<esben@geanix.com>, <knaerzche@gmail.com>,
	<code@reto-schneider.ch>, <zhengxunli@mxic.com.tw>,
	<jaimeliao@mxic.com.tw>, <heiko.thiery@gmail.com>,
	<macromorgan@hotmail.com>, <sr@denx.de>,
	<miquel.raynal@bootlin.com>, <richard@nod.at>,
	<linux-mtd@lists.infradead.org>
Subject: Re: [PATCH 3/7] mtd: spi-nor: macronix: Handle ID collision b/w MX25L3233F and MX25L3205D
Date: Tue, 6 Jul 2021 11:44:58 +0530	[thread overview]
Message-ID: <20210706061456.74i4qvusa64s6bc3@ti.com> (raw)
In-Reply-To: <20210702144110.250481-4-tudor.ambarus@microchip.com>

On 02/07/21 05:41PM, Tudor Ambarus wrote:
> 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.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/macronix.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> index 27498ed0cc0d..83a097708949 100644
> --- a/drivers/mtd/spi-nor/macronix.c
> +++ b/drivers/mtd/spi-nor/macronix.c
> @@ -8,6 +8,26 @@
>  
>  #include "core.h"
>  
> +static const char *mx25l3233f = "mx25l3233f";
> +
> +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;

Do we need to use the const char pointer here? Won't just doing 
nor->name = "mx2513233f"; do the trick?

> +
> +	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 +59,9 @@ 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 |

Maybe add a comment here listing all the flashes that could collide with 
this ID?

Other than this,

Acked-by: Pratyush Yadav <p.yadav@ti.com>

> +			      SECT_4K)
> +		.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) },
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/