Attention is currently required from: Hung-Te Lin, Roger Lu, Yu-Ping Wu, Yidi Lin, Yuchen Huang. Ran Bi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46878 )
Change subject: soc/mediatek/mt8192: add clkbuf and srclken_rc MT6359P driver ......................................................................
Patch Set 58:
(13 comments)
File src/soc/mediatek/mt8192/clkbuf.c:
https://review.coreboot.org/c/coreboot/+/46878/comment/df53dd23_a336a880 PS50, Line 21: = NULL
No need for this.
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/6691389b_13d973e1 PS50, Line 43: (
No parentheses.
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/00460a2d_55d6c482 PS50, Line 60: u32 pmic_cw00 = 0, pmic_cw09 = 0, pmic_cw12 = 0, pmic_cw13 = 0, : pmic_cw15 = 0, pmic_cw19 = 0, top_spi_con1 = 0, : ldo_vrfck_op_en = 0, ldo_vbbck_op_en = 0, ldo_vrfck_en = 0, : ldo_vbbck_en = 0; : u32 vrfck_hv_en = 0;
No need for initialization.
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/fbcfca5c_519fb79d PS50, Line 138: #ifndef MTK_SRCLKEN_RC_SUPPORT
Is this always defined for mt8192? If we really need an if-else statement, I'd prefer using Kconfig.
Done
File src/soc/mediatek/mt8192/include/soc/clkbuf.h:
https://review.coreboot.org/c/coreboot/+/46878/comment/4e7c12cd_49752293 PS50, Line 52: #define PMIC_RG_VRFCK_HV_EN_MASK 0x1 : #define PMIC_RG_VRFCK_HV_EN_SHIFT 9 : #define PMIC_RG_LDO_VRFCK_EN_MASK 0x1 : #define PMIC_RG_LDO_VRFCK_EN_SHIFT 0 : #define PMIC_RG_LDO_VRFCK_ANA_SEL_MASK 0x1 : #define PMIC_RG_LDO_VRFCK_ANA_SEL_SHIFT 0 : #define PMIC_RG_LDO_VBBCK_EN_MASK 0x1 : #define PMIC_RG_LDO_VBBCK_EN_SHIFT 0 : #define PMIC_RG_VRFCK_NDIS_EN_MASK 0x1 : #define PMIC_RG_VRFCK_NDIS_EN_SHIFT 11 : #define PMIC_RG_VRFCK_1_NDIS_EN_MASK 0x1 : #define PMIC_RG_VRFCK_1_NDIS_EN_SHIFT 0 : #define PMIC_RG_LDO_VRFCK_HW14_OP_EN_MASK 0x1 : #define PMIC_RG_LDO_VRFCK_HW14_OP_EN_SHIFT 14 : #define PMIC_RG_LDO_VBBCK_HW14_OP_EN_MASK 0x1 : #define PMIC_RG_LDO_VBBCK_HW14_OP_EN_SHIFT 14
Can we use DEFINE_BITFIELD to replace BIT and SHIFT? […]
Done
File src/soc/mediatek/mt8192/include/soc/pmif.h:
https://review.coreboot.org/c/coreboot/+/46878/comment/b895ee73_600cf313 PS50, Line 200: start external
External
Done
File src/soc/mediatek/mt8192/include/soc/srclken_rc.h:
https://review.coreboot.org/c/coreboot/+/46878/comment/b6eb1d1a_3be20668 PS50, Line 59: #define SW_RESET_MSK 0x1 : #define SW_RESET_SHFT 0 : #define CG_32K_EN_MSK 0x1 : #define CG_32K_EN_SHFT 1 : #define CG_FCLK_EN_MSK 0x1 : #define CG_FCLK_EN_SHFT 2 : #define CG_FCLK_FR_EN_MSK 0x1 : #define CG_FCLK_FR_EN_SHFT 3 : #define MUX_FCLK_FR_MSK 0x1 : #define MUX_FCLK_FR_SHFT 4 : DEFINE_BIT(SW_RESET, 0) : : /* RC_CENTRAL_CFG1 */ : #define SRCLKEN_RC_EN_MSK 0x1 : #define SRCLKEN_RC_EN_SHFT 0 : #define RCEN_ISSUE_M_MSK 0x1 : #define RCEN_ISSUE_M_SHFT 1 : #define RC_SPI_ACTIVE_MSK 0x1 : #define RC_SPI_ACTIVE_SHFT 2 : #define SRCLKEN_RC_EN_SEL_MSK 0x1 : #define SRCLKEN_RC_EN_SEL_SHFT 3 : #define VCORE_SETTLE_T_MSK 0x7 : #define VCORE_SETTLE_T_SHFT 5 : #define ULPOSC_SETTLE_T_MSK 0xf : #define ULPOSC_SETTLE_T_SHFT 8 : #define NON_DCXO_SETTLE_T_MSK 0x3ff : #define NON_DCXO_SETTLE_T_SHFT 12 : #define DCXO_SETTLE_T_MSK 0x3ff : #define DCXO_SETTLE_T_SHFT 22 : DEFINE_BIT(SRCLKEN_RC_EN, 0) : : /* RC_CENTRAL_CFG2 */ : #define SRCVOLTEN_CTRL_MSK 0xf : #define SRCVOLTEN_CTRL_SHFT 0 : #define VREQ_CTRL_MSK 0xf : #define VREQ_CTRL_SHFT 4 : #define SRCVOLTEN_VREQ_SEL_MSK 0x1 : #define SRCVOLTEN_VREQ_SEL_SHFT 8 : #define SRCVOLTEN_VREQ_M_MSK 0x1 : #define SRCVOLTEN_VREQ_M_SHFT 9 : #define ULPOSC_CTRL_M_MSK 0xf : #define ULPOSC_CTRL_M_SHFT 12 : #define PWRAP_SLP_CTRL_M_MSK 0xf : #define PWRAP_SLP_CTRL_M_SHFT 21 : #define PWRAP_SLP_MUX_SEL_MSK 0x1 : #define PWRAP_SLP_MUX_SEL_SHFT 25 : : /* RC_DCXO_FPM_CFG */ : #define DCXO_FPM_CTRL_M_MSK 0xf : #define DCXO_FPM_CTRL_M_SHFT 0 : #define SRCVOLTEN_FPM_MSK_B_MSK 0x1 : #define SRCVOLTEN_FPM_MSK_B_SHFT 4 : #define SUB_SRCLKEN_FPM_MSK_B_MSK 0x1fff : #define SUB_SRCLKEN_FPM_MSK_B_SHFT 16 : : /* RC_CENTRAL_CFG3 */ : #define TO_LPM_SETTLE_EN_MSK 0x1 : #define TO_LPM_SETTLE_EN_SHFT 0 : #define BLK_SCP_DXCO_MD_TARGET_MSK 0x1 : #define BLK_SCP_DXCO_MD_TARGET_SHFT 1 : #define BLK_COANT_DXCO_MD_TARGET_MSK 0x1 : #define BLK_COANT_DXCO_MD_TARGET_SHFT 2 : #define TO_BBLPM_SETTLE_EN_MSK 0x1 : #define TO_BBLPM_SETTLE_EN_SHFT 0x3 : #define TO_LPM_SETTLE_T_MSK 0x2ff : #define TO_LPM_SETTLE_T_SHFT 12 : : /* RC_CENTRAL_CFG4 */ : #define KEEP_RC_SPI_ACTIVE_MSK 0x1ff : #define KEEP_RC_SPI_ACTIVE_SHFT 0 : #define PWRAP_VLD_FORCE_MAK 0x1 : #define PWRAP_VLD_FORCE_SHFT 16 : #define SLEEP_VLD_MODE_MAK 0x1 : #define SLEEP_VLD_MODE_SHFT 17 : : /* RC_MXX_SRCLKEN_CFG */ : #define SW_SRCLKEN_FPM_MSK 0x1 : #define SW_SRCLKEN_FPM_SHFT 4 : #define SW_SRCLKEN_BBLPM_MSK 0x1 : #define SW_SRCLKEN_BBLPM_SHFT 5 :
can we use DEFINE_BIT and DEFINE_BITFIELD for all?
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/5e37c6de_ac667353 PS50, Line 155: SRCLKEN_FPM_MASK_B_MSK
This is not used anywhere.
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/a4d2aa46_849d44ad PS50, Line 230: RC_SUBSYS_SET
Where's the usage?
Removed.
File src/soc/mediatek/mt8192/pmif.c:
https://review.coreboot.org/c/coreboot/+/46878/comment/7de8bfdc_106b8bf2 PS50, Line 183: if (mode == PMIF_VLD_RDY) { : /* spm and scp sleep request disable spi and spmi */ : SET32_BITFIELDS(&pmif_spi_arb[0].mtk_pmif->sleep_protection_ctrl, : PMIFSPI_SPM_SLEEP_REQ_SEL, 1, : PMIFSPI_SCP_SLEEP_REQ_SEL, 1); : SET32_BITFIELDS(&pmif_spmi_arb[0].mtk_pmif->sleep_protection_ctrl, : PMIFSPMI_SPM_SLEEP_REQ_SEL, 1, : PMIFSPMI_SCP_SLEEP_REQ_SEL, 1); : : /* pmic vld/rdy control spi mode enable : * srclken control spi mode disable : * vreq control spi mode disable : */ : SET32_BITFIELDS(&pmif_spi_arb[0].mtk_pmif->spi_mode_ctrl, : PMIFSPI_MD_CTL_PMIF_RDY, 1, : PMIFSPI_MD_CTL_SRCLK_EN, 0, : PMIFSPI_MD_CTL_SRVOL_EN, 0); : SET32_BITFIELDS(&pmif_spmi_arb[0].mtk_pmif->spi_mode_ctrl, : PMIFSPMI_MD_CTL_PMIF_RDY, 1, : PMIFSPMI_MD_CTL_SRCLK_EN, 0, : PMIFSPMI_MD_CTL_SRVOL_EN, 0); : : /* srclken rc interface enable */ : SET32_BITFIELDS(&pmif_spi_arb[0].mtk_pmif->inf_en, : PMIFSPI_INF_EN_SRCLKEN_RC_HW, 1); : : /* dcxo interface disable */ : SET32_BITFIELDS(&pmif_spi_arb[0].mtk_pmif->other_inf_en, : PMIFSPI_OTHER_INF_DXCO0_EN, 0, : PMIFSPI_OTHER_INF_DXCO1_EN, 0); : : /* srclken enable, dcxo0,1 disable */ : SET32_BITFIELDS(&pmif_spi_arb[0].mtk_pmif->arb_en, : PMIFSPI_ARB_EN_SRCLKEN_RC_HW, 1, : PMIFSPI_ARB_EN_DCXO_CONN, 0, : PMIFSPI_ARB_EN_DCXO_NFC, 0); : } else if (mode == PMIF_SLP_REQ) { : /* spm and scp sleep request enable spi and spmi */ : SET32_BITFIELDS(&pmif_spi_arb[0].mtk_pmif->sleep_protection_ctrl, : PMIFSPI_SPM_SLEEP_REQ_SEL, 0, : PMIFSPI_SCP_SLEEP_REQ_SEL, 0); : SET32_BITFIELDS(&pmif_spmi_arb[0].mtk_pmif->sleep_protection_ctrl, : PMIFSPMI_SPM_SLEEP_REQ_SEL, 0, : PMIFSPMI_SCP_SLEEP_REQ_SEL, 0); : : /* pmic vld/rdy control spi mode disable : * srclken control spi mode enable : * vreq control spi mode enable : */ : SET32_BITFIELDS(&pmif_spi_arb[0].mtk_pmif->spi_mode_ctrl, : PMIFSPI_MD_CTL_PMIF_RDY, 0, : PMIFSPI_MD_CTL_SRCLK_EN, 1, : PMIFSPI_MD_CTL_SRVOL_EN, 1); : SET32_BITFIELDS(&pmif_spmi_arb[0].mtk_pmif->spi_mode_ctrl, : PMIFSPMI_MD_CTL_PMIF_RDY, 0, : PMIFSPMI_MD_CTL_SRCLK_EN, 1, : PMIFSPMI_MD_CTL_SRVOL_EN, 1); : : /* srclken rc interface disable */ : SET32_BITFIELDS(&pmif_spi_arb[0].mtk_pmif->inf_en, : PMIFSPI_INF_EN_SRCLKEN_RC_HW, 0); : : /* dcxo interface enable */ : SET32_BITFIELDS(&pmif_spi_arb[0].mtk_pmif->other_inf_en, : PMIFSPI_OTHER_INF_DXCO0_EN, 1, : PMIFSPI_OTHER_INF_DXCO1_EN, 1); : : /* srclken disable, dcxo0,1 enable */ : SET32_BITFIELDS(&pmif_spi_arb[0].mtk_pmif->arb_en, : PMIFSPI_ARB_EN_SRCLKEN_RC_HW, 0, : PMIFSPI_ARB_EN_DCXO_CONN, 1, : PMIFSPI_ARB_EN_DCXO_NFC, 1); : } else : return;
We should try to minimize duplication, e.g, […]
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/49178efb_de1e7e08 PS50, Line 261: #ifdef MTK_SRCLKEN_RC_SUPPORT : printk(BIOS_ERR, "[%s] PMIF_VLD_RDY\n", __func__); : pmif_select(PMIF_VLD_RDY); : #else : printk(BIOS_ERR, "[%s] PMIF_SLP_REQ\n", __func__); : pmif_select(PMIF_SLP_REQ); : #endif
if (CONFIG(.... […]
Done
File src/soc/mediatek/mt8192/srclken_rc.c:
https://review.coreboot.org/c/coreboot/+/46878/comment/47ccc5b1_04d94ae1 PS50, Line 14: /* RC settle time setting */ : #define FULL_SET_HW_MODE 0 /* dcxo mode use pmrc_en */ : #define VCORE_SETTLE_TIME 1 /* ~= 30us */ : #define ULPOSC_SETTLE_TIME 4 /* ~= ? 150us */ : #define XO_SETTLE_TIME 0x1 /* 2^(step_sz+5) * 0x33 *30.77ns~=400us */ : #define DCXO_SETTLE_TIME 0x1 /* 2^(step_sz+5) * 0x87 *30.77ns~= 1063us */ : #define CENTROL_CNT_STEP 0x3 /* Fix in 3, central align with Mxx Channel */ : #define DCXO_STABLE_TIME 0x70 : #define XO_STABLE_TIME 0x70 : #define KEEP_RC_SPI_ACTIVE 1 : #define SRCLKEN_RC_EN_SEL 0
enum is better than #define
Done
https://review.coreboot.org/c/coreboot/+/46878/comment/1cfd03cb_4bac5041 PS50, Line 26: #define INIT_SUBSYS_FPM_TO_LPM (1 << CHN_RF | 1 << CHN_DEEPIDLE | 1 << CHN_MD \ : | 1 << CHN_GPS | 1 << CHN_BT | 1 << CHN_WIFI \ : | 1 << CHN_MCU | 1 << CHN_COANT | 1 << CHN_NFC \ : | 1 << CHN_UFS | 1 << CHN_SCP | 1 << CHN_RESERVE) : : #define INIT_SUBSYS_FPM_TO_BBLPM (1 << CHN_DEEPIDLE) : : #define INIT_SUBSYS_TO_HW (1 << CHN_SUSPEND | 1 << CHN_DEEPIDLE | 1 << CHN_MCU)
try enum
Done