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

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

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