[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


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

[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


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

[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


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

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
> 

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

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

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

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(-)
>>

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

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

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

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(-)
>>>>

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

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

[Manivannan Sadhasivam]

+ Mayank (with whom I discussed this topic internally)

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

It is not necessary in an ideal world, but what we have seen is Qcom releasing
updated PHY init sequence after upstreaming the initial PHY driver support. In
that case, the devices with old firmware will become outdated unless the fw is
updated (which is not straightforward compared to updating the kernel).

But, I do like this idea of reusing the PHY init sequence in the kernel. Though
we cannot just do it for all platforms. Maybe we can enable it on platforms like
compute starting from X1E and see how it goes? Just to minimize the impact if it
didn't go well.

> On the other hand, since the no_csr reset can retain register values,
> maybe we should still make full use of it, even if we don't want to
> rely on UEFI. For example, during runtime suspend/resume
> (the D3cold -> D0 cycle)

D3Cold during runtime suspend in bizarre.

>, when re-initializing the PHY, same PHY
> settings will be programmed again. This is a bit redundant.
> 

Hmm, what would happen if the CX collapse happens during system suspend? Will
the PHY registers be retained?

- Mani

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

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

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
> 

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

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

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

[Konrad Dybcio]

On 24.01.2025 8:08 AM, Manivannan Sadhasivam wrote:
> + Mayank (with whom I discussed this topic internally)
> 
> On Fri, Jan 24, 2025 at 02:22:01PM +0800, Qiang Yu wrote:
>>
>> On 1/22/2025 5:43 PM, Dmitry Baryshkov wrote:
>>> On Wed, Jan 22, 2025 at 03:17:39PM +0800, Wenbin Yao (Consultant) wrote:
>>>> On 1/21/2025 6:36 PM, Dmitry Baryshkov wrote:
>>>>> On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@quicinc.com> wrote:
>>>>>> From: Qiang Yu <quic_qianyu@quicinc.com>
>>>>>>
>>>>>> Currently, BCR reset and PHY register setting are mandatory for every port
>>>>>> before link training. However, some QCOM PCIe PHYs support no_csr reset.
>>>>>> Different than BCR reset that is used to reset entire PHY including
>>>>>> hardware and register, once no_csr reset is toggled, only PHY hardware will
>>>>>> be reset but PHY registers will be retained,
>>>>> I'm sorry, I can't parse this.
>>>> The difference between no_csr reset and bcr reset is that no_csr reset
>>>> doesn't reset the phy registers. If a phy is enabled in UEFI, its registers
>>>> are programed. After Linux boot up, the registers will not be reset but
>>>> keep the value programmed by UEFI if we only do no_csr reset, so we can
>>>> skip phy setting.
>>> Please fix capitalization of the abbreviations (PHY, BCR) and add
>>> similar text to the commit message.
>>>
>>>>>> which means PHY setting can
>>>>>> be skipped during PHY init if PCIe link was enabled in booltloader and only
>>>>>> no_csr is toggled after that.
>>>>>>
>>>>>> Hence, determine whether the PHY has been enabled in bootloader by
>>>>>> verifying QPHY_START_CTRL register. If it is programmed and no_csr reset is
>>>>>> present, skip BCR reset and PHY register setting, so that PCIe link can be
>>>>>> established with no_csr reset only.
>>>>> This doesn't tell us why we want to do so. The general rule is not to
>>>>> depend on the bootloaders at all. The reason is pretty simple: it is
>>>>> hard to update bootloaders, while it is relatively easy to update the
>>>>> kernel. If the hardware team issues any kind of changes to the
>>>>> programming tables, the kernel will get them earlier than the
>>>>> bootloader.

We're assuming that if a product has shipped, the sequences used to power up
the PHY in the bootloader (e.g. for NVMe) are already good.

If some tragedy happens and an erratum is needed, we can always introduce a
small override with the existing driver infrastructure (i.e. adding a new
entry with a couple registers worth of programming sequence, leaving the other
values in tact)


While updates to the PHY init sequences have happened in the past, I believe
they are mostly a result of oneOf:

* someone upstreaming a pre-release revision / premature release
* someone making a mistake when typing in the numbers

Although sometimes there may be an update that isn't a result of any dev fault.
It's worth to keep in mind certain values in there can be board specific, as
they affect the analog interface. But we haven't seen that being used much if
at all.

>>>> With this change, we don't need to upstream phy setting for all phys
>>>> support no_csr reset, this will save a great deal of efforts and simplify
>>>> the phy driver. Our goal is to remove proprietary PCIe firmware operations
>>>> from kernel. PHY is just the start and will be followed by controller,
>>>> clocks, regulators, etc. If program table need to be changed, the place to
>>>> do that will be UEFI.
>>> Well, that sounds like a very bad idea. Please don't do that. Linux
>>> kernel drivers should not depend on the UEFI or a bootloader. Unless
>>> there is a good reason for that, Linux should continue to be able to
>>> reset and program the PCIe PHY (as well as all other hw blocks).

While I personally like being able to look into what's being programmed
onto my hardware myself, this unfortunately does not scale, both from
maintainability and reviewability perspectives with tables that host
hundreds of essentially magic values.

Plus this approach makes it easier to extend to other OSes, without
polluting them with the same data.

>> I'm wondering if it's really necessary for Linux to be able to program the
>> PHY. Perhaps Linux should only care about common aspects defined by the
>> PCIe spec like bus scanning, BAR space allocation, and functions provided
>> by other PCIe capabilities. As for the specific operations that are
>> different on various platforms, it might be more appropriate for the
>> firmware to take care of them. This way, the responsibilities can be more
>> clearly divided, and the driver could potentially be
>> more streamlined.
>>
> 
> It is not necessary in an ideal world, but what we have seen is Qcom releasing
> updated PHY init sequence after upstreaming the initial PHY driver support. In
> that case, the devices with old firmware will become outdated unless the fw is
> updated (which is not straightforward compared to updating the kernel).
> 
> But, I do like this idea of reusing the PHY init sequence in the kernel. Though
> we cannot just do it for all platforms. Maybe we can enable it on platforms like
> compute starting from X1E and see how it goes? Just to minimize the impact if it
> didn't go well.

X1 is just the first of many here.

We can possibly move some existing ones over after confirming that:

a) the bootloader programs the PHYs at all
b) the values programmed are reasonable

on a case-by-case basis to ensure we're not regressing anything. Perhaps a
piece of code and a modparam could be added that would read out the values
present at boot and compare them to what the kernel has in store.


>> On the other hand, since the no_csr reset can retain register values,
>> maybe we should still make full use of it, even if we don't want to
>> rely on UEFI. For example, during runtime suspend/resume
>> (the D3cold -> D0 cycle)
> 
> D3Cold during runtime suspend in bizarre.

Very much so. But this extends to system suspend as well - we can still shave
a couple ms off the wakeup times if we can skip all the programming.

> 
>> , when re-initializing the PHY, same PHY
>> settings will be programmed again. This is a bit redundant.
>>
> 
> Hmm, what would happen if the CX collapse happens during system suspend? Will
> the PHY registers be retained?

PHYs are always(asterisk) backed by some flavor of MX instead

Konrad

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

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

[neil.armstrong]

On 25/01/2025 14:10, Konrad Dybcio wrote:
> On 24.01.2025 8:08 AM, Manivannan Sadhasivam wrote:
>> + Mayank (with whom I discussed this topic internally)
>>
>> On Fri, Jan 24, 2025 at 02:22:01PM +0800, Qiang Yu wrote:
>>>
>>> On 1/22/2025 5:43 PM, Dmitry Baryshkov wrote:
>>>> On Wed, Jan 22, 2025 at 03:17:39PM +0800, Wenbin Yao (Consultant) wrote:
>>>>> On 1/21/2025 6:36 PM, Dmitry Baryshkov wrote:
>>>>>> On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@quicinc.com> wrote:
>>>>>>> From: Qiang Yu <quic_qianyu@quicinc.com>
>>>>>>>
>>>>>>> Currently, BCR reset and PHY register setting are mandatory for every port
>>>>>>> before link training. However, some QCOM PCIe PHYs support no_csr reset.
>>>>>>> Different than BCR reset that is used to reset entire PHY including
>>>>>>> hardware and register, once no_csr reset is toggled, only PHY hardware will
>>>>>>> be reset but PHY registers will be retained,
>>>>>> I'm sorry, I can't parse this.
>>>>> The difference between no_csr reset and bcr reset is that no_csr reset
>>>>> doesn't reset the phy registers. If a phy is enabled in UEFI, its registers
>>>>> are programed. After Linux boot up, the registers will not be reset but
>>>>> keep the value programmed by UEFI if we only do no_csr reset, so we can
>>>>> skip phy setting.
>>>> Please fix capitalization of the abbreviations (PHY, BCR) and add
>>>> similar text to the commit message.
>>>>
>>>>>>> which means PHY setting can
>>>>>>> be skipped during PHY init if PCIe link was enabled in booltloader and only
>>>>>>> no_csr is toggled after that.
>>>>>>>
>>>>>>> Hence, determine whether the PHY has been enabled in bootloader by
>>>>>>> verifying QPHY_START_CTRL register. If it is programmed and no_csr reset is
>>>>>>> present, skip BCR reset and PHY register setting, so that PCIe link can be
>>>>>>> established with no_csr reset only.
>>>>>> This doesn't tell us why we want to do so. The general rule is not to
>>>>>> depend on the bootloaders at all. The reason is pretty simple: it is
>>>>>> hard to update bootloaders, while it is relatively easy to update the
>>>>>> kernel. If the hardware team issues any kind of changes to the
>>>>>> programming tables, the kernel will get them earlier than the
>>>>>> bootloader.
> 
> We're assuming that if a product has shipped, the sequences used to power up
> the PHY in the bootloader (e.g. for NVMe) are already good.
> 
> If some tragedy happens and an erratum is needed, we can always introduce a
> small override with the existing driver infrastructure (i.e. adding a new
> entry with a couple registers worth of programming sequence, leaving the other
> values in tact)

Assuming Linux will be always ran directly after the bootloader is a wild assumption.

Yes, we should make use the noscr if the PHY is always programmed, but we should be
always able to reprogram the PHY entirely to recover a faulty programmation.

Neil


<snip>


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

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

[Konrad Dybcio]

On 29.01.2025 9:29 AM, neil.armstrong@linaro.org wrote:
> On 25/01/2025 14:10, Konrad Dybcio wrote:
>> On 24.01.2025 8:08 AM, Manivannan Sadhasivam wrote:
>>> + Mayank (with whom I discussed this topic internally)
>>>
>>> On Fri, Jan 24, 2025 at 02:22:01PM +0800, Qiang Yu wrote:
>>>>
>>>> On 1/22/2025 5:43 PM, Dmitry Baryshkov wrote:
>>>>> On Wed, Jan 22, 2025 at 03:17:39PM +0800, Wenbin Yao (Consultant) wrote:
>>>>>> On 1/21/2025 6:36 PM, Dmitry Baryshkov wrote:
>>>>>>> On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@quicinc.com> wrote:
>>>>>>>> From: Qiang Yu <quic_qianyu@quicinc.com>
>>>>>>>>
>>>>>>>> Currently, BCR reset and PHY register setting are mandatory for every port
>>>>>>>> before link training. However, some QCOM PCIe PHYs support no_csr reset.
>>>>>>>> Different than BCR reset that is used to reset entire PHY including
>>>>>>>> hardware and register, once no_csr reset is toggled, only PHY hardware will
>>>>>>>> be reset but PHY registers will be retained,
>>>>>>> I'm sorry, I can't parse this.
>>>>>> The difference between no_csr reset and bcr reset is that no_csr reset
>>>>>> doesn't reset the phy registers. If a phy is enabled in UEFI, its registers
>>>>>> are programed. After Linux boot up, the registers will not be reset but
>>>>>> keep the value programmed by UEFI if we only do no_csr reset, so we can
>>>>>> skip phy setting.
>>>>> Please fix capitalization of the abbreviations (PHY, BCR) and add
>>>>> similar text to the commit message.
>>>>>
>>>>>>>> which means PHY setting can
>>>>>>>> be skipped during PHY init if PCIe link was enabled in booltloader and only
>>>>>>>> no_csr is toggled after that.
>>>>>>>>
>>>>>>>> Hence, determine whether the PHY has been enabled in bootloader by
>>>>>>>> verifying QPHY_START_CTRL register. If it is programmed and no_csr reset is
>>>>>>>> present, skip BCR reset and PHY register setting, so that PCIe link can be
>>>>>>>> established with no_csr reset only.
>>>>>>> This doesn't tell us why we want to do so. The general rule is not to
>>>>>>> depend on the bootloaders at all. The reason is pretty simple: it is
>>>>>>> hard to update bootloaders, while it is relatively easy to update the
>>>>>>> kernel. If the hardware team issues any kind of changes to the
>>>>>>> programming tables, the kernel will get them earlier than the
>>>>>>> bootloader.
>>
>> We're assuming that if a product has shipped, the sequences used to power up
>> the PHY in the bootloader (e.g. for NVMe) are already good.
>>
>> If some tragedy happens and an erratum is needed, we can always introduce a
>> small override with the existing driver infrastructure (i.e. adding a new
>> entry with a couple registers worth of programming sequence, leaving the other
>> values in tact)
> 
> Assuming Linux will be always ran directly after the bootloader is a wild assumption.

Situations like

[normal boot chain] -> [... (resets the PHY and doesn't reprogram it)] -> Linux

are both so unlikely and so intentional-by-the-user that it doesn't seem
worth considering really.

If whatever sits in the middle *must* hard-reset the phy, it can save the
register state beforehand and restore them after the reset

> Yes, we should make use the noscr if the PHY is always programmed, but we should be
> always able to reprogram the PHY entirely to recover a faulty programmation.

We aren't considering any possibility of faulty programming - it's either
programmed, or not. And if the values configured by the bootloader are wrong,
the device's firmware is considered faulty.

Most devices probably follow the exact same magic values as our reference
boards (though these values relate to analog characteristics, so perhaps not
*all* of them, which is another argument for keeping the BL state) and these
are extensively tested internally before any production devices make it out
the door. Any updates deep into the product life are most likely just "nice
to have"s and not anything critical, and as I've mentioned, we can still have
overrides with the current logic inside this driver.

Konrad

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

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

[neil.armstrong]

On 29/01/2025 12:29, Konrad Dybcio wrote:
> On 29.01.2025 9:29 AM, neil.armstrong@linaro.org wrote:
>> On 25/01/2025 14:10, Konrad Dybcio wrote:
>>> On 24.01.2025 8:08 AM, Manivannan Sadhasivam wrote:
>>>> + Mayank (with whom I discussed this topic internally)
>>>>
>>>> On Fri, Jan 24, 2025 at 02:22:01PM +0800, Qiang Yu wrote:
>>>>>
>>>>> On 1/22/2025 5:43 PM, Dmitry Baryshkov wrote:
>>>>>> On Wed, Jan 22, 2025 at 03:17:39PM +0800, Wenbin Yao (Consultant) wrote:
>>>>>>> On 1/21/2025 6:36 PM, Dmitry Baryshkov wrote:
>>>>>>>> On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@quicinc.com> wrote:
>>>>>>>>> From: Qiang Yu <quic_qianyu@quicinc.com>
>>>>>>>>>
>>>>>>>>> Currently, BCR reset and PHY register setting are mandatory for every port
>>>>>>>>> before link training. However, some QCOM PCIe PHYs support no_csr reset.
>>>>>>>>> Different than BCR reset that is used to reset entire PHY including
>>>>>>>>> hardware and register, once no_csr reset is toggled, only PHY hardware will
>>>>>>>>> be reset but PHY registers will be retained,
>>>>>>>> I'm sorry, I can't parse this.
>>>>>>> The difference between no_csr reset and bcr reset is that no_csr reset
>>>>>>> doesn't reset the phy registers. If a phy is enabled in UEFI, its registers
>>>>>>> are programed. After Linux boot up, the registers will not be reset but
>>>>>>> keep the value programmed by UEFI if we only do no_csr reset, so we can
>>>>>>> skip phy setting.
>>>>>> Please fix capitalization of the abbreviations (PHY, BCR) and add
>>>>>> similar text to the commit message.
>>>>>>
>>>>>>>>> which means PHY setting can
>>>>>>>>> be skipped during PHY init if PCIe link was enabled in booltloader and only
>>>>>>>>> no_csr is toggled after that.
>>>>>>>>>
>>>>>>>>> Hence, determine whether the PHY has been enabled in bootloader by
>>>>>>>>> verifying QPHY_START_CTRL register. If it is programmed and no_csr reset is
>>>>>>>>> present, skip BCR reset and PHY register setting, so that PCIe link can be
>>>>>>>>> established with no_csr reset only.
>>>>>>>> This doesn't tell us why we want to do so. The general rule is not to
>>>>>>>> depend on the bootloaders at all. The reason is pretty simple: it is
>>>>>>>> hard to update bootloaders, while it is relatively easy to update the
>>>>>>>> kernel. If the hardware team issues any kind of changes to the
>>>>>>>> programming tables, the kernel will get them earlier than the
>>>>>>>> bootloader.
>>>
>>> We're assuming that if a product has shipped, the sequences used to power up
>>> the PHY in the bootloader (e.g. for NVMe) are already good.
>>>
>>> If some tragedy happens and an erratum is needed, we can always introduce a
>>> small override with the existing driver infrastructure (i.e. adding a new
>>> entry with a couple registers worth of programming sequence, leaving the other
>>> values in tact)
>>
>> Assuming Linux will be always ran directly after the bootloader is a wild assumption.
> 
> Situations like
> 
> [normal boot chain] -> [... (resets the PHY and doesn't reprogram it)] -> Linux
> 
> are both so unlikely and so intentional-by-the-user that it doesn't seem
> worth considering really.

In embedded/mobile/edge world, definitely, in compute/PC-like market, not really.

You'll have people add some custom bootloaders, hypervisors, who knows what...

> 
> If whatever sits in the middle *must* hard-reset the phy, it can save the
> register state beforehand and restore them after the reset
> 
>> Yes, we should make use the noscr if the PHY is always programmed, but we should be
>> always able to reprogram the PHY entirely to recover a faulty programmation.
> 
> We aren't considering any possibility of faulty programming - it's either
> programmed, or not. And if the values configured by the bootloader are wrong,
> the device's firmware is considered faulty.
> 
> Most devices probably follow the exact same magic values as our reference
> boards (though these values relate to analog characteristics, so perhaps not
> *all* of them, which is another argument for keeping the BL state) and these
> are extensively tested internally before any production devices make it out
> the door. Any updates deep into the product life are most likely just "nice
> to have"s and not anything critical, and as I've mentioned, we can still have
> overrides with the current logic inside this driver.
> 
> Konrad


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

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

[Konrad Dybcio]

On 29.01.2025 2:41 PM, neil.armstrong@linaro.org wrote:
> On 29/01/2025 12:29, Konrad Dybcio wrote:
>> On 29.01.2025 9:29 AM, neil.armstrong@linaro.org wrote:
>>> On 25/01/2025 14:10, Konrad Dybcio wrote:
>>>> On 24.01.2025 8:08 AM, Manivannan Sadhasivam wrote:
>>>>> + Mayank (with whom I discussed this topic internally)
>>>>>
>>>>> On Fri, Jan 24, 2025 at 02:22:01PM +0800, Qiang Yu wrote:
>>>>>>
>>>>>> On 1/22/2025 5:43 PM, Dmitry Baryshkov wrote:
>>>>>>> On Wed, Jan 22, 2025 at 03:17:39PM +0800, Wenbin Yao (Consultant) wrote:
>>>>>>>> On 1/21/2025 6:36 PM, Dmitry Baryshkov wrote:
>>>>>>>>> On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@quicinc.com> wrote:
>>>>>>>>>> From: Qiang Yu <quic_qianyu@quicinc.com>
>>>>>>>>>>
>>>>>>>>>> Currently, BCR reset and PHY register setting are mandatory for every port
>>>>>>>>>> before link training. However, some QCOM PCIe PHYs support no_csr reset.
>>>>>>>>>> Different than BCR reset that is used to reset entire PHY including
>>>>>>>>>> hardware and register, once no_csr reset is toggled, only PHY hardware will
>>>>>>>>>> be reset but PHY registers will be retained,
>>>>>>>>> I'm sorry, I can't parse this.
>>>>>>>> The difference between no_csr reset and bcr reset is that no_csr reset
>>>>>>>> doesn't reset the phy registers. If a phy is enabled in UEFI, its registers
>>>>>>>> are programed. After Linux boot up, the registers will not be reset but
>>>>>>>> keep the value programmed by UEFI if we only do no_csr reset, so we can
>>>>>>>> skip phy setting.
>>>>>>> Please fix capitalization of the abbreviations (PHY, BCR) and add
>>>>>>> similar text to the commit message.
>>>>>>>
>>>>>>>>>> which means PHY setting can
>>>>>>>>>> be skipped during PHY init if PCIe link was enabled in booltloader and only
>>>>>>>>>> no_csr is toggled after that.
>>>>>>>>>>
>>>>>>>>>> Hence, determine whether the PHY has been enabled in bootloader by
>>>>>>>>>> verifying QPHY_START_CTRL register. If it is programmed and no_csr reset is
>>>>>>>>>> present, skip BCR reset and PHY register setting, so that PCIe link can be
>>>>>>>>>> established with no_csr reset only.
>>>>>>>>> This doesn't tell us why we want to do so. The general rule is not to
>>>>>>>>> depend on the bootloaders at all. The reason is pretty simple: it is
>>>>>>>>> hard to update bootloaders, while it is relatively easy to update the
>>>>>>>>> kernel. If the hardware team issues any kind of changes to the
>>>>>>>>> programming tables, the kernel will get them earlier than the
>>>>>>>>> bootloader.
>>>>
>>>> We're assuming that if a product has shipped, the sequences used to power up
>>>> the PHY in the bootloader (e.g. for NVMe) are already good.
>>>>
>>>> If some tragedy happens and an erratum is needed, we can always introduce a
>>>> small override with the existing driver infrastructure (i.e. adding a new
>>>> entry with a couple registers worth of programming sequence, leaving the other
>>>> values in tact)
>>>
>>> Assuming Linux will be always ran directly after the bootloader is a wild assumption.
>>
>> Situations like
>>
>> [normal boot chain] -> [... (resets the PHY and doesn't reprogram it)] -> Linux
>>
>> are both so unlikely and so intentional-by-the-user that it doesn't seem
>> worth considering really.
> 
> In embedded/mobile/edge world, definitely, in compute/PC-like market, not really.
> 
> You'll have people add some custom bootloaders, hypervisors, who knows what...

I see, however you actually have to intentionally assert the non-NO_CSR PHY
reset from said custom bootloaders, hypervisors and whoknowswhats for the
programmed sequence to be erased. So I have no idea what the issue is here.

Konrad

> 
>>
>> If whatever sits in the middle *must* hard-reset the phy, it can save the
>> register state beforehand and restore them after the reset
>>
>>> Yes, we should make use the noscr if the PHY is always programmed, but we should be
>>> always able to reprogram the PHY entirely to recover a faulty programmation.
>>
>> We aren't considering any possibility of faulty programming - it's either
>> programmed, or not. And if the values configured by the bootloader are wrong,
>> the device's firmware is considered faulty.
>>
>> Most devices probably follow the exact same magic values as our reference
>> boards (though these values relate to analog characteristics, so perhaps not
>> *all* of them, which is another argument for keeping the BL state) and these
>> are extensively tested internally before any production devices make it out
>> the door. Any updates deep into the product life are most likely just "nice
>> to have"s and not anything critical, and as I've mentioned, we can still have
>> overrides with the current logic inside this driver.
>>
>> Konrad
> 

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

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

[neil.armstrong]

On 29/01/2025 14:55, Konrad Dybcio wrote:
> On 29.01.2025 2:41 PM, neil.armstrong@linaro.org wrote:
>> On 29/01/2025 12:29, Konrad Dybcio wrote:
>>> On 29.01.2025 9:29 AM, neil.armstrong@linaro.org wrote:
>>>> On 25/01/2025 14:10, Konrad Dybcio wrote:
>>>>> On 24.01.2025 8:08 AM, Manivannan Sadhasivam wrote:
>>>>>> + Mayank (with whom I discussed this topic internally)
>>>>>>
>>>>>> On Fri, Jan 24, 2025 at 02:22:01PM +0800, Qiang Yu wrote:
>>>>>>>
>>>>>>> On 1/22/2025 5:43 PM, Dmitry Baryshkov wrote:
>>>>>>>> On Wed, Jan 22, 2025 at 03:17:39PM +0800, Wenbin Yao (Consultant) wrote:
>>>>>>>>> On 1/21/2025 6:36 PM, Dmitry Baryshkov wrote:
>>>>>>>>>> On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@quicinc.com> wrote:
>>>>>>>>>>> From: Qiang Yu <quic_qianyu@quicinc.com>
>>>>>>>>>>>
>>>>>>>>>>> Currently, BCR reset and PHY register setting are mandatory for every port
>>>>>>>>>>> before link training. However, some QCOM PCIe PHYs support no_csr reset.
>>>>>>>>>>> Different than BCR reset that is used to reset entire PHY including
>>>>>>>>>>> hardware and register, once no_csr reset is toggled, only PHY hardware will
>>>>>>>>>>> be reset but PHY registers will be retained,
>>>>>>>>>> I'm sorry, I can't parse this.
>>>>>>>>> The difference between no_csr reset and bcr reset is that no_csr reset
>>>>>>>>> doesn't reset the phy registers. If a phy is enabled in UEFI, its registers
>>>>>>>>> are programed. After Linux boot up, the registers will not be reset but
>>>>>>>>> keep the value programmed by UEFI if we only do no_csr reset, so we can
>>>>>>>>> skip phy setting.
>>>>>>>> Please fix capitalization of the abbreviations (PHY, BCR) and add
>>>>>>>> similar text to the commit message.
>>>>>>>>
>>>>>>>>>>> which means PHY setting can
>>>>>>>>>>> be skipped during PHY init if PCIe link was enabled in booltloader and only
>>>>>>>>>>> no_csr is toggled after that.
>>>>>>>>>>>
>>>>>>>>>>> Hence, determine whether the PHY has been enabled in bootloader by
>>>>>>>>>>> verifying QPHY_START_CTRL register. If it is programmed and no_csr reset is
>>>>>>>>>>> present, skip BCR reset and PHY register setting, so that PCIe link can be
>>>>>>>>>>> established with no_csr reset only.
>>>>>>>>>> This doesn't tell us why we want to do so. The general rule is not to
>>>>>>>>>> depend on the bootloaders at all. The reason is pretty simple: it is
>>>>>>>>>> hard to update bootloaders, while it is relatively easy to update the
>>>>>>>>>> kernel. If the hardware team issues any kind of changes to the
>>>>>>>>>> programming tables, the kernel will get them earlier than the
>>>>>>>>>> bootloader.
>>>>>
>>>>> We're assuming that if a product has shipped, the sequences used to power up
>>>>> the PHY in the bootloader (e.g. for NVMe) are already good.
>>>>>
>>>>> If some tragedy happens and an erratum is needed, we can always introduce a
>>>>> small override with the existing driver infrastructure (i.e. adding a new
>>>>> entry with a couple registers worth of programming sequence, leaving the other
>>>>> values in tact)
>>>>
>>>> Assuming Linux will be always ran directly after the bootloader is a wild assumption.
>>>
>>> Situations like
>>>
>>> [normal boot chain] -> [... (resets the PHY and doesn't reprogram it)] -> Linux
>>>
>>> are both so unlikely and so intentional-by-the-user that it doesn't seem
>>> worth considering really.
>>
>> In embedded/mobile/edge world, definitely, in compute/PC-like market, not really.
>>
>> You'll have people add some custom bootloaders, hypervisors, who knows what...
> 
> I see, however you actually have to intentionally assert the non-NO_CSR PHY
> reset from said custom bootloaders, hypervisors and whoknowswhats for the
> programmed sequence to be erased. So I have no idea what the issue is here.

I won't argue further, but you know as I do that relying on the bootloader state
is a dangerous game, and we already rely a lot for dsp stuff and we have
a lot lot of issue related to the UEFI implementation already on production
devices.

I'm not against the nocsr stuff, which can be a big win for boot time, but
honestly not adding a few registers in table seems risky enough, and we should
probably delay this experiment until we are sure the nocsr stuff works fine.

Neil

> 
> Konrad
> 
>>
>>>
>>> If whatever sits in the middle *must* hard-reset the phy, it can save the
>>> register state beforehand and restore them after the reset
>>>
>>>> Yes, we should make use the noscr if the PHY is always programmed, but we should be
>>>> always able to reprogram the PHY entirely to recover a faulty programmation.
>>>
>>> We aren't considering any possibility of faulty programming - it's either
>>> programmed, or not. And if the values configured by the bootloader are wrong,
>>> the device's firmware is considered faulty.
>>>
>>> Most devices probably follow the exact same magic values as our reference
>>> boards (though these values relate to analog characteristics, so perhaps not
>>> *all* of them, which is another argument for keeping the BL state) and these
>>> are extensively tested internally before any production devices make it out
>>> the door. Any updates deep into the product life are most likely just "nice
>>> to have"s and not anything critical, and as I've mentioned, we can still have
>>> overrides with the current logic inside this driver.
>>>
>>> Konrad
>>


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

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

[Konrad Dybcio]

On 29.01.2025 3:19 PM, neil.armstrong@linaro.org wrote:
> On 29/01/2025 14:55, Konrad Dybcio wrote:
>> On 29.01.2025 2:41 PM, neil.armstrong@linaro.org wrote:
>>> On 29/01/2025 12:29, Konrad Dybcio wrote:
>>>> On 29.01.2025 9:29 AM, neil.armstrong@linaro.org wrote:
>>>>> On 25/01/2025 14:10, Konrad Dybcio wrote:
>>>>>> On 24.01.2025 8:08 AM, Manivannan Sadhasivam wrote:
>>>>>>> + Mayank (with whom I discussed this topic internally)
>>>>>>>
>>>>>>> On Fri, Jan 24, 2025 at 02:22:01PM +0800, Qiang Yu wrote:
>>>>>>>>
>>>>>>>> On 1/22/2025 5:43 PM, Dmitry Baryshkov wrote:
>>>>>>>>> On Wed, Jan 22, 2025 at 03:17:39PM +0800, Wenbin Yao (Consultant) wrote:
>>>>>>>>>> On 1/21/2025 6:36 PM, Dmitry Baryshkov wrote:
>>>>>>>>>>> On Tue, 21 Jan 2025 at 11:43, Wenbin Yao <quic_wenbyao@quicinc.com> wrote:
>>>>>>>>>>>> From: Qiang Yu <quic_qianyu@quicinc.com>
>>>>>>>>>>>>
>>>>>>>>>>>> Currently, BCR reset and PHY register setting are mandatory for every port
>>>>>>>>>>>> before link training. However, some QCOM PCIe PHYs support no_csr reset.
>>>>>>>>>>>> Different than BCR reset that is used to reset entire PHY including
>>>>>>>>>>>> hardware and register, once no_csr reset is toggled, only PHY hardware will
>>>>>>>>>>>> be reset but PHY registers will be retained,
>>>>>>>>>>> I'm sorry, I can't parse this.
>>>>>>>>>> The difference between no_csr reset and bcr reset is that no_csr reset
>>>>>>>>>> doesn't reset the phy registers. If a phy is enabled in UEFI, its registers
>>>>>>>>>> are programed. After Linux boot up, the registers will not be reset but
>>>>>>>>>> keep the value programmed by UEFI if we only do no_csr reset, so we can
>>>>>>>>>> skip phy setting.
>>>>>>>>> Please fix capitalization of the abbreviations (PHY, BCR) and add
>>>>>>>>> similar text to the commit message.
>>>>>>>>>
>>>>>>>>>>>> which means PHY setting can
>>>>>>>>>>>> be skipped during PHY init if PCIe link was enabled in booltloader and only
>>>>>>>>>>>> no_csr is toggled after that.
>>>>>>>>>>>>
>>>>>>>>>>>> Hence, determine whether the PHY has been enabled in bootloader by
>>>>>>>>>>>> verifying QPHY_START_CTRL register. If it is programmed and no_csr reset is
>>>>>>>>>>>> present, skip BCR reset and PHY register setting, so that PCIe link can be
>>>>>>>>>>>> established with no_csr reset only.
>>>>>>>>>>> This doesn't tell us why we want to do so. The general rule is not to
>>>>>>>>>>> depend on the bootloaders at all. The reason is pretty simple: it is
>>>>>>>>>>> hard to update bootloaders, while it is relatively easy to update the
>>>>>>>>>>> kernel. If the hardware team issues any kind of changes to the
>>>>>>>>>>> programming tables, the kernel will get them earlier than the
>>>>>>>>>>> bootloader.
>>>>>>
>>>>>> We're assuming that if a product has shipped, the sequences used to power up
>>>>>> the PHY in the bootloader (e.g. for NVMe) are already good.
>>>>>>
>>>>>> If some tragedy happens and an erratum is needed, we can always introduce a
>>>>>> small override with the existing driver infrastructure (i.e. adding a new
>>>>>> entry with a couple registers worth of programming sequence, leaving the other
>>>>>> values in tact)
>>>>>
>>>>> Assuming Linux will be always ran directly after the bootloader is a wild assumption.
>>>>
>>>> Situations like
>>>>
>>>> [normal boot chain] -> [... (resets the PHY and doesn't reprogram it)] -> Linux
>>>>
>>>> are both so unlikely and so intentional-by-the-user that it doesn't seem
>>>> worth considering really.
>>>
>>> In embedded/mobile/edge world, definitely, in compute/PC-like market, not really.
>>>
>>> You'll have people add some custom bootloaders, hypervisors, who knows what...
>>
>> I see, however you actually have to intentionally assert the non-NO_CSR PHY
>> reset from said custom bootloaders, hypervisors and whoknowswhats for the
>> programmed sequence to be erased. So I have no idea what the issue is here.
> 
> I won't argue further, but you know as I do that relying on the bootloader state
> is a dangerous game, and we already rely a lot for dsp stuff and we have
> a lot lot of issue related to the UEFI implementation already on production
> devices.
> 
> I'm not against the nocsr stuff, which can be a big win for boot time, but
> honestly not adding a few registers in table seems risky enough, and we should
> probably delay this experiment until we are sure the nocsr stuff works fine.

I tested a range of mobile/compute platforms and only the latter kept
the PCIe PHYs initialized after dropping to the OS. No adverse effects
that I can tell.

Konrad

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