coreboot-gerrit March 2024

coreboot-gerrit@coreboot.org

1 participants
3668 discussions

[XS] Change in coreboot[main]: drivers/intel/fsp2_0: Perform MP init post FSP-MultiPhase SI Init
by Jérémy Compostella (Code Review) 06 Mar '24

06 Mar '24

Attention is currently required from: Andrey Petrov, Jérémy Compostella, Ronak Kanabar. Hello Andrey Petrov, Ronak Kanabar, Subrata Banik, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/80691?usp=email to look at the new patch set (#7). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: drivers/intel/fsp2_0: Perform MP init post FSP-MultiPhase SI Init ...................................................................... drivers/intel/fsp2_0: Perform MP init post FSP-MultiPhase SI Init FSP can also make use of Multi-Processor services during its multi-phase stages. If `USE_INTEL_FSP_MP_INIT' is set and `USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI' unset coreboot cannot take MP ownership as FSP-S may still use EDK2 MP services concurrently. TEST=verified on Lunar Lake RVP board (lnlrvp) Change-Id: If0397f5cc8d0f4f1872bd37a001fe42e0c37ec92 Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com> --- M src/drivers/intel/fsp2_0/silicon_init.c 1 file changed, 5 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/80691/7 -- To view, visit https://review.coreboot.org/c/coreboot/+/80691?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: If0397f5cc8d0f4f1872bd37a001fe42e0c37ec92 Gerrit-Change-Number: 80691 Gerrit-PatchSet: 7 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in coreboot[main]: drivers/intel/fsp2_0/ppi: Fix FSP PPI callback calling convention
by Jérémy Compostella (Code Review) 06 Mar '24

06 Mar '24

Jérémy Compostella has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/80574?usp=email ) Change subject: drivers/intel/fsp2_0/ppi: Fix FSP PPI callback calling convention ...................................................................... Abandoned This patch has been squashed into https://review.coreboot.org/c/coreboot/+/80277 as a result of https://review.coreboot.org/c/coreboot/+/80574/comment/3833c31a_173a00c8/ discussion. -- To view, visit https://review.coreboot.org/c/coreboot/+/80574?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: If0397f5cc8d0f4f1872bd37a001fe42e0c37ec95 Gerrit-Change-Number: 80574 Gerrit-PatchSet: 15 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Chen, Gang C <gang.c.chen(a)intel.com> Gerrit-Reviewer: Jincheng Li <jincheng.li(a)intel.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: abandon

1 0

[M] Change in coreboot[main]: drivers/intel/fsp2_0: Support FSP 2.4 64-bits
by Jérémy Compostella (Code Review) 06 Mar '24

06 Mar '24

Attention is currently required from: Andrey Petrov, Arthur Heymans, Bora Guvendik, Christian Walter, Felix Held, Fred Reitberger, Jason Glenesk, Johnny Lin, Lean Sheng Tan, Matt DeVillier, Patrick Rudolph, Paul Menzel, Ronak Kanabar, Shuo Liu, Subrata Banik, Tim Chu, Wonkyu Kim. Hello Andrey Petrov, Arthur Heymans, Bora Guvendik, Christian Walter, Felix Held, Fred Reitberger, Jason Glenesk, Johnny Lin, Lean Sheng Tan, Matt DeVillier, Patrick Rudolph, Ronak Kanabar, Shuo Liu, Tim Chu, Wonkyu Kim, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/80277?usp=email to look at the new patch set (#26). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: drivers/intel/fsp2_0: Support FSP 2.4 64-bits ...................................................................... drivers/intel/fsp2_0: Support FSP 2.4 64-bits FSP 2.4 brings FSP 64-bits support which requires some adjustments in coreboot: - Stack alignment: 1. FSP functions must be called with the stack 16-bytes aligned. This is already setup properly with the default value of the `mpreferred-stack-boundary' compiler option (4). 2. The FSP stack buffer supplied by coreboot through the `StackBase' UPD must be 16-bytes aligned. - The EDK2 EFIAPI macro definition relies on compiler flags such as __GNUC__ which is not working well when included by coreboot. While it has no side-effect on i386 because the C calling convention used by coreboot and FSP are the same, it breaks on x86_64 because FSP/UEFI uses the Microsoft x64 calling convention while coreboot uses the System V AMD64 ABI. Fortunately, EDK2 header allows to override the EFIAPI definition. This appropriate attribute has to be set to all functions calling or called by the FSP. - Add FSP 2 Multi Processor Platform Initialization module a function indirection to ensure that efi_ap_procedure functions are called with the appropriate C calling convention. - Add fsp print helper macros to print `efi_return_status_t' with the appropriate format - This commit adds a function indirection in MP PPI implementation to ensure FSP callbacks are invoked using the appropriate C calling convention. TEST=verified on Lunar Lake RVP board (lnlrvp) Change-Id: If0397f5cc8d0f4f1872bd37a001fe42e0c37ec99 Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com> --- M src/drivers/intel/fsp2_0/debug.c M src/drivers/intel/fsp2_0/fsp_debug_event.c M src/drivers/intel/fsp2_0/include/fsp/debug.h M src/drivers/intel/fsp2_0/include/fsp/fsp_debug_event.h M src/drivers/intel/fsp2_0/include/fsp/info_header.h M src/drivers/intel/fsp2_0/include/fsp/soc_binding.h M src/drivers/intel/fsp2_0/include/fsp/util.h M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/intel/fsp2_0/ppi/mp_service1.c M src/drivers/intel/fsp2_0/ppi/mp_service2.c M src/drivers/intel/fsp2_0/ppi/mp_service_ppi.c M src/drivers/intel/fsp2_0/silicon_init.c M src/drivers/intel/fsp2_0/util.c M src/include/efi/efi_datatype.h M src/soc/amd/common/fsp/fsp_reset.c M src/soc/intel/common/fsp_reset.c 16 files changed, 125 insertions(+), 68 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/80277/26 -- To view, visit https://review.coreboot.org/c/coreboot/+/80277?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: If0397f5cc8d0f4f1872bd37a001fe42e0c37ec99 Gerrit-Change-Number: 80277 Gerrit-PatchSet: 26 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Appukuttan V K <appukuttan.vk(a)intel.com> Gerrit-CC: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.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: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in coreboot[main]: drivers/intel/fsp2_0/ppi: Fix FSP PPI callback calling convention
by Jérémy Compostella (Code Review) 06 Mar '24

06 Mar '24

Attention is currently required from: Andrey Petrov, Arthur Heymans, Chen, Gang C, Jincheng Li, Ronak Kanabar, Shuo Liu, Subrata Banik. Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80574?usp=email ) Change subject: drivers/intel/fsp2_0/ppi: Fix FSP PPI callback calling convention ...................................................................... Patch Set 15: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/80574/comment/fb3477dd_1c447eaf : PS7, Line 9: Multi Processor PEIM-to-PEIM Interface deals with pointer to function : living in FSP space > > Discarding a function type by casting to (Void *) and then calling is wrong IMO. It makes the assumption that the current compiler / system calling convention complies with the one original function type. > > we had an agreement that all functions will be running over APs in coreboot context have to be with `void*` and no print statement should present. i don't expect such functions to return any return type while running on APs. In the present case it has nothing to do with the return value, the issue is related the convention to pass the parameter (which register ...) which is required here. > > > > The fact it works is because the calling convention is the same but technically we should not discard it. That's what this code is fixing. Let's say for instance you want to compile coreboot with a Microsoft compiler using the stdcall calling convention by default, that piece of code would break even on 32-bits. From C point of view the current code is not correct as it relies on knowledge compiler of specifics which turns out to be fine because most of them just use the same C calling convention. > > > > Honestly, I don't think there is any value debating this more. If you want me to move that to the 64-bits support patch then fine, I'll just to that. > > I will say lets move this into 64-bit support specific one As you wish. -- To view, visit https://review.coreboot.org/c/coreboot/+/80574?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: If0397f5cc8d0f4f1872bd37a001fe42e0c37ec95 Gerrit-Change-Number: 80574 Gerrit-PatchSet: 15 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Chen, Gang C <gang.c.chen(a)intel.com> Gerrit-Reviewer: Jincheng Li <jincheng.li(a)intel.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Chen, Gang C <gang.c.chen(a)intel.com> Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Attention: Jincheng Li <jincheng.li(a)intel.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Comment-Date: Wed, 06 Mar 2024 19:57:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: soc/intel/alderlake: Add options to set CNVi Reset and Clkreq pins
by Sean Rhodes (Code Review) 06 Mar '24

06 Mar '24

Attention is currently required from: Kapil Porwal. Sean Rhodes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81080?usp=email ) Change subject: soc/intel/alderlake: Add options to set CNVi Reset and Clkreq pins ...................................................................... Set Ready For Review -- To view, visit https://review.coreboot.org/c/coreboot/+/81080?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: Ib534d3cfe888c1c538267caaf324c8ae743da496 Gerrit-Change-Number: 81080 Gerrit-PatchSet: 2 Gerrit-Owner: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Comment-Date: Wed, 06 Mar 2024 19:55:45 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: vc/amd/opensil/genoa_poc/memmap: use GiB define
by Felix Held (Code Review) 06 Mar '24

06 Mar '24

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/81093?usp=email ) Change subject: vc/amd/opensil/genoa_poc/memmap: use GiB define ...................................................................... vc/amd/opensil/genoa_poc/memmap: use GiB define Use the GiB define to make the 4 GiB boundary used in some places in the code a bit easier to read. Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de> Change-Id: I81877a5d293c883d2e31bdb18ae3b22b8a44e62f Reviewed-on: https://review.coreboot.org/c/coreboot/+/81093 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Varshit Pandya <pandyavarshit(a)gmail.com> Reviewed-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M src/vendorcode/amd/opensil/genoa_poc/memmap.c 1 file changed, 4 insertions(+), 4 deletions(-) Approvals: Arthur Heymans: Looks good to me, approved Varshit Pandya: Looks good to me, approved build bot (Jenkins): Verified diff --git a/src/vendorcode/amd/opensil/genoa_poc/memmap.c b/src/vendorcode/amd/opensil/genoa_poc/memmap.c index 31e6f04..b1d4b93 100644 --- a/src/vendorcode/amd/opensil/genoa_poc/memmap.c +++ b/src/vendorcode/amd/opensil/genoa_poc/memmap.c @@ -115,7 +115,7 @@ reserved_ram_from_to(dev, (*idx)++, mem_usable, top_mem); // Check if we're done - if (top_of_mem <= 0x100000000) + if (top_of_mem <= 4ULL * GiB) return; // Holes in upper DRAM @@ -124,11 +124,11 @@ if (hole_info == NULL) return; uint64_t lowest_upper_hole_base = top_of_mem; - uint64_t highest_upper_hole_end = 0x100000000; + uint64_t highest_upper_hole_end = 4ULL * GiB; for (int hole = 0; hole < n_holes; hole++) { if (hole_info[hole].Type == MMIO) continue; - if (hole_info[hole].Base < 0x100000000) + if (hole_info[hole].Base < 4ULL * GiB) continue; lowest_upper_hole_base = MIN(lowest_upper_hole_base, hole_info[hole].Base); highest_upper_hole_end = MAX(highest_upper_hole_end, hole_info[hole].Base + hole_info[hole].Size); @@ -138,7 +138,7 @@ reserved_ram_range(dev, (*idx)++, hole_info[hole].Base, hole_info[hole].Size); } - ram_from_to(dev, (*idx)++, 0x100000000, lowest_upper_hole_base); + ram_from_to(dev, (*idx)++, 4ULL * GiB, lowest_upper_hole_base); // Do we need this? if (top_of_mem > highest_upper_hole_end) -- To view, visit https://review.coreboot.org/c/coreboot/+/81093?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: I81877a5d293c883d2e31bdb18ae3b22b8a44e62f Gerrit-Change-Number: 81093 Gerrit-PatchSet: 2 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Varshit Pandya <pandyavarshit(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[XS] Change in coreboot[main]: vc/amd/opensil/genoa_poc/memmap: use get_top_of_mem_below_4gb
by Felix Held (Code Review) 06 Mar '24

06 Mar '24

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/81092?usp=email ) Change subject: vc/amd/opensil/genoa_poc/memmap: use get_top_of_mem_below_4gb ...................................................................... vc/amd/opensil/genoa_poc/memmap: use get_top_of_mem_below_4gb Use get_top_of_mem_below_4gb instead of open-coding the functionality. Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de> Change-Id: I5885e9ad89ed9f0aa657c56804e98c352267267f Reviewed-on: https://review.coreboot.org/c/coreboot/+/81092 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Varshit Pandya <pandyavarshit(a)gmail.com> --- M src/vendorcode/amd/opensil/genoa_poc/memmap.c 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Varshit Pandya: Looks good to me, approved diff --git a/src/vendorcode/amd/opensil/genoa_poc/memmap.c b/src/vendorcode/amd/opensil/genoa_poc/memmap.c index 39b27ec..31e6f04 100644 --- a/src/vendorcode/amd/opensil/genoa_poc/memmap.c +++ b/src/vendorcode/amd/opensil/genoa_poc/memmap.c @@ -110,7 +110,7 @@ mem_usable); // Account for UMA and TSEG - const uint32_t top_mem = ALIGN_DOWN(rdmsr(TOP_MEM).lo, 1 * MiB); + const uint32_t top_mem = ALIGN_DOWN(get_top_of_mem_below_4gb(), 1 * MiB); if (mem_usable != top_mem) reserved_ram_from_to(dev, (*idx)++, mem_usable, top_mem); -- To view, visit https://review.coreboot.org/c/coreboot/+/81092?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: I5885e9ad89ed9f0aa657c56804e98c352267267f Gerrit-Change-Number: 81092 Gerrit-PatchSet: 2 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Varshit Pandya <pandyavarshit(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[S] Change in coreboot[main]: drivers/intel/fsp2_0/ppi: Fix FSP PPI callback calling convention
by Subrata Banik (Code Review) 06 Mar '24

06 Mar '24

Attention is currently required from: Andrey Petrov, Arthur Heymans, Chen, Gang C, Jincheng Li, Jérémy Compostella, Ronak Kanabar, Shuo Liu. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80574?usp=email ) Change subject: drivers/intel/fsp2_0/ppi: Fix FSP PPI callback calling convention ...................................................................... Patch Set 15: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/80574/comment/422c9fd5_542c5383 : PS7, Line 9: Multi Processor PEIM-to-PEIM Interface deals with pointer to function : living in FSP space > Discarding a function type by casting to (Void *) and then calling is wrong IMO. It makes the assumption that the current compiler / system calling convention complies with the one original function type. we had an agreement that all functions will be running over APs in coreboot context have to be with `void*` and no print statement should present. i don't expect such functions to return any return type while running on APs. > > The fact it works is because the calling convention is the same but technically we should not discard it. That's what this code is fixing. Let's say for instance you want to compile coreboot with a Microsoft compiler using the stdcall calling convention by default, that piece of code would break even on 32-bits. From C point of view the current code is not correct as it relies on knowledge compiler of specifics which turns out to be fine because most of them just use the same C calling convention. > > Honestly, I don't think there is any value debating this more. If you want me to move that to the 64-bits support patch then fine, I'll just to that. I will say lets move this into 64-bit support specific one -- To view, visit https://review.coreboot.org/c/coreboot/+/80574?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: If0397f5cc8d0f4f1872bd37a001fe42e0c37ec95 Gerrit-Change-Number: 80574 Gerrit-PatchSet: 15 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Chen, Gang C <gang.c.chen(a)intel.com> Gerrit-Reviewer: Jincheng Li <jincheng.li(a)intel.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Chen, Gang C <gang.c.chen(a)intel.com> Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Attention: Jincheng Li <jincheng.li(a)intel.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Comment-Date: Wed, 06 Mar 2024 19:23:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: drivers/intel/fsp2_0: Add limited to 32-bits FSP 2.4 support
by Subrata Banik (Code Review) 06 Mar '24

06 Mar '24

Attention is currently required from: Andrey Petrov, Bora Guvendik, Chen, Gang C, Dinesh Gehlot, Eran Mitrani, Jakub Czapiga, Jérémy Compostella, Kapil Porwal, Karthik Ramasubramanian, Martin L Roth, Nick Vaccaro, Ronak Kanabar, Shuo Liu, Tarun, Wonkyu Kim. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80275?usp=email ) The change is no longer submittable: All-Comments-Resolved is unsatisfied now. Change subject: drivers/intel/fsp2_0: Add limited to 32-bits FSP 2.4 support ...................................................................... Patch Set 24: (1 comment) Patchset: PS24: please allow one more day for other to review the code, otherwise i will mark this comment done and land this cl -- To view, visit https://review.coreboot.org/c/coreboot/+/80275?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: I1c24d26e105c3dcbd9cca0e7197ab1362344aa97 Gerrit-Change-Number: 80275 Gerrit-PatchSet: 24 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Chen, Gang C <gang.c.chen(a)intel.com> Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Eran Mitrani <mitrani(a)google.com> Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun <tstuli(a)gmail.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Appukuttan V K <appukuttan.vk(a)intel.com> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Eran Mitrani <mitrani(a)google.com> Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com> Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Attention: Tarun <tstuli(a)gmail.com> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Chen, Gang C <gang.c.chen(a)intel.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Wed, 06 Mar 2024 19:21:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[L] Change in coreboot[main]: treewide: Change I2C/SMBUS master/slave to controller/target
by Werner Zeh (Code Review) 06 Mar '24

06 Mar '24

Attention is currently required from: Arthur Heymans, Caveh Jalali, Cliff Huang, Felix Singer, Julius Werner, Martin L Roth, Yu-Ping Wu. Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/77098?usp=email ) Change subject: treewide: Change I2C/SMBUS master/slave to controller/target ...................................................................... Patch Set 5: (15 comments) File src/drivers/intel/gma/edid.c: https://review.coreboot.org/c/coreboot/+/77098/comment/2270457b_f1adea6d : PS5, Line 72: SLAVE TARGET https://review.coreboot.org/c/coreboot/+/77098/comment/8e41a6d8_f618e8ef : PS5, Line 86: SLAVE TARGET File src/soc/intel/baytrail/scc.c: https://review.coreboot.org/c/coreboot/+/77098/comment/3587a134_5ee62fd9 : PS5, Line 63: /* Override Target Path - populate DLL settings. */ This one looks not related to I2C. File src/soc/intel/common/block/smbus/smbus.c: https://review.coreboot.org/c/coreboot/+/77098/comment/2793f03e_54023b36 : PS5, Line 25: SLAVE TARGET File src/soc/nvidia/tegra/i2c.c: https://review.coreboot.org/c/coreboot/+/77098/comment/12310e5a_fd15b735 : PS5, Line 148: SLAVE TARGET File src/soc/qualcomm/ipq40xx/i2c.c: https://review.coreboot.org/c/coreboot/+/77098/comment/ef1dba5b_60b83e01 : PS5, Line 35: QUP_MINICORE_I2C_MASTER Looks like you would like to keep MASTER here? File src/soc/samsung/exynos5250/i2c.c: https://review.coreboot.org/c/coreboot/+/77098/comment/33ad9953_f908a446 : PS5, Line 271: SLAVE What about this? File src/soc/samsung/exynos5420/i2c.c: https://review.coreboot.org/c/coreboot/+/77098/comment/90409896_27cf7e7a : PS5, Line 145: Master Controller? There is a bunch of other hits in this file for this one. https://review.coreboot.org/c/coreboot/+/77098/comment/35ff6684_1e6cf4f4 : PS5, Line 344: Master Controller https://review.coreboot.org/c/coreboot/+/77098/comment/61d1fb32_2c2b2ce1 : PS5, Line 433: Master Should this be Controller instead? https://review.coreboot.org/c/coreboot/+/77098/comment/9ee2083c_c8ea9db5 : PS5, Line 435: SLV This can still mean SALVE. https://review.coreboot.org/c/coreboot/+/77098/comment/8ec1eb83_e08afa9b : PS5, Line 619: Master Here, too. File src/southbridge/intel/bd82x6x/smbus.c: https://review.coreboot.org/c/coreboot/+/77098/comment/feacae06_3d7dc662 : PS5, Line 23: SLAVE TARGET File src/southbridge/intel/ibexpeak/smbus.c: https://review.coreboot.org/c/coreboot/+/77098/comment/a6bd01cb_2e9a64fe : PS5, Line 23: SLAVE TARGET File src/southbridge/intel/lynxpoint/smbus.c: https://review.coreboot.org/c/coreboot/+/77098/comment/8a7613f0_f178b6b6 : PS5, Line 23: SLAVE TARGET -- To view, visit https://review.coreboot.org/c/coreboot/+/77098?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: I6c141fa4322de9e9ed208589f591560fed609825 Gerrit-Change-Number: 77098 Gerrit-PatchSet: 5 Gerrit-Owner: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: Forest Mittelberg <bmbm(a)google.com> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Jeff Daly <jeffd(a)silicom-usa.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: Vanessa Eusebio <vanessa.f.eusebio(a)intel.com> Gerrit-Reviewer: Xi Chen <xixi.chen(a)mediatek.com> Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Comment-Date: Wed, 06 Mar 2024 19:21:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit March 2024