From: Patrice Chotard <[email protected]>
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
From: Patrice Chotard <[email protected]>
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 <[email protected]>
Signed-off-by: Christophe Kerello <[email protected]>
---
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 <[email protected]>
*/
#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
On Fri, 7 May 2021 15:17:54 +0200
<[email protected]> wrote:
> From: Patrice Chotard <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Christophe Kerello <[email protected]>
> ---
> 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 <[email protected]>
> */
> #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);
>
Hi Boris
On 5/8/21 9:55 AM, Boris Brezillon wrote:
> On Fri, 7 May 2021 15:17:54 +0200
> <[email protected]> wrote:
>
>> From: Patrice Chotard <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Christophe Kerello <[email protected]>
>> ---
>> 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 <[email protected]>
>> */
>> #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/[email protected]/#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
On Mon, 10 May 2021 10:46:48 +0200
Patrice CHOTARD <[email protected]> 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.
Hi Boris
On 5/10/21 11:22 AM, Boris Brezillon wrote:
> On Mon, 10 May 2021 10:46:48 +0200
> Patrice CHOTARD <[email protected]> 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
On Fri, 7 May 2021 15:17:54 +0200
<[email protected]> 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);
> +
Hi Boris
On 5/17/21 9:41 AM, Boris Brezillon wrote:
> On Fri, 7 May 2021 15:17:54 +0200
> <[email protected]> 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);
>> +
On 5/17/21 1:25 PM, Boris Brezillon wrote:
> On Mon, 17 May 2021 11:24:25 +0200
> Patrice CHOTARD <[email protected]> wrote:
>
>> Hi Boris
>>
>> On 5/17/21 9:41 AM, Boris Brezillon wrote:
>>> On Fri, 7 May 2021 15:17:54 +0200
>>> <[email protected]> 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.
>
On Mon, 17 May 2021 11:24:25 +0200
Patrice CHOTARD <[email protected]> wrote:
> Hi Boris
>
> On 5/17/21 9:41 AM, Boris Brezillon wrote:
> > On Fri, 7 May 2021 15:17:54 +0200
> > <[email protected]> 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.
Hi
On 5/17/21 1:25 PM, Boris Brezillon wrote:
> On Mon, 17 May 2021 11:24:25 +0200
> Patrice CHOTARD <[email protected]> wrote:
>
>> Hi Boris
>>
>> On 5/17/21 9:41 AM, Boris Brezillon wrote:
>>> On Fri, 7 May 2021 15:17:54 +0200
>>> <[email protected]> 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
On Mon, 17 May 2021 13:59:54 +0200
Patrice CHOTARD <[email protected]> wrote:
> Hi
>
> On 5/17/21 1:25 PM, Boris Brezillon wrote:
> > On Mon, 17 May 2021 11:24:25 +0200
> > Patrice CHOTARD <[email protected]> wrote:
> >
> >> Hi Boris
> >>
> >> On 5/17/21 9:41 AM, Boris Brezillon wrote:
> >>> On Fri, 7 May 2021 15:17:54 +0200
> >>> <[email protected]> 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