[PATCH 1/2] reset: Add API to count number of reset available with device

[PATCH 1/2] reset: Add API to count number of reset available with device

[Vivek Gautam]

Count number of reset phandles available with the device node
to know the resets a given device has.

Cc: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
---

Based on torvald's master branch.

 include/linux/reset.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/linux/reset.h b/include/linux/reset.h
index 5daff15722d3..88f63a640153 100644
--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -2,6 +2,7 @@
 #define _LINUX_RESET_H_
 
 #include <linux/device.h>
+#include <linux/of.h>
 
 struct reset_control;
 
@@ -234,6 +235,21 @@ static inline struct reset_control *of_reset_control_get_shared_by_index(
 }
 
 /**
+ * of_reset_control_get_count - Count number of resets available with a device
+ * @node: device to be reset by the controller
+ */
+static inline unsigned int of_reset_control_get_count(struct device_node *node)
+{
+	int count;
+
+	count = of_count_phandle_with_args(node, "resets", "#reset-cells");
+	if (count < 0)
+		return 0;
+
+	return count;
+}
+
+/**
  * devm_reset_control_get_exclusive - resource managed
  *                                    reset_control_get_exclusive()
  * @dev: device to be reset by the controller
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/2] usb; dwc3: of-simple: Add support to get resets for the device

[Vivek Gautam]

Add support to get a list of resets available for the device.
These resets must be kept de-asserted until the device is
in use.

Cc: Felipe Balbi <balbi@kernel.org>
Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
---

Based on torvald's master branch.

 drivers/usb/dwc3/dwc3-of-simple.c | 49 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
index fe414e7a9c78..025de7342d28 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -29,13 +29,52 @@
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/pm_runtime.h>
+#include <linux/reset.h>
 
 struct dwc3_of_simple {
 	struct device		*dev;
 	struct clk		**clks;
 	int			num_clocks;
+	struct reset_control	**resets;
+	int			num_resets;
 };
 
+static int dwc3_of_simple_reset_init(struct dwc3_of_simple *simple, int count)
+{
+	struct device		*dev = simple->dev;
+	int			i;
+
+	simple->num_resets = count;
+
+	if (!count)
+		return 0;
+
+	simple->resets = devm_kcalloc(dev, simple->num_resets,
+				sizeof(struct reset_control *), GFP_KERNEL);
+	if (!simple->resets)
+		return -ENOMEM;
+
+	for (i = 0; i < simple->num_resets; i++) {
+		struct reset_control *reset;
+		int ret;
+
+		reset = devm_reset_control_get_by_index(dev, i);
+		if (IS_ERR(reset))
+			return PTR_ERR(reset);
+
+		simple->resets[i] = reset;
+
+		ret = reset_control_deassert(reset);
+		if (ret) {
+			while (--i >= 0)
+				reset_control_assert(reset);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static int dwc3_of_simple_clk_init(struct dwc3_of_simple *simple, int count)
 {
 	struct device		*dev = simple->dev;
@@ -100,6 +139,10 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = dwc3_of_simple_reset_init(simple, of_reset_control_get_count(np));
+	if (ret)
+		return ret;
+
 	ret = of_platform_populate(np, NULL, NULL, dev);
 	if (ret) {
 		for (i = 0; i < simple->num_clocks; i++) {
@@ -107,6 +150,9 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
 			clk_put(simple->clks[i]);
 		}
 
+		for (i = 0; i < simple->num_resets; i++)
+			reset_control_assert(simple->resets[i]);
+
 		return ret;
 	}
 
@@ -128,6 +174,9 @@ static int dwc3_of_simple_remove(struct platform_device *pdev)
 		clk_put(simple->clks[i]);
 	}
 
+	for (i = 0; i < simple->num_resets; i++)
+		reset_control_assert(simple->resets[i]);
+
 	of_platform_depopulate(dev);
 
 	pm_runtime_put_sync(dev);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/2] usb; dwc3: of-simple: Add support to get resets for the device

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 379 bytes --]

Vivek Gautam <vivek.gautam@codeaurora.org> writes:

> Add support to get a list of resets available for the device.
> These resets must be kept de-asserted until the device is
> in use.
>
> Cc: Felipe Balbi <balbi@kernel.org>
> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>

how do you plan on merging this since it depends on previous patch?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 2/2] usb; dwc3: of-simple: Add support to get resets for the device

[Vivek Gautam]

On Fri, Mar 10, 2017 at 4:54 PM, Felipe Balbi <balbi@kernel.org> wrote:
> Vivek Gautam <vivek.gautam@codeaurora.org> writes:
>
>> Add support to get a list of resets available for the device.
>> These resets must be kept de-asserted until the device is
>> in use.
>>
>> Cc: Felipe Balbi <balbi@kernel.org>
>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>
> how do you plan on merging this since it depends on previous patch?

Will ask Phillip if he is fine with the previous patch. He can then consider
giving an ack so that both patches can be pulled in together.

Will it be okay to do that ?

Regards
Vivek

>
> --
> balbi



-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/2] usb; dwc3: of-simple: Add support to get resets for the device

[Felipe Balbi]

Hi,

Vivek Gautam <vivek.gautam@codeaurora.org> writes:
> On Fri, Mar 10, 2017 at 4:54 PM, Felipe Balbi <balbi@kernel.org> wrote:
>> Vivek Gautam <vivek.gautam@codeaurora.org> writes:
>>
>>> Add support to get a list of resets available for the device.
>>> These resets must be kept de-asserted until the device is
>>> in use.
>>>
>>> Cc: Felipe Balbi <balbi@kernel.org>
>>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>>
>> how do you plan on merging this since it depends on previous patch?
>
> Will ask Phillip if he is fine with the previous patch. He can then consider
> giving an ack so that both patches can be pulled in together.
>
> Will it be okay to do that ?

sounds like a plan :-)

-- 
balbi

Re: [PATCH 1/2] reset: Add API to count number of reset available with device

[Vivek Gautam]

Hi Philipp,


On Wed, Feb 22, 2017 at 10:54 AM, Vivek Gautam
<vivek.gautam@codeaurora.org> wrote:
> Count number of reset phandles available with the device node
> to know the resets a given device has.
>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> ---

Any thoughts on this change?
A small addition that seems useful.

>
> Based on torvald's master branch.
>
>  include/linux/reset.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/include/linux/reset.h b/include/linux/reset.h
> index 5daff15722d3..88f63a640153 100644
> --- a/include/linux/reset.h
> +++ b/include/linux/reset.h
> @@ -2,6 +2,7 @@
>  #define _LINUX_RESET_H_
>
>  #include <linux/device.h>
> +#include <linux/of.h>
>
>  struct reset_control;
>
> @@ -234,6 +235,21 @@ static inline struct reset_control *of_reset_control_get_shared_by_index(
>  }
>
>  /**
> + * of_reset_control_get_count - Count number of resets available with a device
> + * @node: device to be reset by the controller
> + */
> +static inline unsigned int of_reset_control_get_count(struct device_node *node)
> +{
> +       int count;
> +
> +       count = of_count_phandle_with_args(node, "resets", "#reset-cells");
> +       if (count < 0)
> +               return 0;
> +
> +       return count;
> +}
> +
> +/**
>   * devm_reset_control_get_exclusive - resource managed
>   *                                    reset_control_get_exclusive()
>   * @dev: device to be reset by the controller
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Regards
Vivek
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] reset: Add API to count number of reset available with device

[Philipp Zabel]

Hi Vivek,

On Fri, 2017-03-10 at 20:10 +0530, Vivek Gautam wrote:
> Hi Philipp,
> 
> 
> On Wed, Feb 22, 2017 at 10:54 AM, Vivek Gautam
> <vivek.gautam@codeaurora.org> wrote:
> > Count number of reset phandles available with the device node
> > to know the resets a given device has.
> >
> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> > ---
> 
> Any thoughts on this change?
> A small addition that seems useful.

Sorry I missed this one earlier.

> >
> > Based on torvald's master branch.
> >
> >  include/linux/reset.h | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/include/linux/reset.h b/include/linux/reset.h
> > index 5daff15722d3..88f63a640153 100644
> > --- a/include/linux/reset.h
> > +++ b/include/linux/reset.h
> > @@ -2,6 +2,7 @@
> >  #define _LINUX_RESET_H_
> >
> >  #include <linux/device.h>
> > +#include <linux/of.h>
> >
> >  struct reset_control;
> >
> > @@ -234,6 +235,21 @@ static inline struct reset_control *of_reset_control_get_shared_by_index(
> >  }
> >
> >  /**
> > + * of_reset_control_get_count - Count number of resets available with a device
> > + * @node: device to be reset by the controller
> > + */
> > +static inline unsigned int of_reset_control_get_count(struct device_node *node)
> > +{
> > +       int count;
> > +
> > +       count = of_count_phandle_with_args(node, "resets", "#reset-cells");
> > +       if (count < 0)
> > +               return 0;

Please don't silently ignore errors. gpiod_count doesn't ignore errors
either. tegra_powergate_of_resets in drivers/soc/tegra/pmc.c open codes
this, too. This should be changed so it can be used there, too.

regards
Philipp

Re: [PATCH 2/2] usb; dwc3: of-simple: Add support to get resets for the device

[Philipp Zabel]

On Wed, 2017-02-22 at 10:54 +0530, Vivek Gautam wrote:
> Add support to get a list of resets available for the device.
> These resets must be kept de-asserted until the device is
> in use.
> 
> Cc: Felipe Balbi <balbi@kernel.org>
> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> ---
> 
> Based on torvald's master branch.
> 
>  drivers/usb/dwc3/dwc3-of-simple.c | 49 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
> index fe414e7a9c78..025de7342d28 100644
> --- a/drivers/usb/dwc3/dwc3-of-simple.c
> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> @@ -29,13 +29,52 @@
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/reset.h>
>  
>  struct dwc3_of_simple {
>  	struct device		*dev;
>  	struct clk		**clks;
>  	int			num_clocks;
> +	struct reset_control	**resets;
> +	int			num_resets;
>  };
>  
> +static int dwc3_of_simple_reset_init(struct dwc3_of_simple *simple, int count)
> +{
> +	struct device		*dev = simple->dev;
> +	int			i;
> +
> +	simple->num_resets = count;
> +
> +	if (!count)
> +		return 0;
> +
> +	simple->resets = devm_kcalloc(dev, simple->num_resets,
> +				sizeof(struct reset_control *), GFP_KERNEL);
> +	if (!simple->resets)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < simple->num_resets; i++) {
> +		struct reset_control *reset;
> +		int ret;
> +
> +		reset = devm_reset_control_get_by_index(dev, i);

Please use devm_reset_control_get_exclusive_by_index instead. See
include/linux/reset.h for details.

> +		if (IS_ERR(reset))
> +			return PTR_ERR(reset);
> +
> +		simple->resets[i] = reset;
> +
> +		ret = reset_control_deassert(reset);
> +		if (ret) {
> +			while (--i >= 0)
> +				reset_control_assert(reset);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}

This looks rather generic. Should we have a
reset_control_get/assert/deassert_array functionality at the reset API
level?

>  static int dwc3_of_simple_clk_init(struct dwc3_of_simple *simple, int count)
>  {
>  	struct device		*dev = simple->dev;
> @@ -100,6 +139,10 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	ret = dwc3_of_simple_reset_init(simple, of_reset_control_get_count(np));
> +	if (ret)
> +		return ret;
> +

Not a blocker, but it seems a bit inconsistent to count the reset
controls via the device node (of_...), but then get them via the device
(devm_reset_control_get_... instead of of_reset_control_get_...).

regards
Philipp

Re: [PATCH 1/2] reset: Add API to count number of reset available with device

[Vivek Gautam]

Hi Philipp,


On Wed, Mar 15, 2017 at 4:10 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Hi Vivek,
>
> On Fri, 2017-03-10 at 20:10 +0530, Vivek Gautam wrote:
>> Hi Philipp,
>>
>>
>> On Wed, Feb 22, 2017 at 10:54 AM, Vivek Gautam
>> <vivek.gautam@codeaurora.org> wrote:
>> > Count number of reset phandles available with the device node
>> > to know the resets a given device has.
>> >
>> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
>> > Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>> > ---
>>
>> Any thoughts on this change?
>> A small addition that seems useful.
>
> Sorry I missed this one earlier.
>
>> >
>> > Based on torvald's master branch.
>> >
>> >  include/linux/reset.h | 16 ++++++++++++++++
>> >  1 file changed, 16 insertions(+)
>> >
>> > diff --git a/include/linux/reset.h b/include/linux/reset.h
>> > index 5daff15722d3..88f63a640153 100644
>> > --- a/include/linux/reset.h
>> > +++ b/include/linux/reset.h
>> > @@ -2,6 +2,7 @@
>> >  #define _LINUX_RESET_H_
>> >
>> >  #include <linux/device.h>
>> > +#include <linux/of.h>
>> >
>> >  struct reset_control;
>> >
>> > @@ -234,6 +235,21 @@ static inline struct reset_control *of_reset_control_get_shared_by_index(
>> >  }
>> >
>> >  /**
>> > + * of_reset_control_get_count - Count number of resets available with a device
>> > + * @node: device to be reset by the controller
>> > + */
>> > +static inline unsigned int of_reset_control_get_count(struct device_node *node)
>> > +{
>> > +       int count;
>> > +
>> > +       count = of_count_phandle_with_args(node, "resets", "#reset-cells");
>> > +       if (count < 0)
>> > +               return 0;
>
> Please don't silently ignore errors. gpiod_count doesn't ignore errors
> either. tegra_powergate_of_resets in drivers/soc/tegra/pmc.c open codes
> this, too. This should be changed so it can be used there, too.

Sure, will change this. I can prepare a patch for tegra/pmc.c as well
using this.

Best regards
Vivek

>
> regards
> Philipp
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/2] usb; dwc3: of-simple: Add support to get resets for the device

[Vivek Gautam]

Hi,


On 03/15/2017 04:15 PM, Philipp Zabel wrote:
> On Wed, 2017-02-22 at 10:54 +0530, Vivek Gautam wrote:
>> Add support to get a list of resets available for the device.
>> These resets must be kept de-asserted until the device is
>> in use.
>>
>> Cc: Felipe Balbi <balbi@kernel.org>
>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>> ---
>>
>> Based on torvald's master branch.
>>
>>   drivers/usb/dwc3/dwc3-of-simple.c | 49 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 49 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
>> index fe414e7a9c78..025de7342d28 100644
>> --- a/drivers/usb/dwc3/dwc3-of-simple.c
>> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
>> @@ -29,13 +29,52 @@
>>   #include <linux/of.h>
>>   #include <linux/of_platform.h>
>>   #include <linux/pm_runtime.h>
>> +#include <linux/reset.h>
>>   
>>   struct dwc3_of_simple {
>>   	struct device		*dev;
>>   	struct clk		**clks;
>>   	int			num_clocks;
>> +	struct reset_control	**resets;
>> +	int			num_resets;
>>   };
>>   
>> +static int dwc3_of_simple_reset_init(struct dwc3_of_simple *simple, int count)
>> +{
>> +	struct device		*dev = simple->dev;
>> +	int			i;
>> +
>> +	simple->num_resets = count;
>> +
>> +	if (!count)
>> +		return 0;
>> +
>> +	simple->resets = devm_kcalloc(dev, simple->num_resets,
>> +				sizeof(struct reset_control *), GFP_KERNEL);
>> +	if (!simple->resets)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < simple->num_resets; i++) {
>> +		struct reset_control *reset;
>> +		int ret;
>> +
>> +		reset = devm_reset_control_get_by_index(dev, i);
> Please use devm_reset_control_get_exclusive_by_index instead. See
> include/linux/reset.h for details.

Sure, will make use of *exclusive version of the api.

>
>> +		if (IS_ERR(reset))
>> +			return PTR_ERR(reset);
>> +
>> +		simple->resets[i] = reset;
>> +
>> +		ret = reset_control_deassert(reset);
>> +		if (ret) {
>> +			while (--i >= 0)
>> +				reset_control_assert(reset);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
> This looks rather generic. Should we have a
> reset_control_get/assert/deassert_array functionality at the reset API
> level?

Yes, i think we should. Something on the lines of 'regulator_bulk_*' 
interface?

>
>>   static int dwc3_of_simple_clk_init(struct dwc3_of_simple *simple, int count)
>>   {
>>   	struct device		*dev = simple->dev;
>> @@ -100,6 +139,10 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>>   	if (ret)
>>   		return ret;
>>   
>> +	ret = dwc3_of_simple_reset_init(simple, of_reset_control_get_count(np));
>> +	if (ret)
>> +		return ret;
>> +
> Not a blocker, but it seems a bit inconsistent to count the reset
> controls via the device node (of_...), but then get them via the device
> (devm_reset_control_get_... instead of of_reset_control_get_...).

You are right, it looks inconsistent. I thought of using a resource
managed API. But now i think it doesn't make much sense.


Best Regards
Vivek

>
> regards
> Philipp
>

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project