Patch Set 1:

Patch Set 1:

Patch Set 1:

Patch Set 1:

What does the patch do? The description only says that it's required for various ChromeOS devices and I would like to understand the background.

This patch alone doesn't quite paint the full picture, I suspect others will be added to the chain.

The idea came about when ECs started appearing on different buses on different platforms - Some had an EC connected via LPC, some via I2C, and some on SPI. The problem was that `-p internal:bus=<lpc|i2c|spi>` became ambiguous and very cumbersome to use when trying to target ECs. Probing an EC could interfere with system operation so we wanted to bail out immediately when the probe function was called if we didn't actually intend to target an EC.

Similarly, this approach helped when host firmware ROMs started using linux_spi and linux_mtd programmer interfaces.

In both cases, this helps eliminate ambiguity (and potentially disruptive behavior) when flashrom is probing and obviates the need for every program and script that calls flashrom to have platform-specific logic to map component (host firmware ROM or EC) to programmer argument.

Thanks David for the enriched background! That is super helpful as I could only surmise a limited understanding just by digging alone. As Carl I suppose is getting at - can we get away with not needing to upstream it or do you see this is something we definitely envision should to be part of the overall convergence effort? The answer now seems to be yes however I thought I would ask explicitly as some of this stuff is unclear if it should be rewritten or ported near verbatim or just dropped without knowing some context lost to history.

Sure thing! Personally I think the idea is useful upstream just to help users avoid certain EC pitfalls since many of our users are using laptops these days. It might also be nice to have `-p usb` or `-p serial` for external programmers as a convenient shorthand, though they might require some additional argument parsing for things like setting voltage correctly. I'm sure any addition to upstream syntax will be met with a fair bit of resistance/bikeshedding, though maybe it's not such a bad thing if the premise is at least accepted...

For better or worse I suspect it's already in several of places in the ChromeOS codebase including the `vpd` tool as Wim pointed out, factory scripts, update scripts, and test suites [2].

For the benefit of others in this thread, here is the original and a follow-up patch that uses it:
[1] https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/+/58013
[2] https://chromium-review.googlesource.com/c/chromiumos/third_party/autotest/+/226951

Thank you all so much for the explanation! The motivation behind the patch is now clear.

As you noticed, the programmer parameter bus=<whatever> is not fit for the purpose. Admittedly, that parameter was never designed to handle an EC vs. host selection, but for some cases, it was possible to use it for such a selection. The dummy programmer probably has the usage matching the purpose of bus= best.

Just to make sure I understand you correctly: You want to select which onboard flash chip is accessed by flashrom, and there may be multiple flash chips hanging off different masters.
Do the following scenarios look complete to you?
1. Flash chip attached directly to the southbridge (Parallel, LPC, FWH, SPI) and we handle all hardware access inside flashrom.
2. Like 1, but the master accesses are abstracted by using Linux spidev.
3. Like 2, but the chip accesses are abstracted as well by using Linux MTD.
4. Like 1, but sometimes a dual chip setup is used and we may have to set which chip is active (e.g. in Dual BIOS setups), and sometimes a dual chip setup is only accessible as some opaque combined chip.
5. Flash chip attached to some master (Super I/O, EC, whatever) which performs implicit translation for all read accesses from the chipset, but writing and other commands have to be done by talking to the master directly and we handle all hardware access inside flashrom.
6. Like 5, but the implicit translation only covers part of the flash chip and all accesses from flashrom have to be done by talking to the master EC directly.
7. Like 6, but there is no implicit translation.

Scenario 1 is the classic internal case for flashrom.
Scenario 2 is covered by linux_spi.
Scenario 3 is covered by linux_mtd.
Scenario 4 is covered by adding a programmer parameter to internal.
Scenario 5 (it87) was added pretty early to flashrom after we had covered the classic case and is enabled implicity with internal.
Scenario 6 is similar to current Intel southbridges with IFD, except that with Intel IFD there's no translation and we're prevented from accessing the whole chip in most cases.
Scenario 7 without any chipset requirements is the easiest scenario because it is just a separate programmer (see satamv or others).

The scenarios above are made more complicated by the fact that some ECs require chipset setup to be accessible, and some ECs may really dislike the probe sequence for another EC. There may be combinations of 5+7 or 6+7 for two different flash chips hanging off the same master, but I have no idea if those really exist. Combinations of 7+7 exist (see nicintel_spi and nicintel_eeprom), and we have one driver for each part of the combination.

So overall, AFAICS the desire is to be able to specify "probe just the masters which may have access to the X" where X is "firmware for the main CPU", "VPD", or something else. You also do not want to have detailed hardware knowledge in the scripts calling flashrom, but rather inside flashrom.

Please help me correct any misunderstandings above. Maybe we can come up with a clean design which satisfies all of your needs.

View Change

To view, visit change 38671. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I73766451cd8900a9e0fe08efc7a4d81b2a35ac8d
Gerrit-Change-Number: 38671
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec@chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com>
Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h@gmx.de>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer@coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org>
Gerrit-CC: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
Gerrit-CC: Wim Vervoorn <wvervoorn@eltan.com>
Gerrit-Comment-Date: Wed, 05 Feb 2020 09:19:15 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment