Nitheesh Sekar has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33160
Change subject: qcs405: Add board_id entry in the interface table ......................................................................
qcs405: Add board_id entry in the interface table
This patch adds board id entry in the interface table.
Change-Id: I2de11b6357db0eb2e1966aaf5069d002dc39a10e Signed-off-by: Nitheesh Sekar nsekar@codeaurora.org --- M src/mainboard/google/mistral/romstage.c M src/soc/qualcomm/common/include/soc/qclib_common.h M src/soc/qualcomm/qcs405/include/soc/memlayout.ld M src/soc/qualcomm/qcs405/include/soc/symbols.h M src/soc/qualcomm/qcs405/soc_blob_load.c 5 files changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/33160/1
diff --git a/src/mainboard/google/mistral/romstage.c b/src/mainboard/google/mistral/romstage.c index bce258d..a21f6f3 100644 --- a/src/mainboard/google/mistral/romstage.c +++ b/src/mainboard/google/mistral/romstage.c @@ -16,6 +16,10 @@ #include <arch/stages.h> #include <soc/qclib.h> #include <soc/usb.h> +#include <symbols.h> +#include <soc/symbols.h> +#include <boardid.h> +
static void prepare_usb(void) { @@ -28,6 +32,7 @@
void platform_romstage_main(void) { + *_board_id = (uint8_t)board_id(); /* QCLib: DDR init & train */ qclib_load_and_run(); prepare_usb(); diff --git a/src/soc/qualcomm/common/include/soc/qclib_common.h b/src/soc/qualcomm/common/include/soc/qclib_common.h index 19ec083..834a6aa 100644 --- a/src/soc/qualcomm/common/include/soc/qclib_common.h +++ b/src/soc/qualcomm/common/include/soc/qclib_common.h @@ -36,6 +36,7 @@ #define QCLIB_TE_DDR_TRAINING_DATA "ddr_training_data" #define QCLIB_TE_LIMITS_CFG_DATA "limits_cfg_data" #define QCLIB_TE_QCSDI "qcsdi" +#define QCLIB_TE_BOARD_ID "board_id"
/* BA_BMASK_VALUES (blob_attributes bit mask values) */ #define QCLIB_BA_SAVE_TO_STORAGE 0x00000001 diff --git a/src/soc/qualcomm/qcs405/include/soc/memlayout.ld b/src/soc/qualcomm/qcs405/include/soc/memlayout.ld index 4505b11..2abb33e 100644 --- a/src/soc/qualcomm/qcs405/include/soc/memlayout.ld +++ b/src/soc/qualcomm/qcs405/include/soc/memlayout.ld @@ -54,6 +54,7 @@ REGION(ddr_training, 0x8cfe000, 0x2000, 0x1000) REGION(ddr_information, 0x8d00000, 0x100, 0x100) REGION(dcb, 0x8d01000, 0x1000, 0x1000) + REGION(board_id, 0x8d02000, 0x8, 0x10) REGION(qclib_shared_data, 0x8d78000, 0x8000, 0x1000) BSRAM_END(0x8D80000)
diff --git a/src/soc/qualcomm/qcs405/include/soc/symbols.h b/src/soc/qualcomm/qcs405/include/soc/symbols.h index c0e2dcf..d8b54ec 100644 --- a/src/soc/qualcomm/qcs405/include/soc/symbols.h +++ b/src/soc/qualcomm/qcs405/include/soc/symbols.h @@ -24,6 +24,7 @@ DECLARE_REGION(dram_reserved); DECLARE_REGION(dcb); DECLARE_REGION(pmic); +DECLARE_REGION(board_id);
extern u8 _rpm[]; extern u8 _erpm[]; diff --git a/src/soc/qualcomm/qcs405/soc_blob_load.c b/src/soc/qualcomm/qcs405/soc_blob_load.c index 67ef831..4519536 100644 --- a/src/soc/qualcomm/qcs405/soc_blob_load.c +++ b/src/soc/qualcomm/qcs405/soc_blob_load.c @@ -45,6 +45,9 @@
qclib_add_if_table_entry(QCLIB_TE_DCB_SETTINGS, _dcb, size, 0);
+ qclib_add_if_table_entry(QCLIB_TE_BOARD_ID, _board_id, + REGION_SIZE(board_id), 0); + return 0;
fail:
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33160 )
Change subject: qcs405: Add board_id entry in the interface table ......................................................................
Patch Set 1:
What is this used for? QcLib should not have direct knowledge of coreboot board revisions, it should be completely board-independent. If there are board-specific differences that it needs to take into account, the interface table should offer options for specific features, and coreboot should select them based on the board.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33160 )
Change subject: qcs405: Add board_id entry in the interface table ......................................................................
Patch Set 4:
Patch Set 1:
What is this used for? QcLib should not have direct knowledge of coreboot board revisions, it should be completely board-independent. If there are board-specific differences that it needs to take into account, the interface table should offer options for specific features, and coreboot should select them based on the board.
Discussed with Julius: for stuff where we need to differentiate like that, please look into creating feature flags that are exported to the blob (e.g. "use_int_buck") and set them from coreboot rather than hiding the decision made in qclib
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33160 )
Change subject: qcs405: Add buck_type entry in the interface table ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/33160/5/src/mainboard/google/mistral/romstag... File src/mainboard/google/mistral/romstage.c:
https://review.coreboot.org/#/c/33160/5/src/mainboard/google/mistral/romstag... PS5, Line 40: if ((uint8_t)board_id() == 2) { braces {} are not necessary for single statement blocks
Hello build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33160
to look at the new patch set (#6).
Change subject: qcs405: Add buck_type entry in the interface table ......................................................................
qcs405: Add buck_type entry in the interface table
This patch adds buck_type entry in the interface table. The type of buck is selected based on the board_id.
Change-Id: I2de11b6357db0eb2e1966aaf5069d002dc39a10e Signed-off-by: Nitheesh Sekar nsekar@codeaurora.org --- M src/mainboard/google/mistral/romstage.c M src/soc/qualcomm/common/include/soc/qclib_common.h M src/soc/qualcomm/qcs405/include/soc/memlayout.ld M src/soc/qualcomm/qcs405/include/soc/symbols.h M src/soc/qualcomm/qcs405/soc_blob_load.c 5 files changed, 18 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/33160/6
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33160 )
Change subject: qcs405: Add buck_type entry in the interface table ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/#/c/33160/6/src/mainboard/google/mistral/romstag... File src/mainboard/google/mistral/romstage.c:
https://review.coreboot.org/#/c/33160/6/src/mainboard/google/mistral/romstag... PS6, Line 41: *_buck_type = ext_buck; We should not directly manipulate this memory from here. You should either implement a function like qclib_set_buck_type() in soc_blob_load.c that you call from here before calling qclib_load_and_run(), or you should make qclib_soc_blob_load() call a callback (e.g. mainboard_qclib_get_buck_type()) that is implemented in this file.
https://review.coreboot.org/#/c/33160/6/src/soc/qualcomm/common/include/soc/... File src/soc/qualcomm/common/include/soc/qclib_common.h:
https://review.coreboot.org/#/c/33160/6/src/soc/qualcomm/common/include/soc/... PS6, Line 39: #define QCLIB_TE_BUCK_TYPE "buck_type" Can we make this something a little more generic? Because it's really chipset-specific hardware configuration data and I don't think we should add a new interface table name string for every specific thing in every platform. I'd just call this chipset_config or board_config or something like that, and then each chipset can decide how big it is and what goes in it (e.g. for QCS405 you could just define a struct qcs405_board_config { u8 buck_type; }, for other SoCs it could have more fields if necessary).
https://review.coreboot.org/#/c/33160/6/src/soc/qualcomm/qcs405/include/soc/... File src/soc/qualcomm/qcs405/include/soc/memlayout.ld:
https://review.coreboot.org/#/c/33160/6/src/soc/qualcomm/qcs405/include/soc/... PS6, Line 57: REGION(buck_type, 0x8D02000, 0x8, 16) You don't really need to put an 8-byte field in here. Just have a static or global variable in the romstage code.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33160 )
Change subject: qcs405: Add board_config entry in the interface table ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33160/7/src/soc/qualcomm/qcs405/soc... File src/soc/qualcomm/qcs405/soc_blob_load.c:
https://review.coreboot.org/c/coreboot/+/33160/7/src/soc/qualcomm/qcs405/soc... PS7, Line 28: struct board_config{ missing space after struct definition
Hello Julius Werner, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33160
to look at the new patch set (#8).
Change subject: qcs405: Add board_config entry in the interface table ......................................................................
qcs405: Add board_config entry in the interface table
This patch adds board_config entry in the interface table. The type of buck is selected based on the board_id and is stored in the board_config entry.
Change-Id: I2de11b6357db0eb2e1966aaf5069d002dc39a10e Signed-off-by: Nitheesh Sekar nsekar@codeaurora.org --- M src/mainboard/google/mistral/romstage.c M src/soc/qualcomm/common/include/soc/qclib_common.h M src/soc/qualcomm/qcs405/include/soc/memlayout.ld M src/soc/qualcomm/qcs405/include/soc/symbols.h M src/soc/qualcomm/qcs405/soc_blob_load.c 5 files changed, 28 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/33160/8
Nitheesh Sekar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33160 )
Change subject: qcs405: Add board_config entry in the interface table ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33160/6/src/mainboard/google/mistra... File src/mainboard/google/mistral/romstage.c:
https://review.coreboot.org/c/coreboot/+/33160/6/src/mainboard/google/mistra... PS6, Line 41: *_buck_type = ext_buck;
We should not directly manipulate this memory from here. […]
Done
https://review.coreboot.org/c/coreboot/+/33160/6/src/soc/qualcomm/common/inc... File src/soc/qualcomm/common/include/soc/qclib_common.h:
https://review.coreboot.org/c/coreboot/+/33160/6/src/soc/qualcomm/common/inc... PS6, Line 39: #define QCLIB_TE_BUCK_TYPE "buck_type"
Can we make this something a little more generic? Because it's really chipset-specific hardware conf […]
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33160 )
Change subject: qcs405: Add board_config entry in the interface table ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33160/8/src/soc/qualcomm/qcs405/soc... File src/soc/qualcomm/qcs405/soc_blob_load.c:
https://review.coreboot.org/c/coreboot/+/33160/8/src/soc/qualcomm/qcs405/soc... PS8, Line 30: } qcs405_board_config; Just define this as a pointer:
struct board_config { ... } *qcs405_board_config = (void *)_board_config;
And then you can do
qcs405_board_config->buck_type = buck_type;
below which looks nicer.
https://review.coreboot.org/c/coreboot/+/33160/8/src/soc/qualcomm/qcs405/soc... PS8, Line 36: qcs405_board_config.buck_type = int_buck; Sorry, this code also needs to go in the mainboard. board_id() is completely mainboard-specific. qclib_set_buck_type() should take an enum buck_type parameter.
Nitheesh Sekar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33160 )
Change subject: qcs405: Add board_config entry in the interface table ......................................................................
Patch Set 8:
Hi Patrick,
Since the buck type code has to be cleaned up in coreboot, can we just abandon this change?
Thanks, Nitheesh
Patrick Georgi has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/33160 )
Change subject: qcs405: Add board_config entry in the interface table ......................................................................
Abandoned
we don't differentiate between buck types anymore
-
build bot (Jenkins) (Code Review) -
Julius Werner (Code Review)
-
Nitheesh Sekar (Code Review)
-
Patrick Georgi (Code Review)