Re: [PATCH v2 4/5] mtd: spinand: micron: OTP access for MT29F2G01ABAGD

[Miquel Raynal]

Hi Martin,

mmkurbanov@xxxxxxxxxxxxxxxxx wrote on Tue, 27 Aug 2024 20:49:02 +0300:

> Support for OTP area access on Micron MT29F2G01ABAGD chip.
> 
> Signed-off-by: Martin Kurbanov <mmkurbanov@xxxxxxxxxxxxxxxxx>
> ---
>  drivers/mtd/nand/spi/micron.c | 117 +++++++++++++++++++++++++++++++++-
>  1 file changed, 116 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
> index 8d741be6d5f3e..a538409db4ccd 100644
> --- a/drivers/mtd/nand/spi/micron.c
> +++ b/drivers/mtd/nand/spi/micron.c
> @@ -9,6 +9,7 @@
>  #include <linux/device.h>
>  #include <linux/kernel.h>
>  #include <linux/mtd/spinand.h>
> +#include <linux/spi/spi-mem.h>
>  
>  #define SPINAND_MFR_MICRON		0x2c
>  
> @@ -28,6 +29,16 @@
>  
>  #define MICRON_SELECT_DIE(x)	((x) << 6)
>  
> +#define MICRON_MT29F2G01ABAGD_OTP_PAGES			12
> +#define MICRON_MT29F2G01ABAGD_OTP_PAGE_SIZE		2176

In the core we did add the data size and the OOB size to get the OTP
page size. I would prefer something dynamic here as well, otherwise the
implementation is very device specific for now reason?

> +#define MICRON_MT29F2G01ABAGD_OTP_SIZE_BYTES		\
> +	(MICRON_MT29F2G01ABAGD_OTP_PAGES *		\
> +	 MICRON_MT29F2G01ABAGD_OTP_PAGE_SIZE)

This is a function from the core as well once you've filled all the
information in the core structures, so why hardcoding it?

> +#define MICRON_MT29F2G01ABAGD_CFG_OTP_STATE		BIT(7)
> +#define MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK		\
> +	(CFG_OTP_ENABLE | MICRON_MT29F2G01ABAGD_CFG_OTP_STATE)
> +
>  static SPINAND_OP_VARIANTS(quadio_read_cache_variants,
>  		//SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
>  		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
> @@ -182,6 +193,108 @@ static int micron_8_ecc_get_status(struct spinand_device *spinand,
>  	return -EINVAL;
>  }
>  
> +static int mt29f2g01abagd_otp_is_locked(struct spinand_device *spinand)
> +{
> +	size_t buf_size = MICRON_MT29F2G01ABAGD_OTP_PAGE_SIZE;
> +	size_t retlen;
> +	u8 *buf;
> +	int ret;
> +
> +	buf = kmalloc(buf_size, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = spinand_upd_cfg(spinand,
> +			      MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK,
> +			      MICRON_MT29F2G01ABAGD_CFG_OTP_STATE);
> +	if (ret)
> +		goto out;

can we name the label free_buf?

> +
> +	ret = spinand_otp_read(spinand, 0, buf_size, buf, &retlen);
> +
> +	if (spinand_upd_cfg(spinand, MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK,
> +			    0)) {
> +		WARN(1, "Can not disable OTP mode\n");

I prefer dev_warn as well here, so we know which device is concerned.

> +		ret = -EIO;
> +	}
> +
> +	if (!ret) {

if (ret)
	goto out;

> +		size_t i = 0;
> +
> +		/* If all zeros, then the OTP area is locked. */
> +		while (i < buf_size && *(uint32_t *)(&buf[i]) == 0)
> +			i += 4;

Shall we expect buf_size to always be a multiple of 4? (real question)

I am not a big fan of the casting game here. I see the optimization
you're attempting to do, but I'm a little bit skeptical. I must admit I
didn't find a helper for that, buf at least maybe you can use an
intermediate variable and loop over it.

> +
> +		if (i == buf_size)
> +			ret = 1;

If buf_size is not a multiple of 4, this is not gonna work.

> +	}
> +
> +out:
> +	kfree(buf);
> +	return ret;
> +}
> +
> +static int mt29f2g01abagd_otp_info(struct spinand_device *spinand, size_t len,
> +				   struct otp_info *buf, size_t *retlen)
> +{
> +	int locked;
> +
> +	if (len < sizeof(*buf))
> +		return -EINVAL;
> +
> +	locked = mt29f2g01abagd_otp_is_locked(spinand);
> +	if (locked < 0)
> +		return locked;
> +
> +	buf->locked = locked;
> +	buf->start = 0;
> +	buf->length = MICRON_MT29F2G01ABAGD_OTP_SIZE_BYTES;
> +
> +	*retlen = sizeof(*buf);
> +	return 0;
> +}
> +
> +static int mt29f2g01abagd_otp_lock(struct spinand_device *spinand, loff_t from,
> +				   size_t len)
> +{
> +	struct spi_mem_op write_op = SPINAND_WR_EN_DIS_OP(true);
> +	struct spi_mem_op exec_op = SPINAND_PROG_EXEC_OP(0);
> +	int ret;
> +
> +	ret = spinand_upd_cfg(spinand,
> +			      MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK,
> +			      MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK);
> +	if (!ret)
> +		return ret;
> +
> +	ret = spi_mem_exec_op(spinand->spimem, &write_op);
> +	if (!ret)
> +		goto out;
> +
> +	ret = spi_mem_exec_op(spinand->spimem, &exec_op);
> +	if (!ret)
> +		goto out;
> +
> +	ret = spinand_wait(spinand, 10, 5, NULL);

Usually I expect timeouts to be bigger.

> +	if (!ret)
> +		goto out;

This goto seems to have not impact :)

> +
> +out:
> +	if (spinand_upd_cfg(spinand, MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK, 0)) {
> +		WARN(1, "Can not disable OTP mode\n");

dev_warn()

> +		ret = -EIO;
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct spinand_otp_ops mt29f2g01abagd_otp_ops = {
> +	.info = mt29f2g01abagd_otp_info,
> +	.lock = mt29f2g01abagd_otp_lock,
> +	.read = spinand_otp_read,
> +	.write = spinand_otp_write,
> +};
> +
>  static const struct spinand_info micron_spinand_table[] = {
>  	/* M79A 2Gb 3.3V */
>  	SPINAND_INFO("MT29F2G01ABAGD",
> @@ -193,7 +306,9 @@ static const struct spinand_info micron_spinand_table[] = {
>  					      &x4_update_cache_variants),
>  		     0,
>  		     SPINAND_ECCINFO(&micron_8_ooblayout,
> -				     micron_8_ecc_get_status)),
> +				     micron_8_ecc_get_status),
> +		     SPINAND_OTP_INFO(MICRON_MT29F2G01ABAGD_OTP_PAGES,
> +				      &mt29f2g01abagd_otp_ops)),
>  	/* M79A 2Gb 1.8V */
>  	SPINAND_INFO("MT29F2G01ABBGD",
>  		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x25),


Thanks,
Miquèl