Nico Huber 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)
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;
For the divisor, that algorithm doesn't always result in the optimal divisor value (higher error than some other values).
For which value exactly??? I tried all you add below and got the exact same numbers. Actually one of the lower ones would be more accurate if you don't round down in (4,000,000 / baud) but I don't see a reason for anything below 115200 anyway.
I obtained the values used in the patch from this link: http://unwind.se/buspirate-baudrate/#std250000.
Don't care.
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.
Yes, please do so. Actually, I think we should just use 2M for newer versions and stick to 115200 for older ones. Less options, less confusion and if anybody comes up with a pre-v3 model that could go faster, we can still add an option for that.
https://review.coreboot.org/#/c/23057/1/buspirate_spi.c@202 PS1, Line 202: goto out_shutdown;
I added the goto to prevent the code from breaking if more statements were added before the `out_shu […]
I would just drop it. We already take care of it when we open the UART in buspirate_serialport_setup(). (IMHO, if any effect, reset- ting it here might hide bugs in other programs that just assume it's at 115200.)
https://review.coreboot.org/#/c/23057/1/buspirate_spi.c@352 PS1, Line 352: msg_pdbg("Detected Bus Pirate hardware %s\n", bp_commbuf);
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.
v3 pirates all have the USB to UART integrated, right? If so, I think it's fair to assume that there are no "different use cases".
-
Nico Huber (Code Review)