* [PATCH v3 0/2] phy: qcom: qmp-pcie: Add PCIe PHY no_csr reset support @ 2025-02-14 10:45 Wenbin Yao 2025-02-14 10:45 ` [PATCH v3 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically Wenbin Yao 2025-02-14 10:45 ` [PATCH v3 2/2] phy: qcom: qmp-pcie: Add PHY register retention support Wenbin Yao 0 siblings, 2 replies; 7+ messages in thread From: Wenbin Yao @ 2025-02-14 10:45 UTC (permalink / raw) To: vkoul, kishon, p.zabel, dmitry.baryshkov, abel.vesa, quic_qianyu, neil.armstrong, manivannan.sadhasivam, quic_devipriy, konrad.dybcio, linux-arm-msm, linux-phy, linux-kernel Cc: Wenbin Yao The series aims to skip phy register programming and drive PCIe PHY with register setting programmed in bootloader by simply toggling no_csr reset, which once togglled, PHY hardware will be reset while PHY registers are retained. First, determine whether PHY setting can be skipped by checking QPHY_START_CTRL register and the existence of nocsr reset. If it is programmed and no_csr reset is supported, do no_csr reset and skip BCR reset which will reset entire PHY. This series also remove has_nocsr_reset flag in qmp_phy_cfg structure and decide whether the PHY supports nocsr reset by checking the existence of nocsr reset in device tree. The series are tested on X1E80100-QCP and HDK8550. The commit messages of this patchset have been modified based on comments and suggestions. Changes in v3: - Replace devm_reset_control_get_exclusive with devm_reset_control_get_optional_exclusive when get phy_nocsr reset control in Patch 1/2. - Do not ignore -EINVAL when get phy_nocsr reset control in Patch 1/2. - Replace phy_initialized with skip_init in struct qmp_pcie in Patch 2/2. - Add a comment to why not check qmp->skip_init in function qmp_pcie_power_off in Patch 2/2. - Link to v2: https://lore.kernel.org/all/20250211094231.1813558-1-quic_wenbyao@quicinc.com/ Changes in v2: - Add Abel's and Manivannan's Reviewed-by tag to Patch 1/2. - Refine commit msg of Patch 2/2. - Link to v1: https://lore.kernel.org/all/20250121094140.4006801-1-quic_wenbyao@quicinc.com/ Konrad Dybcio (1): phy: qcom: pcie: Determine has_nocsr_reset dynamically Qiang Yu (1): phy: qcom: qmp-pcie: Add PHY register retention support drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 113 ++++++++++++++--------- 1 file changed, 67 insertions(+), 46 deletions(-) base-commit: bcf2acd8f64b0a5783deeeb5fd70c6163ec5acd7 -- 2.34.1 -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically 2025-02-14 10:45 [PATCH v3 0/2] phy: qcom: qmp-pcie: Add PCIe PHY no_csr reset support Wenbin Yao @ 2025-02-14 10:45 ` Wenbin Yao 2025-02-14 10:49 ` Philipp Zabel 2025-02-14 10:45 ` [PATCH v3 2/2] phy: qcom: qmp-pcie: Add PHY register retention support Wenbin Yao 1 sibling, 1 reply; 7+ messages in thread From: Wenbin Yao @ 2025-02-14 10:45 UTC (permalink / raw) To: vkoul, kishon, p.zabel, dmitry.baryshkov, abel.vesa, quic_qianyu, neil.armstrong, manivannan.sadhasivam, quic_devipriy, konrad.dybcio, linux-arm-msm, linux-phy, linux-kernel Cc: Wenbin Yao From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> Decide the in-driver logic based on whether the nocsr reset is present and defer checking the appropriateness of that to dt-bindings to save on boilerplate. Reset controller APIs are fine consuming a nullptr, so no additional checks are necessary there. Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com> Reviewed-by: Abel Vesa <abel.vesa@linaro.org> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c index 873f2f9844c6..219266125cf2 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c @@ -2793,8 +2793,6 @@ struct qmp_phy_cfg { bool skip_start_delay; - bool has_nocsr_reset; - /* QMP PHY pipe clock interface rate */ unsigned long pipe_clock_rate; @@ -3685,7 +3683,6 @@ static const struct qmp_phy_cfg sm8550_qmp_gen4x2_pciephy_cfg = { .pwrdn_ctrl = SW_PWRDN | REFCLK_DRV_DSBL, .phy_status = PHYSTATUS_4_20, - .has_nocsr_reset = true, /* 20MHz PHY AUX Clock */ .aux_clock_rate = 20000000, @@ -3718,7 +3715,6 @@ static const struct qmp_phy_cfg sm8650_qmp_gen4x2_pciephy_cfg = { .pwrdn_ctrl = SW_PWRDN | REFCLK_DRV_DSBL, .phy_status = PHYSTATUS_4_20, - .has_nocsr_reset = true, /* 20MHz PHY AUX Clock */ .aux_clock_rate = 20000000, @@ -3836,7 +3832,6 @@ static const struct qmp_phy_cfg x1e80100_qmp_gen4x2_pciephy_cfg = { .pwrdn_ctrl = SW_PWRDN | REFCLK_DRV_DSBL, .phy_status = PHYSTATUS_4_20, - .has_nocsr_reset = true, }; static const struct qmp_phy_cfg x1e80100_qmp_gen4x4_pciephy_cfg = { @@ -3870,7 +3865,6 @@ static const struct qmp_phy_cfg x1e80100_qmp_gen4x4_pciephy_cfg = { .pwrdn_ctrl = SW_PWRDN | REFCLK_DRV_DSBL, .phy_status = PHYSTATUS_4_20, - .has_nocsr_reset = true, }; static const struct qmp_phy_cfg x1e80100_qmp_gen4x8_pciephy_cfg = { @@ -3902,7 +3896,6 @@ static const struct qmp_phy_cfg x1e80100_qmp_gen4x8_pciephy_cfg = { .pwrdn_ctrl = SW_PWRDN | REFCLK_DRV_DSBL, .phy_status = PHYSTATUS_4_20, - .has_nocsr_reset = true, }; static void qmp_pcie_init_port_b(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tbls *tbls) @@ -4203,12 +4196,10 @@ static int qmp_pcie_reset_init(struct qmp_pcie *qmp) if (ret) return dev_err_probe(dev, ret, "failed to get resets\n"); - if (cfg->has_nocsr_reset) { - qmp->nocsr_reset = devm_reset_control_get_exclusive(dev, "phy_nocsr"); - if (IS_ERR(qmp->nocsr_reset)) - return dev_err_probe(dev, PTR_ERR(qmp->nocsr_reset), - "failed to get no-csr reset\n"); - } + qmp->nocsr_reset = devm_reset_control_get_optional_exclusive(dev, "phy_nocsr"); + if (IS_ERR(qmp->nocsr_reset)) + return dev_err_probe(dev, PTR_ERR(qmp->nocsr_reset), + "failed to get no-csr reset\n"); return 0; } -- 2.34.1 -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically 2025-02-14 10:45 ` [PATCH v3 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically Wenbin Yao @ 2025-02-14 10:49 ` Philipp Zabel 0 siblings, 0 replies; 7+ messages in thread From: Philipp Zabel @ 2025-02-14 10:49 UTC (permalink / raw) To: Wenbin Yao, vkoul, kishon, dmitry.baryshkov, abel.vesa, quic_qianyu, neil.armstrong, manivannan.sadhasivam, quic_devipriy, konrad.dybcio, linux-arm-msm, linux-phy, linux-kernel On Fr, 2025-02-14 at 18:45 +0800, Wenbin Yao wrote: > From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> > > Decide the in-driver logic based on whether the nocsr reset is present > and defer checking the appropriateness of that to dt-bindings to save > on boilerplate. > > Reset controller APIs are fine consuming a nullptr, so no additional > checks are necessary there. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> > Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com> > Reviewed-by: Abel Vesa <abel.vesa@linaro.org> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> regards Philipp -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 2/2] phy: qcom: qmp-pcie: Add PHY register retention support 2025-02-14 10:45 [PATCH v3 0/2] phy: qcom: qmp-pcie: Add PCIe PHY no_csr reset support Wenbin Yao 2025-02-14 10:45 ` [PATCH v3 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically Wenbin Yao @ 2025-02-14 10:45 ` Wenbin Yao 2025-02-14 14:46 ` Manivannan Sadhasivam 1 sibling, 1 reply; 7+ messages in thread From: Wenbin Yao @ 2025-02-14 10:45 UTC (permalink / raw) To: vkoul, kishon, p.zabel, dmitry.baryshkov, abel.vesa, quic_qianyu, neil.armstrong, manivannan.sadhasivam, quic_devipriy, konrad.dybcio, linux-arm-msm, linux-phy, linux-kernel Cc: Wenbin Yao From: Qiang Yu <quic_qianyu@quicinc.com> Some QCOM PCIe PHYs support no_csr reset. Unlike BCR reset which resets the whole PHY (hardware and register), no_csr reset only resets PHY hardware but retains register values, which means PHY setting can be skipped during PHY init if PCIe link is enabled in booltloader and only no_csr is toggled after that. Hence, determine whether the PHY has been enabled in bootloader by verifying QPHY_START_CTRL register. If it's programmed and no_csr reset is available, skip BCR reset and PHY register setting to establish the PCIe link with bootloader - programmed PHY settings. Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com> --- drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 96 ++++++++++++++++-------- 1 file changed, 63 insertions(+), 33 deletions(-) diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c index 219266125cf2..b3912c1a6de8 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c @@ -2805,6 +2805,7 @@ struct qmp_pcie { const struct qmp_phy_cfg *cfg; bool tcsr_4ln_config; + bool skip_init; void __iomem *serdes; void __iomem *pcs; @@ -3976,7 +3977,9 @@ static int qmp_pcie_init(struct phy *phy) { struct qmp_pcie *qmp = phy_get_drvdata(phy); const struct qmp_phy_cfg *cfg = qmp->cfg; + void __iomem *pcs = qmp->pcs; int ret; + bool phy_initialized; ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs); if (ret) { @@ -3984,10 +3987,18 @@ static int qmp_pcie_init(struct phy *phy) return ret; } - ret = reset_control_bulk_assert(cfg->num_resets, qmp->resets); - if (ret) { - dev_err(qmp->dev, "reset assert failed\n"); - goto err_disable_regulators; + phy_initialized = !!(readl(pcs + cfg->regs[QPHY_START_CTRL])); + qmp->skip_init = qmp->nocsr_reset && phy_initialized; + /* + * Toggle BCR reset for phy that doesn't support no_csr + * reset or has not been initialized + */ + if (!qmp->skip_init) { + ret = reset_control_bulk_assert(cfg->num_resets, qmp->resets); + if (ret) { + dev_err(qmp->dev, "reset assert failed\n"); + goto err_disable_regulators; + } } ret = reset_control_assert(qmp->nocsr_reset); @@ -3998,10 +4009,12 @@ static int qmp_pcie_init(struct phy *phy) usleep_range(200, 300); - ret = reset_control_bulk_deassert(cfg->num_resets, qmp->resets); - if (ret) { - dev_err(qmp->dev, "reset deassert failed\n"); - goto err_assert_reset; + if (!qmp->skip_init) { + ret = reset_control_bulk_deassert(cfg->num_resets, qmp->resets); + if (ret) { + dev_err(qmp->dev, "reset deassert failed\n"); + goto err_assert_reset; + } } ret = clk_bulk_prepare_enable(ARRAY_SIZE(qmp_pciephy_clk_l), qmp->clks); @@ -4011,7 +4024,8 @@ static int qmp_pcie_init(struct phy *phy) return 0; err_assert_reset: - reset_control_bulk_assert(cfg->num_resets, qmp->resets); + if (!qmp->skip_init) + reset_control_bulk_assert(cfg->num_resets, qmp->resets); err_disable_regulators: regulator_bulk_disable(cfg->num_vregs, qmp->vregs); @@ -4023,7 +4037,10 @@ static int qmp_pcie_exit(struct phy *phy) struct qmp_pcie *qmp = phy_get_drvdata(phy); const struct qmp_phy_cfg *cfg = qmp->cfg; - reset_control_bulk_assert(cfg->num_resets, qmp->resets); + if (!qmp->nocsr_reset) + reset_control_bulk_assert(cfg->num_resets, qmp->resets); + else + reset_control_assert(qmp->nocsr_reset); clk_bulk_disable_unprepare(ARRAY_SIZE(qmp_pciephy_clk_l), qmp->clks); @@ -4042,16 +4059,22 @@ static int qmp_pcie_power_on(struct phy *phy) unsigned int mask, val; int ret; - qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], - cfg->pwrdn_ctrl); + /* + * Write CSR register for phy that doesn't support no_csr + * reset or has not been initialized + */ + if (!qmp->skip_init) { + qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], + cfg->pwrdn_ctrl); - if (qmp->mode == PHY_MODE_PCIE_RC) - mode_tbls = cfg->tbls_rc; - else - mode_tbls = cfg->tbls_ep; + if (qmp->mode == PHY_MODE_PCIE_RC) + mode_tbls = cfg->tbls_rc; + else + mode_tbls = cfg->tbls_ep; - qmp_pcie_init_registers(qmp, &cfg->tbls); - qmp_pcie_init_registers(qmp, mode_tbls); + qmp_pcie_init_registers(qmp, &cfg->tbls); + qmp_pcie_init_registers(qmp, mode_tbls); + } ret = clk_bulk_prepare_enable(qmp->num_pipe_clks, qmp->pipe_clks); if (ret) @@ -4063,15 +4086,16 @@ static int qmp_pcie_power_on(struct phy *phy) goto err_disable_pipe_clk; } - /* Pull PHY out of reset state */ - qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); + if (!qmp->skip_init) { + /* Pull PHY out of reset state */ + qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); - /* start SerDes and Phy-Coding-Sublayer */ - qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], SERDES_START | PCS_START); - - if (!cfg->skip_start_delay) - usleep_range(1000, 1200); + /* start SerDes and Phy-Coding-Sublayer */ + qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], SERDES_START | PCS_START); + if (!cfg->skip_start_delay) + usleep_range(1000, 1200); + } status = pcs + cfg->regs[QPHY_PCS_STATUS]; mask = cfg->phy_status; ret = readl_poll_timeout(status, val, !(val & mask), 200, @@ -4096,16 +4120,22 @@ static int qmp_pcie_power_off(struct phy *phy) clk_bulk_disable_unprepare(qmp->num_pipe_clks, qmp->pipe_clks); - /* PHY reset */ - qphy_setbits(qmp->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); + /* If a PHY supports no_csr reset but isn't enabled in the bootloader, + * phy settings can be reused during the D3cold -> D0 cycle. So it is + * unnecessary to check qmp->skip_init. + */ + if (!qmp->nocsr_reset) { + /* PHY reset */ + qphy_setbits(qmp->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); - /* stop SerDes and Phy-Coding-Sublayer */ - qphy_clrbits(qmp->pcs, cfg->regs[QPHY_START_CTRL], - SERDES_START | PCS_START); + /* stop SerDes and Phy-Coding-Sublayer */ + qphy_clrbits(qmp->pcs, cfg->regs[QPHY_START_CTRL], + SERDES_START | PCS_START); - /* Put PHY into POWER DOWN state: active low */ - qphy_clrbits(qmp->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], - cfg->pwrdn_ctrl); + /* Put PHY into POWER DOWN state: active low */ + qphy_clrbits(qmp->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], + cfg->pwrdn_ctrl); + } return 0; } -- 2.34.1 -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/2] phy: qcom: qmp-pcie: Add PHY register retention support 2025-02-14 10:45 ` [PATCH v3 2/2] phy: qcom: qmp-pcie: Add PHY register retention support Wenbin Yao @ 2025-02-14 14:46 ` Manivannan Sadhasivam 2025-02-17 10:05 ` Qiang Yu 2025-02-17 10:08 ` Wenbin Yao (Consultant) 0 siblings, 2 replies; 7+ messages in thread From: Manivannan Sadhasivam @ 2025-02-14 14:46 UTC (permalink / raw) To: Wenbin Yao Cc: vkoul, kishon, p.zabel, dmitry.baryshkov, abel.vesa, quic_qianyu, neil.armstrong, manivannan.sadhasivam, quic_devipriy, konrad.dybcio, linux-arm-msm, linux-phy, linux-kernel On Fri, Feb 14, 2025 at 06:45:39PM +0800, Wenbin Yao wrote: > From: Qiang Yu <quic_qianyu@quicinc.com> > > Some QCOM PCIe PHYs support no_csr reset. Unlike BCR reset which resets the > whole PHY (hardware and register), no_csr reset only resets PHY hardware > but retains register values, which means PHY setting can be skipped during > PHY init if PCIe link is enabled in booltloader and only no_csr is toggled > after that. > > Hence, determine whether the PHY has been enabled in bootloader by > verifying QPHY_START_CTRL register. If it's programmed and no_csr reset is > available, skip BCR reset and PHY register setting to establish the PCIe > link with bootloader - programmed PHY settings. > > Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com> > Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com> > --- > drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 96 ++++++++++++++++-------- > 1 file changed, 63 insertions(+), 33 deletions(-) > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > index 219266125cf2..b3912c1a6de8 100644 > --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > @@ -2805,6 +2805,7 @@ struct qmp_pcie { > > const struct qmp_phy_cfg *cfg; > bool tcsr_4ln_config; > + bool skip_init; > > void __iomem *serdes; > void __iomem *pcs; > @@ -3976,7 +3977,9 @@ static int qmp_pcie_init(struct phy *phy) > { > struct qmp_pcie *qmp = phy_get_drvdata(phy); > const struct qmp_phy_cfg *cfg = qmp->cfg; > + void __iomem *pcs = qmp->pcs; > int ret; > + bool phy_initialized; > > ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs); > if (ret) { > @@ -3984,10 +3987,18 @@ static int qmp_pcie_init(struct phy *phy) > return ret; > } > > - ret = reset_control_bulk_assert(cfg->num_resets, qmp->resets); > - if (ret) { > - dev_err(qmp->dev, "reset assert failed\n"); > - goto err_disable_regulators; > + phy_initialized = !!(readl(pcs + cfg->regs[QPHY_START_CTRL])); > + qmp->skip_init = qmp->nocsr_reset && phy_initialized; > + /* > + * Toggle BCR reset for phy that doesn't support no_csr s/phy/PHY. Here and below. > + * reset or has not been initialized > + */ > + if (!qmp->skip_init) { > + ret = reset_control_bulk_assert(cfg->num_resets, qmp->resets); > + if (ret) { > + dev_err(qmp->dev, "reset assert failed\n"); > + goto err_disable_regulators; > + } > } > > ret = reset_control_assert(qmp->nocsr_reset); > @@ -3998,10 +4009,12 @@ static int qmp_pcie_init(struct phy *phy) > > usleep_range(200, 300); > > - ret = reset_control_bulk_deassert(cfg->num_resets, qmp->resets); > - if (ret) { > - dev_err(qmp->dev, "reset deassert failed\n"); > - goto err_assert_reset; > + if (!qmp->skip_init) { > + ret = reset_control_bulk_deassert(cfg->num_resets, qmp->resets); > + if (ret) { > + dev_err(qmp->dev, "reset deassert failed\n"); > + goto err_assert_reset; > + } > } > > ret = clk_bulk_prepare_enable(ARRAY_SIZE(qmp_pciephy_clk_l), qmp->clks); > @@ -4011,7 +4024,8 @@ static int qmp_pcie_init(struct phy *phy) > return 0; > > err_assert_reset: > - reset_control_bulk_assert(cfg->num_resets, qmp->resets); > + if (!qmp->skip_init) > + reset_control_bulk_assert(cfg->num_resets, qmp->resets); > err_disable_regulators: > regulator_bulk_disable(cfg->num_vregs, qmp->vregs); > > @@ -4023,7 +4037,10 @@ static int qmp_pcie_exit(struct phy *phy) > struct qmp_pcie *qmp = phy_get_drvdata(phy); > const struct qmp_phy_cfg *cfg = qmp->cfg; > > - reset_control_bulk_assert(cfg->num_resets, qmp->resets); > + if (!qmp->nocsr_reset) > + reset_control_bulk_assert(cfg->num_resets, qmp->resets); > + else > + reset_control_assert(qmp->nocsr_reset); I'd flip the if condition for readability: if (qmp->nocsr_reset) ... else ... > > clk_bulk_disable_unprepare(ARRAY_SIZE(qmp_pciephy_clk_l), qmp->clks); > > @@ -4042,16 +4059,22 @@ static int qmp_pcie_power_on(struct phy *phy) > unsigned int mask, val; > int ret; > > - qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], > - cfg->pwrdn_ctrl); > + /* > + * Write CSR register for phy that doesn't support no_csr > + * reset or has not been initialized > + */ > + if (!qmp->skip_init) { How about: if (qmp->skip_init) goto skip_phy_init; This is my personal preference btw. If anyone feels the other way, feel free to drop this suggestion. > + qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], > + cfg->pwrdn_ctrl); > > - if (qmp->mode == PHY_MODE_PCIE_RC) > - mode_tbls = cfg->tbls_rc; > - else > - mode_tbls = cfg->tbls_ep; > + if (qmp->mode == PHY_MODE_PCIE_RC) > + mode_tbls = cfg->tbls_rc; > + else > + mode_tbls = cfg->tbls_ep; > > - qmp_pcie_init_registers(qmp, &cfg->tbls); > - qmp_pcie_init_registers(qmp, mode_tbls); > + qmp_pcie_init_registers(qmp, &cfg->tbls); > + qmp_pcie_init_registers(qmp, mode_tbls); > + } > > ret = clk_bulk_prepare_enable(qmp->num_pipe_clks, qmp->pipe_clks); > if (ret) > @@ -4063,15 +4086,16 @@ static int qmp_pcie_power_on(struct phy *phy) > goto err_disable_pipe_clk; > } > > - /* Pull PHY out of reset state */ > - qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); > + if (!qmp->skip_init) { if (qmp->skip_init) goto skip_serdes_start; > + /* Pull PHY out of reset state */ > + qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); > > - /* start SerDes and Phy-Coding-Sublayer */ > - qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], SERDES_START | PCS_START); > - > - if (!cfg->skip_start_delay) > - usleep_range(1000, 1200); > + /* start SerDes and Phy-Coding-Sublayer */ > + qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], SERDES_START | PCS_START); > > + if (!cfg->skip_start_delay) > + usleep_range(1000, 1200); > + } > status = pcs + cfg->regs[QPHY_PCS_STATUS]; > mask = cfg->phy_status; > ret = readl_poll_timeout(status, val, !(val & mask), 200, > @@ -4096,16 +4120,22 @@ static int qmp_pcie_power_off(struct phy *phy) > > clk_bulk_disable_unprepare(qmp->num_pipe_clks, qmp->pipe_clks); > > - /* PHY reset */ > - qphy_setbits(qmp->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); > + /* If a PHY supports no_csr reset but isn't enabled in the bootloader, > + * phy settings can be reused during the D3cold -> D0 cycle. So it is I cannot parse this sentence. If PHY is not initialized, how can you reuse the settings? Also what is the D3cold->D0 reference? > + * unnecessary to check qmp->skip_init. > + */ > + if (!qmp->nocsr_reset) { if (qmp->nocsr_reset) goto skip_phy_reset; - Mani -- மணிவண்ணன் சதாசிவம் -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/2] phy: qcom: qmp-pcie: Add PHY register retention support 2025-02-14 14:46 ` Manivannan Sadhasivam @ 2025-02-17 10:05 ` Qiang Yu 2025-02-17 10:08 ` Wenbin Yao (Consultant) 1 sibling, 0 replies; 7+ messages in thread From: Qiang Yu @ 2025-02-17 10:05 UTC (permalink / raw) To: Manivannan Sadhasivam, Wenbin Yao Cc: vkoul, kishon, p.zabel, dmitry.baryshkov, abel.vesa, neil.armstrong, manivannan.sadhasivam, quic_devipriy, konrad.dybcio, linux-arm-msm, linux-phy, linux-kernel On 2/14/2025 10:46 PM, Manivannan Sadhasivam wrote: > On Fri, Feb 14, 2025 at 06:45:39PM +0800, Wenbin Yao wrote: >> From: Qiang Yu <quic_qianyu@quicinc.com> >> >> Some QCOM PCIe PHYs support no_csr reset. Unlike BCR reset which resets the >> whole PHY (hardware and register), no_csr reset only resets PHY hardware >> but retains register values, which means PHY setting can be skipped during >> PHY init if PCIe link is enabled in booltloader and only no_csr is toggled >> after that. >> >> Hence, determine whether the PHY has been enabled in bootloader by >> verifying QPHY_START_CTRL register. If it's programmed and no_csr reset is >> available, skip BCR reset and PHY register setting to establish the PCIe >> link with bootloader - programmed PHY settings. >> >> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com> >> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com> >> --- >> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 96 ++++++++++++++++-------- >> 1 file changed, 63 insertions(+), 33 deletions(-) >> >> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c >> index 219266125cf2..b3912c1a6de8 100644 >> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c >> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c >> @@ -2805,6 +2805,7 @@ struct qmp_pcie { >> >> const struct qmp_phy_cfg *cfg; >> bool tcsr_4ln_config; >> + bool skip_init; >> >> void __iomem *serdes; >> void __iomem *pcs; >> @@ -3976,7 +3977,9 @@ static int qmp_pcie_init(struct phy *phy) >> { >> struct qmp_pcie *qmp = phy_get_drvdata(phy); >> const struct qmp_phy_cfg *cfg = qmp->cfg; >> + void __iomem *pcs = qmp->pcs; >> int ret; >> + bool phy_initialized; >> >> ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs); >> if (ret) { >> @@ -3984,10 +3987,18 @@ static int qmp_pcie_init(struct phy *phy) >> return ret; >> } >> >> - ret = reset_control_bulk_assert(cfg->num_resets, qmp->resets); >> - if (ret) { >> - dev_err(qmp->dev, "reset assert failed\n"); >> - goto err_disable_regulators; >> + phy_initialized = !!(readl(pcs + cfg->regs[QPHY_START_CTRL])); >> + qmp->skip_init = qmp->nocsr_reset && phy_initialized; >> + /* >> + * Toggle BCR reset for phy that doesn't support no_csr > s/phy/PHY. Here and below. > >> + * reset or has not been initialized >> + */ >> + if (!qmp->skip_init) { >> + ret = reset_control_bulk_assert(cfg->num_resets, qmp->resets); >> + if (ret) { >> + dev_err(qmp->dev, "reset assert failed\n"); >> + goto err_disable_regulators; >> + } >> } >> >> ret = reset_control_assert(qmp->nocsr_reset); >> @@ -3998,10 +4009,12 @@ static int qmp_pcie_init(struct phy *phy) >> >> usleep_range(200, 300); >> >> - ret = reset_control_bulk_deassert(cfg->num_resets, qmp->resets); >> - if (ret) { >> - dev_err(qmp->dev, "reset deassert failed\n"); >> - goto err_assert_reset; >> + if (!qmp->skip_init) { >> + ret = reset_control_bulk_deassert(cfg->num_resets, qmp->resets); >> + if (ret) { >> + dev_err(qmp->dev, "reset deassert failed\n"); >> + goto err_assert_reset; >> + } >> } >> >> ret = clk_bulk_prepare_enable(ARRAY_SIZE(qmp_pciephy_clk_l), qmp->clks); >> @@ -4011,7 +4024,8 @@ static int qmp_pcie_init(struct phy *phy) >> return 0; >> >> err_assert_reset: >> - reset_control_bulk_assert(cfg->num_resets, qmp->resets); >> + if (!qmp->skip_init) >> + reset_control_bulk_assert(cfg->num_resets, qmp->resets); >> err_disable_regulators: >> regulator_bulk_disable(cfg->num_vregs, qmp->vregs); >> >> @@ -4023,7 +4037,10 @@ static int qmp_pcie_exit(struct phy *phy) >> struct qmp_pcie *qmp = phy_get_drvdata(phy); >> const struct qmp_phy_cfg *cfg = qmp->cfg; >> >> - reset_control_bulk_assert(cfg->num_resets, qmp->resets); >> + if (!qmp->nocsr_reset) >> + reset_control_bulk_assert(cfg->num_resets, qmp->resets); >> + else >> + reset_control_assert(qmp->nocsr_reset); > I'd flip the if condition for readability: > > if (qmp->nocsr_reset) > ... > else > ... >> >> clk_bulk_disable_unprepare(ARRAY_SIZE(qmp_pciephy_clk_l), qmp->clks); >> >> @@ -4042,16 +4059,22 @@ static int qmp_pcie_power_on(struct phy *phy) >> unsigned int mask, val; >> int ret; >> >> - qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], >> - cfg->pwrdn_ctrl); >> + /* >> + * Write CSR register for phy that doesn't support no_csr >> + * reset or has not been initialized >> + */ >> + if (!qmp->skip_init) { > How about: > if (qmp->skip_init) > goto skip_phy_init; > > This is my personal preference btw. If anyone feels the other way, feel free > to drop this suggestion. > >> + qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], >> + cfg->pwrdn_ctrl); >> >> - if (qmp->mode == PHY_MODE_PCIE_RC) >> - mode_tbls = cfg->tbls_rc; >> - else >> - mode_tbls = cfg->tbls_ep; >> + if (qmp->mode == PHY_MODE_PCIE_RC) >> + mode_tbls = cfg->tbls_rc; >> + else >> + mode_tbls = cfg->tbls_ep; >> >> - qmp_pcie_init_registers(qmp, &cfg->tbls); >> - qmp_pcie_init_registers(qmp, mode_tbls); >> + qmp_pcie_init_registers(qmp, &cfg->tbls); >> + qmp_pcie_init_registers(qmp, mode_tbls); >> + } >> >> ret = clk_bulk_prepare_enable(qmp->num_pipe_clks, qmp->pipe_clks); >> if (ret) >> @@ -4063,15 +4086,16 @@ static int qmp_pcie_power_on(struct phy *phy) >> goto err_disable_pipe_clk; >> } >> >> - /* Pull PHY out of reset state */ >> - qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); >> + if (!qmp->skip_init) { > if (qmp->skip_init) > goto skip_serdes_start; > >> + /* Pull PHY out of reset state */ >> + qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); >> >> - /* start SerDes and Phy-Coding-Sublayer */ >> - qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], SERDES_START | PCS_START); >> - >> - if (!cfg->skip_start_delay) >> - usleep_range(1000, 1200); >> + /* start SerDes and Phy-Coding-Sublayer */ >> + qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], SERDES_START | PCS_START); >> >> + if (!cfg->skip_start_delay) >> + usleep_range(1000, 1200); >> + } >> status = pcs + cfg->regs[QPHY_PCS_STATUS]; >> mask = cfg->phy_status; >> ret = readl_poll_timeout(status, val, !(val & mask), 200, >> @@ -4096,16 +4120,22 @@ static int qmp_pcie_power_off(struct phy *phy) >> >> clk_bulk_disable_unprepare(qmp->num_pipe_clks, qmp->pipe_clks); >> >> - /* PHY reset */ >> - qphy_setbits(qmp->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); >> + /* If a PHY supports no_csr reset but isn't enabled in the bootloader, >> + * phy settings can be reused during the D3cold -> D0 cycle. So it is > I cannot parse this sentence. If PHY is not initialized, how can you reuse the > settings? Also what is the D3cold->D0 reference? If PHY is not initialized, PHY settings will not be reused next time PHY is powered on as !!(readl(pcs + cfg->regs[QPHY_START_CTRL])) is false. For PHY driver, D3cold->D0 cycle means PHY power off -> power on. The comment is not very clear, perhaps we can use this comment: During PHY is powered off, only qmp->nocsr_reset need to be checked. In this way, no matter whether the PHY settings were initially programmed by bootloader or PHY driver itself, we can reuse them when PHY is powered on next time. Thanks, Qiang > >> + * unnecessary to check qmp->skip_init. >> + */ >> + if (!qmp->nocsr_reset) { > if (qmp->nocsr_reset) > goto skip_phy_reset; > > - Mani > -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/2] phy: qcom: qmp-pcie: Add PHY register retention support 2025-02-14 14:46 ` Manivannan Sadhasivam 2025-02-17 10:05 ` Qiang Yu @ 2025-02-17 10:08 ` Wenbin Yao (Consultant) 1 sibling, 0 replies; 7+ messages in thread From: Wenbin Yao (Consultant) @ 2025-02-17 10:08 UTC (permalink / raw) To: Manivannan Sadhasivam Cc: vkoul, kishon, p.zabel, dmitry.baryshkov, abel.vesa, quic_qianyu, neil.armstrong, manivannan.sadhasivam, quic_devipriy, konrad.dybcio, linux-arm-msm, linux-phy, linux-kernel On 2/14/2025 10:46 PM, Manivannan Sadhasivam wrote: > On Fri, Feb 14, 2025 at 06:45:39PM +0800, Wenbin Yao wrote: >> From: Qiang Yu <quic_qianyu@quicinc.com> >> >> Some QCOM PCIe PHYs support no_csr reset. Unlike BCR reset which resets the >> whole PHY (hardware and register), no_csr reset only resets PHY hardware >> but retains register values, which means PHY setting can be skipped during >> PHY init if PCIe link is enabled in booltloader and only no_csr is toggled >> after that. >> >> Hence, determine whether the PHY has been enabled in bootloader by >> verifying QPHY_START_CTRL register. If it's programmed and no_csr reset is >> available, skip BCR reset and PHY register setting to establish the PCIe >> link with bootloader - programmed PHY settings. >> >> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com> >> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com> >> --- >> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 96 ++++++++++++++++-------- >> 1 file changed, 63 insertions(+), 33 deletions(-) >> >> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c >> index 219266125cf2..b3912c1a6de8 100644 >> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c >> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c >> @@ -2805,6 +2805,7 @@ struct qmp_pcie { >> >> const struct qmp_phy_cfg *cfg; >> bool tcsr_4ln_config; >> + bool skip_init; >> >> void __iomem *serdes; >> void __iomem *pcs; >> @@ -3976,7 +3977,9 @@ static int qmp_pcie_init(struct phy *phy) >> { >> struct qmp_pcie *qmp = phy_get_drvdata(phy); >> const struct qmp_phy_cfg *cfg = qmp->cfg; >> + void __iomem *pcs = qmp->pcs; >> int ret; >> + bool phy_initialized; >> >> ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs); >> if (ret) { >> @@ -3984,10 +3987,18 @@ static int qmp_pcie_init(struct phy *phy) >> return ret; >> } >> >> - ret = reset_control_bulk_assert(cfg->num_resets, qmp->resets); >> - if (ret) { >> - dev_err(qmp->dev, "reset assert failed\n"); >> - goto err_disable_regulators; >> + phy_initialized = !!(readl(pcs + cfg->regs[QPHY_START_CTRL])); >> + qmp->skip_init = qmp->nocsr_reset && phy_initialized; >> + /* >> + * Toggle BCR reset for phy that doesn't support no_csr > s/phy/PHY. Here and below. Will use PHY instead. > >> + * reset or has not been initialized >> + */ >> + if (!qmp->skip_init) { >> + ret = reset_control_bulk_assert(cfg->num_resets, qmp->resets); >> + if (ret) { >> + dev_err(qmp->dev, "reset assert failed\n"); >> + goto err_disable_regulators; >> + } >> } >> >> ret = reset_control_assert(qmp->nocsr_reset); >> @@ -3998,10 +4009,12 @@ static int qmp_pcie_init(struct phy *phy) >> >> usleep_range(200, 300); >> >> - ret = reset_control_bulk_deassert(cfg->num_resets, qmp->resets); >> - if (ret) { >> - dev_err(qmp->dev, "reset deassert failed\n"); >> - goto err_assert_reset; >> + if (!qmp->skip_init) { >> + ret = reset_control_bulk_deassert(cfg->num_resets, qmp->resets); >> + if (ret) { >> + dev_err(qmp->dev, "reset deassert failed\n"); >> + goto err_assert_reset; >> + } >> } >> >> ret = clk_bulk_prepare_enable(ARRAY_SIZE(qmp_pciephy_clk_l), qmp->clks); >> @@ -4011,7 +4024,8 @@ static int qmp_pcie_init(struct phy *phy) >> return 0; >> >> err_assert_reset: >> - reset_control_bulk_assert(cfg->num_resets, qmp->resets); >> + if (!qmp->skip_init) >> + reset_control_bulk_assert(cfg->num_resets, qmp->resets); >> err_disable_regulators: >> regulator_bulk_disable(cfg->num_vregs, qmp->vregs); >> >> @@ -4023,7 +4037,10 @@ static int qmp_pcie_exit(struct phy *phy) >> struct qmp_pcie *qmp = phy_get_drvdata(phy); >> const struct qmp_phy_cfg *cfg = qmp->cfg; >> >> - reset_control_bulk_assert(cfg->num_resets, qmp->resets); >> + if (!qmp->nocsr_reset) >> + reset_control_bulk_assert(cfg->num_resets, qmp->resets); >> + else >> + reset_control_assert(qmp->nocsr_reset); > I'd flip the if condition for readability: > > if (qmp->nocsr_reset) > ... > else > ... Will flip the if condition. >> >> clk_bulk_disable_unprepare(ARRAY_SIZE(qmp_pciephy_clk_l), qmp->clks); >> >> @@ -4042,16 +4059,22 @@ static int qmp_pcie_power_on(struct phy *phy) >> unsigned int mask, val; >> int ret; >> >> - qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], >> - cfg->pwrdn_ctrl); >> + /* >> + * Write CSR register for phy that doesn't support no_csr >> + * reset or has not been initialized >> + */ >> + if (!qmp->skip_init) { > How about: > if (qmp->skip_init) > goto skip_phy_init; > > This is my personal preference btw. If anyone feels the other way, feel free > to drop this suggestion. Will use goto statements. > >> + qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], >> + cfg->pwrdn_ctrl); >> >> - if (qmp->mode == PHY_MODE_PCIE_RC) >> - mode_tbls = cfg->tbls_rc; >> - else >> - mode_tbls = cfg->tbls_ep; >> + if (qmp->mode == PHY_MODE_PCIE_RC) >> + mode_tbls = cfg->tbls_rc; >> + else >> + mode_tbls = cfg->tbls_ep; >> >> - qmp_pcie_init_registers(qmp, &cfg->tbls); >> - qmp_pcie_init_registers(qmp, mode_tbls); >> + qmp_pcie_init_registers(qmp, &cfg->tbls); >> + qmp_pcie_init_registers(qmp, mode_tbls); >> + } >> >> ret = clk_bulk_prepare_enable(qmp->num_pipe_clks, qmp->pipe_clks); >> if (ret) >> @@ -4063,15 +4086,16 @@ static int qmp_pcie_power_on(struct phy *phy) >> goto err_disable_pipe_clk; >> } >> >> - /* Pull PHY out of reset state */ >> - qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); >> + if (!qmp->skip_init) { > if (qmp->skip_init) > goto skip_serdes_start; Will use goto statements. > >> + /* Pull PHY out of reset state */ >> + qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); >> >> - /* start SerDes and Phy-Coding-Sublayer */ >> - qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], SERDES_START | PCS_START); >> - >> - if (!cfg->skip_start_delay) >> - usleep_range(1000, 1200); >> + /* start SerDes and Phy-Coding-Sublayer */ >> + qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], SERDES_START | PCS_START); >> >> + if (!cfg->skip_start_delay) >> + usleep_range(1000, 1200); >> + } >> status = pcs + cfg->regs[QPHY_PCS_STATUS]; >> mask = cfg->phy_status; >> ret = readl_poll_timeout(status, val, !(val & mask), 200, >> @@ -4096,16 +4120,22 @@ static int qmp_pcie_power_off(struct phy *phy) >> >> clk_bulk_disable_unprepare(qmp->num_pipe_clks, qmp->pipe_clks); >> >> - /* PHY reset */ >> - qphy_setbits(qmp->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); >> + /* If a PHY supports no_csr reset but isn't enabled in the bootloader, >> + * phy settings can be reused during the D3cold -> D0 cycle. So it is > I cannot parse this sentence. If PHY is not initialized, how can you reuse the > settings? Also what is the D3cold->D0 reference? > >> + * unnecessary to check qmp->skip_init. >> + */ >> + if (!qmp->nocsr_reset) { > if (qmp->nocsr_reset) > goto skip_phy_reset; > > - Mani > -- With best wishes Wenbin -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-02-17 10:53 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-02-14 10:45 [PATCH v3 0/2] phy: qcom: qmp-pcie: Add PCIe PHY no_csr reset support Wenbin Yao 2025-02-14 10:45 ` [PATCH v3 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically Wenbin Yao 2025-02-14 10:49 ` Philipp Zabel 2025-02-14 10:45 ` [PATCH v3 2/2] phy: qcom: qmp-pcie: Add PHY register retention support Wenbin Yao 2025-02-14 14:46 ` Manivannan Sadhasivam 2025-02-17 10:05 ` Qiang Yu 2025-02-17 10:08 ` Wenbin Yao (Consultant)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox