coreboot-gerrit November 2021

coreboot-gerrit@coreboot.org

1 participants
6394 discussions

Change in coreboot[master]: mb/google/brya/var/felwinter: Correct USB3 TCSS setting
by Tim Wawrzynczak (Code Review) 17 Nov '21

17 Nov '21

Attention is currently required from: EricR Lai. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59385 ) Change subject: mb/google/brya/var/felwinter: Correct USB3 TCSS setting ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/59385/comment/66ad10e5_d91b2da2 PS3, Line 9: DAM `DMA` -- To view, visit https://review.coreboot.org/c/coreboot/+/59385 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I303d042d6c80194ff48130fe4e9c04b49ca13ee8 Gerrit-Change-Number: 59385 Gerrit-PatchSet: 3 Gerrit-Owner: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Comment-Date: Wed, 17 Nov 2021 15:53:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/brya/var/felwinter: Correct USB3 TCSS setting
by Tim Wawrzynczak (Code Review) 17 Nov '21

17 Nov '21

Attention is currently required from: EricR Lai. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59385 ) Change subject: mb/google/brya/var/felwinter: Correct USB3 TCSS setting ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/59385 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I303d042d6c80194ff48130fe4e9c04b49ca13ee8 Gerrit-Change-Number: 59385 Gerrit-PatchSet: 3 Gerrit-Owner: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Comment-Date: Wed, 17 Nov 2021 15:53:05 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/brya/var/redrix: Configure _DSC for CAM devices to ACPI_DEV...
by Tim Wawrzynczak (Code Review) 17 Nov '21

17 Nov '21

Attention is currently required from: Varshit B Pandya, Arec, Chen Wisley. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59260 ) Change subject: mb/google/brya/var/redrix: Configure _DSC for CAM devices to ACPI_DEVICE_SLEEP_D3_COLD ...................................................................... Patch Set 3: (1 comment) Patchset: PS2: > As far as I know, the camera sensor needs to read NVM data while camera is ON so how can we make sur […] doesn't the kernel driver make sure all of the devices are on D0 before they can be used? -- To view, visit https://review.coreboot.org/c/coreboot/+/59260 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I88ea1b87698c63e1bd69367ee857fba3f25c84ea Gerrit-Change-Number: 59260 Gerrit-PatchSet: 3 Gerrit-Owner: Varshit B Pandya <varshit.b.pandya(a)intel.com> Gerrit-Reviewer: Chen Wisley <wisley.chen(a)quantatw.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arec <arec.kao(a)intel.com> Gerrit-CC: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Varshit B Pandya <varshit.b.pandya(a)intel.com> Gerrit-Attention: Arec <arec.kao(a)intel.com> Gerrit-Attention: Chen Wisley <wisley.chen(a)quantatw.com> Gerrit-Comment-Date: Wed, 17 Nov 2021 15:52:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arec <arec.kao(a)intel.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: ec/starlabs: Add standardised ITE EC support
by Angel Pons (Code Review) 17 Nov '21

17 Nov '21

Attention is currently required from: Sean Rhodes, Andy Pont, Paul Menzel, Felix Held. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58343 ) Change subject: ec/starlabs: Add standardised ITE EC support ...................................................................... Patch Set 49: Code-Review+1 (3 comments) File src/ec/starlabs/merlin/Makefile.inc: https://review.coreboot.org/c/coreboot/+/58343/comment/af2d23ef_6ea1f381 PS42, Line 18: files_added:: warn_no_ite_fw : : PHONY+=warn_no_ite_fw : warn_no_ite_fw: : printf "\n\t** WARNING **\n" : printf "coreboot has been built without the ITE EC Firmware.\n" : printf "Do not flash this image. Your laptop's power button\n" : printf "may not respond when you press it.\n\n" > That's what it was meant to do - only CML and TGL need the EC bin, and if they are built without it, […] Ok, then we need another Kconfig option: `EC_STARLABS_NEED_ITE_BIN` or similar name: config EC_STARLABS_NEED_ITE_BIN bool depends on EC_STARLABS_ITE help Select if the mainboard requires EC firmware in the main flash chip. config EC_STARLABS_ADD_ITE_BIN bool "Add Star Labs EC binary file" default n depends on EC_STARLABS_NEED_ITE_BIN help Select to add an EC firmware binary into the coreboot image. EC firmware is necessary, flashing a coreboot image without EC firmware will render your laptop unusable. config EC_STARLABS_ITE_BIN_PATH string "Star Labs EC binary file path" depends on EC_STARLABS_ADD_ITE_BIN default "" The idea of `EC_STARLABS_NEED_ITE_BIN` is to only show the options to add the EC firmware for boards that actually need it. Mainboards which need EC firmware and whose EC firmware is available in the blobs repo would do the following: config BOARD_STARLABS_STARBOOK_TGL select EC_STARLABS_NEED_ITE_BIN config EC_STARLABS_ADD_ITE_BIN default y config EC_STARLABS_ITE_BIN_PATH default "3rdparty/blobs/mainboard/starlabs/..." This works because mainboard scope defaults take precedence over EC scope defaults (Mainboard Kconfig is sourced before EC Kconfig). Note that the user-visible `EC_STARLABS_ADD_ITE_BIN` option isn't selected: selects force-enable an option, and someone might want to build a coreboot image without the EC firmware for some reason (unusual, but no need to restrict the decision). File src/ec/starlabs/merlin/Makefile.inc: https://review.coreboot.org/c/coreboot/+/58343/comment/63840d1b_d9eb643a PS49, Line 18: CONFIG_EC_STARLABS_IT_BIN_PATH Hmmm, just realized this Kconfig option doesn't exist. I assumed this is a string option with a prompt (in case the user wants to use a different EC firmware). Even with the prompt, boards can still provide default paths to the files in the blobs repo. Anyway, see complete suggestion in the comment below. File src/ec/starlabs/merlin/ec.c: https://review.coreboot.org/c/coreboot/+/58343/comment/8f304397_6329b672 PS44, Line 47: static u16 ite_get_chip_id(unsigned int port) : { : return (pnp_read_index(port, ITE_CHIPID1) << 8) | : pnp_read_index(port, ITE_CHIPID2); : } : > Done Indeed, sorry if I wasn't clear about it -- To view, visit https://review.coreboot.org/c/coreboot/+/58343 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8023c26de23c874c84106fda96e64dcfa0c5ba32 Gerrit-Change-Number: 58343 Gerrit-PatchSet: 49 Gerrit-Owner: Sean Rhodes <admin(a)starlabs.systems> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Andy Pont <andy.pont(a)sdcsystems.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sean Rhodes <admin(a)starlabs.systems> Gerrit-Attention: Andy Pont <andy.pont(a)sdcsystems.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Wed, 17 Nov 2021 15:52:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Sean Rhodes <admin(a)starlabs.systems> Comment-In-Reply-To: Andy Pont <andy.pont(a)sdcsystems.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: driver/intel/mipi_camera: Add support for _DSC field
by Tim Wawrzynczak (Code Review) 17 Nov '21

17 Nov '21

Attention is currently required from: Varshit B Pandya, Paul Menzel, Patrick Rudolph, Felix Held. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58767 ) Change subject: driver/intel/mipi_camera: Add support for _DSC field ...................................................................... Patch Set 11: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/58767 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5471f144918413a2982f86beaf3dbf7e4e66cc9b Gerrit-Change-Number: 58767 Gerrit-PatchSet: 11 Gerrit-Owner: Varshit B Pandya <varshit.b.pandya(a)intel.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-CC: Selma Bensaid <selma.bensaid(a)intel.com> Gerrit-CC: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com> Gerrit-Attention: Varshit B Pandya <varshit.b.pandya(a)intel.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Wed, 17 Nov 2021 15:51:03 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: device/pci_device.c: Improve pci_bridge_route() readability
by Nico Huber (Code Review) 17 Nov '21

17 Nov '21

Attention is currently required from: Arthur Heymans, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59131 ) Change subject: device/pci_device.c: Improve pci_bridge_route() readability ...................................................................... Patch Set 2: (2 comments) File src/device/pci_device.c: https://review.coreboot.org/c/coreboot/+/59131/comment/812bb998_0b9aa37c PS1, Line 1337: u32 raw; : struct { : u8 primary; : u8 secondary; : u8 subordinate; : u8 _latency; : } __packed; > > This is not portable. […] Depends on the implementation of pci_write_config32(). IMHO, it should be aware of the endianness and then converting the word here locally would actually break things. Usually, I would recommend explicit serialization / de-serialization. But in this case don't we actually want to write 3 registers? Would it hurt to use pci_write_config8()? For readability, I'd simply use plain, simple variables, e.g. u8 primary, secondary, subordinate; if (state == PCI_ROUTE_CLOSE) { primary = 0x00; secondary = 0xff; subordinate = 0xfe; } else if (...) { ... } else { ... } Then, either serialize the results into a u32 in a single spot and use pci_write_config32(), or just use pci_write_config8() three times. File src/device/pci_device.c: https://review.coreboot.org/c/coreboot/+/59131/comment/2deae925_dccda98e PS2, Line 1351: : buses.primary = 0xff; : buses.secondary = 0xfe; Should be `secondary` and `subordinate`, right? -- To view, visit https://review.coreboot.org/c/coreboot/+/59131 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3bafd6a2e1d3a0b8d1d43997868a787ce3940ca9 Gerrit-Change-Number: 59131 Gerrit-PatchSet: 2 Gerrit-Owner: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Wed, 17 Nov 2021 15:49:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: fsp: Change post codes to lower case
by Angel Pons (Code Review) 17 Nov '21

17 Nov '21

Attention is currently required from: Sean Rhodes, Lee Leahy, Huang Jin, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59393 ) Change subject: fsp: Change post codes to lower case ...................................................................... Patch Set 1: Code-Review+2 (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/59393/comment/7a5e8bb7_6bf6993a PS1, Line 7: fsp: Change post codes to lower case How about: drivers/fsp: Rewrite post code hex values in lowercase -- To view, visit https://review.coreboot.org/c/coreboot/+/59393 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I65a83fcd69296f13c63329701ba9ce53f7cc2cb3 Gerrit-Change-Number: 59393 Gerrit-PatchSet: 1 Gerrit-Owner: Sean Rhodes <admin(a)starlabs.systems> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Huang Jin <huang.jin(a)intel.com> Gerrit-Reviewer: Lee Leahy <leroy.p.leahy(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sean Rhodes <admin(a)starlabs.systems> Gerrit-Attention: Lee Leahy <leroy.p.leahy(a)intel.com> Gerrit-Attention: Huang Jin <huang.jin(a)intel.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Wed, 17 Nov 2021 15:46:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: [RFC]cpu/x86/mp_init.c: Copy BSP chip_info to AP devices
by Angel Pons (Code Review) 17 Nov '21

17 Nov '21

Attention is currently required from: Arthur Heymans, Raul Rangel, Nico Huber. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58603 ) Change subject: [RFC]cpu/x86/mp_init.c: Copy BSP chip_info to AP devices ...................................................................... Patch Set 1: (1 comment) File src/cpu/x86/mp_init.c: https://review.coreboot.org/c/coreboot/+/58603/comment/8d4c06d8_636a1207 PS1, Line 392: new->chip_info = info->cpu->chip_info; > I was more wondering if you should check `new->chip_info == NULL` before overriding the value. I analyzed the cases where `new->chip_info` would be non-null, and concluded that it's very unusual and unlikely to happen anytime soon. I think adding the check is a good idea, and it wouldn't make a difference most (if not all) cases. -- To view, visit https://review.coreboot.org/c/coreboot/+/58603 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6b9b83b096da07821b7c1f9b36b3e311cd751718 Gerrit-Change-Number: 58603 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Wed, 17 Nov 2021 15:25:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/intel/adlrvp: Enable CPU PCIe RP 2
by Rizwan Qureshi (Code Review) 17 Nov '21

17 Nov '21

Attention is currently required from: Meera Ravindranath. Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59392 ) Change subject: mb/intel/adlrvp: Enable CPU PCIe RP 2 ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/59392/comment/9577de2f_a455d451 PS2, Line 9: commit:3fd39467b Fix S0ix regression what fixes the S0ix regression now? just the FIVR change? -- To view, visit https://review.coreboot.org/c/coreboot/+/59392 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0b8b76a5537d8b80777cb7588ce6b22281af7882 Gerrit-Change-Number: 59392 Gerrit-PatchSet: 2 Gerrit-Owner: Meera Ravindranath <meera.ravindranath(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Meera Ravindranath <meera.ravindranath(a)intel.com> Gerrit-Comment-Date: Wed, 17 Nov 2021 15:18:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/intel/adlrvp: Enable CPU PCIe RP 2
by Meera Ravindranath (Code Review) 17 Nov '21

17 Nov '21

Meera Ravindranath has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59392 ) Change subject: mb/intel/adlrvp: Enable CPU PCIe RP 2 ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/59392/comment/93149ac4_2521c89e PS1, Line 9: > please refer to the commit hash 3fd39467b and title that causes this regression ? Done -- To view, visit https://review.coreboot.org/c/coreboot/+/59392 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0b8b76a5537d8b80777cb7588ce6b22281af7882 Gerrit-Change-Number: 59392 Gerrit-PatchSet: 2 Gerrit-Owner: Meera Ravindranath <meera.ravindranath(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 17 Nov 2021 15:14:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subrata.banik(a)intel.com> Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit November 2021