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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Vinod [mailto:vkoul@xxxxxxxxxx]
> Sent: 2018年5月29日 18:20
> To: Wen He <wen.he_1@xxxxxxx>
> Cc: dmaengine@xxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; Leo Li <leoyang.li@xxxxxxx>; Jiafei Pan
> <jiafei.pan@xxxxxxx>; Jiaheng Fan <jiaheng.fan@xxxxxxx>
> Subject: Re: [v5 2/6] dmaengine: fsl-qdma: Add qDMA controller driver for
> Layerscape SoCs
> 
> On 29-05-18, 09:59, Wen He wrote:
> > > On 25-05-18, 19:19, Wen He wrote:
> > >
> > > > +/**
> > > > + * struct fsl_qdma_format - This is the struct holding describing
> compound
> > > > + *			    descriptor format with qDMA.
> > > > + * @status:		    This field which describes command status and
> > > > + *			    enqueue status notification.
> > > > + * @cfg:		    This field which describes frame offset and
> frame
> > > > + *			    format.
> > > > + * @addr_lo:		    This field which indicating the start of the
> buffer
> > > > + *			    holding the compound descriptor of the lower 32-bits
> > > > + *			    address in memory 40-bit address.
> > > > + * @addr_hi:		    This field's the same as above field, but
> point
> > > high
> > > > + *			    8-bits in memory 40-bit address.
> > > > + * @__reserved1:	    Reserved field.
> > > > + * @cfg8b_w1:		    This field which describes compound
> descriptor
> > > > + *			    command queue origin produced by qDMA and
> > > dynamic
> > >
> > > you may remove 'This field which describes'... in above lines, give
> > > reader no information :)
> > >
> >
> > Ok, so I remove 'this filed which describes' but keep the second half, right?
> 
> right
> 
> > > > +	for (i = 0; i < queue_num; i++) {
> > > > +		if (queue_size[i] > FSL_QDMA_CIRCULAR_DESC_SIZE_MAX
> > > > +			|| queue_size[i] <
> FSL_QDMA_CIRCULAR_DESC_SIZE_MIN) {
> > > > +			dev_err(&pdev->dev, "Get wrong queue-sizes.\n");
> > > > +			return NULL;
> > >
> > > the indents here are bad for reading..
> > >
> >
> > So I add a empty line in here?
> 
> No, indent of trailing condition is same as code block, that causes confusion,
> they should have different indent
> 

Got it.

> > > > +	/*
> > > > +	 * 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?

> > > > +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.

Best Regards,
Wen

> --
> ~Vinod
?韬{.n?????%??檩??w?{.n???Ё?撖)?骅w*jg????????G??⒏⒎?:+v????????????"??????



[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux PCI]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux