[v3,1/2] spi: dw: Clear DMAC register when done or stopped

Commit Message

Serge Semin May 15, 2020, 5:48 p.m. UTC

If DMAC register is left uncleared any further DMAless transfers
may cause the DMAC hardware handshaking interface getting activated.
So the next DMA-based Rx/Tx transaction will be started right
after the dma_async_issue_pending() method is invoked even if no
DMATDLR/DMARDLR conditions are met. This at the same time may cause
the Tx/Rx FIFO buffers underrun/overrun. In order to fix this we
must clear DMAC register after a current DMA-based transaction is
finished.

Co-developed-by: Georgy Vlasov <[e-mail]>
Signed-off-by: Georgy Vlasov <[e-mail]>
Signed-off-by: Serge Semin <[e-mail]>
Acked-by: Mark Brown <[e-mail]>
Cc: Ramil Zaripov <[e-mail]>
Cc: Alexey Malahov <[e-mail]>

---

Changelog v2:
- Move the patch to the head of the series so one could be picked up to
  the stable kernels as a fix.
- Clear the DMACR in the DMA exit callback too.

Changelog v3:
- Rebase on top of the spi/for-5.7.
---
 drivers/spi/spi-dw-mid.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Mark Brown May 15, 2020, 5:51 p.m. UTC | #1

On Fri, May 15, 2020 at 08:48:54PM +0300, Serge Semin wrote:

> Acked-by: Mark Brown <[e-mail]>

I didn't ack this but Andy did (or at least the for-5.8 version)?

Serge Semin May 15, 2020, 5:54 p.m. UTC | #2

On Fri, May 15, 2020 at 06:51:00PM +0100, Mark Brown wrote:
> On Fri, May 15, 2020 at 08:48:54PM +0300, Serge Semin wrote:
> 
> > Acked-by: Mark Brown <[e-mail]>
> 
> I didn't ack this but Andy did (or at least the for-5.8 version)?

Andy said he needs Fixes field to get his RB tag. You said:
"This and patch 4 look good ..." Normally this means to get Acked-by.

-Sergey

Mark Brown May 15, 2020, 6:12 p.m. UTC | #3

On Fri, May 15, 2020 at 08:54:20PM +0300, Serge Semin wrote:
> On Fri, May 15, 2020 at 06:51:00PM +0100, Mark Brown wrote:
> > On Fri, May 15, 2020 at 08:48:54PM +0300, Serge Semin wrote:

> > > Acked-by: Mark Brown <[e-mail]>

> > I didn't ack this but Andy did (or at least the for-5.8 version)?

> Andy said he needs Fixes field to get his RB tag. You said:
> "This and patch 4 look good ..." Normally this means to get Acked-by.

Unless someone explicitly gives you a tag for something you shouldn't
usually assume that one applies, especially with maintainers giving acks
on their own trees since that has process meaning - it's saying that the
maintainer is OK with it being applied to some other tree which is
something it's worth being careful about.

On the other hand if someone says "if you do X you can add tag Y" and
you do X then you can generally add the tag if you do Y.

Serge Semin May 15, 2020, 6:23 p.m. UTC | #4

On Fri, May 15, 2020 at 07:12:19PM +0100, Mark Brown wrote:
> On Fri, May 15, 2020 at 08:54:20PM +0300, Serge Semin wrote:
> > On Fri, May 15, 2020 at 06:51:00PM +0100, Mark Brown wrote:
> > > On Fri, May 15, 2020 at 08:48:54PM +0300, Serge Semin wrote:
> 
> > > > Acked-by: Mark Brown <[e-mail]>
> 
> > > I didn't ack this but Andy did (or at least the for-5.8 version)?
> 
> > Andy said he needs Fixes field to get his RB tag. You said:
> > "This and patch 4 look good ..." Normally this means to get Acked-by.
> 
> Unless someone explicitly gives you a tag for something you shouldn't
> usually assume that one applies, especially with maintainers giving acks
> on their own trees since that has process meaning - it's saying that the
> maintainer is OK with it being applied to some other tree which is
> something it's worth being careful about.

Hm, it depends on maintainer and sub-system then. I did such "looks good" ->
Acked-by conversions before. Not that many, but noone argued. [1] also says it's
sometimes possible. Anyway, sorry for inconvenience. I'll get to remember that
I'd better ask explicit acked-by from you, no assumption.

[1] Documentation/process/submitting-patches.rst

-Sergey

> 
> On the other hand if someone says "if you do X you can add tag Y" and
> you do X then you can generally add the tag if you do Y.

Mark Brown May 15, 2020, 7:14 p.m. UTC | #5

On Fri, May 15, 2020 at 09:23:22PM +0300, Serge Semin wrote:
> On Fri, May 15, 2020 at 07:12:19PM +0100, Mark Brown wrote:

> > Unless someone explicitly gives you a tag for something you shouldn't
> > usually assume that one applies, especially with maintainers giving acks
> > on their own trees since that has process meaning - it's saying that the
> > maintainer is OK with it being applied to some other tree which is
> > something it's worth being careful about.

> Hm, it depends on maintainer and sub-system then. I did such "looks good" ->
> Acked-by conversions before. Not that many, but noone argued. [1] also says it's
> sometimes possible. Anyway, sorry for inconvenience. I'll get to remember that
> I'd better ask explicit acked-by from you, no assumption.

Like I say it's much more important when it's a patch that someone would
normally apply themselves since it makes a much bigger difference
process wise if you ack or apply a patch.

Mark Brown May 19, 2020, 1:40 p.m. UTC | #6

On Fri, 15 May 2020 20:48:54 +0300, Serge Semin wrote:
> If DMAC register is left uncleared any further DMAless transfers
> may cause the DMAC hardware handshaking interface getting activated.
> So the next DMA-based Rx/Tx transaction will be started right
> after the dma_async_issue_pending() method is invoked even if no
> DMATDLR/DMARDLR conditions are met. This at the same time may cause
> the Tx/Rx FIFO buffers underrun/overrun. In order to fix this we
> must clear DMAC register after a current DMA-based transaction is
> finished.

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-5.8

Thanks!

[1/2] spi: dw: Clear DMAC register when done or stopped
      commit: 0327f0b881dc5645c7ba670331e822cdaa8c5e09
[2/2] spi: dw: Fix native CS being unset
      commit: 9aea644ca17b94f82ad7fa767cbc4509642f4420

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Patch

diff --git a/drivers/spi/spi-dw-mid.c b/drivers/spi/spi-dw-mid.c
index 0d86c37e0aeb..6f55a7ae2299 100644
--- a/drivers/spi/spi-dw-mid.c
+++ b/drivers/spi/spi-dw-mid.c
@@ -86,6 +86,8 @@  static void mid_spi_dma_exit(struct dw_spi *dws)
 
 	dmaengine_terminate_sync(dws->rxchan);
 	dma_release_channel(dws->rxchan);
+
+	dw_writel(dws, DW_SPI_DMACR, 0);
 }
 
 static irqreturn_t dma_transfer(struct dw_spi *dws)
@@ -135,6 +137,8 @@  static void dw_spi_dma_tx_done(void *arg)
 	clear_bit(TX_BUSY, &dws->dma_chan_busy);
 	if (test_bit(RX_BUSY, &dws->dma_chan_busy))
 		return;
+
+	dw_writel(dws, DW_SPI_DMACR, 0);
 	spi_finalize_current_transfer(dws->master);
 }
 
@@ -181,6 +185,8 @@  static void dw_spi_dma_rx_done(void *arg)
 	clear_bit(RX_BUSY, &dws->dma_chan_busy);
 	if (test_bit(TX_BUSY, &dws->dma_chan_busy))
 		return;
+
+	dw_writel(dws, DW_SPI_DMACR, 0);
 	spi_finalize_current_transfer(dws->master);
 }
 
@@ -273,6 +279,8 @@  static void mid_spi_dma_stop(struct dw_spi *dws)
 		dmaengine_terminate_sync(dws->rxchan);
 		clear_bit(RX_BUSY, &dws->dma_chan_busy);
 	}
+
+	dw_writel(dws, DW_SPI_DMACR, 0);
 }
 
 static const struct dw_spi_dma_ops mid_dma_ops = {