| Message ID | 20210508161254.2217178-1-seanga2@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Andes |
| Headers | show |
| Series | riscv: Fix arch_fixup_fdt always failing without /chosen | expand |
On Sun, May 9, 2021 at 12:13 AM Sean Anderson <seanga2@gmail.com> wrote: > > If /chosen was missing, chosen_offset would never get updated with the new > /chosen node. This would cause fdt_setprop_u32 to fail. This patch fixes > this by setting chosen_offset. In addition, log any errors from setting > boot-hartid as well. > > Fixes: 177c53fe6c6 ("riscv: Move all fdt fixups together") I think the correct commit id to fix is: commit 5370478d1c7afb8b2a8a0a00b31ff97c3a2c0451 ("riscv: Add boot hartid to device tree") > Signed-off-by: Sean Anderson <seanga2@gmail.com> > --- > I have not actually tested this (nor observed the original failure). But this > seemed buggy from inspection. > > arch/riscv/lib/fdt_fixup.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c > index 1f3f23621c..f636b28449 100644 > --- a/arch/riscv/lib/fdt_fixup.c > +++ b/arch/riscv/lib/fdt_fixup.c > @@ -151,14 +151,17 @@ int arch_fixup_fdt(void *blob) > } > chosen_offset = fdt_path_offset(blob, "/chosen"); > if (chosen_offset < 0) { > - err = fdt_add_subnode(blob, 0, "chosen"); > - if (err < 0) { > + chosen_offset = fdt_add_subnode(blob, 0, "chosen"); > + if (chosen_offset < 0) { > log_err("chosen node cannot be added\n"); > - return err; > + return chosen_offset; > } > } > /* Overwrite the boot-hartid as U-Boot is the last stage BL */ > - fdt_setprop_u32(blob, chosen_offset, "boot-hartid", gd->arch.boot_hart); > + err = fdt_setprop_u32(blob, chosen_offset, "boot-hartid", > + gd->arch.boot_hart); > + if (err < 0) > + return log_msg_ret("could not set boot-hartid", err); > #endif > > /* Copy the reserved-memory node to the DT used by OS */ > -- Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
On 5/8/21 12:22 PM, Bin Meng wrote: > On Sun, May 9, 2021 at 12:13 AM Sean Anderson <seanga2@gmail.com> wrote: >> >> If /chosen was missing, chosen_offset would never get updated with the new >> /chosen node. This would cause fdt_setprop_u32 to fail. This patch fixes >> this by setting chosen_offset. In addition, log any errors from setting >> boot-hartid as well. >> >> Fixes: 177c53fe6c6 ("riscv: Move all fdt fixups together") > > I think the correct commit id to fix is: > commit 5370478d1c7afb8b2a8a0a00b31ff97c3a2c0451 ("riscv: Add boot > hartid to device tree") Ah, so it is. --Sean > >> Signed-off-by: Sean Anderson <seanga2@gmail.com> >> --- >> I have not actually tested this (nor observed the original failure). But this >> seemed buggy from inspection. >> >> arch/riscv/lib/fdt_fixup.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c >> index 1f3f23621c..f636b28449 100644 >> --- a/arch/riscv/lib/fdt_fixup.c >> +++ b/arch/riscv/lib/fdt_fixup.c >> @@ -151,14 +151,17 @@ int arch_fixup_fdt(void *blob) >> } >> chosen_offset = fdt_path_offset(blob, "/chosen"); >> if (chosen_offset < 0) { >> - err = fdt_add_subnode(blob, 0, "chosen"); >> - if (err < 0) { >> + chosen_offset = fdt_add_subnode(blob, 0, "chosen"); >> + if (chosen_offset < 0) { >> log_err("chosen node cannot be added\n"); >> - return err; >> + return chosen_offset; >> } >> } >> /* Overwrite the boot-hartid as U-Boot is the last stage BL */ >> - fdt_setprop_u32(blob, chosen_offset, "boot-hartid", gd->arch.boot_hart); >> + err = fdt_setprop_u32(blob, chosen_offset, "boot-hartid", >> + gd->arch.boot_hart); >> + if (err < 0) >> + return log_msg_ret("could not set boot-hartid", err); >> #endif >> >> /* Copy the reserved-memory node to the DT used by OS */ >> -- > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> >
> If /chosen was missing, chosen_offset would never get updated with the new > /chosen node. This would cause fdt_setprop_u32 to fail. This patch fixes > this by setting chosen_offset. In addition, log any errors from setting > boot-hartid as well. > > Fixes: 177c53fe6c6 ("riscv: Move all fdt fixups together") > Signed-off-by: Sean Anderson <seanga2@gmail.com> > --- > I have not actually tested this (nor observed the original failure). But this > seemed buggy from inspection. > > arch/riscv/lib/fdt_fixup.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) Good catch ! Reviewed-by: Rick Chen <rick@andestech.com>
Hi Sean, On Sun, May 9, 2021 at 12:24 AM Sean Anderson <seanga2@gmail.com> wrote: > > On 5/8/21 12:22 PM, Bin Meng wrote: > > On Sun, May 9, 2021 at 12:13 AM Sean Anderson <seanga2@gmail.com> wrote: > >> > >> If /chosen was missing, chosen_offset would never get updated with the new > >> /chosen node. This would cause fdt_setprop_u32 to fail. This patch fixes > >> this by setting chosen_offset. In addition, log any errors from setting > >> boot-hartid as well. > >> > >> Fixes: 177c53fe6c6 ("riscv: Move all fdt fixups together") > > > > I think the correct commit id to fix is: > > commit 5370478d1c7afb8b2a8a0a00b31ff97c3a2c0451 ("riscv: Add boot > > hartid to device tree") > > Ah, so it is. > Would you send v2 with the correct commit id? Regards, Bin
On Sat, May 8, 2021 at 9:13 AM Sean Anderson <seanga2@gmail.com> wrote: > > If /chosen was missing, chosen_offset would never get updated with the new > /chosen node. This would cause fdt_setprop_u32 to fail. This patch fixes > this by setting chosen_offset. In addition, log any errors from setting > boot-hartid as well. > > Fixes: 177c53fe6c6 ("riscv: Move all fdt fixups together") > Signed-off-by: Sean Anderson <seanga2@gmail.com> > --- > I have not actually tested this (nor observed the original failure). But this > seemed buggy from inspection. > > arch/riscv/lib/fdt_fixup.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c > index 1f3f23621c..f636b28449 100644 > --- a/arch/riscv/lib/fdt_fixup.c > +++ b/arch/riscv/lib/fdt_fixup.c > @@ -151,14 +151,17 @@ int arch_fixup_fdt(void *blob) > } > chosen_offset = fdt_path_offset(blob, "/chosen"); > if (chosen_offset < 0) { > - err = fdt_add_subnode(blob, 0, "chosen"); > - if (err < 0) { > + chosen_offset = fdt_add_subnode(blob, 0, "chosen"); > + if (chosen_offset < 0) { > log_err("chosen node cannot be added\n"); > - return err; > + return chosen_offset; > } > } > /* Overwrite the boot-hartid as U-Boot is the last stage BL */ > - fdt_setprop_u32(blob, chosen_offset, "boot-hartid", gd->arch.boot_hart); > + err = fdt_setprop_u32(blob, chosen_offset, "boot-hartid", > + gd->arch.boot_hart); > + if (err < 0) > + return log_msg_ret("could not set boot-hartid", err); > #endif > > /* Copy the reserved-memory node to the DT used by OS */ > -- > 2.31.0 > Thanks for the fix. Good catch. With update commit text as suggested by Bin: Reviewed-by: Atish Patra <atish.patra@wdc.com>
diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c index 1f3f23621c..f636b28449 100644 --- a/arch/riscv/lib/fdt_fixup.c +++ b/arch/riscv/lib/fdt_fixup.c @@ -151,14 +151,17 @@ int arch_fixup_fdt(void *blob) } chosen_offset = fdt_path_offset(blob, "/chosen"); if (chosen_offset < 0) { - err = fdt_add_subnode(blob, 0, "chosen"); - if (err < 0) { + chosen_offset = fdt_add_subnode(blob, 0, "chosen"); + if (chosen_offset < 0) { log_err("chosen node cannot be added\n"); - return err; + return chosen_offset; } } /* Overwrite the boot-hartid as U-Boot is the last stage BL */ - fdt_setprop_u32(blob, chosen_offset, "boot-hartid", gd->arch.boot_hart); + err = fdt_setprop_u32(blob, chosen_offset, "boot-hartid", + gd->arch.boot_hart); + if (err < 0) + return log_msg_ret("could not set boot-hartid", err); #endif /* Copy the reserved-memory node to the DT used by OS */
If /chosen was missing, chosen_offset would never get updated with the new /chosen node. This would cause fdt_setprop_u32 to fail. This patch fixes this by setting chosen_offset. In addition, log any errors from setting boot-hartid as well. Fixes: 177c53fe6c6 ("riscv: Move all fdt fixups together") Signed-off-by: Sean Anderson <seanga2@gmail.com> --- I have not actually tested this (nor observed the original failure). But this seemed buggy from inspection. arch/riscv/lib/fdt_fixup.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)