drivers: dma: Add Andestech atcdmac300 driver. #59041

kevinwang821020 · 2023-06-08T01:42:52Z

Support the Andes atcdmac300 dma driver.
Also add the dma property in spi node.

edersondisouza · 2023-06-21T17:05:07Z

drivers/dma/dma_andes_atcdmac300.c

+
+	cfg_blocks = cfg->head_block;
+	if (cfg_blocks == NULL) {
+		return -EINVAL;


Shouldn't the spinlock be unlocked in this - and all following - early returns?

Yes you are right.
It is early returns, we have update the source code for this request.
Thank you.

edersondisouza

Seems ok, just some nits. But it seems the build issues on CI are relevant - could you please take a look?

edersondisouza · 2023-06-28T19:35:35Z

drivers/dma/dma_andes_atcdmac300.h

+#define DMA_INT_STATUS_CH_MSK(ch) (0x111 << ch)
+
+#define INWORD(x) sys_read32(x)
+#define OUTWORD(x, d) sys_write32(d, x)


Why redefine these?

Thanks for your remind, these macro is redundant.
We have update the source code for it.

edersondisouza · 2023-06-28T19:36:18Z

drivers/dma/dma_andes_atcdmac300.c

+
+	return 0;
+}
+static int dma_atcdmac300_get_status(const struct device *dev,


Nit: missing newline before this.

Oops, thank you, we have fixed this problem.

My points were addressed, but not sure I can approve the whole PR.

teburd

Would be nice to use a simpler object pool here rather than a heap

teburd · 2023-06-29T00:37:02Z

drivers/dma/dma_andes_atcdmac300.c

+#include <zephyr/logging/log.h>
+LOG_MODULE_REGISTER(dma_andes_atcdmac300);
+
+K_HEAP_DEFINE(dma_chain, 1024);


a pool of chain objects would be more sensible here than a heap and simpler, generally Zephyr drivers shouldn't be using a general purpose heap given the misra rules

I got it. We weren't aware of that.
We have update the source code.

carlocaione · 2023-06-29T08:25:21Z

drivers/dma/dma_andes_atcdmac300.h

+#define DMA_IDR (DEV_CFG(dev)->base + 0x00)
+#define DMA_CFG (DEV_CFG(dev)->base + 0x10)
+#define DMA_CTRL (DEV_CFG(dev)->base + 0x20)
+#define DMA_ABORT (DEV_CFG(dev)->base + 0x24)
+#define DMA_INT_STATUS (DEV_CFG(dev)->base + 0x30)
+#define DMA_CH_ENABLE (DEV_CFG(dev)->base + 0x34)
+
+#define DMA_CH_OFFSET(ch) (ch * 0x20)
+#define DMA_CH_CTRL(ch) (DEV_CFG(dev)->base + 0x40 + DMA_CH_OFFSET(ch))
+#define DMA_CH_TRANSIZE(ch) (DEV_CFG(dev)->base + 0x44 + DMA_CH_OFFSET(ch))
+#define DMA_CH_SRC_ADDR_L(ch) (DEV_CFG(dev)->base + 0x48 + DMA_CH_OFFSET(ch))
+#define DMA_CH_SRC_ADDR_H(ch) (DEV_CFG(dev)->base + 0x4C + DMA_CH_OFFSET(ch))
+#define DMA_CH_DST_ADDR_L(ch) (DEV_CFG(dev)->base + 0x50 + DMA_CH_OFFSET(ch))
+#define DMA_CH_DST_ADDR_H(ch) (DEV_CFG(dev)->base + 0x54 + DMA_CH_OFFSET(ch))
+#define DMA_CH_LL_PTR_L(ch) (DEV_CFG(dev)->base + 0x58 + DMA_CH_OFFSET(ch))
+#define DMA_CH_LL_PTR_H(ch) (DEV_CFG(dev)->base + 0x5C + DMA_CH_OFFSET(ch))


add an explicit (dev) parameter to all these macros.

Sorry for that we didn't notice that the macro like DEV_CFG is redundant now.
We have done the modification for it.

carlocaione · 2023-06-29T08:25:40Z

drivers/dma/dma_andes_atcdmac300.h

+#define DEV_CFG(dev) ((const struct dma_atcdmac300_cfg * const) (dev)->config)
+#define DEV_DATA(dev) ((struct dma_atcdmac300_data *) (dev)->data)


We deprecated the usage of these macros.

Sorry for that we didn't notice that the macro like DEV_CFG is redundant now.
We have done the modification for it.

carlocaione · 2023-06-29T08:26:09Z

drivers/dma/dma_andes_atcdmac300.h

+#define INWORD(x) sys_read32(x)
+#define OUTWORD(x, d) sys_write32(d, x)


carlocaione · 2023-06-29T08:27:51Z

tests/drivers/dma/loop_transfer/boards/adp_xc7k_ae350.conf

@@ -0,0 +1 @@
+CONFIG_CACHE_ENABLE=n


uhm, it's a bit too late but having this symbol is wrong since it overlaps with the cache symbols we already have in place in Zephyr. Can you ask some Andes folk to remove it?

I got it!
We use the CONFIG_ICACHE and CONFIG_DCACHE to replace it now.

kevinwang821020 · 2023-07-26T08:48:48Z

Sorry to bother you reviewers, I would like to know if there are any other parts of source code that I can improve/refined.

Thanks for helping us.

carlocaione

In general LGTM, not vouching for the DMA bits.

kevinwang821020 · 2023-09-15T08:56:02Z

Sorry to bother you reviewers, there are some build error in ci testing about qemu_x86_64 platform, it seems like we can't avoid it.
What can we do for it if we want to pass the ci tesing.
Thank you very much.

carlocaione · 2023-09-18T07:01:08Z

Sorry to bother you reviewers, there are some build error in ci testing about qemu_x86_64 platform, it seems like we can't avoid it. What can we do for it if we want to pass the ci tesing. Thank you very much.

That was recently fixed. Please rebase.

teburd · 2023-07-28T13:21:24Z

drivers/dma/Kconfig.andes_atcdmac300

@@ -0,0 +1,12 @@
+# Andestech ATCDMAC300 configuration options
+# Copyright (c) 2021 Andes Technology Corporation.


Oh! Thank you for the reminder, and I'm sorry I overlooked it.

teburd · 2023-09-20T13:58:20Z

drivers/dma/dma_andes_atcdmac300.c

+		return -EINVAL;
+	}
+
+	key = k_spin_lock(&data->chan[channel].lock);


Is there a particular reason to have a per channel spin lock? The semantics of DMA in Zephyr are predominantly single owner, as in a single owner is responsible for each channel so no locking is needed.

If you need to have share a channel among call contexts its up to the caller to provide synchronization.

Oh, I got it.
We have removed the spin_lock for per channel.
Use the common spin_lock of dma driver to handle the critical section of dma common register of all channels.

Thanks a lot for this request.

It seems like there's still a per channel spin lock in use, has the change been pushed?

teburd · 2023-09-20T13:59:38Z

drivers/dma/dma_andes_atcdmac300.h

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#ifndef ZEPHYR_DRIVERS_DMA_ANDES_ATCDMAC300_H_


Are there going to be other variants of this dma controller implemented? If not then the header file contents could be placed in the .c file directly.

No, there will only be this implemented.
We have update the source code to integrate them.

Thanks a lot.

teburd · 2023-09-27T13:21:07Z

drivers/dma/dma_andes_atcdmac300.c

+#define DMA_CH_CTRL_DSTREQ_MASK		(0x0F << DMA_CH_CTRL_DSTREQ_POS)
+#define DMA_CH_CTRL_DSTREQ(n) \
+		(((n) << DMA_CH_CTRL_DSTREQ_POS) & DMA_CH_CTRL_DSTREQ_MASK)
+#define DMA_CH_CTRL_INTABT		(1 << 3)


BIT() macro could be used in place of these shifts

drivers/dma/dma_andes_atcdmac300.c

teburd · 2023-09-27T13:23:05Z

drivers/dma/dma_andes_atcdmac300.c

+		return -EINVAL;
+	}
+
+	key = k_spin_lock(&data->chan[channel].lock);


It seems like there's still a per channel spin lock in use, has the change been pushed?

teburd · 2023-09-27T13:25:15Z

Looking pretty good! Usage of the FIELD_PREP/FIELD_GET/GENMASK/BIT macros is highly encouraged but not a requirement. The removal of the per channel spin lock to me is.

Support the Andes atcdmac300 dma driver. Signed-off-by: Kevin Wang <[email protected]>

kevinwang821020 · 2023-09-28T06:49:59Z

@teburd
I am so sorry for that, I lost a commit which is for removing channel spin_lock.
I apologize for the inconvenience.
We have update the source code for it(also handle the FIELD_PREP/FIELD_GET/GENMASK/BIT suggestion)

Thanks a lot.

kevinwang821020 · 2023-10-13T02:42:13Z

Sorry for bothering the reviewers, we know that the version 3.5 is about to be released, and we look forward that this source code can be included in this release. Is there any good way to do this or any recommendations?

Thank you.

kevinwang821020 requested a review from nashif as a code owner June 8, 2023 01:42

zephyrbot added area: DMA Direct Memory Access area: Devicetree Binding PR modifies or adds a Device Tree binding area: RISCV RISCV Architecture (32-bit & 64-bit) labels Jun 8, 2023

zephyrbot requested review from carlocaione, edersondisouza, fkokosinski, galak, katsuster, kgugala, mgielda, teburd and tgorochowik June 8, 2023 01:43

zephyrbot assigned teburd Jun 8, 2023

kevinwang821020 force-pushed the dma_driver branch 5 times, most recently from ba8c995 to 7cd16d0 Compare June 9, 2023 01:37

edersondisouza requested changes Jun 21, 2023

View reviewed changes

kevinwang821020 force-pushed the dma_driver branch from 7cd16d0 to 089f9e6 Compare June 27, 2023 09:13

kevinwang821020 requested a review from edersondisouza June 28, 2023 02:03

kevinwang821020 force-pushed the dma_driver branch from 089f9e6 to 2504e44 Compare June 28, 2023 05:59

edersondisouza reviewed Jun 28, 2023

View reviewed changes

edersondisouza previously approved these changes Jun 28, 2023

View reviewed changes

teburd requested changes Jun 29, 2023

View reviewed changes

kevinwang821020 force-pushed the dma_driver branch from 2504e44 to 4a03e25 Compare June 29, 2023 08:14

carlocaione requested changes Jun 29, 2023

View reviewed changes

jimmyzhe mentioned this pull request Jul 3, 2023

soc: riscv: andes_v5: update ae350 soc #59813

Merged

kevinwang821020 force-pushed the dma_driver branch from 7400e52 to 6d35786 Compare July 26, 2023 08:08

carlocaione previously approved these changes Jul 28, 2023

View reviewed changes

kevinwang821020 requested a review from edersondisouza August 10, 2023 05:52

kevinwang821020 dismissed carlocaione’s stale review via 75aa78f August 23, 2023 08:07

kevinwang821020 force-pushed the dma_driver branch from 6d35786 to 75aa78f Compare August 23, 2023 08:07

kevinwang821020 force-pushed the dma_driver branch 3 times, most recently from 2b06fad to 604b79b Compare September 15, 2023 08:20

kevinwang821020 force-pushed the dma_driver branch from 604b79b to dd41701 Compare September 19, 2023 05:57

carlocaione previously approved these changes Sep 19, 2023

View reviewed changes

teburd reviewed Sep 20, 2023

View reviewed changes

kevinwang821020 dismissed carlocaione’s stale review via ed0abac September 27, 2023 07:28

kevinwang821020 force-pushed the dma_driver branch 2 times, most recently from ed0abac to 1499096 Compare September 27, 2023 07:30

teburd reviewed Sep 27, 2023

View reviewed changes

drivers: dma: Add Andestech atcdmac300 driver.

adf1c43

Support the Andes atcdmac300 dma driver. Signed-off-by: Kevin Wang <[email protected]>

kevinwang821020 force-pushed the dma_driver branch from 1499096 to adf1c43 Compare September 28, 2023 06:38

teburd approved these changes Sep 28, 2023

View reviewed changes

kevinwang821020 requested a review from carlocaione October 2, 2023 02:05

carlocaione approved these changes Oct 13, 2023

View reviewed changes

fabiobaltieri added this to the v3.6.0 milestone Oct 13, 2023

carlescufi merged commit d3a73cd into zephyrproject-rtos:main Oct 20, 2023
19 checks passed

kevinwang821020 deleted the dma_driver branch October 25, 2023 02:09

kevinwang821020 mentioned this pull request Sep 23, 2024

Nominate myself (kevinwang821020) as Contributor #78825

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: dma: Add Andestech atcdmac300 driver. #59041

drivers: dma: Add Andestech atcdmac300 driver. #59041

kevinwang821020 commented Jun 8, 2023

edersondisouza Jun 21, 2023

kevinwang821020 Jun 27, 2023

edersondisouza left a comment

edersondisouza Jun 28, 2023 •

edited

Loading

kevinwang821020 Jul 6, 2023

edersondisouza Jun 28, 2023

kevinwang821020 Jul 6, 2023

teburd left a comment

teburd Jun 29, 2023

kevinwang821020 Jul 6, 2023

carlocaione Jun 29, 2023

kevinwang821020 Jul 6, 2023

carlocaione Jun 29, 2023

kevinwang821020 Jul 6, 2023

carlocaione Jun 29, 2023

carlocaione Jun 29, 2023

kevinwang821020 Jul 6, 2023

kevinwang821020 commented Jul 26, 2023

carlocaione left a comment

kevinwang821020 commented Sep 15, 2023

carlocaione commented Sep 18, 2023

teburd Jul 28, 2023

kevinwang821020 Sep 27, 2023

teburd Sep 20, 2023 •

edited

Loading

kevinwang821020 Sep 27, 2023

teburd Sep 27, 2023

teburd Sep 20, 2023

kevinwang821020 Sep 27, 2023

teburd Sep 27, 2023

teburd Sep 27, 2023

teburd commented Sep 27, 2023 •

edited

Loading

kevinwang821020 commented Sep 28, 2023

kevinwang821020 commented Oct 13, 2023 •

edited

Loading

		#define DEV_CFG(dev) ((const struct dma_atcdmac300_cfg * const) (dev)->config)
		#define DEV_DATA(dev) ((struct dma_atcdmac300_data *) (dev)->data)

		#define INWORD(x) sys_read32(x)
		#define OUTWORD(x, d) sys_write32(d, x)

		@@ -0,0 +1,12 @@
		# Andestech ATCDMAC300 configuration options
		# Copyright (c) 2021 Andes Technology Corporation.

		@@ -0,0 +1 @@
		CONFIG_CACHE_ENABLE=n

drivers: dma: Add Andestech atcdmac300 driver. #59041

drivers: dma: Add Andestech atcdmac300 driver. #59041

Conversation

kevinwang821020 commented Jun 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edersondisouza left a comment

Choose a reason for hiding this comment

edersondisouza Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teburd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinwang821020 commented Jul 26, 2023

carlocaione left a comment

Choose a reason for hiding this comment

kevinwang821020 commented Sep 15, 2023

carlocaione commented Sep 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teburd Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teburd commented Sep 27, 2023 • edited Loading

kevinwang821020 commented Sep 28, 2023

kevinwang821020 commented Oct 13, 2023 • edited Loading

edersondisouza Jun 28, 2023 •

edited

Loading

teburd Sep 20, 2023 •

edited

Loading

teburd commented Sep 27, 2023 •

edited

Loading

kevinwang821020 commented Oct 13, 2023 •

edited

Loading