- flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: libflashrom,writeprotect: add flashrom_wp_{read,write}_chip_config()
by Nikolai Artemiev (Code Review) 05 Jan '22

05 Jan '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Sergii Dmytruk. Nikolai Artemiev 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 23: (1 comment) Patchset: PS22: > Disregard typo above, start and len shouldn't be pointers in flashrom_wp_protect_flash(). […] Passing an abstract configuration structure to WP functions is probably a better option than the interface in my previous comment. Something like: ``` struct flashrom_wp_cfg { enum flashrom_wp_mode { WP_MODE_DISABLED, WP_MODE_HARDWARE } prot_mode; struct flashrom_wp_range { chipsize_t start, len; } prot_range; }; int flashrom_wp_get_cfg(struct flashrom_wp_cfg *, struct flashrom_flashctx *); int flashrom_wp_set_cfg(struct flashrom_flashctx *, struct flashrom_wp_cfg); ``` Then libflashrom users just populate the structure with the settings they want and the WP code will write them to the chip if it's possible to do so. Direct SPI WP will be able to write any configuration, so won't lose any functionality that we have right now with the individual {get,set}_wp_{mode,range} functions. And linux_mtd WP can just return an error if it's not possible to write the config (i.e. it doesn't map to a MTD ioctl). -- 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: 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: 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-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Tue, 04 Jan 2022 22:44:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add Elkhart Lake support
by Werner Zeh (Code Review) 03 Jan '22

03 Jan '22

Werner Zeh has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/60711 ) Change subject: Add Elkhart Lake support ...................................................................... Add Elkhart Lake support Elkhart Lake has a chipset called Mule Creek Canyon which is quite compatible with 300 series chipsets. There are a few differences though, e.g. different encoding for the SPI clock values for read and write in the FLCOMP register. In addition Elkhart Lake has a new PCI device ID for the SPI controller which is added, too. TEST=Read and flash complete flash on Siemens MC EHL1 Change-Id: I711e39a3ec9cd7098389231eaa1cb864d615a475 Signed-off-by: Werner Zeh <werner.zeh(a)siemens.com> --- M chipset_enable.c M ich_descriptors.c M ichspi.c M programmer.h M util/ich_descriptors_tool/ich_descriptors_tool.c 5 files changed, 42 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/11/60711/1 diff --git a/chipset_enable.c b/chipset_enable.c index 2e4bb1e..8528cfc 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -601,6 +601,7 @@ case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_400_SERIES_COMET_POINT: case CHIPSET_500_SERIES_TIGER_POINT: + case CHIPSET_ELKHART_LAKE: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: reg_name = "BIOS_SPI_BC"; @@ -704,6 +705,7 @@ boot_straps = boot_straps_pch8_lp; break; case CHIPSET_500_SERIES_TIGER_POINT: + case CHIPSET_ELKHART_LAKE: boot_straps = boot_straps_pch500; break; case CHIPSET_APOLLO_LAKE: @@ -737,6 +739,7 @@ case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: + case CHIPSET_ELKHART_LAKE: bbs = (gcs >> 6) & 0x1; break; default: @@ -987,6 +990,11 @@ return enable_flash_pch100_or_c620(dev, name, 0x1f, 5, CHIPSET_500_SERIES_TIGER_POINT); } +static int enable_flash_mcc(struct pci_dev *const dev, const char *const name) +{ + return enable_flash_pch100_or_c620(dev, name, 0x1f, 5, CHIPSET_ELKHART_LAKE); +} + static int enable_flash_apl(struct pci_dev *const dev, const char *const name) { return enable_flash_pch100_or_c620(dev, name, 0x0d, 2, CHIPSET_APOLLO_LAKE); @@ -2101,6 +2109,7 @@ {0x8086, 0x5af0, B_S, DEP, "Intel", "Apollo Lake", enable_flash_apl}, {0x8086, 0x3197, B_S, NT, "Intel", "Gemini Lake", enable_flash_glk}, {0x8086, 0x31e8, B_S, DEP, "Intel", "Gemini Lake", enable_flash_glk}, + {0x8086, 0x4b24, B_S, DEP, "Intel", "Elkhart Lake", enable_flash_mcc}, {0x8086, 0xa303, B_S, NT, "Intel", "H310", enable_flash_pch300}, {0x8086, 0xa304, B_S, NT, "Intel", "H370", enable_flash_pch300}, {0x8086, 0xa305, B_S, DEP, "Intel", "Z390", enable_flash_pch300}, diff --git a/ich_descriptors.c b/ich_descriptors.c index 56a3c67..3d253a1 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -46,6 +46,7 @@ case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_400_SERIES_COMET_POINT: case CHIPSET_500_SERIES_TIGER_POINT: + case CHIPSET_ELKHART_LAKE: return 16; case CHIPSET_100_SERIES_SUNRISE_POINT: return 10; @@ -72,6 +73,7 @@ case CHIPSET_C620_SERIES_LEWISBURG: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: + case CHIPSET_ELKHART_LAKE: if (cont->NM <= MAX_NUM_MASTERS) return cont->NM; break; @@ -109,7 +111,7 @@ "8 series Lynx Point", "Baytrail", "8 series Lynx Point LP", "8 series Wellsburg", "9 series Wildcat Point", "9 series Wildcat Point LP", "100 series Sunrise Point", "C620 series Lewisburg", "300 series Cannon Point", "400 series Comet Point", - "500 series Tiger Point", "Apollo Lake", "Gemini Lake", + "500 series Tiger Point", "Apollo Lake", "Gemini Lake", "Elkhart Lake", }; if (cs < CHIPSET_ICH8 || cs - CHIPSET_ICH8 + 1 >= ARRAY_SIZE(chipset_names)) cs = 0; @@ -205,7 +207,8 @@ case CHIPSET_400_SERIES_COMET_POINT: case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_APOLLO_LAKE: - case CHIPSET_GEMINI_LAKE: { + case CHIPSET_GEMINI_LAKE: + case CHIPSET_ELKHART_LAKE: { uint8_t size_enc; if (idx == 0) { size_enc = desc->component.dens_new.comp1_density; @@ -224,7 +227,7 @@ static const char *pprint_freq(enum ich_chipset cs, uint8_t value) { - static const char *const freq_str[4][8] = { { + static const char *const freq_str[5][8] = { { "20 MHz", "33 MHz", "reserved", @@ -260,6 +263,15 @@ "reserved", "14 MHz", "reserved" + }, { + "reserved", + "50 MHz", + "reserved", + "reserved", + "33 MHz", + "20 MHz", + "reserved", + "reserved", }}; switch (cs) { @@ -289,6 +301,8 @@ return freq_str[2][value]; case CHIPSET_500_SERIES_TIGER_POINT: return freq_str[3][value]; + case CHIPSET_ELKHART_LAKE: + return freq_str[4][value]; case CHIPSET_ICH_UNKNOWN: default: return "unknown"; @@ -334,6 +348,7 @@ case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: + case CHIPSET_ELKHART_LAKE: has_flill1 = true; break; default: @@ -513,7 +528,7 @@ desc->master.mstr[i].write & (1 << j) ? 'w' : ' '); msg_pdbg2("\n"); } - } else if (cs == CHIPSET_APOLLO_LAKE || cs == CHIPSET_GEMINI_LAKE) { + } else if (cs == CHIPSET_APOLLO_LAKE || cs == CHIPSET_GEMINI_LAKE || cs == CHIPSET_ELKHART_LAKE) { const char *const master_names[] = { "BIOS", "TXE", }; if (nm > (ssize_t)ARRAY_SIZE(master_names)) { msg_pdbg2("%s: number of masters too high (%d).\n", __func__, desc->content.NM); @@ -1194,6 +1209,7 @@ case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: + case CHIPSET_ELKHART_LAKE: if (idx == 0) { size_enc = desc->component.dens_new.comp1_density; } else { @@ -1232,6 +1248,7 @@ case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: + case CHIPSET_ELKHART_LAKE: mmio_le_writel(control, spibar + PCH100_REG_FDOC); return mmio_le_readl(spibar + PCH100_REG_FDOD); default: diff --git a/ichspi.c b/ichspi.c index 290deaa..76d853b 100644 --- a/ichspi.c +++ b/ichspi.c @@ -427,6 +427,7 @@ case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_400_SERIES_COMET_POINT: case CHIPSET_500_SERIES_TIGER_POINT: + case CHIPSET_ELKHART_LAKE: break; default: pprint_reg(HSFS, BERASE, reg_val, ", "); @@ -439,6 +440,7 @@ case CHIPSET_300_SERIES_CANNON_POINT: case CHIPSET_400_SERIES_COMET_POINT: case CHIPSET_500_SERIES_TIGER_POINT: + case CHIPSET_ELKHART_LAKE: pprint_reg(HSFS, PRR34_LOCKDN, reg_val, ", "); pprint_reg(HSFS, WRSDIS, reg_val, ", "); break; @@ -1741,6 +1743,7 @@ case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: + case CHIPSET_ELKHART_LAKE: num_pr = 6; /* Includes GPR0 */ reg_pr0 = PCH100_REG_FPR0; swseq_data.reg_ssfsc = PCH100_REG_SSFSC; @@ -1768,6 +1771,7 @@ num_freg = 10; break; case CHIPSET_C620_SERIES_LEWISBURG: + case CHIPSET_ELKHART_LAKE: num_freg = 12; /* 12 MMIO regs, but 16 regions in FD spec */ break; case CHIPSET_300_SERIES_CANNON_POINT: @@ -1873,6 +1877,7 @@ case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: + case CHIPSET_ELKHART_LAKE: tmp = mmio_readl(spibar + PCH100_REG_DLOCK); msg_pdbg("0x0c: 0x%08x (DLOCK)\n", tmp); prettyprint_pch100_reg_dlock(tmp); @@ -1897,7 +1902,6 @@ "most likely\ndue to an active ME. Please see " "https://flashrom.org/ME for details.\n"); } - /* Handle PR registers */ for (i = 0; i < num_pr; i++) { /* if not locked down try to disable PR locks first */ @@ -1951,6 +1955,7 @@ case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: case CHIPSET_BAYTRAIL: + case CHIPSET_ELKHART_LAKE: break; default: ichspi_bbar = mmio_readl(spibar + ICH9_REG_BBAR); @@ -1985,6 +1990,7 @@ case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: + case CHIPSET_ELKHART_LAKE: break; default: tmp = mmio_readl(spibar + ICH9_REG_FPB); @@ -2017,7 +2023,8 @@ (ich_gen == CHIPSET_100_SERIES_SUNRISE_POINT || ich_gen == CHIPSET_300_SERIES_CANNON_POINT || ich_gen == CHIPSET_400_SERIES_COMET_POINT || - ich_gen == CHIPSET_500_SERIES_TIGER_POINT)) { + ich_gen == CHIPSET_500_SERIES_TIGER_POINT || + ich_gen == CHIPSET_ELKHART_LAKE)) { msg_pdbg("Enabling hardware sequencing by default for 100+ series PCH.\n"); ich_spi_mode = ich_hwseq; } diff --git a/programmer.h b/programmer.h index b4f04e7..530706a 100644 --- a/programmer.h +++ b/programmer.h @@ -362,6 +362,7 @@ CHIPSET_500_SERIES_TIGER_POINT, CHIPSET_APOLLO_LAKE, CHIPSET_GEMINI_LAKE, + CHIPSET_ELKHART_LAKE, }; /* ichspi.c */ diff --git a/util/ich_descriptors_tool/ich_descriptors_tool.c b/util/ich_descriptors_tool/ich_descriptors_tool.c index a1e353b..f743510 100644 --- a/util/ich_descriptors_tool/ich_descriptors_tool.c +++ b/util/ich_descriptors_tool/ich_descriptors_tool.c @@ -238,6 +238,8 @@ cs = CHIPSET_APOLLO_LAKE; else if (strcmp(csn, "gemini") == 0) cs = CHIPSET_GEMINI_LAKE; + else if (strcmp(csn, "elkhart") == 0) + cs = CHIPSET_ELKHART_LAKE; } ret = read_ich_descriptors_from_dump(buf, len, &cs, &desc); -- To view, visit https://review.coreboot.org/c/flashrom/+/60711 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I711e39a3ec9cd7098389231eaa1cb864d615a475 Gerrit-Change-Number: 60711 Gerrit-PatchSet: 1 Gerrit-Owner: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-MessageType: newchange

1 0

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

29 Dec '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Sergii Dmytruk. Nikolai Artemiev 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 23: (1 comment) Patchset: PS22: > This will be a fairly long comment and cover some things mentioned in other comments, but it's proba […] Disregard typo above, start and len shouldn't be pointers in flashrom_wp_protect_flash(). Also that's just a rough idea for the function name. -- 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: 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: 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-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Wed, 29 Dec 2021 11:36:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: comment

1 0

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

29 Dec '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Sergii Dmytruk. Nikolai Artemiev 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 23: (16 comments) Patchset: PS22: > Thinking further about this, there are more reasons to tie […] This will be a fairly long comment and cover some things mentioned in other comments, but it's probably easiest to put everything here: I've thought more about what sort of API should be exposed through libflashrom and I don't think exposing the wp_chip_config structure is a good idea, even if the structure is opaque to libflashrom users. In addition to the config being to tied to the specific chip as you mention, properly sequencing read/modify/write/verify operations is unnecessary work and it increases the risk that introducing a new chip will break existing assumptions in the future. Another reason to simplify the API is that linux's MTD interface exposes a much simpler protection API. It's really inconvenient to have two APIs for SPI/MTD protection and unifying them makes sense if we don't lost much IMO. Specifically MTD gives us these operations: - MEMLOCK: Enable HW protection and set range - MEMUNLOCK: Disable protection / clear range - MEMISLOCKED: Get current protection range / HW protection status. The final libdflashrom API / functionality would be something like: - flashrom_wp_protect_flash(flashctx *, flashsize_t *start, flashsize_t *len) // Read config, set HW protection, set range, write, verify - flashrom_wp_unprotect_flash(flashctx *) // Read config, unset HW protection, set empty range, write, verify - flashrom_wp_get_protection_status(flashctx *, flashsize_t *start, flashsize_t *len, bool *hw_prot_enabled) // Read config, decode range / mode, write values to output vars We can of course expand with additional functions if they're useful, but for now I think these will cover most standard use cases and provide a more flexible API. The only major thing the new API doesn't allow is setting a protection range without enabling HW protection, but that would just get undone by flashrom or whatever other tool someone next uses to write the flash. As for migrating the current code, I plan to keep the current writeprotect functions, but keep them inside writeprotect.c and only expose the new functions. Some things that need to be shared will probably get moved to writeprotect.h, like the chip config structure. What do you think? File libflashrom.h: https://review.coreboot.org/c/flashrom/+/58479/comment/2dc379c2_f68bd077 PS22, Line 131: const > We should be careful with `const` here. Flash access goes very deep down […] Done File writeprotect.c: https://review.coreboot.org/c/flashrom/+/58479/comment/e7b9526e_6fae517b PS22, Line 35: 0 > Will this ever be consumed? Good point. I thought it would be helpful to ensure it is zeroed if the bit isn't present, but the structure is zero-initialized anyway so we don't need to do that here. https://review.coreboot.org/c/flashrom/+/58479/comment/cbf7d480_3ccb70ae PS22, Line 59: * > No dangling asterisk, please. I've made the comment a bit bigger, I think the asterisks are ok now since there are leading and trailing comment lines. https://review.coreboot.org/c/flashrom/+/58479/comment/d9aa7786_76ca184c PS22, Line 59: intrepret > int*er*pret Done https://review.coreboot.org/c/flashrom/+/58479/comment/b7e234e5_66393683 PS22, Line 61: tmp > rather `unused` or `ignored`? Done https://review.coreboot.org/c/flashrom/+/58479/comment/eefce310_37c9f4b7 PS22, Line 63: *cfg = (struct wp_chip_config) {0}; > Won't this be overwritten where it matters? iow. […] I think I might have relied on this in an earlier patch set but I don't remember now. I think it's kind of nice to zero it out before we do anything, else but I can drop it if you want. https://review.coreboot.org/c/flashrom/+/58479/comment/02149fac_a29c6663 PS22, Line 70: r( > Missing space. Done https://review.coreboot.org/c/flashrom/+/58479/comment/926f9597_d5f22b5d PS22, Line 70: size_t > Declarations inside a `for` made trouble in some environment. Please avoid it for now. Done https://review.coreboot.org/c/flashrom/+/58479/comment/694d2b85_2eb0d8a4 PS22, Line 70: for(size_t i = 0; bits->bp[i].reg != INVALID_REG; i++) { > Generally, it's preferred to loop over the target array. This way you […] Done https://review.coreboot.org/c/flashrom/+/58479/comment/37c2fc13_d2beeb61 PS22, Line 75: r( > Missing space. Done https://review.coreboot.org/c/flashrom/+/58479/comment/bb2ddb25_6cc35253 PS22, Line 84: * > No dangling asterisk, please. Or maybe just use the long comment style […] Done https://review.coreboot.org/c/flashrom/+/58479/comment/da2b5e4b_56f87daa PS22, Line 122: INVALID_REG > This is very confusing. I see it's skipped because the mask would be 0, […] I've changed it to (INVALID_REG + 1), perhaps STATUS1 would be better though. I'll change it if you want. https://review.coreboot.org/c/flashrom/+/58479/comment/1b44b521_87138102 PS22, Line 126: = 0 > Why initialize? Done https://review.coreboot.org/c/flashrom/+/58479/comment/00045ff9_57888c6e PS22, Line 144: configuraitons > configura*ti*ons Done https://review.coreboot.org/c/flashrom/+/58479/comment/37c351de_7c726a76 PS22, Line 145: which of two configurations should be used if they both select the same : * protection range > Does this really work? How can we say, for instance, that SRP1 being […] This was kind of an adaptation of an earlier function that just compared ranges so they could be sorted and a preferred range encoding could be chosen. I realized that it could also be used to compare most bits in the config structure, so I extended it to allow all bits to be compared, which is useful when we want to verify a config we have read back from a chip. In this context "preferred" is arbitrary, it should only be used to pick between two configurations if both of them are acceptable. -- 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: 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: 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-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Wed, 29 Dec 2021 11:31:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.c: Move do_*() helpers into cli_classic.c
by Edward O'Callaghan (Code Review) 29 Dec '21

29 Dec '21

Attention is currently required from: Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60070 ) Change subject: flashrom.c: Move do_*() helpers into cli_classic.c ...................................................................... Patch Set 4: -Code-Review -- To view, visit https://review.coreboot.org/c/flashrom/+/60070 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If1112155e2421e0178fd73f847cbb80868387433 Gerrit-Change-Number: 60070 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 29 Dec 2021 00:40:22 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: build: Remove cli_classic need for internal symbols [WIP]
by Edward O'Callaghan (Code Review) 29 Dec '21

29 Dec '21

Attention is currently required from: Edward O'Callaghan. Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/60074 to look at the new patch set (#3). Change subject: build: Remove cli_classic need for internal symbols [WIP] ...................................................................... build: Remove cli_classic need for internal symbols [WIP] cli_classic should now only use the libflashrom interface. BUG=b:208132085 TEST=`make && meson/ninja`. Change-Id: Ib04293c14f53c8e798c7d3e35483067520668161 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M Makefile M meson.build 2 files changed, 2 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/60074/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/60074 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib04293c14f53c8e798c7d3e35483067520668161 Gerrit-Change-Number: 60074 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: flashrom: Drop read_flash_to_file() usage
by Edward O'Callaghan (Code Review) 29 Dec '21

29 Dec '21

Attention is currently required from: Nico Huber, Angel Pons, Daniel Campello, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59291 ) Change subject: flashrom: Drop read_flash_to_file() usage ...................................................................... Patch Set 7: (1 comment) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/59291/comment/170c4ccf_02c36805 PS4, Line 635: if (map_flash(&flashes[0]) != 0) { : free(flashes[0].chip); : ret = 1; : goto out_shutdown; : } : msg_cinfo("Please note that forced reads most likely contain garbage.\n"); : ret = do_read(&flashes[0], filename); : unmap_flash(&flashes[0]); > > That is a good point, I removed them. […] 100% makes sense, I put the easy part out into https://review.coreboot.org/c/flashrom/+/60430 and kept this hunk in this change-id so the discussion thread doesn't get mixed up. Good call on the PWM, will try to jig something up at home outside the lab to see if I can make that happen. -- To view, visit https://review.coreboot.org/c/flashrom/+/59291 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4b690b688acf9d5deb46e8642a252a2132ea8c73 Gerrit-Change-Number: 59291 Gerrit-PatchSet: 7 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: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> 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: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 29 Dec 2021 00:04:21 +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> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom: Convert do_read() into a libflashrom user
by Edward O'Callaghan (Code Review) 29 Dec '21

29 Dec '21

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/60430 ) Change subject: flashrom: Convert do_read() into a libflashrom user ...................................................................... flashrom: Convert do_read() into a libflashrom user Aspire towards a goal of making cli_classic more of just a user of libflashrom than having quasi-parallel paths in flashrom.c This converts the do_read() provider wrapper into a pure libflashrom user. BUG=b:208132085 TEST=`$ sudo ./flashrom -p internal -r /tmp/bios.bin` Change-Id: Id2addadb891c482ee3f69da806062d7a88776675 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M flashrom.c 1 file changed, 17 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/30/60430/1 diff --git a/flashrom.c b/flashrom.c index 48d953b..445db30 100644 --- a/flashrom.c +++ b/flashrom.c @@ -2193,13 +2193,27 @@ int do_read(struct flashctx *const flash, const char *const filename) { - if (prepare_flash_access(flash, true, false, false, false)) + int ret; + + unsigned long size = flash->chip->total_size * 1024; + unsigned char *buf = calloc(size, sizeof(unsigned char)); + if (!buf) { + msg_gerr("Memory allocation failed!\n"); return 1; + } - const int ret = read_flash_to_file(flash, filename); + ret = flashrom_image_read(flash, buf, size); + if (ret > 0) + goto free_out; - finalize_flash_access(flash); + if (write_buf_to_include_args(flash, buf)) { + ret = 1; + goto free_out; + } + ret = write_buf_to_file(buf, size, filename); +free_out: + free(buf); return ret; } -- To view, visit https://review.coreboot.org/c/flashrom/+/60430 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id2addadb891c482ee3f69da806062d7a88776675 Gerrit-Change-Number: 60430 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: flashrom: Drop read_flash_to_file() usage
by Edward O'Callaghan (Code Review) 29 Dec '21

29 Dec '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk. Hello build bot (Jenkins), Daniel Campello, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/59291 to look at the new patch set (#7). Change subject: flashrom: Drop read_flash_to_file() usage ...................................................................... flashrom: Drop read_flash_to_file() usage Aspire towards a goal of making cli_classic more of just a user of libflashrom than having quasi-parallel paths in flashrom.c This converts remaining read_flash_to_file() usage to the do_read() provider wrapper around libflashrom. BUG=b:208132085 TEST=<TODO> Change-Id: I4b690b688acf9d5deb46e8642a252a2132ea8c73 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M cli_classic.c M flash.h M flashrom.c 3 files changed, 2 insertions(+), 42 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/91/59291/7 -- To view, visit https://review.coreboot.org/c/flashrom/+/59291 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4b690b688acf9d5deb46e8642a252a2132ea8c73 Gerrit-Change-Number: 59291 Gerrit-PatchSet: 7 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: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> 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: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: flashrom.c: Move do_*() helpers into cli_classic.c
by Edward O'Callaghan (Code Review) 29 Dec '21

29 Dec '21

Attention is currently required from: Anastasia Klimchuk. Hello build bot (Jenkins), Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/60070 to look at the new patch set (#3). Change subject: flashrom.c: Move do_*() helpers into cli_classic.c ...................................................................... flashrom.c: Move do_*() helpers into cli_classic.c These helpers are only used by the CLI logic and so we localise them here to move towards cli_classic being a pure libflashrom user. BUG=b:208132085 TEST=`make` Change-Id: If1112155e2421e0178fd73f847cbb80868387433 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M cli_classic.c M flash.h M flashrom.c 3 files changed, 96 insertions(+), 100 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/70/60070/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/60070 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If1112155e2421e0178fd73f847cbb80868387433 Gerrit-Change-Number: 60070 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit