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

[Yu-Ping Wu (Code Review)]

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

Yu-Ping Wu 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:

(25 comments)

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

https://review.coreboot.org/c/coreboot/+/85950/comment/60febca4_8c547fb2?usp... :
PS2, Line 46: __weak int mtk_edp_enable(void)
From another [comment](https://review.coreboot.org/c/coreboot/+/85949/comment/65d79f70_a820a51e/), I think this could be merged into `mtk_edp_init`.

File src/soc/mediatek/mt8196/Makefile.mk:

https://review.coreboot.org/c/coreboot/+/85950/comment/d1643519_4a7dbda8?usp... :
PS2, Line 67: ramstage-y += mtcmos.c
What's this for?

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

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

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

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

```
#define SIZE(w, h) ((u32)(h) << 16 | (width))
```

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

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

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

https://review.coreboot.org/c/coreboot/+/85950/comment/27305aa1_bb520e9f?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.

```
int i;
enum mtk_disp_blender_layer blender_type[ARRAY_SIZE(blenders)] = {
    FIRST_BLENDER,
    OTHER_BLENDER,
    OTHER_BLENDER,
    LAST_BLENDER
};

for (i = 0; i < ARRAY_SIZE(blenders); i++) {
    struct blender *blender = &blenders[i];
    // I assume we can adjust the calling order
    blender_config(blender, width, height, blender_type[i]);
    blender_start(blender);
}
```

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

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

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

https://review.coreboot.org/c/coreboot/+/85950/comment/8edde20f_893094f0?usp... :
PS2, Line 308: 
Check that `width <= XXX && height <= XXX`, and change the type of `width` and `height` to u16 for `main_disp_path_setup`, `ovlsys_layer_config` and other functions in this file (except the local variables here).

`XXX` is the maximum supported width/height in this driver. From `BIT(30) | height << 16 | width`, I assume height can be 14 bits, and width can be 16 bits. Please confirm.

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

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

https://review.coreboot.org/c/coreboot/+/85950/comment/6a95b9a3_c8e1f609?usp... :
PS2, Line 12: 0xffffffff
upper case

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

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

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

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

https://review.coreboot.org/c/coreboot/+/85950/comment/90080e8a_2c82c482?usp... :
PS2, Line 281: (void *)MMSYS_MUTEX_BASE,
             : 	(void *)MMSYS1_MUTEX_BASE,
             : 	(void *)OVLSYS_MUTEX_BASE,
```
[DISP0] = (void *)MMSYS_MUTEX_BASE,
[DISP1] = (void *)MMSYS1_MUTEX_BASE,
[OVL0] = (void *)OVLSYS_MUTEX_BASE,
```

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

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

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

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

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