[PATCH v2 1/2] Documentation: DT: vdma: Add clock support for vdma

[PATCH v2 1/2] Documentation: DT: vdma: Add clock support for vdma

[Kedareswara rao Appana]

This patch updates the binding doc with clock description
for vdma.

Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
---
Changes for v2:
--> Listed down all the clocks supported by the h/w
    as suggested by the Datta.
--> Used IP clock names instead of shortcut clock names.

 Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
index fcc2b65..afe9eb7 100644
--- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
+++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
@@ -21,6 +21,11 @@ Required properties:
 - dma-channel child node: Should have at least one channel and can have up to
 	two channels per device. This node specifies the properties of each
 	DMA channel (see child node properties below).
+- clocks: Input clock specifier. Refer to common clock bindings.
+- clock-names: List of input clocks "s_axi_lite_aclk", "m_axi_mm2s_aclk"
+	       "m_axi_s2mm_aclk", "m_axis_mm2s_aclk", "s_axis_s2mm_aclk"
+	       (list of input cloks may vary based on the ip configuration.
+		see clock bindings for more info).
 
 Required properties for VDMA:
 - xlnx,num-fstores: Should be the number of framebuffers as configured in h/w.
@@ -60,6 +65,9 @@ axi_vdma_0: axivdma@40030000 {
 	xlnx,num-fstores = <0x8>;
 	xlnx,flush-fsync = <0x1>;
 	xlnx,addrwidth = <0x20>;
+	clocks = <&clk 0>, <&clk 1>, <&clk 2>, <&clk 3>, <&clk 4>;
+	clock-names = "s_axi_lite_aclk", "m_axi_mm2s_aclk", "m_axi_s2mm_aclk",
+		      "m_axis_mm2s_aclk", "s_axis_s2mm_aclk";
 	dma-channel@40030000 {
 		compatible = "xlnx,axi-vdma-mm2s-channel";
 		interrupts = < 0 54 4 >;
-- 
2.1.2

[PATCH v2 2/2] dmaengine: vdma: Add clock support

[Kedareswara rao Appana]

Added basic clock support. The clocks are requested at probe
and released at remove.

Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
---
Changes for v2:
--> None.

 drivers/dma/xilinx/xilinx_vdma.c | 56 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/drivers/dma/xilinx/xilinx_vdma.c b/drivers/dma/xilinx/xilinx_vdma.c
index 70caea6..d526029 100644
--- a/drivers/dma/xilinx/xilinx_vdma.c
+++ b/drivers/dma/xilinx/xilinx_vdma.c
@@ -44,6 +44,7 @@
 #include <linux/of_platform.h>
 #include <linux/of_irq.h>
 #include <linux/slab.h>
+#include <linux/clk.h>
 
 #include "../dmaengine.h"
 
@@ -352,6 +353,8 @@ struct xilinx_dma_chan {
  * @flush_on_fsync: Flush on frame sync
  * @ext_addr: Indicates 64 bit addressing is supported by dma device
  * @dmatype: DMA ip type
+ * @clks:	pointer to array of clocks
+ * @numclks:	number of clocks available
  */
 struct xilinx_dma_device {
 	void __iomem *regs;
@@ -362,6 +365,8 @@ struct xilinx_dma_device {
 	u32 flush_on_fsync;
 	bool ext_addr;
 	enum xdma_ip_type dmatype;
+	struct clk **clks;
+	int numclks;
 };
 
 /* Macros */
@@ -1731,6 +1736,26 @@ int xilinx_vdma_channel_set_config(struct dma_chan *dchan,
 }
 EXPORT_SYMBOL(xilinx_vdma_channel_set_config);
 
+static int xdma_clk_init(struct xilinx_dma_device *xdev, bool enable)
+{
+	int i = 0, ret;
+
+	for (i = 0; i < xdev->numclks; i++) {
+		if (enable) {
+			ret = clk_prepare_enable(xdev->clks[i]);
+			if (ret) {
+				dev_err(xdev->dev,
+					"failed to enable the axidma clock\n");
+				return ret;
+			}
+		} else {
+			clk_disable_unprepare(xdev->clks[i]);
+		}
+	}
+
+	return 0;
+}
+
 /* -----------------------------------------------------------------------------
  * Probe and remove
  */
@@ -1919,6 +1944,7 @@ static int xilinx_dma_probe(struct platform_device *pdev)
 	struct resource *io;
 	u32 num_frames, addr_width;
 	int i, err;
+	const char *str;
 
 	/* Allocate and initialize the DMA engine structure */
 	xdev = devm_kzalloc(&pdev->dev, sizeof(*xdev), GFP_KERNEL);
@@ -1965,6 +1991,32 @@ static int xilinx_dma_probe(struct platform_device *pdev)
 	/* Set the dma mask bits */
 	dma_set_mask(xdev->dev, DMA_BIT_MASK(addr_width));
 
+	xdev->numclks = of_property_count_strings(pdev->dev.of_node,
+						  "clock-names");
+	if (xdev->numclks > 0) {
+		xdev->clks = devm_kmalloc_array(&pdev->dev, xdev->numclks,
+						sizeof(struct clk *),
+						GFP_KERNEL);
+		if (!xdev->clks)
+			return -ENOMEM;
+
+		for (i = 0; i < xdev->numclks; i++) {
+			of_property_read_string_index(pdev->dev.of_node,
+						      "clock-names", i, &str);
+			xdev->clks[i] = devm_clk_get(xdev->dev, str);
+			if (IS_ERR(xdev->clks[i])) {
+				if (PTR_ERR(xdev->clks[i]) == -ENOENT)
+					xdev->clks[i] = NULL;
+				else
+					return PTR_ERR(xdev->clks[i]);
+			}
+		}
+
+		err = xdma_clk_init(xdev, true);
+		if (err)
+			return err;
+	}
+
 	/* Initialize the DMA engine */
 	xdev->common.dev = &pdev->dev;
 
@@ -2025,6 +2077,8 @@ static int xilinx_dma_probe(struct platform_device *pdev)
 	return 0;
 
 error:
+	if (xdev->numclks > 0)
+		xdma_clk_init(xdev, false);
 	for (i = 0; i < XILINX_DMA_MAX_CHANS_PER_DEVICE; i++)
 		if (xdev->chan[i])
 			xilinx_dma_chan_remove(xdev->chan[i]);
@@ -2050,6 +2104,8 @@ static int xilinx_dma_remove(struct platform_device *pdev)
 	for (i = 0; i < XILINX_DMA_MAX_CHANS_PER_DEVICE; i++)
 		if (xdev->chan[i])
 			xilinx_dma_chan_remove(xdev->chan[i]);
+	if (xdev->numclks > 0)
+		xdma_clk_init(xdev, false);
 
 	return 0;
 }
-- 
2.1.2

Re: [PATCH v2 2/2] dmaengine: vdma: Add clock support

[Shubhrajyoti Datta]

On Wed, Apr 20, 2016 at 5:13 PM, Kedareswara rao Appana
<appana.durga.rao-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> wrote:
> Added basic clock support. The clocks are requested at probe
> and released at remove.
>
> Signed-off-by: Kedareswara rao Appana <appanad-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>


Reviewed-by: Shubhrajyoti Datta  <shubhraj-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] Documentation: DT: vdma: Add clock support for vdma

[Sören Brinkmann]

On Wed, 2016-04-20 at 17:13:18 +0530, Kedareswara rao Appana wrote:
> This patch updates the binding doc with clock description
> for vdma.
> 
> Signed-off-by: Kedareswara rao Appana <appanad-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> ---
> Changes for v2:
> --> Listed down all the clocks supported by the h/w
>     as suggested by the Datta.
> --> Used IP clock names instead of shortcut clock names.
> 
>  Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> index fcc2b65..afe9eb7 100644
> --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> @@ -21,6 +21,11 @@ Required properties:
>  - dma-channel child node: Should have at least one channel and can have up to
>  	two channels per device. This node specifies the properties of each
>  	DMA channel (see child node properties below).
> +- clocks: Input clock specifier. Refer to common clock bindings.
> +- clock-names: List of input clocks "s_axi_lite_aclk", "m_axi_mm2s_aclk"
> +	       "m_axi_s2mm_aclk", "m_axis_mm2s_aclk", "s_axis_s2mm_aclk"
> +	       (list of input cloks may vary based on the ip configuration.

s/cloks/clocks

Acked-by: Sören Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>

	Sören
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] dmaengine: vdma: Add clock support

[Sören Brinkmann]

On Wed, 2016-04-20 at 17:13:19 +0530, Kedareswara rao Appana wrote:
> Added basic clock support. The clocks are requested at probe
> and released at remove.
> 
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> ---
> Changes for v2:
> --> None.
> 
>  drivers/dma/xilinx/xilinx_vdma.c | 56 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/drivers/dma/xilinx/xilinx_vdma.c b/drivers/dma/xilinx/xilinx_vdma.c
> index 70caea6..d526029 100644
> --- a/drivers/dma/xilinx/xilinx_vdma.c
> +++ b/drivers/dma/xilinx/xilinx_vdma.c
> @@ -44,6 +44,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/of_irq.h>
>  #include <linux/slab.h>
> +#include <linux/clk.h>
>  
>  #include "../dmaengine.h"
>  
> @@ -352,6 +353,8 @@ struct xilinx_dma_chan {
>   * @flush_on_fsync: Flush on frame sync
>   * @ext_addr: Indicates 64 bit addressing is supported by dma device
>   * @dmatype: DMA ip type
> + * @clks:	pointer to array of clocks
> + * @numclks:	number of clocks available
>   */
>  struct xilinx_dma_device {
>  	void __iomem *regs;
> @@ -362,6 +365,8 @@ struct xilinx_dma_device {
>  	u32 flush_on_fsync;
>  	bool ext_addr;
>  	enum xdma_ip_type dmatype;
> +	struct clk **clks;
> +	int numclks;
>  };
>  
>  /* Macros */
> @@ -1731,6 +1736,26 @@ int xilinx_vdma_channel_set_config(struct dma_chan *dchan,
>  }
>  EXPORT_SYMBOL(xilinx_vdma_channel_set_config);
>  
> +static int xdma_clk_init(struct xilinx_dma_device *xdev, bool enable)
> +{
> +	int i = 0, ret;
> +
> +	for (i = 0; i < xdev->numclks; i++) {
> +		if (enable) {
> +			ret = clk_prepare_enable(xdev->clks[i]);
> +			if (ret) {
> +				dev_err(xdev->dev,
> +					"failed to enable the axidma clock\n");
> +				return ret;
> +			}
> +		} else {
> +			clk_disable_unprepare(xdev->clks[i]);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Probe and remove
>   */
> @@ -1919,6 +1944,7 @@ static int xilinx_dma_probe(struct platform_device *pdev)
>  	struct resource *io;
>  	u32 num_frames, addr_width;
>  	int i, err;
> +	const char *str;
>  
>  	/* Allocate and initialize the DMA engine structure */
>  	xdev = devm_kzalloc(&pdev->dev, sizeof(*xdev), GFP_KERNEL);
> @@ -1965,6 +1991,32 @@ static int xilinx_dma_probe(struct platform_device *pdev)
>  	/* Set the dma mask bits */
>  	dma_set_mask(xdev->dev, DMA_BIT_MASK(addr_width));
>  
> +	xdev->numclks = of_property_count_strings(pdev->dev.of_node,
> +						  "clock-names");
> +	if (xdev->numclks > 0) {
> +		xdev->clks = devm_kmalloc_array(&pdev->dev, xdev->numclks,
> +						sizeof(struct clk *),
> +						GFP_KERNEL);
> +		if (!xdev->clks)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < xdev->numclks; i++) {
> +			of_property_read_string_index(pdev->dev.of_node,
> +						      "clock-names", i, &str);
> +			xdev->clks[i] = devm_clk_get(xdev->dev, str);
> +			if (IS_ERR(xdev->clks[i])) {
> +				if (PTR_ERR(xdev->clks[i]) == -ENOENT)
> +					xdev->clks[i] = NULL;
> +				else
> +					return PTR_ERR(xdev->clks[i]);
> +			}
> +		}
> +
> +		err = xdma_clk_init(xdev, true);
> +		if (err)
> +			return err;
> +	}

I guess this works, but the relation to the binding is a little loose,
IMHO. Instead of using the clock names from the binding this is just
using whatever is provided in DT and enabling it. Also, all the clocks
here are handled as optional feature, while binding - and I guess
reality too - have them as mandatory.
It would be nicer if the driver specifically asks for the clocks it
needs and return an error if a mandatory clock is missing.

	Sören

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 1/2] Documentation: DT: vdma: Add clock support for vdma

[Appana Durga Kedareswara Rao]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2651 bytes --]

Hi Soren,	

> -----Original Message-----
> From: Sören Brinkmann [mailto:soren.brinkmann@xilinx.com]
> Sent: Wednesday, April 20, 2016 7:58 PM
> To: Appana Durga Kedareswara Rao <appanad@xilinx.com>
> Cc: robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com;
> ijc+devicetree@hellion.org.uk; galak@codeaurora.org; Michal Simek
> <michals@xilinx.com>; vinod.koul@intel.com; dan.j.williams@intel.com;
> Appana Durga Kedareswara Rao <appanad@xilinx.com>;
> moritz.fischer@ettus.com; laurent.pinchart@ideasonboard.com;
> luis@debethencourt.com; Anirudha Sarangi <anirudh@xilinx.com>; Punnaiah
> Choudary Kalluri <punnaia@xilinx.com>; Shubhrajyoti Datta
> <shubhraj@xilinx.com>; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> dmaengine@vger.kernel.org
> Subject: Re: [PATCH v2 1/2] Documentation: DT: vdma: Add clock support for
> vdma
> 
> On Wed, 2016-04-20 at 17:13:18 +0530, Kedareswara rao Appana wrote:
> > This patch updates the binding doc with clock description for vdma.
> >
> > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> > ---
> > Changes for v2:
> > --> Listed down all the clocks supported by the h/w
> >     as suggested by the Datta.
> > --> Used IP clock names instead of shortcut clock names.
> >
> >  Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt | 8
> > ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> > b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> > index fcc2b65..afe9eb7 100644
> > --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> > +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> > @@ -21,6 +21,11 @@ Required properties:
> >  - dma-channel child node: Should have at least one channel and can have up
> to
> >  	two channels per device. This node specifies the properties of each
> >  	DMA channel (see child node properties below).
> > +- clocks: Input clock specifier. Refer to common clock bindings.
> > +- clock-names: List of input clocks "s_axi_lite_aclk", "m_axi_mm2s_aclk"
> > +	       "m_axi_s2mm_aclk", "m_axis_mm2s_aclk", "s_axis_s2mm_aclk"
> > +	       (list of input cloks may vary based on the ip configuration.
> 
> s/cloks/clocks

Ok will fix in next version...
> 
> Acked-by: Sören Brinkmann <soren.brinkmann@xilinx.com>

Thanks for the ack...

Regards,
Kedar.

> 
> 	Sören
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·zøœzÚÞz)í…æèw*\x1fjg¬±¨\x1e¶‰šŽŠÝ¢j.ïÛ°\½½MŽúgjÌæa×\x02››–' ™©Þ¢¸\f¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾\a«‘êçzZ+ƒùšŽŠÝ¢j"ú!¶i

RE: [PATCH v2 2/2] dmaengine: vdma: Add clock support

[Appana Durga Kedareswara Rao]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 5582 bytes --]

Hi Soren,

> -----Original Message-----
> From: Sören Brinkmann [mailto:soren.brinkmann@xilinx.com]
> Sent: Wednesday, April 20, 2016 8:06 PM
> To: Appana Durga Kedareswara Rao <appanad@xilinx.com>
> Cc: robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com;
> ijc+devicetree@hellion.org.uk; galak@codeaurora.org; Michal Simek
> <michals@xilinx.com>; vinod.koul@intel.com; dan.j.williams@intel.com;
> Appana Durga Kedareswara Rao <appanad@xilinx.com>;
> moritz.fischer@ettus.com; laurent.pinchart@ideasonboard.com;
> luis@debethencourt.com; Anirudha Sarangi <anirudh@xilinx.com>; Punnaiah
> Choudary Kalluri <punnaia@xilinx.com>; Shubhrajyoti Datta
> <shubhraj@xilinx.com>; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> dmaengine@vger.kernel.org
> Subject: Re: [PATCH v2 2/2] dmaengine: vdma: Add clock support
> 
> On Wed, 2016-04-20 at 17:13:19 +0530, Kedareswara rao Appana wrote:
> > Added basic clock support. The clocks are requested at probe and
> > released at remove.
> >
> > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> > ---
> > Changes for v2:
> > --> None.
> >
> >  drivers/dma/xilinx/xilinx_vdma.c | 56
> > ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> >
> > diff --git a/drivers/dma/xilinx/xilinx_vdma.c
> > b/drivers/dma/xilinx/xilinx_vdma.c
> > index 70caea6..d526029 100644
> > --- a/drivers/dma/xilinx/xilinx_vdma.c
> > +++ b/drivers/dma/xilinx/xilinx_vdma.c
> > @@ -44,6 +44,7 @@
> >  #include <linux/of_platform.h>
> >  #include <linux/of_irq.h>
> >  #include <linux/slab.h>
> > +#include <linux/clk.h>
> >
> >  #include "../dmaengine.h"
> >
> > @@ -352,6 +353,8 @@ struct xilinx_dma_chan {
> >   * @flush_on_fsync: Flush on frame sync
> >   * @ext_addr: Indicates 64 bit addressing is supported by dma device
> >   * @dmatype: DMA ip type
> > + * @clks:	pointer to array of clocks
> > + * @numclks:	number of clocks available
> >   */
> >  struct xilinx_dma_device {
> >  	void __iomem *regs;
> > @@ -362,6 +365,8 @@ struct xilinx_dma_device {
> >  	u32 flush_on_fsync;
> >  	bool ext_addr;
> >  	enum xdma_ip_type dmatype;
> > +	struct clk **clks;
> > +	int numclks;
> >  };
> >
> >  /* Macros */
> > @@ -1731,6 +1736,26 @@ int xilinx_vdma_channel_set_config(struct
> > dma_chan *dchan,  }  EXPORT_SYMBOL(xilinx_vdma_channel_set_config);
> >
> > +static int xdma_clk_init(struct xilinx_dma_device *xdev, bool enable)
> > +{
> > +	int i = 0, ret;
> > +
> > +	for (i = 0; i < xdev->numclks; i++) {
> > +		if (enable) {
> > +			ret = clk_prepare_enable(xdev->clks[i]);
> > +			if (ret) {
> > +				dev_err(xdev->dev,
> > +					"failed to enable the axidma clock\n");
> > +				return ret;
> > +			}
> > +		} else {
> > +			clk_disable_unprepare(xdev->clks[i]);
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /* -----------------------------------------------------------------------------
> >   * Probe and remove
> >   */
> > @@ -1919,6 +1944,7 @@ static int xilinx_dma_probe(struct platform_device
> *pdev)
> >  	struct resource *io;
> >  	u32 num_frames, addr_width;
> >  	int i, err;
> > +	const char *str;
> >
> >  	/* Allocate and initialize the DMA engine structure */
> >  	xdev = devm_kzalloc(&pdev->dev, sizeof(*xdev), GFP_KERNEL); @@
> > -1965,6 +1991,32 @@ static int xilinx_dma_probe(struct platform_device
> *pdev)
> >  	/* Set the dma mask bits */
> >  	dma_set_mask(xdev->dev, DMA_BIT_MASK(addr_width));
> >
> > +	xdev->numclks = of_property_count_strings(pdev->dev.of_node,
> > +						  "clock-names");
> > +	if (xdev->numclks > 0) {
> > +		xdev->clks = devm_kmalloc_array(&pdev->dev, xdev->numclks,
> > +						sizeof(struct clk *),
> > +						GFP_KERNEL);
> > +		if (!xdev->clks)
> > +			return -ENOMEM;
> > +
> > +		for (i = 0; i < xdev->numclks; i++) {
> > +			of_property_read_string_index(pdev->dev.of_node,
> > +						      "clock-names", i, &str);
> > +			xdev->clks[i] = devm_clk_get(xdev->dev, str);
> > +			if (IS_ERR(xdev->clks[i])) {
> > +				if (PTR_ERR(xdev->clks[i]) == -ENOENT)
> > +					xdev->clks[i] = NULL;
> > +				else
> > +					return PTR_ERR(xdev->clks[i]);
> > +			}
> > +		}
> > +
> > +		err = xdma_clk_init(xdev, true);
> > +		if (err)
> > +			return err;
> > +	}
> 
> I guess this works, but the relation to the binding is a little loose, IMHO. Instead
> of using the clock names from the binding this is just using whatever is provided
> in DT and enabling it. Also, all the clocks here are handled as optional feature,
> while binding - and I guess reality too - have them as mandatory.
> It would be nicer if the driver specifically asks for the clocks it needs and return
> an error if a mandatory clock is missing.

Here DMA channels are configurable I mean if user select only mm2s channel then we will have
Only 3 clocks. If user selects both mm2s and s2mm channels we will have 5 clocks.
That’s why reading the number of clocks from the clock-names property.

And also the AXI DMA/CDMA Ip support also getting added to this driver for those IP's also the clocks are configurable
For AXI DMA it is either 3 or 4 clocks and for AXI CDMA it is 2 clocks.

That's why I thought it is the proper solution.

If you have any better idea please let me know will try in that way...

Regards,
Kedar.
> 
> 	Sören
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·zøœzÚÞz)í…æèw*\x1fjg¬±¨\x1e¶‰šŽŠÝ¢j.ïÛ°\½½MŽúgjÌæa×\x02››–' ™©Þ¢¸\f¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾\a«‘êçzZ+ƒùšŽŠÝ¢j"ú!¶i

Re: [PATCH v2 2/2] dmaengine: vdma: Add clock support

[Sören Brinkmann]

On Wed, 2016-04-20 at 07:55:27 -0700, Appana Durga Kedareswara Rao wrote:
> Hi Soren,
> 
> > -----Original Message-----
> > From: Sören Brinkmann [mailto:soren.brinkmann@xilinx.com]
> > Sent: Wednesday, April 20, 2016 8:06 PM
> > To: Appana Durga Kedareswara Rao <appanad@xilinx.com>
> > Cc: robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com;
> > ijc+devicetree@hellion.org.uk; galak@codeaurora.org; Michal Simek
> > <michals@xilinx.com>; vinod.koul@intel.com; dan.j.williams@intel.com;
> > Appana Durga Kedareswara Rao <appanad@xilinx.com>;
> > moritz.fischer@ettus.com; laurent.pinchart@ideasonboard.com;
> > luis@debethencourt.com; Anirudha Sarangi <anirudh@xilinx.com>; Punnaiah
> > Choudary Kalluri <punnaia@xilinx.com>; Shubhrajyoti Datta
> > <shubhraj@xilinx.com>; devicetree@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > dmaengine@vger.kernel.org
> > Subject: Re: [PATCH v2 2/2] dmaengine: vdma: Add clock support
> > 
> > On Wed, 2016-04-20 at 17:13:19 +0530, Kedareswara rao Appana wrote:
> > > Added basic clock support. The clocks are requested at probe and
> > > released at remove.
> > >
> > > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> > > ---
> > > Changes for v2:
> > > --> None.
> > >
> > >  drivers/dma/xilinx/xilinx_vdma.c | 56
> > > ++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 56 insertions(+)
> > >
> > > diff --git a/drivers/dma/xilinx/xilinx_vdma.c
> > > b/drivers/dma/xilinx/xilinx_vdma.c
> > > index 70caea6..d526029 100644
> > > --- a/drivers/dma/xilinx/xilinx_vdma.c
> > > +++ b/drivers/dma/xilinx/xilinx_vdma.c
> > > @@ -44,6 +44,7 @@
> > >  #include <linux/of_platform.h>
> > >  #include <linux/of_irq.h>
> > >  #include <linux/slab.h>
> > > +#include <linux/clk.h>
> > >
> > >  #include "../dmaengine.h"
> > >
> > > @@ -352,6 +353,8 @@ struct xilinx_dma_chan {
> > >   * @flush_on_fsync: Flush on frame sync
> > >   * @ext_addr: Indicates 64 bit addressing is supported by dma device
> > >   * @dmatype: DMA ip type
> > > + * @clks:	pointer to array of clocks
> > > + * @numclks:	number of clocks available
> > >   */
> > >  struct xilinx_dma_device {
> > >  	void __iomem *regs;
> > > @@ -362,6 +365,8 @@ struct xilinx_dma_device {
> > >  	u32 flush_on_fsync;
> > >  	bool ext_addr;
> > >  	enum xdma_ip_type dmatype;
> > > +	struct clk **clks;
> > > +	int numclks;
> > >  };
> > >
> > >  /* Macros */
> > > @@ -1731,6 +1736,26 @@ int xilinx_vdma_channel_set_config(struct
> > > dma_chan *dchan,  }  EXPORT_SYMBOL(xilinx_vdma_channel_set_config);
> > >
> > > +static int xdma_clk_init(struct xilinx_dma_device *xdev, bool enable)
> > > +{
> > > +	int i = 0, ret;
> > > +
> > > +	for (i = 0; i < xdev->numclks; i++) {
> > > +		if (enable) {
> > > +			ret = clk_prepare_enable(xdev->clks[i]);
> > > +			if (ret) {
> > > +				dev_err(xdev->dev,
> > > +					"failed to enable the axidma clock\n");
> > > +				return ret;
> > > +			}
> > > +		} else {
> > > +			clk_disable_unprepare(xdev->clks[i]);
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  /* -----------------------------------------------------------------------------
> > >   * Probe and remove
> > >   */
> > > @@ -1919,6 +1944,7 @@ static int xilinx_dma_probe(struct platform_device
> > *pdev)
> > >  	struct resource *io;
> > >  	u32 num_frames, addr_width;
> > >  	int i, err;
> > > +	const char *str;
> > >
> > >  	/* Allocate and initialize the DMA engine structure */
> > >  	xdev = devm_kzalloc(&pdev->dev, sizeof(*xdev), GFP_KERNEL); @@
> > > -1965,6 +1991,32 @@ static int xilinx_dma_probe(struct platform_device
> > *pdev)
> > >  	/* Set the dma mask bits */
> > >  	dma_set_mask(xdev->dev, DMA_BIT_MASK(addr_width));
> > >
> > > +	xdev->numclks = of_property_count_strings(pdev->dev.of_node,
> > > +						  "clock-names");
> > > +	if (xdev->numclks > 0) {
> > > +		xdev->clks = devm_kmalloc_array(&pdev->dev, xdev->numclks,
> > > +						sizeof(struct clk *),
> > > +						GFP_KERNEL);
> > > +		if (!xdev->clks)
> > > +			return -ENOMEM;
> > > +
> > > +		for (i = 0; i < xdev->numclks; i++) {
> > > +			of_property_read_string_index(pdev->dev.of_node,
> > > +						      "clock-names", i, &str);
> > > +			xdev->clks[i] = devm_clk_get(xdev->dev, str);
> > > +			if (IS_ERR(xdev->clks[i])) {
> > > +				if (PTR_ERR(xdev->clks[i]) == -ENOENT)
> > > +					xdev->clks[i] = NULL;
> > > +				else
> > > +					return PTR_ERR(xdev->clks[i]);
> > > +			}
> > > +		}
> > > +
> > > +		err = xdma_clk_init(xdev, true);
> > > +		if (err)
> > > +			return err;
> > > +	}
> > 
> > I guess this works, but the relation to the binding is a little loose, IMHO. Instead
> > of using the clock names from the binding this is just using whatever is provided
> > in DT and enabling it. Also, all the clocks here are handled as optional feature,
> > while binding - and I guess reality too - have them as mandatory.
> > It would be nicer if the driver specifically asks for the clocks it needs and return
> > an error if a mandatory clock is missing.
> 
> Here DMA channels are configurable I mean if user select only mm2s channel then we will have
> Only 3 clocks. If user selects both mm2s and s2mm channels we will have 5 clocks.
> That’s why reading the number of clocks from the clock-names property.
> 
> And also the AXI DMA/CDMA Ip support also getting added to this driver for those IP's also the clocks are configurable
> For AXI DMA it is either 3 or 4 clocks and for AXI CDMA it is 2 clocks.
> 
> That's why I thought it is the proper solution.
> 
> If you have any better idea please let me know will try in that way...

I guess the driver know all these things: whether it controls vdma or
cdma, what interfaces it has and how many channels? Based on that, I
guess it should be possible to derive what clocks are required for
correct operation.

	Sören

Re: [PATCH v2 1/2] Documentation: DT: vdma: Add clock support for vdma

[Rob Herring]

On Wed, Apr 20, 2016 at 05:13:18PM +0530, Kedareswara rao Appana wrote:
> This patch updates the binding doc with clock description
> for vdma.
> 
> Signed-off-by: Kedareswara rao Appana <appanad-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> ---
> Changes for v2:
> --> Listed down all the clocks supported by the h/w
>     as suggested by the Datta.
> --> Used IP clock names instead of shortcut clock names.
> 
>  Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> index fcc2b65..afe9eb7 100644
> --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> @@ -21,6 +21,11 @@ Required properties:
>  - dma-channel child node: Should have at least one channel and can have up to
>  	two channels per device. This node specifies the properties of each
>  	DMA channel (see child node properties below).
> +- clocks: Input clock specifier. Refer to common clock bindings.
> +- clock-names: List of input clocks "s_axi_lite_aclk", "m_axi_mm2s_aclk"
> +	       "m_axi_s2mm_aclk", "m_axis_mm2s_aclk", "s_axis_s2mm_aclk"
> +	       (list of input cloks may vary based on the ip configuration.

s/cloks/clocks/

> +		see clock bindings for more info).

This does not make sense. The common clock binding is going to tell me 
more about how the clocks vary?

You need to define here how the clocks can vary.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 1/2] Documentation: DT: vdma: Add clock support for vdma

[Appana Durga Kedareswara Rao]

Hi Rob,

	Thanks for the review...

> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: Saturday, April 23, 2016 1:07 AM
> To: Appana Durga Kedareswara Rao <appanad@xilinx.com>
> Cc: pawel.moll@arm.com; mark.rutland@arm.com;
> ijc+devicetree@hellion.org.uk; galak@codeaurora.org; Michal Simek
> <michals@xilinx.com>; Soren Brinkmann <sorenb@xilinx.com>;
> vinod.koul@intel.com; dan.j.williams@intel.com; Appana Durga Kedareswara
> Rao <appanad@xilinx.com>; moritz.fischer@ettus.com;
> laurent.pinchart@ideasonboard.com; luis@debethencourt.com; Anirudha
> Sarangi <anirudh@xilinx.com>; Punnaiah Choudary Kalluri
> <punnaia@xilinx.com>; Shubhrajyoti Datta <shubhraj@xilinx.com>;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; dmaengine@vger.kernel.org
> Subject: Re: [PATCH v2 1/2] Documentation: DT: vdma: Add clock support for
> vdma
> 
> On Wed, Apr 20, 2016 at 05:13:18PM +0530, Kedareswara rao Appana wrote:
> > This patch updates the binding doc with clock description for vdma.
> >
> > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> > ---
> > Changes for v2:
> > --> Listed down all the clocks supported by the h/w
> >     as suggested by the Datta.
> > --> Used IP clock names instead of shortcut clock names.
> >
> >  Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt | 8
> > ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> > b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> > index fcc2b65..afe9eb7 100644
> > --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> > +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> > @@ -21,6 +21,11 @@ Required properties:
> >  - dma-channel child node: Should have at least one channel and can have up
> to
> >  	two channels per device. This node specifies the properties of each
> >  	DMA channel (see child node properties below).
> > +- clocks: Input clock specifier. Refer to common clock bindings.
> > +- clock-names: List of input clocks "s_axi_lite_aclk", "m_axi_mm2s_aclk"
> > +	       "m_axi_s2mm_aclk", "m_axis_mm2s_aclk", "s_axis_s2mm_aclk"
> > +	       (list of input cloks may vary based on the ip configuration.
> 
> s/cloks/clocks/
> 
> > +		see clock bindings for more info).
> 
> This does not make sense. The common clock binding is going to tell me more
> about how the clocks vary?

I have fixed these comments in the other version (v4) you acked that patch...

Regards,
Kedar.

> 
> You need to define here how the clocks can vary.