| Message ID | 20240206-axi-spi-engine-round-2-1-v1-2-ea6eeb60f4fb@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: axi-spi-engine: performance improvements | expand |
On Tue, 2024-02-06 at 14:31 -0600, David Lechner wrote: > As a general principal, it is best to do as little as possible in an > interrupt handler. This patch reworks the AXI SPI Engine driver to move > timer_delete_sync() and spi_finalize_current_message() out of the > interrupt handler. Instead, spi_finalize_current_message() is moved to > the transfer_one_message function (similar to nearly all other SPI > controllers). A completion is now used to wait for the sync interrupt > that indicates that the message is complete. The watchdog timer is no > longer needed since we can use the wait_for_completion_timeout() > function to wait for the message to complete with the same effect. > > As a bonus, these changes also improve throughput of the SPI controller. > For example, this was tested on a ZynqMP with a 80MHz SCLK reading 4 > byte samples from an ADC. The max measured throughput increased from > 26k to 28k samples per second. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- Nice one... Reviewed-by: Nuno Sa <nuno.sa@analog.com> > drivers/spi/spi-axi-spi-engine.c | 40 +++++++++++++++------------------------ > - > 1 file changed, 15 insertions(+), 25 deletions(-) > > diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi- > engine.c > index 9cc602075c17..3c96aa9232b3 100644 > --- a/drivers/spi/spi-axi-spi-engine.c > +++ b/drivers/spi/spi-axi-spi-engine.c > @@ -6,6 +6,7 @@ > */ > > #include <linux/clk.h> > +#include <linux/completion.h> > #include <linux/fpga/adi-axi-common.h> > #include <linux/idr.h> > #include <linux/interrupt.h> > @@ -14,7 +15,6 @@ > #include <linux/module.h> > #include <linux/platform_device.h> > #include <linux/spi/spi.h> > -#include <linux/timer.h> > > #define SPI_ENGINE_REG_RESET 0x40 > > @@ -110,9 +110,7 @@ struct spi_engine { > spinlock_t lock; > > void __iomem *base; > - struct timer_list watchdog_timer; > - struct spi_controller *controller; > - > + struct completion msg_complete; > unsigned int int_enable; > }; > > @@ -484,11 +482,9 @@ static irqreturn_t spi_engine_irq(int irq, void *devid) > > if (pending & SPI_ENGINE_INT_SYNC && msg) { > if (completed_id == AXI_SPI_ENGINE_CUR_MSG_SYNC_ID) { > - if (timer_delete_sync(&spi_engine->watchdog_timer)) { > - msg->status = 0; > - msg->actual_length = msg->frame_length; > - spi_finalize_current_message(host); > - } > + msg->status = 0; > + msg->actual_length = msg->frame_length; > + complete(&spi_engine->msg_complete); > disable_int |= SPI_ENGINE_INT_SYNC; > } > } > @@ -559,7 +555,7 @@ static int spi_engine_transfer_one_message(struct > spi_controller *host, > unsigned int int_enable = 0; > unsigned long flags; > > - mod_timer(&spi_engine->watchdog_timer, jiffies + > msecs_to_jiffies(5000)); > + reinit_completion(&spi_engine->msg_complete); > > spin_lock_irqsave(&spi_engine->lock, flags); > > @@ -581,21 +577,16 @@ static int spi_engine_transfer_one_message(struct > spi_controller *host, > spi_engine->int_enable = int_enable; > spin_unlock_irqrestore(&spi_engine->lock, flags); > > - return 0; > -} > - > -static void spi_engine_timeout(struct timer_list *timer) > -{ > - struct spi_engine *spi_engine = from_timer(spi_engine, timer, > watchdog_timer); > - struct spi_controller *host = spi_engine->controller; > - > - if (WARN_ON(!host->cur_msg)) > - return; > + if (!wait_for_completion_timeout(&spi_engine->msg_complete, > + msecs_to_jiffies(5000))) { > + dev_err(&host->dev, > + "Timeout occurred while waiting for transfer to > complete. Hardware is probably broken.\n"); > + msg->status = -ETIMEDOUT; > + } > > - dev_err(&host->dev, > - "Timeout occurred while waiting for transfer to complete. > Hardware is probably broken.\n"); > - host->cur_msg->status = -ETIMEDOUT; > spi_finalize_current_message(host); > + > + return msg->status; > } > > static void spi_engine_release_hw(void *p) > @@ -626,8 +617,7 @@ static int spi_engine_probe(struct platform_device *pdev) > spi_engine = spi_controller_get_devdata(host); > > spin_lock_init(&spi_engine->lock); > - timer_setup(&spi_engine->watchdog_timer, spi_engine_timeout, > TIMER_IRQSAFE); > - spi_engine->controller = host; > + init_completion(&spi_engine->msg_complete); > > spi_engine->clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk"); > if (IS_ERR(spi_engine->clk)) >
diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c index 9cc602075c17..3c96aa9232b3 100644 --- a/drivers/spi/spi-axi-spi-engine.c +++ b/drivers/spi/spi-axi-spi-engine.c @@ -6,6 +6,7 @@ */ #include <linux/clk.h> +#include <linux/completion.h> #include <linux/fpga/adi-axi-common.h> #include <linux/idr.h> #include <linux/interrupt.h> @@ -14,7 +15,6 @@ #include <linux/module.h> #include <linux/platform_device.h> #include <linux/spi/spi.h> -#include <linux/timer.h> #define SPI_ENGINE_REG_RESET 0x40 @@ -110,9 +110,7 @@ struct spi_engine { spinlock_t lock; void __iomem *base; - struct timer_list watchdog_timer; - struct spi_controller *controller; - + struct completion msg_complete; unsigned int int_enable; }; @@ -484,11 +482,9 @@ static irqreturn_t spi_engine_irq(int irq, void *devid) if (pending & SPI_ENGINE_INT_SYNC && msg) { if (completed_id == AXI_SPI_ENGINE_CUR_MSG_SYNC_ID) { - if (timer_delete_sync(&spi_engine->watchdog_timer)) { - msg->status = 0; - msg->actual_length = msg->frame_length; - spi_finalize_current_message(host); - } + msg->status = 0; + msg->actual_length = msg->frame_length; + complete(&spi_engine->msg_complete); disable_int |= SPI_ENGINE_INT_SYNC; } } @@ -559,7 +555,7 @@ static int spi_engine_transfer_one_message(struct spi_controller *host, unsigned int int_enable = 0; unsigned long flags; - mod_timer(&spi_engine->watchdog_timer, jiffies + msecs_to_jiffies(5000)); + reinit_completion(&spi_engine->msg_complete); spin_lock_irqsave(&spi_engine->lock, flags); @@ -581,21 +577,16 @@ static int spi_engine_transfer_one_message(struct spi_controller *host, spi_engine->int_enable = int_enable; spin_unlock_irqrestore(&spi_engine->lock, flags); - return 0; -} - -static void spi_engine_timeout(struct timer_list *timer) -{ - struct spi_engine *spi_engine = from_timer(spi_engine, timer, watchdog_timer); - struct spi_controller *host = spi_engine->controller; - - if (WARN_ON(!host->cur_msg)) - return; + if (!wait_for_completion_timeout(&spi_engine->msg_complete, + msecs_to_jiffies(5000))) { + dev_err(&host->dev, + "Timeout occurred while waiting for transfer to complete. Hardware is probably broken.\n"); + msg->status = -ETIMEDOUT; + } - dev_err(&host->dev, - "Timeout occurred while waiting for transfer to complete. Hardware is probably broken.\n"); - host->cur_msg->status = -ETIMEDOUT; spi_finalize_current_message(host); + + return msg->status; } static void spi_engine_release_hw(void *p) @@ -626,8 +617,7 @@ static int spi_engine_probe(struct platform_device *pdev) spi_engine = spi_controller_get_devdata(host); spin_lock_init(&spi_engine->lock); - timer_setup(&spi_engine->watchdog_timer, spi_engine_timeout, TIMER_IRQSAFE); - spi_engine->controller = host; + init_completion(&spi_engine->msg_complete); spi_engine->clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk"); if (IS_ERR(spi_engine->clk))
As a general principal, it is best to do as little as possible in an interrupt handler. This patch reworks the AXI SPI Engine driver to move timer_delete_sync() and spi_finalize_current_message() out of the interrupt handler. Instead, spi_finalize_current_message() is moved to the transfer_one_message function (similar to nearly all other SPI controllers). A completion is now used to wait for the sync interrupt that indicates that the message is complete. The watchdog timer is no longer needed since we can use the wait_for_completion_timeout() function to wait for the message to complete with the same effect. As a bonus, these changes also improve throughput of the SPI controller. For example, this was tested on a ZynqMP with a 80MHz SCLK reading 4 byte samples from an ADC. The max measured throughput increased from 26k to 28k samples per second. Signed-off-by: David Lechner <dlechner@baylibre.com> --- drivers/spi/spi-axi-spi-engine.c | 40 +++++++++++++++------------------------- 1 file changed, 15 insertions(+), 25 deletions(-)