coreboot-gerrit February 2022

coreboot-gerrit@coreboot.org

1 participants
7066 discussions

Change in coreboot[master]: cpu/x86/lapic: Support switching to X2APIC mode
by Kyösti Mälkki (Code Review) 05 Feb '22

05 Feb '22

Kyösti Mälkki has submitted this change. ( https://review.coreboot.org/c/coreboot/+/55262 ) Change subject: cpu/x86/lapic: Support switching to X2APIC mode ...................................................................... cpu/x86/lapic: Support switching to X2APIC mode The options X2APIC_ONLY and X2APIC_RUNTIME were already user-visible choices in menuconfig, but the functionality was not actually provided except for platforms where FSP presumably enabled X2APIC. Add the logic and related logging for switching to X2APIC operation. TEST: qemu-system-x86_64 -M Q35 -accel kvm -bios coreboot.rom -serial stdio -smp 2 PARALLEL_MP, and either X2APIC_ONLY or X2APIC_RUNTIME, need to be selected for the build of emulation/qemu-q35. Change-Id: I19a990ba287d21ccddaa64601923f1c4830e95e9 Signed-off-by: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/55262 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Arthur Heymans <arthur(a)aheymans.xyz> Reviewed-by: Wonkyu Kim <wonkyu.kim(a)intel.com> --- M src/cpu/x86/lapic/lapic.c M src/cpu/x86/mp_init.c M src/include/cpu/x86/lapic.h M src/include/cpu/x86/msr.h 4 files changed, 39 insertions(+), 11 deletions(-) Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved Wonkyu Kim: Looks good to me, but someone else must approve diff --git a/src/cpu/x86/lapic/lapic.c b/src/cpu/x86/lapic/lapic.c index b4d3c4d..9003534 100644 --- a/src/cpu/x86/lapic/lapic.c +++ b/src/cpu/x86/lapic/lapic.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */ +#include <assert.h> #include <cpu/cpu.h> #include <cpu/x86/lapic.h> #include <cpu/x86/lapic_def.h> @@ -10,23 +11,52 @@ void enable_lapic(void) { + uintptr_t apic_base; + bool use_x2apic; msr_t msr; msr = rdmsr(LAPIC_BASE_MSR); - msr.hi &= 0xffffff00; - msr.lo &= ~LAPIC_BASE_MSR_ADDR_MASK; - msr.lo |= LAPIC_DEFAULT_BASE; - msr.lo |= LAPIC_BASE_MSR_ENABLE; - wrmsr(LAPIC_BASE_MSR, msr); + if (!(msr.lo & LAPIC_BASE_MSR_ENABLE)) { + msr.hi &= 0xffffff00; + msr.lo &= ~LAPIC_BASE_MSR_ADDR_MASK; + msr.lo |= LAPIC_DEFAULT_BASE; + msr.lo |= LAPIC_BASE_MSR_ENABLE; + wrmsr(LAPIC_BASE_MSR, msr); + msr = rdmsr(LAPIC_BASE_MSR); + } - printk(BIOS_INFO, "Setting up local APIC 0x%x\n", lapicid()); + ASSERT(msr.lo & LAPIC_BASE_MSR_ENABLE); + + apic_base = msr.lo & LAPIC_BASE_MSR_ADDR_MASK; + ASSERT(apic_base == LAPIC_DEFAULT_BASE); + + if (CONFIG(XAPIC_ONLY)) { + use_x2apic = false; + } else { + use_x2apic = !!(cpu_get_feature_flags_ecx() & CPUID_X2APIC); + ASSERT(CONFIG(X2APIC_RUNTIME) || use_x2apic); + } + + if (use_x2apic == !!(msr.lo & LAPIC_BASE_MSR_X2APIC_MODE)) { + printk(BIOS_INFO, "LAPIC 0x%x in %s mode.\n", lapicid(), + use_x2apic ? "X2APIC" : "XAPIC"); + } else if (use_x2apic) { + msr.lo |= LAPIC_BASE_MSR_X2APIC_MODE; + wrmsr(LAPIC_BASE_MSR, msr); + msr = rdmsr(LAPIC_BASE_MSR); + ASSERT(!!(msr.lo & LAPIC_BASE_MSR_X2APIC_MODE)); + printk(BIOS_INFO, "LAPIC 0x%x switched to X2APIC mode.\n", lapicid()); + } else { + die("Switching from X2APIC to XAPIC mode is not implemented."); + } + } void disable_lapic(void) { msr_t msr; msr = rdmsr(LAPIC_BASE_MSR); - msr.lo &= ~LAPIC_BASE_MSR_ENABLE; + msr.lo &= ~(LAPIC_BASE_MSR_ENABLE | LAPIC_BASE_MSR_X2APIC_MODE); wrmsr(LAPIC_BASE_MSR, msr); } diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c index 888c97f..e1979c8 100644 --- a/src/cpu/x86/mp_init.c +++ b/src/cpu/x86/mp_init.c @@ -454,9 +454,6 @@ printk(BIOS_DEBUG, "Attempting to start %d APs\n", ap_count); - int x2apic_mode = is_x2apic_mode(); - printk(BIOS_DEBUG, "Starting CPUs in %s mode\n", x2apic_mode ? "x2apic" : "xapic"); - if (lapic_busy()) { printk(BIOS_DEBUG, "Waiting for ICR not to be busy...\n"); if (apic_wait_timeout(1000 /* 1 ms */, 50) != CB_SUCCESS) { diff --git a/src/include/cpu/x86/lapic.h b/src/include/cpu/x86/lapic.h index 537fa97..af0793b 100644 --- a/src/include/cpu/x86/lapic.h +++ b/src/include/cpu/x86/lapic.h @@ -59,7 +59,7 @@ wrmsr(X2APIC_MSR_ICR_ADDRESS, icr); } -static inline bool is_x2apic_mode(void) +static __always_inline bool is_x2apic_mode(void) { if (CONFIG(XAPIC_ONLY)) return false; diff --git a/src/include/cpu/x86/msr.h b/src/include/cpu/x86/msr.h index a8d5e22..4d1cb68 100644 --- a/src/include/cpu/x86/msr.h +++ b/src/include/cpu/x86/msr.h @@ -24,6 +24,7 @@ #define CPUID_VMX (1 << 5) #define CPUID_SMX (1 << 6) #define CPUID_DCA (1 << 18) +#define CPUID_X2APIC (1 << 21) #define CPUID_AES (1 << 25) #define SGX_GLOBAL_ENABLE (1 << 18) #define PLATFORM_INFO_SET_TDP (1 << 29) -- To view, visit https://review.coreboot.org/c/coreboot/+/55262 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I19a990ba287d21ccddaa64601923f1c4830e95e9 Gerrit-Change-Number: 55262 Gerrit-PatchSet: 17 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: cpu/x86/lapic: Unconditionally use CPUID leaf 0xb if available
by Kyösti Mälkki (Code Review) 05 Feb '22

05 Feb '22

Kyösti Mälkki has submitted this change. ( https://review.coreboot.org/c/coreboot/+/58386 ) Change subject: cpu/x86/lapic: Unconditionally use CPUID leaf 0xb if available ...................................................................... cpu/x86/lapic: Unconditionally use CPUID leaf 0xb if available Even when we're not in X2APIC mode, the information in CPUID leaf 0xb will be valid if that leaf is implemented on the CPU. Change-Id: I0f1f46fe5091ebeab6dfb4c7e151150cf495d0cb Signed-off-by: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/58386 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Reviewed-by: Wonkyu Kim <wonkyu.kim(a)intel.com> Reviewed-by: Arthur Heymans <arthur(a)aheymans.xyz> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> --- M src/cpu/x86/smm/smm_stub.S M src/include/cpu/x86/lapic.h 2 files changed, 8 insertions(+), 13 deletions(-) Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve Wonkyu Kim: Looks good to me, but someone else must approve Tim Wawrzynczak: Looks good to me, but someone else must approve diff --git a/src/cpu/x86/smm/smm_stub.S b/src/cpu/x86/smm/smm_stub.S index c83839c..25a35aa 100644 --- a/src/cpu/x86/smm/smm_stub.S +++ b/src/cpu/x86/smm/smm_stub.S @@ -101,30 +101,25 @@ * the OS can manipulate the APIC id use the non-changing cpuid result * for APIC id (eax). A table is used to handle a discontiguous * APIC id space. */ -apic_id: - mov $LAPIC_BASE_MSR, %ecx - rdmsr - and $LAPIC_BASE_X2APIC_ENABLED, %eax - cmp $LAPIC_BASE_X2APIC_ENABLED, %eax - jne xapic -x2apic: + mov $0, %eax + cpuid + cmp $0xb, %eax + jc 1f mov $0xb, %eax mov $0, %ecx cpuid mov %edx, %eax - jmp apicid_end - -xapic: + jmp 2f +1: mov $1, %eax cpuid mov %ebx, %eax shr $24, %eax +2: -apicid_end: mov $(apic_to_cpu_num), %ebx xor %ecx, %ecx - 1: cmp (%ebx, %ecx, 2), %ax je 1f diff --git a/src/include/cpu/x86/lapic.h b/src/include/cpu/x86/lapic.h index 05d096e..537fa97 100644 --- a/src/include/cpu/x86/lapic.h +++ b/src/include/cpu/x86/lapic.h @@ -126,7 +126,7 @@ static __always_inline unsigned int initial_lapicid(void) { uint32_t lapicid; - if (is_x2apic_mode() && cpuid_get_max_func() >= 0xb) + if (cpuid_get_max_func() >= 0xb) lapicid = cpuid_ext(0xb, 0).edx; else lapicid = cpuid_ebx(1) >> 24; -- To view, visit https://review.coreboot.org/c/coreboot/+/58386 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0f1f46fe5091ebeab6dfb4c7e151150cf495d0cb Gerrit-Change-Number: 58386 Gerrit-PatchSet: 7 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: sb/intel/i82801dx/smihandler.c: Use <southbridge/intel/common/pmbase.h>
by HAOUAS Elyes (Code Review) 05 Feb '22

05 Feb '22

HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/61647 ) Change subject: sb/intel/i82801dx/smihandler.c: Use <southbridge/intel/common/pmbase.h> ...................................................................... sb/intel/i82801dx/smihandler.c: Use <southbridge/intel/common/pmbase.h> Change-Id: Ic1b6db6ad1a61b575f009c9f5895bcfcbae301b7 Signed-off-by: Elyes HAOUAS <ehaouas(a)noos.fr> --- M src/southbridge/intel/i82801dx/smihandler.c 1 file changed, 29 insertions(+), 28 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/61647/1 diff --git a/src/southbridge/intel/i82801dx/smihandler.c b/src/southbridge/intel/i82801dx/smihandler.c index 575d1ba..107d58d 100644 --- a/src/southbridge/intel/i82801dx/smihandler.c +++ b/src/southbridge/intel/i82801dx/smihandler.c @@ -7,6 +7,7 @@ #include <cpu/x86/cache.h> #include <cpu/x86/smm.h> #include <device/pci_def.h> +#include <southbridge/intel/common/pmbase.h> #include "i82801dx.h" #define DEBUG_SMI @@ -32,9 +33,9 @@ { u16 reg16; - reg16 = inw(pmbase + PM1_STS); + reg16 = read_pmbase16(PM1_STS); /* set status bits are cleared by writing 1 to them */ - outw(reg16, pmbase + PM1_STS); + write_pmbase16(PM1_STS, reg16); return reg16; } @@ -51,7 +52,7 @@ if (pm1_sts & (1 << 4)) printk(BIOS_SPEW, "BM "); if (pm1_sts & (1 << 0)) printk(BIOS_SPEW, "TMROF "); printk(BIOS_SPEW, "\n"); - int reg16 = inw(pmbase + PM1_EN); + int reg16 = read_pmbase16(PM1_EN); printk(BIOS_SPEW, "PM1_EN: %x\n", reg16); } @@ -63,9 +64,9 @@ { u32 reg32; - reg32 = inl(pmbase + SMI_STS); + reg32 = read_pmbase32(SMI_STS); /* set status bits are cleared by writing 1 to them */ - outl(reg32, pmbase + SMI_STS); + write_pmbase32(SMI_STS, reg32); return reg32; } @@ -104,9 +105,9 @@ { u32 reg32; - reg32 = inl(pmbase + GPE0_STS); + reg32 = read_pmbase32(GPE0_STS); /* set status bits are cleared by writing 1 to them */ - outl(reg32, pmbase + GPE0_STS); + write_pmbase32(GPE0_STS, reg32); return reg32; } @@ -179,9 +180,9 @@ { u8 reg8; - reg8 = inb(pmbase + SMI_EN); + reg8 = read_pmbase8(SMI_EN); reg8 |= EOS; - outb(reg8, pmbase + SMI_EN); + write_pmbase8(SMI_EN, reg8); } static void busmaster_disable_on_bus(int bus) @@ -231,12 +232,12 @@ u8 s5pwr = CONFIG_MAINBOARD_POWER_FAILURE_STATE; /* First, disable further SMIs */ - reg8 = inb(pmbase + SMI_EN); + reg8 = read_pmbase8(SMI_EN); reg8 &= ~SLP_SMI_EN; - outb(reg8, pmbase + SMI_EN); + write_pmbase8(SMI_EN, reg8); /* Figure out SLP_TYP */ - reg32 = inl(pmbase + PM1_CNT); + reg32 = read_pmbase32(PM1_CNT); printk(BIOS_SPEW, "SMI#: SLP = 0x%08x\n", reg32); slp_typ = acpi_sleep_from_pm1(reg32); @@ -255,7 +256,7 @@ case ACPI_S5: printk(BIOS_DEBUG, "SMI#: Entering S5 (Soft Power off)\n"); - outl(0, pmbase + GPE0_EN); + write_pmbase32(GPE0_EN, 0); /* Should we keep the power state after a power loss? * In case the setting is "ON" or "OFF" we don't have @@ -278,17 +279,17 @@ * event again. We need to set BIT13 (SLP_EN) though to make the * sleep happen. */ - outl(reg32 | SLP_EN, pmbase + PM1_CNT); + write_pmbase32(PM1_CNT, reg32 | SLP_EN); /* In most sleep states, the code flow of this function ends at * the line above. However, if we entered sleep state S1 and wake * up again, we will continue to execute code in this function. */ - reg32 = inl(pmbase + PM1_CNT); + reg32 = read_pmbase32(PM1_CNT); if (reg32 & SCI_EN) { /* The OS is not an ACPI OS, so we set the state to S0 */ reg32 &= ~(SLP_EN | SLP_TYP); - outl(reg32, pmbase + PM1_CNT); + write_pmbase32(PM1_CNT, reg32); } } @@ -300,14 +301,14 @@ reg8 = apm_get_apmc(); switch (reg8) { case APM_CNT_ACPI_DISABLE: - pmctrl = inl(pmbase + PM1_CNT); + pmctrl = read_pmbase32(PM1_CNT); pmctrl &= ~SCI_EN; - outl(pmctrl, pmbase + PM1_CNT); + write_pmbase32(PM1_CNT, pmctrl); break; case APM_CNT_ACPI_ENABLE: - pmctrl = inl(pmbase + PM1_CNT); + pmctrl = read_pmbase32(PM1_CNT); pmctrl |= SCI_EN; - outl(pmctrl, pmbase + PM1_CNT); + write_pmbase32(PM1_CNT, pmctrl); break; } } @@ -326,7 +327,7 @@ // power button pressed u32 reg32; reg32 = (7 << 10) | (1 << 13); - outl(reg32, pmbase + PM1_CNT); + write_pmbase32(PM1_CNT, reg32); } } @@ -341,10 +342,10 @@ static void southbridge_smi_gpi(void) { u16 reg16; - reg16 = inw(pmbase + ALT_GP_SMI_STS); - outl(reg16, pmbase + ALT_GP_SMI_STS); + reg16 = read_pmbase16(ALT_GP_SMI_STS); + write_pmbase16(ALT_GP_SMI_STS, reg16); - reg16 &= inw(pmbase + ALT_GP_SMI_EN); + reg16 &= read_pmbase16(ALT_GP_SMI_EN); mainboard_smi_gpi(reg16); @@ -356,7 +357,7 @@ { u32 reg32; - reg32 = inl(pmbase + SMI_EN); + reg32 = read_pmbase32(SMI_EN); /* Are periodic SMIs enabled? */ if ((reg32 & MCSMI_EN) == 0) @@ -406,7 +407,7 @@ { u32 reg32; - reg32 = inl(pmbase + SMI_EN); + reg32 = read_pmbase32(SMI_EN); /* Are periodic SMIs enabled? */ if ((reg32 & PERIODIC_EN) == 0) @@ -461,7 +462,7 @@ u32 smi_sts; /* Update global variable pmbase */ - pmbase = pci_read_config16(PCI_DEV(0, 0x1f, 0), 0x40) & 0xfffc; + pmbase = lpc_get_pmbase(); /* We need to clear the SMI status registers, or we won't see what's * happening in the following calls. @@ -470,7 +471,7 @@ /* Filter all non-enabled SMI events */ // FIXME Double check, this clears MONITOR - // smi_sts &= inl(pmbase + SMI_EN); + // smi_sts &= read_pmbase32(SMI_EN); /* Call SMI sub handler for each of the status bits */ for (i = 0; i < 31; i++) { -- To view, visit https://review.coreboot.org/c/coreboot/+/61647 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic1b6db6ad1a61b575f009c9f5895bcfcbae301b7 Gerrit-Change-Number: 61647 Gerrit-PatchSet: 1 Gerrit-Owner: HAOUAS Elyes <ehaouas(a)noos.fr> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: soc/intel/common/basecode/eop: coreboot driver perform EOP operations
by Subrata Banik (Code Review) 05 Feb '22

05 Feb '22

Attention is currently required from: Arthur Heymans, Felix Singer, Tim Wawrzynczak, Angel Pons, Lean Sheng Tan, Werner Zeh, Patrick Rudolph, EricR Lai. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/60405 ) Change subject: soc/intel/common/basecode/eop: coreboot driver perform EOP operations ...................................................................... Patch Set 23: (1 comment) File src/soc/intel/common/basecode/eop/eop.c: https://review.coreboot.org/c/coreboot/+/60405/comment/87f909ed_4759dc32 PS23, Line 20: static void do_eop_operations(void *unused) : { : if (CONFIG(SKIP_FSP_NOTIFY_PHASE_READY_TO_BOOT)) { : /* Step 1 */ : cse_send_end_of_post(); : : /* Step 2 */ : cse_lock_config(); : : /* Step 3 */ : if (CONFIG(DISABLE_HECI1_AT_PRE_BOOT)) { : cse_set_to_d0i3(); : heci1_disable(); : } : } : : if (CONFIG(SKIP_FSP_NOTIFY_PHASE_END_OF_FIRMWARE)) { : /* Step 4 */ : heci_set_to_d0i3(); : /* Step 5 */ : pmc_clear_pmcon_sts(); : } : } > Now that CSE device state is independent of hiding (see DISABLE_HECI1_AT_PRE_BOOT Kconfig), we should use its device operations instead of bootstate callbacks. Good feedback! then ideally we will be ending up adding `.final` in all relevant IP code (cse, pmc, spi etc. etc.) that need some additional configuration, isn't it. Can we consider this as design assumption ? -- To view, visit https://review.coreboot.org/c/coreboot/+/60405 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I70bde33f77026e8be165ff082defe3cab6686ec7 Gerrit-Change-Number: 60405 Gerrit-PatchSet: 23 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-CC: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Attention: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Comment-Date: Sat, 05 Feb 2022 07:31:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common/cse: Add function to perform CSE lock configuration
by Subrata Banik (Code Review) 05 Feb '22

05 Feb '22

Attention is currently required from: Tim Wawrzynczak, Sridhar Siricilla, Werner Zeh, Patrick Rudolph. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61520 ) Change subject: soc/intel/common/cse: Add function to perform CSE lock configuration ...................................................................... Patch Set 6: (2 comments) File src/soc/intel/common/block/cse/cse.c: https://review.coreboot.org/c/coreboot/+/61520/comment/33bd52f8_e5f13bed PS5, Line 1014: /* > >Ack ? > means? When you acknowledge it's you had wrong understanding this code don't need any change (as you have initially suggested in line 1014) then folks ideally *click* on the ACK to close the thread. File src/soc/intel/common/block/cse/cse.c: https://review.coreboot.org/c/coreboot/+/61520/comment/2c0a0757_ae041265 PS6, Line 1018: (cse_is_hfs1_com_normal() || cse_is_hfs1_com_soft_temp_disable()) > >3rd para says. > >"Furthermore, if Intel® CSME is in ERROR state, **BIOS** can use I/O 0xCF9 write of >06h or 0Eh command with PMC PWRM offset 1048h register bit [20] CF9GR set to perform the global reset > > I commented based on 3rd para you posted in this comment thread. Can you please create crosbug, we can discuss further there? I'm also specific about 3rd para that says about error condition in CSE. **When CSE is always in bad state then how can one follow #2 so the only way is #1 hence, we should avoid locking the PMC cf9.** Based on my reply above, please let me know if you still have some more opens? Do we still need a dedicated crossbus for such chipset configuration discussion ? -- To view, visit https://review.coreboot.org/c/coreboot/+/61520 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3894b2cd8b90dc033f475384486815ab2fadf381 Gerrit-Change-Number: 61520 Gerrit-PatchSet: 6 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Sat, 05 Feb 2022 07:27:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Comment-In-Reply-To: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common/cse: Add function to perform CSE lock configuration
by Sridhar Siricilla (Code Review) 05 Feb '22

05 Feb '22

Attention is currently required from: Subrata Banik, Tim Wawrzynczak, Werner Zeh, Patrick Rudolph. Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61520 ) Change subject: soc/intel/common/cse: Add function to perform CSE lock configuration ...................................................................... Patch Set 6: (2 comments) File src/soc/intel/common/block/cse/cse.c: https://review.coreboot.org/c/coreboot/+/61520/comment/99626557_c7753ce9 PS5, Line 1014: /* > Ack ? >Ack ? means? File src/soc/intel/common/block/cse/cse.c: https://review.coreboot.org/c/coreboot/+/61520/comment/d294cf94_0f241b17 PS6, Line 1018: (cse_is_hfs1_com_normal() || cse_is_hfs1_com_soft_temp_disable()) > > Only BIOS should have capability to trigger global reset and sending GLOBAL RESET MEI message. […] >3rd para says. >"Furthermore, if Intel® CSME is in ERROR state, **BIOS** can use I/O 0xCF9 write of >06h or 0Eh command with PMC PWRM offset 1048h register bit [20] CF9GR set to perform the global reset I commented based on 3rd para you posted in this comment thread. Can you please create crosbug, we can discuss further there? -- To view, visit https://review.coreboot.org/c/coreboot/+/61520 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3894b2cd8b90dc033f475384486815ab2fadf381 Gerrit-Change-Number: 61520 Gerrit-PatchSet: 6 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Sat, 05 Feb 2022 07:16:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Comment-In-Reply-To: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: sb/intel/common/pmutil.c: Fix comment
by HAOUAS Elyes (Code Review) 05 Feb '22

05 Feb '22

HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/61646 ) Change subject: sb/intel/common/pmutil.c: Fix comment ...................................................................... sb/intel/common/pmutil.c: Fix comment Change-Id: I3e151959d8f9b6ddec7c5f70dbb4cd5ffde44f5d Signed-off-by: Elyes HAOUAS <ehaouas(a)noos.fr> --- M src/southbridge/intel/common/pmutil.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/61646/1 diff --git a/src/southbridge/intel/common/pmutil.c b/src/southbridge/intel/common/pmutil.c index 02a8375..040d359 100644 --- a/src/southbridge/intel/common/pmutil.c +++ b/src/southbridge/intel/common/pmutil.c @@ -147,7 +147,7 @@ reg32 = read_pmbase32(TCO1_STS); /* - * set status bits are cleared by writing 1 to them, but don't + * Status bits are cleared by writing 1 to them, but don't * clear BOOT_STS before SECOND_TO_STS. */ write_pmbase32(TCO1_STS, reg32 & ~BOOT_STS); -- To view, visit https://review.coreboot.org/c/coreboot/+/61646 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3e151959d8f9b6ddec7c5f70dbb4cd5ffde44f5d Gerrit-Change-Number: 61646 Gerrit-PatchSet: 1 Gerrit-Owner: HAOUAS Elyes <ehaouas(a)noos.fr> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: drivers/pcie/generic: Add new pcie generic chip driver
by HAOUAS Elyes (Code Review) 05 Feb '22

05 Feb '22

Attention is currently required from: Subrata Banik, Tim Wawrzynczak, Angel Pons, Nick Vaccaro. HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61627 ) Change subject: drivers/pcie/generic: Add new pcie generic chip driver ...................................................................... Patch Set 3: (1 comment) File src/drivers/pcie/generic/chip.h: https://review.coreboot.org/c/coreboot/+/61627/comment/53866df6_317dae29 PS3, Line 5: include stdbool.h ? -- To view, visit https://review.coreboot.org/c/coreboot/+/61627 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I53986614dcbf4d10a6bb4010e131f5ff5a9d25cf Gerrit-Change-Number: 61627 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: HAOUAS Elyes <ehaouas(a)noos.fr> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Comment-Date: Sat, 05 Feb 2022 05:52:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: drivers/uart/uart8250reg.h: use BIT() macro for bit definitions
by HAOUAS Elyes (Code Review) 05 Feb '22

05 Feb '22

Attention is currently required from: Felix Held. HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61643 ) Change subject: drivers/uart/uart8250reg.h: use BIT() macro for bit definitions ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/61643 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib726dd77eaf1a4f8a7d9fbf8ab6d46a7bb1de6c0 Gerrit-Change-Number: 61643 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: HAOUAS Elyes <ehaouas(a)noos.fr> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Sat, 05 Feb 2022 05:44:31 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: drivers/uart/uart8250reg.h: use shifts in constants
by HAOUAS Elyes (Code Review) 05 Feb '22

05 Feb '22

Attention is currently required from: Felix Held. HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61644 ) Change subject: drivers/uart/uart8250reg.h: use shifts in constants ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/61644 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0663c1a641355b7bfb59f41479d17117178fb895 Gerrit-Change-Number: 61644 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: HAOUAS Elyes <ehaouas(a)noos.fr> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Sat, 05 Feb 2022 05:44:08 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit February 2022