[PATCH v2 0/2] phy: qcom: qmp-pcie: Add PCIe PHY no_csr reset support

[PATCH v2 0/2] phy: qcom: qmp-pcie: Add PCIe PHY no_csr reset support

[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 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 | 109 ++++++++++++++---------
 1 file changed, 65 insertions(+), 44 deletions(-)


base-commit: bcf2acd8f64b0a5783deeeb5fd70c6163ec5acd7
-- 
2.34.1

[PATCH v2 0/2] phy: qcom: qmp-pcie: Add PCIe PHY no_csr reset support

[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 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 | 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

[PATCH v2 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically

[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 | 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

[PATCH v2 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically

[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 | 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

[PATCH v2 2/2] phy: qcom: qmp-pcie: Add PHY register retention support

[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 | 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

[PATCH v2 2/2] phy: qcom: qmp-pcie: Add PHY register retention support

[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 | 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

Re: [PATCH v2 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically

[Philipp Zabel]

On Di, 2025-02-11 at 17:42 +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>
> ---
>  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)

Why is -EINVAL ignored here?

Without this you could just use
devm_reset_control_get_optional_exclusive(), which already turns -
ENOENT into NULL. That seems to me the correct thing to do, as from
driver point-of-view, this reset control is optional.

> +			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;

regards
Philipp

Re: [PATCH v2 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically

[Philipp Zabel]

On Di, 2025-02-11 at 17:42 +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>
> ---
>  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)

Why is -EINVAL ignored here?

Without this you could just use
devm_reset_control_get_optional_exclusive(), which already turns -
ENOENT into NULL. That seems to me the correct thing to do, as from
driver point-of-view, this reset control is optional.

> +			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;

regards
Philipp

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/2] phy: qcom: qmp-pcie: Add PHY register retention support

[Dmitry Baryshkov]

On Tue, Feb 11, 2025 at 05:42:31PM +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 | 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) {

Instead of having phy_initialized please add another boolean field,
qmp->skip_init = !!qmp->nocsr_reset && !!phy_initialized;
Use qmp->skip_init through the code.

> +		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

what is CSR register?

> +	 * 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) {

Why this one doesn't check for the qmp->phy_initialized?

> +		/* 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
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/2] phy: qcom: qmp-pcie: Add PHY register retention support

[Dmitry Baryshkov]

On Tue, Feb 11, 2025 at 05:42:31PM +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 | 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) {

Instead of having phy_initialized please add another boolean field,
qmp->skip_init = !!qmp->nocsr_reset && !!phy_initialized;
Use qmp->skip_init through the code.

> +		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

what is CSR register?

> +	 * 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) {

Why this one doesn't check for the qmp->phy_initialized?

> +		/* 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
> 

-- 
With best wishes
Dmitry

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/2] phy: qcom: qmp-pcie: Add PHY register retention support

[Wenbin Yao (Consultant)]

On 2/12/2025 8:13 AM, Dmitry Baryshkov wrote:
> On Tue, Feb 11, 2025 at 05:42:31PM +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 | 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) {
> Instead of having phy_initialized please add another boolean field,
> qmp->skip_init = !!qmp->nocsr_reset && !!phy_initialized;
> Use qmp->skip_init through the code.

In qmp_pcie_power_off and qmp_pcie_exit, we only check qmp->nocsr_reset. It

seems unnecessary to combine qmp->nocsr_reset with 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
> what is CSR register?
The registers of PHY.
>
>> +	 * 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) {
> Why this one doesn't check for the qmp->phy_initialized?

If a PHY supports no_csr reset but isn't enabled in the bootloader, we

still need to program the phy settings only once so that we can reuse them

during the D3cold -> D0 cycle. Therefore, we don't check

qmp->phy_initialized here.

>
>> +		/* 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
>>
-- 
With best wishes
Wenbin

Re: [PATCH v2 2/2] phy: qcom: qmp-pcie: Add PHY register retention support

[Wenbin Yao (Consultant)]

On 2/12/2025 8:13 AM, Dmitry Baryshkov wrote:
> On Tue, Feb 11, 2025 at 05:42:31PM +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 | 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) {
> Instead of having phy_initialized please add another boolean field,
> qmp->skip_init = !!qmp->nocsr_reset && !!phy_initialized;
> Use qmp->skip_init through the code.

In qmp_pcie_power_off and qmp_pcie_exit, we only check qmp->nocsr_reset. It

seems unnecessary to combine qmp->nocsr_reset with 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
> what is CSR register?
The registers of PHY.
>
>> +	 * 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) {
> Why this one doesn't check for the qmp->phy_initialized?

If a PHY supports no_csr reset but isn't enabled in the bootloader, we

still need to program the phy settings only once so that we can reuse them

during the D3cold -> D0 cycle. Therefore, we don't check

qmp->phy_initialized here.

>
>> +		/* 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
>>
-- 
With best wishes
Wenbin

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/2] phy: qcom: qmp-pcie: Add PHY register retention support

[Dmitry Baryshkov]

On Wed, Feb 12, 2025 at 04:31:21PM +0800, Wenbin Yao (Consultant) wrote:
> On 2/12/2025 8:13 AM, Dmitry Baryshkov wrote:
> > On Tue, Feb 11, 2025 at 05:42:31PM +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 | 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) {
> > Instead of having phy_initialized please add another boolean field,
> > qmp->skip_init = !!qmp->nocsr_reset && !!phy_initialized;
> > Use qmp->skip_init through the code.
> 
> In qmp_pcie_power_off and qmp_pcie_exit, we only check qmp->nocsr_reset. It
> 
> seems unnecessary to combine qmp->nocsr_reset with phy_initialized.

The PHY is going to be initialized after qmp_pcie_init() completes, but
you are not updating phy_initialized. On the other hand skip_init still
does what it is supposed to do: tells the driver to skip (re-)init of
the registers.

> 
> > 
> > > +		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
> > what is CSR register?
> The registers of PHY.
> > 
> > > +	 * 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) {
> > Why this one doesn't check for the qmp->phy_initialized?
> 
> If a PHY supports no_csr reset but isn't enabled in the bootloader, we
> 
> still need to program the phy settings only once so that we can reuse them
> 
> during the D3cold -> D0 cycle. Therefore, we don't check
> 
> qmp->phy_initialized here.

Please add a comment. In future please make sure that your answer
doesn't contain unnecessary empty lines. It makes it harder to read your
response.

> 
> > 
> > > +		/* 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
> > > 
> -- 
> With best wishes
> Wenbin

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/2] phy: qcom: qmp-pcie: Add PHY register retention support

[Dmitry Baryshkov]

On Wed, Feb 12, 2025 at 04:31:21PM +0800, Wenbin Yao (Consultant) wrote:
> On 2/12/2025 8:13 AM, Dmitry Baryshkov wrote:
> > On Tue, Feb 11, 2025 at 05:42:31PM +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 | 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) {
> > Instead of having phy_initialized please add another boolean field,
> > qmp->skip_init = !!qmp->nocsr_reset && !!phy_initialized;
> > Use qmp->skip_init through the code.
> 
> In qmp_pcie_power_off and qmp_pcie_exit, we only check qmp->nocsr_reset. It
> 
> seems unnecessary to combine qmp->nocsr_reset with phy_initialized.

The PHY is going to be initialized after qmp_pcie_init() completes, but
you are not updating phy_initialized. On the other hand skip_init still
does what it is supposed to do: tells the driver to skip (re-)init of
the registers.

> 
> > 
> > > +		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
> > what is CSR register?
> The registers of PHY.
> > 
> > > +	 * 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) {
> > Why this one doesn't check for the qmp->phy_initialized?
> 
> If a PHY supports no_csr reset but isn't enabled in the bootloader, we
> 
> still need to program the phy settings only once so that we can reuse them
> 
> during the D3cold -> D0 cycle. Therefore, we don't check
> 
> qmp->phy_initialized here.

Please add a comment. In future please make sure that your answer
doesn't contain unnecessary empty lines. It makes it harder to read your
response.

> 
> > 
> > > +		/* 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
> > > 
> -- 
> With best wishes
> Wenbin

-- 
With best wishes
Dmitry

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/2] phy: qcom: qmp-pcie: Add PHY register retention support

[Dmitry Baryshkov]

On Wed, Feb 12, 2025 at 04:31:21PM +0800, Wenbin Yao (Consultant) wrote:
> On 2/12/2025 8:13 AM, Dmitry Baryshkov wrote:
> > On Tue, Feb 11, 2025 at 05:42:31PM +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 | 91 +++++++++++++++---------
> > >   1 file changed, 58 insertions(+), 33 deletions(-)
> > > 

> > > @@ -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
> > what is CSR register?
> The registers of PHY.

So 'CSR registers for phy' means 'registers of PHY for phy'? that seems
incorrect.

> > 
> > > +	 * 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)

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/2] phy: qcom: qmp-pcie: Add PHY register retention support

[Dmitry Baryshkov]

On Wed, Feb 12, 2025 at 04:31:21PM +0800, Wenbin Yao (Consultant) wrote:
> On 2/12/2025 8:13 AM, Dmitry Baryshkov wrote:
> > On Tue, Feb 11, 2025 at 05:42:31PM +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 | 91 +++++++++++++++---------
> > >   1 file changed, 58 insertions(+), 33 deletions(-)
> > > 

> > > @@ -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
> > what is CSR register?
> The registers of PHY.

So 'CSR registers for phy' means 'registers of PHY for phy'? that seems
incorrect.

> > 
> > > +	 * 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)

-- 
With best wishes
Dmitry

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically

[Konrad Dybcio]

On 11.02.2025 10:53 AM, Philipp Zabel wrote:
> On Di, 2025-02-11 at 17:42 +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>
>> ---

[...]

>>  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)
> 
> Why is -EINVAL ignored here?

If the NOCSR (partial) reset is missing, we can still assert the "full" reset
and program the hardware from the ground up. It's also needed for backwards
dt compat as not all platforms described it when originally added.

> Without this you could just use
> devm_reset_control_get_optional_exclusive(), which already turns -
> ENOENT into NULL. That seems to me the correct thing to do, as from
> driver point-of-view, this reset control is optional.

Good point, I forgot _optional_ was a thing in the reset framework

Konrad

Re: [PATCH v2 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically

[Konrad Dybcio]

On 11.02.2025 10:53 AM, Philipp Zabel wrote:
> On Di, 2025-02-11 at 17:42 +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>
>> ---

[...]

>>  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)
> 
> Why is -EINVAL ignored here?

If the NOCSR (partial) reset is missing, we can still assert the "full" reset
and program the hardware from the ground up. It's also needed for backwards
dt compat as not all platforms described it when originally added.

> Without this you could just use
> devm_reset_control_get_optional_exclusive(), which already turns -
> ENOENT into NULL. That seems to me the correct thing to do, as from
> driver point-of-view, this reset control is optional.

Good point, I forgot _optional_ was a thing in the reset framework

Konrad

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/2] phy: qcom: qmp-pcie: Add PHY register retention support

[Konrad Dybcio]

On 12.02.2025 12:07 PM, Dmitry Baryshkov wrote:
> On Wed, Feb 12, 2025 at 04:31:21PM +0800, Wenbin Yao (Consultant) wrote:
>> On 2/12/2025 8:13 AM, Dmitry Baryshkov wrote:
>>> On Tue, Feb 11, 2025 at 05:42:31PM +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 | 91 +++++++++++++++---------
>>>>   1 file changed, 58 insertions(+), 33 deletions(-)
>>>>
> 
>>>> @@ -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
>>> what is CSR register?
>> The registers of PHY.
> 
> So 'CSR registers for phy' means 'registers of PHY for phy'? that seems
> incorrect.

"Control and Status Registers"

Konrad

Re: [PATCH v2 2/2] phy: qcom: qmp-pcie: Add PHY register retention support

[Konrad Dybcio]

On 12.02.2025 12:07 PM, Dmitry Baryshkov wrote:
> On Wed, Feb 12, 2025 at 04:31:21PM +0800, Wenbin Yao (Consultant) wrote:
>> On 2/12/2025 8:13 AM, Dmitry Baryshkov wrote:
>>> On Tue, Feb 11, 2025 at 05:42:31PM +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 | 91 +++++++++++++++---------
>>>>   1 file changed, 58 insertions(+), 33 deletions(-)
>>>>
> 
>>>> @@ -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
>>> what is CSR register?
>> The registers of PHY.
> 
> So 'CSR registers for phy' means 'registers of PHY for phy'? that seems
> incorrect.

"Control and Status Registers"

Konrad

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically

[Wenbin Yao (Consultant)]

On 2/12/2025 8:31 PM, Konrad Dybcio wrote:
> On 11.02.2025 10:53 AM, Philipp Zabel wrote:
>> On Di, 2025-02-11 at 17:42 +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>
>>> ---
> [...]
>
>>>   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)
>> Why is -EINVAL ignored here?
> If the NOCSR (partial) reset is missing, we can still assert the "full" reset
> and program the hardware from the ground up. It's also needed for backwards
> dt compat as not all platforms described it when originally added.

Seems like we really can't ignore -EINVAL. If no_csr reset is missing in
dts, it will return -ENOENT, which is turned into NULL by
devm_reset_control_get_optional_exclusive. -EINVAL indicates something
wrong in args we passed to the function and the reset property that need to
be fixed.

>
>> Without this you could just use
>> devm_reset_control_get_optional_exclusive(), which already turns -
>> ENOENT into NULL. That seems to me the correct thing to do, as from
>> driver point-of-view, this reset control is optional.
> Good point, I forgot _optional_ was a thing in the reset framework
>
> Konrad

Will use devm_reset_control_get_optional_exclusive function in patch v3.

-- 
With best wishes
Wenbin

Re: [PATCH v2 1/2] phy: qcom: pcie: Determine has_nocsr_reset dynamically

[Wenbin Yao (Consultant)]

On 2/12/2025 8:31 PM, Konrad Dybcio wrote:
> On 11.02.2025 10:53 AM, Philipp Zabel wrote:
>> On Di, 2025-02-11 at 17:42 +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>
>>> ---
> [...]
>
>>>   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)
>> Why is -EINVAL ignored here?
> If the NOCSR (partial) reset is missing, we can still assert the "full" reset
> and program the hardware from the ground up. It's also needed for backwards
> dt compat as not all platforms described it when originally added.

Seems like we really can't ignore -EINVAL. If no_csr reset is missing in
dts, it will return -ENOENT, which is turned into NULL by
devm_reset_control_get_optional_exclusive. -EINVAL indicates something
wrong in args we passed to the function and the reset property that need to
be fixed.

>
>> Without this you could just use
>> devm_reset_control_get_optional_exclusive(), which already turns -
>> ENOENT into NULL. That seems to me the correct thing to do, as from
>> driver point-of-view, this reset control is optional.
> Good point, I forgot _optional_ was a thing in the reset framework
>
> Konrad

Will use devm_reset_control_get_optional_exclusive function in patch v3.

-- 
With best wishes
Wenbin

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/2] phy: qcom: qmp-pcie: Add PHY register retention support

[Wenbin Yao (Consultant)]

On 2/12/2025 7:06 PM, Dmitry Baryshkov wrote:
> On Wed, Feb 12, 2025 at 04:31:21PM +0800, Wenbin Yao (Consultant) wrote:
>> On 2/12/2025 8:13 AM, Dmitry Baryshkov wrote:
>>> On Tue, Feb 11, 2025 at 05:42:31PM +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 | 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) {
>>> Instead of having phy_initialized please add another boolean field,
>>> qmp->skip_init = !!qmp->nocsr_reset && !!phy_initialized;
>>> Use qmp->skip_init through the code.
>> In qmp_pcie_power_off and qmp_pcie_exit, we only check qmp->nocsr_reset. It
>>
>> seems unnecessary to combine qmp->nocsr_reset with phy_initialized.
> The PHY is going to be initialized after qmp_pcie_init() completes, but
> you are not updating phy_initialized. On the other hand skip_init still
> does what it is supposed to do: tells the driver to skip (re-)init of
> the registers.

make sense, will use 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 +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
>>> what is CSR register?
>> The registers of PHY.
>>>> +	 * 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) {
>>> Why this one doesn't check for the qmp->phy_initialized?
>> If a PHY supports no_csr reset but isn't enabled in the bootloader, we
>>
>> still need to program the phy settings only once so that we can reuse them
>>
>> during the D3cold -> D0 cycle. Therefore, we don't check
>>
>> qmp->phy_initialized here.
> Please add a comment. In future please make sure that your answer
> doesn't contain unnecessary empty lines. It makes it harder to read your
> response.

Will add a comment in patch v3.

>
>>>> +		/* 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
>>>>
>> -- 
>> With best wishes
>> Wenbin

-- 
With best wishes
Wenbin

Re: [PATCH v2 2/2] phy: qcom: qmp-pcie: Add PHY register retention support

[Wenbin Yao (Consultant)]

On 2/12/2025 7:06 PM, Dmitry Baryshkov wrote:
> On Wed, Feb 12, 2025 at 04:31:21PM +0800, Wenbin Yao (Consultant) wrote:
>> On 2/12/2025 8:13 AM, Dmitry Baryshkov wrote:
>>> On Tue, Feb 11, 2025 at 05:42:31PM +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 | 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) {
>>> Instead of having phy_initialized please add another boolean field,
>>> qmp->skip_init = !!qmp->nocsr_reset && !!phy_initialized;
>>> Use qmp->skip_init through the code.
>> In qmp_pcie_power_off and qmp_pcie_exit, we only check qmp->nocsr_reset. It
>>
>> seems unnecessary to combine qmp->nocsr_reset with phy_initialized.
> The PHY is going to be initialized after qmp_pcie_init() completes, but
> you are not updating phy_initialized. On the other hand skip_init still
> does what it is supposed to do: tells the driver to skip (re-)init of
> the registers.

make sense, will use 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 +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
>>> what is CSR register?
>> The registers of PHY.
>>>> +	 * 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) {
>>> Why this one doesn't check for the qmp->phy_initialized?
>> If a PHY supports no_csr reset but isn't enabled in the bootloader, we
>>
>> still need to program the phy settings only once so that we can reuse them
>>
>> during the D3cold -> D0 cycle. Therefore, we don't check
>>
>> qmp->phy_initialized here.
> Please add a comment. In future please make sure that your answer
> doesn't contain unnecessary empty lines. It makes it harder to read your
> response.

Will add a comment in patch v3.

>
>>>> +		/* 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
>>>>
>> -- 
>> With best wishes
>> Wenbin

-- 
With best wishes
Wenbin


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy