[coreboot-gerrit] [M] Change in coreboot[main]: soc/mediatek/mt8196: Add modem power driver to disable unused power

[Jarried Lin (Code Review)]

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

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

Change subject: soc/mediatek/mt8196: Add modem power driver to disable unused power
......................................................................

Patch Set 5:

(9 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/85651/comment/dfb27896_476f2a8d?usp... :
PS3, Line 7: disable modem power driver
> Any update?

Done

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

https://review.coreboot.org/c/coreboot/+/85651/comment/c29c93cb_c9ebf8b2?usp... :
PS3, Line 6: /*
           :  * md_power_down() - Power down modem
           :  */
> `/* Power down modem. […]

Done

https://review.coreboot.org/c/coreboot/+/85651/comment/8043f4df_e8f08882?usp... :
PS3, Line 9: md
> Also rename the file to modem_power_ctrl.*.

Done

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

https://review.coreboot.org/c/coreboot/+/85651/comment/facdcba4_0e259a48?usp... :
PS3, Line 28: mtk_apifrbus_ao_io_reg_bus
> As this is used only in this file, can we have a shorter name?

Done

https://review.coreboot.org/c/coreboot/+/85651/comment/01d08985_c307ff7b?usp... :
PS3, Line 31: struct mtk_spm_regs {
            : 	u32 reserved0[896];
            : 	u32 md1_pwr_con;
            : 	u32 reserved1[20];
            : 	u32 adsp_top_pwr_con;
            : 	u32 adsp_infra_pwr_con;
            : 	u32 adsp_ao_pwr_con;
            : 	u32 reserved2[11];
            : 	u32 spu_hwrot_pwr_con;
            : 	u32 reserved3[27];
            : 	u32 md_buck_iso_con;
            : 	u32 reserved4[5];
            : 	u32 pwr_status;
            : 	u32 pwr_status_2nd;
            : };
            : 
            : static struct mtk_spm_regs *const mtk_spm = (void *)SPM_BASE;
> This is already defined in spm.h. Please rebase onto CB:85599.

Done

https://review.coreboot.org/c/coreboot/+/85651/comment/b12f1abe_d214c83f?usp... :
PS3, Line 50: OTHER_SRAM_CKISO        = BIT(0),
            : 	PWR_RST_B		= BIT(0),
> Are these both for the `md1_pwr_con` register?

Done

https://review.coreboot.org/c/coreboot/+/85651/comment/7a665c32_7f6b5014?usp... :
PS3, Line 52: OTHER_SRAM_ISOINT_B	= BIT(1),
            : 	PWR_ISO			= BIT(1),
> Same here. […]

Done

https://review.coreboot.org/c/coreboot/+/85651/comment/9b643eb7_5947c4cc?usp... :
PS3, Line 90: 	struct pmif *pmif_arb = get_pmif_controller(PMIF_SPMI, SPMI_MASTER_1);
            : 
            : 	/* Vsram_vosel = 0x24d, read Vsram value */
            : 	reg = VSRAM_VOSEL;
            : 	pmif_arb->read(pmif_arb, SPMI_SLAVE_4, reg, &val);
            : 
            : 	/* Vmodem_vosel = 0x24e, write Vmodem 0.75V */
            : 	reg = VMODEM_VOSEL;
            : 	pmif_arb->write(pmif_arb, SPMI_SLAVE_4, reg, SPMI_SLAVE_4_750MV);
            : 	pmif_arb->read(pmif_arb, SPMI_SLAVE_4, reg, &val);
> One minor side effect is that `pmif_check_init_done` will be called multiple times. […]

Done

https://review.coreboot.org/c/coreboot/+/85651/comment/6a03dc15_6d1a7497?usp... :
PS3, Line 124: 
             : 	while ((read32(&mtk_apifrbus_ao_io_reg_bus->sleep_protect_rdy_sta_1) &
             : 		       MD1_PROT_STEP0_0_ACK_MASK) != MD1_PROT_STEP0_0_ACK_MASK) {
             : 		printk(BIOS_INFO, "First check: Does not get MD1_PROT_STEP0_0_ACK_MASK\n");
             : 		retryCount++;
             : 		if (retryCount > MAX_RETRY_COUNT) {
             : 			printk(BIOS_INFO, "First check: check failed\n");
             : 			break;
             : 		}
             : 	}
> use `retry` API or implement a `wait_for_state_ready` function

Done