Hi Vinod, > -----Original Message----- > From: Vinod Koul [mailto:vinod.koul@xxxxxxxxx] > Sent: 2017年12月19日 16:50 > To: Wen He <wen.he_1@xxxxxxx> > Cc: Leo Li <leoyang.li@xxxxxxx>; dmaengine@xxxxxxxxxxxxxxx; Jiafei Pan > <jiafei.pan@xxxxxxx>; Jiaheng Fan <jiaheng.fan@xxxxxxx> > Subject: Re: [PATCH 1/4] dma: fsl-qdma: add qDMA Command queue mode > driver > > On Tue, Dec 19, 2017 at 06:28:01AM +0000, Wen He wrote: > > > > > +#include <asm/cacheflush.h> > > > > +#include <linux/clk.h> > > > > +#include <linux/delay.h> > > > > +#include <linux/dma-mapping.h> > > > > +#include <linux/dmapool.h> > > > > +#include <linux/init.h> > > > > +#include <linux/interrupt.h> > > > > +#include <linux/module.h> > > > > +#include <linux/of.h> > > > > +#include <linux/of_address.h> > > > > +#include <linux/of_device.h> > > > > +#include <linux/of_dma.h> > > > > +#include <linux/of_irq.h> > > > > +#include <linux/slab.h> > > > > +#include <linux/spinlock.h> > > > > > > Do you need all these? > > > > > > > Don't need all these, Is that ok? > > Okay please remove the ones not needed > > > > BIT() or GENMASK() pls for register definations, here and other > > > places in the driver > > > > > > > Okay, does this means, I should be defined BIT() or GENMASK() to here? > > Yes please > > > > this looks terribly similar to fsl_qdma_ccdf, if you have union then > > > why not add this too to it? > > > > > > > This struct defined description two different dma message format that > > Compound Command Descriptor Format and qDMA Compound S/G Format, > if add this too to union, it could be ambiguous. > > at lower level do we care about the format, we fill and submit to HW > Okay, Is that ok? Will and below using fsl_qdma_desc_format instead fsl_qdma_ccfg and delete fsl_qdma_csgf in next version. Keep and fixed all of description call function, the desc pointer defined by the fsl_qdma_descs_format. struct fsl_qdma_desc_format *ccdf, *csgf; then fill and submit to HW. > > > > +static u32 qdma_readl(struct fsl_qdma_engine *qdma, void __iomem > > > > +*addr) { > > > > + if (qdma->big_endian) > > > > + return ioread32be(addr); > > > > + else > > > > + return ioread32(addr); > > > > +} > > > > > > IIRC there are macros which which take care of this, can you check > > > other FSL drivers please > > > > > > > Okay, but I search the FSL drivers and no found. Do you know where are > them ? > > See DMA_IN/OUT in dma/fsldma.h > Okay, but DMA_IN/OUT need struct fsldma_chan. May be I can refer to it that Implement a similar macro, will and below to instead it in next version. #define QDMA_IN(fsl_qdma_engine, addr) \ (((fsl_qdma_engine)->big_endian & FSL_DMA_BIG_ENDIAN) ? \ ioread32be(addr) : ioread32(addr)) #define QDMA_OUT(fsl_qdma_engine, addr, val) \ (((fsl_qdma_engine)->big_endian & FSL_DMA_BIG_ENDIAN) ? \ iowrite32be(val, addr) : iowrite32(val, addr)) > > > > + sg_entry_total = dst_sg_entry_block + src_sg_entry_block; > > > > + if (sg_entry_total) { > > > > + sg_block = kzalloc(sizeof(*sg_block) * > > > > + sg_entry_total, > > > > + GFP_KERNEL); > > > > + if (!sg_block) > > > > + return NULL; > > > > > > okay you are leaking here the pool allocation need to be rolled back > > > > > > > okay, I got it, Is that ok? > > > > if (!sg_block) { > > dma_pool_free(queue->comp_pool, > > comp_temp->virt_addr, > > comp_temp->bus_addr); > > return NULL; > > Looks okay > > > > > + > > > > + dma_free_coherent(&pdev->dev, sizeof(struct fsl_qdma_ccdf) * > > > > + status->n_cq, status->cq, status->bus_addr); > > > > > > at this point you can still get irq and tasklets can be still > > > scheduled, how do you ensure they are stopped > > > > > > > May be I missed, will and below to funtions call In fsl_qdma_remove() in > next version. > > Is that ok? > > sure > -- > ~Vinod -- ~Best Regards, ~Wen ?韬{.n?????%??檩??w?{.n???Ё?撖)?骅w* jg????????G??⒏ ⒎?:+v???????? ????＂??????
