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

From: Yicong Yang <yangyicong@hisilicon.com>
To: "Vignesh Raghavendra" <vigneshr@ti.com>,
	"Matthias Weißer" <m.weisser.m@gmail.com>,
	p.yadav@ti.com
Cc: sergei.shtylyov@cogentembedded.com, tudor.ambarus@microchip.com,
	richard@nod.at, me@yadavpratyush.com, john.garry@huawei.com,
	linuxarm@huawei.com, linux-mtd@lists.infradead.org,
	miquel.raynal@bootlin.com, alexander.sverdlin@nokia.com
Subject: Re: [PATCH 2/2] mtd: spi-nor: Disable the flash quad mode in spi_nor_restore()
Date: Fri, 4 Sep 2020 15:54:51 +0800	[thread overview]
Message-ID: <0c3dcb65-3a6d-d742-da83-3b71f5417115@hisilicon.com> (raw)
In-Reply-To: <10ab34bf-653a-ae72-286e-43b08dc7f909@ti.com>

On 2020/9/3 13:59, Vignesh Raghavendra wrote:
>
> 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?

we use module paramters to indicate the bus width in a private version of our driver,
yet this hasn't been upstreamed.

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

well this make sense to me.

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

I've send the revert patches. You may found at :
https://lore.kernel.org/linux-mtd/1599205640-26690-1-git-send-email-yangyicong@hisilicon.com/

but I still have something uncertain, I think we should avoid setting the non-volatile bits
in spi-nor driver, should we?

Regards
Yicong

>
>
> Regards
> Vignesh
> .
>

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