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

(14 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/84497/comment/a185912a_693f90f1?usp... :
PS18, Line 9: Add mtcmos init code and APIs for controlling power domain
> Add `.` at the end.

Done

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

https://review.coreboot.org/c/coreboot/+/84497/comment/f01d3adc_3b8f8420?usp... :
PS18, Line 155: {NULL, NULL}
> `{ NULL, NULL }`

Done

https://review.coreboot.org/c/coreboot/+/84497/comment/b4855dfe_b88df54b?usp... :
PS18, Line 428: 		if (cb->id == id) {
> Is it possible to have multiple `cb` with this id?

yes, users call register different cb with the same id.

https://review.coreboot.org/c/coreboot/+/84497/comment/723842ff_f728f051?usp... :
PS18, Line 508: 		return -ETIMEDOUT;
> Print an error message?

Done

https://review.coreboot.org/c/coreboot/+/84497/comment/f88888e1_aa298921?usp... :
PS18, Line 518: 	if (!mask)
> How's this possible? Is your concern that `shift` might be too large?

Done

https://review.coreboot.org/c/coreboot/+/84497/comment/ca1c96c5_478c909a?usp... :
PS18, Line 528: bp_sta
> Doesn't need this variable.

Done

https://review.coreboot.org/c/coreboot/+/84497/comment/d2e41553_1de40011?usp... :
PS18, Line 546: end_step
> Just write `bp_num - 1` here. It's clear what that means. […]

Done

https://review.coreboot.org/c/coreboot/+/84497/comment/293b5f1f_98fb807e?usp... :
PS18, Line 550: 			mtcmos_bus_prot_ctrl(&bp_table[step_idx], !set);
> Print an error (or warning) here?

Done

https://review.coreboot.org/c/coreboot/+/84497/comment/7d2775c5_60109620?usp... :
PS18, Line 615: 			printk(BIOS_ERR, "mtcmos_pre_off_callback failed(%d)\n", ret);
> Can we move the error messages *inside* `mtcmos_pre_off_callback` and other functions, so that we do […]

Done

https://review.coreboot.org/c/coreboot/+/84497/comment/20766926_6596c758?usp... :
PS18, Line 714: vote_mds[id]
> Make it a local variable of const pointer. […]

Done

https://review.coreboot.org/c/coreboot/+/84497/comment/f73885b8_d95aa7ab?usp... :
PS18, Line 729: 20000
> Define a constant (either local const variable or macro)?

Done

https://review.coreboot.org/c/coreboot/+/84497/comment/5d29fe5e_7b445705?usp... :
PS18, Line 824: 	.name = "vdisp"
> Add `,`

Done

https://review.coreboot.org/c/coreboot/+/84497/comment/6d475759_0c2b5e7a?usp... :
PS18, Line 850: 		assert(!mtcmos_onoff(id, state));
> Don't put function calls inside `assert()`. It may not be evaluated. […]

Done

https://review.coreboot.org/c/coreboot/+/84497/comment/dd5b412e_f9caed8a?usp... :
PS18, Line 921: isolationoff
> Do you mean `isolation off`?

Done