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

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

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;

Regards,
Christophe Kerello.

On 05/27/2014 03:12 AM, Huang Shijie wrote:
> On Mon, Apr 14, 2014 at 06:09:34PM +0800, Huang Shijie wrote:
>> 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>
>> ---
>> based on Brian's patch set.
>> ---
>>   drivers/mtd/spi-nor/spi-nor.c |   27 ++++++++++++++++++++++-----
>>   1 files changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index d6f44d5..cd4b277 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,23 @@ 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];
>> +			}
>> +		} else {
>> +			/* shortcut */
>> +			if (matched != -1)
>> +				return &spi_nor_ids[matched];
>>   		}
>>   	}
>>   	dev_err(nor->dev, "unrecognized JEDEC id %06x\n", jedec);
> Hi Brian:
> 	any comment about this patch?
>
> thanks
> Huang Shijie
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/