Hello build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Maulik V Vaghela, Duncan Laurie, Sridhar Siricilla, Aamir Bohra, Srinidhi N Kaushik, Andrey Petrov, Aaron Durbin, Patrick Rudolph, Nathaniel L Desimone,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41728
to look at the new patch set (#12).
Change subject: drivers/intel/fsp2_0: Add FSP 2.2 specific support
......................................................................
drivers/intel/fsp2_0: Add FSP 2.2 specific support
• Based on FSP EAS v2.1 – Backward compatibility is retained.
• Added multi-phase silicon initialization to increase the modularity of the
FspSiliconInit() API.
• Added FspMultiPhaseSiInit() API
• FSP_INFO_HEADER changes
o Added FspMultiPhaseSiInitEntryOffset
• Added FSPS_ARCH_UPD
o Added EnableMultiPhaseSiliconInit, bootloaders designed for
FSP 2.0/2.1 can disable the FspMultiPhaseSiInit() API and
continue to use FspSiliconInit() without change.
FSP 2.2 Specification:
https://www.intel.com/content/www/us/en/intelligent-systems/intel-firmware-…
Change-Id: If7177a267f3a9b4cbb60a639f1c737b9a3341913
Signed-off-by: Subrata Banik <subrata.banik(a)intel.com>
---
M src/commonlib/include/commonlib/timestamp_serialized.h
M src/drivers/intel/fsp2_0/Kconfig
M src/drivers/intel/fsp2_0/include/fsp/api.h
M src/drivers/intel/fsp2_0/include/fsp/info_header.h
M src/drivers/intel/fsp2_0/include/fsp/upd.h
M src/drivers/intel/fsp2_0/include/fsp/util.h
M src/drivers/intel/fsp2_0/silicon_init.c
M src/drivers/intel/fsp2_0/util.c
M src/include/console/post_codes.h
9 files changed, 172 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/41728/12
--
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: 12
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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-MessageType: newpatchset
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 11:
(4 comments)
https://review.coreboot.org/c/coreboot/+/41728/11/src/drivers/intel/fsp2_0/…
File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/41728/11/src/drivers/intel/fsp2_0/…
PS11, Line 27: impacts
> impact
Ack
https://review.coreboot.org/c/coreboot/+/41728/11/src/drivers/intel/fsp2_0/…
File src/drivers/intel/fsp2_0/include/fsp/info_header.h:
https://review.coreboot.org/c/coreboot/+/41728/11/src/drivers/intel/fsp2_0/…
PS11, Line 33: size_t multi_phase_si_init_entry_offset;
> Doesn't this make the struct layout incompatible with FSP 2. […]
not actually as its not a packed structure that mean we don't use this structure directly anywhere rather we use dedicated field hence it won't break anything, tested on FSP2.0 and FSP2.1 platform already
https://review.coreboot.org/c/coreboot/+/41728/11/src/drivers/intel/fsp2_0/…
File src/drivers/intel/fsp2_0/include/fsp/util.h:
https://review.coreboot.org/c/coreboot/+/41728/11/src/drivers/intel/fsp2_0/…
PS11, Line 36: uint32_t phase_index
> Are these phase indices enumerated? If so, I'd use an enum for this
we really don't know how many phase FSP will populate hence we can't use enum here. what is possible been mentioned at https://review.coreboot.org/c/coreboot/+/41728/11/src/drivers/intel/fsp2_0/…
Here is the flow
1. Check if FSP header has implemented multiphasesiinit API
2. If yes, then first call multiphasesiinit API with GET_NUMBER_OF_PHASES (0) to know the number of phase supported
3.Based on return value from #2, we call each phase index (from 1 till struct fsp_multi_phase_params.phase_index) multiphasesiinit API with EXECUTE_PHASE (1)
https://review.coreboot.org/c/coreboot/+/41728/11/src/drivers/intel/fsp2_0/…
File src/drivers/intel/fsp2_0/silicon_init.c:
https://review.coreboot.org/c/coreboot/+/41728/11/src/drivers/intel/fsp2_0/…
PS11, Line 57: BIOS_SPEW
> BIOS_EMERG […]
Thanks 😊
--
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: 11
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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Comment-Date: Sat, 06 Jun 2020 07:22:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41758 )
Change subject: mb/intel/jasperlake_rvp: camera SSDT changes for JSLRVP
......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41758/5/src/mainboard/intel/jasper…
File src/mainboard/intel/jasperlake_rvp/variants/jslrvp/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/41758/5/src/mainboard/intel/jasper…
PS5, Line 320: register "on_seq.ops[0].type" = "IMGCLK"
: register "on_seq.ops[0].index" = "0" #clk enable
: register "on_seq.ops[0].action" = "ENABLE"
suggestion: making a macro to build these structs could make this easier to read.
maybe:
#define SEQ_OPS_CLK(index, act) \
{.type=IMGCLK, .index = (index), .action = (act) }
and
#define SEQ_OPS_GPIO(index, act, delay) \
{.type=GPIO, .index = (index), .action = (act), .delay_ms = (delay) }
etc.
it would also reduce each one to one line which greatly shortens the devicetree 😊
https://review.coreboot.org/c/coreboot/+/41758/5/src/mainboard/intel/jasper…
PS5, Line 446: register "cio2_lanes_used[0]" = "2"
: register "cio2_lanes_used[1]" = "2"
could be register "cio2_lanes_used" = "{2, 2}"
--
To view, visit https://review.coreboot.org/c/coreboot/+/41758
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib439572bc1d15ef02c86c7bfa88af6b16eb06f97
Gerrit-Change-Number: 41758
Gerrit-PatchSet: 5
Gerrit-Owner: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com>
Gerrit-Reviewer: Daniel Kang <daniel.h.kang(a)intel.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Varshit B Pandya <varshit.b.pandya(a)intel.com>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-CC: Kiran2 Kumar <kiran2.kumar(a)intel.corp-partner.google.com>
Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Comment-Date: Sat, 06 Jun 2020 02:03:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41607 )
Change subject: drivers/intel/mipi_camera: camera SSDT generation
......................................................................
Patch Set 11:
(12 comments)
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca…
File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca…
PS11, Line 54: struct acpi_dp *lanes = acpi_dp_new_table("data-lanes");
: uint32_t j;
in coreboot, we usually declare all local variables at the top of the function.
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca…
PS11, Line 94: if (!config->pld.ignore_color)
: config->pld.ignore_color = 1;
:
: if (!config->pld.visible)
: config->pld.visible = 1;
:
: if (!config->pld.vertical_offset)
: config->pld.vertical_offset = 0xffff;
:
: if (!config->pld.horizontal_offset)
: config->pld.horizontal_offset = 0xffff;
:
: /*
: * PLD_PANEL_TOP has a value of zero, so the following will change any instance
: * of PLD_PANEL_TOP to PLD_PANEL_FRONT unless disable_pld_defaults is set.
: */
: if (!config->pld.panel)
: config->pld.panel = PLD_PANEL_FRONT;
:
: /*
: * PLD_HORIZONTAL_POSITION_LEFT has a value of zero, so the following will
: * change any instance of that value to PLD_HORIZONTAL_POSITION_CENTER unless
: * disable_pld_defaults is set.
: */
: if (!config->pld.horizontal_position)
: config->pld.horizontal_position = PLD_HORIZONTAL_POSITION_CENTER;
:
: /*
: * The desired default for |vertical_position| is PLD_VERTICAL_POSITION_UPPER,
: * which has a value of zero so no work is needed to set a default. The same
: * applies for setting |shape| to PLD_SHAPE_ROUND.
: */
suggestion: break this out into a separate function
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca…
PS11, Line 139: 0x4
4
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca…
PS11, Line 148: 822ace8f-2814-4174-a56b-5f029fe079ee
a symbolic constant with a nice name would be helpful
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca…
PS11, Line 156: 26257549-9271-4ca4-bb43-c4899d5a4881
a symbolic constant with a nice name would be helpful
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca…
PS11, Line 167: acpigen_write_return_integer(((uint32_t)i2c_bus) << 24 |
: ((uint32_t)i2c_addr) << 8 | 0x0); /* type 0 for sensor */
: acpigen_pop_len(); /* If Arg2=2 */
:
: if (config->ssdb.vcm_type) {
: /* If (LEqual (Local1, 3)) */
: acpigen_write_if_lequal_op_int(LOCAL1_OP, 3);
: acpigen_write_return_integer(((uint32_t)i2c_bus) << 24 | /* type 1 for vcm */
: ((uint32_t)config->vcm_address) << 8 | 0x01);
: acpigen_pop_len(); /* If Arg2=3 */
: }
:
: if (config->ssdb.rom_type) {
: /* If (LEqual (Local1, 3 or 4)) */
: acpigen_write_if_lequal_op_int(LOCAL1_OP, 3 + (config->ssdb.vcm_type ? 1 : 0));
: acpigen_write_return_integer(((uint32_t)i2c_bus) << 24 | /* type 2 for rom */
: ((uint32_t)config->rom_address) << 8 | 0x02);
: acpigen_pop_len(); /* If Arg2=3 or 4 */
suggestion: add a helper function for creating these return values, with maybe an enum per type, sensor, vcm or rom
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca…
PS11, Line 229: if (!config->disable_ssdb_defaults) {
: if (!config->ssdb.bdf_value)
: config->ssdb.bdf_value = PCI_DEVFN(CIO2_PCI_DEV, CIO2_PCI_FN);
:
: if (!config->ssdb.platform)
: config->ssdb.platform = PLATFORM_SKC;
:
: if (!config->ssdb.flash_support)
: config->ssdb.flash_support = FLASH_DISABLE;
:
: if (!config->ssdb.privacy_led)
: config->ssdb.privacy_led = PRIVACY_LED_A_16mA;
:
: if (!config->ssdb.mipi_define)
: config->ssdb.mipi_define = MIPI_INFO_ACPI_DEFINED;
:
: if (!config->ssdb.mclk_speed)
: config->ssdb.mclk_speed = CLK_FREQ_19_2MHZ;
:
: if (!config->ssdb.lanes_used) {
: cio2_config = cio2 ? cio2->chip_info : NULL;
:
: if (!cio2_config) {
: printk(BIOS_ERR, "Failed to get CIO2 config\n");
: } else if (cio2_config->device_type != INTEL_ACPI_CAMERA_CIO2) {
: printk(BIOS_ERR, "Device type isn't CIO2: %u\n",
: (u32)cio2_config->device_type);
: } else if (config->ssdb.link_used >= cio2_config->cio2_num_ports) {
: printk(BIOS_ERR, "%u exceeds CIO2's %u links\n",
: (u32)config->ssdb.link_used,
: (u32)cio2_config->cio2_num_ports);
: } else {
: config->ssdb.lanes_used =
: cio2_config->cio2_lanes_used[config->ssdb.link_used];
: }
: }
: }
suggestion: same here, separate this out into a separate function
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca…
PS11, Line 272: 0
Is this always supposed to be 0?
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca…
PS11, Line 298: remote_name = "\\_SB.PCI0.CIO2";
Could you add this to the rest of the defaults?
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca…
PS11, Line 303: /* endpoint? */
?
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca…
PS11, Line 315: 180
always 180?
https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca…
PS11, Line 566: if (config->device_type == INTEL_ACPI_CAMERA_CIO2)
: return "CIO2";
: else if (config->device_type == INTEL_ACPI_CAMERA_IMGU)
: return "IMGU";
: else if (config->device_type == INTEL_ACPI_CAMERA_PMIC)
: return "PMIC";
:
: switch (config->device_type) {
: case INTEL_ACPI_CAMERA_SENSOR:
: prefix = "CAM";
: break;
: case INTEL_ACPI_CAMERA_VCM:
: prefix = "VCM";
: break;
: case INTEL_ACPI_CAMERA_NVM:
: prefix = "NVM";
: break;
: default:
: prefix = "ERR"; /* Error */
: break;
: }
This can just be one switch statement; you can just return from the cases that have all 4 characters already, and let the prefixed ones keep going.
--
To view, visit https://review.coreboot.org/c/coreboot/+/41607
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I15979f345fb823df2560db269e902a1ea650b69e
Gerrit-Change-Number: 41607
Gerrit-PatchSet: 11
Gerrit-Owner: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com>
Gerrit-Reviewer: Daniel Kang <daniel.h.kang(a)intel.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Matt Delco <delco(a)chromium.org>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com>
Gerrit-Reviewer: Varshit B Pandya <varshit.b.pandya(a)intel.com>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-CC: Kiran2 Kumar <kiran2.kumar(a)intel.corp-partner.google.com>
Gerrit-Comment-Date: Sat, 06 Jun 2020 01:57:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Tim Wawrzynczak has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42072 )
Change subject: libpayload: drivers/usb: add a USB pre-poll hook
......................................................................
libpayload: drivers/usb: add a USB pre-poll hook
This adds a hook so that a payload can optionally perform USB service
functions in conjunction with regular USB port status polling. In
particular, this allows depthcharge to control the state of an
external USB mux. Some SoCs like Tiger Lake have a USB mux for Type-C
ports that must be kept in sync with the state of the port as reported
by the TCPC. This can be achieved by hooking into the poll routine to
refresh the state of the USB mux.
BUG=b:149883933
TEST=booted into recovery from Type-C flash drive on volteer
Change-Id: Ic6c23756f64b891b3c5683cd650c605b8630b0fb
Signed-off-by: Caveh Jalali <caveh(a)chromium.org>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/42072
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Julius Werner <jwerner(a)chromium.org>
---
M payloads/libpayload/drivers/usb/usb.c
M payloads/libpayload/include/usb/usb.h
2 files changed, 11 insertions(+), 0 deletions(-)
Approvals:
build bot (Jenkins): Verified
Julius Werner: Looks good to me, approved
diff --git a/payloads/libpayload/drivers/usb/usb.c b/payloads/libpayload/drivers/usb/usb.c
index accb228..b14abb4 100644
--- a/payloads/libpayload/drivers/usb/usb.c
+++ b/payloads/libpayload/drivers/usb/usb.c
@@ -86,6 +86,10 @@
{
if (usb_hcs == 0)
return;
+
+ if (usb_poll_prepare)
+ usb_poll_prepare();
+
hci_t *controller = usb_hcs;
while (controller != NULL) {
int i;
diff --git a/payloads/libpayload/include/usb/usb.h b/payloads/libpayload/include/usb/usb.h
index 8f3169e..f79fc27 100644
--- a/payloads/libpayload/include/usb/usb.h
+++ b/payloads/libpayload/include/usb/usb.h
@@ -345,6 +345,13 @@
}
/**
+ * To be implemented by libpayload-client. It's called by the USB
+ * stack just before iterating over known devices to poll them for
+ * status change.
+ */
+void __attribute__((weak)) usb_poll_prepare (void);
+
+/**
* To be implemented by libpayload-client. It's called by the USB stack
* when a new USB device is found which isn't claimed by a built in driver,
* so the client has the chance to know about it.
--
To view, visit https://review.coreboot.org/c/coreboot/+/42072
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic6c23756f64b891b3c5683cd650c605b8630b0fb
Gerrit-Change-Number: 42072
Gerrit-PatchSet: 3
Gerrit-Owner: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
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-MessageType: merged