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

Wed Oct 18 13:21:48 PDT 2017

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 at 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
>