diff for duplicates of <1493049305.25985.4.camel@synopsys.com> diff --git a/a/1.txt b/N1/1.txt index ca3dd4c..0fbaa5b 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,10 +1,10 @@ Hi, -On Fri, 2017-04-21@18:13 +0300, Andy Shevchenko wrote: -> On Fri, 2017-04-21@14:29 +0000, Eugeniy Paltsev wrote: -> > On Tue, 2017-04-18@15:31 +0300, Andy Shevchenko wrote: -> > > On Fri, 2017-04-07@17:04 +0300, Eugeniy Paltsev wrote: +On Fri, 2017-04-21 at 18:13 +0300, Andy Shevchenko wrote: +> On Fri, 2017-04-21 at 14:29 +0000, Eugeniy Paltsev wrote: +> > On Tue, 2017-04-18 at 15:31 +0300, Andy Shevchenko wrote: +> > > On Fri, 2017-04-07 at 17:04 +0300, Eugeniy Paltsev wrote: > > > > This patch adds support for the DW AXI DMAC controller. -> > > > +#define AXI_DMA_BUSWIDTHS ??\ +> > > > +#define AXI_DMA_BUSWIDTHS \ > > > > + (DMA_SLAVE_BUSWIDTH_1_BYTE | \ > > > > + DMA_SLAVE_BUSWIDTH_2_BYTES | \ > > > > + DMA_SLAVE_BUSWIDTH_4_BYTES | \ @@ -24,5 +24,349 @@ On Fri, 2017-04-21@18:13 +0300, Andy Shevchenko wrote: > > Strange. AFAIK they are representing bits (which is not the best > idea) in the resulting u64 field. So, anything bigger than 63 doesn't ->?make sense. +> make sense. They are u32 fields! +From dmaengine.h : +struct dma_device { +... + u32 src_addr_widths; + u32 dst_addr_widths; +}; + +> In drivers where they are not bits quite likely a bug is hidden. +For example (from pxa_dma.c): +const enum dma_slave_buswidth widths = DMA_SLAVE_BUSWIDTH_1_BYTE | +DMA_SLAVE_BUSWIDTH_2_BYTES | DMA_SLAVE_BUSWIDTH_4_BYTES; + +And there are a lot of drivers, which don't use BIT for this fields. +sh/shdmac.c +sh/rcar-dmac.c +qcom/bam_dma.c +mmp_pdma.c +ste_dma40.c +And many others... + +> > +> > > > + +> > > > +static inline void +> > > > +axi_dma_iowrite32(struct axi_dma_chip *chip, u32 reg, u32 val) +> > > > +{ +> > > > + iowrite32(val, chip->regs + reg); +> > > +> > > Are you going to use IO ports for this IP? I don't think so. +> > > Wouldn't be better to call readl()/writel() instead? +> > +> > As I understand, it's better to use ioread/iowrite as more +> > universal +> > IO +> > access way. Am I wrong? +> +> As I said above the ioreadX/iowriteX makes only sense when your IP +> would be accessed via IO region or MMIO. I'm pretty sure IO is not +> the case at all for this IP. +MMIO? This IP works exactly via memory-mapped I/O. + +> > > > +static inline void axi_chan_irq_disable(struct axi_dma_chan +> > > > *chan, +> > > > u32 irq_mask) +> > > > +{ +> > > > + u32 val; +> > > > + +> > > > + if (likely(irq_mask == DWAXIDMAC_IRQ_ALL)) { +> > > > + axi_chan_iowrite32(chan, CH_INTSTATUS_ENA, +> > > > DWAXIDMAC_IRQ_NONE); +> > > > + } else { +> > > +> > > I don't see the benefit. (Yes, I see one read-less path, I think +> > > it +> > > makes perplexity for nothing here) +> > +> > This function is called mostly with irq_mask = DWAXIDMAC_IRQ_ALL. +> > Actually it is called only with irq_mask = DWAXIDMAC_IRQ_ALL until +> > I add DMA_SLAVE support. +> > But I can cut off this 'if' statment, if it is necessery. +> +> It's not necessary, but from my point of view increases readability +> of the code a lot for the price of an additional readl(). +Ok. + +> > +> > > > + val = axi_chan_ioread32(chan, +> > > > CH_INTSTATUS_ENA); +> > > > + val &= ~irq_mask; +> > > > + axi_chan_iowrite32(chan, CH_INTSTATUS_ENA, +> > > > val); +> > > > + } +> > > > + +> > > > + return min_t(size_t, __ffs(sdl), max_width); +> > > > +} +> > > > +static void axi_desc_put(struct axi_dma_desc *desc) +> > > > +{ +> > > > + struct axi_dma_chan *chan = desc->chan; +> > > > + struct dw_axi_dma *dw = chan->chip->dw; +> > > > + struct axi_dma_desc *child, *_next; +> > > > + unsigned int descs_put = 0; +> > > > + list_for_each_entry_safe(child, _next, &desc- +> > > > >xfer_list, +> > > > xfer_list) { +> > > +> > > xfer_list looks redundant. +> > > Can you elaborate why virtual channel management is not working +> > > for +> > > you? +> > +> > Each virtual descriptor encapsulates several hardware descriptors, +> > which belong to same transfer. +> > This list (xfer_list) is used only for allocating/freeing these +> > descriptors and it doesn't affect on virtual dma work logic. +> > I can see this approach in several drivers with VirtDMA (but they +> > mostly use array instead of list) +> +> You described how most of the DMA drivers are implemented, though +> they +> are using just sg_list directly. I would recommend to do the same and +> get rid of this list. +This IP can be (ans is) configured with small block size. +(note, that I am not saying about runtime HW configuration) + +And there is opportunity what we can't use sg_list directly and need to +split sg_list to a smaller chunks. + +> > > Btw, are you planning to use priority at all? For now on I didn't +> > > see +> > > a single driver (from the set I have checked, like 4-5 of them) +> > > that +> > > uses priority anyhow. It makes driver more complex for nothing. +> > +> > Only for dma slave operations. +> +> So, in other words you *have* an actual two or more users that *need* +> prioritization? +As I remember there was an idea to give higher priority to audio dma +chanels. + +> > > > + if (unlikely(dst_nents == 0 || src_nents == 0)) +> > > > + return NULL; +> > > > + +> > > > + if (unlikely(dst_sg == NULL || src_sg == NULL)) +> > > > + return NULL; +> > > +> > > If we need those checks they should go to +> > > dmaengine.h/dmaengine.c. +> > +> > I checked several drivers, which implements device_prep_dma_sg and +> > they +> > implements this checkers. +> > Should I add something like "dma_sg_desc_invalid" function to +> > dmaengine.h ? +> +> I suppose it's better to implement them in the corresponding helpers +> in dmaengine.h. +Ok. + +> > > > +static void axi_chan_list_dump_lli(struct axi_dma_chan *chan, +> > > > + struct axi_dma_desc +> > > > *desc_head) +> > > > +{ +> > > > + struct axi_dma_desc *desc; +> > > > + +> > > > + axi_chan_dump_lli(chan, desc_head); +> > > > + list_for_each_entry(desc, &desc_head->xfer_list, +> > > > xfer_list) +> > > > + axi_chan_dump_lli(chan, desc); +> > > > +} +> > > > + +> > > > +static void axi_chan_handle_err(struct axi_dma_chan *chan, u32 +> > > > status) +> > > > +{ +> > > > + /* WARN about bad descriptor */ +> > > > +> > > > + dev_err(chan2dev(chan), +> > > > + "Bad descriptor submitted for %s, cookie: %d, +> > > > irq: +> > > > 0x%08x\n", +> > > > + axi_chan_name(chan), vd->tx.cookie, status); +> > > > + axi_chan_list_dump_lli(chan, vd_to_axi_desc(vd)); +> > > +> > > As I said earlier dw_dmac is *bad* example of the (virtual +> > > channel +> > > based) DMA driver. +> > > +> > > I guess you may just fail the descriptor and don't pretend it has +> > > been processed successfully. +> > +> > What do you mean by saying "fail the descriptor"? +> > After I get error I cancel current transfer and free all +> > descriptors +> > from it (by calling vchan_cookie_complete). +> > I can't store error status in descriptor structure because it will +> > be +> > freed by vchan_cookie_complete. +> > I can't store error status in channel structure because it will be +> > overwritten by next transfer. +> +> Better not to pretend that it has been processed successfully. Don't +> call callback on it and set its status to DMA_ERROR (that's why +> descriptors in many drivers have dma_status field). When user asks +> for +> status (using cookie) the saved value would be returned until +> descriptor +> is active. +> +> Do you have some other workflow in mind? + +Hmm... +Do you mean I should left error descriptors in desc_issued list +or I should create another list (like desc_error) in my driver and move +error descriptors to desc_error list? + +And when exactly should I free error descriptors? + +I checked hsu/hsu.c dma driver implementation: + vdma descriptor is deleted from desc_issued list when transfer + starts. When descriptor marked as error descriptor + vchan_cookie_complete isn't called for this descriptor. And this + descriptor isn't placed in any list. So error descriptors *never* + will be freed. + I don't actually like this approach. + +> > > > + +> > > > +static const struct dev_pm_ops dw_axi_dma_pm_ops = { +> > > > + SET_RUNTIME_PM_OPS(axi_dma_runtime_suspend, +> > > > axi_dma_runtime_resume, NULL) +> > > > +}; +> > > +> > > Have you tried to build with CONFIG_PM disabled? +> > +> > Yes. +> > +> > > I'm pretty sure you need __maybe_unused applied to your PM ops. +> > +> > I call axi_dma_runtime_suspend / axi_dma_runtime_resume even I +> > dont't +> > use PM. +> > (I call them in probe / remove function.) +> +> Hmm... I didn't check your ->probe() and ->remove(). Do you mean you +> call them explicitly by those names? +> +> If so, please don't do that. Use pm_runtime_*() instead. And... +> +> > So I don't need to declare them with __maybe_unused. +> +> ...in that case it's possible you have them defined but not used. +> +From my ->probe() function: + +pm_runtime_get_noresume(chip->dev); +ret = axi_dma_runtime_resume(chip->dev); + +Firstly I only incrememt counter. +Secondly explicitly call my resume function. + +I call them explicitly because I need driver to work also without +Runtime PM. So I can't just call pm_runtime_get here instead of +pm_runtime_get_noresume + axi_dma_runtime_resume. + +Of course I can copy *all* code from axi_dma_runtime_resume +to ->probe() function, but I don't really like this idea. + +> > > > +struct axi_dma_chan { +> > > > + struct axi_dma_chip *chip; +> > > > + void __iomem *chan_regs; +> > > > + u8 id; +> > > > + atomic_t descs_allocated; +> > > > + +> > > > + struct virt_dma_chan vc; +> > > > + +> > > > + /* these other elements are all protected by vc.lock +> > > > */ +> > > > + bool is_paused; +> > > +> > > I still didn't get (already forgot) why you can't use dma_status +> > > instead for the active descriptor? +> > +> > As I said before, I checked several driver, which have status +> > variable +> > in their channel structure - it is used *only* for determinating is +> > channel paused or not. So there is no much sense in replacing +> > "is_paused" to "status" and I left "is_paused" variable untouched. +> +> Not only (see above), the errored descriptor keeps that status. +> +> > (I described above why we can't use status in channel structure for +> > error handling) +> +> Ah, I'm talking about descriptor. + +Again - PAUSED is per-channel flag. So, even if we have status field in +each descriptor, it is simpler to use one per-channel flag instead of +plenty per-descriptor flags. +When we pausing/resuming dma channel it is simpler to set only one flag +instead of writing DMA_PAUSED to *each* descriptor status field. + +> > > > Status Fetch Addr */ +> > > > +#define CH_INTSTATUS_ENA 0x080 /* R/W Chan Interrupt +> > > > Status +> > > > Enable */ +> > > > +#define CH_INTSTATUS 0x088 /* R/W Chan +> > > > Interrupt +> > > > Status */ +> > > > +#define CH_INTSIGNAL_ENA 0x090 /* R/W Chan Interrupt +> > > > Signal +> > > > Enable */ +> > > > +#define CH_INTCLEAR 0x098 /* W Chan Interrupt +> > > > Clear +> > > > */ +> > > +> > > I'm wondering if you can use regmap API instead. +> > +> > Is it really necessary? It'll make driver more complex for nothing. +> +> That's why I'm not suggesting but wondering. +> > +> > > > +}; +> > > +> > > Hmm... do you need them in the header? +> > +> > I use some of these definitions in my code so I guess yes. +> > /* Maybe I misunderstood your question... */ +> +> I mean, who are the users of them? If it's only one module, there is +> no need to put them in header. +Yes, only one module. +Should I move all this definitions to axi_dma_platform.c file and rid +of both axi_dma_platform_reg.h and axi_dma_platform.h headers? + +> > +> > > > +#define CH_CFG_H_TT_FC_POS 0 +> > > > +enum { +> > > > + DWAXIDMAC_TT_FC_MEM_TO_MEM_DMAC = 0x0, +> > > > + DWAXIDMAC_TT_FC_MEM_TO_PER_DMAC, +> > > > + DWAXIDMAC_TT_FC_PER_TO_MEM_DMAC, +> > > > + DWAXIDMAC_TT_FC_PER_TO_PER_DMAC, +> > > > + DWAXIDMAC_TT_FC_PER_TO_MEM_SRC, +> > > > + DWAXIDMAC_TT_FC_PER_TO_PER_SRC, +> > > > + DWAXIDMAC_TT_FC_MEM_TO_PER_DST, +> > > > + DWAXIDMAC_TT_FC_PER_TO_PER_DST +> > > > +}; +> > > +> > > Some of definitions are the same as for dw_dmac, right? We might +> > > split them to a common header, though I have no strong opinion +> > > about +> > +> > it. +> > > Vinod? +> > +> > APB DMAC and AXI DMAC have completely different regmap. So there is +> > no +> > much sense to do that. +> +> I'm not talking about registers, I'm talking about definitions like +> above. Though it would be indeed little sense to split some common +> code between those two. +This definitions are the fields of some registers. So they are also +different. + +-- + Eugeniy Paltsev diff --git a/a/content_digest b/N1/content_digest index 7fe943d..7afcd12 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -3,19 +3,29 @@ "ref\01492518695.24567.56.camel@linux.intel.com\0" "ref\01492784977.16657.6.camel@synopsys.com\0" "ref\01492787583.24567.120.camel@linux.intel.com\0" - "From\0Eugeniy.Paltsev@synopsys.com (Eugeniy Paltsev)\0" - "Subject\0[PATCH v2 2/2] dmaengine: Add DW AXI DMAC driver\0" + "ref\01492787583.24567.120.camel-VuQAYsv1563Yd54FQh9/CA@public.gmane.org\0" + "From\0Eugeniy Paltsev <Eugeniy.Paltsev-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>\0" + "Subject\0Re: [PATCH v2 2/2] dmaengine: Add DW AXI DMAC driver\0" "Date\0Mon, 24 Apr 2017 15:55:06 +0000\0" - "To\0linux-snps-arc@lists.infradead.org\0" + "To\0andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>\0" + "Cc\0vinod.koul-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org <vinod.koul-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>" + linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> + robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> + Alexey.Brodkin-HKixBCOQz3hWk0Htik3J/w@public.gmane.org <Alexey.Brodkin-HKixBCOQz3hWk0Htik3J/w@public.gmane.org> + devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org <devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> + Eugeniy.Paltsev-HKixBCOQz3hWk0Htik3J/w@public.gmane.org <Eugeniy.Paltsev-HKixBCOQz3hWk0Htik3J/w@public.gmane.org> + linux-snps-arc-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org <linux-snps-arc-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org> + dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> + " dmaengine-u79uwXL29TY76Z2rM5mHXA@public.gmane.org <dmaengine-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>\0" "\00:1\0" "b\0" "Hi,\n" - "On Fri, 2017-04-21@18:13 +0300, Andy Shevchenko wrote:\n" - "> On Fri, 2017-04-21@14:29 +0000, Eugeniy Paltsev wrote:\n" - "> > On Tue, 2017-04-18@15:31 +0300, Andy Shevchenko wrote:\n" - "> > > On Fri, 2017-04-07@17:04 +0300, Eugeniy Paltsev wrote:\n" + "On Fri, 2017-04-21 at 18:13 +0300, Andy Shevchenko wrote:\n" + "> On Fri, 2017-04-21 at 14:29 +0000, Eugeniy Paltsev wrote:\n" + "> > On Tue, 2017-04-18 at 15:31 +0300, Andy Shevchenko wrote:\n" + "> > > On Fri, 2017-04-07 at 17:04 +0300, Eugeniy Paltsev wrote:\n" "> > > > This patch adds support for the DW AXI DMAC controller.\n" - "> > > > +#define AXI_DMA_BUSWIDTHS\t\t??\\\n" + "> > > > +#define AXI_DMA_BUSWIDTHS\t\t\302\240\302\240\\\n" "> > > > +\t(DMA_SLAVE_BUSWIDTH_1_BYTE\t| \\\n" "> > > > +\tDMA_SLAVE_BUSWIDTH_2_BYTES\t| \\\n" "> > > > +\tDMA_SLAVE_BUSWIDTH_4_BYTES\t| \\\n" @@ -35,7 +45,351 @@ ">\n" "> Strange. AFAIK they are representing bits (which is not the best\n" "> idea) in the resulting u64 field. So, anything bigger than 63 doesn't\n" - ">?make sense.\n" - They are u32 fields! + ">\302\240make sense.\n" + "They are u32 fields!\n" + "From dmaengine.h :\n" + "struct dma_device {\n" + "...\n" + "\302\240\302\240\302\240\302\240u32 src_addr_widths;\n" + "\302\240\302\240\302\240\302\240u32 dst_addr_widths;\n" + "};\n" + "\n" + "> In drivers where they are not bits quite likely a bug is hidden.\n" + "For example (from pxa_dma.c):\n" + "const enum dma_slave_buswidth widths = DMA_SLAVE_BUSWIDTH_1_BYTE |\n" + "DMA_SLAVE_BUSWIDTH_2_BYTES | DMA_SLAVE_BUSWIDTH_4_BYTES;\n" + "\n" + "And there are a lot of drivers, which don't use BIT for this fields.\n" + "sh/shdmac.c\n" + "sh/rcar-dmac.c\n" + "qcom/bam_dma.c\n" + "mmp_pdma.c\n" + "ste_dma40.c\n" + "And many others...\n" + "\n" + "> >\n" + "> > > > +\n" + "> > > > +static inline void\n" + "> > > > +axi_dma_iowrite32(struct axi_dma_chip *chip, u32 reg, u32 val)\n" + "> > > > +{\n" + "> > > > +\tiowrite32(val, chip->regs + reg);\n" + "> > >\n" + "> > > Are you going to use IO ports for this IP? I don't think so.\n" + "> > > Wouldn't be better to call readl()/writel() instead?\n" + "> >\n" + "> > As I understand, it's better to use ioread/iowrite as more\n" + "> > universal\n" + "> > IO\n" + "> > access way. Am I wrong?\n" + ">\n" + "> As I said above the ioreadX/iowriteX makes only sense when your IP\n" + "> would be accessed via IO region or MMIO. I'm pretty sure IO is not\n" + "> the case at all for this IP.\n" + "MMIO? This IP works exactly via memory-mapped I/O.\n" + "\n" + "> > > > +static inline void axi_chan_irq_disable(struct axi_dma_chan\n" + "> > > > *chan,\n" + "> > > > u32 irq_mask)\n" + "> > > > +{\n" + "> > > > +\tu32 val;\n" + "> > > > +\n" + "> > > > +\tif (likely(irq_mask == DWAXIDMAC_IRQ_ALL)) {\n" + "> > > > +\t\taxi_chan_iowrite32(chan, CH_INTSTATUS_ENA,\n" + "> > > > DWAXIDMAC_IRQ_NONE);\n" + "> > > > +\t} else {\n" + "> > >\n" + "> > > I don't see the benefit. (Yes, I see one read-less path, I think\n" + "> > > it\n" + "> > > makes perplexity for nothing here)\n" + "> >\n" + "> > This function is called mostly with irq_mask = DWAXIDMAC_IRQ_ALL.\n" + "> > Actually it is called only with irq_mask = DWAXIDMAC_IRQ_ALL until\n" + "> > I add DMA_SLAVE support.\n" + "> > But I can cut off this 'if' statment, if it is necessery.\n" + ">\n" + "> It's not necessary, but from my point of view increases readability\n" + "> of the code a lot for the price of an additional readl().\n" + "Ok.\n" + "\n" + "> >\n" + "> > > > +\t\tval = axi_chan_ioread32(chan,\n" + "> > > > CH_INTSTATUS_ENA);\n" + "> > > > +\t\tval &= ~irq_mask;\n" + "> > > > +\t\taxi_chan_iowrite32(chan, CH_INTSTATUS_ENA,\n" + "> > > > val);\n" + "> > > > +\t}\n" + "> > > > +\n" + "> > > > +\treturn min_t(size_t, __ffs(sdl), max_width);\n" + "> > > > +}\n" + "> > > > +static void axi_desc_put(struct axi_dma_desc *desc)\n" + "> > > > +{\n" + "> > > > +\tstruct axi_dma_chan *chan = desc->chan;\n" + "> > > > +\tstruct dw_axi_dma *dw = chan->chip->dw;\n" + "> > > > +\tstruct axi_dma_desc *child, *_next;\n" + "> > > > +\tunsigned int descs_put = 0;\n" + "> > > > +\tlist_for_each_entry_safe(child, _next, &desc-\n" + "> > > > >xfer_list,\n" + "> > > > xfer_list) {\n" + "> > >\n" + "> > > xfer_list looks redundant.\n" + "> > > Can you elaborate why virtual channel management is not working\n" + "> > > for\n" + "> > > you?\n" + "> >\n" + "> > Each virtual descriptor encapsulates several hardware descriptors,\n" + "> > which belong to same transfer.\n" + "> > This list (xfer_list) is used only for allocating/freeing these\n" + "> > descriptors and it doesn't affect on virtual dma work logic.\n" + "> > I can see this approach in several drivers with VirtDMA (but they\n" + "> > mostly use array instead of list)\n" + ">\n" + "> You described how most of the DMA drivers are implemented, though\n" + "> they\n" + "> are using just sg_list directly. I would recommend to do the same and\n" + "> get rid of this list.\n" + "This IP can be (ans is) configured with small block size.\n" + "(note, that I am not saying about runtime HW configuration)\n" + "\n" + "And there is opportunity what we can't use sg_list directly and need to\n" + "split sg_list to a smaller chunks.\n" + "\n" + "> > > Btw, are you planning to use priority at all? For now on I didn't\n" + "> > > see\n" + "> > > a single driver (from the set I have checked, like 4-5 of them)\n" + "> > > that\n" + "> > > uses priority anyhow. It makes driver more complex for nothing.\n" + "> >\n" + "> > Only for dma slave operations.\n" + ">\n" + "> So, in other words you *have* an actual two or more users that *need*\n" + "> prioritization?\n" + "As I remember there was an idea to give higher priority to audio dma\n" + "chanels.\n" + "\n" + "> > > > +\tif (unlikely(dst_nents == 0 || src_nents == 0))\n" + "> > > > +\t\treturn NULL;\n" + "> > > > +\n" + "> > > > +\tif (unlikely(dst_sg == NULL || src_sg == NULL))\n" + "> > > > +\t\treturn NULL;\n" + "> > >\n" + "> > > If we need those checks they should go to\n" + "> > > dmaengine.h/dmaengine.c.\n" + "> >\n" + "> > I checked several drivers, which implements device_prep_dma_sg and\n" + "> > they\n" + "> > implements this checkers.\n" + "> > Should I add something like \"dma_sg_desc_invalid\" function to\n" + "> > dmaengine.h ?\n" + ">\n" + "> I suppose it's better to implement them in the corresponding helpers\n" + "> in dmaengine.h.\n" + "Ok.\n" + "\n" + "> > > > +static void axi_chan_list_dump_lli(struct axi_dma_chan *chan,\n" + "> > > > +\t\t\t\t\302\240\302\240\302\240struct axi_dma_desc\n" + "> > > > *desc_head)\n" + "> > > > +{\n" + "> > > > +\tstruct axi_dma_desc *desc;\n" + "> > > > +\n" + "> > > > +\taxi_chan_dump_lli(chan, desc_head);\n" + "> > > > +\tlist_for_each_entry(desc, &desc_head->xfer_list,\n" + "> > > > xfer_list)\n" + "> > > > +\t\taxi_chan_dump_lli(chan, desc);\n" + "> > > > +}\n" + "> > > > +\n" + "> > > > +static void axi_chan_handle_err(struct axi_dma_chan *chan, u32\n" + "> > > > status)\n" + "> > > > +{\n" + "> > > > +\t/* WARN about bad descriptor */\n" + "> > > >\n" + "> > > > +\tdev_err(chan2dev(chan),\n" + "> > > > +\t\t\"Bad descriptor submitted for %s, cookie: %d,\n" + "> > > > irq:\n" + "> > > > 0x%08x\\n\",\n" + "> > > > +\t\taxi_chan_name(chan), vd->tx.cookie, status);\n" + "> > > > +\taxi_chan_list_dump_lli(chan, vd_to_axi_desc(vd));\n" + "> > >\n" + "> > > As I said earlier dw_dmac is *bad* example of the (virtual\n" + "> > > channel\n" + "> > > based) DMA driver.\n" + "> > >\n" + "> > > I guess you may just fail the descriptor and don't pretend it has\n" + "> > > been processed successfully.\n" + "> >\n" + "> > What do you mean by saying \"fail the descriptor\"?\n" + "> > After I get error I cancel current transfer and free all\n" + "> > descriptors\n" + "> > from it (by calling vchan_cookie_complete).\n" + "> > I can't store error status in descriptor structure because it will\n" + "> > be\n" + "> > freed by vchan_cookie_complete.\n" + "> > I can't store error status in channel structure because it will be\n" + "> > overwritten by next transfer.\n" + ">\n" + "> Better not to pretend that it has been processed successfully. Don't\n" + "> call callback on it and set its status to DMA_ERROR (that's why\n" + "> descriptors in many drivers have dma_status field). When user asks\n" + "> for\n" + "> status (using cookie) the saved value would be returned until\n" + "> descriptor\n" + "> is active.\n" + ">\n" + "> Do you have some other workflow in mind?\n" + "\n" + "Hmm...\n" + "Do you mean I should left error descriptors in desc_issued list\n" + "or I should create another list (like desc_error) in my driver and move\n" + "error descriptors to desc_error list?\n" + "\n" + "And when exactly should I free error descriptors?\n" + "\n" + "I checked hsu/hsu.c dma driver implementation:\n" + "\302\240 vdma descriptor is deleted from desc_issued list when transfer\n" + "\302\240 starts. When descriptor marked as error descriptor\n" + "\302\240 vchan_cookie_complete isn't called for this descriptor. And this\n" + "\302\240 descriptor isn't placed in any list. So error descriptors *never*\n" + "\302\240 will be freed.\n" + "\302\240 I don't actually like this approach.\n" + "\n" + "> > > > +\n" + "> > > > +static const struct dev_pm_ops dw_axi_dma_pm_ops = {\n" + "> > > > +\tSET_RUNTIME_PM_OPS(axi_dma_runtime_suspend,\n" + "> > > > axi_dma_runtime_resume, NULL)\n" + "> > > > +};\n" + "> > >\n" + "> > > Have you tried to build with CONFIG_PM disabled?\n" + "> >\n" + "> > Yes.\n" + "> >\n" + "> > > I'm pretty sure you need __maybe_unused applied to your PM ops.\n" + "> >\n" + "> > I call axi_dma_runtime_suspend / axi_dma_runtime_resume even I\n" + "> > dont't\n" + "> > use PM.\n" + "> > (I call them in probe / remove function.)\n" + ">\n" + "> Hmm... I didn't check your ->probe() and ->remove(). Do you mean you\n" + "> call them explicitly by those names?\n" + ">\n" + "> If so, please don't do that. Use pm_runtime_*() instead. And...\n" + ">\n" + "> > So I don't need to declare them with __maybe_unused.\n" + ">\n" + "> ...in that case it's possible you have them defined but not used.\n" + ">\n" + "From my ->probe() function:\n" + "\n" + "pm_runtime_get_noresume(chip->dev);\n" + "ret = axi_dma_runtime_resume(chip->dev);\n" + "\n" + "Firstly I only incrememt counter.\n" + "Secondly explicitly call my resume function.\n" + "\n" + "I call them explicitly because I need driver to work also without\n" + "Runtime PM. So I can't just call pm_runtime_get here instead of\n" + "pm_runtime_get_noresume + axi_dma_runtime_resume.\n" + "\n" + "Of course I can copy *all* code from axi_dma_runtime_resume\n" + "to ->probe() function, but I don't really like this idea.\n" + "\n" + "> > > > +struct axi_dma_chan {\n" + "> > > > +\tstruct axi_dma_chip\t\t*chip;\n" + "> > > > +\tvoid __iomem\t\t\t*chan_regs;\n" + "> > > > +\tu8\t\t\t\tid;\n" + "> > > > +\tatomic_t\t\t\tdescs_allocated;\n" + "> > > > +\n" + "> > > > +\tstruct virt_dma_chan\t\tvc;\n" + "> > > > +\n" + "> > > > +\t/* these other elements are all protected by vc.lock\n" + "> > > > */\n" + "> > > > +\tbool\t\t\t\tis_paused;\n" + "> > >\n" + "> > > I still didn't get (already forgot) why you can't use dma_status\n" + "> > > instead for the active descriptor?\n" + "> >\n" + "> > As I said before, I checked several driver, which have status\n" + "> > variable\n" + "> > in their channel structure - it is used *only* for determinating is\n" + "> > channel paused or not. So there is no much sense in replacing\n" + "> > \"is_paused\" to \"status\" and I left \"is_paused\" variable untouched.\n" + ">\n" + "> Not only (see above), the errored descriptor keeps that status.\n" + ">\n" + "> > (I described above why we can't use status in channel structure for\n" + "> > error handling)\n" + ">\n" + "> Ah, I'm talking about descriptor.\n" + "\n" + "Again - PAUSED is per-channel flag. So, even if we have status field in\n" + "each descriptor, it is simpler to use one per-channel flag instead of\n" + "plenty per-descriptor flags.\n" + "When we pausing/resuming dma channel it is simpler to set only one flag\n" + "instead of writing DMA_PAUSED to *each* descriptor status field.\n" + "\n" + "> > > > Status Fetch Addr */\n" + "> > > > +#define CH_INTSTATUS_ENA\t0x080 /* R/W Chan Interrupt\n" + "> > > > Status\n" + "> > > > Enable */\n" + "> > > > +#define CH_INTSTATUS\t\t0x088 /* R/W Chan\n" + "> > > > Interrupt\n" + "> > > > Status */\n" + "> > > > +#define CH_INTSIGNAL_ENA\t0x090 /* R/W Chan Interrupt\n" + "> > > > Signal\n" + "> > > > Enable */\n" + "> > > > +#define CH_INTCLEAR\t\t0x098 /* W Chan Interrupt\n" + "> > > > Clear\n" + "> > > > */\n" + "> > >\n" + "> > > I'm wondering if you can use regmap API instead.\n" + "> >\n" + "> > Is it really necessary? It'll make driver more complex for nothing.\n" + ">\n" + "> That's why I'm not suggesting but wondering.\n" + "> >\n" + "> > > > +};\n" + "> > >\n" + "> > > Hmm... do you need them in the header?\n" + "> >\n" + "> > I use some of these definitions in my code so I guess yes.\n" + "> > /* Maybe I misunderstood your question... */\n" + ">\n" + "> I mean, who are the users of them? If it's only one module, there is\n" + "> no need to put them in header.\n" + "Yes, only one module.\n" + "Should I move all this definitions to axi_dma_platform.c file and rid\n" + "of both axi_dma_platform_reg.h and axi_dma_platform.h headers?\n" + "\n" + "> >\n" + "> > > > +#define CH_CFG_H_TT_FC_POS\t0\n" + "> > > > +enum {\n" + "> > > > +\tDWAXIDMAC_TT_FC_MEM_TO_MEM_DMAC\t= 0x0,\n" + "> > > > +\tDWAXIDMAC_TT_FC_MEM_TO_PER_DMAC,\n" + "> > > > +\tDWAXIDMAC_TT_FC_PER_TO_MEM_DMAC,\n" + "> > > > +\tDWAXIDMAC_TT_FC_PER_TO_PER_DMAC,\n" + "> > > > +\tDWAXIDMAC_TT_FC_PER_TO_MEM_SRC,\n" + "> > > > +\tDWAXIDMAC_TT_FC_PER_TO_PER_SRC,\n" + "> > > > +\tDWAXIDMAC_TT_FC_MEM_TO_PER_DST,\n" + "> > > > +\tDWAXIDMAC_TT_FC_PER_TO_PER_DST\n" + "> > > > +};\n" + "> > >\n" + "> > > Some of definitions are the same as for dw_dmac, right? We might\n" + "> > > split them to a common header, though I have no strong opinion\n" + "> > > about\n" + "> >\n" + "> > it.\n" + "> > > Vinod?\n" + "> >\n" + "> > APB DMAC and AXI DMAC have completely different regmap. So there is\n" + "> > no\n" + "> > much sense to do that.\n" + ">\n" + "> I'm not talking about registers, I'm talking about definitions like\n" + "> above. Though it would be indeed little sense to split some common\n" + "> code between those two.\n" + "This definitions are the fields of some registers. So they are also\n" + "different.\n" + "\n" + "--\n" + "\302\240Eugeniy Paltsev" -644b21629fbf035165f06e21bb6d27283769d773bc5c8f1a2af422e82f768a39 +b4929cc28eb8b7db8781020a26337baf1affc253be262c32b0a28caeda51b03d
diff --git a/a/1.txt b/N2/1.txt index ca3dd4c..92b245d 100644 --- a/a/1.txt +++ b/N2/1.txt @@ -1,10 +1,10 @@ Hi, -On Fri, 2017-04-21@18:13 +0300, Andy Shevchenko wrote: -> On Fri, 2017-04-21@14:29 +0000, Eugeniy Paltsev wrote: -> > On Tue, 2017-04-18@15:31 +0300, Andy Shevchenko wrote: -> > > On Fri, 2017-04-07@17:04 +0300, Eugeniy Paltsev wrote: +On Fri, 2017-04-21 at 18:13 +0300, Andy Shevchenko wrote: +> On Fri, 2017-04-21 at 14:29 +0000, Eugeniy Paltsev wrote: +> > On Tue, 2017-04-18 at 15:31 +0300, Andy Shevchenko wrote: +> > > On Fri, 2017-04-07 at 17:04 +0300, Eugeniy Paltsev wrote: > > > > This patch adds support for the DW AXI DMAC controller. -> > > > +#define AXI_DMA_BUSWIDTHS ??\ +> > > > +#define AXI_DMA_BUSWIDTHS \ > > > > + (DMA_SLAVE_BUSWIDTH_1_BYTE | \ > > > > + DMA_SLAVE_BUSWIDTH_2_BYTES | \ > > > > + DMA_SLAVE_BUSWIDTH_4_BYTES | \ @@ -24,5 +24,349 @@ On Fri, 2017-04-21@18:13 +0300, Andy Shevchenko wrote: > > Strange. AFAIK they are representing bits (which is not the best > idea) in the resulting u64 field. So, anything bigger than 63 doesn't ->?make sense. +> make sense. They are u32 fields! +>From dmaengine.h : +struct dma_device { +... + u32 src_addr_widths; + u32 dst_addr_widths; +}; + +> In drivers where they are not bits quite likely a bug is hidden. +For example (from pxa_dma.c): +const enum dma_slave_buswidth widths = DMA_SLAVE_BUSWIDTH_1_BYTE | +DMA_SLAVE_BUSWIDTH_2_BYTES | DMA_SLAVE_BUSWIDTH_4_BYTES; + +And there are a lot of drivers, which don't use BIT for this fields. +sh/shdmac.c +sh/rcar-dmac.c +qcom/bam_dma.c +mmp_pdma.c +ste_dma40.c +And many others... + +> > +> > > > + +> > > > +static inline void +> > > > +axi_dma_iowrite32(struct axi_dma_chip *chip, u32 reg, u32 val) +> > > > +{ +> > > > + iowrite32(val, chip->regs + reg); +> > > +> > > Are you going to use IO ports for this IP? I don't think so. +> > > Wouldn't be better to call readl()/writel() instead? +> > +> > As I understand, it's better to use ioread/iowrite as more +> > universal +> > IO +> > access way. Am I wrong? +> +> As I said above the ioreadX/iowriteX makes only sense when your IP +> would be accessed via IO region or MMIO. I'm pretty sure IO is not +> the case at all for this IP. +MMIO? This IP works exactly via memory-mapped I/O. + +> > > > +static inline void axi_chan_irq_disable(struct axi_dma_chan +> > > > *chan, +> > > > u32 irq_mask) +> > > > +{ +> > > > + u32 val; +> > > > + +> > > > + if (likely(irq_mask == DWAXIDMAC_IRQ_ALL)) { +> > > > + axi_chan_iowrite32(chan, CH_INTSTATUS_ENA, +> > > > DWAXIDMAC_IRQ_NONE); +> > > > + } else { +> > > +> > > I don't see the benefit. (Yes, I see one read-less path, I think +> > > it +> > > makes perplexity for nothing here) +> > +> > This function is called mostly with irq_mask = DWAXIDMAC_IRQ_ALL. +> > Actually it is called only with irq_mask = DWAXIDMAC_IRQ_ALL until +> > I add DMA_SLAVE support. +> > But I can cut off this 'if' statment, if it is necessery. +> +> It's not necessary, but from my point of view increases readability +> of the code a lot for the price of an additional readl(). +Ok. + +> > +> > > > + val = axi_chan_ioread32(chan, +> > > > CH_INTSTATUS_ENA); +> > > > + val &= ~irq_mask; +> > > > + axi_chan_iowrite32(chan, CH_INTSTATUS_ENA, +> > > > val); +> > > > + } +> > > > + +> > > > + return min_t(size_t, __ffs(sdl), max_width); +> > > > +} +> > > > +static void axi_desc_put(struct axi_dma_desc *desc) +> > > > +{ +> > > > + struct axi_dma_chan *chan = desc->chan; +> > > > + struct dw_axi_dma *dw = chan->chip->dw; +> > > > + struct axi_dma_desc *child, *_next; +> > > > + unsigned int descs_put = 0; +> > > > + list_for_each_entry_safe(child, _next, &desc- +> > > > >xfer_list, +> > > > xfer_list) { +> > > +> > > xfer_list looks redundant. +> > > Can you elaborate why virtual channel management is not working +> > > for +> > > you? +> > +> > Each virtual descriptor encapsulates several hardware descriptors, +> > which belong to same transfer. +> > This list (xfer_list) is used only for allocating/freeing these +> > descriptors and it doesn't affect on virtual dma work logic. +> > I can see this approach in several drivers with VirtDMA (but they +> > mostly use array instead of list) +> +> You described how most of the DMA drivers are implemented, though +> they +> are using just sg_list directly. I would recommend to do the same and +> get rid of this list. +This IP can be (ans is) configured with small block size. +(note, that I am not saying about runtime HW configuration) + +And there is opportunity what we can't use sg_list directly and need to +split sg_list to a smaller chunks. + +> > > Btw, are you planning to use priority at all? For now on I didn't +> > > see +> > > a single driver (from the set I have checked, like 4-5 of them) +> > > that +> > > uses priority anyhow. It makes driver more complex for nothing. +> > +> > Only for dma slave operations. +> +> So, in other words you *have* an actual two or more users that *need* +> prioritization? +As I remember there was an idea to give higher priority to audio dma +chanels. + +> > > > + if (unlikely(dst_nents == 0 || src_nents == 0)) +> > > > + return NULL; +> > > > + +> > > > + if (unlikely(dst_sg == NULL || src_sg == NULL)) +> > > > + return NULL; +> > > +> > > If we need those checks they should go to +> > > dmaengine.h/dmaengine.c. +> > +> > I checked several drivers, which implements device_prep_dma_sg and +> > they +> > implements this checkers. +> > Should I add something like "dma_sg_desc_invalid" function to +> > dmaengine.h ? +> +> I suppose it's better to implement them in the corresponding helpers +> in dmaengine.h. +Ok. + +> > > > +static void axi_chan_list_dump_lli(struct axi_dma_chan *chan, +> > > > + struct axi_dma_desc +> > > > *desc_head) +> > > > +{ +> > > > + struct axi_dma_desc *desc; +> > > > + +> > > > + axi_chan_dump_lli(chan, desc_head); +> > > > + list_for_each_entry(desc, &desc_head->xfer_list, +> > > > xfer_list) +> > > > + axi_chan_dump_lli(chan, desc); +> > > > +} +> > > > + +> > > > +static void axi_chan_handle_err(struct axi_dma_chan *chan, u32 +> > > > status) +> > > > +{ +> > > > + /* WARN about bad descriptor */ +> > > > +> > > > + dev_err(chan2dev(chan), +> > > > + "Bad descriptor submitted for %s, cookie: %d, +> > > > irq: +> > > > 0x%08x\n", +> > > > + axi_chan_name(chan), vd->tx.cookie, status); +> > > > + axi_chan_list_dump_lli(chan, vd_to_axi_desc(vd)); +> > > +> > > As I said earlier dw_dmac is *bad* example of the (virtual +> > > channel +> > > based) DMA driver. +> > > +> > > I guess you may just fail the descriptor and don't pretend it has +> > > been processed successfully. +> > +> > What do you mean by saying "fail the descriptor"? +> > After I get error I cancel current transfer and free all +> > descriptors +> > from it (by calling vchan_cookie_complete). +> > I can't store error status in descriptor structure because it will +> > be +> > freed by vchan_cookie_complete. +> > I can't store error status in channel structure because it will be +> > overwritten by next transfer. +> +> Better not to pretend that it has been processed successfully. Don't +> call callback on it and set its status to DMA_ERROR (that's why +> descriptors in many drivers have dma_status field). When user asks +> for +> status (using cookie) the saved value would be returned until +> descriptor +> is active. +> +> Do you have some other workflow in mind? + +Hmm... +Do you mean I should left error descriptors in desc_issued list +or I should create another list (like desc_error) in my driver and move +error descriptors to desc_error list? + +And when exactly should I free error descriptors? + +I checked hsu/hsu.c dma driver implementation: + vdma descriptor is deleted from desc_issued list when transfer + starts. When descriptor marked as error descriptor + vchan_cookie_complete isn't called for this descriptor. And this + descriptor isn't placed in any list. So error descriptors *never* + will be freed. + I don't actually like this approach. + +> > > > + +> > > > +static const struct dev_pm_ops dw_axi_dma_pm_ops = { +> > > > + SET_RUNTIME_PM_OPS(axi_dma_runtime_suspend, +> > > > axi_dma_runtime_resume, NULL) +> > > > +}; +> > > +> > > Have you tried to build with CONFIG_PM disabled? +> > +> > Yes. +> > +> > > I'm pretty sure you need __maybe_unused applied to your PM ops. +> > +> > I call axi_dma_runtime_suspend / axi_dma_runtime_resume even I +> > dont't +> > use PM. +> > (I call them in probe / remove function.) +> +> Hmm... I didn't check your ->probe() and ->remove(). Do you mean you +> call them explicitly by those names? +> +> If so, please don't do that. Use pm_runtime_*() instead. And... +> +> > So I don't need to declare them with __maybe_unused. +> +> ...in that case it's possible you have them defined but not used. +> +>From my ->probe() function: + +pm_runtime_get_noresume(chip->dev); +ret = axi_dma_runtime_resume(chip->dev); + +Firstly I only incrememt counter. +Secondly explicitly call my resume function. + +I call them explicitly because I need driver to work also without +Runtime PM. So I can't just call pm_runtime_get here instead of +pm_runtime_get_noresume + axi_dma_runtime_resume. + +Of course I can copy *all* code from axi_dma_runtime_resume +to ->probe() function, but I don't really like this idea. + +> > > > +struct axi_dma_chan { +> > > > + struct axi_dma_chip *chip; +> > > > + void __iomem *chan_regs; +> > > > + u8 id; +> > > > + atomic_t descs_allocated; +> > > > + +> > > > + struct virt_dma_chan vc; +> > > > + +> > > > + /* these other elements are all protected by vc.lock +> > > > */ +> > > > + bool is_paused; +> > > +> > > I still didn't get (already forgot) why you can't use dma_status +> > > instead for the active descriptor? +> > +> > As I said before, I checked several driver, which have status +> > variable +> > in their channel structure - it is used *only* for determinating is +> > channel paused or not. So there is no much sense in replacing +> > "is_paused" to "status" and I left "is_paused" variable untouched. +> +> Not only (see above), the errored descriptor keeps that status. +> +> > (I described above why we can't use status in channel structure for +> > error handling) +> +> Ah, I'm talking about descriptor. + +Again - PAUSED is per-channel flag. So, even if we have status field in +each descriptor, it is simpler to use one per-channel flag instead of +plenty per-descriptor flags. +When we pausing/resuming dma channel it is simpler to set only one flag +instead of writing DMA_PAUSED to *each* descriptor status field. + +> > > > Status Fetch Addr */ +> > > > +#define CH_INTSTATUS_ENA 0x080 /* R/W Chan Interrupt +> > > > Status +> > > > Enable */ +> > > > +#define CH_INTSTATUS 0x088 /* R/W Chan +> > > > Interrupt +> > > > Status */ +> > > > +#define CH_INTSIGNAL_ENA 0x090 /* R/W Chan Interrupt +> > > > Signal +> > > > Enable */ +> > > > +#define CH_INTCLEAR 0x098 /* W Chan Interrupt +> > > > Clear +> > > > */ +> > > +> > > I'm wondering if you can use regmap API instead. +> > +> > Is it really necessary? It'll make driver more complex for nothing. +> +> That's why I'm not suggesting but wondering. +> > +> > > > +}; +> > > +> > > Hmm... do you need them in the header? +> > +> > I use some of these definitions in my code so I guess yes. +> > /* Maybe I misunderstood your question... */ +> +> I mean, who are the users of them? If it's only one module, there is +> no need to put them in header. +Yes, only one module. +Should I move all this definitions to axi_dma_platform.c file and rid +of both axi_dma_platform_reg.h and axi_dma_platform.h headers? + +> > +> > > > +#define CH_CFG_H_TT_FC_POS 0 +> > > > +enum { +> > > > + DWAXIDMAC_TT_FC_MEM_TO_MEM_DMAC = 0x0, +> > > > + DWAXIDMAC_TT_FC_MEM_TO_PER_DMAC, +> > > > + DWAXIDMAC_TT_FC_PER_TO_MEM_DMAC, +> > > > + DWAXIDMAC_TT_FC_PER_TO_PER_DMAC, +> > > > + DWAXIDMAC_TT_FC_PER_TO_MEM_SRC, +> > > > + DWAXIDMAC_TT_FC_PER_TO_PER_SRC, +> > > > + DWAXIDMAC_TT_FC_MEM_TO_PER_DST, +> > > > + DWAXIDMAC_TT_FC_PER_TO_PER_DST +> > > > +}; +> > > +> > > Some of definitions are the same as for dw_dmac, right? We might +> > > split them to a common header, though I have no strong opinion +> > > about +> > +> > it. +> > > Vinod? +> > +> > APB DMAC and AXI DMAC have completely different regmap. So there is +> > no +> > much sense to do that. +> +> I'm not talking about registers, I'm talking about definitions like +> above. Though it would be indeed little sense to split some common +> code between those two. +This definitions are the fields of some registers. So they are also +different. + +-- + Eugeniy Paltsev diff --git a/a/content_digest b/N2/content_digest index 7fe943d..741b5a4 100644 --- a/a/content_digest +++ b/N2/content_digest @@ -3,19 +3,28 @@ "ref\01492518695.24567.56.camel@linux.intel.com\0" "ref\01492784977.16657.6.camel@synopsys.com\0" "ref\01492787583.24567.120.camel@linux.intel.com\0" - "From\0Eugeniy.Paltsev@synopsys.com (Eugeniy Paltsev)\0" - "Subject\0[PATCH v2 2/2] dmaengine: Add DW AXI DMAC driver\0" + "From\0Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>\0" + "Subject\0Re: [PATCH v2 2/2] dmaengine: Add DW AXI DMAC driver\0" "Date\0Mon, 24 Apr 2017 15:55:06 +0000\0" - "To\0linux-snps-arc@lists.infradead.org\0" + "To\0andriy.shevchenko@linux.intel.com <andriy.shevchenko@linux.intel.com>\0" + "Cc\0vinod.koul@intel.com <vinod.koul@intel.com>" + linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> + robh+dt@kernel.org <robh+dt@kernel.org> + Alexey.Brodkin@synopsys.com <Alexey.Brodkin@synopsys.com> + devicetree@vger.kernel.org <devicetree@vger.kernel.org> + Eugeniy.Paltsev@synopsys.com <Eugeniy.Paltsev@synopsys.com> + linux-snps-arc@lists.infradead.org <linux-snps-arc@lists.infradead.org> + dan.j.williams@intel.com <dan.j.williams@intel.com> + " dmaengine@vger.kernel.org <dmaengine@vger.kernel.org>\0" "\00:1\0" "b\0" "Hi,\n" - "On Fri, 2017-04-21@18:13 +0300, Andy Shevchenko wrote:\n" - "> On Fri, 2017-04-21@14:29 +0000, Eugeniy Paltsev wrote:\n" - "> > On Tue, 2017-04-18@15:31 +0300, Andy Shevchenko wrote:\n" - "> > > On Fri, 2017-04-07@17:04 +0300, Eugeniy Paltsev wrote:\n" + "On Fri, 2017-04-21 at 18:13 +0300, Andy Shevchenko wrote:\n" + "> On Fri, 2017-04-21 at 14:29 +0000, Eugeniy Paltsev wrote:\n" + "> > On Tue, 2017-04-18 at 15:31 +0300, Andy Shevchenko wrote:\n" + "> > > On Fri, 2017-04-07 at 17:04 +0300, Eugeniy Paltsev wrote:\n" "> > > > This patch adds support for the DW AXI DMAC controller.\n" - "> > > > +#define AXI_DMA_BUSWIDTHS\t\t??\\\n" + "> > > > +#define AXI_DMA_BUSWIDTHS\t\t\302\240\302\240\\\n" "> > > > +\t(DMA_SLAVE_BUSWIDTH_1_BYTE\t| \\\n" "> > > > +\tDMA_SLAVE_BUSWIDTH_2_BYTES\t| \\\n" "> > > > +\tDMA_SLAVE_BUSWIDTH_4_BYTES\t| \\\n" @@ -35,7 +44,351 @@ ">\n" "> Strange. AFAIK they are representing bits (which is not the best\n" "> idea) in the resulting u64 field. So, anything bigger than 63 doesn't\n" - ">?make sense.\n" - They are u32 fields! + ">\302\240make sense.\n" + "They are u32 fields!\n" + ">From dmaengine.h :\n" + "struct dma_device {\n" + "...\n" + "\302\240\302\240\302\240\302\240u32 src_addr_widths;\n" + "\302\240\302\240\302\240\302\240u32 dst_addr_widths;\n" + "};\n" + "\n" + "> In drivers where they are not bits quite likely a bug is hidden.\n" + "For example (from pxa_dma.c):\n" + "const enum dma_slave_buswidth widths = DMA_SLAVE_BUSWIDTH_1_BYTE |\n" + "DMA_SLAVE_BUSWIDTH_2_BYTES | DMA_SLAVE_BUSWIDTH_4_BYTES;\n" + "\n" + "And there are a lot of drivers, which don't use BIT for this fields.\n" + "sh/shdmac.c\n" + "sh/rcar-dmac.c\n" + "qcom/bam_dma.c\n" + "mmp_pdma.c\n" + "ste_dma40.c\n" + "And many others...\n" + "\n" + "> >\n" + "> > > > +\n" + "> > > > +static inline void\n" + "> > > > +axi_dma_iowrite32(struct axi_dma_chip *chip, u32 reg, u32 val)\n" + "> > > > +{\n" + "> > > > +\tiowrite32(val, chip->regs + reg);\n" + "> > >\n" + "> > > Are you going to use IO ports for this IP? I don't think so.\n" + "> > > Wouldn't be better to call readl()/writel() instead?\n" + "> >\n" + "> > As I understand, it's better to use ioread/iowrite as more\n" + "> > universal\n" + "> > IO\n" + "> > access way. Am I wrong?\n" + ">\n" + "> As I said above the ioreadX/iowriteX makes only sense when your IP\n" + "> would be accessed via IO region or MMIO. I'm pretty sure IO is not\n" + "> the case at all for this IP.\n" + "MMIO? This IP works exactly via memory-mapped I/O.\n" + "\n" + "> > > > +static inline void axi_chan_irq_disable(struct axi_dma_chan\n" + "> > > > *chan,\n" + "> > > > u32 irq_mask)\n" + "> > > > +{\n" + "> > > > +\tu32 val;\n" + "> > > > +\n" + "> > > > +\tif (likely(irq_mask == DWAXIDMAC_IRQ_ALL)) {\n" + "> > > > +\t\taxi_chan_iowrite32(chan, CH_INTSTATUS_ENA,\n" + "> > > > DWAXIDMAC_IRQ_NONE);\n" + "> > > > +\t} else {\n" + "> > >\n" + "> > > I don't see the benefit. (Yes, I see one read-less path, I think\n" + "> > > it\n" + "> > > makes perplexity for nothing here)\n" + "> >\n" + "> > This function is called mostly with irq_mask = DWAXIDMAC_IRQ_ALL.\n" + "> > Actually it is called only with irq_mask = DWAXIDMAC_IRQ_ALL until\n" + "> > I add DMA_SLAVE support.\n" + "> > But I can cut off this 'if' statment, if it is necessery.\n" + ">\n" + "> It's not necessary, but from my point of view increases readability\n" + "> of the code a lot for the price of an additional readl().\n" + "Ok.\n" + "\n" + "> >\n" + "> > > > +\t\tval = axi_chan_ioread32(chan,\n" + "> > > > CH_INTSTATUS_ENA);\n" + "> > > > +\t\tval &= ~irq_mask;\n" + "> > > > +\t\taxi_chan_iowrite32(chan, CH_INTSTATUS_ENA,\n" + "> > > > val);\n" + "> > > > +\t}\n" + "> > > > +\n" + "> > > > +\treturn min_t(size_t, __ffs(sdl), max_width);\n" + "> > > > +}\n" + "> > > > +static void axi_desc_put(struct axi_dma_desc *desc)\n" + "> > > > +{\n" + "> > > > +\tstruct axi_dma_chan *chan = desc->chan;\n" + "> > > > +\tstruct dw_axi_dma *dw = chan->chip->dw;\n" + "> > > > +\tstruct axi_dma_desc *child, *_next;\n" + "> > > > +\tunsigned int descs_put = 0;\n" + "> > > > +\tlist_for_each_entry_safe(child, _next, &desc-\n" + "> > > > >xfer_list,\n" + "> > > > xfer_list) {\n" + "> > >\n" + "> > > xfer_list looks redundant.\n" + "> > > Can you elaborate why virtual channel management is not working\n" + "> > > for\n" + "> > > you?\n" + "> >\n" + "> > Each virtual descriptor encapsulates several hardware descriptors,\n" + "> > which belong to same transfer.\n" + "> > This list (xfer_list) is used only for allocating/freeing these\n" + "> > descriptors and it doesn't affect on virtual dma work logic.\n" + "> > I can see this approach in several drivers with VirtDMA (but they\n" + "> > mostly use array instead of list)\n" + ">\n" + "> You described how most of the DMA drivers are implemented, though\n" + "> they\n" + "> are using just sg_list directly. I would recommend to do the same and\n" + "> get rid of this list.\n" + "This IP can be (ans is) configured with small block size.\n" + "(note, that I am not saying about runtime HW configuration)\n" + "\n" + "And there is opportunity what we can't use sg_list directly and need to\n" + "split sg_list to a smaller chunks.\n" + "\n" + "> > > Btw, are you planning to use priority at all? For now on I didn't\n" + "> > > see\n" + "> > > a single driver (from the set I have checked, like 4-5 of them)\n" + "> > > that\n" + "> > > uses priority anyhow. It makes driver more complex for nothing.\n" + "> >\n" + "> > Only for dma slave operations.\n" + ">\n" + "> So, in other words you *have* an actual two or more users that *need*\n" + "> prioritization?\n" + "As I remember there was an idea to give higher priority to audio dma\n" + "chanels.\n" + "\n" + "> > > > +\tif (unlikely(dst_nents == 0 || src_nents == 0))\n" + "> > > > +\t\treturn NULL;\n" + "> > > > +\n" + "> > > > +\tif (unlikely(dst_sg == NULL || src_sg == NULL))\n" + "> > > > +\t\treturn NULL;\n" + "> > >\n" + "> > > If we need those checks they should go to\n" + "> > > dmaengine.h/dmaengine.c.\n" + "> >\n" + "> > I checked several drivers, which implements device_prep_dma_sg and\n" + "> > they\n" + "> > implements this checkers.\n" + "> > Should I add something like \"dma_sg_desc_invalid\" function to\n" + "> > dmaengine.h ?\n" + ">\n" + "> I suppose it's better to implement them in the corresponding helpers\n" + "> in dmaengine.h.\n" + "Ok.\n" + "\n" + "> > > > +static void axi_chan_list_dump_lli(struct axi_dma_chan *chan,\n" + "> > > > +\t\t\t\t\302\240\302\240\302\240struct axi_dma_desc\n" + "> > > > *desc_head)\n" + "> > > > +{\n" + "> > > > +\tstruct axi_dma_desc *desc;\n" + "> > > > +\n" + "> > > > +\taxi_chan_dump_lli(chan, desc_head);\n" + "> > > > +\tlist_for_each_entry(desc, &desc_head->xfer_list,\n" + "> > > > xfer_list)\n" + "> > > > +\t\taxi_chan_dump_lli(chan, desc);\n" + "> > > > +}\n" + "> > > > +\n" + "> > > > +static void axi_chan_handle_err(struct axi_dma_chan *chan, u32\n" + "> > > > status)\n" + "> > > > +{\n" + "> > > > +\t/* WARN about bad descriptor */\n" + "> > > >\n" + "> > > > +\tdev_err(chan2dev(chan),\n" + "> > > > +\t\t\"Bad descriptor submitted for %s, cookie: %d,\n" + "> > > > irq:\n" + "> > > > 0x%08x\\n\",\n" + "> > > > +\t\taxi_chan_name(chan), vd->tx.cookie, status);\n" + "> > > > +\taxi_chan_list_dump_lli(chan, vd_to_axi_desc(vd));\n" + "> > >\n" + "> > > As I said earlier dw_dmac is *bad* example of the (virtual\n" + "> > > channel\n" + "> > > based) DMA driver.\n" + "> > >\n" + "> > > I guess you may just fail the descriptor and don't pretend it has\n" + "> > > been processed successfully.\n" + "> >\n" + "> > What do you mean by saying \"fail the descriptor\"?\n" + "> > After I get error I cancel current transfer and free all\n" + "> > descriptors\n" + "> > from it (by calling vchan_cookie_complete).\n" + "> > I can't store error status in descriptor structure because it will\n" + "> > be\n" + "> > freed by vchan_cookie_complete.\n" + "> > I can't store error status in channel structure because it will be\n" + "> > overwritten by next transfer.\n" + ">\n" + "> Better not to pretend that it has been processed successfully. Don't\n" + "> call callback on it and set its status to DMA_ERROR (that's why\n" + "> descriptors in many drivers have dma_status field). When user asks\n" + "> for\n" + "> status (using cookie) the saved value would be returned until\n" + "> descriptor\n" + "> is active.\n" + ">\n" + "> Do you have some other workflow in mind?\n" + "\n" + "Hmm...\n" + "Do you mean I should left error descriptors in desc_issued list\n" + "or I should create another list (like desc_error) in my driver and move\n" + "error descriptors to desc_error list?\n" + "\n" + "And when exactly should I free error descriptors?\n" + "\n" + "I checked hsu/hsu.c dma driver implementation:\n" + "\302\240 vdma descriptor is deleted from desc_issued list when transfer\n" + "\302\240 starts. When descriptor marked as error descriptor\n" + "\302\240 vchan_cookie_complete isn't called for this descriptor. And this\n" + "\302\240 descriptor isn't placed in any list. So error descriptors *never*\n" + "\302\240 will be freed.\n" + "\302\240 I don't actually like this approach.\n" + "\n" + "> > > > +\n" + "> > > > +static const struct dev_pm_ops dw_axi_dma_pm_ops = {\n" + "> > > > +\tSET_RUNTIME_PM_OPS(axi_dma_runtime_suspend,\n" + "> > > > axi_dma_runtime_resume, NULL)\n" + "> > > > +};\n" + "> > >\n" + "> > > Have you tried to build with CONFIG_PM disabled?\n" + "> >\n" + "> > Yes.\n" + "> >\n" + "> > > I'm pretty sure you need __maybe_unused applied to your PM ops.\n" + "> >\n" + "> > I call axi_dma_runtime_suspend / axi_dma_runtime_resume even I\n" + "> > dont't\n" + "> > use PM.\n" + "> > (I call them in probe / remove function.)\n" + ">\n" + "> Hmm... I didn't check your ->probe() and ->remove(). Do you mean you\n" + "> call them explicitly by those names?\n" + ">\n" + "> If so, please don't do that. Use pm_runtime_*() instead. And...\n" + ">\n" + "> > So I don't need to declare them with __maybe_unused.\n" + ">\n" + "> ...in that case it's possible you have them defined but not used.\n" + ">\n" + ">From my ->probe() function:\n" + "\n" + "pm_runtime_get_noresume(chip->dev);\n" + "ret = axi_dma_runtime_resume(chip->dev);\n" + "\n" + "Firstly I only incrememt counter.\n" + "Secondly explicitly call my resume function.\n" + "\n" + "I call them explicitly because I need driver to work also without\n" + "Runtime PM. So I can't just call pm_runtime_get here instead of\n" + "pm_runtime_get_noresume + axi_dma_runtime_resume.\n" + "\n" + "Of course I can copy *all* code from axi_dma_runtime_resume\n" + "to ->probe() function, but I don't really like this idea.\n" + "\n" + "> > > > +struct axi_dma_chan {\n" + "> > > > +\tstruct axi_dma_chip\t\t*chip;\n" + "> > > > +\tvoid __iomem\t\t\t*chan_regs;\n" + "> > > > +\tu8\t\t\t\tid;\n" + "> > > > +\tatomic_t\t\t\tdescs_allocated;\n" + "> > > > +\n" + "> > > > +\tstruct virt_dma_chan\t\tvc;\n" + "> > > > +\n" + "> > > > +\t/* these other elements are all protected by vc.lock\n" + "> > > > */\n" + "> > > > +\tbool\t\t\t\tis_paused;\n" + "> > >\n" + "> > > I still didn't get (already forgot) why you can't use dma_status\n" + "> > > instead for the active descriptor?\n" + "> >\n" + "> > As I said before, I checked several driver, which have status\n" + "> > variable\n" + "> > in their channel structure - it is used *only* for determinating is\n" + "> > channel paused or not. So there is no much sense in replacing\n" + "> > \"is_paused\" to \"status\" and I left \"is_paused\" variable untouched.\n" + ">\n" + "> Not only (see above), the errored descriptor keeps that status.\n" + ">\n" + "> > (I described above why we can't use status in channel structure for\n" + "> > error handling)\n" + ">\n" + "> Ah, I'm talking about descriptor.\n" + "\n" + "Again - PAUSED is per-channel flag. So, even if we have status field in\n" + "each descriptor, it is simpler to use one per-channel flag instead of\n" + "plenty per-descriptor flags.\n" + "When we pausing/resuming dma channel it is simpler to set only one flag\n" + "instead of writing DMA_PAUSED to *each* descriptor status field.\n" + "\n" + "> > > > Status Fetch Addr */\n" + "> > > > +#define CH_INTSTATUS_ENA\t0x080 /* R/W Chan Interrupt\n" + "> > > > Status\n" + "> > > > Enable */\n" + "> > > > +#define CH_INTSTATUS\t\t0x088 /* R/W Chan\n" + "> > > > Interrupt\n" + "> > > > Status */\n" + "> > > > +#define CH_INTSIGNAL_ENA\t0x090 /* R/W Chan Interrupt\n" + "> > > > Signal\n" + "> > > > Enable */\n" + "> > > > +#define CH_INTCLEAR\t\t0x098 /* W Chan Interrupt\n" + "> > > > Clear\n" + "> > > > */\n" + "> > >\n" + "> > > I'm wondering if you can use regmap API instead.\n" + "> >\n" + "> > Is it really necessary? It'll make driver more complex for nothing.\n" + ">\n" + "> That's why I'm not suggesting but wondering.\n" + "> >\n" + "> > > > +};\n" + "> > >\n" + "> > > Hmm... do you need them in the header?\n" + "> >\n" + "> > I use some of these definitions in my code so I guess yes.\n" + "> > /* Maybe I misunderstood your question... */\n" + ">\n" + "> I mean, who are the users of them? If it's only one module, there is\n" + "> no need to put them in header.\n" + "Yes, only one module.\n" + "Should I move all this definitions to axi_dma_platform.c file and rid\n" + "of both axi_dma_platform_reg.h and axi_dma_platform.h headers?\n" + "\n" + "> >\n" + "> > > > +#define CH_CFG_H_TT_FC_POS\t0\n" + "> > > > +enum {\n" + "> > > > +\tDWAXIDMAC_TT_FC_MEM_TO_MEM_DMAC\t= 0x0,\n" + "> > > > +\tDWAXIDMAC_TT_FC_MEM_TO_PER_DMAC,\n" + "> > > > +\tDWAXIDMAC_TT_FC_PER_TO_MEM_DMAC,\n" + "> > > > +\tDWAXIDMAC_TT_FC_PER_TO_PER_DMAC,\n" + "> > > > +\tDWAXIDMAC_TT_FC_PER_TO_MEM_SRC,\n" + "> > > > +\tDWAXIDMAC_TT_FC_PER_TO_PER_SRC,\n" + "> > > > +\tDWAXIDMAC_TT_FC_MEM_TO_PER_DST,\n" + "> > > > +\tDWAXIDMAC_TT_FC_PER_TO_PER_DST\n" + "> > > > +};\n" + "> > >\n" + "> > > Some of definitions are the same as for dw_dmac, right? We might\n" + "> > > split them to a common header, though I have no strong opinion\n" + "> > > about\n" + "> >\n" + "> > it.\n" + "> > > Vinod?\n" + "> >\n" + "> > APB DMAC and AXI DMAC have completely different regmap. So there is\n" + "> > no\n" + "> > much sense to do that.\n" + ">\n" + "> I'm not talking about registers, I'm talking about definitions like\n" + "> above. Though it would be indeed little sense to split some common\n" + "> code between those two.\n" + "This definitions are the fields of some registers. So they are also\n" + "different.\n" + "\n" + "--\n" + "\302\240Eugeniy Paltsev" -644b21629fbf035165f06e21bb6d27283769d773bc5c8f1a2af422e82f768a39 +9f1ede68b520be19b9d05496cda3efac051ae9d08a3d42c8773964fb89a21ca1
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.