From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: [v2,1/2] dmaengine: Introduce DW AXI DMAC driver From: Vinod Koul Message-Id: <20180302081326.GJ15443@localhost> Date: Fri, 2 Mar 2018 13:43:26 +0530 To: Eugeniy Paltsev Cc: dmaengine@vger.kernel.org, linux-snps-arc@lists.infradead.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, Dan Williams , Rob Herring , Wan Ahmad Zainie , Alexey Brodkin , Andy Shevchenko List-ID: T24gTW9uLCBGZWIgMjYsIDIwMTggYXQgMDU6NTY6MjdQTSArMDMwMCwgRXVnZW5peSBQYWx0c2V2 IHdyb3RlOgoKPiArLyoKPiArICogU3lub3BzeXMgRGVzaWduV2FyZSBBWEkgRE1BIENvbnRyb2xs ZXIgZHJpdmVyLgo+ICsgKgo+ICsgKiBDb3B5cmlnaHQgKEMpIDIwMTctMjAxOCBTeW5vcHN5cywg SW5jLiAod3d3LnN5bm9wc3lzLmNvbSkKPiArICogQXV0aG9yOiBFdWdlbml5IFBhbHRzZXYgPEV1 Z2VuaXkuUGFsdHNldkBzeW5vcHN5cy5jb20+Cj4gKyAqCj4gKyAqIFNQRFgtTGljZW5zZS1JZGVu dGlmaWVyOglHUEwtMi4wCj4gKyAqLwoKVGhpcyBuZWVkcyB0byBiZSBpbiBiZWxvdyBzdHlsZQoK Ly8gU1BEWC1MaWNlbnNlLUlkZW50aWZpZXI6ICBHUEwtMi4wCi8vIENvcHlyaWdodCAoQykgLi4u LgoKLyoKICogb3RoZXIgdGhpbmdzIHUgd2FudCB0byBhZGQKICovCgo+ICsjZGVmaW5lIERSVl9O QU1FCSJheGlfZHdfZG1hYyIKCmhvdyBhYm91dCB1c2UgS0JVSUxEX01PRE5BTUUgaGVyZT8KCj4g K3N0YXRpYyBlbnVtIGRtYV9zdGF0dXMKPiArZG1hX2NoYW5fdHhfc3RhdHVzKHN0cnVjdCBkbWFf Y2hhbiAqZGNoYW4sIGRtYV9jb29raWVfdCBjb29raWUsCj4gKwkJICBzdHJ1Y3QgZG1hX3R4X3N0 YXRlICp0eHN0YXRlKQo+ICt7Cj4gKwlzdHJ1Y3QgYXhpX2RtYV9jaGFuICpjaGFuID0gZGNoYW5f dG9fYXhpX2RtYV9jaGFuKGRjaGFuKTsKPiArCWVudW0gZG1hX3N0YXR1cyByZXQ7Cj4gKwo+ICsJ cmV0ID0gZG1hX2Nvb2tpZV9zdGF0dXMoZGNoYW4sIGNvb2tpZSwgdHhzdGF0ZSk7Cj4gKwo+ICsJ aWYgKGNoYW4tPmlzX3BhdXNlZCAmJiByZXQgPT0gRE1BX0lOX1BST0dSRVNTKQo+ICsJCXJldHVy biBETUFfUEFVU0VEOwo+ICsKPiArCXJldHVybiByZXQ7CgpuaXRwaWNrLCBob3cgYWJvdXQKCglp ZiAoY2hhbi0+aXNfcGF1c2VkICYmIHJldCA9PSBETUFfSU5fUFJPR1JFU1MpCgkJcmV0ID0gRE1B X1BBVVNFRDsKCglyZXR1cm4gcmV0OwoKdGhhdCB3YXkgd2UgaGF2ZSBzaW5nbGUgcG9pbnQgb2Yg cmV0dXJuLgoKPiArc3RhdGljIGludCBkbWFfY2hhbl9hbGxvY19jaGFuX3Jlc291cmNlcyhzdHJ1 Y3QgZG1hX2NoYW4gKmRjaGFuKQo+ICt7Cj4gKwlzdHJ1Y3QgYXhpX2RtYV9jaGFuICpjaGFuID0g ZGNoYW5fdG9fYXhpX2RtYV9jaGFuKGRjaGFuKTsKPiArCj4gKwkvKiBBU1NFUlQ6IGNoYW5uZWwg aXMgaWRsZSAqLwo+ICsJaWYgKGF4aV9jaGFuX2lzX2h3X2VuYWJsZShjaGFuKSkgewo+ICsJCWRl dl9lcnIoY2hhbjJkZXYoY2hhbiksICIlcyBpcyBub24taWRsZSFcbiIsCj4gKwkJCWF4aV9jaGFu X25hbWUoY2hhbikpOwo+ICsJCXJldHVybiAtRUJVU1k7Cj4gKwl9Cj4gKwo+ICsJZGV2X3ZkYmco ZGNoYW4yZGV2KGRjaGFuKSwgIiVzOiBhbGxvY2F0aW5nXG4iLCBheGlfY2hhbl9uYW1lKGNoYW4p KTsKPiArCj4gKwlkbWFfY29va2llX2luaXQoZGNoYW4pOwoKVGhpcyBpcyBhbHJlYWR5IGRvbmUg YXMgcGFydCBvZiB2Y2hhbl9pbml0KCksIHNvIHdoeSBpcyBpdCBjYWxsZWQgZXhwbGljaXRseQph bmQgd2h5IGlzIHRoYXQgbm90IGludm9rZWQgaGVyZT8KCj4gK3N0YXRpYyB2b2lkIGRtYV9jaGFu X2ZyZWVfY2hhbl9yZXNvdXJjZXMoc3RydWN0IGRtYV9jaGFuICpkY2hhbikKPiArewo+ICsJc3Ry dWN0IGF4aV9kbWFfY2hhbiAqY2hhbiA9IGRjaGFuX3RvX2F4aV9kbWFfY2hhbihkY2hhbik7Cj4g Kwo+ICsJLyogQVNTRVJUOiBjaGFubmVsIGlzIGlkbGUgKi8KPiArCWlmIChheGlfY2hhbl9pc19o d19lbmFibGUoY2hhbikpCj4gKwkJZGV2X2VycihkY2hhbjJkZXYoZGNoYW4pLCAiJXMgaXMgbm9u LWlkbGUhXG4iLAo+ICsJCQlheGlfY2hhbl9uYW1lKGNoYW4pKTsKPiArCj4gKwlheGlfY2hhbl9k aXNhYmxlKGNoYW4pOwo+ICsJYXhpX2NoYW5faXJxX2Rpc2FibGUoY2hhbiwgRFdBWElETUFDX0lS UV9BTEwpOwo+ICsKPiArCXZjaGFuX2ZyZWVfY2hhbl9yZXNvdXJjZXMoJmNoYW4tPnZjKTsKPiAr Cj4gKwlkZXZfdmRiZyhkY2hhbjJkZXYoZGNoYW4pLCAiJXM6IGZyZWUgcmVzb3VyY2VzLCBkZXNj cmlwdG9yIHN0aWxsIGFsbG9jYXRlZDogJXVcbiIsCgptYWtpbmcgdGhlIHN0cmluZyBpbiBzdWJz ZXF1ZW50IGxpbmUgbWFrZSBpdCBsZXNzZXIgdGhhbiA4MCBjaGFycywgaGVyZSBhbmQKZmV3IG90 aGVyIHBsYWNlcyBjYW4gYmVuZWZpdC4uCgo+ICtzdGF0aWMgaXJxcmV0dXJuX3QgZHdfYXhpX2Rt YV9pbnRyZXR1cHQoaW50IGlycSwgdm9pZCAqZGV2X2lkKQovcy9pbnRyZXR1cHQvaW50ZXJydXB0 LyA/Cgo+ICtzdGF0aWMgaW50IGF4aV9kbWFfcmVzdW1lKHN0cnVjdCBheGlfZG1hX2NoaXAgKmNo aXApCj4gK3sKPiArCWludCByZXQgPSAwOwoKc3VwZXJmbHVvdXMgaW5pdAoKPiArc3RhdGljIGlu dCBwYXJzZV9kZXZpY2VfcHJvcGVydGllcyhzdHJ1Y3QgYXhpX2RtYV9jaGlwICpjaGlwKQo+ICt7 Cj4gKwlzdHJ1Y3QgZGV2aWNlICpkZXYgPSBjaGlwLT5kZXY7Cj4gKwl1MzIgdG1wLCBjYXJyW0RN QUNfTUFYX0NIQU5ORUxTXTsKPiArCWludCByZXQ7Cj4gKwo+ICsJcmV0ID0gZGV2aWNlX3Byb3Bl cnR5X3JlYWRfdTMyKGRldiwgImRtYS1jaGFubmVscyIsICZ0bXApOwo+ICsJaWYgKHJldCkKPiAr CQlyZXR1cm4gcmV0Owo+ICsJaWYgKHRtcCA9PSAwIHx8IHRtcCA+IERNQUNfTUFYX0NIQU5ORUxT KQo+ICsJCXJldHVybiAtRUlOVkFMOwo+ICsKPiArCWNoaXAtPmR3LT5oZGF0YS0+bnJfY2hhbm5l bHMgPSB0bXA7Cj4gKwo+ICsJcmV0ID0gZGV2aWNlX3Byb3BlcnR5X3JlYWRfdTMyKGRldiwgInNu cHMsZG1hLW1hc3RlcnMiLCAmdG1wKTsKCndoeSBpcyBpdCBzbnBzLC4uLiBzaG91bGRuJ3QgaXQg YmUgb25seSBkbWEtbWFzdGVycz8KCj4gK2VudW0gewo+ICsJRFdBWElETUFDX0FSV0xFTl8xCQk9 IDB4MCwKPiArCURXQVhJRE1BQ19BUldMRU5fMgkJPSAweDEsCj4gKwlEV0FYSURNQUNfQVJXTEVO XzQJCT0gMHgzLAo+ICsJRFdBWElETUFDX0FSV0xFTl84CQk9IDB4NywKPiArCURXQVhJRE1BQ19B UldMRU5fMTYJCT0gMHhGLAo+ICsJRFdBWElETUFDX0FSV0xFTl8zMgkJPSAweDFGLAo+ICsJRFdB WElETUFDX0FSV0xFTl82NAkJPSAweDNGLAo+ICsJRFdBWElETUFDX0FSV0xFTl8xMjgJCT0gMHg3 RiwKPiArCURXQVhJRE1BQ19BUldMRU5fMjU2CQk9IDB4RkYsCgpHRU5NQVNLKCkgZm9yIHRoZXNl Pwo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: vinod.koul@intel.com (Vinod Koul) Date: Fri, 2 Mar 2018 13:43:26 +0530 Subject: [PATCH v2 1/2] dmaengine: Introduce DW AXI DMAC driver In-Reply-To: <20180226145628.11892-2-Eugeniy.Paltsev@synopsys.com> References: <20180226145628.11892-1-Eugeniy.Paltsev@synopsys.com> <20180226145628.11892-2-Eugeniy.Paltsev@synopsys.com> List-ID: Message-ID: <20180302081326.GJ15443@localhost> To: linux-snps-arc@lists.infradead.org On Mon, Feb 26, 2018@05:56:27PM +0300, Eugeniy Paltsev wrote: > +/* > + * Synopsys DesignWare AXI DMA Controller driver. > + * > + * Copyright (C) 2017-2018 Synopsys, Inc. (www.synopsys.com) > + * Author: Eugeniy Paltsev > + * > + * SPDX-License-Identifier: GPL-2.0 > + */ This needs to be in below style // SPDX-License-Identifier: GPL-2.0 // Copyright (C) .... /* * other things u want to add */ > +#define DRV_NAME "axi_dw_dmac" how about use KBUILD_MODNAME here? > +static enum dma_status > +dma_chan_tx_status(struct dma_chan *dchan, dma_cookie_t cookie, > + struct dma_tx_state *txstate) > +{ > + struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan); > + enum dma_status ret; > + > + ret = dma_cookie_status(dchan, cookie, txstate); > + > + if (chan->is_paused && ret == DMA_IN_PROGRESS) > + return DMA_PAUSED; > + > + return ret; nitpick, how about if (chan->is_paused && ret == DMA_IN_PROGRESS) ret = DMA_PAUSED; return ret; that way we have single point of return. > +static int dma_chan_alloc_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(chan2dev(chan), "%s is non-idle!\n", > + axi_chan_name(chan)); > + return -EBUSY; > + } > + > + dev_vdbg(dchan2dev(dchan), "%s: allocating\n", axi_chan_name(chan)); > + > + dma_cookie_init(dchan); This is already done as part of vchan_init(), so why is it called explicitly and why is that not invoked here? > +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", > + axi_chan_name(chan)); > + > + axi_chan_disable(chan); > + axi_chan_irq_disable(chan, DWAXIDMAC_IRQ_ALL); > + > + vchan_free_chan_resources(&chan->vc); > + > + dev_vdbg(dchan2dev(dchan), "%s: free resources, descriptor still allocated: %u\n", making the string in subsequent line make it lesser than 80 chars, here and few other places can benefit.. > +static irqreturn_t dw_axi_dma_intretupt(int irq, void *dev_id) /s/intretupt/interrupt/ ? > +static int axi_dma_resume(struct axi_dma_chip *chip) > +{ > + int ret = 0; superfluous init > +static int parse_device_properties(struct axi_dma_chip *chip) > +{ > + struct device *dev = chip->dev; > + u32 tmp, carr[DMAC_MAX_CHANNELS]; > + int ret; > + > + ret = device_property_read_u32(dev, "dma-channels", &tmp); > + if (ret) > + return ret; > + if (tmp == 0 || tmp > DMAC_MAX_CHANNELS) > + return -EINVAL; > + > + chip->dw->hdata->nr_channels = tmp; > + > + ret = device_property_read_u32(dev, "snps,dma-masters", &tmp); why is it snps,... shouldn't it be only dma-masters? > +enum { > + DWAXIDMAC_ARWLEN_1 = 0x0, > + DWAXIDMAC_ARWLEN_2 = 0x1, > + DWAXIDMAC_ARWLEN_4 = 0x3, > + DWAXIDMAC_ARWLEN_8 = 0x7, > + DWAXIDMAC_ARWLEN_16 = 0xF, > + DWAXIDMAC_ARWLEN_32 = 0x1F, > + DWAXIDMAC_ARWLEN_64 = 0x3F, > + DWAXIDMAC_ARWLEN_128 = 0x7F, > + DWAXIDMAC_ARWLEN_256 = 0xFF, GENMASK() for these? -- ~Vinod From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vinod Koul Subject: Re: [PATCH v2 1/2] dmaengine: Introduce DW AXI DMAC driver Date: Fri, 2 Mar 2018 13:43:26 +0530 Message-ID: <20180302081326.GJ15443@localhost> References: <20180226145628.11892-1-Eugeniy.Paltsev@synopsys.com> <20180226145628.11892-2-Eugeniy.Paltsev@synopsys.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20180226145628.11892-2-Eugeniy.Paltsev@synopsys.com> Sender: linux-kernel-owner@vger.kernel.org To: Eugeniy Paltsev Cc: dmaengine@vger.kernel.org, linux-snps-arc@lists.infradead.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, Dan Williams , Rob Herring , Wan Ahmad Zainie , Alexey Brodkin , Andy Shevchenko List-Id: devicetree@vger.kernel.org On Mon, Feb 26, 2018 at 05:56:27PM +0300, Eugeniy Paltsev wrote: > +/* > + * Synopsys DesignWare AXI DMA Controller driver. > + * > + * Copyright (C) 2017-2018 Synopsys, Inc. (www.synopsys.com) > + * Author: Eugeniy Paltsev > + * > + * SPDX-License-Identifier: GPL-2.0 > + */ This needs to be in below style // SPDX-License-Identifier: GPL-2.0 // Copyright (C) .... /* * other things u want to add */ > +#define DRV_NAME "axi_dw_dmac" how about use KBUILD_MODNAME here? > +static enum dma_status > +dma_chan_tx_status(struct dma_chan *dchan, dma_cookie_t cookie, > + struct dma_tx_state *txstate) > +{ > + struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan); > + enum dma_status ret; > + > + ret = dma_cookie_status(dchan, cookie, txstate); > + > + if (chan->is_paused && ret == DMA_IN_PROGRESS) > + return DMA_PAUSED; > + > + return ret; nitpick, how about if (chan->is_paused && ret == DMA_IN_PROGRESS) ret = DMA_PAUSED; return ret; that way we have single point of return. > +static int dma_chan_alloc_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(chan2dev(chan), "%s is non-idle!\n", > + axi_chan_name(chan)); > + return -EBUSY; > + } > + > + dev_vdbg(dchan2dev(dchan), "%s: allocating\n", axi_chan_name(chan)); > + > + dma_cookie_init(dchan); This is already done as part of vchan_init(), so why is it called explicitly and why is that not invoked here? > +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", > + axi_chan_name(chan)); > + > + axi_chan_disable(chan); > + axi_chan_irq_disable(chan, DWAXIDMAC_IRQ_ALL); > + > + vchan_free_chan_resources(&chan->vc); > + > + dev_vdbg(dchan2dev(dchan), "%s: free resources, descriptor still allocated: %u\n", making the string in subsequent line make it lesser than 80 chars, here and few other places can benefit.. > +static irqreturn_t dw_axi_dma_intretupt(int irq, void *dev_id) /s/intretupt/interrupt/ ? > +static int axi_dma_resume(struct axi_dma_chip *chip) > +{ > + int ret = 0; superfluous init > +static int parse_device_properties(struct axi_dma_chip *chip) > +{ > + struct device *dev = chip->dev; > + u32 tmp, carr[DMAC_MAX_CHANNELS]; > + int ret; > + > + ret = device_property_read_u32(dev, "dma-channels", &tmp); > + if (ret) > + return ret; > + if (tmp == 0 || tmp > DMAC_MAX_CHANNELS) > + return -EINVAL; > + > + chip->dw->hdata->nr_channels = tmp; > + > + ret = device_property_read_u32(dev, "snps,dma-masters", &tmp); why is it snps,... shouldn't it be only dma-masters? > +enum { > + DWAXIDMAC_ARWLEN_1 = 0x0, > + DWAXIDMAC_ARWLEN_2 = 0x1, > + DWAXIDMAC_ARWLEN_4 = 0x3, > + DWAXIDMAC_ARWLEN_8 = 0x7, > + DWAXIDMAC_ARWLEN_16 = 0xF, > + DWAXIDMAC_ARWLEN_32 = 0x1F, > + DWAXIDMAC_ARWLEN_64 = 0x3F, > + DWAXIDMAC_ARWLEN_128 = 0x7F, > + DWAXIDMAC_ARWLEN_256 = 0xFF, GENMASK() for these? -- ~Vinod