Julius Werner has posted comments on this change. ( https://review.coreboot.org/19557 )
Change subject: rockchip: rk3399: enable DPLL SSC for DDR EMI test on bob ......................................................................
Patch Set 3:
(6 comments)
https://review.coreboot.org/#/c/19557/3/src/soc/rockchip/rk3399/Kconfig File src/soc/rockchip/rk3399/Kconfig:
Line 28: bool I think it might make sense to make this user configurable, so let's add an option text here (e.g. bool "Spread-spectrum DDR clock").
Also, please add a 'help' section with a more detailed description of what this does and why it might be useful.
Line 29: default y if BOARD_GOOGLE_BOB No board references in this file, please. Instead just have 'default n' here and put 'select RK3399_SPREAD_SPECTRUM_DDR if BOARD_GOOGLE_BOB' into gru/Kconfig:BOARD_SPECIFIC_OPTIONS.
Also, since Paul already found this CL you should probably make a separate patch for the gru/Kconfig part, or he's going to ask you to do it later anyway. ;)
https://review.coreboot.org/#/c/19557/3/src/soc/rockchip/rk3399/clock.c File src/soc/rockchip/rk3399/clock.c:
Line 344: #if (IS_ENABLED(CONFIG_RK3399_SPREAD_SPECTRUM_DDR)) This function doesn't need to be guarded. The compiler will eliminate it automatically if it can.
Line 375: * SSMOD SPREADAMP value 8 appears to mitigate EMI on boards tested. nit: Can you say how many Hz of spread "8" will do exactly? (Or if it's dependent on frequency express it in percentage or some-such.)
Line 632: /* configure DPLL SSC */ nit: slightly pointless comment, the function name already tells you this much
Line 633: rkclk_set_dpllssc(&dpll_cfg); Please use a C if, not the preprocessor.