Hung-Te Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32541
Change subject: soc/mediatek: Register coreboot table for ATF ......................................................................
soc/mediatek: Register coreboot table for ATF
Register coreboot table into BL31 parameter so ATF can find coreboot information and use it (for example, use memory console).
BUG=None TEST=emerge-kukui coreboot; Boots on Kukui.
Change-Id: I6e39495988159608c1c4bbf16a97c86e526bd82b Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/soc/mediatek/common/bl31_plat_params.c 1 file changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/32541/1
diff --git a/src/soc/mediatek/common/bl31_plat_params.c b/src/soc/mediatek/common/bl31_plat_params.c index eb7477a..51edab3 100644 --- a/src/soc/mediatek/common/bl31_plat_params.c +++ b/src/soc/mediatek/common/bl31_plat_params.c @@ -14,6 +14,7 @@ */
#include <arm_tf.h> +#include <cbmem.h> #include <soc/bl31_plat_params.h>
static struct bl31_plat_param *plat_params; @@ -26,5 +27,13 @@
void *soc_get_bl31_plat_params(bl31_params_t *bl31_params) { + static struct bl31_u64_param cbtable_param = { + .h = { .type = PARAM_COREBOOT_TABLE, }, + }; + if (!cbtable_param.value) { + cbtable_param.value = (uint64_t)cbmem_find(CBMEM_ID_CBTABLE); + if (cbtable_param.value) + register_bl31_param(&cbtable_param.h); + } return plat_params; }
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32541 )
Change subject: soc/mediatek: Register coreboot table for ATF ......................................................................
Patch Set 1: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32541 )
Change subject: soc/mediatek: Register coreboot table for ATF ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32541/4/src/soc/mediatek/common/bl3... File src/soc/mediatek/common/bl31_plat_params.c:
https://review.coreboot.org/c/coreboot/+/32541/4/src/soc/mediatek/common/bl3... PS4, Line 33: if (!cbtable_param.value) { Why is this check useful? Can’t it be removed and the assignment always happen?
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32541 )
Change subject: soc/mediatek: Register coreboot table for ATF ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32541/4/src/soc/mediatek/common/bl3... File src/soc/mediatek/common/bl31_plat_params.c:
https://review.coreboot.org/c/coreboot/+/32541/4/src/soc/mediatek/common/bl3... PS4, Line 33: if (!cbtable_param.value) {
Why is this check useful? Can’t it be removed and the assignment always happen?
This is following rk3399 implementation.
@jwerner may confirm, but I believe the underlying expectation is - ATF may call soc_get_bl31_plat_params multiple times (although it seems not doing that today) - We want to prevent unnecessary cbmem_find calls - We want to only insert bl31_plat_param into bl31_params one time - If the function is called multiple times, we want CBTABLE is still injected in the last invocation.
If there's some promise that ATF &coreboot only ever only call soc_get_bl31_plat_params one time, then yes we may revise this as always-happen.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32541 )
Change subject: soc/mediatek: Register coreboot table for ATF ......................................................................
Patch Set 4:
If the function is called multiple times, we want CBTABLE is still injected in the last invocation.
correction:
f the function is called multiple times and CBTABLE was not available in the early calls, we want CBTABLE is still injected whenever it's available.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32541 )
Change subject: soc/mediatek: Register coreboot table for ATF ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32541/4/src/soc/mediatek/common/bl3... File src/soc/mediatek/common/bl31_plat_params.c:
https://review.coreboot.org/c/coreboot/+/32541/4/src/soc/mediatek/common/bl3... PS4, Line 33: if (!cbtable_param.value) {
This is following rk3399 implementation. […]
To make this easier to read, it's also possible to introduce one flag:
static int cbtable_injected = 0;
if (ctable_injected) return plat_params;
cbtable_param.value = (uint64_t)cbmem_find(CBMEM_ID_CBTABLE); if (cbtable_param.value) { register_bl31_param(&cbtable_param.h); cbtable_injected = 1; }
return plat_params;
Do you prefer this version?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32541 )
Change subject: soc/mediatek: Register coreboot table for ATF ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32541/4/src/soc/mediatek/common/bl3... File src/soc/mediatek/common/bl31_plat_params.c:
https://review.coreboot.org/c/coreboot/+/32541/4/src/soc/mediatek/common/bl3... PS4, Line 33: if (!cbtable_param.value) {
To make this easier to read, it's also possible to introduce one flag: […]
Let’s wait for Julius’ answer first.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32541 )
Change subject: soc/mediatek: Register coreboot table for ATF ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32541/4/src/soc/mediatek/common/bl3... File src/soc/mediatek/common/bl31_plat_params.c:
https://review.coreboot.org/c/coreboot/+/32541/4/src/soc/mediatek/common/bl3... PS4, Line 33: if (!cbtable_param.value) {
Let’s wait for Julius’ answer first.
Yes, the goal was just to make this function idempotent as a precaution in case it's ever called twice somewhere. Please just submit it like this (following existing practice) for now... I have changes in the pipe that will unify all of this in generic code anyway, they're just taking a while to go through on the Trusted Firmware side.
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32541 )
Change subject: soc/mediatek: Register coreboot table for ATF ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32541/4/src/soc/mediatek/common/bl3... File src/soc/mediatek/common/bl31_plat_params.c:
https://review.coreboot.org/c/coreboot/+/32541/4/src/soc/mediatek/common/bl3... PS4, Line 33: if (!cbtable_param.value) {
Yes, the goal was just to make this function idempotent as a precaution in case it's ever called twi […]
Ack
Hung-Te Lin has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/32541 )
Change subject: soc/mediatek: Register coreboot table for ATF ......................................................................
Abandoned
no longer needed since jwerner's new version.
-
Hung-Te Lin (Code Review) -
Julius Werner (Code Review)
-
Paul Menzel (Code Review)
-
Yu-Ping Wu (Code Review)