[PATCH 2/2] mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()

[Yicong Yang]

Hi,

Thanks for reviewing the patch.


On 2020/7/3 19:52, Tudor.Ambarus at microchip.com wrote:
> On 7/3/20 2:19 PM, Pratyush Yadav wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 02/07/20 11:02AM, Tudor.Ambarus at microchip.com wrote:
>>> On 6/16/20 4:02 PM, Yicong Yang wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> If the flash's quad mode is enabled, it'll remain in the quad mode when
>>>> it's removed. If we drive the flash next time in SPI/Dual mode, then
>>>> problem occurs as the flash's quad enable bit is not cleared.
>>> Please describe the problems that occur. When QE bit is one the flash
>>> operates in Standard/Dual/Quad SPI modes. WP# and RESET#/HOLD# are
>>> affected as they change their functionality to IO2 and IO3 when QE
>>> is 1. Is there anything else?
>> IIUC if we do anything that introduces a state on the flash, we want to
>> clear that state up on restore. That's what we (will) do for 8D mode and
> correct
>
>> for 4-byte addressing mode. Does that not apply here?
> yes, it does. I've just asked Yicong to describe in the commit message
> the problems that he encounters, for better understanding. Standard and
> Dual modes should still work with QE = 1. The only problem that I see
> is that WP# and RESET#/HOLD# are changing their functionality to IO2
> and IO3 when QE is 1. Is there anything else that I miss?

I think I have mixup the issues.
The problem I met is when I load the driver in Quad mode first and reload it in
Standard SPI/Dual mode, and tested the flash's read/write but I got mirrored
data which differs from what I wrote.  But seems the QE bit won't cause this,
(thanks for your illustration and it may not be the same issue) so it seems
improper to mention it here and I'll reword the commit.

(PS: the flash I tested is Cypress s25fs128s1 with spi-hisi-sfc-v3xx controller. )

Thanks,
Yicong

>>> While I find the intention good, there might be some problems here:
>>> 1/ w25q jvm variants come with QE "fixed" to 1. This probably means
>>> that QE is not writable, and a writing of QE to zero will be ignored,
>>> but we have to check.
>> In that case they shouldn't have a quad_enable() hook, no?
> Right. Although this scenario should be a false positive, probably the
> write of QE bit is ignored. There is a superfluous write of QE indeed,
> but maybe we can live with it.
>
>>> 2/S25FS128S: CR1NV[1] can set the default power-on state for the
>>> CR1V[1] to 1, i.e. QE to be set to 1 at power-on by default. The
>>> logic here complicates a bit, and maybe we'll have to amend the
>>> patch.
>>>
> We can come with a patch on top of these for 2/. Yicong, please address
> the minor comments and resubmit.
> Cheers.
>
>>>> Disable the quad mode in spi_nor_restore(), the flash will leave
>>>> quad mode when remove. This will make sure the flash always enter the
>>>> correct mode when loaded.
>>> s/correct/ Standard/Dual SPI
>>>
>>> Cheers,
>>> ta
>> --
>> Regards,
>> Pratyush Yadav
>> Texas Instruments India
>>