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
Add reset property documentation for Xilinx DMA
Signed-off-by: Ramiro Oliveira <[email protected]>
---
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
Add a DT property to control an optional external reset line
Signed-off-by: Ramiro Oliveira <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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)
>
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 <[email protected]>
> >> ---
> >>
> >> 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
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 <[email protected]>
> > >> ---
> > >>
> > >> 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