| Message ID | 20240722-dlech-mainline-spi-engine-offload-2-v3-3-7420e45df69b@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | spi: axi-spi-engine: add offload support | expand |
On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote: > This extends the SPI framework to support hardware triggered offloading. > This allows an arbitrary hardware trigger to be used to start a SPI > transfer that was previously set up with spi_offload_prepare(). > > Since the hardware trigger can happen at any time, this means the SPI > bus must be reserved for exclusive use as long as the hardware trigger > is enabled. Since a hardware trigger could be enabled indefinitely, > we can't use the existing spi_bus_lock() and spi_bus_unlock() functions, > otherwise this could cause deadlocks. So we introduce a new flag so that > any attempt to lock or use the bus will fail with -EBUSY as long as the > hardware trigger is enabled. > > Peripheral drivers may need to control the trigger source as well. For > this, we introduce a new spi_offload_hw_trigger_get_clk() function that > can be used to get a clock trigger source. This is intended for used > by ADC drivers that will use the clock to control the sample rate. > Additional functions to get other types of trigger sources could be > added in the future. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > > TODO: Currently, spi_bus_lock() always returns 0, so none of the callers > check the return value. All callers will need to be updated first before > this can be merged. > > v3 changes: > * renamed enable/disable functions to spi_offload_hw_trigger_*mode*_... > * added spi_offload_hw_trigger_get_clk() function > * fixed missing EXPORT_SYMBOL_GPL > > v2 changes: > > This is split out from "spi: add core support for controllers with > offload capabilities". > > Mark suggested that the standard SPI APIs should be aware that the > hardware trigger is enabled. So I've added some locking for this. Nuno > suggested that this might be overly strict though, and that we should > let each individual controller driver decide what to do. For our use > case though, I think we generally are going to have a single peripheral > on the SPI bus, so this seems like a reasonable starting place anyway. > --- How explicitly do we want to be about returning errors? It seems that if the trigger is enabled we can't anything else on the controller/offload_engine so we could very well just hold the controller lock when enabling the trigger and release it when disabling it. Pretty much the same behavior as spi_bus_lock()... ... > > + > +/** > + * spi_offload_hw_trigger_get_clk - Get the clock for the offload trigger > + * @spi: SPI device > + * @id: Function ID if SPI device uses more than one offload or NULL. > + * > + * The caller is responsible for calling clk_put() on the returned clock. > + * > + * Return: The clock for the offload trigger, or negative error code > + */ > +static inline > +struct clk *spi_offload_hw_trigger_get_clk(struct spi_device *spi, const char > *id) > +{ > + struct spi_controller *ctlr = spi->controller; > + > + if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_get_clk) > + return ERR_PTR(-EOPNOTSUPP); > + > + return ctlr->offload_ops->hw_trigger_get_clk(spi, id); > +} > It would be nice if we could have some kind of spi abstraction... - Nuno Sá
On 7/23/24 2:53 AM, Nuno Sá wrote: > On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote: >> This extends the SPI framework to support hardware triggered offloading. >> This allows an arbitrary hardware trigger to be used to start a SPI >> transfer that was previously set up with spi_offload_prepare(). >> >> Since the hardware trigger can happen at any time, this means the SPI >> bus must be reserved for exclusive use as long as the hardware trigger >> is enabled. Since a hardware trigger could be enabled indefinitely, >> we can't use the existing spi_bus_lock() and spi_bus_unlock() functions, >> otherwise this could cause deadlocks. So we introduce a new flag so that >> any attempt to lock or use the bus will fail with -EBUSY as long as the >> hardware trigger is enabled. >> >> Peripheral drivers may need to control the trigger source as well. For >> this, we introduce a new spi_offload_hw_trigger_get_clk() function that >> can be used to get a clock trigger source. This is intended for used >> by ADC drivers that will use the clock to control the sample rate. >> Additional functions to get other types of trigger sources could be >> added in the future. >> >> Signed-off-by: David Lechner <dlechner@baylibre.com> >> --- >> >> TODO: Currently, spi_bus_lock() always returns 0, so none of the callers >> check the return value. All callers will need to be updated first before >> this can be merged. >> >> v3 changes: >> * renamed enable/disable functions to spi_offload_hw_trigger_*mode*_... >> * added spi_offload_hw_trigger_get_clk() function >> * fixed missing EXPORT_SYMBOL_GPL >> >> v2 changes: >> >> This is split out from "spi: add core support for controllers with >> offload capabilities". >> >> Mark suggested that the standard SPI APIs should be aware that the >> hardware trigger is enabled. So I've added some locking for this. Nuno >> suggested that this might be overly strict though, and that we should >> let each individual controller driver decide what to do. For our use >> case though, I think we generally are going to have a single peripheral >> on the SPI bus, so this seems like a reasonable starting place anyway. >> --- > > How explicitly do we want to be about returning errors? It seems that if the > trigger is enabled we can't anything else on the controller/offload_engine so we > could very well just hold the controller lock when enabling the trigger and > release it when disabling it. Pretty much the same behavior as spi_bus_lock()... The problem I see with using spi_bus_lock() in it's current form is that SPI offload triggers could be enabled indefinitely. So any other devices on the bus that tried to use the bus and take the lock would essentially deadlock waiting for the offload user to release the lock. This is why I added the -BUSY return, to avoid this deadlock. > > ... > >> >> + >> +/** >> + * spi_offload_hw_trigger_get_clk - Get the clock for the offload trigger >> + * @spi: SPI device >> + * @id: Function ID if SPI device uses more than one offload or NULL. >> + * >> + * The caller is responsible for calling clk_put() on the returned clock. >> + * >> + * Return: The clock for the offload trigger, or negative error code >> + */ >> +static inline >> +struct clk *spi_offload_hw_trigger_get_clk(struct spi_device *spi, const char >> *id) >> +{ >> + struct spi_controller *ctlr = spi->controller; >> + >> + if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_get_clk) >> + return ERR_PTR(-EOPNOTSUPP); >> + >> + return ctlr->offload_ops->hw_trigger_get_clk(spi, id); >> +} >> > > It would be nice if we could have some kind of spi abstraction... After reading your other replies, I think I understand what you mean here. Are you thinking some kind of `struct spi_offload_trigger` that could be any kind of trigger (clk, gpio, etc.)?
On Tue, 2024-07-23 at 09:30 -0500, David Lechner wrote: > On 7/23/24 2:53 AM, Nuno Sá wrote: > > On Mon, 2024-07-22 at 16:57 -0500, David Lechner wrote: > > > This extends the SPI framework to support hardware triggered offloading. > > > This allows an arbitrary hardware trigger to be used to start a SPI > > > transfer that was previously set up with spi_offload_prepare(). > > > > > > Since the hardware trigger can happen at any time, this means the SPI > > > bus must be reserved for exclusive use as long as the hardware trigger > > > is enabled. Since a hardware trigger could be enabled indefinitely, > > > we can't use the existing spi_bus_lock() and spi_bus_unlock() functions, > > > otherwise this could cause deadlocks. So we introduce a new flag so that > > > any attempt to lock or use the bus will fail with -EBUSY as long as the > > > hardware trigger is enabled. > > > > > > Peripheral drivers may need to control the trigger source as well. For > > > this, we introduce a new spi_offload_hw_trigger_get_clk() function that > > > can be used to get a clock trigger source. This is intended for used > > > by ADC drivers that will use the clock to control the sample rate. > > > Additional functions to get other types of trigger sources could be > > > added in the future. > > > > > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > > --- > > > > > > TODO: Currently, spi_bus_lock() always returns 0, so none of the callers > > > check the return value. All callers will need to be updated first before > > > this can be merged. > > > > > > v3 changes: > > > * renamed enable/disable functions to spi_offload_hw_trigger_*mode*_... > > > * added spi_offload_hw_trigger_get_clk() function > > > * fixed missing EXPORT_SYMBOL_GPL > > > > > > v2 changes: > > > > > > This is split out from "spi: add core support for controllers with > > > offload capabilities". > > > > > > Mark suggested that the standard SPI APIs should be aware that the > > > hardware trigger is enabled. So I've added some locking for this. Nuno > > > suggested that this might be overly strict though, and that we should > > > let each individual controller driver decide what to do. For our use > > > case though, I think we generally are going to have a single peripheral > > > on the SPI bus, so this seems like a reasonable starting place anyway. > > > --- > > > > How explicitly do we want to be about returning errors? It seems that if the > > trigger is enabled we can't anything else on the controller/offload_engine so we > > could very well just hold the controller lock when enabling the trigger and > > release it when disabling it. Pretty much the same behavior as spi_bus_lock()... > > The problem I see with using spi_bus_lock() in it's current form is that > SPI offload triggers could be enabled indefinitely. So any other devices > on the bus that tried to use the bus and take the lock would essentially > deadlock waiting for the offload user to release the lock. This is why > I added the -BUSY return, to avoid this deadlock. > If someone does not disable the trigger at some point that's a bug. If I understood things correctly, even if someone tries to access the bus will get -EBUSY. But yeah, arguably it's better to get a valid error rather than blocking/sleeping > > > > ... > > > > > > > > + > > > +/** > > > + * spi_offload_hw_trigger_get_clk - Get the clock for the offload trigger > > > + * @spi: SPI device > > > + * @id: Function ID if SPI device uses more than one offload or NULL. > > > + * > > > + * The caller is responsible for calling clk_put() on the returned clock. > > > + * > > > + * Return: The clock for the offload trigger, or negative error code > > > + */ > > > +static inline > > > +struct clk *spi_offload_hw_trigger_get_clk(struct spi_device *spi, const char > > > *id) > > > +{ > > > + struct spi_controller *ctlr = spi->controller; > > > + > > > + if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_get_clk) > > > + return ERR_PTR(-EOPNOTSUPP); > > > + > > > + return ctlr->offload_ops->hw_trigger_get_clk(spi, id); > > > +} > > > > > > > It would be nice if we could have some kind of spi abstraction... > > After reading your other replies, I think I understand what you mean here. > > Are you thinking some kind of `struct spi_offload_trigger` that could be > any kind of trigger (clk, gpio, etc.)? > Yeah, something in that direction... - Nuno Sá
On Mon, 22 Jul 2024 16:57:10 -0500 David Lechner <dlechner@baylibre.com> wrote: > This extends the SPI framework to support hardware triggered offloading. > This allows an arbitrary hardware trigger to be used to start a SPI > transfer that was previously set up with spi_offload_prepare(). > > Since the hardware trigger can happen at any time, this means the SPI > bus must be reserved for exclusive use as long as the hardware trigger > is enabled. Since a hardware trigger could be enabled indefinitely, > we can't use the existing spi_bus_lock() and spi_bus_unlock() functions, > otherwise this could cause deadlocks. So we introduce a new flag so that > any attempt to lock or use the bus will fail with -EBUSY as long as the > hardware trigger is enabled. > > Peripheral drivers may need to control the trigger source as well. For > this, we introduce a new spi_offload_hw_trigger_get_clk() function that > can be used to get a clock trigger source. This is intended for used > by ADC drivers that will use the clock to control the sample rate. > Additional functions to get other types of trigger sources could be > added in the future. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > > TODO: Currently, spi_bus_lock() always returns 0, so none of the callers > check the return value. All callers will need to be updated first before > this can be merged. If it's going to fail sometimes, probably needs a name that indicates that. I'm not sure spi_bus_try_lock() is appropriate though. > > v3 changes: > * renamed enable/disable functions to spi_offload_hw_trigger_*mode*_... > * added spi_offload_hw_trigger_get_clk() function > * fixed missing EXPORT_SYMBOL_GPL > > v2 changes: > > This is split out from "spi: add core support for controllers with > offload capabilities". > > Mark suggested that the standard SPI APIs should be aware that the > hardware trigger is enabled. So I've added some locking for this. Nuno > suggested that this might be overly strict though, and that we should > let each individual controller driver decide what to do. For our use > case though, I think we generally are going to have a single peripheral > on the SPI bus, so this seems like a reasonable starting place anyway. ... > +int spi_offload_hw_trigger_mode_enable(struct spi_device *spi, const char *id) > +{ > + struct spi_controller *ctlr = spi->controller; > + unsigned long flags; > + int ret; > + > + if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_mode_enable) > + return -EOPNOTSUPP; > + > + mutex_lock(&ctlr->bus_lock_mutex); > + > + if (ctlr->offload_hw_trigger_mode_enabled) { > + mutex_unlock(&ctlr->bus_lock_mutex); > + return -EBUSY; > + } > + > + spin_lock_irqsave(&ctlr->bus_lock_spinlock, flags); > + ctlr->offload_hw_trigger_mode_enabled = true; Why do you need to take the spinlock when setting this to true, but not when setting it to fast (in the error path below)? > + spin_unlock_irqrestore(&ctlr->bus_lock_spinlock, flags); > + > + /* TODO: how to wait for empty message queue? */ > + > + mutex_lock(&ctlr->io_mutex); > + ret = ctlr->offload_ops->hw_trigger_mode_enable(spi, id); > + mutex_unlock(&ctlr->io_mutex); > + > + if (ret) { > + ctlr->offload_hw_trigger_mode_enabled = false; > + mutex_unlock(&ctlr->bus_lock_mutex); > + return ret; > + } > + > + mutex_unlock(&ctlr->bus_lock_mutex); > + > + return 0; > +} >
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index d01b2e5c8c44..7488e71f159f 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -4495,7 +4495,7 @@ int spi_async(struct spi_device *spi, struct spi_message *message) spin_lock_irqsave(&ctlr->bus_lock_spinlock, flags); - if (ctlr->bus_lock_flag) + if (ctlr->bus_lock_flag || ctlr->offload_hw_trigger_mode_enabled) ret = -EBUSY; else ret = __spi_async(spi, message); @@ -4638,6 +4638,12 @@ int spi_sync(struct spi_device *spi, struct spi_message *message) int ret; mutex_lock(&spi->controller->bus_lock_mutex); + + if (spi->controller->offload_hw_trigger_mode_enabled) { + mutex_unlock(&spi->controller->bus_lock_mutex); + return -EBUSY; + } + ret = __spi_sync(spi, message); mutex_unlock(&spi->controller->bus_lock_mutex); @@ -4680,7 +4686,7 @@ EXPORT_SYMBOL_GPL(spi_sync_locked); * exclusive access is over. Data transfer must be done by spi_sync_locked * and spi_async_locked calls when the SPI bus lock is held. * - * Return: always zero. + * Return: 0 on success, -EBUSY if the bus is reserved by offload hardware. */ int spi_bus_lock(struct spi_controller *ctlr) { @@ -4688,6 +4694,11 @@ int spi_bus_lock(struct spi_controller *ctlr) mutex_lock(&ctlr->bus_lock_mutex); + if (ctlr->offload_hw_trigger_mode_enabled) { + mutex_unlock(&ctlr->bus_lock_mutex); + return -EBUSY; + } + spin_lock_irqsave(&ctlr->bus_lock_spinlock, flags); ctlr->bus_lock_flag = 1; spin_unlock_irqrestore(&ctlr->bus_lock_spinlock, flags); @@ -4874,6 +4885,85 @@ void spi_offload_unprepare(struct spi_device *spi, const char *id, } EXPORT_SYMBOL_GPL(spi_offload_unprepare); +/** + * spi_offload_hw_trigger_mode_enable - enables hardware trigger for offload + * @spi: The spi device to use for the transfers. + * @id: Function ID if SPI device uses more than one offload or NULL. + * + * There must be a prepared offload instance with the specified ID (i.e. + * spi_offload_prepare() was called with the same ID). This will also reserve + * the bus for exclusive use by the offload instance until the hardware trigger + * is disabled. Any other attempts to send a transfer or lock the bus will fail + * with -EBUSY during this time. + * + * Calls must be balanced with spi_offload_hw_trigger_mode_disable(). + * + * Context: can sleep + * Return: 0 on success, else a negative error code. + */ +int spi_offload_hw_trigger_mode_enable(struct spi_device *spi, const char *id) +{ + struct spi_controller *ctlr = spi->controller; + unsigned long flags; + int ret; + + if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_mode_enable) + return -EOPNOTSUPP; + + mutex_lock(&ctlr->bus_lock_mutex); + + if (ctlr->offload_hw_trigger_mode_enabled) { + mutex_unlock(&ctlr->bus_lock_mutex); + return -EBUSY; + } + + spin_lock_irqsave(&ctlr->bus_lock_spinlock, flags); + ctlr->offload_hw_trigger_mode_enabled = true; + spin_unlock_irqrestore(&ctlr->bus_lock_spinlock, flags); + + /* TODO: how to wait for empty message queue? */ + + mutex_lock(&ctlr->io_mutex); + ret = ctlr->offload_ops->hw_trigger_mode_enable(spi, id); + mutex_unlock(&ctlr->io_mutex); + + if (ret) { + ctlr->offload_hw_trigger_mode_enabled = false; + mutex_unlock(&ctlr->bus_lock_mutex); + return ret; + } + + mutex_unlock(&ctlr->bus_lock_mutex); + + return 0; +} +EXPORT_SYMBOL_GPL(spi_offload_hw_trigger_mode_enable); + +/** + * spi_offload_hw_trigger_mode_disable - disables hardware trigger for offload + * @spi: The same SPI device passed to spi_offload_hw_trigger_mode_enable() + * @id: The same ID device passed to spi_offload_hw_trigger_mode_enable() + * + * Disables the hardware trigger for the offload instance with the specified ID + * and releases the bus for use by other clients. + * + * Context: can sleep + */ +void spi_offload_hw_trigger_mode_disable(struct spi_device *spi, const char *id) +{ + struct spi_controller *ctlr = spi->controller; + + if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_mode_disable) + return; + + mutex_lock(&ctlr->io_mutex); + ctlr->offload_ops->hw_trigger_mode_disable(spi, id); + mutex_unlock(&ctlr->io_mutex); + + ctlr->offload_hw_trigger_mode_enabled = false; +} +EXPORT_SYMBOL_GPL(spi_offload_hw_trigger_mode_disable); + /*-------------------------------------------------------------------------*/ #if IS_ENABLED(CONFIG_OF_DYNAMIC) diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 4998b48ea7fd..685548883004 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -634,6 +634,9 @@ struct spi_controller { /* Flag indicating that the SPI bus is locked for exclusive use */ bool bus_lock_flag; + /* Flag indicating the bus is reserved for use by hardware trigger */ + bool offload_hw_trigger_mode_enabled; + /* * Setup mode and clock, etc (SPI driver may call many times). * @@ -1604,12 +1607,49 @@ struct spi_controller_offload_ops { * @unprepare: Required callback to release any resources used by prepare(). */ void (*unprepare)(struct spi_device *spi, const char *id); + /** + * @hw_trigger_mode_enable: Optional callback to enable the hardware + * trigger for the given offload instance. + */ + int (*hw_trigger_mode_enable)(struct spi_device *spi, const char *id); + /** + * @hw_trigger_mode_disable: Optional callback to disable the hardware + * trigger for the given offload instance. + */ + void (*hw_trigger_mode_disable)(struct spi_device *spi, const char *id); + /** + * @hw_trigger_get_clk: Optional callback for controllers that have a + * hardware offload trigger that is connected to a clock. + */ + struct clk *(*hw_trigger_get_clk)(struct spi_device *spi, const char *id); }; extern int spi_offload_prepare(struct spi_device *spi, const char *id, struct spi_message *msg); extern void spi_offload_unprepare(struct spi_device *spi, const char *id, struct spi_message *msg); +extern int spi_offload_hw_trigger_mode_enable(struct spi_device *spi, const char *id); +extern void spi_offload_hw_trigger_mode_disable(struct spi_device *spi, const char *id); + +/** + * spi_offload_hw_trigger_get_clk - Get the clock for the offload trigger + * @spi: SPI device + * @id: Function ID if SPI device uses more than one offload or NULL. + * + * The caller is responsible for calling clk_put() on the returned clock. + * + * Return: The clock for the offload trigger, or negative error code + */ +static inline +struct clk *spi_offload_hw_trigger_get_clk(struct spi_device *spi, const char *id) +{ + struct spi_controller *ctlr = spi->controller; + + if (!ctlr->offload_ops || !ctlr->offload_ops->hw_trigger_get_clk) + return ERR_PTR(-EOPNOTSUPP); + + return ctlr->offload_ops->hw_trigger_get_clk(spi, id); +} /*---------------------------------------------------------------------------*/
This extends the SPI framework to support hardware triggered offloading. This allows an arbitrary hardware trigger to be used to start a SPI transfer that was previously set up with spi_offload_prepare(). Since the hardware trigger can happen at any time, this means the SPI bus must be reserved for exclusive use as long as the hardware trigger is enabled. Since a hardware trigger could be enabled indefinitely, we can't use the existing spi_bus_lock() and spi_bus_unlock() functions, otherwise this could cause deadlocks. So we introduce a new flag so that any attempt to lock or use the bus will fail with -EBUSY as long as the hardware trigger is enabled. Peripheral drivers may need to control the trigger source as well. For this, we introduce a new spi_offload_hw_trigger_get_clk() function that can be used to get a clock trigger source. This is intended for used by ADC drivers that will use the clock to control the sample rate. Additional functions to get other types of trigger sources could be added in the future. Signed-off-by: David Lechner <dlechner@baylibre.com> --- TODO: Currently, spi_bus_lock() always returns 0, so none of the callers check the return value. All callers will need to be updated first before this can be merged. v3 changes: * renamed enable/disable functions to spi_offload_hw_trigger_*mode*_... * added spi_offload_hw_trigger_get_clk() function * fixed missing EXPORT_SYMBOL_GPL v2 changes: This is split out from "spi: add core support for controllers with offload capabilities". Mark suggested that the standard SPI APIs should be aware that the hardware trigger is enabled. So I've added some locking for this. Nuno suggested that this might be overly strict though, and that we should let each individual controller driver decide what to do. For our use case though, I think we generally are going to have a single peripheral on the SPI bus, so this seems like a reasonable starting place anyway. --- drivers/spi/spi.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++-- include/linux/spi/spi.h | 40 +++++++++++++++++++++ 2 files changed, 132 insertions(+), 2 deletions(-)