[1/2] dt: bindings: dma: xilinx: dpdma: DT bindings for Xilinx DPDMA

Message ID 1515204848-3493-1-git-send-email-hyun.kwon@xilinx.com
State New
Headers show
Series
  • [1/2] dt: bindings: dma: xilinx: dpdma: DT bindings for Xilinx DPDMA
Related show

Commit Message

Hyun Kwon Jan. 6, 2018, 2:14 a.m.
The ZynqMP includes the DisplayPort subsystem with its own DMA engine
called DPDMA. The DPDMA IP comes with 6 individual channels
(4 for display, 2 for audio). This documentation describes DT bindings
of DPDMA.

Signed-off-by: Hyun Kwon <hyun.kwon@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
 .../bindings/dma/xilinx/xilinx_dpdma.txt           | 64 ++++++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/xilinx/xilinx_dpdma.txt

Comments

Vinod Koul Jan. 12, 2018, 1:28 p.m. | #1
On Fri, Jan 05, 2018 at 06:14:07PM -0800, Hyun Kwon wrote:
> The ZynqMP includes the DisplayPort subsystem with its own DMA engine
> called DPDMA. The DPDMA IP comes with 6 individual channels
> (4 for display, 2 for audio). This documentation describes DT bindings
> of DPDMA.

Pls cc DT maintainers on this patch

> Signed-off-by: Hyun Kwon <hyun.kwon@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>  .../bindings/dma/xilinx/xilinx_dpdma.txt           | 64 ++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/dma/xilinx/xilinx_dpdma.txt
> 
> diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dpdma.txt b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dpdma.txt
> new file mode 100644
> index 0000000..51016d8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dpdma.txt
> @@ -0,0 +1,64 @@
> +Device-Tree bindings for Xilinx ZynqMP DP Subsystem DMA
> +
> +The ZynqMP DisplayPort subsystem handles DMA channel buffer management,
> +blending, and audio mixing. The DisplayPort subsystem receives display
> +and audio frames from DPDMA and transmits output to the DisplayPort IP core.
> +
> +Required properties:
> + - compatible: Should be "xlnx,dpdma".
> + - reg: Base address and size of the IP core.
> + - interrupts: Interrupt number.
> + - interrupts-parent: phandle for interrupt controller.
> + - clocks: phandle for AXI clock
> + - clock-names: The identification string, "axi_clk", is always required.
> +
> +Required child node properties:
> +- compatible: Should be one of "xlnx,video0", "xlnx,video1", "xlnx,video2",
> +  "xlnx,graphics", "xlnx,audio0", or "xlnx,audio1".
> +
> +Example:
> +
> +	xlnx_dpdma: axidpdma@43c10000 {
> +		compatible = "xlnx,dpdma";
> +		reg = <0x43c10000 0x1000>;
> +		interrupts = <0 54 4>;
> +		interrupt-parent = <&intc>;
> +		clocks = <&clkc 16>;
> +		clock-names = "axi_clk";
> +
> +		#dma-cells = <1>;
> +		dma-video0channel {
> +			compatible = "xlnx,video0";
> +		};
> +		dma-video1channel {
> +			compatible = "xlnx,video1";
> +		};
> +		dma-video2channel {
> +			compatible = "xlnx,video2";
> +		};
> +		dma-graphicschannel {
> +			compatible = "xlnx,graphics";
> +		};
> +		dma-audio0channel {
> +			compatible = "xlnx,audio0";
> +		};
> +		dma-audio1channel {
> +			compatible = "xlnx,audio1";
> +		};
> +	};
> +
> +* DMA client
> +
> +Required properties:
> +- dmas: a list of <[DPDMA device phandle] [Channel ID]> pairs. "Channel ID"
> +  is defined as video0 = 0, video1 = 1, video2 = 2, graphics = 3, audio0 = 4,
> +  and audio1 = 5.
> +
> +Example:
> +
> +	xlnx_drm {
> +		...
> +		dmas = <&xlnx_dpdma 3>;
> +		dma-names = "dma";
> +		...
> +	};
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vinod Koul Jan. 12, 2018, 2:13 p.m. | #2
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@xilinx.com>
> Signed-off-by: Tejas Upadhyay <tejasu@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>  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@xilinx.com>
> + *
> + * 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.

Patch

diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dpdma.txt b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dpdma.txt
new file mode 100644
index 0000000..51016d8
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dpdma.txt
@@ -0,0 +1,64 @@ 
+Device-Tree bindings for Xilinx ZynqMP DP Subsystem DMA
+
+The ZynqMP DisplayPort subsystem handles DMA channel buffer management,
+blending, and audio mixing. The DisplayPort subsystem receives display
+and audio frames from DPDMA and transmits output to the DisplayPort IP core.
+
+Required properties:
+ - compatible: Should be "xlnx,dpdma".
+ - reg: Base address and size of the IP core.
+ - interrupts: Interrupt number.
+ - interrupts-parent: phandle for interrupt controller.
+ - clocks: phandle for AXI clock
+ - clock-names: The identification string, "axi_clk", is always required.
+
+Required child node properties:
+- compatible: Should be one of "xlnx,video0", "xlnx,video1", "xlnx,video2",
+  "xlnx,graphics", "xlnx,audio0", or "xlnx,audio1".
+
+Example:
+
+	xlnx_dpdma: axidpdma@43c10000 {
+		compatible = "xlnx,dpdma";
+		reg = <0x43c10000 0x1000>;
+		interrupts = <0 54 4>;
+		interrupt-parent = <&intc>;
+		clocks = <&clkc 16>;
+		clock-names = "axi_clk";
+
+		#dma-cells = <1>;
+		dma-video0channel {
+			compatible = "xlnx,video0";
+		};
+		dma-video1channel {
+			compatible = "xlnx,video1";
+		};
+		dma-video2channel {
+			compatible = "xlnx,video2";
+		};
+		dma-graphicschannel {
+			compatible = "xlnx,graphics";
+		};
+		dma-audio0channel {
+			compatible = "xlnx,audio0";
+		};
+		dma-audio1channel {
+			compatible = "xlnx,audio1";
+		};
+	};
+
+* DMA client
+
+Required properties:
+- dmas: a list of <[DPDMA device phandle] [Channel ID]> pairs. "Channel ID"
+  is defined as video0 = 0, video1 = 1, video2 = 2, graphics = 3, audio0 = 4,
+  and audio1 = 5.
+
+Example:
+
+	xlnx_drm {
+		...
+		dmas = <&xlnx_dpdma 3>;
+		dma-names = "dma";
+		...
+	};