Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41728 )
Change subject: drivers/intel/fsp2_0: Add FSP 2.2 specific support
......................................................................
Patch Set 9:
(6 comments)
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/i…
File src/drivers/intel/fsp2_0/include/fsp/info_header.h:
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/i…
PS9, Line 33: #if CONFIG(PLATFORM_USES_FSP2_2)
> this doesn't seem to be used as a packed structure (each field read separately in fsp_identify) so d […]
Ack
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/i…
File src/drivers/intel/fsp2_0/include/fsp/util.h:
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/i…
PS9, Line 24: enum fsp_multi_phase_action {
: GET_NUMBER_OF_PHASES,
: EXECUTE_PHASE
: };
> This is part of an ABI, it would be good to explicitly call out the integer value of each enum.
Ack
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/s…
File src/drivers/intel/fsp2_0/silicon_init.c:
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/s…
PS9, Line 45: erros
> errors
Ack
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/s…
PS9, Line 46: !=
> This isn't the correct way to test for success anyway. It should be if(!EFI_ERROR(status)).
We don't use those EFI_ERROR handling case in coreboot.
> It is possible to have "successful" error codes other than EFI_SUCCESS. The MSB indicates error.
Right now its mostly 5 return types as per spec
EFI_SUCCESS FSP execution environment was initialized successfully.
EFI_INVALID_PARAMETER Input parameters are invalid.
EFI_UNSUPPORTED The FSP calling conditions were not met.
EFI_DEVICE_ERROR FSP silicon initialization failed.
FSP_STATUS_RESET_REQUIRED_* A reset is required. These status codes will not be
returned during S3.
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/s… handles FSP_STATUS_RESET_REQUIRED_* return type case
EFI_SUCCESS been checked here
Rest everything is error code which results into die in coreboot code
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/s…
PS9, Line 146: (void *)
> I don't think you need to cast here
Ack
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/u…
File src/drivers/intel/fsp2_0/util.c:
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/u…
PS9, Line 52: #if CONFIG(PLATFORM_USES_FSP2_2)
> if the header didn't use #if to guard this field the code could just be "if (CONFIG(PLATFORM_USES_FS […]
Ack
--
To view, visit https://review.coreboot.org/c/coreboot/+/41728
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: If7177a267f3a9b4cbb60a639f1c737b9a3341913
Gerrit-Change-Number: 41728
Gerrit-PatchSet: 9
Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com>
Gerrit-Reviewer: Nathaniel L Desimone <nathaniel.l.desimone(a)intel.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Comment-Date: Fri, 05 Jun 2020 08:04:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Duncan Laurie <dlaurie(a)chromium.org>
Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Comment-In-Reply-To: Nathaniel L Desimone <nathaniel.l.desimone(a)intel.com>
Gerrit-MessageType: comment
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36606 )
Change subject: device/pci: Add early PCI dump
......................................................................
Patch Set 3: Code-Review+1
(1 comment)
Sorry, I think I removed the WIP status in February due to the confusing Gerrit user interface. I can set it back if you want.
https://review.coreboot.org/c/coreboot/+/36606/3/src/device/pci_dump.c
File src/device/pci_dump.c:
https://review.coreboot.org/c/coreboot/+/36606/3/src/device/pci_dump.c@12
PS3, Line 12: */
The new SPDX headers should be used.
--
To view, visit https://review.coreboot.org/c/coreboot/+/36606
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic095213ab51a3c29af462782fdb75cfcd409714f
Gerrit-Change-Number: 36606
Gerrit-PatchSet: 3
Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Dmitry Ponamorev
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Reviewer: Дмитрий Понаморев <dponamorev(a)gmail.com>
Gerrit-Comment-Date: Fri, 05 Jun 2020 07:18:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Duncan Laurie has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42047 )
Change subject: acpi: Accomodate non-standard UUIDs in device properties
......................................................................
acpi: Accomodate non-standard UUIDs in device properties
There have been changes to the way device properties are supported
in Linux[1] and Windows[2] which add flexibilty:
- non-standard UUIDs can be used instead of only ACPI_DP_UUID
- support for multiple different packages within the _DSD that
associate different properties with unique UUIDs.
To handle this I extracted the part that does the write of UUID and
properties to a separate function and defined a new PACKAGE type
which has the custom UUID as a name and can be used the same way that
child properties are today.
For example a PCIe root port for a USB4 port has a standard property
indicating the USB4 reference, and then two custom properties which
are defined for different attributes.
Example code:
/* Create property table */
acpi_dp *dsd = acpi_dp_new_table("_DSD");
acpi_dp_add_reference(dsd, "usb4-port", usb4_path);
/* Add package for hotplug */
acpi_dp *pkg = acpi_dp_new_table("6211e2c0-58a3-4af3-90e1-927a4e0c55a4");
acpi_dp_add_integer(pkg, "HotPlugSupportInD3", 1);
acpi_dp_add_package(dsd, pkg);
/* Add package for external port info */
pkg = acpi_dp_new_table("efcc06cc-73ac-4bc3-bff0-76143807c389");
acpi_dp_add_integer(pkg, "ExternalFacingPort", 1);
acpi_dp_add_package(dsd, pkg);
/* Write all properties */
acpi_dp_write(dsd);
Resulting ACPI:
Scope (\_SB.PCI0.TRP0)
{
Name (_DSD, Package ()
{
ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301")
Package ()
{
Package (0x02) { "usb4-port", \_SB.PCI0.TDM0.RHUB.PRTA }
},
ToUUID ("6211e2c0-58a3-4af3-90e1-927a4e0c55a4"),
Package ()
{
Package () { "HotPlugSupportInD3", One }
},
ToUUID ("efcc06cc-73ac-4bc3-bff0-76143807c389"),
Package ()
{
Package () { "ExternalFacingPort", One },
}
})
}
[1] https://patchwork.kernel.org/patch/10599675/
[2] https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-…
Change-Id: I75f47825bf4ffc5e9e92af2c45790d1b5945576e
Signed-off-by: Duncan Laurie <dlaurie(a)google.com>
---
M src/acpi/device.c
M src/include/acpi/acpi_device.h
2 files changed, 77 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/42047/1
diff --git a/src/acpi/device.c b/src/acpi/device.c
index 8b03bfd..9e8efa4 100644
--- a/src/acpi/device.c
+++ b/src/acpi/device.c
@@ -53,7 +53,7 @@
return NULL;
/* Check for device specific handler */
- if (dev->ops->acpi_name)
+ if (dev->ops && dev->ops->acpi_name)
return dev->ops->acpi_name(dev);
/* Walk up the tree to find if any parent can identify this device */
@@ -658,10 +658,48 @@
}
}
+static bool acpi_dp_write_properties(struct acpi_dp *prop, const char *uuid)
+{
+ struct acpi_dp *dp;
+ char *prop_count = NULL;
+
+ /* Print base properties */
+ for (dp = prop; dp; dp = dp->next) {
+ if (dp->type == ACPI_DP_TYPE_TABLE ||
+ dp->type == ACPI_DP_TYPE_CHILD ||
+ dp->type == ACPI_DP_TYPE_PACKAGE)
+ continue;
+
+ /*
+ * The UUID and package is only added when
+ * we come across the first property. This
+ * is to avoid creating a zero-length package
+ * in situations where there are only children.
+ */
+ if (!prop_count) {
+ /* ToUUID (dp->uuid) */
+ acpigen_write_uuid(uuid);
+ /*
+ * Package (PROP), element count determined as
+ * it is populated
+ */
+ prop_count = acpigen_write_package(0);
+ }
+ (*prop_count)++;
+ acpi_dp_write_property(dp);
+ }
+ if (prop_count) {
+ /* Package (PROP) length, if a package was written */
+ acpigen_pop_len();
+ return true;
+ }
+ return false;
+}
+
void acpi_dp_write(struct acpi_dp *table)
{
struct acpi_dp *dp, *prop;
- char *dp_count, *prop_count = NULL;
+ char *dp_count;
int child_count = 0;
if (!table || table->type != ACPI_DP_TYPE_TABLE || !table->next)
@@ -677,37 +715,17 @@
dp_count = acpigen_write_package(0);
/* Print base properties */
- for (dp = prop; dp; dp = dp->next) {
- if (dp->type == ACPI_DP_TYPE_CHILD) {
- child_count++;
- } else {
- /*
- * The UUID and package is only added when
- * we come across the first property. This
- * is to avoid creating a zero-length package
- * in situations where there are only children.
- */
- if (!prop_count) {
- *dp_count += 2;
- /* ToUUID (ACPI_DP_UUID) */
- acpigen_write_uuid(ACPI_DP_UUID);
- /*
- * Package (PROP), element count determined as
- * it is populated
- */
- prop_count = acpigen_write_package(0);
- }
- (*prop_count)++;
- acpi_dp_write_property(dp);
- }
- }
- if (prop_count) {
- /* Package (PROP) length, if a package was written */
- acpigen_pop_len();
- }
+ if (acpi_dp_write_properties(prop, table->uuid))
+ *dp_count += 2;
+ /* Count child properties */
+ for (dp = prop; dp; dp = dp->next)
+ if (dp->type == ACPI_DP_TYPE_CHILD)
+ child_count++;
+
+ /* Add child properties to the base table */
if (child_count) {
- /* Update DP package count to 2 or 4 */
+ /* Update DP package count */
*dp_count += 2;
/* ToUUID (ACPI_DP_CHILD_UUID) */
acpigen_write_uuid(ACPI_DP_CHILD_UUID);
@@ -722,6 +740,12 @@
acpigen_pop_len();
}
+ /* Write packages of properties with unique UUID */
+ for (dp = prop; dp; dp = dp->next)
+ if (dp->type == ACPI_DP_TYPE_PACKAGE)
+ if (acpi_dp_write_properties(dp->child, dp->uuid))
+ *dp_count += 2;
+
/* Package (DP) length */
acpigen_pop_len();
@@ -746,6 +770,7 @@
memset(new, 0, sizeof(*new));
new->type = type;
new->name = name;
+ new->uuid = ACPI_DP_UUID;
if (dp) {
/* Add to end of property list */
@@ -863,6 +888,22 @@
return new;
}
+struct acpi_dp *acpi_dp_add_package(struct acpi_dp *dp, struct acpi_dp *package)
+{
+ struct acpi_dp *new;
+
+ if (!dp || !package || package->type != ACPI_DP_TYPE_TABLE)
+ return NULL;
+
+ new = acpi_dp_new(dp, ACPI_DP_TYPE_PACKAGE, NULL);
+ if (new) {
+ new->uuid = package->name;
+ new->child = package;
+ }
+
+ return new;
+}
+
struct acpi_dp *acpi_dp_add_array(struct acpi_dp *dp, struct acpi_dp *array)
{
struct acpi_dp *new;
diff --git a/src/include/acpi/acpi_device.h b/src/include/acpi/acpi_device.h
index 20dd8a4..2c2c67a 100644
--- a/src/include/acpi/acpi_device.h
+++ b/src/include/acpi/acpi_device.h
@@ -15,11 +15,13 @@
ACPI_DP_TYPE_TABLE,
ACPI_DP_TYPE_ARRAY,
ACPI_DP_TYPE_CHILD,
+ ACPI_DP_TYPE_PACKAGE,
};
struct acpi_dp {
enum acpi_dp_type type;
const char *name;
+ const char *uuid;
struct acpi_dp *next;
union {
struct acpi_dp *child;
@@ -464,6 +466,9 @@
/* Start a new Device Property table with provided ACPI reference */
struct acpi_dp *acpi_dp_new_table(const char *ref);
+/* Add package of device properties with a unique UUID */
+struct acpi_dp *acpi_dp_add_package(struct acpi_dp *dp, struct acpi_dp *package);
+
/* Add integer Device Property */
struct acpi_dp *acpi_dp_add_integer(struct acpi_dp *dp, const char *name,
uint64_t value);
--
To view, visit https://review.coreboot.org/c/coreboot/+/42047
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I75f47825bf4ffc5e9e92af2c45790d1b5945576e
Gerrit-Change-Number: 42047
Gerrit-PatchSet: 1
Gerrit-Owner: Duncan Laurie <dlaurie(a)chromium.org>
Gerrit-MessageType: newchange
Keith Hui has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41460 )
Change subject: cpu/intel/slot_1: Select 16KiB bootblock if console is enabled
......................................................................
cpu/intel/slot_1: Select 16KiB bootblock if console is enabled
We previously confirmed [1] that bootblock will grow beyond current
8KiB size if console is enabled. Automatically change to 16KiB if user
enabled it in menuconfig.
[1] https://review.coreboot.org/c/coreboot/+/36775
Change-Id: Ic9988c77cf9677167a382aa4dc7dcfa2bc4cbe02
Signed-off-by: Keith Hui <buurin(a)gmail.com>
---
M src/cpu/intel/slot_1/Kconfig
1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/41460/1
diff --git a/src/cpu/intel/slot_1/Kconfig b/src/cpu/intel/slot_1/Kconfig
index 1ba71ad..a56208b 100644
--- a/src/cpu/intel/slot_1/Kconfig
+++ b/src/cpu/intel/slot_1/Kconfig
@@ -34,6 +34,7 @@
config C_ENV_BOOTBLOCK_SIZE
hex
+ default 0x4000 if BOOTBLOCK_CONSOLE
default 0x2000
endif
--
To view, visit https://review.coreboot.org/c/coreboot/+/41460
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic9988c77cf9677167a382aa4dc7dcfa2bc4cbe02
Gerrit-Change-Number: 41460
Gerrit-PatchSet: 1
Gerrit-Owner: Keith Hui <buurin(a)gmail.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-MessageType: newchange
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41853 )
Change subject: mb/google/dedede: config spk_en gpio to low by default
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/41853/2/src/mainboard/google/deded…
File src/mainboard/google/dedede/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/41853/2/src/mainboard/google/deded…
PS2, Line 178: GPP_D17
> I think Furquan was referring to register "sdmode_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPP_D17)" in […]
As mentioned by Usha, audio support for waddledee is yet to be added. The GPIO will be exposed through ACPI when the audio support gets added in waddledee. Even though the amplifier is different, the GPIO purpose is still the same i.e to enable the speaker.
--
To view, visit https://review.coreboot.org/c/coreboot/+/41853
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I070679457b06cb82633c1197b893a5d89c8b2cf0
Gerrit-Change-Number: 41853
Gerrit-PatchSet: 3
Gerrit-Owner: Usha P <usha.p(a)intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Divagar Mohandass <divagar.mohandass(a)intel.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Fri, 05 Jun 2020 04:33:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com>
Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Comment-In-Reply-To: Usha P <usha.p(a)intel.com>
Gerrit-MessageType: comment