Julius Werner has posted comments on this change. ( https://review.coreboot.org/19477 )
Change subject: rockchip/rk3399: Add MIPI driver ......................................................................
Patch Set 2:
(6 comments)
https://review.coreboot.org/#/c/19477/2/src/soc/rockchip/rk3399/display.c File src/soc/rockchip/rk3399/display.c:
Line 116: rkclk_configure_vop_aclk(vop_id, 200 * MHz);
I am not sure,because i see it will be 384Mhz when hdmi output.
Okay, fair enough, then leave it here.
https://review.coreboot.org/#/c/19477/3/src/soc/rockchip/rk3399/display.c File src/soc/rockchip/rk3399/display.c:
PS3, Line 63: :
If you're going to remove this (and HDMI down below), you should delete the
The header is shared with RK3288 code which still needs them, so please don't. ;)
https://review.coreboot.org/#/c/19477/2/src/soc/rockchip/rk3399/mipi.c File src/soc/rockchip/rk3399/mipi.c:
Line 68: return dptdin_map[i].testdin;
refer to 《MIPI D-PHY Bidir 4L for TSMC 28-nm HPC /1.8V Databook》
Okay, fair enough. But shouldn't the comparison be (dptdin_map[i].max_mbps >= max_mbps) (not '>' ), then?
Line 192: mpclk = DIV_ROUND_UP(edid->mode.pixel_clock, MSEC_PER_SEC);
because i think porting from kernel code, and then we can refer to kernel p
That is not a good reason to have bad code, though. You're doing a lot of integer divisions on these numbers and you're loosing a lot of precision all over the place. I didn't review the kernel change so I can't comment on that, but here I can see that it is likely not ideal and I want you to change it. Please change these variables to hold Hz or at least KHz.
Line 193: /* take 1 / 0.8, since mbps must big than bandwidth of RGB */
refer to kernel code
Hmm. It would be nice if we could get a better explanation for *why* the margin is not enough if calculations show it should be. (Maybe because you're calculating in MHz everywhere and getting giant rounding errors?)
https://review.coreboot.org/#/c/19477/3/src/soc/rockchip/rk3399/mipi.c File src/soc/rockchip/rk3399/mipi.c:
PS3, Line 194: si->lanes) * 10 / This is also a bad integer division in the 18bpp case, btw. You should probably write it
mpclk / dsi->lanes * bpp / 8 * 10;
(Assuming you'll change mpclk to Hz so doing the division first isn't as bad.)
-
Julius Werner (Code Review)