[PATCH v2 1/2] mtd: spi-nor: Add Global Block Unlock command

[PATCH v2 1/2] mtd: spi-nor: Add Global Block Unlock command

[Tudor Ambarus]

The Global Block Unlock command has different names depending
on the manufacturer, but always the same command value: 0x98.
Macronix's MX25U12835F names it Gang Block Unlock, Winbound's
W25Q128FV names it Global Block Unlock and Microchip's
SST26VF064B names it Global Block Protection Unlock.

Used in the Individual Block Protection mode, which is mutually
exclusive with the Block Protection mode (BP0-3).

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
---
v2:
- s/mutual/mutually/
- set the GBULK cmd buswidth to 0 and call spi_nor_spimem_setup_op()
to update the it, because the op can can be issued in QPI mode as well.
- add Pratyush's R-b tag

 drivers/mtd/spi-nor/core.c  | 37 +++++++++++++++++++++++++++++++++++++
 drivers/mtd/spi-nor/core.h  |  1 +
 include/linux/mtd/spi-nor.h |  1 +
 3 files changed, 39 insertions(+)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 20df44b753da..e82732dd31e1 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -853,6 +853,43 @@ int spi_nor_wait_till_ready(struct spi_nor *nor)
 						    DEFAULT_READY_WAIT_JIFFIES);
 }
 
+/**
+ * spi_nor_global_block_unlock() - Unlock Global Block Protection.
+ * @nor:	pointer to 'struct spi_nor'.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+int spi_nor_global_block_unlock(struct spi_nor *nor)
+{
+	int ret;
+
+	ret = spi_nor_write_enable(nor);
+	if (ret)
+		return ret;
+
+	if (nor->spimem) {
+		struct spi_mem_op op =
+			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_GBULK, 0),
+				   SPI_MEM_OP_NO_ADDR,
+				   SPI_MEM_OP_NO_DUMMY,
+				   SPI_MEM_OP_NO_DATA);
+
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
+		ret = spi_mem_exec_op(nor->spimem, &op);
+	} else {
+		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_GBULK,
+						       NULL, 0);
+	}
+
+	if (ret) {
+		dev_dbg(nor->dev, "error %d on Global Block Unlock\n", ret);
+		return ret;
+	}
+
+	return spi_nor_wait_till_ready(nor);
+}
+
 /**
  * spi_nor_write_sr() - Write the Status Register.
  * @nor:	pointer to 'struct spi_nor'.
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index d631ee299de3..eb26796db026 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -434,6 +434,7 @@ int spi_nor_write_disable(struct spi_nor *nor);
 int spi_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable);
 int spi_nor_write_ear(struct spi_nor *nor, u8 ear);
 int spi_nor_wait_till_ready(struct spi_nor *nor);
+int spi_nor_global_block_unlock(struct spi_nor *nor);
 int spi_nor_lock_and_prep(struct spi_nor *nor);
 void spi_nor_unlock_and_unprep(struct spi_nor *nor);
 int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor);
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index d13958de6d8a..a0d572855444 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -53,6 +53,7 @@
 #define SPINOR_OP_WREAR		0xc5	/* Write Extended Address Register */
 #define SPINOR_OP_SRSTEN	0x66	/* Software Reset Enable */
 #define SPINOR_OP_SRST		0x99	/* Software Reset */
+#define SPINOR_OP_GBULK		0x98    /* Global Block Unlock */
 
 /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */
 #define SPINOR_OP_READ_4B	0x13	/* Read data bytes (low frequency) */
-- 
2.25.1

[PATCH v2 1/2] mtd: spi-nor: Add Global Block Unlock command

[Tudor Ambarus]

The Global Block Unlock command has different names depending
on the manufacturer, but always the same command value: 0x98.
Macronix's MX25U12835F names it Gang Block Unlock, Winbound's
W25Q128FV names it Global Block Unlock and Microchip's
SST26VF064B names it Global Block Protection Unlock.

Used in the Individual Block Protection mode, which is mutually
exclusive with the Block Protection mode (BP0-3).

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
---
v2:
- s/mutual/mutually/
- set the GBULK cmd buswidth to 0 and call spi_nor_spimem_setup_op()
to update the it, because the op can can be issued in QPI mode as well.
- add Pratyush's R-b tag

 drivers/mtd/spi-nor/core.c  | 37 +++++++++++++++++++++++++++++++++++++
 drivers/mtd/spi-nor/core.h  |  1 +
 include/linux/mtd/spi-nor.h |  1 +
 3 files changed, 39 insertions(+)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 20df44b753da..e82732dd31e1 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -853,6 +853,43 @@ int spi_nor_wait_till_ready(struct spi_nor *nor)
 						    DEFAULT_READY_WAIT_JIFFIES);
 }
 
+/**
+ * spi_nor_global_block_unlock() - Unlock Global Block Protection.
+ * @nor:	pointer to 'struct spi_nor'.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+int spi_nor_global_block_unlock(struct spi_nor *nor)
+{
+	int ret;
+
+	ret = spi_nor_write_enable(nor);
+	if (ret)
+		return ret;
+
+	if (nor->spimem) {
+		struct spi_mem_op op =
+			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_GBULK, 0),
+				   SPI_MEM_OP_NO_ADDR,
+				   SPI_MEM_OP_NO_DUMMY,
+				   SPI_MEM_OP_NO_DATA);
+
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
+		ret = spi_mem_exec_op(nor->spimem, &op);
+	} else {
+		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_GBULK,
+						       NULL, 0);
+	}
+
+	if (ret) {
+		dev_dbg(nor->dev, "error %d on Global Block Unlock\n", ret);
+		return ret;
+	}
+
+	return spi_nor_wait_till_ready(nor);
+}
+
 /**
  * spi_nor_write_sr() - Write the Status Register.
  * @nor:	pointer to 'struct spi_nor'.
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index d631ee299de3..eb26796db026 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -434,6 +434,7 @@ int spi_nor_write_disable(struct spi_nor *nor);
 int spi_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable);
 int spi_nor_write_ear(struct spi_nor *nor, u8 ear);
 int spi_nor_wait_till_ready(struct spi_nor *nor);
+int spi_nor_global_block_unlock(struct spi_nor *nor);
 int spi_nor_lock_and_prep(struct spi_nor *nor);
 void spi_nor_unlock_and_unprep(struct spi_nor *nor);
 int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor);
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index d13958de6d8a..a0d572855444 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -53,6 +53,7 @@
 #define SPINOR_OP_WREAR		0xc5	/* Write Extended Address Register */
 #define SPINOR_OP_SRSTEN	0x66	/* Software Reset Enable */
 #define SPINOR_OP_SRST		0x99	/* Software Reset */
+#define SPINOR_OP_GBULK		0x98    /* Global Block Unlock */
 
 /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */
 #define SPINOR_OP_READ_4B	0x13	/* Read data bytes (low frequency) */
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 2/2] mtd: spi-nor: sst: Add support for Global Unlock on sst26vf

[Tudor Ambarus]

Even if sst26vf shares the SPINOR_OP_GBULK opcode with
Macronix (ex. MX25U12835F) and Winbound (ex. W25Q128FV),
it has its own Individual Block Protection scheme, which
is also capable to read-lock individual parameter blocks.
Thus the sst26vf's Individual Block Protection scheme will
reside in the sst.c manufacturer driver.

Add support to unlock the entire flash memory. The device
is write-protected by default after a power-on reset cycle
(volatile software protection), in order to avoid inadvertent
writes during power-up. Could do an erase, write, read back,
and compare when MTD_SPI_NOR_SWP_DISABLE_ON_VOLATILE=y.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
v2: s/!ofs/ofs == 0/

 drivers/mtd/spi-nor/sst.c | 38 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
index 00e48da0744a..d6e1396abb96 100644
--- a/drivers/mtd/spi-nor/sst.c
+++ b/drivers/mtd/spi-nor/sst.c
@@ -8,6 +8,39 @@
 
 #include "core.h"
 
+static int sst26vf_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
+{
+	return -EOPNOTSUPP;
+}
+
+static int sst26vf_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
+{
+	if (ofs == 0 && len == nor->params->size)
+		return spi_nor_global_block_unlock(nor);
+
+	return -EOPNOTSUPP;
+}
+
+static int sst26vf_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
+{
+	return -EOPNOTSUPP;
+}
+
+static const struct spi_nor_locking_ops sst26vf_locking_ops = {
+	.lock = sst26vf_lock,
+	.unlock = sst26vf_unlock,
+	.is_locked = sst26vf_is_locked,
+};
+
+static void sst26vf_default_init(struct spi_nor *nor)
+{
+	nor->params->locking_ops = &sst26vf_locking_ops;
+}
+
+static const struct spi_nor_fixups sst26vf_fixups = {
+	.default_init = sst26vf_default_init,
+};
+
 static const struct flash_info sst_parts[] = {
 	/* SST -- large erase sizes are "overlays", "sectors" are 4K */
 	{ "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024,  8,
@@ -39,8 +72,9 @@ static const struct flash_info sst_parts[] = {
 	{ "sst26vf016b", INFO(0xbf2641, 0, 64 * 1024, 32,
 			      SECT_4K | SPI_NOR_DUAL_READ) },
 	{ "sst26vf064b", INFO(0xbf2643, 0, 64 * 1024, 128,
-			      SECT_4K | SPI_NOR_DUAL_READ |
-			      SPI_NOR_QUAD_READ) },
+			      SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
+			      SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE)
+		.fixups = &sst26vf_fixups },
 };
 
 static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
-- 
2.25.1

[PATCH v2 2/2] mtd: spi-nor: sst: Add support for Global Unlock on sst26vf

[Tudor Ambarus]

Even if sst26vf shares the SPINOR_OP_GBULK opcode with
Macronix (ex. MX25U12835F) and Winbound (ex. W25Q128FV),
it has its own Individual Block Protection scheme, which
is also capable to read-lock individual parameter blocks.
Thus the sst26vf's Individual Block Protection scheme will
reside in the sst.c manufacturer driver.

Add support to unlock the entire flash memory. The device
is write-protected by default after a power-on reset cycle
(volatile software protection), in order to avoid inadvertent
writes during power-up. Could do an erase, write, read back,
and compare when MTD_SPI_NOR_SWP_DISABLE_ON_VOLATILE=y.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
v2: s/!ofs/ofs == 0/

 drivers/mtd/spi-nor/sst.c | 38 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
index 00e48da0744a..d6e1396abb96 100644
--- a/drivers/mtd/spi-nor/sst.c
+++ b/drivers/mtd/spi-nor/sst.c
@@ -8,6 +8,39 @@
 
 #include "core.h"
 
+static int sst26vf_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
+{
+	return -EOPNOTSUPP;
+}
+
+static int sst26vf_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
+{
+	if (ofs == 0 && len == nor->params->size)
+		return spi_nor_global_block_unlock(nor);
+
+	return -EOPNOTSUPP;
+}
+
+static int sst26vf_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
+{
+	return -EOPNOTSUPP;
+}
+
+static const struct spi_nor_locking_ops sst26vf_locking_ops = {
+	.lock = sst26vf_lock,
+	.unlock = sst26vf_unlock,
+	.is_locked = sst26vf_is_locked,
+};
+
+static void sst26vf_default_init(struct spi_nor *nor)
+{
+	nor->params->locking_ops = &sst26vf_locking_ops;
+}
+
+static const struct spi_nor_fixups sst26vf_fixups = {
+	.default_init = sst26vf_default_init,
+};
+
 static const struct flash_info sst_parts[] = {
 	/* SST -- large erase sizes are "overlays", "sectors" are 4K */
 	{ "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024,  8,
@@ -39,8 +72,9 @@ static const struct flash_info sst_parts[] = {
 	{ "sst26vf016b", INFO(0xbf2641, 0, 64 * 1024, 32,
 			      SECT_4K | SPI_NOR_DUAL_READ) },
 	{ "sst26vf064b", INFO(0xbf2643, 0, 64 * 1024, 128,
-			      SECT_4K | SPI_NOR_DUAL_READ |
-			      SPI_NOR_QUAD_READ) },
+			      SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
+			      SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE)
+		.fixups = &sst26vf_fixups },
 };
 
 static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 1/2] mtd: spi-nor: Add Global Block Unlock command

[Michael Walle]

Am 2021-01-20 14:19, schrieb Tudor Ambarus:
> The Global Block Unlock command has different names depending
> on the manufacturer, but always the same command value: 0x98.
> Macronix's MX25U12835F names it Gang Block Unlock, Winbound's

Winbond

> W25Q128FV names it Global Block Unlock and Microchip's
> SST26VF064B names it Global Block Protection Unlock.
> 
> Used in the Individual Block Protection mode, which is mutually
> exclusive with the Block Protection mode (BP0-3).
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

Reviewed-by: Michael Walle <michael@walle.cc>

-michael

> ---
> v2:
> - s/mutual/mutually/
> - set the GBULK cmd buswidth to 0 and call spi_nor_spimem_setup_op()
> to update the it, because the op can can be issued in QPI mode as well.
> - add Pratyush's R-b tag
> 
>  drivers/mtd/spi-nor/core.c  | 37 +++++++++++++++++++++++++++++++++++++
>  drivers/mtd/spi-nor/core.h  |  1 +
>  include/linux/mtd/spi-nor.h |  1 +
>  3 files changed, 39 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 20df44b753da..e82732dd31e1 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -853,6 +853,43 @@ int spi_nor_wait_till_ready(struct spi_nor *nor)
>  						    DEFAULT_READY_WAIT_JIFFIES);
>  }
> 
> +/**
> + * spi_nor_global_block_unlock() - Unlock Global Block Protection.
> + * @nor:	pointer to 'struct spi_nor'.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +int spi_nor_global_block_unlock(struct spi_nor *nor)
> +{
> +	int ret;
> +
> +	ret = spi_nor_write_enable(nor);
> +	if (ret)
> +		return ret;
> +
> +	if (nor->spimem) {
> +		struct spi_mem_op op =
> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_GBULK, 0),
> +				   SPI_MEM_OP_NO_ADDR,
> +				   SPI_MEM_OP_NO_DUMMY,
> +				   SPI_MEM_OP_NO_DATA);
> +
> +		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> +
> +		ret = spi_mem_exec_op(nor->spimem, &op);
> +	} else {
> +		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_GBULK,
> +						       NULL, 0);
> +	}
> +
> +	if (ret) {
> +		dev_dbg(nor->dev, "error %d on Global Block Unlock\n", ret);
> +		return ret;
> +	}
> +
> +	return spi_nor_wait_till_ready(nor);
> +}
> +
>  /**
>   * spi_nor_write_sr() - Write the Status Register.
>   * @nor:	pointer to 'struct spi_nor'.
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index d631ee299de3..eb26796db026 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -434,6 +434,7 @@ int spi_nor_write_disable(struct spi_nor *nor);
>  int spi_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable);
>  int spi_nor_write_ear(struct spi_nor *nor, u8 ear);
>  int spi_nor_wait_till_ready(struct spi_nor *nor);
> +int spi_nor_global_block_unlock(struct spi_nor *nor);
>  int spi_nor_lock_and_prep(struct spi_nor *nor);
>  void spi_nor_unlock_and_unprep(struct spi_nor *nor);
>  int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor);
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index d13958de6d8a..a0d572855444 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -53,6 +53,7 @@
>  #define SPINOR_OP_WREAR		0xc5	/* Write Extended Address Register */
>  #define SPINOR_OP_SRSTEN	0x66	/* Software Reset Enable */
>  #define SPINOR_OP_SRST		0x99	/* Software Reset */
> +#define SPINOR_OP_GBULK		0x98    /* Global Block Unlock */
> 
>  /* 4-byte address opcodes - used on Spansion and some Macronix 
> flashes. */
>  #define SPINOR_OP_READ_4B	0x13	/* Read data bytes (low frequency) */

Re: [PATCH v2 1/2] mtd: spi-nor: Add Global Block Unlock command

[Michael Walle]

Am 2021-01-20 14:19, schrieb Tudor Ambarus:
> The Global Block Unlock command has different names depending
> on the manufacturer, but always the same command value: 0x98.
> Macronix's MX25U12835F names it Gang Block Unlock, Winbound's

Winbond

> W25Q128FV names it Global Block Unlock and Microchip's
> SST26VF064B names it Global Block Protection Unlock.
> 
> Used in the Individual Block Protection mode, which is mutually
> exclusive with the Block Protection mode (BP0-3).
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

Reviewed-by: Michael Walle <michael@walle.cc>

-michael

> ---
> v2:
> - s/mutual/mutually/
> - set the GBULK cmd buswidth to 0 and call spi_nor_spimem_setup_op()
> to update the it, because the op can can be issued in QPI mode as well.
> - add Pratyush's R-b tag
> 
>  drivers/mtd/spi-nor/core.c  | 37 +++++++++++++++++++++++++++++++++++++
>  drivers/mtd/spi-nor/core.h  |  1 +
>  include/linux/mtd/spi-nor.h |  1 +
>  3 files changed, 39 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 20df44b753da..e82732dd31e1 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -853,6 +853,43 @@ int spi_nor_wait_till_ready(struct spi_nor *nor)
>  						    DEFAULT_READY_WAIT_JIFFIES);
>  }
> 
> +/**
> + * spi_nor_global_block_unlock() - Unlock Global Block Protection.
> + * @nor:	pointer to 'struct spi_nor'.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +int spi_nor_global_block_unlock(struct spi_nor *nor)
> +{
> +	int ret;
> +
> +	ret = spi_nor_write_enable(nor);
> +	if (ret)
> +		return ret;
> +
> +	if (nor->spimem) {
> +		struct spi_mem_op op =
> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_GBULK, 0),
> +				   SPI_MEM_OP_NO_ADDR,
> +				   SPI_MEM_OP_NO_DUMMY,
> +				   SPI_MEM_OP_NO_DATA);
> +
> +		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> +
> +		ret = spi_mem_exec_op(nor->spimem, &op);
> +	} else {
> +		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_GBULK,
> +						       NULL, 0);
> +	}
> +
> +	if (ret) {
> +		dev_dbg(nor->dev, "error %d on Global Block Unlock\n", ret);
> +		return ret;
> +	}
> +
> +	return spi_nor_wait_till_ready(nor);
> +}
> +
>  /**
>   * spi_nor_write_sr() - Write the Status Register.
>   * @nor:	pointer to 'struct spi_nor'.
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index d631ee299de3..eb26796db026 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -434,6 +434,7 @@ int spi_nor_write_disable(struct spi_nor *nor);
>  int spi_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable);
>  int spi_nor_write_ear(struct spi_nor *nor, u8 ear);
>  int spi_nor_wait_till_ready(struct spi_nor *nor);
> +int spi_nor_global_block_unlock(struct spi_nor *nor);
>  int spi_nor_lock_and_prep(struct spi_nor *nor);
>  void spi_nor_unlock_and_unprep(struct spi_nor *nor);
>  int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor);
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index d13958de6d8a..a0d572855444 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -53,6 +53,7 @@
>  #define SPINOR_OP_WREAR		0xc5	/* Write Extended Address Register */
>  #define SPINOR_OP_SRSTEN	0x66	/* Software Reset Enable */
>  #define SPINOR_OP_SRST		0x99	/* Software Reset */
> +#define SPINOR_OP_GBULK		0x98    /* Global Block Unlock */
> 
>  /* 4-byte address opcodes - used on Spansion and some Macronix 
> flashes. */
>  #define SPINOR_OP_READ_4B	0x13	/* Read data bytes (low frequency) */

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 2/2] mtd: spi-nor: sst: Add support for Global Unlock on sst26vf

[Michael Walle]

Am 2021-01-20 14:19, schrieb Tudor Ambarus:
> Even if sst26vf shares the SPINOR_OP_GBULK opcode with
> Macronix (ex. MX25U12835F) and Winbound (ex. W25Q128FV),
> it has its own Individual Block Protection scheme, which
> is also capable to read-lock individual parameter blocks.
> Thus the sst26vf's Individual Block Protection scheme will
> reside in the sst.c manufacturer driver.
> 
> Add support to unlock the entire flash memory. The device
> is write-protected by default after a power-on reset cycle
> (volatile software protection), in order to avoid inadvertent
> writes during power-up. Could do an erase, write, read back,
> and compare when MTD_SPI_NOR_SWP_DISABLE_ON_VOLATILE=y.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---

Damn, who on earth assigned the "block protection" bits to the
8/32/64kb sectors on the flash (SST26VF064B DS, Table 5-6). That is
nuts.

Except one comment below:

Reviewed-by: Michael Walle <michael@walle.cc>

> v2: s/!ofs/ofs == 0/
> 
>  drivers/mtd/spi-nor/sst.c | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
> index 00e48da0744a..d6e1396abb96 100644
> --- a/drivers/mtd/spi-nor/sst.c
> +++ b/drivers/mtd/spi-nor/sst.c
> @@ -8,6 +8,39 @@
> 
>  #include "core.h"
> 
> +static int sst26vf_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int sst26vf_unlock(struct spi_nor *nor, loff_t ofs, uint64_t 
> len)
> +{
> +	if (ofs == 0 && len == nor->params->size)
> +		return spi_nor_global_block_unlock(nor);


Some blocks might not be unlocked because they are permanently
locked. Does it make sense to read BPNV of the control register
and add a debug message here?

-michael

> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static int sst26vf_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t 
> len)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static const struct spi_nor_locking_ops sst26vf_locking_ops = {
> +	.lock = sst26vf_lock,
> +	.unlock = sst26vf_unlock,
> +	.is_locked = sst26vf_is_locked,
> +};
> +
> +static void sst26vf_default_init(struct spi_nor *nor)
> +{
> +	nor->params->locking_ops = &sst26vf_locking_ops;
> +}
> +
> +static const struct spi_nor_fixups sst26vf_fixups = {
> +	.default_init = sst26vf_default_init,
> +};
> +
>  static const struct flash_info sst_parts[] = {
>  	/* SST -- large erase sizes are "overlays", "sectors" are 4K */
>  	{ "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024,  8,
> @@ -39,8 +72,9 @@ static const struct flash_info sst_parts[] = {
>  	{ "sst26vf016b", INFO(0xbf2641, 0, 64 * 1024, 32,
>  			      SECT_4K | SPI_NOR_DUAL_READ) },
>  	{ "sst26vf064b", INFO(0xbf2643, 0, 64 * 1024, 128,
> -			      SECT_4K | SPI_NOR_DUAL_READ |
> -			      SPI_NOR_QUAD_READ) },
> +			      SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> +			      SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE)
> +		.fixups = &sst26vf_fixups },
>  };
> 
>  static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,

Re: [PATCH v2 2/2] mtd: spi-nor: sst: Add support for Global Unlock on sst26vf

[Michael Walle]

Am 2021-01-20 14:19, schrieb Tudor Ambarus:
> Even if sst26vf shares the SPINOR_OP_GBULK opcode with
> Macronix (ex. MX25U12835F) and Winbound (ex. W25Q128FV),
> it has its own Individual Block Protection scheme, which
> is also capable to read-lock individual parameter blocks.
> Thus the sst26vf's Individual Block Protection scheme will
> reside in the sst.c manufacturer driver.
> 
> Add support to unlock the entire flash memory. The device
> is write-protected by default after a power-on reset cycle
> (volatile software protection), in order to avoid inadvertent
> writes during power-up. Could do an erase, write, read back,
> and compare when MTD_SPI_NOR_SWP_DISABLE_ON_VOLATILE=y.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---

Damn, who on earth assigned the "block protection" bits to the
8/32/64kb sectors on the flash (SST26VF064B DS, Table 5-6). That is
nuts.

Except one comment below:

Reviewed-by: Michael Walle <michael@walle.cc>

> v2: s/!ofs/ofs == 0/
> 
>  drivers/mtd/spi-nor/sst.c | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
> index 00e48da0744a..d6e1396abb96 100644
> --- a/drivers/mtd/spi-nor/sst.c
> +++ b/drivers/mtd/spi-nor/sst.c
> @@ -8,6 +8,39 @@
> 
>  #include "core.h"
> 
> +static int sst26vf_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int sst26vf_unlock(struct spi_nor *nor, loff_t ofs, uint64_t 
> len)
> +{
> +	if (ofs == 0 && len == nor->params->size)
> +		return spi_nor_global_block_unlock(nor);


Some blocks might not be unlocked because they are permanently
locked. Does it make sense to read BPNV of the control register
and add a debug message here?

-michael

> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static int sst26vf_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t 
> len)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static const struct spi_nor_locking_ops sst26vf_locking_ops = {
> +	.lock = sst26vf_lock,
> +	.unlock = sst26vf_unlock,
> +	.is_locked = sst26vf_is_locked,
> +};
> +
> +static void sst26vf_default_init(struct spi_nor *nor)
> +{
> +	nor->params->locking_ops = &sst26vf_locking_ops;
> +}
> +
> +static const struct spi_nor_fixups sst26vf_fixups = {
> +	.default_init = sst26vf_default_init,
> +};
> +
>  static const struct flash_info sst_parts[] = {
>  	/* SST -- large erase sizes are "overlays", "sectors" are 4K */
>  	{ "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024,  8,
> @@ -39,8 +72,9 @@ static const struct flash_info sst_parts[] = {
>  	{ "sst26vf016b", INFO(0xbf2641, 0, 64 * 1024, 32,
>  			      SECT_4K | SPI_NOR_DUAL_READ) },
>  	{ "sst26vf064b", INFO(0xbf2643, 0, 64 * 1024, 128,
> -			      SECT_4K | SPI_NOR_DUAL_READ |
> -			      SPI_NOR_QUAD_READ) },
> +			      SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> +			      SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE)
> +		.fixups = &sst26vf_fixups },
>  };
> 
>  static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 2/2] mtd: spi-nor: sst: Add support for Global Unlock on sst26vf

[Tudor.Ambarus]

On 1/20/21 4:05 PM, Michael Walle wrote:
>> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
>> index 00e48da0744a..d6e1396abb96 100644
>> --- a/drivers/mtd/spi-nor/sst.c
>> +++ b/drivers/mtd/spi-nor/sst.c
>> @@ -8,6 +8,39 @@
>>
>>  #include "core.h"
>>
>> +static int sst26vf_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>> +{
>> +     return -EOPNOTSUPP;
>> +}
>> +
>> +static int sst26vf_unlock(struct spi_nor *nor, loff_t ofs, uint64_t
>> len)
>> +{
>> +     if (ofs == 0 && len == nor->params->size)
>> +             return spi_nor_global_block_unlock(nor);
> 
> 
> Some blocks might not be unlocked because they are permanently
> locked. Does it make sense to read BPNV of the control register
> and add a debug message here?

It would, yes. If any block is permanently locked in the unlock_all case,
I'll just print a dbg message and return -EINVAL. Sounds good?

Cheers,
ta

Re: [PATCH v2 2/2] mtd: spi-nor: sst: Add support for Global Unlock on sst26vf

[Tudor.Ambarus]

On 1/20/21 4:05 PM, Michael Walle wrote:
>> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
>> index 00e48da0744a..d6e1396abb96 100644
>> --- a/drivers/mtd/spi-nor/sst.c
>> +++ b/drivers/mtd/spi-nor/sst.c
>> @@ -8,6 +8,39 @@
>>
>>  #include "core.h"
>>
>> +static int sst26vf_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
>> +{
>> +     return -EOPNOTSUPP;
>> +}
>> +
>> +static int sst26vf_unlock(struct spi_nor *nor, loff_t ofs, uint64_t
>> len)
>> +{
>> +     if (ofs == 0 && len == nor->params->size)
>> +             return spi_nor_global_block_unlock(nor);
> 
> 
> Some blocks might not be unlocked because they are permanently
> locked. Does it make sense to read BPNV of the control register
> and add a debug message here?

It would, yes. If any block is permanently locked in the unlock_all case,
I'll just print a dbg message and return -EINVAL. Sounds good?

Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 2/2] mtd: spi-nor: sst: Add support for Global Unlock on sst26vf

[Michael Walle]

Am 2021-01-20 15:52, schrieb Tudor.Ambarus@microchip.com:
> On 1/20/21 4:05 PM, Michael Walle wrote:
>>> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
>>> index 00e48da0744a..d6e1396abb96 100644
>>> --- a/drivers/mtd/spi-nor/sst.c
>>> +++ b/drivers/mtd/spi-nor/sst.c
>>> @@ -8,6 +8,39 @@
>>> 
>>>  #include "core.h"
>>> 
>>> +static int sst26vf_lock(struct spi_nor *nor, loff_t ofs, uint64_t 
>>> len)
>>> +{
>>> +     return -EOPNOTSUPP;
>>> +}
>>> +
>>> +static int sst26vf_unlock(struct spi_nor *nor, loff_t ofs, uint64_t
>>> len)
>>> +{
>>> +     if (ofs == 0 && len == nor->params->size)
>>> +             return spi_nor_global_block_unlock(nor);
>> 
>> 
>> Some blocks might not be unlocked because they are permanently
>> locked. Does it make sense to read BPNV of the control register
>> and add a debug message here?
> 
> It would, yes. If any block is permanently locked in the unlock_all 
> case,
> I'll just print a dbg message and return -EINVAL. Sounds good?

spi_nor_sr_unlock(), atmel_at25fs_unlock() and atmel_global_unprotect()
will return -EIO in case the SR wasn't writable.

-michael

Re: [PATCH v2 2/2] mtd: spi-nor: sst: Add support for Global Unlock on sst26vf

[Michael Walle]

Am 2021-01-20 15:52, schrieb Tudor.Ambarus@microchip.com:
> On 1/20/21 4:05 PM, Michael Walle wrote:
>>> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
>>> index 00e48da0744a..d6e1396abb96 100644
>>> --- a/drivers/mtd/spi-nor/sst.c
>>> +++ b/drivers/mtd/spi-nor/sst.c
>>> @@ -8,6 +8,39 @@
>>> 
>>>  #include "core.h"
>>> 
>>> +static int sst26vf_lock(struct spi_nor *nor, loff_t ofs, uint64_t 
>>> len)
>>> +{
>>> +     return -EOPNOTSUPP;
>>> +}
>>> +
>>> +static int sst26vf_unlock(struct spi_nor *nor, loff_t ofs, uint64_t
>>> len)
>>> +{
>>> +     if (ofs == 0 && len == nor->params->size)
>>> +             return spi_nor_global_block_unlock(nor);
>> 
>> 
>> Some blocks might not be unlocked because they are permanently
>> locked. Does it make sense to read BPNV of the control register
>> and add a debug message here?
> 
> It would, yes. If any block is permanently locked in the unlock_all 
> case,
> I'll just print a dbg message and return -EINVAL. Sounds good?

spi_nor_sr_unlock(), atmel_at25fs_unlock() and atmel_global_unprotect()
will return -EIO in case the SR wasn't writable.

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 2/2] mtd: spi-nor: sst: Add support for Global Unlock on sst26vf

[Tudor.Ambarus]

On 1/20/21 5:02 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2021-01-20 15:52, schrieb Tudor.Ambarus@microchip.com:
>> On 1/20/21 4:05 PM, Michael Walle wrote:
>>>> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
>>>> index 00e48da0744a..d6e1396abb96 100644
>>>> --- a/drivers/mtd/spi-nor/sst.c
>>>> +++ b/drivers/mtd/spi-nor/sst.c
>>>> @@ -8,6 +8,39 @@
>>>>
>>>>  #include "core.h"
>>>>
>>>> +static int sst26vf_lock(struct spi_nor *nor, loff_t ofs, uint64_t
>>>> len)
>>>> +{
>>>> +     return -EOPNOTSUPP;
>>>> +}
>>>> +
>>>> +static int sst26vf_unlock(struct spi_nor *nor, loff_t ofs, uint64_t
>>>> len)
>>>> +{
>>>> +     if (ofs == 0 && len == nor->params->size)
>>>> +             return spi_nor_global_block_unlock(nor);
>>>
>>>
>>> Some blocks might not be unlocked because they are permanently
>>> locked. Does it make sense to read BPNV of the control register
>>> and add a debug message here?
>>
>> It would, yes. If any block is permanently locked in the unlock_all
>> case,
>> I'll just print a dbg message and return -EINVAL. Sounds good?
> 
> spi_nor_sr_unlock(), atmel_at25fs_unlock() and atmel_global_unprotect()
> will return -EIO in case the SR wasn't writable.

You mean in the spi_nor_write_sr_and_check() calls. -EIO is fine
there if what we wrote is different than what we read back, it would
indicate an IO error.

GBULK command clears all the write-protection bits in the Block
Protection register, except for those bits that have been permanently
locked down. So even if we have few blocks permanently locked, i.e.
CR.BPNV == 1, the GBULK can clear the protection for the remaining
blocks. So not really an IO error, but rather an -EINVAL, because
the user asks to unlock more than we can.

Cheers,
ta

Re: [PATCH v2 2/2] mtd: spi-nor: sst: Add support for Global Unlock on sst26vf

[Tudor.Ambarus]

On 1/20/21 5:02 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2021-01-20 15:52, schrieb Tudor.Ambarus@microchip.com:
>> On 1/20/21 4:05 PM, Michael Walle wrote:
>>>> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
>>>> index 00e48da0744a..d6e1396abb96 100644
>>>> --- a/drivers/mtd/spi-nor/sst.c
>>>> +++ b/drivers/mtd/spi-nor/sst.c
>>>> @@ -8,6 +8,39 @@
>>>>
>>>>  #include "core.h"
>>>>
>>>> +static int sst26vf_lock(struct spi_nor *nor, loff_t ofs, uint64_t
>>>> len)
>>>> +{
>>>> +     return -EOPNOTSUPP;
>>>> +}
>>>> +
>>>> +static int sst26vf_unlock(struct spi_nor *nor, loff_t ofs, uint64_t
>>>> len)
>>>> +{
>>>> +     if (ofs == 0 && len == nor->params->size)
>>>> +             return spi_nor_global_block_unlock(nor);
>>>
>>>
>>> Some blocks might not be unlocked because they are permanently
>>> locked. Does it make sense to read BPNV of the control register
>>> and add a debug message here?
>>
>> It would, yes. If any block is permanently locked in the unlock_all
>> case,
>> I'll just print a dbg message and return -EINVAL. Sounds good?
> 
> spi_nor_sr_unlock(), atmel_at25fs_unlock() and atmel_global_unprotect()
> will return -EIO in case the SR wasn't writable.

You mean in the spi_nor_write_sr_and_check() calls. -EIO is fine
there if what we wrote is different than what we read back, it would
indicate an IO error.

GBULK command clears all the write-protection bits in the Block
Protection register, except for those bits that have been permanently
locked down. So even if we have few blocks permanently locked, i.e.
CR.BPNV == 1, the GBULK can clear the protection for the remaining
blocks. So not really an IO error, but rather an -EINVAL, because
the user asks to unlock more than we can.

Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 2/2] mtd: spi-nor: sst: Add support for Global Unlock on sst26vf

[Michael Walle]

Am 2021-01-20 16:39, schrieb Tudor.Ambarus@microchip.com:
> On 1/20/21 5:02 PM, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Am 2021-01-20 15:52, schrieb Tudor.Ambarus@microchip.com:
>>> On 1/20/21 4:05 PM, Michael Walle wrote:
>>>>> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
>>>>> index 00e48da0744a..d6e1396abb96 100644
>>>>> --- a/drivers/mtd/spi-nor/sst.c
>>>>> +++ b/drivers/mtd/spi-nor/sst.c
>>>>> @@ -8,6 +8,39 @@
>>>>> 
>>>>>  #include "core.h"
>>>>> 
>>>>> +static int sst26vf_lock(struct spi_nor *nor, loff_t ofs, uint64_t
>>>>> len)
>>>>> +{
>>>>> +     return -EOPNOTSUPP;
>>>>> +}
>>>>> +
>>>>> +static int sst26vf_unlock(struct spi_nor *nor, loff_t ofs, 
>>>>> uint64_t
>>>>> len)
>>>>> +{
>>>>> +     if (ofs == 0 && len == nor->params->size)
>>>>> +             return spi_nor_global_block_unlock(nor);
>>>> 
>>>> 
>>>> Some blocks might not be unlocked because they are permanently
>>>> locked. Does it make sense to read BPNV of the control register
>>>> and add a debug message here?
>>> 
>>> It would, yes. If any block is permanently locked in the unlock_all
>>> case,
>>> I'll just print a dbg message and return -EINVAL. Sounds good?
>> 
>> spi_nor_sr_unlock(), atmel_at25fs_unlock() and 
>> atmel_global_unprotect()
>> will return -EIO in case the SR wasn't writable.
> 
> You mean in the spi_nor_write_sr_and_check() calls. -EIO is fine
> there if what we wrote is different than what we read back, it would
> indicate an IO error.
> 
> GBULK command clears all the write-protection bits in the Block
> Protection register, except for those bits that have been permanently
> locked down. So even if we have few blocks permanently locked, i.e.
> CR.BPNV == 1, the GBULK can clear the protection for the remaining
> blocks. So not really an IO error, but rather an -EINVAL, because
> the user asks to unlock more than we can.

Doesn't EINVAL indicate wrong parameters, but does nothing? In this
case, unlock would be partially successful.

In any case, my point was that depending on the underlying locking
ops, either -EIO or -EINVAL is returned if spi_nor_unlock() fails
for the same reason, that is unlock() wasn't possible because of
some sort of hardware write protection. And IMHO it should return
the same errno (whatever the correct errno is in this case).

-michael

Re: [PATCH v2 2/2] mtd: spi-nor: sst: Add support for Global Unlock on sst26vf

[Michael Walle]

Am 2021-01-20 16:39, schrieb Tudor.Ambarus@microchip.com:
> On 1/20/21 5:02 PM, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Am 2021-01-20 15:52, schrieb Tudor.Ambarus@microchip.com:
>>> On 1/20/21 4:05 PM, Michael Walle wrote:
>>>>> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
>>>>> index 00e48da0744a..d6e1396abb96 100644
>>>>> --- a/drivers/mtd/spi-nor/sst.c
>>>>> +++ b/drivers/mtd/spi-nor/sst.c
>>>>> @@ -8,6 +8,39 @@
>>>>> 
>>>>>  #include "core.h"
>>>>> 
>>>>> +static int sst26vf_lock(struct spi_nor *nor, loff_t ofs, uint64_t
>>>>> len)
>>>>> +{
>>>>> +     return -EOPNOTSUPP;
>>>>> +}
>>>>> +
>>>>> +static int sst26vf_unlock(struct spi_nor *nor, loff_t ofs, 
>>>>> uint64_t
>>>>> len)
>>>>> +{
>>>>> +     if (ofs == 0 && len == nor->params->size)
>>>>> +             return spi_nor_global_block_unlock(nor);
>>>> 
>>>> 
>>>> Some blocks might not be unlocked because they are permanently
>>>> locked. Does it make sense to read BPNV of the control register
>>>> and add a debug message here?
>>> 
>>> It would, yes. If any block is permanently locked in the unlock_all
>>> case,
>>> I'll just print a dbg message and return -EINVAL. Sounds good?
>> 
>> spi_nor_sr_unlock(), atmel_at25fs_unlock() and 
>> atmel_global_unprotect()
>> will return -EIO in case the SR wasn't writable.
> 
> You mean in the spi_nor_write_sr_and_check() calls. -EIO is fine
> there if what we wrote is different than what we read back, it would
> indicate an IO error.
> 
> GBULK command clears all the write-protection bits in the Block
> Protection register, except for those bits that have been permanently
> locked down. So even if we have few blocks permanently locked, i.e.
> CR.BPNV == 1, the GBULK can clear the protection for the remaining
> blocks. So not really an IO error, but rather an -EINVAL, because
> the user asks to unlock more than we can.

Doesn't EINVAL indicate wrong parameters, but does nothing? In this
case, unlock would be partially successful.

In any case, my point was that depending on the underlying locking
ops, either -EIO or -EINVAL is returned if spi_nor_unlock() fails
for the same reason, that is unlock() wasn't possible because of
some sort of hardware write protection. And IMHO it should return
the same errno (whatever the correct errno is in this case).

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 2/2] mtd: spi-nor: sst: Add support for Global Unlock on sst26vf

[Tudor.Ambarus]

On 1/20/21 5:49 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2021-01-20 16:39, schrieb Tudor.Ambarus@microchip.com:
>> On 1/20/21 5:02 PM, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Am 2021-01-20 15:52, schrieb Tudor.Ambarus@microchip.com:
>>>> On 1/20/21 4:05 PM, Michael Walle wrote:
>>>>>> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
>>>>>> index 00e48da0744a..d6e1396abb96 100644
>>>>>> --- a/drivers/mtd/spi-nor/sst.c
>>>>>> +++ b/drivers/mtd/spi-nor/sst.c
>>>>>> @@ -8,6 +8,39 @@
>>>>>>
>>>>>>  #include "core.h"
>>>>>>
>>>>>> +static int sst26vf_lock(struct spi_nor *nor, loff_t ofs, uint64_t
>>>>>> len)
>>>>>> +{
>>>>>> +     return -EOPNOTSUPP;
>>>>>> +}
>>>>>> +
>>>>>> +static int sst26vf_unlock(struct spi_nor *nor, loff_t ofs,
>>>>>> uint64_t
>>>>>> len)
>>>>>> +{
>>>>>> +     if (ofs == 0 && len == nor->params->size)
>>>>>> +             return spi_nor_global_block_unlock(nor);
>>>>>
>>>>>
>>>>> Some blocks might not be unlocked because they are permanently
>>>>> locked. Does it make sense to read BPNV of the control register
>>>>> and add a debug message here?
>>>>
>>>> It would, yes. If any block is permanently locked in the unlock_all
>>>> case,
>>>> I'll just print a dbg message and return -EINVAL. Sounds good?
>>>
>>> spi_nor_sr_unlock(), atmel_at25fs_unlock() and
>>> atmel_global_unprotect()
>>> will return -EIO in case the SR wasn't writable.
>>
>> You mean in the spi_nor_write_sr_and_check() calls. -EIO is fine
>> there if what we wrote is different than what we read back, it would
>> indicate an IO error.
>>
>> GBULK command clears all the write-protection bits in the Block
>> Protection register, except for those bits that have been permanently
>> locked down. So even if we have few blocks permanently locked, i.e.
>> CR.BPNV == 1, the GBULK can clear the protection for the remaining
>> blocks. So not really an IO error, but rather an -EINVAL, because
>> the user asks to unlock more than we can.
> 
> Doesn't EINVAL indicate wrong parameters, but does nothing? In this
> case, unlock would be partially successful.
> 
yes, that's what I said I'll do: "If any block is permanently locked
in the unlock_all case, I'll just print a dbg message and return -EINVAL",
without sending a GBULK cmd. Caller wrongly asks to unlock all, when we
can just unlock partial memory. 

It's similar to what is at:
https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/tree/drivers/mtd/spi-nor/core.c?h=spi-nor/next#n1946

> In any case, my point was that depending on the underlying locking
> ops, either -EIO or -EINVAL is returned if spi_nor_unlock() fails
> for the same reason, that is unlock() wasn't possible because of
> some sort of hardware write protection. And IMHO it should return
> the same errno (whatever the correct errno is in this case).
> 

But the reasons are different: 1/caller wrongly asks to unlock
more than we can, thus -EINVAL 2/ -EIO when we don't read what
we expect to read.

Cheers,
ta

Re: [PATCH v2 2/2] mtd: spi-nor: sst: Add support for Global Unlock on sst26vf

[Tudor.Ambarus]

On 1/20/21 5:49 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2021-01-20 16:39, schrieb Tudor.Ambarus@microchip.com:
>> On 1/20/21 5:02 PM, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Am 2021-01-20 15:52, schrieb Tudor.Ambarus@microchip.com:
>>>> On 1/20/21 4:05 PM, Michael Walle wrote:
>>>>>> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
>>>>>> index 00e48da0744a..d6e1396abb96 100644
>>>>>> --- a/drivers/mtd/spi-nor/sst.c
>>>>>> +++ b/drivers/mtd/spi-nor/sst.c
>>>>>> @@ -8,6 +8,39 @@
>>>>>>
>>>>>>  #include "core.h"
>>>>>>
>>>>>> +static int sst26vf_lock(struct spi_nor *nor, loff_t ofs, uint64_t
>>>>>> len)
>>>>>> +{
>>>>>> +     return -EOPNOTSUPP;
>>>>>> +}
>>>>>> +
>>>>>> +static int sst26vf_unlock(struct spi_nor *nor, loff_t ofs,
>>>>>> uint64_t
>>>>>> len)
>>>>>> +{
>>>>>> +     if (ofs == 0 && len == nor->params->size)
>>>>>> +             return spi_nor_global_block_unlock(nor);
>>>>>
>>>>>
>>>>> Some blocks might not be unlocked because they are permanently
>>>>> locked. Does it make sense to read BPNV of the control register
>>>>> and add a debug message here?
>>>>
>>>> It would, yes. If any block is permanently locked in the unlock_all
>>>> case,
>>>> I'll just print a dbg message and return -EINVAL. Sounds good?
>>>
>>> spi_nor_sr_unlock(), atmel_at25fs_unlock() and
>>> atmel_global_unprotect()
>>> will return -EIO in case the SR wasn't writable.
>>
>> You mean in the spi_nor_write_sr_and_check() calls. -EIO is fine
>> there if what we wrote is different than what we read back, it would
>> indicate an IO error.
>>
>> GBULK command clears all the write-protection bits in the Block
>> Protection register, except for those bits that have been permanently
>> locked down. So even if we have few blocks permanently locked, i.e.
>> CR.BPNV == 1, the GBULK can clear the protection for the remaining
>> blocks. So not really an IO error, but rather an -EINVAL, because
>> the user asks to unlock more than we can.
> 
> Doesn't EINVAL indicate wrong parameters, but does nothing? In this
> case, unlock would be partially successful.
> 
yes, that's what I said I'll do: "If any block is permanently locked
in the unlock_all case, I'll just print a dbg message and return -EINVAL",
without sending a GBULK cmd. Caller wrongly asks to unlock all, when we
can just unlock partial memory. 

It's similar to what is at:
https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/tree/drivers/mtd/spi-nor/core.c?h=spi-nor/next#n1946

> In any case, my point was that depending on the underlying locking
> ops, either -EIO or -EINVAL is returned if spi_nor_unlock() fails
> for the same reason, that is unlock() wasn't possible because of
> some sort of hardware write protection. And IMHO it should return
> the same errno (whatever the correct errno is in this case).
> 

But the reasons are different: 1/caller wrongly asks to unlock
more than we can, thus -EINVAL 2/ -EIO when we don't read what
we expect to read.

Cheers,
ta

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 2/2] mtd: spi-nor: sst: Add support for Global Unlock on sst26vf

[Michael Walle]

Am 2021-01-20 17:25, schrieb Tudor.Ambarus@microchip.com:
> On 1/20/21 5:49 PM, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Am 2021-01-20 16:39, schrieb Tudor.Ambarus@microchip.com:
>>> On 1/20/21 5:02 PM, Michael Walle wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you 
>>>> know
>>>> the content is safe
>>>> 
>>>> Am 2021-01-20 15:52, schrieb Tudor.Ambarus@microchip.com:
>>>>> On 1/20/21 4:05 PM, Michael Walle wrote:
>>>>>>> diff --git a/drivers/mtd/spi-nor/sst.c 
>>>>>>> b/drivers/mtd/spi-nor/sst.c
>>>>>>> index 00e48da0744a..d6e1396abb96 100644
>>>>>>> --- a/drivers/mtd/spi-nor/sst.c
>>>>>>> +++ b/drivers/mtd/spi-nor/sst.c
>>>>>>> @@ -8,6 +8,39 @@
>>>>>>> 
>>>>>>>  #include "core.h"
>>>>>>> 
>>>>>>> +static int sst26vf_lock(struct spi_nor *nor, loff_t ofs, 
>>>>>>> uint64_t
>>>>>>> len)
>>>>>>> +{
>>>>>>> +     return -EOPNOTSUPP;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int sst26vf_unlock(struct spi_nor *nor, loff_t ofs,
>>>>>>> uint64_t
>>>>>>> len)
>>>>>>> +{
>>>>>>> +     if (ofs == 0 && len == nor->params->size)
>>>>>>> +             return spi_nor_global_block_unlock(nor);
>>>>>> 
>>>>>> 
>>>>>> Some blocks might not be unlocked because they are permanently
>>>>>> locked. Does it make sense to read BPNV of the control register
>>>>>> and add a debug message here?
>>>>> 
>>>>> It would, yes. If any block is permanently locked in the unlock_all
>>>>> case,
>>>>> I'll just print a dbg message and return -EINVAL. Sounds good?
>>>> 
>>>> spi_nor_sr_unlock(), atmel_at25fs_unlock() and
>>>> atmel_global_unprotect()
>>>> will return -EIO in case the SR wasn't writable.
>>> 
>>> You mean in the spi_nor_write_sr_and_check() calls. -EIO is fine
>>> there if what we wrote is different than what we read back, it would
>>> indicate an IO error.
>>> 
>>> GBULK command clears all the write-protection bits in the Block
>>> Protection register, except for those bits that have been permanently
>>> locked down. So even if we have few blocks permanently locked, i.e.
>>> CR.BPNV == 1, the GBULK can clear the protection for the remaining
>>> blocks. So not really an IO error, but rather an -EINVAL, because
>>> the user asks to unlock more than we can.
>> 
>> Doesn't EINVAL indicate wrong parameters, but does nothing? In this
>> case, unlock would be partially successful.
>> 
> yes, that's what I said I'll do: "If any block is permanently locked
> in the unlock_all case, I'll just print a dbg message and return 
> -EINVAL",
> without sending a GBULK cmd. Caller wrongly asks to unlock all, when we
> can just unlock partial memory.

Doh, I've missed that you will do it beforehand. Yes then EINVAL
is fine by me.

But you won't unlock the flash during startup (given the config option
is enabled) if any blocks has been permanently locked. Thus if just the
topmost 4k block is permanently locked down, the whole flash wouldn't be
writable, right?. I don't have a strong opinion on that.

-michael

> 
> It's similar to what is at:
> https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/tree/drivers/mtd/spi-nor/core.c?h=spi-nor/next#n1946
> 
>> In any case, my point was that depending on the underlying locking
>> ops, either -EIO or -EINVAL is returned if spi_nor_unlock() fails
>> for the same reason, that is unlock() wasn't possible because of
>> some sort of hardware write protection. And IMHO it should return
>> the same errno (whatever the correct errno is in this case).
>> 
> 
> But the reasons are different: 1/caller wrongly asks to unlock
> more than we can, thus -EINVAL 2/ -EIO when we don't read what
> we expect to read.

-michael

Re: [PATCH v2 2/2] mtd: spi-nor: sst: Add support for Global Unlock on sst26vf

[Michael Walle]

Am 2021-01-20 17:25, schrieb Tudor.Ambarus@microchip.com:
> On 1/20/21 5:49 PM, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Am 2021-01-20 16:39, schrieb Tudor.Ambarus@microchip.com:
>>> On 1/20/21 5:02 PM, Michael Walle wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you 
>>>> know
>>>> the content is safe
>>>> 
>>>> Am 2021-01-20 15:52, schrieb Tudor.Ambarus@microchip.com:
>>>>> On 1/20/21 4:05 PM, Michael Walle wrote:
>>>>>>> diff --git a/drivers/mtd/spi-nor/sst.c 
>>>>>>> b/drivers/mtd/spi-nor/sst.c
>>>>>>> index 00e48da0744a..d6e1396abb96 100644
>>>>>>> --- a/drivers/mtd/spi-nor/sst.c
>>>>>>> +++ b/drivers/mtd/spi-nor/sst.c
>>>>>>> @@ -8,6 +8,39 @@
>>>>>>> 
>>>>>>>  #include "core.h"
>>>>>>> 
>>>>>>> +static int sst26vf_lock(struct spi_nor *nor, loff_t ofs, 
>>>>>>> uint64_t
>>>>>>> len)
>>>>>>> +{
>>>>>>> +     return -EOPNOTSUPP;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int sst26vf_unlock(struct spi_nor *nor, loff_t ofs,
>>>>>>> uint64_t
>>>>>>> len)
>>>>>>> +{
>>>>>>> +     if (ofs == 0 && len == nor->params->size)
>>>>>>> +             return spi_nor_global_block_unlock(nor);
>>>>>> 
>>>>>> 
>>>>>> Some blocks might not be unlocked because they are permanently
>>>>>> locked. Does it make sense to read BPNV of the control register
>>>>>> and add a debug message here?
>>>>> 
>>>>> It would, yes. If any block is permanently locked in the unlock_all
>>>>> case,
>>>>> I'll just print a dbg message and return -EINVAL. Sounds good?
>>>> 
>>>> spi_nor_sr_unlock(), atmel_at25fs_unlock() and
>>>> atmel_global_unprotect()
>>>> will return -EIO in case the SR wasn't writable.
>>> 
>>> You mean in the spi_nor_write_sr_and_check() calls. -EIO is fine
>>> there if what we wrote is different than what we read back, it would
>>> indicate an IO error.
>>> 
>>> GBULK command clears all the write-protection bits in the Block
>>> Protection register, except for those bits that have been permanently
>>> locked down. So even if we have few blocks permanently locked, i.e.
>>> CR.BPNV == 1, the GBULK can clear the protection for the remaining
>>> blocks. So not really an IO error, but rather an -EINVAL, because
>>> the user asks to unlock more than we can.
>> 
>> Doesn't EINVAL indicate wrong parameters, but does nothing? In this
>> case, unlock would be partially successful.
>> 
> yes, that's what I said I'll do: "If any block is permanently locked
> in the unlock_all case, I'll just print a dbg message and return 
> -EINVAL",
> without sending a GBULK cmd. Caller wrongly asks to unlock all, when we
> can just unlock partial memory.

Doh, I've missed that you will do it beforehand. Yes then EINVAL
is fine by me.

But you won't unlock the flash during startup (given the config option
is enabled) if any blocks has been permanently locked. Thus if just the
topmost 4k block is permanently locked down, the whole flash wouldn't be
writable, right?. I don't have a strong opinion on that.

-michael

> 
> It's similar to what is at:
> https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/tree/drivers/mtd/spi-nor/core.c?h=spi-nor/next#n1946
> 
>> In any case, my point was that depending on the underlying locking
>> ops, either -EIO or -EINVAL is returned if spi_nor_unlock() fails
>> for the same reason, that is unlock() wasn't possible because of
>> some sort of hardware write protection. And IMHO it should return
>> the same errno (whatever the correct errno is in this case).
>> 
> 
> But the reasons are different: 1/caller wrongly asks to unlock
> more than we can, thus -EINVAL 2/ -EIO when we don't read what
> we expect to read.

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 2/2] mtd: spi-nor: sst: Add support for Global Unlock on sst26vf

[Tudor.Ambarus]

On 1/20/21 6:47 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2021-01-20 17:25, schrieb Tudor.Ambarus@microchip.com:
>> On 1/20/21 5:49 PM, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Am 2021-01-20 16:39, schrieb Tudor.Ambarus@microchip.com:
>>>> On 1/20/21 5:02 PM, Michael Walle wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>> know
>>>>> the content is safe
>>>>>
>>>>> Am 2021-01-20 15:52, schrieb Tudor.Ambarus@microchip.com:
>>>>>> On 1/20/21 4:05 PM, Michael Walle wrote:
>>>>>>>> diff --git a/drivers/mtd/spi-nor/sst.c
>>>>>>>> b/drivers/mtd/spi-nor/sst.c
>>>>>>>> index 00e48da0744a..d6e1396abb96 100644
>>>>>>>> --- a/drivers/mtd/spi-nor/sst.c
>>>>>>>> +++ b/drivers/mtd/spi-nor/sst.c
>>>>>>>> @@ -8,6 +8,39 @@
>>>>>>>>
>>>>>>>>  #include "core.h"
>>>>>>>>
>>>>>>>> +static int sst26vf_lock(struct spi_nor *nor, loff_t ofs,
>>>>>>>> uint64_t
>>>>>>>> len)
>>>>>>>> +{
>>>>>>>> +     return -EOPNOTSUPP;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int sst26vf_unlock(struct spi_nor *nor, loff_t ofs,
>>>>>>>> uint64_t
>>>>>>>> len)
>>>>>>>> +{
>>>>>>>> +     if (ofs == 0 && len == nor->params->size)
>>>>>>>> +             return spi_nor_global_block_unlock(nor);
>>>>>>>
>>>>>>>
>>>>>>> Some blocks might not be unlocked because they are permanently
>>>>>>> locked. Does it make sense to read BPNV of the control register
>>>>>>> and add a debug message here?
>>>>>>
>>>>>> It would, yes. If any block is permanently locked in the unlock_all
>>>>>> case,
>>>>>> I'll just print a dbg message and return -EINVAL. Sounds good?
>>>>>
>>>>> spi_nor_sr_unlock(), atmel_at25fs_unlock() and
>>>>> atmel_global_unprotect()
>>>>> will return -EIO in case the SR wasn't writable.
>>>>
>>>> You mean in the spi_nor_write_sr_and_check() calls. -EIO is fine
>>>> there if what we wrote is different than what we read back, it would
>>>> indicate an IO error.
>>>>
>>>> GBULK command clears all the write-protection bits in the Block
>>>> Protection register, except for those bits that have been permanently
>>>> locked down. So even if we have few blocks permanently locked, i.e.
>>>> CR.BPNV == 1, the GBULK can clear the protection for the remaining
>>>> blocks. So not really an IO error, but rather an -EINVAL, because
>>>> the user asks to unlock more than we can.
>>>
>>> Doesn't EINVAL indicate wrong parameters, but does nothing? In this
>>> case, unlock would be partially successful.
>>>
>> yes, that's what I said I'll do: "If any block is permanently locked
>> in the unlock_all case, I'll just print a dbg message and return
>> -EINVAL",
>> without sending a GBULK cmd. Caller wrongly asks to unlock all, when we
>> can just unlock partial memory.
> 
> Doh, I've missed that you will do it beforehand. Yes then EINVAL
> is fine by me.
> 
> But you won't unlock the flash during startup (given the config option
> is enabled) if any blocks has been permanently locked. Thus if just the
> topmost 4k block is permanently locked down, the whole flash wouldn't be
> writable, right?. I don't have a strong opinion on that.

Correct. I don't see problems with that. Individual Block protection
with unlock on a smaller granularity can be added later on, and the
behavior during boot will remain the same.

Cheers,
ta

Re: [PATCH v2 2/2] mtd: spi-nor: sst: Add support for Global Unlock on sst26vf

[Tudor.Ambarus]

On 1/20/21 6:47 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2021-01-20 17:25, schrieb Tudor.Ambarus@microchip.com:
>> On 1/20/21 5:49 PM, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Am 2021-01-20 16:39, schrieb Tudor.Ambarus@microchip.com:
>>>> On 1/20/21 5:02 PM, Michael Walle wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>> know
>>>>> the content is safe
>>>>>
>>>>> Am 2021-01-20 15:52, schrieb Tudor.Ambarus@microchip.com:
>>>>>> On 1/20/21 4:05 PM, Michael Walle wrote:
>>>>>>>> diff --git a/drivers/mtd/spi-nor/sst.c
>>>>>>>> b/drivers/mtd/spi-nor/sst.c
>>>>>>>> index 00e48da0744a..d6e1396abb96 100644
>>>>>>>> --- a/drivers/mtd/spi-nor/sst.c
>>>>>>>> +++ b/drivers/mtd/spi-nor/sst.c
>>>>>>>> @@ -8,6 +8,39 @@
>>>>>>>>
>>>>>>>>  #include "core.h"
>>>>>>>>
>>>>>>>> +static int sst26vf_lock(struct spi_nor *nor, loff_t ofs,
>>>>>>>> uint64_t
>>>>>>>> len)
>>>>>>>> +{
>>>>>>>> +     return -EOPNOTSUPP;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int sst26vf_unlock(struct spi_nor *nor, loff_t ofs,
>>>>>>>> uint64_t
>>>>>>>> len)
>>>>>>>> +{
>>>>>>>> +     if (ofs == 0 && len == nor->params->size)
>>>>>>>> +             return spi_nor_global_block_unlock(nor);
>>>>>>>
>>>>>>>
>>>>>>> Some blocks might not be unlocked because they are permanently
>>>>>>> locked. Does it make sense to read BPNV of the control register
>>>>>>> and add a debug message here?
>>>>>>
>>>>>> It would, yes. If any block is permanently locked in the unlock_all
>>>>>> case,
>>>>>> I'll just print a dbg message and return -EINVAL. Sounds good?
>>>>>
>>>>> spi_nor_sr_unlock(), atmel_at25fs_unlock() and
>>>>> atmel_global_unprotect()
>>>>> will return -EIO in case the SR wasn't writable.
>>>>
>>>> You mean in the spi_nor_write_sr_and_check() calls. -EIO is fine
>>>> there if what we wrote is different than what we read back, it would
>>>> indicate an IO error.
>>>>
>>>> GBULK command clears all the write-protection bits in the Block
>>>> Protection register, except for those bits that have been permanently
>>>> locked down. So even if we have few blocks permanently locked, i.e.
>>>> CR.BPNV == 1, the GBULK can clear the protection for the remaining
>>>> blocks. So not really an IO error, but rather an -EINVAL, because
>>>> the user asks to unlock more than we can.
>>>
>>> Doesn't EINVAL indicate wrong parameters, but does nothing? In this
>>> case, unlock would be partially successful.
>>>
>> yes, that's what I said I'll do: "If any block is permanently locked
>> in the unlock_all case, I'll just print a dbg message and return
>> -EINVAL",
>> without sending a GBULK cmd. Caller wrongly asks to unlock all, when we
>> can just unlock partial memory.
> 
> Doh, I've missed that you will do it beforehand. Yes then EINVAL
> is fine by me.
> 
> But you won't unlock the flash during startup (given the config option
> is enabled) if any blocks has been permanently locked. Thus if just the
> topmost 4k block is permanently locked down, the whole flash wouldn't be
> writable, right?. I don't have a strong opinion on that.

Correct. I don't see problems with that. Individual Block protection
with unlock on a smaller granularity can be added later on, and the
behavior during boot will remain the same.

Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/