[PATCH 0/2] xilinx_dma: Add external reset control

[PATCH 0/2] xilinx_dma: Add external reset control

[Ramiro Oliveira]

This patchset adds support for controlling an external reset line. Since
this reset line is optional it won't break compatibility.

Ramiro Oliveira (2):
  xilinx_dma: Edit device tree bindings documentation
  xilinx_dma: Add reset support

 .../devicetree/bindings/dma/xilinx/xilinx_dma.txt  |  6 ++++++
 drivers/dma/xilinx/xilinx_dma.c                    | 23 ++++++++++++++++++++++
 2 files changed, 29 insertions(+)

-- 
2.10.2


--
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

[PATCH 1/2] xilinx_dma: Edit device tree bindings documentation

[Ramiro Oliveira]

Add reset property documentation for Xilinx DMA

Signed-off-by: Ramiro Oliveira <roliveir@synopsys.com>
---
 Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
index a2b8bfa..7ebce72 100644
--- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
+++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
@@ -40,6 +40,10 @@ Required properties for VDMA:
 Optional properties:
 - xlnx,include-sg: Tells configured for Scatter-mode in
 	the hardware.
+- resets : Must contain an entry for each entry in reset-names.
+  See ../reset/reset.txt for details.
+- reset-names : Must include the following entries:
+  - reset
 Optional properties for AXI DMA:
 - xlnx,mcdma: Tells whether configured for multi-channel mode in the hardware.
 Optional properties for VDMA:
@@ -83,6 +87,8 @@ axi_vdma_0: axivdma@40030000 {
 	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";
+	resets = <&rst 2>;
+	reset-names = "reset";
 	dma-channel@40030000 {
 		compatible = "xlnx,axi-vdma-mm2s-channel";
 		interrupts = < 0 54 4 >;
-- 
2.10.2

Re: [PATCH 1/2] xilinx_dma: Edit device tree bindings documentation

[Laurent Pinchart]

Hi Ramiro,

Thank you for the patch.

On Wednesday 14 Dec 2016 17:18:23 Ramiro Oliveira wrote:
> Add reset property documentation for Xilinx DMA
> 
> Signed-off-by: Ramiro Oliveira <roliveir@synopsys.com>
> ---
>  Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt index
> a2b8bfa..7ebce72 100644
> --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> @@ -40,6 +40,10 @@ Required properties for VDMA:
>  Optional properties:
>  - xlnx,include-sg: Tells configured for Scatter-mode in
>  	the hardware.
> +- resets : Must contain an entry for each entry in reset-names.
> +  See ../reset/reset.txt for details.
> +- reset-names : Must include the following entries:
> +  - reset

If the IP core has a single reset, can't we omit the name ?

>  Optional properties for AXI DMA:
>  - xlnx,mcdma: Tells whether configured for multi-channel mode in the
> hardware. Optional properties for VDMA:
> @@ -83,6 +87,8 @@ axi_vdma_0: axivdma@40030000 {
>  	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";
> +	resets = <&rst 2>;
> +	reset-names = "reset";
>  	dma-channel@40030000 {
>  		compatible = "xlnx,axi-vdma-mm2s-channel";
>  		interrupts = < 0 54 4 >;

-- 
Regards,

Laurent Pinchart

[PATCH 2/2] xilinx_dma: Add reset support

[Ramiro Oliveira]

Add a DT property to control an optional external reset line

Signed-off-by: Ramiro Oliveira <roliveir-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
---
 drivers/dma/xilinx/xilinx_dma.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index 5c9f11b..b845224 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -46,6 +46,7 @@
 #include <linux/slab.h>
 #include <linux/clk.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/reset.h>
 
 #include "../dmaengine.h"
 
@@ -409,6 +410,7 @@ struct xilinx_dma_device {
 	struct clk *rxs_clk;
 	u32 nr_channels;
 	u32 chan_id;
+	struct reset_control *rst;
 };
 
 /* Macros */
@@ -2543,6 +2545,27 @@ static int xilinx_dma_probe(struct platform_device *pdev)
 	if (IS_ERR(xdev->regs))
 		return PTR_ERR(xdev->regs);
 
+	xdev->rst = devm_reset_control_get_optional(&pdev->dev, "reset");
+	if (IS_ERR(xdev->rst)) {
+		err = PTR_ERR(xdev->rst);
+		switch (err) {
+		case -ENOENT:
+		case -ENOTSUPP:
+			xdev->rst = NULL;
+		break;
+		default:
+			dev_err(xdev->dev, "error getting reset %d\n", err);
+		return err;
+		}
+	} else {
+		err = reset_control_deassert(xdev->rst);
+		if (err) {
+			dev_err(xdev->dev, "failed to deassert reset: %d\n",
+					err);
+			return err;
+		}
+	}
+
 	/* Retrieve the DMA engine properties from the device tree */
 	xdev->has_sg = of_property_read_bool(node, "xlnx,include-sg");
 	if (xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA)
-- 
2.10.2


--
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 2/2] xilinx_dma: Add reset support

[Laurent Pinchart]

Hi Ramiro,

Thank you for the patch.

On Wednesday 14 Dec 2016 17:18:24 Ramiro Oliveira wrote:
> Add a DT property to control an optional external reset line
> 
> Signed-off-by: Ramiro Oliveira <roliveir@synopsys.com>
> ---
>  drivers/dma/xilinx/xilinx_dma.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c
> b/drivers/dma/xilinx/xilinx_dma.c index 5c9f11b..b845224 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -46,6 +46,7 @@
>  #include <linux/slab.h>
>  #include <linux/clk.h>
>  #include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/reset.h>

I had neatly sorted the header alphabetically until someone added clk.h and 
io-64-nonatomic-lo-hi.h :-( Could you please move reset.h just before slab.h ?

> 
>  #include "../dmaengine.h"
> 
> @@ -409,6 +410,7 @@ struct xilinx_dma_device {
>  	struct clk *rxs_clk;
>  	u32 nr_channels;
>  	u32 chan_id;
> +	struct reset_control *rst;
>  };
> 
>  /* Macros */
> @@ -2543,6 +2545,27 @@ static int xilinx_dma_probe(struct platform_device
> *pdev) if (IS_ERR(xdev->regs))
>  		return PTR_ERR(xdev->regs);
> 
> +	xdev->rst = devm_reset_control_get_optional(&pdev->dev, "reset");

devm_reset_control_get_optional() is deprecated as explained in linux/reset.h, 
you should use devm_reset_control_get_optional_exclusive() or 
devm_reset_control_get_optional_shared() instead, as applicable.

This being said, I'm wondering why the optional versions of those functions 
exist, as they do exactly the same as the non-optional versions. The API feels 
wrong, it should have been modelled like the GPIO API. Feel free to fix it if 
you want :-) But that's out of scope for this patch.

> +	if (IS_ERR(xdev->rst)) {
> +		err = PTR_ERR(xdev->rst);
> +		switch (err) {
> +		case -ENOENT:

If you drop the name as proposed in the review of patch 1/2 you don't have to 
check for -ENOENT.

> +		case -ENOTSUPP:
> +			xdev->rst = NULL;
> +		break;

Wrong indentation.

You need to handle -EPROBE_DEFER and defer probing of the xilinx_dma device.

> +		default:
> +			dev_err(xdev->dev, "error getting reset %d\n", err);
> +		return err;

Wrong indentation.

> +		}
> +	} else {
> +		err = reset_control_deassert(xdev->rst);
> +		if (err) {
> +			dev_err(xdev->dev, "failed to deassert reset: %d\n",
> +					err);

Wrong indentation.

> +			return err;
> +		}
> +	}
> +
>  	/* Retrieve the DMA engine properties from the device tree */
>  	xdev->has_sg = of_property_read_bool(node, "xlnx,include-sg");
>  	if (xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] xilinx_dma: Add reset support

[Ramiro Oliveira]

Hi Laurent,

Thank you for your feedback.

On 12/14/2016 8:16 PM, Laurent Pinchart wrote:
> Hi Ramiro,
> 
> Thank you for the patch.
> 
> On Wednesday 14 Dec 2016 17:18:24 Ramiro Oliveira wrote:
>> Add a DT property to control an optional external reset line
>>
>> Signed-off-by: Ramiro Oliveira <roliveir-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
>> ---
>>  drivers/dma/xilinx/xilinx_dma.c | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/dma/xilinx/xilinx_dma.c
>> b/drivers/dma/xilinx/xilinx_dma.c index 5c9f11b..b845224 100644
>> --- a/drivers/dma/xilinx/xilinx_dma.c
>> +++ b/drivers/dma/xilinx/xilinx_dma.c
>> @@ -46,6 +46,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/clk.h>
>>  #include <linux/io-64-nonatomic-lo-hi.h>
>> +#include <linux/reset.h>
> 
> I had neatly sorted the header alphabetically until someone added clk.h and 
> io-64-nonatomic-lo-hi.h :-( Could you please move reset.h just before slab.h ?
> 

Sure. Actually I was tempted to reorder it, but I decided not to do it. I'll do
it now

>>
>>  #include "../dmaengine.h"
>>
>> @@ -409,6 +410,7 @@ struct xilinx_dma_device {
>>  	struct clk *rxs_clk;
>>  	u32 nr_channels;
>>  	u32 chan_id;
>> +	struct reset_control *rst;
>>  };
>>
>>  /* Macros */
>> @@ -2543,6 +2545,27 @@ static int xilinx_dma_probe(struct platform_device
>> *pdev) if (IS_ERR(xdev->regs))
>>  		return PTR_ERR(xdev->regs);
>>
>> +	xdev->rst = devm_reset_control_get_optional(&pdev->dev, "reset");
> 
> devm_reset_control_get_optional() is deprecated as explained in linux/reset.h, 
> you should use devm_reset_control_get_optional_exclusive() or 
> devm_reset_control_get_optional_shared() instead, as applicable.
> 
> This being said, I'm wondering why the optional versions of those functions 
> exist, as they do exactly the same as the non-optional versions. The API feels 
> wrong, it should have been modelled like the GPIO API. Feel free to fix it if 
> you want :-) But that's out of scope for this patch.
> 

I missed the comment stating that devm_reset_control_get_optional() was deprecated.

I could fix it. Your sugestion is modelling these functions like the GPIO API?

>> +	if (IS_ERR(xdev->rst)) {
>> +		err = PTR_ERR(xdev->rst);
>> +		switch (err) {
>> +		case -ENOENT:
> 
> If you drop the name as proposed in the review of patch 1/2 you don't have to 
> check for -ENOENT.
> 

I'll do that.

>> +		case -ENOTSUPP:
>> +			xdev->rst = NULL;
>> +		break;
> 
> Wrong indentation.
> 
> You need to handle -EPROBE_DEFER and defer probing of the xilinx_dma device.
> 
>> +		default:
>> +			dev_err(xdev->dev, "error getting reset %d\n", err);
>> +		return err;
> 
> Wrong indentation.
> 
>> +		}
>> +	} else {
>> +		err = reset_control_deassert(xdev->rst);
>> +		if (err) {
>> +			dev_err(xdev->dev, "failed to deassert reset: %d\n",
>> +					err);
> 
> Wrong indentation.
> 
>> +			return err;
>> +		}
>> +	}
>> +
>>  	/* Retrieve the DMA engine properties from the device tree */
>>  	xdev->has_sg = of_property_read_bool(node, "xlnx,include-sg");
>>  	if (xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA)
> 
--
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 2/2] xilinx_dma: Add reset support

[Laurent Pinchart]

Hi Ramiro,

(CC'ing Philipp Zabel)

On Thursday 15 Dec 2016 11:26:54 Ramiro Oliveira wrote:
> On 12/14/2016 8:16 PM, Laurent Pinchart wrote:
> > Hi Ramiro,
> > 
> > Thank you for the patch.
> > 
> > On Wednesday 14 Dec 2016 17:18:24 Ramiro Oliveira wrote:
> >> Add a DT property to control an optional external reset line
> >> 
> >> Signed-off-by: Ramiro Oliveira <roliveir-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> >> ---
> >> 
> >>  drivers/dma/xilinx/xilinx_dma.c | 23 +++++++++++++++++++++++
> >>  1 file changed, 23 insertions(+)
> >> 
> >> diff --git a/drivers/dma/xilinx/xilinx_dma.c
> >> b/drivers/dma/xilinx/xilinx_dma.c index 5c9f11b..b845224 100644
> >> --- a/drivers/dma/xilinx/xilinx_dma.c
> >> +++ b/drivers/dma/xilinx/xilinx_dma.c
> >> @@ -46,6 +46,7 @@
> >>  #include <linux/slab.h>
> >>  #include <linux/clk.h>
> >>  #include <linux/io-64-nonatomic-lo-hi.h>
> >> +#include <linux/reset.h>
> > 
> > I had neatly sorted the header alphabetically until someone added clk.h
> > and io-64-nonatomic-lo-hi.h :-( Could you please move reset.h just before
> > slab.h ?
>
> Sure. Actually I was tempted to reorder it, but I decided not to do it. I'll
> do it now

Yeah, I'll sleep better tonight :-D

> >>  #include "../dmaengine.h"
> >> 
> >> @@ -409,6 +410,7 @@ struct xilinx_dma_device {
> >>  	struct clk *rxs_clk;
> >>  	u32 nr_channels;
> >>  	u32 chan_id;
> >> +	struct reset_control *rst;
> >>  };
> >>  
> >>  /* Macros */
> >> @@ -2543,6 +2545,27 @@ static int xilinx_dma_probe(struct platform_device
> >> *pdev) if (IS_ERR(xdev->regs))
> >>  		return PTR_ERR(xdev->regs);
> >> 
> >> +	xdev->rst = devm_reset_control_get_optional(&pdev->dev, "reset");
> > 
> > devm_reset_control_get_optional() is deprecated as explained in
> > linux/reset.h, you should use devm_reset_control_get_optional_exclusive()
> > or devm_reset_control_get_optional_shared() instead, as applicable.
> > 
> > This being said, I'm wondering why the optional versions of those
> > functions exist, as they do exactly the same as the non-optional versions.
> > The API feels wrong, it should have been modelled like the GPIO API. Feel
> > free to fix it if you want :-) But that's out of scope for this patch.
> 
> I missed the comment stating that devm_reset_control_get_optional() was
> deprecated.
> 
> I could fix it. Your sugestion is modelling these functions like the GPIO
> API?

I think it would be better for driver if the _get_optional functions would 
return an ERR_PTR() for errors and NULL when reset control is not available, 
and then have the rest of the reset controller API accept NULL as a no-op. 
Your implementation would then be

	xdev->rst = devm_reset_control_get_optional_exclusive(&pdev->dev,
							      "reset");
	if (IS_ERR(xdev->rst)) {
		err = PTR_ERR(xdev->rst);
		if (err != -EPROBE_DEFER)
			dev_err(xdev->dev, "error getting reset %d\n", err);
		return err;
	}

	err = reset_control_deassert(xdev->rst);
	if (err) {
		dev_err(xdev->dev, "failed to deassert reset: %d\n", err);
		return err;
	}

That requires modifying the reset controller API, so it's a bit out-of-scope, 
but if you could give it a go, it would be great.

-- 
Regards,

Laurent Pinchart

--
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 2/2] xilinx_dma: Add reset support

[Philipp Zabel]

Am Donnerstag, den 15.12.2016, 15:56 +0200 schrieb Laurent Pinchart:
> Hi Ramiro,
> 
> (CC'ing Philipp Zabel)
> 
> On Thursday 15 Dec 2016 11:26:54 Ramiro Oliveira wrote:
> > On 12/14/2016 8:16 PM, Laurent Pinchart wrote:
> > > Hi Ramiro,
> > > 
> > > Thank you for the patch.
> > > 
> > > On Wednesday 14 Dec 2016 17:18:24 Ramiro Oliveira wrote:
> > >> Add a DT property to control an optional external reset line
> > >> 
> > >> Signed-off-by: Ramiro Oliveira <roliveir-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> > >> ---
> > >> 
> > >>  drivers/dma/xilinx/xilinx_dma.c | 23 +++++++++++++++++++++++
> > >>  1 file changed, 23 insertions(+)
> > >> 
> > >> diff --git a/drivers/dma/xilinx/xilinx_dma.c
> > >> b/drivers/dma/xilinx/xilinx_dma.c index 5c9f11b..b845224 100644
> > >> --- a/drivers/dma/xilinx/xilinx_dma.c
> > >> +++ b/drivers/dma/xilinx/xilinx_dma.c
> > >> @@ -46,6 +46,7 @@
> > >>  #include <linux/slab.h>
> > >>  #include <linux/clk.h>
> > >>  #include <linux/io-64-nonatomic-lo-hi.h>
> > >> +#include <linux/reset.h>
> > > 
> > > I had neatly sorted the header alphabetically until someone added clk.h
> > > and io-64-nonatomic-lo-hi.h :-( Could you please move reset.h just before
> > > slab.h ?
> >
> > Sure. Actually I was tempted to reorder it, but I decided not to do it. I'll
> > do it now
> 
> Yeah, I'll sleep better tonight :-D
> 
> > >>  #include "../dmaengine.h"
> > >> 
> > >> @@ -409,6 +410,7 @@ struct xilinx_dma_device {
> > >>  	struct clk *rxs_clk;
> > >>  	u32 nr_channels;
> > >>  	u32 chan_id;
> > >> +	struct reset_control *rst;
> > >>  };
> > >>  
> > >>  /* Macros */
> > >> @@ -2543,6 +2545,27 @@ static int xilinx_dma_probe(struct platform_device
> > >> *pdev) if (IS_ERR(xdev->regs))
> > >>  		return PTR_ERR(xdev->regs);
> > >> 
> > >> +	xdev->rst = devm_reset_control_get_optional(&pdev->dev, "reset");
> > > 
> > > devm_reset_control_get_optional() is deprecated as explained in
> > > linux/reset.h, you should use devm_reset_control_get_optional_exclusive()
> > > or devm_reset_control_get_optional_shared() instead, as applicable.
> > > 
> > > This being said, I'm wondering why the optional versions of those
> > > functions exist, as they do exactly the same as the non-optional versions.
> > > The API feels wrong, it should have been modelled like the GPIO API. Feel
> > > free to fix it if you want :-) But that's out of scope for this patch.
> > 
> > I missed the comment stating that devm_reset_control_get_optional() was
> > deprecated.
> > 
> > I could fix it. Your sugestion is modelling these functions like the GPIO
> > API?
> 
> I think it would be better for driver if the _get_optional functions would 
> return an ERR_PTR() for errors and NULL when reset control is not available, 
> and then have the rest of the reset controller API accept NULL as a no-op. 
> Your implementation would then be
> 
> 	xdev->rst = devm_reset_control_get_optional_exclusive(&pdev->dev,
> 							      "reset");
> 	if (IS_ERR(xdev->rst)) {
> 		err = PTR_ERR(xdev->rst);
> 		if (err != -EPROBE_DEFER)
> 			dev_err(xdev->dev, "error getting reset %d\n", err);
> 		return err;
> 	}
> 
> 	err = reset_control_deassert(xdev->rst);
> 	if (err) {
> 		dev_err(xdev->dev, "failed to deassert reset: %d\n", err);
> 		return err;
> 	}
> 
> That requires modifying the reset controller API, so it's a bit out-of-scope, 
> but if you could give it a go, it would be great.

Seeing that the clk and gpiod APIs behave in the same way, I think it
would be good to align the reset API with the common behaviour.

regards
Philipp
--
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