coreboot-gerrit November 2021

coreboot-gerrit@coreboot.org

1 participants
6394 discussions

Change in coreboot[master]: soc/intel/common: Implement ACPI CPPCv3 package to support hybrid core
by Arthur Heymans (Code Review) 24 Nov '21

24 Nov '21

Attention is currently required from: Nico Huber, Paul Menzel, Tim Wawrzynczak, Ravindra, Sridhar Siricilla, Subrata Banik, Arthur Heymans, Michael Niewöhner, Patrick Rudolph. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59359 ) Change subject: soc/intel/common: Implement ACPI CPPCv3 package to support hybrid core ...................................................................... Patch Set 3: Code-Review-1 (4 comments) Patchset: PS3: Having read this code I'm very much unconvinced of the code to count big/small core into a bitmask and using it in SSDT generated code. I propose to add the cpu information in some way in struct device and generate simple code in dsdt/ssdt based on that. That is better than encoding it into a bitmask, passing it to the ssdt generator and then consuming it as a magic number inside generated ssdt code. File src/soc/intel/common/block/acpi/cpu_hybrid.c: https://review.coreboot.org/c/coreboot/+/59359/comment/05e7f1df_e42b5393 PS3, Line 102: acpi_get_cpu_hybrid_info(&cpu_hybrid_info); : acpigen_write_method(XPPC_PACKAGE_NAME, 1); : : if (cpu_is_nominal_freq_supported()) { : : /* : * If Nominal Frequency is supported, update Nominal Frequency and set core's : * Nominal Performance to a scaling factor which depends on core type(big or : * small). : */ : acpigen_set_package_element_int(CPPC_PACKAGE_NAME, 22, : cpu_hybrid_info.nominal_freq); : : /* LOCAL2_OP = (CORE >> core_id) & 1 */ : acpigen_emit_byte(SHIFT_RIGHT_OP); : acpigen_write_integer(cpu_hybrid_info.type_mask); : acpigen_emit_byte(ARG0_OP); : acpigen_emit_byte(LOCAL1_OP); : acpigen_write_and(LOCAL1_OP, ONE_OP, LOCAL2_OP); : : /* : * If core id is big Core, then set big core's scaling factor : * to core's nominal frequency otherwise set small core's scaling factor. : */ : acpigen_write_if_lequal_op_int(LOCAL2_OP, 1); : acpigen_set_package_element_int(CPPC_PACKAGE_NAME, 3, : cpu_hybrid_info.big_core_nom_perf); : acpigen_write_else(); : acpigen_set_package_element_int(CPPC_PACKAGE_NAME, 3, : cpu_hybrid_info.small_core_nom_perf); : acpigen_pop_len(); : } : : acpigen_emit_byte(RETURN_OP); : acpigen_emit_namestring(CPPC_PACKAGE_NAME); : acpigen_pop_len(); Is it worth it to implement it fully in SSDT? It looks like you could implement it in DSDT and just fill in some namespace int variables in SSDT. https://review.coreboot.org/c/coreboot/+/59359/comment/317853d3_92efaa37 PS3, Line 152: acpigen_write_dword(core_id); why not call XPPC using the actual information you want: big or small core instead of passing the core ID and then decoding that information. That bitmask thing will be very hard to read when decompiling the bytecode. https://review.coreboot.org/c/coreboot/+/59359/comment/3d60b14d_0441d864 PS3, Line 98: void acpi_write_xppc_method(int core_id) : { : struct cpu_hybrid cpu_hybrid_info; : : acpi_get_cpu_hybrid_info(&cpu_hybrid_info); : acpigen_write_method(XPPC_PACKAGE_NAME, 1); : : if (cpu_is_nominal_freq_supported()) { : : /* : * If Nominal Frequency is supported, update Nominal Frequency and set core's : * Nominal Performance to a scaling factor which depends on core type(big or : * small). : */ : acpigen_set_package_element_int(CPPC_PACKAGE_NAME, 22, : cpu_hybrid_info.nominal_freq); : : /* LOCAL2_OP = (CORE >> core_id) & 1 */ : acpigen_emit_byte(SHIFT_RIGHT_OP); : acpigen_write_integer(cpu_hybrid_info.type_mask); : acpigen_emit_byte(ARG0_OP); : acpigen_emit_byte(LOCAL1_OP); : acpigen_write_and(LOCAL1_OP, ONE_OP, LOCAL2_OP); : : /* : * If core id is big Core, then set big core's scaling factor : * to core's nominal frequency otherwise set small core's scaling factor. : */ : acpigen_write_if_lequal_op_int(LOCAL2_OP, 1); : acpigen_set_package_element_int(CPPC_PACKAGE_NAME, 3, : cpu_hybrid_info.big_core_nom_perf); : acpigen_write_else(); : acpigen_set_package_element_int(CPPC_PACKAGE_NAME, 3, : cpu_hybrid_info.small_core_nom_perf); : acpigen_pop_len(); : } : : acpigen_emit_byte(RETURN_OP); : acpigen_emit_namestring(CPPC_PACKAGE_NAME); : acpigen_pop_len(); : } : : void acpigen_write_CPPC_hybrid_method(int core_id) : { : char scope[16]; : : if (core_id == 0) : snprintf(scope, 12, XPPC_PACKAGE_NAME, 0); : else : snprintf(scope, 16, CONFIG_ACPI_CPU_STRING "." XPPC_PACKAGE_NAME, 0); : : acpigen_write_method("_CPC", 0); : acpigen_emit_byte(RETURN_OP); : acpigen_emit_namestring(scope); : acpigen_write_dword(core_id); : acpigen_pop_len(); I'm a bit confused. Each CPU has _CPC method and they all use the implementation called XPPC_PACKAGE_NAME of CPU0? Might just as well generate it for each CPU and drop that weird bitmask code altogether. -- To view, visit https://review.coreboot.org/c/coreboot/+/59359 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icd5ea9e70bebd1e66d3cea2bcf8a6678e5cc95ca Gerrit-Change-Number: 59359 Gerrit-PatchSet: 3 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Ravindra <ravindra(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Sridhar Siricilla <sridhar.siricilla(a)intel.corp-partner.google.com> Gerrit-CC: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Ravindra <ravindra(a)intel.com> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Wed, 24 Nov 2021 15:34:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/amd/*/data_fabric: use DF_ prefix for bit and shift defines
by Felix Held (Code Review) 24 Nov '21

24 Nov '21

Attention is currently required from: Jason Glenesk, Raul Rangel, Marshall Dawson. Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/59627 ) Change subject: soc/amd/*/data_fabric: use DF_ prefix for bit and shift defines ...................................................................... soc/amd/*/data_fabric: use DF_ prefix for bit and shift defines Adding the DP_ prefix to the defines for MMIO_NP, MMIO_WE and MMIO_RE clarifies the scope of those definitions. For consistency also add this prefix to MMIO_DST_FABRIC_ID_SHIFT. Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de> Change-Id: I3a509ccc071aa51a67552fb9e7195358a76fe4dc --- M src/soc/amd/cezanne/data_fabric.c M src/soc/amd/cezanne/include/soc/data_fabric.h M src/soc/amd/common/block/data_fabric/data_fabric_helper.c M src/soc/amd/common/block/include/amdblocks/data_fabric.h M src/soc/amd/picasso/data_fabric.c M src/soc/amd/picasso/include/soc/data_fabric.h 6 files changed, 13 insertions(+), 13 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/59627/1 diff --git a/src/soc/amd/cezanne/data_fabric.c b/src/soc/amd/cezanne/data_fabric.c index 1a0f9cb..2a13d20 100644 --- a/src/soc/amd/cezanne/data_fabric.c +++ b/src/soc/amd/cezanne/data_fabric.c @@ -44,7 +44,7 @@ for (i = 0; i < NUM_NB_MMIO_REGS; i++) { /* Adjust all registers that overlap */ ctrl = data_fabric_broadcast_read32(0, NB_MMIO_CONTROL(i)); - if (!(ctrl & (MMIO_WE | MMIO_RE))) + if (!(ctrl & (DF_MMIO_WE | DF_MMIO_RE))) continue; /* not enabled */ base = data_fabric_broadcast_read32(0, NB_MMIO_BASE(i)); @@ -92,8 +92,8 @@ data_fabric_broadcast_write32(0, NB_MMIO_BASE(reg), np_bot); data_fabric_broadcast_write32(0, NB_MMIO_LIMIT(reg), np_top); data_fabric_broadcast_write32(0, NB_MMIO_CONTROL(reg), - (IOMS0_FABRIC_ID << MMIO_DST_FABRIC_ID_SHIFT) | MMIO_NP | MMIO_WE - | MMIO_RE); + (IOMS0_FABRIC_ID << DF_MMIO_DST_FABRIC_ID_SHIFT) | DF_MMIO_NP + | DF_MMIO_WE | DF_MMIO_RE); data_fabric_print_mmio_conf(); } diff --git a/src/soc/amd/cezanne/include/soc/data_fabric.h b/src/soc/amd/cezanne/include/soc/data_fabric.h index 13a96e0..27444be 100644 --- a/src/soc/amd/cezanne/include/soc/data_fabric.h +++ b/src/soc/amd/cezanne/include/soc/data_fabric.h @@ -6,7 +6,7 @@ #include <types.h> /* SoC-specific bits in D18F0_MMIO_CTRL0 */ -#define MMIO_NP BIT(16) +#define DF_MMIO_NP BIT(16) #define IOMS0_FABRIC_ID 10 diff --git a/src/soc/amd/common/block/data_fabric/data_fabric_helper.c b/src/soc/amd/common/block/data_fabric/data_fabric_helper.c index abb4ba8..0fcee36 100644 --- a/src/soc/amd/common/block/data_fabric/data_fabric_helper.c +++ b/src/soc/amd/common/block/data_fabric/data_fabric_helper.c @@ -63,7 +63,7 @@ void data_fabric_disable_mmio_reg(unsigned int reg) { data_fabric_broadcast_write32(0, NB_MMIO_CONTROL(reg), - IOMS0_FABRIC_ID << MMIO_DST_FABRIC_ID_SHIFT); + IOMS0_FABRIC_ID << DF_MMIO_DST_FABRIC_ID_SHIFT); data_fabric_broadcast_write32(0, NB_MMIO_BASE(reg), 0); data_fabric_broadcast_write32(0, NB_MMIO_LIMIT(reg), 0); } @@ -71,7 +71,7 @@ static bool is_mmio_reg_disabled(unsigned int reg) { uint32_t val = data_fabric_broadcast_read32(0, NB_MMIO_CONTROL(reg)); - return !(val & (MMIO_WE | MMIO_RE)); + return !(val & (DF_MMIO_WE | DF_MMIO_RE)); } int data_fabric_find_unused_mmio_reg(void) diff --git a/src/soc/amd/common/block/include/amdblocks/data_fabric.h b/src/soc/amd/common/block/include/amdblocks/data_fabric.h index ede5075..604c24e 100644 --- a/src/soc/amd/common/block/include/amdblocks/data_fabric.h +++ b/src/soc/amd/common/block/include/amdblocks/data_fabric.h @@ -16,9 +16,9 @@ #define D18F0_MMIO_SHIFT 16 #define D18F0_MMIO_CTRL0 0x208 /* The MMIO_NP bit is SoC-specific */ -#define MMIO_DST_FABRIC_ID_SHIFT 4 -#define MMIO_WE BIT(1) -#define MMIO_RE BIT(0) +#define DF_MMIO_DST_FABRIC_ID_SHIFT 4 +#define DF_MMIO_WE BIT(1) +#define DF_MMIO_RE BIT(0) /* The number of data fabric MMIO registers is SoC-specific */ #define NB_MMIO_BASE(reg) ((reg) * 4 * sizeof(uint32_t) + D18F0_MMIO_BASE0) diff --git a/src/soc/amd/picasso/data_fabric.c b/src/soc/amd/picasso/data_fabric.c index b5949f0..aec8a9e 100644 --- a/src/soc/amd/picasso/data_fabric.c +++ b/src/soc/amd/picasso/data_fabric.c @@ -44,7 +44,7 @@ for (i = 0; i < NUM_NB_MMIO_REGS; i++) { /* Adjust all registers that overlap */ ctrl = data_fabric_broadcast_read32(0, NB_MMIO_CONTROL(i)); - if (!(ctrl & (MMIO_WE | MMIO_RE))) + if (!(ctrl & (DF_MMIO_WE | DF_MMIO_RE))) continue; /* not enabled */ base = data_fabric_broadcast_read32(0, NB_MMIO_BASE(i)); @@ -92,8 +92,8 @@ data_fabric_broadcast_write32(0, NB_MMIO_BASE(reg), np_bot); data_fabric_broadcast_write32(0, NB_MMIO_LIMIT(reg), np_top); data_fabric_broadcast_write32(0, NB_MMIO_CONTROL(reg), - (IOMS0_FABRIC_ID << MMIO_DST_FABRIC_ID_SHIFT) | MMIO_NP | MMIO_WE - | MMIO_RE); + (IOMS0_FABRIC_ID << DF_MMIO_DST_FABRIC_ID_SHIFT) | DF_MMIO_NP + | DF_MMIO_WE | DF_MMIO_RE); data_fabric_print_mmio_conf(); } diff --git a/src/soc/amd/picasso/include/soc/data_fabric.h b/src/soc/amd/picasso/include/soc/data_fabric.h index d652d19..5533433 100644 --- a/src/soc/amd/picasso/include/soc/data_fabric.h +++ b/src/soc/amd/picasso/include/soc/data_fabric.h @@ -8,7 +8,7 @@ /* D18F0 - Fabric Configuration registers */ /* SoC-specific bits in D18F0_MMIO_CTRL0 */ -#define MMIO_NP BIT(12) +#define DF_MMIO_NP BIT(12) #define IOMS0_FABRIC_ID 9 -- To view, visit https://review.coreboot.org/c/coreboot/+/59627 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3a509ccc071aa51a67552fb9e7195358a76fe4dc Gerrit-Change-Number: 59627 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: soc/amd/*/include/data_fabric: make MMIO_NP definition SoC-specific
by Felix Held (Code Review) 24 Nov '21

24 Nov '21

Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/59626 ) Change subject: soc/amd/*/include/data_fabric: make MMIO_NP definition SoC-specific ...................................................................... soc/amd/*/include/data_fabric: make MMIO_NP definition SoC-specific On Picasso the MMIO_NP bit in the D18F0_MMIO_CTRL0 data fabric register is bit 12, but that has changed to bit 16 in Cezanne. Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de> Change-Id: I64c06b84e2c0737b259077e7932f418306638e19 --- M src/soc/amd/cezanne/include/soc/data_fabric.h M src/soc/amd/common/block/include/amdblocks/data_fabric.h M src/soc/amd/picasso/include/soc/data_fabric.h 3 files changed, 8 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/59626/1 diff --git a/src/soc/amd/cezanne/include/soc/data_fabric.h b/src/soc/amd/cezanne/include/soc/data_fabric.h index 5dcbdd0..13a96e0 100644 --- a/src/soc/amd/cezanne/include/soc/data_fabric.h +++ b/src/soc/amd/cezanne/include/soc/data_fabric.h @@ -5,6 +5,9 @@ #include <types.h> +/* SoC-specific bits in D18F0_MMIO_CTRL0 */ +#define MMIO_NP BIT(16) + #define IOMS0_FABRIC_ID 10 #define NUM_NB_MMIO_REGS 8 diff --git a/src/soc/amd/common/block/include/amdblocks/data_fabric.h b/src/soc/amd/common/block/include/amdblocks/data_fabric.h index 6df778d..ede5075 100644 --- a/src/soc/amd/common/block/include/amdblocks/data_fabric.h +++ b/src/soc/amd/common/block/include/amdblocks/data_fabric.h @@ -15,7 +15,7 @@ #define D18F0_MMIO_LIMIT0 0x204 #define D18F0_MMIO_SHIFT 16 #define D18F0_MMIO_CTRL0 0x208 -#define MMIO_NP BIT(12) +/* The MMIO_NP bit is SoC-specific */ #define MMIO_DST_FABRIC_ID_SHIFT 4 #define MMIO_WE BIT(1) #define MMIO_RE BIT(0) diff --git a/src/soc/amd/picasso/include/soc/data_fabric.h b/src/soc/amd/picasso/include/soc/data_fabric.h index 3a1f2c7..d652d19 100644 --- a/src/soc/amd/picasso/include/soc/data_fabric.h +++ b/src/soc/amd/picasso/include/soc/data_fabric.h @@ -6,6 +6,10 @@ #include <types.h> /* D18F0 - Fabric Configuration registers */ + +/* SoC-specific bits in D18F0_MMIO_CTRL0 */ +#define MMIO_NP BIT(12) + #define IOMS0_FABRIC_ID 9 #define NUM_NB_MMIO_REGS 8 -- To view, visit https://review.coreboot.org/c/coreboot/+/59626 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I64c06b84e2c0737b259077e7932f418306638e19 Gerrit-Change-Number: 59626 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: soc/intel/alderlake: Define the helper functions
by Arthur Heymans (Code Review) 24 Nov '21

24 Nov '21

Attention is currently required from: Tim Wawrzynczak, Sridhar Siricilla, Patrick Rudolph. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59362 ) Change subject: soc/intel/alderlake: Define the helper functions ...................................................................... Patch Set 3: (2 comments) File src/soc/intel/alderlake/cpu.c: https://review.coreboot.org/c/coreboot/+/59362/comment/7ba79235_1ab9b166 PS3, Line 88: TRUE no UEFI types please File src/soc/intel/alderlake/include/soc/cpu.h: https://review.coreboot.org/c/coreboot/+/59362/comment/46275430_2ac1f9dc PS3, Line 34: uint8_t get_cpu_type(void); this belongs in a different CL -- To view, visit https://review.coreboot.org/c/coreboot/+/59362 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I963690a4fadad322095d202bcc08c92dcd845360 Gerrit-Change-Number: 59362 Gerrit-PatchSet: 3 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Sridhar Siricilla <sridhar.siricilla(a)intel.corp-partner.google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Wed, 24 Nov 2021 15:13:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/alderlake, soc/common: Add method to determine the cpu type...
by Arthur Heymans (Code Review) 24 Nov '21

24 Nov '21

Attention is currently required from: Tim Wawrzynczak, Sridhar Siricilla, Arthur Heymans, Patrick Rudolph. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59360 ) Change subject: soc/intel/alderlake, soc/common: Add method to determine the cpu type mask ...................................................................... Patch Set 3: (5 comments) File src/soc/intel/common/block/acpi/Kconfig: https://review.coreboot.org/c/coreboot/+/59360/comment/1f2b6316_c922be04 PS3, Line 50: Generate CPPCv3 entries for Intel hybrid processors That is not what the code does in this CL. File src/soc/intel/common/block/acpi/cpu_hybrid.c: https://review.coreboot.org/c/coreboot/+/59360/comment/9aa2d3ba_11d05fce PS1, Line 25: return global_cpu_type_bitmask; > The bitmask is updated in the multi core context, but accessed by only BSP. So, it's a thread-safe. That's not necesarily threadsafe. You need to make sure the multi core code is done doing this before accessing this variable on the BSP. Where is this checked? File src/soc/intel/common/block/acpi/cpu_hybrid.c: https://review.coreboot.org/c/coreboot/+/59360/comment/0bc75f28_820bf204 PS3, Line 37: set_cpu_type_bitmask Is it really useful to encode this information in a uint32_t? It won't scale past 32 big cores? Why not add the cpu type to struct device? https://review.coreboot.org/c/coreboot/+/59360/comment/a15d897e_fbd45c40 PS3, Line 43: get_cpu_index global_cpu_type_bitmaks will overflow if there are more than 32 CPUs. File src/soc/intel/common/block/include/intelblocks/acpi.h: https://review.coreboot.org/c/coreboot/+/59360/comment/50ed37a6_a0e9e5d4 PS3, Line 11: : #define CPUID_CORE_TYPE_INTEL_ATOM 0x20 : #define CPUID_CORE_TYPE_INTEL_CORE 0x40 Maybe use an enum for this and also in the return value for the functions. -- To view, visit https://review.coreboot.org/c/coreboot/+/59360 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If4ceb24d9bb1e808750bf618c29b2b9ea6d4191b Gerrit-Change-Number: 59360 Gerrit-PatchSet: 3 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Sridhar Siricilla <sridhar.siricilla(a)intel.corp-partner.google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Wed, 24 Nov 2021 15:09:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common: Add CPU related APIs
by Arthur Heymans (Code Review) 24 Nov '21

24 Nov '21

Attention is currently required from: Nico Huber, Maulik V Vaghela, Tim Wawrzynczak, Sugnan Prabhu S, Sridhar Siricilla, Subrata Banik, Patrick Rudolph. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59124 ) Change subject: soc/intel/common: Add CPU related APIs ...................................................................... Patch Set 4: (1 comment) File src/soc/intel/common/block/include/intelblocks/cpulib.h: https://review.coreboot.org/c/coreboot/+/59124/comment/ef0b06e8_8d2ee2d9 PS4, Line 150: : : / drop newline -- To view, visit https://review.coreboot.org/c/coreboot/+/59124 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I680f43952ab4abce6e342206688ad32814970a91 Gerrit-Change-Number: 59124 Gerrit-PatchSet: 4 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Sridhar Siricilla <sridhar.siricilla(a)intel.corp-partner.google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Wed, 24 Nov 2021 15:07:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common: Implement ACPI CPPCv3 package to support hybrid core
by Sridhar Siricilla (Code Review) 24 Nov '21

24 Nov '21

Attention is currently required from: Nico Huber, Paul Menzel, Tim Wawrzynczak, Ravindra, Subrata Banik, Arthur Heymans, Michael Niewöhner, Patrick Rudolph. Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59359 ) Change subject: soc/intel/common: Implement ACPI CPPCv3 package to support hybrid core ...................................................................... Patch Set 3: (15 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/59359/comment/b6d3bdd6_b0e2ff43 PS1, Line 7: Implements > Implement Done https://review.coreboot.org/c/coreboot/+/59359/comment/0bb653a0_3f2b8c34 PS1, Line 11: update > updates Done https://review.coreboot.org/c/coreboot/+/59359/comment/8a3468cc_9dce34a6 PS1, Line 18: generate > generates Done https://review.coreboot.org/c/coreboot/+/59359/comment/7a8263d1_b82c6e4d PS1, Line 20: update > updates Done https://review.coreboot.org/c/coreboot/+/59359/comment/8a36bd6e_f01fa8d3 PS1, Line 24: Cpu > CPU Done https://review.coreboot.org/c/coreboot/+/59359/comment/9cba2a0d_da94dc54 PS1, Line 25: cpu > CPU Ack https://review.coreboot.org/c/coreboot/+/59359/comment/7d070699_2ec4c8fc PS1, Line 25: Nominal > frequency or performance Ack https://review.coreboot.org/c/coreboot/+/59359/comment/ce94754d_73fd2605 PS1, Line 27: It also updates GNVS structure. > It also adds the new members to the GNVS structure, as those are consumed(?) …. Ack https://review.coreboot.org/c/coreboot/+/59359/comment/96f3f719_7fb4ac0a PS1, Line 29: TEST=Verified on Brya > How? Ack https://review.coreboot.org/c/coreboot/+/59359/comment/66ff69e6_65be3a9d PS1, Line 34: methods to generate ACPI code. > ???? Ack https://review.coreboot.org/c/coreboot/+/59359/comment/c0450f13_3a436a93 PS1, Line 36: Change-Id: Icd5ea9e70bebd1e66d3cea2bcf8a6678e5cc95ca > One Change-Id should be enough. Ack File src/soc/intel/common/block/acpi/acpi.c: https://review.coreboot.org/c/coreboot/+/59359/comment/4ab79980_c0e79d14 PS1, Line 376: int > unsigned int? But the signature of `cpu_init_cppc_config()` seems to use `uint32_t`. Ack File src/soc/intel/common/block/acpi/acpi/globalnvs.asl: https://review.coreboot.org/c/coreboot/+/59359/comment/1c4a1dd0_b8c873f3 PS1, Line 28: SFBC, 16, // 0x48 - 0x49 Indicates Scaling factor for Big core : SFSC, 16, // 0x50 - 0x51 Indicates Scaling Factor for Small Core : NMFQ, 16, // 0x52 - 0x53 Indicates Nominal Frequency : CORE, 32, // 0x54 - 0x57 Each marked bit indicates Big Core corresponding to core index : INFS, 8, // 0x58 - Nominal Frequency is supported : > > but with coreboot producing AML at runtime, it is rarely actually required to use GNVS, and we pre […] Correct, I have updated the patch without using GNVS. Thanks! File src/soc/intel/common/block/acpi/cpu_hybrid.c: https://review.coreboot.org/c/coreboot/+/59359/comment/a44b5870_b4b91ca9 PS1, Line 66: pscope = (char *) malloc(12); > You can just allocate these on the stack. No need for malloc. Ack File src/soc/intel/common/block/acpi/cpu_hybrid.c: https://review.coreboot.org/c/coreboot/+/59359/comment/3dd7e7ab_f2aa2ab7 PS3, Line 63: get_cpu_type_bitmask global_cpu_type_bitmask can be accessed by acpi_get_cpu_hybrid_info() directly, so get_cpu_type_bitmask() will be removed in later patch. -- To view, visit https://review.coreboot.org/c/coreboot/+/59359 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icd5ea9e70bebd1e66d3cea2bcf8a6678e5cc95ca Gerrit-Change-Number: 59359 Gerrit-PatchSet: 3 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Ravindra <ravindra(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Sridhar Siricilla <sridhar.siricilla(a)intel.corp-partner.google.com> Gerrit-CC: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Ravindra <ravindra(a)intel.com> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Wed, 24 Nov 2021 14:36:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: src/include/acpi: Move CPPC_PACKAGE_NAME macro definition
by Sridhar Siricilla (Code Review) 24 Nov '21

24 Nov '21

Attention is currently required from: Arthur Heymans, Lance Zhao, Tim Wawrzynczak, Michael Niewöhner. Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59358 ) Change subject: src/include/acpi: Move CPPC_PACKAGE_NAME macro definition ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/59358/comment/d0527b58_9f0d4c94 PS1, Line 9: The patch move CPPC_PACKAGE_NAME macro definition from acpi/acpigen.c : to include/acpi/acpigen.h. > commit messages should say why you do things, not just what you do. Ack -- To view, visit https://review.coreboot.org/c/coreboot/+/59358 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic547445cdbe2b1a3efe44390bd127f577386e7fc Gerrit-Change-Number: 59358 Gerrit-PatchSet: 3 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Sridhar Siricilla <sridhar.siricilla(a)intel.corp-partner.google.com> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Lance Zhao Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Comment-Date: Wed, 24 Nov 2021 14:33:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/alderlake, soc/common: Add method to determine the cpu type...
by Sridhar Siricilla (Code Review) 24 Nov '21

24 Nov '21

Attention is currently required from: Tim Wawrzynczak, Arthur Heymans, Patrick Rudolph. Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59360 ) Change subject: soc/intel/alderlake, soc/common: Add method to determine the cpu type mask ...................................................................... Patch Set 3: (3 comments) File src/soc/intel/common/block/acpi/cpu_hybrid.c: https://review.coreboot.org/c/coreboot/+/59360/comment/da409eec_76296adf PS1, Line 18: get_cpu_type > this is only implemented in the following patch. Please order them correctly... Done, https://review.coreboot.org/c/coreboot/+/59360/comment/c8780350_6b99de90 PS1, Line 23: get_cpu_type_bitmask > Where is this called? It's very hard to know what the purpose of all this is, as it's not explained […] This is implemented in the cpu_hybrid.c. The function is no longer required as the caller and set_cpu_type_bitmask() are moved to cpu_hybrid.c https://review.coreboot.org/c/coreboot/+/59360/comment/9c489824_81a93189 PS1, Line 25: return global_cpu_type_bitmask; > This is not thread safe and probably should only be consume when all AP have run it. The bitmask is updated in the multi core context, but accessed by only BSP. So, it's a thread-safe. -- To view, visit https://review.coreboot.org/c/coreboot/+/59360 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If4ceb24d9bb1e808750bf618c29b2b9ea6d4191b Gerrit-Change-Number: 59360 Gerrit-PatchSet: 3 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Sridhar Siricilla <sridhar.siricilla(a)intel.corp-partner.google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Wed, 24 Nov 2021 14:33:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/brya/var/gimble: Swap TPM I2C with touchscreen I2C
by Mark Hsieh (Code Review) 24 Nov '21

24 Nov '21

Attention is currently required from: YH Lin, Tim Wawrzynczak, Marco Chen, Zhuohao Lee, Felix Held. Mark Hsieh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59580 ) Change subject: mb/google/brya/var/gimble: Swap TPM I2C with touchscreen I2C ...................................................................... Patch Set 5: Code-Review+1 This change is ready for review. -- To view, visit https://review.coreboot.org/c/coreboot/+/59580 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I26d059a7ea5a3fdf00de260214c00d3bba9aa7f7 Gerrit-Change-Number: 59580 Gerrit-PatchSet: 5 Gerrit-Owner: Mark Hsieh <mark_hsieh(a)wistron.corp-partner.google.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Marco Chen <marcochen(a)google.com> Gerrit-Reviewer: Mark Hsieh <mark_hsieh(a)wistron.corp-partner.google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: YH Lin <yueherngl(a)google.com> Gerrit-Reviewer: Zhuohao Lee <zhuohao(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anfernee Chen <anfernee_chen(a)wistron.corp-partner.google.com> Gerrit-CC: Ariel Fang <ariel_fang(a)wistron.corp-partner.google.com> Gerrit-CC: Casper Chang <casper_chang(a)wistron.corp-partner.google.com> Gerrit-CC: Malik Hsu <malik_hsu(a)wistron.corp-partner.google.com> Gerrit-CC: Scott Chao <scott_chao(a)wistron.corp-partner.google.com> Gerrit-CC: Terry Chen <terry_chen(a)wistron.corp-partner.google.com> Gerrit-CC: Will Tsai <will_tsai(a)wistron.corp-partner.google.com> Gerrit-Attention: YH Lin <yueherngl(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Marco Chen <marcochen(a)google.com> Gerrit-Attention: Zhuohao Lee <zhuohao(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Wed, 24 Nov 2021 14:30:03 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit November 2021