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

[Anonymous Coward (Code Review)]

nickey.yang at rock-chips.com has posted comments on this change. ( https://review.coreboot.org/19477 )

Change subject: rockchip/rk3399: Add MIPI driver
......................................................................


Patch Set 3:

(17 comments)

https://review.coreboot.org/#/c/19477/2//COMMIT_MSG
Commit Message:

PS2, Line 7: rockchip/rk3399: 
> Please use the prefix: `rockchip/rk3399`.
Done


PS2, Line 7: dd MIPI driver
> Add MIPI driver
Done


PS2, Line 10: add
> adds
Done


https://review.coreboot.org/#/c/19477/2/src/soc/rockchip/rk3399/chip.h
File src/soc/rockchip/rk3399/chip.h:

Line 29: 	u32 panel_pixel_clock;	/* below only be considered for MIPI displays */
> nit: add a comment that the below will only be considered for MIPI displays
Done


https://review.coreboot.org/#/c/19477/2/src/soc/rockchip/rk3399/display.c
File src/soc/rockchip/rk3399/display.c:

Line 79: 	case VOP_MODE_EDP:
> Neither auto-detect nor HDMI works on 3399, can we just remove these cases?
Done


Line 116: 		/* select mipi-dsi0 signal from vop0 */
> This is the same for all display modes, right? Should we maybe just move it
I am not sure,because i see it will be 384Mhz when hdmi output.


Line 128: 				     edid.mode.pixel_clock * KHz)) {
> If you remove the HDMI and auto-detect cases, this should probably change t
Done


Line 150: 	default:
> This needs to be done for all cases, so it should move below the switch() b
Done


Line 153: 	mainboard_power_on_backlight();
> I'm not sure why this default: case is here, actually. There should probabl
Done


https://review.coreboot.org/#/c/19477/2/src/soc/rockchip/rk3399/mipi.c
File src/soc/rockchip/rk3399/mipi.c:

PS2, Line 45: MSEC_PER_SEC
> nit: pull the *2 in here so you don't get double the rounding error.
Done


Line 68: 			return dptdin_map[i].testdin;
> Please respond to my comment from https://chromium-review.googlesource.com/
refer to 《MIPI D-PHY Bidir 4L for TSMC 28-nm HPC /1.8V Databook》 
5.2.1.3
PLL Locking Mode and AFE Initialization

table 
Range (Mbps) hsfreqrange[5:0]
80-89 000000
90-99 010000
100-109 100000
110-129 000001
130-139 010001
140-149 100001
..

and refer kernel code:
http://lxr.free-electrons.com/source/drivers/gpu/drm/rockchip/dw-mipi-dsi.c 
(line 314)


Line 192: 	mpclk = DIV_ROUND_UP(edid->mode.pixel_clock, MSEC_PER_SEC);
> As mentioned in the other review, please switch to calculating in Hz or exp
because i think porting from kernel code, and then we can refer to kernel patch to fix some issues easier.


Line 193: 	/* take 1 / 0.8, since mbps must big than bandwidth of RGB */
> Please explain why you need this when your PLL calculation already makes su
refer to  kernel code 
  https://patchwork.freedesktop.org/patch/139913/


Line 200: 	tmp = pllref;
> nit: would look better as
Done


PS2, Line 209:  pre)) && (t
> I think this should be DIV_ROUND_UP(pllref, 40) to ensure you're not overst
Done


PS2, Line 377: T);
> Would DIV_ROUND_UP(dsi->lane_mbps, 8 * 20) maybe be safer here?
Done


Line 404: {
> This function doesn't really seem to have a separate purpose. Why not merge
Done


-- 
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: 3
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: Shunqian Zheng <zhengsq at rock-chips.com>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-Reviewer: nickey.yang at rock-chips.com
Gerrit-HasComments: Yes