* [PATCH 0/2] xilinx_dma: Add external reset control @ 2016-12-14 17:18 Ramiro Oliveira 2016-12-14 17:18 ` [PATCH 1/2] xilinx_dma: Edit device tree bindings documentation Ramiro Oliveira 2016-12-14 17:18 ` [PATCH 2/2] xilinx_dma: Add reset support Ramiro Oliveira 0 siblings, 2 replies; 8+ messages in thread From: Ramiro Oliveira @ 2016-12-14 17:18 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] xilinx_dma: Edit device tree bindings documentation 2016-12-14 17:18 [PATCH 0/2] xilinx_dma: Add external reset control Ramiro Oliveira @ 2016-12-14 17:18 ` Ramiro Oliveira 2016-12-14 19:56 ` Laurent Pinchart 2016-12-14 17:18 ` [PATCH 2/2] xilinx_dma: Add reset support Ramiro Oliveira 1 sibling, 1 reply; 8+ messages in thread From: Ramiro Oliveira @ 2016-12-14 17:18 UTC (permalink / raw) To: linux-arm-kernel 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 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 >; -- 2.10.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 1/2] xilinx_dma: Edit device tree bindings documentation 2016-12-14 17:18 ` [PATCH 1/2] xilinx_dma: Edit device tree bindings documentation Ramiro Oliveira @ 2016-12-14 19:56 ` Laurent Pinchart 0 siblings, 0 replies; 8+ messages in thread From: Laurent Pinchart @ 2016-12-14 19:56 UTC (permalink / raw) To: linux-arm-kernel 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 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 >; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] xilinx_dma: Add reset support 2016-12-14 17:18 [PATCH 0/2] xilinx_dma: Add external reset control Ramiro Oliveira 2016-12-14 17:18 ` [PATCH 1/2] xilinx_dma: Edit device tree bindings documentation Ramiro Oliveira @ 2016-12-14 17:18 ` Ramiro Oliveira 2016-12-14 20:16 ` Laurent Pinchart 1 sibling, 1 reply; 8+ messages in thread From: Ramiro Oliveira @ 2016-12-14 17:18 UTC (permalink / raw) To: linux-arm-kernel 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> #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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] xilinx_dma: Add reset support 2016-12-14 17:18 ` [PATCH 2/2] xilinx_dma: Add reset support Ramiro Oliveira @ 2016-12-14 20:16 ` Laurent Pinchart 2016-12-15 11:26 ` Ramiro Oliveira 0 siblings, 1 reply; 8+ messages in thread From: Laurent Pinchart @ 2016-12-14 20:16 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] xilinx_dma: Add reset support 2016-12-14 20:16 ` Laurent Pinchart @ 2016-12-15 11:26 ` Ramiro Oliveira 2016-12-15 13:56 ` Laurent Pinchart 0 siblings, 1 reply; 8+ messages in thread From: Ramiro Oliveira @ 2016-12-15 11:26 UTC (permalink / raw) To: linux-arm-kernel 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@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 ? > 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) > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] xilinx_dma: Add reset support 2016-12-15 11:26 ` Ramiro Oliveira @ 2016-12-15 13:56 ` Laurent Pinchart 2016-12-23 10:26 ` Philipp Zabel 0 siblings, 1 reply; 8+ messages in thread From: Laurent Pinchart @ 2016-12-15 13:56 UTC (permalink / raw) To: linux-arm-kernel 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@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 ? > > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] xilinx_dma: Add reset support 2016-12-15 13:56 ` Laurent Pinchart @ 2016-12-23 10:26 ` Philipp Zabel 0 siblings, 0 replies; 8+ messages in thread From: Philipp Zabel @ 2016-12-23 10:26 UTC (permalink / raw) To: linux-arm-kernel 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@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 ? > > > > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-12-23 10:26 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-12-14 17:18 [PATCH 0/2] xilinx_dma: Add external reset control Ramiro Oliveira 2016-12-14 17:18 ` [PATCH 1/2] xilinx_dma: Edit device tree bindings documentation Ramiro Oliveira 2016-12-14 19:56 ` Laurent Pinchart 2016-12-14 17:18 ` [PATCH 2/2] xilinx_dma: Add reset support Ramiro Oliveira 2016-12-14 20:16 ` Laurent Pinchart 2016-12-15 11:26 ` Ramiro Oliveira 2016-12-15 13:56 ` Laurent Pinchart 2016-12-23 10:26 ` Philipp Zabel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).