On Fri, Jan 05, 2018 at 06:14:08PM -0800, Hyun Kwon wrote:
> The ZynqMP includes the DisplayPort subsystem with own DMA engine
> called DPDMA. The DPDMA IP comes with 6 individual channels
> (4 for display, 2 for audio). This driver implements DMAENGINE API
> which can be used for audio (ALSA) and display (DRM) drivers.
The subsystem name is dmaengine.
> Signed-off-by: Hyun Kwon <hyun.kwon@xxxxxxxxxx>
> Signed-off-by: Tejas Upadhyay <tejasu@xxxxxxxxxx>
> Signed-off-by: Michal Simek <michal.simek@xxxxxxxxxx>
> ---
> MAINTAINERS | 7 +
> drivers/dma/Kconfig | 19 +
> drivers/dma/xilinx/Makefile | 1 +
> drivers/dma/xilinx/xilinx_dpdma.c | 2309 +++++++++++++++++++++++++++++++++++++
That is a very long file for review! Pls split it up to reasonable logical
chunks
> +config XILINX_DPDMA_DEBUG_FS
> + bool "Xilinx DPDMA debugfs"
> + depends on DEBUG_FS && XILINX_DPDMA
> + help
> + Enable the debugfs code for DPDMA driver. The debugfs code
> + enables debugging or testing related features. It exposes some
> + low level controls to the user space to help testing automation,
> + as well as can enable additional diagnostic or statistical
> + information.
why should this be a compile option
> + * Xilinx ZynqMP DPDMA Engine driver
> + *
> + * Copyright (C) 2015 - 2018 Xilinx, Inc.
> + *
> + * Author: Hyun Woo Kwon <hyun.kwon@xxxxxxxxxx>
> + *
> + * SPDX-License-Identifier: GPL-2.0
this should be first line in file with c99 style,
// SPDX-License-Identifier: GPL-2.0
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dmapool.h>
> +#include <linux/gfp.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqreturn.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_dma.h>
> +#include <linux/platform_device.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>
> +#include <linux/uaccess.h>
> +#include <linux/wait.h>
do you need all these headers?
> +
> +#include "../dmaengine.h"
> +
> +/* DPDMA registers */
> +#define XILINX_DPDMA_ERR_CTRL 0x0
> +#define XILINX_DPDMA_ISR 0x4
> +#define XILINX_DPDMA_IMR 0x8
> +#define XILINX_DPDMA_IEN 0xc
> +#define XILINX_DPDMA_IDS 0x10
> +#define XILINX_DPDMA_INTR_DESC_DONE_MASK (0x3f << 0)
GENMASK() for this and others
> +#define XILINX_DPDMA_INTR_DESC_DONE_SHIFT 0
you can define a common shift using ffs
> + * struct xilinx_dpdma_hw_desc - DPDMA hardware descriptor
> + * @control: control configuration field
> + * @desc_id: descriptor ID
> + * @xfer_size: transfer size
> + * @hsize_stride: horizontal size and stride
> + * @timestamp_lsb: LSB of time stamp
> + * @timestamp_msb: MSB of time stamp
> + * @addr_ext: upper 16 bit of 48 bit address (next_desc and src_addr)
> + * @next_desc: next descriptor 32 bit address
> + * @src_addr: payload source address (lower 32 bit of 1st 4KB page)
> + * @addr_ext_23: upper 16 bit of 48 bit address (src_addr2 and src_addr3)
> + * @addr_ext_45: upper 16 bit of 48 bit address (src_addr4 and src_addr5)
> + * @src_addr2: payload source address (lower 32 bit of 2nd 4KB page)
> + * @src_addr3: payload source address (lower 32 bit of 3rd 4KB page)
> + * @src_addr4: payload source address (lower 32 bit of 4th 4KB page)
> + * @src_addr5: payload source address (lower 32 bit of 5th 4KB page)
what does it mean (lower 32 bit of Nth 4KB page)
> +struct xilinx_dpdma_sw_desc {
> + struct xilinx_dpdma_hw_desc hw;
> + struct list_head node;
> + dma_addr_t phys;
dma_addr_t is not a physical addr
> +struct xilinx_dpdma_device {
> + struct dma_device common;
> + void __iomem *reg;
> + struct device *dev;
> +
> + struct clk *axi_clk;
> + struct xilinx_dpdma_chan *chan[XILINX_DPDMA_NUM_CHAN];
> +
> + bool ext_addr;
> + void (*desc_addr)(struct xilinx_dpdma_sw_desc *sw_desc,
> + struct xilinx_dpdma_sw_desc *prev,
> + dma_addr_t dma_addr[], unsigned int num_src_addr);
> +};
> +
> +#ifdef CONFIG_XILINX_DPDMA_DEBUG_FS
this can be CONFIG_DEBUGFS, also am skipping this part to focus on driver.
Pls split this to a separate patch
> +static int xilinx_dpdma_config(struct dma_chan *dchan,
> + struct dma_slave_config *config)
> +{
> + if (config->direction != DMA_MEM_TO_DEV)
> + return -EINVAL;
?? Why are the config values not used, how else do you configure the
channel?
> +
> + return 0;
> +}
> +
> +static int xilinx_dpdma_pause(struct dma_chan *dchan)
> +{
> + xilinx_dpdma_chan_pause(to_xilinx_chan(dchan));
> +
> + return 0;
> +}
> +
> +static int xilinx_dpdma_resume(struct dma_chan *dchan)
> +{
> + xilinx_dpdma_chan_unpause(to_xilinx_chan(dchan));
> +
> + return 0;
> +}
> +
> +static int xilinx_dpdma_terminate_all(struct dma_chan *dchan)
> +{
> + return xilinx_dpdma_chan_terminate_all(to_xilinx_chan(dchan));
> +}
> +
> +static void xilinx_dpdma_synchronize(struct dma_chan *dchan)
> +{
> + xilinx_dpdma_chan_synchronize(to_xilinx_chan(dchan));
> +}
why have these wrappers which do not do anything?
> +
> + chan->reg = xdev->reg + XILINX_DPDMA_CH_BASE + XILINX_DPDMA_CH_OFFSET *
> + chan->id;
> + chan->status = IDLE;
> +
> + spin_lock_init(&chan->lock);
> + INIT_LIST_HEAD(&chan->done_list);
> + init_waitqueue_head(&chan->wait_to_stop);
> +
> + tasklet_init(&chan->done_task, xilinx_dpdma_chan_done_task,
> + (unsigned long)chan);
> + tasklet_init(&chan->err_task, xilinx_dpdma_chan_err_task,
> + (unsigned long)chan);
Have you checked the vchan code, i dont see that used. It will help you in
descriptor management and reduce code from driver
> +static int xilinx_dpdma_remove(struct platform_device *pdev)
> +{
> + struct xilinx_dpdma_device *xdev;
> + unsigned int i;
> +
> + xdev = platform_get_drvdata(pdev);
> +
> + xilinx_dpdma_disable_intr(xdev);
> + of_dma_controller_free(pdev->dev.of_node);
> + dma_async_device_unregister(&xdev->common);
> + clk_disable_unprepare(xdev->axi_clk);
> +
> + for (i = 0; i < XILINX_DPDMA_NUM_CHAN; i++)
> + if (xdev->chan[i])
> + xilinx_dpdma_chan_remove(xdev->chan[i]);
At this point your irq is still enabled and can fire, and can schedule
tasklet.. Are you sure you are okay with that?
> +MODULE_AUTHOR("Xilinx, Inc.");
> +MODULE_DESCRIPTION("Xilinx DPDMA driver");
> +MODULE_LICENSE("GPL v2");
No MODULE_ALIAS()?
I haven't reviewed in details though as it is too large patch. Pls use vchan
and split things up for better review.
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
