[RFC PATCH v1 1/2] include: fdtdec: decouple fdt_addr_t and phys_addr_t size

[RFC PATCH v1 1/2] include: fdtdec: decouple fdt_addr_t and phys_addr_t size

[Johan Jonker]

The DT specification supports CPUs with both 32-bit and 64-bit addressing
capabilities. In U-boot the fdt_addr_t and phys_addr_t size are coupled
by a typedef. The MTD NAND drivers for 32-bit CPU's can describe partitions
with a 64-bit reg property. These partitions synced from Linux end up with
the wrong offset and sizes when only the lower 32-bit is passed.
Decouple the fdt_addr_t and phys_addr_t size as they don't necessary
match.

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---

Note for Tom Rini or others:

fdt_addr_t is referenced in 230 files and fdt_size_t in 50 files.
Most drivers mix up FDT and CPU capabilities.
Please advise how to move forward with proper DT parsing.

---

This is related to a possible future serie of bug fixes
for the Rockchip nfc driver.
---
 Kconfig          | 8 ++++++++
 include/fdtdec.h | 8 +++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/Kconfig b/Kconfig
index a75cce7e..8101f1a6 100644
--- a/Kconfig
+++ b/Kconfig
@@ -422,11 +422,19 @@ endif # EXPERT

 config PHYS_64BIT
 	bool "64bit physical address support"
+	select FDT_64BIT
 	help
 	  Say Y here to support 64bit physical memory address.
 	  This can be used not only for 64bit SoCs, but also for
 	  large physical address extension on 32bit SoCs.

+config FDT_64BIT
+	bool "64bit fdt address support"
+	help
+	  Say Y here to support 64bit fdt memory address.
+	  This can be used not only for 64bit SoCs, but also for
+	  large physical address extension on 32bit SoCs.
+
 config HAS_ROM
 	bool
 	select BINMAN
diff --git a/include/fdtdec.h b/include/fdtdec.h
index 12355afd..0adde92a 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -21,12 +21,12 @@
  * A typedef for a physical address. Note that fdt data is always big
  * endian even on a litle endian machine.
  */
-typedef phys_addr_t fdt_addr_t;
-typedef phys_size_t fdt_size_t;

 #define FDT_SIZE_T_NONE (-1U)

-#ifdef CONFIG_PHYS_64BIT
+#ifdef CONFIG_FDT_64BIT
+typedef u64 fdt_addr_t;
+typedef u64 fdt_size_t;
 #define FDT_ADDR_T_NONE ((ulong)(-1))

 #define fdt_addr_to_cpu(reg) be64_to_cpu(reg)
@@ -35,6 +35,8 @@ typedef phys_size_t fdt_size_t;
 #define cpu_to_fdt_size(reg) cpu_to_be64(reg)
 typedef fdt64_t fdt_val_t;
 #else
+typedef u32 fdt_addr_t;
+typedef u32 fdt_size_t;
 #define FDT_ADDR_T_NONE (-1U)

 #define fdt_addr_to_cpu(reg) be32_to_cpu(reg)
--
2.20.1

[RFC PATCH v1 2/2] include: dm: ofnode: fix headers

[Johan Jonker]

When fdt_addr_t and phys_addr_t are split it turns out that
the header don't match the functions, so fix the headers.

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---
 include/dm/ofnode.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h
index fa986560..287b0c35 100644
--- a/include/dm/ofnode.h
+++ b/include/dm/ofnode.h
@@ -678,8 +678,8 @@ int ofnode_read_size(ofnode node, const char *propname);
  * @size: Pointer to size of the address
  * Return: address, or FDT_ADDR_T_NONE if not present or invalid
  */
-phys_addr_t ofnode_get_addr_size_index(ofnode node, int index,
-				       fdt_size_t *size);
+fdt_addr_t ofnode_get_addr_size_index(ofnode node, int index,
+				      fdt_size_t *size);

 /**
  * ofnode_get_addr_size_index_notrans() - get an address/size from a node
@@ -695,8 +695,8 @@ phys_addr_t ofnode_get_addr_size_index(ofnode node, int index,
  * @size: Pointer to size of the address
  * Return: address, or FDT_ADDR_T_NONE if not present or invalid
  */
-phys_addr_t ofnode_get_addr_size_index_notrans(ofnode node, int index,
-					       fdt_size_t *size);
+fdt_addr_t ofnode_get_addr_size_index_notrans(ofnode node, int index,
+					      fdt_size_t *size);

 /**
  * ofnode_get_addr_index() - get an address from a node
@@ -707,7 +707,7 @@ phys_addr_t ofnode_get_addr_size_index_notrans(ofnode node, int index,
  * @index: Index of address to read (0 for first)
  * Return: address, or FDT_ADDR_T_NONE if not present or invalid
  */
-phys_addr_t ofnode_get_addr_index(ofnode node, int index);
+fdt_addr_t ofnode_get_addr_index(ofnode node, int index);

 /**
  * ofnode_get_addr() - get an address from a node
@@ -717,7 +717,7 @@ phys_addr_t ofnode_get_addr_index(ofnode node, int index);
  * @node: node to read from
  * Return: address, or FDT_ADDR_T_NONE if not present or invalid
  */
-phys_addr_t ofnode_get_addr(ofnode node);
+fdt_addr_t ofnode_get_addr(ofnode node);

 /**
  * ofnode_get_size() - get size from a node
@@ -1055,8 +1055,8 @@ const void *ofprop_get_property(const struct ofprop *prop,
  * @sizep: place to put size value (on success)
  * Return: address value, or FDT_ADDR_T_NONE on error
  */
-phys_addr_t ofnode_get_addr_size(ofnode node, const char *propname,
-				 phys_size_t *sizep);
+fdt_addr_t ofnode_get_addr_size(ofnode node, const char *propname,
+				fdt_size_t *sizep);

 /**
  * ofnode_read_u8_array_ptr() - find an 8-bit array
--
2.20.1

Re: [RFC PATCH v1 2/2] include: dm: ofnode: fix headers

[Simon Glass]

On Thu, 2 Feb 2023 at 10:59, Johan Jonker <jbx6244@gmail.com> wrote:
>
> When fdt_addr_t and phys_addr_t are split it turns out that
> the header don't match the functions, so fix the headers.
>
> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> ---
>  include/dm/ofnode.h | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>

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

Re: [RFC PATCH v1 1/2] include: fdtdec: decouple fdt_addr_t and phys_addr_t size

[Simon Glass]

Hi Johan,

On Thu, 2 Feb 2023 at 10:58, Johan Jonker <jbx6244@gmail.com> wrote:
>
> The DT specification supports CPUs with both 32-bit and 64-bit addressing
> capabilities. In U-boot the fdt_addr_t and phys_addr_t size are coupled
> by a typedef. The MTD NAND drivers for 32-bit CPU's can describe partitions
> with a 64-bit reg property. These partitions synced from Linux end up with
> the wrong offset and sizes when only the lower 32-bit is passed.
> Decouple the fdt_addr_t and phys_addr_t size as they don't necessary
> match.
>
> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> ---
>
> Note for Tom Rini or others:
>
> fdt_addr_t is referenced in 230 files and fdt_size_t in 50 files.
> Most drivers mix up FDT and CPU capabilities.
> Please advise how to move forward with proper DT parsing.
>
> ---
>
> This is related to a possible future serie of bug fixes
> for the Rockchip nfc driver.
> ---
>  Kconfig          | 8 ++++++++
>  include/fdtdec.h | 8 +++++---
>  2 files changed, 13 insertions(+), 3 deletions(-)

I wonder what the impact of this might be? So long as fdt_addr_t is at
least as big as phys_addr_t then perhaps this is OK.

But at present, changing fdt_addr_t affects fdtdec_get_addr_size().
Perhaps that doesn't matter, so long as '#address-cells' is respected.

So I'm going to go with:

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

>
> diff --git a/Kconfig b/Kconfig
> index a75cce7e..8101f1a6 100644
> --- a/Kconfig
> +++ b/Kconfig
> @@ -422,11 +422,19 @@ endif # EXPERT
>
>  config PHYS_64BIT
>         bool "64bit physical address support"
> +       select FDT_64BIT
>         help
>           Say Y here to support 64bit physical memory address.
>           This can be used not only for 64bit SoCs, but also for
>           large physical address extension on 32bit SoCs.
>
> +config FDT_64BIT
> +       bool "64bit fdt address support"
> +       help
> +         Say Y here to support 64bit fdt memory address.
> +         This can be used not only for 64bit SoCs, but also for
> +         large physical address extension on 32bit SoCs.
> +
>  config HAS_ROM
>         bool
>         select BINMAN
> diff --git a/include/fdtdec.h b/include/fdtdec.h
> index 12355afd..0adde92a 100644
> --- a/include/fdtdec.h
> +++ b/include/fdtdec.h
> @@ -21,12 +21,12 @@
>   * A typedef for a physical address. Note that fdt data is always big
>   * endian even on a litle endian machine.
>   */
> -typedef phys_addr_t fdt_addr_t;
> -typedef phys_size_t fdt_size_t;
>
>  #define FDT_SIZE_T_NONE (-1U)
>
> -#ifdef CONFIG_PHYS_64BIT
> +#ifdef CONFIG_FDT_64BIT
> +typedef u64 fdt_addr_t;
> +typedef u64 fdt_size_t;
>  #define FDT_ADDR_T_NONE ((ulong)(-1))
>
>  #define fdt_addr_to_cpu(reg) be64_to_cpu(reg)
> @@ -35,6 +35,8 @@ typedef phys_size_t fdt_size_t;
>  #define cpu_to_fdt_size(reg) cpu_to_be64(reg)
>  typedef fdt64_t fdt_val_t;
>  #else
> +typedef u32 fdt_addr_t;
> +typedef u32 fdt_size_t;
>  #define FDT_ADDR_T_NONE (-1U)

Could you add some comments about what fdt_addr/size_t are for and
what it means.

The original intent was to avoid using 64-bit values on a 32-bit machine.

>
>  #define fdt_addr_to_cpu(reg) be32_to_cpu(reg)
> --
> 2.20.1
>

Regards,
Simon