| Message ID | 20220613092744.9726-1-jon.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: rockchip: Disable local irq when pio write out of interrupt service | expand |
On Mon, Jun 13, 2022 at 05:27:44PM +0800, Jon Lin wrote: > Avoid interrupt come and interrupt the pio_writer. > > + spin_lock_irqsave(&rs->lock, flags); > + tx_free = rs->fifo_len - readl_relaxed(rs->regs + ROCKCHIP_SPI_TXFLR); > + words = min(rs->tx_left, tx_free); > rs->tx_left -= words; > for (; words; words--) { > u32 txw; > @@ -308,6 +313,7 @@ static void rockchip_spi_pio_writer(struct rockchip_spi *rs) > writel_relaxed(txw, rs->regs + ROCKCHIP_SPI_TXDR); > rs->tx += rs->n_bytes; > } > + spin_unlock_irqrestore(&rs->lock, flags); So this is effectively just disabling interrupts during PIO, there's no other users of the lock which is rather heavyweight. What's the actual issue here? We should also have something saying what's going on in the code since right now the lock just looks redundant.
On Fri, Jun 17, 2022 at 02:24:10PM +0800, Jon Lin wrote: > On 2022/6/13 20:37, Mark Brown wrote: > > On Mon, Jun 13, 2022 at 05:27:44PM +0800, Jon Lin wrote: > > > + spin_unlock_irqrestore(&rs->lock, flags); > > So this is effectively just disabling interrupts during PIO, there's no > > other users of the lock which is rather heavyweight. What's the actual > > issue here? We should also have something saying what's going on in the > > code since right now the lock just looks redundant. > For lock: In order to avoid special situations, such as when the CPU > frequency drops to close to the IO rate, the water line interrupt is > triggered during FIFO filling (triggered by other CPUs), resulting in > critical resources still not being protected in place. For local IRQ So essentially we're so slow in filling the FIFO when starting a transfer that the interrupt triggers in the middle of the initial FIFO fill? Something that tricky *really* needs a comment adding. Ideally we'd just leave the interrupt masked until the FIFO is filled though, looking at the driver I see that there is an interrupt mask register which seems to have some level of masking available and I do note that in rockchip_spi_prepare_irq() we unmask interrupts before we start filling the FIFO rather than afterwards. Would reversing the unmask order there address the issue more cleanly? > disable: Turning off the local interrupt is mainly to prevent the CPU > schedule from being interrupted when filling FIFO. If it were just this then there's preempt_disable(), but what's the problem with being preempted during the FIFO fill?
On 2022/6/17 19:45, Mark Brown wrote: > On Fri, Jun 17, 2022 at 02:24:10PM +0800, Jon Lin wrote: >> On 2022/6/13 20:37, Mark Brown wrote: >>> On Mon, Jun 13, 2022 at 05:27:44PM +0800, Jon Lin wrote: > >>>> + spin_unlock_irqrestore(&rs->lock, flags); > >>> So this is effectively just disabling interrupts during PIO, there's no >>> other users of the lock which is rather heavyweight. What's the actual >>> issue here? We should also have something saying what's going on in the >>> code since right now the lock just looks redundant. > >> For lock: In order to avoid special situations, such as when the CPU >> frequency drops to close to the IO rate, the water line interrupt is >> triggered during FIFO filling (triggered by other CPUs), resulting in >> critical resources still not being protected in place. For local IRQ > > So essentially we're so slow in filling the FIFO when starting a > transfer that the interrupt triggers in the middle of the initial FIFO > fill? Something that tricky *really* needs a comment adding. > > Ideally we'd just leave the interrupt masked until the FIFO is filled > though, looking at the driver I see that there is an interrupt mask > register which seems to have some level of masking available and I do > note that in rockchip_spi_prepare_irq() we unmask interrupts before we > start filling the FIFO rather than afterwards. Would reversing the > unmask order there address the issue more cleanly? This idea is workable, and it's more efficient than previous code, So I send a new commit: https://patchwork.kernel.org/project/spi-devel-general/patch/20220617124251.5051-1-jon.lin@rock-chips.com/ > >> disable: Turning off the local interrupt is mainly to prevent the CPU >> schedule from being interrupted when filling FIFO. > > If it were just this then there's preempt_disable(), but what's the > problem with being preempted during the FIFO fill? I think
diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c index a08215eb9e14..2184de146928 100644 --- a/drivers/spi/spi-rockchip.c +++ b/drivers/spi/spi-rockchip.c @@ -199,6 +199,7 @@ struct rockchip_spi { bool cs_high_supported; /* native CS supports active-high polarity */ struct spi_transfer *xfer; /* Store xfer temporarily */ + spinlock_t lock; /* prevent I/O concurrent access */ }; static inline void spi_enable_chip(struct rockchip_spi *rs, bool enable) @@ -293,9 +294,13 @@ static void rockchip_spi_handle_err(struct spi_controller *ctlr, static void rockchip_spi_pio_writer(struct rockchip_spi *rs) { - u32 tx_free = rs->fifo_len - readl_relaxed(rs->regs + ROCKCHIP_SPI_TXFLR); - u32 words = min(rs->tx_left, tx_free); + u32 tx_free; + u32 words; + unsigned long flags; + spin_lock_irqsave(&rs->lock, flags); + tx_free = rs->fifo_len - readl_relaxed(rs->regs + ROCKCHIP_SPI_TXFLR); + words = min(rs->tx_left, tx_free); rs->tx_left -= words; for (; words; words--) { u32 txw; @@ -308,6 +313,7 @@ static void rockchip_spi_pio_writer(struct rockchip_spi *rs) writel_relaxed(txw, rs->regs + ROCKCHIP_SPI_TXDR); rs->tx += rs->n_bytes; } + spin_unlock_irqrestore(&rs->lock, flags); } static void rockchip_spi_pio_reader(struct rockchip_spi *rs) @@ -779,6 +785,8 @@ static int rockchip_spi_probe(struct platform_device *pdev) goto err_put_ctlr; } + spin_lock_init(&rs->lock); + rs->apb_pclk = devm_clk_get(&pdev->dev, "apb_pclk"); if (IS_ERR(rs->apb_pclk)) { dev_err(&pdev->dev, "Failed to get apb_pclk\n");