Attention is currently required from: Nico Huber, Thomas Heijligen, Edward O'Callaghan, Nikolai Artemiev, Peter Marheine. Anastasia Klimchuk 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 2:
(6 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/64488/comment/6195146d_bdb43c4b PS2, Line 20: TEST A comment for myself: check with the users of "variable size" feature to make sure their stuff does not break (that's another team).
File dummyflasher.c:
https://review.coreboot.org/c/flashrom/+/64488/comment/a800a727_cdc59637 PS1, Line 129: return 0;
Seems unnecessary. […]
I removed. Although I understand this is relevant to the conversation about spi vs opaque master, so may gets changed depending on what we decide. I will return back if needed.
https://review.coreboot.org/c/flashrom/+/64488/comment/82ead7e9_82140c88 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 […]
Oh yes I removed the comment. I should have done from the very beginning... I discovered that for spi infra flashctx is const and the only way the feature works is because `probe_variable_size` is linked at the very high level, directly into the chip. I did few attempts to wire the feature via spi infra, but I couldn't because spi assumes flashctx is const.
https://review.coreboot.org/c/flashrom/+/64488/comment/8f591e84_129c56d5 PS1, Line 153: for (i = 0; i < NUM_ERASEFUNCTIONS; i++) {
There is only one by definition (opaque entry in flashchips.c).
Thanks, I removed the loop, so now it updates that one block eraser.
https://review.coreboot.org/c/flashrom/+/64488/comment/47d853ff_55432b2d 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 […]
I added dummy_opaque_read(), dummy_opaque_write(), dummy_opaque_erase() into the struct. However they call the same functions: default_spi_read, dummy_spi_write_256, spi_block_erase_c7. The reason is that I wanted to keep the behaviour.
But this also depends on spi vs opaque conversation.
https://review.coreboot.org/c/flashrom/+/64488/comment/8a642a7e_23b28025 PS1, Line 1269: rmst.spi = *s_mst; : rmst.opaque = *o_mst;
It's not supposed to work like this.
Yes I admit this is a hack! :) I thought this is an improvement from current situation because it fixes the build with CONFIG_DUMMY=no, and also now hack is contained in dummy and not splashed around the code.
I continue thinking if there is better solution, but I decided to send this patch so that we can discuss.
I also wanted to explain why this double master is needed.
First my attempt was to wire the feature as a spi master, but I couldn't make it compile: because spi infra expects flashctx to be const.
Then I started trying to wire as opaque master, as only opaque (not double). Probing was working nicely, but read/write did not work. The problem was that functions in spi.c and spi25.c they all expect `flash->mst->spi` to be present. And I wanted to call all the same functions for read / write (the same as now). I mean right now read/write will go to dummy_spi_send_command, and I didn't want to re-write it.
This is how I got to double master. It is really an opaque master, but also it has `flash->mst->spi` available. And as the end result, dummy is using opaque chip and spi send commands.
So... do you think it's too bad? :) I don't have another ideas at the moment :\
I can see this needs to be wired as opaque master, that's for sure. But also I don't want to re-write existing dummy_spi_send_command...