Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk, Peter Marheine.
9 comments:
Patchset:
If it works with the requirements, I think an opaque master is the way to go!
File dummyflasher.c:
Patch Set #1, Line 129: return 0;
Seems unnecessary. If it's not our master, why should we get called?
Patch Set #1, Line 133: * Once that happens, we need to have special hacks in functions:
For opaque masters this should never happen (they all have to fill the
size). Or if it ever happens, it would have to be part of an compatible
API change anyway.
Patch Set #1, Line 146: flash->chip->feature_bits = FEATURE_4BA_READ | FEATURE_4BA_WRITE;
These are SPI specific and wouldn't be needed if we don't mix opaque
and SPI functions. More about that below.
Patch Set #1, Line 153: for (i = 0; i < NUM_ERASEFUNCTIONS; i++) {
There is only one by definition (opaque entry in flashchips.c).
.read = default_spi_read,
.write = dummy_spi_write_256,
.erase = spi_block_erase_c7,
It might make reasoning about the code much simpler to not re-use
the SPI functions.
Unless there are specific requirements, all an opaque dummy should
need is memcpy() for read/write and memset() for erase. Wrapping these
into three functions (e.g. dummy_opaque_read(), dummy_opaque_write(),
dummy_opaque_erase()) would add little boilerplate but make it clear
what is supposed to happen.
Patch Set #1, Line 906: free(bustext);
Related to the very last comment below, we might want to update this.
AFAICS, emulation of masters and chips was kept separately so far.
Currently one can do odd things like this:
$ flashrom -p dummy:emulate=W25Q128FV,bus=parallel
...
No EEPROM/flash device found.
Seems stupid, but it's actually a test that the master infrastructure
works as expected :)
rmst.spi = *s_mst;
rmst.opaque = *o_mst;
It's not supposed to work like this. The .spi and .opaque fields were
originally part of a union, i.e. mutually exclusive. That's not visible
anymore because the code got a little obfuscated by CB:50246.
AIUI, register_master() is not supposed to be called by programmer drivers
directly. They should go through the specific register_*_master() functions.
I guess that your tests worked out shows that we actually don't need a SPI
master and register_opaque_master() should suffice.
If we need both masters, they can be registered individually.
Patch Set #1, Line 1346: } else {
It doesn't have to be mutually exclusive. We could just add all possible
masters (like done for NONSPI and SPI below). The probe function would be
responsible to check for `emu_chip == EMULATE_VARIABLE_SIZE`.
To view, visit change 64488. To unsubscribe, or for help writing mail filters, visit settings.