Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60074 )
Change subject: build: Remove cli_classic need for internal symbols [WIP]
......................................................................
Patch Set 7: -Code-Review
--
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: 7
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-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-Comment-Date: Tue, 19 Apr 2022 04:11:40 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Subrata Banik, Angel Pons, Anastasia Klimchuk.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63584 )
Change subject: ichspi: Add support for Ice Lake
......................................................................
Patch Set 1:
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/63584/comment/8b531d74_23c13e66
PS1, Line 7: ichspi: Add support for Ice Lake
> As discussed during review of the first of these patches, adding all […]
Not sure what you mean by "undone" as those add legitimate new chipset support, seems like hyperbole. I believe you mean to say there should be some refactors happening to help consolidate so many enum branches. As you pointed out Nico, you tried a number of times in the past and didn't manage to trivially do it.
In any case, I actually did upload a start towards making progress in that direction in CB:63321 but it has been sitting unreviewed for a few weeks at the moment. I believe the theme of that patch can be applied to a few other places towards a direction of having most of the chipset static details in a lookup table and a switch.
More careful review of SPI Programmer Guides between generations is required to find better common patterns.
File chipset_enable.c:
https://review.coreboot.org/c/flashrom/+/63584/comment/c687b8e2_19acc666
PS1, Line 998: if (strcmp(name, "Ice Lake") == 0)
: return enable_flash_pch100_or_c620(dev, name, 0x1f, 5, CHIPSET_400_SERIES_ICE_POINT);
: else
: return enable_flash_pch100_or_c620(dev, name, 0x1f, 5, CHIPSET_400_SERIES_COMET_POINT);
This feels wrong to have a strcmp(). I do not have a copy of IceLake/IcePoint SPI Programming Guide to figure out if there is any difference, if you have a copy Subrata can we look together and figure out a strategy?
If this patch is right to say there is no difference then can we rename `CHIPSET_400_SERIES_COMET_POINT` to `CHIPSET_400_SERIES_COMET_ICE_POINT` Maybe?
https://review.coreboot.org/c/flashrom/+/63584/comment/68caca04_44159b7d
PS1, Line 2149: 0x34a4
> But can it be hidden? Maybe not everybody have updated their code […]
Subrata, flashrom can be used in "odd" situations when folks are porting coreboot to boards in the community. I think this is what Nico is alluding to here as to make flashrom as robust as possible to allow for flashing.
Should DID's in the table used for detection of hidden situations perhaps have
" (hidden)" suffix and a more detailed code comment explaining so we can avoid confusion? Feel like this is it's own patch.
--
To view, visit https://review.coreboot.org/c/flashrom/+/63584
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If000b152c6e454617a7ad0245d16f8b1dea97fa0
Gerrit-Change-Number: 63584
Gerrit-PatchSet: 1
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-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-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Tue, 19 Apr 2022 01:26:48 +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>
Gerrit-MessageType: comment
Edward O'Callaghan has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/62311 )
Change subject: tests/realtek_mst: Allow test to check multiple paths
......................................................................
Abandoned
--
To view, visit https://review.coreboot.org/c/flashrom/+/62311
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I80d5e1fc26ad9caf49a98a7671d069bbd8428045
Gerrit-Change-Number: 62311
Gerrit-PatchSet: 8
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-MessageType: abandon
Attention is currently required from: Nico Huber.
Hello build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/63321
to look at the new patch set (#2).
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(+), 80 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/63321/2
--
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: 2
Gerrit-Owner: 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-MessageType: newpatchset
Attention is currently required from: Felix Singer, Nico Huber, Martin L Roth, Angel Pons, Nikolai Artemiev.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62768 )
Change subject: flashrom.8.tmpl: Add raiden_debug_spi doc entry
......................................................................
Patch Set 5:
(1 comment)
File flashrom.8.tmpl:
https://review.coreboot.org/c/flashrom/+/62768/comment/37195401_6fc8328e
PS3, Line 1166: false
> I agree, it is "buggy" in that it is inconsistent […]
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/62768
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I186920006bdfcc7a9f89542f84b452dfc72b18e4
Gerrit-Change-Number: 62768
Gerrit-PatchSet: 5
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: Martin L Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.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: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Martin L Roth <martinroth(a)google.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Mon, 18 Apr 2022 09:33:03 +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