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

(9 comments)

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

Line 225: #define DIV_ROUND_UP(x, y)  (((x) + (y) - 1) / (y))
Oh, I only just noticed that you're redefining these here. Please don't do that... instead, use the existing implementations from <stdlib.h> and <timer.h> (this changes the naming from MSEC_PER_SEC to MSECS_PER_SEC, etc.).


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;
> if max_mbps = 100； when we use >= will return 0x10,but we want 0x20
Yeah... right. I don't know what I was thinking there. Sorry.


Line 406: 
> rk3288 and rk3399 have the same mipi ip，it will be more convenient if we wa
That doesn't require you to have this here, though. You can just put a

 static struct rk_mipi_regs *mipi_regs = (void *)MIPI_BASE;

at the top of this file and use that... it will still pick the right address for 3288 and 3399 because MIPI_BASE comes from the right <soc/addressmap.h> header.

(That reminds me, btw... if this code is already compatible with 3288, just put it in rockchip/common/mipi.c instead.)


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

PS4, Line 182: target_bps
target_bps should also be unsigned long, just in case. Also, I think it would be clearer to use u32 and u64 instead of unsigned int and unsigned long so there can be no confusion about how long the types are.


PS4, Line 183: 1500000000
1500 * MHz


PS4, Line 206: 5000000
5 * MHz and 40 * MHz


PS4, Line 216: lane_mbps
This should also be lane_bps instead and all subsequent calculations with it should be in bits/Hz if possible.


Line 405: 	int ret;
Remove this and just do

 if (function_call(...) < 0)
   return;

directly.


Line 436: 	mdelay(500);
I feel like I must have asked this somewhere already... where do the 500ms come from and are they really necessary? We don't usually care about display init that much because we skip it on normal boot, but 500ms is a *really* long time...


-- 
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: 4
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) <no-reply at coreboot.org>
Gerrit-Reviewer: nickey.yang at rock-chips.com
Gerrit-HasComments: Yes