Re: [PATCH 2/2] dmaengine: Add DW AXI DMAC driver

[Vinod Koul]

On Wed, Jan 25, 2017 at 06:34:17PM +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.

How different is AXI from DW Synopsys?

Is the spec publicly available?

> +config AXI_DW_DMAC
> + tristate "Synopsys DesignWare AXI DMA support"
> + depends on OF && !64BIT

why not 64 bit, can you also add compile test

> +#define AXI_DMA_BUSWIDTHS  \
> + (DMA_SLAVE_BUSWIDTH_UNDEFINED   | \

DMA_SLAVE_BUSWIDTH_UNDEFINED??

> +static irqreturn_t dw_axi_dma_intretupt(int irq, void *dev_id)
> +{
> + struct axi_dma_chip *chip = dev_id;
> +
> + /* Disable DMAC inerrupts. We'll enable them in the tasklet */
> + axi_dma_irq_disable(chip);
> +
> + tasklet_schedule(>dw->tasklet);

This is very inefficient, we would want to submit next txn here and not in
tasklet

> +static void axi_chan_block_xfer_complete(struct axi_dma_chan *chan)
> +{
> + struct virt_dma_desc *vd;
> + unsigned long flags;
> +
> + spin_lock_irqsave(>vc.lock, flags);
> + if (unlikely(axi_chan_is_hw_enable(chan))) {
> + dev_err(chan2dev(chan), "BUG: %s catched DWAXIDMAC_IRQ_DMA_TRF, 
> but channel not idle!\n",
> + axi_chan_name(chan));
> + axi_chan_disable(chan);
> + }
> +
> + /* The completed descriptor currently is in the head of vc list */
> + vd = vchan_next_desc(>vc);
> + /* Remove the completed descriptor from issued list before completing */
> + list_del(>node);
> + vchan_cookie_complete(vd);

this should be called from irq handler


> +static int dw_remove(struct platform_device *pdev)
> +{
> + struct axi_dma_chip *chip = platform_get_drvdata(pdev);
> + struct dw_axi_dma *dw = chip->dw;
> + struct axi_dma_chan *chan, *_chan;
> + u32 i;
> +
> + axi_dma_irq_disable(chip);
> + for (i = 0; i < dw->hdata->nr_channels; i++) {
> + axi_chan_disable(>dw->chan[i]);
> + axi_chan_irq_disable(>dw->chan[i], DWAXIDMAC_IRQ_ALL);
> + }
> + axi_dma_disable(chip);
> +
> + tasklet_kill(>tasklet);
> +
> + list_for_each_entry_safe(chan, _chan, >dma.channels,
> + vc.chan.device_node) {
> + list_del(>vc.chan.device_node);
> + tasklet_kill(>vc.task);
> + }

very nice :)

But please freeup irq as well

> +module_platform_driver(dw_driver);
> +
> +static int __init dw_init(void)
> +{
> + return platform_driver_register(_driver);
> +}
> +subsys_initcall(dw_init);

why subsys_initcall??

> +/* Common registers offset */
> +#define DMAC_ID  0x000 // R DMAC ID
> +#define DMAC_COMPVER 0x008 // R DMAC Component Version
> +#define DMAC_CFG 0x010 // R/W DMAC Configuration
> +#define DMAC_CHEN0x018 // R/W DMAC Channel Enable
> +#define DMAC_CHEN_L  0x018 // R/W DMAC Channel Enable 00-31
> +#define DMAC_CHEN_H  0x01C // R/W DMAC Channel Enable 32-63
> +#define DMAC_INTSTATUS   0x030 // R DMAC Interrupt Status
> +#define DMAC_COMMON_INTCLEAR 0x038 // W DMAC Interrupt Clear
> +#define DMAC_COMMON_INTSTATUS_ENA 0x040 // R DMAC Interrupt Status Enable
> +#define DMAC_COMMON_INTSIGNAL_ENA 0x048 // R/W DMAC Interrupt Signal Enable
> +#define DMAC_COMMON_INTSTATUS0x050 // R DMAC Interrupt Status
> +#define DMAC_RESET   0x058 // R DMAC Reset Register1

DMAC is a generic term, AX_DMAC and no C99 style comment, checkpatch would
have cribbed

Use BITS and GENMASK here

> +
> +/* DMA channel registers offset */
> +#define CH_SAR   0x000 // R/W Chan Source Address
> +#define CH_DAR   0x008 // R/W Chan Destination Address
> +#define CH_BLOCK_TS  0x010 // R/W Chan Block Transfer Size
> +#define CH_CTL   0x018 // R/W Chan Control
> +#define CH_CTL_L 0x018 // R/W Chan Control 00-31
> +#define CH_CTL_H 0x01C // R/W Chan Control 32-63
> +#define CH_CFG   0x020 // R/W Chan Configuration
> +#define CH_CFG_L 0x020 // R/W Chan Configuration 00-31
> +#define CH_CFG_H 0x024 // R/W Chan Configuration 32-63
> +#define CH_LLP   0x028 // R/W Chan Linked List Pointer
> +#define CH_STATUS0x030 // R Chan Status
> +#define CH_SWHSSRC   0x038 // R/W Chan SW Handshake Source
> +#define CH_SWHSDST   0x040 // R/W Chan SW Handshake Destination
> +#define CH_BLK_TFR_RESUMEREQ 0x048 // W Chan Block Transfer Resume Req
> +#define CH_AXI_ID0x050 // R/W Chan AXI ID
> +#define CH_AXI_QOS   0x058 // R/W Chan AXI QOS
> +#define CH_SSTAT 0x060 // R Chan Source Status
> +#define CH_DSTAT 0x068 // R Chan Destination Status
> +#define CH_SSTATAR   0x070 // R/W Chan Source Status Fetch Addr
> +#define CH_DSTATAR   0x078 // R/W Chan Destination Status Fetch Addr

Re: [PATCH] dmaengine: dmatest: Restore "memcpy" as default mode

[Vinod Koul]

On Tue, Oct 18, 2016 at 11:14:08AM -0700, Dave Jiang wrote:
> On 09/15/2016 08:48 AM, Vinod Koul wrote:
> > On Wed, Sep 14, 2016 at 08:40:38PM +0300, Eugeniy Paltsev wrote:
> >> Commit 0d4cb44da6ca0e8 ("dmaengine: dmatest: Add support for
> >> scatter-gather DMA mode") changes default "dmatest" behavior by
> >> changing default mode from "memcpy" to "scatter-gather".
> >> Now "memcpy" gets back as default mode.
> > 
> > Applied, thanks
> 
> Did that go into stable? It's breaking the default expected behavior in
> 4.8 without the fix.

No it wasnt, and it should have...

I send to stable and lokks like it should applied on 4.7 and 4.8.
Commit 0d4cb44da6ca0e8 ("dmaengine: dmatest: Add support for scatter-gather
DMA mode") came in 4.7 IIUC..

Thanks
-- 
~Vinod

___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Re: [PATCH v3 0/2] DW DMAC: update device tree

[Vinod Koul]

On Mon, Nov 21, 2016 at 12:37:06PM +0200, Andy Shevchenko wrote:
> On Mon, 2016-11-21 at 10:02 +, Alexey Brodkin wrote:
> > Hi Andy,
> > 
> > On Fri, 2016-11-18 at 21:26 +0200, Andy Shevchenko wrote:
> > > On Fri, 2016-11-18 at 22:12 +0300, Eugeniy Paltsev wrote:
> > > > 
> > > > It wasn't possible to enable some features like
> > > > memory-to-memory transfers or multi block transfers via DT.
> > > > It is fixed by these patches.
> > > 
> > > First of all, please, give time to reviewers to comment the patches.
> > > Usually it should be at least 24h (for the series that has been sent
> > > first time 1 week approximately).
> > 
> > I'm not really sure a lot of people get disturbed by this series
> > and given this all has been discussed for months now I'd really like
> > to see changes required for our HW to work to land in upstream ASAP.
> 
> I understand your concern, I'm often in the same position in many areas,
> including this driver (I'm not a maintainer of slave DMA subsystem).
> 
> Though let's face the issues we have with the series:
> - stuff regarding to style and alike (would be fixed in a day)
> - DTS naming and conventions, this is apparently a big area, where I
> might share opinion, but can't decide for
> - last word by the subsystem maintainer
> 
> > Too bad we're late for 4.9 (which is supposed to be the next LTS) but
> > > we need to make sure this series hits 4.10 for sure.
> 
> Vinod, is it possible to get in for this series (if we get Ack from DT
> people)?

We still have a week or so... But holding race agaisnt upstream is a bad
idea... Doesnt work that way.

-- 
~Vinod

___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Re: [PATCH v3 2/2] DW DMAC: add multi-block property to device tree

[Vinod Koul]

On Fri, Nov 18, 2016 at 09:33:13PM +0200, Andy Shevchenko wrote:
> > @@ -1569,7 +1569,7 @@ int dw_dma_probe(struct dw_dma_chip *chip)
> >     (dwc_params >> DWC_PARAMS_MBLK_EN &
> > 0x1) == 0;
> >     } else {
> >     dwc->block_size = pdata->block_size;
> > -   dwc->nollp = pdata->is_nollp;
> > +   dwc->nollp = pdata->multi_block[i];
> 
> You missed the point. You assign positive value to negative variable.
> It's a bug. Have you tested this? How?
> 
> In case of positive property you have to update DTS. By the way, I'm
> pretty sure that spare13xx boards has auto configuration enabled, though
> it has to be checked with vendor (I assume you may have fast response
> from them).

Yeah why are we not using auto configuration here would be the first
question..

-- 
~Vinod

___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Re: [PATCH v6 0/2] DW DMAC: update device tree

[Vinod Koul]

On Fri, Nov 25, 2016 at 05:59:05PM +0300, Eugeniy Paltsev wrote:
> It wasn't possible to enable some features like
> memory-to-memory transfers or multi block transfers via DT.
> It is fixed by these patches.

Applied after adding substem name tag.

-- 
~Vinod

___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Re: [PATCH v1 2/2] dmaengine: Add DW AXI DMAC driver

[Vinod Koul]

On Tue, Feb 21, 2017 at 11:38:04PM +0300, Eugeniy Paltsev wrote:

> +static struct axi_dma_desc *axi_desc_get(struct axi_dma_chan *chan)
> +{
> + struct dw_axi_dma *dw = chan->chip->dw;
> + struct axi_dma_desc *desc;
> + dma_addr_t phys;
> +
> + desc = dma_pool_zalloc(dw->desc_pool, GFP_ATOMIC, );

GFP_NOWAIT please

> +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, >xfer_list, xfer_list) {
> + list_del(>xfer_list);
> + dma_pool_free(dw->desc_pool, child, child->vd.tx.phys);
> + descs_put++;
> + }
> +
> + dma_pool_free(dw->desc_pool, desc, desc->vd.tx.phys);
> + descs_put++;
> +
> + chan->descs_allocated -= descs_put;

why not decrement chan->descs_allocated?

> +
> + dev_vdbg(chan2dev(chan), "%s: %d descs put, %d still allocated\n",
> + axi_chan_name(chan), descs_put, chan->descs_allocated);
> +}
> +
> +static void vchan_desc_put(struct virt_dma_desc *vdesc)
> +{
> + axi_desc_put(vd_to_axi_desc(vdesc));
> +}

well this has no logic and becomes a dummy fn, so why do we need it.

> +
> +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;

no residue calculation?

> +static void axi_chan_start_first_queued(struct axi_dma_chan *chan)
> +{
> + struct axi_dma_desc *desc;
> + struct virt_dma_desc *vd;
> +
> + vd = vchan_next_desc(>vc);

unnecessary empty line

> +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(>vc);
> +
> + dev_vdbg(dchan2dev(dchan), "%s: %s: descriptor still allocated: %u\n",
> + __func__, axi_chan_name(chan), chan->descs_allocated);
> +
> + pm_runtime_put_sync_suspend(chan->chip->dev);

any reason why sync variant is used?

> +static struct dma_async_tx_descriptor *
> +dma_chan_prep_dma_sg(struct dma_chan *dchan,
> +  struct scatterlist *dst_sg, unsigned int dst_nents,
> +  struct scatterlist *src_sg, unsigned int src_nents,
> +  unsigned long flags)
> +{
> + struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan);
> + struct axi_dma_desc *first = NULL, *desc = NULL, *prev = NULL;
> + size_t dst_len = 0, src_len = 0, xfer_len = 0;
> + dma_addr_t dst_adr = 0, src_adr = 0;
> + u32 src_width, dst_width;
> + size_t block_ts, max_block_ts;
> + u32 reg;
> + u8 lms = 0;
> +
> + dev_vdbg(chan2dev(chan), "%s: %s: sn: %d dn: %d flags: 0x%lx",
> + __func__, axi_chan_name(chan), src_nents, dst_nents, flags);
> +
> + if (unlikely(dst_nents == 0 || src_nents == 0))
> + return NULL;
> +
> + if (unlikely(dst_sg == NULL || src_sg == NULL))
> + return NULL;
> +
> + max_block_ts = chan->chip->dw->hdata->block_size[chan->id];
> +
> + /*
> +  * Loop until there is either no more source or no more destination
> +  * scatterlist entry.
> +  */
> + while ((dst_len || (dst_sg && dst_nents)) &&
> +(src_len || (src_sg && src_nents))) {
> + if (dst_len == 0) {
> + dst_adr = sg_dma_address(dst_sg);
> + dst_len = sg_dma_len(dst_sg);
> +
> + dst_sg = sg_next(dst_sg);
> + dst_nents--;
> + }
> +
> + /* Process src sg list */
> + if (src_len == 0) {
> + src_adr = sg_dma_address(src_sg);
> + src_len = sg_dma_len(src_sg);
> +
> + src_sg = sg_next(src_sg);
> + src_nents--;
> + }
> +
> + /* Min of src and dest length will be this xfer length */
> + xfer_len = min_t(size_t, src_len, dst_len);
> + if (xfer_len == 0)
> + continue;
> +
> + /* Take care for the alignment */
> + src_width = axi_chan_get_xfer_width(chan, src_adr,
> + dst_adr, xfer_len);
> + /*
> +  * Actually src_width and dst_width can be different, but 

Re: [PATCH] Allow to use DMA_CTRL_REUSE flag for all channel types

[Vinod Koul]

On Tue, May 02, 2017 at 03:16:18PM +, Eugeniy Paltsev wrote:
> Hi Vinod,
> 
> On Mon, 2017-05-01 at 11:21 +0530, Vinod Koul wrote:
> > On Fri, Apr 28, 2017 at 04:37:46PM +0300, Eugeniy Paltsev wrote:
> > > In the current implementation dma_get_slave_caps is used to check
> > > state of descriptor_reuse option. But dma_get_slave_caps includes
> > > check if the channel supports slave transactions.
> > > So DMA_CTRL_REUSE flag can be set (even for MEM-TO-MEM tranfers)
> > > only if channel supports slave transactions.
> > > 
> > > Now we can use DMA_CTRL_REUSE flag for all channel types.
> > > Also it allows to test reusing mechanism with simply mem-to-mem dma
> > > test.
> > 
> > We do not want to allow that actually. Slave is always treated as a
> > special
> > case, so resue was allowed.
> > 
> > With memcpy the assumptions are different and clients can do reuse.
> 
> Could you please clarify why don't we want to allow use DMA_CTRL_REUSE
> for mem-to-mem transfers?
> 
> Reusing of mem-to-mem (MEMCPY and DMA_SG) descriptors will work fine on
> virt-dma based drivers.

Precisely, the client does not know if you have a virt-dma or some other
kind if implementation

For them they see a channel and use it!

> Anyway the current implementation behaviour is quite strange:
> If channel supports *slave* transfers DMA_CTRL_REUSE can be set to
> slave and *mem-to-mem* transfers.
> 
> And, of course, we can pass DMA_CTRL_REUSE flag to device_prep_dma_sg
> or device_prep_dma_memcpy directly without checks.

Yeah thats bad, do send a patch to forbid that..

-- 
~Vinod

___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Re: [PATCH v3 0/2] Introduce DW AXI DMAC driver

[Vinod Koul]

On Tue, Mar 06, 2018 at 02:46:13PM +0300, Eugeniy Paltsev wrote:
> This patch series add support for the DW AXI DMAC controller.
> 
> DW AXI DMAC is a part of HSDK development board from Synopsys.
> 
> In this driver implementation only DMA_MEMCPY transfers
> are supported.

Applied, thanks

-- 
~Vinod

___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Re: [PATCH v2 2/2] dt-bindings: Document the Synopsys DW AXI DMA bindings

[Vinod Koul]

On Fri, Mar 02, 2018 at 08:32:20AM +, Alexey Brodkin wrote:
> Hi Vinod,
> 
> On Fri, 2018-03-02 at 13:44 +0530, Vinod Koul wrote:
> > On Mon, Feb 26, 2018 at 05:56:28PM +0300, Eugeniy Paltsev wrote:
> > > This patch adds documentation of device tree bindings for the Synopsys
> > > DesignWare AXI DMA controller.
> > > 
> > > Signed-off-by: Eugeniy Paltsev <eugeniy.palt...@synopsys.com>
> > > ---
> > >  .../devicetree/bindings/dma/snps,dw-axi-dmac.txt   | 41 
> > > ++
> > >  1 file changed, 41 insertions(+)
> > >  create mode 100644 
> > > Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.txt 
> > > b/Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.txt
> > > new file mode 100644
> > > index 000..f237b79
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.txt
> > > @@ -0,0 +1,41 @@
> > > +Synopsys DesignWare AXI DMA Controller
> > > +
> > > +Required properties:
> > > +- compatible: "snps,axi-dma-1.01a"
> > > +- reg: Address range of the DMAC registers. This should include
> > > +  all of the per-channel registers.
> > > +- interrupt: Should contain the DMAC interrupt number.
> > > +- interrupt-parent: Should be the phandle for the interrupt controller
> > > +  that services interrupts for this device.
> > > +- dma-channels: Number of channels supported by hardware.
> > > +- snps,dma-masters: Number of AXI masters supported by the hardware.
> > > +- snps,data-width: Maximum AXI data width supported by hardware.
> > > +  (0 - 8bits, 1 - 16bits, 2 - 32bits, ..., 6 - 512bits)
> > > +- snps,priority: Priority of channel. Array size is equal to the number 
> > > of
> > > +  dma-channels. Priority value must be programmed within 
> > > [0:dma-channels-1]
> > > +  range. (0 - minimum priority)
> > > +- snps,block-size: Maximum block size supported by the controller 
> > > channel.
> > > +  Array size is equal to the number of dma-channels.
> > > +
> > > +Optional properties:
> > > +- snps,axi-max-burst-len: Restrict master AXI burst length by value 
> > > specified
> > > +  in this property. If this property is missing the maximum AXI burst 
> > > length
> > > +  supported by DMAC is used. [1:256]
> > > +
> > > +Example:
> > > +
> > > +dmac: dma-controller@8 {
> > > + compatible = "snps,axi-dma-1.01a";
> > 
> > do we need "snps here..?
> 
> Synopsys is this IP-block vendor so shouldn't we put it that way?

Not a DT expert but why should vendor name come here, you can read the
properties from device node, vendor name seems redundant to me

-- 
~Vinod

___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Re: [PATCH v2 1/2] dmaengine: Introduce DW AXI DMAC driver

[Vinod Koul]

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(>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", );
> + 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", );

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

___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Re: [PATCH v2 2/2] dt-bindings: Document the Synopsys DW AXI DMA bindings

[Vinod Koul]

On Mon, Feb 26, 2018 at 05:56:28PM +0300, Eugeniy Paltsev wrote:
> This patch adds documentation of device tree bindings for the Synopsys
> DesignWare AXI DMA controller.
> 
> Signed-off-by: Eugeniy Paltsev 
> ---
>  .../devicetree/bindings/dma/snps,dw-axi-dmac.txt   | 41 
> ++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.txt
> 
> diff --git a/Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.txt 
> b/Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.txt
> new file mode 100644
> index 000..f237b79
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.txt
> @@ -0,0 +1,41 @@
> +Synopsys DesignWare AXI DMA Controller
> +
> +Required properties:
> +- compatible: "snps,axi-dma-1.01a"
> +- reg: Address range of the DMAC registers. This should include
> +  all of the per-channel registers.
> +- interrupt: Should contain the DMAC interrupt number.
> +- interrupt-parent: Should be the phandle for the interrupt controller
> +  that services interrupts for this device.
> +- dma-channels: Number of channels supported by hardware.
> +- snps,dma-masters: Number of AXI masters supported by the hardware.
> +- snps,data-width: Maximum AXI data width supported by hardware.
> +  (0 - 8bits, 1 - 16bits, 2 - 32bits, ..., 6 - 512bits)
> +- snps,priority: Priority of channel. Array size is equal to the number of
> +  dma-channels. Priority value must be programmed within [0:dma-channels-1]
> +  range. (0 - minimum priority)
> +- snps,block-size: Maximum block size supported by the controller channel.
> +  Array size is equal to the number of dma-channels.
> +
> +Optional properties:
> +- snps,axi-max-burst-len: Restrict master AXI burst length by value specified
> +  in this property. If this property is missing the maximum AXI burst length
> +  supported by DMAC is used. [1:256]
> +
> +Example:
> +
> +dmac: dma-controller@8 {
> + compatible = "snps,axi-dma-1.01a";

do we need "snps here..?

> + reg = <0x8 0x400>;
> + clocks = <_clk>, <_clk>;
> + clock-names = "core-clk", "cfgr-clk";
> + interrupt-parent = <>;
> + interrupts = <27>;
> +
> + dma-channels = <4>;
> + snps,dma-masters = <2>;
> + snps,data-width = <3>;
> + snps,block-size = <4096 4096 4096 4096>;
> + snps,priority = <0 1 2 3>;
> + snps,axi-max-burst-len = <16>;
> +};
> -- 
> 2.9.3
> 

-- 
~Vinod

___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc