RE: [v5 2/6] dmaengine: fsl-qdma: Add qDMA controller driver for Layerscape SoCs

Wen He
2018-05-31T02:00:46Z

> -----Original Message-----
> From: Vinod Koul [mailto:vinod.koul@linaro.org]
> Sent: 2018年5月30日 18:28
> To: Wen He <wen.he_1@nxp.com>
> Cc: dmaengine@vger.kernel.org; robh+dt@kernel.org;
> devicetree@vger.kernel.org; Leo Li <leoyang.li@nxp.com>; Jiafei Pan
> <jiafei.pan@nxp.com>; Jiaheng Fan <jiaheng.fan@nxp.com>
> Subject: Re: [v5 2/6] dmaengine: fsl-qdma: Add qDMA controller driver for
> Layerscape SoCs
>
> On 29-05-18, 10:38, Wen He wrote:
>
> > > > > > + /*
> > > > > > + * Clear the command queue interrupt detect register for all
> > > queues.
> > > > > > + */
> > > > > > + qdma_writel(fsl_qdma, 0xffffffff, block +
> > > > > > +FSL_QDMA_BCQIDR(0));
> > > > >
> > > > > bunch of writes with 0xffffffff, can you explain why? Also helps
> > > > > to make a macro for this
> > > > >
> > > >
> > > > Maybe I missed that I should defined the value to the macro and
> > > > add
> > > comment.
> > > > Right?
> > >
> > > that would help, but why are you writing 0xffffffff at all these places?
> >
> > This value is from the datasheet.
> > Should I write comments to here?
>
> Yes that would help explaining what this does..
>

Got it. Thanks

> > > > > > +static void fsl_qdma_issue_pending(struct dma_chan *chan) {
> > > > > > + struct fsl_qdma_chan *fsl_chan = to_fsl_qdma_chan(chan);
> > > > > > + struct fsl_qdma_queue *fsl_queue = fsl_chan->queue;
> > > > > > + unsigned long flags;
> > > > > > +
> > > > > > + spin_lock_irqsave(&fsl_queue->queue_lock, flags);
> > > > > > + spin_lock(&fsl_chan->vchan.lock);
> > > > > > + if (vchan_issue_pending(&fsl_chan->vchan))
> > > > > > + fsl_qdma_enqueue_desc(fsl_chan);
> > > > > > + spin_unlock(&fsl_chan->vchan.lock);
> > > > > > + spin_unlock_irqrestore(&fsl_queue->queue_lock, flags);
> > > > >
> > > > > why do we need two locks, and since you are doing vchan why
> > > > > should you add your own lock on top
> > > > >
> > > >
> > > > Yes, we need two locks.
> > > > As you know, the QDMA support multiple virtualized blocks for
> > > > multi-core
> > > support.
> > > > so we need to make sure that muliti-core access issues.
> > >
> > > but why cant you use vchan lock for all?
> > >
> >
> > We can't only use vchan lock for all. otherwise enqueue action will be
> interrupted.
>
> I think it is possible to use only vchan lock
>

I tried that if I use only vchan lock then qdma will be can't work.
Do you have a other good idea?

Best Regards,
Wen

> --
> ~Vinod