[PATCH] mtd: spi-nor: Use CLSR command for FL-L chips

Tue Feb 23 11:36:04 EST 2021

Hi, Tudor

So sorry to reply so late, now I'm committed to solving this problem.
After checking over sixty datasheets and almost at least one datasheet 
for each family of the manufacturers, I can finally deviler some 
conclusions.

1. RDSR2(07H) instruction is just used by Spansion manufacturer.
As for all other manufacturers, most common instructions for reading SR2 
is "35H",  and "15H" is for reading SR3. Some manufactures have specific 
registers that are similar to status register, such as "Information 
Register", "Suspend Status Register", "Extended Read Register", 
"Security Register", "Flag Status Register" and also have specific 
instructions for reading them, such as"2BH/09H/81H/2BH/70H".

2. RDSR2(07H) instruction is not just existed in Spansion "S25FL-L" 
family, also existed in Spansion"S79FL-S, S70FS-S, S25FL-S, S25FS-S, 
CYRS, CY15B/V-SN" families. We just haven't use it before, it has been 
there for a long time.

3. Many manufacturers are using CLSR(30H) instruction, but the functions 
are different.
The most common function of "30H" instruction is  to resume the 
suspended bits, but in many Spansion's products , the function is to 
clear P_ERR and E_ERR bits. In some other Spansion's products(such as 
S25FS-S), Spansion provided a configure register, which can make "30H" 
instruction execute the alternative resuming suspended bits function 
like other manufacturers.

4. P_ERR and E_ERR bits are existed in many manufacturers, reside in 
specific register, such as "Information Register""Flag Status Register" 
and so on, and the instructions for reading those register and clearing 
the ERR bits varies according to the different manufacturer.

Based on the facts above, and to resolve the original "sr_ready" problem 
within "S25FL-L" family of Spansion, I think the solution is not much 
different. We need a "sr_ready" function pointer and maybe a new flag to 
mark the twisted status  register.  Actually I have made a rough fix 
based on that(not include fixing spi_nor_xread_sr and 
spi_nor_fsr_ready). Would like to learn your suggestions before going 
further.

The checked datasheets and related marks are too many, it may not be 
appropriate to post them here, but I'd like to share, once needed.

Best Regards,
Yaliang

On 1/26/21 4:51 PM, Tudor.Ambarus at microchip.com wrote:
> Hi, Yaliang,
>
> On 1/26/21 4:06 AM, Wang, Yaliang wrote:
>> or xxh(EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Hi, Vignesh and Tudor
>>
>> It's really inspiring receiving from you, and
>>
>> On 1/24/2021 2:12 PM, Vignesh Raghavendra wrote:
>>>    Hi, Yaliang
>>>
>>> On 1/23/21 11:15 PM, Tudor.Ambarus at microchip.com wrote:
>>>> Hi, Yaliang,
>>>>
>>> [...]
>>>>> +static int spi_nor_s25fl_l_sr_ready(struct spi_nor *nor)
>>>>> +{
>>>>> +   u8 *sr = nor->bouncebuf;
>>>>> +   int ret;
>>>>> +
>>>>> +   ret = spi_nor_read_sr(nor, sr);
>>>>> +   if (ret)
>>>>> +           return ret;
>>>>> +
>>>>> +   /**
>>>>> +    * P_ERR and E_ERR bits are located in the Status Register 2
>>>>> +    * of Cypress FL-L series chips.
>>>>> +    */
>>>>> +   ret = spi_nor_s25fl_l_read_sr2(nor, &sr[1]);
>>>>> +   if (ret)
>>>>> +           return ret;
>>>>> +
>>>>> +   if (nor->flags & SNOR_F_USE_CLSR && sr[1] & (SR_E_ERR | SR_P_ERR)) {
>>>> If checking other manufacturer datasheets, would you please check if
>>>> CLSR is used by any other manufacturer?
>>>>
>>> This is limited to certain subsets of Cypress/Spansion flashes.
>>>
>>>>> +           if (sr[1] & SR_E_ERR)
>>>>> +                   dev_err(nor->dev, "Erase Error occurred\n");
>>>>> +           else
>>>>> +                   dev_err(nor->dev, "Programming Error occurred\n");
>>>>> +
>>>>> +           spi_nor_clear_sr(nor);
>>>>> +
>>>>> +           /*
>>>>> +            * WEL bit remains set to one when an erase or page program
>>>>> +            * error occurs. Issue a Write Disable command to protect
>>>>> +            * against inadvertent writes that can possibly corrupt the
>>>>> +            * contents of the memory.
>>>>> +            */
>>>>> +           ret = spi_nor_write_disable(nor);
>>>>> +           if (ret)
>>>>> +                   return ret;
>>>>> +
>>>>> +           return -EIO;
>>>>> +   }
>>>>> +
>>>>> +   return !(sr[0] & SR_WIP);
>>>>> +}
>>>>> +
>>>>>    /**
>>>>>     * spi_nor_sr_ready() - Query the Status Register to see if the flash is ready
>>>>>     * for new commands.
>>>>> @@ -546,7 +628,19 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
>>>>>     */
>>>>>    static int spi_nor_sr_ready(struct spi_nor *nor)
>>>>>    {
>>>>> -   int ret = spi_nor_read_sr(nor, nor->bouncebuf);
>>>>> +   int ret;
>>>>> +   const struct flash_info *tmpinfo = nor->info ? nor->info : spi_nor_read_id(nor);
>>>>> +
>>>>> +   if (IS_ERR_OR_NULL(tmpinfo))
>>>>> +           return -ENOENT;
>>>>> +
>>>>> +   if (!strcmp(tmpinfo->name, "s25fl064l")
>>>>> +                   || !strcmp(tmpinfo->name, "s25fl128l")
>>>>> +                   || !strcmp(tmpinfo->name, "s25fl256l")) {
>>>>> +           return spi_nor_s25fl_l_sr_ready(nor);
>>>>> +   }
>>>> No, we can't accept flash name comparisons in the SPI NOR core.
>>>> If CLSR and RDSR2 are just spansion specific, we can provide a sr_ready
>>>> function pointer that can be filled by spansion flashes. Or some other
>>>> method depending on the CLSR and RDSR2 exposure through other
>>>> manufacturers.
>>>>
>>>> Ideally one would skim through at least 2 - 3 datasheets from each
>>>> manufacturer available in SPI NOR. Preferable each datasheet from
>>>> a different manufacturer family. Unfortunately I'm not aware of any
>>>> standard that describes all the supported SPI NOR commands.
>>>> If you find this overwhelming, I can share the workload with you,
>>>> but at best effort. If you go through this by yourself, please
>>>> save the name of the datasheet flashes that you go through.
>>>>
>>> Agree with Tudor. There is quite a bit of variation even within
>>> Cypress/Spansion devices. S28 series of flashes have these error bits
>>> within SR1 register. See: https://www.cypress.com/file/513996/download
>>>
>>> So, this cannot be in SPI NOR core.
>>>
>>> spi_nor_xread_sr(), spi_nor_fsr_ready() and spi_nor_clear_sr() are all
>>> vendor/family specific and should be moved to appropriate vendor
>>> specific drivers.
>> Have to admit the comparisons and the function here are inappropriate,
>> though it's the fastest way to fix this problem.
>>
>> I'd like to move spansion specific operations out of the core.c first,
>> basing on sr_ready function pointer or other mechanisms mentioned by
>> Tudor, and after that I'll see if I'm capable to cope with the other
>> several vendor/family specific functions.
>>
> The difficulty is not in moving the code in spansion.c, but rather
> the documentation effort, to skim through all the datasheets. So if
> you're going through all the datasheets to check whether RDSR2 is used
> by other vendor or not, please also check the other ops that me and
> Vignesh indicated, so that we don't duplicate the effort.
>
> Cheers,
> ta