dmaengine.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RESEND v7 00/11] dmaengine: dw: Take Baikal-T1 SoC DW DMAC peculiarities into account
@ 2020-07-09 22:45 Serge Semin
  2020-07-09 22:45 ` [PATCH v7 01/11] dt-bindings: dma: dw: Convert DW DMAC to DT binding Serge Semin
                   ` (10 more replies)
  0 siblings, 11 replies; 33+ messages in thread
From: Serge Semin @ 2020-07-09 22:45 UTC (permalink / raw)
  To: Vinod Koul, Viresh Kumar
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Maxim Kaurkin,
	Pavel Parkhomenko, Ramil Zaripov, Ekaterina Skachko,
	Vadim Vlasov, Alexey Kolotnikov, Thomas Bogendoerfer,
	Arnd Bergmann, Andy Shevchenko, Rob Herring, linux-mips,
	dmaengine, devicetree, linux-kernel

In the previous patchset I've written the next message:

> Folks, note I've removed the next patches from the series:
> [PATCH v7 04/11] dmaengine: Introduce max SG list entries capability
> [PATCH v7 11/11] dmaengine: dw: Initialize max_sg_nents capability
> It turns out the problem with the asynchronous handling of Tx- and Rx-
> SPI transfers interrupts is more complex than I expected. So in order to
> solve the problem it isn't enough to split the SG list entries submission
> up based on the max_sg_nents capability setting (though the synchronous
> one-by-one SG list entries handling does fix a part of the problem). So
> if and when I get to find a comprehensive solution for it I'll submit a
> new series with fixups. Until then please consider to merge the patchset
> in without those patches.

Those patches are returned back to the series. I've found a solution, which
fixes the problem for our hardware. A new patchset with several fixes for the
DW DMAC driver will be sent shortly after this one is merged in. Note the same
concerns the DW APB SPI driver. So please review and merge in as soon as
possible.

Regarding the patchset. Baikal-T1 SoC has an DW DMAC on-board to provide a
Mem-to-Mem, low-speed peripherals Dev-to-Mem and Mem-to-Dev functionality.
Mostly it's compatible with currently implemented in the kernel DW DMAC
driver, but there are some peculiarities which must be taken into account
in order to have the device fully supported.

First of all traditionally we replaced the legacy plain text-based dt-binding
file with yaml-based one. Secondly Baikal-T1 DW DMA Controller provides eight
channels, which alas have different max burst length configuration.
In particular first two channels may burst up to 128 bits (16 bytes) at a time
while the rest of them just up to 32 bits. We must make sure that the DMA
subsystem doesn't set values exceeding these limitations otherwise the
controller will hang up. In third currently we discovered the problem in using
the DW APB SPI driver together with DW DMAC. The problem happens if there is no
natively implemented multi-block LLP transfers support and the SPI-transfer
length exceeds the max lock size. In this case due to asynchronous handling of
Tx- and Rx- SPI transfers interrupt we might end up with DW APB SSI Rx FIFO
overflow. So if DW APB SSI (or any other DMAC service consumer) intends to use
the DMAC to asynchronously execute the transfers we'd have to at least warn
the user of the possible errors. In forth it's worth to set the DMA device max
segment size with max block size config specific to the DW DMA controller. It
shall help the DMA clients to create size-optimized SG-list items for the
controller. This in turn will cause less dw_desc allocations, less LLP
reinitializations, better DMA device performance.

Finally there is a bug in the algorithm of the nollp flag detection.
In particular even if DW DMAC parameters state the multi-block transfers
support there is still HC_LLP (hardcode LLP) flag, which if set makes expected
by the driver true multi-block LLP functionality unusable. This happens cause'
if HC_LLP flag is set the LLP registers will be hardcoded to zero so the
contiguous multi-block transfers will be only supported. We must take the
flag into account when detecting the LLP support otherwise the driver just
won't work correctly.

This patchset is rebased and tested on the mainline Linux kernel 5.7-rc4:
base-commit: 0e698dfa2822 ("Linux 5.7-rc4")
tag: v5.7-rc4

Changelog v2:
- Rearrange SoBs.
- Move $ref to the root level of the properties. So do do with the
  constraints in the DT binding.
- Replace "additionalProperties: false" with "unevaluatedProperties: false"
  property in the DT binding file.
- Discard default settings defined out of property enum constraint.
- Set default max-burst-len to 256 TR-WIDTH words in the DT binding.
- Discard noLLP and block_size accessors.
- Set max segment size of the DMA device structure with the DW DMA block size
  config.
- Print warning if noLLP flag is set.
- Discard max burst length accessor.
- Add comment about why hardware accelerated LLP list support depends
  on both MBLK_EN and HC_LLP configs setting.
- Use explicit bits state comparison operator in noLLP flag setting.

Link: https://lore.kernel.org/dmaengine/20200508105304.14065-1-Sergey.Semin@baikalelectronics.ru/
Changelog v3:
- Use the block_size found for the very first channel instead of looking for
  the maximum of maximum block sizes.
- Don't define device-specific device_dma_parameters object, since it has
  already been defined by the platform device core.
- Add more details into the property description about what limitations
  snps,max-burst-len defines.
- Move commit fb7e3bbfc830 ("dmaengine: dw: Take HC_LLP flag into account for
  noLLP auto-config") to the head of the series.
- Add a new patch "dmaengine: Introduce min burst length capability" as a
  result of the discussion with Vinod and Andy regarding the burst length
  capability.
- Add a new patch "dmaengine: Introduce max SG list entries capability"
  suggested by Andy.
- Add a new patch "dmaengine: Introduce DMA-device device_caps callback" as
  a result of the discussion with Vinud and Andy in the framework of DW DMA
  burst and LLP capabilities.
- Add a new patch "dmaengine: dw: Add dummy device_caps callback" as a
  preparation commit before setting the max_burst and max_sg_nents
  DW DMA capabilities.
- Override the slave channel max_burst capability instead of calculating
  the minimum value of max burst lengths and setting the DMA-device
  generic capability.
- Add a new patch "dmaengine: dw: Initialize max_sg_nents with nollp flag".
  This is required to fix the DW APB SSI issue of the Tx and Rx DMA
  channels de-synchronization.

Link: https://lore.kernel.org/dmaengine/20200526225022.20405-1-Sergey.Semin@baikalelectronics.ru/
Changelog v4:
- Use explicit if-else statement when assigning the max_sg_nents field.
- Clamp the dst and src burst lengths in the generic dwc_config() method
  instead of doing that in the encode_maxburst() callback.
- Define max_burst with u32 type in struct dw_dma_platform_data.
- Perform of_property_read_u32_array() with the platform data
  max_burst member passed directly.
- Add a new patch "dmaengine: dw: Initialize min_burst capability",
  which initializes the min_burst capability with 1.
- Fix of->if typo. It should be definitely "of" in the max_sg_list
  capability description.

Link: https://lore.kernel.org/dmaengine/20200528222401.26941-1-Sergey.Semin@baikalelectronics.ru
Changelog v5:
- Introduce macro with extreme min and max burst lengths supported by the
  DW DMA controller. Define them in the patch with default min and max burst
  length iintializations.
- Initialize max_burst length capability with extreme burst length supported
  by the DW DMAC IP-core.
- Move DW_DMA_MAX_BURST macro definition to the patch "dmaengine: dw:
  Initialize min and max burst DMA device capability".
- Add in-line comment at the point of the device_caps callback invocation.
- Add doc-comment for the device_caps member of struct dma_device

Link: https://lore.kernel.org/dmaengine/20200529144054.4251-1-Sergey.Semin@baikalelectronics.ru
Changelog v6:
- Discard patches:
  [PATCH v5 04/11] dmaengine: Introduce max SG list entries capability
  [PATCH v5 11/11] dmaengine: dw: Initialize max_sg_nents capability
  since for now it's not enough to have them merged in to provide the SPI
  Rx FIFO overrun error fix. They will be included into another series.

Link: https://lore.kernel.org/dmaengine/20200617234028.25808-1-Sergey.Semin@baikalelectronics.ru
Changelog v7:
- Get the patches:
  [PATCH v5 04/11] dmaengine: Introduce max SG list entries capability
  [PATCH v5 11/11] dmaengine: dw: Initialize max_sg_nents capability
  back.

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: Ramil Zaripov <Ramil.Zaripov@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: 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: dmaengine@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

Serge Semin (11):
  dt-bindings: dma: dw: Convert DW DMAC to DT binding
  dt-bindings: dma: dw: Add max burst transaction length property
  dmaengine: Introduce min burst length capability
  dmaengine: Introduce max SG list entries capability
  dmaengine: Introduce DMA-device device_caps callback
  dmaengine: dw: Take HC_LLP flag into account for noLLP auto-config
  dmaengine: dw: Set DMA device max segment size parameter
  dmaengine: dw: Add dummy device_caps callback
  dmaengine: dw: Initialize min and max burst DMA device capability
  dmaengine: dw: Introduce max burst length hw config
  dmaengine: dw: Initialize max_sg_nents capability

 .../bindings/dma/snps,dma-spear1340.yaml      | 176 ++++++++++++++++++
 .../devicetree/bindings/dma/snps-dma.txt      |  69 -------
 drivers/dma/dmaengine.c                       |  12 ++
 drivers/dma/dw/core.c                         |  48 ++++-
 drivers/dma/dw/of.c                           |   5 +
 drivers/dma/dw/regs.h                         |   3 +
 include/linux/dmaengine.h                     |  16 ++
 include/linux/platform_data/dma-dw.h          |   5 +
 8 files changed, 264 insertions(+), 70 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
 delete mode 100644 Documentation/devicetree/bindings/dma/snps-dma.txt

-- 
2.26.2


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

* [PATCH v7 01/11] dt-bindings: dma: dw: Convert DW DMAC to DT binding
  2020-07-09 22:45 [PATCH RESEND v7 00/11] dmaengine: dw: Take Baikal-T1 SoC DW DMAC peculiarities into account Serge Semin
@ 2020-07-09 22:45 ` Serge Semin
  2020-07-09 22:45 ` [PATCH v7 02/11] dt-bindings: dma: dw: Add max burst transaction length property Serge Semin
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 33+ messages in thread
From: Serge Semin @ 2020-07-09 22:45 UTC (permalink / raw)
  To: Vinod Koul, Viresh Kumar, Rob Herring, Andy Shevchenko
  Cc: Serge Semin, Serge Semin, Rob Herring, Viresh Kumar,
	Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, linux-mips,
	dmaengine, devicetree, linux-kernel

Modern device tree bindings are supposed to be created as YAML-files
in accordance with dt-schema. This commit replaces the Synopsis
Designware DMA controller legacy bare text bindings with YAML file.
The only required prorties are "compatible", "reg", "#dma-cells" and
"interrupts", which will be used by the driver to correctly find the
controller memory region and handle its events. The rest of the properties
are optional, since in case if either "dma-channels" or "dma-masters" isn't
specified, the driver will attempt to auto-detect the IP core
configuration.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: linux-mips@vger.kernel.org

---

Changelog v2:
- Rearrange SoBs.
- Move $ref to the root level of the properties. So do do with the
  constraints.
- Discard default settings defined out of the property enum constraint.
- Replace "additionalProperties: false" with "unevaluatedProperties: false"
  property.
- Remove a label definition from the binding example.
---
 .../bindings/dma/snps,dma-spear1340.yaml      | 161 ++++++++++++++++++
 .../devicetree/bindings/dma/snps-dma.txt      |  69 --------
 2 files changed, 161 insertions(+), 69 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
 delete mode 100644 Documentation/devicetree/bindings/dma/snps-dma.txt

diff --git a/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml b/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
new file mode 100644
index 000000000000..e7611840a7cf
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
@@ -0,0 +1,161 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/snps,dma-spear1340.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Synopsys Designware DMA Controller
+
+maintainers:
+  - Viresh Kumar <vireshk@kernel.org>
+  - Andy Shevchenko <andriy.shevchenko@linux.intel.com>
+
+allOf:
+  - $ref: "dma-controller.yaml#"
+
+properties:
+  compatible:
+    const: snps,dma-spear1340
+
+  "#dma-cells":
+    const: 3
+    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.
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    description: AHB interface reference clock.
+    const: hclk
+
+  dma-channels:
+    description: |
+      Number of DMA channels supported by the controller. In case if
+      not specified the driver will try to auto-detect this and
+      the rest of the optional parameters.
+    minimum: 1
+    maximum: 8
+
+  dma-requests:
+    minimum: 1
+    maximum: 16
+
+  dma-masters:
+    $ref: /schemas/types.yaml#definitions/uint32
+    description: |
+      Number of DMA masters supported by the controller. In case if
+      not specified the driver will try to auto-detect this and
+      the rest of the optional parameters.
+    minimum: 1
+    maximum: 4
+
+  chan_allocation_order:
+    $ref: /schemas/types.yaml#definitions/uint32
+    description: |
+      DMA channels allocation order specifier. Zero means ascending order
+      (first free allocated), while one - descending (last free allocated).
+    default: 0
+    enum: [0, 1]
+
+  chan_priority:
+    $ref: /schemas/types.yaml#definitions/uint32
+    description: |
+      DMA channels priority order. Zero means ascending channels priority
+      so the very first channel has the highest priority. While 1 means
+      descending priority (the last channel has the highest priority).
+    default: 0
+    enum: [0, 1]
+
+  block_size:
+    $ref: /schemas/types.yaml#definitions/uint32
+    description: Maximum block size supported by the DMA controller.
+    enum: [3, 7, 15, 31, 63, 127, 255, 511, 1023, 2047, 4095]
+
+  data-width:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description: Data bus width per each DMA master in bytes.
+    items:
+      maxItems: 4
+      items:
+        enum: [4, 8, 16, 32]
+
+  data_width:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    deprecated: true
+    description: |
+      Data bus width per each DMA master in (2^n * 8) bits. This property is
+      deprecated. It' usage is discouraged in favor of data-width one. Moreover
+      the property incorrectly permits to define data-bus width of 8 and 16
+      bits, which is impossible in accordance with DW DMAC IP-core data book.
+    items:
+      maxItems: 4
+      items:
+        enum:
+          - 0 # 8 bits
+          - 1 # 16 bits
+          - 2 # 32 bits
+          - 3 # 64 bits
+          - 4 # 128 bits
+          - 5 # 256 bits
+        default: 0
+
+  multi-block:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description: |
+      LLP-based multi-block transfer supported by hardware per
+      each DMA channel.
+    items:
+      maxItems: 8
+      items:
+        enum: [0, 1]
+        default: 1
+
+  snps,dma-protection-control:
+    $ref: /schemas/types.yaml#definitions/uint32
+    description: |
+      Bits one-to-one passed to the AHB HPROT[3:1] bus. Each bit setting
+      indicates the following features: bit 0 - privileged mode,
+      bit 1 - DMA is bufferable, bit 2 - DMA is cacheable.
+    default: 0
+    minimum: 0
+    maximum: 7
+
+unevaluatedProperties: false
+
+required:
+  - compatible
+  - "#dma-cells"
+  - reg
+  - interrupts
+
+examples:
+  - |
+    dma-controller@fc000000 {
+      compatible = "snps,dma-spear1340";
+      reg = <0xfc000000 0x1000>;
+      interrupt-parent = <&vic1>;
+      interrupts = <12>;
+
+      dma-channels = <8>;
+      dma-requests = <16>;
+      dma-masters = <4>;
+      #dma-cells = <3>;
+
+      chan_allocation_order = <1>;
+      chan_priority = <1>;
+      block_size = <0xfff>;
+      data-width = <8 8>;
+      multi-block = <0 0 0 0 0 0 0 0>;
+      snps,max-burst-len = <16 16 4 4 4 4 4 4>;
+    };
+...
diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt b/Documentation/devicetree/bindings/dma/snps-dma.txt
deleted file mode 100644
index 0bedceed1963..000000000000
--- a/Documentation/devicetree/bindings/dma/snps-dma.txt
+++ /dev/null
@@ -1,69 +0,0 @@
-* Synopsys Designware DMA Controller
-
-Required properties:
-- compatible: "snps,dma-spear1340"
-- reg: Address range of the DMAC registers
-- interrupt: Should contain the DMAC interrupt number
-- dma-channels: Number of channels supported by hardware
-- dma-requests: Number of DMA request lines supported, up to 16
-- dma-masters: Number of AHB masters supported by the controller
-- #dma-cells: must be <3>
-- chan_allocation_order: order of allocation of channel, 0 (default): ascending,
-  1: descending
-- chan_priority: priority of channels. 0 (default): increase from chan 0->n, 1:
-  increase from chan n->0
-- block_size: Maximum block size supported by the controller
-- data-width: Maximum data width supported by hardware per AHB master
-  (in bytes, power of 2)
-
-
-Deprecated properties:
-- data_width: Maximum data width supported by hardware per AHB master
-  (0 - 8bits, 1 - 16bits, ..., 5 - 256bits)
-
-
-Optional properties:
-- multi-block: Multi block transfers supported by hardware. Array property with
-  one cell per channel. 0: not supported, 1 (default): supported.
-- snps,dma-protection-control: AHB HPROT[3:1] protection setting.
-  The default value is 0 (for non-cacheable, non-buffered,
-  unprivileged data access).
-  Refer to include/dt-bindings/dma/dw-dmac.h for possible values.
-
-Example:
-
-	dmahost: dma@fc000000 {
-		compatible = "snps,dma-spear1340";
-		reg = <0xfc000000 0x1000>;
-		interrupt-parent = <&vic1>;
-		interrupts = <12>;
-
-		dma-channels = <8>;
-		dma-requests = <16>;
-		dma-masters = <2>;
-		#dma-cells = <3>;
-		chan_allocation_order = <1>;
-		chan_priority = <1>;
-		block_size = <0xfff>;
-		data-width = <8 8>;
-	};
-
-DMA clients connected to the Designware DMA controller must use the format
-described in the dma.txt file, using a four-cell specifier for each channel.
-The four cells in order are:
-
-1. A phandle pointing to the DMA controller
-2. The DMA request line number
-3. Memory master for transfers on allocated channel
-4. Peripheral master for transfers on allocated channel
-
-Example:
-	
-	serial@e0000000 {
-		compatible = "arm,pl011", "arm,primecell";
-		reg = <0xe0000000 0x1000>;
-		interrupts = <0 35 0x4>;
-		dmas = <&dmahost 12 0 1>,
-			<&dmahost 13 1 0>;
-		dma-names = "rx", "rx";
-	};
-- 
2.26.2


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

* [PATCH v7 02/11] dt-bindings: dma: dw: Add max burst transaction length property
  2020-07-09 22:45 [PATCH RESEND v7 00/11] dmaengine: dw: Take Baikal-T1 SoC DW DMAC peculiarities into account Serge Semin
  2020-07-09 22:45 ` [PATCH v7 01/11] dt-bindings: dma: dw: Convert DW DMAC to DT binding Serge Semin
@ 2020-07-09 22:45 ` Serge Semin
  2020-07-09 22:45 ` [PATCH v7 03/11] dmaengine: Introduce min burst length capability Serge Semin
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 33+ messages in thread
From: Serge Semin @ 2020-07-09 22:45 UTC (permalink / raw)
  To: Vinod Koul, Viresh Kumar, Rob Herring
  Cc: Serge Semin, Serge Semin, Rob Herring, Viresh Kumar,
	Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann,
	Andy Shevchenko, linux-mips, dmaengine, devicetree, linux-kernel

This array property is used to indicate the maximum burst transaction
length supported by each DMA channel.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: linux-mips@vger.kernel.org

---

Changelog v2:
- Rearrange SoBs.
- Move $ref to the root level of the properties. So do with the
  constraints.
- Set default max-burst-len to 256 TR-WIDTH words.

Changelog v3:
- Add more details into the property description about what limitations
  snps,max-burst-len defines.
---
 .../bindings/dma/snps,dma-spear1340.yaml          | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml b/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
index e7611840a7cf..20870f5c14dd 100644
--- a/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
+++ b/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
@@ -120,6 +120,21 @@ properties:
         enum: [0, 1]
         default: 1
 
+  snps,max-burst-len:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description: |
+      Maximum length of the burst transactions supported by the controller.
+      This property defines the upper limit of the run-time burst setting
+      (CTLx.SRC_MSIZE/CTLx.DST_MSIZE fields) so the allowed burst length
+      will be from 1 to max-burst-len words. It's an array property with one
+      cell per channel in the units determined by the value set in the
+      CTLx.SRC_TR_WIDTH/CTLx.DST_TR_WIDTH fields (data width).
+    items:
+      maxItems: 8
+      items:
+        enum: [4, 8, 16, 32, 64, 128, 256]
+        default: 256
+
   snps,dma-protection-control:
     $ref: /schemas/types.yaml#definitions/uint32
     description: |
-- 
2.26.2


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

* [PATCH v7 03/11] dmaengine: Introduce min burst length capability
  2020-07-09 22:45 [PATCH RESEND v7 00/11] dmaengine: dw: Take Baikal-T1 SoC DW DMAC peculiarities into account Serge Semin
  2020-07-09 22:45 ` [PATCH v7 01/11] dt-bindings: dma: dw: Convert DW DMAC to DT binding Serge Semin
  2020-07-09 22:45 ` [PATCH v7 02/11] dt-bindings: dma: dw: Add max burst transaction length property Serge Semin
@ 2020-07-09 22:45 ` Serge Semin
  2020-07-09 22:45 ` [PATCH v7 04/11] dmaengine: Introduce max SG list entries capability Serge Semin
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 33+ messages in thread
From: Serge Semin @ 2020-07-09 22:45 UTC (permalink / raw)
  To: Vinod Koul, Viresh Kumar, Dan Williams
  Cc: Serge Semin, Serge Semin, Andy Shevchenko, Alexey Malahov,
	Thomas Bogendoerfer, Arnd Bergmann, Rob Herring, linux-mips,
	devicetree, dmaengine, linux-kernel

Some hardware aside from default 0/1 may have greater minimum burst
transactions length constraints. Here we introduce the DMA device
and slave capability, which if required can be initialized by the DMA
engine driver with the device-specific value.

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: 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:
- This is a new patch created as a result of the discussion with Vinud and
  Andy in the framework of DW DMA burst and LLP capabilities.
---
 drivers/dma/dmaengine.c   | 1 +
 include/linux/dmaengine.h | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index d31076d9ef25..b332ffe52780 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -590,6 +590,7 @@ int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_caps *caps)
 	caps->src_addr_widths = device->src_addr_widths;
 	caps->dst_addr_widths = device->dst_addr_widths;
 	caps->directions = device->directions;
+	caps->min_burst = device->min_burst;
 	caps->max_burst = device->max_burst;
 	caps->residue_granularity = device->residue_granularity;
 	caps->descriptor_reuse = device->descriptor_reuse;
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index e1c03339918f..0c7403b27133 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -465,6 +465,7 @@ enum dma_residue_granularity {
  *	Since the enum dma_transfer_direction is not defined as bit flag for
  *	each type, the dma controller should set BIT(<TYPE>) and same
  *	should be checked by controller as well
+ * @min_burst: min burst capability per-transfer
  * @max_burst: max burst capability per-transfer
  * @cmd_pause: true, if pause is supported (i.e. for reading residue or
  *	       for resume later)
@@ -478,6 +479,7 @@ struct dma_slave_caps {
 	u32 src_addr_widths;
 	u32 dst_addr_widths;
 	u32 directions;
+	u32 min_burst;
 	u32 max_burst;
 	bool cmd_pause;
 	bool cmd_resume;
@@ -769,6 +771,7 @@ struct dma_filter {
  *	Since the enum dma_transfer_direction is not defined as bit flag for
  *	each type, the dma controller should set BIT(<TYPE>) and same
  *	should be checked by controller as well
+ * @min_burst: min burst capability per-transfer
  * @max_burst: max burst capability per-transfer
  * @residue_granularity: granularity of the transfer residue reported
  *	by tx_status
@@ -839,6 +842,7 @@ struct dma_device {
 	u32 src_addr_widths;
 	u32 dst_addr_widths;
 	u32 directions;
+	u32 min_burst;
 	u32 max_burst;
 	bool descriptor_reuse;
 	enum dma_residue_granularity residue_granularity;
-- 
2.26.2


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

* [PATCH v7 04/11] dmaengine: Introduce max SG list entries capability
  2020-07-09 22:45 [PATCH RESEND v7 00/11] dmaengine: dw: Take Baikal-T1 SoC DW DMAC peculiarities into account Serge Semin
                   ` (2 preceding siblings ...)
  2020-07-09 22:45 ` [PATCH v7 03/11] dmaengine: Introduce min burst length capability Serge Semin
@ 2020-07-09 22:45 ` Serge Semin
  2020-07-10  8:31   ` Peter Ujfalusi
  2020-07-09 22:45 ` [PATCH v7 05/11] dmaengine: Introduce DMA-device device_caps callback Serge Semin
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 33+ messages in thread
From: Serge Semin @ 2020-07-09 22:45 UTC (permalink / raw)
  To: Vinod Koul, Viresh Kumar, Dan Williams
  Cc: Serge Semin, Serge Semin, Andy Shevchenko, Alexey Malahov,
	Thomas Bogendoerfer, Arnd Bergmann, Rob Herring, linux-mips,
	devicetree, dmaengine, linux-kernel

Some devices may lack the support of the hardware accelerated SG list
entries automatic walking through and execution. In this case a burden of
the SG list traversal and DMA engine re-initialization lies on the
DMA engine driver (normally implemented by using a DMA transfer completion
IRQ to recharge the DMA device with a next SG list entry). But such
solution may not be suitable for some DMA consumers. In particular SPI
devices need both Tx and Rx DMA channels work synchronously in order
to avoid the Rx FIFO overflow. In case if Rx DMA channel is paused for
some time while the Tx DMA channel works implicitly pulling data into the
Rx FIFO, the later will be eventually overflown, which will cause the data
loss. So if SG list entries aren't automatically fetched by the DMA
engine, but are one-by-one manually selected for execution in the
ISRs/deferred work/etc., such problem will eventually happen due to the
non-deterministic latencies of the service execution.

In order to let the DMA consumer know about the DMA device capabilities
regarding the hardware accelerated SG list traversal we introduce the
max_sg_list capability. It is supposed to be initialized by the DMA engine
driver with 0 if there is no limitation for the number of SG entries
atomically executed and with non-zero value if there is such constraints,
so the upper limit is determined by the number set to the property.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
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: 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:
- This is a new patch created as a result of the discussion with Vinud and
  Andy in the framework of DW DMA burst and LLP capabilities.

Changelog v4:
- Fix of->if typo. It should be definitely of.
---
 drivers/dma/dmaengine.c   | 1 +
 include/linux/dmaengine.h | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index b332ffe52780..ad56ad58932c 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -592,6 +592,7 @@ int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_caps *caps)
 	caps->directions = device->directions;
 	caps->min_burst = device->min_burst;
 	caps->max_burst = device->max_burst;
+	caps->max_sg_nents = device->max_sg_nents;
 	caps->residue_granularity = device->residue_granularity;
 	caps->descriptor_reuse = device->descriptor_reuse;
 	caps->cmd_pause = !!device->device_pause;
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 0c7403b27133..a7e4d8dfdd19 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -467,6 +467,9 @@ enum dma_residue_granularity {
  *	should be checked by controller as well
  * @min_burst: min burst capability per-transfer
  * @max_burst: max burst capability per-transfer
+ * @max_sg_nents: max number of SG list entries executed in a single atomic
+ *	DMA tansaction with no intermediate IRQ for reinitialization. Zero
+ *	value means unlimited number of entries.
  * @cmd_pause: true, if pause is supported (i.e. for reading residue or
  *	       for resume later)
  * @cmd_resume: true, if resume is supported
@@ -481,6 +484,7 @@ struct dma_slave_caps {
 	u32 directions;
 	u32 min_burst;
 	u32 max_burst;
+	u32 max_sg_nents;
 	bool cmd_pause;
 	bool cmd_resume;
 	bool cmd_terminate;
@@ -773,6 +777,9 @@ struct dma_filter {
  *	should be checked by controller as well
  * @min_burst: min burst capability per-transfer
  * @max_burst: max burst capability per-transfer
+ * @max_sg_nents: max number of SG list entries executed in a single atomic
+ *	DMA tansaction with no intermediate IRQ for reinitialization. Zero
+ *	value means unlimited number of entries.
  * @residue_granularity: granularity of the transfer residue reported
  *	by tx_status
  * @device_alloc_chan_resources: allocate resources and return the
@@ -844,6 +851,7 @@ struct dma_device {
 	u32 directions;
 	u32 min_burst;
 	u32 max_burst;
+	u32 max_sg_nents;
 	bool descriptor_reuse;
 	enum dma_residue_granularity residue_granularity;
 
-- 
2.26.2


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

* [PATCH v7 05/11] dmaengine: Introduce DMA-device device_caps callback
  2020-07-09 22:45 [PATCH RESEND v7 00/11] dmaengine: dw: Take Baikal-T1 SoC DW DMAC peculiarities into account Serge Semin
                   ` (3 preceding siblings ...)
  2020-07-09 22:45 ` [PATCH v7 04/11] dmaengine: Introduce max SG list entries capability Serge Semin
@ 2020-07-09 22:45 ` Serge Semin
  2020-07-10  8:45   ` Andy Shevchenko
  2020-07-09 22:45 ` [PATCH v7 06/11] dmaengine: dw: Take HC_LLP flag into account for noLLP auto-config Serge Semin
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 33+ messages in thread
From: Serge Semin @ 2020-07-09 22:45 UTC (permalink / raw)
  To: Vinod Koul, Viresh Kumar, Dan Williams
  Cc: Serge Semin, Serge Semin, Andy Shevchenko, Alexey Malahov,
	Thomas Bogendoerfer, Arnd Bergmann, Rob Herring, linux-mips,
	devicetree, dmaengine, linux-kernel

There are DMA devices (like ours version of Synopsys DW DMAC) which have
DMA capabilities non-uniformly redistributed between the device channels.
In order to provide a way of exposing the channel-specific parameters to
the DMA engine consumers, we introduce a new DMA-device callback. In case
if provided it gets called from the dma_get_slave_caps() method and is
able to override the generic DMA-device capabilities.

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: 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:
- This is a new patch created as a result of the discussion with Vinod and
  Andy in the framework of DW DMA burst and LLP capabilities.

Changelog v5:
- Add in-line comment at the point of the device_caps callback invocation.
- Add doc-comment for the device_caps member of struct dma_device.
---
 drivers/dma/dmaengine.c   | 10 ++++++++++
 include/linux/dmaengine.h |  4 ++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index ad56ad58932c..ed1fd376f1a5 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -599,6 +599,16 @@ int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_caps *caps)
 	caps->cmd_resume = !!device->device_resume;
 	caps->cmd_terminate = !!device->device_terminate_all;
 
+	/*
+	 * DMA engine device might be configured with non-uniformly
+	 * distributed slave capabilities per device channels. In this
+	 * case the corresponding driver may provide the device_caps
+	 * callback to override the generic capabilities with
+	 * channel-specific ones.
+	 */
+	if (device->device_caps)
+		device->device_caps(chan, caps);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(dma_get_slave_caps);
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index a7e4d8dfdd19..298b721c8b9f 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -799,6 +799,8 @@ struct dma_filter {
  *	be called after period_len bytes have been transferred.
  * @device_prep_interleaved_dma: Transfer expression in a generic way.
  * @device_prep_dma_imm_data: DMA's 8 byte immediate data to the dst address
+ * @device_caps: May be used to override the generic DMA slave capabilities
+ *	with per-channel specific ones
  * @device_config: Pushes a new configuration to a channel, return 0 or an error
  *	code
  * @device_pause: Pauses any transfer happening on a channel. Returns
@@ -899,6 +901,8 @@ struct dma_device {
 		struct dma_chan *chan, dma_addr_t dst, u64 data,
 		unsigned long flags);
 
+	void (*device_caps)(struct dma_chan *chan,
+			    struct dma_slave_caps *caps);
 	int (*device_config)(struct dma_chan *chan,
 			     struct dma_slave_config *config);
 	int (*device_pause)(struct dma_chan *chan);
-- 
2.26.2


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

* [PATCH v7 06/11] dmaengine: dw: Take HC_LLP flag into account for noLLP auto-config
  2020-07-09 22:45 [PATCH RESEND v7 00/11] dmaengine: dw: Take Baikal-T1 SoC DW DMAC peculiarities into account Serge Semin
                   ` (4 preceding siblings ...)
  2020-07-09 22:45 ` [PATCH v7 05/11] dmaengine: Introduce DMA-device device_caps callback Serge Semin
@ 2020-07-09 22:45 ` Serge Semin
  2020-07-09 22:45 ` [PATCH v7 07/11] dmaengine: dw: Set DMA device max segment size parameter Serge Semin
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 33+ messages in thread
From: Serge Semin @ 2020-07-09 22:45 UTC (permalink / raw)
  To: Vinod Koul, Viresh Kumar, Andy Shevchenko, Dan Williams
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Thomas Bogendoerfer,
	Arnd Bergmann, Rob Herring, linux-mips, devicetree, dmaengine,
	linux-kernel

Full multi-block transfers functionality is enabled in DW DMA
controller only if CHx_MULTI_BLK_EN is set. But LLP-based transfers
can be executed only if hardcode channel x LLP register feature isn't
enabled, which can be switched on at the IP core synthesis for
optimization. If it's enabled then the LLP register is hardcoded to
zero, so the blocks chaining based on the LLPs is unsupported.

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: Arnd Bergmann <arnd@arndb.de>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-mips@vger.kernel.org
Cc: devicetree@vger.kernel.org

---

Changelog v2:
- Rearrange SoBs.
- Add comment about why hardware accelerated LLP list support depends
  on both MBLK_EN and HC_LLP configs setting.
- Use explicit bits state comparison operator.

Changelog v3:
- Move the patch to the head of the series.
---
 drivers/dma/dw/core.c | 11 ++++++++++-
 drivers/dma/dw/regs.h |  1 +
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index 21cb2a58dbd2..33e99d95b3d3 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -1178,8 +1178,17 @@ int do_dma_probe(struct dw_dma_chip *chip)
 			 */
 			dwc->block_size =
 				(4 << ((pdata->block_size >> 4 * i) & 0xf)) - 1;
+
+			/*
+			 * According to the DW DMA databook the true scatter-
+			 * gether LLPs aren't available if either multi-block
+			 * config is disabled (CHx_MULTI_BLK_EN == 0) or the
+			 * LLP register is hard-coded to zeros
+			 * (CHx_HC_LLP == 1).
+			 */
 			dwc->nollp =
-				(dwc_params >> DWC_PARAMS_MBLK_EN & 0x1) == 0;
+				(dwc_params >> DWC_PARAMS_MBLK_EN & 0x1) == 0 ||
+				(dwc_params >> DWC_PARAMS_HC_LLP & 0x1) == 1;
 		} else {
 			dwc->block_size = pdata->block_size;
 			dwc->nollp = !pdata->multi_block[i];
diff --git a/drivers/dma/dw/regs.h b/drivers/dma/dw/regs.h
index 3fce66ecee7a..1ab840b06e79 100644
--- a/drivers/dma/dw/regs.h
+++ b/drivers/dma/dw/regs.h
@@ -125,6 +125,7 @@ struct dw_dma_regs {
 
 /* Bitfields in DWC_PARAMS */
 #define DWC_PARAMS_MBLK_EN	11		/* multi block transfer */
+#define DWC_PARAMS_HC_LLP	13		/* set LLP register to zero */
 
 /* bursts size */
 enum dw_dma_msize {
-- 
2.26.2


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

* [PATCH v7 07/11] dmaengine: dw: Set DMA device max segment size parameter
  2020-07-09 22:45 [PATCH RESEND v7 00/11] dmaengine: dw: Take Baikal-T1 SoC DW DMAC peculiarities into account Serge Semin
                   ` (5 preceding siblings ...)
  2020-07-09 22:45 ` [PATCH v7 06/11] dmaengine: dw: Take HC_LLP flag into account for noLLP auto-config Serge Semin
@ 2020-07-09 22:45 ` Serge Semin
  2020-07-09 22:45 ` [PATCH v7 08/11] dmaengine: dw: Add dummy device_caps callback Serge Semin
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 33+ messages in thread
From: Serge Semin @ 2020-07-09 22:45 UTC (permalink / raw)
  To: Vinod Koul, Viresh Kumar, Andy Shevchenko, Dan Williams
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Thomas Bogendoerfer,
	Arnd Bergmann, Rob Herring, linux-mips, devicetree, dmaengine,
	linux-kernel

Maximum block size DW DMAC configuration corresponds to the max segment
size DMA parameter in the DMA core subsystem notation. Lets set it with a
value specific to the probed DW DMA controller. It shall help the DMA
clients to create size-optimized SG-list items for the controller. This in
turn will cause less dw_desc allocations, less LLP reinitializations,
better DMA device performance.

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: Arnd Bergmann <arnd@arndb.de>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-mips@vger.kernel.org
Cc: devicetree@vger.kernel.org

---

Changelog v2:
- This is a new patch created in place of the dropped one:
  "dmaengine: dw: Add LLP and block size config accessors".

Changelog v3:
- Use the block_size found for the very first channel instead of looking for
  the maximum of maximum block sizes.
- Don't define device-specific device_dma_parameters object, since it has
  already been defined by the platform device core.
---
 drivers/dma/dw/core.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index 33e99d95b3d3..fb95920c429e 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -1229,6 +1229,13 @@ int do_dma_probe(struct dw_dma_chip *chip)
 			     BIT(DMA_MEM_TO_MEM);
 	dw->dma.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
 
+	/*
+	 * For now there is no hardware with non uniform maximum block size
+	 * across all of the device channels, so we set the maximum segment
+	 * size as the block size found for the very first channel.
+	 */
+	dma_set_max_seg_size(dw->dma.dev, dw->chan[0].block_size);
+
 	err = dma_async_device_register(&dw->dma);
 	if (err)
 		goto err_dma_register;
-- 
2.26.2


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

* [PATCH v7 08/11] dmaengine: dw: Add dummy device_caps callback
  2020-07-09 22:45 [PATCH RESEND v7 00/11] dmaengine: dw: Take Baikal-T1 SoC DW DMAC peculiarities into account Serge Semin
                   ` (6 preceding siblings ...)
  2020-07-09 22:45 ` [PATCH v7 07/11] dmaengine: dw: Set DMA device max segment size parameter Serge Semin
@ 2020-07-09 22:45 ` Serge Semin
  2020-07-10  8:51   ` Andy Shevchenko
  2020-07-09 22:45 ` [PATCH v7 09/11] dmaengine: dw: Initialize min and max burst DMA device capability Serge Semin
                   ` (2 subsequent siblings)
  10 siblings, 1 reply; 33+ messages in thread
From: Serge Semin @ 2020-07-09 22:45 UTC (permalink / raw)
  To: Vinod Koul, Viresh Kumar, Andy Shevchenko, Dan Williams
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Thomas Bogendoerfer,
	Arnd Bergmann, Rob Herring, linux-mips, devicetree, dmaengine,
	linux-kernel

Since some DW DMA controllers (like one installed on Baikal-T1 SoC) may
have non-uniform DMA capabilities per device channels, let's add
the DW DMA specific device_caps callback to expose that specifics up to
the DMA consumer. It's a dummy function for now. We'll fill it in with
capabilities overrides in the next commits.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
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:
- This is a new patch created as a result of the discussion with Vinud and
  Andy in the framework of DW DMA burst and LLP capabilities.
---
 drivers/dma/dw/core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index fb95920c429e..ceded21537e2 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -1049,6 +1049,11 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
 	dev_vdbg(chan2dev(chan), "%s: done\n", __func__);
 }
 
+static void dwc_caps(struct dma_chan *chan, struct dma_slave_caps *caps)
+{
+
+}
+
 int do_dma_probe(struct dw_dma_chip *chip)
 {
 	struct dw_dma *dw = chip->dw;
@@ -1214,6 +1219,7 @@ int do_dma_probe(struct dw_dma_chip *chip)
 	dw->dma.device_prep_dma_memcpy = dwc_prep_dma_memcpy;
 	dw->dma.device_prep_slave_sg = dwc_prep_slave_sg;
 
+	dw->dma.device_caps = dwc_caps;
 	dw->dma.device_config = dwc_config;
 	dw->dma.device_pause = dwc_pause;
 	dw->dma.device_resume = dwc_resume;
-- 
2.26.2


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

* [PATCH v7 09/11] dmaengine: dw: Initialize min and max burst DMA device capability
  2020-07-09 22:45 [PATCH RESEND v7 00/11] dmaengine: dw: Take Baikal-T1 SoC DW DMAC peculiarities into account Serge Semin
                   ` (7 preceding siblings ...)
  2020-07-09 22:45 ` [PATCH v7 08/11] dmaengine: dw: Add dummy device_caps callback Serge Semin
@ 2020-07-09 22:45 ` Serge Semin
  2020-07-09 22:45 ` [PATCH v7 10/11] dmaengine: dw: Introduce max burst length hw config Serge Semin
  2020-07-09 22:45 ` [PATCH v7 11/11] dmaengine: dw: Initialize max_sg_nents capability Serge Semin
  10 siblings, 0 replies; 33+ messages in thread
From: Serge Semin @ 2020-07-09 22:45 UTC (permalink / raw)
  To: Vinod Koul, Viresh Kumar, Andy Shevchenko, Dan Williams
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Thomas Bogendoerfer,
	Arnd Bergmann, Rob Herring, linux-mips, devicetree, dmaengine,
	linux-kernel

According to the DW APB DMAC data book the minimum burst transaction
length is 1 and it's true for any version of the controller since
isn't parametrised in the coreAssembler so can't be changed at the
IP-core synthesis stage. The maximum burst transaction can vary from
channel to channel and from controller to controller depending on a
IP-core parameter the system engineer activated during the IP-core
synthesis. Let's initialise both min_burst and max_burst members of the
DMA controller descriptor with extreme values so the DMA clients could
use them to properly optimize the DMA requests. The channels and
controller-specific max_burst length initialization will be introduced
by the follow-up patches.

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: Arnd Bergmann <arnd@arndb.de>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-mips@vger.kernel.org
Cc: devicetree@vger.kernel.org

---

Changelog v4:
- This is a new patch suggested by Andy.

Changelog v5:
- Introduce macro with extreme min and max burst length supported by the
  DW DMA controller.
- Initialize max_burst length capability with extreme burst length supported
  by the DW DMAC IP-core.
---
 drivers/dma/dw/core.c                | 2 ++
 include/linux/platform_data/dma-dw.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index ceded21537e2..4887aa2fc73c 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -1229,6 +1229,8 @@ int do_dma_probe(struct dw_dma_chip *chip)
 	dw->dma.device_issue_pending = dwc_issue_pending;
 
 	/* DMA capabilities */
+	dw->dma.min_burst = DW_DMA_MIN_BURST;
+	dw->dma.max_burst = DW_DMA_MAX_BURST;
 	dw->dma.src_addr_widths = DW_DMA_BUSWIDTHS;
 	dw->dma.dst_addr_widths = DW_DMA_BUSWIDTHS;
 	dw->dma.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV) |
diff --git a/include/linux/platform_data/dma-dw.h b/include/linux/platform_data/dma-dw.h
index f3eaf9ec00a1..369e41e9dcc9 100644
--- a/include/linux/platform_data/dma-dw.h
+++ b/include/linux/platform_data/dma-dw.h
@@ -12,6 +12,8 @@
 
 #define DW_DMA_MAX_NR_MASTERS	4
 #define DW_DMA_MAX_NR_CHANNELS	8
+#define DW_DMA_MIN_BURST	1
+#define DW_DMA_MAX_BURST	256
 
 /**
  * struct dw_dma_slave - Controller-specific information about a slave
-- 
2.26.2


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

* [PATCH v7 10/11] dmaengine: dw: Introduce max burst length hw config
  2020-07-09 22:45 [PATCH RESEND v7 00/11] dmaengine: dw: Take Baikal-T1 SoC DW DMAC peculiarities into account Serge Semin
                   ` (8 preceding siblings ...)
  2020-07-09 22:45 ` [PATCH v7 09/11] dmaengine: dw: Initialize min and max burst DMA device capability Serge Semin
@ 2020-07-09 22:45 ` Serge Semin
  2020-07-09 22:45 ` [PATCH v7 11/11] dmaengine: dw: Initialize max_sg_nents capability Serge Semin
  10 siblings, 0 replies; 33+ messages in thread
From: Serge Semin @ 2020-07-09 22:45 UTC (permalink / raw)
  To: Vinod Koul, Viresh Kumar, Andy Shevchenko, Dan Williams
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Thomas Bogendoerfer,
	Arnd Bergmann, Rob Herring, linux-mips, devicetree, dmaengine,
	linux-kernel

IP core of the DW DMA controller may be synthesized with different
max burst length of the transfers per each channel. According to Synopsis
having the fixed maximum burst transactions length may provide some
performance gain. At the same time setting up the source and destination
multi size exceeding the max burst length limitation may cause a serious
problems. In our case the DMA transaction just hangs up. In order to fix
this lets introduce the max burst length platform config of the DW DMA
controller device and don't let the DMA channels configuration code
exceed the burst length hardware limitation.

Note the maximum burst length parameter can be detected either in runtime
from the DWC parameter registers or from the dedicated DT property.
Depending on the IP core configuration the maximum value can vary from
channel to channel so by overriding the channel slave max_burst capability
we make sure a DMA consumer will get the channel-specific max burst
length.

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: Arnd Bergmann <arnd@arndb.de>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-mips@vger.kernel.org
Cc: devicetree@vger.kernel.org

---

Changelog v2:
- Rearrange SoBs.
- Discard dwc_get_maxburst() accessor. It's enough to have a clamping
  guard against exceeding the hardware max burst limitation.

Changelog v3:
- Override the slave channel max_burst capability instead of calculating
  the minimum value of max burst lengths and setting the DMA-device
  generic capability.

Changelog v5:
- Clamp the dst and src burst lengths in the generic dwc_config() method
  instead of doing that in the encode_maxburst() callback.
- Define max_burst with u32 type in struct dw_dma_platform_data.
- Perform of_property_read_u32_array() directly into the platform data
  max_burst member.

Changelog v6:
- Move DW_DMA_MAX_BURST macro definition to the previous patch.
---
 drivers/dma/dw/core.c                | 10 ++++++++++
 drivers/dma/dw/of.c                  |  5 +++++
 drivers/dma/dw/regs.h                |  2 ++
 include/linux/platform_data/dma-dw.h |  3 +++
 4 files changed, 20 insertions(+)

diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index 4887aa2fc73c..588b9bae827c 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -791,6 +791,11 @@ static int dwc_config(struct dma_chan *chan, struct dma_slave_config *sconfig)
 
 	memcpy(&dwc->dma_sconfig, sconfig, sizeof(*sconfig));
 
+	dwc->dma_sconfig.src_maxburst =
+		clamp(dwc->dma_sconfig.src_maxburst, 0U, dwc->max_burst);
+	dwc->dma_sconfig.dst_maxburst =
+		clamp(dwc->dma_sconfig.dst_maxburst, 0U, dwc->max_burst);
+
 	dw->encode_maxburst(dwc, &dwc->dma_sconfig.src_maxburst);
 	dw->encode_maxburst(dwc, &dwc->dma_sconfig.dst_maxburst);
 
@@ -1051,7 +1056,9 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
 
 static void dwc_caps(struct dma_chan *chan, struct dma_slave_caps *caps)
 {
+	struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
 
+	caps->max_burst = dwc->max_burst;
 }
 
 int do_dma_probe(struct dw_dma_chip *chip)
@@ -1194,9 +1201,12 @@ int do_dma_probe(struct dw_dma_chip *chip)
 			dwc->nollp =
 				(dwc_params >> DWC_PARAMS_MBLK_EN & 0x1) == 0 ||
 				(dwc_params >> DWC_PARAMS_HC_LLP & 0x1) == 1;
+			dwc->max_burst =
+				(0x4 << (dwc_params >> DWC_PARAMS_MSIZE & 0x7));
 		} else {
 			dwc->block_size = pdata->block_size;
 			dwc->nollp = !pdata->multi_block[i];
+			dwc->max_burst = pdata->max_burst[i] ?: DW_DMA_MAX_BURST;
 		}
 	}
 
diff --git a/drivers/dma/dw/of.c b/drivers/dma/dw/of.c
index 9e27831dee32..1474b3817ef4 100644
--- a/drivers/dma/dw/of.c
+++ b/drivers/dma/dw/of.c
@@ -98,6 +98,11 @@ struct dw_dma_platform_data *dw_dma_parse_dt(struct platform_device *pdev)
 			pdata->multi_block[tmp] = 1;
 	}
 
+	if (of_property_read_u32_array(np, "snps,max-burst-len", pdata->max_burst,
+				       nr_channels)) {
+		memset32(pdata->max_burst, DW_DMA_MAX_BURST, nr_channels);
+	}
+
 	if (!of_property_read_u32(np, "snps,dma-protection-control", &tmp)) {
 		if (tmp > CHAN_PROTCTL_MASK)
 			return NULL;
diff --git a/drivers/dma/dw/regs.h b/drivers/dma/dw/regs.h
index 1ab840b06e79..76654bd13c1a 100644
--- a/drivers/dma/dw/regs.h
+++ b/drivers/dma/dw/regs.h
@@ -126,6 +126,7 @@ struct dw_dma_regs {
 /* Bitfields in DWC_PARAMS */
 #define DWC_PARAMS_MBLK_EN	11		/* multi block transfer */
 #define DWC_PARAMS_HC_LLP	13		/* set LLP register to zero */
+#define DWC_PARAMS_MSIZE	16		/* max group transaction size */
 
 /* bursts size */
 enum dw_dma_msize {
@@ -284,6 +285,7 @@ struct dw_dma_chan {
 	/* hardware configuration */
 	unsigned int		block_size;
 	bool			nollp;
+	u32			max_burst;
 
 	/* custom slave configuration */
 	struct dw_dma_slave	dws;
diff --git a/include/linux/platform_data/dma-dw.h b/include/linux/platform_data/dma-dw.h
index 369e41e9dcc9..4f681df85c27 100644
--- a/include/linux/platform_data/dma-dw.h
+++ b/include/linux/platform_data/dma-dw.h
@@ -44,6 +44,8 @@ struct dw_dma_slave {
  * @data_width: Maximum data width supported by hardware per AHB master
  *		(in bytes, power of 2)
  * @multi_block: Multi block transfers supported by hardware per channel.
+ * @max_burst: Maximum value of burst transaction size supported by hardware
+ *	       per channel (in units of CTL.SRC_TR_WIDTH/CTL.DST_TR_WIDTH).
  * @protctl: Protection control signals setting per channel.
  */
 struct dw_dma_platform_data {
@@ -58,6 +60,7 @@ struct dw_dma_platform_data {
 	unsigned char	nr_masters;
 	unsigned char	data_width[DW_DMA_MAX_NR_MASTERS];
 	unsigned char	multi_block[DW_DMA_MAX_NR_CHANNELS];
+	u32		max_burst[DW_DMA_MAX_NR_CHANNELS];
 #define CHAN_PROTCTL_PRIVILEGED		BIT(0)
 #define CHAN_PROTCTL_BUFFERABLE		BIT(1)
 #define CHAN_PROTCTL_CACHEABLE		BIT(2)
-- 
2.26.2


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

* [PATCH v7 11/11] dmaengine: dw: Initialize max_sg_nents capability
  2020-07-09 22:45 [PATCH RESEND v7 00/11] dmaengine: dw: Take Baikal-T1 SoC DW DMAC peculiarities into account Serge Semin
                   ` (9 preceding siblings ...)
  2020-07-09 22:45 ` [PATCH v7 10/11] dmaengine: dw: Introduce max burst length hw config Serge Semin
@ 2020-07-09 22:45 ` Serge Semin
  10 siblings, 0 replies; 33+ messages in thread
From: Serge Semin @ 2020-07-09 22:45 UTC (permalink / raw)
  To: Vinod Koul, Viresh Kumar, Andy Shevchenko, Dan Williams
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Thomas Bogendoerfer,
	Arnd Bergmann, Rob Herring, linux-mips, devicetree, dmaengine,
	linux-kernel

Multi-block support provides a way to map the kernel-specific SG-table so
the DW DMA device would handle it as a whole instead of handling the
SG-list items or so called LLP block items one by one. So if true LLP
list isn't supported by the DW DMA engine, then soft-LLP mode will be
utilized to load and execute each LLP-block one by one. The soft-LLP mode
of the DMA transactions execution might not work well for some DMA
consumers like SPI due to its Tx and Rx buffers inter-dependency. Let's
initialize the max_sg_nents DMA channels capability based on the nollp
flag state. If it's true, no hardware accelerated LLP is available and
max_sg_nents should be set with 1, which means that the DMA engine
can handle only a single SG list entry at a time. If noLLP is set to
false, then hardware accelerated LLP is supported and the DMA engine
can handle infinite number of SG entries in a single DMA transaction.

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: 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:
- This is a new patch created as a result of the discussion with Vinud and
  Andy in the framework of DW DMA burst and LLP capabilities.

Changelog v4:
- Use explicit if-else statement when assigning the max_sg_nents field.
---
 drivers/dma/dw/core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index 588b9bae827c..9dee5fa65153 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -1059,6 +1059,18 @@ static void dwc_caps(struct dma_chan *chan, struct dma_slave_caps *caps)
 	struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
 
 	caps->max_burst = dwc->max_burst;
+
+	/*
+	 * It might be crucial for some devices to have the hardware
+	 * accelerated multi-block transfers supported, aka LLPs in DW DMAC
+	 * notation. So if LLPs are supported then max_sg_nents is set to
+	 * zero which means unlimited number of SG entries can be handled in a
+	 * single DMA transaction, otherwise it's just one SG entry.
+	 */
+	if (dwc->nollp)
+		caps->max_sg_nents = 1;
+	else
+		caps->max_sg_nents = 0;
 }
 
 int do_dma_probe(struct dw_dma_chip *chip)
-- 
2.26.2


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

* Re: [PATCH v7 04/11] dmaengine: Introduce max SG list entries capability
  2020-07-09 22:45 ` [PATCH v7 04/11] dmaengine: Introduce max SG list entries capability Serge Semin
@ 2020-07-10  8:31   ` Peter Ujfalusi
  2020-07-10  9:27     ` Serge Semin
  0 siblings, 1 reply; 33+ messages in thread
From: Peter Ujfalusi @ 2020-07-10  8:31 UTC (permalink / raw)
  To: Serge Semin, Vinod Koul, Viresh Kumar, Dan Williams
  Cc: Serge Semin, Andy Shevchenko, Alexey Malahov,
	Thomas Bogendoerfer, Arnd Bergmann, Rob Herring, linux-mips,
	devicetree, dmaengine, linux-kernel



On 10/07/2020 1.45, Serge Semin wrote:
> Some devices may lack the support of the hardware accelerated SG list
> entries automatic walking through and execution. In this case a burden of
> the SG list traversal and DMA engine re-initialization lies on the
> DMA engine driver (normally implemented by using a DMA transfer completion
> IRQ to recharge the DMA device with a next SG list entry). But such
> solution may not be suitable for some DMA consumers. In particular SPI
> devices need both Tx and Rx DMA channels work synchronously in order
> to avoid the Rx FIFO overflow. In case if Rx DMA channel is paused for
> some time while the Tx DMA channel works implicitly pulling data into the
> Rx FIFO, the later will be eventually overflown, which will cause the data
> loss. So if SG list entries aren't automatically fetched by the DMA
> engine, but are one-by-one manually selected for execution in the
> ISRs/deferred work/etc., such problem will eventually happen due to the
> non-deterministic latencies of the service execution.

It is not really the number of sg nents which is the problem, but the
combination of total number of bytes _and_ the number of nents used to
map them.
Obviously the TX and RX number of bytes must match in duplex case and at
the same time neither nents should be over the number of SGs the DMA
device can handle without interruption (linking, chaining, or whatever
means).

The EDMA from TI have similar limitation (we set the limit to 20 nents).
Longer lists will be broken up to maximum of 20 segment transfers.
This setup has bigger impact on audio (cyclic) as we need to limit the
number of periods to not exceed this limit of 20.

The sDMA on the other hand has different limits. Earlier versions
without linking support can execute one SG chunk at the time and needs
to reconfigure for the next one -> max_sg_nents is 1 for the older sDMAs...

> In order to let the DMA consumer know about the DMA device capabilities
> regarding the hardware accelerated SG list traversal we introduce the
> max_sg_list capability. It is supposed to be initialized by the DMA engine
> driver with 0 if there is no limitation for the number of SG entries
> atomically executed and with non-zero value if there is such constraints,
> so the upper limit is determined by the number set to the property.
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 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: 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:
> - This is a new patch created as a result of the discussion with Vinud and
>   Andy in the framework of DW DMA burst and LLP capabilities.
> 
> Changelog v4:
> - Fix of->if typo. It should be definitely of.
> ---
>  drivers/dma/dmaengine.c   | 1 +
>  include/linux/dmaengine.h | 8 ++++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index b332ffe52780..ad56ad58932c 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -592,6 +592,7 @@ int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_caps *caps)
>  	caps->directions = device->directions;
>  	caps->min_burst = device->min_burst;
>  	caps->max_burst = device->max_burst;
> +	caps->max_sg_nents = device->max_sg_nents;
>  	caps->residue_granularity = device->residue_granularity;
>  	caps->descriptor_reuse = device->descriptor_reuse;
>  	caps->cmd_pause = !!device->device_pause;
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 0c7403b27133..a7e4d8dfdd19 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -467,6 +467,9 @@ enum dma_residue_granularity {
>   *	should be checked by controller as well
>   * @min_burst: min burst capability per-transfer
>   * @max_burst: max burst capability per-transfer
> + * @max_sg_nents: max number of SG list entries executed in a single atomic
> + *	DMA tansaction with no intermediate IRQ for reinitialization. Zero
> + *	value means unlimited number of entries.

Without looking at the comment the name max_sg_nents implies that the
DMA can not handle longer lists, but it is not really true.
max_sg_nents_burst might be a bit cleaner for the first look?

>   * @cmd_pause: true, if pause is supported (i.e. for reading residue or
>   *	       for resume later)
>   * @cmd_resume: true, if resume is supported
> @@ -481,6 +484,7 @@ struct dma_slave_caps {
>  	u32 directions;
>  	u32 min_burst;
>  	u32 max_burst;
> +	u32 max_sg_nents;
>  	bool cmd_pause;
>  	bool cmd_resume;
>  	bool cmd_terminate;
> @@ -773,6 +777,9 @@ struct dma_filter {
>   *	should be checked by controller as well
>   * @min_burst: min burst capability per-transfer
>   * @max_burst: max burst capability per-transfer
> + * @max_sg_nents: max number of SG list entries executed in a single atomic
> + *	DMA tansaction with no intermediate IRQ for reinitialization. Zero
> + *	value means unlimited number of entries.
>   * @residue_granularity: granularity of the transfer residue reported
>   *	by tx_status
>   * @device_alloc_chan_resources: allocate resources and return the
> @@ -844,6 +851,7 @@ struct dma_device {
>  	u32 directions;
>  	u32 min_burst;
>  	u32 max_burst;
> +	u32 max_sg_nents;
>  	bool descriptor_reuse;
>  	enum dma_residue_granularity residue_granularity;
>  
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki


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

* Re: [PATCH v7 05/11] dmaengine: Introduce DMA-device device_caps callback
  2020-07-09 22:45 ` [PATCH v7 05/11] dmaengine: Introduce DMA-device device_caps callback Serge Semin
@ 2020-07-10  8:45   ` Andy Shevchenko
  2020-07-10  9:38     ` Serge Semin
  0 siblings, 1 reply; 33+ messages in thread
From: Andy Shevchenko @ 2020-07-10  8:45 UTC (permalink / raw)
  To: Serge Semin
  Cc: Vinod Koul, Viresh Kumar, Dan Williams, Serge Semin,
	Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, Rob Herring,
	linux-mips, devicetree, dmaengine, linux-kernel

On Fri, Jul 10, 2020 at 01:45:44AM +0300, Serge Semin wrote:
> There are DMA devices (like ours version of Synopsys DW DMAC) which have
> DMA capabilities non-uniformly redistributed between the device channels.
> In order to provide a way of exposing the channel-specific parameters to
> the DMA engine consumers, we introduce a new DMA-device callback. In case
> if provided it gets called from the dma_get_slave_caps() method and is
> able to override the generic DMA-device capabilities.

In light of recent developments consider not to add 'slave' and a such words to the kernel.


-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH v7 08/11] dmaengine: dw: Add dummy device_caps callback
  2020-07-09 22:45 ` [PATCH v7 08/11] dmaengine: dw: Add dummy device_caps callback Serge Semin
@ 2020-07-10  8:51   ` Andy Shevchenko
  2020-07-10  8:52     ` Andy Shevchenko
  2020-07-10  9:45     ` Serge Semin
  0 siblings, 2 replies; 33+ messages in thread
From: Andy Shevchenko @ 2020-07-10  8:51 UTC (permalink / raw)
  To: Serge Semin
  Cc: Vinod Koul, Viresh Kumar, Dan Williams, Serge Semin,
	Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, Rob Herring,
	linux-mips, devicetree, dmaengine, linux-kernel

On Fri, Jul 10, 2020 at 01:45:47AM +0300, Serge Semin wrote:
> Since some DW DMA controllers (like one installed on Baikal-T1 SoC) may
> have non-uniform DMA capabilities per device channels, let's add
> the DW DMA specific device_caps callback to expose that specifics up to
> the DMA consumer. It's a dummy function for now. We'll fill it in with
> capabilities overrides in the next commits.

Just a reminder (mainly to Vinod) of my view to this.
Unneeded churn, should be folded to patch 9.

> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> 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:
> - This is a new patch created as a result of the discussion with Vinud and
>   Andy in the framework of DW DMA burst and LLP capabilities.
> ---
>  drivers/dma/dw/core.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
> index fb95920c429e..ceded21537e2 100644
> --- a/drivers/dma/dw/core.c
> +++ b/drivers/dma/dw/core.c
> @@ -1049,6 +1049,11 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
>  	dev_vdbg(chan2dev(chan), "%s: done\n", __func__);
>  }
>  
> +static void dwc_caps(struct dma_chan *chan, struct dma_slave_caps *caps)
> +{
> +
> +}
> +
>  int do_dma_probe(struct dw_dma_chip *chip)
>  {
>  	struct dw_dma *dw = chip->dw;
> @@ -1214,6 +1219,7 @@ int do_dma_probe(struct dw_dma_chip *chip)
>  	dw->dma.device_prep_dma_memcpy = dwc_prep_dma_memcpy;
>  	dw->dma.device_prep_slave_sg = dwc_prep_slave_sg;
>  
> +	dw->dma.device_caps = dwc_caps;
>  	dw->dma.device_config = dwc_config;
>  	dw->dma.device_pause = dwc_pause;
>  	dw->dma.device_resume = dwc_resume;
> -- 
> 2.26.2
> 

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH v7 08/11] dmaengine: dw: Add dummy device_caps callback
  2020-07-10  8:51   ` Andy Shevchenko
@ 2020-07-10  8:52     ` Andy Shevchenko
  2020-07-10  9:45     ` Serge Semin
  1 sibling, 0 replies; 33+ messages in thread
From: Andy Shevchenko @ 2020-07-10  8:52 UTC (permalink / raw)
  To: Serge Semin
  Cc: Vinod Koul, Viresh Kumar, Dan Williams, Serge Semin,
	Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, Rob Herring,
	linux-mips, devicetree, dmaengine, linux-kernel

On Fri, Jul 10, 2020 at 11:51:23AM +0300, Andy Shevchenko wrote:
> On Fri, Jul 10, 2020 at 01:45:47AM +0300, Serge Semin wrote:
> > Since some DW DMA controllers (like one installed on Baikal-T1 SoC) may
> > have non-uniform DMA capabilities per device channels, let's add
> > the DW DMA specific device_caps callback to expose that specifics up to
> > the DMA consumer. It's a dummy function for now. We'll fill it in with
> > capabilities overrides in the next commits.
> 
> Just a reminder (mainly to Vinod) of my view to this.
> Unneeded churn, should be folded to patch 9.

Sorry, s/9/10/ (also the sign that they should be in one, rather than spread
over the series).

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH v7 04/11] dmaengine: Introduce max SG list entries capability
  2020-07-10  8:31   ` Peter Ujfalusi
@ 2020-07-10  9:27     ` Serge Semin
  2020-07-10 11:51       ` Peter Ujfalusi
  0 siblings, 1 reply; 33+ messages in thread
From: Serge Semin @ 2020-07-10  9:27 UTC (permalink / raw)
  To: Peter Ujfalusi
  Cc: Vinod Koul, Viresh Kumar, Dan Williams, Andy Shevchenko,
	Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, Rob Herring,
	linux-mips, devicetree, dmaengine, linux-kernel

Hello Peter

On Fri, Jul 10, 2020 at 11:31:47AM +0300, Peter Ujfalusi wrote:
> 
> 
> On 10/07/2020 1.45, Serge Semin wrote:
> > Some devices may lack the support of the hardware accelerated SG list
> > entries automatic walking through and execution. In this case a burden of
> > the SG list traversal and DMA engine re-initialization lies on the
> > DMA engine driver (normally implemented by using a DMA transfer completion
> > IRQ to recharge the DMA device with a next SG list entry). But such
> > solution may not be suitable for some DMA consumers. In particular SPI
> > devices need both Tx and Rx DMA channels work synchronously in order
> > to avoid the Rx FIFO overflow. In case if Rx DMA channel is paused for
> > some time while the Tx DMA channel works implicitly pulling data into the
> > Rx FIFO, the later will be eventually overflown, which will cause the data
> > loss. So if SG list entries aren't automatically fetched by the DMA
> > engine, but are one-by-one manually selected for execution in the
> > ISRs/deferred work/etc., such problem will eventually happen due to the
> > non-deterministic latencies of the service execution.
> 

> It is not really the number of sg nents which is the problem, but the
> combination of total number of bytes _and_ the number of nents used to
> map them.

No, in the case described above the latency only matters. The length of the SG
entries doesn't (at most of extents). The same is with the nents. If there are
more than one SG entries in the Rx SG-list handled with the non-deterministic
latency, the problem may happen at the moment of any Rx SG entry reload.

(In fact the DW DMAC driver is working with so called Linked-List entries,
which are used to map the SG-list entries into the list of items with length
less than or equal to the max block size the engine supports. But for
simplicity I call them the SG-list entries, since if dma_set_max_seg_size()
is specified the mapping will be uniform.)

> Obviously the TX and RX number of bytes must match in duplex case and at
> the same time neither nents should be over the number of SGs the DMA
> device can handle without interruption (linking, chaining, or whatever
> means).

Right.

> 
> The EDMA from TI have similar limitation (we set the limit to 20 nents).
> Longer lists will be broken up to maximum of 20 segment transfers.
> This setup has bigger impact on audio (cyclic) as we need to limit the
> number of periods to not exceed this limit of 20.

As I said the problem described above isn't about the number of entries, but
about how they are handled. If there is a latency, then the problem may and most
like will eventually happen.

The complexity of the situation is that the DW DMAC driver may split a SG-list
entry up if its length exceeds max block size the engine supports. That's
why in order to fix the problem described in the patch log the DMA client driver
developed needs to take into account the next two points:
1) Detect the maximum number of SG entries the DMA engine can handle without the
non-deterministic latency (you called it max_sg_nents_burst). So the client
drivers would know, that the DMA-channels responsible for Tx and Rx transfers
may be executed with latencies.
2) Make sure the length of each SG-list entry doesn't exceed the max block size
the DW DMAC supports. If some of them does, then the DW DMAC code will break
these SG-entries up into the smaller DMA Linked-list entries, which will get us
back to the re-submission latency problem described in the patch log. This
peculiarity is covered by calling dma_set_max_seg_size() method in the DW DMAC
driver (at least for SPI subsystem it works out-of-box).

> 
> The sDMA on the other hand has different limits. Earlier versions
> without linking support can execute one SG chunk at the time and needs
> to reconfigure for the next one -> max_sg_nents is 1 for the older sDMAs...

Yes, this is our case.

> 
> > In order to let the DMA consumer know about the DMA device capabilities
> > regarding the hardware accelerated SG list traversal we introduce the
> > max_sg_list capability. It is supposed to be initialized by the DMA engine
> > driver with 0 if there is no limitation for the number of SG entries
> > atomically executed and with non-zero value if there is such constraints,
> > so the upper limit is determined by the number set to the property.
> > 
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 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: 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:
> > - This is a new patch created as a result of the discussion with Vinud and
> >   Andy in the framework of DW DMA burst and LLP capabilities.
> > 
> > Changelog v4:
> > - Fix of->if typo. It should be definitely of.
> > ---
> >  drivers/dma/dmaengine.c   | 1 +
> >  include/linux/dmaengine.h | 8 ++++++++
> >  2 files changed, 9 insertions(+)
> > 
> > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> > index b332ffe52780..ad56ad58932c 100644
> > --- a/drivers/dma/dmaengine.c
> > +++ b/drivers/dma/dmaengine.c
> > @@ -592,6 +592,7 @@ int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_caps *caps)
> >  	caps->directions = device->directions;
> >  	caps->min_burst = device->min_burst;
> >  	caps->max_burst = device->max_burst;
> > +	caps->max_sg_nents = device->max_sg_nents;
> >  	caps->residue_granularity = device->residue_granularity;
> >  	caps->descriptor_reuse = device->descriptor_reuse;
> >  	caps->cmd_pause = !!device->device_pause;
> > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> > index 0c7403b27133..a7e4d8dfdd19 100644
> > --- a/include/linux/dmaengine.h
> > +++ b/include/linux/dmaengine.h
> > @@ -467,6 +467,9 @@ enum dma_residue_granularity {
> >   *	should be checked by controller as well
> >   * @min_burst: min burst capability per-transfer
> >   * @max_burst: max burst capability per-transfer
> > + * @max_sg_nents: max number of SG list entries executed in a single atomic
> > + *	DMA tansaction with no intermediate IRQ for reinitialization. Zero
> > + *	value means unlimited number of entries.
> 

> Without looking at the comment the name max_sg_nents implies that the
> DMA can not handle longer lists, but it is not really true.
> max_sg_nents_burst might be a bit cleaner for the first look?

Seems reasonable. I also thought of a better naming so there wouldn't be need to
read a comment in order get a notion what exactly the parameter is responsible
for. Although at the moment of the patchset preparation nothing better got in my
mind.

I like what you suggest, but I'd better make it "max_sg_burst" or "max_sg_chain".
What do you think?

Vinod?
Andy (since you've already acked the patch)?

-Sergey

> 
> >   * @cmd_pause: true, if pause is supported (i.e. for reading residue or
> >   *	       for resume later)
> >   * @cmd_resume: true, if resume is supported
> > @@ -481,6 +484,7 @@ struct dma_slave_caps {
> >  	u32 directions;
> >  	u32 min_burst;
> >  	u32 max_burst;
> > +	u32 max_sg_nents;
> >  	bool cmd_pause;
> >  	bool cmd_resume;
> >  	bool cmd_terminate;
> > @@ -773,6 +777,9 @@ struct dma_filter {
> >   *	should be checked by controller as well
> >   * @min_burst: min burst capability per-transfer
> >   * @max_burst: max burst capability per-transfer
> > + * @max_sg_nents: max number of SG list entries executed in a single atomic
> > + *	DMA tansaction with no intermediate IRQ for reinitialization. Zero
> > + *	value means unlimited number of entries.
> >   * @residue_granularity: granularity of the transfer residue reported
> >   *	by tx_status
> >   * @device_alloc_chan_resources: allocate resources and return the
> > @@ -844,6 +851,7 @@ struct dma_device {
> >  	u32 directions;
> >  	u32 min_burst;
> >  	u32 max_burst;
> > +	u32 max_sg_nents;
> >  	bool descriptor_reuse;
> >  	enum dma_residue_granularity residue_granularity;
> >  
> > 
> 
> - Péter
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 

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

* Re: [PATCH v7 05/11] dmaengine: Introduce DMA-device device_caps callback
  2020-07-10  8:45   ` Andy Shevchenko
@ 2020-07-10  9:38     ` Serge Semin
  2020-07-13  6:51       ` Vinod Koul
  2020-07-13 20:55       ` Dave Jiang
  0 siblings, 2 replies; 33+ messages in thread
From: Serge Semin @ 2020-07-10  9:38 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Vinod Koul, Viresh Kumar, Dan Williams, Alexey Malahov,
	Thomas Bogendoerfer, Arnd Bergmann, Rob Herring, linux-mips,
	devicetree, dmaengine, linux-kernel

On Fri, Jul 10, 2020 at 11:45:03AM +0300, Andy Shevchenko wrote:
> On Fri, Jul 10, 2020 at 01:45:44AM +0300, Serge Semin wrote:
> > There are DMA devices (like ours version of Synopsys DW DMAC) which have
> > DMA capabilities non-uniformly redistributed between the device channels.
> > In order to provide a way of exposing the channel-specific parameters to
> > the DMA engine consumers, we introduce a new DMA-device callback. In case
> > if provided it gets called from the dma_get_slave_caps() method and is
> > able to override the generic DMA-device capabilities.
> 

> In light of recent developments consider not to add 'slave' and a such words to the kernel.

As long as the 'slave' word is used in the name of the dma_slave_caps
structure and in the rest of the DMA-engine subsystem, it will be ambiguous
to use some else terminology. If renaming needs to be done, then it should be
done synchronously for the whole subsystem.

-Sergey

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

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

* Re: [PATCH v7 08/11] dmaengine: dw: Add dummy device_caps callback
  2020-07-10  8:51   ` Andy Shevchenko
  2020-07-10  8:52     ` Andy Shevchenko
@ 2020-07-10  9:45     ` Serge Semin
  2020-07-15 12:01       ` Vinod Koul
  1 sibling, 1 reply; 33+ messages in thread
From: Serge Semin @ 2020-07-10  9:45 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Vinod Koul, Viresh Kumar, Dan Williams, Alexey Malahov,
	Thomas Bogendoerfer, Arnd Bergmann, Rob Herring, linux-mips,
	devicetree, dmaengine, linux-kernel

On Fri, Jul 10, 2020 at 11:51:23AM +0300, Andy Shevchenko wrote:
> On Fri, Jul 10, 2020 at 01:45:47AM +0300, Serge Semin wrote:
> > Since some DW DMA controllers (like one installed on Baikal-T1 SoC) may
> > have non-uniform DMA capabilities per device channels, let's add
> > the DW DMA specific device_caps callback to expose that specifics up to
> > the DMA consumer. It's a dummy function for now. We'll fill it in with
> > capabilities overrides in the next commits.
> 

> Just a reminder (mainly to Vinod) of my view to this.
> Unneeded churn, should be folded to patch 9.

Just to remind (mainly to Vinod). That's Andy's bikeshedding.
This isn't a churn, since it's totally normal to design the patchset in this way:
introduce a callback, then fill it in with functionality.

-Sergey

> 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > 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:
> > - This is a new patch created as a result of the discussion with Vinud and
> >   Andy in the framework of DW DMA burst and LLP capabilities.
> > ---
> >  drivers/dma/dw/core.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
> > index fb95920c429e..ceded21537e2 100644
> > --- a/drivers/dma/dw/core.c
> > +++ b/drivers/dma/dw/core.c
> > @@ -1049,6 +1049,11 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
> >  	dev_vdbg(chan2dev(chan), "%s: done\n", __func__);
> >  }
> >  
> > +static void dwc_caps(struct dma_chan *chan, struct dma_slave_caps *caps)
> > +{
> > +
> > +}
> > +
> >  int do_dma_probe(struct dw_dma_chip *chip)
> >  {
> >  	struct dw_dma *dw = chip->dw;
> > @@ -1214,6 +1219,7 @@ int do_dma_probe(struct dw_dma_chip *chip)
> >  	dw->dma.device_prep_dma_memcpy = dwc_prep_dma_memcpy;
> >  	dw->dma.device_prep_slave_sg = dwc_prep_slave_sg;
> >  
> > +	dw->dma.device_caps = dwc_caps;
> >  	dw->dma.device_config = dwc_config;
> >  	dw->dma.device_pause = dwc_pause;
> >  	dw->dma.device_resume = dwc_resume;
> > -- 
> > 2.26.2
> > 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

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

* Re: [PATCH v7 04/11] dmaengine: Introduce max SG list entries capability
  2020-07-10  9:27     ` Serge Semin
@ 2020-07-10 11:51       ` Peter Ujfalusi
  2020-07-10 16:14         ` Serge Semin
  0 siblings, 1 reply; 33+ messages in thread
From: Peter Ujfalusi @ 2020-07-10 11:51 UTC (permalink / raw)
  To: Serge Semin
  Cc: Vinod Koul, Viresh Kumar, Dan Williams, Andy Shevchenko,
	Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, Rob Herring,
	linux-mips, devicetree, dmaengine, linux-kernel

Hi Sergey,

On 10/07/2020 12.27, Serge Semin wrote:
> Hello Peter
> 
> On Fri, Jul 10, 2020 at 11:31:47AM +0300, Peter Ujfalusi wrote:
>>
>>
>> On 10/07/2020 1.45, Serge Semin wrote:
>>> Some devices may lack the support of the hardware accelerated SG list
>>> entries automatic walking through and execution. In this case a burden of
>>> the SG list traversal and DMA engine re-initialization lies on the
>>> DMA engine driver (normally implemented by using a DMA transfer completion
>>> IRQ to recharge the DMA device with a next SG list entry). But such
>>> solution may not be suitable for some DMA consumers. In particular SPI
>>> devices need both Tx and Rx DMA channels work synchronously in order
>>> to avoid the Rx FIFO overflow. In case if Rx DMA channel is paused for
>>> some time while the Tx DMA channel works implicitly pulling data into the
>>> Rx FIFO, the later will be eventually overflown, which will cause the data
>>> loss. So if SG list entries aren't automatically fetched by the DMA
>>> engine, but are one-by-one manually selected for execution in the
>>> ISRs/deferred work/etc., such problem will eventually happen due to the
>>> non-deterministic latencies of the service execution.
>>
> 
>> It is not really the number of sg nents which is the problem, but the
>> combination of total number of bytes _and_ the number of nents used to
>> map them.
> 
> No, in the case described above the latency only matters.

Sure, the latency comes in (at least for EDMA) when moving from one
sub-sg_list to the next one when needing to break the long list into
smaller chunks. In the sub-completion we need to move to the next chunk.

> The length of the SG
> entries doesn't (at most of extents). The same is with the nents. If there are
> more than one SG entries in the Rx SG-list handled with the non-deterministic
> latency, the problem may happen at the moment of any Rx SG entry reload.

You have latency in the hardware when progressing between the sg entries?

> (In fact the DW DMAC driver is working with so called Linked-List entries,
> which are used to map the SG-list entries into the list of items with length
> less than or equal to the max block size the engine supports. But for
> simplicity I call them the SG-list entries, since if dma_set_max_seg_size()
> is specified the mapping will be uniform.)

Right, I'll come back to the dma_set_max_seq_size() later for my cases ;)

>> The EDMA from TI have similar limitation (we set the limit to 20 nents).
>> Longer lists will be broken up to maximum of 20 segment transfers.
>> This setup has bigger impact on audio (cyclic) as we need to limit the
>> number of periods to not exceed this limit of 20.
> 
> As I said the problem described above isn't about the number of entries, but
> about how they are handled. If there is a latency, then the problem may and most
> like will eventually happen.
> 
> The complexity of the situation is that the DW DMAC driver may split a SG-list
> entry up if its length exceeds max block size the engine supports. That's
> why in order to fix the problem described in the patch log the DMA client driver
> developed needs to take into account the next two points:
> 1) Detect the maximum number of SG entries the DMA engine can handle without the
> non-deterministic latency (you called it max_sg_nents_burst). So the client
> drivers would know, that the DMA-channels responsible for Tx and Rx transfers
> may be executed with latencies.

This is really useful information for the clients. But in some cases
they just don't care how the list is going to be processed, given if
they have large enough internal FIFO to hold on while the DMA moves from
one chunk to another.

> 2) Make sure the length of each SG-list entry doesn't exceed the max block size
> the DW DMAC supports. If some of them does, then the DW DMAC code will break
> these SG-entries up into the smaller DMA Linked-list entries, which will get us
> back to the re-submission latency problem described in the patch log. This
> peculiarity is covered by calling dma_set_max_seg_size() method in the DW DMAC
> driver (at least for SPI subsystem it works out-of-box).

This is what does not work for me for neither EDMA or sDMA... The
max_seq_size depends on the src/dst_addr_width the peripheral is
configured. It is different for each DMA_SLAVE_BUSWIDTH_*_BYTES _and_
also for the burst used.

It is on my to-do list to clean up and send this:
https://github.com/omap-audio/linux-audio/commit/dd81de1a343810468b6e7a601e90a9161f46cab1

For EDMA the implementation is:
https://github.com/omap-audio/linux-audio/commit/ee93c45667d0f250baa1e224e5d1266fae2e7c80

along the same lines for sDMA:
https://github.com/omap-audio/linux-audio/commit/d6ac68692efcdac8ec233c70b802d37f594d6d4d

What is obviously missing is that if the driver does not implement the
device_get_max_len, then dma_get_max_seg_size() of the device should be
returned.

Other option would be:
https://github.com/omap-audio/linux-audio/commit/95dda19c92a38437741850309374a2ca4ef1e361

I'm still hesitating which would be more generic to be acceptable.

And to make things a bit more interesting the UDMA have also different
dma_sg_len() supported depending on what mode the channel is used (TR or
packet mode), but that's another story in itself.

>> The sDMA on the other hand has different limits. Earlier versions
>> without linking support can execute one SG chunk at the time and needs
>> to reconfigure for the next one -> max_sg_nents is 1 for the older sDMAs...
> 
> Yes, this is our case.

Interesting. So you prefer that your clients will use
dmaengine_prep_slave_single() if possible?

>>> In order to let the DMA consumer know about the DMA device capabilities
>>> regarding the hardware accelerated SG list traversal we introduce the
>>> max_sg_list capability. It is supposed to be initialized by the DMA engine
>>> driver with 0 if there is no limitation for the number of SG entries
>>> atomically executed and with non-zero value if there is such constraints,
>>> so the upper limit is determined by the number set to the property.
>>>
>>> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> 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: 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:
>>> - This is a new patch created as a result of the discussion with Vinud and
>>>   Andy in the framework of DW DMA burst and LLP capabilities.
>>>
>>> Changelog v4:
>>> - Fix of->if typo. It should be definitely of.
>>> ---
>>>  drivers/dma/dmaengine.c   | 1 +
>>>  include/linux/dmaengine.h | 8 ++++++++
>>>  2 files changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
>>> index b332ffe52780..ad56ad58932c 100644
>>> --- a/drivers/dma/dmaengine.c
>>> +++ b/drivers/dma/dmaengine.c
>>> @@ -592,6 +592,7 @@ int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_caps *caps)
>>>  	caps->directions = device->directions;
>>>  	caps->min_burst = device->min_burst;
>>>  	caps->max_burst = device->max_burst;
>>> +	caps->max_sg_nents = device->max_sg_nents;
>>>  	caps->residue_granularity = device->residue_granularity;
>>>  	caps->descriptor_reuse = device->descriptor_reuse;
>>>  	caps->cmd_pause = !!device->device_pause;
>>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>>> index 0c7403b27133..a7e4d8dfdd19 100644
>>> --- a/include/linux/dmaengine.h
>>> +++ b/include/linux/dmaengine.h
>>> @@ -467,6 +467,9 @@ enum dma_residue_granularity {
>>>   *	should be checked by controller as well
>>>   * @min_burst: min burst capability per-transfer
>>>   * @max_burst: max burst capability per-transfer
>>> + * @max_sg_nents: max number of SG list entries executed in a single atomic
>>> + *	DMA tansaction with no intermediate IRQ for reinitialization. Zero
>>> + *	value means unlimited number of entries.
>>
> 
>> Without looking at the comment the name max_sg_nents implies that the
>> DMA can not handle longer lists, but it is not really true.
>> max_sg_nents_burst might be a bit cleaner for the first look?
> 
> Seems reasonable. I also thought of a better naming so there wouldn't be need to
> read a comment in order get a notion what exactly the parameter is responsible
> for. Although at the moment of the patchset preparation nothing better got in my
> mind.
> 
> I like what you suggest, but I'd better make it "max_sg_burst" or "max_sg_chain".
> What do you think?

Since we should be able to handle longer lists and this is kind of a
hint for clients that above this number of nents the list will be broken
up to smaller 'bursts', which when traversing could cause latency.

sg_chunk_len might be another candidate.

> Vinod?
> Andy (since you've already acked the patch)?
> 
> -Sergey

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki


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

* Re: [PATCH v7 04/11] dmaengine: Introduce max SG list entries capability
  2020-07-10 11:51       ` Peter Ujfalusi
@ 2020-07-10 16:14         ` Serge Semin
  2020-07-15 11:13           ` Vinod Koul
  0 siblings, 1 reply; 33+ messages in thread
From: Serge Semin @ 2020-07-10 16:14 UTC (permalink / raw)
  To: Peter Ujfalusi
  Cc: Serge Semin, Vinod Koul, Viresh Kumar, Dan Williams,
	Andy Shevchenko, Alexey Malahov, Thomas Bogendoerfer,
	Arnd Bergmann, Rob Herring, linux-mips, devicetree, dmaengine,
	linux-kernel

On Fri, Jul 10, 2020 at 02:51:33PM +0300, Peter Ujfalusi wrote:
> Hi Sergey,
> 
> On 10/07/2020 12.27, Serge Semin wrote:
> > Hello Peter
> > 
> > On Fri, Jul 10, 2020 at 11:31:47AM +0300, Peter Ujfalusi wrote:
> >>
> >>
> >> On 10/07/2020 1.45, Serge Semin wrote:
> >>> Some devices may lack the support of the hardware accelerated SG list
> >>> entries automatic walking through and execution. In this case a burden of
> >>> the SG list traversal and DMA engine re-initialization lies on the
> >>> DMA engine driver (normally implemented by using a DMA transfer completion
> >>> IRQ to recharge the DMA device with a next SG list entry). But such
> >>> solution may not be suitable for some DMA consumers. In particular SPI
> >>> devices need both Tx and Rx DMA channels work synchronously in order
> >>> to avoid the Rx FIFO overflow. In case if Rx DMA channel is paused for
> >>> some time while the Tx DMA channel works implicitly pulling data into the
> >>> Rx FIFO, the later will be eventually overflown, which will cause the data
> >>> loss. So if SG list entries aren't automatically fetched by the DMA
> >>> engine, but are one-by-one manually selected for execution in the
> >>> ISRs/deferred work/etc., such problem will eventually happen due to the
> >>> non-deterministic latencies of the service execution.
> >>
> > 
> >> It is not really the number of sg nents which is the problem, but the
> >> combination of total number of bytes _and_ the number of nents used to
> >> map them.
> > 
> > No, in the case described above the latency only matters.
> 

> Sure, the latency comes in (at least for EDMA) when moving from one
> sub-sg_list to the next one when needing to break the long list into
> smaller chunks. In the sub-completion we need to move to the next chunk.

Right. It's crucial. In our case "sub-sg_list" is just a single SG-list entry
because hardware-accelerated Linked-list traversal is unsupported. AFAIU in your
case the sub-sg_list might contain several entries.

> 
> > The length of the SG
> > entries doesn't (at most of extents). The same is with the nents. If there are
> > more than one SG entries in the Rx SG-list handled with the non-deterministic
> > latency, the problem may happen at the moment of any Rx SG entry reload.
> 

> You have latency in the hardware when progressing between the sg entries?

Since there is no support of hardware accelerated SG-list traversal in our
DMA engine, then I can't give you an answer to that question. My message was
that due to the lack of the automatic SG-list entries reloading, each SG-list
entry is re-submitted one-by-one by the DW DMAC driver by means of the
IRQ/tasklet service routines (AFAIU in your case the re-submission is done
for each sub-sg_list). IRQs/tasklet handling may cause the non-deterministic
latency in case if the system is heavy loaded with some other activity. That
latency is dangerous in case if the Tx and Rx data flows are inter-dependent.
In case of SPI If an Rx DMA channel doesn't start fetching data from an SPI Rx
FIFO on time, the later one might get to be overflown eventually.

On the other hand theoretically some latency may still persist even for DMA
engines with hardware accelerated SG-list traversal. That latency can be caused
by an intensive high-priority traffic on the memory-bus generated by CPUs, other
devices or DMAs. But since we haven't got any reports about problems concerning
it so far, then most likely that latency is negligible small. So I assume
that if hardware accelerated SG-list traversal is supported, then the
DMAengine-based operations are working well for any case.

> 
> > (In fact the DW DMAC driver is working with so called Linked-List entries,
> > which are used to map the SG-list entries into the list of items with length
> > less than or equal to the max block size the engine supports. But for
> > simplicity I call them the SG-list entries, since if dma_set_max_seg_size()
> > is specified the mapping will be uniform.)
> 

> Right, I'll come back to the dma_set_max_seq_size() later for my cases ;)

For reference see the patch "[PATCH v7 07/11] dmaengine: dw: Set DMA device max
segment size parameter" in this series. Please also note, that in order to have
the uniform mapping, the DMA client must take into account the max segment size
when creating the SG-list entries.

> 
> >> The EDMA from TI have similar limitation (we set the limit to 20 nents).
> >> Longer lists will be broken up to maximum of 20 segment transfers.
> >> This setup has bigger impact on audio (cyclic) as we need to limit the
> >> number of periods to not exceed this limit of 20.
> > 
> > As I said the problem described above isn't about the number of entries, but
> > about how they are handled. If there is a latency, then the problem may and most
> > like will eventually happen.
> > 
> > The complexity of the situation is that the DW DMAC driver may split a SG-list
> > entry up if its length exceeds max block size the engine supports. That's
> > why in order to fix the problem described in the patch log the DMA client driver
> > developed needs to take into account the next two points:
> > 1) Detect the maximum number of SG entries the DMA engine can handle without the
> > non-deterministic latency (you called it max_sg_nents_burst). So the client
> > drivers would know, that the DMA-channels responsible for Tx and Rx transfers
> > may be executed with latencies.
> 

> This is really useful information for the clients. But in some cases
> they just don't care how the list is going to be processed, given if
> they have large enough internal FIFO to hold on while the DMA moves from
> one chunk to another.

Well, I'd say, not in some but in most of the cases they don't care. The problem
may happen if the DMA-engine is relatively slow to handle in incoming data at
required pace or a slave-device is relatively slow or the slave-device FIFO is
too small. Alas in our case all of those factors exist. The DMA-engine is
clocked with 50MHz, the SPI-bus is clocked with up to 25MHz. That's the
combination when the problem happens very often. The less the SPI-bus speed the
less often the Rx FIFO overflow happens.

> 
> > 2) Make sure the length of each SG-list entry doesn't exceed the max block size
> > the DW DMAC supports. If some of them does, then the DW DMAC code will break
> > these SG-entries up into the smaller DMA Linked-list entries, which will get us
> > back to the re-submission latency problem described in the patch log. This
> > peculiarity is covered by calling dma_set_max_seg_size() method in the DW DMAC
> > driver (at least for SPI subsystem it works out-of-box).
> 

> This is what does not work for me for neither EDMA or sDMA... The
> max_seq_size depends on the src/dst_addr_width the peripheral is
> configured. It is different for each DMA_SLAVE_BUSWIDTH_*_BYTES _and_
> also for the burst used.

Firstly noone prevents a DMA client from creating SG-list entries with length
greater than the max_seg_size value and the DMA-engine driver shall correctly
handle such SG-lists. In our case the max segment size in <bytes> also depends
on the addr-width parameter (MAX SG-list entry length in fact is (add_width *
max_block_size) bytes), but setting max_seg_size for smallest resolution
(addr_width = 1 byte) will keep us on a safe side for any case. No matter with
what address-width the DMA-engine channel is configured If a DMA client submits
a buffer with length less the max_seg_size it will always be handled as a
single Linked-List entry.

> 
> It is on my to-do list to clean up and send this:
> https://github.com/omap-audio/linux-audio/commit/dd81de1a343810468b6e7a601e90a9161f46cab1
> 
> For EDMA the implementation is:
> https://github.com/omap-audio/linux-audio/commit/ee93c45667d0f250baa1e224e5d1266fae2e7c80
> 
> along the same lines for sDMA:
> https://github.com/omap-audio/linux-audio/commit/d6ac68692efcdac8ec233c70b802d37f594d6d4d
> 
> What is obviously missing is that if the driver does not implement the
> device_get_max_len, then dma_get_max_seg_size() of the device should be
> returned.
> 
> Other option would be:
> https://github.com/omap-audio/linux-audio/commit/95dda19c92a38437741850309374a2ca4ef1e361
> 
> I'm still hesitating which would be more generic to be acceptable.
> 
> And to make things a bit more interesting the UDMA have also different
> dma_sg_len() supported depending on what mode the channel is used (TR or
> packet mode), but that's another story in itself.
> 
> >> The sDMA on the other hand has different limits. Earlier versions
> >> without linking support can execute one SG chunk at the time and needs
> >> to reconfigure for the next one -> max_sg_nents is 1 for the older sDMAs...
> > 
> > Yes, this is our case.
> 

> Interesting. So you prefer that your clients will use
> dmaengine_prep_slave_single() if possible?

Yes. I just don't see another way to solve the problem. (Please see the history of
this patchset review, links to the biggest part of which I've collected in the
cover-letter. We pretty much thoroughly discussed it with Andy, Vinod and Viresh).
The DW APB SSI driver (DW DMAC engine client) will synchronously walk over the
SG-lists passed for Tx and Rx transfers and re-charge the DMA-engine channels with
each entry. (The solution will be a bit more complicated since the Tx and Rx SG-lists
might have different number of entries, but in particular case it will look as I
described).

> 
> >>> In order to let the DMA consumer know about the DMA device capabilities
> >>> regarding the hardware accelerated SG list traversal we introduce the
> >>> max_sg_list capability. It is supposed to be initialized by the DMA engine
> >>> driver with 0 if there is no limitation for the number of SG entries
> >>> atomically executed and with non-zero value if there is such constraints,
> >>> so the upper limit is determined by the number set to the property.
> >>>
> >>> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >>> 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: 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:
> >>> - This is a new patch created as a result of the discussion with Vinud and
> >>>   Andy in the framework of DW DMA burst and LLP capabilities.
> >>>
> >>> Changelog v4:
> >>> - Fix of->if typo. It should be definitely of.
> >>> ---
> >>>  drivers/dma/dmaengine.c   | 1 +
> >>>  include/linux/dmaengine.h | 8 ++++++++
> >>>  2 files changed, 9 insertions(+)
> >>>
> >>> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> >>> index b332ffe52780..ad56ad58932c 100644
> >>> --- a/drivers/dma/dmaengine.c
> >>> +++ b/drivers/dma/dmaengine.c
> >>> @@ -592,6 +592,7 @@ int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_caps *caps)
> >>>  	caps->directions = device->directions;
> >>>  	caps->min_burst = device->min_burst;
> >>>  	caps->max_burst = device->max_burst;
> >>> +	caps->max_sg_nents = device->max_sg_nents;
> >>>  	caps->residue_granularity = device->residue_granularity;
> >>>  	caps->descriptor_reuse = device->descriptor_reuse;
> >>>  	caps->cmd_pause = !!device->device_pause;
> >>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> >>> index 0c7403b27133..a7e4d8dfdd19 100644
> >>> --- a/include/linux/dmaengine.h
> >>> +++ b/include/linux/dmaengine.h
> >>> @@ -467,6 +467,9 @@ enum dma_residue_granularity {
> >>>   *	should be checked by controller as well
> >>>   * @min_burst: min burst capability per-transfer
> >>>   * @max_burst: max burst capability per-transfer
> >>> + * @max_sg_nents: max number of SG list entries executed in a single atomic
> >>> + *	DMA tansaction with no intermediate IRQ for reinitialization. Zero
> >>> + *	value means unlimited number of entries.
> >>
> > 
> >> Without looking at the comment the name max_sg_nents implies that the
> >> DMA can not handle longer lists, but it is not really true.
> >> max_sg_nents_burst might be a bit cleaner for the first look?
> > 
> > Seems reasonable. I also thought of a better naming so there wouldn't be need to
> > read a comment in order get a notion what exactly the parameter is responsible
> > for. Although at the moment of the patchset preparation nothing better got in my
> > mind.
> > 
> > I like what you suggest, but I'd better make it "max_sg_burst" or "max_sg_chain".
> > What do you think?
> 

> Since we should be able to handle longer lists and this is kind of a
> hint for clients that above this number of nents the list will be broken
> up to smaller 'bursts', which when traversing could cause latency.
> 
> sg_chunk_len might be another candidate.

Ok. We've got four candidates:
- max_sg_nents_burst
- max_sg_burst
- max_sg_chain
- sg_chunk_len

@Vinod, @Andy, what do you think?

-Sergey

> 
> > Vinod?
> > Andy (since you've already acked the patch)?
> > 
> > -Sergey
> 
> - Péter
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 

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

* Re: [PATCH v7 05/11] dmaengine: Introduce DMA-device device_caps callback
  2020-07-10  9:38     ` Serge Semin
@ 2020-07-13  6:51       ` Vinod Koul
  2020-07-13 20:13         ` Serge Semin
  2020-07-13 20:55       ` Dave Jiang
  1 sibling, 1 reply; 33+ messages in thread
From: Vinod Koul @ 2020-07-13  6:51 UTC (permalink / raw)
  To: Serge Semin
  Cc: Andy Shevchenko, Viresh Kumar, Dan Williams, Alexey Malahov,
	Thomas Bogendoerfer, Arnd Bergmann, Rob Herring, linux-mips,
	devicetree, dmaengine, linux-kernel

On 10-07-20, 12:38, Serge Semin wrote:
> On Fri, Jul 10, 2020 at 11:45:03AM +0300, Andy Shevchenko wrote:
> > On Fri, Jul 10, 2020 at 01:45:44AM +0300, Serge Semin wrote:
> > > There are DMA devices (like ours version of Synopsys DW DMAC) which have
> > > DMA capabilities non-uniformly redistributed between the device channels.
> > > In order to provide a way of exposing the channel-specific parameters to
> > > the DMA engine consumers, we introduce a new DMA-device callback. In case
> > > if provided it gets called from the dma_get_slave_caps() method and is
> > > able to override the generic DMA-device capabilities.
> > 
> 
> > In light of recent developments consider not to add 'slave' and a such words to the kernel.
> 
> As long as the 'slave' word is used in the name of the dma_slave_caps
> structure and in the rest of the DMA-engine subsystem, it will be ambiguous
> to use some else terminology. If renaming needs to be done, then it should be
> done synchronously for the whole subsystem.

Right, I have plans to tackle that during next merge window and have
started changes. Thankfully slave_dma can be replaced by peripheral dma
easily. But getting that in would be tricky as we need to change users
too.

-- 
~Vinod

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

* Re: [PATCH v7 05/11] dmaengine: Introduce DMA-device device_caps callback
  2020-07-13  6:51       ` Vinod Koul
@ 2020-07-13 20:13         ` Serge Semin
  0 siblings, 0 replies; 33+ messages in thread
From: Serge Semin @ 2020-07-13 20:13 UTC (permalink / raw)
  To: Vinod Koul
  Cc: Serge Semin, Andy Shevchenko, Viresh Kumar, Dan Williams,
	Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, Rob Herring,
	linux-mips, devicetree, dmaengine, linux-kernel

Hello Vinod,
  
Could you please keep on this patchset review? Really, the patchset isn't that
big and complicated to be working on it for such a long time. I've sent it out
at the time of the kernel 5.6. I've considered all the Andy's comments since
then. There is going to be 5.9 merge window soon, but the patchset still under
review procedure, while I still have some work, which depends on the changes
provided by this patchset. It would be great to at least submit it for review
before the next merge window, and super-great have it merged in before that.

There is a Peter Ujfalusi comment to the patch
"[PATCH v7 04/11] dmaengine: Introduce max SG list entries capability", which
needs your attention. Could you please take a look at that? So I could submit
the next patchset revision if you agree with the Peter' suggestion.

-Sergey

On Mon, Jul 13, 2020 at 12:21:31PM +0530, Vinod Koul wrote:
> On 10-07-20, 12:38, Serge Semin wrote:
> > On Fri, Jul 10, 2020 at 11:45:03AM +0300, Andy Shevchenko wrote:
> > > On Fri, Jul 10, 2020 at 01:45:44AM +0300, Serge Semin wrote:
> > > > There are DMA devices (like ours version of Synopsys DW DMAC) which have
> > > > DMA capabilities non-uniformly redistributed between the device channels.
> > > > In order to provide a way of exposing the channel-specific parameters to
> > > > the DMA engine consumers, we introduce a new DMA-device callback. In case
> > > > if provided it gets called from the dma_get_slave_caps() method and is
> > > > able to override the generic DMA-device capabilities.
> > > 
> > 
> > > In light of recent developments consider not to add 'slave' and a such words to the kernel.
> > 
> > As long as the 'slave' word is used in the name of the dma_slave_caps
> > structure and in the rest of the DMA-engine subsystem, it will be ambiguous
> > to use some else terminology. If renaming needs to be done, then it should be
> > done synchronously for the whole subsystem.
> 
> Right, I have plans to tackle that during next merge window and have
> started changes. Thankfully slave_dma can be replaced by peripheral dma
> easily. But getting that in would be tricky as we need to change users
> too.
> 
> -- 
> ~Vinod

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

* Re: [PATCH v7 05/11] dmaengine: Introduce DMA-device device_caps callback
  2020-07-10  9:38     ` Serge Semin
  2020-07-13  6:51       ` Vinod Koul
@ 2020-07-13 20:55       ` Dave Jiang
  2020-07-14 16:08         ` Vinod Koul
  1 sibling, 1 reply; 33+ messages in thread
From: Dave Jiang @ 2020-07-13 20:55 UTC (permalink / raw)
  To: Serge Semin, Andy Shevchenko
  Cc: Vinod Koul, Viresh Kumar, Dan Williams, Alexey Malahov,
	Thomas Bogendoerfer, Arnd Bergmann, Rob Herring, linux-mips,
	devicetree, dmaengine, linux-kernel



On 7/10/2020 2:38 AM, Serge Semin wrote:
> On Fri, Jul 10, 2020 at 11:45:03AM +0300, Andy Shevchenko wrote:
>> On Fri, Jul 10, 2020 at 01:45:44AM +0300, Serge Semin wrote:
>>> There are DMA devices (like ours version of Synopsys DW DMAC) which have
>>> DMA capabilities non-uniformly redistributed between the device channels.
>>> In order to provide a way of exposing the channel-specific parameters to
>>> the DMA engine consumers, we introduce a new DMA-device callback. In case
>>> if provided it gets called from the dma_get_slave_caps() method and is
>>> able to override the generic DMA-device capabilities.
>>
> 
>> In light of recent developments consider not to add 'slave' and a such words to the kernel.
> 
> As long as the 'slave' word is used in the name of the dma_slave_caps
> structure and in the rest of the DMA-engine subsystem, it will be ambiguous
> to use some else terminology. If renaming needs to be done, then it should be
> done synchronously for the whole subsystem.

What about just calling it dma_device_caps? Consider this is a useful function 
not only slave DMA will utilize this. I can see this being useful for some of my 
future code with idxd driver.

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

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

* Re: [PATCH v7 05/11] dmaengine: Introduce DMA-device device_caps callback
  2020-07-13 20:55       ` Dave Jiang
@ 2020-07-14 16:08         ` Vinod Koul
  2020-07-14 16:18           ` Dave Jiang
  0 siblings, 1 reply; 33+ messages in thread
From: Vinod Koul @ 2020-07-14 16:08 UTC (permalink / raw)
  To: Dave Jiang
  Cc: Serge Semin, Andy Shevchenko, Viresh Kumar, Dan Williams,
	Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, Rob Herring,
	linux-mips, devicetree, dmaengine, linux-kernel

On 13-07-20, 13:55, Dave Jiang wrote:
> 
> 
> On 7/10/2020 2:38 AM, Serge Semin wrote:
> > On Fri, Jul 10, 2020 at 11:45:03AM +0300, Andy Shevchenko wrote:
> > > On Fri, Jul 10, 2020 at 01:45:44AM +0300, Serge Semin wrote:
> > > > There are DMA devices (like ours version of Synopsys DW DMAC) which have
> > > > DMA capabilities non-uniformly redistributed between the device channels.
> > > > In order to provide a way of exposing the channel-specific parameters to
> > > > the DMA engine consumers, we introduce a new DMA-device callback. In case
> > > > if provided it gets called from the dma_get_slave_caps() method and is
> > > > able to override the generic DMA-device capabilities.
> > > 
> > 
> > > In light of recent developments consider not to add 'slave' and a such words to the kernel.
> > 
> > As long as the 'slave' word is used in the name of the dma_slave_caps
> > structure and in the rest of the DMA-engine subsystem, it will be ambiguous
> > to use some else terminology. If renaming needs to be done, then it should be
> > done synchronously for the whole subsystem.
> 
> What about just calling it dma_device_caps? Consider this is a useful
> function not only slave DMA will utilize this. I can see this being useful
> for some of my future code with idxd driver.

Some of the caps may make sense to generic dmaengine but few of them do
not :) While at it, am planning to make it dmaengine_periph_caps to
denote that these are dmaengine peripheral capabilities.

-- 
~Vinod

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

* Re: [PATCH v7 05/11] dmaengine: Introduce DMA-device device_caps callback
  2020-07-14 16:08         ` Vinod Koul
@ 2020-07-14 16:18           ` Dave Jiang
  2020-07-14 16:29             ` Serge Semin
  0 siblings, 1 reply; 33+ messages in thread
From: Dave Jiang @ 2020-07-14 16:18 UTC (permalink / raw)
  To: Vinod Koul
  Cc: Serge Semin, Andy Shevchenko, Viresh Kumar, Dan Williams,
	Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, Rob Herring,
	linux-mips, devicetree, dmaengine, linux-kernel



On 7/14/2020 9:08 AM, Vinod Koul wrote:
> On 13-07-20, 13:55, Dave Jiang wrote:
>>
>>
>> On 7/10/2020 2:38 AM, Serge Semin wrote:
>>> On Fri, Jul 10, 2020 at 11:45:03AM +0300, Andy Shevchenko wrote:
>>>> On Fri, Jul 10, 2020 at 01:45:44AM +0300, Serge Semin wrote:
>>>>> There are DMA devices (like ours version of Synopsys DW DMAC) which have
>>>>> DMA capabilities non-uniformly redistributed between the device channels.
>>>>> In order to provide a way of exposing the channel-specific parameters to
>>>>> the DMA engine consumers, we introduce a new DMA-device callback. In case
>>>>> if provided it gets called from the dma_get_slave_caps() method and is
>>>>> able to override the generic DMA-device capabilities.
>>>>
>>>
>>>> In light of recent developments consider not to add 'slave' and a such words to the kernel.
>>>
>>> As long as the 'slave' word is used in the name of the dma_slave_caps
>>> structure and in the rest of the DMA-engine subsystem, it will be ambiguous
>>> to use some else terminology. If renaming needs to be done, then it should be
>>> done synchronously for the whole subsystem.
>>
>> What about just calling it dma_device_caps? Consider this is a useful
>> function not only slave DMA will utilize this. I can see this being useful
>> for some of my future code with idxd driver.
> 
> Some of the caps may make sense to generic dmaengine but few of them do
> not :) While at it, am planning to make it dmaengine_periph_caps to
> denote that these are dmaengine peripheral capabilities.
> 

If the function only passes in periph_caps, how do we allow the non periph DMA 
utilize this function?

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

* Re: [PATCH v7 05/11] dmaengine: Introduce DMA-device device_caps callback
  2020-07-14 16:18           ` Dave Jiang
@ 2020-07-14 16:29             ` Serge Semin
  2020-07-14 16:49               ` Dave Jiang
  0 siblings, 1 reply; 33+ messages in thread
From: Serge Semin @ 2020-07-14 16:29 UTC (permalink / raw)
  To: Dave Jiang
  Cc: Vinod Koul, Andy Shevchenko, Viresh Kumar, Dan Williams,
	Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, Rob Herring,
	linux-mips, devicetree, dmaengine, linux-kernel

On Tue, Jul 14, 2020 at 09:18:16AM -0700, Dave Jiang wrote:
> 
> 
> On 7/14/2020 9:08 AM, Vinod Koul wrote:
> > On 13-07-20, 13:55, Dave Jiang wrote:
> > > 
> > > 
> > > On 7/10/2020 2:38 AM, Serge Semin wrote:
> > > > On Fri, Jul 10, 2020 at 11:45:03AM +0300, Andy Shevchenko wrote:
> > > > > On Fri, Jul 10, 2020 at 01:45:44AM +0300, Serge Semin wrote:
> > > > > > There are DMA devices (like ours version of Synopsys DW DMAC) which have
> > > > > > DMA capabilities non-uniformly redistributed between the device channels.
> > > > > > In order to provide a way of exposing the channel-specific parameters to
> > > > > > the DMA engine consumers, we introduce a new DMA-device callback. In case
> > > > > > if provided it gets called from the dma_get_slave_caps() method and is
> > > > > > able to override the generic DMA-device capabilities.
> > > > > 
> > > > 
> > > > > In light of recent developments consider not to add 'slave' and a such words to the kernel.
> > > > 
> > > > As long as the 'slave' word is used in the name of the dma_slave_caps
> > > > structure and in the rest of the DMA-engine subsystem, it will be ambiguous
> > > > to use some else terminology. If renaming needs to be done, then it should be
> > > > done synchronously for the whole subsystem.
> > > 
> > > What about just calling it dma_device_caps? Consider this is a useful
> > > function not only slave DMA will utilize this. I can see this being useful
> > > for some of my future code with idxd driver.
> > 
> > Some of the caps may make sense to generic dmaengine but few of them do
> > not :) While at it, am planning to make it dmaengine_periph_caps to
> > denote that these are dmaengine peripheral capabilities.
> > 
> 

> If the function only passes in periph_caps, how do we allow the non periph
> DMA utilize this function?

Hello Dave. That seems reasonable. "dma_device_caps" or even "dma_chan_caps"
might be more suitable seeing after this patchset merged in the "dma_slave_caps"
may really provide the DMA channel-specific configs. Moreover that structure is
accessible only by means of the dma_chan descriptor:

int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_caps *caps);

which makes those caps being the channel-specific even without this patchset.

So as I see it "dma_chan_caps" might be the better choice.

-Sergey

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

* Re: [PATCH v7 05/11] dmaengine: Introduce DMA-device device_caps callback
  2020-07-14 16:29             ` Serge Semin
@ 2020-07-14 16:49               ` Dave Jiang
  0 siblings, 0 replies; 33+ messages in thread
From: Dave Jiang @ 2020-07-14 16:49 UTC (permalink / raw)
  To: Serge Semin
  Cc: Vinod Koul, Andy Shevchenko, Viresh Kumar, Dan Williams,
	Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, Rob Herring,
	linux-mips, devicetree, dmaengine, linux-kernel



On 7/14/2020 9:29 AM, Serge Semin wrote:
> On Tue, Jul 14, 2020 at 09:18:16AM -0700, Dave Jiang wrote:
>>
>>
>> On 7/14/2020 9:08 AM, Vinod Koul wrote:
>>> On 13-07-20, 13:55, Dave Jiang wrote:
>>>>
>>>>
>>>> On 7/10/2020 2:38 AM, Serge Semin wrote:
>>>>> On Fri, Jul 10, 2020 at 11:45:03AM +0300, Andy Shevchenko wrote:
>>>>>> On Fri, Jul 10, 2020 at 01:45:44AM +0300, Serge Semin wrote:
>>>>>>> There are DMA devices (like ours version of Synopsys DW DMAC) which have
>>>>>>> DMA capabilities non-uniformly redistributed between the device channels.
>>>>>>> In order to provide a way of exposing the channel-specific parameters to
>>>>>>> the DMA engine consumers, we introduce a new DMA-device callback. In case
>>>>>>> if provided it gets called from the dma_get_slave_caps() method and is
>>>>>>> able to override the generic DMA-device capabilities.
>>>>>>
>>>>>
>>>>>> In light of recent developments consider not to add 'slave' and a such words to the kernel.
>>>>>
>>>>> As long as the 'slave' word is used in the name of the dma_slave_caps
>>>>> structure and in the rest of the DMA-engine subsystem, it will be ambiguous
>>>>> to use some else terminology. If renaming needs to be done, then it should be
>>>>> done synchronously for the whole subsystem.
>>>>
>>>> What about just calling it dma_device_caps? Consider this is a useful
>>>> function not only slave DMA will utilize this. I can see this being useful
>>>> for some of my future code with idxd driver.
>>>
>>> Some of the caps may make sense to generic dmaengine but few of them do
>>> not :) While at it, am planning to make it dmaengine_periph_caps to
>>> denote that these are dmaengine peripheral capabilities.
>>>
>>
> 
>> If the function only passes in periph_caps, how do we allow the non periph
>> DMA utilize this function?
> 
> Hello Dave. That seems reasonable. "dma_device_caps" or even "dma_chan_caps"
> might be more suitable seeing after this patchset merged in the "dma_slave_caps"
> may really provide the DMA channel-specific configs. Moreover that structure is
> accessible only by means of the dma_chan descriptor:
> 
> int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_caps *caps);
> 
> which makes those caps being the channel-specific even without this patchset.
> 
> So as I see it "dma_chan_caps" might be the better choice.

Hi Sergey. Yes I think that sounds pretty good. Especially seeing there are DMA 
engines that have channels with different/asymmetric capabilities now.

> 
> -Sergey
> 

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

* Re: [PATCH v7 04/11] dmaengine: Introduce max SG list entries capability
  2020-07-10 16:14         ` Serge Semin
@ 2020-07-15 11:13           ` Vinod Koul
  2020-07-15 17:08             ` Serge Semin
  0 siblings, 1 reply; 33+ messages in thread
From: Vinod Koul @ 2020-07-15 11:13 UTC (permalink / raw)
  To: Serge Semin
  Cc: Peter Ujfalusi, Serge Semin, Viresh Kumar, Dan Williams,
	Andy Shevchenko, Alexey Malahov, Thomas Bogendoerfer,
	Arnd Bergmann, Rob Herring, linux-mips, devicetree, dmaengine,
	linux-kernel

On 10-07-20, 19:14, Serge Semin wrote:
> On Fri, Jul 10, 2020 at 02:51:33PM +0300, Peter Ujfalusi wrote:

> > Since we should be able to handle longer lists and this is kind of a
> > hint for clients that above this number of nents the list will be broken
> > up to smaller 'bursts', which when traversing could cause latency.
> > 
> > sg_chunk_len might be another candidate.
> 
> Ok. We've got four candidates:
> - max_sg_nents_burst
> - max_sg_burst
> - max_sg_chain
> - sg_chunk_len
> 
> @Vinod, @Andy, what do you think?

So IIUC your hw supports single sg and in that you would like to publish
the length of each chunk, is that correct? If so sg_chunk_len seems
apt..

-- 
~Vinod

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

* Re: [PATCH v7 08/11] dmaengine: dw: Add dummy device_caps callback
  2020-07-10  9:45     ` Serge Semin
@ 2020-07-15 12:01       ` Vinod Koul
  0 siblings, 0 replies; 33+ messages in thread
From: Vinod Koul @ 2020-07-15 12:01 UTC (permalink / raw)
  To: Serge Semin
  Cc: Andy Shevchenko, Viresh Kumar, Dan Williams, Alexey Malahov,
	Thomas Bogendoerfer, Arnd Bergmann, Rob Herring, linux-mips,
	devicetree, dmaengine, linux-kernel

On 10-07-20, 12:45, Serge Semin wrote:
> On Fri, Jul 10, 2020 at 11:51:23AM +0300, Andy Shevchenko wrote:
> > On Fri, Jul 10, 2020 at 01:45:47AM +0300, Serge Semin wrote:
> > > Since some DW DMA controllers (like one installed on Baikal-T1 SoC) may
> > > have non-uniform DMA capabilities per device channels, let's add
> > > the DW DMA specific device_caps callback to expose that specifics up to
> > > the DMA consumer. It's a dummy function for now. We'll fill it in with
> > > capabilities overrides in the next commits.
> > 
> 
> > Just a reminder (mainly to Vinod) of my view to this.
> > Unneeded churn, should be folded to patch 9.
> 
> Just to remind (mainly to Vinod). That's Andy's bikeshedding.
> This isn't a churn, since it's totally normal to design the patchset in this way:
> introduce a callback, then fill it in with functionality.

Looking at both patches, they do one thing, so please fold them in..

-- 
~Vinod

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

* Re: [PATCH v7 04/11] dmaengine: Introduce max SG list entries capability
  2020-07-15 11:13           ` Vinod Koul
@ 2020-07-15 17:08             ` Serge Semin
  2020-07-17  8:14               ` Vinod Koul
  0 siblings, 1 reply; 33+ messages in thread
From: Serge Semin @ 2020-07-15 17:08 UTC (permalink / raw)
  To: Vinod Koul
  Cc: Serge Semin, Peter Ujfalusi, Viresh Kumar, Dan Williams,
	Andy Shevchenko, Alexey Malahov, Thomas Bogendoerfer,
	Arnd Bergmann, Rob Herring, linux-mips, devicetree, dmaengine,
	linux-kernel

On Wed, Jul 15, 2020 at 04:43:15PM +0530, Vinod Koul wrote:
> On 10-07-20, 19:14, Serge Semin wrote:
> > On Fri, Jul 10, 2020 at 02:51:33PM +0300, Peter Ujfalusi wrote:
> 
> > > Since we should be able to handle longer lists and this is kind of a
> > > hint for clients that above this number of nents the list will be broken
> > > up to smaller 'bursts', which when traversing could cause latency.
> > > 
> > > sg_chunk_len might be another candidate.
> > 
> > Ok. We've got four candidates:
> > - max_sg_nents_burst
> > - max_sg_burst
> > - max_sg_chain
> > - sg_chunk_len
> > 
> > @Vinod, @Andy, what do you think?
> 

> So IIUC your hw supports single sg and in that you would like to publish
> the length of each chunk, is that correct?

No. My DMA engine does support only a single-entry SG-list, but the new DMA
{~~slave~~,channel,device,peripheral,...} capability isn't about the length, but
is about the maximum number of SG-list entries a DMA engine is able to
automatically/"without software help" walk through and execute. In this thread
we are debating about that new capability naming.

The name suggested in this patch: max_sg_nents. Peter noted (I mostly agree with
him), that it might be ambiguous, since from it (without looking into the
dma_slave_caps structure comment) a user might think that it's a maximum number of
SG-entries, which can be submitted for the DMA engine execution, while in fact it's
about the DMA engine capability of automatic/burst/"without software intervention"
SG-list entries walking through. (Such information will be helpful to solve a
problem discussed in this mailing thread, and described in the cover-letter to
this patchset. We also discussed it with you and Andy in the framework of this
patchset many times.)

As an alternative Peter suggested: max_sg_nents_burst. I also think it's better
than "max_sg_nents" but for me it seems a bit long. max_sg_burst seems better.
There is no need in having the "nents" in the name, since SG-list implies a list,
which main parameter (if not to say only parameter) is the number of entries.
"burst" is pointing out to the automatic/accelerated/"without software intervention"
SG-list entries walking through.

On the second thought suggested by me "max_sg_chain" sounds worse than "max_sg_burst",
because it also might be perceived as a parameter limiting the number of SG-list
entries is able to be submitted for the DMA engine execution, while in fact it
describes another matter.

Regarding "sg_chunk_len". I think it's ambiguous too, since the "chunk
length" might be referred to both the entries length and to the sub-SG-list
length.

So what do you think? What name is better describing the new DMA capability?

-Sergey

> If so sg_chunk_len seems apt..
> 
> -- 
> ~Vinod

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

* Re: [PATCH v7 04/11] dmaengine: Introduce max SG list entries capability
  2020-07-15 17:08             ` Serge Semin
@ 2020-07-17  8:14               ` Vinod Koul
  2020-07-17 12:36                 ` Serge Semin
  0 siblings, 1 reply; 33+ messages in thread
From: Vinod Koul @ 2020-07-17  8:14 UTC (permalink / raw)
  To: Serge Semin
  Cc: Serge Semin, Peter Ujfalusi, Viresh Kumar, Dan Williams,
	Andy Shevchenko, Alexey Malahov, Thomas Bogendoerfer,
	Arnd Bergmann, Rob Herring, linux-mips, devicetree, dmaengine,
	linux-kernel

On 15-07-20, 20:08, Serge Semin wrote:
> On Wed, Jul 15, 2020 at 04:43:15PM +0530, Vinod Koul wrote:
> > On 10-07-20, 19:14, Serge Semin wrote:
> > > On Fri, Jul 10, 2020 at 02:51:33PM +0300, Peter Ujfalusi wrote:
> > 
> > > > Since we should be able to handle longer lists and this is kind of a
> > > > hint for clients that above this number of nents the list will be broken
> > > > up to smaller 'bursts', which when traversing could cause latency.
> > > > 
> > > > sg_chunk_len might be another candidate.
> > > 
> > > Ok. We've got four candidates:
> > > - max_sg_nents_burst
> > > - max_sg_burst
> > > - max_sg_chain
> > > - sg_chunk_len
> > > 
> > > @Vinod, @Andy, what do you think?
> > 
> 
> > So IIUC your hw supports single sg and in that you would like to publish
> > the length of each chunk, is that correct?
> 
> No. My DMA engine does support only a single-entry SG-list, but the new DMA
> {~~slave~~,channel,device,peripheral,...} capability isn't about the length, but
> is about the maximum number of SG-list entries a DMA engine is able to
> automatically/"without software help" walk through and execute. In this thread
> we are debating about that new capability naming.
> 
> The name suggested in this patch: max_sg_nents. Peter noted (I mostly agree with
> him), that it might be ambiguous, since from it (without looking into the
> dma_slave_caps structure comment) a user might think that it's a maximum number of
> SG-entries, which can be submitted for the DMA engine execution, while in fact it's
> about the DMA engine capability of automatic/burst/"without software intervention"
> SG-list entries walking through. (Such information will be helpful to solve a
> problem discussed in this mailing thread, and described in the cover-letter to
> this patchset. We also discussed it with you and Andy in the framework of this
> patchset many times.)
> 
> As an alternative Peter suggested: max_sg_nents_burst. I also think it's better
> than "max_sg_nents" but for me it seems a bit long. max_sg_burst seems better.
> There is no need in having the "nents" in the name, since SG-list implies a list,
> which main parameter (if not to say only parameter) is the number of entries.
> "burst" is pointing out to the automatic/accelerated/"without software intervention"
> SG-list entries walking through.
> 
> On the second thought suggested by me "max_sg_chain" sounds worse than "max_sg_burst",
> because it also might be perceived as a parameter limiting the number of SG-list
> entries is able to be submitted for the DMA engine execution, while in fact it
> describes another matter.
> 
> Regarding "sg_chunk_len". I think it's ambiguous too, since the "chunk
> length" might be referred to both the entries length and to the sub-SG-list
> length.
> 
> So what do you think? What name is better describing the new DMA capability?

How about max_nents_per_sg or max_nents_sg to signify that this implies
max nents for sg not sg entries. IMO Burst/chain are not better than
max_sg_nents.

-- 
~Vinod

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

* Re: [PATCH v7 04/11] dmaengine: Introduce max SG list entries capability
  2020-07-17  8:14               ` Vinod Koul
@ 2020-07-17 12:36                 ` Serge Semin
  0 siblings, 0 replies; 33+ messages in thread
From: Serge Semin @ 2020-07-17 12:36 UTC (permalink / raw)
  To: Vinod Koul
  Cc: Peter Ujfalusi, Viresh Kumar, Dan Williams, Andy Shevchenko,
	Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, Rob Herring,
	linux-mips, devicetree, dmaengine, linux-kernel

On Fri, Jul 17, 2020 at 01:44:03PM +0530, Vinod Koul wrote:
> On 15-07-20, 20:08, Serge Semin wrote:
> > On Wed, Jul 15, 2020 at 04:43:15PM +0530, Vinod Koul wrote:
> > > On 10-07-20, 19:14, Serge Semin wrote:
> > > > On Fri, Jul 10, 2020 at 02:51:33PM +0300, Peter Ujfalusi wrote:
> > > 
> > > > > Since we should be able to handle longer lists and this is kind of a
> > > > > hint for clients that above this number of nents the list will be broken
> > > > > up to smaller 'bursts', which when traversing could cause latency.
> > > > > 
> > > > > sg_chunk_len might be another candidate.
> > > > 
> > > > Ok. We've got four candidates:
> > > > - max_sg_nents_burst
> > > > - max_sg_burst
> > > > - max_sg_chain
> > > > - sg_chunk_len
> > > > 
> > > > @Vinod, @Andy, what do you think?
> > > 
> > 
> > > So IIUC your hw supports single sg and in that you would like to publish
> > > the length of each chunk, is that correct?
> > 
> > No. My DMA engine does support only a single-entry SG-list, but the new DMA
> > {~~slave~~,channel,device,peripheral,...} capability isn't about the length, but
> > is about the maximum number of SG-list entries a DMA engine is able to
> > automatically/"without software help" walk through and execute. In this thread
> > we are debating about that new capability naming.
> > 
> > The name suggested in this patch: max_sg_nents. Peter noted (I mostly agree with
> > him), that it might be ambiguous, since from it (without looking into the
> > dma_slave_caps structure comment) a user might think that it's a maximum number of
> > SG-entries, which can be submitted for the DMA engine execution, while in fact it's
> > about the DMA engine capability of automatic/burst/"without software intervention"
> > SG-list entries walking through. (Such information will be helpful to solve a
> > problem discussed in this mailing thread, and described in the cover-letter to
> > this patchset. We also discussed it with you and Andy in the framework of this
> > patchset many times.)
> > 
> > As an alternative Peter suggested: max_sg_nents_burst. I also think it's better
> > than "max_sg_nents" but for me it seems a bit long. max_sg_burst seems better.
> > There is no need in having the "nents" in the name, since SG-list implies a list,
> > which main parameter (if not to say only parameter) is the number of entries.
> > "burst" is pointing out to the automatic/accelerated/"without software intervention"
> > SG-list entries walking through.
> > 
> > On the second thought suggested by me "max_sg_chain" sounds worse than "max_sg_burst",
> > because it also might be perceived as a parameter limiting the number of SG-list
> > entries is able to be submitted for the DMA engine execution, while in fact it
> > describes another matter.
> > 
> > Regarding "sg_chunk_len". I think it's ambiguous too, since the "chunk
> > length" might be referred to both the entries length and to the sub-SG-list
> > length.
> > 
> > So what do you think? What name is better describing the new DMA capability?
> 

> How about max_nents_per_sg or max_nents_sg to signify that this implies
> max nents for sg not sg entries.

Well, as I see it those versions are no better than "max_sg_nents" suggested
in this patch, which Peter and me considered ambiguous. By reading just
capability name all "max_sg_nents" and "max_nents_per_sg" and "max_nents_sg"
seem like describing a hard limitation of the number of SG-list entries, but
in fact they should merely mean a maximum number of entries executed in a single
DMA engine transaction without software intervention. We need to have some new
word which would define that "automatic/accelerated/burst/chained/etc." DMA
transaction.

> IMO Burst/chain are not better than max_sg_nents.

Could you elaborate why? In case of having "max_sg_burst" we could give to a
user an impression of this capability describing something similar to the
"max_burst" (maximum burst transaction length), but in application to the
SG-list. The main SG-list parameter is the number of entries, so having the
"burst" word in the capability name we'd imply the bursted number of entries
instead of the total number of entries if the "nents" word would have been used.

-Sergey

> 
> -- 
> ~Vinod

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

end of thread, other threads:[~2020-07-17 12:36 UTC | newest]

Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-09 22:45 [PATCH RESEND v7 00/11] dmaengine: dw: Take Baikal-T1 SoC DW DMAC peculiarities into account Serge Semin
2020-07-09 22:45 ` [PATCH v7 01/11] dt-bindings: dma: dw: Convert DW DMAC to DT binding Serge Semin
2020-07-09 22:45 ` [PATCH v7 02/11] dt-bindings: dma: dw: Add max burst transaction length property Serge Semin
2020-07-09 22:45 ` [PATCH v7 03/11] dmaengine: Introduce min burst length capability Serge Semin
2020-07-09 22:45 ` [PATCH v7 04/11] dmaengine: Introduce max SG list entries capability Serge Semin
2020-07-10  8:31   ` Peter Ujfalusi
2020-07-10  9:27     ` Serge Semin
2020-07-10 11:51       ` Peter Ujfalusi
2020-07-10 16:14         ` Serge Semin
2020-07-15 11:13           ` Vinod Koul
2020-07-15 17:08             ` Serge Semin
2020-07-17  8:14               ` Vinod Koul
2020-07-17 12:36                 ` Serge Semin
2020-07-09 22:45 ` [PATCH v7 05/11] dmaengine: Introduce DMA-device device_caps callback Serge Semin
2020-07-10  8:45   ` Andy Shevchenko
2020-07-10  9:38     ` Serge Semin
2020-07-13  6:51       ` Vinod Koul
2020-07-13 20:13         ` Serge Semin
2020-07-13 20:55       ` Dave Jiang
2020-07-14 16:08         ` Vinod Koul
2020-07-14 16:18           ` Dave Jiang
2020-07-14 16:29             ` Serge Semin
2020-07-14 16:49               ` Dave Jiang
2020-07-09 22:45 ` [PATCH v7 06/11] dmaengine: dw: Take HC_LLP flag into account for noLLP auto-config Serge Semin
2020-07-09 22:45 ` [PATCH v7 07/11] dmaengine: dw: Set DMA device max segment size parameter Serge Semin
2020-07-09 22:45 ` [PATCH v7 08/11] dmaengine: dw: Add dummy device_caps callback Serge Semin
2020-07-10  8:51   ` Andy Shevchenko
2020-07-10  8:52     ` Andy Shevchenko
2020-07-10  9:45     ` Serge Semin
2020-07-15 12:01       ` Vinod Koul
2020-07-09 22:45 ` [PATCH v7 09/11] dmaengine: dw: Initialize min and max burst DMA device capability Serge Semin
2020-07-09 22:45 ` [PATCH v7 10/11] dmaengine: dw: Introduce max burst length hw config Serge Semin
2020-07-09 22:45 ` [PATCH v7 11/11] dmaengine: dw: Initialize max_sg_nents capability Serge Semin

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