[PATCH V3 0/4] reset: APIs to manage a list of resets

[PATCH V3 0/4] reset: APIs to manage a list of resets

[Vivek Gautam]

Set of patches to support getting and de/asserting a list (array)
of reset controllers available with the device.
This series also contains reset controls patches for dwc3-of-simple
and tegra pmc drivers.

This V3 version addresses Philipp's review comments to make the
APIs inline with gpiod APIs. So, the reset framework does all the
job of storing the reset controls and their count.
The users can just pass the device pointer/node along with 'optional'
'shared' flags.
The *_reset_control_array_* APIs _assert() and _deassert() required
'struct reset_control_array' to handle the reset controls.

The series is tested on torvald's master branch with following support -
a) pza/linux - reset/next
b) gregkh/usb - usb-next
c) agross/linux - for-next
d) device tree patches to enable usb on db820c.

Changes since v2:
 - Addressed comments to make APIs inline with gpiod API.
 - Moved number of reset controls in 'struct reset_control_array'
   so that the footprint is reduced.
 - of_reset_control_array_get() and devm_reset_control_array_get()
   now return pointer to the newly created reset control array.
 - Added comments to mention that the reset control array APIs don't
   guarantee any particular order when handling the reset controls.
 - Dropped 'name' from reset_control_array' since the interface is meant
   for a bunch of anonymous resets that can all be asserted or deasserted
   in arbitrary order.
 - Fixed returns for APIs reported by kbuild.
 - Fixed 'for' clause guards reported by kbuild.

Changes since v1:
 - Addressed comment for error handling in of_reset_control_get_count()
 - Added patch to manage reset controller array.
 - Rebased dwc3-of-simple changes based on the new set of APIs
   for reset control array.
 - Added a patch for soc/tegra/pmc driver to use the new set of
   reset control array APIs.

Vivek Gautam (4):
  reset: Add API to count number of reset available with device
  reset: Add APIs to manage array of resets
  usb: dwc3: of-simple: Add support to get resets for the device
  soc/tegra: pmc: Use the new reset APIs to manage reset controllers

 drivers/reset/core.c              | 200 ++++++++++++++++++++++++++++++++++++++
 drivers/soc/tegra/pmc.c           |  99 +++++++------------
 drivers/usb/dwc3/dwc3-of-simple.c |  36 +++++++
 include/linux/reset.h             |  99 +++++++++++++++++++
 4 files changed, 371 insertions(+), 63 deletions(-)

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

[PATCH V3 1/4] 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>
---
 drivers/reset/core.c  | 23 +++++++++++++++++++++++
 include/linux/reset.h |  6 ++++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index cd739d2fa160..f0a06a7aca93 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -465,3 +465,26 @@ int device_reset(struct device *dev)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(device_reset);
+
+/**
+ * of_reset_control_get_count - Count number of resets available with a device
+ *
+ * @node: device node that contains 'resets'.
+ *
+ * Returns positive reset count on success, or error number on failure and
+ * on count being zero.
+ */
+int of_reset_control_get_count(struct device_node *node)
+{
+	int count;
+
+	if (!node)
+		return -EINVAL;
+
+	count = of_count_phandle_with_args(node, "resets", "#reset-cells");
+	if (count == 0)
+		count = -ENOENT;
+
+	return count;
+}
+EXPORT_SYMBOL_GPL(of_reset_control_get_count);
diff --git a/include/linux/reset.h b/include/linux/reset.h
index 13d8681210d5..1b5a6aafd3e6 100644
--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -24,6 +24,7 @@ struct reset_control *__devm_reset_control_get(struct device *dev,
 				     bool optional);
 
 int __must_check device_reset(struct device *dev);
+int of_reset_control_get_count(struct device_node *node);
 
 static inline int device_reset_optional(struct device *dev)
 {
@@ -89,6 +90,11 @@ static inline struct reset_control *__devm_reset_control_get(
 	return optional ? NULL : ERR_PTR(-ENOTSUPP);
 }
 
+static inline int of_reset_control_get_count(struct device_node *node)
+{
+	return -ENOTSUPP;
+}
+
 #endif /* CONFIG_RESET_CONTROLLER */
 
 /**
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Philipp Zabel]

On Tue, 2017-04-18 at 16:51 +0530, Vivek Gautam 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>
> ---
>  drivers/reset/core.c  | 23 +++++++++++++++++++++++
>  include/linux/reset.h |  6 ++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index cd739d2fa160..f0a06a7aca93 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -465,3 +465,26 @@ int device_reset(struct device *dev)
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(device_reset);
> +
> +/**
> + * of_reset_control_get_count - Count number of resets available with a device
> + *
> + * @node: device node that contains 'resets'.
> + *
> + * Returns positive reset count on success, or error number on failure and
> + * on count being zero.
> + */
> +int of_reset_control_get_count(struct device_node *node)
> +{
> +	int count;
> +
> +	if (!node)
> +		return -EINVAL;
> +
> +	count = of_count_phandle_with_args(node, "resets", "#reset-cells");
> +	if (count == 0)
> +		count = -ENOENT;
> +
> +	return count;
> +}
> +EXPORT_SYMBOL_GPL(of_reset_control_get_count);

This doesn't need to be public anymore. You can make it static and merge
it into the second patch.

regards
Philipp

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

[Vivek Gautam]

On 04/19/2017 03:55 PM, Philipp Zabel wrote:
> On Tue, 2017-04-18 at 16:51 +0530, Vivek Gautam 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>
>> ---
>>   drivers/reset/core.c  | 23 +++++++++++++++++++++++
>>   include/linux/reset.h |  6 ++++++
>>   2 files changed, 29 insertions(+)
>>
>> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
>> index cd739d2fa160..f0a06a7aca93 100644
>> --- a/drivers/reset/core.c
>> +++ b/drivers/reset/core.c
>> @@ -465,3 +465,26 @@ int device_reset(struct device *dev)
>>   	return ret;
>>   }
>>   EXPORT_SYMBOL_GPL(device_reset);
>> +
>> +/**
>> + * of_reset_control_get_count - Count number of resets available with a device
>> + *
>> + * @node: device node that contains 'resets'.
>> + *
>> + * Returns positive reset count on success, or error number on failure and
>> + * on count being zero.
>> + */
>> +int of_reset_control_get_count(struct device_node *node)
>> +{
>> +	int count;
>> +
>> +	if (!node)
>> +		return -EINVAL;
>> +
>> +	count = of_count_phandle_with_args(node, "resets", "#reset-cells");
>> +	if (count == 0)
>> +		count = -ENOENT;
>> +
>> +	return count;
>> +}
>> +EXPORT_SYMBOL_GPL(of_reset_control_get_count);
> This doesn't need to be public anymore. You can make it static and merge
> it into the second patch.

Thanks for reviewing the series.
Sure, make sense to make it static and squash in the second patch.

Best Regards
Vivek

>
> regards
> Philipp
>

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

[PATCH V3 2/4] reset: Add APIs to manage array of resets

[Vivek Gautam]

Many devices may want to request a bunch of resets
and control them. So it's better to manage them as an
array. Add APIs to _get(), _assert(), and _deassert()
an array of reset_control.

Cc: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
---
 drivers/reset/core.c  | 177 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/reset.h |  93 ++++++++++++++++++++++++++
 2 files changed, 270 insertions(+)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index f0a06a7aca93..54bd3be5e7a4 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -488,3 +488,180 @@ int of_reset_control_get_count(struct device_node *node)
 	return count;
 }
 EXPORT_SYMBOL_GPL(of_reset_control_get_count);
+
+/**
+ * APIs to manage an array of reset controls.
+ */
+/**
+ * reset_control_array_assert: assert a list of resets
+ *
+ * @resets: reset control array holding info about the list of resets
+ *
+ * This API doesn't guarantee that the reset lines controlled by
+ * the reset array are asserted in any particular order.
+ *
+ * Returns 0 on success or error number on failure.
+ */
+int reset_control_array_assert(struct reset_control_array *resets)
+{
+	int ret, i;
+
+	if (!resets)
+		return 0;
+
+	if (IS_ERR(resets))
+		return -EINVAL;
+
+	for (i = 0; i < resets->num_rstcs; i++) {
+		ret = reset_control_assert(resets->rstc[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(reset_control_array_assert);
+
+/**
+ * reset_control_array_deassert: deassert a list of resets
+ *
+ * @resets: reset control array holding info about the list of resets
+ *
+ * This API doesn't guarantee that the reset lines controlled by
+ * the reset array are deasserted in any particular order.
+ *
+ * Returns 0 on success or error number on failure.
+ */
+int reset_control_array_deassert(struct reset_control_array *resets)
+{
+	int ret, i;
+
+	if (!resets)
+		return 0;
+
+	if (IS_ERR(resets))
+		return -EINVAL;
+
+	for (i = 0; i < resets->num_rstcs; i++) {
+		ret = reset_control_deassert(resets->rstc[i]);
+		if (ret)
+			goto err;
+	}
+
+	return 0;
+
+err:
+	while (i--)
+		reset_control_assert(resets->rstc[i]);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(reset_control_array_deassert);
+
+static void devm_reset_control_array_release(struct device *dev, void *res)
+{
+	struct reset_control_array *resets = res;
+
+	reset_control_array_put(resets);
+}
+
+/**
+ * of_reset_control_array_get - Get a list of reset controls using
+ *				device node.
+ *
+ * @np: device node for the device that requests the reset controls array
+ * @shared: whether reset controls are shared or not
+ * @optional: whether it is optional to get the reset controls
+ *
+ * Returns pointer to allocated reset_control_array on success or
+ * error on failure
+ */
+struct reset_control_array *
+of_reset_control_array_get(struct device_node *np, bool shared, bool optional)
+{
+	struct reset_control_array *resets;
+	struct reset_control *rstc;
+	int num, i;
+	void *err;
+
+	num = of_reset_control_get_count(np);
+	if (num < 0)
+		return ERR_PTR(num);
+
+	resets = kzalloc(sizeof(*resets) + sizeof(resets->rstc[0]) * num,
+			 GFP_KERNEL);
+	if (!resets)
+		return ERR_PTR(-ENOMEM);
+
+	for (i = 0; i < num; i++) {
+		rstc = __of_reset_control_get(np, NULL, i, shared, optional);
+		if (IS_ERR(rstc)) {
+			err = ERR_CAST(rstc);
+			goto err_rst;
+		}
+		resets->rstc[i] = rstc;
+	}
+	resets->num_rstcs = num;
+
+	return resets;
+
+err_rst:
+	while (--i >= 0)
+		reset_control_put(resets->rstc[i]);
+
+	kfree(resets);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(of_reset_control_array_get);
+
+/**
+ * devm_reset_control_array_get - Resource managed reset control array get
+ *
+ * @dev: device that requests the list of reset controls
+ * @shared: whether reset controls are shared or not
+ * @optional: whether it is optional to get the reset controls
+ *
+ * The reset control array APIs are intended for a list of resets
+ * that just have to be asserted or deasserted, without any
+ * requirements on the order.
+ *
+ * Returns pointer to allocated reset_control_array on success or
+ * error on failure
+ */
+struct reset_control_array *
+devm_reset_control_array_get(struct device *dev, bool shared, bool optional)
+{
+	struct reset_control_array **devres;
+	struct reset_control_array *resets;
+
+	devres = devres_alloc(devm_reset_control_array_release,
+			      sizeof(*devres), GFP_KERNEL);
+	if (!devres)
+		return ERR_PTR(-ENOMEM);
+
+	resets = of_reset_control_array_get(dev->of_node, shared, optional);
+	if (IS_ERR(resets)) {
+		devres_free(resets);
+		return resets;
+	}
+
+	*devres = resets;
+	devres_add(dev, devres);
+
+	return resets;
+}
+EXPORT_SYMBOL_GPL(devm_reset_control_array_get);
+
+void reset_control_array_put(struct reset_control_array *resets)
+{
+	int i;
+
+	if (IS_ERR_OR_NULL(resets))
+		return;
+
+	for (i = 0; i < resets->num_rstcs; i++)
+		reset_control_put(resets->rstc[i]);
+
+	kfree(resets);
+}
+EXPORT_SYMBOL_GPL(reset_control_array_put);
diff --git a/include/linux/reset.h b/include/linux/reset.h
index 1b5a6aafd3e6..8edb69e93355 100644
--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -5,6 +5,11 @@
 
 struct reset_control;
 
+struct reset_control_array {
+	unsigned int num_rstcs;
+	struct reset_control *rstc[];
+};
+
 #ifdef CONFIG_RESET_CONTROLLER
 
 int reset_control_reset(struct reset_control *rstc);
@@ -26,6 +31,14 @@ struct reset_control *__devm_reset_control_get(struct device *dev,
 int __must_check device_reset(struct device *dev);
 int of_reset_control_get_count(struct device_node *node);
 
+int reset_control_array_assert(struct reset_control_array *resets);
+int reset_control_array_deassert(struct reset_control_array *resets);
+struct reset_control_array *devm_reset_control_array_get(struct device *dev,
+						bool shared, bool optional);
+struct reset_control_array *of_reset_control_array_get(struct device_node *np,
+						bool shared, bool optional);
+void reset_control_array_put(struct reset_control_array *resets);
+
 static inline int device_reset_optional(struct device *dev)
 {
 	return device_reset(dev);
@@ -95,6 +108,35 @@ static inline int of_reset_control_get_count(struct device_node *node)
 	return -ENOTSUPP;
 }
 
+static inline
+int reset_control_array_assert(struct reset_control_array *resets)
+{
+	return 0;
+}
+
+static inline
+int reset_control_array_deassert(struct reset_control_array *resets)
+{
+	return 0;
+}
+
+static inline struct reset_control_array *
+devm_reset_control_array_get(struct device *dev, bool shared, bool optional)
+{
+	return optional ? NULL : ERR_PTR(-ENOTSUPP);
+}
+
+static inline struct reset_control_array *
+of_reset_control_array_get(struct device_node *np, bool shared, bool optional)
+{
+	return optional ? NULL : ERR_PTR(-ENOTSUPP);
+}
+
+static inline
+void reset_control_array_put(struct reset_control_array *resets)
+{
+}
+
 #endif /* CONFIG_RESET_CONTROLLER */
 
 /**
@@ -380,4 +422,55 @@ static inline struct reset_control *devm_reset_control_get_by_index(
 {
 	return devm_reset_control_get_exclusive_by_index(dev, index);
 }
+
+/*
+ * APIs to manage a list of reset controllers
+ */
+static inline struct reset_control_array *
+devm_reset_control_array_get_exclusive(struct device *dev)
+{
+	return devm_reset_control_array_get(dev, false, false);
+}
+
+static inline struct reset_control_array *
+devm_reset_control_array_get_shared(struct device *dev)
+{
+	return devm_reset_control_array_get(dev, true, false);
+}
+
+static inline struct reset_control_array *
+devm_reset_control_array_get_optional_exclusive(struct device *dev)
+{
+	return devm_reset_control_array_get(dev, false, true);
+}
+
+static inline struct reset_control_array *
+devm_reset_control_array_get_optional_shared(struct device *dev)
+{
+	return devm_reset_control_array_get(dev, true, true);
+}
+
+static inline struct reset_control_array *
+of_reset_control_array_get_exclusive(struct device_node *node)
+{
+	return of_reset_control_array_get(node, false, false);
+}
+
+static inline struct reset_control_array *
+of_reset_control_array_get_shared(struct device_node *node)
+{
+	return of_reset_control_array_get(node, true, false);
+}
+
+static inline struct reset_control_array *
+of_reset_control_array_get_optional_exclusive(struct device_node *node)
+{
+	return of_reset_control_array_get(node, false, true);
+}
+
+static inline struct reset_control_array *
+of_reset_control_array_get_optional_shared(struct device_node *node)
+{
+	return of_reset_control_array_get(node, true, true);
+}
 #endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V3 2/4] reset: Add APIs to manage array of resets

[Philipp Zabel]

On Tue, 2017-04-18 at 16:51 +0530, Vivek Gautam wrote:
> Many devices may want to request a bunch of resets
> and control them. So it's better to manage them as an
> array. Add APIs to _get(), _assert(), and _deassert()
> an array of reset_control.

Thanks! This looks good to me, one small issue below.

> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> ---
>  drivers/reset/core.c  | 177 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/reset.h |  93 ++++++++++++++++++++++++++
>  2 files changed, 270 insertions(+)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index f0a06a7aca93..54bd3be5e7a4 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -488,3 +488,180 @@ int of_reset_control_get_count(struct device_node *node)
>  	return count;
>  }
>  EXPORT_SYMBOL_GPL(of_reset_control_get_count);
> +
> +/**
> + * APIs to manage an array of reset controls.
> + */
> +/**
> + * reset_control_array_assert: assert a list of resets
> + *
> + * @resets: reset control array holding info about the list of resets
> + *
> + * This API doesn't guarantee that the reset lines controlled by
> + * the reset array are asserted in any particular order.
> + *
> + * Returns 0 on success or error number on failure.
> + */
> +int reset_control_array_assert(struct reset_control_array *resets)
> +{
> +	int ret, i;
> +
> +	if (!resets)
> +		return 0;
> +
> +	if (IS_ERR(resets))
> +		return -EINVAL;
> +
> +	for (i = 0; i < resets->num_rstcs; i++) {
> +		ret = reset_control_assert(resets->rstc[i]);
> +		if (ret)
> +			return ret;

This should try to deassert the already asserted resets in the error
case.

> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(reset_control_array_assert);
> +
> +/**
> + * reset_control_array_deassert: deassert a list of resets
> + *
> + * @resets: reset control array holding info about the list of resets
> + *
> + * This API doesn't guarantee that the reset lines controlled by
> + * the reset array are deasserted in any particular order.
> + *
> + * Returns 0 on success or error number on failure.
> + */
> +int reset_control_array_deassert(struct reset_control_array *resets)
> +{
> +	int ret, i;
> +
> +	if (!resets)
> +		return 0;
> +
> +	if (IS_ERR(resets))
> +		return -EINVAL;
> +
> +	for (i = 0; i < resets->num_rstcs; i++) {
> +		ret = reset_control_deassert(resets->rstc[i]);
> +		if (ret)
> +			goto err;
> +	}
> +
> +	return 0;
> +
> +err:
> +	while (i--)
> +		reset_control_assert(resets->rstc[i]);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(reset_control_array_deassert);

As this already does.

regards
Philipp

Re: [PATCH V3 2/4] reset: Add APIs to manage array of resets

[Vivek Gautam]

Hi Philipp,


On 04/19/2017 04:01 PM, Philipp Zabel wrote:
> On Tue, 2017-04-18 at 16:51 +0530, Vivek Gautam wrote:
>> Many devices may want to request a bunch of resets
>> and control them. So it's better to manage them as an
>> array. Add APIs to _get(), _assert(), and _deassert()
>> an array of reset_control.
> Thanks! This looks good to me, one small issue below.
>
>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>> ---
>>   drivers/reset/core.c  | 177 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/reset.h |  93 ++++++++++++++++++++++++++
>>   2 files changed, 270 insertions(+)
>>
>> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
>> index f0a06a7aca93..54bd3be5e7a4 100644
>> --- a/drivers/reset/core.c
>> +++ b/drivers/reset/core.c
>> @@ -488,3 +488,180 @@ int of_reset_control_get_count(struct device_node *node)
>>   	return count;
>>   }
>>   EXPORT_SYMBOL_GPL(of_reset_control_get_count);
>> +
>> +/**
>> + * APIs to manage an array of reset controls.
>> + */
>> +/**
>> + * reset_control_array_assert: assert a list of resets
>> + *
>> + * @resets: reset control array holding info about the list of resets
>> + *
>> + * This API doesn't guarantee that the reset lines controlled by
>> + * the reset array are asserted in any particular order.
>> + *
>> + * Returns 0 on success or error number on failure.
>> + */
>> +int reset_control_array_assert(struct reset_control_array *resets)
>> +{
>> +	int ret, i;
>> +
>> +	if (!resets)
>> +		return 0;
>> +
>> +	if (IS_ERR(resets))
>> +		return -EINVAL;
>> +
>> +	for (i = 0; i < resets->num_rstcs; i++) {
>> +		ret = reset_control_assert(resets->rstc[i]);
>> +		if (ret)
>> +			return ret;
> This should try to deassert the already asserted resets in the error
> case.

I assumed that the user will call _assert in case of failure, driver removal
or may be suspend, and thought that we may not care even if the _assert
failed. But i see now that we also care about the (de)assert count, so that
the shared resets are handled properly.

Will add the core to deassert the already asserted resets in error case.

Thanks

>
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(reset_control_array_assert);
>> +
>> +/**
>> + * reset_control_array_deassert: deassert a list of resets
>> + *
>> + * @resets: reset control array holding info about the list of resets
>> + *
>> + * This API doesn't guarantee that the reset lines controlled by
>> + * the reset array are deasserted in any particular order.
>> + *
>> + * Returns 0 on success or error number on failure.
>> + */
>> +int reset_control_array_deassert(struct reset_control_array *resets)
>> +{
>> +	int ret, i;
>> +
>> +	if (!resets)
>> +		return 0;
>> +
>> +	if (IS_ERR(resets))
>> +		return -EINVAL;
>> +
>> +	for (i = 0; i < resets->num_rstcs; i++) {
>> +		ret = reset_control_deassert(resets->rstc[i]);
>> +		if (ret)
>> +			goto err;
>> +	}
>> +
>> +	return 0;
>> +
>> +err:
>> +	while (i--)
>> +		reset_control_assert(resets->rstc[i]);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(reset_control_array_deassert);
> As this already does.

Yea, like this one.

Best Regards
Vivek

>
> regards
> Philipp
>

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

[PATCH V3 3/4] 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>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
---
 drivers/usb/dwc3/dwc3-of-simple.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
index fe414e7a9c78..9116df649f0b 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -29,13 +29,39 @@
 #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_array *resets;
 };
 
+static int dwc3_of_simple_reset_init(struct dwc3_of_simple *simple)
+{
+	struct device		*dev = simple->dev;
+	int			ret;
+
+	simple->resets = of_reset_control_array_get_exclusive(dev->of_node);
+	if (IS_ERR(simple->resets)) {
+		ret = PTR_ERR(simple->resets);
+		if (ret == -ENOENT)
+			/* not all controllers required resets */
+			return 0;
+		dev_err(dev, "failed to get device resets\n");
+		return ret;
+	}
+
+	ret = reset_control_array_deassert(simple->resets);
+	if (ret) {
+		reset_control_array_put(simple->resets);
+		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 +126,10 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = dwc3_of_simple_reset_init(simple);
+	if (ret)
+		return ret;
+
 	ret = of_platform_populate(np, NULL, NULL, dev);
 	if (ret) {
 		for (i = 0; i < simple->num_clocks; i++) {
@@ -107,6 +137,9 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
 			clk_put(simple->clks[i]);
 		}
 
+		reset_control_array_assert(simple->resets);
+		reset_control_array_put(simple->resets);
+
 		return ret;
 	}
 
@@ -128,6 +161,9 @@ static int dwc3_of_simple_remove(struct platform_device *pdev)
 		clk_put(simple->clks[i]);
 	}
 
+	reset_control_array_assert(simple->resets);
+	reset_control_array_put(simple->resets);
+
 	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 V3 3/4] usb: dwc3: of-simple: Add support to get resets for the device

[Philipp Zabel]

On Tue, 2017-04-18 at 16:51 +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>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> ---
>  drivers/usb/dwc3/dwc3-of-simple.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
> index fe414e7a9c78..9116df649f0b 100644
> --- a/drivers/usb/dwc3/dwc3-of-simple.c
> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> @@ -29,13 +29,39 @@
>  #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_array *resets;
>  };
>  
> +static int dwc3_of_simple_reset_init(struct dwc3_of_simple *simple)
> +{
> +	struct device		*dev = simple->dev;
> +	int			ret;
> +
> +	simple->resets = of_reset_control_array_get_exclusive(dev->of_node);
> +	if (IS_ERR(simple->resets)) {
> +		ret = PTR_ERR(simple->resets);
> +		if (ret == -ENOENT)
> +			/* not all controllers required resets */
> +			return 0;

If you use the of_reset_control_array_get_optional_exclusive variant,
you can remove the three lines directly above.

> +		dev_err(dev, "failed to get device resets\n");

It would be nice to print the error code here.

> +		return ret;
> +	}
> +
> +	ret = reset_control_array_deassert(simple->resets);
> +	if (ret) {
> +		reset_control_array_put(simple->resets);
> +		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 +126,10 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	ret = dwc3_of_simple_reset_init(simple);
> +	if (ret)
> +		return ret;
> +

I would just move the contents of dwc3_of_simple_reset_init here and add
a goto error path at the end of dwc3_of_simple_clk_init to handle the
reset control cleanup as needed.

>  	ret = of_platform_populate(np, NULL, NULL, dev);
>  	if (ret) {
>  		for (i = 0; i < simple->num_clocks; i++) {
> @@ -107,6 +137,9 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>  			clk_put(simple->clks[i]);
>  		}
>  
> +		reset_control_array_assert(simple->resets);
> +		reset_control_array_put(simple->resets);
> +
>  		return ret;
>  	}
>  
> @@ -128,6 +161,9 @@ static int dwc3_of_simple_remove(struct platform_device *pdev)
>  		clk_put(simple->clks[i]);
>  	}
>  
> +	reset_control_array_assert(simple->resets);
> +	reset_control_array_put(simple->resets);
> +

Must the reset be asserted before of_platform_depopulate?

>  	of_platform_depopulate(dev);

Given the order above, I'd expect the reset cleanup here.

>  	pm_runtime_put_sync(dev);

regards
Philipp

Re: [PATCH V3 3/4] usb: dwc3: of-simple: Add support to get resets for the device

[Vivek Gautam]

On 04/19/2017 04:02 PM, Philipp Zabel wrote:
> On Tue, 2017-04-18 at 16:51 +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>
>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>> ---
>>   drivers/usb/dwc3/dwc3-of-simple.c | 36 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 36 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
>> index fe414e7a9c78..9116df649f0b 100644
>> --- a/drivers/usb/dwc3/dwc3-of-simple.c
>> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
>> @@ -29,13 +29,39 @@
>>   #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_array *resets;
>>   };
>>   
>> +static int dwc3_of_simple_reset_init(struct dwc3_of_simple *simple)
>> +{
>> +	struct device		*dev = simple->dev;
>> +	int			ret;
>> +
>> +	simple->resets = of_reset_control_array_get_exclusive(dev->of_node);
>> +	if (IS_ERR(simple->resets)) {
>> +		ret = PTR_ERR(simple->resets);
>> +		if (ret == -ENOENT)
>> +			/* not all controllers required resets */
>> +			return 0;
> If you use the of_reset_control_array_get_optional_exclusive variant,
> you can remove the three lines directly above.

No, it's a little tricky here.
of_reset_control_get_count() returns an error code, that we then return from
of_reset_control_array_get().

This is something that tegra/pmc driver required (return errorno in case 
count
of reset controls is zero.

So we handle that error separately here.

>
>> +		dev_err(dev, "failed to get device resets\n");
> It would be nice to print the error code here.

Sure.

>
>> +		return ret;
>> +	}
>> +
>> +	ret = reset_control_array_deassert(simple->resets);
>> +	if (ret) {
>> +		reset_control_array_put(simple->resets);
>> +		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 +126,10 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>>   	if (ret)
>>   		return ret;
>>   
>> +	ret = dwc3_of_simple_reset_init(simple);
>> +	if (ret)
>> +		return ret;
>> +
> I would just move the contents of dwc3_of_simple_reset_init here and add
> a goto error path at the end of dwc3_of_simple_clk_init to handle the
> reset control cleanup as needed.

Okay, i can restructure it.

>
>>   	ret = of_platform_populate(np, NULL, NULL, dev);
>>   	if (ret) {
>>   		for (i = 0; i < simple->num_clocks; i++) {
>> @@ -107,6 +137,9 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>>   			clk_put(simple->clks[i]);
>>   		}
>>   
>> +		reset_control_array_assert(simple->resets);
>> +		reset_control_array_put(simple->resets);
>> +
>>   		return ret;
>>   	}
>>   
>> @@ -128,6 +161,9 @@ static int dwc3_of_simple_remove(struct platform_device *pdev)
>>   		clk_put(simple->clks[i]);
>>   	}
>>   
>> +	reset_control_array_assert(simple->resets);
>> +	reset_control_array_put(simple->resets);
>> +
> Must the reset be asserted before of_platform_depopulate?

Right, the order must be taken care of here. Will change this.

>
>>   	of_platform_depopulate(dev);
> Given the order above, I'd expect the reset cleanup here.

Sure.

>
>>   	pm_runtime_put_sync(dev);
> regards
> Philipp

Thanks for reviewing.

Best Regards
Vivek

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

[PATCH V3 4/4] soc/tegra: pmc: Use the new reset APIs to manage reset controllers

[Vivek Gautam]

Make use of reset_control_array_*() set of APIs to manage
an array of reset controllers available with the device.

Cc: Thierry Reding <treding@nvidia.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
---
 drivers/soc/tegra/pmc.c | 99 ++++++++++++++++++-------------------------------
 1 file changed, 36 insertions(+), 63 deletions(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index e233dd5dcab3..4d039fa3db1b 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -124,8 +124,7 @@ struct tegra_powergate {
 	unsigned int id;
 	struct clk **clks;
 	unsigned int num_clks;
-	struct reset_control **resets;
-	unsigned int num_resets;
+	struct reset_control_array *resets;
 };
 
 struct tegra_io_pad_soc {
@@ -348,32 +347,14 @@ static int tegra_powergate_enable_clocks(struct tegra_powergate *pg)
 	return err;
 }
 
-static int tegra_powergate_reset_assert(struct tegra_powergate *pg)
+static inline int tegra_powergate_reset_assert(struct tegra_powergate *pg)
 {
-	unsigned int i;
-	int err;
-
-	for (i = 0; i < pg->num_resets; i++) {
-		err = reset_control_assert(pg->resets[i]);
-		if (err)
-			return err;
-	}
-
-	return 0;
+	return reset_control_array_assert(pg->resets);
 }
 
-static int tegra_powergate_reset_deassert(struct tegra_powergate *pg)
+static inline int tegra_powergate_reset_deassert(struct tegra_powergate *pg)
 {
-	unsigned int i;
-	int err;
-
-	for (i = 0; i < pg->num_resets; i++) {
-		err = reset_control_deassert(pg->resets[i]);
-		if (err)
-			return err;
-	}
-
-	return 0;
+	return reset_control_array_deassert(pg->resets);
 }
 
 static int tegra_powergate_power_up(struct tegra_powergate *pg,
@@ -558,6 +539,7 @@ int tegra_powergate_sequence_power_up(unsigned int id, struct clk *clk,
 				      struct reset_control *rst)
 {
 	struct tegra_powergate pg;
+	struct reset_control_array *resets;
 	int err;
 
 	if (!tegra_powergate_is_available(id))
@@ -566,12 +548,25 @@ int tegra_powergate_sequence_power_up(unsigned int id, struct clk *clk,
 	pg.id = id;
 	pg.clks = &clk;
 	pg.num_clks = 1;
-	pg.resets = &rst;
-	pg.num_resets = 1;
+
+	resets = kzalloc(sizeof(*resets) + sizeof(resets->rstc[0]) * 1,
+			 GFP_KERNEL);
+	if (!resets)
+		return -ENOMEM;
+
+	resets->rstc[0] = rst;
+	pg.resets = resets;
 
 	err = tegra_powergate_power_up(&pg, false);
-	if (err)
+	if (err) {
 		pr_err("failed to turn on partition %d: %d\n", id, err);
+		goto free_reset;
+	}
+
+	return 0;
+
+free_reset:
+	kfree(resets);
 
 	return err;
 }
@@ -755,45 +750,26 @@ static int tegra_powergate_of_get_clks(struct tegra_powergate *pg,
 static int tegra_powergate_of_get_resets(struct tegra_powergate *pg,
 					 struct device_node *np, bool off)
 {
-	struct reset_control *rst;
-	unsigned int i, count;
 	int err;
 
-	count = of_count_phandle_with_args(np, "resets", "#reset-cells");
-	if (count == 0)
-		return -ENODEV;
-
-	pg->resets = kcalloc(count, sizeof(rst), GFP_KERNEL);
-	if (!pg->resets)
-		return -ENOMEM;
-
-	for (i = 0; i < count; i++) {
-		pg->resets[i] = of_reset_control_get_by_index(np, i);
-		if (IS_ERR(pg->resets[i])) {
-			err = PTR_ERR(pg->resets[i]);
-			goto error;
-		}
-
-		if (off)
-			err = reset_control_assert(pg->resets[i]);
-		else
-			err = reset_control_deassert(pg->resets[i]);
-
-		if (err) {
-			reset_control_put(pg->resets[i]);
-			goto error;
-		}
+	pg->resets = of_reset_control_array_get_exclusive(np);
+	if (IS_ERR(pg->resets)) {
+		pr_err("failed to get device resets\n");
+		return PTR_ERR(pg->resets);
 	}
 
-	pg->num_resets = count;
+	if (off)
+		err = reset_control_array_assert(pg->resets);
+	else
+		err = reset_control_array_deassert(pg->resets);
 
-	return 0;
+	if (err)
+		goto put_reset;
 
-error:
-	while (i--)
-		reset_control_put(pg->resets[i]);
+	return 0;
 
-	kfree(pg->resets);
+put_reset:
+	reset_control_array_put(pg->resets);
 
 	return err;
 }
@@ -885,10 +861,7 @@ static void tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np)
 	pm_genpd_remove(&pg->genpd);
 
 remove_resets:
-	while (pg->num_resets--)
-		reset_control_put(pg->resets[pg->num_resets]);
-
-	kfree(pg->resets);
+	reset_control_array_put(pg->resets);
 
 remove_clks:
 	while (pg->num_clks--)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V3 4/4] soc/tegra: pmc: Use the new reset APIs to manage reset controllers

[Philipp Zabel]

On Tue, 2017-04-18 at 16:51 +0530, Vivek Gautam wrote:
> Make use of reset_control_array_*() set of APIs to manage
> an array of reset controllers available with the device.
> 
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> ---
>  drivers/soc/tegra/pmc.c | 99 ++++++++++++++++++-------------------------------
>  1 file changed, 36 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index e233dd5dcab3..4d039fa3db1b 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -124,8 +124,7 @@ struct tegra_powergate {
>  	unsigned int id;
>  	struct clk **clks;
>  	unsigned int num_clks;
> -	struct reset_control **resets;
> -	unsigned int num_resets;
> +	struct reset_control_array *resets;
>  };
>  
>  struct tegra_io_pad_soc {
> @@ -348,32 +347,14 @@ static int tegra_powergate_enable_clocks(struct tegra_powergate *pg)
>  	return err;
>  }
>  
> -static int tegra_powergate_reset_assert(struct tegra_powergate *pg)
> +static inline int tegra_powergate_reset_assert(struct tegra_powergate *pg)
>  {
> -	unsigned int i;
> -	int err;
> -
> -	for (i = 0; i < pg->num_resets; i++) {
> -		err = reset_control_assert(pg->resets[i]);
> -		if (err)
> -			return err;
> -	}
> -
> -	return 0;
> +	return reset_control_array_assert(pg->resets);
>  }
>  
> -static int tegra_powergate_reset_deassert(struct tegra_powergate *pg)
> +static inline int tegra_powergate_reset_deassert(struct tegra_powergate *pg)
>  {
> -	unsigned int i;
> -	int err;
> -
> -	for (i = 0; i < pg->num_resets; i++) {
> -		err = reset_control_deassert(pg->resets[i]);
> -		if (err)
> -			return err;
> -	}
> -
> -	return 0;
> +	return reset_control_array_deassert(pg->resets);
>  }
>  
>  static int tegra_powergate_power_up(struct tegra_powergate *pg,
> @@ -558,6 +539,7 @@ int tegra_powergate_sequence_power_up(unsigned int id, struct clk *clk,
>  				      struct reset_control *rst)
>  {
>  	struct tegra_powergate pg;
> +	struct reset_control_array *resets;
>  	int err;
>  
>  	if (!tegra_powergate_is_available(id))
> @@ -566,12 +548,25 @@ int tegra_powergate_sequence_power_up(unsigned int id, struct clk *clk,
>  	pg.id = id;
>  	pg.clks = &clk;
>  	pg.num_clks = 1;
> -	pg.resets = &rst;
> -	pg.num_resets = 1;

Oh, I didn't notice that the resets array is often used for a single
reset. I think in this case instead of wrapping a reset_control_array
around the passed reset, you could store it as an additional pointer in
tegra_powergate->reset and add
	if (pg->reset)
		return reset_control_(de)assert(pg->resets);
to the tegra_powergate_reset_(de)assert functions ...

> +
> +	resets = kzalloc(sizeof(*resets) + sizeof(resets->rstc[0]) * 1,
> +			 GFP_KERNEL);
> +	if (!resets)
> +		return -ENOMEM;
> +
> +	resets->rstc[0] = rst;
> +	pg.resets = resets;
>  
>  	err = tegra_powergate_power_up(&pg, false);
> -	if (err)
> +	if (err) {
>  		pr_err("failed to turn on partition %d: %d\n", id, err);
> +		goto free_reset;
> +	}
> +
> +	return 0;
> +
> +free_reset:
> +	kfree(resets);
>  

... as having to open code this handling of struct reset_control_array
internals completely negates its convenience.

>  	return err;
>  }
> @@ -755,45 +750,26 @@ static int tegra_powergate_of_get_clks(struct tegra_powergate *pg,
>  static int tegra_powergate_of_get_resets(struct tegra_powergate *pg,
>  					 struct device_node *np, bool off)
>  {
> -	struct reset_control *rst;
> -	unsigned int i, count;
>  	int err;
>  
> -	count = of_count_phandle_with_args(np, "resets", "#reset-cells");
> -	if (count == 0)
> -		return -ENODEV;
> -
> -	pg->resets = kcalloc(count, sizeof(rst), GFP_KERNEL);
> -	if (!pg->resets)
> -		return -ENOMEM;
> -
> -	for (i = 0; i < count; i++) {
> -		pg->resets[i] = of_reset_control_get_by_index(np, i);
> -		if (IS_ERR(pg->resets[i])) {
> -			err = PTR_ERR(pg->resets[i]);
> -			goto error;
> -		}
> -
> -		if (off)
> -			err = reset_control_assert(pg->resets[i]);
> -		else
> -			err = reset_control_deassert(pg->resets[i]);
> -
> -		if (err) {
> -			reset_control_put(pg->resets[i]);
> -			goto error;
> -		}
> +	pg->resets = of_reset_control_array_get_exclusive(np);
> +	if (IS_ERR(pg->resets)) {
> +		pr_err("failed to get device resets\n");
> +		return PTR_ERR(pg->resets);
>  	}
>  
> -	pg->num_resets = count;
> +	if (off)
> +		err = reset_control_array_assert(pg->resets);
> +	else
> +		err = reset_control_array_deassert(pg->resets);
>  
> -	return 0;
> +	if (err)
> +		goto put_reset;
>  
> -error:
> -	while (i--)
> -		reset_control_put(pg->resets[i]);
> +	return 0;
>  
> -	kfree(pg->resets);
> +put_reset:
> +	reset_control_array_put(pg->resets);
>  
>  	return err;
>  }
> @@ -885,10 +861,7 @@ static void tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np)
>  	pm_genpd_remove(&pg->genpd);
>  
>  remove_resets:
> -	while (pg->num_resets--)
> -		reset_control_put(pg->resets[pg->num_resets]);
> -
> -	kfree(pg->resets);
> +	reset_control_array_put(pg->resets);
>  
>  remove_clks:
>  	while (pg->num_clks--)

regards
Philipp

Re: [PATCH V3 4/4] soc/tegra: pmc: Use the new reset APIs to manage reset controllers

[Jon Hunter]

On 18/04/17 12:21, Vivek Gautam wrote:
> Make use of reset_control_array_*() set of APIs to manage
> an array of reset controllers available with the device.

Before we apply this patch, I need to check to see if the order of the
resets managed by the PMC driver matter. Today the order of the resets
is determined by the order they appear in the DT node and although the
new APIs work in the same way they do not guarantee this. So let me
check to see if we can any concerns about ordering here. Otherwise would
be nice to use these APIs.

Cheers
Jon

-- 
nvpublic

Re: [PATCH V3 4/4] soc/tegra: pmc: Use the new reset APIs to manage reset controllers

[Vivek Gautam]

On 04/24/2017 06:15 PM, Jon Hunter wrote:
> On 18/04/17 12:21, Vivek Gautam wrote:
>> Make use of reset_control_array_*() set of APIs to manage
>> an array of reset controllers available with the device.
> Before we apply this patch, I need to check to see if the order of the
> resets managed by the PMC driver matter. Today the order of the resets
> is determined by the order they appear in the DT node and although the
> new APIs work in the same way they do not guarantee this. So let me
> check to see if we can any concerns about ordering here. Otherwise would
> be nice to use these APIs.

Right, that will be perfect.

Best regards
Vivek

>
> Cheers
> Jon
>

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

Re: [PATCH V3 4/4] soc/tegra: pmc: Use the new reset APIs to manage reset controllers

[Jon Hunter]

On 25/04/17 05:15, Vivek Gautam wrote:
> On 04/24/2017 06:15 PM, Jon Hunter wrote:
>> On 18/04/17 12:21, Vivek Gautam wrote:
>>> Make use of reset_control_array_*() set of APIs to manage
>>> an array of reset controllers available with the device.
>> Before we apply this patch, I need to check to see if the order of the
>> resets managed by the PMC driver matter. Today the order of the resets
>> is determined by the order they appear in the DT node and although the
>> new APIs work in the same way they do not guarantee this. So let me
>> check to see if we can any concerns about ordering here. Otherwise would
>> be nice to use these APIs.
> 
> Right, that will be perfect.

So I don't see any restrictions here and so I think this change is fine.

BTW, for the DT case, is there any reason why we don't just say the
order will be determine by the order the resets are list in the DT node?

Cheers
Jon

-- 
nvpublic

Re: [PATCH V3 4/4] soc/tegra: pmc: Use the new reset APIs to manage reset controllers

[Philipp Zabel]

On Tue, 2017-04-25 at 11:05 +0100, Jon Hunter wrote:
> On 25/04/17 05:15, Vivek Gautam wrote:
> > On 04/24/2017 06:15 PM, Jon Hunter wrote:
> >> On 18/04/17 12:21, Vivek Gautam wrote:
> >>> Make use of reset_control_array_*() set of APIs to manage
> >>> an array of reset controllers available with the device.
> >> Before we apply this patch, I need to check to see if the order of the
> >> resets managed by the PMC driver matter. Today the order of the resets
> >> is determined by the order they appear in the DT node and although the
> >> new APIs work in the same way they do not guarantee this. So let me
> >> check to see if we can any concerns about ordering here. Otherwise would
> >> be nice to use these APIs.
> > 
> > Right, that will be perfect.
> 
> So I don't see any restrictions here and so I think this change is fine.

Thank you for checking.

> BTW, for the DT case, is there any reason why we don't just say the
> order will be determine by the order the resets are list in the DT node?

I'd rather not make any promises, so I don't have to care about keeping
them. This makes it easier to think about and allows for more freedom in
changing the core code if needed.

What if in the future there is a use case for enabling a bunch of resets
by flipping a number of bits in a single register at the same time? Or
if people accidentally depend on the ordering when in reality there is a
small delay necessary between assertions that just happens to be hidden
by the framework overhead?

If there is a use case for an array of reset controls that must be
(de)asserted in a fixed order and doesn't need any delay between the
steps and is not suitable to be described by named resets for some
reason, we can discuss this. Until then, I'm happy that tegra pmc can
handle arrays without any particular ordering.

regards
Philipp

Re: [PATCH V3 4/4] soc/tegra: pmc: Use the new reset APIs to manage reset controllers

[Jon Hunter]

On 25/04/17 11:33, Philipp Zabel wrote:
> On Tue, 2017-04-25 at 11:05 +0100, Jon Hunter wrote:
>> On 25/04/17 05:15, Vivek Gautam wrote:
>>> On 04/24/2017 06:15 PM, Jon Hunter wrote:
>>>> On 18/04/17 12:21, Vivek Gautam wrote:
>>>>> Make use of reset_control_array_*() set of APIs to manage
>>>>> an array of reset controllers available with the device.
>>>> Before we apply this patch, I need to check to see if the order of the
>>>> resets managed by the PMC driver matter. Today the order of the resets
>>>> is determined by the order they appear in the DT node and although the
>>>> new APIs work in the same way they do not guarantee this. So let me
>>>> check to see if we can any concerns about ordering here. Otherwise would
>>>> be nice to use these APIs.
>>>
>>> Right, that will be perfect.
>>
>> So I don't see any restrictions here and so I think this change is fine.
> 
> Thank you for checking.
> 
>> BTW, for the DT case, is there any reason why we don't just say the
>> order will be determine by the order the resets are list in the DT node?
> 
> I'd rather not make any promises, so I don't have to care about keeping
> them. This makes it easier to think about and allows for more freedom in
> changing the core code if needed.
> 
> What if in the future there is a use case for enabling a bunch of resets
> by flipping a number of bits in a single register at the same time? Or
> if people accidentally depend on the ordering when in reality there is a
> small delay necessary between assertions that just happens to be hidden
> by the framework overhead?
> 
> If there is a use case for an array of reset controls that must be
> (de)asserted in a fixed order and doesn't need any delay between the
> steps and is not suitable to be described by named resets for some
> reason, we can discuss this. Until then, I'm happy that tegra pmc can
> handle arrays without any particular ordering.

OK, makes sense.

Thanks
Jon

-- 
nvpublic

Re: [PATCH V3 4/4] soc/tegra: pmc: Use the new reset APIs to manage reset controllers

[Vivek Gautam]

On 04/25/2017 04:24 PM, Jon Hunter wrote:
> On 25/04/17 11:33, Philipp Zabel wrote:
>> On Tue, 2017-04-25 at 11:05 +0100, Jon Hunter wrote:
>>> On 25/04/17 05:15, Vivek Gautam wrote:
>>>> On 04/24/2017 06:15 PM, Jon Hunter wrote:
>>>>> On 18/04/17 12:21, Vivek Gautam wrote:
>>>>>> Make use of reset_control_array_*() set of APIs to manage
>>>>>> an array of reset controllers available with the device.
>>>>> Before we apply this patch, I need to check to see if the order of the
>>>>> resets managed by the PMC driver matter. Today the order of the resets
>>>>> is determined by the order they appear in the DT node and although the
>>>>> new APIs work in the same way they do not guarantee this. So let me
>>>>> check to see if we can any concerns about ordering here. Otherwise would
>>>>> be nice to use these APIs.
>>>> Right, that will be perfect.
>>> So I don't see any restrictions here and so I think this change is fine.
>> Thank you for checking.
>>
>>> BTW, for the DT case, is there any reason why we don't just say the
>>> order will be determine by the order the resets are list in the DT node?
>> I'd rather not make any promises, so I don't have to care about keeping
>> them. This makes it easier to think about and allows for more freedom in
>> changing the core code if needed.
>>
>> What if in the future there is a use case for enabling a bunch of resets
>> by flipping a number of bits in a single register at the same time? Or
>> if people accidentally depend on the ordering when in reality there is a
>> small delay necessary between assertions that just happens to be hidden
>> by the framework overhead?
>>
>> If there is a use case for an array of reset controls that must be
>> (de)asserted in a fixed order and doesn't need any delay between the
>> steps and is not suitable to be described by named resets for some
>> reason, we can discuss this. Until then, I'm happy that tegra pmc can
>> handle arrays without any particular ordering.
> OK, makes sense.

Thanks Jon for testing this.

Best Regards
Vivek
>
> Thanks
> Jon
>

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

Re: [PATCH V3 4/4] soc/tegra: pmc: Use the new reset APIs to manage reset controllers

[Jon Hunter]

On 25/04/17 12:06, Vivek Gautam wrote:
> On 04/25/2017 04:24 PM, Jon Hunter wrote:
>> On 25/04/17 11:33, Philipp Zabel wrote:
>>> On Tue, 2017-04-25 at 11:05 +0100, Jon Hunter wrote:
>>>> On 25/04/17 05:15, Vivek Gautam wrote:
>>>>> On 04/24/2017 06:15 PM, Jon Hunter wrote:
>>>>>> On 18/04/17 12:21, Vivek Gautam wrote:
>>>>>>> Make use of reset_control_array_*() set of APIs to manage
>>>>>>> an array of reset controllers available with the device.
>>>>>> Before we apply this patch, I need to check to see if the order of
>>>>>> the
>>>>>> resets managed by the PMC driver matter. Today the order of the
>>>>>> resets
>>>>>> is determined by the order they appear in the DT node and although
>>>>>> the
>>>>>> new APIs work in the same way they do not guarantee this. So let me
>>>>>> check to see if we can any concerns about ordering here. Otherwise
>>>>>> would
>>>>>> be nice to use these APIs.
>>>>> Right, that will be perfect.
>>>> So I don't see any restrictions here and so I think this change is
>>>> fine.
>>> Thank you for checking.
>>>
>>>> BTW, for the DT case, is there any reason why we don't just say the
>>>> order will be determine by the order the resets are list in the DT
>>>> node?
>>> I'd rather not make any promises, so I don't have to care about keeping
>>> them. This makes it easier to think about and allows for more freedom in
>>> changing the core code if needed.
>>>
>>> What if in the future there is a use case for enabling a bunch of resets
>>> by flipping a number of bits in a single register at the same time? Or
>>> if people accidentally depend on the ordering when in reality there is a
>>> small delay necessary between assertions that just happens to be hidden
>>> by the framework overhead?
>>>
>>> If there is a use case for an array of reset controls that must be
>>> (de)asserted in a fixed order and doesn't need any delay between the
>>> steps and is not suitable to be described by named resets for some
>>> reason, we can discuss this. Until then, I'm happy that tegra pmc can
>>> handle arrays without any particular ordering.
>> OK, makes sense.
> 
> Thanks Jon for testing this.

Not tested yet :-)

However, I will test this just to confirm. Are you planning on sending
out a v4 soon?

Jon

-- 
nvpublic

Re: [PATCH V3 4/4] soc/tegra: pmc: Use the new reset APIs to manage reset controllers

[Vivek Gautam]

On Tue, Apr 25, 2017 at 4:41 PM, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> On 25/04/17 12:06, Vivek Gautam wrote:
>> On 04/25/2017 04:24 PM, Jon Hunter wrote:
>>> On 25/04/17 11:33, Philipp Zabel wrote:
>>>> On Tue, 2017-04-25 at 11:05 +0100, Jon Hunter wrote:
>>>>> On 25/04/17 05:15, Vivek Gautam wrote:
>>>>>> On 04/24/2017 06:15 PM, Jon Hunter wrote:
>>>>>>> On 18/04/17 12:21, Vivek Gautam wrote:
>>>>>>>> Make use of reset_control_array_*() set of APIs to manage
>>>>>>>> an array of reset controllers available with the device.
>>>>>>> Before we apply this patch, I need to check to see if the order of
>>>>>>> the
>>>>>>> resets managed by the PMC driver matter. Today the order of the
>>>>>>> resets
>>>>>>> is determined by the order they appear in the DT node and although
>>>>>>> the
>>>>>>> new APIs work in the same way they do not guarantee this. So let me
>>>>>>> check to see if we can any concerns about ordering here. Otherwise
>>>>>>> would
>>>>>>> be nice to use these APIs.
>>>>>> Right, that will be perfect.
>>>>> So I don't see any restrictions here and so I think this change is
>>>>> fine.
>>>> Thank you for checking.
>>>>
>>>>> BTW, for the DT case, is there any reason why we don't just say the
>>>>> order will be determine by the order the resets are list in the DT
>>>>> node?
>>>> I'd rather not make any promises, so I don't have to care about keeping
>>>> them. This makes it easier to think about and allows for more freedom in
>>>> changing the core code if needed.
>>>>
>>>> What if in the future there is a use case for enabling a bunch of resets
>>>> by flipping a number of bits in a single register at the same time? Or
>>>> if people accidentally depend on the ordering when in reality there is a
>>>> small delay necessary between assertions that just happens to be hidden
>>>> by the framework overhead?
>>>>
>>>> If there is a use case for an array of reset controls that must be
>>>> (de)asserted in a fixed order and doesn't need any delay between the
>>>> steps and is not suitable to be described by named resets for some
>>>> reason, we can discuss this. Until then, I'm happy that tegra pmc can
>>>> handle arrays without any particular ordering.
>>> OK, makes sense.
>>
>> Thanks Jon for testing this.
>
> Not tested yet :-)
>
> However, I will test this just to confirm. Are you planning on sending
> out a v4 soon?

Yes, I will send a v4 soon this week.

Thanks
Vivek


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