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

[Vignesh Raghavendra <vigneshr@ti.com>]

On 9/2/20 3:42 PM, Yicong Yang wrote:
> Hi Vignesh,
> 
> 
> On 2020/9/2 15:50, Vignesh Raghavendra wrote:
>> Hi Yicong,
>>
>> On 9/1/20 7:50 PM, Yicong Yang wrote:
>>> Hi Mathhias and Pratyush,
>>>
[...]
>>>
>> This will break backward compatibility... Imagine a new board being 
>> flashed from Kernel. Before this series, QE bit would be set at the end of flashing
>> and  ROM/bootloader (such as the one reported by Matthias) would work fine. 
>> After this series, QE bit would no longer be set and would most likely
>> break boot..
>>
>> I still am unable to understand what is the underlying problem that is 
>> being addressed here?
>>
>> You mention addressing issue loading the driver in Quad mode first and reload it in
>> Standard SPI/Dual mode. But per s25fs128s data sheet:
>> "
>> Quad Data Width (QUAD) CR1V[1]: When set to 1, this bit switches the data width of the device to 4-bit Quad Mode. That is, WP#
>> becomes IO2 and IO3 / RESET# becomes an active I/O signal when CS# is low or the RESET# input when CS# is high. The WP#
>> input is not monitored for its normal function and is internally set to high (inactive). The commands for Serial, and Dual I/O Read still
>> function normally but, there is no need to drive the WP# input for those commands when switching between commands using
>> different data path widths. Similarly, there is no requirement to drive the IO3 / RESET# during those commands (while CS# is low)."
>>
>> So setting QE bit should have no impact for serial/dual IO modes?
> 
> yes. and I reword the commit like below,  as suggested by Tudor and send a v2 patch. This thread is the v1 one.
> "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 Standard/Dual SPI mode,
> the QE bit is not cleared and the function of flash's WP# and RESET#/HOLD#
> have been switched to IO2 and IO3 and are not restored."
> 
> I believe we should restore the state of the flash when it's unloaded from the kernel. In previous code, if we load the flash
> in Quad mode (originally in Standard SPI mode) and shutdown, its WP# and RESET# won't be restored correctly. Seems
> the patch doesn't consider the condition that the flash has already in Quad mode before loaded and restore the flash
> in a wrong state.
> 

How do you load driver in Quad mode first and then reload in Single/Dual  
mode later on? What is the use case?

I don't think relying on WP# and RESET#/HOLD# functionality for a QSPI 
flash is right thing to do as these lines would act as data lines in 
Quad mode and thus WP# wont really protect contents of the flash 
when in Quad mode.

Also, below patch is not fool proof even for hypothetical case that you are
trying to solve: 
Consider a scenario of kernel crash or hard reset, then there will be no 
chance to call spi_nor_restore() and you would end up with QE bit set.. 
Upon reboot, kernel will find that QE bit and will simply restore the same
back on shutdown.

Given the fact that setting and unsetting NV bit causes wearing of this 
rather important bit and also breaks backward compatibility of tools 
that expect Kernel to set QE bit on flashing, I suggest reverting these patches:

cc59e6bb6cd6 mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()
be192209d5a3 mtd: spi-nor: Add capability to disable flash quad mode


Regards
Vignesh

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/