Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Christophe KERELLO <christophe.kerello@st.com>]

On 05/28/2014 07:22 AM, Huang Shijie wrote:
> On Tue, May 27, 2014 at 05:57:31PM +0200, Christophe KERELLO wrote:
>> Hello Huang,
>>
>> I have one remark on this patch.
>>
>> S25FL128S flash has 2 variants:
>>   - Uniform 256-kB sectors (ext_id = 0x4D00)
>>   - 4-kB parameter sectors with uniform 64-kB sectors (ext_id = 0x4D01)
>>
>> If i would like to distinguish these 2 variants in spi_nor_ids
>> array, i replace:
>> { "s25fl128s", INFO(0x012018, 0x4d0180, 64 * 1024, 256,
>> SPI_NOR_QUAD_READ) },
>> with
>> { "s25fl128s0", INFO(0x012018, 0x4d0080, 256 * 1024, 64,
>> SPI_NOR_QUAD_READ) },
>> { "s25fl128s1", INFO(0x012018, 0x4d0180, 64 * 1024, 256,
>> SPI_NOR_QUAD_READ) },
>>
>> In this case, i fail to find s25fl128s1 device.
>>
>> The problem is coming from ext_jedec calculation.
>> first try:
>>      - jedec = 0x012018,
>>      - ext_jedec = 0x4d0180,
>>      - info->ext_id = 0x4d0080,
>>      =>s25fl128s0 doesn't match
>>
>> second try:
>>      - jedec = 0x012018,
>>      - ext_jedec = 0x4d018080,
>>      - info->ext_id = 0x4d0180,
>>      => s25fl128s1 doesn't match
>>
>> I think there is similar issue with s25fl129p1 (if i use this patch
>> without modifying it).
>> first try:
>>      - jedec = 0x012018,
>>      - ext_jedec = 0x4d01xx, (xx = probably 0x00 or 0xff)
>>      - info->ext_id = 0x4d0180,
>>      =>s25fl128s doesn't match
>>
>> second try:
>>      - jedec = 0x012018,
>>      - ext_jedec = 0x4d01xx,
>>      - info->ext_id = 0x4d01,
>>      => s25fl129p1 doesn't match
>>
>> If I reset ext_jedec after each try, it works.
>>
>> ext_jedec = ext_jedec << 8 | id[5];
>> if (info->ext_id == ext_jedec)
>>      return &spi_nor_ids[tmp];
>> else
>>      /* reset ext_jdec for next try */
>>      ext_jedec = ext_jedec >> 8;
>>
> hi Christophe:
>    thanks for pointing this!
>
>    Please try the new version.
>
> thanks
> Huang Shijie
Hello Huang,

Thanks.
Patch v2 works fine.

Regards,
Christophe Kerello.
>
> ---------------------------------------
>  From 7b920141899e4e7249bd23792dc8d5f1558cb7ca Mon Sep 17 00:00:00 2001
> From: Huang Shijie <b32955@freescale.com>
> Date: Mon, 14 Apr 2014 18:09:34 +0800
> Subject: [PATCH v2] mtd: spi-nor: read 6 bytes for the ID
>
> Currently, we read 5 bytes for ID, but s25fl128s has the same ext_id(0x4d01)
> with s25fl129p1. The s25fl128s can support the DDR Quad read, while s25fl129p1
> does not. So we have to distinguish the two NOR flashs.
>
> This patch reads out 6 bytes for the ID, and use the 6 bytes ID to search the
> right flash_info.
>
> The detail of the patch is:
>    [1] change the "ext_id" from u16 to u32.
>        We can store two bytes or three bytes with the @ext_id now.
>
>    [2] search the right flash_info with the 6byte ID and the new @ext_id.
>        We use "matched" variable to track the legacy two bytes @ext_id.
>        If the flash_info's @ext_id is three bytes, we will use the
>        sixth byte of the ID to check it.
>
>    [3] add the new item to spi_nor_ids for s25fl128s.
>
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>   drivers/mtd/spi-nor/spi-nor.c |   30 +++++++++++++++++++++++++-----
>   1 files changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index c713c86..f21d3ef 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -383,7 +383,7 @@ struct flash_info {
>   	 * then a two byte device id.
>   	 */
>   	u32		jedec_id;
> -	u16             ext_id;
> +	u32		ext_id;
>   
>   	/* The size listed here is what works with SPINOR_OP_SE, which isn't
>   	 * necessarily called a "sector" by the vendor.
> @@ -505,6 +505,7 @@ const struct spi_device_id spi_nor_ids[] = {
>   	{ "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
>   	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
>   	{ "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
> +	{ "s25fl128s",	INFO(0x012018, 0x4d0180, 64 * 1024, 256, SPI_NOR_QUAD_READ) },
>   	{ "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64, 0) },
>   	{ "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, 0) },
>   	{ "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8, 0) },
> @@ -593,12 +594,13 @@ EXPORT_SYMBOL_GPL(spi_nor_ids);
>   static const struct spi_device_id *spi_nor_read_id(struct spi_nor *nor)
>   {
>   	int			tmp;
> -	u8			id[5];
> +	u8			id[6];
>   	u32			jedec;
> -	u16                     ext_jedec;
> +	u32                     ext_jedec;
>   	struct flash_info	*info;
> +	int			matched = -1;
>   
> -	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, 5);
> +	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, 6);
>   	if (tmp < 0) {
>   		dev_dbg(nor->dev, " error %d reading JEDEC ID\n", tmp);
>   		return ERR_PTR(tmp);
> @@ -614,8 +616,26 @@ static const struct spi_device_id *spi_nor_read_id(struct spi_nor *nor)
>   	for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
>   		info = (void *)spi_nor_ids[tmp].driver_data;
>   		if (info->jedec_id == jedec) {
> -			if (info->ext_id == 0 || info->ext_id == ext_jedec)
> +			if (info->ext_id == 0)
>   				return &spi_nor_ids[tmp];
> +
> +			/* the legacy two bytes ext_id */
> +			if ((info->ext_id >> 16) == 0) {
> +				if (info->ext_id == ext_jedec)
> +					matched = tmp;
> +			} else {
> +				/* check the sixth byte now */
> +				ext_jedec = ext_jedec << 8 | id[5];
> +				if (info->ext_id == ext_jedec)
> +					return &spi_nor_ids[tmp];
> +
> +				/* reset back the ext_jedec */
> +				ext_jedec >>= 8;
> +			}
> +		} else {
> +			/* shortcut */
> +			if (matched != -1)
> +				return &spi_nor_ids[matched];
>   		}
>   	}
>   	dev_err(nor->dev, "unrecognized JEDEC id %06x\n", jedec);