December 2021 - flashrom-gerrit

Change in flashrom[master]: ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops
by Michał Żygowski (Code Review) 17 Dec '21

17 Dec '21

Attention is currently required from: Felix Singer, Nico Huber, Paul Menzel, Michael Niewöhner, Sergii Dmytruk. Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 ) Change subject: ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops ...................................................................... Patch Set 24: (1 comment) File ite_ecfw.c: https://review.coreboot.org/c/flashrom/+/55715/comment/4831a1e6_b6ee3984 PS20, Line 332: if (blocks_1_2) { : itestring->flags.clock_source = CLOCK_SOURCE_INTERNAL; : > Maybe, maybe not :S Let's keep it for now but add a comment The datasheet also says the clock source bit should be always 1. The only path that selects external source is the autoload off when the flash size is larger than 128k. Maybe later we can force the internal oscillator in all paths and remove the comment... -- To view, visit https://review.coreboot.org/c/flashrom/+/55715 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0e42260155ffea38a6f60790871cd8da7b657031 Gerrit-Change-Number: 55715 Gerrit-PatchSet: 24 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Fri, 17 Dec 2021 13:32:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: hwaccess: move x86 port I/O related code into own files
by Anastasia Klimchuk (Code Review) 17 Dec '21

17 Dec '21

Attention is currently required from: Nico Huber, Thomas Heijligen, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60110 ) Change subject: hwaccess: move x86 port I/O related code into own files ...................................................................... Patch Set 5: Code-Review+1 (1 comment) File Makefile: https://review.coreboot.org/c/flashrom/+/60110/comment/606202e6_116f9fdd PS4, Line 783: ifneq ($(NEED_RAW_ACCESS), ) : # Raw memory, MSR or PCI port I/O access. : FEATURE_CFLAGS += -D'NEED_RAW_ACCESS=1' -D'__FLASHROM_HAVE_OUTB__=1' : PROGRAMMER_OBJS += physmap.o hwaccess.o hwaccess_x86_io.o > `make CONFIG_ENABLE_LIBPCI_PROGRAMMERS=no CONFIG_RAYER_SPI=no` build successful on x86_64 Linux and […] Thank you for explanation! -- To view, visit https://review.coreboot.org/c/flashrom/+/60110 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I372b4a409f036da766c42bc406b596bc41b0f75a Gerrit-Change-Number: 60110 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> 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: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Fri, 17 Dec 2021 10:14:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: hwaccess: move x86 port I/O related code into own files
by Thomas Heijligen (Code Review) 17 Dec '21

17 Dec '21

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60110 ) Change subject: hwaccess: move x86 port I/O related code into own files ...................................................................... Patch Set 5: (1 comment) File Makefile: https://review.coreboot.org/c/flashrom/+/60110/comment/6e467f4f_742e6516 PS4, Line 783: ifneq ($(NEED_RAW_ACCESS), ) : # Raw memory, MSR or PCI port I/O access. : FEATURE_CFLAGS += -D'NEED_RAW_ACCESS=1' -D'__FLASHROM_HAVE_OUTB__=1' : PROGRAMMER_OBJS += physmap.o hwaccess.o hwaccess_x86_io.o > > Yes, currently it's disabling all libpci programmer and rayer_spi. […] `make CONFIG_ENABLE_LIBPCI_PROGRAMMERS=no CONFIG_RAYER_SPI=no` build successful on x86_64 Linux and hwaccess* files are not build as expected. NEED_RAW_ACCESS is an internal make variable. This should not be overwritten by the user. So `make NEED_RAW_ACCESS=` is not supported. The user should select the programmers to build and the Makefile selects all necessary features for them or disable the programmer when it's not supported on the platform. -- To view, visit https://review.coreboot.org/c/flashrom/+/60110 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I372b4a409f036da766c42bc406b596bc41b0f75a Gerrit-Change-Number: 60110 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> 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: Fri, 17 Dec 2021 09:33:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom,writeprotect: add flashrom_wp_{read,write}_chip_config()
by Nico Huber (Code Review) 17 Dec '21

17 Dec '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58479 ) Change subject: libflashrom,writeprotect: add flashrom_wp_{read,write}_chip_config() ...................................................................... Patch Set 22: (1 comment) File libflashrom.h: https://review.coreboot.org/c/flashrom/+/58479/comment/a9b4adb1_f81b57e0 PS22, Line 131: const struct flashrom_flashctx *, struct flashrom_wp_chip_config * Something about APIs in general. We are used to have the destination argument first. Just like in an assignment where the destination is on the left-hand side. This also applies to functions added later. Also to internal functions but we can make exceptions if there is a good reason. -- To view, visit https://review.coreboot.org/c/flashrom/+/58479 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3ad25708c3321b8fb0216c3eaf6ffc07616537ad Gerrit-Change-Number: 58479 Gerrit-PatchSet: 22 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: 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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Fri, 17 Dec 2021 01:02:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: hwaccess: move x86 port I/O related code into own files
by Anastasia Klimchuk (Code Review) 17 Dec '21

17 Dec '21

Attention is currently required from: Nico Huber, Thomas Heijligen, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60110 ) Change subject: hwaccess: move x86 port I/O related code into own files ...................................................................... Patch Set 5: (1 comment) File Makefile: https://review.coreboot.org/c/flashrom/+/60110/comment/1565d87a_103a872c PS4, Line 783: ifneq ($(NEED_RAW_ACCESS), ) : # Raw memory, MSR or PCI port I/O access. : FEATURE_CFLAGS += -D'NEED_RAW_ACCESS=1' -D'__FLASHROM_HAVE_OUTB__=1' : PROGRAMMER_OBJS += physmap.o hwaccess.o hwaccess_x86_io.o > Yes, currently it's disabling all libpci programmer and rayer_spi. is it realistic to check that such a case builds? does it matter? or maybe you did it already > Building with `make NEED_RAW_ACCESS=` fails during linking. Maybe I am missing something, why this is fine? was it always like this, building `make NEED_RAW_ACCESS=` never intent to work? -- To view, visit https://review.coreboot.org/c/flashrom/+/60110 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I372b4a409f036da766c42bc406b596bc41b0f75a Gerrit-Change-Number: 60110 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> 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: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Fri, 17 Dec 2021 01:01:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom,writeprotect: add flashrom_wp_{read,write}_chip_config()
by Nico Huber (Code Review) 17 Dec '21

17 Dec '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58479 ) Change subject: libflashrom,writeprotect: add flashrom_wp_{read,write}_chip_config() ...................................................................... Patch Set 22: (1 comment) File writeprotect.c: https://review.coreboot.org/c/flashrom/+/58479/comment/6957c4a1_1c2f2ed3 PS22, Line 56: struct wp_chip_config *cfg Only realized this now: this requires the caller to know the size of `struct wp_chip_config` which is currently not given by `libflashrom.h`. IMO, we should use dynamic allocation instead, like in the rest of the API. It will require yet another de-allocation function, but having a clean, consistent API seems worth it. -- To view, visit https://review.coreboot.org/c/flashrom/+/58479 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3ad25708c3321b8fb0216c3eaf6ffc07616537ad Gerrit-Change-Number: 58479 Gerrit-PatchSet: 22 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: 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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Fri, 17 Dec 2021 00:56:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom,writeprotect: add flashrom_wp_get_range()
by Nico Huber (Code Review) 17 Dec '21

17 Dec '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59183 ) Change subject: libflashrom,writeprotect: add flashrom_wp_get_range() ...................................................................... Patch Set 15: (1 comment) File writeprotect.c: https://review.coreboot.org/c/flashrom/+/59183/comment/7fa716a4_c684d1ae PS15, Line 189: decode_range We should check here if the pointer is set. -- To view, visit https://review.coreboot.org/c/flashrom/+/59183 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5a1dfcf384166b1bac319d286306747e1dcaa000 Gerrit-Change-Number: 59183 Gerrit-PatchSet: 15 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: 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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Fri, 17 Dec 2021 00:49:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: hwaccess: move x86 port I/O related code into own files
by Thomas Heijligen (Code Review) 16 Dec '21

16 Dec '21

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60110 ) Change subject: hwaccess: move x86 port I/O related code into own files ...................................................................... Patch Set 5: (2 comments) File Makefile: https://review.coreboot.org/c/flashrom/+/60110/comment/c071fe9e_9a2c1f8f PS4, Line 783: ifneq ($(NEED_RAW_ACCESS), ) : # Raw memory, MSR or PCI port I/O access. : FEATURE_CFLAGS += -D'NEED_RAW_ACCESS=1' -D'__FLASHROM_HAVE_OUTB__=1' : PROGRAMMER_OBJS += physmap.o hwaccess.o hwaccess_x86_io.o > Mostly curiosity: what is the way to build with NEED_RAW_ACCESS empty? Would that be disabling all t […] Yes, currently it's disabling all libpci programmer and rayer_spi. Building with `make NEED_RAW_ACCESS=` fails during linking. On x86 Linux NEED_RAW_ACCESS is enabled by CONFIG_RAYER_SPI which builds by default and all libcpi programmer when libpci is found File tests/hwaccess_x86_io_unittest.h: https://review.coreboot.org/c/flashrom/+/60110/comment/f790396a_9fbacab0 PS4, Line 33: int rget_io_perms(void); > If you could move it below all defines and includes (right above iopl)? thanks! Done -- To view, visit https://review.coreboot.org/c/flashrom/+/60110 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I372b4a409f036da766c42bc406b596bc41b0f75a Gerrit-Change-Number: 60110 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> 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: Thu, 16 Dec 2021 20:39:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: hwaccess: move x86 port I/O related code into own files
by Thomas Heijligen (Code Review) 16 Dec '21

16 Dec '21

Attention is currently required from: Nico Huber, Thomas Heijligen, Angel Pons. Hello build bot (Jenkins), Nico Huber, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/60110 to look at the new patch set (#5). Change subject: hwaccess: move x86 port I/O related code into own files ...................................................................... hwaccess: move x86 port I/O related code into own files Allow port I/O related code to be compiled independent from memory mapping functionality. This enables for a better selection of needed hardware access types. Change-Id: I372b4a409f036da766c42bc406b596bc41b0f75a Signed-off-by: Thomas Heijligen <thomas.heijligen(a)secunet.com> --- M Makefile M amd_imc.c M atahpt.c M atapromise.c M atavia.c M board_enable.c M chipset_enable.c M drkaiser.c M gfxnvidia.c M hwaccess.c M hwaccess.h A hwaccess_x86_io.c M hwaccess_x86_io.h M internal.c M it8212.c M it85spi.c M it87spi.c M meson.build M nic3com.c M nicintel.c M nicintel_eeprom.c M nicintel_spi.c M nicnatsemi.c M nicrealtek.c M ogp_spi.c M programmer.h M rayer_spi.c M satamv.c M satasii.c M tests/hwaccess_x86_io_unittest.h M wbsio_spi.c 31 files changed, 118 insertions(+), 69 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/10/60110/5 -- To view, visit https://review.coreboot.org/c/flashrom/+/60110 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I372b4a409f036da766c42bc406b596bc41b0f75a Gerrit-Change-Number: 60110 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> 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: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: flashchips,writeprotect_ranges: add range decoding function
by Nico Huber (Code Review) 16 Dec '21

16 Dec '21

Attention is currently required from: Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58480 ) Change subject: flashchips,writeprotect_ranges: add range decoding function ...................................................................... Patch Set 23: (14 comments) File flash.h: https://review.coreboot.org/c/flashrom/+/58480/comment/1bea2530_6e62729d PS23, Line 203: range Says less than the type, can be dropped. https://review.coreboot.org/c/flashrom/+/58480/comment/b92e456e_f036e317 PS23, Line 203: const struct flashrom_wp_chip_config *cfg, uint32_t chip_len It seems this is exactly the input we need for the `w25` implementation. But will it suit all future implementations? Should we pass the full flash definition or even context instead? https://review.coreboot.org/c/flashrom/+/58480/comment/8ebd4319_a8d1f009 PS23, Line 203: typedef Why have a typedef for it? All the other function pointers in `struct flashchip` don't have/need one? https://review.coreboot.org/c/flashrom/+/58480/comment/959f09d8_7739a02e PS23, Line 203: uint32_t Please don't hard-code fixed-width types. `unsigned int` should work as well, `size_t` too, and for most flexibility there is `chipsize_t` in `layout.h`. I don't promote the latter much because I'm not sure if it's really necessary. Maybe it would become handy if we'd ever encounter >4GiB chips (doesn't seem that far away anymore ^^). https://review.coreboot.org/c/flashrom/+/58480/comment/39774cfd_1376078a PS23, Line 203: cfg Says less than the type already does, can be dropped. File libflashrom.h: https://review.coreboot.org/c/flashrom/+/58480/comment/2962edc7_3661c590 PS23, Line 134: }; flashrom_flash_getsize() uses `size_t`? Should this follow? File writeprotect_ranges.c: https://review.coreboot.org/c/flashrom/+/58480/comment/0d66a75e_b3d9880c PS23, Line 19: Line break, please. https://review.coreboot.org/c/flashrom/+/58480/comment/3a440c7f_d7844f7e PS23, Line 21: TODO Not sure if it's worth a TODO. FWIW, this kind of TODO will just stick. https://review.coreboot.org/c/flashrom/+/58480/comment/9d461a5a_95010486 PS23, Line 27: 32 Why 32? https://review.coreboot.org/c/flashrom/+/58480/comment/c663f78d_4834b28b PS23, Line 29: r( Missing space. https://review.coreboot.org/c/flashrom/+/58480/comment/26fd2df0_486e8c71 PS23, Line 34: cfg->bp Isn't this an implicit pointer? Looks like GCC doesn't warn if compared to 0. Why does it work? I guess `coeff` overflows and is 0 ;) https://review.coreboot.org/c/flashrom/+/58480/comment/a5d85173_90597189 PS23, Line 50: 4 * 1024 4*KiB https://review.coreboot.org/c/flashrom/+/58480/comment/b28bd977_107e19e3 PS23, Line 51: 64 * 1024 64*KiB https://review.coreboot.org/c/flashrom/+/58480/comment/ff061593_9ade5f1f PS23, Line 85: && range->len > 0 Does it matter? i.e. where's the start of an empty range? ^^ -- To view, visit https://review.coreboot.org/c/flashrom/+/58480 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id163ed80938a946a502ed116e48e8236e36eb203 Gerrit-Change-Number: 58480 Gerrit-PatchSet: 23 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: 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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Thu, 16 Dec 2021 17:43:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit December 2021