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

[Guangjie Song (Code Review)]

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

Guangjie Song 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 27:

(11 comments)

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

https://review.coreboot.org/c/coreboot/+/84497/comment/929d412f_2baa83ab?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.

Done

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

Done

https://review.coreboot.org/c/coreboot/+/84497/comment/b703ad4d_b1551092?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 […]

Done

https://review.coreboot.org/c/coreboot/+/84497/comment/6e6a25d8_5dea1dc6?usp... :
PS26, Line 538: (
> remove

Done

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

Done

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

Done

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

Done

https://review.coreboot.org/c/coreboot/+/84497/comment/4f322ee1_9d06e567?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?

no, here are requirements for the order of setting

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

no, here are requirements for the order of setting

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

Done

https://review.coreboot.org/c/coreboot/+/84497/comment/17a3dd09_41614c1b?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 no […]

no, the api would be called in mminfra