| Message ID | 20210507131756.17028-1-patrice.chotard@foss.st.com |
|---|---|
| Headers | show |
| Series | MTD: spinand: Add spi_mem_poll_status() support | expand |
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); >
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
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.
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
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(-)