* [PATCH 1/3] mtd: spi-nor: reimplement block protection handling [not found] <CGME20200304110830epcas1p168bd480847959dc497ac5cc272fa2f80@epcas1p1.samsung.com> @ 2020-03-04 11:07 ` Jungseung Lee [not found] ` <CGME20200304110833epcas1p42958d6dce0081afabfdd4200258eddb8@epcas1p4.samsung.com> ` (3 more replies) 0 siblings, 4 replies; 18+ messages in thread From: Jungseung Lee @ 2020-03-04 11:07 UTC (permalink / raw) To: Tudor Ambarus, Vignesh Raghavendra, linux-mtd, js07.lee, js07.lee, chenxiang, Michael Walle 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 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; -- 2.17.1 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 18+ messages in thread
[parent not found: <CGME20200304110833epcas1p42958d6dce0081afabfdd4200258eddb8@epcas1p4.samsung.com>]
* [PATCH 2/3] mtd: spi-nor: add 4bit block protection support [not found] ` <CGME20200304110833epcas1p42958d6dce0081afabfdd4200258eddb8@epcas1p4.samsung.com> @ 2020-03-04 11:07 ` Jungseung Lee 2020-03-13 16:24 ` Michael Walle 0 siblings, 1 reply; 18+ messages in thread From: Jungseung Lee @ 2020-03-04 11:07 UTC (permalink / raw) To: Tudor Ambarus, Vignesh Raghavendra, linux-mtd, js07.lee, js07.lee, chenxiang, Michael Walle 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 flashes with 4 block protection bits(bp0-3). Signed-off-by: Jungseung Lee <js07.lee@samsung.com> --- drivers/mtd/spi-nor/spi-nor.c | 82 ++++++++++++++++++++++++++++++++--- include/linux/mtd/spi-nor.h | 4 ++ 2 files changed, 79 insertions(+), 7 deletions(-) diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index c58c27552a74..31a2106e529a 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_4BIT_BP 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_4BIT_BP. + */ /* Part specific fixup hooks. */ const struct spi_nor_fixups *fixups; @@ -1786,7 +1794,16 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) static u8 spi_nor_get_bp_mask(struct spi_nor *nor) { - return SR_BP2 | SR_BP1 | SR_BP0; + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; + + if (nor->flags & SNOR_F_HAS_4BIT_BP) { + if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6) + mask = mask | SR_BP3_BIT6; + else + mask = mask | SR_BP3_BIT5; + } + + return mask; } static u8 spi_nor_get_tb_mask(struct spi_nor *nor) @@ -1797,12 +1814,26 @@ static u8 spi_nor_get_tb_mask(struct spi_nor *nor) return SR_TB_BIT5; } +static u8 stm_get_bpval_from_sr(struct spi_nor *nor, u8 sr) { + u8 mask = spi_nor_get_bp_mask(nor); + u8 bp = sr & mask; + + if (bp & SR_BP3_BIT6) + bp = (bp & ~BIT(6)) | BIT(5); + + bp = bp >> SR_BP_SHIFT; + + return bp; +} + 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; + if (nor->flags & SNOR_F_HAS_4BIT_BP) + bp_slots = 1 << 4; + else + bp_slots = 1 << 3; /* Reserved one for "protect none" and one for "protect all". */ bp_slots = bp_slots - 2; @@ -1821,9 +1852,8 @@ static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs, { struct mtd_info *mtd = &nor->mtd; 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; + u8 bp = stm_get_bpval_from_sr(nor, sr); if (!bp) { /* No protection */ @@ -1881,7 +1911,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}/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) @@ -1889,7 +1919,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 * -------------------------------------------------------------------------- @@ -1909,6 +1939,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) @@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len) min_prot_len = stm_get_min_prot_length(nor); pow = ilog2(lock_len) - ilog2(min_prot_len) + 1; val = pow << SR_BP_SHIFT; + + if (val & BIT(5) && mask & SR_BP3_BIT6) + val = (val & ~BIT(5)) | BIT(6); } if (val & ~mask) @@ -2042,6 +2101,9 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len) pow = ilog2(lock_len) - ilog2(min_prot_len) + 1; val = pow << SR_BP_SHIFT; + if (val & BIT(5) && mask & SR_BP3_BIT6) + val = (val & ~BIT(5)) | BIT(6); + /* Some power-of-two sizes are not supported */ if (val & ~mask) return -EINVAL; @@ -5244,6 +5306,12 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, if (info->flags & USE_CLSR) nor->flags |= SNOR_F_USE_CLSR; + if (info->flags & SPI_NOR_4BIT_BP) { + nor->flags |= SNOR_F_HAS_4BIT_BP; + 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; diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index de90724f62f1..0190ed21576a 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 */ #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) @@ -240,6 +242,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_4BIT_BP = BIT(12), + SNOR_F_HAS_SR_BP3_BIT6 = BIT(13), }; -- 2.17.1 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] mtd: spi-nor: add 4bit block protection support 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 0 siblings, 1 reply; 18+ messages in thread From: Michael Walle @ 2020-03-13 16:24 UTC (permalink / raw) To: Jungseung Lee Cc: chenxiang, js07.lee, linux-mtd, Vignesh Raghavendra, Tudor Ambarus Hi Jungseung, sorry for the late review. Am 2020-03-04 12:07, schrieb Jungseung Lee: > 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 flashes with > 4 block protection bits(bp0-3). > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com> > --- > drivers/mtd/spi-nor/spi-nor.c | 82 ++++++++++++++++++++++++++++++++--- > include/linux/mtd/spi-nor.h | 4 ++ > 2 files changed, 79 insertions(+), 7 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c > b/drivers/mtd/spi-nor/spi-nor.c > index c58c27552a74..31a2106e529a 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_4BIT_BP 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_4BIT_BP. > + */ > > /* Part specific fixup hooks. */ > const struct spi_nor_fixups *fixups; > @@ -1786,7 +1794,16 @@ static int spi_nor_erase(struct mtd_info *mtd, > struct erase_info *instr) > > static u8 spi_nor_get_bp_mask(struct spi_nor *nor) > { > - return SR_BP2 | SR_BP1 | SR_BP0; > + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > + can we just use the SR_BP3 eg: u8 mask = SR_BP2 | SR_BP1 | SR_BP0; if (nor->flags & SNOR_F_HAS_4BIT_BP) mask |= SR_BP3; return mask; > + if (nor->flags & SNOR_F_HAS_4BIT_BP) { > + if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6) > + mask = mask | SR_BP3_BIT6; > + else > + mask = mask | SR_BP3_BIT5; > + } > + > + return mask; > } > > static u8 spi_nor_get_tb_mask(struct spi_nor *nor) > @@ -1797,12 +1814,26 @@ static u8 spi_nor_get_tb_mask(struct spi_nor > *nor) > return SR_TB_BIT5; > } > > +static u8 stm_get_bpval_from_sr(struct spi_nor *nor, u8 sr) { > + u8 mask = spi_nor_get_bp_mask(nor); > + u8 bp = sr & mask; > + > + if (bp & SR_BP3_BIT6) > + bp = (bp & ~BIT(6)) | BIT(5); > + > + bp = bp >> SR_BP_SHIFT; > + > + return bp; > +} Don't convert this. It makes the code really hard to read. See below. > + > 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; Then just keep this. > + if (nor->flags & SNOR_F_HAS_4BIT_BP) > + bp_slots = 1 << 4; > + else > + bp_slots = 1 << 3; > > /* Reserved one for "protect none" and one for "protect all". */ > bp_slots = bp_slots - 2; > @@ -1821,9 +1852,8 @@ static void stm_get_locked_range(struct spi_nor > *nor, u8 sr, loff_t *ofs, > { > struct mtd_info *mtd = &nor->mtd; > 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; > + u8 bp = stm_get_bpval_from_sr(nor, sr); also this. > > if (!bp) { > /* No protection */ > @@ -1881,7 +1911,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}/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) > @@ -1889,7 +1919,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 > * > -------------------------------------------------------------------------- > @@ -1909,6 +1939,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) > @@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor *nor, loff_t > ofs, uint64_t len) > min_prot_len = stm_get_min_prot_length(nor); > pow = ilog2(lock_len) - ilog2(min_prot_len) + 1; > val = pow << SR_BP_SHIFT; > + > + if (val & BIT(5) && mask & SR_BP3_BIT6) > + val = (val & ~BIT(5)) | BIT(6); .. and the use just the following here: if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3) val = (val & ~SR_BP3) | SR_BP3_BIT6; Ie. just use the "normal case" where all BP bits are next to each other and then fixup the resulting value and shift the SR3 bit if necessary. This will be much easier to read. > } > > if (val & ~mask) > @@ -2042,6 +2101,9 @@ static int stm_unlock(struct spi_nor *nor, > loff_t ofs, uint64_t len) > pow = ilog2(lock_len) - ilog2(min_prot_len) + 1; > val = pow << SR_BP_SHIFT; > > + if (val & BIT(5) && mask & SR_BP3_BIT6) > + val = (val & ~BIT(5)) | BIT(6); > + here would be the other way around: if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6) val = (val & ~SR_BP3_BIT6) | SR_BP3; > /* Some power-of-two sizes are not supported */ > if (val & ~mask) > return -EINVAL; > @@ -5244,6 +5306,12 @@ int spi_nor_scan(struct spi_nor *nor, const char > *name, > if (info->flags & USE_CLSR) > nor->flags |= SNOR_F_USE_CLSR; > > + if (info->flags & SPI_NOR_4BIT_BP) { > + nor->flags |= SNOR_F_HAS_4BIT_BP; > + 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; > > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > index de90724f62f1..0190ed21576a 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 */ IMHO just SR_BP3. but that is a matter of taste. But it is easier on the eye in the mask = SR_BP3 | SR_BP2 etc case. -michael > #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) > @@ -240,6 +242,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_4BIT_BP = BIT(12), > + SNOR_F_HAS_SR_BP3_BIT6 = BIT(13), > > }; ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] mtd: spi-nor: add 4bit block protection support 2020-03-13 16:24 ` Michael Walle @ 2020-03-17 11:00 ` Jungseung Lee 2020-03-17 11:35 ` Jungseung Lee 0 siblings, 1 reply; 18+ messages in thread From: Jungseung Lee @ 2020-03-17 11:00 UTC (permalink / raw) To: Michael Walle Cc: Vignesh Raghavendra, chenxiang, Tudor Ambarus, linux-mtd, js07.lee, js07.lee Hi, Michael, On Fri, 2020-03-13 at 17:24 +0100, Michael Walle wrote: > Hi Jungseung, > > sorry for the late review. > Not at all. thanks for your review. > Am 2020-03-04 12:07, schrieb Jungseung Lee: > > 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 flashes with > > 4 block protection bits(bp0-3). > > > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com> > > --- > > drivers/mtd/spi-nor/spi-nor.c | 82 > > ++++++++++++++++++++++++++++++++--- > > include/linux/mtd/spi-nor.h | 4 ++ > > 2 files changed, 79 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c > > b/drivers/mtd/spi-nor/spi-nor.c > > index c58c27552a74..31a2106e529a 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_4BIT_BP 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_4BIT_BP. > > + */ > > > > /* Part specific fixup hooks. */ > > const struct spi_nor_fixups *fixups; > > @@ -1786,7 +1794,16 @@ static int spi_nor_erase(struct mtd_info > > *mtd, > > struct erase_info *instr) > > > > static u8 spi_nor_get_bp_mask(struct spi_nor *nor) > > { > > - return SR_BP2 | SR_BP1 | SR_BP0; > > + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > > + > > can we just use the SR_BP3 eg: > > u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > if (nor->flags & SNOR_F_HAS_4BIT_BP) > mask |= SR_BP3; > return mask; > > I'd prefer this one too if we can, but there are some places to need the real mask value. It is also used in other places such as spi_nor_sr_lock/unlock. > > + if (nor->flags & SNOR_F_HAS_4BIT_BP) { > > + if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6) > > + mask = mask | SR_BP3_BIT6; > > + else > > + mask = mask | SR_BP3_BIT5; > > + } > > + > > + return mask; > > } > > > > static u8 spi_nor_get_tb_mask(struct spi_nor *nor) > > @@ -1797,12 +1814,26 @@ static u8 spi_nor_get_tb_mask(struct > > spi_nor > > *nor) > > return SR_TB_BIT5; > > } > > > > +static u8 stm_get_bpval_from_sr(struct spi_nor *nor, u8 sr) { > > + u8 mask = spi_nor_get_bp_mask(nor); > > + u8 bp = sr & mask; > > + > > + if (bp & SR_BP3_BIT6) > > + bp = (bp & ~BIT(6)) | BIT(5); > > + > > + bp = bp >> SR_BP_SHIFT; > > + > > + return bp; > > +} > > Don't convert this. It makes the code really hard to read. See below. > > > + > > 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; > > Then just keep this. > > > + if (nor->flags & SNOR_F_HAS_4BIT_BP) > > + bp_slots = 1 << 4; > > + else > > + bp_slots = 1 << 3; > > > > /* Reserved one for "protect none" and one for "protect all". > > */ > > bp_slots = bp_slots - 2; > > @@ -1821,9 +1852,8 @@ static void stm_get_locked_range(struct > > spi_nor > > *nor, u8 sr, loff_t *ofs, > > { > > struct mtd_info *mtd = &nor->mtd; > > 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; > > + u8 bp = stm_get_bpval_from_sr(nor, sr); > > also this. > > > > > if (!bp) { > > /* No protection */ > > @@ -1881,7 +1911,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}/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) > > @@ -1889,7 +1919,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 > > * > > ----------------------------------------------------------------- > > --------- > > @@ -1909,6 +1939,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) > > @@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor *nor, > > loff_t > > ofs, uint64_t len) > > min_prot_len = stm_get_min_prot_length(nor); > > pow = ilog2(lock_len) - ilog2(min_prot_len) + 1; > > val = pow << SR_BP_SHIFT; > > + > > + if (val & BIT(5) && mask & SR_BP3_BIT6) > > + val = (val & ~BIT(5)) | BIT(6); > > .. and the use just the following here: > > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3) > val = (val & ~SR_BP3) | SR_BP3_BIT6; > > Ie. just use the "normal case" where all BP bits are next to each > other > and then fixup the resulting value and shift the SR3 bit if > necessary. > This will be much easier to read. > Yes, I agree. It would be better to minimize this kind of conversion in one place. > > } > > > > if (val & ~mask) > > @@ -2042,6 +2101,9 @@ static int stm_unlock(struct spi_nor *nor, > > loff_t ofs, uint64_t len) > > pow = ilog2(lock_len) - ilog2(min_prot_len) + 1; > > val = pow << SR_BP_SHIFT; > > > > + if (val & BIT(5) && mask & SR_BP3_BIT6) > > + val = (val & ~BIT(5)) | BIT(6); > > + > > here would be the other way around: > > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6) > val = (val & ~SR_BP3_BIT6) | SR_BP3; > > > > /* Some power-of-two sizes are not supported */ > > if (val & ~mask) > > return -EINVAL; > > @@ -5244,6 +5306,12 @@ int spi_nor_scan(struct spi_nor *nor, const > > char > > *name, > > if (info->flags & USE_CLSR) > > nor->flags |= SNOR_F_USE_CLSR; > > > > + if (info->flags & SPI_NOR_4BIT_BP) { > > + nor->flags |= SNOR_F_HAS_4BIT_BP; > > + 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; > > > > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi- > > nor.h > > index de90724f62f1..0190ed21576a 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 > > */ > > IMHO just SR_BP3. but that is a matter of taste. But it is easier on > the eye in the mask = SR_BP3 | SR_BP2 etc case. > SR_BP3 would be a more appropriate name if we could set the case with all BP bits next to each other as the "normal case." I'm going to write patches based on latest spi-nor/next including what you mentioned. Thanks, > -michael > > > #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) > > @@ -240,6 +242,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_4BIT_BP = BIT(12), > > + SNOR_F_HAS_SR_BP3_BIT6 = BIT(13), > > > > }; > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] mtd: spi-nor: add 4bit block protection support 2020-03-17 11:00 ` Jungseung Lee @ 2020-03-17 11:35 ` Jungseung Lee 2020-03-17 14:52 ` Michael Walle 0 siblings, 1 reply; 18+ messages in thread From: Jungseung Lee @ 2020-03-17 11:35 UTC (permalink / raw) To: Michael Walle Cc: Vignesh Raghavendra, chenxiang, Tudor Ambarus, linux-mtd, js07.lee, js07.lee Hi, Micahel On Tue, 2020-03-17 at 20:00 +0900, Jungseung Lee wrote: > Hi, Michael, > > On Fri, 2020-03-13 at 17:24 +0100, Michael Walle wrote: > > Hi Jungseung, > > > > sorry for the late review. > > > > Not at all. thanks for your review. > > > Am 2020-03-04 12:07, schrieb Jungseung Lee: > > > 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 flashes with > > > 4 block protection bits(bp0-3). > > > > > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com> > > > --- > > > drivers/mtd/spi-nor/spi-nor.c | 82 > > > ++++++++++++++++++++++++++++++++--- > > > include/linux/mtd/spi-nor.h | 4 ++ > > > 2 files changed, 79 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c > > > b/drivers/mtd/spi-nor/spi-nor.c > > > index c58c27552a74..31a2106e529a 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_4BIT_BP 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_4BIT_BP. > > > + */ > > > > > > /* Part specific fixup hooks. */ > > > const struct spi_nor_fixups *fixups; > > > @@ -1786,7 +1794,16 @@ static int spi_nor_erase(struct mtd_info > > > *mtd, > > > struct erase_info *instr) > > > > > > static u8 spi_nor_get_bp_mask(struct spi_nor *nor) > > > { > > > - return SR_BP2 | SR_BP1 | SR_BP0; > > > + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > > > + > > > > can we just use the SR_BP3 eg: > > > > u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > > if (nor->flags & SNOR_F_HAS_4BIT_BP) > > mask |= SR_BP3; > > return mask; > > > > > > I'd prefer this one too if we can, but there are some places to need > the real mask value. It is also used in other places such as > spi_nor_sr_lock/unlock. > > > > + if (nor->flags & SNOR_F_HAS_4BIT_BP) { > > > + if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6) > > > + mask = mask | SR_BP3_BIT6; > > > + else > > > + mask = mask | SR_BP3_BIT5; > > > + } > > > + > > > + return mask; > > > } > > > > > > static u8 spi_nor_get_tb_mask(struct spi_nor *nor) > > > @@ -1797,12 +1814,26 @@ static u8 spi_nor_get_tb_mask(struct > > > spi_nor > > > *nor) > > > return SR_TB_BIT5; > > > } > > > > > > +static u8 stm_get_bpval_from_sr(struct spi_nor *nor, u8 sr) { > > > + u8 mask = spi_nor_get_bp_mask(nor); > > > + u8 bp = sr & mask; > > > + > > > + if (bp & SR_BP3_BIT6) > > > + bp = (bp & ~BIT(6)) | BIT(5); > > > + > > > + bp = bp >> SR_BP_SHIFT; > > > + > > > + return bp; > > > +} > > > > Don't convert this. It makes the code really hard to read. See > > below. > > > > > + > > > 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; > > > > Then just keep this. > > > > > + if (nor->flags & SNOR_F_HAS_4BIT_BP) > > > + bp_slots = 1 << 4; > > > + else > > > + bp_slots = 1 << 3; > > > > > > /* Reserved one for "protect none" and one for "protect all". > > > */ > > > bp_slots = bp_slots - 2; > > > @@ -1821,9 +1852,8 @@ static void stm_get_locked_range(struct > > > spi_nor > > > *nor, u8 sr, loff_t *ofs, > > > { > > > struct mtd_info *mtd = &nor->mtd; > > > 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; > > > + u8 bp = stm_get_bpval_from_sr(nor, sr); > > > > also this. > > hmm, it looks like we still need some convertion here to get the exact bp value. Thanks, > > > > > > if (!bp) { > > > /* No protection */ > > > @@ -1881,7 +1911,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}/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) > > > @@ -1889,7 +1919,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 > > > * > > > ----------------------------------------------------------------- > > > --------- > > > @@ -1909,6 +1939,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) > > > @@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor *nor, > > > loff_t > > > ofs, uint64_t len) > > > min_prot_len = stm_get_min_prot_length(nor); > > > pow = ilog2(lock_len) - ilog2(min_prot_len) + 1; > > > val = pow << SR_BP_SHIFT; > > > + > > > + if (val & BIT(5) && mask & SR_BP3_BIT6) > > > + val = (val & ~BIT(5)) | BIT(6); > > > > .. and the use just the following here: > > > > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3) > > val = (val & ~SR_BP3) | SR_BP3_BIT6; > > > > Ie. just use the "normal case" where all BP bits are next to each > > other > > and then fixup the resulting value and shift the SR3 bit if > > necessary. > > This will be much easier to read. > > > > Yes, I agree. It would be better to minimize this kind of conversion > in > one place. > > > > } > > > > > > if (val & ~mask) > > > @@ -2042,6 +2101,9 @@ static int stm_unlock(struct spi_nor *nor, > > > loff_t ofs, uint64_t len) > > > pow = ilog2(lock_len) - ilog2(min_prot_len) + 1; > > > val = pow << SR_BP_SHIFT; > > > > > > + if (val & BIT(5) && mask & SR_BP3_BIT6) > > > + val = (val & ~BIT(5)) | BIT(6); > > > + > > > > here would be the other way around: > > > > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6) > > val = (val & ~SR_BP3_BIT6) | SR_BP3; > > > > > > > /* Some power-of-two sizes are not supported */ > > > if (val & ~mask) > > > return -EINVAL; > > > @@ -5244,6 +5306,12 @@ int spi_nor_scan(struct spi_nor *nor, > > > const > > > char > > > *name, > > > if (info->flags & USE_CLSR) > > > nor->flags |= SNOR_F_USE_CLSR; > > > > > > + if (info->flags & SPI_NOR_4BIT_BP) { > > > + nor->flags |= SNOR_F_HAS_4BIT_BP; > > > + 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; > > > > > > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi- > > > nor.h > > > index de90724f62f1..0190ed21576a 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 > > > */ > > > > IMHO just SR_BP3. but that is a matter of taste. But it is easier > > on > > the eye in the mask = SR_BP3 | SR_BP2 etc case. > > > > SR_BP3 would be a more appropriate name if we could set the case with > all BP bits next to each other as the "normal case." > > I'm going to write patches based on latest spi-nor/next including > what > you mentioned. > > Thanks, > > > -michael > > > > > #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) > > > @@ -240,6 +242,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_4BIT_BP = BIT(12), > > > + SNOR_F_HAS_SR_BP3_BIT6 = BIT(13), > > > > > > }; > > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] mtd: spi-nor: add 4bit block protection support 2020-03-17 11:35 ` Jungseung Lee @ 2020-03-17 14:52 ` Michael Walle 2020-03-18 6:01 ` Jungseung Lee 0 siblings, 1 reply; 18+ messages in thread From: Michael Walle @ 2020-03-17 14:52 UTC (permalink / raw) To: Jungseung Lee Cc: chenxiang, js07.lee, linux-mtd, Vignesh Raghavendra, Tudor Ambarus Am 2020-03-17 12:35, schrieb Jungseung Lee: > Hi, Micahel > > On Tue, 2020-03-17 at 20:00 +0900, Jungseung Lee wrote: >> Hi, Michael, >> >> On Fri, 2020-03-13 at 17:24 +0100, Michael Walle wrote: >> > Hi Jungseung, >> > >> > sorry for the late review. >> > >> >> Not at all. thanks for your review. >> >> > Am 2020-03-04 12:07, schrieb Jungseung Lee: >> > > 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 flashes with >> > > 4 block protection bits(bp0-3). >> > > >> > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com> >> > > --- >> > > drivers/mtd/spi-nor/spi-nor.c | 82 >> > > ++++++++++++++++++++++++++++++++--- >> > > include/linux/mtd/spi-nor.h | 4 ++ >> > > 2 files changed, 79 insertions(+), 7 deletions(-) >> > > >> > > diff --git a/drivers/mtd/spi-nor/spi-nor.c >> > > b/drivers/mtd/spi-nor/spi-nor.c >> > > index c58c27552a74..31a2106e529a 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_4BIT_BP 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_4BIT_BP. >> > > + */ >> > > >> > > /* Part specific fixup hooks. */ >> > > const struct spi_nor_fixups *fixups; >> > > @@ -1786,7 +1794,16 @@ static int spi_nor_erase(struct mtd_info >> > > *mtd, >> > > struct erase_info *instr) >> > > >> > > static u8 spi_nor_get_bp_mask(struct spi_nor *nor) >> > > { >> > > - return SR_BP2 | SR_BP1 | SR_BP0; >> > > + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; >> > > + >> > >> > can we just use the SR_BP3 eg: >> > >> > u8 mask = SR_BP2 | SR_BP1 | SR_BP0; >> > if (nor->flags & SNOR_F_HAS_4BIT_BP) >> > mask |= SR_BP3; >> > return mask; >> > >> > >> >> I'd prefer this one too if we can, but there are some places to need >> the real mask value. It is also used in other places such as >> spi_nor_sr_lock/unlock. >> >> > > + if (nor->flags & SNOR_F_HAS_4BIT_BP) { >> > > + if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6) >> > > + mask = mask | SR_BP3_BIT6; >> > > + else >> > > + mask = mask | SR_BP3_BIT5; >> > > + } >> > > + >> > > + return mask; >> > > } >> > > >> > > static u8 spi_nor_get_tb_mask(struct spi_nor *nor) >> > > @@ -1797,12 +1814,26 @@ static u8 spi_nor_get_tb_mask(struct >> > > spi_nor >> > > *nor) >> > > return SR_TB_BIT5; >> > > } >> > > >> > > +static u8 stm_get_bpval_from_sr(struct spi_nor *nor, u8 sr) { >> > > + u8 mask = spi_nor_get_bp_mask(nor); >> > > + u8 bp = sr & mask; >> > > + >> > > + if (bp & SR_BP3_BIT6) >> > > + bp = (bp & ~BIT(6)) | BIT(5); >> > > + >> > > + bp = bp >> SR_BP_SHIFT; >> > > + >> > > + return bp; >> > > +} >> > >> > Don't convert this. It makes the code really hard to read. See >> > below. >> > >> > > + >> > > 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; >> > >> > Then just keep this. >> > >> > > + if (nor->flags & SNOR_F_HAS_4BIT_BP) >> > > + bp_slots = 1 << 4; >> > > + else >> > > + bp_slots = 1 << 3; >> > > >> > > /* Reserved one for "protect none" and one for "protect all". >> > > */ >> > > bp_slots = bp_slots - 2; >> > > @@ -1821,9 +1852,8 @@ static void stm_get_locked_range(struct >> > > spi_nor >> > > *nor, u8 sr, loff_t *ofs, >> > > { >> > > struct mtd_info *mtd = &nor->mtd; >> > > 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; >> > > + u8 bp = stm_get_bpval_from_sr(nor, sr); >> > >> > also this. >> > > > hmm, it looks like we still need some convertion here to get the exact > bp value. OK I see. What has confused me before was that some "fixups" were done in helper functions whereas others where done in the actual stm_lock/stm_unlock(). What do you think about: (1) read the BP bits, if they are not consecutive move them around and normalize the value, eg. fix them up to be SR_BP3 | SR_BP2 | SR_BP1 | SR_BP0 >> SR_BP_SHIFT (2) do the operations on these bits, eg this should not shift the value by SR_BP_SHIFT anymore. This would be done in (3) (3) convert it back to the representation the flash would need it. So there could be a function spi_nor_get_bp_val() for (1) and spi_nor_set_bp_val() for (3). u8 spi_nor_get_bp_val(u8 sr) { u8 val; val = sr & (SR_BP2 | SR_BP1 | SR_BP0); if (nor->flags & SNOR_F_HAS_4BIT_BP) if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && sr & SR_BP3_BIT6) val |= SR_BP3; else val |= sr & SR_BP3 return val >> SR_BP_SHIFT; } void spi_nor_set_bp_val(u8 val, u8 *sr) { u8 bp3_bit; val <<= SR_BP_SHIFT; /* clear and set common bits */ *sr &= ~(SR_BP2 | SR_BP1 | SR_BP0); *sr |= val & (SR_BP2 | SR_BP1 | SR_BP0) /* BP3 is special because it may be on different bits */ if (nor->flags & SNOR_F_HAS_4BIT_BP) { if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6) bp3_bit = SR_BP3_BIT6; else bp3_bit = SR_BP3; if (val & SR_BP3) *sr |= bp3_bit; else *sr &= ~bp3_bit; } } This way we'd have all the fixups in one place. -michael >> > u8 mask = SR_BP2 | SR_BP1 | SR_BP0; >> > if (nor->flags & SNOR_F_HAS_4BIT_BP) >> > mask |= SR_BP3; >> > return mask; } -michael > > Thanks, > >> > > >> > > if (!bp) { >> > > /* No protection */ >> > > @@ -1881,7 +1911,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}/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) >> > > @@ -1889,7 +1919,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 >> > > * >> > > ----------------------------------------------------------------- >> > > --------- >> > > @@ -1909,6 +1939,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) >> > > @@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor *nor, >> > > loff_t >> > > ofs, uint64_t len) >> > > min_prot_len = stm_get_min_prot_length(nor); >> > > pow = ilog2(lock_len) - ilog2(min_prot_len) + 1; >> > > val = pow << SR_BP_SHIFT; >> > > + >> > > + if (val & BIT(5) && mask & SR_BP3_BIT6) >> > > + val = (val & ~BIT(5)) | BIT(6); >> > >> > .. and the use just the following here: >> > >> > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3) >> > val = (val & ~SR_BP3) | SR_BP3_BIT6; >> > >> > Ie. just use the "normal case" where all BP bits are next to each >> > other >> > and then fixup the resulting value and shift the SR3 bit if >> > necessary. >> > This will be much easier to read. >> > >> >> Yes, I agree. It would be better to minimize this kind of conversion >> in >> one place. >> >> > > } >> > > >> > > if (val & ~mask) >> > > @@ -2042,6 +2101,9 @@ static int stm_unlock(struct spi_nor *nor, >> > > loff_t ofs, uint64_t len) >> > > pow = ilog2(lock_len) - ilog2(min_prot_len) + 1; >> > > val = pow << SR_BP_SHIFT; >> > > >> > > + if (val & BIT(5) && mask & SR_BP3_BIT6) >> > > + val = (val & ~BIT(5)) | BIT(6); >> > > + >> > >> > here would be the other way around: >> > >> > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6) >> > val = (val & ~SR_BP3_BIT6) | SR_BP3; >> > >> > >> > > /* Some power-of-two sizes are not supported */ >> > > if (val & ~mask) >> > > return -EINVAL; >> > > @@ -5244,6 +5306,12 @@ int spi_nor_scan(struct spi_nor *nor, >> > > const >> > > char >> > > *name, >> > > if (info->flags & USE_CLSR) >> > > nor->flags |= SNOR_F_USE_CLSR; >> > > >> > > + if (info->flags & SPI_NOR_4BIT_BP) { >> > > + nor->flags |= SNOR_F_HAS_4BIT_BP; >> > > + 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; >> > > >> > > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi- >> > > nor.h >> > > index de90724f62f1..0190ed21576a 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 >> > > */ >> > >> > IMHO just SR_BP3. but that is a matter of taste. But it is easier >> > on >> > the eye in the mask = SR_BP3 | SR_BP2 etc case. >> > >> >> SR_BP3 would be a more appropriate name if we could set the case with >> all BP bits next to each other as the "normal case." >> >> I'm going to write patches based on latest spi-nor/next including >> what >> you mentioned. >> >> Thanks, >> >> > -michael >> > >> > > #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) >> > > @@ -240,6 +242,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_4BIT_BP = BIT(12), >> > > + SNOR_F_HAS_SR_BP3_BIT6 = BIT(13), >> > > >> > > }; >> >> ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] mtd: spi-nor: add 4bit block protection support 2020-03-17 14:52 ` Michael Walle @ 2020-03-18 6:01 ` Jungseung Lee 0 siblings, 0 replies; 18+ messages in thread From: Jungseung Lee @ 2020-03-18 6:01 UTC (permalink / raw) To: Michael Walle Cc: Vignesh Raghavendra, chenxiang, Tudor Ambarus, linux-mtd, js07.lee, js07.lee Hi, On Tue, 2020-03-17 at 15:52 +0100, Michael Walle wrote: > Am 2020-03-17 12:35, schrieb Jungseung Lee: > > Hi, Micahel > > > > On Tue, 2020-03-17 at 20:00 +0900, Jungseung Lee wrote: > > > Hi, Michael, > > > > > > On Fri, 2020-03-13 at 17:24 +0100, Michael Walle wrote: > > > > Hi Jungseung, > > > > > > > > sorry for the late review. > > > > > > > > > > Not at all. thanks for your review. > > > > > > > Am 2020-03-04 12:07, schrieb Jungseung Lee: > > > > > 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 flashes with > > > > > 4 block protection bits(bp0-3). > > > > > > > > > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com> > > > > > --- > > > > > drivers/mtd/spi-nor/spi-nor.c | 82 > > > > > ++++++++++++++++++++++++++++++++--- > > > > > include/linux/mtd/spi-nor.h | 4 ++ > > > > > 2 files changed, 79 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c > > > > > b/drivers/mtd/spi-nor/spi-nor.c > > > > > index c58c27552a74..31a2106e529a 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_4BIT_BP 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_4BIT_BP. > > > > > + */ > > > > > > > > > > /* Part specific fixup hooks. */ > > > > > const struct spi_nor_fixups *fixups; > > > > > @@ -1786,7 +1794,16 @@ static int spi_nor_erase(struct > > > > > mtd_info > > > > > *mtd, > > > > > struct erase_info *instr) > > > > > > > > > > static u8 spi_nor_get_bp_mask(struct spi_nor *nor) > > > > > { > > > > > - return SR_BP2 | SR_BP1 | SR_BP0; > > > > > + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > > > > > + > > > > > > > > can we just use the SR_BP3 eg: > > > > > > > > u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > > > > if (nor->flags & SNOR_F_HAS_4BIT_BP) > > > > mask |= SR_BP3; > > > > return mask; > > > > > > > > > > > > > > I'd prefer this one too if we can, but there are some places to > > > need > > > the real mask value. It is also used in other places such as > > > spi_nor_sr_lock/unlock. > > > > > > > > + if (nor->flags & SNOR_F_HAS_4BIT_BP) { > > > > > + if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6) > > > > > + mask = mask | SR_BP3_BIT6; > > > > > + else > > > > > + mask = mask | SR_BP3_BIT5; > > > > > + } > > > > > + > > > > > + return mask; > > > > > } > > > > > > > > > > static u8 spi_nor_get_tb_mask(struct spi_nor *nor) > > > > > @@ -1797,12 +1814,26 @@ static u8 spi_nor_get_tb_mask(struct > > > > > spi_nor > > > > > *nor) > > > > > return SR_TB_BIT5; > > > > > } > > > > > > > > > > +static u8 stm_get_bpval_from_sr(struct spi_nor *nor, u8 sr) > > > > > { > > > > > + u8 mask = spi_nor_get_bp_mask(nor); > > > > > + u8 bp = sr & mask; > > > > > + > > > > > + if (bp & SR_BP3_BIT6) > > > > > + bp = (bp & ~BIT(6)) | BIT(5); > > > > > + > > > > > + bp = bp >> SR_BP_SHIFT; > > > > > + > > > > > + return bp; > > > > > +} > > > > > > > > Don't convert this. It makes the code really hard to read. See > > > > below. > > > > > > > > > + > > > > > 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; > > > > > > > > Then just keep this. > > > > > > > > > + if (nor->flags & SNOR_F_HAS_4BIT_BP) > > > > > + bp_slots = 1 << 4; > > > > > + else > > > > > + bp_slots = 1 << 3; > > > > > > > > > > /* Reserved one for "protect none" and one for "protect > > > > > all". > > > > > */ > > > > > bp_slots = bp_slots - 2; > > > > > @@ -1821,9 +1852,8 @@ static void stm_get_locked_range(struct > > > > > spi_nor > > > > > *nor, u8 sr, loff_t *ofs, > > > > > { > > > > > struct mtd_info *mtd = &nor->mtd; > > > > > 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; > > > > > + u8 bp = stm_get_bpval_from_sr(nor, sr); > > > > > > > > also this. > > > > > > > > hmm, it looks like we still need some convertion here to get the > > exact > > bp value. > > OK I see. What has confused me before was that some "fixups" were > done > in helper functions whereas others where done in the actual > stm_lock/stm_unlock(). > > What do you think about: > > (1) read the BP bits, if they are not consecutive move them around > and > normalize the value, eg. fix them up to be > SR_BP3 | SR_BP2 | SR_BP1 | SR_BP0 >> SR_BP_SHIFT > (2) do the operations on these bits, eg this should not shift the > value by SR_BP_SHIFT anymore. This would be done in (3) > (3) convert it back to the representation the flash would need it. > > So there could be a function spi_nor_get_bp_val() for (1) and > spi_nor_set_bp_val() for (3). > > u8 spi_nor_get_bp_val(u8 sr) > { > u8 val; > > val = sr & (SR_BP2 | SR_BP1 | SR_BP0); > > if (nor->flags & SNOR_F_HAS_4BIT_BP) > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && sr & SR_BP3_BIT6) > val |= SR_BP3; > else > val |= sr & SR_BP3 > > return val >> SR_BP_SHIFT; > } > > void spi_nor_set_bp_val(u8 val, u8 *sr) > { > u8 bp3_bit; > > val <<= SR_BP_SHIFT; > > /* clear and set common bits */ > *sr &= ~(SR_BP2 | SR_BP1 | SR_BP0); > *sr |= val & (SR_BP2 | SR_BP1 | SR_BP0) > > /* BP3 is special because it may be on different bits */ > if (nor->flags & SNOR_F_HAS_4BIT_BP) { > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6) > bp3_bit = SR_BP3_BIT6; > else > bp3_bit = SR_BP3; > > if (val & SR_BP3) > *sr |= bp3_bit; > else > *sr &= ~bp3_bit; > } > } > > This way we'd have all the fixups in one place. > It's good suggestion, but if there are no additional use cases, it would be good idea to place them within without using helper function. I'm going to post new patch soon. Thanks, > > -michael > > > > > u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > > > > if (nor->flags & SNOR_F_HAS_4BIT_BP) > > > > mask |= SR_BP3; > > > > return mask; > > > } > > > -michael > > > > > Thanks, > > > > > > > > > > > > if (!bp) { > > > > > /* No protection */ > > > > > @@ -1881,7 +1911,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}/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) > > > > > @@ -1889,7 +1919,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 > > > > > * > > > > > ----------------------------------------------------------- > > > > > ------ > > > > > --------- > > > > > @@ -1909,6 +1939,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) > > > > > @@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor > > > > > *nor, > > > > > loff_t > > > > > ofs, uint64_t len) > > > > > min_prot_len = stm_get_min_prot_length(nor); > > > > > pow = ilog2(lock_len) - ilog2(min_prot_len) + > > > > > 1; > > > > > val = pow << SR_BP_SHIFT; > > > > > + > > > > > + if (val & BIT(5) && mask & SR_BP3_BIT6) > > > > > + val = (val & ~BIT(5)) | BIT(6); > > > > > > > > .. and the use just the following here: > > > > > > > > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3) > > > > val = (val & ~SR_BP3) | SR_BP3_BIT6; > > > > > > > > Ie. just use the "normal case" where all BP bits are next to > > > > each > > > > other > > > > and then fixup the resulting value and shift the SR3 bit if > > > > necessary. > > > > This will be much easier to read. > > > > > > > > > > Yes, I agree. It would be better to minimize this kind of > > > conversion > > > in > > > one place. > > > > > > > > } > > > > > > > > > > if (val & ~mask) > > > > > @@ -2042,6 +2101,9 @@ static int stm_unlock(struct spi_nor > > > > > *nor, > > > > > loff_t ofs, uint64_t len) > > > > > pow = ilog2(lock_len) - ilog2(min_prot_len) + > > > > > 1; > > > > > val = pow << SR_BP_SHIFT; > > > > > > > > > > + if (val & BIT(5) && mask & SR_BP3_BIT6) > > > > > + val = (val & ~BIT(5)) | BIT(6); > > > > > + > > > > > > > > here would be the other way around: > > > > > > > > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6) > > > > val = (val & ~SR_BP3_BIT6) | SR_BP3; > > > > > > > > > > > > > /* Some power-of-two sizes are not supported */ > > > > > if (val & ~mask) > > > > > return -EINVAL; > > > > > @@ -5244,6 +5306,12 @@ int spi_nor_scan(struct spi_nor *nor, > > > > > const > > > > > char > > > > > *name, > > > > > if (info->flags & USE_CLSR) > > > > > nor->flags |= SNOR_F_USE_CLSR; > > > > > > > > > > + if (info->flags & SPI_NOR_4BIT_BP) { > > > > > + nor->flags |= SNOR_F_HAS_4BIT_BP; > > > > > + 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; > > > > > > > > > > diff --git a/include/linux/mtd/spi-nor.h > > > > > b/include/linux/mtd/spi- > > > > > nor.h > > > > > index de90724f62f1..0190ed21576a 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 > > > > > */ > > > > > > > > IMHO just SR_BP3. but that is a matter of taste. But it is > > > > easier > > > > on > > > > the eye in the mask = SR_BP3 | SR_BP2 etc case. > > > > > > > > > > SR_BP3 would be a more appropriate name if we could set the case > > > with > > > all BP bits next to each other as the "normal case." > > > > > > I'm going to write patches based on latest spi-nor/next including > > > what > > > you mentioned. > > > > > > Thanks, > > > > > > > -michael > > > > > > > > > #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) > > > > > @@ -240,6 +242,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_4BIT_BP = BIT(12), > > > > > + SNOR_F_HAS_SR_BP3_BIT6 = BIT(13), > > > > > > > > > > }; > > > > > > > > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <CGME20200304110835epcas1p3a9daac6383c7ae2fa57a084d4992d5a9@epcas1p3.samsung.com>]
* [PATCH 3/3] mtd: spi-nor: support lock/unlock for a few Micron chips [not found] ` <CGME20200304110835epcas1p3a9daac6383c7ae2fa57a084d4992d5a9@epcas1p3.samsung.com> @ 2020-03-04 11:08 ` Jungseung Lee 0 siblings, 0 replies; 18+ messages in thread From: Jungseung Lee @ 2020-03-04 11:08 UTC (permalink / raw) To: Tudor Ambarus, Vignesh Raghavendra, linux-mtd, js07.lee, js07.lee, chenxiang, Michael Walle Some Micron models are known to have lock/unlock support, and that also support 4bit block protection (bp0-3). This patch support lock/unlock feature on the flashes. Tested on n25q512ax3. The other is modified following the datasheet. Signed-off-by: Jungseung Lee <js07.lee@samsung.com> --- drivers/mtd/spi-nor/spi-nor.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index 31a2106e529a..e8d1fbe4062f 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -2607,12 +2607,17 @@ static const struct flash_info spi_nor_ids[] = { { "mt25ql512a", INFO6(0x20ba20, 0x104400, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) }, - { "n25q512ax3", INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) }, + { "n25q512ax3", INFO(0x20ba20, 0, 64 * 1024, 1024, + SECT_4K | USE_FSR | SPI_NOR_DUAL_READ | + SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | + SPI_NOR_4BIT_BP | SPI_NOR_BP3_SR_BIT6) }, { "mt25qu512a", INFO6(0x20bb20, 0x104400, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) }, - { "n25q512a", INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K | - USE_FSR | SPI_NOR_QUAD_READ) }, + { "n25q512a", INFO(0x20bb20, 0, 64 * 1024, 1024, + SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | + SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | + SPI_NOR_4BIT_BP | SPI_NOR_BP3_SR_BIT6) }, { "n25q00", INFO(0x20ba21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) }, { "n25q00a", INFO(0x20bb21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) }, { "mt25ql02g", INFO(0x20ba22, 0, 64 * 1024, 4096, -- 2.17.1 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 18+ messages in thread
[parent not found: <3b7e6d52-e7e2-c444-1d59-5225a7260ea4@hisilicon.com>]
* Re: [PATCH 1/3] mtd: spi-nor: reimplement block protection handling [not found] ` <3b7e6d52-e7e2-c444-1d59-5225a7260ea4@hisilicon.com> @ 2020-03-07 8:24 ` Jungseung Lee 2020-03-09 7:50 ` chenxiang (M) 0 siblings, 1 reply; 18+ messages in thread From: Jungseung Lee @ 2020-03-07 8:24 UTC (permalink / raw) To: chenxiang (M), Tudor Ambarus, Vignesh Raghavendra, linux-mtd, js07.lee, Michael Walle, js07.lee Cc: John Garry, Linuxarm 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 ?) 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 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/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] mtd: spi-nor: reimplement block protection handling 2020-03-07 8:24 ` [PATCH 1/3] mtd: spi-nor: reimplement block protection handling Jungseung Lee @ 2020-03-09 7:50 ` chenxiang (M) 2020-03-09 11:20 ` Jungseung Lee 2020-03-09 11:44 ` Jungseung Lee 0 siblings, 2 replies; 18+ messages in thread From: chenxiang (M) @ 2020-03-09 7:50 UTC (permalink / raw) To: Jungseung Lee, Tudor Ambarus, Vignesh Raghavendra, linux-mtd, js07.lee, Michael Walle Cc: John Garry, Linuxarm 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/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] mtd: spi-nor: reimplement block protection handling 2020-03-09 7:50 ` chenxiang (M) @ 2020-03-09 11:20 ` Jungseung Lee 2020-03-09 11:44 ` Jungseung Lee 1 sibling, 0 replies; 18+ messages in thread From: Jungseung Lee @ 2020-03-09 11:20 UTC (permalink / raw) To: chenxiang (M), Tudor Ambarus, Vignesh Raghavendra, linux-mtd, js07.lee, Michael Walle Cc: John Garry, Linuxarm Hi, 2020-03-09 (Mon), 15:50 +0800, chenxiang (M): > Hi Jungseung, > > 在 2020/3/7 16:24, Jungseung Lee 写道: > > Hi, > > > > 2020-03-06 (Fri), 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@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)? > 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 locking portion of the flash, lock the some of portion including the request area. UNLOCK - unlock operation return success if the requested area is already unlocked. - unlock operation try to unlock all portions includes the request area. (But user will not knowing well) > > > > 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/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] mtd: spi-nor: reimplement block protection handling 2020-03-09 7:50 ` chenxiang (M) 2020-03-09 11:20 ` Jungseung Lee @ 2020-03-09 11:44 ` Jungseung Lee 2020-03-14 9:58 ` chenxiang (M) 1 sibling, 1 reply; 18+ messages in thread From: Jungseung Lee @ 2020-03-09 11:44 UTC (permalink / raw) To: chenxiang (M), Tudor Ambarus, Vignesh Raghavendra, linux-mtd, js07.lee, Michael Walle Cc: John Garry, Linuxarm, js07.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@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). > I am reviewing tudor's patches and it seems solve your issue effectively. http://lists.infradead.org/pipermail/linux-mtd/2020-March/094231.html > > > > 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. 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@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/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] mtd: spi-nor: reimplement block protection handling 2020-03-09 11:44 ` Jungseung Lee @ 2020-03-14 9:58 ` chenxiang (M) 2020-03-14 13:50 ` Jungseung Lee 0 siblings, 1 reply; 18+ messages in thread From: chenxiang (M) @ 2020-03-14 9:58 UTC (permalink / raw) To: Jungseung Lee, Tudor Ambarus, Vignesh Raghavendra, linux-mtd, js07.lee, Michael Walle Cc: John Garry, Linuxarm 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@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). >> > 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). 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@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/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] mtd: spi-nor: reimplement block protection handling 2020-03-14 9:58 ` chenxiang (M) @ 2020-03-14 13:50 ` Jungseung Lee 2020-03-16 7:21 ` chenxiang (M) 0 siblings, 1 reply; 18+ messages in thread From: Jungseung Lee @ 2020-03-14 13:50 UTC (permalink / raw) To: chenxiang (M) Cc: Vignesh Raghavendra, Tudor Ambarus, John Garry, Linuxarm, Michael Walle, linux-mtd, Jungseung Lee Hi, chenxiang, On Sat, Mar 14, 2020 at 6:58 PM chenxiang (M) <chenxiang66@hisilicon.com> 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@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). > >> > > 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. 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@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/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] mtd: spi-nor: reimplement block protection handling 2020-03-14 13:50 ` Jungseung Lee @ 2020-03-16 7:21 ` chenxiang (M) 2020-03-18 7:17 ` Jungseung Lee 0 siblings, 1 reply; 18+ messages in thread From: chenxiang (M) @ 2020-03-16 7:21 UTC (permalink / raw) To: Jungseung Lee Cc: Vignesh Raghavendra, Tudor Ambarus, John Garry, Linuxarm, Michael Walle, linux-mtd, Jungseung Lee Hi Jungseung, 在 2020/3/14 21:50, Jungseung Lee 写道: > Hi, chenxiang, > > On Sat, Mar 14, 2020 at 6:58 PM chenxiang (M) <chenxiang66@hisilicon.com> 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@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). >>>> >>> 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@hisilicon.com>. 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@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/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] mtd: spi-nor: reimplement block protection handling 2020-03-16 7:21 ` chenxiang (M) @ 2020-03-18 7:17 ` Jungseung Lee 0 siblings, 0 replies; 18+ messages in thread From: Jungseung Lee @ 2020-03-18 7:17 UTC (permalink / raw) To: chenxiang (M), Jungseung Lee Cc: Vignesh Raghavendra, Tudor Ambarus, John Garry, Linuxarm, Michael Walle, linux-mtd, js07.lee Hi, On Mon, 2020-03-16 at 15:21 +0800, chenxiang (M) wrote: > Hi Jungseung, > > 在 2020/3/14 21:50, Jungseung Lee 写道: > > Hi, chenxiang, > > > > On Sat, Mar 14, 2020 at 6:58 PM chenxiang (M) < > > chenxiang66@hisilicon.com> 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://protect2.fireeye.com/url?k=ed8659ca-b0544ec3-ed87d285-0cc47a31cdf8-aa60cbf507f7bb2c&u=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). > > > > > > > > > > > > > I am reviewing tudor's patches and it seems solve your issue > > > > effectively. > > > > https://protect2.fireeye.com/url?k=a6aef5a7-fb7ce2ae-a6af7ee8-0cc47a31cdf8-1b34841aa21abc3e&u=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@hisilicon.com>. > If there would be next version, i will test them also. > Good, I'll post new version by the end of the day. Thanks, > > 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@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/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] mtd: spi-nor: reimplement block protection handling 2020-03-04 11:07 ` [PATCH 1/3] mtd: spi-nor: reimplement block protection handling Jungseung Lee ` (2 preceding siblings ...) [not found] ` <3b7e6d52-e7e2-c444-1d59-5225a7260ea4@hisilicon.com> @ 2020-03-13 15:21 ` Tudor.Ambarus 2020-03-13 17:20 ` Jungseung Lee 3 siblings, 1 reply; 18+ messages in thread From: Tudor.Ambarus @ 2020-03-13 15:21 UTC (permalink / raw) To: js07.lee; +Cc: michael, chenxiang66, linux-mtd, vigneshr, js07.lee Hi, Jungseung, I'm going to respin your patches on top on the what will be the manufacturer move v2 series, https://patchwork.ozlabs.org/cover/1247794/, I want both included for next. On Wednesday, March 4, 2020 1:07:58 PM EET Jungseung Lee wrote: > 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 What corner case, do you refer to EON? Are you aware of other corner cases? We should be more precise, for easier review and understanding. Cheers, ta > 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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] mtd: spi-nor: reimplement block protection handling 2020-03-13 15:21 ` Tudor.Ambarus @ 2020-03-13 17:20 ` Jungseung Lee 0 siblings, 0 replies; 18+ messages in thread From: Jungseung Lee @ 2020-03-13 17:20 UTC (permalink / raw) To: Tudor.Ambarus; +Cc: michael, chenxiang66, linux-mtd, vigneshr, Jungseung Lee Hi, Tudor, On Sat, Mar 14, 2020 at 12:22 AM <Tudor.Ambarus@microchip.com> wrote: > > Hi, Jungseung, > > I'm going to respin your patches on top on the what will be the manufacturer > move v2 series, https://patchwork.ozlabs.org/cover/1247794/, I want both > included for next. > > On Wednesday, March 4, 2020 1:07:58 PM EET Jungseung Lee wrote: > > 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 > > What corner case, do you refer to EON? Are you aware of other corner cases? We > should be more precise, for easier review and understanding. > Yes, that is eon. eon is the only corner case I've ever seen in 3 bit and 4 bit block protection. In 2 bit block protection case, a significant number of flash fully use available bp_slots (2 bit = 4) regardless of the above rule. That is the case with microchip, catalyst and more.. Of course, none of the 2 bit block protection flash has been set to lockable so far. Thanks, > Cheers, > ta > > > 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 > > > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-03-18 7:18 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[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)
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
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).