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.in... 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_confi... 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_param... 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...)