Attention is currently required from: Felix Singer, Nico Huber, Michał Żygowski, Paul Menzel, Michael Niewöhner.
Hello Felix Singer, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/55715
to look at the new patch set (#9).
Change subject: ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops
......................................................................
ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops
Add a new programmer supporting ITE Embedded Controllers found on
TUXEDO laptops. Tested on TUXEDO InfinityBook S 14 Gen6 and 15 Gen6
with EC firmware updates from IBV, EC versions 1.07.02TR1 and
1.07.04TR4.
Example standard command to update EC firmware on a TUXEDO laptop:
"flashrom -p ite_ec:romsize=128K,autoload=disable -w eL14MU02.TR1"
Signed-off-by: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Signed-off-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Change-Id: I0e42260155ffea38a6f60790871cd8da7b657031
---
M Makefile
M flashrom.8.tmpl
A ite_ec.c
A ite_ec.h
M programmer.h
M programmer_table.c
6 files changed, 1,053 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/15/55715/9
--
To view, visit https://review.coreboot.org/c/flashrom/+/55715
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I0e42260155ffea38a6f60790871cd8da7b657031
Gerrit-Change-Number: 55715
Gerrit-PatchSet: 9
Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.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: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: Felix Singer, Nico Huber, Michał Żygowski, Paul Menzel, Michael Niewöhner.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 )
Change subject: ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops
......................................................................
Patch Set 8:
(1 comment)
Patchset:
PS4:
> Maybe they are unique. However I have this feeling that DMI checks are more reliable. […]
We could use both DMI and SVID/SDID checks:
- DMI good, SVID/SDID good ---> [1]
- DMI good, SVID/SDID bad ---> [2]
- DMI bad, SVID/SDID good ---> [3]
- DMI bad, SVID/SDID bad ---> [4]
[1]: full match, proceed as usual.
[2]: unexpected (DMI is more specific than SVID/SDID), abort unless forced
[3]: mention that things could work, but abort unless forced
[4]: unknown machine, abort unless forced
Running into [2] would be very unusual. I'd explain the situation (DMI strings match, but SVID/SDID do not; could be a boot firmware bug or an unsupported system) and warn against continuing unless one is certain that the mismatched SVID/SDID is spurious (not something the mailing list would know).
For [3], I'd make flashrom say:
- What happened: SVID/SDID match, but DMI strings do not
- What the above implies: the machine might just be a rebranded system
- What to do: if one can risk a brick, try forcing and see what happens. if it works, then add the DMI strings to flashrom and submit a patch
In both cases, log the known DMI strings and SVID/SDID values (print both expected and actual SVID/SDID in [2]).
Thoughts?
--
To view, visit https://review.coreboot.org/c/flashrom/+/55715
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I0e42260155ffea38a6f60790871cd8da7b657031
Gerrit-Change-Number: 55715
Gerrit-PatchSet: 8
Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.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: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Comment-Date: Fri, 25 Jun 2021 10:23:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Thomas Heijligen, Anastasia Klimchuk.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55295 )
Change subject: tests: Do not run a test if its driver is not built
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/55295/comment/9b7b4150_8dffdd9c
PS5, Line 10: however config options can be disabled and in that case test should
: not be run.
> I'd even say it's better to return back to this later. Why switch […]
Agreed. This doesn't need to be resolved to the logical end point right now. The currently solution is perfectly reasonable for the number of tests we currently have.
Let's just resolve this and not be distracted by future problems here.
I totally agree with Nico's assessment about the weak symbol situation in coreboot!!! Can't stress that one enough and thanks for contrasting that here as it's a excellent case study why weak mostly sucks as well in the general case.
My only claim here is that, while I would think weak symbols would be a extremely exceptional to use, I suggest it is perhaps valid in this case. Only for the purposes of a test framework like cmocka that is designed around the weak symbol concept and that is to say it fits the paradigm and problem space. It is very unfortunately we do not have the features of Ada or Rust's traits at our disposal in C however if we did go the route of a clutter of `if (CONFIG(MEC1308))` I would prefer that it be all isolated into one object file in a very narrowly scoped way. Thanks C.
--
To view, visit https://review.coreboot.org/c/flashrom/+/55295
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ic1c48e41f658045a608f46636071f478ba646f77
Gerrit-Change-Number: 55295
Gerrit-PatchSet: 5
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
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: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Fri, 25 Jun 2021 10:01:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
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
Attention is currently required from: Nico Huber, Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55295 )
Change subject: tests: Do not run a test if its driver is not built
......................................................................
Patch Set 5:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/55295/comment/d99e4585_b1bd3ef7
PS5, Line 10: however config options can be disabled and in that case test should
: not be run.
> I'd even say it's better to return back to this later. Why switch
> to another scheme before we have seen how this one turns out? I
> grasp from the name `init_shutdown.c` that the idea was to have
> individual files per tested path. Once we try to add another file,
> we'll probably much easier see what the advantages and disadvan-
> tages are.
>
> As stated earlier, missing dependencies might be a reason to favor
> per programmer files. For instance if we need some library's
> header file to compile the test code. But that's a problem that
> I'd try to solve only once it's clear to exist :)
I fully agree. Predicting all potential advantages and disadvantages of some approach is hard (at least for me), and I find it easier to wait until the issues manifest themselves.
> As for weak functions. I'd rather clutter the code with
>
> if (CONFIG(MEC1308))
>
> than with weak declarations. In the only project I know that makes
> heavily use of weak symbols (coreboot) they often turn bad.
I concur. Most uses of weak symbols in coreboot cause more trouble than benefits, and this is why I try to get rid of weak symbols where possible.
For instance, CB:55812 could have added a weak declaration for the `mb_get_lpddr3_dq_dqs_map()` function instead of a new Kconfig option. However, mainboards with LPDDR3 *must* provide the DQ/DQS map, or else raminit will always fail. If one forgets to define the `mb_get_lpddr3_dq_dqs_map()` function, the current Kconfig approach will result in a build error (linker complains about undefined reference). Using a weak symbol would result in a runtime failure, and finding out what went wrong wouldn't be trivial (raminit is done by the MRC.bin blob, which doesn't log nor return useful error information).
--
To view, visit https://review.coreboot.org/c/flashrom/+/55295
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ic1c48e41f658045a608f46636071f478ba646f77
Gerrit-Change-Number: 55295
Gerrit-PatchSet: 5
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
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: Thomas Heijligen <src(a)posteo.de>
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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Fri, 25 Jun 2021 09:56:16 +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
Attention is currently required from: Michał Żygowski, Angel Pons.
Hello build bot (Jenkins), Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/55713
to look at the new patch set (#3).
Change subject: flashrom.c: Add 64KiB write granularity
......................................................................
flashrom.c: Add 64KiB write granularity
Add 64KiB write granularity for the incoming ITE Embedded Controllers
found on Tuxedo laptops. These ECs can only operate on 64KiB blocks and
any different operations result in a failure.
Signed-off-by: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Change-Id: I69801f581d0cb9b85f6596de7e1267ef9317673f
---
M flash.h
M flashrom.c
2 files changed, 7 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/13/55713/3
--
To view, visit https://review.coreboot.org/c/flashrom/+/55713
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I69801f581d0cb9b85f6596de7e1267ef9317673f
Gerrit-Change-Number: 55713
Gerrit-PatchSet: 3
Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Felix Singer, Nico Huber, Paul Menzel, Angel Pons, Michael Niewöhner.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 )
Change subject: ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops
......................................................................
Patch Set 7:
(1 comment)
Patchset:
PS4:
> > After a second thought, I think not all laptops with ITE ECs may have a distinguishable SVID/SDID […]
Maybe they are unique. However I have this feeling that DMI checks are more reliable. Do we know whether all vendors set PCI SVID/SDID?
--
To view, visit https://review.coreboot.org/c/flashrom/+/55715
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I0e42260155ffea38a6f60790871cd8da7b657031
Gerrit-Change-Number: 55715
Gerrit-PatchSet: 7
Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.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: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Comment-Date: Fri, 25 Jun 2021 09:51:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55295 )
Change subject: tests: Do not run a test if its driver is not built
......................................................................
Patch Set 5:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/55295/comment/7eb1f553_18538c5d
PS5, Line 10: however config options can be disabled and in that case test should
: not be run.
> >>>What was discussed exactly? […]
I'd even say it's better to return back to this later. Why switch
to another scheme before we have seen how this one turns out? I
grasp from the name `init_shutdown.c` that the idea was to have
individual files per tested path. Once we try to add another file,
we'll probably much easier see what the advantages and disadvan-
tages are.
As stated earlier, missing dependencies might be a reason to favor
per programmer files. For instance if we need some library's
header file to compile the test code. But that's a problem that
I'd try to solve only once it's clear to exist :)
As for weak functions. I'd rather clutter the code with
if (CONFIG(MEC1308))
than with weak declarations. In the only project I know that makes
heavily use of weak symbols (coreboot) they often turn bad.
--
To view, visit https://review.coreboot.org/c/flashrom/+/55295
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ic1c48e41f658045a608f46636071f478ba646f77
Gerrit-Change-Number: 55295
Gerrit-PatchSet: 5
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
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: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Fri, 25 Jun 2021 09:00:09 +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
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/+/55741 )
Change subject: hwaccess_x86_io_unittest: Add dummy iopl to avoid including sys/io.h
......................................................................
Patch Set 6:
(1 comment)
Patchset:
PS5:
> It is working!! No more sys/io. […]
This is why I suggested trying to run the test on ARM. If we're mocking the arch-specific stuff, we don't need system support 😉
--
To view, visit https://review.coreboot.org/c/flashrom/+/55741
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I3f2f0408be7c00f954b899031b52b2b97ef19ca3
Gerrit-Change-Number: 55741
Gerrit-PatchSet: 6
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: Fri, 25 Jun 2021 07:59:17 +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