Daniel Thompson has posted comments on this change. ( https://review.coreboot.org/26946 )
Change subject: bitbang_spi: Add functions to optimize xfers ......................................................................
Patch Set 2:
(2 comments)
Nico: Not sure I understood your review comments but I've replied anyway.
Right now I don't plan to act (based on my estimation of the benefit) but if you really do want me to investigate what happens if we change the idle state of the clock then let me know.
https://review.coreboot.org/#/c/26946/2/bitbang_spi.c File bitbang_spi.c:
https://review.coreboot.org/#/c/26946/2/bitbang_spi.c@114 PS2, Line 114: bitbang_spi_set_sck_set_mosi(master, 0, 0);
Shouldn't we start with CLK = 1 now?
I don't really understand what you mean here.
I've not really changed how the clocking works with this patch. Other than the combining the clk/mosi/miso changes the only other change is to move bitbang_spi_set_clk() from the end of the byte loop to the beginning so it could be combined with the mosi (and adding in the final set_clk to ensure we idle the clock at the right polarity).
https://review.coreboot.org/#/c/26946/2/bitbang_spi.c@157 PS2, Line 157: bitbang_spi_set_sck(master, 0);
Wouldn't be needed if we assume CLK = 1 between commands.
You mean change from a CPOL=0, CPHA=0 controller into a CPOL=1, CPHA=1 controller (using terminology from https://en.wikipedia.org/wiki/Serial_Peripheral_Interface#Clock_polarity_and... )?
I don't think that belongs in *this* patch (since its a much bigger change).
I guess I could look into it but my gut feeling is that the gains would be extremely marginal.
The effect of my combined patches was to reduce the packet count to the CP210x from 32 packets per byte to 24 packets per *byte*. Changing the idling polarity would save no more than 2 packets per *command*. Even for a tiny two byte transfer the gains will be pretty small and I believe most of the runtime is spent doing *much* bigger transfers where 2 packets would probably be lost in the noise.
-
Daniel Thompson (Code Review)