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

Wen He
2018-05-29T09:59:43Z

> -----Original Message-----
> From: dmaengine-owner@vger.kernel.org
> [mailto:dmaengine-owner@vger.kernel.org] On Behalf Of Vinod
> Sent: 2018年5月29日 15:07
> 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 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?

> > + * debug field.
> > + * @data Pointer to the memory 40-bit address, describes
> DMA
> > + * source informaion and DMA destination information.
>
> typo informaion
>

I am sorry , It's stupid.

> > +static struct fsl_qdma_queue *fsl_qdma_alloc_queue_resources(
> > + struct platform_device *pdev,
> > + unsigned int queue_num)
> > +{
> > + struct fsl_qdma_queue *queue_head, *queue_temp;
> > + int ret, len, i;
> > + unsigned int queue_size[FSL_QDMA_QUEUE_MAX];
> > +
> > + if (queue_num > FSL_QDMA_QUEUE_MAX)
> > + queue_num = FSL_QDMA_QUEUE_MAX;
> > + len = sizeof(*queue_head) * queue_num;
> > + queue_head = devm_kzalloc(&pdev->dev, len, GFP_KERNEL);
> > + if (!queue_head)
> > + return NULL;
> > +
> > + ret = device_property_read_u32_array(&pdev->dev, "queue-sizes",
> > + queue_size, queue_num);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Can't get queue-sizes.\n");
> > + return NULL;
> > + }
> > +
> > + 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?

> > +static int fsl_qdma_queue_transfer_complete(struct fsl_qdma_engine
> > +*fsl_qdma) {
> > + struct fsl_qdma_queue *fsl_queue = fsl_qdma->queue;
> > + struct fsl_qdma_queue *fsl_status = fsl_qdma->status;
> > + struct fsl_qdma_queue *temp_queue;
> > + struct fsl_qdma_comp *fsl_comp;
> > + struct fsl_qdma_format *status_addr;
> > + struct fsl_qdma_format *csgf_src;
> > + struct fsl_pre_status pre;
> > + void __iomem *block = fsl_qdma->block_base;
> > + u32 reg, i;
> > + bool duplicate, duplicate_handle;
> > +
> > + memset(&pre, 0, sizeof(struct fsl_pre_status));
> > +
> > + while (1) {
> > + duplicate = 0;
> > + duplicate_handle = 0;
> > + reg = qdma_readl(fsl_qdma, block + FSL_QDMA_BSQSR);
> > + if (reg & FSL_QDMA_BSQSR_QE)
> > + return 0;
> > + status_addr = fsl_status->virt_head;
> > + if (qdma_ccdf_get_queue(status_addr) == pre.queue &&
> > + qdma_ccdf_addr_get64(status_addr) == pre.addr)
> > + duplicate = 1;
> > + i = qdma_ccdf_get_queue(status_addr);
> > + pre.queue = qdma_ccdf_get_queue(status_addr);
> > + pre.addr = qdma_ccdf_addr_get64(status_addr);
> > + temp_queue = fsl_queue + i;
> > + spin_lock(&temp_queue->queue_lock);
> > + if (list_empty(&temp_queue->comp_used)) {
> > + if (duplicate)
> > + duplicate_handle = 1;
>
> code style mandates braces for this as else has braces..
>

Thanks, it's good comments.

> > + else {
> > + spin_unlock(&temp_queue->queue_lock);
> > + return -EAGAIN;
> > + }
> > + } else {
> > + fsl_comp = list_first_entry(&temp_queue->comp_used,
> > + struct fsl_qdma_comp,
> > + list);
> > + csgf_src = fsl_comp->virt_addr + 2;
> > + if (fsl_comp->bus_addr + 16 != pre.addr) {
> > + if (duplicate)
> > + duplicate_handle = 1;
>
> here as well
>

sure

> > +static irqreturn_t fsl_qdma_error_handler(int irq, void *dev_id) {
> > + struct fsl_qdma_engine *fsl_qdma = dev_id;
> > + unsigned int intr;
> > + void __iomem *status = fsl_qdma->status_base;
> > +
> > + intr = qdma_readl(fsl_qdma, status + FSL_QDMA_DEDR);
> > +
> > + if (intr)
> > + dev_err(fsl_qdma->dma_dev.dev, "DMA transaction error!\n");
> > +
> > + qdma_writel(fsl_qdma, 0xffffffff, status + FSL_QDMA_DEDR);
>
> why unconditional write, was expecting that you would write if intr is non null
>
> > +static int fsl_qdma_reg_init(struct fsl_qdma_engine *fsl_qdma) {
> > + struct fsl_qdma_queue *fsl_queue = fsl_qdma->queue;
> > + struct fsl_qdma_queue *temp;
> > + void __iomem *ctrl = fsl_qdma->ctrl_base;
> > + void __iomem *status = fsl_qdma->status_base;
> > + void __iomem *block = fsl_qdma->block_base;
> > + int i, ret;
> > + u32 reg;
> > +
> > + /* Try to halt the qDMA engine first. */
> > + ret = fsl_qdma_halt(fsl_qdma);
> > + if (ret) {
> > + dev_err(fsl_qdma->dma_dev.dev, "DMA halt failed!");
> > + return ret;
> > + }
> > +
> > + /*
> > + * 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?

> > +
> > + for (i = 0; i < fsl_qdma->n_queues; i++) {
> > + temp = fsl_queue + i;
> > + /*
> > + * Initialize Command Queue registers to point to the first
> > + * command descriptor in memory.
> > + * Dequeue Pointer Address Registers
> > + * Enqueue Pointer Address Registers
> > + */
> > + qdma_writel(fsl_qdma, temp->bus_addr,
> > + block + FSL_QDMA_BCQDPA_SADDR(i));
> > + qdma_writel(fsl_qdma, temp->bus_addr,
> > + block + FSL_QDMA_BCQEPA_SADDR(i));
> > +
> > + /* Initialize the queue mode. */
> > + reg = FSL_QDMA_BCQMR_EN;
> > + reg |= FSL_QDMA_BCQMR_CD_THLD(ilog2(temp->n_cq)-4);
> > + reg |= FSL_QDMA_BCQMR_CQ_SIZE(ilog2(temp->n_cq)-6);
>
> space around - in the above two lines
>

Sure, thanks.

> > +static enum dma_status fsl_qdma_tx_status(struct dma_chan *chan,
> > + dma_cookie_t cookie, struct dma_tx_state *txstate) {
> > + enum dma_status ret;
> > +
> > + ret = dma_cookie_status(chan, cookie, txstate);
> > + if (ret == DMA_COMPLETE || !txstate)
> > + return ret;
> > +
> > + return ret;
>
> hmmm, this seems same as return dma_cookie_status() so why should we
> have rest of the code
>

Right, here code only fill the struct dma_device member's device_tx_status.

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

Best Regards,
Wen
> ...
>
> Overall the patch has some code style issues which keep catching my eye, can
> you please check them. Also would help to run checkpatch with --strict and
> --codespell option to catch typos and alignment issue. Please beware
> checkpatch is a guide and NOT a rulebook so use your discretion :)
>
> --
> ~Vinod
> --
> To unsubscribe from this list: send the line "unsubscribe dmaengine" in the
> body of a message to majordomo@vger.kernel.org More majordomo info at
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.
> kernel.org%2Fmajordomo-info.html&data=02%7C01%7Cwen.he_1%40nxp.co
> m%7C15adc08d93884118988108d5c532d7ef%7C686ea1d3bc2b4c6fa92cd99
> c5c301635%7C0%7C0%7C636631744524104561&sdata=0ucY71mrKgPEe8e3
> IhON91zaUeOzTgFVbzRnFPwKwQo%3D&reserved=0