- flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: Add Tiger Lake U Premium support
by Michał Żygowski (Code Review) 01 Jul '21

01 Jul '21

Attention is currently required from: Nico Huber. Hello build bot (Jenkins), Nico Huber, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/55578 to look at the new patch set (#2). Change subject: Add Tiger Lake U Premium support ...................................................................... Add Tiger Lake U Premium support Tiger Lake has very low ICCRIBA (TGL=0x11, CNL=0x34 and CML=0x34) and detects as uknown chipset compatible with 300 series chipset. Add a new enum CHIPSET_500_SERIES_TIGER_POINT and treat it identically to CHIPSET_400_SERIES_COMET_POINT except the ICCRIBA in descriptor guessing, pprint_freq and boot straps. TEST=Flash BIOS region on Intel i5-1135G7 Signed-off-by: Michał Żygowski <michal.zygowski(a)3mdeb.com> Change-Id: I28f3b6fe9f8ce9e976a6808683f46b6f4ec72bdd --- 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, 39 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/78/55578/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/55578 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I28f3b6fe9f8ce9e976a6808683f46b6f4ec72bdd Gerrit-Change-Number: 55578 Gerrit-PatchSet: 2 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: programmer: Introduce default shutdown function
by Angel Pons (Code Review) 01 Jul '21

01 Jul '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54890 ) Change subject: programmer: Introduce default shutdown function ...................................................................... Patch Set 5: Code-Review+2 (1 comment) Patchset: PS5: > There are two reasons why I thought of default shutdown function. […] Personally, I don't mind adding something that ends up being removed later, as long as it provides some benefit. I can see why this `default_shutdown()` function may end up being unnecessary in the future, but this commit would ease (even if only slightly) follow-up works as there would be less shutdown functions to adapt and/or remove. -- To view, visit https://review.coreboot.org/c/flashrom/+/54890 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8b20717ba549e12edffbc5d1643ff064a4f0c517 Gerrit-Change-Number: 54890 Gerrit-PatchSet: 5 Gerrit-Owner: 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: 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-Comment-Date: Thu, 01 Jul 2021 09:09:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ich_descriptors.c: Fix PCH detection for Tiger Lake
by Michał Żygowski (Code Review) 01 Jul '21

01 Jul '21

Michał Żygowski has restored this change. ( https://review.coreboot.org/c/flashrom/+/55577 ) Change subject: ich_descriptors.c: Fix PCH detection for Tiger Lake ...................................................................... Restored -- To view, visit https://review.coreboot.org/c/flashrom/+/55577 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib61d432ab4eaf00aa4eef50d2844940e73b5cad6 Gerrit-Change-Number: 55577 Gerrit-PatchSet: 1 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: restore

1 0

Change in flashrom[master]: ich_descriptors.c: Fix PCH detection for Tiger Lake
by Michał Żygowski (Code Review) 01 Jul '21

01 Jul '21

Michał Żygowski has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/55577 ) Change subject: ich_descriptors.c: Fix PCH detection for Tiger Lake ...................................................................... Abandoned ICH descriptor probing clean up in separate patch series -- To view, visit https://review.coreboot.org/c/flashrom/+/55577 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib61d432ab4eaf00aa4eef50d2844940e73b5cad6 Gerrit-Change-Number: 55577 Gerrit-PatchSet: 1 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: abandon

1 0

Change in flashrom[master]: dediprog: Init-shutdown test for dediprog
by Anastasia Klimchuk (Code Review) 01 Jul '21

01 Jul '21

Attention is currently required from: Nico Huber, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55934 ) Change subject: dediprog: Init-shutdown test for dediprog ...................................................................... Patch Set 3: (5 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/55934/comment/032ff14a_606a4d45 PS3, Line 9: This patch adds mocks for libusb functions. To avoid dependency on > I expect that some of the code could be reused to test other libusb programmers. […] Yes, hopefully all __wrap functions will be reused for other programmers, because programmer-specific behaviour goes into current_io. Actually, the wraps here are not expected to have much code, if something needs code that code would probably go into current_io and live in the test itself. Moving mocks into separate compilation unit would need a bit of refactoring, because at the moment there is current_io in this file, and also io_mock_register. Ideally, I would add one or two more tests which use libusb wraps, and then maybe it will be more obvious [to me] how to move it / where to move it? File tests/init_shutdown.c: https://review.coreboot.org/c/flashrom/+/55934/comment/88644d17_af5ef6ad PS3, Line 98: /* dediprog_cmds CMD_READ_PROG_INFO */ > Hmmm, how about making a patch that moves the dediprog definitions the tests need into a header? This question came up in the review process of commit 21e22ba8a7750f1cfe5cd3323e3137695ffef0a4, discussion started at Patchset 10 May 01 01:59 init_shutdown.c#53 and the decision was not to move definitions into the header. But things could have changed, if everyone agrees it's a good idea I can do it. File tests/io_mock.h: https://review.coreboot.org/c/flashrom/+/55934/comment/f5519b3b_0d77aa24 PS3, Line 34: /* Define libusb symbols to avoid dependency on libusb.h */ > Now that I realized that we don't mock `libusb.h` for the code under […] I wrote this tests few weeks ago, but was waiting for other patches for tests/ to be done. So initially I included libusb.h without much thinking. But then commit 20ff3d464ba20b385b84aa0eb132eeff3345accc and especially commit 4abb62c4ac1f25e40f2569535322342b9c939558 have happened, and I became more aware on what can be a consequence of including a header. Returned back to tests, and first thing I saw was libusb.h with may not be supported. In an environment where libusb.h is not supported, I can't include it right? This would mean adding pre-processor guards, which I understand better to avoid? Then I thought the only reason I included the header is for the libusb_device_handle type. But real handles are not needed for tests, so I replaced with void* and boom, everything works and header not needed. Individual tests are guarded by CONFIG_XXX anyway, so test would be skipped if driver is not built? and driver not built is libusb not supported. File tests/io_mock.h: https://review.coreboot.org/c/flashrom/+/55934/comment/ebc6961a_0765c50b PS2, Line 52: */ > Oh, I missed earlier that you avoid the libusb dependency here while […] WOW thank you for explanation... So many opportunities to hack things! File tests/tests.c: https://review.coreboot.org/c/flashrom/+/55934/comment/a03f0286_26c59f57 PS3, Line 140: 2021 > Is this magic number the current year? Or does it have another meaning? Yeah that's the current year. I needed something to pretend it's a valid file descriptor, and then something as a valid device handle, and there will be more of that I am sure. The number itself doesn't matter. Is this an acceptable approach, or maybe I should create a VALID_POINTER macro instead of inline magic number? -- To view, visit https://review.coreboot.org/c/flashrom/+/55934 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I38508dfb6d7c24d42522f22fcae0c5e410c5f7ea Gerrit-Change-Number: 55934 Gerrit-PatchSet: 3 Gerrit-Owner: 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-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Thu, 01 Jul 2021 00:48:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [RFC] spi_master: Add shutdown function in spi_master struct
by Anastasia Klimchuk (Code Review) 01 Jul '21

01 Jul '21

Attention is currently required from: Nico Huber, Thomas Heijligen, Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55932 ) Change subject: [RFC] spi_master: Add shutdown function in spi_master struct ...................................................................... Patch Set 2: (1 comment) Patchset: PS1: > The "few" wasn't meant literally :) […] Oh and I also can see, for the masters without programmer entry, the entry does not even exist in programmer_table.c. Maybe then my question doesn't make sense, but initially it was, why two structs are separated, and there is no way to get programmer entry from spi_master. There is for example programmer name and type in programmer_entry, but no way to access this from an instance of spi_master. Is the answer "it should never be needed" or "it was never needed"? -- To view, visit https://review.coreboot.org/c/flashrom/+/55932 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2dc80dceca2f8204bcd0dad1f51753d7e79f1af5 Gerrit-Change-Number: 55932 Gerrit-PatchSet: 2 Gerrit-Owner: 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: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 30 Jun 2021 23:25:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: programmer: Introduce default shutdown function
by Anastasia Klimchuk (Code Review) 01 Jul '21

01 Jul '21

Attention is currently required from: Nico Huber, Edward O'Callaghan. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54890 ) Change subject: programmer: Introduce default shutdown function ...................................................................... Patch Set 5: (1 comment) Patchset: PS5: > I wanted to say: let's not bikeshed, we won't need it for long. […] There are two reasons why I thought of default shutdown function. First reason is illustrated in this patch: instead of adding the simplest (and identical code) shutdown function into individual masters, have it added once and reuse. There are ~10 more par masters in the same situation. I am afraid we may need it for longer, the more I think about it. Strictly speaking, it matters when freeing the data (not allocation) moves to common code. But looking at how it currently works, shutdown function is needed to be registered (even if it does nothing), as a signal to free data. It all can be re-written, but that's what I mean "we may need it for longer". Second reason, completely independent is that some time ago when we were discussing CB:51676, there was an idea that shutdown function should be required. I think, if it is required, maybe to have default function available? There is `default_spi_send_multicommand` for example. -- To view, visit https://review.coreboot.org/c/flashrom/+/54890 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8b20717ba549e12edffbc5d1643ff064a4f0c517 Gerrit-Change-Number: 54890 Gerrit-PatchSet: 5 Gerrit-Owner: 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Wed, 30 Jun 2021 21:57:15 +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]: ich_descriptors: Normalize chipset detection
by Nico Huber (Code Review) 01 Jul '21

01 Jul '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55651 ) Change subject: ich_descriptors: Normalize chipset detection ...................................................................... ich_descriptors: Normalize chipset detection Normalize the fallback paths: * Always end with the newest, assumed compatible chipset. * Perform tighter checks when it's about warnings only. * If two chipsets seem compatible, always return the same (this is currently the case for 8/9 series and 300/400 series which we can't distinguish). Change-Id: Ic5a5fee870202173b3a9813b03ec261e8ee45155 Signed-off-by: Nico Huber <nico.h(a)gmx.de> Reviewed-on: https://review.coreboot.org/c/flashrom/+/55651 Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Michał Żygowski <michal.zygowski(a)3mdeb.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M ich_descriptors.c 1 file changed, 7 insertions(+), 11 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Michał Żygowski: Looks good to me, approved diff --git a/ich_descriptors.c b/ich_descriptors.c index 10355d9..aba7528 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -933,15 +933,13 @@ return CHIPSET_ICH9; if (content->ISL <= 10) return CHIPSET_ICH10; - if (content->ISL <= 16) - return CHIPSET_5_SERIES_IBEX_PEAK; if (content->FLMAP2 == 0) { - if (content->ISL == 23) - return CHIPSET_GEMINI_LAKE; - warn_peculiar_desc(content->ISL != 19, "Apollo Lake"); - return CHIPSET_APOLLO_LAKE; + if (content->ISL == 19) + return CHIPSET_APOLLO_LAKE; + warn_peculiar_desc(content->ISL != 23, "Gemini Lake"); + return CHIPSET_GEMINI_LAKE; } - warn_peculiar_desc(true, "Ibex Peak"); + warn_peculiar_desc(content->ISL != 16, "Ibex Peak"); return CHIPSET_5_SERIES_IBEX_PEAK; } else if (upper->MDTBA == 0x00) { if (content->ICCRIBA < 0x31 && content->FMSBA < 0x30) { @@ -949,10 +947,8 @@ return CHIPSET_BAYTRAIL; if (content->MSL <= 1 && content->ISL <= 18) return CHIPSET_6_SERIES_COUGAR_POINT; - if (content->MSL <= 1 && content->ISL <= 21) - return CHIPSET_8_SERIES_LYNX_POINT; - warn_peculiar_desc(true, "Wildcat Point"); - return CHIPSET_9_SERIES_WILDCAT_POINT; + warn_peculiar_desc(content->MSL != 1 || content->ISL != 21, "Lynx Point"); + return CHIPSET_8_SERIES_LYNX_POINT; } if (content->NM == 6) { warn_peculiar_desc(content->ICCRIBA > 0x34, "C620 series"); -- To view, visit https://review.coreboot.org/c/flashrom/+/55651 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic5a5fee870202173b3a9813b03ec261e8ee45155 Gerrit-Change-Number: 55651 Gerrit-PatchSet: 5 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: ich_descriptors: Don't base chipset detection on `freq_read`
by Nico Huber (Code Review) 01 Jul '21

01 Jul '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55650 ) Change subject: ich_descriptors: Don't base chipset detection on `freq_read` ...................................................................... ich_descriptors: Don't base chipset detection on `freq_read` Only warn if the `freq_read` setting looks odd but don't override our previous guess. The `freq_read` check was taken from `ifdtool` but seems less reliable than our own detection scheme. Change-Id: I658d76ec2567d1d660a18d0b0ae71c744e603e8f Signed-off-by: Nico Huber <nico.h(a)gmx.de> Reviewed-on: https://review.coreboot.org/c/flashrom/+/55650 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Michał Żygowski <michal.zygowski(a)3mdeb.com> --- M ich_descriptors.c 1 file changed, 3 insertions(+), 5 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Michał Żygowski: Looks good to me, approved diff --git a/ich_descriptors.c b/ich_descriptors.c index dbc2da4..10355d9 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -984,7 +984,7 @@ case CHIPSET_400_SERIES_COMET_POINT: case CHIPSET_GEMINI_LAKE: /* `freq_read` was repurposed, so can't check on it any more. */ - return guess; + break; case CHIPSET_100_SERIES_SUNRISE_POINT: case CHIPSET_C620_SERIES_LEWISBURG: case CHIPSET_APOLLO_LAKE: @@ -993,19 +993,17 @@ "However, the read frequency isn't set to 17MHz (the only valid value).\n" "Please report this message, the output of `ich_descriptors_tool` for\n" "your descriptor and the output of `lspci -nn` to flashrom(a)flashrom.org\n\n"); - return CHIPSET_9_SERIES_WILDCAT_POINT; } - return guess; + break; default: if (component->modes.freq_read == 6) { msg_pwarn("\nThe flash descriptor has the read frequency set to 17MHz. However,\n" "it doesn't look like a Skylake/Sunrise Point compatible descriptor.\n" "Please report this message, the output of `ich_descriptors_tool` for\n" "your descriptor and the output of `lspci -nn` to flashrom(a)flashrom.org\n\n"); - return CHIPSET_100_SERIES_SUNRISE_POINT; } - return guess; } + return guess; } /* len is the length of dump in bytes */ -- To view, visit https://review.coreboot.org/c/flashrom/+/55650 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I658d76ec2567d1d660a18d0b0ae71c744e603e8f Gerrit-Change-Number: 55650 Gerrit-PatchSet: 5 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: ich_descriptors: Revise detection for chipsets w/ ICCRIBA
by Nico Huber (Code Review) 01 Jul '21

01 Jul '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55647 ) Change subject: ich_descriptors: Revise detection for chipsets w/ ICCRIBA ...................................................................... ich_descriptors: Revise detection for chipsets w/ ICCRIBA Detection based on ICCRIBA and FMSBA became a little messy lately. However, there's a new static difference: Since 300 series (Cannon Point), there is an MDTBA field in FLUMAP1 that has always been 0 (reserved) before. Taking this into account, we can relax the checks on ICCRIBA. Change-Id: I587ad1abe390843d4a9e74431b6fc4b63f8ba512 Signed-off-by: Nico Huber <nico.h(a)gmx.de> Reviewed-on: https://review.coreboot.org/c/flashrom/+/55647 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Michał Żygowski <michal.zygowski(a)3mdeb.com> --- M ich_descriptors.c M ich_descriptors.h 2 files changed, 26 insertions(+), 22 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Michał Żygowski: Looks good to me, approved diff --git a/ich_descriptors.c b/ich_descriptors.c index 21fc40b..dbc2da4 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -921,9 +921,10 @@ /* * Guesses a minimum chipset version based on the maximum number of - * soft straps per generation. + * soft straps per generation and presence of the MIP base (MDTBA). */ -static enum ich_chipset guess_ich_chipset_from_content(const struct ich_desc_content *const content) +static enum ich_chipset guess_ich_chipset_from_content(const struct ich_desc_content *const content, + const struct ich_desc_upper_map *const upper) { if (content->ICCRIBA == 0x00) { if (content->MSL == 0 && content->ISL <= 2) @@ -942,25 +943,26 @@ } warn_peculiar_desc(true, "Ibex Peak"); return CHIPSET_5_SERIES_IBEX_PEAK; - } else if (content->ICCRIBA < 0x31 && content->FMSBA < 0x30) { - if (content->MSL == 0 && content->ISL <= 17) - return CHIPSET_BAYTRAIL; - if (content->MSL <= 1 && content->ISL <= 18) - return CHIPSET_6_SERIES_COUGAR_POINT; - if (content->MSL <= 1 && content->ISL <= 21) - return CHIPSET_8_SERIES_LYNX_POINT; - warn_peculiar_desc(true, "Wildcat Point"); - return CHIPSET_9_SERIES_WILDCAT_POINT; - } else if (content->ICCRIBA < 0x34) { - if (content->NM == 6) + } else if (upper->MDTBA == 0x00) { + if (content->ICCRIBA < 0x31 && content->FMSBA < 0x30) { + if (content->MSL == 0 && content->ISL <= 17) + return CHIPSET_BAYTRAIL; + if (content->MSL <= 1 && content->ISL <= 18) + return CHIPSET_6_SERIES_COUGAR_POINT; + if (content->MSL <= 1 && content->ISL <= 21) + return CHIPSET_8_SERIES_LYNX_POINT; + warn_peculiar_desc(true, "Wildcat Point"); + return CHIPSET_9_SERIES_WILDCAT_POINT; + } + if (content->NM == 6) { + warn_peculiar_desc(content->ICCRIBA > 0x34, "C620 series"); return CHIPSET_C620_SERIES_LEWISBURG; + } + warn_peculiar_desc(content->ICCRIBA != 0x31, "100 series"); return CHIPSET_100_SERIES_SUNRISE_POINT; - } else if (content->ICCRIBA == 0x34) { - if (content->NM == 6) - return CHIPSET_C620_SERIES_LEWISBURG; - return CHIPSET_300_SERIES_CANNON_POINT; } else { - msg_pwarn("Unknown flash descriptor, assuming 300 series compatibility.\n"); + if (content->ICCRIBA != 0x34) + msg_pwarn("Unknown flash descriptor, assuming 300 series compatibility.\n"); return CHIPSET_300_SERIES_CANNON_POINT; } } @@ -972,9 +974,10 @@ * tinction because of the dropped number of regions field (NR). */ static enum ich_chipset guess_ich_chipset(const struct ich_desc_content *const content, - const struct ich_desc_component *const component) + const struct ich_desc_component *const component, + const struct ich_desc_upper_map *const upper) { - const enum ich_chipset guess = guess_ich_chipset_from_content(content); + const enum ich_chipset guess = guess_ich_chipset_from_content(content, upper); switch (guess) { case CHIPSET_300_SERIES_CANNON_POINT: @@ -1055,7 +1058,7 @@ } if (*cs == CHIPSET_ICH_UNKNOWN) { - *cs = guess_ich_chipset(&desc->content, &desc->component); + *cs = guess_ich_chipset(&desc->content, &desc->component, &desc->upper); prettyprint_ich_chipset(*cs); } diff --git a/ich_descriptors.h b/ich_descriptors.h index 7683ed0..d00af63 100644 --- a/ich_descriptors.h +++ b/ich_descriptors.h @@ -521,7 +521,8 @@ struct { uint32_t VTBA :8, /* ME VSCC Table Base Address */ VTL :8, /* ME VSCC Table Length */ - :16; + :8, + MDTBA :8; /* MIP Descr. Table Base Addr. (new since Cannon Point/300) */ }; }; struct { -- To view, visit https://review.coreboot.org/c/flashrom/+/55647 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I587ad1abe390843d4a9e74431b6fc4b63f8ba512 Gerrit-Change-Number: 55647 Gerrit-PatchSet: 6 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit