[U-Boot] [PATCH v1 0/3] reset: Add a managed API

[U-Boot] [PATCH v1 0/3] reset: Add a managed API

[Jean-Jacques Hiblot]

This is the 3rd of a few series, the goal of which is to facilitate
porting drivers from the linux kernel. Most of the series will be about
adding managed API to existing infrastructure (GPIO, reset, phy,...)

This particular series is about reset controllers. It adds a managed API,
close to that of linux. The main difference is that bulk and reset_ctl
are handled with different functions.


Jean-Jacques Hiblot (3):
  drivers: reset: Handle gracefully NULL pointers
  drivers: reset: Add a managed API to get reset controllers from the DT
  test: reset: Add tests for the managed API

 arch/sandbox/include/asm/reset.h   |   1 +
 drivers/reset/reset-uclass.c       | 146 +++++++++++++++++++++++++----
 drivers/reset/sandbox-reset-test.c |  50 ++++++++--
 drivers/reset/sandbox-reset.c      |  19 ++++
 include/reset.h                    | 135 +++++++++++++++++++++++++-
 test/dm/reset.c                    |  59 ++++++++++++
 6 files changed, 387 insertions(+), 23 deletions(-)

-- 
2.17.1

[U-Boot] [PATCH v1 1/3] drivers: reset: Handle gracefully NULL pointers

[Jean-Jacques Hiblot]

Prepare the way for a managed reset API by handling NULL pointers without
crashing nor failing.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---

 drivers/reset/reset-uclass.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c
index ee1a423ffb..1cfcc8b367 100644
--- a/drivers/reset/reset-uclass.c
+++ b/drivers/reset/reset-uclass.c
@@ -9,9 +9,12 @@
 #include <reset.h>
 #include <reset-uclass.h>
 
-static inline struct reset_ops *reset_dev_ops(struct udevice *dev)
+struct reset_ops nop_reset_ops = {
+};
+
+static inline struct reset_ops *reset_dev_ops(struct reset_ctl *r)
 {
-	return (struct reset_ops *)dev->driver->ops;
+	return r ? (struct reset_ops *)r->dev->driver->ops : &nop_reset_ops;
 }
 
 static int reset_of_xlate_default(struct reset_ctl *reset_ctl,
@@ -50,9 +53,10 @@ static int reset_get_by_index_tail(int ret, ofnode node,
 		debug("%s %d\n", ofnode_get_name(args->node), args->args[0]);
 		return ret;
 	}
-	ops = reset_dev_ops(dev_reset);
 
 	reset_ctl->dev = dev_reset;
+	ops = reset_dev_ops(reset_ctl);
+
 	if (ops->of_xlate)
 		ret = ops->of_xlate(reset_ctl, args);
 	else
@@ -151,29 +155,29 @@ int reset_get_by_name(struct udevice *dev, const char *name,
 
 int reset_request(struct reset_ctl *reset_ctl)
 {
-	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
+	struct reset_ops *ops = reset_dev_ops(reset_ctl);
 
 	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
 
-	return ops->request(reset_ctl);
+	return ops->request ? ops->request(reset_ctl) : 0;
 }
 
 int reset_free(struct reset_ctl *reset_ctl)
 {
-	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
+	struct reset_ops *ops = reset_dev_ops(reset_ctl);
 
 	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
 
-	return ops->free(reset_ctl);
+	return ops->free ? ops->free(reset_ctl) : 0;
 }
 
 int reset_assert(struct reset_ctl *reset_ctl)
 {
-	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
+	struct reset_ops *ops = reset_dev_ops(reset_ctl);
 
 	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
 
-	return ops->rst_assert(reset_ctl);
+	return ops->rst_assert ? ops->rst_assert(reset_ctl) : 0;
 }
 
 int reset_assert_bulk(struct reset_ctl_bulk *bulk)
@@ -191,11 +195,11 @@ int reset_assert_bulk(struct reset_ctl_bulk *bulk)
 
 int reset_deassert(struct reset_ctl *reset_ctl)
 {
-	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
+	struct reset_ops *ops = reset_dev_ops(reset_ctl);
 
 	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
 
-	return ops->rst_deassert(reset_ctl);
+	return ops->rst_deassert ? ops->rst_deassert(reset_ctl) : 0;
 }
 
 int reset_deassert_bulk(struct reset_ctl_bulk *bulk)
@@ -213,11 +217,11 @@ int reset_deassert_bulk(struct reset_ctl_bulk *bulk)
 
 int reset_status(struct reset_ctl *reset_ctl)
 {
-	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
+	struct reset_ops *ops = reset_dev_ops(reset_ctl);
 
 	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
 
-	return ops->rst_status(reset_ctl);
+	return ops->rst_status ? ops->rst_status(reset_ctl) : 0;
 }
 
 int reset_release_all(struct reset_ctl *reset_ctl, int count)
-- 
2.17.1

[U-Boot] [PATCH v1 1/3] drivers: reset: Handle gracefully NULL pointers

[Simon Glass]

Hi Jean-Jacques,

On Mon, 30 Sep 2019 at 08:31, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>
> Prepare the way for a managed reset API by handling NULL pointers without
> crashing nor failing.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> ---
>
>  drivers/reset/reset-uclass.c | 30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)

Same comment here about code size / Kconfig option.

Regards,
Simon

[PATCH v1 1/3] drivers: reset: Handle gracefully NULL pointers

[Pratyush Yadav]

Hi Simon,

On 29/10/19 07:48PM, Simon Glass wrote:
> Hi Jean-Jacques,
> 
> On Mon, 30 Sep 2019 at 08:31, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
> >
> > Prepare the way for a managed reset API by handling NULL pointers without
> > crashing nor failing.
> >
> > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> > ---
> >
> >  drivers/reset/reset-uclass.c | 30 +++++++++++++++++-------------
> >  1 file changed, 17 insertions(+), 13 deletions(-)
> 
> Same comment here about code size / Kconfig option.

A lot of the changes below are ternary operators. I'm not sure how to 
put them behind a Kconfig option to reduce code size. Do you want me to 
change the NULL check to a separate if() block to put behind an #ifdef?

One way of doing this is like in this patch [0]. The other would be to 
wrap individual if statements in #ifdef, which tends to hurt 
readability.

[0] https://patchwork.ozlabs.org/project/uboot/patch/20191001115130.18886-2-jjhiblot at ti.com/

> diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c
> index ee1a423ffb..1cfcc8b367 100644
> --- a/drivers/reset/reset-uclass.c
> +++ b/drivers/reset/reset-uclass.c
> @@ -9,9 +9,12 @@ 
>  #include <reset.h>
>  #include <reset-uclass.h>
>  
> -static inline struct reset_ops *reset_dev_ops(struct udevice *dev)
> +struct reset_ops nop_reset_ops = {
> +};
> +
> +static inline struct reset_ops *reset_dev_ops(struct reset_ctl *r)
>  {
> -	return (struct reset_ops *)dev->driver->ops;
> +	return r ? (struct reset_ops *)r->dev->driver->ops : &nop_reset_ops;
>  }
>  
>  static int reset_of_xlate_default(struct reset_ctl *reset_ctl,
> @@ -50,9 +53,10 @@  static int reset_get_by_index_tail(int ret, ofnode node,
>  		debug("%s %d\n", ofnode_get_name(args->node), args->args[0]);
>  		return ret;
>  	}
> -	ops = reset_dev_ops(dev_reset);
>  
>  	reset_ctl->dev = dev_reset;
> +	ops = reset_dev_ops(reset_ctl);
> +
>  	if (ops->of_xlate)
>  		ret = ops->of_xlate(reset_ctl, args);
>  	else
> @@ -151,29 +155,29 @@  int reset_get_by_name(struct udevice *dev, const char *name,
>  
>  int reset_request(struct reset_ctl *reset_ctl)
>  {
> -	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
> +	struct reset_ops *ops = reset_dev_ops(reset_ctl);
>  
>  	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
>  
> -	return ops->request(reset_ctl);
> +	return ops->request ? ops->request(reset_ctl) : 0;
>  }
>  
>  int reset_free(struct reset_ctl *reset_ctl)
>  {
> -	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
> +	struct reset_ops *ops = reset_dev_ops(reset_ctl);
>  
>  	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
>  
> -	return ops->free(reset_ctl);
> +	return ops->free ? ops->free(reset_ctl) : 0;
>  }
>  
>  int reset_assert(struct reset_ctl *reset_ctl)
>  {
> -	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
> +	struct reset_ops *ops = reset_dev_ops(reset_ctl);
>  
>  	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
>  
> -	return ops->rst_assert(reset_ctl);
> +	return ops->rst_assert ? ops->rst_assert(reset_ctl) : 0;
>  }
>  
>  int reset_assert_bulk(struct reset_ctl_bulk *bulk)
> @@ -191,11 +195,11 @@  int reset_assert_bulk(struct reset_ctl_bulk *bulk)
>  
>  int reset_deassert(struct reset_ctl *reset_ctl)
>  {
> -	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
> +	struct reset_ops *ops = reset_dev_ops(reset_ctl);
>  
>  	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
>  
> -	return ops->rst_deassert(reset_ctl);
> +	return ops->rst_deassert ? ops->rst_deassert(reset_ctl) : 0;
>  }
>  
>  int reset_deassert_bulk(struct reset_ctl_bulk *bulk)
> @@ -213,11 +217,11 @@  int reset_deassert_bulk(struct reset_ctl_bulk *bulk)
>  
>  int reset_status(struct reset_ctl *reset_ctl)
>  {
> -	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
> +	struct reset_ops *ops = reset_dev_ops(reset_ctl);
>  
>  	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
>  
> -	return ops->rst_status(reset_ctl);
> +	return ops->rst_status ? ops->rst_status(reset_ctl) : 0;
>  }
>  
>  int reset_release_all(struct reset_ctl *reset_ctl, int count)

-- 
Regards,
Pratyush Yadav
Texas Instruments India

[PATCH v1 1/3] drivers: reset: Handle gracefully NULL pointers

[Simon Glass]

Hi Pratyush,

On Wed, 27 May 2020 at 11:12, Pratyush Yadav <p.yadav@ti.com> wrote:
>
> Hi Simon,
>
> On 29/10/19 07:48PM, Simon Glass wrote:
> > Hi Jean-Jacques,
> >
> > On Mon, 30 Sep 2019 at 08:31, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
> > >
> > > Prepare the way for a managed reset API by handling NULL pointers without
> > > crashing nor failing.
> > >
> > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> > > ---
> > >
> > >  drivers/reset/reset-uclass.c | 30 +++++++++++++++++-------------
> > >  1 file changed, 17 insertions(+), 13 deletions(-)
> >
> > Same comment here about code size / Kconfig option.
>
> A lot of the changes below are ternary operators. I'm not sure how to
> put them behind a Kconfig option to reduce code size. Do you want me to
> change the NULL check to a separate if() block to put behind an #ifdef?
>
> One way of doing this is like in this patch [0]. The other would be to
> wrap individual if statements in #ifdef, which tends to hurt
> readability.

Well for this I would really favour:

  int reset_request(struct reset_ctl *reset_ctl)
  {
     struct reset_ops *ops;

if (!result_ctl)
    return 0;

ops = reset_dev_ops(reset_ctl->dev);

But why allow result_ctl to be NULL? You should explain that in your
commit message.

> > -     struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
> > +     struct reset_ops *ops = reset_dev_ops(reset_ctl);
> >
> >       debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
> >
> > -     return ops->request(reset_ctl);
> > +     return ops->request ? ops->request(reset_ctl) : 0;
> >  }
> >
>
> [0] https://patchwork.ozlabs.org/project/uboot/patch/20191001115130.18886-2-jjhiblot at ti.com/

[..]

Regards,
Simon

[U-Boot] [PATCH v1 2/3] drivers: reset: Add a managed API to get reset controllers from the DT

[Jean-Jacques Hiblot]

Add managed functions to get a reset_ctl from the device-tree, based on a
name or an index.
Also add a managed functions to get a reset_ctl_bulk (array of reset_ctl)
from the device-tree.

When the device is unbound, the reset controllers are automatically
released and the data structure is freed.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---

 drivers/reset/reset-uclass.c | 116 +++++++++++++++++++++++++++++-
 include/reset.h              | 135 ++++++++++++++++++++++++++++++++++-
 2 files changed, 247 insertions(+), 4 deletions(-)

diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c
index 1cfcc8b367..ae8395a5e9 100644
--- a/drivers/reset/reset-uclass.c
+++ b/drivers/reset/reset-uclass.c
@@ -8,6 +8,7 @@
 #include <fdtdec.h>
 #include <reset.h>
 #include <reset-uclass.h>
+#include <dm/lists.h>
 
 struct reset_ops nop_reset_ops = {
 };
@@ -101,13 +102,14 @@ int reset_get_by_index_nodev(ofnode node, int index,
 				       index > 0, reset_ctl);
 }
 
-int reset_get_bulk(struct udevice *dev, struct reset_ctl_bulk *bulk)
+static int __reset_get_bulk(struct udevice *dev, ofnode node,
+			    struct reset_ctl_bulk *bulk)
 {
 	int i, ret, err, count;
 	
 	bulk->count = 0;
 
-	count = dev_count_phandle_with_args(dev, "resets", "#reset-cells");
+	count = ofnode_count_phandle_with_args(node, "resets", "#reset-cells");
 	if (count < 1)
 		return count;
 
@@ -117,7 +119,7 @@ int reset_get_bulk(struct udevice *dev, struct reset_ctl_bulk *bulk)
 		return -ENOMEM;
 
 	for (i = 0; i < count; i++) {
-		ret = reset_get_by_index(dev, i, &bulk->resets[i]);
+		ret = reset_get_by_index_nodev(node, i, &bulk->resets[i]);
 		if (ret < 0)
 			goto bulk_get_err;
 
@@ -135,6 +137,11 @@ bulk_get_err:
 	return ret;
 }
 
+int reset_get_bulk(struct udevice *dev, struct reset_ctl_bulk *bulk)
+{
+	return __reset_get_bulk(dev, dev_ofnode(dev), bulk);
+}
+
 int reset_get_by_name(struct udevice *dev, const char *name,
 		     struct reset_ctl *reset_ctl)
 {
@@ -247,6 +254,109 @@ int reset_release_all(struct reset_ctl *reset_ctl, int count)
 	return 0;
 }
 
+static void devm_reset_release(struct udevice *dev, void *res)
+{
+	reset_free(res);
+}
+
+struct reset_ctl *devm_reset_control_get_by_index(struct udevice *dev,
+						  int index)
+{
+	int rc;
+	struct reset_ctl *reset_ctl;
+
+	reset_ctl = devres_alloc(devm_reset_release, sizeof(struct reset_ctl),
+				 __GFP_ZERO);
+	if (unlikely(!reset_ctl))
+		return ERR_PTR(-ENOMEM);
+
+	rc = reset_get_by_index(dev, index, reset_ctl);
+	if (rc)
+		return ERR_PTR(rc);
+
+	devres_add(dev, reset_ctl);
+	return reset_ctl;
+}
+
+struct reset_ctl *devm_reset_control_get(struct udevice *dev, const char *id)
+{
+	int rc;
+	struct reset_ctl *reset_ctl;
+
+	reset_ctl = devres_alloc(devm_reset_release, sizeof(struct reset_ctl),
+				 __GFP_ZERO);
+	if (unlikely(!reset_ctl))
+		return ERR_PTR(-ENOMEM);
+
+	rc = reset_get_by_name(dev, id, reset_ctl);
+	if (rc)
+		return ERR_PTR(rc);
+
+	devres_add(dev, reset_ctl);
+	return reset_ctl;
+}
+
+struct reset_ctl *devm_reset_control_get_optional(struct udevice *dev,
+						  const char *id)
+{
+	struct reset_ctl *r = devm_reset_control_get(dev, id);
+
+	if (IS_ERR(r))
+		return NULL;
+
+	return r;
+}
+
+static void devm_reset_bulk_release(struct udevice *dev, void *res)
+{
+	struct reset_ctl_bulk *bulk = res;
+
+	reset_release_all(bulk->resets, bulk->count);
+}
+
+struct reset_ctl_bulk *devm_reset_bulk_get_by_node(struct udevice *dev,
+						   ofnode node)
+{
+	int rc;
+	struct reset_ctl_bulk *bulk;
+
+	bulk = devres_alloc(devm_reset_bulk_release,
+			    sizeof(struct reset_ctl_bulk),
+			    __GFP_ZERO);
+	if (unlikely(!bulk))
+		return ERR_PTR(-ENOMEM);
+
+	rc = __reset_get_bulk(dev, node, bulk);
+	if (rc)
+		return ERR_PTR(rc);
+
+	devres_add(dev, bulk);
+	return bulk;
+}
+
+struct reset_ctl_bulk *devm_reset_bulk_get_optional_by_node(struct udevice *dev,
+							    ofnode node)
+{
+	struct reset_ctl_bulk *bulk;
+
+	bulk = devm_reset_bulk_get_by_node(dev, node);
+
+	if (IS_ERR(bulk))
+		return NULL;
+
+	return bulk;
+}
+
+struct reset_ctl_bulk *devm_reset_bulk_get(struct udevice *dev)
+{
+	return devm_reset_bulk_get_by_node(dev, dev_ofnode(dev));
+}
+
+struct reset_ctl_bulk *devm_reset_bulk_get_optional(struct udevice *dev)
+{
+	return devm_reset_bulk_get_optional_by_node(dev, dev_ofnode(dev));
+}
+
 UCLASS_DRIVER(reset) = {
 	.id		= UCLASS_RESET,
 	.name		= "reset",
diff --git a/include/reset.h b/include/reset.h
index 4fac4e6a20..6d0fceefff 100644
--- a/include/reset.h
+++ b/include/reset.h
@@ -6,7 +6,7 @@
 #ifndef _RESET_H
 #define _RESET_H
 
-#include <dm/ofnode.h>
+#include <dm.h>
 #include <linux/errno.h>
 
 /**
@@ -84,6 +84,98 @@ struct reset_ctl_bulk {
 };
 
 #if CONFIG_IS_ENABLED(DM_RESET)
+
+/**
+ * devm_reset_control_get - resource managed reset_get_by_name()
+ * @dev: device to be reset by the controller
+ * @id: reset line name
+ *
+ * Managed reset_get_by_name(). For reset controllers returned
+ * from this function, reset_free() is called automatically on driver
+ * detach.
+ *
+ * Returns a struct reset_ctl or IS_ERR() condition containing errno.
+ */
+struct reset_ctl *devm_reset_control_get(struct udevice *dev, const char *id);
+
+/**
+ * devm_reset_control_get_optional - resource managed reset_get_by_name() that
+ *                                   can fail
+ * @dev:	The client device.
+ * @id:		reset line name
+ *
+ * Managed reset_get_by_name(). For reset controllers returned
+ * from this function, reset_free() is called automatically on driver
+ * detach.
+ *
+ * Returns a struct reset_ctl or a dummy reset controller if it failed.
+ */
+struct reset_ctl *devm_reset_control_get_optional(struct udevice *dev,
+						  const char *id);
+
+/**
+ * devm_reset_control_get - resource managed reset_get_by_index()
+ * @dev:	The client device.
+ * @index:	The index of the reset signal to request, within the client's
+ *		list of reset signals.
+ *
+ * Managed reset_get_by_index(). For reset controllers returned
+ * from this function, reset_free() is called automatically on driver
+ * detach.
+ *
+ * Returns a struct reset_ctl or IS_ERR() condition containing errno.
+ */
+struct reset_ctl *devm_reset_control_get_by_index(struct udevice *dev,
+						  int index);
+
+/**
+ * devm_reset_bulk_get - resource managed reset_get_bulk()
+ * @dev: device to be reset by the controller
+ *
+ * Managed reset_get_bulk(). For reset controllers returned
+ * from this function, reset_free() is called automatically on driver
+ * detach.
+ *
+ * Returns a struct reset_ctl or IS_ERR() condition containing errno.
+ */
+struct reset_ctl_bulk *devm_reset_bulk_get(struct udevice *dev);
+
+/**
+ * devm_reset_bulk_get_optional - resource managed reset_get_bulk() that
+ *                                can fail
+ * @dev:	The client device.
+ *
+ * Managed reset_get_bulk(). For reset controllers returned
+ * from this function, reset_free() is called automatically on driver
+ * detach.
+ *
+ * Returns a struct reset_ctl or NULL if it failed.
+ */
+struct reset_ctl_bulk *devm_reset_bulk_get_optional(struct udevice *dev);
+
+/**
+ * devm_reset_bulk_get_by_node - resource managed reset_get_bulk()
+ * @dev: device to be reset by the controller
+ * @node: ofnode where the "resets" property is. Usually a sub-node of
+ *        the dev's node.
+ *
+ * see devm_reset_bulk_get()
+ */
+struct reset_ctl_bulk *devm_reset_bulk_get_by_node(struct udevice *dev,
+						   ofnode node);
+
+/**
+ * devm_reset_bulk_get_optional_by_node - resource managed reset_get_bulk()
+ *                                        that can fail
+ * @dev: device to be reset by the controller
+ * @node: ofnode where the "resets" property is. Usually a sub-node of
+ *        the dev's node.
+ *
+ * see devm_reset_bulk_get_optional()
+ */
+struct reset_ctl_bulk *devm_reset_bulk_get_optional_by_node(struct udevice *dev,
+							    ofnode node);
+
 /**
  * reset_get_by_index - Get/request a reset signal by integer index.
  *
@@ -265,7 +357,48 @@ static inline int reset_release_bulk(struct reset_ctl_bulk *bulk)
 {
 	return reset_release_all(bulk->resets, bulk->count);
 }
+
 #else
+static inline struct reset_ctl *devm_reset_control_get(struct udevice *dev,
+						       const char *id)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
+
+static inline struct reset_ctl *devm_reset_control_get_optional(struct udevice *dev,
+								const char *id)
+{
+	return NULL;
+}
+
+static inline struct reset_ctl *devm_reset_control_get_by_index(struct udevice *dev,
+								int index)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
+
+static inline struct reset_ctl_bulk *devm_reset_bulk_get(struct udevice *dev)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
+
+static inline struct reset_ctl_bulk *devm_reset_bulk_get_optional(struct udevice *dev)
+{
+	return NULL;
+}
+
+static inline struct reset_ctl_bulk *devm_reset_bulk_get_by_node(struct udevice *dev,
+								 ofnode node)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
+
+static inline struct reset_ctl_bulk *devm_reset_bulk_get_optional_by_node(struct udevice *dev,
+									  ofnode node)
+{
+	return NULL;
+}
+
 static inline int reset_get_by_index(struct udevice *dev, int index,
 				     struct reset_ctl *reset_ctl)
 {
-- 
2.17.1

[U-Boot] [PATCH v1 2/3] drivers: reset: Add a managed API to get reset controllers from the DT

[Simon Glass]

On Mon, 30 Sep 2019 at 10:15, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>
> Add managed functions to get a reset_ctl from the device-tree, based on a
> name or an index.
> Also add a managed functions to get a reset_ctl_bulk (array of reset_ctl)
> from the device-tree.
>
> When the device is unbound, the reset controllers are automatically
> released and the data structure is freed.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> ---
>
>  drivers/reset/reset-uclass.c | 116 +++++++++++++++++++++++++++++-
>  include/reset.h              | 135 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 247 insertions(+), 4 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

I really don't like these ERR_PTR returns. I suppose they make the
code easier to port, and we can be sure that pointers will not be in
the last 4KB of address space?

Regards,
Simon

[U-Boot] [PATCH v1 2/3] drivers: reset: Add a managed API to get reset controllers from the DT

[Jean-Jacques Hiblot]

On 30/10/2019 02:48, Simon Glass wrote:
> On Mon, 30 Sep 2019 at 10:15, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>> Add managed functions to get a reset_ctl from the device-tree, based on a
>> name or an index.
>> Also add a managed functions to get a reset_ctl_bulk (array of reset_ctl)
>> from the device-tree.
>>
>> When the device is unbound, the reset controllers are automatically
>> released and the data structure is freed.
>>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>> ---
>>
>>   drivers/reset/reset-uclass.c | 116 +++++++++++++++++++++++++++++-
>>   include/reset.h              | 135 ++++++++++++++++++++++++++++++++++-
>>   2 files changed, 247 insertions(+), 4 deletions(-)
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> I really don't like these ERR_PTR returns. I suppose they make the
> code easier to port, and we can be sure that pointers will not be in
> the last 4KB of address space?

It seems rather unlikely because the returned pointer points to actual 
RAM allocated from the heap. On most platforms I've worked with, the top 
of the address space is not dedicated to memory. If ever the need to fix 
this  arises it could done by tweaking the macros to use another unused 
address space.

JJ

>
> Regards,
> Simon
>

[U-Boot] [PATCH v1 2/3] drivers: reset: Add a managed API to get reset controllers from the DT

[Simon Glass]

Hi Jean-Jacques,

On Mon, 4 Nov 2019 at 08:41, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>
>
> On 30/10/2019 02:48, Simon Glass wrote:
> > On Mon, 30 Sep 2019 at 10:15, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
> >> Add managed functions to get a reset_ctl from the device-tree, based on a
> >> name or an index.
> >> Also add a managed functions to get a reset_ctl_bulk (array of reset_ctl)
> >> from the device-tree.
> >>
> >> When the device is unbound, the reset controllers are automatically
> >> released and the data structure is freed.
> >>
> >> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> >> ---
> >>
> >>   drivers/reset/reset-uclass.c | 116 +++++++++++++++++++++++++++++-
> >>   include/reset.h              | 135 ++++++++++++++++++++++++++++++++++-
> >>   2 files changed, 247 insertions(+), 4 deletions(-)
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > I really don't like these ERR_PTR returns. I suppose they make the
> > code easier to port, and we can be sure that pointers will not be in
> > the last 4KB of address space?
>
> It seems rather unlikely because the returned pointer points to actual
> RAM allocated from the heap. On most platforms I've worked with, the top
> of the address space is not dedicated to memory.

Yes that's my comfort.

> If ever the need to fix
> this  arises it could done by tweaking the macros to use another unused
> address space.

Not easily without doing something platform-specific, as someone else
is currently pushing.

Regards,
Simon

[U-Boot] [PATCH v1 2/3] drivers: reset: Add a managed API to get reset controllers from the DT

[Simon Goldschmidt]

Am 05.11.2019 um 17:33 schrieb Simon Glass:
> Hi Jean-Jacques,
> 
> On Mon, 4 Nov 2019 at 08:41, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>>
>>
>> On 30/10/2019 02:48, Simon Glass wrote:
>>> On Mon, 30 Sep 2019 at 10:15, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>>>> Add managed functions to get a reset_ctl from the device-tree, based on a
>>>> name or an index.
>>>> Also add a managed functions to get a reset_ctl_bulk (array of reset_ctl)
>>>> from the device-tree.
>>>>
>>>> When the device is unbound, the reset controllers are automatically
>>>> released and the data structure is freed.
>>>>
>>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>>>> ---
>>>>
>>>>    drivers/reset/reset-uclass.c | 116 +++++++++++++++++++++++++++++-
>>>>    include/reset.h              | 135 ++++++++++++++++++++++++++++++++++-
>>>>    2 files changed, 247 insertions(+), 4 deletions(-)
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>
>>> I really don't like these ERR_PTR returns. I suppose they make the
>>> code easier to port, and we can be sure that pointers will not be in
>>> the last 4KB of address space?
>>
>> It seems rather unlikely because the returned pointer points to actual
>> RAM allocated from the heap. On most platforms I've worked with, the top
>> of the address space is not dedicated to memory.

Most != all: on socfpga, the internal SRAM is at the end of the address 
spcae. In SPL, this means the last 4K cannot be used.

However, that shouldn't keep us from porting ERR_PTR returns from Linux 
code.

> 
> Yes that's my comfort.
> 
>> If ever the need to fix
>> this  arises it could done by tweaking the macros to use another unused
>> address space.
> 
> Not easily without doing something platform-specific, as someone else
> is currently pushing.

That "someone else" would be me. Sadly, I did not get any response:

https://patchwork.ozlabs.org/project/uboot/list/?series=137880


Regards,
Simon

[U-Boot] [PATCH v1 2/3] drivers: reset: Add a managed API to get reset controllers from the DT

[Jean-Jacques Hiblot]

On 05/11/2019 17:42, Simon Goldschmidt wrote:
> Am 05.11.2019 um 17:33 schrieb Simon Glass:
>> Hi Jean-Jacques,
>>
>> On Mon, 4 Nov 2019 at 08:41, Jean-Jacques Hiblot <jjhiblot@ti.com> 
>> wrote:
>>>
>>>
>>> On 30/10/2019 02:48, Simon Glass wrote:
>>>> On Mon, 30 Sep 2019 at 10:15, Jean-Jacques Hiblot <jjhiblot@ti.com> 
>>>> wrote:
>>>>> Add managed functions to get a reset_ctl from the device-tree, 
>>>>> based on a
>>>>> name or an index.
>>>>> Also add a managed functions to get a reset_ctl_bulk (array of 
>>>>> reset_ctl)
>>>>> from the device-tree.
>>>>>
>>>>> When the device is unbound, the reset controllers are automatically
>>>>> released and the data structure is freed.
>>>>>
>>>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>>>>> ---
>>>>>
>>>>>    drivers/reset/reset-uclass.c | 116 +++++++++++++++++++++++++++++-
>>>>>    include/reset.h              | 135 
>>>>> ++++++++++++++++++++++++++++++++++-
>>>>>    2 files changed, 247 insertions(+), 4 deletions(-)
>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>
>>>> I really don't like these ERR_PTR returns. I suppose they make the
>>>> code easier to port, and we can be sure that pointers will not be in
>>>> the last 4KB of address space?
>>>
>>> It seems rather unlikely because the returned pointer points to actual
>>> RAM allocated from the heap. On most platforms I've worked with, the 
>>> top
>>> of the address space is not dedicated to memory.
>
> Most != all: on socfpga, the internal SRAM is at the end of the 
> address spcae. In SPL, this means the last 4K cannot be used.
>
> However, that shouldn't keep us from porting ERR_PTR returns from 
> Linux code.
>
>>
>> Yes that's my comfort.
>>
>>> If ever the need to fix
>>> this  arises it could done by tweaking the macros to use another unused
>>> address space.
>>
>> Not easily without doing something platform-specific, as someone else
>> is currently pushing.
>
> That "someone else" would be me. Sadly, I did not get any response:
>
> https://patchwork.ozlabs.org/project/uboot/list/?series=137880


Alternatively we could use BIT(0) to flag an error since allocations are 
always aligned on 4 or more (sizeof(ulong) or 2*sizeof(size_t))


>
>
> Regards,
> Simon
>

[U-Boot] [PATCH v1 3/3] test: reset: Add tests for the managed API

[Jean-Jacques Hiblot]

The tests are basically the same as for the regular API. Except that
the reset are initialized using the managed API, and no freed manually.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>

---

 arch/sandbox/include/asm/reset.h   |  1 +
 drivers/reset/sandbox-reset-test.c | 50 ++++++++++++++++++++++---
 drivers/reset/sandbox-reset.c      | 19 ++++++++++
 test/dm/reset.c                    | 59 ++++++++++++++++++++++++++++++
 4 files changed, 123 insertions(+), 6 deletions(-)

diff --git a/arch/sandbox/include/asm/reset.h b/arch/sandbox/include/asm/reset.h
index c4205eabef..a0065b96ad 100644
--- a/arch/sandbox/include/asm/reset.h
+++ b/arch/sandbox/include/asm/reset.h
@@ -11,6 +11,7 @@
 struct udevice;
 
 int sandbox_reset_query(struct udevice *dev, unsigned long id);
+int sandbox_reset_is_requested(struct udevice *dev, unsigned long id);
 
 int sandbox_reset_test_get(struct udevice *dev);
 int sandbox_reset_test_get_bulk(struct udevice *dev);
diff --git a/drivers/reset/sandbox-reset-test.c b/drivers/reset/sandbox-reset-test.c
index 95ce2ca117..e6e976ea11 100644
--- a/drivers/reset/sandbox-reset-test.c
+++ b/drivers/reset/sandbox-reset-test.c
@@ -12,62 +12,100 @@
 struct sandbox_reset_test {
 	struct reset_ctl ctl;
 	struct reset_ctl_bulk bulk;
+
+	struct reset_ctl *ctlp;
+	struct reset_ctl_bulk *bulkp;
 };
 
 int sandbox_reset_test_get(struct udevice *dev)
 {
 	struct sandbox_reset_test *sbrt = dev_get_priv(dev);
 
+	sbrt->ctlp = &sbrt->ctl;
 	return reset_get_by_name(dev, "test", &sbrt->ctl);
 }
 
+int sandbox_reset_test_get_devm(struct udevice *dev)
+{
+	struct sandbox_reset_test *sbrt = dev_get_priv(dev);
+	struct reset_ctl *r;
+
+	r = devm_reset_control_get(dev, "not-a-valid-reset-ctl");
+	if (!IS_ERR(r))
+		return -EINVAL;
+
+	r = devm_reset_control_get_optional(dev, "not-a-valid-reset-ctl");
+	if (r)
+		return -EINVAL;
+
+	sbrt->ctlp = devm_reset_control_get(dev, "test");
+	if (IS_ERR(sbrt->ctlp))
+		return PTR_ERR(sbrt->ctlp);
+
+	return 0;
+}
+
 int sandbox_reset_test_get_bulk(struct udevice *dev)
 {
 	struct sandbox_reset_test *sbrt = dev_get_priv(dev);
 
+	sbrt->bulkp = &sbrt->bulk;
 	return reset_get_bulk(dev, &sbrt->bulk);
 }
 
+int sandbox_reset_test_get_bulk_devm(struct udevice *dev)
+{
+	struct sandbox_reset_test *sbrt = dev_get_priv(dev);
+	struct reset_ctl_bulk *r;
+
+	r = devm_reset_bulk_get_optional(dev);
+	if (IS_ERR(r))
+		return PTR_ERR(r);
+
+	sbrt->bulkp = r;
+	return 0;
+}
+
 int sandbox_reset_test_assert(struct udevice *dev)
 {
 	struct sandbox_reset_test *sbrt = dev_get_priv(dev);
 
-	return reset_assert(&sbrt->ctl);
+	return reset_assert(sbrt->ctlp);
 }
 
 int sandbox_reset_test_assert_bulk(struct udevice *dev)
 {
 	struct sandbox_reset_test *sbrt = dev_get_priv(dev);
 
-	return reset_assert_bulk(&sbrt->bulk);
+	return reset_assert_bulk(sbrt->bulkp);
 }
 
 int sandbox_reset_test_deassert(struct udevice *dev)
 {
 	struct sandbox_reset_test *sbrt = dev_get_priv(dev);
 
-	return reset_deassert(&sbrt->ctl);
+	return reset_deassert(sbrt->ctlp);
 }
 
 int sandbox_reset_test_deassert_bulk(struct udevice *dev)
 {
 	struct sandbox_reset_test *sbrt = dev_get_priv(dev);
 
-	return reset_deassert_bulk(&sbrt->bulk);
+	return reset_deassert_bulk(sbrt->bulkp);
 }
 
 int sandbox_reset_test_free(struct udevice *dev)
 {
 	struct sandbox_reset_test *sbrt = dev_get_priv(dev);
 
-	return reset_free(&sbrt->ctl);
+	return reset_free(sbrt->ctlp);
 }
 
 int sandbox_reset_test_release_bulk(struct udevice *dev)
 {
 	struct sandbox_reset_test *sbrt = dev_get_priv(dev);
 
-	return reset_release_bulk(&sbrt->bulk);
+	return reset_release_bulk(sbrt->bulkp);
 }
 
 static const struct udevice_id sandbox_reset_test_ids[] = {
diff --git a/drivers/reset/sandbox-reset.c b/drivers/reset/sandbox-reset.c
index 40f2654d8e..2b5ae88187 100644
--- a/drivers/reset/sandbox-reset.c
+++ b/drivers/reset/sandbox-reset.c
@@ -13,6 +13,7 @@
 
 struct sandbox_reset_signal {
 	bool asserted;
+	bool requested;
 };
 
 struct sandbox_reset {
@@ -21,18 +22,24 @@ struct sandbox_reset {
 
 static int sandbox_reset_request(struct reset_ctl *reset_ctl)
 {
+	struct sandbox_reset *sbr = dev_get_priv(reset_ctl->dev);
+
 	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
 
 	if (reset_ctl->id >= SANDBOX_RESET_SIGNALS)
 		return -EINVAL;
 
+	sbr->signals[reset_ctl->id].requested = true;
 	return 0;
 }
 
 static int sandbox_reset_free(struct reset_ctl *reset_ctl)
 {
+	struct sandbox_reset *sbr = dev_get_priv(reset_ctl->dev);
+
 	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
 
+	sbr->signals[reset_ctl->id].requested = false;
 	return 0;
 }
 
@@ -105,3 +112,15 @@ int sandbox_reset_query(struct udevice *dev, unsigned long id)
 
 	return sbr->signals[id].asserted;
 }
+
+int sandbox_reset_is_requested(struct udevice *dev, unsigned long id)
+{
+	struct sandbox_reset *sbr = dev_get_priv(dev);
+
+	debug("%s(dev=%p, id=%ld)\n", __func__, dev, id);
+
+	if (id >= SANDBOX_RESET_SIGNALS)
+		return -EINVAL;
+
+	return sbr->signals[id].requested;
+}
diff --git a/test/dm/reset.c b/test/dm/reset.c
index c61daed490..7b4ae971bf 100644
--- a/test/dm/reset.c
+++ b/test/dm/reset.c
@@ -57,12 +57,39 @@ static int dm_test_reset(struct unit_test_state *uts)
 	ut_assertok(sandbox_reset_test_deassert(dev_test));
 	ut_asserteq(0, sandbox_reset_query(dev_reset, TEST_RESET_ID));
 
+	ut_asserteq(1, sandbox_reset_is_requested(dev_reset, TEST_RESET_ID));
 	ut_assertok(sandbox_reset_test_free(dev_test));
+	ut_asserteq(0, sandbox_reset_is_requested(dev_reset, TEST_RESET_ID));
 
 	return 0;
 }
 DM_TEST(dm_test_reset, DM_TESTF_SCAN_FDT);
 
+static int dm_test_reset_devm(struct unit_test_state *uts)
+{
+	struct udevice *dev_reset;
+	struct udevice *dev_test;
+
+	ut_assertok(uclass_get_device_by_name(UCLASS_RESET, "reset-ctl",
+					      &dev_reset));
+	ut_asserteq(0, sandbox_reset_query(dev_reset, TEST_RESET_ID));
+	ut_assertok(uclass_get_device_by_name(UCLASS_MISC, "reset-ctl-test",
+					      &dev_test));
+	ut_assertok(sandbox_reset_test_get_devm(dev_test));
+
+	ut_assertok(sandbox_reset_test_assert(dev_test));
+	ut_asserteq(1, sandbox_reset_query(dev_reset, TEST_RESET_ID));
+	ut_assertok(sandbox_reset_test_deassert(dev_test));
+	ut_asserteq(0, sandbox_reset_query(dev_reset, TEST_RESET_ID));
+
+	ut_asserteq(1, sandbox_reset_is_requested(dev_reset, TEST_RESET_ID));
+	ut_assertok(device_remove(dev_test, DM_REMOVE_NORMAL));
+	ut_asserteq(0, sandbox_reset_is_requested(dev_reset, TEST_RESET_ID));
+
+	return 0;
+}
+DM_TEST(dm_test_reset_devm, DM_TESTF_SCAN_FDT);
+
 static int dm_test_reset_bulk(struct unit_test_state *uts)
 {
 	struct udevice *dev_reset;
@@ -92,3 +119,35 @@ static int dm_test_reset_bulk(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_reset_bulk, DM_TESTF_SCAN_FDT);
+
+static int dm_test_reset_bulk_devm(struct unit_test_state *uts)
+{
+	struct udevice *dev_reset;
+	struct udevice *dev_test;
+
+	ut_assertok(uclass_get_device_by_name(UCLASS_RESET, "reset-ctl",
+					      &dev_reset));
+	ut_asserteq(0, sandbox_reset_query(dev_reset, TEST_RESET_ID));
+	ut_asserteq(0, sandbox_reset_query(dev_reset, OTHER_RESET_ID));
+
+	ut_assertok(uclass_get_device_by_name(UCLASS_MISC, "reset-ctl-test",
+					      &dev_test));
+	ut_assertok(sandbox_reset_test_get_bulk_devm(dev_test));
+
+	ut_assertok(sandbox_reset_test_assert_bulk(dev_test));
+	ut_asserteq(1, sandbox_reset_query(dev_reset, TEST_RESET_ID));
+	ut_asserteq(1, sandbox_reset_query(dev_reset, OTHER_RESET_ID));
+
+	ut_assertok(sandbox_reset_test_deassert_bulk(dev_test));
+	ut_asserteq(0, sandbox_reset_query(dev_reset, TEST_RESET_ID));
+	ut_asserteq(0, sandbox_reset_query(dev_reset, OTHER_RESET_ID));
+
+	ut_asserteq(1, sandbox_reset_is_requested(dev_reset, OTHER_RESET_ID));
+	ut_asserteq(1, sandbox_reset_is_requested(dev_reset, TEST_RESET_ID));
+	ut_assertok(device_remove(dev_test, DM_REMOVE_NORMAL));
+	ut_asserteq(0, sandbox_reset_is_requested(dev_reset, TEST_RESET_ID));
+	ut_asserteq(0, sandbox_reset_is_requested(dev_reset, OTHER_RESET_ID));
+
+	return 0;
+}
+DM_TEST(dm_test_reset_bulk_devm, DM_TESTF_SCAN_FDT);
-- 
2.17.1