| 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
| ||
|
Message-ID: <CAHp75VcJOxE9iwBJvhp6-TjuF85rLYT4O2F-wHNvDxQpEpMiGg@mail.gmail.com>
Date: Thu, 29 Oct 2015 23:03:37 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Jon Hunter <jonathanh@...dia.com>
Cc: Laxman Dewangan <ldewangan@...dia.com>,
Vinod Koul <vinod.koul@...el.com>,
Stephen Warren <swarren@...dotorg.org>,
Thierry Reding <thierry.reding@...il.com>,
Alexandre Courbot <gnurou@...il.com>,
Rob Herring <robh+dt@...nel.org>,
Pawel Moll <pawel.moll@....com>,
Mark Rutland <mark.rutland@....com>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Kumar Gala <galak@...eaurora.org>,
dmaengine <dmaengine@...r.kernel.org>,
"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V3 2/2] dmaengine: tegra-adma: Add support for Tegra210 ADMA
On Fri, Oct 16, 2015 at 10:35 AM, Jon Hunter <jonathanh@...dia.com> wrote:
> Add support for the Tegra210 Audio DMA controller that is used for
> transferring data between system memory and the Audio sub-system.
> The driver only supports cyclic transfers because this is being solely
> used for audio.
>
> This driver is based upon the work by Dara Ramesh <dramesh@...dia.com>.
>
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/mm.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_dma.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +#include <linux/clk/tegra.h>
Do we really need all of them present here?
> +
> +#include "dmaengine.h"
> +#include "virt-dma.h"
> +
> +#define ADMA_CH_CMD 0x00
> +#define ADMA_CH_STATUS 0x0c
> +#define ADMA_CH_STATUS_XFER_EN BIT(0)
> +
> +#define ADMA_CH_INT_STATUS 0x10
> +#define ADMA_CH_INT_STATUS_XFER_DONE BIT(0)
> +
> +#define ADMA_CH_INT_CLEAR 0x1c
> +#define ADMA_CH_CTRL 0x24
> +#define ADMA_CH_CTRL_TX_REQ(val) (((val) & 0xf) << 28)
> +#define ADMA_CH_CTRL_TX_REQ_MAX 10
> +#define ADMA_CH_CTRL_RX_REQ(val) (((val) & 0xf) << 24)
> +#define ADMA_CH_CTRL_RX_REQ_MAX 10
> +#define ADMA_CH_CTRL_DIR(val) (((val) & 0xf) << 12)
> +#define ADMA_CH_CTRL_DIR_AHUB2MEM 2
> +#define ADMA_CH_CTRL_DIR_MEM2AHUB 4
> +#define ADMA_CH_CTRL_MODE_CONTINUOUS (2 << 8)
> +#define ADMA_CH_CTRL_FLOWCTRL_EN BIT(1)
> +
> +#define ADMA_CH_CONFIG 0x28
> +#define ADMA_CH_CONFIG_SRC_BUF(val) (((val) & 0x7) << 28)
> +#define ADMA_CH_CONFIG_TRG_BUF(val) (((val) & 0x7) << 24)
> +#define ADMA_CH_CONFIG_BURST_SIZE(val) (((val) & 0x7) << 20)
> +#define ADMA_CH_CONFIG_BURST_16 5
> +#define ADMA_CH_CONFIG_WEIGHT_FOR_WRR(val) ((val) & 0xf)
> +#define ADMA_CH_CONFIG_MAX_BUFS 8
> +
> +#define ADMA_CH_FIFO_CTRL 0x2c
> +#define ADMA_CH_FIFO_CTRL_OVRFW_THRES(val) (((val) & 0xf) << 24)
> +#define ADMA_CH_FIFO_CTRL_STARV_THRES(val) (((val) & 0xf) << 16)
> +#define ADMA_CH_FIFO_CTRL_TX_SIZE(val) (((val) & 0xf) << 8)
> +#define ADMA_CH_FIFO_CTRL_RX_SIZE(val) ((val) & 0xf)
> +
> +#define ADMA_CH_TC_STATUS 0x30
> +#define ADMA_CH_LOWER_SRC_ADDR 0x34
> +#define ADMA_CH_LOWER_TRG_ADDR 0x3c
> +#define ADMA_CH_TC 0x44
> +#define ADMA_CH_TC_COUNT_MASK 0x3ffffffc
> +
> +#define ADMA_CH_XFER_STATUS 0x54
> +#define ADMA_CH_XFER_STATUS_COUNT_MASK 0xffff
> +
> +#define ADMA_GLOBAL_CMD 0xc00
> +#define ADMA_GLOBAL_SOFT_RESET 0xc04
> +#define ADMA_GLOBAL_INT_CLEAR 0xc20
> +#define ADMA_GLOBAL_CTRL 0xc24
> +
> +#define ADMA_CH_REG_OFFSET(a) (a * 0x80)
> +
> +#define ADMA_CH_FIFO_CTRL_DEFAULT (ADMA_CH_FIFO_CTRL_OVRFW_THRES(1) | \
> + ADMA_CH_FIFO_CTRL_STARV_THRES(1) | \
> + ADMA_CH_FIFO_CTRL_TX_SIZE(3) | \
> + ADMA_CH_FIFO_CTRL_RX_SIZE(3))
> +struct tegra_adma;
> +
> +/*
> + * struct tegra_adma_chip_data - Tegra chip specific data
> + * @nr_channels: Number of DMA channels available.
> + */
> +struct tegra_adma_chip_data {
> + int nr_channels;
> +};
> +
> +/*
> + * struct tegra_adma_chan_regs - Tegra ADMA channel registers
> + */
> +struct tegra_adma_chan_regs {
> + unsigned int ctrl;
> + unsigned int config;
> + unsigned int src_addr;
> + unsigned int trg_addr;
> + unsigned int fifo_ctrl;
> + unsigned int tc;
> +};
> +
> +/*
> + * struct tegra_adma_desc - Tegra ADMA descriptor to manage transfer requests.
> + */
> +struct tegra_adma_desc {
> + struct virt_dma_desc vd;
> + struct tegra_adma_chan_regs ch_regs;
> + unsigned long bytes_requested;
> + unsigned long bytes_transferred;
> +};
> +
> +/*
> + * struct tegra_adma_chan - Tegra ADMA channel information
> + */
> +struct tegra_adma_chan {
> + struct virt_dma_chan vc;
> + struct tegra_adma_desc *desc;
> + struct tegra_adma *tdma;
> + char name[30];
Is it default naming scheme not enough here?
> + int irq;
> + void __iomem *chan_addr;
> + spinlock_t lock;
Do the virtual channel's lock is not enough?
> +
> + /* Slave channel configuration info */
> + struct dma_slave_config sconfig;
> + bool sconfig_valid;
> + unsigned int sreq_dir;
> + unsigned int sreq_index;
> + bool sreq_reserved;
> +
> + /* Transfer count and position info */
> + unsigned int tx_buf_count;
> + unsigned int tx_buf_pos;
> +};
> +
> +/*
> + * struct tegra_adma - Tegra ADMA controller information
> + */
> +struct tegra_adma {
> + struct dma_device dma_dev;
> + struct device *dev;
> + struct clk *adma_clk;
> + void __iomem *base_addr;
> + unsigned int nr_channels;
> + unsigned long rx_requests_reserved;
> + unsigned long tx_requests_reserved;
> +
> + /* Used to store global command register state when suspending */
> + unsigned int global_cmd;
> +
> + /* Last member of the structure */
> + struct tegra_adma_chan channels[0];
> +};
> +
> +static inline void tdma_write(struct tegra_adma *tdma, u32 reg, u32 val)
> +{
> + writel(val, tdma->base_addr + reg);
> +}
> +
> +static inline u32 tdma_read(struct tegra_adma *tdma, u32 reg)
> +{
> + return readl(tdma->base_addr + reg);
> +}
> +
> +static inline void tdma_ch_write(struct tegra_adma_chan *tdc,
> + u32 reg, u32 val)
> +{
> + writel(val, tdc->chan_addr + reg);
> +}
> +
> +static inline u32 tdma_ch_read(struct tegra_adma_chan *tdc, u32 reg)
> +{
> + return readl(tdc->chan_addr + reg);
> +}
> +
> +static inline struct tegra_adma_chan *to_tegra_adma_chan(struct dma_chan *dc)
> +{
> + return container_of(dc, struct tegra_adma_chan, vc.chan);
> +}
> +
> +static inline struct tegra_adma_desc *to_tegra_adma_desc(
> + struct dma_async_tx_descriptor *td)
> +{
> + return container_of(td, struct tegra_adma_desc, vd.tx);
> +}
> +
> +static inline struct device *tdc2dev(struct tegra_adma_chan *tdc)
> +{
> + return tdc->tdma->dev;
> +}
> +
> +static void tegra_adma_desc_free(struct virt_dma_desc *vd)
> +{
> + kfree(container_of(vd, struct tegra_adma_desc, vd));
> +}
> +
> +static int tegra_adma_slave_config(struct dma_chan *dc,
> + struct dma_slave_config *sconfig)
> +{
> + struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
> +
> + memcpy(&tdc->sconfig, sconfig, sizeof(*sconfig));
> + tdc->sconfig_valid = true;
What kind of workflow may end up with wrong slave configuration?
> +
> + return 0;
> +}
> +
> +static int tegra_adma_init(struct tegra_adma *tdma)
> +{
> + u32 status;
> + int ret;
> +
> + /* Clear any interrupts */
> + tdma_write(tdma, ADMA_GLOBAL_INT_CLEAR, 0x1);
> +
> + /* Assert soft reset */
> + tdma_write(tdma, ADMA_GLOBAL_SOFT_RESET, 0x1);
> +
> + /* Wait for reset to clear */
> + ret = readx_poll_timeout(readl,
> + tdma->base_addr + ADMA_GLOBAL_SOFT_RESET,
> + status, status == 0, 20, 10000);
> + if (ret)
> + return ret;
> +
> + /* Enable global ADMA registers */
> + tdma_write(tdma, ADMA_GLOBAL_CMD, 1);
> +
> + return 0;
> +}
> +
> +static int tegra_adma_request_alloc(struct tegra_adma_chan *tdc,
> + unsigned int sreq_dir)
> +{
> + struct tegra_adma *tdma = tdc->tdma;
> + unsigned int sreq_index = tdc->sreq_index;
> +
> + if (tdc->sreq_reserved)
> + return tdc->sreq_dir == sreq_dir ? 0 : -EINVAL;
> +
> + switch (sreq_dir) {
> + case ADMA_CH_CTRL_DIR_MEM2AHUB:
> + if (sreq_index > ADMA_CH_CTRL_TX_REQ_MAX) {
> + dev_err(tdma->dev, "invalid DMA request\n");
> + return -EINVAL;
> + }
> +
> + if (test_and_set_bit(sreq_index, &tdma->tx_requests_reserved)) {
> + dev_err(tdma->dev, "DMA request reserved\n");
> + return -EINVAL;
> + }
> + break;
> +
> + case ADMA_CH_CTRL_DIR_AHUB2MEM:
> + if (sreq_index > ADMA_CH_CTRL_RX_REQ_MAX) {
> + dev_err(tdma->dev, "invalid DMA request\n");
> + return -EINVAL;
> + }
> +
> + if (test_and_set_bit(sreq_index, &tdma->rx_requests_reserved)) {
> + dev_err(tdma->dev, "DMA request reserved\n");
> + return -EINVAL;
> + }
> + break;
> +
> + default:
> + dev_WARN(tdma->dev, "channel %s has invalid transfer type\n",
> + tdc->name);
Can you use classical enum dma_direction and do conversion to your
values exactly when it's needed?
In such case before you may call helper is_slave_direction(dir).
> + return -EINVAL;
> + }
> +
> + tdc->sreq_dir = sreq_dir;
> + tdc->sreq_reserved = true;
> +
> + return 0;
> +}
> +
> +static void tegra_adma_request_free(struct tegra_adma_chan *tdc)
> +{
> + struct tegra_adma *tdma = tdc->tdma;
> +
> + if (!tdc->sreq_reserved)
> + return;
> +
> + switch (tdc->sreq_dir) {
> + case ADMA_CH_CTRL_DIR_MEM2AHUB:
> + clear_bit(tdc->sreq_index, &tdma->tx_requests_reserved);
> + break;
> + case ADMA_CH_CTRL_DIR_AHUB2MEM:
> + clear_bit(tdc->sreq_index, &tdma->rx_requests_reserved);
> + break;
> + default:
> + dev_WARN(tdma->dev, "channel %s has invalid transfer type\n",
> + tdc->name);
> + return;
Ditto.
> + }
> +
> + tdc->sreq_reserved = false;
> +}
> +
> +static u32 tegra_adma_irq_status(struct tegra_adma_chan *tdc)
> +{
> + u32 status = tdma_ch_read(tdc, ADMA_CH_INT_STATUS);
> +
> + return status & ADMA_CH_INT_STATUS_XFER_DONE;
> +}
> +
> +static u32 tegra_adma_irq_clear(struct tegra_adma_chan *tdc)
> +{
> + u32 status = tegra_adma_irq_status(tdc);
> +
> + if (status)
> + tdma_ch_write(tdc, ADMA_CH_INT_CLEAR, status);
> +
> + return status;
> +}
> +
> +static void tegra_adma_stop(struct tegra_adma_chan *tdc)
> +{
> + unsigned int status;
> +
> + /* Disable ADMA */
> + tdma_ch_write(tdc, ADMA_CH_CMD, 0);
> +
> + /* Clear interrupt status */
> + tegra_adma_irq_clear(tdc);
> +
> + if (readx_poll_timeout_atomic(readl, tdc->chan_addr + ADMA_CH_STATUS,
> + status, !(status & ADMA_CH_STATUS_XFER_EN),
> + 20, 10000)) {
> + dev_err(tdc2dev(tdc), "unable to stop DMA channel\n");
> + return;
> + }
> +
> + tdc->desc = NULL;
Would it be memory leak here when called from terminate_all() ?
> +}
> +
> +static void tegra_adma_start(struct tegra_adma_chan *tdc)
> +{
> + struct virt_dma_desc *vd = vchan_next_desc(&tdc->vc);
> + struct tegra_adma_chan_regs *ch_regs;
> + struct tegra_adma_desc *desc;
> +
> + if (!vd)
> + return;
> +
> + list_del(&vd->node);
> +
> + desc = to_tegra_adma_desc(&vd->tx);
> +
Redundant empty line.
> + if (!desc) {
> + dev_warn(tdc2dev(tdc), "unable to start DMA, no descriptor\n");
> + return;
> + }
> +
> + ch_regs = &desc->ch_regs;
> +
> + tdc->tx_buf_pos = 0;
> + tdc->tx_buf_count = 0;
> + tdma_ch_write(tdc, ADMA_CH_TC, ch_regs->tc);
> + tdma_ch_write(tdc, ADMA_CH_CTRL, ch_regs->ctrl);
> + tdma_ch_write(tdc, ADMA_CH_LOWER_SRC_ADDR, ch_regs->src_addr);
> + tdma_ch_write(tdc, ADMA_CH_LOWER_TRG_ADDR, ch_regs->trg_addr);
> + tdma_ch_write(tdc, ADMA_CH_FIFO_CTRL, ch_regs->fifo_ctrl);
> + tdma_ch_write(tdc, ADMA_CH_CONFIG, ch_regs->config);
> +
> + /* Start ADMA */
> + tdma_ch_write(tdc, ADMA_CH_CMD, 1);
> +
> + tdc->desc = desc;
> +}
> +
> +static void tegra_adma_update_position(struct tegra_adma_chan *tdc)
> +{
> + struct tegra_adma_desc *desc = tdc->desc;
> + unsigned int max = ADMA_CH_XFER_STATUS_COUNT_MASK + 1;
> + unsigned int pos = tdma_ch_read(tdc, ADMA_CH_XFER_STATUS);
> +
> + /*
> + * Handle wrap around of buffer count register
> + */
> + if (pos < tdc->tx_buf_pos)
> + tdc->tx_buf_count += pos + (max - tdc->tx_buf_pos);
> + else
> + tdc->tx_buf_count += pos - tdc->tx_buf_pos;
> +
> + tdc->tx_buf_pos = pos;
> +
> + desc->bytes_transferred = tdc->tx_buf_count * desc->ch_regs.tc;
> +
> + /*
> + * If we are not currently active, then it is safe to read the
> + * remaining words from the TC_STATUS register and add the partial
> + * buffer to the total transferred.
> + */
> + if (!tdc->desc)
if (desc)
return;
...
?
> + desc->bytes_transferred += desc->ch_regs.tc -
> + tdma_ch_read(tdc, ADMA_CH_TC_STATUS);
> +}
> +
> +static unsigned int tegra_adma_get_residue(struct tegra_adma_desc *desc)
> +{
> + return desc->bytes_requested - (desc->bytes_transferred %
> + desc->bytes_requested);
> +}
> +
> +static irqreturn_t tegra_adma_isr(int irq, void *dev_id)
> +{
> + struct tegra_adma_chan *tdc = dev_id;
> + unsigned long status;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&tdc->lock, flags);
> +
> + status = tegra_adma_irq_clear(tdc);
> + if (status == 0 || !tdc->desc) {
> + spin_unlock_irqrestore(&tdc->lock, flags);
> + return IRQ_NONE;
> + }
> +
> + vchan_cyclic_callback(&tdc->desc->vd);
> +
> + spin_unlock_irqrestore(&tdc->lock, flags);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void tegra_adma_issue_pending(struct dma_chan *dc)
> +{
> + struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&tdc->lock, flags);
> +
> + if (vchan_issue_pending(&tdc->vc)) {
> + if (tdc->desc)
> + dev_warn(tdc2dev(tdc), "DMA already running\n");
The message makes not much sense here. User can call this as many
times as they want to.
> + else
> + tegra_adma_start(tdc);
> + }
> +
> + spin_unlock_irqrestore(&tdc->lock, flags);
> +}
> +
> +static int tegra_adma_terminate_all(struct dma_chan *dc)
> +{
> + struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
> + unsigned long flags;
> + LIST_HEAD(head);
> +
> + spin_lock_irqsave(&tdc->lock, flags);
> +
> + if (tdc->desc)
> + tegra_adma_stop(tdc);
> +
> + tegra_adma_request_free(tdc);
> + vchan_get_all_descriptors(&tdc->vc, &head);
> + spin_unlock_irqrestore(&tdc->lock, flags);
> + vchan_dma_desc_free_list(&tdc->vc, &head);
> +
> + return 0;
> +}
> +
> +static enum dma_status tegra_adma_tx_status(struct dma_chan *dc,
> + dma_cookie_t cookie,
> + struct dma_tx_state *txstate)
> +{
> + struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
> + struct tegra_adma_desc *desc;
> + struct virt_dma_desc *vd;
> + enum dma_status ret;
> + unsigned long flags;
> + unsigned int residual;
> +
> + spin_lock_irqsave(&tdc->lock, flags);
> +
> + ret = dma_cookie_status(dc, cookie, txstate);
No need to run this under spin lock.
> + if (ret == DMA_COMPLETE || !txstate) {
> + spin_unlock_irqrestore(&tdc->lock, flags);
> + return ret;
> + }
> +
> + vd = vchan_find_desc(&tdc->vc, cookie);
> + if (vd) {
> + desc = to_tegra_adma_desc(&vd->tx);
> + residual = desc->ch_regs.tc;
> + } else if (tdc->desc && tdc->desc->vd.tx.cookie == cookie) {
> + tegra_adma_update_position(tdc);
> + residual = tegra_adma_get_residue(tdc->desc);
> + } else {
> + residual = 0;
> + }
> +
> + dma_set_residue(txstate, residual);
This could be out of spin lock. We are protecting data, not the code.
> +
> + spin_unlock_irqrestore(&tdc->lock, flags);
> +
> + return ret;
> +}
> +
> +static int tegra_adma_set_xfer_params(struct tegra_adma_chan *tdc,
> + struct tegra_adma_desc *desc,
> + dma_addr_t buf_addr, size_t buf_len,
> + size_t period_len,
> + enum dma_transfer_direction direction)
> +{
> + struct tegra_adma_chan_regs *ch_regs = &desc->ch_regs;
> + unsigned int burst_size, num_bufs, sreq_dir;
> +
> + num_bufs = buf_len / period_len;
> +
> + if (num_bufs > ADMA_CH_CONFIG_MAX_BUFS)
> + return -EINVAL;
> +
> + switch (direction) {
> + case DMA_MEM_TO_DEV:
> + sreq_dir = ADMA_CH_CTRL_DIR_MEM2AHUB;
> + burst_size = fls(tdc->sconfig.dst_maxburst);
> + ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(num_bufs - 1);
> + ch_regs->ctrl = ADMA_CH_CTRL_TX_REQ(tdc->sreq_index);
> + ch_regs->src_addr = buf_addr;
> + break;
> +
> + case DMA_DEV_TO_MEM:
> + sreq_dir = ADMA_CH_CTRL_DIR_AHUB2MEM;
> + burst_size = fls(tdc->sconfig.src_maxburst);
> + ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(num_bufs - 1);
> + ch_regs->ctrl = ADMA_CH_CTRL_RX_REQ(tdc->sreq_index);
> + ch_regs->trg_addr = buf_addr;
> + break;
> +
> + default:
> + dev_err(tdc2dev(tdc), "DMA direction is not supported\n");
> + return -EINVAL;
Call to is_slave_direction() before switch-case might look better.
> + }
> +
> + if (!burst_size || burst_size > ADMA_CH_CONFIG_BURST_16)
> + burst_size = ADMA_CH_CONFIG_BURST_16;
> +
> + ch_regs->ctrl |= ADMA_CH_CTRL_DIR(sreq_dir) |
> + ADMA_CH_CTRL_MODE_CONTINUOUS |
> + ADMA_CH_CTRL_FLOWCTRL_EN;
> + ch_regs->config |= ADMA_CH_CONFIG_BURST_SIZE(burst_size);
> + ch_regs->config |= ADMA_CH_CONFIG_WEIGHT_FOR_WRR(1);
> + ch_regs->fifo_ctrl = ADMA_CH_FIFO_CTRL_DEFAULT;
> + ch_regs->tc = period_len & ADMA_CH_TC_COUNT_MASK;
> +
> + return tegra_adma_request_alloc(tdc, sreq_dir);
> +}
> +
> +static struct dma_async_tx_descriptor *tegra_adma_prep_slave_sg(
> + struct dma_chan *dc, struct scatterlist *sgl, unsigned int sg_len,
> + enum dma_transfer_direction direction, unsigned long flags,
> + void *context)
> +{
> + struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
> +
> + dev_warn(tdc2dev(tdc), "scatter-gather transfers are not supported\n");
Any plans to do that? If no, just remove the entire function.
> +
> + return NULL;
> +}
> +
> +static struct dma_async_tx_descriptor *tegra_adma_prep_dma_cyclic(
> + struct dma_chan *dc, dma_addr_t buf_addr, size_t buf_len,
> + size_t period_len, enum dma_transfer_direction direction,
> + unsigned long flags)
> +{
> + struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
> + struct tegra_adma_desc *desc = NULL;
> +
> + if (!tdc->sconfig_valid) {
Looks like excessive check. If user didn't call slave_config(), it's a
problem of user and it should be fixed. Protective programming here
seems not needed.
> + dev_err(tdc2dev(tdc), "ADMA slave configuration not set\n");
> + return NULL;
> + }
> +
> + if (!buf_len || !period_len || period_len > ADMA_CH_TC_COUNT_MASK) {
> + dev_err(tdc2dev(tdc), "invalid buffer/period len\n");
> + return NULL;
> + }
> +
> + if (buf_len % period_len) {
> + dev_err(tdc2dev(tdc), "buf_len not a multiple of period_len\n");
> + return NULL;
> + }
> +
> + if (!IS_ALIGNED(buf_addr, 4)) {
> + dev_err(tdc2dev(tdc), "invalid buffer alignment\n");
> + return NULL;
> + }
> +
> + desc = kzalloc(sizeof(*desc), GFP_NOWAIT);
> + if (!desc)
> + return NULL;
> +
> + desc->bytes_transferred = 0;
> + desc->bytes_requested = buf_len;
> +
> + if (tegra_adma_set_xfer_params(tdc, desc, buf_addr, buf_len, period_len,
> + direction)) {
> + kfree(desc);
> + return NULL;
> + }
> +
> + return vchan_tx_prep(&tdc->vc, &desc->vd, flags);
> +}
> +
> +static int tegra_adma_alloc_chan_resources(struct dma_chan *dc)
> +{
> + struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
> + int ret;
> +
> + ret = pm_runtime_get_sync(tdc2dev(tdc));
> + if (ret)
> + return ret;
> +
> + dma_cookie_init(&tdc->vc.chan);
> + tdc->sconfig_valid = false;
> +
> + return 0;
> +}
> +
> +static void tegra_adma_free_chan_resources(struct dma_chan *dc)
> +{
> + struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
> +
> + if (tdc->desc)
> + tegra_adma_terminate_all(dc);
Seems after Lars' patchset be merged this will become redundant.
And you may call this unconditionally of course.
> +
> + tdc->sconfig_valid = false;
> + vchan_free_chan_resources(&tdc->vc);
> +
> + pm_runtime_put(tdc2dev(tdc));
pm_runtime_get_sync() in alloc() till pm_runtime_put() in free() seems
too much to cover in time. Imagine if user allocates resources, but
will never use them. How possible to suspend device?
> +
> + tegra_adma_request_free(tdc);
> +
> + tdc->sreq_index = 0;
> + tdc->sreq_dir = 0;
> +}
> +
> +static struct dma_chan *tegra_dma_of_xlate(struct of_phandle_args *dma_spec,
> + struct of_dma *ofdma)
> +{
> + struct tegra_adma *tdma = ofdma->of_dma_data;
> + struct tegra_adma_chan *tdc;
> + struct dma_chan *chan;
> + unsigned int sreq_index;
> +
> + if (dma_spec->args_count != 1)
> + return NULL;
Shouldn't be already checked by of-dma library?
> +
> + sreq_index = dma_spec->args[0];
> +
> + if (sreq_index == 0) {
> + dev_err(tdma->dev, "DMA request must not be 0\n");
Why not? HW limitation?
> + return NULL;
> + }
> +
> + chan = dma_get_any_slave_channel(&tdma->dma_dev);
> + if (!chan)
> + return NULL;
> +
> + tdc = to_tegra_adma_chan(chan);
> + tdc->sreq_index = sreq_index;
> +
> + return chan;
> +}
> +
> +static int tegra_adma_runtime_suspend(struct device *dev)
> +{
> + struct tegra_adma *tdma = dev_get_drvdata(dev);
> +
> + tdma->global_cmd = tdma_read(tdma, ADMA_GLOBAL_CMD);
> +
> + clk_disable_unprepare(tdma->adma_clk);
> +
> + return 0;
> +}
> +
> +static int tegra_adma_runtime_resume(struct device *dev)
> +{
> + struct tegra_adma *tdma = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = clk_prepare_enable(tdma->adma_clk);
> + if (ret < 0) {
> + dev_err(dev, "failed to enable ADMA clock: %d\n", ret);
> + return ret;
> + }
> +
> + tdma_write(tdma, ADMA_GLOBAL_CMD, tdma->global_cmd);
> +
> + return 0;
> +}
> +
> +static const struct tegra_adma_chip_data tegra210_chip_data = {
> + .nr_channels = 22,
> +};
> +
> +static const struct of_device_id tegra_adma_of_match[] = {
> + { .compatible = "nvidia,tegra210-adma", .data = &tegra210_chip_data },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, tegra_adma_of_match);
> +
> +static int tegra_adma_probe(struct platform_device *pdev)
> +{
> + const struct of_device_id *match;
> + const struct tegra_adma_chip_data *cdata;
> + struct tegra_adma *tdma;
> + struct resource *res;
> + int ret, i;
> +
> + if (!pdev->dev.of_node) {
> + dev_err(&pdev->dev, "no device tree node for ADMA\n");
> + return -ENODEV;
> + }
Do you need this check since you later call of_match_device() ?
> +
> + match = of_match_device(tegra_adma_of_match, &pdev->dev);
> + if (!match) {
> + dev_err(&pdev->dev, "no device match found\n");
> + return -ENODEV;
> + }
> + cdata = match->data;
> +
> + tdma = devm_kzalloc(&pdev->dev, sizeof(*tdma) + cdata->nr_channels *
> + sizeof(struct tegra_adma_chan), GFP_KERNEL);
> + if (!tdma)
> + return -ENOMEM;
> +
> + tdma->dev = &pdev->dev;
> + tdma->nr_channels = cdata->nr_channels;
> + platform_set_drvdata(pdev, tdma);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + tdma->base_addr = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(tdma->base_addr))
> + return PTR_ERR(tdma->base_addr);
> +
> + tdma->adma_clk = devm_clk_get(&pdev->dev, "adma_ape");
> + if (IS_ERR(tdma->adma_clk)) {
> + dev_err(&pdev->dev, "ADMA clock not found\n");
> + return PTR_ERR(tdma->adma_clk);
> + }
> +
> + pm_runtime_enable(&pdev->dev);
> + if (pm_runtime_enabled(&pdev->dev))
> + ret = pm_runtime_get_sync(&pdev->dev);
> + else
> + ret = tegra_adma_runtime_resume(&pdev->dev);
> +
> + if (ret) {
> + pm_runtime_disable(&pdev->dev);
> + return ret;
> + }
> +
> + ret = tegra_adma_init(tdma);
> + if (ret)
> + goto err_pm_disable;
> +
> + INIT_LIST_HEAD(&tdma->dma_dev.channels);
> + for (i = 0; i < tdma->nr_channels; i++) {
> + struct tegra_adma_chan *tdc = &tdma->channels[i];
> +
> + tdc->chan_addr = tdma->base_addr + ADMA_CH_REG_OFFSET(i);
> +
> + snprintf(tdc->name, sizeof(tdc->name), "adma.%d", i);
> +
> + tdc->irq = platform_get_irq(pdev, i);
> + if (tdc->irq < 0) {
> + ret = -EPROBE_DEFER;
So, any failure of getting an IRQ resource will be considered as
deferral? I doubt it's a good idea.
> + goto err_irq;
> + }
> +
> + ret = devm_request_irq(&pdev->dev, tdc->irq, tegra_adma_isr, 0,
> + tdc->name, tdc);
Better to use request_irq(). There is no benefit of devm_ variant in
few cases (not only DMA Engine drivers).
> + if (ret) {
> + dev_err(&pdev->dev,
> + "failed to get interrupt for channel %d\n", i);
> + goto err_irq;
> + }
> +
> + spin_lock_init(&tdc->lock);
> + vchan_init(&tdc->vc, &tdma->dma_dev);
> + tdc->vc.desc_free = tegra_adma_desc_free;
> + tdc->tdma = tdma;
> + }
> +
> + dma_cap_set(DMA_SLAVE, tdma->dma_dev.cap_mask);
> + dma_cap_set(DMA_PRIVATE, tdma->dma_dev.cap_mask);
> + dma_cap_set(DMA_CYCLIC, tdma->dma_dev.cap_mask);
> +
> + tdma->dma_dev.dev = &pdev->dev;
> + tdma->dma_dev.device_alloc_chan_resources =
> + tegra_adma_alloc_chan_resources;
> + tdma->dma_dev.device_free_chan_resources =
> + tegra_adma_free_chan_resources;
> + tdma->dma_dev.device_issue_pending = tegra_adma_issue_pending;
> + tdma->dma_dev.device_prep_slave_sg = tegra_adma_prep_slave_sg;
> + tdma->dma_dev.device_prep_dma_cyclic = tegra_adma_prep_dma_cyclic;
> + tdma->dma_dev.device_config = tegra_adma_slave_config;
> + tdma->dma_dev.device_tx_status = tegra_adma_tx_status;
> + tdma->dma_dev.device_terminate_all = tegra_adma_terminate_all;
> + tdma->dma_dev.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> + tdma->dma_dev.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> +
> + ret = dma_async_device_register(&tdma->dma_dev);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "ADMA registration failed: %d\n", ret);
> + goto err_irq;
> + }
> +
> + ret = of_dma_controller_register(pdev->dev.of_node,
> + tegra_dma_of_xlate, tdma);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "ADMA OF registration failed %d\n", ret);
> + goto err_unregister_dma_dev;
> + }
> +
> + pm_runtime_put(&pdev->dev);
It might be called earlier, mightn't it?
> +
> + dev_info(&pdev->dev, "Tegra210 ADMA driver registered %d channels\n",
> + tdma->nr_channels);
> +
> + return 0;
> +
> +err_unregister_dma_dev:
> + dma_async_device_unregister(&tdma->dma_dev);
> +err_irq:
> + while (--i >= 0) {
> + struct tegra_adma_chan *tdc = &tdma->channels[i];
> +
> + tasklet_kill(&tdc->vc.task);
> + }
> +err_pm_disable:
> + pm_runtime_disable(&pdev->dev);
> + if (!pm_runtime_status_suspended(&pdev->dev))
> + tegra_adma_runtime_suspend(&pdev->dev);
> +
> + return ret;
> +}
> +
> +static int tegra_adma_remove(struct platform_device *pdev)
> +{
> + struct tegra_adma *tdma = platform_get_drvdata(pdev);
> + struct tegra_adma_chan *tdc;
> + int i;
> +
> + dma_async_device_unregister(&tdma->dma_dev);
> +
> + for (i = 0; i < tdma->nr_channels; ++i) {
> + tdc = &tdma->channels[i];
> + disable_irq(tdc->irq);
> + tasklet_kill(&tdc->vc.task);
> + }
> +
> + pm_runtime_disable(&pdev->dev);
> + if (!pm_runtime_status_suspended(&pdev->dev))
> + tegra_adma_runtime_suspend(&pdev->dev);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int tegra_adma_pm_suspend(struct device *dev)
> +{
> + return pm_runtime_suspended(dev);
> +}
> +#endif
> +
> +static const struct dev_pm_ops tegra_adma_dev_pm_ops = {
> + SET_RUNTIME_PM_OPS(tegra_adma_runtime_suspend,
> + tegra_adma_runtime_resume, NULL)
> + SET_SYSTEM_SLEEP_PM_OPS(tegra_adma_pm_suspend, NULL)
The runtime PM calls and system suspend ones are more or less
equivalent on LATE stage. Also, it's usually a problem with DMA that
you may try to suspend active device used by others.
> +};
> +
> +static struct platform_driver tegra_admac_driver = {
> + .driver = {
> + .name = "tegra-adma",
> + .pm = &tegra_adma_dev_pm_ops,
> + .of_match_table = tegra_adma_of_match,
> + },
> + .probe = tegra_adma_probe,
> + .remove = tegra_adma_remove,
> +};
> +
> +module_platform_driver(tegra_admac_driver);
> +
> +MODULE_ALIAS("platform:tegra210-adma");
> +MODULE_DESCRIPTION("NVIDIA Tegra ADMA driver");
> +MODULE_AUTHOR("Dara Ramesh <dramesh@...dia.com>");
> +MODULE_AUTHOR("Jon Hunter <jonathanh@...dia.com>");
> +MODULE_LICENSE("GPL v2");
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists