On Tue, Jan 15, 2019 at 02:26:02PM +0000, Jon Hunter wrote: > It seems that __spi_pump_messages() is getting called several times > during boot when registering the spi-flash, then after the spi-flash has > been registered, about a 1 sec later spi_pump_idle_teardown() is called > (as expected), but exits because 'ctlr->running' is true. However, > spi_pump_idle_teardown() is never called again and when we suspend we > are stuck in the busy/running state. In this case should something be > scheduling spi_pump_idle_teardown() again? Although even if it does I > don't see where the busy flag would be cleared in this path? Right, I think with the current code we just shouldn't be checking for busy in teardown, since there's now a fairly big delay between idle and actually turning the hardware off the name is just super misleading and the logic confused. I don't have time to test right now but does something like the below which changes it to a flag for the hardware being powered up work: diff --git a/drivers/spi/spi-stm32-qspi.c b/drivers/spi/spi-stm32-qspi.c index 3b2a9a6b990d..0170b0ef5d37 100644 --- a/drivers/spi/spi-stm32-qspi.c +++ b/drivers/spi/spi-stm32-qspi.c @@ -358,9 +358,6 @@ static int stm32_qspi_setup(struct spi_device *spi) struct stm32_qspi_flash *flash; u32 cr, presc; - if (ctrl->busy) - return -EBUSY; - if (!spi->max_speed_hz) return -EINVAL; diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c index 5f19016bbf10..3d746a0782eb 100644 --- a/drivers/spi/spi-ti-qspi.c +++ b/drivers/spi/spi-ti-qspi.c @@ -152,11 +152,6 @@ static int ti_qspi_setup(struct spi_device *spi) int clk_div = 0, ret; u32 clk_ctrl_reg, clk_rate, clk_mask; - if (spi->master->busy) { - dev_dbg(qspi->dev, "master busy doing other transfers\n"); - return -EBUSY; - } - if (!qspi->spi_max_frequency) { dev_err(qspi->dev, "spi max frequency not defined\n"); return -EINVAL; diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c index 9f83e1b17aa1..a173a3114813 100644 --- a/drivers/spi/spi-zynqmp-gqspi.c +++ b/drivers/spi/spi-zynqmp-gqspi.c @@ -487,22 +487,6 @@ static int zynqmp_qspi_setup_transfer(struct spi_device *qspi, return 0; } -/** - * zynqmp_qspi_setup: Configure the QSPI controller - * @qspi: Pointer to the spi_device structure - * - * Sets the operational mode of QSPI controller for the next QSPI transfer, - * baud rate and divisor value to setup the requested qspi clock. - * - * Return: 0 on success; error value otherwise. - */ -static int zynqmp_qspi_setup(struct spi_device *qspi) -{ - if (qspi->master->busy) - return -EBUSY; - return 0; -} - /** * zynqmp_qspi_filltxfifo: Fills the TX FIFO as long as there is room in * the FIFO or the bytes required to be @@ -1088,7 +1072,6 @@ static int zynqmp_qspi_probe(struct platform_device *pdev) master->num_chipselect = GQSPI_DEFAULT_NUM_CS; - master->setup = zynqmp_qspi_setup; master->set_cs = zynqmp_qspi_chipselect; master->transfer_one = zynqmp_qspi_start_transfer; master->prepare_transfer_hardware = zynqmp_prepare_transfer_hardware; diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 06b9139664a3..6d10ad9d768f 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1234,7 +1234,7 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread) /* Check if the queue is idle */ if (list_empty(&ctlr->queue) || !ctlr->running) { - if (!ctlr->busy) { + if (!ctlr->hw_active) { spin_unlock_irqrestore(&ctlr->queue_lock, flags); return; } @@ -1252,10 +1252,10 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread) list_first_entry(&ctlr->queue, struct spi_message, queue); list_del_init(&ctlr->cur_msg->queue); - if (ctlr->busy) + if (ctlr->hw_active) was_busy = true; else - ctlr->busy = true; + ctlr->hw_active = true; spin_unlock_irqrestore(&ctlr->queue_lock, flags); mutex_lock(&ctlr->io_mutex); @@ -1362,10 +1362,6 @@ static void spi_pump_idle_teardown(struct kthread_work *work) if (ctlr->running) goto out; - /* if the controller is busy then exit */ - if (ctlr->busy) - goto out; - /* if the controller is idling then exit * this is actually a bit strange and would indicate that * this function is scheduled twice, which should not happen @@ -1374,7 +1370,6 @@ static void spi_pump_idle_teardown(struct kthread_work *work) goto out; /* set up the initial states */ - ctlr->busy = false; ctlr->idling = true; spin_unlock_irqrestore(&ctlr->queue_lock, flags); @@ -1384,15 +1379,17 @@ static void spi_pump_idle_teardown(struct kthread_work *work) kfree(ctlr->dummy_tx); ctlr->dummy_tx = NULL; - /* unprepare hardware */ - if (ctlr->unprepare_transfer_hardware && - ctlr->unprepare_transfer_hardware(ctlr)) - dev_err(&ctlr->dev, - "failed to unprepare transfer hardware\n"); - /* handle pm */ - if (ctlr->auto_runtime_pm) { - pm_runtime_mark_last_busy(ctlr->dev.parent); - pm_runtime_put_autosuspend(ctlr->dev.parent); + if (ctlr->hw_active) { + /* unprepare hardware */ + if (ctlr->unprepare_transfer_hardware && + ctlr->unprepare_transfer_hardware(ctlr)) + dev_err(&ctlr->dev, + "failed to unprepare transfer hardware\n"); + /* handle pm */ + if (ctlr->auto_runtime_pm) { + pm_runtime_mark_last_busy(ctlr->dev.parent); + pm_runtime_put_autosuspend(ctlr->dev.parent); + } } /* mark controller as idle */ @@ -1401,6 +1398,7 @@ static void spi_pump_idle_teardown(struct kthread_work *work) /* finally put us from idling into stopped */ spin_lock_irqsave(&ctlr->queue_lock, flags); ctlr->idling = false; + ctlr->hw_active = false; out: spin_unlock_irqrestore(&ctlr->queue_lock, flags); @@ -1411,7 +1409,7 @@ static int spi_init_queue(struct spi_controller *ctlr) struct sched_param param = { .sched_priority = MAX_RT_PRIO - 1 }; ctlr->running = false; - ctlr->busy = false; + ctlr->hw_active = false; kthread_init_worker(&ctlr->kworker); ctlr->kworker_task = kthread_run(kthread_worker_fn, &ctlr->kworker, @@ -1520,7 +1518,7 @@ static int spi_start_queue(struct spi_controller *ctlr) spin_lock_irqsave(&ctlr->queue_lock, flags); - if (ctlr->running || ctlr->busy) { + if (ctlr->running || ctlr->hw_active) { spin_unlock_irqrestore(&ctlr->queue_lock, flags); return -EBUSY; } @@ -1543,18 +1541,19 @@ static int spi_stop_queue(struct spi_controller *ctlr) spin_lock_irqsave(&ctlr->queue_lock, flags); /* - * This is a bit lame, but is optimized for the common execution path. - * A wait_queue on the ctlr->busy could be used, but then the common - * execution path (pump_messages) would be required to call wake_up or - * friends on every SPI message. Do this instead. + * This is a bit sad, but is optimized for the common execution path. + * A wait_queue on the ctlr->hw_active could be used, but then + * the common execution path (pump_messages) would be required + * to call wake_up or friends on every SPI message. Do this + * instead. */ - while ((!list_empty(&ctlr->queue) || ctlr->busy) && limit--) { + while ((!list_empty(&ctlr->queue) || ctlr->hw_active) && limit--) { spin_unlock_irqrestore(&ctlr->queue_lock, flags); usleep_range(10000, 11000); spin_lock_irqsave(&ctlr->queue_lock, flags); } - if (!list_empty(&ctlr->queue) || ctlr->busy) + if (!list_empty(&ctlr->queue) || ctlr->hw_active) ret = -EBUSY; else ctlr->running = false; diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 79ad62e2487c..d9b7be89e50b 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -343,7 +343,7 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv) * in-flight message * @cur_msg_mapped: message has been mapped for DMA * @xfer_completion: used by core transfer_one_message() - * @busy: message pump is busy + * @hw_active: message pump has hardware powered up * @running: message pump is running * @rt: whether this queue is set to run as a realtime task * @auto_runtime_pm: the core should ensure a runtime PM reference is held @@ -538,7 +538,7 @@ struct spi_controller { struct list_head queue; struct spi_message *cur_msg; bool idling; - bool busy; + bool hw_active; bool running; bool rt; bool auto_runtime_pm;