[2/2] dma: xilinx: Add driver for Video Framebuffer IP

[Vinod Koul <vinod.koul@intel.com>]

On Wed, Dec 20, 2017 at 02:00:18PM +0530, Vishal Sagar wrote:
> The Video Framebuffer IP is available in two forms: read or write.
> This driver supports either form of the IP.
> Each IP is a single channel DMA and is video format aware.
> It can read/write video data arranged from/to memory as packed or
> semi-planar way based on the selected video format.
> To get list of supported video formats, clients can call certain
> APIs exposed by the driver. This driver introduces support for
> these IPs and integrates with the DMA Engine Framework.

The subsytem name is dmaengine!

> Signed-off-by: Radhey Shyam Pandey <radheys@xilinx.com>
> Signed-off-by: John Nichols <jnichol@xilinx.com>
> Signed-off-by: Jeffrey Mouroux <jmouroux@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Signed-off-by: Hyun Kwon <hyun.kwon@xilinx.com>
> Signed-off-by: Vishal Sagar <vsagar@xilinx.com>
> ---
>  drivers/dma/Kconfig                |   14 +-
>  drivers/dma/xilinx/Makefile        |    1 +
>  drivers/dma/xilinx/xilinx_frmbuf.c | 1155 ++++++++++++++++++++++++++++++++++++

Thats a very large file for review, do consider splitting stuff

> @@ -342,7 +342,7 @@ config MOXART_DMA
>  	select DMA_VIRTUAL_CHANNELS
>  	help
>  	  Enable support for the MOXA ART SoC DMA controller.
> -
> +

why this noise, this patch has some many sign-off's but didnt anyone see this
noise?

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

Copyright needs to follow this in C99 style

> +/*
> + * DMAEngine driver for Xilinx Framebuffer IP
> + *
> + * Copyright (C) 2016 - 2017 Xilinx, Inc. All rights reserved.
> + *
> + * Authors: Radhey Shyam Pandey <radheys@xilinx.com>
> + *          John Nichols <jnichol@xilinx.com>
> + *          Jeffrey Mouroux <jmouroux@xilinx.com>
> + *
> + * Based on the Freescale DMA driver.
> + *
> + * Description:
> + * The AXI Framebuffer core is a soft Xilinx IP core that
> + * provides high-bandwidth direct memory access between memory
> + * and AXI4-Stream.
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.

why do you need this license text? With SPDX tag this is implied..

> +#include <linux/bitops.h>
> +#include <linux/dma/xilinx_frmbuf.h>
> +#include <linux/dmapool.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_dma.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/slab.h>
> +#include <linux/videodev2.h>

do you need so many headers?

> +struct xilinx_frmbuf_chan {
> +	struct xilinx_frmbuf_device *xdev;
> +	/* Descriptor operation lock */

this is duplicate comment

> +static LIST_HEAD(frmbuf_chan_list);
> +static DEFINE_MUTEX(frmbuf_chan_list_lock);

why do you need these globals?

> +static const struct xilinx_frmbuf_format_desc xilinx_frmbuf_formats[] = {
> +	{
> +		.dts_name = "xbgr8888",
> +		.id = XILINX_FRMBUF_FMT_RGBX8,
> +		.bpp = 4,
> +		.num_planes = 1,
> +		.drm_fmt = DRM_FORMAT_XBGR8888,
> +		.v4l2_fmt = 0,
> +		.fmt_bitmask = BIT(0),
> +	},
> +	{
> +		.dts_name = "unsupported",
> +		.id = XILINX_FRMBUF_FMT_YUVX8,
> +		.bpp = 4,
> +		.num_planes = 1,
> +		.drm_fmt = 0,
> +		.v4l2_fmt = 0,
> +		.fmt_bitmask = BIT(1),
> +	},

am no DT expert, but this doesn't look right to me. Care to explain what
this struct does?

> +static const struct of_device_id xilinx_frmbuf_of_ids[] = {
> +	{ .compatible = "xlnx,axi-frmbuf-wr-v2",
> +		.data = (void *)DMA_DEV_TO_MEM},
> +	{ .compatible = "xlnx,axi-frmbuf-rd-v2",
> +		.data = (void *)DMA_MEM_TO_DEV},

is the direction a hw property or configurable?

> +static inline void frmbuf_write(struct xilinx_frmbuf_chan *chan, u32 reg,
> +				u32 value)

right justfied pls, here and other places

> +{
> +	iowrite32(value, chan->xdev->regs + reg);
> +}
> +
> +static inline void frmbuf_writeq(struct xilinx_frmbuf_chan *chan, u32 reg,
> +				 u64 value)
> +{
> +	iowrite32(lower_32_bits(value), chan->xdev->regs + reg);
> +	iowrite32(upper_32_bits(value), chan->xdev->regs + reg + 4);
> +}
> +
> +static void writeq_addr(struct xilinx_frmbuf_chan *chan, u32 reg,
> +			dma_addr_t addr)
> +{
> +	frmbuf_writeq(chan, reg, (u64)addr);

why the cast?


> +static struct xilinx_frmbuf_device *frmbuf_find_dev(struct dma_chan *chan)
> +{
> +	struct xilinx_frmbuf_chan *xchan, *temp;
> +	struct xilinx_frmbuf_device *xdev;
> +	bool is_frmbuf_chan = false;
> +
> +	list_for_each_entry_safe(xchan, temp, &frmbuf_chan_list, chan_node) {
> +		if (chan == &xchan->common)
> +			is_frmbuf_chan = true;
> +	}
> +
> +	if (!is_frmbuf_chan)
> +		return ERR_PTR(-ENODEV);
> +
> +	xchan = to_xilinx_chan(chan);
> +	xdev = container_of(xchan, struct xilinx_frmbuf_device, chan);
> +
> +	return xdev;

hmm this sounds strange to me, care to explain this pls?

> +static int frmbuf_verify_format(struct dma_chan *chan, u32 fourcc, u32 type)
> +{
> +	struct xilinx_frmbuf_chan *xil_chan = to_xilinx_chan(chan);
> +	u32 i, sz = ARRAY_SIZE(xilinx_frmbuf_formats);
> +
> +	for (i = 0; i < sz; i++) {
> +		if ((type == XDMA_DRM &&
> +		     fourcc != xilinx_frmbuf_formats[i].drm_fmt) ||
> +		   (type == XDMA_V4L2 &&
> +		    fourcc != xilinx_frmbuf_formats[i].v4l2_fmt))

this is very hard to read..

> +static void xilinx_xdma_set_config(struct dma_chan *chan, u32 fourcc, u32 type)
> +{
> +	struct xilinx_frmbuf_chan *xil_chan;
> +	bool found_xchan = false;
> +	int ret;
> +
> +	mutex_lock(&frmbuf_chan_list_lock);
> +	list_for_each_entry(xil_chan, &frmbuf_chan_list, chan_node) {
> +		if (chan == &xil_chan->common) {
> +			found_xchan = true;
> +			break;
> +		}
> +	}

again this seaching and am not sure why?

> +void xilinx_xdma_drm_config(struct dma_chan *chan, u32 drm_fourcc)
> +{
> +	xilinx_xdma_set_config(chan, drm_fourcc, XDMA_DRM);
> +
> +} EXPORT_SYMBOL_GPL(xilinx_xdma_drm_config);

first EXPORT_SYMBOL_GPL should be in different line, second what is being
set? some documentation for EXPORT_SYMBOLs is required..

> +
> +void xilinx_xdma_v4l2_config(struct dma_chan *chan, u32 v4l2_fourcc)
> +{
> +	xilinx_xdma_set_config(chan, v4l2_fourcc, XDMA_V4L2);
> +
> +} EXPORT_SYMBOL_GPL(xilinx_xdma_v4l2_config);

here too..

> +
> +int xilinx_xdma_get_drm_vid_fmts(struct dma_chan *chan, u32 *fmt_cnt,
> +				 u32 **fmts)
> +{
> +	struct xilinx_frmbuf_device *xdev;
> +
> +	xdev = frmbuf_find_dev(chan);
> +
> +	if (IS_ERR(xdev))
> +		return PTR_ERR(xdev);
> +
> +	*fmt_cnt = xdev->drm_fmt_cnt;
> +	*fmts = xdev->drm_memory_fmts;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(xilinx_xdma_get_drm_vid_fmts);

and here

> +
> +int xilinx_xdma_get_v4l2_vid_fmts(struct dma_chan *chan, u32 *fmt_cnt,
> +				  u32 **fmts)
> +{
> +	struct xilinx_frmbuf_device *xdev;
> +
> +	xdev = frmbuf_find_dev(chan);
> +
> +	if (IS_ERR(xdev))
> +		return PTR_ERR(xdev);
> +
> +	*fmt_cnt = xdev->v4l2_fmt_cnt;
> +	*fmts = xdev->v4l2_memory_fmts;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(xilinx_xdma_get_v4l2_vid_fmts);

and this

> +/* -----------------------------------------------------------------------------

do you mind getting rid of these, at best they are distraction

> +static struct xilinx_frmbuf_tx_descriptor *
> +xilinx_frmbuf_alloc_tx_descriptor(struct xilinx_frmbuf_chan *chan)
> +{
> +	struct xilinx_frmbuf_tx_descriptor *desc;
> +
> +	desc = kzalloc(sizeof(*desc), GFP_KERNEL);
> +	if (!desc)
> +		return NULL;

this can be skipped here!

> +static void xilinx_frmbuf_free_descriptors(struct xilinx_frmbuf_chan *chan)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&chan->lock, flags);
> +
> +	xilinx_frmbuf_free_desc_list(chan, &chan->pending_list);
> +	xilinx_frmbuf_free_desc_list(chan, &chan->done_list);
> +	kfree(chan->active_desc);
> +	kfree(chan->staged_desc);
> +
> +	chan->staged_desc = NULL;
> +	chan->active_desc = NULL;
> +	INIT_LIST_HEAD(&chan->pending_list);
> +	INIT_LIST_HEAD(&chan->done_list);

initializing lists in free?

> +static int xilinx_frmbuf_alloc_chan_resources(struct dma_chan *dchan)
> +{
> +	dma_cookie_init(dchan);

this can be done in your probe and not on alloc.. and consequently you can
get rid of alloc..

> +static enum dma_status xilinx_frmbuf_tx_status(struct dma_chan *dchan,
> +					       dma_cookie_t cookie,
> +					       struct dma_tx_state *txstate)
> +{
> +	return dma_cookie_status(dchan, cookie, txstate);

no residue caln?

> +static int xilinx_frmbuf_chan_probe(struct xilinx_frmbuf_device *xdev,
> +				    struct device_node *node)
> +{
> +	struct xilinx_frmbuf_chan *chan;
> +	int err;
> +	u32 dma_addr_size;
> +
> +	chan = &xdev->chan;
> +
> +	chan->dev = xdev->dev;
> +	chan->xdev = xdev;
> +	chan->idle = true;
> +
> +	err = of_property_read_u32(node, "xlnx,dma-addr-width",

why not device_property_read_u32

> +static int xilinx_frmbuf_probe(struct platform_device *pdev)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	struct xilinx_frmbuf_device *xdev;
> +	struct resource *io;
> +	enum dma_transfer_direction dma_dir;
> +	const struct of_device_id *match;
> +	int err;
> +	u32 i, j;
> +	int hw_vid_fmt_cnt;
> +	const char *vid_fmts[ARRAY_SIZE(xilinx_frmbuf_formats)];

inverted x-mas tree helps in readablity

> +	INIT_LIST_HEAD(&xdev->common.channels);
> +	dma_cap_set(DMA_SLAVE, xdev->common.cap_mask);

why slave and why not DMA_INTERLEAVE?

> +	dma_cap_set(DMA_PRIVATE, xdev->common.cap_mask);
> +
> +	/* Initialize the channels */
> +	err = xilinx_frmbuf_chan_probe(xdev, node);
> +	if (err < 0)
> +		return err;
> +
> +	xdev->chan.direction = dma_dir;
> +
> +	if (xdev->chan.direction == DMA_DEV_TO_MEM) {
> +		xdev->common.directions = BIT(DMA_DEV_TO_MEM);
> +		dev_info(&pdev->dev, "Xilinx AXI frmbuf DMA_DEV_TO_MEM\n");
> +	} else if (xdev->chan.direction == DMA_MEM_TO_DEV) {
> +		xdev->common.directions = BIT(DMA_MEM_TO_DEV);
> +		dev_info(&pdev->dev, "Xilinx AXI frmbuf DMA_MEM_TO_DEV\n");
> +	} else {
> +		xilinx_frmbuf_chan_remove(&xdev->chan);

why not do the checks first!

> +MODULE_AUTHOR("Xilinx, Inc.");
> +MODULE_DESCRIPTION("Xilinx Framebuffer driver");
> +MODULE_LICENSE("GPL v2");

no MODULE_ALIAS, how did you test this?

> +#if IS_ENABLED(CONFIG_XILINX_FRMBUF)
> +/**
> + * xilinx_xdma_drm_config - configure video format in video aware DMA
> + * @chan: dma channel instance
> + * @drm_fourcc: DRM fourcc code describing the memory layout of video data
> + *
> + * This routine is used when utilizing "video format aware" Xilinx DMA IP
> + * (such as Video Framebuffer Read or Video Framebuffer Write).  This call
> + * must be made prior to dma_async_issue_pending() to enstablish the video
> + * data memory format within the hardware DMA.
> + */

pls move this to src..