Change in flashrom[master]: [RFC] dummyflasher: Wire variable size feature via opaque infra
Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk, Peter Marheine. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64488 )
Change subject: [RFC] dummyflasher: Wire variable size feature via opaque infra ......................................................................
Patch Set 1:
(9 comments)
Patchset:
PS1: If it works with the requirements, I think an opaque master is the way to go!
File dummyflasher.c:
https://review.coreboot.org/c/flashrom/+/64488/comment/283d36d5_4d5f35bb PS1, Line 129: return 0; Seems unnecessary. If it's not our master, why should we get called?
https://review.coreboot.org/c/flashrom/+/64488/comment/1aeb6ade_2830ab2f PS1, 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.
https://review.coreboot.org/c/flashrom/+/64488/comment/72ad7e73_cf0a9114 PS1, 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.
https://review.coreboot.org/c/flashrom/+/64488/comment/a8fbbb67_ceca2014 PS1, Line 153: for (i = 0; i < NUM_ERASEFUNCTIONS; i++) { There is only one by definition (opaque entry in flashchips.c).
https://review.coreboot.org/c/flashrom/+/64488/comment/3d3c4644_901313d7 PS1, Line 865: .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.
https://review.coreboot.org/c/flashrom/+/64488/comment/68971faf_fbc6af8f PS1, 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 :)
https://review.coreboot.org/c/flashrom/+/64488/comment/67e28289_d38d3c1d PS1, Line 1269: 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.
https://review.coreboot.org/c/flashrom/+/64488/comment/7bca48fa_a5c1209d PS1, 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`.
-
Nico Huber (Code Review)