coreboot-gerrit July 2021

coreboot-gerrit@coreboot.org

1 participants
5824 discussions

Change in coreboot[master]: sc7280: gpio: eGPIO support on sc7280
by Taniya Das (Code Review) 09 Jul '21

09 Jul '21

Attention is currently required from: Shelley Chen, Ravi kumar, Paul Menzel, mturney mturney. Taniya Das has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55078 ) Change subject: sc7280: gpio: eGPIO support on sc7280 ...................................................................... Patch Set 10: (6 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/55078/comment/24446ab1_75af6702 PS8, Line 7: sc7280: gpio: eGPIO support on sc7280 > Please use statement (by using a verb in imperative mood) [1], and avoid redundancy. […] Done https://review.coreboot.org/c/coreboot/+/55078/comment/5b2c70cf_8ea41bca PS8, Line 15: This change provides an API to configure the eGPIO's. > Please describe the API. Done https://review.coreboot.org/c/coreboot/+/55078/comment/ac7c8d8f_42319b61 PS8, Line 18: TEST=Validated on qualcomm sc7280 development board > How? Done File src/soc/qualcomm/sc7280/include/soc/gpio.h: https://review.coreboot.org/c/coreboot/+/55078/comment/3dc2a2bf_04970214 PS8, Line 198: EGPIO_CFG_BMSK = 0x1, > Why hex, when below decimal is used? Done File src/soc/qualcomm/sc7280/sc7280_egpio.c: https://review.coreboot.org/c/coreboot/+/55078/comment/6ec1cc38_c781ffe5 PS8, Line 7: int egpio_configure_pins(int start, int end) > I’d use size_t or unsigned int. Done https://review.coreboot.org/c/coreboot/+/55078/comment/2c77b5a7_c5a4a38b PS8, Line 19: EGPIO_CFG_BMSK)) { > Fits on the line above. Done -- To view, visit https://review.coreboot.org/c/coreboot/+/55078 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2c54a14c50fb7b5921d1961d2de83098ed2d4358 Gerrit-Change-Number: 55078 Gerrit-PatchSet: 10 Gerrit-Owner: Ravi kumar <rbokka(a)codeaurora.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Taniya Das <tdas(a)codeaurora.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: mturney mturney <mturney(a)codeaurora.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Ravi Kumar Bokka <c_rbokka(a)qualcomm.corp-partner.google.com> Gerrit-Attention: Shelley Chen <shchen(a)google.com> Gerrit-Attention: Ravi kumar <rbokka(a)codeaurora.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: mturney mturney <mturney(a)codeaurora.org> Gerrit-Comment-Date: Fri, 09 Jul 2021 01:05:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: Revert "Makefile.inc: Drop the cbfs master header from non-X86"
by Julius Werner (Code Review) 09 Jul '21

09 Jul '21

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56130 ) Change subject: Revert "Makefile.inc: Drop the cbfs master header from non-X86" ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/56130/comment/22bb31e4_90a14e54 PS1, Line 11: Breaks libpayload CBFS code when accessing : non-primary CBFS. > > I see that there is FMAP support in libpayload. […] (In fact I just remembered FMAP cache isn't optional in CBMEM, only in SRAM. In CBMEM it should always exist. So libpayload should always be able to just grab it out of there and doesn't even have to worry about flash access then, at least for the FMAP part.) -- To view, visit https://review.coreboot.org/c/coreboot/+/56130 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id7f47406e6126f19e1fd6bc9d33c8c9d0cb9450d Gerrit-Change-Number: 56130 Gerrit-PatchSet: 3 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Fri, 09 Jul 2021 00:55:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: Revert "Makefile.inc: Drop the cbfs master header from non-X86"
by Julius Werner (Code Review) 09 Jul '21

09 Jul '21

Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/56130 ) Change subject: Revert "Makefile.inc: Drop the cbfs master header from non-X86" ...................................................................... Revert "Makefile.inc: Drop the cbfs master header from non-X86" This reverts commit d109354c0f1d4b155c60701cd42e632213350d72. Reason for revert: Breaks libpayload CBFS code when accessing non-default CBFS. BUG=b:193093750 Change-Id: Id7f47406e6126f19e1fd6bc9d33c8c9d0cb9450d Signed-off-by: Julius Werner <jwerner(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/coreboot/+/56130 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M Makefile.inc 1 file changed, 9 insertions(+), 2 deletions(-) Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved diff --git a/Makefile.inc b/Makefile.inc index fee73f5..b1c7067 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -1101,15 +1101,22 @@ -b -$(call file-size,$(objcbfs)/bootblock.bin) $(cbfs-autogen-attributes) \ $(TS_OPTIONS) \ $(CBFSTOOL_ADD_CMD_OPTIONS) - # the cbfs master header is a deprecated feature only used on x86 - $(CBFSTOOL) $@.tmp add-master-header $(TS_OPTIONS) $(CBFSTOOL_ADD_CMD_OPTIONS) else # ifeq ($(CONFIG_ARCH_X86),y) $(CBFSTOOL) $@.tmp write -u \ -r BOOTBLOCK \ -f $(objcbfs)/bootblock.bin # make space for the CBFS master header pointer. "ptr_" is just # arbitrary 4 bytes that will be overwritten by add-master-header. + printf "ptr_" > $@.tmp.2 + $(CBFSTOOL) $@.tmp add \ + -f $@.tmp.2 \ + -n "header pointer" \ + -t "cbfs header" \ + -b -4 \ + $(CBFSTOOL_ADD_CMD_OPTIONS) + rm -f $@.tmp.2 endif # ifeq ($(CONFIG_ARCH_X86),y) + $(CBFSTOOL) $@.tmp add-master-header $(TS_OPTIONS) $(CBFSTOOL_ADD_CMD_OPTIONS) $(prebuild-files) true mv $@.tmp $@ else # ifneq ($(CONFIG_UPDATE_IMAGE),y) -- To view, visit https://review.coreboot.org/c/coreboot/+/56130 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id7f47406e6126f19e1fd6bc9d33c8c9d0cb9450d Gerrit-Change-Number: 56130 Gerrit-PatchSet: 3 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: Revert "Makefile.inc: Drop the cbfs master header from non-X86"
by Julius Werner (Code Review) 09 Jul '21

09 Jul '21

Attention is currently required from: Hung-Te Lin, Martin Roth, Yu-Ping Wu. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56130 ) Change subject: Revert "Makefile.inc: Drop the cbfs master header from non-X86" ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/56130/comment/61b9dcaa_36125cf8 PS1, Line 11: Breaks libpayload CBFS code when accessing : non-primary CBFS. > I see that there is FMAP support in libpayload. No, there isn... wait, you mean libc/fmap.c? Hmmm... never seen that before to be honest, depthcharge isn't using it. Not sure if anyone else is. Maybe Patrick knows more about that, it looks like he wrote it. Maybe it's some sort of aborted attempt? I agree that putting FMAP integration into the libpayload CBFS code would probably be the right long-term move, but I'm not sure if that existing FMAP code actually works. I think it has never been used. Would be good to have something there that could ideally also fetch and use coreboot's FMAP cache from CBMEM if it exists. -- To view, visit https://review.coreboot.org/c/coreboot/+/56130 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id7f47406e6126f19e1fd6bc9d33c8c9d0cb9450d Gerrit-Change-Number: 56130 Gerrit-PatchSet: 2 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Comment-Date: Fri, 09 Jul 2021 00:52:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: Makefile.inc: Drop the cbfs master header from non-X86
by Julius Werner (Code Review) 09 Jul '21

09 Jul '21

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56120 ) Change subject: Makefile.inc: Drop the cbfs master header from non-X86 ...................................................................... Patch Set 4: (1 comment) Patchset: PS4: > There was a rewrite on gerrit: https://review.coreboot.org/c/coreboot/+/15774 but it was blocked due to licensing issues... Yeah, the licensing issue with libpayload is always a pain. Unfortunately I think the rdev API has had too many patches by different people now to make relicensing feasible. At least the new CBFS core code I wrote is all in commonlib/bsd so it should be possible to reuse there, and it's written to be able to tie into different storage backends. Anyway, I think that patch didn't touch the parts at issue here anyway, it's more about all the stuff that happens in ram_media.c. > SeaBIOS has a patch pending to not use it. I need to write one for grub, it's not much work. I think it's worth getting rid of the header as updating a hardcoded pointer at the top of the flash is not so nice + most options in the header are pretty meaningless, like cbfs file alignment. boot_media_params has been around for a long time now too, so updating payloads to not rely on it and dropping the header is my current plan. Hmmm... okay. I'm a bit worried about other payloads we don't know about out there, but if nobody else complains I guess we can do it. FWIW the pointer was never at the top of flash anyway, just at the top of the CBFS region (for the cases where those are not the same). So it's really just an extra file at the end. > It's ok. I'll see if it's easy to fix libpayload. It's a big mess made by most of the libpayload code being really old (from before there were FMAP sections), and then some glue code to hooking into that in a way that can be made towork for more cases without touching the core libpayload code but doesn't make a lot of sense in itself. I think for depthcharge our cbfs_media is written so that accesses with a positive offset are treated as absolute flash offsets, but accesses with a negative (i.e. higher than 0x80000000) offset are treated as if counting back from the end of the CBFS section represented by that (which may not touch either the start or the end of the flash), so that when libpayload was trying to follow the master header pointer we would make sure it found the master header in the right section. I think this was forced by the cbfs_media structure not being very clear on whether it represented a region or a whole flash, and because the offset in the master header is always counted absolute from the start of the flash instead of the start of the CBFS region. So I don't think there's really a way to keep letting this work the way it did while taking the master header out of the equation. You'll probably have to change how cbfs_media is treated and then we'll have to change depthcharge to match that (and whoever else has payloads relying on this). You could make one cbfs_media only represent one region, but that's weird again because for the default media from the coreboot tables is also expects that whatever libpayload_init_default_cbfs_media() returns represents the whole flash. I think the long term good way to resolve this is to make the payload only provide low-level access to the whole flash and move FMAP integration into libpayload, similar to how coreboot does it (with explicit APIs to access the default CBFS, RO CBFS, or other named section). -- To view, visit https://review.coreboot.org/c/coreboot/+/56120 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6d693bdd4ddaf4c9b3cffb4ea9879c761200aca9 Gerrit-Change-Number: 56120 Gerrit-PatchSet: 4 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Fri, 09 Jul 2021 00:45:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: Revert "Makefile.inc: Drop the cbfs master header from non-X86"
by Arthur Heymans (Code Review) 09 Jul '21

09 Jul '21

Attention is currently required from: Hung-Te Lin, Martin Roth, Julius Werner, Yu-Ping Wu. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56130 ) Change subject: Revert "Makefile.inc: Drop the cbfs master header from non-X86" ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/56130/comment/958e8a88_e55fd8a8 PS1, Line 11: Breaks libpayload CBFS code when accessing : non-primary CBFS. > No, by "primary CBFS" I meant the RW CBFS in the case where vboot is booting from RW. I guess "primary" was the wrong term... libpayload calls this "default CBFS media", the one that's passed in the boot_media_params coreboot table entry. Ok thanks for clarifying. I see that there is FMAP support in libpayload. If the non-default CBFS media is on the COREBOOT fmap (the recovery path?) that could be used instead. I haven't look to deep into the code. Anyway lgtm and try to improve things later on. -- To view, visit https://review.coreboot.org/c/coreboot/+/56130 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id7f47406e6126f19e1fd6bc9d33c8c9d0cb9450d Gerrit-Change-Number: 56130 Gerrit-PatchSet: 2 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Comment-Date: Fri, 09 Jul 2021 00:09:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: sc7180: Add display support for mipi panels
by Julius Werner (Code Review) 09 Jul '21

09 Jul '21

Attention is currently required from: Vinod Polimera, Ravi kumar, Shelley Chen, Mars Chen, mturney mturney. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52662 ) Change subject: sc7180: Add display support for mipi panels ...................................................................... Patch Set 22: (1 comment) File src/mainboard/google/trogdor/mainboard.c: https://review.coreboot.org/c/coreboot/+/52662/comment/0db632fd_3c9d8020 PS22, Line 106: get_panel_config > You can append new panels info to the same panel_driver.c and attach it to the get_panel_config(). […] Well, the question is how get_panel_config() should know which panel to return. This should be decided based on SKU ID for each board. I think you should move panel_driver.c from src/soc/qualcomm into src/mainboard/google/trogdor, because the panels are entirely board-specific and not really related to the SoC. Then you should expand get_panel_config() to decide what to return based on board name and SKU ID, e.g. something like const struct panel_data *get_panel_config(struct edid *edid) { struct panel_data *info; if (CONFIG(BOARD_GOOGLE_HOMESTAR)) { memcpy(edid, &visionox_edid, sizeof(struct edid)); info = &visinox_panel_info; } else if (CONFIG(BOARD_GOOGLE_MRBLAND)) { switch (sku_id()) { case 1: memcpy(edid, &some_panel_edid, sizeof(struct edid)); info = &some_panel_info; case 2: memcpy(edid, &some_other_panel_edid, sizeof(struct edid)); info = &some_other_panel_info; ...etc... } } edid_set_framebuffer_bits_per_pixel(edid, 32, 0); return info; } (Names and numbers made up, I don't know what the exact IDs and panels are gonna be. Maybe don't add this code yet but at least that's what it should get expanded to when we add the second panel.) -- To view, visit https://review.coreboot.org/c/coreboot/+/52662 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id698265a4e2399ad1c26e026e9a5f8ecd305467f Gerrit-Change-Number: 52662 Gerrit-PatchSet: 22 Gerrit-Owner: Ravi kumar <rbokka(a)codeaurora.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: mturney mturney <mturney(a)codeaurora.org> Gerrit-CC: Mars Chen <chenxiangrui(a)huaqin.corp-partner.google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Ravi Kumar Bokka <c_rbokka(a)qualcomm.corp-partner.google.com> Gerrit-CC: Vinod Polimera <vpolimer(a)qualcomm.corp-partner.google.com> Gerrit-Attention: Vinod Polimera <vpolimer(a)qualcomm.corp-partner.google.com> Gerrit-Attention: Ravi kumar <rbokka(a)codeaurora.org> Gerrit-Attention: Shelley Chen <shchen(a)google.com> Gerrit-Attention: Mars Chen <chenxiangrui(a)huaqin.corp-partner.google.com> Gerrit-Attention: mturney mturney <mturney(a)codeaurora.org> Gerrit-Comment-Date: Fri, 09 Jul 2021 00:03:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Vinod Polimera <vpolimer(a)qualcomm.corp-partner.google.com> Comment-In-Reply-To: Mars Chen <chenxiangrui(a)huaqin.corp-partner.google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: Makefile.inc: Drop the cbfs master header from non-X86
by Arthur Heymans (Code Review) 09 Jul '21

09 Jul '21

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56120 ) Change subject: Makefile.inc: Drop the cbfs master header from non-X86 ...................................................................... Patch Set 4: (1 comment) Patchset: PS4: > Apologies for not noticing this before submission, but this is breaking payloads. libpayload CBFS code currently requires a master header (https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/pa…) to access anything but the primary CBFS (e.g. access the RO CBFS when booting with vboot in RW). The libpayload CBFS code is an ancient mess that's in dire need of a complete rewrite (and I was hoping to do that as part of my CBFS verification work some day, but have been swamped with other crap for months now :/ ), but for now it's the only thing we have so it needs to keep working. > There was a rewrite on gerrit: https://review.coreboot.org/c/coreboot/+/15774 but it was blocked due to licensing issues... > However, looking further, there are also payloads like SeaBIOS and Grub that do not link libpayload and instead maintain their own (ancient) CBFS code that tends to rely on a master header. I believe with all of our CBFS additions to date we've always made sure that the most basic coreboot build remains backwards-compatible to CBFS readers from 10+ years ago, which I think is kinda nice. This change would break that. Don't we wanna just keep the header there for those readers, I don't think it really costs us much? > SeaBIOS has a patch pending to not use it. I need to write one for grub, it's not much work. I think it's worth getting rid of the header as updating a hardcoded pointer at the top of the flash is not so nice + most options in the header are pretty meaningless, like cbfs file alignment. boot_media_params has been around for a long time now too, so updating payloads to not rely on it and dropping the header is my current plan. > I hope it's okay if I revert this patch to resolve breakages for the time being. It's ok. I'll see if it's easy to fix libpayload. -- To view, visit https://review.coreboot.org/c/coreboot/+/56120 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6d693bdd4ddaf4c9b3cffb4ea9879c761200aca9 Gerrit-Change-Number: 56120 Gerrit-PatchSet: 4 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Thu, 08 Jul 2021 23:56:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: Revert "Makefile.inc: Drop the cbfs master header from non-X86"
by Julius Werner (Code Review) 09 Jul '21

09 Jul '21

Attention is currently required from: Hung-Te Lin, Martin Roth, Yu-Ping Wu. Julius Werner has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/56130 ) Change subject: Revert "Makefile.inc: Drop the cbfs master header from non-X86" ...................................................................... Removed Verified-1 by build bot (Jenkins) <no-reply(a)coreboot.org> -- To view, visit https://review.coreboot.org/c/coreboot/+/56130 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id7f47406e6126f19e1fd6bc9d33c8c9d0cb9450d Gerrit-Change-Number: 56130 Gerrit-PatchSet: 2 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-MessageType: deleteVote

1 0

Change in coreboot[master]: Revert "Makefile.inc: Drop the cbfs master header from non-X86"
by Julius Werner (Code Review) 09 Jul '21

09 Jul '21

Attention is currently required from: Hung-Te Lin, Martin Roth, Yu-Ping Wu. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56130 ) Change subject: Revert "Makefile.inc: Drop the cbfs master header from non-X86" ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/56130/comment/bbd82c48_5b115d00 PS1, Line 11: Breaks libpayload CBFS code when accessing : non-primary CBFS. > So not having a master cbfs header in the primary CBFS breaks CBFS that never contain the header? Th […] No, by "primary CBFS" I meant the RW CBFS in the case where vboot is booting from RW. I guess "primary" was the wrong term... libpayload calls this "default CBFS media", the one that's passed in the boot_media_params coreboot table entry. -- To view, visit https://review.coreboot.org/c/coreboot/+/56130 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id7f47406e6126f19e1fd6bc9d33c8c9d0cb9450d Gerrit-Change-Number: 56130 Gerrit-PatchSet: 2 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Comment-Date: Thu, 08 Jul 2021 23:48:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit July 2021