Usha P has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/18457 )
Change subject: soc/intel/common: Add bootblock common stage file ......................................................................
Patch Set 33:
(7 comments)
https://review.coreboot.org/c/coreboot/+/18457/33/src/soc/intel/common/basec... File src/soc/intel/common/basecode/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/18457/33/src/soc/intel/common/basec... PS33, Line 38: static const struct bootblock_ops g_bb_ops = { : &bootblock_cmn_soc_early_init, : &bootblock_cmn_soc_init, : &bootblock_cmn_pch_early_init, : &bootblock_cmn_pch_init, : &bootblock_cmn_cpu_early_init, : &bootblock_cmn_cpu_init : };
Why do you want to use a struct with function pointers?
As per the design assumption, we do not want to keep the function name generic across Soc, so the SoC can decide on the name. Hence made this a struct with function pointers.
https://review.coreboot.org/c/coreboot/+/18457/33/src/soc/intel/common/basec... PS33, Line 47: __weak
why weak?
Not required. Will remove it. Thanks
https://review.coreboot.org/c/coreboot/+/18457/33/src/soc/intel/common/basec... PS33, Line 49: (struct bootblock_ops *)
cast not needed.
Yes, Will remove it.
https://review.coreboot.org/c/coreboot/+/18457/33/src/soc/intel/common/basec... PS33, Line 70: bb_ops->bootblock_cpu_init();
Should we also check pointer prior calling functions? […]
Yes Thankyou, I will add to check pointer prior calling functions.
https://review.coreboot.org/c/coreboot/+/18457/33/src/soc/intel/common/basec... PS33, Line 105: bootblock_fsp_temp_ram_init();
what is this supposed to do? I can't find a definition.
FSP 1.1 is not valid, hence will drop it.
https://review.coreboot.org/c/coreboot/+/18457/33/src/soc/intel/common/basec... PS33, Line 111: /* Program SMBUS_BASE_ADDRESS and Enable it */ : smbus_common_init();
why would you need smbus in the bootblock?
This is moved to romstage. Will delete it from here. Thank you.
https://review.coreboot.org/c/coreboot/+/18457/33/src/soc/intel/common/basec... PS33, Line 119: /* initialize Heci interface */ : heci_init(HECI1_BASE_ADDRESS);
why does this need to happen in the bootblock?
This is moved to romstage. Will delete it from here.