View Change

5 comments:

• File src/mainboard/sifive/hifive-unleashed/flash.c:

  • Patch Set #13, 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?

• File src/mainboard/sifive/hifive-unleashed/romstage.c:

  • Patch Set #13, Line 24: extern void flash_init(void);

    even a .h file in this directory, the standard is to call it
    mainboard.h

• File src/soc/sifive/fu540/include/soc/spi.h:

  • Patch Set #13, 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?

• File src/soc/sifive/fu540/include/soc/spi_flash.h:

  • Patch Set #13, 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 :-)

• File src/soc/sifive/fu540/spi.c:

  • Patch Set #13, 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?

To view, visit change 30466. To unsubscribe, or for help writing mail filters, visit settings.