coreboot-gerrit April 2022

coreboot-gerrit@coreboot.org

1 participants
5879 discussions

Change in coreboot[master]: mb/google/brya/var/agah: Add GPU power sequencing
by Angel Pons (Code Review) 05 Apr '22

05 Apr '22

Attention is currently required from: Subrata Banik, Tim Wawrzynczak, Nick Vaccaro. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62384 ) Change subject: mb/google/brya/var/agah: Add GPU power sequencing ...................................................................... Patch Set 3: (1 comment) File src/mainboard/google/brya/variants/agah/variant.c: https://review.coreboot.org/c/coreboot/+/62384/comment/8a147379_6ce855bb PS1, Line 104: void variant_finalize(void) > was called before, but it's fine, because the resources are saved into coreboot data structures Ack -- To view, visit https://review.coreboot.org/c/coreboot/+/62384 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1072be12ef58af5859e2a2d19c4a9c1adc0b0f88 Gerrit-Change-Number: 62384 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.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)mailbox.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Comment-Date: Tue, 05 Apr 2022 08:43:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: drivers/intel/fsp1_1/hob.c: Don't use broken FSP macros
by Patrick Rudolph (Code Review) 05 Apr '22

05 Apr '22

Attention is currently required from: Lee Leahy, Arthur Heymans. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63205 ) Change subject: drivers/intel/fsp1_1/hob.c: Don't use broken FSP macros ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: This should probably go into src/lib/hob.c together with unittests. -- To view, visit https://review.coreboot.org/c/coreboot/+/63205 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic5fbde9666be461232c2d6af181e72c6fbcf7425 Gerrit-Change-Number: 63205 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Lee Leahy <leroy.p.leahy(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Lee Leahy <leroy.p.leahy(a)intel.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Tue, 05 Apr 2022 08:41:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/alderlake: Add support to update descriptor at runtime
by Reka Norman (Code Review) 05 Apr '22

05 Apr '22

Attention is currently required from: Sam McNally, Reka Norman, Kangheui Won, Rizwan Qureshi, Tim Wawrzynczak, Sridhar Siricilla. Reka Norman has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63365 ) Change subject: soc/intel/alderlake: Add support to update descriptor at runtime ...................................................................... Patch Set 4: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/63365/comment/649277d4_36979085 PS2, Line 7: CSE > Reference to CSE is not required here since CSE and DESC are different regions in the flash. […] Done -- 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: 4 Gerrit-Owner: Reka Norman <rekanorman(a)chromium.org> Gerrit-Reviewer: Kangheui Won <khwon(a)chromium.org> Gerrit-Reviewer: Reka Norman <rekanorman(a)google.com> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Reka Norman <rekanorman(a)google.com> Gerrit-Attention: Kangheui Won <khwon(a)chromium.org> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Comment-Date: Tue, 05 Apr 2022 08:41:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/alderlake: Remove ALDERLAKE_A0_CONFIG_PMC_DESCRIPTOR Kconfig
by Reka Norman (Code Review) 05 Apr '22

05 Apr '22

Attention is currently required from: Subrata Banik, Tim Wawrzynczak, Sridhar Siricilla. Reka Norman has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63339 ) Change subject: soc/intel/alderlake: Remove ALDERLAKE_A0_CONFIG_PMC_DESCRIPTOR Kconfig ...................................................................... Patch Set 5: (1 comment) Patchset: PS5: Sorry, I accidentally rebased this when updating my change. -- To view, visit https://review.coreboot.org/c/coreboot/+/63339 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2a629353a4194a7505655346dcab4ef53059e0b7 Gerrit-Change-Number: 63339 Gerrit-PatchSet: 5 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(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: Reka Norman <rekanorman(a)chromium.org> Gerrit-CC: Reka Norman <rekanorman(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Comment-Date: Tue, 05 Apr 2022 08:39:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/alderlake: Add support for UFS controller
by Angel Pons (Code Review) 05 Apr '22

05 Apr '22

Attention is currently required from: Wonkyu Kim, Subrata Banik, Kangheui Won, Tim Wawrzynczak, Paul Menzel, Rizwan Qureshi, Meera Ravindranath, Lean Sheng Tan. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62856 ) Change subject: soc/intel/alderlake: Add support for UFS controller ...................................................................... Patch Set 2: (1 comment) File src/soc/intel/alderlake/fsp_params.c: https://review.coreboot.org/c/coreboot/+/62856/comment/2c9c263c_62b2104d PS2, Line 622: if (!dev) : s_cfg->UfsEnable[1] = 0; : else : s_cfg->UfsEnable[1] = dev->enabled; : > `s_cfg->UfsEnable[1] = is_dev_enabled(dev):` Or what I suggested in the other comment thread: /* UfsEnable[0] is unused on ADL-M/N/P */ s_cfg->UfsEnable[1] = is_devfn_enabled(PCH_DEVFN_UFS); -- To view, visit https://review.coreboot.org/c/coreboot/+/62856 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I92f024ded64e1eaef41a7807133361d74b5009d4 Gerrit-Change-Number: 62856 Gerrit-PatchSet: 2 Gerrit-Owner: Meera Ravindranath <meera.ravindranath(a)intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Kangheui Won <khwon(a)chromium.org> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Daniil Lunev <dlunev(a)chromium.org> Gerrit-CC: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Subrata Banik <subratabanik(a)google.com> Gerrit-CC: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Kangheui Won <khwon(a)chromium.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Meera Ravindranath <meera.ravindranath(a)intel.com> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Comment-Date: Tue, 05 Apr 2022 08:39:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: intel/common/block: move gpmr api to gpmr driver
by Subrata Banik (Code Review) 05 Apr '22

05 Apr '22

Attention is currently required from: Jamie Ryu, Wonkyu Kim, Ethan Tsao, Ravishankar Sarawadi, Nick Vaccaro, Tim Wawrzynczak. Subrata Banik 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: (1 comment) Patchset: PS9: At high-level, what you have done here is renaming the CB:47988 (soc/intel/common/dmi: Add DMI driver support) from DMI to GPMR. I don't understand the purpose of this renaming. The DMI sideband access is used for all GPMR read and write. Now as you have decided to rename DMI driver to GPMR then, take a look into the dependency over `SOC_INTEL_COMMON_BLOCK_DMI` > grep -rsn "SOC_INTEL_COMMON_BLOCK_DMI" src/1. src//soc/intel/common/pch/Kconfig:29: select SOC_INTEL_COMMON_BLOCK_DMI2. src//soc/intel/common/block/include/intelblocks/dmi.h:3:#ifndef SOC_INTEL_COMMON_BLOCK_DMI_H3. src//soc/intel/common/block/include/intelblocks/dmi.h:4:#define SOC_INTEL_COMMON_BLOCK_DMI_H4. src//soc/intel/common/block/include/intelblocks/dmi.h:20:#endif /* SOC_INTEL_COMMON_BLOCK_DMI_H */5. src//soc/intel/common/block/lpc/Kconfig:17: depends on SOC_INTEL_COMMON_BLOCK_DMI6. src//soc/intel/common/block/dmi/Kconfig:1:config SOC_INTEL_COMMON_BLOCK_DMI7. src//soc/intel/common/block/dmi/Makefile.inc:1:ifeq ($(CONFIG_SOC_INTEL_COMMON_BLOCK_DMI), y) This changes already dropped #7, which compiled the `dmi.c` file for other IA common code for consumption. When you have introduced `SOC_INTEL_COMMON_BLOCK_GPMR` then why still have dependency over a Kconfig (https://review.coreboot.org/c/coreboot/+/63170/9/src/soc/intel/common/block…) which have zero value with this code change. What I understood, is the dependency that you like to keep here (https://review.coreboot.org/c/coreboot/+/63170/9/src/soc/intel/common/block…) is SOC_INTEL_COMMON_BLOCK_PCR because you are accessing DMI over sideband hence PCR library is needed. Ideally #1 and #6 also should get dropped. #5 dependency would not revised to SOC_INTEL_COMMON_BLOCK_GPMR instead SOC_INTEL_COMMON_BLOCK_DMI A better representation of CB:63170 IMO would be as below. 1. Add GPMR common driver in IA common code. 2. Migrate all DMI API usage to GPMR -- that includes changes in #5 above, https://review.coreboot.org/c/coreboot/+/63170/9/src/soc/intel/common/block… etc. 3. Drop DMI driver from IA common code -- that includes #1 as above -- 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: 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 08:39:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common: implement ioc driver
by Subrata Banik (Code Review) 05 Apr '22

05 Apr '22

Attention is currently required from: Jamie Ryu, Wonkyu Kim, Ethan Tsao, Ravishankar Sarawadi, Tim Wawrzynczak, Paul Menzel, Nick Vaccaro. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63198 ) Change subject: soc/intel/common: implement ioc driver ...................................................................... Patch Set 5: (17 comments) File src/soc/intel/common/block/gpmr/Kconfig: https://review.coreboot.org/c/coreboot/+/63198/comment/c654ad16_687627e8 PS5, Line 3: SOC_INTEL_COMMON_BLOCK_DMI || SOC_INTEL_COMMON_BLOCK_IOC how do you plan to restrict a user from selecting both Kconfig. Prior to MTL, the GPMR is configurable using PCR read/write and from MTL, it's using IOC mmio read/write. Check this, https://review.coreboot.org/c/coreboot/+/59804/6/src/soc/intel/alderlake/in… you can also apply the same logic in gpmr.c file as well. https://review.coreboot.org/c/coreboot/+/63198/comment/9295571b_85fb1375 PS5, Line 5: DMI I guess you are talking about access mechanism rather interface here hence, PCR might be more applicable File src/soc/intel/common/block/gpmr/gpmr.c: https://review.coreboot.org/c/coreboot/+/63198/comment/969c24c5_003f4026 PS5, Line 11: if (CONFIG(SOC_INTEL_COMMON_BLOCK_IOC)) : return ioc_reg_read32(offset); : else : return pcr_read32(PID_DMI, offset); nit: you can implement as https://review.coreboot.org/c/coreboot/+/62723 to abstract the common code with SoC specific function. File src/soc/intel/common/block/include/intelblocks/gpmr.h: https://review.coreboot.org/c/coreboot/+/63198/comment/70eb4b1e_a2348882 PS5, Line 12: #define MAX_GPMR_REGS IOC_MAX_GPMR_REGS : #define GPMR_OFFSET IOC_GPMR_OFFSET : #define GPMR_LIMIT_MASK IOC_GPMR_LIMIT_MASK : #define GPMR_BASE_SHIFT IOC_GPMR_BASE_SHIFT : #define GPMR_BASE_MASK IOC_GPMR_BASE_MASK : #define GPMR_DID_OFFSET IOC_GPMR_DID_OFFSET : #define GPMR_EN IOC_GPMR_EN suggestion: keep two .h file as ioc_gpmr.h and pcr_gpmr.h and pick the one based on CONFIG(SOC_INTEL_COMMON_BLOCK_IOC) File src/soc/intel/common/block/include/intelblocks/ioc.h: https://review.coreboot.org/c/coreboot/+/63198/comment/95530b59_df96db89 PS5, Line 10: <soc/ioc_reg.h> I don't prefer override like UEFI does, it creates confusion while debugging, if you like to refer few macros from SoC, please have it defined there, in that case, any SoC that selects IOC driver in future and missed to implement the correct macros as needed, would run int compilation error https://review.coreboot.org/c/coreboot/+/63198/comment/15ff2d97_ddae2380 PS5, Line 15: : void ioc_reg_write32(uint32_t offset, uint32_t value); : uint32_t ioc_reg_read32(uint32_t offset); outside gpmr.c, is there any other consumer of these APIs ? File src/soc/intel/common/block/ioc/Kconfig: https://review.coreboot.org/c/coreboot/+/63198/comment/85595a37_5af8c5f8 PS5, Line 1: SOC_INTEL_COMMON_BLOCK_IOC you are depending on MCH_BASE_ADDRESS being programmed for IOC read/write, hence, required driver is needed. File src/soc/intel/common/block/ioc/ioc.c: https://review.coreboot.org/c/coreboot/+/63198/comment/53c74b4a_c62d6b68 PS5, Line 9: write32 use `write32p` so, you don't need (void *) casting https://review.coreboot.org/c/coreboot/+/63198/comment/67c5563a_7c660b0c PS5, Line 9: MCH_BASE_ADDRESS worth checking MCHBAR bit 0 if memory space is enabled ? https://review.coreboot.org/c/coreboot/+/63198/comment/55f70868_9b307327 PS5, Line 15: read32 read32p File src/soc/intel/common/block/lpc/lpc_lib.c: https://review.coreboot.org/c/coreboot/+/63198/comment/17300b22_8239e963 PS5, Line 28: if (CONFIG(SOC_INTEL_COMMON_BLOCK_IOC)) : ioc_reg_write32(R_IOC_CFG_LPCIOE, io_enables); : else if (CONFIG(SOC_INTEL_COMMON_BLOCK_LPC_MIRROR_TO_DMI)) : pcr_write16(PID_DMI, PCR_DMI_LPCIOE, io_enables); why not `gpmr_write32`, i guess it's possible to unify based on register name. https://review.coreboot.org/c/coreboot/+/63198/comment/4c318f15_303f0d40 PS5, Line 51: pcr_write16(PID_DMI, PCR_DMI_LPCIOD, io_ranges); same as above https://review.coreboot.org/c/coreboot/+/63198/comment/c6332945_f80d9f46 PS5, Line 125: same as above https://review.coreboot.org/c/coreboot/+/63198/comment/810aa5f1_900fbe10 PS5, Line 161: pcr_write32(PID_DMI, PCR_DMI_LPCGMR, lgmr); same as above https://review.coreboot.org/c/coreboot/+/63198/comment/0f0cedd0_e07bfd65 PS5, Line 265: } same as above File src/soc/intel/common/block/smbus/tco.c: https://review.coreboot.org/c/coreboot/+/63198/comment/e50b731b_e25f938d PS5, Line 130: pcr_write32(PID_DMI, PCR_DMI_TCOBASE, tcobase | TCOEN); use gpmr_write32 File src/soc/intel/common/pch/lockdown/lockdown.c: https://review.coreboot.org/c/coreboot/+/63198/comment/b2c942ff_05c76fae PS5, Line 53: } you can create gmpr read_or8/32 function. -- To view, visit https://review.coreboot.org/c/coreboot/+/63198 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I768027c2ca78310c03845f70f17df19dc8cd0982 Gerrit-Change-Number: 63198 Gerrit-PatchSet: 5 Gerrit-Owner: Wonkyu Kim <wonkyu.kim(a)intel.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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Attention: Ethan Tsao <ethan.tsao(a)intel.com> Gerrit-Attention: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Comment-Date: Tue, 05 Apr 2022 08:38:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common: Abstract the common TCSS functions
by Subrata Banik (Code Review) 05 Apr '22

05 Apr '22

Attention is currently required from: John Zhao, Tim Wawrzynczak, Paul Menzel, Angel Pons, Patrick Rudolph, EricR Lai. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62723 ) Change subject: soc/intel/common: Abstract the common TCSS functions ...................................................................... Patch Set 15: Code-Review+1 (1 comment) Patchset: PS15: will wait for Tim to review once more. -- To view, visit https://review.coreboot.org/c/coreboot/+/62723 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3302aabfb5f540c41da6359f11376b4202c6310b Gerrit-Change-Number: 62723 Gerrit-PatchSet: 15 Gerrit-Owner: John Zhao <john.zhao(a)intel.com> Gerrit-Reviewer: Angel Pons <angel.pons(a)9elements.com> Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> 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)mailbox.org> Gerrit-CC: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Attention: John Zhao <john.zhao(a)intel.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <angel.pons(a)9elements.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Comment-Date: Tue, 05 Apr 2022 08:34:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/alderlake: Remove ALDERLAKE_A0_CONFIG_PMC_DESCRIPTOR Kconfig
by Reka Norman (Code Review) 05 Apr '22

05 Apr '22

Attention is currently required from: Subrata Banik, Reka Norman, Tim Wawrzynczak. Reka Norman has uploaded a new patch set (#5) to the change originally created by Sridhar Siricilla. ( https://review.coreboot.org/c/coreboot/+/63339 ) Change subject: soc/intel/alderlake: Remove ALDERLAKE_A0_CONFIG_PMC_DESCRIPTOR Kconfig ...................................................................... soc/intel/alderlake: Remove ALDERLAKE_A0_CONFIG_PMC_DESCRIPTOR Kconfig The patch removes Kconfig CONFIG_ALDERLAKE_A0_CONFIGURE_PMC_DESCRIPTOR code which updates PMC descriptor for an intermediate ADL-P SoC stepping. Since intermediate ADL-P SoC is no longer supported and no board is selecting the Kconfig, so removing the code that updates PMC descriptor. TEST=Build and boot Gimble board Signed-off-by: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Change-Id: I2a629353a4194a7505655346dcab4ef53059e0b7 --- M src/soc/intel/alderlake/Kconfig M src/soc/intel/alderlake/Makefile.inc M src/soc/intel/alderlake/bootblock/bootblock.c D src/soc/intel/alderlake/bootblock/pmc_descriptor.c 4 files changed, 0 insertions(+), 101 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/63339/5 -- To view, visit https://review.coreboot.org/c/coreboot/+/63339 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2a629353a4194a7505655346dcab4ef53059e0b7 Gerrit-Change-Number: 63339 Gerrit-PatchSet: 5 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(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: Reka Norman <rekanorman(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Reka Norman <rekanorman(a)chromium.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: soc/intel/mtl: Do initial Meteor Lake SoC commit till ramstage
by Angel Pons (Code Review) 05 Apr '22

05 Apr '22

Attention is currently required from: Wonkyu Kim, Ravishankar Sarawadi, Raj Astekar. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63364 ) Change subject: soc/intel/mtl: Do initial Meteor Lake SoC commit till ramstage ...................................................................... Patch Set 2: (25 comments) File src/soc/intel/meteorlake/Kconfig: https://review.coreboot.org/c/coreboot/+/63364/comment/41244bff_3b943dc7 PS2, Line 37: select REG_SCRIPT Shouldn't be needed https://review.coreboot.org/c/coreboot/+/63364/comment/e7913c0a_76a85731 PS2, Line 73: select UDK_202005_BINDING : select DISPLAY_FSP_VERSION_INFO : select HECI_DISABLE_USING_SMM : # select DEBUG_GPIO nit: Order these alphabetically as well? File src/soc/intel/meteorlake/acpi.c: https://review.coreboot.org/c/coreboot/+/63364/comment/86d9e634_0131aff2 PS2, Line 134: memcpy(&map[i], &cstate_map[set[i]], sizeof(acpi_cstate_t)); map[i] = cstate_map[set[i]]; File src/soc/intel/meteorlake/chip.c: https://review.coreboot.org/c/coreboot/+/63364/comment/6589e7f4_6337f957 PS2, Line 21: const char *soc_acpi_name(const struct device *dev) Not all devices are listed here, is it intentional? File src/soc/intel/meteorlake/chipset.cb: https://review.coreboot.org/c/coreboot/+/63364/comment/545755f2_aec4d790 PS2, Line 75: device pci 13.0 alias p2sb2 off end : device pci 13.2 alias pmc2 hidden end : device pci 13.3 alias shared_sram2 off end Given that these devices are for IOE, I'd name them as follows: device pci 13.0 alias ioe_p2sb off end device pci 13.2 alias ioe_pmc hidden end device pci 13.3 alias ioe_shared_sram off end File src/soc/intel/meteorlake/cpu.c: https://review.coreboot.org/c/coreboot/+/63364/comment/3400e52b_6e1b68e9 PS2, Line 171: /* : * enable_x2apic () is temporary WA for fixing S3 resume hang issue : * Once formal patch merge, this WA can be removed: : * formal patch: https://review.coreboot.org/c/coreboot/+/55262 : */ It's merged already? File src/soc/intel/meteorlake/crashlog.c: https://review.coreboot.org/c/coreboot/+/63364/comment/0382a1a7_a3098213 PS2, Line 3: UPDATEME What needs to be updated? File src/soc/intel/meteorlake/finalize.c: https://review.coreboot.org/c/coreboot/+/63364/comment/4016622a_290898f7 PS2, Line 23: static void pch_handle_sideband(config_t *config) : { : : } Is this needed? https://review.coreboot.org/c/coreboot/+/63364/comment/168850f2_1ef816bf PS2, Line 44: if (config->PmTimerDisabled) : pmc_disable_acpi_timer(); This seems to have been removed from other platforms. File src/soc/intel/meteorlake/fsp_params.c: https://review.coreboot.org/c/coreboot/+/63364/comment/c58764f9_b4fc5e62 PS2, Line 173: /* Chipset Lockdown */ : if (get_lockdown_config() == CHIPSET_LOCKDOWN_COREBOOT) { : s_cfg->PchLockDownGlobalSmi = 0; : s_cfg->PchLockDownBiosInterface = 0; : s_cfg->PchUnlockGpioPads = 1; : s_cfg->RtcMemoryLock = 0; : } else { : s_cfg->PchLockDownGlobalSmi = 1; : s_cfg->PchLockDownBiosInterface = 1; : s_cfg->PchUnlockGpioPads = 0; : s_cfg->RtcMemoryLock = 1; : } This can be simplified, see ADL code https://review.coreboot.org/c/coreboot/+/63364/comment/08b5e616_d4a0c82f PS2, Line 285: nit: double blank line https://review.coreboot.org/c/coreboot/+/63364/comment/1e27b252_370f1ce4 PS2, Line 323: nit: double blank line https://review.coreboot.org/c/coreboot/+/63364/comment/4ad42c99_d9a3d62d PS2, Line 417: nit: double blank line https://review.coreboot.org/c/coreboot/+/63364/comment/a096d067_121ab6a4 PS2, Line 426: nit: double blank line File src/soc/intel/meteorlake/include/soc/crashlog.h: https://review.coreboot.org/c/coreboot/+/63364/comment/ba213f60_0e4a63e7 PS2, Line 8: nit: double blank line File src/soc/intel/meteorlake/include/soc/meminit.h: PS2: Shouldn't this be part of the romstage commit? File src/soc/intel/meteorlake/pcie_rp.c: https://review.coreboot.org/c/coreboot/+/63364/comment/fde0e1c2_35a282f1 PS2, Line 11: { .slot = PCI_DEV_SLOT_PCIE_2, .count = 1 }, I'm pretty sure root port function swap only applies to PCIE_1. https://review.coreboot.org/c/coreboot/+/63364/comment/9b6af33e_0356ee3e PS2, Line 17: { .slot = PCI_DEV_SLOT_PCIE_2, .count = 3 }, : { .slot = PCI_DEV_SLOT_PCIE_3, .count = 1 }, Ditto. If PCIE_2 works as it does on older platforms, the multiple functions come from bifurcation (x16, x8 + x8, x8 + x4 + x4) and function 0 always exists. File src/soc/intel/meteorlake/pmc.c: https://review.coreboot.org/c/coreboot/+/63364/comment/8ea5f997_7cdc9e12 PS2, Line 104: Alder Hmmmmmmmmmm... https://review.coreboot.org/c/coreboot/+/63364/comment/6b4b0075_c221f5e0 PS2, Line 122: if (CONFIG(SOC_INTEL_COMMON_BLOCK_ACPI_PEP)) { Indentation looks off, it should be one tab. File src/soc/intel/meteorlake/pmutil.c: https://review.coreboot.org/c/coreboot/+/63364/comment/f1ec1bfa_fbb6c2e5 PS2, Line 210: * S5 because the PCH does not set the WAK_STS bit when waking : * from a true G3 state. : */ nit: add one space before the asterisks File src/soc/intel/meteorlake/smihandler.c: https://review.coreboot.org/c/coreboot/+/63364/comment/25cc6366_a300f154 PS2, Line 14: ADP Hmmmmmmmmm... File src/soc/intel/meteorlake/soc_info.c: https://review.coreboot.org/c/coreboot/+/63364/comment/77bea7fa_1f18ce46 PS2, Line 14: #ifdef __SIMPLE_DEVICE__ : did = pci_read_config16(PCI_DEVFN_ROOT, PCI_DEVICE_ID); : #else : struct device *dev; : dev = pcidev_path_on_root(PCI_DEVFN_ROOT); : did = pci_read_config16(dev, PCI_DEVICE_ID); : #endif Instead of doing this, why not simply #define __SIMPLE_DEVICE__ at the start of the file (before the includes)? Then, you can simply do: did = pci_read_config16(PCI_DEVFN_ROOT, PCI_DEVICE_ID); https://review.coreboot.org/c/coreboot/+/63364/comment/3f26730a_88325d1f PS2, Line 26: Trailing whitespace File src/soc/intel/meteorlake/systemagent.c: https://review.coreboot.org/c/coreboot/+/63364/comment/fb05fb72_12e422c7 PS2, Line 91: uint32_t soc_systemagent_max_chan_capacity_mib(u8 capid0_a_ddrsz) Does this still apply? -- To view, visit https://review.coreboot.org/c/coreboot/+/63364 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie746c0bfcf1f315a4ab6f540cc7c4933157551d7 Gerrit-Change-Number: 63364 Gerrit-PatchSet: 2 Gerrit-Owner: Ravishankar Sarawadi <ravishankar.sarawadi(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-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Attention: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Attention: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Comment-Date: Tue, 05 Apr 2022 08:26:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit April 2022