Attention is currently required from: Moritz Fischer. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51129 )
Change subject: soc/rockchip/rk3399/sdram: Introduce pctl_start ......................................................................
Patch Set 1:
(4 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/51129/comment/e35b52bc_f962dff1 PS1, Line 9: Adapted from u-boot commit a0aebe8398 ("ram: rk3399: Add pctl start Can you please also explain what this actually does and why it is necessary?
File src/soc/rockchip/rk3399/sdram.c:
https://review.coreboot.org/c/coreboot/+/51129/comment/6d183aed_a4371449 PS1, Line 97: con I think this should be called get_ddrc_con0, not get_ddrc0_con (that's something U-Boot didn't update when they later fixed which of these is the channel-dependent one).
https://review.coreboot.org/c/coreboot/+/51129/comment/1931e304_998322a5 PS1, Line 347: write32(&ddrc0_con, 0x01000000); This is one of the special Rockchip write-mask registers, so what you're actually doing here is clearing bit 8 (ddr_ie_polarity). Please write this as
write32(&ddrc_con0, RK_CLRBITS(1 << 8));
(or define a DDR_CON_IE_POLARITY (1 << 8) somewhere and use that).
https://review.coreboot.org/c/coreboot/+/51129/comment/e6993dab_b6d2855f PS1, Line 360: write32(&ddrc0_con, 0x01000100); This would then be
write32(&ddr_con0, RK_SETBITS(1 << 8));