coreboot-gerrit November 2022

coreboot-gerrit@coreboot.org

1 participants
7286 discussions

[S] Change in coreboot[master]: UPSTREAM: cbfstool: Fix possible memory leak
by Shahina Shaik (Code Review) 07 Nov '22

07 Nov '22

Attention is currently required from: V Sowmya, Shahina Shaik, Haribalaraman Ramasubramanian, Sridhar Siricilla, Balaji Manigandan, Yu-Ping Wu. Shahina Shaik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69220 ) Change subject: UPSTREAM: cbfstool: Fix possible memory leak ...................................................................... Patch Set 4: (3 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/69220/comment/c0b0cc9e_f712be4e PS4, Line 7: UPSTREAM > Please remove the `UPSTREAM` prefix. Removed the "UPSTREAM" prefix from commit message. File util/cbfstool/cbfstool.c: https://review.coreboot.org/c/coreboot/+/69220/comment/d072e644_d8ac1c50 PS4, Line 773: header_offset > Take the chance to fix the indent (by aligning with `&image`)? Fixed the indent as suggested. File util/cbfstool/common.c: https://review.coreboot.org/c/coreboot/+/69220/comment/308a1135_fc03680d PS4, Line 40: } > Not related to your change, but I think writing `return -1` here is more straightforward In line no:30, strdup is using malloc internally to allocate memory for the buffer->name, which has to be freed incase of there is no memory allocated for "buffer->data. -- To view, visit https://review.coreboot.org/c/coreboot/+/69220 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I01c4643d1e671d9bd9971ac6db8031634fffd61e Gerrit-Change-Number: 69220 Gerrit-PatchSet: 4 Gerrit-Owner: Shahina Shaik <shahina.shaik(a)intel.com> Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Reviewer: Haribalaraman Ramasubramanian <haribalaraman.r(a)intel.com> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Shahina Shaik <shahina.shaik(a)intel.corp-partner.google.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Attention: V Sowmya <v.sowmya(a)intel.com> Gerrit-Attention: Shahina Shaik <shahina.shaik(a)intel.com> Gerrit-Attention: Haribalaraman Ramasubramanian <haribalaraman.r(a)intel.com> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Comment-Date: Mon, 07 Nov 2022 06:35:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Yu-Ping Wu <yupingso(a)google.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: soc/intel: Use PCI offset 0x10 over static `Index 0` for PMC
by Werner Zeh (Code Review) 07 Nov '22

07 Nov '22

Attention is currently required from: Tarun Tuli, Subrata Banik, Kapil Porwal, Arthur Heymans, Lean Sheng Tan. Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69227 ) Change subject: soc/intel: Use PCI offset 0x10 over static `Index 0` for PMC ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: The PMC bar is set up in booblock/pch.c for these SoCs (meteorlkae seems to have it in soc_die.c) and here PWRMBASE is used (whihc is 0x10 so matching the BAR0 define). Should we synchronize these two defines to stay consistent? So either use PWRMBASE in this patch or push another one to fix it in pch.c. WDYT? -- To view, visit https://review.coreboot.org/c/coreboot/+/69227 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iee2523876a8045e70effd5824afc327d1113038b Gerrit-Change-Number: 69227 Gerrit-PatchSet: 2 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Comment-Date: Mon, 07 Nov 2022 05:57:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[L] Change in coreboot[master]: [WIP] mb/prodrive/atlas: Populate smbios table with VPD from ECs EMI
by Maximilian Brune (Code Review) 07 Nov '22

07 Nov '22

Attention is currently required from: David Milosevic. Maximilian Brune has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68137 ) Change subject: [WIP] mb/prodrive/atlas: Populate smbios table with VPD from ECs EMI ...................................................................... Patch Set 6: (12 comments) File src/mainboard/prodrive/atlas/emi.h: https://review.coreboot.org/c/coreboot/+/68137/comment/f176343d_1809f136 PS6, Line 33: typedef enum { avoid typedef as per coreboot coding style https://review.coreboot.org/c/coreboot/+/68137/comment/04168093_d79c0636 PS6, Line 36: EMI_INSTANCE_1 ///< EMI instance 1 (EMI1) the comment is unnecessary. It would probably make more sense to explain in short what an EMI_INSTANCE/EMI_REGION actually is and how they relate to each other. https://review.coreboot.org/c/coreboot/+/68137/comment/60c1a9ce_5de4d728 PS6, Line 40: typedef enum { avoid typedef as per coreboot coding style https://review.coreboot.org/c/coreboot/+/68137/comment/b9f1d664_35058d6a PS6, Line 43: EMI_REGION_1 ///< EMI region 1 the comment is unnecessary https://review.coreboot.org/c/coreboot/+/68137/comment/8b395072_bc0af328 PS6, Line 47: typedef enum { avoid typedef as per coreboot coding style https://review.coreboot.org/c/coreboot/+/68137/comment/fd5f3490_141e14a5 PS6, Line 76: const u16 addr, do not use tabs for this kind of indentation. Since this kind of indentation is supposed to look the same on all editors, spaces are far more appropriate. If jenkins complains about that, ignore it. File src/mainboard/prodrive/atlas/emi.c: https://review.coreboot.org/c/coreboot/+/68137/comment/e44d5634_b9f464d3 PS6, Line 41: int emi_read_region(const EMI_INSTANCE instance, const EMI_REGION region, as far as I can see this function is only used by "emi_read_region_blocK". You can therefore completely remove this function and put only a very small subset of it into "emi_read_region_block", since you don't need the whole switch case then. https://review.coreboot.org/c/coreboot/+/68137/comment/ada21463_b7d0a76f PS6, Line 83: { wouldn't it be easier to directly use an u32 value instead of a pointer? It would remove the need to cast (below) and would also get rid of endianness issues. Also it would be easier to call the function. Also it's generally safer to not use pointers if possible. Even better: remove the function, since it isn't used anyway (only clutters up code). https://review.coreboot.org/c/coreboot/+/68137/comment/4ca8989d_73c58c17 PS6, Line 139: inline void emi_write_register(const EMI_INSTANCE instance, const u8 offs, const u8 value) function can be removed, since it's not used anywhere (only clutters up code). https://review.coreboot.org/c/coreboot/+/68137/comment/d75db0fa_a2c5bffc PS6, Line 144: inline u8 emi_read_register(const EMI_INSTANCE instance, const u8 offs) function can be removed, since it's not used anywhere (only clutters up code). File src/mainboard/prodrive/atlas/vpd.h: https://review.coreboot.org/c/coreboot/+/68137/comment/930cc522_d4c82ef0 PS6, Line 26: #define vpd(x) offsetof(vpd_section_t, x) this macro only makes the rest of the code far less obvious. File src/mainboard/prodrive/atlas/vpd.c: https://review.coreboot.org/c/coreboot/+/68137/comment/5bc04c02_34fb7de9 PS6, Line 36: case vpd(part_number): return PN_LENGTH; as mentioned above, this macro makes it very hard to read (especially considering it has the same name as the parameter). It would be much more readable if you would directly write: `case offsetof(struct vpd_section, part_number):` -- To view, visit https://review.coreboot.org/c/coreboot/+/68137 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I47bb4883c43ff344a9bda92c3106dd025533b391 Gerrit-Change-Number: 68137 Gerrit-PatchSet: 6 Gerrit-Owner: David Milosevic <David.Milosevic(a)9elements.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: David Milosevic <David.Milosevic(a)9elements.com> Gerrit-Comment-Date: Mon, 07 Nov 2022 04:44:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: UPSTREAM: cbfstool: Fix possible memory leak
by Yu-Ping Wu (Code Review) 07 Nov '22

07 Nov '22

Attention is currently required from: V Sowmya, Shahina Shaik, Haribalaraman Ramasubramanian, Sridhar Siricilla, Balaji Manigandan. Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69220 ) Change subject: UPSTREAM: cbfstool: Fix possible memory leak ...................................................................... Patch Set 4: (3 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/69220/comment/2ac60dff_93bcf5ed PS4, Line 7: UPSTREAM Please remove the `UPSTREAM` prefix. File util/cbfstool/cbfstool.c: https://review.coreboot.org/c/coreboot/+/69220/comment/0de46e2c_ac923f1b PS4, Line 773: header_offset Take the chance to fix the indent (by aligning with `&image`)? File util/cbfstool/common.c: https://review.coreboot.org/c/coreboot/+/69220/comment/3cddb9d1_37586662 PS4, Line 40: } Not related to your change, but I think writing `return -1` here is more straightforward -- To view, visit https://review.coreboot.org/c/coreboot/+/69220 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I01c4643d1e671d9bd9971ac6db8031634fffd61e Gerrit-Change-Number: 69220 Gerrit-PatchSet: 4 Gerrit-Owner: Shahina Shaik <shahina.shaik(a)intel.com> Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Reviewer: Haribalaraman Ramasubramanian <haribalaraman.r(a)intel.com> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Shahina Shaik <shahina.shaik(a)intel.corp-partner.google.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Attention: V Sowmya <v.sowmya(a)intel.com> Gerrit-Attention: Shahina Shaik <shahina.shaik(a)intel.com> Gerrit-Attention: Haribalaraman Ramasubramanian <haribalaraman.r(a)intel.com> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Shahina Shaik <shahina.shaik(a)intel.corp-partner.google.com> Gerrit-Attention: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Comment-Date: Mon, 07 Nov 2022 04:42:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: drivers/i2c/sx9324: Add support for Linux's SX9324 driver
by Victor Ding (Code Review) 07 Nov '22

07 Nov '22

Attention is currently required from: Jason Nien, Martin Roth. Victor Ding has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69192 ) Change subject: drivers/i2c/sx9324: Add support for Linux's SX9324 driver ...................................................................... Patch Set 7: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/69192/comment/394f346c_d90a7af1 PS6, Line 26: TEST=Dump ACPI SSDT then verify _DSD entries related to the legacy > nit: do you mind including what board(s)/platform(s) you verified the SSDT on Done -- To view, visit https://review.coreboot.org/c/coreboot/+/69192 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I42cd6841c3a270c242ed2e739db245e858eadb3b Gerrit-Change-Number: 69192 Gerrit-PatchSet: 7 Gerrit-Owner: Victor Ding <victording(a)google.com> Gerrit-Reviewer: Jason Nien <jason.nien(a)amd.corp-partner.google.com> Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Reviewer: Reka Norman <rekanorman(a)chromium.org> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jason Nien <jason.nien(a)amd.corp-partner.google.com> Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Comment-Date: Mon, 07 Nov 2022 04:28:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tarun Tuli <taruntuli(a)google.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: lib: Add LPDDR5x DRAM type
by 9elements QA (Code Review) 07 Nov '22

07 Nov '22

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69226 ) Change subject: lib: Add LPDDR5x DRAM type ...................................................................... Patch Set 3: Automatic boot test returned (PASS/FAIL/TOTAL): 11 / 1 / 12 PASS: x86_32 "Hermes CFL" , build config PRODRIVE_HERMES_ and payload TianoCore_UefiPayloadPkg : https://lava.9esec.io/r/134079 PASS: x86_32 "Hermes CFL" , build config PRODRIVE_HERMES and payload TianoCore_UefiPayloadPkg : https://lava.9esec.io/r/134078 PASS: x86_32 "ThinkPad T500" , build config LENOVO_T500 and payload SeaBIOS : https://lava.9esec.io/r/134077 PASS: x86_32 "HP Z220 SFF Workstation" , build config HP_Z220_SFF_WORKSTATION and payload LinuxBoot_BB_kexec : https://lava.9esec.io/r/134076 PASS: x86_64 "HP Compaq 8200 Elite SFF PC" , build config HP_COMPAQ_8200_ELITE_SFF_PC.X86_64 and payload TianoCore : https://lava.9esec.io/r/134075 PASS: x86_64 "HP Compaq 8200 Elite SFF PC" , build config HP_COMPAQ_8200_ELITE_SFF_PC.X86_64 and payload SeaBIOS : https://lava.9esec.io/r/134074 FAIL: x86_32 "HP Compaq 8200 Elite SFF PC" , build config HP_COMPAQ_8200_ELITE_SFF_PC and payload TianoCore : https://lava.9esec.io/r/134073 PASS: x86_32 "HP Compaq 8200 Elite SFF PC" , build config HP_COMPAQ_8200_ELITE_SFF_PC and payload SeaBIOS : https://lava.9esec.io/r/134072 PASS: x86_64 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX_X86_64 and payload SeaBIOS : https://lava.9esec.io/r/134071 PASS: x86_32 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX_ASAN and payload SeaBIOS : https://lava.9esec.io/r/134070 PASS: x86_32 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX_ and payload SeaBIOS : https://lava.9esec.io/r/134069 PASS: x86_32 "QEMU x86 i440fx/piix4" , build config EMULATION_QEMU_X86_I440FX and payload SeaBIOS : https://lava.9esec.io/r/134068 Please note: This test is under development and might not be accurate at all! -- To view, visit https://review.coreboot.org/c/coreboot/+/69226 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If620cf51133ca295fd3f1cbecbb472beb337b9fc Gerrit-Change-Number: 69226 Gerrit-PatchSet: 3 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Ivy Jian <ivy.jian(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com> Gerrit-Comment-Date: Mon, 07 Nov 2022 04:28:04 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: drivers/i2c/sx9324: Add support for Linux's SX9324 driver
by Victor Ding (Code Review) 07 Nov '22

07 Nov '22

Attention is currently required from: Victor Ding, Jason Nien, Martin Roth. Hello build bot (Jenkins), Tarun Tuli, Jason Nien, Reka Norman, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/69192 to look at the new patch set (#7). Change subject: drivers/i2c/sx9324: Add support for Linux's SX9324 driver ...................................................................... drivers/i2c/sx9324: Add support for Linux's SX9324 driver SX9324 driver is updated per Linux's documentation found at https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Doc… Supporting logic for the deprecated SX932x driver is hence guarded by DRIVERS_I2C_SX9324_SUPPORT_LEGACY_LINUX_DRIVER This patch by itself does not introduce functional changes to any board. The legacy SX932x Linux driver never reached upstream Linux and is only available in ChromeOS kernel fork of 4.4 and 5.4. Linux later accepted a different implementation named SX9324 and has been available since 5.4. Ideally all variants should adopt the new driver; however, during the transition phase, coreboot must support both drivers. It is better to have a single firmware build that can work with both Linux kernel drivers by specifying both sets of properties. Legacy driver support should be deleted once all variants finish migration. BUG=b:242662878 TEST=Dump ACPI SSDT then verify _DSD entries related to the legacy SX932x driver are identical w/ and w/o this patch (Tested on Craask and Nivviks) Change-Id: I42cd6841c3a270c242ed2e739db245e858eadb3b Signed-off-by: Victor Ding <victording(a)google.com> --- M src/drivers/i2c/sx9324/Kconfig M src/drivers/i2c/sx9324/chip.h M src/drivers/i2c/sx9324/registers.h M src/drivers/i2c/sx9324/sx9324.c M src/mainboard/google/brya/Kconfig.name M src/mainboard/google/dedede/Kconfig.name M src/mainboard/google/zork/Kconfig.name 7 files changed, 219 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/69192/7 -- To view, visit https://review.coreboot.org/c/coreboot/+/69192 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I42cd6841c3a270c242ed2e739db245e858eadb3b Gerrit-Change-Number: 69192 Gerrit-PatchSet: 7 Gerrit-Owner: Victor Ding <victording(a)google.com> Gerrit-Reviewer: Jason Nien <jason.nien(a)amd.corp-partner.google.com> Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Reviewer: Reka Norman <rekanorman(a)chromium.org> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Victor Ding <victording(a)google.com> Gerrit-Attention: Jason Nien <jason.nien(a)amd.corp-partner.google.com> Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in coreboot[master]: mb/google/nissa/var/craask: Add ambient thermal sensor settings
by Vidya Gopalakrishnan (Code Review) 07 Nov '22

07 Nov '22

Attention is currently required from: V Sowmya, Tarun Tuli, Kangheui Won, Sumeet R Pawnikar, Tyler Wang, Nick Vaccaro, Peter Ou. Vidya Gopalakrishnan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69197 ) Change subject: mb/google/nissa/var/craask: Add ambient thermal sensor settings ...................................................................... Patch Set 6: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/69197 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I026a8b3e1a27bedc3e0082e15e80a74a2f8adfda Gerrit-Change-Number: 69197 Gerrit-PatchSet: 6 Gerrit-Owner: Tyler Wang <tyler.wang(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Kangheui Won <khwon(a)chromium.org> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Peter Ou <peter.ou(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Reka Norman <rekanorman(a)chromium.org> Gerrit-Reviewer: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: Vidya Gopalakrishnan <vidya.gopalakrishnan(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Cathy Chen <cathy_chen(a)quanta.corp-partner.google.com> Gerrit-CC: David Wu <david_wu(a)quanta.corp-partner.google.com> Gerrit-CC: Ren Kuo <ren.kuo(a)quanta.corp-partner.google.com> Gerrit-Attention: V Sowmya <v.sowmya(a)intel.com> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Kangheui Won <khwon(a)chromium.org> Gerrit-Attention: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com> Gerrit-Attention: Tyler Wang <tyler.wang(a)quanta.corp-partner.google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Peter Ou <peter.ou(a)quanta.corp-partner.google.com> Gerrit-Comment-Date: Mon, 07 Nov 2022 04:07:32 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: Revert "Revert "drivers/ocp/ewl: Add EWL driver for EWL type 3 error ...
by Johnny Lin (Code Review) 07 Nov '22

07 Nov '22

Johnny Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/69280 ) Change subject: Revert "Revert "drivers/ocp/ewl: Add EWL driver for EWL type 3 error handling"" ...................................................................... Revert "Revert "drivers/ocp/ewl: Add EWL driver for EWL type 3 error handling"" This reverts commit c8f25e0c0551821554b96c449b6323de8a14a030. Change-Id: Ib83b7653a839d18a065b929127549acd10bce7a7 Signed-off-by: Johnny Lin <johnny_lin(a)wiwynn.com> --- A src/drivers/ocp/ewl/Kconfig A src/drivers/ocp/ewl/Makefile.inc A src/drivers/ocp/ewl/ewl.c A src/drivers/ocp/ewl/ocp_ewl.h 4 files changed, 86 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/69280/1 diff --git a/src/drivers/ocp/ewl/Kconfig b/src/drivers/ocp/ewl/Kconfig new file mode 100644 index 0000000..3c3b82d --- /dev/null +++ b/src/drivers/ocp/ewl/Kconfig @@ -0,0 +1,5 @@ +config OCP_EWL + bool + default n + help + It implements checking FSP Enhanced Warning Log Hob. diff --git a/src/drivers/ocp/ewl/Makefile.inc b/src/drivers/ocp/ewl/Makefile.inc new file mode 100644 index 0000000..43112e9 --- /dev/null +++ b/src/drivers/ocp/ewl/Makefile.inc @@ -0,0 +1 @@ +romstage-$(CONFIG_OCP_EWL) += ewl.c diff --git a/src/drivers/ocp/ewl/ewl.c b/src/drivers/ocp/ewl/ewl.c new file mode 100644 index 0000000..d70a23f --- /dev/null +++ b/src/drivers/ocp/ewl/ewl.c @@ -0,0 +1,58 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#include <soc/soc_util.h> +#include <lib.h> +#include "ocp_ewl.h" + +static void process_ewl_type3(EWL_ENTRY_HEADER *header, EWL_ENTRY_MEMORY_LOCATION memory_location) +{ + /* Treat warning as type 3, collect basic information and print to serial log */ + EWL_ENTRY_TYPE3 *basic_warning; + basic_warning = (EWL_ENTRY_TYPE3 *)header; + printk(BIOS_ERR, "Major Warning Code = 0x%02x, Minor Warning Code = 0x%02x,\n", + basic_warning->Context.MajorWarningCode, + basic_warning->Context.MinorWarningCode); + printk(BIOS_ERR, "Major Checkpoint: 0x%02x\n", basic_warning->Context.MajorCheckpoint); + printk(BIOS_ERR, "Minor Checkpoint: 0x%02x\n", basic_warning->Context.MinorCheckpoint); + + if (memory_location.Socket != 0xff) + printk(BIOS_ERR, "Socket %d\n", memory_location.Socket); + if (memory_location.Channel != 0xff) + printk(BIOS_ERR, "Channel %d\n", memory_location.Channel); + if (memory_location.Dimm != 0xff) + printk(BIOS_ERR, "Dimm %d\n", memory_location.Dimm); + if (memory_location.Rank != 0xff) + printk(BIOS_ERR, "Rank %d\n", memory_location.Rank); +} + +void get_ewl(void) +{ + const EWL_PRIVATE_DATA *hob = get_ewl_hob(); + int offset = 0; + bool type3_flag = 0; + EWL_ENTRY_HEADER *warning_header; + printk(BIOS_DEBUG, "Number of EWL entries %d\n", hob->numEntries); + while (offset < hob->status.Header.FreeOffset) { + warning_header = (EWL_ENTRY_HEADER *) &(hob->status.Buffer[offset]); + if (warning_header->Type == EwlType3) { + printk(BIOS_ERR, "EWL type: %d\n", warning_header->Type); + if (warning_header->Size != sizeof(EWL_ENTRY_TYPE3)) { + printk(BIOS_ERR, "EWL type3 size mismatch!\n"); + return; + } + EWL_ENTRY_TYPE3 *type3; + type3 = (EWL_ENTRY_TYPE3 *)warning_header; + process_ewl_type3(warning_header, type3->MemoryLocation); + type3_flag = 1; + } else { + printk(BIOS_DEBUG, "EWL type: %d size:%d severity level:%d\n", + warning_header->Type, + warning_header->Size, + warning_header->Severity); + hexdump(&(hob->status.Buffer[offset]), warning_header->Size); + } + offset += warning_header->Size; + } + if (type3_flag) + die("Memory Training Error!\n"); +} diff --git a/src/drivers/ocp/ewl/ocp_ewl.h b/src/drivers/ocp/ewl/ocp_ewl.h new file mode 100644 index 0000000..38d5286d --- /dev/null +++ b/src/drivers/ocp/ewl/ocp_ewl.h @@ -0,0 +1,10 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#ifndef __OCP_EWL_H +#define __OCP_EWL_H + +#include <soc/soc_util.h> + +void get_ewl(void); + +#endif -- To view, visit https://review.coreboot.org/c/coreboot/+/69280 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib83b7653a839d18a065b929127549acd10bce7a7 Gerrit-Change-Number: 69280 Gerrit-PatchSet: 1 Gerrit-Owner: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-MessageType: newchange

1 0

[S] Change in coreboot[master]: Revert "Revert "soc/intel/xeon_sp/cpx: Add get_ewl_hob() utility func...
by Johnny Lin (Code Review) 07 Nov '22

07 Nov '22

Johnny Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/69279 ) Change subject: Revert "Revert "soc/intel/xeon_sp/cpx: Add get_ewl_hob() utility function"" ...................................................................... Revert "Revert "soc/intel/xeon_sp/cpx: Add get_ewl_hob() utility function"" This reverts commit e141f358eb1b4386c041ed433e8977c4cdf409c1. Change-Id: Iec715d687a4452bb6f48a3328865e53572a39ec1 Signed-off-by: Johnny Lin <johnny_lin(a)wiwynn.com> --- M src/soc/intel/xeon_sp/cpx/Makefile.inc M src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h M src/soc/intel/xeon_sp/cpx/soc_util.c 3 files changed, 29 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/69279/1 diff --git a/src/soc/intel/xeon_sp/cpx/Makefile.inc b/src/soc/intel/xeon_sp/cpx/Makefile.inc index 0cd267b..7e18b92 100644 --- a/src/soc/intel/xeon_sp/cpx/Makefile.inc +++ b/src/soc/intel/xeon_sp/cpx/Makefile.inc @@ -5,7 +5,7 @@ subdirs-y += ../../../../cpu/intel/turbo subdirs-y += ../../../../cpu/intel/microcode -romstage-y += romstage.c ddr.c +romstage-y += romstage.c ddr.c soc_util.c romstage-$(CONFIG_DISPLAY_UPD_DATA) += upd_display.c romstage-$(CONFIG_DISPLAY_HOBS) += hob_display.c 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 5ce3928..c4e2a7d 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 @@ -5,11 +5,13 @@ #include <hob_iiouds.h> #include <hob_memmap.h> +#include <hob_enhancedwarningloglib.h> const struct SystemMemoryMapHob *get_system_memory_map(void); uint32_t get_socket_stack_busno(uint32_t socket, uint32_t stack); int soc_get_stack_for_port(int port); +const EWL_PRIVATE_DATA *get_ewl_hob(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 cd1e06d..2a8b6c9 100644 --- a/src/soc/intel/xeon_sp/cpx/soc_util.c +++ b/src/soc/intel/xeon_sp/cpx/soc_util.c @@ -9,6 +9,20 @@ #include <soc/soc_util.h> #include <soc/util.h> +const EWL_PRIVATE_DATA *get_ewl_hob(void) +{ + size_t hob_size; + static const EWL_PRIVATE_DATA *hob; + const uint8_t ewl_id_hob_guid[16] = FSP_HOB_EWLID_GUID; + + if (hob != NULL) + return hob; + + hob = fsp_find_extension_hob_by_guid(ewl_id_hob_guid, &hob_size); + assert(hob != NULL && hob_size != 0); + return hob; +} + const struct SystemMemoryMapHob *get_system_memory_map(void) { size_t hob_size; -- To view, visit https://review.coreboot.org/c/coreboot/+/69279 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iec715d687a4452bb6f48a3328865e53572a39ec1 Gerrit-Change-Number: 69279 Gerrit-PatchSet: 1 Gerrit-Owner: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-MessageType: newchange

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit November 2022