[PATCH v4 0/4] mtd: spi-nor: otp: 4 byte mode fix and erase support

[PATCH v4 0/4] mtd: spi-nor: otp: 4 byte mode fix and erase support

[Michael Walle]

This series is the follow up on the single patch
mtd: spi-nor: implement OTP erase for Winbond and similar flashes

Pratyush Yadav discovered a likely problem with bigger flashes, the address
to access the security registers is either 3 or 4 byte (at least for
winbond flashes).

Changes since v3:
 - new patch to check for read-only OTP regions before writing
 - clarify term "security register"
 - don't combine lock and erase functions anymore. there are now
   more difference than similarities.

Changes since v2:
 - fix 3/4 byte mode access
 - use spi_nor_erase_sector() by swapping the nor->erase_opcode
 - use more consistent wording regarding the security registers

Changes since v1:
- fixed kernel doc

Michael Walle (4):
  mtd: spi-nor: otp: fix access to security registers in 4 byte mode
  mtd: spi-nor: otp: use more consistent wording
  mtd: spi-nor: otp: return -EROFS if region is read-only
  mtd: spi-nor: otp: implement erase for Winbond and similar flashes

 drivers/mtd/spi-nor/core.c    |   2 +-
 drivers/mtd/spi-nor/core.h    |   4 +
 drivers/mtd/spi-nor/otp.c     | 147 +++++++++++++++++++++++++++++++---
 drivers/mtd/spi-nor/winbond.c |   1 +
 4 files changed, 143 insertions(+), 11 deletions(-)

-- 
2.20.1

[PATCH v4 0/4] mtd: spi-nor: otp: 4 byte mode fix and erase support

[Michael Walle]

This series is the follow up on the single patch
mtd: spi-nor: implement OTP erase for Winbond and similar flashes

Pratyush Yadav discovered a likely problem with bigger flashes, the address
to access the security registers is either 3 or 4 byte (at least for
winbond flashes).

Changes since v3:
 - new patch to check for read-only OTP regions before writing
 - clarify term "security register"
 - don't combine lock and erase functions anymore. there are now
   more difference than similarities.

Changes since v2:
 - fix 3/4 byte mode access
 - use spi_nor_erase_sector() by swapping the nor->erase_opcode
 - use more consistent wording regarding the security registers

Changes since v1:
- fixed kernel doc

Michael Walle (4):
  mtd: spi-nor: otp: fix access to security registers in 4 byte mode
  mtd: spi-nor: otp: use more consistent wording
  mtd: spi-nor: otp: return -EROFS if region is read-only
  mtd: spi-nor: otp: implement erase for Winbond and similar flashes

 drivers/mtd/spi-nor/core.c    |   2 +-
 drivers/mtd/spi-nor/core.h    |   4 +
 drivers/mtd/spi-nor/otp.c     | 147 +++++++++++++++++++++++++++++++---
 drivers/mtd/spi-nor/winbond.c |   1 +
 4 files changed, 143 insertions(+), 11 deletions(-)

-- 
2.20.1


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

[PATCH v4 1/4] mtd: spi-nor: otp: fix access to security registers in 4 byte mode

[Michael Walle]

The security registers either take a 3 byte or a 4 byte address offset,
depending on the address mode of the flash. Thus just leave the
nor->addr_width as is.

Fixes: cad3193fe9d1 ("mtd: spi-nor: implement OTP support for Winbond and similar flashes")
Signed-off-by: Michael Walle <michael@walle.cc>
Acked-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/mtd/spi-nor/otp.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
index 61036c716abb..91a4c510ed51 100644
--- a/drivers/mtd/spi-nor/otp.c
+++ b/drivers/mtd/spi-nor/otp.c
@@ -40,7 +40,6 @@ int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf)
 	rdesc = nor->dirmap.rdesc;
 
 	nor->read_opcode = SPINOR_OP_RSECR;
-	nor->addr_width = 3;
 	nor->read_dummy = 8;
 	nor->read_proto = SNOR_PROTO_1_1_1;
 	nor->dirmap.rdesc = NULL;
@@ -84,7 +83,6 @@ int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
 	wdesc = nor->dirmap.wdesc;
 
 	nor->program_opcode = SPINOR_OP_PSECR;
-	nor->addr_width = 3;
 	nor->write_proto = SNOR_PROTO_1_1_1;
 	nor->dirmap.wdesc = NULL;
 
-- 
2.20.1

[PATCH v4 1/4] mtd: spi-nor: otp: fix access to security registers in 4 byte mode

[Michael Walle]

The security registers either take a 3 byte or a 4 byte address offset,
depending on the address mode of the flash. Thus just leave the
nor->addr_width as is.

Fixes: cad3193fe9d1 ("mtd: spi-nor: implement OTP support for Winbond and similar flashes")
Signed-off-by: Michael Walle <michael@walle.cc>
Acked-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/mtd/spi-nor/otp.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
index 61036c716abb..91a4c510ed51 100644
--- a/drivers/mtd/spi-nor/otp.c
+++ b/drivers/mtd/spi-nor/otp.c
@@ -40,7 +40,6 @@ int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf)
 	rdesc = nor->dirmap.rdesc;
 
 	nor->read_opcode = SPINOR_OP_RSECR;
-	nor->addr_width = 3;
 	nor->read_dummy = 8;
 	nor->read_proto = SNOR_PROTO_1_1_1;
 	nor->dirmap.rdesc = NULL;
@@ -84,7 +83,6 @@ int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
 	wdesc = nor->dirmap.wdesc;
 
 	nor->program_opcode = SPINOR_OP_PSECR;
-	nor->addr_width = 3;
 	nor->write_proto = SNOR_PROTO_1_1_1;
 	nor->dirmap.wdesc = NULL;
 
-- 
2.20.1


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

Re: [PATCH v4 1/4] mtd: spi-nor: otp: fix access to security registers in 4 byte mode

[Tudor.Ambarus]

On 5/21/21 10:40 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> The security registers either take a 3 byte or a 4 byte address offset,
> depending on the address mode of the flash. Thus just leave the
> nor->addr_width as is.
> 
> Fixes: cad3193fe9d1 ("mtd: spi-nor: implement OTP support for Winbond and similar flashes")
> Signed-off-by: Michael Walle <michael@walle.cc>
> Acked-by: Pratyush Yadav <p.yadav@ti.com>

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> ---
>  drivers/mtd/spi-nor/otp.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> index 61036c716abb..91a4c510ed51 100644
> --- a/drivers/mtd/spi-nor/otp.c
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -40,7 +40,6 @@ int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf)
>         rdesc = nor->dirmap.rdesc;
> 
>         nor->read_opcode = SPINOR_OP_RSECR;
> -       nor->addr_width = 3;
>         nor->read_dummy = 8;
>         nor->read_proto = SNOR_PROTO_1_1_1;
>         nor->dirmap.rdesc = NULL;
> @@ -84,7 +83,6 @@ int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
>         wdesc = nor->dirmap.wdesc;
> 
>         nor->program_opcode = SPINOR_OP_PSECR;
> -       nor->addr_width = 3;
>         nor->write_proto = SNOR_PROTO_1_1_1;
>         nor->dirmap.wdesc = NULL;
> 
> --
> 2.20.1
> 

Re: [PATCH v4 1/4] mtd: spi-nor: otp: fix access to security registers in 4 byte mode

[Tudor.Ambarus]

On 5/21/21 10:40 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> The security registers either take a 3 byte or a 4 byte address offset,
> depending on the address mode of the flash. Thus just leave the
> nor->addr_width as is.
> 
> Fixes: cad3193fe9d1 ("mtd: spi-nor: implement OTP support for Winbond and similar flashes")
> Signed-off-by: Michael Walle <michael@walle.cc>
> Acked-by: Pratyush Yadav <p.yadav@ti.com>

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> ---
>  drivers/mtd/spi-nor/otp.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> index 61036c716abb..91a4c510ed51 100644
> --- a/drivers/mtd/spi-nor/otp.c
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -40,7 +40,6 @@ int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf)
>         rdesc = nor->dirmap.rdesc;
> 
>         nor->read_opcode = SPINOR_OP_RSECR;
> -       nor->addr_width = 3;
>         nor->read_dummy = 8;
>         nor->read_proto = SNOR_PROTO_1_1_1;
>         nor->dirmap.rdesc = NULL;
> @@ -84,7 +83,6 @@ int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
>         wdesc = nor->dirmap.wdesc;
> 
>         nor->program_opcode = SPINOR_OP_PSECR;
> -       nor->addr_width = 3;
>         nor->write_proto = SNOR_PROTO_1_1_1;
>         nor->dirmap.wdesc = NULL;
> 
> --
> 2.20.1
> 

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

[PATCH v4 2/4] mtd: spi-nor: otp: use more consistent wording

[Michael Walle]

Use the wording as used in the datasheet to describe the access methods
of the security registers (aka OTP storage). This will also match the
function names.

Signed-off-by: Michael Walle <michael@walle.cc>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/mtd/spi-nor/otp.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
index 91a4c510ed51..3898ed67ba1c 100644
--- a/drivers/mtd/spi-nor/otp.c
+++ b/drivers/mtd/spi-nor/otp.c
@@ -15,14 +15,21 @@
 #define spi_nor_otp_n_regions(nor) ((nor)->params->otp.org->n_regions)
 
 /**
- * spi_nor_otp_read_secr() - read OTP data
+ * spi_nor_otp_read_secr() - read security register
  * @nor:	pointer to 'struct spi_nor'
  * @addr:       offset to read from
  * @len:        number of bytes to read
  * @buf:        pointer to dst buffer
  *
- * Read OTP data from one region by using the SPINOR_OP_RSECR commands. This
- * method is used on GigaDevice and Winbond flashes.
+ * Read a security register by using the SPINOR_OP_RSECR commands.
+ *
+ * In Winbond/GigaDevice datasheets the term "security register" stands for
+ * an one-time-programmable memory area, consisting of multiple bytes (usually
+ * 256). Thus one "security register" maps to one OTP region.
+ *
+ * This method is used on GigaDevice and Winbond flashes.
+ *
+ * Please note, the read must not span multiple registers.
  *
  * Return: number of bytes read successfully, -errno otherwise
  */
@@ -56,16 +63,20 @@ int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf)
 }
 
 /**
- * spi_nor_otp_write_secr() - write OTP data
+ * spi_nor_otp_write_secr() - write security register
  * @nor:        pointer to 'struct spi_nor'
  * @addr:       offset to write to
  * @len:        number of bytes to write
  * @buf:        pointer to src buffer
  *
- * Write OTP data to one region by using the SPINOR_OP_PSECR commands. This
- * method is used on GigaDevice and Winbond flashes.
+ * Write a security register by using the SPINOR_OP_PSECR commands.
+ *
+ * For more information on the term "security register", see the documentation
+ * of spi_nor_otp_read_secr().
+ *
+ * This method is used on GigaDevice and Winbond flashes.
  *
- * Please note, the write must not span multiple OTP regions.
+ * Please note, the write must not span multiple registers.
  *
  * Return: number of bytes written successfully, -errno otherwise
  */
@@ -88,7 +99,7 @@ int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
 
 	/*
 	 * We only support a write to one single page. For now all winbond
-	 * flashes only have one page per OTP region.
+	 * flashes only have one page per security register.
 	 */
 	ret = spi_nor_write_enable(nor);
 	if (ret)
-- 
2.20.1

[PATCH v4 2/4] mtd: spi-nor: otp: use more consistent wording

[Michael Walle]

Use the wording as used in the datasheet to describe the access methods
of the security registers (aka OTP storage). This will also match the
function names.

Signed-off-by: Michael Walle <michael@walle.cc>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/mtd/spi-nor/otp.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
index 91a4c510ed51..3898ed67ba1c 100644
--- a/drivers/mtd/spi-nor/otp.c
+++ b/drivers/mtd/spi-nor/otp.c
@@ -15,14 +15,21 @@
 #define spi_nor_otp_n_regions(nor) ((nor)->params->otp.org->n_regions)
 
 /**
- * spi_nor_otp_read_secr() - read OTP data
+ * spi_nor_otp_read_secr() - read security register
  * @nor:	pointer to 'struct spi_nor'
  * @addr:       offset to read from
  * @len:        number of bytes to read
  * @buf:        pointer to dst buffer
  *
- * Read OTP data from one region by using the SPINOR_OP_RSECR commands. This
- * method is used on GigaDevice and Winbond flashes.
+ * Read a security register by using the SPINOR_OP_RSECR commands.
+ *
+ * In Winbond/GigaDevice datasheets the term "security register" stands for
+ * an one-time-programmable memory area, consisting of multiple bytes (usually
+ * 256). Thus one "security register" maps to one OTP region.
+ *
+ * This method is used on GigaDevice and Winbond flashes.
+ *
+ * Please note, the read must not span multiple registers.
  *
  * Return: number of bytes read successfully, -errno otherwise
  */
@@ -56,16 +63,20 @@ int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf)
 }
 
 /**
- * spi_nor_otp_write_secr() - write OTP data
+ * spi_nor_otp_write_secr() - write security register
  * @nor:        pointer to 'struct spi_nor'
  * @addr:       offset to write to
  * @len:        number of bytes to write
  * @buf:        pointer to src buffer
  *
- * Write OTP data to one region by using the SPINOR_OP_PSECR commands. This
- * method is used on GigaDevice and Winbond flashes.
+ * Write a security register by using the SPINOR_OP_PSECR commands.
+ *
+ * For more information on the term "security register", see the documentation
+ * of spi_nor_otp_read_secr().
+ *
+ * This method is used on GigaDevice and Winbond flashes.
  *
- * Please note, the write must not span multiple OTP regions.
+ * Please note, the write must not span multiple registers.
  *
  * Return: number of bytes written successfully, -errno otherwise
  */
@@ -88,7 +99,7 @@ int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
 
 	/*
 	 * We only support a write to one single page. For now all winbond
-	 * flashes only have one page per OTP region.
+	 * flashes only have one page per security register.
 	 */
 	ret = spi_nor_write_enable(nor);
 	if (ret)
-- 
2.20.1


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

Re: [PATCH v4 2/4] mtd: spi-nor: otp: use more consistent wording

[Tudor.Ambarus]

On 5/21/21 10:40 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Use the wording as used in the datasheet to describe the access methods
> of the security registers (aka OTP storage). This will also match the
> function names.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> ---
>  drivers/mtd/spi-nor/otp.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> index 91a4c510ed51..3898ed67ba1c 100644
> --- a/drivers/mtd/spi-nor/otp.c
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -15,14 +15,21 @@
>  #define spi_nor_otp_n_regions(nor) ((nor)->params->otp.org->n_regions)
> 
>  /**
> - * spi_nor_otp_read_secr() - read OTP data
> + * spi_nor_otp_read_secr() - read security register
>   * @nor:       pointer to 'struct spi_nor'
>   * @addr:       offset to read from
>   * @len:        number of bytes to read
>   * @buf:        pointer to dst buffer
>   *
> - * Read OTP data from one region by using the SPINOR_OP_RSECR commands. This
> - * method is used on GigaDevice and Winbond flashes.
> + * Read a security register by using the SPINOR_OP_RSECR commands.
> + *
> + * In Winbond/GigaDevice datasheets the term "security register" stands for
> + * an one-time-programmable memory area, consisting of multiple bytes (usually
> + * 256). Thus one "security register" maps to one OTP region.
> + *
> + * This method is used on GigaDevice and Winbond flashes.
> + *
> + * Please note, the read must not span multiple registers.
>   *
>   * Return: number of bytes read successfully, -errno otherwise
>   */
> @@ -56,16 +63,20 @@ int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf)
>  }
> 
>  /**
> - * spi_nor_otp_write_secr() - write OTP data
> + * spi_nor_otp_write_secr() - write security register
>   * @nor:        pointer to 'struct spi_nor'
>   * @addr:       offset to write to
>   * @len:        number of bytes to write
>   * @buf:        pointer to src buffer
>   *
> - * Write OTP data to one region by using the SPINOR_OP_PSECR commands. This
> - * method is used on GigaDevice and Winbond flashes.
> + * Write a security register by using the SPINOR_OP_PSECR commands.
> + *
> + * For more information on the term "security register", see the documentation
> + * of spi_nor_otp_read_secr().
> + *
> + * This method is used on GigaDevice and Winbond flashes.
>   *
> - * Please note, the write must not span multiple OTP regions.
> + * Please note, the write must not span multiple registers.
>   *
>   * Return: number of bytes written successfully, -errno otherwise
>   */
> @@ -88,7 +99,7 @@ int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
> 
>         /*
>          * We only support a write to one single page. For now all winbond
> -        * flashes only have one page per OTP region.
> +        * flashes only have one page per security register.
>          */
>         ret = spi_nor_write_enable(nor);
>         if (ret)
> --
> 2.20.1
> 

Re: [PATCH v4 2/4] mtd: spi-nor: otp: use more consistent wording

[Tudor.Ambarus]

On 5/21/21 10:40 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Use the wording as used in the datasheet to describe the access methods
> of the security registers (aka OTP storage). This will also match the
> function names.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> ---
>  drivers/mtd/spi-nor/otp.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> index 91a4c510ed51..3898ed67ba1c 100644
> --- a/drivers/mtd/spi-nor/otp.c
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -15,14 +15,21 @@
>  #define spi_nor_otp_n_regions(nor) ((nor)->params->otp.org->n_regions)
> 
>  /**
> - * spi_nor_otp_read_secr() - read OTP data
> + * spi_nor_otp_read_secr() - read security register
>   * @nor:       pointer to 'struct spi_nor'
>   * @addr:       offset to read from
>   * @len:        number of bytes to read
>   * @buf:        pointer to dst buffer
>   *
> - * Read OTP data from one region by using the SPINOR_OP_RSECR commands. This
> - * method is used on GigaDevice and Winbond flashes.
> + * Read a security register by using the SPINOR_OP_RSECR commands.
> + *
> + * In Winbond/GigaDevice datasheets the term "security register" stands for
> + * an one-time-programmable memory area, consisting of multiple bytes (usually
> + * 256). Thus one "security register" maps to one OTP region.
> + *
> + * This method is used on GigaDevice and Winbond flashes.
> + *
> + * Please note, the read must not span multiple registers.
>   *
>   * Return: number of bytes read successfully, -errno otherwise
>   */
> @@ -56,16 +63,20 @@ int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf)
>  }
> 
>  /**
> - * spi_nor_otp_write_secr() - write OTP data
> + * spi_nor_otp_write_secr() - write security register
>   * @nor:        pointer to 'struct spi_nor'
>   * @addr:       offset to write to
>   * @len:        number of bytes to write
>   * @buf:        pointer to src buffer
>   *
> - * Write OTP data to one region by using the SPINOR_OP_PSECR commands. This
> - * method is used on GigaDevice and Winbond flashes.
> + * Write a security register by using the SPINOR_OP_PSECR commands.
> + *
> + * For more information on the term "security register", see the documentation
> + * of spi_nor_otp_read_secr().
> + *
> + * This method is used on GigaDevice and Winbond flashes.
>   *
> - * Please note, the write must not span multiple OTP regions.
> + * Please note, the write must not span multiple registers.
>   *
>   * Return: number of bytes written successfully, -errno otherwise
>   */
> @@ -88,7 +99,7 @@ int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
> 
>         /*
>          * We only support a write to one single page. For now all winbond
> -        * flashes only have one page per OTP region.
> +        * flashes only have one page per security register.
>          */
>         ret = spi_nor_write_enable(nor);
>         if (ret)
> --
> 2.20.1
> 

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

[PATCH v4 3/4] mtd: spi-nor: otp: return -EROFS if region is read-only

[Michael Walle]

SPI NOR flashes will just ignore program commands if the OTP region is
locked. Thus, a user might not notice that the intended write didn't end
up in the flash. Return -EROFS to the user in this case. From what I can
tell, chips/cfi_cmdset_0001.c also return this error code.

One could optimize spi_nor_mtd_otp_range_is_locked() to read the status
register only once and not for every OTP region, but for that we would
need some more invasive changes. Given that this is
one-time-programmable memory and the normal access mode is reading, we
just live with the small overhead.

Fixes: 069089acf88b ("mtd: spi-nor: add OTP support")
Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/otp.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
index 3898ed67ba1c..b87f96593c13 100644
--- a/drivers/mtd/spi-nor/otp.c
+++ b/drivers/mtd/spi-nor/otp.c
@@ -249,6 +249,31 @@ static int spi_nor_mtd_otp_info(struct mtd_info *mtd, size_t len,
 	return ret;
 }
 
+static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor, loff_t ofs,
+					   size_t len)
+{
+	const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
+	unsigned int region;
+	int locked;
+
+	if (!len)
+		return 0;
+
+	/*
+	 * If any of the affected OTP regions are locked the entire range is
+	 * considered locked.
+	 */
+	for (region = spi_nor_otp_offset_to_region(nor, ofs);
+	     region <= spi_nor_otp_offset_to_region(nor, ofs + len - 1);
+	     region++) {
+		locked = ops->is_locked(nor, region);
+		if (locked)
+			return locked;
+	}
+
+	return 0;
+}
+
 static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
 				      size_t total_len, size_t *retlen,
 				      const u8 *buf, bool is_write)
@@ -271,6 +296,16 @@ static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
 	/* don't access beyond the end */
 	total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - ofs);
 
+	if (is_write) {
+		ret = spi_nor_mtd_otp_range_is_locked(nor, ofs, total_len);
+		if (ret < 0) {
+			goto out;
+		} else if (ret) {
+			ret = -EROFS;
+			goto out;
+		}
+	}
+
 	*retlen = 0;
 	while (total_len) {
 		/*
-- 
2.20.1

[PATCH v4 3/4] mtd: spi-nor: otp: return -EROFS if region is read-only

[Michael Walle]

SPI NOR flashes will just ignore program commands if the OTP region is
locked. Thus, a user might not notice that the intended write didn't end
up in the flash. Return -EROFS to the user in this case. From what I can
tell, chips/cfi_cmdset_0001.c also return this error code.

One could optimize spi_nor_mtd_otp_range_is_locked() to read the status
register only once and not for every OTP region, but for that we would
need some more invasive changes. Given that this is
one-time-programmable memory and the normal access mode is reading, we
just live with the small overhead.

Fixes: 069089acf88b ("mtd: spi-nor: add OTP support")
Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/otp.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
index 3898ed67ba1c..b87f96593c13 100644
--- a/drivers/mtd/spi-nor/otp.c
+++ b/drivers/mtd/spi-nor/otp.c
@@ -249,6 +249,31 @@ static int spi_nor_mtd_otp_info(struct mtd_info *mtd, size_t len,
 	return ret;
 }
 
+static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor, loff_t ofs,
+					   size_t len)
+{
+	const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
+	unsigned int region;
+	int locked;
+
+	if (!len)
+		return 0;
+
+	/*
+	 * If any of the affected OTP regions are locked the entire range is
+	 * considered locked.
+	 */
+	for (region = spi_nor_otp_offset_to_region(nor, ofs);
+	     region <= spi_nor_otp_offset_to_region(nor, ofs + len - 1);
+	     region++) {
+		locked = ops->is_locked(nor, region);
+		if (locked)
+			return locked;
+	}
+
+	return 0;
+}
+
 static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
 				      size_t total_len, size_t *retlen,
 				      const u8 *buf, bool is_write)
@@ -271,6 +296,16 @@ static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
 	/* don't access beyond the end */
 	total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - ofs);
 
+	if (is_write) {
+		ret = spi_nor_mtd_otp_range_is_locked(nor, ofs, total_len);
+		if (ret < 0) {
+			goto out;
+		} else if (ret) {
+			ret = -EROFS;
+			goto out;
+		}
+	}
+
 	*retlen = 0;
 	while (total_len) {
 		/*
-- 
2.20.1


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

Re: [PATCH v4 3/4] mtd: spi-nor: otp: return -EROFS if region is read-only

[Pratyush Yadav]

On 21/05/21 09:40PM, Michael Walle wrote:
> SPI NOR flashes will just ignore program commands if the OTP region is
> locked. Thus, a user might not notice that the intended write didn't end
> up in the flash. Return -EROFS to the user in this case. From what I can
> tell, chips/cfi_cmdset_0001.c also return this error code.
> 
> One could optimize spi_nor_mtd_otp_range_is_locked() to read the status
> register only once and not for every OTP region, but for that we would
> need some more invasive changes. Given that this is
> one-time-programmable memory and the normal access mode is reading, we
> just live with the small overhead.

Ok.

> 
> Fixes: 069089acf88b ("mtd: spi-nor: add OTP support")
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/otp.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> index 3898ed67ba1c..b87f96593c13 100644
> --- a/drivers/mtd/spi-nor/otp.c
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -249,6 +249,31 @@ static int spi_nor_mtd_otp_info(struct mtd_info *mtd, size_t len,
>  	return ret;
>  }
>  
> +static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor, loff_t ofs,
> +					   size_t len)
> +{
> +	const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
> +	unsigned int region;
> +	int locked;
> +
> +	if (!len)
> +		return 0;

I was inclined to say that the loop conditional below would take care of 
this but it can cause an underflow when ofs and len are both 0.

> +
> +	/*
> +	 * If any of the affected OTP regions are locked the entire range is
> +	 * considered locked.
> +	 */
> +	for (region = spi_nor_otp_offset_to_region(nor, ofs);
> +	     region <= spi_nor_otp_offset_to_region(nor, ofs + len - 1);
> +	     region++) {
> +		locked = ops->is_locked(nor, region);
> +		if (locked)
> +			return locked;
> +	}

Ok.

> +
> +	return 0;
> +}
> +
>  static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
>  				      size_t total_len, size_t *retlen,
>  				      const u8 *buf, bool is_write)
> @@ -271,6 +296,16 @@ static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
>  	/* don't access beyond the end */
>  	total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - ofs);
>  
> +	if (is_write) {
> +		ret = spi_nor_mtd_otp_range_is_locked(nor, ofs, total_len);
> +		if (ret < 0) {
> +			goto out;
> +		} else if (ret) {
> +			ret = -EROFS;

I wonder if we should have a dev_info() or dev_err() here. I think this 
warrants a dev_dbg() at least.

> +			goto out;
> +		}

So it returns -errno when the check for is_locked() fails and 1 or 0 
when it is locked or not. Ok.

It would be nice if you add a dev_dbg or dev_err() or dev_info() above. 
Nonetheless,

Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

> +	}
> +
>  	*retlen = 0;
>  	while (total_len) {
>  		/*

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v4 3/4] mtd: spi-nor: otp: return -EROFS if region is read-only

[Pratyush Yadav]

On 21/05/21 09:40PM, Michael Walle wrote:
> SPI NOR flashes will just ignore program commands if the OTP region is
> locked. Thus, a user might not notice that the intended write didn't end
> up in the flash. Return -EROFS to the user in this case. From what I can
> tell, chips/cfi_cmdset_0001.c also return this error code.
> 
> One could optimize spi_nor_mtd_otp_range_is_locked() to read the status
> register only once and not for every OTP region, but for that we would
> need some more invasive changes. Given that this is
> one-time-programmable memory and the normal access mode is reading, we
> just live with the small overhead.

Ok.

> 
> Fixes: 069089acf88b ("mtd: spi-nor: add OTP support")
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/otp.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> index 3898ed67ba1c..b87f96593c13 100644
> --- a/drivers/mtd/spi-nor/otp.c
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -249,6 +249,31 @@ static int spi_nor_mtd_otp_info(struct mtd_info *mtd, size_t len,
>  	return ret;
>  }
>  
> +static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor, loff_t ofs,
> +					   size_t len)
> +{
> +	const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
> +	unsigned int region;
> +	int locked;
> +
> +	if (!len)
> +		return 0;

I was inclined to say that the loop conditional below would take care of 
this but it can cause an underflow when ofs and len are both 0.

> +
> +	/*
> +	 * If any of the affected OTP regions are locked the entire range is
> +	 * considered locked.
> +	 */
> +	for (region = spi_nor_otp_offset_to_region(nor, ofs);
> +	     region <= spi_nor_otp_offset_to_region(nor, ofs + len - 1);
> +	     region++) {
> +		locked = ops->is_locked(nor, region);
> +		if (locked)
> +			return locked;
> +	}

Ok.

> +
> +	return 0;
> +}
> +
>  static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
>  				      size_t total_len, size_t *retlen,
>  				      const u8 *buf, bool is_write)
> @@ -271,6 +296,16 @@ static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
>  	/* don't access beyond the end */
>  	total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - ofs);
>  
> +	if (is_write) {
> +		ret = spi_nor_mtd_otp_range_is_locked(nor, ofs, total_len);
> +		if (ret < 0) {
> +			goto out;
> +		} else if (ret) {
> +			ret = -EROFS;

I wonder if we should have a dev_info() or dev_err() here. I think this 
warrants a dev_dbg() at least.

> +			goto out;
> +		}

So it returns -errno when the check for is_locked() fails and 1 or 0 
when it is locked or not. Ok.

It would be nice if you add a dev_dbg or dev_err() or dev_info() above. 
Nonetheless,

Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

> +	}
> +
>  	*retlen = 0;
>  	while (total_len) {
>  		/*

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

Re: [PATCH v4 3/4] mtd: spi-nor: otp: return -EROFS if region is read-only

[Michael Walle]

Am 2021-05-25 21:33, schrieb Pratyush Yadav:
> On 21/05/21 09:40PM, Michael Walle wrote:
>> SPI NOR flashes will just ignore program commands if the OTP region is
>> locked. Thus, a user might not notice that the intended write didn't 
>> end
>> up in the flash. Return -EROFS to the user in this case. From what I 
>> can
>> tell, chips/cfi_cmdset_0001.c also return this error code.
>> 
>> One could optimize spi_nor_mtd_otp_range_is_locked() to read the 
>> status
>> register only once and not for every OTP region, but for that we would
>> need some more invasive changes. Given that this is
>> one-time-programmable memory and the normal access mode is reading, we
>> just live with the small overhead.
> 
> Ok.
> 
>> 
>> Fixes: 069089acf88b ("mtd: spi-nor: add OTP support")
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  drivers/mtd/spi-nor/otp.c | 35 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>> 
>> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
>> index 3898ed67ba1c..b87f96593c13 100644
>> --- a/drivers/mtd/spi-nor/otp.c
>> +++ b/drivers/mtd/spi-nor/otp.c
>> @@ -249,6 +249,31 @@ static int spi_nor_mtd_otp_info(struct mtd_info 
>> *mtd, size_t len,
>>  	return ret;
>>  }
>> 
>> +static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor, 
>> loff_t ofs,
>> +					   size_t len)
>> +{
>> +	const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
>> +	unsigned int region;
>> +	int locked;
>> +
>> +	if (!len)
>> +		return 0;
> 
> I was inclined to say that the loop conditional below would take care 
> of
> this but it can cause an underflow when ofs and len are both 0.

Correct. I didn't want to put an extra check to the caller, because
as you noticed, it is checked by the loop there later.

>> +
>> +	/*
>> +	 * If any of the affected OTP regions are locked the entire range is
>> +	 * considered locked.
>> +	 */
>> +	for (region = spi_nor_otp_offset_to_region(nor, ofs);
>> +	     region <= spi_nor_otp_offset_to_region(nor, ofs + len - 1);
>> +	     region++) {
>> +		locked = ops->is_locked(nor, region);
>> +		if (locked)
>> +			return locked;
>> +	}
> 
> Ok.

Btw I didn't know if I should put a comment here that this if () handles
both locked state and errors. But it seems you've already found out by
looking at the caller ;) I'm not sure if this is obvious, though.

>> +
>> +	return 0;
>> +}
>> +
>>  static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t 
>> ofs,
>>  				      size_t total_len, size_t *retlen,
>>  				      const u8 *buf, bool is_write)
>> @@ -271,6 +296,16 @@ static int spi_nor_mtd_otp_read_write(struct 
>> mtd_info *mtd, loff_t ofs,
>>  	/* don't access beyond the end */
>>  	total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - ofs);
>> 
>> +	if (is_write) {
>> +		ret = spi_nor_mtd_otp_range_is_locked(nor, ofs, total_len);
>> +		if (ret < 0) {
>> +			goto out;
>> +		} else if (ret) {
>> +			ret = -EROFS;
> 
> I wonder if we should have a dev_info() or dev_err() here. I think this
> warrants a dev_dbg() at least.

Are you sure? Reporting something to the user via an error code is
enough IMHO. I wouldn't want my syslog to be cluttered with messages
I already know. I mean the program tell me "hey, you aren't allowed
to write there". Why would the kernel still need to tell me that again?
Without any connection to the caller, I don't get much out of the kernel
message by looking at it alone, just that someone tried to write there.

So definetly no dev_info() or dev_err(). But IMHO no dev_dbg() either.
Tudor, Vingesh, any opinions?


>> +			goto out;
>> +		}
> 
> So it returns -errno when the check for is_locked() fails and 1 or 0
> when it is locked or not. Ok.
> 
> It would be nice if you add a dev_dbg or dev_err() or dev_info() above.
> Nonetheless,
> 
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

Thanks for reviewing!

-michael

Re: [PATCH v4 3/4] mtd: spi-nor: otp: return -EROFS if region is read-only

[Michael Walle]

Am 2021-05-25 21:33, schrieb Pratyush Yadav:
> On 21/05/21 09:40PM, Michael Walle wrote:
>> SPI NOR flashes will just ignore program commands if the OTP region is
>> locked. Thus, a user might not notice that the intended write didn't 
>> end
>> up in the flash. Return -EROFS to the user in this case. From what I 
>> can
>> tell, chips/cfi_cmdset_0001.c also return this error code.
>> 
>> One could optimize spi_nor_mtd_otp_range_is_locked() to read the 
>> status
>> register only once and not for every OTP region, but for that we would
>> need some more invasive changes. Given that this is
>> one-time-programmable memory and the normal access mode is reading, we
>> just live with the small overhead.
> 
> Ok.
> 
>> 
>> Fixes: 069089acf88b ("mtd: spi-nor: add OTP support")
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  drivers/mtd/spi-nor/otp.c | 35 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>> 
>> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
>> index 3898ed67ba1c..b87f96593c13 100644
>> --- a/drivers/mtd/spi-nor/otp.c
>> +++ b/drivers/mtd/spi-nor/otp.c
>> @@ -249,6 +249,31 @@ static int spi_nor_mtd_otp_info(struct mtd_info 
>> *mtd, size_t len,
>>  	return ret;
>>  }
>> 
>> +static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor, 
>> loff_t ofs,
>> +					   size_t len)
>> +{
>> +	const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
>> +	unsigned int region;
>> +	int locked;
>> +
>> +	if (!len)
>> +		return 0;
> 
> I was inclined to say that the loop conditional below would take care 
> of
> this but it can cause an underflow when ofs and len are both 0.

Correct. I didn't want to put an extra check to the caller, because
as you noticed, it is checked by the loop there later.

>> +
>> +	/*
>> +	 * If any of the affected OTP regions are locked the entire range is
>> +	 * considered locked.
>> +	 */
>> +	for (region = spi_nor_otp_offset_to_region(nor, ofs);
>> +	     region <= spi_nor_otp_offset_to_region(nor, ofs + len - 1);
>> +	     region++) {
>> +		locked = ops->is_locked(nor, region);
>> +		if (locked)
>> +			return locked;
>> +	}
> 
> Ok.

Btw I didn't know if I should put a comment here that this if () handles
both locked state and errors. But it seems you've already found out by
looking at the caller ;) I'm not sure if this is obvious, though.

>> +
>> +	return 0;
>> +}
>> +
>>  static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t 
>> ofs,
>>  				      size_t total_len, size_t *retlen,
>>  				      const u8 *buf, bool is_write)
>> @@ -271,6 +296,16 @@ static int spi_nor_mtd_otp_read_write(struct 
>> mtd_info *mtd, loff_t ofs,
>>  	/* don't access beyond the end */
>>  	total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - ofs);
>> 
>> +	if (is_write) {
>> +		ret = spi_nor_mtd_otp_range_is_locked(nor, ofs, total_len);
>> +		if (ret < 0) {
>> +			goto out;
>> +		} else if (ret) {
>> +			ret = -EROFS;
> 
> I wonder if we should have a dev_info() or dev_err() here. I think this
> warrants a dev_dbg() at least.

Are you sure? Reporting something to the user via an error code is
enough IMHO. I wouldn't want my syslog to be cluttered with messages
I already know. I mean the program tell me "hey, you aren't allowed
to write there". Why would the kernel still need to tell me that again?
Without any connection to the caller, I don't get much out of the kernel
message by looking at it alone, just that someone tried to write there.

So definetly no dev_info() or dev_err(). But IMHO no dev_dbg() either.
Tudor, Vingesh, any opinions?


>> +			goto out;
>> +		}
> 
> So it returns -errno when the check for is_locked() fails and 1 or 0
> when it is locked or not. Ok.
> 
> It would be nice if you add a dev_dbg or dev_err() or dev_info() above.
> Nonetheless,
> 
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

Thanks for reviewing!

-michael

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

Re: [PATCH v4 3/4] mtd: spi-nor: otp: return -EROFS if region is read-only

[Pratyush Yadav]

On 26/05/21 12:41PM, Michael Walle wrote:
> Am 2021-05-25 21:33, schrieb Pratyush Yadav:
> > On 21/05/21 09:40PM, Michael Walle wrote:
> > > SPI NOR flashes will just ignore program commands if the OTP region is
> > > locked. Thus, a user might not notice that the intended write didn't
> > > end
> > > up in the flash. Return -EROFS to the user in this case. From what I
> > > can
> > > tell, chips/cfi_cmdset_0001.c also return this error code.
> > > 
> > > One could optimize spi_nor_mtd_otp_range_is_locked() to read the
> > > status
> > > register only once and not for every OTP region, but for that we would
> > > need some more invasive changes. Given that this is
> > > one-time-programmable memory and the normal access mode is reading, we
> > > just live with the small overhead.
> > 
> > Ok.
> > 
> > > 
> > > Fixes: 069089acf88b ("mtd: spi-nor: add OTP support")
> > > Signed-off-by: Michael Walle <michael@walle.cc>
> > > ---
> > >  drivers/mtd/spi-nor/otp.c | 35 +++++++++++++++++++++++++++++++++++
> > >  1 file changed, 35 insertions(+)
> > > 
> > > diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> > > index 3898ed67ba1c..b87f96593c13 100644
> > > --- a/drivers/mtd/spi-nor/otp.c
> > > +++ b/drivers/mtd/spi-nor/otp.c
> > > @@ -249,6 +249,31 @@ static int spi_nor_mtd_otp_info(struct mtd_info
> > > *mtd, size_t len,
> > >  	return ret;
> > >  }
> > > 
> > > +static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor,
> > > loff_t ofs,
> > > +					   size_t len)
> > > +{
> > > +	const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
> > > +	unsigned int region;
> > > +	int locked;
> > > +
> > > +	if (!len)
> > > +		return 0;
> > 
> > I was inclined to say that the loop conditional below would take care of
> > this but it can cause an underflow when ofs and len are both 0.
> 
> Correct. I didn't want to put an extra check to the caller, because
> as you noticed, it is checked by the loop there later.
> 
> > > +
> > > +	/*
> > > +	 * If any of the affected OTP regions are locked the entire range is
> > > +	 * considered locked.
> > > +	 */
> > > +	for (region = spi_nor_otp_offset_to_region(nor, ofs);
> > > +	     region <= spi_nor_otp_offset_to_region(nor, ofs + len - 1);
> > > +	     region++) {
> > > +		locked = ops->is_locked(nor, region);
> > > +		if (locked)
> > > +			return locked;
> > > +	}
> > 
> > Ok.
> 
> Btw I didn't know if I should put a comment here that this if () handles
> both locked state and errors. But it seems you've already found out by
> looking at the caller ;) I'm not sure if this is obvious, though.

I didn't catch this on the first read. I only figured it out when I 
looked at the return check below. So it is certainly not obvious.

> 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t
> > > ofs,
> > >  				      size_t total_len, size_t *retlen,
> > >  				      const u8 *buf, bool is_write)
> > > @@ -271,6 +296,16 @@ static int spi_nor_mtd_otp_read_write(struct
> > > mtd_info *mtd, loff_t ofs,
> > >  	/* don't access beyond the end */
> > >  	total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - ofs);
> > > 
> > > +	if (is_write) {
> > > +		ret = spi_nor_mtd_otp_range_is_locked(nor, ofs, total_len);
> > > +		if (ret < 0) {
> > > +			goto out;
> > > +		} else if (ret) {
> > > +			ret = -EROFS;
> > 
> > I wonder if we should have a dev_info() or dev_err() here. I think this
> > warrants a dev_dbg() at least.
> 
> Are you sure? Reporting something to the user via an error code is
> enough IMHO. I wouldn't want my syslog to be cluttered with messages
> I already know. I mean the program tell me "hey, you aren't allowed
> to write there". Why would the kernel still need to tell me that again?
> Without any connection to the caller, I don't get much out of the kernel
> message by looking at it alone, just that someone tried to write there.
> 
> So definetly no dev_info() or dev_err(). But IMHO no dev_dbg() either.
> Tudor, Vingesh, any opinions?

Either is fine by me.

> 
> 
> > > +			goto out;
> > > +		}
> > 
> > So it returns -errno when the check for is_locked() fails and 1 or 0
> > when it is locked or not. Ok.
> > 
> > It would be nice if you add a dev_dbg or dev_err() or dev_info() above.
> > Nonetheless,
> > 
> > Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
> 
> Thanks for reviewing!
> 
> -michael

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v4 3/4] mtd: spi-nor: otp: return -EROFS if region is read-only

[Pratyush Yadav]

On 26/05/21 12:41PM, Michael Walle wrote:
> Am 2021-05-25 21:33, schrieb Pratyush Yadav:
> > On 21/05/21 09:40PM, Michael Walle wrote:
> > > SPI NOR flashes will just ignore program commands if the OTP region is
> > > locked. Thus, a user might not notice that the intended write didn't
> > > end
> > > up in the flash. Return -EROFS to the user in this case. From what I
> > > can
> > > tell, chips/cfi_cmdset_0001.c also return this error code.
> > > 
> > > One could optimize spi_nor_mtd_otp_range_is_locked() to read the
> > > status
> > > register only once and not for every OTP region, but for that we would
> > > need some more invasive changes. Given that this is
> > > one-time-programmable memory and the normal access mode is reading, we
> > > just live with the small overhead.
> > 
> > Ok.
> > 
> > > 
> > > Fixes: 069089acf88b ("mtd: spi-nor: add OTP support")
> > > Signed-off-by: Michael Walle <michael@walle.cc>
> > > ---
> > >  drivers/mtd/spi-nor/otp.c | 35 +++++++++++++++++++++++++++++++++++
> > >  1 file changed, 35 insertions(+)
> > > 
> > > diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> > > index 3898ed67ba1c..b87f96593c13 100644
> > > --- a/drivers/mtd/spi-nor/otp.c
> > > +++ b/drivers/mtd/spi-nor/otp.c
> > > @@ -249,6 +249,31 @@ static int spi_nor_mtd_otp_info(struct mtd_info
> > > *mtd, size_t len,
> > >  	return ret;
> > >  }
> > > 
> > > +static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor,
> > > loff_t ofs,
> > > +					   size_t len)
> > > +{
> > > +	const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
> > > +	unsigned int region;
> > > +	int locked;
> > > +
> > > +	if (!len)
> > > +		return 0;
> > 
> > I was inclined to say that the loop conditional below would take care of
> > this but it can cause an underflow when ofs and len are both 0.
> 
> Correct. I didn't want to put an extra check to the caller, because
> as you noticed, it is checked by the loop there later.
> 
> > > +
> > > +	/*
> > > +	 * If any of the affected OTP regions are locked the entire range is
> > > +	 * considered locked.
> > > +	 */
> > > +	for (region = spi_nor_otp_offset_to_region(nor, ofs);
> > > +	     region <= spi_nor_otp_offset_to_region(nor, ofs + len - 1);
> > > +	     region++) {
> > > +		locked = ops->is_locked(nor, region);
> > > +		if (locked)
> > > +			return locked;
> > > +	}
> > 
> > Ok.
> 
> Btw I didn't know if I should put a comment here that this if () handles
> both locked state and errors. But it seems you've already found out by
> looking at the caller ;) I'm not sure if this is obvious, though.

I didn't catch this on the first read. I only figured it out when I 
looked at the return check below. So it is certainly not obvious.

> 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t
> > > ofs,
> > >  				      size_t total_len, size_t *retlen,
> > >  				      const u8 *buf, bool is_write)
> > > @@ -271,6 +296,16 @@ static int spi_nor_mtd_otp_read_write(struct
> > > mtd_info *mtd, loff_t ofs,
> > >  	/* don't access beyond the end */
> > >  	total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - ofs);
> > > 
> > > +	if (is_write) {
> > > +		ret = spi_nor_mtd_otp_range_is_locked(nor, ofs, total_len);
> > > +		if (ret < 0) {
> > > +			goto out;
> > > +		} else if (ret) {
> > > +			ret = -EROFS;
> > 
> > I wonder if we should have a dev_info() or dev_err() here. I think this
> > warrants a dev_dbg() at least.
> 
> Are you sure? Reporting something to the user via an error code is
> enough IMHO. I wouldn't want my syslog to be cluttered with messages
> I already know. I mean the program tell me "hey, you aren't allowed
> to write there". Why would the kernel still need to tell me that again?
> Without any connection to the caller, I don't get much out of the kernel
> message by looking at it alone, just that someone tried to write there.
> 
> So definetly no dev_info() or dev_err(). But IMHO no dev_dbg() either.
> Tudor, Vingesh, any opinions?

Either is fine by me.

> 
> 
> > > +			goto out;
> > > +		}
> > 
> > So it returns -errno when the check for is_locked() fails and 1 or 0
> > when it is locked or not. Ok.
> > 
> > It would be nice if you add a dev_dbg or dev_err() or dev_info() above.
> > Nonetheless,
> > 
> > Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
> 
> Thanks for reviewing!
> 
> -michael

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

Re: [PATCH v4 3/4] mtd: spi-nor: otp: return -EROFS if region is read-only

[Tudor.Ambarus]

On 5/21/21 10:40 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> SPI NOR flashes will just ignore program commands if the OTP region is
> locked. Thus, a user might not notice that the intended write didn't end
> up in the flash. Return -EROFS to the user in this case. From what I can
> tell, chips/cfi_cmdset_0001.c also return this error code.
> 
> One could optimize spi_nor_mtd_otp_range_is_locked() to read the status
> register only once and not for every OTP region, but for that we would
> need some more invasive changes. Given that this is
> one-time-programmable memory and the normal access mode is reading, we
> just live with the small overhead.

:)

Shouldn't we change
struct spi_nor_otp_ops {                                                        
	...                                       
        int (*lock)(struct spi_nor *nor, unsigned int region);                  
        int (*is_locked)(struct spi_nor *nor, unsigned int region);             
}; 

to:
struct spi_nor_otp_ops {                                                        
	...                                       
        int (*lock)(struct spi_nor *nor, loff_t addr, size_t len);                  
        int (*is_locked)(struct spi_nor *nor, loff_t addr, size_t len);
}; 

instead?

> 
> Fixes: 069089acf88b ("mtd: spi-nor: add OTP support")
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/otp.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> index 3898ed67ba1c..b87f96593c13 100644
> --- a/drivers/mtd/spi-nor/otp.c
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -249,6 +249,31 @@ static int spi_nor_mtd_otp_info(struct mtd_info *mtd, size_t len,
>         return ret;
>  }
> 
> +static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor, loff_t ofs,
> +                                          size_t len)
> +{
> +       const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
> +       unsigned int region;
> +       int locked;
> +
> +       if (!len)
> +               return 0;
> +
> +       /*
> +        * If any of the affected OTP regions are locked the entire range is
> +        * considered locked.
> +        */
> +       for (region = spi_nor_otp_offset_to_region(nor, ofs);
> +            region <= spi_nor_otp_offset_to_region(nor, ofs + len - 1);
> +            region++) {
> +               locked = ops->is_locked(nor, region);
> +               if (locked)
> +                       return locked;
> +       }
> +
> +       return 0;
> +}
> +
>  static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
>                                       size_t total_len, size_t *retlen,
>                                       const u8 *buf, bool is_write)
> @@ -271,6 +296,16 @@ static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
>         /* don't access beyond the end */
>         total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - ofs);
> 
> +       if (is_write) {
> +               ret = spi_nor_mtd_otp_range_is_locked(nor, ofs, total_len);
> +               if (ret < 0) {
> +                       goto out;
> +               } else if (ret) {
> +                       ret = -EROFS;
> +                       goto out;
> +               }
> +       }
> +
>         *retlen = 0;
>         while (total_len) {
>                 /*
> --
> 2.20.1
> 

Re: [PATCH v4 3/4] mtd: spi-nor: otp: return -EROFS if region is read-only

[Tudor.Ambarus]

On 5/21/21 10:40 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> SPI NOR flashes will just ignore program commands if the OTP region is
> locked. Thus, a user might not notice that the intended write didn't end
> up in the flash. Return -EROFS to the user in this case. From what I can
> tell, chips/cfi_cmdset_0001.c also return this error code.
> 
> One could optimize spi_nor_mtd_otp_range_is_locked() to read the status
> register only once and not for every OTP region, but for that we would
> need some more invasive changes. Given that this is
> one-time-programmable memory and the normal access mode is reading, we
> just live with the small overhead.

:)

Shouldn't we change
struct spi_nor_otp_ops {                                                        
	...                                       
        int (*lock)(struct spi_nor *nor, unsigned int region);                  
        int (*is_locked)(struct spi_nor *nor, unsigned int region);             
}; 

to:
struct spi_nor_otp_ops {                                                        
	...                                       
        int (*lock)(struct spi_nor *nor, loff_t addr, size_t len);                  
        int (*is_locked)(struct spi_nor *nor, loff_t addr, size_t len);
}; 

instead?

> 
> Fixes: 069089acf88b ("mtd: spi-nor: add OTP support")
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/otp.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> index 3898ed67ba1c..b87f96593c13 100644
> --- a/drivers/mtd/spi-nor/otp.c
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -249,6 +249,31 @@ static int spi_nor_mtd_otp_info(struct mtd_info *mtd, size_t len,
>         return ret;
>  }
> 
> +static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor, loff_t ofs,
> +                                          size_t len)
> +{
> +       const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
> +       unsigned int region;
> +       int locked;
> +
> +       if (!len)
> +               return 0;
> +
> +       /*
> +        * If any of the affected OTP regions are locked the entire range is
> +        * considered locked.
> +        */
> +       for (region = spi_nor_otp_offset_to_region(nor, ofs);
> +            region <= spi_nor_otp_offset_to_region(nor, ofs + len - 1);
> +            region++) {
> +               locked = ops->is_locked(nor, region);
> +               if (locked)
> +                       return locked;
> +       }
> +
> +       return 0;
> +}
> +
>  static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
>                                       size_t total_len, size_t *retlen,
>                                       const u8 *buf, bool is_write)
> @@ -271,6 +296,16 @@ static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
>         /* don't access beyond the end */
>         total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - ofs);
> 
> +       if (is_write) {
> +               ret = spi_nor_mtd_otp_range_is_locked(nor, ofs, total_len);
> +               if (ret < 0) {
> +                       goto out;
> +               } else if (ret) {
> +                       ret = -EROFS;
> +                       goto out;
> +               }
> +       }
> +
>         *retlen = 0;
>         while (total_len) {
>                 /*
> --
> 2.20.1
> 

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

Re: [PATCH v4 3/4] mtd: spi-nor: otp: return -EROFS if region is read-only

[Michael Walle]

Am 2021-05-31 10:52, schrieb Tudor.Ambarus@microchip.com:
> On 5/21/21 10:40 PM, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> SPI NOR flashes will just ignore program commands if the OTP region is
>> locked. Thus, a user might not notice that the intended write didn't 
>> end
>> up in the flash. Return -EROFS to the user in this case. From what I 
>> can
>> tell, chips/cfi_cmdset_0001.c also return this error code.
>> 
>> One could optimize spi_nor_mtd_otp_range_is_locked() to read the 
>> status
>> register only once and not for every OTP region, but for that we would
>> need some more invasive changes. Given that this is
>> one-time-programmable memory and the normal access mode is reading, we
>> just live with the small overhead.
> 
> :)
> 
> Shouldn't we change
> struct spi_nor_otp_ops {
> 	...
>         int (*lock)(struct spi_nor *nor, unsigned int region);
>         int (*is_locked)(struct spi_nor *nor, unsigned int region);
> };
> 
> to:
> struct spi_nor_otp_ops {
> 	...
>         int (*lock)(struct spi_nor *nor, loff_t addr, size_t len);
> 
>         int (*is_locked)(struct spi_nor *nor, loff_t addr, size_t len);
> };
> 
> instead?

I had that, but then
  (1) it doesn't fit the hardware (the one's I know of) and the function
      itself would need to convert to the given range
  (2) each lock()/is_locked() would need to implement the "if at least
      one region is locked everything is locked", which might lead to
      different implementations.
  (3) in what address space is addr and len? I'd presume the one of the
      device (so is orthogonal to read()/write()). So if you get lets
      say addr=0x1000 len=512, you'd need to convert that into region
      0 and 1. Thus you'd have this mapping cluttered over all functions.
      And additionally, you'd first need to convert the mtd offsets
      addr=0 len=512 to addr=0x1000 and len=512.

-michael

Re: [PATCH v4 3/4] mtd: spi-nor: otp: return -EROFS if region is read-only

[Michael Walle]

Am 2021-05-31 10:52, schrieb Tudor.Ambarus@microchip.com:
> On 5/21/21 10:40 PM, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> SPI NOR flashes will just ignore program commands if the OTP region is
>> locked. Thus, a user might not notice that the intended write didn't 
>> end
>> up in the flash. Return -EROFS to the user in this case. From what I 
>> can
>> tell, chips/cfi_cmdset_0001.c also return this error code.
>> 
>> One could optimize spi_nor_mtd_otp_range_is_locked() to read the 
>> status
>> register only once and not for every OTP region, but for that we would
>> need some more invasive changes. Given that this is
>> one-time-programmable memory and the normal access mode is reading, we
>> just live with the small overhead.
> 
> :)
> 
> Shouldn't we change
> struct spi_nor_otp_ops {
> 	...
>         int (*lock)(struct spi_nor *nor, unsigned int region);
>         int (*is_locked)(struct spi_nor *nor, unsigned int region);
> };
> 
> to:
> struct spi_nor_otp_ops {
> 	...
>         int (*lock)(struct spi_nor *nor, loff_t addr, size_t len);
> 
>         int (*is_locked)(struct spi_nor *nor, loff_t addr, size_t len);
> };
> 
> instead?

I had that, but then
  (1) it doesn't fit the hardware (the one's I know of) and the function
      itself would need to convert to the given range
  (2) each lock()/is_locked() would need to implement the "if at least
      one region is locked everything is locked", which might lead to
      different implementations.
  (3) in what address space is addr and len? I'd presume the one of the
      device (so is orthogonal to read()/write()). So if you get lets
      say addr=0x1000 len=512, you'd need to convert that into region
      0 and 1. Thus you'd have this mapping cluttered over all functions.
      And additionally, you'd first need to convert the mtd offsets
      addr=0 len=512 to addr=0x1000 and len=512.

-michael

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

Re: [PATCH v4 3/4] mtd: spi-nor: otp: return -EROFS if region is read-only

[Tudor.Ambarus]

On 5/21/21 10:40 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> SPI NOR flashes will just ignore program commands if the OTP region is
> locked. Thus, a user might not notice that the intended write didn't end
> up in the flash. Return -EROFS to the user in this case. From what I can
> tell, chips/cfi_cmdset_0001.c also return this error code.
> 
> One could optimize spi_nor_mtd_otp_range_is_locked() to read the status
> register only once and not for every OTP region, but for that we would
> need some more invasive changes. Given that this is
> one-time-programmable memory and the normal access mode is reading, we
> just live with the small overhead.
> 
> Fixes: 069089acf88b ("mtd: spi-nor: add OTP support")
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/otp.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> index 3898ed67ba1c..b87f96593c13 100644
> --- a/drivers/mtd/spi-nor/otp.c
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -249,6 +249,31 @@ static int spi_nor_mtd_otp_info(struct mtd_info *mtd, size_t len,
>         return ret;
>  }
> 
> +static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor, loff_t ofs,
> +                                          size_t len)
> +{
> +       const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
> +       unsigned int region;
> +       int locked;
> +
> +       if (!len)
> +               return 0;

meh

> +
> +       /*
> +        * If any of the affected OTP regions are locked the entire range is
> +        * considered locked.
> +        */
> +       for (region = spi_nor_otp_offset_to_region(nor, ofs);
> +            region <= spi_nor_otp_offset_to_region(nor, ofs + len - 1);
> +            region++) {
> +               locked = ops->is_locked(nor, region);
> +               if (locked)
> +                       return locked;
> +       }
> +
> +       return 0;
> +}
> +
>  static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
>                                       size_t total_len, size_t *retlen,
>                                       const u8 *buf, bool is_write)
> @@ -271,6 +296,16 @@ static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
>         /* don't access beyond the end */
>         total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - ofs);

can we move the total_len computation before calling spi_nor_lock_and_prep(),
and just return 0 if total_len is zero? It will return early, avoiding unnecessary
steps.

> 
> +       if (is_write) {
> +               ret = spi_nor_mtd_otp_range_is_locked(nor, ofs, total_len);
> +               if (ret < 0) {
> +                       goto out;
> +               } else if (ret) {
> +                       ret = -EROFS;
> +                       goto out;
> +               }
> +       }
> +
>         *retlen = 0;
>         while (total_len) {
>                 /*
> --
> 2.20.1
> 

Re: [PATCH v4 3/4] mtd: spi-nor: otp: return -EROFS if region is read-only

[Tudor.Ambarus]

On 5/21/21 10:40 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> SPI NOR flashes will just ignore program commands if the OTP region is
> locked. Thus, a user might not notice that the intended write didn't end
> up in the flash. Return -EROFS to the user in this case. From what I can
> tell, chips/cfi_cmdset_0001.c also return this error code.
> 
> One could optimize spi_nor_mtd_otp_range_is_locked() to read the status
> register only once and not for every OTP region, but for that we would
> need some more invasive changes. Given that this is
> one-time-programmable memory and the normal access mode is reading, we
> just live with the small overhead.
> 
> Fixes: 069089acf88b ("mtd: spi-nor: add OTP support")
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/otp.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> index 3898ed67ba1c..b87f96593c13 100644
> --- a/drivers/mtd/spi-nor/otp.c
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -249,6 +249,31 @@ static int spi_nor_mtd_otp_info(struct mtd_info *mtd, size_t len,
>         return ret;
>  }
> 
> +static int spi_nor_mtd_otp_range_is_locked(struct spi_nor *nor, loff_t ofs,
> +                                          size_t len)
> +{
> +       const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
> +       unsigned int region;
> +       int locked;
> +
> +       if (!len)
> +               return 0;

meh

> +
> +       /*
> +        * If any of the affected OTP regions are locked the entire range is
> +        * considered locked.
> +        */
> +       for (region = spi_nor_otp_offset_to_region(nor, ofs);
> +            region <= spi_nor_otp_offset_to_region(nor, ofs + len - 1);
> +            region++) {
> +               locked = ops->is_locked(nor, region);
> +               if (locked)
> +                       return locked;
> +       }
> +
> +       return 0;
> +}
> +
>  static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
>                                       size_t total_len, size_t *retlen,
>                                       const u8 *buf, bool is_write)
> @@ -271,6 +296,16 @@ static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
>         /* don't access beyond the end */
>         total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - ofs);

can we move the total_len computation before calling spi_nor_lock_and_prep(),
and just return 0 if total_len is zero? It will return early, avoiding unnecessary
steps.

> 
> +       if (is_write) {
> +               ret = spi_nor_mtd_otp_range_is_locked(nor, ofs, total_len);
> +               if (ret < 0) {
> +                       goto out;
> +               } else if (ret) {
> +                       ret = -EROFS;
> +                       goto out;
> +               }
> +       }
> +
>         *retlen = 0;
>         while (total_len) {
>                 /*
> --
> 2.20.1
> 

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

[PATCH v4 4/4] mtd: spi-nor: otp: implement erase for Winbond and similar flashes

[Michael Walle]

Winbond flashes with OTP support provide a command to erase the OTP
data. This might come in handy during development.

This was tested with a Winbond W25Q32JW on a LS1028A SoC with the
NXP FSPI controller.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/core.c    |  2 +-
 drivers/mtd/spi-nor/core.h    |  4 ++
 drivers/mtd/spi-nor/otp.c     | 83 +++++++++++++++++++++++++++++++++++
 drivers/mtd/spi-nor/winbond.c |  1 +
 4 files changed, 89 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index bd2c7717eb10..9551effb6a44 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1318,7 +1318,7 @@ static u32 spi_nor_convert_addr(struct spi_nor *nor, loff_t addr)
 /*
  * Initiate the erasure of a single sector
  */
-static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
+int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
 {
 	int i;
 
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 28a2e0be97a3..9398a8738857 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -207,6 +207,7 @@ struct spi_nor_otp_organization {
  * @read:	read from the SPI NOR OTP area.
  * @write:	write to the SPI NOR OTP area.
  * @lock:	lock an OTP region.
+ * @erase:	erase an OTP region.
  * @is_locked:	check if an OTP region of the SPI NOR is locked.
  */
 struct spi_nor_otp_ops {
@@ -214,6 +215,7 @@ struct spi_nor_otp_ops {
 	int (*write)(struct spi_nor *nor, loff_t addr, size_t len,
 		     const u8 *buf);
 	int (*lock)(struct spi_nor *nor, unsigned int region);
+	int (*erase)(struct spi_nor *nor, loff_t addr);
 	int (*is_locked)(struct spi_nor *nor, unsigned int region);
 };
 
@@ -503,10 +505,12 @@ ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
 			  u8 *buf);
 ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
 			   const u8 *buf);
+int spi_nor_erase_sector(struct spi_nor *nor, u32 addr);
 
 int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
 int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
 			   const u8 *buf);
+int spi_nor_otp_erase_secr(struct spi_nor *nor, loff_t addr);
 int spi_nor_otp_lock_sr2(struct spi_nor *nor, unsigned int region);
 int spi_nor_otp_is_locked_sr2(struct spi_nor *nor, unsigned int region);
 
diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
index b87f96593c13..afa348a3af70 100644
--- a/drivers/mtd/spi-nor/otp.c
+++ b/drivers/mtd/spi-nor/otp.c
@@ -120,6 +120,38 @@ int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
 	return ret ?: written;
 }
 
+/**
+ * spi_nor_otp_erase_secr() - erase a security register
+ * @nor:        pointer to 'struct spi_nor'
+ * @addr:       offset of the security register to be erased
+ *
+ * Erase a security register by using the SPINOR_OP_ESECR command.
+ *
+ * For more information on the term "security register", see the documentation
+ * of spi_nor_otp_read_secr().
+ *
+ * This method is used on GigaDevice and Winbond flashes.
+ *
+ * Return: 0 on success, -errno otherwise
+ */
+int spi_nor_otp_erase_secr(struct spi_nor *nor, loff_t addr)
+{
+	u8 erase_opcode = nor->erase_opcode;
+	int ret;
+
+	ret = spi_nor_write_enable(nor);
+	if (ret)
+		return ret;
+
+	nor->erase_opcode = SPINOR_OP_ESECR;
+	ret = spi_nor_erase_sector(nor, addr);
+	nor->erase_opcode = erase_opcode;
+	if (ret)
+		return ret;
+
+	return spi_nor_wait_till_ready(nor);
+}
+
 static int spi_nor_otp_lock_bit_cr(unsigned int region)
 {
 	static const int lock_bits[] = { SR2_LB1, SR2_LB2, SR2_LB3 };
@@ -360,6 +392,56 @@ static int spi_nor_mtd_otp_write(struct mtd_info *mtd, loff_t to, size_t len,
 	return spi_nor_mtd_otp_read_write(mtd, to, len, retlen, buf, true);
 }
 
+static int spi_nor_mtd_otp_erase(struct mtd_info *mtd, loff_t from, size_t len)
+{
+	struct spi_nor *nor = mtd_to_spi_nor(mtd);
+	const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
+	const size_t rlen = spi_nor_otp_region_len(nor);
+	unsigned int region;
+	loff_t rstart;
+	int ret;
+
+	/* OTP erase is optional */
+	if (!ops->erase)
+		return -EOPNOTSUPP;
+
+	if (from < 0 || (from + len) > spi_nor_otp_size(nor))
+		return -EINVAL;
+
+	/* the user has to explicitly ask for whole regions */
+	if (!IS_ALIGNED(len, rlen) || !IS_ALIGNED(from, rlen))
+		return -EINVAL;
+
+	ret = spi_nor_lock_and_prep(nor);
+	if (ret)
+		return ret;
+
+	ret = spi_nor_mtd_otp_range_is_locked(nor, from, len);
+	if (ret < 0) {
+		goto out;
+	} else if (ret) {
+		ret = -EROFS;
+		goto out;
+	}
+
+	while (len) {
+		region = spi_nor_otp_offset_to_region(nor, from);
+		rstart = spi_nor_otp_region_start(nor, region);
+
+		ret = ops->erase(nor, rstart);
+		if (ret)
+			goto out;
+
+		len -= rlen;
+		from += rlen;
+	}
+
+out:
+	spi_nor_unlock_and_unprep(nor);
+
+	return ret;
+}
+
 static int spi_nor_mtd_otp_lock(struct mtd_info *mtd, loff_t from, size_t len)
 {
 	struct spi_nor *nor = mtd_to_spi_nor(mtd);
@@ -418,4 +500,5 @@ void spi_nor_otp_init(struct spi_nor *nor)
 	mtd->_read_user_prot_reg = spi_nor_mtd_otp_read;
 	mtd->_write_user_prot_reg = spi_nor_mtd_otp_write;
 	mtd->_lock_user_prot_reg = spi_nor_mtd_otp_lock;
+	mtd->_erase_user_prot_reg = spi_nor_mtd_otp_erase;
 }
diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
index 9a81c67a60c6..96573f61caf5 100644
--- a/drivers/mtd/spi-nor/winbond.c
+++ b/drivers/mtd/spi-nor/winbond.c
@@ -139,6 +139,7 @@ static int winbond_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
 static const struct spi_nor_otp_ops winbond_otp_ops = {
 	.read = spi_nor_otp_read_secr,
 	.write = spi_nor_otp_write_secr,
+	.erase = spi_nor_otp_erase_secr,
 	.lock = spi_nor_otp_lock_sr2,
 	.is_locked = spi_nor_otp_is_locked_sr2,
 };
-- 
2.20.1

[PATCH v4 4/4] mtd: spi-nor: otp: implement erase for Winbond and similar flashes

[Michael Walle]

Winbond flashes with OTP support provide a command to erase the OTP
data. This might come in handy during development.

This was tested with a Winbond W25Q32JW on a LS1028A SoC with the
NXP FSPI controller.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/core.c    |  2 +-
 drivers/mtd/spi-nor/core.h    |  4 ++
 drivers/mtd/spi-nor/otp.c     | 83 +++++++++++++++++++++++++++++++++++
 drivers/mtd/spi-nor/winbond.c |  1 +
 4 files changed, 89 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index bd2c7717eb10..9551effb6a44 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1318,7 +1318,7 @@ static u32 spi_nor_convert_addr(struct spi_nor *nor, loff_t addr)
 /*
  * Initiate the erasure of a single sector
  */
-static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
+int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
 {
 	int i;
 
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 28a2e0be97a3..9398a8738857 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -207,6 +207,7 @@ struct spi_nor_otp_organization {
  * @read:	read from the SPI NOR OTP area.
  * @write:	write to the SPI NOR OTP area.
  * @lock:	lock an OTP region.
+ * @erase:	erase an OTP region.
  * @is_locked:	check if an OTP region of the SPI NOR is locked.
  */
 struct spi_nor_otp_ops {
@@ -214,6 +215,7 @@ struct spi_nor_otp_ops {
 	int (*write)(struct spi_nor *nor, loff_t addr, size_t len,
 		     const u8 *buf);
 	int (*lock)(struct spi_nor *nor, unsigned int region);
+	int (*erase)(struct spi_nor *nor, loff_t addr);
 	int (*is_locked)(struct spi_nor *nor, unsigned int region);
 };
 
@@ -503,10 +505,12 @@ ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
 			  u8 *buf);
 ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
 			   const u8 *buf);
+int spi_nor_erase_sector(struct spi_nor *nor, u32 addr);
 
 int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
 int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
 			   const u8 *buf);
+int spi_nor_otp_erase_secr(struct spi_nor *nor, loff_t addr);
 int spi_nor_otp_lock_sr2(struct spi_nor *nor, unsigned int region);
 int spi_nor_otp_is_locked_sr2(struct spi_nor *nor, unsigned int region);
 
diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
index b87f96593c13..afa348a3af70 100644
--- a/drivers/mtd/spi-nor/otp.c
+++ b/drivers/mtd/spi-nor/otp.c
@@ -120,6 +120,38 @@ int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
 	return ret ?: written;
 }
 
+/**
+ * spi_nor_otp_erase_secr() - erase a security register
+ * @nor:        pointer to 'struct spi_nor'
+ * @addr:       offset of the security register to be erased
+ *
+ * Erase a security register by using the SPINOR_OP_ESECR command.
+ *
+ * For more information on the term "security register", see the documentation
+ * of spi_nor_otp_read_secr().
+ *
+ * This method is used on GigaDevice and Winbond flashes.
+ *
+ * Return: 0 on success, -errno otherwise
+ */
+int spi_nor_otp_erase_secr(struct spi_nor *nor, loff_t addr)
+{
+	u8 erase_opcode = nor->erase_opcode;
+	int ret;
+
+	ret = spi_nor_write_enable(nor);
+	if (ret)
+		return ret;
+
+	nor->erase_opcode = SPINOR_OP_ESECR;
+	ret = spi_nor_erase_sector(nor, addr);
+	nor->erase_opcode = erase_opcode;
+	if (ret)
+		return ret;
+
+	return spi_nor_wait_till_ready(nor);
+}
+
 static int spi_nor_otp_lock_bit_cr(unsigned int region)
 {
 	static const int lock_bits[] = { SR2_LB1, SR2_LB2, SR2_LB3 };
@@ -360,6 +392,56 @@ static int spi_nor_mtd_otp_write(struct mtd_info *mtd, loff_t to, size_t len,
 	return spi_nor_mtd_otp_read_write(mtd, to, len, retlen, buf, true);
 }
 
+static int spi_nor_mtd_otp_erase(struct mtd_info *mtd, loff_t from, size_t len)
+{
+	struct spi_nor *nor = mtd_to_spi_nor(mtd);
+	const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
+	const size_t rlen = spi_nor_otp_region_len(nor);
+	unsigned int region;
+	loff_t rstart;
+	int ret;
+
+	/* OTP erase is optional */
+	if (!ops->erase)
+		return -EOPNOTSUPP;
+
+	if (from < 0 || (from + len) > spi_nor_otp_size(nor))
+		return -EINVAL;
+
+	/* the user has to explicitly ask for whole regions */
+	if (!IS_ALIGNED(len, rlen) || !IS_ALIGNED(from, rlen))
+		return -EINVAL;
+
+	ret = spi_nor_lock_and_prep(nor);
+	if (ret)
+		return ret;
+
+	ret = spi_nor_mtd_otp_range_is_locked(nor, from, len);
+	if (ret < 0) {
+		goto out;
+	} else if (ret) {
+		ret = -EROFS;
+		goto out;
+	}
+
+	while (len) {
+		region = spi_nor_otp_offset_to_region(nor, from);
+		rstart = spi_nor_otp_region_start(nor, region);
+
+		ret = ops->erase(nor, rstart);
+		if (ret)
+			goto out;
+
+		len -= rlen;
+		from += rlen;
+	}
+
+out:
+	spi_nor_unlock_and_unprep(nor);
+
+	return ret;
+}
+
 static int spi_nor_mtd_otp_lock(struct mtd_info *mtd, loff_t from, size_t len)
 {
 	struct spi_nor *nor = mtd_to_spi_nor(mtd);
@@ -418,4 +500,5 @@ void spi_nor_otp_init(struct spi_nor *nor)
 	mtd->_read_user_prot_reg = spi_nor_mtd_otp_read;
 	mtd->_write_user_prot_reg = spi_nor_mtd_otp_write;
 	mtd->_lock_user_prot_reg = spi_nor_mtd_otp_lock;
+	mtd->_erase_user_prot_reg = spi_nor_mtd_otp_erase;
 }
diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
index 9a81c67a60c6..96573f61caf5 100644
--- a/drivers/mtd/spi-nor/winbond.c
+++ b/drivers/mtd/spi-nor/winbond.c
@@ -139,6 +139,7 @@ static int winbond_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
 static const struct spi_nor_otp_ops winbond_otp_ops = {
 	.read = spi_nor_otp_read_secr,
 	.write = spi_nor_otp_write_secr,
+	.erase = spi_nor_otp_erase_secr,
 	.lock = spi_nor_otp_lock_sr2,
 	.is_locked = spi_nor_otp_is_locked_sr2,
 };
-- 
2.20.1


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

Re: [PATCH v4 4/4] mtd: spi-nor: otp: implement erase for Winbond and similar flashes

[Pratyush Yadav]

On 21/05/21 09:40PM, Michael Walle wrote:
> Winbond flashes with OTP support provide a command to erase the OTP
> data. This might come in handy during development.
> 
> This was tested with a Winbond W25Q32JW on a LS1028A SoC with the
> NXP FSPI controller.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v4 4/4] mtd: spi-nor: otp: implement erase for Winbond and similar flashes

[Pratyush Yadav]

On 21/05/21 09:40PM, Michael Walle wrote:
> Winbond flashes with OTP support provide a command to erase the OTP
> data. This might come in handy during development.
> 
> This was tested with a Winbond W25Q32JW on a LS1028A SoC with the
> NXP FSPI controller.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

Re: [PATCH v4 4/4] mtd: spi-nor: otp: implement erase for Winbond and similar flashes

[Tudor.Ambarus]

On 5/21/21 10:40 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Winbond flashes with OTP support provide a command to erase the OTP
> data. This might come in handy during development.
> 
> This was tested with a Winbond W25Q32JW on a LS1028A SoC with the
> NXP FSPI controller.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/core.c    |  2 +-
>  drivers/mtd/spi-nor/core.h    |  4 ++
>  drivers/mtd/spi-nor/otp.c     | 83 +++++++++++++++++++++++++++++++++++
>  drivers/mtd/spi-nor/winbond.c |  1 +
>  4 files changed, 89 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index bd2c7717eb10..9551effb6a44 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1318,7 +1318,7 @@ static u32 spi_nor_convert_addr(struct spi_nor *nor, loff_t addr)
>  /*
>   * Initiate the erasure of a single sector
>   */
> -static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
> +int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>  {
>         int i;
> 
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 28a2e0be97a3..9398a8738857 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -207,6 +207,7 @@ struct spi_nor_otp_organization {
>   * @read:      read from the SPI NOR OTP area.
>   * @write:     write to the SPI NOR OTP area.
>   * @lock:      lock an OTP region.
> + * @erase:     erase an OTP region.
>   * @is_locked: check if an OTP region of the SPI NOR is locked.
>   */
>  struct spi_nor_otp_ops {
> @@ -214,6 +215,7 @@ struct spi_nor_otp_ops {
>         int (*write)(struct spi_nor *nor, loff_t addr, size_t len,
>                      const u8 *buf);
>         int (*lock)(struct spi_nor *nor, unsigned int region);
> +       int (*erase)(struct spi_nor *nor, loff_t addr);

maybe better:
int (*erase)(struct spi_nor *nor, loff_t addr, size_t len);

>         int (*is_locked)(struct spi_nor *nor, unsigned int region);
>  };

Re: [PATCH v4 4/4] mtd: spi-nor: otp: implement erase for Winbond and similar flashes

[Tudor.Ambarus]

On 5/21/21 10:40 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Winbond flashes with OTP support provide a command to erase the OTP
> data. This might come in handy during development.
> 
> This was tested with a Winbond W25Q32JW on a LS1028A SoC with the
> NXP FSPI controller.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/core.c    |  2 +-
>  drivers/mtd/spi-nor/core.h    |  4 ++
>  drivers/mtd/spi-nor/otp.c     | 83 +++++++++++++++++++++++++++++++++++
>  drivers/mtd/spi-nor/winbond.c |  1 +
>  4 files changed, 89 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index bd2c7717eb10..9551effb6a44 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1318,7 +1318,7 @@ static u32 spi_nor_convert_addr(struct spi_nor *nor, loff_t addr)
>  /*
>   * Initiate the erasure of a single sector
>   */
> -static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
> +int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>  {
>         int i;
> 
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 28a2e0be97a3..9398a8738857 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -207,6 +207,7 @@ struct spi_nor_otp_organization {
>   * @read:      read from the SPI NOR OTP area.
>   * @write:     write to the SPI NOR OTP area.
>   * @lock:      lock an OTP region.
> + * @erase:     erase an OTP region.
>   * @is_locked: check if an OTP region of the SPI NOR is locked.
>   */
>  struct spi_nor_otp_ops {
> @@ -214,6 +215,7 @@ struct spi_nor_otp_ops {
>         int (*write)(struct spi_nor *nor, loff_t addr, size_t len,
>                      const u8 *buf);
>         int (*lock)(struct spi_nor *nor, unsigned int region);
> +       int (*erase)(struct spi_nor *nor, loff_t addr);

maybe better:
int (*erase)(struct spi_nor *nor, loff_t addr, size_t len);

>         int (*is_locked)(struct spi_nor *nor, unsigned int region);
>  };

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

Re: [PATCH v4 4/4] mtd: spi-nor: otp: implement erase for Winbond and similar flashes

[Michael Walle]

Am 2021-05-31 10:56, schrieb Tudor.Ambarus@microchip.com:
> On 5/21/21 10:40 PM, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Winbond flashes with OTP support provide a command to erase the OTP
>> data. This might come in handy during development.
>> 
>> This was tested with a Winbond W25Q32JW on a LS1028A SoC with the
>> NXP FSPI controller.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  drivers/mtd/spi-nor/core.c    |  2 +-
>>  drivers/mtd/spi-nor/core.h    |  4 ++
>>  drivers/mtd/spi-nor/otp.c     | 83 
>> +++++++++++++++++++++++++++++++++++
>>  drivers/mtd/spi-nor/winbond.c |  1 +
>>  4 files changed, 89 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index bd2c7717eb10..9551effb6a44 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -1318,7 +1318,7 @@ static u32 spi_nor_convert_addr(struct spi_nor 
>> *nor, loff_t addr)
>>  /*
>>   * Initiate the erasure of a single sector
>>   */
>> -static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>> +int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>>  {
>>         int i;
>> 
>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> index 28a2e0be97a3..9398a8738857 100644
>> --- a/drivers/mtd/spi-nor/core.h
>> +++ b/drivers/mtd/spi-nor/core.h
>> @@ -207,6 +207,7 @@ struct spi_nor_otp_organization {
>>   * @read:      read from the SPI NOR OTP area.
>>   * @write:     write to the SPI NOR OTP area.
>>   * @lock:      lock an OTP region.
>> + * @erase:     erase an OTP region.
>>   * @is_locked: check if an OTP region of the SPI NOR is locked.
>>   */
>>  struct spi_nor_otp_ops {
>> @@ -214,6 +215,7 @@ struct spi_nor_otp_ops {
>>         int (*write)(struct spi_nor *nor, loff_t addr, size_t len,
>>                      const u8 *buf);
>>         int (*lock)(struct spi_nor *nor, unsigned int region);
>> +       int (*erase)(struct spi_nor *nor, loff_t addr);
> 
> maybe better:
> int (*erase)(struct spi_nor *nor, loff_t addr, size_t len);

Right now these ops represent what is sent to the hardware. The address
is what is really sent over the wire. The read and write has the same
semantics (and limitations, like you can't cross region boundaries etc).

As there is only one kind of method to access OTP right now, I wouldn't
want to prematurely optimize it for future ones. It is easy to change
afterwards if there is need to. And supplying a length right now would
mean to have a loop and some checks in the erase() op; where it should
really be "erase one region" and not "erase multiple ones". Mapping
from multiple to one should be the duty of the otp core code.

So, one could argue about the parameter "unsigned in region" vs.
"loff_t addr" because both represent the same thing. But
spi_nor_otp_erase_secr() should really take the address, because
that is what the opcode takes as an argument. Thus, changing the
argument of the .erase to "unsigned int region" you couldn't just
assign spi_nor_otp_erase_secr to .erase but you'd need to have a
small wrapper which converts the argument.

So IMHO future will tell if we have to change this to somewhat
more generic.

-michael

Re: [PATCH v4 4/4] mtd: spi-nor: otp: implement erase for Winbond and similar flashes

[Michael Walle]

Am 2021-05-31 10:56, schrieb Tudor.Ambarus@microchip.com:
> On 5/21/21 10:40 PM, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Winbond flashes with OTP support provide a command to erase the OTP
>> data. This might come in handy during development.
>> 
>> This was tested with a Winbond W25Q32JW on a LS1028A SoC with the
>> NXP FSPI controller.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  drivers/mtd/spi-nor/core.c    |  2 +-
>>  drivers/mtd/spi-nor/core.h    |  4 ++
>>  drivers/mtd/spi-nor/otp.c     | 83 
>> +++++++++++++++++++++++++++++++++++
>>  drivers/mtd/spi-nor/winbond.c |  1 +
>>  4 files changed, 89 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index bd2c7717eb10..9551effb6a44 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -1318,7 +1318,7 @@ static u32 spi_nor_convert_addr(struct spi_nor 
>> *nor, loff_t addr)
>>  /*
>>   * Initiate the erasure of a single sector
>>   */
>> -static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>> +int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>>  {
>>         int i;
>> 
>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> index 28a2e0be97a3..9398a8738857 100644
>> --- a/drivers/mtd/spi-nor/core.h
>> +++ b/drivers/mtd/spi-nor/core.h
>> @@ -207,6 +207,7 @@ struct spi_nor_otp_organization {
>>   * @read:      read from the SPI NOR OTP area.
>>   * @write:     write to the SPI NOR OTP area.
>>   * @lock:      lock an OTP region.
>> + * @erase:     erase an OTP region.
>>   * @is_locked: check if an OTP region of the SPI NOR is locked.
>>   */
>>  struct spi_nor_otp_ops {
>> @@ -214,6 +215,7 @@ struct spi_nor_otp_ops {
>>         int (*write)(struct spi_nor *nor, loff_t addr, size_t len,
>>                      const u8 *buf);
>>         int (*lock)(struct spi_nor *nor, unsigned int region);
>> +       int (*erase)(struct spi_nor *nor, loff_t addr);
> 
> maybe better:
> int (*erase)(struct spi_nor *nor, loff_t addr, size_t len);

Right now these ops represent what is sent to the hardware. The address
is what is really sent over the wire. The read and write has the same
semantics (and limitations, like you can't cross region boundaries etc).

As there is only one kind of method to access OTP right now, I wouldn't
want to prematurely optimize it for future ones. It is easy to change
afterwards if there is need to. And supplying a length right now would
mean to have a loop and some checks in the erase() op; where it should
really be "erase one region" and not "erase multiple ones". Mapping
from multiple to one should be the duty of the otp core code.

So, one could argue about the parameter "unsigned in region" vs.
"loff_t addr" because both represent the same thing. But
spi_nor_otp_erase_secr() should really take the address, because
that is what the opcode takes as an argument. Thus, changing the
argument of the .erase to "unsigned int region" you couldn't just
assign spi_nor_otp_erase_secr to .erase but you'd need to have a
small wrapper which converts the argument.

So IMHO future will tell if we have to change this to somewhat
more generic.

-michael

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

Re: [PATCH v4 4/4] mtd: spi-nor: otp: implement erase for Winbond and similar flashes

[Tudor.Ambarus]

On 6/1/21 4:30 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2021-05-31 10:56, schrieb Tudor.Ambarus@microchip.com:
>> On 5/21/21 10:40 PM, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Winbond flashes with OTP support provide a command to erase the OTP
>>> data. This might come in handy during development.
>>>
>>> This was tested with a Winbond W25Q32JW on a LS1028A SoC with the
>>> NXP FSPI controller.
>>>
>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>> ---
>>>  drivers/mtd/spi-nor/core.c    |  2 +-
>>>  drivers/mtd/spi-nor/core.h    |  4 ++
>>>  drivers/mtd/spi-nor/otp.c     | 83
>>> +++++++++++++++++++++++++++++++++++
>>>  drivers/mtd/spi-nor/winbond.c |  1 +
>>>  4 files changed, 89 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>> index bd2c7717eb10..9551effb6a44 100644
>>> --- a/drivers/mtd/spi-nor/core.c
>>> +++ b/drivers/mtd/spi-nor/core.c
>>> @@ -1318,7 +1318,7 @@ static u32 spi_nor_convert_addr(struct spi_nor
>>> *nor, loff_t addr)
>>>  /*
>>>   * Initiate the erasure of a single sector
>>>   */
>>> -static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>>> +int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>>>  {
>>>         int i;
>>>
>>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>>> index 28a2e0be97a3..9398a8738857 100644
>>> --- a/drivers/mtd/spi-nor/core.h
>>> +++ b/drivers/mtd/spi-nor/core.h
>>> @@ -207,6 +207,7 @@ struct spi_nor_otp_organization {
>>>   * @read:      read from the SPI NOR OTP area.
>>>   * @write:     write to the SPI NOR OTP area.
>>>   * @lock:      lock an OTP region.
>>> + * @erase:     erase an OTP region.
>>>   * @is_locked: check if an OTP region of the SPI NOR is locked.
>>>   */
>>>  struct spi_nor_otp_ops {
>>> @@ -214,6 +215,7 @@ struct spi_nor_otp_ops {
>>>         int (*write)(struct spi_nor *nor, loff_t addr, size_t len,
>>>                      const u8 *buf);
>>>         int (*lock)(struct spi_nor *nor, unsigned int region);
>>> +       int (*erase)(struct spi_nor *nor, loff_t addr);
>>
>> maybe better:
>> int (*erase)(struct spi_nor *nor, loff_t addr, size_t len);
> 
> Right now these ops represent what is sent to the hardware. The address
> is what is really sent over the wire. The read and write has the same
> semantics (and limitations, like you can't cross region boundaries etc).
> 
> As there is only one kind of method to access OTP right now, I wouldn't
> want to prematurely optimize it for future ones. It is easy to change
> afterwards if there is need to. And supplying a length right now would
> mean to have a loop and some checks in the erase() op; where it should
> really be "erase one region" and not "erase multiple ones". Mapping
> from multiple to one should be the duty of the otp core code.

Indeed. Duty of the SPI NOR otp core, because I see now that mtd passes
this responsibility with its OTPGETREGIONCOUNT, and how it handles its
otp_info struct.

> 
> So, one could argue about the parameter "unsigned in region" vs.
> "loff_t addr" because both represent the same thing. But
> spi_nor_otp_erase_secr() should really take the address, because
> that is what the opcode takes as an argument. Thus, changing the
> argument of the .erase to "unsigned int region" you couldn't just
> assign spi_nor_otp_erase_secr to .erase but you'd need to have a
> small wrapper which converts the argument.
> 

Ok. Looks good.

> So IMHO future will tell if we have to change this to somewhat
> more generic.
> 
> -michael

Re: [PATCH v4 4/4] mtd: spi-nor: otp: implement erase for Winbond and similar flashes

[Tudor.Ambarus]

On 6/1/21 4:30 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2021-05-31 10:56, schrieb Tudor.Ambarus@microchip.com:
>> On 5/21/21 10:40 PM, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Winbond flashes with OTP support provide a command to erase the OTP
>>> data. This might come in handy during development.
>>>
>>> This was tested with a Winbond W25Q32JW on a LS1028A SoC with the
>>> NXP FSPI controller.
>>>
>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>> ---
>>>  drivers/mtd/spi-nor/core.c    |  2 +-
>>>  drivers/mtd/spi-nor/core.h    |  4 ++
>>>  drivers/mtd/spi-nor/otp.c     | 83
>>> +++++++++++++++++++++++++++++++++++
>>>  drivers/mtd/spi-nor/winbond.c |  1 +
>>>  4 files changed, 89 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>> index bd2c7717eb10..9551effb6a44 100644
>>> --- a/drivers/mtd/spi-nor/core.c
>>> +++ b/drivers/mtd/spi-nor/core.c
>>> @@ -1318,7 +1318,7 @@ static u32 spi_nor_convert_addr(struct spi_nor
>>> *nor, loff_t addr)
>>>  /*
>>>   * Initiate the erasure of a single sector
>>>   */
>>> -static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>>> +int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>>>  {
>>>         int i;
>>>
>>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>>> index 28a2e0be97a3..9398a8738857 100644
>>> --- a/drivers/mtd/spi-nor/core.h
>>> +++ b/drivers/mtd/spi-nor/core.h
>>> @@ -207,6 +207,7 @@ struct spi_nor_otp_organization {
>>>   * @read:      read from the SPI NOR OTP area.
>>>   * @write:     write to the SPI NOR OTP area.
>>>   * @lock:      lock an OTP region.
>>> + * @erase:     erase an OTP region.
>>>   * @is_locked: check if an OTP region of the SPI NOR is locked.
>>>   */
>>>  struct spi_nor_otp_ops {
>>> @@ -214,6 +215,7 @@ struct spi_nor_otp_ops {
>>>         int (*write)(struct spi_nor *nor, loff_t addr, size_t len,
>>>                      const u8 *buf);
>>>         int (*lock)(struct spi_nor *nor, unsigned int region);
>>> +       int (*erase)(struct spi_nor *nor, loff_t addr);
>>
>> maybe better:
>> int (*erase)(struct spi_nor *nor, loff_t addr, size_t len);
> 
> Right now these ops represent what is sent to the hardware. The address
> is what is really sent over the wire. The read and write has the same
> semantics (and limitations, like you can't cross region boundaries etc).
> 
> As there is only one kind of method to access OTP right now, I wouldn't
> want to prematurely optimize it for future ones. It is easy to change
> afterwards if there is need to. And supplying a length right now would
> mean to have a loop and some checks in the erase() op; where it should
> really be "erase one region" and not "erase multiple ones". Mapping
> from multiple to one should be the duty of the otp core code.

Indeed. Duty of the SPI NOR otp core, because I see now that mtd passes
this responsibility with its OTPGETREGIONCOUNT, and how it handles its
otp_info struct.

> 
> So, one could argue about the parameter "unsigned in region" vs.
> "loff_t addr" because both represent the same thing. But
> spi_nor_otp_erase_secr() should really take the address, because
> that is what the opcode takes as an argument. Thus, changing the
> argument of the .erase to "unsigned int region" you couldn't just
> assign spi_nor_otp_erase_secr to .erase but you'd need to have a
> small wrapper which converts the argument.
> 

Ok. Looks good.

> So IMHO future will tell if we have to change this to somewhat
> more generic.
> 
> -michael

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

Re: [PATCH v4 4/4] mtd: spi-nor: otp: implement erase for Winbond and similar flashes

[Tudor.Ambarus]

On 5/21/21 10:40 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Winbond flashes with OTP support provide a command to erase the OTP
> data. This might come in handy during development.
> 
> This was tested with a Winbond W25Q32JW on a LS1028A SoC with the
> NXP FSPI controller.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/core.c    |  2 +-
>  drivers/mtd/spi-nor/core.h    |  4 ++
>  drivers/mtd/spi-nor/otp.c     | 83 +++++++++++++++++++++++++++++++++++
>  drivers/mtd/spi-nor/winbond.c |  1 +
>  4 files changed, 89 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index bd2c7717eb10..9551effb6a44 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1318,7 +1318,7 @@ static u32 spi_nor_convert_addr(struct spi_nor *nor, loff_t addr)
>  /*
>   * Initiate the erasure of a single sector
>   */
> -static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
> +int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>  {
>         int i;
> 
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 28a2e0be97a3..9398a8738857 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -207,6 +207,7 @@ struct spi_nor_otp_organization {
>   * @read:      read from the SPI NOR OTP area.
>   * @write:     write to the SPI NOR OTP area.
>   * @lock:      lock an OTP region.
> + * @erase:     erase an OTP region.
>   * @is_locked: check if an OTP region of the SPI NOR is locked.
>   */
>  struct spi_nor_otp_ops {
> @@ -214,6 +215,7 @@ struct spi_nor_otp_ops {
>         int (*write)(struct spi_nor *nor, loff_t addr, size_t len,
>                      const u8 *buf);
>         int (*lock)(struct spi_nor *nor, unsigned int region);
> +       int (*erase)(struct spi_nor *nor, loff_t addr);
>         int (*is_locked)(struct spi_nor *nor, unsigned int region);
>  };
> 
> @@ -503,10 +505,12 @@ ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
>                           u8 *buf);
>  ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
>                            const u8 *buf);
> +int spi_nor_erase_sector(struct spi_nor *nor, u32 addr);
> 
>  int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
>  int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
>                            const u8 *buf);
> +int spi_nor_otp_erase_secr(struct spi_nor *nor, loff_t addr);
>  int spi_nor_otp_lock_sr2(struct spi_nor *nor, unsigned int region);
>  int spi_nor_otp_is_locked_sr2(struct spi_nor *nor, unsigned int region);
> 
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> index b87f96593c13..afa348a3af70 100644
> --- a/drivers/mtd/spi-nor/otp.c
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -120,6 +120,38 @@ int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
>         return ret ?: written;
>  }
> 
> +/**
> + * spi_nor_otp_erase_secr() - erase a security register
> + * @nor:        pointer to 'struct spi_nor'
> + * @addr:       offset of the security register to be erased
> + *
> + * Erase a security register by using the SPINOR_OP_ESECR command.
> + *
> + * For more information on the term "security register", see the documentation
> + * of spi_nor_otp_read_secr().
> + *
> + * This method is used on GigaDevice and Winbond flashes.
> + *
> + * Return: 0 on success, -errno otherwise
> + */
> +int spi_nor_otp_erase_secr(struct spi_nor *nor, loff_t addr)
> +{
> +       u8 erase_opcode = nor->erase_opcode;
> +       int ret;
> +
> +       ret = spi_nor_write_enable(nor);
> +       if (ret)
> +               return ret;
> +
> +       nor->erase_opcode = SPINOR_OP_ESECR;
> +       ret = spi_nor_erase_sector(nor, addr);
> +       nor->erase_opcode = erase_opcode;
> +       if (ret)
> +               return ret;
> +
> +       return spi_nor_wait_till_ready(nor);
> +}
> +
>  static int spi_nor_otp_lock_bit_cr(unsigned int region)
>  {
>         static const int lock_bits[] = { SR2_LB1, SR2_LB2, SR2_LB3 };
> @@ -360,6 +392,56 @@ static int spi_nor_mtd_otp_write(struct mtd_info *mtd, loff_t to, size_t len,
>         return spi_nor_mtd_otp_read_write(mtd, to, len, retlen, buf, true);
>  }
> 
> +static int spi_nor_mtd_otp_erase(struct mtd_info *mtd, loff_t from, size_t len)
> +{
> +       struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +       const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
> +       const size_t rlen = spi_nor_otp_region_len(nor);
> +       unsigned int region;
> +       loff_t rstart;
> +       int ret;
> +
> +       /* OTP erase is optional */
> +       if (!ops->erase)
> +               return -EOPNOTSUPP;

if (!len)
	return 0;

> +
> +       if (from < 0 || (from + len) > spi_nor_otp_size(nor))
> +               return -EINVAL;
> +
> +       /* the user has to explicitly ask for whole regions */
> +       if (!IS_ALIGNED(len, rlen) || !IS_ALIGNED(from, rlen))
> +               return -EINVAL;
> +
> +       ret = spi_nor_lock_and_prep(nor);
> +       if (ret)
> +               return ret;
> +
> +       ret = spi_nor_mtd_otp_range_is_locked(nor, from, len);
> +       if (ret < 0) {
> +               goto out;
> +       } else if (ret) {
> +               ret = -EROFS;
> +               goto out;
> +       }
> +
> +       while (len) {
> +               region = spi_nor_otp_offset_to_region(nor, from);
> +               rstart = spi_nor_otp_region_start(nor, region);
> +
> +               ret = ops->erase(nor, rstart);
> +               if (ret)
> +                       goto out;
> +
> +               len -= rlen;
> +               from += rlen;
> +       }
> +
> +out:
> +       spi_nor_unlock_and_unprep(nor);
> +
> +       return ret;
> +}
> +
>  static int spi_nor_mtd_otp_lock(struct mtd_info *mtd, loff_t from, size_t len)
>  {
>         struct spi_nor *nor = mtd_to_spi_nor(mtd);
> @@ -418,4 +500,5 @@ void spi_nor_otp_init(struct spi_nor *nor)
>         mtd->_read_user_prot_reg = spi_nor_mtd_otp_read;
>         mtd->_write_user_prot_reg = spi_nor_mtd_otp_write;
>         mtd->_lock_user_prot_reg = spi_nor_mtd_otp_lock;
> +       mtd->_erase_user_prot_reg = spi_nor_mtd_otp_erase;
>  }
> diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
> index 9a81c67a60c6..96573f61caf5 100644
> --- a/drivers/mtd/spi-nor/winbond.c
> +++ b/drivers/mtd/spi-nor/winbond.c
> @@ -139,6 +139,7 @@ static int winbond_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
>  static const struct spi_nor_otp_ops winbond_otp_ops = {
>         .read = spi_nor_otp_read_secr,
>         .write = spi_nor_otp_write_secr,
> +       .erase = spi_nor_otp_erase_secr,
>         .lock = spi_nor_otp_lock_sr2,
>         .is_locked = spi_nor_otp_is_locked_sr2,
>  };
> --
> 2.20.1
> 

Re: [PATCH v4 4/4] mtd: spi-nor: otp: implement erase for Winbond and similar flashes

[Tudor.Ambarus]

On 5/21/21 10:40 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Winbond flashes with OTP support provide a command to erase the OTP
> data. This might come in handy during development.
> 
> This was tested with a Winbond W25Q32JW on a LS1028A SoC with the
> NXP FSPI controller.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/core.c    |  2 +-
>  drivers/mtd/spi-nor/core.h    |  4 ++
>  drivers/mtd/spi-nor/otp.c     | 83 +++++++++++++++++++++++++++++++++++
>  drivers/mtd/spi-nor/winbond.c |  1 +
>  4 files changed, 89 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index bd2c7717eb10..9551effb6a44 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1318,7 +1318,7 @@ static u32 spi_nor_convert_addr(struct spi_nor *nor, loff_t addr)
>  /*
>   * Initiate the erasure of a single sector
>   */
> -static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
> +int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>  {
>         int i;
> 
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 28a2e0be97a3..9398a8738857 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -207,6 +207,7 @@ struct spi_nor_otp_organization {
>   * @read:      read from the SPI NOR OTP area.
>   * @write:     write to the SPI NOR OTP area.
>   * @lock:      lock an OTP region.
> + * @erase:     erase an OTP region.
>   * @is_locked: check if an OTP region of the SPI NOR is locked.
>   */
>  struct spi_nor_otp_ops {
> @@ -214,6 +215,7 @@ struct spi_nor_otp_ops {
>         int (*write)(struct spi_nor *nor, loff_t addr, size_t len,
>                      const u8 *buf);
>         int (*lock)(struct spi_nor *nor, unsigned int region);
> +       int (*erase)(struct spi_nor *nor, loff_t addr);
>         int (*is_locked)(struct spi_nor *nor, unsigned int region);
>  };
> 
> @@ -503,10 +505,12 @@ ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
>                           u8 *buf);
>  ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
>                            const u8 *buf);
> +int spi_nor_erase_sector(struct spi_nor *nor, u32 addr);
> 
>  int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
>  int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
>                            const u8 *buf);
> +int spi_nor_otp_erase_secr(struct spi_nor *nor, loff_t addr);
>  int spi_nor_otp_lock_sr2(struct spi_nor *nor, unsigned int region);
>  int spi_nor_otp_is_locked_sr2(struct spi_nor *nor, unsigned int region);
> 
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> index b87f96593c13..afa348a3af70 100644
> --- a/drivers/mtd/spi-nor/otp.c
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -120,6 +120,38 @@ int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len,
>         return ret ?: written;
>  }
> 
> +/**
> + * spi_nor_otp_erase_secr() - erase a security register
> + * @nor:        pointer to 'struct spi_nor'
> + * @addr:       offset of the security register to be erased
> + *
> + * Erase a security register by using the SPINOR_OP_ESECR command.
> + *
> + * For more information on the term "security register", see the documentation
> + * of spi_nor_otp_read_secr().
> + *
> + * This method is used on GigaDevice and Winbond flashes.
> + *
> + * Return: 0 on success, -errno otherwise
> + */
> +int spi_nor_otp_erase_secr(struct spi_nor *nor, loff_t addr)
> +{
> +       u8 erase_opcode = nor->erase_opcode;
> +       int ret;
> +
> +       ret = spi_nor_write_enable(nor);
> +       if (ret)
> +               return ret;
> +
> +       nor->erase_opcode = SPINOR_OP_ESECR;
> +       ret = spi_nor_erase_sector(nor, addr);
> +       nor->erase_opcode = erase_opcode;
> +       if (ret)
> +               return ret;
> +
> +       return spi_nor_wait_till_ready(nor);
> +}
> +
>  static int spi_nor_otp_lock_bit_cr(unsigned int region)
>  {
>         static const int lock_bits[] = { SR2_LB1, SR2_LB2, SR2_LB3 };
> @@ -360,6 +392,56 @@ static int spi_nor_mtd_otp_write(struct mtd_info *mtd, loff_t to, size_t len,
>         return spi_nor_mtd_otp_read_write(mtd, to, len, retlen, buf, true);
>  }
> 
> +static int spi_nor_mtd_otp_erase(struct mtd_info *mtd, loff_t from, size_t len)
> +{
> +       struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +       const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
> +       const size_t rlen = spi_nor_otp_region_len(nor);
> +       unsigned int region;
> +       loff_t rstart;
> +       int ret;
> +
> +       /* OTP erase is optional */
> +       if (!ops->erase)
> +               return -EOPNOTSUPP;

if (!len)
	return 0;

> +
> +       if (from < 0 || (from + len) > spi_nor_otp_size(nor))
> +               return -EINVAL;
> +
> +       /* the user has to explicitly ask for whole regions */
> +       if (!IS_ALIGNED(len, rlen) || !IS_ALIGNED(from, rlen))
> +               return -EINVAL;
> +
> +       ret = spi_nor_lock_and_prep(nor);
> +       if (ret)
> +               return ret;
> +
> +       ret = spi_nor_mtd_otp_range_is_locked(nor, from, len);
> +       if (ret < 0) {
> +               goto out;
> +       } else if (ret) {
> +               ret = -EROFS;
> +               goto out;
> +       }
> +
> +       while (len) {
> +               region = spi_nor_otp_offset_to_region(nor, from);
> +               rstart = spi_nor_otp_region_start(nor, region);
> +
> +               ret = ops->erase(nor, rstart);
> +               if (ret)
> +                       goto out;
> +
> +               len -= rlen;
> +               from += rlen;
> +       }
> +
> +out:
> +       spi_nor_unlock_and_unprep(nor);
> +
> +       return ret;
> +}
> +
>  static int spi_nor_mtd_otp_lock(struct mtd_info *mtd, loff_t from, size_t len)
>  {
>         struct spi_nor *nor = mtd_to_spi_nor(mtd);
> @@ -418,4 +500,5 @@ void spi_nor_otp_init(struct spi_nor *nor)
>         mtd->_read_user_prot_reg = spi_nor_mtd_otp_read;
>         mtd->_write_user_prot_reg = spi_nor_mtd_otp_write;
>         mtd->_lock_user_prot_reg = spi_nor_mtd_otp_lock;
> +       mtd->_erase_user_prot_reg = spi_nor_mtd_otp_erase;
>  }
> diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
> index 9a81c67a60c6..96573f61caf5 100644
> --- a/drivers/mtd/spi-nor/winbond.c
> +++ b/drivers/mtd/spi-nor/winbond.c
> @@ -139,6 +139,7 @@ static int winbond_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
>  static const struct spi_nor_otp_ops winbond_otp_ops = {
>         .read = spi_nor_otp_read_secr,
>         .write = spi_nor_otp_write_secr,
> +       .erase = spi_nor_otp_erase_secr,
>         .lock = spi_nor_otp_lock_sr2,
>         .is_locked = spi_nor_otp_is_locked_sr2,
>  };
> --
> 2.20.1
> 

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