Attention is currently required from: Nico Huber, Thomas Heijligen, Edward O'Callaghan, Nikolai Artemiev, Peter Marheine.

View Change

6 comments:

• Commit Message:

  • Patch Set #2, 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:

  • Patch Set #1, 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.

  • 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 […]

    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.

  • Patch Set #1, 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.

  • Patch Set #1, 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.

  • Patch Set #1, 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...

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