coreboot-gerrit April 2024

coreboot-gerrit@coreboot.org

1 participants
3293 discussions

[S] Change in coreboot[main]: block/fast_spi: Use read32p/write32p for spi rw
by Arthur Heymans (Code Review) 24 Apr '24

24 Apr '24

Attention is currently required from: Angel Pons, Ashish Kumar Mishra, Saurabh Mishra, Subrata Banik. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/82079?usp=email ) Change subject: block/fast_spi: Use read32p/write32p for spi rw ...................................................................... Patch Set 2: (1 comment) File src/soc/intel/common/block/fast_spi/fast_spi_flash.c: https://review.coreboot.org/c/coreboot/+/82079/comment/8d7e4d3c_9a229fb4 : PS2, Line 80: const uint8_t *byte_ptr = (const uint8_t *)data; : size_t bytes_to_copy; : union { : uint32_t full; : uint8_t bytes[4]; : } dword; : : for (size_t i = 0; i < len; i += 4) { : dword.full = 0; : : bytes_to_copy = (len - i < 4) ? len - i : 4; : for (size_t j = 0; j < bytes_to_copy; j++) : dword.bytes[j] = byte_ptr[i + j]; : : write32p(ctx->mmio_base + SPIBAR_FDATA(i >> 2), dword.full); : } > This is what I suggested in CB:81959 but back then I wasn't sure if the hardware can also accept smaller width transactions: > > ``` > static void fill_xfer_fifo(struct fast_spi_flash_ctx *ctx, const uint8_t *data, > size_t len) > { > /* FDATAn requires 32-bit accesses */ > const uint32_t *dwords = (const uint32_t *)data; > for (size_t i = 0; i < len / 4; i++) > write32p(ctx->mmio_base + SPIBAR_FDATA(i), dwords[i]); > > if (len % 4) { > uint32_t last_dword = 0; > for (size_t i = 0; i < len % 4; i++) > last_dword |= data[len / 4 + i] << (i * 8); > > write32p(ctx->mmio_base + SPIBAR_FDATA(len / 4), last_dword); > } > } > ``` > But if the hardware can tolerate byte-width transactions: > > ``` > static void fill_xfer_fifo(struct fast_spi_flash_ctx *ctx, const uint8_t *data, > size_t len) > { > const uint32_t *dwords = (const uint32_t *)data; > for (size_t i = 0; i < len / 4; i++) > write32p(ctx->mmio_base + SPIBAR_FDATA(i), dwords[i]); > > for (size_t i = 0; i < len % 4; i++) > write8p(ctx->mmio_base + SPIBAR_FDATA(len / 4) + i, data[len / 4 + i]) > } > ``` > Maximum code simplicity would be to only use byte reads/writes, but I'm not sure if there's a performance impact: > ``` > static void fill_xfer_fifo(struct fast_spi_flash_ctx *ctx, const uint8_t *data, > size_t len) > { > for (size_t i = 0; i < len; i++) > write8p(ctx->mmio_base + SPIBAR_FDATA(0) + i, data[i]) > } > ``` There usually is a performance impact as you have 4x the amount of transactions. Both flashrom and sb/intel/common do the following, which is similar to this code (maybe copy it to keep it consistent?): if (len <= 0) return; for (i = 0; i < len; i++) { if ((i % 4) == 0) temp32 = 0; temp32 |= ((uint32_t)data[i]) << ((i % 4) * 8); if ((i % 4) == 3) /* 32 bits are full, write them to regs. */ writel_(temp32, cntlr.data + (i - (i % 4))); } i--; if ((i % 4) != 3) /* Write remaining data to regs. */ writel_(temp32, cntlr.data + (i - (i % 4))); -- To view, visit https://review.coreboot.org/c/coreboot/+/82079?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I317c7160bf192dd2aeacebf6029a809bc97f3420 Gerrit-Change-Number: 82079 Gerrit-PatchSet: 2 Gerrit-Owner: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com> Gerrit-Reviewer: Saurabh Mishra <mishra.saurabh(a)intel.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com> Gerrit-Attention: Saurabh Mishra <mishra.saurabh(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 24 Apr 2024 14:22:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: soc/intel/xeon_sp/gnr: Add IIO config utils
by Shuo Liu (Code Review) 24 Apr '24

24 Apr '24

Attention is currently required from: Angel Pons, Arthur Heymans, Chen, Gang C, Christian Walter, David Hendricks, Johnny Lin, Jonathan Zhang, Lean Sheng Tan, Martin L Roth, Patrick Rudolph, Shuo Liu, TangYiwei, Tim Chu. Hello Angel Pons, Arthur Heymans, Chen, Gang C, Christian Walter, David Hendricks, Johnny Lin, Jonathan Zhang, Lean Sheng Tan, Patrick Rudolph, TangYiwei, Tim Chu, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/81318?usp=email to look at the new patch set (#45). Change subject: soc/intel/xeon_sp/gnr: Add IIO config utils ...................................................................... soc/intel/xeon_sp/gnr: Add IIO config utils Add IIO configuration utils shared in GNR boards to handle the complex IIO configuration settings. Change-Id: If7146761db6f73a0c4b0d31b010c0d30a42bf690 Signed-off-by: Gang Chen <gang.c.chen(a)intel.com> Co-authored-by: Shuo Liu <shuo.liu(a)intel.com> --- M src/soc/intel/xeon_sp/gnr/Makefile.mk A src/soc/intel/xeon_sp/gnr/include/soc/iio.h A src/soc/intel/xeon_sp/gnr/soc_iio.c 3 files changed, 200 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/81318/45 -- To view, visit https://review.coreboot.org/c/coreboot/+/81318?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: If7146761db6f73a0c4b0d31b010c0d30a42bf690 Gerrit-Change-Number: 81318 Gerrit-PatchSet: 45 Gerrit-Owner: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Chen, Gang C <gang.c.chen(a)intel.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: TangYiwei Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: TangYiwei Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Chen, Gang C <gang.c.chen(a)intel.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in coreboot[main]: soc/intel/xeon_sp/gnr: Use OCP_VPD drivers
by Shuo Liu (Code Review) 24 Apr '24

24 Apr '24

Attention is currently required from: Arthur Heymans, Christian Walter, David Hendricks, Jincheng Li, Johnny Lin, Jonathan Zhang, Patrick Rudolph, Shuo Liu, TangYiwei, Tim Chu. Hello Angel Pons, Arthur Heymans, Christian Walter, David Hendricks, Jincheng Li, Johnny Lin, Jonathan Zhang, Lean Sheng Tan, Patrick Rudolph, TangYiwei, Tim Chu, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/81317?usp=email to look at the new patch set (#43). Change subject: soc/intel/xeon_sp/gnr: Use OCP_VPD drivers ...................................................................... soc/intel/xeon_sp/gnr: Use OCP_VPD drivers Use OCP_VPD driver provided functions to get VPD value. Change-Id: Ifeca8cf4312ab66ca03188fe25af88a952073130 Signed-off-by: Johnny Lin <johnny_lin(a)wiwynn.com> Signed-off-by: Jincheng Li <jincheng.li(a)intel.com> --- M src/soc/intel/xeon_sp/gnr/Kconfig M src/soc/intel/xeon_sp/gnr/chip.h M src/soc/intel/xeon_sp/gnr/romstage.c 3 files changed, 6 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/81317/43 -- To view, visit https://review.coreboot.org/c/coreboot/+/81317?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ifeca8cf4312ab66ca03188fe25af88a952073130 Gerrit-Change-Number: 81317 Gerrit-PatchSet: 43 Gerrit-Owner: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Jincheng Li <jincheng.li(a)intel.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: TangYiwei Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Jincheng Li <jincheng.li(a)intel.com> Gerrit-Attention: TangYiwei Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in coreboot[main]: soc/intel/alderlake: Set UsbTcPortEn based on tcss_port[x]
by Sean Rhodes (Code Review) 24 Apr '24

24 Apr '24

Sean Rhodes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/82058?usp=email ) Change subject: soc/intel/alderlake: Set UsbTcPortEn based on tcss_port[x] ...................................................................... Set Ready For Review -- To view, visit https://review.coreboot.org/c/coreboot/+/82058?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I07ef0759057f7f40210766a73643c9ccf1dc986d Gerrit-Change-Number: 82058 Gerrit-PatchSet: 1 Gerrit-Owner: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Comment-Date: Wed, 24 Apr 2024 14:02:28 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XL] Change in coreboot[main]: soc/intel/xeon_sp: Add Granite Rapids initial codes
by Shuo Liu (Code Review) 24 Apr '24

24 Apr '24

1 0

[XS] Change in coreboot[main]: vc/google/chromeos: Move RAMOOPS region creation to BS_DEV_INIT_CHIPS
by Subrata Banik (Code Review) 24 Apr '24

24 Apr '24

Attention is currently required from: Anil Kumar K, Bora Guvendik, Hannah Williams, Jamie Ryu, Kapil Porwal, Wonkyu Kim. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/82042?usp=email ) Change subject: vc/google/chromeos: Move RAMOOPS region creation to BS_DEV_INIT_CHIPS ...................................................................... Patch Set 5: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/82042?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ibae06362cd80eacb16f6cf0eed8c9aa1fbfb2535 Gerrit-Change-Number: 82042 Gerrit-PatchSet: 5 Gerrit-Owner: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Attention: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Attention: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Comment-Date: Wed, 24 Apr 2024 13:56:30 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XL] Change in coreboot[main]: soc/intel/xeon_sp: Add Granite Rapids initial codes
by Shuo Liu (Code Review) 24 Apr '24

24 Apr '24

1 0

[XL] Change in coreboot[main]: soc/intel/xeon_sp: Add Granite Rapids initial codes
by Shuo Liu (Code Review) 24 Apr '24

24 Apr '24

Attention is currently required from: Angel Pons, Arthur Heymans, Chen, Gang C, Christian Walter, David Hendricks, Felix Held, Jincheng Li, Johnny Lin, Jonathan Zhang, Lean Sheng Tan, Nico Huber, Patrick Rudolph, Paul Menzel, TangYiwei, Tim Chu. Shuo Liu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81316?usp=email ) Change subject: soc/intel/xeon_sp: Add Granite Rapids initial codes ...................................................................... Patch Set 53: (28 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/81316/comment/92610315_17311d3f : PS53, Line 16: Emmisburg > Emmi*t*sburg Done https://review.coreboot.org/c/coreboot/+/81316/comment/57311617_21286dda : PS53, Line 20: > Please list the datasheet(s) with name and revision. We have to add this after SoC launch since it is shift-left opensource at the moment. I updated the commit message to clarify this, This patch initially sets the code set up as a build target with Granite Rapids N-1 FSP (src/vc/intel/fsp/fsp2_0/ graniterapids). https://review.coreboot.org/c/coreboot/+/81316/comment/398612ec_d9e30c24 : PS53, Line 21: TEST=Build and boot on intel/archercity CRB > Does Archer City also support Granite Rapdis? […] Sorry, here the test means this patch doesn't impact archercity CRB. P.S. archercity CRB doesn't support GNR. I removed the TEST= line. For CBMEM timestamps, we have to add it after SoC launch (now it is shift-left opensource but only pass build with N-1 FSP). File MAINTAINERS: https://review.coreboot.org/c/coreboot/+/81316/comment/ace9c878_1d5a43f6 : PS53, Line 954: F: src/vendorcode/intel/fsp/fsp2_0/graniterapids/ > Please mention this in the commit message, or even make it a separate commit. Done File src/soc/intel/xeon_sp/chip_gen6.c: https://review.coreboot.org/c/coreboot/+/81316/comment/fae8b3ec_dd9e4275 : PS53, Line 28: uint8_t > unsigned int […] Interesting. Thanks! Updated. https://review.coreboot.org/c/coreboot/+/81316/comment/5e83390f_cf52a62c : PS53, Line 57: IORESOURCE_SUBTRACTIVE > Is this actually a subtractive resource? I think various platforms in coreboot get this wrong. […] I agree with you. Updated. This cannot be counted as subtractive decoding. I checked the lpc driver which reports the same set of resources in src/soc/intel/common/block/lpc/lpc.c, the subtractive tag is not used either. P.S. If the platform is with an lpc driver loaded, the reporting here maybe be abled to be removed. however, let me keep it now and handle this in a separate patch/discussion (e.g. do we have a strong needs for that?) https://review.coreboot.org/c/coreboot/+/81316/comment/d8fe7e05_dfac49ea : PS53, Line 93: uint8_t > Ditto. Done File src/soc/intel/xeon_sp/gnr/Kconfig: https://review.coreboot.org/c/coreboot/+/81316/comment/5bebec3a_0a8352ab : PS53, Line 20: GraniteRapids > Granite Rapids Done File src/soc/intel/xeon_sp/gnr/chip.c: https://review.coreboot.org/c/coreboot/+/81316/comment/668a11f4_2b6808df : PS53, Line 56: fsp_silicon_init(); > Should be close to the printk() statement referring to this? Done File src/soc/intel/xeon_sp/gnr/chipset.cb: https://review.coreboot.org/c/coreboot/+/81316/comment/5bbc4ddd_28d44e83 : PS53, Line 19: HEPT > typo: HPET Done File src/soc/intel/xeon_sp/gnr/cpu.c: https://review.coreboot.org/c/coreboot/+/81316/comment/9272e4aa_65260750 : PS53, Line 33: //FIXME: > Please add a space after //. Done https://review.coreboot.org/c/coreboot/+/81316/comment/46066c81_75a073a0 : PS53, Line 39: Loading MCU on AP in parallel seems to fail in 10% of the cases : * so do it serialized. > Please add a reference to the corresponding bug report. Oh sorry, this is originally forked from SPR codes and GNR do not need these comments. Updated. https://review.coreboot.org/c/coreboot/+/81316/comment/0f32db08_5a1c5ae1 : PS53, Line 62: {X86_VENDOR_INTEL, CPUID_GRANITERAPIDS, CPUID_ALL_STEPPINGS_MASK}, : {X86_VENDOR_INTEL, CPUID_SIERRAFOREST, CPUID_ALL_STEPPINGS_MASK}, > Add space after { and before }? Done https://review.coreboot.org/c/coreboot/+/81316/comment/56f62cb2_4c3faaf4 : PS53, Line 87: return num_virts * soc_get_num_cpus(); > So `num_phys` isn’t actually needed? This API calculates the thread counts which correspond to num_virts (logical cores), while num_phys is corresponding to physical cores (in SMT system, one physical cores has multiple threads, a.k.a. logical cores). https://review.coreboot.org/c/coreboot/+/81316/comment/5805560f_76f22545 : PS53, Line 106: cpu > CPU Done https://review.coreboot.org/c/coreboot/+/81316/comment/499e35a1_510addaf : PS53, Line 106: cpu > CPU Done https://review.coreboot.org/c/coreboot/+/81316/comment/9766c75c_6c4e915a : PS53, Line 114: printk(BIOS_ERR, "microcode not found in CBFS!\n"); > What is the consequence of this error? It will caused the subsequent call to skip the microcode loading. When BIOS doesn't have a microcode with it, CPU will boot with its default microcode. void intel_microcode_load_unlocked(const void *microcode_patch) { u32 current_rev; const struct microcode *m = microcode_patch; if (!m) { printk(BIOS_WARNING, "microcode: failed because no ucode was found\n"); return; } ... } https://review.coreboot.org/c/coreboot/+/81316/comment/7dbd878c_cf5485e9 : PS53, Line 116: intel_microcode_load_unlocked(microcode_patch); > Can this be called with NULL? Yes. https://review.coreboot.org/c/coreboot/+/81316/comment/06ac5908_f0339891 : PS53, Line 119: printk(BIOS_ERR, "MP initialization failure.\n"); > Print the return value? Can the system still function with this failing? Done File src/soc/intel/xeon_sp/gnr/include/soc/cpu.h: https://review.coreboot.org/c/coreboot/+/81316/comment/b561778a_6730642d : PS53, Line 6: #define CPUID_GRANITERAPIDS 0xA06D0 > Use tabs for alignment? Done File src/soc/intel/xeon_sp/gnr/include/soc/vpd.h: https://review.coreboot.org/c/coreboot/+/81316/comment/1efc34e2_c2105d0e : PS53, Line 6: #define FAST_BOOT_EN "fast_boot_en" /* 1 or 0: enable or disable fast boot for warm/cold reset */ > What is fast boot exactly? Fast boot will use saved silicon init data to skip calculating these settings (e.g. MRC is skipped) so that to boot fast. I will make this a separate patch to make this clear. File src/soc/intel/xeon_sp/gnr/romstage.c: https://review.coreboot.org/c/coreboot/+/81316/comment/3c22fa31_d790d7f9 : PS53, Line 43: case 0x4000000: // 64M > Use the macros instead of comments? Done File src/soc/intel/xeon_sp/gnr/soc_acpi.c: https://review.coreboot.org/c/coreboot/+/81316/comment/4a009029_ae210682 : PS53, Line 19: if (val & (1 << offset)) { : return MP_IRQ_POLARITY_LOW; : } else { : return MP_IRQ_POLARITY_HIGH; : } > I’d use the ternary operator. Done File src/soc/intel/xeon_sp/gnr/soc_util.c: https://review.coreboot.org/c/coreboot/+/81316/comment/bbf26cdb_e02185d5 : PS53, Line 131: uint8_t > unsigned int Done https://review.coreboot.org/c/coreboot/+/81316/comment/5e3c350a_7541f32f : PS53, Line 139: return 0xff; > Why pass the arguments, if they are not needed? Why aren’t they unsigned? Add some kind of comment I will remove this API at all. Updated. File src/soc/intel/xeon_sp/include/soc/acpi.h: https://review.coreboot.org/c/coreboot/+/81316/comment/424226ef_3b4a9926 : PS53, Line 21: unsigned long xeonsp_acpi_create_madt_lapics(unsigned long current); > Do this in a separate patch? We have to keep the declaration to pass build, however, I remove the null implementation, unsigned long acpi_fill_cedt(unsigned long current) { return current; } File src/soc/intel/xeon_sp/include/soc/fsp_upd.h: https://review.coreboot.org/c/coreboot/+/81316/comment/2481b650_74e50df3 : PS53, Line 15: * GNR needs below macro to be in effect before any FSP header references > Why? Please reference the source. N-1 FSP header do not need this, I removed it at the moment. Will update when the real FSP header is released. File src/soc/intel/xeon_sp/lockdown.c: https://review.coreboot.org/c/coreboot/+/81316/comment/f8b58b77_a7e8613c : PS53, Line 25: return; > Do this in a separate patch? Done -- To view, visit https://review.coreboot.org/c/coreboot/+/81316?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I3084e1b5abf25d8d9504bebeaed2a15b916ed56b Gerrit-Change-Number: 81316 Gerrit-PatchSet: 53 Gerrit-Owner: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Chen, Gang C <gang.c.chen(a)intel.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Jincheng Li <jincheng.li(a)intel.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: TangYiwei Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-CC: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: TangYiwei Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Chen, Gang C <gang.c.chen(a)intel.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Jincheng Li <jincheng.li(a)intel.com> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Comment-Date: Wed, 24 Apr 2024 13:53:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: MAINTAINERS: Add Granite Rapids FSP into Xeon-SP
by Shuo Liu (Code Review) 24 Apr '24

24 Apr '24

Shuo Liu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/82082?usp=email ) Change subject: MAINTAINERS: Add Granite Rapids FSP into Xeon-SP ...................................................................... MAINTAINERS: Add Granite Rapids FSP into Xeon-SP Change-Id: I5170a69d798d0e8198b89f6932a80e6051228ac2 Signed-off-by: Shuo Liu <shuo.liu(a)intel.com> --- M MAINTAINERS 1 file changed, 1 insertion(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/82082/1 diff --git a/MAINTAINERS b/MAINTAINERS index 2963ab4..c23e3d0 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -951,6 +951,7 @@ F: src/vendorcode/intel/fsp/fsp2_0/skylake_sp/ F: src/vendorcode/intel/fsp/fsp2_0/copperlake_sp/ F: src/vendorcode/intel/fsp/fsp2_0/sapphirerapids_sp/ +F: src/vendorcode/intel/fsp/fsp2_0/graniterapids/ MEDIATEK SOCS M: Hung-Te Lin <hungte(a)chromium.org> -- To view, visit https://review.coreboot.org/c/coreboot/+/82082?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I5170a69d798d0e8198b89f6932a80e6051228ac2 Gerrit-Change-Number: 82082 Gerrit-PatchSet: 1 Gerrit-Owner: Shuo Liu <shuo.liu(a)intel.com> Gerrit-MessageType: newchange

1 0

[XS] Change in coreboot[main]: soc/intel/xeon_sp: Support CHIPSET_LOCKDOWN_FSP
by Shuo Liu (Code Review) 24 Apr '24

24 Apr '24

Attention is currently required from: Arthur Heymans, Christian Walter, Johnny Lin, Jonathan Zhang, Lean Sheng Tan, Patrick Rudolph, Tim Chu. Shuo Liu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/82081?usp=email ) Change subject: soc/intel/xeon_sp: Support CHIPSET_LOCKDOWN_FSP ...................................................................... soc/intel/xeon_sp: Support CHIPSET_LOCKDOWN_FSP In a server platform many silicon specific register lock operations are by default in FSP space. CHIPSET_LOCKDOWN_FSP provides an option to make sure the codes could be used out-of-box to build products. Change-Id: I8efcc1f27446be8e35f51e2568c4af6f8165486b Signed-off-by: Shuo Liu <shuo.liu(a)intel.com> --- M src/soc/intel/xeon_sp/lockdown.c 1 file changed, 4 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/82081/1 diff --git a/src/soc/intel/xeon_sp/lockdown.c b/src/soc/intel/xeon_sp/lockdown.c index 9e25920..a3d17b4 100644 --- a/src/soc/intel/xeon_sp/lockdown.c +++ b/src/soc/intel/xeon_sp/lockdown.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */ +#include <intelblocks/cfg.h> #include <intelblocks/lpc_lib.h> #include <intelpch/lockdown.h> #include <soc/lockdown.h> @@ -20,6 +21,9 @@ void soc_lockdown_config(int chipset_lockdown) { + if (chipset_lockdown == CHIPSET_LOCKDOWN_FSP) + return; + lpc_lockdown_config(); pmc_lockdown_config(); sata_lockdown_config(chipset_lockdown); -- To view, visit https://review.coreboot.org/c/coreboot/+/82081?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I8efcc1f27446be8e35f51e2568c4af6f8165486b Gerrit-Change-Number: 82081 Gerrit-PatchSet: 1 Gerrit-Owner: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-MessageType: newchange

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit April 2024