diff mbox series

spi: rockchip: Disable local irq when pio write out of interrupt service

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

Commit Message

Jon Lin June 13, 2022, 9:27 a.m. UTC
Avoid interrupt come and interrupt the pio_writer.

Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
Signed-off-by: Jon <jon.lin@rock-chips.com>
---

 drivers/spi/spi-rockchip.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Mark Brown June 13, 2022, 12:37 p.m. UTC | #1
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.
Mark Brown June 17, 2022, 11:45 a.m. UTC | #2
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?
Jon Lin June 17, 2022, 12:46 p.m. UTC | #3
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 mbox series

Patch

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");