[coreboot-gerrit] [L] Change in coreboot[main]: soc/mediatek/mt8196: Add mtcmos init support

[Yu-Ping Wu (Code Review)]

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

Yu-Ping Wu has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/84497?usp=email )

Change subject: soc/mediatek/mt8196: Add mtcmos init support
......................................................................

Patch Set 26:

(11 comments)

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

https://review.coreboot.org/c/coreboot/+/84497/comment/e621d6d3_1bb80945?usp... :
PS26, Line 257: MTCMOS_ID_SSUSB_DP_PHY_P0
We don't need to store the `id` in `mtcmos_data`, if the array index is the id.

https://review.coreboot.org/c/coreboot/+/84497/comment/d916ff58_b02f83f1?usp... :
PS26, Line 457: ret = -EINVAL;
              : 				break;
Print an error message and return `-EINVAL` directly.

https://review.coreboot.org/c/coreboot/+/84497/comment/44a7fbe3_c3ae1f44?usp... :
PS26, Line 475: 		printk(BIOS_ERR, "%s failed(%d)\n", __func__, ret);
I think the error message could be further moved inside `mtcmos_callback()`, to further simplify the code.

```
switch (...)
    case CB_TYPE_PRE_ON:
        ...
        name = "pre_on";
        break;

printk(BIOS_ERR, "%s %s failed (%d)\n", __func__, name, ret);
```

https://review.coreboot.org/c/coreboot/+/84497/comment/0fb17237_19d21264?usp... :
PS26, Line 538: (
remove

https://review.coreboot.org/c/coreboot/+/84497/comment/e8ef94b0_45bafb48?usp... :
PS26, Line 540: (
remove

https://review.coreboot.org/c/coreboot/+/84497/comment/9c128042_5ed10e4e?usp... :
PS26, Line 591: const struct mtcmos_data md
Pass the const pointer instead.

https://review.coreboot.org/c/coreboot/+/84497/comment/a1453386_fb8b1684?usp... :
PS26, Line 640: mds[id]
Make this a local variable (as a pointer).

https://review.coreboot.org/c/coreboot/+/84497/comment/f51e5b0b_36b1472b?usp... :
PS26, Line 658: setbits32(ctl_addr, PWR_ISO);
              : 		setbits32(ctl_addr, PWR_CLK_DIS);
              : 		clrbits32(ctl_addr, PWR_RST_B);
              : 		clrbits32(ctl_addr, PWR_ON);
Can these be done in one clrsetbits32 call?

https://review.coreboot.org/c/coreboot/+/84497/comment/e625fb77_992b685b?usp... :
PS26, Line 690: clrbits32(ctl_addr, PWR_CLK_DIS);
              : 		clrbits32(ctl_addr, PWR_ISO);
One clrbits32 call?

https://review.coreboot.org/c/coreboot/+/84497/comment/d3989656_831eb6dc?usp... :
PS26, Line 843: {
remove

https://review.coreboot.org/c/coreboot/+/84497/comment/632a3980_cd0475a1?usp... :
PS26, Line 886: mtcmos_cb_register
Is this the only caller of `mtcmos_cb_register`? Will there be other callers from other files? If not, we don't need the `mtcmos_cb_register` API in the header.