* [PATCH 1/3] mtd: spi-nor: introduce SR_BP_SHIFT define
[not found] <CGME20200110102307epcas1p2a8b32fc2a2455d1052982d8ac499d21b@epcas1p2.samsung.com>
@ 2020-01-10 10:22 ` Jungseung Lee
[not found] ` <CGME20200110102310epcas1p40589cbb4370dcd4db08efa4008deb755@epcas1p4.samsung.com>
[not found] ` <CGME20200110102313epcas1p17aed8316bfacb0a0fb9a42fc0db24292@epcas1p1.samsung.com>
0 siblings, 2 replies; 9+ messages in thread
From: Jungseung Lee @ 2020-01-10 10:22 UTC (permalink / raw)
To: Tudor Ambarus, Vignesh Raghavendra, linux-mtd, js07.lee, js07.lee
The shift variable of SR_BP is conclusive because the first bit of SR_BP
is fixed on all known flashes.
Introduce SR_BP_SHIFT define and let them used by stm_* functions
to replace ffs operation to get shift value.
Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
---
drivers/mtd/spi-nor/spi-nor.c | 11 +++++------
include/linux/mtd/spi-nor.h | 2 ++
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index addb6319fcbb..e3da6a8654a8 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1767,7 +1767,6 @@ static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs,
struct mtd_info *mtd = &nor->mtd;
u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
u8 tb_mask = SR_TB_BIT5;
- int shift = ffs(mask) - 1;
int pow;
if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
@@ -1778,7 +1777,7 @@ static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs,
*ofs = 0;
*len = 0;
} else {
- pow = ((sr & mask) ^ mask) >> shift;
+ pow = ((sr & mask) ^ mask) >> SR_BP_SHIFT;
*len = mtd->size >> pow;
if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
*ofs = 0;
@@ -1860,7 +1859,7 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
int ret, status_old, status_new;
u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
u8 tb_mask = SR_TB_BIT5;
- u8 shift = ffs(mask) - 1, pow, val;
+ u8 pow, val;
loff_t lock_len;
bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
bool use_top;
@@ -1909,7 +1908,7 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
* pow = ceil(log2(size / len)) = log2(size) - floor(log2(len))
*/
pow = ilog2(mtd->size) - ilog2(lock_len);
- val = mask - (pow << shift);
+ val = mask - (pow << SR_BP_SHIFT);
if (val & ~mask)
return -EINVAL;
/* Don't "lock" with no region! */
@@ -1946,7 +1945,7 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
int ret, status_old, status_new;
u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
u8 tb_mask = SR_TB_BIT5;
- u8 shift = ffs(mask) - 1, pow, val;
+ u8 pow, val;
loff_t lock_len;
bool can_be_top = true, can_be_bottom = nor->flags & SNOR_F_HAS_SR_TB;
bool use_top;
@@ -1997,7 +1996,7 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
if (lock_len == 0) {
val = 0; /* fully unlocked */
} else {
- val = mask - (pow << shift);
+ val = mask - (pow << SR_BP_SHIFT);
/* Some power-of-two sizes are not supported */
if (val & ~mask)
return -EINVAL;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 7e32adce72f7..541c06d042e8 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -137,6 +137,8 @@
#define SR1_QUAD_EN_BIT6 BIT(6)
+#define SR_BP_SHIFT 2
+
/* Enhanced Volatile Configuration Register bits */
#define EVCR_QUAD_EN_MICRON BIT(7) /* Micron Quad I/O */
--
2.17.1
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] mtd: spi-nor: add 4bit block protection support
[not found] ` <CGME20200110102310epcas1p40589cbb4370dcd4db08efa4008deb755@epcas1p4.samsung.com>
@ 2020-01-10 10:22 ` Jungseung Lee
0 siblings, 0 replies; 9+ messages in thread
From: Jungseung Lee @ 2020-01-10 10:22 UTC (permalink / raw)
To: Tudor Ambarus, Vignesh Raghavendra, linux-mtd, js07.lee, js07.lee
Currently, we are supporting block protection only for
flash chips with 3 block protection bits in the SR register.
This patch enables block protection support for some flash with
4 block protection bits(bp0-3).
Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
---
drivers/mtd/spi-nor/spi-nor.c | 90 +++++++++++++++++++++++++++++++----
include/linux/mtd/spi-nor.h | 8 ++++
2 files changed, 88 insertions(+), 10 deletions(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index e3da6a8654a8..214f3b733e9b 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -238,6 +238,14 @@ struct flash_info {
* status register. Must be used with
* SPI_NOR_HAS_TB.
*/
+#define SPI_NOR_HAS_BP3 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_HAS_BP3.
+ */
/* Part specific fixup hooks. */
const struct spi_nor_fixups *fixups;
@@ -1766,24 +1774,48 @@ static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs,
{
struct mtd_info *mtd = &nor->mtd;
u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
- u8 tb_mask = SR_TB_BIT5;
- int pow;
+ u8 tb_mask = SR_TB_BIT5, bp;
+ int pow = 0;
if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
tb_mask = SR_TB_BIT6;
- if (!(sr & mask)) {
+ if (nor->flags & SNOR_F_HAS_SR_BP3) {
+ u8 tmp;
+
+ if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
+ tmp = sr & (mask | SR_BP3_BIT6);
+ else
+ tmp = sr & (mask | SR_BP3_BIT5);
+
+ if (tmp & SR_BP3_BIT6)
+ tmp = (tmp & ~BIT(6)) | BIT(5);
+
+ bp = tmp >> SR_BP_SHIFT;
+ } else {
+ bp = (sr & mask) >> SR_BP_SHIFT;
+ }
+
+ if (!bp) {
/* No protection */
*ofs = 0;
*len = 0;
+ return;
+ }
+
+ if (nor->flags & SNOR_F_HAS_SR_BP3) {
+ if (bp <= ilog2(nor->n_sectors))
+ pow = ilog2(nor->n_sectors) + 1 - bp;
} 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;
+ pow = bp ^ (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;
}
/*
@@ -1898,6 +1930,12 @@ static int stm_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;
+ if (nor->flags & SNOR_F_HAS_SR_BP3) {
+ if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
+ mask = mask | SR_BP3_BIT6;
+ else
+ mask = mask | SR_BP3_BIT5;
+ }
/*
* Need smallest pow such that:
*
@@ -1908,7 +1946,17 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
* pow = ceil(log2(size / len)) = log2(size) - floor(log2(len))
*/
pow = ilog2(mtd->size) - ilog2(lock_len);
- val = mask - (pow << SR_BP_SHIFT);
+
+ if (nor->flags & SNOR_F_HAS_SR_BP3) {
+ val = ilog2(nor->n_sectors) + 1 - pow;
+ val = val << SR_BP_SHIFT;
+
+ if (val & BIT(5) && mask & SR_BP3_BIT6)
+ val = (val & ~BIT(5)) | BIT(6);
+ } else {
+ val = mask - (pow << SR_BP_SHIFT);
+ }
+
if (val & ~mask)
return -EINVAL;
/* Don't "lock" with no region! */
@@ -1983,6 +2031,13 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
if (nor->flags & SNOR_F_HAS_SR_TB_BIT6)
tb_mask = SR_TB_BIT6;
+
+ if (nor->flags & SNOR_F_HAS_SR_BP3) {
+ if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
+ mask = mask | SR_BP3_BIT6;
+ else
+ mask = mask | SR_BP3_BIT5;
+ }
/*
* Need largest pow such that:
*
@@ -1995,6 +2050,14 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
pow = ilog2(mtd->size) - order_base_2(lock_len);
if (lock_len == 0) {
val = 0; /* fully unlocked */
+ } else if (nor->flags & SNOR_F_HAS_SR_BP3) {
+ val = ilog2(nor->n_sectors) - pow + 1;
+ val = val << SR_BP_SHIFT;
+
+ if (val & BIT(5) && mask & SR_BP3_BIT6)
+ val = (val & ~BIT(5)) | BIT(6);
+ if (val & ~mask)
+ return -EINVAL;
} else {
val = mask - (pow << SR_BP_SHIFT);
/* Some power-of-two sizes are not supported */
@@ -4736,6 +4799,7 @@ static void spi_nor_info_init_params(struct spi_nor *nor)
/* Set SPI NOR sizes. */
params->size = (u64)info->sector_size * info->n_sectors;
params->page_size = info->page_size;
+ params->n_sectors = info->n_sectors;
if (!(info->flags & SPI_NOR_NO_FR)) {
/* Default to Fast Read for DT and non-DT platform devices. */
@@ -5192,6 +5256,11 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
if (info->flags & USE_CLSR)
nor->flags |= SNOR_F_USE_CLSR;
+ if (info->flags & SPI_NOR_HAS_BP3) {
+ nor->flags |= SNOR_F_HAS_SR_BP3;
+ if (info->flags & SPI_NOR_BP3_SR_BIT6)
+ nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
+ }
if (info->flags & SPI_NOR_NO_ERASE)
mtd->flags |= MTD_NO_ERASE;
@@ -5199,6 +5268,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
mtd->dev.parent = dev;
nor->page_size = params->page_size;
mtd->writebufsize = nor->page_size;
+ nor->n_sectors = params->n_sectors;
if (of_property_read_bool(np, "broken-flash-reset"))
nor->flags |= SNOR_F_BROKEN_RESET;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 541c06d042e8..92d550501daf 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -129,7 +129,9 @@
#define SR_BP1 BIT(3) /* Block protect 1 */
#define SR_BP2 BIT(4) /* Block protect 2 */
#define SR_TB_BIT5 BIT(5) /* Top/Bottom protect */
+#define SR_BP3_BIT5 BIT(5) /* Block protect 3 */
#define SR_TB_BIT6 BIT(6) /* Top/Bottom protect */
+#define SR_BP3_BIT6 BIT(6) /* Block protect 3 */
#define SR_SRWD BIT(7) /* SR write protect */
/* Spansion/Cypress specific status bits */
#define SR_E_ERR BIT(5)
@@ -248,6 +250,8 @@ enum spi_nor_option_flags {
SNOR_F_HAS_16BIT_SR = BIT(9),
SNOR_F_NO_READ_CR = BIT(10),
SNOR_F_HAS_SR_TB_BIT6 = BIT(11),
+ SNOR_F_HAS_SR_BP3 = BIT(12),
+ SNOR_F_HAS_SR_BP3_BIT6 = BIT(13),
};
@@ -519,6 +523,7 @@ struct spi_nor_locking_ops {
*
* @size: the flash memory density in bytes.
* @page_size: the page size of the SPI NOR flash memory.
+ * @n_sectors: number of sectors
* @hwcaps: describes the read and page program hardware
* capabilities.
* @reads: read capabilities ordered by priority: the higher index
@@ -541,6 +546,7 @@ struct spi_nor_locking_ops {
struct spi_nor_flash_parameter {
u64 size;
u32 page_size;
+ u16 n_sectors;
struct spi_nor_hwcaps hwcaps;
struct spi_nor_read_command reads[SNOR_CMD_READ_MAX];
@@ -573,6 +579,7 @@ struct flash_info;
* @bouncebuf_size: size of the bounce buffer
* @info: spi-nor part JDEC MFR id and other info
* @page_size: the page size of the SPI NOR
+ * @n_sector: number of sectors
* @addr_width: number of address bytes
* @erase_opcode: the opcode for erasing a sector
* @read_opcode: the read opcode
@@ -599,6 +606,7 @@ struct spi_nor {
size_t bouncebuf_size;
const struct flash_info *info;
u32 page_size;
+ u16 n_sectors;
u8 addr_width;
u8 erase_opcode;
u8 read_opcode;
--
2.17.1
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] mtd: spi-nor: support lock/unlock for a few Micron chip s
[not found] ` <CGME20200110102313epcas1p17aed8316bfacb0a0fb9a42fc0db24292@epcas1p1.samsung.com>
@ 2020-01-10 10:22 ` Jungseung Lee
0 siblings, 0 replies; 9+ messages in thread
From: Jungseung Lee @ 2020-01-10 10:22 UTC (permalink / raw)
To: Tudor Ambarus, Vignesh Raghavendra, linux-mtd, js07.lee, js07.lee
Some Micron models are known to have lock/unlock support,
and that also support 4bit block protection bit (bp0-3).
This patch support lock/unlock feature on the flash.
Tested on w25q512ax3. The Other is modified following the datasheet.
Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
---
drivers/mtd/spi-nor/spi-nor.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 214f3b733e9b..da285ab56a3d 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2558,12 +2558,17 @@ static const struct flash_info spi_nor_ids[] = {
{ "mt25ql512a", INFO6(0x20ba20, 0x104400, 64 * 1024, 1024,
SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
- { "n25q512ax3", INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
+ { "n25q512ax3", INFO(0x20ba20, 0, 64 * 1024, 1024,
+ SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
+ SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
+ SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6) },
{ "mt25qu512a", INFO6(0x20bb20, 0x104400, 64 * 1024, 1024,
SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
- { "n25q512a", INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K |
- USE_FSR | SPI_NOR_QUAD_READ) },
+ { "n25q512a", INFO(0x20bb20, 0, 64 * 1024, 1024,
+ SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
+ SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
+ SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6) },
{ "n25q00", INFO(0x20ba21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
{ "n25q00a", INFO(0x20bb21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
{ "mt25ql02g", INFO(0x20ba22, 0, 64 * 1024, 4096,
--
2.17.1
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] mtd: spi-nor: add 4bit block protection support
2020-03-17 14:52 ` Michael Walle
@ 2020-03-18 6:01 ` Jungseung Lee
0 siblings, 0 replies; 9+ messages in thread
From: Jungseung Lee @ 2020-03-18 6:01 UTC (permalink / raw)
To: Michael Walle
Cc: Vignesh Raghavendra, chenxiang, Tudor Ambarus, linux-mtd,
js07.lee, js07.lee
Hi,
On Tue, 2020-03-17 at 15:52 +0100, Michael Walle wrote:
> Am 2020-03-17 12:35, schrieb Jungseung Lee:
> > Hi, Micahel
> >
> > On Tue, 2020-03-17 at 20:00 +0900, Jungseung Lee wrote:
> > > Hi, Michael,
> > >
> > > On Fri, 2020-03-13 at 17:24 +0100, Michael Walle wrote:
> > > > Hi Jungseung,
> > > >
> > > > sorry for the late review.
> > > >
> > >
> > > Not at all. thanks for your review.
> > >
> > > > Am 2020-03-04 12:07, schrieb Jungseung Lee:
> > > > > Currently, we are supporting block protection only for
> > > > > flash chips with 3 block protection bits in the SR register.
> > > > > This patch enables block protection support for flashes with
> > > > > 4 block protection bits(bp0-3).
> > > > >
> > > > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> > > > > ---
> > > > > drivers/mtd/spi-nor/spi-nor.c | 82
> > > > > ++++++++++++++++++++++++++++++++---
> > > > > include/linux/mtd/spi-nor.h | 4 ++
> > > > > 2 files changed, 79 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > > > > b/drivers/mtd/spi-nor/spi-nor.c
> > > > > index c58c27552a74..31a2106e529a 100644
> > > > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > > > > @@ -238,6 +238,14 @@ struct flash_info {
> > > > > * status register.
> > > > > Must be
> > > > > used with
> > > > > * SPI_NOR_HAS_TB.
> > > > > */
> > > > > +#define SPI_NOR_4BIT_BP BIT(17) /*
> > > > > + * Flash SR has 4 bit
> > > > > fields
> > > > > (BP0-3)
> > > > > + * for block
> > > > > protection.
> > > > > + */
> > > > > +#define SPI_NOR_BP3_SR_BIT6 BIT(18) /*
> > > > > + * BP3 is bit 6 of
> > > > > status
> > > > > register.
> > > > > + * Must be used with
> > > > > SPI_NOR_4BIT_BP.
> > > > > + */
> > > > >
> > > > > /* Part specific fixup hooks. */
> > > > > const struct spi_nor_fixups *fixups;
> > > > > @@ -1786,7 +1794,16 @@ static int spi_nor_erase(struct
> > > > > mtd_info
> > > > > *mtd,
> > > > > struct erase_info *instr)
> > > > >
> > > > > static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
> > > > > {
> > > > > - return SR_BP2 | SR_BP1 | SR_BP0;
> > > > > + u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > > > > +
> > > >
> > > > can we just use the SR_BP3 eg:
> > > >
> > > > u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > > > if (nor->flags & SNOR_F_HAS_4BIT_BP)
> > > > mask |= SR_BP3;
> > > > return mask;
> > > >
> > > >
> > >
> > > I'd prefer this one too if we can, but there are some places to
> > > need
> > > the real mask value. It is also used in other places such as
> > > spi_nor_sr_lock/unlock.
> > >
> > > > > + if (nor->flags & SNOR_F_HAS_4BIT_BP) {
> > > > > + if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
> > > > > + mask = mask | SR_BP3_BIT6;
> > > > > + else
> > > > > + mask = mask | SR_BP3_BIT5;
> > > > > + }
> > > > > +
> > > > > + return mask;
> > > > > }
> > > > >
> > > > > static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
> > > > > @@ -1797,12 +1814,26 @@ static u8 spi_nor_get_tb_mask(struct
> > > > > spi_nor
> > > > > *nor)
> > > > > return SR_TB_BIT5;
> > > > > }
> > > > >
> > > > > +static u8 stm_get_bpval_from_sr(struct spi_nor *nor, u8 sr)
> > > > > {
> > > > > + u8 mask = spi_nor_get_bp_mask(nor);
> > > > > + u8 bp = sr & mask;
> > > > > +
> > > > > + if (bp & SR_BP3_BIT6)
> > > > > + bp = (bp & ~BIT(6)) | BIT(5);
> > > > > +
> > > > > + bp = bp >> SR_BP_SHIFT;
> > > > > +
> > > > > + return bp;
> > > > > +}
> > > >
> > > > Don't convert this. It makes the code really hard to read. See
> > > > below.
> > > >
> > > > > +
> > > > > static int stm_get_min_prot_length(struct spi_nor *nor)
> > > > > {
> > > > > int bp_slots, bp_slots_needed;
> > > > > - u8 mask = spi_nor_get_bp_mask(nor);
> > > > >
> > > > > - bp_slots = (mask >> SR_BP_SHIFT) + 1;
> > > >
> > > > Then just keep this.
> > > >
> > > > > + if (nor->flags & SNOR_F_HAS_4BIT_BP)
> > > > > + bp_slots = 1 << 4;
> > > > > + else
> > > > > + bp_slots = 1 << 3;
> > > > >
> > > > > /* Reserved one for "protect none" and one for "protect
> > > > > all".
> > > > > */
> > > > > bp_slots = bp_slots - 2;
> > > > > @@ -1821,9 +1852,8 @@ static void stm_get_locked_range(struct
> > > > > spi_nor
> > > > > *nor, u8 sr, loff_t *ofs,
> > > > > {
> > > > > struct mtd_info *mtd = &nor->mtd;
> > > > > int min_prot_len;
> > > > > - u8 mask = spi_nor_get_bp_mask(nor);
> > > > > u8 tb_mask = spi_nor_get_tb_mask(nor);
> > > > > - u8 bp = (sr & mask) >> SR_BP_SHIFT;
> > > > > + u8 bp = stm_get_bpval_from_sr(nor, sr);
> > > >
> > > > also this.
> > > >
> >
> > hmm, it looks like we still need some convertion here to get the
> > exact
> > bp value.
>
> OK I see. What has confused me before was that some "fixups" were
> done
> in helper functions whereas others where done in the actual
> stm_lock/stm_unlock().
>
> What do you think about:
>
> (1) read the BP bits, if they are not consecutive move them around
> and
> normalize the value, eg. fix them up to be
> SR_BP3 | SR_BP2 | SR_BP1 | SR_BP0 >> SR_BP_SHIFT
> (2) do the operations on these bits, eg this should not shift the
> value by SR_BP_SHIFT anymore. This would be done in (3)
> (3) convert it back to the representation the flash would need it.
>
> So there could be a function spi_nor_get_bp_val() for (1) and
> spi_nor_set_bp_val() for (3).
>
> u8 spi_nor_get_bp_val(u8 sr)
> {
> u8 val;
>
> val = sr & (SR_BP2 | SR_BP1 | SR_BP0);
>
> if (nor->flags & SNOR_F_HAS_4BIT_BP)
> if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && sr & SR_BP3_BIT6)
> val |= SR_BP3;
> else
> val |= sr & SR_BP3
>
> return val >> SR_BP_SHIFT;
> }
>
> void spi_nor_set_bp_val(u8 val, u8 *sr)
> {
> u8 bp3_bit;
>
> val <<= SR_BP_SHIFT;
>
> /* clear and set common bits */
> *sr &= ~(SR_BP2 | SR_BP1 | SR_BP0);
> *sr |= val & (SR_BP2 | SR_BP1 | SR_BP0)
>
> /* BP3 is special because it may be on different bits */
> if (nor->flags & SNOR_F_HAS_4BIT_BP) {
> if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
> bp3_bit = SR_BP3_BIT6;
> else
> bp3_bit = SR_BP3;
>
> if (val & SR_BP3)
> *sr |= bp3_bit;
> else
> *sr &= ~bp3_bit;
> }
> }
>
> This way we'd have all the fixups in one place.
>
It's good suggestion, but if there are no additional use cases, it
would be good idea to place them within without using helper function.
I'm going to post new patch soon.
Thanks,
>
> -michael
>
> > > > u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > > > if (nor->flags & SNOR_F_HAS_4BIT_BP)
> > > > mask |= SR_BP3;
> > > > return mask;
>
>
> }
>
>
> -michael
>
> >
> > Thanks,
> >
> > > > >
> > > > > if (!bp) {
> > > > > /* No protection */
> > > > > @@ -1881,7 +1911,7 @@ static int stm_is_unlocked_sr(struct
> > > > > spi_nor
> > > > > *nor, loff_t ofs, uint64_t len,
> > > > >
> > > > > /*
> > > > > * Lock a region of the flash. Compatible with ST Micro and
> > > > > similar
> > > > > flash.
> > > > > - * Supports the block protection bits BP{0,1,2} in the
> > > > > status
> > > > > register
> > > > > + * Supports the block protection bits BP{0,1,2}/BP{0,1,2,3}
> > > > > in
> > > > > the
> > > > > status register
> > > > > * (SR). Does not support these features found in newer SR
> > > > > bitfields:
> > > > > * - SEC: sector/block protect - only handle SEC=0 (block
> > > > > protect)
> > > > > * - CMP: complement protect - only support CMP=0 (range
> > > > > is
> > > > > not
> > > > > complemented)
> > > > > @@ -1889,7 +1919,7 @@ static int stm_is_unlocked_sr(struct
> > > > > spi_nor
> > > > > *nor, loff_t ofs, uint64_t len,
> > > > > * Support for the following is provided conditionally for
> > > > > some
> > > > > flash:
> > > > > * - TB: top/bottom protect
> > > > > *
> > > > > - * Sample table portion for 8MB flash (Winbond w25q64fw):
> > > > > + * Sample table portion for 8MB flash (Winbond w25q64fw /
> > > > > BP0-
> > > > > 2):
> > > > > *
> > > > > * SEC | TB | BP2 | BP1 | BP0 | Prot Length |
> > > > > Protected
> > > > > Portion
> > > > > *
> > > > > -----------------------------------------------------------
> > > > > ------
> > > > > ---------
> > > > > @@ -1909,6 +1939,32 @@ static int stm_is_unlocked_sr(struct
> > > > > spi_nor
> > > > > *nor, loff_t ofs, uint64_t len,
> > > > > * 0 | 1 | 1 | 0 | 1 | 2 MB |
> > > > > Lower
> > > > > 1/4
> > > > > * 0 | 1 | 1 | 1 | 0 | 4 MB |
> > > > > Lower
> > > > > 1/2
> > > > > *
> > > > > + * Sample table portion for 64MB flash (Micron n25q512ax3 /
> > > > > BP0-
> > > > > 3):
> > > > > + *
> > > > > + * TB | BP3 | BP2 | BP1 | BP0 | Prot Length |
> > > > > Protected
> > > > > Portion
> > > > > + *
> > > > > -----------------------------------------------------------
> > > > > ------
> > > > > ---------
> > > > > + * 0 | 0 | 0 | 0 | 0 | NONE |
> > > > > NONE
> > > > > + * 0 | 0 | 0 | 0 | 1 | 64 KB |
> > > > > Upper
> > > > > 1/1024
> > > > > + * 0 | 0 | 0 | 1 | 0 | 128 KB |
> > > > > Upper
> > > > > 1/512
> > > > > + * 0 | 0 | 0 | 1 | 1 | 256 KB |
> > > > > Upper
> > > > > 1/256
> > > > > + * ...
> > > > > + * 0 | 1 | 0 | 0 | 1 | 16 MB |
> > > > > Upper
> > > > > 1/4
> > > > > + * 0 | 1 | 0 | 1 | 0 | 32 MB |
> > > > > Upper
> > > > > 1/2
> > > > > + * 0 | 1 | 0 | 1 | 1 | 64 MB |
> > > > > ALL
> > > > > + * 0 | 1 | 1 | 0 | 0 | 64 MB |
> > > > > ALL
> > > > > + * ...
> > > > > + *
> > > > > ------|-------|-------|-------|-------|---------------|----
> > > > > ------
> > > > > ---------
> > > > > + * 1 | 0 | 0 | 0 | 0 | NONE |
> > > > > NONE
> > > > > + * 1 | 0 | 0 | 0 | 1 | 64 KB |
> > > > > Lower
> > > > > 1/1024
> > > > > + * 1 | 0 | 0 | 1 | 0 | 128 KB |
> > > > > Lower
> > > > > 1/512
> > > > > + * 1 | 0 | 0 | 1 | 1 | 256 KB |
> > > > > Lower
> > > > > 1/256
> > > > > + * ...
> > > > > + * 1 | 1 | 0 | 0 | 1 | 16 MB |
> > > > > Lower
> > > > > 1/4
> > > > > + * 1 | 1 | 0 | 1 | 0 | 32 MB |
> > > > > Lower
> > > > > 1/2
> > > > > + * 1 | 1 | 0 | 1 | 1 | 64 MB |
> > > > > ALL
> > > > > + * 1 | 1 | 1 | 0 | 0 | 64 MB |
> > > > > ALL
> > > > > + * ...
> > > > > + *
> > > > > * Returns negative on errors, 0 on success.
> > > > > */
> > > > > static int stm_lock(struct spi_nor *nor, loff_t ofs,
> > > > > uint64_t
> > > > > len)
> > > > > @@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor
> > > > > *nor,
> > > > > loff_t
> > > > > ofs, uint64_t len)
> > > > > min_prot_len = stm_get_min_prot_length(nor);
> > > > > pow = ilog2(lock_len) - ilog2(min_prot_len) +
> > > > > 1;
> > > > > val = pow << SR_BP_SHIFT;
> > > > > +
> > > > > + if (val & BIT(5) && mask & SR_BP3_BIT6)
> > > > > + val = (val & ~BIT(5)) | BIT(6);
> > > >
> > > > .. and the use just the following here:
> > > >
> > > > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
> > > > val = (val & ~SR_BP3) | SR_BP3_BIT6;
> > > >
> > > > Ie. just use the "normal case" where all BP bits are next to
> > > > each
> > > > other
> > > > and then fixup the resulting value and shift the SR3 bit if
> > > > necessary.
> > > > This will be much easier to read.
> > > >
> > >
> > > Yes, I agree. It would be better to minimize this kind of
> > > conversion
> > > in
> > > one place.
> > >
> > > > > }
> > > > >
> > > > > if (val & ~mask)
> > > > > @@ -2042,6 +2101,9 @@ static int stm_unlock(struct spi_nor
> > > > > *nor,
> > > > > loff_t ofs, uint64_t len)
> > > > > pow = ilog2(lock_len) - ilog2(min_prot_len) +
> > > > > 1;
> > > > > val = pow << SR_BP_SHIFT;
> > > > >
> > > > > + if (val & BIT(5) && mask & SR_BP3_BIT6)
> > > > > + val = (val & ~BIT(5)) | BIT(6);
> > > > > +
> > > >
> > > > here would be the other way around:
> > > >
> > > > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6)
> > > > val = (val & ~SR_BP3_BIT6) | SR_BP3;
> > > >
> > > >
> > > > > /* Some power-of-two sizes are not supported */
> > > > > if (val & ~mask)
> > > > > return -EINVAL;
> > > > > @@ -5244,6 +5306,12 @@ int spi_nor_scan(struct spi_nor *nor,
> > > > > const
> > > > > char
> > > > > *name,
> > > > > if (info->flags & USE_CLSR)
> > > > > nor->flags |= SNOR_F_USE_CLSR;
> > > > >
> > > > > + if (info->flags & SPI_NOR_4BIT_BP) {
> > > > > + nor->flags |= SNOR_F_HAS_4BIT_BP;
> > > > > + if (info->flags & SPI_NOR_BP3_SR_BIT6)
> > > > > + nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
> > > > > + }
> > > > > +
> > > > > if (info->flags & SPI_NOR_NO_ERASE)
> > > > > mtd->flags |= MTD_NO_ERASE;
> > > > >
> > > > > diff --git a/include/linux/mtd/spi-nor.h
> > > > > b/include/linux/mtd/spi-
> > > > > nor.h
> > > > > index de90724f62f1..0190ed21576a 100644
> > > > > --- a/include/linux/mtd/spi-nor.h
> > > > > +++ b/include/linux/mtd/spi-nor.h
> > > > > @@ -129,7 +129,9 @@
> > > > > #define SR_BP1 BIT(3) /* Block
> > > > > protect
> > > > > 1
> > > > > */
> > > > > #define SR_BP2 BIT(4) /* Block
> > > > > protect
> > > > > 2
> > > > > */
> > > > > #define SR_TB_BIT5 BIT(5) /* Top/Bottom
> > > > > protect */
> > > > > +#define SR_BP3_BIT5 BIT(5) /* Block protect
> > > > > 3
> > > > > */
> > > >
> > > > IMHO just SR_BP3. but that is a matter of taste. But it is
> > > > easier
> > > > on
> > > > the eye in the mask = SR_BP3 | SR_BP2 etc case.
> > > >
> > >
> > > SR_BP3 would be a more appropriate name if we could set the case
> > > with
> > > all BP bits next to each other as the "normal case."
> > >
> > > I'm going to write patches based on latest spi-nor/next including
> > > what
> > > you mentioned.
> > >
> > > Thanks,
> > >
> > > > -michael
> > > >
> > > > > #define SR_TB_BIT6 BIT(6) /* Top/Bottom
> > > > > protect */
> > > > > +#define SR_BP3_BIT6 BIT(6) /* Block protect
> > > > > 3
> > > > > */
> > > > > #define SR_SRWD BIT(7) /* SR write
> > > > > protect
> > > > > */
> > > > > /* Spansion/Cypress specific status bits */
> > > > > #define SR_E_ERR BIT(5)
> > > > > @@ -240,6 +242,8 @@ enum spi_nor_option_flags {
> > > > > SNOR_F_HAS_16BIT_SR = BIT(9),
> > > > > SNOR_F_NO_READ_CR = BIT(10),
> > > > > SNOR_F_HAS_SR_TB_BIT6 = BIT(11),
> > > > > + SNOR_F_HAS_4BIT_BP = BIT(12),
> > > > > + SNOR_F_HAS_SR_BP3_BIT6 = BIT(13),
> > > > >
> > > > > };
> > >
> > >
>
>
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] mtd: spi-nor: add 4bit block protection support
2020-03-17 11:35 ` Jungseung Lee
@ 2020-03-17 14:52 ` Michael Walle
2020-03-18 6:01 ` Jungseung Lee
0 siblings, 1 reply; 9+ messages in thread
From: Michael Walle @ 2020-03-17 14:52 UTC (permalink / raw)
To: Jungseung Lee
Cc: chenxiang, js07.lee, linux-mtd, Vignesh Raghavendra, Tudor Ambarus
Am 2020-03-17 12:35, schrieb Jungseung Lee:
> Hi, Micahel
>
> On Tue, 2020-03-17 at 20:00 +0900, Jungseung Lee wrote:
>> Hi, Michael,
>>
>> On Fri, 2020-03-13 at 17:24 +0100, Michael Walle wrote:
>> > Hi Jungseung,
>> >
>> > sorry for the late review.
>> >
>>
>> Not at all. thanks for your review.
>>
>> > Am 2020-03-04 12:07, schrieb Jungseung Lee:
>> > > Currently, we are supporting block protection only for
>> > > flash chips with 3 block protection bits in the SR register.
>> > > This patch enables block protection support for flashes with
>> > > 4 block protection bits(bp0-3).
>> > >
>> > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
>> > > ---
>> > > drivers/mtd/spi-nor/spi-nor.c | 82
>> > > ++++++++++++++++++++++++++++++++---
>> > > include/linux/mtd/spi-nor.h | 4 ++
>> > > 2 files changed, 79 insertions(+), 7 deletions(-)
>> > >
>> > > diff --git a/drivers/mtd/spi-nor/spi-nor.c
>> > > b/drivers/mtd/spi-nor/spi-nor.c
>> > > index c58c27552a74..31a2106e529a 100644
>> > > --- a/drivers/mtd/spi-nor/spi-nor.c
>> > > +++ b/drivers/mtd/spi-nor/spi-nor.c
>> > > @@ -238,6 +238,14 @@ struct flash_info {
>> > > * status register. Must be
>> > > used with
>> > > * SPI_NOR_HAS_TB.
>> > > */
>> > > +#define SPI_NOR_4BIT_BP BIT(17) /*
>> > > + * Flash SR has 4 bit fields
>> > > (BP0-3)
>> > > + * for block protection.
>> > > + */
>> > > +#define SPI_NOR_BP3_SR_BIT6 BIT(18) /*
>> > > + * BP3 is bit 6 of status
>> > > register.
>> > > + * Must be used with
>> > > SPI_NOR_4BIT_BP.
>> > > + */
>> > >
>> > > /* Part specific fixup hooks. */
>> > > const struct spi_nor_fixups *fixups;
>> > > @@ -1786,7 +1794,16 @@ static int spi_nor_erase(struct mtd_info
>> > > *mtd,
>> > > struct erase_info *instr)
>> > >
>> > > static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
>> > > {
>> > > - return SR_BP2 | SR_BP1 | SR_BP0;
>> > > + u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
>> > > +
>> >
>> > can we just use the SR_BP3 eg:
>> >
>> > u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
>> > if (nor->flags & SNOR_F_HAS_4BIT_BP)
>> > mask |= SR_BP3;
>> > return mask;
>> >
>> >
>>
>> I'd prefer this one too if we can, but there are some places to need
>> the real mask value. It is also used in other places such as
>> spi_nor_sr_lock/unlock.
>>
>> > > + if (nor->flags & SNOR_F_HAS_4BIT_BP) {
>> > > + if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
>> > > + mask = mask | SR_BP3_BIT6;
>> > > + else
>> > > + mask = mask | SR_BP3_BIT5;
>> > > + }
>> > > +
>> > > + return mask;
>> > > }
>> > >
>> > > static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
>> > > @@ -1797,12 +1814,26 @@ static u8 spi_nor_get_tb_mask(struct
>> > > spi_nor
>> > > *nor)
>> > > return SR_TB_BIT5;
>> > > }
>> > >
>> > > +static u8 stm_get_bpval_from_sr(struct spi_nor *nor, u8 sr) {
>> > > + u8 mask = spi_nor_get_bp_mask(nor);
>> > > + u8 bp = sr & mask;
>> > > +
>> > > + if (bp & SR_BP3_BIT6)
>> > > + bp = (bp & ~BIT(6)) | BIT(5);
>> > > +
>> > > + bp = bp >> SR_BP_SHIFT;
>> > > +
>> > > + return bp;
>> > > +}
>> >
>> > Don't convert this. It makes the code really hard to read. See
>> > below.
>> >
>> > > +
>> > > static int stm_get_min_prot_length(struct spi_nor *nor)
>> > > {
>> > > int bp_slots, bp_slots_needed;
>> > > - u8 mask = spi_nor_get_bp_mask(nor);
>> > >
>> > > - bp_slots = (mask >> SR_BP_SHIFT) + 1;
>> >
>> > Then just keep this.
>> >
>> > > + if (nor->flags & SNOR_F_HAS_4BIT_BP)
>> > > + bp_slots = 1 << 4;
>> > > + else
>> > > + bp_slots = 1 << 3;
>> > >
>> > > /* Reserved one for "protect none" and one for "protect all".
>> > > */
>> > > bp_slots = bp_slots - 2;
>> > > @@ -1821,9 +1852,8 @@ static void stm_get_locked_range(struct
>> > > spi_nor
>> > > *nor, u8 sr, loff_t *ofs,
>> > > {
>> > > struct mtd_info *mtd = &nor->mtd;
>> > > int min_prot_len;
>> > > - u8 mask = spi_nor_get_bp_mask(nor);
>> > > u8 tb_mask = spi_nor_get_tb_mask(nor);
>> > > - u8 bp = (sr & mask) >> SR_BP_SHIFT;
>> > > + u8 bp = stm_get_bpval_from_sr(nor, sr);
>> >
>> > also this.
>> >
>
> hmm, it looks like we still need some convertion here to get the exact
> bp value.
OK I see. What has confused me before was that some "fixups" were done
in helper functions whereas others where done in the actual
stm_lock/stm_unlock().
What do you think about:
(1) read the BP bits, if they are not consecutive move them around and
normalize the value, eg. fix them up to be
SR_BP3 | SR_BP2 | SR_BP1 | SR_BP0 >> SR_BP_SHIFT
(2) do the operations on these bits, eg this should not shift the
value by SR_BP_SHIFT anymore. This would be done in (3)
(3) convert it back to the representation the flash would need it.
So there could be a function spi_nor_get_bp_val() for (1) and
spi_nor_set_bp_val() for (3).
u8 spi_nor_get_bp_val(u8 sr)
{
u8 val;
val = sr & (SR_BP2 | SR_BP1 | SR_BP0);
if (nor->flags & SNOR_F_HAS_4BIT_BP)
if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && sr & SR_BP3_BIT6)
val |= SR_BP3;
else
val |= sr & SR_BP3
return val >> SR_BP_SHIFT;
}
void spi_nor_set_bp_val(u8 val, u8 *sr)
{
u8 bp3_bit;
val <<= SR_BP_SHIFT;
/* clear and set common bits */
*sr &= ~(SR_BP2 | SR_BP1 | SR_BP0);
*sr |= val & (SR_BP2 | SR_BP1 | SR_BP0)
/* BP3 is special because it may be on different bits */
if (nor->flags & SNOR_F_HAS_4BIT_BP) {
if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
bp3_bit = SR_BP3_BIT6;
else
bp3_bit = SR_BP3;
if (val & SR_BP3)
*sr |= bp3_bit;
else
*sr &= ~bp3_bit;
}
}
This way we'd have all the fixups in one place.
-michael
>> > u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
>> > if (nor->flags & SNOR_F_HAS_4BIT_BP)
>> > mask |= SR_BP3;
>> > return mask;
}
-michael
>
> Thanks,
>
>> > >
>> > > if (!bp) {
>> > > /* No protection */
>> > > @@ -1881,7 +1911,7 @@ static int stm_is_unlocked_sr(struct
>> > > spi_nor
>> > > *nor, loff_t ofs, uint64_t len,
>> > >
>> > > /*
>> > > * Lock a region of the flash. Compatible with ST Micro and
>> > > similar
>> > > flash.
>> > > - * Supports the block protection bits BP{0,1,2} in the status
>> > > register
>> > > + * Supports the block protection bits BP{0,1,2}/BP{0,1,2,3} in
>> > > the
>> > > status register
>> > > * (SR). Does not support these features found in newer SR
>> > > bitfields:
>> > > * - SEC: sector/block protect - only handle SEC=0 (block
>> > > protect)
>> > > * - CMP: complement protect - only support CMP=0 (range is
>> > > not
>> > > complemented)
>> > > @@ -1889,7 +1919,7 @@ static int stm_is_unlocked_sr(struct
>> > > spi_nor
>> > > *nor, loff_t ofs, uint64_t len,
>> > > * Support for the following is provided conditionally for some
>> > > flash:
>> > > * - TB: top/bottom protect
>> > > *
>> > > - * Sample table portion for 8MB flash (Winbond w25q64fw):
>> > > + * Sample table portion for 8MB flash (Winbond w25q64fw / BP0-
>> > > 2):
>> > > *
>> > > * SEC | TB | BP2 | BP1 | BP0 | Prot Length |
>> > > Protected
>> > > Portion
>> > > *
>> > > -----------------------------------------------------------------
>> > > ---------
>> > > @@ -1909,6 +1939,32 @@ static int stm_is_unlocked_sr(struct
>> > > spi_nor
>> > > *nor, loff_t ofs, uint64_t len,
>> > > * 0 | 1 | 1 | 0 | 1 | 2 MB |
>> > > Lower
>> > > 1/4
>> > > * 0 | 1 | 1 | 1 | 0 | 4 MB |
>> > > Lower
>> > > 1/2
>> > > *
>> > > + * Sample table portion for 64MB flash (Micron n25q512ax3 / BP0-
>> > > 3):
>> > > + *
>> > > + * TB | BP3 | BP2 | BP1 | BP0 | Prot Length |
>> > > Protected
>> > > Portion
>> > > + *
>> > > -----------------------------------------------------------------
>> > > ---------
>> > > + * 0 | 0 | 0 | 0 | 0 | NONE | NONE
>> > > + * 0 | 0 | 0 | 0 | 1 | 64 KB |
>> > > Upper
>> > > 1/1024
>> > > + * 0 | 0 | 0 | 1 | 0 | 128 KB |
>> > > Upper
>> > > 1/512
>> > > + * 0 | 0 | 0 | 1 | 1 | 256 KB |
>> > > Upper
>> > > 1/256
>> > > + * ...
>> > > + * 0 | 1 | 0 | 0 | 1 | 16 MB |
>> > > Upper
>> > > 1/4
>> > > + * 0 | 1 | 0 | 1 | 0 | 32 MB |
>> > > Upper
>> > > 1/2
>> > > + * 0 | 1 | 0 | 1 | 1 | 64 MB | ALL
>> > > + * 0 | 1 | 1 | 0 | 0 | 64 MB | ALL
>> > > + * ...
>> > > + *
>> > > ------|-------|-------|-------|-------|---------------|----------
>> > > ---------
>> > > + * 1 | 0 | 0 | 0 | 0 | NONE | NONE
>> > > + * 1 | 0 | 0 | 0 | 1 | 64 KB |
>> > > Lower
>> > > 1/1024
>> > > + * 1 | 0 | 0 | 1 | 0 | 128 KB |
>> > > Lower
>> > > 1/512
>> > > + * 1 | 0 | 0 | 1 | 1 | 256 KB |
>> > > Lower
>> > > 1/256
>> > > + * ...
>> > > + * 1 | 1 | 0 | 0 | 1 | 16 MB |
>> > > Lower
>> > > 1/4
>> > > + * 1 | 1 | 0 | 1 | 0 | 32 MB |
>> > > Lower
>> > > 1/2
>> > > + * 1 | 1 | 0 | 1 | 1 | 64 MB | ALL
>> > > + * 1 | 1 | 1 | 0 | 0 | 64 MB | ALL
>> > > + * ...
>> > > + *
>> > > * Returns negative on errors, 0 on success.
>> > > */
>> > > static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t
>> > > len)
>> > > @@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor *nor,
>> > > loff_t
>> > > ofs, uint64_t len)
>> > > min_prot_len = stm_get_min_prot_length(nor);
>> > > pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
>> > > val = pow << SR_BP_SHIFT;
>> > > +
>> > > + if (val & BIT(5) && mask & SR_BP3_BIT6)
>> > > + val = (val & ~BIT(5)) | BIT(6);
>> >
>> > .. and the use just the following here:
>> >
>> > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
>> > val = (val & ~SR_BP3) | SR_BP3_BIT6;
>> >
>> > Ie. just use the "normal case" where all BP bits are next to each
>> > other
>> > and then fixup the resulting value and shift the SR3 bit if
>> > necessary.
>> > This will be much easier to read.
>> >
>>
>> Yes, I agree. It would be better to minimize this kind of conversion
>> in
>> one place.
>>
>> > > }
>> > >
>> > > if (val & ~mask)
>> > > @@ -2042,6 +2101,9 @@ static int stm_unlock(struct spi_nor *nor,
>> > > loff_t ofs, uint64_t len)
>> > > pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
>> > > val = pow << SR_BP_SHIFT;
>> > >
>> > > + if (val & BIT(5) && mask & SR_BP3_BIT6)
>> > > + val = (val & ~BIT(5)) | BIT(6);
>> > > +
>> >
>> > here would be the other way around:
>> >
>> > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6)
>> > val = (val & ~SR_BP3_BIT6) | SR_BP3;
>> >
>> >
>> > > /* Some power-of-two sizes are not supported */
>> > > if (val & ~mask)
>> > > return -EINVAL;
>> > > @@ -5244,6 +5306,12 @@ int spi_nor_scan(struct spi_nor *nor,
>> > > const
>> > > char
>> > > *name,
>> > > if (info->flags & USE_CLSR)
>> > > nor->flags |= SNOR_F_USE_CLSR;
>> > >
>> > > + if (info->flags & SPI_NOR_4BIT_BP) {
>> > > + nor->flags |= SNOR_F_HAS_4BIT_BP;
>> > > + if (info->flags & SPI_NOR_BP3_SR_BIT6)
>> > > + nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
>> > > + }
>> > > +
>> > > if (info->flags & SPI_NOR_NO_ERASE)
>> > > mtd->flags |= MTD_NO_ERASE;
>> > >
>> > > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-
>> > > nor.h
>> > > index de90724f62f1..0190ed21576a 100644
>> > > --- a/include/linux/mtd/spi-nor.h
>> > > +++ b/include/linux/mtd/spi-nor.h
>> > > @@ -129,7 +129,9 @@
>> > > #define SR_BP1 BIT(3) /* Block protect
>> > > 1
>> > > */
>> > > #define SR_BP2 BIT(4) /* Block protect
>> > > 2
>> > > */
>> > > #define SR_TB_BIT5 BIT(5) /* Top/Bottom
>> > > protect */
>> > > +#define SR_BP3_BIT5 BIT(5) /* Block protect
>> > > 3
>> > > */
>> >
>> > IMHO just SR_BP3. but that is a matter of taste. But it is easier
>> > on
>> > the eye in the mask = SR_BP3 | SR_BP2 etc case.
>> >
>>
>> SR_BP3 would be a more appropriate name if we could set the case with
>> all BP bits next to each other as the "normal case."
>>
>> I'm going to write patches based on latest spi-nor/next including
>> what
>> you mentioned.
>>
>> Thanks,
>>
>> > -michael
>> >
>> > > #define SR_TB_BIT6 BIT(6) /* Top/Bottom
>> > > protect */
>> > > +#define SR_BP3_BIT6 BIT(6) /* Block protect
>> > > 3
>> > > */
>> > > #define SR_SRWD BIT(7) /* SR write
>> > > protect
>> > > */
>> > > /* Spansion/Cypress specific status bits */
>> > > #define SR_E_ERR BIT(5)
>> > > @@ -240,6 +242,8 @@ enum spi_nor_option_flags {
>> > > SNOR_F_HAS_16BIT_SR = BIT(9),
>> > > SNOR_F_NO_READ_CR = BIT(10),
>> > > SNOR_F_HAS_SR_TB_BIT6 = BIT(11),
>> > > + SNOR_F_HAS_4BIT_BP = BIT(12),
>> > > + SNOR_F_HAS_SR_BP3_BIT6 = BIT(13),
>> > >
>> > > };
>>
>>
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] mtd: spi-nor: add 4bit block protection support
2020-03-17 11:00 ` Jungseung Lee
@ 2020-03-17 11:35 ` Jungseung Lee
2020-03-17 14:52 ` Michael Walle
0 siblings, 1 reply; 9+ messages in thread
From: Jungseung Lee @ 2020-03-17 11:35 UTC (permalink / raw)
To: Michael Walle
Cc: Vignesh Raghavendra, chenxiang, Tudor Ambarus, linux-mtd,
js07.lee, js07.lee
Hi, Micahel
On Tue, 2020-03-17 at 20:00 +0900, Jungseung Lee wrote:
> Hi, Michael,
>
> On Fri, 2020-03-13 at 17:24 +0100, Michael Walle wrote:
> > Hi Jungseung,
> >
> > sorry for the late review.
> >
>
> Not at all. thanks for your review.
>
> > Am 2020-03-04 12:07, schrieb Jungseung Lee:
> > > Currently, we are supporting block protection only for
> > > flash chips with 3 block protection bits in the SR register.
> > > This patch enables block protection support for flashes with
> > > 4 block protection bits(bp0-3).
> > >
> > > Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> > > ---
> > > drivers/mtd/spi-nor/spi-nor.c | 82
> > > ++++++++++++++++++++++++++++++++---
> > > include/linux/mtd/spi-nor.h | 4 ++
> > > 2 files changed, 79 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > > b/drivers/mtd/spi-nor/spi-nor.c
> > > index c58c27552a74..31a2106e529a 100644
> > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > > @@ -238,6 +238,14 @@ struct flash_info {
> > > * status register. Must be
> > > used with
> > > * SPI_NOR_HAS_TB.
> > > */
> > > +#define SPI_NOR_4BIT_BP BIT(17) /*
> > > + * Flash SR has 4 bit fields
> > > (BP0-3)
> > > + * for block protection.
> > > + */
> > > +#define SPI_NOR_BP3_SR_BIT6 BIT(18) /*
> > > + * BP3 is bit 6 of status
> > > register.
> > > + * Must be used with
> > > SPI_NOR_4BIT_BP.
> > > + */
> > >
> > > /* Part specific fixup hooks. */
> > > const struct spi_nor_fixups *fixups;
> > > @@ -1786,7 +1794,16 @@ static int spi_nor_erase(struct mtd_info
> > > *mtd,
> > > struct erase_info *instr)
> > >
> > > static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
> > > {
> > > - return SR_BP2 | SR_BP1 | SR_BP0;
> > > + u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > > +
> >
> > can we just use the SR_BP3 eg:
> >
> > u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > if (nor->flags & SNOR_F_HAS_4BIT_BP)
> > mask |= SR_BP3;
> > return mask;
> >
> >
>
> I'd prefer this one too if we can, but there are some places to need
> the real mask value. It is also used in other places such as
> spi_nor_sr_lock/unlock.
>
> > > + if (nor->flags & SNOR_F_HAS_4BIT_BP) {
> > > + if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
> > > + mask = mask | SR_BP3_BIT6;
> > > + else
> > > + mask = mask | SR_BP3_BIT5;
> > > + }
> > > +
> > > + return mask;
> > > }
> > >
> > > static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
> > > @@ -1797,12 +1814,26 @@ static u8 spi_nor_get_tb_mask(struct
> > > spi_nor
> > > *nor)
> > > return SR_TB_BIT5;
> > > }
> > >
> > > +static u8 stm_get_bpval_from_sr(struct spi_nor *nor, u8 sr) {
> > > + u8 mask = spi_nor_get_bp_mask(nor);
> > > + u8 bp = sr & mask;
> > > +
> > > + if (bp & SR_BP3_BIT6)
> > > + bp = (bp & ~BIT(6)) | BIT(5);
> > > +
> > > + bp = bp >> SR_BP_SHIFT;
> > > +
> > > + return bp;
> > > +}
> >
> > Don't convert this. It makes the code really hard to read. See
> > below.
> >
> > > +
> > > static int stm_get_min_prot_length(struct spi_nor *nor)
> > > {
> > > int bp_slots, bp_slots_needed;
> > > - u8 mask = spi_nor_get_bp_mask(nor);
> > >
> > > - bp_slots = (mask >> SR_BP_SHIFT) + 1;
> >
> > Then just keep this.
> >
> > > + if (nor->flags & SNOR_F_HAS_4BIT_BP)
> > > + bp_slots = 1 << 4;
> > > + else
> > > + bp_slots = 1 << 3;
> > >
> > > /* Reserved one for "protect none" and one for "protect all".
> > > */
> > > bp_slots = bp_slots - 2;
> > > @@ -1821,9 +1852,8 @@ static void stm_get_locked_range(struct
> > > spi_nor
> > > *nor, u8 sr, loff_t *ofs,
> > > {
> > > struct mtd_info *mtd = &nor->mtd;
> > > int min_prot_len;
> > > - u8 mask = spi_nor_get_bp_mask(nor);
> > > u8 tb_mask = spi_nor_get_tb_mask(nor);
> > > - u8 bp = (sr & mask) >> SR_BP_SHIFT;
> > > + u8 bp = stm_get_bpval_from_sr(nor, sr);
> >
> > also this.
> >
hmm, it looks like we still need some convertion here to get the exact
bp value.
Thanks,
> > >
> > > if (!bp) {
> > > /* No protection */
> > > @@ -1881,7 +1911,7 @@ static int stm_is_unlocked_sr(struct
> > > spi_nor
> > > *nor, loff_t ofs, uint64_t len,
> > >
> > > /*
> > > * Lock a region of the flash. Compatible with ST Micro and
> > > similar
> > > flash.
> > > - * Supports the block protection bits BP{0,1,2} in the status
> > > register
> > > + * Supports the block protection bits BP{0,1,2}/BP{0,1,2,3} in
> > > the
> > > status register
> > > * (SR). Does not support these features found in newer SR
> > > bitfields:
> > > * - SEC: sector/block protect - only handle SEC=0 (block
> > > protect)
> > > * - CMP: complement protect - only support CMP=0 (range is
> > > not
> > > complemented)
> > > @@ -1889,7 +1919,7 @@ static int stm_is_unlocked_sr(struct
> > > spi_nor
> > > *nor, loff_t ofs, uint64_t len,
> > > * Support for the following is provided conditionally for some
> > > flash:
> > > * - TB: top/bottom protect
> > > *
> > > - * Sample table portion for 8MB flash (Winbond w25q64fw):
> > > + * Sample table portion for 8MB flash (Winbond w25q64fw / BP0-
> > > 2):
> > > *
> > > * SEC | TB | BP2 | BP1 | BP0 | Prot Length |
> > > Protected
> > > Portion
> > > *
> > > -----------------------------------------------------------------
> > > ---------
> > > @@ -1909,6 +1939,32 @@ static int stm_is_unlocked_sr(struct
> > > spi_nor
> > > *nor, loff_t ofs, uint64_t len,
> > > * 0 | 1 | 1 | 0 | 1 | 2 MB |
> > > Lower
> > > 1/4
> > > * 0 | 1 | 1 | 1 | 0 | 4 MB |
> > > Lower
> > > 1/2
> > > *
> > > + * Sample table portion for 64MB flash (Micron n25q512ax3 / BP0-
> > > 3):
> > > + *
> > > + * TB | BP3 | BP2 | BP1 | BP0 | Prot Length |
> > > Protected
> > > Portion
> > > + *
> > > -----------------------------------------------------------------
> > > ---------
> > > + * 0 | 0 | 0 | 0 | 0 | NONE | NONE
> > > + * 0 | 0 | 0 | 0 | 1 | 64 KB |
> > > Upper
> > > 1/1024
> > > + * 0 | 0 | 0 | 1 | 0 | 128 KB |
> > > Upper
> > > 1/512
> > > + * 0 | 0 | 0 | 1 | 1 | 256 KB |
> > > Upper
> > > 1/256
> > > + * ...
> > > + * 0 | 1 | 0 | 0 | 1 | 16 MB |
> > > Upper
> > > 1/4
> > > + * 0 | 1 | 0 | 1 | 0 | 32 MB |
> > > Upper
> > > 1/2
> > > + * 0 | 1 | 0 | 1 | 1 | 64 MB | ALL
> > > + * 0 | 1 | 1 | 0 | 0 | 64 MB | ALL
> > > + * ...
> > > + *
> > > ------|-------|-------|-------|-------|---------------|----------
> > > ---------
> > > + * 1 | 0 | 0 | 0 | 0 | NONE | NONE
> > > + * 1 | 0 | 0 | 0 | 1 | 64 KB |
> > > Lower
> > > 1/1024
> > > + * 1 | 0 | 0 | 1 | 0 | 128 KB |
> > > Lower
> > > 1/512
> > > + * 1 | 0 | 0 | 1 | 1 | 256 KB |
> > > Lower
> > > 1/256
> > > + * ...
> > > + * 1 | 1 | 0 | 0 | 1 | 16 MB |
> > > Lower
> > > 1/4
> > > + * 1 | 1 | 0 | 1 | 0 | 32 MB |
> > > Lower
> > > 1/2
> > > + * 1 | 1 | 0 | 1 | 1 | 64 MB | ALL
> > > + * 1 | 1 | 1 | 0 | 0 | 64 MB | ALL
> > > + * ...
> > > + *
> > > * Returns negative on errors, 0 on success.
> > > */
> > > static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t
> > > len)
> > > @@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor *nor,
> > > loff_t
> > > ofs, uint64_t len)
> > > min_prot_len = stm_get_min_prot_length(nor);
> > > pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
> > > val = pow << SR_BP_SHIFT;
> > > +
> > > + if (val & BIT(5) && mask & SR_BP3_BIT6)
> > > + val = (val & ~BIT(5)) | BIT(6);
> >
> > .. and the use just the following here:
> >
> > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
> > val = (val & ~SR_BP3) | SR_BP3_BIT6;
> >
> > Ie. just use the "normal case" where all BP bits are next to each
> > other
> > and then fixup the resulting value and shift the SR3 bit if
> > necessary.
> > This will be much easier to read.
> >
>
> Yes, I agree. It would be better to minimize this kind of conversion
> in
> one place.
>
> > > }
> > >
> > > if (val & ~mask)
> > > @@ -2042,6 +2101,9 @@ static int stm_unlock(struct spi_nor *nor,
> > > loff_t ofs, uint64_t len)
> > > pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
> > > val = pow << SR_BP_SHIFT;
> > >
> > > + if (val & BIT(5) && mask & SR_BP3_BIT6)
> > > + val = (val & ~BIT(5)) | BIT(6);
> > > +
> >
> > here would be the other way around:
> >
> > if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6)
> > val = (val & ~SR_BP3_BIT6) | SR_BP3;
> >
> >
> > > /* Some power-of-two sizes are not supported */
> > > if (val & ~mask)
> > > return -EINVAL;
> > > @@ -5244,6 +5306,12 @@ int spi_nor_scan(struct spi_nor *nor,
> > > const
> > > char
> > > *name,
> > > if (info->flags & USE_CLSR)
> > > nor->flags |= SNOR_F_USE_CLSR;
> > >
> > > + if (info->flags & SPI_NOR_4BIT_BP) {
> > > + nor->flags |= SNOR_F_HAS_4BIT_BP;
> > > + if (info->flags & SPI_NOR_BP3_SR_BIT6)
> > > + nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
> > > + }
> > > +
> > > if (info->flags & SPI_NOR_NO_ERASE)
> > > mtd->flags |= MTD_NO_ERASE;
> > >
> > > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-
> > > nor.h
> > > index de90724f62f1..0190ed21576a 100644
> > > --- a/include/linux/mtd/spi-nor.h
> > > +++ b/include/linux/mtd/spi-nor.h
> > > @@ -129,7 +129,9 @@
> > > #define SR_BP1 BIT(3) /* Block protect
> > > 1
> > > */
> > > #define SR_BP2 BIT(4) /* Block protect
> > > 2
> > > */
> > > #define SR_TB_BIT5 BIT(5) /* Top/Bottom
> > > protect */
> > > +#define SR_BP3_BIT5 BIT(5) /* Block protect
> > > 3
> > > */
> >
> > IMHO just SR_BP3. but that is a matter of taste. But it is easier
> > on
> > the eye in the mask = SR_BP3 | SR_BP2 etc case.
> >
>
> SR_BP3 would be a more appropriate name if we could set the case with
> all BP bits next to each other as the "normal case."
>
> I'm going to write patches based on latest spi-nor/next including
> what
> you mentioned.
>
> Thanks,
>
> > -michael
> >
> > > #define SR_TB_BIT6 BIT(6) /* Top/Bottom
> > > protect */
> > > +#define SR_BP3_BIT6 BIT(6) /* Block protect
> > > 3
> > > */
> > > #define SR_SRWD BIT(7) /* SR write
> > > protect
> > > */
> > > /* Spansion/Cypress specific status bits */
> > > #define SR_E_ERR BIT(5)
> > > @@ -240,6 +242,8 @@ enum spi_nor_option_flags {
> > > SNOR_F_HAS_16BIT_SR = BIT(9),
> > > SNOR_F_NO_READ_CR = BIT(10),
> > > SNOR_F_HAS_SR_TB_BIT6 = BIT(11),
> > > + SNOR_F_HAS_4BIT_BP = BIT(12),
> > > + SNOR_F_HAS_SR_BP3_BIT6 = BIT(13),
> > >
> > > };
>
>
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] mtd: spi-nor: add 4bit block protection support
2020-03-13 16:24 ` Michael Walle
@ 2020-03-17 11:00 ` Jungseung Lee
2020-03-17 11:35 ` Jungseung Lee
0 siblings, 1 reply; 9+ messages in thread
From: Jungseung Lee @ 2020-03-17 11:00 UTC (permalink / raw)
To: Michael Walle
Cc: Vignesh Raghavendra, chenxiang, Tudor Ambarus, linux-mtd,
js07.lee, js07.lee
Hi, Michael,
On Fri, 2020-03-13 at 17:24 +0100, Michael Walle wrote:
> Hi Jungseung,
>
> sorry for the late review.
>
Not at all. thanks for your review.
> Am 2020-03-04 12:07, schrieb Jungseung Lee:
> > Currently, we are supporting block protection only for
> > flash chips with 3 block protection bits in the SR register.
> > This patch enables block protection support for flashes with
> > 4 block protection bits(bp0-3).
> >
> > Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> > ---
> > drivers/mtd/spi-nor/spi-nor.c | 82
> > ++++++++++++++++++++++++++++++++---
> > include/linux/mtd/spi-nor.h | 4 ++
> > 2 files changed, 79 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > b/drivers/mtd/spi-nor/spi-nor.c
> > index c58c27552a74..31a2106e529a 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -238,6 +238,14 @@ struct flash_info {
> > * status register. Must be
> > used with
> > * SPI_NOR_HAS_TB.
> > */
> > +#define SPI_NOR_4BIT_BP BIT(17) /*
> > + * Flash SR has 4 bit fields
> > (BP0-3)
> > + * for block protection.
> > + */
> > +#define SPI_NOR_BP3_SR_BIT6 BIT(18) /*
> > + * BP3 is bit 6 of status
> > register.
> > + * Must be used with
> > SPI_NOR_4BIT_BP.
> > + */
> >
> > /* Part specific fixup hooks. */
> > const struct spi_nor_fixups *fixups;
> > @@ -1786,7 +1794,16 @@ static int spi_nor_erase(struct mtd_info
> > *mtd,
> > struct erase_info *instr)
> >
> > static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
> > {
> > - return SR_BP2 | SR_BP1 | SR_BP0;
> > + u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> > +
>
> can we just use the SR_BP3 eg:
>
> u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> if (nor->flags & SNOR_F_HAS_4BIT_BP)
> mask |= SR_BP3;
> return mask;
>
>
I'd prefer this one too if we can, but there are some places to need
the real mask value. It is also used in other places such as
spi_nor_sr_lock/unlock.
> > + if (nor->flags & SNOR_F_HAS_4BIT_BP) {
> > + if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
> > + mask = mask | SR_BP3_BIT6;
> > + else
> > + mask = mask | SR_BP3_BIT5;
> > + }
> > +
> > + return mask;
> > }
> >
> > static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
> > @@ -1797,12 +1814,26 @@ static u8 spi_nor_get_tb_mask(struct
> > spi_nor
> > *nor)
> > return SR_TB_BIT5;
> > }
> >
> > +static u8 stm_get_bpval_from_sr(struct spi_nor *nor, u8 sr) {
> > + u8 mask = spi_nor_get_bp_mask(nor);
> > + u8 bp = sr & mask;
> > +
> > + if (bp & SR_BP3_BIT6)
> > + bp = (bp & ~BIT(6)) | BIT(5);
> > +
> > + bp = bp >> SR_BP_SHIFT;
> > +
> > + return bp;
> > +}
>
> Don't convert this. It makes the code really hard to read. See below.
>
> > +
> > static int stm_get_min_prot_length(struct spi_nor *nor)
> > {
> > int bp_slots, bp_slots_needed;
> > - u8 mask = spi_nor_get_bp_mask(nor);
> >
> > - bp_slots = (mask >> SR_BP_SHIFT) + 1;
>
> Then just keep this.
>
> > + if (nor->flags & SNOR_F_HAS_4BIT_BP)
> > + bp_slots = 1 << 4;
> > + else
> > + bp_slots = 1 << 3;
> >
> > /* Reserved one for "protect none" and one for "protect all".
> > */
> > bp_slots = bp_slots - 2;
> > @@ -1821,9 +1852,8 @@ static void stm_get_locked_range(struct
> > spi_nor
> > *nor, u8 sr, loff_t *ofs,
> > {
> > struct mtd_info *mtd = &nor->mtd;
> > int min_prot_len;
> > - u8 mask = spi_nor_get_bp_mask(nor);
> > u8 tb_mask = spi_nor_get_tb_mask(nor);
> > - u8 bp = (sr & mask) >> SR_BP_SHIFT;
> > + u8 bp = stm_get_bpval_from_sr(nor, sr);
>
> also this.
>
> >
> > if (!bp) {
> > /* No protection */
> > @@ -1881,7 +1911,7 @@ static int stm_is_unlocked_sr(struct spi_nor
> > *nor, loff_t ofs, uint64_t len,
> >
> > /*
> > * Lock a region of the flash. Compatible with ST Micro and
> > similar
> > flash.
> > - * Supports the block protection bits BP{0,1,2} in the status
> > register
> > + * Supports the block protection bits BP{0,1,2}/BP{0,1,2,3} in the
> > status register
> > * (SR). Does not support these features found in newer SR
> > bitfields:
> > * - SEC: sector/block protect - only handle SEC=0 (block
> > protect)
> > * - CMP: complement protect - only support CMP=0 (range is not
> > complemented)
> > @@ -1889,7 +1919,7 @@ static int stm_is_unlocked_sr(struct spi_nor
> > *nor, loff_t ofs, uint64_t len,
> > * Support for the following is provided conditionally for some
> > flash:
> > * - TB: top/bottom protect
> > *
> > - * Sample table portion for 8MB flash (Winbond w25q64fw):
> > + * Sample table portion for 8MB flash (Winbond w25q64fw / BP0-2):
> > *
> > * SEC | TB | BP2 | BP1 | BP0 | Prot Length |
> > Protected
> > Portion
> > *
> > -----------------------------------------------------------------
> > ---------
> > @@ -1909,6 +1939,32 @@ static int stm_is_unlocked_sr(struct spi_nor
> > *nor, loff_t ofs, uint64_t len,
> > * 0 | 1 | 1 | 0 | 1 | 2 MB | Lower
> > 1/4
> > * 0 | 1 | 1 | 1 | 0 | 4 MB | Lower
> > 1/2
> > *
> > + * Sample table portion for 64MB flash (Micron n25q512ax3 / BP0-
> > 3):
> > + *
> > + * TB | BP3 | BP2 | BP1 | BP0 | Prot Length |
> > Protected
> > Portion
> > + *
> > -----------------------------------------------------------------
> > ---------
> > + * 0 | 0 | 0 | 0 | 0 | NONE | NONE
> > + * 0 | 0 | 0 | 0 | 1 | 64 KB | Upper
> > 1/1024
> > + * 0 | 0 | 0 | 1 | 0 | 128 KB | Upper
> > 1/512
> > + * 0 | 0 | 0 | 1 | 1 | 256 KB | Upper
> > 1/256
> > + * ...
> > + * 0 | 1 | 0 | 0 | 1 | 16 MB | Upper
> > 1/4
> > + * 0 | 1 | 0 | 1 | 0 | 32 MB | Upper
> > 1/2
> > + * 0 | 1 | 0 | 1 | 1 | 64 MB | ALL
> > + * 0 | 1 | 1 | 0 | 0 | 64 MB | ALL
> > + * ...
> > + *
> > ------|-------|-------|-------|-------|---------------|----------
> > ---------
> > + * 1 | 0 | 0 | 0 | 0 | NONE | NONE
> > + * 1 | 0 | 0 | 0 | 1 | 64 KB | Lower
> > 1/1024
> > + * 1 | 0 | 0 | 1 | 0 | 128 KB | Lower
> > 1/512
> > + * 1 | 0 | 0 | 1 | 1 | 256 KB | Lower
> > 1/256
> > + * ...
> > + * 1 | 1 | 0 | 0 | 1 | 16 MB | Lower
> > 1/4
> > + * 1 | 1 | 0 | 1 | 0 | 32 MB | Lower
> > 1/2
> > + * 1 | 1 | 0 | 1 | 1 | 64 MB | ALL
> > + * 1 | 1 | 1 | 0 | 0 | 64 MB | ALL
> > + * ...
> > + *
> > * Returns negative on errors, 0 on success.
> > */
> > static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> > @@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor *nor,
> > loff_t
> > ofs, uint64_t len)
> > min_prot_len = stm_get_min_prot_length(nor);
> > pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
> > val = pow << SR_BP_SHIFT;
> > +
> > + if (val & BIT(5) && mask & SR_BP3_BIT6)
> > + val = (val & ~BIT(5)) | BIT(6);
>
> .. and the use just the following here:
>
> if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
> val = (val & ~SR_BP3) | SR_BP3_BIT6;
>
> Ie. just use the "normal case" where all BP bits are next to each
> other
> and then fixup the resulting value and shift the SR3 bit if
> necessary.
> This will be much easier to read.
>
Yes, I agree. It would be better to minimize this kind of conversion in
one place.
> > }
> >
> > if (val & ~mask)
> > @@ -2042,6 +2101,9 @@ static int stm_unlock(struct spi_nor *nor,
> > loff_t ofs, uint64_t len)
> > pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
> > val = pow << SR_BP_SHIFT;
> >
> > + if (val & BIT(5) && mask & SR_BP3_BIT6)
> > + val = (val & ~BIT(5)) | BIT(6);
> > +
>
> here would be the other way around:
>
> if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6)
> val = (val & ~SR_BP3_BIT6) | SR_BP3;
>
>
> > /* Some power-of-two sizes are not supported */
> > if (val & ~mask)
> > return -EINVAL;
> > @@ -5244,6 +5306,12 @@ int spi_nor_scan(struct spi_nor *nor, const
> > char
> > *name,
> > if (info->flags & USE_CLSR)
> > nor->flags |= SNOR_F_USE_CLSR;
> >
> > + if (info->flags & SPI_NOR_4BIT_BP) {
> > + nor->flags |= SNOR_F_HAS_4BIT_BP;
> > + if (info->flags & SPI_NOR_BP3_SR_BIT6)
> > + nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
> > + }
> > +
> > if (info->flags & SPI_NOR_NO_ERASE)
> > mtd->flags |= MTD_NO_ERASE;
> >
> > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-
> > nor.h
> > index de90724f62f1..0190ed21576a 100644
> > --- a/include/linux/mtd/spi-nor.h
> > +++ b/include/linux/mtd/spi-nor.h
> > @@ -129,7 +129,9 @@
> > #define SR_BP1 BIT(3) /* Block protect 1
> > */
> > #define SR_BP2 BIT(4) /* Block protect 2
> > */
> > #define SR_TB_BIT5 BIT(5) /* Top/Bottom protect */
> > +#define SR_BP3_BIT5 BIT(5) /* Block protect 3
> > */
>
> IMHO just SR_BP3. but that is a matter of taste. But it is easier on
> the eye in the mask = SR_BP3 | SR_BP2 etc case.
>
SR_BP3 would be a more appropriate name if we could set the case with
all BP bits next to each other as the "normal case."
I'm going to write patches based on latest spi-nor/next including what
you mentioned.
Thanks,
> -michael
>
> > #define SR_TB_BIT6 BIT(6) /* Top/Bottom protect */
> > +#define SR_BP3_BIT6 BIT(6) /* Block protect 3
> > */
> > #define SR_SRWD BIT(7) /* SR write protect
> > */
> > /* Spansion/Cypress specific status bits */
> > #define SR_E_ERR BIT(5)
> > @@ -240,6 +242,8 @@ enum spi_nor_option_flags {
> > SNOR_F_HAS_16BIT_SR = BIT(9),
> > SNOR_F_NO_READ_CR = BIT(10),
> > SNOR_F_HAS_SR_TB_BIT6 = BIT(11),
> > + SNOR_F_HAS_4BIT_BP = BIT(12),
> > + SNOR_F_HAS_SR_BP3_BIT6 = BIT(13),
> >
> > };
>
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] mtd: spi-nor: add 4bit block protection support
2020-03-04 11:07 ` [PATCH 2/3] mtd: spi-nor: add 4bit block protection support Jungseung Lee
@ 2020-03-13 16:24 ` Michael Walle
2020-03-17 11:00 ` Jungseung Lee
0 siblings, 1 reply; 9+ messages in thread
From: Michael Walle @ 2020-03-13 16:24 UTC (permalink / raw)
To: Jungseung Lee
Cc: chenxiang, js07.lee, linux-mtd, Vignesh Raghavendra, Tudor Ambarus
Hi Jungseung,
sorry for the late review.
Am 2020-03-04 12:07, schrieb Jungseung Lee:
> Currently, we are supporting block protection only for
> flash chips with 3 block protection bits in the SR register.
> This patch enables block protection support for flashes with
> 4 block protection bits(bp0-3).
>
> Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
> ---
> drivers/mtd/spi-nor/spi-nor.c | 82 ++++++++++++++++++++++++++++++++---
> include/linux/mtd/spi-nor.h | 4 ++
> 2 files changed, 79 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c
> b/drivers/mtd/spi-nor/spi-nor.c
> index c58c27552a74..31a2106e529a 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -238,6 +238,14 @@ struct flash_info {
> * status register. Must be used with
> * SPI_NOR_HAS_TB.
> */
> +#define SPI_NOR_4BIT_BP BIT(17) /*
> + * Flash SR has 4 bit fields (BP0-3)
> + * for block protection.
> + */
> +#define SPI_NOR_BP3_SR_BIT6 BIT(18) /*
> + * BP3 is bit 6 of status register.
> + * Must be used with SPI_NOR_4BIT_BP.
> + */
>
> /* Part specific fixup hooks. */
> const struct spi_nor_fixups *fixups;
> @@ -1786,7 +1794,16 @@ static int spi_nor_erase(struct mtd_info *mtd,
> struct erase_info *instr)
>
> static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
> {
> - return SR_BP2 | SR_BP1 | SR_BP0;
> + u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
> +
can we just use the SR_BP3 eg:
u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
if (nor->flags & SNOR_F_HAS_4BIT_BP)
mask |= SR_BP3;
return mask;
> + if (nor->flags & SNOR_F_HAS_4BIT_BP) {
> + if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
> + mask = mask | SR_BP3_BIT6;
> + else
> + mask = mask | SR_BP3_BIT5;
> + }
> +
> + return mask;
> }
>
> static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
> @@ -1797,12 +1814,26 @@ static u8 spi_nor_get_tb_mask(struct spi_nor
> *nor)
> return SR_TB_BIT5;
> }
>
> +static u8 stm_get_bpval_from_sr(struct spi_nor *nor, u8 sr) {
> + u8 mask = spi_nor_get_bp_mask(nor);
> + u8 bp = sr & mask;
> +
> + if (bp & SR_BP3_BIT6)
> + bp = (bp & ~BIT(6)) | BIT(5);
> +
> + bp = bp >> SR_BP_SHIFT;
> +
> + return bp;
> +}
Don't convert this. It makes the code really hard to read. See below.
> +
> static int stm_get_min_prot_length(struct spi_nor *nor)
> {
> int bp_slots, bp_slots_needed;
> - u8 mask = spi_nor_get_bp_mask(nor);
>
> - bp_slots = (mask >> SR_BP_SHIFT) + 1;
Then just keep this.
> + if (nor->flags & SNOR_F_HAS_4BIT_BP)
> + bp_slots = 1 << 4;
> + else
> + bp_slots = 1 << 3;
>
> /* Reserved one for "protect none" and one for "protect all". */
> bp_slots = bp_slots - 2;
> @@ -1821,9 +1852,8 @@ static void stm_get_locked_range(struct spi_nor
> *nor, u8 sr, loff_t *ofs,
> {
> struct mtd_info *mtd = &nor->mtd;
> int min_prot_len;
> - u8 mask = spi_nor_get_bp_mask(nor);
> u8 tb_mask = spi_nor_get_tb_mask(nor);
> - u8 bp = (sr & mask) >> SR_BP_SHIFT;
> + u8 bp = stm_get_bpval_from_sr(nor, sr);
also this.
>
> if (!bp) {
> /* No protection */
> @@ -1881,7 +1911,7 @@ static int stm_is_unlocked_sr(struct spi_nor
> *nor, loff_t ofs, uint64_t len,
>
> /*
> * Lock a region of the flash. Compatible with ST Micro and similar
> flash.
> - * Supports the block protection bits BP{0,1,2} in the status register
> + * Supports the block protection bits BP{0,1,2}/BP{0,1,2,3} in the
> status register
> * (SR). Does not support these features found in newer SR bitfields:
> * - SEC: sector/block protect - only handle SEC=0 (block protect)
> * - CMP: complement protect - only support CMP=0 (range is not
> complemented)
> @@ -1889,7 +1919,7 @@ static int stm_is_unlocked_sr(struct spi_nor
> *nor, loff_t ofs, uint64_t len,
> * Support for the following is provided conditionally for some flash:
> * - TB: top/bottom protect
> *
> - * Sample table portion for 8MB flash (Winbond w25q64fw):
> + * Sample table portion for 8MB flash (Winbond w25q64fw / BP0-2):
> *
> * SEC | TB | BP2 | BP1 | BP0 | Prot Length | Protected
> Portion
> *
> --------------------------------------------------------------------------
> @@ -1909,6 +1939,32 @@ static int stm_is_unlocked_sr(struct spi_nor
> *nor, loff_t ofs, uint64_t len,
> * 0 | 1 | 1 | 0 | 1 | 2 MB | Lower 1/4
> * 0 | 1 | 1 | 1 | 0 | 4 MB | Lower 1/2
> *
> + * Sample table portion for 64MB flash (Micron n25q512ax3 / BP0-3):
> + *
> + * TB | BP3 | BP2 | BP1 | BP0 | Prot Length | Protected
> Portion
> + *
> --------------------------------------------------------------------------
> + * 0 | 0 | 0 | 0 | 0 | NONE | NONE
> + * 0 | 0 | 0 | 0 | 1 | 64 KB | Upper
> 1/1024
> + * 0 | 0 | 0 | 1 | 0 | 128 KB | Upper
> 1/512
> + * 0 | 0 | 0 | 1 | 1 | 256 KB | Upper
> 1/256
> + * ...
> + * 0 | 1 | 0 | 0 | 1 | 16 MB | Upper 1/4
> + * 0 | 1 | 0 | 1 | 0 | 32 MB | Upper 1/2
> + * 0 | 1 | 0 | 1 | 1 | 64 MB | ALL
> + * 0 | 1 | 1 | 0 | 0 | 64 MB | ALL
> + * ...
> + *
> ------|-------|-------|-------|-------|---------------|-------------------
> + * 1 | 0 | 0 | 0 | 0 | NONE | NONE
> + * 1 | 0 | 0 | 0 | 1 | 64 KB | Lower
> 1/1024
> + * 1 | 0 | 0 | 1 | 0 | 128 KB | Lower
> 1/512
> + * 1 | 0 | 0 | 1 | 1 | 256 KB | Lower
> 1/256
> + * ...
> + * 1 | 1 | 0 | 0 | 1 | 16 MB | Lower 1/4
> + * 1 | 1 | 0 | 1 | 0 | 32 MB | Lower 1/2
> + * 1 | 1 | 0 | 1 | 1 | 64 MB | ALL
> + * 1 | 1 | 1 | 0 | 0 | 64 MB | ALL
> + * ...
> + *
> * Returns negative on errors, 0 on success.
> */
> static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> @@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor *nor, loff_t
> ofs, uint64_t len)
> min_prot_len = stm_get_min_prot_length(nor);
> pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
> val = pow << SR_BP_SHIFT;
> +
> + if (val & BIT(5) && mask & SR_BP3_BIT6)
> + val = (val & ~BIT(5)) | BIT(6);
.. and the use just the following here:
if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3)
val = (val & ~SR_BP3) | SR_BP3_BIT6;
Ie. just use the "normal case" where all BP bits are next to each other
and then fixup the resulting value and shift the SR3 bit if necessary.
This will be much easier to read.
> }
>
> if (val & ~mask)
> @@ -2042,6 +2101,9 @@ static int stm_unlock(struct spi_nor *nor,
> loff_t ofs, uint64_t len)
> pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
> val = pow << SR_BP_SHIFT;
>
> + if (val & BIT(5) && mask & SR_BP3_BIT6)
> + val = (val & ~BIT(5)) | BIT(6);
> +
here would be the other way around:
if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6 && val & SR_BP3_BIT6)
val = (val & ~SR_BP3_BIT6) | SR_BP3;
> /* Some power-of-two sizes are not supported */
> if (val & ~mask)
> return -EINVAL;
> @@ -5244,6 +5306,12 @@ int spi_nor_scan(struct spi_nor *nor, const char
> *name,
> if (info->flags & USE_CLSR)
> nor->flags |= SNOR_F_USE_CLSR;
>
> + if (info->flags & SPI_NOR_4BIT_BP) {
> + nor->flags |= SNOR_F_HAS_4BIT_BP;
> + if (info->flags & SPI_NOR_BP3_SR_BIT6)
> + nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
> + }
> +
> if (info->flags & SPI_NOR_NO_ERASE)
> mtd->flags |= MTD_NO_ERASE;
>
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index de90724f62f1..0190ed21576a 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -129,7 +129,9 @@
> #define SR_BP1 BIT(3) /* Block protect 1 */
> #define SR_BP2 BIT(4) /* Block protect 2 */
> #define SR_TB_BIT5 BIT(5) /* Top/Bottom protect */
> +#define SR_BP3_BIT5 BIT(5) /* Block protect 3 */
IMHO just SR_BP3. but that is a matter of taste. But it is easier on
the eye in the mask = SR_BP3 | SR_BP2 etc case.
-michael
> #define SR_TB_BIT6 BIT(6) /* Top/Bottom protect */
> +#define SR_BP3_BIT6 BIT(6) /* Block protect 3 */
> #define SR_SRWD BIT(7) /* SR write protect */
> /* Spansion/Cypress specific status bits */
> #define SR_E_ERR BIT(5)
> @@ -240,6 +242,8 @@ enum spi_nor_option_flags {
> SNOR_F_HAS_16BIT_SR = BIT(9),
> SNOR_F_NO_READ_CR = BIT(10),
> SNOR_F_HAS_SR_TB_BIT6 = BIT(11),
> + SNOR_F_HAS_4BIT_BP = BIT(12),
> + SNOR_F_HAS_SR_BP3_BIT6 = BIT(13),
>
> };
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] mtd: spi-nor: add 4bit block protection support
[not found] ` <CGME20200304110833epcas1p42958d6dce0081afabfdd4200258eddb8@epcas1p4.samsung.com>
@ 2020-03-04 11:07 ` Jungseung Lee
2020-03-13 16:24 ` Michael Walle
0 siblings, 1 reply; 9+ messages in thread
From: Jungseung Lee @ 2020-03-04 11:07 UTC (permalink / raw)
To: Tudor Ambarus, Vignesh Raghavendra, linux-mtd, js07.lee,
js07.lee, chenxiang, Michael Walle
Currently, we are supporting block protection only for
flash chips with 3 block protection bits in the SR register.
This patch enables block protection support for flashes with
4 block protection bits(bp0-3).
Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
---
drivers/mtd/spi-nor/spi-nor.c | 82 ++++++++++++++++++++++++++++++++---
include/linux/mtd/spi-nor.h | 4 ++
2 files changed, 79 insertions(+), 7 deletions(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index c58c27552a74..31a2106e529a 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -238,6 +238,14 @@ struct flash_info {
* status register. Must be used with
* SPI_NOR_HAS_TB.
*/
+#define SPI_NOR_4BIT_BP BIT(17) /*
+ * Flash SR has 4 bit fields (BP0-3)
+ * for block protection.
+ */
+#define SPI_NOR_BP3_SR_BIT6 BIT(18) /*
+ * BP3 is bit 6 of status register.
+ * Must be used with SPI_NOR_4BIT_BP.
+ */
/* Part specific fixup hooks. */
const struct spi_nor_fixups *fixups;
@@ -1786,7 +1794,16 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
static u8 spi_nor_get_bp_mask(struct spi_nor *nor)
{
- return SR_BP2 | SR_BP1 | SR_BP0;
+ u8 mask = SR_BP2 | SR_BP1 | SR_BP0;
+
+ if (nor->flags & SNOR_F_HAS_4BIT_BP) {
+ if (nor->flags & SNOR_F_HAS_SR_BP3_BIT6)
+ mask = mask | SR_BP3_BIT6;
+ else
+ mask = mask | SR_BP3_BIT5;
+ }
+
+ return mask;
}
static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
@@ -1797,12 +1814,26 @@ static u8 spi_nor_get_tb_mask(struct spi_nor *nor)
return SR_TB_BIT5;
}
+static u8 stm_get_bpval_from_sr(struct spi_nor *nor, u8 sr) {
+ u8 mask = spi_nor_get_bp_mask(nor);
+ u8 bp = sr & mask;
+
+ if (bp & SR_BP3_BIT6)
+ bp = (bp & ~BIT(6)) | BIT(5);
+
+ bp = bp >> SR_BP_SHIFT;
+
+ return bp;
+}
+
static int stm_get_min_prot_length(struct spi_nor *nor)
{
int bp_slots, bp_slots_needed;
- u8 mask = spi_nor_get_bp_mask(nor);
- bp_slots = (mask >> SR_BP_SHIFT) + 1;
+ if (nor->flags & SNOR_F_HAS_4BIT_BP)
+ bp_slots = 1 << 4;
+ else
+ bp_slots = 1 << 3;
/* Reserved one for "protect none" and one for "protect all". */
bp_slots = bp_slots - 2;
@@ -1821,9 +1852,8 @@ static void stm_get_locked_range(struct spi_nor *nor, u8 sr, loff_t *ofs,
{
struct mtd_info *mtd = &nor->mtd;
int min_prot_len;
- u8 mask = spi_nor_get_bp_mask(nor);
u8 tb_mask = spi_nor_get_tb_mask(nor);
- u8 bp = (sr & mask) >> SR_BP_SHIFT;
+ u8 bp = stm_get_bpval_from_sr(nor, sr);
if (!bp) {
/* No protection */
@@ -1881,7 +1911,7 @@ static int stm_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
/*
* Lock a region of the flash. Compatible with ST Micro and similar flash.
- * Supports the block protection bits BP{0,1,2} in the status register
+ * Supports the block protection bits BP{0,1,2}/BP{0,1,2,3} in the status register
* (SR). Does not support these features found in newer SR bitfields:
* - SEC: sector/block protect - only handle SEC=0 (block protect)
* - CMP: complement protect - only support CMP=0 (range is not complemented)
@@ -1889,7 +1919,7 @@ static int stm_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
* Support for the following is provided conditionally for some flash:
* - TB: top/bottom protect
*
- * Sample table portion for 8MB flash (Winbond w25q64fw):
+ * Sample table portion for 8MB flash (Winbond w25q64fw / BP0-2):
*
* SEC | TB | BP2 | BP1 | BP0 | Prot Length | Protected Portion
* --------------------------------------------------------------------------
@@ -1909,6 +1939,32 @@ static int stm_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
* 0 | 1 | 1 | 0 | 1 | 2 MB | Lower 1/4
* 0 | 1 | 1 | 1 | 0 | 4 MB | Lower 1/2
*
+ * Sample table portion for 64MB flash (Micron n25q512ax3 / BP0-3):
+ *
+ * TB | BP3 | BP2 | BP1 | BP0 | Prot Length | Protected Portion
+ * --------------------------------------------------------------------------
+ * 0 | 0 | 0 | 0 | 0 | NONE | NONE
+ * 0 | 0 | 0 | 0 | 1 | 64 KB | Upper 1/1024
+ * 0 | 0 | 0 | 1 | 0 | 128 KB | Upper 1/512
+ * 0 | 0 | 0 | 1 | 1 | 256 KB | Upper 1/256
+ * ...
+ * 0 | 1 | 0 | 0 | 1 | 16 MB | Upper 1/4
+ * 0 | 1 | 0 | 1 | 0 | 32 MB | Upper 1/2
+ * 0 | 1 | 0 | 1 | 1 | 64 MB | ALL
+ * 0 | 1 | 1 | 0 | 0 | 64 MB | ALL
+ * ...
+ * ------|-------|-------|-------|-------|---------------|-------------------
+ * 1 | 0 | 0 | 0 | 0 | NONE | NONE
+ * 1 | 0 | 0 | 0 | 1 | 64 KB | Lower 1/1024
+ * 1 | 0 | 0 | 1 | 0 | 128 KB | Lower 1/512
+ * 1 | 0 | 0 | 1 | 1 | 256 KB | Lower 1/256
+ * ...
+ * 1 | 1 | 0 | 0 | 1 | 16 MB | Lower 1/4
+ * 1 | 1 | 0 | 1 | 0 | 32 MB | Lower 1/2
+ * 1 | 1 | 0 | 1 | 1 | 64 MB | ALL
+ * 1 | 1 | 1 | 0 | 0 | 64 MB | ALL
+ * ...
+ *
* Returns negative on errors, 0 on success.
*/
static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
@@ -1960,6 +2016,9 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
min_prot_len = stm_get_min_prot_length(nor);
pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
val = pow << SR_BP_SHIFT;
+
+ if (val & BIT(5) && mask & SR_BP3_BIT6)
+ val = (val & ~BIT(5)) | BIT(6);
}
if (val & ~mask)
@@ -2042,6 +2101,9 @@ static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
pow = ilog2(lock_len) - ilog2(min_prot_len) + 1;
val = pow << SR_BP_SHIFT;
+ if (val & BIT(5) && mask & SR_BP3_BIT6)
+ val = (val & ~BIT(5)) | BIT(6);
+
/* Some power-of-two sizes are not supported */
if (val & ~mask)
return -EINVAL;
@@ -5244,6 +5306,12 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
if (info->flags & USE_CLSR)
nor->flags |= SNOR_F_USE_CLSR;
+ if (info->flags & SPI_NOR_4BIT_BP) {
+ nor->flags |= SNOR_F_HAS_4BIT_BP;
+ if (info->flags & SPI_NOR_BP3_SR_BIT6)
+ nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
+ }
+
if (info->flags & SPI_NOR_NO_ERASE)
mtd->flags |= MTD_NO_ERASE;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index de90724f62f1..0190ed21576a 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -129,7 +129,9 @@
#define SR_BP1 BIT(3) /* Block protect 1 */
#define SR_BP2 BIT(4) /* Block protect 2 */
#define SR_TB_BIT5 BIT(5) /* Top/Bottom protect */
+#define SR_BP3_BIT5 BIT(5) /* Block protect 3 */
#define SR_TB_BIT6 BIT(6) /* Top/Bottom protect */
+#define SR_BP3_BIT6 BIT(6) /* Block protect 3 */
#define SR_SRWD BIT(7) /* SR write protect */
/* Spansion/Cypress specific status bits */
#define SR_E_ERR BIT(5)
@@ -240,6 +242,8 @@ enum spi_nor_option_flags {
SNOR_F_HAS_16BIT_SR = BIT(9),
SNOR_F_NO_READ_CR = BIT(10),
SNOR_F_HAS_SR_TB_BIT6 = BIT(11),
+ SNOR_F_HAS_4BIT_BP = BIT(12),
+ SNOR_F_HAS_SR_BP3_BIT6 = BIT(13),
};
--
2.17.1
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-03-18 6:01 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20200110102307epcas1p2a8b32fc2a2455d1052982d8ac499d21b@epcas1p2.samsung.com>
2020-01-10 10:22 ` [PATCH 1/3] mtd: spi-nor: introduce SR_BP_SHIFT define Jungseung Lee
[not found] ` <CGME20200110102310epcas1p40589cbb4370dcd4db08efa4008deb755@epcas1p4.samsung.com>
2020-01-10 10:22 ` [PATCH 2/3] mtd: spi-nor: add 4bit block protection support Jungseung Lee
[not found] ` <CGME20200110102313epcas1p17aed8316bfacb0a0fb9a42fc0db24292@epcas1p1.samsung.com>
2020-01-10 10:22 ` [PATCH 3/3] mtd: spi-nor: support lock/unlock for a few Micron chip s Jungseung Lee
2020-03-04 11:07 [PATCH 1/3] mtd: spi-nor: reimplement block protection handling Jungseung Lee
[not found] ` <CGME20200304110833epcas1p42958d6dce0081afabfdd4200258eddb8@epcas1p4.samsung.com>
2020-03-04 11:07 ` [PATCH 2/3] mtd: spi-nor: add 4bit block protection support Jungseung Lee
2020-03-13 16:24 ` Michael Walle
2020-03-17 11:00 ` Jungseung Lee
2020-03-17 11:35 ` Jungseung Lee
2020-03-17 14:52 ` Michael Walle
2020-03-18 6:01 ` Jungseung Lee
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.