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

(4 comments)

https://review.coreboot.org/#/c/19557/4/src/soc/rockchip/rk3399/clock.c
File src/soc/rockchip/rk3399/clock.c:

Line 349:  *    extern wave table).
nit: Should you maybe do this as you're writing it here? You're only setting SPREAD and DIVVAL at the end, after you've already enabled SSC. Wouldn't it be better to set them first?


Line 360: 		printk(BIOS_ERR,"%s: failed to get refdiv(%d)\n", __func__,
> From the actual test, we need 10us delay at least.
Okay. Please change the comment to reflect that you don't know why the delay is needed yet, then. Your comment reads like you knew what you're doing here when you don't.


PS4, Line 359: if (!(dpll_cfg->refdiv && dpll_cfg->refdiv <= 6)) {
             : 		printk(BIOS_ERR,"%s: failed to get refdiv(%d)\n", __func__,
             : 		       dpll_cfg->refdiv);
> Can we change it with the below patch?
Sorry, not quite sure what you mean here? I think this should be an assert()... it's a condition that shouldn't happen with the current code and is only dependent on compile-time factors.


Line 373: 	write32(&cru_ptr->dpll_con[3],
> In other word, the SSC register just enable the decimal mode, do not set th
Sorry, I don't understand what you mean. According to the TRM (PLL initialization section, 3.6.2):

> If DSMPD = 0 (DSM is enabled, "fractional mode")
> FOUTVCO = FREF / REFDIV * (FBDIV + FRAC / 224)

We are setting DSMPD to 0 here, so we are enabling fractional mode, right? That means FRAC (DPLL_CON2[23:0]) becomes relevant to calculate the output frequency and we should ensure that it's initialized correctly.


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