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

[Eugeniy Paltsev]

On Mon, 2018-02-26 at 18:42 +0200, Andy Shevchenko wrote:
> On Mon, Feb 26, 2018 at 4:56 PM, Eugeniy Paltsev
>  wrote:
> 
> > +   chip->core_clk = devm_clk_get(chip->dev, "core-clk");
> 
> Does the name come from datasheet?
> 
> > +   chip->cfgr_clk = devm_clk_get(chip->dev, "cfgr-clk");
> 
> Ditto?

Yes, these names came from datasheet.

> > +   }
> > +   /* Enable clk before accessing to registers */
> > +   clk_prepare_enable(chip->cfgr_clk);
> > +   clk_prepare_enable(chip->core_clk);
> 
> Each of them may fail. Is it okay?

As it is driver remove callback I guess it is okay.

> > +static const struct dev_pm_ops dw_axi_dma_pm_ops = {
> > +   SET_RUNTIME_PM_OPS(axi_dma_runtime_suspend, axi_dma_runtime_resume, 
> > NULL)
> > +};
> 
> No system suspend?

Are you talking about SET_SYSTEM_SLEEP_PM_OPS?
I don't implement these OPS as we don't have any platform which has DW AXI DMAC
and supports sleeping - so I can't test them.

-- 
 Eugeniy Paltsev
___
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

[Eugeniy Paltsev]

On Mon, 2018-02-26 at 18:42 +0200, Andy Shevchenko wrote:
> On Mon, Feb 26, 2018 at 4:56 PM, Eugeniy Paltsev
>  wrote:
> > +static int parse_device_properties(struct axi_dma_chip *chip)
> > +{
> > +   ret = device_property_read_u32(dev, "snps,dma-masters", );
> 
> Why it has prefix?
> 
> > +   ret = device_property_read_u32(dev, "snps,data-width", );
> 
> Ditto.
> 
> > +   ret = device_property_read_u32_array(dev, "snps,block-size", carr,
> > +chip->dw->hdata->nr_channels);
> 
> (perhaps this one can be moved outside of local namespace)
> 
> > +   ret = device_property_read_u32_array(dev, "snps,priority", carr,
> > +chip->dw->hdata->nr_channels);
> 
> Can you use just "priority"?
> 
> 

Rob Herring asked me to add vendor prefix to this properties.
See http://patchwork.ozlabs.org/patch/719708/

>8--
> > +Required properties:
> > +- compatible: "snps,axi-dma"
> 
> Too generic. This needs an IP version at least.
> 
> > +- 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.
> > +- dma-masters: Number of AXI masters supported by the hardware.
> > +- data-width: Maximum AXI data width supported by hardware.
> > +  (0 - 8bits, 1 - 16bits, 2 - 32bits, ..., 6 - 512bits)
> 
> > +- priority: Priority of channel. Array property. Priority value must be
> > +  programmed within [0:dma-channels-1] range. (0 - minimum priority)
> > +- block-size: Maximum block size supported by the controller channel. Array
> > +  property.
> 
> Other than dma-channels, all these either need vendor prefix.
> 
>8--



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:
> > +dmac: dma-controller@8 {
> > +   compatible = "snps,axi-dma-1.01a";
> 
> do we need "snps here..?
> 

Ditto.

-- 
 Eugeniy Paltsev
___
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 1/2] dmaengine: Introduce DW AXI DMAC driver

[Andy Shevchenko]

On Mon, Feb 26, 2018 at 4:56 PM, Eugeniy Paltsev
 wrote:
> This patch adds 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.

> +/*
> + * Synopsys DesignWare AXI DMA Controller driver.
> + *
> + * Copyright (C) 2017-2018 Synopsys, Inc. (www.synopsys.com)
> + * Author: Eugeniy Paltsev 
> + *

> + * SPDX-License-Identifier:GPL-2.0

According to license-rules.rst it should be first line in the file
with C++ style comment.

> + */

> +static inline void
> +axi_chan_iowrite64(struct axi_dma_chan *chan, u32 reg, u64 val)
> +{
> +   /*
> +* We split one 64 bit write for two 32 bit write as some HW doesn't
> +* support 64 bit access.
> +*/

> +   iowrite32((u32)val, chan->chan_regs + reg);
> +   iowrite32(val >> 32, chan->chan_regs + reg + 4);

upper_32_bits()
lower_32_bits()

?

> +}

> +static int dma_chan_pause(struct dma_chan *dchan)
> +{
> +   struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan);
> +   unsigned long flags;
> +   unsigned int timeout = 20; /* timeout iterations */

> +   int ret = -EAGAIN;

Perhaps,

int ret;
...

> +   u32 val;
> +
> +   spin_lock_irqsave(>vc.lock, flags);
> +
> +   val = axi_dma_ioread32(chan->chip, DMAC_CHEN);
> +   val |= BIT(chan->id) << DMAC_CHAN_SUSP_SHIFT |
> +  BIT(chan->id) << DMAC_CHAN_SUSP_WE_SHIFT;
> +   axi_dma_iowrite32(chan->chip, DMAC_CHEN, val);
> +

> +   do  {
> +   if (axi_chan_irq_read(chan) & DWAXIDMAC_IRQ_SUSPENDED) {
> +   ret = 0;
> +   break;
> +   }

...
if (...)
 break;
...

> +   udelay(2);
> +   } while (--timeout);

...
ret = timeout ? 0 : -EAGAIN;

?

> +
> +   axi_chan_irq_clear(chan, DWAXIDMAC_IRQ_SUSPENDED);
> +
> +   chan->is_paused = true;
> +
> +   spin_unlock_irqrestore(>vc.lock, flags);
> +
> +   return ret;
> +}

> +/* pm_runtime callbacks */

Noise.

> +#ifdef CONFIG_PM

__maybe_unused

> +static int axi_dma_runtime_suspend(struct device *dev)
> +{
> +   struct axi_dma_chip *chip = dev_get_drvdata(dev);
> +
> +   return axi_dma_suspend(chip);
> +}
> +
> +static int axi_dma_runtime_resume(struct device *dev)
> +{
> +   struct axi_dma_chip *chip = dev_get_drvdata(dev);
> +
> +   return axi_dma_resume(chip);
> +}
> +#endif

> +static int parse_device_properties(struct axi_dma_chip *chip)
> +{

> +   ret = device_property_read_u32(dev, "snps,dma-masters", );

Why it has prefix?

> +   ret = device_property_read_u32(dev, "snps,data-width", );

Ditto.

> +   ret = device_property_read_u32_array(dev, "snps,block-size", carr,
> +chip->dw->hdata->nr_channels);

(perhaps this one can be moved outside of local namespace)

> +   ret = device_property_read_u32_array(dev, "snps,priority", carr,
> +chip->dw->hdata->nr_channels);

Can you use just "priority"?

> +   /* axi-max-burst-len is optional property */
> +   ret = device_property_read_u32(dev, "snps,axi-max-burst-len", );

"dma-burst-sz" ?

> +   chip->core_clk = devm_clk_get(chip->dev, "core-clk");

Does the name come from datasheet?

> +   chip->cfgr_clk = devm_clk_get(chip->dev, "cfgr-clk");

Ditto?

> +   for (i = 0; i < hdata->nr_channels; i++) {

> +   chan->id = (u8)i;

Hmm... Do you need explicit casting?

> +   }

> +   /* Enable clk before accessing to registers */
> +   clk_prepare_enable(chip->cfgr_clk);
> +   clk_prepare_enable(chip->core_clk);

Each of them may fail. Is it okay?

> +static const struct dev_pm_ops dw_axi_dma_pm_ops = {
> +   SET_RUNTIME_PM_OPS(axi_dma_runtime_suspend, axi_dma_runtime_resume, 
> NULL)
> +};

No system suspend?

> +/*
> + * Synopsys DesignWare AXI DMA Controller driver.
> + *
> + * Copyright (C) 2017-2018 Synopsys, Inc. (www.synopsys.com)
> + * Author: Eugeniy Paltsev 
> + *

> + * SPDX-License-Identifier:GPL-2.0

First line, but C style of the comment.

> + */

-- 
With Best Regards,
Andy Shevchenko

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