dmaengine Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/2] dt-bindings: pl330: document the optional resets property
@ 2019-05-24  0:28 Dinh Nguyen
  2019-05-24  0:28 ` [PATCH 2/2] dmagengine: pl330: add code to get reset property Dinh Nguyen
  0 siblings, 1 reply; 9+ messages in thread
From: Dinh Nguyen @ 2019-05-24  0:28 UTC (permalink / raw)
  To: dmaengine; +Cc: dinguyen, robh+dt, mark.rutland, devicetree, vkoul

Add the optional resets property the pl330 dma node.

Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
---
 Documentation/devicetree/bindings/dma/arm-pl330.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/dma/arm-pl330.txt b/Documentation/devicetree/bindings/dma/arm-pl330.txt
index db7e2260f9c5..2c7fd1941abb 100644
--- a/Documentation/devicetree/bindings/dma/arm-pl330.txt
+++ b/Documentation/devicetree/bindings/dma/arm-pl330.txt
@@ -16,6 +16,9 @@ Optional properties:
   - dma-channels: contains the total number of DMA channels supported by the DMAC
   - dma-requests: contains the total number of DMA requests supported by the DMAC
   - arm,pl330-broken-no-flushp: quirk for avoiding to execute DMAFLUSHP
+  - resets: contains an entry for each entry in reset-names.
+	    See ../reset/reset.txt for details.
+  - reset-names: must contain at least "dma", and optional is "dma-ocp".
 
 Example:
 
-- 
2.20.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/2] dmagengine: pl330: add code to get reset property
  2019-05-24  0:28 [PATCH 1/2] dt-bindings: pl330: document the optional resets property Dinh Nguyen
@ 2019-05-24  0:28 ` Dinh Nguyen
  2019-06-04 12:14   ` Vinod Koul
  0 siblings, 1 reply; 9+ messages in thread
From: Dinh Nguyen @ 2019-05-24  0:28 UTC (permalink / raw)
  To: dmaengine; +Cc: dinguyen, robh+dt, mark.rutland, devicetree, vkoul

The DMA controller on some SoCs can be held in reset, and thus requires
the reset signal(s) to deasserted. Most SoCs will have just one reset
signal, but there are others, i.e. Arria10/Stratix10 will have an
additional reset signal, referred to as the OCP.

Add code to get the reset property from the device tree for deassert and
assert.

Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
---
 drivers/dma/pl330.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 6e6837214210..6018c43e785d 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -29,6 +29,7 @@
 #include <linux/err.h>
 #include <linux/pm_runtime.h>
 #include <linux/bug.h>
+#include <linux/reset.h>
 
 #include "dmaengine.h"
 #define PL330_MAX_CHAN		8
@@ -500,6 +501,9 @@ struct pl330_dmac {
 	unsigned int num_peripherals;
 	struct dma_pl330_chan *peripherals; /* keep at end */
 	int quirks;
+
+	struct reset_control	*rstc;
+	struct reset_control	*rstc_ocp;
 };
 
 static struct pl330_of_quirks {
@@ -3028,6 +3032,30 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 
 	amba_set_drvdata(adev, pl330);
 
+	pl330->rstc = devm_reset_control_get_optional(&adev->dev, "dma");
+	if (IS_ERR(pl330->rstc)) {
+		dev_err(&adev->dev, "No reset controller specified.\n");
+		return PTR_ERR(pl330->rstc);
+	} else {
+		ret = reset_control_deassert(pl330->rstc);
+		if (ret) {
+			dev_err(&adev->dev, "Couldn't deassert the device from reset!\n");
+			return ret;
+		}
+	}
+
+	pl330->rstc_ocp = devm_reset_control_get_optional(&adev->dev, "dma-ocp");
+	if (IS_ERR(pl330->rstc_ocp)) {
+		dev_err(&adev->dev, "No reset controller specified.\n");
+		return PTR_ERR(pl330->rstc_ocp);
+	} else {
+		ret = reset_control_deassert(pl330->rstc_ocp);
+		if (ret) {
+			dev_err(&adev->dev, "Couldn't deassert the device from OCP reset!\n");
+			return ret;
+		}
+	}
+
 	for (i = 0; i < AMBA_NR_IRQS; i++) {
 		irq = adev->irq[i];
 		if (irq) {
@@ -3168,6 +3196,11 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 probe_err2:
 	pl330_del(pl330);
 
+	if (pl330->rstc_ocp)
+		reset_control_assert(pl330->rstc_ocp);
+
+	if (pl330->rstc)
+		reset_control_assert(pl330->rstc);
 	return ret;
 }
 
@@ -3206,6 +3239,11 @@ static int pl330_remove(struct amba_device *adev)
 
 	pl330_del(pl330);
 
+	if (pl330->rstc_ocp)
+		reset_control_assert(pl330->rstc_ocp);
+
+	if (pl330->rstc)
+		reset_control_assert(pl330->rstc);
 	return 0;
 }
 
-- 
2.20.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] dmagengine: pl330: add code to get reset property
  2019-05-24  0:28 ` [PATCH 2/2] dmagengine: pl330: add code to get reset property Dinh Nguyen
@ 2019-06-04 12:14   ` Vinod Koul
  2019-06-04 14:21     ` Dinh Nguyen
  0 siblings, 1 reply; 9+ messages in thread
From: Vinod Koul @ 2019-06-04 12:14 UTC (permalink / raw)
  To: Dinh Nguyen; +Cc: dmaengine, robh+dt, mark.rutland, devicetree

On 23-05-19, 19:28, Dinh Nguyen wrote:
> The DMA controller on some SoCs can be held in reset, and thus requires
> the reset signal(s) to deasserted. Most SoCs will have just one reset
> signal, but there are others, i.e. Arria10/Stratix10 will have an
> additional reset signal, referred to as the OCP.
> 
> Add code to get the reset property from the device tree for deassert and
> assert.
> 
> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
> ---
>  drivers/dma/pl330.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 6e6837214210..6018c43e785d 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -29,6 +29,7 @@
>  #include <linux/err.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/bug.h>
> +#include <linux/reset.h>
>  
>  #include "dmaengine.h"
>  #define PL330_MAX_CHAN		8
> @@ -500,6 +501,9 @@ struct pl330_dmac {
>  	unsigned int num_peripherals;
>  	struct dma_pl330_chan *peripherals; /* keep at end */
>  	int quirks;
> +
> +	struct reset_control	*rstc;
> +	struct reset_control	*rstc_ocp;
>  };
>  
>  static struct pl330_of_quirks {
> @@ -3028,6 +3032,30 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>  
>  	amba_set_drvdata(adev, pl330);
>  
> +	pl330->rstc = devm_reset_control_get_optional(&adev->dev, "dma");
> +	if (IS_ERR(pl330->rstc)) {
> +		dev_err(&adev->dev, "No reset controller specified.\n");

Wasnt this optional??

> +		return PTR_ERR(pl330->rstc);
> +	} else {
> +		ret = reset_control_deassert(pl330->rstc);
> +		if (ret) {
> +			dev_err(&adev->dev, "Couldn't deassert the device from reset!\n");
> +			return ret;
> +		}
> +	}
> +
> +	pl330->rstc_ocp = devm_reset_control_get_optional(&adev->dev, "dma-ocp");
> +	if (IS_ERR(pl330->rstc_ocp)) {
> +		dev_err(&adev->dev, "No reset controller specified.\n");
> +		return PTR_ERR(pl330->rstc_ocp);
> +	} else {
> +		ret = reset_control_deassert(pl330->rstc_ocp);
> +		if (ret) {
> +			dev_err(&adev->dev, "Couldn't deassert the device from OCP reset!\n");
> +			return ret;
> +		}
> +	}
> +
>  	for (i = 0; i < AMBA_NR_IRQS; i++) {
>  		irq = adev->irq[i];
>  		if (irq) {
> @@ -3168,6 +3196,11 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>  probe_err2:
>  	pl330_del(pl330);
>  
> +	if (pl330->rstc_ocp)
> +		reset_control_assert(pl330->rstc_ocp);
> +
> +	if (pl330->rstc)
> +		reset_control_assert(pl330->rstc);
>  	return ret;
>  }
>  
> @@ -3206,6 +3239,11 @@ static int pl330_remove(struct amba_device *adev)
>  
>  	pl330_del(pl330);
>  
> +	if (pl330->rstc_ocp)
> +		reset_control_assert(pl330->rstc_ocp);
> +
> +	if (pl330->rstc)
> +		reset_control_assert(pl330->rstc);
>  	return 0;
>  }
>  
> -- 
> 2.20.0

-- 
~Vinod

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] dmagengine: pl330: add code to get reset property
  2019-06-04 12:14   ` Vinod Koul
@ 2019-06-04 14:21     ` Dinh Nguyen
  2019-06-04 16:31       ` Geert Uytterhoeven
  0 siblings, 1 reply; 9+ messages in thread
From: Dinh Nguyen @ 2019-06-04 14:21 UTC (permalink / raw)
  To: Vinod Koul; +Cc: dmaengine, robh+dt, mark.rutland, devicetree

Hi Vinod,

On 6/4/19 7:14 AM, Vinod Koul wrote:
> On 23-05-19, 19:28, Dinh Nguyen wrote:
>> The DMA controller on some SoCs can be held in reset, and thus requires
>> the reset signal(s) to deasserted. Most SoCs will have just one reset
>> signal, but there are others, i.e. Arria10/Stratix10 will have an
>> additional reset signal, referred to as the OCP.
>>
>> Add code to get the reset property from the device tree for deassert and
>> assert.
>>
>> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
>> ---
>>  drivers/dma/pl330.c | 38 ++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 38 insertions(+)
>>
>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>> index 6e6837214210..6018c43e785d 100644
>> --- a/drivers/dma/pl330.c
>> +++ b/drivers/dma/pl330.c
>> @@ -29,6 +29,7 @@
>>  #include <linux/err.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/bug.h>
>> +#include <linux/reset.h>
>>  
>>  #include "dmaengine.h"
>>  #define PL330_MAX_CHAN		8
>> @@ -500,6 +501,9 @@ struct pl330_dmac {
>>  	unsigned int num_peripherals;
>>  	struct dma_pl330_chan *peripherals; /* keep at end */
>>  	int quirks;
>> +
>> +	struct reset_control	*rstc;
>> +	struct reset_control	*rstc_ocp;
>>  };
>>  
>>  static struct pl330_of_quirks {
>> @@ -3028,6 +3032,30 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>>  
>>  	amba_set_drvdata(adev, pl330);
>>  
>> +	pl330->rstc = devm_reset_control_get_optional(&adev->dev, "dma");
>> +	if (IS_ERR(pl330->rstc)) {
>> +		dev_err(&adev->dev, "No reset controller specified.\n");
> 
> Wasnt this optional??

Yes, this is optional. The call devm_reset_control_get_optional() will
just return NULL if the reset property is not there, but an error
pointer if something really went wrong. Thus, I'm using IS_ERR() for the
error checking.

Dinh

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] dmagengine: pl330: add code to get reset property
  2019-06-04 14:21     ` Dinh Nguyen
@ 2019-06-04 16:31       ` Geert Uytterhoeven
  2019-06-05 14:41         ` Dinh Nguyen
  0 siblings, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2019-06-04 16:31 UTC (permalink / raw)
  To: Dinh Nguyen
  Cc: Vinod Koul, dmaengine, Rob Herring, Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

Hi Dinh,

On Tue, Jun 4, 2019 at 4:21 PM Dinh Nguyen <dinguyen@kernel.org> wrote:
> On 6/4/19 7:14 AM, Vinod Koul wrote:
> > On 23-05-19, 19:28, Dinh Nguyen wrote:
> >> The DMA controller on some SoCs can be held in reset, and thus requires
> >> the reset signal(s) to deasserted. Most SoCs will have just one reset
> >> signal, but there are others, i.e. Arria10/Stratix10 will have an
> >> additional reset signal, referred to as the OCP.
> >>
> >> Add code to get the reset property from the device tree for deassert and
> >> assert.
> >>
> >> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
> >> ---
> >>  drivers/dma/pl330.c | 38 ++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 38 insertions(+)
> >>
> >> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> >> index 6e6837214210..6018c43e785d 100644
> >> --- a/drivers/dma/pl330.c
> >> +++ b/drivers/dma/pl330.c
> >> @@ -29,6 +29,7 @@
> >>  #include <linux/err.h>
> >>  #include <linux/pm_runtime.h>
> >>  #include <linux/bug.h>
> >> +#include <linux/reset.h>
> >>
> >>  #include "dmaengine.h"
> >>  #define PL330_MAX_CHAN              8
> >> @@ -500,6 +501,9 @@ struct pl330_dmac {
> >>      unsigned int num_peripherals;
> >>      struct dma_pl330_chan *peripherals; /* keep at end */
> >>      int quirks;
> >> +
> >> +    struct reset_control    *rstc;
> >> +    struct reset_control    *rstc_ocp;
> >>  };
> >>
> >>  static struct pl330_of_quirks {
> >> @@ -3028,6 +3032,30 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
> >>
> >>      amba_set_drvdata(adev, pl330);
> >>
> >> +    pl330->rstc = devm_reset_control_get_optional(&adev->dev, "dma");
> >> +    if (IS_ERR(pl330->rstc)) {
> >> +            dev_err(&adev->dev, "No reset controller specified.\n");
> >
> > Wasnt this optional??
>
> Yes, this is optional. The call devm_reset_control_get_optional() will
> just return NULL if the reset property is not there, but an error
> pointer if something really went wrong. Thus, I'm using IS_ERR() for the
> error checking.

So the error message is incorrect, as this is a real error condition?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] dmagengine: pl330: add code to get reset property
  2019-06-04 16:31       ` Geert Uytterhoeven
@ 2019-06-05 14:41         ` Dinh Nguyen
  2019-06-05 15:31           ` Dinh Nguyen
  0 siblings, 1 reply; 9+ messages in thread
From: Dinh Nguyen @ 2019-06-05 14:41 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Vinod Koul, dmaengine, Rob Herring, Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

Hi Geert,

On 6/4/19 11:31 AM, Geert Uytterhoeven wrote:
> Hi Dinh,
> 
> On Tue, Jun 4, 2019 at 4:21 PM Dinh Nguyen <dinguyen@kernel.org> wrote:
>> On 6/4/19 7:14 AM, Vinod Koul wrote:
>>> On 23-05-19, 19:28, Dinh Nguyen wrote:
>>>> The DMA controller on some SoCs can be held in reset, and thus requires
>>>> the reset signal(s) to deasserted. Most SoCs will have just one reset
>>>> signal, but there are others, i.e. Arria10/Stratix10 will have an
>>>> additional reset signal, referred to as the OCP.
>>>>
>>>> Add code to get the reset property from the device tree for deassert and
>>>> assert.
>>>>
>>>> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
>>>> ---
>>>>  drivers/dma/pl330.c | 38 ++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 38 insertions(+)
>>>>
>>>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>>>> index 6e6837214210..6018c43e785d 100644
>>>> --- a/drivers/dma/pl330.c
>>>> +++ b/drivers/dma/pl330.c
>>>> @@ -29,6 +29,7 @@
>>>>  #include <linux/err.h>
>>>>  #include <linux/pm_runtime.h>
>>>>  #include <linux/bug.h>
>>>> +#include <linux/reset.h>
>>>>
>>>>  #include "dmaengine.h"
>>>>  #define PL330_MAX_CHAN              8
>>>> @@ -500,6 +501,9 @@ struct pl330_dmac {
>>>>      unsigned int num_peripherals;
>>>>      struct dma_pl330_chan *peripherals; /* keep at end */
>>>>      int quirks;
>>>> +
>>>> +    struct reset_control    *rstc;
>>>> +    struct reset_control    *rstc_ocp;
>>>>  };
>>>>
>>>>  static struct pl330_of_quirks {
>>>> @@ -3028,6 +3032,30 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>>>>
>>>>      amba_set_drvdata(adev, pl330);
>>>>
>>>> +    pl330->rstc = devm_reset_control_get_optional(&adev->dev, "dma");
>>>> +    if (IS_ERR(pl330->rstc)) {
>>>> +            dev_err(&adev->dev, "No reset controller specified.\n");
>>>
>>> Wasnt this optional??
>>
>> Yes, this is optional. The call devm_reset_control_get_optional() will
>> just return NULL if the reset property is not there, but an error
>> pointer if something really went wrong. Thus, I'm using IS_ERR() for the
>> error checking.
> 
> So the error message is incorrect, as this is a real error condition?
> 

Yes, you're right! Will correct in V2.

Dinh

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] dmagengine: pl330: add code to get reset property
  2019-06-05 14:41         ` Dinh Nguyen
@ 2019-06-05 15:31           ` Dinh Nguyen
  2019-06-07  7:37             ` Geert Uytterhoeven
  0 siblings, 1 reply; 9+ messages in thread
From: Dinh Nguyen @ 2019-06-05 15:31 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Vinod Koul, dmaengine, Rob Herring, Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

Hi Geert,

On 6/5/19 9:41 AM, Dinh Nguyen wrote:
> Hi Geert,
> 
> On 6/4/19 11:31 AM, Geert Uytterhoeven wrote:
>> Hi Dinh,
>>
>> On Tue, Jun 4, 2019 at 4:21 PM Dinh Nguyen <dinguyen@kernel.org> wrote:
>>> On 6/4/19 7:14 AM, Vinod Koul wrote:
>>>> On 23-05-19, 19:28, Dinh Nguyen wrote:
>>>>> The DMA controller on some SoCs can be held in reset, and thus requires
>>>>> the reset signal(s) to deasserted. Most SoCs will have just one reset
>>>>> signal, but there are others, i.e. Arria10/Stratix10 will have an
>>>>> additional reset signal, referred to as the OCP.
>>>>>
>>>>> Add code to get the reset property from the device tree for deassert and
>>>>> assert.
>>>>>
>>>>> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
>>>>> ---
>>>>>  drivers/dma/pl330.c | 38 ++++++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 38 insertions(+)
>>>>>
>>>>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>>>>> index 6e6837214210..6018c43e785d 100644
>>>>> --- a/drivers/dma/pl330.c
>>>>> +++ b/drivers/dma/pl330.c
>>>>> @@ -29,6 +29,7 @@
>>>>>  #include <linux/err.h>
>>>>>  #include <linux/pm_runtime.h>
>>>>>  #include <linux/bug.h>
>>>>> +#include <linux/reset.h>
>>>>>
>>>>>  #include "dmaengine.h"
>>>>>  #define PL330_MAX_CHAN              8
>>>>> @@ -500,6 +501,9 @@ struct pl330_dmac {
>>>>>      unsigned int num_peripherals;
>>>>>      struct dma_pl330_chan *peripherals; /* keep at end */
>>>>>      int quirks;
>>>>> +
>>>>> +    struct reset_control    *rstc;
>>>>> +    struct reset_control    *rstc_ocp;
>>>>>  };
>>>>>
>>>>>  static struct pl330_of_quirks {
>>>>> @@ -3028,6 +3032,30 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>>>>>
>>>>>      amba_set_drvdata(adev, pl330);
>>>>>
>>>>> +    pl330->rstc = devm_reset_control_get_optional(&adev->dev, "dma");
>>>>> +    if (IS_ERR(pl330->rstc)) {
>>>>> +            dev_err(&adev->dev, "No reset controller specified.\n");
>>>>
>>>> Wasnt this optional??
>>>
>>> Yes, this is optional. The call devm_reset_control_get_optional() will
>>> just return NULL if the reset property is not there, but an error
>>> pointer if something really went wrong. Thus, I'm using IS_ERR() for the
>>> error checking.
>>
>> So the error message is incorrect, as this is a real error condition?
>>
> 
> Yes, you're right! Will correct in V2.

Looking at this again, I think the error message is correct. The
optional call will return NULL if the resets property is not specified,
and will return an error pointer if the reset propert is specified, but
the pointer to the reset controller is not found.

So I think the error message is correct.

Dinh

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] dmagengine: pl330: add code to get reset property
  2019-06-05 15:31           ` Dinh Nguyen
@ 2019-06-07  7:37             ` Geert Uytterhoeven
  2019-06-10 23:40               ` Dinh Nguyen
  0 siblings, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2019-06-07  7:37 UTC (permalink / raw)
  To: Dinh Nguyen
  Cc: Vinod Koul, dmaengine, Rob Herring, Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

Hi Dinh,

On Wed, Jun 5, 2019 at 5:31 PM Dinh Nguyen <dinguyen@kernel.org> wrote:
> On 6/5/19 9:41 AM, Dinh Nguyen wrote:
> > On 6/4/19 11:31 AM, Geert Uytterhoeven wrote:
> >> On Tue, Jun 4, 2019 at 4:21 PM Dinh Nguyen <dinguyen@kernel.org> wrote:
> >>> On 6/4/19 7:14 AM, Vinod Koul wrote:
> >>>> On 23-05-19, 19:28, Dinh Nguyen wrote:
> >>>>> The DMA controller on some SoCs can be held in reset, and thus requires
> >>>>> the reset signal(s) to deasserted. Most SoCs will have just one reset
> >>>>> signal, but there are others, i.e. Arria10/Stratix10 will have an
> >>>>> additional reset signal, referred to as the OCP.
> >>>>>
> >>>>> Add code to get the reset property from the device tree for deassert and
> >>>>> assert.
> >>>>>
> >>>>> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>

> >>>>> --- a/drivers/dma/pl330.c
> >>>>> +++ b/drivers/dma/pl330.c

> >>>>> @@ -3028,6 +3032,30 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
> >>>>>
> >>>>>      amba_set_drvdata(adev, pl330);
> >>>>>
> >>>>> +    pl330->rstc = devm_reset_control_get_optional(&adev->dev, "dma");
> >>>>> +    if (IS_ERR(pl330->rstc)) {
> >>>>> +            dev_err(&adev->dev, "No reset controller specified.\n");

"No reset controller specified.\n"

> >>>>
> >>>> Wasnt this optional??
> >>>
> >>> Yes, this is optional. The call devm_reset_control_get_optional() will
> >>> just return NULL if the reset property is not there, but an error
> >>> pointer if something really went wrong. Thus, I'm using IS_ERR() for the
> >>> error checking.
> >>
> >> So the error message is incorrect, as this is a real error condition?
> >
> > Yes, you're right! Will correct in V2.
>
> Looking at this again, I think the error message is correct. The
> optional call will return NULL if the resets property is not specified,
> and will return an error pointer if the reset propert is specified, but
> the pointer to the reset controller is not found.
>
> So I think the error message is correct.

Please reread the error message, and what you wrote above.

Error message: "No reset controller specified".
Rationale: NULL (i.e. no error) if "the resets property is not specified".

If an error pointer is returned, this may be due to probe deferral (to be
propagated, but further ignored), or due to a real failure.

So IMHO the code should read:

        if (IS_ERR(pl330->rstc)) {
                if (PTR_ERR(pl330->rstc) != -EPROBE_DEFER)
                        dev_err(&adev->dev, "Failed to get reset.\n");
                return PTR_ERR(pl330->rstc);
        } else { ... }

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] dmagengine: pl330: add code to get reset property
  2019-06-07  7:37             ` Geert Uytterhoeven
@ 2019-06-10 23:40               ` Dinh Nguyen
  0 siblings, 0 replies; 9+ messages in thread
From: Dinh Nguyen @ 2019-06-10 23:40 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Vinod Koul, dmaengine, Rob Herring, Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

Hi Geert,

On 6/7/19 2:37 AM, Geert Uytterhoeven wrote:
> Hi Dinh,
> 
> On Wed, Jun 5, 2019 at 5:31 PM Dinh Nguyen <dinguyen@kernel.org> wrote:
>> On 6/5/19 9:41 AM, Dinh Nguyen wrote:
>>> On 6/4/19 11:31 AM, Geert Uytterhoeven wrote:
>>>> On Tue, Jun 4, 2019 at 4:21 PM Dinh Nguyen <dinguyen@kernel.org> wrote:
>>>>> On 6/4/19 7:14 AM, Vinod Koul wrote:
>>>>>> On 23-05-19, 19:28, Dinh Nguyen wrote:
>>>>>>> The DMA controller on some SoCs can be held in reset, and thus requires
>>>>>>> the reset signal(s) to deasserted. Most SoCs will have just one reset
>>>>>>> signal, but there are others, i.e. Arria10/Stratix10 will have an
>>>>>>> additional reset signal, referred to as the OCP.
>>>>>>>
>>>>>>> Add code to get the reset property from the device tree for deassert and
>>>>>>> assert.
>>>>>>>
>>>>>>> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
> 
>>>>>>> --- a/drivers/dma/pl330.c
>>>>>>> +++ b/drivers/dma/pl330.c
> 
>>>>>>> @@ -3028,6 +3032,30 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>>>>>>>
>>>>>>>      amba_set_drvdata(adev, pl330);
>>>>>>>
>>>>>>> +    pl330->rstc = devm_reset_control_get_optional(&adev->dev, "dma");
>>>>>>> +    if (IS_ERR(pl330->rstc)) {
>>>>>>> +            dev_err(&adev->dev, "No reset controller specified.\n");
> 
> "No reset controller specified.\n"
> 
>>>>>>
>>>>>> Wasnt this optional??
>>>>>
>>>>> Yes, this is optional. The call devm_reset_control_get_optional() will
>>>>> just return NULL if the reset property is not there, but an error
>>>>> pointer if something really went wrong. Thus, I'm using IS_ERR() for the
>>>>> error checking.
>>>>
>>>> So the error message is incorrect, as this is a real error condition?
>>>
>>> Yes, you're right! Will correct in V2.
>>
>> Looking at this again, I think the error message is correct. The
>> optional call will return NULL if the resets property is not specified,
>> and will return an error pointer if the reset propert is specified, but
>> the pointer to the reset controller is not found.
>>
>> So I think the error message is correct.
> 
> Please reread the error message, and what you wrote above.
> 
> Error message: "No reset controller specified".
> Rationale: NULL (i.e. no error) if "the resets property is not specified".
> 
> If an error pointer is returned, this may be due to probe deferral (to be
> propagated, but further ignored), or due to a real failure.
> 
> So IMHO the code should read:
> 
>         if (IS_ERR(pl330->rstc)) {
>                 if (PTR_ERR(pl330->rstc) != -EPROBE_DEFER)
>                         dev_err(&adev->dev, "Failed to get reset.\n");
>                 return PTR_ERR(pl330->rstc);
>         } else { ... }
> 

You're right! Will update in v2.

Thanks!

Dinh

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-24  0:28 [PATCH 1/2] dt-bindings: pl330: document the optional resets property Dinh Nguyen
2019-05-24  0:28 ` [PATCH 2/2] dmagengine: pl330: add code to get reset property Dinh Nguyen
2019-06-04 12:14   ` Vinod Koul
2019-06-04 14:21     ` Dinh Nguyen
2019-06-04 16:31       ` Geert Uytterhoeven
2019-06-05 14:41         ` Dinh Nguyen
2019-06-05 15:31           ` Dinh Nguyen
2019-06-07  7:37             ` Geert Uytterhoeven
2019-06-10 23:40               ` Dinh Nguyen

dmaengine Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dmaengine/0 dmaengine/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dmaengine dmaengine/ https://lore.kernel.org/dmaengine \
		dmaengine@vger.kernel.org dmaengine@archiver.kernel.org
	public-inbox-index dmaengine


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.dmaengine


AGPL code for this site: git clone https://public-inbox.org/ public-inbox