* [PATCH 0/2] phy: qcom: qmp-pcie: Add PCIe PHY no_csr reset support @ 2025-01-21 9:41 Wenbin Yao 2025-01-21 9:41 ` [PATCH 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically Wenbin Yao 2025-01-21 9:41 ` [PATCH 2/2] phy: qcom: qmp-pcie: Add PHY register retention support Wenbin Yao 0 siblings, 2 replies; 17+ messages in thread From: Wenbin Yao @ 2025-01-21 9:41 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: quic_wenbyao 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. Wenbin Yao (2): phy: qcom: pcie: Determine has_nocsr_reset dynamically phy: qcom: qmp-pcie: Add PHY register retention support drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 109 ++++++++++++++--------- 1 file changed, 65 insertions(+), 44 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] 17+ messages in thread
* [PATCH 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically 2025-01-21 9:41 [PATCH 0/2] phy: qcom: qmp-pcie: Add PCIe PHY no_csr reset support Wenbin Yao @ 2025-01-21 9:41 ` Wenbin Yao 2025-01-21 9:55 ` Abel Vesa 2025-01-24 7:10 ` Manivannan Sadhasivam 2025-01-21 9:41 ` [PATCH 2/2] phy: qcom: qmp-pcie: Add PHY register retention support Wenbin Yao 1 sibling, 2 replies; 17+ messages in thread From: Wenbin Yao @ 2025-01-21 9:41 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: quic_wenbyao 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> --- drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c index 873f2f9844c6..ac42e4b01065 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,11 +4196,14 @@ 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)) + qmp->nocsr_reset = devm_reset_control_get_exclusive(dev, "phy_nocsr"); + if (IS_ERR(qmp->nocsr_reset)) { + if (PTR_ERR(qmp->nocsr_reset) == -ENOENT || + PTR_ERR(qmp->nocsr_reset) == -EINVAL) + qmp->nocsr_reset = NULL; + else return dev_err_probe(dev, PTR_ERR(qmp->nocsr_reset), - "failed to get no-csr reset\n"); + "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] 17+ messages in thread
* Re: [PATCH 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically 2025-01-21 9:41 ` [PATCH 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically Wenbin Yao @ 2025-01-21 9:55 ` Abel Vesa 2025-01-24 7:10 ` Manivannan Sadhasivam 1 sibling, 0 replies; 17+ messages in thread From: Abel Vesa @ 2025-01-21 9:55 UTC (permalink / raw) To: Wenbin Yao Cc: vkoul, kishon, p.zabel, dmitry.baryshkov, quic_qianyu, neil.armstrong, manivannan.sadhasivam, quic_devipriy, konrad.dybcio, linux-arm-msm, linux-phy, linux-kernel On 25-01-21 17:41:39, 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> > --- > drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 18 +++++++----------- > 1 file changed, 7 insertions(+), 11 deletions(-) > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > index 873f2f9844c6..ac42e4b01065 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,11 +4196,14 @@ 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)) > + qmp->nocsr_reset = devm_reset_control_get_exclusive(dev, "phy_nocsr"); > + if (IS_ERR(qmp->nocsr_reset)) { > + if (PTR_ERR(qmp->nocsr_reset) == -ENOENT || > + PTR_ERR(qmp->nocsr_reset) == -EINVAL) > + qmp->nocsr_reset = NULL; > + else > return dev_err_probe(dev, PTR_ERR(qmp->nocsr_reset), > - "failed to get no-csr reset\n"); > + "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 [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically 2025-01-21 9:41 ` [PATCH 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically Wenbin Yao 2025-01-21 9:55 ` Abel Vesa @ 2025-01-24 7:10 ` Manivannan Sadhasivam 1 sibling, 0 replies; 17+ messages in thread From: Manivannan Sadhasivam @ 2025-01-24 7:10 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 Tue, Jan 21, 2025 at 05:41:39PM +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: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani > --- > drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 18 +++++++----------- > 1 file changed, 7 insertions(+), 11 deletions(-) > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > index 873f2f9844c6..ac42e4b01065 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,11 +4196,14 @@ 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)) > + qmp->nocsr_reset = devm_reset_control_get_exclusive(dev, "phy_nocsr"); > + if (IS_ERR(qmp->nocsr_reset)) { > + if (PTR_ERR(qmp->nocsr_reset) == -ENOENT || > + PTR_ERR(qmp->nocsr_reset) == -EINVAL) > + qmp->nocsr_reset = NULL; > + else > return dev_err_probe(dev, PTR_ERR(qmp->nocsr_reset), > - "failed to get no-csr reset\n"); > + "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 [flat|nested] 17+ messages in thread
* [PATCH 2/2] phy: qcom: qmp-pcie: Add PHY register retention support 2025-01-21 9:41 [PATCH 0/2] phy: qcom: qmp-pcie: Add PCIe PHY no_csr reset support Wenbin Yao 2025-01-21 9:41 ` [PATCH 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically Wenbin Yao @ 2025-01-21 9:41 ` Wenbin Yao 2025-01-21 10:36 ` Dmitry Baryshkov 1 sibling, 1 reply; 17+ messages in thread From: Wenbin Yao @ 2025-01-21 9:41 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: quic_wenbyao From: Qiang Yu <quic_qianyu@quicinc.com> Currently, BCR reset and PHY register setting are mandatory for every port before link training. However, some QCOM PCIe PHYs support no_csr reset. Different than BCR reset that is used to reset entire PHY including hardware and register, once no_csr reset is toggled, only PHY hardware will be reset but PHY registers will be retained, which means PHY setting can be skipped during PHY init if PCIe link was 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 is programmed and no_csr reset is present, skip BCR reset and PHY register setting, so that PCIe link can be established with no_csr reset only. 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 | 91 +++++++++++++++--------- 1 file changed, 58 insertions(+), 33 deletions(-) diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c index ac42e4b01065..7f0802d09812 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 phy_initialized; void __iomem *serdes; void __iomem *pcs; @@ -3976,6 +3977,7 @@ 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; ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs); @@ -3984,10 +3986,17 @@ 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; + qmp->phy_initialized = !!(readl(pcs + cfg->regs[QPHY_START_CTRL])); + /* + * Toggle BCR reset for phy that doesn't support no_csr + * reset or has not been initialized + */ + if (!qmp->nocsr_reset || !qmp->phy_initialized) { + 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 +4007,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->nocsr_reset || !qmp->phy_initialized) { + 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 +4022,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->nocsr_reset || !qmp->phy_initialized) + reset_control_bulk_assert(cfg->num_resets, qmp->resets); err_disable_regulators: regulator_bulk_disable(cfg->num_vregs, qmp->vregs); @@ -4023,7 +4035,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 +4057,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->nocsr_reset || !qmp->phy_initialized) { + 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 +4084,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->nocsr_reset || !qmp->phy_initialized) { + /* 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 +4118,19 @@ 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); - /* stop SerDes and Phy-Coding-Sublayer */ - qphy_clrbits(qmp->pcs, cfg->regs[QPHY_START_CTRL], - SERDES_START | PCS_START); + if (!qmp->nocsr_reset) { + /* PHY reset */ + qphy_setbits(qmp->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); - /* Put PHY into POWER DOWN state: active low */ - qphy_clrbits(qmp->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], - cfg->pwrdn_ctrl); + /* 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); + } 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] 17+ messages in thread
* Re: [PATCH 2/2] phy: qcom: qmp-pcie: Add PHY register retention support 2025-01-21 9:41 ` [PATCH 2/2] phy: qcom: qmp-pcie: Add PHY register retention support Wenbin Yao @ 2025-01-21 10:36 ` Dmitry Baryshkov 2025-01-22 7:17 ` Wenbin Yao (Consultant) 0 siblings, 1 reply; 17+ messages in thread From: Dmitry Baryshkov @ 2025-01-21 10:36 UTC (permalink / raw) To: Wenbin Yao Cc: vkoul, kishon, p.zabel, abel.vesa, quic_qianyu, neil.armstrong, manivannan.sadhasivam, quic_devipriy, konrad.dybcio, linux-arm-msm, linux-phy, linux-kernel On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@quicinc.com> wrote: > > From: Qiang Yu <quic_qianyu@quicinc.com> > > Currently, BCR reset and PHY register setting are mandatory for every port > before link training. However, some QCOM PCIe PHYs support no_csr reset. > Different than BCR reset that is used to reset entire PHY including > hardware and register, once no_csr reset is toggled, only PHY hardware will > be reset but PHY registers will be retained, I'm sorry, I can't parse this. > which means PHY setting can > be skipped during PHY init if PCIe link was 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 is programmed and no_csr reset is > present, skip BCR reset and PHY register setting, so that PCIe link can be > established with no_csr reset only. This doesn't tell us why we want to do so. The general rule is not to depend on the bootloaders at all. The reason is pretty simple: it is hard to update bootloaders, while it is relatively easy to update the kernel. If the hardware team issues any kind of changes to the programming tables, the kernel will get them earlier than the bootloader. > > 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 | 91 +++++++++++++++--------- > 1 file changed, 58 insertions(+), 33 deletions(-) > -- With best wishes Dmitry -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] phy: qcom: qmp-pcie: Add PHY register retention support 2025-01-21 10:36 ` Dmitry Baryshkov @ 2025-01-22 7:17 ` Wenbin Yao (Consultant) 2025-01-22 9:43 ` Dmitry Baryshkov 0 siblings, 1 reply; 17+ messages in thread From: Wenbin Yao (Consultant) @ 2025-01-22 7:17 UTC (permalink / raw) To: Dmitry Baryshkov Cc: vkoul, kishon, p.zabel, abel.vesa, quic_qianyu, neil.armstrong, manivannan.sadhasivam, quic_devipriy, konrad.dybcio, linux-arm-msm, linux-phy, linux-kernel On 1/21/2025 6:36 PM, Dmitry Baryshkov wrote: > On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@quicinc.com> wrote: >> From: Qiang Yu <quic_qianyu@quicinc.com> >> >> Currently, BCR reset and PHY register setting are mandatory for every port >> before link training. However, some QCOM PCIe PHYs support no_csr reset. >> Different than BCR reset that is used to reset entire PHY including >> hardware and register, once no_csr reset is toggled, only PHY hardware will >> be reset but PHY registers will be retained, > I'm sorry, I can't parse this. The difference between no_csr reset and bcr reset is that no_csr reset doesn't reset the phy registers. If a phy is enabled in UEFI, its registers are programed. After Linux boot up, the registers will not be reset but keep the value programmed by UEFI if we only do no_csr reset, so we can skip phy setting. > >> which means PHY setting can >> be skipped during PHY init if PCIe link was 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 is programmed and no_csr reset is >> present, skip BCR reset and PHY register setting, so that PCIe link can be >> established with no_csr reset only. > This doesn't tell us why we want to do so. The general rule is not to > depend on the bootloaders at all. The reason is pretty simple: it is > hard to update bootloaders, while it is relatively easy to update the > kernel. If the hardware team issues any kind of changes to the > programming tables, the kernel will get them earlier than the > bootloader. With this change, we don't need to upstream phy setting for all phys support no_csr reset, this will save a great deal of efforts and simplify the phy driver. Our goal is to remove proprietary PCIe firmware operations from kernel. PHY is just the start and will be followed by controller, clocks, regulators, etc. If program table need to be changed, the place to do that will be UEFI. > >> 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 | 91 +++++++++++++++--------- >> 1 file changed, 58 insertions(+), 33 deletions(-) >> -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] phy: qcom: qmp-pcie: Add PHY register retention support 2025-01-22 7:17 ` Wenbin Yao (Consultant) @ 2025-01-22 9:43 ` Dmitry Baryshkov 2025-01-24 6:22 ` Qiang Yu 0 siblings, 1 reply; 17+ messages in thread From: Dmitry Baryshkov @ 2025-01-22 9:43 UTC (permalink / raw) To: Wenbin Yao (Consultant) Cc: vkoul, kishon, p.zabel, abel.vesa, quic_qianyu, neil.armstrong, manivannan.sadhasivam, quic_devipriy, konrad.dybcio, linux-arm-msm, linux-phy, linux-kernel On Wed, Jan 22, 2025 at 03:17:39PM +0800, Wenbin Yao (Consultant) wrote: > On 1/21/2025 6:36 PM, Dmitry Baryshkov wrote: > > On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@quicinc.com> wrote: > > > From: Qiang Yu <quic_qianyu@quicinc.com> > > > > > > Currently, BCR reset and PHY register setting are mandatory for every port > > > before link training. However, some QCOM PCIe PHYs support no_csr reset. > > > Different than BCR reset that is used to reset entire PHY including > > > hardware and register, once no_csr reset is toggled, only PHY hardware will > > > be reset but PHY registers will be retained, > > I'm sorry, I can't parse this. > The difference between no_csr reset and bcr reset is that no_csr reset > doesn't reset the phy registers. If a phy is enabled in UEFI, its registers > are programed. After Linux boot up, the registers will not be reset but > keep the value programmed by UEFI if we only do no_csr reset, so we can > skip phy setting. Please fix capitalization of the abbreviations (PHY, BCR) and add similar text to the commit message. > > > > > which means PHY setting can > > > be skipped during PHY init if PCIe link was 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 is programmed and no_csr reset is > > > present, skip BCR reset and PHY register setting, so that PCIe link can be > > > established with no_csr reset only. > > This doesn't tell us why we want to do so. The general rule is not to > > depend on the bootloaders at all. The reason is pretty simple: it is > > hard to update bootloaders, while it is relatively easy to update the > > kernel. If the hardware team issues any kind of changes to the > > programming tables, the kernel will get them earlier than the > > bootloader. > With this change, we don't need to upstream phy setting for all phys > support no_csr reset, this will save a great deal of efforts and simplify > the phy driver. Our goal is to remove proprietary PCIe firmware operations > from kernel. PHY is just the start and will be followed by controller, > clocks, regulators, etc. If program table need to be changed, the place to > do that will be UEFI. Well, that sounds like a very bad idea. Please don't do that. Linux kernel drivers should not depend on the UEFI or a bootloader. Unless there is a good reason for that, Linux should continue to be able to reset and program the PCIe PHY (as well as all other hw blocks). > > > > > 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 | 91 +++++++++++++++--------- > > > 1 file changed, 58 insertions(+), 33 deletions(-) > > > -- With best wishes Dmitry -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] phy: qcom: qmp-pcie: Add PHY register retention support 2025-01-22 9:43 ` Dmitry Baryshkov @ 2025-01-24 6:22 ` Qiang Yu 2025-01-24 7:08 ` Manivannan Sadhasivam 0 siblings, 1 reply; 17+ messages in thread From: Qiang Yu @ 2025-01-24 6:22 UTC (permalink / raw) To: Dmitry Baryshkov, Wenbin Yao (Consultant) Cc: vkoul, kishon, p.zabel, abel.vesa, neil.armstrong, manivannan.sadhasivam, quic_devipriy, konrad.dybcio, linux-arm-msm, linux-phy, linux-kernel On 1/22/2025 5:43 PM, Dmitry Baryshkov wrote: > On Wed, Jan 22, 2025 at 03:17:39PM +0800, Wenbin Yao (Consultant) wrote: >> On 1/21/2025 6:36 PM, Dmitry Baryshkov wrote: >>> On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@quicinc.com> wrote: >>>> From: Qiang Yu <quic_qianyu@quicinc.com> >>>> >>>> Currently, BCR reset and PHY register setting are mandatory for every port >>>> before link training. However, some QCOM PCIe PHYs support no_csr reset. >>>> Different than BCR reset that is used to reset entire PHY including >>>> hardware and register, once no_csr reset is toggled, only PHY hardware will >>>> be reset but PHY registers will be retained, >>> I'm sorry, I can't parse this. >> The difference between no_csr reset and bcr reset is that no_csr reset >> doesn't reset the phy registers. If a phy is enabled in UEFI, its registers >> are programed. After Linux boot up, the registers will not be reset but >> keep the value programmed by UEFI if we only do no_csr reset, so we can >> skip phy setting. > Please fix capitalization of the abbreviations (PHY, BCR) and add > similar text to the commit message. > >>>> which means PHY setting can >>>> be skipped during PHY init if PCIe link was 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 is programmed and no_csr reset is >>>> present, skip BCR reset and PHY register setting, so that PCIe link can be >>>> established with no_csr reset only. >>> This doesn't tell us why we want to do so. The general rule is not to >>> depend on the bootloaders at all. The reason is pretty simple: it is >>> hard to update bootloaders, while it is relatively easy to update the >>> kernel. If the hardware team issues any kind of changes to the >>> programming tables, the kernel will get them earlier than the >>> bootloader. >> With this change, we don't need to upstream phy setting for all phys >> support no_csr reset, this will save a great deal of efforts and simplify >> the phy driver. Our goal is to remove proprietary PCIe firmware operations >> from kernel. PHY is just the start and will be followed by controller, >> clocks, regulators, etc. If program table need to be changed, the place to >> do that will be UEFI. > Well, that sounds like a very bad idea. Please don't do that. Linux > kernel drivers should not depend on the UEFI or a bootloader. Unless > there is a good reason for that, Linux should continue to be able to > reset and program the PCIe PHY (as well as all other hw blocks). I'm wondering if it's really necessary for Linux to be able to program the PHY. Perhaps Linux should only care about common aspects defined by the PCIe spec like bus scanning, BAR space allocation, and functions provided by other PCIe capabilities. As for the specific operations that are different on various platforms, it might be more appropriate for the firmware to take care of them. This way, the responsibilities can be more clearly divided, and the driver could potentially be more streamlined. On the other hand, since the no_csr reset can retain register values, maybe we should still make full use of it, even if we don't want to rely on UEFI. For example, during runtime suspend/resume (the D3cold -> D0 cycle), when re-initializing the PHY, same PHY settings will be programmed again. This is a bit redundant. Thanks, Qiang >>>> 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 | 91 +++++++++++++++--------- >>>> 1 file changed, 58 insertions(+), 33 deletions(-) >>>> -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] phy: qcom: qmp-pcie: Add PHY register retention support 2025-01-24 6:22 ` Qiang Yu @ 2025-01-24 7:08 ` Manivannan Sadhasivam 2025-01-25 13:10 ` Konrad Dybcio 0 siblings, 1 reply; 17+ messages in thread From: Manivannan Sadhasivam @ 2025-01-24 7:08 UTC (permalink / raw) To: Qiang Yu Cc: Dmitry Baryshkov, Wenbin Yao (Consultant), vkoul, kishon, p.zabel, abel.vesa, neil.armstrong, manivannan.sadhasivam, quic_devipriy, konrad.dybcio, linux-arm-msm, linux-phy + Mayank (with whom I discussed this topic internally) On Fri, Jan 24, 2025 at 02:22:01PM +0800, Qiang Yu wrote: > > On 1/22/2025 5:43 PM, Dmitry Baryshkov wrote: > > On Wed, Jan 22, 2025 at 03:17:39PM +0800, Wenbin Yao (Consultant) wrote: > > > On 1/21/2025 6:36 PM, Dmitry Baryshkov wrote: > > > > On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@quicinc.com> wrote: > > > > > From: Qiang Yu <quic_qianyu@quicinc.com> > > > > > > > > > > Currently, BCR reset and PHY register setting are mandatory for every port > > > > > before link training. However, some QCOM PCIe PHYs support no_csr reset. > > > > > Different than BCR reset that is used to reset entire PHY including > > > > > hardware and register, once no_csr reset is toggled, only PHY hardware will > > > > > be reset but PHY registers will be retained, > > > > I'm sorry, I can't parse this. > > > The difference between no_csr reset and bcr reset is that no_csr reset > > > doesn't reset the phy registers. If a phy is enabled in UEFI, its registers > > > are programed. After Linux boot up, the registers will not be reset but > > > keep the value programmed by UEFI if we only do no_csr reset, so we can > > > skip phy setting. > > Please fix capitalization of the abbreviations (PHY, BCR) and add > > similar text to the commit message. > > > > > > > which means PHY setting can > > > > > be skipped during PHY init if PCIe link was 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 is programmed and no_csr reset is > > > > > present, skip BCR reset and PHY register setting, so that PCIe link can be > > > > > established with no_csr reset only. > > > > This doesn't tell us why we want to do so. The general rule is not to > > > > depend on the bootloaders at all. The reason is pretty simple: it is > > > > hard to update bootloaders, while it is relatively easy to update the > > > > kernel. If the hardware team issues any kind of changes to the > > > > programming tables, the kernel will get them earlier than the > > > > bootloader. > > > With this change, we don't need to upstream phy setting for all phys > > > support no_csr reset, this will save a great deal of efforts and simplify > > > the phy driver. Our goal is to remove proprietary PCIe firmware operations > > > from kernel. PHY is just the start and will be followed by controller, > > > clocks, regulators, etc. If program table need to be changed, the place to > > > do that will be UEFI. > > Well, that sounds like a very bad idea. Please don't do that. Linux > > kernel drivers should not depend on the UEFI or a bootloader. Unless > > there is a good reason for that, Linux should continue to be able to > > reset and program the PCIe PHY (as well as all other hw blocks). > I'm wondering if it's really necessary for Linux to be able to program the > PHY. Perhaps Linux should only care about common aspects defined by the > PCIe spec like bus scanning, BAR space allocation, and functions provided > by other PCIe capabilities. As for the specific operations that are > different on various platforms, it might be more appropriate for the > firmware to take care of them. This way, the responsibilities can be more > clearly divided, and the driver could potentially be > more streamlined. > It is not necessary in an ideal world, but what we have seen is Qcom releasing updated PHY init sequence after upstreaming the initial PHY driver support. In that case, the devices with old firmware will become outdated unless the fw is updated (which is not straightforward compared to updating the kernel). But, I do like this idea of reusing the PHY init sequence in the kernel. Though we cannot just do it for all platforms. Maybe we can enable it on platforms like compute starting from X1E and see how it goes? Just to minimize the impact if it didn't go well. > On the other hand, since the no_csr reset can retain register values, > maybe we should still make full use of it, even if we don't want to > rely on UEFI. For example, during runtime suspend/resume > (the D3cold -> D0 cycle) D3Cold during runtime suspend in bizarre. >, when re-initializing the PHY, same PHY > settings will be programmed again. This is a bit redundant. > Hmm, what would happen if the CX collapse happens during system suspend? Will the PHY registers be retained? - Mani -- மணிவண்ணன் சதாசிவம் -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] phy: qcom: qmp-pcie: Add PHY register retention support 2025-01-24 7:08 ` Manivannan Sadhasivam @ 2025-01-25 13:10 ` Konrad Dybcio 2025-01-29 8:29 ` neil.armstrong 0 siblings, 1 reply; 17+ messages in thread From: Konrad Dybcio @ 2025-01-25 13:10 UTC (permalink / raw) To: Manivannan Sadhasivam, Qiang Yu Cc: Dmitry Baryshkov, Wenbin Yao (Consultant), vkoul, kishon, p.zabel, abel.vesa, neil.armstrong, manivannan.sadhasivam, quic_devipriy, linux-arm-msm, linux-phy On 24.01.2025 8:08 AM, Manivannan Sadhasivam wrote: > + Mayank (with whom I discussed this topic internally) > > On Fri, Jan 24, 2025 at 02:22:01PM +0800, Qiang Yu wrote: >> >> On 1/22/2025 5:43 PM, Dmitry Baryshkov wrote: >>> On Wed, Jan 22, 2025 at 03:17:39PM +0800, Wenbin Yao (Consultant) wrote: >>>> On 1/21/2025 6:36 PM, Dmitry Baryshkov wrote: >>>>> On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@quicinc.com> wrote: >>>>>> From: Qiang Yu <quic_qianyu@quicinc.com> >>>>>> >>>>>> Currently, BCR reset and PHY register setting are mandatory for every port >>>>>> before link training. However, some QCOM PCIe PHYs support no_csr reset. >>>>>> Different than BCR reset that is used to reset entire PHY including >>>>>> hardware and register, once no_csr reset is toggled, only PHY hardware will >>>>>> be reset but PHY registers will be retained, >>>>> I'm sorry, I can't parse this. >>>> The difference between no_csr reset and bcr reset is that no_csr reset >>>> doesn't reset the phy registers. If a phy is enabled in UEFI, its registers >>>> are programed. After Linux boot up, the registers will not be reset but >>>> keep the value programmed by UEFI if we only do no_csr reset, so we can >>>> skip phy setting. >>> Please fix capitalization of the abbreviations (PHY, BCR) and add >>> similar text to the commit message. >>> >>>>>> which means PHY setting can >>>>>> be skipped during PHY init if PCIe link was 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 is programmed and no_csr reset is >>>>>> present, skip BCR reset and PHY register setting, so that PCIe link can be >>>>>> established with no_csr reset only. >>>>> This doesn't tell us why we want to do so. The general rule is not to >>>>> depend on the bootloaders at all. The reason is pretty simple: it is >>>>> hard to update bootloaders, while it is relatively easy to update the >>>>> kernel. If the hardware team issues any kind of changes to the >>>>> programming tables, the kernel will get them earlier than the >>>>> bootloader. We're assuming that if a product has shipped, the sequences used to power up the PHY in the bootloader (e.g. for NVMe) are already good. If some tragedy happens and an erratum is needed, we can always introduce a small override with the existing driver infrastructure (i.e. adding a new entry with a couple registers worth of programming sequence, leaving the other values in tact) While updates to the PHY init sequences have happened in the past, I believe they are mostly a result of oneOf: * someone upstreaming a pre-release revision / premature release * someone making a mistake when typing in the numbers Although sometimes there may be an update that isn't a result of any dev fault. It's worth to keep in mind certain values in there can be board specific, as they affect the analog interface. But we haven't seen that being used much if at all. >>>> With this change, we don't need to upstream phy setting for all phys >>>> support no_csr reset, this will save a great deal of efforts and simplify >>>> the phy driver. Our goal is to remove proprietary PCIe firmware operations >>>> from kernel. PHY is just the start and will be followed by controller, >>>> clocks, regulators, etc. If program table need to be changed, the place to >>>> do that will be UEFI. >>> Well, that sounds like a very bad idea. Please don't do that. Linux >>> kernel drivers should not depend on the UEFI or a bootloader. Unless >>> there is a good reason for that, Linux should continue to be able to >>> reset and program the PCIe PHY (as well as all other hw blocks). While I personally like being able to look into what's being programmed onto my hardware myself, this unfortunately does not scale, both from maintainability and reviewability perspectives with tables that host hundreds of essentially magic values. Plus this approach makes it easier to extend to other OSes, without polluting them with the same data. >> I'm wondering if it's really necessary for Linux to be able to program the >> PHY. Perhaps Linux should only care about common aspects defined by the >> PCIe spec like bus scanning, BAR space allocation, and functions provided >> by other PCIe capabilities. As for the specific operations that are >> different on various platforms, it might be more appropriate for the >> firmware to take care of them. This way, the responsibilities can be more >> clearly divided, and the driver could potentially be >> more streamlined. >> > > It is not necessary in an ideal world, but what we have seen is Qcom releasing > updated PHY init sequence after upstreaming the initial PHY driver support. In > that case, the devices with old firmware will become outdated unless the fw is > updated (which is not straightforward compared to updating the kernel). > > But, I do like this idea of reusing the PHY init sequence in the kernel. Though > we cannot just do it for all platforms. Maybe we can enable it on platforms like > compute starting from X1E and see how it goes? Just to minimize the impact if it > didn't go well. X1 is just the first of many here. We can possibly move some existing ones over after confirming that: a) the bootloader programs the PHYs at all b) the values programmed are reasonable on a case-by-case basis to ensure we're not regressing anything. Perhaps a piece of code and a modparam could be added that would read out the values present at boot and compare them to what the kernel has in store. >> On the other hand, since the no_csr reset can retain register values, >> maybe we should still make full use of it, even if we don't want to >> rely on UEFI. For example, during runtime suspend/resume >> (the D3cold -> D0 cycle) > > D3Cold during runtime suspend in bizarre. Very much so. But this extends to system suspend as well - we can still shave a couple ms off the wakeup times if we can skip all the programming. > >> , when re-initializing the PHY, same PHY >> settings will be programmed again. This is a bit redundant. >> > > Hmm, what would happen if the CX collapse happens during system suspend? Will > the PHY registers be retained? PHYs are always(asterisk) backed by some flavor of MX instead Konrad -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] phy: qcom: qmp-pcie: Add PHY register retention support 2025-01-25 13:10 ` Konrad Dybcio @ 2025-01-29 8:29 ` neil.armstrong 2025-01-29 11:29 ` Konrad Dybcio 0 siblings, 1 reply; 17+ messages in thread From: neil.armstrong @ 2025-01-29 8:29 UTC (permalink / raw) To: Konrad Dybcio, Manivannan Sadhasivam, Qiang Yu Cc: Dmitry Baryshkov, Wenbin Yao (Consultant), vkoul, kishon, p.zabel, abel.vesa, manivannan.sadhasivam, quic_devipriy, linux-arm-msm, linux-phy On 25/01/2025 14:10, Konrad Dybcio wrote: > On 24.01.2025 8:08 AM, Manivannan Sadhasivam wrote: >> + Mayank (with whom I discussed this topic internally) >> >> On Fri, Jan 24, 2025 at 02:22:01PM +0800, Qiang Yu wrote: >>> >>> On 1/22/2025 5:43 PM, Dmitry Baryshkov wrote: >>>> On Wed, Jan 22, 2025 at 03:17:39PM +0800, Wenbin Yao (Consultant) wrote: >>>>> On 1/21/2025 6:36 PM, Dmitry Baryshkov wrote: >>>>>> On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@quicinc.com> wrote: >>>>>>> From: Qiang Yu <quic_qianyu@quicinc.com> >>>>>>> >>>>>>> Currently, BCR reset and PHY register setting are mandatory for every port >>>>>>> before link training. However, some QCOM PCIe PHYs support no_csr reset. >>>>>>> Different than BCR reset that is used to reset entire PHY including >>>>>>> hardware and register, once no_csr reset is toggled, only PHY hardware will >>>>>>> be reset but PHY registers will be retained, >>>>>> I'm sorry, I can't parse this. >>>>> The difference between no_csr reset and bcr reset is that no_csr reset >>>>> doesn't reset the phy registers. If a phy is enabled in UEFI, its registers >>>>> are programed. After Linux boot up, the registers will not be reset but >>>>> keep the value programmed by UEFI if we only do no_csr reset, so we can >>>>> skip phy setting. >>>> Please fix capitalization of the abbreviations (PHY, BCR) and add >>>> similar text to the commit message. >>>> >>>>>>> which means PHY setting can >>>>>>> be skipped during PHY init if PCIe link was 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 is programmed and no_csr reset is >>>>>>> present, skip BCR reset and PHY register setting, so that PCIe link can be >>>>>>> established with no_csr reset only. >>>>>> This doesn't tell us why we want to do so. The general rule is not to >>>>>> depend on the bootloaders at all. The reason is pretty simple: it is >>>>>> hard to update bootloaders, while it is relatively easy to update the >>>>>> kernel. If the hardware team issues any kind of changes to the >>>>>> programming tables, the kernel will get them earlier than the >>>>>> bootloader. > > We're assuming that if a product has shipped, the sequences used to power up > the PHY in the bootloader (e.g. for NVMe) are already good. > > If some tragedy happens and an erratum is needed, we can always introduce a > small override with the existing driver infrastructure (i.e. adding a new > entry with a couple registers worth of programming sequence, leaving the other > values in tact) Assuming Linux will be always ran directly after the bootloader is a wild assumption. Yes, we should make use the noscr if the PHY is always programmed, but we should be always able to reprogram the PHY entirely to recover a faulty programmation. Neil <snip> -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] phy: qcom: qmp-pcie: Add PHY register retention support 2025-01-29 8:29 ` neil.armstrong @ 2025-01-29 11:29 ` Konrad Dybcio 2025-01-29 13:41 ` neil.armstrong 0 siblings, 1 reply; 17+ messages in thread From: Konrad Dybcio @ 2025-01-29 11:29 UTC (permalink / raw) To: neil.armstrong, Manivannan Sadhasivam, Qiang Yu Cc: Dmitry Baryshkov, Wenbin Yao (Consultant), vkoul, kishon, p.zabel, abel.vesa, manivannan.sadhasivam, quic_devipriy, linux-arm-msm, linux-phy On 29.01.2025 9:29 AM, neil.armstrong@linaro.org wrote: > On 25/01/2025 14:10, Konrad Dybcio wrote: >> On 24.01.2025 8:08 AM, Manivannan Sadhasivam wrote: >>> + Mayank (with whom I discussed this topic internally) >>> >>> On Fri, Jan 24, 2025 at 02:22:01PM +0800, Qiang Yu wrote: >>>> >>>> On 1/22/2025 5:43 PM, Dmitry Baryshkov wrote: >>>>> On Wed, Jan 22, 2025 at 03:17:39PM +0800, Wenbin Yao (Consultant) wrote: >>>>>> On 1/21/2025 6:36 PM, Dmitry Baryshkov wrote: >>>>>>> On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@quicinc.com> wrote: >>>>>>>> From: Qiang Yu <quic_qianyu@quicinc.com> >>>>>>>> >>>>>>>> Currently, BCR reset and PHY register setting are mandatory for every port >>>>>>>> before link training. However, some QCOM PCIe PHYs support no_csr reset. >>>>>>>> Different than BCR reset that is used to reset entire PHY including >>>>>>>> hardware and register, once no_csr reset is toggled, only PHY hardware will >>>>>>>> be reset but PHY registers will be retained, >>>>>>> I'm sorry, I can't parse this. >>>>>> The difference between no_csr reset and bcr reset is that no_csr reset >>>>>> doesn't reset the phy registers. If a phy is enabled in UEFI, its registers >>>>>> are programed. After Linux boot up, the registers will not be reset but >>>>>> keep the value programmed by UEFI if we only do no_csr reset, so we can >>>>>> skip phy setting. >>>>> Please fix capitalization of the abbreviations (PHY, BCR) and add >>>>> similar text to the commit message. >>>>> >>>>>>>> which means PHY setting can >>>>>>>> be skipped during PHY init if PCIe link was 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 is programmed and no_csr reset is >>>>>>>> present, skip BCR reset and PHY register setting, so that PCIe link can be >>>>>>>> established with no_csr reset only. >>>>>>> This doesn't tell us why we want to do so. The general rule is not to >>>>>>> depend on the bootloaders at all. The reason is pretty simple: it is >>>>>>> hard to update bootloaders, while it is relatively easy to update the >>>>>>> kernel. If the hardware team issues any kind of changes to the >>>>>>> programming tables, the kernel will get them earlier than the >>>>>>> bootloader. >> >> We're assuming that if a product has shipped, the sequences used to power up >> the PHY in the bootloader (e.g. for NVMe) are already good. >> >> If some tragedy happens and an erratum is needed, we can always introduce a >> small override with the existing driver infrastructure (i.e. adding a new >> entry with a couple registers worth of programming sequence, leaving the other >> values in tact) > > Assuming Linux will be always ran directly after the bootloader is a wild assumption. Situations like [normal boot chain] -> [... (resets the PHY and doesn't reprogram it)] -> Linux are both so unlikely and so intentional-by-the-user that it doesn't seem worth considering really. If whatever sits in the middle *must* hard-reset the phy, it can save the register state beforehand and restore them after the reset > Yes, we should make use the noscr if the PHY is always programmed, but we should be > always able to reprogram the PHY entirely to recover a faulty programmation. We aren't considering any possibility of faulty programming - it's either programmed, or not. And if the values configured by the bootloader are wrong, the device's firmware is considered faulty. Most devices probably follow the exact same magic values as our reference boards (though these values relate to analog characteristics, so perhaps not *all* of them, which is another argument for keeping the BL state) and these are extensively tested internally before any production devices make it out the door. Any updates deep into the product life are most likely just "nice to have"s and not anything critical, and as I've mentioned, we can still have overrides with the current logic inside this driver. Konrad -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] phy: qcom: qmp-pcie: Add PHY register retention support 2025-01-29 11:29 ` Konrad Dybcio @ 2025-01-29 13:41 ` neil.armstrong 2025-01-29 13:55 ` Konrad Dybcio 0 siblings, 1 reply; 17+ messages in thread From: neil.armstrong @ 2025-01-29 13:41 UTC (permalink / raw) To: Konrad Dybcio, Manivannan Sadhasivam, Qiang Yu Cc: Dmitry Baryshkov, Wenbin Yao (Consultant), vkoul, kishon, p.zabel, abel.vesa, manivannan.sadhasivam, quic_devipriy, linux-arm-msm, linux-phy On 29/01/2025 12:29, Konrad Dybcio wrote: > On 29.01.2025 9:29 AM, neil.armstrong@linaro.org wrote: >> On 25/01/2025 14:10, Konrad Dybcio wrote: >>> On 24.01.2025 8:08 AM, Manivannan Sadhasivam wrote: >>>> + Mayank (with whom I discussed this topic internally) >>>> >>>> On Fri, Jan 24, 2025 at 02:22:01PM +0800, Qiang Yu wrote: >>>>> >>>>> On 1/22/2025 5:43 PM, Dmitry Baryshkov wrote: >>>>>> On Wed, Jan 22, 2025 at 03:17:39PM +0800, Wenbin Yao (Consultant) wrote: >>>>>>> On 1/21/2025 6:36 PM, Dmitry Baryshkov wrote: >>>>>>>> On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@quicinc.com> wrote: >>>>>>>>> From: Qiang Yu <quic_qianyu@quicinc.com> >>>>>>>>> >>>>>>>>> Currently, BCR reset and PHY register setting are mandatory for every port >>>>>>>>> before link training. However, some QCOM PCIe PHYs support no_csr reset. >>>>>>>>> Different than BCR reset that is used to reset entire PHY including >>>>>>>>> hardware and register, once no_csr reset is toggled, only PHY hardware will >>>>>>>>> be reset but PHY registers will be retained, >>>>>>>> I'm sorry, I can't parse this. >>>>>>> The difference between no_csr reset and bcr reset is that no_csr reset >>>>>>> doesn't reset the phy registers. If a phy is enabled in UEFI, its registers >>>>>>> are programed. After Linux boot up, the registers will not be reset but >>>>>>> keep the value programmed by UEFI if we only do no_csr reset, so we can >>>>>>> skip phy setting. >>>>>> Please fix capitalization of the abbreviations (PHY, BCR) and add >>>>>> similar text to the commit message. >>>>>> >>>>>>>>> which means PHY setting can >>>>>>>>> be skipped during PHY init if PCIe link was 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 is programmed and no_csr reset is >>>>>>>>> present, skip BCR reset and PHY register setting, so that PCIe link can be >>>>>>>>> established with no_csr reset only. >>>>>>>> This doesn't tell us why we want to do so. The general rule is not to >>>>>>>> depend on the bootloaders at all. The reason is pretty simple: it is >>>>>>>> hard to update bootloaders, while it is relatively easy to update the >>>>>>>> kernel. If the hardware team issues any kind of changes to the >>>>>>>> programming tables, the kernel will get them earlier than the >>>>>>>> bootloader. >>> >>> We're assuming that if a product has shipped, the sequences used to power up >>> the PHY in the bootloader (e.g. for NVMe) are already good. >>> >>> If some tragedy happens and an erratum is needed, we can always introduce a >>> small override with the existing driver infrastructure (i.e. adding a new >>> entry with a couple registers worth of programming sequence, leaving the other >>> values in tact) >> >> Assuming Linux will be always ran directly after the bootloader is a wild assumption. > > Situations like > > [normal boot chain] -> [... (resets the PHY and doesn't reprogram it)] -> Linux > > are both so unlikely and so intentional-by-the-user that it doesn't seem > worth considering really. In embedded/mobile/edge world, definitely, in compute/PC-like market, not really. You'll have people add some custom bootloaders, hypervisors, who knows what... > > If whatever sits in the middle *must* hard-reset the phy, it can save the > register state beforehand and restore them after the reset > >> Yes, we should make use the noscr if the PHY is always programmed, but we should be >> always able to reprogram the PHY entirely to recover a faulty programmation. > > We aren't considering any possibility of faulty programming - it's either > programmed, or not. And if the values configured by the bootloader are wrong, > the device's firmware is considered faulty. > > Most devices probably follow the exact same magic values as our reference > boards (though these values relate to analog characteristics, so perhaps not > *all* of them, which is another argument for keeping the BL state) and these > are extensively tested internally before any production devices make it out > the door. Any updates deep into the product life are most likely just "nice > to have"s and not anything critical, and as I've mentioned, we can still have > overrides with the current logic inside this driver. > > Konrad -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] phy: qcom: qmp-pcie: Add PHY register retention support 2025-01-29 13:41 ` neil.armstrong @ 2025-01-29 13:55 ` Konrad Dybcio 2025-01-29 14:19 ` neil.armstrong 0 siblings, 1 reply; 17+ messages in thread From: Konrad Dybcio @ 2025-01-29 13:55 UTC (permalink / raw) To: neil.armstrong, Manivannan Sadhasivam, Qiang Yu Cc: Dmitry Baryshkov, Wenbin Yao (Consultant), vkoul, kishon, p.zabel, abel.vesa, manivannan.sadhasivam, quic_devipriy, linux-arm-msm, linux-phy On 29.01.2025 2:41 PM, neil.armstrong@linaro.org wrote: > On 29/01/2025 12:29, Konrad Dybcio wrote: >> On 29.01.2025 9:29 AM, neil.armstrong@linaro.org wrote: >>> On 25/01/2025 14:10, Konrad Dybcio wrote: >>>> On 24.01.2025 8:08 AM, Manivannan Sadhasivam wrote: >>>>> + Mayank (with whom I discussed this topic internally) >>>>> >>>>> On Fri, Jan 24, 2025 at 02:22:01PM +0800, Qiang Yu wrote: >>>>>> >>>>>> On 1/22/2025 5:43 PM, Dmitry Baryshkov wrote: >>>>>>> On Wed, Jan 22, 2025 at 03:17:39PM +0800, Wenbin Yao (Consultant) wrote: >>>>>>>> On 1/21/2025 6:36 PM, Dmitry Baryshkov wrote: >>>>>>>>> On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@quicinc.com> wrote: >>>>>>>>>> From: Qiang Yu <quic_qianyu@quicinc.com> >>>>>>>>>> >>>>>>>>>> Currently, BCR reset and PHY register setting are mandatory for every port >>>>>>>>>> before link training. However, some QCOM PCIe PHYs support no_csr reset. >>>>>>>>>> Different than BCR reset that is used to reset entire PHY including >>>>>>>>>> hardware and register, once no_csr reset is toggled, only PHY hardware will >>>>>>>>>> be reset but PHY registers will be retained, >>>>>>>>> I'm sorry, I can't parse this. >>>>>>>> The difference between no_csr reset and bcr reset is that no_csr reset >>>>>>>> doesn't reset the phy registers. If a phy is enabled in UEFI, its registers >>>>>>>> are programed. After Linux boot up, the registers will not be reset but >>>>>>>> keep the value programmed by UEFI if we only do no_csr reset, so we can >>>>>>>> skip phy setting. >>>>>>> Please fix capitalization of the abbreviations (PHY, BCR) and add >>>>>>> similar text to the commit message. >>>>>>> >>>>>>>>>> which means PHY setting can >>>>>>>>>> be skipped during PHY init if PCIe link was 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 is programmed and no_csr reset is >>>>>>>>>> present, skip BCR reset and PHY register setting, so that PCIe link can be >>>>>>>>>> established with no_csr reset only. >>>>>>>>> This doesn't tell us why we want to do so. The general rule is not to >>>>>>>>> depend on the bootloaders at all. The reason is pretty simple: it is >>>>>>>>> hard to update bootloaders, while it is relatively easy to update the >>>>>>>>> kernel. If the hardware team issues any kind of changes to the >>>>>>>>> programming tables, the kernel will get them earlier than the >>>>>>>>> bootloader. >>>> >>>> We're assuming that if a product has shipped, the sequences used to power up >>>> the PHY in the bootloader (e.g. for NVMe) are already good. >>>> >>>> If some tragedy happens and an erratum is needed, we can always introduce a >>>> small override with the existing driver infrastructure (i.e. adding a new >>>> entry with a couple registers worth of programming sequence, leaving the other >>>> values in tact) >>> >>> Assuming Linux will be always ran directly after the bootloader is a wild assumption. >> >> Situations like >> >> [normal boot chain] -> [... (resets the PHY and doesn't reprogram it)] -> Linux >> >> are both so unlikely and so intentional-by-the-user that it doesn't seem >> worth considering really. > > In embedded/mobile/edge world, definitely, in compute/PC-like market, not really. > > You'll have people add some custom bootloaders, hypervisors, who knows what... I see, however you actually have to intentionally assert the non-NO_CSR PHY reset from said custom bootloaders, hypervisors and whoknowswhats for the programmed sequence to be erased. So I have no idea what the issue is here. Konrad > >> >> If whatever sits in the middle *must* hard-reset the phy, it can save the >> register state beforehand and restore them after the reset >> >>> Yes, we should make use the noscr if the PHY is always programmed, but we should be >>> always able to reprogram the PHY entirely to recover a faulty programmation. >> >> We aren't considering any possibility of faulty programming - it's either >> programmed, or not. And if the values configured by the bootloader are wrong, >> the device's firmware is considered faulty. >> >> Most devices probably follow the exact same magic values as our reference >> boards (though these values relate to analog characteristics, so perhaps not >> *all* of them, which is another argument for keeping the BL state) and these >> are extensively tested internally before any production devices make it out >> the door. Any updates deep into the product life are most likely just "nice >> to have"s and not anything critical, and as I've mentioned, we can still have >> overrides with the current logic inside this driver. >> >> Konrad > -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] phy: qcom: qmp-pcie: Add PHY register retention support 2025-01-29 13:55 ` Konrad Dybcio @ 2025-01-29 14:19 ` neil.armstrong 2025-02-08 2:24 ` Konrad Dybcio 0 siblings, 1 reply; 17+ messages in thread From: neil.armstrong @ 2025-01-29 14:19 UTC (permalink / raw) To: Konrad Dybcio, Manivannan Sadhasivam, Qiang Yu Cc: Dmitry Baryshkov, Wenbin Yao (Consultant), vkoul, kishon, p.zabel, abel.vesa, manivannan.sadhasivam, quic_devipriy, linux-arm-msm, linux-phy On 29/01/2025 14:55, Konrad Dybcio wrote: > On 29.01.2025 2:41 PM, neil.armstrong@linaro.org wrote: >> On 29/01/2025 12:29, Konrad Dybcio wrote: >>> On 29.01.2025 9:29 AM, neil.armstrong@linaro.org wrote: >>>> On 25/01/2025 14:10, Konrad Dybcio wrote: >>>>> On 24.01.2025 8:08 AM, Manivannan Sadhasivam wrote: >>>>>> + Mayank (with whom I discussed this topic internally) >>>>>> >>>>>> On Fri, Jan 24, 2025 at 02:22:01PM +0800, Qiang Yu wrote: >>>>>>> >>>>>>> On 1/22/2025 5:43 PM, Dmitry Baryshkov wrote: >>>>>>>> On Wed, Jan 22, 2025 at 03:17:39PM +0800, Wenbin Yao (Consultant) wrote: >>>>>>>>> On 1/21/2025 6:36 PM, Dmitry Baryshkov wrote: >>>>>>>>>> On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@quicinc.com> wrote: >>>>>>>>>>> From: Qiang Yu <quic_qianyu@quicinc.com> >>>>>>>>>>> >>>>>>>>>>> Currently, BCR reset and PHY register setting are mandatory for every port >>>>>>>>>>> before link training. However, some QCOM PCIe PHYs support no_csr reset. >>>>>>>>>>> Different than BCR reset that is used to reset entire PHY including >>>>>>>>>>> hardware and register, once no_csr reset is toggled, only PHY hardware will >>>>>>>>>>> be reset but PHY registers will be retained, >>>>>>>>>> I'm sorry, I can't parse this. >>>>>>>>> The difference between no_csr reset and bcr reset is that no_csr reset >>>>>>>>> doesn't reset the phy registers. If a phy is enabled in UEFI, its registers >>>>>>>>> are programed. After Linux boot up, the registers will not be reset but >>>>>>>>> keep the value programmed by UEFI if we only do no_csr reset, so we can >>>>>>>>> skip phy setting. >>>>>>>> Please fix capitalization of the abbreviations (PHY, BCR) and add >>>>>>>> similar text to the commit message. >>>>>>>> >>>>>>>>>>> which means PHY setting can >>>>>>>>>>> be skipped during PHY init if PCIe link was 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 is programmed and no_csr reset is >>>>>>>>>>> present, skip BCR reset and PHY register setting, so that PCIe link can be >>>>>>>>>>> established with no_csr reset only. >>>>>>>>>> This doesn't tell us why we want to do so. The general rule is not to >>>>>>>>>> depend on the bootloaders at all. The reason is pretty simple: it is >>>>>>>>>> hard to update bootloaders, while it is relatively easy to update the >>>>>>>>>> kernel. If the hardware team issues any kind of changes to the >>>>>>>>>> programming tables, the kernel will get them earlier than the >>>>>>>>>> bootloader. >>>>> >>>>> We're assuming that if a product has shipped, the sequences used to power up >>>>> the PHY in the bootloader (e.g. for NVMe) are already good. >>>>> >>>>> If some tragedy happens and an erratum is needed, we can always introduce a >>>>> small override with the existing driver infrastructure (i.e. adding a new >>>>> entry with a couple registers worth of programming sequence, leaving the other >>>>> values in tact) >>>> >>>> Assuming Linux will be always ran directly after the bootloader is a wild assumption. >>> >>> Situations like >>> >>> [normal boot chain] -> [... (resets the PHY and doesn't reprogram it)] -> Linux >>> >>> are both so unlikely and so intentional-by-the-user that it doesn't seem >>> worth considering really. >> >> In embedded/mobile/edge world, definitely, in compute/PC-like market, not really. >> >> You'll have people add some custom bootloaders, hypervisors, who knows what... > > I see, however you actually have to intentionally assert the non-NO_CSR PHY > reset from said custom bootloaders, hypervisors and whoknowswhats for the > programmed sequence to be erased. So I have no idea what the issue is here. I won't argue further, but you know as I do that relying on the bootloader state is a dangerous game, and we already rely a lot for dsp stuff and we have a lot lot of issue related to the UEFI implementation already on production devices. I'm not against the nocsr stuff, which can be a big win for boot time, but honestly not adding a few registers in table seems risky enough, and we should probably delay this experiment until we are sure the nocsr stuff works fine. Neil > > Konrad > >> >>> >>> If whatever sits in the middle *must* hard-reset the phy, it can save the >>> register state beforehand and restore them after the reset >>> >>>> Yes, we should make use the noscr if the PHY is always programmed, but we should be >>>> always able to reprogram the PHY entirely to recover a faulty programmation. >>> >>> We aren't considering any possibility of faulty programming - it's either >>> programmed, or not. And if the values configured by the bootloader are wrong, >>> the device's firmware is considered faulty. >>> >>> Most devices probably follow the exact same magic values as our reference >>> boards (though these values relate to analog characteristics, so perhaps not >>> *all* of them, which is another argument for keeping the BL state) and these >>> are extensively tested internally before any production devices make it out >>> the door. Any updates deep into the product life are most likely just "nice >>> to have"s and not anything critical, and as I've mentioned, we can still have >>> overrides with the current logic inside this driver. >>> >>> Konrad >> -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] phy: qcom: qmp-pcie: Add PHY register retention support 2025-01-29 14:19 ` neil.armstrong @ 2025-02-08 2:24 ` Konrad Dybcio 0 siblings, 0 replies; 17+ messages in thread From: Konrad Dybcio @ 2025-02-08 2:24 UTC (permalink / raw) To: neil.armstrong, Manivannan Sadhasivam, Qiang Yu Cc: Dmitry Baryshkov, Wenbin Yao (Consultant), vkoul, kishon, p.zabel, abel.vesa, manivannan.sadhasivam, quic_devipriy, linux-arm-msm, linux-phy On 29.01.2025 3:19 PM, neil.armstrong@linaro.org wrote: > On 29/01/2025 14:55, Konrad Dybcio wrote: >> On 29.01.2025 2:41 PM, neil.armstrong@linaro.org wrote: >>> On 29/01/2025 12:29, Konrad Dybcio wrote: >>>> On 29.01.2025 9:29 AM, neil.armstrong@linaro.org wrote: >>>>> On 25/01/2025 14:10, Konrad Dybcio wrote: >>>>>> On 24.01.2025 8:08 AM, Manivannan Sadhasivam wrote: >>>>>>> + Mayank (with whom I discussed this topic internally) >>>>>>> >>>>>>> On Fri, Jan 24, 2025 at 02:22:01PM +0800, Qiang Yu wrote: >>>>>>>> >>>>>>>> On 1/22/2025 5:43 PM, Dmitry Baryshkov wrote: >>>>>>>>> On Wed, Jan 22, 2025 at 03:17:39PM +0800, Wenbin Yao (Consultant) wrote: >>>>>>>>>> On 1/21/2025 6:36 PM, Dmitry Baryshkov wrote: >>>>>>>>>>> On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@quicinc.com> wrote: >>>>>>>>>>>> From: Qiang Yu <quic_qianyu@quicinc.com> >>>>>>>>>>>> >>>>>>>>>>>> Currently, BCR reset and PHY register setting are mandatory for every port >>>>>>>>>>>> before link training. However, some QCOM PCIe PHYs support no_csr reset. >>>>>>>>>>>> Different than BCR reset that is used to reset entire PHY including >>>>>>>>>>>> hardware and register, once no_csr reset is toggled, only PHY hardware will >>>>>>>>>>>> be reset but PHY registers will be retained, >>>>>>>>>>> I'm sorry, I can't parse this. >>>>>>>>>> The difference between no_csr reset and bcr reset is that no_csr reset >>>>>>>>>> doesn't reset the phy registers. If a phy is enabled in UEFI, its registers >>>>>>>>>> are programed. After Linux boot up, the registers will not be reset but >>>>>>>>>> keep the value programmed by UEFI if we only do no_csr reset, so we can >>>>>>>>>> skip phy setting. >>>>>>>>> Please fix capitalization of the abbreviations (PHY, BCR) and add >>>>>>>>> similar text to the commit message. >>>>>>>>> >>>>>>>>>>>> which means PHY setting can >>>>>>>>>>>> be skipped during PHY init if PCIe link was 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 is programmed and no_csr reset is >>>>>>>>>>>> present, skip BCR reset and PHY register setting, so that PCIe link can be >>>>>>>>>>>> established with no_csr reset only. >>>>>>>>>>> This doesn't tell us why we want to do so. The general rule is not to >>>>>>>>>>> depend on the bootloaders at all. The reason is pretty simple: it is >>>>>>>>>>> hard to update bootloaders, while it is relatively easy to update the >>>>>>>>>>> kernel. If the hardware team issues any kind of changes to the >>>>>>>>>>> programming tables, the kernel will get them earlier than the >>>>>>>>>>> bootloader. >>>>>> >>>>>> We're assuming that if a product has shipped, the sequences used to power up >>>>>> the PHY in the bootloader (e.g. for NVMe) are already good. >>>>>> >>>>>> If some tragedy happens and an erratum is needed, we can always introduce a >>>>>> small override with the existing driver infrastructure (i.e. adding a new >>>>>> entry with a couple registers worth of programming sequence, leaving the other >>>>>> values in tact) >>>>> >>>>> Assuming Linux will be always ran directly after the bootloader is a wild assumption. >>>> >>>> Situations like >>>> >>>> [normal boot chain] -> [... (resets the PHY and doesn't reprogram it)] -> Linux >>>> >>>> are both so unlikely and so intentional-by-the-user that it doesn't seem >>>> worth considering really. >>> >>> In embedded/mobile/edge world, definitely, in compute/PC-like market, not really. >>> >>> You'll have people add some custom bootloaders, hypervisors, who knows what... >> >> I see, however you actually have to intentionally assert the non-NO_CSR PHY >> reset from said custom bootloaders, hypervisors and whoknowswhats for the >> programmed sequence to be erased. So I have no idea what the issue is here. > > I won't argue further, but you know as I do that relying on the bootloader state > is a dangerous game, and we already rely a lot for dsp stuff and we have > a lot lot of issue related to the UEFI implementation already on production > devices. > > I'm not against the nocsr stuff, which can be a big win for boot time, but > honestly not adding a few registers in table seems risky enough, and we should > probably delay this experiment until we are sure the nocsr stuff works fine. I tested a range of mobile/compute platforms and only the latter kept the PCIe PHYs initialized after dropping to the OS. No adverse effects that I can tell. Konrad -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2025-02-08 2:24 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-01-21 9:41 [PATCH 0/2] phy: qcom: qmp-pcie: Add PCIe PHY no_csr reset support Wenbin Yao 2025-01-21 9:41 ` [PATCH 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically Wenbin Yao 2025-01-21 9:55 ` Abel Vesa 2025-01-24 7:10 ` Manivannan Sadhasivam 2025-01-21 9:41 ` [PATCH 2/2] phy: qcom: qmp-pcie: Add PHY register retention support Wenbin Yao 2025-01-21 10:36 ` Dmitry Baryshkov 2025-01-22 7:17 ` Wenbin Yao (Consultant) 2025-01-22 9:43 ` Dmitry Baryshkov 2025-01-24 6:22 ` Qiang Yu 2025-01-24 7:08 ` Manivannan Sadhasivam 2025-01-25 13:10 ` Konrad Dybcio 2025-01-29 8:29 ` neil.armstrong 2025-01-29 11:29 ` Konrad Dybcio 2025-01-29 13:41 ` neil.armstrong 2025-01-29 13:55 ` Konrad Dybcio 2025-01-29 14:19 ` neil.armstrong 2025-02-08 2:24 ` Konrad Dybcio
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).