Martin Roth has posted comments on this change. ( https://review.coreboot.org/19212 )
Change subject: mainboard/intel/galileo: Add SD controller configuration
......................................................................
Patch Set 11:
(3 comments)
https://review.coreboot.org/#/c/19212/11/src/mainboard/intel/galileo/Kconfig
File src/mainboard/intel/galileo/Kconfig:
Line 25:
add select DRIVERS_STORAGE_SD here, or default it to on below?
PS11, Line 182: ENABLE_SD_TESTING
if not adding the select above, add depends on DRIVERS_STORAGE_SD here?
https://review.coreboot.org/#/c/19212/11/src/mainboard/intel/galileo/Makefi…
File src/mainboard/intel/galileo/Makefile.inc:
PS11, Line 22: y
$(CONFIG_DRIVERS_STORAGE_SD)?
--
To view, visit https://review.coreboot.org/19212
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf4faa40fe01eca98abffa2681f61fd8e059f0c4
Gerrit-PatchSet: 11
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Lee Leahy <leroy.p.leahy(a)intel.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: Yes
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(a)intel.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: Yes
Julius Werner has posted comments on this change. ( https://review.coreboot.org/19558 )
Change subject: google/gru: support 800M/928M frequency for bob
......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/#/c/19558/3/src/mainboard/google/gru/Kconfig
File src/mainboard/google/gru/Kconfig:
Line 97
I think it might be nice to keep this option and use it for snprintf. In fact, why not make it user-configurable (i.e. add a selection text such as int "SDRAM frequency (800, 928)" and a 'help' block)?
PS3, Line 99:
Also, while we're here, the Kconfig name should be namespaced, e.g. GRU_SDRAM_FREQ.
https://review.coreboot.org/#/c/19558/3/src/mainboard/google/gru/Makefile.i…
File src/mainboard/google/gru/Makefile.inc:
PS3, Line 16: 933/
Maybe now would be a good time to officially rename all of these to 928.
https://review.coreboot.org/#/c/19558/3/src/mainboard/google/gru/sdram_conf…
File src/mainboard/google/gru/sdram_configs.c:
Line 49: if (IS_ENABLED(CONFIG_BOARD_GOOGLE_BOB) && board_id() < 4)
Rather than duplicating the file name list for all frequencies, why not just do this:
const char config_file[64];
ramcode = ram_code();
if (ramcode >= MAX_SDRAM_CONFIGS ||
snprintf(config_file, sizeof(config_file), "%s-%d",
sdram_configs[ramcode], CONFIG_GRU_SDRAM_FREQ) ||
(cbfs_boot_load_struct(config_file, ¶ms,
sizeof(params)) != sizeof(params)))
(You will have to put in a small patch to src/console/Makefile.inc to enable vsprintf.c in all stages first.)
https://review.coreboot.org/#/c/19558/3/src/mainboard/google/gru/sdram_para…
File src/mainboard/google/gru/sdram_params_800/Makefile.inc:
Line 23: sdram-params := $(addsuffix -$(freq), $(sdram-params))
I think I'd prefer if we keep using the exact same name (minus the .c extension) for the source files and the params in CBFS. Why not just rename them and throw them all into the same directory? (Then we don't need to have this Makefile logic duplicated either...)
--
To view, visit https://review.coreboot.org/19558
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I613050292a09ff56f4636d7af285075e32259ef4
Gerrit-PatchSet: 3
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Caesar Wang <wxt(a)rock-chips.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Philip Chen <philipchen(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: Yes