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

(15 comments)

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;
nit: add a comment that the below will only be considered for MIPI displays.


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

Line 79: 		/* try EDP first, then HDMI */
Neither auto-detect nor HDMI works on 3399, can we just remove these cases?


Line 116: 		rkclk_configure_vop_aclk(vop_id, 200 * MHz);
This is the same for all display modes, right? Should we maybe just move it below the switch (e.g. right above rkvop_mode_set())? Or does it need to be enabled this early?


Line 128: 		printk(BIOS_WARNING, "Cannot read any EDID info, aborting.\n");
If you remove the HDMI and auto-detect cases, this should probably change to "Unsupported vop_mode %d, aborting.\n"


Line 150: 		mainboard_power_on_backlight();
This needs to be done for all cases, so it should move below the switch() block.


Line 153: 	default:
I'm not sure why this default: case is here, actually. There should probably be a separate default: that just calls die() or something, because it shouldn't happen. (You can take out detected_mode and just switch over conf->vop_mode then as well, if we take out auto detect.)


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.


Line 68: 			return dptdin_map[i].testdin;
Please respond to my comment from https://chromium-review.googlesource.com/c/477811/1/src/soc/rockchip/rk3399/mipi.c#82 and explain what you're trying to do.


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 explain why you can't.


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 sure it always rounds the bit rate up.


Line 200: 	}
nit: would look better as

 target_mbps = mpclk * (bpp...
 if (target_mbps >= max_mbps) {
   printk(...);
   return -1;
 }


PS2, Line 209: pllref / 40)
I think this should be DIV_ROUND_UP(pllref, 40) to ensure you're not overstepping the allowed range due to rounding errors.

(Also isn't a smaller pre-divisor usually preferable over a larger one, everything else being equal? Wouldn't it be better to make this an upwards-counting loop?)


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


Line 404: void rk_mipi_init(struct edid *edid)
This function doesn't really seem to have a separate purpose. Why not merge it with rk_mipi_prepare()?


Line 406: 	rk_mipi.regs = (struct rk_mipi_regs *)MIPI_BASE;
Does this need to be part of the structure? Do you have more than one MIPI controller and do we ever expect we might want to use the other one in firmware? Otherwise, just make this pointer a global constant.


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