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

[Lee Leahy (Code Review)]

Lee Leahy has posted comments on this change. ( https://review.coreboot.org/19211 )

Change subject: soc/intel/quark: Add SD/MMC test support
......................................................................


Patch Set 12:

(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
A "depends on" is necessary since the intent is to make it easy to test on all Quark boards.


https://review.coreboot.org/#/c/19211/11/src/soc/intel/quark/Makefile.inc
File src/soc/intel/quark/Makefile.inc:

PS11, Line 65: 
> Why so many stages?  Is it doing useful things in all of them?  It seems li
Removed extra stages and moved into romstage and ramstage


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: E)
> I think we got rid of CONFIG_CAR_DRIVERS_STORAGE, so we should just be able
Removed CONFIG_CAR_DRIVERS_STORAGE


PS11, Line 170: RT((struct sd
> We're in a !ENV_BOOTBLOCK #if section here, so this can't be true.  Same wi
Removed ENV_BOOTBLOCK


PS11, Line 194: 				err);
> For these printk statements, you could do something like this in console.h.
Defined LOG_DEBUG as shown above and defined STORAGE_DEBUG as BIOS_DEBUG


PS11, Line 219: name = storage_partition_name(media
> No need for this if when calling display log, as everything in the function
Done


PS11, Line 232: 
> BIOS_SPEW?
Using STORAGE_DEBUG to output at same level as LOG_DEBUG.


PS11, Line 247: tr_t)(media + 1) + 0x7) & 
> again, i think we got rid of this.
Done


-- 
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: 12
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Lee Leahy <leroy.p.leahy at intel.com>
Gerrit-Reviewer: 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