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

[Caesar Wang (Code Review)]

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.


-- 
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: 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