Change in ...coreboot[master]: riscv: currently riscv does not support fit payload, remove redundant...
Xiang Wang has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31477
Change subject: riscv: currently riscv does not support fit payload, remove redundant code ......................................................................
riscv: currently riscv does not support fit payload, remove redundant code
This code is not necessary and will trigger an error when I try to boot linux with bbl.If you want to add this code, it is recommended to add it with the support code of the fit payload.
Change-Id: If6929897c7f12d8acb079eeebaef512ae506ca8b Signed-off-by: Xiang Wang wxjstz@126.com --- M src/arch/riscv/boot.c 1 file changed, 1 insertion(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/31477/1
diff --git a/src/arch/riscv/boot.c b/src/arch/riscv/boot.c index 29064b1..0c8143f 100644 --- a/src/arch/riscv/boot.c +++ b/src/arch/riscv/boot.c @@ -32,14 +32,7 @@ { void (*doit)(int hart_id, void *fdt); int hart_id; - void *fdt = prog_entry_arg(prog); - - /* - * If prog_entry_arg is not set (e.g. by fit_payload), use fdt from HLS - * instead. - */ - if (fdt == NULL) - fdt = HLS()->fdt; + void *fdt = HLS()->fdt;
if (ENV_RAMSTAGE && prog_type(prog) == PROG_PAYLOAD) { run_payload(prog, fdt, RISCV_PAYLOAD_MODE_S);
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31477 )
Change subject: riscv: currently riscv does not support fit payload, remove redundant code ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31477/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31477/2//COMMIT_MSG@10 PS2, Line 10: bbl.If Please add a space after the dot/period.
Hello ron minnich, Shawn C, Jonathan Neuschäfer, build bot (Jenkins), Philipp Hug,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31477
to look at the new patch set (#3).
Change subject: riscv: currently riscv does not support fit payload, remove redundant code ......................................................................
riscv: currently riscv does not support fit payload, remove redundant code
This code is not necessary and will trigger an error when I try to boot linux with bbl. If you want to add this code, it is recommended to add it with the support code of the fit payload.
Change-Id: If6929897c7f12d8acb079eeebaef512ae506ca8b Signed-off-by: Xiang Wang wxjstz@126.com --- M src/arch/riscv/boot.c 1 file changed, 1 insertion(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/31477/3
Philipp Hug has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31477 )
Change subject: riscv: currently riscv does not support fit payload, remove redundant code ......................................................................
Patch Set 3:
(1 comment)
See my comment, this should fix all payload types.
https://review.coreboot.org/#/c/31477/3/src/arch/riscv/boot.c File src/arch/riscv/boot.c:
https://review.coreboot.org/#/c/31477/3/src/arch/riscv/boot.c@a35 PS3, Line 35: prog_entry_arg should contain the argument to the payload. So the problem is within selfboot as it puts the coreboot tables into args, whereas we need the fdt in the case of an elf payload.
I also discussed this with Ron and he also suggested to just work around it in riscv: Please do the following here:
/* * Workaround selfboot putting the coreboot table into prog_entry_arg */ if (prog_cbfs_type(payload) == CBFS_TYPE_SELF) { fdt = HLS()->fdt; break;
Hello ron minnich, Shawn C, Jonathan Neuschäfer, build bot (Jenkins), Philipp Hug,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31477
to look at the new patch set (#5).
Change subject: riscv: workaround selfboot putting the coreboot table into prog_entry_arg ......................................................................
riscv: workaround selfboot putting the coreboot table into prog_entry_arg
Change-Id: If6929897c7f12d8acb079eeebaef512ae506ca8b Signed-off-by: Xiang Wang wxjstz@126.com --- M src/arch/riscv/boot.c 1 file changed, 6 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/31477/5
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31477 )
Change subject: riscv: workaround selfboot putting the coreboot table into prog_entry_arg ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/#/c/31477/5/src/arch/riscv/boot.c File src/arch/riscv/boot.c:
https://review.coreboot.org/#/c/31477/5/src/arch/riscv/boot.c@45 PS5, Line 45: * Workaround selfboot putting the coreboot table into prog_entry_arg code indent should use tabs where possible
https://review.coreboot.org/#/c/31477/5/src/arch/riscv/boot.c@46 PS5, Line 46: */ code indent should use tabs where possible
Xiang Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31477 )
Change subject: riscv: workaround selfboot putting the coreboot table into prog_entry_arg ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/31477/3/src/arch/riscv/boot.c File src/arch/riscv/boot.c:
https://review.coreboot.org/#/c/31477/3/src/arch/riscv/boot.c@a35 PS3, Line 35:
prog_entry_arg should contain the argument to the payload. […]
Done
Hello ron minnich, Shawn C, Jonathan Neuschäfer, build bot (Jenkins), Philipp Hug,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31477
to look at the new patch set (#6).
Change subject: riscv: workaround selfboot putting the coreboot table into prog_entry_arg ......................................................................
riscv: workaround selfboot putting the coreboot table into prog_entry_arg
Change-Id: If6929897c7f12d8acb079eeebaef512ae506ca8b Signed-off-by: Xiang Wang wxjstz@126.com --- M src/arch/riscv/boot.c 1 file changed, 6 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/31477/6
Hello ron minnich, Shawn C, Jonathan Neuschäfer, build bot (Jenkins), Philipp Hug,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31477
to look at the new patch set (#7).
Change subject: riscv: workaround selfboot putting the coreboot table into prog_entry_arg ......................................................................
riscv: workaround selfboot putting the coreboot table into prog_entry_arg
Change-Id: If6929897c7f12d8acb079eeebaef512ae506ca8b Signed-off-by: Xiang Wang wxjstz@126.com --- M src/arch/riscv/boot.c 1 file changed, 7 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/31477/7
Philipp Hug has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31477 )
Change subject: riscv: workaround selfboot putting the coreboot table into prog_entry_arg ......................................................................
Patch Set 7: Code-Review+1
Thanks. Can you add this to the commit message?
On RISC-V the argument to a payload is always the hartid and a pointer to a FDT. selfboot sets the coreboot tables as an argument, work around this here.
Hello ron minnich, Shawn C, Jonathan Neuschäfer, build bot (Jenkins), Philipp Hug,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31477
to look at the new patch set (#8).
Change subject: riscv: workaround selfboot putting the coreboot table into prog_entry_arg ......................................................................
riscv: workaround selfboot putting the coreboot table into prog_entry_arg
On RISC-V the argument to a payload is always the hartid and a pointer to a FDT. selfboot sets the coreboot tables as an argument, work around this here.
Change-Id: If6929897c7f12d8acb079eeebaef512ae506ca8b Signed-off-by: Xiang Wang wxjstz@126.com --- M src/arch/riscv/boot.c 1 file changed, 7 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/31477/8
Xiang Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31477 )
Change subject: riscv: workaround selfboot putting the coreboot table into prog_entry_arg ......................................................................
Patch Set 8:
Thanks. Can you add this to the commit message?
On RISC-V the argument to a payload is always the hartid and a pointer to a FDT. selfboot sets the coreboot tables as an argument, work around this here.
Thank you! Done
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31477 )
Change subject: riscv: workaround selfboot putting the coreboot table into prog_entry_arg ......................................................................
Patch Set 8: Code-Review-1
The selfboot standard was written with x86 in mind. Please propose a new architecture specific selfboot standard first. In all cases it needs to be documented in Documentation/
Philipp Hug has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31477 )
Change subject: riscv: workaround selfboot putting the coreboot table into prog_entry_arg ......................................................................
Patch Set 8:
Stage handoff protocol is documented here: https://doc.coreboot.org/arch/riscv/index.html#stage-handoff-protocol
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31477 )
Change subject: riscv: workaround selfboot putting the coreboot table into prog_entry_arg ......................................................................
Patch Set 10: Code-Review+2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31477 )
Change subject: riscv: workaround selfboot putting the coreboot table into prog_entry_arg ......................................................................
Patch Set 10: Code-Review-2
We have two conflicting standards here: selfboot and RiscV BBL. Other arch like arm still use the coreboot table on SELF, even though the kernel requires a devicetree as argument. Changing the argument of SELF would break libpayload (which could be fixed). If we'd decide to always use the RiscV bbl calling conventions it needs to be documented in Documentation/. -2 until at least documentation is added to avoid future confusion about conflicting standards.
Philipp Hug has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31477 )
Change subject: riscv: workaround selfboot putting the coreboot table into prog_entry_arg ......................................................................
Patch Set 10:
The RISC-V stage handoff protocol is documented here: https://doc.coreboot.org/arch/riscv/index.html
Philipp Hug has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31477 )
Change subject: riscv: workaround selfboot putting the coreboot table into prog_entry_arg ......................................................................
Patch Set 10:
Patch Set 10: Code-Review-2
We have two conflicting standards here: selfboot and RiscV BBL. Other arch like arm still use the coreboot table on SELF, even though the kernel requires a devicetree as argument. Changing the argument of SELF would break libpayload (which could be fixed). If we'd decide to always use the RiscV bbl calling conventions it needs to be documented in Documentation/. -2 until at least documentation is added to avoid future confusion about conflicting standards.
The WIP libpayload implementation also uses the RISC-V calling convention with hartid+fdt as arguments.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31477 )
Change subject: riscv: workaround selfboot putting the coreboot table into prog_entry_arg ......................................................................
Patch Set 10:
Patch Set 10:
Patch Set 10: Code-Review-2
We have two conflicting standards here: selfboot and RiscV BBL. Other arch like arm still use the coreboot table on SELF, even though the kernel requires a devicetree as argument. Changing the argument of SELF would break libpayload (which could be fixed). If we'd decide to always use the RiscV bbl calling conventions it needs to be documented in Documentation/. -2 until at least documentation is added to avoid future confusion about conflicting standards.
The WIP libpayload implementation also uses the RISC-V calling convention with hartid+fdt as arguments.
Oh great, we have documentation that doesn't match what the code does.
Again, please update the documentation, explaining the conflict and mention that on SELF payloads we ignore the SELF standard and also use the RISC-V calling convention.
Philipp Hug has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31477 )
Change subject: riscv: workaround selfboot putting the coreboot table into prog_entry_arg ......................................................................
Patch Set 10:
Patch Set 10:
Patch Set 10:
Patch Set 10: Code-Review-2
We have two conflicting standards here: selfboot and RiscV BBL. Other arch like arm still use the coreboot table on SELF, even though the kernel requires a devicetree as argument. Changing the argument of SELF would break libpayload (which could be fixed). If we'd decide to always use the RiscV bbl calling conventions it needs to be documented in Documentation/. -2 until at least documentation is added to avoid future confusion about conflicting standards.
The WIP libpayload implementation also uses the RISC-V calling convention with hartid+fdt as arguments.
Oh great, we have documentation that doesn't match what the code does.
Again, please update the documentation, explaining the conflict and mention that on SELF payloads we ignore the SELF standard and also use the RISC-V calling convention.
I don't quite understand. On RISC-V we always use the same convention as documented here: https://doc.coreboot.org/arch/riscv/index.html
Philipp Hug has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31477 )
Change subject: riscv: workaround selfboot putting the coreboot table into prog_entry_arg ......................................................................
Patch Set 10:
@Xiang: Please change the following in the RISC-V documentation as well:
-On entry to a stage or payload, +On entry to a stage or payload (including SELF payloads),
Xiang Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31477 )
Change subject: riscv: workaround selfboot putting the coreboot table into prog_entry_arg ......................................................................
Patch Set 10:
Patch Set 10:
@Xiang: Please change the following in the RISC-V documentation as well:
-On entry to a stage or payload, +On entry to a stage or payload (including SELF payloads),
I don't have permission to modify
Xiang Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31477 )
Change subject: riscv: workaround selfboot putting the coreboot table into prog_entry_arg ......................................................................
Patch Set 10:
Patch Set 10:
@Xiang: Please change the following in the RISC-V documentation as well:
-On entry to a stage or payload, +On entry to a stage or payload (including SELF payloads),
Updated! Please refer https://review.coreboot.org/c/coreboot/+/33460
Philipp Hug has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31477 )
Change subject: riscv: workaround selfboot putting the coreboot table into prog_entry_arg ......................................................................
Patch Set 10: Code-Review+2
Patrick Rudolph has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31477 )
Change subject: riscv: workaround selfboot putting the coreboot table into prog_entry_arg ......................................................................
riscv: workaround selfboot putting the coreboot table into prog_entry_arg
On RISC-V the argument to a payload is always the hartid and a pointer to a FDT. selfboot sets the coreboot tables as an argument, work around this here.
Change-Id: If6929897c7f12d8acb079eeebaef512ae506ca8b Signed-off-by: Xiang Wang wxjstz@126.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/31477 Reviewed-by: ron minnich rminnich@gmail.com Reviewed-by: Patrick Rudolph siro@das-labor.org Reviewed-by: Philipp Hug philipp@hug.cx Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/arch/riscv/boot.c 1 file changed, 7 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified ron minnich: Looks good to me, approved Patrick Rudolph: Looks good to me, but someone else must approve Philipp Hug: Looks good to me, approved
diff --git a/src/arch/riscv/boot.c b/src/arch/riscv/boot.c index edf5295..8e4bb36 100644 --- a/src/arch/riscv/boot.c +++ b/src/arch/riscv/boot.c @@ -19,6 +19,7 @@ #include <arch/encoding.h> #include <arch/smp/smp.h> #include <mcall.h> +#include <commonlib/cbfs_serialized.h>
/* * A pointer to the Flattened Device Tree passed to coreboot by the boot ROM. @@ -34,6 +35,12 @@ void *fdt = prog_entry_arg(prog);
/* + * Workaround selfboot putting the coreboot table into prog_entry_arg + */ + if (prog_cbfs_type(prog) == CBFS_TYPE_SELF) + fdt = HLS()->fdt; + + /* * If prog_entry_arg is not set (e.g. by fit_payload), use fdt from HLS * instead. */
-
build bot (Jenkins) (Code Review) -
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Philipp Hug (Code Review)
-
ron minnich (Code Review)
-
Xiang Wang (Code Review)