coreboot-gerrit April 2021

coreboot-gerrit@coreboot.org

1 participants
6269 discussions

Change in coreboot[master]: *x86: Support x2apic mode
by Nico Huber (Code Review) 02 Apr '21

02 Apr '21

Attention is currently required from: Furquan Shaikh, Jamie Ryu, Wonkyu Kim, Paul Menzel, Angel Pons, Subrata Banik, Raj Astekar, Patrick Rudolph. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51723 ) Change subject: *x86: Support x2apic mode ...................................................................... Patch Set 6: (2 comments) File src/cpu/x86/lapic/lapic_cpu_init.c: PS6: I'm not 100%, but I thought the code here was abandoned in favor of the mp_init version. If this is the case, the changes here are probably untested and belong into a separate commit. File src/cpu/x86/mp_init.c: https://review.coreboot.org/c/coreboot/+/51723/comment/dc06e117_cb53ff62 PS6, Line 458: } else { > Return 0 in the if-block and drop the `else`? This way, you don't need to reindent everything. Or even better, add more functions. e.g. start_aps_lapic(), start_aps_lx2apic(). Only the call would have to be inside an if. With a forward declaration the original code could even stay in place, i.e. wouldn't show up much in the diff. If you want to move it, that could be done in a later commit. -- To view, visit https://review.coreboot.org/c/coreboot/+/51723 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0bb729b0521fb9dc38b7981014755daeaf9ca817 Gerrit-Change-Number: 51723 Gerrit-PatchSet: 6 Gerrit-Owner: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(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-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Attention: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Fri, 02 Apr 2021 21:38:24 +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]: soc/intel/common/block/irq: Add support for intel_write_pci0_PRT
by Tim Wawrzynczak (Code Review) 02 Apr '21

02 Apr '21

Attention is currently required from: Nico Huber, Furquan Shaikh, Patrick Rudolph. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51159 ) Change subject: soc/intel/common/block/irq: Add support for intel_write_pci0_PRT ...................................................................... Patch Set 8: (1 comment) File src/soc/intel/common/block/irq/irq.c: https://review.coreboot.org/c/coreboot/+/51159/comment/ddc091ab_2e8fd363 PS7, Line 378: /* Map INTA->PIRQ_A, INTB->PIRQ_B, INTC->PIRQ_C, INTD->PIRQ_D */ > > Sorry I can make the comment a little more clear, this is how Intel does the mapping for PIC-mode […] You are correct, this recommendation comes from the BWG guides IIRC. However I agree with you that I think without the OS' ability to rewrite the PIRx, it is effectively locked out of being able to use legacy PIC mode. This patch currently just retains the legacy-mode table intact. However, here is where things stand: the respective soc/espi.c files call pch_pirq_init() in common/intel/block/lpc_lib, which attempts to program in the legacy IRQ settings, but AFAICT, the devices that require these unique IRQs ignore what is programmed in this register, and use what is programmed in the PCR space instead. In the OS, I can read config space PCI_INTERRUPT_LINE as 0xa or 0xb for an I2C device for example, but /proc/interrupts shows what the ACPI table included, and the IRQs are working. I will try an experiment with `noapic` with and without my patch train and see if it even works at all. -- To view, visit https://review.coreboot.org/c/coreboot/+/51159 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I21a4835890ca03bff83ed0e8791441b3af54cb62 Gerrit-Change-Number: 51159 Gerrit-PatchSet: 8 Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Fri, 02 Apr 2021 21:36:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: sb/intel/common: Refactor _PRT generation to support GSI-based tables
by Angel Pons (Code Review) 02 Apr '21

02 Apr '21

Attention is currently required from: Nico Huber, Tim Wawrzynczak, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50857 ) Change subject: sb/intel/common: Refactor _PRT generation to support GSI-based tables ...................................................................... Patch Set 16: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/50857 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica420a3d12fd1d64c8fe6e4b326fd779b3f10868 Gerrit-Change-Number: 50857 Gerrit-PatchSet: 16 Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Fri, 02 Apr 2021 21:31:02 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/amd: Make espi_clear_decodes private
by Furquan Shaikh (Code Review) 02 Apr '21

02 Apr '21

Attention is currently required from: Jason Glenesk, Raul Rangel, Marshall Dawson, Felix Held. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52064 ) Change subject: soc/amd: Make espi_clear_decodes private ...................................................................... Patch Set 1: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/52064/comment/ec3627fb_d89b438e PS1, Line 10: change consolidates the clear logic into one spot. > Right, we should guard espi_setup in bootblock.c with !VBOOT_STARTS_BEFORE_BOOTBLOCK. […] I think the problem is what part of PSP is doing the reconfiguration of eSPI. If it is PSP BL, then yes the above check makes sense. But, if it is ABL or PSP OS, then x86 will have to redo the configuration. That is the reason I had mentioned on one of the previous CLs that PSP ideally should not touch the eSPI controller at all or at least leave it back in the state as it found at reset. -- To view, visit https://review.coreboot.org/c/coreboot/+/52064 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic57689e50febd29796d8ac8d99c81e41fee5b41c Gerrit-Change-Number: 52064 Gerrit-PatchSet: 1 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Fri, 02 Apr 2021 21:26:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org> Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: sb/intel/common: Refactor _PRT generation to support GSI-based tables
by Tim Wawrzynczak (Code Review) 02 Apr '21

02 Apr '21

Attention is currently required from: Nico Huber, Angel Pons, Patrick Rudolph. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50857 ) Change subject: sb/intel/common: Refactor _PRT generation to support GSI-based tables ...................................................................... Patch Set 16: (1 comment) File src/southbridge/intel/common/acpi_pirq_gen.h: https://review.coreboot.org/c/coreboot/+/50857/comment/5c7bf457_22524ac1 PS15, Line 53: unsigned int > nit: use `enum pirq` instead? Done -- To view, visit https://review.coreboot.org/c/coreboot/+/50857 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica420a3d12fd1d64c8fe6e4b326fd779b3f10868 Gerrit-Change-Number: 50857 Gerrit-PatchSet: 16 Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Fri, 02 Apr 2021 21:26:22 +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]: coreboot_tables: Print strapping IDs when adding them to coreboot table
by Julius Werner (Code Review) 02 Apr '21

02 Apr '21

Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/52011 ) Change subject: coreboot_tables: Print strapping IDs when adding them to coreboot table ...................................................................... coreboot_tables: Print strapping IDs when adding them to coreboot table These used to be printed before CB:46605. Having them in the logs can be a huge timesaver when debugging logs sent to you by other people (especially from systems that don't boot all the way). Let's add them back. Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: Ifdbfdd29d25a0937c27113ace776f7aec231a57d Reviewed-on: https://review.coreboot.org/c/coreboot/+/52011 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Tim Wawrzynczak <twawrzynczak(a)chromium.org> --- M src/lib/coreboot_table.c 1 file changed, 12 insertions(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, approved diff --git a/src/lib/coreboot_table.c b/src/lib/coreboot_table.c index 29be857..cb85a18 100644 --- a/src/lib/coreboot_table.c +++ b/src/lib/coreboot_table.c @@ -19,6 +19,7 @@ #include <cbmem.h> #include <bootmem.h> #include <bootsplash.h> +#include <inttypes.h> #include <spi_flash.h> #include <smmstore.h> @@ -305,10 +306,20 @@ config->tag = LB_TAG_BOARD_CONFIG; config->size = sizeof(*config); + const uint64_t fw_config = fw_config_get(); config->board_id = board_id(); config->ram_code = ram_code(); config->sku_id = sku_id(); - config->fw_config = pack_lb64(fw_config_get()); + config->fw_config = pack_lb64(fw_config); + + if (config->board_id != UNDEFINED_STRAPPING_ID) + printk(BIOS_INFO, "Board ID: %d\n", config->board_id); + if (config->ram_code != UNDEFINED_STRAPPING_ID) + printk(BIOS_INFO, "RAM code: %d\n", config->ram_code); + if (config->sku_id != UNDEFINED_STRAPPING_ID) + printk(BIOS_INFO, "SKU ID: %d\n", config->sku_id); + if (fw_config != UNDEFINED_FW_CONFIG) + printk(BIOS_INFO, "FW config: %#" PRIx64 "\n", fw_config); return config; } -- To view, visit https://review.coreboot.org/c/coreboot/+/52011 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifdbfdd29d25a0937c27113ace776f7aec231a57d Gerrit-Change-Number: 52011 Gerrit-PatchSet: 3 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: sb/intel/common: Refactor _PRT generation to support GSI-based tables
by Tim Wawrzynczak (Code Review) 02 Apr '21

02 Apr '21

Attention is currently required from: Nico Huber, Tim Wawrzynczak, Patrick Rudolph. Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Angel Pons, Arthur Heymans, Patrick Rudolph, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/50857 to look at the new patch set (#16). Change subject: sb/intel/common: Refactor _PRT generation to support GSI-based tables ...................................................................... sb/intel/common: Refactor _PRT generation to support GSI-based tables Newer Intel SoCs also support _PRT tables, but they route PCI devices to more than just PIRQs, and statically specify IRQs instead of using link devices. Extend/refactor intel_acpi_gen_def_acpi_pirq to support this additional use case. Signed-off-by: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Change-Id: Ica420a3d12fd1d64c8fe6e4b326fd779b3f10868 --- M src/southbridge/intel/bd82x6x/lpc.c M src/southbridge/intel/common/acpi_pirq_gen.c M src/southbridge/intel/common/acpi_pirq_gen.h M src/southbridge/intel/common/rcba_pirq.c M src/southbridge/intel/common/rcba_pirq.h M src/southbridge/intel/i82801gx/lpc.c M src/southbridge/intel/i82801ix/lpc.c M src/southbridge/intel/i82801jx/lpc.c M src/southbridge/intel/ibexpeak/lpc.c M src/southbridge/intel/lynxpoint/lpc.c 10 files changed, 182 insertions(+), 79 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/50857/16 -- To view, visit https://review.coreboot.org/c/coreboot/+/50857 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ica420a3d12fd1d64c8fe6e4b326fd779b3f10868 Gerrit-Change-Number: 50857 Gerrit-PatchSet: 16 Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: src/*acpi: create acpi table for x2apic mode
by Nico Huber (Code Review) 02 Apr '21

02 Apr '21

Attention is currently required from: Furquan Shaikh, Wonkyu Kim, Subrata Banik, Raj Astekar, Patrick Rudolph. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51724 ) Change subject: src/*acpi: create acpi table for x2apic mode ...................................................................... Patch Set 6: (2 comments) File src/acpi/acpi.c: https://review.coreboot.org/c/coreboot/+/51724/comment/51c46e72_35c34c78 PS6, Line 165: if (apic_ids[index] < 0xff) No need for further indentation, this could be an `else if`. https://review.coreboot.org/c/coreboot/+/51724/comment/05a16a09_2c3a27f9 PS6, Line 170: current += acpi_create_madt_lx2apic( : (acpi_madt_lx2apic_t *)current, : index, apic_ids[index]); This already uses the lx2apic function. Please explain what is wrong with the existing code. -- To view, visit https://review.coreboot.org/c/coreboot/+/51724 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9399d30b686b55d86806f5db4110bf4a80fe459b Gerrit-Change-Number: 51724 Gerrit-PatchSet: 6 Gerrit-Owner: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Fri, 02 Apr 2021 21:19:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/amd: Make espi_clear_decodes private
by Raul Rangel (Code Review) 02 Apr '21

02 Apr '21

Attention is currently required from: Jason Glenesk, Furquan Shaikh, Marshall Dawson, Felix Held. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52064 ) Change subject: soc/amd: Make espi_clear_decodes private ...................................................................... Patch Set 1: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/52064/comment/a548b5bc_511f7e8b PS1, Line 10: change consolidates the clear logic into one spot. > I think it would be good to mention here that this changes the behavior for Picasso since the decode […] Right, we should guard espi_setup in bootblock.c with !VBOOT_STARTS_BEFORE_BOOTBLOCK. There is no reason to redo it if psp_verstage did it. -- To view, visit https://review.coreboot.org/c/coreboot/+/52064 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic57689e50febd29796d8ac8d99c81e41fee5b41c Gerrit-Change-Number: 52064 Gerrit-PatchSet: 1 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Fri, 02 Apr 2021 21:16:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: *x86: Support x2apic mode
by Angel Pons (Code Review) 02 Apr '21

02 Apr '21

Attention is currently required from: Nico Huber, Furquan Shaikh, Jamie Ryu, Wonkyu Kim, Paul Menzel, Subrata Banik, Raj Astekar, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51723 ) Change subject: *x86: Support x2apic mode ...................................................................... Patch Set 6: (5 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/51723/comment/e73c6659_b379e705 PS6, Line 21: cat /proc/cpuinfo | grep "apicid" > No need to use cat. ;-) […] I don't like the grep-only approach, especially when I want to change what I'm grepping for. I prefer to put the path first. File src/cpu/x86/lapic/lapic_cpu_init.c: https://review.coreboot.org/c/coreboot/+/51723/comment/54f935db_7e689677 PS6, Line 355: LAPIC_DM_INIT | LAPIC_INT_LEVELTRIG, id); No need to break the line here. File src/cpu/x86/mp_init.c: https://review.coreboot.org/c/coreboot/+/51723/comment/3750ada8_0f86ee54 PS6, Line 458: } else { Return 0 in the if-block and drop the `else`? This way, you don't need to reindent everything. https://review.coreboot.org/c/coreboot/+/51723/comment/6aff1b80_9c002745 PS6, Line 678: } else { Simpler: if (is_x2apic_mode()) { x2apic_send_ipi(LAPIC_DM_SMI | LAPIC_INT_LEVELTRIG, lapicid()); return; } File src/include/cpu/x86/lapic_def.h: https://review.coreboot.org/c/coreboot/+/51723/comment/3c6f407d_603a2028 PS6, Line 99: #define X2APIC_LAPIC_ID (X2APIC_MSR_BASE_ADDRESS | (LAPIC_ID >> 4)) These definitions are misplaced (they're not fields in LAPIC_TDCR, not even LAPIC registers) -- To view, visit https://review.coreboot.org/c/coreboot/+/51723 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0bb729b0521fb9dc38b7981014755daeaf9ca817 Gerrit-Change-Number: 51723 Gerrit-PatchSet: 6 Gerrit-Owner: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(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-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Attention: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Fri, 02 Apr 2021 21:11:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit April 2021