From: Tudor Ambarus <tudor.ambarus@microchip.com> Hi, Jungseung, Michael, Vignesh, I took Jungseung's v2 and amend it here and there, the description of the changes are in each patch. You'll notice that there are 2 new small patches, the first one is needed so that we start the BP3 support from something that works. The new formula should not change what was before, and silently fix a bug, so the first patch is to address this. The second patch is Michael's suggestion split from Jungseung's patch, for a clearer separation of the fixes. This was just compiled, not (yet) tested on a flash, I'll test it later today. So I need you guys to double check the changes, do some testing, and if all good, let me know. Jungseung Lee (3): mtd: spi-nor: Add new formula for SR block protection handling mtd: spi-nor: Add SR 4bit block protection support mtd: spi-nor: Enable locking for n25q512ax3/n25q512a Tudor Ambarus (2): mtd: spi-nor: Fix gap in SR block protection locking mtd: spi-nor: Set all BP bits to one when lock_len == mtd->size drivers/mtd/spi-nor/core.c | 144 +++++++++++++++++++++----------- drivers/mtd/spi-nor/core.h | 10 +++ drivers/mtd/spi-nor/micron-st.c | 8 +- include/linux/mtd/spi-nor.h | 2 + 4 files changed, 113 insertions(+), 51 deletions(-) -- 2.23.0 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
From: Tudor Ambarus <tudor.ambarus@microchip.com> When there are more BP settings than needed for defining the protected areas of the flash memory, most flashes will define the remaining settings as "protect all", i.e. the equivalent of having all the BP bits set to one. But there are flashes where the in-between BP values are undefined (not mentioned), and only the "all bits set" is protecting the entire memory. One such example is w25q80, where BP[2:0]=0b101 and 0b110 are not defined. Set all the BP bits to one when lock_len == mtd->size, to treat this special case. Suggested-by: Michael Walle <michael@walle.cc> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> --- drivers/mtd/spi-nor/core.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 36660068bc04..3788a95c0a47 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -1660,13 +1660,19 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, uint64_t len) * * pow = ceil(log2(size / len)) = log2(size) - floor(log2(len)) + 1 */ - pow = ilog2(mtd->size) - ilog2(lock_len) + 1; - val = mask - (pow << SR_BP_SHIFT); - if (val & ~mask) - return -EINVAL; - /* Don't "lock" with no region! */ - if (!(val & mask)) - return -EINVAL; + if (lock_len == mtd->size) { + val = mask; + } else { + pow = ilog2(mtd->size) - ilog2(lock_len) + 1; + val = mask - (pow << SR_BP_SHIFT); + + if (val & ~mask) + return -EINVAL; + + /* Don't "lock" with no region! */ + if (!(val & mask)) + return -EINVAL; + } status_new = (status_old & ~mask & ~tb_mask) | val; -- 2.23.0 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
From: Tudor Ambarus <tudor.ambarus@microchip.com> Fix the gap for the SR block protection, the BP bits were set with a +1 value than actually needed. This patch does not change the behavior of the locking operations, just fixes the protected areas. On a 16Mbit flash with 64KByte erase sector, the following changed for the lock operation: Number of blocks | BP2:0 before | BP2:0 now | 1 | 010b | 001b | 2 | 110b | 010b | 3 | 110b | 010b | 4 | 100b | 011b | 5 | 100b | 011b | 6 | 100b | 011b | 7 | 100b | 011b | 8 | 101b | 100b | 9 | 101b | 100b | ... | ... | ... | For the lock operation, if one requests to lock an area that is not matching the upper boundary of a BP protected area, we round down the total length and lock less than the user requested, in order to not lock more than the user actually requested. For the unlock operation, read the number of blocks column as "locked all but number of blocks value". On a 16Mbit flash with 64KByte erase sector, the following changed for the lock operation: Number of blocks | BP2:0 before | BP2:0 now | 1 | 111b | 101b | ... | ... | ... | 15 | 111b | 101b | 16 | 110b | 101b | 17 | 110b | 100b | ... | ... | ... | 24 | 101b | 100b | 25 | 101b | 011b | 26 | 101b | 011b | 27 | 101b | 011b | 28 | 100b | 011b | 29 | 100b | 010b | 30 | 011b | 010b | 31 | 010b | 001b | 32 | 000b | 000b | For the unlock operation, if one requests to unlock an area that is not matching the upper boundary of a BP protected area, we round up the total length and unlock more than the user actually requested. Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> --- drivers/mtd/spi-nor/core.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 877557dbda7f..36660068bc04 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -1654,13 +1654,13 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, uint64_t len) /* * Need smallest pow such that: * - * 1 / (2^pow) <= (len / size) + * 1 / ((2^pow) - 1) <= (len / size) * * so (assuming power-of-2 size) we do: * - * pow = ceil(log2(size / len)) = log2(size) - floor(log2(len)) + * pow = ceil(log2(size / len)) = log2(size) - floor(log2(len)) + 1 */ - pow = ilog2(mtd->size) - ilog2(lock_len); + pow = ilog2(mtd->size) - ilog2(lock_len) + 1; val = mask - (pow << SR_BP_SHIFT); if (val & ~mask) return -EINVAL; @@ -1739,13 +1739,13 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len) /* * Need largest pow such that: * - * 1 / (2^pow) >= (len / size) + * 1 / ((2^pow) - 1) >= (len / size) * * so (assuming power-of-2 size) we do: * - * pow = floor(log2(size / len)) = log2(size) - ceil(log2(len)) + * pow = floor(log2(size / len)) = log2(size) - ceil(log2(len)) + 1 */ - pow = ilog2(mtd->size) - order_base_2(lock_len); + pow = ilog2(mtd->size) - order_base_2(lock_len) + 1; if (lock_len == 0) { val = 0; /* fully unlocked */ } else { -- 2.23.0 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
From: Jungseung Lee <js07.lee@samsung.com> 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 logic to handle block protection based on min_prot_length. It is suitable for the overall flashes with exception of some corner cases (see EON and catalyst) 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> Reviewed-by: Michael Walle <michael@walle.cc> Tested-by: Michael Walle <michael@walle.cc> [ta: - drop spi_nor_get_bp_mask(), spi_nor_get_tb_mask() - rename spi_nor_get_min_prot_length/spi_nor_get_min_prot_length_sr - static u64 spi_nor_get_min_prot_length - unsigned int bp_slots, bp_slots_needed; - bp_slots = (mask >> SR_BP_SHIFT) + 1 - 2; - amend commit description] Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> --- drivers/mtd/spi-nor/core.c | 72 ++++++++++++++++++++++---------------- 1 file changed, 41 insertions(+), 31 deletions(-) diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 3788a95c0a47..c0d186f417d8 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -1514,29 +1514,51 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) return ret; } +static u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor) +{ + unsigned int bp_slots, bp_slots_needed; + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; + + /* Reserved one for "protect none" and one for "protect all". */ + bp_slots = (mask >> SR_BP_SHIFT) + 1 - 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 spi_nor_get_locked_range_sr(struct spi_nor *nor, u8 sr, loff_t *ofs, uint64_t *len) { struct mtd_info *mtd = &nor->mtd; + u64 min_prot_len; u8 mask = SR_BP2 | SR_BP1 | SR_BP0; u8 tb_mask = SR_TB_BIT5; - int pow; + 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 = spi_nor_get_min_prot_length_sr(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; } /* @@ -1609,6 +1631,7 @@ static int spi_nor_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len, static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, uint64_t len) { struct mtd_info *mtd = &nor->mtd; + u64 min_prot_len; int ret, status_old, status_new; u8 mask = SR_BP2 | SR_BP1 | SR_BP0; u8 tb_mask = SR_TB_BIT5; @@ -1651,20 +1674,12 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, uint64_t len) if (nor->flags & SNOR_F_HAS_SR_TB_BIT6) tb_mask = SR_TB_BIT6; - /* - * Need smallest pow such that: - * - * 1 / ((2^pow) - 1) <= (len / size) - * - * so (assuming power-of-2 size) we do: - * - * pow = ceil(log2(size / len)) = log2(size) - floor(log2(len)) + 1 - */ if (lock_len == mtd->size) { val = mask; } else { - pow = ilog2(mtd->size) - ilog2(lock_len) + 1; - val = mask - (pow << SR_BP_SHIFT); + min_prot_len = spi_nor_get_min_prot_length_sr(nor); + pow = ilog2(lock_len) - ilog2(min_prot_len) + 1; + val = pow << SR_BP_SHIFT; if (val & ~mask) return -EINVAL; @@ -1701,6 +1716,7 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, uint64_t len) static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len) { struct mtd_info *mtd = &nor->mtd; + u64 min_prot_len; int ret, status_old, status_new; u8 mask = SR_BP2 | SR_BP1 | SR_BP0; u8 tb_mask = SR_TB_BIT5; @@ -1742,20 +1758,14 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len) if (nor->flags & SNOR_F_HAS_SR_TB_BIT6) tb_mask = SR_TB_BIT6; - /* - * Need largest pow such that: - * - * 1 / ((2^pow) - 1) >= (len / size) - * - * so (assuming power-of-2 size) we do: - * - * pow = floor(log2(size / len)) = log2(size) - ceil(log2(len)) + 1 - */ - pow = ilog2(mtd->size) - order_base_2(lock_len) + 1; + if (lock_len == 0) { val = 0; /* fully unlocked */ } else { - val = mask - (pow << SR_BP_SHIFT); + min_prot_len = spi_nor_get_min_prot_length_sr(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.23.0 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
From: Jungseung Lee <js07.lee@samsung.com> Currently, we are supporting block protection only for flash chips with 3 block protection bits (BP0-2) in the SR register. Enable block protection support for flashes with 4 block protection bits (BP0-3). Add a flash_info flag for flashes that describe 4 block protection bits. Add another flash_info flag for flashes in which BP3 bit is not adjacent to the BP0-2 bits. Signed-off-by: Jungseung Lee <js07.lee@samsung.com> Reviewed-by: Michael Walle <michael@walle.cc> Tested-by: Michael Walle <michael@walle.cc> [ta: - introduce spi_nor_get_sr_bp_mask(), spi_nor_get_sr_tb_mask() - drop Micron n25q512ax3 / BP0-3) boilerplate description - be explicit in spi_nor_get_locked_range_sr aboyt SR_BP3 as Michael suggested, - amend commit description] Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> --- drivers/mtd/spi-nor/core.c | 66 +++++++++++++++++++++++++++---------- drivers/mtd/spi-nor/core.h | 10 ++++++ include/linux/mtd/spi-nor.h | 2 ++ 3 files changed, 60 insertions(+), 18 deletions(-) diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index c0d186f417d8..b70c0b2e0958 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -1514,13 +1514,34 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) return ret; } +static u8 spi_nor_get_sr_bp_mask(struct spi_nor *nor) +{ + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; + + if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6) + return mask | SR_BP3_BIT6; + + if (nor->flags & SNOR_F_HAS_4BIT_BP) + return mask | SR_BP3; + + return mask; +} + +static u8 spi_nor_get_sr_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 u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor) { unsigned int bp_slots, bp_slots_needed; - u8 mask = SR_BP2 | SR_BP1 | SR_BP0; + u8 mask = spi_nor_get_sr_bp_mask(nor); /* Reserved one for "protect none" and one for "protect all". */ - bp_slots = (mask >> SR_BP_SHIFT) + 1 - 2; + bp_slots = (1 << hweight8(mask)) - 2; bp_slots_needed = ilog2(nor->info->n_sectors); if (bp_slots_needed > bp_slots) @@ -1535,12 +1556,14 @@ static void spi_nor_get_locked_range_sr(struct spi_nor *nor, u8 sr, loff_t *ofs, { struct mtd_info *mtd = &nor->mtd; u64 min_prot_len; - u8 mask = SR_BP2 | SR_BP1 | SR_BP0; - u8 tb_mask = SR_TB_BIT5; - u8 bp = (sr & mask) >> SR_BP_SHIFT; + u8 mask = spi_nor_get_sr_bp_mask(nor); + u8 tb_mask = spi_nor_get_sr_tb_mask(nor); + u8 bp, val = sr & mask; - if (nor->flags & SNOR_F_HAS_SR_TB_BIT6) - tb_mask = SR_TB_BIT6; + if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6) + val = (val & ~SR_BP3_BIT6) | SR_BP3; + + bp = val >> SR_BP_SHIFT; if (!bp) { /* No protection */ @@ -1598,7 +1621,8 @@ static int spi_nor_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) @@ -1633,8 +1657,8 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, uint64_t len) struct mtd_info *mtd = &nor->mtd; u64 min_prot_len; int ret, status_old, status_new; - u8 mask = SR_BP2 | SR_BP1 | SR_BP0; - u8 tb_mask = SR_TB_BIT5; + u8 mask = spi_nor_get_sr_bp_mask(nor); + u8 tb_mask = spi_nor_get_sr_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; @@ -1671,9 +1695,6 @@ static int spi_nor_sr_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; } else { @@ -1681,6 +1702,9 @@ static int spi_nor_sr_lock(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 (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3) + val = (val & ~SR_BP3) | SR_BP3_BIT6; + if (val & ~mask) return -EINVAL; @@ -1718,8 +1742,8 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len) struct mtd_info *mtd = &nor->mtd; u64 min_prot_len; int ret, status_old, status_new; - u8 mask = SR_BP2 | SR_BP1 | SR_BP0; - u8 tb_mask = SR_TB_BIT5; + u8 mask = spi_nor_get_sr_bp_mask(nor); + u8 tb_mask = spi_nor_get_sr_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; @@ -1756,9 +1780,6 @@ static int spi_nor_sr_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; - if (lock_len == 0) { val = 0; /* fully unlocked */ } else { @@ -1766,6 +1787,9 @@ static int spi_nor_sr_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 (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3) + val = (val & ~SR_BP3) | SR_BP3_BIT6; + /* Some power-of-two sizes are not supported */ if (val & ~mask) return -EINVAL; @@ -3125,6 +3149,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/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h index 3ce826b35ad1..6f2f6b27173f 100644 --- a/drivers/mtd/spi-nor/core.h +++ b/drivers/mtd/spi-nor/core.h @@ -24,6 +24,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), }; struct spi_nor_read_command { @@ -301,6 +303,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; diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index e656858b50a5..1e2af0ec1f03 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -111,7 +111,9 @@ #define SR_BP0 BIT(2) /* Block protect 0 */ #define SR_BP1 BIT(3) /* Block protect 1 */ #define SR_BP2 BIT(4) /* Block protect 2 */ +#define SR_BP3 BIT(5) /* Block protect 3 */ #define SR_TB_BIT5 BIT(5) /* Top/Bottom protect */ +#define SR_BP3_BIT6 BIT(6) /* Block protect 3 */ #define SR_TB_BIT6 BIT(6) /* Top/Bottom protect */ #define SR_SRWD BIT(7) /* SR write protect */ /* Spansion/Cypress specific status bits */ -- 2.23.0 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
From: Jungseung Lee <js07.lee@samsung.com> Currently, we are supporting block protection only for flash chips with 3 block protection bits (BP0-2) in the SR register. Enable block protection support for flashes with 4 block protection bits (BP0-3). Add a flash_info flag for flashes that describe 4 block protection bits. Add another flash_info flag for flashes in which BP3 bit is not adjacent to the BP0-2 bits. Signed-off-by: Jungseung Lee <js07.lee@samsung.com> Reviewed-by: Michael Walle <michael@walle.cc> Tested-by: Michael Walle <michael@walle.cc> [ta: - introduce spi_nor_get_sr_bp_mask(), spi_nor_get_sr_tb_mask() - drop Micron n25q512ax3 / BP0-3) boilerplate description - be explicit in spi_nor_get_locked_range_sr aboyt SR_BP3 as Michael suggested, - amend commit description] Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> --- drivers/mtd/spi-nor/core.c | 66 +++++++++++++++++++++++++++---------- drivers/mtd/spi-nor/core.h | 10 ++++++ include/linux/mtd/spi-nor.h | 2 ++ 3 files changed, 60 insertions(+), 18 deletions(-) diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index c0d186f417d8..b70c0b2e0958 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -1514,13 +1514,34 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) return ret; } +static u8 spi_nor_get_sr_bp_mask(struct spi_nor *nor) +{ + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; + + if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6) + return mask | SR_BP3_BIT6; + + if (nor->flags & SNOR_F_HAS_4BIT_BP) + return mask | SR_BP3; + + return mask; +} + +static u8 spi_nor_get_sr_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 u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor) { unsigned int bp_slots, bp_slots_needed; - u8 mask = SR_BP2 | SR_BP1 | SR_BP0; + u8 mask = spi_nor_get_sr_bp_mask(nor); /* Reserved one for "protect none" and one for "protect all". */ - bp_slots = (mask >> SR_BP_SHIFT) + 1 - 2; + bp_slots = (1 << hweight8(mask)) - 2; bp_slots_needed = ilog2(nor->info->n_sectors); if (bp_slots_needed > bp_slots) @@ -1535,12 +1556,14 @@ static void spi_nor_get_locked_range_sr(struct spi_nor *nor, u8 sr, loff_t *ofs, { struct mtd_info *mtd = &nor->mtd; u64 min_prot_len; - u8 mask = SR_BP2 | SR_BP1 | SR_BP0; - u8 tb_mask = SR_TB_BIT5; - u8 bp = (sr & mask) >> SR_BP_SHIFT; + u8 mask = spi_nor_get_sr_bp_mask(nor); + u8 tb_mask = spi_nor_get_sr_tb_mask(nor); + u8 bp, val = sr & mask; - if (nor->flags & SNOR_F_HAS_SR_TB_BIT6) - tb_mask = SR_TB_BIT6; + if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6) + val = (val & ~SR_BP3_BIT6) | SR_BP3; + + bp = val >> SR_BP_SHIFT; if (!bp) { /* No protection */ @@ -1598,7 +1621,8 @@ static int spi_nor_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) @@ -1633,8 +1657,8 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, uint64_t len) struct mtd_info *mtd = &nor->mtd; u64 min_prot_len; int ret, status_old, status_new; - u8 mask = SR_BP2 | SR_BP1 | SR_BP0; - u8 tb_mask = SR_TB_BIT5; + u8 mask = spi_nor_get_sr_bp_mask(nor); + u8 tb_mask = spi_nor_get_sr_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; @@ -1671,9 +1695,6 @@ static int spi_nor_sr_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; } else { @@ -1681,6 +1702,9 @@ static int spi_nor_sr_lock(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 (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3) + val = (val & ~SR_BP3) | SR_BP3_BIT6; + if (val & ~mask) return -EINVAL; @@ -1718,8 +1742,8 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len) struct mtd_info *mtd = &nor->mtd; u64 min_prot_len; int ret, status_old, status_new; - u8 mask = SR_BP2 | SR_BP1 | SR_BP0; - u8 tb_mask = SR_TB_BIT5; + u8 mask = spi_nor_get_sr_bp_mask(nor); + u8 tb_mask = spi_nor_get_sr_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; @@ -1756,9 +1780,6 @@ static int spi_nor_sr_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; - if (lock_len == 0) { val = 0; /* fully unlocked */ } else { @@ -1766,6 +1787,9 @@ static int spi_nor_sr_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 (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3) + val = (val & ~SR_BP3) | SR_BP3_BIT6; + /* Some power-of-two sizes are not supported */ if (val & ~mask) return -EINVAL; @@ -3125,6 +3149,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/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h index 3ce826b35ad1..6f2f6b27173f 100644 --- a/drivers/mtd/spi-nor/core.h +++ b/drivers/mtd/spi-nor/core.h @@ -24,6 +24,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), }; struct spi_nor_read_command { @@ -301,6 +303,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; diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index e656858b50a5..1e2af0ec1f03 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -111,7 +111,9 @@ #define SR_BP0 BIT(2) /* Block protect 0 */ #define SR_BP1 BIT(3) /* Block protect 1 */ #define SR_BP2 BIT(4) /* Block protect 2 */ +#define SR_BP3 BIT(5) /* Block protect 3 */ #define SR_TB_BIT5 BIT(5) /* Top/Bottom protect */ +#define SR_BP3_BIT6 BIT(6) /* Block protect 3 */ #define SR_TB_BIT6 BIT(6) /* Top/Bottom protect */ #define SR_SRWD BIT(7) /* SR write protect */ /* Spansion/Cypress specific status bits */ -- 2.23.0 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
From: Jungseung Lee <js07.lee@samsung.com> 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> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> --- drivers/mtd/spi-nor/micron-st.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c index 3874a62d8b47..6c034b9718e2 100644 --- a/drivers/mtd/spi-nor/micron-st.c +++ b/drivers/mtd/spi-nor/micron-st.c @@ -47,12 +47,16 @@ static const struct flash_info st_parts[] = { 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) }, + 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) }, { "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) }, + 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) }, -- 2.23.0 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Please ignore this patch, it duplicates " [PATCH v3 4/5] mtd: spi-nor: Add SR 4bit block protection support" ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Hi, On Mon, 2020-03-23 at 09:24 +0000, Tudor.Ambarus@microchip.com wrote: > From: Jungseung Lee <js07.lee@samsung.com> > > Currently, we are supporting block protection only for flash chips > with > 3 block protection bits (BP0-2) in the SR register. > > Enable block protection support for flashes with 4 block protection > bits > (BP0-3). > > Add a flash_info flag for flashes that describe 4 block protection > bits. > Add another flash_info flag for flashes in which BP3 bit is not > adjacent > to the BP0-2 bits. > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com> > Reviewed-by: Michael Walle <michael@walle.cc> > Tested-by: Michael Walle <michael@walle.cc> > [ta: > - introduce spi_nor_get_sr_bp_mask(), spi_nor_get_sr_tb_mask() > - drop Micron n25q512ax3 / BP0-3) boilerplate description > - be explicit in spi_nor_get_locked_range_sr aboyt SR_BP3 as Michael > suggested, > - amend commit description] > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/mtd/spi-nor/core.c | 66 +++++++++++++++++++++++++++------ > ---- > drivers/mtd/spi-nor/core.h | 10 ++++++ > include/linux/mtd/spi-nor.h | 2 ++ > 3 files changed, 60 insertions(+), 18 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index c0d186f417d8..b70c0b2e0958 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -1514,13 +1514,34 @@ static int spi_nor_erase(struct mtd_info > *mtd, struct erase_info *instr) > return ret; > } > > +static u8 spi_nor_get_sr_bp_mask(struct spi_nor *nor) > +{ > + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > + > + if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6) > + return mask | SR_BP3_BIT6; > + > + if (nor->flags & SNOR_F_HAS_4BIT_BP) > + return mask | SR_BP3; > + > + return mask; > +} > + > +static u8 spi_nor_get_sr_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 u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor) > { > unsigned int bp_slots, bp_slots_needed; > - u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > + u8 mask = spi_nor_get_sr_bp_mask(nor); > > /* Reserved one for "protect none" and one for "protect all". > */ > - bp_slots = (mask >> SR_BP_SHIFT) + 1 - 2; > + bp_slots = (1 << hweight8(mask)) - 2; > bp_slots_needed = ilog2(nor->info->n_sectors); > > if (bp_slots_needed > bp_slots) > @@ -1535,12 +1556,14 @@ static void > spi_nor_get_locked_range_sr(struct spi_nor *nor, u8 sr, loff_t *ofs, > { > struct mtd_info *mtd = &nor->mtd; > u64 min_prot_len; > - u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > - u8 tb_mask = SR_TB_BIT5; > - u8 bp = (sr & mask) >> SR_BP_SHIFT; > + u8 mask = spi_nor_get_sr_bp_mask(nor); > + u8 tb_mask = spi_nor_get_sr_tb_mask(nor); > + u8 bp, val = sr & mask; > > - if (nor->flags & SNOR_F_HAS_SR_TB_BIT6) > - tb_mask = SR_TB_BIT6; > + if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6) > + val = (val & ~SR_BP3_BIT6) | SR_BP3; > + > + bp = val >> SR_BP_SHIFT; > > if (!bp) { > /* No protection */ > @@ -1598,7 +1621,8 @@ static int spi_nor_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) > @@ -1633,8 +1657,8 @@ static int spi_nor_sr_lock(struct spi_nor *nor, > loff_t ofs, uint64_t len) > struct mtd_info *mtd = &nor->mtd; > u64 min_prot_len; > int ret, status_old, status_new; > - u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > - u8 tb_mask = SR_TB_BIT5; > + u8 mask = spi_nor_get_sr_bp_mask(nor); > + u8 tb_mask = spi_nor_get_sr_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; > @@ -1671,9 +1695,6 @@ static int spi_nor_sr_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; > } else { > @@ -1681,6 +1702,9 @@ static int spi_nor_sr_lock(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 (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & > SR_BP3) > + val = (val & ~SR_BP3) | SR_BP3_BIT6; > + > if (val & ~mask) > return -EINVAL; > > @@ -1718,8 +1742,8 @@ static int spi_nor_sr_unlock(struct spi_nor > *nor, loff_t ofs, uint64_t len) > struct mtd_info *mtd = &nor->mtd; > u64 min_prot_len; > int ret, status_old, status_new; > - u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > - u8 tb_mask = SR_TB_BIT5; > + u8 mask = spi_nor_get_sr_bp_mask(nor); > + u8 tb_mask = spi_nor_get_sr_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; > @@ -1756,9 +1780,6 @@ static int spi_nor_sr_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; > - > if (lock_len == 0) { > val = 0; /* fully unlocked */ > } else { > @@ -1766,6 +1787,9 @@ static int spi_nor_sr_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 (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & > SR_BP3) > + val = (val & ~SR_BP3) | SR_BP3_BIT6; > + > /* Some power-of-two sizes are not supported */ > if (val & ~mask) > return -EINVAL; > @@ -3125,6 +3149,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/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h > index 3ce826b35ad1..6f2f6b27173f 100644 > --- a/drivers/mtd/spi-nor/core.h > +++ b/drivers/mtd/spi-nor/core.h > @@ -24,6 +24,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), > }; > > struct spi_nor_read_command { > @@ -301,6 +303,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; > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi- > nor.h > index e656858b50a5..1e2af0ec1f03 100644 > --- a/include/linux/mtd/spi-nor.h > +++ b/include/linux/mtd/spi-nor.h > @@ -111,7 +111,9 @@ > #define SR_BP0 BIT(2) /* Block protect 0 */ > #define SR_BP1 BIT(3) /* Block protect 1 */ > #define SR_BP2 BIT(4) /* Block protect 2 */ > +#define SR_BP3 BIT(5) /* Block protect 3 */ > #define SR_TB_BIT5 BIT(5) /* Top/Bottom protect */ > +#define SR_BP3_BIT6 BIT(6) /* Block protect 3 */ > #define SR_TB_BIT6 BIT(6) /* Top/Bottom protect */ > #define SR_SRWD BIT(7) /* SR write protect > */ > /* Spansion/Cypress specific status bits */ Tested with a n25q512ax3 (bp0-3) and w25q128 (bp0-2). It passed all of my test cases. Tested-by: Jungseung Lee <js07.lee@samsung.com> ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Monday, March 23, 2020 2:43:09 PM EET Jungseung Lee wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > Hi, Hi, Jungseung, > > On Mon, 2020-03-23 at 09:24 +0000, Tudor.Ambarus@microchip.com wrote: > > From: Jungseung Lee <js07.lee@samsung.com> > > > > Currently, we are supporting block protection only for flash chips > > with > > 3 block protection bits (BP0-2) in the SR register. > > > > Enable block protection support for flashes with 4 block protection > > bits > > (BP0-3). > > > > Add a flash_info flag for flashes that describe 4 block protection > > bits. > > Add another flash_info flag for flashes in which BP3 bit is not > > adjacent > > to the BP0-2 bits. > > > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com> > > Reviewed-by: Michael Walle <michael@walle.cc> > > Tested-by: Michael Walle <michael@walle.cc> > > [ta: > > - introduce spi_nor_get_sr_bp_mask(), spi_nor_get_sr_tb_mask() from the previous patch set > > - drop Micron n25q512ax3 / BP0-3) boilerplate description > > - be explicit in spi_nor_get_locked_range_sr aboyt SR_BP3 as Michael > > suggested, > > - amend commit description] I'll drop these comments when/if applying. Let me know if you're ok with the changes that I did. Please do the same for patches 3/5 and 5/5. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > > --- > > > > drivers/mtd/spi-nor/core.c | 66 +++++++++++++++++++++++++++------ > > > > ---- > > > > drivers/mtd/spi-nor/core.h | 10 ++++++ > > include/linux/mtd/spi-nor.h | 2 ++ > > 3 files changed, 60 insertions(+), 18 deletions(-) cut > Tested with a n25q512ax3 (bp0-3) and w25q128 (bp0-2). > It passed all of my test cases. > > Tested-by: Jungseung Lee <js07.lee@samsung.com> Great! Since you are the author of the patch, it's not necessary to add your T-b tag, but I'll amend the commit description to say that it was tested on n25q512ax3 (bp0-3) and w25q128 (bp0-2). Also, would you please review patches 1 and 2 from the series? Cheers, ta ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Hi, On Mon, 2020-03-23 at 12:55 +0000, Tudor.Ambarus@microchip.com wrote: > On Monday, March 23, 2020 2:43:09 PM EET Jungseung Lee wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > know the > > content is safe > > > > Hi, > > Hi, Jungseung, > > > > On Mon, 2020-03-23 at 09:24 +0000, Tudor.Ambarus@microchip.com > > wrote: > > > From: Jungseung Lee <js07.lee@samsung.com> > > > > > > Currently, we are supporting block protection only for flash > > > chips > > > with > > > 3 block protection bits (BP0-2) in the SR register. > > > > > > Enable block protection support for flashes with 4 block > > > protection > > > bits > > > (BP0-3). > > > > > > Add a flash_info flag for flashes that describe 4 block > > > protection > > > bits. > > > Add another flash_info flag for flashes in which BP3 bit is not > > > adjacent > > > to the BP0-2 bits. > > > > > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com> > > > Reviewed-by: Michael Walle <michael@walle.cc> > > > Tested-by: Michael Walle <michael@walle.cc> > > > [ta: > > > - introduce spi_nor_get_sr_bp_mask(), spi_nor_get_sr_tb_mask() > > from the previous patch set > > > - drop Micron n25q512ax3 / BP0-3) boilerplate description > > > - be explicit in spi_nor_get_locked_range_sr aboyt SR_BP3 as > > > Michael > > > suggested, > > > - amend commit description] > > I'll drop these comments when/if applying. Let me know if you're ok > with the > changes that I did. Please do the same for patches 3/5 and 5/5. > It looks much better. All the parts you modified are those that I thought were ambiguous. now it's ok. Thanks, > > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > > > --- > > > > > > drivers/mtd/spi-nor/core.c | 66 +++++++++++++++++++++++++++-- > > > ---- > > > > > > ---- > > > > > > drivers/mtd/spi-nor/core.h | 10 ++++++ > > > include/linux/mtd/spi-nor.h | 2 ++ > > > 3 files changed, 60 insertions(+), 18 deletions(-) > > cut > > > Tested with a n25q512ax3 (bp0-3) and w25q128 (bp0-2). > > It passed all of my test cases. > > > > Tested-by: Jungseung Lee <js07.lee@samsung.com> > > Great! Since you are the author of the patch, it's not necessary to > add your > T-b tag, but I'll amend the commit description to say that it was > tested on > n25q512ax3 (bp0-3) and w25q128 (bp0-2). > > Also, would you please review patches 1 and 2 from the series? > > Cheers, > ta > > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
> From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com> > Sent: Monday, March 23, 2020 6:25 PM > To: js07.lee@samsung.com; michael@walle.cc; vigneshr@ti.com > Cc: linux-mtd@lists.infradead.org; Tudor.Ambarus@microchip.com > Subject: [PATCH v3 3/5] mtd: spi-nor: Add new formula for SR block > protection handling > > From: Jungseung Lee <js07.lee@samsung.com> > > 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 logic to handle block protection based on > min_prot_length. > It is suitable for the overall flashes with exception of some corner > cases > (see EON and catalyst) and easy to extend and apply for the case of > 2bit or > 4bit block protection. > > [1] https://protect2.fireeye.com/url?k=d62c9c1b-8bf82073-d62d1754- > 0cc47a3356b2-012ef3655070329a&u= > http://lists.infradead.org/pipermail/linux- > mtd/2020-February/093934.html > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com> > Reviewed-by: Michael Walle <michael@walle.cc> > Tested-by: Michael Walle <michael@walle.cc> > [ta: - drop spi_nor_get_bp_mask(), spi_nor_get_tb_mask() > - rename spi_nor_get_min_prot_length/spi_nor_get_min_prot_length_sr > - static u64 spi_nor_get_min_prot_length > - unsigned int bp_slots, bp_slots_needed; > - bp_slots = (mask >> SR_BP_SHIFT) + 1 - 2; > - amend commit description] All looks good and it's ok for me. Thanks, > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/mtd/spi-nor/core.c | 72 ++++++++++++++++++++++------------ > ---- > 1 file changed, 41 insertions(+), 31 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 3788a95c0a47..c0d186f417d8 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -1514,29 +1514,51 @@ static int spi_nor_erase(struct mtd_info > *mtd, > struct erase_info *instr) > return ret; > } > > +static u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor) > +{ > + unsigned int bp_slots, bp_slots_needed; > + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > + > + /* Reserved one for "protect none" and one for "protect all". > */ > + bp_slots = (mask >> SR_BP_SHIFT) + 1 - 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 spi_nor_get_locked_range_sr(struct spi_nor *nor, u8 sr, > loff_t > *ofs, > uint64_t *len) > { > struct mtd_info *mtd = &nor->mtd; > + u64 min_prot_len; > u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > u8 tb_mask = SR_TB_BIT5; > - int pow; > + 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 = spi_nor_get_min_prot_length_sr(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; > } > > /* > @@ -1609,6 +1631,7 @@ static int spi_nor_is_unlocked_sr(struct > spi_nor > *nor, loff_t ofs, uint64_t len, > static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, uint64_t > len) > { > struct mtd_info *mtd = &nor->mtd; > + u64 min_prot_len; > int ret, status_old, status_new; > u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > u8 tb_mask = SR_TB_BIT5; > @@ -1651,20 +1674,12 @@ static int spi_nor_sr_lock(struct spi_nor > *nor, > loff_t ofs, uint64_t len) > if (nor->flags & SNOR_F_HAS_SR_TB_BIT6) > tb_mask = SR_TB_BIT6; > > - /* > - * Need smallest pow such that: > - * > - * 1 / ((2^pow) - 1) <= (len / size) > - * > - * so (assuming power-of-2 size) we do: > - * > - * pow = ceil(log2(size / len)) = log2(size) - > floor(log2(len)) + > 1 > - */ > if (lock_len == mtd->size) { > val = mask; > } else { > - pow = ilog2(mtd->size) - ilog2(lock_len) + 1; > - val = mask - (pow << SR_BP_SHIFT); > + min_prot_len = spi_nor_get_min_prot_length_sr(nor); > + pow = ilog2(lock_len) - ilog2(min_prot_len) + 1; > + val = pow << SR_BP_SHIFT; > > if (val & ~mask) > return -EINVAL; > @@ -1701,6 +1716,7 @@ static int spi_nor_sr_lock(struct spi_nor *nor, > loff_t ofs, uint64_t len) > static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, > uint64_t len) > { > struct mtd_info *mtd = &nor->mtd; > + u64 min_prot_len; > int ret, status_old, status_new; > u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > u8 tb_mask = SR_TB_BIT5; > @@ -1742,20 +1758,14 @@ static int spi_nor_sr_unlock(struct spi_nor > *nor, > loff_t ofs, uint64_t len) > > if (nor->flags & SNOR_F_HAS_SR_TB_BIT6) > tb_mask = SR_TB_BIT6; > - /* > - * Need largest pow such that: > - * > - * 1 / ((2^pow) - 1) >= (len / size) > - * > - * so (assuming power-of-2 size) we do: > - * > - * pow = floor(log2(size / len)) = log2(size) - > ceil(log2(len)) + > 1 > - */ > - pow = ilog2(mtd->size) - order_base_2(lock_len) + 1; > + > if (lock_len == 0) { > val = 0; /* fully unlocked */ > } else { > - val = mask - (pow << SR_BP_SHIFT); > + min_prot_len = spi_nor_get_min_prot_length_sr(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/
On Mon, 2020-03-23 at 09:24 +0000, Tudor.Ambarus@microchip.com wrote: > From: Tudor Ambarus <tudor.ambarus@microchip.com> > > When there are more BP settings than needed for defining the > protected > areas of the flash memory, most flashes will define the remaining > settings as "protect all", i.e. the equivalent of having all the BP > bits > set to one. But there are flashes where the in-between BP values > are undefined (not mentioned), and only the "all bits set" is > protecting > the entire memory. One such example is w25q80, where BP[2:0]=0b101 > and > 0b110 are not defined. > > Set all the BP bits to one when lock_len == mtd->size, to treat this > special case. > > Suggested-by: Michael Walle <michael@walle.cc> > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/mtd/spi-nor/core.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 36660068bc04..3788a95c0a47 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -1660,13 +1660,19 @@ static int spi_nor_sr_lock(struct spi_nor > *nor, loff_t ofs, uint64_t len) > * > * pow = ceil(log2(size / len)) = log2(size) - > floor(log2(len)) + 1 > */ > - pow = ilog2(mtd->size) - ilog2(lock_len) + 1; > - val = mask - (pow << SR_BP_SHIFT); > - if (val & ~mask) > - return -EINVAL; > - /* Don't "lock" with no region! */ > - if (!(val & mask)) > - return -EINVAL; > + if (lock_len == mtd->size) { > + val = mask; > + } else { > + pow = ilog2(mtd->size) - ilog2(lock_len) + 1; > + val = mask - (pow << SR_BP_SHIFT); > + > + if (val & ~mask) > + return -EINVAL; > + > + /* Don't "lock" with no region! */ > + if (!(val & mask)) > + return -EINVAL; > + } > > status_new = (status_old & ~mask & ~tb_mask) | val; > Reviewed-by: Jungseung Lee <js07.lee@samsung.com> ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Hi, Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com: > From: Tudor Ambarus <tudor.ambarus@microchip.com> > > Fix the gap for the SR block protection, the BP bits were set with > a +1 value than actually needed. This patch does not change the > behavior of the locking operations, just fixes the protected areas. So instead of rounding up, it does round down now? > > On a 16Mbit flash with 64KByte erase sector, the following changed > for the lock operation: > > Number of blocks | BP2:0 before | BP2:0 now | > 1 | 010b | 001b | > 2 | 110b | 010b | > 3 | 110b | 010b | > 4 | 100b | 011b | > 5 | 100b | 011b | > 6 | 100b | 011b | > 7 | 100b | 011b | > 8 | 101b | 100b | > 9 | 101b | 100b | > ... | ... | ... | > > For the lock operation, if one requests to lock an area that is not > matching the upper boundary of a BP protected area, we round down > the total length and lock less than the user requested, in order to > not lock more than the user actually requested. I don't know if that is really what a user really want. Because you'd end up with regions which the user believes are locked but are not. IMHO if you'd have to make a choice I'd prefer to have the remainder locked. Not the other way around. Esp. since the user explicitly express to have a region locked. -michael > For the unlock operation, read the number of blocks column as > "locked all but number of blocks value". On a 16Mbit flash with > 64KByte erase sector, the following changed for the lock operation: > > Number of blocks | BP2:0 before | BP2:0 now | > 1 | 111b | 101b | > ... | ... | ... | > 15 | 111b | 101b | > 16 | 110b | 101b | > 17 | 110b | 100b | > ... | ... | ... | > 24 | 101b | 100b | > 25 | 101b | 011b | > 26 | 101b | 011b | > 27 | 101b | 011b | > 28 | 100b | 011b | > 29 | 100b | 010b | > 30 | 011b | 010b | > 31 | 010b | 001b | > 32 | 000b | 000b | > > For the unlock operation, if one requests to unlock an area that is > not matching the upper boundary of a BP protected area, we round up > the total length and unlock more than the user actually requested. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/mtd/spi-nor/core.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 877557dbda7f..36660068bc04 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -1654,13 +1654,13 @@ static int spi_nor_sr_lock(struct spi_nor > *nor, loff_t ofs, uint64_t len) > /* > * Need smallest pow such that: > * > - * 1 / (2^pow) <= (len / size) > + * 1 / ((2^pow) - 1) <= (len / size) > * > * so (assuming power-of-2 size) we do: > * > - * pow = ceil(log2(size / len)) = log2(size) - floor(log2(len)) > + * pow = ceil(log2(size / len)) = log2(size) - floor(log2(len)) + 1 > */ > - pow = ilog2(mtd->size) - ilog2(lock_len); > + pow = ilog2(mtd->size) - ilog2(lock_len) + 1; > val = mask - (pow << SR_BP_SHIFT); > if (val & ~mask) > return -EINVAL; > @@ -1739,13 +1739,13 @@ static int spi_nor_sr_unlock(struct spi_nor > *nor, loff_t ofs, uint64_t len) > /* > * Need largest pow such that: > * > - * 1 / (2^pow) >= (len / size) > + * 1 / ((2^pow) - 1) >= (len / size) > * > * so (assuming power-of-2 size) we do: > * > - * pow = floor(log2(size / len)) = log2(size) - ceil(log2(len)) > + * pow = floor(log2(size / len)) = log2(size) - ceil(log2(len)) + 1 > */ > - pow = ilog2(mtd->size) - order_base_2(lock_len); > + pow = ilog2(mtd->size) - order_base_2(lock_len) + 1; > if (lock_len == 0) { > val = 0; /* fully unlocked */ > } else { ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com: > From: Tudor Ambarus <tudor.ambarus@microchip.com> > > When there are more BP settings than needed for defining the protected > areas of the flash memory, most flashes will define the remaining > settings as "protect all", i.e. the equivalent of having all the BP > bits > set to one. But there are flashes where the in-between BP values > are undefined (not mentioned), and only the "all bits set" is > protecting > the entire memory. One such example is w25q80, where BP[2:0]=0b101 and > 0b110 are not defined. > > Set all the BP bits to one when lock_len == mtd->size, to treat this > special case. > > Suggested-by: Michael Walle <michael@walle.cc> > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> Reviewed-by: Michael Walle <michael@walle.cc> > --- > drivers/mtd/spi-nor/core.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 36660068bc04..3788a95c0a47 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -1660,13 +1660,19 @@ static int spi_nor_sr_lock(struct spi_nor > *nor, loff_t ofs, uint64_t len) > * > * pow = ceil(log2(size / len)) = log2(size) - floor(log2(len)) + 1 > */ > - pow = ilog2(mtd->size) - ilog2(lock_len) + 1; > - val = mask - (pow << SR_BP_SHIFT); > - if (val & ~mask) > - return -EINVAL; > - /* Don't "lock" with no region! */ > - if (!(val & mask)) > - return -EINVAL; > + if (lock_len == mtd->size) { > + val = mask; > + } else { > + pow = ilog2(mtd->size) - ilog2(lock_len) + 1; > + val = mask - (pow << SR_BP_SHIFT); > + > + if (val & ~mask) > + return -EINVAL; > + > + /* Don't "lock" with no region! */ > + if (!(val & mask)) > + return -EINVAL; > + } > > status_new = (status_old & ~mask & ~tb_mask) | val; ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com: > From: Jungseung Lee <js07.lee@samsung.com> > > Currently, we are supporting block protection only for flash chips with > 3 block protection bits (BP0-2) in the SR register. > > Enable block protection support for flashes with 4 block protection > bits > (BP0-3). > > Add a flash_info flag for flashes that describe 4 block protection > bits. > Add another flash_info flag for flashes in which BP3 bit is not > adjacent > to the BP0-2 bits. > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com> > Reviewed-by: Michael Walle <michael@walle.cc> > Tested-by: Michael Walle <michael@walle.cc> > [ta: > - introduce spi_nor_get_sr_bp_mask(), spi_nor_get_sr_tb_mask() > - drop Micron n25q512ax3 / BP0-3) boilerplate description that was actually a comment on my side some time ago. Because the current example isn't really good and lacks the second case (which is added by this patch). -michael > - be explicit in spi_nor_get_locked_range_sr aboyt SR_BP3 as Michael > suggested, > - amend commit description] > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/mtd/spi-nor/core.c | 66 +++++++++++++++++++++++++++---------- > drivers/mtd/spi-nor/core.h | 10 ++++++ > include/linux/mtd/spi-nor.h | 2 ++ > 3 files changed, 60 insertions(+), 18 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index c0d186f417d8..b70c0b2e0958 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -1514,13 +1514,34 @@ static int spi_nor_erase(struct mtd_info *mtd, > struct erase_info *instr) > return ret; > } > > +static u8 spi_nor_get_sr_bp_mask(struct spi_nor *nor) > +{ > + u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > + > + if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6) > + return mask | SR_BP3_BIT6; > + > + if (nor->flags & SNOR_F_HAS_4BIT_BP) > + return mask | SR_BP3; > + > + return mask; > +} > + > +static u8 spi_nor_get_sr_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 u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor) > { > unsigned int bp_slots, bp_slots_needed; > - u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > + u8 mask = spi_nor_get_sr_bp_mask(nor); > > /* Reserved one for "protect none" and one for "protect all". */ > - bp_slots = (mask >> SR_BP_SHIFT) + 1 - 2; > + bp_slots = (1 << hweight8(mask)) - 2; > bp_slots_needed = ilog2(nor->info->n_sectors); > > if (bp_slots_needed > bp_slots) > @@ -1535,12 +1556,14 @@ static void spi_nor_get_locked_range_sr(struct > spi_nor *nor, u8 sr, loff_t *ofs, > { > struct mtd_info *mtd = &nor->mtd; > u64 min_prot_len; > - u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > - u8 tb_mask = SR_TB_BIT5; > - u8 bp = (sr & mask) >> SR_BP_SHIFT; > + u8 mask = spi_nor_get_sr_bp_mask(nor); > + u8 tb_mask = spi_nor_get_sr_tb_mask(nor); > + u8 bp, val = sr & mask; > > - if (nor->flags & SNOR_F_HAS_SR_TB_BIT6) > - tb_mask = SR_TB_BIT6; > + if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6) > + val = (val & ~SR_BP3_BIT6) | SR_BP3; > + > + bp = val >> SR_BP_SHIFT; > > if (!bp) { > /* No protection */ > @@ -1598,7 +1621,8 @@ static int spi_nor_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) > @@ -1633,8 +1657,8 @@ static int spi_nor_sr_lock(struct spi_nor *nor, > loff_t ofs, uint64_t len) > struct mtd_info *mtd = &nor->mtd; > u64 min_prot_len; > int ret, status_old, status_new; > - u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > - u8 tb_mask = SR_TB_BIT5; > + u8 mask = spi_nor_get_sr_bp_mask(nor); > + u8 tb_mask = spi_nor_get_sr_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; > @@ -1671,9 +1695,6 @@ static int spi_nor_sr_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; > } else { > @@ -1681,6 +1702,9 @@ static int spi_nor_sr_lock(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 (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3) > + val = (val & ~SR_BP3) | SR_BP3_BIT6; > + > if (val & ~mask) > return -EINVAL; > > @@ -1718,8 +1742,8 @@ static int spi_nor_sr_unlock(struct spi_nor > *nor, loff_t ofs, uint64_t len) > struct mtd_info *mtd = &nor->mtd; > u64 min_prot_len; > int ret, status_old, status_new; > - u8 mask = SR_BP2 | SR_BP1 | SR_BP0; > - u8 tb_mask = SR_TB_BIT5; > + u8 mask = spi_nor_get_sr_bp_mask(nor); > + u8 tb_mask = spi_nor_get_sr_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; > @@ -1756,9 +1780,6 @@ static int spi_nor_sr_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; > - > if (lock_len == 0) { > val = 0; /* fully unlocked */ > } else { > @@ -1766,6 +1787,9 @@ static int spi_nor_sr_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 (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3) > + val = (val & ~SR_BP3) | SR_BP3_BIT6; > + > /* Some power-of-two sizes are not supported */ > if (val & ~mask) > return -EINVAL; > @@ -3125,6 +3149,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/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h > index 3ce826b35ad1..6f2f6b27173f 100644 > --- a/drivers/mtd/spi-nor/core.h > +++ b/drivers/mtd/spi-nor/core.h > @@ -24,6 +24,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), > }; > > struct spi_nor_read_command { > @@ -301,6 +303,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; > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > index e656858b50a5..1e2af0ec1f03 100644 > --- a/include/linux/mtd/spi-nor.h > +++ b/include/linux/mtd/spi-nor.h > @@ -111,7 +111,9 @@ > #define SR_BP0 BIT(2) /* Block protect 0 */ > #define SR_BP1 BIT(3) /* Block protect 1 */ > #define SR_BP2 BIT(4) /* Block protect 2 */ > +#define SR_BP3 BIT(5) /* Block protect 3 */ > #define SR_TB_BIT5 BIT(5) /* Top/Bottom protect */ > +#define SR_BP3_BIT6 BIT(6) /* Block protect 3 */ > #define SR_TB_BIT6 BIT(6) /* Top/Bottom protect */ > #define SR_SRWD BIT(7) /* SR write protect */ > /* Spansion/Cypress specific status bits */ ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Monday, March 23, 2020 8:33:19 PM EET Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com: > > From: Jungseung Lee <js07.lee@samsung.com> > > > > Currently, we are supporting block protection only for flash chips with > > 3 block protection bits (BP0-2) in the SR register. > > > > Enable block protection support for flashes with 4 block protection > > bits > > (BP0-3). > > > > Add a flash_info flag for flashes that describe 4 block protection > > bits. > > Add another flash_info flag for flashes in which BP3 bit is not > > adjacent > > to the BP0-2 bits. > > > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com> > > Reviewed-by: Michael Walle <michael@walle.cc> > > Tested-by: Michael Walle <michael@walle.cc> > > [ta: > > - introduce spi_nor_get_sr_bp_mask(), spi_nor_get_sr_tb_mask() > > - drop Micron n25q512ax3 / BP0-3) boilerplate description > > that was actually a comment on my side some time ago. Because the > current > example isn't really good and lacks the second case (which is added by > this patch). > I didn't like the example that was introduced by Jungseung because of the last column, the "Protected Portion" -> it focuses on Upper/Lower 1/pow(2, n). I think it is better to replace the "Protected Portion" column with a "Protected Block(s)" column (see a winbond datasheet), in order to be in sync with how the code looks now. It's true that the current example has the same problem. Would you care to send a patch to replace the current example? (keeping two examples would be too much). Or maybe remove the example entirely? Also, would you please review 1/5 as well? I need an agreement on that before applying the series. Cheers, ta ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Monday, March 23, 2020 8:27:13 PM EET Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > Hi, > > Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com: > > From: Tudor Ambarus <tudor.ambarus@microchip.com> > > > > Fix the gap for the SR block protection, the BP bits were set with > > a +1 value than actually needed. This patch does not change the > > behavior of the locking operations, just fixes the protected areas. > > So instead of rounding up, it does round down now? No. Why do you say that it rounds up? The behavior is not changed, the patch merely fix the protected area, which was wrong before. The round down is present before this patch. > > > On a 16Mbit flash with 64KByte erase sector, the following changed > > for the lock operation: > > > > Number of blocks | BP2:0 before | BP2:0 now | > > > > 1 | 010b | 001b | > > 2 | 110b | 010b | > > 3 | 110b | 010b | > > 4 | 100b | 011b | > > 5 | 100b | 011b | > > 6 | 100b | 011b | > > 7 | 100b | 011b | > > 8 | 101b | 100b | > > 9 | 101b | 100b | > > > > ... | ... | ... | > > > > For the lock operation, if one requests to lock an area that is not > > matching the upper boundary of a BP protected area, we round down > > the total length and lock less than the user requested, in order to > > not lock more than the user actually requested. > > I don't know if that is really what a user really want. Because you'd > end up with regions which the user believes are locked but are not. True. I'm thinking of how we can improve this, but it's not in the scope of this patch set, because the behavior is not changed. > IMHO if you'd have to make a choice I'd prefer to have the remainder > locked. Not the other way around. Esp. since the user explicitly > express to have a region locked. > We can still talk about this. Please notice that the formula that we want to introduce does the same thing as described in this commit message: for unaligned locks, it round down the length, and for unaligned unlocks it rounds up the length. I'm waiting for a reply. ta ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Am 2020-03-23 20:20, schrieb Tudor.Ambarus@microchip.com: > On Monday, March 23, 2020 8:27:13 PM EET Michael Walle wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the >> content is safe >> >> Hi, >> >> Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com: >> > From: Tudor Ambarus <tudor.ambarus@microchip.com> >> > >> > Fix the gap for the SR block protection, the BP bits were set with >> > a +1 value than actually needed. This patch does not change the >> > behavior of the locking operations, just fixes the protected areas. >> >> So instead of rounding up, it does round down now? > > No. Why do you say that it rounds up? The behavior is not changed, the > patch > merely fix the protected area, which was wrong before. The round down > is > present before this patch. TBH I don't understand what this patch should do. Could you give an example? >> >> > On a 16Mbit flash with 64KByte erase sector, the following changed >> > for the lock operation: 16MBit is a bad example, because it is broken anyway, isn't it? We use a 32Mbit flash where 2MB are locked and the second 2MB are unlocked. Eg. a 50/50 split. I haven't seen any issued. Shouldn't it be then completely locked according this the following example? >> > >> > Number of blocks | BP2:0 before | BP2:0 now | >> > >> > 1 | 010b | 001b | >> > 2 | 110b | 010b | >> > 3 | 110b | 010b | >> > 4 | 100b | 011b | >> > 5 | 100b | 011b | >> > 6 | 100b | 011b | >> > 7 | 100b | 011b | >> > 8 | 101b | 100b | >> > 9 | 101b | 100b | >> > >> > ... | ... | ... | >> > >> > For the lock operation, if one requests to lock an area that is not >> > matching the upper boundary of a BP protected area, we round down >> > the total length and lock less than the user requested, in order to >> > not lock more than the user actually requested. >> >> I don't know if that is really what a user really want. Because you'd >> end up with regions which the user believes are locked but are not. > > True. I'm thinking of how we can improve this, but it's not in the > scope of > this patch set, because the behavior is not changed. ok, agreed, > >> IMHO if you'd have to make a choice I'd prefer to have the remainder >> locked. Not the other way around. Esp. since the user explicitly >> express to have a region locked. >> > > We can still talk about this. Please notice that the formula that we > want to > introduce does the same thing as described in this commit message: for > unaligned locks, it round down the length, and for unaligned unlocks it > rounds > up the length. ok -michael > > I'm waiting for a reply. > ta ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Monday, March 23, 2020 9:54:38 PM EET Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > Am 2020-03-23 20:20, schrieb Tudor.Ambarus@microchip.com: > > On Monday, March 23, 2020 8:27:13 PM EET Michael Walle wrote: > >> EXTERNAL EMAIL: Do not click links or open attachments unless you know > >> the > >> content is safe > >> > >> Hi, > >> > >> Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com: > >> > From: Tudor Ambarus <tudor.ambarus@microchip.com> > >> > > >> > Fix the gap for the SR block protection, the BP bits were set with > >> > a +1 value than actually needed. This patch does not change the > >> > behavior of the locking operations, just fixes the protected areas. > >> > >> So instead of rounding up, it does round down now? > > > > No. Why do you say that it rounds up? The behavior is not changed, the > > patch > > merely fix the protected area, which was wrong before. The round down > > is > > present before this patch. > > TBH I don't understand what this patch should do. Could you give an > example? sure, let me try to be more explicit. > > >> > On a 16Mbit flash with 64KByte erase sector, the following changed > > >> > for the lock operation: > 16MBit is a bad example, because it is broken anyway, isn't it? We use a it's not. > 32Mbit flash where 2MB are locked and the second 2MB are unlocked. Eg. a > 50/50 split. I haven't seen any issued. Shouldn't it be then completely > locked according this the following example? I don't follow. The table from below was generated for the S25FL116K 16 Mbit flash. BTW, one has to disable CONFIG_MTD_SPI_NOR_USE_4K_SECTORS in order to test the locking. When you have a 4k sector erase, the locking is simply wrong, but this is another topic. > > >> > Number of blocks | BP2:0 before | BP2:0 now | > >> > > >> > 1 | 010b | 001b | - number of blocks is how many blocks you want to lock. One would do for one block: flash_lock /dev/mtd 0 1 i.e. lock a single erase block starting from offset 0. - "BP0:2 before" is the result of the operation "flash_lock /dev/mtd 0 1" before this patch - "BP0:2 now" is the result of the operation "flash_lock /dev/mtd 0 1" using this patch So before this patch, the lock operation was bad, because it locked 2 blocks instead of one. > >> > 2 | 110b | 010b | - lock 2 erase blocks starting from offset 0. Results before this patch, and after this patch. Continue the logic on the following lines. oops there's a typo in column 2, sorry. The value in column 2 should have been 011b. So before this patch, when one requested to lock 2 block starting from offset 0, we would obtain 4 blocks locked, and he should have obtained just 2. The scope of this patch is to first fix the locking ops, so that we can introduce a more generic formula that gives the same results as before introducing it. Without this patch, the new formula will silently fix the bug that is described here. > >> > 3 | 110b | 010b | ^ typo s/110b/011b rest of the examples are good. Cheers, ta > >> > 4 | 100b | 011b | > >> > 5 | 100b | 011b | > >> > 6 | 100b | 011b | > >> > 7 | 100b | 011b | > >> > 8 | 101b | 100b | > >> > 9 | 101b | 100b | > >> > > >> > ... | ... | ... | > >> > > >> > For the lock operation, if one requests to lock an area that is not > >> > matching the upper boundary of a BP protected area, we round down > >> > the total length and lock less than the user requested, in order to > >> > not lock more than the user actually requested. ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Am 2020-03-23 21:26, schrieb Tudor.Ambarus@microchip.com: > On Monday, March 23, 2020 9:54:38 PM EET Michael Walle wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the >> content is safe >> Am 2020-03-23 20:20, schrieb Tudor.Ambarus@microchip.com: >> > On Monday, March 23, 2020 8:27:13 PM EET Michael Walle wrote: >> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> >> the >> >> content is safe >> >> >> >> Hi, >> >> >> >> Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com: >> >> > From: Tudor Ambarus <tudor.ambarus@microchip.com> >> >> > >> >> > Fix the gap for the SR block protection, the BP bits were set with >> >> > a +1 value than actually needed. This patch does not change the >> >> > behavior of the locking operations, just fixes the protected areas. >> >> >> >> So instead of rounding up, it does round down now? >> > >> > No. Why do you say that it rounds up? The behavior is not changed, the >> > patch >> > merely fix the protected area, which was wrong before. The round down >> > is >> > present before this patch. >> >> TBH I don't understand what this patch should do. Could you give an >> example? > > sure, let me try to be more explicit. > >> >> >> > On a 16Mbit flash with 64KByte erase sector, the following changed >> >> >> > for the lock operation: >> 16MBit is a bad example, because it is broken anyway, isn't it? We use >> a > > it's not. If I'm not mistaken this falls into the same category like the new 4bits BP flashes, because there are more slots free than needed. Ie. the last one "protect all" is either 110b or 111b and thus don't work with the old formula. This was actually my reason why there is no new formula for the 4 bits BP flashes; but the current one is not working with flashes <32Mbit. See the old long thread. > >> 32Mbit flash where 2MB are locked and the second 2MB are unlocked. Eg. >> a >> 50/50 split. I haven't seen any issued. Shouldn't it be then >> completely >> locked according this the following example? > > I don't follow. We've successfully used the "flash_lock 0 0x200" (with 4k sectors) on our boards to lock the first half of our 4MiB flash. > The table from below was generated for the S25FL116K 16 Mbit flash. > BTW, one > has to disable CONFIG_MTD_SPI_NOR_USE_4K_SECTORS in order to test the > locking. > When you have a 4k sector erase, the locking is simply wrong, but this > is > another topic. it should work with that too if you convert the number to the smaller sectors, ie multiply by 16; But yeah the cli tool has a broken interface. It should accept both offset and length in bytes; not one one in bytes and one in sectors, where the latter also changes with CONFIG_MTD_SPI_NOR_USE_4K_SECTORS. >> >> >> > Number of blocks | BP2:0 before | BP2:0 now | >> >> > >> >> > 1 | 010b | 001b | > > - number of blocks is how many blocks you want to lock. One would do > for one > block: > flash_lock /dev/mtd 0 1 > i.e. lock a single erase block starting from offset 0. > > - "BP0:2 before" is the result of the operation "flash_lock /dev/mtd 0 > 1" > before this patch Without your patch applied it works like expected: [ 1.914329] spi-nor spi0.0: w25q32dw (4096 Kbytes) # flash_lock -l /dev/mtd1 0 1 # cat /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg a4 A4 is 1010_0100, ie BP[2:0] = 001b and TB=1 # flash_lock -u /dev/mtd1 0 64 # flash_lock -l /dev/mtd1 0 32 # cat /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg b8 With this patch applied: # flash_lock -u /dev/mtd1 0 64 # cat /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg 00 # flash_lock -l /dev/mtd1 0 1 flash_lock: error!: could not lock device: /dev/mtd1 error 22 (Invalid argument) # flash_lock -l /dev/mtd1 0 2 # cat /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg a4 which is wrong, isn't it? -michael > > - "BP0:2 now" is the result of the operation "flash_lock /dev/mtd 0 1" > using > this patch > > So before this patch, the lock operation was bad, because it locked 2 > blocks > instead of one. > >> >> > 2 | 110b | 010b | > > - lock 2 erase blocks starting from offset 0. Results before this > patch, and > after this patch. Continue the logic on the following lines. > > oops there's a typo in column 2, sorry. The value in column 2 should > have been > 011b. > > So before this patch, when one requested to lock 2 block starting from > offset > 0, we would obtain 4 blocks locked, and he should have obtained just 2. > > The scope of this patch is to first fix the locking ops, so that we can > introduce a more generic formula that gives the same results as before > introducing it. Without this patch, the new formula will silently fix > the bug > that is described here. > >> >> > 3 | 110b | 010b | > ^ typo s/110b/011b > > rest of the examples are good. > > Cheers, > ta > >> >> > 4 | 100b | 011b | >> >> > 5 | 100b | 011b | >> >> > 6 | 100b | 011b | >> >> > 7 | 100b | 011b | >> >> > 8 | 101b | 100b | >> >> > 9 | 101b | 100b | >> >> > >> >> > ... | ... | ... | >> >> > >> >> > For the lock operation, if one requests to lock an area that is not >> >> > matching the upper boundary of a BP protected area, we round down >> >> > the total length and lock less than the user requested, in order to >> >> > not lock more than the user actually requested. ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Monday, March 23, 2020 11:14:05 PM EET Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > Am 2020-03-23 21:26, schrieb Tudor.Ambarus@microchip.com: > > On Monday, March 23, 2020 9:54:38 PM EET Michael Walle wrote: > >> EXTERNAL EMAIL: Do not click links or open attachments unless you know > >> the > >> content is safe > >> > >> Am 2020-03-23 20:20, schrieb Tudor.Ambarus@microchip.com: > >> > On Monday, March 23, 2020 8:27:13 PM EET Michael Walle wrote: > >> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know > >> >> the > >> >> content is safe > >> >> > >> >> Hi, > >> >> > >> >> Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com: > >> >> > From: Tudor Ambarus <tudor.ambarus@microchip.com> > >> >> > > >> >> > Fix the gap for the SR block protection, the BP bits were set with > >> >> > a +1 value than actually needed. This patch does not change the > >> >> > behavior of the locking operations, just fixes the protected areas. > >> >> > >> >> So instead of rounding up, it does round down now? > >> > > >> > No. Why do you say that it rounds up? The behavior is not changed, the > >> > patch > >> > merely fix the protected area, which was wrong before. The round down > >> > is > >> > present before this patch. > >> > >> TBH I don't understand what this patch should do. Could you give an > >> example? > > > > sure, let me try to be more explicit. > > > >> >> > On a 16Mbit flash with 64KByte erase sector, the following changed > >> > >> >> > for the lock operation: > >> 16MBit is a bad example, because it is broken anyway, isn't it? We use > >> a > > > > it's not. > > If I'm not mistaken this falls into the same category like the new 4bits > BP > flashes, because there are more slots free than needed. Ie. the last one > "protect all" is either 110b or 111b and thus don't work with the old > formula. This was actually my reason why there is no new formula for the > 4 bits BP flashes; but the current one is not working with flashes > <32Mbit. > See the old long thread. > > >> 32Mbit flash where 2MB are locked and the second 2MB are unlocked. Eg. > >> a > >> 50/50 split. I haven't seen any issued. Shouldn't it be then > >> completely > >> locked according this the following example? > > > > I don't follow. > > We've successfully used the "flash_lock 0 0x200" (with 4k sectors) on > our > boards to lock the first half of our 4MiB flash. > > > The table from below was generated for the S25FL116K 16 Mbit flash. > > BTW, one > > has to disable CONFIG_MTD_SPI_NOR_USE_4K_SECTORS in order to test the > > locking. > > When you have a 4k sector erase, the locking is simply wrong, but this > > is > > another topic. > > it should work with that too if you convert the number to the smaller > sectors, > ie multiply by 16; But yeah the cli tool has a broken interface. It > should > accept both offset and length in bytes; not one one in bytes and one in > sectors, > where the latter also changes with CONFIG_MTD_SPI_NOR_USE_4K_SECTORS. > > >> >> > Number of blocks | BP2:0 before | BP2:0 now | > >> >> > > >> >> > 1 | 010b | 001b | > > > > - number of blocks is how many blocks you want to lock. One would do > > for one > > > > block: > > flash_lock /dev/mtd 0 1 > > > > i.e. lock a single erase block starting from offset 0. > > > > - "BP0:2 before" is the result of the operation "flash_lock /dev/mtd 0 > > 1" > > before this patch > > Without your patch applied it works like expected: > > [ 1.914329] spi-nor spi0.0: w25q32dw (4096 Kbytes) > # flash_lock -l /dev/mtd1 0 1 > # cat > /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg > a4 > > A4 is 1010_0100, ie BP[2:0] = 001b and TB=1 > what happens if you request flash_lock -l /dev/mtd1 0 3? > # flash_lock -u /dev/mtd1 0 64 > # flash_lock -l /dev/mtd1 0 32 > # cat > /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg > b8 > > > With this patch applied: > > # flash_lock -u /dev/mtd1 0 64 > # cat > /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg > 00 > # flash_lock -l /dev/mtd1 0 1 > flash_lock: error!: could not lock device: /dev/mtd1 > > error 22 (Invalid argument) I'm wondering what was the reason for the -EINVAL. > # flash_lock -l /dev/mtd1 0 2 > # cat > /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg > a4 > > which is wrong, isn't it? > Looks so. You should have obtained, 0xa8, right? Will recheck tomorrow morning. Thanks for testing this! I don't have a 32Mbit flash ... Cheers, ta > > > - "BP0:2 now" is the result of the operation "flash_lock /dev/mtd 0 1" > > using > > this patch > > > > So before this patch, the lock operation was bad, because it locked 2 > > blocks > > instead of one. > > > >> >> > 2 | 110b | 010b | > > > > - lock 2 erase blocks starting from offset 0. Results before this > > patch, and > > after this patch. Continue the logic on the following lines. > > > > oops there's a typo in column 2, sorry. The value in column 2 should > > have been > > 011b. > > > > So before this patch, when one requested to lock 2 block starting from > > offset > > 0, we would obtain 4 blocks locked, and he should have obtained just 2. > > > > The scope of this patch is to first fix the locking ops, so that we can > > introduce a more generic formula that gives the same results as before > > introducing it. Without this patch, the new formula will silently fix > > the bug > > that is described here. > > > >> >> > 3 | 110b | 010b | > > > > ^ typo s/110b/011b > > > > rest of the examples are good. > > > > Cheers, > > ta > > > >> >> > 4 | 100b | 011b | > >> >> > 5 | 100b | 011b | > >> >> > 6 | 100b | 011b | > >> >> > 7 | 100b | 011b | > >> >> > 8 | 101b | 100b | > >> >> > 9 | 101b | 100b | > >> >> > > >> >> > ... | ... | ... | > >> >> > > >> >> > For the lock operation, if one requests to lock an area that is not > >> >> > matching the upper boundary of a BP protected area, we round down > >> >> > the total length and lock less than the user requested, in order to > >> >> > not lock more than the user actually requested. ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Monday, March 23, 2020 11:30:44 PM EET Tudor Ambarus wrote: > > With this patch applied: > > > > # flash_lock -u /dev/mtd1 0 64 > > # cat > > /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg > > 00 > > # flash_lock -l /dev/mtd1 0 1 > > flash_lock: error!: could not lock device: /dev/mtd1 > > > > error 22 (Invalid argument) > > I'm wondering what was the reason for the -EINVAL. Probably because the BP bits would have value zero. ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Am 2020-03-23 22:30, schrieb Tudor.Ambarus@microchip.com: > On Monday, March 23, 2020 11:14:05 PM EET Michael Walle wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the >> content is safe >> Am 2020-03-23 21:26, schrieb Tudor.Ambarus@microchip.com: >> > On Monday, March 23, 2020 9:54:38 PM EET Michael Walle wrote: >> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> >> the >> >> content is safe >> >> >> >> Am 2020-03-23 20:20, schrieb Tudor.Ambarus@microchip.com: >> >> > On Monday, March 23, 2020 8:27:13 PM EET Michael Walle wrote: >> >> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> >> >> the >> >> >> content is safe >> >> >> >> >> >> Hi, >> >> >> >> >> >> Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com: >> >> >> > From: Tudor Ambarus <tudor.ambarus@microchip.com> >> >> >> > >> >> >> > Fix the gap for the SR block protection, the BP bits were set with >> >> >> > a +1 value than actually needed. This patch does not change the >> >> >> > behavior of the locking operations, just fixes the protected areas. >> >> >> >> >> >> So instead of rounding up, it does round down now? >> >> > >> >> > No. Why do you say that it rounds up? The behavior is not changed, the >> >> > patch >> >> > merely fix the protected area, which was wrong before. The round down >> >> > is >> >> > present before this patch. >> >> >> >> TBH I don't understand what this patch should do. Could you give an >> >> example? >> > >> > sure, let me try to be more explicit. >> > >> >> >> > On a 16Mbit flash with 64KByte erase sector, the following changed >> >> >> >> >> > for the lock operation: >> >> 16MBit is a bad example, because it is broken anyway, isn't it? We use >> >> a >> > >> > it's not. >> >> If I'm not mistaken this falls into the same category like the new >> 4bits >> BP >> flashes, because there are more slots free than needed. Ie. the last >> one >> "protect all" is either 110b or 111b and thus don't work with the old >> formula. This was actually my reason why there is no new formula for >> the >> 4 bits BP flashes; but the current one is not working with flashes >> <32Mbit. >> See the old long thread. >> >> >> 32Mbit flash where 2MB are locked and the second 2MB are unlocked. Eg. >> >> a >> >> 50/50 split. I haven't seen any issued. Shouldn't it be then >> >> completely >> >> locked according this the following example? >> > >> > I don't follow. >> >> We've successfully used the "flash_lock 0 0x200" (with 4k sectors) on >> our >> boards to lock the first half of our 4MiB flash. >> >> > The table from below was generated for the S25FL116K 16 Mbit flash. >> > BTW, one >> > has to disable CONFIG_MTD_SPI_NOR_USE_4K_SECTORS in order to test the >> > locking. >> > When you have a 4k sector erase, the locking is simply wrong, but this >> > is >> > another topic. >> >> it should work with that too if you convert the number to the smaller >> sectors, >> ie multiply by 16; But yeah the cli tool has a broken interface. It >> should >> accept both offset and length in bytes; not one one in bytes and one >> in >> sectors, >> where the latter also changes with CONFIG_MTD_SPI_NOR_USE_4K_SECTORS. >> >> >> >> > Number of blocks | BP2:0 before | BP2:0 now | >> >> >> > >> >> >> > 1 | 010b | 001b | >> > >> > - number of blocks is how many blocks you want to lock. One would do >> > for one >> > >> > block: >> > flash_lock /dev/mtd 0 1 >> > >> > i.e. lock a single erase block starting from offset 0. >> > >> > - "BP0:2 before" is the result of the operation "flash_lock /dev/mtd 0 >> > 1" >> > before this patch >> >> Without your patch applied it works like expected: >> >> [ 1.914329] spi-nor spi0.0: w25q32dw (4096 Kbytes) >> # flash_lock -l /dev/mtd1 0 1 >> # cat >> /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg >> a4 >> >> A4 is 1010_0100, ie BP[2:0] = 001b and TB=1 >> > > what happens if you request flash_lock -l /dev/mtd1 0 3? with your patch applied: # flash_lock -u /dev/mtd1 0 64 # cat /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg 00 # flash_lock -l /dev/mtd1 0 3 # cat /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg a4 without it: # flash_lock -u /dev/mtd1 0 64 # cat /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg 00 # flash_lock -l /dev/mtd1 0 3 # cat /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg a8 >> # flash_lock -u /dev/mtd1 0 64 >> # flash_lock -l /dev/mtd1 0 32 >> # cat >> /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg >> b8 >> >> >> With this patch applied: >> >> # flash_lock -u /dev/mtd1 0 64 >> # cat >> /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg >> 00 >> # flash_lock -l /dev/mtd1 0 1 >> flash_lock: error!: could not lock device: /dev/mtd1 >> >> error 22 (Invalid argument) > > I'm wondering what was the reason for the -EINVAL. > >> # flash_lock -l /dev/mtd1 0 2 >> # cat >> /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg >> a4 >> >> which is wrong, isn't it? >> > Looks so. You should have obtained, 0xa8, right? correct, BP should be 010b for the first two sectors. > Will recheck tomorrow > morning. > > Thanks for testing this! I don't have a 32Mbit flash ... You should be able to reproduce it with every flash >=32Mbit which has 3 BP bits. -michael > > Cheers, > ta >> >> > - "BP0:2 now" is the result of the operation "flash_lock /dev/mtd 0 1" >> > using >> > this patch >> > >> > So before this patch, the lock operation was bad, because it locked 2 >> > blocks >> > instead of one. >> > >> >> >> > 2 | 110b | 010b | >> > >> > - lock 2 erase blocks starting from offset 0. Results before this >> > patch, and >> > after this patch. Continue the logic on the following lines. >> > >> > oops there's a typo in column 2, sorry. The value in column 2 should >> > have been >> > 011b. >> > >> > So before this patch, when one requested to lock 2 block starting from >> > offset >> > 0, we would obtain 4 blocks locked, and he should have obtained just 2. >> > >> > The scope of this patch is to first fix the locking ops, so that we can >> > introduce a more generic formula that gives the same results as before >> > introducing it. Without this patch, the new formula will silently fix >> > the bug >> > that is described here. >> > >> >> >> > 3 | 110b | 010b | >> > >> > ^ typo s/110b/011b >> > >> > rest of the examples are good. >> > >> > Cheers, >> > ta >> > >> >> >> > 4 | 100b | 011b | >> >> >> > 5 | 100b | 011b | >> >> >> > 6 | 100b | 011b | >> >> >> > 7 | 100b | 011b | >> >> >> > 8 | 101b | 100b | >> >> >> > 9 | 101b | 100b | >> >> >> > >> >> >> > ... | ... | ... | >> >> >> > >> >> >> > For the lock operation, if one requests to lock an area that is not >> >> >> > matching the upper boundary of a BP protected area, we round down >> >> >> > the total length and lock less than the user requested, in order to >> >> >> > not lock more than the user actually requested. ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Hi, Tudor, On Mon, 2020-03-23 at 09:24 +0000, Tudor.Ambarus@microchip.com wrote: > From: Tudor Ambarus <tudor.ambarus@microchip.com> > > Fix the gap for the SR block protection, the BP bits were set with > a +1 value than actually needed. This patch does not change the > behavior of the locking operations, just fixes the protected areas. > > On a 16Mbit flash with 64KByte erase sector, the following changed > for the lock operation: > > Number of blocks | BP2:0 before | BP2:0 now | > 1 | 010b | 001b | > 2 | 110b | 010b | > 3 | 110b | 010b | > 4 | 100b | 011b | > 5 | 100b | 011b | > 6 | 100b | 011b | > 7 | 100b | 011b | > 8 | 101b | 100b | > 9 | 101b | 100b | > ... | ... | ... | > > For the lock operation, if one requests to lock an area that is not > matching the upper boundary of a BP protected area, we round down > the total length and lock less than the user requested, in order to > not lock more than the user actually requested. > > For the unlock operation, read the number of blocks column as > "locked all but number of blocks value". On a 16Mbit flash with > 64KByte erase sector, the following changed for the lock operation: > > Number of blocks | BP2:0 before | BP2:0 now | > 1 | 111b | 101b | > ... | ... | ... | > 15 | 111b | 101b | > 16 | 110b | 101b | > 17 | 110b | 100b | > ... | ... | ... | > 24 | 101b | 100b | > 25 | 101b | 011b | > 26 | 101b | 011b | > 27 | 101b | 011b | > 28 | 100b | 011b | > 29 | 100b | 010b | > 30 | 011b | 010b | > 31 | 010b | 001b | > 32 | 000b | 000b | > > For the unlock operation, if one requests to unlock an area that is > not matching the upper boundary of a BP protected area, we round up > the total length and unlock more than the user actually requested. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/mtd/spi-nor/core.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 877557dbda7f..36660068bc04 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -1654,13 +1654,13 @@ static int spi_nor_sr_lock(struct spi_nor > *nor, loff_t ofs, uint64_t len) > /* > * Need smallest pow such that: > * > - * 1 / (2^pow) <= (len / size) > + * 1 / ((2^pow) - 1) <= (len / size) > * > * so (assuming power-of-2 size) we do: > * > - * pow = ceil(log2(size / len)) = log2(size) - > floor(log2(len)) > + * pow = ceil(log2(size / len)) = log2(size) - > floor(log2(len)) + 1 > */ > - pow = ilog2(mtd->size) - ilog2(lock_len); > + pow = ilog2(mtd->size) - ilog2(lock_len) + 1; > val = mask - (pow << SR_BP_SHIFT); > if (val & ~mask) > return -EINVAL; (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; I think that current mainline implementation is only valid for flashes in case (1). (for BP0-2 : 1/64, 1/32 ...) Isn't it? This is current implementaion. pow = ilog2(mtd->size) - order_base_2(lock_len) val = mask - (pow << SR_BP_SHIFT); 1/64 6 = 110b -> 001b 1/32 5 = 101b -> 010b 1/16 4 = 100b -> 011b 1/8 3 = 011b -> 100b 1/4 2 = 010b -> 101b 1/2 1 = 001b -> 110b ALL 0 = 000b -> 111b It is handling case (1) admirably. In other cases, however, the situation would be different. A 16Mbit flash with 64KByte erase sector(which you mentioned on this patch) should get 32 erase sectors and seems that it's smallest protected portion is 1/32. So supporting the flash, it needs some modification as you did. pow = ilog2(mtd->size) - order_base_2(lock_len) + 1 val = mask - (pow << SR_BP_SHIFT); 1/64 6 = 110b 111b -> 000b (execption case) 1/32 5 = 101b 110b -> 001b 1/16 4 = 100b 101b -> 010b 1/8 3 = 011b 100b -> 011b 1/4 2 = 010b 011b -> 100b 1/2 1 = 001b 010b -> 101b ALL 0 = 000b 001b -> 110b It would works for the flash, but it will conflicts with flashes in case (1) what has already been applied on mainline. And there are too various flashes that has different protected portions to handle with the above. Btw, the description on this patch is exactly what I had been looking for before and seems it's very useful. Thanks, > @@ -1739,13 +1739,13 @@ static int spi_nor_sr_unlock(struct spi_nor > *nor, loff_t ofs, uint64_t len) > /* > * Need largest pow such that: > * > - * 1 / (2^pow) >= (len / size) > + * 1 / ((2^pow) - 1) >= (len / size) > * > * so (assuming power-of-2 size) we do: > * > - * pow = floor(log2(size / len)) = log2(size) - > ceil(log2(len)) > + * pow = floor(log2(size / len)) = log2(size) - > ceil(log2(len)) + 1 > */ > - pow = ilog2(mtd->size) - order_base_2(lock_len); > + pow = ilog2(mtd->size) - order_base_2(lock_len) + 1; > if (lock_len == 0) { > val = 0; /* fully unlocked */ > } else { ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Tuesday, March 24, 2020 12:35:30 AM EET Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > Am 2020-03-23 22:30, schrieb Tudor.Ambarus@microchip.com: > > On Monday, March 23, 2020 11:14:05 PM EET Michael Walle wrote: > >> EXTERNAL EMAIL: Do not click links or open attachments unless you know > >> the > >> content is safe > >> > >> Am 2020-03-23 21:26, schrieb Tudor.Ambarus@microchip.com: > >> > On Monday, March 23, 2020 9:54:38 PM EET Michael Walle wrote: > >> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know > >> >> the > >> >> content is safe > >> >> > >> >> Am 2020-03-23 20:20, schrieb Tudor.Ambarus@microchip.com: > >> >> > On Monday, March 23, 2020 8:27:13 PM EET Michael Walle wrote: > >> >> >> EXTERNAL EMAIL: Do not click links or open attachments unless you > >> >> >> know > >> >> >> the > >> >> >> content is safe > >> >> >> > >> >> >> Hi, > >> >> >> > >> >> >> Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com: > >> >> >> > From: Tudor Ambarus <tudor.ambarus@microchip.com> > >> >> >> > > >> >> >> > Fix the gap for the SR block protection, the BP bits were set > >> >> >> > with > >> >> >> > a +1 value than actually needed. This patch does not change the > >> >> >> > behavior of the locking operations, just fixes the protected > >> >> >> > areas. > >> >> >> > >> >> >> So instead of rounding up, it does round down now? > >> >> > > >> >> > No. Why do you say that it rounds up? The behavior is not changed, > >> >> > the > >> >> > patch > >> >> > merely fix the protected area, which was wrong before. The round > >> >> > down > >> >> > is > >> >> > present before this patch. > >> >> > >> >> TBH I don't understand what this patch should do. Could you give an > >> >> example? > >> > > >> > sure, let me try to be more explicit. > >> > > >> >> >> > On a 16Mbit flash with 64KByte erase sector, the following > >> >> >> > changed > >> >> > >> >> >> > for the lock operation: > >> >> 16MBit is a bad example, because it is broken anyway, isn't it? We use > >> >> a > >> > > >> > it's not. > >> > >> If I'm not mistaken this falls into the same category like the new > >> 4bits > >> BP > >> flashes, because there are more slots free than needed. Ie. the last > >> one > >> "protect all" is either 110b or 111b and thus don't work with the old > >> formula. This was actually my reason why there is no new formula for > >> the > >> 4 bits BP flashes; but the current one is not working with flashes > >> <32Mbit. > >> See the old long thread. You're right, I was trying to fix a dead horse. 16Mbit BP0:2 flashes are fixed with the generic formula. I'm going to respin without this patch. Thanks! ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Monday, March 23, 2020 11:24:33 AM EET Tudor Ambarus - M18064 wrote: > For the unlock operation, read the number of blocks column as > "locked all but number of blocks value". On a 16Mbit flash with > 64KByte erase sector, the following changed for the lock operation: ^ typo s/lock/unlock ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/