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.