coreboot-gerrit December 2021

coreboot-gerrit@coreboot.org

1 participants
5525 discussions

Change in coreboot[master]: mb/google/guybrush: Configure EN_SPKR GPIO in PSP verstage
by Rob Barnes (Code Review) 02 Dec '21

02 Dec '21

Attention is currently required from: Paul Menzel, Jon Murphy, Karthik Ramasubramanian. Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59813 ) Change subject: mb/google/guybrush: Configure EN_SPKR GPIO in PSP verstage ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/59813 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9a52a167da9c7040731da5d355ec345fd9b13762 Gerrit-Change-Number: 59813 Gerrit-PatchSet: 2 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Jon Murphy <jpmurphy(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Rob Barnes <robbarnes(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jon Murphy <jpmurphy(a)google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Thu, 02 Dec 2021 00:34:43 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/guybrush: Configure EN_SPKR GPIO in PSP verstage
by Karthik Ramasubramanian (Code Review) 02 Dec '21

02 Dec '21

Attention is currently required from: Paul Menzel, Jon Murphy, Rob Barnes. Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59813 ) Change subject: mb/google/guybrush: Configure EN_SPKR GPIO in PSP verstage ...................................................................... Patch Set 2: (2 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/59813/comment/755e4c82_e0bc8c31 PS1, Line 13: mechanical off(G3) > Please add a space before the (. Done https://review.coreboot.org/c/coreboot/+/59813/comment/26a9b4b7_39d1329c PS1, Line 17: EN_SPKR GPIO(in S5 domain) > Please add a space before the (. Done -- To view, visit https://review.coreboot.org/c/coreboot/+/59813 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9a52a167da9c7040731da5d355ec345fd9b13762 Gerrit-Change-Number: 59813 Gerrit-PatchSet: 2 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Jon Murphy <jpmurphy(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Rob Barnes <robbarnes(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jon Murphy <jpmurphy(a)google.com> Gerrit-Attention: Rob Barnes <robbarnes(a)google.com> Gerrit-Comment-Date: Thu, 02 Dec 2021 00:33:41 +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]: mb/google/guybrush: Configure EN_SPKR GPIO in PSP verstage
by Karthik Ramasubramanian (Code Review) 02 Dec '21

02 Dec '21

Attention is currently required from: Jon Murphy, Rob Barnes, Karthik Ramasubramanian. Hello build bot (Jenkins), Raul Rangel, Paul Menzel, Jon Murphy, Rob Barnes, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/59813 to look at the new patch set (#2). Change subject: mb/google/guybrush: Configure EN_SPKR GPIO in PSP verstage ...................................................................... mb/google/guybrush: Configure EN_SPKR GPIO in PSP verstage EN_SPKR GPIO is used as a multiplexer select signal between RAM_ID straps and Developer Mode Beep signals. During boot up it is LOW and selects RAM_ID straps. When the system enters OS, it is driven HIGH and selects DEV BEEP signals. Since in some boards, the GPIO chosen is in S5 domain it does not reset until the system enters mechanical off (G3) state. On scenarios where the power button is pressed when the system is in S5, incorrect RAM_ID strap is being read because the EN_SPKR is still selecting DEV BEEP signal. This causes boot up failures. Fix this by configuring the EN_SPKR GPIO (in S5 domain) explicitly in PSP verstage. BUG=b:204450368 TEST=Build and boot to OS in Guybrush. Perform suspend-resume cycle followed by a S5 -> S0 boot cycle for 2 iterations successfully. Signed-off-by: Karthikeyan Ramasubramanian <kramasub(a)google.com> Change-Id: I9a52a167da9c7040731da5d355ec345fd9b13762 --- M src/mainboard/google/guybrush/variants/guybrush/gpio.c M src/mainboard/google/guybrush/variants/nipperkin/gpio.c 2 files changed, 4 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/59813/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/59813 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9a52a167da9c7040731da5d355ec345fd9b13762 Gerrit-Change-Number: 59813 Gerrit-PatchSet: 2 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Jon Murphy <jpmurphy(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Rob Barnes <robbarnes(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jon Murphy <jpmurphy(a)google.com> Gerrit-Attention: Rob Barnes <robbarnes(a)google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: guybrush: add RO_GSCVD area to FMAP
by Julius Werner (Code Review) 02 Dec '21

02 Dec '21

Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/57864 ) Change subject: guybrush: add RO_GSCVD area to FMAP ...................................................................... guybrush: add RO_GSCVD area to FMAP This area is used for storing AP RO verification information. BRANCH=none BUG=b:141191727 TEST=built a guybrush firmware image and verified that the RO_GSCVD area was indeed added: $ dump_fmap /build/guybrush/firmware/image-guybrush.bin | \ grep -B3 RO_GSCVD area: 25 area_offset: 0x00808000 area_size: 0x00002000 (8192) area_name: RO_GSCVD $ - verified that guybrush device boots fine with the new image. Change-Id: Ifa24d5a6271a8bcbf737d4580ec85b9cfdd9af01 Signed-off-by: Vadim Bendebury <vbendeb(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/coreboot/+/57864 Reviewed-by: Julius Werner <jwerner(a)chromium.org> Reviewed-by: Karthik Ramasubramanian <kramasub(a)google.com> Reviewed-by: Raul Rangel <rrangel(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M src/mainboard/google/guybrush/chromeos.fmd 1 file changed, 1 insertion(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Vadim Bendebury: Looks good to me, but someone else must approve Julius Werner: Looks good to me, but someone else must approve Raul Rangel: Looks good to me, approved Karthik Ramasubramanian: Looks good to me, approved diff --git a/src/mainboard/google/guybrush/chromeos.fmd b/src/mainboard/google/guybrush/chromeos.fmd index 3875700..a6dbc2c 100644 --- a/src/mainboard/google/guybrush/chromeos.fmd +++ b/src/mainboard/google/guybrush/chromeos.fmd @@ -22,6 +22,7 @@ RW_LEGACY(CBFS) WP_RO@8M 8M { RO_VPD(PRESERVE) 16K + RO_GSCVD 8K RO_SECTION { FMAP 2K RO_FRID 64 -- To view, visit https://review.coreboot.org/c/coreboot/+/57864 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa24d5a6271a8bcbf737d4580ec85b9cfdd9af01 Gerrit-Change-Number: 57864 Gerrit-PatchSet: 3 Gerrit-Owner: Vadim Bendebury <vbendeb(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan.m.shaikh(a)gmail.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Mathew King <mathewk(a)chromium.org> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Rob Barnes <robbarnes(a)google.com> Gerrit-Reviewer: Vadim Bendebury <vbendeb(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: guybrush: add RO_GSCVD area to FMAP
by Julius Werner (Code Review) 02 Dec '21

02 Dec '21

Attention is currently required from: Hung-Te Lin, Vadim Bendebury, Furquan Shaikh, Mathew King, Paul Menzel, Rob Barnes. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57864 ) Change subject: guybrush: add RO_GSCVD area to FMAP ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > For upstream tree, it is manually submitted after 24 hours of uploading the latest patchset - provid […] ...and remember that all comments need to be resolved. -- To view, visit https://review.coreboot.org/c/coreboot/+/57864 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifa24d5a6271a8bcbf737d4580ec85b9cfdd9af01 Gerrit-Change-Number: 57864 Gerrit-PatchSet: 2 Gerrit-Owner: Vadim Bendebury <vbendeb(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan.m.shaikh(a)gmail.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Mathew King <mathewk(a)chromium.org> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Rob Barnes <robbarnes(a)google.com> Gerrit-Reviewer: Vadim Bendebury <vbendeb(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Vadim Bendebury <vbendeb(a)chromium.org> Gerrit-Attention: Furquan Shaikh <furquan.m.shaikh(a)gmail.com> Gerrit-Attention: Mathew King <mathewk(a)chromium.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Rob Barnes <robbarnes(a)google.com> Gerrit-Comment-Date: Wed, 01 Dec 2021 23:34:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Vadim Bendebury <vbendeb(a)chromium.org> Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc: Add dram information to cbmem
by Julius Werner (Code Review) 02 Dec '21

02 Dec '21

Attention is currently required from: Ravi kumar, Shelley Chen, mturney mturney. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59195 ) Change subject: soc: Add dram information to cbmem ...................................................................... Patch Set 6: (1 comment) File src/soc/qualcomm/common/qclib.c: https://review.coreboot.org/c/coreboot/+/59195/comment/1dd8f696_7d1a3438 PS5, Line 174: mem_chip_info > Since CB isn't passing data into QcLib, would it be better to pass NULL (address) and 0 (size) as th […] Okay, so you're ignoring the pointer coreboot passes in the table, and having QcLib overwrite it with a pointer to somewhere within its own .data section. That also works, although it wasn't what I was thinking. In that case, yes, please just pass NULL, 0 to clarify that coreboot isn't passing anything in here. There's no need for a global variable then. It would be good to have some clear documentation on how all of this works, because there are a lot of implicit agreements attached to each interface table entry name now. Please add a comment here to explain that QcLib will write back a pointer to an internal buffer with the data, and also update that qclib-interface.md document with clear explanations on how each interface table entry is supposed to work. -- To view, visit https://review.coreboot.org/c/coreboot/+/59195 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0f1dd05ee224bf8284661c0afaa01d0a9d71daa7 Gerrit-Change-Number: 59195 Gerrit-PatchSet: 6 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-Reviewer: mturney mturney <quic_mturney(a)quicinc.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Ravi Kumar Bokka <c_rbokka(a)qualcomm.corp-partner.google.com> Gerrit-Attention: Ravi kumar <rbokka(a)codeaurora.org> Gerrit-Attention: Shelley Chen <shchen(a)google.com> Gerrit-Attention: mturney mturney <mturney(a)codeaurora.org> Gerrit-Attention: mturney mturney <quic_mturney(a)quicinc.com> Gerrit-Comment-Date: Wed, 01 Dec 2021 23:33:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Comment-In-Reply-To: mturney mturney <quic_mturney(a)quicinc.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: lib: Add a mutex
by Julius Werner (Code Review) 02 Dec '21

02 Dec '21

Attention is currently required from: Raul Rangel, Kyösti Mälkki, Rob Barnes, Karthik Ramasubramanian. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59320 ) Change subject: lib: Add a mutex ...................................................................... Patch Set 9: (2 comments) File src/include/mutex.h: https://review.coreboot.org/c/coreboot/+/59320/comment/8bd9a35a_879c2c8f PS9, Line 62: assert(__atomic_load_1(&mutex->locked, __ATOMIC_RELAXED)); I'm not sure if we actually want to inline these, since assert() takes a notable amount of instructions (and a big string constant where I'm not sure if it manages to deduplicate that for an inline function). My idea was to have mutex_lock()/mutex_unlock() static inline, but keep these other ones in the .c file. File src/include/mutex.h: https://review.coreboot.org/c/coreboot/+/59320/comment/199eca96_b172837a PS6, Line 32: } > AMD x86 romstages run APs. Oh okay, I didn't know that. But then there needs to be a Kconfig for that, or we need to explicitly check for those platforms in the rules macros. We shouldn't have all x86 platforms include spinlock code in romstage if only some AMD boards need it. > Say you do some PCI configuration register read-modify-write ops. Would you put a spinlock or mutex around it? I would use a spinlock I think that should probably be a mutex, honestly... I don't think we'd want to get into the business of having to analyze exactly which sequences of code can yield() and which can't. But does it happen often that you need to guard a single register access? I would assume most of the time there's just a big lock for every PCI device, and then the driver takes that at the start of the function and releases it at the end. > If you have SMP=y is it really worth the trouble to drop the atomicity for stages where you don't really need it? I am asking if it is really worth having _mutex_{lock/unlock]_coop() implemented separately. A better optimisation might be to have spinlock implememntations, here _mutex_(un)lock_{coop,smp} always inlined. Is there any cost to it? Using different code paths with compile-time checks is free. Sure, it only saves a few instructions, but why shouldn't we do that if we can? (Mostly, we can turn the whole thing into a noop in platforms/stages where there's neither SMP nor COOP, which I think is worthwhile... with some platforms' pre-RAM stages, every byte of binary size help.) -- To view, visit https://review.coreboot.org/c/coreboot/+/59320 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I41e02a54a17b1f6513b36a0274e43fc715472d78 Gerrit-Change-Number: 59320 Gerrit-PatchSet: 9 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Rob Barnes <robbarnes(a)google.com> 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-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Attention: Rob Barnes <robbarnes(a)google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Wed, 01 Dec 2021 23:23:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Comment-In-Reply-To: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: region: Rename rdev_readat_full to rdev_read_full
by Raul Rangel (Code Review) 01 Dec '21

01 Dec '21

Attention is currently required from: Julius Werner. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59820 ) Change subject: region: Rename rdev_readat_full to rdev_read_full ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/59820 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I60d595f0cbd161df171eaa4a76c7a00b6377e2b0 Gerrit-Change-Number: 59820 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Comment-Date: Wed, 01 Dec 2021 22:56:36 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common: Add support for CSE IOM/NPHY sub-parition update
by Julius Werner (Code Review) 01 Dec '21

01 Dec '21

Attention is currently required from: Tim Wawrzynczak, Sridhar Siricilla, Krishna P Bhat D. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59685 ) Change subject: soc/intel/common: Add support for CSE IOM/NPHY sub-parition update ...................................................................... Patch Set 13: (9 comments) Patchset: PS11: > Do to urgency of this feature, we would like to update this in a subsequent patch, as it would requi […] Well, what would you prefer: that you make this change now and test it on your hardware, or that I make this change in your code later blindly because I have no hardware to test it on? Besides, "we need this urgently" should never be a valid reason to reject review feedback on coreboot ToT. If individual companies have schedules to keep, they're free to build their temporary code in local branches while the upstream discussion is still ongoing. There should never be a need to rush unfinished code into the master branch. Honestly, looking closer at what you're doing here, cbfs_locate_file_in_region() is absolutely not the right approach for that to begin with anyway. You're not trying to access a custom CBFS (like the other case that's looking into the actual CSE FMAP section), you're just reading the default boot CBFS here. You're always supposed to use the main cbfs_load()/cbfs_map() APIs for that, not manually dig through vboot internals and hardcode the matching FMAP sections yourself. Here, I'll point out in other comments how this code should be changed... File src/soc/intel/common/block/cse/cse_lite.c: https://review.coreboot.org/c/coreboot/+/59685/comment/4dd7a9fd_86fa5752 PS13, Line 406: __weak bool is_cse_sub_part_update_req(void) This doesn't seem like a good use case for weak functions (see CB:58934 for some ongoing discussion about where weak functions are appropriate and where they aren't). You already have a Kconfig for this feature, so it doesn't make sense to default to not using it on boards that have that Kconfig enabled unless they also implement some extra function. I would either flip this to default to doing the update (i.e. name it `bool skip_cse_sub_part_update()` and the update always happens on platforms with this Kconfig unless they explicitly implement this function and return true from it), or make the function non-weak so that every platform with the Kconfig is forced to implement it. https://review.coreboot.org/c/coreboot/+/59685/comment/b40264dc_14b8c1d4 PS13, Line 755: static ssize_t rdev_readat_offset(const struct region_device *rd, void *b, size_t offset, Why are you redefining a well-known existing function with a different name, anyway? All this achieves is confuse people who are used to the existing name. Just use rdev_readat() directly. https://review.coreboot.org/c/coreboot/+/59685/comment/2a2478c8_89993bfa PS13, Line 833: static const char *get_source_rdev_fmap_name(void) Get rid of this function. Don't reimplement what vboot and the CBFS core are already doing for you. https://review.coreboot.org/c/coreboot/+/59685/comment/1647e9cd_48e08e05 PS13, Line 854: static bool cse_sub_part_get_source_rdev(struct region_device *rdev, const char *name) Get rid of this function. The new CBFS API intentionally abstracts away from raw rdevs. Just map the whole file and use a pointer to that. https://review.coreboot.org/c/coreboot/+/59685/comment/3b25b3ec_db33acb9 PS13, Line 932: if (!cse_sub_part_get_source_rdev(&source_rdev, name)) If you want to get the `name` CBFS file from the currently active boot CBFS, just use size_t size; void *subpart_cbfs_rw = cbfs_map(name, &size); if (!subpart_cbfs_rw) return ...; https://review.coreboot.org/c/coreboot/+/59685/comment/f7de642b_08a71bcb PS13, Line 936: if (!cse_get_sub_part_fw_version(type, &source_rdev, &source_fw_ver)) Just pass the `subpart_cbfs_rw` pointer returned from the above in here, and add the appropriate offsets onto it when you dereference. https://review.coreboot.org/c/coreboot/+/59685/comment/b05f97af_e66f8c8b PS13, Line 983: region_device_sz(&source_rdev) Here you would just use the `size` variable that got filled out above by cbfs_map(). https://review.coreboot.org/c/coreboot/+/59685/comment/bdc3a107_563405a4 PS13, Line 989: rdev_munmap(&source_rdev, subpart_cbfs_rw); ...and here you would use cbfs_unmap(subpart_cbfs_rw); instead. -- To view, visit https://review.coreboot.org/c/coreboot/+/59685 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7c0cda51314c4f722f5432486a43e19b46f4b240 Gerrit-Change-Number: 59685 Gerrit-PatchSet: 13 Gerrit-Owner: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com> Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Reviewer: Kane Chen <kane.chen(a)intel.com> Gerrit-Reviewer: Marx Wang <marx.wang(a)intel.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anfernee Chen <anfernee_chen(a)wistron.corp-partner.google.com> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-CC: Mark Hsieh <mark_hsieh(a)wistron.corp-partner.google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com> Gerrit-Comment-Date: Wed, 01 Dec 2021 22:53:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Comment-In-Reply-To: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: region: Rename rdev_readat_full to rdev_read_full
by Julius Werner (Code Review) 01 Dec '21

01 Dec '21

Attention is currently required from: Raul Rangel. Hello Raul Rangel, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/59820 to review the following change. Change subject: region: Rename rdev_readat_full to rdev_read_full ...................................................................... region: Rename rdev_readat_full to rdev_read_full The 'at' part of the name refers to starting to read from a specific offset, so it doesn't make sense for the 'full' version of the function. Signed-off-by: Julius Werner <jwerner(a)chromium.org> Change-Id: I60d595f0cbd161df171eaa4a76c7a00b6377e2b0 --- M src/commonlib/include/commonlib/region.h M src/lib/cbfs.c 2 files changed, 2 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/59820/1 diff --git a/src/commonlib/include/commonlib/region.h b/src/commonlib/include/commonlib/region.h index 5d73d9e..25efcc8 100644 --- a/src/commonlib/include/commonlib/region.h +++ b/src/commonlib/include/commonlib/region.h @@ -163,7 +163,7 @@ * * You must ensure the buffer is large enough to hold the full region_device. */ -static inline ssize_t rdev_readat_full(const struct region_device *rd, void *b) +static inline ssize_t rdev_read_full(const struct region_device *rd, void *b) { return rdev_readat(rd, b, 0, region_device_sz(rd)); } diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index 600d259..b4b6eb1 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -316,7 +316,7 @@ { struct cbfs_preload_context *context = arg; - if (rdev_readat_full(&context->rdev, context->buffer) < 0) { + if (rdev_read_full(&context->rdev, context->buffer) < 0) { ERROR("%s(name='%s') readat failed\n", __func__, context->name); return CB_ERR; } -- To view, visit https://review.coreboot.org/c/coreboot/+/59820 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I60d595f0cbd161df171eaa4a76c7a00b6377e2b0 Gerrit-Change-Number: 59820 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-MessageType: newchange

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit December 2021