Am 2020-01-28 12:01, schrieb Jungseung Lee:
> Hi, Michael
>
> 2020-01-23 (Thu), 10:31 +0100, Michael Walle:
> > Am 2020-01-23 09:53, schrieb Jungseung Lee:
> > > Hi, Michael
> > >
> > > 2020-01-23 (Thu), 09:10 +0100, Michael Walle:
> > > > Hi Jungseung,
> > > >
> > > > Am 2020-01-23 07:22, schrieb Jungseung Lee:
> > > > > Hi, Michael
> > > > >
> > > > > 2020-01-22 (Wed), 20:36 +0100, Michael Walle:
> > > > > > Hi,
> > > > > >
> > > > > > > Currently, we are supporting block protection only for
> > > > > > > flash chips with 3 block protection bits in the SR
> > > > > > > register.
> > > > > > > This patch enables block protection support for some
> > > > > > > flash
> > > > > > > with
> > > > > > > 4 block protection bits(bp0-3).
> > > > > > >
> > > > > > > Signed-off-by: Jungseung Lee <js07.lee@xxxxxxxxxxx>
> > > > > > > ---
> > > > > > > v3 :
> > > > > > > Fix wrong ofs calculation on v2 patch
> > > > > > > v2 :
> > > > > > > Add sample table portion about 4bit block protection
> > > > > > > on
> > > > > > > the
> > > > > > > comment
> > > > > > > Trivial coding style change
> > > > > > >
> > > > > > > drivers/mtd/spi-nor/spi-nor.c | 127
> > > > > > > +++++++++++++++++++++++++++++-
> > > > > > > ----
> > > > > > > include/linux/mtd/spi-nor.h | 8 +++
> > > > > > > 2 files changed, 119 insertions(+), 16 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > > > > > > b/drivers/mtd/spi-
> > > > > > > nor/spi-nor.c
> > > > > > > index e3da6a8654a8..7e8af6c4fdfa 100644
> > > > > > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > > > > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > > > > > > @@ -238,6 +238,14 @@ struct flash_info {
> > > > > > > * status
> > > > > > > register.
> > > > > > > Must be
> > > > > > > used with
> > > > > > > *
> > > > > > > SPI_NOR_HAS_TB.
> > > > > > > */
> > > > > > > +#define SPI_NOR_HAS_BP3 BIT(17) /*
> > > > > > > + * Flash SR has
> > > > > > > 4 bit
> > > > > > > fields
> > > > > > > (BP0-3)
> > > > > > > + * for block
> > > > > > > protection.
> > > > > > > + */
> > > > > > > +#define SPI_NOR_BP3_SR_BIT6 BIT(18) /*
> > > > > > > + * BP3 is bit 6
> > > > > > > of
> > > > > > > status
> > > > > > > register.
> > > > > > > + * Must be used
> > > > > > > with
> > > > > > > SPI_NOR_HAS_BP3.
> > > > > > > + */
> > > > > > >
> > > > > > > /* Part specific fixup hooks. */
> > > > > > > const struct spi_nor_fixups *fixups;
> > > > > > > @@ -1767,23 +1775,47 @@ static void
> > > > > > > stm_get_locked_range(struct
> > > > > > > spi_nor *nor, u8 sr, loff_t *ofs,
> > > > > > > struct mtd_info *mtd = &nor->mtd;
> > > > > > > u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > > > > > > u8 tb_mask = SR_TB_BIT5;
> > > > > > > - int pow;
> > > > > > > + u8 bp;
> > > > > > > + int pow = 0;
> > > > > > >
> > > > > > > if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> > > > > > > tb_mask = SR_TB_BIT6;
> > > > > > >
> > > > > > > - if (!(sr & mask)) {
> > > > > > > - /* No protection */
> > > > > > > - *ofs = 0;
> > > > > > > - *len = 0;
> > > > > > > + if (nor->flags & SNOR_F_HAS_SR_BP3) {
> > > > > > > + u8 tmp;
> > > > > > > +
> > > > > > > + if (nor->flags &
> > > > > > > SNOR_F_HAS_SR_BP3_BIT6)
> > > > > > > + tmp = sr & (mask |
> > > > > > > SR_BP3_BIT6);
> > > > > > > + else
> > > > > > > + tmp = sr & (mask |
> > > > > > > SR_BP3_BIT5);
> > > > > > > +
> > > > > > > + if (tmp & SR_BP3_BIT6)
> > > > > > > + tmp = (tmp & ~BIT(6)) | BIT(5);
> > > > > > > +
> > > > > > > + bp = tmp >> SR_BP_SHIFT;
> > > > > > > + if (!bp) {
> > > > > > > + *ofs = 0;
> > > > > > > + *len = 0;
> > > > > > > + return;
> > > > > > > + }
> > > > > > > + if (bp <= ilog2(nor->n_sectors))
> > > > > > > + pow = ilog2(nor->n_sectors) + 1
> > > > > > > - bp;
> > > > > > > } else {
> > > > > > > - pow = ((sr & mask) ^ mask) >>
> > > > > > > SR_BP_SHIFT;
> > > > > > > - *len = mtd->size >> pow;
> > > > > > > - if (nor->flags & SNOR_F_HAS_SR_TB && sr
> > > > > > > &
> > > > > > > tb_mask)
> > > > > > > + bp = (sr & mask) >> SR_BP_SHIFT;
> > > > > > > + if (!bp) {
> > > > > > > *ofs = 0;
> > > > > > > - else
> > > > > > > - *ofs = mtd->size - *len;
> > > > > > > + *len = 0;
> > > > > > > + return;
> > > > > > > + }
> > > > > > > + pow = bp ^ (mask >> SR_BP_SHIFT);
> > > > > > > }
> > > > > > > +
> > > > > > > + *len = mtd->size >> pow;
> > > > > > > +
> > > > > > > + if (nor->flags & SNOR_F_HAS_SR_TB && sr &
> > > > > > > tb_mask)
> > > > > > > + *ofs = 0;
> > > > > > > + else
> > > > > > > + *ofs = mtd->size - *len;
> > > > > > > }
> > > > > > >
> > > > > > > /*
> > > > > > > @@ -1823,7 +1855,7 @@ static int
> > > > > > > stm_is_unlocked_sr(struct
> > > > > > > spi_nor
> > > > > > > *nor, loff_t ofs, uint64_t len,
> > > > > > >
> > > > > > > /*
> > > > > > > * Lock a region of the flash. Compatible with ST
> > > > > > > Micro
> > > > > > > and
> > > > > > > similar flash.
> > > > > > > - * Supports the block protection bits BP{0,1,2} in the
> > > > > > > status
> > > > > > > register
> > > > > > > + * Supports the block protection bits BP{0,1,2,3} in
> > > > > > > the
> > > > > > > status
> > > > > > > register
> > > > > > > * (SR). Does not support these features found in
> > > > > > > newer SR
> > > > > > > bitfields:
> > > > > > > * - SEC: sector/block protect - only handle SEC=0
> > > > > > > (block
> > > > > > > protect)
> > > > > > > * - CMP: complement protect - only support CMP=0
> > > > > > > (range
> > > > > > > is
> > > > > > > not
> > > > > > > complemented)
> > > > > > > @@ -1831,7 +1863,7 @@ static int
> > > > > > > stm_is_unlocked_sr(struct
> > > > > > > spi_nor
> > > > > > > *nor, loff_t ofs, uint64_t len,
> > > > > > > * Support for the following is provided conditionally
> > > > > > > for
> > > > > > > some
> > > > > > > flash:
> > > > > > > * - TB: top/bottom protect
> > > > > > > *
> > > > > > > - * Sample table portion for 8MB flash (Winbond
> > > > > > > w25q64fw):
> > > > > > > + * Sample table portion for 8MB flash (Winbond
> > > > > > > w25q64fw /
> > > > > > > BP0-
> > > > > > > 2):
> > > > > > > *
> > > > > > > * SEC | TB | BP2 | BP1 | BP0 | Prot
> > > > > > > Length |
> > > > > > > Protected Portion
> > > > > > > * ------------------------------------------------
> > > > > > > ----
> > > > > > > ----
> > > > > > > ----
> > > > > > > --------------
> > > > > > > @@ -1851,6 +1883,32 @@ static int
> > > > > > > stm_is_unlocked_sr(struct
> > > > > > > spi_nor
> > > > > > > *nor, loff_t ofs, uint64_t len,
> > > > > > > * 0 | 1 | 1 | 0 | 1 | 2
> > > > > > > MB |
> > > > > > > Lower
> > > > > > > 1/4
> > > > > > > * 0 | 1 | 1 | 1 | 0 | 4
> > > > > > > MB |
> > > > > > > Lower
> > > > > > > 1/2
> > > > > > > *
> > > > > > > + * Sample table portion for 64MB flash (Micron
> > > > > > > n25q512ax3
> > > > > > > /
> > > > > > > BP0-
> > > > > > > 3):
> > > > > > > + *
> > > > > > > + * TB | BP3 | BP2 | BP1 | BP0 | Prot
> > > > > > > Length |
> > > > > > > Protected Portion
> > > > > > > + * ------------------------------------------------
> > > > > > > ----
> > > > > > > ----
> > > > > > > ----
> > > > > > > --------------
> > > > > > > +
> > > > > > > * 0 | 0 | 0 | 0 | 0 | NONE
> > > > > > > |
> > > > > > > NONE
> > > > > > > + * 0 | 0 | 0 | 0 | 1 | 64
> > > > > > > KB |
> > > > > > > Upper
> > > > > > > 1/1024
> > > > > > > + * 0 | 0 | 0 | 1 | 0 | 128
> > > > > > > KB |
> > > > > > > Upper
> > > > > > > 1/512
> > > > > > > + * 0 | 0 | 0 | 1 | 1 | 256
> > > > > > > KB |
> > > > > > > Upper
> > > > > > > 1/256
> > > > > > > + * ...
> > > > > > > + * 0 | 1 | 0 | 0 | 1 | 16
> > > > > > > MB |
> > > > > > > Upper
> > > > > > > 1/4
> > > > > > > + * 0 | 1 | 0 | 1 | 0 | 32
> > > > > > > MB |
> > > > > > > Upper
> > > > > > > 1/2
> > > > > > > + * 0 | 1 | 0 | 1 | 1 | 64
> > > > > > > MB |
> > > > > > > ALL
> > > > > > > + * 0 | 1 | 1 | 0 | 0 | 64
> > > > > > > MB |
> > > > > > > ALL
> > > > > > > + * ...
> > > > > > > + * ------|-------|-------|-------|-------|-----------
> > > > > > > ----
> > > > > > > > ---
> > > > > > >
> > > > > > > ----
> > > > > > > ------------
> > > > > > > +
> > > > > > > * 1 | 0 | 0 | 0 | 0 | NONE
> > > > > > > |
> > > > > > > NONE
> > > > > > > + * 1 | 0 | 0 | 0 | 1 | 64
> > > > > > > KB |
> > > > > > > Lower
> > > > > > > 1/1024
> > > > > > > + * 1 | 0 | 0 | 1 | 0 | 128
> > > > > > > KB |
> > > > > > > Lower
> > > > > > > 1/512
> > > > > > > + * 1 | 0 | 0 | 1 | 1 | 256
> > > > > > > KB |
> > > > > > > Lower
> > > > > > > 1/256
> > > > > > > + * ...
> > > > > > > + * 1 | 1 | 0 | 0 | 1 | 16
> > > > > > > MB |
> > > > > > > Lower
> > > > > > > 1/4
> > > > > > > + * 1 | 1 | 0 | 1 | 0 | 32
> > > > > > > MB |
> > > > > > > Lower
> > > > > > > 1/2
> > > > > > > + * 1 | 1 | 0 | 1 | 1 | 64
> > > > > > > MB |
> > > > > > > ALL
> > > > > > > + * 1 | 1 | 1 | 0 | 0 | 64
> > > > > > > MB |
> > > > > > > ALL
> > > > > > > + * ...
> > > > > > > + *
> > > > > > > * Returns negative on errors, 0 on success.
> > > > > > > */
> > > > > > > static int stm_lock(struct spi_nor *nor, loff_t ofs,
> > > > > > > uint64_t
> > > > > > > len)
> > > > > > > @@ -1898,6 +1956,12 @@ static int stm_lock(struct
> > > > > > > spi_nor
> > > > > > > *nor,
> > > > > > > loff_t ofs, uint64_t len)
> > > > > > > if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
> > > > > > > tb_mask = SR_TB_BIT6;
> > > > > > >
> > > > > > > + if (nor->flags & SNOR_F_HAS_SR_BP3) {
> > > > > > > + if (nor->flags &
> > > > > > > SNOR_F_HAS_SR_BP3_BIT6)
> > > > > > > + mask = mask | SR_BP3_BIT6;
> > > > > > > + else
> > > > > > > + mask = mask | SR_BP3_BIT5;
> > > > > > > + }
> > > > > > > /*
> > > > > > > * Need smallest pow such that:
> > > > > > > *
> > > > > > > @@ -1908,7 +1972,17 @@ static int stm_lock(struct
> > > > > > > spi_nor
> > > > > > > *nor,
> > > > > > > loff_t ofs, uint64_t len)
> > > > > > > * pow = ceil(log2(size / len)) = log2(size)
> > > > > > > -
> > > > > > > floor(log2(len))
> > > > > > > */
> > > > > > > pow = ilog2(mtd->size) - ilog2(lock_len);
> > > > > > > - val = mask - (pow << SR_BP_SHIFT);
> > > > > > > +
> > > > > > > + if (nor->flags & SNOR_F_HAS_SR_BP3) {
> > > > > > > + val = ilog2(nor->n_sectors) + 1 - pow;
> > > > > >
> > > > > > Why do you use a new calculation here? As far as I can
> > > > > > see,
> > > > > > the
> > > > > > method is
> > > > > > the same except that is has one bit more. That also
> > > > > > raises
> > > > > > the
> > > > > > question why
> > > > > > n_sectors is now needed?
> > > > > >
> > > > > > Can't we just initialize the mask with
> > > > > >
> > > > > > mask = SR_BP2 | SR_BP1 | SR_BP0;
> > > > > > if (nor->flags & SNOR_F_HAS_SR_BP3)
> > > > > > mask |= SR_BP3_BIT5;
> > > > > >
> > > > > > do the calculation and checks and then move the
> > > > > > SR_BP3_BIT5
> > > > > > to
> > > > > > SR_BP3_BIT6
> > > > > > if SNOR_F_HAS_SR_BP3_BIT6 is set.
> > > > > >
> > > > >
> > > > > For most of flashes that supporting BP0-2, the smallest
> > > > > protected
> > > > > portion is fixed as 1/64
> > > > > and it can be properly handled by existing
> > > > > calculation. (Actually it's not fully generic, see flashes
> > > > > like
> > > > > w25q40bw or m25p80. Of course, it doesn't have
> > > > > SPI_NOR_HAS_LOCK
> > > > > flag
> > > > > even though it has BP0-2 bit in SR)
> > > >
> > > > No. The rules are always the same wether there are three or
> > > > four
> > > > BP
> > > > bits (the example in stm_lock() has not enough information on
> > > > this):
> > > >
> > > > (1) the first setting (besides 0) protects one sector. The
> > > > second
> > > > protects 2, the third 4 and so on. eg 2^N
> > > > (2) the last setting is _always_ protect all, just like the
> > > > '0'
> > > > setting
> > > > is always protect none.
> > > > (3) if there is an overflow because there are no more free
> > > > slots
> > > > for
> > > > further settings (for 3 bits with flashes > 32MBit, for
> > > > 4
> > > > bits if should be flashes > 16GBit), the first entry
> > > > will be
> > > > discarded (eg the very first is the "just one sector"
> > > > entry).
> > > >
> > > > This is true for all flashes which uses this kind of setting,
> > > > have a
> > > > look at the m25p80 or w25q40bw, these are no exception. It is
> > > > just
> > > > the
> > > > notation "lower 1/64" which is only true for flashes which
> > > > either
> > > > overflows in (3) or fill all entries (eg. with 3bits that
> > > > would
> > > > be
> > > > the
> > > > 32Mbit version).
> > > >
> > >
> > > Looks like you noticed that we need new calculation method that
> > > would
> > > be based on n_sectors :).
> >
> > No it will work without that (if I'm not mistaken). Give me some
> > time
> > and I'll post a patch.
>
> No, it must be based on n_sectors. To make 4bit block protection
> more
> generic, the lock sector size must NOT fixed as 64KB (as can be
> checked
> from your patch). See "mt35xu02g" and check the protected area and
> number of sectors from it's datasheet.
There is no public datasheet as far as I can see. And yes, actually
n_sectors is my "mtd-size / sector_size". But I don't see how
n_sectors
would help if the sector size changes.
> The rule you mentioned "the first setting (besides 0) protects one
> sector" is alawys true for *4bit* block protection. That's why I
> choose
> n_sectors for new calculation.
And how does flashes behave once all the free slots are full? It was
the
same with the 3bit flashes, they only overflowed with "newer"/bigger
flashes.
> >
> > > Rule (1) is NOT true for some flashes
> > > supporting BP0-2 and that's why I said that smallest protected
> > > portion
> > > is fixed as '1/64' for these flashes.
> >
> > No, you have to apply rule (3). (1) is only the starting point.
> > It
> > is
> > kind
> > of a sliding window.
> >
> > > See this one.
> > >
> > > W25Q20EW 256KB 1/4 ... = 64KB BP2
> > > W25Q128JV 16MB 1/64 ... = 256KB BP2 <--
> > > S25FL132K 4MB 1/64 ... = 64KB BP2 <--
> > > S25FL164K 8MB
> > > 1/64 ... = 128KB BP2 <--
> >
> > All these flashes need (3) to be applied, thus (1) doesn't apply
> > anymore.
> >
> > Let me give you an example for the 64MBit case, the settings
> > would
> > be:
> >
> > 0 sectors (corresponds to protect none)
> > 1 sector
> > 2 sectors
> > 4 sectors
> > 8 sectors
> > 16 sectors
> > 32 sectors
> > 64 sectors
> > 128 sectors (corresponds to protect all)
> >
> > Unfortunately, we have only 8 slots (because 3 BP bits),
> > therefore
> > we
> > have
> > to discard some setting. According to rule (2) 0 is always
> > "protect
> > none"
> > and 7 is always "protect all". Thus we have 6 settings left.
> > According
> > to
> > rule (3) we discard the first ones. In this case, this is the "1
> > sector"
> > setting. Thus we end up with the following possible settings:
> >
> > 0 sectors (corresponds to protect none)
> > 2 sectors
> > 4 sectors
> > 8 sectors
> > 16 sectors
> > 32 sectors
> > 64 sectors
> > 128 sectors (corresponds to protect all)
> >
> > If you have a 128Mbit flash, the next setting that would be
> > discarded
> > is
> > "2 sectors". Thus it would start with:
> >
> > 0 sectors (corresponds to protect none)
> > 4 sectors
> > [..]
> > 256 sectors (corresponds to protect all)
> >
> >
> > Another example W25Q20EW, following possible settings:
> >
> > 0 sectors (corresponds to protect none)
> > 1 sector
> > 2 sectors
> > 4 sectors (corresponds to protect all)
> >
> > We now have less settings then our 8 possible slots. So this is
> > where
> > rule (1) applies, because according to that the "1 sector"
> > setting is
> > the first possible setting besides 0.
> >
> > And this also applies to the 4 bit protection bits.
> >
> >
> >
> > > W25Q256JV 32MB 1/512 ... =
> > > 64KB BP3
> > > S25FL128L 16MB 1/256 ... = 64KB BP3
> > > S25FL256L 32MB 1/512 ... = 64KB BP3
> > >
> > > In current BP implementation, block protection is just working
> > > for
> > > some
> > > flashes that has smallest protected portion as '1/64'.
> >
> > No its currently working for all except flashes smaller than
> > 32Mbit.
>
> No. Not working for flashes supporting 4bit block protection.
>
> > Applied to the 4 bits, this would mean "it works for all except
> > flashes
> > smaller than 8Gbit" which are practially all. As I said, this is
> > a
> > bug
> > and once this bug is fixed, there should be no difference between
> > 3
> > and 4 bits.
> >
> > -michael
> >
>
> The exact fact is that locks operate in two different ways
> according to
> flash model.
>
> (1) the smallest protected portion is fixed.
> for BP0-2 : 1/64
> for BP0-1 : 1/4
As mentioned earlier, the ratio nomenclature is missleading and only
valid if the table is completely filled up.
Take a flash with 128kB and three BP bits (or even only two BP
bits).
The
smallest portion will be 64kB (which is one sector and not 1/64).
Thus the smallest portion is always one sector, unless the table is
overflowing, then the smallest will settle to 1/4 (for two), 1/64
(for
three)
and 1/16384 (for four bits).
> (2) the smallest protected portion is inversely propotional with
> number
> of sectors.
>
> For the flashes supporting 3bit block protection, (1) and (2) are
> mixed
> and used. But all the flashes supporting 4bit block protection
> listed
> on spi-nor.c, only (2) is used.
It is not mixed it just depends on the flash size (and the number of
protection bits).
