Re: [PATCH v11 0/2] Add Intel LGM SoC DMA support
Ok Vinod. Thanks, Mallikarjuna reddy A On 1/17/2021 1:57 PM, Vinod Koul wrote: On 15-01-21, 17:56, Amireddy Mallikarjuna reddy wrote: 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. These controllers provide DMA capabilities for a variety of on-chip devices such as SSC, HSNAND and GSWIP (Gigabit Switch IP). - Future Plans: - LGM SOC also supports Hardware Memory Copy engine. The role of the HW Memory copy engine is to offload memory copy operations from the CPU. ?? Please send updates against already applied patches and not an updated series!
Re: [RESEND PATCH v10 1/2] dt-bindings: dma: Add bindings for Intel LGM SoC
Sure Rob. Thanks, Mallikarjuna reddy A On 1/15/2021 4:25 AM, Rob Herring wrote: On Tue, Dec 15, 2020 at 10:08 PM Amireddy Mallikarjuna reddy wrote: Add DT bindings YAML schema for DMA controller driver of Lightning Mountain (LGM) SoC. Signed-off-by: Amireddy Mallikarjuna reddy Reviewed-by: Rob Herring --- 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 - No change. v8: - rebased to 5.10-rc3 - Fixing the bot issues (wrong indentation) v9: - 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 v10: - rebased to 5.10-rc6 - Add Reviewed-by: Rob Herring - Fixed typo. - moved property dma-desc-in-sram to driver side. - Moved property dma-orrc to driver side. v10-resend: - rebased to 5.10 - No change --- .../devicetree/bindings/dma/intel,ldma.yaml | 116 ++ 1 file changed, 116 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/intel,ldma.yaml diff --git a/Documentation/devicetree/bindings/dma/intel,ldma.yaml b/Documentation/devicetree/bindings/dma/intel,ldma.yaml new file mode 100644 index ..866d4c758a7a --- /dev/null +++ b/Documentation/devicetree/bindings/dma/intel,ldma.yaml @@ -0,0 +1,116 @@ +# 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 DMA controllers. + +maintainers: + - chuanhua@intel.com + - mallikarjunax.re...@intel.com + +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 + + interrupts: +maxItems: 1 + + intel,dma-poll-cnt: +$ref: /schemas/types.yaml#definitions/uint32 Since this was sent, there have been some fixes for JSON pointers and this is missing a '/'. The tools now check: /builds/robherring/linux-dt-bindings/Documentation/devicetree/bindings/dma/intel,ldma.yaml: properties:intel,dma-poll-cnt: 'oneOf' conditional failed, one must be fixed: 'enum' is a required property 'const' is a required property '/schemas/types.yaml#definitions/uint32' does not match 'types.yaml#/definitions/' Please send a fix for this. Thanks, Rob
Re: [PATCH v9 2/2] Add Intel LGM SoC DMA support.
Hi Vinod,
Thanks for your valuable comments. My reply inline.
On 11/26/2020 12:50 PM, Vinod Koul wrote:
On 25-11-20, 18:39, Reddy, MallikarjunaX wrote:
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.
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
1)
client needs to set the descriptor base address and also number of
descriptors used in the descriptor list.
reg DMA_CDBA used to configure descriptor base address and reg DMA_CDLEN
used to configure number of descriptors used in the descriptor list.
In case of (ver > DMA_VER22) all descriptor fields and data pointer will
be set by client, so we just need to write desc base and num desc length
in to corresponding registers from the driver side.
dma_async_tx_descriptor * data is not really needed from driver to
client side , so i am not planned to return 'struct
dma_async_tx_descriptor *'.
because of this reason i used custom API (return -Ve for error and ZERO
for success) instead of standard dmaengine_prep_slave_sg() callback
(return 'struct dma_async_tx_descriptor *' descriptor)
2)
We can also use the dmaengine_prep_slave_sg( ) to pass desc base addr &
desc number from client.
In that case we have to use (sg)->dma_address as desc base address and
(sg)->length as desc length.
dmaengine prep_* callback return 'struct dma_async_tx_descriptor *, this
can be used on client side as to check prep_* callback SUCCESS/FAIL.
Example:
/* code snippet */
static struct dma_async_tx_descriptor *
ldma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
unsigned int sglen, enum dma_transfer_direction dir,
unsigned long flags, void *context)
{
.
if (d->ver > DMA_VER22)
return ldma_chan_desc_cfg(chan, sgl->dma_address, sglen);
.
}
static struct dma_async_tx_descriptor *
ldma_chan_desc_cfg(struct dma_chan *chan, dma_addr_t desc_base, int
desc_num)
{
struct ldma_chan *c = to_ldma_chan(chan);
struct ldma_dev *d = to_ldma_dev(c->vchan.chan.device);
struct dma_async_tx_descriptor *tx;
struct dw2_desc_sw *ds;
if (!desc_num) {
dev_err(d->dev, "Channel %d must allocate descriptor
first\n",
c->nr);
return NULL;
}
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 NULL;
}
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;
ds = kzalloc(sizeof(*ds), GFP_NOWAIT);
if (!ds)
return NULL;
tx = >vdesc.tx;
dma_async_tx_descriptor_init(tx, chan);
return tx;
}
Please let me know if this is OK, So that i will include in the next patch.
Re: [PATCH v9 1/2] dt-bindings: dma: Add bindings for Intel LGM SoC
Hi Vinod, Thanks for your review comments, My comments inline. On 11/25/2020 1:23 AM, Vinod Koul wrote: On 24-11-20, 00:30, Reddy, MallikarjunaX wrote: Hi Vinod, Thanks for your valuable review. My comments inline. On 11/21/2020 8:19 PM, Vinod Koul wrote: On 20-11-20, 19:30, Reddy, MallikarjunaX wrote: Hi Vinod, Thanks for the review. My comments inline. On 11/18/2020 11:55 PM, Vinod Koul wrote: On 12-11-20, 13:38, Amireddy Mallikarjuna reddy wrote: Add DT bindings YAML schema for DMA controller driver of Lightning Mountain (LGM) SoC. Signed-off-by: Amireddy Mallikarjuna reddy --- 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 diff --git a/Documentation/devicetree/bindings/dma/intel,ldma.yaml b/Documentation/devicetree/bindings/dma/intel,ldma.yaml new file mode 100644 index ..c06281a10178 --- /dev/null +++ b/Documentation/devicetree/bindings/dma/intel,ldma.yaml @@ -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: + - chuanhua@intel.com + - mallikarjunax.re...@intel.com + +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 + + 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. + 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..? 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. + + 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. + +
Re: [PATCH v9 2/2] Add Intel LGM SoC DMA support.
Hi Vinod,
Thanks for the review comments, my comments inline.
On 11/25/2020 1:21 AM, Vinod Koul wrote:
On 24-11-20, 00:29, Reddy, MallikarjunaX wrote:
Hi Vinod,
Thanks for your valuable review comments. Please see my comments inline.
On 11/21/2020 8:17 PM, Vinod Koul wrote:
On 20-11-20, 19:30, Reddy, MallikarjunaX wrote:
Hi Vinod,
Thanks for the review. My comments inline.
On 11/19/2020 1:38 AM, Vinod Koul wrote:
On 12-11-20, 13:38, Amireddy Mallikarjuna reddy wrote:
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
---
v1:
- Initial version.
You have a cover letter, use that to keep track of these changes
ok.
+++ b/drivers/dma/lgm/Kconfig
@@ -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"
+++ 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.
+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
Let me check on this...
+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,
+ 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.
+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;
+ }
+
+ 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)
Whats GSWIP?
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.
Ok
desc needs to be configure for each dma channel and the remapped address of
the IGP &
Re: [PATCH v9 1/2] dt-bindings: dma: Add bindings for Intel LGM SoC
Hi Vinod, Thanks for your valuable review. My comments inline. On 11/21/2020 8:19 PM, Vinod Koul wrote: On 20-11-20, 19:30, Reddy, MallikarjunaX wrote: Hi Vinod, Thanks for the review. My comments inline. On 11/18/2020 11:55 PM, Vinod Koul wrote: On 12-11-20, 13:38, Amireddy Mallikarjuna reddy wrote: Add DT bindings YAML schema for DMA controller driver of Lightning Mountain (LGM) SoC. Signed-off-by: Amireddy Mallikarjuna reddy --- 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 diff --git a/Documentation/devicetree/bindings/dma/intel,ldma.yaml b/Documentation/devicetree/bindings/dma/intel,ldma.yaml new file mode 100644 index ..c06281a10178 --- /dev/null +++ b/Documentation/devicetree/bindings/dma/intel,ldma.yaml @@ -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: + - chuanhua@intel.com + - mallikarjunax.re...@intel.com + +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 + + 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. + 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..? 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. + + 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. + + intel,dma-orrc: +$ref: /schemas/types.yaml#definitions/uint32 +description: + DMA outstanding read counter value determine the number of + ORR-Out
Re: [PATCH v9 2/2] Add Intel LGM SoC DMA support.
Hi Vinod,
Thanks for your valuable review comments. Please see my comments inline.
On 11/21/2020 8:17 PM, Vinod Koul wrote:
On 20-11-20, 19:30, Reddy, MallikarjunaX wrote:
Hi Vinod,
Thanks for the review. My comments inline.
On 11/19/2020 1:38 AM, Vinod Koul wrote:
On 12-11-20, 13:38, Amireddy Mallikarjuna reddy wrote:
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
---
v1:
- Initial version.
You have a cover letter, use that to keep track of these changes
ok.
+++ b/drivers/dma/lgm/Kconfig
@@ -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"
+++ 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.
+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
Let me check on this...
+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,
+ 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.
+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;
+ }
+
+ 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)
Whats GSWIP?
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.
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 c
Re: [PATCH v9 1/2] dt-bindings: dma: Add bindings for Intel LGM SoC
Hi Vinod, Thanks for the review. My comments inline. On 11/18/2020 11:55 PM, Vinod Koul wrote: On 12-11-20, 13:38, Amireddy Mallikarjuna reddy wrote: Add DT bindings YAML schema for DMA controller driver of Lightning Mountain (LGM) SoC. Signed-off-by: Amireddy Mallikarjuna reddy --- 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 diff --git a/Documentation/devicetree/bindings/dma/intel,ldma.yaml b/Documentation/devicetree/bindings/dma/intel,ldma.yaml new file mode 100644 index ..c06281a10178 --- /dev/null +++ b/Documentation/devicetree/bindings/dma/intel,ldma.yaml @@ -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: + - chuanhua@intel.com + - mallikarjunax.re...@intel.com + +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 + + 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. + 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..? 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. + + 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. + + 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..? 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
Re: [PATCH v9 2/2] Add Intel LGM SoC DMA support.
Hi Vinod,
Thanks for the review. My comments inline.
On 11/19/2020 1:38 AM, Vinod Koul wrote:
On 12-11-20, 13:38, Amireddy Mallikarjuna reddy wrote:
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
---
v1:
- Initial version.
You have a cover letter, use that to keep track of these changes
ok.
+++ b/drivers/dma/lgm/Kconfig
@@ -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 :-)
+++ 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.
+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
Let me check on this...
+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,
+ 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.
+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;
+ }
+
+ 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.
+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.
+static enum dma_status
Re: [PATCH v7 1/2] dt-bindings: dma: Add bindings for intel LGM SOC
Hi Thomas, On 11/11/2020 1:39 AM, Thomas Langer wrote: -Original Message- From: Reddy, MallikarjunaX Sent: Montag, 2. November 2020 15:42 To: Thomas Langer ; dmaeng...@vger.kernel.org; vk...@kernel.org; devicet...@vger.kernel.org; robh...@kernel.org Cc: linux-kernel@vger.kernel.org; Shevchenko, Andriy ; chuanhua@linux.intel.com; Kim, Cheol Yong ; Wu, Qiming ; malliamireddy...@gmail.com; peter.ujfal...@ti.com; Langer, Thomas Subject: Re: [PATCH v7 1/2] dt-bindings: dma: Add bindings for intel LGM SOC This email was sent from outside of MaxLinear. Hi Thomas, Thanks for the review, my comments inline. On 10/28/2020 3:24 AM, Thomas Langer wrote: Hello Reddy, I think "Intel" should always be written with a capital "I" (like in the Subject, but except in the binding below) OK. + compatible: + oneOf: + - const: intel,lgm-cdma + - const: intel,lgm-dma2tx + - const: intel,lgm-dma1rx + - const: intel,lgm-dma1tx + - const: intel,lgm-dma0tx + - const: intel,lgm-dma3 + - const: intel,lgm-toe-dma30 + - const: intel,lgm-toe-dma31 Bindings are normally not per instance. What if next generation chip gets more DMA modules but has no other changes in the HW block? What is wrong with - const: intel,lgm-cdma - const: intel,lgm-hdma and extra attributes to define the rx/tx restriction (or what do it mean?)? From the driver code I saw that "toe" is also just of type "hdma" and no further differences in code are done. We had a discussion on the same in the previous patches and Rob Herring said Okay using Different compatibles. below the snippet. ++ + >>> + compatible: >>> + anyOf: >>> + - const: intel,lgm-cdma >>> + - const: intel,lgm-dma2tx >>> + - const: intel,lgm-dma1rx >>> + - const: intel,lgm-dma1tx >>> + - const: intel,lgm-dma0tx >>> + - const: intel,lgm-dma3 >>> + - const: intel,lgm-toe-dma30 >>> + - const: intel,lgm-toe-dma31 >> Please explain why you need so many different compatible strings. > This hw dma has 7 DMA instances. > Some for datapath, some for memcpy and some for TOE. > Some for TX only, some for RX only, and some for TX/RX(memcpy and ToE). > > dma TX/RX type we considered as driver specific data of each instance and > used different compatible strings for each instance. > And also idea is in future if any driver specific data of any particular > instance we can handle. > > Here if dma name and type(tx or rx) will be accepted as devicetree > attributes then we can move .name = "toe_dma31", & .type = DMA_TYPE_MCPY > to devicetree. So that the compatible strings can be limited to two. > intel,lgm-cdma & intel,lgm-hdma . [Rob] Different compatibles are okay if the instances are different and we don't have properties to describe the differences. Okay, but then explain what the differences are, that cannot be described by other properties/attributes. In the driver code I cannot see anything, except the "name". But for printouts in driver, "drv_dbg" or similar will just use the node path for the instance. On patch4 series we had the same discussion. i will brief it here again. This hw dma has 7 DMA instances, and each Some for TX only, some for RX only, and some for TX/RX. dma TX/RX type we considered as driver specific data and it cant be used as dt property as per the previous reviewers. So i moved it to driver specific data. If type(tx or rx) will be accepted as devicetree attributes then we can move it to devicetree. So as you said we can limit compatible strings can be limited to two. intel,lgm-cdma & intel,lgm-hdma . One more advantage i see with this model is in future if any driver specific data of any particular instance we can handle easily. For some of what you have in this binding, I think it should be part of the consumer cells. ++ ++ Best regards, Thomas
Re: [PATCH v1 1/2] dt-bindings: leds: Add bindings for intel LGM SOC
Hi Pavel, Thanks for your valuable comments, Please see my comments inline. On 11/5/2020 5:52 PM, Pavel Machek wrote: On Thu 2020-11-05 17:43:50, Amireddy Mallikarjuna reddy wrote: Add DT bindings YAML schema for SSO controller driver of Lightning Mountain(LGM) SoC. intel -> Intel in the title. "Lightning Mountain(LGM)" -> 'Lightning Mountain (LGM)" ok. +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/leds/leds-lgm.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Intel LGM Soc LED SSO driver Please spell out LGM and SSO here. Soc -> SoC? ok. + intel,sso-brightness: +$ref: /schemas/types.yaml#/definitions/uint32 +description: brightness level of the LED. +minimum: 0 +maximum: 255 ? In case of using "default-state" property, it is redundant. + intel,sso-hw-trigger: +type: boolean +description: This property indicates Hardware driven/control LED. Why is this intel specific? This is not common property, so i used vendor name. do you suggest only property name without vendor name? + intel,sso-hw-blink: +type: boolean +description: This property indicates Enable LED blink by Hardware. ? This is not common property, so i used vendor name. do you suggest only property name without vendor name? + intel,sso-blink-rate: +$ref: /schemas/types.yaml#/definitions/uint32 +description: LED HW blink frequency. ? This is not common property, so i used vendor name. do you suggest only property name without vendor name? Best regards, Pavel
Re: [PATCH v1 2/2] leds: lgm: Add LED controller driver for LGM Soc
Hi Pavel,
Thanks for your valuable comments. My comments inline.
On 11/5/2020 6:11 PM, Pavel Machek wrote:
Hi!
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index ed943140e1fd..6445b39fe4fc 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -886,6 +886,16 @@ config LEDS_SGM3140
This option enables support for the SGM3140 500mA Buck/Boost Charge
Pump LED Driver.
+config LEDS_LGM_SSO
+ tristate "LED support for Intel LGM SOC series"
+ depends on LEDS_CLASS
+ depends on MFD_SYSCON
+ depends on OF
+ help
+ Parallel to serial conversion, which is also called SSO controller,
+ can drive external shift register for LED outputs.
+ This enables LED support for Serial Shift Output Controller(SSO).
Something is wrong with indentation here.
Seems tabbing.. i will fix it.
diff --git a/drivers/leds/leds-lgm-sso.c b/drivers/leds/leds-lgm-sso.c
Could we put it into drivers/leds/blink/ directory? You'll need to
create it.
Sure, i will update in the next patch.
index ..f1bae1c6ed3c
--- /dev/null
+++ b/drivers/leds/leds-lgm-sso.c
@@ -0,0 +1,881 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel LGM Soc LED SSO driver
Spell out LGM, SSO. Soc->SoC.
Pointer to documentation would be welcome here.
Public documentation is not available.
+enum {
+ US_SW = 0,
+ US_GPTC = 1,
+ US_FPID = 2
+};
This is not really useful without additional comments.
ok, i will update the additional comments.
+static u32 sso_rectify_brightness(u32 brightness)
+{
+ if (brightness > LED_FULL)
+ return LED_FULL;
+ else
+ return brightness;
+}
Why?
As per below review comments if we use "default-state" property, it will
be redundant.
+static int sso_rectify_blink_rate(struct sso_led_priv *priv, u32 rate)
+{
+ int i;
+
+ for (i = 0; i < MAX_FREQ_RANK; i++) {
+ if (rate <= priv->freq[i])
+ return i;
+ }
+
+ return i - 1;
+}
Can return -1. Is that expected?
It return the frequency index, if 'rate' is not matching with
'priv->freq' it will return maximum index.
In case of return -1 need to adjust the code in the function from where
it is called.
func name 'sso_get_blink_rate_idx' is more appropriate i think.
+
+ desc->np = to_of_node(fwnode_child);
+ if (fwnode_property_read_string(fwnode_child, "label",
+ >name)) {
+ dev_err(dev, "LED no label name!\n");
+ goto __dt_err;
+ }
Can you use appropriate helper from the core? labels are getting
deprecated...
Agree.
+ if (fwnode_property_present(fwnode_child,
+ "retain-state-suspended"))
+ desc->retain_state_suspended = 1;
Was this documented in the binding?
No, i will udpate.
+ if (fwnode_property_read_u32(fwnode_child, "intel,led-pin",
+)) {
+ dev_err(dev, "Failed to find led pin id!\n");
+ goto __dt_err;
Would not we normally use something like reg = to indicate pin?
Yes, we can do that. i will update in the next patch.
+ if (fwnode_property_present(fwnode_child,
+ "intel,sso-hw-trigger"))
+ desc->hw_trig = 1;
Should not that be selectable on runtime?
Agree, i will fix this.
+ if (fwnode_property_read_u32(fwnode_child,
+"intel,sso-brightness", ))
+ desc->brightness = priv->brightness;
+ else
+ desc->brightness = sso_rectify_brightness(prop);
Can you look at "default-state" property?
sure. i will update with "default-state" property
+ ret = sso_gpio_gc_init(dev, priv);
+ if (ret)
+ return ret;
+
+ return 0;
+}
Just return ret.
ok.
+
+ ret = clk_prepare_enable(priv->gclk);
+ if (ret) {
+ dev_err(dev, "Failed to prepate/enable sso gate clock!\n");
+ return ret;
+ }
+
+ priv->fpid_clk = devm_clk_get(dev, "fpid");
+ if (IS_ERR(priv->fpid_clk)) {
+ dev_err(dev, "Failed to get fpid clock!\n");
+ return PTR_ERR(priv->fpid_clk);
+ }
clk disable here?
ok. i will use devm_add_action_or_reset to disable clocks.
+ ret = clk_prepare_enable(priv->fpid_clk);
+ if (ret) {
+ dev_err(dev, "Failed to prepare/enable fpid clock!\n");
+ return ret;
+ }
+ priv->fpid_clkrate = clk_get_rate(priv->fpid_clk);
+
+ priv->mmap = syscon_node_to_regmap(dev->of_node);
+ if (IS_ERR(priv->mmap)) {
+ dev_err(dev, "Failed to map iomem!\n");
+
Re: [PATCH v7 2/2] Add Intel LGM soc DMA support.
Hi Thomas,
Thanks for valuable review, My comments inline.
On 10/28/2020 3:24 AM, Thomas Langer wrote:
Hello Reddy,
some comments below. I only commented regarding some structural things,
as I am not so familiar with the functional side of DMA.
-Original Message-
From: Amireddy Mallikarjuna reddy
Sent: Dienstag, 27. Oktober 2020 09:03
To: dmaeng...@vger.kernel.org; vk...@kernel.org;
devicet...@vger.kernel.org; robh...@kernel.org
Cc: linux-kernel@vger.kernel.org; Shevchenko, Andriy
; chuanhua@linux.intel.com; Kim,
Cheol Yong ; Wu, Qiming ; mallikarjunax.re...@linux.intel.com;
malliamireddy...@gmail.com; peter.ujfal...@ti.com
Subject: [PATCH v7 2/2] Add Intel LGM soc DMA support.
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
DPlus compliant peripheral to/from the memory. A memory to memory copy
capability can also be configured.
What is "DPlus compliant"? This needs some explanation.
HAS Document described D+ or Dplus as 'Proprietary Parallel Data
Interface for DMA and Peripherals'
To make the things simple can change the description like this.
'The main function of the DMA controller is the transfer of data from/to
any peripheral to/from the memory.'
This ldma driver is used for configure the device and channnels for
data
and control paths.
Signed-off-by: Amireddy Mallikarjuna reddy
---
v1:
- Initial version.
v2:
- Fix device tree bot issues, correspondign driver changes done.
- Fix kerntel test robot warnings.
>> drivers/dma/lgm/lgm-dma.c:729:5: warning: no previous prototype
for function 'intel_dma_chan_desc_cfg' [-Wmissing-prototypes]
int intel_dma_chan_desc_cfg(struct dma_chan *chan, dma_addr_t
desc_base,
^
drivers/dma/lgm/lgm-dma.c:729:1: note: declare 'static' if the
function is not intended to be used outside of this translation unit
int intel_dma_chan_desc_cfg(struct dma_chan *chan, dma_addr_t
desc_base,
^
static
1 warning generated.
vim +/intel_dma_chan_desc_cfg +729 drivers/dma/lgm/lgm-dma.c
728
> 729 int intel_dma_chan_desc_cfg(struct dma_chan *chan, dma_addr_t
desc_base,
730 int desc_num)
731 {
732 return ldma_chan_desc_cfg(to_ldma_chan(chan),
desc_base, desc_num);
733 }
734 EXPORT_SYMBOL_GPL(intel_dma_chan_desc_cfg);
735
Reported-by: kernel test robot
---
v3:
- Fix smatch warning.
smatch warnings:
drivers/dma/lgm/lgm-dma.c:1306 ldma_cfg_init() error: uninitialized
symbol 'ret'.
Reported-by: kernel test robot
Reported-by: Dan Carpenter
v4:
- Address Thomas Langer comments in dtbinding and corresponding driver
side changes.
- Driver side changes to corresponding device tree changes.
v5:
- Add changes to read 'dmas' properties and update the config
properties driver side.
- Add virt_dma_desc utilizes virt-dma API.
v6:
- Driver changes corresponding to the device tree changes.
- Restructure things to have less activity with the spinlock.
- Save the slave config in dma_slave_config() and used in prepare
time.
- Addressed & fixed issues related to desc_free callback _free_ up the
memory.
- Addressed peter review comments.
v7:
- Change bool to tristate in Kconfig
I see this "tristate" (compile as module) and the explanation
for the _initcall confusing. Either this is a driver for the SoC
that must be available early or it can be used as module (which
is typically loaded late). You cannot fulfill both things.
For LGM Soc it must be available early. I will update accordingly.
- Explained the _initcall()
- change of_property*() to device_property_*()
- split the code to functions at version checks
- Remove the dma caller capability restrictions
- used for_each_set_bit()
- Addressed minor comments and fine tune the code.
---
drivers/dma/Kconfig |2 +
drivers/dma/Makefile|1 +
drivers/dma/lgm/Kconfig |9 +
drivers/dma/lgm/Makefile|2 +
drivers/dma/lgm/lgm-dma.c | 1765
+++
include/linux/dma/lgm_dma.h | 27 +
6 files changed, 1806 insertions(+)
create mode 100644 drivers/dma/lgm/Kconfig
create mode 100644 drivers/dma/lgm/Makefile
create mode 100644 drivers/dma/lgm/lgm-dma.c
create mode 100644 include/linux/dma/lgm_dma.h
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index de41d7928bff..caeaf12fd524 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -737,6 +737,8 @@ source "drivers/dma/ti/Kconfig"
source "drivers/dma/fsl-dpaa2-qdma/Kconfig"
+source "drivers/dma/lgm/Kconfig"
+
# clients
comment "DMA Clients"
depends on DMA_ENGINE
diff --git
Re: [PATCH v7 1/2] dt-bindings: dma: Add bindings for intel LGM SOC
Hi Thomas, Thanks for the review, my comments inline. On 10/28/2020 3:24 AM, Thomas Langer wrote: Hello Reddy, I think "Intel" should always be written with a capital "I" (like in the Subject, but except in the binding below) OK. + compatible: + oneOf: + - const: intel,lgm-cdma + - const: intel,lgm-dma2tx + - const: intel,lgm-dma1rx + - const: intel,lgm-dma1tx + - const: intel,lgm-dma0tx + - const: intel,lgm-dma3 + - const: intel,lgm-toe-dma30 + - const: intel,lgm-toe-dma31 Bindings are normally not per instance. What if next generation chip gets more DMA modules but has no other changes in the HW block? What is wrong with - const: intel,lgm-cdma - const: intel,lgm-hdma and extra attributes to define the rx/tx restriction (or what do it mean?)? From the driver code I saw that "toe" is also just of type "hdma" and no further differences in code are done. We had a discussion on the same in the previous patches and Rob Herring said Okay using Different compatibles. below the snippet. +++ >>> + compatible: >>> + anyOf: >>> + - const: intel,lgm-cdma >>> + - const: intel,lgm-dma2tx >>> + - const: intel,lgm-dma1rx >>> + - const: intel,lgm-dma1tx >>> + - const: intel,lgm-dma0tx >>> + - const: intel,lgm-dma3 >>> + - const: intel,lgm-toe-dma30 >>> + - const: intel,lgm-toe-dma31 >> Please explain why you need so many different compatible strings. > This hw dma has 7 DMA instances. > Some for datapath, some for memcpy and some for TOE. > Some for TX only, some for RX only, and some for TX/RX(memcpy and ToE). > > dma TX/RX type we considered as driver specific data of each instance and > used different compatible strings for each instance. > And also idea is in future if any driver specific data of any particular > instance we can handle. > > Here if dma name and type(tx or rx) will be accepted as devicetree > attributes then we can move .name = "toe_dma31", & .type = DMA_TYPE_MCPY > to devicetree. So that the compatible strings can be limited to two. > intel,lgm-cdma & intel,lgm-hdma . [Rob] Different compatibles are okay if the instances are different and we don't have properties to describe the differences. For some of what you have in this binding, I think it should be part of the consumer cells. Best regards, Thomas
Re: [PATCH v6 2/2] Add Intel LGM soc DMA support.
Hi Andy,
Thanks for your comments. My comments are in line.
On 9/18/2020 8:20 PM, Andy Shevchenko wrote:
On Fri, Sep 18, 2020 at 11:42:54AM +0800, Reddy, MallikarjunaX wrote:
On 9/9/2020 7:14 PM, Andy Shevchenko wrote:
On Wed, Sep 09, 2020 at 07:07:34AM +0800, Amireddy Mallikarjuna reddy wrote:
...
+ help
+ Enable support for intel Lightning Mountain SOC DMA controllers.
+ These controllers provide DMA capabilities for a variety of on-chip
+ devices such as SSC, HSNAND and GSWIP.
And how module will be called?
are you expecting to include 'default y' ?
I'm expecting to see something like "if you choose M the module will be called
bla-foo-bar." Look at the existing examples in the kernel.
ok, i will change bool to tristate.
...
+ldma_update_bits(struct ldma_dev *d, u32 mask, u32 val, u32 ofs)
+{
+ u32 old_val, new_val;
+
+ old_val = readl(d->base + ofs);
+ new_val = (old_val & ~mask) | (val & mask);
With bitfield.h you will have this as u32_replace_bits().
- new_val = (old_val & ~mask) | (val & mask);
+ new_val = old_val;
+ u32_replace_bits(new_val, val, mask);
I think in this function we cant use this because of compilation issues
thrown by bitfield.h . Expecting 2nd and 3rd arguments as constant numbers
not as type variables.
ex:
u32_replace_bits(val, 0, IPA_REG_ENDP_ROUTER_HASH_MSK_ALL);
How comes these are constants? In the above you have a function which does
r-m-w approach to the register. It should be something like
old = read();
new = u32_replace_bits(old, ...);
write(new);
./include/linux/bitfield.h:131:3: error: call to '__field_overflow' declared
with attribute error: value doesn't fit into mask
__field_overflow(); \
^~
./include/linux/bitfield.h:119:3: error: call to '__bad_mask' declared with
attribute error: bad bitfield mask
__bad_mask();
^~~~
So, even with constants u32_replace_bits() must work. Maybe you didn't get how?
Thanks Andy, now i know how u32_replace_bits() is working.
The mask is not the continuous bits in some cases. Due to the mask bits
are not continuous u32_replace_bits() can't be used here.
Ex:
u32 mask = DMA_CPOLL_EN | DMA_CPOLL_CNT;
Comes to __field_overflow error, update bits in the 'val' are aligned
with mask bits. Because of the this reason in u32_replace_bits() 'val'
exceeds the 'mask' in some cases which is causing __field_overflow error.
+ if (new_val != old_val)
+ writel(new_val, d->base + ofs);
+}
...
+ /* High 4 bits */
Why only 4?
this is higher 4 bits of 36 bit addressing..
Make it clear in the comment.
ok.
...
+device_initcall(intel_ldma_init);
Each _initcall() in general should be explained.
ok. is it fine?
/* Perform this driver as device_initcall to make sure initialization
happens
* before its dma clients of some are platform specific. make sure to
provice
* registered dma channels and dma capabilities to client before their
* initialization.
*/
/*
* Just follow proper multi-line comment style.
* And use dma -> DMA.
*/
Ok.
Re: [PATCH v5 2/2] Add Intel LGM soc DMA support.
Hi Peter,
Thanks for the review, please see my comment inline..
On 9/4/2020 2:31 PM, Peter Ujfalusi wrote:
On 31/08/2020 11.07, Reddy, MallikarjunaX wrote:
On 8/28/2020 7:17 PM, Peter Ujfalusi wrote:
Hi,
On 27/08/2020 17.41, Reddy, MallikarjunaX wrote:
+
+ dma_dev->device_alloc_chan_resources =
+ d->inst->ops->device_alloc_chan_resources;
+ dma_dev->device_free_chan_resources =
+ d->inst->ops->device_free_chan_resources;
+ dma_dev->device_terminate_all =
d->inst->ops->device_terminate_all;
+ dma_dev->device_issue_pending =
d->inst->ops->device_issue_pending;
+ dma_dev->device_tx_status = d->inst->ops->device_tx_status;
+ dma_dev->device_resume = d->inst->ops->device_resume;
+ dma_dev->device_pause = d->inst->ops->device_pause;
+ dma_dev->device_config = d->inst->ops->device_config;
+ dma_dev->device_prep_slave_sg =
d->inst->ops->device_prep_slave_sg;
+ dma_dev->device_synchronize = d->inst->ops->device_synchronize;
+
+ if (d->ver == DMA_VER22) {
+ dma_dev->src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
+ dma_dev->dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
+ dma_dev->directions = BIT(DMA_MEM_TO_DEV) |
+ BIT(DMA_DEV_TO_MEM);
+ dma_dev->residue_granularity =
+ DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
+ }
So, if version is != DMA_VER22, then you don't support any direction?
Why register the DMA device if it can not do any transfer?
Only dma0 instance (intel,lgm-cdma) is used as a general purpose slave
DMA. we set both control and datapath here.
Other instances we set only control path. data path is taken care
by dma
client(GSWIP).
How the client (GSWIP) can request a channel from intel,lgm-* ? Don't
you need some capabilities for the DMA device so core can sort out the
request?
client request channel by name, dma_request_slave_channel(dev, name);
clients should use dma_request_chan(dev, name);
If the channel can be requested via DT or ACPI then we don't check the
capabilities at all, so yes, that could work.
Only thing needs to do is get the channel, set the descriptor and just
on the channel.
How do you 'on' the channel?
we on the channel in issue_pending.
Right.
Basically you only prep_slave_sg/single for the DMA_VER22? Or do you
that for the others w/o direction support?
Yes. prep_slave_sg/single only for the DMA_VER22.
So, you place the transfers to DMA_VER22's channel
For the intel,lgm-* DMAs you only call issue_pending() and probably
terminate_all?
Yes, correct.
and issue_pending on a channel which does not have anything pending?
How client knows which of the 'only need to be ON' channel to enable
when it placed a transfer to another channel?
How would the client driver (and the IP) would be integrated with
different system DMA which have standard channel management?
If DMA_VER22 knows which intel,lgm-* it should place the transfer then
with virt-dma you can handle that just fine?
In the version point of view, "intel,lgm-cdma" dma instance support
DMA_VER22. and other "intel,lgm-*" dma instances support > DMA_VER22 .
The channels registered to kernel from "intel,lgm-cdma" dma instance
used as a general purpose dma. The clients are SPI, ebu_nand etc..
It uses standard dmaengine calls dma_request_chan(),
dmaengine_slave_config(), dmaengine_prep_slave_sg() &
dma_async_issue_pending()..
and other dma instances the clients are GSWIP & CQM (Central Queue
manager) which uses the DMA in their packet processing.
Each dma dma2tx, dma1rx, dma1tx, dma0tx and dma3 had channels 16, 8,
16,16 and 16 respectively and these channels are used by GSWIP & CQM as
part of
ingress(IGP) and egress(EGP) ports. and each of the dma port uses a dma
channel. This is one to one mapping.
The GSWIP & CQM talk to the dma controller driver via dmaengine to get
the dma channels using dma_request_slave_channel().
Configures the descriptor base address using intel_dma_chan_desc_cfg()
which is EXPORT API from dma driver(this desc base address is the
register address(descriptor) of the IGP/EGP), and ON the channel using
dma_async_issue_pending.
GSWIP & CQM is highly low level register configurable/programmable take
care about the the packet processing through the register configurations.
Do you have public documentation for the DMA?
Unfortunately we dont have public documentation for this DMA IP.
It sounds a bit like TI's
EDMA which is in essence is a two part setup:
CC: Channel Controller - to submit transfers (like your DMA_VER22)
TC: Transfer Controller - it executes the transfers submitted by the CC
One CC can be backed by multiple TCs. I don't have direct control over
the TC (can not start/stop), it is controlled by the CC.
Documentation/devicetree/bindings/dma/ti-edma.t
Re: [PATCH v5 1/2] dt-bindings: dma: Add bindings for intel LGM SOC
On 9/4/2020 2:31 PM, Peter Ujfalusi wrote: On 18/08/2020 10.00, Reddy, MallikarjunaX wrote: Hi Rob, Thanks for your valuable comments. Please see my comments inline.. On 8/15/2020 4:32 AM, Rob Herring wrote: On Fri, Aug 14, 2020 at 01:26:09PM +0800, Amireddy Mallikarjuna reddy wrote: Add DT bindings YAML schema for DMA controller driver of Lightning Mountain(LGM) SoC. Signed-off-by: Amireddy Mallikarjuna reddy --- 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. --- .../devicetree/bindings/dma/intel,ldma.yaml | 319 + 1 file changed, 319 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/intel,ldma.yaml diff --git a/Documentation/devicetree/bindings/dma/intel,ldma.yaml b/Documentation/devicetree/bindings/dma/intel,ldma.yaml new file mode 100644 index ..9beaf191a6de --- /dev/null +++ b/Documentation/devicetree/bindings/dma/intel,ldma.yaml @@ -0,0 +1,319 @@ +# 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: + - chuanhua@intel.com + - mallikarjunax.re...@intel.com + +allOf: + - $ref: "dma-controller.yaml#" + +properties: + $nodename: + pattern: "^dma-controller(@.*)?$" + + "#dma-cells": + const: 1 Example says 3. OK, i will fix it. It would help if you would add description of what is the meaning of the individual cell. I am already prepared the patch by addressing previous comments and just before sending i received your review comment. :-) Let me Edit , include the description and prepare the patch again. - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v5 1/2] dt-bindings: dma: Add bindings for intel LGM SOC
Hi Vinod, Thanks for the review, please see my comments inline. On 8/31/2020 7:00 PM, Vinod Koul wrote: On 31-08-20, 16:06, Reddy, MallikarjunaX wrote: Hi Vinod, Thanks for the review. Please see my comment inline. On 8/28/2020 6:45 PM, Vinod Koul wrote: On 27-08-20, 17:54, Reddy, MallikarjunaX wrote: Hi Vinod, Thanks for the review comments. On 8/25/2020 7:21 PM, Vinod Koul wrote: On 18-08-20, 15:00, Reddy, MallikarjunaX wrote: + +intel,chans: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: + The channels included on this port. Format is channel start + number and how many channels on this port. Why does this need to be in DT? This all seems like it can be in the dma cells for each client. (*ABC) Yes. We need this. for dma0(lgm-cdma) old SOC supports 16 channels and the new SOC supports 22 channels. and the logical channel mapping for the peripherals also differ b/w old and new SOCs. Because of this hardware limitation we are trying to configure the total channels and port-channel mapping dynamically from device tree. based on port name we are trying to configure the default values for different peripherals(ports). Example: burst length is not same for all ports, so using port name to do default configurations. Sorry that does not make sense to me, why not specify the values to be used here instead of defining your own name scheme! OK. Agreed. I will remove port name from DT and only use intel,chans what is intel,chans, why not use dma-channels? The intel,chans says about the channels included on the correspondng port. What do you mean by a port here? Here Port is nothing but Peripheral(SPI, HSNAND...) Format is channel start number and how many channels on this port. It is perfectly reasonable to have 16 channels but linux not use use all, lets say from 5th channel channel onwards So you need to use standard dma-channels also with dma-channel-mask to specify which channels linux can use Ok, let me verify and use the standard dma-channels also dma-channel-mask. The reasong behind using this attribute instead of standrad dma-channels is... DMA_VER22 HW supports 22 channels. But there is a hole in HW, total it can use only 16. Old soc supports 4ports and 16 channels. New soc supports 6ports and 22 channels. (old and new soc carry the same version VER22) port channel mapping for the old and new soc also not the same. old soc: logical channels:(Rx, Tx) 0, 1 - SPI0 2, 3 - SPI1 4, 5 - HSNAND 12, 14, 13, 15 - Memcopy New soc: Logical channels(Rx, Tx) 0, 1 - SPI0 2, 3 - SPI1 4, 5 - SPI2 6, 7 - SPI3 8, 9 - HSNAND 10 to 21 - Mcopy Mapping is different, client can set that channel required in dmas property and use a specific required channel. OK. Because of these reasons we are trying to use "intel,chans" attribute, and reading then number of channels from the dt. Advantaage: 1. we can map the channels correspondign to port 2. Dynamically configure the channels (due to hw limitation) If this is not ok, please suggest us the better way to handle this.
Re: [PATCH v5 2/2] Add Intel LGM soc DMA support.
On 8/28/2020 7:17 PM, Peter Ujfalusi wrote:
Hi,
On 27/08/2020 17.41, Reddy, MallikarjunaX wrote:
+
+ dma_dev->device_alloc_chan_resources =
+ d->inst->ops->device_alloc_chan_resources;
+ dma_dev->device_free_chan_resources =
+ d->inst->ops->device_free_chan_resources;
+ dma_dev->device_terminate_all =
d->inst->ops->device_terminate_all;
+ dma_dev->device_issue_pending =
d->inst->ops->device_issue_pending;
+ dma_dev->device_tx_status = d->inst->ops->device_tx_status;
+ dma_dev->device_resume = d->inst->ops->device_resume;
+ dma_dev->device_pause = d->inst->ops->device_pause;
+ dma_dev->device_config = d->inst->ops->device_config;
+ dma_dev->device_prep_slave_sg =
d->inst->ops->device_prep_slave_sg;
+ dma_dev->device_synchronize = d->inst->ops->device_synchronize;
+
+ if (d->ver == DMA_VER22) {
+ dma_dev->src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
+ dma_dev->dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
+ dma_dev->directions = BIT(DMA_MEM_TO_DEV) |
+ BIT(DMA_DEV_TO_MEM);
+ dma_dev->residue_granularity =
+ DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
+ }
So, if version is != DMA_VER22, then you don't support any direction?
Why register the DMA device if it can not do any transfer?
Only dma0 instance (intel,lgm-cdma) is used as a general purpose slave
DMA. we set both control and datapath here.
Other instances we set only control path. data path is taken care by dma
client(GSWIP).
How the client (GSWIP) can request a channel from intel,lgm-* ? Don't
you need some capabilities for the DMA device so core can sort out the
request?
client request channel by name, dma_request_slave_channel(dev, name);
clients should use dma_request_chan(dev, name);
If the channel can be requested via DT or ACPI then we don't check the
capabilities at all, so yes, that could work.
Only thing needs to do is get the channel, set the descriptor and just
on the channel.
How do you 'on' the channel?
we on the channel in issue_pending.
Right.
Basically you only prep_slave_sg/single for the DMA_VER22? Or do you
that for the others w/o direction support?
Yes. prep_slave_sg/single only for the DMA_VER22.
For the intel,lgm-* DMAs you only call issue_pending() and probably
terminate_all?
Yes, correct.
Interesting setup ;)
- Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH v5 1/2] dt-bindings: dma: Add bindings for intel LGM SOC
Hi Vinod, Thanks for the review. Please see my comment inline. On 8/28/2020 6:45 PM, Vinod Koul wrote: On 27-08-20, 17:54, Reddy, MallikarjunaX wrote: Hi Vinod, Thanks for the review comments. On 8/25/2020 7:21 PM, Vinod Koul wrote: On 18-08-20, 15:00, Reddy, MallikarjunaX wrote: + +intel,chans: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: + The channels included on this port. Format is channel start + number and how many channels on this port. Why does this need to be in DT? This all seems like it can be in the dma cells for each client. (*ABC) Yes. We need this. for dma0(lgm-cdma) old SOC supports 16 channels and the new SOC supports 22 channels. and the logical channel mapping for the peripherals also differ b/w old and new SOCs. Because of this hardware limitation we are trying to configure the total channels and port-channel mapping dynamically from device tree. based on port name we are trying to configure the default values for different peripherals(ports). Example: burst length is not same for all ports, so using port name to do default configurations. Sorry that does not make sense to me, why not specify the values to be used here instead of defining your own name scheme! OK. Agreed. I will remove port name from DT and only use intel,chans what is intel,chans, why not use dma-channels? The intel,chans says about the channels included on the correspondng port. Format is channel start number and how many channels on this port. The reasong behind using this attribute instead of standrad dma-channels is... DMA_VER22 HW supports 22 channels. But there is a hole in HW, total it can use only 16. Old soc supports 4ports and 16 channels. New soc supports 6ports and 22 channels. (old and new soc carry the same version VER22) port channel mapping for the old and new soc also not the same. old soc: logical channels:(Rx, Tx) 0, 1 - SPI0 2, 3 - SPI1 4, 5 - HSNAND 12, 14, 13, 15 - Memcopy New soc: Logical channels(Rx, Tx) 0, 1 - SPI0 2, 3 - SPI1 4, 5 - SPI2 6, 7 - SPI3 8, 9 - HSNAND 10 to 21 - Mcopy Because of these reasons we are trying to use "intel,chans" attribute, and reading then number of channels from the dt. Advantaage: 1. we can map the channels correspondign to port 2. Dynamically configure the channels (due to hw limitation) If this is not ok, please suggest us the better way to handle this.
Re: [PATCH v5 2/2] Add Intel LGM soc DMA support.
Hi Peter,
Thank you very much for the review and detailed explanation, i will take
the inputs and update in the next patch.
My comments inline..
On 8/24/2020 7:24 PM, Peter Ujfalusi wrote:
Hi,
On 24/08/2020 5.30, Reddy, MallikarjunaX wrote:
+static void dma_free_desc_resource(struct virt_dma_desc *vdesc)
+{
+ struct dw2_desc_sw *ds = to_lgm_dma_desc(vdesc);
+ struct ldma_chan *c = ds->chan;
+
+ dma_pool_free(c->desc_pool, ds->desc_hw, ds->desc_phys);
+ kfree(ds);
+ c->ds = NULL;
Is there a chance that c->ds != ds?
No, from the code i don't see any such scenario, let me know if you find
any corner case.
The desc_free callback is used to _free_ up the memory used for the
descriptor. Nothing less, nothing more.
You should not touch the c->ds in this callback, just free up the memory
used for the given vdesc.
More on that a bit later.
Ok.
+}
+
+static struct dw2_desc_sw *
+dma_alloc_desc_resource(int num, struct ldma_chan *c)
+{
+ struct device *dev = c->vchan.chan.device->dev;
+ struct dw2_desc_sw *ds;
+
+ if (num > c->desc_num) {
+ dev_err(dev, "sg num %d exceed max %d\n", num, c->desc_num);
+ return NULL;
+ }
+
+ ds = kzalloc(sizeof(*ds), GFP_NOWAIT);
+ if (!ds)
+ return NULL;
+
+ ds->chan = c;
+
+ ds->desc_hw = dma_pool_zalloc(c->desc_pool, GFP_ATOMIC,
+ >desc_phys);
+ if (!ds->desc_hw) {
+ dev_dbg(dev, "out of memory for link descriptor\n");
+ kfree(ds);
+ return NULL;
+ }
+ ds->desc_cnt = num;
+
+ return ds;
+}
+
+static void ldma_chan_irq_en(struct ldma_chan *c)
+{
+ struct ldma_dev *d = to_ldma_dev(c->vchan.chan.device);
+ unsigned long flags;
+
+ spin_lock_irqsave(>dev_lock, flags);
+ writel(c->nr, d->base + DMA_CS);
+ writel(DMA_CI_EOP, d->base + DMA_CIE);
+ writel(BIT(c->nr), d->base + DMA_IRNEN);
+ spin_unlock_irqrestore(>dev_lock, flags);
+}
+
+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) {
+ spin_lock_irqsave(>vchan.lock, flags);
+ if (vchan_issue_pending(>vchan)) {
+ struct virt_dma_desc *vdesc;
+
+ /* Get the next descriptor */
+ vdesc = vchan_next_desc(>vchan);
+ if (!vdesc) {
+ c->ds = NULL;
+ return;
+ }
+ list_del(>node);
+ c->ds = to_lgm_dma_desc(vdesc);
you have set c->ds in dma_prep_slave_sg and the only way I can see that
you will not leak memory is that the client must terminate_sync() after
each transfer so that the synchronize callback is invoked between each
prep_sg/issue_pending/competion.
Yes, client must call dmaengine_synchronize after each transfer to make
sure free the memory assoicated with previously issued descriptors if any.
No, client should not need to invoke synchronize between transfers,
clients must be able to do:
dmaengine_prep_slave_single/dmaengine_prep_slave_sg
dma_async_issue_pending
* handle the callback
dmaengine_prep_slave_single/dmaengine_prep_slave_sg
dma_async_issue_pending
* handle the callback
without any terminate_all_sync() in between.
Imagine that the client is preparing/issuing a new transfer in the
completion callback for example.
Clients must be able to do also:
dmaengine_prep_slave_single/dmaengine_prep_slave_sg
dmaengine_prep_slave_single/dmaengine_prep_slave_sg
...
dmaengine_prep_slave_single/dmaengine_prep_slave_sg
dma_async_issue_pending
and then the DMA will complete the transfers in FIFO order and when the
first is completed it will move to the next one. Client will receive
callbacks for each completion (if requested).
Thanks for the detailed explanation peter.
and also from the driver we are freeing up the descriptor from work
queue atfer each transfer.(addressed below comments **)
+ spin_unlock_irqrestore(>vchan.lock, flags);
+ ldma_chan_desc_hw_cfg(c, c->ds->desc_phys,
c->ds->desc_cnt);
+ ldma_chan_irq_en(c);
+ }
If there is nothing peding, you will leave the spinlock wide open...
Seems i misplaced the lock. i will fix it in next version.
+ }
+ ldma_chan_on(c);
+}
+
+static void dma_synchronize(struct dma_chan *chan)
+{
+ struct ldma_chan *c = to_ldma_chan(chan);
+
+ /*
+ * clear any pending work if any. In that
+ * case the resource needs to be free here.
+ */
+ cancel_work_sync(>work);
+ vchan_synchronize(>vchan);
+ if (c->ds)
+ dma_free_desc_resource(>ds->vdesc);
+}
+
+static int dma_terminate_all(struct dma_chan *chan)
+{
+ struct ldma_chan *c = to_ldma_chan(chan);
+ unsigned long flags;
+ LIST_HEAD(head);
+
+ spin_lock_i
Re: [PATCH v5 1/2] dt-bindings: dma: Add bindings for intel LGM SOC
Hi Vinod, Thanks for the review comments. On 8/25/2020 7:21 PM, Vinod Koul wrote: On 18-08-20, 15:00, Reddy, MallikarjunaX wrote: + +intel,chans: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: + The channels included on this port. Format is channel start + number and how many channels on this port. Why does this need to be in DT? This all seems like it can be in the dma cells for each client. (*ABC) Yes. We need this. for dma0(lgm-cdma) old SOC supports 16 channels and the new SOC supports 22 channels. and the logical channel mapping for the peripherals also differ b/w old and new SOCs. Because of this hardware limitation we are trying to configure the total channels and port-channel mapping dynamically from device tree. based on port name we are trying to configure the default values for different peripherals(ports). Example: burst length is not same for all ports, so using port name to do default configurations. Sorry that does not make sense to me, why not specify the values to be used here instead of defining your own name scheme! OK. Agreed. I will remove port name from DT and only use intel,chans Only older soc it should create 16 channels and new 22 (hint this is hw description so perfectly okay to specify in DT or in using driver_data and compatible for each version + + required: +- reg +- intel,name +- intel,chans + + + ldma-channels: +type: object +description: + This sub-node must contain a sub-node for each DMA channel. +properties: + '#address-cells': +const: 1 + '#size-cells': +const: 0 + +patternProperties: + "^ldma-channels@[0-15]+$": + type: object + + properties: +reg: + items: +- enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15] + description: + Which channel this node refers to. + +intel,desc_num: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + Per channel maximum descriptor number. The max value is 255. + +intel,hdr-mode: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: + The first parameter is header mode size, the second + parameter is checksum enable or disable. If enabled, + header mode size is ignored. If disabled, header mode + size must be provided. + +intel,hw-desc: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: + Per channel dma hardware descriptor configuration. + The first parameter is descriptor physical address and the + second parameter hardware descriptor number. Again, this all seems like per client information for dma cells. Ok, if we move all these attributes to 'dmas' then 'dma-channels' child node is not needed in dtsi. #dma-cells number i am already using 7. If we move all these attributes to 'dmas' then integer cells will increase. Is there any limitation in using a number of integer cells & as determined by the #dma-cells property? No I dont think there is but it needs to make sense :-) OK.
Re: [PATCH v5 2/2] Add Intel LGM soc DMA support.
Hi Peter,
Thanks for the review comments. Please see my comments inline..
On 8/18/2020 6:16 PM, Peter Ujfalusi wrote:
Hi,
On 14/08/2020 8.26, Amireddy Mallikarjuna reddy wrote:
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
DPlus compliant 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
...
+static int ldma_chan_cfg(struct ldma_chan *c)
+{
+ struct ldma_dev *d = to_ldma_dev(c->vchan.chan.device);
+ u32 reg;
+
+ reg = c->pden ? DMA_CCTRL_PDEN : 0;
+ reg |= c->onoff ? DMA_CCTRL_ON : 0;
+ reg |= c->rst ? DMA_CCTRL_RST : 0;
+
+ ldma_chan_cctrl_cfg(c, reg);
+ ldma_chan_irq_init(c);
+
+ if (d->ver > DMA_VER22) {
+ ldma_chan_set_class(c, c->nr);
+ ldma_chan_byte_offset_cfg(c, c->boff_len);
+ ldma_chan_data_endian_cfg(c, c->data_endian_en, c->data_endian);
+ ldma_chan_desc_endian_cfg(c, c->desc_endian_en, c->desc_endian);
+ ldma_chan_hdr_mode_cfg(c, c->hdrm_len, c->hdrm_csum);
+ ldma_chan_rxwr_np_cfg(c, c->desc_rx_np);
+ ldma_chan_abc_cfg(c, c->abc_en);
Each of these functions will lock and unlock the same lock, would it
make sense to restructur things to have less activity with the spinlock?
Ok. Instead of lock & unlock at each function i will try to lock &
unlock only once from here.
+
+ if (ldma_chan_is_hw_desc(c))
+ ldma_chan_desc_hw_cfg(c, c->desc_phys, c->desc_cnt);
+ }
+
+ return 0;
+}
...
+static void dma_free_desc_resource(struct virt_dma_desc *vdesc)
+{
+ struct dw2_desc_sw *ds = to_lgm_dma_desc(vdesc);
+ struct ldma_chan *c = ds->chan;
+
+ dma_pool_free(c->desc_pool, ds->desc_hw, ds->desc_phys);
+ kfree(ds);
+ c->ds = NULL;
Is there a chance that c->ds != ds?
No, from the code i don't see any such scenario, let me know if you find
any corner case.
+}
+
+static struct dw2_desc_sw *
+dma_alloc_desc_resource(int num, struct ldma_chan *c)
+{
+ struct device *dev = c->vchan.chan.device->dev;
+ struct dw2_desc_sw *ds;
+
+ if (num > c->desc_num) {
+ dev_err(dev, "sg num %d exceed max %d\n", num, c->desc_num);
+ return NULL;
+ }
+
+ ds = kzalloc(sizeof(*ds), GFP_NOWAIT);
+ if (!ds)
+ return NULL;
+
+ ds->chan = c;
+
+ ds->desc_hw = dma_pool_zalloc(c->desc_pool, GFP_ATOMIC,
+ >desc_phys);
+ if (!ds->desc_hw) {
+ dev_dbg(dev, "out of memory for link descriptor\n");
+ kfree(ds);
+ return NULL;
+ }
+ ds->desc_cnt = num;
+
+ return ds;
+}
+
+static void ldma_chan_irq_en(struct ldma_chan *c)
+{
+ struct ldma_dev *d = to_ldma_dev(c->vchan.chan.device);
+ unsigned long flags;
+
+ spin_lock_irqsave(>dev_lock, flags);
+ writel(c->nr, d->base + DMA_CS);
+ writel(DMA_CI_EOP, d->base + DMA_CIE);
+ writel(BIT(c->nr), d->base + DMA_IRNEN);
+ spin_unlock_irqrestore(>dev_lock, flags);
+}
+
+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) {
+ spin_lock_irqsave(>vchan.lock, flags);
+ if (vchan_issue_pending(>vchan)) {
+ struct virt_dma_desc *vdesc;
+
+ /* Get the next descriptor */
+ vdesc = vchan_next_desc(>vchan);
+ if (!vdesc) {
+ c->ds = NULL;
+ return;
+ }
+ list_del(>node);
+ c->ds = to_lgm_dma_desc(vdesc);
you have set c->ds in dma_prep_slave_sg and the only way I can see that
you will not leak memory is that the client must terminate_sync() after
each transfer so that the synchronize callback is invoked between each
prep_sg/issue_pending/competion.
Yes, client must call dmaengine_synchronize after each transfer to make
sure free the memory assoicated with previously issued descriptors if any.
and also from the driver we are freeing up the descriptor from work
queue atfer each transfer.(addressed below comments **)
+ spin_unlock_irqrestore(>vchan.lock, flags);
+ ldma_chan_desc_hw_cfg(c, c->ds->desc_phys,
c->ds->desc_cnt);
+ ldma_chan_irq_en(c);
+ }
If there is nothing peding, you will leave the spinlock wide open...
Seems i misplaced the lock. i will fix it in next
Re: [PATCH v5 1/2] dt-bindings: dma: Add bindings for intel LGM SOC
Hi Rob, Thanks for your valuable comments. Please see my comments inline.. On 8/15/2020 4:32 AM, Rob Herring wrote: On Fri, Aug 14, 2020 at 01:26:09PM +0800, Amireddy Mallikarjuna reddy wrote: Add DT bindings YAML schema for DMA controller driver of Lightning Mountain(LGM) SoC. Signed-off-by: Amireddy Mallikarjuna reddy --- 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. --- .../devicetree/bindings/dma/intel,ldma.yaml| 319 + 1 file changed, 319 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/intel,ldma.yaml diff --git a/Documentation/devicetree/bindings/dma/intel,ldma.yaml b/Documentation/devicetree/bindings/dma/intel,ldma.yaml new file mode 100644 index ..9beaf191a6de --- /dev/null +++ b/Documentation/devicetree/bindings/dma/intel,ldma.yaml @@ -0,0 +1,319 @@ +# 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: + - chuanhua@intel.com + - mallikarjunax.re...@intel.com + +allOf: + - $ref: "dma-controller.yaml#" + +properties: + $nodename: + pattern: "^dma-controller(@.*)?$" + + "#dma-cells": + const: 1 Example says 3. OK, i will fix it. + + compatible: + anyOf: + - const: intel,lgm-cdma + - const: intel,lgm-dma2tx + - const: intel,lgm-dma1rx + - const: intel,lgm-dma1tx + - const: intel,lgm-dma0tx + - const: intel,lgm-dma3 + - const: intel,lgm-toe-dma30 + - const: intel,lgm-toe-dma31 'anyOf' doesn't make sense here. This should be a single 'enum'. ok. I will update it. + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + resets: + maxItems: 1 + + interrupts: + maxItems: 1 + + intel,dma-poll-cnt: + $ref: /schemas/types.yaml#definitions/uint32 + description: + DMA descriptor polling counter. It may need fine tune according + to the system application scenario. + + 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. + + intel,dma-drb: +type: boolean +description: + DMA descriptor read back to make sure data and desc synchronization. + + intel,dma-burst: +$ref: /schemas/types.yaml#definitions/uint32 +description: + Specifiy the DMA burst size(in dwords), the valid value will be 8, 16, 32. + Default is 16 for data path dma, 32 is for memcopy DMA. + + intel,dma-polling-cnt: +$ref: /schemas/types.yaml#definitions/uint32 +description: + DMA descriptor polling counter. It may need fine tune according to + the system application scenario. + + 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. + + intel,dma-orrc: +$ref: /schemas/types.yaml#definitions/uint32 +description: + DMA outstanding read counter. The maximum value is 16, and it may + need fine tune according to the system application scenarios. + + intel,dma-dburst-wr: +type: boolean +description: + Enable RX dynamic burst write. It only applies to RX DMA and memcopy DMA. + + + dma-ports: +type: object +description: + This sub-node must contain a sub-node for each DMA port. +properties: + '#address-cells': +const: 1 + '#size-cells': +const: 0 + +patternProperties: + "^dma-ports@[0-9]+$": + type: object + + properties: +reg: + items: +- enum: [0, 1, 2, 3, 4, 5] + description: + Which port this node refers to. + +intel,name: + $ref: /schemas/types.yaml#definitions/string-array + description: + Port name of each DMA port. No other DMA controller needs this, why do you? Answered below. (*ABC) + +intel,chans: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: + The channels included on this port. Format is channel start + number and how many channels on this port. Why does this need to be in DT? This all seems like it can be in the dma cells for each client. (*ABC) Yes. We need this. for dma0(lgm-cdma) old SOC supports 16
Re: [PATCH v4 1/2] dt-bindings: dma: Add bindings for intel LGM SOC
Thanks Rob for the review. Please see my comments inline. On 7/14/2020 1:22 AM, Rob Herring wrote: On Mon, Jul 13, 2020 at 11:39:49AM +0800, Reddy, MallikarjunaX wrote: Hi Thomas, Thanks for the review. My comments inline. On 7/9/2020 3:54 PM, Langer, Thomas wrote: -Original Message- From: devicetree-ow...@vger.kernel.org On Behalf Of Amireddy Mallikarjuna reddy Sent: Donnerstag, 9. Juli 2020 08:01 To: dmaeng...@vger.kernel.org; vk...@kernel.org; devicet...@vger.kernel.org; robh...@kernel.org Cc: linux-kernel@vger.kernel.org; Shevchenko, Andriy ; chuanhua@linux.intel.com; Kim, Cheol Yong ; Wu, Qiming ; malliamireddy...@gmail.com; Amireddy Mallikarjuna reddy Subject: [PATCH v4 1/2] dt-bindings: dma: Add bindings for intel LGM SOC Add DT bindings YAML schema for DMA controller driver of Lightning Mountain(LGM) SoC. Signed-off-by: Amireddy Mallikarjuna reddy --- v1: - Initial version. v2: - Fix bot errors. v3: - No change. v4: - Address Thomas langer comments Please read my comments again and then respond about the topics you ignored. I added some hints below again. Thanks. --- .../devicetree/bindings/dma/intel,ldma.yaml| 416 + 1 file changed, 416 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/intel,ldma.yaml diff --git a/Documentation/devicetree/bindings/dma/intel,ldma.yaml b/Documentation/devicetree/bindings/dma/intel,ldma.yaml new file mode 100644 index ..7f666b9812e4 --- /dev/null +++ b/Documentation/devicetree/bindings/dma/intel,ldma.yaml @@ -0,0 +1,416 @@ +# 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: + - chuanhua@intel.com + - mallikarjunax.re...@intel.com + +properties: + $nodename: + pattern: "^dma(@.*)?$" Please explain the difference to the common dma binding. No difference. we can use "^dma-controller(@.*)?$" as in the common binding. Its My bad. I missed the changes to include in this patch. Surely update in the upcoming patch. + + "#dma-cells": + const: 1 + + compatible: + anyOf: + - const: intel,lgm-cdma + - const: intel,lgm-dma2tx + - const: intel,lgm-dma1rx + - const: intel,lgm-dma1tx + - const: intel,lgm-dma0tx + - const: intel,lgm-dma3 + - const: intel,lgm-toe-dma30 + - const: intel,lgm-toe-dma31 Please explain why you need so many different compatible strings. This hw dma has 7 DMA instances. Some for datapath, some for memcpy and some for TOE. Some for TX only, some for RX only, and some for TX/RX(memcpy and ToE). dma TX/RX type we considered as driver specific data of each instance and used different compatible strings for each instance. And also idea is in future if any driver specific data of any particular instance we can handle. Here if dma name and type(tx or rx) will be accepted as devicetree attributes then we can move .name = "toe_dma31", & .type = DMA_TYPE_MCPY to devicetree. So that the compatible strings can be limited to two. intel,lgm-cdma & intel,lgm-hdma . Different compatibles are okay if the instances are different and we don't have properties to describe the differences. For some of what you have in this binding, I think it should be part of the consumer cells. please suggest us the better proposal. + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + resets: + maxItems: 1 + + interrupts: + maxItems: 1 + + intel,dma-poll-cnt: + $ref: /schemas/types.yaml#definitions/uint32 + description: + DMA descriptor polling counter. It may need fine tune according + to the system application scenario. + + 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. + + intel,dma-drb: +type: boolean +description: + DMA descriptor read back to make sure data and desc synchronization. + + intel,dma-burst: +$ref: /schemas/types.yaml#definitions/uint32 +description: + Specifiy the DMA burst size(in dwords), the valid value will be 8, 16, 32. + Default is 16 for data path dma, 32 is for memcopy DMA. + + intel,dma-polling-cnt: What's the difference with intel,dma-poll-cnt? This is redundant. I will remove it. +$ref: /schemas/types.yaml#definitions/uint32 +description: + DMA descriptor polling counter. It may need fine tune according to + the system application scenario. + + 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. + + intel,dma-orrc: +$ref: /schemas/types.yaml#definitions/uint3
Re: [PATCH v4 2/2] Add Intel LGM soc DMA support.
Thanks for the review Andy. My comments inline. On 7/9/2020 7:09 PM, Andy Shevchenko wrote: On Thu, Jul 09, 2020 at 02:01:06PM +0800, Amireddy Mallikarjuna reddy wrote: 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 DPlus compliant 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. +#include "../virt-dma.h" I didn't find any evidence this driver utilizes virt-dma API in full. For example, there is a virt_dma_desc structure and descriptor management around it. Why don't you use it? Lgm dma soc has its own hardware descriptor. and each dma channel is associated with a peripheral, it is one to one mapping between channel and associated peripheral.
Re: [PATCH v4 1/2] dt-bindings: dma: Add bindings for intel LGM SOC
Hi Thomas, Thanks for the review. My comments inline. On 7/9/2020 3:54 PM, Langer, Thomas wrote: -Original Message- From: devicetree-ow...@vger.kernel.org On Behalf Of Amireddy Mallikarjuna reddy Sent: Donnerstag, 9. Juli 2020 08:01 To: dmaeng...@vger.kernel.org; vk...@kernel.org; devicet...@vger.kernel.org; robh...@kernel.org Cc: linux-kernel@vger.kernel.org; Shevchenko, Andriy ; chuanhua@linux.intel.com; Kim, Cheol Yong ; Wu, Qiming ; malliamireddy...@gmail.com; Amireddy Mallikarjuna reddy Subject: [PATCH v4 1/2] dt-bindings: dma: Add bindings for intel LGM SOC Add DT bindings YAML schema for DMA controller driver of Lightning Mountain(LGM) SoC. Signed-off-by: Amireddy Mallikarjuna reddy --- v1: - Initial version. v2: - Fix bot errors. v3: - No change. v4: - Address Thomas langer comments Please read my comments again and then respond about the topics you ignored. I added some hints below again. Thanks. --- .../devicetree/bindings/dma/intel,ldma.yaml| 416 + 1 file changed, 416 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/intel,ldma.yaml diff --git a/Documentation/devicetree/bindings/dma/intel,ldma.yaml b/Documentation/devicetree/bindings/dma/intel,ldma.yaml new file mode 100644 index ..7f666b9812e4 --- /dev/null +++ b/Documentation/devicetree/bindings/dma/intel,ldma.yaml @@ -0,0 +1,416 @@ +# 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: + - chuanhua@intel.com + - mallikarjunax.re...@intel.com + +properties: + $nodename: + pattern: "^dma(@.*)?$" Please explain the difference to the common dma binding. No difference. we can use "^dma-controller(@.*)?$" as in the common binding. Its My bad. I missed the changes to include in this patch. Surely update in the upcoming patch. + + "#dma-cells": + const: 1 + + compatible: + anyOf: + - const: intel,lgm-cdma + - const: intel,lgm-dma2tx + - const: intel,lgm-dma1rx + - const: intel,lgm-dma1tx + - const: intel,lgm-dma0tx + - const: intel,lgm-dma3 + - const: intel,lgm-toe-dma30 + - const: intel,lgm-toe-dma31 Please explain why you need so many different compatible strings. This hw dma has 7 DMA instances. Some for datapath, some for memcpy and some for TOE. Some for TX only, some for RX only, and some for TX/RX(memcpy and ToE). dma TX/RX type we considered as driver specific data of each instance and used different compatible strings for each instance. And also idea is in future if any driver specific data of any particular instance we can handle. Here if dma name and type(tx or rx) will be accepted as devicetree attributes then we can move .name = "toe_dma31", & .type = DMA_TYPE_MCPY to devicetree. So that the compatible strings can be limited to two. intel,lgm-cdma & intel,lgm-hdma . please suggest us the better proposal. + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + resets: + maxItems: 1 + + interrupts: + maxItems: 1 + + intel,dma-poll-cnt: + $ref: /schemas/types.yaml#definitions/uint32 + description: + DMA descriptor polling counter. It may need fine tune according + to the system application scenario. + + 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. + + intel,dma-drb: +type: boolean +description: + DMA descriptor read back to make sure data and desc synchronization. + + intel,dma-burst: +$ref: /schemas/types.yaml#definitions/uint32 +description: + Specifiy the DMA burst size(in dwords), the valid value will be 8, 16, 32. + Default is 16 for data path dma, 32 is for memcopy DMA. + + intel,dma-polling-cnt: +$ref: /schemas/types.yaml#definitions/uint32 +description: + DMA descriptor polling counter. It may need fine tune according to + the system application scenario. + + 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. + + intel,dma-orrc: +$ref: /schemas/types.yaml#definitions/uint32 +description: + DMA outstanding read counter. The maximum value is 16, and it may + need fine tune according to the system application scenarios. + + intel,dma-dburst-wr: +type: boolean +description: + Enable RX dynamic burst write. It only applies to RX DMA and memcopy DMA. + + + dma-ports: +type: object +description: + This sub-node must contain a sub-node for each DMA port. +properties: + '#address-cells': +const: 1 + '#size-cells': +
Re: [PATCH v3 1/2] dt-bindings: dma: Add bindings for intel LGM SOC
Thanks Thomas for the review. My comments inline. On 6/23/2020 6:49 PM, Langer, Thomas wrote: Hi, I have some questions about the binding. Sorry I missed to ask during internal review, as I was busy with other tasks at that time. See my questions below. -Original Message- From: devicetree-ow...@vger.kernel.org On Behalf Of Amireddy Mallikarjuna reddy Sent: Dienstag, 23. Juni 2020 11:20 To: dmaeng...@vger.kernel.org; vk...@kernel.org; devicet...@vger.kernel.org; robh...@kernel.org Cc: linux-kernel@vger.kernel.org; Shevchenko, Andriy ; chuanhua@linux.intel.com; Kim, Cheol Yong ; Wu, Qiming ; malliamireddy...@gmail.com; Amireddy Mallikarjuna reddy Subject: [PATCH v3 1/2] dt-bindings: dma: Add bindings for intel LGM SOC Add DT bindings YAML schema for DMA controller driver of Lightning Mountain(LGM) SoC. Signed-off-by: Amireddy Mallikarjuna reddy --- v1: - Initial version. v2: - Fix bot errors. v3: - No change. --- .../devicetree/bindings/dma/intel,ldma.yaml| 428 + 1 file changed, 428 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/intel,ldma.yaml diff --git a/Documentation/devicetree/bindings/dma/intel,ldma.yaml b/Documentation/devicetree/bindings/dma/intel,ldma.yaml new file mode 100644 index ..d474c3e47126 --- /dev/null +++ b/Documentation/devicetree/bindings/dma/intel,ldma.yaml @@ -0,0 +1,428 @@ +# 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: + - chuanhua@intel.com + - mallikarjunax.re...@intel.com + +properties: + $nodename: + pattern: "^dma(@.*)?$" Why is this not "^dma-controller(@.*)?$" as in the common binding? Agreed. I will update it. + + "#dma-cells": + const: 1 + + compatible: + anyOf: + - const: intel,lgm-cdma + - const: intel,lgm-dma2tx + - const: intel,lgm-dma1rx + - const: intel,lgm-dma1tx + - const: intel,lgm-dma0tx + - const: intel,lgm-dma3 + - const: intel,lgm-toe_dma30 + - const: intel,lgm-toe_dma31 Why do you need so many different compatible strings? I assume the hw blocks are mostly the same, some of them limited to rx or tx. Why is it not possible to describe that as an attribute? Also, is there a difference in the toe_dma instances (which should not use "_" here!)? Or is it only the way they are connected to other hw blocks? In code they refer the same "hdma_ops" struct. If this is only about a name to be printed in the driver, maybe a "label" or "name" attribute will be accepted? we used different compatible strings only for driver specific data of each dma instances. If name and dma-type(tx or rx) can accepts as attribute, we can move these to device tree. So that the compatible strings can be limited to two. intel,lgm-cdma & intel,lgm-hdma or else please suggest us the better proposal. I will fix "_" in the compatible string in next version. + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + resets: + maxItems: 1 + + interrupts: + maxItems: 1 + + intel,dma-poll-cnt: + $ref: /schemas/types.yaml#definitions/uint32 + description: + DMA descriptor polling counter. It may need fine tune according + to the system application scenario. + + 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. + + intel,dma-drb: +type: boolean +description: + DMA descriptor read back to make sure data and desc synchronization. + + intel,dma-burst: +$ref: /schemas/types.yaml#definitions/uint32 +description: + Specifiy the DMA burst size(in dwords), the valid value will be 8, 16, 32. + Default is 16 for data path dma, 32 is for memcopy DMA. + + intel,dma-polling-cnt: +$ref: /schemas/types.yaml#definitions/uint32 +description: + DMA descriptor polling counter. It may need fine tune according to + the system application scenario. + + 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. + + intel,dma-orrc: +$ref: /schemas/types.yaml#definitions/uint32 +description: + DMA outstanding read counter. The maximum value is 16, and it may + need fine tune according to the system application scenarios. + + intel,dma-txendi: +$ref: /schemas/types.yaml#definitions/uint32 +description: + DMA TX endianness conversion due to SoC endianness difference. What does this mean? The SoC endianness is know to the driver at compile time. Is it the difference of the device connected to the dma? As I know the dma is used also for PCIe connected
