[v2] phy: qcom: qmp-pcie: Add PCIe PHY no_csr reset support

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

Posted by Wenbin Yao 10 months, 1 week ago

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

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

Posted by Dmitry Baryshkov 10 months, 1 week ago

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

Posted by Wenbin Yao (Consultant) 10 months, 1 week ago

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

Posted by Dmitry Baryshkov 10 months, 1 week ago

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

Posted by Wenbin Yao (Consultant) 10 months, 1 week ago

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

Posted by Dmitry Baryshkov 10 months, 1 week ago

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

Posted by Konrad Dybcio 10 months, 1 week ago

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

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