[coreboot-gerrit] Change in coreboot[master]: google/gru: support 800M/928M frequency for bob

[Julius Werner (Code Review)]

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.inc
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_configs.c
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, &params,
                            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_params_800/Makefile.inc
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 at rock-chips.com>
Gerrit-Reviewer: Julius Werner <jwerner at chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Philip Chen <philipchen at google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-HasComments: Yes