[v2,2/5] mtd: spinand: add OTP support

The MTD subsystem already supports accessing two OTP areas: user and
factory. User areas can be written by the user. This patch only adds
support for the user areas.

In this patch the OTP_INFO macro is provided to add parameters to
spinand_info.
To implement OTP operations, the client (flash driver) is provided with
5 callbacks: .read(), .write(), .info(), .lock(), .erase().

Signed-off-by: Martin Kurbanov <mmkurbanov@salutedevices.com>
---
 drivers/mtd/nand/spi/Makefile |   3 +-
 drivers/mtd/nand/spi/core.c   |   3 +
 drivers/mtd/nand/spi/otp.c    | 232 ++++++++++++++++++++++++++++++++++
 include/linux/mtd/spinand.h   |  56 ++++++++
 4 files changed, 293 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mtd/nand/spi/otp.c

Hello, Miquel. Thank you for the review.
Regarding your question ( https://lore.kernel.org/all/20240717103623.6d6b63be@xps-13/ ):

>> +int spinand_otp_read(struct spinand_device *spinand, loff_t from, size_t len,
>> +		     u8 *buf, size_t *retlen);
>> +
>> +int spinand_otp_write(struct spinand_device *spinand, loff_t from, size_t len,
>> +		      const u8 *buf, size_t *retlen);
>> +
> 
> Why exposing spinand_otp_read and spinand_otp_write ?

For the SPI-NAND chips we have (Micron, ESMT, FORESEE), the command
sequence for reading/writing OTP is the same. I decided to make these
functions global because other chips probably have similar read/write
OTP operations as well.

Hi Martin,

mmkurbanov@salutedevices.com wrote on Tue, 27 Aug 2024 20:57:04 +0300:

> Hello, Miquel. Thank you for the review.
> Regarding your question ( https://lore.kernel.org/all/20240717103623.6d6b63be@xps-13/ ):
> 
> >> +int spinand_otp_read(struct spinand_device *spinand, loff_t from, size_t len,
> >> +		     u8 *buf, size_t *retlen);
> >> +
> >> +int spinand_otp_write(struct spinand_device *spinand, loff_t from, size_t len,
> >> +		      const u8 *buf, size_t *retlen);
> >> +  
> > 
> > Why exposing spinand_otp_read and spinand_otp_write ?  
> 
> For the SPI-NAND chips we have (Micron, ESMT, FORESEE), the command
> sequence for reading/writing OTP is the same. I decided to make these
> functions global because other chips probably have similar read/write
> OTP operations as well.

Of course, I understand you might need them, but then the change does
not belong to this patch. Actually you've done that correctly for the
spinand_wait() helper.

You can do all the "make foo() global" operations in a single patch if
you want.

I will soon review more in deep the content of this patchset again.

Thanks,
Miquèl

Hi Martin,

mmkurbanov@salutedevices.com wrote on Tue, 27 Aug 2024 20:49:00 +0300:

> The MTD subsystem already supports accessing two OTP areas: user and
> factory. User areas can be written by the user. This patch only adds
> support for the user areas.
> 
> In this patch the OTP_INFO macro is provided to add parameters to
> spinand_info.
> To implement OTP operations, the client (flash driver) is provided with
> 5 callbacks: .read(), .write(), .info(), .lock(), .erase().

Overall this looks good, I have minor changes to request. I'm not yet
done down to the last patch, but I think the implementation is neat.

> Signed-off-by: Martin Kurbanov <mmkurbanov@salutedevices.com>
> ---
>  drivers/mtd/nand/spi/Makefile |   3 +-
>  drivers/mtd/nand/spi/core.c   |   3 +
>  drivers/mtd/nand/spi/otp.c    | 232 ++++++++++++++++++++++++++++++++++
>  include/linux/mtd/spinand.h   |  56 ++++++++
>  4 files changed, 293 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/mtd/nand/spi/otp.c
> 
> diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile
> index 19cc77288ebbc..60d2e830ffc6b 100644
> --- a/drivers/mtd/nand/spi/Makefile
> +++ b/drivers/mtd/nand/spi/Makefile
> @@ -1,4 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0
> -spinand-objs := core.o alliancememory.o ato.o esmt.o foresee.o gigadevice.o macronix.o
> +spinand-objs := core.o otp.o
> +spinand-objs += alliancememory.o ato.o esmt.o foresee.o gigadevice.o macronix.o
>  spinand-objs += micron.o paragon.o toshiba.o winbond.o xtx.o
>  obj-$(CONFIG_MTD_SPI_NAND) += spinand.o
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 807c24b0c7c4f..2cb825edd49d0 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -1111,6 +1111,7 @@ int spinand_match_and_init(struct spinand_device *spinand,
>  		spinand->flags = table[i].flags;
>  		spinand->id.len = 1 + table[i].devid.len;
>  		spinand->select_target = table[i].select_target;
> +		spinand->otp = &table[i].otp;
>  
>  		op = spinand_select_op_variant(spinand,
>  					       info->op_variants.read_cache);
> @@ -1292,6 +1293,8 @@ static int spinand_init(struct spinand_device *spinand)
>  	mtd->_max_bad_blocks = nanddev_mtd_max_bad_blocks;
>  	mtd->_resume = spinand_mtd_resume;
>  
> +	spinand_set_mtd_otp_ops(spinand);
> +
>  	if (nand->ecc.engine) {
>  		ret = mtd_ooblayout_count_freebytes(mtd);
>  		if (ret < 0)
> diff --git a/drivers/mtd/nand/spi/otp.c b/drivers/mtd/nand/spi/otp.c
> new file mode 100644
> index 0000000000000..d459f811f9c04
> --- /dev/null
> +++ b/drivers/mtd/nand/spi/otp.c
> @@ -0,0 +1,232 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
> + *
> + * Author: Martin Kurbanov <mmkurbanov@salutedevices.com>
> + */
> +
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/spinand.h>
> +
> +static unsigned int spinand_otp_npages(const struct spinand_device *spinand)
> +{
> +	return spinand->otp->layout.npages;
> +}
> +
> +static size_t spinand_otp_size(struct spinand_device *spinand)
> +{
> +	struct nand_device *nand = spinand_to_nand(spinand);
> +	size_t otp_pagesize = nanddev_page_size(nand) +
> +			      nanddev_per_page_oobsize(nand);
> +
> +	return spinand_otp_npages(spinand) * otp_pagesize;
> +}
> +
> +static int spinand_otp_rw(struct spinand_device *spinand, loff_t ofs,
> +			  size_t len, u8 *buf, size_t *retlen, bool is_write)
> +{
> +	struct nand_device *nand = spinand_to_nand(spinand);
> +	struct nand_page_io_req req = { 0 };

					= {}; is enough

> +	unsigned long long page;
> +	size_t copied = 0;
> +	size_t otp_pagesize = nanddev_page_size(nand) +
> +			      nanddev_per_page_oobsize(nand);
> +	int ret = 0;
> +
> +	page = ofs;
> +	req.dataoffs = do_div(page, otp_pagesize);
> +	req.pos.page = page;
> +	req.type = is_write ? NAND_PAGE_WRITE : NAND_PAGE_READ;
> +	req.mode = MTD_OPS_RAW;
> +	req.databuf.in = buf;
> +
> +	while (copied < len && req.pos.page < spinand_otp_npages(spinand)) {
> +		req.datalen = min_t(unsigned int,
> +				    otp_pagesize - req.dataoffs,
> +				    len - copied);
> +
> +		if (is_write)
> +			ret = spinand_write_page(spinand, &req);
> +		else
> +			ret = spinand_read_page(spinand, &req);
> +
> +		if (ret < 0)
> +			break;
> +
> +		req.dataoffs = 0;
> +		copied += req.datalen;
> +		req.pos.page++;
> +	}
> +
> +	*retlen = copied;
> +
> +	return ret;
> +}
> +
> +/**
> + * spinand_otp_read() - Read from OTP area
> + * @spinand: the spinand device
> + * @ofs: the offset to read
> + * @len: the number of data bytes to read
> + * @buf: the buffer to store the read data
> + * @retlen: the pointer to variable to store the number of read bytes
> + *
> + * Return: 0 on success, an error code otherwise.
> + */
> +int spinand_otp_read(struct spinand_device *spinand, loff_t ofs, size_t len,
> +		     u8 *buf, size_t *retlen)
> +{
> +	return spinand_otp_rw(spinand, ofs, len, buf, retlen, false);
> +}
> +
> +/**
> + * spinand_otp_write() - Write to OTP area
> + * @spinand:  the spinand device
> + * @ofs: the offset to write to
> + * @len: the number of bytes to write
> + * @buf: the buffer with data to write
> + * @retlen: the pointer to variable to store the number of written bytes
> + *
> + * Return: 0 on success, an error code otherwise.
> + */
> +int spinand_otp_write(struct spinand_device *spinand, loff_t ofs, size_t len,
> +		      const u8 *buf, size_t *retlen)
> +{
> +	return spinand_otp_rw(spinand, ofs, len, (u8 *)buf, retlen, true);
> +}

These spinand_otp_xxx() helpers are not yet used, and thus should be
introduced later, when they are useful.

> +
> +static int spinand_otp_check_bounds(struct spinand_device *spinand, loff_t ofs,
> +				    size_t len)
> +{
> +	if (ofs < 0 || ofs + len > spinand_otp_size(spinand))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int spinand_mtd_otp_info(struct mtd_info *mtd, size_t len,
> +				size_t *retlen, struct otp_info *buf)
> +{
> +	struct spinand_device *spinand = mtd_to_spinand(mtd);
> +	const struct spinand_otp_ops *ops = spinand->otp->ops;
> +	int ret;
> +
> +	mutex_lock(&spinand->lock);
> +	ret = ops->info(spinand, len, buf, retlen);
> +	mutex_unlock(&spinand->lock);
> +
> +	return ret;
> +}
> +
> +static int spinand_mtd_otp_rw(struct mtd_info *mtd, loff_t ofs, size_t len,
> +			      size_t *retlen, u8 *buf, bool is_write)
> +{
> +	struct spinand_device *spinand = mtd_to_spinand(mtd);
> +	const struct spinand_otp_ops *ops = spinand->otp->ops;
> +	int ret;
> +
> +	if (!len)
> +		return 0;
> +
> +	ret = spinand_otp_check_bounds(spinand, ofs, len);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&spinand->lock);
> +
> +	ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, CFG_OTP_ENABLE);
> +	if (ret)
> +		goto out_unlock;
> +
> +	if (is_write)
> +		ret = ops->write(spinand, ofs, len, buf, retlen);
> +	else
> +		ret = ops->read(spinand, ofs, len, buf, retlen);
> +
> +	if (spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0)) {
> +		pr_warn(1, "Can not disable OTP mode\n");

dev_warn?

> +		ret = -EIO;
> +	}
> +
> +out_unlock:
> +	mutex_unlock(&spinand->lock);
> +	return ret;
> +}
> +
> +static int spinand_mtd_otp_read(struct mtd_info *mtd, loff_t ofs, size_t len,
> +				size_t *retlen, u8 *buf)
> +{
> +	return spinand_mtd_otp_rw(mtd, ofs, len, retlen, buf, false);
> +}
> +
> +static int spinand_mtd_otp_write(struct mtd_info *mtd, loff_t ofs, size_t len,
> +				 size_t *retlen, const u8 *buf)
> +{
> +	return spinand_mtd_otp_rw(mtd, ofs, len, retlen, (u8 *)buf, true);
> +}
> +
> +static int spinand_mtd_otp_erase(struct mtd_info *mtd, loff_t ofs, size_t len)
> +{
> +	struct spinand_device *spinand = mtd_to_spinand(mtd);
> +	const struct spinand_otp_ops *ops = spinand->otp->ops;
> +	int ret;
> +
> +	if (!ops->erase)
> +		return -EOPNOTSUPP;
> +
> +	if (!len)
> +		return 0;
> +
> +	ret = spinand_otp_check_bounds(spinand, ofs, len);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&spinand->lock);
> +	ret = ops->erase(spinand, ofs, len);
> +	mutex_unlock(&spinand->lock);
> +
> +	return ret;
> +}
> +
> +static int spinand_mtd_otp_lock(struct mtd_info *mtd, loff_t ofs, size_t len)
> +{
> +	struct spinand_device *spinand = mtd_to_spinand(mtd);
> +	const struct spinand_otp_ops *ops = spinand->otp->ops;
> +	int ret;
> +
> +	if (!ops->lock)
> +		return -EOPNOTSUPP;
> +
> +	if (!len)
> +		return 0;
> +
> +	ret = spinand_otp_check_bounds(spinand, ofs, len);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&spinand->lock);
> +	ret = ops->lock(spinand, ofs, len);
> +	mutex_unlock(&spinand->lock);
> +
> +	return ret;
> +}
> +
> +/**
> + * spinand_set_mtd_otp_ops() - Set up OTP methods
> + * @spinand: the spinand device
> + *
> + * Set up OTP methods.
> + */
> +void spinand_set_mtd_otp_ops(struct spinand_device *spinand)
> +{
> +	struct mtd_info *mtd = spinand_to_mtd(spinand);
> +
> +	if (!spinand->otp->ops)

Could we use something else as check? It feels odd to check for otp ops
and then just ignore the fact that they are here. Maybe check npages or
otp_size() ?

> +		return;
> +
> +	mtd->_get_user_prot_info = spinand_mtd_otp_info;
> +	mtd->_read_user_prot_reg = spinand_mtd_otp_read;
> +	mtd->_write_user_prot_reg = spinand_mtd_otp_write;
> +	mtd->_lock_user_prot_reg = spinand_mtd_otp_lock;
> +	mtd->_erase_user_prot_reg = spinand_mtd_otp_erase;
> +}
> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> index 555846517faf6..8099f35f0e051 100644
> --- a/include/linux/mtd/spinand.h
> +++ b/include/linux/mtd/spinand.h
> @@ -322,6 +322,43 @@ struct spinand_ondie_ecc_conf {
>  	u8 status;
>  };

Thanks,
Miquèl

Hi Miquel,

On 10/1/24 12:12, Miquel Raynal wrote:

>> +/**
>> + * spinand_set_mtd_otp_ops() - Set up OTP methods
>> + * @spinand: the spinand device
>> + *
>> + * Set up OTP methods.
>> + */
>> +void spinand_set_mtd_otp_ops(struct spinand_device *spinand)
>> +{
>> +	struct mtd_info *mtd = spinand_to_mtd(spinand);
>> +
>> +	if (!spinand->otp->ops)
> 
> Could we use something else as check? It feels odd to check for otp ops
> and then just ignore the fact that they are here. Maybe check npages or
> otp_size() ?

A developer may not specify OTP callbacks:
SPINAND_OTP_INFO(otp_pages, NULL /* OTP ops */)

Or do you mean that it is better to check in each function
spinand_mtd_otp_{info,read,write,lock}? E.g.:
static int spinand_mtd_otp_erase(struct mtd_info *mtd, loff_t ofs, size_t len)
{
	struct spinand_device *spinand = mtd_to_spinand(mtd);
	const struct spinand_otp_ops *ops = spinand->otp->ops;
	int ret;

	if (!ops || !ops->erase)
		return -EOPNOTSUPP;

Hi Martin,

Sorry for the slow feedback.

> >> +/**
> >> + * spinand_set_mtd_otp_ops() - Set up OTP methods
> >> + * @spinand: the spinand device
> >> + *
> >> + * Set up OTP methods.
> >> + */
> >> +void spinand_set_mtd_otp_ops(struct spinand_device *spinand)
> >> +{
> >> +	struct mtd_info *mtd = spinand_to_mtd(spinand);
> >> +
> >> +	if (!spinand->otp->ops)  
> > 
> > Could we use something else as check? It feels odd to check for otp ops
> > and then just ignore the fact that they are here. Maybe check npages or
> > otp_size() ?  
> 
> A developer may not specify OTP callbacks:
> SPINAND_OTP_INFO(otp_pages, NULL /* OTP ops */)

Is this really a valid situation?

In set_mtd_otp_ops() you set spinand functions only if there are otp
operations. First, is it relevant to consider the fact that a device
would have an otp and not provide operations? Otherwise, my initial
comment was about the fact that the check seems uncorrelated with the
second part of the function.

Maybe setting these functions only if relevant is the best choice, so
you no longer have to make the checks after the init.

	if (ops && ops->erase)
		mtd->_otp_erase = spinand_otp_erase;
	...

?

Thanks,
Miquèl