From patchwork Wed Nov 29 16:31:56 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nam Cao X-Patchwork-Id: 13473205 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linutronix.de header.i=@linutronix.de header.b="xFPMZp13"; dkim=permerror (0-bit key) header.d=linutronix.de header.i=@linutronix.de header.b="ODU/+3aZ" Received: from galois.linutronix.de (Galois.linutronix.de [193.142.43.55]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 61F2AD5E; Wed, 29 Nov 2023 08:32:30 -0800 (PST) From: Nam Cao DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1701275549; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=i5rraSNTBdA3JVa2KGxZa5cyKj/U746068dpaHhp5sM=; b=xFPMZp13wXPtOdVOxoBeWxponbwaT5BKMMvqKzR3JBZNYj9sVn268zhBPGJZ2FHF2tPU77 l6cW8kmScf1F5R0Hxww125sM2vqvYD2JRyVFGrVAC7nb08pEAFAbB70gNNPPEZHze8Qh5y Tqi7oYVvIOfGB0BCVjtQc5MvDlFIEzHkda+ThB4d/kjMOjo9iegQHx3b81+tvQBT/Ad8wI FbZ2jKuC31oimfYwXw6zV2EzzStMnwA7Ich4iSrKR5TLS02RJp0evCG65D5gbjqiwLR0cq YQjbba7hhaH/6HGKqc5kBhXiCQpmz3xGwy1EWNhj+FRlzToK2eE04qh1TG7fIw== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1701275549; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=i5rraSNTBdA3JVa2KGxZa5cyKj/U746068dpaHhp5sM=; b=ODU/+3aZ49XzOrbMcajy8TKYypq241eAd63SbU46RRP2uPZYwzYUguEyo6ugb7LozbkKdU sY6xBY/Sc54Ve5Ag== To: linus.walleij@linaro.org, broonie@kernel.org, linux-arm-kernel@lists.infradead.org, linux-spi@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Nam Cao Subject: [PATCH v2 2/2] spi: spl022: switch to use default spi_transfer_one_message() Date: Wed, 29 Nov 2023 17:31:56 +0100 Message-Id: In-Reply-To: References: Precedence: bulk X-Mailing-List: linux-spi@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Except for polling mode, this driver's transfer_one_message() makes use of interrupt handler and tasklet. This is problematic because spi_transfer_delay_exec(), who may sleep, is called in interrupt handler and tasklet. This causes the following warning: BUG: sleeping function called from invalid context at drivers/spi/spi.c:1428 Switch to use the default spi_transfer_one_message() instead, which calls spi_transfer_delay_exec() appropriately. Signed-off-by: Nam Cao Reviewed-by: Linus Walleij --- Tested with polling mode and interrupt mode, NOT with DMA mode. Support with testing very appreciated! v2: no change drivers/spi/spi-pl022.c | 372 +++++++--------------------------------- 1 file changed, 66 insertions(+), 306 deletions(-) diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c index d1b6110b38fc..1e3bd6f3303a 100644 --- a/drivers/spi/spi-pl022.c +++ b/drivers/spi/spi-pl022.c @@ -338,7 +338,6 @@ struct vendor_data { * @clk: outgoing clock "SPICLK" for the SPI bus * @host: SPI framework hookup * @host_info: controller-specific data from machine setup - * @pump_transfers: Tasklet used in Interrupt Transfer mode * @cur_msg: Pointer to current spi_message being processed * @cur_transfer: Pointer to current spi_transfer * @cur_chip: pointer to current clients chip(assigned from controller_state) @@ -372,9 +371,6 @@ struct pl022 { struct clk *clk; struct spi_controller *host; struct pl022_ssp_controller *host_info; - /* Message per-transfer pump */ - struct tasklet_struct pump_transfers; - struct spi_message *cur_msg; struct spi_transfer *cur_transfer; struct chip_data *cur_chip; bool next_msg_cs_active; @@ -437,93 +433,23 @@ struct chip_data { * (vendor extension). Each of the 5 LSB in the register controls one chip * select signal. */ -static void internal_cs_control(struct pl022 *pl022, u32 command) +static void internal_cs_control(struct pl022 *pl022, bool enable) { u32 tmp; tmp = readw(SSP_CSR(pl022->virtbase)); - if (command == SSP_CHIP_SELECT) + if (enable) tmp &= ~BIT(pl022->cur_cs); else tmp |= BIT(pl022->cur_cs); writew(tmp, SSP_CSR(pl022->virtbase)); } -static void pl022_cs_control(struct pl022 *pl022, u32 command) +static void pl022_cs_control(struct spi_device *spi, bool enable) { + struct pl022 *pl022 = spi_controller_get_devdata(spi->controller); if (pl022->vendor->internal_cs_ctrl) - internal_cs_control(pl022, command); - else if (pl022->cur_gpiod) - /* - * This needs to be inverted since with GPIOLIB in - * control, the inversion will be handled by - * GPIOLIB's active low handling. The "command" - * passed into this function will be SSP_CHIP_SELECT - * which is enum:ed to 0, so we need the inverse - * (1) to activate chip select. - */ - gpiod_set_value(pl022->cur_gpiod, !command); -} - -/** - * giveback - current spi_message is over, schedule next message and call - * callback of this message. Assumes that caller already - * set message->status; dma and pio irqs are blocked - * @pl022: SSP driver private data structure - */ -static void giveback(struct pl022 *pl022) -{ - struct spi_transfer *last_transfer; - pl022->next_msg_cs_active = false; - - last_transfer = list_last_entry(&pl022->cur_msg->transfers, - struct spi_transfer, transfer_list); - - /* Delay if requested before any change in chip select */ - /* - * FIXME: This runs in interrupt context. - * Is this really smart? - */ - spi_transfer_delay_exec(last_transfer); - - if (!last_transfer->cs_change) { - struct spi_message *next_msg; - - /* - * cs_change was not set. We can keep the chip select - * enabled if there is message in the queue and it is - * for the same spi device. - * - * We cannot postpone this until pump_messages, because - * after calling msg->complete (below) the driver that - * sent the current message could be unloaded, which - * could invalidate the cs_control() callback... - */ - /* get a pointer to the next message, if any */ - next_msg = spi_get_next_queued_message(pl022->host); - - /* - * see if the next and current messages point - * to the same spi device. - */ - if (next_msg && next_msg->spi != pl022->cur_msg->spi) - next_msg = NULL; - if (!next_msg || pl022->cur_msg->state == STATE_ERROR) - pl022_cs_control(pl022, SSP_CHIP_DESELECT); - else - pl022->next_msg_cs_active = true; - - } - - pl022->cur_msg = NULL; - pl022->cur_transfer = NULL; - pl022->cur_chip = NULL; - - /* disable the SPI/SSP operation */ - writew((readw(SSP_CR1(pl022->virtbase)) & - (~SSP_CR1_MASK_SSE)), SSP_CR1(pl022->virtbase)); - - spi_finalize_current_message(pl022->host); + internal_cs_control(pl022, enable); } /** @@ -757,30 +683,6 @@ static void readwriter(struct pl022 *pl022) */ } -/** - * next_transfer - Move to the Next transfer in the current spi message - * @pl022: SSP driver private data structure - * - * This function moves though the linked list of spi transfers in the - * current spi message and returns with the state of current spi - * message i.e whether its last transfer is done(STATE_DONE) or - * Next transfer is ready(STATE_RUNNING) - */ -static void *next_transfer(struct pl022 *pl022) -{ - struct spi_message *msg = pl022->cur_msg; - struct spi_transfer *trans = pl022->cur_transfer; - - /* Move to next transfer */ - if (trans->transfer_list.next != &msg->transfers) { - pl022->cur_transfer = - list_entry(trans->transfer_list.next, - struct spi_transfer, transfer_list); - return STATE_RUNNING; - } - return STATE_DONE; -} - /* * This DMA functionality is only compiled in if we have * access to the generic DMA devices/DMA engine. @@ -800,7 +702,6 @@ static void unmap_free_dma_scatter(struct pl022 *pl022) static void dma_callback(void *data) { struct pl022 *pl022 = data; - struct spi_message *msg = pl022->cur_msg; BUG_ON(!pl022->sgt_rx.sgl); @@ -845,13 +746,7 @@ static void dma_callback(void *data) unmap_free_dma_scatter(pl022); - /* Update total bytes transferred */ - msg->actual_length += pl022->cur_transfer->len; - /* Move to next transfer */ - msg->state = next_transfer(pl022); - if (msg->state != STATE_DONE && pl022->cur_transfer->cs_change) - pl022_cs_control(pl022, SSP_CHIP_DESELECT); - tasklet_schedule(&pl022->pump_transfers); + spi_finalize_current_transfer(pl022->host); } static void setup_dma_scatter(struct pl022 *pl022, @@ -1189,6 +1084,9 @@ static int pl022_dma_autoprobe(struct pl022 *pl022) static void terminate_dma(struct pl022 *pl022) { + if (!pl022->dma_running) + return; + struct dma_chan *rxchan = pl022->dma_rx_channel; struct dma_chan *txchan = pl022->dma_tx_channel; @@ -1200,8 +1098,7 @@ static void terminate_dma(struct pl022 *pl022) static void pl022_dma_remove(struct pl022 *pl022) { - if (pl022->dma_running) - terminate_dma(pl022); + terminate_dma(pl022); if (pl022->dma_tx_channel) dma_release_channel(pl022->dma_tx_channel); if (pl022->dma_rx_channel) @@ -1225,6 +1122,10 @@ static inline int pl022_dma_probe(struct pl022 *pl022) return 0; } +static inline void terminate_dma(struct pl022 *pl022) +{ +} + static inline void pl022_dma_remove(struct pl022 *pl022) { } @@ -1246,16 +1147,7 @@ static inline void pl022_dma_remove(struct pl022 *pl022) static irqreturn_t pl022_interrupt_handler(int irq, void *dev_id) { struct pl022 *pl022 = dev_id; - struct spi_message *msg = pl022->cur_msg; u16 irq_status = 0; - - if (unlikely(!msg)) { - dev_err(&pl022->adev->dev, - "bad message state in interrupt handler"); - /* Never fail */ - return IRQ_HANDLED; - } - /* Read the Interrupt Status Register */ irq_status = readw(SSP_MIS(pl022->virtbase)); @@ -1287,10 +1179,8 @@ static irqreturn_t pl022_interrupt_handler(int irq, void *dev_id) writew(CLEAR_ALL_INTERRUPTS, SSP_ICR(pl022->virtbase)); writew((readw(SSP_CR1(pl022->virtbase)) & (~SSP_CR1_MASK_SSE)), SSP_CR1(pl022->virtbase)); - msg->state = STATE_ERROR; - - /* Schedule message queue handler */ - tasklet_schedule(&pl022->pump_transfers); + pl022->cur_transfer->error |= SPI_TRANS_FAIL_IO; + spi_finalize_current_transfer(pl022->host); return IRQ_HANDLED; } @@ -1318,13 +1208,7 @@ static irqreturn_t pl022_interrupt_handler(int irq, void *dev_id) "number of bytes on a 16bit bus?)\n", (u32) (pl022->rx - pl022->rx_end)); } - /* Update total bytes transferred */ - msg->actual_length += pl022->cur_transfer->len; - /* Move to next transfer */ - msg->state = next_transfer(pl022); - if (msg->state != STATE_DONE && pl022->cur_transfer->cs_change) - pl022_cs_control(pl022, SSP_CHIP_DESELECT); - tasklet_schedule(&pl022->pump_transfers); + spi_finalize_current_transfer(pl022->host); return IRQ_HANDLED; } @@ -1361,98 +1245,20 @@ static int set_up_next_transfer(struct pl022 *pl022, return 0; } -/** - * pump_transfers - Tasklet function which schedules next transfer - * when running in interrupt or DMA transfer mode. - * @data: SSP driver private data structure - * - */ -static void pump_transfers(unsigned long data) +static int do_interrupt_dma_transfer(struct pl022 *pl022) { - struct pl022 *pl022 = (struct pl022 *) data; - struct spi_message *message = NULL; - struct spi_transfer *transfer = NULL; - struct spi_transfer *previous = NULL; - - /* Get current state information */ - message = pl022->cur_msg; - transfer = pl022->cur_transfer; - - /* Handle for abort */ - if (message->state == STATE_ERROR) { - message->status = -EIO; - giveback(pl022); - return; - } - - /* Handle end of message */ - if (message->state == STATE_DONE) { - message->status = 0; - giveback(pl022); - return; - } - - /* Delay if requested at end of transfer before CS change */ - if (message->state == STATE_RUNNING) { - previous = list_entry(transfer->transfer_list.prev, - struct spi_transfer, - transfer_list); - /* - * FIXME: This runs in interrupt context. - * Is this really smart? - */ - spi_transfer_delay_exec(previous); - - /* Reselect chip select only if cs_change was requested */ - if (previous->cs_change) - pl022_cs_control(pl022, SSP_CHIP_SELECT); - } else { - /* STATE_START */ - message->state = STATE_RUNNING; - } - - if (set_up_next_transfer(pl022, transfer)) { - message->state = STATE_ERROR; - message->status = -EIO; - giveback(pl022); - return; - } - /* Flush the FIFOs and let's go! */ - flush(pl022); - - if (pl022->cur_chip->enable_dma) { - if (configure_dma(pl022)) { - dev_dbg(&pl022->adev->dev, - "configuration of DMA failed, fall back to interrupt mode\n"); - goto err_config_dma; - } - return; - } - -err_config_dma: - /* enable all interrupts except RX */ - writew(ENABLE_ALL_INTERRUPTS & ~SSP_IMSC_MASK_RXIM, SSP_IMSC(pl022->virtbase)); -} + int ret; -static void do_interrupt_dma_transfer(struct pl022 *pl022) -{ /* * Default is to enable all interrupts except RX - * this will be enabled once TX is complete */ u32 irqflags = (u32)(ENABLE_ALL_INTERRUPTS & ~SSP_IMSC_MASK_RXIM); - /* Enable target chip, if not already active */ - if (!pl022->next_msg_cs_active) - pl022_cs_control(pl022, SSP_CHIP_SELECT); + ret = set_up_next_transfer(pl022, pl022->cur_transfer); + if (ret) + return ret; - if (set_up_next_transfer(pl022, pl022->cur_transfer)) { - /* Error path */ - pl022->cur_msg->state = STATE_ERROR; - pl022->cur_msg->status = -EIO; - giveback(pl022); - return; - } /* If we're using DMA, set up DMA here */ if (pl022->cur_chip->enable_dma) { /* Configure DMA transfer */ @@ -1469,6 +1275,7 @@ static void do_interrupt_dma_transfer(struct pl022 *pl022) writew((readw(SSP_CR1(pl022->virtbase)) | SSP_CR1_MASK_SSE), SSP_CR1(pl022->virtbase)); writew(irqflags, SSP_IMSC(pl022->virtbase)); + return 1; } static void print_current_status(struct pl022 *pl022) @@ -1495,111 +1302,67 @@ static void print_current_status(struct pl022 *pl022) } -static void do_polling_transfer(struct pl022 *pl022) +static int do_polling_transfer(struct pl022 *pl022) { - struct spi_message *message = NULL; - struct spi_transfer *transfer = NULL; - struct spi_transfer *previous = NULL; + int ret; unsigned long time, timeout; - message = pl022->cur_msg; - - while (message->state != STATE_DONE) { - /* Handle for abort */ - if (message->state == STATE_ERROR) - break; - transfer = pl022->cur_transfer; - - /* Delay if requested at end of transfer */ - if (message->state == STATE_RUNNING) { - previous = - list_entry(transfer->transfer_list.prev, - struct spi_transfer, transfer_list); - spi_transfer_delay_exec(previous); - if (previous->cs_change) - pl022_cs_control(pl022, SSP_CHIP_SELECT); - } else { - /* STATE_START */ - message->state = STATE_RUNNING; - if (!pl022->next_msg_cs_active) - pl022_cs_control(pl022, SSP_CHIP_SELECT); - } - - /* Configuration Changing Per Transfer */ - if (set_up_next_transfer(pl022, transfer)) { - /* Error path */ - message->state = STATE_ERROR; - break; - } - /* Flush FIFOs and enable SSP */ - flush(pl022); - writew((readw(SSP_CR1(pl022->virtbase)) | SSP_CR1_MASK_SSE), - SSP_CR1(pl022->virtbase)); - - dev_dbg(&pl022->adev->dev, "polling transfer ongoing ...\n"); - - timeout = jiffies + msecs_to_jiffies(SPI_POLLING_TIMEOUT); - while (pl022->tx < pl022->tx_end || pl022->rx < pl022->rx_end) { - time = jiffies; - readwriter(pl022); - if (time_after(time, timeout)) { - dev_warn(&pl022->adev->dev, - "%s: timeout!\n", __func__); - message->state = STATE_TIMEOUT; - print_current_status(pl022); - goto out; - } - cpu_relax(); + /* Configuration Changing Per Transfer */ + ret = set_up_next_transfer(pl022, pl022->cur_transfer); + if (ret) + return ret; + /* Flush FIFOs and enable SSP */ + flush(pl022); + writew((readw(SSP_CR1(pl022->virtbase)) | SSP_CR1_MASK_SSE), + SSP_CR1(pl022->virtbase)); + + dev_dbg(&pl022->adev->dev, "polling transfer ongoing ...\n"); + + timeout = jiffies + msecs_to_jiffies(SPI_POLLING_TIMEOUT); + while (pl022->tx < pl022->tx_end || pl022->rx < pl022->rx_end) { + time = jiffies; + readwriter(pl022); + if (time_after(time, timeout)) { + dev_warn(&pl022->adev->dev, + "%s: timeout!\n", __func__); + print_current_status(pl022); + return -ETIMEDOUT; } - - /* Update total byte transferred */ - message->actual_length += pl022->cur_transfer->len; - /* Move to next transfer */ - message->state = next_transfer(pl022); - if (message->state != STATE_DONE - && pl022->cur_transfer->cs_change) - pl022_cs_control(pl022, SSP_CHIP_DESELECT); + cpu_relax(); } -out: - /* Handle end of message */ - if (message->state == STATE_DONE) - message->status = 0; - else if (message->state == STATE_TIMEOUT) - message->status = -EAGAIN; - else - message->status = -EIO; - giveback(pl022); - return; + return 0; } -static int pl022_transfer_one_message(struct spi_controller *host, - struct spi_message *msg) +static int pl022_transfer_one(struct spi_controller *host, struct spi_device *spi, + struct spi_transfer *transfer) { struct pl022 *pl022 = spi_controller_get_devdata(host); - /* Initial message state */ - pl022->cur_msg = msg; - msg->state = STATE_START; - - pl022->cur_transfer = list_entry(msg->transfers.next, - struct spi_transfer, transfer_list); + pl022->cur_transfer = transfer; /* Setup the SPI using the per chip configuration */ - pl022->cur_chip = spi_get_ctldata(msg->spi); - pl022->cur_cs = spi_get_chipselect(msg->spi, 0); + pl022->cur_chip = spi_get_ctldata(spi); + pl022->cur_cs = spi_get_chipselect(spi, 0); /* This is always available but may be set to -ENOENT */ - pl022->cur_gpiod = spi_get_csgpiod(msg->spi, 0); + pl022->cur_gpiod = spi_get_csgpiod(spi, 0); restore_state(pl022); flush(pl022); if (pl022->cur_chip->xfer_type == POLLING_TRANSFER) - do_polling_transfer(pl022); + return do_polling_transfer(pl022); else - do_interrupt_dma_transfer(pl022); + return do_interrupt_dma_transfer(pl022); +} - return 0; +static void pl022_handle_err(struct spi_controller *ctlr, struct spi_message *message) +{ + struct pl022 *pl022 = spi_controller_get_devdata(ctlr); + + terminate_dma(pl022); + writew(DISABLE_ALL_INTERRUPTS, SSP_IMSC(pl022->virtbase)); + writew(CLEAR_ALL_INTERRUPTS, SSP_ICR(pl022->virtbase)); } static int pl022_unprepare_transfer_hardware(struct spi_controller *host) @@ -2138,7 +1901,9 @@ static int pl022_probe(struct amba_device *adev, const struct amba_id *id) host->cleanup = pl022_cleanup; host->setup = pl022_setup; host->auto_runtime_pm = true; - host->transfer_one_message = pl022_transfer_one_message; + host->transfer_one = pl022_transfer_one; + host->set_cs = pl022_cs_control; + host->handle_err = pl022_handle_err; host->unprepare_transfer_hardware = pl022_unprepare_transfer_hardware; host->rt = platform_info->rt; host->dev.of_node = dev->of_node; @@ -2175,10 +1940,6 @@ static int pl022_probe(struct amba_device *adev, const struct amba_id *id) goto err_no_clk; } - /* Initialize transfer pump */ - tasklet_init(&pl022->pump_transfers, pump_transfers, - (unsigned long)pl022); - /* Disable SSP */ writew((readw(SSP_CR1(pl022->virtbase)) & (~SSP_CR1_MASK_SSE)), SSP_CR1(pl022->virtbase)); @@ -2261,7 +2022,6 @@ pl022_remove(struct amba_device *adev) pl022_dma_remove(pl022); amba_release_regions(adev); - tasklet_disable(&pl022->pump_transfers); } #ifdef CONFIG_PM_SLEEP