[PATCH] mtd: spi-nor: Add support for ISSI is25lp128

[PATCH] mtd: spi-nor: Add support for ISSI is25lp128

[Nikita Nazarenko]

Signed-off-by: Nikita Nazarenko <nnazarenko@radiofid.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index ed0c19c..e0bb151 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -739,6 +739,7 @@ static const struct flash_info spi_nor_ids[] = {
 
 	/* ISSI */
 	{ "is25cd512", INFO(0x7f9d20, 0, 32 * 1024,   2, SECT_4K) },
+	{ "is25lp128", INFO(0x9d6018, 0, 32 * 1024,   512, SECT_4K) },
 
 	/* Macronix */
 	{ "mx25l512e",   INFO(0xc22010, 0, 64 * 1024,   1, SECT_4K) },
-- 
2.7.0

Re: [PATCH] mtd: spi-nor: Add support for ISSI is25lp128

[Brian Norris]

+ Gabor, who submitted the other ISSI entry; and some others

On Tue, Feb 09, 2016 at 09:12:43PM +0300, Nikita Nazarenko wrote:
> Signed-off-by: Nikita Nazarenko <nnazarenko@radiofid.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index ed0c19c..e0bb151 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -739,6 +739,7 @@ static const struct flash_info spi_nor_ids[] = {
>  
>  	/* ISSI */

Both of these ISSI entries look wrong.

>  	{ "is25cd512", INFO(0x7f9d20, 0, 32 * 1024,   2, SECT_4K) },

Looking at the datasheet for this part [1], shouldn't the ID be
0x9d7f20 (i.e., the MFR ID is 0x9d, not 0x7f)? That would match the
existing CFI definition for "PMC". And IIUC, PMC got bought by ISSI? Or
some kind of merger, I don't follow these things.

> +	{ "is25lp128", INFO(0x9d6018, 0, 32 * 1024,   512, SECT_4K) },

This datasheet [1] says that for SPI mode (not QPI mode), 4K erase is
done with opcode 0xD7 (i.e., SPINOR_OP_BE_4K_PMC) not 0x20
(SPINOR_OP_BE_4K). So we would need the SECT_4K_PMC, not the SECT_4K
flag.

Also, the datasheet for this device says it supports 64K sectors, and
the 32K sectors require a different erase command (SPINOR_OP_BE_32K; not
currently supported in this driver). Did you test erase to be sure it
worked as expected? Or are one or more datasheets wrong?

Regards,
Brian

>  	/* Macronix */
>  	{ "mx25l512e",   INFO(0xc22010, 0, 64 * 1024,   1, SECT_4K) },

[1] I'm looking at these:

http://www.issi.com/WW/pdf/25LP128.pdf
http://www.issi.com/WW/pdf/25CD512-010-020.pdf

Re: [PATCH] mtd: spi-nor: Add support for ISSI is25lp128

[Gabor Juhos]

Hi Brian,

> + Gabor, who submitted the other ISSI entry; and some others
> 
> On Tue, Feb 09, 2016 at 09:12:43PM +0300, Nikita Nazarenko wrote:
>> Signed-off-by: Nikita Nazarenko <nnazarenko@radiofid.com>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index ed0c19c..e0bb151 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -739,6 +739,7 @@ static const struct flash_info spi_nor_ids[] = {
>>  
>>  	/* ISSI */
> 
> Both of these ISSI entries look wrong.
> 
>>  	{ "is25cd512", INFO(0x7f9d20, 0, 32 * 1024,   2, SECT_4K) },
> 
> Looking at the datasheet for this part [1], shouldn't the ID be
> 0x9d7f20 (i.e., the MFR ID is 0x9d, not 0x7f)? That would match the
> existing CFI definition for "PMC". And IIUC, PMC got bought by ISSI? Or
> some kind of merger, I don't follow these things.

The ID is definitely 0x7d9d20, the m28p80 driver detects the device correctly
with that:

  m25p80 spi0.0: found is25cd512, expected m25p80
  m25p80 spi0.0: is25cd512 (64 Kbytes)
  Creating 4 MTD partitions on "spi0.0":
  0x000000000000-0x00000000c000 : "routerboot"
  0x00000000c000-0x00000000d000 : "hard_config"
  0x00000000d000-0x00000000e000 : "bios"
  0x00000000e000-0x00000000f000 : "soft_config"

The first byte of the manufacturer ID is indeed 0x9d, but the device returns
that as the second byte throught the JEDEC ID READ command. Even though it is a
weird behaviour, it is described on page 11 of the datasheet.

>> +	{ "is25lp128", INFO(0x9d6018, 0, 32 * 1024,   512, SECT_4K) },
> 
> This datasheet [1] says that for SPI mode (not QPI mode), 4K erase is
> done with opcode 0xD7 (i.e., SPINOR_OP_BE_4K_PMC) not 0x20
> (SPINOR_OP_BE_4K). So we would need the SECT_4K_PMC, not the SECT_4K
> flag.

I don't have a board with such flash device, but Table 8.1 "Instruction Set" is
misleading a bit. In my understanding, erasing 4K sectors should work with both
commands. Look at Clause 8.10 "SECTOR ERASE OPERATION (SER, D7h/20h)" on page 33
in the datasheet. Both Figure 8.12 "Sector Erase Sequence" and Figure 8.13
"Sector Erase Sequence (QPI) " says 'D7h/20h'.

> Also, the datasheet for this device says it supports 64K sectors, and
> the 32K sectors require a different erase command (SPINOR_OP_BE_32K; not
> currently supported in this driver). Did you test erase to be sure it
> worked as expected? Or are one or more datasheets wrong?

It seems that you are correct about this.

-Gabor

> 
> Regards,
> Brian
> 
>>  	/* Macronix */
>>  	{ "mx25l512e",   INFO(0xc22010, 0, 64 * 1024,   1, SECT_4K) },
> 
> [1] I'm looking at these:
> 
> http://www.issi.com/WW/pdf/25LP128.pdf
> http://www.issi.com/WW/pdf/25CD512-010-020.pdf
> 

Re: [PATCH] mtd: spi-nor: Add support for ISSI is25lp128

[Brian Norris]

Hi Gabor,

On Tue, Mar 08, 2016 at 06:26:41PM +0100, Gabor Juhos wrote:
> > On Tue, Feb 09, 2016 at 09:12:43PM +0300, Nikita Nazarenko wrote:
> >> Signed-off-by: Nikita Nazarenko <nnazarenko@radiofid.com>
> >> ---
> >>  drivers/mtd/spi-nor/spi-nor.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> >> index ed0c19c..e0bb151 100644
> >> --- a/drivers/mtd/spi-nor/spi-nor.c
> >> +++ b/drivers/mtd/spi-nor/spi-nor.c
> >> @@ -739,6 +739,7 @@ static const struct flash_info spi_nor_ids[] = {
> >>  
> >>  	/* ISSI */
> > 
> > Both of these ISSI entries look wrong.
> > 
> >>  	{ "is25cd512", INFO(0x7f9d20, 0, 32 * 1024,   2, SECT_4K) },
> > 
> > Looking at the datasheet for this part [1], shouldn't the ID be
> > 0x9d7f20 (i.e., the MFR ID is 0x9d, not 0x7f)? That would match the
> > existing CFI definition for "PMC". And IIUC, PMC got bought by ISSI? Or
> > some kind of merger, I don't follow these things.
> 
> The ID is definitely 0x7d9d20, the m28p80 driver detects the device correctly
> with that:
> 
>   m25p80 spi0.0: found is25cd512, expected m25p80
>   m25p80 spi0.0: is25cd512 (64 Kbytes)
>   Creating 4 MTD partitions on "spi0.0":
>   0x000000000000-0x00000000c000 : "routerboot"
>   0x00000000c000-0x00000000d000 : "hard_config"
>   0x00000000d000-0x00000000e000 : "bios"
>   0x00000000e000-0x00000000f000 : "soft_config"
> 
> The first byte of the manufacturer ID is indeed 0x9d, but the device returns
> that as the second byte throught the JEDEC ID READ command. Even though it is a
> weird behaviour, it is described on page 11 of the datasheet.

What a stupid implementation. And they changed it to the sane
implementation for is25lp128 it seems?

> >> +	{ "is25lp128", INFO(0x9d6018, 0, 32 * 1024,   512, SECT_4K) },
> > 
> > This datasheet [1] says that for SPI mode (not QPI mode), 4K erase is
> > done with opcode 0xD7 (i.e., SPINOR_OP_BE_4K_PMC) not 0x20
> > (SPINOR_OP_BE_4K). So we would need the SECT_4K_PMC, not the SECT_4K
> > flag.
> 
> I don't have a board with such flash device, but Table 8.1 "Instruction Set" is
> misleading a bit. In my understanding, erasing 4K sectors should work with both
> commands. Look at Clause 8.10 "SECTOR ERASE OPERATION (SER, D7h/20h)" on page 33
> in the datasheet. Both Figure 8.12 "Sector Erase Sequence" and Figure 8.13
> "Sector Erase Sequence (QPI) " says 'D7h/20h'.

Ah, I could see how the table could be read either way. I expect that
the submitter (Nikita) should be able to confirm this through testing.

> > Also, the datasheet for this device says it supports 64K sectors, and
> > the 32K sectors require a different erase command (SPINOR_OP_BE_32K; not
> > currently supported in this driver). Did you test erase to be sure it
> > worked as expected? Or are one or more datasheets wrong?
> 
> It seems that you are correct about this.

To be clear, it looks like your submission (is25cd512) does use D8h for
32K erase blocks, since it's such a tiny device. But Nikita's device
appears to use 64K erase blocks.

I expect Nikita can test and resubmit a revised entry here.

Regards,
Brian

Re: [PATCH] mtd: spi-nor: Add support for ISSI is25lp128

[Gabor Juhos]

Hi Brian,

> On Tue, Mar 08, 2016 at 06:26:41PM +0100, Gabor Juhos wrote:
>>> On Tue, Feb 09, 2016 at 09:12:43PM +0300, Nikita Nazarenko wrote:
>>>> Signed-off-by: Nikita Nazarenko <nnazarenko@radiofid.com>
>>>> ---
>>>>  drivers/mtd/spi-nor/spi-nor.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>>>> index ed0c19c..e0bb151 100644
>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>>> @@ -739,6 +739,7 @@ static const struct flash_info spi_nor_ids[] = {
>>>>  
>>>>  	/* ISSI */
>>>
>>> Both of these ISSI entries look wrong.
>>>
>>>>  	{ "is25cd512", INFO(0x7f9d20, 0, 32 * 1024,   2, SECT_4K) },
>>>
>>> Looking at the datasheet for this part [1], shouldn't the ID be
>>> 0x9d7f20 (i.e., the MFR ID is 0x9d, not 0x7f)? That would match the
>>> existing CFI definition for "PMC". And IIUC, PMC got bought by ISSI? Or
>>> some kind of merger, I don't follow these things.
>>
>> The ID is definitely 0x7d9d20, the m28p80 driver detects the device correctly
>> with that:
>>
>>   m25p80 spi0.0: found is25cd512, expected m25p80
>>   m25p80 spi0.0: is25cd512 (64 Kbytes)
>>   Creating 4 MTD partitions on "spi0.0":
>>   0x000000000000-0x00000000c000 : "routerboot"
>>   0x00000000c000-0x00000000d000 : "hard_config"
>>   0x00000000d000-0x00000000e000 : "bios"
>>   0x00000000e000-0x00000000f000 : "soft_config"
>>
>> The first byte of the manufacturer ID is indeed 0x9d, but the device returns
>> that as the second byte throught the JEDEC ID READ command. Even though it is a
>> weird behaviour, it is described on page 11 of the datasheet.
> 
> What a stupid implementation. 

Maybe it is not so stupid if they wanted to integrate the continuation id in
that. And because 0x7f if not a valid manufacturer ID, it should not collide
with any other manufacturer.

> And they changed it to the sane implementation for is25lp128 it seems?

Yes it seems that they have changed that, but i can't decide that it is better
or not.

>>>> +	{ "is25lp128", INFO(0x9d6018, 0, 32 * 1024,   512, SECT_4K) },
>>>
>>> This datasheet [1] says that for SPI mode (not QPI mode), 4K erase is
>>> done with opcode 0xD7 (i.e., SPINOR_OP_BE_4K_PMC) not 0x20
>>> (SPINOR_OP_BE_4K). So we would need the SECT_4K_PMC, not the SECT_4K
>>> flag.
>>
>> I don't have a board with such flash device, but Table 8.1 "Instruction Set" is
>> misleading a bit. In my understanding, erasing 4K sectors should work with both
>> commands. Look at Clause 8.10 "SECTOR ERASE OPERATION (SER, D7h/20h)" on page 33
>> in the datasheet. Both Figure 8.12 "Sector Erase Sequence" and Figure 8.13
>> "Sector Erase Sequence (QPI) " says 'D7h/20h'.
> 
> Ah, I could see how the table could be read either way. I expect that
> the submitter (Nikita) should be able to confirm this through testing.
> 
>>> Also, the datasheet for this device says it supports 64K sectors, and
>>> the 32K sectors require a different erase command (SPINOR_OP_BE_32K; not
>>> currently supported in this driver). Did you test erase to be sure it
>>> worked as expected? Or are one or more datasheets wrong?
>>
>> It seems that you are correct about this.
> 
> To be clear, it looks like your submission (is25cd512) does use D8h for
> 32K erase blocks, since it's such a tiny device. 

Yes.

> But Nikita's device appears to use 64K erase blocks.

Yes, 64K blocks with the D8h opcode.

-Gabor

> I expect Nikita can test and resubmit a revised entry here.
> 
> Regards,
> Brian
> 

Re: [PATCH] mtd: spi-nor: Add support for ISSI is25lp128

[angelo]

Hi Nikita and Gabor,

Tested-by: Angelo Dureghello <angelo@sysam.it>


On 09/02/2016 19:12, Nikita Nazarenko wrote:
> Hi Gabor,
>
> On Tue, Mar 08, 2016 at 06:26:41PM +0100, Gabor Juhos wrote:
>>> On Tue, Feb 09, 2016 at 09:12:43PM +0300, Nikita Nazarenko wrote:
>>>> Signed-off-by: Nikita Nazarenko <nnazarenko at radiofid.com>
>>>> ---
>>>>   drivers/mtd/spi-nor/spi-nor.c | 1 +
>>>>   1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>>>> index ed0c19c..e0bb151 100644
>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>>> @@ -739,6 +739,7 @@ static const struct flash_info spi_nor_ids[] = {
>>>>
>>>>   	/* ISSI */
>>> Both of these ISSI entries look wrong.
>>>
>>>>   	{ "is25cd512", INFO(0x7f9d20, 0, 32 * 1024,   2, SECT_4K) },
>>> Looking at the datasheet for this part [1], shouldn't the ID be
>>> 0x9d7f20 (i.e., the MFR ID is 0x9d, not 0x7f)? That would match the
>>> existing CFI definition for "PMC". And IIUC, PMC got bought by ISSI? Or
>>> some kind of merger, I don't follow these things.
>> The ID is definitely 0x7d9d20, the m28p80 driver detects the device correctly
>> with that:
>>
>>    m25p80 spi0.0: found is25cd512, expected m25p80
>>    m25p80 spi0.0: is25cd512 (64 Kbytes)
>>    Creating 4 MTD partitions on "spi0.0":
>>    0x000000000000-0x00000000c000 : "routerboot"
>>    0x00000000c000-0x00000000d000 : "hard_config"
>>    0x00000000d000-0x00000000e000 : "bios"
>>    0x00000000e000-0x00000000f000 : "soft_config"
>>
>> The first byte of the manufacturer ID is indeed 0x9d, but the device returns
>> that as the second byte throught the JEDEC ID READ command. Even though it is a
>> weird behaviour, it is described on page 11 of the datasheet.
> What a stupid implementation. And they changed it to the sane
> implementation for is25lp128 it seems?
>
>>>> +	{ "is25lp128", INFO(0x9d6018, 0, 32 * 1024,   512, SECT_4K) },
>>> This datasheet [1] says that for SPI mode (not QPI mode), 4K erase is
>>> done with opcode 0xD7 (i.e., SPINOR_OP_BE_4K_PMC) not 0x20
>>> (SPINOR_OP_BE_4K). So we would need the SECT_4K_PMC, not the SECT_4K
>>> flag.
>> I don't have a board with such flash device, but Table 8.1 "Instruction Set" is
>> misleading a bit. In my understanding, erasing 4K sectors should work with both
>> commands. Look at Clause 8.10 "SECTOR ERASE OPERATION (SER, D7h/20h)" on page 33
>> in the datasheet. Both Figure 8.12 "Sector Erase Sequence" and Figure 8.13
>> "Sector Erase Sequence (QPI) " says 'D7h/20h'.
> Ah, I could see how the table could be read either way. I expect that
> the submitter (Nikita) should be able to confirm this through testing.
>
it happen i mounted the same spi nor (is25lp128) here on a custom Coldfire board running Linux,
so i appliedthe line

{ "is25lp128", INFO(0x9d6018, 0, 32 * 1024, 512, SECT_4K) },

and preformed some testing, the 4k erase works.

[    8.260000] Creating 3 MTD partitions on "is25lp128":
[    8.260000] 0x000000000000-0x000000100000 : "U-Boot (1024K)"
[    8.330000] 0x000000100000-0x000000800000 : "Kernel+initramfs (7168K)"
[    8.400000] 0x000000800000-0x000001000000 : "Flash Free Space (8192K)"


/bin # cat /proc/mtd
dev:    size   erasesize  name
mtd0: 00100000 00001000 "U-Boot (1024K)"
mtd1: 00700000 00001000 "Kernel+initramfs (7168K)"
mtd2: 00800000 00001000 "Flash Free Space (8192K)"

# flash_eraseall /dev/mtd2

/ # hexdump -C -n 8192 /dev/mtd2
00000000  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff |................|
*
00002000
/ #

/ # dd if=/dev/zero of=/dev/mtd2 bs=4096 count=1
1+0 records in
1+0 records out
4096 bytes (4.0KB) copied, 0.228499 seconds, 17.5KB/s


/ # hexdump -C -n 8192 /dev/mtd2
00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
*
00001000  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff |................|
*
00002000
/ #


/ # flash_erase /dev/mtd2 0 1
Erasing 4 Kibyte @ 0 -- 100 % complete
/ #

/ # hexdump -C -n 8192 /dev/mtd2
00000000  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff |................|
*
00002000
/ #



>>> Also, the datasheet for this device says it supports 64K sectors, and
>>> the 32K sectors require a different erase command (SPINOR_OP_BE_32K; not
>>> currently supported in this driver). Did you test erase to be sure it
>>> worked as expected? Or are one or more datasheets wrong?
>> It seems that you are correct about this.
> To be clear, it looks like your submission (is25cd512) does use D8h for
> 32K erase blocks, since it's such a tiny device. But Nikita's device
> appears to use 64K erase blocks.
>
> I expect Nikita can test and resubmit a revised entry here.
>
> Regards,
> Brian
Regards,
Angelo Dureghello

Re: [PATCH] mtd: spi-nor: Add support for ISSI is25lp128

[Angelo Dureghello]

Hi Nikita,

On 18/10/2017 12:33, Nikita Nazarenko wrote:
> Hello Angelo.
>
> I almost forgot about that patch, and in my tree it little different:
>
> + { "is25lp128", INFO(0x9d6018, 0, 64 * 1024,   256, 0) },
>
> I don't remember on which board that flash were used and why i did that change, so i can't test it again properly.
>
> Maybe that will help you.
>
>

many thanks for clarifying,

ok, looks like 64K granularity should be better in any case
for this chip.
Also,  this driver uses SPINOR_OP_SE <https://elixir.free-electrons.com/linux/latest/ident/SPINOR_OP_SE> (0xd8) so the size should be set
to 64k.

The SECT_4K can be used since the flash accepts it.
Clause 8.10 "SECTOR ERASE OPERATION (SER, D7h/20h)", so
both commands are valid.

This is my last tuning:

{ "is25lp128", INFO(0x9d6018, 0, 64 * 1024,   256, SECT_4K) },


Did some more tests on this entry above:

TEST 1)

With  MTD_SPI_NOR_USE_4K_SECTORS set:

/ # flash_eraseall /dev/mtd2
flash_eraseall has been replaced by `flash_erase <mtddev> 0 0`; please use it
Erasing 4 Kibyte @ 7ff000 -- 100 % complete

Checked all the flash has been properly erased:


TEST 2)

Removed MTD_SPI_NOR_USE_4K_SECTORS set:

I written 2 blocks full of 0, at begin and at the end of the
8KB partition:

/ # hexdump -C -n 8192 /dev/mtd2
00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
*
00001000  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff |................|
*
00002000

/ # hexdump -C -n 8192 -s 8380416 /dev/mtd2
007fe000  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff |................|
*
007ff000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
*
00800000
/ #

Doing now flash erase at 64K blocks:

/ # flash_eraseall /dev/mtd2
flash_eraseall has been replaced by `flash_erase <mtddev> 0 0`; please use it
Erasing 64 Kibyte @ 7f0000 -- 100 % complete

All erased properly.

/ # hexdump -C -n 8192 /dev/mtd2
00000000  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff |................|
*
00002000
/ # hexdump -C -n 8192 -s 8380416 /dev/mtd2
007fe000  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff |................|
*
00800000
/ #

So the new line above is well working for me. Let me know how we
could proceed, i am very interested to have this chip supported into
mainline, mainly for my open hw called stmark2.
Do you want to send the patch ? Or, if it's not a problem for you, i can
send it, or maybe i can be able to keep your name and adding a tested-by
from me.

Many thanks,

Best regards,
Angelo Dureghello


> Best regards
>
> Nikita Nazarenko
>
>
> On 18.10.2017 02:14, angelo wrote:
>> Hi Nikita and Gabor,
>>
>> Tested-by: Angelo Dureghello <angelo@sysam.it>
>>
>>
>> On 09/02/2016 19:12, Nikita Nazarenko wrote:
>>> Hi Gabor,
>>>
>>> On Tue, Mar 08, 2016 at 06:26:41PM +0100, Gabor Juhos wrote:
>>>>> On Tue, Feb 09, 2016 at 09:12:43PM +0300, Nikita Nazarenko wrote:
>>>>>> Signed-off-by: Nikita Nazarenko <nnazarenko at radiofid.com>
>>>>>> ---
>>>>>>   drivers/mtd/spi-nor/spi-nor.c | 1 +
>>>>>>   1 file changed, 1 insertion(+)
>>>>>>
>>>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>>>>>> index ed0c19c..e0bb151 100644
>>>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>>>>> @@ -739,6 +739,7 @@ static const struct flash_info spi_nor_ids[] = {
>>>>>>
>>>>>>       /* ISSI */
>>>>> Both of these ISSI entries look wrong.
>>>>>
>>>>>>       { "is25cd512", INFO(0x7f9d20, 0, 32 * 1024,   2, SECT_4K) },
>>>>> Looking at the datasheet for this part [1], shouldn't the ID be
>>>>> 0x9d7f20 (i.e., the MFR ID is 0x9d, not 0x7f)? That would match the
>>>>> existing CFI definition for "PMC". And IIUC, PMC got bought by ISSI? Or
>>>>> some kind of merger, I don't follow these things.
>>>> The ID is definitely 0x7d9d20, the m28p80 driver detects the device correctly
>>>> with that:
>>>>
>>>>    m25p80 spi0.0: found is25cd512, expected m25p80
>>>>    m25p80 spi0.0: is25cd512 (64 Kbytes)
>>>>    Creating 4 MTD partitions on "spi0.0":
>>>>    0x000000000000-0x00000000c000 : "routerboot"
>>>>    0x00000000c000-0x00000000d000 : "hard_config"
>>>>    0x00000000d000-0x00000000e000 : "bios"
>>>>    0x00000000e000-0x00000000f000 : "soft_config"
>>>>
>>>> The first byte of the manufacturer ID is indeed 0x9d, but the device returns
>>>> that as the second byte throught the JEDEC ID READ command. Even though it is a
>>>> weird behaviour, it is described on page 11 of the datasheet.
>>> What a stupid implementation. And they changed it to the sane
>>> implementation for is25lp128 it seems?
>>>
>>>>>> +    { "is25lp128", INFO(0x9d6018, 0, 32 * 1024,   512, SECT_4K) },
>>>>> This datasheet [1] says that for SPI mode (not QPI mode), 4K erase is
>>>>> done with opcode 0xD7 (i.e., SPINOR_OP_BE_4K_PMC) not 0x20
>>>>> (SPINOR_OP_BE_4K). So we would need the SECT_4K_PMC, not the SECT_4K
>>>>> flag.
>>>> I don't have a board with such flash device, but Table 8.1 "Instruction Set" is
>>>> misleading a bit. In my understanding, erasing 4K sectors should work with both
>>>> commands. Look at Clause 8.10 "SECTOR ERASE OPERATION (SER, D7h/20h)" on page 33
>>>> in the datasheet. Both Figure 8.12 "Sector Erase Sequence" and Figure 8.13
>>>> "Sector Erase Sequence (QPI) " says 'D7h/20h'.
>>> Ah, I could see how the table could be read either way. I expect that
>>> the submitter (Nikita) should be able to confirm this through testing.
>>>
>> it happen i mounted the same spi nor (is25lp128) here on a custom Coldfire board running Linux,
>> so i appliedthe line
>>
>> { "is25lp128", INFO(0x9d6018, 0, 32 * 1024, 512, SECT_4K) },
>>
>> and preformed some testing, the 4k erase works.
>>
>> [    8.260000] Creating 3 MTD partitions on "is25lp128":
>> [    8.260000] 0x000000000000-0x000000100000 : "U-Boot (1024K)"
>> [    8.330000] 0x000000100000-0x000000800000 : "Kernel+initramfs (7168K)"
>> [    8.400000] 0x000000800000-0x000001000000 : "Flash Free Space (8192K)"
>>
>>
>> /bin # cat /proc/mtd
>> dev:    size   erasesize  name
>> mtd0: 00100000 00001000 "U-Boot (1024K)"
>> mtd1: 00700000 00001000 "Kernel+initramfs (7168K)"
>> mtd2: 00800000 00001000 "Flash Free Space (8192K)"
>>
>> # flash_eraseall /dev/mtd2
>>
>> / # hexdump -C -n 8192 /dev/mtd2
>> 00000000  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff |................|
>> *
>> 00002000
>> / #
>>
>> / # dd if=/dev/zero of=/dev/mtd2 bs=4096 count=1
>> 1+0 records in
>> 1+0 records out
>> 4096 bytes (4.0KB) copied, 0.228499 seconds, 17.5KB/s
>>
>>
>> / # hexdump -C -n 8192 /dev/mtd2
>> 00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
>> *
>> 00001000  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff |................|
>> *
>> 00002000
>> / #
>>
>>
>> / # flash_erase /dev/mtd2 0 1
>> Erasing 4 Kibyte @ 0 -- 100 % complete
>> / #
>>
>> / # hexdump -C -n 8192 /dev/mtd2
>> 00000000  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff |................|
>> *
>> 00002000
>> / #
>>
>>
>>
>>>>> Also, the datasheet for this device says it supports 64K sectors, and
>>>>> the 32K sectors require a different erase command (SPINOR_OP_BE_32K; not
>>>>> currently supported in this driver). Did you test erase to be sure it
>>>>> worked as expected? Or are one or more datasheets wrong?
>>>> It seems that you are correct about this.
>>> To be clear, it looks like your submission (is25cd512) does use D8h for
>>> 32K erase blocks, since it's such a tiny device. But Nikita's device
>>> appears to use 64K erase blocks.
>>>
>>> I expect Nikita can test and resubmit a revised entry here.
>>>
>>> Regards,
>>> Brian
>> Regards,
>> Angelo Dureghello
>

Re: [PATCH] mtd: spi-nor: Add support for ISSI is25lp128

[Angelo Dureghello]

Hi Nikita,

many thanks, i try.

On 19/10/2017 15:12, Nikita Nazarenko wrote:
> Hello Angelo
> 
> 
>> So the new line above is well working for me. Let me know how we
>> could proceed, i am very interested to have this chip supported into
>> mainline, mainly for my open hw called stmark2.
>> Do you want to send the patch ? Or, if it's not a problem for you, i can
>> send it, or maybe i can be able to keep your name and adding a tested-by
>> from me.
>>
> You can send that patch from yourself, even without mentioning me. you did most of work.
> 
> Best regards
> Nikita Nazarenko

Best regards
Angelo Durghello

[PATCH] mtd: spi-nor: add support for ISSI is25lp128

[Angelo Dureghello]

Signed-off-by: Angelo Dureghello <angelo@sysam.it>
---
 drivers/mtd/spi-nor/spi-nor.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 19c000722cbc..88cd10a0ee9e 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1005,6 +1005,7 @@ static const struct flash_info spi_nor_ids[] = {
 
 	/* ISSI */
 	{ "is25cd512", INFO(0x7f9d20, 0, 32 * 1024,   2, SECT_4K) },
+	{ "is25lp128", INFO(0x9d6018, 0, 64 * 1024,   256, SECT_4K) },
 
 	/* Macronix */
 	{ "mx25l512e",   INFO(0xc22010, 0, 64 * 1024,   1, SECT_4K) },
-- 
2.14.1