[coreboot-gerrit] Change in coreboot[master]: rockchip/rk3399: Add MIPI driver

[Julius Werner (Code Review)]

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.)


-- 
To view, visit https://review.coreboot.org/19477
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I02475eefb187c619c614b1cd20e97074bc8d917f
Gerrit-PatchSet: 2
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: nickey.yang at rock-chips.com
Gerrit-Reviewer: Julius Werner <jwerner at chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Sean Paul <seanpaul at chromium.org>
Gerrit-Reviewer: Shunqian Zheng <zhengsq at rock-chips.com>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-Reviewer: nickey.yang at rock-chips.com
Gerrit-HasComments: Yes