[PATCH] mtd: m25p80: add support for Spansion s25fl128s chip

[PATCH] mtd: m25p80: add support for Spansion s25fl128s chip

[Huang Shijie]

This chip supports the quad read.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/devices/m25p80.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 7dc2c14..720899b 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -941,6 +941,7 @@ static const struct spi_device_id m25p_ids[] = {
 	 */
 	{ "s25sl032p",  INFO(0x010215, 0x4d00,  64 * 1024,  64, 0) },
 	{ "s25sl064p",  INFO(0x010216, 0x4d00,  64 * 1024, 128, 0) },
+	{ "s25fl128s",	INFO(0x012018, 0x4d01,  64 * 1024, 256, M25P80_QUAD_READ) },
 	{ "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, 0) },
 	{ "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512, M25P80_QUAD_READ) },
 	{ "s25fl512s",  INFO(0x010220, 0x4d00, 256 * 1024, 256, 0) },
-- 
1.7.2.rc3

Re: [PATCH] mtd: m25p80: add support for Spansion s25fl128s chip

[Marek Vasut]

Dear Huang Shijie,

> This chip supports the quad read.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  drivers/mtd/devices/m25p80.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 7dc2c14..720899b 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -941,6 +941,7 @@ static const struct spi_device_id m25p_ids[] = {
>  	 */
>  	{ "s25sl032p",  INFO(0x010215, 0x4d00,  64 * 1024,  64, 0) },
>  	{ "s25sl064p",  INFO(0x010216, 0x4d00,  64 * 1024, 128, 0) },
> +	{ "s25fl128s",	INFO(0x012018, 0x4d01,  64 * 1024, 256, 
M25P80_QUAD_READ)

The entry looks correct.

Acked-by: Marek Vasut <marex@denx.de>

Thanks!

Best regards,
Marek Vasut

Re: [PATCH] mtd: m25p80: add support for Spansion s25fl128s chip

[Angus Clark]

Hi Huang Shijie,

On 11/20/2013 08:52 AM, Huang Shijie wrote:
> This chip supports the quad read.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  drivers/mtd/devices/m25p80.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 7dc2c14..720899b 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -941,6 +941,7 @@ static const struct spi_device_id m25p_ids[] = {
>  	 */
>  	{ "s25sl032p",  INFO(0x010215, 0x4d00,  64 * 1024,  64, 0) },
>  	{ "s25sl064p",  INFO(0x010216, 0x4d00,  64 * 1024, 128, 0) },
> +	{ "s25fl128s",	INFO(0x012018, 0x4d01,  64 * 1024, 256, M25P80_QUAD_READ) },
>  	{ "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, 0) },
>  	{ "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512, M25P80_QUAD_READ) },
>  	{ "s25fl512s",  INFO(0x010220, 0x4d00, 256 * 1024, 256, 0) },

I would suggest using the name "s25fl128s1" to indicate the 64KiB sector variant
[1].  However, I would also point out that there is already an entry in the
table that matches the jedec_id/ext_id:

	{ "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, 0) },

As far as I can tell, the m25p80.c driver is not sensitive to the differences
between the 'P' and the 'S' generations; both support M25P80_QUAD_READ, so the
flag could be added to the s25fl129p1 entry if required.

If it was deemed necessary to differentiate between the 'P' and 'S', then the
jedec_probe() code would need to be updated to consider the 6th READID byte
(0x80 for 'S').

Cheers,

Angus

[1] The name should really be "s25fl128s0" where the appended '0' represents the
"model number" 0 for uniform 64KiB sectors.  However, all the other Spansion
entries get this the wrong way round so perhaps it's best to stick with the
existing scheme.

Re: [PATCH] mtd: m25p80: add support for Spansion s25fl128s chip

[Huang Shijie]

于 2013年11月20日 18:16, Angus Clark 写道:
> Hi Huang Shijie,
>
> On 11/20/2013 08:52 AM, Huang Shijie wrote:
>> This chip supports the quad read.
>>
>> Signed-off-by: Huang Shijie<b32955@freescale.com>
>> ---
>>   drivers/mtd/devices/m25p80.c |    1 +
>>   1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>> index 7dc2c14..720899b 100644
>> --- a/drivers/mtd/devices/m25p80.c
>> +++ b/drivers/mtd/devices/m25p80.c
>> @@ -941,6 +941,7 @@ static const struct spi_device_id m25p_ids[] = {
>>   	 */
>>   	{ "s25sl032p",  INFO(0x010215, 0x4d00,  64 * 1024,  64, 0) },
>>   	{ "s25sl064p",  INFO(0x010216, 0x4d00,  64 * 1024, 128, 0) },
>> +	{ "s25fl128s",	INFO(0x012018, 0x4d01,  64 * 1024, 256, M25P80_QUAD_READ) },
>>   	{ "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, 0) },
>>   	{ "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512, M25P80_QUAD_READ) },
>>   	{ "s25fl512s",  INFO(0x010220, 0x4d00, 256 * 1024, 256, 0) },
> I would suggest using the name "s25fl128s1" to indicate the 64KiB sector variant
> [1].  However, I would also point out that there is already an entry in the
> table that matches the jedec_id/ext_id:
>
> 	{ "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, 0) },
yes.
> As far as I can tell, the m25p80.c driver is not sensitive to the differences
> between the 'P' and the 'S' generations; both support M25P80_QUAD_READ, so the
> flag could be added to the s25fl129p1 entry if required.
>
Does the s25fl129p1 support the DDR quad read?
I do not have the datasheet.

it's a little strange if we add the flag to the s25fl129p1.
The one who wants to enable the s25fl128s on its board, will add the the 
name s25fl129p1 in the DTS.



> If it was deemed necessary to differentiate between the 'P' and 'S', then the
> jedec_probe() code would need to be updated to consider the 6th READID byte
> (0x80 for 'S').
>
yes. I also think we should probe the 6th READID byte.
> Cheers,
>
> Angus
>
> [1] The name should really be "s25fl128s0" where the appended '0' represents the
> "model number" 0 for uniform 64KiB sectors.  However, all the other Spansion
yes. the s25fl128s0 is beter.

thanks
Huang Shijie

Re: [PATCH] mtd: m25p80: add support for Spansion s25fl128s chip

[Angus Clark]

On 11/21/2013 08:18 AM, Huang Shijie wrote:
> 于 2013年11月20日 18:16, Angus Clark 写道:
>> Hi Huang Shijie,
>>
>> On 11/20/2013 08:52 AM, Huang Shijie wrote:
>>> This chip supports the quad read.
>>>
>>> Signed-off-by: Huang Shijie<b32955@freescale.com>
>>> ---
>>>   drivers/mtd/devices/m25p80.c |    1 +
>>>   1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>>> index 7dc2c14..720899b 100644
>>> --- a/drivers/mtd/devices/m25p80.c
>>> +++ b/drivers/mtd/devices/m25p80.c
>>> @@ -941,6 +941,7 @@ static const struct spi_device_id m25p_ids[] = {
>>>        */
>>>       { "s25sl032p",  INFO(0x010215, 0x4d00,  64 * 1024,  64, 0) },
>>>       { "s25sl064p",  INFO(0x010216, 0x4d00,  64 * 1024, 128, 0) },
>>> +    { "s25fl128s",    INFO(0x012018, 0x4d01,  64 * 1024, 256,
>>> M25P80_QUAD_READ) },
>>>       { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, 0) },
>>>       { "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512,
>>> M25P80_QUAD_READ) },
>>>       { "s25fl512s",  INFO(0x010220, 0x4d00, 256 * 1024, 256, 0) },
>> I would suggest using the name "s25fl128s1" to indicate the 64KiB
>> sector variant
>> [1].  However, I would also point out that there is already an entry
>> in the
>> table that matches the jedec_id/ext_id:> 
>>
>>     { "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, 0) },
> yes.
>> As far as I can tell, the m25p80.c driver is not sensitive to the
>> differences
>> between the 'P' and the 'S' generations; both support
>> M25P80_QUAD_READ, so the
>> flag could be added to the s25fl129p1 entry if required.
>>
> Does the s25fl129p1 support the DDR quad read?
> I do not have the datasheet.

No, the s25fl129p1 only supports SDR "Quad Output Read" and "Quad I/O Read".  I
thought the M25P80_QUAD_READ flag was for the SDR operations though, hence why I
said the m25p80 driver was not sensitive to the differences between s25fl129p1
and s25fl128s1.  Is that not the case?

> it's a little strange if we add the flag to the s25fl129p1.
> The one who wants to enable the s25fl128s on its board, will add the the
> name s25fl129p1 in the DTS.

That should work, but the m25p_ids[] table is also used for probing unknown
devices, so we need to be careful when adding a new entry.

>> If it was deemed necessary to differentiate between the 'P' and 'S',
>> then the
>> jedec_probe() code would need to be updated to consider the 6th READID
>> byte
>> (0x80 for 'S').
>>
> yes. I also think we should probe the 6th READID byte.

I think that is by far the best approach...

Cheers,

Angus

>> Cheers,
>>
>> Angus
>>
>> [1] The name should really be "s25fl128s0" where the appended '0'
>> represents the
>> "model number" 0 for uniform 64KiB sectors.  However, all the other
>> Spansion
> yes. the s25fl128s0 is beter.
> 
> thanks
> Huang Shijie
> 
> 
> 
> 

-- 
-------------------------------------
Angus Clark
ST Microelectronics (R&D) Ltd.
1000 Aztec West, Bristol, BS32 4SQ
email: angus.clark@st.com
tel: +44 (0) 1454 462389
st-tina: 065 2389
-------------------------------------

Re: [PATCH] mtd: m25p80: add support for Spansion s25fl128s chip

[Huang Shijie]

于 2013年11月21日 17:17, Angus Clark 写道:
> No, the s25fl129p1 only supports SDR "Quad Output Read" and "Quad I/O Read".  I
> thought the M25P80_QUAD_READ flag was for the SDR operations though, hence why I
> said the m25p80 driver was not sensitive to the differences between s25fl129p1
> and s25fl128s1.  Is that not the case?
the target of adding this item is to support the DDR quad read.
So we should add a new item for this nor.

thanks
Huang Shijie

Re: [PATCH] mtd: m25p80: add support for Spansion s25fl128s chip

[Angus Clark]

On 11/21/2013 09:48 AM, Huang Shijie wrote:
> 于 2013年11月21日 17:17, Angus Clark 写道:
>> No, the s25fl129p1 only supports SDR "Quad Output Read" and "Quad I/O
>> Read".  I
>> thought the M25P80_QUAD_READ flag was for the SDR operations though,
>> hence why I
>> said the m25p80 driver was not sensitive to the differences between
>> s25fl129p1
>> and s25fl128s1.  Is that not the case?
> the target of adding this item is to support the DDR quad read.
> So we should add a new item for this nor.

Assuming you a referring to the same M25P80_QUAD_READ flag as introduced by
"[PATCH] drivers: mtd: m25p80: Add quad read support.", then the flag invokes
OPCODE_QUAD_READ = 0x6b, which corresponds to the Spansion *SDR* "QOR" command.
 The DDR QUAD Read command is 0xed, "DDRQIOR" (there is no DDR equivalent of
"QOR").  So, again, I believe the m25p80 driver is not sensitive to the
differences between s25fl129p1 and s25fl128s1; the M25P80_QUAD_READ flag, as is
stands, applies equally to both.

Cheers,

Angus

-- 
-------------------------------------
Angus Clark
ST Microelectronics (R&D) Ltd.
1000 Aztec West, Bristol, BS32 4SQ
email: angus.clark@st.com
tel: +44 (0) 1454 462389
st-tina: 065 2389
-------------------------------------

Re: [PATCH] mtd: m25p80: add support for Spansion s25fl128s chip

[Brian Norris]

(Leaving Huang's email intact, since it was in HTML format and didn't
make it through the list's filter; Huang, you really need to fix this.)

On Fri, Nov 22, 2013 at 10:13:02AM +0800, Huang Shijie wrote:
> 于 2013年11月21日 19:08, Angus Clark 写道:
> 
>     Assuming you a referring to the same M25P80_QUAD_READ flag as introduced by
>     "[PATCH] drivers: mtd: m25p80: Add quad read support.", then the flag invokes
>     OPCODE_QUAD_READ = 0x6b, which corresponds to the Spansion *SDR* "QOR" command.
>      The DDR QUAD Read command is 0xed, "DDRQIOR" (there is no DDR equivalent of
>     "QOR").  So, again, I believe the m25p80 driver is not sensitive to the
>     differences between s25fl129p1 and s25fl128s1; the M25P80_QUAD_READ flag, as is
>     stands, applies equally to both.
> 
> The current m25p80 is not sensitive, but i am changing the this driver now.
> In the future, it should support the DDR QUAD READ. this patch is just making
> preparation for
> the DDR QUAD read.
> 
> And I will submit a patch to read out the 6th READID to distinguish the two
> different NOR.

So to be clear, this patch can't go in as-is.

BTW, if we update m25p80 to use 6th-byte READID, we need to double-check
the entries that currently use the 5-byte ID string, so they don't break
if we extend them implicitly.

Brian

Re: [PATCH] mtd: m25p80: add support for Spansion s25fl128s chip

[Huang Shijie]

On Wed, Dec 04, 2013 at 03:58:31PM -0800, Brian Norris wrote:
> 
> BTW, if we update m25p80 to use 6th-byte READID, we need to double-check
> the entries that currently use the 5-byte ID string, so they don't break
> if we extend them implicitly.
yes, i think we should read out more bytes of the ID.

Is there any NOR that can only read out 5bytes, and can not be read out 6 bytes?


thanks
Huang Shijie

Re: [PATCH] mtd: m25p80: add support for Spansion s25fl128s chip

[Brian Norris]

On Wed, Dec 4, 2013 at 6:20 PM, Huang Shijie <b32955@freescale.com> wrote:
> On Wed, Dec 04, 2013 at 03:58:31PM -0800, Brian Norris wrote:
>>
>> BTW, if we update m25p80 to use 6th-byte READID, we need to double-check
>> the entries that currently use the 5-byte ID string, so they don't break
>> if we extend them implicitly.
> yes, i think we should read out more bytes of the ID.
>
> Is there any NOR that can only read out 5bytes, and can not be read out 6 bytes?

I doubt it. But I wasn't talking about "can we read 6 bytes?", but
rather "what happens when we compare 6 bytes of ID with the 5 byte IDs
in the table?". We'll have a mixture of 3-byte, 5-byte and 6-byte IDs
in our table, and we need to be careful to match properly.

Brian

Re: [PATCH] mtd: m25p80: add support for Spansion s25fl128s chip

[Angus Clark]

Hi Huang,

On 12/05/2013 02:51 AM, Brian Norris wrote:
> On Wed, Dec 4, 2013 at 6:20 PM, Huang Shijie <b32955@freescale.com> wrote:
>> Is there any NOR that can only read out 5bytes, and can not be read out 6 bytes?
> 
> I doubt it. But I wasn't talking about "can we read 6 bytes?", but
> rather "what happens when we compare 6 bytes of ID with the 5 byte IDs
> in the table?". We'll have a mixture of 3-byte, 5-byte and 6-byte IDs
> in our table, and we need to be careful to match properly.
> 

Just in case it is of interest, I have implemented something similar in my own
out-of-tree driver (relevant code segments below).  My 'flash_info' table
structure is a little different to m25p80, but the approach could be similar,
and it has been tested on numerous devices.

/*
 * SPI Flash Device Table
 */
struct flash_info {
	char		*name;

	/* READID data, as returned by 'FLASH_CMD_RDID' (0x9f). */
	u8		readid[MAX_READID_LEN];
	int		readid_len;

	/* The size listed here is what works with FLASH_CMD_SE, which isn't
	 * necessarily called a "sector" by the vendor.
	 */
	unsigned	sector_size;
	u16		n_sectors;

	/* FLASH device capabilities.  Note, in contrast to the other
	 * capabilities, 'FLASH_CAPS_32BITADDR' is set at probe time, based on
	 * the size of the device found.
	 */
	u32		capabilities;

	/* Maximum operating frequency.  Note, where FAST_READ is supported,
	 * freq_max specifies the FAST_READ frequency, not the READ frequency.
	 */
	u32		max_freq;

	int		(*config)(struct stm_spi_fsm *, struct flash_info *);
};

/* Device with standard 3-byte JEDEC ID */
#define JEDEC_INFO(_name, _jedec_id, _sector_size, _n_sectors,		\
		   _capabilities, _max_freq, _config)			\
	{								\
		.name = (_name),					\
		.readid[0] = ((_jedec_id) >> 16 & 0xff),		\
		.readid[1] = ((_jedec_id) >>  8 & 0xff),		\
		.readid[2] = ((_jedec_id) >>  0 & 0xff),		\
		.readid_len = 3,					\
		.sector_size = (_sector_size),				\
		.n_sectors = (_n_sectors),				\
		.capabilities = (_capabilities),			\
		.max_freq = (_max_freq),				\
		.config = (_config)					\
	}

/* Device with arbitrary-length READID */
#define RDID(...) __VA_ARGS__	/* Dummy macro to protect array argument. */
#define RDID_INFO(_name, _readid, _readid_len, _sector_size,		\
		  _n_sectors, _capabilities, _max_freq, _config)	\
	{								\
		.name = (_name),					\
		.readid = _readid,					\
		.readid_len = _readid_len,				\
		.capabilities = (_capabilities),			\
		.sector_size = (_sector_size),				\
		.n_sectors = (_n_sectors),				\
		.capabilities = (_capabilities),			\
		.max_freq = (_max_freq),				\
		.config = (_config)					\
	}


static struct flash_info __devinitdata flash_types[] = {

#define M25P_CAPS (FLASH_CAPS_READ_WRITE | FLASH_CAPS_READ_FAST)
	JEDEC_INFO("m25p40",  0x202013,  64 * 1024,   8, M25P_CAPS, 25, NULL),
	JEDEC_INFO("m25p80",  0x202014,  64 * 1024,  16, M25P_CAPS, 25, NULL),
	JEDEC_INFO("m25p16",  0x202015,  64 * 1024,  32, M25P_CAPS, 25, NULL),
	JEDEC_INFO("m25p32",  0x202016,  64 * 1024,  64, M25P_CAPS, 50, NULL),
	JEDEC_INFO("m25p64",  0x202017,  64 * 1024, 128, M25P_CAPS, 50, NULL),
	JEDEC_INFO("m25p128", 0x202018, 256 * 1024,  64, M25P_CAPS, 50, NULL),

#define M25PX_CAPS (FLASH_CAPS_READ_WRITE	| \
		    FLASH_CAPS_READ_FAST	| \
		    FLASH_CAPS_READ_1_1_2	| \
		    FLASH_CAPS_WRITE_1_1_2)
	JEDEC_INFO("m25px32", 0x207116,  64 * 1024,  64, M25PX_CAPS, 75, NULL),
	JEDEC_INFO("m25px64", 0x207117,  64 * 1024, 128, M25PX_CAPS, 75, NULL),

	/* Spansion S25FLxxxP
	 *     - 256KiB and 64KiB sector variants (identified by ext. JEDEC)
	 *     - S25FL128Px devices do not support DUAL or QUAD I/O
	 */
#define S25FLXXXP_CAPS (FLASH_CAPS_READ_WRITE	| \
			FLASH_CAPS_READ_1_1_2	| \
			FLASH_CAPS_READ_1_2_2	| \
			FLASH_CAPS_READ_1_1_4	| \
			FLASH_CAPS_READ_1_4_4	| \
			FLASH_CAPS_WRITE_1_1_4	| \
			FLASH_CAPS_READ_FAST)
	RDID_INFO("s25fl032p", RDID({0x01, 0x02, 0x15, 0x4d, 0x00}), 5,
		  64 * 1024,  64, S25FLXXXP_CAPS, 80, s25fl_config),
	RDID_INFO("s25fl128p1", RDID({0x01, 0x20, 0x18, 0x03, 0x00}), 5,
		  256 * 1024, 64,
		 (FLASH_CAPS_READ_WRITE | FLASH_CAPS_READ_FAST), 104, NULL),
	RDID_INFO("s25fl128p0", RDID({0x01, 0x20, 0x18, 0x03, 0x01}), 5,
		  64 * 1024, 256,
		  (FLASH_CAPS_READ_WRITE | FLASH_CAPS_READ_FAST), 104, NULL),
	RDID_INFO("s25fl129p0", RDID({0x01, 0x20, 0x18, 0x4d, 0x00}), 5,
		  256 * 1024,  64, S25FLXXXP_CAPS, 80, sstatic int
cmp_flash_info_readid_len(const void *a, const void *b)
{
	return ((struct flash_info *)b)->readid_len -
		((struct flash_info *)a)->readid_len;
}
25fl_config),
	RDID_INFO("s25fl129p1", RDID({0x01, 0x20, 0x18, 0x4d, 0x01}), 5,
		  64 * 1024, 256, S25FLXXXP_CAPS, 80, s25fl_config),

	/* Spansion S25FLxxxS
	 *     - 256KiB and 64KiB sector variants (identified by ext. JEDEC)
	 *     - RESET# signal supported by die but not bristled out on all
	 *       package types.  The package type is a function of board design,
	 *       so this information is captured in the board's capabilities.
	 *     - Supports 'DYB' sector protection. Depending on variant, sectors
	 *       may default to locked state on power-on.
	 *     - S25FL127Sx handled as S25FL128Sx
	 */
#define S25FLXXXS_CAPS (S25FLXXXP_CAPS		| \
			FLASH_CAPS_RESET	| \
			FLASH_CAPS_DYB_LOCKING)
	RDID_INFO("s25fl128s0", RDID({0x01, 0x20, 0x18, 0x4d, 0x00, 0x80}), 6,
		  256 * 1024, 64, S25FLXXXS_CAPS, 80, s25fl_config),
	RDID_INFO("s25fl128s1", RDID({0x01, 0x20, 0x18, 0x4d, 0x01, 0x80}), 6,
		  64 * 1024, 256, S25FLXXXS_CAPS, 80, s25fl_config),
	RDID_INFO("s25fl256s0", RDID({0x01, 0x02, 0x19, 0x4d, 0x00, 0x80}), 6,
		  256 * 1024, 128, S25FLXXXS_CAPS, 80, s25fl_config),
	RDID_INFO("s25fl256s1", RDID({0x01, 0x02, 0x19, 0x4d, 0x01, 0x80}), 6,
		  64 * 1024, 512, S25FLXXXS_CAPS, 80, s25fl_config),

	{ },
};

static int cmp_flash_info_readid_len(const void *a, const void *b)
{
	return ((struct flash_info *)b)->readid_len -
		((struct flash_info *)a)->readid_len;
}

static struct flash_info *__devinit fsm_jedec_probe(struct stm_spi_fsm *fsm)
{
	uint8_t	readid[MAX_READID_LEN];
	char readid_str[MAX_READID_LEN * 3 + 1];
	struct flash_info *info;

	if (fsm_read_jedec(fsm, readid) != 0) {
		dev_info(fsm->dev, "error reading JEDEC ID\n");
		return NULL;
	}

	hex_dump_to_buffer(readid, MAX_READID_LEN, 16, 1,
			   readid_str, sizeof(readid_str), 0);

	dev_dbg(fsm->dev, "READID = %s\n", readid_str);

	/* The 'readid' may match multiple entries in the table.  To ensure we
	 * retrieve the most specific match, the table is sorted in order of
	 * 'readid_len'.
	 */
	sort(flash_types, ARRAY_SIZE(flash_types) - 1,
	     sizeof(struct flash_info), cmp_flash_info_readid_len, NULL);

	for (info = flash_types; info->name; info++) {
		if (memcmp(info->readid, readid, info->readid_len) == 0)
			return info;
	}

	dev_err(fsm->dev, "Unrecognized READID [%s]\n", readid_str);

	return NULL;
}

Cheers,

Angus

Re: [PATCH] mtd: m25p80: add support for Spansion s25fl128s chip

[Huang Shijie]

On Thu, Dec 05, 2013 at 10:55:34AM +0000, Angus Clark wrote:
> static struct flash_info *__devinit fsm_jedec_probe(struct stm_spi_fsm *fsm)
> {
> 	uint8_t	readid[MAX_READID_LEN];

How long is the MAX_READID_LEN? 5 or 8?

Brian doubt that some NOR can not be read out more then 5 bytes.

> 	char readid_str[MAX_READID_LEN * 3 + 1];
> 	struct flash_info *info;
> 
> 	if (fsm_read_jedec(fsm, readid) != 0) {
> 		dev_info(fsm->dev, "error reading JEDEC ID\n");
> 		return NULL;
> 	}
> 
> 	hex_dump_to_buffer(readid, MAX_READID_LEN, 16, 1,
> 			   readid_str, sizeof(readid_str), 0);
> 
> 	dev_dbg(fsm->dev, "READID = %s\n", readid_str);
> 
> 	/* The 'readid' may match multiple entries in the table.  To ensure we
> 	 * retrieve the most specific match, the table is sorted in order of
> 	 * 'readid_len'.
> 	 */
> 	sort(flash_types, ARRAY_SIZE(flash_types) - 1,
> 	     sizeof(struct flash_info), cmp_flash_info_readid_len, NULL);
> 
If we keep the flash_types table be inserted by a specific order.
We can remove this sort code.

> 	for (info = flash_types; info->name; info++) {
> 		if (memcmp(info->readid, readid, info->readid_len) == 0)
> 			return info;
> 	}
> 

thanks
Huang Shijie

Re: [PATCH] mtd: m25p80: add support for Spansion s25fl128s chip

[Angus Clark]

Hi Huang,

On 12/06/2013 10:02 AM, Huang Shijie wrote:
> On Thu, Dec 05, 2013 at 10:55:34AM +0000, Angus Clark wrote:
>> static struct flash_info *__devinit fsm_jedec_probe(struct stm_spi_fsm *fsm)
>> {
>> 	uint8_t	readid[MAX_READID_LEN];
> 
> How long is the MAX_READID_LEN? 5 or 8?

At the moment, I have it set to 6 (for the Spansion 'S' generation devices).  It
just needs to be set to the maximum READID length given in the table.

> Brian doubt that some NOR can not be read out more then 5 bytes.

I believe it is always safe to over-read the READID data.  I have found that
devices either repeat the READID, or start returning 0x00 bytes.

>> 	sort(flash_types, ARRAY_SIZE(flash_types) - 1,
>> 	     sizeof(struct flash_info), cmp_flash_info_readid_len, NULL);
>>
> If we keep the flash_types table be inserted by a specific order.
> We can remove this sort code.
> 

We could do, but that relies on people adding entries in the correct order, and
the correct order in terms of the search leads to a table that is not so easy to
read -- my first version required an ordered table!  I'm easy either way though,
there are clearly benefits to having an ordered table.

Cheers,

Angus

Re: [PATCH] mtd: m25p80: add support for Spansion s25fl128s chip

[Huang Shijie]

On Mon, Dec 09, 2013 at 08:52:39AM +0000, Angus Clark wrote:
> 
> > Brian doubt that some NOR can not be read out more then 5 bytes.
> 
> I believe it is always safe to over-read the READID data.  I have found that
> devices either repeat the READID, or start returning 0x00 bytes.

I will send a patch set after the spi-nor framework is merged.
This issue is a low priority now.


thanks
Huang Shijie