[coreboot-gerrit] [L] Change in coreboot[main]: soc/mediatek/mt8196: Add display driver

[Nancy Lin (Code Review)]

Attention is currently required from: Hung-Te Lin, Jarried Lin.

Nancy Lin has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/85950?usp=email )

Change subject: soc/mediatek/mt8196: Add display driver
......................................................................

Patch Set 2:

(23 comments)

File src/soc/mediatek/common/display.c:

https://review.coreboot.org/c/coreboot/+/85950/comment/eca5a211_2b97b8b0?usp... :
PS2, Line 46: __weak int mtk_edp_enable(void)
> From another [comment](https://review.coreboot. […]

This can't merged into mtk_edp_init. If this is merged in mtk_edp_init, then the flow is as following:
EDP config/enable first and then display pipe initialization. In this flow, it could display garbage because display pipe didn't send data to eDP. So the correct sequence is to enable the EDP after both EDP and display config done.

File src/soc/mediatek/mt8196/ddp.c:

https://review.coreboot.org/c/coreboot/+/85950/comment/fa99a196_2639c8cd?usp... :
PS2, Line 17: clrbits32(&reg->datapath_con, BIT(4));
            : 		setbits32(&reg->datapath_con, BIT(5));
> Single `clrsetbits32` call. Same for other blender types.

OK, I will fix it.

https://review.coreboot.org/c/coreboot/+/85950/comment/df8ade3f_b8ae403d?usp... :
PS2, Line 28: 		break;
> Add `OTHER_BLENDER:`

OK, I will fix it.

https://review.coreboot.org/c/coreboot/+/85950/comment/e92370ae_42f26d5a?usp... :
PS2, Line 36: height << 16 | width
> Define a macro for this […]

OK, I will fix it.

https://review.coreboot.org/c/coreboot/+/85950/comment/69093ef6_484cf030?usp... :
PS2, Line 38: ff
> upper case

OK, I will fix it.

https://review.coreboot.org/c/coreboot/+/85950/comment/9f666de2_6ef501c9?usp... :
PS2, Line 130: 0xff
> upper case

OK, I will fix it.

https://review.coreboot.org/c/coreboot/+/85950/comment/a28a18c5_6466c28b?usp... :
PS2, Line 196: 	/* mutex enable*/
> wrong style

OK, I will fix it.

https://review.coreboot.org/c/coreboot/+/85950/comment/8e339a6c_8c6f3311?usp... :
PS2, Line 205: blender_config(0, width, height, FIRST_BLENDER);
             : 	blender_config(1, width, height, OTHERS);
             : 	blender_config(2, width, height, OTHERS);
             : 	blender_config(3, width, height, LAST_BLENDER);
             : 	blender_start(0);
             : 	blender_start(1);
             : 	blender_start(2);
             : 	blender_start(3);
> Use for loops. […]

OK, I will fix it.

https://review.coreboot.org/c/coreboot/+/85950/comment/20c8b8e0_c55657d2?usp... :
PS2, Line 233: 	/* async config*/
> wrong style

OK, I will fix it.

https://review.coreboot.org/c/coreboot/+/85950/comment/96cb3c40_8263e545?usp... :
PS2, Line 264: 	/* exdma start*/
> wrong style

OK, I will fix it.

https://review.coreboot.org/c/coreboot/+/85950/comment/f149564f_03255887?usp... :
PS2, Line 275: 0xffffffff
> upper case

OK, I will fix it.

https://review.coreboot.org/c/coreboot/+/85950/comment/d8e4d11d_b3aa12c6?usp... :
PS2, Line 308:
> Check that `width <= XXX && height <= XXX`, and change the type of `width` and `height` to u16 for ` […]

OK, I will fix it

File src/soc/mediatek/mt8196/include/soc/ddp.h:

https://review.coreboot.org/c/coreboot/+/85950/comment/27893187_a25dba51?usp... :
PS2, Line 10: #define MAIN_PATH_OVL_NR 2
> Remove if unused

OK, I will remove it.

https://review.coreboot.org/c/coreboot/+/85950/comment/9a436097_d9d60f6e?usp... :
PS2, Line 15: 	/* 0x000 */
> Remove all of these comments, as we already have `check_member` below.

OK, I will fix it.

https://review.coreboot.org/c/coreboot/+/85950/comment/536c9919_5f24f2d1?usp... :
PS2, Line 58: check_member(ovlsys_cfg, rsz_in_cb2, 0xF70);
> One blank line below. Same for others.

OK, I will fix it.

https://review.coreboot.org/c/coreboot/+/85950/comment/23aaf5f4_7099f8cc?usp... :
PS2, Line 110: 0xc30
> upper case

OK, I will fix it.

https://review.coreboot.org/c/coreboot/+/85950/comment/d23cca1f_3da46e01?usp... :
PS2, Line 279: };
> blank line below

OK, I will fix it.

https://review.coreboot.org/c/coreboot/+/85950/comment/0508c3e1_98423cbd?usp... :
PS2, Line 281: (void *)MMSYS_MUTEX_BASE,
             : 	(void *)MMSYS1_MUTEX_BASE,
             : 	(void *)OVLSYS_MUTEX_BASE,
>

OK, I will fix it.

https://review.coreboot.org/c/coreboot/+/85950/comment/36f044a3_2986c9a7?usp... :
PS2, Line 286: OVL_INPUT_FORMAT
> Remove this unused name.

OK, I will fix it

https://review.coreboot.org/c/coreboot/+/85950/comment/e8f7e4bc_fe277263?usp... :
PS2, Line 317: /* DISPSYS mutex module*/
> wrong style

OK, I will fix it

https://review.coreboot.org/c/coreboot/+/85950/comment/e094fd25_ad28902f?usp... :
PS2, Line 343: /* DISPSYS1 mutex module*/
> wrong style

OK, I will fix it

https://review.coreboot.org/c/coreboot/+/85950/comment/ec1a2cff_26e69284?usp... :
PS2, Line 365: OTHERS
> Can we rename to `OTHER_BLENDER`?

OK, I will fix it

https://review.coreboot.org/c/coreboot/+/85950/comment/d12b25eb_31c6c072?usp... :
PS2, Line 366: 	BLENDER_LAYER_MAX,
> remove

OK, I will fix it