Messages in this thread | | | From | Andy Shevchenko <> | Date | Mon, 26 Feb 2018 18:42:59 +0200 | Subject | Re: [PATCH v2 1/2] dmaengine: Introduce DW AXI DMAC driver |
| |
On Mon, Feb 26, 2018 at 4:56 PM, Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com> 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 <Eugeniy.Paltsev@synopsys.com> > + *
> + * 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(&chan->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(&chan->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", &tmp);
Why it has prefix?
> + ret = device_property_read_u32(dev, "snps,data-width", &tmp);
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", &tmp);
"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 <Eugeniy.Paltsev@synopsys.com> > + *
> + * SPDX-License-Identifier: GPL-2.0
First line, but C style of the comment.
> + */
-- With Best Regards, Andy Shevchenko
| |