coreboot-gerrit December 2024

coreboot-gerrit@coreboot.org

1 participants
3018 discussions

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

18 Dec '24

Attention is currently required from: Bora Guvendik, Cliff Huang, Kapil Porwal, Kyoung Il Kim, Paul Menzel, Pranava Y N, Subrata Banik. Jérémy Compostella has posted comments on this change by Cliff Huang. ( https://review.coreboot.org/c/coreboot/+/85199?usp=email ) Change subject: soc/intel/pantherlake: Update Touch Controller UPD params ...................................................................... Patch Set 8: (1 comment) File src/soc/intel/pantherlake/chip.h: https://review.coreboot.org/c/coreboot/+/85199/comment/ca9b7630_15e96c03?us… : PS8, Line 520: Hid > Spi? Done -- 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: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I15fb62eaadc03b9a17e94609b97c686518150e2e Gerrit-Change-Number: 85199 Gerrit-PatchSet: 8 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: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Pranava Y N <pranavayn(a)google.com> Gerrit-Comment-Date: Wed, 18 Dec 2024 17:21:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Kapil Porwal <kapilporwal(a)google.com>

1 0

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

18 Dec '24

Attention is currently required from: Bora Guvendik, Cliff Huang, Jérémy Compostella, Kyoung Il Kim, Paul Menzel, Pranava Y N, Subrata Banik. Jérémy Compostella has uploaded a new patch set (#9) to the change originally created by Cliff Huang. ( https://review.coreboot.org/c/coreboot/+/85199?usp=email ) The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) 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/9 -- 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: 9 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: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Pranava Y N <pranavayn(a)google.com>

1 0

[S] Change in coreboot[main]: soc/intel/pantherlake: Bind SoC config VR settings to respective UPD
by Jérémy Compostella (Code Review) 18 Dec '24

18 Dec '24

Attention is currently required from: Kapil Porwal, Paul Menzel, Pranava Y N, Subrata Banik. Jérémy Compostella has posted comments on this change by Jérémy Compostella. ( https://review.coreboot.org/c/coreboot/+/85131?usp=email ) Change subject: soc/intel/pantherlake: Bind SoC config VR settings to respective UPD ...................................................................... Patch Set 6: (1 comment) This change is ready for review. File src/soc/intel/pantherlake/romstage/fsp_params.c: https://review.coreboot.org/c/coreboot/+/85131/comment/058d75e6_3dea20f7?us… : PS6, Line 271: ARRAY_SIZE(config->enable_fast_vmode > nit: Can we use NUM_VR_DOMAINS macro directly here? I have a couple of reasons to like doing it like this but the most important is that it does not bind this code to a declared constant. Instead the compiler provides the size at compilation time avoid any de-synchronization between declaration and use. -- To view, visit https://review.coreboot.org/c/coreboot/+/85131?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: Ie72e4725cb97b4af7843a43eeaedd687d28b6752 Gerrit-Change-Number: 85131 Gerrit-PatchSet: 6 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Pranava Y N <pranavayn(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Pranava Y N <pranavayn(a)google.com> Gerrit-Comment-Date: Wed, 18 Dec 2024 17:10:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Pranava Y N <pranavayn(a)google.com>

1 0

[XS] Change in coreboot[main]: mb/google/corsola: Add new board variant Ponyta360
by Yu-Ping Wu (Code Review) 18 Dec '24

18 Dec '24

Attention is currently required from: Hung-Te Lin, Xinxiong Xu, Yang Wu. Yu-Ping Wu has posted comments on this change by Yang Wu. ( https://review.coreboot.org/c/coreboot/+/85652?usp=email ) The change is no longer submittable: All-Comments-Resolved is unsatisfied now. Change subject: mb/google/corsola: Add new board variant Ponyta360 ...................................................................... Patch Set 1: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/85652/comment/b1bc7c04_a21d7278?us… : PS1, Line 7: mb/google/corsola: Add new board variant Ponyta360 > As [b:375298183](https://partnerissuetracker.corp.google. […] Replied on the bug. Let's discuss there. -- To view, visit https://review.coreboot.org/c/coreboot/+/85652?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: Ic0ddbfc667d1ba583f3cb95f60c73eb344b454c6 Gerrit-Change-Number: 85652 Gerrit-PatchSet: 1 Gerrit-Owner: Yang Wu <wuyang5(a)huaqin.corp-partner.google.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Xinxiong Xu <xuxinxiong(a)huaqin.corp-partner.google.com> Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Xinxiong Xu <xuxinxiong(a)huaqin.corp-partner.google.com> Gerrit-Attention: Yang Wu <wuyang5(a)huaqin.corp-partner.google.com> Gerrit-Comment-Date: Wed, 18 Dec 2024 16:07:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Yang Wu <wuyang5(a)huaqin.corp-partner.google.com> Comment-In-Reply-To: Yu-Ping Wu <yupingso(a)google.com>

1 0

[XL] Change in coreboot[main]: soc/meidatek/mt8196: Add SPM loader
by Yu-Ping Wu (Code Review) 18 Dec '24

18 Dec '24

Attention is currently required from: Ccong Chen, Hung-Te Lin, Jarried Lin, Yidi Lin. Yu-Ping Wu has posted comments on this change by Ccong Chen. ( https://review.coreboot.org/c/coreboot/+/85599?usp=email ) Change subject: soc/meidatek/mt8196: Add SPM loader ...................................................................... Patch Set 6: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/85599?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: Ie09346f46cb734c74776b760485e7f35d4357e5e Gerrit-Change-Number: 85599 Gerrit-PatchSet: 6 Gerrit-Owner: Ccong Chen <ot_ccong.chen(a)mediatek.corp-partner.google.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-CC: Wenzhen Yu <wenzhen.yu(a)mediatek.corp-partner.google.com> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Ccong Chen <ot_ccong.chen(a)mediatek.corp-partner.google.com> Gerrit-Attention: Yidi Lin <yidilin(a)google.com> Gerrit-Attention: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-Comment-Date: Wed, 18 Dec 2024 15:53:47 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

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

18 Dec '24

Attention is currently required from: Benjamin Doron, Felix Held, Lean Sheng Tan, Matt DeVillier, Maximilian Brune. Julius Werner 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 6: (19 comments) File src/lib/fit.c: https://review.coreboot.org/c/coreboot/+/84796/comment/ee4e7333_3adaf0e9?us… : PS4, Line 121: be32_to_cpu(*(size_t *) > I'm surprised there are two ways to convert endianness in coreboot. The difference is just about whether you already have an integer or want to read from a pointer. For cases like this the enc/dec variants can make things look a little cleaner. > EDK2 has no address-cells and size-cells props They should fix that, but until they do we'll assume the default of 2/1. > 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 Well... to be fair, the FIT spec doesn't exactly say what length the data-xxx values are supposed to be (it does say for `load` and `entry`, though). I kind of assumed it would make sense to use #xxx-cells, but it does seem dangerously underspecified. Maybe you should reach out to the FIT spec maintainers to get some clarification. Just blindly assuming 32-bit seems wrong to me unless the spec explicitly says so. But since we know it should always be a single value, I guess we could just read the size and decode as 32 or 64 based on that (basically the function I suggested above, just without checking the `cells` value and matching `prop->size == 4` and `prop->size == 8` instead? If you're introducing it as a new helper, maybe we could say that passing `0` for the `cells` parameter makes it use that mode, so that it's also useful for cases like `load` below). > Are you sure about `cells != prop->size * 4`? I'd expect `cells * 4 != prop->size`. Right, that was a typo, sorry. https://review.coreboot.org/c/coreboot/+/84796/comment/bcd4c98e_e3d5627b?us… : PS4, Line 523: } > Yeah, that's pretty much what's going on here. […] Hmm... can't say I really like that design tbh, it doesn't really match how FIT images normally work as a container of images that the configuration node picks the right ones out of. It would have fit in better with the existing design if they added a prop that lists those names explicitly, e.g. `extra-images = "uefi-fv", "bds-fv";`. But okay, if they want it that way I guess we can load it that way. File src/lib/fit.c: https://review.coreboot.org/c/coreboot/+/84796/comment/58770343_e748cb52?us… : PS6, Line 19: static void *fit_bin_base; (FWIW the use of all these static globals seems like a bad idea. They would actually break things if we ever try to load a FIT, fail because of some parsing error half-way through, and then try to load some other fallback FIT instead. But the existing lists here are a bigger problem than this new base pointer, so I guess it doesn't make things worse to add it for now, but it would be great to eventually refactor all of that into a state structure that gets created in `fit_load()` and passed through all the functions instead.) https://review.coreboot.org/c/coreboot/+/84796/comment/c30f1999_3e3a191d?us… : PS6, Line 119: if (!strcmp("data-offset", prop->prop.name)) nit: IIRC coding style says that if one if-else branch has curly braces, all of them must use them (except for the `else if` on a single line, that's allowed). https://review.coreboot.org/c/coreboot/+/84796/comment/273d7988_2c724e72?us… : PS6, Line 136: image->load_address = be64dec(prop->prop.data); These two still need to be #address-cells dependent, since in this case it actually says so in the spec and the default cells value matches what you need here. https://review.coreboot.org/c/coreboot/+/84796/comment/af68d47e_6f58a885?us… : PS6, Line 509: if (fit_update_compat(config) && strcmp(config->name, default_config_name)) Let's rearrange these lines so you don't need to `strcmp()` twice: ``` if (default_config_name && !strcmp(config->name, default_config_name)) default_config = config; if (fit_update_compat(config) && config != default_config) continue; printk(BIOS_DEBUG, "FIT: config %s", config->name); if (config == default_config) printk(BIOS_DEBUG, " (default)"); ``` File src/lib/fit_payload.c: https://review.coreboot.org/c/coreboot/+/84796/comment/3a63fe8c_86f0f3fe?us… : PS6, Line 18: __weak void platform_fdt_add_secondaries(struct device_tree *tree, const char *name, struct region *secondary) I think weak functions should only be used when the base case is a very trivial "default" implementation (e.g. a no-op, or returning some hardcoded value). It's not good to implement significant logic in weak functions because it is unintuitive that just implementing that function in the platform part removes stuff that would otherwise happen. This goes way beyond a no-op or default value so I would say it shouldn't be implemented like this. I'm not exactly sure where the `/firmware/coreboot/image@...` stuff is coming from (note that for the Linux kernel, `/firmware/coreboot` bindings are clearly specced in https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Doc…, you shouldn't just invent new ones that aren't upstreamed there)... if they're part of UPL, maybe guard this by the UPL Kconfig, or if they're their own thing maybe add a new Kconfig. https://review.coreboot.org/c/coreboot/+/84796/comment/d9defc06_bed6b9cc?us… : PS6, Line 26: 64 nit: `sizeof(node_name)` Also, maybe `assert()` that return value is `<= sizeof(node_name)` https://review.coreboot.org/c/coreboot/+/84796/comment/cd7ceb6f_676f2b9d?us… : PS6, Line 31: dt_add_reg_prop(image_node, &addr, &size, 1, 2, 1); Also, adding a `reg` prop to `/firmware/coreboot/image@...` would just formally require you to add `#address-cells`, `#size-cells` and `ranges` props to `/firmware/coreboot` which it normally doesn't have. https://review.coreboot.org/c/coreboot/+/84796/comment/f99c9fa1_d4cd5da1?us… : PS6, Line 105: kernel `code` (or maybe `target_region` or something like that if you want it to be clearer in context; either way, it's usually not a kernel here) https://review.coreboot.org/c/coreboot/+/84796/comment/d8319adb_ae35b520?us… : PS6, Line 112: return; Why is this here? If there are secondaries defined and they don't fit, shouldn't that be a hard error? https://review.coreboot.org/c/coreboot/+/84796/comment/7977230d_a0e92ca3?us… : PS6, Line 116: secondaries.size = kernel->size - primary_image->size; I know Nico never quite finished his work on hiding region struct members (CB:79907), unfortunately, and the FIT code was one of the remaining issues... but at least when adding new code we should try to stick to the external APIs (e.g. `struct region secondaries = region_create(region_offset(kernel) + primary_image->size, region_size(kernel) - primary_image->size);`; you could also use `region_create_untrusted()` to integrate the size check right here). https://review.coreboot.org/c/coreboot/+/84796/comment/a43cfe5a_487307f7?us… : PS6, Line 118: /* We may want to check if relocation is needed and then at least do something: nit: 5 lines is a bit long for short multiline comment style, maybe better use ``` /* * ... */ ``` here. (Also, I don't quite understand what the comment is trying to tell me tbh. Are you saying that this code is not yet doing the right thing here? Then why not write code that does instead?) https://review.coreboot.org/c/coreboot/+/84796/comment/54e3f31a_aaaaec60?us… : PS6, Line 131: continue; Shouldn't you return a hard error that makes the parent function fail in these cases? That's what we do elsewhere when the image is broken. https://review.coreboot.org/c/coreboot/+/84796/comment/4546c568_2204f3ae?us… : PS6, Line 134: secondaries Your use of this in the function above implies that it should be the space taken up by the image just added, but that's not what this is — this is the entire remaining space for adding more images. You'd have to modify `extract()` to return the decompressed size if you want to know the exact size of only that image. Actually, you need to do that anyway, because your `secondaries.size -= image_chain->image->size` below is not correct in the case of compressed images. https://review.coreboot.org/c/coreboot/+/84796/comment/f97e90c3_1e9b53b5?us… : PS6, Line 137: secondaries.size -= image_chain->image->size; Maybe we should create a new helper called `region_shrink(&region, size)` for this kind of thing in `<region.h>`. Could also use that above then, to initially create this as ``` struct region secondary = kernel; if (region_shrink(&secondary, primary_image->size)) ...error... ``` https://review.coreboot.org/c/coreboot/+/84796/comment/2ce82212_dbff06f4?us… : PS6, Line 271: code.size += image_chain->image->size; Yeah, this also doesn't do the right thing with compression. In general I think you need to decide whether you either want to forbid compression for these secondary images entirely (then that should be tested), or decode and make use of that `uncomp-size` prop in the UPL spec, or just allocate a huge area in bootmem to begin with and later shrink that back down once you have decompressed everything and know the actual size. https://review.coreboot.org/c/coreboot/+/84796/comment/787f2613_0832ea80?us… : PS6, Line 274: if (!fit_payload_arch(payload, config, &code, &fdt, &initrd)) { After looking through all of this some more I'm not really convinced that reusing this function really makes sense for your firmware image node. The point of calling into arch code here is to implement all the Linux kernel loading restrictions and quirks for different architectures. But you're not loading a kernel and none of those make sense for you. The only reason you're getting away with this right now is that you just disallow your case for Arm and Arm64, and only implement it for x86 which didn't have any FIT arch code before. I think it would make more sense to split this between the two cases: maybe rename `fit_payload_arch()` to something like `fit_linux_arch()` or `fit_place_linux()`, and create a different function (e.g. `fit_place_firmware(struct prog *payload, struct fit_config_node *config, struct region *code)` — shouldn't need the FDT or initrd parts which your case doesn't have anyway) to implement the firmware case where you just find a random free spot in bootmem. That probably doesn't even need to be arch-specific then. You could also do the secondary image stuff in there. https://review.coreboot.org/c/coreboot/+/84796/comment/19ce45f9_92364bb2?us… : PS6, Line 306: dt_print_node(dt->root); This prints the whole DT, right? I don't think we should do that by default? If you think it is useful for debugging, maybe add a Kconfig for it? -- 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: 6 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: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com> 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: Wed, 18 Dec 2024 15:49:08 +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

[XS] Change in coreboot[main]: DEMO: Enable AB recovery
by Bao Zheng (Code Review) 18 Dec '24

18 Dec '24

Attention is currently required from: Felix Held, Fred Reitberger, Jason Glenesk, Matt DeVillier, Zheng Bao. Hello Zheng Bao, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/85659?usp=email to review the following change. Change subject: DEMO: Enable AB recovery ...................................................................... DEMO: Enable AB recovery Change-Id: I2042071bbe273053d75d32d6a06e992ff55cc98e Signed-off-by: Zheng Bao <fishbaozi(a)gmail.com> --- M src/soc/amd/cezanne/Kconfig 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/85659/1 diff --git a/src/soc/amd/cezanne/Kconfig b/src/soc/amd/cezanne/Kconfig index c6846a1..cd1cd07 100644 --- a/src/soc/amd/cezanne/Kconfig +++ b/src/soc/amd/cezanne/Kconfig @@ -410,7 +410,7 @@ config PSP_AB_RECOVERY bool "Use A/B Recovery scheme" - default n + default y help Enable the PSP A/B Recovery mechanism -- To view, visit https://review.coreboot.org/c/coreboot/+/85659?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: I2042071bbe273053d75d32d6a06e992ff55cc98e Gerrit-Change-Number: 85659 Gerrit-PatchSet: 1 Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Zheng Bao Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Zheng Bao Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>

1 0

[S] Change in coreboot[main]: mb/majolica: Add flashmap data for B partition
by Bao Zheng (Code Review) 18 Dec '24

18 Dec '24

Attention is currently required from: Bao Zheng, Felix Held, Fred Reitberger, Jason Glenesk, Zheng Bao. Hello Felix Held, Fred Reitberger, Jason Glenesk, Zheng Bao, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/85648?usp=email to look at the new patch set (#3). Change subject: mb/majolica: Add flashmap data for B partition ...................................................................... mb/majolica: Add flashmap data for B partition Change-Id: I5eb4cc8a5a64f47a9a7fe2085e3b601fe7162ffb Signed-off-by: Zheng Bao <fishbaozi(a)gmail.com> --- M src/mainboard/amd/majolica/Kconfig A src/mainboard/amd/majolica/board_ab.fmd 2 files changed, 15 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/85648/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/85648?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: I5eb4cc8a5a64f47a9a7fe2085e3b601fe7162ffb Gerrit-Change-Number: 85648 Gerrit-PatchSet: 3 Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Zheng Bao Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Zheng Bao Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>

1 0

[L] Change in coreboot[main]: soc/mediatek/mt8196: Add GPUEB support
by Yidi Lin (Code Review) 18 Dec '24

18 Dec '24

Attention is currently required from: Andy Hsu, Hung-Te Lin, Jarried Lin, Yu-Ping Wu. Yidi Lin has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/85654?usp=email ) Change subject: soc/mediatek/mt8196: Add GPUEB support ...................................................................... Patch Set 1: (18 comments) File src/soc/mediatek/mt8196/gpueb.c: https://review.coreboot.org/c/coreboot/+/85654/comment/e33e26d5_90781aec?us… : PS1, Line 96: uint32_t align with first param https://review.coreboot.org/c/coreboot/+/85654/comment/cdc07421_7c2ee3fa?us… : PS1, Line 126: 50 define this value https://review.coreboot.org/c/coreboot/+/85654/comment/845ff0ed_3ca2e5ca?us… : PS1, Line 131: 1000 define this value https://review.coreboot.org/c/coreboot/+/85654/comment/074a1ca4_1fc758dc?us… : PS1, Line 187: 500 define https://review.coreboot.org/c/coreboot/+/85654/comment/fd02e677_53556142?us… : PS1, Line 198: mfg0_rpc_ctrl_shutdown why do we pass `POWER_ON` to a function named with `_shutdown` ? https://review.coreboot.org/c/coreboot/+/85654/comment/06faaf1e_c49ac33e?us… : PS1, Line 235: tmp_addr `buf_addr` or just `buf` https://review.coreboot.org/c/coreboot/+/85654/comment/b5f6e59f_31570c3a?us… : PS1, Line 253: need include no need to https://review.coreboot.org/c/coreboot/+/85654/comment/7048a7c1_b7386f45?us… : PS1, Line 276: int pt_id `enum pt_fw_idx id` https://review.coreboot.org/c/coreboot/+/85654/comment/b768ebb6_72b423c8?us… : PS1, Line 279: a align with 2nd `(` https://review.coreboot.org/c/coreboot/+/85654/comment/08de34b2_dbf8077c?us… : PS1, Line 339: . remove tailing `.` https://review.coreboot.org/c/coreboot/+/85654/comment/a8057f4b_074d5a8f?us… : PS1, Line 369: wait_us(BOOT_TIMEOUT_US, is_boot_up_complete()) == 0 if (!wait_us(BOOT_TIMEOUT_US, is_boot_up_complete())) File src/soc/mediatek/mt8196/include/soc/gpueb_priv.h: https://review.coreboot.org/c/coreboot/+/85654/comment/35eec812_7b94236e?us… : PS1, Line 21: define MFG_RPC_MFG1_PWR_CON (GPU_EB_RPC_BASE + 0x0500) /* 0x4B800500 */ : #define MFG_RPC_MFG0_PWR_CON (GPU_EB_RPC_BASE + 0x0504) /* 0x4B800504 */ just want to confirm that the ordering is correct. https://review.coreboot.org/c/coreboot/+/85654/comment/008bf51e_bfac3c31?us… : PS1, Line 104: 0x00000064 100 https://review.coreboot.org/c/coreboot/+/85654/comment/c8672425_943be18f?us… : PS1, Line 117: SW align with above `SW` https://review.coreboot.org/c/coreboot/+/85654/comment/0c8117f2_08e694b9?us… : PS1, Line 124: PMIC_ACK_SYNC align https://review.coreboot.org/c/coreboot/+/85654/comment/393542b3_6737075e?us… : PS1, Line 129: MFG_RPC_MFG0_PWR_ACK_2ND_BIT align https://review.coreboot.org/c/coreboot/+/85654/comment/1b3c6cff_67ecacce?us… : PS1, Line 160: PT_FW_IDX lower case File src/soc/mediatek/mt8196/soc.c: https://review.coreboot.org/c/coreboot/+/85654/comment/cb14dfeb_3a58e663?us… : PS1, Line 24: (uint64_t) no need. -- To view, visit https://review.coreboot.org/c/coreboot/+/85654?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: I0f10dfc753f73df97ea08a4c23e97de416832be2 Gerrit-Change-Number: 85654 Gerrit-PatchSet: 1 Gerrit-Owner: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-Reviewer: Andy Hsu <andy.hsu(a)mediatek.corp-partner.google.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Andy Hsu <andy.hsu(a)mediatek.corp-partner.google.com> Gerrit-Attention: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Comment-Date: Wed, 18 Dec 2024 15:41:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[S] Change in coreboot[main]: CEZANNE:AB recovery: Add offsets for A/B to amdfwtool command line
by Bao Zheng (Code Review) 18 Dec '24

18 Dec '24

Attention is currently required from: Felix Held, Fred Reitberger, Jason Glenesk, Matt DeVillier, Zheng Bao. Hello Felix Held, Fred Reitberger, Jason Glenesk, Matt DeVillier, Zheng Bao, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/85658?usp=email to look at the new patch set (#2). Change subject: CEZANNE:AB recovery: Add offsets for A/B to amdfwtool command line ...................................................................... CEZANNE:AB recovery: Add offsets for A/B to amdfwtool command line Change-Id: Ib6a65b935b234df7a7b8d95c02694d2068da7a9f Signed-off-by: Zheng Bao <fishbaozi(a)gmail.com> --- M src/soc/amd/cezanne/Makefile.mk 1 file changed, 11 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/85658/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/85658?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: Ib6a65b935b234df7a7b8d95c02694d2068da7a9f Gerrit-Change-Number: 85658 Gerrit-PatchSet: 2 Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Zheng Bao Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Zheng Bao Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit December 2024