diff for duplicates of <1486648722.2657.18.camel@synopsys.com> diff --git a/a/1.txt b/N1/1.txt index 8d92b2d..03be1fb 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,26 +1,26 @@ Thanks for response. My comments are given inline below. -On Wed, 2017-01-25@19:25 +0200, Andy Shevchenko wrote: -> On Wed, 2017-01-25@18:34 +0300, Eugeniy Paltsev wrote: -> >? +On Wed, 2017-01-25 at 19:25 +0200, Andy Shevchenko wrote: +> On Wed, 2017-01-25 at 18:34 +0300, Eugeniy Paltsev wrote: +> > > > This patch adds support for the DW AXI DMAC controller. -> >? +> > > > DW AXI DMAC is a part of upcoming development board from Synopsys. -> >? +> > > > In this driver implementation only DMA_MEMCPY and DMA_SG transfers > > are supported. -> >? +> > > Few more comments on top of not addressed/answered yet. ->? -> >? +> +> > > > +static inline void axi_chan_disable(struct axi_dma_chan *chan) > > +{ > > + u32 val; > > + > > + val = axi_dma_ioread32(chan->chip, DMAC_CHEN); > > + val &= ~(BIT(chan->id) << DMAC_CHAN_EN_SHIFT); -> > + val |=??(BIT(chan->id) << DMAC_CHAN_EN_WE_SHIFT); +> > + val |= (BIT(chan->id) << DMAC_CHAN_EN_WE_SHIFT); > > + axi_dma_iowrite32(chan->chip, DMAC_CHEN, val); > > +} > > + @@ -29,18 +29,18 @@ On Wed, 2017-01-25@19:25 +0200, Andy Shevchenko wrote: > > + u32 val; > > + > > + val = axi_dma_ioread32(chan->chip, DMAC_CHEN); -> >? +> > > > + val |= (BIT(chan->id) << DMAC_CHAN_EN_SHIFT | > > + BIT(chan->id) << DMAC_CHAN_EN_WE_SHIFT); -> >? +> > > Redundant parens. Will be fixed. -> >? +> > > > + axi_dma_iowrite32(chan->chip, DMAC_CHEN, val); > > +} ->? -> >? +> +> > > > +static void axi_desc_put(struct axi_dma_desc *desc) > > +{ > > + struct axi_dma_chan *chan = desc->chan; @@ -48,7 +48,7 @@ Will be fixed. > > + struct axi_dma_desc *child, *_next; > > + unsigned int descs_put = 0; > > + -> >? +> > > > + if (unlikely(!desc)) > > + return; > Would it be the case? @@ -57,28 +57,28 @@ remove it. Also about your previous question about likely/unlikely: I checked disassembly - gcc generates different code when I use likely -and unlikely. So, I guess they are useful.? +and unlikely. So, I guess they are useful. -> >? +> > > > +static void dma_chan_issue_pending(struct dma_chan *dchan) > > +{ > > + struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan); > > + unsigned long flags; > > + -> >? +> > > > + dev_dbg(dchan2dev(dchan), "%s: %s\n", __func__, > > axi_chan_name(chan)); > Messages like this kinda redundant. > Either you use function tracer and see them anyway, or you are using > Dynamic Debug, which may include function name. ->? +> > Basically you wrote an equivalent to something like ->? +> > dev_dbg(dev, "%s\n", channame); -Agreed. I'll remove dev_dbg from here because I also have it in? +Agreed. I'll remove dev_dbg from here because I also have it in axi_chan_start_first_queued. -> >? +> > > > + > > + spin_lock_irqsave(&chan->vc.lock, flags); > > + if (vchan_issue_pending(&chan->vc)) @@ -86,10 +86,10 @@ axi_chan_start_first_queued. > > + spin_unlock_irqrestore(&chan->vc.lock, flags); > ...and taking into account the function itself one might expect > something useful printed in _start_first_queued(). ->? +> > For some cases there is also dev_vdbg(). ->? -> >? +> +> > > > +} > > + > > +static int dma_chan_alloc_chan_resources(struct dma_chan *dchan) @@ -103,23 +103,23 @@ axi_chan_start_first_queued. > > + return -EBUSY; > > + } > > + -> >? +> > > > + dev_dbg(dchan2dev(dchan), "%s: allocating\n", > > axi_chan_name(chan)); > Can you try to enable DMADEVICES_DEBUG and VDEBUG and see what is > useful > and what is not? ->? +> > Give a chance to function tracer as well. Yep, this dev_dbg is redundant, so I'll remove it. -> >? +> > > > +static void dma_chan_free_chan_resources(struct dma_chan *dchan) > > +{ > > + struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan); > > + -> >? -> >? +> > +> > > > + /* ASSERT: channel is idle */ > > + if (axi_chan_is_hw_enable(chan)) > > + dev_err(dchan2dev(dchan), "%s is non-idle!\n", @@ -131,18 +131,18 @@ See comment below. > >> > +static inline bool axi_dma_is_sw_enable(struct axi_dma_chip *chip) > >> > +{ -> >> > +???struct dw_axi_dma *dw = chip->dw; -> >> > +???u32 i; +> >> > + struct dw_axi_dma *dw = chip->dw; +> >> > + u32 i; > >> > + -> >> > +???for (i = 0; i < dw->hdata->nr_channels; i++) { -> >> > +???????????if (dw->chan[i].in_use) +> >> > + for (i = 0; i < dw->hdata->nr_channels; i++) { +> >> > + if (dw->chan[i].in_use) > >> Hmm... I know why we have such flag in dw_dmac, but I doubt it's > >> needed > >> in this driver. Just double check the need of it. > > I use this flag to check state of channel (used now/unused) to disable > > dmac if all channels are unused for now. ->? +> > Okay, but wouldn't be easier to use runtime PM for that? You will not > need any special counter and runtime PM will take case of > enabling/disabling the device. @@ -153,7 +153,7 @@ prefer leave it untouched. Also all existing SoCs with this DMAC don't support power management - so there is no really profit from implementing PM. -> >? +> > > > + > > +/* TODO: REVISIT: how we should choose AXI master for mem-to-mem > > transfer? */ @@ -164,23 +164,23 @@ Yep, for existing SoCs there is no difference - both masters have access to memory. But it isn't necessarily true for future SoCs. But possibly I shouldn't think about it now. -> >? +> > > > + while (true) { > Usually it makes readability harder and rises a red flag to the code. I can write something like next code. Not sure is it looks better. ----------->8------------------ while ((dst_len || dst_sg && dst_nents) && -? ? ? ?(src_len || src_sg && src_nents)) { + (src_len || src_sg && src_nents)) { ... ----------->8------------------ -> >? -> >? /* Manage transfer list (xfer_list) */ +> > +> > /* Manage transfer list (xfer_list) */ > > + if (!first) { > > + first = desc; > > + } else { > > + list_add_tail(&desc->xfer_list, &first- -> > >? +> > > > > > xfer_list); > > + write_desc_llp(prev, desc->vd.tx.phys | > > lms); @@ -211,17 +211,17 @@ really little overhead, so I prefer leave this code unchanged. > for the DMA device. In my opinion it is driver responsibility, not the caller. -> >? +> > > > + > > +static struct dma_async_tx_descriptor * > > +dma_chan_prep_dma_memcpy(struct dma_chan *dchan, dma_addr_t dest, -> > + ?dma_addr_t src, size_t len, unsigned long +> > + dma_addr_t src, size_t len, unsigned long > > flags) > > +{ > Do you indeed have a use case for this except debugging and testing? At least for IO coherency engine testing. -> >? +> > > > + unsigned int nents = 1; > > + struct scatterlist dst_sg; > > + struct scatterlist src_sg; @@ -238,18 +238,18 @@ At least for IO coherency engine testing. > > + /* Implement memcpy transfer as sg transfer with single > > list > > */ -> >? +> > > > + return dma_chan_prep_dma_sg(dchan, &dst_sg, nents, -> > + ????&src_sg, nents, flags); +> > + &src_sg, nents, flags); > One line? It will be more than 80 character. -> >? +> > > > +} -> >? +> > > > + > > +static void axi_chan_dump_lli(struct axi_dma_chan *chan, -> > + ??????struct axi_dma_desc *desc) +> > + struct axi_dma_desc *desc) > > +{ > > + dev_err(dchan2dev(&chan->vc.chan), > > + "SAR: 0x%x DAR: 0x%x LLP: 0x%x BTS 0x%x CTL: @@ -264,9 +264,9 @@ It will be more than 80 character. > IP) can implement MMIO tracer. But we had to use such code until it happens. -> >? +> > > > +} -> >? +> > > > + axi_chan_dump_lli(chan, desc); > > +} > > + @@ -279,11 +279,11 @@ list_for_each_entry(desc, &desc_head->xfer_list, xfer_list) axi_chan_dump_lli(chan, desc); ----------->8------------------ -> >? +> > > > +static void axi_chan_handle_err(struct axi_dma_chan *chan, u32 > > status) > > +{ -> >? +> > > > + > > +static int dma_chan_pause(struct dma_chan *dchan) > > +{ @@ -301,27 +301,27 @@ list_for_each_entry(desc, &desc_head->xfer_list, xfer_list) > Redundant parens. Will be fixed. -> >? +> > > > + axi_dma_iowrite32(chan->chip, DMAC_CHEN, val); > > + > > + while (timeout--) { -> >? +> > > > + if (axi_chan_irq_read(chan) & > > DWAXIDMAC_IRQ_SUSPENDED) { -> >? +> > > > + axi_chan_irq_clear(chan, > > DWAXIDMAC_IRQ_SUSPENDED); > You may move this invariant out from the loop. Makes code cleaner. Agreed. -> >? +> > > > + ret = 0; > > + break; > > + } -> >? +> > > > + udelay(2); ->? -> >? +> +> > > > + } > > + > > + chan->is_paused = true; @@ -331,7 +331,7 @@ Agreed. > with > descriptors. So, that makes channel(paused) == active > descriptor(paused). ->? +> > The trick allows to make code cleaner. I don't understand how we can use use descriptor status for it. @@ -343,7 +343,7 @@ if (chan->is_paused && ret == DMA_IN_PROGRESS) return DMA_PAUSED; ----------->8------------------ -> >? +> > > > + ret = device_property_read_u32_array(dev, "priority", > > carr, > I'm not sure you will have a use case for that. Have you? @@ -352,20 +352,20 @@ transfers. But we need priority management for slave dma (I'll implement slave dma api soon) -> >? +> > > > + ret = devm_request_irq(chip->dev, chip->irq, > > dw_axi_dma_intretupt, -> > + ???????IRQF_SHARED, DRV_NAME, chip); +> > + IRQF_SHARED, DRV_NAME, chip); > > + if (ret) > > + return ret; > You can't do this unless you are using threaded IRQ handler without > any > tasklets involved. ->? +> > There was a nice mail by Thomas who explained what the problem you > have > there. ->? +> > It's a bug in your code. Yep, thanks a lot. Looks like I have problem with driver remove function. @@ -381,7 +381,7 @@ static int dw_remove(...) } ----------->8------------------ -> >? +> > > > +static int __init dw_init(void) > > +{ > > + return platform_driver_register(&dw_driver); @@ -390,21 +390,21 @@ static int dw_remove(...) > Hmm... Why it can't be just a platform driver? You describe the > dependency in Device Tree, if you have something happened, you may > utilize -EPROBE_DEFER. ->? +> Will be fixed. -> >??* Add DT bindings -> >? +> > * Add DT bindings +> > > > Eugeniy Paltsev (2): -> >???dt-bindings: Document the Synopsys DW AXI DMA bindings -> >???dmaengine: Add DW AXI DMAC driver -> >? -> >??.../devicetree/bindings/dma/snps,axi-dw-dmac.txt???|???33 + -> >??drivers/dma/Kconfig????????????????????????????????|????8 + -> >??drivers/dma/Makefile???????????????????????????????|????1 + -> >? -> >??drivers/dma/axi_dma_platform.c?????????????????????| 1060 +> > dt-bindings: Document the Synopsys DW AXI DMA bindings +> > dmaengine: Add DW AXI DMAC driver +> > +> > .../devicetree/bindings/dma/snps,axi-dw-dmac.txt | 33 + +> > drivers/dma/Kconfig | 8 + +> > drivers/dma/Makefile | 1 + +> > +> > drivers/dma/axi_dma_platform.c | 1060 > > ++++++++++++++++++++ > This surprises me. I would expect more then 100+ LOC reduction when > switched to virt-dma API. Can you double check that you are using it @@ -415,17 +415,17 @@ increase). Also about previous comment, which I missed before: > > +static u32 axi_chan_get_xfer_width(struct axi_dma_chan *chan, -> >????????????????????dma_addr_t src, dma_addr_t dst, size_t len) +> > dma_addr_t src, dma_addr_t dst, size_t len) > > +{ > > + u32 width; > > + dma_addr_t sdl = (src | dst | len); > > + u32 max_width = chan->chip->dw->hdata->m_data_width; ->? +> > Use reverse tree. ->? +> > dma_addr_t use above is wrong. (size_t might be bigger in some cases) What do you mean by the phrase "Use reverse tree" ? ---? -?Eugeniy Paltsev +-- + Eugeniy Paltsev diff --git a/a/content_digest b/N1/content_digest index 2d5aa4a..731c157 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -1,35 +1,41 @@ "ref\01485358457-22957-1-git-send-email-Eugeniy.Paltsev@synopsys.com\0" "ref\01485358457-22957-3-git-send-email-Eugeniy.Paltsev@synopsys.com\0" "ref\01485365106.2133.331.camel@linux.intel.com\0" - "From\0Eugeniy.Paltsev@synopsys.com (Eugeniy Paltsev)\0" - "Subject\0[PATCH 2/2] dmaengine: Add DW AXI DMAC driver\0" + "From\0Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>\0" + "Subject\0Re: [PATCH 2/2] dmaengine: Add DW AXI DMAC driver\0" "Date\0Thu, 9 Feb 2017 13:58:42 +0000\0" - "To\0linux-snps-arc@lists.infradead.org\0" + "To\0andriy.shevchenko@linux.intel.com <andriy.shevchenko@linux.intel.com>\0" + "Cc\0dmaengine@vger.kernel.org <dmaengine@vger.kernel.org>" + dan.j.williams@intel.com <dan.j.williams@intel.com> + linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> + Alexey.Brodkin@synopsys.com <Alexey.Brodkin@synopsys.com> + vinod.koul@intel.com <vinod.koul@intel.com> + " linux-snps-arc@lists.infradead.org <linux-snps-arc@lists.infradead.org>\0" "\00:1\0" "b\0" "Thanks for response.\n" "My comments are given inline below.\n" "\n" - "On Wed, 2017-01-25@19:25 +0200, Andy Shevchenko wrote:\n" - "> On Wed, 2017-01-25@18:34 +0300, Eugeniy Paltsev wrote:\n" - "> >?\n" + "On Wed, 2017-01-25 at 19:25 +0200, Andy Shevchenko wrote:\n" + "> On Wed, 2017-01-25 at 18:34 +0300, Eugeniy Paltsev wrote:\n" + "> >\302\240\n" "> > This patch adds support for the DW AXI DMAC controller.\n" - "> >?\n" + "> >\302\240\n" "> > DW AXI DMAC is a part of upcoming development board from Synopsys.\n" - "> >?\n" + "> >\302\240\n" "> > In this driver implementation only DMA_MEMCPY and DMA_SG transfers\n" "> > are supported.\n" - "> >?\n" + "> >\302\240\n" "> Few more comments on top of not addressed/answered yet.\n" - ">?\n" - "> >?\n" + ">\302\240\n" + "> >\302\240\n" "> > +static inline void axi_chan_disable(struct axi_dma_chan *chan)\n" "> > +{\n" "> > +\tu32 val;\n" "> > +\n" "> > +\tval = axi_dma_ioread32(chan->chip, DMAC_CHEN);\n" "> > +\tval &= ~(BIT(chan->id) << DMAC_CHAN_EN_SHIFT);\n" - "> > +\tval |=??(BIT(chan->id) << DMAC_CHAN_EN_WE_SHIFT);\n" + "> > +\tval |=\302\240\302\240(BIT(chan->id) << DMAC_CHAN_EN_WE_SHIFT);\n" "> > +\taxi_dma_iowrite32(chan->chip, DMAC_CHEN, val);\n" "> > +}\n" "> > +\n" @@ -38,18 +44,18 @@ "> > +\tu32 val;\n" "> > +\n" "> > +\tval = axi_dma_ioread32(chan->chip, DMAC_CHEN);\n" - "> >?\n" + "> >\302\240\n" "> > +\tval |= (BIT(chan->id) << DMAC_CHAN_EN_SHIFT |\n" "> > +\t\tBIT(chan->id) << DMAC_CHAN_EN_WE_SHIFT);\n" - "> >?\n" + "> >\302\240\n" "> Redundant parens.\n" "Will be fixed.\n" "\n" - "> >?\n" + "> >\302\240\n" "> > +\taxi_dma_iowrite32(chan->chip, DMAC_CHEN, val);\n" "> > +}\n" - ">?\n" - "> >?\n" + ">\302\240\n" + "> >\302\240\n" "> > +static void axi_desc_put(struct axi_dma_desc *desc)\n" "> > +{\n" "> > +\tstruct axi_dma_chan *chan = desc->chan;\n" @@ -57,7 +63,7 @@ "> > +\tstruct axi_dma_desc *child, *_next;\n" "> > +\tunsigned int descs_put = 0;\n" "> > +\n" - "> >?\n" + "> >\302\240\n" "> > +\tif (unlikely(!desc))\n" "> > +\t\treturn;\n" "> Would it be the case?\n" @@ -66,28 +72,28 @@ "\n" "Also about your previous question about likely/unlikely:\n" "I checked disassembly - gcc generates different code when I use likely\n" - "and unlikely. So, I guess they are useful.?\n" + "and unlikely. So, I guess they are useful.\302\240\n" "\n" - "> >?\n" + "> >\302\240\n" "> > +static void dma_chan_issue_pending(struct dma_chan *dchan)\n" "> > +{\n" "> > +\tstruct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan);\n" "> > +\tunsigned long flags;\n" "> > +\n" - "> >?\n" + "> >\302\240\n" "> > +\tdev_dbg(dchan2dev(dchan), \"%s: %s\\n\", __func__,\n" "> > axi_chan_name(chan));\n" "> Messages like this kinda redundant.\n" "> Either you use function tracer and see them anyway, or you are using\n" "> Dynamic Debug, which may include function name.\n" - ">?\n" + ">\302\240\n" "> Basically you wrote an equivalent to something like\n" - ">?\n" + ">\302\240\n" "> dev_dbg(dev, \"%s\\n\", channame);\n" - "Agreed. I'll remove dev_dbg from here because I also have it in?\n" + "Agreed. I'll remove dev_dbg from here because I also have it in\302\240\n" "axi_chan_start_first_queued.\n" "\n" - "> >?\n" + "> >\302\240\n" "> > +\n" "> > +\tspin_lock_irqsave(&chan->vc.lock, flags);\n" "> > +\tif (vchan_issue_pending(&chan->vc))\n" @@ -95,10 +101,10 @@ "> > +\tspin_unlock_irqrestore(&chan->vc.lock, flags);\n" "> ...and taking into account the function itself one might expect\n" "> something useful printed in _start_first_queued().\n" - ">?\n" + ">\302\240\n" "> For some cases there is also dev_vdbg().\n" - ">?\n" - "> >?\n" + ">\302\240\n" + "> >\302\240\n" "> > +}\n" "> > +\n" "> > +static int dma_chan_alloc_chan_resources(struct dma_chan *dchan)\n" @@ -112,23 +118,23 @@ "> > +\t\treturn -EBUSY;\n" "> > +\t}\n" "> > +\n" - "> >?\n" + "> >\302\240\n" "> > +\tdev_dbg(dchan2dev(dchan), \"%s: allocating\\n\",\n" "> > axi_chan_name(chan));\n" "> Can you try to enable DMADEVICES_DEBUG and VDEBUG and see what is\n" "> useful\n" "> and what is not?\n" - ">?\n" + ">\302\240\n" "> Give a chance to function tracer as well.\n" "Yep, this dev_dbg is redundant, so I'll remove it.\n" "\n" - "> >?\n" + "> >\302\240\n" "> > +static void dma_chan_free_chan_resources(struct dma_chan *dchan)\n" "> > +{\n" "> > +\tstruct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan);\n" "> > +\n" - "> >?\n" - "> >?\n" + "> >\302\240\n" + "> >\302\240\n" "> > +\t/* ASSERT: channel is idle */\n" "> > +\tif (axi_chan_is_hw_enable(chan))\n" "> > +\t\tdev_err(dchan2dev(dchan), \"%s is non-idle!\\n\",\n" @@ -140,18 +146,18 @@ "> >> > +static inline bool axi_dma_is_sw_enable(struct axi_dma_chip\n" "*chip)\n" "> >> > +{\n" - "> >> > +???struct dw_axi_dma *dw = chip->dw;\n" - "> >> > +???u32 i;\n" + "> >> > +\302\240\302\240\302\240struct dw_axi_dma *dw = chip->dw;\n" + "> >> > +\302\240\302\240\302\240u32 i;\n" "> >> > +\n" - "> >> > +???for (i = 0; i < dw->hdata->nr_channels; i++) {\n" - "> >> > +???????????if (dw->chan[i].in_use)\n" + "> >> > +\302\240\302\240\302\240for (i = 0; i < dw->hdata->nr_channels; i++) {\n" + "> >> > +\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240if (dw->chan[i].in_use)\n" "> >> Hmm... I know why we have such flag in dw_dmac, but I doubt it's\n" "> >> needed\n" "> >> in this driver. Just double check the need of it.\n" "> > I use this flag to check state of channel (used now/unused) to\n" "disable\n" "> > dmac if all channels are unused for now.\n" - ">?\n" + ">\302\240\n" "> Okay, but wouldn't be easier to use runtime PM for that? You will not\n" "> need any special counter and runtime PM will take case of\n" "> enabling/disabling the device.\n" @@ -162,7 +168,7 @@ "Also all existing SoCs with this DMAC don't support power management -\n" "so there is no really profit from implementing PM.\n" "\n" - "> >?\n" + "> >\302\240\n" "> > +\n" "> > +/* TODO: REVISIT: how we should choose AXI master for mem-to-mem\n" "> > transfer? */\n" @@ -173,23 +179,23 @@ "access to memory. But it isn't necessarily true for future SoCs.\n" "But possibly I shouldn't think about it now.\n" "\n" - "> >?\n" + "> >\302\240\n" "> > +\twhile (true) {\n" "> Usually it makes readability harder and rises a red flag to the code.\n" "I can write something like next code. Not sure is it looks better.\n" "----------->8------------------\n" "while ((dst_len || dst_sg && dst_nents) &&\n" - "? ? ? ?(src_len || src_sg && src_nents)) {\n" + "\302\240 \302\240 \302\240 \302\240(src_len || src_sg && src_nents)) {\n" "...\n" "----------->8------------------\n" "\n" - "> >?\n" - "> >?\t\t/* Manage transfer list (xfer_list) */\n" + "> >\302\240\n" + "> >\302\240\t\t/* Manage transfer list (xfer_list) */\n" "> > +\t\tif (!first) {\n" "> > +\t\t\tfirst = desc;\n" "> > +\t\t} else {\n" "> > +\t\t\tlist_add_tail(&desc->xfer_list, &first-\n" - "> > >?\n" + "> > >\302\240\n" "> > > xfer_list);\n" "> > +\t\t\twrite_desc_llp(prev, desc->vd.tx.phys |\n" "> > lms);\n" @@ -220,17 +226,17 @@ "> for the DMA device.\n" "In my opinion it is driver responsibility, not the caller.\n" "\n" - "> >?\n" + "> >\302\240\n" "> > +\n" "> > +static struct dma_async_tx_descriptor *\n" "> > +dma_chan_prep_dma_memcpy(struct dma_chan *dchan, dma_addr_t dest,\n" - "> > +\t\t\t?dma_addr_t src, size_t len, unsigned long\n" + "> > +\t\t\t\302\240dma_addr_t src, size_t len, unsigned long\n" "> > flags)\n" "> > +{\n" "> Do you indeed have a use case for this except debugging and testing?\n" "At least for IO coherency engine testing.\n" "\n" - "> >?\n" + "> >\302\240\n" "> > +\tunsigned int nents = 1;\n" "> > +\tstruct scatterlist dst_sg;\n" "> > +\tstruct scatterlist src_sg;\n" @@ -247,18 +253,18 @@ "> > +\t/* Implement memcpy transfer as sg transfer with single\n" "> > list\n" "> > */\n" - "> >?\n" + "> >\302\240\n" "> > +\treturn dma_chan_prep_dma_sg(dchan, &dst_sg, nents,\n" - "> > +\t\t\t\t????&src_sg, nents, flags);\n" + "> > +\t\t\t\t\302\240\302\240\302\240\302\240&src_sg, nents, flags);\n" "> One line?\n" "It will be more than 80 character.\n" "\n" - "> >?\n" + "> >\302\240\n" "> > +}\n" - "> >?\n" + "> >\302\240\n" "> > +\n" "> > +static void axi_chan_dump_lli(struct axi_dma_chan *chan,\n" - "> > +\t\t\t??????struct axi_dma_desc *desc)\n" + "> > +\t\t\t\302\240\302\240\302\240\302\240\302\240\302\240struct axi_dma_desc *desc)\n" "> > +{\n" "> > +\tdev_err(dchan2dev(&chan->vc.chan),\n" "> > +\t\t\"SAR: 0x%x DAR: 0x%x LLP: 0x%x BTS 0x%x CTL:\n" @@ -273,9 +279,9 @@ "> IP) can implement MMIO tracer.\n" "But we had to use such code until it happens.\n" "\n" - "> >?\n" + "> >\302\240\n" "> > +}\n" - "> >?\n" + "> >\302\240\n" "> > +\t\taxi_chan_dump_lli(chan, desc);\n" "> > +}\n" "> > +\n" @@ -288,11 +294,11 @@ "\taxi_chan_dump_lli(chan, desc);\n" "----------->8------------------\n" "\t\n" - "> >?\n" + "> >\302\240\n" "> > +static void axi_chan_handle_err(struct axi_dma_chan *chan, u32\n" "> > status)\n" "> > +{\n" - "> >?\n" + "> >\302\240\n" "> > +\n" "> > +static int dma_chan_pause(struct dma_chan *dchan)\n" "> > +{\n" @@ -310,27 +316,27 @@ "> Redundant parens.\n" "Will be fixed.\n" "\n" - "> >?\n" + "> >\302\240\n" "> > +\taxi_dma_iowrite32(chan->chip, DMAC_CHEN, val);\n" "> > +\n" "> > +\twhile (timeout--) {\n" - "> >?\n" + "> >\302\240\n" "> > +\t\tif (axi_chan_irq_read(chan) &\n" "> > DWAXIDMAC_IRQ_SUSPENDED) {\n" - "> >?\n" + "> >\302\240\n" "> > +\t\t\taxi_chan_irq_clear(chan,\n" "> > DWAXIDMAC_IRQ_SUSPENDED);\n" "> You may move this invariant out from the loop. Makes code cleaner.\n" "Agreed.\n" "\n" - "> >?\n" + "> >\302\240\n" "> > +\t\t\tret = 0;\n" "> > +\t\t\tbreak;\n" "> > +\t\t}\n" - "> >?\n" + "> >\302\240\n" "> > +\t\tudelay(2);\n" - ">?\n" - "> >?\n" + ">\302\240\n" + "> >\302\240\n" "> > +\t}\n" "> > +\n" "> > +\tchan->is_paused = true;\n" @@ -340,7 +346,7 @@ "> with\n" "> descriptors. So, that makes channel(paused) == active\n" "> descriptor(paused).\n" - ">?\n" + ">\302\240\n" "> The trick allows to make code cleaner.\n" "\n" "I don't understand how we can use use descriptor status for it.\n" @@ -352,7 +358,7 @@ "\treturn DMA_PAUSED;\n" "----------->8------------------\n" "\n" - "> >?\n" + "> >\302\240\n" "> > +\tret = device_property_read_u32_array(dev, \"priority\",\n" "> > carr,\n" "> I'm not sure you will have a use case for that. Have you?\n" @@ -361,20 +367,20 @@ "But we need priority management for slave dma (I'll implement slave dma\n" "api soon)\n" "\n" - "> >?\n" + "> >\302\240\n" "> > +\tret = devm_request_irq(chip->dev, chip->irq,\n" "> > dw_axi_dma_intretupt,\n" - "> > +\t\t\t???????IRQF_SHARED, DRV_NAME, chip);\n" + "> > +\t\t\t\302\240\302\240\302\240\302\240\302\240\302\240\302\240IRQF_SHARED, DRV_NAME, chip);\n" "> > +\tif (ret)\n" "> > +\t\treturn ret;\n" "> You can't do this unless you are using threaded IRQ handler without\n" "> any\n" "> tasklets involved.\n" - ">?\n" + ">\302\240\n" "> There was a nice mail by Thomas who explained what the problem you\n" "> have\n" "> there.\n" - ">?\n" + ">\302\240\n" "> It's a bug in your code.\n" "Yep, thanks a lot.\n" "Looks like I have problem with driver remove function.\n" @@ -390,7 +396,7 @@ "}\n" "----------->8------------------\n" "\n" - "> >?\n" + "> >\302\240\n" "> > +static int __init dw_init(void)\n" "> > +{\n" "> > +\treturn platform_driver_register(&dw_driver);\n" @@ -399,21 +405,21 @@ "> Hmm... Why it can't be just a platform driver? You describe the\n" "> dependency in Device Tree, if you have something happened, you may\n" "> utilize -EPROBE_DEFER.\n" - ">?\n" + ">\302\240\n" "Will be fixed.\n" "\n" "\n" - "> >??* Add DT bindings\n" - "> >?\n" + "> >\302\240\302\240* Add DT bindings\n" + "> >\302\240\n" "> > Eugeniy Paltsev (2):\n" - "> >???dt-bindings: Document the Synopsys DW AXI DMA bindings\n" - "> >???dmaengine: Add DW AXI DMAC driver\n" - "> >?\n" - "> >??.../devicetree/bindings/dma/snps,axi-dw-dmac.txt???|???33 +\n" - "> >??drivers/dma/Kconfig????????????????????????????????|????8 +\n" - "> >??drivers/dma/Makefile???????????????????????????????|????1 +\n" - "> >?\n" - "> >??drivers/dma/axi_dma_platform.c?????????????????????| 1060\n" + "> >\302\240\302\240\302\240dt-bindings: Document the Synopsys DW AXI DMA bindings\n" + "> >\302\240\302\240\302\240dmaengine: Add DW AXI DMAC driver\n" + "> >\302\240\n" + "> >\302\240\302\240.../devicetree/bindings/dma/snps,axi-dw-dmac.txt\302\240\302\240\302\240|\302\240\302\240\302\24033 +\n" + "> >\302\240\302\240drivers/dma/Kconfig\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240|\302\240\302\240\302\240\302\2408 +\n" + "> >\302\240\302\240drivers/dma/Makefile\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240|\302\240\302\240\302\240\302\2401 +\n" + "> >\302\240\n" + "> >\302\240\302\240drivers/dma/axi_dma_platform.c\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240| 1060\n" "> > ++++++++++++++++++++\n" "> This surprises me. I would expect more then 100+ LOC reduction when\n" "> switched to virt-dma API. Can you double check that you are using it\n" @@ -424,19 +430,19 @@ "\n" "Also about previous comment, which I missed before:\n" "> > +static u32 axi_chan_get_xfer_width(struct axi_dma_chan *chan,\n" - "> >????????????????????dma_addr_t src, dma_addr_t dst, size_t len)\n" + "> >\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240dma_addr_t src, dma_addr_t dst, size_t len)\n" "> > +{\n" "> > +\tu32 width;\n" "> > +\tdma_addr_t sdl = (src | dst | len);\n" "> > +\tu32 max_width = chan->chip->dw->hdata->m_data_width;\n" - ">?\n" + ">\302\240\n" "> Use reverse tree.\n" - ">?\n" + ">\302\240\n" "> dma_addr_t use above is wrong. (size_t might be bigger in some cases)\n" "\n" "What do you mean by the phrase \"Use reverse tree\" ?\n" "\n" - "--?\n" - ?Eugeniy Paltsev + "--\302\240\n" + "\302\240Eugeniy Paltsev" -fc6a30cead11dca3c6e970fafc9ce2828371b4afd487a01565d370fcfa9c8de3 +f709cc4115ad5115f67d90389c8e2879f16d1528662cce09b0f8426d08b0916e
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.