December 2022 - flashrom-gerrit

[S] Change in flashrom[master]: chipset_enable.c: add PCI ID for TGL-UP3 SPI controller
by Angel Pons (Code Review) 16 Dec '22

16 Dec '22

Attention is currently required from: Jan Samek. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70388 ) Change subject: chipset_enable.c: add PCI ID for TGL-UP3 SPI controller ...................................................................... Patch Set 2: Code-Review+2 (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/70388/comment/21fbbd38_0d184113 PS2, Line 7: SPI controller Drop, since it's not for the SPI controller? File chipset_enable.c: https://review.coreboot.org/c/flashrom/+/70388/comment/6f8326bc_55c08313 PS1, Line 2083: 0xa0a4 > All operations (-r, -E, -w, -v) on TGL-UP3 proved working with 8086:0a88. Thanks! -- To view, visit https://review.coreboot.org/c/flashrom/+/70388 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie93af14eb5857bfe51964f6565e475b6249dd407 Gerrit-Change-Number: 70388 Gerrit-PatchSet: 2 Gerrit-Owner: Jan Samek <jan.samek(a)siemens.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jan Samek <jan.samek(a)siemens.com> Gerrit-Comment-Date: Fri, 16 Dec 2022 13:28:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Jan Samek <jan.samek(a)siemens.com> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: chipset_enable.c: add PCI ID for TGL-UP3 SPI controller
by Jan Samek (Code Review) 16 Dec '22

16 Dec '22

Attention is currently required from: Angel Pons. Jan Samek has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70388 ) Change subject: chipset_enable.c: add PCI ID for TGL-UP3 SPI controller ...................................................................... Patch Set 2: (1 comment) File chipset_enable.c: https://review.coreboot.org/c/flashrom/+/70388/comment/4afe29de_c155ab1f PS1, Line 2083: 0xa0a4 > Thanks for looking it up! All operations (-r, -E, -w, -v) on TGL-UP3 proved working with 8086:0a88. -- To view, visit https://review.coreboot.org/c/flashrom/+/70388 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie93af14eb5857bfe51964f6565e475b6249dd407 Gerrit-Change-Number: 70388 Gerrit-PatchSet: 2 Gerrit-Owner: Jan Samek <jan.samek(a)siemens.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Fri, 16 Dec 2022 11:19:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Jan Samek <jan.samek(a)siemens.com> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: chipset_enable.c: add PCI ID for TGL-UP3 SPI controller
by Jan Samek (Code Review) 16 Dec '22

16 Dec '22

Attention is currently required from: Jan Samek. Hello build bot (Jenkins), Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/70388 to look at the new patch set (#2). Change subject: chipset_enable.c: add PCI ID for TGL-UP3 SPI controller ...................................................................... chipset_enable.c: add PCI ID for TGL-UP3 SPI controller Add PCI ID for the Tiger Lake UP3 (Industrial SKU) SoC. Change-Id: Ie93af14eb5857bfe51964f6565e475b6249dd407 Signed-off-by: Jan Samek <jan.samek(a)siemens.com> --- M chipset_enable.c 1 file changed, 13 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/88/70388/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/70388 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie93af14eb5857bfe51964f6565e475b6249dd407 Gerrit-Change-Number: 70388 Gerrit-PatchSet: 2 Gerrit-Owner: Jan Samek <jan.samek(a)siemens.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jan Samek <jan.samek(a)siemens.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[master]: flashchips.c: Mark W25Q128.V wp as tested
by Angel Pons (Code Review) 16 Dec '22

16 Dec '22

Attention is currently required from: Edward O'Callaghan. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70866 ) Change subject: flashchips.c: Mark W25Q128.V wp as tested ...................................................................... Patch Set 1: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/70866/comment/80d885bf_d6c57247 PS1, Line 7: wp nit: Capitalise `WP`? -- To view, visit https://review.coreboot.org/c/flashrom/+/70866 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifbd5ee76f2087764ab8841ca96de6990cb31260d Gerrit-Change-Number: 70866 Gerrit-PatchSet: 1 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Fri, 16 Dec 2022 11:03:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: flashchips.c: Mark W25Q128.V wp as tested
by Angel Pons (Code Review) 16 Dec '22

16 Dec '22

Attention is currently required from: Edward O'Callaghan. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70866 ) Change subject: flashchips.c: Mark W25Q128.V wp as tested ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/70866 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifbd5ee76f2087764ab8841ca96de6990cb31260d Gerrit-Change-Number: 70866 Gerrit-PatchSet: 1 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Fri, 16 Dec 2022 11:02:50 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom.c:Add function to get a flattened view of the chip erase blocks
by Simon Buhrow (Code Review) 16 Dec '22

16 Dec '22

Attention is currently required from: Nico Huber, Thomas Heijligen, Paul Menzel, Edward O'Callaghan, Aarya. Simon Buhrow has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/65879 ) Change subject: flashrom.c:Add function to get a flattened view of the chip erase blocks ...................................................................... Patch Set 45: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/65879/comment/17f9df2c_0d26676c PS42, Line 1249: index > maybe a better identifier than "index" and short hand for "i"ndex then? Maybe: `index` -> `layout_idx` `i` -> `eraser_idx` looks like this: layout[layout_idx] chip->block_erasers[eraser_idx] Would be better to understand at declaration level. However as an array_index it´s a bit doubling the words. Don´t know what to prefer. What do you think? -- To view, visit https://review.coreboot.org/c/flashrom/+/65879 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iafe78de00daa55f7114bd4ce09465dd88074ece4 Gerrit-Change-Number: 65879 Gerrit-PatchSet: 45 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Simon Buhrow Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.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: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Fri, 16 Dec 2022 09:42:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Simon Buhrow Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Aarya <aarya.chaumal(a)gmail.com> Gerrit-MessageType: comment

1 0

[L] Change in flashrom[master]: ch347_spi.c: Add initial support for the WCH CH347
by Anastasia Klimchuk (Code Review) 16 Dec '22

16 Dec '22

Attention is currently required from: Thomas Heijligen, Angel Pons, qianfan, Nicholas Chin. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70573 ) Change subject: ch347_spi.c: Add initial support for the WCH CH347 ...................................................................... Patch Set 3: (1 comment) Patchset: PS3: > WCH has a greate tool for CH347 named CH347Demo and you can get it from https://www.wch. […] Nicholas, Qianfan, thank you so much for your contribution! It doesn't happen often that we get a new programmer, and now you two are working on it - this is so cool! I really hope you will be able to rebase one patch on the top of the other, to get the best of both, and so that none of your work is lost. I apologise that I haven't reviewed patches myself, and I won't be able until around mid-January. I will do that, unless of course the patches already merged to that time. Although, realistically, end of Dec - beginning Jan is a time when people might have less availability. In any case: if we fast-forward to the time when patches are merged, I have a question for you both. Would you agree to add yourselves into flashrom's MAINTAINERS file for ch347_spi.c ? That would mean: if someone sends a patch modifying ch347_spi.c, you will be automatically added as reviewers. Also, MAINTAINERS serves as a piece of documentation, to find people "who knows something about X?". It would be really great if you agree! -- To view, visit https://review.coreboot.org/c/flashrom/+/70573 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I31b86c41076cc45d4a416a73fa1131350fb745ba Gerrit-Change-Number: 70573 Gerrit-PatchSet: 3 Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Alexander Goncharov <chat(a)joursoir.net> Gerrit-CC: qianfan <qianfanguijin(a)163.com> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: qianfan <qianfanguijin(a)163.com> Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Comment-Date: Fri, 16 Dec 2022 06:25:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: qianfan <qianfanguijin(a)163.com> Comment-In-Reply-To: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: flashchips.c: Mark W25Q128.V wp as tested
by Anastasia Klimchuk (Code Review) 16 Dec '22

16 Dec '22

Attention is currently required from: Edward O'Callaghan. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70866 ) Change subject: flashchips.c: Mark W25Q128.V wp as tested ...................................................................... Patch Set 1: Code-Review+2 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/70866/comment/711131ed_2043cc85 PS1, Line 9: b:? What does it mean? :) -- To view, visit https://review.coreboot.org/c/flashrom/+/70866 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifbd5ee76f2087764ab8841ca96de6990cb31260d Gerrit-Change-Number: 70866 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Fri, 16 Dec 2022 04:52:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: flashchips.c: Mark W25Q128.V wp as tested
by Edward O'Callaghan (Code Review) 16 Dec '22

16 Dec '22

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/70866 ) Change subject: flashchips.c: Mark W25Q128.V wp as tested ...................................................................... flashchips.c: Mark W25Q128.V wp as tested BUG=b:? TEST=`-p internal --wp-status`. Change-Id: Ifbd5ee76f2087764ab8841ca96de6990cb31260d Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M flashchips.c 1 file changed, 14 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/66/70866/1 diff --git a/flashchips.c b/flashchips.c index 168cc7d..9818eac 100644 --- a/flashchips.c +++ b/flashchips.c @@ -17586,7 +17586,7 @@ /* OTP: 1024B total, 256B reserved; read 0x48; write 0x42, erase 0x44, read ID 0x4B */ .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_WRSR_EXT2 | FEATURE_WRSR2 | FEATURE_WRSR3, - .tested = TEST_OK_PREW, + .tested = TEST_OK_PREWB, .probe = PROBE_SPI_RDID, .probe_timing = TIMING_ZERO, .block_erasers = -- To view, visit https://review.coreboot.org/c/flashrom/+/70866 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifbd5ee76f2087764ab8841ca96de6990cb31260d Gerrit-Change-Number: 70866 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[master]: ichspi: Expose flash descriptor regions through get_region()
by Edward O'Callaghan (Code Review) 16 Dec '22

16 Dec '22

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/70127 ) ( 9 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: ichspi: Expose flash descriptor regions through get_region() ...................................................................... ichspi: Expose flash descriptor regions through get_region() Region attributes are now stored in a `fd_regions` array after being decoded in ich9_handle_frap() and used by ich_get_region(). A special cases is handled in ich_get_region(): if there is a gap between two flash regions, an artificial region is created to fill the gap. I.e. any address inside the gap will return a region that spans the gap between the end the of the previous region and the start of the next region. This allows ich_get_region() to be used to iterate the entire flash region-by-region. Read and write operations are assumed to be allowed inside gaps between regions. BUG=b:260440773 BRANCH=none TEST=flashrom -{r,w,E,v} on dedede (JSL) Change-Id: I019f3f407f6a2a82f686a168457e0e32961ff483 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/70127 Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M ichspi.c 1 file changed, 102 insertions(+), 4 deletions(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved diff --git a/ichspi.c b/ichspi.c index bc52522..7b23b42 100644 --- a/ichspi.c +++ b/ichspi.c @@ -1262,12 +1262,22 @@ return result; } +#define MAX_FD_REGIONS 16 +struct fd_region { + const char* name; + enum ich_access_protection level; + uint32_t base; + uint32_t limit; +}; + struct hwseq_data { uint32_t size_comp0; uint32_t size_comp1; uint32_t addr_mask; bool only_4k; uint32_t hsfc_fcycle; + + struct fd_region fd_regions[MAX_FD_REGIONS]; }; static struct hwseq_data *get_hwseq_data_from_context(const struct flashctx *flash) @@ -1401,6 +1411,57 @@ return ich_hwseq_wait_for_cycle_complete(len, ich_gen, addr_mask); } +static void ich_get_region(const struct flashctx *flash, unsigned int addr, struct flash_region *region) +{ + struct ich_descriptors desc = { 0 }; + const ssize_t nr = ich_number_of_regions(ich_generation, &desc.content); + const struct hwseq_data *hwseq_data = get_hwseq_data_from_context(flash); + const struct fd_region *fd_regions = hwseq_data->fd_regions; + + /* + * Set default values for the region. If no flash descriptor containing + * addr is found, these values will be used instead. + * + * The region start and end are constrained so that they do not overlap + * any flash descriptor regions. + */ + const char *name = ""; + region->read_prot = false; + region->write_prot = false; + region->start = 0; + region->end = flashrom_flash_getsize(flash); + + for (ssize_t i = 0; i < nr; i++) { + uint32_t base = fd_regions[i].base; + uint32_t limit = fd_regions[i].limit; + enum ich_access_protection level = fd_regions[i].level; + + if (addr < base) { + /* + * fd_regions[i] starts after addr, constrain + * region->end so that it does not overlap. + */ + region->end = min(region->end, base); + } else if (addr > limit) { + /* + * fd_regions[i] ends before addr, constrain + * region->start so that it does not overlap. + */ + region->start = max(region->start, limit + 1); + } else { + /* fd_regions[i] contains addr, copy to *region. */ + name = fd_regions[i].name; + region->start = base; + region->end = limit + 1; + region->read_prot = (level == LOCKED) || (level == READ_PROT); + region->write_prot = (level == LOCKED) || (level == WRITE_PROT); + break; + } + } + + region->name = strdup(name); +} + /* Given RDID info, return pointer to entry in flashchips[] */ static const struct flashchip *flash_id_to_entry(uint32_t mfg_id, uint32_t model_id) { @@ -1768,7 +1829,8 @@ "read-write", "write-only", "read-only", "locked" }; -static enum ich_access_protection ich9_handle_frap(uint32_t frap, unsigned int i) +static enum ich_access_protection ich9_handle_frap(struct fd_region *fd_regions, + uint32_t frap, unsigned int i) { static const char *const region_names[] = { "Flash Descriptor", "BIOS", "Management Engine", @@ -1811,6 +1873,12 @@ msg_pinfo("FREG%u: %s region (0x%08x-0x%08x) is %s.\n", i, region_name, base, limit, access_names[rwperms]); + /* Save region attributes for use by ich_get_region(). */ + fd_regions[i].base = base; + fd_regions[i].limit = limit; + fd_regions[i].level = rwperms; + fd_regions[i].name = region_name; + return rwperms; } @@ -1904,6 +1972,7 @@ .erase = ich_hwseq_block_erase, .read_register = ich_hwseq_read_status, .write_register = ich_hwseq_write_status, + .get_region = ich_get_region, .shutdown = ich_hwseq_shutdown, }; @@ -2047,8 +2116,7 @@ struct ich_descriptors desc = { 0 }; enum ich_spi_mode ich_spi_mode = ich_auto; size_t num_freg, num_pr, reg_pr0; - - struct hwseq_data hwseq_data; + struct hwseq_data hwseq_data = { 0 }; init_chipset_properties(&swseq_data, &hwseq_data, &num_freg, &num_pr, &reg_pr0, ich_gen); int ret = get_ich_spi_mode_param(cfg, &ich_spi_mode); @@ -2109,7 +2177,7 @@ /* Handle FREGx and FRAP registers */ for (i = 0; i < num_freg; i++) - ich_spi_rw_restricted |= ich9_handle_frap(tmp, i); + ich_spi_rw_restricted |= ich9_handle_frap(hwseq_data.fd_regions, tmp, i); if (ich_spi_rw_restricted) msg_pinfo("Not all flash regions are freely accessible by flashrom. This is " "most likely\ndue to an active ME. Please see " -- To view, visit https://review.coreboot.org/c/flashrom/+/70127 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I019f3f407f6a2a82f686a168457e0e32961ff483 Gerrit-Change-Number: 70127 Gerrit-PatchSet: 14 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit December 2022