Attention is currently required from: Arthur Heymans, Christian Walter, Johnny Lin, Nico Huber, Patrick Rudolph, Shuo Liu, Tim Chu.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80093?usp=email )
Change subject: soc/intel/xeon_sp: Locate PCU by PCI device ID
......................................................................
Patch Set 12: Code-Review+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/80093?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: I06694715cba76b101165f1cef66d161b0f896b26
Gerrit-Change-Number: 80093
Gerrit-PatchSet: 12
Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.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: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Comment-Date: Mon, 12 Feb 2024 14:11:46 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/80413?usp=email )
(
2 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
)Change subject: soc/amd/picasso: Use pcie_gpp_dxio_update_clk_req_config
......................................................................
soc/amd/picasso: Use pcie_gpp_dxio_update_clk_req_config
This function turns off gpp_clk for the devices which are disabled, and
adds the code to fix up the clock configuration depending on dxio
descriptors. Also this brings picasso in line with cezanne, mendocino
and phoenix. This also prepares picasso to use the common function
gpp_clk_setup_common.
Change-Id: Ice2e3a5a78359da9a438434c7d4aa1eca878d396
Signed-off-by: Varshit Pandya <pandyavarshit(a)gmail.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/80413
Reviewed-by: Felix Held <felix-coreboot(a)felixheld.de>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
---
M src/soc/amd/picasso/Kconfig
M src/soc/amd/picasso/chip.h
M src/soc/amd/picasso/fch.c
3 files changed, 7 insertions(+), 6 deletions(-)
Approvals:
Matt DeVillier: Looks good to me, approved
build bot (Jenkins): Verified
Felix Held: Looks good to me, approved
diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig
index 3768cfb..5266e8a 100644
--- a/src/soc/amd/picasso/Kconfig
+++ b/src/soc/amd/picasso/Kconfig
@@ -66,6 +66,7 @@
select SOC_AMD_COMMON_BLOCK_UART
select SOC_AMD_COMMON_BLOCK_UCODE
select SOC_AMD_COMMON_FSP_DMI_TABLES
+ select SOC_AMD_COMMON_FSP_PCIE_CLK_REQ
select SOC_AMD_SUPPORTS_WARM_RESET
select SSE2
select UDK_2017_BINDING
diff --git a/src/soc/amd/picasso/chip.h b/src/soc/amd/picasso/chip.h
index 0b65960..d0974a5 100644
--- a/src/soc/amd/picasso/chip.h
+++ b/src/soc/amd/picasso/chip.h
@@ -4,6 +4,7 @@
#define __PICASSO_CHIP_H__
#include <amdblocks/chip.h>
+#include <amdblocks/pci_clk_req.h>
#include <commonlib/helpers.h>
#include <drivers/i2c/designware/dw_i2c.h>
#include <gpio.h>
@@ -258,11 +259,7 @@
/* The array index is the general purpose PCIe clock output number. Values in here
aren't the values written to the register to have the default to be always on. */
- enum {
- GPP_CLK_ON, /* GPP clock always on; default */
- GPP_CLK_REQ, /* GPP clock controlled by corresponding #CLK_REQx pin */
- GPP_CLK_OFF, /* GPP clk off */
- } gpp_clk_config[GPP_CLK_OUTPUT_COUNT];
+ enum gpp_clk_req gpp_clk_config[GPP_CLK_OUTPUT_COUNT];
/* performance policy for the PCIe links: power consumption vs. link speed */
enum {
diff --git a/src/soc/amd/picasso/fch.c b/src/soc/amd/picasso/fch.c
index bfe65a2..efa008d 100644
--- a/src/soc/amd/picasso/fch.c
+++ b/src/soc/amd/picasso/fch.c
@@ -7,6 +7,7 @@
#include <device/pci.h>
#include <device/pci_ops.h>
#include <amdblocks/amd_pci_util.h>
+#include <amdblocks/pci_clk_req.h>
#include <amdblocks/reset.h>
#include <amdblocks/acpimmio.h>
#include <amdblocks/acpi.h>
@@ -174,7 +175,7 @@
/* configure the general purpose PCIe clock outputs according to the devicetree settings */
static void gpp_clk_setup(void)
{
- const struct soc_amd_picasso_config *cfg = config_of_soc();
+ struct soc_amd_picasso_config *cfg = config_of_soc();
/* look-up table to be able to iterate over the PCIe clock output settings */
const uint8_t gpp_clk_shift_lut[GPP_CLK_OUTPUT_COUNT] = {
@@ -189,6 +190,8 @@
uint32_t gpp_clk_ctl = misc_read32(GPP_CLK_CNTRL);
+ pcie_gpp_dxio_update_clk_req_config(&cfg->gpp_clk_config[0],
+ ARRAY_SIZE(cfg->gpp_clk_config));
for (int i = 0; i < GPP_CLK_OUTPUT_COUNT; i++) {
gpp_clk_ctl &= ~GPP_CLK_REQ_MASK(gpp_clk_shift_lut[i]);
/*
--
To view, visit https://review.coreboot.org/c/coreboot/+/80413?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: Ice2e3a5a78359da9a438434c7d4aa1eca878d396
Gerrit-Change-Number: 80413
Gerrit-PatchSet: 4
Gerrit-Owner: Varshit Pandya <pandyavarshit(a)gmail.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: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: merged
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/80412?usp=email )
(
2 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
)Change subject: vc/amd/fsp/picasso: Bring picasso inline with other AMD SoC
......................................................................
vc/amd/fsp/picasso: Bring picasso inline with other AMD SoC
In preparation to using gpp_clk_setup_common for picasso, bring enum
defined in picasso more in line with other AMD SoC.
Change-Id: I9753acdff15921c84516ec873c925f36afdd2aa3
Signed-off-by: Varshit Pandya <pandyavarshit(a)gmail.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/80412
Reviewed-by: Fred Reitberger <reitbergerfred(a)gmail.com>
Reviewed-by: Felix Held <felix-coreboot(a)felixheld.de>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
---
M src/vendorcode/amd/fsp/picasso/platform_descriptors.h
1 file changed, 3 insertions(+), 2 deletions(-)
Approvals:
build bot (Jenkins): Verified
Matt DeVillier: Looks good to me, approved
Fred Reitberger: Looks good to me, but someone else must approve
Felix Held: Looks good to me, approved
diff --git a/src/vendorcode/amd/fsp/picasso/platform_descriptors.h b/src/vendorcode/amd/fsp/picasso/platform_descriptors.h
index 0b0de10..184db27 100644
--- a/src/vendorcode/amd/fsp/picasso/platform_descriptors.h
+++ b/src/vendorcode/amd/fsp/picasso/platform_descriptors.h
@@ -44,7 +44,7 @@
} dxio_sata_channel_type;
/* CLKREQ for PCIe type descriptors */
-typedef enum {
+enum cpm_clk_req {
CLK_DISABLE = 0x00,
CLK_REQ0,
CLK_REQ1,
@@ -56,7 +56,8 @@
CLK_REQ7,
CLK_REQ8,
CLK_REQGFX = 0x0c,
-} cpm_clk_req;
+ CLK_ENABLE = 0xff,
+};
/* PCIe link ASPM initialization */
typedef enum {
--
To view, visit https://review.coreboot.org/c/coreboot/+/80412?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: I9753acdff15921c84516ec873c925f36afdd2aa3
Gerrit-Change-Number: 80412
Gerrit-PatchSet: 4
Gerrit-Owner: Varshit Pandya <pandyavarshit(a)gmail.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: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: merged
Felix Singer has submitted this change. ( https://review.coreboot.org/c/coreboot/+/80437?usp=email )
Change subject: ec/lenovo/h8/acpi: Support pulsing LEDLOGO on Haswell ThinkPads
......................................................................
ec/lenovo/h8/acpi: Support pulsing LEDLOGO on Haswell ThinkPads
The name LEDLOGO comes from schematics. It's the red indicator, embedded
in the dot of the 'i' of the ThinkPad logo on laptop's lid.
In vendor firmware, this led starts fading in-and-out, or, in other
words, pulsing, when laptop is put to S3. It helps to determine whether
the laptop is in S3 just by taking a look at the logo.
As of now, coreboot doesn't do anything with this particular indicator,
it's always in enabled (on) state, which is not very convenient.
This patch fixes it.
Tested on T440p.
Change-Id: I85fb69c8c1bed8635a1b31e9b8385c7036bb46dd
Signed-off-by: Evgeny Zinoviev <me(a)ch1p.io>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/80437
Reviewed-by: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Reviewed-by: Nicholas Chin <nic.c3.14(a)gmail.com>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
---
M src/ec/lenovo/h8/Kconfig
M src/ec/lenovo/h8/acpi/systemstatus.asl
M src/mainboard/lenovo/haswell/Kconfig
3 files changed, 21 insertions(+), 0 deletions(-)
Approvals:
build bot (Jenkins): Verified
Felix Singer: Looks good to me, approved
Nicholas Chin: Looks good to me, approved
diff --git a/src/ec/lenovo/h8/Kconfig b/src/ec/lenovo/h8/Kconfig
index 57d7da6..6311d54 100644
--- a/src/ec/lenovo/h8/Kconfig
+++ b/src/ec/lenovo/h8/Kconfig
@@ -52,6 +52,10 @@
bool
default n
+config H8_HAS_LEDLOGO
+ bool
+ default n
+
config THINKPADEC_HKEY_EISAID
string
default "IBM0068"
diff --git a/src/ec/lenovo/h8/acpi/systemstatus.asl b/src/ec/lenovo/h8/acpi/systemstatus.asl
index 7598154..8054449 100644
--- a/src/ec/lenovo/h8/acpi/systemstatus.asl
+++ b/src/ec/lenovo/h8/acpi/systemstatus.asl
@@ -12,6 +12,10 @@
\_SB.PCI0.LPCB.EC.TLED(0x00)
/* suspend TLED off */
\_SB.PCI0.LPCB.EC.TLED(0x07)
+#if CONFIG(H8_HAS_LEDLOGO)
+ /* logo TLED off */
+ \_SB.PCI0.LPCB.EC.TLED(0x0a)
+#endif
}
If (Arg0 == 1) {
@@ -21,6 +25,10 @@
\_SB.PCI0.LPCB.EC.TLED(0x80)
/* suspend TLED off */
\_SB.PCI0.LPCB.EC.TLED(0x07)
+#if CONFIG(H8_HAS_LEDLOGO)
+ /* logo TLED on */
+ \_SB.PCI0.LPCB.EC.TLED(0x8a)
+#endif
}
If (Arg0 == 2) {
@@ -30,6 +38,10 @@
\_SB.PCI0.LPCB.EC.TLED(0x80)
/* suspend LED blinking */
\_SB.PCI0.LPCB.EC.TLED(0xc7)
+#if CONFIG(H8_HAS_LEDLOGO)
+ /* logo TLED on */
+ \_SB.PCI0.LPCB.EC.TLED(0x8a)
+#endif
}
If (Arg0 == 3) {
@@ -39,6 +51,10 @@
\_SB.PCI0.LPCB.EC.TLED(0xa0)
/* suspend TLED on */
\_SB.PCI0.LPCB.EC.TLED(0x87)
+#if CONFIG(H8_HAS_LEDLOGO)
+ /* logo TLED pulsing */
+ \_SB.PCI0.LPCB.EC.TLED(0xaa)
+#endif
}
}
}
diff --git a/src/mainboard/lenovo/haswell/Kconfig b/src/mainboard/lenovo/haswell/Kconfig
index 486baf0..6e39556 100644
--- a/src/mainboard/lenovo/haswell/Kconfig
+++ b/src/mainboard/lenovo/haswell/Kconfig
@@ -4,6 +4,7 @@
select EC_LENOVO_H8
select EC_LENOVO_PMH7
select H8_HAS_BAT_THRESHOLDS_IMPL
+ select H8_HAS_LEDLOGO
select H8_HAS_PRIMARY_FN_KEYS
select HAVE_ACPI_RESUME
select HAVE_ACPI_TABLES
--
To view, visit https://review.coreboot.org/c/coreboot/+/80437?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: I85fb69c8c1bed8635a1b31e9b8385c7036bb46dd
Gerrit-Change-Number: 80437
Gerrit-PatchSet: 6
Gerrit-Owner: Evgeny Zinoviev <me(a)ch1p.io>
Gerrit-Reviewer: Alexander Couzens <lynxis(a)fe80.eu>
Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: merged
Attention is currently required from: Christian Walter, Julius Werner, Jérémy Compostella, Martin L Roth, Michał Żygowski, Sergii Dmytruk.
Krystian Hebel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69159?usp=email )
Change subject: security/tpm: make tis_probe() return tpm_family
......................................................................
Patch Set 23:
(3 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/69159/comment/96762192_ad938657 :
PS23, Line 9:
Nit: double space
File src/drivers/i2c/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/69159/comment/47f4f456_4653c9c3 :
PS1, Line 461: *tpm_family = 1;
> To revive this thread: even if this driver (`tpm. […]
It seems that this is the sanest way to go, but please add a comment explaining it above the lines wherever version is set unconditionally.
File src/drivers/pc80/tpm/tis.c:
https://review.coreboot.org/c/coreboot/+/69159/comment/9313186c_e6dd8dbf :
PS23, Line 718: int
This is no longer int.
--
To view, visit https://review.coreboot.org/c/coreboot/+/69159?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: I5006e0cdfef76ff79ce9e1cf280fcd5515ae01b0
Gerrit-Change-Number: 69159
Gerrit-PatchSet: 23
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Martin L Roth <gaumless(a)gmail.com>
Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Mon, 12 Feb 2024 12:51:31 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Comment-In-Reply-To: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-MessageType: comment
Attention is currently required from: Angel Pons, Arthur Heymans, Felix Held, Krystian Hebel, Kyösti Mälkki, Nico Huber.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/77338?usp=email )
Change subject: device/pciexp_device.c: Fix setting Max Payload Size
......................................................................
Patch Set 19:
(3 comments)
File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/77338/comment/2fc8b46b_92475192 :
PS18, Line 596: unsigned int
> u16
All `pci_find_capability(dev, PCI_CAP_ID_PCIE);` calls were assigning a value to unsigned int (with a small u16 exceptions I made in `pcie_is_root_port` and `pcie_is_endpoint`, which I just have changed to unsigned int for consistency in patchset 19).
If you insist on using u16, I can make a follow-up patch to batch-change all `pci_find_capability(dev, PCI_CAP_ID_PCIE)` calls to assign a value to u16 in this file.
https://review.coreboot.org/c/coreboot/+/77338/comment/5e5cf38c_c0ddcbb8 :
PS18, Line 610: unsigned int
> u16
All `pci_find_capability(dev, PCI_CAP_ID_PCIE);` calls were assigning a value to unsigned int (with a small u16 exceptions I made in `pcie_is_root_port` and `pcie_is_endpoint`, which I just have changed to unsigned int for consistency in patchset 19).
If you insist on using u16, I can make a follow-up patch to batch-change all `pci_find_capability(dev, PCI_CAP_ID_PCIE)` calls to assign a value to u16 in this file.
https://review.coreboot.org/c/coreboot/+/77338/comment/99e854d8_cabe929f :
PS18, Line 623: unsigned int
> u16
All `pci_find_capability(dev, PCI_CAP_ID_PCIE);` calls were assigning a value to unsigned int (with a small u16 exceptions I made in `pcie_is_root_port` and `pcie_is_endpoint`, which I just have changed to unsigned int for consistency in patchset 19).
If you insist on using u16, I can make a follow-up patch to batch-change all `pci_find_capability(dev, PCI_CAP_ID_PCIE)` calls to assign a value to u16 in this file.
--
To view, visit https://review.coreboot.org/c/coreboot/+/77338?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: I24386dc208363b7d94fea46dec25c231a3968225
Gerrit-Change-Number: 77338
Gerrit-PatchSet: 19
Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Angel Pons <angel.pons(a)9elements.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Attention: Angel Pons <angel.pons(a)9elements.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Mon, 12 Feb 2024 12:47:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-MessageType: comment
Attention is currently required from: Angel Pons, Arthur Heymans, Felix Held, Kyösti Mälkki, Michał Żygowski, Nico Huber.
Hello Angel Pons, Arthur Heymans, Felix Held, Kyösti Mälkki, Nico Huber, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/77338?usp=email
to look at the new patch set (#19).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: device/pciexp_device.c: Fix setting Max Payload Size
......................................................................
device/pciexp_device.c: Fix setting Max Payload Size
Current implementation assumes that the endpoint device is connected
directly to the PCIe Root Port, which does not always have to be true.
In a case where there is a PCIe switch between the endpoint and the
root port, the Max Payload Size capability may differ across the
devices in the chain and coreboot will not set a correct Max Payload
Size. This results in a PCIe device malfunction in pre-OS environment,
e.g. if the Ethernet NICs are connected behind a PCIe switch, the iPXE
fails to obtain the DHCP configuration.
Fix this by traversing the topology and programming the highest common
Max Payload Size in the given PCIe device chain during enumeration.
Once finished, the root port has the highest common Max Payload Size
supported by all the devices in the chain. So at the end of root port
bus scan, propagate the root port's Max Payload Size to all downstream
devices to keep Max Payload Size in sync within the whole chain.
TEST=Perform successful dhcp command in iPXE on the NIC connected to
the PCIe root port via ASMedia ASM1806 PCIe switch and again on the
NIC connected directly to the PCIe root port.
Change-Id: I24386dc208363b7d94fea46dec25c231a3968225
Signed-off-by: Michał Żygowski <michal.zygowski(a)3mdeb.com>
---
M src/device/pciexp_device.c
1 file changed, 129 insertions(+), 28 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/77338/19
--
To view, visit https://review.coreboot.org/c/coreboot/+/77338?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: I24386dc208363b7d94fea46dec25c231a3968225
Gerrit-Change-Number: 77338
Gerrit-PatchSet: 19
Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Angel Pons <angel.pons(a)9elements.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Attention: Angel Pons <angel.pons(a)9elements.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: Angel Pons, Arthur Heymans, Felix Held, Kyösti Mälkki, Michał Żygowski, Nico Huber.
Krystian Hebel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/77338?usp=email )
Change subject: device/pciexp_device.c: Fix setting Max Payload Size
......................................................................
Patch Set 18:
(8 comments)
File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/77338/comment/78d71582_991bf0e7 :
PS11, Line 630: root->max_payload_set = 1;
> Current implementation looks good to me. Krystian, Patrick, do you want to […]
I've left comments in where I think the implementation still needs fixing for specific corner case. Marking this discussion as resolved.
File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/77338/comment/ebd69af5_3fcd0bf3 :
PS17, Line 771: Paylaod
> Done
Done
https://review.coreboot.org/c/coreboot/+/77338/comment/ac6261e2_6bf46520 :
PS17, Line 774: dvices
> Done
Done
File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/77338/comment/f2027548_09f8c322 :
PS18, Line 596: unsigned int
u16
https://review.coreboot.org/c/coreboot/+/77338/comment/459583c5_3dc0dc54 :
PS18, Line 610: unsigned int
u16
https://review.coreboot.org/c/coreboot/+/77338/comment/1820542c_09da347a :
PS18, Line 623: unsigned int
u16
https://review.coreboot.org/c/coreboot/+/77338/comment/dd4ec3a3_6e8a00fe :
PS18, Line 653: pciexp_dev_set_max_payload_size(parent, max_payload);
Would it make sense to put this inside `if` below?
https://review.coreboot.org/c/coreboot/+/77338/comment/2ad76b33_6d9a5214 :
PS18, Line 721: parent's
Shouldn't the root's (real one, not what `root` variable points to) Max Payload Size be limited here instead? If I understand this correctly, it only limits immediate parent's MPS and stops there, without propagating it further, so it may be overwritten by `pciexp_sync_max_payload_size()` called from `pciexp_scan_bus()`. This only matters for multi-level bridges, but it should be an easy fix.
--
To view, visit https://review.coreboot.org/c/coreboot/+/77338?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: I24386dc208363b7d94fea46dec25c231a3968225
Gerrit-Change-Number: 77338
Gerrit-PatchSet: 18
Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Angel Pons <angel.pons(a)9elements.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Attention: Angel Pons <angel.pons(a)9elements.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Mon, 12 Feb 2024 12:10:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Comment-In-Reply-To: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-MessageType: comment