coreboot-gerrit June 2020

coreboot-gerrit@coreboot.org

1 participants
2826 discussions

Change in coreboot[master]: usb/xhci: Fix timeout logic
by Patrick Georgi (Code Review) 06 Jun '20

06 Jun '20

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41854 ) Change subject: usb/xhci: Fix timeout logic ...................................................................... usb/xhci: Fix timeout logic This fixes a logic bug in how timeouts are reported back. In the timeout case, the original code would return -1 instead of 0. All call sites expect a return value of 0 as the timeout indicator. Signed-off-by: Caveh Jalali <caveh(a)chromium.org> Change-Id: I81a888aa0a1544e55e6a680be8f3b7f6e0d87812 Reviewed-on: https://review.coreboot.org/c/coreboot/+/41854 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Tim Wawrzynczak <twawrzynczak(a)chromium.org> --- M payloads/libpayload/drivers/usb/xhci.c 1 file changed, 6 insertions(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, approved diff --git a/payloads/libpayload/drivers/usb/xhci.c b/payloads/libpayload/drivers/usb/xhci.c index 07d5b16..749ff0a 100644 --- a/payloads/libpayload/drivers/usb/xhci.c +++ b/payloads/libpayload/drivers/usb/xhci.c @@ -129,7 +129,12 @@ static long xhci_handshake(volatile u32 *const reg, u32 mask, u32 wait_for, long timeout_us) { - while ((*reg & mask) != wait_for && timeout_us--) udelay(1); + if (timeout_us <= 0) + return 0; + while ((*reg & mask) != wait_for && timeout_us != 0) { + --timeout_us; + udelay(1); + } return timeout_us; } -- To view, visit https://review.coreboot.org/c/coreboot/+/41854 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I81a888aa0a1544e55e6a680be8f3b7f6e0d87812 Gerrit-Change-Number: 41854 Gerrit-PatchSet: 3 Gerrit-Owner: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: arch/x86: Declare permanent_smi_handler()
by Kyösti Mälkki (Code Review) 06 Jun '20

06 Jun '20

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41910 ) Change subject: arch/x86: Declare permanent_smi_handler() ...................................................................... arch/x86: Declare permanent_smi_handler() Advertising SMI triggers in FADT is only valid if we exit with SMI installed. Change-Id: Iad92c4a180524e15199633693446a087787ad3a2 Signed-off-by: Kyösti Mälkki <kyosti.malkki(a)gmail.com> --- M src/include/acpi/acpi.h M src/mainboard/aopen/dxplplusu/fadt.c M src/mainboard/emulation/qemu-q35/acpi_tables.c M src/mainboard/lenovo/t400/fadt.c M src/mainboard/lenovo/x200/fadt.c M src/mainboard/roda/rk9/fadt.c M src/soc/amd/picasso/acpi.c M src/soc/amd/stoneyridge/acpi.c M src/soc/intel/baytrail/acpi.c M src/soc/intel/braswell/acpi.c M src/soc/intel/broadwell/acpi.c M src/soc/intel/common/block/acpi/acpi.c M src/soc/intel/skylake/acpi.c M src/soc/intel/xeon_sp/cpx/acpi.c M src/soc/intel/xeon_sp/skx/acpi.c M src/southbridge/amd/agesa/hudson/fadt.c M src/southbridge/amd/cimx/sb800/fadt.c M src/southbridge/amd/pi/hudson/fadt.c M src/southbridge/intel/bd82x6x/lpc.c M src/southbridge/intel/i82371eb/fadt.c M src/southbridge/intel/i82801gx/lpc.c M src/southbridge/intel/i82801ix/i82801ix.c M src/southbridge/intel/i82801jx/i82801jx.c M src/southbridge/intel/i82801jx/lpc.c M src/southbridge/intel/ibexpeak/lpc.c M src/southbridge/intel/lynxpoint/lpc.c 26 files changed, 36 insertions(+), 31 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/41910/1 diff --git a/src/include/acpi/acpi.h b/src/include/acpi/acpi.h index ea80e3b..544e2c5 100644 --- a/src/include/acpi/acpi.h +++ b/src/include/acpi/acpi.h @@ -1011,6 +1011,11 @@ /* Read and clear GPE status */ int acpi_get_gpe(int gpe); +static inline int permanent_smi_handler(void) +{ + return CONFIG(HAVE_SMI_HANDLER); +} + static inline int acpi_s3_resume_allowed(void) { return CONFIG(HAVE_ACPI_RESUME); diff --git a/src/mainboard/aopen/dxplplusu/fadt.c b/src/mainboard/aopen/dxplplusu/fadt.c index 339c243..ad522c4 100644 --- a/src/mainboard/aopen/dxplplusu/fadt.c +++ b/src/mainboard/aopen/dxplplusu/fadt.c @@ -38,7 +38,7 @@ fadt->sci_int = 0x9; - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { fadt->smi_cmd = APM_CNT; fadt->acpi_enable = ACPI_ENABLE; fadt->acpi_disable = ACPI_DISABLE; diff --git a/src/mainboard/emulation/qemu-q35/acpi_tables.c b/src/mainboard/emulation/qemu-q35/acpi_tables.c index 4236257..9bf58a6 100644 --- a/src/mainboard/emulation/qemu-q35/acpi_tables.c +++ b/src/mainboard/emulation/qemu-q35/acpi_tables.c @@ -43,7 +43,7 @@ fadt->preferred_pm_profile = PM_MOBILE; fadt->sci_int = 0x9; - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { /* Do not advertise SMI even if installed. */ fadt->smi_cmd = 0; fadt->acpi_enable = 0; diff --git a/src/mainboard/lenovo/t400/fadt.c b/src/mainboard/lenovo/t400/fadt.c index d15b9b3..257fb41 100644 --- a/src/mainboard/lenovo/t400/fadt.c +++ b/src/mainboard/lenovo/t400/fadt.c @@ -26,7 +26,7 @@ fadt->preferred_pm_profile = PM_MOBILE; fadt->sci_int = 0x9; - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { fadt->smi_cmd = APM_CNT; fadt->acpi_enable = APM_CNT_ACPI_ENABLE; fadt->acpi_disable = APM_CNT_ACPI_DISABLE; diff --git a/src/mainboard/lenovo/x200/fadt.c b/src/mainboard/lenovo/x200/fadt.c index d15b9b3..257fb41 100644 --- a/src/mainboard/lenovo/x200/fadt.c +++ b/src/mainboard/lenovo/x200/fadt.c @@ -26,7 +26,7 @@ fadt->preferred_pm_profile = PM_MOBILE; fadt->sci_int = 0x9; - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { fadt->smi_cmd = APM_CNT; fadt->acpi_enable = APM_CNT_ACPI_ENABLE; fadt->acpi_disable = APM_CNT_ACPI_DISABLE; diff --git a/src/mainboard/roda/rk9/fadt.c b/src/mainboard/roda/rk9/fadt.c index 102865d..5127545 100644 --- a/src/mainboard/roda/rk9/fadt.c +++ b/src/mainboard/roda/rk9/fadt.c @@ -26,7 +26,7 @@ fadt->preferred_pm_profile = PM_MOBILE; fadt->sci_int = 0x9; - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { fadt->smi_cmd = APM_CNT; fadt->acpi_enable = APM_CNT_ACPI_ENABLE; fadt->acpi_disable = APM_CNT_ACPI_DISABLE; diff --git a/src/soc/amd/picasso/acpi.c b/src/soc/amd/picasso/acpi.c index d5a890b..817ec51 100644 --- a/src/soc/amd/picasso/acpi.c +++ b/src/soc/amd/picasso/acpi.c @@ -107,7 +107,7 @@ fadt->preferred_pm_profile = FADT_PM_PROFILE; fadt->sci_int = 9; /* IRQ 09 - ACPI SCI */ - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { fadt->smi_cmd = APM_CNT; fadt->acpi_enable = APM_CNT_ACPI_ENABLE; fadt->acpi_disable = APM_CNT_ACPI_DISABLE; diff --git a/src/soc/amd/stoneyridge/acpi.c b/src/soc/amd/stoneyridge/acpi.c index 47d12bf..a7caa93 100644 --- a/src/soc/amd/stoneyridge/acpi.c +++ b/src/soc/amd/stoneyridge/acpi.c @@ -79,7 +79,7 @@ fadt->preferred_pm_profile = FADT_PM_PROFILE; fadt->sci_int = 9; /* IRQ 09 - ACPI SCI */ - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { fadt->smi_cmd = APM_CNT; fadt->acpi_enable = APM_CNT_ACPI_ENABLE; fadt->acpi_disable = APM_CNT_ACPI_DISABLE; diff --git a/src/soc/intel/baytrail/acpi.c b/src/soc/intel/baytrail/acpi.c index 66263e6..f9eb500 100644 --- a/src/soc/intel/baytrail/acpi.c +++ b/src/soc/intel/baytrail/acpi.c @@ -129,7 +129,7 @@ fadt->sci_int = acpi_sci_irq(); - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { fadt->smi_cmd = APM_CNT; fadt->acpi_enable = APM_CNT_ACPI_ENABLE; fadt->acpi_disable = APM_CNT_ACPI_DISABLE; diff --git a/src/soc/intel/braswell/acpi.c b/src/soc/intel/braswell/acpi.c index 4e3795e..e6790a7 100644 --- a/src/soc/intel/braswell/acpi.c +++ b/src/soc/intel/braswell/acpi.c @@ -133,7 +133,7 @@ fadt->sci_int = acpi_sci_irq(); - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { fadt->smi_cmd = APM_CNT; fadt->acpi_enable = APM_CNT_ACPI_ENABLE; fadt->acpi_disable = APM_CNT_ACPI_DISABLE; diff --git a/src/soc/intel/broadwell/acpi.c b/src/soc/intel/broadwell/acpi.c index 9df7dee..081b08a 100644 --- a/src/soc/intel/broadwell/acpi.c +++ b/src/soc/intel/broadwell/acpi.c @@ -185,7 +185,7 @@ fadt->sci_int = acpi_sci_irq(); - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { fadt->smi_cmd = APM_CNT; fadt->acpi_enable = APM_CNT_ACPI_ENABLE; fadt->acpi_disable = APM_CNT_ACPI_DISABLE; diff --git a/src/soc/intel/common/block/acpi/acpi.c b/src/soc/intel/common/block/acpi/acpi.c index c4a0698..bda2399 100644 --- a/src/soc/intel/common/block/acpi/acpi.c +++ b/src/soc/intel/common/block/acpi/acpi.c @@ -101,7 +101,7 @@ fadt->sci_int = acpi_sci_irq(); - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { fadt->smi_cmd = APM_CNT; fadt->acpi_enable = APM_CNT_ACPI_ENABLE; fadt->acpi_disable = APM_CNT_ACPI_DISABLE; diff --git a/src/soc/intel/skylake/acpi.c b/src/soc/intel/skylake/acpi.c index 34aed02..bdfacd3 100644 --- a/src/soc/intel/skylake/acpi.c +++ b/src/soc/intel/skylake/acpi.c @@ -226,7 +226,7 @@ fadt->sci_int = acpi_sci_irq(); - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { fadt->smi_cmd = APM_CNT; fadt->acpi_enable = APM_CNT_ACPI_ENABLE; fadt->acpi_disable = APM_CNT_ACPI_DISABLE; diff --git a/src/soc/intel/xeon_sp/cpx/acpi.c b/src/soc/intel/xeon_sp/cpx/acpi.c index d6d847b..b090d12 100644 --- a/src/soc/intel/xeon_sp/cpx/acpi.c +++ b/src/soc/intel/xeon_sp/cpx/acpi.c @@ -148,7 +148,7 @@ fadt->x_gpe1_blk.space_id = 1; - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { fadt->smi_cmd = APM_CNT; fadt->acpi_enable = APM_CNT_ACPI_ENABLE; fadt->acpi_disable = APM_CNT_ACPI_DISABLE; diff --git a/src/soc/intel/xeon_sp/skx/acpi.c b/src/soc/intel/xeon_sp/skx/acpi.c index 9818508..374275b 100644 --- a/src/soc/intel/xeon_sp/skx/acpi.c +++ b/src/soc/intel/xeon_sp/skx/acpi.c @@ -339,7 +339,7 @@ fadt->sci_int = acpi_sci_irq(); /* TODO: enabled SMM mode switch when SMM handlers are set up. */ - if (0 && CONFIG(HAVE_SMI_HANDLER)) { + if (0 && permanent_smi_handler()) { fadt->smi_cmd = APM_CNT; fadt->acpi_enable = APM_CNT_ACPI_ENABLE; fadt->acpi_disable = APM_CNT_ACPI_DISABLE; diff --git a/src/southbridge/amd/agesa/hudson/fadt.c b/src/southbridge/amd/agesa/hudson/fadt.c index 660598a..a328a88 100644 --- a/src/southbridge/amd/agesa/hudson/fadt.c +++ b/src/southbridge/amd/agesa/hudson/fadt.c @@ -57,7 +57,7 @@ fadt->preferred_pm_profile = FADT_PM_PROFILE; fadt->sci_int = 9; /* HUDSON - IRQ 09 - ACPI SCI */ - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { fadt->smi_cmd = ACPI_SMI_CTL_PORT; fadt->acpi_enable = ACPI_SMI_CMD_ENABLE; fadt->acpi_disable = ACPI_SMI_CMD_DISABLE; diff --git a/src/southbridge/amd/cimx/sb800/fadt.c b/src/southbridge/amd/cimx/sb800/fadt.c index e836cfb..bd0ec9c 100644 --- a/src/southbridge/amd/cimx/sb800/fadt.c +++ b/src/southbridge/amd/cimx/sb800/fadt.c @@ -62,7 +62,7 @@ fadt->preferred_pm_profile = FADT_PM_PROFILE; fadt->sci_int = 9; /* HUDSON 1 - IRQ 09 - ACPI SCI */ - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { fadt->smi_cmd = ACPI_SMI_CTL_PORT; fadt->acpi_enable = ACPI_SMI_CMD_ENABLE; fadt->acpi_disable = ACPI_SMI_CMD_DISABLE; diff --git a/src/southbridge/amd/pi/hudson/fadt.c b/src/southbridge/amd/pi/hudson/fadt.c index 100cfb6..686b669 100644 --- a/src/southbridge/amd/pi/hudson/fadt.c +++ b/src/southbridge/amd/pi/hudson/fadt.c @@ -49,7 +49,7 @@ fadt->preferred_pm_profile = FADT_PM_PROFILE; fadt->sci_int = 9; /* HUDSON - IRQ 09 - ACPI SCI */ - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { fadt->smi_cmd = ACPI_SMI_CTL_PORT; fadt->acpi_enable = ACPI_SMI_CMD_ENABLE; fadt->acpi_disable = ACPI_SMI_CMD_DISABLE; diff --git a/src/southbridge/intel/bd82x6x/lpc.c b/src/southbridge/intel/bd82x6x/lpc.c index 45215f1..d939429 100644 --- a/src/southbridge/intel/bd82x6x/lpc.c +++ b/src/southbridge/intel/bd82x6x/lpc.c @@ -698,7 +698,7 @@ fadt->sci_int = 0x9; - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { fadt->smi_cmd = APM_CNT; fadt->acpi_enable = APM_CNT_ACPI_ENABLE; fadt->acpi_disable = APM_CNT_ACPI_DISABLE; @@ -836,7 +836,7 @@ spi_finalize_ops(); /* Call SMM finalize() handlers before resume */ - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { if (CONFIG(INTEL_CHIPSET_LOCKDOWN) || acpi_is_wakeup_s3()) { outb(APM_CNT_FINALIZE, APM_CNT); diff --git a/src/southbridge/intel/i82371eb/fadt.c b/src/southbridge/intel/i82371eb/fadt.c index 26a1856..583a9ed 100644 --- a/src/southbridge/intel/i82371eb/fadt.c +++ b/src/southbridge/intel/i82371eb/fadt.c @@ -36,7 +36,7 @@ fadt->preferred_pm_profile = 0; /* unspecified */ fadt->sci_int = 9; - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { fadt->smi_cmd = ACPI_SMI_CTL_PORT; fadt->acpi_enable = ACPI_SMI_CMD_ENABLE; fadt->acpi_disable = ACPI_SMI_CMD_DISABLE; diff --git a/src/southbridge/intel/i82801gx/lpc.c b/src/southbridge/intel/i82801gx/lpc.c index 86c3e58..d4aef55 100644 --- a/src/southbridge/intel/i82801gx/lpc.c +++ b/src/southbridge/intel/i82801gx/lpc.c @@ -522,7 +522,7 @@ fadt->reserved = 0; fadt->sci_int = 0x9; - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { fadt->smi_cmd = APM_CNT; fadt->acpi_enable = APM_CNT_ACPI_ENABLE; fadt->acpi_disable = APM_CNT_ACPI_DISABLE; diff --git a/src/southbridge/intel/i82801ix/i82801ix.c b/src/southbridge/intel/i82801ix/i82801ix.c index b2b55ce..5e5e667 100644 --- a/src/southbridge/intel/i82801ix/i82801ix.c +++ b/src/southbridge/intel/i82801ix/i82801ix.c @@ -204,9 +204,9 @@ i82801ix_hide_functions(); /* Reset watchdog timer. */ -#if !CONFIG(HAVE_SMI_HANDLER) - outw(0x0008, DEFAULT_TCOBASE + 0x12); /* Set higher timer value. */ -#endif + if (!permanent_smi_handler()) + outw(0x0008, DEFAULT_TCOBASE + 0x12); /* Set higher timer value. */ + outw(0x0000, DEFAULT_TCOBASE + 0x00); /* Update timer. */ } diff --git a/src/southbridge/intel/i82801jx/i82801jx.c b/src/southbridge/intel/i82801jx/i82801jx.c index b4ffcfa..7c6371b 100644 --- a/src/southbridge/intel/i82801jx/i82801jx.c +++ b/src/southbridge/intel/i82801jx/i82801jx.c @@ -203,9 +203,9 @@ i82801jx_hide_functions(); /* Reset watchdog timer. */ -#if !CONFIG(HAVE_SMI_HANDLER) - outw(0x0008, DEFAULT_TCOBASE + 0x12); /* Set higher timer value. */ -#endif + if (!permanent_smi_handler()) + outw(0x0008, DEFAULT_TCOBASE + 0x12); /* Set higher timer value. */ + outw(0x0000, DEFAULT_TCOBASE + 0x00); /* Update timer. */ } diff --git a/src/southbridge/intel/i82801jx/lpc.c b/src/southbridge/intel/i82801jx/lpc.c index b3aeda2..7a1b24b 100644 --- a/src/southbridge/intel/i82801jx/lpc.c +++ b/src/southbridge/intel/i82801jx/lpc.c @@ -533,7 +533,7 @@ fadt->reserved = 0; fadt->sci_int = 0x9; - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { fadt->smi_cmd = APM_CNT; fadt->acpi_enable = APM_CNT_ACPI_ENABLE; fadt->acpi_disable = APM_CNT_ACPI_DISABLE; diff --git a/src/southbridge/intel/ibexpeak/lpc.c b/src/southbridge/intel/ibexpeak/lpc.c index 1209f9b..c64f6b4 100644 --- a/src/southbridge/intel/ibexpeak/lpc.c +++ b/src/southbridge/intel/ibexpeak/lpc.c @@ -592,7 +592,7 @@ fadt->sci_int = 0x9; - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { fadt->smi_cmd = APM_CNT; fadt->acpi_enable = APM_CNT_ACPI_ENABLE; fadt->acpi_disable = APM_CNT_ACPI_DISABLE; @@ -729,7 +729,7 @@ spi_finalize_ops(); /* Call SMM finalize() handlers before resume */ - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { if (CONFIG(INTEL_CHIPSET_LOCKDOWN) || acpi_is_wakeup_s3()) { outb(APM_CNT_FINALIZE, APM_CNT); diff --git a/src/southbridge/intel/lynxpoint/lpc.c b/src/southbridge/intel/lynxpoint/lpc.c index da744bf..1d75555 100644 --- a/src/southbridge/intel/lynxpoint/lpc.c +++ b/src/southbridge/intel/lynxpoint/lpc.c @@ -741,7 +741,7 @@ fadt->sci_int = 0x9; - if (CONFIG(HAVE_SMI_HANDLER)) { + if (permanent_smi_handler()) { fadt->smi_cmd = APM_CNT; fadt->acpi_enable = APM_CNT_ACPI_ENABLE; fadt->acpi_disable = APM_CNT_ACPI_DISABLE; -- To view, visit https://review.coreboot.org/c/coreboot/+/41910 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iad92c4a180524e15199633693446a087787ad3a2 Gerrit-Change-Number: 41910 Gerrit-PatchSet: 1 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Alexander Couzens <lynxis(a)fe80.eu> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newchange

5 12

Change in coreboot[master]: mb/lenovo/t400,x200: Control SMI related FADT entries
by Kyösti Mälkki (Code Review) 06 Jun '20

06 Jun '20

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41906 ) Change subject: mb/lenovo/t400,x200: Control SMI related FADT entries ...................................................................... mb/lenovo/t400,x200: Control SMI related FADT entries When no SMI is installed, FADT should not advertise a trigger mechanism that does respond. Change-Id: Ia61e50fb49719e9e18e81a27f355cdbd755a3f40 Signed-off-by: Kyösti Mälkki <kyosti.malkki(a)gmail.com> --- M src/mainboard/lenovo/t400/fadt.c M src/mainboard/lenovo/x200/fadt.c 2 files changed, 16 insertions(+), 12 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/41906/1 diff --git a/src/mainboard/lenovo/t400/fadt.c b/src/mainboard/lenovo/t400/fadt.c index 56355ee..d15b9b3 100644 --- a/src/mainboard/lenovo/t400/fadt.c +++ b/src/mainboard/lenovo/t400/fadt.c @@ -25,11 +25,14 @@ fadt->reserved = 0x00; fadt->preferred_pm_profile = PM_MOBILE; fadt->sci_int = 0x9; - fadt->smi_cmd = APM_CNT; - fadt->acpi_enable = APM_CNT_ACPI_ENABLE; - fadt->acpi_disable = APM_CNT_ACPI_DISABLE; - fadt->s4bios_req = 0x0; - fadt->pstate_cnt = APM_CNT_PST_CONTROL; + + if (CONFIG(HAVE_SMI_HANDLER)) { + fadt->smi_cmd = APM_CNT; + fadt->acpi_enable = APM_CNT_ACPI_ENABLE; + fadt->acpi_disable = APM_CNT_ACPI_DISABLE; + fadt->pstate_cnt = APM_CNT_PST_CONTROL; + fadt->cst_cnt = APM_CNT_CST_CONTROL; + } fadt->pm1a_evt_blk = pmbase; fadt->pm1b_evt_blk = 0x0; @@ -48,7 +51,6 @@ fadt->gpe0_blk_len = 16; fadt->gpe1_blk_len = 0; fadt->gpe1_base = 0; - fadt->cst_cnt = APM_CNT_CST_CONTROL; fadt->p_lvl2_lat = 1; fadt->p_lvl3_lat = 0x39; fadt->flush_size = 0; diff --git a/src/mainboard/lenovo/x200/fadt.c b/src/mainboard/lenovo/x200/fadt.c index 56355ee..d15b9b3 100644 --- a/src/mainboard/lenovo/x200/fadt.c +++ b/src/mainboard/lenovo/x200/fadt.c @@ -25,11 +25,14 @@ fadt->reserved = 0x00; fadt->preferred_pm_profile = PM_MOBILE; fadt->sci_int = 0x9; - fadt->smi_cmd = APM_CNT; - fadt->acpi_enable = APM_CNT_ACPI_ENABLE; - fadt->acpi_disable = APM_CNT_ACPI_DISABLE; - fadt->s4bios_req = 0x0; - fadt->pstate_cnt = APM_CNT_PST_CONTROL; + + if (CONFIG(HAVE_SMI_HANDLER)) { + fadt->smi_cmd = APM_CNT; + fadt->acpi_enable = APM_CNT_ACPI_ENABLE; + fadt->acpi_disable = APM_CNT_ACPI_DISABLE; + fadt->pstate_cnt = APM_CNT_PST_CONTROL; + fadt->cst_cnt = APM_CNT_CST_CONTROL; + } fadt->pm1a_evt_blk = pmbase; fadt->pm1b_evt_blk = 0x0; @@ -48,7 +51,6 @@ fadt->gpe0_blk_len = 16; fadt->gpe1_blk_len = 0; fadt->gpe1_base = 0; - fadt->cst_cnt = APM_CNT_CST_CONTROL; fadt->p_lvl2_lat = 1; fadt->p_lvl3_lat = 0x39; fadt->flush_size = 0; -- To view, visit https://review.coreboot.org/c/coreboot/+/41906 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia61e50fb49719e9e18e81a27f355cdbd755a3f40 Gerrit-Change-Number: 41906 Gerrit-PatchSet: 1 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Alexander Couzens <lynxis(a)fe80.eu> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newchange

4 5

Change in coreboot[master]: spi/winbond: Rename bp4.srp0 to srp to follow datasheet naming
by Daniel Gröber (dxld) (Code Review) 06 Jun '20

06 Jun '20

Hello Daniel Gröber, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/42116 to review the following change. Change subject: spi/winbond: Rename bp4.srp0 to srp to follow datasheet naming ...................................................................... spi/winbond: Rename bp4.srp0 to srp to follow datasheet naming The datasheets for 4-bit bp parts seem to call the two status register protection bits srp and srl instead of srp{0,1} because they have slightly different behaviour. Change-Id: Ieca68d8628a2b0275d310ef5a77ffc76fe4dafe6 Signed-off-by: Daniel Gröber <dxld(a)darkboxed.org> --- M src/drivers/spi/winbond.c 1 file changed, 6 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/42116/1 diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c index 51d43a8..f68d9d9 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -19,14 +19,14 @@ uint8_t tb : 1; uint8_t sec : 1; uint8_t srp0 : 1; - } bp3; + } bp3; /* for example: W25Q128FW */ struct { uint8_t busy : 1; uint8_t wel : 1; uint8_t bp : 4; uint8_t tb : 1; - uint8_t srp0 : 1; - } bp4; + uint8_t srp : 1; + } bp4; /* for example: W25Q256J */ }; union status_reg2 { @@ -326,7 +326,7 @@ .cmp = reg2.cmp, .tb = reg1.bp4.tb, .winbond = { - .srp = reg1.bp4.srp0, + .srp = reg1.bp4.srp, .srl = reg2.srp1, }, }; @@ -497,8 +497,8 @@ val.reg1.bp3.srp0 = bpbits.winbond.srp0; mask.reg1.bp3.srp0 = 1; } else { - val.reg1.bp4.srp0 = bpbits.winbond.srp0; - mask.reg1.bp4.srp0 = 1; + val.reg1.bp4.srp = bpbits.winbond.srp0; + mask.reg1.bp4.srp = 1; } val.reg2.srp1 = bpbits.winbond.srp1; -- To view, visit https://review.coreboot.org/c/coreboot/+/42116 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ieca68d8628a2b0275d310ef5a77ffc76fe4dafe6 Gerrit-Change-Number: 42116 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Gröber (dxld) Gerrit-Reviewer: Daniel Gröber <dxld(a)darkboxed.org> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: soc/intel/xeon_sp/cpx: set up cpus
by Philipp Deppenwiese (Code Review) 06 Jun '20

06 Jun '20

Philipp Deppenwiese has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40112 ) Change subject: soc/intel/xeon_sp/cpx: set up cpus ...................................................................... soc/intel/xeon_sp/cpx: set up cpus Set up cpus: * setup apic IDs. * setup MSR to enable fast string, speed step, etc. * Enable turbo Signed-off-by: Jonathan Zhang <jonzhang(a)fb.com> Signed-off-by: Reddy Chagam <anjaneya.chagam(a)intel.com> Change-Id: I5765e98151f6ceebaabccc06db63d5911caf7ce8 Reviewed-on: https://review.coreboot.org/c/coreboot/+/40112 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> --- M src/soc/intel/xeon_sp/cpx/Makefile.inc M src/soc/intel/xeon_sp/cpx/cpu.c M src/soc/intel/xeon_sp/cpx/include/soc/cpu.h A src/soc/intel/xeon_sp/cpx/include/soc/msr.h M src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h M src/soc/intel/xeon_sp/cpx/soc_util.c 6 files changed, 336 insertions(+), 2 deletions(-) Approvals: build bot (Jenkins): Verified Philipp Deppenwiese: Looks good to me, approved diff --git a/src/soc/intel/xeon_sp/cpx/Makefile.inc b/src/soc/intel/xeon_sp/cpx/Makefile.inc index 54004a5..aca8572 100644 --- a/src/soc/intel/xeon_sp/cpx/Makefile.inc +++ b/src/soc/intel/xeon_sp/cpx/Makefile.inc @@ -2,6 +2,7 @@ ifeq ($(CONFIG_SOC_INTEL_COOPERLAKE_SP),y) +subdirs-y += ../../../../cpu/intel/turbo subdirs-y += ../../../../cpu/x86/lapic subdirs-y += ../../../../cpu/x86/mtrr subdirs-y += ../../../../cpu/x86/tsc diff --git a/src/soc/intel/xeon_sp/cpx/cpu.c b/src/soc/intel/xeon_sp/cpx/cpu.c index 1c33218..e2724ce 100644 --- a/src/soc/intel/xeon_sp/cpx/cpu.c +++ b/src/soc/intel/xeon_sp/cpx/cpu.c @@ -2,18 +2,49 @@ #include <acpi/acpigen.h> #include <acpi/acpi.h> +#include <assert.h> #include <console/console.h> #include <cpu/cpu.h> #include <cpu/intel/microcode.h> +#include <cpu/intel/turbo.h> #include <cpu/x86/lapic.h> #include <cpu/x86/mp.h> #include <cpu/x86/mtrr.h> #include <intelblocks/cpulib.h> #include <intelblocks/mp_init.h> #include <soc/cpu.h> +#include <soc/msr.h> +#include <soc/soc_util.h> +#include "chip.h" static const void *microcode_patch; +static const config_t *chip_config = NULL; + +static void xeon_configure_mca(void) +{ + msr_t msr; + struct cpuid_result cpuid_regs; + + /* + * Check feature flag in CPUID.(EAX=1):EDX[7]==1 MCE + * and CPUID.(EAX=1):EDX[14]==1 MCA + */ + cpuid_regs = cpuid(1); + if ((cpuid_regs.edx & (1 << 7 | 1 << 14)) != (1 << 7 | 1 << 14)) + return; + + msr = rdmsr(IA32_MCG_CAP); + if (msr.lo & IA32_MCG_CAP_CTL_P_MASK) { + /* Enable all error logging */ + msr.lo = msr.hi = 0xffffffff; + wrmsr(IA32_MCG_CTL, msr); + } + + mca_configure(); +} + + void get_microcode_info(const void **microcode, int *parallel) { *microcode = intel_mp_current_microcode(); @@ -27,10 +58,28 @@ static void each_cpu_init(struct device *cpu) { + msr_t msr; + printk(BIOS_SPEW, "%s dev: %s, cpu: %d, apic_id: 0x%x\n", __func__, dev_path(cpu), cpu_index(), cpu->path.apic.apic_id); - setup_lapic(); + + /* Enable Fast Strings */ + msr = rdmsr(IA32_MISC_ENABLE); + msr.lo |= FAST_STRINGS_ENABLE_BIT; + wrmsr(IA32_MISC_ENABLE, msr); + /* Enable Turbo */ + enable_turbo(); + + /* Enable speed step. */ + if (get_turbo_state() == TURBO_ENABLED) { + msr = rdmsr(IA32_MISC_ENABLE); + msr.lo |= SPEED_STEP_ENABLE_BIT; + wrmsr(IA32_MISC_ENABLE, msr); + } + + /* Clear out pending MCEs */ + xeon_configure_mca(); } static struct device_operations cpu_dev_ops = { @@ -47,6 +96,33 @@ .id_table = cpu_table, }; +static void set_max_turbo_freq(void) +{ + msr_t msr, perf_ctl; + + FUNC_ENTER(); + perf_ctl.hi = 0; + + /* Check for configurable TDP option */ + if (get_turbo_state() == TURBO_ENABLED) { + msr = rdmsr(MSR_TURBO_RATIO_LIMIT); + perf_ctl.lo = (msr.lo & 0xff) << 8; + } else if (cpu_config_tdp_levels()) { + /* Set to nominal TDP ratio */ + msr = rdmsr(MSR_CONFIG_TDP_NOMINAL); + perf_ctl.lo = (msr.lo & 0xff) << 8; + } else { + /* Platform Info bits 15:8 give max ratio */ + msr = rdmsr(MSR_PLATFORM_INFO); + perf_ctl.lo = msr.lo & 0xff00; + } + wrmsr(IA32_PERF_CTL, perf_ctl); + + printk(BIOS_DEBUG, "cpu: frequency set to %d\n", + ((perf_ctl.lo >> 8) & 0xff) * CPU_BCLK); + FUNC_EXIT(); +} + /* * Do essential initialization tasks before APs can be fired up */ @@ -72,10 +148,17 @@ return num_virts * CONFIG_MAX_SOCKET; } +static void post_mp_init(void) +{ + /* Set Max Ratio */ + set_max_turbo_freq(); +} + static const struct mp_ops mp_ops = { .pre_mp_init = pre_mp_init, .get_cpu_count = get_thread_count, - .get_microcode_info = get_microcode_info + .get_microcode_info = get_microcode_info, + .post_mp_init = post_mp_init, }; void cpx_init_cpus(struct device *dev) @@ -89,4 +172,13 @@ if (mp_init_with_smm(dev->link_list, &mp_ops) < 0) printk(BIOS_ERR, "MP initialization failure.\n"); + + /* + * chip_config is used in cpu device callback. Other than cpu 0, + * rest of the CPU devices do not have chip_info updated. + */ + chip_config = dev->chip_info; + + /* update numa domain for all cpu devices */ + xeonsp_init_cpu_config(); } diff --git a/src/soc/intel/xeon_sp/cpx/include/soc/cpu.h b/src/soc/intel/xeon_sp/cpx/include/soc/cpu.h index 7f2f6cd..2d12699 100644 --- a/src/soc/intel/xeon_sp/cpx/include/soc/cpu.h +++ b/src/soc/intel/xeon_sp/cpx/include/soc/cpu.h @@ -7,6 +7,9 @@ #define CPUID_COOPERLAKE_SP_A0 0x05065a +/* CPU bus clock is fixed at 100MHz */ +#define CPU_BCLK 100 + void cpx_init_cpus(struct device *dev); #endif diff --git a/src/soc/intel/xeon_sp/cpx/include/soc/msr.h b/src/soc/intel/xeon_sp/cpx/include/soc/msr.h new file mode 100644 index 0000000..f9d59f1 --- /dev/null +++ b/src/soc/intel/xeon_sp/cpx/include/soc/msr.h @@ -0,0 +1,99 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef _SOC_MSR_H_ +#define _SOC_MSR_H_ + +#include <intelblocks/msr.h> + +#define IA32_MCG_CAP 0x179 +#define IA32_MCG_CAP_COUNT_MASK 0xff +#define IA32_MCG_CAP_CTL_P_BIT 8 +#define IA32_MCG_CAP_CTL_P_MASK (1 << IA32_MCG_CAP_CTL_P_BIT) + +#define IA32_MCG_CTL 0x17b + +/* IA32_MISC_ENABLE bits */ +#define FAST_STRINGS_ENABLE_BIT (1 << 0) +#define SPEED_STEP_ENABLE_BIT (1 << 16) +#define MONIOR_ENABLE_BIT (1 << 18) + +#define MSR_IA32_ENERGY_PERF_BIAS 0x1b0 + +/* MSR_PKG_CST_CONFIG_CONTROL bits */ +#define MSR_PKG_CST_CONFIG_CONTROL 0xe2 +#define PKG_CSTATE_LIMIT_SHIFT 0 /* 0:3 */ +/* No package C-state limit. All C-States supported by the processor are available. */ +#define PKG_CSTATE_LIMIT_MASK (0xf << PKG_CSTATE_LIMIT_SHIFT) +#define PKG_CSTATE_NO_LIMIT (0x7 << PKG_CSTATE_LIMIT_SHIFT) +#define IO_MWAIT_REDIRECTION_SHIFT 10 +#define IO_MWAIT_REDIRECTION_ENABLE (1 << IO_MWAIT_REDIRECTION_SHIFT) +#define CFG_LOCK_SHIFT 15 +#define CFG_LOCK_ENABLE (1 << CFG_LOCK_SHIFT) + +/* MSR_PMG_IO_CAPTURE_BASE bits */ +#define MSR_PMG_IO_CAPTURE_BASE 0xe4 +#define LVL_2_BASE_ADDRESS_SHIFT 0 /* 15:0 bits */ +#define LVL_2_BASE_ADDRESS (0x0514 << LVL_2_BASE_ADDRESS_SHIFT) +#define CST_RANGE_SHIFT 16 /* 18:16 bits */ +#define CST_RANGE_MAX_C6 (0x1 << CST_RANGE_SHIFT) + +/* MSR_POWER_CTL bits */ +#define MSR_POWER_CTL 0x1fc +#define BIDIR_PROCHOT_ENABLE_SHIFT 0 +#define BIDIR_PROCHOT_ENABLE (1 << BIDIR_PROCHOT_ENABLE_SHIFT) +#define FAST_BRK_SNP_ENABLE_SHIFT 3 +#define FAST_BRK_SNP_ENABLE (1 << FAST_BRK_SNP_ENABLE_SHIFT) +#define FAST_BRK_INT_ENABLE_SHIFT 4 +#define FAST_BRK_INT_ENABLE (1 << FAST_BRK_INT_ENABLE_SHIFT) +#define PHOLD_CST_PREVENTION_INIT_SHIFT 6 +#define PHOLD_CST_PREVENTION_INIT_VALUE (1 << PHOLD_CST_PREVENTION_INIT_SHIFT) +#define ENERGY_PERF_BIAS_ACCESS_ENABLE_SHIFT 18 +#define ENERGY_PERF_BIAS_ACCESS_ENABLE (1 << ENERGY_PERF_BIAS_ACCESS_ENABLE_SHIFT) +#define PROCHOT_OUTPUT_DISABLE_SHIFT 21 +#define PROCHOT_OUTPUT_DISABLE (1 << PROCHOT_OUTPUT_DISABLE_SHIFT) +#define PWR_PERF_TUNING_DYN_SWITCHING_SHIFT 24 +#define PWR_PERF_TUNING_DYN_SWITCHING_ENABLE (1 << PWR_PERF_TUNING_DYN_SWITCHING_SHIFT) +#define PROCHOT_LOCK_SHIFT 27 +#define PROCHOT_LOCK_ENABLE (1 << PROCHOT_LOCK_SHIFT) +#define LTR_IIO_DISABLE_SHIFT 29 +#define LTR_IIO_DISABLE (1 << LTR_IIO_DISABLE_SHIFT) + +/* MSR_IA32_PERF_CTRL (0x199) bits */ +#define MSR_IA32_PERF_CTRL 0x199 +#define PSTATE_REQ_SHIFT 8 /* 8:14 bits */ +#define PSTATE_REQ_MASK (0x7f << PSTATE_REQ_SHIFT) +#define PSTATE_REQ_RATIO (0xa << PSTATE_REQ_SHIFT) + +/* MSR_MISC_PWR_MGMT bits */ +#define MSR_MISC_PWR_MGMT 0x1aa +#define HWP_ENUM_SHIFT 6 +#define HWP_ENUM_ENABLE (1 << HWP_ENUM_SHIFT) +#define HWP_EPP_SHIFT 12 +#define HWP_EPP_ENUM_ENABLE (1 << HWP_EPP_SHIFT) +#define LOCK_MISC_PWR_MGMT_MSR_SHIFT 13 +#define LOCK_MISC_PWR_MGMT_MSR (1 << LOCK_MISC_PWR_MGMT_MSR_SHIFT) +#define LOCK_THERM_INT_SHIFT 22 +#define LOCK_THERM_INT (1 << LOCK_THERM_INT_SHIFT) + +/* MSR_TURBO_RATIO_LIMIT bits */ +#define MSR_TURBO_RATIO_LIMIT 0x1ad + +/* MSR_TURBO_RATIO_LIMIT_CORES (0x1ae) */ +#define MSR_TURBO_RATIO_LIMIT_CORES 0x1ae + +/* MSR_VR_CURRENT_CONFIG bits */ +#define MSR_VR_CURRENT_CONFIG 0x601 +#define CURRENT_LIMIT_LOCK_SHIFT 31 +#define CURRENT_LIMIT_LOCK (0x1 << CURRENT_LIMIT_LOCK_SHIFT) + +/* MSR_TURBO_ACTIVATION_RATIO bits */ +#define MSR_TURBO_ACTIVATION_RATIO 0x64c +#define MAX_NON_TURBO_RATIO_SHIFT 0 +#define MAX_NON_TURBO_RATIO (0xff << MAX_NON_TURBO_RATIO_SHIFT) + +/* MSR_ENERGY_PERF_BIAS_CONFIG bits */ +#define MSR_ENERGY_PERF_BIAS_CONFIG 0xa01 +#define EPB_ENERGY_POLICY_SHIFT 3 +#define EPB_ENERGY_POLICY_MASK (0xf << EPB_ENERGY_POLICY_SHIFT) + +#endif /* _SOC_MSR_H_ */ diff --git a/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h b/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h index d8b038c..679ad4a 100644 --- a/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h +++ b/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h @@ -16,10 +16,24 @@ printk(BIOS_SPEW, "%s:%s:%d: EXIT (dev: %s)\n", __FILE__, \ __func__, __LINE__, dev_path(dev)) +#define FUNC_ENTER() \ + printk(BIOS_SPEW, "%s:%s:%d: ENTER\n", __FILE__, __func__, __LINE__) + +#define FUNC_EXIT() \ + printk(BIOS_SPEW, "%s:%s:%d: EXIT\n", __FILE__, __func__, __LINE__) + struct iiostack_resource { uint8_t no_of_stacks; STACK_RES res[MAX_SOCKET * MAX_LOGIC_IIO_STACK]; }; uint8_t get_iiostack_info(struct iiostack_resource *info); + +void xeonsp_init_cpu_config(void); +const IIO_UDS *get_iio_uds(void); +void get_core_thread_bits(uint32_t *core_bits, uint32_t *thread_bits); +void get_cpu_info_from_apicid(uint32_t apicid, uint32_t core_bits, uint32_t thread_bits, + uint8_t *package, uint8_t *core, uint8_t *thread); +unsigned int xeon_sp_get_cpu_count(void); + #endif /* _SOC_UTIL_H_ */ diff --git a/src/soc/intel/xeon_sp/cpx/soc_util.c b/src/soc/intel/xeon_sp/cpx/soc_util.c index 8548615..2d10657 100644 --- a/src/soc/intel/xeon_sp/cpx/soc_util.c +++ b/src/soc/intel/xeon_sp/cpx/soc_util.c @@ -1,10 +1,135 @@ /* SPDX-License-Identifier: GPL-2.0-only */ #include <assert.h> +#include <commonlib/sort.h> +#include <device/device.h> +#include <intelblocks/cpulib.h> +#include <soc/cpu.h> #include <soc/soc_util.h> #include <stdlib.h> #include <string.h> +void get_cpu_info_from_apicid(uint32_t apicid, uint32_t core_bits, uint32_t thread_bits, + uint8_t *package, uint8_t *core, uint8_t *thread) +{ + if (package != NULL) + *package = (apicid >> (thread_bits + core_bits)); + if (core != NULL) + *core = (uint32_t)((apicid >> thread_bits) & ~((~0) << core_bits)); + if (thread != NULL) + *thread = (uint32_t)(apicid & ~((~0) << thread_bits)); +} + +void get_core_thread_bits(uint32_t *core_bits, uint32_t *thread_bits) +{ + register int ecx; + struct cpuid_result cpuid_regs; + + /* get max index of CPUID */ + cpuid_regs = cpuid(0); + assert(cpuid_regs.eax >= 0xb); /* cpuid_regs.eax is max input value for cpuid */ + + *thread_bits = *core_bits = 0; + ecx = 0; + while (1) { + cpuid_regs = cpuid_ext(0xb, ecx); + if (ecx == 0) { + *thread_bits = (cpuid_regs.eax & 0x1f); + } else { + *core_bits = (cpuid_regs.eax & 0x1f) - *thread_bits; + break; + } + ecx++; + } +} + +const IIO_UDS *get_iio_uds(void) +{ + size_t hob_size; + const IIO_UDS *hob; + const uint8_t fsp_hob_iio_universal_data_guid[16] = FSP_HOB_IIO_UNIVERSAL_DATA_GUID; + + hob = fsp_find_extension_hob_by_guid(fsp_hob_iio_universal_data_guid, &hob_size); + assert(hob != NULL && hob_size != 0); + return hob; +} + +unsigned int xeon_sp_get_cpu_count(void) +{ + return get_iio_uds()->SystemStatus.numCpus; +} + +void xeonsp_init_cpu_config(void) +{ + struct device *dev; + int apic_ids[CONFIG_MAX_CPUS] = {0}, apic_ids_by_thread[CONFIG_MAX_CPUS] = {0}; + int num_apics = 0; + uint32_t core_bits, thread_bits; + unsigned int core_count, thread_count; + unsigned int num_cpus; + + /* + * sort APIC ids in asending order to identify apicid ranges for + * each numa domain + */ + for (dev = all_devices; dev; dev = dev->next) { + if ((dev->path.type != DEVICE_PATH_APIC) || + (dev->bus->dev->path.type != DEVICE_PATH_CPU_CLUSTER)) { + continue; + } + if (!dev->enabled) + continue; + if (num_apics >= ARRAY_SIZE(apic_ids)) + break; + apic_ids[num_apics++] = dev->path.apic.apic_id; + } + if (num_apics > 1) + bubblesort(apic_ids, num_apics, NUM_ASCENDING); + + /* Here num_cpus is the number of processors */ + /* The FSP HOB parameter has it named as num_cpus */ + num_cpus = xeon_sp_get_cpu_count(); + cpu_read_topology(&core_count, &thread_count); + assert(num_apics == (num_cpus * thread_count)); + + /* sort them by thread i.e., all cores with thread 0 and then thread 1 */ + int index = 0; + for (int id = 0; id < num_apics; ++id) { + int apic_id = apic_ids[id]; + if (apic_id & 0x1) { /* 2nd thread */ + apic_ids_by_thread[index + (num_apics/2) - 1] = apic_id; + } else { /* 1st thread */ + apic_ids_by_thread[index++] = apic_id; + } + } + + + /* update apic_id, node_id in sorted order */ + num_apics = 0; + get_core_thread_bits(&core_bits, &thread_bits); + for (dev = all_devices; dev; dev = dev->next) { + uint8_t package; + + if ((dev->path.type != DEVICE_PATH_APIC) || + (dev->bus->dev->path.type != DEVICE_PATH_CPU_CLUSTER)) { + continue; + } + if (!dev->enabled) + continue; + if (num_apics >= ARRAY_SIZE(apic_ids)) + break; + dev->path.apic.apic_id = apic_ids_by_thread[num_apics]; + get_cpu_info_from_apicid(dev->path.apic.apic_id, core_bits, thread_bits, + &package, NULL, NULL); + dev->path.apic.node_id = package; + printk(BIOS_DEBUG, "CPU %d apic_id: 0x%x (%d), node_id: 0x%x\n", + num_apics, dev->path.apic.apic_id, + dev->path.apic.apic_id, dev->path.apic.node_id); + + ++num_apics; + } +} + uint8_t get_iiostack_info(struct iiostack_resource *info) { size_t hob_size; -- To view, visit https://review.coreboot.org/c/coreboot/+/40112 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5765e98151f6ceebaabccc06db63d5911caf7ce8 Gerrit-Change-Number: 40112 Gerrit-PatchSet: 20 Gerrit-Owner: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Anjaneya "Reddy" Chagam <anjaneya.chagam(a)intel.com> Gerrit-Reviewer: Jingle Hsu <jingle_hsu(a)wiwynn.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Morgan Jang Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: drivers/intel/fsp2_0: Add FSP 2.2 specific support
by Subrata Banik (Code Review) 06 Jun '20

06 Jun '20

Hello build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Maulik V Vaghela, Duncan Laurie, Sridhar Siricilla, Aamir Bohra, Srinidhi N Kaushik, Andrey Petrov, Aaron Durbin, Patrick Rudolph, Nathaniel L Desimone, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/41728 to look at the new patch set (#12). Change subject: drivers/intel/fsp2_0: Add FSP 2.2 specific support ...................................................................... drivers/intel/fsp2_0: Add FSP 2.2 specific support • Based on FSP EAS v2.1 – Backward compatibility is retained. • Added multi-phase silicon initialization to increase the modularity of the FspSiliconInit() API. • Added FspMultiPhaseSiInit() API • FSP_INFO_HEADER changes o Added FspMultiPhaseSiInitEntryOffset • Added FSPS_ARCH_UPD o Added EnableMultiPhaseSiliconInit, bootloaders designed for FSP 2.0/2.1 can disable the FspMultiPhaseSiInit() API and continue to use FspSiliconInit() without change. FSP 2.2 Specification: https://www.intel.com/content/www/us/en/intelligent-systems/intel-firmware-… Change-Id: If7177a267f3a9b4cbb60a639f1c737b9a3341913 Signed-off-by: Subrata Banik <subrata.banik(a)intel.com> --- M src/commonlib/include/commonlib/timestamp_serialized.h M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/include/fsp/api.h M src/drivers/intel/fsp2_0/include/fsp/info_header.h M src/drivers/intel/fsp2_0/include/fsp/upd.h M src/drivers/intel/fsp2_0/include/fsp/util.h M src/drivers/intel/fsp2_0/silicon_init.c M src/drivers/intel/fsp2_0/util.c M src/include/console/post_codes.h 9 files changed, 172 insertions(+), 13 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/41728/12 -- To view, visit https://review.coreboot.org/c/coreboot/+/41728 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If7177a267f3a9b4cbb60a639f1c737b9a3341913 Gerrit-Change-Number: 41728 Gerrit-PatchSet: 12 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Nathaniel L Desimone <nathaniel.l.desimone(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: drivers/intel/fsp2_0: Add FSP 2.2 specific support
by Subrata Banik (Code Review) 06 Jun '20

06 Jun '20

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41728 ) Change subject: drivers/intel/fsp2_0: Add FSP 2.2 specific support ...................................................................... Patch Set 11: (4 comments) https://review.coreboot.org/c/coreboot/+/41728/11/src/drivers/intel/fsp2_0/… File src/drivers/intel/fsp2_0/Kconfig: https://review.coreboot.org/c/coreboot/+/41728/11/src/drivers/intel/fsp2_0/… PS11, Line 27: impacts > impact Ack https://review.coreboot.org/c/coreboot/+/41728/11/src/drivers/intel/fsp2_0/… File src/drivers/intel/fsp2_0/include/fsp/info_header.h: https://review.coreboot.org/c/coreboot/+/41728/11/src/drivers/intel/fsp2_0/… PS11, Line 33: size_t multi_phase_si_init_entry_offset; > Doesn't this make the struct layout incompatible with FSP 2. […] not actually as its not a packed structure that mean we don't use this structure directly anywhere rather we use dedicated field hence it won't break anything, tested on FSP2.0 and FSP2.1 platform already https://review.coreboot.org/c/coreboot/+/41728/11/src/drivers/intel/fsp2_0/… File src/drivers/intel/fsp2_0/include/fsp/util.h: https://review.coreboot.org/c/coreboot/+/41728/11/src/drivers/intel/fsp2_0/… PS11, Line 36: uint32_t phase_index > Are these phase indices enumerated? If so, I'd use an enum for this we really don't know how many phase FSP will populate hence we can't use enum here. what is possible been mentioned at https://review.coreboot.org/c/coreboot/+/41728/11/src/drivers/intel/fsp2_0/… Here is the flow 1. Check if FSP header has implemented multiphasesiinit API 2. If yes, then first call multiphasesiinit API with GET_NUMBER_OF_PHASES (0) to know the number of phase supported 3.Based on return value from #2, we call each phase index (from 1 till struct fsp_multi_phase_params.phase_index) multiphasesiinit API with EXECUTE_PHASE (1) https://review.coreboot.org/c/coreboot/+/41728/11/src/drivers/intel/fsp2_0/… File src/drivers/intel/fsp2_0/silicon_init.c: https://review.coreboot.org/c/coreboot/+/41728/11/src/drivers/intel/fsp2_0/… PS11, Line 57: BIOS_SPEW > BIOS_EMERG […] Thanks 😊 -- To view, visit https://review.coreboot.org/c/coreboot/+/41728 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If7177a267f3a9b4cbb60a639f1c737b9a3341913 Gerrit-Change-Number: 41728 Gerrit-PatchSet: 11 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Nathaniel L Desimone <nathaniel.l.desimone(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Comment-Date: Sat, 06 Jun 2020 07:22:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/intel/jasperlake_rvp: camera SSDT changes for JSLRVP
by Tim Wawrzynczak (Code Review) 05 Jun '20

05 Jun '20

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41758 ) Change subject: mb/intel/jasperlake_rvp: camera SSDT changes for JSLRVP ...................................................................... Patch Set 5: (2 comments) https://review.coreboot.org/c/coreboot/+/41758/5/src/mainboard/intel/jasper… File src/mainboard/intel/jasperlake_rvp/variants/jslrvp/devicetree.cb: https://review.coreboot.org/c/coreboot/+/41758/5/src/mainboard/intel/jasper… PS5, Line 320: register "on_seq.ops[0].type" = "IMGCLK" : register "on_seq.ops[0].index" = "0" #clk enable : register "on_seq.ops[0].action" = "ENABLE" suggestion: making a macro to build these structs could make this easier to read. maybe: #define SEQ_OPS_CLK(index, act) \ {.type=IMGCLK, .index = (index), .action = (act) } and #define SEQ_OPS_GPIO(index, act, delay) \ {.type=GPIO, .index = (index), .action = (act), .delay_ms = (delay) } etc. it would also reduce each one to one line which greatly shortens the devicetree 😊 https://review.coreboot.org/c/coreboot/+/41758/5/src/mainboard/intel/jasper… PS5, Line 446: register "cio2_lanes_used[0]" = "2" : register "cio2_lanes_used[1]" = "2" could be register "cio2_lanes_used" = "{2, 2}" -- To view, visit https://review.coreboot.org/c/coreboot/+/41758 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib439572bc1d15ef02c86c7bfa88af6b16eb06f97 Gerrit-Change-Number: 41758 Gerrit-PatchSet: 5 Gerrit-Owner: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com> Gerrit-Reviewer: Daniel Kang <daniel.h.kang(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Varshit B Pandya <varshit.b.pandya(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-CC: Kiran2 Kumar <kiran2.kumar(a)intel.corp-partner.google.com> Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Comment-Date: Sat, 06 Jun 2020 02:03:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: acpi: Add support for call method and store value
by Tim Wawrzynczak (Code Review) 05 Jun '20

05 Jun '20

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41620 ) Change subject: acpi: Add support for call method and store value ...................................................................... Patch Set 10: (2 comments) https://review.coreboot.org/c/coreboot/+/41620/5/src/acpi/acpigen.c File src/acpi/acpigen.c: https://review.coreboot.org/c/coreboot/+/41620/5/src/acpi/acpigen.c@1928 PS5, Line 1928: void acpigen_call_method(const char *name, unsigned char *params, int params_len) : { : int i; : char ud[] = "____"; : for (i = 0; i < 4; i++) { : if ((name[i] == '\0')) { : acpigen_emit_stream(ud, 4 - i); : break; : } : acpigen_emit_byte(name[i]); : } : : for (i = 0; i < params_len; i++) : acpigen_write_byte(params[i]); : } Why is this necessary? https://review.coreboot.org/c/coreboot/+/41620/5/src/acpi/acpigen.c@1944 PS5, Line 1944: void acpigen_write_store_var(const char *name, uint8_t value) : { : int i; : char ud[] = "____"; : : acpigen_write_store(); : acpigen_emit_byte(value); : : for (i = 0; i < 4; i++) { : if ((name[i] == '\0')) { : acpigen_emit_stream(ud, 4 - i); : break; : } : acpigen_emit_byte(name[i]); : } : } Why is this necessary? -- To view, visit https://review.coreboot.org/c/coreboot/+/41620 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibcb67bd756dc8ae1dfd2d40020c273325583df0f Gerrit-Change-Number: 41620 Gerrit-PatchSet: 10 Gerrit-Owner: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com> Gerrit-Reviewer: Daniel Kang <daniel.h.kang(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Reviewer: Varshit B Pandya <varshit.b.pandya(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Kiran2 Kumar <kiran2.kumar(a)intel.corp-partner.google.com> Gerrit-Comment-Date: Sat, 06 Jun 2020 01:57:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: drivers/intel/mipi_camera: camera SSDT generation
by Tim Wawrzynczak (Code Review) 05 Jun '20

05 Jun '20

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41607 ) Change subject: drivers/intel/mipi_camera: camera SSDT generation ...................................................................... Patch Set 11: (12 comments) https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca… File src/drivers/intel/mipi_camera/camera.c: https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca… PS11, Line 54: struct acpi_dp *lanes = acpi_dp_new_table("data-lanes"); : uint32_t j; in coreboot, we usually declare all local variables at the top of the function. https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca… PS11, Line 94: if (!config->pld.ignore_color) : config->pld.ignore_color = 1; : : if (!config->pld.visible) : config->pld.visible = 1; : : if (!config->pld.vertical_offset) : config->pld.vertical_offset = 0xffff; : : if (!config->pld.horizontal_offset) : config->pld.horizontal_offset = 0xffff; : : /* : * PLD_PANEL_TOP has a value of zero, so the following will change any instance : * of PLD_PANEL_TOP to PLD_PANEL_FRONT unless disable_pld_defaults is set. : */ : if (!config->pld.panel) : config->pld.panel = PLD_PANEL_FRONT; : : /* : * PLD_HORIZONTAL_POSITION_LEFT has a value of zero, so the following will : * change any instance of that value to PLD_HORIZONTAL_POSITION_CENTER unless : * disable_pld_defaults is set. : */ : if (!config->pld.horizontal_position) : config->pld.horizontal_position = PLD_HORIZONTAL_POSITION_CENTER; : : /* : * The desired default for |vertical_position| is PLD_VERTICAL_POSITION_UPPER, : * which has a value of zero so no work is needed to set a default. The same : * applies for setting |shape| to PLD_SHAPE_ROUND. : */ suggestion: break this out into a separate function https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca… PS11, Line 139: 0x4 4 https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca… PS11, Line 148: 822ace8f-2814-4174-a56b-5f029fe079ee a symbolic constant with a nice name would be helpful https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca… PS11, Line 156: 26257549-9271-4ca4-bb43-c4899d5a4881 a symbolic constant with a nice name would be helpful https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca… PS11, Line 167: acpigen_write_return_integer(((uint32_t)i2c_bus) << 24 | : ((uint32_t)i2c_addr) << 8 | 0x0); /* type 0 for sensor */ : acpigen_pop_len(); /* If Arg2=2 */ : : if (config->ssdb.vcm_type) { : /* If (LEqual (Local1, 3)) */ : acpigen_write_if_lequal_op_int(LOCAL1_OP, 3); : acpigen_write_return_integer(((uint32_t)i2c_bus) << 24 | /* type 1 for vcm */ : ((uint32_t)config->vcm_address) << 8 | 0x01); : acpigen_pop_len(); /* If Arg2=3 */ : } : : if (config->ssdb.rom_type) { : /* If (LEqual (Local1, 3 or 4)) */ : acpigen_write_if_lequal_op_int(LOCAL1_OP, 3 + (config->ssdb.vcm_type ? 1 : 0)); : acpigen_write_return_integer(((uint32_t)i2c_bus) << 24 | /* type 2 for rom */ : ((uint32_t)config->rom_address) << 8 | 0x02); : acpigen_pop_len(); /* If Arg2=3 or 4 */ suggestion: add a helper function for creating these return values, with maybe an enum per type, sensor, vcm or rom https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca… PS11, Line 229: if (!config->disable_ssdb_defaults) { : if (!config->ssdb.bdf_value) : config->ssdb.bdf_value = PCI_DEVFN(CIO2_PCI_DEV, CIO2_PCI_FN); : : if (!config->ssdb.platform) : config->ssdb.platform = PLATFORM_SKC; : : if (!config->ssdb.flash_support) : config->ssdb.flash_support = FLASH_DISABLE; : : if (!config->ssdb.privacy_led) : config->ssdb.privacy_led = PRIVACY_LED_A_16mA; : : if (!config->ssdb.mipi_define) : config->ssdb.mipi_define = MIPI_INFO_ACPI_DEFINED; : : if (!config->ssdb.mclk_speed) : config->ssdb.mclk_speed = CLK_FREQ_19_2MHZ; : : if (!config->ssdb.lanes_used) { : cio2_config = cio2 ? cio2->chip_info : NULL; : : if (!cio2_config) { : printk(BIOS_ERR, "Failed to get CIO2 config\n"); : } else if (cio2_config->device_type != INTEL_ACPI_CAMERA_CIO2) { : printk(BIOS_ERR, "Device type isn't CIO2: %u\n", : (u32)cio2_config->device_type); : } else if (config->ssdb.link_used >= cio2_config->cio2_num_ports) { : printk(BIOS_ERR, "%u exceeds CIO2's %u links\n", : (u32)config->ssdb.link_used, : (u32)cio2_config->cio2_num_ports); : } else { : config->ssdb.lanes_used = : cio2_config->cio2_lanes_used[config->ssdb.link_used]; : } : } : } suggestion: same here, separate this out into a separate function https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca… PS11, Line 272: 0 Is this always supposed to be 0? https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca… PS11, Line 298: remote_name = "\\_SB.PCI0.CIO2"; Could you add this to the rest of the defaults? https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca… PS11, Line 303: /* endpoint? */ ? https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca… PS11, Line 315: 180 always 180? https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca… PS11, Line 566: if (config->device_type == INTEL_ACPI_CAMERA_CIO2) : return "CIO2"; : else if (config->device_type == INTEL_ACPI_CAMERA_IMGU) : return "IMGU"; : else if (config->device_type == INTEL_ACPI_CAMERA_PMIC) : return "PMIC"; : : switch (config->device_type) { : case INTEL_ACPI_CAMERA_SENSOR: : prefix = "CAM"; : break; : case INTEL_ACPI_CAMERA_VCM: : prefix = "VCM"; : break; : case INTEL_ACPI_CAMERA_NVM: : prefix = "NVM"; : break; : default: : prefix = "ERR"; /* Error */ : break; : } This can just be one switch statement; you can just return from the cases that have all 4 characters already, and let the prefixed ones keep going. -- To view, visit https://review.coreboot.org/c/coreboot/+/41607 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I15979f345fb823df2560db269e902a1ea650b69e Gerrit-Change-Number: 41607 Gerrit-PatchSet: 11 Gerrit-Owner: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com> Gerrit-Reviewer: Daniel Kang <daniel.h.kang(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Matt Delco <delco(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Reviewer: Varshit B Pandya <varshit.b.pandya(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Kiran2 Kumar <kiran2.kumar(a)intel.corp-partner.google.com> Gerrit-Comment-Date: Sat, 06 Jun 2020 01:57:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit June 2020