[PATCH v2 3/4] Revert "mtd: spi-nor-core: Perform a Soft Reset on boot"

[Tudor.Ambarus at microchip.com]

On 12/16/21 8:45 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Tudor,
> 
> I am not sure if you have sent a re-roll of this series. I am catching
> back up on my email backlog.

I haven't, I was busy with other things, but I will.

> 
> On 15/11/21 05:44AM, Tudor.Ambarus at microchip.com wrote:
>> On 11/12/21 3:13 PM, Pratyush Yadav wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 10/11/21 08:44AM, Tudor.Ambarus at microchip.com wrote:
>>>> On 11/9/21 9:26 PM, Pratyush Yadav wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> Hi Tudor,
>>>>
>>>> Hi, Pratyush,
>>>>
>>>> Thanks for reviewing the series.
>>>>
>>>>>
>>>>> On 04/11/21 01:49AM, Tudor Ambarus wrote:
>>>>>> This reverts commit 0be8ab1f166844d53477387dc9a1184161ef44ef.
>>>>>>
>>>>>> There are multiple reset commands and it was used one at guess,
>>>>>
>>>>> Correct.
>>>>>
>>>>>> see BFPT[dword(16)]. It is preferable to avoid issuing unsupported
>>>>>> commands to a flash. Since there's no config in mainline that actually
>>>>>> uses SPI_FLASH_SOFT_RESET_ON_BOOT, remove it entirely until proper
>>>>>
>>>>> I have been lagging behind on that front. I have some defconfig patches
>>>>> for TI platforms but I did not get around to cleaning them up and
>>>>> posting them upstream. Still, this feature is very much needed on TI
>>>>> platforms.
>>>>>
>>>>>> support is added. One should instead determine the mode in which the
>>>>>> flash device is configured, then to parse SFDP to determine the
>>>>>> cmd_ext_type and then to issue a READID command if flash identification
>>>>>> is really a must. JESD216D-01 proposes an algorithm to try to read the
>>>>>
>>>>> Firstly, Read ID command is not standardized in 8D-8D-8D mode. For
>>>>> example, Cypress S28 flash family expects 4 address bytes for Read ID in
>>>>> 8D-8D-8D mode whereas Micron MT35 flash family does not. Number of dummy
>>>>> cycles also tends to vary. There is no way to determine these parameters
>>>>> from SFDP.
>>>>
>>>> If flash supports SFDP, the read ID becomes of minor importance. We can as
>>>> well not issue the read ID at all.
>>>
>>> We need to read the ID so we can apply fixups. One option is to specify
>>
>> Fixup hooks are there just because the manufacturers can't get their SFDP
>> tables right. Read ID determines the static flags and parameters, which should
>> be used just as a fallback.
>>
>>> Read ID type in device tree.
>>
>> we may consider this when really needed. Until then we can use the recommended
>> sequence I guess.
> 
> Right. And I have two flashes which both need fixups and both have
> different Read ID commands in 8D-8D-8D mode: Micron MT35X and Cypress
> S28H. Both of those fortunately have the correct data about how to reset
> them so we should be good for now.
> 
>>
>>>
>>>>
>>>>>
>>>>> Also, you would have to run the detection algorithm for _every_ flash
>>>>> that SPI NOR supports since we don't really know what we are dealing
>>>>> with at this point. This would include flashes that do not support the
>>>>> Read SFDP command at all. If your goal is to not send unsupported
>>>>
>>>> but we can have a dt property or a config option to indicate when to issue
>>>> readSFDP.
>>>
>>> Okay.
>>>
>>>>
>>>>> commands to a flash then you won't get very far. On top of that Read
>>>>> SFDP is not mandatory in 8D-8D-8D mode per the xSPI standard so there is
>>>>> no guarantee that the detection algorithm would even work. It _should_
>>>>> work for most flashes but we will eventually have to deal with the
>>>>> corner cases.
>>>>
>>>> in case we can't determine the mode in which the flash is configured,
>>>> we can adopt other approach, see below.
>>>>>
>>>>> In other words, if you drop this then you would have to run the
>>>>> detection algorithm for every flash and see what mode it is in. If an
>>>>
>>>> not for every flash, just the ones that we marked as SFDP compliant.
>>>>
>>>>> 8D-8D-8D mode flash does support Read SFDP in 8D-8D-8D mode, then you
>>>>> would have to read SFDP, determine the extension and reset type, and
>>>>> then perform the reset. If it does not support the Read SFDP command
>>>>
>>>> right.
>>>>
>>>>> then you are left with a non-working flash. You would still probably end
>>>>> up issuing unsupported commands.
>>>>
>>>> not necessarily.
>>>>
>>>>>
>>>>> I can implement such an algorithm but is it really worth the hassle?
>>>>
>>>> I find having such an alg would be of benefit, yes.
>>>>
>>>>>
>>>>>> SFDP signature to determine the mode in which the flash is configured:
>>>>>> '''
>>>>>> try to read the SFDP signature (see 6.1) in 4-4-4 mode, if that fails
>>>>>> try 2-2-2 mode, and if that fails try 1-1-1 mode. For Octal devices,
>>>>>> these typically support SFDP read operation in both 1S-1S-1S mode and
>>>>>> 8D-8D-8D mode. If the host controller does not know exactly which
>>>>>> protocol mode is used for SFDP in 8D-8D-8D mode, this information can be
>>>>>> found by reading SFDP in 1S-1S-1S mode first. (To read an unknown device
>>>>>> directly in 8D-8D-8D mode, the host controller may read from address 0,
>>>>>> and count the number of dummy clocks required before the SFDP signature
>>>>>> is received.)
>>>>>> '''
>>>>
>>>> Below is the approach for the flashes that are not SFDP compliant:
>>>>
>>>>>> If the flash does not support SFDP at all, one should introduce dedicated
>>>>>> configs for each reset type and issue just the needed reset command.
>>>
>>> There are some problems with this approach. What if we have two flashes
>>> on the board and both use different reset types? How do we figure out
>>> which reset to apply? This applies to the current implementation as
>>> well. If there are two flashes then it will issue the reset to both even
>>> if one of them does not support/need it.
>>
>> One would have to choose the NOR manufacturer with care next time. If we'll
>> have to statically define the reset type for both the flashes, there's nothing
>> much we can do. Maybe if you have a gpio reset line connected to the flash you
>> can toggle that instead.
> 
> Or we can specify the reset type in the device tree? That should neatly
> solve the problem I believe.
> 

It is surely an approach to consider. The downside with it is that people might
abuse it, and use it regardless if the reset type is defined in SFDP or not.
Do we care?

ta