Re: [PATCH 1/2] dt-bindings: mtd: spi-nor: document new flag

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Vignesh,

Am 2019-12-19 06:33, schrieb Vignesh Raghavendra:
Hi Michael,

[...]
+- no-unlock : By default, linux unlocks the whole flash because there
+           are legacy flash devices which are locked by default
+           after reset. Set this flag if you don't want linux to
+           unlock the whole flash automatically. In this case you
+           can control the non-volatile bits by the
+           flash_lock/flash_unlock tools.


Current SPI NOR framework unconditionally unlocks entire flash which
I agree is not the best thing to do, but I don't think we need
new DT property here. MTD cmdline partitions and DT partitions already
provide a way to specify that a partition should remain locked[1][2]

I know that the MTD layer has the same kind of unlocking. But that
unlocking is done on a per mtd partition basis. Eg. consider something
like the following

 mtd1 bootloader  (locked)
 mtd2 firmware    (locked)
 mtd3 kernel
 mtd4 environment

Further assume, that the end of mtd2 aligns with one of the possible
locking areas which are supported by the flash chip. Eg. the first quarter.

The mtd layer would do two (or four, if "lock" property is set) unlock()
calls, one for mtd1 and one for mtd2.



My point here is, that the mtd partitions doesn't always map to the
locking regions of the SPI flash (at least if the are not merged together).


You are right! This will be an issue if existing partitions are not
aligned to locking regions.

I take my comments back... But I am not sure if a new DT property is the
needed. This does not describe HW and is specific to Linux SPI NOR
stack. How about a module parameter instead?
Module parameter won't provide per flash granularity in controlling
unlocking behavior. But I don't think that matters.

I don't argue against having a kernel parameter, but just wanting to point
out another alternative (which might be controversial):

 - What is the purpose of this unlock_all() at all. Apparently there are
   some flashes which have the protection bits set. Either at startup
(and then they are non-volatile) or they come in that state out of the
   factory. The latter makes little sense IMHO.

- Actually, all newer flashes we've used have these bits non-volatile and
   are unlocked by default.

So can't we have a whitelist (ie. a new flag in the spi_nor_ids) which
supresses the unlock if they haven't any block protections bit set
according to the manual? Because in this case the unlocking makes never
sense IMHO.

-michael


Tudor,

You had a patch doing something similar. Does module param sound good to
you?



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux