dmaengine: fsl-edma: extract common fsl-edma code (no changes in behavior intended)
PATCH v8 1/3 dmaengine: fsl-edma: extract common fsl-edma code (no changes in behavior intended)
Hi Stefan,
i am about to post a v9 in short.
Quoted 120 lines · Stefan Agner
Quoted 94 lines · Krzysztof Kozlowski
Quoted 93 lines · Stefan Agner
Quoted 76 lines · Angelo Dureghello
This patch adds a new fsl-edma-common module to allow new mcf-edma module code to use most of the fsl-edma code.
Signed-off-by Angelo Dureghello <[email protected]>---
Changes for v2:
- patch splitted into 4
- add mcf-edma as minimal different parts from fsl-edmaChanges for v3:
noneChanges for v4:
- patch simplified from 4/4 into 2/2
- collecting all the mcf-edma-related changesChanges for v5:
noneChanges for v6:
- adjusted comment header
- fixed bit shift withBIT()
- we need to free the interrupts atremove(), so removed alldevm_interrupt related callsChanges for v7:
noneChanges for v8:
- patch rewritten from scratch, splitted into 3, common code isolated, minimal changes from the original Freescale code have been done. The patch has been tested with both Iris + Colibri Vybrid VF50 and stmark2/mcf54415 Coldfire boards.--- drivers/dma/Makefile | 2 +- drivers/dma/fsl-edma-common.c | 576 ++++++++++++++++++++++++++++ drivers/dma/fsl-edma-common.h | 196 ++++++++++ drivers/dma/fsl-edma.c | 697 +--------------------------------- 4 files changed, 774 insertions(+), 697 deletions(-) create mode 100644 drivers/dma/fsl-edma-common.c create mode 100644 drivers/dma/fsl-edma-common.hdrivers/dma/Makefile +1
-1@@ -31,7 +31,7 @@ obj-$(CONFIG_DW_AXI_DMAC) += dw-axi-dmac/ obj-$(CONFIG_DW_DMAC_CORE) += dw/ obj-$(CONFIG_EP93XX_DMA) += ep93xx_dma.o obj-$(CONFIG_FSL_DMA) += fsldma.o -obj-$(CONFIG_FSL_EDMA) += fsl-edma.o +obj-$(CONFIG_FSL_EDMA) += fsl-edma.o fsl-edma-common.o obj-$(CONFIG_FSL_RAID) += fsl_raid.o obj-$(CONFIG_HSU_DMA) += hsu/ obj-$(CONFIG_IMG_MDC_DMA) += img-mdc-dma.odrivers/dma/fsl-edma-common.c new file +15
-0@@ -0,0 +1,576 @@ +// SPDX-License-Identifier: GPL-2.0 +// +// Copyright (c) 2013-2014 Freescale Semiconductor, Inc +// Copyright (c) 2017 Sysam, Angelo Dureghello <[email protected]> + +#include <linux/dmapool.h> +#include <linux/module.h> +#include <linux/slab.h> + +#include "fsl-edma-common.h" + +/* +* R/W functions for big- or little-endian registers: +* The eDMA controller's endian is independent of the CPU core's endian. +* For the big-endian IP module, the offset for 8-bit or 16-bit registers drivers/dma/fsl-edma-common.c line 25
Quoted 10 lines
drivers/dma/fsl-edma-common.c new file +10
-0+* should also be swapped opposite to that in little-endian IP. +*/ +u32 edma_readl (struct fsl_edma_engine *edma,void __iomem *addr) +{ +if (edma->big_endian) +return ioread32be (addr); +else +return ioread32 (addr); +} +EXPORT_SYMBOL_GPL (edma_readl);In 3/3 you link the common object into the two modules individually:
obj-$(CONFIG_FSL_EDMA) += fsl-edma.o fsl-edma-common.o obj-$(CONFIG_MCF_EDMA) += mcf-edma.o fsl-edma-common.o
Quoted 6 lines
Quoted 3 lines
Therefor you do not access those functions from another module (they are within the module). No exporting should be necessary. Drop all those exports.
The fsl-edma-common will be its own module so the exports are necessary for proper linking/modpost.
Hm, oh I see, I got that wrong.
We could use
fsl-edma-all-y = fsl-edma.o fsl-edma-common.o
obj-$(CONFIG_FSL_EDMA) += fsl-edma-all.o
mcf-edma-all-y = mcf-edma.o fsl-edma-common.o
obj-$(CONFIG_MCF_EDMA) += mcf-edma-all.oto create two modules but that duplicates some code. It probably wouldn't matter in practise since the two IPs are used on different architecture...
However, if we stay with the three modules approach, we should introduce a hidden config symbol, e.g.
config
FSL_EDMA_COMMON
tristate
Quoted 9 lines
In FSL_EDMA/MCF_EDMA use
selectFSL_EDMA_COMMONAnd in the
Makefileobj-$(CONFIG_FSL_EDMA_COMMON) += fsl-edma-common.o obj-$(CONFIG_FSL_EDMA) += fsl-edma.o obj-$(CONFIG_MCF_EDMA) += mcf-edma.o
There was already a discussion with Vinod and Geert on this, and i finally previously changed things in this way
obj-$(CONFIG_FSL_EDMA) += fsl-edma.o fsl-edma-common.o
The idea was to simplify things avoiding to use an additional config symbol, as i did initially. It's a common pattern in several Makefiles (e.g.drivers/net/ethernet/8390/Makefile and drivers/scsi/Makefile).
Also we have a single common code part since obj-y is a list, and IIRC it's filtered for duplicates.
So if can be ok for you too, i would not roll back again.
Quoted 6 lines
However, I do not like that the compiler can no longer inline simple acesors like
edma_(readl|writel). So if we stay with the three modules approach then move at least the accesors and to_fsl_edma_chan/to_fsl_edma_desc as inline functions to the header file.
I agree too, my initial patch was a single separate driver. But it resulted in too much duplicated code, so a common module has been required. Btw, seems i cannot inline those functions. fsl-edma-common.h is included from both fsl-edma-common.c and mcf/fsl-edma.c resulting in multiple definitions linking error.
Mainly until v7 there was several discussions and a first approval, if possible i would not change again heavily the patch structure for a bug that seemss already been fixed.
So i fixed all the possible points of this thread and organized next patch as
1/4 dmaengine: fsl-edma: extract common fsl-edma code (no changes .. 2/4 dmaengine: fsl-edma: add edma version and configurable registers 3/4 dmaengine: fsl-edma: fix macros
4/4 dmaengine: fsl-edma: add ColdFire mcf5441x edma support
Let me know if you have any other point or if can proceed.
Best regards,
Angelo Dureghello
Quoted 20 lines
--
StefanQuoted 13 lines
Best regards,
KrzysztofQuoted 9 lines
If possible I would prefer if you start with cleanup/conversions, then split-up and finally add functionality.
So ideally:
1. Use macros for preprocessor defines (where you move to BIT/GENMASK)
2. Split
3. Add EDMA macros etc.
4. Add ColdFire mcf5441x edma support--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html