| Message ID | 1461152599-28858-1-git-send-email-appanad@xilinx.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Wed, Apr 20, 2016 at 5:13 PM, Kedareswara rao Appana <appana.durga.rao@xilinx.com> wrote: > Added basic clock support. The clocks are requested at probe > and released at remove. > > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> Reviewed-by: Shubhrajyoti Datta <shubhraj@xilinx.com> -- 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
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 Acked-by: Sören Brinkmann <soren.brinkmann@xilinx.com> Sören -- 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
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 -- 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
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 -- 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
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? 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- 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
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 >;
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(+)