Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36087 )
Change subject: soc/intel/tigerlake: Do initial SoC commit ......................................................................
Patch Set 10:
(7 comments)
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/Kc... File src/soc/intel/tigerlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/Kc... PS10, Line 73: FSP_USES_CB_STACK
PLATFORM_USES_FSP2_1 indirectly selects this. You might want to drop the dependency.
Done
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/Ma... File src/soc/intel/tigerlake/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/Ma... PS10, Line 1: feq ($(CONFIG_SOC_INTEL_TIGERLAKE),y)
There is now an 'all' class to link code in bootblock,verstage,romstage,postcar,ramstage. […]
Don't understand your feedback here Arthur. Do you mean that we can remove this guard ?
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/bootblock.h:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/in... PS10, Line 18: : #include <intelblocks/systemagent.h>
Don't include it here but in the file implementing the callbacks below.
Done
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/cpu.h:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/in... PS10, Line 18: : #include <device/device.h> : #include <intelblocks/msr.h>
unused here. Include this in the file defining the function.
removed unused #include <device/device.h>
and#include <intelblocks/msr.h> used here for IRTL_1024_NS so can't move
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/in... PS10, Line 21: : /* Latency times in units of 32768ns */ : #define C_STATE_LATENCY_CONTROL_0_LIMIT 0x9d : #define C_STATE_LATENCY_CONTROL_1_LIMIT 0x9d : #define C_STATE_LATENCY_CONTROL_2_LIMIT 0x9d : #define C_STATE_LATENCY_CONTROL_3_LIMIT 0x9d : #define C_STATE_LATENCY_CONTROL_4_LIMIT 0x9d : #define C_STATE_LATENCY_CONTROL_5_LIMIT 0x9d : : /* Power in units of mW */ : #define C1_POWER 0x3e8 : #define C6_POWER 0x15e : #define C7_POWER 0xc8 : #define C8_POWER 0xc8 : #define C9_POWER 0xc8 : #define C10_POWER 0xc8 : : /* Common Timer Copy (CTC) frequency - 38.4MHz. */ : #define CTC_FREQ 38400000 : : #define C_STATE_LATENCY_MICRO_SECONDS(limit, base) \ : (((1 << ((base)*5)) * (limit)) / 1000) : #define C_STATE_LATENCY_FROM_LAT_REG(reg) \ : C_STATE_LATENCY_MICRO_SECONDS(C_STATE_LATENCY_CONTROL_ ##reg## _LIMIT, \ : (IRTL_1024_NS >> 10))
Is this used in multiple files? If not better move this inside the compilation unit defining the fun […]
getting also used from acpi.c hence keeping it here
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/ebda.h:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/in... PS10, Line 21: struct ebda_config { : uint32_t signature; /* 0x00 - EBDA signature */ : uint32_t tolum_base; /* 0x04 - coreboot memory start */ : uint32_t reserved_mem_size; /* 0x08 - chipset reserved memory size */ : };
note: we might get rid of this soon: CB:36362
Sure will remove from TGL copy CL
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/espi.h:
https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/in... PS10, Line 19: #include <stdint.h>
not needed here?
Done
-
Subrata Banik (Code Review)