[PATCH v2 0/3] MTD: spinand: Add spi_mem_poll_status() support

[PATCH v2 0/3] MTD: spinand: Add spi_mem_poll_status() support

[patrice.chotard]

From: Patrice Chotard <patrice.chotard@foss.st.com>

This series adds support for the spi_mem_poll_status() spinand
interface.
Some QSPI controllers allows to poll automatically memory 
status during operations (erase, read or write). This allows to 
offload the CPU for this task.
STM32 QSPI is supporting this feature, driver update are also
part of this series.

Changes in v2:
  - Indicates the spi_mem_poll_status() timeout unit
  - Use 2-byte wide status register
  - Add spi_mem_supports_op() call in spi_mem_poll_status()
  - Add completion management in spi_mem_poll_status()
  - Add offload/non-offload case mangement in spi_mem_poll_status()
  - Optimize the non-offload case by using read_poll_timeout()
  - mask and match stm32_qspi_poll_status()'s parameters are 2-byte wide
  - Make usage of new spi_mem_finalize_op() API in
    stm32_qspi_wait_poll_status()

Patrice Chotard (3):
  spi: spi-mem: add automatic poll status functions
  mtd: spinand: use the spi-mem poll status APIs
  spi: stm32-qspi: add automatic poll status feature

 drivers/mtd/nand/spi/core.c  | 17 ++++----
 drivers/spi/spi-mem.c        | 71 +++++++++++++++++++++++++++++++
 drivers/spi/spi-stm32-qspi.c | 81 ++++++++++++++++++++++++++++++++----
 include/linux/mtd/spinand.h  |  1 +
 include/linux/spi/spi-mem.h  | 10 +++++
 5 files changed, 164 insertions(+), 16 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/3] spi: spi-mem: add automatic poll status functions

[patrice.chotard]

From: Patrice Chotard <patrice.chotard@foss.st.com>

With STM32 QSPI, it is possible to poll the status register of the device.
This could be done to offload the CPU during an operation (erase or
program a SPI NAND for example).

spi_mem_poll_status API has been added to handle this feature.
This new function take care of the offload/non-offload cases.

For the non-offload case, use read_poll_timeout() to poll the status in
order to release CPU during this phase.

Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
---
Changes in v2:
  - Indicates the spi_mem_poll_status() timeout unit
  - Use 2-byte wide status register
  - Add spi_mem_supports_op() call in spi_mem_poll_status()
  - Add completion management in spi_mem_poll_status()
  - Add offload/non-offload case mangement in spi_mem_poll_status()
  - Optimize the non-offload case by using read_poll_timeout()

 drivers/spi/spi-mem.c       | 71 +++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi-mem.h | 10 ++++++
 2 files changed, 81 insertions(+)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index 1513553e4080..3f29c604df7d 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -6,6 +6,7 @@
  * Author: Boris Brezillon <boris.brezillon@bootlin.com>
  */
 #include <linux/dmaengine.h>
+#include <linux/iopoll.h>
 #include <linux/pm_runtime.h>
 #include <linux/spi/spi.h>
 #include <linux/spi/spi-mem.h>
@@ -743,6 +744,75 @@ static inline struct spi_mem_driver *to_spi_mem_drv(struct device_driver *drv)
 	return container_of(drv, struct spi_mem_driver, spidrv.driver);
 }
 
+/**
+ * spi_mem_finalize_op - report completion of spi_mem_op
+ * @ctlr: the controller reporting completion
+ *
+ * Called by SPI drivers using the spi-mem spi_mem_poll_status()
+ * implementation to notify it that the current spi_mem_op has
+ * finished.
+ */
+void spi_mem_finalize_op(struct spi_controller *ctlr)
+{
+	complete(&ctlr->xfer_completion);
+}
+EXPORT_SYMBOL_GPL(spi_mem_finalize_op);
+
+/**
+ * spi_mem_poll_status() - Poll memory device status
+ * @mem: SPI memory device
+ * @op: the memory operation to execute
+ * @mask: status bitmask to ckeck
+ * @match: (status & mask) expected value
+ * @timeout_ms: timeout in milliseconds
+ *
+ * This function send a polling status request to the controller driver
+ *
+ * Return: 0 in case of success, -ETIMEDOUT in case of error,
+ *         -EOPNOTSUPP if not supported.
+ */
+int spi_mem_poll_status(struct spi_mem *mem,
+			const struct spi_mem_op *op,
+			u16 mask, u16 match, u16 timeout_ms)
+{
+	struct spi_controller *ctlr = mem->spi->controller;
+	unsigned long ms;
+	int ret = -EOPNOTSUPP;
+	int exec_op_ret;
+	u16 *status;
+
+	if (!spi_mem_supports_op(mem, op))
+		return ret;
+
+	if (ctlr->mem_ops && ctlr->mem_ops->poll_status) {
+		ret = spi_mem_access_start(mem);
+		if (ret)
+			return ret;
+
+		reinit_completion(&ctlr->xfer_completion);
+
+		ret = ctlr->mem_ops->poll_status(mem, op, mask, match,
+						 timeout_ms);
+
+		ms = wait_for_completion_timeout(&ctlr->xfer_completion,
+						 msecs_to_jiffies(timeout_ms));
+
+		spi_mem_access_end(mem);
+		if (!ms)
+			return -ETIMEDOUT;
+	} else {
+		status = (u16 *)op->data.buf.in;
+		ret = read_poll_timeout(spi_mem_exec_op, exec_op_ret,
+					((*status) & mask) == match, 20,
+					timeout_ms * 1000, false, mem, op);
+		if (exec_op_ret)
+			return exec_op_ret;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(spi_mem_poll_status);
+
 static int spi_mem_probe(struct spi_device *spi)
 {
 	struct spi_mem_driver *memdrv = to_spi_mem_drv(spi->dev.driver);
@@ -763,6 +833,7 @@ static int spi_mem_probe(struct spi_device *spi)
 	if (IS_ERR_OR_NULL(mem->name))
 		return PTR_ERR_OR_ZERO(mem->name);
 
+	init_completion(&ctlr->xfer_completion);
 	spi_set_drvdata(spi, mem);
 
 	return memdrv->probe(mem);
diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index 2b65c9edc34e..0fbf5d0a3d31 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -250,6 +250,7 @@ static inline void *spi_mem_get_drvdata(struct spi_mem *mem)
  *		  the currently mapped area), and the caller of
  *		  spi_mem_dirmap_write() is responsible for calling it again in
  *		  this case.
+ * @poll_status: poll memory device status
  *
  * This interface should be implemented by SPI controllers providing an
  * high-level interface to execute SPI memory operation, which is usually the
@@ -274,6 +275,9 @@ struct spi_controller_mem_ops {
 			       u64 offs, size_t len, void *buf);
 	ssize_t (*dirmap_write)(struct spi_mem_dirmap_desc *desc,
 				u64 offs, size_t len, const void *buf);
+	int (*poll_status)(struct spi_mem *mem,
+			   const struct spi_mem_op *op,
+			   u16 mask, u16 match, unsigned long timeout);
 };
 
 /**
@@ -369,6 +373,12 @@ devm_spi_mem_dirmap_create(struct device *dev, struct spi_mem *mem,
 void devm_spi_mem_dirmap_destroy(struct device *dev,
 				 struct spi_mem_dirmap_desc *desc);
 
+void spi_mem_finalize_op(struct spi_controller *ctlr);
+
+int spi_mem_poll_status(struct spi_mem *mem,
+			const struct spi_mem_op *op,
+			u16 mask, u16 match, u16 timeout);
+
 int spi_mem_driver_register_with_owner(struct spi_mem_driver *drv,
 				       struct module *owner);
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] spi: spi-mem: add automatic poll status functions

[Boris Brezillon]

On Fri, 7 May 2021 15:17:54 +0200
<patrice.chotard@foss.st.com> wrote:

> From: Patrice Chotard <patrice.chotard@foss.st.com>
> 
> With STM32 QSPI, it is possible to poll the status register of the device.
> This could be done to offload the CPU during an operation (erase or
> program a SPI NAND for example).
> 
> spi_mem_poll_status API has been added to handle this feature.
> This new function take care of the offload/non-offload cases.
> 
> For the non-offload case, use read_poll_timeout() to poll the status in
> order to release CPU during this phase.
> 
> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
> ---
> Changes in v2:
>   - Indicates the spi_mem_poll_status() timeout unit
>   - Use 2-byte wide status register
>   - Add spi_mem_supports_op() call in spi_mem_poll_status()
>   - Add completion management in spi_mem_poll_status()
>   - Add offload/non-offload case mangement in spi_mem_poll_status()
>   - Optimize the non-offload case by using read_poll_timeout()
> 
>  drivers/spi/spi-mem.c       | 71 +++++++++++++++++++++++++++++++++++++
>  include/linux/spi/spi-mem.h | 10 ++++++
>  2 files changed, 81 insertions(+)
> 
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index 1513553e4080..3f29c604df7d 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -6,6 +6,7 @@
>   * Author: Boris Brezillon <boris.brezillon@bootlin.com>
>   */
>  #include <linux/dmaengine.h>
> +#include <linux/iopoll.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/spi/spi.h>
>  #include <linux/spi/spi-mem.h>
> @@ -743,6 +744,75 @@ static inline struct spi_mem_driver *to_spi_mem_drv(struct device_driver *drv)
>  	return container_of(drv, struct spi_mem_driver, spidrv.driver);
>  }
>  
> +/**
> + * spi_mem_finalize_op - report completion of spi_mem_op
> + * @ctlr: the controller reporting completion
> + *
> + * Called by SPI drivers using the spi-mem spi_mem_poll_status()
> + * implementation to notify it that the current spi_mem_op has
> + * finished.
> + */
> +void spi_mem_finalize_op(struct spi_controller *ctlr)
> +{
> +	complete(&ctlr->xfer_completion);
> +}
> +EXPORT_SYMBOL_GPL(spi_mem_finalize_op);
> +
> +/**
> + * spi_mem_poll_status() - Poll memory device status
> + * @mem: SPI memory device
> + * @op: the memory operation to execute
> + * @mask: status bitmask to ckeck
> + * @match: (status & mask) expected value
> + * @timeout_ms: timeout in milliseconds
> + *
> + * This function send a polling status request to the controller driver
> + *
> + * Return: 0 in case of success, -ETIMEDOUT in case of error,
> + *         -EOPNOTSUPP if not supported.
> + */
> +int spi_mem_poll_status(struct spi_mem *mem,
> +			const struct spi_mem_op *op,
> +			u16 mask, u16 match, u16 timeout_ms)
> +{
> +	struct spi_controller *ctlr = mem->spi->controller;
> +	unsigned long ms;
> +	int ret = -EOPNOTSUPP;
> +	int exec_op_ret;
> +	u16 *status;
> +
> +	if (!spi_mem_supports_op(mem, op))
> +		return ret;

You should only test that in the SW-based polling path. The driver
->poll_status() method is expected to check the operation and
return -EOPNOTSUPP if HW-based polling doesn't work for this op,
no need to check things twice.

> +
> +	if (ctlr->mem_ops && ctlr->mem_ops->poll_status) {
> +		ret = spi_mem_access_start(mem);
> +		if (ret)
> +			return ret;
> +
> +		reinit_completion(&ctlr->xfer_completion);
> +
> +		ret = ctlr->mem_ops->poll_status(mem, op, mask, match,
> +						 timeout_ms);
> +
> +		ms = wait_for_completion_timeout(&ctlr->xfer_completion,
> +						 msecs_to_jiffies(timeout_ms));

Why do you need to wait here? I'd expect the poll_status to take care
of this wait.

> +
> +		spi_mem_access_end(mem);
> +		if (!ms)
> +			return -ETIMEDOUT;
> +	} else {
> +		status = (u16 *)op->data.buf.in;

Hm, I don't think it's safe, for 2 reasons:

1/ op->data.buf.in might be a 1byte buffer, but you're doing a 2byte check
2/ data is in big endian in the SPI buffer, which means your check
   won't work on LE architectures.

You really need a dedicated spi_mem_read_status() function that's passed
an u16 pointer:

int spi_mem_read_status(struct spi_mem *mem,
			const struct spi_mem_op *op,
			u16 *status)
{
	const u8 *bytes = (u8 *)op->data.buf.in;
	int ret;

	ret = spi_mem_exec_op(mem, op);
	if (ret)
		return ret;

	if (op->data.nbytes > 1)
		*status = ((u16)bytes[0] << 8) | bytes[1];
	else
		*status = bytes[0];

	return 0;
}

> +		ret = read_poll_timeout(spi_mem_exec_op, exec_op_ret,
> +					((*status) & mask) == match, 20,
> +					timeout_ms * 1000, false, mem, op);
> +		if (exec_op_ret)
> +			return exec_op_ret;
> +	}
> +

I would do something like this instead:

int spi_mem_poll_status(struct spi_mem *mem,
			const struct spi_mem_op *op,
			u16 mask, u16 match, u16 timeout_ms)
{
	struct spi_controller *ctlr = mem->spi->controller;
	int ret = -EOPNOTSUPP;

	if (op->data.nbytes < 1 || op->data.nbytes > 2)
		return -EINVAL;

	if (ctlr->mem_ops && ctlr->mem_ops->poll_status) {
		ret = spi_mem_access_start(mem);
		if (ret)
			return ret;

		ret = ctlr->mem_ops->poll_status(mem, op, mask, match,
						 timeout_ms);

		spi_mem_access_end(mem);
	}


	if (ret == -EOPNOTSUPP) {
                u16 status;
		int read_status_ret;

		if (!spi_mem_supports_op(mem, op))
			return -EOPNOTSUPP;

		ret = read_poll_timeout(spi_mem_read_status, exec_op_ret,
					(read_status_ret || ((status & mask) == match), 20,
					timeout_ms * 1000, false, mem, op, &status);

		if (read_status_ret)
			return read_status_ret;
	}

	return ret;
}

> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(spi_mem_poll_status);
> +
>  static int spi_mem_probe(struct spi_device *spi)
>  {
>  	struct spi_mem_driver *memdrv = to_spi_mem_drv(spi->dev.driver);
> @@ -763,6 +833,7 @@ static int spi_mem_probe(struct spi_device *spi)
>  	if (IS_ERR_OR_NULL(mem->name))
>  		return PTR_ERR_OR_ZERO(mem->name);
>  
> +	init_completion(&ctlr->xfer_completion);
>  	spi_set_drvdata(spi, mem);
>  
>  	return memdrv->probe(mem);
> diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
> index 2b65c9edc34e..0fbf5d0a3d31 100644
> --- a/include/linux/spi/spi-mem.h
> +++ b/include/linux/spi/spi-mem.h
> @@ -250,6 +250,7 @@ static inline void *spi_mem_get_drvdata(struct spi_mem *mem)
>   *		  the currently mapped area), and the caller of
>   *		  spi_mem_dirmap_write() is responsible for calling it again in
>   *		  this case.
> + * @poll_status: poll memory device status
>   *
>   * This interface should be implemented by SPI controllers providing an
>   * high-level interface to execute SPI memory operation, which is usually the
> @@ -274,6 +275,9 @@ struct spi_controller_mem_ops {
>  			       u64 offs, size_t len, void *buf);
>  	ssize_t (*dirmap_write)(struct spi_mem_dirmap_desc *desc,
>  				u64 offs, size_t len, const void *buf);
> +	int (*poll_status)(struct spi_mem *mem,
> +			   const struct spi_mem_op *op,
> +			   u16 mask, u16 match, unsigned long timeout);
>  };
>  
>  /**
> @@ -369,6 +373,12 @@ devm_spi_mem_dirmap_create(struct device *dev, struct spi_mem *mem,
>  void devm_spi_mem_dirmap_destroy(struct device *dev,
>  				 struct spi_mem_dirmap_desc *desc);
>  
> +void spi_mem_finalize_op(struct spi_controller *ctlr);
> +
> +int spi_mem_poll_status(struct spi_mem *mem,
> +			const struct spi_mem_op *op,
> +			u16 mask, u16 match, u16 timeout);
> +
>  int spi_mem_driver_register_with_owner(struct spi_mem_driver *drv,
>  				       struct module *owner);
>  


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] spi: spi-mem: add automatic poll status functions

[Patrice CHOTARD]

Hi Boris

On 5/8/21 9:55 AM, Boris Brezillon wrote:
> On Fri, 7 May 2021 15:17:54 +0200
> <patrice.chotard@foss.st.com> wrote:
> 
>> From: Patrice Chotard <patrice.chotard@foss.st.com>
>>
>> With STM32 QSPI, it is possible to poll the status register of the device.
>> This could be done to offload the CPU during an operation (erase or
>> program a SPI NAND for example).
>>
>> spi_mem_poll_status API has been added to handle this feature.
>> This new function take care of the offload/non-offload cases.
>>
>> For the non-offload case, use read_poll_timeout() to poll the status in
>> order to release CPU during this phase.
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
>> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
>> ---
>> Changes in v2:
>>   - Indicates the spi_mem_poll_status() timeout unit
>>   - Use 2-byte wide status register
>>   - Add spi_mem_supports_op() call in spi_mem_poll_status()
>>   - Add completion management in spi_mem_poll_status()
>>   - Add offload/non-offload case mangement in spi_mem_poll_status()
>>   - Optimize the non-offload case by using read_poll_timeout()
>>
>>  drivers/spi/spi-mem.c       | 71 +++++++++++++++++++++++++++++++++++++
>>  include/linux/spi/spi-mem.h | 10 ++++++
>>  2 files changed, 81 insertions(+)
>>
>> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
>> index 1513553e4080..3f29c604df7d 100644
>> --- a/drivers/spi/spi-mem.c
>> +++ b/drivers/spi/spi-mem.c
>> @@ -6,6 +6,7 @@
>>   * Author: Boris Brezillon <boris.brezillon@bootlin.com>
>>   */
>>  #include <linux/dmaengine.h>
>> +#include <linux/iopoll.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/spi/spi.h>
>>  #include <linux/spi/spi-mem.h>
>> @@ -743,6 +744,75 @@ static inline struct spi_mem_driver *to_spi_mem_drv(struct device_driver *drv)
>>  	return container_of(drv, struct spi_mem_driver, spidrv.driver);
>>  }
>>  
>> +/**
>> + * spi_mem_finalize_op - report completion of spi_mem_op
>> + * @ctlr: the controller reporting completion
>> + *
>> + * Called by SPI drivers using the spi-mem spi_mem_poll_status()
>> + * implementation to notify it that the current spi_mem_op has
>> + * finished.
>> + */
>> +void spi_mem_finalize_op(struct spi_controller *ctlr)
>> +{
>> +	complete(&ctlr->xfer_completion);
>> +}
>> +EXPORT_SYMBOL_GPL(spi_mem_finalize_op);
>> +
>> +/**
>> + * spi_mem_poll_status() - Poll memory device status
>> + * @mem: SPI memory device
>> + * @op: the memory operation to execute
>> + * @mask: status bitmask to ckeck
>> + * @match: (status & mask) expected value
>> + * @timeout_ms: timeout in milliseconds
>> + *
>> + * This function send a polling status request to the controller driver
>> + *
>> + * Return: 0 in case of success, -ETIMEDOUT in case of error,
>> + *         -EOPNOTSUPP if not supported.
>> + */
>> +int spi_mem_poll_status(struct spi_mem *mem,
>> +			const struct spi_mem_op *op,
>> +			u16 mask, u16 match, u16 timeout_ms)
>> +{
>> +	struct spi_controller *ctlr = mem->spi->controller;
>> +	unsigned long ms;
>> +	int ret = -EOPNOTSUPP;
>> +	int exec_op_ret;
>> +	u16 *status;
>> +
>> +	if (!spi_mem_supports_op(mem, op))
>> +		return ret;
> 
> You should only test that in the SW-based polling path. The driver
> ->poll_status() method is expected to check the operation and
> return -EOPNOTSUPP if HW-based polling doesn't work for this op,
> no need to check things twice.

Ok, i will fix this.

> 
>> +
>> +	if (ctlr->mem_ops && ctlr->mem_ops->poll_status) {
>> +		ret = spi_mem_access_start(mem);
>> +		if (ret)
>> +			return ret;
>> +
>> +		reinit_completion(&ctlr->xfer_completion);
>> +
>> +		ret = ctlr->mem_ops->poll_status(mem, op, mask, match,
>> +						 timeout_ms);
>> +
>> +		ms = wait_for_completion_timeout(&ctlr->xfer_completion,
>> +						 msecs_to_jiffies(timeout_ms));
> 
> Why do you need to wait here? I'd expect the poll_status to take care
> of this wait.

It was a request from Mark Brown [1]. The idea is to implement
similar mechanism already used in SPI framework.

[1] https://patchwork.kernel.org/project/linux-arm-kernel/patch/20210426143934.25275-2-patrice.chotard@foss.st.com/#24140527

> 
>> +
>> +		spi_mem_access_end(mem);
>> +		if (!ms)
>> +			return -ETIMEDOUT;
>> +	} else {
>> +		status = (u16 *)op->data.buf.in;
> 
> Hm, I don't think it's safe, for 2 reasons:
> 
> 1/ op->data.buf.in might be a 1byte buffer, but you're doing a 2byte check
> 2/ data is in big endian in the SPI buffer, which means your check
>    won't work on LE architectures.
> 
> You really need a dedicated spi_mem_read_status() function that's passed
> an u16 pointer:

Yes, agree

> 
> int spi_mem_read_status(struct spi_mem *mem,
> 			const struct spi_mem_op *op,
> 			u16 *status)
> {
> 	const u8 *bytes = (u8 *)op->data.buf.in;
> 	int ret;
> 
> 	ret = spi_mem_exec_op(mem, op);
> 	if (ret)
> 		return ret;
> 
> 	if (op->data.nbytes > 1)
> 		*status = ((u16)bytes[0] << 8) | bytes[1];
> 	else
> 		*status = bytes[0];
> 
> 	return 0;
> }
> 
>> +		ret = read_poll_timeout(spi_mem_exec_op, exec_op_ret,
>> +					((*status) & mask) == match, 20,
>> +					timeout_ms * 1000, false, mem, op);
>> +		if (exec_op_ret)
>> +			return exec_op_ret;
>> +	}
>> +
> 
> I would do something like this instead:
> 
> int spi_mem_poll_status(struct spi_mem *mem,
> 			const struct spi_mem_op *op,
> 			u16 mask, u16 match, u16 timeout_ms)
> {
> 	struct spi_controller *ctlr = mem->spi->controller;
> 	int ret = -EOPNOTSUPP;
> 
> 	if (op->data.nbytes < 1 || op->data.nbytes > 2)
> 		return -EINVAL;
> 
> 	if (ctlr->mem_ops && ctlr->mem_ops->poll_status) {
> 		ret = spi_mem_access_start(mem);
> 		if (ret)
> 			return ret;
> 
> 		ret = ctlr->mem_ops->poll_status(mem, op, mask, match,
> 						 timeout_ms);
> 
> 		spi_mem_access_end(mem);
> 	}
> 
> 
> 	if (ret == -EOPNOTSUPP) {
>                 u16 status;
> 		int read_status_ret;
> 
> 		if (!spi_mem_supports_op(mem, op))
> 			return -EOPNOTSUPP;
> 
> 		ret = read_poll_timeout(spi_mem_read_status, exec_op_ret,
> 					(read_status_ret || ((status & mask) == match), 20,
> 					timeout_ms * 1000, false, mem, op, &status);
> 
> 		if (read_status_ret)
> 			return read_status_ret;
> 	}
> 
> 	return ret;
> }
> 
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(spi_mem_poll_status);
>> +
>>  static int spi_mem_probe(struct spi_device *spi)
>>  {
>>  	struct spi_mem_driver *memdrv = to_spi_mem_drv(spi->dev.driver);
>> @@ -763,6 +833,7 @@ static int spi_mem_probe(struct spi_device *spi)
>>  	if (IS_ERR_OR_NULL(mem->name))
>>  		return PTR_ERR_OR_ZERO(mem->name);
>>  
>> +	init_completion(&ctlr->xfer_completion);
>>  	spi_set_drvdata(spi, mem);
>>  
>>  	return memdrv->probe(mem);
>> diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
>> index 2b65c9edc34e..0fbf5d0a3d31 100644
>> --- a/include/linux/spi/spi-mem.h
>> +++ b/include/linux/spi/spi-mem.h
>> @@ -250,6 +250,7 @@ static inline void *spi_mem_get_drvdata(struct spi_mem *mem)
>>   *		  the currently mapped area), and the caller of
>>   *		  spi_mem_dirmap_write() is responsible for calling it again in
>>   *		  this case.
>> + * @poll_status: poll memory device status
>>   *
>>   * This interface should be implemented by SPI controllers providing an
>>   * high-level interface to execute SPI memory operation, which is usually the
>> @@ -274,6 +275,9 @@ struct spi_controller_mem_ops {
>>  			       u64 offs, size_t len, void *buf);
>>  	ssize_t (*dirmap_write)(struct spi_mem_dirmap_desc *desc,
>>  				u64 offs, size_t len, const void *buf);
>> +	int (*poll_status)(struct spi_mem *mem,
>> +			   const struct spi_mem_op *op,
>> +			   u16 mask, u16 match, unsigned long timeout);
>>  };
>>  
>>  /**
>> @@ -369,6 +373,12 @@ devm_spi_mem_dirmap_create(struct device *dev, struct spi_mem *mem,
>>  void devm_spi_mem_dirmap_destroy(struct device *dev,
>>  				 struct spi_mem_dirmap_desc *desc);
>>  
>> +void spi_mem_finalize_op(struct spi_controller *ctlr);
>> +
>> +int spi_mem_poll_status(struct spi_mem *mem,
>> +			const struct spi_mem_op *op,
>> +			u16 mask, u16 match, u16 timeout);
>> +
>>  int spi_mem_driver_register_with_owner(struct spi_mem_driver *drv,
>>  				       struct module *owner);
>>  
> 
Thanks

Patrice

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] spi: spi-mem: add automatic poll status functions

[Boris Brezillon]

On Mon, 10 May 2021 10:46:48 +0200
Patrice CHOTARD <patrice.chotard@foss.st.com> wrote:

> >   
> >> +
> >> +	if (ctlr->mem_ops && ctlr->mem_ops->poll_status) {
> >> +		ret = spi_mem_access_start(mem);
> >> +		if (ret)
> >> +			return ret;
> >> +
> >> +		reinit_completion(&ctlr->xfer_completion);
> >> +
> >> +		ret = ctlr->mem_ops->poll_status(mem, op, mask, match,
> >> +						 timeout_ms);
> >> +
> >> +		ms = wait_for_completion_timeout(&ctlr->xfer_completion,
> >> +						 msecs_to_jiffies(timeout_ms));  
> > 
> > Why do you need to wait here? I'd expect the poll_status to take care
> > of this wait.  
> 
> It was a request from Mark Brown [1]. The idea is to implement
> similar mechanism already used in SPI framework.

Well, you have to choose, either you pass a timeout to ->poll_status()
and let the driver wait for the status change (and return -ETIMEDOUT if
it didn't happen in time), or you do it here and the driver only has to
signal the core completion object. I think it's preferable to let the
driver handle the timeout though, because you don't know how the
status check will be implemented, and it's not like the
reinit_completion()+wait_for_completion_timeout() done here would
greatly simplify the drivers wait logic anyway.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] spi: spi-mem: add automatic poll status functions

[Patrice CHOTARD]

Hi Boris

On 5/10/21 11:22 AM, Boris Brezillon wrote:
> On Mon, 10 May 2021 10:46:48 +0200
> Patrice CHOTARD <patrice.chotard@foss.st.com> wrote:
> 
>>>   
>>>> +
>>>> +	if (ctlr->mem_ops && ctlr->mem_ops->poll_status) {
>>>> +		ret = spi_mem_access_start(mem);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +
>>>> +		reinit_completion(&ctlr->xfer_completion);
>>>> +
>>>> +		ret = ctlr->mem_ops->poll_status(mem, op, mask, match,
>>>> +						 timeout_ms);
>>>> +
>>>> +		ms = wait_for_completion_timeout(&ctlr->xfer_completion,
>>>> +						 msecs_to_jiffies(timeout_ms));  
>>>
>>> Why do you need to wait here? I'd expect the poll_status to take care
>>> of this wait.  
>>
>> It was a request from Mark Brown [1]. The idea is to implement
>> similar mechanism already used in SPI framework.
> 
> Well, you have to choose, either you pass a timeout to ->poll_status()
> and let the driver wait for the status change (and return -ETIMEDOUT if
> it didn't happen in time), or you do it here and the driver only has to
> signal the core completion object. I think it's preferable to let the
> driver handle the timeout though, because you don't know how the
> status check will be implemented, and it's not like the
> reinit_completion()+wait_for_completion_timeout() done here would
> greatly simplify the drivers wait logic anyway.
> 

Ok i will remove the reinit/wait_completion() as you suggested.
Thanks
Patrice

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] spi: spi-mem: add automatic poll status functions

[Boris Brezillon]

On Fri, 7 May 2021 15:17:54 +0200
<patrice.chotard@foss.st.com> wrote:

> +/**
> + * spi_mem_poll_status() - Poll memory device status
> + * @mem: SPI memory device
> + * @op: the memory operation to execute
> + * @mask: status bitmask to ckeck
> + * @match: (status & mask) expected value
> + * @timeout_ms: timeout in milliseconds
> + *
> + * This function send a polling status request to the controller driver
> + *
> + * Return: 0 in case of success, -ETIMEDOUT in case of error,
> + *         -EOPNOTSUPP if not supported.
> + */
> +int spi_mem_poll_status(struct spi_mem *mem,
> +			const struct spi_mem_op *op,
> +			u16 mask, u16 match, u16 timeout_ms)

Maybe you should pass a delay_us too, to poll the status at the right
rate in the SW-based case (can also be used by drivers if they need to
configure the polling rate). You could also add an initial_delay_us to
avoid polling the status too early: an erase operation will take longer
than a write which will take longer than a read. No need to check the
status just after issuing the command, especially if the polling is
done in SW. Those 2 arguments should also be passed to the driver.

> +{
> +	struct spi_controller *ctlr = mem->spi->controller;
> +	unsigned long ms;
> +	int ret = -EOPNOTSUPP;
> +	int exec_op_ret;
> +	u16 *status;
> +
> +	if (!spi_mem_supports_op(mem, op))
> +		return ret;
> +
> +	if (ctlr->mem_ops && ctlr->mem_ops->poll_status) {
> +		ret = spi_mem_access_start(mem);
> +		if (ret)
> +			return ret;
> +
> +		reinit_completion(&ctlr->xfer_completion);
> +
> +		ret = ctlr->mem_ops->poll_status(mem, op, mask, match,
> +						 timeout_ms);
> +
> +		ms = wait_for_completion_timeout(&ctlr->xfer_completion,
> +						 msecs_to_jiffies(timeout_ms));
> +
> +		spi_mem_access_end(mem);
> +		if (!ms)
> +			return -ETIMEDOUT;
> +	} else {
> +		status = (u16 *)op->data.buf.in;
> +		ret = read_poll_timeout(spi_mem_exec_op, exec_op_ret,
> +					((*status) & mask) == match, 20,
> +					timeout_ms * 1000, false, mem, op);
> +		if (exec_op_ret)
> +			return exec_op_ret;
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(spi_mem_poll_status);
> +

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] spi: spi-mem: add automatic poll status functions

[Patrice CHOTARD]

Hi Boris

On 5/17/21 9:41 AM, Boris Brezillon wrote:
> On Fri, 7 May 2021 15:17:54 +0200
> <patrice.chotard@foss.st.com> wrote:
> 
>> +/**
>> + * spi_mem_poll_status() - Poll memory device status
>> + * @mem: SPI memory device
>> + * @op: the memory operation to execute
>> + * @mask: status bitmask to ckeck
>> + * @match: (status & mask) expected value
>> + * @timeout_ms: timeout in milliseconds
>> + *
>> + * This function send a polling status request to the controller driver
>> + *
>> + * Return: 0 in case of success, -ETIMEDOUT in case of error,
>> + *         -EOPNOTSUPP if not supported.
>> + */
>> +int spi_mem_poll_status(struct spi_mem *mem,
>> +			const struct spi_mem_op *op,
>> +			u16 mask, u16 match, u16 timeout_ms)
> 
> Maybe you should pass a delay_us too, to poll the status at the right
> rate in the SW-based case (can also be used by drivers if they need to

Ok, i will add a polling_rate_us parameter to poll_status() callback,
even if in STM32 driver case we will not use it, i agree it should be useful 
depending of driver's implementation.

> configure the polling rate). You could also add an initial_delay_us to
> avoid polling the status too early: an erase operation will take longer
> than a write which will take longer than a read. No need to check the
> status just after issuing the command, especially if the polling is
> done in SW. Those 2 arguments should also be passed to the driver.

Regarding the addition of an initial_delay_us. We got two solution:
  - use the same polling rate already used by read_poll_timeout() and 
    set read_poll_timeout()'s sleep_before_read parameter to true (in our case 20 us
    will be used as initial delay and as polling rate).

  - add an udelay(initial_delay_us) or even better usleep_range(initial_delay_us,
    initial_delay_us + delta) before calling read_poll_timeout().

I imagine you prefer the second solution ?

By adding polling_rate_us and initial_delay_us parameters to 
spi_mem_poll_status(), it implies to update all spinand_wait() calls for 
different operations (reset, read page, write page, erase) with respective  
initial_delay_us/polling_rate_us values for spi_mem_poll_status()'s parameters.

Can you provide adequate initial_delay_us and polling rate_us for each operation type ?.

Thanks
Patrice
> 
>> +{
>> +	struct spi_controller *ctlr = mem->spi->controller;
>> +	unsigned long ms;
>> +	int ret = -EOPNOTSUPP;
>> +	int exec_op_ret;
>> +	u16 *status;
>> +
>> +	if (!spi_mem_supports_op(mem, op))
>> +		return ret;
>> +
>> +	if (ctlr->mem_ops && ctlr->mem_ops->poll_status) {
>> +		ret = spi_mem_access_start(mem);
>> +		if (ret)
>> +			return ret;
>> +
>> +		reinit_completion(&ctlr->xfer_completion);
>> +
>> +		ret = ctlr->mem_ops->poll_status(mem, op, mask, match,
>> +						 timeout_ms);
>> +
>> +		ms = wait_for_completion_timeout(&ctlr->xfer_completion,
>> +						 msecs_to_jiffies(timeout_ms));
>> +
>> +		spi_mem_access_end(mem);
>> +		if (!ms)
>> +			return -ETIMEDOUT;
>> +	} else {
>> +		status = (u16 *)op->data.buf.in;
>> +		ret = read_poll_timeout(spi_mem_exec_op, exec_op_ret,
>> +					((*status) & mask) == match, 20,
>> +					timeout_ms * 1000, false, mem, op);
>> +		if (exec_op_ret)
>> +			return exec_op_ret;
>> +	}
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(spi_mem_poll_status);
>> +

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] spi: spi-mem: add automatic poll status functions

[Boris Brezillon]

On Mon, 17 May 2021 11:24:25 +0200
Patrice CHOTARD <patrice.chotard@foss.st.com> wrote:

> Hi Boris
> 
> On 5/17/21 9:41 AM, Boris Brezillon wrote:
> > On Fri, 7 May 2021 15:17:54 +0200
> > <patrice.chotard@foss.st.com> wrote:
> >   
> >> +/**
> >> + * spi_mem_poll_status() - Poll memory device status
> >> + * @mem: SPI memory device
> >> + * @op: the memory operation to execute
> >> + * @mask: status bitmask to ckeck
> >> + * @match: (status & mask) expected value
> >> + * @timeout_ms: timeout in milliseconds
> >> + *
> >> + * This function send a polling status request to the controller driver
> >> + *
> >> + * Return: 0 in case of success, -ETIMEDOUT in case of error,
> >> + *         -EOPNOTSUPP if not supported.
> >> + */
> >> +int spi_mem_poll_status(struct spi_mem *mem,
> >> +			const struct spi_mem_op *op,
> >> +			u16 mask, u16 match, u16 timeout_ms)  
> > 
> > Maybe you should pass a delay_us too, to poll the status at the right
> > rate in the SW-based case (can also be used by drivers if they need to  
> 
> Ok, i will add a polling_rate_us parameter to poll_status() callback,
> even if in STM32 driver case we will not use it, i agree it should be useful 
> depending of driver's implementation.
> 
> > configure the polling rate). You could also add an initial_delay_us to
> > avoid polling the status too early: an erase operation will take longer
> > than a write which will take longer than a read. No need to check the
> > status just after issuing the command, especially if the polling is
> > done in SW. Those 2 arguments should also be passed to the driver.  
> 
> Regarding the addition of an initial_delay_us. We got two solution:
>   - use the same polling rate already used by read_poll_timeout() and 
>     set read_poll_timeout()'s sleep_before_read parameter to true (in our case 20 us
>     will be used as initial delay and as polling rate).
> 
>   - add an udelay(initial_delay_us) or even better usleep_range(initial_delay_us,
>     initial_delay_us + delta) before calling read_poll_timeout().
> 
> I imagine you prefer the second solution ?

Yep, you might want to use udelay() when the delay is small and
usleep_range() otherwise.

> 
> By adding polling_rate_us and initial_delay_us parameters to 
> spi_mem_poll_status(), it implies to update all spinand_wait() calls for 
> different operations (reset, read page, write page, erase) with respective  
> initial_delay_us/polling_rate_us values for spi_mem_poll_status()'s parameters.
> 
> Can you provide adequate initial_delay_us and polling rate_us for each operation type ?.

If I refer to the datasheets I have,

tBERS (erase) 1ms to 4ms
tPROG 300us to 400us
tREAD 25us to 100us

Let's assume we want to minimize the latency, I'd recommend dividing
the min value by 4 for the initial delay, and dividing it by 20 for the
poll delay, which gives:

ERASE -> initial_delay = 250us, poll_delay = 50us
PROG -> initial_delay = 100us, poll_delay = 20us
READ -> initial_delay = 6us, poll_delay = 5us

Of course, that'd be even better if we were able to extract this
information from the NAND ID (or ONFI table), but I guess we can live
with those optimistic values in the meantime.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] spi: spi-mem: add automatic poll status functions

[Patrice CHOTARD]

Hi 

On 5/17/21 1:25 PM, Boris Brezillon wrote:
> On Mon, 17 May 2021 11:24:25 +0200
> Patrice CHOTARD <patrice.chotard@foss.st.com> wrote:
> 
>> Hi Boris
>>
>> On 5/17/21 9:41 AM, Boris Brezillon wrote:
>>> On Fri, 7 May 2021 15:17:54 +0200
>>> <patrice.chotard@foss.st.com> wrote:
>>>   
>>>> +/**
>>>> + * spi_mem_poll_status() - Poll memory device status
>>>> + * @mem: SPI memory device
>>>> + * @op: the memory operation to execute
>>>> + * @mask: status bitmask to ckeck
>>>> + * @match: (status & mask) expected value
>>>> + * @timeout_ms: timeout in milliseconds
>>>> + *
>>>> + * This function send a polling status request to the controller driver
>>>> + *
>>>> + * Return: 0 in case of success, -ETIMEDOUT in case of error,
>>>> + *         -EOPNOTSUPP if not supported.
>>>> + */
>>>> +int spi_mem_poll_status(struct spi_mem *mem,
>>>> +			const struct spi_mem_op *op,
>>>> +			u16 mask, u16 match, u16 timeout_ms)  
>>>
>>> Maybe you should pass a delay_us too, to poll the status at the right
>>> rate in the SW-based case (can also be used by drivers if they need to  
>>
>> Ok, i will add a polling_rate_us parameter to poll_status() callback,
>> even if in STM32 driver case we will not use it, i agree it should be useful 
>> depending of driver's implementation.
>>
>>> configure the polling rate). You could also add an initial_delay_us to
>>> avoid polling the status too early: an erase operation will take longer
>>> than a write which will take longer than a read. No need to check the
>>> status just after issuing the command, especially if the polling is
>>> done in SW. Those 2 arguments should also be passed to the driver.  
>>
>> Regarding the addition of an initial_delay_us. We got two solution:
>>   - use the same polling rate already used by read_poll_timeout() and 
>>     set read_poll_timeout()'s sleep_before_read parameter to true (in our case 20 us
>>     will be used as initial delay and as polling rate).
>>
>>   - add an udelay(initial_delay_us) or even better usleep_range(initial_delay_us,
>>     initial_delay_us + delta) before calling read_poll_timeout().
>>
>> I imagine you prefer the second solution ?
> 
> Yep, you might want to use udelay() when the delay is small and
> usleep_range() otherwise.
> 
>>
>> By adding polling_rate_us and initial_delay_us parameters to 
>> spi_mem_poll_status(), it implies to update all spinand_wait() calls for 
>> different operations (reset, read page, write page, erase) with respective  
>> initial_delay_us/polling_rate_us values for spi_mem_poll_status()'s parameters.
>>
>> Can you provide adequate initial_delay_us and polling rate_us for each operation type ?.
> 
> If I refer to the datasheets I have,
> 
> tBERS (erase) 1ms to 4ms
> tPROG 300us to 400us
> tREAD 25us to 100us
> 
> Let's assume we want to minimize the latency, I'd recommend dividing
> the min value by 4 for the initial delay, and dividing it by 20 for the
> poll delay, which gives:
> 
> ERASE -> initial_delay = 250us, poll_delay = 50us
> PROG -> initial_delay = 100us, poll_delay = 20us
> READ -> initial_delay = 6us, poll_delay = 5us


What about RESET ? we also need an initial and poll delay too (see spinand_reset_op() )

> 
> Of course, that'd be even better if we were able to extract this
> information from the NAND ID (or ONFI table), but I guess we can live
> with those optimistic values in the meantime.
> 

Thanks
Patrice

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] spi: spi-mem: add automatic poll status functions

[Boris Brezillon]

On Mon, 17 May 2021 13:59:54 +0200
Patrice CHOTARD <patrice.chotard@foss.st.com> wrote:

> Hi 
> 
> On 5/17/21 1:25 PM, Boris Brezillon wrote:
> > On Mon, 17 May 2021 11:24:25 +0200
> > Patrice CHOTARD <patrice.chotard@foss.st.com> wrote:
> >   
> >> Hi Boris
> >>
> >> On 5/17/21 9:41 AM, Boris Brezillon wrote:  
> >>> On Fri, 7 May 2021 15:17:54 +0200
> >>> <patrice.chotard@foss.st.com> wrote:
> >>>     
> >>>> +/**
> >>>> + * spi_mem_poll_status() - Poll memory device status
> >>>> + * @mem: SPI memory device
> >>>> + * @op: the memory operation to execute
> >>>> + * @mask: status bitmask to ckeck
> >>>> + * @match: (status & mask) expected value
> >>>> + * @timeout_ms: timeout in milliseconds
> >>>> + *
> >>>> + * This function send a polling status request to the controller driver
> >>>> + *
> >>>> + * Return: 0 in case of success, -ETIMEDOUT in case of error,
> >>>> + *         -EOPNOTSUPP if not supported.
> >>>> + */
> >>>> +int spi_mem_poll_status(struct spi_mem *mem,
> >>>> +			const struct spi_mem_op *op,
> >>>> +			u16 mask, u16 match, u16 timeout_ms)    
> >>>
> >>> Maybe you should pass a delay_us too, to poll the status at the right
> >>> rate in the SW-based case (can also be used by drivers if they need to    
> >>
> >> Ok, i will add a polling_rate_us parameter to poll_status() callback,
> >> even if in STM32 driver case we will not use it, i agree it should be useful 
> >> depending of driver's implementation.
> >>  
> >>> configure the polling rate). You could also add an initial_delay_us to
> >>> avoid polling the status too early: an erase operation will take longer
> >>> than a write which will take longer than a read. No need to check the
> >>> status just after issuing the command, especially if the polling is
> >>> done in SW. Those 2 arguments should also be passed to the driver.    
> >>
> >> Regarding the addition of an initial_delay_us. We got two solution:
> >>   - use the same polling rate already used by read_poll_timeout() and 
> >>     set read_poll_timeout()'s sleep_before_read parameter to true (in our case 20 us
> >>     will be used as initial delay and as polling rate).
> >>
> >>   - add an udelay(initial_delay_us) or even better usleep_range(initial_delay_us,
> >>     initial_delay_us + delta) before calling read_poll_timeout().
> >>
> >> I imagine you prefer the second solution ?  
> > 
> > Yep, you might want to use udelay() when the delay is small and
> > usleep_range() otherwise.
> >   
> >>
> >> By adding polling_rate_us and initial_delay_us parameters to 
> >> spi_mem_poll_status(), it implies to update all spinand_wait() calls for 
> >> different operations (reset, read page, write page, erase) with respective  
> >> initial_delay_us/polling_rate_us values for spi_mem_poll_status()'s parameters.
> >>
> >> Can you provide adequate initial_delay_us and polling rate_us for each operation type ?.  
> > 
> > If I refer to the datasheets I have,
> > 
> > tBERS (erase) 1ms to 4ms
> > tPROG 300us to 400us
> > tREAD 25us to 100us
> > 
> > Let's assume we want to minimize the latency, I'd recommend dividing
> > the min value by 4 for the initial delay, and dividing it by 20 for the
> > poll delay, which gives:
> > 
> > ERASE -> initial_delay = 250us, poll_delay = 50us
> > PROG -> initial_delay = 100us, poll_delay = 20us
> > READ -> initial_delay = 6us, poll_delay = 5us  
> 
> 
> What about RESET ? we also need an initial and poll delay too (see spinand_reset_op() )

5us/10us/500us if the device is respectively
reading/programming/erasing when the RESET occurs. Since we always
issue a RESET when the device is IDLE, I'd recommend going for 5us for
both the initial_delay and poll_delay.

> 
> > 
> > Of course, that'd be even better if we were able to extract this
> > information from the NAND ID (or ONFI table), but I guess we can live
> > with those optimistic values in the meantime.
> >   
> 
> Thanks
> Patrice


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] spi: spi-mem: add automatic poll status functions

[Patrice CHOTARD]

On 5/17/21 1:25 PM, Boris Brezillon wrote:
> On Mon, 17 May 2021 11:24:25 +0200
> Patrice CHOTARD <patrice.chotard@foss.st.com> wrote:
> 
>> Hi Boris
>>
>> On 5/17/21 9:41 AM, Boris Brezillon wrote:
>>> On Fri, 7 May 2021 15:17:54 +0200
>>> <patrice.chotard@foss.st.com> wrote:
>>>   
>>>> +/**
>>>> + * spi_mem_poll_status() - Poll memory device status
>>>> + * @mem: SPI memory device
>>>> + * @op: the memory operation to execute
>>>> + * @mask: status bitmask to ckeck
>>>> + * @match: (status & mask) expected value
>>>> + * @timeout_ms: timeout in milliseconds
>>>> + *
>>>> + * This function send a polling status request to the controller driver
>>>> + *
>>>> + * Return: 0 in case of success, -ETIMEDOUT in case of error,
>>>> + *         -EOPNOTSUPP if not supported.
>>>> + */
>>>> +int spi_mem_poll_status(struct spi_mem *mem,
>>>> +			const struct spi_mem_op *op,
>>>> +			u16 mask, u16 match, u16 timeout_ms)  
>>>
>>> Maybe you should pass a delay_us too, to poll the status at the right
>>> rate in the SW-based case (can also be used by drivers if they need to  
>>
>> Ok, i will add a polling_rate_us parameter to poll_status() callback,
>> even if in STM32 driver case we will not use it, i agree it should be useful 
>> depending of driver's implementation.
>>
>>> configure the polling rate). You could also add an initial_delay_us to
>>> avoid polling the status too early: an erase operation will take longer
>>> than a write which will take longer than a read. No need to check the
>>> status just after issuing the command, especially if the polling is
>>> done in SW. Those 2 arguments should also be passed to the driver.  
>>
>> Regarding the addition of an initial_delay_us. We got two solution:
>>   - use the same polling rate already used by read_poll_timeout() and 
>>     set read_poll_timeout()'s sleep_before_read parameter to true (in our case 20 us
>>     will be used as initial delay and as polling rate).
>>
>>   - add an udelay(initial_delay_us) or even better usleep_range(initial_delay_us,
>>     initial_delay_us + delta) before calling read_poll_timeout().
>>
>> I imagine you prefer the second solution ?
> 
> Yep, you might want to use udelay() when the delay is small and
> usleep_range() otherwise.
> 
>>
>> By adding polling_rate_us and initial_delay_us parameters to 
>> spi_mem_poll_status(), it implies to update all spinand_wait() calls for 
>> different operations (reset, read page, write page, erase) with respective  
>> initial_delay_us/polling_rate_us values for spi_mem_poll_status()'s parameters.
>>
>> Can you provide adequate initial_delay_us and polling rate_us for each operation type ?.
> 
> If I refer to the datasheets I have,
> 
> tBERS (erase) 1ms to 4ms
> tPROG 300us to 400us
> tREAD 25us to 100us
> 
> Let's assume we want to minimize the latency, I'd recommend dividing
> the min value by 4 for the initial delay, and dividing it by 20 for the
> poll delay, which gives:
> 
> ERASE -> initial_delay = 250us, poll_delay = 50us
> PROG -> initial_delay = 100us, poll_delay = 20us

another remark, it should be:  PROG -> initial_delay = 75 us (300 / 4) , poll_delay = 15us ( 300 / 20)

Patrice

> READ -> initial_delay = 6us, poll_delay = 5us
> 
> Of course, that'd be even better if we were able to extract this
> information from the NAND ID (or ONFI table), but I guess we can live
> with those optimistic values in the meantime.
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/3] mtd: spinand: use the spi-mem poll status APIs

[patrice.chotard]

From: Patrice Chotard <patrice.chotard@foss.st.com>

Make use of spi-mem poll status APIs to let advanced controllers
optimize wait operations.

Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
---
Changes in v2:
  - non-offload case is now managed by spi_mem_poll_status()

 drivers/mtd/nand/spi/core.c | 17 +++++++++--------
 include/linux/mtd/spinand.h |  1 +
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 17f63f95f4a2..56f81c7a73a6 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -475,18 +475,19 @@ static int spinand_erase_op(struct spinand_device *spinand,
 
 static int spinand_wait(struct spinand_device *spinand, u8 *s)
 {
-	unsigned long timeo =  jiffies + msecs_to_jiffies(400);
+	struct spi_mem_op op = SPINAND_GET_FEATURE_OP(REG_STATUS,
+						      spinand->scratchbuf);
 	u8 status;
 	int ret;
 
-	do {
-		ret = spinand_read_status(spinand, &status);
-		if (ret)
-			return ret;
+	ret = spi_mem_poll_status(spinand->spimem, &op, STATUS_BUSY, 0,
+				  SPINAND_STATUS_TIMEOUT_MS);
+	if (ret)
+		return ret;
 
-		if (!(status & STATUS_BUSY))
-			goto out;
-	} while (time_before(jiffies, timeo));
+	status = *spinand->scratchbuf;
+	if (!(status & STATUS_BUSY))
+		goto out;
 
 	/*
 	 * Extra read, just in case the STATUS_READY bit has changed
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index 6bb92f26833e..28ee481d96eb 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -170,6 +170,7 @@ struct spinand_op;
 struct spinand_device;
 
 #define SPINAND_MAX_ID_LEN	4
+#define SPINAND_STATUS_TIMEOUT_MS	400
 
 /**
  * struct spinand_id - SPI NAND id structure
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/3] spi: stm32-qspi: add automatic poll status feature

[patrice.chotard]

From: Patrice Chotard <patrice.chotard@foss.st.com>

STM32 QSPI is able to automatically poll a specified register inside the
memory and relieve the CPU from this task.

As example, when erasing a large memory area, we got cpu load
equal to 50%. This patch allows to perform the same operation
with a cpu load around 2%.

Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
---
Changes in v2:
  - mask and match stm32_qspi_poll_status() parameters are 2-byte wide
  - Make usage of new spi_mem_finalize_op() API in stm32_qspi_wait_poll_status() 

 drivers/spi/spi-stm32-qspi.c | 81 ++++++++++++++++++++++++++++++++----
 1 file changed, 73 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-stm32-qspi.c b/drivers/spi/spi-stm32-qspi.c
index 7e640ccc7e77..3cb1436ff393 100644
--- a/drivers/spi/spi-stm32-qspi.c
+++ b/drivers/spi/spi-stm32-qspi.c
@@ -36,6 +36,7 @@
 #define CR_FTIE			BIT(18)
 #define CR_SMIE			BIT(19)
 #define CR_TOIE			BIT(20)
+#define CR_APMS			BIT(22)
 #define CR_PRESC_MASK		GENMASK(31, 24)
 
 #define QSPI_DCR		0x04
@@ -53,6 +54,7 @@
 #define QSPI_FCR		0x0c
 #define FCR_CTEF		BIT(0)
 #define FCR_CTCF		BIT(1)
+#define FCR_CSMF		BIT(3)
 
 #define QSPI_DLR		0x10
 
@@ -107,6 +109,7 @@ struct stm32_qspi {
 	u32 clk_rate;
 	struct stm32_qspi_flash flash[STM32_QSPI_MAX_NORCHIP];
 	struct completion data_completion;
+	struct completion match_completion;
 	u32 fmode;
 
 	struct dma_chan *dma_chtx;
@@ -115,6 +118,7 @@ struct stm32_qspi {
 
 	u32 cr_reg;
 	u32 dcr_reg;
+	unsigned long status_timeout;
 
 	/*
 	 * to protect device configuration, could be different between
@@ -128,11 +132,20 @@ static irqreturn_t stm32_qspi_irq(int irq, void *dev_id)
 	struct stm32_qspi *qspi = (struct stm32_qspi *)dev_id;
 	u32 cr, sr;
 
+	cr = readl_relaxed(qspi->io_base + QSPI_CR);
 	sr = readl_relaxed(qspi->io_base + QSPI_SR);
 
+	if (cr & CR_SMIE && sr & SR_SMF) {
+		/* disable irq */
+		cr &= ~CR_SMIE;
+		writel_relaxed(cr, qspi->io_base + QSPI_CR);
+		complete(&qspi->match_completion);
+
+		return IRQ_HANDLED;
+	}
+
 	if (sr & (SR_TEF | SR_TCF)) {
 		/* disable irq */
-		cr = readl_relaxed(qspi->io_base + QSPI_CR);
 		cr &= ~CR_TCIE & ~CR_TEIE;
 		writel_relaxed(cr, qspi->io_base + QSPI_CR);
 		complete(&qspi->data_completion);
@@ -319,6 +332,25 @@ static int stm32_qspi_wait_cmd(struct stm32_qspi *qspi,
 	return err;
 }
 
+static int stm32_qspi_wait_poll_status(struct stm32_qspi *qspi,
+				       const struct spi_mem_op *op)
+{
+	u32 cr;
+
+	reinit_completion(&qspi->match_completion);
+	cr = readl_relaxed(qspi->io_base + QSPI_CR);
+	writel_relaxed(cr | CR_SMIE, qspi->io_base + QSPI_CR);
+
+	if (!wait_for_completion_timeout(&qspi->match_completion,
+				msecs_to_jiffies(qspi->status_timeout)))
+		return -ETIMEDOUT;
+
+	writel_relaxed(FCR_CSMF, qspi->io_base + QSPI_FCR);
+	spi_mem_finalize_op(qspi->ctrl);
+
+	return 0;
+}
+
 static int stm32_qspi_get_mode(struct stm32_qspi *qspi, u8 buswidth)
 {
 	if (buswidth == 4)
@@ -332,7 +364,7 @@ static int stm32_qspi_send(struct spi_mem *mem, const struct spi_mem_op *op)
 	struct stm32_qspi *qspi = spi_controller_get_devdata(mem->spi->master);
 	struct stm32_qspi_flash *flash = &qspi->flash[mem->spi->chip_select];
 	u32 ccr, cr;
-	int timeout, err = 0;
+	int timeout, err = 0, err_poll_status = 0;
 
 	dev_dbg(qspi->dev, "cmd:%#x mode:%d.%d.%d.%d addr:%#llx len:%#x\n",
 		op->cmd.opcode, op->cmd.buswidth, op->addr.buswidth,
@@ -378,6 +410,9 @@ static int stm32_qspi_send(struct spi_mem *mem, const struct spi_mem_op *op)
 	if (op->addr.nbytes && qspi->fmode != CCR_FMODE_MM)
 		writel_relaxed(op->addr.val, qspi->io_base + QSPI_AR);
 
+	if (qspi->fmode == CCR_FMODE_APM)
+		err_poll_status = stm32_qspi_wait_poll_status(qspi, op);
+
 	err = stm32_qspi_tx(qspi, op);
 
 	/*
@@ -387,7 +422,7 @@ static int stm32_qspi_send(struct spi_mem *mem, const struct spi_mem_op *op)
 	 *  byte of device (device size - fifo size). like device size is not
 	 *  knows, the prefetching is always stop.
 	 */
-	if (err || qspi->fmode == CCR_FMODE_MM)
+	if (err || err_poll_status || qspi->fmode == CCR_FMODE_MM)
 		goto abort;
 
 	/* wait end of tx in indirect mode */
@@ -406,15 +441,43 @@ static int stm32_qspi_send(struct spi_mem *mem, const struct spi_mem_op *op)
 						    cr, !(cr & CR_ABORT), 1,
 						    STM32_ABT_TIMEOUT_US);
 
-	writel_relaxed(FCR_CTCF, qspi->io_base + QSPI_FCR);
+	writel_relaxed(FCR_CTCF | FCR_CSMF, qspi->io_base + QSPI_FCR);
 
-	if (err || timeout)
-		dev_err(qspi->dev, "%s err:%d abort timeout:%d\n",
-			__func__, err, timeout);
+	if (err || err_poll_status || timeout)
+		dev_err(qspi->dev, "%s err:%d err_poll_status:%d abort timeout:%d\n",
+			__func__, err, err_poll_status, timeout);
 
 	return err;
 }
 
+static int stm32_qspi_poll_status(struct spi_mem *mem, const struct spi_mem_op *op,
+				  u16 mask, u16 match, unsigned long timeout_ms)
+{
+	struct stm32_qspi *qspi = spi_controller_get_devdata(mem->spi->master);
+	int ret;
+
+	ret = pm_runtime_get_sync(qspi->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(qspi->dev);
+		return ret;
+	}
+
+	mutex_lock(&qspi->lock);
+
+	writel_relaxed(mask, qspi->io_base + QSPI_PSMKR);
+	writel_relaxed(match, qspi->io_base + QSPI_PSMAR);
+	qspi->fmode = CCR_FMODE_APM;
+	qspi->status_timeout = timeout_ms;
+
+	ret = stm32_qspi_send(mem, op);
+	mutex_unlock(&qspi->lock);
+
+	pm_runtime_mark_last_busy(qspi->dev);
+	pm_runtime_put_autosuspend(qspi->dev);
+
+	return ret;
+}
+
 static int stm32_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 {
 	struct stm32_qspi *qspi = spi_controller_get_devdata(mem->spi->master);
@@ -527,7 +590,7 @@ static int stm32_qspi_setup(struct spi_device *spi)
 	flash->presc = presc;
 
 	mutex_lock(&qspi->lock);
-	qspi->cr_reg = 3 << CR_FTHRES_SHIFT | CR_SSHIFT | CR_EN;
+	qspi->cr_reg = CR_APMS | 3 << CR_FTHRES_SHIFT | CR_SSHIFT | CR_EN;
 	writel_relaxed(qspi->cr_reg, qspi->io_base + QSPI_CR);
 
 	/* set dcr fsize to max address */
@@ -607,6 +670,7 @@ static const struct spi_controller_mem_ops stm32_qspi_mem_ops = {
 	.exec_op	= stm32_qspi_exec_op,
 	.dirmap_create	= stm32_qspi_dirmap_create,
 	.dirmap_read	= stm32_qspi_dirmap_read,
+	.poll_status	= stm32_qspi_poll_status,
 };
 
 static int stm32_qspi_probe(struct platform_device *pdev)
@@ -661,6 +725,7 @@ static int stm32_qspi_probe(struct platform_device *pdev)
 	}
 
 	init_completion(&qspi->data_completion);
+	init_completion(&qspi->match_completion);
 
 	qspi->clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(qspi->clk)) {
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/3] spi: stm32-qspi: add automatic poll status feature

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2651 bytes --]

Hi,

I love your patch! Yet something to improve:

[auto build test ERROR on spi/for-next]
[also build test ERROR on next-20210507]
[cannot apply to mtd/nand/next stm32/stm32-next v5.12]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/patrice-chotard-foss-st-com/MTD-spinand-Add-spi_mem_poll_status-support/20210507-211933
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: arc-randconfig-r021-20210507 (attached as .config)
compiler: arc-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/7f27dc0ce5a74374bac3b33a0e77098ad6465ab8
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review patrice-chotard-foss-st-com/MTD-spinand-Add-spi_mem_poll_status-support/20210507-211933
        git checkout 7f27dc0ce5a74374bac3b33a0e77098ad6465ab8
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=arc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arc-elf-ld: lib/stackdepot.o: in function `filter_irq_stacks':
   stackdepot.c:(.text+0x3d2): undefined reference to `__irqentry_text_start'
   arc-elf-ld: stackdepot.c:(.text+0x3d2): undefined reference to `__irqentry_text_start'
   arc-elf-ld: stackdepot.c:(.text+0x3da): undefined reference to `__irqentry_text_end'
   arc-elf-ld: stackdepot.c:(.text+0x3da): undefined reference to `__irqentry_text_end'
   arc-elf-ld: stackdepot.c:(.text+0x3e2): undefined reference to `__softirqentry_text_start'
   arc-elf-ld: stackdepot.c:(.text+0x3e2): undefined reference to `__softirqentry_text_start'
   arc-elf-ld: stackdepot.c:(.text+0x3ea): undefined reference to `__softirqentry_text_end'
   arc-elf-ld: stackdepot.c:(.text+0x3ea): undefined reference to `__softirqentry_text_end'
   arc-elf-ld: drivers/spi/spi-stm32-qspi.o: in function `stm32_qspi_send.isra.0':
   spi-stm32-qspi.c:(.text+0x39a): undefined reference to `spi_mem_finalize_op'
>> arc-elf-ld: spi-stm32-qspi.c:(.text+0x39a): undefined reference to `spi_mem_finalize_op'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31475 bytes --]

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/3] spi: stm32-qspi: add automatic poll status feature

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2185 bytes --]

Hi,

I love your patch! Yet something to improve:

[auto build test ERROR on spi/for-next]
[also build test ERROR on next-20210507]
[cannot apply to mtd/nand/next stm32/stm32-next v5.12]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/patrice-chotard-foss-st-com/MTD-spinand-Add-spi_mem_poll_status-support/20210507-211933
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: nios2-randconfig-s032-20210507 (attached as .config)
compiler: nios2-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/7f27dc0ce5a74374bac3b33a0e77098ad6465ab8
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review patrice-chotard-foss-st-com/MTD-spinand-Add-spi_mem_poll_status-support/20210507-211933
        git checkout 7f27dc0ce5a74374bac3b33a0e77098ad6465ab8
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' W=1 ARCH=nios2 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   nios2-linux-ld: drivers/spi/spi-stm32-qspi.o: in function `stm32_qspi_send.isra.0':
>> spi-stm32-qspi.c:(.text+0x4e0): undefined reference to `spi_mem_finalize_op'
   spi-stm32-qspi.c:(.text+0x4e0): relocation truncated to fit: R_NIOS2_CALL26 against `spi_mem_finalize_op'

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for SERIAL_CORE_CONSOLE
   Depends on TTY && HAS_IOMEM
   Selected by
   - EARLY_PRINTK

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34960 bytes --]

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel