Change in coreboot[master]: /soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Hello Jason Glenesk,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/45340
to review the following change.
Change subject: /soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
/soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Add code to generate p-state and c-state SSDT objects to coreboot. Publish objects generated in native coreboot, rather than the ones created by FSP binary.
BUG=b:155307433 TEST=Boot morphius to shell and extract and compare objects created in coreboot with tables generated by FSP. Confirm they are equivalent. BRANCH=Zork
Change-Id: I5f4db3c0c2048ea1d6c6ce55f5e252cb15598514 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/include/cpu/amd/msr.h M src/soc/amd/common/block/include/amdblocks/acpi.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c 5 files changed, 288 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/45340/1
diff --git a/src/include/cpu/amd/msr.h b/src/include/cpu/amd/msr.h index 8bc00d1..710ba43 100644 --- a/src/include/cpu/amd/msr.h +++ b/src/include/cpu/amd/msr.h @@ -12,6 +12,9 @@
#define CPUID_EXT_PM 0x80000007 #define CPUID_MODEL 1 +#define CPUID_EBX_CORE_ID 0x8000001E +#define CPUID_EBX_THREADS_SHIFT 8 +#define CPUID_EBX_THREADS_MASK (0xFF << CPUID_EBX_THREADS_SHIFT) #define MC4_MISC0 0x00000413 #define MC4_MISC1 0xC0000408 #define MC4_MISC2 0xC0000409 @@ -43,7 +46,23 @@ #define PSTATE_2_MSR 0xC0010066 #define PSTATE_3_MSR 0xC0010067 #define PSTATE_4_MSR 0xC0010068 +#define PSTATE_DEF_HI_ENABLE_SHIFT 31 +#define PSTATE_DEF_HI_ENABLE_MASK (0x1 << PSTATE_DEF_HI_ENABLE_SHIFT) +#define PSTATE_DEF_LO_CUR_DIV_SHIFT 30 +#define PSTATE_DEF_LO_CUR_DIV_MASK (0x3 << PSTATE_DEF_LO_CUR_DIV_SHIFT) +#define PSTATE_DEF_LO_CUR_VAL_SHIFT 22 +#define PSTATE_DEF_LO_CUR_VAL_MASK (0xFF << PSTATE_DEF_LO_CUR_VAL_SHIFT) +#define PSTATE_DEF_LO_CORE_VID_SHIFT 14 +#define PSTATE_DEF_LO_CORE_VID_MASK (0xFF << PSTATE_DEF_LO_CORE_VID_SHIFT) +#define PSTATE_DEF_LO_FREQ_DIV_SHIFT 8 +#define PSTATE_DEF_LO_FREQ_DIV_MASK (0x3F << PSTATE_DEF_LO_FREQ_DIV_SHIFT) +#define PSTATE_DEF_LO_FREQ_MIN 0x8 +#define PSTATE_DEF_LO_FREQ_MAX 0x30 +#define PSTATE_DEF_LO_FREQ_MUL_SHIFT 0 +#define PSTATE_DEF_LO_FREQ_MUL_MASK (0xFF << PSTATE_DEF_LO_FREQ_MUL_SHIFT) +#define PSTATE_DEF_LO_CORE_FREQ_BASE 25
+#define CORE_VOLTAGE_INCREMENT_UV 6250 #define MSR_PATCH_LOADER 0xC0010020
#define MSR_COFVID_STS 0xC0010071 diff --git a/src/soc/amd/common/block/include/amdblocks/acpi.h b/src/soc/amd/common/block/include/amdblocks/acpi.h index b6244cb..cef3151 100644 --- a/src/soc/amd/common/block/include/amdblocks/acpi.h +++ b/src/soc/amd/common/block/include/amdblocks/acpi.h @@ -5,6 +5,7 @@
#include <types.h> #include <soc/nvs.h> +#include <cpu/amd/msr.h>
/* ACPI MMIO registers 0xfed80800 */ #define MMIO_ACPI_PM1_STS 0x00 @@ -27,6 +28,14 @@ uint16_t aligning_field; };
+/* P-state structure for each state */ +typedef struct sw_pstate_values { + uint32_t p_state_enable; ///< Pstate enable + uint32_t core_freq; ///< MHz + uint32_t power; ///< milliWatts + uint32_t sw_state_number; ///< Software P-state number +} sw_pstate_values_t; + /* Fill object with the ACPI PM and GPE state. */ void acpi_fill_pm_gpe_state(struct acpi_pm_gpe_state *state); /* Save events to eventlog log and also print information on console. */ @@ -36,6 +45,22 @@ /* Fill GNVS object from PM GPE object. */ void acpi_fill_gnvs(struct global_nvs *gnvs, const struct acpi_pm_gpe_state *state);
+void write_pct_object(void); + +uint32_t get_pstate_core_freq(msr_t pstate_def); + +uint32_t get_pstate_core_power(msr_t pstate_def); + +int get_pstate_info( sw_pstate_values_t *pstate_values, int *max_pstate, uint32_t *thread_count); + +void write_pss_object(int pstate_count, int max_pstate, sw_pstate_values_t* new_pstate_values); + +void write_xpss_object(int pstate_count, int max_pstate, sw_pstate_values_t* new_pstate_values); + +void write_ppc_object(void); + +void write_cst_object(void); + /* * If a system reset is about to be requested, modify the PM1 register so it * will never be misinterpreted as an S3 resume. diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig index 642935f..b198acc 100644 --- a/src/soc/amd/picasso/Kconfig +++ b/src/soc/amd/picasso/Kconfig @@ -338,6 +338,14 @@ Specify the amount of DRAM reserved for gathering the data used to generate the ACPI table.
+config ACPI_SSDT_PSD_INDEPENDENT + bool "Allow core p-state independent transitions" + default y + help + AMD recommends the ACPI _PSD object to be configured to cause + cores to transition between p-states independently. A vendor may + choose to generate _PSD object to allow cores to transition together. + config CHROMEOS select CHROMEOS_RAMOOPS_DYNAMIC select ALWAYS_LOAD_OPROM diff --git a/src/soc/amd/picasso/acpi.c b/src/soc/amd/picasso/acpi.c index 1b9c0ca..a92c7bf 100644 --- a/src/soc/amd/picasso/acpi.c +++ b/src/soc/amd/picasso/acpi.c @@ -26,6 +26,7 @@ #include <soc/gpio.h> #include <version.h> #include "chip.h" +#include <cpu/amd/msr.h>
unsigned long acpi_fill_mcfg(unsigned long current) { @@ -166,20 +167,246 @@ fadt->x_gpe0_blk.addrh = 0x0; }
+void write_pct_object(void) +{ + acpi_addr_t perf_ctrl, perf_sts; + + acpigen_write_name("_PCT"); + acpigen_write_package(0x02); + + memset(&perf_ctrl, 0, sizeof(acpi_addr_t)); + perf_ctrl.space_id = ACPI_ADDRESS_SPACE_FIXED; + perf_ctrl.bit_width = 0x40; + perf_ctrl.addrl = PS_CTL_REG; + acpigen_write_register_resource(&perf_ctrl); + + memset(&perf_sts, 0, sizeof(acpi_addr_t)); + perf_sts.space_id = ACPI_ADDRESS_SPACE_FIXED; + perf_sts.bit_width = 0x40; + acpigen_write_register_resource(&perf_sts); + + acpigen_pop_len(); +} + +uint32_t get_pstate_core_freq(msr_t pstate_def) +{ + uint32_t core_freq, core_freq_mul, core_freq_div; + + /* Core frequency multiplier */ + core_freq_mul = pstate_def.lo & PSTATE_DEF_LO_FREQ_MUL_MASK; + + /* Core frequency divisor ID */ + core_freq_div = (pstate_def.lo & PSTATE_DEF_LO_FREQ_DIV_MASK) + >> PSTATE_DEF_LO_FREQ_DIV_SHIFT; + + if (core_freq_div == 0) { + core_freq = 0; + } else if ((core_freq_div >= PSTATE_DEF_LO_FREQ_MIN) && + (core_freq_div <= PSTATE_DEF_LO_FREQ_MAX)) { + core_freq = ((200 * core_freq_mul) /(core_freq_div)); + } else { + printk (BIOS_WARNING, "Undefined core_freq_div %x used. Force to 1.\n", core_freq_div); + core_freq = (PSTATE_DEF_LO_CORE_FREQ_BASE * core_freq_mul); + } + return core_freq; +} + +uint32_t get_pstate_core_power(msr_t pstate_def) +{ + uint32_t voltage_in_uvolts, core_vid, current_value, current_divisor, power_in_mw; + + /* Core voltage ID */ + core_vid = (pstate_def.lo & PSTATE_DEF_LO_CORE_VID_MASK) + >> PSTATE_DEF_LO_CORE_VID_SHIFT; + + /* Current value in amps */ + current_value = (pstate_def.lo & PSTATE_DEF_LO_CUR_VAL_MASK) + >> PSTATE_DEF_LO_CUR_VAL_SHIFT; + + /* Current divisor */ + current_divisor = (pstate_def.lo & PSTATE_DEF_LO_CUR_DIV_MASK) + >> PSTATE_DEF_LO_CUR_DIV_SHIFT; + + /* Voltage */ + if ((core_vid >= 0xF8) && (core_vid <= 0xFF)) { + /* Voltage off for VID codes 0xF8 to 0xFF */ + voltage_in_uvolts = 0; + } else { + voltage_in_uvolts = 1550000L - (CORE_VOLTAGE_INCREMENT_UV * core_vid); + } + + /* Power in mW */ + power_in_mw = (voltage_in_uvolts) / 10 * current_value; + switch (current_divisor) { + case 0: + power_in_mw= power_in_mw / 100L; + break; + case 1: + power_in_mw = power_in_mw / 1000L; + break; + case 2: + power_in_mw = power_in_mw / 10000L; + break; + default: + /* current_divisor is set to an undefined value.*/ + power_in_mw = 0; + break; + } + return power_in_mw; +} + +int get_pstate_info( sw_pstate_values_t *pstate_values, int *max_pstate, uint32_t *thread_count) +{ + msr_t pstate_def; + int pstate_count, pstate; + + pstate_count = 0; + + *max_pstate = (rdmsr(PS_LIM_REG).lo >> PS_MAX_VAL_SHFT) & 0x3; + + *thread_count = (cpuid_ebx(CPUID_EBX_CORE_ID) & CPUID_EBX_THREADS_MASK) ? 2:1; + + for(pstate = 0; pstate <= *max_pstate; pstate++){ + pstate_def = rdmsr(PSTATE_0_MSR + pstate); + pstate_values[pstate].p_state_enable = (pstate_def.hi & PSTATE_DEF_HI_ENABLE_MASK) + >> PSTATE_DEF_HI_ENABLE_SHIFT; + pstate_values[pstate].core_freq = get_pstate_core_freq(pstate_def); + pstate_values[pstate].power = get_pstate_core_power(pstate_def); + pstate_values[pstate].sw_state_number = pstate; + + if(pstate_values[pstate].p_state_enable){ + pstate_count++; + } + } + + return pstate_count; +} + +void write_pss_object(int pstate_count, int max_pstate, sw_pstate_values_t* pstate_values) +{ + int pstate; + + acpigen_write_name("_PSS"); + acpigen_write_package(pstate_count); + for(pstate = 0; pstate <= max_pstate; pstate++){ + if(pstate_values->p_state_enable){ + acpigen_write_PSS_package(pstate_values->core_freq, + pstate_values->power,0, 0, pstate_values->sw_state_number, + pstate_values->sw_state_number); + } + pstate_values++; + } + acpigen_pop_len(); +} + +void write_xpss_object(int pstate_count, int max_pstate, sw_pstate_values_t* pstate_values) +{ + int pstate; + uint64_t bytes; + + acpigen_write_name("XPSS"); + acpigen_write_package(pstate_count); + for(pstate = 0; pstate <= max_pstate; pstate++){ + acpigen_write_package(0x08); + acpigen_write_dword(pstate_values->core_freq); + acpigen_write_dword( pstate_values->power); + acpigen_write_dword(0); + acpigen_write_dword(0); + + bytes = pstate_values->sw_state_number; + acpigen_write_byte_buffer((uint8_t*)&bytes,8); + acpigen_write_byte_buffer((uint8_t*)&bytes,8); + + bytes=0; + acpigen_write_byte_buffer((uint8_t*)&bytes,8); + acpigen_write_byte_buffer((uint8_t*)&bytes,8); + + acpigen_pop_len(); + pstate_values++; + } + acpigen_pop_len(); +} + +void write_ppc_object(void) +{ + acpigen_write_name("PPCV"); + acpigen_write_byte(0); + + acpigen_write_method("_PPC", 0); + acpigen_emit_byte(RETURN_OP); + acpigen_emit_namestring("PPCV"); + acpigen_pop_len(); +} + +void write_cst_object(void) +{ + acpi_cstate_t cstate_info[3]; + acpi_addr_t cstate_addr; + + cstate_addr.space_id = ACPI_ADDRESS_SPACE_FIXED; + cstate_addr.bit_width = 0x2; + cstate_addr.bit_offset = 0x2; + cstate_addr.access_size = 0; + cstate_addr.addrl = 0; + cstate_addr.addrh = 0; + + cstate_info[0].ctype = 1; + cstate_info[0].latency = 1; + cstate_info[0].power = 0; + cstate_info[0].resource = cstate_addr; + + cstate_addr.space_id = ACPI_ADDRESS_SPACE_IO; + cstate_addr.bit_width = 0x8; + cstate_addr.bit_offset = 0x0; + cstate_addr.access_size = 1; + cstate_addr.addrl = 0x414; + cstate_addr.addrh = 0; + + cstate_info[1].ctype = 2; + cstate_info[1].latency = 400; + cstate_info[1].power = 0; + cstate_info[1].resource = cstate_addr; + acpigen_write_CST_package(cstate_info, 2); +} + void generate_cpu_entries(const struct device *device) { - int cores, cpu; + int logical_cores , pstate_count, max_pstate, cpu; + sw_pstate_values_t pstate_values[7]; + uint32_t thread_count;
- cores = get_cpu_count(); - printk(BIOS_DEBUG, "ACPI \_SB report %d core(s)\n", cores); + pstate_count = get_pstate_info(pstate_values, &max_pstate, &thread_count); + logical_cores = get_cpu_count();
- /* Generate BSP _SB.P000 */ - acpigen_write_processor(0, ACPI_GPE0_BLK, 6); - acpigen_pop_len(); + for (cpu = 0; cpu < logical_cores; cpu++) {
- /* Generate AP _SB.Pxxx */ - for (cpu = 1; cpu < cores; cpu++) { - acpigen_write_processor(cpu, 0, 0); + if(cpu == 0){ + /* Generate BSP _SB.P000 */ + acpigen_write_processor(0, ACPI_GPE0_BLK, 6); + + }else{ + /* Generate AP _SB.Pxxx */ + acpigen_write_processor(cpu, 0, 0); + } + + write_pct_object(); + + write_pss_object(pstate_count, max_pstate, &pstate_values[0]); + + write_xpss_object(pstate_count, max_pstate, &pstate_values[0]); + + if(CONFIG(ACPI_SSDT_PSD_INDEPENDENT)){ + acpigen_write_PSD_package(cpu /thread_count, thread_count, HW_ALL); + }else{ + acpigen_write_PSD_package(0, logical_cores, SW_ALL); + } + + write_ppc_object(); + + write_cst_object(); + + acpigen_write_CSD_package(cpu >> 1,thread_count, HW_ALL,0 ); + acpigen_pop_len(); } } diff --git a/src/soc/amd/picasso/agesa_acpi.c b/src/soc/amd/picasso/agesa_acpi.c index a651d6e..7ddd31e 100644 --- a/src/soc/amd/picasso/agesa_acpi.c +++ b/src/soc/amd/picasso/agesa_acpi.c @@ -508,7 +508,6 @@
printk(BIOS_DEBUG, "Searching for AGESA FSP ACPI Tables\n");
- current = add_agesa_acpi_table(AMD_FSP_ACPI_SSDT_HOB_GUID, "SSDT", rsdp, current); current = add_agesa_acpi_table(AMD_FSP_ACPI_CRAT_HOB_GUID, "CRAT", rsdp, current); current = add_agesa_acpi_table(AMD_FSP_ACPI_ALIB_HOB_GUID, "ALIB", rsdp, current);
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: /soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 1:
(53 comments)
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpi.h:
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/common/block/in... PS1, Line 33: uint32_t p_state_enable; ///< Pstate enable please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/common/block/in... PS1, Line 34: uint32_t core_freq; ///< MHz please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/common/block/in... PS1, Line 35: uint32_t power; ///< milliWatts please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/common/block/in... PS1, Line 36: uint32_t sw_state_number; ///< Software P-state number please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/common/block/in... PS1, Line 54: int get_pstate_info( sw_pstate_values_t *pstate_values, int *max_pstate, uint32_t *thread_count); line over 96 characters
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/common/block/in... PS1, Line 54: int get_pstate_info( sw_pstate_values_t *pstate_values, int *max_pstate, uint32_t *thread_count); space prohibited after that open parenthesis '('
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/common/block/in... PS1, Line 56: void write_pss_object(int pstate_count, int max_pstate, sw_pstate_values_t* new_pstate_values); "foo* bar" should be "foo *bar"
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/common/block/in... PS1, Line 58: void write_xpss_object(int pstate_count, int max_pstate, sw_pstate_values_t* new_pstate_values); "foo* bar" should be "foo *bar"
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 206: core_freq = ((200 * core_freq_mul) /(core_freq_div)); need consistent spacing around '/' (ctx:WxV)
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 208: printk (BIOS_WARNING, "Undefined core_freq_div %x used. Force to 1.\n", core_freq_div); line over 96 characters
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 208: printk (BIOS_WARNING, "Undefined core_freq_div %x used. Force to 1.\n", core_freq_div); space prohibited between function name and open parenthesis '('
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 232: /* Voltage off for VID codes 0xF8 to 0xFF */ trailing whitespace
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 242: power_in_mw= power_in_mw / 100L; spaces required around that '=' (ctx:VxW)
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 258: int get_pstate_info( sw_pstate_values_t *pstate_values, int *max_pstate, uint32_t *thread_count) line over 96 characters
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 258: int get_pstate_info( sw_pstate_values_t *pstate_values, int *max_pstate, uint32_t *thread_count) space prohibited after that open parenthesis '('
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 269: for(pstate = 0; pstate <= *max_pstate; pstate++){ space required before the open brace '{'
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 269: for(pstate = 0; pstate <= *max_pstate; pstate++){ space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 271: pstate_values[pstate].p_state_enable = (pstate_def.hi & PSTATE_DEF_HI_ENABLE_MASK) line over 96 characters
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 277: if(pstate_values[pstate].p_state_enable){ space required before the open brace '{'
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 277: if(pstate_values[pstate].p_state_enable){ space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 277: if(pstate_values[pstate].p_state_enable){ braces {} are not necessary for single statement blocks
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 285: void write_pss_object(int pstate_count, int max_pstate, sw_pstate_values_t* pstate_values) "foo* bar" should be "foo *bar"
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 291: for(pstate = 0; pstate <= max_pstate; pstate++){ space required before the open brace '{'
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 291: for(pstate = 0; pstate <= max_pstate; pstate++){ space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 292: if(pstate_values->p_state_enable){ space required before the open brace '{'
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 292: if(pstate_values->p_state_enable){ space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 294: pstate_values->power,0, 0, pstate_values->sw_state_number, space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 302: void write_xpss_object(int pstate_count, int max_pstate, sw_pstate_values_t* pstate_values) "foo* bar" should be "foo *bar"
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 309: for(pstate = 0; pstate <= max_pstate; pstate++){ space required before the open brace '{'
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 309: for(pstate = 0; pstate <= max_pstate; pstate++){ space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 312: acpigen_write_dword( pstate_values->power); space prohibited after that open parenthesis '('
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 317: acpigen_write_byte_buffer((uint8_t*)&bytes,8); "(foo*)" should be "(foo *)"
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 317: acpigen_write_byte_buffer((uint8_t*)&bytes,8); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 318: acpigen_write_byte_buffer((uint8_t*)&bytes,8); "(foo*)" should be "(foo *)"
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 318: acpigen_write_byte_buffer((uint8_t*)&bytes,8); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 320: bytes=0; spaces required around that '=' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 321: acpigen_write_byte_buffer((uint8_t*)&bytes,8); "(foo*)" should be "(foo *)"
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 321: acpigen_write_byte_buffer((uint8_t*)&bytes,8); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 322: acpigen_write_byte_buffer((uint8_t*)&bytes,8); "(foo*)" should be "(foo *)"
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 322: acpigen_write_byte_buffer((uint8_t*)&bytes,8); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 374: int logical_cores , pstate_count, max_pstate, cpu; space prohibited before that ',' (ctx:WxW)
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 383: if(cpu == 0){ space required before the open brace '{'
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 383: if(cpu == 0){ space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 387: }else{ space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 393: trailing whitespace
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 398: if(CONFIG(ACPI_SSDT_PSD_INDEPENDENT)){ space required before the open brace '{'
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 398: if(CONFIG(ACPI_SSDT_PSD_INDEPENDENT)){ space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 398: if(CONFIG(ACPI_SSDT_PSD_INDEPENDENT)){ braces {} are not necessary for any arm of this statement
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 399: acpigen_write_PSD_package(cpu /thread_count, thread_count, HW_ALL); need consistent spacing around '/' (ctx:WxV)
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 400: }else{ space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 408: acpigen_write_CSD_package(cpu >> 1,thread_count, HW_ALL,0 ); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 408: acpigen_write_CSD_package(cpu >> 1,thread_count, HW_ALL,0 ); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/45340/1/src/soc/amd/picasso/acpi.c@... PS1, Line 408: acpigen_write_CSD_package(cpu >> 1,thread_count, HW_ALL,0 ); space prohibited before that close parenthesis ')'
Hello build bot (Jenkins), Jason Glenesk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45340
to look at the new patch set (#2).
Change subject: /soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
/soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Add code to generate p-state and c-state SSDT objects to coreboot. Publish objects generated in native coreboot, rather than the ones created by FSP binary.
BUG=b:155307433 TEST=Boot morphius to shell and extract and compare objects created in coreboot with tables generated by FSP. Confirm they are equivalent. BRANCH=Zork
Change-Id: I5f4db3c0c2048ea1d6c6ce55f5e252cb15598514 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/include/cpu/amd/msr.h M src/soc/amd/common/block/include/amdblocks/acpi.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c 5 files changed, 289 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/45340/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: /soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45340/2/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/2/src/soc/amd/picasso/acpi.c@... PS2, Line 320: bytes=0; spaces required around that '=' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/45340/2/src/soc/amd/picasso/acpi.c@... PS2, Line 398: if (CONFIG(ACPI_SSDT_PSD_INDEPENDENT)){ space required before the open brace '{'
https://review.coreboot.org/c/coreboot/+/45340/2/src/soc/amd/picasso/acpi.c@... PS2, Line 398: if (CONFIG(ACPI_SSDT_PSD_INDEPENDENT)){ braces {} are not necessary for any arm of this statement
Hello build bot (Jenkins), Jason Glenesk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45340
to look at the new patch set (#3).
Change subject: /soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
/soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Add code to generate p-state and c-state SSDT objects to coreboot. Publish objects generated in native coreboot, rather than the ones created by FSP binary.
BUG=b:155307433 TEST=Boot morphius to shell and extract and compare objects created in coreboot with tables generated by FSP. Confirm they are equivalent. BRANCH=Zork
Change-Id: I5f4db3c0c2048ea1d6c6ce55f5e252cb15598514 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/include/cpu/amd/msr.h M src/soc/amd/common/block/include/amdblocks/acpi.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c 5 files changed, 289 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/45340/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: /soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45340/3/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/3/src/soc/amd/picasso/acpi.c@... PS3, Line 398: if (CONFIG(ACPI_SSDT_PSD_INDEPENDENT)) { braces {} are not necessary for any arm of this statement
Hello build bot (Jenkins), Jason Glenesk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45340
to look at the new patch set (#4).
Change subject: /soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
/soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Add code to generate p-state and c-state SSDT objects to coreboot. Publish objects generated in native coreboot, rather than the ones created by FSP binary.
BUG=b:155307433 TEST=Boot morphius to shell and extract and compare objects created in coreboot with tables generated by FSP. Confirm they are equivalent. BRANCH=Zork
Change-Id: I5f4db3c0c2048ea1d6c6ce55f5e252cb15598514 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/include/cpu/amd/msr.h M src/soc/amd/common/block/include/amdblocks/acpi.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c 5 files changed, 288 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/45340/4
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: /soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 4:
Please help to review this change.
Hello build bot (Jenkins), Furquan Shaikh, Marshall Dawson, Jason Glenesk, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45340
to look at the new patch set (#5).
Change subject: /soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
/soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Add code to generate p-state and c-state SSDT objects to coreboot. Publish objects generated in native coreboot, rather than the ones created by FSP binary.
BUG=b:155307433 TEST=Boot morphius to shell and extract and compare objects created in coreboot with tables generated by FSP. Confirm they are equivalent. BRANCH=Zork
Change-Id: I5f4db3c0c2048ea1d6c6ce55f5e252cb15598514 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/include/cpu/amd/msr.h M src/soc/amd/common/block/include/amdblocks/acpi.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c 5 files changed, 288 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/45340/5
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Marshall Dawson, Jason Glenesk, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45340
to look at the new patch set (#6).
Change subject: /soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
/soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Add code to generate p-state and c-state SSDT objects to coreboot. Publish objects generated in native coreboot, rather than the ones created by FSP binary.
BUG=b:155307433 TEST=Boot morphius to shell and extract and compare objects created in coreboot with tables generated by FSP. Confirm they are equivalent. BRANCH=Zork
Change-Id: I5f4db3c0c2048ea1d6c6ce55f5e252cb15598514 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/include/cpu/amd/msr.h M src/soc/amd/common/block/include/amdblocks/acpi.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c 5 files changed, 288 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/45340/6
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Marshall Dawson, Jason Glenesk, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45340
to look at the new patch set (#7).
Change subject: /soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
/soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Add code to generate p-state and c-state SSDT objects to coreboot. Publish objects generated in native coreboot, rather than the ones created by FSP binary.
BUG=b:155307433 TEST=Boot morphius to shell and extract and compare objects created in coreboot with tables generated by FSP. Confirm they are equivalent. BRANCH=Zork
Change-Id: I5f4db3c0c2048ea1d6c6ce55f5e252cb15598514 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/include/cpu/amd/msr.h M src/soc/amd/common/block/include/amdblocks/acpi.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c 5 files changed, 288 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/45340/7
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: /soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 7:
(5 comments)
https://review.coreboot.org/c/coreboot/+/45340/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45340/7//COMMIT_MSG@7 PS7, Line 7: soc/amd/picasso no leading slash. just the relative path.
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpi.h:
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/common/block/in... PS7, Line 37: } sw_pstate_values_t; Can we please not add typedefs?
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/acpi.c@... PS7, Line 170: void write_pct_object(void) I don't think all these functions need to be global (i.e. outside of the compilation unit). Please keep the symbol leaking to a minimum and only expose what is necessary.
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/acpi.c@... PS7, Line 310: acpigen_write_package(0x08); It seems that the format of the AML could live in helpers while the values would be passed to the library. Or am I not understanding how all of this is specific picasso?
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/acpi.c@... PS7, Line 317: 8 sizeof(bytes)
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: /soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 7:
(7 comments)
https://review.coreboot.org/c/coreboot/+/45340/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45340/5//COMMIT_MSG@7 PS5, Line 7: / You can drop the leading slash
https://review.coreboot.org/c/coreboot/+/45340/5/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/5/src/include/cpu/amd/msr.h@6... PS5, Line 60: 0x30 There's some relevant discussion in https://review.coreboot.org/c/coreboot/+/45055/4/src/soc/amd/picasso/tsc_fre.... I would tend more toward how the register is said to behave, and less toward what its documented max value is.
It might be useful to implement a single function for determining the frequency defined in a p-state register that can be use throughout.
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/Kconfig... PS5, Line 347: Help text is indented by two spaces following the tab.
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/acpi.c@... PS5, Line 343: 3 curious why 3. It looks like we're only doing 2.
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/acpi.c@... PS5, Line 362: 0x414 This should probably be ACPI_CPU_CONTROL.
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/acpi.c@... PS5, Line 376: thread_count nit, this is threads per core and not total thread count, right?
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/acpi.c@... PS5, Line 383: if (cpu == 0) { There would be no harm in making this look like Intel vs. doing the comparison on each loop: https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/common/bloc...
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: /soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 7:
(8 comments)
https://review.coreboot.org/c/coreboot/+/45340/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45340/5//COMMIT_MSG@7 PS5, Line 7: /
You can drop the leading slash
Ack
https://review.coreboot.org/c/coreboot/+/45340/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45340/7//COMMIT_MSG@7 PS7, Line 7: soc/amd/picasso
no leading slash. just the relative path.
Ack
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpi.h:
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/common/block/in... PS7, Line 37: } sw_pstate_values_t;
Can we please not add typedefs?
Ack
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/acpi.c@... PS5, Line 362: 0x414
This should probably be ACPI_CPU_CONTROL.
Doh! I meant to replace this with the right value, vs what i took from the agesa tables. Changing to Core::X86::Msr::CStateBaseAddr[CstateAddr] + 1
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/acpi.c@... PS5, Line 376: thread_count
nit, this is threads per core and not total thread count, right?
Ack
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/acpi.c@... PS7, Line 170: void write_pct_object(void)
I don't think all these functions need to be global (i.e. outside of the compilation unit). […]
Good point. Will make these static.
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/acpi.c@... PS7, Line 310: acpigen_write_package(0x08);
It seems that the format of the AML could live in helpers while the values would be passed to the li […]
That's fair. I will make these more generic and toss them in with the rest of the helpers in acpigen.c
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/acpi.c@... PS7, Line 317: 8
sizeof(bytes)
Ack
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: /soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/Kconfig... PS5, Line 347:
Help text is indented by two spaces following the tab.
Ack
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/acpi.c@... PS5, Line 343: 3
curious why 3. It looks like we're only doing 2.
PPR says that we support up to 3 Cstates, however it only defines values for C1 and C2 and agesa does not have C3 enabled or C3 latency values defined from calculation. Will change to 2, since any change adding the 3rd cstate would also increase the size of the array.
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Marshall Dawson, Jason Glenesk, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45340
to look at the new patch set (#8).
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Add code to generate p-state and c-state SSDT objects to coreboot. Publish objects generated in native coreboot, rather than the ones created by FSP binary.
BUG=b:155307433 TEST=Boot morphius to shell and extract and compare objects created in coreboot with tables generated by FSP. Confirm they are equivalent. BRANCH=Zork
Change-Id: I5f4db3c0c2048ea1d6c6ce55f5e252cb15598514 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c 4 files changed, 229 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/45340/8
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 8:
(12 comments)
https://review.coreboot.org/c/coreboot/+/45340/8/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/8/src/soc/amd/picasso/acpi.c@... PS8, Line 33: static int get_pstate_info(struct acpi_sw_pstate *pstate_values, struct acpi_xpss_sw_pstate *pstate_xpss_values, int *max_pstate, uint32_t *thread_count); line over 96 characters
https://review.coreboot.org/c/coreboot/+/45340/8/src/soc/amd/picasso/acpi.c@... PS8, Line 244: static int get_pstate_info(struct acpi_sw_pstate *pstate_values, struct acpi_xpss_sw_pstate *pstate_xpss_values, int *max_pstate, uint32_t *thread_count) line over 96 characters
https://review.coreboot.org/c/coreboot/+/45340/8/src/soc/amd/picasso/acpi.c@... PS8, Line 260: pstate_values[pstate_count].core_freq = get_pstate_core_freq(pstate_def); line over 96 characters
https://review.coreboot.org/c/coreboot/+/45340/8/src/soc/amd/picasso/acpi.c@... PS8, Line 267: pstate_xpss_values[pstate_count].core_freq = (uint64_t) pstate_values[pstate_count].core_freq; line over 96 characters
https://review.coreboot.org/c/coreboot/+/45340/8/src/soc/amd/picasso/acpi.c@... PS8, Line 268: pstate_xpss_values[pstate_count].power = (uint64_t) pstate_values[pstate_count].power; line over 96 characters
https://review.coreboot.org/c/coreboot/+/45340/8/src/soc/amd/picasso/acpi.c@... PS8, Line 297: cstate_base_address = rdmsr(MSR_CSTATE_ADDRESS ).lo & MSR_CSTATE_ADDRESS_MASK; space prohibited before that close parenthesis ')'
https://review.coreboot.org/c/coreboot/+/45340/8/src/soc/amd/picasso/acpi.c@... PS8, Line 315: cstate_addr.addrl = cstate_base_address +1; need consistent spacing around '+' (ctx:WxV)
https://review.coreboot.org/c/coreboot/+/45340/8/src/soc/amd/picasso/acpi.c@... PS8, Line 335: trailing whitespace
https://review.coreboot.org/c/coreboot/+/45340/8/src/soc/amd/picasso/acpi.c@... PS8, Line 336: pstate_count = get_pstate_info(pstate_values, pstate_xpss_values, &max_pstate, &threads_per_core); line over 96 characters
https://review.coreboot.org/c/coreboot/+/45340/8/src/soc/amd/picasso/acpi.c@... PS8, Line 354: trailing whitespace
https://review.coreboot.org/c/coreboot/+/45340/8/src/soc/amd/picasso/acpi.c@... PS8, Line 356: trailing whitespace
https://review.coreboot.org/c/coreboot/+/45340/8/src/soc/amd/picasso/acpi.c@... PS8, Line 360: acpigen_write_PSD_package(cpu / threads_per_core, threads_per_core, HW_ALL); line over 96 characters
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Marshall Dawson, Jason Glenesk, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45340
to look at the new patch set (#9).
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Add code to generate p-state and c-state SSDT objects to coreboot. Publish objects generated in native coreboot, rather than the ones created by FSP binary.
BUG=b:155307433 TEST=Boot morphius to shell and extract and compare objects created in coreboot with tables generated by FSP. Confirm they are equivalent. BRANCH=Zork
Change-Id: I5f4db3c0c2048ea1d6c6ce55f5e252cb15598514 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c 4 files changed, 238 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/45340/9
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Marshall Dawson, Jason Glenesk, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45340
to look at the new patch set (#10).
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Add code to generate p-state and c-state SSDT objects to coreboot. Publish objects generated in native coreboot, rather than the ones created by FSP binary.
BUG=b:155307433 TEST=Boot morphius to shell and extract and compare objects created in coreboot with tables generated by FSP. Confirm they are equivalent. BRANCH=Zork
Change-Id: I5f4db3c0c2048ea1d6c6ce55f5e252cb15598514 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c 4 files changed, 239 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/45340/10
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 10:
(6 comments)
https://review.coreboot.org/c/coreboot/+/45340/5/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/5/src/include/cpu/amd/msr.h@6... PS5, Line 60: 0x30
There's some relevant discussion in https://review.coreboot. […]
Changing to 0x3e as you recommended in the other review. May have to come back and implement the single function to use across the board in another review.
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/acpi.c@... PS5, Line 343: 3
PPR says that we support up to 3 Cstates, however it only defines values for C1 and C2 and agesa doe […]
Done
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/acpi.c@... PS5, Line 362: 0x414
Doh! I meant to replace this with the right value, vs what i took from the agesa tables. […]
Done
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/acpi.c@... PS5, Line 383: if (cpu == 0) {
There would be no harm in making this look like Intel vs. […]
Done
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/acpi.c@... PS7, Line 170: void write_pct_object(void)
Good point. Will make these static.
Done
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/acpi.c@... PS7, Line 310: acpigen_write_package(0x08);
That's fair. […]
Done
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45340/5/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/5/src/include/cpu/amd/msr.h@6... PS5, Line 60: 0x30
Changing to 0x3e as you recommended in the other review. […]
Done
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 10: Code-Review+1
(9 comments)
ybe
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 14: maybe add another white space.
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 16: maybe 2 whitespaces
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 17: also here ?
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 34: 2 whitespaces?
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 50: #define PSTATE_DEF_HI_ENABLE_SHIFT 31 : #define PSTATE_DEF_HI_ENABLE_MASK (0x1 << PSTATE_DEF_HI_ENABLE_SHIFT) : #define PSTATE_DEF_LO_CUR_DIV_SHIFT 30 : #define PSTATE_DEF_LO_CUR_DIV_MASK (0x3 << PSTATE_DEF_LO_CUR_DIV_SHIFT) : #define PSTATE_DEF_LO_CUR_VAL_SHIFT 22 : #define PSTATE_DEF_LO_CUR_VAL_MASK (0xFF << PSTATE_DEF_LO_CUR_VAL_SHIFT) : #define PSTATE_DEF_LO_CORE_VID_SHIFT 14 : #define PSTATE_DEF_LO_CORE_VID_MASK (0xFF << PSTATE_DEF_LO_CORE_VID_SHIFT) : #define PSTATE_DEF_LO_FREQ_DIV_SHIFT 8 : #define PSTATE_DEF_LO_FREQ_DIV_MASK (0x3F << PSTATE_DEF_LO_FREQ_DIV_SHIFT) : #define PSTATE_DEF_LO_FREQ_DIV_MIN 0x8 : #define PSTATE_DEF_LO_FREQ_DIV_MAX 0x3E : #define PSTATE_DEF_LO_FREQ_MUL_SHIFT 0 : #define PSTATE_DEF_LO_FREQ_MUL_MASK (0xFF << PSTATE_DEF_LO_FREQ_MUL_SHIFT) : #define PSTATE_DEF_LO_CORE_FREQ_BASE 2 whitespaces?
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 66: CORE_VOLTAGE_INCREMENT_UV msr?
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 71: also here ?
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 310: 0 ACPI_ACCESS_SIZE_UNDEFINED
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 322: 1 ACPI_ACCESS_SIZE_BYTE_ACCESS
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 10:
Patch Set 10: Code-Review+1
(9 comments)
Oops, "ybe" means nothing :)) Thx
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 10:
(7 comments)
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 34: 7 Isn't this 16 since the field is 4 bits wide?
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/Kconfig... PS7, Line 345: AMD recommends the ACPI _PSD object to be configured to cause : cores to transition between p-states independently. A vendor may : choose to generate _PSD object to allow cores to transition together. Please align the help text similar to other configs i.e. 1 tab and 2 spaces.
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 31: static uint32_t get_pstate_core_freq(msr_t pstate_def); : static uint32_t get_pstate_core_power(msr_t pstate_def); : static int get_pstate_info(struct acpi_sw_pstate *pstate_values, : struct acpi_xpss_sw_pstate *pstate_xpss_values, int *max_pstate, : uint32_t *thread_count); : static void fill_pct_objects(acpi_addr_t *perf_ctrl, acpi_addr_t *perf_sts); : static void fill_cst_objects(acpi_cstate_t *cstate_info); : Why are these all declared at the top? I don't think that is required.
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 256: 0x3 Define a macro for this. Also, this is 4-bit field.
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 257: : space around :
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 259: for Isn't all of this logic common to AMD platforms? Can this be put under soc/amd/common or some other amd common directory?
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 297: perf_sts->bit_width = 0x40; What about the register address?
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 34: 7
Isn't this 16 since the field is 4 bits wide?
max and current are 3 bits [6:4]maxval [2:0] curpstate in 0xC0010061
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 256: 0x3
Define a macro for this. Also, this is 4-bit field.
This field is 3 bits [6:4]. I can define a macro for a mask and then shift.
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/Kconfig... PS7, Line 345: AMD recommends the ACPI _PSD object to be configured to cause : cores to transition between p-states independently. A vendor may : choose to generate _PSD object to allow cores to transition together.
Please align the help text similar to other configs i.e. 1 tab and 2 spaces.
I didn't know initially. I updated in patch 10.
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 11:
(7 comments)
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 14:
maybe add another white space.
I will submit cleanup in a follow up review and set them all to 2 spaces.
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 16:
maybe 2 whitespaces
Ack
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 17:
also here ?
Ack
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 34:
2 whitespaces?
Ack
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 50: #define PSTATE_DEF_HI_ENABLE_SHIFT 31 : #define PSTATE_DEF_HI_ENABLE_MASK (0x1 << PSTATE_DEF_HI_ENABLE_SHIFT) : #define PSTATE_DEF_LO_CUR_DIV_SHIFT 30 : #define PSTATE_DEF_LO_CUR_DIV_MASK (0x3 << PSTATE_DEF_LO_CUR_DIV_SHIFT) : #define PSTATE_DEF_LO_CUR_VAL_SHIFT 22 : #define PSTATE_DEF_LO_CUR_VAL_MASK (0xFF << PSTATE_DEF_LO_CUR_VAL_SHIFT) : #define PSTATE_DEF_LO_CORE_VID_SHIFT 14 : #define PSTATE_DEF_LO_CORE_VID_MASK (0xFF << PSTATE_DEF_LO_CORE_VID_SHIFT) : #define PSTATE_DEF_LO_FREQ_DIV_SHIFT 8 : #define PSTATE_DEF_LO_FREQ_DIV_MASK (0x3F << PSTATE_DEF_LO_FREQ_DIV_SHIFT) : #define PSTATE_DEF_LO_FREQ_DIV_MIN 0x8 : #define PSTATE_DEF_LO_FREQ_DIV_MAX 0x3E : #define PSTATE_DEF_LO_FREQ_MUL_SHIFT 0 : #define PSTATE_DEF_LO_FREQ_MUL_MASK (0xFF << PSTATE_DEF_LO_FREQ_MUL_SHIFT) : #define PSTATE_DEF_LO_CORE_FREQ_BASE
2 whitespaces?
Ack
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 71:
also here ?
Ack
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/Kconfig... PS7, Line 345: AMD recommends the ACPI _PSD object to be configured to cause : cores to transition between p-states independently. A vendor may : choose to generate _PSD object to allow cores to transition together.
I didn't know initially. I updated in patch 10.
Done
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 11:
(5 comments)
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 14:
I will submit cleanup in a follow up review and set them all to 2 spaces.
Since you need to open this patch up one more time, can we go ahead and get that changed?
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 66: CORE_VOLTAGE_INCREMENT_UV
msr?
No, it's a value to translate between serial VID codes and actual voltage. Jason, maybe go with a different name? But also, I'd add a comment that this is defined in the Serial VID Interface 2.0 spec (#48022, NDA only).
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 31: static uint32_t get_pstate_core_freq(msr_t pstate_def); : static uint32_t get_pstate_core_power(msr_t pstate_def); : static int get_pstate_info(struct acpi_sw_pstate *pstate_values, : struct acpi_xpss_sw_pstate *pstate_xpss_values, int *max_pstate, : uint32_t *thread_count); : static void fill_pct_objects(acpi_addr_t *perf_ctrl, acpi_addr_t *perf_sts); : static void fill_cst_objects(acpi_cstate_t *cstate_info); :
Why are these all declared at the top? I don't think that is required.
I agree. I'd like to see them come out.
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 259: for
Isn't all of this logic common to AMD platforms? Can this be put under soc/amd/common or some other […]
I'm getting ready to throw a temper tantrum! No, I just checked the PPR against the Family 15h BKDG and the registers have been redefined...
It's worth commonizing IMO but we'll want some additional smarts in the source to know what Family we're on (and without needing to wait for cpuid). Maybe that's good prepwork for the next device.
https://review.coreboot.org/c/coreboot/+/45340/11/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/11/src/soc/amd/picasso/acpi.c... PS11, Line 268: latency transition_latency?
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Marshall Dawson, Jason Glenesk, Aaron Durbin, HAOUAS Elyes, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45340
to look at the new patch set (#12).
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Add code to generate p-state and c-state SSDT objects to coreboot. Publish objects generated in native coreboot, rather than the ones created by FSP binary.
BUG=b:155307433 TEST=Boot morphius to shell and extract and compare objects created in coreboot with tables generated by FSP. Confirm they are equivalent. BRANCH=Zork
Change-Id: I5f4db3c0c2048ea1d6c6ce55f5e252cb15598514 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c 4 files changed, 284 insertions(+), 61 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/45340/12
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 12:
(9 comments)
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 14:
Since you need to open this patch up one more time, can we go ahead and get that changed?
Done
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 66: CORE_VOLTAGE_INCREMENT_UV
No, it's a value to translate between serial VID codes and actual voltage. […]
Done
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 31: static uint32_t get_pstate_core_freq(msr_t pstate_def); : static uint32_t get_pstate_core_power(msr_t pstate_def); : static int get_pstate_info(struct acpi_sw_pstate *pstate_values, : struct acpi_xpss_sw_pstate *pstate_xpss_values, int *max_pstate, : uint32_t *thread_count); : static void fill_pct_objects(acpi_addr_t *perf_ctrl, acpi_addr_t *perf_sts); : static void fill_cst_objects(acpi_cstate_t *cstate_info); :
I agree. I'd like to see them come out.
Done
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 256: 0x3
This field is 3 bits [6:4]. I can define a macro for a mask and then shift.
Done
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 257: :
space around :
Done
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 297: perf_sts->bit_width = 0x40;
What about the register address?
Done
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 310: 0
ACPI_ACCESS_SIZE_UNDEFINED
Done
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 322: 1
ACPI_ACCESS_SIZE_BYTE_ACCESS
Done
https://review.coreboot.org/c/coreboot/+/45340/11/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/11/src/soc/amd/picasso/acpi.c... PS11, Line 268: latency
transition_latency?
Done
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Marshall Dawson, Jason Glenesk, Aaron Durbin, HAOUAS Elyes, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45340
to look at the new patch set (#13).
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Add code to generate p-state and c-state SSDT objects to coreboot. Publish objects generated in native coreboot, rather than the ones created by FSP binary.
BUG=b:155307433 TEST=Boot morphius to shell and extract and compare objects created in coreboot with tables generated by FSP. Confirm they are equivalent. BRANCH=Zork
Change-Id: I5f4db3c0c2048ea1d6c6ce55f5e252cb15598514 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c 4 files changed, 284 insertions(+), 61 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/45340/13
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 13:
(14 comments)
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 13: Why the two spaces? If you are going to change the spacing, can you do that in a different CL? It makes it hard to see what actually changed.
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 232: undefined Should we print an error?
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 239: size_t Can you add a comment describing the return value.
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 239: static size_t get_pstate_info(struct acpi_sw_pstate *pstate_values, : struct acpi_xpss_sw_pstate *pstate_xpss_values, uint32_t *max_pstate, : uint32_t *thread_count) : { clang-format
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 293: fill_cst_objects Maybe add a comment that you expect 2 cstate_infos to be passed in. Or you could split the function into two.
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 298: cstate_base_address = rdmsr(MSR_CSTATE_ADDRESS).lo & MSR_CSTATE_ADDRESS_MASK; Move this below the first cstate_info, since it's not used until the second.
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 335: MAX_PSTATES * sizeof(struct acpi_sw_pstate) sizeof(pstate_values), or in the declaration you could do `= {{0}};` and avoid calling memset.
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 338: max_pstate Doesn't look like you ever use this.
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 347: Can you move the proc_blk_len logic into this part of the loop? This way all the logic is right next to each other and I don't have to back trace while reading the code.
if (cpu == 0 ) { /* BSP values for _SB.Pxxx */ proc_blk_len = 6; proc_blk_addr = ACPI_GPE0_BLK; } else { /* AP values for _SB.Pxxx */ proc_blk_addr = 0; proc_blk_len = 0; }
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 355: perf_ctrl Can you declare the variables in the loop and avoid the memset in the function?
acpi_addr_t perf_ctrl = {0}, perf_sts = {0};
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 358: &pstate_values[0] Use `pstate_values`, no need to deref.
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 360: &pstate_xpss_values[0] same
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 370: fill_cst_objects(&cstate_info[0]); Does this always populate the same values? I would pull it out of the loop.
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 370: &cstate_info[0] same
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 15:
(14 comments)
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 13: Please only one space here
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 15: also here, only one space
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 16: nice, 2 spaces. The 2 whitespaces is to "say" that CPUID_EBX_THREADS_SHIFT is to be used with CPUID_EBX_CORE_ID
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 17: great. Thx
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 18: MC4_MISC0 0x00000413 : #define MC4_MISC1 0xC0000408 : #define MC4_MISC2 0xC0000409 : #define FS_Base 0xC0000100 : #define please keep using only 1 space here
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 24: NB_CFG_MSR 0xC001001f : #define FidVidStatus 0xC0010042 : #define MC1_CTL_MASK 0xC0010045 : #define MC4_CTL_MASK 0xC0010048 : #define MSR_INTPEND 0xC0010055 : #define please keep using only one space here
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 33: one space
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 34: 2 white spaces if PS_MAX_VAL_SHIFT is to be used with PS_LIM_REG
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 39: only 1 space
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 45: PS_STS_REG 0xC0010063 : #define PSTATE_0_MSR 0xC0010064 : #define PSTATE_1_MSR 0xC0010065 : #define PSTATE_2_MSR 0xC0010066 : #define PSTATE_3_MSR 0xC0010067 : #define only one space
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 69: MSR_PATCH_LOADER 0xC0010020 : : #define MSR_COFVID_STS 0xC0010071 : #define only 1
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 73: great, thx
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 75: e OSVW_ID_Length 0xC0010140 : #define OSVW_Status 0xC0010141 : : #define SMM_BASE_MSR 0xC0010111 : #define SMM_ADDR_MSR 0xC0010112 : #define SMM_MASK_MSR 0xC0010113 only one space
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 84: fine CPU_ID_FEATURES_MSR 0xC0011004 : #define CPU_ID_EXT_FEATURES_MSR 0xC0011005 : #define CPU_ID_HYPER_EXT_FEATURES 0xC001100d : #define LOGICAL_CPUS_NUM_MSR 0xC001100d : #define LS_CFG_MSR 0xC0011020 : #define IC_CFG_MSR 0xC0011021 : #define DC_CFG_MSR 0xC0011022 : #define BU_CFG_MSR 0xC0011023 : #define FP_CFG_MSR 0xC0011028 : #define DE_CFG_MSR 0xC0011029 : #define BU_CFG2_MSR 0xC001102A : #define BU_CFG3_MSR 0xC001102B : #define EX_CFG_MSR 0xC001102C : #define LS_CFG2_MSR 0xC001102D : #define IBS_OP_DATA3_MSR 0xC0011037 : #define S3_RESUME_EIP_MSR 0xC00110E0 : : #define M only one space
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Marshall Dawson, Jason Glenesk, Aaron Durbin, HAOUAS Elyes, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45340
to look at the new patch set (#16).
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Add code to generate p-state and c-state SSDT objects to coreboot. Publish objects generated in native coreboot, rather than the ones created by FSP binary.
BUG=b:155307433 TEST=Boot morphius to shell and extract and compare objects created in coreboot with tables generated by FSP. Confirm they are equivalent. BRANCH=Zork
Change-Id: I5f4db3c0c2048ea1d6c6ce55f5e252cb15598514 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c 4 files changed, 243 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/45340/16
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 16:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45340/16/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/16/src/soc/amd/picasso/acpi.c... PS16, Line 336: struct acpi_sw_pstate pstate_values[MAX_PSTATES] = {{0}}; space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/45340/16/src/soc/amd/picasso/acpi.c... PS16, Line 337: struct acpi_xpss_sw_pstate pstate_xpss_values[MAX_PSTATES] = {{0}}; space required after that close brace '}'
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 16:
(25 comments)
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 13:
Please only one space here
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 13:
Why the two spaces? If you are going to change the spacing, can you do that in a different CL? It ma […]
I completely misunderstood previous feedback from Haouas. I have fixed now.
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 15:
also here, only one space
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 18: MC4_MISC0 0x00000413 : #define MC4_MISC1 0xC0000408 : #define MC4_MISC2 0xC0000409 : #define FS_Base 0xC0000100 : #define
please keep using only 1 space here
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 24: NB_CFG_MSR 0xC001001f : #define FidVidStatus 0xC0010042 : #define MC1_CTL_MASK 0xC0010045 : #define MC4_CTL_MASK 0xC0010048 : #define MSR_INTPEND 0xC0010055 : #define
please keep using only one space here
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 33:
one space
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 34:
2 white spaces if PS_MAX_VAL_SHIFT is to be used with PS_LIM_REG
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 39:
only 1 space
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 45: PS_STS_REG 0xC0010063 : #define PSTATE_0_MSR 0xC0010064 : #define PSTATE_1_MSR 0xC0010065 : #define PSTATE_2_MSR 0xC0010066 : #define PSTATE_3_MSR 0xC0010067 : #define
only one space
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 69: MSR_PATCH_LOADER 0xC0010020 : : #define MSR_COFVID_STS 0xC0010071 : #define
only 1
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 75: e OSVW_ID_Length 0xC0010140 : #define OSVW_Status 0xC0010141 : : #define SMM_BASE_MSR 0xC0010111 : #define SMM_ADDR_MSR 0xC0010112 : #define SMM_MASK_MSR 0xC0010113
only one space
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 84: fine CPU_ID_FEATURES_MSR 0xC0011004 : #define CPU_ID_EXT_FEATURES_MSR 0xC0011005 : #define CPU_ID_HYPER_EXT_FEATURES 0xC001100d : #define LOGICAL_CPUS_NUM_MSR 0xC001100d : #define LS_CFG_MSR 0xC0011020 : #define IC_CFG_MSR 0xC0011021 : #define DC_CFG_MSR 0xC0011022 : #define BU_CFG_MSR 0xC0011023 : #define FP_CFG_MSR 0xC0011028 : #define DE_CFG_MSR 0xC0011029 : #define BU_CFG2_MSR 0xC001102A : #define BU_CFG3_MSR 0xC001102B : #define EX_CFG_MSR 0xC001102C : #define LS_CFG2_MSR 0xC001102D : #define IBS_OP_DATA3_MSR 0xC0011037 : #define S3_RESUME_EIP_MSR 0xC00110E0 : : #define M
only one space
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 232: undefined
Should we print an error?
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 239: size_t
Can you add a comment describing the return value.
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 239: static size_t get_pstate_info(struct acpi_sw_pstate *pstate_values, : struct acpi_xpss_sw_pstate *pstate_xpss_values, uint32_t *max_pstate, : uint32_t *thread_count) : {
clang-format
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 293: fill_cst_objects
Maybe add a comment that you expect 2 cstate_infos to be passed in. […]
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 298: cstate_base_address = rdmsr(MSR_CSTATE_ADDRESS).lo & MSR_CSTATE_ADDRESS_MASK;
Move this below the first cstate_info, since it's not used until the second.
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 335: MAX_PSTATES * sizeof(struct acpi_sw_pstate)
sizeof(pstate_values), or in the declaration you could do `= {{0}};` and avoid calling memset.
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 338: max_pstate
Doesn't look like you ever use this.
Removed this hold over from earlier versions of the flow.
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 347:
Can you move the proc_blk_len logic into this part of the loop? This way all the logic is right next […]
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 355: perf_ctrl
Can you declare the variables in the loop and avoid the memset in the function? […]
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 358: &pstate_values[0]
Use `pstate_values`, no need to deref.
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 360: &pstate_xpss_values[0]
same
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 370: &cstate_info[0]
same
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 370: fill_cst_objects(&cstate_info[0]);
Does this always populate the same values? I would pull it out of the loop.
Done
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 16: Code-Review+1
(11 comments)
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 13:
Please only one space here
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 15:
also here, only one space
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 18: MC4_MISC0 0x00000413 : #define MC4_MISC1 0xC0000408 : #define MC4_MISC2 0xC0000409 : #define FS_Base 0xC0000100 : #define
please keep using only 1 space here
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 24: NB_CFG_MSR 0xC001001f : #define FidVidStatus 0xC0010042 : #define MC1_CTL_MASK 0xC0010045 : #define MC4_CTL_MASK 0xC0010048 : #define MSR_INTPEND 0xC0010055 : #define
please keep using only one space here
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 33:
one space
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 34:
2 white spaces if PS_MAX_VAL_SHIFT is to be used with PS_LIM_REG
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 39:
only 1 space
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 45: PS_STS_REG 0xC0010063 : #define PSTATE_0_MSR 0xC0010064 : #define PSTATE_1_MSR 0xC0010065 : #define PSTATE_2_MSR 0xC0010066 : #define PSTATE_3_MSR 0xC0010067 : #define
only one space
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 69: MSR_PATCH_LOADER 0xC0010020 : : #define MSR_COFVID_STS 0xC0010071 : #define
only 1
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 75: e OSVW_ID_Length 0xC0010140 : #define OSVW_Status 0xC0010141 : : #define SMM_BASE_MSR 0xC0010111 : #define SMM_ADDR_MSR 0xC0010112 : #define SMM_MASK_MSR 0xC0010113
only one space
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 84: fine CPU_ID_FEATURES_MSR 0xC0011004 : #define CPU_ID_EXT_FEATURES_MSR 0xC0011005 : #define CPU_ID_HYPER_EXT_FEATURES 0xC001100d : #define LOGICAL_CPUS_NUM_MSR 0xC001100d : #define LS_CFG_MSR 0xC0011020 : #define IC_CFG_MSR 0xC0011021 : #define DC_CFG_MSR 0xC0011022 : #define BU_CFG_MSR 0xC0011023 : #define FP_CFG_MSR 0xC0011028 : #define DE_CFG_MSR 0xC0011029 : #define BU_CFG2_MSR 0xC001102A : #define BU_CFG3_MSR 0xC001102B : #define EX_CFG_MSR 0xC001102C : #define LS_CFG2_MSR 0xC001102D : #define IBS_OP_DATA3_MSR 0xC0011037 : #define S3_RESUME_EIP_MSR 0xC00110E0 : : #define M
only one space
Done
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Marshall Dawson, Jason Glenesk, Aaron Durbin, HAOUAS Elyes, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45340
to look at the new patch set (#17).
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Add code to generate p-state and c-state SSDT objects to coreboot. Publish objects generated in native coreboot, rather than the ones created by FSP binary.
BUG=b:155307433 TEST=Boot morphius to shell and extract and compare objects created in coreboot with tables generated by FSP. Confirm they are equivalent. BRANCH=Zork
Change-Id: I5f4db3c0c2048ea1d6c6ce55f5e252cb15598514 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c 4 files changed, 246 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/45340/17
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 17:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45340/17/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/17/src/soc/amd/picasso/acpi.c... PS17, Line 339: struct acpi_sw_pstate pstate_values[MAX_PSTATES] = {{0}} ; space prohibited before semicolon
https://review.coreboot.org/c/coreboot/+/45340/17/src/soc/amd/picasso/acpi.c... PS17, Line 339: struct acpi_sw_pstate pstate_values[MAX_PSTATES] = {{0}} ; space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/45340/17/src/soc/amd/picasso/acpi.c... PS17, Line 340: struct acpi_xpss_sw_pstate pstate_xpss_values[MAX_PSTATES] = {{0}} ; space prohibited before semicolon
https://review.coreboot.org/c/coreboot/+/45340/17/src/soc/amd/picasso/acpi.c... PS17, Line 340: struct acpi_xpss_sw_pstate pstate_xpss_values[MAX_PSTATES] = {{0}} ; space required after that close brace '}'
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Marshall Dawson, Jason Glenesk, Aaron Durbin, HAOUAS Elyes, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45340
to look at the new patch set (#18).
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Add code to generate p-state and c-state SSDT objects to coreboot. Publish objects generated in native coreboot, rather than the ones created by FSP binary.
BUG=b:155307433 TEST=Boot morphius to shell and extract and compare objects created in coreboot with tables generated by FSP. Confirm they are equivalent. BRANCH=Zork
Change-Id: I5f4db3c0c2048ea1d6c6ce55f5e252cb15598514 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c 4 files changed, 246 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/45340/18
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 18: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/45340/18/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/18/src/soc/amd/picasso/acpi.c... PS18, Line 298: } nit: add a new line between the end of the function and the comment.
https://review.coreboot.org/c/coreboot/+/45340/18/src/soc/amd/picasso/acpi.c... PS18, Line 341: cstate_info Should you zero this one out too?
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45340/18/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/18/src/soc/amd/picasso/acpi.c... PS18, Line 341: cstate_info
Should you zero this one out too?
I thought about it, but ultimately decided against. I thought it is useful to see everything being set intentionally to zero, based on the PPR.
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Marshall Dawson, Jason Glenesk, Aaron Durbin, HAOUAS Elyes, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45340
to look at the new patch set (#19).
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Add code to generate p-state and c-state SSDT objects to coreboot. Publish objects generated in native coreboot, rather than the ones created by FSP binary.
BUG=b:155307433 TEST=Boot morphius to shell and extract and compare objects created in coreboot with tables generated by FSP. Confirm they are equivalent. BRANCH=Zork
Change-Id: I5f4db3c0c2048ea1d6c6ce55f5e252cb15598514 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c 4 files changed, 247 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/45340/19
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45340/18/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/18/src/soc/amd/picasso/acpi.c... PS18, Line 298: }
nit: add a new line between the end of the function and the comment.
Done
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 19: Code-Review+2
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45340/18/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/18/src/soc/amd/picasso/acpi.c... PS18, Line 341: cstate_info
I thought about it, but ultimately decided against. […]
Moving to resolved. I see you +2'd after this.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 19:
(19 comments)
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 17: 0xFF This should be 0x3. It is a 2-bit field. (Reference: doc#25481, rev 2.35)
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 37: 7 Looking at the way this is being used, this should be 8. This macro defines the maximum number of P-states that are supported and it is 0-7 i.e. total 8 (at least for family 17h)
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 52: #define PSTATE_DEF_HI_ENABLE_SHIFT 31 : #define PSTATE_DEF_HI_ENABLE_MASK (0x1 << PSTATE_DEF_HI_ENABLE_SHIFT) : #define PSTATE_DEF_LO_CUR_DIV_SHIFT 30 : #define PSTATE_DEF_LO_CUR_DIV_MASK (0x3 << PSTATE_DEF_LO_CUR_DIV_SHIFT) : #define PSTATE_DEF_LO_CUR_VAL_SHIFT 22 : #define PSTATE_DEF_LO_CUR_VAL_MASK (0xFF << PSTATE_DEF_LO_CUR_VAL_SHIFT) : #define PSTATE_DEF_LO_CORE_VID_SHIFT 14 : #define PSTATE_DEF_LO_CORE_VID_MASK (0xFF << PSTATE_DEF_LO_CORE_VID_SHIFT) : #define PSTATE_DEF_LO_FREQ_DIV_SHIFT 8 : #define PSTATE_DEF_LO_FREQ_DIV_MASK (0x3F << PSTATE_DEF_LO_FREQ_DIV_SHIFT) : #define PSTATE_DEF_LO_FREQ_DIV_MIN 0x8 : #define PSTATE_DEF_LO_FREQ_DIV_MAX 0x3E : #define PSTATE_DEF_LO_FREQ_MUL_SHIFT 0 : #define PSTATE_DEF_LO_FREQ_MUL_MASK (0xFF << PSTATE_DEF_LO_FREQ_MUL_SHIFT) : #define PSTATE_DEF_LO_CORE_FREQ_BASE 25 Based on what Marshall mentioned on the other patchset, this seems to be specific to Family 17h. In that case, they should not be added to common cpu/amd/msr.h file and instead moved to soc/amd/picasso/include/msr.h
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 69: SERIAL_VID_DECODE_VALUE probably say _MICROVOLTS to indicate the unit.
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 34: 7
max and current are 3 bits [6:4]maxval [2:0] curpstate in 0xC0010061
I am looking at System Programmer's Manual Volume 2 Figure 17-1 (https://www.amd.com/system/files/TechDocs/24593.pdf) where it says: CurPstateLimite bits 3:0 PstateMaxVal bits 7:4
Or am I looking at the wrong thing?
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 259: for
I'm getting ready to throw a temper tantrum! No, I just checked the PPR against the Family 15h BKDG […]
In that case, the macros being added to common cpu/amd/msr.h is not correct. Those should be moved to soc/amd/picasso/include/soc/msr.h
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 29: #include <cpu/amd/msr.h> nit: Can you please put this above in alphabetic order before cpu/x86/smm.h
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 183: (core_freq_div >= PSTATE_DEF_LO_FREQ_DIV_MIN) : && (core_freq_div <= PSTATE_DEF_LO_FREQ_DIV_MAX I don't think all values between this min and max are really valid. Example: I see 0x1B, 0x1D and a lot others marked as reserved?
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 186: 200 * core_freq_mul) / (core_freq_div I understand that it's the same, but probably better to represent it like:
(PSTATE_DEF_LO_CORE_FREQ_BASE * core_freq_mul) / (core_freq_div / 8);
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 216: voltage_in_uvolts BTW, from PPR 55570 Rev 3.14, section 2.1.6.1.1.3.2, it looks like power is calculated as CpuVid[7:0] * IddValue * 1000. But, here you are looking at SVID decode table to actually calculate the voltage. I believe this is just a miss in the PPR?
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 216: 1550000L I think it would be good to have a macro for this.
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 220: 10 Shouldn't this be 1000 to convert the power to mW?
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 231: default The only other value that divisor can be is 3. PPR does not say anything about it being invalid. Again, just a miss in the PPR?
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 253: *thread_count I don't understand the significance of setting the thread count here. It is not really used in this function. So, why can't this be done within the calling function?
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 253: ? 2 : 1 Shouldn't this be: ((cpuid_ebx(CPUID_EBX_CORE_ID) & CPUID_EBX_THREADS_MASK) >> CPUID_EBX_THREADS_SHIFT) + 1
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 260: if (pstate_enable) nit: If you add a early continue here, then the block below wouldn't have to be aligned an extra tab.
i.e. if (!pstate_enable) continue;
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 342: cstate_info Same here as mentioned below about perf_ctrl and perf_sts:
uint32_t cstate_base_address = rdmsr(MSR_CSTATE_ADDRESS).lo & MSR_CSTATE_ADDRESS_MASK; static const acpi_cstate_t cstate_info[] = { [0] = { .ctype = 1, .latency = 1, .power = 0, .resource = { .space_id = ACPI_ADDRESS_SPACE_FIXED, .bit_width = 2, .bit_offset = 2, .addrl = 0, .addrh = 0, }, }, [1] = { .ctype = 2, .latency = 400, .power = 0, .resource = { .space_id = ACPI_ADDRESS_SPACE_IO, .bit_width = 8, .bit_offset = 0, .addrl = cstate_base_address + 1, .addrh = 0, .access_size = ACPI_ACCESS_SIZE_BYTE_ACCESS, }, }, };
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 343: 0 Instead of setting these to 0, can't we just initialize them right away?
static const acpi_addr_t perf_ctrl = { .space_id = ACPI_ADDRESS_SPACE_FIXED, .bit_width = 64, .addrl = PS_CTL_REG, };
static const acpi_addr_t perf_sts = { .space_id = ACPI_ADDRESS_SPACE_FIXED, .bit_width = 64, .addrl = PS_STS_REG, };
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 381: 2 ARRAY_SIZE(cstate_info)
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 19:
(5 comments)
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 17: 0xFF
This should be 0x3. It is a 2-bit field. (Reference: doc#25481, rev 2. […]
I found this one in the PPR to be [15:8] doc# 55570.
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 37: 7
Looking at the way this is being used, this should be 8. […]
In the PPR, which i used for a lot of this code cur [2:0] in PstateCtrl. I inferred max state being 7 from this.
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 52: #define PSTATE_DEF_HI_ENABLE_SHIFT 31 : #define PSTATE_DEF_HI_ENABLE_MASK (0x1 << PSTATE_DEF_HI_ENABLE_SHIFT) : #define PSTATE_DEF_LO_CUR_DIV_SHIFT 30 : #define PSTATE_DEF_LO_CUR_DIV_MASK (0x3 << PSTATE_DEF_LO_CUR_DIV_SHIFT) : #define PSTATE_DEF_LO_CUR_VAL_SHIFT 22 : #define PSTATE_DEF_LO_CUR_VAL_MASK (0xFF << PSTATE_DEF_LO_CUR_VAL_SHIFT) : #define PSTATE_DEF_LO_CORE_VID_SHIFT 14 : #define PSTATE_DEF_LO_CORE_VID_MASK (0xFF << PSTATE_DEF_LO_CORE_VID_SHIFT) : #define PSTATE_DEF_LO_FREQ_DIV_SHIFT 8 : #define PSTATE_DEF_LO_FREQ_DIV_MASK (0x3F << PSTATE_DEF_LO_FREQ_DIV_SHIFT) : #define PSTATE_DEF_LO_FREQ_DIV_MIN 0x8 : #define PSTATE_DEF_LO_FREQ_DIV_MAX 0x3E : #define PSTATE_DEF_LO_FREQ_MUL_SHIFT 0 : #define PSTATE_DEF_LO_FREQ_MUL_MASK (0xFF << PSTATE_DEF_LO_FREQ_MUL_SHIFT) : #define PSTATE_DEF_LO_CORE_FREQ_BASE 25
Based on what Marshall mentioned on the other patchset, this seems to be specific to Family 17h. […]
Thanks, that makes sense to me, i'll move it over.
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 69: SERIAL_VID_DECODE_VALUE
probably say _MICROVOLTS to indicate the unit.
Sure.
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 34: 7
I am looking at System Programmer's Manual Volume 2 Figure 17-1 (https://www.amd. […]
I think it's just a miss in the doc. The definitions I have been using have been out of the PPR. In https://devhub.amd.com/download/processor-programming-reference-ppr-for-amd-... it has the definition that i used.
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 19:
(10 comments)
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 29: #include <cpu/amd/msr.h>
nit: Can you please put this above in alphabetic order before cpu/x86/smm. […]
NP.
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 216: voltage_in_uvolts
BTW, from PPR 55570 Rev 3.14, section 2.1.6.1.1.3. […]
Agree, they inverted the unit shift direction from micro to mili
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 216: 1550000L
I think it would be good to have a macro for this.
Will do.
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 220: 10
Shouldn't this be 1000 to convert the power to mW?
I can factor in the 10*100. I kept this format as I ported over the way Agesa does the calculation.
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 231: default
The only other value that divisor can be is 3. PPR does not say anything about it being invalid. […]
I believe so. I had to go back to Stoney BKDG to see the values. 0x11 is a reserved value.
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 253: ? 2 : 1
Shouldn't this be: […]
I can change it to that. i took some liberty, knowing each core can only run in 1T or 2T.
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 260: if (pstate_enable)
nit: If you add a early continue here, then the block below wouldn't have to be aligned an extra tab […]
Thanks. I agree that's easier to read :)
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 342: cstate_info
Same here as mentioned below about perf_ctrl and perf_sts: […]
Will do
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 343: 0
Instead of setting these to 0, can't we just initialize them right away? […]
Will do.
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 381: 2
ARRAY_SIZE(cstate_info)
Sure.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 19:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 17: 0xFF
I found this one in the PPR to be [15:8] doc# 55570.
Ack. If this is something that is specific to Family 17h, I think it should go in picasso/include/soc/
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 37: 7
In the PPR, which i used for a lot of this code cur [2:0] in PstateCtrl. […]
Bits 2:0 correspond to states 0 - 7 which makes it a total of 8. This macro is used to decide the total number of entries in pstate array which should be 8.
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 220: 10
I can factor in the 10*100. I kept this format as I ported over the way Agesa does the calculation.
Sorry, I don't understand the comment. If you don't change this to 1000, then, power is not really mW?
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 231: default
I believe so. I had to go back to Stoney BKDG to see the values. 0x11 is a reserved value.
For Family 17h this cannot be 0x11 as this is a 2-bit field.
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Marshall Dawson, Jason Glenesk, Aaron Durbin, HAOUAS Elyes, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45340
to look at the new patch set (#20).
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Add code to generate p-state and c-state SSDT objects to coreboot. Publish objects generated in native coreboot, rather than the ones created by FSP binary.
BUG=b:155307433 TEST=Boot morphius to shell and extract and compare objects created in coreboot with tables generated by FSP. Confirm they are equivalent. BRANCH=Zork
Change-Id: I5f4db3c0c2048ea1d6c6ce55f5e252cb15598514 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c A src/soc/amd/picasso/include/soc/msr.h 5 files changed, 268 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/45340/20
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 20:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45340/20/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/20/src/soc/amd/picasso/acpi.c... PS20, Line 318: { that open brace { should be on the previous line
https://review.coreboot.org/c/coreboot/+/45340/20/src/soc/amd/picasso/acpi.c... PS20, Line 323: { that open brace { should be on the previous line
https://review.coreboot.org/c/coreboot/+/45340/20/src/soc/amd/picasso/acpi.c... PS20, Line 332: { that open brace { should be on the previous line
https://review.coreboot.org/c/coreboot/+/45340/20/src/soc/amd/picasso/acpi.c... PS20, Line 337: { that open brace { should be on the previous line
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 20:
(18 comments)
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 17: 0xFF
Ack. […]
I double checked against 15h BKDG and this is uniform between family 15h and 17h
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 37: 7
Bits 2:0 correspond to states 0 - 7 which makes it a total of 8. […]
Done
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 52: #define PSTATE_DEF_HI_ENABLE_SHIFT 31 : #define PSTATE_DEF_HI_ENABLE_MASK (0x1 << PSTATE_DEF_HI_ENABLE_SHIFT) : #define PSTATE_DEF_LO_CUR_DIV_SHIFT 30 : #define PSTATE_DEF_LO_CUR_DIV_MASK (0x3 << PSTATE_DEF_LO_CUR_DIV_SHIFT) : #define PSTATE_DEF_LO_CUR_VAL_SHIFT 22 : #define PSTATE_DEF_LO_CUR_VAL_MASK (0xFF << PSTATE_DEF_LO_CUR_VAL_SHIFT) : #define PSTATE_DEF_LO_CORE_VID_SHIFT 14 : #define PSTATE_DEF_LO_CORE_VID_MASK (0xFF << PSTATE_DEF_LO_CORE_VID_SHIFT) : #define PSTATE_DEF_LO_FREQ_DIV_SHIFT 8 : #define PSTATE_DEF_LO_FREQ_DIV_MASK (0x3F << PSTATE_DEF_LO_FREQ_DIV_SHIFT) : #define PSTATE_DEF_LO_FREQ_DIV_MIN 0x8 : #define PSTATE_DEF_LO_FREQ_DIV_MAX 0x3E : #define PSTATE_DEF_LO_FREQ_MUL_SHIFT 0 : #define PSTATE_DEF_LO_FREQ_MUL_MASK (0xFF << PSTATE_DEF_LO_FREQ_MUL_SHIFT) : #define PSTATE_DEF_LO_CORE_FREQ_BASE 25
Thanks, that makes sense to me, i'll move it over.
Done
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 69: SERIAL_VID_DECODE_VALUE
Sure.
Done
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 34: 7
I think it's just a miss in the doc. The definitions I have been using have been out of the PPR. […]
Done
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 259: for
In that case, the macros being added to common cpu/amd/msr.h is not correct. […]
Done
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 29: #include <cpu/amd/msr.h>
NP.
Done
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 183: (core_freq_div >= PSTATE_DEF_LO_FREQ_DIV_MIN) : && (core_freq_div <= PSTATE_DEF_LO_FREQ_DIV_MAX
I don't think all values between this min and max are really valid. […]
Done
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 216: 1550000L
Will do.
Done
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 216: voltage_in_uvolts
Agree, they inverted the unit shift direction from micro to mili
Done
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 220: 10
Sorry, I don't understand the comment. […]
I went ahead and moved the 1000 up instead of first dividing by 10 and then by 100, like agesa had it.
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 231: default
For Family 17h this cannot be 0x11 as this is a 2-bit field.
Meant to type 11, rather than 0x11. I've changed the default case to 3.
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 253: *thread_count
I don't understand the significance of setting the thread count here. […]
Done
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 253: ? 2 : 1
I can change it to that. i took some liberty, knowing each core can only run in 1T or 2T.
Done
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 260: if (pstate_enable)
Thanks. […]
Done
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 342: cstate_info
Will do
Done
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 343: 0
Will do.
Done
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 381: 2
Sure.
Done
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Marshall Dawson, Jason Glenesk, Aaron Durbin, HAOUAS Elyes, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45340
to look at the new patch set (#21).
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Add code to generate p-state and c-state SSDT objects to coreboot. Publish objects generated in native coreboot, rather than the ones created by FSP binary.
BUG=b:155307433 TEST=Boot morphius to shell and extract and compare objects created in coreboot with tables generated by FSP. Confirm they are equivalent. BRANCH=Zork
Change-Id: I5f4db3c0c2048ea1d6c6ce55f5e252cb15598514 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c A src/soc/amd/picasso/include/soc/msr.h 5 files changed, 264 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/45340/21
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 21:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45340/21/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/21/src/soc/amd/picasso/acpi.c... PS21, Line 329: [1] = { code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/45340/21/src/soc/amd/picasso/acpi.c... PS21, Line 329: [1] = { please, no space before tabs
https://review.coreboot.org/c/coreboot/+/45340/21/src/soc/amd/picasso/acpi.c... PS21, Line 329: [1] = { please, no spaces at the start of a line
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 186: 200 * core_freq_mul) / (core_freq_div
I understand that it's the same, but probably better to represent it like: […]
In order to do this I would have to use floating arithmetic. Integer division rounding will make the values incorrect. In the Dev Guidelines it prohibits floating arithmetic. Are you ok with leaving this as is? The formula is documented above it in the comment.
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Marshall Dawson, Jason Glenesk, Aaron Durbin, HAOUAS Elyes, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45340
to look at the new patch set (#22).
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Add code to generate p-state and c-state SSDT objects to coreboot. Publish objects generated in native coreboot, rather than the ones created by FSP binary.
BUG=b:155307433 TEST=Boot morphius to shell and extract and compare objects created in coreboot with tables generated by FSP. Confirm they are equivalent. BRANCH=Zork
Change-Id: I5f4db3c0c2048ea1d6c6ce55f5e252cb15598514 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c A src/soc/amd/picasso/include/soc/msr.h 5 files changed, 264 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/45340/22
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 22:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 186: 200 * core_freq_mul) / (core_freq_div
In order to do this I would have to use floating arithmetic. […]
How about: (PSTATE_DEF_LO_CORE_FREQ_BASE * core_freq_mul * 8) / core_freq_div;
https://review.coreboot.org/c/coreboot/+/45340/22/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/22/src/soc/amd/picasso/acpi.c... PS22, Line 184: core_freq Can you just return 0?
https://review.coreboot.org/c/coreboot/+/45340/22/src/soc/amd/picasso/acpi.c... PS22, Line 234: current_value Can you rename this to current_value_amps or something like it. This way it's easier to make sure all the units are correct.
https://review.coreboot.org/c/coreboot/+/45340/22/src/soc/amd/picasso/acpi.c... PS22, Line 305: static Does this need to be static?
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 22:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 186: 200 * core_freq_mul) / (core_freq_div
How about: […]
I can do that. I'll leave the formula comment as well so who ever visits the file later can see that we just pulled the 1/8 out of the denominator.
https://review.coreboot.org/c/coreboot/+/45340/22/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/22/src/soc/amd/picasso/acpi.c... PS22, Line 184: core_freq
Can you just return 0?
Yea
https://review.coreboot.org/c/coreboot/+/45340/22/src/soc/amd/picasso/acpi.c... PS22, Line 234: current_value
Can you rename this to current_value_amps or something like it. […]
Will do.
https://review.coreboot.org/c/coreboot/+/45340/22/src/soc/amd/picasso/acpi.c... PS22, Line 305: static
Does this need to be static?
Definitely not. I was going to define it as a global, but then decided I liked being able to see the values here in the function without scrolling too far. I'll remove the static.
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Marshall Dawson, Jason Glenesk, Aaron Durbin, HAOUAS Elyes, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45340
to look at the new patch set (#23).
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Add code to generate p-state and c-state SSDT objects to coreboot. Publish objects generated in native coreboot, rather than the ones created by FSP binary.
BUG=b:155307433 TEST=Boot morphius to shell and extract and compare objects created in coreboot with tables generated by FSP. Confirm they are equivalent. BRANCH=Zork
Change-Id: I5f4db3c0c2048ea1d6c6ce55f5e252cb15598514 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c A src/soc/amd/picasso/include/soc/msr.h 5 files changed, 263 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/45340/23
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 23:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45340/23/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/23/src/soc/amd/picasso/acpi.c... PS23, Line 198: core_freq = ((PSTATE_DEF_LO_CORE_FREQ_BASE * core_freq_mul * 8) / (core_freq_div)); line over 96 characters
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Marshall Dawson, Jason Glenesk, Aaron Durbin, HAOUAS Elyes, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45340
to look at the new patch set (#24).
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Add code to generate p-state and c-state SSDT objects to coreboot. Publish objects generated in native coreboot, rather than the ones created by FSP binary.
BUG=b:155307433 TEST=Boot morphius to shell and extract and compare objects created in coreboot with tables generated by FSP. Confirm they are equivalent. BRANCH=Zork
Change-Id: I5f4db3c0c2048ea1d6c6ce55f5e252cb15598514 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c A src/soc/amd/picasso/include/soc/msr.h 5 files changed, 264 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/45340/24
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 24:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 186: 200 * core_freq_mul) / (core_freq_div
I can do that. […]
Done
https://review.coreboot.org/c/coreboot/+/45340/22/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/22/src/soc/amd/picasso/acpi.c... PS22, Line 184: core_freq
Yea
Done
https://review.coreboot.org/c/coreboot/+/45340/22/src/soc/amd/picasso/acpi.c... PS22, Line 234: current_value
Will do.
Done
https://review.coreboot.org/c/coreboot/+/45340/22/src/soc/amd/picasso/acpi.c... PS22, Line 305: static
Definitely not. […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 24:
(8 comments)
https://review.coreboot.org/c/coreboot/+/45340/24/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/24/src/include/cpu/amd/msr.h@... PS24, Line 36: #define PS_LIM_MAX_VAL_MASK (0x7 << PS_MAX_VAL_SHFT) : #define MAX_PSTATES 8 Should these also be added to picasso/.../msr.h?
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/acpi.c... PS24, Line 318: .ctype = 1, : .latency = 1, : .power = 0, : .resource = { : .space_id = ACPI_ADDRESS_SPACE_FIXED, : .bit_width = 2, : .bit_offset = 2, : .addrl = 0, : .addrh = 0, : }, : } Needs one less tab.
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/acpi.c... PS24, Line 330: .ctype = 2, : .latency = 400, : .power = 0, : .resource = { : .space_id = ACPI_ADDRESS_SPACE_IO, : .bit_width = 8, : .bit_offset = 0, : .addrl = 0, /* Address will be initialized later : from MSR_CSTATE_ADDRESS */ : .addrh = 0, : .access_size = ACPI_ACCESS_SIZE_BYTE_ACCESS, : }, : }, Same here.
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/acpi.c... PS24, Line 379: cstate_info[1].resource.addrl = : (rdmsr(MSR_CSTATE_ADDRESS).lo & MSR_CSTATE_ADDRESS_MASK) + 1; Why does this need to be done in the loop? I believe it is the same for all CPUs? I think you can do this above before cstate_info: uint32_t cstate_base_address = rdmsr(MSR_CSTATE_ADDRESS).lo & MSR_CSTATE_ADDRESS_MASK;
and use cstate_base_address in cstate_info. That will also allow you to make it const.
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/includ... File src/soc/amd/picasso/include/soc/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/includ... PS24, Line 3: /* This file applies to AMD64 products. : * The definitions come from the AMD64 Programmers Manual vol2 : * Revision 3.30 and/or the device's PPR. This is not really correct. The definitions here are very Family 17h specific and coming from the PPR. That is the reason why they cannot be added to common cpu/amd/msr.h
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/includ... PS24, Line 11: #include <cpu/amd/msr.h> Why is this included here? This is required by picasso/acpi.c and should be included directly in that file.
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/includ... PS24, Line 26: 0x3E Shouldn't this be 0x2C?
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/includ... PS24, Line 32: #define SERIAL_VID_DECODE_MICROVOLTS 6250 : #define SERIAL_VID_MAX_MICROVOLTS 1550000L Is this also Family 17h specific?
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Marshall Dawson, Jason Glenesk, Aaron Durbin, HAOUAS Elyes, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45340
to look at the new patch set (#25).
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Add code to generate p-state and c-state SSDT objects to coreboot. Publish objects generated in native coreboot, rather than the ones created by FSP binary.
BUG=b:155307433 TEST=Boot morphius to shell and extract and compare objects created in coreboot with tables generated by FSP. Confirm they are equivalent. BRANCH=Zork
Change-Id: I5f4db3c0c2048ea1d6c6ce55f5e252cb15598514 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c A src/soc/amd/picasso/include/soc/msr.h 5 files changed, 260 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/45340/25
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 25:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45340/24/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/24/src/include/cpu/amd/msr.h@... PS24, Line 36: #define PS_LIM_MAX_VAL_MASK (0x7 << PS_MAX_VAL_SHFT) : #define MAX_PSTATES 8
Should these also be added to picasso/.../msr. […]
This MSR is consistent between family 15h and 17h for these fields, so i left these here intentionally.
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/acpi.c... PS24, Line 318: .ctype = 1, : .latency = 1, : .power = 0, : .resource = { : .space_id = ACPI_ADDRESS_SPACE_FIXED, : .bit_width = 2, : .bit_offset = 2, : .addrl = 0, : .addrh = 0, : }, : }
Needs one less tab.
I was using clang-format --style=file to help with some of the formatting here. Might need a format rule update?
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/acpi.c... PS24, Line 379: cstate_info[1].resource.addrl = : (rdmsr(MSR_CSTATE_ADDRESS).lo & MSR_CSTATE_ADDRESS_MASK) + 1;
Why does this need to be done in the loop? I believe it is the same for all CPUs? I think you can do […]
Moved out of the loop, but in order to make this const, i will have to modify the helper function. This function is utilized by a ton of other intel files right now. If I go down that road, i recommend doing it in a separate effort. Let me know if that is ok.
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/includ... File src/soc/amd/picasso/include/soc/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/includ... PS24, Line 26: 0x3E
Shouldn't this be 0x2C?
The next reserved ranged starts at 0x3f. After some internal discussion on a different issue, it was decided to accept the widest interpretation of this section.
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 25:
(5 comments)
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/acpi.c... PS24, Line 330: .ctype = 2, : .latency = 400, : .power = 0, : .resource = { : .space_id = ACPI_ADDRESS_SPACE_IO, : .bit_width = 8, : .bit_offset = 0, : .addrl = 0, /* Address will be initialized later : from MSR_CSTATE_ADDRESS */ : .addrh = 0, : .access_size = ACPI_ACCESS_SIZE_BYTE_ACCESS, : }, : },
Same here.
Done
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/acpi.c... PS24, Line 379: cstate_info[1].resource.addrl = : (rdmsr(MSR_CSTATE_ADDRESS).lo & MSR_CSTATE_ADDRESS_MASK) + 1;
Moved out of the loop, but in order to make this const, i will have to modify the helper function. […]
Done
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/includ... File src/soc/amd/picasso/include/soc/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/includ... PS24, Line 3: /* This file applies to AMD64 products. : * The definitions come from the AMD64 Programmers Manual vol2 : * Revision 3.30 and/or the device's PPR.
This is not really correct. […]
Done
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/includ... PS24, Line 11: #include <cpu/amd/msr.h>
Why is this included here? This is required by picasso/acpi. […]
Done
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/includ... PS24, Line 32: #define SERIAL_VID_DECODE_MICROVOLTS 6250 : #define SERIAL_VID_MAX_MICROVOLTS 1550000L
Is this also Family 17h specific?
I moved this back into common
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 25: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/45340/24/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/24/src/include/cpu/amd/msr.h@... PS24, Line 36: #define PS_LIM_MAX_VAL_MASK (0x7 << PS_MAX_VAL_SHFT) : #define MAX_PSTATES 8
This MSR is consistent between family 15h and 17h for these fields, so i left these here intentional […]
But it doesn't look like it's applicable to all AMD families. We can leave it here, but we might have to move this to SoC eventually at some point.
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/acpi.c... PS24, Line 379: cstate_info[1].resource.addrl = : (rdmsr(MSR_CSTATE_ADDRESS).lo & MSR_CSTATE_ADDRESS_MASK) + 1;
Done
SG
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 25: Code-Review+2
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
soc/amd/picasso: Generate ACPI pstate and cstate objects in cb
Add code to generate p-state and c-state SSDT objects to coreboot. Publish objects generated in native coreboot, rather than the ones created by FSP binary.
BUG=b:155307433 TEST=Boot morphius to shell and extract and compare objects created in coreboot with tables generated by FSP. Confirm they are equivalent. BRANCH=Zork
Change-Id: I5f4db3c0c2048ea1d6c6ce55f5e252cb15598514 Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/45340 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Raul Rangel rrangel@chromium.org --- M src/include/cpu/amd/msr.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/acpi.c M src/soc/amd/picasso/agesa_acpi.c A src/soc/amd/picasso/include/soc/msr.h 5 files changed, 260 insertions(+), 13 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Raul Rangel: Looks good to me, approved
diff --git a/src/include/cpu/amd/msr.h b/src/include/cpu/amd/msr.h index 8bc00d1..f9e7b20 100644 --- a/src/include/cpu/amd/msr.h +++ b/src/include/cpu/amd/msr.h @@ -12,13 +12,16 @@
#define CPUID_EXT_PM 0x80000007 #define CPUID_MODEL 1 +#define CPUID_EBX_CORE_ID 0x8000001E +#define CPUID_EBX_THREADS_SHIFT 8 +#define CPUID_EBX_THREADS_MASK (0xFF << CPUID_EBX_THREADS_SHIFT) #define MC4_MISC0 0x00000413 #define MC4_MISC1 0xC0000408 #define MC4_MISC2 0xC0000409 #define FS_Base 0xC0000100 -#define HWCR_MSR 0xC0010015 +#define HWCR_MSR 0xC0010015 #define SMM_LOCK (1 << 0) -#define NB_CFG_MSR 0xC001001f +#define NB_CFG_MSR 0xC001001f #define FidVidStatus 0xC0010042 #define MC1_CTL_MASK 0xC0010045 #define MC4_CTL_MASK 0xC0010048 @@ -30,6 +33,9 @@ #define PS_LIM_REG 0xC0010061 /* P-state Maximum Value shift position */ #define PS_MAX_VAL_SHFT 4 +#define PS_LIM_MAX_VAL_MASK (0x7 << PS_MAX_VAL_SHFT) +#define MAX_PSTATES 8 + /* P-state Control Register */ #define PS_CTL_REG 0xC0010062 /* P-state Control Register CMD Mask OFF */ @@ -43,11 +49,15 @@ #define PSTATE_2_MSR 0xC0010066 #define PSTATE_3_MSR 0xC0010067 #define PSTATE_4_MSR 0xC0010068 - +/* Value defined in Serial VID Interface 2.0 spec (#48022, NDA only) */ +#define SERIAL_VID_DECODE_MICROVOLTS 6250 +#define SERIAL_VID_MAX_MICROVOLTS 1550000L #define MSR_PATCH_LOADER 0xC0010020
#define MSR_COFVID_STS 0xC0010071 #define MSR_CSTATE_ADDRESS 0xC0010073 +#define MSR_CSTATE_ADDRESS_MASK 0xFFFF + #define OSVW_ID_Length 0xC0010140 #define OSVW_Status 0xC0010141
diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig index 824b1b0..849e27b 100644 --- a/src/soc/amd/picasso/Kconfig +++ b/src/soc/amd/picasso/Kconfig @@ -354,6 +354,14 @@ Specify the amount of DRAM reserved for gathering the data used to generate the ACPI table.
+config ACPI_SSDT_PSD_INDEPENDENT + bool "Allow core p-state independent transitions" + default y + help + AMD recommends the ACPI _PSD object to be configured to cause + cores to transition between p-states independently. A vendor may + choose to generate _PSD object to allow cores to transition together. + config CHROMEOS select CHROMEOS_RAMOOPS_DYNAMIC select ALWAYS_LOAD_OPROM diff --git a/src/soc/amd/picasso/acpi.c b/src/soc/amd/picasso/acpi.c index 2c04295..17940e9 100644 --- a/src/soc/amd/picasso/acpi.c +++ b/src/soc/amd/picasso/acpi.c @@ -12,6 +12,7 @@ #include <device/pci_ops.h> #include <arch/ioapic.h> #include <arch/smp/mpspec.h> +#include <cpu/amd/msr.h> #include <cpu/x86/smm.h> #include <cbmem.h> #include <device/device.h> @@ -21,6 +22,7 @@ #include <soc/acpi.h> #include <soc/pci_devs.h> #include <soc/cpu.h> +#include <soc/msr.h> #include <soc/southbridge.h> #include <soc/nvs.h> #include <soc/gpio.h> @@ -166,20 +168,220 @@ fadt->x_gpe0_blk.addrh = 0x0; }
+static uint32_t get_pstate_core_freq(msr_t pstate_def) +{ + uint32_t core_freq, core_freq_mul, core_freq_div; + bool valid_freq_divisor; + + /* Core frequency multiplier */ + core_freq_mul = pstate_def.lo & PSTATE_DEF_LO_FREQ_MUL_MASK; + + /* Core frequency divisor ID */ + core_freq_div = + (pstate_def.lo & PSTATE_DEF_LO_FREQ_DIV_MASK) >> PSTATE_DEF_LO_FREQ_DIV_SHIFT; + + if (core_freq_div == 0) { + return 0; + } else if ((core_freq_div >= PSTATE_DEF_LO_FREQ_DIV_MIN) + && (core_freq_div <= PSTATE_DEF_LO_EIGHTH_STEP_MAX)) { + /* Allow 1/8 integer steps for this range */ + valid_freq_divisor = 1; + } else if ((core_freq_div > PSTATE_DEF_LO_EIGHTH_STEP_MAX) + && (core_freq_div <= PSTATE_DEF_LO_FREQ_DIV_MAX) && !(core_freq_div & 0x1)) { + /* Only allow 1/4 integer steps for this range */ + valid_freq_divisor = 1; + } else { + valid_freq_divisor = 0; + } + + if (valid_freq_divisor) { + /* 25 * core_freq_mul / (core_freq_div / 8) */ + core_freq = + ((PSTATE_DEF_LO_CORE_FREQ_BASE * core_freq_mul * 8) / (core_freq_div)); + } else { + printk(BIOS_WARNING, "Undefined core_freq_div %x used. Force to 1.\n", + core_freq_div); + core_freq = (PSTATE_DEF_LO_CORE_FREQ_BASE * core_freq_mul); + } + return core_freq; +} + +static uint32_t get_pstate_core_power(msr_t pstate_def) +{ + uint32_t voltage_in_uvolts, core_vid, current_value_amps, current_divisor, power_in_mw; + + /* Core voltage ID */ + core_vid = + (pstate_def.lo & PSTATE_DEF_LO_CORE_VID_MASK) >> PSTATE_DEF_LO_CORE_VID_SHIFT; + + /* Current value in amps */ + current_value_amps = + (pstate_def.lo & PSTATE_DEF_LO_CUR_VAL_MASK) >> PSTATE_DEF_LO_CUR_VAL_SHIFT; + + /* Current divisor */ + current_divisor = + (pstate_def.lo & PSTATE_DEF_LO_CUR_DIV_MASK) >> PSTATE_DEF_LO_CUR_DIV_SHIFT; + + /* Voltage */ + if ((core_vid >= 0xF8) && (core_vid <= 0xFF)) { + /* Voltage off for VID codes 0xF8 to 0xFF */ + voltage_in_uvolts = 0; + } else { + voltage_in_uvolts = + SERIAL_VID_MAX_MICROVOLTS - (SERIAL_VID_DECODE_MICROVOLTS * core_vid); + } + + /* Power in mW */ + power_in_mw = (voltage_in_uvolts) / 1000 * current_value_amps; + + switch (current_divisor) { + case 0: + break; + case 1: + power_in_mw = power_in_mw / 10L; + break; + case 2: + power_in_mw = power_in_mw / 100L; + break; + case 3: + /* current_divisor is set to an undefined value.*/ + printk(BIOS_WARNING, "Undefined current_divisor set for enabled P-state .\n"); + power_in_mw = 0; + break; + } + + return power_in_mw; +} + +/* + * Populate structure describing enabled p-states and return count of enabled p-states. + */ +static size_t get_pstate_info(struct acpi_sw_pstate *pstate_values, + struct acpi_xpss_sw_pstate *pstate_xpss_values) +{ + msr_t pstate_def; + size_t pstate_count, pstate; + uint32_t pstate_enable, max_pstate; + + pstate_count = 0; + max_pstate = (rdmsr(PS_LIM_REG).lo & PS_LIM_MAX_VAL_MASK) >> PS_MAX_VAL_SHFT; + + for (pstate = 0; pstate <= max_pstate; pstate++) { + pstate_def = rdmsr(PSTATE_0_MSR + pstate); + + pstate_enable = (pstate_def.hi & PSTATE_DEF_HI_ENABLE_MASK) + >> PSTATE_DEF_HI_ENABLE_SHIFT; + if (!pstate_enable) + continue; + + pstate_values[pstate_count].core_freq = get_pstate_core_freq(pstate_def); + pstate_values[pstate_count].power = get_pstate_core_power(pstate_def); + pstate_values[pstate_count].transition_latency = 0; + pstate_values[pstate_count].bus_master_latency = 0; + pstate_values[pstate_count].control_value = pstate; + pstate_values[pstate_count].status_value = pstate; + + pstate_xpss_values[pstate_count].core_freq = + (uint64_t)pstate_values[pstate_count].core_freq; + pstate_xpss_values[pstate_count].power = + (uint64_t)pstate_values[pstate_count].power; + pstate_xpss_values[pstate_count].transition_latency = 0; + pstate_xpss_values[pstate_count].bus_master_latency = 0; + pstate_xpss_values[pstate_count].control_value = (uint64_t)pstate; + pstate_xpss_values[pstate_count].status_value = (uint64_t)pstate; + pstate_count++; + } + + return pstate_count; +} + void generate_cpu_entries(const struct device *device) { - int cores, cpu; + int logical_cores; + size_t pstate_count, cpu, proc_blk_len; + struct acpi_sw_pstate pstate_values[MAX_PSTATES] = { {0} }; + struct acpi_xpss_sw_pstate pstate_xpss_values[MAX_PSTATES] = { {0} }; + uint32_t threads_per_core, proc_blk_addr; + uint32_t cstate_base_address = + rdmsr(MSR_CSTATE_ADDRESS).lo & MSR_CSTATE_ADDRESS_MASK;
- cores = get_cpu_count(); - printk(BIOS_DEBUG, "ACPI \_SB report %d core(s)\n", cores); + const acpi_addr_t perf_ctrl = { + .space_id = ACPI_ADDRESS_SPACE_FIXED, + .bit_width = 64, + .addrl = PS_CTL_REG, + }; + const acpi_addr_t perf_sts = { + .space_id = ACPI_ADDRESS_SPACE_FIXED, + .bit_width = 64, + .addrl = PS_STS_REG, + };
- /* Generate BSP _SB.P000 */ - acpigen_write_processor(0, ACPI_GPE0_BLK, 6); - acpigen_pop_len(); + acpi_cstate_t cstate_info[] = { + [0] = { + .ctype = 1, + .latency = 1, + .power = 0, + .resource = { + .space_id = ACPI_ADDRESS_SPACE_FIXED, + .bit_width = 2, + .bit_offset = 2, + .addrl = 0, + .addrh = 0, + }, + }, + [1] = { + .ctype = 2, + .latency = 400, + .power = 0, + .resource = { + .space_id = ACPI_ADDRESS_SPACE_IO, + .bit_width = 8, + .bit_offset = 0, + .addrl = cstate_base_address + 1, + .addrh = 0, + .access_size = ACPI_ACCESS_SIZE_BYTE_ACCESS, + }, + }, + };
- /* Generate AP _SB.Pxxx */ - for (cpu = 1; cpu < cores; cpu++) { - acpigen_write_processor(cpu, 0, 0); + threads_per_core = ((cpuid_ebx(CPUID_EBX_CORE_ID) & CPUID_EBX_THREADS_MASK) + >> CPUID_EBX_THREADS_SHIFT) + + 1; + pstate_count = get_pstate_info(pstate_values, pstate_xpss_values); + logical_cores = get_cpu_count(); + + for (cpu = 0; cpu < logical_cores; cpu++) { + + if (cpu == 0) { + /* BSP values for _SB.Pxxx */ + proc_blk_len = 6; + proc_blk_addr = ACPI_GPE0_BLK; + } else { + /* AP values for _SB.Pxxx */ + proc_blk_addr = 0; + proc_blk_len = 0; + } + + acpigen_write_processor(cpu, proc_blk_addr, proc_blk_len); + + acpigen_write_pct_package(&perf_ctrl, &perf_sts); + + acpigen_write_pss_object(pstate_values, pstate_count); + + acpigen_write_xpss_object(pstate_xpss_values, pstate_count); + + if (CONFIG(ACPI_SSDT_PSD_INDEPENDENT)) + acpigen_write_PSD_package(cpu / threads_per_core, threads_per_core, + HW_ALL); + else + acpigen_write_PSD_package(0, logical_cores, SW_ALL); + + acpigen_write_PPC(0); + + acpigen_write_CST_package(cstate_info, ARRAY_SIZE(cstate_info)); + + acpigen_write_CSD_package(cpu >> 1, threads_per_core, HW_ALL, 0); + acpigen_pop_len(); } } diff --git a/src/soc/amd/picasso/agesa_acpi.c b/src/soc/amd/picasso/agesa_acpi.c index c2ff81d..ec9924b 100644 --- a/src/soc/amd/picasso/agesa_acpi.c +++ b/src/soc/amd/picasso/agesa_acpi.c @@ -500,7 +500,6 @@
printk(BIOS_DEBUG, "Searching for AGESA FSP ACPI Tables\n");
- current = add_agesa_acpi_table(AMD_FSP_ACPI_SSDT_HOB_GUID, "SSDT", rsdp, current); current = add_agesa_acpi_table(AMD_FSP_ACPI_CRAT_HOB_GUID, "CRAT", rsdp, current); current = add_agesa_acpi_table(AMD_FSP_ACPI_ALIB_HOB_GUID, "ALIB", rsdp, current);
diff --git a/src/soc/amd/picasso/include/soc/msr.h b/src/soc/amd/picasso/include/soc/msr.h new file mode 100644 index 0000000..0743ba0 --- /dev/null +++ b/src/soc/amd/picasso/include/soc/msr.h @@ -0,0 +1,28 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +/* This file applies to AMD64 products. + * The definitions come from the device's PPR. + */ + +#ifndef SOC_AMD_PICASSO_MSR_H +#define SOC_AMD_PICASSO_MSR_H + +/* MSRC001_00[6B:64] P-state [7:0] bit definitions */ +#define PSTATE_DEF_HI_ENABLE_SHIFT 31 +#define PSTATE_DEF_HI_ENABLE_MASK (0x1 << PSTATE_DEF_HI_ENABLE_SHIFT) +#define PSTATE_DEF_LO_CUR_DIV_SHIFT 30 +#define PSTATE_DEF_LO_CUR_DIV_MASK (0x3 << PSTATE_DEF_LO_CUR_DIV_SHIFT) +#define PSTATE_DEF_LO_CUR_VAL_SHIFT 22 +#define PSTATE_DEF_LO_CUR_VAL_MASK (0xFF << PSTATE_DEF_LO_CUR_VAL_SHIFT) +#define PSTATE_DEF_LO_CORE_VID_SHIFT 14 +#define PSTATE_DEF_LO_CORE_VID_MASK (0xFF << PSTATE_DEF_LO_CORE_VID_SHIFT) +#define PSTATE_DEF_LO_FREQ_DIV_SHIFT 8 +#define PSTATE_DEF_LO_FREQ_DIV_MASK (0x3F << PSTATE_DEF_LO_FREQ_DIV_SHIFT) +#define PSTATE_DEF_LO_FREQ_DIV_MIN 0x8 +#define PSTATE_DEF_LO_EIGHTH_STEP_MAX 0x1A +#define PSTATE_DEF_LO_FREQ_DIV_MAX 0x3E +#define PSTATE_DEF_LO_FREQ_MUL_SHIFT 0 +#define PSTATE_DEF_LO_FREQ_MUL_MASK (0xFF << PSTATE_DEF_LO_FREQ_MUL_SHIFT) +#define PSTATE_DEF_LO_CORE_FREQ_BASE 25 + +#endif /* SOC_AMD_PICASSO_MSR_H */
-
Aaron Durbin (Code Review) -
build bot (Jenkins) (Code Review)
-
Furquan Shaikh (Code Review)
-
HAOUAS Elyes (Code Review)
-
Jason Glenesk (Code Review)
-
Marshall Dawson (Code Review)
-
Raul Rangel (Code Review)