October 2022 - flashrom-gerrit

[S] Change in flashrom[master]: flashchips: Add write protect bits to W25Q64JW...M
by Evan Benn (Code Review) 04 Oct '22

04 Oct '22

Attention is currently required from: Edward O'Callaghan. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67719 ) Change subject: flashchips: Add write protect bits to W25Q64JW...M ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: Please take a look -- To view, visit https://review.coreboot.org/c/flashrom/+/67719 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Idf2289b7c90724ececc122d2a05c7cae3af2cf62 Gerrit-Change-Number: 67719 Gerrit-PatchSet: 2 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Tue, 04 Oct 2022 02:31:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: writeprotect_ranges.c: add more range functions
by Anastasia Klimchuk (Code Review) 04 Oct '22

04 Oct '22

Attention is currently required from: Nico Huber, Maciej Pijanowski, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66212 ) Change subject: writeprotect_ranges.c: add more range functions ...................................................................... Patch Set 6: (2 comments) Patchset: PS3: > There's a .tested member in the struct for flash chips. I guess it could be extended to also indicate whether WP support has been tested. Yes that's what https://ticket.coreboot.org/issues/377 is about, not sure if you clicked ;) Good that we agree. Maciej, Sergii, I have a straight question, would you implement support for WP in `flashchip.tested` ? That would be ideal, and so much appreciated! File writeprotect_ranges.c: https://review.coreboot.org/c/flashrom/+/66212/comment/ab10caa5_c65ae034 PS2, Line 20: _generic > Sure, but I used the suffix for consistency with other similar functions: […] Okay makes sense. -- To view, visit https://review.coreboot.org/c/flashrom/+/66212 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ied7b27be2ee2426af8f473432e2b01a290de2365 Gerrit-Change-Number: 66212 Gerrit-PatchSet: 6 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> 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: Tue, 04 Oct 2022 00:23:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: drivers: Move (un)map_flash_region to par/spi/opaque_master
by Jonathon Hall (Code Review) 03 Oct '22

03 Oct '22

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Jonathon Hall, Nikolai Artemiev. Jonathon Hall has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67695 ) Change subject: drivers: Move (un)map_flash_region to par/spi/opaque_master ...................................................................... Patch Set 8: (1 comment) File ichspi.c: https://review.coreboot.org/c/flashrom/+/67695/comment/343fcc62_9bbbe6e8 PS2, Line 1865: &mapper_phys > Yep thank you all, I totally agree! I'm still getting the hang of the Gerrit workflow and what works […] Alrighty this is a lot more understandable now! I split this up into several patches and rebased onto CB:67404. The second patch (ICH hwseq) is the one I really need, the later ones do some additional cleanup. -- To view, visit https://review.coreboot.org/c/flashrom/+/67695 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9c3df6ae260bcdb246dfb0cd8e043919609b014b Gerrit-Change-Number: 67695 Gerrit-PatchSet: 8 Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Mon, 03 Oct 2022 21:17:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.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: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: serprog.c: Replace custom mapper with max_rom_decode
by Jonathon Hall (Code Review) 03 Oct '22

03 Oct '22

Attention is currently required from: Felix Singer, Thomas Heijligen. Jonathon Hall has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68093 ) Change subject: serprog.c: Replace custom mapper with max_rom_decode ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: I don't have the hardware to test this on, but I was asked to give this a try while refactoring CB:67695. It turned out we need the SPI mappers anyway (for now), so this wasn't critical to that patch after all. I think it is still a decent improvement though. -- To view, visit https://review.coreboot.org/c/flashrom/+/68093 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If8b313708a52813b9354cbf45cb91cdd1fe36b13 Gerrit-Change-Number: 68093 Gerrit-PatchSet: 1 Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Mon, 03 Oct 2022 21:17:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: drivers: Move (un)map_flash_region to par/spi/opaque_master
by Angel Pons (Code Review) 03 Oct '22

03 Oct '22

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Jonathon Hall, Nikolai Artemiev. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67695 ) Change subject: drivers: Move (un)map_flash_region to par/spi/opaque_master ...................................................................... Patch Set 8: (1 comment) File ichspi.c: https://review.coreboot.org/c/flashrom/+/67695/comment/36e2ecb4_a70012ef PS2, Line 1865: &mapper_phys > Yep thank you all, I totally agree! I'm still getting the hang of the Gerrit workflow and what works best for review but I really appreciate all the help! It is our duty to introduce newcomers to the Gerrit workflows and mechanics, as we all were newcomers at some point. And given that we're going to run into each other rather frequently, we might as well work towards making the review process as pleasant as possible for everyone involved. > I will rebase this on those patches and split it up. I think this will require temporarily adding a mapper to the opaque bus master that I then remove in a later patch, but that should be straightforward. Sounds good. It can't be worse than that one temporary macro with 24 parameters that we devised to enable refactoring of RAM init code in a reviewable way. RAM init code is full of magic numbers, and extremely hard to test. By leveraging reproducible builds to adapt the code, most of the non-reproducible refactoring got reduced to CB:40980 which replaces the implementation without changing the magic numbers. 😄 -- To view, visit https://review.coreboot.org/c/flashrom/+/67695 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9c3df6ae260bcdb246dfb0cd8e043919609b014b Gerrit-Change-Number: 67695 Gerrit-PatchSet: 8 Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Mon, 03 Oct 2022 21:11:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.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: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: ichspi: Do not mmap for hwseq
by Jonathon Hall (Code Review) 03 Oct '22

03 Oct '22

Jonathon Hall has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/67696 ) Change subject: ichspi: Do not mmap for hwseq ...................................................................... Abandoned Superseded by CB:68090 -- To view, visit https://review.coreboot.org/c/flashrom/+/67696 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic7761327b4072dcb9917000db9eaacc9404e6823 Gerrit-Change-Number: 67696 Gerrit-PatchSet: 2 Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: abandon

1 0

[S] Change in flashrom[master]: serprog.c: Replace custom mapper with max_rom_decode
by Jonathon Hall (Code Review) 03 Oct '22

03 Oct '22

Jonathon Hall has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/68093 ) Change subject: serprog.c: Replace custom mapper with max_rom_decode ...................................................................... serprog.c: Replace custom mapper with max_rom_decode serprog's custom mapper is actually a hack to check for a maximum chip size. It would not have actually failed a read/write/erase attempt since mappers are allowed to return NULL, it only printed a warning. We can't remove custom mappers from SPI masters yet since some still use it, but many of those are not actually needed or could be reworked, so this is one step closer to removing SPI custom mappers. Change-Id: If8b313708a52813b9354cbf45cb91cdd1fe36b13 Signed-off-by: Jonathon Hall <jonathon.hall(a)puri.sm> --- M serprog.c 1 file changed, 28 insertions(+), 17 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/93/68093/1 diff --git a/serprog.c b/serprog.c index a3a3db3..bdcf697 100644 --- a/serprog.c +++ b/serprog.c @@ -438,23 +438,7 @@ return 0; } -static void *serprog_map(const char *descr, uintptr_t phys_addr, size_t len) -{ - /* Serprog transmits 24 bits only and assumes the underlying implementation handles any remaining bits - * correctly (usually setting them to one either in software (for FWH/LPC) or relying on the fact that - * the hardware observes a subset of the address bits only). Combined with the standard mapping of - * flashrom this creates a 16 MB-wide window just below the 4 GB boundary where serprog can operate (as - * needed for non-SPI chips). Below we make sure that the requested range is within this window. */ - if ((phys_addr & 0xFF000000) == 0xFF000000) { - return (void*)phys_addr; - } - msg_pwarn(MSGHEADER "requested mapping %s is incompatible: 0x%zx bytes at 0x%0*" PRIxPTR ".\n", - descr, len, PRIxPTR_WIDTH, phys_addr); - return NULL; -} - static struct spi_master spi_master_serprog = { - .map_flash_region = serprog_map, .features = SPI_MASTER_4BA, .max_data_read = MAX_DATA_READ_UNLIMITED, .max_data_write = MAX_DATA_WRITE_UNLIMITED, @@ -569,7 +553,6 @@ } static const struct par_master par_master_serprog = { - .map_flash_region = serprog_map, .chip_readb = serprog_chip_readb, .chip_readw = fallback_chip_readw, .chip_readl = fallback_chip_readl, @@ -928,6 +911,16 @@ sp_streamed_transmit_bytes = 0; sp_opbuf_usage = 0; + /* Serprog transmits 24 bits only and assumes the underlying + * implementation handles any remaining bits correctly (usually setting + * them to one either in software (for FWH/LPC) or relying on the fact + * that the hardware observes a subset of the address bits only). Only + * 16MB or smaller chips can be used with serprog. */ + max_rom_decode.parallel = 16 * MiB; + max_rom_decode.lpc = 16 * MiB; + max_rom_decode.fwh = 16 * MiB; + max_rom_decode.spi = 16 * MiB; + if (register_shutdown(serprog_shutdown, NULL)) goto init_err_cleanup_exit; if (serprog_buses_supported & BUS_SPI) -- To view, visit https://review.coreboot.org/c/flashrom/+/68093 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If8b313708a52813b9354cbf45cb91cdd1fe36b13 Gerrit-Change-Number: 68093 Gerrit-PatchSet: 1 Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[master]: flashrom.c: Remove custom mappers from opaque_master
by Jonathon Hall (Code Review) 03 Oct '22

03 Oct '22

Jonathon Hall has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/68092 ) Change subject: flashrom.c: Remove custom mappers from opaque_master ...................................................................... flashrom.c: Remove custom mappers from opaque_master No opaque masters have a custom mapper. The returned chipaddr is not fed back into the read/write/erase functions, so this would only be useful for side effects. Change-Id: I36f05154edda371b51f8ff416f019837ff1c243d Signed-off-by: Jonathon Hall <jonathon.hall(a)puri.sm> --- M flashrom.c M include/programmer.h 2 files changed, 17 insertions(+), 11 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/92/68092/1 diff --git a/flashrom.c b/flashrom.c index 7f9fdea..0d114fc 100644 --- a/flashrom.c +++ b/flashrom.c @@ -211,9 +211,7 @@ * fallback if it does not specialize it */ void *(*map_flash_region) (const char *descr, uintptr_t phys_addr, size_t len) = NULL; - if (mst->buses_supported & BUS_PROG) - map_flash_region = mst->opaque.map_flash_region; - else if (mst->buses_supported & BUS_SPI) + if (mst->buses_supported & BUS_SPI) map_flash_region = mst->spi.map_flash_region; else if (mst->buses_supported & BUS_NONSPI) map_flash_region = mst->par.map_flash_region; @@ -232,9 +230,7 @@ void *virt_addr, size_t len) { void (*unmap_flash_region) (void *virt_addr, size_t len) = NULL; - if (mst->buses_supported & BUS_PROG) - unmap_flash_region = mst->opaque.unmap_flash_region; - else if (mst->buses_supported & BUS_SPI) + if (mst->buses_supported & BUS_SPI) unmap_flash_region = mst->spi.unmap_flash_region; else if (mst->buses_supported & BUS_NONSPI) unmap_flash_region = mst->par.unmap_flash_region; @@ -249,9 +245,7 @@ #if CONFIG_INTERNAL == 1 static bool master_uses_physmap(const struct registered_master *mst) { - if (mst->buses_supported & BUS_PROG) - return mst->opaque.map_flash_region == physmap; - else if (mst->buses_supported & BUS_SPI) + if (mst->buses_supported & BUS_SPI) return mst->spi.map_flash_region == physmap; else if (mst->buses_supported & BUS_NONSPI) return mst->par.map_flash_region == physmap; diff --git a/include/programmer.h b/include/programmer.h index 4b5bf92..bfce5d6 100644 --- a/include/programmer.h +++ b/include/programmer.h @@ -398,8 +398,6 @@ /* opaque.c */ struct opaque_master { - void *(*map_flash_region) (const char *descr, uintptr_t phys_addr, size_t len); - void (*unmap_flash_region) (void *virt_addr, size_t len); int max_data_read; int max_data_write; /* Specific functions for this master */ -- To view, visit https://review.coreboot.org/c/flashrom/+/68092 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I36f05154edda371b51f8ff416f019837ff1c243d Gerrit-Change-Number: 68092 Gerrit-PatchSet: 1 Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[master]: dummyflasher.c: Remove custom mapper from opaque_master
by Jonathon Hall (Code Review) 03 Oct '22

03 Oct '22

Jonathon Hall has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/68091 ) Change subject: dummyflasher.c: Remove custom mapper from opaque_master ...................................................................... dummyflasher.c: Remove custom mapper from opaque_master Dummy doesn't need a custom mapper on opaque_master; the returned address is not used and the mapper has no side effects. This is the only remaining opaque master with a custom mapper, so this permits removing custom mapper support from opaque masters. Change-Id: I76ae3e0c2e91ecba4fd320941bd1eff038050731 Signed-off-by: Jonathon Hall <jonathon.hall(a)puri.sm> --- M dummyflasher.c 1 file changed, 16 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/91/68091/1 diff --git a/dummyflasher.c b/dummyflasher.c index ad734f0..22f392a 100644 --- a/dummyflasher.c +++ b/dummyflasher.c @@ -947,8 +947,6 @@ }; static const struct opaque_master opaque_master_dummyflasher = { - .map_flash_region = dummy_map, - .unmap_flash_region = dummy_unmap, .probe = probe_variable_size, .read = dummy_opaque_read, .write = dummy_opaque_write, -- To view, visit https://review.coreboot.org/c/flashrom/+/68091 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I76ae3e0c2e91ecba4fd320941bd1eff038050731 Gerrit-Change-Number: 68091 Gerrit-PatchSet: 1 Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[master]: ichspi: Do not attempt to map physical memory for hwseq
by Jonathon Hall (Code Review) 03 Oct '22

03 Oct '22

Jonathon Hall has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/68090 ) Change subject: ichspi: Do not attempt to map physical memory for hwseq ...................................................................... ichspi: Do not attempt to map physical memory for hwseq ICH hwseq does not need to actually mmap flash, and this fails for flash chips >16 MB, since only the top 16 MB is mapped into the address space. Test: Read and write flash on ICH hwseq with 32 MB flash chip. Change-Id: Ie698071c3181e988f10b750b0e50c9700efaa1a3 Signed-off-by: Jonathon Hall <jonathon.hall(a)puri.sm> --- M ichspi.c 1 file changed, 16 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/90/68090/1 diff --git a/ichspi.c b/ichspi.c index 7e7967e..aefbe2f 100644 --- a/ichspi.c +++ b/ichspi.c @@ -1794,8 +1794,6 @@ }; static const struct opaque_master opaque_master_ich_hwseq = { - .map_flash_region = physmap, - .unmap_flash_region = physunmap, .max_data_read = 64, .max_data_write = 64, .probe = ich_hwseq_probe, -- To view, visit https://review.coreboot.org/c/flashrom/+/68090 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie698071c3181e988f10b750b0e50c9700efaa1a3 Gerrit-Change-Number: 68090 Gerrit-PatchSet: 1 Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-MessageType: newchange

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit October 2022