Attention is currently required from: Stefan Reinauer, Paul Menzel, Angel Pons.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54943 )
Change subject: Add AmigaOS suppport to flashrom
......................................................................
Patch Set 1:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/54943/comment/94e8d71f_fb3d73c1
PS1, Line 8:
> How would I do this? Create a Dockerfile.amiga that contains a crosscompile environment?
If there is no way to compile it natively, exactly that. I was lazy with
the DJGPP Dockerfile and just used a published image. Maybe there is
one for AmigaOS too?
If you want to write one, I suggest to base it on a particular Debian
release, so we can get the same results for a few years (the idea of
Manibuilder includes local builds of all Docker images so developers
can investigate on their own without server access). Beside the tool-
chain a user `mani` and a checkout of flashrom in `/home/mani/flashrom`
is expected. And it needs the `mani-wrapper.sh`. I guess you could
copy all that from the DJGPP setup. Cross-compilation is a bit ugly
in the manibuilder/Makefile, but there DJGPP should serve as example
too :)
--
To view, visit https://review.coreboot.org/c/flashrom/+/54943
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I3e4a3885f88313aad019454fdbdf4be176b35330
Gerrit-Change-Number: 54943
Gerrit-PatchSet: 1
Gerrit-Owner: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Sat, 05 Jun 2021 12:50:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Ivan V has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54964 )
Change subject: platform: Fix endianness detection for Apple Silicon Macs
......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/54964/comment/7de13465_b1643227
PS1, Line 18: 3. __LITTLE_ENDIAN__ and __BIG_ENDIAN__ macros are used only
: for PowerPC architecture.
> This has confused me a little because it describes the flashrom […]
Yes, you're right. I could've worded it better.
--
To view, visit https://review.coreboot.org/c/flashrom/+/54964
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ifdb1523ee2c7023e657cfd7b823b091d5deef513
Gerrit-Change-Number: 54964
Gerrit-PatchSet: 2
Gerrit-Owner: Ivan V <root(a)pcm720.me>
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-Comment-Date: Sat, 05 Jun 2021 12:46:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Attention is currently required from: Ivan V.
Hello build bot (Jenkins), Nico Huber, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/54964
to look at the new patch set (#2).
Change subject: platform: Fix endianness detection for Apple Silicon Macs
......................................................................
platform: Fix endianness detection for Apple Silicon Macs
Building flashrom on Apple Silicon Macs fails with
"Unable to determine endianness" error. It seems that current
endianness detection fails on macOS due to a combination of
three issues:
1. On macOS, neither GCC nor Clang have __ARMEL__ macros used
by architecture-specific detection;
2. Generic detection fails because Apple uses LITTLE_ENDIAN,
BIG_ENDIAN and BYTE_ORDER macros instead of __BYTE_ORDER and
__LITTLE_ENDIAN;
3. In platform.h, __LITTLE_ENDIAN__ and __BIG_ENDIAN__ macros
are checked only for PowerPC architecture.
This error can be fixed by appending __LITTLE_ENDIAN__ and
__BIG_ENDIAN__ to conditions in IS_ARM branch. I've considered
multiple approaches, but this one seems the cleanest to me.
Signed-off-by: Ivan V <root(a)pcm720.me>
Change-Id: Ifdb1523ee2c7023e657cfd7b823b091d5deef513
---
M platform.h
1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/64/54964/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/54964
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ifdb1523ee2c7023e657cfd7b823b091d5deef513
Gerrit-Change-Number: 54964
Gerrit-PatchSet: 2
Gerrit-Owner: Ivan V <root(a)pcm720.me>
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: Ivan V <root(a)pcm720.me>
Gerrit-MessageType: newpatchset
Attention is currently required from: Ivan V.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54964 )
Change subject: platform: Fix endianness detection for Apple Silicon Macs
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/54964/comment/b656f61b_afb598da
PS1, Line 18: 3. __LITTLE_ENDIAN__ and __BIG_ENDIAN__ macros are used only
: for PowerPC architecture.
This has confused me a little because it describes the flashrom
code, not what Apple does ^^
Might suffice to change the verb, e.g. "[...] macros are checked [...]".
--
To view, visit https://review.coreboot.org/c/flashrom/+/54964
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ifdb1523ee2c7023e657cfd7b823b091d5deef513
Gerrit-Change-Number: 54964
Gerrit-PatchSet: 1
Gerrit-Owner: Ivan V <root(a)pcm720.me>
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: Ivan V <root(a)pcm720.me>
Gerrit-Comment-Date: Sat, 05 Jun 2021 12:34:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Angel Pons, Anastasia Klimchuk.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54748 )
Change subject: dummyflasher.c: Extract params processing into a separate function
......................................................................
Patch Set 4: Code-Review+1
(4 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/54748/comment/82e23e4b_a1ad3e53
PS4, Line 11: A good side-effect of the change is: 13 error paths of params
: processing are not leaking data anymore. All those error paths
: return 1 back to init function which frees data.
:
: And there was just one more error path in init function were free(data)
: needed to be added.
These are the actual changes. Everything else is a mere refactoring.
The summary line should cover this.
Extracting things into a separate function is just how the change
is made easier. Maybe worth mentioning but not what should be
emphasized.
https://review.coreboot.org/c/flashrom/+/54748/comment/57269786_87a3387c
PS4, Line 20:
Could add a
Fixes: 3b8fe0f8 (dummyflasher.c: Factor out global state)
File dummyflasher.c:
https://review.coreboot.org/c/flashrom/+/54748/comment/3c1a9478_139f6599
PS1, Line 26: /* Remove the #define below if you don't want SPI flash chip emulation. */
: #define EMULATE_SPI_CHIP 1
> This comment still valid, I think.
IMO, both defines should just be dropped. If they were compile-time configs
like the selection of programmer drivers to built in, we could at least build
test every combination. But if nobody needs different values anyway, just
drop them and all the #if?
File dummyflasher.c:
https://review.coreboot.org/c/flashrom/+/54748/comment/fcbfc2a0_cbe49ae4
PS4, Line 994: dummy_buses_supported
Makes one wonder why this is not part of `emu_data`...
--
To view, visit https://review.coreboot.org/c/flashrom/+/54748
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I04f55f77bb4703f1d88b2191c45a22be3c97bf87
Gerrit-Change-Number: 54748
Gerrit-PatchSet: 4
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: Namyoon Woo <namyoon(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Sat, 05 Jun 2021 08:30:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52958 )
Change subject: buspirate_spi.c: Refactor singleton states into reentrant pattern
......................................................................
Patch Set 3: Code-Review+1
(3 comments)
File buspirate_spi.c:
https://review.coreboot.org/c/flashrom/+/52958/comment/cd75ce9e_7512a654
PS3, Line 58: unsigned char **bp_commbuf
> I was considering the idea to pass a pointer to `struct bp_spi_data` into buspirate_commbuf_grow but […]
Well, it's roughly the same either way. Local variables and bp_data need to be
synchronized after a change. So after a call to buspirate_commbuf_grow(), it's
either
bp_commbuf = bp_data->bp_commbuf;
or
bp_data->bp_commbuf = bp_commbuf;
As `bp_commbufsize` is only consumed in buspirate_commbuf_grow() we could even
save us a local variable for it when passing `bp_data`. We'd have to switch
allocation of `bp_commbuf` and `bp_data`, though.
This can all change later, however. This patch looks good already.
https://review.coreboot.org/c/flashrom/+/52958/comment/88dacd1f_0f3a1441
PS3, Line 329: unsigned char *bp_commbuf = NULL;
: int bp_commbufsize = 0;
:
Nit, `NULL`/`0` should never be consumed, so the initializations are unnecessary.
https://review.coreboot.org/c/flashrom/+/52958/comment/e44066da_ca50916c
PS3, Line 695: /* Storing the latest values into data, commbuf migth have grown during init. */
: bp_data->bp_commbuf = bp_commbuf;
: bp_data->bp_commbufsize = bp_commbufsize;
In the light of the discussion above, this shouldn't be needed?
--
To view, visit https://review.coreboot.org/c/flashrom/+/52958
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I418bbfff15fb126b042fbc9be09dbf59f4d243b8
Gerrit-Change-Number: 52958
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: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Sat, 05 Jun 2021 07:25:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
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