Add Intel LGM SoC DMA support

Quoted 1 line

drivers/dma/lgm/lgm-dma.c:729:5: warning: no previous prototype for function 'intel_dma_chan_desc_cfg' [-Wmissing-prototypes]

729 int intel_dma_chan_desc_cfg(struct dma_chan *chan, dma_addr_t desc_base,

Add DT bindings YAML schema for DMA controller driver of Lightning Mountain (LGM) SoC.

Signed-off-by Amireddy Mallikarjuna reddy <[email protected]>

---
v1:
- Initial version.

v2:
- Fix bot errors.

v3:
- No change.

v4:
- Address Thomas langer comments
- use node name pattern as dma-controller as in common binding.
- Remove "_" (underscore) in instance name.
- Remove "port-" and "chan-" in attribute name for both 'dma-ports' & 'dma-channels' child nodes.

v5:
- Moved some of the attributes in 'dma-ports' & 'dma-channels' child nodes to dma client/consumer side as cells in 'dmas' properties.

v6:
- Add additionalProperties: false
- completely removed 'dma-ports' and 'dma-channels' child nodes.
- Moved channel dt properties to client side dmas.
- Use standard dma-channels and dma-channel-mask properties.
- Documented reset-names
- Add description for dma-cells

v7:
- modified compatible to oneof
- Reduced number of dma-cells to 3
- Fine tune the description of some properties.

v7-resend:
- rebase to 5.10-rc1

v8:
- rebased to 5.10-rc3
- Fixing the bot issues (wrong indentation)

v9:
- rebased to 5.10-rc3
- Use 'enum' instead of oneOf+const
- Drop '#dma-cells' in required:, already covered in dma-common.yaml
- Drop nodename Already covered by dma-controller.yaml

---
 .../devicetree/bindings/dma/intel,ldma.yaml        | 130 +++++++++++++++++++++
 1 file changed, 130 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/intel,ldma.yaml

Add DT bindings YAML schema for DMA controller driver of Lightning Mountain (LGM) SoC.

Signed-off-by Amireddy Mallikarjuna reddy <[email protected]>

---
v1:
- Initial version.

v2:
- Fix bot errors.

v3:
- No change.

v4:
- Address Thomas langer comments
- use node name pattern as dma-controller as in common binding.
- Remove "_" (underscore) in instance name.
- Remove "port-" and "chan-" in attribute name for both 'dma-ports' & 'dma-channels' child nodes.

v5:
- Moved some of the attributes in 'dma-ports' & 'dma-channels' child nodes to dma client/consumer side as cells in 'dmas' properties.

v6:
- Add additionalProperties: false
- completely removed 'dma-ports' and 'dma-channels' child nodes.
- Moved channel dt properties to client side dmas.
- Use standard dma-channels and dma-channel-mask properties.
- Documented reset-names
- Add description for dma-cells

v7:
- modified compatible to oneof
- Reduced number of dma-cells to 3
- Fine tune the description of some properties.

v7-resend:
- rebase to 5.10-rc1

v8:
- rebased to 5.10-rc3
- Fixing the bot issues (wrong indentation)

v9:
- rebased to 5.10-rc3
- Use 'enum' instead of oneOf+const
- Drop '#dma-cells' in required:, already covered in dma-common.yaml
- Drop nodename Already covered by dma-controller.yaml

---
 .../devicetree/bindings/dma/intel,ldma.yaml        | 130 +++++++++++++++++++++
 1 file changed, 130 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/intel,ldma.yaml

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +51 -0

new file mode 100644
index 000000000000..c06281a10178
@@ -0,0 +1,130 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/intel,ldma.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Lightning Mountain centralized low speed DMA and high speed DMA controllers.
+
+maintainers:
+  - [email protected]
+  - [email protected]
+
+allOf:
+  - $ref: "dma-controller.yaml#"
+
+properties:
+  compatible:
+    enum:
+      - intel,lgm-cdma
+      - intel,lgm-dma2tx
+      - intel,lgm-dma1rx
+      - intel,lgm-dma1tx
+      - intel,lgm-dma0tx
+      - intel,lgm-dma3
+      - intel,lgm-toe-dma30
+      - intel,lgm-toe-dma31
+
+  reg:
+    maxItems: 1
+
+  "#dma-cells":
+    const: 3
+    description:
+      The first cell is the peripheral's DMA request line.
+      The second cell is the peripheral's (port) number corresponding to the channel.
+      The third cell is the burst length of the channel.
+
+  dma-channels:
+    minimum: 1
+    maximum: 16
+
+  dma-channel-mask:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  reset-names:

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +33 -0

+
+additionalProperties: false
+
+examples:
+  - |
+    dma0: dma-controller@e0e00000 {
+      compatible = "intel,lgm-cdma";
+      reg = <0xe0e00000 0x1000>;
+      #dma-cells = <3>;
+      dma-channels = <16>;
+      dma-channel-mask = <0xFFFF>;
+      interrupt-parent = <&ioapic1>;
+      interrupts = <82 1>;
+      resets = <&rcu0 0x30 0>;
+      reset-names = "ctrl";
+      clocks = <&cgu0 80>;
+      intel,dma-poll-cnt = <4>;
+      intel,dma-byte-en;
+      intel,dma-drb;
+    };
+  - |
+    dma3: dma-controller@ec800000 {
+      compatible = "intel,lgm-dma3";
+      reg = <0xec800000 0x1000>;
+      clocks = <&cgu0 71>;
+      resets = <&rcu0 0x10 9>;
+      #dma-cells = <3>;
+      intel,dma-poll-cnt = <16>;
+      intel,dma-desc-in-sram;
+      intel,dma-orrc = <16>;
+      intel,dma-byte-en;
+      intel,dma-dburst-wr;
+    };

Signature · 1 line

2.11.0

Add DMA controller driver for Lightning Mountain (LGM) family of SoCs.

The main function of the DMA controller is the transfer of data from/to any peripheral to/from the memory. A memory to memory copy capability can also be configured.

This ldma driver is used for configure the device and channnels for data and control paths.

Signed-off-by Amireddy Mallikarjuna reddy <[email protected]>

---
v1:
- Initial version.

drivers/dma/lgm/lgm-dma.c new file +8 -0

+struct dma_dev_ops {
+	int (*device_alloc_chan_resources)(struct dma_chan *chan);
+	void (*device_free_chan_resources)(struct dma_chan *chan);
+	int (*device_config)(struct dma_chan *chan,
+			     struct dma_slave_config *config);
+	int (*device_pause)(struct dma_chan *chan);
+	int (*device_resume)(struct dma_chan *chan);
+	int (*device_terminate_all)(struct dma_chan *chan);

drivers/dma/lgm/lgm-dma.c new file +12 -0

+static int ldma_chan_desc_cfg(struct ldma_chan *c, dma_addr_t desc_base,
+			      int desc_num)
+{
+	struct ldma_dev *d = to_ldma_dev(c->vchan.chan.device);
+
+	if (!desc_num) {
+		dev_err(d->dev, "Channel %d must allocate descriptor first\n",
+			c->nr);
+		return -EINVAL;
+	}
+
+	if (desc_num > DMA_MAX_DESC_NUM) {

new file mode 100644
index 000000000000..6942e0ef0977

include/linux/dma/lgm_dma.h new file +9 -0

@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2016 ~ 2020 Intel Corporation.
+ */
+#ifndef LGM_DMA_H
+#define LGM_DMA_H
+
+#include <linux/types.h>
+#include <linux/dmaengine.h>

Quoted 6 lines · Amireddy Mallikarjuna reddy

Add DMA controller driver for Lightning Mountain (LGM) family of SoCs.

The main function of the DMA controller is the transfer of data from/to any peripheral to/from the memory. A memory to memory copy capability can also be configured.

Quoted 7 lines

This ldma driver is used for configure the device and channnels for data and control paths.

Signed-off-by Amireddy Mallikarjuna reddy <[email protected]>

---
v1:
- Initial version.

You have a cover letter, use that to keep track of these changes

drivers/dma/lgm/Kconfig line 3

Quoted 6 lines

+++ b/drivers/dma/lgm/Kconfig

drivers/dma/lgm/Kconfig new file +3 -0

@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config INTEL_LDMA
+	bool "Lightning Mountain centralized low speed DMA and high speed DMA controllers"

Do we have any other speeds :D

drivers/dma/lgm/lgm-dma.c line 5

Quoted 8 lines

drivers/dma/lgm/lgm-dma.c new file +6 -0

+++ b/drivers/dma/lgm/lgm-dma.c

@@ -0,0 +1,1742 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Lightning Mountain centralized low speed and high speed DMA controller driver
+ *
+ * Copyright (c) 2016 ~ 2020 Intel Corporation.

I think you mean 2016 - 2020, a dash which refers to duration

drivers/dma/lgm/lgm-dma.c line 264

Quoted 12 lines

drivers/dma/lgm/lgm-dma.c new file +11 -0

+struct dw2_desc {
+	struct {
+		u32 len		:16;
+		u32 res0	:7;
+		u32 bofs	:2;
+		u32 res1	:3;
+		u32 eop		:1;
+		u32 sop		:1;
+		u32 c		:1;
+		u32 own		:1;
+	} __packed field;

Another one, looks like folks adding dmaengine patches love this approach, second one for the day..

Now why do you need the bit fields, why not use register defines and helpers in bitfield.h to help configure the fields See FIELD_GET, FIELD_PREP etc

Quoted 12 lines

drivers/dma/lgm/lgm-dma.c new file +11 -0

+struct dma_dev_ops {
+	int (*device_alloc_chan_resources)(struct dma_chan *chan);
+	void (*device_free_chan_resources)(struct dma_chan *chan);
+	int (*device_config)(struct dma_chan *chan,
+			     struct dma_slave_config *config);
+	int (*device_pause)(struct dma_chan *chan);
+	int (*device_resume)(struct dma_chan *chan);
+	int (*device_terminate_all)(struct dma_chan *chan);
+	void (*device_synchronize)(struct dma_chan *chan);
+	enum dma_status (*device_tx_status)(struct dma_chan *chan,
+					    dma_cookie_t cookie,

drivers/dma/lgm/lgm-dma.c line 294

Quoted 7 lines

drivers/dma/lgm/lgm-dma.c new file +7 -0

+					    struct dma_tx_state *txstate);
+	struct dma_async_tx_descriptor *(*device_prep_slave_sg)
+		(struct dma_chan *chan, struct scatterlist *sgl,
+		unsigned int sg_len, enum dma_transfer_direction direction,
+		unsigned long flags, void *context);
+	void (*device_issue_pending)(struct dma_chan *chan);
+};

Heh! why do you have a copy of dmaengine ops here?

Quoted 17 lines

drivers/dma/lgm/lgm-dma.c new file +16 -0

+static int ldma_chan_desc_cfg(struct ldma_chan *c, dma_addr_t desc_base,
+			      int desc_num)
+{
+	struct ldma_dev *d = to_ldma_dev(c->vchan.chan.device);
+
+	if (!desc_num) {
+		dev_err(d->dev, "Channel %d must allocate descriptor first\n",
+			c->nr);
+		return -EINVAL;
+	}
+
+	if (desc_num > DMA_MAX_DESC_NUM) {
+		dev_err(d->dev, "Channel %d descriptor number out of range %d\n",
+			c->nr, desc_num);
+		return -EINVAL;
+	}

drivers/dma/lgm/lgm-dma.c line 707

Quoted 6 lines

drivers/dma/lgm/lgm-dma.c new file +6 -0

+
+	ldma_chan_desc_hw_cfg(c, desc_base, desc_num);
+
+	c->flags |= DMA_HW_DESC;
+	c->desc_cnt = desc_num;
+	c->desc_phys = desc_base;

So you have a custom API which is used to configure this flag, a number and an address. The question is why, can you please help explain this?

drivers/dma/lgm/lgm-dma.c line 1052

Quoted 8 lines

drivers/dma/lgm/lgm-dma.c new file +7 -0

+static void dma_issue_pending(struct dma_chan *chan)
+{
+	struct ldma_chan *c = to_ldma_chan(chan);
+	struct ldma_dev *d = to_ldma_dev(c->vchan.chan.device);
+	unsigned long flags;
+
+	if (d->ver == DMA_VER22) {

why is this specific to this version?

drivers/dma/lgm/lgm-dma.c line 1127

Quoted 11 lines

drivers/dma/lgm/lgm-dma.c new file +10 -0

+static enum dma_status
+dma_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
+	      struct dma_tx_state *txstate)
+{
+	struct ldma_chan *c = to_ldma_chan(chan);
+	struct ldma_dev *d = to_ldma_dev(c->vchan.chan.device);
+	enum dma_status status = DMA_COMPLETE;
+
+	if (d->ver == DMA_VER22)
+		status = dma_cookie_status(chan, cookie, txstate);

so for non DMA_VER22 status is always complete, even if I may havent invoked issue_pending() right!

Quoted 18 lines

new file mode 100644
index 000000000000..6942e0ef0977

include/linux/dma/lgm_dma.h new file +12 -0

@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2016 ~ 2020 Intel Corporation.
+ */
+#ifndef LGM_DMA_H
+#define LGM_DMA_H
+
+#include <linux/types.h>
+#include <linux/dmaengine.h>
+
+/*!
+ * \fn int intel_dma_chan_desc_cfg(struct dma_chan *chan, dma_addr_t desc_base,

include/linux/dma/lgm_dma.h line 19

Quoted 7 lines

include/linux/dma/lgm_dma.h new file +7 -0

+ *                                 int desc_num)
+ * \brief Configure low level channel descriptors
+ * \param[in] chan   pointer to DMA channel that the client is using
+ * \param[in] desc_base   descriptor base physical address
+ * \param[in] desc_num   number of descriptors
+ * \return   0 on success
+ * \return   kernel bug reported on failure

See Documentation/process/coding-style.rst!

Quoted 113 lines · Amireddy Mallikarjuna reddy

Add DT bindings YAML schema for DMA controller driver of Lightning Mountain (LGM) SoC.

Signed-off-by Amireddy Mallikarjuna reddy <[email protected]>

---
v1:
- Initial version.

v2:
- Fix bot errors.

v3:
- No change.

v4:
- Address Thomas langer comments
- use node name pattern as dma-controller as in common binding.
- Remove "_" (underscore) in instance name.
- Remove "port-" and "chan-" in attribute name for both 'dma-ports' & 'dma-channels' child nodes.

v5:
- Moved some of the attributes in 'dma-ports' & 'dma-channels' child nodes to dma client/consumer side as cells in 'dmas' properties.

v6:
- Add additionalProperties: false
- completely removed 'dma-ports' and 'dma-channels' child nodes.
- Moved channel dt properties to client side dmas.
- Use standard dma-channels and dma-channel-mask properties.
- Documented reset-names
- Add description for dma-cells

v7:
- modified compatible to oneof
- Reduced number of dma-cells to 3
- Fine tune the description of some properties.

v7-resend:
- rebase to 5.10-rc1

v8:
- rebased to 5.10-rc3
- Fixing the bot issues (wrong indentation)

v9:
- rebased to 5.10-rc3
- Use 'enum' instead of oneOf+const
- Drop '#dma-cells' in required:, already covered in dma-common.yaml
- Drop nodename Already covered by dma-controller.yaml

---
  .../devicetree/bindings/dma/intel,ldma.yaml        | 130 +++++++++++++++++++++
  1 file changed, 130 insertions(+)

create mode 100644 Documentation/devicetree/bindings/dma/intel,ldma.yaml

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +54 -0

new file mode 100644
index 000000000000..c06281a10178
@@ -0,0 +1,130 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/intel,ldma.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Lightning Mountain centralized low speed DMA and high speed DMA controllers.
+
+maintainers:
+  - [email protected]
+  - [email protected]
+
+allOf:
+  - $ref: "dma-controller.yaml#"
+
+properties:
+  compatible:
+    enum:
+      - intel,lgm-cdma
+      - intel,lgm-dma2tx
+      - intel,lgm-dma1rx
+      - intel,lgm-dma1tx
+      - intel,lgm-dma0tx
+      - intel,lgm-dma3
+      - intel,lgm-toe-dma30
+      - intel,lgm-toe-dma31
+
+  reg:
+    maxItems: 1
+
+  "#dma-cells":
+    const: 3
+    description:
+      The first cell is the peripheral's DMA request line.
+      The second cell is the peripheral's (port) number corresponding to the channel.
+      The third cell is the burst length of the channel.
+
+  dma-channels:
+    minimum: 1
+    maximum: 16
+
+  dma-channel-mask:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  reset-names:
+    items:
+      - const: ctrl
+

Documentation/devicetree/bindings/dma/intel,ldma.yaml line 61

Quoted 7 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +7 -0

+  interrupts:
+    maxItems: 1
+
+  intel,dma-poll-cnt:
+    $ref: /schemas/types.yaml#definitions/uint32
+    description:
+      DMA descriptor polling counter is used to control the poling mechanism

s/poling/polling

Documentation/devicetree/bindings/dma/intel,ldma.yaml line 69

Quoted 9 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +8 -0

+      for the descriptor fetching for all channels.
+
+  intel,dma-byte-en:
+    type: boolean
+    description:
+      DMA byte enable is only valid for DMA write(RX).
+      Byte enable(1) means DMA write will be based on the number of dwords
+      instead of the whole burst.

Can you explain this, also sounds you could use _maxburst values..?

Documentation/devicetree/bindings/dma/intel,ldma.yaml line 80

Quoted 12 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +11 -0

+
+  intel,dma-drb:
+    type: boolean
+    description:
+      DMA descriptor read back to make sure data and desc synchronization.
+
+  intel,dma-desc-in-sram:
+    type: boolean
+    description:
+      DMA descritpors in SRAM or not. Some old controllers descriptors
+      can be in DRAM or SRAM. The new ones are all in SRAM.

should that not be decided by driver..? Or is this a hw property?

Documentation/devicetree/bindings/dma/intel,ldma.yaml line 86

Quoted 7 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +6 -0

+
+  intel,dma-orrc:
+    $ref: /schemas/types.yaml#definitions/uint32
+    description:
+      DMA outstanding read counter value determine the number of
+      ORR-Outstanding Read Request. The maximum value is 16.

How would this be used by folks..?

Documentation/devicetree/bindings/dma/intel,ldma.yaml line 97

Quoted 12 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +11 -0

+
+  intel,dma-dburst-wr:
+    type: boolean
+    description:
+      Enable RX dynamic burst write. When it is enabled, the DMA does RX dynamic burst;
+      if it is disabled, the DMA RX will still support programmable fixed burst size of 2,4,8,16.
+      It only applies to RX DMA and memcopy DMA.
+
+required:
+  - compatible
+  - reg

So only two are mandatory, what about the bunch of intel properties you added above..?

Quoted 36 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +33 -0

+
+additionalProperties: false
+
+examples:
+  - |
+    dma0: dma-controller@e0e00000 {
+      compatible = "intel,lgm-cdma";
+      reg = <0xe0e00000 0x1000>;
+      #dma-cells = <3>;
+      dma-channels = <16>;
+      dma-channel-mask = <0xFFFF>;
+      interrupt-parent = <&ioapic1>;
+      interrupts = <82 1>;
+      resets = <&rcu0 0x30 0>;
+      reset-names = "ctrl";
+      clocks = <&cgu0 80>;
+      intel,dma-poll-cnt = <4>;
+      intel,dma-byte-en;
+      intel,dma-drb;
+    };
+  - |
+    dma3: dma-controller@ec800000 {
+      compatible = "intel,lgm-dma3";
+      reg = <0xec800000 0x1000>;
+      clocks = <&cgu0 71>;
+      resets = <&rcu0 0x10 9>;
+      #dma-cells = <3>;
+      intel,dma-poll-cnt = <16>;
+      intel,dma-desc-in-sram;
+      intel,dma-orrc = <16>;
+      intel,dma-byte-en;
+      intel,dma-dburst-wr;
+    };

Signature · 1 line

2.11.0

Hi Vinod,

Thanks for the review. My comments inline.

Quoted 7 lines · Vinod Koul

Quoted 6 lines · Amireddy Mallikarjuna reddy

Add DMA controller driver for Lightning Mountain (LGM) family of SoCs.

The main function of the DMA controller is the transfer of data from/to any peripheral to/from the memory. A memory to memory copy capability can also be configured.

Quoted 8 lines

Quoted 7 lines

This ldma driver is used for configure the device and channnels for data and control paths.

Signed-off-by Amireddy Mallikarjuna reddy <[email protected]>

---
v1:
- Initial version.

You have a cover letter, use that to keep track of these changes

ok.

Quoted 1 line

Quoted 6 lines

drivers/dma/lgm/Kconfig line 3

Quoted 5 lines

+++ b/drivers/dma/lgm/Kconfig

drivers/dma/lgm/Kconfig new file +3 -0

@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config INTEL_LDMA
+	bool "Lightning Mountain centralized low speed DMA and high speed DMA controllers"

Do we have any other speeds :D

No other speeds :-)

Quoted 9 lines

drivers/dma/lgm/lgm-dma.c line 5

Quoted 8 lines

drivers/dma/lgm/lgm-dma.c new file +6 -0

+++ b/drivers/dma/lgm/lgm-dma.c

@@ -0,0 +1,1742 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Lightning Mountain centralized low speed and high speed DMA controller driver
+ *
+ * Copyright (c) 2016 ~ 2020 Intel Corporation.

I think you mean 2016 - 2020, a dash which refers to duration

ok.

Quoted 15 lines

drivers/dma/lgm/lgm-dma.c line 264

Quoted 12 lines

drivers/dma/lgm/lgm-dma.c new file +11 -0

+struct dw2_desc {
+	struct {
+		u32 len		:16;
+		u32 res0	:7;
+		u32 bofs	:2;
+		u32 res1	:3;
+		u32 eop		:1;
+		u32 sop		:1;
+		u32 c		:1;
+		u32 own		:1;
+	} __packed field;

Another one, looks like folks adding dmaengine patches love this approach, second one for the day..

Quoted 3 lines

Now why do you need the bit fields, why not use register defines and helpers in bitfield.h to help configure the fields See FIELD_GET, FIELD_PREP etc

Let me check on this...

Quoted 12 lines

Quoted 12 lines

drivers/dma/lgm/lgm-dma.c new file +11 -0

+struct dma_dev_ops {
+	int (*device_alloc_chan_resources)(struct dma_chan *chan);
+	void (*device_free_chan_resources)(struct dma_chan *chan);
+	int (*device_config)(struct dma_chan *chan,
+			     struct dma_slave_config *config);
+	int (*device_pause)(struct dma_chan *chan);
+	int (*device_resume)(struct dma_chan *chan);
+	int (*device_terminate_all)(struct dma_chan *chan);
+	void (*device_synchronize)(struct dma_chan *chan);
+	enum dma_status (*device_tx_status)(struct dma_chan *chan,
+					    dma_cookie_t cookie,

Quoted 8 lines

drivers/dma/lgm/lgm-dma.c line 294

Quoted 7 lines

drivers/dma/lgm/lgm-dma.c new file +7 -0

+					    struct dma_tx_state *txstate);
+	struct dma_async_tx_descriptor *(*device_prep_slave_sg)
+		(struct dma_chan *chan, struct scatterlist *sgl,
+		unsigned int sg_len, enum dma_transfer_direction direction,
+		unsigned long flags, void *context);
+	void (*device_issue_pending)(struct dma_chan *chan);
+};

Heh! why do you have a copy of dmaengine ops here?

Ok, i will remove the ops and update the code accordingly.

Quoted 17 lines

Quoted 17 lines

drivers/dma/lgm/lgm-dma.c new file +16 -0

+static int ldma_chan_desc_cfg(struct ldma_chan *c, dma_addr_t desc_base,
+			      int desc_num)
+{
+	struct ldma_dev *d = to_ldma_dev(c->vchan.chan.device);
+
+	if (!desc_num) {
+		dev_err(d->dev, "Channel %d must allocate descriptor first\n",
+			c->nr);
+		return -EINVAL;
+	}
+
+	if (desc_num > DMA_MAX_DESC_NUM) {
+		dev_err(d->dev, "Channel %d descriptor number out of range %d\n",
+			c->nr, desc_num);
+		return -EINVAL;
+	}

Quoted 8 lines

drivers/dma/lgm/lgm-dma.c line 707

Quoted 6 lines

drivers/dma/lgm/lgm-dma.c new file +6 -0

+
+	ldma_chan_desc_hw_cfg(c, desc_base, desc_num);
+
+	c->flags |= DMA_HW_DESC;
+	c->desc_cnt = desc_num;
+	c->desc_phys = desc_base;

So you have a custom API which is used to configure this flag, a number and an address. The question is why, can you please help explain this?

LDMA used as general purpose dma(ver == DMA_VER22) and also supports DMA capability for GSWIP in their network packet processing.( ver > DMA_VER22)

Each Ingress(IGP) & Egress(EGP) ports of CQM use a dma channel.

desc needs to be configure for each dma channel and the remapped address of the IGP & EGP is desc base adress.

CQM client is using ldma_chan_desc_cfg() to configure the descriptior.

Quoted 9 lines

drivers/dma/lgm/lgm-dma.c line 1052

Quoted 8 lines

drivers/dma/lgm/lgm-dma.c new file +7 -0

+static void dma_issue_pending(struct dma_chan *chan)
+{
+	struct ldma_chan *c = to_ldma_chan(chan);
+	struct ldma_dev *d = to_ldma_dev(c->vchan.chan.device);
+	unsigned long flags;
+
+	if (d->ver == DMA_VER22) {

why is this specific to this version?

Only dma0 instance (ver == DMA_VER22) is used as a general purpose slave DMA. we set both control and datapath here.
Other instances (ver > DMA_VER22) we set only control path. data path is taken care by dma client(GSWIP).
Only thing needs to do is get the channel, set the descriptor and just 'ON' the channel.

CQM is highly low level register configurable/programmable take care about the the packet processing through the register configurations.

Hi Vinod,
Thanks for the review. My comments inline.

Quoted 114 lines · Vinod Koul

Quoted 113 lines · Amireddy Mallikarjuna reddy

Add DT bindings YAML schema for DMA controller driver of Lightning Mountain (LGM) SoC.

Signed-off-by Amireddy Mallikarjuna reddy <[email protected]>

---
v1:
- Initial version.

v2:
- Fix bot errors.

v3:
- No change.

v4:
- Address Thomas langer comments
- use node name pattern as dma-controller as in common binding.
- Remove "_" (underscore) in instance name.
- Remove "port-" and "chan-" in attribute name for both 'dma-ports' & 'dma-channels' child nodes.

v5:
- Moved some of the attributes in 'dma-ports' & 'dma-channels' child nodes to dma client/consumer side as cells in 'dmas' properties.

v6:
- Add additionalProperties: false
- completely removed 'dma-ports' and 'dma-channels' child nodes.
- Moved channel dt properties to client side dmas.
- Use standard dma-channels and dma-channel-mask properties.
- Documented reset-names
- Add description for dma-cells

v7:
- modified compatible to oneof
- Reduced number of dma-cells to 3
- Fine tune the description of some properties.

v7-resend:
- rebase to 5.10-rc1

v8:
- rebased to 5.10-rc3
- Fixing the bot issues (wrong indentation)

v9:
- rebased to 5.10-rc3
- Use 'enum' instead of oneOf+const
- Drop '#dma-cells' in required:, already covered in dma-common.yaml
- Drop nodename Already covered by dma-controller.yaml

---
  .../devicetree/bindings/dma/intel,ldma.yaml        | 130 +++++++++++++++++++++
  1 file changed, 130 insertions(+)

create mode 100644 Documentation/devicetree/bindings/dma/intel,ldma.yaml

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +54 -0

new file mode 100644
index 000000000000..c06281a10178
@@ -0,0 +1,130 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/intel,ldma.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Lightning Mountain centralized low speed DMA and high speed DMA controllers.
+
+maintainers:
+  - [email protected]
+  - [email protected]
+
+allOf:
+  - $ref: "dma-controller.yaml#"
+
+properties:
+  compatible:
+    enum:
+      - intel,lgm-cdma
+      - intel,lgm-dma2tx
+      - intel,lgm-dma1rx
+      - intel,lgm-dma1tx
+      - intel,lgm-dma0tx
+      - intel,lgm-dma3
+      - intel,lgm-toe-dma30
+      - intel,lgm-toe-dma31
+
+  reg:
+    maxItems: 1
+
+  "#dma-cells":
+    const: 3
+    description:
+      The first cell is the peripheral's DMA request line.
+      The second cell is the peripheral's (port) number corresponding to the channel.
+      The third cell is the burst length of the channel.
+
+  dma-channels:
+    minimum: 1
+    maximum: 16
+
+  dma-channel-mask:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  reset-names:
+    items:
+      - const: ctrl
+

Quoted 8 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml line 61

Quoted 7 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +7 -0

+  interrupts:
+    maxItems: 1
+
+  intel,dma-poll-cnt:
+    $ref: /schemas/types.yaml#definitions/uint32
+    description:
+      DMA descriptor polling counter is used to control the poling mechanism

s/poling/polling

Ok, Thanks.

Quoted 9 lines

Quoted 9 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +8 -0

+      for the descriptor fetching for all channels.
+
+  intel,dma-byte-en:
+    type: boolean
+    description:
+      DMA byte enable is only valid for DMA write(RX).
+      Byte enable(1) means DMA write will be based on the number of dwords
+      instead of the whole burst.

Quoted 1 line

Can you explain this, also sounds you could use _maxburst values..?

when dma-byte-en = 0 (disabled) DMA write will be in terms of burst length, dma-byte-en = 1 (enabled) write will be in terms of Dwords.

Byte enable = 0 (Disabled) means that DMA write will be based on the burst length, even if it only transmits one byte.
Byte enable = 1(enabled) means that DMA write will be based on the number of Dwords, instead of the whole burst.

Quoted 14 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml line 80

Quoted 13 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +11 -0

+
+  intel,dma-drb:
+    type: boolean
+    description:
+      DMA descriptor read back to make sure data and desc synchronization.
+
+  intel,dma-desc-in-sram:
+    type: boolean
+    description:
+      DMA descritpors in SRAM or not. Some old controllers descriptors
+      can be in DRAM or SRAM. The new ones are all in SRAM.

should that not be decided by driver..? Or is this a hw property?

This is DMA controller capability. It can be decided from driver also. i will change accordingly.

Quoted 7 lines

Quoted 7 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +6 -0

+
+  intel,dma-orrc:
+    $ref: /schemas/types.yaml#definitions/uint32
+    description:
+      DMA outstanding read counter value determine the number of
+      ORR-Outstanding Read Request. The maximum value is 16.

Quoted 1 line

How would this be used by folks..?

A register bit will be used to enable/disable the ORR feature.

Outstanding Read Capability introduce CMD FIFO to support up to 16 outstanding reads for different packet in same channel.

For large packets up to 16 OR can be issued, the number of OR is configurable.

Quoted 14 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml line 97

Quoted 12 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +11 -0

+
+  intel,dma-dburst-wr:
+    type: boolean
+    description:
+      Enable RX dynamic burst write. When it is enabled, the DMA does RX dynamic burst;
+      if it is disabled, the DMA RX will still support programmable fixed burst size of 2,4,8,16.
+      It only applies to RX DMA and memcopy DMA.
+
+required:
+  - compatible
+  - reg

So only two are mandatory, what about the bunch of intel properties you added above..?

Some of the properties are DMA capabilities, Enabling from device tree. other properties will use default values from driver if we dont pass it from device tree.

Quoted 21 lines · MallikarjunaX

Hi Vinod,

Thanks for the review. My comments inline.

Quoted 7 lines · Vinod Koul

Quoted 6 lines · Amireddy Mallikarjuna reddy

Add DMA controller driver for Lightning Mountain (LGM) family of SoCs.

The main function of the DMA controller is the transfer of data from/to any peripheral to/from the memory. A memory to memory copy capability can also be configured.

Quoted 8 lines

Quoted 7 lines

This ldma driver is used for configure the device and channnels for data and control paths.

Signed-off-by Amireddy Mallikarjuna reddy <[email protected]>

---
v1:
- Initial version.

You have a cover letter, use that to keep track of these changes

ok.

Quoted 7 lines

Quoted 6 lines

drivers/dma/lgm/Kconfig line 3

Quoted 5 lines

+++ b/drivers/dma/lgm/Kconfig

drivers/dma/lgm/Kconfig new file +3 -0

@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config INTEL_LDMA
+	bool "Lightning Mountain centralized low speed DMA and high speed DMA controllers"

Do we have any other speeds :D

No other speeds :-)

Right, so possibly drop the speed characterization here!

Quoted 72 lines

Quoted 9 lines

drivers/dma/lgm/lgm-dma.c line 5

Quoted 8 lines

drivers/dma/lgm/lgm-dma.c new file +6 -0

+++ b/drivers/dma/lgm/lgm-dma.c

@@ -0,0 +1,1742 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Lightning Mountain centralized low speed and high speed DMA controller driver
+ *
+ * Copyright (c) 2016 ~ 2020 Intel Corporation.

I think you mean 2016 - 2020, a dash which refers to duration

ok.

Quoted 14 lines

drivers/dma/lgm/lgm-dma.c line 264

Quoted 11 lines

drivers/dma/lgm/lgm-dma.c new file +11 -0

+struct dw2_desc {
+	struct {
+		u32 len		:16;
+		u32 res0	:7;
+		u32 bofs	:2;
+		u32 res1	:3;
+		u32 eop		:1;
+		u32 sop		:1;
+		u32 c		:1;
+		u32 own		:1;
+	} __packed field;

Another one, looks like folks adding dmaengine patches love this approach, second one for the day..

Quoted 3 lines

Now why do you need the bit fields, why not use register defines and helpers in bitfield.h to help configure the fields See FIELD_GET, FIELD_PREP etc

Let me check on this...

Quoted 11 lines

Quoted 11 lines

drivers/dma/lgm/lgm-dma.c new file +11 -0

+struct dma_dev_ops {
+	int (*device_alloc_chan_resources)(struct dma_chan *chan);
+	void (*device_free_chan_resources)(struct dma_chan *chan);
+	int (*device_config)(struct dma_chan *chan,
+			     struct dma_slave_config *config);
+	int (*device_pause)(struct dma_chan *chan);
+	int (*device_resume)(struct dma_chan *chan);
+	int (*device_terminate_all)(struct dma_chan *chan);
+	void (*device_synchronize)(struct dma_chan *chan);
+	enum dma_status (*device_tx_status)(struct dma_chan *chan,
+					    dma_cookie_t cookie,

Quoted 8 lines

drivers/dma/lgm/lgm-dma.c line 294

Quoted 7 lines

drivers/dma/lgm/lgm-dma.c new file +7 -0

+					    struct dma_tx_state *txstate);
+	struct dma_async_tx_descriptor *(*device_prep_slave_sg)
+		(struct dma_chan *chan, struct scatterlist *sgl,
+		unsigned int sg_len, enum dma_transfer_direction direction,
+		unsigned long flags, void *context);
+	void (*device_issue_pending)(struct dma_chan *chan);
+};

Heh! why do you have a copy of dmaengine ops here?

Ok, i will remove the ops and update the code accordingly.

Quoted 24 lines

Quoted 12 lines

drivers/dma/lgm/lgm-dma.c new file +12 -0

+static int ldma_chan_desc_cfg(struct ldma_chan *c, dma_addr_t desc_base,
+			      int desc_num)
+{
+	struct ldma_dev *d = to_ldma_dev(c->vchan.chan.device);
+
+	if (!desc_num) {
+		dev_err(d->dev, "Channel %d must allocate descriptor first\n",
+			c->nr);
+		return -EINVAL;
+	}
+
+	if (desc_num > DMA_MAX_DESC_NUM) {

drivers/dma/lgm/lgm-dma.c line 707

Quoted 10 lines

drivers/dma/lgm/lgm-dma.c new file +10 -0

+		dev_err(d->dev, "Channel %d descriptor number out of range %d\n",
+			c->nr, desc_num);
+		return -EINVAL;
+	}
+
+	ldma_chan_desc_hw_cfg(c, desc_base, desc_num);
+
+	c->flags |= DMA_HW_DESC;
+	c->desc_cnt = desc_num;
+	c->desc_phys = desc_base;

So you have a custom API which is used to configure this flag, a number and an address. The question is why, can you please help explain this?

Quoted 2 lines

LDMA used as general purpose dma(ver == DMA_VER22) and also supports DMA capability for GSWIP in their network packet processing.( ver > DMA_VER22)

Whats GSWIP?

Quoted 1 line

Each Ingress(IGP) & Egress(EGP) ports of CQM use a dma channel.

CQM?

Quoted 3 lines

desc needs to be configure for each dma channel and the remapped address of the IGP & EGP is desc base adress.

Why should this address not passed as src_addr/dst_addr?

Quoted 17 lines

CQM client is using ldma_chan_desc_cfg() to configure the descriptior.

Quoted 8 lines

drivers/dma/lgm/lgm-dma.c line 1052

Quoted 7 lines

drivers/dma/lgm/lgm-dma.c new file +7 -0

+static void dma_issue_pending(struct dma_chan *chan)
+{
+	struct ldma_chan *c = to_ldma_chan(chan);
+	struct ldma_dev *d = to_ldma_dev(c->vchan.chan.device);
+	unsigned long flags;
+
+	if (d->ver == DMA_VER22) {

why is this specific to this version?

Only dma0 instance (ver == DMA_VER22) is used as a general purpose slave DMA. we set both control and datapath here.
Other instances (ver > DMA_VER22) we set only control path. data path is taken care by dma client(GSWIP).
Only thing needs to do is get the channel, set the descriptor and just 'ON' the channel.

Quoted 2 lines

CQM is highly low level register configurable/programmable take care about the the packet processing through the register configurations.

DMAengine fwk take care of channel management for clients and transaction management, if you not going to do transactions then why bother with dmaengine ?

Quoted 136 lines · MallikarjunaX

Hi Vinod,
Thanks for the review. My comments inline.

Quoted 114 lines · Vinod Koul

Quoted 113 lines · Amireddy Mallikarjuna reddy

Add DT bindings YAML schema for DMA controller driver of Lightning Mountain (LGM) SoC.

Signed-off-by Amireddy Mallikarjuna reddy <[email protected]>

---
v1:
- Initial version.

v2:
- Fix bot errors.

v3:
- No change.

v4:
- Address Thomas langer comments
- use node name pattern as dma-controller as in common binding.
- Remove "_" (underscore) in instance name.
- Remove "port-" and "chan-" in attribute name for both 'dma-ports' & 'dma-channels' child nodes.

v5:
- Moved some of the attributes in 'dma-ports' & 'dma-channels' child nodes to dma client/consumer side as cells in 'dmas' properties.

v6:
- Add additionalProperties: false
- completely removed 'dma-ports' and 'dma-channels' child nodes.
- Moved channel dt properties to client side dmas.
- Use standard dma-channels and dma-channel-mask properties.
- Documented reset-names
- Add description for dma-cells

v7:
- modified compatible to oneof
- Reduced number of dma-cells to 3
- Fine tune the description of some properties.

v7-resend:
- rebase to 5.10-rc1

v8:
- rebased to 5.10-rc3
- Fixing the bot issues (wrong indentation)

v9:
- rebased to 5.10-rc3
- Use 'enum' instead of oneOf+const
- Drop '#dma-cells' in required:, already covered in dma-common.yaml
- Drop nodename Already covered by dma-controller.yaml

---
   .../devicetree/bindings/dma/intel,ldma.yaml        | 130 +++++++++++++++++++++
   1 file changed, 130 insertions(+)

create mode 100644 Documentation/devicetree/bindings/dma/intel,ldma.yaml

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +54 -0

new file mode 100644
index 000000000000..c06281a10178
@@ -0,0 +1,130 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/intel,ldma.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Lightning Mountain centralized low speed DMA and high speed DMA controllers.
+
+maintainers:
+  - [email protected]
+  - [email protected]
+
+allOf:
+  - $ref: "dma-controller.yaml#"
+
+properties:
+  compatible:
+    enum:
+      - intel,lgm-cdma
+      - intel,lgm-dma2tx
+      - intel,lgm-dma1rx
+      - intel,lgm-dma1tx
+      - intel,lgm-dma0tx
+      - intel,lgm-dma3
+      - intel,lgm-toe-dma30
+      - intel,lgm-toe-dma31
+
+  reg:
+    maxItems: 1
+
+  "#dma-cells":
+    const: 3
+    description:
+      The first cell is the peripheral's DMA request line.
+      The second cell is the peripheral's (port) number corresponding to the channel.
+      The third cell is the burst length of the channel.
+
+  dma-channels:
+    minimum: 1
+    maximum: 16
+
+  dma-channel-mask:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  reset-names:
+    items:
+      - const: ctrl
+

Quoted 8 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml line 61

Quoted 7 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +7 -0

+  interrupts:
+    maxItems: 1
+
+  intel,dma-poll-cnt:
+    $ref: /schemas/types.yaml#definitions/uint32
+    description:
+      DMA descriptor polling counter is used to control the poling mechanism

s/poling/polling

Ok, Thanks.

Quoted 9 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml line 69

Quoted 8 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +8 -0

+      for the descriptor fetching for all channels.
+
+  intel,dma-byte-en:
+    type: boolean
+    description:
+      DMA byte enable is only valid for DMA write(RX).
+      Byte enable(1) means DMA write will be based on the number of dwords
+      instead of the whole burst.

Can you explain this, also sounds you could use _maxburst values..?

Quoted 7 lines

when dma-byte-en = 0 (disabled) DMA write will be in terms of burst length, dma-byte-en = 1 (enabled) write will be in terms of Dwords.

Byte enable = 0 (Disabled) means that DMA write will be based on the burst length, even if it only transmits one byte.
Byte enable = 1(enabled) means that DMA write will be based on the number of Dwords, instead of the whole burst.

Sounds like a hw property or is this configurable to engine..?

Quoted 21 lines

Quoted 4 lines

Quoted 4 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +4 -0

+
+  intel,dma-drb:
+    type: boolean
+    description:

Quoted 8 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml line 80

Quoted 7 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +7 -0

+      DMA descriptor read back to make sure data and desc synchronization.
+
+  intel,dma-desc-in-sram:
+    type: boolean
+    description:
+      DMA descritpors in SRAM or not. Some old controllers descriptors
+      can be in DRAM or SRAM. The new ones are all in SRAM.

should that not be decided by driver..? Or is this a hw property?

This is DMA controller capability. It can be decided from driver also. i will change accordingly.

Quoted 7 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml line 86

Quoted 6 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +6 -0

+
+  intel,dma-orrc:
+    $ref: /schemas/types.yaml#definitions/uint32
+    description:
+      DMA outstanding read counter value determine the number of
+      ORR-Outstanding Read Request. The maximum value is 16.

How would this be used by folks..?

Quoted 7 lines

A register bit will be used to enable/disable the ORR feature.

Outstanding Read Capability introduce CMD FIFO to support up to 16 outstanding reads for different packet in same channel.

For large packets up to 16 OR can be issued, the number of OR is configurable.

How will configure this and when..?

Quoted 17 lines

Quoted 14 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml line 97

Quoted 12 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +11 -0

+
+  intel,dma-dburst-wr:
+    type: boolean
+    description:
+      Enable RX dynamic burst write. When it is enabled, the DMA does RX dynamic burst;
+      if it is disabled, the DMA RX will still support programmable fixed burst size of 2,4,8,16.
+      It only applies to RX DMA and memcopy DMA.
+
+required:
+  - compatible
+  - reg

So only two are mandatory, what about the bunch of intel properties you added above..?

Some of the properties are DMA capabilities, Enabling from device tree. other properties will use default values from driver if we dont pass it from device tree.

Hi Vinod,

Thanks for your valuable review comments. Please see my comments inline.

Quoted 22 lines · Vinod Koul

Quoted 21 lines · MallikarjunaX

Hi Vinod,

Thanks for the review. My comments inline.

Quoted 7 lines · Vinod Koul

Quoted 6 lines · Amireddy Mallikarjuna reddy

Add DMA controller driver for Lightning Mountain (LGM) family of SoCs.

The main function of the DMA controller is the transfer of data from/to any peripheral to/from the memory. A memory to memory copy capability can also be configured.

Quoted 8 lines

Quoted 7 lines

This ldma driver is used for configure the device and channnels for data and control paths.

Signed-off-by Amireddy Mallikarjuna reddy <[email protected]>

---
v1:
- Initial version.

You have a cover letter, use that to keep track of these changes

ok.

Quoted 8 lines

Quoted 7 lines

Quoted 6 lines

drivers/dma/lgm/Kconfig line 3

Quoted 5 lines

+++ b/drivers/dma/lgm/Kconfig

drivers/dma/lgm/Kconfig new file +3 -0

@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config INTEL_LDMA
+	bool "Lightning Mountain centralized low speed DMA and high speed DMA controllers"

Do we have any other speeds :D

No other speeds :-)

Right, so possibly drop the speed characterization here!

"Lightning Mountain centralized DMA controller"

Quoted 74 lines

Quoted 74 lines

Quoted 9 lines

drivers/dma/lgm/lgm-dma.c line 5

Quoted 8 lines

drivers/dma/lgm/lgm-dma.c new file +6 -0

+++ b/drivers/dma/lgm/lgm-dma.c

@@ -0,0 +1,1742 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Lightning Mountain centralized low speed and high speed DMA controller driver
+ *
+ * Copyright (c) 2016 ~ 2020 Intel Corporation.

I think you mean 2016 - 2020, a dash which refers to duration

ok.

Quoted 14 lines

drivers/dma/lgm/lgm-dma.c line 264

Quoted 11 lines

drivers/dma/lgm/lgm-dma.c new file +11 -0

+struct dw2_desc {
+	struct {
+		u32 len		:16;
+		u32 res0	:7;
+		u32 bofs	:2;
+		u32 res1	:3;
+		u32 eop		:1;
+		u32 sop		:1;
+		u32 c		:1;
+		u32 own		:1;
+	} __packed field;

Another one, looks like folks adding dmaengine patches love this approach, second one for the day..

Quoted 3 lines

Now why do you need the bit fields, why not use register defines and helpers in bitfield.h to help configure the fields See FIELD_GET, FIELD_PREP etc

Let me check on this...

Quoted 11 lines

Quoted 11 lines

drivers/dma/lgm/lgm-dma.c new file +11 -0

+struct dma_dev_ops {
+	int (*device_alloc_chan_resources)(struct dma_chan *chan);
+	void (*device_free_chan_resources)(struct dma_chan *chan);
+	int (*device_config)(struct dma_chan *chan,
+			     struct dma_slave_config *config);
+	int (*device_pause)(struct dma_chan *chan);
+	int (*device_resume)(struct dma_chan *chan);
+	int (*device_terminate_all)(struct dma_chan *chan);
+	void (*device_synchronize)(struct dma_chan *chan);
+	enum dma_status (*device_tx_status)(struct dma_chan *chan,
+					    dma_cookie_t cookie,

Quoted 8 lines

drivers/dma/lgm/lgm-dma.c line 294

Quoted 7 lines

drivers/dma/lgm/lgm-dma.c new file +7 -0

+					    struct dma_tx_state *txstate);
+	struct dma_async_tx_descriptor *(*device_prep_slave_sg)
+		(struct dma_chan *chan, struct scatterlist *sgl,
+		unsigned int sg_len, enum dma_transfer_direction direction,
+		unsigned long flags, void *context);
+	void (*device_issue_pending)(struct dma_chan *chan);
+};

Heh! why do you have a copy of dmaengine ops here?

Ok, i will remove the ops and update the code accordingly.

Quoted 16 lines

Quoted 16 lines

drivers/dma/lgm/lgm-dma.c new file +16 -0

+static int ldma_chan_desc_cfg(struct ldma_chan *c, dma_addr_t desc_base,
+			      int desc_num)
+{
+	struct ldma_dev *d = to_ldma_dev(c->vchan.chan.device);
+
+	if (!desc_num) {
+		dev_err(d->dev, "Channel %d must allocate descriptor first\n",
+			c->nr);
+		return -EINVAL;
+	}
+
+	if (desc_num > DMA_MAX_DESC_NUM) {
+		dev_err(d->dev, "Channel %d descriptor number out of range %d\n",
+			c->nr, desc_num);
+		return -EINVAL;
+	}

Quoted 8 lines

drivers/dma/lgm/lgm-dma.c line 707

Quoted 6 lines

drivers/dma/lgm/lgm-dma.c new file +6 -0

+
+	ldma_chan_desc_hw_cfg(c, desc_base, desc_num);
+
+	c->flags |= DMA_HW_DESC;
+	c->desc_cnt = desc_num;
+	c->desc_phys = desc_base;

So you have a custom API which is used to configure this flag, a number and an address. The question is why, can you please help explain this?

LDMA used as general purpose dma(ver == DMA_VER22) and also supports DMA capability for GSWIP in their network packet processing.( ver > DMA_VER22)

Quoted 4 lines

Whats GSWIP?

Quoted 1 line

Each Ingress(IGP) & Egress(EGP) ports of CQM use a dma channel.

CQM?

GSWIP stands for  Gigabit Switch IP, and CQM is Central Queue Manager.

GSWIP & CQM are the clients for the DMA. These are used in networking purpose to increase transfer rates for peripheral like the GSWIP LAN switch.

Quoted 4 lines

Quoted 3 lines

desc needs to be configure for each dma channel and the remapped address of the IGP & EGP is desc base adress.

Why should this address not passed as src_addr/dst_addr?

src_addr/dst_addr is the data pointer. Data pointer indicates address pointer of data buffer.

ldma_chan_desc_cfg() carries the descriptor address.

The descriptor list entry contains the data pointer, which points to the data section in the memory.

So we should not use src_addr/dst_addr as desc base address.

Quoted 16 lines

Quoted 16 lines

CQM client is using ldma_chan_desc_cfg() to configure the descriptior.

Quoted 8 lines

drivers/dma/lgm/lgm-dma.c line 1052

Quoted 7 lines

drivers/dma/lgm/lgm-dma.c new file +7 -0

+static void dma_issue_pending(struct dma_chan *chan)
+{
+	struct ldma_chan *c = to_ldma_chan(chan);
+	struct ldma_dev *d = to_ldma_dev(c->vchan.chan.device);
+	unsigned long flags;
+
+	if (d->ver == DMA_VER22) {

why is this specific to this version?

Only dma0 instance (ver == DMA_VER22) is used as a general purpose slave DMA. we set both control and datapath here.
Other instances (ver > DMA_VER22) we set only control path. data path is taken care by dma client(GSWIP).
Only thing needs to do is get the channel, set the descriptor and just 'ON' the channel.

Quoted 5 lines

Quoted 2 lines

CQM is highly low level register configurable/programmable take care about the the packet processing through the register configurations.

DMAengine fwk take care of channel management for clients and transaction management, if you not going to do transactions then why bother with dmaengine ?

dma0 instance (ver == DMA_VER22) uses DMAengine framework for both channel management and transaction management.

Other instances (ver > DMA_VER22) uses DMAengine mainly for channel management.
To initiate the transaction client needs to ON the corresponding channel, So dmaengine ops are using to 'ON'  and 'OFF' the channels.

Hi Vinod,

Thanks for your valuable review. My comments inline.

Quoted 137 lines · Vinod Koul

Quoted 136 lines · MallikarjunaX

Hi Vinod,
Thanks for the review. My comments inline.

Quoted 114 lines · Vinod Koul

Quoted 113 lines · Amireddy Mallikarjuna reddy

Add DT bindings YAML schema for DMA controller driver of Lightning Mountain (LGM) SoC.

Signed-off-by Amireddy Mallikarjuna reddy <[email protected]>

---
v1:
- Initial version.

v2:
- Fix bot errors.

v3:
- No change.

v4:
- Address Thomas langer comments
- use node name pattern as dma-controller as in common binding.
- Remove "_" (underscore) in instance name.
- Remove "port-" and "chan-" in attribute name for both 'dma-ports' & 'dma-channels' child nodes.

v5:
- Moved some of the attributes in 'dma-ports' & 'dma-channels' child nodes to dma client/consumer side as cells in 'dmas' properties.

v6:
- Add additionalProperties: false
- completely removed 'dma-ports' and 'dma-channels' child nodes.
- Moved channel dt properties to client side dmas.
- Use standard dma-channels and dma-channel-mask properties.
- Documented reset-names
- Add description for dma-cells

v7:
- modified compatible to oneof
- Reduced number of dma-cells to 3
- Fine tune the description of some properties.

v7-resend:
- rebase to 5.10-rc1

v8:
- rebased to 5.10-rc3
- Fixing the bot issues (wrong indentation)

v9:
- rebased to 5.10-rc3
- Use 'enum' instead of oneOf+const
- Drop '#dma-cells' in required:, already covered in dma-common.yaml
- Drop nodename Already covered by dma-controller.yaml

---
   .../devicetree/bindings/dma/intel,ldma.yaml        | 130 +++++++++++++++++++++
   1 file changed, 130 insertions(+)

create mode 100644 Documentation/devicetree/bindings/dma/intel,ldma.yaml

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +54 -0

new file mode 100644
index 000000000000..c06281a10178
@@ -0,0 +1,130 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/intel,ldma.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Lightning Mountain centralized low speed DMA and high speed DMA controllers.
+
+maintainers:
+  - [email protected]
+  - [email protected]
+
+allOf:
+  - $ref: "dma-controller.yaml#"
+
+properties:
+  compatible:
+    enum:
+      - intel,lgm-cdma
+      - intel,lgm-dma2tx
+      - intel,lgm-dma1rx
+      - intel,lgm-dma1tx
+      - intel,lgm-dma0tx
+      - intel,lgm-dma3
+      - intel,lgm-toe-dma30
+      - intel,lgm-toe-dma31
+
+  reg:
+    maxItems: 1
+
+  "#dma-cells":
+    const: 3
+    description:
+      The first cell is the peripheral's DMA request line.
+      The second cell is the peripheral's (port) number corresponding to the channel.
+      The third cell is the burst length of the channel.
+
+  dma-channels:
+    minimum: 1
+    maximum: 16
+
+  dma-channel-mask:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  reset-names:
+    items:
+      - const: ctrl
+

Quoted 8 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml line 61

Quoted 7 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +7 -0

+  interrupts:
+    maxItems: 1
+
+  intel,dma-poll-cnt:
+    $ref: /schemas/types.yaml#definitions/uint32
+    description:
+      DMA descriptor polling counter is used to control the poling mechanism

s/poling/polling

Ok, Thanks.

Quoted 9 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml line 69

Quoted 8 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +8 -0

+      for the descriptor fetching for all channels.
+
+  intel,dma-byte-en:
+    type: boolean
+    description:
+      DMA byte enable is only valid for DMA write(RX).
+      Byte enable(1) means DMA write will be based on the number of dwords
+      instead of the whole burst.

Can you explain this, also sounds you could use _maxburst values..?

Quoted 8 lines

Quoted 7 lines

when dma-byte-en = 0 (disabled) DMA write will be in terms of burst length, dma-byte-en = 1 (enabled) write will be in terms of Dwords.

Byte enable = 0 (Disabled) means that DMA write will be based on the burst length, even if it only transmits one byte.
Byte enable = 1(enabled) means that DMA write will be based on the number of Dwords, instead of the whole burst.

Sounds like a hw property or is this configurable to engine..?

Yes its hw property. Not configurable to engine.

Quoted 21 lines

Quoted 21 lines

Quoted 4 lines

Quoted 4 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +4 -0

+
+  intel,dma-drb:
+    type: boolean
+    description:

Quoted 8 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml line 80

Quoted 7 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +7 -0

+      DMA descriptor read back to make sure data and desc synchronization.
+
+  intel,dma-desc-in-sram:
+    type: boolean
+    description:
+      DMA descritpors in SRAM or not. Some old controllers descriptors
+      can be in DRAM or SRAM. The new ones are all in SRAM.

should that not be decided by driver..? Or is this a hw property?

This is DMA controller capability. It can be decided from driver also. i will change accordingly.

Quoted 7 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml line 86

Quoted 6 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +6 -0

+
+  intel,dma-orrc:
+    $ref: /schemas/types.yaml#definitions/uint32
+    description:
+      DMA outstanding read counter value determine the number of
+      ORR-Outstanding Read Request. The maximum value is 16.

How would this be used by folks..?

Quoted 8 lines

Quoted 7 lines

A register bit will be used to enable/disable the ORR feature.

Outstanding Read Capability introduce CMD FIFO to support up to 16 outstanding reads for different packet in same channel.

For large packets up to 16 OR can be issued, the number of OR is configurable.

How will configure this and when..?

This is DMA (ver > DMA_VER22) hw capability and is configured from device tree.

If this property is not present or count is zero means orrc capability is disabled.
If orrc count is 4 <= orr_cnt < 16 then write the enable bit and value to corresponding register.

Ex:
        if (d->orrc > 0 && d->orrc <= DMA_ORRC_MAX_CNT)
                val = DMA_ORRC_EN | FIELD_PREP(DMA_ORRC_ORRCNT, d->orrc);

        ldma_update_bits(d, mask, val, DMA_ORRC);

This hw capability supports dma instances ver > DMA_VER22.

Quoted 111 lines · MallikarjunaX

Hi Vinod,

Thanks for your valuable review comments. Please see my comments inline.

Quoted 22 lines · Vinod Koul

Quoted 21 lines · MallikarjunaX

Hi Vinod,

Thanks for the review. My comments inline.

Quoted 7 lines · Vinod Koul

Quoted 6 lines · Amireddy Mallikarjuna reddy

Add DMA controller driver for Lightning Mountain (LGM) family of SoCs.

The main function of the DMA controller is the transfer of data from/to any peripheral to/from the memory. A memory to memory copy capability can also be configured.

Quoted 8 lines

Quoted 7 lines

This ldma driver is used for configure the device and channnels for data and control paths.

Signed-off-by Amireddy Mallikarjuna reddy <[email protected]>

---
v1:
- Initial version.

You have a cover letter, use that to keep track of these changes

ok.

Quoted 8 lines

Quoted 7 lines

Quoted 6 lines

drivers/dma/lgm/Kconfig line 3

Quoted 5 lines

+++ b/drivers/dma/lgm/Kconfig

drivers/dma/lgm/Kconfig new file +3 -0

@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config INTEL_LDMA
+	bool "Lightning Mountain centralized low speed DMA and high speed DMA controllers"

Do we have any other speeds :D

No other speeds :-)

Right, so possibly drop the speed characterization here!

"Lightning Mountain centralized DMA controller"

Quoted 75 lines

Quoted 71 lines

Quoted 8 lines

drivers/dma/lgm/lgm-dma.c line 5

Quoted 7 lines

drivers/dma/lgm/lgm-dma.c new file +6 -0

+++ b/drivers/dma/lgm/lgm-dma.c

@@ -0,0 +1,1742 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Lightning Mountain centralized low speed and high speed DMA controller driver
+ *
+ * Copyright (c) 2016 ~ 2020 Intel Corporation.

I think you mean 2016 - 2020, a dash which refers to duration

ok.

Quoted 14 lines

drivers/dma/lgm/lgm-dma.c line 264

Quoted 11 lines

drivers/dma/lgm/lgm-dma.c new file +11 -0

+struct dw2_desc {
+	struct {
+		u32 len		:16;
+		u32 res0	:7;
+		u32 bofs	:2;
+		u32 res1	:3;
+		u32 eop		:1;
+		u32 sop		:1;
+		u32 c		:1;
+		u32 own		:1;
+	} __packed field;

Another one, looks like folks adding dmaengine patches love this approach, second one for the day..

Quoted 3 lines

Now why do you need the bit fields, why not use register defines and helpers in bitfield.h to help configure the fields See FIELD_GET, FIELD_PREP etc

Let me check on this...

Quoted 11 lines

Quoted 11 lines

drivers/dma/lgm/lgm-dma.c new file +11 -0

+struct dma_dev_ops {
+	int (*device_alloc_chan_resources)(struct dma_chan *chan);
+	void (*device_free_chan_resources)(struct dma_chan *chan);
+	int (*device_config)(struct dma_chan *chan,
+			     struct dma_slave_config *config);
+	int (*device_pause)(struct dma_chan *chan);
+	int (*device_resume)(struct dma_chan *chan);
+	int (*device_terminate_all)(struct dma_chan *chan);
+	void (*device_synchronize)(struct dma_chan *chan);
+	enum dma_status (*device_tx_status)(struct dma_chan *chan,
+					    dma_cookie_t cookie,

Quoted 8 lines

drivers/dma/lgm/lgm-dma.c line 294

Quoted 7 lines

drivers/dma/lgm/lgm-dma.c new file +7 -0

+					    struct dma_tx_state *txstate);
+	struct dma_async_tx_descriptor *(*device_prep_slave_sg)
+		(struct dma_chan *chan, struct scatterlist *sgl,
+		unsigned int sg_len, enum dma_transfer_direction direction,
+		unsigned long flags, void *context);
+	void (*device_issue_pending)(struct dma_chan *chan);
+};

Heh! why do you have a copy of dmaengine ops here?

Ok, i will remove the ops and update the code accordingly.

Quoted 16 lines

Quoted 16 lines

drivers/dma/lgm/lgm-dma.c new file +16 -0

+static int ldma_chan_desc_cfg(struct ldma_chan *c, dma_addr_t desc_base,
+			      int desc_num)
+{
+	struct ldma_dev *d = to_ldma_dev(c->vchan.chan.device);
+
+	if (!desc_num) {
+		dev_err(d->dev, "Channel %d must allocate descriptor first\n",
+			c->nr);
+		return -EINVAL;
+	}
+
+	if (desc_num > DMA_MAX_DESC_NUM) {
+		dev_err(d->dev, "Channel %d descriptor number out of range %d\n",
+			c->nr, desc_num);
+		return -EINVAL;
+	}

Quoted 8 lines

drivers/dma/lgm/lgm-dma.c line 707

Quoted 6 lines

drivers/dma/lgm/lgm-dma.c new file +6 -0

+
+	ldma_chan_desc_hw_cfg(c, desc_base, desc_num);
+
+	c->flags |= DMA_HW_DESC;
+	c->desc_cnt = desc_num;
+	c->desc_phys = desc_base;

So you have a custom API which is used to configure this flag, a number and an address. The question is why, can you please help explain this?

Quoted 2 lines

LDMA used as general purpose dma(ver == DMA_VER22) and also supports DMA capability for GSWIP in their network packet processing.( ver > DMA_VER22)

Whats GSWIP?

Quoted 6 lines

Quoted 2 lines

Quoted 1 line

Each Ingress(IGP) & Egress(EGP) ports of CQM use a dma channel.

CQM?

GSWIP stands for  Gigabit Switch IP, and CQM is Central Queue Manager.

GSWIP & CQM are the clients for the DMA. These are used in networking purpose to increase transfer rates for peripheral like the GSWIP LAN switch.

Please do add that when using these terms, folks outside Intel may not be aware of these terms.

Quoted 7 lines

Quoted 4 lines

Quoted 3 lines

desc needs to be configure for each dma channel and the remapped address of the IGP & EGP is desc base adress.

Why should this address not passed as src_addr/dst_addr?

src_addr/dst_addr is the data pointer. Data pointer indicates address pointer of data buffer.

Quoted 6 lines

ldma_chan_desc_cfg() carries the descriptor address.

The descriptor list entry contains the data pointer, which points to the data section in the memory.

So we should not use src_addr/dst_addr as desc base address.

Okay sounds reasonable. why is this using in API here?

Quoted 25 lines

Quoted 17 lines

Quoted 17 lines

CQM client is using ldma_chan_desc_cfg() to configure the descriptior.

Quoted 8 lines

drivers/dma/lgm/lgm-dma.c line 1052

Quoted 7 lines

drivers/dma/lgm/lgm-dma.c new file +7 -0

+static void dma_issue_pending(struct dma_chan *chan)
+{
+	struct ldma_chan *c = to_ldma_chan(chan);
+	struct ldma_dev *d = to_ldma_dev(c->vchan.chan.device);
+	unsigned long flags;
+
+	if (d->ver == DMA_VER22) {

why is this specific to this version?

Only dma0 instance (ver == DMA_VER22) is used as a general purpose slave DMA. we set both control and datapath here.
Other instances (ver > DMA_VER22) we set only control path. data path is taken care by dma client(GSWIP).
Only thing needs to do is get the channel, set the descriptor and just 'ON' the channel.

Quoted 5 lines

Quoted 2 lines

CQM is highly low level register configurable/programmable take care about the the packet processing through the register configurations.

DMAengine fwk take care of channel management for clients and transaction management, if you not going to do transactions then why bother with dmaengine ?

dma0 instance (ver == DMA_VER22) uses DMAengine framework for both channel management and transaction management.

Quoted 4 lines

Other instances (ver > DMA_VER22) uses DMAengine mainly for channel management.
To initiate the transaction client needs to ON the corresponding channel, So dmaengine ops are using to 'ON'  and 'OFF' the channels.

Is the addresses for transactions all hardcoded? WHo configures the transaction in (ver > DMA_VER22)

Quoted 188 lines · MallikarjunaX

Hi Vinod,

Thanks for your valuable review. My comments inline.

Quoted 137 lines · Vinod Koul

Quoted 136 lines · MallikarjunaX

Hi Vinod,
Thanks for the review. My comments inline.

Quoted 114 lines · Vinod Koul

Quoted 113 lines · Amireddy Mallikarjuna reddy

Add DT bindings YAML schema for DMA controller driver of Lightning Mountain (LGM) SoC.

Signed-off-by Amireddy Mallikarjuna reddy <[email protected]>

---
v1:
- Initial version.

v2:
- Fix bot errors.

v3:
- No change.

v4:
- Address Thomas langer comments
- use node name pattern as dma-controller as in common binding.
- Remove "_" (underscore) in instance name.
- Remove "port-" and "chan-" in attribute name for both 'dma-ports' & 'dma-channels' child nodes.

v5:
- Moved some of the attributes in 'dma-ports' & 'dma-channels' child nodes to dma client/consumer side as cells in 'dmas' properties.

v6:
- Add additionalProperties: false
- completely removed 'dma-ports' and 'dma-channels' child nodes.
- Moved channel dt properties to client side dmas.
- Use standard dma-channels and dma-channel-mask properties.
- Documented reset-names
- Add description for dma-cells

v7:
- modified compatible to oneof
- Reduced number of dma-cells to 3
- Fine tune the description of some properties.

v7-resend:
- rebase to 5.10-rc1

v8:
- rebased to 5.10-rc3
- Fixing the bot issues (wrong indentation)

v9:
- rebased to 5.10-rc3
- Use 'enum' instead of oneOf+const
- Drop '#dma-cells' in required:, already covered in dma-common.yaml
- Drop nodename Already covered by dma-controller.yaml

---
    .../devicetree/bindings/dma/intel,ldma.yaml        | 130 +++++++++++++++++++++
    1 file changed, 130 insertions(+)

create mode 100644 Documentation/devicetree/bindings/dma/intel,ldma.yaml

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +54 -0

new file mode 100644
index 000000000000..c06281a10178
@@ -0,0 +1,130 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/intel,ldma.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Lightning Mountain centralized low speed DMA and high speed DMA controllers.
+
+maintainers:
+  - [email protected]
+  - [email protected]
+
+allOf:
+  - $ref: "dma-controller.yaml#"
+
+properties:
+  compatible:
+    enum:
+      - intel,lgm-cdma
+      - intel,lgm-dma2tx
+      - intel,lgm-dma1rx
+      - intel,lgm-dma1tx
+      - intel,lgm-dma0tx
+      - intel,lgm-dma3
+      - intel,lgm-toe-dma30
+      - intel,lgm-toe-dma31
+
+  reg:
+    maxItems: 1
+
+  "#dma-cells":
+    const: 3
+    description:
+      The first cell is the peripheral's DMA request line.
+      The second cell is the peripheral's (port) number corresponding to the channel.
+      The third cell is the burst length of the channel.
+
+  dma-channels:
+    minimum: 1
+    maximum: 16
+
+  dma-channel-mask:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  reset-names:
+    items:
+      - const: ctrl
+

Quoted 8 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml line 61

Quoted 7 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +7 -0

+  interrupts:
+    maxItems: 1
+
+  intel,dma-poll-cnt:
+    $ref: /schemas/types.yaml#definitions/uint32
+    description:
+      DMA descriptor polling counter is used to control the poling mechanism

s/poling/polling

Ok, Thanks.

Quoted 9 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml line 69

Quoted 8 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +8 -0

+      for the descriptor fetching for all channels.
+
+  intel,dma-byte-en:
+    type: boolean
+    description:
+      DMA byte enable is only valid for DMA write(RX).
+      Byte enable(1) means DMA write will be based on the number of dwords
+      instead of the whole burst.

Can you explain this, also sounds you could use _maxburst values..?

Quoted 8 lines

Quoted 7 lines

when dma-byte-en = 0 (disabled) DMA write will be in terms of burst length, dma-byte-en = 1 (enabled) write will be in terms of Dwords.

Byte enable = 0 (Disabled) means that DMA write will be based on the burst length, even if it only transmits one byte.
Byte enable = 1(enabled) means that DMA write will be based on the number of Dwords, instead of the whole burst.

Sounds like a hw property or is this configurable to engine..?

Yes its hw property. Not configurable to engine.

Quoted 21 lines

Quoted 21 lines

Quoted 4 lines

Quoted 4 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +4 -0

+
+  intel,dma-drb:
+    type: boolean
+    description:

Quoted 8 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml line 80

Quoted 7 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +7 -0

+      DMA descriptor read back to make sure data and desc synchronization.
+
+  intel,dma-desc-in-sram:
+    type: boolean
+    description:
+      DMA descritpors in SRAM or not. Some old controllers descriptors
+      can be in DRAM or SRAM. The new ones are all in SRAM.

should that not be decided by driver..? Or is this a hw property?

This is DMA controller capability. It can be decided from driver also. i will change accordingly.

Quoted 7 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml line 86

Quoted 6 lines

Documentation/devicetree/bindings/dma/intel,ldma.yaml new file +6 -0

+
+  intel,dma-orrc:
+    $ref: /schemas/types.yaml#definitions/uint32
+    description:
+      DMA outstanding read counter value determine the number of
+      ORR-Outstanding Read Request. The maximum value is 16.

How would this be used by folks..?

Quoted 8 lines

Quoted 7 lines

A register bit will be used to enable/disable the ORR feature.

Outstanding Read Capability introduce CMD FIFO to support up to 16 outstanding reads for different packet in same channel.

For large packets up to 16 OR can be issued, the number of OR is configurable.

How will configure this and when..?

This is DMA (ver > DMA_VER22) hw capability and is configured from device tree.

If this property is not present or count is zero means orrc capability is disabled.
If orrc count is 4 <= orr_cnt < 16 then write the enable bit and value to corresponding register.

Quoted 7 lines

Ex:
        if (d->orrc > 0 && d->orrc <= DMA_ORRC_MAX_CNT)
                val = DMA_ORRC_EN | FIELD_PREP(DMA_ORRC_ORRCNT, d->orrc);

        ldma_update_bits(d, mask, val, DMA_ORRC);

This hw capability supports dma instances ver > DMA_VER22.

Sounds like this can be coded in driver and used based on compatible?

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

Quoted 2 lines

desc needs to be configure for each dma channel and the remapped address of the IGP & EGP is desc base adress.

Why should this address not passed as src_addr/dst_addr?

src_addr/dst_addr is the data pointer. Data pointer indicates address pointer of data buffer.

Quoted 7 lines

Quoted 6 lines

ldma_chan_desc_cfg() carries the descriptor address.

The descriptor list entry contains the data pointer, which points to the data section in the memory.

So we should not use src_addr/dst_addr as desc base address.

Okay sounds reasonable. why is this using in API here?

descriptor base address needs to be write into the dma register (DMA_CDBA).

Quoted 8 lines · MallikarjunaX

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

Quoted 2 lines

desc needs to be configure for each dma channel and the remapped address of the IGP & EGP is desc base adress.

Why should this address not passed as src_addr/dst_addr?

src_addr/dst_addr is the data pointer. Data pointer indicates address pointer of data buffer.

ldma_chan_desc_cfg() carries the descriptor address.

Quoted 6 lines

Quoted 5 lines

Quoted 4 lines

The descriptor list entry contains the data pointer, which points to the data section in the memory.

So we should not use src_addr/dst_addr as desc base address.

Okay sounds reasonable. why is this using in API here?

descriptor base address needs to be write into the dma register (DMA_CDBA).

Why cant descriptor be allocated by damenegine driver, passed to client as we normally do in prep_* callbacks ? Why do you need a custom API