Re: [U-Boot] [PATCH v3 03/13] fdtdec: Add fdt_{addr, size}_unpack() helpers
On Fri, Apr 12, 2019 at 03:45:53PM -0600, Simon Glass wrote:
> Hi Thierry,
>
> On Fri, 22 Mar 2019 at 02:31, Thierry Reding wrote:
> >
> > On Fri, Mar 22, 2019 at 03:53:00PM +0800, Simon Glass wrote:
> > > Hi Thierry,
> > >
> > > On Fri, 22 Mar 2019 at 02:10, Thierry Reding
> > > wrote:
> > > >
> > > > From: Thierry Reding
> > > >
> > > > These helpers can be used to unpack variables of type fdt_addr_t and
> > > > fdt_size_t into a pair of 32-bit variables. This is useful in cases
> > > > where such variables need to be written to properties (such as "reg")
> > > > of a device tree node where they need to be split into cells.
> > > >
> > > > Signed-off-by: Thierry Reding
> > > > ---
> > > > Changes in v2:
> > > > - new patch
> > > >
> > > > include/fdtdec.h | 25 +
> > > > 1 file changed, 25 insertions(+)
> > > >
> > > > diff --git a/include/fdtdec.h b/include/fdtdec.h
> > > > index a965c33157c9..a0ba57c6318b 100644
> > > > --- a/include/fdtdec.h
> > > > +++ b/include/fdtdec.h
> > > > @@ -23,6 +23,31 @@
> > > > */
> > > > typedef phys_addr_t fdt_addr_t;
> > > > typedef phys_size_t fdt_size_t;
> > > > +
> > > > +static inline fdt32_t fdt_addr_unpack(fdt_addr_t addr, fdt32_t *upper)
> > > > +{
> > > > + if (upper)
> > > > +#ifdef CONFIG_PHYS_64BIT
> > >
> > > Could we use 'if IS_ENABLED()' instead?
> >
> > Are you suggesting to use IS_ENABLED() in a preprocessor #if or a
> > compiler if (IS_ENABLED(...)) { ... } construct? For the former, yes we
> > could certainly do that.
> >
> > The latter would probably work as well if we did something like this:
> >
> > > > + *upper = addr >> 32;
> >
> > *upper = upper_32_bits(addr);
> >
> > where upper_32_bits() is a macro such as the one defined in Linux'
> > include/linux/kernel.h. That prevents the right-shift of 32 bits for
> > 32-bit quantities to not produce a warning.
> >
> > That said, I see that we already have that macro in U-Boot's kernel.h
> > header file, so I could switch to using that. With that I think we could
> > even leave out the conditional. The compiler would hopefully optimize it
> > (the upper_32_bits() invocation) out by itself.
> >
> > I'll do some testing and respin if that works out.
>
> Applied to u-boot-dm, thanks!
>
> Please do a fixup patch if this works out. I hope I didn't miss an
> email/patch.
Sorry, I had been meaning to send out a v4 of the series, but it took me
too long. I just sent out a couple of follow-up patches for this:
http://patchwork.ozlabs.org/project/uboot/list/?series=102732
Thierry
signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v3 03/13] fdtdec: Add fdt_{addr, size}_unpack() helpers
Hi Thierry,
On Fri, 22 Mar 2019 at 02:31, Thierry Reding wrote:
>
> On Fri, Mar 22, 2019 at 03:53:00PM +0800, Simon Glass wrote:
> > Hi Thierry,
> >
> > On Fri, 22 Mar 2019 at 02:10, Thierry Reding
> > wrote:
> > >
> > > From: Thierry Reding
> > >
> > > These helpers can be used to unpack variables of type fdt_addr_t and
> > > fdt_size_t into a pair of 32-bit variables. This is useful in cases
> > > where such variables need to be written to properties (such as "reg")
> > > of a device tree node where they need to be split into cells.
> > >
> > > Signed-off-by: Thierry Reding
> > > ---
> > > Changes in v2:
> > > - new patch
> > >
> > > include/fdtdec.h | 25 +
> > > 1 file changed, 25 insertions(+)
> > >
> > > diff --git a/include/fdtdec.h b/include/fdtdec.h
> > > index a965c33157c9..a0ba57c6318b 100644
> > > --- a/include/fdtdec.h
> > > +++ b/include/fdtdec.h
> > > @@ -23,6 +23,31 @@
> > > */
> > > typedef phys_addr_t fdt_addr_t;
> > > typedef phys_size_t fdt_size_t;
> > > +
> > > +static inline fdt32_t fdt_addr_unpack(fdt_addr_t addr, fdt32_t *upper)
> > > +{
> > > + if (upper)
> > > +#ifdef CONFIG_PHYS_64BIT
> >
> > Could we use 'if IS_ENABLED()' instead?
>
> Are you suggesting to use IS_ENABLED() in a preprocessor #if or a
> compiler if (IS_ENABLED(...)) { ... } construct? For the former, yes we
> could certainly do that.
>
> The latter would probably work as well if we did something like this:
>
> > > + *upper = addr >> 32;
>
> *upper = upper_32_bits(addr);
>
> where upper_32_bits() is a macro such as the one defined in Linux'
> include/linux/kernel.h. That prevents the right-shift of 32 bits for
> 32-bit quantities to not produce a warning.
>
> That said, I see that we already have that macro in U-Boot's kernel.h
> header file, so I could switch to using that. With that I think we could
> even leave out the conditional. The compiler would hopefully optimize it
> (the upper_32_bits() invocation) out by itself.
>
> I'll do some testing and respin if that works out.
Applied to u-boot-dm, thanks!
Please do a fixup patch if this works out. I hope I didn't miss an email/patch.
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v3 03/13] fdtdec: Add fdt_{addr, size}_unpack() helpers
On Fri, Mar 22, 2019 at 03:53:00PM +0800, Simon Glass wrote:
> Hi Thierry,
>
> On Fri, 22 Mar 2019 at 02:10, Thierry Reding wrote:
> >
> > From: Thierry Reding
> >
> > These helpers can be used to unpack variables of type fdt_addr_t and
> > fdt_size_t into a pair of 32-bit variables. This is useful in cases
> > where such variables need to be written to properties (such as "reg")
> > of a device tree node where they need to be split into cells.
> >
> > Signed-off-by: Thierry Reding
> > ---
> > Changes in v2:
> > - new patch
> >
> > include/fdtdec.h | 25 +
> > 1 file changed, 25 insertions(+)
> >
> > diff --git a/include/fdtdec.h b/include/fdtdec.h
> > index a965c33157c9..a0ba57c6318b 100644
> > --- a/include/fdtdec.h
> > +++ b/include/fdtdec.h
> > @@ -23,6 +23,31 @@
> > */
> > typedef phys_addr_t fdt_addr_t;
> > typedef phys_size_t fdt_size_t;
> > +
> > +static inline fdt32_t fdt_addr_unpack(fdt_addr_t addr, fdt32_t *upper)
> > +{
> > + if (upper)
> > +#ifdef CONFIG_PHYS_64BIT
>
> Could we use 'if IS_ENABLED()' instead?
Are you suggesting to use IS_ENABLED() in a preprocessor #if or a
compiler if (IS_ENABLED(...)) { ... } construct? For the former, yes we
could certainly do that.
The latter would probably work as well if we did something like this:
> > + *upper = addr >> 32;
*upper = upper_32_bits(addr);
where upper_32_bits() is a macro such as the one defined in Linux'
include/linux/kernel.h. That prevents the right-shift of 32 bits for
32-bit quantities to not produce a warning.
That said, I see that we already have that macro in U-Boot's kernel.h
header file, so I could switch to using that. With that I think we could
even leave out the conditional. The compiler would hopefully optimize it
(the upper_32_bits() invocation) out by itself.
I'll do some testing and respin if that works out.
Thierry
> > +#else
> > + *upper = 0;
> > +#endif
> > +
> > + return addr;
> > +}
> > +
> > +static inline fdt32_t fdt_size_unpack(fdt_size_t size, fdt32_t *upper)
> > +{
> > + if (upper)
> > +#ifdef CONFIG_PHYS_64BIT
> > + *upper = size >> 32;
> > +#else
> > + *upper = 0;
> > +#endif
> > +
> > + return size;
> > +}
> > +
> > #ifdef CONFIG_PHYS_64BIT
> > #define FDT_ADDR_T_NONE (-1U)
> > #define fdt_addr_to_cpu(reg) be64_to_cpu(reg)
> > --
> > 2.21.0
> >
>
> Regards,
> Simon
signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v3 03/13] fdtdec: Add fdt_{addr, size}_unpack() helpers
Hi Thierry,
On Fri, 22 Mar 2019 at 02:10, Thierry Reding wrote:
>
> From: Thierry Reding
>
> These helpers can be used to unpack variables of type fdt_addr_t and
> fdt_size_t into a pair of 32-bit variables. This is useful in cases
> where such variables need to be written to properties (such as "reg")
> of a device tree node where they need to be split into cells.
>
> Signed-off-by: Thierry Reding
> ---
> Changes in v2:
> - new patch
>
> include/fdtdec.h | 25 +
> 1 file changed, 25 insertions(+)
>
> diff --git a/include/fdtdec.h b/include/fdtdec.h
> index a965c33157c9..a0ba57c6318b 100644
> --- a/include/fdtdec.h
> +++ b/include/fdtdec.h
> @@ -23,6 +23,31 @@
> */
> typedef phys_addr_t fdt_addr_t;
> typedef phys_size_t fdt_size_t;
> +
> +static inline fdt32_t fdt_addr_unpack(fdt_addr_t addr, fdt32_t *upper)
> +{
> + if (upper)
> +#ifdef CONFIG_PHYS_64BIT
Could we use 'if IS_ENABLED()' instead?
> + *upper = addr >> 32;
> +#else
> + *upper = 0;
> +#endif
> +
> + return addr;
> +}
> +
> +static inline fdt32_t fdt_size_unpack(fdt_size_t size, fdt32_t *upper)
> +{
> + if (upper)
> +#ifdef CONFIG_PHYS_64BIT
> + *upper = size >> 32;
> +#else
> + *upper = 0;
> +#endif
> +
> + return size;
> +}
> +
> #ifdef CONFIG_PHYS_64BIT
> #define FDT_ADDR_T_NONE (-1U)
> #define fdt_addr_to_cpu(reg) be64_to_cpu(reg)
> --
> 2.21.0
>
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH v3 03/13] fdtdec: Add fdt_{addr, size}_unpack() helpers
From: Thierry Reding
These helpers can be used to unpack variables of type fdt_addr_t and
fdt_size_t into a pair of 32-bit variables. This is useful in cases
where such variables need to be written to properties (such as "reg")
of a device tree node where they need to be split into cells.
Signed-off-by: Thierry Reding
---
Changes in v2:
- new patch
include/fdtdec.h | 25 +
1 file changed, 25 insertions(+)
diff --git a/include/fdtdec.h b/include/fdtdec.h
index a965c33157c9..a0ba57c6318b 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -23,6 +23,31 @@
*/
typedef phys_addr_t fdt_addr_t;
typedef phys_size_t fdt_size_t;
+
+static inline fdt32_t fdt_addr_unpack(fdt_addr_t addr, fdt32_t *upper)
+{
+ if (upper)
+#ifdef CONFIG_PHYS_64BIT
+ *upper = addr >> 32;
+#else
+ *upper = 0;
+#endif
+
+ return addr;
+}
+
+static inline fdt32_t fdt_size_unpack(fdt_size_t size, fdt32_t *upper)
+{
+ if (upper)
+#ifdef CONFIG_PHYS_64BIT
+ *upper = size >> 32;
+#else
+ *upper = 0;
+#endif
+
+ return size;
+}
+
#ifdef CONFIG_PHYS_64BIT
#define FDT_ADDR_T_NONE (-1U)
#define fdt_addr_to_cpu(reg) be64_to_cpu(reg)
--
2.21.0
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
