Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30466 )
Change subject: soc/sifive/fu540: add code to initialize flash ......................................................................
Patch Set 29:
(3 comments)
Please let me know if you plan to update this commit. I've you don't got time I can work on the requested code changes.
https://review.coreboot.org/#/c/30466/29/src/mainboard/sifive/hifive-unleash... File src/mainboard/sifive/hifive-unleashed/flash.c:
https://review.coreboot.org/#/c/30466/29/src/mainboard/sifive/hifive-unleash... PS29, Line 35: .data_proto = SPI_PROTO_Q,
These codes are for HiFive Unleashed, the hardware link as QPI, and using SPI here only makes the pe […]
I don't see why ignoring the MSEL should be a good choice. If the user selects SPI mode instead of QSPI, for whatever reason, we should support it. And it's only a bit that needs to be toggled.
https://review.coreboot.org/#/c/30466/29/src/mainboard/sifive/hifive-unleash... PS29, Line 42: initialize_spi_flash_mmap(
These codes used for HiFive Unleashed. This board doesn't have that much choice.
As far as I understand you can connect an additional flash on SPI1 and there's an MSEL to support that booting mode. SPI2 seems to be SDcard only, so out of scope if this patch.
https://review.coreboot.org/#/c/30466/29/src/soc/sifive/fu540/spi_flash.c File src/soc/sifive/fu540/spi_flash.c:
https://review.coreboot.org/#/c/30466/29/src/soc/sifive/fu540/spi_flash.c@52 PS29, Line 52: spi_txrx(spictrl, command_enable);
spi-generic.c is too heavy. […]
There's no reason to increase code fragmentation. In order to write to flash we need src/drivers/spi anyways. Also you might want to attach additional hardware to the SPI header which uses an existing driver in coreboot.