Hung-Te Lin 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 50:
(9 comments)
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/in... File src/soc/mediatek/mt8192/include/soc/clkbuf.h:
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/in... 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?
Or at least change these to enum.
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/in... File src/soc/mediatek/mt8192/include/soc/srclken_rc.h:
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/in... PS50, Line 6: (0) replace this by enum?
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/in... PS50, Line 7: #if !MTK_SRCLKEN_RC_BRINGUP : #define MTK_SRCLKEN_RC_SUPPORT : #endif I'd prefer a Kconfig for this (and only do the logic in *.c files, not having any #if/define in header files), for example CONFIG_MTK_SRCLKEN_RC_BRINGUP or _DEBUG or _PRODUCTION etc.
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/in... PS50, Line 10: (1) replace by enum
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/in... 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?
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/pm... File src/soc/mediatek/mt8192/pmif.c:
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/pm... 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,
int sleep_request, pmic_rdy, rc_rw,....;
switch (mode) { case PMIF_VLD_RDY: sleep_request = 1; pmic_rdy = 1; ... break; case ... break; default: return; }
SET32_BITFIELDS(pmif_spi_arb[0].mtk_pmif->sleep_protection_ctrl, PMIFSPI_SPM_SLEEP_REQ_SEL, sleep_request, PMIFSPI_SCP_SLEEP_REQ_SEL, sleep_request);
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/pm... 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(....))
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/sr... File src/soc/mediatek/mt8192/srclken_rc.c:
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/sr... 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
https://review.coreboot.org/c/coreboot/+/46878/50/src/soc/mediatek/mt8192/sr... 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