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
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
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
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
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
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
Attention is currently required from: Felix Singer, Martin L Roth, Paul Menzel, Angel Pons, Elyes Haouas.
David Hendricks 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.
--
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: 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: 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 07:06:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Tarun Tuli, Kapil Porwal, Ivy Jian, Eric Lai, Sukumar Ghorai.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73332 )
Change subject: soc/intel: Update API name `pmc_send_bios_reset_pci_enum_done`
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> This patch is working as expected - soc pcie bar address is loaded.
> however ioe-pcie-bar address is not loaded. In debug with SOC team (will comeback).
> Will hold this patch for couple of days.
Sure
--
To view, visit https://review.coreboot.org/c/coreboot/+/73332
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I1cf8cb1ecadeb68c109be6b0e751a3f2c448ae4f
Gerrit-Change-Number: 73332
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
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: Sukumar Ghorai <sukumar.ghorai(a)intel.com>
Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Ivy Jian <ivy.jian(a)quanta.corp-partner.google.com>
Gerrit-Attention: Eric Lai <eric_lai(a)quanta.corp-partner.google.com>
Gerrit-Attention: Sukumar Ghorai <sukumar.ghorai(a)intel.com>
Gerrit-Comment-Date: Wed, 01 Mar 2023 07:04:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sukumar Ghorai <sukumar.ghorai(a)intel.com>
Gerrit-MessageType: comment
Attention is currently required from: Lance Zhao, Bora Guvendik, Hannah Williams, Jakub Czapiga, Jérémy Compostella, Paul Menzel, Tim Wawrzynczak, Nick Vaccaro.
Cliff Huang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73113 )
Change subject: acpi: Add function to print debug string directly from formatted string
......................................................................
Patch Set 4:
(3 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/73113/comment/148231e6_2afc2206
PS3, Line 27: TEST=Add above functions in the acpigen code and check the generated SSDT
> Possible unwrapped commit description (prefer a maximum 72 chars per line)
Please fix.
File src/include/acpi/acpigen.h:
https://review.coreboot.org/c/coreboot/+/73113/comment/a3fdca3e_4e6600d1
PS3, Line 502: void acpigen_write_debug_sprintf(const char *fmt, ...) __attribute__((format(printf, 1, 2)));
__prtintf() not available.
File src/include/acpi/acpigen.h:
https://review.coreboot.org/c/coreboot/+/73113/comment/e2a963bd_0a3e221b
PS2, Line 508: acpigen_write_debug
> With acpigen_write_debug_sprintf(), the following functions won't be needed […]
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/73113
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id4a42e5854516a22b7bc4559c2ed08680722c5ba
Gerrit-Change-Number: 73113
Gerrit-PatchSet: 4
Gerrit-Owner: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Jakub Czapiga <jacz(a)semihalf.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Lance Zhao <lance.zhao(a)gmail.com>
Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Attention: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-Attention: Jakub Czapiga <jacz(a)semihalf.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Comment-Date: Wed, 01 Mar 2023 06:12:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Lance Zhao <lance.zhao(a)gmail.com>
Comment-In-Reply-To: Cliff Huang <cliff.huang(a)intel.com>
Comment-In-Reply-To: Jakub Czapiga <jacz(a)semihalf.com>
Gerrit-MessageType: comment