coreboot-gerrit June 2021

coreboot-gerrit@coreboot.org

1 participants
7127 discussions

Change in coreboot[master]: soc/amd/common/fsp/pci: Add size field to PCIe interrupt routing HOB
by Raul Rangel (Code Review) 04 Jun '21

04 Jun '21

Attention is currently required from: Marshall Dawson, Nikolai Vyssotski, Matt Papageorge, Felix Held. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55237 ) Change subject: soc/amd/common/fsp/pci: Add size field to PCIe interrupt routing HOB ...................................................................... Patch Set 3: (3 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/55237/comment/b483784d_ea8eddd8 PS3, Line 18: Change Add a Cq-depend: chrome-internal:3889619 File src/soc/amd/common/block/include/amdblocks/amd_pci_util.h: https://review.coreboot.org/c/coreboot/+/55237/comment/da92f478_4e3b8708 PS3, Line 55: struct pci_routing_info_hob { : uint32_t num_of_entries; : struct pci_routing_info routing_table[]; : } __packed; : Can you move this struct into pci_routing_info.c. It's an implementation detail of FSP. File src/soc/amd/common/fsp/pci/pci_routing_info.c: https://review.coreboot.org/c/coreboot/+/55237/comment/9c9b6746_ecab754f PS3, Line 33: d u? -- To view, visit https://review.coreboot.org/c/coreboot/+/55237 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iaafae304f04a5f26d75a41a6d6fcb4ee69954d20 Gerrit-Change-Number: 55237 Gerrit-PatchSet: 3 Gerrit-Owner: Nikolai Vyssotski <nikolai.vyssotski(a)amd.corp-partner.google.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Matt Papageorge <matthewpapa07(a)gmail.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Nikolai Vyssotski <nikolai.vyssotski(a)amd.corp-partner.google.com> Gerrit-Attention: Matt Papageorge <matthewpapa07(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Fri, 04 Jun 2021 21:14:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: [RFC] pc80/rtc/option: Move register backup for SMI
by build bot (Jenkins) (Code Review) 04 Jun '21

04 Jun '21

Attention is currently required from: Alexander Couzens, Patrick Rudolph. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49253 ) Change subject: [RFC] pc80/rtc/option: Move register backup for SMI ...................................................................... Patch Set 7: (2 comments) File src/drivers/pc80/rtc/option.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-120813): https://review.coreboot.org/c/coreboot/+/49253/comment/9225111d_89e6bf57 PS7, Line 202: trailing whitespace Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-120813): https://review.coreboot.org/c/coreboot/+/49253/comment/fbfdd5dd_d85ffa6b PS7, Line 207: trailing whitespace -- To view, visit https://review.coreboot.org/c/coreboot/+/49253 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I025e9952a949834755f5cbc3dfe7b26ab9aaa032 Gerrit-Change-Number: 49253 Gerrit-PatchSet: 7 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-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Alexander Couzens <lynxis(a)fe80.eu> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Fri, 04 Jun 2021 21:11:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: [WIP,RFC] sb,soc/intel: Remove option power on after fail from SMI
by Kyösti Mälkki (Code Review) 04 Jun '21

04 Jun '21

Attention is currently required from: Patrick Rudolph. Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/55238 ) Change subject: [WIP,RFC] sb,soc/intel: Remove option power_on_after_fail from SMI ...................................................................... [WIP,RFC] sb,soc/intel: Remove option power_on_after_fail from SMI The mechanism was flawed. In the case that nvram contents were modified runtime, it still required entry to ACPI S5 for the related control registers in PCI config space to be written. Should it happen that there was a power failure before a first proper shutdown, new option setting would not be honoured. Change-Id: I4ae6aa08177d4915a7acd5a53e0aae1e968e60a2 Signed-off-by: Kyösti Mälkki <kyosti.malkki(a)gmail.com> --- M src/soc/intel/broadwell/pch/smihandler.c M src/southbridge/intel/common/smihandler.c M src/southbridge/intel/lynxpoint/smihandler.c 3 files changed, 6 insertions(+), 63 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/55238/1 diff --git a/src/soc/intel/broadwell/pch/smihandler.c b/src/soc/intel/broadwell/pch/smihandler.c index a8e2067..43d4fba 100644 --- a/src/soc/intel/broadwell/pch/smihandler.c +++ b/src/soc/intel/broadwell/pch/smihandler.c @@ -13,7 +13,6 @@ #include <spi-generic.h> #include <elog.h> #include <halt.h> -#include <option.h> #include <soc/lpc.h> #include <soc/nvs.h> #include <soc/pci_devs.h> @@ -125,21 +124,6 @@ printk(BIOS_INFO, "Backlight turned off\n"); } -static int power_on_after_fail(void) -{ - /* save and recover RTC port values */ - u8 tmp70, tmp72; - tmp70 = inb(0x70); - tmp72 = inb(0x72); - const unsigned int s5pwr = get_uint_option("power_on_after_fail", - CONFIG_MAINBOARD_POWER_FAILURE_STATE); - outb(tmp70, 0x70); - outb(tmp72, 0x72); - - /* For "KEEP", switch to "OFF" - KEEP is software emulated. */ - return (s5pwr == MAINBOARD_POWER_ON); -} - static void southbridge_smi_sleep(void) { u32 reg32; @@ -195,11 +179,8 @@ /* Disable all GPE */ disable_all_gpe(); - /* Always set the flag in case CMOS was changed on runtime. */ - if (power_on_after_fail()) - pci_and_config8(PCH_DEV_LPC, GEN_PMCON_3, ~1); - else - pci_or_config8(PCH_DEV_LPC, GEN_PMCON_3, 1); + /* Set which state system will be after power reapplied */ + lpc_set_power_failure_state(false); /* also iterates over all bridges on bus 0 */ busmaster_disable_on_bus(0); diff --git a/src/southbridge/intel/common/smihandler.c b/src/southbridge/intel/common/smihandler.c index 798f2f1..e2a99d2 100644 --- a/src/southbridge/intel/common/smihandler.c +++ b/src/southbridge/intel/common/smihandler.c @@ -11,7 +11,6 @@ #include <cpu/intel/em64t101_save_state.h> #include <elog.h> #include <halt.h> -#include <option.h> #include <southbridge/intel/common/pmbase.h> #include <smmstore.h> @@ -93,21 +92,6 @@ { } -static int power_on_after_fail(void) -{ - /* save and recover RTC port values */ - u8 tmp70, tmp72; - tmp70 = inb(0x70); - tmp72 = inb(0x72); - const unsigned int s5pwr = get_uint_option("power_on_after_fail", - CONFIG_MAINBOARD_POWER_FAILURE_STATE); - outb(tmp70, 0x70); - outb(tmp72, 0x72); - - /* For "KEEP", switch to "OFF" - KEEP is software emulated. */ - return (s5pwr == MAINBOARD_POWER_ON); -} - static void southbridge_smi_sleep(void) { u32 reg32; @@ -158,11 +142,8 @@ write_pmbase32(GPE0_EN, 0); - /* Always set the flag in case CMOS was changed on runtime. */ - if (power_on_after_fail()) - pci_and_config8(PCI_DEV(0, 0x1f, 0), D31F0_GEN_PMCON_3, ~1); - else - pci_or_config8(PCI_DEV(0, 0x1f, 0), D31F0_GEN_PMCON_3, 1); + /* Set which state system will be after power reapplied */ + lpc_set_power_failure_state(false); /* also iterates over all bridges on bus 0 */ busmaster_disable_on_bus(0); diff --git a/src/southbridge/intel/lynxpoint/smihandler.c b/src/southbridge/intel/lynxpoint/smihandler.c index 769cacb..9fe93b0 100644 --- a/src/southbridge/intel/lynxpoint/smihandler.c +++ b/src/southbridge/intel/lynxpoint/smihandler.c @@ -10,7 +10,6 @@ #include <cpu/intel/em64t101_save_state.h> #include <elog.h> #include <halt.h> -#include <option.h> #include <southbridge/intel/common/finalize.h> #include <northbridge/intel/haswell/haswell.h> #include <cpu/intel/haswell/haswell.h> @@ -76,21 +75,6 @@ } } -static int power_on_after_fail(void) -{ - /* save and recover RTC port values */ - u8 tmp70, tmp72; - tmp70 = inb(0x70); - tmp72 = inb(0x72); - const unsigned int s5pwr = get_uint_option("power_on_after_fail", - CONFIG_MAINBOARD_POWER_FAILURE_STATE); - outb(tmp70, 0x70); - outb(tmp72, 0x72); - - /* For "KEEP", switch to "OFF" - KEEP is software emulated. */ - return (s5pwr == MAINBOARD_POWER_ON); -} - static void southbridge_smi_sleep(void) { u32 reg32; @@ -144,11 +128,8 @@ /* Disable all GPE */ disable_all_gpe(); - /* Always set the flag in case CMOS was changed on runtime. */ - if (power_on_after_fail()) - pci_and_config8(PCH_LPC_DEV, GEN_PMCON_3, ~1); - else - pci_or_config8(PCH_LPC_DEV, GEN_PMCON_3, 1); + /* Set which state system will be after power reapplied */ + lpc_set_power_failure_state(false); /* also iterates over all bridges on bus 0 */ busmaster_disable_on_bus(0); -- To view, visit https://review.coreboot.org/c/coreboot/+/55238 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4ae6aa08177d4915a7acd5a53e0aae1e968e60a2 Gerrit-Change-Number: 55238 Gerrit-PatchSet: 1 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: [RFC] pc80/rtc/option: Move register backup for SMI
by Kyösti Mälkki (Code Review) 04 Jun '21

04 Jun '21

Attention is currently required from: Kyösti Mälkki, Alexander Couzens, Patrick Rudolph. Hello build bot (Jenkins), Alexander Couzens, Patrick Rudolph, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/49253 to look at the new patch set (#7). Change subject: [RFC] pc80/rtc/option: Move register backup for SMI ...................................................................... [RFC] pc80/rtc/option: Move register backup for SMI Change-Id: I025e9952a949834755f5cbc3dfe7b26ab9aaa032 Signed-off-by: Kyösti Mälkki <kyosti.malkki(a)gmail.com> --- M src/drivers/pc80/rtc/option.c M src/mainboard/lenovo/x60/smihandler.c 2 files changed, 22 insertions(+), 10 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/49253/7 -- To view, visit https://review.coreboot.org/c/coreboot/+/49253 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I025e9952a949834755f5cbc3dfe7b26ab9aaa032 Gerrit-Change-Number: 49253 Gerrit-PatchSet: 7 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-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Attention: Alexander Couzens <lynxis(a)fe80.eu> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: soc/intel/adl: Add SKU specific power limits support
by Selma Bensaid (Code Review) 04 Jun '21

04 Jun '21

Attention is currently required from: Furquan Shaikh, Paul Menzel, Sumeet R Pawnikar, Rajasekhar Venkatanagapavan, Patrick Rudolph, Karthik Ramasubramanian. Selma Bensaid has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54676 ) Change subject: soc/intel/adl: Add SKU specific power limits support ...................................................................... Patch Set 8: Code-Review-1 (1 comment) Patchset: PS8: patchset 8 has the mainboard changes from 54926 -- To view, visit https://review.coreboot.org/c/coreboot/+/54676 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic1676e2b4d611cdc85e770f131d5b6d5ecd180be Gerrit-Change-Number: 54676 Gerrit-PatchSet: 8 Gerrit-Owner: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com> Gerrit-Reviewer: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rajasekhar Venkatanagapavan <rajasekhar.venkatanagapavan(a)intel.corp-partner.google.com> Gerrit-Reviewer: Selma Bensaid <selma.bensaid(a)intel.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)mailbox.org> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com> Gerrit-Attention: Rajasekhar Venkatanagapavan <rajasekhar.venkatanagapavan(a)intel.corp-partner.google.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Fri, 04 Jun 2021 21:09:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: cpu/x86/lapic: Avoid aliasing MMIO writes
by Kyösti Mälkki (Code Review) 04 Jun '21

04 Jun '21

Attention is currently required from: Kyösti Mälkki. Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/55200 to look at the new patch set (#5). Change subject: cpu/x86/lapic: Avoid aliasing MMIO writes ...................................................................... cpu/x86/lapic: Avoid aliasing MMIO writes Change-Id: I75440cb0742eeaa140f49e7e4a4c51a96465441b Signed-off-by: Kyösti Mälkki <kyosti.malkki(a)gmail.com> --- M src/cpu/x86/lapic/lapic.c M src/include/cpu/x86/lapic.h 2 files changed, 10 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/55200/5 -- To view, visit https://review.coreboot.org/c/coreboot/+/55200 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I75440cb0742eeaa140f49e7e4a4c51a96465441b Gerrit-Change-Number: 55200 Gerrit-PatchSet: 5 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: cpu/x86/mp_init: Support both xapic and x2apic with common code
by Kyösti Mälkki (Code Review) 04 Jun '21

04 Jun '21

Attention is currently required from: Wonkyu Kim, Arthur Heymans. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55197 ) Change subject: cpu/x86/mp_init: Support both xapic and x2apic with common code ...................................................................... Patch Set 3: (1 comment) File src/cpu/x86/mp_init.c: https://review.coreboot.org/c/coreboot/+/55197/comment/80c67edc_a96904c5 PS3, Line 664: /* FIXME: Why the difference? */ I'd like to write in the commit message why/if this is the correct thing to do. -- To view, visit https://review.coreboot.org/c/coreboot/+/55197 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7264f39143cc6edb7a9687d0bd763cb2703a8265 Gerrit-Change-Number: 55197 Gerrit-PatchSet: 3 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> 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-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Fri, 04 Jun 2021 20:55:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: cpu/x86: Default to PARALLEL_MP selected
by Kyösti Mälkki (Code Review) 04 Jun '21

04 Jun '21

Attention is currently required from: Raul Rangel, Mariusz Szafrański, Jonathan Zhang, Angel Pons, Arthur Heymans, Andrey Petrov, Patrick Rudolph, Jason Glenesk, Anjaneya "Reddy" Chagam, Damien Zammit, Marshall Dawson, Johnny Lin, Tim Wawrzynczak, Suresh Bellampalli, Vanessa Eusebio, Michal Motyl, Morgan Jang, Felix Held. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55196 ) Change subject: cpu/x86: Default to PARALLEL_MP selected ...................................................................... Patch Set 3: (1 comment) File src/cpu/x86/Kconfig: https://review.coreboot.org/c/coreboot/+/55196/comment/5a01a102_5b88097b PS3, Line 6: PARALLEL_MP > It might be a good idea to guard it for single CPU systems: depends on MAX_CPUS != 1, or depends on […] If we decide LEGACY_CPU_INIT should go away, I'd rather have them tagged explicitly. And we want PARALLEL_MP to be tested with MAX_CPUS==1 and HAVE_SMI_HANDLER=n too and perhaps watchout for the code size to not explode. -- To view, visit https://review.coreboot.org/c/coreboot/+/55196 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9833c4f6c43b3e67f95bd465c42d7a5036dff914 Gerrit-Change-Number: 55196 Gerrit-PatchSet: 3 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anjaneya "Reddy" Chagam <anjaneya.chagam(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Damien Zammit Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Mariusz Szafrański <mariuszx.szafranski(a)intel.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Michal Motyl <michalx.motyl(a)intel.com> Gerrit-Reviewer: Morgan Jang <Morgan_Jang(a)wiwynn.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Suresh Bellampalli <suresh.bellampalli(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Vanessa Eusebio <vanessa.f.eusebio(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Mariusz Szafrański <mariuszx.szafranski(a)intel.com> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Anjaneya "Reddy" Chagam <anjaneya.chagam(a)intel.com> Gerrit-Attention: Damien Zammit Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Suresh Bellampalli <suresh.bellampalli(a)intel.com> Gerrit-Attention: Vanessa Eusebio <vanessa.f.eusebio(a)intel.com> Gerrit-Attention: Michal Motyl <michalx.motyl(a)intel.com> Gerrit-Attention: Morgan Jang <Morgan_Jang(a)wiwynn.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Fri, 04 Jun 2021 20:53:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: src/mainboard: Add Star Labs labtop series
by Sean Rhodes (Code Review) 04 Jun '21

04 Jun '21

Sean Rhodes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55128 ) Change subject: src/mainboard: Add Star Labs labtop series ...................................................................... Patch Set 15: (34 comments) Patchset: PS15: Does this mean the patch has to submitted again? File src/mainboard/starlabs/labtop/Kconfig: https://review.coreboot.org/c/coreboot/+/55128/comment/521d3a57_74265cfc PS15, Line 26: # select HAVE_IFD_BIN : # select HAVE_ME_BIN > Remove? Nope. See previous comment. https://review.coreboot.org/c/coreboot/+/55128/comment/0da78656_5ff93500 PS15, Line 61: config MAX_CPUS : int : default 8 if BOARD_STARLABS_LABTOP_KBL : default 12 > Same values are already specified in SoC Kconfig Wasn't when we wrote it, will change. https://review.coreboot.org/c/coreboot/+/55128/comment/1d09876d_221aae18 PS15, Line 66: #config DRIVER_TPM_SPI_CHIP : # int : # default 2 > Commented-out, remove? Nope https://review.coreboot.org/c/coreboot/+/55128/comment/a356ecec_1f1a729d PS15, Line 90: config IFD_BIN_PATH : string : default "3rdparty/blobs/mainboard/\$(MAINBOARDDIR)/\$(CONFIG_VARIANT_DIR)/flashregion_0_flashdescriptor.bin" : : config ME_BIN_PATH : string : default "3rdparty/blobs/mainboard/\$(MAINBOARDDIR)/\$(CONFIG_VARIANT_DIR)/flashregion_2_intel_me.bin" > Do these files exist in the blobs repo? Nope. See previous comment. https://review.coreboot.org/c/coreboot/+/55128/comment/3e80dd11_adf7aa27 PS15, Line 107: config EC_STARLABS_IT_BIN_PATH : string : depends on EC_STARLABS_IT_BIN : default "3rdparty/blobs/mainboard/\$(MAINBOARDDIR)/\$(CONFIG_VARIANT_DIR)/flashregion_8_ec.bin" > Same here Same here https://review.coreboot.org/c/coreboot/+/55128/comment/f26694dc_e57708f0 PS15, Line 120: default "3rdparty/blobs/mainboard/starlabs/Logo.bmp" > Same here Same here File src/mainboard/starlabs/labtop/acpi/mainboard.asl: https://review.coreboot.org/c/coreboot/+/55128/comment/0116447f_1bff4b2c PS15, Line 6: /* Power button device. */ : Device (PWRB) : { : Name (_HID, EisaId ("PNP0C0C")) : Name (PBST, One) : : Method (_STA, 0, NotSerialized) : { : Return (0x0F) : } : } > This should be removed. For what reason? File src/mainboard/starlabs/labtop/mainboard.c: https://review.coreboot.org/c/coreboot/+/55128/comment/f65fe767_fe59ba14 PS15, Line 33: #if CONFIG(BOARD_STARLABS_LABTOP_CML) > Preprocessor not needed. How? https://review.coreboot.org/c/coreboot/+/55128/comment/ef8c8967_d877e625 PS15, Line 52: /* Override smbios_mainboard_board_type */ : smbios_board_type smbios_mainboard_board_type(void) : { : return SMBIOS_BOARD_TYPE_MOTHERBOARD; : } > Remove. Why? File src/mainboard/starlabs/labtop/ramstage.c: https://review.coreboot.org/c/coreboot/+/55128/comment/255f3f65_9910e935 PS15, Line 1: > Spurious blank line? Ack https://review.coreboot.org/c/coreboot/+/55128/comment/c211b425_77c58056 PS15, Line 10: #if CONFIG(BOARD_STARLABS_LABTOP_CML) > Doing something like CB:48143 avoids the need for preprocessor. Is there any advantage? Not being difficult, simply interested. File src/mainboard/starlabs/labtop/spd/spd_util.c: https://review.coreboot.org/c/coreboot/+/55128/comment/1b87730e_b4839123 PS15, Line 8: void mainboard_fill_dq_map_data(void *dq_map_ptr) : { : /* DQ byte map */ : const u8 dq_map[2][12] = { : {0x0F, 0xF0, 0x00, 0xF0, 0x0F, 0xF0, 0x0F, 0x00, 0xFF, 0x00, 0xFF, 0x00}, : {0x33, 0xCC, 0x00, 0xCC, 0x33, 0xCC, 0x33, 0x00, 0xFF, 0x00, 0xFF, 0x00} }; : memcpy(dq_map_ptr, dq_map, sizeof(dq_map)); : } : : void mainboard_fill_dqs_map_data(void *dqs_map_ptr) : { : /* DQS CPU<>DRAM map */ : const u8 dqs_map[2][8] = {{0, 6, 3, 1, 5, 2, 7, 4}, {7, 5, 3, 6, 2, 4, 0, 1} }; : memcpy(dqs_map_ptr, dqs_map, sizeof(dqs_map)); : } > Meaningless with DDR4 Ack File src/mainboard/starlabs/labtop/variants/cml/devicetree.cb: https://review.coreboot.org/c/coreboot/+/55128/comment/04193031_2f2c3b58 PS15, Line 101: StarPoint > This doesn't look like a valid HID It isn't. https://review.coreboot.org/c/coreboot/+/55128/comment/77cf0156_ed2defa3 PS15, Line 140: device pci 00.0 on end > This root port is wired to a slot, so this device may not always be present. […] It will always be present on our hardware, can remove though File src/mainboard/starlabs/labtop/variants/cml/include/variant/gpio.h: PS15: > This file shouldn't be a header, but a compilation unit (.c file) of its own. Ack File src/mainboard/starlabs/labtop/variants/cml/include/variant/hda_verb.h: PS15: > Same here, this should be a compilation unit (.c file). Ack https://review.coreboot.org/c/coreboot/+/55128/comment/5416d634_2820eb40 PS15, Line 13: 0x0000002b > This makes more sense if written in decimal. […] Ack https://review.coreboot.org/c/coreboot/+/55128/comment/8c592616_f7f12c9b PS15, Line 20: 0x0017209E, : 0x00172111, : 0x001722EC, : 0x00172310, > There's an AZALIA_SUBVENDOR() macro Cool. https://review.coreboot.org/c/coreboot/+/55128/comment/137e61b8_58e2a82f PS15, Line 36: /* ONE DOES NOT SIMPLY : MAKE IT WORK WITH WINDOWS */ > I cna understand the frustration, but is this comment necessary? A comment is necassary. I'll give you that "Drivers in WU illogically only reference Front or Rear ports so must be set incorrectly" is probably more helpful, but who doesn't love a Sean Bean meme? File src/mainboard/starlabs/labtop/variants/cml/romstage.c: https://review.coreboot.org/c/coreboot/+/55128/comment/2d56f8b5_13f5d8c6 PS15, Line 52: /* : * The dqs_map arrays map the DDR4 pins to the SoC pins : * for both channels. : * : * the index = pin number on DDR4 part : * the value = pin number on SoC : */ : .dqs_map[DDR_CH0] = {0, 6, 1, 3, 5, 2, 7, 4}, : .dqs_map[DDR_CH1] = {7, 5, 3, 6, 2, 4, 0, 1}, > This doesn't matter for DDR4. Done File src/mainboard/starlabs/labtop/variants/kbl/devicetree.cb: https://review.coreboot.org/c/coreboot/+/55128/comment/246a1c2e_fc355517 PS15, Line 19: # Send an extra VR mailbox command for the PS4 exit issue : # register "SendVrMbxCmd" = "2" > Commented-out, remove? Ack https://review.coreboot.org/c/coreboot/+/55128/comment/b636f7ab_a53e1f95 PS15, Line 24: .up_delay_ms= 200,// T3 : .down_delay_ms= 0,// T10 : .cycle_delay_ms = 500,// T12 : .backlight_on_delay_ms=50,// T7 : .backlight_off_delay_ms = 0,// T9 : .backlight_pwm_hz = 200, > nit: alignment... As far as I know, nit is an achaic term for brightness of a display. What do you mean? I'm not up to date with the lingo that the cool kids use... https://review.coreboot.org/c/coreboot/+/55128/comment/b6f83aa2_1efaab97 PS15, Line 48: : [PchSerialIoIndexI2C2]= PchSerialIoPci, : [PchSerialIoIndexI2C3]= PchSerialIoPci, > Corresponding PCI devices are off. Ack https://review.coreboot.org/c/coreboot/+/55128/comment/ee4ff975_8e064a6e PS15, Line 52: [PchSerialIoIndexI2C5]= PchSerialIoPci, : [PchSerialIoIndexSpi0]= PchSerialIoPci, : [PchSerialIoIndexSpi1]= PchSerialIoPci, > Corresponding PCI devices are off. Ack https://review.coreboot.org/c/coreboot/+/55128/comment/6da260e3_3ef999a7 PS15, Line 55: [PchSerialIoIndexUart0] = PchSerialIoSkipInit, > If unused, can be disabled. No other function of PCI device 30 is enabled. Ack https://review.coreboot.org/c/coreboot/+/55128/comment/78a93f74_2717e688 PS15, Line 110: StarPoint > Doesn't look like a valid HID It's not. https://review.coreboot.org/c/coreboot/+/55128/comment/04caa196_058bf58b PS15, Line 159: device pci 00.0 on end > Same as CML: this PCIe port is wired to a slot, and this device may not always be present. It will on our hardware https://review.coreboot.org/c/coreboot/+/55128/comment/b9107369_5fd0ddd9 PS15, Line 198: subsystemid 0x10ec 0x111e > Suspicious indentation What's the relavance of it being suspicious? It works. https://review.coreboot.org/c/coreboot/+/55128/comment/0c145388_61ad9544 PS15, Line 200: off > Should be on Should be off File src/mainboard/starlabs/labtop/variants/kbl/include/variant/gpio.h: PS15: > Same here, this should be a compilation unit (.c file). Ack File src/mainboard/starlabs/labtop/variants/kbl/include/variant/hda_verb.h: PS15: > Same here, this should be a compilation unit (.c file). Ack https://review.coreboot.org/c/coreboot/+/55128/comment/6d93b6e7_159ded9d PS15, Line 13: 0x0000002b > Same as CML, this makes more sense if written in decimal. […] Ack https://review.coreboot.org/c/coreboot/+/55128/comment/63af8e91_1664e004 PS15, Line 18: 0x0017201E, : 0x00172111, : 0x001722EC, : 0x00172310, > Use the AZALIA_SUBVENDOR macro? Ack -- To view, visit https://review.coreboot.org/c/coreboot/+/55128 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iffa6061b0e600880b0c93746f35b1731e4841e31 Gerrit-Change-Number: 55128 Gerrit-PatchSet: 15 Gerrit-Owner: Sean Rhodes <admin(a)starlabs.systems> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Comment-Date: Fri, 04 Jun 2021 20:50:43 +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]: drivers/generic/ioapic: Use arch/x86/ioapic
by Kyösti Mälkki (Code Review) 04 Jun '21

04 Jun '21

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55185 ) Change subject: drivers/generic/ioapic: Use arch/x86/ioapic ...................................................................... Patch Set 3: (1 comment) File src/drivers/generic/ioapic/ioapic.c: https://review.coreboot.org/c/coreboot/+/55185/comment/34235380_dc80cf26 PS3, Line 17: config->enable_virtual_wire); I doubt if irq_on_fsb is worth keeping in devicetree. Usage is bound to CPU model AFAICS. I am not convinced when one is allowed to leave enable_virtual_wire unselected. It's a bit confusing and needs better documentation. There is unconditional LINT0/1 virtual wire also in cpu/x86/lapic/lapic.c that is legacy PCI i8259 related. -- To view, visit https://review.coreboot.org/c/coreboot/+/55185 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibfaf6693288005463e45831fe100a5052e97cf2f Gerrit-Change-Number: 55185 Gerrit-PatchSet: 3 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Fri, 04 Jun 2021 20:49:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit June 2021