diff mbox series

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

Message ID 20190930142449.4480-3-jjhiblot@ti.com
State Deferred
Delegated to: Tom Rini
Headers show
Series reset: Add a managed API | expand

Commit Message

Jean-Jacques Hiblot Sept. 30, 2019, 2:24 p.m. UTC
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(-)

Comments

Simon Glass Oct. 30, 2019, 1:48 a.m. UTC | #1
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
Jean-Jacques Hiblot Nov. 4, 2019, 3:17 p.m. UTC | #2
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
>
Simon Glass Nov. 5, 2019, 4:33 p.m. UTC | #3
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
Simon Goldschmidt Nov. 5, 2019, 4:42 p.m. UTC | #4
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
Jean-Jacques Hiblot Nov. 5, 2019, 5:27 p.m. UTC | #5
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
>
diff mbox series

Patch

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)
 {