April 2022 - coreboot-gerrit - mail.coreboot.org

Change in coreboot[master]: soc/intel/alderlake: Add support to update CSE descriptor at runtime
by build bot (Jenkins) (Code Review) April 5, 2022

April 5, 2022

Attention is currently required from: Tim Wawrzynczak. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63365 ) Change subject: soc/intel/alderlake: Add support to update CSE descriptor at runtime ...................................................................... Patch Set 1: (2 comments) File src/soc/intel/alderlake/bootblock/cse_descriptor.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145364): https://review.coreboot.org/c/coreboot/+/63365/comment/6294f711_b4b87411 PS1, Line 40: void configure_cse_descriptor(size_t byte, uint8_t desired_value) { open brace '{' following function definitions go on the next line Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145364): https://review.coreboot.org/c/coreboot/+/63365/comment/bc7c9d95_f0abf10d PS1, Line 58: printk(BIOS_DEBUG, "Current value of Descriptor byte 0x%lx: 0x%x\n", byte, si_desc_buf[byte]); line over 96 characters -- To view, visit https://review.coreboot.org/c/coreboot/+/63365 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I43c4d2888706561e42ff6b8ce0377eedbc38dbfe Gerrit-Change-Number: 63365 Gerrit-PatchSet: 1 Gerrit-Owner: Reka Norman <rekanorman(a)chromium.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-CC: Reka Norman <rekanorman(a)google.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Comment-Date: Tue, 05 Apr 2022 07:37:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/brya/var/nereid: Configure Type-C CSE strap based on fw_config
by Reka Norman (Code Review) April 5, 2022

April 5, 2022

Attention is currently required from: Reka Norman, Tim Wawrzynczak. Hello Reka Norman, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/63366 to review the following change. Change subject: mb/google/brya/var/nereid: Configure Type-C CSE strap based on fw_config ...................................................................... mb/google/brya/var/nereid: Configure Type-C CSE strap based on fw_config The Type-C Port 2 CSE strap needs to be configured differently for Type-C and HDMI. To allow using a single firmware variant for both cases, update the strap at runtime based on fw_config. This is a temporary workaround while we find a better solution. BUG=b:226848617 TEST=Type-C and HDMI both work on nereid with fw_config set correctly. Change-Id: I2cc230e3bd35816c81989ae7e01df5d2c152062e Signed-off-by: Reka Norman <rekanorman(a)google.com> --- M src/mainboard/google/brya/Kconfig.name M src/mainboard/google/brya/bootblock.c M src/mainboard/google/brya/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/brya/variants/nereid/Makefile.inc M src/mainboard/google/brya/variants/nereid/variant.c 5 files changed, 29 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/63366/1 diff --git a/src/mainboard/google/brya/Kconfig.name b/src/mainboard/google/brya/Kconfig.name index 3126d28..c4a8530 100644 --- a/src/mainboard/google/brya/Kconfig.name +++ b/src/mainboard/google/brya/Kconfig.name @@ -90,6 +90,7 @@ config BOARD_GOOGLE_NEREID bool "-> Nereid" + select ALDERLAKE_CONFIGURE_CSE_DESCRIPTOR select BOARD_GOOGLE_BASEBOARD_NISSA config BOARD_GOOGLE_PRIMUS diff --git a/src/mainboard/google/brya/bootblock.c b/src/mainboard/google/brya/bootblock.c index 1815615..c24e959 100644 --- a/src/mainboard/google/brya/bootblock.c +++ b/src/mainboard/google/brya/bootblock.c @@ -10,3 +10,10 @@ pads = variant_early_gpio_table(&num); gpio_configure_pads(pads, num); } + +void bootblock_mainboard_init(void) +{ + variant_update_descriptor(); +} + +void __weak variant_update_descriptor(void) {} diff --git a/src/mainboard/google/brya/variants/baseboard/include/baseboard/variants.h b/src/mainboard/google/brya/variants/baseboard/include/baseboard/variants.h index 9accc08..f890171 100644 --- a/src/mainboard/google/brya/variants/baseboard/include/baseboard/variants.h +++ b/src/mainboard/google/brya/variants/baseboard/include/baseboard/variants.h @@ -35,6 +35,8 @@ /* Modify devictree settings during ramstage */ void variant_devtree_update(void); +void variant_update_descriptor(void); + struct cpu_power_limits { uint16_t mchid; u8 cpu_tdp; diff --git a/src/mainboard/google/brya/variants/nereid/Makefile.inc b/src/mainboard/google/brya/variants/nereid/Makefile.inc index 2e8157e..16c9748 100644 --- a/src/mainboard/google/brya/variants/nereid/Makefile.inc +++ b/src/mainboard/google/brya/variants/nereid/Makefile.inc @@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-2.0-only bootblock-y += gpio.c +bootblock-y += variant.c romstage-y += gpio.c romstage-y += memory.c diff --git a/src/mainboard/google/brya/variants/nereid/variant.c b/src/mainboard/google/brya/variants/nereid/variant.c index 967fc9a..11aeb40 100644 --- a/src/mainboard/google/brya/variants/nereid/variant.c +++ b/src/mainboard/google/brya/variants/nereid/variant.c @@ -1,7 +1,14 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */ +#include <baseboard/variants.h> +#include <console/console.h> #include <drivers/intel/gma/opregion.h> #include <fw_config.h> +#include <soc/bootblock.h> + +#define CPU_DESC_5_BYTE1 0xc7c +#define CPU_DESC_5_BYTE1_VALUE_TYPEC 0xee +#define CPU_DESC_5_BYTE1_VALUE_HDMI 0x1e const char *mainboard_vbt_filename(void) { @@ -10,3 +17,14 @@ return "vbt.bin"; } + +void variant_update_descriptor(void) +{ + if (fw_config_probe(FW_CONFIG(DB_USB, DB_1A_HDMI))) { + printk(BIOS_DEBUG, "Configuring CSE strap for HDMI\n"); + configure_cse_descriptor(CPU_DESC_5_BYTE1, CPU_DESC_5_BYTE1_VALUE_HDMI); + } else { + printk(BIOS_DEBUG, "Configuring CSE strap for Type-C\n"); + configure_cse_descriptor(CPU_DESC_5_BYTE1, CPU_DESC_5_BYTE1_VALUE_TYPEC); + } +} -- To view, visit https://review.coreboot.org/c/coreboot/+/63366 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2cc230e3bd35816c81989ae7e01df5d2c152062e Gerrit-Change-Number: 63366 Gerrit-PatchSet: 1 Gerrit-Owner: Reka Norman <rekanorman(a)chromium.org> Gerrit-Reviewer: Reka Norman <rekanorman(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Reka Norman <rekanorman(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: soc/intel/alderlake: Add support to update CSE descriptor at runtime
by Reka Norman (Code Review) April 5, 2022

April 5, 2022

Reka Norman has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/63365 ) Change subject: soc/intel/alderlake: Add support to update CSE descriptor at runtime ...................................................................... soc/intel/alderlake: Add support to update CSE descriptor at runtime On nereid, we need to update the CSE descriptor based on fw_config (see the following patch), so add support to update the descriptor at runtime. This is a temporary workaround while we find a better solution. This is basically adding back the configure_pmc_descriptor() function removed in CB:63339, just making it generic. BUG=b:226848617 TEST=With the following patch, Type-C and HDMI work on nereid. Change-Id: I43c4d2888706561e42ff6b8ce0377eedbc38dbfe --- M src/soc/intel/alderlake/Kconfig M src/soc/intel/alderlake/Makefile.inc A src/soc/intel/alderlake/bootblock/cse_descriptor.c M src/soc/intel/alderlake/include/soc/bootblock.h 4 files changed, 91 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/63365/1 diff --git a/src/soc/intel/alderlake/Kconfig b/src/soc/intel/alderlake/Kconfig index eed157a..03b8160 100644 --- a/src/soc/intel/alderlake/Kconfig +++ b/src/soc/intel/alderlake/Kconfig @@ -112,6 +112,13 @@ select UDK_202005_BINDING select USE_FSP_NOTIFY_PHASE_POST_PCI_ENUM +config ALDERLAKE_CONFIGURE_CSE_DESCRIPTOR + bool + help + Select this if the CSE descriptor needs to be updated at runtime. This + can only be done if the descriptor region is writable, and should only + be used as a temporary workaround. + config ALDERLAKE_CAR_ENHANCED_NEM bool default y if !INTEL_CAR_NEM diff --git a/src/soc/intel/alderlake/Makefile.inc b/src/soc/intel/alderlake/Makefile.inc index d2c48b7..bd0bba6 100644 --- a/src/soc/intel/alderlake/Makefile.inc +++ b/src/soc/intel/alderlake/Makefile.inc @@ -16,6 +16,7 @@ bootblock-y += espi.c bootblock-y += gpio.c bootblock-y += p2sb.c +bootblock-$(CONFIG_ALDERLAKE_CONFIGURE_CSE_DESCRIPTOR) += bootblock/cse_descriptor.c romstage-y += espi.c romstage-y += gpio.c diff --git a/src/soc/intel/alderlake/bootblock/cse_descriptor.c b/src/soc/intel/alderlake/bootblock/cse_descriptor.c new file mode 100644 index 0000000..8b6550d --- /dev/null +++ b/src/soc/intel/alderlake/bootblock/cse_descriptor.c @@ -0,0 +1,82 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#include <arch/cpu.h> +#include <arch/mmio.h> +#include <cf9_reset.h> +#include <commonlib/region.h> +#include <console/console.h> +#include <cpu/intel/cpu_ids.h> +#include <fmap.h> +#include <intelblocks/pmclib.h> +#include <soc/bootblock.h> +#include <types.h> + +/* Flash Master 1 : HOST/BIOS */ +#define FLMSTR1 0x80 + +/* Flash signature Offset */ +#define FLASH_SIGN_OFFSET 0x10 +#define FLMSTR_WR_SHIFT_V2 20 +#define FLASH_VAL_SIGN 0xFF0A55A + +/* It checks whether host(Flash Master 1) has write access to the Descriptor Region or not */ +static int is_descriptor_writeable(uint8_t *desc) +{ + /* Check flash has valid signature */ + if (read32((void *)(desc + FLASH_SIGN_OFFSET)) != FLASH_VAL_SIGN) { + printk(BIOS_DEBUG, "Flash Descriptor is not valid\n"); + return 0; + } + + /* Check host has write access to the Descriptor Region */ + if (!((read32((void *)(desc + FLMSTR1)) >> FLMSTR_WR_SHIFT_V2) & BIT(0))) { + printk(BIOS_DEBUG, "Host doesn't have write access to Descriptor Region\n"); + return 0; + } + + return 1; +} + +void configure_cse_descriptor(size_t byte, uint8_t desired_value) { + uint8_t si_desc_buf[CONFIG_SI_DESC_REGION_SZ]; + struct region_device desc_rdev; + + if (fmap_locate_area_as_rdev_rw(CONFIG_SI_DESC_REGION, &desc_rdev) < 0) { + printk(BIOS_ERR, "Failed to locate %s in the FMAP\n", CONFIG_SI_DESC_REGION); + return; + } + + if (rdev_readat(&desc_rdev, si_desc_buf, 0, CONFIG_SI_DESC_REGION_SZ) != + CONFIG_SI_DESC_REGION_SZ) { + printk(BIOS_ERR, "Failed to read Descriptor Region from SPI Flash\n"); + return; + } + + if (!is_descriptor_writeable(si_desc_buf)) + return; + + printk(BIOS_DEBUG, "Current value of Descriptor byte 0x%lx: 0x%x\n", byte, si_desc_buf[byte]); + if (si_desc_buf[byte] == desired_value) { + printk(BIOS_DEBUG, "Update of Descriptor is not required!\n"); + return; + } + + si_desc_buf[byte] = desired_value; + + if (rdev_eraseat(&desc_rdev, 0, CONFIG_SI_DESC_REGION_SZ) != CONFIG_SI_DESC_REGION_SZ) { + printk(BIOS_ERR, "Failed to erase Descriptor Region area\n"); + return; + } + + if (rdev_writeat(&desc_rdev, si_desc_buf, 0, CONFIG_SI_DESC_REGION_SZ) + != CONFIG_SI_DESC_REGION_SZ) { + printk(BIOS_ERR, "Failed to update Descriptor Region\n"); + return; + } + + printk(BIOS_DEBUG, "Update of Descriptor successful, trigger GLOBAL RESET\n"); + + pmc_global_reset_enable(true); + do_full_reset(); + die("Failed to trigger GLOBAL RESET\n"); +} diff --git a/src/soc/intel/alderlake/include/soc/bootblock.h b/src/soc/intel/alderlake/include/soc/bootblock.h index e989bdd..54467fa 100644 --- a/src/soc/intel/alderlake/include/soc/bootblock.h +++ b/src/soc/intel/alderlake/include/soc/bootblock.h @@ -17,6 +17,6 @@ void pch_early_iorange_init(void); void report_platform_info(void); -void configure_pmc_descriptor(void); +void configure_cse_descriptor(size_t byte, uint8_t desired_value); #endif -- To view, visit https://review.coreboot.org/c/coreboot/+/63365 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I43c4d2888706561e42ff6b8ce0377eedbc38dbfe Gerrit-Change-Number: 63365 Gerrit-PatchSet: 1 Gerrit-Owner: Reka Norman <rekanorman(a)chromium.org> Gerrit-CC: Reka Norman <rekanorman(a)google.com> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: soc/intel/mtl: Do initial Meteor Lake SoC commit till bootblock
by Angel Pons (Code Review) April 5, 2022

April 5, 2022

Attention is currently required from: Felix Singer, Jamie Ryu, Subrata Banik, Ethan Tsao, Ravishankar Sarawadi, Raj Astekar. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62772 ) Change subject: soc/intel/mtl: Do initial Meteor Lake SoC commit till bootblock ...................................................................... Patch Set 7: (13 comments) File src/soc/intel/meteorlake/Kconfig: https://review.coreboot.org/c/coreboot/+/62772/comment/1600e492_f58f77b5 PS7, Line 56: ifd2 Why not "mtl" instead? ifdtool seems to know about it. File src/soc/intel/meteorlake/bootblock/pch.c: https://review.coreboot.org/c/coreboot/+/62772/comment/b79253fb_df72a49b PS7, Line 85: */ nit: add space before comment end File src/soc/intel/meteorlake/bootblock/report_platform.c: https://review.coreboot.org/c/coreboot/+/62772/comment/3031443f_128514a5 PS7, Line 27: MeteorLake Hmmm, the other entries spell "Meteorlake" with the 'l' in lowercase. Which form is preferred? File src/soc/intel/meteorlake/include/soc/espi.h: https://review.coreboot.org/c/coreboot/+/62772/comment/e1309db8_82f4db03 PS7, Line 17: */ nit: Add space before comment end https://review.coreboot.org/c/coreboot/+/62772/comment/ddc68bae_bba74706 PS7, Line 18: */ nit: Add space before comment end https://review.coreboot.org/c/coreboot/+/62772/comment/c84c95d7_cf2fe071 PS7, Line 24: #define PCCTL 0xE0 /* PCI Clock Control */ Does this register actually exist? It only seems to exist on platforms that have LPC. File src/soc/intel/meteorlake/include/soc/iomap.h: https://review.coreboot.org/c/coreboot/+/62772/comment/94b80dfd_b6298d82 PS7, Line 51: 4KB nit: Technically, 4 KiB https://review.coreboot.org/c/coreboot/+/62772/comment/8a36b854_becb3e82 PS7, Line 73: /* : #define PID_IOM 0xAA : #define IOM_BASE_ADDR (IOE_PCR_ABOVE_4G_BASE_ADDR + (PID_IOM << 16)) : */ Why is this commented out? File src/soc/intel/meteorlake/include/soc/pci_devs.h: https://review.coreboot.org/c/coreboot/+/62772/comment/1253b53b_f3786178 PS7, Line 11: pcidev_path_on_root_debug Could we please use `pcidev_path_on_root` instead? https://review.coreboot.org/c/coreboot/+/62772/comment/80fb56c3_2f982a9e PS7, Line 59: #define PCI_DEV_SLOT_GNA 0x08 : #define PCI_DEVFN_GNA _PCI_DEVFN(GNA, 0) : #define PCI_DEV_GNA _PCI_DEV(GNA, 0) nit: Move above `PCI_DEV_SLOT_TCSS` to keep ordering? https://review.coreboot.org/c/coreboot/+/62772/comment/81fc5dd3_4f792931 PS7, Line 159: #define PCI_DEV_SLOT_PCIE_2 0x6 : #define PCI_DEVFN_PCIE9 _PCI_DEVFN(PCIE_2, 0) : #define PCI_DEVFN_PCIE10 _PCI_DEVFN(PCIE_2, 1) : #define PCI_DEVFN_PCIE11 _PCI_DEVFN(PCIE_2, 2) : #define PCI_DEV_PCIE9 _PCI_DEV(PCIE_2, 0) : #define PCI_DEV_PCIE10 _PCI_DEV(PCIE_2, 1) : #define PCI_DEV_PCIE11 _PCI_DEV(PCIE_2, 2) : : #define PCI_DEV_SLOT_PCIE_3 0x1 : #define PCI_DEVFN_PCIE12 _PCI_DEVFN(PCIE_3, 0) : #define PCI_DEV_PCIE12 _PCI_DEV(PCIE_3, 0) I'd prefer to keep the macros ordered as per device/function numbers. Looking at ADL, I imagine these PCIe RPs are located on the "north" part of the SoC, whereas the PCIe RPs of device 0x1c are located on the "south" part of the SoC. File src/soc/intel/meteorlake/include/soc/pcr_ids.h: https://review.coreboot.org/c/coreboot/+/62772/comment/e88232d7_bca99dc2 PS7, Line 29: #define PID_IOM 0xAA : #define PID_TC_IOM 0xAA Is it intentional that these defines have the same value? File src/soc/intel/meteorlake/include/soc/pm.h: https://review.coreboot.org/c/coreboot/+/62772/comment/f2eb619b_42d588e0 PS7, Line 150: /* Get base address PMC memory mapped registers. */ : uint8_t *pmc_mmio_regs(void); : : /* Get base address of TCO I/O registers. */ : uint16_t smbus_tco_regs(void); : : /* Set the DISB after DRAM init */ : void pmc_set_disb(void); : : /* Clear PMCON status bits */ : void pmc_clear_pmcon_sts(void); : : /* STM Support */ : uint16_t get_pmbase(void); These are unused -- To view, visit https://review.coreboot.org/c/coreboot/+/62772 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I26479fcc3a3f9c6f8ebf5f198ab0809f0b4a2cc4 Gerrit-Change-Number: 62772 Gerrit-PatchSet: 7 Gerrit-Owner: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Ethan Tsao <ethan.tsao(a)intel.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Raj Astekar <raj.astekar(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: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.corp-partner.google.com> Gerrit-CC: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Ethan Tsao <ethan.tsao(a)intel.com> Gerrit-Attention: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Attention: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Attention: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.corp-partner.google.com> Gerrit-Comment-Date: Tue, 05 Apr 2022 07:32:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: amdfwtool: remove duplicates
by Kangheui Won (Code Review) April 5, 2022

April 5, 2022

Attention is currently required from: Bao Zheng, Angel Pons, Felix Held. Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59581 ) Change subject: amdfwtool: remove duplicates ...................................................................... Patch Set 1: (1 comment) File util/amdfwtool/data_parse.c: https://review.coreboot.org/c/coreboot/+/59581/comment/b488191e_2c56bafe PS1, Line 205: if (cb_config->use_secureos) { : fw_type = AMD_FW_PSP_TRUSTLETS; : subprog = 0; : } else { : fw_type = AMD_FW_SKIP; : } > Just to be sure, is this correct? The removed duplicate condition always used `AMD_FW_PSP_TRUSTLETS` […] To be honest there's no documentation so I don't know, need review from AMD. Practically use_secureos is enabled on stoney/picasso/cezanne/sabrina(afaik all platforms with PSP) so it won't make any difference. -- To view, visit https://review.coreboot.org/c/coreboot/+/59581 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3b86e24ba1c2769985e2864528e4dfcbf3cccc4b Gerrit-Change-Number: 59581 Gerrit-PatchSet: 1 Gerrit-Owner: Kangheui Won <khwon(a)chromium.org> Gerrit-Reviewer: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Reviewer: Bao Zheng <zheng.bao(a)amd.corp-partner.google.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Bao Zheng <zheng.bao(a)amd.corp-partner.google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Tue, 05 Apr 2022 07:22:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: intel/common/block: move gpmr api to gpmr driver
by Angel Pons (Code Review) April 5, 2022

April 5, 2022

Attention is currently required from: Jamie Ryu, Wonkyu Kim, Subrata Banik, Ethan Tsao, Ravishankar Sarawadi, Nick Vaccaro, Tim Wawrzynczak. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63170 ) Change subject: intel/common/block: move gpmr api to gpmr driver ...................................................................... Patch Set 9: Code-Review+1 (1 comment) File src/soc/intel/common/block/gpmr/Kconfig: https://review.coreboot.org/c/coreboot/+/63170/comment/c5a63f77_2411eeea PS9, Line 3: depends on SOC_INTEL_COMMON_BLOCK_DMI The commit message says that the "gpmr api is not DMI specific". I guess this dependency is here because the non-DMI code depends on CB:63198 -- To view, visit https://review.coreboot.org/c/coreboot/+/63170 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4d57f4b8bd06e0cf6c9afa4baf4a7bed64ecb56b Gerrit-Change-Number: 63170 Gerrit-PatchSet: 9 Gerrit-Owner: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Ethan Tsao <ethan.tsao(a)intel.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Ethan Tsao <ethan.tsao(a)intel.com> Gerrit-Attention: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Comment-Date: Tue, 05 Apr 2022 07:15:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/alderlake: Allow mainboard to configure USB2 Phy power gating
by Sridhar Siricilla (Code Review) April 5, 2022

April 5, 2022

Attention is currently required from: Subrata Banik, Tim Wawrzynczak, Paul Menzel, Angel Pons, Kane Chen. Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63293 ) Change subject: soc/intel/alderlake: Allow mainboard to configure USB2 Phy power gating ...................................................................... Patch Set 5: (4 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/63293/comment/2f378d56_7507f086 PS3, Line 11: informatrion > information Ack Patchset: PS2: > Can we at least add the Problem statement from the TA? Sure, I will add in other CL where . File src/soc/intel/alderlake/chip.h: https://review.coreboot.org/c/coreboot/+/63293/comment/41225d5e_4fad9972 PS4, Line 571: Enable > I'd use `Disable` instead, given the option's name Ack https://review.coreboot.org/c/coreboot/+/63293/comment/06f59cc7_d4f5e5c7 PS4, Line 572: Default 0. Set this to 1 in order to disable PCH USB2 Phy Power gating. > I feel this is redundant. […] I will add the comment in other CL. -- To view, visit https://review.coreboot.org/c/coreboot/+/63293 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3d80a3e36c6f8a3c0f174f955b11457752809f4d Gerrit-Change-Number: 63293 Gerrit-PatchSet: 5 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Kane Chen <kane.chen(a)intel.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.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)mailbox.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Kane Chen <kane.chen(a)intel.com> Gerrit-Comment-Date: Tue, 05 Apr 2022 06:38:08 +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: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/{brya, brask}: Disable PCH USB2 phy power gating
by Sridhar Siricilla (Code Review) April 5, 2022

April 5, 2022

Attention is currently required from: Subrata Banik, Paul Menzel, Tim Wawrzynczak. Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63294 ) Change subject: mb/google/{brya, brask}: Disable PCH USB2 phy power gating ...................................................................... Patch Set 5: (3 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/63294/comment/d23adc5e_1b8bff0f PS4, Line 9: The patch disables PCH USB2 Phy power gating. This change is required : to prevent possible display flicker issue. > Shorter: […] Ack https://review.coreboot.org/c/coreboot/+/63294/comment/6073c5fb_57d092ca PS4, Line 10: to prevent possible display flicker issue > “possible” means it was never reproduced in practice and is theoretical, or it happens sometimes, an […] Correct, theoretical possibility and we have noticed the issue from few boards! File src/mainboard/google/brya/variants/baseboard/brya/devicetree.cb: https://review.coreboot.org/c/coreboot/+/63294/comment/c4abefa8_b1dcf808 PS4, Line 25: # Disable PCH USB2 Phy power gating > I see all the other entries have comments, but in this case the variable names are self-describing, […] Ack -- To view, visit https://review.coreboot.org/c/coreboot/+/63294 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I25033ea218fa3154eb99af6be43c4198f4db3bcb Gerrit-Change-Number: 63294 Gerrit-PatchSet: 5 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-CC: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-CC: Kane Chen <kane.chen(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Comment-Date: Tue, 05 Apr 2022 06:37:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/ehl/fsp_params: Set Intel Speed Step (Eist) from devicetree
by Uwe Poeche (Code Review) April 5, 2022

April 5, 2022

Attention is currently required from: Paul Menzel, Werner Zeh. Uwe Poeche has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63352 ) Change subject: soc/intel/ehl/fsp_params: Set Intel Speed Step (Eist) from devicetree ...................................................................... Patch Set 3: (7 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/63352/comment/e5505c5d_350a4a46 PS1, Line 7: ehl/fsp_params.c > soc/intel/ehl/fsp_params Done https://review.coreboot.org/c/coreboot/+/63352/comment/6c123aed_9d4c1844 PS1, Line 7: setting for intel speed step from device tree > Set Intel Speed Step (Eist) from devicetree Done https://review.coreboot.org/c/coreboot/+/63352/comment/cb30f301_22b0cd3f PS1, Line 9: device tree > devicetree Done https://review.coreboot.org/c/coreboot/+/63352/comment/6ee54d55_99f91ab5 PS1, Line 10: fsps > FSPS Done https://review.coreboot.org/c/coreboot/+/63352/comment/443dfea0_d2a1138e PS1, Line 11: fsp > FSP Done https://review.coreboot.org/c/coreboot/+/63352/comment/38f94b15_3e3f8f9e PS1, Line 13: / > Remove Done File src/soc/intel/elkhartlake/fsp_params.c: https://review.coreboot.org/c/coreboot/+/63352/comment/b2dc418e_0dda81f1 PS1, Line 267: setting for intel speed step from device tree > Just use: Intel Speed Step? Done -- To view, visit https://review.coreboot.org/c/coreboot/+/63352 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0a5ef4968a27978116c21ce35b3818c6b36e086f Gerrit-Change-Number: 63352 Gerrit-PatchSet: 3 Gerrit-Owner: Uwe Poeche <uwe.poeche(a)siemens.com> Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.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: Tue, 05 Apr 2022 05:02:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/ehl/fsp_params: Set Intel Speed Step (Eist) from devicetree
by Uwe Poeche (Code Review) April 5, 2022

April 5, 2022

Attention is currently required from: Uwe Poeche, Werner Zeh. Hello build bot (Jenkins), Werner Zeh, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/63352 to look at the new patch set (#3). Change subject: soc/intel/ehl/fsp_params: Set Intel Speed Step (Eist) from devicetree ...................................................................... soc/intel/ehl/fsp_params: Set Intel Speed Step (Eist) from devicetree This patch provides the set value for intel speed step in devicetree for FSPS. Before that in case of not set value in device tree the default value of disabled was overwritten by default enabled of FSP. Test: mainboard/siemens/mc_ehl/variants/mc_ehl1 Check status of Bit 16 in MSR 0x1a0 after boot. Change-Id: I0a5ef4968a27978116c21ce35b3818c6b36e086f Signed-off-by: Uwe Poeche <uwe.poeche(a)siemens.com> --- M src/soc/intel/elkhartlake/fsp_params.c 1 file changed, 3 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/63352/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/63352 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0a5ef4968a27978116c21ce35b3818c6b36e086f Gerrit-Change-Number: 63352 Gerrit-PatchSet: 3 Gerrit-Owner: Uwe Poeche <uwe.poeche(a)siemens.com> Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Uwe Poeche <uwe.poeche(a)siemens.com> Gerrit-Attention: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-MessageType: newpatchset

1 0