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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75Vc6KtppecYJBtuXTGCzXySOUZiYqxBfb6w69f1j3J0jDQ@mail.gmail.com>
Date:   Mon, 26 Feb 2018 18:42:59 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Eugeniy Paltsev <Eugeniy.Paltsev@...opsys.com>
Cc:     dmaengine <dmaengine@...r.kernel.org>,
        linux-snps-arc@...ts.infradead.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Vinod Koul <vinod.koul@...el.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Rob Herring <robh+dt@...nel.org>,
        Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@...el.com>,
        Alexey Brodkin <Alexey.Brodkin@...opsys.com>
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@...opsys.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@...opsys.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@...opsys.com>
> + *

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

First line, but C style of the comment.

> + */

-- 
With Best Regards,
Andy Shevchenko