Message ID | 1515204848-3493-1-git-send-email-hyun.kwon@xilinx.com |
---|---|
State | Changes Requested, archived |
Headers | show |
Series | [1/2] dt: bindings: dma: xilinx: dpdma: DT bindings for Xilinx DPDMA | expand |
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
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.
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. > > 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 Use the compatible string plus .txt for the filename. > @@ -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". xlnx,zynqmp-dpdam > + - 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". No. This is an abuse of compatible strings. Why do you need these? You have the DMA channel numbering fixed. > + > +Example: > + > + xlnx_dpdma: axidpdma@43c10000 { dma-controller@ > + 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 devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rob, Thanks for the review. > -----Original Message----- > From: dmaengine-owner@vger.kernel.org [mailto:dmaengine- > owner@vger.kernel.org] On Behalf Of Rob Herring > Sent: Friday, January 19, 2018 11:38 AM > To: Hyun Kwon <hyunk@xilinx.com> > Cc: dmaengine@vger.kernel.org; devicetree@vger.kernel.org; Michal Simek > <michal.simek@xilinx.com> > Subject: Re: [PATCH 1/2] dt: bindings: dma: xilinx: dpdma: DT bindings for > Xilinx DPDMA > > 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. > > > > 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 > > Use the compatible string plus .txt for the filename. > > > @@ -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". > > xlnx,zynqmp-dpdam > > > + - 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". > > No. This is an abuse of compatible strings. > > Why do you need these? You have the DMA channel numbering fixed. > > > + > > +Example: > > + > > + xlnx_dpdma: axidpdma@43c10000 { > > dma-controller@ > I agree with your comments. Will address in v2. Thanks, -hyun > > + 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 devicetree" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > 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
On Tue, Jan 23, 2018 at 05:03:19PM +0000, Hyun Kwon wrote: > > why should this be a compile option > > > > This debugfs code is for testing / regression, and we don't want to enable it > for regular users. Right and that is why you have CONFIG_DEBUGFS, why not use that? > > do you need all these headers? > > > > I directly included all headers that are used in this driver. Some of them can > be removed from indirect inclusions, and I'm fine with that. Please let me know > if that's your preference. Yes pls remove the ones that are needed > > > +#define XILINX_DPDMA_INTR_DESC_DONE_SHIFT 0 > > > > you can define a common shift using ffs > > > > I guess you mean to replace, (value & MASK) << SHIFT, with > (value & MASK) << ffs(MASK). I'll change to that way. Let me know otherwise. yes and you may use the ones in bitfield.h > > what does it mean (lower 32 bit of Nth 4KB page) > > > > Each descriptor can point up to 5 - 48bit address payloads. src_addr* fields > contain lower 32bit of 48bit address. Remaining upper 16bit is programmed in > addr_ext* fields. pls document this > > > +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? > > > > There's not much configuration to be done. Channels are pretty much identical > with fixed configurations. hmmm, you do support DMA_SLAVE right? > > why have these wrappers which do not do anything? > > > > It's just my personal preference to split into different code partitions, and > each section is partitioned / grouped with some comment line. :-) > Ex, a partition for struct dma_chan, and another one for > struct xilinx_dpdma_chan. It gives me sort of abstracted view. But it may be > just me, and it comes with extra indirections. I'll remove unnecessary wrappers. wrapper without any logic dont help > > > + 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? > > > > Ok. You mean that an interrupt can occur right before > xilinx_dpdma_disable_intr(), and the interrupt may be handled in the middle or > after removing this driver. I'll switch to request_irq() from > devm_request_irq(), and remove the irq when removing the driver. yes and ensure tasklets are quiesced > > > +MODULE_AUTHOR("Xilinx, Inc."); > > > +MODULE_DESCRIPTION("Xilinx DPDMA driver"); > > > +MODULE_LICENSE("GPL v2"); > > > > No MODULE_ALIAS()? > > Is it required? Could you please elaborate, to help my understanding? did you try builing as modules and testing this?
> -----Original Message----- > From: Vinod Koul [mailto:vinod.koul@intel.com] > Sent: Wednesday, January 24, 2018 6:41 AM > To: Hyun Kwon <hyunk@xilinx.com> > Cc: dmaengine@vger.kernel.org; devicetree@vger.kernel.org; Michal Simek > <michal.simek@xilinx.com>; Tejas Upadhyay <TEJASU@xilinx.com> > Subject: Re: [PATCH 2/2] dma: xilinx: dpdma: Add the Xilinx DisplayPort > DMA engine driver > > On Tue, Jan 23, 2018 at 05:03:19PM +0000, Hyun Kwon wrote: > > > > why should this be a compile option > > > > > > > This debugfs code is for testing / regression, and we don't want to enable > it > > for regular users. > > Right and that is why you have CONFIG_DEBUGFS, why not use that? > > > > do you need all these headers? > > > > > > > I directly included all headers that are used in this driver. Some of them > can > > be removed from indirect inclusions, and I'm fine with that. Please let me > know > > if that's your preference. > > Yes pls remove the ones that are needed > > > > > +#define XILINX_DPDMA_INTR_DESC_DONE_SHIFT 0 > > > > > > you can define a common shift using ffs > > > > > > > I guess you mean to replace, (value & MASK) << SHIFT, with > > (value & MASK) << ffs(MASK). I'll change to that way. Let me know > otherwise. > > yes and you may use the ones in bitfield.h > > > > what does it mean (lower 32 bit of Nth 4KB page) > > > > > > > Each descriptor can point up to 5 - 48bit address payloads. src_addr* > fields > > contain lower 32bit of 48bit address. Remaining upper 16bit is > programmed in > > addr_ext* fields. > > pls document this > > > > > +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? > > > > > > > There's not much configuration to be done. Channels are pretty much > identical > > with fixed configurations. > > hmmm, you do support DMA_SLAVE right? > Yes, and it's single direction, DMA_MEM_TO_DEV. > > > why have these wrappers which do not do anything? > > > > > > > It's just my personal preference to split into different code partitions, and > > each section is partitioned / grouped with some comment line. :-) > > Ex, a partition for struct dma_chan, and another one for > > struct xilinx_dpdma_chan. It gives me sort of abstracted view. But it may > be > > just me, and it comes with extra indirections. I'll remove unnecessary > wrappers. > > wrapper without any logic dont help > > > > > + 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? > > > > > > > Ok. You mean that an interrupt can occur right before > > xilinx_dpdma_disable_intr(), and the interrupt may be handled in the > middle or > > after removing this driver. I'll switch to request_irq() from > > devm_request_irq(), and remove the irq when removing the driver. > > yes and ensure tasklets are quiesced > > > > > +MODULE_AUTHOR("Xilinx, Inc."); > > > > +MODULE_DESCRIPTION("Xilinx DPDMA driver"); > > > > +MODULE_LICENSE("GPL v2"); > > > > > > No MODULE_ALIAS()? > > > > Is it required? Could you please elaborate, to help my understanding? > > did you try builing as modules and testing this? I'm testing it as a loadable kernel module. I'll address rest of your comments. Thanks, -hyun > > -- > ~Vinod
On Wed, Jan 24, 2018 at 05:45:00PM +0000, Hyun Kwon wrote: > > > > > +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? > > > > > > > > > > There's not much configuration to be done. Channels are pretty much > > identical > > > with fixed configurations. > > > > hmmm, you do support DMA_SLAVE right? > > > > Yes, and it's single direction, DMA_MEM_TO_DEV. Okay how do you find the DMA data width and the burst to apply when you DMA to device? Are these not programmable? I think you need to go back and check that part. Moreover a client may send you parameters and program the Device FIFO, you not honoring those will result in bad data received on Device, am sure you don't want that
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"; + ... + };