Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33055 )
Change subject: soc/sifive/fu540: add code for spi and map flash to memory spaces ......................................................................
Patch Set 10:
(5 comments)
https://review.coreboot.org/#/c/33055/9/src/mainboard/sifive/hifive-unleashe... File src/mainboard/sifive/hifive-unleashed/media.c:
https://review.coreboot.org/#/c/33055/9/src/mainboard/sifive/hifive-unleashe... PS9, Line 40: void boot_device_init(void)
SPI flash can be linked to QSPI0/QSPI1/QSPI2, which is related to the connection of the specific mot […]
but in the end only MSEL decides which boot media to use. As MSEL is SoC specific can't this be moved to SoC code?
https://review.coreboot.org/#/c/33055/9/src/mainboard/sifive/hifive-unleashe... PS9, Line 42: const static uint8_t reset_en = 0x66;
There is no spi_flash_probe used here, so needs to add code to reset spi flash
Any reason not to call the generic code, i.e. use of spi_flash_generic_probe()?
https://review.coreboot.org/#/c/33055/9/src/soc/sifive/fu540/spi.c File src/soc/sifive/fu540/spi.c:
https://review.coreboot.org/#/c/33055/9/src/soc/sifive/fu540/spi.c@56 PS9, Line 56: static uint8_t spi_rx(volatile struct spi_ctrl *spictrl)
It separate call requires SPI work in DPI / QPI mode
Ah yes, that makes totally sense. Thanks for explaining.
https://review.coreboot.org/#/c/33055/9/src/soc/sifive/fu540/spi.c@71 PS9, Line 71: if (fmt.proto == FU540_SPI_PROTO_S) {
The SPI is full-duplex and needs to send data to trigger receive. […]
Please add this information as inline comment, as it's not clear on the first sight.
https://review.coreboot.org/#/c/33055/9/src/soc/sifive/fu540/spi.c@78 PS9, Line 78: spi_tx(spictrl, out); 1) you can't use half duplex function here, as hardware operates in full duplex mode. As said, it hangs forever in spi_rx.
2) the xfer() method always assumes half duplex operation, even if hardware runs in full duplex mode