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 6:
(3 comments)
https://review.coreboot.org/#/c/19558/6/src/mainboard/google/gru/sdram_confi... File src/mainboard/google/gru/sdram_configs.c:
Line 39: [6] = "sdram-lpddr3-micron-4GB-928", ...and still you have two tables? The whole point of using snprintf was that you wouldn't need to have multiple tables anymore! (Use "%s-%d" to add the frequency.)
Line 53: sdram_configs = sdram_configs_928; nit: would be better to factor out into a separate function, I think (i.e. int get_sdram_target_mhz(void) that returns either 800 or 928).
https://review.coreboot.org/#/c/19558/6/src/mainboard/google/gru/sdram_param... File src/mainboard/google/gru/sdram_params/Makefile.inc:
Line 22: sdram-params += sdram-lpddr3-samsung-2GB-24EB-928 nit: would look better grouping all frequencies of the same chip together, with a blank line between chips, I think?