> -----Original Message----- > From: dmaengine-owner@xxxxxxxxxxxxxxx > [mailto:dmaengine-owner@xxxxxxxxxxxxxxx] On Behalf Of Vinod > Sent: 2018年5月18日 12:21 > 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: [v4 2/6] dmaengine: fsl-qdma: Add qDMA controller driver for > Layerscape SoCs > > On 17-05-18, 11:27, Wen He wrote: > > > > + > > > > +/* Registers for bit and genmask */ > > > > +#define FSL_QDMA_CQIDR_SQT 0x8000 > > > > > > BIT() ? > > > > Sorry, Maybe I should replace 0x8000 to BIT(15). > > yes please > > > > > +u64 pre_addr, pre_queue; > > > > > > why do we have a global? > > > > Let's us see qDMA that how is works? > > > > First, the status notification for DMA jobs are reported back to the status > queue. > > Status information is carried within the command descriptor > > status/command field, bits 120-127. The command descriptor dequeue > > pointer advances only after the transaction has completed and the status > information field has been updated. > > > > Then, the command descriptor address field wiil pointer to the command > > descriptor in its original format. It is the responsibity of the > > address of the status queue consumer to deallocate buffers as needed when > the command descriptor address pointer is non-zero. > > > > More details of the Status Queue can be found in QorIQ Layerscape Soc > datasheet. > > > > So, these variable is used to record latest value that command > > descriptor queue and status field. > > > > Every time variables value is zero when set these variable to local, that's not > what I want. > > Why not store them in driver context? > okay, maybe I should remove these variable to private struct? > > > > +static void fsl_qdma_comp_fill_memcpy(struct fsl_qdma_comp > *fsl_comp, > > > > + dma_addr_t dst, dma_addr_t src, u32 len) { > > > > + struct fsl_qdma_format *ccdf, *csgf_desc, *csgf_src, *csgf_dest; > > > > + struct fsl_qdma_sdf *sdf; > > > > + struct fsl_qdma_ddf *ddf; > > > > + > > > > + ccdf = (struct fsl_qdma_format *)fsl_comp->virt_addr; > > > > > > Cast are not required to/away from void > > > > > > > Does means: remove force conver? > > yes and it would work > > > > > +static struct fsl_qdma_comp *fsl_qdma_request_enqueue_desc( > > > > + struct fsl_qdma_chan *fsl_chan, > > > > + unsigned int dst_nents, > > > > + unsigned int src_nents) > > > > +{ > > > > + struct fsl_qdma_comp *comp_temp; > > > > + struct fsl_qdma_sg *sg_block; > > > > + struct fsl_qdma_queue *queue = fsl_chan->queue; > > > > + unsigned long flags; > > > > + unsigned int dst_sg_entry_block, src_sg_entry_block, > > > > +sg_entry_total, i; > > > > + > > > > + spin_lock_irqsave(&queue->queue_lock, flags); > > > > + if (list_empty(&queue->comp_free)) { > > > > + spin_unlock_irqrestore(&queue->queue_lock, flags); > > > > + comp_temp = kzalloc(sizeof(*comp_temp), GFP_KERNEL); > > > > + if (!comp_temp) > > > > + return NULL; > > > > + comp_temp->virt_addr = dma_pool_alloc(queue->comp_pool, > > > > + GFP_KERNEL, > > > > + &comp_temp->bus_addr); > > > > + if (!comp_temp->virt_addr) { > > > > + kfree(comp_temp); > > > > + return NULL; > > > > + } > > > > + > > > > + } else { > > > > + comp_temp = list_first_entry(&queue->comp_free, > > > > + struct fsl_qdma_comp, > > > > + list); > > > > + list_del(&comp_temp->list); > > > > + spin_unlock_irqrestore(&queue->queue_lock, flags); > > > > + } > > > > + > > > > + if (dst_nents != 0) > > > > + dst_sg_entry_block = dst_nents / > > > > + (FSL_QDMA_EXPECT_SG_ENTRY_NUM - 1) + 1; > > > > > > DIV_ROUND_UP()? > > > > > > > The DIV_ROUND_UP() definition see below: > > > > #define DIV_ROUND_UP __KERNEL_DIV_ROUND_UP #define > > __KERNEL_DIV_ROUND_UP(n ,d) (((n) + (d) - 1) / (d)) > > > > But here is 'd / (n - 1) + 1' ? > > Yeah this doesn't look apt here, check if any other macros suits... > Got it. > > > > + memset(sg_block->virt_addr, 0, > > > > + FSL_QDMA_EXPECT_SG_ENTRY_NUM * 16); > > > > > > why FSL_QDMA_EXPECT_SG_ENTRY_NUM * 16? and not what you > allocated? > > > > > > > see line 497. > > The sg_pool buffer size created is FSL_QDMA_EXPECT_SG_ENTRY_NUM * > 16. > > Please document this > Add comment to this? > > > > +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; > > > > + void __iomem *block = fsl_qdma->block_base; > > > > + u32 reg, i; > > > > + bool duplicate, duplicate_handle; > > > > + > > > > + 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; > > > > + else { > > > > + spin_unlock(&temp_queue->queue_lock); > > > > + return -1; > > > > > > -1? really. You are in while(1) wouldn't break make sense here? > > > > > > > Does means: using break? > > it means two things, we don't use return -1, if it is valid error then return a > proper kernel error code second, since it is a while loop, do you want to use a > break? > Got it, Thanks. > -- > ~Vinod > -- > To unsubscribe from this list: send the line "unsubscribe dmaengine" in the > body of a message to majordomo@xxxxxxxxxxxxxxx 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%7Cedcdbb818c534ec70b3108d5bc76d7b3%7C686ea1d3bc2b4c6fa92cd9 > 9c5c301635%7C0%7C0%7C636622140978593180&sdata=TuE40l7HejTz%2Bq > S8LibIkdYV84Esh5sfD1jSIzvAwTY%3D&reserved=0 ?韬{.n?????%??檩??w?{.n????z谵{???塄}?财??j:+v?? ? ???2??璀??摺?囤??z夸z罐?+?????w棹f
