coreboot-gerrit December 2024

coreboot-gerrit@coreboot.org

1 participants
3018 discussions

[XL] Change in coreboot[main]: drivers/intel/touch: Add driver for Intel Touch Controller and Devices
by Jérémy Compostella (Code Review) 17 Dec '24

17 Dec '24

1 0

[XL] Change in coreboot[main]: drivers/intel/touch: Add driver for Intel Touch Controller and Devices
by Jérémy Compostella (Code Review) 17 Dec '24

17 Dec '24

Attention is currently required from: Bora Guvendik, Cliff Huang, Kyoung Il Kim, Subrata Banik. Jérémy Compostella has posted comments on this change by Cliff Huang. ( https://review.coreboot.org/c/coreboot/+/85198?usp=email ) Change subject: drivers/intel/touch: Add driver for Intel Touch Controller and Devices ...................................................................... Patch Set 4: (1 comment) File src/drivers/intel/touch/touch.c: https://review.coreboot.org/c/coreboot/+/85198/comment/cab62350_30715d61?us… : PS2, Line 28: truct thc_hidi2c_info { : /* device I2C address */ : uint16_t dev_addr; : /* device connection speed */ : uint32_t connection_speed; : /* device address mode */ : uint8_t addr_mode; : /* Standard Mode (100 kbit/s) Serial Clock Line HIGH Period */ : uint32_t sm_scl_high_period; : /* Standard Mode (100 kbit/s) Serial Clock Line LOW Period */ : uint32_t sm_scl_low_period; : /* Standard Mode (100 kbit/s) Serial Data Line Transmit Hold Period*/ : uint32_t sm_sda_hold_tx_period; : /* Standard Mode (100 kbit/s) Serial Data Receive Hold Period */ : uint32_t sm_sda_hold_rx_period; : /* Fast Mode (400 kbit/s) Serial Clock Line HIGH Period */ : uint32_t fm_scl_high_period; : /* Fast Mode (400 kbit/s) Serial Clock Line LOW Period */ : uint32_t fm_scl_low_period; : /* Fast Mode (400 kbit/s) Serial Data Line Transmit Hold Period */ : uint32_t fm_sda_hold_tx_period; : /* Fast Mode (400 kbit/s) Serial Data Line Receive Hold Period */ : uint32_t fm_sda_hold_rx_period; : /* Maximum length (in ic_clk_cycles) of suppressed spikes in Std Mode, Fast : Mode, and Fast Mode Plus. */ : uint32_t suppressed_spikes_s_f_fp; : /* Fast Mode Plus (1Mbit/sec) Serial Clock Line HIGH Period */ : uint32_t fmp_scl_high_period; : /* Fast Mode Plus (1Mbit/sec) Serial Clock Line LOW Period */ : uint32_t fmp_scl_low_period; : /* Fast Mode Plus (1Mbit/sec) Serial Data Line Transmit HOLD Period */ : uint32_t fmp_sda_hold_tx_period; : /* Fast Mode Plus (1Mbit/sec) Serial Data Line Receive HOLD Period */ : uint32_t fmp_sda_hold_rx_period; : /* High Speed Mode Plus (3.4Mbits/sec) Serial Clock Line HIGH Period */ : uint32_t hm_scl_high_period; : /* High Speed Mode Plus (3.4Mbits/sec) Serial Clock Line LOW Period */ : uint32_t hm_scl_low_period; : /* High Speed Mode Plus (3.4Mbits/sec) Serial Data Line Transmit HOLD Period */ : uint32_t hm_sda_hold_tx_period; : /* High Speed Mode Plus (3.4Mbits/sec) Serial Data Line Receive HOLD Period */ : uint32_t hm_sda_hold_rx_period; : /* Maximum length (in ic_clk_cycles) of suppressed spikes in High Speed Mode */ : uint32_t suppressed_spikes_h_fp; > It looks like this data structure could be a simple sub data structure of `struct drivers_intel_touc […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/85198?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ibcd2a75a41460dee67aebdc61ee9e85fa98b71bf Gerrit-Change-Number: 85198 Gerrit-PatchSet: 4 Gerrit-Owner: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Kyoung Il Kim <kyoung.il.kim(a)intel.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Kyoung Il Kim <kyoung.il.kim(a)intel.com> Gerrit-Comment-Date: Tue, 17 Dec 2024 22:37:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com>

1 0

[S] Change in coreboot[main]: soc/intel/pantherlake: Update Touch Controller UPD params
by Jérémy Compostella (Code Review) 17 Dec '24

17 Dec '24

Attention is currently required from: Bora Guvendik, Cliff Huang, Jérémy Compostella, Kapil Porwal, Kyoung Il Kim, Pranava Y N, Subrata Banik. Jérémy Compostella has uploaded a new patch set (#6) to the change originally created by Cliff Huang. ( https://review.coreboot.org/c/coreboot/+/85199?usp=email ) Change subject: soc/intel/pantherlake: Update Touch Controller UPD params ...................................................................... soc/intel/pantherlake: Update Touch Controller UPD params Configure ThcMode and ThcWakeOnTouch UPDs according to the SoC chip configuration from the devicetree. BUG=none TEST=TBD Signed-off-by: Cliff Huang <cliff.huang(a)intel.com> Change-Id: I15fb62eaadc03b9a17e94609b97c686518150e2e --- M src/soc/intel/pantherlake/chip.h M src/soc/intel/pantherlake/fsp_params.c 2 files changed, 28 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/85199/6 -- To view, visit https://review.coreboot.org/c/coreboot/+/85199?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I15fb62eaadc03b9a17e94609b97c686518150e2e Gerrit-Change-Number: 85199 Gerrit-PatchSet: 6 Gerrit-Owner: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Kyoung Il Kim <kyoung.il.kim(a)intel.com> Gerrit-Reviewer: Pranava Y N <pranavayn(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Kyoung Il Kim <kyoung.il.kim(a)intel.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Pranava Y N <pranavayn(a)google.com>

1 0

[XL] Change in coreboot[main]: drivers/intel/touch: Add driver for Intel Touch Controller and Devices
by Jérémy Compostella (Code Review) 17 Dec '24

17 Dec '24

Attention is currently required from: Bora Guvendik, Jérémy Compostella, Kyoung Il Kim, Subrata Banik. Jérémy Compostella has uploaded a new patch set (#4) to the change originally created by Cliff Huang. ( https://review.coreboot.org/c/coreboot/+/85198?usp=email ) The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: drivers/intel/touch: Add driver for Intel Touch Controller and Devices ...................................................................... drivers/intel/touch: Add driver for Intel Touch Controller and Devices This driver is for Intel Touch Host controller (THC). THC supports a host controller interface to the touch IC for high bandwidth touch data transfer from SPI based touch ICs. This driver supports WACOM, ELAN HYNITRON, and generic touch sensor devices. - Change MTL_DID define names Modified MTL THC DIDs according to: 81330/comment/b7f5bc9d_a34d6a77 When touch controller is configured as THC-SPI mode, DID is 0x7e49 for THC0, and 0x7e4b for THC1. 0x7e48 and 0x7e4a are the DIDs when ThcMode is 0 (Default:Intel) for THC0 and THC1 respectively. Refer MTL EDS vol 1: 640228. - Add PTL DIDs When touch controller is configured as THC-I2C mode, DID is 0xe448 for THC0, and 0xe44a for THC1. 0xe449 and 0xe44b are the DIDs when ThcMode is THC-SPI for THC0 and THC1 respectively. ref: PTL EDS vol 1: 815002 - THC ACPI: This driver generates the following ACPI objects: - _DSM - _CRS - _DSD (for THC-I2C only) - Power resource with _STA, _ON, and _OFF - _RST (for THC-SPI only) supported devices: WACOM: THC-SPI touchscreen only ELAN: both THC-SPI and THC-I2C touchscreen HYNITRON: THC-I2C touchpad only BUG=none Signed-off-by: Cliff Huang <cliff.huang(a)intel.com> Change-Id: Ibcd2a75a41460dee67aebdc61ee9e85fa98b71bf --- A src/drivers/intel/touch/Kconfig A src/drivers/intel/touch/Makefile.mk A src/drivers/intel/touch/chip.h A src/drivers/intel/touch/elan.h A src/drivers/intel/touch/hynitron.h A src/drivers/intel/touch/touch.c A src/drivers/intel/touch/wacom.h M src/include/device/pci_ids.h 8 files changed, 1,069 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/85198/4 -- To view, visit https://review.coreboot.org/c/coreboot/+/85198?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ibcd2a75a41460dee67aebdc61ee9e85fa98b71bf Gerrit-Change-Number: 85198 Gerrit-PatchSet: 4 Gerrit-Owner: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Kyoung Il Kim <kyoung.il.kim(a)intel.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Kyoung Il Kim <kyoung.il.kim(a)intel.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>

1 0

[XS] Change in coreboot[main]: mb/google/brox: Include CSE reset in mainboard reset expectation
by Karthik Ramasubramanian (Code Review) 17 Dec '24

17 Dec '24

Attention is currently required from: Nick Vaccaro, Ren Kuo, Subrata Banik. Hello Nick Vaccaro, Ren Kuo, Subrata Banik, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/85642?usp=email to look at the new patch set (#2). Change subject: mb/google/brox: Include CSE reset in mainboard reset expectation ...................................................................... mb/google/brox: Include CSE reset in mainboard reset expectation If CSE is in RO, then a reset is expected for CSE to jump to RW. Include that reset in mainboard_expects_another_reset() logic. This will avoid unnecessary warm reset during regular boot flow in boards with non-UFS storage. BUG=None TEST=Build Brox BIOS image and boot to OS. Ensure that redundant reset to disable UFS controller is avoided. Before this change: [INFO ] Disabling UFS controllers [INFO ] Warm Reset after disabling UFS controllers [INFO ] system_reset() called! <snip> [DEBUG] HECI: Global Reset(Type:1) Command <snip> [INFO ] Disabling UFS controllers [INFO ] Warm Reset after disabling UFS controllers [INFO ] system_reset() called! After this change: [DEBUG] HECI: Global Reset(Type:1) Command <snip> [INFO ] Disabling UFS controllers [INFO ] Warm Reset after disabling UFS controllers [INFO ] system_reset() called! Change-Id: I80a46b15813b6bdfa6c029c54590f4b7c2a6754b Signed-off-by: Karthikeyan Ramasubramanian <kramasub(a)google.com> --- M src/mainboard/google/brox/variants/baseboard/brox/romstage.c 1 file changed, 6 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/85642/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/85642?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I80a46b15813b6bdfa6c029c54590f4b7c2a6754b Gerrit-Change-Number: 85642 Gerrit-PatchSet: 2 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: Ren Kuo <ren.kuo(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Ren Kuo <ren.kuo(a)quanta.corp-partner.google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>

1 0

[M] Change in coreboot[main]: soc/amd/glinda/cpu: Update cache info
by Jérémy Compostella (Code Review) 17 Dec '24

17 Dec '24

Attention is currently required from: Naresh Solanki. Jérémy Compostella has posted comments on this change by Naresh Solanki. ( https://review.coreboot.org/c/coreboot/+/85640?usp=email ) Change subject: soc/amd/glinda/cpu: Update cache info ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: I am a bit confused by this set of patches. Could you try to clarify the need, the technical limits of the current implementation and available weak functions ? -- To view, visit https://review.coreboot.org/c/coreboot/+/85640?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I46947e8ac62c903036a81642e03201e353c3dac6 Gerrit-Change-Number: 85640 Gerrit-PatchSet: 1 Gerrit-Owner: Naresh Solanki <naresh.solanki(a)9elements.com> Gerrit-CC: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Naresh Solanki <naresh.solanki(a)9elements.com> Gerrit-Comment-Date: Tue, 17 Dec 2024 22:20:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[S] Change in coreboot[main]: soc/intel: Log CSE Sync Early Sign of Life event from a better place
by Karthik Ramasubramanian (Code Review) 17 Dec '24

17 Dec '24

Attention is currently required from: Dinesh Gehlot, Eran Mitrani, Jakub Czapiga, Jayvik Desai, Kapil Porwal, Nick Vaccaro, Subrata Banik, Tarun. Karthik Ramasubramanian has posted comments on this change by Karthik Ramasubramanian. ( https://review.coreboot.org/c/coreboot/+/85111?usp=email ) Change subject: soc/intel: Log CSE Sync Early Sign of Life event from a better place ...................................................................... Patch Set 5: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/85111?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Iaa0dbb87ddde69dc3f4a9e058fc6bed8711b29e7 Gerrit-Change-Number: 85111 Gerrit-PatchSet: 5 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Eran Mitrani <mitrani(a)google.com> Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com> Gerrit-Reviewer: Jayvik Desai <jayvik(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun <tstuli(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jayvik Desai <jayvik(a)google.com> Gerrit-Attention: Eran Mitrani <mitrani(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Attention: Tarun <tstuli(a)gmail.com> Gerrit-Comment-Date: Tue, 17 Dec 2024 22:00:21 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[L] Change in coreboot[main]: Add UPL FDT handoff
by Benjamin Doron (Code Review) 17 Dec '24

17 Dec '24

Attention is currently required from: Arthur Heymans, Jakub Czapiga, Jérémy Compostella, Lean Sheng Tan, Matt DeVillier, Maximilian Brune, Philipp Hug, Simon Glass, ron minnich. Benjamin Doron has posted comments on this change by Maximilian Brune. ( https://review.coreboot.org/c/coreboot/+/76591?usp=email ) Change subject: Add UPL FDT handoff ...................................................................... Patch Set 28: (1 comment) File src/lib/upl_fdt_table.c: https://review.coreboot.org/c/coreboot/+/76591/comment/2a8c89ec_cc3c6b4e?us… : PS24, Line 99: // add memory and memory-reserved nodes; TODO(benjamindoron): Some mmio instead of memory > what is the intention behind the TODO? I believe some of these should be "mmio" not "memory". The payload may be able to use these more intelligently if it knows what it's reserved as -- To view, visit https://review.coreboot.org/c/coreboot/+/76591?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I36148e9de6ee992a67ec853ef5cbf1b5f83b44ae Gerrit-Change-Number: 76591 Gerrit-PatchSet: 28 Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Philipp Hug <philipp(a)hug.cx> Gerrit-Reviewer: Simon Glass <sjg(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: ron minnich <rminnich(a)gmail.com> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Shuo Liu <shuo.liu(a)intel.com> Gerrit-CC: coreboot org <coreboot.org(a)gmail.com> Gerrit-Attention: Philipp Hug <philipp(a)hug.cx> Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Attention: Simon Glass <sjg(a)chromium.org> Gerrit-Attention: ron minnich <rminnich(a)gmail.com> Gerrit-Comment-Date: Tue, 17 Dec 2024 21:57:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Maximilian Brune <maximilian.brune(a)9elements.com>

1 0

[M] Change in coreboot[main]: lib/{fit,fit_payload}.c: Enhance support for FIT images
by Benjamin Doron (Code Review) 17 Dec '24

17 Dec '24

Attention is currently required from: Felix Held, Julius Werner, Lean Sheng Tan, Matt DeVillier, Maximilian Brune. Benjamin Doron has posted comments on this change by Benjamin Doron. ( https://review.coreboot.org/c/coreboot/+/84796?usp=email ) Change subject: lib/{fit,fit_payload}.c: Enhance support for FIT images ...................................................................... Patch Set 5: (7 comments) File src/include/fit.h: https://review.coreboot.org/c/coreboot/+/84796/comment/0388d448_c540f889?us… : PS4, Line 19: uintptr_t entry_offset; > Yeah, they are, and I will. […] Done File src/lib/fit.c: https://review.coreboot.org/c/coreboot/+/84796/comment/a0122802_364837da?us… : PS4, Line 121: be32_to_cpu(*(size_t *) I'm surprised there are two ways to convert endianness in coreboot. Is one just the internals for the other? Either way, that's done. > Also, if you do need to directly dereference (and hopefully we should have enough helpers for every case so you never do), > never use architecture-dependent-length types like size_t or uintptr_t. Ah, I think I see now. As I saw it, I was casting a data pointer (probably `void *`) into a natively sized pointer to dereference, then swapping the bytes for converting it. But this doesn't work when dealing with external data. There are multiple problems though: - EDK2 has no address-cells and size-cells props - EDK2 might not comply with its own props: the implementation as of patchset 5 reflects EDK2's build. If data-offset is a kind of address, well, they generate it as a fdt32_t anyways - Are you sure about `cells != prop->size * 4`? I'd expect `cells * 4 != prop->size`. This seems to be the case, and it's fine if this was pseudo-code, but I can't actually test it because of the above So, I have no compliant FIT to test. I've asked around for a FIT to look at. For the moment, I'm pushing this enhancement as a follow-up. Is that okay with you? Since all of these are additions, we know it won't break for previously supported FITs. https://review.coreboot.org/c/coreboot/+/84796/comment/5b5c5e37_d46fd584?us… : PS4, Line 136: else if (!strcmp("entry-start", prop->prop.name)) > If there is no semantic difference between this and the normal entrypoint address that we always use […] Done https://review.coreboot.org/c/coreboot/+/84796/comment/f35d3756_1eb4dd17?us… : PS4, Line 398: "processing config %s.\n", config->name); > It still feels weird returning 0 from here. […] That's fair, we'll make each function responsible for its task. This one isn't driving the flow. https://review.coreboot.org/c/coreboot/+/84796/comment/a003a076_12f3c3c5?us… : PS4, Line 491: config->name); > This changes behavior for kernel loading. […] My thought was that if somebody implemented UPL support for LinuxBoot, the names in the config nodes might be `(kernel, initrd)` rather than `(firmware, initrd)`. In which case an FDT will be generated by the follow-up, not inserted in the FIT. I've since confirmed that the node names would be `(firmware, initrd)` though, so I can change this. File src/lib/fit_payload.c: https://review.coreboot.org/c/coreboot/+/84796/comment/873ba2ed_e0f3fb14?us… : PS4, Line 171: kernel > Now that this isn't always a kernel, should we name it something different (maybe `code`)? Done File src/lib/fit_payload.c: https://review.coreboot.org/c/coreboot/+/84796/comment/23391615_92cca6b9?us… : PS5, Line 105: load_secondaries > This doesn't even make an effort to check if a load address was requested. […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/84796?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I6a21954c0dc5fd820d135e8cd0599ce87903a1c0 Gerrit-Change-Number: 84796 Gerrit-PatchSet: 5 Gerrit-Owner: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Tue, 17 Dec 2024 21:43:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Benjamin Doron <benjamin.doron00(a)gmail.com> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>

1 0

[S] Change in coreboot[main]: [WIP] commonlib/device_tree.c,lib/fit.c: Consider address-cells and s...
by Benjamin Doron (Code Review) 17 Dec '24

17 Dec '24

Benjamin Doron has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/85643?usp=email ) Change subject: [WIP] commonlib/device_tree.c,lib/fit.c: Consider address-cells and size-cells ...................................................................... [WIP] commonlib/device_tree.c,lib/fit.c: Consider address-cells and size-cells Follow the recommendation at https://review.coreboot.org/c/coreboot/+/84796/comment/21f615a2_99a41147/ and implement support for reading integer properties generically, using their size to determine how much to read. This is used for reading `load`, `entry`, etc. I still need to confirm with the FIT spec if we should find these cells in the root, or /images? TODO: I want to confirm the maths, but I don't have a known-good FIT to use. This is broken for EDK2's UPL: they're not compliant, there is no {address,size}-cells prop there. Change-Id: I02d27eb5e23dfbfc1404d209ee8d60968e22bb80 Signed-off-by: Benjamin Doron <benjamin.doron(a)9elements.com> --- M src/commonlib/device_tree.c M src/commonlib/include/commonlib/device_tree.h M src/lib/fit.c 3 files changed, 27 insertions(+), 7 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/85643/1 diff --git a/src/commonlib/device_tree.c b/src/commonlib/device_tree.c index b2daac9..5997201 100644 --- a/src/commonlib/device_tree.c +++ b/src/commonlib/device_tree.c @@ -260,6 +260,23 @@ return offset; } +uint64_t fdt_read_int_prop(struct fdt_property *prop, u32 cells) +{ + printk(BIOS_DEBUG, "cells %d, prop->size %d\n", cells, prop->size); + if (false) {// FIXME: Confirm the math, I suspect the other order: cells != prop->size * 4) { + printk(BIOS_ERR, "FDT integer property of size %u @%p doesn't match expected cell count %u\n", prop->size, prop->data, cells); + return 0; + } + + if (cells == 2) + return be64dec(prop->data); + else if (cells == 1) + return be32dec(prop->data); + + printk(BIOS_ERR, "Illegal FDT integer property size %u @%p\n", prop->size, prop); + return 0; +} + /* * fdt_find_node searches for a node relative to another node * diff --git a/src/commonlib/include/commonlib/device_tree.h b/src/commonlib/include/commonlib/device_tree.h index c15d7ae..df17a1a 100644 --- a/src/commonlib/include/commonlib/device_tree.h +++ b/src/commonlib/include/commonlib/device_tree.h @@ -118,6 +118,8 @@ /* Read reg property and save regions inside 'regions'. Returns number of regions read */ u32 fdt_read_reg_prop(const void *blob, u32 node_offset, u32 addr_cells, u32 size_cells, struct device_tree_region regions[], size_t regions_count); +/* Reads value for a fdt_prop, considering the cells */ +uint64_t fdt_read_int_prop(struct fdt_property *prop, u32 cells); /* Find a node by a given path and return the offset */ u32 fdt_find_node_by_path(const void *blob, const char *path, u32 *addrcp, u32 *sizecp); /* Find multiple nodes matching a given pattern. Returns number of nodes found */ diff --git a/src/lib/fit.c b/src/lib/fit.c index 3e384d7..25e9ee6 100644 --- a/src/lib/fit.c +++ b/src/lib/fit.c @@ -107,7 +107,7 @@ return base; } -static void image_node(struct device_tree_node *node) +static void image_node(struct device_tree_node *node, u32 addr_cells, u32 size_cells) { struct fit_image_node *image = xzalloc(sizeof(*image)); @@ -117,9 +117,9 @@ struct device_tree_property *prop; list_for_each(prop, node->properties, list_node) { if (!strcmp("data-offset", prop->prop.name)) - image->data = fit_bin_base + be32dec(prop->prop.data); + image->data = fit_bin_base + fdt_read_int_prop(&prop->prop, addr_cells); else if (!strcmp("data-size", prop->prop.name)) - image->size = be32dec(prop->prop.data); + image->size = fdt_read_int_prop(&prop->prop, size_cells); else if (!strcmp("data", prop->prop.name)) { image->data = prop->prop.data; image->size = prop->prop.size; @@ -133,9 +133,9 @@ else image->compression = -1; } else if (!strcmp("load", prop->prop.name)) - image->load_address = be64dec(prop->prop.data); + image->load_address = fdt_read_int_prop(&prop->prop, addr_cells); else if (!strcmp("entry", prop->prop.name)) - image->entrypoint_address = be64dec(prop->prop.data); + image->entrypoint_address = fdt_read_int_prop(&prop->prop, addr_cells); } list_insert_after(&image->list_node, &image_nodes); @@ -166,12 +166,13 @@ static void fit_unpack(struct device_tree *tree, const char **default_config) { + u32 addr_cells, size_cells; struct device_tree_node *child; struct device_tree_node *images = dt_find_node_by_path(tree, "/images", - NULL, NULL, 0); + &addr_cells, &size_cells, 0); if (images) list_for_each(child, images->children, list_node) - image_node(child); + image_node(child, addr_cells, size_cells); struct device_tree_node *configs = dt_find_node_by_path(tree, "/configurations", NULL, NULL, 0); -- To view, visit https://review.coreboot.org/c/coreboot/+/85643?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I02d27eb5e23dfbfc1404d209ee8d60968e22bb80 Gerrit-Change-Number: 85643 Gerrit-PatchSet: 1 Gerrit-Owner: Benjamin Doron <benjamin.doron00(a)gmail.com>

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit December 2024