[PATCH v2 1/3] riscv: Avoid the reserved memory fixup if src and dst point to the same place

[PATCH v2 1/3] riscv: Avoid the reserved memory fixup if src and dst point to the same place

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

The copy of reserved memory node from source dtb to destination dtb
can be avoided if they point to the same place. This is useful when
OF_PRIOR_STAGE is used.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Rick Chen <rick@andestech.com>
---

 arch/riscv/lib/fdt_fixup.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c
index 6db48ad..5f523f0 100644
--- a/arch/riscv/lib/fdt_fixup.c
+++ b/arch/riscv/lib/fdt_fixup.c
@@ -82,10 +82,9 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst)
  * @fdt: Pointer to the device tree in which reserved memory node needs to be
  *	 added.
  *
- * In RISC-V, any board compiled with OF_SEPARATE needs to copy the reserved
- * memory node from the device tree provided by the firmware to the device tree
- * used by U-Boot. This is a common function that individual board fixup
- * functions can invoke.
+ * In RISC-V, any board needs to copy the reserved memory node from the device
+ * tree provided by the firmware to the device tree used by U-Boot. This is a
+ * common function that individual board fixup functions can invoke.
  *
  * Return: 0 on success or error otherwise.
  */
@@ -95,6 +94,11 @@ int riscv_board_reserved_mem_fixup(void *fdt)
 	void *src_fdt_addr;
 
 	src_fdt_addr = map_sysmem(gd->arch.firmware_fdt_addr, 0);
+
+	/* avoid the copy if we are using the same device tree */
+	if (src_fdt_addr == fdt)
+		return 0;
+
 	err = riscv_fdt_copy_resv_mem_node(src_fdt_addr, fdt);
 	if (err < 0)
 		return err;
-- 
2.7.4

[PATCH v2 2/3] riscv: Expand the DT size before copy reserved memory node

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

The FDT blob might not have sufficient space to hold a copy of
reserved memory node. Expand it before the copy.

Reported-by: Rick Chen <rick@andestech.com>
Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

 arch/riscv/lib/fdt_fixup.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c
index 5f523f0..1290a64 100644
--- a/arch/riscv/lib/fdt_fixup.c
+++ b/arch/riscv/lib/fdt_fixup.c
@@ -41,6 +41,12 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst)
 		return 0;
 	}
 
+	err = fdt_open_into(dst, dst, fdt_totalsize(dst) + 32);
+	if (err < 0) {
+		printf("Device Tree can't be expanded to accommodate new node");
+		return err;
+	}
+
 	fdt_for_each_subnode(node, src, offset) {
 		name = fdt_get_name(src, node, NULL);
 
-- 
2.7.4

[PATCH v2 3/3] riscv: Enable CONFIG_OF_BOARD_FIXUP by default

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

Starting from OpenSBI v0.7, the SBI firmware inserts/fixes up the
reserved memory node for PMP protected memory regions. All RISC-V
boards needs to copy the reserved memory node from the device tree
provided by the firmware to the device tree used by U-Boot.

Turn on CONFIG_OF_BOARD_FIXUP by default.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

 arch/riscv/Kconfig             | 3 +++
 configs/sifive_fu540_defconfig | 1 -
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index fb5fe5a..5176b35 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -272,4 +272,7 @@ config STACK_SIZE_SHIFT
 	int
 	default 14
 
+config OF_BOARD_FIXUP
+	default y
+
 endmenu
diff --git a/configs/sifive_fu540_defconfig b/configs/sifive_fu540_defconfig
index f805aac..6d61e6c 100644
--- a/configs/sifive_fu540_defconfig
+++ b/configs/sifive_fu540_defconfig
@@ -9,7 +9,6 @@ CONFIG_FIT=y
 CONFIG_MISC_INIT_R=y
 CONFIG_DISPLAY_CPUINFO=y
 CONFIG_DISPLAY_BOARDINFO=y
-CONFIG_OF_BOARD_FIXUP=y
 CONFIG_DEFAULT_DEVICE_TREE="hifive-unleashed-a00"
 CONFIG_SYS_RELOC_GD_ENV_ADDR=y
 CONFIG_DM_MTD=y
-- 
2.7.4

[PATCH v2 2/3] riscv: Expand the DT size before copy reserved memory node

[Atish Patra]

On Thu, May 28, 2020 at 1:47 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> From: Bin Meng <bin.meng@windriver.com>
>
> The FDT blob might not have sufficient space to hold a copy of
> reserved memory node. Expand it before the copy.
>
> Reported-by: Rick Chen <rick@andestech.com>
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
>
>  arch/riscv/lib/fdt_fixup.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c
> index 5f523f0..1290a64 100644
> --- a/arch/riscv/lib/fdt_fixup.c
> +++ b/arch/riscv/lib/fdt_fixup.c
> @@ -41,6 +41,12 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst)
>                 return 0;
>         }
>
> +       err = fdt_open_into(dst, dst, fdt_totalsize(dst) + 32);

The size may be bigger than 32 bytes in future given that we may have
multiple reserved pmp regions.
How about calculating the size from the source and using that instead
of a fixed size ?


> +       if (err < 0) {
> +               printf("Device Tree can't be expanded to accommodate new node");
> +               return err;
> +       }
> +
>         fdt_for_each_subnode(node, src, offset) {
>                 name = fdt_get_name(src, node, NULL);
>
> --
> 2.7.4
>


--
Regards,
Atish

[PATCH v2 2/3] riscv: Expand the DT size before copy reserved memory node

[Bin Meng]

Hi Atish,

On Wed, Jun 3, 2020 at 2:23 AM Atish Patra <atishp@atishpatra.org> wrote:
>
> On Thu, May 28, 2020 at 1:47 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > The FDT blob might not have sufficient space to hold a copy of
> > reserved memory node. Expand it before the copy.
> >
> > Reported-by: Rick Chen <rick@andestech.com>
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > ---
> >
> >  arch/riscv/lib/fdt_fixup.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c
> > index 5f523f0..1290a64 100644
> > --- a/arch/riscv/lib/fdt_fixup.c
> > +++ b/arch/riscv/lib/fdt_fixup.c
> > @@ -41,6 +41,12 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst)
> >                 return 0;
> >         }
> >
> > +       err = fdt_open_into(dst, dst, fdt_totalsize(dst) + 32);
>
> The size may be bigger than 32 bytes in future given that we may have
> multiple reserved pmp regions.
> How about calculating the size from the source and using that instead
> of a fixed size ?
>

This looks way too complicated. That means the codes here need to
understand the FDT blob binary representation to know exactly how much
additional size is needed. By looking at existing calls to
fdt_open_into() none of these do such things.

I guess we will have to give an estimated size to fit all possible
situations. Say there are 16 PMP regions, and one occupies 64 bytes,
so we can extend the FDT by 64 * 16 = 1024 bytes. Thoughts ??

>
> > +       if (err < 0) {
> > +               printf("Device Tree can't be expanded to accommodate new node");
> > +               return err;
> > +       }
> > +
> >         fdt_for_each_subnode(node, src, offset) {
> >                 name = fdt_get_name(src, node, NULL);
> >

Regards,
Bin

[PATCH v2 2/3] riscv: Expand the DT size before copy reserved memory node

[Atish Patra]

On Wed, Jun 10, 2020 at 2:05 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Atish,
>
> On Wed, Jun 3, 2020 at 2:23 AM Atish Patra <atishp@atishpatra.org> wrote:
> >
> > On Thu, May 28, 2020 at 1:47 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > From: Bin Meng <bin.meng@windriver.com>
> > >
> > > The FDT blob might not have sufficient space to hold a copy of
> > > reserved memory node. Expand it before the copy.
> > >
> > > Reported-by: Rick Chen <rick@andestech.com>
> > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > > ---
> > >
> > >  arch/riscv/lib/fdt_fixup.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c
> > > index 5f523f0..1290a64 100644
> > > --- a/arch/riscv/lib/fdt_fixup.c
> > > +++ b/arch/riscv/lib/fdt_fixup.c
> > > @@ -41,6 +41,12 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst)
> > >                 return 0;
> > >         }
> > >
> > > +       err = fdt_open_into(dst, dst, fdt_totalsize(dst) + 32);
> >
> > The size may be bigger than 32 bytes in future given that we may have
> > multiple reserved pmp regions.
> > How about calculating the size from the source and using that instead
> > of a fixed size ?
> >
>
> This looks way too complicated. That means the codes here need to
> understand the FDT blob binary representation to know exactly how much
> additional size is needed. By looking at existing calls to
> fdt_open_into() none of these do such things.
>
> I guess we will have to give an estimated size to fit all possible
> situations. Say there are 16 PMP regions, and one occupies 64 bytes,
> so we can extend the FDT by 64 * 16 = 1024 bytes. Thoughts ??
>

That works too. I see we only allocate 256 bytes in OpenSBI.
Can you send a patch to OpenSBI as well with the above reasoning ?

> >
> > > +       if (err < 0) {
> > > +               printf("Device Tree can't be expanded to accommodate new node");
> > > +               return err;
> > > +       }
> > > +
> > >         fdt_for_each_subnode(node, src, offset) {
> > >                 name = fdt_get_name(src, node, NULL);
> > >
>
> Regards,
> Bin



-- 
Regards,
Atish

[PATCH v2 2/3] riscv: Expand the DT size before copy reserved memory node

[Bin Meng]

On Fri, Jun 12, 2020 at 3:18 AM Atish Patra <atishp@atishpatra.org> wrote:
>
> On Wed, Jun 10, 2020 at 2:05 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Atish,
> >
> > On Wed, Jun 3, 2020 at 2:23 AM Atish Patra <atishp@atishpatra.org> wrote:
> > >
> > > On Thu, May 28, 2020 at 1:47 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > From: Bin Meng <bin.meng@windriver.com>
> > > >
> > > > The FDT blob might not have sufficient space to hold a copy of
> > > > reserved memory node. Expand it before the copy.
> > > >
> > > > Reported-by: Rick Chen <rick@andestech.com>
> > > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > > > ---
> > > >
> > > >  arch/riscv/lib/fdt_fixup.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c
> > > > index 5f523f0..1290a64 100644
> > > > --- a/arch/riscv/lib/fdt_fixup.c
> > > > +++ b/arch/riscv/lib/fdt_fixup.c
> > > > @@ -41,6 +41,12 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst)
> > > >                 return 0;
> > > >         }
> > > >
> > > > +       err = fdt_open_into(dst, dst, fdt_totalsize(dst) + 32);
> > >
> > > The size may be bigger than 32 bytes in future given that we may have
> > > multiple reserved pmp regions.
> > > How about calculating the size from the source and using that instead
> > > of a fixed size ?
> > >
> >
> > This looks way too complicated. That means the codes here need to
> > understand the FDT blob binary representation to know exactly how much
> > additional size is needed. By looking at existing calls to
> > fdt_open_into() none of these do such things.
> >
> > I guess we will have to give an estimated size to fit all possible
> > situations. Say there are 16 PMP regions, and one occupies 64 bytes,
> > so we can extend the FDT by 64 * 16 = 1024 bytes. Thoughts ??
> >
>
> That works too. I see we only allocate 256 bytes in OpenSBI.
> Can you send a patch to OpenSBI as well with the above reasoning ?
>

Sure, will do.

Regards,
Bin