From: "chenxiang (M)" <chenxiang66@hisilicon.com>
To: Jungseung Lee <js07.lee@samsung.com>,
Tudor Ambarus <tudor.ambarus@microchip.com>,
Vignesh Raghavendra <vigneshr@ti.com>,
<linux-mtd@lists.infradead.org>, <js07.lee@gmail.com>,
Michael Walle <michael@walle.cc>
Cc: John Garry <john.garry@huawei.com>, Linuxarm <linuxarm@huawei.com>
Subject: Re: [PATCH 1/3] mtd: spi-nor: reimplement block protection handling
Date: Mon, 9 Mar 2020 15:50:19 +0800 [thread overview]
Message-ID: <61b1cc1a-1b2d-9537-3bb3-8a7b5157d7df@hisilicon.com> (raw)
In-Reply-To: <23f0694c111ca096917ea69ef984e4e3b3cc22c4.camel@samsung.com>
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]
>>> 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@microchip.com/),
>>
>> 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).
>
> 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)?
>
> 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@samsung.com>
>>> ---
>>> 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/
next prev parent reply other threads:[~2020-03-09 7:50 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20200304110830epcas1p168bd480847959dc497ac5cc272fa2f80@epcas1p1.samsung.com>
2020-03-04 11:07 ` [PATCH 1/3] mtd: spi-nor: reimplement block protection handling Jungseung Lee
[not found] ` <CGME20200304110833epcas1p42958d6dce0081afabfdd4200258eddb8@epcas1p4.samsung.com>
2020-03-04 11:07 ` [PATCH 2/3] mtd: spi-nor: add 4bit block protection support Jungseung Lee
2020-03-13 16:24 ` Michael Walle
2020-03-17 11:00 ` Jungseung Lee
2020-03-17 11:35 ` Jungseung Lee
2020-03-17 14:52 ` Michael Walle
2020-03-18 6:01 ` Jungseung Lee
[not found] ` <CGME20200304110835epcas1p3a9daac6383c7ae2fa57a084d4992d5a9@epcas1p3.samsung.com>
2020-03-04 11:08 ` [PATCH 3/3] mtd: spi-nor: support lock/unlock for a few Micron chips Jungseung Lee
[not found] ` <3b7e6d52-e7e2-c444-1d59-5225a7260ea4@hisilicon.com>
2020-03-07 8:24 ` [PATCH 1/3] mtd: spi-nor: reimplement block protection handling Jungseung Lee
2020-03-09 7:50 ` chenxiang (M) [this message]
2020-03-09 11:20 ` Jungseung Lee
2020-03-09 11:44 ` Jungseung Lee
2020-03-14 9:58 ` chenxiang (M)
2020-03-14 13:50 ` Jungseung Lee
2020-03-16 7:21 ` chenxiang (M)
2020-03-18 7:17 ` Jungseung Lee
2020-03-13 15:21 ` Tudor.Ambarus
2020-03-13 17:20 ` Jungseung Lee
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=61b1cc1a-1b2d-9537-3bb3-8a7b5157d7df@hisilicon.com \
--to=chenxiang66@hisilicon.com \
--cc=john.garry@huawei.com \
--cc=js07.lee@gmail.com \
--cc=js07.lee@samsung.com \
--cc=linux-mtd@lists.infradead.org \
--cc=linuxarm@huawei.com \
--cc=michael@walle.cc \
--cc=tudor.ambarus@microchip.com \
--cc=vigneshr@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).