coreboot-gerrit March 2023

coreboot-gerrit@coreboot.org

1 participants
4880 discussions

[M] Change in coreboot[master]: clang-format: Update 'clang-format' file
by Elyes Haouas (Code Review) 01 Mar '23

01 Mar '23

Attention is currently required from: Felix Singer, Martin L Roth, David Hendricks, Paul Menzel, Angel Pons, Arthur Heymans. Elyes Haouas has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/64779 ) Change subject: clang-format: Update 'clang-format' file ...................................................................... Patch Set 20: (1 comment) Patchset: PS20: > > FYI, one thing clang-format seems to handle poorly is consecutive macros that have indented bit de […] maybe : "AlignConsecutiveMacros: Consecutive" ? -- To view, visit https://review.coreboot.org/c/coreboot/+/64779 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibec6b6d47c58c393dc28f91cbf109f3e87f935bf Gerrit-Change-Number: 64779 Gerrit-PatchSet: 20 Gerrit-Owner: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.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: David Hendricks <david.hendricks(a)gmail.com> Gerrit-CC: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: David Hendricks <david.hendricks(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-Comment-Date: Wed, 01 Mar 2023 10:52:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: mb/siemens/mc_ehl2: Fix GPIO settings
by Mario Scheithauer (Code Review) 01 Mar '23

01 Mar '23

Mario Scheithauer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/73341 ) Change subject: mb/siemens/mc_ehl2: Fix GPIO settings ...................................................................... mb/siemens/mc_ehl2: Fix GPIO settings With the latest hardware revision, the two GPIOs GPP_B15 and GPP_E19 are no longer connected to a native function. BUG=none TEST=Checked output verbose GPIO debug messages Change-Id: I266612f041b749aa83b366497b4211fc075c7bd7 Signed-off-by: Mario Scheithauer <mario.scheithauer(a)siemens.com> --- M src/mainboard/siemens/mc_ehl/variants/mc_ehl2/gpio.c 1 file changed, 18 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/73341/1 diff --git a/src/mainboard/siemens/mc_ehl/variants/mc_ehl2/gpio.c b/src/mainboard/siemens/mc_ehl/variants/mc_ehl2/gpio.c index b83fabc1..9115de7 100644 --- a/src/mainboard/siemens/mc_ehl/variants/mc_ehl2/gpio.c +++ b/src/mainboard/siemens/mc_ehl/variants/mc_ehl2/gpio.c @@ -14,7 +14,7 @@ PAD_NC(GPP_B10, NONE), /* Not connected */ PAD_CFG_NF(GPP_B11, NONE, PLTRST, NF1), /* PMC_ALERT_N */ PAD_NC(GPP_B14, NONE), /* Not connected */ - PAD_CFG_NF(GPP_B15, NONE, PLTRST, NF5), /* ESPI_CS1_N */ + PAD_NC(GPP_B15, NONE), /* Not connected */ PAD_NC(GPP_B18, NONE), /* Not connected */ PAD_NC(GPP_B19, NONE), /* Not connected */ PAD_NC(GPP_B23, NONE), /* Not connected */ @@ -134,7 +134,7 @@ PAD_NC(GPP_E15, NONE), /* Not connected */ PAD_NC(GPP_E16, NONE), /* Not connected */ PAD_NC(GPP_E18, NONE), /* Not connected */ - PAD_CFG_NF(GPP_E19, NONE, DEEP, NF1), /* DDI0_DDC_SCL */ + PAD_NC(GPP_E19, NONE), /* Not connected */ PAD_NC(GPP_E23, NONE), /* Not connected */ /* Community 5 - GpioGroup GPP_R */ -- To view, visit https://review.coreboot.org/c/coreboot/+/73341 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I266612f041b749aa83b366497b4211fc075c7bd7 Gerrit-Change-Number: 73341 Gerrit-PatchSet: 1 Gerrit-Owner: Mario Scheithauer <mario.scheithauer(a)siemens.com> Gerrit-MessageType: newchange

1 0

[M] Change in coreboot[master]: device/pciexp_device.c: Do not enable common clock if already active
by Subrata Banik (Code Review) 01 Mar '23

01 Mar '23

1 0

[M] Change in coreboot[master]: device/pciexp_device.c: Do not enable common clock if already active
by Werner Zeh (Code Review) 01 Mar '23

01 Mar '23

Attention is currently required from: Subrata Banik, Paul Menzel. Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73310 ) Change subject: device/pciexp_device.c: Do not enable common clock if already active ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/73310/comment/a9a6aaf6_d6707343 PS1, Line 33: and thus execution time > Depends how fast the link re-training is finished, which in turn depends on the root and endpoint de […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/73310 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I747fa406a120a215de189d7252f160c8ea2e3716 Gerrit-Change-Number: 73310 Gerrit-PatchSet: 2 Gerrit-Owner: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Wed, 01 Mar 2023 09:54:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Comment-In-Reply-To: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: device/pciexp_device.c: Do not enable common clock if already active
by Werner Zeh (Code Review) 01 Mar '23

01 Mar '23

Attention is currently required from: Subrata Banik, Paul Menzel. Hello build bot (Jenkins), Subrata Banik, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/73310 to look at the new patch set (#2). Change subject: device/pciexp_device.c: Do not enable common clock if already active ...................................................................... device/pciexp_device.c: Do not enable common clock if already active The Common Clock Configuration (CCC) is a PCIe feature for cases where the upstream and downstream device of a link share the same reference clock. After a change in this setting a link re-training is mandatory to make it effective. On recent Intel platforms (tested on Elkhart Lake) the FSP code which is executed before coreboot performs the PCI scan already enumerates all PCI buses for its internal uses. While this is done, all the PCI express features of a link are configured, which includes CCC. If the link supports common clock, FSP performs the link re-training already. When the execution flow is returned to coreboot, the same link treatment is applied again (coded in 'pciexp_tune_dev()') and CCC is enabled a second time, just a few milliseconds after FSP did this already. Because enabling CCC requires a link re-training, there are two link re-trainings on the PCIe link within a few milliseconds (one from the FSP code and one from coreboot) which can lead to issues with a connected PCIe device on this link. In particular, link issues were discovered with a Pericom PCIe switch (PI7C9X2G608) on mc_ehl1 where the link has stalled for a while after the second re-training. This in turn leads to non-initialized PCI devices on the bus after coreboot has finished. This patch checks if CCC is already enabled on a link and does not perform the steps to enable it again in coreboot which safes a link re-training (and thus execution time) and a potential link stability issue. Test=Check log output on mc_ehl1 which shows the following lines: [DEBUG] PCI: pci_scan_bus for bus 09 [DEBUG] PCI: 09:00.0 [8086/1533] enabled [INFO ] PCIe: Common Clock Configuration already enabled Change-Id: I747fa406a120a215de189d7252f160c8ea2e3716 Signed-off-by: Werner Zeh <werner.zeh(a)siemens.com> --- M src/device/pciexp_device.c 1 file changed, 61 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/73310/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/73310 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I747fa406a120a215de189d7252f160c8ea2e3716 Gerrit-Change-Number: 73310 Gerrit-PatchSet: 2 Gerrit-Owner: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: newpatchset

1 0

[S] Change in coreboot[master]: device/pciexp_device.c: Do not enable common clock if already active
by Werner Zeh (Code Review) 01 Mar '23

01 Mar '23

Attention is currently required from: Subrata Banik, Paul Menzel. Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73310 ) Change subject: device/pciexp_device.c: Do not enable common clock if already active ...................................................................... Patch Set 1: (1 comment) File src/device/pciexp_device.c: https://review.coreboot.org/c/coreboot/+/73310/comment/d5770e1a_aa42c1c0 PS1, Line 205: printk(BIOS_INFO, "Common Clock Configuration already enabled\n"); > PCIe: Common Clock Configuration already enabled, so skip it Paul, this can sound confusing as "so skip it" actually refers to skip the CCC enablement. I would rather prefer a the used phrasing as it is a clear statement and implies that the feature will stay enabled. -- To view, visit https://review.coreboot.org/c/coreboot/+/73310 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I747fa406a120a215de189d7252f160c8ea2e3716 Gerrit-Change-Number: 73310 Gerrit-PatchSet: 1 Gerrit-Owner: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Wed, 01 Mar 2023 09:34:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Comment-In-Reply-To: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: device/pciexp_device.c: Do not enable common clock if already active
by Werner Zeh (Code Review) 01 Mar '23

01 Mar '23

Attention is currently required from: Subrata Banik, Paul Menzel. Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73310 ) Change subject: device/pciexp_device.c: Do not enable common clock if already active ...................................................................... Patch Set 1: (1 comment) File src/device/pciexp_device.c: https://review.coreboot.org/c/coreboot/+/73310/comment/0bf6b758_7f5bed42 PS1, Line 205: printk(BIOS_INFO, "Common Clock Configuration already enabled\n"); > > Hey Subrata, any response to my question from your side? […] OK, will update the patch. -- To view, visit https://review.coreboot.org/c/coreboot/+/73310 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I747fa406a120a215de189d7252f160c8ea2e3716 Gerrit-Change-Number: 73310 Gerrit-PatchSet: 1 Gerrit-Owner: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Wed, 01 Mar 2023 09:28:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Comment-In-Reply-To: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: device/pciexp_device.c: Do not enable common clock if already active
by Subrata Banik (Code Review) 01 Mar '23

01 Mar '23

Attention is currently required from: Paul Menzel, Werner Zeh. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73310 ) Change subject: device/pciexp_device.c: Do not enable common clock if already active ...................................................................... Patch Set 1: (1 comment) File src/device/pciexp_device.c: https://review.coreboot.org/c/coreboot/+/73310/comment/9aae5065_f4c96763 PS1, Line 205: printk(BIOS_INFO, "Common Clock Configuration already enabled\n"); > Hey Subrata, any response to my question from your side? Yeah, something like below code ? WDYT? ``` static bool is_ccc_enabled(struct device *root, unsigned int root_cap, struct device *endp, unsigned int endp_cap) { uint16_t root_ccc, endp_ccc; /* No need to enable common clock if it is already active. */ root_ccc = pci_read_config16(root, root_cap + PCI_EXP_LNKCTL) & PCI_EXP_LNKCTL_CCC; endp_ccc = pci_read_config16(endp, endp_cap + PCI_EXP_LNKCTL) & PCI_EXP_LNKCTL_CCC; if (root_ccc && endp_ccc) { printk(BIOS_INFO, "Common Clock Configuration already enabled\n"); return true; } return false; } ``` ``` static void pciexp_enable_common_clock(struct device *root, unsigned int root_cap, struct device *endp, unsigned int endp_cap) { if (is_ccc_enabled(....)) return; .... } ``` -- To view, visit https://review.coreboot.org/c/coreboot/+/73310 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I747fa406a120a215de189d7252f160c8ea2e3716 Gerrit-Change-Number: 73310 Gerrit-PatchSet: 1 Gerrit-Owner: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Comment-Date: Wed, 01 Mar 2023 09:08:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Comment-In-Reply-To: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: clang-format: Update 'clang-format' file
by Arthur Heymans (Code Review) 01 Mar '23

01 Mar '23

Attention is currently required from: Felix Singer, Martin L Roth, David Hendricks, Paul Menzel, Angel Pons, Elyes Haouas. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/64779 ) Change subject: clang-format: Update 'clang-format' file ...................................................................... Patch Set 20: (1 comment) Patchset: PS20: > FYI, one thing clang-format seems to handle poorly is consecutive macros that have indented bit definitions. For example, after running `clang-format-15 -i -style=file` with this patch applied, I see things like: > -#define MSR_FEATURE_CONFIG 0x13c > -#define FEATURE_CONFIG_LOCK BIT(0) > +#define MSR_FEATURE_CONFIG 0x13c > +#define FEATURE_CONFIG_LOCK BIT(0) > > In this case the indentation before FEATURE_CONFIG_LOCK makes it more obvious that it's a field within MSR_FEATURE_CONFIG. I'm not sure if there's a way to make clang-format follow that convention, but thought it's worth mentioning. Maybe not aligning macros is a good idea here? -- To view, visit https://review.coreboot.org/c/coreboot/+/64779 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibec6b6d47c58c393dc28f91cbf109f3e87f935bf Gerrit-Change-Number: 64779 Gerrit-PatchSet: 20 Gerrit-Owner: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.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: David Hendricks <david.hendricks(a)gmail.com> Gerrit-CC: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Comment-Date: Wed, 01 Mar 2023 08:36:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: device/pciexp_device.c: Do not enable common clock if already active
by Werner Zeh (Code Review) 01 Mar '23

01 Mar '23

Attention is currently required from: Subrata Banik, Paul Menzel. Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73310 ) Change subject: device/pciexp_device.c: Do not enable common clock if already active ...................................................................... Patch Set 1: (1 comment) File src/device/pciexp_device.c: https://review.coreboot.org/c/coreboot/+/73310/comment/2b150ac6_4dc11ed5 PS1, Line 205: printk(BIOS_INFO, "Common Clock Configuration already enabled\n"); > You mean move the CCC detection into a helper function? Since the CCC enablement is a helper functio […] Hey Subrata, any response to my question from your side? -- To view, visit https://review.coreboot.org/c/coreboot/+/73310 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I747fa406a120a215de189d7252f160c8ea2e3716 Gerrit-Change-Number: 73310 Gerrit-PatchSet: 1 Gerrit-Owner: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Wed, 01 Mar 2023 07:30:33 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Comment-In-Reply-To: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit March 2023