Am 2020-02-03 14:56, schrieb Vignesh Raghavendra:
Hi Michael,
On 30/01/20 1:47 pm, Jungseung Lee wrote:
[...]
/*
* 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?
Flash devices have variable sector size, 64KB, 128KB or 256KB... While
mapping of number of sectors locked to BP bits is dependent on rules 1
to 3 you mentioned below, the size or area of flash protected depends
on
sector size.
Is there any flash device which has another sector size != 64KiB for the
BP bits?
So, the current formula in spi-nor.c (ignoring TB and other
boilerplate):
pow = ilog2(mtd->size) - ilog2(lock_len);
val = mask - (pow << shift);
This works only for devices with 64KB sector size as 8MB flash with
64KB
sector size would have 128 sectors (BP0-2 => 0b111 => 2^7).
It also only works with flashes >= 4MiB. See fix below. And IMHO this is
exactly
the same "problem" the flashes with 4 BP bits have.
A more generic formula would be:
Find n where 2^(n - 1) = len/sector-size
OR 2^ (n - 1) = len * n_sectors / mtd->size
Which solves to:
pow = ilog2(mtd->size) - ilog2(lock_len);
val = ilog2(nor->n_sectors) + 1 - pow;
I see this is what Jungseung has tried to implement here. Please
correct me if I got this wrong.
This, combined with point (3) below should provide a generic
implementation that should support a wide variety of flashes.
Of course, there are always exceptions and they need to be handled
using
custom hooks.
I don't have the patch that you shared with Jungseung. I would greatly
appreciate, if you and Jungseung could work on patch with above logic
as
well as fixes to handle overflow case?
https://lore.kernel.org/linux-mtd/20200123170130.8289-1-michael@xxxxxxxx/
As I said, that should work for both 3 and 4 bits. But be aware that
this
is an RFC and I've just tested it in in userspace, like the calculation
of the bits and transferred that into the driver. So one would actually
have to test that. But apparently no one had a even applied it.
-michael
Thanks a lot Jungseung and Michael for your efforts!
Regards
Vignesh
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).
It is mixed. Let's compare "en25qh128" from EON with "w25q128jv" from
Winbond. They have the same capacity(128MBit) and also supporting 3bit
block protection. (Note that the named BP3 bit of "en25qh128" is
working exactly same with T/B bit.)
"en25qh128" is following (2) and "w25q128jv" is following (1). It
seems
impossible to distinguish them by the flash size or the number of
protection bits.
I also still have no idea how your three rules are applicable for 2bit
block protection (bp0-1).
Each method requires each formula. I have no idea how to handle it
with
one formula (probably adding number of exceptional handling?)
without
any sectional flag. "w25q128jv(bp0-2)" is following (1) and
"n25q128a(bp0-3)" is following (2).
Well one uses three BPs the other four, and they all follow the
three
rules
above. Did you try my patch? Because I've (at least in userspace)
tested
it with 4 bits and got the correct results.
And yes, its actually two different formulas, but not for 3 and 4
bits like in your patch.
The title of my patch is "add 4bit block protection support". I just
let 3bit block protection as it is, I've implemented something what I
could check. As I mentioned, for all the flashes supporting 4bit block
protection only (2) is used and this patch has been implemented based
on this fact.
It is rather one formula (A) for flashes which don't
exhaust
the BP bits (eg. for 3 bits this would be flashes <= 16Mbit and for
4
bits
this would be for flashes <=8Gbit) and one (B) for the flashes where
every
BP bit combination is used. What is at the moment in the kernel is
the
second one, thus it will fail for flashes <= 16Mbit and 3 BP bits;
and
it
also fails for all flashes with 4 bits. My patch does a fixup on (B)
to
match the results of (A), because doing this is less invasive; and as
mentioned in the patch annex, might also be rewritten for a better
understanding.
I've never seen spi flashes greater than 8Gbit (maybe you also), so I
am not sure whether the three rules are applicable to 4 bit block
protection. Even the three rules doesn't seem to be general enough for
3bit or 2bit block protection.
-michael
We need new fomula based on n_sectors for BP3 at least.
No they are the same, but yes there is a bug in the current
implementation.
-michael
So for the 3 bit case the following flashes are border cases:
- 16mbit (less settings than slots)
- 32mbit (number of settings and free slots match)
- 64mbit (more settings than slots, first setting is
discarded)
That being said, I suspect all the 16mbit flashes (and below)
which
have
the _LOCK bit set are broken, because the entries has to be
shifted.
I'll
look into that later. This is the same "issue" you have with
the
4
bits.
So if this is fixed, you should not need another formula for
the
4
bit
case.
We need new calculation method for 4bit block protection
and
for
making
it more generic, I choose n_sectors.
On all the flashes I checked, n_sectors is proper value for
getting
block protected portion.
density portion n_sectors
W25M512JV 64MB 1/512 512
N25Q128A 16MB 1/256 256
N25Q512A 64MB 1/1024 1024
MT25QL02GCBB 256MB 1/4096 4096
The rules above apply to these flashes, too. Could you double
check
that?
-michael
+ val = val << SR_BP_SHIFT;
+
+ if (val & BIT(5) && mask & SR_BP3_BIT6)
+ val = (val & ~BIT(5)) | BIT(6);
+ } else {
+ val = mask - (pow << SR_BP_SHIFT);
+ }
+
if (val & ~mask)
return -EINVAL;
/* Don't "lock" with no region! */
@@ -1983,6 +2057,13 @@ static int stm_unlock(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 largest pow such that:
*
@@ -1995,13 +2076,20 @@ static int stm_unlock(struct
spi_nor
*nor,
loff_t ofs, uint64_t len)
pow = ilog2(mtd->size) -
order_base_2(lock_len);
if (lock_len == 0) {
val = 0; /* fully unlocked */
+ } else if (nor->flags & SNOR_F_HAS_SR_BP3) {
+ val = ilog2(nor->n_sectors) + 1 - pow;
+ val = val << SR_BP_SHIFT;
+
+ if (val & BIT(5) && mask & SR_BP3_BIT6)
+ val = (val & ~BIT(5)) | BIT(6);
} else {
val = mask - (pow << SR_BP_SHIFT);
- /* Some power-of-two sizes are not
supported */
- if (val & ~mask)
- return -EINVAL;
}
+ /* Some power-of-two sizes are not supported */
+ if (val & ~mask)
+ return -EINVAL;
+
status_new = (status_old & ~mask & ~tb_mask) |
val;
/* Don't protect status register if we're fully
unlocked */
@@ -4736,6 +4824,7 @@ static void
spi_nor_info_init_params(struct
spi_nor *nor)
/* Set SPI NOR sizes. */
params->size = (u64)info->sector_size * info-
n_sectors;
params->page_size = info->page_size;
+ params->n_sectors = info->n_sectors;
if (!(info->flags & SPI_NOR_NO_FR)) {
/* Default to Fast Read for DT and non-
DT
platform
devices. */
@@ -5192,6 +5281,11 @@ int spi_nor_scan(struct spi_nor
*nor,
const
char *name,
nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
if (info->flags & USE_CLSR)
nor->flags |= SNOR_F_USE_CLSR;
+ if (info->flags & SPI_NOR_HAS_BP3) {
+ nor->flags |= SNOR_F_HAS_SR_BP3;
+ if (info->flags & SPI_NOR_BP3_SR_BIT6)
+ nor->flags |=
SNOR_F_HAS_SR_BP3_BIT6;
+ }
if (info->flags & SPI_NOR_NO_ERASE)
mtd->flags |= MTD_NO_ERASE;
@@ -5199,6 +5293,7 @@ int spi_nor_scan(struct spi_nor
*nor,
const
char *name,
mtd->dev.parent = dev;
nor->page_size = params->page_size;
mtd->writebufsize = nor->page_size;
+ nor->n_sectors = params->n_sectors;
if (of_property_read_bool(np, "broken-flash-
reset"))
nor->flags |= SNOR_F_BROKEN_RESET;
diff --git a/include/linux/mtd/spi-nor.h
b/include/linux/mtd/spi-
nor.h
index 541c06d042e8..92d550501daf 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -129,7 +129,9 @@
#define SR_BP1 BIT(3) /* Block
protect 1
*/
#define SR_BP2 BIT(4) /* Block
protect 2
*/
#define SR_TB_BIT5 BIT(5) /* Top/Bottom
protect
*/
+#define SR_BP3_BIT5 BIT(5) /* Block
protect 3
*/
maybe just name it SR_BP3? would also be more consistent
with
the
proposal
above.
#define SR_TB_BIT6 BIT(6) /* Top/Bottom
protect
*/
+#define SR_BP3_BIT6 BIT(6) /* Block
protect 3
*/
#define SR_SRWD BIT(7) /* SR
write
protect
*/
/* Spansion/Cypress specific status bits */
#define SR_E_ERR BIT(5)
@@ -248,6 +250,8 @@ enum spi_nor_option_flags {
SNOR_F_HAS_16BIT_SR = BIT(9),
SNOR_F_NO_READ_CR = BIT(10),
SNOR_F_HAS_SR_TB_BIT6 = BIT(11),
+ SNOR_F_HAS_SR_BP3 = BIT(12),
+ SNOR_F_HAS_SR_BP3_BIT6 = BIT(13),
};
@@ -519,6 +523,7 @@ struct spi_nor_locking_ops {
*
* @size: the flash memory density in
bytes.
* @page_size: the page size of the SPI NOR
flash
memory.
+ * @n_sectors: number of sectors
* @hwcaps: describes the read and page
program
hardware
* capabilities.
* @reads: read capabilities ordered by
priority:
the
higher index
@@ -541,6 +546,7 @@ struct spi_nor_locking_ops {
struct spi_nor_flash_parameter {
u64 size;
u32 page_size;
+ u16 n_sectors;
struct spi_nor_hwcaps hwcaps;
struct spi_nor_read_command reads[SNOR_CMD_
READ_MAX
];
@@ -573,6 +579,7 @@ struct flash_info;
* @bouncebuf_size: size of the bounce buffer
* @info: spi-nor part JDEC MFR id and
other info
* @page_size: the page size of the SPI NOR
+ * @n_sector: number of sectors
* @addr_width: number of address bytes
* @erase_opcode: the opcode for erasing a sector
* @read_opcode: the read opcode
@@ -599,6 +606,7 @@ struct spi_nor {
size_t bouncebuf_size;
const struct flash_info *info;
u32 page_size;
+ u16 n_sectors;
u8 addr_width;
u8 erase_opcode;
u8 read_opcode;
--
2.17.1
______________________________________________________
Linux MTD discussion mailing list
https://protect2.fireeye.com/url?k=06b6dd5d-5b7d5a63-06b75612-0cc47a31309a-83164929001f7741&u=http://lists.infradead.org/mailman/listinfo/linux-mtd/
Thanks,
Jungseung Lee
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
