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?
> > > +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...
> > > + 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
> > > +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?
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
