Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45778 )
Change subject: soc/intel/cannonlake: Align cosmetics with Ice Lake ......................................................................
soc/intel/cannonlake: Align cosmetics with Ice Lake
Tested with BUILD_TIMELESS=1, Prodrive Hermes remains identical.
Change-Id: I4d9f882f9f8af1245e937b0d47bc7e993547365f Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/cannonlake/acpi/pch_hda.asl M src/soc/intel/cannonlake/cpu.c M src/soc/intel/cannonlake/me.c M src/soc/intel/cannonlake/sd.c 4 files changed, 46 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/45778/1
diff --git a/src/soc/intel/cannonlake/acpi/pch_hda.asl b/src/soc/intel/cannonlake/acpi/pch_hda.asl index 04e974f..f292901 100644 --- a/src/soc/intel/cannonlake/acpi/pch_hda.asl +++ b/src/soc/intel/cannonlake/acpi/pch_hda.asl @@ -4,7 +4,7 @@
Device (HDAS) { - Name (_ADR, 0x001F0003) + Name (_ADR, 0x001f0003) Name (_DDN, "Audio Controller") Name (UUID, ToUUID ("A69F886E-6CEB-4594-A41F-7B5DCE24C553"))
diff --git a/src/soc/intel/cannonlake/cpu.c b/src/soc/intel/cannonlake/cpu.c index 36d252e..0c24535 100644 --- a/src/soc/intel/cannonlake/cpu.c +++ b/src/soc/intel/cannonlake/cpu.c @@ -54,9 +54,10 @@
static void configure_misc(void) { - config_t *conf = config_of_soc(); msr_t msr;
+ config_t *conf = config_of_soc(); + msr = rdmsr(IA32_MISC_ENABLE); msr.lo |= (1 << 0); /* Fast String enable */ msr.lo |= (1 << 3); /* TM1/TM2/EMTTM enable */ @@ -105,6 +106,33 @@ } }
+/* + * The emulated ACPI timer allows replacing of the ACPI timer + * (PM1_TMR) to have no impart on the system. + */ +static void enable_pm_timer_emulation(void) +{ + const struct soc_intel_cannonlake_config *config; + msr_t msr; + + config = config_of_soc(); + + /* Enable PM timer emulation only if ACPI PM timer is disabled */ + if (!config->PmTimerDisabled) + return; + /* + * The derived frequency is calculated as follows: + * (CTC_FREQ * msr[63:32]) >> 32 = target frequency. + * Back solve the multiplier so the 3.579545MHz ACPI timer + * frequency is used. + */ + msr.hi = (3579545ULL << 32) / CTC_FREQ; + /* Set PM1 timer IO port and enable */ + msr.lo = (EMULATE_DELAY_VALUE << EMULATE_DELAY_OFFSET_VALUE) | + EMULATE_PM_TMR_EN | (ACPI_BASE_ADDRESS + PM1_TMR); + wrmsr(MSR_EMULATE_PM_TIMER, msr); +} + static void set_energy_perf_bias(u8 policy) { msr_t msr; @@ -155,33 +183,6 @@ wrmsr(MSR_C_STATE_LATENCY_CONTROL_5, msr); }
-/* - * The emulated ACPI timer allows replacing of the ACPI timer - * (PM1_TMR) to have no impart on the system. - */ -static void enable_pm_timer_emulation(void) -{ - const struct soc_intel_cannonlake_config *config; - msr_t msr; - - config = config_of_soc(); - - /* Enable PM timer emulation only if ACPI PM timer is disabled */ - if (!config->PmTimerDisabled) - return; - /* - * The derived frequency is calculated as follows: - * (CTC_FREQ * msr[63:32]) >> 32 = target frequency. - * Back solve the multiplier so the 3.579545MHz ACPI timer - * frequency is used. - */ - msr.hi = (3579545ULL << 32) / CTC_FREQ; - /* Set PM1 timer IO port and enable */ - msr.lo = (EMULATE_DELAY_VALUE << EMULATE_DELAY_OFFSET_VALUE) | - EMULATE_PM_TMR_EN | (ACPI_BASE_ADDRESS + PM1_TMR); - wrmsr(MSR_EMULATE_PM_TIMER, msr); -} - /* All CPUs including BSP will run the following function. */ void soc_core_init(struct device *cpu) { diff --git a/src/soc/intel/cannonlake/me.c b/src/soc/intel/cannonlake/me.c index a51b702..7bbe1ae 100644 --- a/src/soc/intel/cannonlake/me.c +++ b/src/soc/intel/cannonlake/me.c @@ -12,7 +12,7 @@
/* Host Firmware Status Register 2 */ union me_hfsts2 { - uint32_t raw; + uint32_t data; struct { uint32_t nftp_load_failure : 1; uint32_t icc_prog_status : 2; @@ -36,7 +36,7 @@
/* Host Firmware Status Register 4 */ union me_hfsts4 { - uint32_t raw; + uint32_t data; struct { uint32_t rsvd0 : 9; uint32_t enforcement_flow : 1; @@ -52,7 +52,7 @@
/* Host Firmware Status Register 5 */ union me_hfsts5 { - uint32_t raw; + uint32_t data; struct { uint32_t acm_active : 1; uint32_t valid : 1; @@ -71,7 +71,7 @@
/* Host Firmware Status Register 6 */ union me_hfsts6 { - uint32_t raw; + uint32_t data; struct { uint32_t force_boot_guard_acm : 1; uint32_t cpu_debug_disable : 1; @@ -107,24 +107,18 @@ return;
hfsts1.data = me_read_config32(PCI_ME_HFSTS1); - hfsts2.raw = me_read_config32(PCI_ME_HFSTS2); + hfsts2.data = me_read_config32(PCI_ME_HFSTS2); hfsts3.data = me_read_config32(PCI_ME_HFSTS3); - hfsts4.raw = me_read_config32(PCI_ME_HFSTS4); - hfsts5.raw = me_read_config32(PCI_ME_HFSTS5); - hfsts6.raw = me_read_config32(PCI_ME_HFSTS6); + hfsts4.data = me_read_config32(PCI_ME_HFSTS4); + hfsts5.data = me_read_config32(PCI_ME_HFSTS5); + hfsts6.data = me_read_config32(PCI_ME_HFSTS6);
- printk(BIOS_DEBUG, "ME: HFSTS1 : 0x%08X\n", - hfsts1.data); - printk(BIOS_DEBUG, "ME: HFSTS2 : 0x%08X\n", - hfsts2.raw); - printk(BIOS_DEBUG, "ME: HFSTS3 : 0x%08X\n", - hfsts3.data); - printk(BIOS_DEBUG, "ME: HFSTS4 : 0x%08X\n", - hfsts4.raw); - printk(BIOS_DEBUG, "ME: HFSTS5 : 0x%08X\n", - hfsts5.raw); - printk(BIOS_DEBUG, "ME: HFSTS6 : 0x%08X\n", - hfsts6.raw); + printk(BIOS_DEBUG, "ME: HFSTS1 : 0x%08X\n", hfsts1.data); + printk(BIOS_DEBUG, "ME: HFSTS2 : 0x%08X\n", hfsts2.data); + printk(BIOS_DEBUG, "ME: HFSTS3 : 0x%08X\n", hfsts3.data); + printk(BIOS_DEBUG, "ME: HFSTS4 : 0x%08X\n", hfsts4.data); + printk(BIOS_DEBUG, "ME: HFSTS5 : 0x%08X\n", hfsts5.data); + printk(BIOS_DEBUG, "ME: HFSTS6 : 0x%08X\n", hfsts6.data);
printk(BIOS_DEBUG, "ME: Manufacturing Mode : %s\n", hfsts1.fields.mfg_mode ? "YES" : "NO"); @@ -159,5 +153,6 @@ printk(BIOS_DEBUG, "ME: TXT Support : %s\n", hfsts6.fields.txt_support ? "YES" : "NO"); } + BOOT_STATE_INIT_ENTRY(BS_DEV_ENABLE, BS_ON_EXIT, print_me_fw_version, NULL); BOOT_STATE_INIT_ENTRY(BS_OS_RESUME_CHECK, BS_ON_EXIT, dump_me_status, NULL); diff --git a/src/soc/intel/cannonlake/sd.c b/src/soc/intel/cannonlake/sd.c index 7663b2f..46d3852 100644 --- a/src/soc/intel/cannonlake/sd.c +++ b/src/soc/intel/cannonlake/sd.c @@ -3,7 +3,7 @@ #include <intelblocks/sd.h> #include "chip.h"
-int sd_fill_soc_gpio_info(struct acpi_gpio* gpio, const struct device *dev) +int sd_fill_soc_gpio_info(struct acpi_gpio *gpio, const struct device *dev) { config_t *config = config_of(dev);
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45778 )
Change subject: soc/intel/cannonlake: Align cosmetics with Ice Lake ......................................................................
Patch Set 1: Code-Review+2
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45778 )
Change subject: soc/intel/cannonlake: Align cosmetics with Ice Lake ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45778/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45778/1//COMMIT_MSG@7 PS1, Line 7: soc/intel/cannonlake: Align cosmetics with Ice Lake Even I like the changes, I wouldn't compare with another SoC (Why this SoC?) and I think this doesn't add more useful information. Why not just "Change cosmetics"?
Hello Felix Singer, build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45778
to look at the new patch set (#2).
Change subject: soc/intel/cannonlake: Align cosmetics with Ice Lake ......................................................................
soc/intel/cannonlake: Align cosmetics with Ice Lake
By ironing out cosmetic differences between Cannon Lake and Ice Lake, comparing actual code differences using a diff tool becomes simpler.
Tested with BUILD_TIMELESS=1, Prodrive Hermes remains identical.
Change-Id: I4d9f882f9f8af1245e937b0d47bc7e993547365f Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/cannonlake/acpi/pch_hda.asl M src/soc/intel/cannonlake/cpu.c M src/soc/intel/cannonlake/me.c M src/soc/intel/cannonlake/sd.c 4 files changed, 46 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/45778/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45778 )
Change subject: soc/intel/cannonlake: Align cosmetics with Ice Lake ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45778/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45778/1//COMMIT_MSG@7 PS1, Line 7: soc/intel/cannonlake: Align cosmetics with Ice Lake
Even I like the changes, I wouldn't compare with another SoC (Why this SoC?) and I think this doesn' […]
Good question. This is merely to reduce non-functional differences between both SoCs, which improves the SNR of diff tools. I chose to change cannonlake just because it would not result in a Jenkins complaint on sd.c (about the asterisk).
Gave a reason in the commit message.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45778 )
Change subject: soc/intel/cannonlake: Align cosmetics with Ice Lake ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45778/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45778/1//COMMIT_MSG@7 PS1, Line 7: soc/intel/cannonlake: Align cosmetics with Ice Lake
Good question. […]
Ok, thanks for clarification :)
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45778 )
Change subject: soc/intel/cannonlake: Align cosmetics with Ice Lake ......................................................................
soc/intel/cannonlake: Align cosmetics with Ice Lake
By ironing out cosmetic differences between Cannon Lake and Ice Lake, comparing actual code differences using a diff tool becomes simpler.
Tested with BUILD_TIMELESS=1, Prodrive Hermes remains identical.
Change-Id: I4d9f882f9f8af1245e937b0d47bc7e993547365f Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/45778 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Felix Singer felixsinger@posteo.net --- M src/soc/intel/cannonlake/acpi/pch_hda.asl M src/soc/intel/cannonlake/cpu.c M src/soc/intel/cannonlake/me.c M src/soc/intel/cannonlake/sd.c 4 files changed, 46 insertions(+), 50 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Singer: Looks good to me, approved
diff --git a/src/soc/intel/cannonlake/acpi/pch_hda.asl b/src/soc/intel/cannonlake/acpi/pch_hda.asl index 04e974f..f292901 100644 --- a/src/soc/intel/cannonlake/acpi/pch_hda.asl +++ b/src/soc/intel/cannonlake/acpi/pch_hda.asl @@ -4,7 +4,7 @@
Device (HDAS) { - Name (_ADR, 0x001F0003) + Name (_ADR, 0x001f0003) Name (_DDN, "Audio Controller") Name (UUID, ToUUID ("A69F886E-6CEB-4594-A41F-7B5DCE24C553"))
diff --git a/src/soc/intel/cannonlake/cpu.c b/src/soc/intel/cannonlake/cpu.c index 36d252e..0c24535 100644 --- a/src/soc/intel/cannonlake/cpu.c +++ b/src/soc/intel/cannonlake/cpu.c @@ -54,9 +54,10 @@
static void configure_misc(void) { - config_t *conf = config_of_soc(); msr_t msr;
+ config_t *conf = config_of_soc(); + msr = rdmsr(IA32_MISC_ENABLE); msr.lo |= (1 << 0); /* Fast String enable */ msr.lo |= (1 << 3); /* TM1/TM2/EMTTM enable */ @@ -105,6 +106,33 @@ } }
+/* + * The emulated ACPI timer allows replacing of the ACPI timer + * (PM1_TMR) to have no impart on the system. + */ +static void enable_pm_timer_emulation(void) +{ + const struct soc_intel_cannonlake_config *config; + msr_t msr; + + config = config_of_soc(); + + /* Enable PM timer emulation only if ACPI PM timer is disabled */ + if (!config->PmTimerDisabled) + return; + /* + * The derived frequency is calculated as follows: + * (CTC_FREQ * msr[63:32]) >> 32 = target frequency. + * Back solve the multiplier so the 3.579545MHz ACPI timer + * frequency is used. + */ + msr.hi = (3579545ULL << 32) / CTC_FREQ; + /* Set PM1 timer IO port and enable */ + msr.lo = (EMULATE_DELAY_VALUE << EMULATE_DELAY_OFFSET_VALUE) | + EMULATE_PM_TMR_EN | (ACPI_BASE_ADDRESS + PM1_TMR); + wrmsr(MSR_EMULATE_PM_TIMER, msr); +} + static void set_energy_perf_bias(u8 policy) { msr_t msr; @@ -155,33 +183,6 @@ wrmsr(MSR_C_STATE_LATENCY_CONTROL_5, msr); }
-/* - * The emulated ACPI timer allows replacing of the ACPI timer - * (PM1_TMR) to have no impart on the system. - */ -static void enable_pm_timer_emulation(void) -{ - const struct soc_intel_cannonlake_config *config; - msr_t msr; - - config = config_of_soc(); - - /* Enable PM timer emulation only if ACPI PM timer is disabled */ - if (!config->PmTimerDisabled) - return; - /* - * The derived frequency is calculated as follows: - * (CTC_FREQ * msr[63:32]) >> 32 = target frequency. - * Back solve the multiplier so the 3.579545MHz ACPI timer - * frequency is used. - */ - msr.hi = (3579545ULL << 32) / CTC_FREQ; - /* Set PM1 timer IO port and enable */ - msr.lo = (EMULATE_DELAY_VALUE << EMULATE_DELAY_OFFSET_VALUE) | - EMULATE_PM_TMR_EN | (ACPI_BASE_ADDRESS + PM1_TMR); - wrmsr(MSR_EMULATE_PM_TIMER, msr); -} - /* All CPUs including BSP will run the following function. */ void soc_core_init(struct device *cpu) { diff --git a/src/soc/intel/cannonlake/me.c b/src/soc/intel/cannonlake/me.c index a51b702..7bbe1ae 100644 --- a/src/soc/intel/cannonlake/me.c +++ b/src/soc/intel/cannonlake/me.c @@ -12,7 +12,7 @@
/* Host Firmware Status Register 2 */ union me_hfsts2 { - uint32_t raw; + uint32_t data; struct { uint32_t nftp_load_failure : 1; uint32_t icc_prog_status : 2; @@ -36,7 +36,7 @@
/* Host Firmware Status Register 4 */ union me_hfsts4 { - uint32_t raw; + uint32_t data; struct { uint32_t rsvd0 : 9; uint32_t enforcement_flow : 1; @@ -52,7 +52,7 @@
/* Host Firmware Status Register 5 */ union me_hfsts5 { - uint32_t raw; + uint32_t data; struct { uint32_t acm_active : 1; uint32_t valid : 1; @@ -71,7 +71,7 @@
/* Host Firmware Status Register 6 */ union me_hfsts6 { - uint32_t raw; + uint32_t data; struct { uint32_t force_boot_guard_acm : 1; uint32_t cpu_debug_disable : 1; @@ -107,24 +107,18 @@ return;
hfsts1.data = me_read_config32(PCI_ME_HFSTS1); - hfsts2.raw = me_read_config32(PCI_ME_HFSTS2); + hfsts2.data = me_read_config32(PCI_ME_HFSTS2); hfsts3.data = me_read_config32(PCI_ME_HFSTS3); - hfsts4.raw = me_read_config32(PCI_ME_HFSTS4); - hfsts5.raw = me_read_config32(PCI_ME_HFSTS5); - hfsts6.raw = me_read_config32(PCI_ME_HFSTS6); + hfsts4.data = me_read_config32(PCI_ME_HFSTS4); + hfsts5.data = me_read_config32(PCI_ME_HFSTS5); + hfsts6.data = me_read_config32(PCI_ME_HFSTS6);
- printk(BIOS_DEBUG, "ME: HFSTS1 : 0x%08X\n", - hfsts1.data); - printk(BIOS_DEBUG, "ME: HFSTS2 : 0x%08X\n", - hfsts2.raw); - printk(BIOS_DEBUG, "ME: HFSTS3 : 0x%08X\n", - hfsts3.data); - printk(BIOS_DEBUG, "ME: HFSTS4 : 0x%08X\n", - hfsts4.raw); - printk(BIOS_DEBUG, "ME: HFSTS5 : 0x%08X\n", - hfsts5.raw); - printk(BIOS_DEBUG, "ME: HFSTS6 : 0x%08X\n", - hfsts6.raw); + printk(BIOS_DEBUG, "ME: HFSTS1 : 0x%08X\n", hfsts1.data); + printk(BIOS_DEBUG, "ME: HFSTS2 : 0x%08X\n", hfsts2.data); + printk(BIOS_DEBUG, "ME: HFSTS3 : 0x%08X\n", hfsts3.data); + printk(BIOS_DEBUG, "ME: HFSTS4 : 0x%08X\n", hfsts4.data); + printk(BIOS_DEBUG, "ME: HFSTS5 : 0x%08X\n", hfsts5.data); + printk(BIOS_DEBUG, "ME: HFSTS6 : 0x%08X\n", hfsts6.data);
printk(BIOS_DEBUG, "ME: Manufacturing Mode : %s\n", hfsts1.fields.mfg_mode ? "YES" : "NO"); @@ -159,5 +153,6 @@ printk(BIOS_DEBUG, "ME: TXT Support : %s\n", hfsts6.fields.txt_support ? "YES" : "NO"); } + BOOT_STATE_INIT_ENTRY(BS_DEV_ENABLE, BS_ON_EXIT, print_me_fw_version, NULL); BOOT_STATE_INIT_ENTRY(BS_OS_RESUME_CHECK, BS_ON_EXIT, dump_me_status, NULL); diff --git a/src/soc/intel/cannonlake/sd.c b/src/soc/intel/cannonlake/sd.c index 7663b2f..46d3852 100644 --- a/src/soc/intel/cannonlake/sd.c +++ b/src/soc/intel/cannonlake/sd.c @@ -3,7 +3,7 @@ #include <intelblocks/sd.h> #include "chip.h"
-int sd_fill_soc_gpio_info(struct acpi_gpio* gpio, const struct device *dev) +int sd_fill_soc_gpio_info(struct acpi_gpio *gpio, const struct device *dev) { config_t *config = config_of(dev);
-
Angel Pons (Code Review) -
Felix Singer (Code Review)
-
Nico Huber (Code Review)