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

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

Wenbin Yao (2):
  phy: qcom: pcie: Determine has_nocsr_reset dynamically
  phy: qcom: qmp-pcie: Add PHY register retention support

 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 109 ++++++++++++++---------
 1 file changed, 65 insertions(+), 44 deletions(-)


base-commit: bcf2acd8f64b0a5783deeeb5fd70c6163ec5acd7
-- 
2.34.1

[PATCH 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>
---
 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 2/2] phy: qcom: qmp-pcie: Add PHY register retention support

[Wenbin Yao]

From: Qiang Yu <quic_qianyu@quicinc.com>

Currently, BCR reset and PHY register setting are mandatory for every port
before link training. However, some QCOM PCIe PHYs support no_csr reset.
Different than BCR reset that is used to reset entire PHY including
hardware and register, once no_csr reset is toggled, only PHY hardware will
be reset but PHY registers will be retained, which means PHY setting can
be skipped during PHY init if PCIe link was enabled in booltloader and only
no_csr is toggled after that.

Hence, determine whether the PHY has been enabled in bootloader by
verifying QPHY_START_CTRL register. If it is programmed and no_csr reset is
present, skip BCR reset and PHY register setting, so that PCIe link can be
established with no_csr reset only.

Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
---
 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 91 +++++++++++++++---------
 1 file changed, 58 insertions(+), 33 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
index ac42e4b01065..7f0802d09812 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
@@ -2805,6 +2805,7 @@ struct qmp_pcie {
 
 	const struct qmp_phy_cfg *cfg;
 	bool tcsr_4ln_config;
+	bool phy_initialized;
 
 	void __iomem *serdes;
 	void __iomem *pcs;
@@ -3976,6 +3977,7 @@ static int qmp_pcie_init(struct phy *phy)
 {
 	struct qmp_pcie *qmp = phy_get_drvdata(phy);
 	const struct qmp_phy_cfg *cfg = qmp->cfg;
+	void __iomem *pcs = qmp->pcs;
 	int ret;
 
 	ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
@@ -3984,10 +3986,17 @@ static int qmp_pcie_init(struct phy *phy)
 		return ret;
 	}
 
-	ret = reset_control_bulk_assert(cfg->num_resets, qmp->resets);
-	if (ret) {
-		dev_err(qmp->dev, "reset assert failed\n");
-		goto err_disable_regulators;
+	qmp->phy_initialized = !!(readl(pcs + cfg->regs[QPHY_START_CTRL]));
+	/*
+	 * Toggle BCR reset for phy that doesn't support no_csr
+	 * reset or has not been initialized
+	 */
+	if (!qmp->nocsr_reset || !qmp->phy_initialized) {
+		ret = reset_control_bulk_assert(cfg->num_resets, qmp->resets);
+		if (ret) {
+			dev_err(qmp->dev, "reset assert failed\n");
+			goto err_disable_regulators;
+		}
 	}
 
 	ret = reset_control_assert(qmp->nocsr_reset);
@@ -3998,10 +4007,12 @@ static int qmp_pcie_init(struct phy *phy)
 
 	usleep_range(200, 300);
 
-	ret = reset_control_bulk_deassert(cfg->num_resets, qmp->resets);
-	if (ret) {
-		dev_err(qmp->dev, "reset deassert failed\n");
-		goto err_assert_reset;
+	if (!qmp->nocsr_reset || !qmp->phy_initialized) {
+		ret = reset_control_bulk_deassert(cfg->num_resets, qmp->resets);
+		if (ret) {
+			dev_err(qmp->dev, "reset deassert failed\n");
+			goto err_assert_reset;
+		}
 	}
 
 	ret = clk_bulk_prepare_enable(ARRAY_SIZE(qmp_pciephy_clk_l), qmp->clks);
@@ -4011,7 +4022,8 @@ static int qmp_pcie_init(struct phy *phy)
 	return 0;
 
 err_assert_reset:
-	reset_control_bulk_assert(cfg->num_resets, qmp->resets);
+	if (!qmp->nocsr_reset || !qmp->phy_initialized)
+		reset_control_bulk_assert(cfg->num_resets, qmp->resets);
 err_disable_regulators:
 	regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
 
@@ -4023,7 +4035,10 @@ static int qmp_pcie_exit(struct phy *phy)
 	struct qmp_pcie *qmp = phy_get_drvdata(phy);
 	const struct qmp_phy_cfg *cfg = qmp->cfg;
 
-	reset_control_bulk_assert(cfg->num_resets, qmp->resets);
+	if (!qmp->nocsr_reset)
+		reset_control_bulk_assert(cfg->num_resets, qmp->resets);
+	else
+		reset_control_assert(qmp->nocsr_reset);
 
 	clk_bulk_disable_unprepare(ARRAY_SIZE(qmp_pciephy_clk_l), qmp->clks);
 
@@ -4042,16 +4057,22 @@ static int qmp_pcie_power_on(struct phy *phy)
 	unsigned int mask, val;
 	int ret;
 
-	qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
-			cfg->pwrdn_ctrl);
+	/*
+	 * Write CSR register for phy that doesn't support no_csr
+	 * reset or has not been initialized
+	 */
+	if (!qmp->nocsr_reset || !qmp->phy_initialized) {
+		qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
+				cfg->pwrdn_ctrl);
 
-	if (qmp->mode == PHY_MODE_PCIE_RC)
-		mode_tbls = cfg->tbls_rc;
-	else
-		mode_tbls = cfg->tbls_ep;
+		if (qmp->mode == PHY_MODE_PCIE_RC)
+			mode_tbls = cfg->tbls_rc;
+		else
+			mode_tbls = cfg->tbls_ep;
 
-	qmp_pcie_init_registers(qmp, &cfg->tbls);
-	qmp_pcie_init_registers(qmp, mode_tbls);
+		qmp_pcie_init_registers(qmp, &cfg->tbls);
+		qmp_pcie_init_registers(qmp, mode_tbls);
+	}
 
 	ret = clk_bulk_prepare_enable(qmp->num_pipe_clks, qmp->pipe_clks);
 	if (ret)
@@ -4063,15 +4084,16 @@ static int qmp_pcie_power_on(struct phy *phy)
 		goto err_disable_pipe_clk;
 	}
 
-	/* Pull PHY out of reset state */
-	qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
+	if (!qmp->nocsr_reset || !qmp->phy_initialized) {
+		/* Pull PHY out of reset state */
+		qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
 
-	/* start SerDes and Phy-Coding-Sublayer */
-	qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], SERDES_START | PCS_START);
-
-	if (!cfg->skip_start_delay)
-		usleep_range(1000, 1200);
+		/* start SerDes and Phy-Coding-Sublayer */
+		qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], SERDES_START | PCS_START);
 
+		if (!cfg->skip_start_delay)
+			usleep_range(1000, 1200);
+	}
 	status = pcs + cfg->regs[QPHY_PCS_STATUS];
 	mask = cfg->phy_status;
 	ret = readl_poll_timeout(status, val, !(val & mask), 200,
@@ -4096,16 +4118,19 @@ static int qmp_pcie_power_off(struct phy *phy)
 
 	clk_bulk_disable_unprepare(qmp->num_pipe_clks, qmp->pipe_clks);
 
-	/* PHY reset */
-	qphy_setbits(qmp->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
 
-	/* stop SerDes and Phy-Coding-Sublayer */
-	qphy_clrbits(qmp->pcs, cfg->regs[QPHY_START_CTRL],
-			SERDES_START | PCS_START);
+	if (!qmp->nocsr_reset) {
+		/* PHY reset */
+		qphy_setbits(qmp->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
 
-	/* Put PHY into POWER DOWN state: active low */
-	qphy_clrbits(qmp->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
-			cfg->pwrdn_ctrl);
+		/* stop SerDes and Phy-Coding-Sublayer */
+		qphy_clrbits(qmp->pcs, cfg->regs[QPHY_START_CTRL],
+				SERDES_START | PCS_START);
+
+		/* Put PHY into POWER DOWN state: active low */
+		qphy_clrbits(qmp->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
+				cfg->pwrdn_ctrl);
+	}
 
 	return 0;
 }
-- 
2.34.1

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

[Abel Vesa]

On 25-01-21 17:41:39, Wenbin Yao wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> Decide the in-driver logic based on whether the nocsr reset is present
> and defer checking the appropriateness of that to dt-bindings to save
> on boilerplate.
> 
> Reset controller APIs are fine consuming a nullptr, so no additional
> checks are necessary there.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>

Reviewed-by: Abel Vesa <abel.vesa@linaro.org>

> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> index 873f2f9844c6..ac42e4b01065 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> @@ -2793,8 +2793,6 @@ struct qmp_phy_cfg {
>  
>  	bool skip_start_delay;
>  
> -	bool has_nocsr_reset;
> -
>  	/* QMP PHY pipe clock interface rate */
>  	unsigned long pipe_clock_rate;
>  
> @@ -3685,7 +3683,6 @@ static const struct qmp_phy_cfg sm8550_qmp_gen4x2_pciephy_cfg = {
>  
>  	.pwrdn_ctrl		= SW_PWRDN | REFCLK_DRV_DSBL,
>  	.phy_status		= PHYSTATUS_4_20,
> -	.has_nocsr_reset	= true,
>  
>  	/* 20MHz PHY AUX Clock */
>  	.aux_clock_rate		= 20000000,
> @@ -3718,7 +3715,6 @@ static const struct qmp_phy_cfg sm8650_qmp_gen4x2_pciephy_cfg = {
>  
>  	.pwrdn_ctrl		= SW_PWRDN | REFCLK_DRV_DSBL,
>  	.phy_status		= PHYSTATUS_4_20,
> -	.has_nocsr_reset	= true,
>  
>  	/* 20MHz PHY AUX Clock */
>  	.aux_clock_rate		= 20000000,
> @@ -3836,7 +3832,6 @@ static const struct qmp_phy_cfg x1e80100_qmp_gen4x2_pciephy_cfg = {
>  
>  	.pwrdn_ctrl		= SW_PWRDN | REFCLK_DRV_DSBL,
>  	.phy_status		= PHYSTATUS_4_20,
> -	.has_nocsr_reset	= true,
>  };
>  
>  static const struct qmp_phy_cfg x1e80100_qmp_gen4x4_pciephy_cfg = {
> @@ -3870,7 +3865,6 @@ static const struct qmp_phy_cfg x1e80100_qmp_gen4x4_pciephy_cfg = {
>  
>  	.pwrdn_ctrl		= SW_PWRDN | REFCLK_DRV_DSBL,
>  	.phy_status		= PHYSTATUS_4_20,
> -	.has_nocsr_reset	= true,
>  };
>  
>  static const struct qmp_phy_cfg x1e80100_qmp_gen4x8_pciephy_cfg = {
> @@ -3902,7 +3896,6 @@ static const struct qmp_phy_cfg x1e80100_qmp_gen4x8_pciephy_cfg = {
>  
>  	.pwrdn_ctrl		= SW_PWRDN | REFCLK_DRV_DSBL,
>  	.phy_status		= PHYSTATUS_4_20,
> -	.has_nocsr_reset	= true,
>  };
>  
>  static void qmp_pcie_init_port_b(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tbls *tbls)
> @@ -4203,11 +4196,14 @@ static int qmp_pcie_reset_init(struct qmp_pcie *qmp)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "failed to get resets\n");
>  
> -	if (cfg->has_nocsr_reset) {
> -		qmp->nocsr_reset = devm_reset_control_get_exclusive(dev, "phy_nocsr");
> -		if (IS_ERR(qmp->nocsr_reset))
> +	qmp->nocsr_reset = devm_reset_control_get_exclusive(dev, "phy_nocsr");
> +	if (IS_ERR(qmp->nocsr_reset)) {
> +		if (PTR_ERR(qmp->nocsr_reset) == -ENOENT ||
> +		    PTR_ERR(qmp->nocsr_reset) == -EINVAL)
> +			qmp->nocsr_reset = NULL;
> +		else
>  			return dev_err_probe(dev, PTR_ERR(qmp->nocsr_reset),
> -						"failed to get no-csr reset\n");
> +					     "failed to get no-csr reset\n");
>  	}
>  
>  	return 0;
> -- 
> 2.34.1
> 

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

[Dmitry Baryshkov]

On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@quicinc.com> wrote:
>
> From: Qiang Yu <quic_qianyu@quicinc.com>
>
> Currently, BCR reset and PHY register setting are mandatory for every port
> before link training. However, some QCOM PCIe PHYs support no_csr reset.
> Different than BCR reset that is used to reset entire PHY including
> hardware and register, once no_csr reset is toggled, only PHY hardware will
> be reset but PHY registers will be retained,

I'm sorry, I can't parse this.

> which means PHY setting can
> be skipped during PHY init if PCIe link was enabled in booltloader and only
> no_csr is toggled after that.
>
> Hence, determine whether the PHY has been enabled in bootloader by
> verifying QPHY_START_CTRL register. If it is programmed and no_csr reset is
> present, skip BCR reset and PHY register setting, so that PCIe link can be
> established with no_csr reset only.

This doesn't tell us why we want to do so. The general rule is not to
depend on the bootloaders at all. The reason is pretty simple: it is
hard to update bootloaders, while it is relatively easy to update the
kernel. If the hardware team issues any kind of changes to the
programming tables, the kernel will get them earlier than the
bootloader.

>
> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 91 +++++++++++++++---------
>  1 file changed, 58 insertions(+), 33 deletions(-)
>
-- 
With best wishes
Dmitry

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

[Wenbin Yao (Consultant)]

On 1/21/2025 6:36 PM, Dmitry Baryshkov wrote:
> On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@quicinc.com> wrote:
>> From: Qiang Yu <quic_qianyu@quicinc.com>
>>
>> Currently, BCR reset and PHY register setting are mandatory for every port
>> before link training. However, some QCOM PCIe PHYs support no_csr reset.
>> Different than BCR reset that is used to reset entire PHY including
>> hardware and register, once no_csr reset is toggled, only PHY hardware will
>> be reset but PHY registers will be retained,
> I'm sorry, I can't parse this.
The difference between no_csr reset and bcr reset is that no_csr reset
doesn't reset the phy registers. If a phy is enabled in UEFI, its registers
are programed. After Linux boot up, the registers will not be reset but
keep the value programmed by UEFI if we only do no_csr reset, so we can
skip phy setting.
>
>> which means PHY setting can
>> be skipped during PHY init if PCIe link was enabled in booltloader and only
>> no_csr is toggled after that.
>>
>> Hence, determine whether the PHY has been enabled in bootloader by
>> verifying QPHY_START_CTRL register. If it is programmed and no_csr reset is
>> present, skip BCR reset and PHY register setting, so that PCIe link can be
>> established with no_csr reset only.
> This doesn't tell us why we want to do so. The general rule is not to
> depend on the bootloaders at all. The reason is pretty simple: it is
> hard to update bootloaders, while it is relatively easy to update the
> kernel. If the hardware team issues any kind of changes to the
> programming tables, the kernel will get them earlier than the
> bootloader.
With this change, we don't need to upstream phy setting for all phys
support no_csr reset, this will save a great deal of efforts and simplify
the phy driver. Our goal is to remove proprietary PCIe firmware operations
from kernel. PHY is just the start and will be followed by controller,
clocks, regulators, etc. If program table need to be changed, the place to
do that will be UEFI.
>
>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
>> ---
>>   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 91 +++++++++++++++---------
>>   1 file changed, 58 insertions(+), 33 deletions(-)
>>

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

[Dmitry Baryshkov]

On Wed, Jan 22, 2025 at 03:17:39PM +0800, Wenbin Yao (Consultant) wrote:
> On 1/21/2025 6:36 PM, Dmitry Baryshkov wrote:
> > On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@quicinc.com> wrote:
> > > From: Qiang Yu <quic_qianyu@quicinc.com>
> > > 
> > > Currently, BCR reset and PHY register setting are mandatory for every port
> > > before link training. However, some QCOM PCIe PHYs support no_csr reset.
> > > Different than BCR reset that is used to reset entire PHY including
> > > hardware and register, once no_csr reset is toggled, only PHY hardware will
> > > be reset but PHY registers will be retained,
> > I'm sorry, I can't parse this.
> The difference between no_csr reset and bcr reset is that no_csr reset
> doesn't reset the phy registers. If a phy is enabled in UEFI, its registers
> are programed. After Linux boot up, the registers will not be reset but
> keep the value programmed by UEFI if we only do no_csr reset, so we can
> skip phy setting.

Please fix capitalization of the abbreviations (PHY, BCR) and add
similar text to the commit message.

> > 
> > > which means PHY setting can
> > > be skipped during PHY init if PCIe link was enabled in booltloader and only
> > > no_csr is toggled after that.
> > > 
> > > Hence, determine whether the PHY has been enabled in bootloader by
> > > verifying QPHY_START_CTRL register. If it is programmed and no_csr reset is
> > > present, skip BCR reset and PHY register setting, so that PCIe link can be
> > > established with no_csr reset only.
> > This doesn't tell us why we want to do so. The general rule is not to
> > depend on the bootloaders at all. The reason is pretty simple: it is
> > hard to update bootloaders, while it is relatively easy to update the
> > kernel. If the hardware team issues any kind of changes to the
> > programming tables, the kernel will get them earlier than the
> > bootloader.
> With this change, we don't need to upstream phy setting for all phys
> support no_csr reset, this will save a great deal of efforts and simplify
> the phy driver. Our goal is to remove proprietary PCIe firmware operations
> from kernel. PHY is just the start and will be followed by controller,
> clocks, regulators, etc. If program table need to be changed, the place to
> do that will be UEFI.

Well, that sounds like a very bad idea. Please don't do that. Linux
kernel drivers should not depend on the UEFI or a bootloader. Unless
there is a good reason for that, Linux should continue to be able to
reset and program the PCIe PHY (as well as all other hw blocks).

> > 
> > > Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
> > > Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
> > > ---
> > >   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 91 +++++++++++++++---------
> > >   1 file changed, 58 insertions(+), 33 deletions(-)
> > > 

-- 
With best wishes
Dmitry

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

[Qiang Yu]

On 1/22/2025 5:43 PM, Dmitry Baryshkov wrote:
> On Wed, Jan 22, 2025 at 03:17:39PM +0800, Wenbin Yao (Consultant) wrote:
>> On 1/21/2025 6:36 PM, Dmitry Baryshkov wrote:
>>> On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@quicinc.com> wrote:
>>>> From: Qiang Yu <quic_qianyu@quicinc.com>
>>>>
>>>> Currently, BCR reset and PHY register setting are mandatory for every port
>>>> before link training. However, some QCOM PCIe PHYs support no_csr reset.
>>>> Different than BCR reset that is used to reset entire PHY including
>>>> hardware and register, once no_csr reset is toggled, only PHY hardware will
>>>> be reset but PHY registers will be retained,
>>> I'm sorry, I can't parse this.
>> The difference between no_csr reset and bcr reset is that no_csr reset
>> doesn't reset the phy registers. If a phy is enabled in UEFI, its registers
>> are programed. After Linux boot up, the registers will not be reset but
>> keep the value programmed by UEFI if we only do no_csr reset, so we can
>> skip phy setting.
> Please fix capitalization of the abbreviations (PHY, BCR) and add
> similar text to the commit message.
>
>>>> which means PHY setting can
>>>> be skipped during PHY init if PCIe link was enabled in booltloader and only
>>>> no_csr is toggled after that.
>>>>
>>>> Hence, determine whether the PHY has been enabled in bootloader by
>>>> verifying QPHY_START_CTRL register. If it is programmed and no_csr reset is
>>>> present, skip BCR reset and PHY register setting, so that PCIe link can be
>>>> established with no_csr reset only.
>>> This doesn't tell us why we want to do so. The general rule is not to
>>> depend on the bootloaders at all. The reason is pretty simple: it is
>>> hard to update bootloaders, while it is relatively easy to update the
>>> kernel. If the hardware team issues any kind of changes to the
>>> programming tables, the kernel will get them earlier than the
>>> bootloader.
>> With this change, we don't need to upstream phy setting for all phys
>> support no_csr reset, this will save a great deal of efforts and simplify
>> the phy driver. Our goal is to remove proprietary PCIe firmware operations
>> from kernel. PHY is just the start and will be followed by controller,
>> clocks, regulators, etc. If program table need to be changed, the place to
>> do that will be UEFI.
> Well, that sounds like a very bad idea. Please don't do that. Linux
> kernel drivers should not depend on the UEFI or a bootloader. Unless
> there is a good reason for that, Linux should continue to be able to
> reset and program the PCIe PHY (as well as all other hw blocks).
I'm wondering if it's really necessary for Linux to be able to program the
PHY. Perhaps Linux should only care about common aspects defined by the
PCIe spec like bus scanning, BAR space allocation, and functions provided
by other PCIe capabilities. As for the specific operations that are
different on various platforms, it might be more appropriate for the
firmware to take care of them. This way, the responsibilities can be more
clearly divided, and the driver could potentially be
more streamlined.

On the other hand, since the no_csr reset can retain register values,
maybe we should still make full use of it, even if we don't want to
rely on UEFI. For example, during runtime suspend/resume
(the D3cold -> D0 cycle), when re-initializing the PHY, same PHY
settings will be programmed again. This is a bit redundant.

Thanks,
Qiang
>>>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>>>> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
>>>> ---
>>>>    drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 91 +++++++++++++++---------
>>>>    1 file changed, 58 insertions(+), 33 deletions(-)
>>>>

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

[Manivannan Sadhasivam]

On Tue, Jan 21, 2025 at 05:41:39PM +0800, Wenbin Yao wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> Decide the in-driver logic based on whether the nocsr reset is present
> and defer checking the appropriateness of that to dt-bindings to save
> on boilerplate.
> 
> Reset controller APIs are fine consuming a nullptr, so no additional
> checks are necessary there.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> index 873f2f9844c6..ac42e4b01065 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> @@ -2793,8 +2793,6 @@ struct qmp_phy_cfg {
>  
>  	bool skip_start_delay;
>  
> -	bool has_nocsr_reset;
> -
>  	/* QMP PHY pipe clock interface rate */
>  	unsigned long pipe_clock_rate;
>  
> @@ -3685,7 +3683,6 @@ static const struct qmp_phy_cfg sm8550_qmp_gen4x2_pciephy_cfg = {
>  
>  	.pwrdn_ctrl		= SW_PWRDN | REFCLK_DRV_DSBL,
>  	.phy_status		= PHYSTATUS_4_20,
> -	.has_nocsr_reset	= true,
>  
>  	/* 20MHz PHY AUX Clock */
>  	.aux_clock_rate		= 20000000,
> @@ -3718,7 +3715,6 @@ static const struct qmp_phy_cfg sm8650_qmp_gen4x2_pciephy_cfg = {
>  
>  	.pwrdn_ctrl		= SW_PWRDN | REFCLK_DRV_DSBL,
>  	.phy_status		= PHYSTATUS_4_20,
> -	.has_nocsr_reset	= true,
>  
>  	/* 20MHz PHY AUX Clock */
>  	.aux_clock_rate		= 20000000,
> @@ -3836,7 +3832,6 @@ static const struct qmp_phy_cfg x1e80100_qmp_gen4x2_pciephy_cfg = {
>  
>  	.pwrdn_ctrl		= SW_PWRDN | REFCLK_DRV_DSBL,
>  	.phy_status		= PHYSTATUS_4_20,
> -	.has_nocsr_reset	= true,
>  };
>  
>  static const struct qmp_phy_cfg x1e80100_qmp_gen4x4_pciephy_cfg = {
> @@ -3870,7 +3865,6 @@ static const struct qmp_phy_cfg x1e80100_qmp_gen4x4_pciephy_cfg = {
>  
>  	.pwrdn_ctrl		= SW_PWRDN | REFCLK_DRV_DSBL,
>  	.phy_status		= PHYSTATUS_4_20,
> -	.has_nocsr_reset	= true,
>  };
>  
>  static const struct qmp_phy_cfg x1e80100_qmp_gen4x8_pciephy_cfg = {
> @@ -3902,7 +3896,6 @@ static const struct qmp_phy_cfg x1e80100_qmp_gen4x8_pciephy_cfg = {
>  
>  	.pwrdn_ctrl		= SW_PWRDN | REFCLK_DRV_DSBL,
>  	.phy_status		= PHYSTATUS_4_20,
> -	.has_nocsr_reset	= true,
>  };
>  
>  static void qmp_pcie_init_port_b(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tbls *tbls)
> @@ -4203,11 +4196,14 @@ static int qmp_pcie_reset_init(struct qmp_pcie *qmp)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "failed to get resets\n");
>  
> -	if (cfg->has_nocsr_reset) {
> -		qmp->nocsr_reset = devm_reset_control_get_exclusive(dev, "phy_nocsr");
> -		if (IS_ERR(qmp->nocsr_reset))
> +	qmp->nocsr_reset = devm_reset_control_get_exclusive(dev, "phy_nocsr");
> +	if (IS_ERR(qmp->nocsr_reset)) {
> +		if (PTR_ERR(qmp->nocsr_reset) == -ENOENT ||
> +		    PTR_ERR(qmp->nocsr_reset) == -EINVAL)
> +			qmp->nocsr_reset = NULL;
> +		else
>  			return dev_err_probe(dev, PTR_ERR(qmp->nocsr_reset),
> -						"failed to get no-csr reset\n");
> +					     "failed to get no-csr reset\n");
>  	}
>  
>  	return 0;
> -- 
> 2.34.1
> 

-- 
மணிவண்ணன் சதாசிவம்