Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36087 )
Change subject: soc/intel/tigerlake: Do initial SoC commit till ramstage ......................................................................
Patch Set 12: Code-Review-1
(8 comments)
Patch Set 12:
Patch Set 12:
I have divided this CL into possible logic groups as https://review.coreboot.org/q/topic:%22TGL_UPSTREAM%22+(status:open%20OR%20s...)
Kindly help to review.
Also this CL is based on all clean up been done so far with
https://review.coreboot.org/q/topic:%22ICL_CLEAN_UP_FOR_TGL_UPSTREAM%22+(sta...)
Thanks to all reviewer so far helping to clean ICL code in order to create soc/intel/tigerlake copy CL as mentioned above.
HI Arthur/Philipp,
Will you please consider your -2 with this new sets of split CL in place and we have icl base clean up cl also merged ? This will help us to land TGL SOC copy code ?
This still has received no real review. It references many docs that I cannot find so how do you expect proper review to happen?
https://review.coreboot.org/c/coreboot/+/36087/12/src/soc/intel/tigerlake/Kc... File src/soc/intel/tigerlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/36087/12/src/soc/intel/tigerlake/Kc... PS12, Line 81: depends on FSP_USES_CB_STACK why?
https://review.coreboot.org/c/coreboot/+/36087/12/src/soc/intel/tigerlake/Ma... File src/soc/intel/tigerlake/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36087/12/src/soc/intel/tigerlake/Ma... PS12, Line 57: postcar-y += memmap.c only needed in romstage anymore in master. Needs a rebase.
https://review.coreboot.org/c/coreboot/+/36087/12/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/memmap.c:
https://review.coreboot.org/c/coreboot/+/36087/12/src/soc/intel/tigerlake/me... PS12, Line 187: /* Fill up memory layout information */ : void fill_soc_memmap_ebda(struct ebda_config *cfg) : { : size_t chipset_mem_size; : : cfg->tolum_base = calculate_dram_base(&chipset_mem_size); : cfg->reserved_mem_size = chipset_mem_size; : } Please don't use this.
https://review.coreboot.org/c/coreboot/+/36087/12/src/soc/intel/tigerlake/me... PS12, Line 195: : void cbmem_top_init(void) : { : /* Fill up EBDA area */ : fill_ebda_area(); : } remove.
https://review.coreboot.org/c/coreboot/+/36087/12/src/soc/intel/tigerlake/me... PS12, Line 229: void *cbmem_top(void) rebase on master please.
https://review.coreboot.org/c/coreboot/+/36087/12/src/soc/intel/tigerlake/pm... File src/soc/intel/tigerlake/pmutil.c:
https://review.coreboot.org/c/coreboot/+/36087/12/src/soc/intel/tigerlake/pm... PS12, Line 140: void cast to uint8_t *
https://review.coreboot.org/c/coreboot/+/36087/12/src/soc/intel/tigerlake/sm... File src/soc/intel/tigerlake/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/36087/12/src/soc/intel/tigerlake/sm... PS12, Line 1: /* Is this file not copied 1 on 1 many times in the tree? Make it common?
https://review.coreboot.org/c/coreboot/+/36087/12/src/soc/intel/tigerlake/sm... PS12, Line 191: tseg_base Check if aligned to tseg_size.