[U-Boot] [EXT] Re: [Patch V3] drivers: mtd :spi: Enable 4B opcodes for SPANSION s25fl512s

Fri Jul 19 18:11:18 UTC 2019

>>>>>> s25fs512s and s25fl512s which has same JEDEC ID but only varies in
>>>>>> operating volatge so s25fs512s shares same command set as
>> mentioned
>>>>>> below:
>>>>>> – Serial Command subset and footprint compatible with S25FL-A,
>>>>>> S25FL-K, S25FL-P, and S25FL-S SPI families – Multi I/O Command
>>>>>> subset and footprint compatible with S25FL-P, and S25FL-S SPI
>>>>>> families
>>>>>>
>>>>>> Signed-off-by: Ashish Kumar <Ashish.Kumar at nxp.com>
>>>>>> ---
>>>>>> v3:
>>>>>> 1. Add version info, rebase to top.
>>>>>> 2. Re-word commit message.
>>>>>> v2:
>>>>>> 1. Adding more description in commit msg.
>>>>>> 2. consolidating "" and  "" in single patch.
>>>>>>
>>>>>>  drivers/mtd/spi/spi-nor-ids.c | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/mtd/spi/spi-nor-ids.c
>>>>>> b/drivers/mtd/spi/spi-nor-ids.c index 3217fc6..a992966 100644
>>>>>> --- a/drivers/mtd/spi/spi-nor-ids.c
>>>>>> +++ b/drivers/mtd/spi/spi-nor-ids.c
>>>>>> @@ -181,7 +181,7 @@ const struct flash_info spi_nor_ids[] = {
>>>>>>         { INFO("s25sl064p",  0x010216, 0x4d00,  64 * 1024, 128,
>>>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>>>>>>         { INFO("s25fl256s0", 0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR)
>> },
>>>>>>         { INFO("s25fl256s1", 0x010219, 0x4d01,  64 * 1024, 512,
>>>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>>>>>> -       { INFO6("s25fl512s",  0x010220, 0x4d0081, 256 * 1024, 256,
>>>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>>>>>> +       { INFO6("s25fl512s",  0x010220, 0x4d0081, 256 * 1024, 256,
>>>>>> + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR |
>>>>>> + SPI_NOR_4B_OPCODES) },
>>>>>
>>>>> I didn't find any diff b/w this with respect to v1 patch, seems like
>>>>> Vignesh commented some issue? any update on that.
>>>>
>>>> Hi Jagan, Vignesh,
>>>>
>>>> I had updated commit message.
>>>>
>>>> Wrt Vignesh's comment:
>>>> To me it seems not valid. This Flash can be used in extended 3-byte
>> addressing mode as well as 4-byte addressing mode.
>>>> Adding SPI_NOR_4B_OPCODE, will make use of 4-byte addressing mode
>> with CONFIG_SPI_FLASH_BAR being __not__ set.
>>>> By adding SPI_NOR_4B_OPCODES code flow will via
>> spi_nor_set_4byte_opcodes() in place of set_4byte().
>>>>
>>
>> This does not seem right. Here is the code snippet from spi-nor-
>> core.c::spi_nor_scan():
>>
>> #ifndef CONFIG_SPI_FLASH_BAR
>>                 /* enable 4-byte addressing if the device exceeds 16MiB */
>>                 nor->addr_width = 4;
>>                 if (JEDEC_MFR(info) == SNOR_MFR_SPANSION ||
>>                     info->flags & SPI_NOR_4B_OPCODES)
> Hi Vignesh,
> 
> 1. It seems old, but at least one of the flash(spansion) on my NXP/Freescale board reported manufacturing ID as 02h in place of 01h as result I had to add this flags, I will try to trace that particular board. Reading the flow again it seems this patch is not required due to presence of || operator as long manufacturer ID is reported correctly.
> if (JEDEC_MFR(info) == SNOR_MFR_SPANSION ||
>                      info->flags & SPI_NOR_4B_OPCODES)

So its just a quirky flash with different manf ID? If manuf ID reads
different then how does it match s25fl512s entry in the spi-nor-ids?

> 2. The very first link on google [1] leads to an APP note from spansion suggesting manufacturing id as 02h, is there a possibility of such flashes in circulation, or am I referring something incorrect ? I will double check this with spansion FAE. 
> Snippet from APP note named: "AN98488 - Quick Guide to Common Flash Interface"
>>>>>>
> The Cypress Manufacturer ID for flash devices is 0002h (historically the AMD ID is
> 0002h and the Fujitsu ID is 0004h; the former Spansion ID is 0002h)."
> <<<<<

Cypress seems to use different ID for CFI NOR flash (on Parallel NOR
interface or HyperBus interface). Thats a different standard (JEDEC CFI
ID codes https://www.jedec.org/standards-documents/docs/jep-137b)

SPI NOR flashes from Cypress/Spansion reads 0x1 for Manufacturer ID.
This is based on JEDEC standard
https://www.jedec.org/standards-documents/docs/jep-106ab

> 3. s25fl064l, s25fl128l flash id has SPI_NOR_4B_OPCODES, what is the use of this flag here then ?
> 

That was an overlook when those were added to Linux spi-nor code and
became part of U-Boot during syncing of framework.

I am not saying that adding SPI_NOR_4B_OPCODES is wrong, because
Spansion flashes does support stateless 4 byte addressing opcodes. But I
am saying that it should not be needed in the first place (as you can
see from code snippets). If you ever need to add SPI_NOR_4B_OPCODES to
spansion flashes (with manf ID 0x1) then I think there is a bug
somewhere in the code which needs to be debugged and understood.

Regards
Vignesh

> [1]: https://www.google.com/url?sa=t&rct=j&q=&esrc=s&source=web&cd=1&cad=rja&uact=8&ved=2ahUKEwjHpY3U0b7jAhVEWX0KHeqSAQkQFjAAegQIBBAB&url=http%3A%2F%2Fwww.cypress.com%2Ffile%2F195291%2Fdownload&usg=AOvVaw0zPEXOjP80J8XVrwmB7TBS
> 
> Regards
> Ashish 
>>                         spi_nor_set_4byte_opcodes(nor, info); #else
>>         /* Configure the BAR - discover bank cmds and read current bank */
>>         nor->addr_width = 3;
>>         ret = read_bar(nor, info);
>>         if (ret < 0)
>>                 return ret;
>> #endif
>>
>>
>> So as long as CONFIG_SPI_FLASH_BAR is not defined and flash is Spansion
>> flash
>> spi_nor_set_4byte_opcodes() is always called irrespective of
>> SPI_NOR_4B_OPCODES
>>
>> Also from spi_nor_init():,
>>
>>
>>         if (nor->addr_width == 4 &&
>>             (JEDEC_MFR(nor->info) != SNOR_MFR_SPANSION) &&
>>             !(nor->info->flags & SPI_NOR_4B_OPCODES)) {
>>                 /*
>>                  * If the RESET# pin isn't hooked up properly, or the system
>>                  * otherwise doesn't perform a reset command in the boot
>>                  * sequence, it's impossible to 100% protect against unexpected
>>                  * reboots (e.g., crashes). Warn the user (or hopefully, system
>>                  * designer) that this is bad.
>>                  */
>>                 if (nor->flags & SNOR_F_BROKEN_RESET)
>>                         printf("enabling reset hack; may not recover from unexpected
>> reboots\n");
>>                 set_4byte(nor, nor->info, 1);
>>         }
>>
>> So set_4byte() is not called for Spansion flashes. So I don't see need for this
>> patch.
>>
>>
>>>> There is patch to unset CONFIG_SPI_FLASH_BAR, for boards that are
>>>> using 4-byte addressing mode with this flash
>>>> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatc
>>>>
>> hwork.ozlabs.org%2Fpatch%2F1108287%2F&data=02%7C01%7Cashish.k
>> umar
>>>>
>> %40nxp.com%7C035eff480dc046fe37c108d70b78630e%7C686ea1d3bc2b4c6f
>> a92cd
>>>>
>> 99c5c301635%7C0%7C0%7C636990484001673615&sdata=dsD80nUUtmJ
>> yOoxGvi
>>>> iDFaOx1SKPXzI6bzXXpH630i8%3D&reserved=0
>>>
>>> Thanks for the information.
>>>
>>> Applied to u-boot-spi/master
>>>
>>
>> --
>> Regards
>> Vignesh