Re: [PATCH v6 12/22] core: fdtaddr: add devfdt_get_addr_size_index_ptr function
Hi Johan,
On Mon, 6 Mar 2023 at 12:55, Johan Jonker wrote:
>
>
>
> On 3/6/23 18:53, Simon Glass wrote:
> > Hi Johan,
> >
> > On Thu, 2 Mar 2023 at 17:15, Johan Jonker wrote:
> >>
> >> Add devfdt_get_addr_size_index_ptr function with the same
> >> functionality as devfdt_get_addr_size_index, but instead
> >> a return pointer is given.
> >>
> >> Suggested-by: Michael Nazzareno Trimarchi
> >> Signed-off-by: Johan Jonker
> >> Reviewed-by: Michael Trimarchi
> >> ---
> >>
> >> Changed V5:
> >> fix spelling
> >> use tabs
> >> ---
> >> drivers/core/fdtaddr.c | 8
> >> include/dm/fdtaddr.h | 17 -
> >> 2 files changed, 24 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/core/fdtaddr.c b/drivers/core/fdtaddr.c
> >> index 91bcd1a2..f5906ff9 100644
> >> --- a/drivers/core/fdtaddr.c
> >> +++ b/drivers/core/fdtaddr.c
> >> @@ -122,6 +122,14 @@ fdt_addr_t devfdt_get_addr_size_index(const struct
> >> udevice *dev, int index,
> >> #endif
> >> }
> >>
> >> +void *devfdt_get_addr_size_index_ptr(const struct udevice *dev, int index,
> >> +fdt_size_t *size)
> >> +{
> >> + fdt_addr_t addr = devfdt_get_addr_size_index(dev, index, size);
> >> +
> >> + return (addr == FDT_ADDR_T_NONE) ? NULL : (void *)(uintptr_t)addr;
> >
> Just wondering, as a side question:
> Why is Uboot maintaining/exporting 2 sets of functions that do the do the
> more or less the same thing.
>
> For example:
> devfdt_get_addr_size_index_ptr vs. dev_read_addr_size_index_ptr
>
> Or should we standardize and replace all by dev_read_addr_size_index_ptr if
> possible?
Yes that is a bit wonky. The flat/live tree selection is supposed to
be in ofnode.c, so we could change it to call the ofnode version in
both cases, moving the logic into there. That is how other functions
work.
Regards,
Simon
Re: [PATCH v6 12/22] core: fdtaddr: add devfdt_get_addr_size_index_ptr function
On 3/6/23 18:53, Simon Glass wrote:
> Hi Johan,
>
> On Thu, 2 Mar 2023 at 17:15, Johan Jonker wrote:
>>
>> Add devfdt_get_addr_size_index_ptr function with the same
>> functionality as devfdt_get_addr_size_index, but instead
>> a return pointer is given.
>>
>> Suggested-by: Michael Nazzareno Trimarchi
>> Signed-off-by: Johan Jonker
>> Reviewed-by: Michael Trimarchi
>> ---
>>
>> Changed V5:
>> fix spelling
>> use tabs
>> ---
>> drivers/core/fdtaddr.c | 8
>> include/dm/fdtaddr.h | 17 -
>> 2 files changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/core/fdtaddr.c b/drivers/core/fdtaddr.c
>> index 91bcd1a2..f5906ff9 100644
>> --- a/drivers/core/fdtaddr.c
>> +++ b/drivers/core/fdtaddr.c
>> @@ -122,6 +122,14 @@ fdt_addr_t devfdt_get_addr_size_index(const struct
>> udevice *dev, int index,
>> #endif
>> }
>>
>> +void *devfdt_get_addr_size_index_ptr(const struct udevice *dev, int index,
>> +fdt_size_t *size)
>> +{
>> + fdt_addr_t addr = devfdt_get_addr_size_index(dev, index, size);
>> +
>> + return (addr == FDT_ADDR_T_NONE) ? NULL : (void *)(uintptr_t)addr;
>
Just wondering, as a side question:
Why is Uboot maintaining/exporting 2 sets of functions that do the do the more
or less the same thing.
For example:
devfdt_get_addr_size_index_ptr vs. dev_read_addr_size_index_ptr
Or should we standardize and replace all by dev_read_addr_size_index_ptr if
possible?
Johan
>
> [..]
>
> Regards,
> SImon
Re: [PATCH v6 12/22] core: fdtaddr: add devfdt_get_addr_size_index_ptr function
Hi Johan,
On Thu, 2 Mar 2023 at 17:15, Johan Jonker wrote:
>
> Add devfdt_get_addr_size_index_ptr function with the same
> functionality as devfdt_get_addr_size_index, but instead
> a return pointer is given.
>
> Suggested-by: Michael Nazzareno Trimarchi
> Signed-off-by: Johan Jonker
> Reviewed-by: Michael Trimarchi
> ---
>
> Changed V5:
> fix spelling
> use tabs
> ---
> drivers/core/fdtaddr.c | 8
> include/dm/fdtaddr.h | 17 -
> 2 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/core/fdtaddr.c b/drivers/core/fdtaddr.c
> index 91bcd1a2..f5906ff9 100644
> --- a/drivers/core/fdtaddr.c
> +++ b/drivers/core/fdtaddr.c
> @@ -122,6 +122,14 @@ fdt_addr_t devfdt_get_addr_size_index(const struct
> udevice *dev, int index,
> #endif
> }
>
> +void *devfdt_get_addr_size_index_ptr(const struct udevice *dev, int index,
> +fdt_size_t *size)
> +{
> + fdt_addr_t addr = devfdt_get_addr_size_index(dev, index, size);
> +
> + return (addr == FDT_ADDR_T_NONE) ? NULL : (void *)(uintptr_t)addr;
Shouldn't this use map_to_sysmem()? We should not cast addresses to pointers.
[..]
Regards,
SImon
[PATCH v6 12/22] core: fdtaddr: add devfdt_get_addr_size_index_ptr function
Add devfdt_get_addr_size_index_ptr function with the same
functionality as devfdt_get_addr_size_index, but instead
a return pointer is given.
Suggested-by: Michael Nazzareno Trimarchi
Signed-off-by: Johan Jonker
Reviewed-by: Michael Trimarchi
---
Changed V5:
fix spelling
use tabs
---
drivers/core/fdtaddr.c | 8
include/dm/fdtaddr.h | 17 -
2 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/drivers/core/fdtaddr.c b/drivers/core/fdtaddr.c
index 91bcd1a2..f5906ff9 100644
--- a/drivers/core/fdtaddr.c
+++ b/drivers/core/fdtaddr.c
@@ -122,6 +122,14 @@ fdt_addr_t devfdt_get_addr_size_index(const struct udevice
*dev, int index,
#endif
}
+void *devfdt_get_addr_size_index_ptr(const struct udevice *dev, int index,
+fdt_size_t *size)
+{
+ fdt_addr_t addr = devfdt_get_addr_size_index(dev, index, size);
+
+ return (addr == FDT_ADDR_T_NONE) ? NULL : (void *)(uintptr_t)addr;
+}
+
fdt_addr_t devfdt_get_addr_name(const struct udevice *dev, const char *name)
{
#if CONFIG_IS_ENABLED(OF_CONTROL)
diff --git a/include/dm/fdtaddr.h b/include/dm/fdtaddr.h
index c9d2b27b..dcdc1913 100644
--- a/include/dm/fdtaddr.h
+++ b/include/dm/fdtaddr.h
@@ -111,7 +111,7 @@ void *devfdt_get_addr_index_ptr(const struct udevice *dev,
int index);
* @dev: Pointer to a device
* @index: the 'reg' property can hold a list of pairs
*and @index is used to select which one is required
- * @size: Pointer to size varible - this function returns the size
+ * @size: Pointer to size variable - this function returns the size
*specified in the 'reg' property here
*
* Return: addr
@@ -119,6 +119,21 @@ void *devfdt_get_addr_index_ptr(const struct udevice *dev,
int index);
fdt_addr_t devfdt_get_addr_size_index(const struct udevice *dev, int index,
fdt_size_t *size);
+/**
+ * devfdt_get_addr_size_index_ptr() - Return indexed pointer to the address of
the
+ *reg property of a device
+ *
+ * @dev: Pointer to a device
+ * @index: the 'reg' property can hold a list of pairs
+ *and @index is used to select which one is required
+ * @size: Pointer to size variable - this function returns the size
+ *specified in the 'reg' property here
+ *
+ * Return: Pointer to addr, or NULL if there is no such property
+ */
+void *devfdt_get_addr_size_index_ptr(const struct udevice *dev, int index,
+fdt_size_t *size);
+
/**
* devfdt_get_addr_name() - Get the reg property of a device, indexed by name
*
--
2.20.1
