| Message ID | 20210426143934.25275-2-patrice.chotard@foss.st.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MTD: spinand: Add spi_mem_poll_status() support | expand |
Hi, On 26/04/21 04:39PM, patrice.chotard@foss.st.com wrote: > From: Christophe Kerello <christophe.kerello@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. > > Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com> > Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com> > --- > drivers/spi/spi-mem.c | 34 ++++++++++++++++++++++++++++++++++ > include/linux/spi/spi-mem.h | 8 ++++++++ > 2 files changed, 42 insertions(+) > > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c > index 1513553e4080..43dce4b0efa4 100644 > --- a/drivers/spi/spi-mem.c > +++ b/drivers/spi/spi-mem.c > @@ -743,6 +743,40 @@ 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_poll_status() - Poll memory device status > + * @mem: SPI memory device > + * @op: the memory operation to execute > + * @mask: status bitmask to ckeck > + * @match: status expected value Technically, (status & mask) expected value. Dunno if that is obvious enough to not spell out explicitly. > + * @timeout: timeout > + * > + * 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, > + u8 mask, u8 match, u16 timeout) > +{ > + struct spi_controller *ctlr = mem->spi->controller; > + int ret = -EOPNOTSUPP; > + > + if (ctlr->mem_ops && ctlr->mem_ops->poll_status) { You should call spi_mem_supports_op() before sending any ops to the controller. Invalid/unsupported ops can cause unexpected behavior. > + ret = spi_mem_access_start(mem); > + if (ret) > + return ret; > + > + ret = ctlr->mem_ops->poll_status(mem, op, mask, match, timeout); I wonder if it is better to let spi-mem core take care of the timeout part. On one hand it reduces code duplication on the driver side a little bit. Plus it makes sure drivers don't mess anything up with bad (or no) handling of the timeout. But on the other hand the interface becomes a bit awkward since you'd have to pass a struct completion around, and it isn't something particularly hard to get right either. What do you think? > + > + spi_mem_access_end(mem); > + } > + > + 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); > diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h > index 2b65c9edc34e..5f78917c0f68 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, > + u8 mask, u8 match, u16 timeout); > }; > > /** > @@ -369,6 +373,10 @@ 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); > > +int spi_mem_poll_status(struct spi_mem *mem, > + const struct spi_mem_op *op, > + u8 mask, u8 match, u16 timeout); > + > int spi_mem_driver_register_with_owner(struct spi_mem_driver *drv, > struct module *owner);
On Mon, Apr 26, 2021 at 09:56:12PM +0530, Pratyush Yadav wrote: > On 26/04/21 04:39PM, 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 expected value > Technically, (status & mask) expected value. Dunno if that is obvious > enough to not spell out explicitly. Is it possible there's some situation where you're waiting for some bits to clear as well? > > + ret = ctlr->mem_ops->poll_status(mem, op, mask, match, timeout); I'm not sure I like this name since it makes me think the driver is going to poll when really it's offloaded to the hardware, but I can't think of any better ideas either and it *is* what the hardware is going to be doing so meh. > I wonder if it is better to let spi-mem core take care of the timeout > part. On one hand it reduces code duplication on the driver side a > little bit. Plus it makes sure drivers don't mess anything up with bad > (or no) handling of the timeout. But on the other hand the interface > becomes a bit awkward since you'd have to pass a struct completion > around, and it isn't something particularly hard to get right either. > What do you think? We already have the core handling other timeouts. We don't pass around completions but rather have an API function that the driver has to call when the operation completes, a similar pattern might work here. Part of the thing with those APIs which I'm missing here is that this will just return -EOPNOTSUPP if the driver can't do the delay in hardware, I think it would be cleaner if this API were similar and the core dealt with doing the delay/poll on the CPU. That way the users don't need to repeat the handling for the offload/non-offload cases.
On 26/04/21 05:51PM, Mark Brown wrote: > On Mon, Apr 26, 2021 at 09:56:12PM +0530, Pratyush Yadav wrote: > > On 26/04/21 04:39PM, 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 expected value > > > Technically, (status & mask) expected value. Dunno if that is obvious > > enough to not spell out explicitly. > > Is it possible there's some situation where you're waiting for some bits > to clear as well? Yes. In fact, that is the more common situation. Both SPI NOR (spi_nor_sr_ready()) and SPI NAND (spinand_wait()) need to wait for the "busy" bit to be cleared. AFAICT this API is supposed to check for (status & mask) == (match & mask) so it should be able to handle both polarities for the bits being polled. > > > > + ret = ctlr->mem_ops->poll_status(mem, op, mask, match, timeout); > > I'm not sure I like this name since it makes me think the driver is > going to poll when really it's offloaded to the hardware, but I can't > think of any better ideas either and it *is* what the hardware is going > to be doing so meh. > > > I wonder if it is better to let spi-mem core take care of the timeout > > part. On one hand it reduces code duplication on the driver side a > > little bit. Plus it makes sure drivers don't mess anything up with bad > > (or no) handling of the timeout. But on the other hand the interface > > becomes a bit awkward since you'd have to pass a struct completion > > around, and it isn't something particularly hard to get right either. > > What do you think? > > We already have the core handling other timeouts. We don't pass around > completions but rather have an API function that the driver has to call > when the operation completes, a similar pattern might work here. Part > of the thing with those APIs which I'm missing here is that this will > just return -EOPNOTSUPP if the driver can't do the delay in hardware, I > think it would be cleaner if this API were similar and the core dealt > with doing the delay/poll on the CPU. That way the users don't need to > repeat the handling for the offload/non-offload cases. Makes sense to me.
Hi Pratyush On 4/26/21 6:26 PM, Pratyush Yadav wrote: > Hi, > > On 26/04/21 04:39PM, patrice.chotard@foss.st.com wrote: >> From: Christophe Kerello <christophe.kerello@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. >> >> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com> >> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com> >> --- >> drivers/spi/spi-mem.c | 34 ++++++++++++++++++++++++++++++++++ >> include/linux/spi/spi-mem.h | 8 ++++++++ >> 2 files changed, 42 insertions(+) >> >> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c >> index 1513553e4080..43dce4b0efa4 100644 >> --- a/drivers/spi/spi-mem.c >> +++ b/drivers/spi/spi-mem.c >> @@ -743,6 +743,40 @@ 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_poll_status() - Poll memory device status >> + * @mem: SPI memory device >> + * @op: the memory operation to execute >> + * @mask: status bitmask to ckeck >> + * @match: status expected value > > Technically, (status & mask) expected value. Dunno if that is obvious > enough to not spell out explicitly. Yes, match = (status & mask) I will update the comment accordingly. > >> + * @timeout: timeout >> + * >> + * 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, >> + u8 mask, u8 match, u16 timeout) >> +{ >> + struct spi_controller *ctlr = mem->spi->controller; >> + int ret = -EOPNOTSUPP; >> + >> + if (ctlr->mem_ops && ctlr->mem_ops->poll_status) { > > You should call spi_mem_supports_op() before sending any ops to the > controller. Invalid/unsupported ops can cause unexpected behavior. Ok i will add it. Thanks Patrice > >> + ret = spi_mem_access_start(mem); >> + if (ret) >> + return ret; >> + >> + ret = ctlr->mem_ops->poll_status(mem, op, mask, match, timeout); > > I wonder if it is better to let spi-mem core take care of the timeout > part. On one hand it reduces code duplication on the driver side a > little bit. Plus it makes sure drivers don't mess anything up with bad > (or no) handling of the timeout. But on the other hand the interface > becomes a bit awkward since you'd have to pass a struct completion > around, and it isn't something particularly hard to get right either. > What do you think? > >> + >> + spi_mem_access_end(mem); >> + } >> + >> + 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); >> diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h >> index 2b65c9edc34e..5f78917c0f68 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, >> + u8 mask, u8 match, u16 timeout); >> }; >> >> /** >> @@ -369,6 +373,10 @@ 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); >> >> +int spi_mem_poll_status(struct spi_mem *mem, >> + const struct spi_mem_op *op, >> + u8 mask, u8 match, u16 timeout); >> + >> int spi_mem_driver_register_with_owner(struct spi_mem_driver *drv, >> struct module *owner); >
Hi Mark, Pratyush On 4/26/21 6:51 PM, Mark Brown wrote: > On Mon, Apr 26, 2021 at 09:56:12PM +0530, Pratyush Yadav wrote: >> On 26/04/21 04:39PM, 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 expected value > >> Technically, (status & mask) expected value. Dunno if that is obvious >> enough to not spell out explicitly. > > Is it possible there's some situation where you're waiting for some bits > to clear as well? > Yes, we are waiting STATUS_BUSY bit to be cleared, see patch 2 which is making usage of this API. >>> + ret = ctlr->mem_ops->poll_status(mem, op, mask, match, timeout); > > I'm not sure I like this name since it makes me think the driver is > going to poll when really it's offloaded to the hardware, but I can't > think of any better ideas either and it *is* what the hardware is going > to be doing so meh. > >> I wonder if it is better to let spi-mem core take care of the timeout >> part. On one hand it reduces code duplication on the driver side a >> little bit. Plus it makes sure drivers don't mess anything up with bad >> (or no) handling of the timeout. But on the other hand the interface >> becomes a bit awkward since you'd have to pass a struct completion >> around, and it isn't something particularly hard to get right either. >> What do you think? > > We already have the core handling other timeouts. We don't pass around > completions but rather have an API function that the driver has to call > when the operation completes, a similar pattern might work here. Part So, if i correctly understood, you make allusion to what is already done in SPI core framework with spi_finalize_current_transfer() right ? > of the thing with those APIs which I'm missing here is that this will > just return -EOPNOTSUPP if the driver can't do the delay in hardware, I > think it would be cleaner if this API were similar and the core dealt > with doing the delay/poll on the CPU. That way the users don't need to > repeat the handling for the offload/non-offload cases. Sorry, i didn't catch what you mean here. In PATCH 2, that's the case, if spi_mem_poll_status() is not supported, the core is dealing with the delay/poll on the CPU in spinand_wait(). Patrice Thanks >
On Fri, Apr 30, 2021 at 04:22:34PM +0200, Patrice CHOTARD wrote: > On 4/26/21 6:51 PM, Mark Brown wrote: > > On Mon, Apr 26, 2021 at 09:56:12PM +0530, Pratyush Yadav wrote: > > Is it possible there's some situation where you're waiting for some bits > > to clear as well? > Yes, we are waiting STATUS_BUSY bit to be cleared, see patch 2 which is making > usage of this API. Then the inverse question applies - is there no circumstance where we might be waiting for a bit to be set? > > We already have the core handling other timeouts. We don't pass around > > completions but rather have an API function that the driver has to call > > when the operation completes, a similar pattern might work here. Part > So, if i correctly understood, you make allusion to what is already done > in SPI core framework with spi_finalize_current_transfer() right ? Yes, and _current_message(). > > of the thing with those APIs which I'm missing here is that this will > > just return -EOPNOTSUPP if the driver can't do the delay in hardware, I > > think it would be cleaner if this API were similar and the core dealt > > with doing the delay/poll on the CPU. That way the users don't need to > > repeat the handling for the offload/non-offload cases. > Sorry, i didn't catch what you mean here. In PATCH 2, that's the case, > if spi_mem_poll_status() is not supported, the core is dealing with > the delay/poll on the CPU in spinand_wait(). That's in the NAND core, not in spi-mem. Any other users of spi-mem will also need to open code stuff.
On Mon, 26 Apr 2021 16:39:32 +0200 <patrice.chotard@foss.st.com> wrote: > From: Christophe Kerello <christophe.kerello@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. > > Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com> > Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com> > --- > drivers/spi/spi-mem.c | 34 ++++++++++++++++++++++++++++++++++ > include/linux/spi/spi-mem.h | 8 ++++++++ > 2 files changed, 42 insertions(+) > > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c > index 1513553e4080..43dce4b0efa4 100644 > --- a/drivers/spi/spi-mem.c > +++ b/drivers/spi/spi-mem.c > @@ -743,6 +743,40 @@ 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_poll_status() - Poll memory device status > + * @mem: SPI memory device > + * @op: the memory operation to execute > + * @mask: status bitmask to ckeck > + * @match: status expected value > + * @timeout: timeout > + * > + * 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, > + u8 mask, u8 match, u16 timeout) > +{ > + struct spi_controller *ctlr = mem->spi->controller; > + int ret = -EOPNOTSUPP; > + > + if (ctlr->mem_ops && ctlr->mem_ops->poll_status) { > + ret = spi_mem_access_start(mem); You should probably check that op is a single byte read before accepting the command. > + if (ret) > + return ret; > + > + ret = ctlr->mem_ops->poll_status(mem, op, mask, match, timeout); You also need some sort of ->poll_status_is_supported() to validate that the controller supports the status polling for this specific op (I can imagine some controllers having a limit on the number of dummy cycles/address bytes). I guess you could just fall back on SW-based status polling if ctlr->mem_ops->poll_status() returns -ENOTSUPP. > + > + spi_mem_access_end(mem); > + } > + > + 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); > diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h > index 2b65c9edc34e..5f78917c0f68 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, > + u8 mask, u8 match, u16 timeout); > }; > > /** > @@ -369,6 +373,10 @@ 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); > > +int spi_mem_poll_status(struct spi_mem *mem, > + const struct spi_mem_op *op, > + u8 mask, u8 match, u16 timeout); > + > int spi_mem_driver_register_with_owner(struct spi_mem_driver *drv, > struct module *owner); >
On Mon, 26 Apr 2021 16:39:32 +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 expected value > + * @timeout: timeout What's the unit?
On 30/04/21 06:51PM, Boris Brezillon wrote: > On Mon, 26 Apr 2021 16:39:32 +0200 > <patrice.chotard@foss.st.com> wrote: > > > From: Christophe Kerello <christophe.kerello@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. > > > > Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com> > > Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com> > > --- > > drivers/spi/spi-mem.c | 34 ++++++++++++++++++++++++++++++++++ > > include/linux/spi/spi-mem.h | 8 ++++++++ > > 2 files changed, 42 insertions(+) > > > > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c > > index 1513553e4080..43dce4b0efa4 100644 > > --- a/drivers/spi/spi-mem.c > > +++ b/drivers/spi/spi-mem.c > > @@ -743,6 +743,40 @@ 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_poll_status() - Poll memory device status > > + * @mem: SPI memory device > > + * @op: the memory operation to execute > > + * @mask: status bitmask to ckeck > > + * @match: status expected value > > + * @timeout: timeout > > + * > > + * 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, > > + u8 mask, u8 match, u16 timeout) > > +{ > > + struct spi_controller *ctlr = mem->spi->controller; > > + int ret = -EOPNOTSUPP; > > + > > + if (ctlr->mem_ops && ctlr->mem_ops->poll_status) { > > + ret = spi_mem_access_start(mem); > > You should probably check that op is a single byte read before > accepting the command. Please do not discriminate against 8D-8D-8D flashes ;-) > > > + if (ret) > > + return ret; > > + > > + ret = ctlr->mem_ops->poll_status(mem, op, mask, match, timeout); > > You also need some sort of ->poll_status_is_supported() to validate > that the controller supports the status polling for this specific op (I I don't think a separate function is needed for checking if the poll status op is supported. Return value of -EOPNOTSUPP should be able to signal that. This can also be used to check if Octal DDR capable controllers are able to poll using 2-byte reads. > can imagine some controllers having a limit on the number of dummy > cycles/address bytes). I guess you could just fall back on SW-based > status polling if ctlr->mem_ops->poll_status() returns -ENOTSUPP. > > > + > > + spi_mem_access_end(mem); > > + } > > + > > + 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); > > diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h > > index 2b65c9edc34e..5f78917c0f68 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, > > + u8 mask, u8 match, u16 timeout); > > }; > > > > /** > > @@ -369,6 +373,10 @@ 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); > > > > +int spi_mem_poll_status(struct spi_mem *mem, > > + const struct spi_mem_op *op, > > + u8 mask, u8 match, u16 timeout); > > + > > int spi_mem_driver_register_with_owner(struct spi_mem_driver *drv, > > struct module *owner); > >
On Mon, 3 May 2021 14:17:44 +0530 Pratyush Yadav <p.yadav@ti.com> wrote: > On 30/04/21 06:51PM, Boris Brezillon wrote: > > On Mon, 26 Apr 2021 16:39:32 +0200 > > <patrice.chotard@foss.st.com> wrote: > > > > > From: Christophe Kerello <christophe.kerello@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. > > > > > > Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com> > > > Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com> > > > --- > > > drivers/spi/spi-mem.c | 34 ++++++++++++++++++++++++++++++++++ > > > include/linux/spi/spi-mem.h | 8 ++++++++ > > > 2 files changed, 42 insertions(+) > > > > > > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c > > > index 1513553e4080..43dce4b0efa4 100644 > > > --- a/drivers/spi/spi-mem.c > > > +++ b/drivers/spi/spi-mem.c > > > @@ -743,6 +743,40 @@ 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_poll_status() - Poll memory device status > > > + * @mem: SPI memory device > > > + * @op: the memory operation to execute > > > + * @mask: status bitmask to ckeck > > > + * @match: status expected value > > > + * @timeout: timeout > > > + * > > > + * 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, > > > + u8 mask, u8 match, u16 timeout) > > > +{ > > > + struct spi_controller *ctlr = mem->spi->controller; > > > + int ret = -EOPNOTSUPP; > > > + > > > + if (ctlr->mem_ops && ctlr->mem_ops->poll_status) { > > > + ret = spi_mem_access_start(mem); > > > > You should probably check that op is a single byte read before > > accepting the command. > > Please do not discriminate against 8D-8D-8D flashes ;-). Then mask and match should probably be u16 :P. And the check as it is seems a bit lax to me. Drivers will of course be able to reject the op when there's more than one byte (or 16bit word in case of 8D) to read, but it feels like the core could automate that a bit. > > > > > > + if (ret) > > > + return ret; > > > + > > > + ret = ctlr->mem_ops->poll_status(mem, op, mask, match, timeout); > > > > You also need some sort of ->poll_status_is_supported() to validate > > that the controller supports the status polling for this specific op (I > > I don't think a separate function is needed for checking if the poll > status op is supported. Return value of -EOPNOTSUPP should be able to > signal that. This can also be used to check if Octal DDR capable > controllers are able to poll using 2-byte reads. Yeah, I had something more complex in mind to avoid doing this 'try native mode and fall back on sw-based more if not supported' dance every time a status poll is requested (something similar to what we do for dirmaps, with a status poll desc), but I guess that's a bit premature (and probably uneeded). > > > can imagine some controllers having a limit on the number of dummy > > cycles/address bytes). I guess you could just fall back on SW-based > > status polling if ctlr->mem_ops->poll_status() returns -ENOTSUPP. > > > > > + > > > + spi_mem_access_end(mem); > > > + } > > > + > > > + 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); > > > diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h > > > index 2b65c9edc34e..5f78917c0f68 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, > > > + u8 mask, u8 match, u16 timeout); > > > }; > > > > > > /** > > > @@ -369,6 +373,10 @@ 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); > > > > > > +int spi_mem_poll_status(struct spi_mem *mem, > > > + const struct spi_mem_op *op, > > > + u8 mask, u8 match, u16 timeout); > > > + > > > int spi_mem_driver_register_with_owner(struct spi_mem_driver *drv, > > > struct module *owner); > > > >
On 03/05/21 11:11AM, Boris Brezillon wrote: > On Mon, 3 May 2021 14:17:44 +0530 > Pratyush Yadav <p.yadav@ti.com> wrote: > > > On 30/04/21 06:51PM, Boris Brezillon wrote: > > > On Mon, 26 Apr 2021 16:39:32 +0200 > > > <patrice.chotard@foss.st.com> wrote: > > > > > > > From: Christophe Kerello <christophe.kerello@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. > > > > > > > > Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com> > > > > Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com> > > > > --- > > > > drivers/spi/spi-mem.c | 34 ++++++++++++++++++++++++++++++++++ > > > > include/linux/spi/spi-mem.h | 8 ++++++++ > > > > 2 files changed, 42 insertions(+) > > > > > > > > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c > > > > index 1513553e4080..43dce4b0efa4 100644 > > > > --- a/drivers/spi/spi-mem.c > > > > +++ b/drivers/spi/spi-mem.c > > > > @@ -743,6 +743,40 @@ 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_poll_status() - Poll memory device status > > > > + * @mem: SPI memory device > > > > + * @op: the memory operation to execute > > > > + * @mask: status bitmask to ckeck > > > > + * @match: status expected value > > > > + * @timeout: timeout > > > > + * > > > > + * 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, > > > > + u8 mask, u8 match, u16 timeout) > > > > +{ > > > > + struct spi_controller *ctlr = mem->spi->controller; > > > > + int ret = -EOPNOTSUPP; > > > > + > > > > + if (ctlr->mem_ops && ctlr->mem_ops->poll_status) { > > > > + ret = spi_mem_access_start(mem); > > > > > > You should probably check that op is a single byte read before > > > accepting the command. > > > > Please do not discriminate against 8D-8D-8D flashes ;-). > > Then mask and match should probably be u16 :P. And the check as it is > seems a bit lax to me. Drivers will of course be able to reject the op > when there's more than one byte (or 16bit word in case of 8D) to read, > but it feels like the core could automate that a bit. The two 8D flashes that are currently supported in SPI NOR both have a 1-byte status register. But to read it, the read op should be 2-byte long to avoid partial cycles at the end. The second byte is simply discarded. 2-byte wide registers might show up in the future, but for now at least we don't have to worry about them. > > > > > > > > > > + if (ret) > > > > + return ret; > > > > + > > > > + ret = ctlr->mem_ops->poll_status(mem, op, mask, match, timeout); > > > > > > You also need some sort of ->poll_status_is_supported() to validate > > > that the controller supports the status polling for this specific op (I > > > > I don't think a separate function is needed for checking if the poll > > status op is supported. Return value of -EOPNOTSUPP should be able to > > signal that. This can also be used to check if Octal DDR capable > > controllers are able to poll using 2-byte reads. > > Yeah, I had something more complex in mind to avoid doing this 'try > native mode and fall back on sw-based more if not supported' dance > every time a status poll is requested (something similar to what we do > for dirmaps, with a status poll desc), but I guess that's a bit > premature (and probably uneeded). I think Mark also suggested something similar. Make the CPU/non-CPU case transparent to the caller. I agree with with this direction. Makes the caller simpler. I also mentioned in a reply to this patch that supports_op() should be called before the op is executed. That should take care of "base" support for the op. The poll-specific checks can go in the poll_status() function itself. If either of those say the op is not supported, it should fall back to CPU based polling. That's the design that makes the most sense to me. > > > > > > can imagine some controllers having a limit on the number of dummy > > > cycles/address bytes). I guess you could just fall back on SW-based > > > status polling if ctlr->mem_ops->poll_status() returns -ENOTSUPP. > > > > > > > + > > > > + spi_mem_access_end(mem); > > > > + } > > > > + > > > > + 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); > > > > diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h > > > > index 2b65c9edc34e..5f78917c0f68 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, > > > > + u8 mask, u8 match, u16 timeout); > > > > }; > > > > > > > > /** > > > > @@ -369,6 +373,10 @@ 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); > > > > > > > > +int spi_mem_poll_status(struct spi_mem *mem, > > > > + const struct spi_mem_op *op, > > > > + u8 mask, u8 match, u16 timeout); > > > > + > > > > int spi_mem_driver_register_with_owner(struct spi_mem_driver *drv, > > > > struct module *owner); > > > > > > >
On Mon, 3 May 2021 14:59:37 +0530 Pratyush Yadav <p.yadav@ti.com> wrote: > On 03/05/21 11:11AM, Boris Brezillon wrote: > > On Mon, 3 May 2021 14:17:44 +0530 > > Pratyush Yadav <p.yadav@ti.com> wrote: > > > > > On 30/04/21 06:51PM, Boris Brezillon wrote: > > > > On Mon, 26 Apr 2021 16:39:32 +0200 > > > > <patrice.chotard@foss.st.com> wrote: > > > > > > > > > From: Christophe Kerello <christophe.kerello@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. > > > > > > > > > > Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com> > > > > > Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com> > > > > > --- > > > > > drivers/spi/spi-mem.c | 34 ++++++++++++++++++++++++++++++++++ > > > > > include/linux/spi/spi-mem.h | 8 ++++++++ > > > > > 2 files changed, 42 insertions(+) > > > > > > > > > > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c > > > > > index 1513553e4080..43dce4b0efa4 100644 > > > > > --- a/drivers/spi/spi-mem.c > > > > > +++ b/drivers/spi/spi-mem.c > > > > > @@ -743,6 +743,40 @@ 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_poll_status() - Poll memory device status > > > > > + * @mem: SPI memory device > > > > > + * @op: the memory operation to execute > > > > > + * @mask: status bitmask to ckeck > > > > > + * @match: status expected value > > > > > + * @timeout: timeout > > > > > + * > > > > > + * 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, > > > > > + u8 mask, u8 match, u16 timeout) > > > > > +{ > > > > > + struct spi_controller *ctlr = mem->spi->controller; > > > > > + int ret = -EOPNOTSUPP; > > > > > + > > > > > + if (ctlr->mem_ops && ctlr->mem_ops->poll_status) { > > > > > + ret = spi_mem_access_start(mem); > > > > > > > > You should probably check that op is a single byte read before > > > > accepting the command. > > > > > > Please do not discriminate against 8D-8D-8D flashes ;-). > > > > Then mask and match should probably be u16 :P. And the check as it is > > seems a bit lax to me. Drivers will of course be able to reject the op > > when there's more than one byte (or 16bit word in case of 8D) to read, > > but it feels like the core could automate that a bit. > > The two 8D flashes that are currently supported in SPI NOR both have a > 1-byte status register. But to read it, the read op should be 2-byte > long to avoid partial cycles at the end. The second byte is simply > discarded. > > 2-byte wide registers might show up in the future, but for now at least > we don't have to worry about them. Well, I guess it doesn't hurt to take it into account now. I mean, what's happening on the bus in that case is a 2byte transfer, with the second byte being ignored, which you can describe with a 16bit mask of 0xMM00 (assuming big endian transfers here, as done for other ops). > > > > > > > > > > > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + ret = ctlr->mem_ops->poll_status(mem, op, mask, match, timeout); > > > > > > > > You also need some sort of ->poll_status_is_supported() to validate > > > > that the controller supports the status polling for this specific op (I > > > > > > I don't think a separate function is needed for checking if the poll > > > status op is supported. Return value of -EOPNOTSUPP should be able to > > > signal that. This can also be used to check if Octal DDR capable > > > controllers are able to poll using 2-byte reads. > > > > Yeah, I had something more complex in mind to avoid doing this 'try > > native mode and fall back on sw-based more if not supported' dance > > every time a status poll is requested (something similar to what we do > > for dirmaps, with a status poll desc), but I guess that's a bit > > premature (and probably uneeded). > > I think Mark also suggested something similar. Make the CPU/non-CPU case > transparent to the caller. I agree with with this direction. Makes the > caller simpler. It's kind of orthogonal to what I was suggesting, but yes, that's definitely a good idea. We certainly don't want the spi-nor layer to open code the same logic if the spi-mem layer can do it for us. > > I also mentioned in a reply to this patch that supports_op() should be > called before the op is executed. That should take care of "base" > support for the op. The poll-specific checks can go in the poll_status() > function itself. If either of those say the op is not supported, it > should fall back to CPU based polling. That's the design that makes the > most sense to me. What I had in mind was more: 1/ create a poll desc with spi_mem_create_poll_status_desc(). The "operation supported" check is done here. The controller can store all its HW-specific state in there. If the operation is not natively supported, a SW-based poll descriptor (similar to the SW-based dirmap) is created 2/ poll the status with spi_mem_poll_status(). This function is passed a poll descriptor which helps select the path that should be taken without having to check every time whether the hardware supports a specific status polling op. I can also imagine some preparation being done during the desc creation if that makes sense (preparing reg values to be written when a status poll request is issued for instance) Anyway, as I said, this sort of optimization might be a bit premature.
On 03/05/21 11:52AM, Boris Brezillon wrote: > On Mon, 3 May 2021 14:59:37 +0530 > Pratyush Yadav <p.yadav@ti.com> wrote: > > > On 03/05/21 11:11AM, Boris Brezillon wrote: > > > On Mon, 3 May 2021 14:17:44 +0530 > > > Pratyush Yadav <p.yadav@ti.com> wrote: > > > > > > > On 30/04/21 06:51PM, Boris Brezillon wrote: > > > > > On Mon, 26 Apr 2021 16:39:32 +0200 > > > > > <patrice.chotard@foss.st.com> wrote: > > > > > > > > > > > From: Christophe Kerello <christophe.kerello@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. > > > > > > > > > > > > Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com> > > > > > > Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com> > > > > > > --- > > > > > > drivers/spi/spi-mem.c | 34 ++++++++++++++++++++++++++++++++++ > > > > > > include/linux/spi/spi-mem.h | 8 ++++++++ > > > > > > 2 files changed, 42 insertions(+) > > > > > > > > > > > > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c > > > > > > index 1513553e4080..43dce4b0efa4 100644 > > > > > > --- a/drivers/spi/spi-mem.c > > > > > > +++ b/drivers/spi/spi-mem.c > > > > > > @@ -743,6 +743,40 @@ 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_poll_status() - Poll memory device status > > > > > > + * @mem: SPI memory device > > > > > > + * @op: the memory operation to execute > > > > > > + * @mask: status bitmask to ckeck > > > > > > + * @match: status expected value > > > > > > + * @timeout: timeout > > > > > > + * > > > > > > + * 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, > > > > > > + u8 mask, u8 match, u16 timeout) > > > > > > +{ > > > > > > + struct spi_controller *ctlr = mem->spi->controller; > > > > > > + int ret = -EOPNOTSUPP; > > > > > > + > > > > > > + if (ctlr->mem_ops && ctlr->mem_ops->poll_status) { > > > > > > + ret = spi_mem_access_start(mem); > > > > > > > > > > You should probably check that op is a single byte read before > > > > > accepting the command. > > > > > > > > Please do not discriminate against 8D-8D-8D flashes ;-). > > > > > > Then mask and match should probably be u16 :P. And the check as it is > > > seems a bit lax to me. Drivers will of course be able to reject the op > > > when there's more than one byte (or 16bit word in case of 8D) to read, > > > but it feels like the core could automate that a bit. > > > > The two 8D flashes that are currently supported in SPI NOR both have a > > 1-byte status register. But to read it, the read op should be 2-byte > > long to avoid partial cycles at the end. The second byte is simply > > discarded. > > > > 2-byte wide registers might show up in the future, but for now at least > > we don't have to worry about them. > > Well, I guess it doesn't hurt to take it into account now. I mean, > what's happening on the bus in that case is a 2byte transfer, with the > second byte being ignored, which you can describe with a 16bit mask > of 0xMM00 (assuming big endian transfers here, as done for other ops). Makes sense. > > > > > > > > > > > > > > > > > > > > > + if (ret) > > > > > > + return ret; > > > > > > + > > > > > > + ret = ctlr->mem_ops->poll_status(mem, op, mask, match, timeout); > > > > > > > > > > You also need some sort of ->poll_status_is_supported() to validate > > > > > that the controller supports the status polling for this specific op (I > > > > > > > > I don't think a separate function is needed for checking if the poll > > > > status op is supported. Return value of -EOPNOTSUPP should be able to > > > > signal that. This can also be used to check if Octal DDR capable > > > > controllers are able to poll using 2-byte reads. > > > > > > Yeah, I had something more complex in mind to avoid doing this 'try > > > native mode and fall back on sw-based more if not supported' dance > > > every time a status poll is requested (something similar to what we do > > > for dirmaps, with a status poll desc), but I guess that's a bit > > > premature (and probably uneeded). > > > > I think Mark also suggested something similar. Make the CPU/non-CPU case > > transparent to the caller. I agree with with this direction. Makes the > > caller simpler. > > It's kind of orthogonal to what I was suggesting, but yes, that's > definitely a good idea. We certainly don't want the spi-nor layer to > open code the same logic if the spi-mem layer can do it for us. > > > > > I also mentioned in a reply to this patch that supports_op() should be > > called before the op is executed. That should take care of "base" > > support for the op. The poll-specific checks can go in the poll_status() > > function itself. If either of those say the op is not supported, it > > should fall back to CPU based polling. That's the design that makes the > > most sense to me. > > What I had in mind was more: > > 1/ create a poll desc with spi_mem_create_poll_status_desc(). The > "operation supported" check is done here. The controller can store > all its HW-specific state in there. If the operation is not natively > supported, a SW-based poll descriptor (similar to the SW-based > dirmap) is created > 2/ poll the status with spi_mem_poll_status(). This function is passed > a poll descriptor which helps select the path that should be taken > without having to check every time whether the hardware supports a > specific status polling op. I can also imagine some preparation > being done during the desc creation if that makes sense (preparing > reg values to be written when a status poll request is issued for > instance) > > Anyway, as I said, this sort of optimization might be a bit premature. Indeed, this sounds a bit premature to me too.
Hi Mark On 4/30/21 5:56 PM, Mark Brown wrote: > On Fri, Apr 30, 2021 at 04:22:34PM +0200, Patrice CHOTARD wrote: >> On 4/26/21 6:51 PM, Mark Brown wrote: >>> On Mon, Apr 26, 2021 at 09:56:12PM +0530, Pratyush Yadav wrote: > >>> Is it possible there's some situation where you're waiting for some bits >>> to clear as well? > >> Yes, we are waiting STATUS_BUSY bit to be cleared, see patch 2 which is making >> usage of this API. > > Then the inverse question applies - is there no circumstance where we > might be waiting for a bit to be set? > >>> We already have the core handling other timeouts. We don't pass around >>> completions but rather have an API function that the driver has to call >>> when the operation completes, a similar pattern might work here. Part > >> So, if i correctly understood, you make allusion to what is already done >> in SPI core framework with spi_finalize_current_transfer() right ? > > Yes, and _current_message(). > >>> of the thing with those APIs which I'm missing here is that this will >>> just return -EOPNOTSUPP if the driver can't do the delay in hardware, I >>> think it would be cleaner if this API were similar and the core dealt >>> with doing the delay/poll on the CPU. That way the users don't need to >>> repeat the handling for the offload/non-offload cases. > >> Sorry, i didn't catch what you mean here. In PATCH 2, that's the case, >> if spi_mem_poll_status() is not supported, the core is dealing with >> the delay/poll on the CPU in spinand_wait(). > > That's in the NAND core, not in spi-mem. Any other users of spi-mem > will also need to open code stuff. > Ok, got it, i will transfer what is done in spi_nand_wait() into spi_mem_poll_status() in order to get the full feature in spi-mem which will profit to all spi-mem users as requested. Thanks Patrice
Hi Boris, Pratyush On 5/3/21 11:52 AM, Boris Brezillon wrote: > On Mon, 3 May 2021 14:59:37 +0530 > Pratyush Yadav <p.yadav@ti.com> wrote: > >> On 03/05/21 11:11AM, Boris Brezillon wrote: >>> On Mon, 3 May 2021 14:17:44 +0530 >>> Pratyush Yadav <p.yadav@ti.com> wrote: >>> >>>> On 30/04/21 06:51PM, Boris Brezillon wrote: >>>>> On Mon, 26 Apr 2021 16:39:32 +0200 >>>>> <patrice.chotard@foss.st.com> wrote: >>>>> >>>>>> From: Christophe Kerello <christophe.kerello@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. >>>>>> >>>>>> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com> >>>>>> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com> >>>>>> --- >>>>>> drivers/spi/spi-mem.c | 34 ++++++++++++++++++++++++++++++++++ >>>>>> include/linux/spi/spi-mem.h | 8 ++++++++ >>>>>> 2 files changed, 42 insertions(+) >>>>>> >>>>>> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c >>>>>> index 1513553e4080..43dce4b0efa4 100644 >>>>>> --- a/drivers/spi/spi-mem.c >>>>>> +++ b/drivers/spi/spi-mem.c >>>>>> @@ -743,6 +743,40 @@ 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_poll_status() - Poll memory device status >>>>>> + * @mem: SPI memory device >>>>>> + * @op: the memory operation to execute >>>>>> + * @mask: status bitmask to ckeck >>>>>> + * @match: status expected value >>>>>> + * @timeout: timeout >>>>>> + * >>>>>> + * 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, >>>>>> + u8 mask, u8 match, u16 timeout) >>>>>> +{ >>>>>> + struct spi_controller *ctlr = mem->spi->controller; >>>>>> + int ret = -EOPNOTSUPP; >>>>>> + >>>>>> + if (ctlr->mem_ops && ctlr->mem_ops->poll_status) { >>>>>> + ret = spi_mem_access_start(mem); >>>>> >>>>> You should probably check that op is a single byte read before >>>>> accepting the command. >>>> >>>> Please do not discriminate against 8D-8D-8D flashes ;-). >>> >>> Then mask and match should probably be u16 :P. And the check as it is >>> seems a bit lax to me. Drivers will of course be able to reject the op >>> when there's more than one byte (or 16bit word in case of 8D) to read, >>> but it feels like the core could automate that a bit. >> >> The two 8D flashes that are currently supported in SPI NOR both have a >> 1-byte status register. But to read it, the read op should be 2-byte >> long to avoid partial cycles at the end. The second byte is simply >> discarded. >> >> 2-byte wide registers might show up in the future, but for now at least >> we don't have to worry about them. > > Well, I guess it doesn't hurt to take it into account now. I mean, > what's happening on the bus in that case is a 2byte transfer, with the > second byte being ignored, which you can describe with a 16bit mask > of 0xMM00 (assuming big endian transfers here, as done for other ops). OK, i will take the 2 byte case into account. I will grab all remarks done and come back with an updated proposal in a few days. Thanks Patrice > >> >>> >>>> >>>>> >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + >>>>>> + ret = ctlr->mem_ops->poll_status(mem, op, mask, match, timeout); >>>>> >>>>> You also need some sort of ->poll_status_is_supported() to validate >>>>> that the controller supports the status polling for this specific op (I >>>> >>>> I don't think a separate function is needed for checking if the poll >>>> status op is supported. Return value of -EOPNOTSUPP should be able to >>>> signal that. This can also be used to check if Octal DDR capable >>>> controllers are able to poll using 2-byte reads. >>> >>> Yeah, I had something more complex in mind to avoid doing this 'try >>> native mode and fall back on sw-based more if not supported' dance >>> every time a status poll is requested (something similar to what we do >>> for dirmaps, with a status poll desc), but I guess that's a bit >>> premature (and probably uneeded). >> >> I think Mark also suggested something similar. Make the CPU/non-CPU case >> transparent to the caller. I agree with with this direction. Makes the >> caller simpler. > > It's kind of orthogonal to what I was suggesting, but yes, that's > definitely a good idea. We certainly don't want the spi-nor layer to > open code the same logic if the spi-mem layer can do it for us. > >> >> I also mentioned in a reply to this patch that supports_op() should be >> called before the op is executed. That should take care of "base" >> support for the op. The poll-specific checks can go in the poll_status() >> function itself. If either of those say the op is not supported, it >> should fall back to CPU based polling. That's the design that makes the >> most sense to me. > > What I had in mind was more: > > 1/ create a poll desc with spi_mem_create_poll_status_desc(). The > "operation supported" check is done here. The controller can store > all its HW-specific state in there. If the operation is not natively > supported, a SW-based poll descriptor (similar to the SW-based > dirmap) is created > 2/ poll the status with spi_mem_poll_status(). This function is passed > a poll descriptor which helps select the path that should be taken > without having to check every time whether the hardware supports a > specific status polling op. I can also imagine some preparation > being done during the desc creation if that makes sense (preparing > reg values to be written when a status poll request is issued for > instance) > > Anyway, as I said, this sort of optimization might be a bit premature. >
diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index 1513553e4080..43dce4b0efa4 100644 --- a/drivers/spi/spi-mem.c +++ b/drivers/spi/spi-mem.c @@ -743,6 +743,40 @@ 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_poll_status() - Poll memory device status + * @mem: SPI memory device + * @op: the memory operation to execute + * @mask: status bitmask to ckeck + * @match: status expected value + * @timeout: timeout + * + * 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, + u8 mask, u8 match, u16 timeout) +{ + struct spi_controller *ctlr = mem->spi->controller; + int ret = -EOPNOTSUPP; + + 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); + + spi_mem_access_end(mem); + } + + 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); diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h index 2b65c9edc34e..5f78917c0f68 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, + u8 mask, u8 match, u16 timeout); }; /** @@ -369,6 +373,10 @@ 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); +int spi_mem_poll_status(struct spi_mem *mem, + const struct spi_mem_op *op, + u8 mask, u8 match, u16 timeout); + int spi_mem_driver_register_with_owner(struct spi_mem_driver *drv, struct module *owner);