Caesar Wang 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:
(9 comments)
https://review.coreboot.org/#/c/19557/3//COMMIT_MSG Commit Message:
PS3, Line 7: rockchip: rk3399
Please use `rockchip/rk3399` as prefix.
Done
PS3, Line 9: Modulator(SSMOD)
Please add a space before the *(*.
Done
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
Done
Line 29: default y if BOARD_GOOGLE_BOB
No board references in this file, please. Instead just have 'default n' her
Okay, done.
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 au
Done
PS3, Line 351: /* Need to acquire ~30kHZ which is the target modulation frequency. : * The modulation frequency ~ 30kHz= OSC_HZ/revdiv/128/divval : * (the 128 is the number points in the query table). : */
Please use the style
done
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 dep
done
Line 632: /* configure DPLL SSC */
nit: slightly pointless comment, the function name already tells you this m
done
Line 633: rkclk_set_dpllssc(&dpll_cfg);
Please use a C if, not the preprocessor.
done.
I'm worrying about that rkclk_set_dpllssc() put here is too early.
Maybe we need put it in src/soc/rockchip/rk3399/sdram.c.
Since I found the reboot hang sdram_init...
Starting SDRAM initialization... PLL at 00000000ff760040: fbdiv=116, refdiv=1, postdiv1=3, postdiv2=1, vco=2784000 khz, output=928000 khz <hang>
..
I try to add the debug log, this issue is gone. that's weird. @@ -346,12 +346,13 @@ static void rkclk_set_dpllssc(struct pll_div *dpll_cfg) { u32 divval;
+ printk(BIOS_INFO,"%s:refdiv=%d====\n",__func__, dpll_cfg->refdiv); /* * Need to acquire ~30kHZ which is the target modulation frequency.