Martin Roth has posted comments on this change. ( https://review.coreboot.org/19211 )
Change subject: soc/intel/quark: Add SD/MMC test support ......................................................................
Patch Set 11:
(8 comments)
https://review.coreboot.org/#/c/19211/11/src/soc/intel/quark/Kconfig File src/soc/intel/quark/Kconfig:
PS11, Line 317: User must : also enable one or both of DRIVERS_STORAGE_SD or DRIVERS_STORAGE_MMC. Should we have a 'depends on' statement for these instead of just adding it in the help?
https://review.coreboot.org/#/c/19211/11/src/soc/intel/quark/Makefile.inc File src/soc/intel/quark/Makefile.inc:
PS11, Line 65: bootblock-y += storage_test.c Why so many stages? Is it doing useful things in all of them? It seems like anything done in bootblock for example, might be better done in romstage, since we don't have memory or console in bootblock.
https://review.coreboot.org/#/c/19211/11/src/soc/intel/quark/storage_test.c File src/soc/intel/quark/storage_test.c:
PS11, Line 164: CONFIG_CAR_DRIVERS_STORAGE I think we got rid of CONFIG_CAR_DRIVERS_STORAGE, so we should just be able to use this without the if, right?
PS11, Line 170: ENV_BOOTBLOCK We're in a !ENV_BOOTBLOCK #if section here, so this can't be true. Same with if above.
PS11, Line 194: if (IS_ENABLED(CONFIG_STORAGE_LOG)) For these printk statements, you could do something like this in console.h.
#define STORAGE_DEBUG (IS_ENABLED(CONFIG_STORAGE_LOG) ? BIOS_DEBUG : BIOS_NEVER)
Then just use: printk(STORAGE_DEBUG, "...
PS11, Line 219: if (IS_ENABLED(CONFIG_STORAGE_LOG)) No need for this if when calling display log, as everything in the function is surrounded by the same if.
PS11, Line 232: BIOS_DEBUG BIOS_SPEW?
PS11, Line 247: CONFIG_CAR_DRIVERS_STORAGE again, i think we got rid of this.
-
Martin Roth (Code Review)