Re: [PATCH v10 05/17] mtd: spi-nor: add support for DTR protocol

From: <Tudor.Ambarus@microchip.com>
To: <p.yadav@ti.com>
Cc: <me@yadavpratyush.com>, <miquel.raynal@bootlin.com>,
	<richard@nod.at>, <vigneshr@ti.com>, <broonie@kernel.org>,
	<Nicolas.Ferre@microchip.com>, <alexandre.belloni@bootlin.com>,
	<Ludovic.Desroches@microchip.com>, <matthias.bgg@gmail.com>,
	<michal.simek@xilinx.com>, <linux-mtd@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>, <linux-spi@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-mediatek@lists.infradead.org>,
	<boris.brezillon@collabora.com>, <nsekhar@ti.com>
Subject: Re: [PATCH v10 05/17] mtd: spi-nor: add support for DTR protocol
Date: Tue, 29 Sep 2020 18:34:52 +0000	[thread overview]
Message-ID: <184f757b-dadc-0fd9-67f2-4c5903ec9c5c@microchip.com> (raw)
In-Reply-To: <20200929162943.qbnzmzgxb75wdpyo@ti.com>

On 9/29/20 7:29 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 29/09/20 03:42PM, Tudor.Ambarus@microchip.com wrote:
>> Hi, Pratyush,
>>
>> I'm replying to v10 so that we continue the discussion, but this applies to v13 as well.
>>
>> On 7/21/20 2:29 PM, Pratyush Yadav wrote:
>>
>>>>> @@ -2368,12 +2517,16 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor *nor, u32 *hwcaps)
>>>>>         struct spi_nor_flash_parameter *params = nor->params;
>>>>>         unsigned int cap;
>>>>>
>>>>> -       /* DTR modes are not supported yet, mask them all. */
>>>>> -       *hwcaps &= ~SNOR_HWCAPS_DTR;
>>>>> -
>>>>>         /* X-X-X modes are not supported yet, mask them all. */
>>>>>         *hwcaps &= ~SNOR_HWCAPS_X_X_X;
>>>>>
>>>>> +       /*
>>>>> +        * If the reset line is broken, we do not want to enter a stateful
>>>>> +        * mode.
>>>>> +        */
>>>>> +       if (nor->flags & SNOR_F_BROKEN_RESET)
>>>>> +               *hwcaps &= ~(SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR);
>>>>
>>>> A dedicated reset line is not enough for flashes that keep their state
>>>> in non-volatile bits. Since we can't protect from unexpected crashes in
>>>> the non volatile state case, we should enter these modes only with an
>>>> explicit request, i.e. an optional DT property: "update-nonvolatile-state",
>>>> or something similar.
>>>
>>> I wrote this patch with the assumption that we won't be supporting> non-volatile configuration as of now. In the previous discussions we
>>
>> I think we have to take care of the stateful flashes now, otherwise we'll risk
>> to end up with users that let their flashes in a mode from which they can't recover.
>> I made some small RFC patches in reply to your v13, let me know what you think.
> 
> I haven't gone through them yet. Will check tomorrow.
> 
>>> came to the conclusion that it is not easy to detect the flash if it
>>> boots in any mode other than 1S-1S-1S [0]. So if we update non-volatile
>>> state, the flash would be useless after a reboot because we won't be
>>> able to detect it in 8D mode. It doesn't matter if the reset line is
>>> connected or not because it will reset the flash to the non-volatile
>>> state, and we can't detect it from the non-volatile state.
>>
>> correct, so a reset line for stateful modes doesn't help and the comment from the
>> code should be updated. s/stateful/stateless
> 
> We are talking about two different kinds of "state" here. The state you

Right, I used 'stateful' for flashes that enter in a X-X-X mode by setting a
non-volatile bit and 'stateless' for those that enter in a X-X-X mode
via volatile bits.

> are talking about is the persistent state of the flash configured via
> non-volatile registers. Yes, a reset line doesn't help in that case at
> all.
> > The other state is the non-persistent state we set on the flash. Using
> 1S-1S-8D mode is stateless in the sense that we didn't change any state
> on the flash to be able to use this mode, and only had to use the
> correct opcode. If we execute a 1S-1S-1S command next it will also work
> because the flash is still interpreting opcodes in 1S mode. Using
> 8D-8D-8D or 4S-4S-4S mode is stateful because we did have to configure
> some state on the flash (which can very well be volatile). Once 8D-8D-8D
> or 4S-4S-4S mode is entered, we cannot execute 1S-1S-1S commands until
> we reset the flash because now the flash is interpreting commands in 4S
> or 8D mode. This means we introduced some state on the flash.
> 
> Having a reset line will not help against the former but will help
> against the latter. If the flash is in a stateful mode like 8D-8D-8D
> without a reset line, an unexpected reset could leave bootloader unable
> to boot because it issues the commands in 1S-1S-1S mode that the flash
> cannot interpret. So even if the state we set is volatile, we still want
> to avoid doing it if there is no reset line.
> 
> So I think the code and comment should stay as they are.

Ok. Cheers,
ta