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

[Tudor.Ambarus at microchip.com]

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.

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

Cheers,
ta
> 
>>
>> Apart of the cmd_ext type, you need to also differentiate between the reset
>> types. So a combination of reset and cmd extension type configs.
>>
>> I prefer discovering the reset sequence dynamically whenever possible.
>> You can choose to use the configs approach directly, if you don't want
>> to spend time on the dynamic discovery approach, the two approaches
>> will be mutual exclusive anyway.
>>
>> So yes, I think we should revert this and at least have a better config
>> approach.
>>
>> Cheers,
>> ta
>>>>
>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus at microchip.com>
>>>> ---
>>>>  drivers/mtd/spi/Kconfig        |  9 ---------
>>>>  drivers/mtd/spi/spi-nor-core.c | 27 ---------------------------
>>>>  2 files changed, 36 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/spi/Kconfig b/drivers/mtd/spi/Kconfig
>>>> index 2d2b71c52d..14800b5e93 100644
>>>> --- a/drivers/mtd/spi/Kconfig
>>>> +++ b/drivers/mtd/spi/Kconfig
>>>> @@ -103,15 +103,6 @@ config SPI_FLASH_SOFT_RESET
>>>>        Enable support for xSPI Software Reset. It will be used to switch from
>>>>        Octal DTR mode to legacy mode on shutdown and boot (if enabled).
>>>>
>>>> -config SPI_FLASH_SOFT_RESET_ON_BOOT
>>>> -     bool "Perform a Software Reset on boot on flashes that boot in stateful mode"
>>>> -     depends on SPI_FLASH_SOFT_RESET
>>>> -     help
>>>> -      Perform a Software Reset on boot to allow detecting flashes that are
>>>> -      handed to us in Octal DTR mode. Do not enable this config on flashes
>>>> -      that are not supposed to be handed to U-Boot in Octal DTR mode, even
>>>> -      if they _do_ support the Soft Reset sequence.
>>>> -
>>>>  config SPI_FLASH_BAR
>>>>       bool "SPI flash Bank/Extended address register support"
>>>>       help
>>>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>>>> index df66a73495..caf764720c 100644
>>>> --- a/drivers/mtd/spi/spi-nor-core.c
>>>> +++ b/drivers/mtd/spi/spi-nor-core.c
>>>> @@ -3796,33 +3796,6 @@ int spi_nor_scan(struct spi_nor *nor)
>>>>
>>>>       nor->setup = spi_nor_default_setup;
>>>>
>>>> -#ifdef CONFIG_SPI_FLASH_SOFT_RESET_ON_BOOT
>>>> -     /*
>>>> -      * When the flash is handed to us in a stateful mode like 8D-8D-8D, it
>>>> -      * is difficult to detect the mode the flash is in. One option is to
>>>> -      * read SFDP in all modes and see which one gives the correct "SFDP"
>>>> -      * signature, but not all flashes support SFDP in 8D-8D-8D mode.
>>>> -      *
>>>> -      * Further, even if you detect the mode of the flash via SFDP, you
>>>> -      * still have the problem of actually reading the ID. The Read ID
>>>> -      * command is not standardized across flash vendors. Flashes can have
>>>> -      * different dummy cycles needed for reading the ID. Some flashes even
>>>> -      * expect a 4-byte dummy address with the Read ID command. All this
>>>> -      * information cannot be obtained from the SFDP table.
>>>> -      *
>>>> -      * So, perform a Software Reset sequence before reading the ID and
>>>> -      * initializing the flash. A Soft Reset will bring back the flash in
>>>> -      * its default protocol mode assuming no non-volatile configuration was
>>>> -      * set. This will let us detect the flash even if ROM hands it to us in
>>>> -      * Octal DTR mode.
>>>> -      *
>>>> -      * To accommodate cases where there is more than one flash on a board,
>>>> -      * and only one of them needs a soft reset, failure to reset is not
>>>> -      * made fatal, and we still try to read ID if possible.
>>>> -      */
>>>> -     spi_nor_soft_reset(nor);
>>>> -#endif /* CONFIG_SPI_FLASH_SOFT_RESET_ON_BOOT */
>>>> -
>>>>       info = spi_nor_read_id(nor);
>>>>       if (IS_ERR_OR_NULL(info))
>>>>               return -ENOENT;
>>>> --
>>>> 2.25.1
>>>>
>>>
>>> --
>>> Regards,
>>> Pratyush Yadav
>>> Texas Instruments Inc.
>>>
>>
> 
> --
> Regards,
> Pratyush Yadav
> Texas Instruments Inc.
>