April 2021 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: hwaccess.c: Fix PowerPC memory barrier
by Angel Pons (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Nico Huber, Stefan Reinauer, Angel Pons, Arthur Heymans. Hello build bot (Jenkins), Nico Huber, Stefan Reinauer, Edward O'Callaghan, Arthur Heymans, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/52470 to look at the new patch set (#3). Change subject: hwaccess.c: Fix PowerPC memory barrier ...................................................................... hwaccess.c: Fix PowerPC memory barrier Use `__asm__ __volatile__` as done in the Linux kernel. Also update rotten link. Tested using manibuilder, flashrom now builds on PowerPC with -std=c99. Change-Id: Ia84fd1362a1d39aca6269eef22ac994dee68ba4b Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M hwaccess.c 1 file changed, 2 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/70/52470/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/52470 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia84fd1362a1d39aca6269eef22ac994dee68ba4b Gerrit-Change-Number: 52470 Gerrit-PatchSet: 3 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: hwaccess.c: Fix PowerPC memory barrier
by Angel Pons (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Nico Huber, Stefan Reinauer, Angel Pons, Arthur Heymans. Hello build bot (Jenkins), Nico Huber, Stefan Reinauer, Edward O'Callaghan, Arthur Heymans, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/52470 to look at the new patch set (#2). Change subject: hwaccess.c: Fix PowerPC memory barrier ...................................................................... hwaccess.c: Fix PowerPC memory barrier Use `__asm__ __volatile__` as done in the Linux kernel. Also update rotten link. Tested using manibuilder. Change-Id: Ia84fd1362a1d39aca6269eef22ac994dee68ba4b Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M hwaccess.c 1 file changed, 2 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/70/52470/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/52470 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia84fd1362a1d39aca6269eef22ac994dee68ba4b Gerrit-Change-Number: 52470 Gerrit-PatchSet: 2 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: s25f.c: Fix mismatched function definitions
by Angel Pons (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Edward O'Callaghan, Arthur Heymans. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52473 ) Change subject: s25f.c: Fix mismatched function definitions ...................................................................... Patch Set 1: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/52473/comment/1f01a15c_7a3d6f14 PS1, Line 8: : This was missed because `uint32_t` is `unsigned int` in most cases. : However, it is not the case for DJGPP 6.1.0 for some reason. > Um, no. […] This issue appeared in CB:46140 between PS14 and PS15: https://review.coreboot.org/c/flashrom/+/46140/14..15/s25f.c#241 I have no idea why it got changed. -- To view, visit https://review.coreboot.org/c/flashrom/+/52473 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I656a72b85d4c70b57f6ff9268186a4a60933f8a9 Gerrit-Change-Number: 52473 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Tue, 20 Apr 2021 08:53:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: s25f.c: Fix mismatched function definitions
by Angel Pons (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Edward O'Callaghan, Arthur Heymans. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52473 ) Change subject: s25f.c: Fix mismatched function definitions ...................................................................... Patch Set 1: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/52473/comment/b4fca2f9_3e513208 PS1, Line 8: : This was missed because `uint32_t` is `unsigned int` in most cases. : However, it is not the case for DJGPP 6.1.0 for some reason. > this does not sit right with me. […] Um, no. The signature of these functions needs to match the type of the `block_erase` function pointer in `struct block_eraser` in `struct flashchip` in flash.h which uses `unsigned int` for `blockaddr` and `blocklen`: https://paste.flashrom.org/view.php?id=3462 -- To view, visit https://review.coreboot.org/c/flashrom/+/52473 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I656a72b85d4c70b57f6ff9268186a4a60933f8a9 Gerrit-Change-Number: 52473 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Tue, 20 Apr 2021 08:44:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: programmer.h,chipset_enable.c: Make penable fn ptr more descript
by Edward O'Callaghan (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52364 ) Change subject: programmer.h,chipset_enable.c: Make penable fn ptr more descript ...................................................................... Patch Set 2: (1 comment) File programmer.h: https://review.coreboot.org/c/flashrom/+/52364/comment/ce9c6381_e3846ea5 PS1, Line 228: enable_flash_xxx > Uh, I've never seen `_xxx` used that way. Example: https://github.com/torvalds/linux/blob/fcadab740480e0e0e9fa9bd272acd409884d… Happy to use another place holder identifier if you have a suggestion? -- To view, visit https://review.coreboot.org/c/flashrom/+/52364 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8b3a4ec51ea488c40c20ccafea883f8dea93577d Gerrit-Change-Number: 52364 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 20 Apr 2021 08:16:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Add unit test to run init/shutdown for mec1308.c
by Edward O'Callaghan (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 ) Change subject: tests: Add unit test to run init/shutdown for mec1308.c ...................................................................... Patch Set 6: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/51487 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc Gerrit-Change-Number: 51487 Gerrit-PatchSet: 6 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 20 Apr 2021 08:12:53 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Add unit test to run init/shutdown for mec1308.c
by Edward O'Callaghan (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 ) Change subject: tests: Add unit test to run init/shutdown for mec1308.c ...................................................................... Patch Set 6: (4 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/51487/comment/c855afa3_0827f182 PS6, Line 9: hwaccess_io.h : because those are needed to test lifecycle of mec1308.c I would like to see one other example of ported i/o usage being addressed with the same test code to build confidence that the approach is flexible enough. It maybe worth considering that there is sufficient complexity in trying to deal with ported i/o mocking that the mocking framework is its own patch. Patchset: PS1: > Thank you! I will try to address your other comment to create mocks in separate compilation unit. […] flashbuses_to_text_test_success() was only introduced to get the ball rolling on starting to think about how a test framework could even be wired into Flashrom. I think we are starting to get well past that point now that Anastasia has gotten the hang of cmocka and is making great progress. In light of that, I wouldn't be opposed to removing the flashbuses_to_text_test_success() test if its just a burden as our tests continue to mature. That is to say, I have no strong feelings about it now that we have more example test code and will let the democracy of the community decide upon the exemplification test of flashbuses_to_text_test_success() ultimate fate. Fundamentally I agree with Nico's assessment so I guess the above perhaps provides some historical context. File tests/tests.c: https://review.coreboot.org/c/flashrom/+/51487/comment/00941c11_1fb9a405 PS6, Line 88: /* : * Uncomment LOG_ME below for verbose logging. : * It is commented by default because of too many calls. : */ : /* LOG_ME; */ does cmocka have a mechanism to allow verbosity to be tuned while running a test? https://review.coreboot.org/c/flashrom/+/51487/comment/42525219_22b8fdfe PS6, Line 102: return ((port == 0x2e /* MEC1308_SIO_PORT1 */ : || port == 0x4e /* MEC1308_SIO_PORT2 */) : ? 0x20 /* MEC1308_DEVICE_ID_REG */ : : ((outb_val == 0x84 /* MEC1308_MBX_DATA_START */) : ? 0xaa /* MEC1308_CMD_PASSTHRU_SUCCESS */ : : 0)); such values and logic may want to be encoded and stashed away within mec1308_init_and_shutdown_test_success() but queried here. I guess will_return() and mock_type() are the tricks to use here? -- To view, visit https://review.coreboot.org/c/flashrom/+/51487 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc Gerrit-Change-Number: 51487 Gerrit-PatchSet: 6 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 20 Apr 2021 08:12:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: programmer.h,chipset_enable.c: Make penable fn ptr more descript
by Angel Pons (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52364 ) Change subject: programmer.h,chipset_enable.c: Make penable fn ptr more descript ...................................................................... Patch Set 2: (1 comment) File programmer.h: https://review.coreboot.org/c/flashrom/+/52364/comment/9883be4e_9556bd06 PS1, Line 228: enable_flash_xxx > I think I much prefer the current pattern as "_xxx" as, in programming 'xxx' is typically recognised […] Uh, I've never seen `_xxx` used that way. -- To view, visit https://review.coreboot.org/c/flashrom/+/52364 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8b3a4ec51ea488c40c20ccafea883f8dea93577d Gerrit-Change-Number: 52364 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 20 Apr 2021 08:06:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: lspcon, realtek_mst: move i2c bus resolution from params to i2c_helper
by Angel Pons (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Sam McNally, Peter Marheine. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52405 ) Change subject: lspcon, realtek_mst: move i2c bus resolution from params to i2c_helper ...................................................................... Patch Set 8: (2 comments) File i2c_helper_linux.c: https://review.coreboot.org/c/flashrom/+/52405/comment/d38ae2df_cd934ecb PS6, Line 42: char *dev = malloc(sizeof(I2C_DEV_PREFIX)); > It must because this ultimately gets returned from i2c_buspath_from_programmer_params where the leng […] I see. Let me try another approach... File i2c_helper_linux.c: https://review.coreboot.org/c/flashrom/+/52405/comment/80b279cc_2ea3b033 PS8, Line 48: { nit: another brace that needs to be moved -- To view, visit https://review.coreboot.org/c/flashrom/+/52405 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ieea57b2445f3aca16c87d8f839dff131009531a0 Gerrit-Change-Number: 52405 Gerrit-PatchSet: 8 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Tue, 20 Apr 2021 08:05:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: hwaccess.h: Split hwaccess.h and extract hwaccess_io.h out of it
by Edward O'Callaghan (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52499 ) Change subject: hwaccess.h: Split hwaccess.h and extract hwaccess_io.h out of it ...................................................................... Patch Set 3: Code-Review+1 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/52499/comment/3842d4d9_50b1f2c3 PS3, Line 10: . "by <insert mechanism here>." this helps others wanting to write tests also know how to leverage this change to achieve the same results. -- To view, visit https://review.coreboot.org/c/flashrom/+/52499 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Idd04c7b36b24e9da339348a015df4f43a03744f7 Gerrit-Change-Number: 52499 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 20 Apr 2021 07:51:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit April 2021