Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35118 )
Change subject: drivers/spi: add drivers for sdcard mounted on the spi bus ......................................................................
Patch Set 14: Code-Review+1
(3 comments)
The code looks reasonable for a "standard" block device access protocol, but I don't know the protocol. Is there some public spec to the protocol that you could cite in the source, in case anybody else needs to do a deep-dive into this code? Something like "implementation follows SD association's SPI access protocol, found as doc id #12345 at https://...%22?
One thing I wonder: there are a bunch of loops waiting for a device register to flip just in the right way that could end up becoming endless loops. Are there reasonable timeouts for these loops? I'm not too concerned about a SPI read not eventually going 0xff, but with some others I'm not quite as confident that they will always hit the condition necessary to proceed.
Other than that there are just some minor style issues left.
https://review.coreboot.org/c/coreboot/+/35118/14/src/drivers/spi/spi_sdcard... File src/drivers/spi/spi_sdcard.c:
https://review.coreboot.org/c/coreboot/+/35118/14/src/drivers/spi/spi_sdcard... PS14, Line 40: all other lines have a space here
https://review.coreboot.org/c/coreboot/+/35118/14/src/drivers/spi/spi_sdcard... PS14, Line 73: 0xFE nit: we generally prefer lower-case hexadecimals
https://review.coreboot.org/c/coreboot/+/35118/14/src/drivers/spi/spi_sdcard... PS14, Line 311: waitting nit: waiting (same issue also elsewhere in this file)