Attention is currently required from: Elyes Haouas, Hung-Te Lin, Jarried Lin, Paul Menzel, 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 36:
(10 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/84497/comment/3609e958_3d18268c?usp... : PS34, Line 9: Add mtcmos init code and APIs for controlling power domain.
Can you please elaborate. What is mtcmos? There is nothing in `Documentation/`. […]
MTCMOS stands for Multi-Threshold CMOS. It is a power switch within the SoC that can control whether to supply power to the hardware module.
https://review.coreboot.org/c/coreboot/+/84497/comment/6c4d904c_955187c6?usp... : PS34, Line 11: driver init ok
How exactly? Some log message?
mtcmos is basic function and system cannot bootup normally if driver init with error, so we can check the driver init ok or not by cheking system can bootup.
File src/soc/mediatek/mt8196/mtcmos.c:
https://review.coreboot.org/c/coreboot/+/84497/comment/b766f14c_c6061f3d?usp... : PS26, Line 886: mtcmos_cb_register
@jarried.lin@mediatek.com @guangjie.song@mediatek.corp-partner.google. […]
mminfra is another module with its owner driver and it is not suitable to be submitted together with mtcmos.
File src/soc/mediatek/mt8196/mtcmos.c:
https://review.coreboot.org/c/coreboot/+/84497/comment/984a5324_ae3efc0a?usp... : PS34, Line 98: /* Already exist */ : #define ENODEV 19 /* No such device */ : #define EINVAL 22 /* Invalid argument */ : #define ETIMEDOUT 25 /* Connection timed out */
No idea about the comments. They seem redundant from the macro names themselves.
removed
https://review.coreboot.org/c/coreboot/+/84497/comment/4c4aa440_c61c25d7?usp... : PS34, Line 150: CB_TYPE_PRE_OFF_SRAM, : CB_TYPE_POST_ON_SRAM,
we don't need them in our case.
Done
https://review.coreboot.org/c/coreboot/+/84497/comment/544baffe_db7434e6?usp... : PS34, Line 390: if (!strcmp(item->name, cb->name)) {
Using a string as the unique identifier is inefficient. […]
add a cb filed in mtcmos_data or mtcmos_vote_data to save the callback function, and remove the logic
https://review.coreboot.org/c/coreboot/+/84497/comment/04091e25_c8b91eb1?usp... : PS34, Line 368: int mtcmos_cb_register(struct mtcmos_cb *cb) : { : struct mtcmos_cb *item = NULL; : bool exist = false; : : if (!cb) { : printk(BIOS_ERR, "node is NULL\n"); : return -EINVAL; : } : : if (!cb->name) { : printk(BIOS_ERR, "callback name NULL\n"); : return -EINVAL; : } : : if (!cb->pre_off && !cb->pre_on && !cb->post_on && !cb->post_off) { : printk(BIOS_ERR, "callback api NULL\n"); : return -EINVAL; : } : : /* add node to link list */ : list_for_each(item, mtcmos_cb_head, node) { : if (!strcmp(item->name, cb->name)) { : exist = true; : break; : } : } : if (exist) { : printk(BIOS_ERR, "ERR: %s exist\n", cb->name); : return -EEXIST; : } : : list_insert_after(&cb->node, &mtcmos_cb_head); : return 0; : } : : static int mtcmos_callback(u32 id, enum mtcmos_cb_type type) : { : struct mtcmos_cb *cb; : int ret = 0; : : list_for_each(cb, mtcmos_cb_head, node) { : if (cb->id == id) { : switch (type) { : case CB_TYPE_PRE_ON: : if (cb->pre_on) : ret = cb->pre_on(); : break; : case CB_TYPE_POST_ON: : if (cb->post_on) : ret = cb->post_on(); : break; : case CB_TYPE_PRE_OFF: : if (cb->pre_off) : ret = cb->pre_off(); : break; : case CB_TYPE_POST_OFF: : if (cb->post_off) : ret = cb->post_off(); : break; : case CB_TYPE_POST_ON_SRAM: : if (cb->post_on_sram) : ret = cb->post_on_sram(); : break; : case CB_TYPE_PRE_OFF_SRAM: : if (cb->pre_off_sram) : ret = cb->pre_off_sram(); : break; : default: : printk(BIOS_ERR, "%s(%u, %d) invalid type\n", __func__, id, type); : return -EINVAL; : } : : if (ret) : break; : } : } : : if (ret) : printk(BIOS_ERR, "%s(%u, %d) failed(%d)\n", __func__, id, type, ret); : : return ret; : } : : static inline int mtcmos_pre_on_callback(u32 id) : { : return mtcmos_callback(id, CB_TYPE_PRE_ON); : } : : static inline int mtcmos_pre_off_callback(u32 id) : { : return mtcmos_callback(id, CB_TYPE_PRE_OFF); : } : : static inline int mtcmos_post_on_callback(u32 id) : { : return mtcmos_callback(id, CB_TYPE_POST_ON); : } : : static inline int mtcmos_post_off_callback(u32 id) : { : return mtcmos_callback(id, CB_TYPE_POST_OFF); : } : : static inline int mtcmos_pre_off_sram_callback(u32 id) : { : return mtcmos_callback(id, CB_TYPE_PRE_OFF_SRAM); : } : : static inline int mtcmos_post_on_sram_callback(u32 id) : { : return mtcmos_callback(id, CB_TYPE_POST_ON_SRAM); : }
I don't think it is worth to add such complex logic to register the callback for only for `4` types […]
Done
https://review.coreboot.org/c/coreboot/+/84497/comment/6b0e1e85_97a7c850?usp... : PS34, Line 836: mtcmos_ctrl(MTCMOS_ID_MM_INFRA_AO, MTCMOS_POWER_ON); : mtcmos_ctrl(MTCMOS_ID_MM_INFRA0, MTCMOS_POWER_ON); : mtcmos_ctrl(MTCMOS_ID_MM_INFRA1, MTCMOS_POWER_ON); : : mtcmos_ctrl(MTCMOS_ID_DISP_VCORE, MTCMOS_POWER_ON);
So, the whole callback code is only for these 4 `mtcmos_ctrl` calls? If there are no other calls tha […]
add a cb filed in mtcmos_data or mtcmos_vote_data to save the callback function, the logic for calling callback is not need change.
File src/soc/mediatek/mt8196/mtcmos.c:
https://review.coreboot.org/c/coreboot/+/84497/comment/65758351_797b08c0?usp... : PS35, Line 7: include <assert.h> : #include <console/console.h> : #include <delay.h> : #include <soc/addressmap.h> : #include <soc/mtcmos.h> : #include <soc/pll.h> : #include <soc/spm_mtcmos.h> : #include <string.h> : #include <timer.h>
I'm not sure if you are using all those headers.
remove unused assert.h & string.h
https://review.coreboot.org/c/coreboot/+/84497/comment/b7991548_b12a42b5?usp... : PS35, Line 353: int
maybe "enum cb_err" ?
no need mtcmos_cb_register is removed
-
Guangjie Song (Code Review)