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.