xilinx_dma: Add external reset control
Subsystems (3)
cover xilinx_dma: Add external reset control
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(+)
Signature · 1 line
2.10.2
PATCH 1/2 xilinx_dma: Edit device tree bindings documentation
Add reset property documentation for Xilinx DMA
--- Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt | 6 ++++++ 1 file changed, 6 insertions(+)
Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt +6 -0
@@ -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 at 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 at 40030000 { compatible = "xlnx,axi-vdma-mm2s-channel"; interrupts = < 0 54 4 >;
Signature · 1 line
2.10.2
PATCH 2/2 xilinx_dma: Add reset support
In reply to Ramiro OliveiraAdd a DT property to control an optional external reset line
--- drivers/dma/xilinx/xilinx_dma.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
drivers/dma/xilinx/xilinx_dma.c +23 -0
@@ -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 )
Signature · 1 line
2.10.2
Re: [PATCH 1/2] xilinx_dma: Edit device tree bindings documentation
In reply to Ramiro OliveiraHi Ramiro,
Thank you for the patch.
Quoted 13 lines · 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(+)(quoted diff) +0
-0
b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txtindex a2b8bfa..7ebce72 100644
Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt Required properties for VDMA: · line 46
Quoted 7 lines
Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt +4
-0@@ -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 ?
Quoted 12 lines
Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt +0
-0Optional properties for AXI DMA: - xlnx,mcdma: Tells whether configured for multi-channel mode in thehardware. Optional properties for VDMA:
Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt +2
-0@@ -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 >;
Signature · 3 lines
Regards, Laurent Pinchart
Re: [PATCH 2/2] xilinx_dma: Add reset support
In reply to Ramiro OliveiraHi Ramiro,
Thank you for the patch.
Quoted 12 lines · 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(+)(quoted diff) +0
-0
b/drivers/dma/xilinx/xilinx_dma.cindex 5c9f11b..b845224 100644
drivers/dma/xilinx/xilinx_dma.c line 49
Quoted 4 lines
drivers/dma/xilinx/xilinx_dma.c +1
-0@@ -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 ?
Quoted 12 lines
drivers/dma/xilinx/xilinx_dma.c +1
-0#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 */
drivers/dma/xilinx/xilinx_dma.c static int xilinx_dma_probe(struct platform_device · line 2548
Quoted 4 lines
drivers/dma/xilinx/xilinx_dma.c +1
-0@@ -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.
drivers/dma/xilinx/xilinx_dma.c static int xilinx_dma_probe(struct platform_device *pdev) · line 2552
Quoted 4 lines
drivers/dma/xilinx/xilinx_dma.c +4
-0+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.
drivers/dma/xilinx/xilinx_dma.c static int xilinx_dma_probe(struct platform_device *pdev) · line 2555
Quoted 3 lines
drivers/dma/xilinx/xilinx_dma.c +3
-0+case -ENOTSUPP : + xdev->rst =NULL ; +break ;
Wrong indentation.
You need to handle -EPROBE_DEFER and defer probing of the xilinx_dma device.
drivers/dma/xilinx/xilinx_dma.c static int xilinx_dma_probe(struct platform_device *pdev) · line 2558
Quoted 3 lines
drivers/dma/xilinx/xilinx_dma.c +3
-0+default : +dev_err (xdev->dev,"error getting reset %d\n" , err); +return err;
Wrong indentation.
drivers/dma/xilinx/xilinx_dma.c static int xilinx_dma_probe(struct platform_device *pdev) · line 2564
Quoted 6 lines
drivers/dma/xilinx/xilinx_dma.c +6
-0+ } + }else { + err =reset_control_deassert (xdev->rst); +if (err) { +dev_err (xdev->dev,"failed to deassert reset: %d\n" , + err);
Wrong indentation.
Quoted 7 lines
drivers/dma/xilinx/xilinx_dma.c +4
-0+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 )
Signature · 3 lines
Regards, Laurent Pinchart
Re: [PATCH 2/2] xilinx_dma: Add reset support
Hi Laurent,
Thank you for your feedback.
Quoted 12 lines · Laurent Pinchart
Hi Ramiro,
Thank you for the patch.
Quoted 7 lines · 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(+)
Quoted 13 lines
drivers/dma/xilinx/xilinx_dma.c line 49
Quoted 9 lines
(quoted diff) +0
-0
b/drivers/dma/xilinx/xilinx_dma.cindex 5c9f11b..b845224 100644drivers/dma/xilinx/xilinx_dma.c +1
-0@@ -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.handio-64-nonatomic-lo-hi.h:-( Could you please movereset.hjust beforeslab.h?
Sure. Actually I was tempted to reorder it, but I decided not to do it. I'll do it now
Quoted 21 lines
Quoted 12 lines
drivers/dma/xilinx/xilinx_dma.c +1
-0#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 */ drivers/dma/xilinx/xilinx_dma.c static int xilinx_dma_probe(struct platform_device · line 2548
Quoted 4 lines
drivers/dma/xilinx/xilinx_dma.c +1
-0@@ -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 inlinux/reset.h, you should usedevm_reset_control_get_optional_exclusive()ordevm_reset_control_get_optional_shared()instead, as applicable.
Quoted 5 lines
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?
Quoted 8 lines
drivers/dma/xilinx/xilinx_dma.c static int xilinx_dma_probe(struct platform_device *pdev) · line 2552
Quoted 4 lines
drivers/dma/xilinx/xilinx_dma.c +4
-0+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.
Quoted 31 lines
drivers/dma/xilinx/xilinx_dma.c static int xilinx_dma_probe(struct platform_device *pdev) · line 2555
Quoted 3 lines
drivers/dma/xilinx/xilinx_dma.c +3
-0+case -ENOTSUPP : + xdev->rst =NULL ; +break ;Wrong indentation.
You need to handle -EPROBE_DEFER and defer probing of the
xilinx_dmadevice.drivers/dma/xilinx/xilinx_dma.c static int xilinx_dma_probe(struct platform_device *pdev) · line 2558
Quoted 3 lines
drivers/dma/xilinx/xilinx_dma.c +3
-0+default : +dev_err (xdev->dev,"error getting reset %d\n" , err); +return err;Wrong indentation.
drivers/dma/xilinx/xilinx_dma.c static int xilinx_dma_probe(struct platform_device *pdev) · line 2564
Quoted 6 lines
drivers/dma/xilinx/xilinx_dma.c +6
-0+ } + }else { + err =reset_control_deassert (xdev->rst); +if (err) { +dev_err (xdev->dev,"failed to deassert reset: %d\n" , + err);Wrong indentation.
Quoted 7 lines
drivers/dma/xilinx/xilinx_dma.c +4
-0+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 )
Re: [PATCH 2/2] xilinx_dma: Add reset support
Hi Ramiro,
(CC'ing Philipp Zabel)
Quoted 28 lines · On Thursday 15 Dec 2016 11:26:54 Ramiro Oliveira wrote:
Quoted 13 lines · Laurent Pinchart
Hi Ramiro,
Thank you for the patch.
Quoted 8 lines · 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(+)Quoted 13 lines
drivers/dma/xilinx/xilinx_dma.c line 49
Quoted 9 lines
(quoted diff) +0
-0
b/drivers/dma/xilinx/xilinx_dma.cindex 5c9f11b..b845224 100644drivers/dma/xilinx/xilinx_dma.c +1
-0@@ -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.handio-64-nonatomic-lo-hi.h:-( Could you please movereset.hjust beforeslab.h?
Quoted 2 lines
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
Quoted 25 lines
Quoted 20 lines
Quoted 11 lines
drivers/dma/xilinx/xilinx_dma.c +1
-0#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 */ drivers/dma/xilinx/xilinx_dma.c static int xilinx_dma_probe(struct platform_device · line 2548
Quoted 4 lines
drivers/dma/xilinx/xilinx_dma.c +1
-0@@ -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 inlinux/reset.h, you should usedevm_reset_control_get_optional_exclusive()ordevm_reset_control_get_optional_shared()instead, as applicable.Quoted 4 lines
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.
Quoted 5 lines
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.
Signature · 3 lines
Regards, Laurent Pinchart
Re: [PATCH 2/2] xilinx_dma: Add reset support
Am Donnerstag, den 15.12.2016, 15:56 +0200 schrieb Laurent Pinchart:
Quoted 89 lines
Hi Ramiro,
(CC'ing Philipp Zabel)
Quoted 28 lines · On Thursday 15 Dec 2016 11:26:54 Ramiro Oliveira wrote:
Quoted 13 lines · Laurent Pinchart
Hi Ramiro,
Thank you for the patch.
Quoted 8 lines · 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(+)Quoted 13 lines
drivers/dma/xilinx/xilinx_dma.c line 49
Quoted 9 lines
(quoted diff) +0
-0
b/drivers/dma/xilinx/xilinx_dma.cindex 5c9f11b..b845224 100644drivers/dma/xilinx/xilinx_dma.c +1
-0@@ -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.handio-64-nonatomic-lo-hi.h:-( Could you please movereset.hjust beforeslab.h?Quoted 2 lines
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
Quoted 25 lines
Quoted 20 lines
Quoted 11 lines
drivers/dma/xilinx/xilinx_dma.c +1
-0#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 */ drivers/dma/xilinx/xilinx_dma.c static int xilinx_dma_probe(struct platform_device · line 2548
Quoted 4 lines
drivers/dma/xilinx/xilinx_dma.c +1
-0@@ -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 inlinux/reset.h, you should usedevm_reset_control_get_optional_exclusive()ordevm_reset_control_get_optional_shared()instead, as applicable.Quoted 4 lines
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.
Quoted 5 lines
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_optionalfunctions would return anERR_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 bexdev->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; }
Quoted 2 lines
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