April 2022 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: ichspi: Unify timeouts across all SPI operations to 30s
by Angel Pons (Code Review) 03 Apr '22

03 Apr '22

Attention is currently required from: Subrata Banik, Nico Huber, Paul Menzel, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62867 ) Change subject: ichspi: Unify timeouts across all SPI operations to 30s ...................................................................... Patch Set 8: (1 comment) Patchset: PS5: > > Oh I was referring to previous code, […] I think the reason to use smaller timeout values in some cases is to avoid unnecessary delays. 30 seconds is the recommended timeout for the worst-case scenario. Personally, I don't think using the 30-sec timeout everywhere is beneficial. -- To view, visit https://review.coreboot.org/c/flashrom/+/62867 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b Gerrit-Change-Number: 62867 Gerrit-PatchSet: 8 Gerrit-Owner: Subrata Banik <subratabanik(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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 03 Apr 2022 21:44:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ich_descriptors.c: Ensure unsigned types >=0 on to prevent underflow
by Angel Pons (Code Review) 03 Apr '22

03 Apr '22

Attention is currently required from: Edward O'Callaghan, Light, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62764 ) Change subject: ich_descriptors.c: Ensure unsigned types >=0 on to prevent underflow ...................................................................... Patch Set 16: (1 comment) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/62764/comment/98f0a539_5620f3a8 PS14, Line 501: for (j = 0; j < (size_t)min(num_regions, 12); j++) > > Not sure why you don't believe it is the max bounds in this case? > > Because the second loop (the one which is modified in the patch) starts with 12, and runs until num_regions. So 12 is a starting point of the loop, not the maximum. We can't use "max" in the name. > > I want to understand what exactly this 12 means. It probably means that Intel initially thought that having 12 regions in the IFD would be more than enough for any future needs. Later, they realized that they needed more regions, so they added a new set of bitfields to have more regions. I imagine they preserved the field position for the original 12 regions for backwards compatibility purposes. You can see the 12 here: /* From Skylake on */ struct { uint32_t ext_read :4, ext_write :4, read :12, write :12; } mstr[MAX_NUM_MASTERS]; -- To view, visit https://review.coreboot.org/c/flashrom/+/62764 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5921cc571f3dca5188ca1973dba6ececbcbe2f39 Gerrit-Change-Number: 62764 Gerrit-PatchSet: 16 Gerrit-Owner: Light <aarya.chaumal(a)gmail.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: 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: Light <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 03 Apr 2022 21:36:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Light <aarya.chaumal(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [WIP] flashchips: Add support for GigaDevice GD25LR256E, GD251R512ME
by Ravishankar Sarawadi (Code Review) 03 Apr '22

03 Apr '22

Attention is currently required from: Ravishankar Sarawadi. Hello build bot (Jenkins), Angel Pons, Sukumar Ghorai, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/58025 to look at the new patch set (#2). Change subject: [WIP] flashchips: Add support for GigaDevice GD25LR256E, GD251R512ME ...................................................................... [WIP] flashchips: Add support for GigaDevice GD25LR256E, GD251R512ME Datasheets: https://www.gigadevice.com/datasheet/gd25r256e/ https://www.gigadevice.com/datasheet/gd25lr512me/ BUG=none BRANCH=none TEST= Flash image using Flashrom Tool sudo dut-control cold_reset:on fw_wp_en:off spi2_buf_en:on spi2_buf_on_flex_en:on spi2_vref:pp1800 spi_hold:off sudo flashrom -V -p raiden_debug_spi -w <test_binary> sudo dut-control spi2_buf_en:off spi2_buf_on_flex_en:off spi2_vref:off spi_hold:off cold_reset:off Change-Id: I2fe6bc1219cd1ee19b93caabab69de938cfc44b0 Signed-off-by: Ravi Sarawadi <ravishankar.sarawadi(a)intel.com> --- M flashchips.c M flashchips.h 2 files changed, 96 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/25/58025/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/58025 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2fe6bc1219cd1ee19b93caabab69de938cfc44b0 Gerrit-Change-Number: 58025 Gerrit-PatchSet: 2 Gerrit-Owner: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Sukumar Ghorai <sukumar.ghorai(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.corp-partner.google.com> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: ch341a_spi.c: Reset device before use
by Angel Pons (Code Review) 03 Apr '22

03 Apr '22

Attention is currently required from: Nico Huber, Thomas Heijligen, Arthur Heymans. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63258 ) Change subject: ch341a_spi.c: Reset device before use ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/63258 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iff824c22dc871bb2d9504abe8b5ad4e4c191c4af Gerrit-Change-Number: 63258 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Sun, 03 Apr 2022 21:14:26 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: .gitignore: Avoid committing libflashrom.a
by Thomas Heijligen (Code Review) 03 Apr '22

03 Apr '22

Attention is currently required from: Edward O'Callaghan. Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63320 ) Change subject: .gitignore: Avoid committing libflashrom.a ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: duplicate of CB:62826 This patch adds all *.a to gitignore, not only libflashrom.a which is mentioned in the commit message -- To view, visit https://review.coreboot.org/c/flashrom/+/63320 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie9ca7c6ab846d0b07c16798cdd3ff5aa0a748a9b Gerrit-Change-Number: 63320 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Sun, 03 Apr 2022 12:43:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ich_descriptor.c: Simplify get_density lookups by deduplicate sw
by Edward O'Callaghan (Code Review) 03 Apr '22

03 Apr '22

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/63321 ) Change subject: ich_descriptor.c: Simplify get_density lookups by deduplicate sw ...................................................................... ich_descriptor.c: Simplify get_density lookups by deduplicate sw These two switch statements look up the same thing with only the return type as different. Therefore deduce out the density encoding lookup from the use-case logic. TEST=`flashrom -p internal --flash-name` on Intel Kaby Lake U w/ iHDCP2.2 Prem. Change-Id: Ieb4e9f19f4f06becd344fe08829dabe20c3c8de0 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M ich_descriptors.c 1 file changed, 50 insertions(+), 79 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/63321/1 diff --git a/ich_descriptors.c b/ich_descriptors.c index 7c03499..ec8546c 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -161,26 +161,17 @@ msg_pdbg2("\n"); } -static const char *pprint_density(enum ich_chipset cs, const struct ich_descriptors *desc, uint8_t idx) +static int get_density(enum ich_chipset cs, const struct ich_descriptors *desc, uint8_t idx, uint8_t *size_enc, uint8_t *size_max) { + *size_enc = 0; + *size_max = 0; + if (idx > 1) { msg_perr("Only ICH SPI component index 0 or 1 are supported yet.\n"); - return NULL; + return -2; } - if (desc->content.NC == 0 && idx > 0) - return "unused"; - - static const char * const size_str[] = { - "512 kB", /* 0000 */ - "1 MB", /* 0001 */ - "2 MB", /* 0010 */ - "4 MB", /* 0011 */ - "8 MB", /* 0100 */ - "16 MB", /* 0101 */ /* Maximum up to Lynx Point (excl.) */ - "32 MB", /* 0110 */ - "64 MB", /* 0111 */ - }; + return 0; switch (cs) { case CHIPSET_ICH8: @@ -190,15 +181,13 @@ case CHIPSET_6_SERIES_COUGAR_POINT: case CHIPSET_7_SERIES_PANTHER_POINT: case CHIPSET_BAYTRAIL: { - uint8_t size_enc; if (idx == 0) { - size_enc = desc->component.dens_old.comp1_density; + *size_enc = desc->component.dens_old.comp1_density; } else { - size_enc = desc->component.dens_old.comp2_density; + *size_enc = desc->component.dens_old.comp2_density; } - if (size_enc > 5) - return "reserved"; - return size_str[size_enc]; + *size_max = 5; + break; } case CHIPSET_8_SERIES_LYNX_POINT: case CHIPSET_8_SERIES_LYNX_POINT_LP: @@ -215,20 +204,48 @@ case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: case CHIPSET_ELKHART_LAKE: { - uint8_t size_enc; if (idx == 0) { - size_enc = desc->component.dens_new.comp1_density; + *size_enc = desc->component.dens_new.comp1_density; } else { - size_enc = desc->component.dens_new.comp2_density; + *size_enc = desc->component.dens_new.comp2_density; } - if (size_enc > 7) - return "reserved"; - return size_str[size_enc]; + *size_max = 7; + break; } case CHIPSET_ICH_UNKNOWN: default: - return "unknown"; + msg_pwarn("Density encoding is unknown on this chipset.\n"); + return -1; } + return 1; +} + +static const char *pprint_density(enum ich_chipset cs, const struct ich_descriptors *desc, uint8_t idx) +{ + static const char * const size_str[] = { + "512 kB", /* 0000 */ + "1 MB", /* 0001 */ + "2 MB", /* 0010 */ + "4 MB", /* 0011 */ + "8 MB", /* 0100 */ + "16 MB", /* 0101 */ /* Maximum up to Lynx Point (excl.) */ + "32 MB", /* 0110 */ + "64 MB", /* 0111 */ + }; + + uint8_t size_enc; + uint8_t size_max; + + int ret = get_density(cs, desc, idx, &size_enc, &size_max); + if (ret == -2) + return NULL; + else if (ret == -1) + return "unknown"; + else if (ret == 0) + return "unused"; + else if (size_enc > size_max) + return "reserved"; + return size_str[size_enc]; } static const char *pprint_freq(enum ich_chipset cs, uint8_t value) @@ -1194,61 +1211,15 @@ \em idx in bytes or -1 if the correct size can not be determined. */ int getFCBA_component_density(enum ich_chipset cs, const struct ich_descriptors *desc, uint8_t idx) { - if (idx > 1) { - msg_perr("Only ICH SPI component index 0 or 1 are supported yet.\n"); - return -1; - } - - if (desc->content.NC == 0 && idx > 0) - return 0; - uint8_t size_enc; uint8_t size_max; - switch (cs) { - case CHIPSET_ICH8: - case CHIPSET_ICH9: - case CHIPSET_ICH10: - case CHIPSET_5_SERIES_IBEX_PEAK: - case CHIPSET_6_SERIES_COUGAR_POINT: - case CHIPSET_7_SERIES_PANTHER_POINT: - case CHIPSET_BAYTRAIL: - if (idx == 0) { - size_enc = desc->component.dens_old.comp1_density; - } else { - size_enc = desc->component.dens_old.comp2_density; - } - size_max = 5; - break; - case CHIPSET_8_SERIES_LYNX_POINT: - case CHIPSET_8_SERIES_LYNX_POINT_LP: - case CHIPSET_8_SERIES_WELLSBURG: - case CHIPSET_9_SERIES_WILDCAT_POINT: - case CHIPSET_9_SERIES_WILDCAT_POINT_LP: - case CHIPSET_100_SERIES_SUNRISE_POINT: - case CHIPSET_C620_SERIES_LEWISBURG: - case CHIPSET_300_SERIES_CANNON_POINT: - case CHIPSET_400_SERIES_COMET_POINT: - case CHIPSET_500_SERIES_TIGER_POINT: - case CHIPSET_600_SERIES_ALDER_POINT: - case CHIPSET_METEOR_LAKE: - case CHIPSET_APOLLO_LAKE: - case CHIPSET_GEMINI_LAKE: - case CHIPSET_ELKHART_LAKE: - if (idx == 0) { - size_enc = desc->component.dens_new.comp1_density; - } else { - size_enc = desc->component.dens_new.comp2_density; - } - size_max = 7; - break; - case CHIPSET_ICH_UNKNOWN: - default: - msg_pwarn("Density encoding is unknown on this chipset.\n"); + int ret = get_density(cs, desc, idx, &size_enc, &size_max); + if (ret < 0) return -1; - } - - if (size_enc > size_max) { + else if (ret == 0) + return 0; + else if (size_enc > size_max) { msg_perr("Density of ICH SPI component with index %d is invalid.\n" "Encoded density is 0x%x while maximum allowed is 0x%x.\n", idx, size_enc, size_max); -- To view, visit https://review.coreboot.org/c/flashrom/+/63321 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ieb4e9f19f4f06becd344fe08829dabe20c3c8de0 Gerrit-Change-Number: 63321 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: .gitignore: Avoid committing libflashrom.a
by Edward O'Callaghan (Code Review) 03 Apr '22

03 Apr '22

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/63320 ) Change subject: .gitignore: Avoid committing libflashrom.a ...................................................................... .gitignore: Avoid committing libflashrom.a Change-Id: Ie9ca7c6ab846d0b07c16798cdd3ff5aa0a748a9b Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M .gitignore 1 file changed, 1 insertion(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/20/63320/1 diff --git a/.gitignore b/.gitignore index 7c45aba..15cc77a 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ +*.a *.d !Makefile.d/ *.o -- To view, visit https://review.coreboot.org/c/flashrom/+/63320 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie9ca7c6ab846d0b07c16798cdd3ff5aa0a748a9b Gerrit-Change-Number: 63320 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: pony_spi.c: Extract out get_params to simplify init
by Angel Pons (Code Review) 03 Apr '22

03 Apr '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Light, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63130 ) Change subject: pony_spi.c: Extract out get_params to simplify init ...................................................................... Patch Set 3: (7 comments) File pony_spi.c: https://review.coreboot.org/c/flashrom/+/63130/comment/8833d157_f0edb292 PS3, Line 153: { nit: add space before opening brace https://review.coreboot.org/c/flashrom/+/63130/comment/10aeaede_615bf794 PS3, Line 167: bool have_device; > From my point of view: Review doesn't scale well with the size of > a patch. Even if it's a very simple patch hunk, every additional > hunk can distract from the error-prone part of a patch. Doing fixes > and simple changes like this in advance works well, FWIW. +1 For me, it's very easy to miss something when reviewing commits that do multiple things at once. I'd rather review multiple commits where each does one and only one thing. Whenever I want/need to make significant code changes, I draft in advance a "plan of action". I use several criteria to segment the changes into commits, one of them is "at most one thing/reason/idea per commit". In this case, "simplify `pony_spi` init logic" and "use `bool` type for boolean variables" are two different things, and they are conceptually independent: disregarding merge conflicts, it's possible to do them in either order, or only do one of them. When accounting for merge conflicts, I start with the easiest (most likely to be approved) commits, so that I don't have to manually rebase them if another commit needs to be fixed. That being said, there's no need to over-split things. For example, I'd retype both `have_device` and `have_prog` in the same commit, because the rationale is the same. TL;DR: Please retype the variables in a separate commit (preferably before the "extract `get_params`" commit to streamline the submission process). https://review.coreboot.org/c/flashrom/+/63130/comment/523207c5_d0f3c3b6 PS3, Line 171: return 1; If the `dev` parameter is correct (i.e. `have_device` is true) but `get_params()` returns -1 because of an error with the `type` parameter, the serial port (what `serialport_shutdown()` releases) will leak. https://review.coreboot.org/c/flashrom/+/63130/comment/3b064e8c_49fe92dc PS3, Line 181: return 1; Serial port can also leak here. https://review.coreboot.org/c/flashrom/+/63130/comment/229d3eda_23804449 PS3, Line 190: serialport_shutdown > I think you are mis-interpreting the diff presented by gerrit Anastasia. This line is OK here, but it's missing in some other return paths. https://review.coreboot.org/c/flashrom/+/63130/comment/54be3b14_cde22584 PS3, Line 232: true This is unrelated. Please do it in a separate commit. https://review.coreboot.org/c/flashrom/+/63130/comment/9797c619_2ea34563 PS3, Line 237: true Ditto -- To view, visit https://review.coreboot.org/c/flashrom/+/63130 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I364febc05c870683cbad114583762b0c006f4bac Gerrit-Change-Number: 63130 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Light <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> 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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Light <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 03 Apr 2022 11:26:14 +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> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: pony_spi.c: Extract out get_params to simplify init
by Paul Menzel (Code Review) 03 Apr '22

03 Apr '22

Attention is currently required from: Edward O'Callaghan, Light, Anastasia Klimchuk. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63130 ) Change subject: pony_spi.c: Extract out get_params to simplify init ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/63130/comment/77310740_a9f5f7d3 PS3, Line 9: `commit caa0335114a81` Nit: No markup needed. Maybe even add the commit message summary in () after it. -- To view, visit https://review.coreboot.org/c/flashrom/+/63130 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I364febc05c870683cbad114583762b0c006f4bac Gerrit-Change-Number: 63130 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Light <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> 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: Light <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 03 Apr 2022 05:43:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Add Jasper Lake support
by Edward O'Callaghan (Code Review) 03 Apr '22

03 Apr '22

Attention is currently required from: Nico Huber, Subrata Banik, Angel Pons. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62282 ) Change subject: ichspi: Add Jasper Lake support ...................................................................... Patch Set 10: (2 comments) Patchset: PS4: > > Unresolving then because it does look like a pch300 although I am not 100% on every tiny idiosyncr […] You are right `NM` is confusing, can we comment it better? Possibly copy what you just said there near verbatim into the `ich_number_of_masters()` function? OK, I picked to follow pch300 in `prettyprint_ich_descriptor_master()` and fixed that up. `ich_number_of_masters()` was left untouched. I wonder how much of this declarative information about the differences could be encoded into just a big table instead of scattered over heaps of switch statements in the control flow itself? A lot of it doesn't seem dynamic just XXX has 3 and YYY has 4 of "thing". Resolved? File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/62282/comment/62044ba6_c49c4d9d PS8, Line 1061: CHIPSET_ELKHART_LAKE > Google should have access too. Since ~SKL, both the SPI guide and FIT are […] Long story about the access ACL's there.. Can talk out of band about it || summarised as 'a lot of nonsense'. -- To view, visit https://review.coreboot.org/c/flashrom/+/62282 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib942d0b8942fe0a991b2af0b187414818485153d Gerrit-Change-Number: 62282 Gerrit-PatchSet: 10 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> 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: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sun, 03 Apr 2022 00:30:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit April 2022