[PATCH 0/5] dmaengine: dw: Introduce non-mem peripherals optimizations

[PATCH 0/5] dmaengine: dw: Introduce non-mem peripherals optimizations

[Serge Semin]

After a lot of tests and thorough DW DMAC databook studying we've
discovered that the driver can be optimized especially when it comes to
working with non-memory peripherals.

First of all we've found out that since each DW DMAC channel can
be synthesized with different parameters, then even when two of them
are configured to perform the same DMA transactions they may execute them
with different performance. Since some DMA client devices might be
sensitive to such important parameter as performance, then it is a good
idea to let them request only suitable DMA channels. In this patchset we
introduce a functionality, which makes it possible by passing the DMA
channels mask either over the "dmas" DT property or in the dw_dma_slave
platform data descriptor.

Secondly FIFO-mode of the "FIFO readiness" criterion is more suitable for
the pure memory DMA transfers, since it minimizes the system bus
utilization, but causes some performance drop. When it comes to working with
non-memory peripherals the DMA engine performance comes to the first
place. Since normally DMA client devices keep data in internal FIFOs, any
latency at some critical moment may cause a FIFO being overflown and
consequently losing data. So in order to minimize a chance of the DW DMAC
internal FIFO being a bottle neck during the DMA transfers to and from
non-memory peripherals we propose not to use FIFO-mode for them.

Thirdly it has been discovered that using a DMA transaction length is
redundant when calculating the destination transfer width for the
dev-to-mem DMA communications. That shall increase performance of the DMA
transfers with unaligned data length.

Finally there is a small optimization in the burst length setting. In
particular we've found out, that according to the DW DMAC databoot it's
pointless to set one for the memory peripherals since they don't have
handshaking interface connected to the DMA controller. So we suggest to
just ignore the burst length config when it comes to setting the memory
peripherals up.

Note this patchset is supposed to be applied on top of the series:
Link: https://lore.kernel.org/dmaengine/20200723005848.31907-1-Sergey.Semin@baikalelectronics.ru

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: dmaengine@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

Serge Semin (5):
  dt-bindings: dma: dw: Add optional DMA-channels mask cell support
  dmaengine: dw: Activate FIFO-mode for memory peripherals only
  dmaengine: dw: Discard dlen from the dev-to-mem xfer width calculation
  dmaengine: dw: Ignore burst setting for memory peripherals
  dmaengine: dw: Add DMA-channels mask cell support

 .../devicetree/bindings/dma/snps,dma-spear1340.yaml        | 7 +++++--
 drivers/dma/dw/core.c                                      | 6 +++++-
 drivers/dma/dw/dw.c                                        | 7 +++----
 drivers/dma/dw/idma32.c                                    | 5 ++---
 drivers/dma/dw/of.c                                        | 7 +++++--
 include/linux/platform_data/dma-dw.h                       | 3 +++
 6 files changed, 23 insertions(+), 12 deletions(-)

-- 
2.27.0

[PATCH 1/5] dt-bindings: dma: dw: Add optional DMA-channels mask cell support

[Serge Semin]

Each DW DMA controller channel can be synthesized with different
parameters like maximum burst-length, multi-block support, maximum data
width, etc. Most of these parameters determine the DW DMAC channels
performance in its own aspect. On the other hand these parameters can
be implicitly responsible for the channels performance degradation
(for instance multi-block support is a very useful feature, but having
it disabled during the DW DMAC synthesize will provide a more optimized
core). Since DMA slave devices may have critical dependency on the DMA
engine performance, let's provide a way for the slave devices to have
the DMA-channels allocated from a pool of the channels, which according
to the system engineer fulfill their performance requirements.

The pool is determined by a mask optionally specified in the fifth
DMA-cell of the DMA DT-property. If the fifth cell is omitted from the
phandle arguments or the mask is zero, then the allocation will be
performed from a set of all channels provided by the DMA controller.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 .../devicetree/bindings/dma/snps,dma-spear1340.yaml        | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml b/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
index 20870f5c14dd..ef1d6879c158 100644
--- a/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
+++ b/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
@@ -18,12 +18,15 @@ properties:
     const: snps,dma-spear1340
 
   "#dma-cells":
-    const: 3
+    minimum: 3
+    maximum: 4
     description: |
       First cell is a phandle pointing to the DMA controller. Second one is
       the DMA request line number. Third cell is the memory master identifier
       for transfers on dynamically allocated channel. Fourth cell is the
-      peripheral master identifier for transfers on an allocated channel.
+      peripheral master identifier for transfers on an allocated channel. Fifth
+      cell is an optional mask of the DMA channels permitted to be allocated
+      for the corresponding client device.
 
   reg:
     maxItems: 1
-- 
2.27.0

[PATCH 2/5] dmaengine: dw: Activate FIFO-mode for memory peripherals only

[Serge Semin]

CFGx.FIFO_MODE field controls a DMA-controller "FIFO readiness" criterion.
In other words it determines when to start pushing data out of a DW
DMAC channel FIFO to a destination peripheral or from a source
peripheral to the DW DMAC channel FIFO. Currently FIFO-mode is set to one
for all DW DMAC channels. It means they are tuned to flush data out of
FIFO (to a memory peripheral or by accepting the burst transaction
requests) when FIFO is at least half-full (except at the end of the block
transfer, when FIFO-flush mode is activated) and are configured to get
data to the FIFO when it's at least half-empty.

Such configuration is a good choice when there is no slave device involved
in the DMA transfers. In that case the number of bursts per block is less
than when CFGx.FIFO_MODE = 0 and, hence, the bus utilization will improve.
But the latency of DMA transfers may increase when CFGx.FIFO_MODE = 1,
since DW DMAC will wait for the channel FIFO contents to be either
half-full or half-empty depending on having the destination or the source
transfers. Such latencies might be dangerous in case if the DMA transfers
are expected to be performed from/to a slave device. Since normally
peripheral devices keep data in internal FIFOs, any latency at some
critical moment may cause one being overflown and consequently losing
data. This especially concerns a case when either a peripheral device is
relatively fast or the DW DMAC engine is relatively slow with respect to
the incoming data pace.

In order to solve problems, which might be caused by the latencies
described above, let's enable the FIFO half-full/half-empty "FIFO
readiness" criterion only for DMA transfers with no slave device involved.
Thanks to the commit ???????????? ("dmaengine: dw: Initialize channel
before each transfer") we can freely do that in the generic
dw_dma_initialize_chan() method.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

---

Note the DMA-engine repository git.infradead.org/users/vkoul/slave-dma.git
isn't accessible. So I couldn't find out the Andy' commit hash to use it in
the log.
---
 drivers/dma/dw/dw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/dw/dw.c b/drivers/dma/dw/dw.c
index 7a085b3c1854..d9810980920a 100644
--- a/drivers/dma/dw/dw.c
+++ b/drivers/dma/dw/dw.c
@@ -14,7 +14,7 @@
 static void dw_dma_initialize_chan(struct dw_dma_chan *dwc)
 {
 	struct dw_dma *dw = to_dw_dma(dwc->chan.device);
-	u32 cfghi = DWC_CFGH_FIFO_MODE;
+	u32 cfghi = is_slave_direction(dwc->direction) ? 0 : DWC_CFGH_FIFO_MODE;
 	u32 cfglo = DWC_CFGL_CH_PRIOR(dwc->priority);
 	bool hs_polarity = dwc->dws.hs_polarity;
 
-- 
2.27.0

[PATCH 3/5] dmaengine: dw: Discard dlen from the dev-to-mem xfer width calculation

[Serge Semin]

Indeed in case of the DMA_DEV_TO_MEM DMA transfers it's enough to take the
destination memory address and the destination master data width into
account to calculate the CTLx.DST_TR_WIDTH setting of the memory
peripheral. According to the DW DMAC IP-core Databook (page 66, Example 5)
at the and of a DMA transfer when the DMA-channel internal FIFO is left
with data less than for a single destination burst transaction, the
destination peripheral will enter the Single Transaction Region where the
DW DMA controller can complete a block transfer to the destination using
single transactions (non-burst transaction of CTLx.DST_TR_WIDTH bytes). If
there is no enough data in the DMA-channel internal FIFO for even a single
non-burst transaction of CTLx.DST_TR_WIDTH bytes, then the channel enters
"FIFO flush mode". That mode is activated to empty the FIFO and flush the
leftovers out to the memory peripheral. The flushing procedure is simple.
The data is sent to the memory by means of a set of single transaction of
CTLx.SRC_TR_WIDTH bytes. To sum up it's redundant to use the LLPs length
to find out the CTLx.DST_TR_WIDTH parameter value, since each DMA transfer
will be completed with the CTLx.SRC_TR_WIDTH bytes transaction if it is
required.

In this commit we remove the LLP entry length from the statement which
calculates the memory peripheral DMA transaction width since it's
redundant due to the feature described above. By doing so we'll improve
the memory bus utilization and speed up the DMA-channel performance for
DMA_DEV_TO_MEM DMA-transfers.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/dma/dw/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index 4700f2e87a62..3da0aea9fe25 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -723,7 +723,7 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 			lli_write(desc, sar, reg);
 			lli_write(desc, dar, mem);
 			lli_write(desc, ctlhi, ctlhi);
-			mem_width = __ffs(data_width | mem | dlen);
+			mem_width = __ffs(data_width | mem);
 			lli_write(desc, ctllo, ctllo | DWC_CTLL_DST_WIDTH(mem_width));
 			desc->len = dlen;
 
-- 
2.27.0

[PATCH 4/5] dmaengine: dw: Ignore burst setting for memory peripherals

[Serge Semin]

According to the DW DMA controller Databook (page 40 "3.5 Memory
Peripherals") memory peripherals don't have handshaking interface
connected to the controller, therefore they can never be a flow
controller. Since the CTLx.SRC_MSIZE and CTLx.DEST_MSIZE are
properties valid only for peripherals with a handshaking
interface, we can freely zero these fields out if the memory peripheral
is selected to be the source or the destination of the DMA transfers.

Note according to the databook, length of burst transfers to memory is
always equal to the number of data items available in a channel FIFO or
data items required to complete the block transfer, whichever is smaller;
length of burst transfers from memory is always equal to the space
available in a channel FIFO or number of data items required to complete
the block transfer, whichever is smaller.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/dma/dw/dw.c     | 5 ++---
 drivers/dma/dw/idma32.c | 5 ++---
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/dw/dw.c b/drivers/dma/dw/dw.c
index d9810980920a..a4862263ff14 100644
--- a/drivers/dma/dw/dw.c
+++ b/drivers/dma/dw/dw.c
@@ -67,9 +67,8 @@ static size_t dw_dma_block2bytes(struct dw_dma_chan *dwc, u32 block, u32 width)
 static u32 dw_dma_prepare_ctllo(struct dw_dma_chan *dwc)
 {
 	struct dma_slave_config	*sconfig = &dwc->dma_sconfig;
-	bool is_slave = is_slave_direction(dwc->direction);
-	u8 smsize = is_slave ? sconfig->src_maxburst : DW_DMA_MSIZE_16;
-	u8 dmsize = is_slave ? sconfig->dst_maxburst : DW_DMA_MSIZE_16;
+	u8 smsize = (dwc->direction == DMA_DEV_TO_MEM) ? sconfig->src_maxburst : 0;
+	u8 dmsize = (dwc->direction == DMA_MEM_TO_DEV) ? sconfig->dst_maxburst : 0;
 	u8 p_master = dwc->dws.p_master;
 	u8 m_master = dwc->dws.m_master;
 	u8 dms = (dwc->direction == DMA_MEM_TO_DEV) ? p_master : m_master;
diff --git a/drivers/dma/dw/idma32.c b/drivers/dma/dw/idma32.c
index f00657308811..3ce44de25d33 100644
--- a/drivers/dma/dw/idma32.c
+++ b/drivers/dma/dw/idma32.c
@@ -73,9 +73,8 @@ static size_t idma32_block2bytes(struct dw_dma_chan *dwc, u32 block, u32 width)
 static u32 idma32_prepare_ctllo(struct dw_dma_chan *dwc)
 {
 	struct dma_slave_config	*sconfig = &dwc->dma_sconfig;
-	bool is_slave = is_slave_direction(dwc->direction);
-	u8 smsize = is_slave ? sconfig->src_maxburst : IDMA32_MSIZE_8;
-	u8 dmsize = is_slave ? sconfig->dst_maxburst : IDMA32_MSIZE_8;
+	u8 smsize = (dwc->direction == DMA_DEV_TO_MEM) ? sconfig->src_maxburst : 0;
+	u8 dmsize = (dwc->direction == DMA_MEM_TO_DEV) ? sconfig->dst_maxburst : 0;
 
 	return DWC_CTLL_LLP_D_EN | DWC_CTLL_LLP_S_EN |
 	       DWC_CTLL_DST_MSIZE(dmsize) | DWC_CTLL_SRC_MSIZE(smsize);
-- 
2.27.0

[PATCH 5/5] dmaengine: dw: Add DMA-channels mask cell support

[Serge Semin]

DW DMA IP-core provides a way to synthesize the DMA controller with
channels having different parameters like maximum burst-length,
multi-block support, maximum data width, etc. Those parameters both
explicitly and implicitly affect the channels performance. Since DMA slave
devices might be very demanding to the DMA performance, let's provide a
functionality for the slaves to be assigned with DW DMA channels, which
performance according to the platform engineer fulfill their requirements.
After this patch is applied it can be done by passing the mask of suitable
DMA-channels either directly in the dw_dma_slave structure instance or as
a fifth cell of the DMA DT-property. If mask is zero or not provided, then
there is no limitation on the channels allocation.

For instance Baikal-T1 SoC is equipped with a DW DMAC engine, which first
two channels are synthesized with max burst length of 16, while the rest
of the channels have been created with max-burst-len=4. It would seem that
the first two channels must be faster than the others and should be more
preferable for the time-critical DMA slave devices. In practice it turned
out that the situation is quite the opposite. The channels with
max-burst-len=4 demonstrated a better performance than the channels with
max-burst-len=16 even when they both had been initialized with the same
settings. The performance drop of the first two DMA-channels made them
unsuitable for the DW APB SSI slave device. No matter what settings they
are configured with, full-duplex SPI transfers occasionally experience the
Rx FIFO overflow. It means that the DMA-engine doesn't keep up with
incoming data pace even though the SPI-bus is enabled with speed of 25MHz
while the DW DMA controller is clocked with 50MHz signal. There is no such
problem has been noticed for the channels synthesized with
max-burst-len=4.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/dma/dw/core.c                | 4 ++++
 drivers/dma/dw/of.c                  | 7 +++++--
 include/linux/platform_data/dma-dw.h | 3 +++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index 3da0aea9fe25..5f7b9badb965 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -772,6 +772,10 @@ bool dw_dma_filter(struct dma_chan *chan, void *param)
 	if (dws->dma_dev != chan->device->dev)
 		return false;
 
+	/* permit channels in accordance with the channels mask */
+	if (dws->channels && !(dws->channels & dwc->mask))
+		return false;
+
 	/* We have to copy data since dws can be temporary storage */
 	memcpy(&dwc->dws, dws, sizeof(struct dw_dma_slave));
 
diff --git a/drivers/dma/dw/of.c b/drivers/dma/dw/of.c
index 1474b3817ef4..abdf22b269b5 100644
--- a/drivers/dma/dw/of.c
+++ b/drivers/dma/dw/of.c
@@ -22,18 +22,21 @@ static struct dma_chan *dw_dma_of_xlate(struct of_phandle_args *dma_spec,
 	};
 	dma_cap_mask_t cap;
 
-	if (dma_spec->args_count != 3)
+	if (dma_spec->args_count < 3 || dma_spec->args_count > 4)
 		return NULL;
 
 	slave.src_id = dma_spec->args[0];
 	slave.dst_id = dma_spec->args[0];
 	slave.m_master = dma_spec->args[1];
 	slave.p_master = dma_spec->args[2];
+	if (dma_spec->args_count >= 4)
+		slave.channels = dma_spec->args[3];
 
 	if (WARN_ON(slave.src_id >= DW_DMA_MAX_NR_REQUESTS ||
 		    slave.dst_id >= DW_DMA_MAX_NR_REQUESTS ||
 		    slave.m_master >= dw->pdata->nr_masters ||
-		    slave.p_master >= dw->pdata->nr_masters))
+		    slave.p_master >= dw->pdata->nr_masters ||
+		    fls(slave.channels) > dw->pdata->nr_channels))
 		return NULL;
 
 	dma_cap_zero(cap);
diff --git a/include/linux/platform_data/dma-dw.h b/include/linux/platform_data/dma-dw.h
index 4f681df85c27..3bc48451a70c 100644
--- a/include/linux/platform_data/dma-dw.h
+++ b/include/linux/platform_data/dma-dw.h
@@ -23,6 +23,8 @@
  * @dst_id:	dst request line
  * @m_master:	memory master for transfers on allocated channel
  * @p_master:	peripheral master for transfers on allocated channel
+ * @channels:	mask of the channels permitted for allocation (zero
+ *		value means any)
  * @hs_polarity:set active low polarity of handshake interface
  */
 struct dw_dma_slave {
@@ -31,6 +33,7 @@ struct dw_dma_slave {
 	u8			dst_id;
 	u8			m_master;
 	u8			p_master;
+	u8			channels;
 	bool			hs_polarity;
 };
 
-- 
2.27.0

Re: [PATCH 2/5] dmaengine: dw: Activate FIFO-mode for memory peripherals only

[Andy Shevchenko]

On Thu, Jul 30, 2020 at 06:45:42PM +0300, Serge Semin wrote:
> CFGx.FIFO_MODE field controls a DMA-controller "FIFO readiness" criterion.
> In other words it determines when to start pushing data out of a DW
> DMAC channel FIFO to a destination peripheral or from a source
> peripheral to the DW DMAC channel FIFO. Currently FIFO-mode is set to one
> for all DW DMAC channels. It means they are tuned to flush data out of
> FIFO (to a memory peripheral or by accepting the burst transaction
> requests) when FIFO is at least half-full (except at the end of the block
> transfer, when FIFO-flush mode is activated) and are configured to get
> data to the FIFO when it's at least half-empty.
> 
> Such configuration is a good choice when there is no slave device involved
> in the DMA transfers. In that case the number of bursts per block is less
> than when CFGx.FIFO_MODE = 0 and, hence, the bus utilization will improve.
> But the latency of DMA transfers may increase when CFGx.FIFO_MODE = 1,
> since DW DMAC will wait for the channel FIFO contents to be either
> half-full or half-empty depending on having the destination or the source
> transfers. Such latencies might be dangerous in case if the DMA transfers
> are expected to be performed from/to a slave device. Since normally
> peripheral devices keep data in internal FIFOs, any latency at some
> critical moment may cause one being overflown and consequently losing
> data. This especially concerns a case when either a peripheral device is
> relatively fast or the DW DMAC engine is relatively slow with respect to
> the incoming data pace.
> 
> In order to solve problems, which might be caused by the latencies
> described above, let's enable the FIFO half-full/half-empty "FIFO
> readiness" criterion only for DMA transfers with no slave device involved.

> Thanks to the commit ???????????? ("dmaengine: dw: Initialize channel

See below.

> before each transfer") we can freely do that in the generic
> dw_dma_initialize_chan() method.

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

> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> ---
> 
> Note the DMA-engine repository git.infradead.org/users/vkoul/slave-dma.git
> isn't accessible. So I couldn't find out the Andy' commit hash to use it in
> the log.

It's dmaengine.git on git.kernel.org.

> ---
>  drivers/dma/dw/dw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/dw/dw.c b/drivers/dma/dw/dw.c
> index 7a085b3c1854..d9810980920a 100644
> --- a/drivers/dma/dw/dw.c
> +++ b/drivers/dma/dw/dw.c
> @@ -14,7 +14,7 @@
>  static void dw_dma_initialize_chan(struct dw_dma_chan *dwc)
>  {
>  	struct dw_dma *dw = to_dw_dma(dwc->chan.device);
> -	u32 cfghi = DWC_CFGH_FIFO_MODE;
> +	u32 cfghi = is_slave_direction(dwc->direction) ? 0 : DWC_CFGH_FIFO_MODE;
>  	u32 cfglo = DWC_CFGL_CH_PRIOR(dwc->priority);
>  	bool hs_polarity = dwc->dws.hs_polarity;
>  
> -- 
> 2.27.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/5] dmaengine: dw: Discard dlen from the dev-to-mem xfer width calculation

[Andy Shevchenko]

On Thu, Jul 30, 2020 at 06:45:43PM +0300, Serge Semin wrote:
> Indeed in case of the DMA_DEV_TO_MEM DMA transfers it's enough to take the
> destination memory address and the destination master data width into
> account to calculate the CTLx.DST_TR_WIDTH setting of the memory

> peripheral. According to the DW DMAC IP-core Databook (page 66, Example 5)

Always put a version of the Databook document. I have several and they may differ.

> at the and of a DMA transfer when the DMA-channel internal FIFO is left
> with data less than for a single destination burst transaction, the
> destination peripheral will enter the Single Transaction Region where the
> DW DMA controller can complete a block transfer to the destination using
> single transactions (non-burst transaction of CTLx.DST_TR_WIDTH bytes). If
> there is no enough data in the DMA-channel internal FIFO for even a single
> non-burst transaction of CTLx.DST_TR_WIDTH bytes, then the channel enters
> "FIFO flush mode". That mode is activated to empty the FIFO and flush the
> leftovers out to the memory peripheral. The flushing procedure is simple.
> The data is sent to the memory by means of a set of single transaction of
> CTLx.SRC_TR_WIDTH bytes. To sum up it's redundant to use the LLPs length
> to find out the CTLx.DST_TR_WIDTH parameter value, since each DMA transfer
> will be completed with the CTLx.SRC_TR_WIDTH bytes transaction if it is
> required.

> In this commit we remove the LLP entry length from the statement which

"In this commit" should be removed, see Submitting Patches ("This patch").

> calculates the memory peripheral DMA transaction width since it's
> redundant due to the feature described above. By doing so we'll improve
> the memory bus utilization and speed up the DMA-channel performance for
> DMA_DEV_TO_MEM DMA-transfers.

Okay, I have no objections.
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> ---
>  drivers/dma/dw/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
> index 4700f2e87a62..3da0aea9fe25 100644
> --- a/drivers/dma/dw/core.c
> +++ b/drivers/dma/dw/core.c
> @@ -723,7 +723,7 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>  			lli_write(desc, sar, reg);
>  			lli_write(desc, dar, mem);
>  			lli_write(desc, ctlhi, ctlhi);
> -			mem_width = __ffs(data_width | mem | dlen);
> +			mem_width = __ffs(data_width | mem);
>  			lli_write(desc, ctllo, ctllo | DWC_CTLL_DST_WIDTH(mem_width));
>  			desc->len = dlen;
>  
> -- 
> 2.27.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/5] dmaengine: dw: Ignore burst setting for memory peripherals

[Andy Shevchenko]

On Thu, Jul 30, 2020 at 06:45:44PM +0300, Serge Semin wrote:
> According to the DW DMA controller Databook (page 40 "3.5 Memory

Which version of it?

> Peripherals") memory peripherals don't have handshaking interface
> connected to the controller, therefore they can never be a flow
> controller. Since the CTLx.SRC_MSIZE and CTLx.DEST_MSIZE are
> properties valid only for peripherals with a handshaking
> interface, we can freely zero these fields out if the memory peripheral
> is selected to be the source or the destination of the DMA transfers.
> 
> Note according to the databook, length of burst transfers to memory is
> always equal to the number of data items available in a channel FIFO or
> data items required to complete the block transfer, whichever is smaller;
> length of burst transfers from memory is always equal to the space
> available in a channel FIFO or number of data items required to complete
> the block transfer, whichever is smaller.

But does it really matter if you program there something or not?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/5] dmaengine: dw: Activate FIFO-mode for memory peripherals only

[Serge Semin]

On Thu, Jul 30, 2020 at 07:24:28PM +0300, Andy Shevchenko wrote:
> On Thu, Jul 30, 2020 at 06:45:42PM +0300, Serge Semin wrote:
> > CFGx.FIFO_MODE field controls a DMA-controller "FIFO readiness" criterion.
> > In other words it determines when to start pushing data out of a DW
> > DMAC channel FIFO to a destination peripheral or from a source
> > peripheral to the DW DMAC channel FIFO. Currently FIFO-mode is set to one
> > for all DW DMAC channels. It means they are tuned to flush data out of
> > FIFO (to a memory peripheral or by accepting the burst transaction
> > requests) when FIFO is at least half-full (except at the end of the block
> > transfer, when FIFO-flush mode is activated) and are configured to get
> > data to the FIFO when it's at least half-empty.
> > 
> > Such configuration is a good choice when there is no slave device involved
> > in the DMA transfers. In that case the number of bursts per block is less
> > than when CFGx.FIFO_MODE = 0 and, hence, the bus utilization will improve.
> > But the latency of DMA transfers may increase when CFGx.FIFO_MODE = 1,
> > since DW DMAC will wait for the channel FIFO contents to be either
> > half-full or half-empty depending on having the destination or the source
> > transfers. Such latencies might be dangerous in case if the DMA transfers
> > are expected to be performed from/to a slave device. Since normally
> > peripheral devices keep data in internal FIFOs, any latency at some
> > critical moment may cause one being overflown and consequently losing
> > data. This especially concerns a case when either a peripheral device is
> > relatively fast or the DW DMAC engine is relatively slow with respect to
> > the incoming data pace.
> > 
> > In order to solve problems, which might be caused by the latencies
> > described above, let's enable the FIFO half-full/half-empty "FIFO
> > readiness" criterion only for DMA transfers with no slave device involved.
> 
> > Thanks to the commit ???????????? ("dmaengine: dw: Initialize channel
> 
> See below.
> 
> > before each transfer") we can freely do that in the generic
> > dw_dma_initialize_chan() method.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Thanks!
> 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > 
> > ---
> > 
> > Note the DMA-engine repository git.infradead.org/users/vkoul/slave-dma.git
> > isn't accessible. So I couldn't find out the Andy' commit hash to use it in
> > the log.
> 

> It's dmaengine.git on git.kernel.org.

Ah, thanks! I've just found out that the repo address has been changed. But I've
also scanned the "next" branch of the repo:
https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git

Your commit isn't there. Am I missing something?

-Sergey

> 
> > ---
> >  drivers/dma/dw/dw.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/dma/dw/dw.c b/drivers/dma/dw/dw.c
> > index 7a085b3c1854..d9810980920a 100644
> > --- a/drivers/dma/dw/dw.c
> > +++ b/drivers/dma/dw/dw.c
> > @@ -14,7 +14,7 @@
> >  static void dw_dma_initialize_chan(struct dw_dma_chan *dwc)
> >  {
> >  	struct dw_dma *dw = to_dw_dma(dwc->chan.device);
> > -	u32 cfghi = DWC_CFGH_FIFO_MODE;
> > +	u32 cfghi = is_slave_direction(dwc->direction) ? 0 : DWC_CFGH_FIFO_MODE;
> >  	u32 cfglo = DWC_CFGL_CH_PRIOR(dwc->priority);
> >  	bool hs_polarity = dwc->dws.hs_polarity;
> >  
> > -- 
> > 2.27.0
> > 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH 4/5] dmaengine: dw: Ignore burst setting for memory peripherals

[Serge Semin]

On Thu, Jul 30, 2020 at 07:31:22PM +0300, Andy Shevchenko wrote:
> On Thu, Jul 30, 2020 at 06:45:44PM +0300, Serge Semin wrote:
> > According to the DW DMA controller Databook (page 40 "3.5 Memory
> 

> Which version of it?

2.18b

> 
> > Peripherals") memory peripherals don't have handshaking interface
> > connected to the controller, therefore they can never be a flow
> > controller. Since the CTLx.SRC_MSIZE and CTLx.DEST_MSIZE are
> > properties valid only for peripherals with a handshaking
> > interface, we can freely zero these fields out if the memory peripheral
> > is selected to be the source or the destination of the DMA transfers.
> > 
> > Note according to the databook, length of burst transfers to memory is
> > always equal to the number of data items available in a channel FIFO or
> > data items required to complete the block transfer, whichever is smaller;
> > length of burst transfers from memory is always equal to the space
> > available in a channel FIFO or number of data items required to complete
> > the block transfer, whichever is smaller.
> 

> But does it really matter if you program there something or not?

For memory peripherals it doesn't. But it's better to remove the redundant
settings for consistency with the doc and to get rid of the unneeded
stack-variable declaration.

-Sergey

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH 5/5] dmaengine: dw: Add DMA-channels mask cell support

[Andy Shevchenko]

On Thu, Jul 30, 2020 at 06:45:45PM +0300, Serge Semin wrote:
> DW DMA IP-core provides a way to synthesize the DMA controller with
> channels having different parameters like maximum burst-length,
> multi-block support, maximum data width, etc. Those parameters both
> explicitly and implicitly affect the channels performance. Since DMA slave
> devices might be very demanding to the DMA performance, let's provide a
> functionality for the slaves to be assigned with DW DMA channels, which
> performance according to the platform engineer fulfill their requirements.
> After this patch is applied it can be done by passing the mask of suitable
> DMA-channels either directly in the dw_dma_slave structure instance or as
> a fifth cell of the DMA DT-property. If mask is zero or not provided, then
> there is no limitation on the channels allocation.
> 
> For instance Baikal-T1 SoC is equipped with a DW DMAC engine, which first
> two channels are synthesized with max burst length of 16, while the rest
> of the channels have been created with max-burst-len=4. It would seem that
> the first two channels must be faster than the others and should be more
> preferable for the time-critical DMA slave devices. In practice it turned
> out that the situation is quite the opposite. The channels with
> max-burst-len=4 demonstrated a better performance than the channels with
> max-burst-len=16 even when they both had been initialized with the same
> settings. The performance drop of the first two DMA-channels made them
> unsuitable for the DW APB SSI slave device. No matter what settings they
> are configured with, full-duplex SPI transfers occasionally experience the
> Rx FIFO overflow. It means that the DMA-engine doesn't keep up with
> incoming data pace even though the SPI-bus is enabled with speed of 25MHz
> while the DW DMA controller is clocked with 50MHz signal. There is no such
> problem has been noticed for the channels synthesized with
> max-burst-len=4.

...

> +	if (dws->channels && !(dws->channels & dwc->mask))

You can drop the first check if...

> +		return false;

...

> +	if (dma_spec->args_count >= 4)
> +		slave.channels = dma_spec->args[3];

...you apply sane default here or somewhere else.

...

> +		    fls(slave.channels) > dw->pdata->nr_channels))

Does it really make sense?

I think it can also be simplified to faster op, i.e.
	BIT(nr_channels) < slave.channels
(but check for off-by-one errors)

...

> + * @channels:	mask of the channels permitted for allocation (zero
> + *		value means any)

Perhaps on one line?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/5] dmaengine: dw: Activate FIFO-mode for memory peripherals only

[Andy Shevchenko]

On Thu, Jul 30, 2020 at 07:31:54PM +0300, Serge Semin wrote:
> On Thu, Jul 30, 2020 at 07:24:28PM +0300, Andy Shevchenko wrote:
> > On Thu, Jul 30, 2020 at 06:45:42PM +0300, Serge Semin wrote:

...

> > > Thanks to the commit ???????????? ("dmaengine: dw: Initialize channel

...

> > > Note the DMA-engine repository git.infradead.org/users/vkoul/slave-dma.git
> > > isn't accessible. So I couldn't find out the Andy' commit hash to use it in
> > > the log.
> 
> > It's dmaengine.git on git.kernel.org.
> 
> Ah, thanks! I've just found out that the repo address has been changed. But I've
> also scanned the "next" branch of the repo:
> https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git
> 
> Your commit isn't there. Am I missing something?

It's a fix. It went to upstream branch (don't remember its name by heart in
Vinod's repo).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/5] dmaengine: dw: Add DMA-channels mask cell support

[Serge Semin]

On Thu, Jul 30, 2020 at 07:41:46PM +0300, Andy Shevchenko wrote:
> On Thu, Jul 30, 2020 at 06:45:45PM +0300, Serge Semin wrote:
> > DW DMA IP-core provides a way to synthesize the DMA controller with
> > channels having different parameters like maximum burst-length,
> > multi-block support, maximum data width, etc. Those parameters both
> > explicitly and implicitly affect the channels performance. Since DMA slave
> > devices might be very demanding to the DMA performance, let's provide a
> > functionality for the slaves to be assigned with DW DMA channels, which
> > performance according to the platform engineer fulfill their requirements.
> > After this patch is applied it can be done by passing the mask of suitable
> > DMA-channels either directly in the dw_dma_slave structure instance or as
> > a fifth cell of the DMA DT-property. If mask is zero or not provided, then
> > there is no limitation on the channels allocation.
> > 
> > For instance Baikal-T1 SoC is equipped with a DW DMAC engine, which first
> > two channels are synthesized with max burst length of 16, while the rest
> > of the channels have been created with max-burst-len=4. It would seem that
> > the first two channels must be faster than the others and should be more
> > preferable for the time-critical DMA slave devices. In practice it turned
> > out that the situation is quite the opposite. The channels with
> > max-burst-len=4 demonstrated a better performance than the channels with
> > max-burst-len=16 even when they both had been initialized with the same
> > settings. The performance drop of the first two DMA-channels made them
> > unsuitable for the DW APB SSI slave device. No matter what settings they
> > are configured with, full-duplex SPI transfers occasionally experience the
> > Rx FIFO overflow. It means that the DMA-engine doesn't keep up with
> > incoming data pace even though the SPI-bus is enabled with speed of 25MHz
> > while the DW DMA controller is clocked with 50MHz signal. There is no such
> > problem has been noticed for the channels synthesized with
> > max-burst-len=4.
> 
> ...
> 

> > +	if (dws->channels && !(dws->channels & dwc->mask))
> 
> You can drop the first check if...

See below.

> 
> > +		return false;
> 
> ...
> 
> > +	if (dma_spec->args_count >= 4)
> > +		slave.channels = dma_spec->args[3];
> 
> ...you apply sane default here or somewhere else.

Alas I can't because dw_dma_slave structure is defined all over the kernel
drivers/spi/spi-dw-dma.c
drivers/spi/spi-pxa2xx-pci.c
drivers/tty/serial/8250/8250_lpss.c

These devices aren't always placed on the OF-based platforms. In that case the
corresponding DMA-channels won't be requested by means of the dw_dma_of_xlate()
method. So we have to preserve a default behavior if dws->channels is zero.

> 
> ...
> 
> > +		    fls(slave.channels) > dw->pdata->nr_channels))
> 

> Does it really make sense?

It does to prevent the clients to specify an invalid channels mask, which can't
have bits set higher than the number of channels the engine supports.

> 
> I think it can also be simplified to faster op, i.e.
> 	BIT(nr_channels) < slave.channels
> (but check for off-by-one errors)

Makes sense. Thanks. I'll replace it with the next statement:
slave.channels >= BIT(dw->pdata->nr_channels)

> 
> ...
> 

> > + * @channels:	mask of the channels permitted for allocation (zero
> > + *		value means any)
> 
> Perhaps on one line?

I don't really care. If you insist on that, I'll make it a single line, but it
will be over 80 columns. 85 to be exact.

-Sergey

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH 2/5] dmaengine: dw: Activate FIFO-mode for memory peripherals only

[Serge Semin]

On Thu, Jul 30, 2020 at 07:47:03PM +0300, Andy Shevchenko wrote:
> On Thu, Jul 30, 2020 at 07:31:54PM +0300, Serge Semin wrote:
> > On Thu, Jul 30, 2020 at 07:24:28PM +0300, Andy Shevchenko wrote:
> > > On Thu, Jul 30, 2020 at 06:45:42PM +0300, Serge Semin wrote:
> 
> ...
> 
> > > > Thanks to the commit ???????????? ("dmaengine: dw: Initialize channel
> 
> ...
> 
> > > > Note the DMA-engine repository git.infradead.org/users/vkoul/slave-dma.git
> > > > isn't accessible. So I couldn't find out the Andy' commit hash to use it in
> > > > the log.
> > 
> > > It's dmaengine.git on git.kernel.org.
> > 
> > Ah, thanks! I've just found out that the repo address has been changed. But I've
> > also scanned the "next" branch of the repo:
> > https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git
> > 
> > Your commit isn't there. Am I missing something?
> 

> It's a fix. It went to upstream branch (don't remember its name by heart in
> Vinod's repo).

Right. Found it. Thanks.

-Sergey

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH 2/5] dmaengine: dw: Activate FIFO-mode for memory peripherals only

[Vinod Koul]

On 30-07-20, 20:13, Serge Semin wrote:
> On Thu, Jul 30, 2020 at 07:47:03PM +0300, Andy Shevchenko wrote:
> > On Thu, Jul 30, 2020 at 07:31:54PM +0300, Serge Semin wrote:
> > > On Thu, Jul 30, 2020 at 07:24:28PM +0300, Andy Shevchenko wrote:
> > > > On Thu, Jul 30, 2020 at 06:45:42PM +0300, Serge Semin wrote:
> > 
> > ...
> > 
> > > > > Thanks to the commit ???????????? ("dmaengine: dw: Initialize channel
> > 
> > ...
> > 
> > > > > Note the DMA-engine repository git.infradead.org/users/vkoul/slave-dma.git
> > > > > isn't accessible. So I couldn't find out the Andy' commit hash to use it in
> > > > > the log.

Yeah I moved tree to k.org after disk issue with infradead, change patch
was on dmaengine ML

> > > > It's dmaengine.git on git.kernel.org.
> > > 
> > > Ah, thanks! I've just found out that the repo address has been changed. But I've
> > > also scanned the "next" branch of the repo:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git
> > > 
> > > Your commit isn't there. Am I missing something?
> > 
> 
> > It's a fix. It went to upstream branch (don't remember its name by heart in
> > Vinod's repo).

Yes it is Linus's tree now and in dmaengine you can find in fixes branch

https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git/commit/?h=fixes&id=99ba8b9b0d9780e9937eb1d488d120e9e5c2533d

> 
> Right. Found it. Thanks.
> 
> -Sergey
> 
> > 
> > -- 
> > With Best Regards,
> > Andy Shevchenko
> > 
> > 

-- 
~Vinod

Re: [PATCH 2/5] dmaengine: dw: Activate FIFO-mode for memory peripherals only

[Serge Semin]

On Fri, Jul 31, 2020 at 10:22:54PM +0530, Vinod Koul wrote:
> On 30-07-20, 20:13, Serge Semin wrote:
> > On Thu, Jul 30, 2020 at 07:47:03PM +0300, Andy Shevchenko wrote:
> > > On Thu, Jul 30, 2020 at 07:31:54PM +0300, Serge Semin wrote:
> > > > On Thu, Jul 30, 2020 at 07:24:28PM +0300, Andy Shevchenko wrote:
> > > > > On Thu, Jul 30, 2020 at 06:45:42PM +0300, Serge Semin wrote:
> > > 
> > > ...
> > > 
> > > > > > Thanks to the commit ???????????? ("dmaengine: dw: Initialize channel
> > > 
> > > ...
> > > 
> > > > > > Note the DMA-engine repository git.infradead.org/users/vkoul/slave-dma.git
> > > > > > isn't accessible. So I couldn't find out the Andy' commit hash to use it in
> > > > > > the log.
> 
> Yeah I moved tree to k.org after disk issue with infradead, change patch
> was on dmaengine ML
> 
> > > > > It's dmaengine.git on git.kernel.org.
> > > > 
> > > > Ah, thanks! I've just found out that the repo address has been changed. But I've
> > > > also scanned the "next" branch of the repo:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git
> > > > 
> > > > Your commit isn't there. Am I missing something?
> > > 
> > 
> > > It's a fix. It went to upstream branch (don't remember its name by heart in
> > > Vinod's repo).
> 
> Yes it is Linus's tree now and in dmaengine you can find in fixes branch
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git/commit/?h=fixes&id=99ba8b9b0d9780e9937eb1d488d120e9e5c2533d

Thanks for pointing out to the commit. I'll add the hash to the patch log and
resend the series shortly.

-Sergey

> 
> > 
> > Right. Found it. Thanks.
> > 
> > -Sergey
> > 
> > > 
> > > -- 
> > > With Best Regards,
> > > Andy Shevchenko
> > > 
> > > 
> 
> -- 
> ~Vinod

Re: [PATCH 1/5] dt-bindings: dma: dw: Add optional DMA-channels mask cell support

[Rob Herring]

On Thu, 30 Jul 2020 18:45:41 +0300, Serge Semin wrote:
> Each DW DMA controller channel can be synthesized with different
> parameters like maximum burst-length, multi-block support, maximum data
> width, etc. Most of these parameters determine the DW DMAC channels
> performance in its own aspect. On the other hand these parameters can
> be implicitly responsible for the channels performance degradation
> (for instance multi-block support is a very useful feature, but having
> it disabled during the DW DMAC synthesize will provide a more optimized
> core). Since DMA slave devices may have critical dependency on the DMA
> engine performance, let's provide a way for the slave devices to have
> the DMA-channels allocated from a pool of the channels, which according
> to the system engineer fulfill their performance requirements.
> 
> The pool is determined by a mask optionally specified in the fifth
> DMA-cell of the DMA DT-property. If the fifth cell is omitted from the
> phandle arguments or the mask is zero, then the allocation will be
> performed from a set of all channels provided by the DMA controller.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> ---
>  .../devicetree/bindings/dma/snps,dma-spear1340.yaml        | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>