Skip to contentSkip to conversation

dmaengine: mcf-edma: add ColdFire mcf5441x eDMA support

DISCUSSION Angelo Dureghello 1 participant 1 message May 4, 2018 dmaengine

lore
Share & tools
lore

Angelo Dureghello

author
#0 lore raw

Hi Vinod,

thanks for the review,

Quoted 7 lines · Vinod Koul
Quoted 3 lines · Angelo Dureghello

This patch adds dma support for NXP mcf5441x (ColdFire) family.

ColdFire mcf5441x implements an edma hw module similar to the

Is it similar to to edma ?

It is similar to Freescale "edma" but with a different number of channels, a bit different register set, different interrupt structure, no channel multiplexer.

Quoted 11 lines
Quoted 7 lines

one implemented in Vybrid VFxxx controllers, but with a slightly different register set, more dma channels (64 instead of 32), a different interrupt mechanism and some other minor differences.

For the above reasons, modfying fsl-edma.c was too complex and likely too ugly. From here, the decision to create a different driver, but starting from fsl-edma.

can the common stuff be made into a lib and shared between then two rather than having a same driver or different drivers?

It should be possible to collect some common code in a kind of mcf_edma_core.c common module, but in this case i cannot then test the Vybrid edma after the changes since i miss that hardware.

Would be maybe possible for you to diff fsl-edma and this mcf-edma, just to confirm if i can still stay this way, or if moving to a library becomes mandatory ?

Quoted 10 lines
Quoted 8 lines

The driver has been tested with mcf5441x (stmark2 board) and dspi driver, it worked fine and seems reliable at least as a first initial version.

Signed-off-by Angelo Dureghello <[email protected]>

---
arch/m68k/configs/stmark2_defconfig

this should be a separate patch please

Ack.

Quoted 7 lines

Review

Quoted 4 lines
(quoted diff) +1 -0
 	  multiplexing capability for DMA request sources(slot).
 	  This module can be found on Freescale Vybrid and LS-1 SoCs.
 
+config MCF_EDMA

Alphabetical sort pls

Ack.

Quoted 4 lines
Quoted 1 line

+// SPDX-License-Identifier: GPL-2.0

Copyright info should be here in c99 style comments

Seems checkpatch.pl, for C files, does not like the C style initial line comment:

WARNING: Missing or malformed SPDX-License-Identifier tag in line 1 #87: FILE: drivers/dma/mcf-edma.c:1:
+/* SPDX-License-Identifier: GPL-2.0 */

While c++ type is accepted.

In contrary, in .h files it wants cpp // style and not C style.

Quoted 5 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

why do you need this, why not use dev_xxx

Well, pr_ style seems to simplify the call a bit, should be allowed but if you prefer i can move all to dev_ format.

Quoted 12 lines

Review

Quoted 9 lines
(quoted diff) +9 -0
+#define EDMA_CHANNELS		64
+#define EDMA_MASK_CH(x)		((x) & 0x3F)
+#define EDMA_MASK_ITER(x)	((x) & 0x7FFF)
+#define EDMA_TCD_MEM_ALIGN	32
+
+#define EDMA_TCD_ATTR_DSZ_8b	(0x0000)
+#define EDMA_TCD_ATTR_DSZ_16b	(0x0001)
+#define EDMA_TCD_ATTR_DSZ_32b	(0x0002)
+#define EDMA_TCD_ATTR_DSZ_16B	(0x0004)

BIT and GENMASK for these..

Ack.

Quoted 12 lines

Review

Quoted 9 lines
(quoted diff) +9 -0
+static unsigned int mcf_edma_get_tcd_attr(enum dma_slave_buswidth addr_width)
+{
+	switch (addr_width) {
+	case 1:
+		return EDMA_TCD_ATTR_SSZ_8b | EDMA_TCD_ATTR_DSZ_8b;
+	case 2:
+		return EDMA_TCD_ATTR_SSZ_16b | EDMA_TCD_ATTR_DSZ_16b;
+	case 4:
+	default:

why default not treated as error?

Ack, will fix that.

Quoted 11 lines
Quoted 11 lines
(quoted diff) +11 -0
+static int mcf_edma_slave_config(struct dma_chan *chan,
+				 struct dma_slave_config *cfg)
+{
+	struct mcf_edma_chan *mcf_chan = to_mcf_edma_chan(chan);
+
+	mcf_chan->esc.dir = cfg->direction;
+	if (cfg->direction == DMA_DEV_TO_MEM) {
+		mcf_chan->esc.dev_addr = cfg->src_addr;
+		mcf_chan->esc.addr_width = cfg->src_addr_width;
+		mcf_chan->esc.burst = cfg->src_maxburst;
+		mcf_chan->esc.attr = mcf_edma_get_tcd_attr(cfg->src_addr_width);
Quoted 9 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+	} else if (cfg->direction == DMA_MEM_TO_DEV) {
+		mcf_chan->esc.dev_addr = cfg->dst_addr;
+		mcf_chan->esc.addr_width = cfg->dst_addr_width;
+		mcf_chan->esc.burst = cfg->dst_maxburst;
+		mcf_chan->esc.attr = mcf_edma_get_tcd_attr(cfg->dst_addr_width);

please save both src/dstn details here, typically we dont at this point about the txn direction... direction comes with prep_xxx call

Ack.

Quoted 5 lines
Quoted 5 lines
(quoted diff) +5 -0
+static void mcf_edma_free_desc(struct virt_dma_desc *vdesc)
+{
+	struct mcf_edma_desc *mcf_desc;
+	int i;
+	struct edma_regs *regs;
Quoted 9 lines

Review

Quoted 6 lines
(quoted diff) +6 -0
+
+	mcf_desc = to_mcf_edma_desc(vdesc);
+	regs = mcf_desc->echan->edma->membase;
+
+	//trace_tcd(&regs->tcd[mcf_desc->echan->slave_id]);
+	//trace_regs(regs);

??

Sorry, forgot those, will remove them.

Quoted 8 lines
Quoted 8 lines
(quoted diff) +8 -0
+static int mcf_edma_irq_init(struct platform_device *pdev,
+				struct mcf_edma_engine *mcf_edma)
+{
+	int ret = 0, i;
+	struct resource *res;
+
+	res = platform_get_resource_byname(pdev,
+				IORESOURCE_IRQ, "edma-tx-00-15");
Quoted 8 lines

Review

Quoted 6 lines
(quoted diff) +6 -0
+	if (!res)
+		return -1;
+
+	for (ret = 0, i = res->start; i <= res->end; ++i) {
+		ret |= devm_request_irq(&pdev->dev, i,
+			mcf_edma_tx_handler, 0, "eDMA", mcf_edma);

you are explicitly freeing irq below, so why use devm_ ?

Ack, Will fix that.

Quoted 2 lines
Signature · 1 line
~Vinod

Many thanks,

Regards,
Angelo
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html