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, [email protected] 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, [email protected] 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.
