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 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
There is also a patch for mtd-utils to add a small tool to issue
the erase:
https://lore.kernel.org/linux-mtd/[email protected]/
Michael Walle (3):
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: implement erase for Winbond and similar flashes
drivers/mtd/spi-nor/core.c | 6 +--
drivers/mtd/spi-nor/core.h | 6 +++
drivers/mtd/spi-nor/otp.c | 79 +++++++++++++++++++++++++++++------
drivers/mtd/spi-nor/winbond.c | 1 +
4 files changed, 77 insertions(+), 15 deletions(-)
--
2.20.1
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 <[email protected]>
---
drivers/mtd/spi-nor/core.c | 2 +-
drivers/mtd/spi-nor/core.h | 4 +++
drivers/mtd/spi-nor/otp.c | 58 +++++++++++++++++++++++++++++++++--
drivers/mtd/spi-nor/winbond.c | 1 +
4 files changed, 62 insertions(+), 3 deletions(-)
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 ec0c1b33f7cc..2dc315b6bffc 100644
--- a/drivers/mtd/spi-nor/otp.c
+++ b/drivers/mtd/spi-nor/otp.c
@@ -111,6 +111,34 @@ 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. This method
+ * is used on GigaDevice and Winbond flashes to erase OTP data.
+ *
+ * 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 };
@@ -316,12 +344,14 @@ 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_lock(struct mtd_info *mtd, loff_t from, size_t len)
+static int spi_nor_mtd_otp_lock_or_erase(struct mtd_info *mtd, loff_t from,
+ size_t len, bool is_erase)
{
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;
if (from < 0 || (from + len) > spi_nor_otp_size(nor))
@@ -337,7 +367,13 @@ static int spi_nor_mtd_otp_lock(struct mtd_info *mtd, loff_t from, size_t len)
while (len) {
region = spi_nor_otp_offset_to_region(nor, from);
- ret = ops->lock(nor, region);
+
+ if (is_erase) {
+ rstart = spi_nor_otp_region_start(nor, region);
+ ret = ops->erase(nor, rstart);
+ } else {
+ ret = ops->lock(nor, region);
+ }
if (ret)
goto out;
@@ -351,6 +387,23 @@ static int spi_nor_mtd_otp_lock(struct mtd_info *mtd, loff_t from, size_t len)
return ret;
}
+static int spi_nor_mtd_otp_lock(struct mtd_info *mtd, loff_t from, size_t len)
+{
+ return spi_nor_mtd_otp_lock_or_erase(mtd, from, len, false);
+}
+
+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;
+
+ /* OTP erase is optional */
+ if (!ops->erase)
+ return -EOPNOTSUPP;
+
+ return spi_nor_mtd_otp_lock_or_erase(mtd, from, len, true);
+}
+
void spi_nor_otp_init(struct spi_nor *nor)
{
struct mtd_info *mtd = &nor->mtd;
@@ -374,4 +427,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
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 <[email protected]>
---
drivers/mtd/spi-nor/otp.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
index 91a4c510ed51..ec0c1b33f7cc 100644
--- a/drivers/mtd/spi-nor/otp.c
+++ b/drivers/mtd/spi-nor/otp.c
@@ -15,14 +15,16 @@
#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. This method
+ * is used on GigaDevice and Winbond flashes to access OTP data.
+ *
+ * Please note, the read must not span multiple registers.
*
* Return: number of bytes read successfully, -errno otherwise
*/
@@ -56,16 +58,16 @@ 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. This method
+ * is used on GigaDevice and Winbond flashes to access OTP data.
*
- * 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 +90,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
On 20/05/21 05:58PM, Michael Walle wrote:
> 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 <[email protected]>
> ---
> drivers/mtd/spi-nor/otp.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> index 91a4c510ed51..ec0c1b33f7cc 100644
> --- a/drivers/mtd/spi-nor/otp.c
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -15,14 +15,16 @@
> #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. This method
> + * is used on GigaDevice and Winbond flashes to access OTP data.
> + *
> + * Please note, the read must not span multiple registers.
Security register is a very vague term. It in no way tells you that it
means OTP data unless you already know. If I read this for the first
time, I would think it is some sort of password or access enable/disable
type of register, and not OTP data region.
Please add a note to clarify this. Same for the write and erase
functions.
With this fixed,
Reviewed-by: Pratyush Yadav <[email protected]>
> *
> * Return: number of bytes read successfully, -errno otherwise
> */
> @@ -56,16 +58,16 @@ 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. This method
> + * is used on GigaDevice and Winbond flashes to access OTP data.
> *
> - * 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 +90,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
>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Am 2021-05-20 19:51, schrieb Pratyush Yadav:
> On 20/05/21 05:58PM, 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 <[email protected]>
>> ---
> [...]
>> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
>> index ec0c1b33f7cc..2dc315b6bffc 100644
>> --- a/drivers/mtd/spi-nor/otp.c
>> +++ b/drivers/mtd/spi-nor/otp.c
>> @@ -111,6 +111,34 @@ 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.
>> This method
>> + * is used on GigaDevice and Winbond flashes to erase OTP data.
>> + *
>> + * 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);
>
> The datasheet for W25Q32JW says in Section 8.2.29:
>
> The Security Register Lock Bits (LB3-1) in the Status Register-2 can
> be used to OTP protect the security registers. Once a lock bit is set
> to 1, the corresponding security register will be permanently locked,
> Erase Security Register instruction to that register will be ignored
>
> So if the region is locked, the flash will happily accept the erase and
> simply do nothing. So will the program. So when the OTP region is
> locked
> and someone does an erase-program cycle, they will think their data
> went
> through even though it was simply thrown away by the flash.
Btw, this is also how it is handled for the "normal" data write
protection.
If block protection bits are set, we happily accept erase and write
commands although the flash will discard the commands.
> I think you should check that bit before doing these operations to make
> sure it is actually allowed. If it isn't, return an error code (-EPERM
> maybe).
It's not that easy. You'd have to check the entire range, not just a
part
of it. Otherwise you'll end up with half of the data committed to the
flash
and the other half rejected with EPERM. So you'd have to do in
spi_nor_mtd_otp_erase() and spi_nor_mtd_otp_write().
I'm not opposed to it, but I tried to behave in the same way as the
other MTD chips which supports the OTP ops and I thought they would
ignore it, too. But I just had another look:
- chips/cfi_cmdset_0001.c: seems to return -EROFS (partial writes are
possible?). I couldn't find any datasheets where command 0xc0 is
used to write OTP data. Thus I can only guess that status bit 1 is
program failed or something like that [1]. All the datasheets I found
will tell you its bit 0.
- chips/cfi_cmdset_0002.c: seems to ignore errors
- nand/onenand/onenand_base.c: I really don't know..
If no one opposes I'll add that check with EROFS.
-michael
[1]
https://elixir.bootlin.com/linux/v5.13-rc2/source/drivers/mtd/chips/cfi_cmdset_0001.c#L1606
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 <[email protected]>
---
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
On 20/05/21 05:58PM, Michael Walle wrote:
> 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 <[email protected]>
I have not done due diligence in researching this topic. But the premise
sounds good to me. So,
Acked-by: Pratyush Yadav <[email protected]>
> ---
> 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
>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
On 20/05/21 05:58PM, 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 <[email protected]>
> ---
[...]
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> index ec0c1b33f7cc..2dc315b6bffc 100644
> --- a/drivers/mtd/spi-nor/otp.c
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -111,6 +111,34 @@ 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. This method
> + * is used on GigaDevice and Winbond flashes to erase OTP data.
> + *
> + * 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);
The datasheet for W25Q32JW says in Section 8.2.29:
The Security Register Lock Bits (LB3-1) in the Status Register-2 can
be used to OTP protect the security registers. Once a lock bit is set
to 1, the corresponding security register will be permanently locked,
Erase Security Register instruction to that register will be ignored
So if the region is locked, the flash will happily accept the erase and
simply do nothing. So will the program. So when the OTP region is locked
and someone does an erase-program cycle, they will think their data went
through even though it was simply thrown away by the flash.
I think you should check that bit before doing these operations to make
sure it is actually allowed. If it isn't, return an error code (-EPERM
maybe).
> +}
> +
> static int spi_nor_otp_lock_bit_cr(unsigned int region)
> {
> static const int lock_bits[] = { SR2_LB1, SR2_LB2, SR2_LB3 };
[...]
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Am 2021-05-20 19:39, schrieb Pratyush Yadav:
> On 20/05/21 05:58PM, Michael Walle wrote:
>> 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 <[email protected]>
>> ---
>> drivers/mtd/spi-nor/otp.c | 18 ++++++++++--------
>> 1 file changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
>> index 91a4c510ed51..ec0c1b33f7cc 100644
>> --- a/drivers/mtd/spi-nor/otp.c
>> +++ b/drivers/mtd/spi-nor/otp.c
>> @@ -15,14 +15,16 @@
>> #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.
>> This method
>> + * is used on GigaDevice and Winbond flashes to access OTP data.
>> + *
>> + * Please note, the read must not span multiple registers.
>
> Security register is a very vague term. It in no way tells you that it
> means OTP data unless you already know. If I read this for the first
> time, I would think it is some sort of password or access
> enable/disable
> type of register, and not OTP data region.
Hence why I didn't use that wording. Also if I read register I think
of some small amount of bits, not a whole 256bytes. But it is like it
is.
-michael
On 5/20/21 6:58 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 <[email protected]>
> ---
> 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;
>
I'm not convinced that this is good. I see that winbond uses 3 bytes addr with for the
SPINOR_OP_RSECR and SPINOR_OP_PSECR, even for bigger flashes:
https://www.winbond.com/hq/product/code-storage-flash-memory/serial-nor-flash/?__locale=en&partNo=W25Q256JW
Micheal, can you double check?
ta
On 5/31/21 11:00 AM, Tudor Ambarus - M18064 wrote:
> On 5/20/21 6:58 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 <[email protected]>
>> ---
>> 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;
>>
>
> I'm not convinced that this is good. I see that winbond uses 3 bytes addr with for the
> SPINOR_OP_RSECR and SPINOR_OP_PSECR, even for bigger flashes:
> https://www.winbond.com/hq/product/code-storage-flash-memory/serial-nor-flash/?__locale=en&partNo=W25Q256JW
>
> Micheal, can you double check?
>
Doubled checked with Michael, looks good:
Reviewed-by: Tudor Ambarus <[email protected]>
On 5/20/21 6:58 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 <[email protected]>
Reviewed-by: Tudor Ambarus <[email protected]>
> ---
> drivers/mtd/spi-nor/otp.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> index 91a4c510ed51..ec0c1b33f7cc 100644
> --- a/drivers/mtd/spi-nor/otp.c
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -15,14 +15,16 @@
> #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. This method
> + * is used on GigaDevice and Winbond flashes to access OTP data.
> + *
> + * Please note, the read must not span multiple registers.
> *
> * Return: number of bytes read successfully, -errno otherwise
> */
> @@ -56,16 +58,16 @@ 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. This method
> + * is used on GigaDevice and Winbond flashes to access OTP data.
> *
> - * 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 +90,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
>