- flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: tests: Do not run a test if its driver is not built
by Nico Huber (Code Review) 22 Jun '21

22 Jun '21

Attention is currently required from: 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 4: (1 comment) File tests/init_shutdown.c: https://review.coreboot.org/c/flashrom/+/55295/comment/d260fae4_d5cf0e93 PS1, Line 35: #if CONFIG_DUMMY == 1 : void dummy_init_and_shutdown_test_success(void **state) : { : run_lifecycle(state, PROGRAMMER_DUMMY, "bus=parallel+lpc+fwh+spi"); : } : #endif /* CONFIG_DUMMY */ > "yes"/"no" is only used on the user end. Both Meson and the Makefile always use > `=1` for the `-D` arguments. However, what is rather unfortunate, they only ever > provide `1` and never `0`. So the only way to use it is useful is indeed with #if. > :-/ I forgot that we had the same problem with Kconfig in coreboot. It was solved long ago by a complex macro. It's quite nice, so we should add that to flashrom at some point: /* * Getting something that works in C and CPP for an arg that may or may * not be defined is tricky. Here, if we have "#define CONFIG_BOOGER 1" * we match on the placeholder define, insert the "0," for arg1 and generate * the triplet (0, 1, 0). Then the last step cherry picks the 2nd arg (a one). * When CONFIG_BOOGER is not defined, we generate a (... 1, 0) pair, and when * the last step cherry picks the 2nd arg, we get a zero. */ #define __ARG_PLACEHOLDER_1 0, #define config_enabled(cfg) _config_enabled(cfg) #define _config_enabled(value) __config_enabled(__ARG_PLACEHOLDER_##value) #define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 1, 0, 0) #define ___config_enabled(__ignored, val, ...) val #define CONFIG(option) config_enabled(CONFIG_##option) How does it work? well, that's a riddle even with the comment ^^ (hint: the comment is not accounting for the last `, 0` which is only necessary to be C compatible where one always needs at least one variadic argument). How is it used? CONFIG(DUMMY) would expand to 1 iff CONFIG_DUMMY == 1, otherwise (even if CONFIG_DUMMY is not defined) it expands to 0. -- 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: 4 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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 22 Jun 2021 18:34:10 +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]: tuxedo_ec: Implement support for flashing ECs on TUXEDO laptops
by Paul Menzel (Code Review) 22 Jun '21

22 Jun '21

Attention is currently required from: Felix Singer, Nico Huber, Michał Żygowski, Angel Pons. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 ) Change subject: tuxedo_ec: Implement support for flashing ECs on TUXEDO laptops ...................................................................... Patch Set 4: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/55715/comment/078826ba_80b80374 PS4, Line 11: updates from IBV. Can you please document the EC firmware version used, and give an example command? Patchset: PS4: That’s great work. Thank you. -- 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: 4 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 22 Jun 2021 18:34:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tuxedo_ec: Implement support for flashing ECs on TUXEDO laptops
by Michael Niewöhner (Code Review) 22 Jun '21

22 Jun '21

Attention is currently required from: Felix Singer, Nico Huber, Michał Żygowski, Angel Pons. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 ) Change subject: tuxedo_ec: Implement support for flashing ECs on TUXEDO laptops ...................................................................... Patch Set 4: (1 comment) Patchset: PS4: > > Ah, it's already there, what I meant: There is that project id (tuxedo_ec_read_project), the firmware version and the chip ID. Having the projectid + chip id together as identifier sounds good to me. What do you think? > > How do you get any of it without first making assumptions about > register layouts? > > > > > or better the firmware id (e.g. the "ITE string" or sth else) because support for this firmware-based programming mechanism is implemented in EC fw > > I'm still wondering where would you get that string from? Forget the string. What I meant was the project id ;-) > > > > > > That's right, we would not only have to detect the chip without probing > > > any non-standard ports, we'd also have to figure out what firmware it > > > runs. How would one get this string without making assumptions that can > > > be broken (e.g. by not using ITEs firmware framework)? > > > > The point is, that you can't flash at all with a broken/incompatible firmware, using that code here. So it doesn't help to know the mainboard IMO > > Knowing the mainboard avoids running the code on the wrong board > where it's more likely to do harm. It's not about the flashing process, > flash contents or the contents to be flashed. The code starts with > `ec_write_reg(0xf9, 0x20, EC_MAX_STATUS_CHECKS)`. There may be > EC firmware that reacts badly to such a write (or chips that com- > pletely misunderstand what is written to their registers). Knowing > the board it runs on, makes it much less likely to hit such a case. Ah, thank you. I got you point. Well, that's indeed a very valid argument. > > > However, I'd like to have some safety checks, too, and maybe a warning but still would like to have some possibility to override that when I really know what I am doing or have ways to recover, > > I'm pretty sure that people hacking on EC firmware can add a DMI > string. > > > like when overriding a test-firmware with a productive one. > > That wouldn't be about the DMI check but the one that currently > compares flash & file contents. Something that should be discussed > separately, imho. Agreed -- 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: 4 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-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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 22 Jun 2021 17: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: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tuxedo_ec: Implement support for flashing ECs on TUXEDO laptops
by Nico Huber (Code Review) 22 Jun '21

22 Jun '21

Attention is currently required from: Felix Singer, Michał Żygowski, Angel Pons, Michael Niewöhner. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 ) Change subject: tuxedo_ec: Implement support for flashing ECs on TUXEDO laptops ...................................................................... Patch Set 4: (1 comment) Patchset: PS4: > Ah, it's already there, what I meant: There is that project id (tuxedo_ec_read_project), the firmware version and the chip ID. Having the projectid + chip id together as identifier sounds good to me. What do you think? How do you get any of it without first making assumptions about register layouts? > > > or better the firmware id (e.g. the "ITE string" or sth else) because support for this firmware-based programming mechanism is implemented in EC fw I'm still wondering where would you get that string from? > > > That's right, we would not only have to detect the chip without probing > > any non-standard ports, we'd also have to figure out what firmware it > > runs. How would one get this string without making assumptions that can > > be broken (e.g. by not using ITEs firmware framework)? > > The point is, that you can't flash at all with a broken/incompatible firmware, using that code here. So it doesn't help to know the mainboard IMO Knowing the mainboard avoids running the code on the wrong board where it's more likely to do harm. It's not about the flashing process, flash contents or the contents to be flashed. The code starts with `ec_write_reg(0xf9, 0x20, EC_MAX_STATUS_CHECKS)`. There may be EC firmware that reacts badly to such a write (or chips that com- pletely misunderstand what is written to their registers). Knowing the board it runs on, makes it much less likely to hit such a case. > However, I'd like to have some safety checks, too, and maybe a warning but still would like to have some possibility to override that when I really know what I am doing or have ways to recover, I'm pretty sure that people hacking on EC firmware can add a DMI string. > like when overriding a test-firmware with a productive one. That wouldn't be about the DMI check but the one that currently compares flash & file contents. Something that should be discussed separately, imho. -- 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: 4 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-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Comment-Date: Tue, 22 Jun 2021 17:46:26 +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: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tuxedo_ec: Implement support for flashing ECs on TUXEDO laptops
by Michael Niewöhner (Code Review) 22 Jun '21

22 Jun '21

Attention is currently required from: Felix Singer, Nico Huber, Michał Żygowski, Angel Pons. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 ) Change subject: tuxedo_ec: Implement support for flashing ECs on TUXEDO laptops ...................................................................... Patch Set 4: (1 comment) Patchset: PS4: @Nico, sorry, I somehow skipped a whole paragraph when reading o.O >>or better the firmware id (e.g. the "ITE string" or sth else) because support for this firmware-based programming mechanism is implemented in EC fw > That's right, we would not only have to detect the chip without probing > any non-standard ports, we'd also have to figure out what firmware it > runs. How would one get this string without making assumptions that can > be broken (e.g. by not using ITEs firmware framework)? The point is, that you can't flash at all with a broken/incompatible firmware, using that code here. So it doesn't help to know the mainboard IMO -- 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: 4 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-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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 22 Jun 2021 17:30:24 +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: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tuxedo_ec: Implement support for flashing ECs on TUXEDO laptops
by Michael Niewöhner (Code Review) 22 Jun '21

22 Jun '21

Attention is currently required from: Felix Singer, Nico Huber, Michał Żygowski, Angel Pons. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 ) Change subject: tuxedo_ec: Implement support for flashing ECs on TUXEDO laptops ...................................................................... Patch Set 4: (1 comment) Patchset: PS4: > > > I think it's awesome that the DMI checks were there from the beginning. This […] Ah, it's already there, what I meant: There is that project id (tuxedo_ec_read_project), the firmware version and the chip ID. Having the projectid + chip id together as identifier sounds good to me. What do you think? -- 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: 4 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-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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 22 Jun 2021 17:23:05 +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: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tuxedo_ec: Implement support for flashing ECs on TUXEDO laptops
by Michael Niewöhner (Code Review) 22 Jun '21

22 Jun '21

Attention is currently required from: Felix Singer, Nico Huber, Michał Żygowski, Angel Pons. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 ) Change subject: tuxedo_ec: Implement support for flashing ECs on TUXEDO laptops ...................................................................... Patch Set 4: (1 comment) Patchset: PS4: > > I think it's awesome that the DMI checks were there from the beginning. This > > is the way to go. Great work! > > > > > retrieve the chip ID at indices 0x20/0x21 and find a match in a table of supported ECs. This could also be used to handle the ITE IT5570E differences. > > > > There is no standard for these registers, any chip could return the same > > as an ITE one. There is not even a guarantee that the 0x2e/0x2ef ports are > > used as a register/data pair. Probing such things is ok for a development > > tool like `inteltool`, who cares if the developer's machine resets/bricks? > > But flashrom is not a hacking tool, it's users are humble. There will be > > people who'll simply try the programmer. > > Good point. I forget about flashrom not being a hacking tool. While I agree that it's not a hacking tool, a) some people want to use it during development of alternative ec firmware. b) you still cannot save all people from bricking their devices. Even with parameters like --please-set-my-house-on-fire, there will be someone who runs it... However, I'd like to have some safety checks, too, and maybe a warning but still would like to have some possibility to override that when I really know what I am doing or have ways to recover, like when overriding a test-firmware with a productive one. > > > > or better the firmware id (e.g. the "ITE string" or sth else) because support for this firmware-based programming mechanism is implemented in EC fw > > > > That's right, we would not only have to detect the chip without probing > > any non-standard ports, we'd also have to figure out what firmware it > > runs. How would one get this string without making assumptions that can > > be broken (e.g. by not using ITEs firmware framework)? > > > > Maintaining a table of compatible boards is much likely the least work. > > Having said that, it might not be a bad idea to add additional checks > > after the board detection, i.e. when we know the board, we know that > > there is an ITE chips, can find the PMC pair, try to check for a com- > > patible firmware (currently the code starts with writes without > > knowing if the firmware is compatible; e.g. did somebody replace it > > with the System76 one?). I'm unsure if having a list of mainboards (like vendor/mainboard) is really required. I need to take another look but IIRC the EC fw framework has capabilities of identifying the firmware/mainboard combination. That should be sufficient. > > I agree. If a board isn't in the compatibility list, it can be added once it's known to work. > > > > So basically we can rename the programmer to something like ite_ec. Any other suggestions? > > > > I'd say `ite_ecfw.c`. After all it's not a driver for their chips but > > for their firmware framework?. > > +1 sounds good -- 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: 4 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-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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 22 Jun 2021 17:11:15 +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: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tuxedo_ec: Implement support for flashing ECs on TUXEDO laptops
by Angel Pons (Code Review) 22 Jun '21

22 Jun '21

Attention is currently required from: Felix Singer, Nico Huber, Michał Żygowski, Michael Niewöhner. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 ) Change subject: tuxedo_ec: Implement support for flashing ECs on TUXEDO laptops ...................................................................... Patch Set 4: (1 comment) Patchset: PS4: > I think it's awesome that the DMI checks were there from the beginning. This > is the way to go. Great work! > > > retrieve the chip ID at indices 0x20/0x21 and find a match in a table of supported ECs. This could also be used to handle the ITE IT5570E differences. > > There is no standard for these registers, any chip could return the same > as an ITE one. There is not even a guarantee that the 0x2e/0x2ef ports are > used as a register/data pair. Probing such things is ok for a development > tool like `inteltool`, who cares if the developer's machine resets/bricks? > But flashrom is not a hacking tool, it's users are humble. There will be > people who'll simply try the programmer. Good point. I forget about flashrom not being a hacking tool. > > or better the firmware id (e.g. the "ITE string" or sth else) because support for this firmware-based programming mechanism is implemented in EC fw > > That's right, we would not only have to detect the chip without probing > any non-standard ports, we'd also have to figure out what firmware it > runs. How would one get this string without making assumptions that can > be broken (e.g. by not using ITEs firmware framework)? > > Maintaining a table of compatible boards is much likely the least work. > Having said that, it might not be a bad idea to add additional checks > after the board detection, i.e. when we know the board, we know that > there is an ITE chips, can find the PMC pair, try to check for a com- > patible firmware (currently the code starts with writes without > knowing if the firmware is compatible; e.g. did somebody replace it > with the System76 one?). I agree. If a board isn't in the compatibility list, it can be added once it's known to work. > > So basically we can rename the programmer to something like ite_ec. Any other suggestions? > > I'd say `ite_ecfw.c`. After all it's not a driver for their chips but > for their firmware framework?. +1 -- 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: 4 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-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: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Comment-Date: Tue, 22 Jun 2021 16:47:17 +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: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tuxedo_ec: Implement support for flashing ECs on TUXEDO laptops
by Nico Huber (Code Review) 22 Jun '21

22 Jun '21

Attention is currently required from: Felix Singer, Michał Żygowski, Angel Pons, Michael Niewöhner. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 ) Change subject: tuxedo_ec: Implement support for flashing ECs on TUXEDO laptops ...................................................................... Patch Set 4: (2 comments) Patchset: PS4: I think it's awesome that the DMI checks were there from the beginning. This is the way to go. Great work! > retrieve the chip ID at indices 0x20/0x21 and find a match in a table of supported ECs. This could also be used to handle the ITE IT5570E differences. There is no standard for these registers, any chip could return the same as an ITE one. There is not even a guarantee that the 0x2e/0x2ef ports are used as a register/data pair. Probing such things is ok for a development tool like `inteltool`, who cares if the developer's machine resets/bricks? But flashrom is not a hacking tool, it's users are humble. There will be people who'll simply try the programmer. > or better the firmware id (e.g. the "ITE string" or sth else) because support for this firmware-based programming mechanism is implemented in EC fw That's right, we would not only have to detect the chip without probing any non-standard ports, we'd also have to figure out what firmware it runs. How would one get this string without making assumptions that can be broken (e.g. by not using ITEs firmware framework)? Maintaining a table of compatible boards is much likely the least work. Having said that, it might not be a bad idea to add additional checks after the board detection, i.e. when we know the board, we know that there is an ITE chips, can find the PMC pair, try to check for a com- patible firmware (currently the code starts with writes without knowing if the firmware is compatible; e.g. did somebody replace it with the System76 one?). > So basically we can rename the programmer to something like ite_ec. Any other suggestions? I'd say `ite_ecfw.c`. After all it's not a driver for their chips but for their firmware framework?. File flashrom.c: https://review.coreboot.org/c/flashrom/+/55715/comment/887b98de_7c92c5b1 PS1, Line 2109: #if CONFIG_TUXEDO_EC == 1 > I feel the same when I look at 40 lines back with internal programmer. […] The coreboot check above is a very bad rolemodel. It does things flashrom shouldn't do (look at the contents) and in a way (doing programmer-specific things in common code; a layering violation) that we try to get rid of. I think it would be best to call _verify_file_project() from programmer init. The UI would then have to feed the file path (or contents) two times. If we really want such a check, flashrom (or the library) shouldn't offer any operation if the contents look incompatible. Usually, we leave the decision if contents are valid to the user. Flashrom is supposed to be a tool that synchronizes flash contents with files, nothing more. However, I can understand the urge to check such things. If we decide to allow such checks and want to do them here, during writing, it should be a function pointer of the programmer. Weak functions can be nice sometimes but are generally highly volatile. Many bugs in coreboot, for instance, are introduced because of weak functions. Anyway, it wouldn't work here unless we would do one flashrom build per programmer xD NB. I think a lot of this could be avoided when people would write more specific CLIs. Using the all-in-one cli_classic might not be a good idea for EC flashing. -- 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: 4 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-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Comment-Date: Tue, 22 Jun 2021 16:35:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No 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

1 0

Change in flashrom[master]: tests: Do not run a test if its driver is not built
by Thomas Heijligen (Code Review) 22 Jun '21

22 Jun '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk. Thomas Heijligen 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 4: (1 comment) Patchset: PS4: cmocka offers a function TO skip a test. this would make clear, that nothing was executed. Currently a test doing nothing is successful. It should be skipped to make clear that nothing happend. E.g.: ``` #if CONFIG_LINUX_SPI == 1 run_lifecycle(state, &programmer_linux_spi, "dev=/dev/null"); #else skip(); #endif ``` -- 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: 4 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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 22 Jun 2021 13:55:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit