Sdma cleanup by paulstelian97 · Pull Request #8 · paulstelian97/sof

paulstelian97 · 2020-03-18T13:12:31Z

No description provided.

dbaluta · 2020-03-18T13:14:15Z

src/platform/imx8m/lib/dai.c

@@ -18,6 +18,17 @@ static SHARED_DATA struct dai sai[] = {
 	.index = 3,
 	.plat_data = {
 		.base = SAI_3_BASE,


This can go as a separate PR. Please add some more info in the commit message. The current commit message is not very informative.

dbaluta · 2020-03-18T13:15:10Z

src/platform/imx8m/lib/dai.c

+			.offset = SAI_3_BASE + REG_SAI_RDR0,
+			.depth = 16,
+			.handshake = 4,
+		},


Maybe we could add some meaningful macros for event 4 and 5.

dbaluta · 2020-03-18T13:16:06Z

src/platform/imx8m/lib/dai.c

+		.fifo[SOF_IPC_STREAM_PLAYBACK] = {
+			.offset = SAI_3_BASE + REG_SAI_TDR0,
+			.depth = 16,
+			/* Handshake is SDMA hardware event */


Looking in the upstream SAI Linux driver (sound/soc/fsl/fsl_sai.c) depth for i.MX8MQ is 128. I think that SAI is the same on 8MP vs 8MP.

Depth is in number of words and is interpreted by SDMA driver. Will need to check.

dbaluta · 2020-03-18T13:17:01Z

src/platform/imx8m/include/platform/lib/dma.h

+
+#define SDMA2_IRQ	7 /* TODO What? */
+#define SDMA2_IRQ_NAME	"irqstr2" /* TODO find the correct one */
+


Whe should remove the TODOs here. If we are sure the numbers are correct.

I am not 100% certain. I may also end up using irqstr_get_sof_int like on the other platforms (perhaps my calculations were in fact incorrect).

dbaluta · 2020-03-18T13:17:49Z

src/platform/imx8m/lib/dma.c

@@ -12,6 +12,7 @@
 #include <sof/spinlock.h>

 extern struct dma_ops dummy_dma_ops;


Here we should update the commit message once all questions are answered.

dbaluta · 2020-03-18T13:19:09Z

src/platform/imx8m/include/platform/lib/dma.h


-#define DMA_ID_EDMA0	0
+#define DMA_ID_SDMA2	0
 #define DMA_ID_HOST	1


good catch. I think we can make the following PRs:

PR with cleanups in preparation for SDMA driver.

PR adding SDMA driver

We could get 1) accepted faster so that i.MX8MP boots fine with qemu.

dbaluta · 2020-03-18T13:20:00Z

src/drivers/imx/CMakeLists.txt

 	timer.c
+	sdma.c
 )



TODO: Add selectable config for SDMA. e.g: CONFIG_IMX_SDMA. This can be done later.

I think I can do this in the same PR that will have the implementation.

dbaluta · 2020-03-18T13:21:00Z

src/drivers/imx/CMakeLists.txt

@@ -6,6 +6,7 @@ add_local_sources(sof
 	interrupt.c


We could cleanup commit message, be deleting DO NOT MERGE and this commit requires the cleanup. Also, would be nice to add a link to a RM containing SDMA bits. Maybe link to 8MQ RM.

That's a reminder for myself that I need to clean up some bits from this commit. We have seen how unreliable direct links for the RM are so I will look into how to make a reasonably-persistent link for future reference.

dbaluta · 2020-03-18T13:21:56Z

src/drivers/imx/sdma.c

+
+struct sdma_bd {
+	/* SDMA BD configuration */
+	uint32_t config;


maybe first time use full name for BD, not sure what it stands for.

dbaluta · 2020-03-18T13:23:12Z

src/drivers/imx/sdma.c

+	trace_sdma_error("sdma_set_overrides(%d, %d)", event_override, host_override);
+	dma_reg_update_bits(channel->dma, SDMA_EVTOVR, BIT(channel->index), event_override ? BIT(channel->index) : 0);
+	dma_reg_update_bits(channel->dma, SDMA_HOSTOVR, BIT(channel->index), host_override ? BIT(channel->index) : 0);
+}


I think this line goes over 80 chars. In fact I think there are several places where this happens. Maybe run checkpatch.ppl with --strict?

Will fix the lines-too-long issues as I look again into this file. I still have a few dummy functions that need to go.

dbaluta · 2020-03-18T13:23:31Z

src/drivers/imx/sdma.c

+
+	/* Writeback descriptors and CCB */
+
+	dcache_writeback_region(c0data->descriptors, sizeof(c0data->descriptors[0]));


No need for new line here.

dbaluta · 2020-03-18T13:24:14Z

src/drivers/imx/sdma.c

+	sdma_set_overrides(c0, true, false);
+	ret = dma_start(&dma->chan[0]);
+
+	if (ret < 0)


Same here, no need for new line.

dbaluta · 2020-03-18T13:25:13Z

src/drivers/imx/sdma.c

+
+	trace_sdma("SDMA: probe");
+	trace_sdma_error("sdma_probe");
+


Just keep trace_sdma here.

The trace_sdma_error calls will all go (some may be converted to trace or trace_verbose if they are important enough)

dbaluta · 2020-03-18T13:26:29Z

src/drivers/imx/sdma.c

+	}
+	trace_sdma_error("sdma_set_event(%d, %d)", channel->index, eventnum);
+	struct sdma_chan *pdata = dma_chan_get_data(channel);
+	dma_reg_update_bits(channel->dma, SDMA_CHNENBL(eventnum), BIT(channel->index), BIT(channel->index));


I would suggest on declaring all the variables at the beginning of function.

dbaluta · 2020-03-18T13:26:57Z

src/drivers/imx/sdma.c

+	for (int i = 0; i < dma->plat_data.channels; i++) {
+		struct dma_chan_data *channel = &dma->chan[i];
+		struct sdma_chan *cdata = &pdata->chan_pdata[i];
+


Declare all the variable at the beginning of function. Hope this wont trigger an compiler error for unused variable.

dbaluta · 2020-03-18T13:27:47Z

src/drivers/imx/sdma.c

+		return -EINVAL;
+	}
+	bool src_may_change, dst_may_change;
+	switch (config->direction) {


move declaration at the beginning of function.

dbaluta · 2020-03-18T13:29:07Z

src/include/sof/drivers/sdma.h

+#define SDMA_SCRIPT_SHP2MCU_OFF		979
+#define SDMA_SCRIPT_MCU2SHP_OFF		1048
+
+#endif /* __SOF_DRIVERS_SDMA_H__ */


Other than that it looks good.

Signed-off-by: Paul Olaru <paul.olaru@nxp.com>

The extra memory blocks are required for SDMA proper functioning. TODO figure out which of the increases are actually required. Signed-off-by: Paul Olaru <paul.olaru@nxp.com>

Some i.MX platforms use SDMA (Smart DMA) controller for DMA related operations. This commit adds the initial functionality for SDMA. Signed-off-by: Paul Olaru <paul.olaru@nxp.com>

This includes interrupt number and a tweak used when probing the driver (which must match hardware configuration). Signed-off-by: Paul Olaru <paul.olaru@nxp.com>

This commit adds the definition of the SDMA controller we use for SOF, namely SDMA2 (the other instances of SDMA are used for non-audio-related DMA in the system). This DMAC can serve the SAI. [DO NOT MERGE] because I need help in how to account for channel 0 being used for management functionality, with only channels 1-31 being usable by SOF directly. Signed-off-by: Paul Olaru <paul.olaru@nxp.com>

SDMA is in dma_array[1], and uses a single interrupt for all channels. Signed-off-by: Paul Olaru <paul.olaru@nxp.com>

paulstelian97 requested a review from dbaluta as a code owner March 18, 2020 13:12

dbaluta reviewed Mar 18, 2020

View reviewed changes

paulstelian97 force-pushed the sdma_cleanup branch 4 times, most recently from 09ecb50 to f9ca3f6 Compare March 19, 2020 12:47

Paul Olaru added 7 commits March 20, 2020 11:17

drivers: imx: interrupt: Adjust IRQ_STEER registers for i.MX8

ed42879

Signed-off-by: Paul Olaru <paul.olaru@nxp.com>

[DO NOT MERGE] platform: imx8m: Add more heap memory

c42dad0

The extra memory blocks are required for SDMA proper functioning. TODO figure out which of the increases are actually required. Signed-off-by: Paul Olaru <paul.olaru@nxp.com>

drivers: imx: sdma: Add SDMA support to SOF

7c7cbd2

Some i.MX platforms use SDMA (Smart DMA) controller for DMA related operations. This commit adds the initial functionality for SDMA. Signed-off-by: Paul Olaru <paul.olaru@nxp.com>

platform: imx8m: sdma: Add SDMA definitions for i.MX8M platform

43b1123

This includes interrupt number and a tweak used when probing the driver (which must match hardware configuration). Signed-off-by: Paul Olaru <paul.olaru@nxp.com>

platform: imx8m: Fix DMA domain initialization

a68d55d

SDMA is in dma_array[1], and uses a single interrupt for all channels. Signed-off-by: Paul Olaru <paul.olaru@nxp.com>

Debug aids

3059764

paulstelian97 force-pushed the sdma_cleanup branch from f9ca3f6 to 3059764 Compare March 20, 2020 09:19

paulstelian97 force-pushed the master branch from df8b969 to 27bae08 Compare July 15, 2020 10:16


		#define SDMA2_IRQ 7 /* TODO What? */
		#define SDMA2_IRQ_NAME "irqstr2" /* TODO find the correct one */

		@@ -12,6 +12,7 @@
		#include <sof/spinlock.h>

		extern struct dma_ops dummy_dma_ops;


		/* Writeback descriptors and CCB */

		dcache_writeback_region(c0data->descriptors, sizeof(c0data->descriptors[0]));

Conversation

paulstelian97 commented Mar 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants