linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 00/16] spi: dw: Add generic DW DMA controller support
@ 2020-05-21  1:21 Serge Semin
  2020-05-21  1:21 ` [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA Serge Semin
                   ` (10 more replies)
  0 siblings, 11 replies; 32+ messages in thread
From: Serge Semin @ 2020-05-21  1:21 UTC (permalink / raw)
  To: Mark Brown
  Cc: Serge Semin, Serge Semin, Georgy Vlasov, Ramil Zaripov,
	Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko,
	Ekaterina Skachko, Vadim Vlasov, Alexey Kolotnikov,
	Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Arnd Bergmann,
	Andy Shevchenko, Rob Herring, linux-mips, linux-spi, devicetree,
	linux-kernel

Baikal-T1 SoC provides a DW DMA controller to perform low-speed peripherals
Mem-to-Dev and Dev-to-Mem transaction. This is also applicable to the DW
APB SSI devices embedded into the SoC. Currently the DMA-based transfers
are supported by the DW APB SPI driver only as a middle layer code for
Intel MID/Elkhart PCI devices. Seeing the same code can be used for normal
platform DMAC device we introduced a set of patches to fix it within this
series.

First of all we need to add the Tx and Rx DMA channels support into the DW
APB SSI binding. Then there are several fixes and cleanups provided as a
initial preparation for the Generic DMA support integration: add Tx/Rx
finish wait methods, clear DMAC register when done or stopped, Fix native
CS being unset, enable interrupts in accordance with DMA xfer mode,
discard static DW DMA slave structures, discard unused void priv pointer
and dma_width member of the dw_spi structure, provide the DMA Tx/Rx burst
length parametrisation and make sure it's optionally set in accordance
with the DMA max-burst capability.

In order to have the DW APB SSI MMIO driver working with DMA we need to
initialize the paddr field with the physical base address of the DW APB SSI
registers space. Then we unpin the Intel MID specific code from the
generic DMA one and placed it into the spi-dw-pci.c driver, which is a
better place for it anyway. After that the naming cleanups are performed
since the code is going to be used for a generic DMAC device. Finally the
Generic DMA initialization can be added to the generic version of the
DW APB SSI IP.

Last but not least we traditionally convert the legacy plain text-based
dt-binding file with yaml-based one and as a cherry on a cake replace
the manually written DebugFS registers read method with a ready-to-use
for the same purpose regset32 DebugFS interface usage.

This patchset is rebased and tested on the spi/for-next (5.7-rc5):
base-commit: fe9fce6b2cf3 ("Merge remote-tracking branch 'spi/for-5.8' into spi-next")

Link: https://lore.kernel.org/linux-spi/20200508132943.9826-1-Sergey.Semin@baikalelectronics.ru/
Changelog v2:
- Rebase on top of the spi repository for-next branch.
- Move bindings conversion patch to the tail of the series.
- Move fixes to the head of the series.
- Apply as many changes as possible to be applied the Generic DMA
  functionality support is added and the spi-dw-mid is moved to the
  spi-dw-dma driver.
- Discard patch "spi: dw: Fix dma_slave_config used partly uninitialized"
  since the problem has already been fixed.
- Add new patch "spi: dw: Discard unused void priv pointer".
- Add new patch "spi: dw: Discard dma_width member of the dw_spi structure".
  n_bytes member of the DW SPI data can be used instead.
- Build the DMA functionality into the DW APB SSI core if required instead
  of creating a separate kernel module.
- Use conditional statement instead of the ternary operator in the ref
  clock getter.

Link: https://lore.kernel.org/linux-spi/20200515104758.6934-1-Sergey.Semin@baikalelectronics.ru/
Changelog v3:
- Use spi_delay_exec() method to wait for the DMA operation completion.
- Explicitly initialize the dw_dma_slave members on stack.
- Discard the dws->fifo_len utilization in the Tx FIFO DMA threshold
  setting from the patch where we just add the default burst length
  constants.
- Use min() method to calculate the optimal burst values.
- Add new patch which moves the spi-dw.c source file to spi-dw-core.c in
  order to preserve the DW APB SSI core driver name.
- Add commas in the debugfs_reg32 structure initializer and after the last
  entry of the dw_spi_dbgfs_regs array.

Co-developed-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
Signed-off-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
Co-developed-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
Signed-off-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Maxim Kaurkin <Maxim.Kaurkin@baikalelectronics.ru>
Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
Cc: Ekaterina Skachko <Ekaterina.Skachko@baikalelectronics.ru>
Cc: Vadim Vlasov <V.Vlasov@baikalelectronics.ru>
Cc: Alexey Kolotnikov <Alexey.Kolotnikov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-mips@vger.kernel.org
Cc: linux-spi@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

Serge Semin (16):
  spi: dw: Add Tx/Rx finish wait methods to the MID DMA
  spi: dw: Enable interrupts in accordance with DMA xfer mode
  spi: dw: Discard static DW DMA slave structures
  spi: dw: Discard unused void priv pointer
  spi: dw: Discard dma_width member of the dw_spi structure
  spi: dw: Parameterize the DMA Rx/Tx burst length
  spi: dw: Use DMA max burst to set the request thresholds
  spi: dw: Fix Rx-only DMA transfers
  spi: dw: Add core suffix to the DW APB SSI core source file
  spi: dw: Move Non-DMA code to the DW PCIe-SPI driver
  spi: dw: Remove DW DMA code dependency from DW_DMAC_PCI
  spi: dw: Add DW SPI DMA/PCI/MMIO dependency on the DW SPI core
  spi: dw: Cleanup generic DW DMA code namings
  spi: dw: Add DMA support to the DW SPI MMIO driver
  spi: dw: Use regset32 DebugFS method to create regdump file
  dt-bindings: spi: Convert DW SPI binding to DT schema

 .../bindings/spi/snps,dw-apb-ssi.txt          |  44 ---
 .../bindings/spi/snps,dw-apb-ssi.yaml         | 127 ++++++++
 .../devicetree/bindings/spi/spi-dw.txt        |  24 --
 drivers/spi/Kconfig                           |  15 +-
 drivers/spi/Makefile                          |   5 +-
 drivers/spi/{spi-dw.c => spi-dw-core.c}       |  88 ++----
 drivers/spi/{spi-dw-mid.c => spi-dw-dma.c}    | 276 +++++++++++-------
 drivers/spi/spi-dw-mmio.c                     |   4 +
 drivers/spi/spi-dw-pci.c                      |  50 +++-
 drivers/spi/spi-dw.h                          |  33 ++-
 10 files changed, 407 insertions(+), 259 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt
 create mode 100644 Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
 delete mode 100644 Documentation/devicetree/bindings/spi/spi-dw.txt
 rename drivers/spi/{spi-dw.c => spi-dw-core.c} (82%)
 rename drivers/spi/{spi-dw-mid.c => spi-dw-dma.c} (53%)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
  2020-05-21  1:21 [PATCH v3 00/16] spi: dw: Add generic DW DMA controller support Serge Semin
@ 2020-05-21  1:21 ` Serge Semin
  2020-05-21  3:09   ` Feng Tang
  2020-05-21 23:33   ` Serge Semin
  2020-05-21  1:21 ` [PATCH v3 02/16] spi: dw: Enable interrupts in accordance with DMA xfer mode Serge Semin
                   ` (9 subsequent siblings)
  10 siblings, 2 replies; 32+ messages in thread
From: Serge Semin @ 2020-05-21  1:21 UTC (permalink / raw)
  To: Mark Brown, Grant Likely, Vinod Koul, Feng Tang, Alan Cox, Linus Walleij
  Cc: Serge Semin, Serge Semin, Georgy Vlasov, Ramil Zaripov,
	Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
	Arnd Bergmann, Andy Shevchenko, Rob Herring, linux-mips,
	devicetree, Jarkko Nikula, Thomas Gleixner, Wan Ahmad Zainie,
	Linus Walleij, Clement Leger, linux-spi, linux-kernel

Since DMA transfers are performed asynchronously with actual SPI
transaction, then even if DMA transfers are finished it doesn't mean
all data is actually pushed to the SPI bus. Some data might still be
in the controller FIFO. This is specifically true for Tx-only
transfers. In this case if the next SPI transfer is recharged while
a tail of the previous one is still in FIFO, we'll loose that tail
data. In order to fix this lets add the wait procedure of the Tx/Rx
SPI transfers completion after the corresponding DMA transactions
are 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>
Fixes: 7063c0d942a1 ("spi/dw_spi: add DMA support")
Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-mips@vger.kernel.org
Cc: devicetree@vger.kernel.org

---

Changelog v2:
- Use conditional statement instead of the ternary operator in the ref
  clock getter.
- Move the patch to the head of the series so one could be picked up to
  the stable kernels as a fix.

Changelog v3:
- Use spi_delay_exec() method to wait for the current operation completion.
---
 drivers/spi/spi-dw-mid.c | 69 ++++++++++++++++++++++++++++++++++++++++
 drivers/spi/spi-dw.h     | 10 ++++++
 2 files changed, 79 insertions(+)

diff --git a/drivers/spi/spi-dw-mid.c b/drivers/spi/spi-dw-mid.c
index f9757a370699..3526b196a7fc 100644
--- a/drivers/spi/spi-dw-mid.c
+++ b/drivers/spi/spi-dw-mid.c
@@ -17,6 +17,7 @@
 #include <linux/pci.h>
 #include <linux/platform_data/dma-dw.h>
 
+#define WAIT_RETRIES	5
 #define RX_BUSY		0
 #define TX_BUSY		1
 
@@ -143,6 +144,47 @@ static enum dma_slave_buswidth convert_dma_width(u32 dma_width) {
 	return DMA_SLAVE_BUSWIDTH_UNDEFINED;
 }
 
+static void dw_spi_dma_calc_delay(struct dw_spi *dws, u32 nents,
+				  struct spi_delay *delay)
+{
+	unsigned long ns, us;
+
+	ns = (NSEC_PER_SEC / spi_get_clk(dws)) * nents * dws->n_bytes *
+	     BITS_PER_BYTE;
+
+	if (ns <= NSEC_PER_USEC) {
+		delay->unit = SPI_DELAY_UNIT_NSECS;
+		delay->value = ns;
+	} else {
+		us = DIV_ROUND_UP(ns, NSEC_PER_USEC);
+		delay->unit = SPI_DELAY_UNIT_USECS;
+		delay->value = clamp_val(us, 0, USHRT_MAX);
+	}
+}
+
+static inline bool dw_spi_dma_tx_busy(struct dw_spi *dws)
+{
+	return !(dw_readl(dws, DW_SPI_SR) & SR_TF_EMPT);
+}
+
+static void dw_spi_dma_wait_tx_done(struct dw_spi *dws)
+{
+	int retry = WAIT_RETRIES;
+	struct spi_delay delay;
+	u32 nents;
+
+	nents = dw_readl(dws, DW_SPI_TXFLR);
+	dw_spi_dma_calc_delay(dws, nents, &delay);
+
+	while (dw_spi_dma_tx_busy(dws) && retry--)
+		spi_delay_exec(&delay, NULL);
+
+	if (retry < 0) {
+		dev_err(&dws->master->dev, "Tx hanged up\n");
+		dws->master->cur_msg->status = -EIO;
+	}
+}
+
 /*
  * dws->dma_chan_busy is set before the dma transfer starts, callback for tx
  * channel will clear a corresponding bit.
@@ -151,6 +193,8 @@ static void dw_spi_dma_tx_done(void *arg)
 {
 	struct dw_spi *dws = arg;
 
+	dw_spi_dma_wait_tx_done(dws);
+
 	clear_bit(TX_BUSY, &dws->dma_chan_busy);
 	if (test_bit(RX_BUSY, &dws->dma_chan_busy))
 		return;
@@ -192,6 +236,29 @@ static struct dma_async_tx_descriptor *dw_spi_dma_prepare_tx(struct dw_spi *dws,
 	return txdesc;
 }
 
+static inline bool dw_spi_dma_rx_busy(struct dw_spi *dws)
+{
+	return !!(dw_readl(dws, DW_SPI_SR) & SR_RF_NOT_EMPT);
+}
+
+static void dw_spi_dma_wait_rx_done(struct dw_spi *dws)
+{
+	int retry = WAIT_RETRIES;
+	struct spi_delay delay;
+	u32 nents;
+
+	nents = dw_readl(dws, DW_SPI_RXFLR);
+	dw_spi_dma_calc_delay(dws, nents, &delay);
+
+	while (dw_spi_dma_rx_busy(dws) && retry--)
+		spi_delay_exec(&delay, NULL);
+
+	if (retry < 0) {
+		dev_err(&dws->master->dev, "Rx hanged up\n");
+		dws->master->cur_msg->status = -EIO;
+	}
+}
+
 /*
  * dws->dma_chan_busy is set before the dma transfer starts, callback for rx
  * channel will clear a corresponding bit.
@@ -200,6 +267,8 @@ static void dw_spi_dma_rx_done(void *arg)
 {
 	struct dw_spi *dws = arg;
 
+	dw_spi_dma_wait_rx_done(dws);
+
 	clear_bit(RX_BUSY, &dws->dma_chan_busy);
 	if (test_bit(TX_BUSY, &dws->dma_chan_busy))
 		return;
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index e92d43b9a9e6..81364f501b7e 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -210,6 +210,16 @@ static inline void spi_set_clk(struct dw_spi *dws, u16 div)
 	dw_writel(dws, DW_SPI_BAUDR, div);
 }
 
+static inline u32 spi_get_clk(struct dw_spi *dws)
+{
+	u32 div = dw_readl(dws, DW_SPI_BAUDR);
+
+	if (!div)
+		return 0;
+
+	return dws->max_freq / div;
+}
+
 /* Disable IRQ bits */
 static inline void spi_mask_intr(struct dw_spi *dws, u32 mask)
 {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v3 02/16] spi: dw: Enable interrupts in accordance with DMA xfer mode
  2020-05-21  1:21 [PATCH v3 00/16] spi: dw: Add generic DW DMA controller support Serge Semin
  2020-05-21  1:21 ` [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA Serge Semin
@ 2020-05-21  1:21 ` Serge Semin
  2020-05-21  1:21 ` [PATCH v3 03/16] spi: dw: Discard static DW DMA slave structures Serge Semin
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 32+ messages in thread
From: Serge Semin @ 2020-05-21  1:21 UTC (permalink / raw)
  To: Mark Brown
  Cc: Serge Semin, Serge Semin, Georgy Vlasov, Ramil Zaripov,
	Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
	Arnd Bergmann, Andy Shevchenko, Rob Herring, linux-mips,
	devicetree, Jarkko Nikula, Thomas Gleixner, Wan Ahmad Zainie,
	linux-spi, linux-kernel

It's pointless to track the Tx overrun interrupts if Rx-only SPI
transfer is issued. Similarly there is no need in handling the Rx
overrun/underrun interrupts if Tx-only SPI transfer is executed.
So lets unmask the interrupts only if corresponding SPI
transactions are implied.

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>
Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-mips@vger.kernel.org
Cc: devicetree@vger.kernel.org
---
 drivers/spi/spi-dw-mid.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-dw-mid.c b/drivers/spi/spi-dw-mid.c
index 3526b196a7fc..cf99832ba271 100644
--- a/drivers/spi/spi-dw-mid.c
+++ b/drivers/spi/spi-dw-mid.c
@@ -312,19 +312,23 @@ static struct dma_async_tx_descriptor *dw_spi_dma_prepare_rx(struct dw_spi *dws,
 
 static int mid_spi_dma_setup(struct dw_spi *dws, struct spi_transfer *xfer)
 {
-	u16 dma_ctrl = 0;
+	u16 imr = 0, dma_ctrl = 0;
 
 	dw_writel(dws, DW_SPI_DMARDLR, 0xf);
 	dw_writel(dws, DW_SPI_DMATDLR, 0x10);
 
-	if (xfer->tx_buf)
+	if (xfer->tx_buf) {
 		dma_ctrl |= SPI_DMA_TDMAE;
-	if (xfer->rx_buf)
+		imr |= SPI_INT_TXOI;
+	}
+	if (xfer->rx_buf) {
 		dma_ctrl |= SPI_DMA_RDMAE;
+		imr |= SPI_INT_RXUI | SPI_INT_RXOI;
+	}
 	dw_writel(dws, DW_SPI_DMACR, dma_ctrl);
 
 	/* Set the interrupt mask */
-	spi_umask_intr(dws, SPI_INT_TXOI | SPI_INT_RXUI | SPI_INT_RXOI);
+	spi_umask_intr(dws, imr);
 
 	dws->transfer_handler = dma_transfer;
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v3 03/16] spi: dw: Discard static DW DMA slave structures
  2020-05-21  1:21 [PATCH v3 00/16] spi: dw: Add generic DW DMA controller support Serge Semin
  2020-05-21  1:21 ` [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA Serge Semin
  2020-05-21  1:21 ` [PATCH v3 02/16] spi: dw: Enable interrupts in accordance with DMA xfer mode Serge Semin
@ 2020-05-21  1:21 ` Serge Semin
  2020-05-21  9:57   ` Andy Shevchenko
  2020-05-21  1:21 ` [PATCH v3 06/16] spi: dw: Parameterize the DMA Rx/Tx burst length Serge Semin
                   ` (7 subsequent siblings)
  10 siblings, 1 reply; 32+ messages in thread
From: Serge Semin @ 2020-05-21  1:21 UTC (permalink / raw)
  To: Mark Brown
  Cc: Serge Semin, Serge Semin, Georgy Vlasov, Ramil Zaripov,
	Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
	Andy Shevchenko, Arnd Bergmann, Rob Herring, linux-mips,
	devicetree, Thomas Gleixner, Wan Ahmad Zainie, Jarkko Nikula,
	Clement Leger, linux-spi, linux-kernel

Having them declared is redundant since each struct dw_dma_chan has
the same structure embedded and the structure from the passed dma_chan
private pointer will be copied there as a result of the next calls
chain:
dma_request_channel() -> find_candidate() -> dma_chan_get() ->
device_alloc_chan_resources() = dwc_alloc_chan_resources() ->
dw_dma_filter().
So just remove the static dw_dma_chan structures and use a locally
declared data instance with dst_id/src_id set to the same values as
the static copies used to have.

Co-developed-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
Signed-off-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
Co-developed-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
Signed-off-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-mips@vger.kernel.org
Cc: devicetree@vger.kernel.org

---

Changelog v3:
- Explicitly initialize the dw_dma_slave members on stack.
---
 drivers/spi/spi-dw-mid.c | 19 ++++++++-----------
 drivers/spi/spi-dw.h     |  2 --
 2 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/spi/spi-dw-mid.c b/drivers/spi/spi-dw-mid.c
index cf99832ba271..8446bad0528c 100644
--- a/drivers/spi/spi-dw-mid.c
+++ b/drivers/spi/spi-dw-mid.c
@@ -21,9 +21,6 @@
 #define RX_BUSY		0
 #define TX_BUSY		1
 
-static struct dw_dma_slave mid_dma_tx = { .dst_id = 1 };
-static struct dw_dma_slave mid_dma_rx = { .src_id = 0 };
-
 static bool mid_spi_dma_chan_filter(struct dma_chan *chan, void *param)
 {
 	struct dw_dma_slave *s = param;
@@ -37,9 +34,11 @@ static bool mid_spi_dma_chan_filter(struct dma_chan *chan, void *param)
 
 static int mid_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
 {
+	struct dw_dma_slave slave = {
+		.src_id = 0,
+		.dst_id = 0
+	};
 	struct pci_dev *dma_dev;
-	struct dw_dma_slave *tx = dws->dma_tx;
-	struct dw_dma_slave *rx = dws->dma_rx;
 	dma_cap_mask_t mask;
 
 	/*
@@ -54,14 +53,14 @@ static int mid_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
 	dma_cap_set(DMA_SLAVE, mask);
 
 	/* 1. Init rx channel */
-	rx->dma_dev = &dma_dev->dev;
-	dws->rxchan = dma_request_channel(mask, mid_spi_dma_chan_filter, rx);
+	slave.dma_dev = &dma_dev->dev;
+	dws->rxchan = dma_request_channel(mask, mid_spi_dma_chan_filter, &slave);
 	if (!dws->rxchan)
 		goto err_exit;
 
 	/* 2. Init tx channel */
-	tx->dma_dev = &dma_dev->dev;
-	dws->txchan = dma_request_channel(mask, mid_spi_dma_chan_filter, tx);
+	slave.dst_id = 1;
+	dws->txchan = dma_request_channel(mask, mid_spi_dma_chan_filter, &slave);
 	if (!dws->txchan)
 		goto free_rxchan;
 
@@ -386,8 +385,6 @@ static const struct dw_spi_dma_ops mfld_dma_ops = {
 
 static void dw_spi_mid_setup_dma_mfld(struct dw_spi *dws)
 {
-	dws->dma_tx = &mid_dma_tx;
-	dws->dma_rx = &mid_dma_rx;
 	dws->dma_ops = &mfld_dma_ops;
 }
 
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index 81364f501b7e..60e9e430ce7b 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -146,8 +146,6 @@ struct dw_spi {
 	unsigned long		dma_chan_busy;
 	dma_addr_t		dma_addr; /* phy address of the Data register */
 	const struct dw_spi_dma_ops *dma_ops;
-	void			*dma_tx;
-	void			*dma_rx;
 
 	/* Bus interface info */
 	void			*priv;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v3 06/16] spi: dw: Parameterize the DMA Rx/Tx burst length
  2020-05-21  1:21 [PATCH v3 00/16] spi: dw: Add generic DW DMA controller support Serge Semin
                   ` (2 preceding siblings ...)
  2020-05-21  1:21 ` [PATCH v3 03/16] spi: dw: Discard static DW DMA slave structures Serge Semin
@ 2020-05-21  1:21 ` Serge Semin
  2020-05-21 10:23   ` Andy Shevchenko
  2020-05-21  1:21 ` [PATCH v3 07/16] spi: dw: Use DMA max burst to set the request thresholds Serge Semin
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 32+ messages in thread
From: Serge Semin @ 2020-05-21  1:21 UTC (permalink / raw)
  To: Mark Brown
  Cc: Serge Semin, Serge Semin, Georgy Vlasov, Ramil Zaripov,
	Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
	Arnd Bergmann, Andy Shevchenko, Rob Herring, linux-mips,
	devicetree, Thomas Gleixner, Wan Ahmad Zainie, Jarkko Nikula,
	linux-spi, linux-kernel

It isn't good to have numeric literals in the code especially if there
are multiple of them and they are related. Let's replace the Tx and Rx
burst level literals with the corresponding constants.

Co-developed-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
Signed-off-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
Co-developed-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
Signed-off-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-mips@vger.kernel.org
Cc: devicetree@vger.kernel.org

---

Changelog v3:
- Discard the dws->fifo_len utilization in the Tx FIFO DMA threshold
  setting.
---
 drivers/spi/spi-dw-mid.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-dw-mid.c b/drivers/spi/spi-dw-mid.c
index 7bba774885cd..be02fedd87cb 100644
--- a/drivers/spi/spi-dw-mid.c
+++ b/drivers/spi/spi-dw-mid.c
@@ -19,7 +19,9 @@
 
 #define WAIT_RETRIES	5
 #define RX_BUSY		0
+#define RX_BURST_LEVEL	16
 #define TX_BUSY		1
+#define TX_BURST_LEVEL	16
 
 static bool mid_spi_dma_chan_filter(struct dma_chan *chan, void *param)
 {
@@ -214,7 +216,7 @@ static struct dma_async_tx_descriptor *dw_spi_dma_prepare_tx(struct dw_spi *dws,
 	memset(&txconf, 0, sizeof(txconf));
 	txconf.direction = DMA_MEM_TO_DEV;
 	txconf.dst_addr = dws->dma_addr;
-	txconf.dst_maxburst = 16;
+	txconf.dst_maxburst = TX_BURST_LEVEL;
 	txconf.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
 	txconf.dst_addr_width = convert_dma_width(dws->n_bytes);
 	txconf.device_fc = false;
@@ -288,7 +290,7 @@ static struct dma_async_tx_descriptor *dw_spi_dma_prepare_rx(struct dw_spi *dws,
 	memset(&rxconf, 0, sizeof(rxconf));
 	rxconf.direction = DMA_DEV_TO_MEM;
 	rxconf.src_addr = dws->dma_addr;
-	rxconf.src_maxburst = 16;
+	rxconf.src_maxburst = RX_BURST_LEVEL;
 	rxconf.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
 	rxconf.src_addr_width = convert_dma_width(dws->n_bytes);
 	rxconf.device_fc = false;
@@ -313,8 +315,8 @@ static int mid_spi_dma_setup(struct dw_spi *dws, struct spi_transfer *xfer)
 {
 	u16 imr = 0, dma_ctrl = 0;
 
-	dw_writel(dws, DW_SPI_DMARDLR, 0xf);
-	dw_writel(dws, DW_SPI_DMATDLR, 0x10);
+	dw_writel(dws, DW_SPI_DMARDLR, RX_BURST_LEVEL - 1);
+	dw_writel(dws, DW_SPI_DMATDLR, TX_BURST_LEVEL);
 
 	if (xfer->tx_buf) {
 		dma_ctrl |= SPI_DMA_TDMAE;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v3 07/16] spi: dw: Use DMA max burst to set the request thresholds
  2020-05-21  1:21 [PATCH v3 00/16] spi: dw: Add generic DW DMA controller support Serge Semin
                   ` (3 preceding siblings ...)
  2020-05-21  1:21 ` [PATCH v3 06/16] spi: dw: Parameterize the DMA Rx/Tx burst length Serge Semin
@ 2020-05-21  1:21 ` Serge Semin
  2020-05-21 10:49   ` Andy Shevchenko
  2020-05-21  1:22 ` [PATCH v3 11/16] spi: dw: Remove DW DMA code dependency from DW_DMAC_PCI Serge Semin
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 32+ messages in thread
From: Serge Semin @ 2020-05-21  1:21 UTC (permalink / raw)
  To: Mark Brown
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Thomas Bogendoerfer,
	Paul Burton, Ralf Baechle, Arnd Bergmann, Andy Shevchenko,
	Rob Herring, linux-mips, devicetree, Georgy Vlasov,
	Ramil Zaripov, Thomas Gleixner, Wan Ahmad Zainie, Jarkko Nikula,
	Clement Leger, linux-spi, linux-kernel

Each channel of DMA controller may have a limited length of burst
transaction (number of IO operations performed at ones in a single
DMA client request). This parameter can be used to setup the most
optimal DMA Tx/Rx data level values. In order to avoid the Tx buffer
overrun we can set the DMA Tx level to be of FIFO depth minus the
maximum burst transactions length. To prevent the Rx buffer underflow
the DMA Rx level should be set to the maximum burst transactions length.
This commit setups the DMA channels and the DW SPI DMA Tx/Rx levels
in accordance with these rules.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-mips@vger.kernel.org
Cc: devicetree@vger.kernel.org

---

Changelog v3:
- Use min() method to calculate the optimal burst values.
---
 drivers/spi/spi-dw-mid.c | 37 +++++++++++++++++++++++++++++++++----
 drivers/spi/spi-dw.h     |  2 ++
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-dw-mid.c b/drivers/spi/spi-dw-mid.c
index be02fedd87cb..0e95d8bc85c5 100644
--- a/drivers/spi/spi-dw-mid.c
+++ b/drivers/spi/spi-dw-mid.c
@@ -34,6 +34,31 @@ static bool mid_spi_dma_chan_filter(struct dma_chan *chan, void *param)
 	return true;
 }
 
+static void mid_spi_maxburst_init(struct dw_spi *dws)
+{
+	struct dma_slave_caps caps;
+	u32 max_burst, def_burst;
+	int ret;
+
+	def_burst = dws->fifo_len / 2;
+
+	ret = dma_get_slave_caps(dws->rxchan, &caps);
+	if (!ret && caps.max_burst)
+		max_burst = caps.max_burst;
+	else
+		max_burst = RX_BURST_LEVEL;
+
+	dws->rxburst = min(max_burst, def_burst);
+
+	ret = dma_get_slave_caps(dws->txchan, &caps);
+	if (!ret && caps.max_burst)
+		max_burst = caps.max_burst;
+	else
+		max_burst = TX_BURST_LEVEL;
+
+	dws->txburst = min(max_burst, def_burst);
+}
+
 static int mid_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
 {
 	struct dw_dma_slave slave = {
@@ -69,6 +94,8 @@ static int mid_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
 	dws->master->dma_rx = dws->rxchan;
 	dws->master->dma_tx = dws->txchan;
 
+	mid_spi_maxburst_init(dws);
+
 	return 0;
 
 free_rxchan:
@@ -94,6 +121,8 @@ static int mid_spi_dma_init_generic(struct device *dev, struct dw_spi *dws)
 	dws->master->dma_rx = dws->rxchan;
 	dws->master->dma_tx = dws->txchan;
 
+	mid_spi_maxburst_init(dws);
+
 	return 0;
 }
 
@@ -216,7 +245,7 @@ static struct dma_async_tx_descriptor *dw_spi_dma_prepare_tx(struct dw_spi *dws,
 	memset(&txconf, 0, sizeof(txconf));
 	txconf.direction = DMA_MEM_TO_DEV;
 	txconf.dst_addr = dws->dma_addr;
-	txconf.dst_maxburst = TX_BURST_LEVEL;
+	txconf.dst_maxburst = dws->txburst;
 	txconf.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
 	txconf.dst_addr_width = convert_dma_width(dws->n_bytes);
 	txconf.device_fc = false;
@@ -290,7 +319,7 @@ static struct dma_async_tx_descriptor *dw_spi_dma_prepare_rx(struct dw_spi *dws,
 	memset(&rxconf, 0, sizeof(rxconf));
 	rxconf.direction = DMA_DEV_TO_MEM;
 	rxconf.src_addr = dws->dma_addr;
-	rxconf.src_maxburst = RX_BURST_LEVEL;
+	rxconf.src_maxburst = dws->rxburst;
 	rxconf.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
 	rxconf.src_addr_width = convert_dma_width(dws->n_bytes);
 	rxconf.device_fc = false;
@@ -315,8 +344,8 @@ static int mid_spi_dma_setup(struct dw_spi *dws, struct spi_transfer *xfer)
 {
 	u16 imr = 0, dma_ctrl = 0;
 
-	dw_writel(dws, DW_SPI_DMARDLR, RX_BURST_LEVEL - 1);
-	dw_writel(dws, DW_SPI_DMATDLR, TX_BURST_LEVEL);
+	dw_writel(dws, DW_SPI_DMARDLR, dws->rxburst - 1);
+	dw_writel(dws, DW_SPI_DMATDLR, dws->fifo_len - dws->txburst);
 
 	if (xfer->tx_buf) {
 		dma_ctrl |= SPI_DMA_TDMAE;
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index 4902f937c3d7..d0c8b7d3a5d2 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -141,7 +141,9 @@ struct dw_spi {
 
 	/* DMA info */
 	struct dma_chan		*txchan;
+	u32			txburst;
 	struct dma_chan		*rxchan;
+	u32			rxburst;
 	unsigned long		dma_chan_busy;
 	dma_addr_t		dma_addr; /* phy address of the Data register */
 	const struct dw_spi_dma_ops *dma_ops;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v3 11/16] spi: dw: Remove DW DMA code dependency from DW_DMAC_PCI
  2020-05-21  1:21 [PATCH v3 00/16] spi: dw: Add generic DW DMA controller support Serge Semin
                   ` (4 preceding siblings ...)
  2020-05-21  1:21 ` [PATCH v3 07/16] spi: dw: Use DMA max burst to set the request thresholds Serge Semin
@ 2020-05-21  1:22 ` Serge Semin
  2020-05-21  1:22 ` [PATCH v3 12/16] spi: dw: Add DW SPI DMA/PCI/MMIO dependency on the DW SPI core Serge Semin
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 32+ messages in thread
From: Serge Semin @ 2020-05-21  1:22 UTC (permalink / raw)
  To: Mark Brown
  Cc: Serge Semin, Serge Semin, Georgy Vlasov, Ramil Zaripov,
	Andy Shevchenko, Alexey Malahov, Thomas Bogendoerfer,
	Paul Burton, Ralf Baechle, Rob Herring, Arnd Bergmann,
	linux-mips, devicetree, John Garry, Chuanhong Guo,
	Krzysztof Kozlowski, Eddie James, Chris Packham, Masahisa Kojima,
	Tomer Maimon, linux-spi, linux-kernel

Since there is a generic method available to initialize the DW SPI DMA
interface on any DT and ACPI-based platforms, which in general can be
designed with not only DW DMAC but with any DMA engine on board, we can
freely remove the CONFIG_DW_DMAC_PCI config from dependency list of
CONFIG_SPI_DW_DMA. Especially seeing that we don't use anything DW DMAC
specific in the new driver.

Co-developed-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
Signed-off-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
Co-developed-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
Signed-off-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: linux-mips@vger.kernel.org
Cc: devicetree@vger.kernel.org
---
 drivers/spi/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 03b061975f70..6a84f3dad35c 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -228,7 +228,7 @@ config SPI_DESIGNWARE
 
 config SPI_DW_DMA
 	bool "DMA support for DW SPI controller"
-	depends on SPI_DESIGNWARE && DW_DMAC_PCI
+	depends on SPI_DESIGNWARE
 
 config SPI_DW_PCI
 	tristate "PCI interface driver for DW SPI core"
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v3 12/16] spi: dw: Add DW SPI DMA/PCI/MMIO dependency on the DW SPI core
  2020-05-21  1:21 [PATCH v3 00/16] spi: dw: Add generic DW DMA controller support Serge Semin
                   ` (5 preceding siblings ...)
  2020-05-21  1:22 ` [PATCH v3 11/16] spi: dw: Remove DW DMA code dependency from DW_DMAC_PCI Serge Semin
@ 2020-05-21  1:22 ` Serge Semin
  2020-05-21  1:22 ` [PATCH v3 14/16] spi: dw: Add DMA support to the DW SPI MMIO driver Serge Semin
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 32+ messages in thread
From: Serge Semin @ 2020-05-21  1:22 UTC (permalink / raw)
  To: Mark Brown
  Cc: Serge Semin, Serge Semin, Georgy Vlasov, Ramil Zaripov,
	Andy Shevchenko, Alexey Malahov, Thomas Bogendoerfer,
	Paul Burton, Ralf Baechle, Arnd Bergmann, Rob Herring,
	linux-mips, devicetree, John Garry, Chuanhong Guo, Eddie James,
	Chris Packham, Masahisa Kojima, Tomer Maimon,
	Krzysztof Kozlowski, linux-spi, linux-kernel

Seeing all of the DW SPI driver components like DW SPI DMA/PCI/MMIO
depend on the DW SPI core code it's better to use the if-endif
conditional kernel config statement to signify that common dependency.

Co-developed-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
Signed-off-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
Co-developed-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
Signed-off-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-mips@vger.kernel.org
Cc: devicetree@vger.kernel.org
---
 drivers/spi/Kconfig | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 6a84f3dad35c..3cdf8310d185 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -226,17 +226,20 @@ config SPI_DESIGNWARE
 	help
 	  general driver for SPI controller core from DesignWare
 
+if SPI_DESIGNWARE
+
 config SPI_DW_DMA
 	bool "DMA support for DW SPI controller"
-	depends on SPI_DESIGNWARE
 
 config SPI_DW_PCI
 	tristate "PCI interface driver for DW SPI core"
-	depends on SPI_DESIGNWARE && PCI
+	depends on PCI
 
 config SPI_DW_MMIO
 	tristate "Memory-mapped io interface driver for DW SPI core"
-	depends on SPI_DESIGNWARE
+	depends on HAS_IOMEM
+
+endif
 
 config SPI_DLN2
        tristate "Diolan DLN-2 USB SPI adapter"
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v3 14/16] spi: dw: Add DMA support to the DW SPI MMIO driver
  2020-05-21  1:21 [PATCH v3 00/16] spi: dw: Add generic DW DMA controller support Serge Semin
                   ` (6 preceding siblings ...)
  2020-05-21  1:22 ` [PATCH v3 12/16] spi: dw: Add DW SPI DMA/PCI/MMIO dependency on the DW SPI core Serge Semin
@ 2020-05-21  1:22 ` Serge Semin
  2020-05-21  1:22 ` [PATCH v3 16/16] dt-bindings: spi: Convert DW SPI binding to DT schema Serge Semin
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 32+ messages in thread
From: Serge Semin @ 2020-05-21  1:22 UTC (permalink / raw)
  To: Mark Brown
  Cc: Serge Semin, Serge Semin, Georgy Vlasov, Ramil Zaripov,
	Andy Shevchenko, Alexey Malahov, Thomas Bogendoerfer,
	Paul Burton, Ralf Baechle, Arnd Bergmann, Rob Herring,
	linux-mips, devicetree, Wan Ahmad Zainie, Gareth Williams,
	Stephen Boyd, YueHaibing, Jarkko Nikula, Thomas Gleixner,
	linux-spi, linux-kernel

Since the common code in the spi-dw-dma.c driver is ready to be used
by the MMIO driver and now provides a method to generically (on any
DT or ACPI-based platforms) retrieve the Tx/Rx DMA channel handlers,
we can use it and a set of the common DW SPI DMA callbacks to enable
DMA at least for generic "snps,dw-apb-ssi" and "snps,dwc-ssi-1.01a"
devices.

Co-developed-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
Signed-off-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
Co-developed-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
Signed-off-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-mips@vger.kernel.org
Cc: devicetree@vger.kernel.org
---
 drivers/spi/spi-dw-mmio.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index 0894b4c09496..e23d0c53a664 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -149,6 +149,8 @@ static int dw_spi_dw_apb_init(struct platform_device *pdev,
 	/* Register hook to configure CTRLR0 */
 	dwsmmio->dws.update_cr0 = dw_spi_update_cr0;
 
+	dw_spi_dma_setup_generic(&dwsmmio->dws);
+
 	return 0;
 }
 
@@ -158,6 +160,8 @@ static int dw_spi_dwc_ssi_init(struct platform_device *pdev,
 	/* Register hook to configure CTRLR0 */
 	dwsmmio->dws.update_cr0 = dw_spi_update_cr0_v1_01a;
 
+	dw_spi_dma_setup_generic(&dwsmmio->dws);
+
 	return 0;
 }
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v3 16/16] dt-bindings: spi: Convert DW SPI binding to DT schema
  2020-05-21  1:21 [PATCH v3 00/16] spi: dw: Add generic DW DMA controller support Serge Semin
                   ` (7 preceding siblings ...)
  2020-05-21  1:22 ` [PATCH v3 14/16] spi: dw: Add DMA support to the DW SPI MMIO driver Serge Semin
@ 2020-05-21  1:22 ` Serge Semin
       [not found] ` <20200521012206.14472-10-Sergey.Semin@baikalelectronics.ru>
  2020-05-21 17:46 ` [PATCH v3 00/16] spi: dw: Add generic DW DMA controller support Andy Shevchenko
  10 siblings, 0 replies; 32+ messages in thread
From: Serge Semin @ 2020-05-21  1:22 UTC (permalink / raw)
  To: Mark Brown, Rob Herring
  Cc: Serge Semin, Serge Semin, Georgy Vlasov, Ramil Zaripov,
	Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
	Arnd Bergmann, Andy Shevchenko, linux-mips, Wan Ahmad Zainie,
	Gareth Williams, linux-spi, devicetree, linux-kernel

Modern device tree bindings are supposed to be created as YAML-files
in accordance with dt-schema. This commit replaces two DW SPI legacy
bare text bindings with YAML file. As before the bindings file states
that the corresponding dts node is supposed to be compatible either
with generic DW APB SSI controller or with Microsemi/Amazon/Renesas/Intel
vendors-specific controllers, to have registers, interrupts and clocks
properties. Though in case of Microsemi version of the controller
there must be two registers resources specified. Properties like
clock-names, reg-io-width, cs-gpio, num-cs, DMA and slave device
sub-nodes are optional.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: linux-mips@vger.kernel.org
---
 .../bindings/spi/snps,dw-apb-ssi.txt          |  44 ------
 .../bindings/spi/snps,dw-apb-ssi.yaml         | 127 ++++++++++++++++++
 .../devicetree/bindings/spi/spi-dw.txt        |  24 ----
 3 files changed, 127 insertions(+), 68 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt
 create mode 100644 Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
 delete mode 100644 Documentation/devicetree/bindings/spi/spi-dw.txt

diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt
deleted file mode 100644
index 020e3168ee41..000000000000
--- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt
+++ /dev/null
@@ -1,44 +0,0 @@
-Synopsys DesignWare AMBA 2.0 Synchronous Serial Interface.
-
-Required properties:
-- compatible : "snps,dw-apb-ssi" or "mscc,<soc>-spi", where soc is "ocelot" or
-  "jaguar2", or "amazon,alpine-dw-apb-ssi", or "snps,dwc-ssi-1.01a" or
-  "intel,keembay-ssi"
-- reg : The register base for the controller. For "mscc,<soc>-spi", a second
-  register set is required (named ICPU_CFG:SPI_MST)
-- interrupts : One interrupt, used by the controller.
-- #address-cells : <1>, as required by generic SPI binding.
-- #size-cells : <0>, also as required by generic SPI binding.
-- clocks : phandles for the clocks, see the description of clock-names below.
-   The phandle for the "ssi_clk" is required. The phandle for the "pclk" clock
-   is optional. If a single clock is specified but no clock-name, it is the
-   "ssi_clk" clock. If both clocks are listed, the "ssi_clk" must be first.
-
-Optional properties:
-- clock-names : Contains the names of the clocks:
-    "ssi_clk", for the core clock used to generate the external SPI clock.
-    "pclk", the interface clock, required for register access. If a clock domain
-     used to enable this clock then it should be named "pclk_clkdomain".
-- cs-gpios : Specifies the gpio pins to be used for chipselects.
-- num-cs : The number of chipselects. If omitted, this will default to 4.
-- reg-io-width : The I/O register width (in bytes) implemented by this
-  device.  Supported values are 2 or 4 (the default).
-- dmas : Phandle + identifiers of Tx and Rx DMA channels.
-- dma-names : Contains the names of the DMA channels. Must be "tx" and "rx".
-
-Child nodes as per the generic SPI binding.
-
-Example:
-
-	spi@fff00000 {
-		compatible = "snps,dw-apb-ssi";
-		reg = <0xfff00000 0x1000>;
-		interrupts = <0 154 4>;
-		#address-cells = <1>;
-		#size-cells = <0>;
-		clocks = <&spi_m_clk>;
-		num-cs = <2>;
-		cs-gpios = <&gpio0 13 0>,
-			   <&gpio0 14 0>;
-	};
-
diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
new file mode 100644
index 000000000000..1fcab6415136
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
@@ -0,0 +1,127 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/snps,dw-apb-ssi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Synopsys DesignWare AMBA 2.0 Synchronous Serial Interface
+
+maintainers:
+  - Mark Brown <broonie@kernel.org>
+
+allOf:
+  - $ref: "spi-controller.yaml#"
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - mscc,ocelot-spi
+              - mscc,jaguar2-spi
+    then:
+      properties:
+        reg:
+          minItems: 2
+
+properties:
+  compatible:
+    oneOf:
+      - description: Generic DW SPI Controller
+        enum:
+          - snps,dw-apb-ssi
+          - snps,dwc-ssi-1.01a
+      - description: Microsemi Ocelot/Jaguar2 SoC SPI Controller
+        items:
+          - enum:
+              - mscc,ocelot-spi
+              - mscc,jaguar2-spi
+          - const: snps,dw-apb-ssi
+      - description: Amazon Alpine SPI Controller
+        const: amazon,alpine-dw-apb-ssi
+      - description: Renesas RZ/N1 SPI Controller
+        items:
+          - const: renesas,rzn1-spi
+          - const: snps,dw-apb-ssi
+      - description: Intel Keem Bay SPI Controller
+        const: intel,keembay-ssi
+
+  reg:
+    minItems: 1
+    items:
+      - description: DW APB SSI controller memory mapped registers
+      - description: SPI MST region map
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    minItems: 1
+    items:
+      - description: SPI Controller reference clock source
+      - description: APB interface clock source
+
+  clock-names:
+    minItems: 1
+    items:
+      - const: ssi_clk
+      - const: pclk
+
+  reg-io-width:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: I/O register width (in bytes) implemented by this device
+    default: 4
+    enum: [ 2, 4 ]
+
+  num-cs:
+    default: 4
+    minimum: 1
+    maximum: 4
+
+  dmas:
+    items:
+      - description: TX DMA Channel
+      - description: RX DMA Channel
+
+  dma-names:
+    items:
+      - const: tx
+      - const: rx
+
+patternProperties:
+  "^.*@[0-9a-f]+$":
+    type: object
+    properties:
+      reg:
+        minimum: 0
+        maximum: 3
+
+      spi-rx-bus-width:
+        const: 1
+
+      spi-tx-bus-width:
+        const: 1
+
+unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+  - interrupts
+  - clocks
+
+examples:
+  - |
+    spi@fff00000 {
+      compatible = "snps,dw-apb-ssi";
+      reg = <0xfff00000 0x1000>;
+      #address-cells = <1>;
+      #size-cells = <0>;
+      interrupts = <0 154 4>;
+      clocks = <&spi_m_clk>;
+      num-cs = <2>;
+      cs-gpios = <&gpio0 13 0>,
+                 <&gpio0 14 0>;
+    };
+...
diff --git a/Documentation/devicetree/bindings/spi/spi-dw.txt b/Documentation/devicetree/bindings/spi/spi-dw.txt
deleted file mode 100644
index 7b63ed601990..000000000000
--- a/Documentation/devicetree/bindings/spi/spi-dw.txt
+++ /dev/null
@@ -1,24 +0,0 @@
-Synopsys DesignWare SPI master
-
-Required properties:
-- compatible: should be "snps,designware-spi"
-- #address-cells: see spi-bus.txt
-- #size-cells: see spi-bus.txt
-- reg: address and length of the spi master registers
-- interrupts: should contain one interrupt
-- clocks: spi clock phandle
-- num-cs: see spi-bus.txt
-
-Optional properties:
-- cs-gpios: see spi-bus.txt
-
-Example:
-
-spi: spi@4020a000 {
-	compatible = "snps,designware-spi";
-	interrupts = <11 1>;
-	reg = <0x4020a000 0x1000>;
-	clocks = <&pclk>;
-	num-cs = <2>;
-	cs-gpios = <&banka 0 0>;
-};
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
  2020-05-21  1:21 ` [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA Serge Semin
@ 2020-05-21  3:09   ` Feng Tang
  2020-05-21 11:47     ` Serge Semin
  2020-05-21 23:33   ` Serge Semin
  1 sibling, 1 reply; 32+ messages in thread
From: Feng Tang @ 2020-05-21  3:09 UTC (permalink / raw)
  To: Serge Semin
  Cc: Mark Brown, Grant Likely, Vinod Koul, Alan Cox, Linus Walleij,
	Serge Semin, Georgy Vlasov, Ramil Zaripov, Alexey Malahov,
	Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Arnd Bergmann,
	Andy Shevchenko, Rob Herring, linux-mips, devicetree,
	Jarkko Nikula, Thomas Gleixner, Wan Ahmad Zainie, Linus Walleij,
	Clement Leger, linux-spi, linux-kernel

Hi Serge,

On Thu, May 21, 2020 at 04:21:51AM +0300, Serge Semin wrote:
> Since DMA transfers are performed asynchronously with actual SPI
> transaction, then even if DMA transfers are finished it doesn't mean
> all data is actually pushed to the SPI bus. Some data might still be
> in the controller FIFO. This is specifically true for Tx-only
> transfers. In this case if the next SPI transfer is recharged while
> a tail of the previous one is still in FIFO, we'll loose that tail
> data. In order to fix this lets add the wait procedure of the Tx/Rx
> SPI transfers completion after the corresponding DMA transactions
> are 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>
> Fixes: 7063c0d942a1 ("spi/dw_spi: add DMA support")
> Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-mips@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> 
> ---
> 
> Changelog v2:
> - Use conditional statement instead of the ternary operator in the ref
>   clock getter.
> - Move the patch to the head of the series so one could be picked up to
>   the stable kernels as a fix.
> 
> Changelog v3:
> - Use spi_delay_exec() method to wait for the current operation completion.
> ---
>  drivers/spi/spi-dw-mid.c | 69 ++++++++++++++++++++++++++++++++++++++++
>  drivers/spi/spi-dw.h     | 10 ++++++
>  2 files changed, 79 insertions(+)
> 
> diff --git a/drivers/spi/spi-dw-mid.c b/drivers/spi/spi-dw-mid.c
> index f9757a370699..3526b196a7fc 100644
> --- a/drivers/spi/spi-dw-mid.c
> +++ b/drivers/spi/spi-dw-mid.c
> @@ -17,6 +17,7 @@
>  #include <linux/pci.h>
>  #include <linux/platform_data/dma-dw.h>
>  
> +#define WAIT_RETRIES	5
>  #define RX_BUSY		0
>  #define TX_BUSY		1
>  
> @@ -143,6 +144,47 @@ static enum dma_slave_buswidth convert_dma_width(u32 dma_width) {
>  	return DMA_SLAVE_BUSWIDTH_UNDEFINED;
>  }
>  
> +static void dw_spi_dma_calc_delay(struct dw_spi *dws, u32 nents,
> +				  struct spi_delay *delay)
> +{
> +	unsigned long ns, us;
> +
> +	ns = (NSEC_PER_SEC / spi_get_clk(dws)) * nents * dws->n_bytes *
> +	     BITS_PER_BYTE;
> +
> +	if (ns <= NSEC_PER_USEC) {
> +		delay->unit = SPI_DELAY_UNIT_NSECS;
> +		delay->value = ns;
> +	} else {
> +		us = DIV_ROUND_UP(ns, NSEC_PER_USEC);
> +		delay->unit = SPI_DELAY_UNIT_USECS;
> +		delay->value = clamp_val(us, 0, USHRT_MAX);
> +	}
> +}
> +
> +static inline bool dw_spi_dma_tx_busy(struct dw_spi *dws)
> +{
> +	return !(dw_readl(dws, DW_SPI_SR) & SR_TF_EMPT);
> +}
> +
> +static void dw_spi_dma_wait_tx_done(struct dw_spi *dws)
> +{
> +	int retry = WAIT_RETRIES;
> +	struct spi_delay delay;
> +	u32 nents;
> +
> +	nents = dw_readl(dws, DW_SPI_TXFLR);
> +	dw_spi_dma_calc_delay(dws, nents, &delay);
> +
> +	while (dw_spi_dma_tx_busy(dws) && retry--)
> +		spi_delay_exec(&delay, NULL);
> +
> +	if (retry < 0) {
> +		dev_err(&dws->master->dev, "Tx hanged up\n");
> +		dws->master->cur_msg->status = -EIO;
> +	}
> +}
> +
>  /*
>   * dws->dma_chan_busy is set before the dma transfer starts, callback for tx
>   * channel will clear a corresponding bit.
> @@ -151,6 +193,8 @@ static void dw_spi_dma_tx_done(void *arg)
>  {
>  	struct dw_spi *dws = arg;
>  
> +	dw_spi_dma_wait_tx_done(dws);
> +
>  	clear_bit(TX_BUSY, &dws->dma_chan_busy);
>  	if (test_bit(RX_BUSY, &dws->dma_chan_busy))
>  		return;
> @@ -192,6 +236,29 @@ static struct dma_async_tx_descriptor *dw_spi_dma_prepare_tx(struct dw_spi *dws,
>  	return txdesc;
>  }
>  
> +static inline bool dw_spi_dma_rx_busy(struct dw_spi *dws)
> +{
> +	return !!(dw_readl(dws, DW_SPI_SR) & SR_RF_NOT_EMPT);
> +}
> +
> +static void dw_spi_dma_wait_rx_done(struct dw_spi *dws)
> +{
> +	int retry = WAIT_RETRIES;
> +	struct spi_delay delay;
> +	u32 nents;
> +
> +	nents = dw_readl(dws, DW_SPI_RXFLR);
> +	dw_spi_dma_calc_delay(dws, nents, &delay);
> +
> +	while (dw_spi_dma_rx_busy(dws) && retry--)
> +		spi_delay_exec(&delay, NULL);
> +
> +	if (retry < 0) {
> +		dev_err(&dws->master->dev, "Rx hanged up\n");
> +		dws->master->cur_msg->status = -EIO;
> +	}
> +}
> +
>  /*
>   * dws->dma_chan_busy is set before the dma transfer starts, callback for rx
>   * channel will clear a corresponding bit.
> @@ -200,6 +267,8 @@ static void dw_spi_dma_rx_done(void *arg)
>  {
>  	struct dw_spi *dws = arg;
>  
> +	dw_spi_dma_wait_rx_done(dws);

I can understand the problem about TX, but I don't see how RX
will get hurt, can you elaborate more? thanks

- Feng


> +
>  	clear_bit(RX_BUSY, &dws->dma_chan_busy);
>  	if (test_bit(TX_BUSY, &dws->dma_chan_busy))
>  		return;
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index e92d43b9a9e6..81364f501b7e 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -210,6 +210,16 @@ static inline void spi_set_clk(struct dw_spi *dws, u16 div)
>  	dw_writel(dws, DW_SPI_BAUDR, div);
>  }
>  
> +static inline u32 spi_get_clk(struct dw_spi *dws)
> +{
> +	u32 div = dw_readl(dws, DW_SPI_BAUDR);
> +
> +	if (!div)
> +		return 0;
> +
> +	return dws->max_freq / div;
> +}
> +
>  /* Disable IRQ bits */
>  static inline void spi_mask_intr(struct dw_spi *dws, u32 mask)
>  {
> -- 
> 2.25.1

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 03/16] spi: dw: Discard static DW DMA slave structures
  2020-05-21  1:21 ` [PATCH v3 03/16] spi: dw: Discard static DW DMA slave structures Serge Semin
@ 2020-05-21  9:57   ` Andy Shevchenko
       [not found]     ` <20200521121228.aqplh6eftylnys3p@mobilestation>
  0 siblings, 1 reply; 32+ messages in thread
From: Andy Shevchenko @ 2020-05-21  9:57 UTC (permalink / raw)
  To: Serge Semin
  Cc: Mark Brown, Serge Semin, Georgy Vlasov, Ramil Zaripov,
	Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
	Andy Shevchenko, Arnd Bergmann, Rob Herring, linux-mips,
	devicetree, Thomas Gleixner, Wan Ahmad Zainie, Jarkko Nikula,
	Clement Leger, linux-spi, Linux Kernel Mailing List

On Thu, May 21, 2020 at 4:23 AM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
>
> Having them declared is redundant since each struct dw_dma_chan has
> the same structure embedded and the structure from the passed dma_chan
> private pointer will be copied there as a result of the next calls
> chain:
> dma_request_channel() -> find_candidate() -> dma_chan_get() ->
> device_alloc_chan_resources() = dwc_alloc_chan_resources() ->
> dw_dma_filter().
> So just remove the static dw_dma_chan structures and use a locally
> declared data instance with dst_id/src_id set to the same values as
> the static copies used to have.

...

> - Explicitly initialize the dw_dma_slave members on stack.

Thanks for an update, but that's not what I asked for...

> -static struct dw_dma_slave mid_dma_tx = { .dst_id = 1 };
> -static struct dw_dma_slave mid_dma_rx = { .src_id = 0 };

>  static int mid_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
>  {
> +       struct dw_dma_slave slave = {
> +               .src_id = 0,
> +               .dst_id = 0
> +       };

(It's member, and not memberS)

> -       struct dw_dma_slave *tx = dws->dma_tx;
> -       struct dw_dma_slave *rx = dws->dma_rx;

May we simple do

struct dw_dma_slave tx = { .dst_id = 1 };
struct dw_dma_slave rx = { .src_id = 0 };

please?

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 06/16] spi: dw: Parameterize the DMA Rx/Tx burst length
  2020-05-21  1:21 ` [PATCH v3 06/16] spi: dw: Parameterize the DMA Rx/Tx burst length Serge Semin
@ 2020-05-21 10:23   ` Andy Shevchenko
  0 siblings, 0 replies; 32+ messages in thread
From: Andy Shevchenko @ 2020-05-21 10:23 UTC (permalink / raw)
  To: Serge Semin
  Cc: Mark Brown, Serge Semin, Georgy Vlasov, Ramil Zaripov,
	Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
	Arnd Bergmann, Rob Herring, linux-mips, devicetree,
	Thomas Gleixner, Wan Ahmad Zainie, Jarkko Nikula, linux-spi,
	linux-kernel

On Thu, May 21, 2020 at 04:21:56AM +0300, Serge Semin wrote:
> It isn't good to have numeric literals in the code especially if there
> are multiple of them and they are related. Let's replace the Tx and Rx
> burst level literals with the corresponding constants.

Thanks!
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> Co-developed-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
> Signed-off-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
> Co-developed-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
> Signed-off-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-mips@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> 
> ---
> 
> Changelog v3:
> - Discard the dws->fifo_len utilization in the Tx FIFO DMA threshold
>   setting.
> ---
>  drivers/spi/spi-dw-mid.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-dw-mid.c b/drivers/spi/spi-dw-mid.c
> index 7bba774885cd..be02fedd87cb 100644
> --- a/drivers/spi/spi-dw-mid.c
> +++ b/drivers/spi/spi-dw-mid.c
> @@ -19,7 +19,9 @@
>  
>  #define WAIT_RETRIES	5
>  #define RX_BUSY		0
> +#define RX_BURST_LEVEL	16
>  #define TX_BUSY		1
> +#define TX_BURST_LEVEL	16
>  
>  static bool mid_spi_dma_chan_filter(struct dma_chan *chan, void *param)
>  {
> @@ -214,7 +216,7 @@ static struct dma_async_tx_descriptor *dw_spi_dma_prepare_tx(struct dw_spi *dws,
>  	memset(&txconf, 0, sizeof(txconf));
>  	txconf.direction = DMA_MEM_TO_DEV;
>  	txconf.dst_addr = dws->dma_addr;
> -	txconf.dst_maxburst = 16;
> +	txconf.dst_maxburst = TX_BURST_LEVEL;
>  	txconf.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
>  	txconf.dst_addr_width = convert_dma_width(dws->n_bytes);
>  	txconf.device_fc = false;
> @@ -288,7 +290,7 @@ static struct dma_async_tx_descriptor *dw_spi_dma_prepare_rx(struct dw_spi *dws,
>  	memset(&rxconf, 0, sizeof(rxconf));
>  	rxconf.direction = DMA_DEV_TO_MEM;
>  	rxconf.src_addr = dws->dma_addr;
> -	rxconf.src_maxburst = 16;
> +	rxconf.src_maxburst = RX_BURST_LEVEL;
>  	rxconf.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
>  	rxconf.src_addr_width = convert_dma_width(dws->n_bytes);
>  	rxconf.device_fc = false;
> @@ -313,8 +315,8 @@ static int mid_spi_dma_setup(struct dw_spi *dws, struct spi_transfer *xfer)
>  {
>  	u16 imr = 0, dma_ctrl = 0;
>  
> -	dw_writel(dws, DW_SPI_DMARDLR, 0xf);
> -	dw_writel(dws, DW_SPI_DMATDLR, 0x10);
> +	dw_writel(dws, DW_SPI_DMARDLR, RX_BURST_LEVEL - 1);
> +	dw_writel(dws, DW_SPI_DMATDLR, TX_BURST_LEVEL);
>  
>  	if (xfer->tx_buf) {
>  		dma_ctrl |= SPI_DMA_TDMAE;
> -- 
> 2.25.1
> 

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 09/16] spi: dw: Add core suffix to the DW APB SSI core source file
       [not found] ` <20200521012206.14472-10-Sergey.Semin@baikalelectronics.ru>
@ 2020-05-21 10:47   ` Andy Shevchenko
  0 siblings, 0 replies; 32+ messages in thread
From: Andy Shevchenko @ 2020-05-21 10:47 UTC (permalink / raw)
  To: Serge Semin
  Cc: Mark Brown, Serge Semin, Georgy Vlasov, Ramil Zaripov,
	Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
	Rob Herring, Arnd Bergmann, linux-mips, devicetree,
	Chuanhong Guo, John Garry, Eddie James, Tomer Maimon,
	Masahisa Kojima, Chris Packham, Wan Ahmad Zainie, Charles Keepax,
	Clement Leger, Linus Walleij, Thomas Gleixner, wuxu.wu,
	Phil Edworthy, linux-spi, linux-kernel

On Thu, May 21, 2020 at 04:21:59AM +0300, Serge Semin wrote:
> Generic DMA support is going to be part of the DW APB SSI core object.
> In order to preserve the kernel loadable module name as spi-dw.ko, let's
> add the "-core" suffix to the object with generic DW APB SSI code and
> build it into the target spi-dw.ko driver.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
> Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: linux-mips@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> 
> ---
> 
> Changelog v3:
> - This is a new patch added as a result of the discussion with Andy
>   Shevchenko.
> ---
>  drivers/spi/Makefile                    | 1 +
>  drivers/spi/{spi-dw.c => spi-dw-core.c} | 0
>  2 files changed, 1 insertion(+)
>  rename drivers/spi/{spi-dw.c => spi-dw-core.c} (100%)
> 
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 28f601327f8c..70ebc2a62e5f 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -36,6 +36,7 @@ obj-$(CONFIG_SPI_COLDFIRE_QSPI)		+= spi-coldfire-qspi.o
>  obj-$(CONFIG_SPI_DAVINCI)		+= spi-davinci.o
>  obj-$(CONFIG_SPI_DLN2)			+= spi-dln2.o
>  obj-$(CONFIG_SPI_DESIGNWARE)		+= spi-dw.o
> +spi-dw-y				:= spi-dw-core.o
>  obj-$(CONFIG_SPI_DW_MMIO)		+= spi-dw-mmio.o
>  obj-$(CONFIG_SPI_DW_PCI)		+= spi-dw-midpci.o
>  spi-dw-midpci-objs			:= spi-dw-pci.o spi-dw-mid.o
> diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw-core.c
> similarity index 100%
> rename from drivers/spi/spi-dw.c
> rename to drivers/spi/spi-dw-core.c
> -- 
> 2.25.1
> 

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 07/16] spi: dw: Use DMA max burst to set the request thresholds
  2020-05-21  1:21 ` [PATCH v3 07/16] spi: dw: Use DMA max burst to set the request thresholds Serge Semin
@ 2020-05-21 10:49   ` Andy Shevchenko
  0 siblings, 0 replies; 32+ messages in thread
From: Andy Shevchenko @ 2020-05-21 10:49 UTC (permalink / raw)
  To: Serge Semin
  Cc: Mark Brown, Serge Semin, Alexey Malahov, Thomas Bogendoerfer,
	Paul Burton, Ralf Baechle, Arnd Bergmann, Rob Herring,
	linux-mips, devicetree, Georgy Vlasov, Ramil Zaripov,
	Thomas Gleixner, Wan Ahmad Zainie, Jarkko Nikula, Clement Leger,
	linux-spi, linux-kernel

On Thu, May 21, 2020 at 04:21:57AM +0300, Serge Semin wrote:
> Each channel of DMA controller may have a limited length of burst
> transaction (number of IO operations performed at ones in a single
> DMA client request). This parameter can be used to setup the most
> optimal DMA Tx/Rx data level values. In order to avoid the Tx buffer
> overrun we can set the DMA Tx level to be of FIFO depth minus the
> maximum burst transactions length. To prevent the Rx buffer underflow
> the DMA Rx level should be set to the maximum burst transactions length.
> This commit setups the DMA channels and the DW SPI DMA Tx/Rx levels
> in accordance with these rules.

Besides one bikeshedding point, looks good to me.
Feel free to add

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-mips@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> 
> ---
> 
> Changelog v3:
> - Use min() method to calculate the optimal burst values.
> ---
>  drivers/spi/spi-dw-mid.c | 37 +++++++++++++++++++++++++++++++++----
>  drivers/spi/spi-dw.h     |  2 ++
>  2 files changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-dw-mid.c b/drivers/spi/spi-dw-mid.c
> index be02fedd87cb..0e95d8bc85c5 100644
> --- a/drivers/spi/spi-dw-mid.c
> +++ b/drivers/spi/spi-dw-mid.c
> @@ -34,6 +34,31 @@ static bool mid_spi_dma_chan_filter(struct dma_chan *chan, void *param)
>  	return true;
>  }
>  
> +static void mid_spi_maxburst_init(struct dw_spi *dws)
> +{
> +	struct dma_slave_caps caps;
> +	u32 max_burst, def_burst;
> +	int ret;
> +
> +	def_burst = dws->fifo_len / 2;
> +
> +	ret = dma_get_slave_caps(dws->rxchan, &caps);
> +	if (!ret && caps.max_burst)
> +		max_burst = caps.max_burst;
> +	else
> +		max_burst = RX_BURST_LEVEL;
> +
> +	dws->rxburst = min(max_burst, def_burst);
> +
> +	ret = dma_get_slave_caps(dws->txchan, &caps);
> +	if (!ret && caps.max_burst)
> +		max_burst = caps.max_burst;
> +	else
> +		max_burst = TX_BURST_LEVEL;
> +
> +	dws->txburst = min(max_burst, def_burst);
> +}
> +
>  static int mid_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
>  {
>  	struct dw_dma_slave slave = {
> @@ -69,6 +94,8 @@ static int mid_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
>  	dws->master->dma_rx = dws->rxchan;
>  	dws->master->dma_tx = dws->txchan;
>  
> +	mid_spi_maxburst_init(dws);
> +
>  	return 0;
>  
>  free_rxchan:
> @@ -94,6 +121,8 @@ static int mid_spi_dma_init_generic(struct device *dev, struct dw_spi *dws)
>  	dws->master->dma_rx = dws->rxchan;
>  	dws->master->dma_tx = dws->txchan;
>  
> +	mid_spi_maxburst_init(dws);
> +
>  	return 0;
>  }
>  
> @@ -216,7 +245,7 @@ static struct dma_async_tx_descriptor *dw_spi_dma_prepare_tx(struct dw_spi *dws,
>  	memset(&txconf, 0, sizeof(txconf));
>  	txconf.direction = DMA_MEM_TO_DEV;
>  	txconf.dst_addr = dws->dma_addr;
> -	txconf.dst_maxburst = TX_BURST_LEVEL;
> +	txconf.dst_maxburst = dws->txburst;
>  	txconf.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
>  	txconf.dst_addr_width = convert_dma_width(dws->n_bytes);
>  	txconf.device_fc = false;
> @@ -290,7 +319,7 @@ static struct dma_async_tx_descriptor *dw_spi_dma_prepare_rx(struct dw_spi *dws,
>  	memset(&rxconf, 0, sizeof(rxconf));
>  	rxconf.direction = DMA_DEV_TO_MEM;
>  	rxconf.src_addr = dws->dma_addr;
> -	rxconf.src_maxburst = RX_BURST_LEVEL;
> +	rxconf.src_maxburst = dws->rxburst;
>  	rxconf.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
>  	rxconf.src_addr_width = convert_dma_width(dws->n_bytes);
>  	rxconf.device_fc = false;
> @@ -315,8 +344,8 @@ static int mid_spi_dma_setup(struct dw_spi *dws, struct spi_transfer *xfer)
>  {
>  	u16 imr = 0, dma_ctrl = 0;
>  
> -	dw_writel(dws, DW_SPI_DMARDLR, RX_BURST_LEVEL - 1);
> -	dw_writel(dws, DW_SPI_DMATDLR, TX_BURST_LEVEL);
> +	dw_writel(dws, DW_SPI_DMARDLR, dws->rxburst - 1);
> +	dw_writel(dws, DW_SPI_DMATDLR, dws->fifo_len - dws->txburst);
>  
>  	if (xfer->tx_buf) {
>  		dma_ctrl |= SPI_DMA_TDMAE;
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index 4902f937c3d7..d0c8b7d3a5d2 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -141,7 +141,9 @@ struct dw_spi {
>  
>  	/* DMA info */
>  	struct dma_chan		*txchan;
> +	u32			txburst;
>  	struct dma_chan		*rxchan;
> +	u32			rxburst;
>  	unsigned long		dma_chan_busy;
>  	dma_addr_t		dma_addr; /* phy address of the Data register */
>  	const struct dw_spi_dma_ops *dma_ops;
> -- 
> 2.25.1
> 

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
  2020-05-21  3:09   ` Feng Tang
@ 2020-05-21 11:47     ` Serge Semin
  2020-05-21 14:55       ` Feng Tang
  0 siblings, 1 reply; 32+ messages in thread
From: Serge Semin @ 2020-05-21 11:47 UTC (permalink / raw)
  To: Feng Tang
  Cc: Serge Semin, Mark Brown, Grant Likely, Vinod Koul, Alan Cox,
	Linus Walleij, Georgy Vlasov, Ramil Zaripov, Alexey Malahov,
	Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Arnd Bergmann,
	Andy Shevchenko, Rob Herring, linux-mips, devicetree,
	Jarkko Nikula, Thomas Gleixner, Wan Ahmad Zainie, Linus Walleij,
	Clement Leger, linux-spi, linux-kernel

Hello Feng,

On Thu, May 21, 2020 at 11:09:24AM +0800, Feng Tang wrote:
> Hi Serge,
> 
> On Thu, May 21, 2020 at 04:21:51AM +0300, Serge Semin wrote:

[nip]

> >  /*
> >   * dws->dma_chan_busy is set before the dma transfer starts, callback for rx
> >   * channel will clear a corresponding bit.
> > @@ -200,6 +267,8 @@ static void dw_spi_dma_rx_done(void *arg)
> >  {
> >  	struct dw_spi *dws = arg;
> >  
> > +	dw_spi_dma_wait_rx_done(dws);
> 
> I can understand the problem about TX, but I don't see how RX
> will get hurt, can you elaborate more? thanks
> 
> - Feng

Your question is correct. You are right with your hypothesis. Ideally upon the
dw_spi_dma_rx_done() execution Rx FIFO must be already empty. That's why the
commit log signifies the error being mostly related with Tx FIFO. But
practically there are many reasons why Rx FIFO might be left with data:
DMA engine failures, incorrect DMA configuration (if DW SPI or DW DMA driver
messed something up), controller hanging up, and so on. It's better to catch
an error at this stage while propagating it up to the SPI device drivers.
Especially seeing the wait-check implementation doesn't gives us much of the
execution overhead in normal conditions. So by calling dw_spi_dma_wait_rx_done()
we make sure that all the data has been fetched and we may freely get the
buffers back to the client driver.

-Sergey

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 03/16] spi: dw: Discard static DW DMA slave structures
       [not found]     ` <20200521121228.aqplh6eftylnys3p@mobilestation>
@ 2020-05-21 12:49       ` Andy Shevchenko
  2020-05-21 15:51       ` Mark Brown
  1 sibling, 0 replies; 32+ messages in thread
From: Andy Shevchenko @ 2020-05-21 12:49 UTC (permalink / raw)
  To: Serge Semin
  Cc: Serge Semin, Mark Brown, Georgy Vlasov, Ramil Zaripov,
	Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
	Andy Shevchenko, Arnd Bergmann, Rob Herring, linux-mips,
	devicetree, Thomas Gleixner, Wan Ahmad Zainie, Jarkko Nikula,
	Clement Leger, linux-spi, Linux Kernel Mailing List

On Thu, May 21, 2020 at 3:12 PM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
> On Thu, May 21, 2020 at 12:57:17PM +0300, Andy Shevchenko wrote:
> > On Thu, May 21, 2020 at 4:23 AM Serge Semin
> > <Sergey.Semin@baikalelectronics.ru> wrote:

...

> > Thanks for an update, but that's not what I asked for...
> >
> > > -static struct dw_dma_slave mid_dma_tx = { .dst_id = 1 };
> > > -static struct dw_dma_slave mid_dma_rx = { .src_id = 0 };
> >
> > >  static int mid_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
> > >  {
> > > +       struct dw_dma_slave slave = {
> > > +               .src_id = 0,
> > > +               .dst_id = 0
> > > +       };

> > > -       struct dw_dma_slave *tx = dws->dma_tx;
> > > -       struct dw_dma_slave *rx = dws->dma_rx;
> >
> > May we simple do
> >
> > struct dw_dma_slave tx = { .dst_id = 1 };
> > struct dw_dma_slave rx = { .src_id = 0 };
> >
> > please?
>
> Well, for me both solutions are equal

I don't think so.

> except mine consumes less stack memory.

And brought confusion and less readability. :-(

> The only reason why your solution might be better is that if DW DMA driver or
> the DMA engine subsystem changed the dw_dma_slave structure instance passed to
> the dma_request_channel() method, which non of them do. So I'll leave this for
> Mark to decide. Mark, could you give us your final word about this?

I explained already why I prefer to see them in that form. Reader can
easily understand what request line is used for what channel.
In your code it's hidden somewhere and on top of that that _one_
structure on the stack adds more confusion.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
  2020-05-21 11:47     ` Serge Semin
@ 2020-05-21 14:55       ` Feng Tang
  2020-05-21 15:33         ` Serge Semin
  0 siblings, 1 reply; 32+ messages in thread
From: Feng Tang @ 2020-05-21 14:55 UTC (permalink / raw)
  To: Serge Semin
  Cc: Serge Semin, Mark Brown, Grant Likely, Vinod Koul, Alan Cox,
	Linus Walleij, Georgy Vlasov, Ramil Zaripov, Alexey Malahov,
	Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Arnd Bergmann,
	Andy Shevchenko, Rob Herring, linux-mips, devicetree,
	Jarkko Nikula, Thomas Gleixner, Wan Ahmad Zainie, Linus Walleij,
	Clement Leger, linux-spi, linux-kernel

Hi Serge,

On Thu, May 21, 2020 at 02:47:36PM +0300, Serge Semin wrote:
> Hello Feng,
> 
> On Thu, May 21, 2020 at 11:09:24AM +0800, Feng Tang wrote:
> > Hi Serge,
> > 
> > On Thu, May 21, 2020 at 04:21:51AM +0300, Serge Semin wrote:
> 
> [nip]
> 
> > >  /*
> > >   * dws->dma_chan_busy is set before the dma transfer starts, callback for rx
> > >   * channel will clear a corresponding bit.
> > > @@ -200,6 +267,8 @@ static void dw_spi_dma_rx_done(void *arg)
> > >  {
> > >  	struct dw_spi *dws = arg;
> > >  
> > > +	dw_spi_dma_wait_rx_done(dws);
> > 
> > I can understand the problem about TX, but I don't see how RX
> > will get hurt, can you elaborate more? thanks
> > 
> > - Feng
> 
> Your question is correct. You are right with your hypothesis. Ideally upon the
> dw_spi_dma_rx_done() execution Rx FIFO must be already empty. That's why the
> commit log signifies the error being mostly related with Tx FIFO. But
> practically there are many reasons why Rx FIFO might be left with data:
> DMA engine failures, incorrect DMA configuration (if DW SPI or DW DMA driver
> messed something up), controller hanging up, and so on. It's better to catch
> an error at this stage while propagating it up to the SPI device drivers.
> Especially seeing the wait-check implementation doesn't gives us much of the
> execution overhead in normal conditions. So by calling dw_spi_dma_wait_rx_done()
> we make sure that all the data has been fetched and we may freely get the
> buffers back to the client driver.

I see your point about checking RX. But I still don't think checking
RX FIFO level is the right way to detect error. Some data left in
RX FIFO doesn't always mean a error, say for some case if there is
20 words in RX FIFO, and the driver starts a DMA request for 16
words, then after a sucessful DMA transaction, there are 4 words
left without any error.

Thanks,
Feng

> 
> -Sergey

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
  2020-05-21 14:55       ` Feng Tang
@ 2020-05-21 15:33         ` Serge Semin
  2020-05-22  7:58           ` Feng Tang
  0 siblings, 1 reply; 32+ messages in thread
From: Serge Semin @ 2020-05-21 15:33 UTC (permalink / raw)
  To: Feng Tang
  Cc: Serge Semin, Mark Brown, Grant Likely, Vinod Koul, Alan Cox,
	Linus Walleij, Georgy Vlasov, Ramil Zaripov, Alexey Malahov,
	Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Arnd Bergmann,
	Andy Shevchenko, Rob Herring, linux-mips, devicetree,
	Jarkko Nikula, Thomas Gleixner, Wan Ahmad Zainie, Linus Walleij,
	Clement Leger, linux-spi, linux-kernel

On Thu, May 21, 2020 at 10:55:20PM +0800, Feng Tang wrote:
> Hi Serge,
> 
> On Thu, May 21, 2020 at 02:47:36PM +0300, Serge Semin wrote:
> > Hello Feng,
> > 
> > On Thu, May 21, 2020 at 11:09:24AM +0800, Feng Tang wrote:
> > > Hi Serge,
> > > 
> > > On Thu, May 21, 2020 at 04:21:51AM +0300, Serge Semin wrote:
> > 
> > [nip]
> > 
> > > >  /*
> > > >   * dws->dma_chan_busy is set before the dma transfer starts, callback for rx
> > > >   * channel will clear a corresponding bit.
> > > > @@ -200,6 +267,8 @@ static void dw_spi_dma_rx_done(void *arg)
> > > >  {
> > > >  	struct dw_spi *dws = arg;
> > > >  
> > > > +	dw_spi_dma_wait_rx_done(dws);
> > > 
> > > I can understand the problem about TX, but I don't see how RX
> > > will get hurt, can you elaborate more? thanks
> > > 
> > > - Feng
> > 
> > Your question is correct. You are right with your hypothesis. Ideally upon the
> > dw_spi_dma_rx_done() execution Rx FIFO must be already empty. That's why the
> > commit log signifies the error being mostly related with Tx FIFO. But
> > practically there are many reasons why Rx FIFO might be left with data:
> > DMA engine failures, incorrect DMA configuration (if DW SPI or DW DMA driver
> > messed something up), controller hanging up, and so on. It's better to catch
> > an error at this stage while propagating it up to the SPI device drivers.
> > Especially seeing the wait-check implementation doesn't gives us much of the
> > execution overhead in normal conditions. So by calling dw_spi_dma_wait_rx_done()
> > we make sure that all the data has been fetched and we may freely get the
> > buffers back to the client driver.
> 
> I see your point about checking RX. But I still don't think checking
> RX FIFO level is the right way to detect error. Some data left in
> RX FIFO doesn't always mean a error, say for some case if there is
> 20 words in RX FIFO, and the driver starts a DMA request for 16
> words, then after a sucessful DMA transaction, there are 4 words
> left without any error.

Neither Tx nor Rx FIFO should be left with any data after transaction is
finished. If they are then something has been wrong.

See, every SPI transfer starts with FIFO clearance since we disable/enable the
SPI controller by means of the SSIENR (spi_enable_chip(dws, 0) and
spi_enable_chip(dws, 1) called in the dw_spi_transfer_one() callback). Here is the
SSIENR register description: "It enables and disables all SPI Controller operations.
When disabled, all serial transfers are halted immediately. Transmit and receive
FIFO buffers are cleared when the device is disabled. It is impossible to program
some of the SPI Controller control registers when enabled"

No mater whether we start DMA request or perform the normal IRQ-based PIO, we
request as much data as we need and neither Tx nor Rx FIFO are supposed to
be left with any data after the request is finished. If data is left, then
either we didn't push all of the necessary data to the SPI bus, or we didn't
pull all the data from the FIFO, and this could have happened only due to some
component mulfunction (drivers, DMA engine, SPI device). In any case the SPI
device driver should be notified about the problem.

-Sergey

> 
> Thanks,
> Feng
> 
> > 
> > -Sergey

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 03/16] spi: dw: Discard static DW DMA slave structures
       [not found]     ` <20200521121228.aqplh6eftylnys3p@mobilestation>
  2020-05-21 12:49       ` Andy Shevchenko
@ 2020-05-21 15:51       ` Mark Brown
  2020-05-21 15:56         ` Andy Shevchenko
  2020-05-21 15:58         ` Serge Semin
  1 sibling, 2 replies; 32+ messages in thread
From: Mark Brown @ 2020-05-21 15:51 UTC (permalink / raw)
  To: Serge Semin
  Cc: Andy Shevchenko, Serge Semin, Georgy Vlasov, Ramil Zaripov,
	Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
	Andy Shevchenko, Arnd Bergmann, Rob Herring, linux-mips,
	devicetree, Thomas Gleixner, Wan Ahmad Zainie, Jarkko Nikula,
	Clement Leger, linux-spi, Linux Kernel Mailing List

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

On Thu, May 21, 2020 at 03:12:28PM +0300, Serge Semin wrote:

> Well, for me both solutions are equal except mine consumes less stack memory.
> The only reason why your solution might be better is that if DW DMA driver or
> the DMA engine subsystem changed the dw_dma_slave structure instance passed to
> the dma_request_channel() method, which non of them do. So I'll leave this for
> Mark to decide. Mark, could you give us your final word about this?

Honestly I'm struggling to care either way.  I guess saving a bit of
stack is potentially useful.

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 03/16] spi: dw: Discard static DW DMA slave structures
  2020-05-21 15:51       ` Mark Brown
@ 2020-05-21 15:56         ` Andy Shevchenko
  2020-05-21 15:58         ` Serge Semin
  1 sibling, 0 replies; 32+ messages in thread
From: Andy Shevchenko @ 2020-05-21 15:56 UTC (permalink / raw)
  To: Mark Brown
  Cc: Serge Semin, Serge Semin, Georgy Vlasov, Ramil Zaripov,
	Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
	Andy Shevchenko, Arnd Bergmann, Rob Herring, linux-mips,
	devicetree, Thomas Gleixner, Wan Ahmad Zainie, Jarkko Nikula,
	Clement Leger, linux-spi, Linux Kernel Mailing List

On Thu, May 21, 2020 at 6:51 PM Mark Brown <broonie@kernel.org> wrote:
> On Thu, May 21, 2020 at 03:12:28PM +0300, Serge Semin wrote:
>
> > Well, for me both solutions are equal except mine consumes less stack memory.
> > The only reason why your solution might be better is that if DW DMA driver or
> > the DMA engine subsystem changed the dw_dma_slave structure instance passed to
> > the dma_request_channel() method, which non of them do. So I'll leave this for
> > Mark to decide. Mark, could you give us your final word about this?
>
> Honestly I'm struggling to care either way.  I guess saving a bit of
> stack is potentially useful.

Yes, but OTOH dropping maintainability by this is worse in my opinion.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 03/16] spi: dw: Discard static DW DMA slave structures
  2020-05-21 15:51       ` Mark Brown
  2020-05-21 15:56         ` Andy Shevchenko
@ 2020-05-21 15:58         ` Serge Semin
  2020-05-21 16:02           ` Andy Shevchenko
  1 sibling, 1 reply; 32+ messages in thread
From: Serge Semin @ 2020-05-21 15:58 UTC (permalink / raw)
  To: Mark Brown
  Cc: Serge Semin, Andy Shevchenko, Georgy Vlasov, Ramil Zaripov,
	Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
	Andy Shevchenko, Arnd Bergmann, Rob Herring, linux-mips,
	devicetree, Thomas Gleixner, Wan Ahmad Zainie, Jarkko Nikula,
	Clement Leger, linux-spi, Linux Kernel Mailing List

On Thu, May 21, 2020 at 04:51:43PM +0100, Mark Brown wrote:
> On Thu, May 21, 2020 at 03:12:28PM +0300, Serge Semin wrote:
> 
> > Well, for me both solutions are equal except mine consumes less stack memory.
> > The only reason why your solution might be better is that if DW DMA driver or
> > the DMA engine subsystem changed the dw_dma_slave structure instance passed to
> > the dma_request_channel() method, which non of them do. So I'll leave this for
> > Mark to decide. Mark, could you give us your final word about this?
> 
> Honestly I'm struggling to care either way.  I guess saving a bit of
> stack is potentially useful.

Settled then. Let's leave the patch as is. I suppose we've finally finished a
review except a question Feng asked to the patch:
[PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA

If you are ok with my responses, then the patchset is ready for you further
actions.

-Sergey

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 03/16] spi: dw: Discard static DW DMA slave structures
  2020-05-21 15:58         ` Serge Semin
@ 2020-05-21 16:02           ` Andy Shevchenko
  2020-05-21 16:39             ` Mark Brown
  0 siblings, 1 reply; 32+ messages in thread
From: Andy Shevchenko @ 2020-05-21 16:02 UTC (permalink / raw)
  To: Serge Semin
  Cc: Mark Brown, Serge Semin, Georgy Vlasov, Ramil Zaripov,
	Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
	Andy Shevchenko, Arnd Bergmann, Rob Herring, linux-mips,
	devicetree, Thomas Gleixner, Wan Ahmad Zainie, Jarkko Nikula,
	Clement Leger, linux-spi, Linux Kernel Mailing List

On Thu, May 21, 2020 at 6:58 PM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
> On Thu, May 21, 2020 at 04:51:43PM +0100, Mark Brown wrote:
> > On Thu, May 21, 2020 at 03:12:28PM +0300, Serge Semin wrote:
> >
> > > Well, for me both solutions are equal except mine consumes less stack memory.
> > > The only reason why your solution might be better is that if DW DMA driver or
> > > the DMA engine subsystem changed the dw_dma_slave structure instance passed to
> > > the dma_request_channel() method, which non of them do. So I'll leave this for
> > > Mark to decide. Mark, could you give us your final word about this?
> >
> > Honestly I'm struggling to care either way.  I guess saving a bit of
> > stack is potentially useful.
>
> Settled then.

With whom?

> Let's leave the patch as is.

Mark, should I send a partial revert afterwards in this case?
I'm not fully satisfied with it.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 03/16] spi: dw: Discard static DW DMA slave structures
  2020-05-21 16:02           ` Andy Shevchenko
@ 2020-05-21 16:39             ` Mark Brown
  2020-05-21 16:44               ` Andy Shevchenko
  2020-05-21 17:29               ` Serge Semin
  0 siblings, 2 replies; 32+ messages in thread
From: Mark Brown @ 2020-05-21 16:39 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Serge Semin, Serge Semin, Georgy Vlasov, Ramil Zaripov,
	Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
	Andy Shevchenko, Arnd Bergmann, Rob Herring, linux-mips,
	devicetree, Thomas Gleixner, Wan Ahmad Zainie, Jarkko Nikula,
	Clement Leger, linux-spi, Linux Kernel Mailing List

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

On Thu, May 21, 2020 at 07:02:32PM +0300, Andy Shevchenko wrote:
> On Thu, May 21, 2020 at 6:58 PM Serge Semin

> > Let's leave the patch as is.

> Mark, should I send a partial revert afterwards in this case?
> I'm not fully satisfied with it.

That might be a suitable way to keep the peace here.  You are clearly
both much more passionate about this choice than I am.

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 03/16] spi: dw: Discard static DW DMA slave structures
  2020-05-21 16:39             ` Mark Brown
@ 2020-05-21 16:44               ` Andy Shevchenko
  2020-05-21 17:29               ` Serge Semin
  1 sibling, 0 replies; 32+ messages in thread
From: Andy Shevchenko @ 2020-05-21 16:44 UTC (permalink / raw)
  To: Mark Brown
  Cc: Serge Semin, Serge Semin, Georgy Vlasov, Ramil Zaripov,
	Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
	Andy Shevchenko, Arnd Bergmann, Rob Herring, linux-mips,
	devicetree, Thomas Gleixner, Wan Ahmad Zainie, Jarkko Nikula,
	Clement Leger, linux-spi, Linux Kernel Mailing List

On Thu, May 21, 2020 at 7:39 PM Mark Brown <broonie@kernel.org> wrote:
> On Thu, May 21, 2020 at 07:02:32PM +0300, Andy Shevchenko wrote:
> > On Thu, May 21, 2020 at 6:58 PM Serge Semin
>
> > > Let's leave the patch as is.
>
> > Mark, should I send a partial revert afterwards in this case?
> > I'm not fully satisfied with it.
>
> That might be a suitable way to keep the peace here.  You are clearly
> both much more passionate about this choice than I am.

Okay, will work for me, thanks!

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 03/16] spi: dw: Discard static DW DMA slave structures
  2020-05-21 16:39             ` Mark Brown
  2020-05-21 16:44               ` Andy Shevchenko
@ 2020-05-21 17:29               ` Serge Semin
  1 sibling, 0 replies; 32+ messages in thread
From: Serge Semin @ 2020-05-21 17:29 UTC (permalink / raw)
  To: Mark Brown
  Cc: Serge Semin, Andy Shevchenko, Georgy Vlasov, Ramil Zaripov,
	Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
	Andy Shevchenko, Arnd Bergmann, Rob Herring, linux-mips,
	devicetree, Thomas Gleixner, Wan Ahmad Zainie, Jarkko Nikula,
	Clement Leger, linux-spi, Linux Kernel Mailing List

On Thu, May 21, 2020 at 05:39:04PM +0100, Mark Brown wrote:
> On Thu, May 21, 2020 at 07:02:32PM +0300, Andy Shevchenko wrote:
> > On Thu, May 21, 2020 at 6:58 PM Serge Semin
> 
> > > Let's leave the patch as is.
> 
> > Mark, should I send a partial revert afterwards in this case?
> > I'm not fully satisfied with it.
> 
> That might be a suitable way to keep the peace here.  You are clearly
> both much more passionate about this choice than I am.

Fine with me.)

-Sergey

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 00/16] spi: dw: Add generic DW DMA controller support
  2020-05-21  1:21 [PATCH v3 00/16] spi: dw: Add generic DW DMA controller support Serge Semin
                   ` (9 preceding siblings ...)
       [not found] ` <20200521012206.14472-10-Sergey.Semin@baikalelectronics.ru>
@ 2020-05-21 17:46 ` Andy Shevchenko
  10 siblings, 0 replies; 32+ messages in thread
From: Andy Shevchenko @ 2020-05-21 17:46 UTC (permalink / raw)
  To: Serge Semin
  Cc: Mark Brown, Serge Semin, Georgy Vlasov, Ramil Zaripov,
	Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko,
	Ekaterina Skachko, Vadim Vlasov, Alexey Kolotnikov,
	Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Arnd Bergmann,
	Rob Herring, linux-mips, linux-spi, devicetree, linux-kernel

On Thu, May 21, 2020 at 04:21:50AM +0300, Serge Semin wrote:
> Baikal-T1 SoC provides a DW DMA controller to perform low-speed peripherals
> Mem-to-Dev and Dev-to-Mem transaction. This is also applicable to the DW
> APB SSI devices embedded into the SoC. Currently the DMA-based transfers
> are supported by the DW APB SPI driver only as a middle layer code for
> Intel MID/Elkhart PCI devices. Seeing the same code can be used for normal
> platform DMAC device we introduced a set of patches to fix it within this
> series.
> 
> First of all we need to add the Tx and Rx DMA channels support into the DW
> APB SSI binding. Then there are several fixes and cleanups provided as a
> initial preparation for the Generic DMA support integration: add Tx/Rx
> finish wait methods, clear DMAC register when done or stopped, Fix native
> CS being unset, enable interrupts in accordance with DMA xfer mode,
> discard static DW DMA slave structures, discard unused void priv pointer
> and dma_width member of the dw_spi structure, provide the DMA Tx/Rx burst
> length parametrisation and make sure it's optionally set in accordance
> with the DMA max-burst capability.
> 
> In order to have the DW APB SSI MMIO driver working with DMA we need to
> initialize the paddr field with the physical base address of the DW APB SSI
> registers space. Then we unpin the Intel MID specific code from the
> generic DMA one and placed it into the spi-dw-pci.c driver, which is a
> better place for it anyway. After that the naming cleanups are performed
> since the code is going to be used for a generic DMAC device. Finally the
> Generic DMA initialization can be added to the generic version of the
> DW APB SSI IP.
> 
> Last but not least we traditionally convert the legacy plain text-based
> dt-binding file with yaml-based one and as a cherry on a cake replace
> the manually written DebugFS registers read method with a ready-to-use
> for the same purpose regset32 DebugFS interface usage.

I have given tags where I agree with content and I left disputed ones untouched
(1st patch, nevertheless, is fine for me, but I'm waiting for Feng).

So, now it seems settled in a way that I will send couple of cleanups
afterwards to avoid blocking this series to go.

Thanks for your work!

> This patchset is rebased and tested on the spi/for-next (5.7-rc5):
> base-commit: fe9fce6b2cf3 ("Merge remote-tracking branch 'spi/for-5.8' into spi-next")
> 
> Link: https://lore.kernel.org/linux-spi/20200508132943.9826-1-Sergey.Semin@baikalelectronics.ru/
> Changelog v2:
> - Rebase on top of the spi repository for-next branch.
> - Move bindings conversion patch to the tail of the series.
> - Move fixes to the head of the series.
> - Apply as many changes as possible to be applied the Generic DMA
>   functionality support is added and the spi-dw-mid is moved to the
>   spi-dw-dma driver.
> - Discard patch "spi: dw: Fix dma_slave_config used partly uninitialized"
>   since the problem has already been fixed.
> - Add new patch "spi: dw: Discard unused void priv pointer".
> - Add new patch "spi: dw: Discard dma_width member of the dw_spi structure".
>   n_bytes member of the DW SPI data can be used instead.
> - Build the DMA functionality into the DW APB SSI core if required instead
>   of creating a separate kernel module.
> - Use conditional statement instead of the ternary operator in the ref
>   clock getter.
> 
> Link: https://lore.kernel.org/linux-spi/20200515104758.6934-1-Sergey.Semin@baikalelectronics.ru/
> Changelog v3:
> - Use spi_delay_exec() method to wait for the DMA operation completion.
> - Explicitly initialize the dw_dma_slave members on stack.
> - Discard the dws->fifo_len utilization in the Tx FIFO DMA threshold
>   setting from the patch where we just add the default burst length
>   constants.
> - Use min() method to calculate the optimal burst values.
> - Add new patch which moves the spi-dw.c source file to spi-dw-core.c in
>   order to preserve the DW APB SSI core driver name.
> - Add commas in the debugfs_reg32 structure initializer and after the last
>   entry of the dw_spi_dbgfs_regs array.
> 
> Co-developed-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
> Signed-off-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
> Co-developed-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
> Signed-off-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Maxim Kaurkin <Maxim.Kaurkin@baikalelectronics.ru>
> Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> Cc: Ekaterina Skachko <Ekaterina.Skachko@baikalelectronics.ru>
> Cc: Vadim Vlasov <V.Vlasov@baikalelectronics.ru>
> Cc: Alexey Kolotnikov <Alexey.Kolotnikov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-mips@vger.kernel.org
> Cc: linux-spi@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> 
> Serge Semin (16):
>   spi: dw: Add Tx/Rx finish wait methods to the MID DMA
>   spi: dw: Enable interrupts in accordance with DMA xfer mode
>   spi: dw: Discard static DW DMA slave structures
>   spi: dw: Discard unused void priv pointer
>   spi: dw: Discard dma_width member of the dw_spi structure
>   spi: dw: Parameterize the DMA Rx/Tx burst length
>   spi: dw: Use DMA max burst to set the request thresholds
>   spi: dw: Fix Rx-only DMA transfers
>   spi: dw: Add core suffix to the DW APB SSI core source file
>   spi: dw: Move Non-DMA code to the DW PCIe-SPI driver
>   spi: dw: Remove DW DMA code dependency from DW_DMAC_PCI
>   spi: dw: Add DW SPI DMA/PCI/MMIO dependency on the DW SPI core
>   spi: dw: Cleanup generic DW DMA code namings
>   spi: dw: Add DMA support to the DW SPI MMIO driver
>   spi: dw: Use regset32 DebugFS method to create regdump file
>   dt-bindings: spi: Convert DW SPI binding to DT schema
> 
>  .../bindings/spi/snps,dw-apb-ssi.txt          |  44 ---
>  .../bindings/spi/snps,dw-apb-ssi.yaml         | 127 ++++++++
>  .../devicetree/bindings/spi/spi-dw.txt        |  24 --
>  drivers/spi/Kconfig                           |  15 +-
>  drivers/spi/Makefile                          |   5 +-
>  drivers/spi/{spi-dw.c => spi-dw-core.c}       |  88 ++----
>  drivers/spi/{spi-dw-mid.c => spi-dw-dma.c}    | 276 +++++++++++-------
>  drivers/spi/spi-dw-mmio.c                     |   4 +
>  drivers/spi/spi-dw-pci.c                      |  50 +++-
>  drivers/spi/spi-dw.h                          |  33 ++-
>  10 files changed, 407 insertions(+), 259 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt
>  create mode 100644 Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>  delete mode 100644 Documentation/devicetree/bindings/spi/spi-dw.txt
>  rename drivers/spi/{spi-dw.c => spi-dw-core.c} (82%)
>  rename drivers/spi/{spi-dw-mid.c => spi-dw-dma.c} (53%)
> 
> -- 
> 2.25.1
> 

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
  2020-05-21  1:21 ` [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA Serge Semin
  2020-05-21  3:09   ` Feng Tang
@ 2020-05-21 23:33   ` Serge Semin
  1 sibling, 0 replies; 32+ messages in thread
From: Serge Semin @ 2020-05-21 23:33 UTC (permalink / raw)
  To: Mark Brown, Grant Likely, Vinod Koul, Feng Tang, Alan Cox, Linus Walleij
  Cc: Serge Semin, Georgy Vlasov, Ramil Zaripov, Alexey Malahov,
	Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Arnd Bergmann,
	Andy Shevchenko, Rob Herring, linux-mips, devicetree,
	Jarkko Nikula, Thomas Gleixner, Wan Ahmad Zainie, Linus Walleij,
	Clement Leger, linux-spi, linux-kernel

Mark, Andy,

On Thu, May 21, 2020 at 04:21:51AM +0300, Serge Semin wrote:
>  

[nip]

> +static void dw_spi_dma_calc_delay(struct dw_spi *dws, u32 nents,
> +				  struct spi_delay *delay)
> +{
> +	unsigned long ns, us;
> +
> +	ns = (NSEC_PER_SEC / spi_get_clk(dws)) * nents * dws->n_bytes *
> +	     BITS_PER_BYTE;
> +
> +	if (ns <= NSEC_PER_USEC) {
> +		delay->unit = SPI_DELAY_UNIT_NSECS;
> +		delay->value = ns;
> +	} else {
> +		us = DIV_ROUND_UP(ns, NSEC_PER_USEC);
> +		delay->unit = SPI_DELAY_UNIT_USECS;
> +		delay->value = clamp_val(us, 0, USHRT_MAX);
> +	}
> +}
> +
> +static inline bool dw_spi_dma_tx_busy(struct dw_spi *dws)
> +{
> +	return !(dw_readl(dws, DW_SPI_SR) & SR_TF_EMPT);
> +}
> +
> +static void dw_spi_dma_wait_tx_done(struct dw_spi *dws)
> +{
> +	int retry = WAIT_RETRIES;
> +	struct spi_delay delay;
> +	u32 nents;
> +
> +	nents = dw_readl(dws, DW_SPI_TXFLR);
> +	dw_spi_dma_calc_delay(dws, nents, &delay);
> +
> +	while (dw_spi_dma_tx_busy(dws) && retry--)

> +		spi_delay_exec(&delay, NULL);

I've just discovered using spi_delay_exec() wasn't a good idea here. Look at the
call stack:
dw_dma_tasklet() -> dwc_scan_descriptors() -> dwc_descriptor_complete() ->
dw_spi_dma_tx_done() -> spi_delay_exec() -> usleep_range() -> ...

So tasklet is calling a sleeping function.((( I've absolutely forgotten to check
the context the DMA completion function is called with. We'll have to manually
select either ndelay or udelay here and nothing else. Since basically both
functions represent an atomic context and most of the platforms ndelay fallsback to
udelay, I'll get the ndelay back to the wait functions. I'll resend a patchset
shortly.

-Sergey

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
  2020-05-21 15:33         ` Serge Semin
@ 2020-05-22  7:58           ` Feng Tang
  2020-05-22 11:32             ` Serge Semin
  0 siblings, 1 reply; 32+ messages in thread
From: Feng Tang @ 2020-05-22  7:58 UTC (permalink / raw)
  To: Serge Semin
  Cc: Serge Semin, Mark Brown, Grant Likely, Vinod Koul, Alan Cox,
	Linus Walleij, Georgy Vlasov, Ramil Zaripov, Alexey Malahov,
	Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Arnd Bergmann,
	Andy Shevchenko, Rob Herring, linux-mips, devicetree,
	Jarkko Nikula, Thomas Gleixner, Wan Ahmad Zainie, Linus Walleij,
	Clement Leger, linux-spi, linux-kernel

Hi Serge,

On Thu, May 21, 2020 at 06:33:17PM +0300, Serge Semin wrote:
> > > > > +	dw_spi_dma_wait_rx_done(dws);
> > > > 
> > > > I can understand the problem about TX, but I don't see how RX
> > > > will get hurt, can you elaborate more? thanks
> > > > 
> > > > - Feng
> > > 
> > > Your question is correct. You are right with your hypothesis. Ideally upon the
> > > dw_spi_dma_rx_done() execution Rx FIFO must be already empty. That's why the
> > > commit log signifies the error being mostly related with Tx FIFO. But
> > > practically there are many reasons why Rx FIFO might be left with data:
> > > DMA engine failures, incorrect DMA configuration (if DW SPI or DW DMA driver
> > > messed something up), controller hanging up, and so on. It's better to catch
> > > an error at this stage while propagating it up to the SPI device drivers.
> > > Especially seeing the wait-check implementation doesn't gives us much of the
> > > execution overhead in normal conditions. So by calling dw_spi_dma_wait_rx_done()
> > > we make sure that all the data has been fetched and we may freely get the
> > > buffers back to the client driver.
> > 
> > I see your point about checking RX. But I still don't think checking
> > RX FIFO level is the right way to detect error. Some data left in
> > RX FIFO doesn't always mean a error, say for some case if there is
> > 20 words in RX FIFO, and the driver starts a DMA request for 16
> > words, then after a sucessful DMA transaction, there are 4 words
> > left without any error.
> 
> Neither Tx nor Rx FIFO should be left with any data after transaction is
> finished. If they are then something has been wrong.
> 
> See, every SPI transfer starts with FIFO clearance since we disable/enable the
> SPI controller by means of the SSIENR (spi_enable_chip(dws, 0) and
> spi_enable_chip(dws, 1) called in the dw_spi_transfer_one() callback). Here is the
> SSIENR register description: "It enables and disables all SPI Controller operations.
> When disabled, all serial transfers are halted immediately. Transmit and receive
> FIFO buffers are cleared when the device is disabled. It is impossible to program
> some of the SPI Controller control registers when enabled"
> 
> No mater whether we start DMA request or perform the normal IRQ-based PIO, we
> request as much data as we need and neither Tx nor Rx FIFO are supposed to
> be left with any data after the request is finished. If data is left, then
> either we didn't push all of the necessary data to the SPI bus, or we didn't
> pull all the data from the FIFO, and this could have happened only due to some
> component mulfunction (drivers, DMA engine, SPI device). In any case the SPI
> device driver should be notified about the problem.

Data left in TX FIFO and Data left in RX FIFO are 2 different stories. The
former in dma case means the dma hw/driver has done its job, and spi hw/driver
hasn't done its job of pushing out the data to spi slave devices, while the
latter means the spi hw/driver has done its job, while the dma hw/driver hasn't. 

And the code is called inside the dma rx channel callback, which means the
dma driver is saying "hey, I've done my job", but apparently it hasn't if
there is data left.

As for the wait time

+	nents = dw_readl(dws, DW_SPI_RXFLR);
+	ns = (NSEC_PER_SEC / spi_get_clk(dws)) * nents * dws->n_bytes *
+	     BITS_PER_BYTE;

Using this formula for checking TX makes sense, but it doesn't for RX.
Because the time of pushing data in TX FIFO to spi device depends on
the clk, but the time of transferring RX FIFO to memory is up to
the DMA controller and peripheral bus. 

Also for the

+	while (dw_spi_dma_rx_busy(dws) && retry--)
+		ndelay(ns);
+

the rx busy bit is cleared after this rx/tx checking, and it should
be always true at this point. Am I mis-reading the code?

Thanks,
Feng

> 
> -Sergey
> 

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
  2020-05-22  7:58           ` Feng Tang
@ 2020-05-22 11:32             ` Serge Semin
  2020-05-22 12:03               ` Feng Tang
  0 siblings, 1 reply; 32+ messages in thread
From: Serge Semin @ 2020-05-22 11:32 UTC (permalink / raw)
  To: Feng Tang
  Cc: Serge Semin, Mark Brown, Grant Likely, Vinod Koul, Alan Cox,
	Linus Walleij, Georgy Vlasov, Ramil Zaripov, Alexey Malahov,
	Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Arnd Bergmann,
	Andy Shevchenko, Rob Herring, linux-mips, devicetree,
	Jarkko Nikula, Thomas Gleixner, Wan Ahmad Zainie, Linus Walleij,
	Clement Leger, linux-spi, linux-kernel

On Fri, May 22, 2020 at 03:58:44PM +0800, Feng Tang wrote:
> Hi Serge,
> 
> On Thu, May 21, 2020 at 06:33:17PM +0300, Serge Semin wrote:
> > > > > > +	dw_spi_dma_wait_rx_done(dws);
> > > > > 
> > > > > I can understand the problem about TX, but I don't see how RX
> > > > > will get hurt, can you elaborate more? thanks
> > > > > 
> > > > > - Feng
> > > > 
> > > > Your question is correct. You are right with your hypothesis. Ideally upon the
> > > > dw_spi_dma_rx_done() execution Rx FIFO must be already empty. That's why the
> > > > commit log signifies the error being mostly related with Tx FIFO. But
> > > > practically there are many reasons why Rx FIFO might be left with data:
> > > > DMA engine failures, incorrect DMA configuration (if DW SPI or DW DMA driver
> > > > messed something up), controller hanging up, and so on. It's better to catch
> > > > an error at this stage while propagating it up to the SPI device drivers.
> > > > Especially seeing the wait-check implementation doesn't gives us much of the
> > > > execution overhead in normal conditions. So by calling dw_spi_dma_wait_rx_done()
> > > > we make sure that all the data has been fetched and we may freely get the
> > > > buffers back to the client driver.
> > > 
> > > I see your point about checking RX. But I still don't think checking
> > > RX FIFO level is the right way to detect error. Some data left in
> > > RX FIFO doesn't always mean a error, say for some case if there is
> > > 20 words in RX FIFO, and the driver starts a DMA request for 16
> > > words, then after a sucessful DMA transaction, there are 4 words
> > > left without any error.
> > 
> > Neither Tx nor Rx FIFO should be left with any data after transaction is
> > finished. If they are then something has been wrong.
> > 
> > See, every SPI transfer starts with FIFO clearance since we disable/enable the
> > SPI controller by means of the SSIENR (spi_enable_chip(dws, 0) and
> > spi_enable_chip(dws, 1) called in the dw_spi_transfer_one() callback). Here is the
> > SSIENR register description: "It enables and disables all SPI Controller operations.
> > When disabled, all serial transfers are halted immediately. Transmit and receive
> > FIFO buffers are cleared when the device is disabled. It is impossible to program
> > some of the SPI Controller control registers when enabled"
> > 
> > No mater whether we start DMA request or perform the normal IRQ-based PIO, we
> > request as much data as we need and neither Tx nor Rx FIFO are supposed to
> > be left with any data after the request is finished. If data is left, then
> > either we didn't push all of the necessary data to the SPI bus, or we didn't
> > pull all the data from the FIFO, and this could have happened only due to some
> > component mulfunction (drivers, DMA engine, SPI device). In any case the SPI
> > device driver should be notified about the problem.
> 
> Data left in TX FIFO and Data left in RX FIFO are 2 different stories. The
> former in dma case means the dma hw/driver has done its job, and spi hw/driver
> hasn't done its job of pushing out the data to spi slave devices,

Agreed.

> while the
> latter means the spi hw/driver has done its job, while the dma hw/driver hasn't.

In this particular case agreed, that the data left in the Rx FIFO means DMA
hw/driver hasn't done its work right. Though SPI hw could be also a reason of
the data left in FIFO (though this only a theoretical consideration).

> 
> And the code is called inside the dma rx channel callback, which means the
> dma driver is saying "hey, I've done my job", but apparently it hasn't if
> there is data left.

Right, either it hasn't, or the DMA engine claimed it has, but still is doing
something (asynchronously or something, depending on the hardware implementation),
or it think it has, but in fact it hasn't due to whatever problem happened
(software/hardware/etc.). In anyway we have to at least check whether it's
really done with fetching data and to be on a safe side give it some time to
make sure that the Rx FIFO isn't going to be emptied. Whatever problem it is
having a non empty Rx FIFO at the stage of calling spi_finalize_current_transfer()
means a certain error.

> 
> As for the wait time
> 
> +	nents = dw_readl(dws, DW_SPI_RXFLR);
> +	ns = (NSEC_PER_SEC / spi_get_clk(dws)) * nents * dws->n_bytes *
> +	     BITS_PER_BYTE;
> 
> Using this formula for checking TX makes sense, but it doesn't for RX.
> Because the time of pushing data in TX FIFO to spi device depends on
> the clk, but the time of transferring RX FIFO to memory is up to
> the DMA controller and peripheral bus. 

On this I agree with you. That formulae doesn't describe exactly the time left
before the Rx FIFO gets empty. But at least it provides an upper limit on the
time needed for the peripheral bus to fetch the data from FIFO. If for some
reason the internal APB bus is slower than the SPI bus, then the hardware
engineers screwed, since the CPU/DMA won't keep up with pulling data from Rx
FIFO on time so the FIFO may get overflown. Though in this case CPU/DMA won't
be able to push data to the Tx FIFO fast enough to cause the Rx FIFO overflown,
so the problem might be unnoticeable until we enable the EEPROM-read or Rx-only
modes of the DW APB SSI controller. Anyway I am pretty much sure all the systems
have the internal bus much faster than the external SPI bus.

Getting back to the formulae. I was thinking of how to make it better and here
is what we can do. We can't predict neither the DMA controller performance,
nor the performance of its driver. In this case we have no choice but to add
some assumption to clarify the task. Let's assume that the reason why Rx FIFO is
non-empty is that even though we are at the DMA completion callback, but the
DMA controller is still fetching data in background (any other reason might be
related with a bug, so we'll detect it here anyway). In this case we need to
give it a time to finish its work. As far as I can see the DW_apb_ssi interface
doesn't use PREADY APB signal, which means the IO access cycle will take 4
reference clock periods for each read and write accesses. Thus taking all of
these into account we can create the next formulae to measure the time needed to
read all the data from the Rx FIFO:

-	ns = (NSEC_PER_SEC / spi_get_clk(dws)) * nents * dws->n_bytes *
-	     BITS_PER_BYTE;
+	ns = (NSEC_PER_SEC / dws->max_freq) * nents * 4;

By doing several busy-wait loop iteration we'll cover the DMA controller and
it's driver possible latency. 

Feng, does it now makes sense for you now? If so, I'll replace the delay
calculation formulae in the patch.

> 
> Also for the
> 
> +	while (dw_spi_dma_rx_busy(dws) && retry--)
> +		ndelay(ns);
> +
> 
> the rx busy bit is cleared after this rx/tx checking, and it should
> be always true at this point. Am I mis-reading the code?

Sorry I don't get your logic here. I am not checking the Rx busy bit here,
but the Rx FIFO non-empty bit. Also SR register bits aren't cleared on read,
so the status bits are left pending until the reason is cleared. In our case
until Rx FIFO gets empty, which will happen eventually either at the point of
all data finally being extracted from it or when the controller is disabled
by means of the SSIENR register.

-Sergey

> 
> Thanks,
> Feng
> 
> > 
> > -Sergey
> > 

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
  2020-05-22 11:32             ` Serge Semin
@ 2020-05-22 12:03               ` Feng Tang
  2020-05-22 12:25                 ` Serge Semin
  0 siblings, 1 reply; 32+ messages in thread
From: Feng Tang @ 2020-05-22 12:03 UTC (permalink / raw)
  To: Serge Semin
  Cc: Serge Semin, Mark Brown, Grant Likely, Vinod Koul, Alan Cox,
	Linus Walleij, Georgy Vlasov, Ramil Zaripov, Alexey Malahov,
	Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Arnd Bergmann,
	Andy Shevchenko, Rob Herring, linux-mips, devicetree,
	Jarkko Nikula, Thomas Gleixner, Wan Ahmad Zainie, Linus Walleij,
	Clement Leger, linux-spi, linux-kernel

On Fri, May 22, 2020 at 02:32:35PM +0300, Serge Semin wrote:
> On Fri, May 22, 2020 at 03:58:44PM +0800, Feng Tang wrote:
> > Hi Serge,
> > 
> > On Thu, May 21, 2020 at 06:33:17PM +0300, Serge Semin wrote:
> > > > > > > +	dw_spi_dma_wait_rx_done(dws);
> > > > > > 
> > > > > > I can understand the problem about TX, but I don't see how RX
> > > > > > will get hurt, can you elaborate more? thanks
> > > > > > 
> > > > > > - Feng
> > > > > 
> > > > > Your question is correct. You are right with your hypothesis. Ideally upon the
> > > > > dw_spi_dma_rx_done() execution Rx FIFO must be already empty. That's why the
> > > > > commit log signifies the error being mostly related with Tx FIFO. But
> > > > > practically there are many reasons why Rx FIFO might be left with data:
> > > > > DMA engine failures, incorrect DMA configuration (if DW SPI or DW DMA driver
> > > > > messed something up), controller hanging up, and so on. It's better to catch
> > > > > an error at this stage while propagating it up to the SPI device drivers.
> > > > > Especially seeing the wait-check implementation doesn't gives us much of the
> > > > > execution overhead in normal conditions. So by calling dw_spi_dma_wait_rx_done()
> > > > > we make sure that all the data has been fetched and we may freely get the
> > > > > buffers back to the client driver.
> > > > 
> > > > I see your point about checking RX. But I still don't think checking
> > > > RX FIFO level is the right way to detect error. Some data left in
> > > > RX FIFO doesn't always mean a error, say for some case if there is
> > > > 20 words in RX FIFO, and the driver starts a DMA request for 16
> > > > words, then after a sucessful DMA transaction, there are 4 words
> > > > left without any error.
> > > 
> > > Neither Tx nor Rx FIFO should be left with any data after transaction is
> > > finished. If they are then something has been wrong.
> > > 
> > > See, every SPI transfer starts with FIFO clearance since we disable/enable the
> > > SPI controller by means of the SSIENR (spi_enable_chip(dws, 0) and
> > > spi_enable_chip(dws, 1) called in the dw_spi_transfer_one() callback). Here is the
> > > SSIENR register description: "It enables and disables all SPI Controller operations.
> > > When disabled, all serial transfers are halted immediately. Transmit and receive
> > > FIFO buffers are cleared when the device is disabled. It is impossible to program
> > > some of the SPI Controller control registers when enabled"
> > > 
> > > No mater whether we start DMA request or perform the normal IRQ-based PIO, we
> > > request as much data as we need and neither Tx nor Rx FIFO are supposed to
> > > be left with any data after the request is finished. If data is left, then
> > > either we didn't push all of the necessary data to the SPI bus, or we didn't
> > > pull all the data from the FIFO, and this could have happened only due to some
> > > component mulfunction (drivers, DMA engine, SPI device). In any case the SPI
> > > device driver should be notified about the problem.
> > 
> > Data left in TX FIFO and Data left in RX FIFO are 2 different stories. The
> > former in dma case means the dma hw/driver has done its job, and spi hw/driver
> > hasn't done its job of pushing out the data to spi slave devices,
> 
> Agreed.
> 
> > while the
> > latter means the spi hw/driver has done its job, while the dma hw/driver hasn't.
> 
> In this particular case agreed, that the data left in the Rx FIFO means DMA
> hw/driver hasn't done its work right. Though SPI hw could be also a reason of
> the data left in FIFO (though this only a theoretical consideration).

Right, that's why I was initially very curious about this RX FIFO thing,
and if possible, please give some details in commit log about the data
left in TX FIFO problem, which will help future developers when they
met simliar bugs.

And I'm fine with adding the rx check, no matter the problem is in
dma side or spi side.

> > 
> > And the code is called inside the dma rx channel callback, which means the
> > dma driver is saying "hey, I've done my job", but apparently it hasn't if
> > there is data left.
> 
> Right, either it hasn't, or the DMA engine claimed it has, but still is doing
> something (asynchronously or something, depending on the hardware implementation),
> or it think it has, but in fact it hasn't due to whatever problem happened
> (software/hardware/etc.). In anyway we have to at least check whether it's
> really done with fetching data and to be on a safe side give it some time to
> make sure that the Rx FIFO isn't going to be emptied. Whatever problem it is
> having a non empty Rx FIFO at the stage of calling spi_finalize_current_transfer()
> means a certain error.
> 
> > 
> > As for the wait time
> > 
> > +	nents = dw_readl(dws, DW_SPI_RXFLR);
> > +	ns = (NSEC_PER_SEC / spi_get_clk(dws)) * nents * dws->n_bytes *
> > +	     BITS_PER_BYTE;
> > 
> > Using this formula for checking TX makes sense, but it doesn't for RX.
> > Because the time of pushing data in TX FIFO to spi device depends on
> > the clk, but the time of transferring RX FIFO to memory is up to
> > the DMA controller and peripheral bus. 
> 
> On this I agree with you. That formulae doesn't describe exactly the time left
> before the Rx FIFO gets empty. But at least it provides an upper limit on the
> time needed for the peripheral bus to fetch the data from FIFO. If for some
> reason the internal APB bus is slower than the SPI bus, then the hardware
> engineers screwed, since the CPU/DMA won't keep up with pulling data from Rx
> FIFO on time so the FIFO may get overflown. Though in this case CPU/DMA won't
> be able to push data to the Tx FIFO fast enough to cause the Rx FIFO overflown,
> so the problem might be unnoticeable until we enable the EEPROM-read or Rx-only
> modes of the DW APB SSI controller. Anyway I am pretty much sure all the systems
> have the internal bus much faster than the external SPI bus.
> 
> Getting back to the formulae. I was thinking of how to make it better and here
> is what we can do. We can't predict neither the DMA controller performance,
> nor the performance of its driver. In this case we have no choice but to add
> some assumption to clarify the task. Let's assume that the reason why Rx FIFO is
> non-empty is that even though we are at the DMA completion callback, but the
> DMA controller is still fetching data in background (any other reason might be
> related with a bug, so we'll detect it here anyway). In this case we need to
> give it a time to finish its work. As far as I can see the DW_apb_ssi interface
> doesn't use PREADY APB signal, which means the IO access cycle will take 4
> reference clock periods for each read and write accesses. Thus taking all of
> these into account we can create the next formulae to measure the time needed to
> read all the data from the Rx FIFO:
> 
> -	ns = (NSEC_PER_SEC / spi_get_clk(dws)) * nents * dws->n_bytes *
> -	     BITS_PER_BYTE;
> +	ns = (NSEC_PER_SEC / dws->max_freq) * nents * 4;
> 
> By doing several busy-wait loop iteration we'll cover the DMA controller and
> it's driver possible latency. 
> 
> Feng, does it now makes sense for you now? If so, I'll replace the delay
> calculation formulae in the patch.

Frankly I don't have a good idea, if it really happens which means
something is abnormal, explicitly waiting for some micro-seconds may
also be acceptable?

> > 
> > Also for the
> > 
> > +	while (dw_spi_dma_rx_busy(dws) && retry--)
> > +		ndelay(ns);
> > +
> > 
> > the rx busy bit is cleared after this rx/tx checking, and it should
> > be always true at this point. Am I mis-reading the code?
> 
> Sorry I don't get your logic here. I am not checking the Rx busy bit here,
> but the Rx FIFO non-empty bit. Also SR register bits aren't cleared on read,
> so the status bits are left pending until the reason is cleared. In our case
> until Rx FIFO gets empty, which will happen eventually either at the point of
> all data finally being extracted from it or when the controller is disabled
> by means of the SSIENR register.

I did misread the code, I thought it is checking the busy bits, sorry
for that. Though the dw_spi_dma_rx_busy() name is a little confusing,
as checking the emptiness of RX FIFO is not dma bound.

Thanks,
Feng

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
  2020-05-22 12:03               ` Feng Tang
@ 2020-05-22 12:25                 ` Serge Semin
  0 siblings, 0 replies; 32+ messages in thread
From: Serge Semin @ 2020-05-22 12:25 UTC (permalink / raw)
  To: Feng Tang
  Cc: Serge Semin, Mark Brown, Grant Likely, Vinod Koul, Alan Cox,
	Linus Walleij, Georgy Vlasov, Ramil Zaripov, Alexey Malahov,
	Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Arnd Bergmann,
	Andy Shevchenko, Rob Herring, linux-mips, devicetree,
	Jarkko Nikula, Thomas Gleixner, Wan Ahmad Zainie, Linus Walleij,
	Clement Leger, linux-spi, linux-kernel

On Fri, May 22, 2020 at 08:03:25PM +0800, Feng Tang wrote:
> On Fri, May 22, 2020 at 02:32:35PM +0300, Serge Semin wrote:
> > On Fri, May 22, 2020 at 03:58:44PM +0800, Feng Tang wrote:
> > > Hi Serge,
> > > 
> > > On Thu, May 21, 2020 at 06:33:17PM +0300, Serge Semin wrote:
> > > > > > > > +	dw_spi_dma_wait_rx_done(dws);
> > > > > > > 
> > > > > > > I can understand the problem about TX, but I don't see how RX
> > > > > > > will get hurt, can you elaborate more? thanks
> > > > > > > 
> > > > > > > - Feng
> > > > > > 
> > > > > > Your question is correct. You are right with your hypothesis. Ideally upon the
> > > > > > dw_spi_dma_rx_done() execution Rx FIFO must be already empty. That's why the
> > > > > > commit log signifies the error being mostly related with Tx FIFO. But
> > > > > > practically there are many reasons why Rx FIFO might be left with data:
> > > > > > DMA engine failures, incorrect DMA configuration (if DW SPI or DW DMA driver
> > > > > > messed something up), controller hanging up, and so on. It's better to catch
> > > > > > an error at this stage while propagating it up to the SPI device drivers.
> > > > > > Especially seeing the wait-check implementation doesn't gives us much of the
> > > > > > execution overhead in normal conditions. So by calling dw_spi_dma_wait_rx_done()
> > > > > > we make sure that all the data has been fetched and we may freely get the
> > > > > > buffers back to the client driver.
> > > > > 
> > > > > I see your point about checking RX. But I still don't think checking
> > > > > RX FIFO level is the right way to detect error. Some data left in
> > > > > RX FIFO doesn't always mean a error, say for some case if there is
> > > > > 20 words in RX FIFO, and the driver starts a DMA request for 16
> > > > > words, then after a sucessful DMA transaction, there are 4 words
> > > > > left without any error.
> > > > 
> > > > Neither Tx nor Rx FIFO should be left with any data after transaction is
> > > > finished. If they are then something has been wrong.
> > > > 
> > > > See, every SPI transfer starts with FIFO clearance since we disable/enable the
> > > > SPI controller by means of the SSIENR (spi_enable_chip(dws, 0) and
> > > > spi_enable_chip(dws, 1) called in the dw_spi_transfer_one() callback). Here is the
> > > > SSIENR register description: "It enables and disables all SPI Controller operations.
> > > > When disabled, all serial transfers are halted immediately. Transmit and receive
> > > > FIFO buffers are cleared when the device is disabled. It is impossible to program
> > > > some of the SPI Controller control registers when enabled"
> > > > 
> > > > No mater whether we start DMA request or perform the normal IRQ-based PIO, we
> > > > request as much data as we need and neither Tx nor Rx FIFO are supposed to
> > > > be left with any data after the request is finished. If data is left, then
> > > > either we didn't push all of the necessary data to the SPI bus, or we didn't
> > > > pull all the data from the FIFO, and this could have happened only due to some
> > > > component mulfunction (drivers, DMA engine, SPI device). In any case the SPI
> > > > device driver should be notified about the problem.
> > > 
> > > Data left in TX FIFO and Data left in RX FIFO are 2 different stories. The
> > > former in dma case means the dma hw/driver has done its job, and spi hw/driver
> > > hasn't done its job of pushing out the data to spi slave devices,
> > 
> > Agreed.
> > 
> > > while the
> > > latter means the spi hw/driver has done its job, while the dma hw/driver hasn't.
> > 
> > In this particular case agreed, that the data left in the Rx FIFO means DMA
> > hw/driver hasn't done its work right. Though SPI hw could be also a reason of
> > the data left in FIFO (though this only a theoretical consideration).
> 
> Right, that's why I was initially very curious about this RX FIFO thing,
> and if possible, please give some details in commit log about the data
> left in TX FIFO problem, which will help future developers when they
> met simliar bugs.

Ok. I'll add a more descriptive patch log.

> 
> And I'm fine with adding the rx check, no matter the problem is in
> dma side or spi side.
> 
> > > 
> > > And the code is called inside the dma rx channel callback, which means the
> > > dma driver is saying "hey, I've done my job", but apparently it hasn't if
> > > there is data left.
> > 
> > Right, either it hasn't, or the DMA engine claimed it has, but still is doing
> > something (asynchronously or something, depending on the hardware implementation),
> > or it think it has, but in fact it hasn't due to whatever problem happened
> > (software/hardware/etc.). In anyway we have to at least check whether it's
> > really done with fetching data and to be on a safe side give it some time to
> > make sure that the Rx FIFO isn't going to be emptied. Whatever problem it is
> > having a non empty Rx FIFO at the stage of calling spi_finalize_current_transfer()
> > means a certain error.
> > 
> > > 
> > > As for the wait time
> > > 
> > > +	nents = dw_readl(dws, DW_SPI_RXFLR);
> > > +	ns = (NSEC_PER_SEC / spi_get_clk(dws)) * nents * dws->n_bytes *
> > > +	     BITS_PER_BYTE;
> > > 
> > > Using this formula for checking TX makes sense, but it doesn't for RX.
> > > Because the time of pushing data in TX FIFO to spi device depends on
> > > the clk, but the time of transferring RX FIFO to memory is up to
> > > the DMA controller and peripheral bus. 
> > 
> > On this I agree with you. That formulae doesn't describe exactly the time left
> > before the Rx FIFO gets empty. But at least it provides an upper limit on the
> > time needed for the peripheral bus to fetch the data from FIFO. If for some
> > reason the internal APB bus is slower than the SPI bus, then the hardware
> > engineers screwed, since the CPU/DMA won't keep up with pulling data from Rx
> > FIFO on time so the FIFO may get overflown. Though in this case CPU/DMA won't
> > be able to push data to the Tx FIFO fast enough to cause the Rx FIFO overflown,
> > so the problem might be unnoticeable until we enable the EEPROM-read or Rx-only
> > modes of the DW APB SSI controller. Anyway I am pretty much sure all the systems
> > have the internal bus much faster than the external SPI bus.
> > 
> > Getting back to the formulae. I was thinking of how to make it better and here
> > is what we can do. We can't predict neither the DMA controller performance,
> > nor the performance of its driver. In this case we have no choice but to add
> > some assumption to clarify the task. Let's assume that the reason why Rx FIFO is
> > non-empty is that even though we are at the DMA completion callback, but the
> > DMA controller is still fetching data in background (any other reason might be
> > related with a bug, so we'll detect it here anyway). In this case we need to
> > give it a time to finish its work. As far as I can see the DW_apb_ssi interface
> > doesn't use PREADY APB signal, which means the IO access cycle will take 4
> > reference clock periods for each read and write accesses. Thus taking all of
> > these into account we can create the next formulae to measure the time needed to
> > read all the data from the Rx FIFO:
> > 
> > -	ns = (NSEC_PER_SEC / spi_get_clk(dws)) * nents * dws->n_bytes *
> > -	     BITS_PER_BYTE;
> > +	ns = (NSEC_PER_SEC / dws->max_freq) * nents * 4;
> > 
> > By doing several busy-wait loop iteration we'll cover the DMA controller and
> > it's driver possible latency. 
> > 
> > Feng, does it now makes sense for you now? If so, I'll replace the delay
> > calculation formulae in the patch.
> 
> Frankly I don't have a good idea, if it really happens which means
> something is abnormal, explicitly waiting for some micro-seconds may
> also be acceptable?

Well, If we can estimate the real delay, then it will be more preferred solution.
Hard-coding a single number is only an option if there isn't any other way.

> 
> > > 
> > > Also for the
> > > 
> > > +	while (dw_spi_dma_rx_busy(dws) && retry--)
> > > +		ndelay(ns);
> > > +
> > > 
> > > the rx busy bit is cleared after this rx/tx checking, and it should
> > > be always true at this point. Am I mis-reading the code?
> > 
> > Sorry I don't get your logic here. I am not checking the Rx busy bit here,
> > but the Rx FIFO non-empty bit. Also SR register bits aren't cleared on read,
> > so the status bits are left pending until the reason is cleared. In our case
> > until Rx FIFO gets empty, which will happen eventually either at the point of
> > all data finally being extracted from it or when the controller is disabled
> > by means of the SSIENR register.
> 
> I did misread the code, I thought it is checking the busy bits, sorry
> for that. Though the dw_spi_dma_rx_busy() name is a little confusing,
> as checking the emptiness of RX FIFO is not dma bound.

dw_spi_dma_* is a common prefix for all methods implemented in this module.
As I said having the Rx FIFO non-empty could mean that DMA in fact busy reading
data from the Rx FIFO or a bug or etc. Also the naming correlates with the
dw_spi_dma_tx_busy() method, which also doesn't mean DMA is busy with doing
something, but SPI Tx engine is busy with pushing data out to the SPI bus.

-Sergey

> 
> Thanks,
> Feng

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2020-05-22 12:25 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-21  1:21 [PATCH v3 00/16] spi: dw: Add generic DW DMA controller support Serge Semin
2020-05-21  1:21 ` [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA Serge Semin
2020-05-21  3:09   ` Feng Tang
2020-05-21 11:47     ` Serge Semin
2020-05-21 14:55       ` Feng Tang
2020-05-21 15:33         ` Serge Semin
2020-05-22  7:58           ` Feng Tang
2020-05-22 11:32             ` Serge Semin
2020-05-22 12:03               ` Feng Tang
2020-05-22 12:25                 ` Serge Semin
2020-05-21 23:33   ` Serge Semin
2020-05-21  1:21 ` [PATCH v3 02/16] spi: dw: Enable interrupts in accordance with DMA xfer mode Serge Semin
2020-05-21  1:21 ` [PATCH v3 03/16] spi: dw: Discard static DW DMA slave structures Serge Semin
2020-05-21  9:57   ` Andy Shevchenko
     [not found]     ` <20200521121228.aqplh6eftylnys3p@mobilestation>
2020-05-21 12:49       ` Andy Shevchenko
2020-05-21 15:51       ` Mark Brown
2020-05-21 15:56         ` Andy Shevchenko
2020-05-21 15:58         ` Serge Semin
2020-05-21 16:02           ` Andy Shevchenko
2020-05-21 16:39             ` Mark Brown
2020-05-21 16:44               ` Andy Shevchenko
2020-05-21 17:29               ` Serge Semin
2020-05-21  1:21 ` [PATCH v3 06/16] spi: dw: Parameterize the DMA Rx/Tx burst length Serge Semin
2020-05-21 10:23   ` Andy Shevchenko
2020-05-21  1:21 ` [PATCH v3 07/16] spi: dw: Use DMA max burst to set the request thresholds Serge Semin
2020-05-21 10:49   ` Andy Shevchenko
2020-05-21  1:22 ` [PATCH v3 11/16] spi: dw: Remove DW DMA code dependency from DW_DMAC_PCI Serge Semin
2020-05-21  1:22 ` [PATCH v3 12/16] spi: dw: Add DW SPI DMA/PCI/MMIO dependency on the DW SPI core Serge Semin
2020-05-21  1:22 ` [PATCH v3 14/16] spi: dw: Add DMA support to the DW SPI MMIO driver Serge Semin
2020-05-21  1:22 ` [PATCH v3 16/16] dt-bindings: spi: Convert DW SPI binding to DT schema Serge Semin
     [not found] ` <20200521012206.14472-10-Sergey.Semin@baikalelectronics.ru>
2020-05-21 10:47   ` [PATCH v3 09/16] spi: dw: Add core suffix to the DW APB SSI core source file Andy Shevchenko
2020-05-21 17:46 ` [PATCH v3 00/16] spi: dw: Add generic DW DMA controller support Andy Shevchenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).