Messages in this thread |  | | Date | Fri, 20 Dec 2019 13:46:40 +0100 | From | Michael Walle <> | Subject | Re: [PATCH 1/2] dt-bindings: mtd: spi-nor: document new flag |
| |
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?
|  |