diff for duplicates of <1486673558.2133.441.camel@linux.intel.com> diff --git a/a/1.txt b/N1/1.txt index c144642..8ceb132 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,5 +1,5 @@ -On Thu, 2017-02-09@13:58 +0000, Eugeniy Paltsev wrote: -?? +On Thu, 2017-02-09 at 13:58 +0000, Eugeniy Paltsev wrote: + > > > +static void axi_desc_put(struct axi_dma_desc *desc) > > > +{ > > > + struct axi_dma_chan *chan = desc->chan; @@ -7,7 +7,7 @@ On Thu, 2017-02-09@13:58 +0000, Eugeniy Paltsev wrote: > > > + struct axi_dma_desc *child, *_next; > > > + unsigned int descs_put = 0; > > > + -> > > ? +> > > > > > + if (unlikely(!desc)) > > > + return; > > @@ -18,14 +18,14 @@ On Thu, 2017-02-09@13:58 +0000, Eugeniy Paltsev wrote: > > 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. They are, but in rare cases. I assume you have read the following article and other material on LWN. https://lwn.net/Articles/420019/ -? + > > > + /* ASSERT: channel is idle */ > > > + if (axi_chan_is_hw_enable(chan)) > > > + dev_err(dchan2dev(dchan), "%s is non-idle!\n", @@ -40,11 +40,11 @@ https://lwn.net/Articles/420019/ > > *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 @@ -55,7 +55,7 @@ https://lwn.net/Articles/420019/ > 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 @@ -86,17 +86,17 @@ Besides Vinod I would suggest Tony Lindgren, for example. > 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)) { Just give a thought about it once more. It might be not easy, but the result should be quite better than "while (true)" approach. -> > > ? /* 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 | @@ -157,7 +157,7 @@ See above. > > 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. @@ -194,17 +194,17 @@ So, either way would be better than current approach in this driver. Yes, it will fix it. -> > > ??* 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 @@ -220,15 +220,15 @@ Ah, that explains, indeed. > 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) > @@ -251,5 +251,5 @@ u32 width; But pay attention to your sdl, which is always nonzero. -- -Andy Shevchenko <andriy.shevchenko at linux.intel.com> +Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy diff --git a/a/content_digest b/N1/content_digest index 2fcdbd8..8090979 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -2,14 +2,20 @@ "ref\01485358457-22957-3-git-send-email-Eugeniy.Paltsev@synopsys.com\0" "ref\01485365106.2133.331.camel@linux.intel.com\0" "ref\01486648722.2657.18.camel@synopsys.com\0" - "From\0andriy.shevchenko@linux.intel.com (Andy Shevchenko)\0" - "Subject\0[PATCH 2/2] dmaengine: Add DW AXI DMAC driver\0" + "From\0Andy Shevchenko <andriy.shevchenko@linux.intel.com>\0" + "Subject\0Re: [PATCH 2/2] dmaengine: Add DW AXI DMAC driver\0" "Date\0Thu, 09 Feb 2017 22:52:38 +0200\0" - "To\0linux-snps-arc@lists.infradead.org\0" + "To\0Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.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" - "On Thu, 2017-02-09@13:58 +0000, Eugeniy Paltsev wrote:\n" - "??\n" + "On Thu, 2017-02-09 at 13:58 +0000, Eugeniy Paltsev wrote:\n" + "\302\240\302\240\n" "> > > +static void axi_desc_put(struct axi_dma_desc *desc)\n" "> > > +{\n" "> > > +\tstruct axi_dma_chan *chan = desc->chan;\n" @@ -17,7 +23,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" "> > \n" @@ -28,14 +34,14 @@ "> \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" "They are, but in rare cases.\n" "\n" "I assume you have read the following article and other material on LWN.\n" "\n" "https://lwn.net/Articles/420019/\n" - "?\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" @@ -50,11 +56,11 @@ "> \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" "> > > > \n" "> > > > Hmm... I know why we have such flag in dw_dmac, but I doubt it's\n" "> > > > needed\n" @@ -65,7 +71,7 @@ "> disable\n" "> > > dmac if all channels are unused for now.\n" "> > \n" - "> > ?\n" + "> > \302\240\n" "> > Okay, but wouldn't be easier to use runtime PM for that? You will\n" "> > not\n" "> > need any special counter and runtime PM will take case of\n" @@ -96,17 +102,17 @@ "> 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" "Just give a thought about it once more. It might be not easy, but the\n" "result should be quite better than \"while (true)\" approach.\n" "\n" - "> > > ?\t\t/* Manage transfer list (xfer_list) */\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" "> > > \n" "> > > +\t\t\twrite_desc_llp(prev, desc->vd.tx.phys |\n" @@ -167,7 +173,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" @@ -204,17 +210,17 @@ "\n" "Yes, it will fix it.\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" "> > \n" "> > This surprises me. I would expect more then 100+ LOC reduction when\n" @@ -230,15 +236,15 @@ "\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" - "> > ?\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\n" "> > cases)\n" "> \n" @@ -261,7 +267,7 @@ "But pay attention to your sdl, which is always nonzero.\n" "\n" "-- \n" - "Andy Shevchenko <andriy.shevchenko at linux.intel.com>\n" + "Andy Shevchenko <andriy.shevchenko@linux.intel.com>\n" Intel Finland Oy -2aa32c82fd73a2b5dfa8aa81d070fef15f9000e30812df97ada6cc2e07a91f79 +6c481f1e0fa5a5f4e9a51a6d81e7b8a1dca7cfad80e95b374e3c1f163df09e59
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.