Re: [PATCH v10 1/4] dmaengine: fsl-edma: extract common fsl-edma code (no changes in behavior intended)
On 19-08-18, 19:27, Angelo Dureghello wrote: > This patch adds a new fsl-edma-common module to allow new > mcf-edma module code to use most of the fsl-edma code. Applied all, thanks -- ~Vinod
Re: [PATCH v7 1/2] dmaengine: fsl-edma: extract common fsl-edma code (no changes in behavior intended)
On 03-07-18, 12:37, Angelo Dureghello wrote: > This patch adds a new fsl-edma-common module to allow new > mcf-edma module code to use most of the fsl-edma code. > > Due to some differences between ColdFire edma (64 channels) and > fsl-edma (32 channels), as register set offsets and some other > points as the different interrupt organization and other minor > things, a common module can collect most of the code for both > 32 and 64 channel edma module versions. ?? You didn't see my email https://marc.info/?l=dmaengine=153060940829531=2 -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-m68k" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/2] dmaengine: fsl-edma: extract common fsl-edma code (no changes in behavior intended)
On 03-07-18, 10:52, Angelo Dureghello wrote: > On Tue, Jul 03, 2018 at 11:19:25AM +0530, Vinod wrote: > > On 01-07-18, 18:33, Angelo Dureghello wrote: > > > This patch adds a new fsl-edma-common module to allow new > > > mcf-edma module code to use most of the fsl-edma code. > > > > > > Due to some differences between ColdFire edma (64 channels) and > > > fsl-edma (32 channels), as register set offsets and some other > > > points as the different interrupt organization and other minor > > > things, a common module can collect most of the code for both > > > 32 and 64 channel edma module versions. > > > > I have applied these, thanks. > > > > While applying I saw a trailing whitespace error, have fixed that up as > > below. > > > Strange checkpatch.pl is not signalling it .. > > > Also, I saw sparse complain a lot, care to fix those too? > > > > Sure. Can send a new version soon. Not a new version as I have applied this, but updates on top of applied changes -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-m68k" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/2] dmaengine: fsl-edma: extract common fsl-edma code (no changes in behavior intended)
On 01-07-18, 18:33, Angelo Dureghello wrote: > This patch adds a new fsl-edma-common module to allow new > mcf-edma module code to use most of the fsl-edma code. > > Due to some differences between ColdFire edma (64 channels) and > fsl-edma (32 channels), as register set offsets and some other > points as the different interrupt organization and other minor > things, a common module can collect most of the code for both > 32 and 64 channel edma module versions. I have applied these, thanks. While applying I saw a trailing whitespace error, have fixed that up as below. Also, I saw sparse complain a lot, care to fix those too? -->8 commit 15efb0b488596a929bc00ce23aeb746ee6a7c14d Author: Vinod Koul Date: Tue Jul 3 11:16:46 2018 +0530 dmaengine: fsl-edma: fix trailing whitespace Signed-off-by: Vinod Koul diff --git a/drivers/dma/fsl-edma-common.h b/drivers/dma/fsl-edma-common.h index e87c2bec882b..8e3c609c62fa 100644 --- a/drivers/dma/fsl-edma-common.h +++ b/drivers/dma/fsl-edma-common.h @@ -159,4 +159,3 @@ int fsl_edma_terminate_all(struct dma_chan *chan); void fsl_edma_issue_pending(struct dma_chan *chan); #endif /* _FSL_EDMA_COMMON_H_ */ - -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-m68k" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] dmaengine: fsl-edma: add ColdFire mcf5441x edma support
On 30-06-18, 15:42, Angelo Dureghello wrote:
> Hi Vinod,
>
> fixed mostly all, but sorry, i have still two questions before
> proceeding,
>
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +// Copyright (c) 2013-2014 Freescale Semiconductor, Inc
> > > +// Copyright (c) 2017 Sysam, Angelo Dureghello
> > > +/*
> > > + * drivers/dma/mcf-edma.c
> > > + *
> > > + * Driver for the Freescale ColdFire 64-ch eDMA implementation,
> > > + * derived from drivers/dma/fsl-edma.c.
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > it
> > > + * under the terms of the GNU General Public License as published by
> > > the
> > > + * Free Software Foundation; either version 2 of the License, or (at
> > > your
> > > + * option) any later version.
> > > + */
> >
> > again, no need for text
> >
>
> It is not clear to me now how the initial header should be (i guess for
> all the 3 c files at this point).
>
> Do you want just something as :
>
> // SPDX-License-Identifier: GPL-2.0
> // Copyright (c) 2013-2014 Freescale Semiconductor, Inc
> // Copyright (c) 2017 Sysam, Angelo Dureghello
That would be nice. Text is no longer required as SPDX represents that.
Copyright needs to be retained.
>
> And nothing else ?
>
> Majority of the files in the dma folder has also generally a line with
> the file name and path, a brief driver explaination and the reduced GPL
> licence text, and, as imx-sdma.c often copyrights at the end. So what is
> the current rule ?
Above and if you would like add a line explaining the role of driver.
File name and path are not required and gets stale as people update
stuff in future.
> > > +static int __init mcf_edma_init(void)
> > > +{
> > > + return platform_driver_register(_edma_driver);
> > > +}
> > > +subsys_initcall(mcf_edma_init);
> >
> > why subsys_initcall?
> >
>
> I find subsys_initcall in several dma drivers, my understanding is that
> it initializes the driver before other drivers can use it.
> It also sets the driver as built in only.
> This seems ok for my case.
Yes that is the case, but in your case I would like to know why you
would want this. Others doing is not a good enough justification and you
need to find your reason :)
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] dmaengine: fsl-edma: add ColdFire mcf5441x edma support
On 28-06-18, 18:56, Angelo Dureghello wrote: > Vinod, > what do you think, am i near to a possible "accept" in a v6 or v7 ? > Or do you see any additional great job to do or other important > blocking points ? I review to merge :) If I dont find any issues I will merge straight away, there is nothing like v6/v7 HTH -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-m68k" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] dmaengine: fsl-edma: add ColdFire mcf5441x edma support
On 28-06-18, 09:43, Geert Uytterhoeven wrote: > Hi Vinod, > > On Thu, Jun 28, 2018 at 9:29 AM Vinod wrote: > > On 28-06-18, 08:50, Geert Uytterhoeven wrote: > > > On Thu, Jun 28, 2018 at 8:29 AM Vinod wrote: > > > > On 22-06-18, 11:44, Angelo Dureghello wrote: > > > > > obj-$(CONFIG_EP93XX_DMA) += ep93xx_dma.o > > > > > obj-$(CONFIG_FSL_DMA) += fsldma.o > > > > > obj-$(CONFIG_FSL_EDMA) += fsl-edma.o fsl-edma-common.o > > > > > +obj-$(CONFIG_MCF_EDMA) += mcf-edma.o fsl-edma-common.o > > > > > > > > that makes kernel have two copies of common.o one in thsi driver and one > > > > > > Does it? It's a common pattern in several Makefiles (e.g. > > > drivers/net/ethernet/8390/Makefile and drivers/scsi/Makefile) > > > > won't each static symbol be part each one? > > Remember, obj-y is a list, and IIRC it's filtered for duplicates. > > > What about when they are modules? > > Same thing, you'll have fsl-edma-common.ko, and fsl-edma.ko and/or > mcf-edma.ko. Yeah that is right, I missed the list part -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-m68k" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] dmaengine: fsl-edma: add ColdFire mcf5441x edma support
On 28-06-18, 08:50, Geert Uytterhoeven wrote: > Hi Vinod, > > On Thu, Jun 28, 2018 at 8:29 AM Vinod wrote: > > On 22-06-18, 11:44, Angelo Dureghello wrote: > > > obj-$(CONFIG_EP93XX_DMA) += ep93xx_dma.o > > > obj-$(CONFIG_FSL_DMA) += fsldma.o > > > obj-$(CONFIG_FSL_EDMA) += fsl-edma.o fsl-edma-common.o > > > +obj-$(CONFIG_MCF_EDMA) += mcf-edma.o fsl-edma-common.o > > > > that makes kernel have two copies of common.o one in thsi driver and one > > Does it? It's a common pattern in several Makefiles (e.g. > drivers/net/ethernet/8390/Makefile and drivers/scsi/Makefile) won't each static symbol be part each one? What about when they are modules? > > in previous one why not do: > > > > CONFIG_FSL_COMMON += fsl-edma-common.o > > CONFIG_FSL_EDMA += fsl-edma.o > > CONFIG_MCF_EDMA += mcf-edma.o > > > > and you select CONFIG_FSL_COMMON in both FSL and MCF Kconfig? > > That's what he had originally, until I suggested to do the above, as > nothing else > needed the CONFIG_FSL_COMMON symbol. Hmmm, okay what are we gaining from this approach? -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-m68k" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] dmaengine: fsl-edma: add ColdFire mcf5441x edma support
On 22-06-18, 11:44, Angelo Dureghello wrote:
> obj-$(CONFIG_EP93XX_DMA) += ep93xx_dma.o
> obj-$(CONFIG_FSL_DMA) += fsldma.o
> obj-$(CONFIG_FSL_EDMA) += fsl-edma.o fsl-edma-common.o
> +obj-$(CONFIG_MCF_EDMA) += mcf-edma.o fsl-edma-common.o
that makes kernel have two copies of common.o one in thsi driver and one
in previous one why not do:
CONFIG_FSL_COMMON += fsl-edma-common.o
CONFIG_FSL_EDMA += fsl-edma.o
CONFIG_MCF_EDMA += mcf-edma.o
and you select CONFIG_FSL_COMMON in both FSL and MCF Kconfig?
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2013-2014 Freescale Semiconductor, Inc
> +// Copyright (c) 2017 Sysam, Angelo Dureghello
> +/*
> + * drivers/dma/mcf-edma.c
> + *
> + * Driver for the Freescale ColdFire 64-ch eDMA implementation,
> + * derived from drivers/dma/fsl-edma.c.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + */
again, no need for text
> +static irqreturn_t mcf_edma_tx_handler(int irq, void *dev_id)
> +{
> + struct fsl_edma_engine *mcf_edma = dev_id;
> + struct edma_regs *regs = _edma->regs;
> + unsigned int ch;
> + struct fsl_edma_chan *mcf_chan;
> + u64 intmap;
> +
> + intmap = ioread32(regs->inth);
> + intmap <<= 32;
> + intmap |= ioread32(regs->intl);
> + if (!intmap)
> + return IRQ_NONE;
> +
> + for (ch = 0; ch < mcf_edma->n_chans; ch++) {
> + if (intmap & (0x1 << ch)) {
intmap & BIT(ch)
> +static irqreturn_t mcf_edma_err_handler(int irq, void *dev_id)
> +{
> + struct fsl_edma_engine *mcf_edma = dev_id;
> + struct edma_regs *regs = _edma->regs;
> + unsigned int err, ch;
> +
> + err = ioread32(regs->errl);
> + if (!err)
> + return IRQ_NONE;
> +
> + for (ch = 0; ch < (EDMA_CHANNELS / 2); ch++) {
> + if (err & (0x1 << ch)) {
here as well
> +static int mcf_edma_remove(struct platform_device *pdev)
> +{
> + struct fsl_edma_engine *mcf_edma = platform_get_drvdata(pdev);
> +
> + fsl_edma_cleanup_vchan(_edma->dma_dev);
> + dma_async_device_unregister(_edma->dma_dev);
at this point your irqs are still registered and running. You vchan
tasklet maybe still pending to be eecuted and can be scheduled again
> +static int __init mcf_edma_init(void)
> +{
> + return platform_driver_register(_edma_driver);
> +}
> +subsys_initcall(mcf_edma_init);
why subsys_initcall?
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/2] dmaengine: fsl-edma: extract common fsl-edma code (no changes in behavior intended)
On 22-06-18, 11:44, Angelo Dureghello wrote: > --- > drivers/dma/Makefile | 2 +- > drivers/dma/fsl-edma-common.c | 683 +++ > drivers/dma/fsl-edma-common.h | 175 > drivers/dma/fsl-edma.c| 739 ++ use git format -M, that will help with renames > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (c) 2013-2014 Freescale Semiconductor, Inc > +// Copyright (c) 2017 Sysam, Angelo Dureghello > +/* > + * drivers/dma/fsl-edma-common.c > + * > + * Common code for Freescale the edma 32 or 64 channel version. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation version 2. > + * > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > + * kind, whether express or implied; without even the implied warranty > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ should we add this text, SPDX tag suffices -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-m68k" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/4] dmaengine: fsl-edma: add config and makefile changes for mcf-edma
On 12-06-18, 18:34, Angelo Dureghello wrote: > This patch adds Kconfig and makefile changes to add ColdFire > mcf5441x family edma support. > A new fsl-edma-common module has been added, to collect common > code to fsl-edma. I think the way you send patch series has some issue and threading is broken, so the patches are all over my inbox. Can you please repost with threading fixed > > Signed-off-by: Angelo Dureghello > --- > Changes for v2: > - patch splitted into 4 > - add fsl-edma-common > > Changes for v3: > - remove Kconfig FSL_EDMA_COMMON and set fsl-edma-common.o as > additional object entry where needed. > --- > drivers/dma/Kconfig | 13 + > drivers/dma/Makefile | 3 ++- > 2 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig > index 6d61cd023633..c7d45e69f021 100644 > --- a/drivers/dma/Kconfig > +++ b/drivers/dma/Kconfig > @@ -220,6 +220,7 @@ config FSL_EDMA > depends on OF > select DMA_ENGINE > select DMA_VIRTUAL_CHANNELS > + select FSL_EDMA_COMMON > help > Support the Freescale eDMA engine with programmable channel > multiplexing capability for DMA request sources(slot). > @@ -327,6 +328,18 @@ config LPC18XX_DMAMUX > Enable support for DMA on NXP LPC18xx/43xx platforms > with PL080 and multiplexed DMA request lines. > > +config MCF_EDMA > + tristate "Freescale eDMA engine support, ColdFire mcf5441x SoCs" > + depends on M5441x > + select DMA_ENGINE > + select DMA_VIRTUAL_CHANNELS > + select FSL_EDMA_COMMON > + help > + Support the Freescale ColdFire eDMA engine, 64-channel > + implementation that performs complex data transfers with > + minimal intervention from a host processor. > + This module can be found on Freescale ColdFire mcf5441x SoCs. > + > config MMP_PDMA > bool "MMP PDMA support" > depends on ARCH_MMP || ARCH_PXA || COMPILE_TEST > diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile > index 0f62a4d49aab..cce5e3400580 100644 > --- a/drivers/dma/Makefile > +++ b/drivers/dma/Makefile > @@ -32,7 +32,8 @@ 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_MCF_EDMA) += mcf-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.o > -- > 2.17.0 -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-m68k" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dmaengine: mcf-edma: add ColdFire mcf5441x eDMA support
Hi Angelo, On 26-05-18, 22:50, Angelo Dureghello wrote: > > wouldn't it be easier to just make common parts and then add edma specific > > code. > > If I was doing this it would be my apprach and that way code edma specific > > will > > be lesser and faster review > > > > I tried to set up a common module, but couldn't reach any good point. > > Issues are: > 1) Edma register set between 32 and 64ch is similar, but some offsets/names > are not matching between the 2 variants, some registers names are swapped over > the reg. address range, > 2) interrupt numbers and scheme is still different, handler implementation > comes > different, > 3) as a corollary of the above, all the common functions that needs to access > edma registers should use same structure pointers. I could use a union > someway but points where register are accessed are many, and i should > differentiate the access in each case, referencing to a different structure > in each case. > > If you have any idea on how i could reach a common module, with 2 different > registers set, that's welcome. > I stay on the thought that a separate 64-channel module is the best > way to go here. > > Currently, as Freescale "edma" variants, i know: > > Vybrid VFXXX 32ch DMA multiplexer reg.set 1 > Kynetis K70 (CortexM4) 32ch DMA multiplexer reg.set 1 > imx8xx (coming)32ch no multiplexerreg.set 1 > MPC57xxk 32ch DMA multiplexer reg.set 1 > ColdFire mcf5441x 64ch no multiplexerreg.set 2 <--- > > There may me other cpu using this fsl edma module but not in my knowledge > right now. > > So i still think at the end, to have 2 separate drivers for the 32 and 64 > variant is good and probably the most ordered/clean solution. Okay there are few ways we can do this. One is to use helpers for register access and these helpers are different for the variant you are loaded on. Another is to use register offsets which are set based on the variant loaded.. HTH -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-m68k" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dmaengine: mcf-edma: add ColdFire mcf5441x eDMA support
On 22-05-18, 23:28, Angelo Dureghello wrote: > Hi Vinod, > > On Mon, May 07, 2018 at 07:45:35PM +0530, Vinod Koul wrote: > > On Fri, May 04, 2018 at 09:18:19PM +0200, Angelo Dureghello wrote: > > > Hi Vinod, > > > > > > thanks for the review, > > > > > > On Thu, May 03, 2018 at 10:18:30PM +0530, Vinod Koul wrote: > > > > On Wed, Apr 25, 2018 at 10:08:17PM +0200, Angelo Dureghello wrote: > > > > > 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. > > > > ok > > > > > > > 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. > > > > Sure you should send the patches and folks who care about fsl driver > > would look it up and test > > > > > 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 ? > > > > well since you know the IP you would make a better guess on that, best is > > to check register sets in drivers > > > I fixed all the discussed points. > > Actaully mcf-edma (ColdFire) has a slightly different register set (due to 64 > channels in place of 16 of fsl-edma) and, for the same reason, a different > DMA interrupt structure. > Also, i simplified some parts of the driver considering ColdFire (mcf) > big endian architecture. > > So i would send a rev 2 patch with all the fixes, than eventually in a second > phase i may try to create some common code, but at least we have the ColdFire > DMA. What do you think ? wouldn't it be easier to just make common parts and then add edma specific code. If I was doing this it would be my apprach and that way code edma specific will be lesser and faster review -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-m68k" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dmaengine: mcf-edma: add ColdFire mcf5441x eDMA support
On Fri, May 04, 2018 at 09:18:19PM +0200, Angelo Dureghello wrote: > Hi Vinod, > > thanks for the review, > > On Thu, May 03, 2018 at 10:18:30PM +0530, Vinod Koul wrote: > > On Wed, Apr 25, 2018 at 10:08:17PM +0200, Angelo Dureghello wrote: > > > 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. ok > > > 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. Sure you should send the patches and folks who care about fsl driver would look it up and test > 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 ? well since you know the IP you would make a better guess on that, best is to check register sets in drivers > > > +// 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. SC99 comments style is // SPDX-License-Identifier: GPL-2.0 Point is the copyright should be added is same formar i.e., // Copyright 20018 - foo bar this line should follow the spdx line > > > + > > > +#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. in hindsight dev_ makes better sense, been there done that :) -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-m68k" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dmaengine: mcf-edma: add ColdFire mcf5441x eDMA support
On Wed, Apr 25, 2018 at 10:08:17PM +0200, Angelo Dureghello wrote:
> 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 ?
> 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?
>
> 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 <ang...@sysam.it>
> ---
> arch/m68k/configs/stmark2_defconfig| 2 +
this should be a separate patch please
> 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
> +// SPDX-License-Identifier: GPL-2.0
Copyright info should be here in c99 style comments
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
why do you need this, why not use dev_xxx
> +#define EDMA_CHANNELS64
> +#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 (0x)
> +#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..
> +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?
> +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);
> + } 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
> +static void mcf_edma_free_desc(struct virt_dma_desc *vdesc)
> +{
> + struct mcf_edma_desc *mcf_desc;
> + int i;
> + struct edma_regs *regs;
> +
> + mcf_desc = to_mcf_edma_desc(vdesc);
> + regs = mcf_desc->echan->edma->membase;
> +
> + //trace_tcd(>tcd[mcf_desc->echan->slave_id]);
> + //trace_regs(regs);
??
> +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");
> + if (!res)
> + return -1;
> +
> + for (ret = 0, i = res->start; i <= res->end; ++i) {
> + ret |= devm_request_irq(>dev, i,
> + mcf_edma_tx_handler, 0, "eDMA", mcf_edma);
you are explicitly freeing irq below, so why use devm_ ?
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
