ron minnich 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 13:
(5 comments)
https://review.coreboot.org/#/c/30466/13/src/mainboard/sifive/hifive-unleash... File src/mainboard/sifive/hifive-unleashed/flash.c:
https://review.coreboot.org/#/c/30466/13/src/mainboard/sifive/hifive-unleash... PS13, Line 21: extern void flash_init(void); This is going to burn you should you ever change flash_init for any reason. You will have to find and change this proto everywhere. Worse, if you miss even a single place, the code will misfunction in strange ways.
Can you put it in the include file for the mainboard or something?
https://review.coreboot.org/#/c/30466/13/src/mainboard/sifive/hifive-unleash... File src/mainboard/sifive/hifive-unleashed/romstage.c:
https://review.coreboot.org/#/c/30466/13/src/mainboard/sifive/hifive-unleash... PS13, Line 24: extern void flash_init(void); even a .h file in this directory, the standard is to call it mainboard.h
https://review.coreboot.org/#/c/30466/13/src/soc/sifive/fu540/include/soc/sp... File src/soc/sifive/fu540/include/soc/spi.h:
https://review.coreboot.org/#/c/30466/13/src/soc/sifive/fu540/include/soc/sp... PS13, Line 217: // Inlining header functions in C : // https://stackoverflow.com/a/23699777/7433423
This stackoverflow link seems unnecessary, because inlining is a common C feature.
and we try to avoid links in coreboot anyway. at least 25% of the links in our tree are out of date.
Further, this case of inlining seems not needed -- are you sure it's worth it? Were you able to measure it?
https://review.coreboot.org/#/c/30466/13/src/soc/sifive/fu540/include/soc/sp... File src/soc/sifive/fu540/include/soc/spi_flash.h:
https://review.coreboot.org/#/c/30466/13/src/soc/sifive/fu540/include/soc/sp... PS13, Line 29: */
That's a lot of functions for a header. Do they have to be in a header? […]
This should probably be in a .c, I can not see the case for inlining, if I am wrong please tell me :-)
https://review.coreboot.org/#/c/30466/13/src/soc/sifive/fu540/spi.c File src/soc/sifive/fu540/spi.c:
https://review.coreboot.org/#/c/30466/13/src/soc/sifive/fu540/spi.c@23 PS13, Line 23: #if __riscv_atomic we're going to need to do this differently, because I'm too dumb to understand this asm code. Would be nice to have a little set of functions called atomic and then call them, and have them compile to the right thing.
I'm not sure I see the need for atomic ops here -- aren't we running with one hart? can we remove this if there is no need?