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

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

[Serge Semin]

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 <Georgy.Vlasov@baikalelectronics.ru>
Signed-off-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Acked-by: Mark Brown <broonie@kernel.org>
Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>

---

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(+)

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 = {
-- 
2.25.1

[PATCH v3 2/2] spi: dw: Fix native CS being unset

[Serge Semin]

Commit 6e0a32d6f376 ("spi: dw: Fix default polarity of native
chipselect") attempted to fix the problem when GPIO active-high
chip-select is utilized to communicate with some SPI slave. It fixed
the problem, but broke the normal native CS support. At the same time
the reversion commit ada9e3fcc175 ("spi: dw: Correct handling of native
chipselect") didn't solve the problem either, since it just inverted
the set_cs() polarity perception without taking into account that
CS-high might be applicable. Here is what is done to finally fix the
problem.

DW SPI controller demands any native CS being set in order to proceed
with data transfer. So in order to activate the SPI communications we
must set any bit in the Slave Select DW SPI controller register no
matter whether the platform requests the GPIO- or native CS. Preferably
it should be the bit corresponding to the SPI slave CS number. But
currently the dw_spi_set_cs() method activates the chip-select
only if the second argument is false. Since the second argument of the
set_cs callback is expected to be a boolean with "is-high" semantics
(actual chip-select pin state value), the bit in the DW SPI Slave
Select register will be set only if SPI core requests the driver
to set the CS in the low state. So this will work for active-low
GPIO-based CS case, and won't work for active-high CS setting
the bit when SPI core actually needs to deactivate the CS.

This commit fixes the problem for all described cases. So no matter
whether an SPI slave needs GPIO- or native-based CS with active-high
or low signal the corresponding bit will be set in SER.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Fixes: ada9e3fcc175 ("spi: dw: Correct handling of native chipselect")
Fixes: 6e0a32d6f376 ("spi: dw: Fix default polarity of native chipselect")
Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Acked-by: Mark Brown <broonie@kernel.org>
Cc: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>

---

Changelog v2:
- Add a comment about SER register, that it must be set to activate the
  SPI communications.

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

diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
index 31e3f866d11a..6c2d8df50507 100644
--- a/drivers/spi/spi-dw.c
+++ b/drivers/spi/spi-dw.c
@@ -128,12 +128,20 @@ void dw_spi_set_cs(struct spi_device *spi, bool enable)
 {
 	struct dw_spi *dws = spi_controller_get_devdata(spi->controller);
 	struct chip_data *chip = spi_get_ctldata(spi);
+	bool cs_high = !!(spi->mode & SPI_CS_HIGH);
 
 	/* Chip select logic is inverted from spi_set_cs() */
 	if (chip && chip->cs_control)
 		chip->cs_control(!enable);
 
-	if (!enable)
+	/*
+	 * DW SPI controller demands any native CS being set in order to
+	 * proceed with data transfer. So in order to activate the SPI
+	 * communications we must set a corresponding bit in the Slave
+	 * Enable register no matter whether the SPI core is configured to
+	 * support active-high or active-low CS level.
+	 */
+	if (cs_high == enable)
 		dw_writel(dws, DW_SPI_SER, BIT(spi->chip_select));
 	else if (dws->cs_override)
 		dw_writel(dws, DW_SPI_SER, 0);
-- 
2.25.1

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 173 bytes --]

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

> Acked-by: Mark Brown <broonie@kernel.org>

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Serge Semin]

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 <broonie@kernel.org>
> 
> 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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 897 bytes --]

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 <broonie@kernel.org>

> > 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Serge Semin]

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 <broonie@kernel.org>
> 
> > > 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.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 955 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Mark Brown]

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