[coreboot-gerrit] Change in coreboot[master]: rockchip: rk3399: enable DPLL SSC for DDR EMI test on bob

[Julius Werner (Code Review)]

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.


-- 
To view, visit https://review.coreboot.org/19557
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I75461d4235bcf55324e6664a1220754e770b4786
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