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.
> > 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 ?
> >
> > 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 <angelo@xxxxxxxx>
> > ---
> > arch/m68k/configs/stmark2_defconfig
> this should be a separate patch please
>
Ack.
> > 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.
> > +// 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.
> > +
> > +#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.
> > +#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.
> > +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.
> > +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
>
Ack.
> > +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(®s->tcd[mcf_desc->echan->slave_id]);
> > + //trace_regs(regs);
>
> ??
>
Sorry, forgot those, will remove them.
> > +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(&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.
> --
> ~Vinod
Many thanks,
Regards,
Angelo
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
