[coreboot-gerrit] Change in coreboot[master]: soc/intel/quark: Add SD/MMC test support

[Martin Roth (Code Review)]

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.


-- 
To view, visit https://review.coreboot.org/19211
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I72785f0dcd466c05c1385cef166731219b583551
Gerrit-PatchSet: 11
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Lee Leahy <leroy.p.leahy at intel.com>
Gerrit-Reviewer: Martin Roth <martinroth at google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Sumeet R Pawnikar <sumeet.r.pawnikar at intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-HasComments: Yes