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

[Michael Walle]

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?