coreboot-gerrit April 2024

coreboot-gerrit@coreboot.org

1 participants
3292 discussions

[M] Change in coreboot[main]: mb/google/brya/var/xol: Reduce power limits according to battery status
by SH Kim (Code Review) 03 Apr '24

03 Apr '24

Attention is currently required from: Dinesh Gehlot, Kapil Porwal, Nick Vaccaro, Subrata Banik, Sumeet R Pawnikar. SH Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81614?usp=email ) Change subject: mb/google/brya/var/xol: Reduce power limits according to battery status ...................................................................... Patch Set 5: (1 comment) Patchset: PS5: Can we merge this? -- To view, visit https://review.coreboot.org/c/coreboot/+/81614?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: I5d71e9edde0ecbd7aaf316cd754a6ebcff9da77d Gerrit-Change-Number: 81614 Gerrit-PatchSet: 5 Gerrit-Owner: SH Kim <sh_.kim(a)samsung.corp-partner.google.com> Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Eric Lai <ericllai(a)google.com> Gerrit-Reviewer: Jamie Chen <jamie.chen(a)intel.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Comment-Date: Wed, 03 Apr 2024 23:16:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[L] Change in coreboot[main]: mb/erying: Add Erying Polestar G613 Pro (TGL-H)
by Felix Singer (Code Review) 03 Apr '24

03 Apr '24

Attention is currently required from: Alicja Michalska, Michał Żygowski, Nicholas Chin, Paul Menzel. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80853?usp=email ) Change subject: mb/erying: Add Erying Polestar G613 Pro (TGL-H) ...................................................................... Patch Set 6: (12 comments) File src/mainboard/erying/tgl/devicetree.cb: https://review.coreboot.org/c/coreboot/+/80853/comment/814f1db9_40ebca74 : PS6, Line 19: register "SkipExtGfxScan" = "0" Set to 0, which is the default. Remove. https://review.coreboot.org/c/coreboot/+/80853/comment/205b1d1d_93a5330f : PS6, Line 21: register "s0ix_enable" = "0" Set to 0, which is the default. Remove. https://review.coreboot.org/c/coreboot/+/80853/comment/81f5ab85_5b72b377 : PS6, Line 23: # Actual device tree begins here: Seems superfluous. https://review.coreboot.org/c/coreboot/+/80853/comment/b3defb02_477b8403 : PS6, Line 26: device ref system_agent on end System agent is enabled by default, remove. https://review.coreboot.org/c/coreboot/+/80853/comment/2741e1f5_75d692b9 : PS6, Line 56: [0] = USB2_PORT_MID(OC0), /* Rear, bottom right */ : [1] = USB2_PORT_MID(OC0), /* Rear, bottom left */ : [2] = USB2_PORT_MID(OC2), /* NIC left */ : [3] = USB2_PORT_MID(OC2), /* NIC right */ : [4] = USB2_PORT_MID(OC2), /* Front Panel 1 */ : [5] = USB2_PORT_MID(OC2), /* Front Panel 2 */ : [8] = USB2_PORT_MID(OC0), /* Front Panel 1 (USB3) */ : [9] = USB2_PORT_MID(OC0), /* Front Panel 2 (USB3) */ : [10] = USB2_PORT_MID(OC0), /* Rear, top left */ : Add one more tab https://review.coreboot.org/c/coreboot/+/80853/comment/7e236ed9_5b0dea1f : PS6, Line 69: [0] = USB3_PORT_DEFAULT(OC0), /* Rear, bottom right */ : [1] = USB3_PORT_DEFAULT(OC0), /* Rear, bottom left */ : [2] = USB3_PORT_DEFAULT(OC0), /* Front Panel 1 */ : [3] = USB3_PORT_DEFAULT(OC0), /* Front Panel 2 */ : [4] = USB3_PORT_DEFAULT(OC0), /* Rear, top left */ : Add one more tab https://review.coreboot.org/c/coreboot/+/80853/comment/4329a299_202bc045 : PS6, Line 81: register "SataMode" = "0" AHCI is the default, remove. File src/mainboard/erying/tgl/ramstage.c: PS6: Same as with romstage_fsp_params.c File src/mainboard/erying/tgl/romstage_fsp_params.c: PS6: I marked a few options which seemed obvious to me. Please have a look at the SoC code and the FSP defaults. Some of them might not be needed since your configuration might correspond to the defaults and there might be an option for others. https://review.coreboot.org/c/coreboot/+/80853/comment/ff511e1c_0cf08002 : PS6, Line 25: mupd->FspmConfig.HyperThreading = 1; Remove. This is hooked up to the runtime setting `hyper_threading` and also to Kconfig. https://review.coreboot.org/c/coreboot/+/80853/comment/41ee9f09_59d04c8f : PS6, Line 58: mupd->FspmConfig.VmxEnable = 1; Remove. This option is hooked up to the `ENABLE_VMX` Kconfig option. https://review.coreboot.org/c/coreboot/+/80853/comment/58eba175_78bf60b9 : PS6, Line 59: mupd->FspmConfig.SmbusEnable = 1; Remove. That option is hooked up to the devicetree and it's enabled there. -- To view, visit https://review.coreboot.org/c/coreboot/+/80853?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: Iffb9e357da2eb686bdcd9a9837df8a60fa94011e Gerrit-Change-Number: 80853 Gerrit-PatchSet: 6 Gerrit-Owner: Alicja Michalska <ahplka19(a)gmail.com> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-CC: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Alicja Michalska <ahplka19(a)gmail.com> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Comment-Date: Wed, 03 Apr 2024 21:06:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: util/crossgcc/buildgcc: Use Intel mirror for ACPICA
by Elyes Haouas (Code Review) 03 Apr '24

03 Apr '24

Attention is currently required from: Felix Singer, Martin L Roth. Elyes Haouas has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80721?usp=email ) Change subject: util/crossgcc/buildgcc: Use Intel mirror for ACPICA ...................................................................... Patch Set 7: (1 comment) File util/crossgcc/buildgcc: https://review.coreboot.org/c/coreboot/+/80721/comment/f30e1c34_13e5cfed : PS7, Line 77: 783534 > Yeah right, but I don't think it's a big issue. […] Acknowledged -- To view, visit https://review.coreboot.org/c/coreboot/+/80721?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: If3738b0cdab07c37ac1459a53e399e5de54435d5 Gerrit-Change-Number: 80721 Gerrit-PatchSet: 7 Gerrit-Owner: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Varshit Pandya <pandyavarshit(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Comment-Date: Wed, 03 Apr 2024 20:42:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Comment-In-Reply-To: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: Documentation: Add Erying Polestar G613 Pro
by Alicja Michalska (Code Review) 03 Apr '24

03 Apr '24

Attention is currently required from: Alicja Michalska. Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/81611?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Verified-1 by build bot (Jenkins) Change subject: Documentation: Add Erying Polestar G613 Pro ...................................................................... Documentation: Add Erying Polestar G613 Pro Documentation entry has to be submitted with board tree that's currently being reviewed upstream. Change-Id: I5d60508dbde10373b0da2fb4ece0992760d3121c Signed-off-by: Alicja Michalska <ahplka19(a)gmail.com> --- A Documentation/mainboard/erying/tgl_matx.md A Documentation/mainboard/erying/tgl_matx_board.jpg M Documentation/mainboard/index.md 3 files changed, 144 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/81611/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/81611?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: I5d60508dbde10373b0da2fb4ece0992760d3121c Gerrit-Change-Number: 81611 Gerrit-PatchSet: 2 Gerrit-Owner: Alicja Michalska <ahplka19(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Alicja Michalska <ahplka19(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in coreboot[main]: util/crossgcc/buildgcc: Use Intel mirror for ACPICA
by Felix Singer (Code Review) 03 Apr '24

03 Apr '24

Attention is currently required from: Elyes Haouas, Martin L Roth. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80721?usp=email ) Change subject: util/crossgcc/buildgcc: Use Intel mirror for ACPICA ...................................................................... Patch Set 7: (1 comment) File util/crossgcc/buildgcc: https://review.coreboot.org/c/coreboot/+/80721/comment/bbd93591_cbe57834 : PS7, Line 77: 783534 > looks like this works only for version "20230628" Yeah right, but I don't think it's a big issue. We will have to adjust the version string in the buildgcc script anyway. We had several issues with GitHub in the past, so I rather want stable tarball hashes instead of less bits that need to be updated. -- To view, visit https://review.coreboot.org/c/coreboot/+/80721?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: If3738b0cdab07c37ac1459a53e399e5de54435d5 Gerrit-Change-Number: 80721 Gerrit-PatchSet: 7 Gerrit-Owner: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Varshit Pandya <pandyavarshit(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Comment-Date: Wed, 03 Apr 2024 19:02:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: util/intelp2m: Add support for TigerLake-H SoC
by Maxim (Code Review) 03 Apr '24

03 Apr '24

Attention is currently required from: Alicja Michalska. Maxim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81587?usp=email ) Change subject: util/intelp2m: Add support for TigerLake-H SoC ...................................................................... Patch Set 4: Code-Review+2 (1 comment) Patchset: PS4: Thank you for this work! -- To view, visit https://review.coreboot.org/c/coreboot/+/81587?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: I26eff225c2045edfe5836283be7b4c63f6b405e8 Gerrit-Change-Number: 81587 Gerrit-PatchSet: 4 Gerrit-Owner: Alicja Michalska <ahplka19(a)gmail.com> Gerrit-Reviewer: Maxim <max.senia.poliak(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Alicja Michalska <ahplka19(a)gmail.com> Gerrit-Comment-Date: Wed, 03 Apr 2024 18:32:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: vc/intel/fsp: Refactor FSP header inclusion for EDK2 compatibility
by Subrata Banik (Code Review) 03 Apr '24

03 Apr '24

Attention is currently required from: Appukuttan V K, Dinesh Gehlot, Kapil Porwal, Nick Vaccaro, Ronak Kanabar. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81623?usp=email ) Change subject: vc/intel/fsp: Refactor FSP header inclusion for EDK2 compatibility ...................................................................... Patch Set 5: (1 comment) File src/vendorcode/intel/fsp/fsp2_0/IntelFspPkg/Include/FspInfoHeader.h: https://review.coreboot.org/c/coreboot/+/81623/comment/bc3e4fa9_c30e15c2 : PS5, Line 7: vendorcode/intel/edk2/UDK2017/ > > > > > > > > > This prefix is used in other places in the tree. Can we use a Kconfig param for it that depends on the UDK kconfig? That would make this file trivial and same with vendorcode/intel/Makefile.mk > > > > > > > > > > > > > > > > can you please help to share a little more details which I can take as an incremental change. > > > > > > > > > > > > > > > > I'm sensing that you are asking to introduce a kconfig for each `UDK version` to mask the `vendorcode/intel/edk2/UDK2017` path ? > > > > > > > > > > > > > > > > Then, this file only includes only one header like > > > > > > > > > > > > > > > > > > > > > > > > ```#include <CONFIG(UDK_PATH)/Include/Guid/FspHeaderFile.h>``` > > > > > > > > > > > > > > > > UDK_PATH would depend on UDK configs like UDK_2017_VERSION etc.? > > > > > > > > > > > > > > Yes that would be my suggestion. It's used in other places (makefile) too so that's why I think it might be a good idea. > > > > > > > > > > > > sure, let me see if I can address it here or i will submit an incremental CL. sounds good to you ? > > > > > > > > > > Sure > > > > > > > > question: wondering if we define a Kconfig with type string then we might ended up with double quote (which we might need to strip before concatenate for #include)? > > > > > > > > ``` > > > > config UDK_2017_PATH > > > > string "vendorcode/intel/edk2/UDK2017" > > > > > > > > > > > > #include <CONFIG(UDK_PATH)/Include/Guid/FspHeaderFile.h> > > > > | > > > > v > > > > #include <"vendorcode/intel/edk2/UDK2017"/Include/Guid/FspHeaderFile.h> > > > > > > > > ``` > > > > > > Hmm I suppose makefile could do the quote stripping and passing it via a compiler commandline argument '-D' in CFLAGS to have it without quotes in C. > > > > yes possible for makefile. I'm wondering for #include in header file. > > I suppose the header is always included in a C file so if the UDK path is passed via commandline define it can be used in the #include statement in this file? yes, using makefile.mk for sure (for fsp_relocate.c) -- To view, visit https://review.coreboot.org/c/coreboot/+/81623?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: I29e5002821843c9cffbc8f6317d1062175f014ff Gerrit-Change-Number: 81623 Gerrit-PatchSet: 5 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Appukuttan V K <appukuttan.vk(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Appukuttan V K <appukuttan.vk(a)intel.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Comment-Date: Wed, 03 Apr 2024 18:29:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: vc/intel/fsp: Refactor FSP header inclusion for EDK2 compatibility
by Arthur Heymans (Code Review) 03 Apr '24

03 Apr '24

Attention is currently required from: Appukuttan V K, Dinesh Gehlot, Kapil Porwal, Nick Vaccaro, Ronak Kanabar, Subrata Banik. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81623?usp=email ) Change subject: vc/intel/fsp: Refactor FSP header inclusion for EDK2 compatibility ...................................................................... Patch Set 5: (1 comment) File src/vendorcode/intel/fsp/fsp2_0/IntelFspPkg/Include/FspInfoHeader.h: https://review.coreboot.org/c/coreboot/+/81623/comment/974327e3_dff00c6f : PS5, Line 7: vendorcode/intel/edk2/UDK2017/ > > > > > > > > This prefix is used in other places in the tree. Can we use a Kconfig param for it that depends on the UDK kconfig? That would make this file trivial and same with vendorcode/intel/Makefile.mk > > > > > > > > > > > > > > can you please help to share a little more details which I can take as an incremental change. > > > > > > > > > > > > > > I'm sensing that you are asking to introduce a kconfig for each `UDK version` to mask the `vendorcode/intel/edk2/UDK2017` path ? > > > > > > > > > > > > > > Then, this file only includes only one header like > > > > > > > > > > > > > > > > > > > > > ```#include <CONFIG(UDK_PATH)/Include/Guid/FspHeaderFile.h>``` > > > > > > > > > > > > > > UDK_PATH would depend on UDK configs like UDK_2017_VERSION etc.? > > > > > > > > > > > > Yes that would be my suggestion. It's used in other places (makefile) too so that's why I think it might be a good idea. > > > > > > > > > > sure, let me see if I can address it here or i will submit an incremental CL. sounds good to you ? > > > > > > > > Sure > > > > > > question: wondering if we define a Kconfig with type string then we might ended up with double quote (which we might need to strip before concatenate for #include)? > > > > > > ``` > > > config UDK_2017_PATH > > > string "vendorcode/intel/edk2/UDK2017" > > > > > > > > > #include <CONFIG(UDK_PATH)/Include/Guid/FspHeaderFile.h> > > > | > > > v > > > #include <"vendorcode/intel/edk2/UDK2017"/Include/Guid/FspHeaderFile.h> > > > > > > ``` > > > > Hmm I suppose makefile could do the quote stripping and passing it via a compiler commandline argument '-D' in CFLAGS to have it without quotes in C. > > yes possible for makefile. I'm wondering for #include in header file. I suppose the header is always included in a C file so if the UDK path is passed via commandline define it can be used in the #include statement in this file? -- To view, visit https://review.coreboot.org/c/coreboot/+/81623?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: I29e5002821843c9cffbc8f6317d1062175f014ff Gerrit-Change-Number: 81623 Gerrit-PatchSet: 5 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Appukuttan V K <appukuttan.vk(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Appukuttan V K <appukuttan.vk(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Comment-Date: Wed, 03 Apr 2024 18:14:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: vc/intel/fsp: Refactor FSP header inclusion for EDK2 compatibility
by Subrata Banik (Code Review) 03 Apr '24

03 Apr '24

Attention is currently required from: Appukuttan V K, Dinesh Gehlot, Kapil Porwal, Nick Vaccaro, Ronak Kanabar. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81623?usp=email ) Change subject: vc/intel/fsp: Refactor FSP header inclusion for EDK2 compatibility ...................................................................... Patch Set 5: (1 comment) File src/vendorcode/intel/fsp/fsp2_0/IntelFspPkg/Include/FspInfoHeader.h: https://review.coreboot.org/c/coreboot/+/81623/comment/303b0ec9_f8df4f86 : PS5, Line 7: vendorcode/intel/edk2/UDK2017/ > > > > > > > This prefix is used in other places in the tree. Can we use a Kconfig param for it that depends on the UDK kconfig? That would make this file trivial and same with vendorcode/intel/Makefile.mk > > > > > > > > > > > > can you please help to share a little more details which I can take as an incremental change. > > > > > > > > > > > > I'm sensing that you are asking to introduce a kconfig for each `UDK version` to mask the `vendorcode/intel/edk2/UDK2017` path ? > > > > > > > > > > > > Then, this file only includes only one header like > > > > > > > > > > > > > > > > > > ```#include <CONFIG(UDK_PATH)/Include/Guid/FspHeaderFile.h>``` > > > > > > > > > > > > UDK_PATH would depend on UDK configs like UDK_2017_VERSION etc.? > > > > > > > > > > Yes that would be my suggestion. It's used in other places (makefile) too so that's why I think it might be a good idea. > > > > > > > > sure, let me see if I can address it here or i will submit an incremental CL. sounds good to you ? > > > > > > Sure > > > > question: wondering if we define a Kconfig with type string then we might ended up with double quote (which we might need to strip before concatenate for #include)? > > > > ``` > > config UDK_2017_PATH > > string "vendorcode/intel/edk2/UDK2017" > > > > > > #include <CONFIG(UDK_PATH)/Include/Guid/FspHeaderFile.h> > > | > > v > > #include <"vendorcode/intel/edk2/UDK2017"/Include/Guid/FspHeaderFile.h> > > > > ``` > > Hmm I suppose makefile could do the quote stripping and passing it via a compiler commandline argument '-D' in CFLAGS to have it without quotes in C. yes possible for makefile. I'm wondering for #include in header file. -- To view, visit https://review.coreboot.org/c/coreboot/+/81623?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: I29e5002821843c9cffbc8f6317d1062175f014ff Gerrit-Change-Number: 81623 Gerrit-PatchSet: 5 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Appukuttan V K <appukuttan.vk(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Appukuttan V K <appukuttan.vk(a)intel.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Comment-Date: Wed, 03 Apr 2024 18:12:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: vc/intel/fsp: Refactor FSP header inclusion for EDK2 compatibility
by Arthur Heymans (Code Review) 03 Apr '24

03 Apr '24

Attention is currently required from: Appukuttan V K, Dinesh Gehlot, Kapil Porwal, Nick Vaccaro, Ronak Kanabar, Subrata Banik. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81623?usp=email ) Change subject: vc/intel/fsp: Refactor FSP header inclusion for EDK2 compatibility ...................................................................... Patch Set 5: (1 comment) File src/vendorcode/intel/fsp/fsp2_0/IntelFspPkg/Include/FspInfoHeader.h: https://review.coreboot.org/c/coreboot/+/81623/comment/e719b038_d89bfd92 : PS5, Line 7: vendorcode/intel/edk2/UDK2017/ > > > > > > This prefix is used in other places in the tree. Can we use a Kconfig param for it that depends on the UDK kconfig? That would make this file trivial and same with vendorcode/intel/Makefile.mk > > > > > > > > > > can you please help to share a little more details which I can take as an incremental change. > > > > > > > > > > I'm sensing that you are asking to introduce a kconfig for each `UDK version` to mask the `vendorcode/intel/edk2/UDK2017` path ? > > > > > > > > > > Then, this file only includes only one header like > > > > > > > > > > > > > > > ```#include <CONFIG(UDK_PATH)/Include/Guid/FspHeaderFile.h>``` > > > > > > > > > > UDK_PATH would depend on UDK configs like UDK_2017_VERSION etc.? > > > > > > > > Yes that would be my suggestion. It's used in other places (makefile) too so that's why I think it might be a good idea. > > > > > > sure, let me see if I can address it here or i will submit an incremental CL. sounds good to you ? > > > > Sure > > question: wondering if we define a Kconfig with type string then we might ended up with double quote (which we might need to strip before concatenate for #include)? > > ``` > config UDK_2017_PATH > string "vendorcode/intel/edk2/UDK2017" > > > #include <CONFIG(UDK_PATH)/Include/Guid/FspHeaderFile.h> > | > v > #include <"vendorcode/intel/edk2/UDK2017"/Include/Guid/FspHeaderFile.h> > > ``` Hmm I suppose makefile could do the quote stripping and passing it via a compiler commandline argument '-D' in CFLAGS to have it without quotes in C. -- To view, visit https://review.coreboot.org/c/coreboot/+/81623?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: I29e5002821843c9cffbc8f6317d1062175f014ff Gerrit-Change-Number: 81623 Gerrit-PatchSet: 5 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Appukuttan V K <appukuttan.vk(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Appukuttan V K <appukuttan.vk(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Comment-Date: Wed, 03 Apr 2024 18:05:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit April 2024