DMAENGINE: driver for the ARM PL080/PL081 PrimeCells
in [Kernel]
|
Prev: AppArmor: update Maintainer and Documentation/kernel-parameters.txt
Next: [GIT PULL] hopefully final SCSI fixes for 2.6.35-rc6
From: Dan Williams on 6 Aug 2010 14:00 On Thu, Aug 5, 2010 at 12:30 AM, Linus Walleij <linus.ml.walleij(a)gmail.com> wrote: > 2010/8/5 Alessandro Rubini <rubini(a)unipv.it>: > >>> Peter, Alessandro and Viresh especially: you will hopefully use this >>> driver for ARM refboards, Nomadik and SPEAr, can you provide >>> an Acked-by:? >> >> I'm sorry I can't test it these times, I must delay stuff to >> the last week of August. > > That's be a Tested-by: tag, Acked-by: was more about whether you > think it looks sane I believe, see Documentation/SubmittingPatches. We have the Acked-by from Viresh, Alessandro looks like he is interested in trying it out, the driver is self contained, marked experimental, and you have a track-record of fixing things up, so I'll include it with the idea it will be easier to fixup in-tree. One thing that should be on the todo list is to review the usage of memory barriers. I don't think they are serving the purpose that you think they are. For example in pl08x_irq() there is: + /* + * Clear only the terminal interrupts on channels we processed + */ + writel(mask, pl08x->base + PL080_TC_CLEAR); + mb(); + + return mask ? IRQ_HANDLED : IRQ_NONE; Which seems to imply you want to ensure the interrupt bits are cleared before the handler returns. But the only way you can guarantee that the write has actually taken effect is to read back the register. Does the amba bus interface make this guarantee with a barrier? If so then we need a comment about why we need a full barrier versus just a wmb()? +static void pl08x_pause_phy_chan(struct pl08x_phy_chan *ch) +{ + u32 val; + + /* Set the HALT bit and wait for the FIFO to drain */ + val = readl(ch->base + PL080_CH_CONFIG); + val |= PL080_CONFIG_HALT; + writel(val, ch->base + PL080_CH_CONFIG); + + /* Wait for channel inactive */ + while (pl08x_phy_channel_busy(ch)) + ; + mb(); +} Doesn't the while loop guarantee that the write has taken effect? +static void pl08x_ensure_on(struct pl08x_driver_data *pl08x) +{ + u32 val; + + val = readl(pl08x->base + PL080_CONFIG); + val &= ~(PL080_CONFIG_M2_BE | PL080_CONFIG_M1_BE | PL080_CONFIG_ENABLE); + /* We implictly clear bit 1 and that means little-endian mode */ + val |= PL080_CONFIG_ENABLE; + mb(); + writel(val, pl08x->base + PL080_CONFIG); + mb(); +} This almost looks like you are using mb() as a compiler barrier, but that is taken care of by readl / writel. Speaking of which is amba always little-endian, should we instead be using __raw_{read|write}l in this driver? -- Dan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Dan Williams on 6 Aug 2010 14:40 On Fri, Aug 6, 2010 at 10:54 AM, Dan Williams <dan.j.williams(a)intel.com> wrote: > On Thu, Aug 5, 2010 at 12:30 AM, Linus Walleij > <linus.ml.walleij(a)gmail.com> wrote: >> 2010/8/5 Alessandro Rubini <rubini(a)unipv.it>: >> >>>> Peter, Alessandro and Viresh especially: you will hopefully use this >>>> driver for ARM refboards, Nomadik and SPEAr, can you provide >>>> an Acked-by:? >>> >>> I'm sorry I can't test it these times, I must delay stuff to >>> the last week of August. >> >> That's be a Tested-by: tag, Acked-by: was more about whether you >> think it looks sane I believe, see Documentation/SubmittingPatches. > > We have the Acked-by from Viresh, Alessandro looks like he is > interested in trying it out, the driver is self contained, marked > experimental, and you have a track-record of fixing things up, so I'll > include it with the idea it will be easier to fixup in-tree. ....except this version no longer compiles: drivers/dma/amba-pl08x.c: In function 'dma_set_runtime_config': drivers/dma/amba-pl08x.c:1399: error: 'struct dma_slave_config' has no member named 'maxburst' drivers/dma/amba-pl08x.c:1405: error: 'struct dma_slave_config' has no member named 'addr' drivers/dma/amba-pl08x.c:1408: error: 'struct dma_slave_config' has no member named 'addr_width' drivers/dma/amba-pl08x.c:1464: error: 'struct dma_slave_config' has no member named 'addr_width' drivers/dma/amba-pl08x.c:1464: error: 'struct dma_slave_config' has no member named 'maxburst' Fix that up and we can try to get it in the second half of the merge window. -- Dan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Linus Walleij on 6 Aug 2010 19:00 2010/8/6 Dan Williams <dan.j.williams(a)intel.com>: >�One > thing that should be on the todo list is to review the usage of memory > barriers. �I don't think they are serving the purpose that you think > they are. All of those are from Peters original implementation, I never use any barriers so they actually looked alien to me. Unless I get a clear rationale from Peter I'll just remove them all for the time being in the fix-up version due after updating to generic slave control. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Linus Walleij on 9 Aug 2010 07:00
2010/8/6 Dan Williams <dan.j.williams(a)intel.com>: > ...except this version no longer compiles: Hm the v3 version works fine with me, but anyway, I mailed out a v4 that also removes the unneeded mb() macros. > Fix that up and we can try to get it in the second half of the merge window. Great, thanks. I'll work on getting this in place to run with the PrimeCell extensions and proof-of-concept for ARM RealView reference designs next. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |