Shawn Anastasio has posted comments on this change. ( https://review.coreboot.org/23057 )
Change subject: buspirate_spi: Add support for variable serial speeds ......................................................................
Patch Set 1:
(3 comments)
Hi, thanks for the comments.
I saw that patch earlier but didn't like how it enforced a single baud rate which might not always be compatible with your hardware, so I added the parameter.
I've addressed some of your replies below. Please let me know which directions to take with them.
Thanks again, Shawn Anastasio
https://review.coreboot.org/#/c/23057/1/buspirate_spi.c File buspirate_spi.c:
https://review.coreboot.org/#/c/23057/1/buspirate_spi.c@41 PS1, Line 41: const int divisor;
Looks like the divisor is simply `4,000,000 / baud - 1`. Though, as […]
For the divisor, that algorithm doesn't always result in the optimal divisor value (higher error than some other values). I obtained the values used in the patch from this link: http://unwind.se/buspirate-baudrate/#std250000.
As for the older buspirates, I'm not sure what custom baud values they support, if any. I could potentially add a hardware version check to prevent this functionality on older boards.
https://review.coreboot.org/#/c/23057/1/buspirate_spi.c@202 PS1, Line 202: goto out_shutdown;
Actually, no goto needed here. […]
I added the goto to prevent the code from breaking if more statements were added before the `out_shutdown` label with the thought that the compiler would optimize it out if it's not needed. I can remove it, however, if you believe it to be unnecessary.
As for the baud change, I figured that since the Bus Pirate was resetting itself to 115200, the host should do the same, but you're right that it's unnecessary since we're closing the UART connection immediately after. I can remove it if you want.
https://review.coreboot.org/#/c/23057/1/buspirate_spi.c@352 PS1, Line 352: msg_pdbg("Detected Bus Pirate hardware %s\n", bp_commbuf);
Here the hardware version is stored in `bp_commbuf` can we use […]
The BP can support many variable baud rates and some may be more appropriate in certain situations for different use cases. Instead of trying to guess this, I added the parameter so that the user can select the correct baud rate if they know it will work for their use case. The wiki could be updated to reflect this information as well.
Since I don't have any BP 2 or older hardware to test, though, I could use this value to prevent baud rate changes entirely on the older hardware as mentioned above.
-
Shawn Anastasio (Code Review)