Attention is currently required from: Felix Singer, Thomas Heijligen, Jean THOMAS, Alexander Goncharov.

View Change

5 comments:

• File dirtyjtag_spi.c:

  • Patch Set #5, Line 18: * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA

    Please remove this paragraph. The address became outdated in the past so
    we decided to drop it.

  • Patch Set #5, Line 121: size_t num_xfer = (len + max_xfer_size - 1 ) / max_xfer_size; // ceil(len/max_xfer_size)

    So this looks like your implementation supports arbitrary `writecnt` and
    `readcnt`. Given that flashrom can limit these based on `.max_data_write`
    and `.max_data_read`, the implementation could look much simpler. But doesn't
    have to; I think there are two options:

    1. Lift the `.max_data_*` limits so this function gets called for bigger
    chunks. There should be less overhead as we wouldn't have to send the
    TMS/STOP sequence as often. I assume this could speed up long reads.
    Some of the memcpy() also seems unnecessary, but the USB transfers
    probably limit speed so much that it doesn't matter.

    2. Set `.max_data_*` such that we can expect `len <= 30` here¹. Then we
    wouldn't need the loop and malloc(). In theory, even a single 32B buffer
    would suffice.

    I assume both could speed things up as currently we probably have a lot
    of send,receive,send,receive,send sequences with little data in the second
    send/receive. (Btw. is it necessary to drain input buffers with the receive
    call even if we don't need the data?)

    ¹As the `max_xfer_size` accounts for both write and read counts, this is
    a little tricky: we'd have to account for the SPI25 protocol overhead.
    For the expected SPI commands, `25` should be the right limit (30B minus
    1B opcode and 4B addresses).

  • Patch Set #5, Line 174:

    	.max_data_read	= 30,
    	.max_data_write	= 30,
    

    This tells flashrom to limit `writecnt` and `readcnt`. It might not be
    necessary with the current implementation, see comment above.

  • Patch Set #5, Line 267: }

    This could probably be written much simpler if we'd focus on the strcmp.
    For instance, something like
    ```
    if (units[0] == '\0' || !strcasecmp(units, "Hz")) {
        freq /= 1000;
    } else if (!strcasecmp(units, "kHz")) {
        /* nothing to do as we want kHz */
    } else if (!strcasecmp(units, "MHz")) {
        freq *= 1000;
    } else {
        ...
        return 1;
    }
    ```

  • Patch Set #5, Line 275: except TRST

    What about SRST and TMS? I might read the code wrong, but it looks like
    there's actually more set to high than to low?

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