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

(6 comments)

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

Line 373: 	/*
> 0x31f / (2 << 24) == 0.00024% of the intended rate (i.e. 22KHz)? =====> tha
Right, I missed that the fraction is additive (not multiplicative) somehow. You're right then, 0 is the correct value.


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

Line 361: 	 * hang in assert() with reboot tests.
Which assert() is failing in the failure case? The one below? (That wouldn't really make sense...)


Line 382: 		(PLL_FRACDIV_MASK << PLL_FRACDIV_SHIFT) &
I don't really get what the top part here means. Are you trying to only write the 23:0 bits? Then you'd have to use clrsetbits_le32(), since this is not a normal Rockchip write mask register. But since the rest of the bits are RO anyway, I think it should be fine to just set the whole thing to 0.


PS8, Line 407: 0
Can you not set spreadamp to 8 here already?


PS8, Line 414: Assert
This should actually read "Deassert reset", right?


Line 423: 			      divval << PLL_SSMOD_DIVVAL_SHIFT));
You're already writing divval above, why write it again?


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