Attention is currently required from: Jason Glenesk, Furquan Shaikh, Marshall Dawson, Eric Peers, Karthik Ramasubramanian, Felix Held. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56228 )
Change subject: soc/amd/common/block/lpc/spi_dma: Implement SPI DMA functionality ......................................................................
Patch Set 6:
(10 comments)
File src/soc/amd/common/block/lpc/spi_dma.c:
https://review.coreboot.org/c/coreboot/+/56228/comment/36466ccc_188ed1f7 PS3, Line 103: ctrl |= LPC_ROM_DMA_CTRL_ERROR; /* Clear error */
I was using the stoney BKDG since it provides more details: […]
Ack
https://review.coreboot.org/c/coreboot/+/56228/comment/2bc879b7_9e7e8963 PS3, Line 209: val |= BIT(6);
Yeah this is the magic bit: b/179699789#comment11 has the explanation. […]
Ack
File src/soc/amd/common/block/lpc/spi_dma.c:
https://review.coreboot.org/c/coreboot/+/56228/comment/f3b4d8ce_d6871220 PS5, Line 134: if (spi_dma_has_error())
Print out an error in this case so that it is clear at what address the error occurred? Also, should […]
Done. I didn't clear the error since we clear it before we start a new transaction.
https://review.coreboot.org/c/coreboot/+/56228/comment/3718dd62_ae9302cc PS5, Line 144: else
nit: else is not required as the if block above returns.
Done
https://review.coreboot.org/c/coreboot/+/56228/comment/4f24e2d1_03b88f56 PS5, Line 173: udelay(10); This is what I replied to Eric:
It was just an arbitrary number. Doing very rough some math (not taking into account the transaction setup that happens every 64 bytes):
250 bytes / 10us @ 100 MHz (2-2-1) 166.65 bytes / 10us @ 66.6 MHz (2-2-1) 83.325 bytes / 10us @ 33.33 MHz (2-2-1)
The min block size is 64 bytes. From what I've seen we normally do 1k - 64k transactions, so I think 10us should be fine.
Thinking about it some more, there is no real downside to making this smaller. I'll make it 2us. That's 50 bytes / 2 us @ 100 MHz.
https://review.coreboot.org/c/coreboot/+/56228/comment/5a2b421a_8d26049a PS5, Line 177: );
Would it be helpful to also dump `stopwatch_duration_usecs()` to see how long transactions are takin […]
Since we are yielding control for an unknown amount of time, there is no easy way to know how long the transaction took. If we get lucky, this is the only thread running, and it would show a pretty accurate value. But if there are multiple threads, there is no telling how long before they yield. There is no hardware timestamp that keeps track either.
https://review.coreboot.org/c/coreboot/+/56228/comment/0439fcf5_d1e1380e PS5, Line 179: transaction.size - transaction.remaining
rdev API expects the function to return < 0 in case of any error. I am guessing if transaction. […]
Ah good catch. I should have read the documentation on it. I saw code doing the size != expected check and just assumed...
https://review.coreboot.org/c/coreboot/+/56228/comment/ae712466_5e3adea7 PS5, Line 185: assert(offset + size < CONFIG_ROM_SIZE); : assert(size);
Do we really need these checks? rdev API already takes care of this as part of `normalize_and_ok()`
Nope, just left overs from me developing the driver.
https://review.coreboot.org/c/coreboot/+/56228/comment/80ba8ef8_d51072a3 PS5, Line 188: size < LPC_ROM_DMA_MIN_ALIGNMENT
nit: Should this check be moved to can_use_dma() so that all attributes are checked in one place?
Done
https://review.coreboot.org/c/coreboot/+/56228/comment/a91e6413_ba66601a PS5, Line 222: /* Internal only registers: RN/CZN+ */
If this is applicable only to certain platforms, I think it would be best to use a Kconfig that enab […]
Updated the documentation.