From: Appana Durga Kedareswara Rao <appana.durga.rao@xilinx.com>
To: Soren Brinkmann <sorenb@xilinx.com>
CC: "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "pawel.moll@arm.com" <pawel.moll@arm.com>,
        "mark.rutland@arm.com" <mark.rutland@arm.com>,
        "ijc+devicetree@hellion.org.uk" <ijc+devicetree@hellion.org.uk>,
        "galak@codeaurora.org" <galak@codeaurora.org>,
        Michal Simek <michals@xilinx.com>,
        "vinod.koul@intel.com" <vinod.koul@intel.com>,
        "dan.j.williams@intel.com" <dan.j.williams@intel.com>,
        "moritz.fischer@ettus.com" <moritz.fischer@ettus.com>,
        "laurent.pinchart@ideasonboard.com" 
	<laurent.pinchart@ideasonboard.com>,
        "luis@debethencourt.com" <luis@debethencourt.com>,
        Anirudha Sarangi <anirudh@xilinx.com>,
        Punnaiah Choudary Kalluri <punnaia@xilinx.com>,
        Shubhrajyoti Datta <shubhraj@xilinx.com>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "dmaengine@vger.kernel.org" <dmaengine@vger.kernel.org>
Subject: RE: [PATCH v2 2/2] dmaengine: vdma: Add clock support
Thread-Topic: [PATCH v2 2/2] dmaengine: vdma: Add clock support
Thread-Index: AQHRmvngVicMewY8nUq2hjpLauS/L5+SZ/QAgACKF/A=
Date: Wed, 20 Apr 2016 14:55:27 +0000
Message-ID: <C246CAC1457055469EF09E3A7AC4E11A4A57930F@XAP-PVEXMBX01.xlnx.xilinx.com>
References: <1461152599-28858-1-git-send-email-appanad@xilinx.com>
 <1461152599-28858-2-git-send-email-appanad@xilinx.com>
 <20160420143622.GP7128@xsjsorenbubuntu>
In-Reply-To: <20160420143622.GP7128@xsjsorenbubuntu>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Apr 2016 14:55:31.7607
 (UTC)
X-MS-Exchange-CrossTenant-Id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=657af505-d5df-48d0-8300-c31994686c5c;Ip=[149.199.60.83];Helo=[xsj-pvapsmtpgw01]
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN1NAM02HT058
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id u3KEtix8003171
Content-Length: 5298
Lines: 150

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