| 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
| ||
|
Date: Fri, 20 Jan 2017 15:38:29 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Eugeniy Paltsev <Eugeniy.Paltsev@...opsys.com>,
dmaengine@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Vinod Koul <vinod.koul@...el.com>,
Dan Williams <dan.j.williams@...el.com>,
Alexey Brodkin <Alexey.Brodkin@...opsys.com>
Subject: Re: [RFC] dmaengine: Add DW AXI DMAC driver
On Fri, 2017-01-20 at 13:50 +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.
>
> In this driver implementation only DMA_MEMCPY and DMA_SG transfers
> are supported.
>
> Note: there is no DT documentation in this patch yet, but it will
> be added in the nearest future.
First of all, please use virt-dma API.
Second, don't look to dw_dmac for examples, it's not a good one to be
learned from.
Some mostly style comments below.
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/bitops.h>
> +#include <linux/platform_device.h>
> +#include <linux/delay.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmapool.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_dma.h>
> +#include <linux/err.h>
Alphabetical order?
Check if you need all of them.
If you are going to use this driver as a library I would recommend to do
it as a library in the first place.
> +/*
> + * The set of bus widths supported by the DMA controller. DW AXI DMAC
> supports
> + * master data bus width up to 512 bits (for both AXI master
> interfaces), but
> + * it depends on IP block configurarion.
> + */
> +#define AXI_DMA_BUSWIDTHS \
> + (DMA_SLAVE_BUSWIDTH_UNDEFINED | \
> + DMA_SLAVE_BUSWIDTH_1_BYTE | \
> + DMA_SLAVE_BUSWIDTH_2_BYTES | \
> + DMA_SLAVE_BUSWIDTH_4_BYTES | \
> + DMA_SLAVE_BUSWIDTH_8_BYTES | \
> + DMA_SLAVE_BUSWIDTH_16_BYTES | \
> + DMA_SLAVE_BUSWIDTH_32_BYTES | \
> + DMA_SLAVE_BUSWIDTH_64_BYTES)
> +/* TODO: check: do we need to use BIT() macro here? */
Yes, and here is the problem of that API.
> +
> +/* NOTE: DW AXI DMAC theoretically can be configured with BE IO */
Do this as ops in your channel or chip struct.
+static inline void axi_dma_disable(struct axi_dma_chip *chip)
> +{
> + u32 val = axi_dma_ioread32(chip, DMAC_CFG);
> + val &= ~DMAC_EN_MASK;
> + axi_dma_iowrite32(chip, DMAC_CFG, val);
Better to use
u32 val;
val = read();
val &= y;
write(val);
pattern.
Same for similar places.
> +static inline void axi_chan_irq_disable(struct axi_dma_chan *chan,
> u32 irq_mask)
> +{
> + u32 val;
> +
> + if (likely(irq_mask == DWAXIDMAC_IRQ_ALL)) {
I doubt likely() is useful here anyhow. Have you looked into assembly?
Does it indeed do what it's supposed to?
> +static inline void axi_chan_disable(struct axi_dma_chan *chan)
> +{
> + u32 val = axi_dma_ioread32(chan->chip, DMAC_CHEN);
> + val &= ~((1 << chan->id) << DMAC_CHAN_EN_SHIFT);
> + val |= ((1 << chan->id) << DMAC_CHAN_EN_WE_SHIFT);
You talked somewhere of a BIT macro, here it is one
val &= ~BIT(chan->id + DMAC_CHAN_EN_SHIFT);
or
val &= ~(BIT(chan->id) << DMAC_CHAN_EN_SHIFT);
whatever suits better.
Check all code for this.
> +static inline bool axi_dma_is_sw_enable(struct axi_dma_chip *chip)
> +{
> + struct dw_axi_dma *dw = chip->dw;
> + u32 i;
> +
> + for (i = 0; i < dw->hdata->nr_channels; i++) {
> + if (dw->chan[i].in_use)
Hmm... I know why we have such flag in dw_dmac, but I doubt it's needed
in this driver. Just double check the need of it.
> + return true;
> + }
> +
> + return false;
> +}
>
> +static u32 axi_chan_get_xfer_width(struct axi_dma_chan *chan,
> dma_addr_t src,
> + dma_addr_t dst, size_t len)
> +{
> + u32 width;
> + dma_addr_t sdl = (src | dst | len);
> + u32 max_width = chan->chip->dw->hdata->m_data_width;
Use reverse tree.
dma_addr_t use above is wrong. (size_t might be bigger in some cases)
> +
> + width = sdl ? __ffs(sdl) : DWAXIDMAC_TRANS_WIDTH_MAX;
> +
> + return width <= max_width ? width : max_width;
min() / min_t()
> +}
> +static void write_desc_sar(struct axi_dma_desc *desc, dma_addr_t adr)
> +{
> + desc->lli.sar_lo = cpu_to_le32(adr);
> +}
Perhaps macros for all them? Choose whatever looks and suits better.
> +/* TODO: REVISIT: how we should choose AXI master for mem-to-mem
> transfer? */
> +static void set_desc_dest_master(struct axi_dma_desc *desc)
> +{
> + u32 val;
> +
> + /* select AXI1 for source master if available*/
Fix indentation, capitalize it.
> +}
> +
> +static int dw_probe(struct platform_device *pdev)
> +{
> + struct axi_dma_chip *chip;
> + struct resource *mem;
> + struct dw_axi_dma *dw;
> + struct dw_axi_dma_hcfg *hdata;
> + u32 i;
> + int ret;
> +
> + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> + if (!chip)
> + return -ENOMEM;
> +
> + dw = devm_kzalloc(&pdev->dev, sizeof(*dw), GFP_KERNEL);
> + if (!dw)
> + return -ENOMEM;
> +
> + hdata = devm_kzalloc(&pdev->dev, sizeof(*hdata), GFP_KERNEL);
> + if (!hdata)
> + return -ENOMEM;
> +
> + chip->dw = dw;
> + chip->dev = &pdev->dev;
> + chip->dw->hdata = hdata;
> +
> + chip->irq = platform_get_irq(pdev, 0);
> + if (chip->irq < 0)
> + return chip->irq;
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + chip->regs = devm_ioremap_resource(chip->dev, mem);
> + if (IS_ERR(chip->regs))
> + return PTR_ERR(chip->regs);
> +
> + ret = dma_coerce_mask_and_coherent(chip->dev,
> DMA_BIT_MASK(32));
It will not work for 64 bits, it will not work for other users of this
driver if any (when you have different DMA mask to be set).
> + if (ret)
> + return ret;
> +
> + chip->clk = devm_clk_get(chip->dev, NULL);
> + if (IS_ERR(chip->clk))
> + return PTR_ERR(chip->clk);
> +
> + ret = parse_device_properties(chip);
> + if (ret)
> + return ret;
> +
> + dw->chan = devm_kcalloc(chip->dev, hdata->nr_channels,
> + sizeof(*dw->chan), GFP_KERNEL);
> + if (!dw->chan)
> + return -ENOMEM;
> +
> + ret = devm_request_irq(chip->dev, chip->irq,
> dw_axi_dma_intretupt,
> + IRQF_SHARED, DRV_NAME, chip);
> + if (ret)
> + return ret;
> +
>
> +
> + INIT_LIST_HEAD(&dw->dma.channels);
> + for (i = 0; i < hdata->nr_channels; i++) {
> + struct axi_dma_chan *chan = &dw->chan[i];
> +
> + chan->chip = chip;
> + chan->chan.device = &dw->dma;
> + dma_cookie_init(&chan->chan);
> + list_add_tail(&chan->chan.device_node, &dw-
> >dma.channels);
> +
>
> + chan->id = (u8)i;
This duplicates what you have in struct dma_chan
> + chan->priority = (u8)i;
> + chan->direction = DMA_TRANS_NONE;
> +
> +
> + dw->dma.device_alloc_chan_resources =
> dma_chan_alloc_chan_resources;
> + dw->dma.device_free_chan_resources =
> dma_chan_free_chan_resources;
> +
> + dw->dma.device_prep_dma_memcpy = dma_chan_prep_dma_memcpy;
> + dw->dma.device_prep_dma_sg = dma_chan_prep_dma_sg;
> +
> + ret = clk_prepare_enable(chip->clk);
> + if (ret < 0)
> + return ret;
> +
> + ret = dma_async_device_register(&dw->dma);
// offtopic
Perhaps someone can eventually introduce devm_ variant of this?
// offtopic
> + if (ret)
> + goto err_clk_disable;
> +
> +MODULE_DESCRIPTION("Synopsys DesignWare AXI DMA Controller platform
> driver");
> +MODULE_AUTHOR("Paltsev Eugeniy <Eugeniy.Paltsev@...opsys.com>");
FirstName LastName <email@...ress.com> ?
--
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Intel Finland Oy
Powered by blists - more mailing lists