Attention is currently required from: Felix Singer, Michał Żygowski, Paul Menzel, Sergii Dmytruk.
12 comments:
Patchset:
The internal DMI decoder checks the legacy region for SMBIOS entry. […]
I was wondering for some time why the external DMI option is not the default.
Maybe it's time to change that. However, this seems like an independent
problem. Thanks for the clarification.
You mean compile the files when CONFIG_INTERNAL is selected? Michael also wanted to add more interfa […]
I mean it should be fully included in `-p internal` eventually. The `internal`
programmer is meant for flash chips on the system mainboard. All the infra-
structure is there, and I hope it wouldn't be hard to hook an EC driver properly
up. Basically a board-enable entry would call the init function of this driver,
and that would register another master. Later logic could decide if this master
is to be used, or we could filter the call beforehand, e.g. something like
`-p internal:master=ec`.
To make use of the board-enable infrastructure for other than the `internal`
programmers would need a lot of refactoring I guess. For now, I would rather
keep the current design. Unless there is something I miss, i.e. any reason
not to make EC drivers part of `-p internal`?
As I said, I'd be ok with merging it as a separate programmer. But we'd have
to expect that it will change in the future (i.e. I would welcome patches that
make it part of `-p internal`).
File flashrom.8.tmpl:
Patch Set #29, Line 1423: .B " flashrom \-p ite_ecfw:portpair=pairnum"
We can add a field that would specify the default port pair for given board. […]
Yes, I would much prefer default values in the board list.
Patch Set #29, Line 1471: .B " flashrom \-p ite_ecfw:boardmismatch=force"
There is a System76 EC firmware which serves as a replacement for the original firmware being used h […]
AIUI (hopefully I'm not wrong) the original boardmismatch option is only useful
in combination with coreboot and the image checks.
To be able to force a board-enable entry there is this syntax:
```
flashrom -p internal:mainboard=<vendor>:<board>
```
This has the advantage that one can force board-specific code or default, i.e.
could be useful in combination with defaults for port pairs like mentioned
above. IOW, if we add per-board defaults, we'd want something like this too.
File it87spi.c:
Patch Set #29, Line 105: case 0x89:
We can split it, however I have no means to answer your question (no hardware with ITE of ID startin […]
Yes, please split. I'd want to carefully read the related code. It's a bit
annoying (definitely not your fault) that the internal programmer always
calls this code, no matter what board was detected.
Your call is actually safer ;)
File ite_ecfw.c:
We replay the behaviour of the original EFI file doing the update. […]
AFAICS, if we'd allow a partial erase, flashrom could decide to only write
the first block. Then this function would never be called for the last
block and the first kilobyte wouldn't get written?
I know it's only a theoretical problem as long as we erase the whole chip.
Also, when the end of the image is empty, i.e. 0xff, same problem could
occure, hmmm. This wouldn't be fun to test /o\ That's also rather
theoretical, right? I guess these chips are always fully stuffed with
data?
ctx_data->ite_string_offset >= start &&
ctx_data->ite_string_offset <= (start + len - sizeof(struct ite_string))
The code should skip the autoload patching if offset is 0, I have to fix it. […]
I rather meant what if the string offset is within start+len but start is
not 0. I would expect ite_ecfw_patch_autoload() to patch the wrong location
(`ite_string_offset` instead of `ite_string_offset - start`?).
But let's discuss the autoload feature further first.
Patch Set #29, Line 495: (uint8_t *)
The autoload option breaks the verification indeed. […]
To be honest I don't see such modifications in the flashrom tool. Wouldn't mind
it in another tool in the project, of course.
It's kind of an unwritten rule (well, unwritten beside the `const`). Flashrom
should just sync file contents with flash contents (maybe I'm too much a fan
of the Unix phylosophy). I would feel very uncomfortable to change that rule
on my own, without a prior discussion on the mailing list where everybody would
have a chance to raise their concerns. Also, removing the `const` from the API
would make it less flexible. Currently the patching works by coincidence but
a refactoring of the generic code could break that (unless we make it officially
non-const constrain refactorings).
I would like to propose an alternative: Leave it to the usual content sync'ing
in `flashrom`. But provide another tool that uses libflashrom and takes care
of everything, including the project check and the autoload patching. I would
be happy to help with the new CLI code if that is an issue.
Probably we can set the erase size to 256 bytes or 1024 bytes, this needs testing. […]
I was more wondering about erasing 4 * 64KiB individually instead of 256KiB
at once (what the code tells flashrom to do). Multiples of 64KiB is what the
first if in ite_ecfw_erase() enforces. But we send individual commands per
chunk indeed. Somewhat odd. Let's leave it for now.
It looks like RDID, but the EC doesn't return reliable data. […]
Does it show something useful yet?
Patch Set #29, Line 812: Probing
No longer true. If the board is not on the list of supported boards (matched by PIC and PCI subsystem of the host bridge), the code won't get to writing to EC RAM, unless we force it.
What I mean is that the message says we would force "probing", but it's more
a force using this driver. Not important atm.
Patch Set #29, Line 905: int ite_ecfw_verify_file_project(uint8_t *const newcontents,
Currently not, but I wish it would be at some point. […]
We could probably find a place for a hook. Could that also be used for the
autoload patching? But again, it might fit a separate tool much better.
To view, visit change 55715. To unsubscribe, or for help writing mail filters, visit settings.