Re: [Patch v5 1/2] dmaengine: add Qualcomm BAM dma driver

[Vinod Koul <vinod.koul@intel.com>]

On Tue, Feb 11, 2014 at 11:49:10AM -0600, Josh Cartwright wrote:
> On Tue, Feb 11, 2014 at 11:00:48PM +0530, Vinod Koul wrote:
> > On Tue, Feb 04, 2014 at 02:42:35PM -0600, Andy Gross wrote:
> > > Add the DMA engine driver for the QCOM Bus Access Manager (BAM) DMA controller
> > > found in the MSM 8x74 platforms.
> > > 
> > > Each BAM DMA device is associated with a specific on-chip peripheral.  Each
> > > channel provides a uni-directional data transfer engine that is capable of
> > > transferring data between the peripheral and system memory (System mode), or
> > > between two peripherals (BAM2BAM).
> > > 
> > > The initial release of this driver only supports slave transfers between
> > > peripherals and system memory.
> > > 
> > > Signed-off-by: Andy Gross <agross@codeaurora.org>
> >
> > > +++ b/drivers/dma/qcom_bam_dma.c
> [..]
> > > +static void bam_reset_channel(struct bam_chan *bchan)
> > > +{
> > > +	struct bam_device *bdev = bchan->bdev;
> > > +
> > > +	/* reset channel */
> > > +	writel_relaxed(1, bdev->regs + BAM_P_RST(bchan->id));
> > > +	writel_relaxed(0, bdev->regs + BAM_P_RST(bchan->id));
> > > +
> > > +	/* don't allow reorder of the channel reset */
> > > +	wmb();
> > Documentation/memory-barriers.txt describes wmb() as a CPU barier but based on
> > above you want it to be a compiler barrier then you should do 1st write,
> > barrier(), second write.
> 
> It could also be that the intent was to prevent these writes from being
> ordered before setting the initialized flag below, either way the
> comment could be made clearer.
yes for that case, but i am suspecting the comment is correct as it would make
sense to ensure reset is in sequence...

-- 
~Vinod
> 
> > > +
> > > +	/* make sure hw is initialized when channel is used the first time  */
> > > +	bchan->initialized = 0;
> > > +}
> 
--