Change in coreboot[master]: arch/riscv: Make the setting of fdt calling argument more explicit

[Arthur Heymans (Code Review)]

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36599 )

Change subject: arch/riscv: Make the setting of fdt calling argument more explicit
......................................................................

arch/riscv: Make the setting of fdt calling argument more explicit

Make sure to only rely on prog_entry_arg() in certain situations.
For instance in romstage cbmem_top() is programmed there instead of
the fdt.

Change-Id: Id08a12ad7b72ad539e934a133acf2c4a5bcdf1f9
Signed-off-by: Arthur Heymans 
---
M src/arch/riscv/boot.c
1 file changed, 8 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/36599/1

diff --git a/src/arch/riscv/boot.c b/src/arch/riscv/boot.c
index 6a23b8a..ecc230f 100644
--- a/src/arch/riscv/boot.c
+++ b/src/arch/riscv/boot.c
@@ -38,20 +38,22 @@
 {
    int hart_id;
    struct prog *prog = args->prog;
-	void *fdt = prog_entry_arg(prog);
+	void *fdt = NULL;

-	/*
-	 * Workaround selfboot putting the coreboot table into prog_entry_arg
-	 */
    if (prog_cbfs_type(prog) == CBFS_TYPE_SELF)
    	fdt = HLS()->fdt;

+	if ((ENV_BOOTBLOCK || ENV_ROMSTAGE) && prog_cbfs_type(prog) == CBFS_TYPE_STAGE)
+		fdt = HLS()->fdt;
    /*
     * If prog_entry_arg is not set (e.g. by fit_payload), use fdt from HLS
     * instead.
     */
-	if (fdt == NULL)
-		fdt = HLS()->fdt;
+	if (ENV_RAMSTAGE) {
+		fdt = prog_entry_arg(prog);
+		if (fdt == NULL)
+			fdt = HLS()->fdt;
+	}

if (ENV_RAMSTAGE && prog_type(prog) == PROG_PAYLOAD) {
    	if (CONFIG(RISCV_OPENSBI))

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36599 )

Change subject: arch/riscv: Make the setting of fdt calling argument more explicit
......................................................................

Patch Set 1:

(3 comments)

https://review.coreboot.org/c/coreboot/+/36599/1/src/arch/riscv/boot.c 
File src/arch/riscv/boot.c:

https://review.coreboot.org/c/coreboot/+/36599/1/src/arch/riscv/boot.c@46 
PS1, Line 46: 	if ((ENV_BOOTBLOCK || ENV_ROMSTAGE) && prog_cbfs_type(prog) == CBFS_TYPE_STAGE)
`else if`

https://review.coreboot.org/c/coreboot/+/36599/1/src/arch/riscv/boot.c@52 
PS1, Line 52: 	if (ENV_RAMSTAGE) {
`else if`

otherwise, we'd overwrite `fdt` from the SELF case. Also what about
RAMPAYLOAD? Couldn't that in theory be a  FIT payload that is not
loaded from ENV_RAMSTAGE?

https://review.coreboot.org/c/coreboot/+/36599/1/src/arch/riscv/boot.c@56 
PS1, Line 56: 	}
Alternative flow (I'm not sure of PROG_PAYLOAD is enough; other cases?):

void *fdt = NULL;

if (prog_type(prog) == PROG_PAYLOAD && prog_cbfs_type(prog) != CBFS_TYPE_SELF) {
            /* prog_entry_arg() may provide the FDT pointer. */
            fdt = prog_entry_arg(prog);
    }
    if (fdt == NULL)
            fdt = HLS()->fdt;

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36599 )

Change subject: arch/riscv: Make the setting of fdt calling argument more explicit
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36599/1/src/arch/riscv/boot.c 
File src/arch/riscv/boot.c:

https://review.coreboot.org/c/coreboot/+/36599/1/src/arch/riscv/boot.c@56 
PS1, Line 56: 	}
> Alternative flow (I'm not sure of PROG_PAYLOAD is enough; other cases?): […]

Actually, just blacklisting needs less assumptions:

void *fdt = NULL;

if (prog_cbfs_type(prog) != CBFS_TYPE_SELF &&
        prog_cbfs_type(prog) != CBFS_TYPE_STAGE) {
            /* prog_entry_arg() may provide the FDT pointer. */
            fdt = prog_entry_arg(prog);
    }
    if (fdt == NULL)
            fdt = HLS()->fdt;

OTOH, if we knew exactly that only the FIT loader uses `arg` for
FDT, and will always set it != NULL, inverting the whole thing
would provide the shortest version:

void *fdt = HLS()->fdt;

if (prog_cbfs_type(prog) == CBFS_TYPE_FIT)
        fdt = prog_entry_arg(prog);

[Philipp Hug (Code Review)]

Philipp Hug has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36599 )

Change subject: arch/riscv: Make the setting of fdt calling argument more explicit
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36599/1/src/arch/riscv/boot.c 
File src/arch/riscv/boot.c:

https://review.coreboot.org/c/coreboot/+/36599/1/src/arch/riscv/boot.c@56 
PS1, Line 56: 	}
> Actually, just blacklisting needs less assumptions: […]

Check for FIT is probably the smartest option as it's the only place where it is used at the moment and makes it more explicit.

Change in coreboot[master]: arch/riscv: Use FDT from calling argument when using FIT

[Arthur Heymans (Code Review)]

Hello Patrick Rudolph, Aaron Durbin, ron minnich, Jonathan Neuschäfer, build bot (Jenkins), Philipp Hug,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/36599

to look at the new patch set (#4).

Change subject: arch/riscv: Use FDT from calling argument when using FIT
......................................................................

arch/riscv: Use FDT from calling argument when using FIT

Only FIT payloads provide their own FDT.

Change-Id: Id08a12ad7b72ad539e934a133acf2c4a5bcdf1f9
Signed-off-by: Arthur Heymans 
---
M src/arch/riscv/boot.c
1 file changed, 3 insertions(+), 13 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/36599/4

Change in coreboot[master]: arch/riscv: Use FDT from calling argument when using FIT

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36599 )

Change subject: arch/riscv: Use FDT from calling argument when using FIT
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36599/4/src/arch/riscv/boot.c 
File src/arch/riscv/boot.c:

https://review.coreboot.org/c/coreboot/+/36599/4/src/arch/riscv/boot.c@43 
PS4, Line 43: PROG_FIT
naah, that's no CBFS type :)

Change in coreboot[master]: arch/riscv: Use FDT from calling argument when using FIT

[Arthur Heymans (Code Review)]

Hello Patrick Rudolph, Aaron Durbin, ron minnich, Jonathan Neuschäfer, build bot (Jenkins), Philipp Hug,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/36599

to look at the new patch set (#5).

Change subject: arch/riscv: Use FDT from calling argument when using FIT
......................................................................

arch/riscv: Use FDT from calling argument when using FIT

Only FIT payloads provide their own FDT.

Change-Id: Id08a12ad7b72ad539e934a133acf2c4a5bcdf1f9
Signed-off-by: Arthur Heymans 
---
M src/arch/riscv/boot.c
1 file changed, 3 insertions(+), 13 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/36599/5

Change in coreboot[master]: arch/riscv: Use FDT from calling argument when using FIT

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36599 )

Change subject: arch/riscv: Use FDT from calling argument when using FIT
......................................................................

Patch Set 5: Code-Review+2

(4 comments)

https://review.coreboot.org/c/coreboot/+/36599/1/src/arch/riscv/boot.c 
File src/arch/riscv/boot.c:

https://review.coreboot.org/c/coreboot/+/36599/1/src/arch/riscv/boot.c@46 
PS1, Line 46: 	if ((ENV_BOOTBLOCK || ENV_ROMSTAGE) && prog_cbfs_type(prog) == CBFS_TYPE_STAGE)
> `else if`

Nope

https://review.coreboot.org/c/coreboot/+/36599/1/src/arch/riscv/boot.c@52 
PS1, Line 52: 	if (ENV_RAMSTAGE) {
> `else if` […]

Nope

https://review.coreboot.org/c/coreboot/+/36599/1/src/arch/riscv/boot.c@56 
PS1, Line 56: 	}
> Check for FIT is probably the smartest option as it's the only place where it is used at the moment  […]

Done

https://review.coreboot.org/c/coreboot/+/36599/4/src/arch/riscv/boot.c 
File src/arch/riscv/boot.c:

https://review.coreboot.org/c/coreboot/+/36599/4/src/arch/riscv/boot.c@43 
PS4, Line 43: PROG_FIT
> naah, that's no CBFS type :)

Done

Change in coreboot[master]: arch/riscv: Use FDT from calling argument when using FIT

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36599 )

Change subject: arch/riscv: Use FDT from calling argument when using FIT
......................................................................

arch/riscv: Use FDT from calling argument when using FIT

Only FIT payloads provide their own FDT.

Change-Id: Id08a12ad7b72ad539e934a133acf2c4a5bcdf1f9
Signed-off-by: Arthur Heymans 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/36599
Reviewed-by: Nico Huber 
Tested-by: build bot (Jenkins) 
---
M src/arch/riscv/boot.c
1 file changed, 3 insertions(+), 13 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved

diff --git a/src/arch/riscv/boot.c b/src/arch/riscv/boot.c
index 6a23b8a..d3ae693 100644
--- a/src/arch/riscv/boot.c
+++ b/src/arch/riscv/boot.c
@@ -38,20 +38,10 @@
 {
    int hart_id;
    struct prog *prog = args->prog;
-	void *fdt = prog_entry_arg(prog);
+	void *fdt = HLS()->fdt;

-	/*
-	 * 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.
-	 */
-	if (fdt == NULL)
-		fdt = HLS()->fdt;
+	if (prog_cbfs_type(prog) == CBFS_TYPE_FIT)
+		fdt = prog_entry_arg(prog);

if (ENV_RAMSTAGE && prog_type(prog) == PROG_PAYLOAD) {
    	if (CONFIG(RISCV_OPENSBI))