Hi Jungseung,
在 2020/3/14 21:50, Jungseung Lee 写道:
Hi, chenxiang,
On Sat, Mar 14, 2020 at 6:58 PM chenxiang (M) <chenxiang66@xxxxxxxxxxxxx> wrote:
Hi Jungseung,
在 2020/3/9 19:44, Jungseung Lee 写道:
Hi,
2020-03-09 (월), 15:50 +0800, chenxiang (M):
Hi Jungseung,
在 2020/3/7 16:24, Jungseung Lee 写道:
Hi,
2020-03-06 (금), 20:19 +0800, chenxiang (M):
Hi Jungseung,
在 2020/3/4 19:07, Jungseung Lee 写道:
The current mainline locking was restricted and could only be
applied
to flashes that has 3 block protection bit and fixed locking
ratio.
A new method of normalization was reached at the end of the
discussion [1].
(1) - if bp slot is insufficient.
(2) - if bp slot is sufficient.
if (bp_slots_needed > bp_slots) // (1)
min_prot_length = sector_size << (bp_slots_needed -
bp_slots);
else // (2)
min_prot_length = sector_size;
This patch changes block protection handling logic based on
min_prot_length.
It is suitable for the overall flashes with exception of some
corner case
and easy to extend and apply for the case of 2bit or 4bit block
protection.
[1]
https://protect2.fireeye.com/url?k=e80b1f1a-b5db17f2-e80a9455-000babff32e3-dadc30d1176f6374&u=http://lists.infradead.org/pipermail/linux-mtd/2020-February/093934.html
I have tested the patchset on one of my board (there is micron
flash
n25q128a11 which supports 4bit BP, and also bp3 is on bit6 of SR,
TB
bit is on bit5 of SR), so
i modify the code as follows to enable the lock/unlock of
n25q128a11.
- { "n25q128a11", INFO(0x20bb18, 0, 64 * 1024, 256,
SECT_4K |
SPI_NOR_QUAD_READ) },
+ { "n25q128a11", INFO(0x20bb18, 0, 64 * 1024, 256,
+ SECT_4K | SPI_NOR_QUAD_READ |
+ USE_FSR | SPI_NOR_HAS_LOCK |
SPI_NOR_HAS_TB |
+ SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6) },
There are two issues i met:
(1) i lock/unlock the full region of the flash, lock is valid,
but
there is error when unlock the flash, i query the status of it is
unlock (the issue i think it is
the same as the issue John has reported before
https://lore.kernel.org/linux-mtd/c1a92c89-020d-0847-b7bf-41dbfd9b972e@xxxxxxxxxxxxx/
),
i screenshot the log of the operation as follows:
Looks like the unlock operation was actually done (as can be
checked
from the following query of the status) but an error is coming with
EIO.
I think another part of sr handling is related with your case.
(maybe
SR read back test ?)
Yes, it is the issue of SR read back test: it writes 0X2 (bit WEL
is
set), but it reads back 0x0 (bit WEL is cleared).
I am reviewing tudor's patches and it seems solve your issue
effectively.
http://lists.infradead.org/pipermail/linux-mtd/2020-March/094231.html
Yes, it solves my issue.
If you can dump the sr value & dev_dbg msg, it will be helpful to
define this issue.
(2) i try to lock part of the flash region such as ./flash_lock
/dev/mtd0 0xc00000 10, it reports invalid argument,
and i am not sure whether it is something wrong with my
operation.
It is unable to lock such region since the spi flash doesn't
support
it. only we can lock it from the top or the bottom.
like this for n25q128a11,
flash_lock /dev/mtd0 0xff0000 0x10
flash_lock /dev/mtd0 0x0 0x10
Do you mean if lock/unlcok from top, the address of lock/unlock
commands should be the address of 255th block (0xff0000), 254th
block(0xfe0000), 252nd block(0xfc0000), ...., 128th block (0x800000)?
If lock/unlock from bottom, the address of lock/unlock commands
should
be always the address of 0th block (0x0)?
I'm not fully understanding the usage of flash_lock, but it would be
better to use such addresses for lock/unlocking to make it under
control.
There are some ambiguous parts to explain that since some lock/unlock
operation is still working well without the addresses.
LOCK
- Return success if the requested area is already locked.
- If requested area is not fully matched with lock portion of the
flash, lock some of the portion including the request area as it could
be.
UNLOCK
- Return success if the requested area is already unlocked.
- If requested area is not fully matched with lock portion of the
flash, unlock all locked portion including the request area. the
portion would be bigger than requested area.
Thanks for you info.
I have tested above situations of lock and unlock, and still have a
question about it:
For unlock function, as you said, it will unlock all the locked portion
including the request area which would be bigger than requested area if
requested area is not fully matched with lock portion of the flash.
But for lock function, it seem not lock some of portion including the
request area as it could be, and it seems require the total locked area
must be matched with
some portion of the flash (it seems not allow hole between those regions).
Yes it is. The spi flash consequently controls the region that will be
locked through only one bp value on sr register.
I wrote only some of the patterns I checked in the current mainline
code, and frankly, I don't know if even this is always right in all
combinations.
Ok, thanks.
So i have tested those patchset + (enabled n25q128a11 private patch) on
flash n25q128a11, and it is ok, so you can add : Tested-by: Xiang Chen
<chenxiang66@xxxxxxxxxxxxx>.
If there would be next version, i will test them also.
Thanks,
For example, 16MB in my envirnment, i do as follows:
- lock [0xff0000, 0x1000000] which is the 255th block -> it is matched
with lock portion of the flash (BP3~0 = 0001, TB=0)
- lock [0xc00000, 0xff0000] or [0xc00000, 0xff1000] -> it also matched
with lock portion of the flash (BP3~0 = 0111, TB=0)
but if do it as follows:
- lock [0xff0000, 0x1000000] which is the 255th block -> it is matched
with lock portion of the flash (BP3~0 = 0001, TB=0)
- lock [0xc00000, 0xc10000] -> it will report invalid argument at the
second time, in my thought it would lock [0xc00000, 0x1000000] which
will including those two regions
So, the lock/unlock would be able to work without the addresses. but in
my case I don't use the way because it will makes hard to tracking the
locked area.
Thanks,
Note the block count of examples is 0x10 not 10. The locking try
with
block count under minimum block protection length will be failed.
Thanks,
Signed-off-by: Jungseung Lee <js07.lee@xxxxxxxxxxx>
---
drivers/mtd/spi-nor/spi-nor.c | 110 ++++++++++++++++++++-----
---
------
1 file changed, 66 insertions(+), 44 deletions(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-
nor/spi-nor.c
index caf0c109cca0..c58c27552a74 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1784,29 +1784,64 @@ static int spi_nor_erase(struct
mtd_info
*mtd, struct erase_info *instr)
return ret;
}
+static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
+{
+ return SR_BP2 | SR_BP1 | SR_BP0;
+}
+
+static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
+{
+ if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
+ return SR_TB_BIT6;
+ else
+ return SR_TB_BIT5;
+}
+
+static int stm_get_min_prot_length(struct spi_nor *nor)
+{
+ int bp_slots, bp_slots_needed;
+ u8 mask = spi_nor_get_bp_mask(nor);
+
+ bp_slots = (mask >> SR_BP_SHIFT) + 1;
+
+ /* Reserved one for "protect none" and one for "protect
all".
*/
+ bp_slots = bp_slots - 2;
+
+ bp_slots_needed = ilog2(nor->info->n_sectors);
+
+ if (bp_slots_needed > bp_slots)
+ return nor->info->sector_size <<
+ (bp_slots_needed - bp_slots);
+ else
+ return nor->info->sector_size;
+}
+
static void stm_get_locked_range(struct spi_nor *nor, u8 sr,
loff_t *ofs,
uint64_t *len)
{
struct mtd_info *mtd = &nor->mtd;
- u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
- u8 tb_mask = SR_TB_BIT5;
- int pow;
+ int min_prot_len;
+ u8 mask = spi_nor_get_bp_mask(nor);
+ u8 tb_mask = spi_nor_get_tb_mask(nor);
+ u8 bp = (sr & mask) >> SR_BP_SHIFT;
- if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
- tb_mask = SR_TB_BIT6;
-
- if (!(sr & mask)) {
+ if (!bp) {
/* No protection */
*ofs = 0;
*len = 0;
- } else {
- pow = ((sr & mask) ^ 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;
+ return;
}
+
+ min_prot_len = stm_get_min_prot_length(nor);
+ *len = min_prot_len << (bp - 1);
+
+ if (*len > mtd->size)
+ *len = mtd->size;
+
+ if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
+ *ofs = 0;
+ else
+ *ofs = mtd->size - *len;
}
/*
@@ -1880,8 +1915,9 @@ static int stm_lock(struct spi_nor *nor,
loff_t ofs, uint64_t len)
{
struct mtd_info *mtd = &nor->mtd;
int ret, status_old, status_new;
- u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
- u8 tb_mask = SR_TB_BIT5;
+ int min_prot_len;
+ u8 mask = spi_nor_get_bp_mask(nor);
+ u8 tb_mask = spi_nor_get_tb_mask(nor);
u8 pow, val;
loff_t lock_len;
bool can_be_top = true, can_be_bottom = nor->flags &
SNOR_F_HAS_SR_TB;
@@ -1918,20 +1954,14 @@ static int stm_lock(struct spi_nor
*nor,
loff_t ofs, uint64_t len)
else
lock_len = ofs + len;
- if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
- tb_mask = SR_TB_BIT6;
+ if (lock_len == mtd->size) {
+ val = mask; /* fully locked */
+ } else {
+ min_prot_len = stm_get_min_prot_length(nor);
+ pow = ilog2(lock_len) - ilog2(min_prot_len) +
1;
+ val = pow << SR_BP_SHIFT;
+ }
- /*
- * Need smallest pow such that:
- *
- * 1 / (2^pow) <= (len / size)
- *
- * so (assuming power-of-2 size) we do:
- *
- * pow = ceil(log2(size / len)) = log2(size) -
floor(log2(len))
- */
- pow = ilog2(mtd->size) - ilog2(lock_len);
- val = mask - (pow << SR_BP_SHIFT);
if (val & ~mask)
return -EINVAL;
/* Don't "lock" with no region! */
@@ -1966,8 +1996,9 @@ static int stm_unlock(struct spi_nor
*nor,
loff_t ofs, uint64_t len)
{
struct mtd_info *mtd = &nor->mtd;
int ret, status_old, status_new;
- u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
- u8 tb_mask = SR_TB_BIT5;
+ int min_prot_len;
+ u8 mask = spi_nor_get_bp_mask(nor);
+ u8 tb_mask = spi_nor_get_tb_mask(nor);
u8 pow, val;
loff_t lock_len;
bool can_be_top = true, can_be_bottom = nor->flags &
SNOR_F_HAS_SR_TB;
@@ -2004,22 +2035,13 @@ static int stm_unlock(struct spi_nor
*nor,
loff_t ofs, uint64_t len)
else
lock_len = ofs;
- if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
- tb_mask = SR_TB_BIT6;
- /*
- * Need largest pow such that:
- *
- * 1 / (2^pow) >= (len / size)
- *
- * so (assuming power-of-2 size) we do:
- *
- * pow = floor(log2(size / len)) = log2(size) -
ceil(log2(len))
- */
- pow = ilog2(mtd->size) - order_base_2(lock_len);
if (lock_len == 0) {
val = 0; /* fully unlocked */
} else {
- val = mask - (pow << SR_BP_SHIFT);
+ min_prot_len = stm_get_min_prot_length(nor);
+ pow = ilog2(lock_len) - ilog2(min_prot_len) +
1;
+ val = pow << SR_BP_SHIFT;
+
/* Some power-of-two sizes are not supported */
if (val & ~mask)
return -EINVAL;
.
.
.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
