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

[Pratyush Yadav <p.yadav@ti.com>]

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.

On 15/11/21 05:44AM, Tudor.Ambarus@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@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.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.