On 9/29/20 9:12 PM, 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:

quoted

quoted

quoted

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

quoted

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

Entering an IO mode even using volatile bits (which gets cleared on SW
or HW reset) creates a "state" that SW needs to keep track of which is
why "stateful" term is used. I think that's what is implied here

AFAIU, Boris's original RFC[1] introducing X-X-X mode also used
"stateful mode" term in the same context .

quoted

quoted

For the volatile state case, we can parse the SFDP SCCR map, save if we
can enter stateful modes in a volatile way, and if yes allow the entering.

If we are not support volatile configurations, the reset line is enough
to take care of unexpected resets, no? I don't see any need to parse

the reset line is excellent for the stateless flashes, it guarantees that the
volatile bits will return to their default state. Disabling the clock, waiting
for a period and re-enabling it again should do the trick too, but probably
a dedicated reset line is safer.

I don't think disable-wait-enable sequence of clock input to flash will
trigger a reset... You have to take down the power and thus force flash
to go through power-down/power-up sequence or use HW or SW reset sequences

[1]
https://patchwork.ozlabs.org/project/linux-mtd/cover/20181012084825.23697-1-boris.brezillon@bootlin.com/

Regards
Vignesh

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek