[v1] phy: qcom: qmp-pcie: Fix PHY initialization when powered down by firmware

[PATCH] phy: qcom: qmp-pcie: Fix PHY initialization when powered down by firmware

Posted by Stephan Gerhold 1 month, 3 weeks ago

Commit 0cc22f5a861c ("phy: qcom: qmp-pcie: Add PHY register retention
support") added support for using the "no_csr" reset to skip configuration
of the PHY if the init sequence was already applied by the boot firmware.
The expectation is that the PHY is only turned on/off by using the "no_csr"
reset, instead of powering it down and re-programming it after a full
reset.

The boot firmware on X1E does not fully conform to this expectation: If the
PCIe3 link fails to come up (e.g. because no PCIe card is inserted), the
firmware powers down the PHY using the QPHY_PCS_POWER_DOWN_CONTROL
register. The QPHY_START_CTRL register is kept as-is, so the driver assumes
the PHY is already initialized and skips the configuration/power up
sequence. The PHY won't come up again without clearing the
QPHY_PCS_POWER_DOWN_CONTROL, so eventually initialization fails:

  qcom-qmp-pcie-phy 1be0000.phy: phy initialization timed-out
  phy phy-1be0000.phy.0: phy poweron failed --> -110
  qcom-pcie 1bd0000.pcie: cannot initialize host
  qcom-pcie 1bd0000.pcie: probe with driver qcom-pcie failed with error -110

This can be reliably reproduced on the X1E CRD, QCP and Devkit when no card
is inserted for PCIe3.

Fix this by checking the QPHY_PCS_POWER_DOWN_CONTROL register in addition
to QPHY_START_CTRL. If the PHY is powered down with the register, it
doesn't conform to the expectations for using the "no_csr" reset, so we
fully re-initialize with the normal reset sequence.

Cc: stable@vger.kernel.org
Fixes: 0cc22f5a861c ("phy: qcom: qmp-pcie: Add PHY register retention support")
Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
index 95830dcfdec9b1f68fd55d1cc3c102985cfafcc1..6a469a8f5ae7eba6e4d1d702efaae1c658c4321e 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
@@ -4339,10 +4339,12 @@ 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;
-	bool phy_initialized = !!(readl(pcs + cfg->regs[QPHY_START_CTRL]));
 	int ret;
 
-	qmp->skip_init = qmp->nocsr_reset && phy_initialized;
+	qmp->skip_init = qmp->nocsr_reset &&
+		readl(pcs + cfg->regs[QPHY_START_CTRL]) &&
+		readl(pcs + cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL]);
+
 	/*
 	 * We need to check the existence of init sequences in two cases:
 	 * 1. The PHY doesn't support no_csr reset.

---
base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
change-id: 20250812-phy-qcom-qmp-pcie-nocsr-fix-1603106294cd

Best regards,
-- 
Stephan Gerhold <stephan.gerhold@linaro.org>

Re: [PATCH] phy: qcom: qmp-pcie: Fix PHY initialization when powered down by firmware

Posted by Konrad Dybcio 1 month, 3 weeks ago

On 8/12/25 6:30 PM, Stephan Gerhold wrote:
> Commit 0cc22f5a861c ("phy: qcom: qmp-pcie: Add PHY register retention
> support") added support for using the "no_csr" reset to skip configuration
> of the PHY if the init sequence was already applied by the boot firmware.
> The expectation is that the PHY is only turned on/off by using the "no_csr"
> reset, instead of powering it down and re-programming it after a full
> reset.
> 
> The boot firmware on X1E does not fully conform to this expectation: If the
> PCIe3 link fails to come up (e.g. because no PCIe card is inserted), the
> firmware powers down the PHY using the QPHY_PCS_POWER_DOWN_CONTROL
> register. The QPHY_START_CTRL register is kept as-is, so the driver assumes
> the PHY is already initialized and skips the configuration/power up
> sequence. The PHY won't come up again without clearing the
> QPHY_PCS_POWER_DOWN_CONTROL, so eventually initialization fails:
> 
>   qcom-qmp-pcie-phy 1be0000.phy: phy initialization timed-out
>   phy phy-1be0000.phy.0: phy poweron failed --> -110
>   qcom-pcie 1bd0000.pcie: cannot initialize host
>   qcom-pcie 1bd0000.pcie: probe with driver qcom-pcie failed with error -110
> 
> This can be reliably reproduced on the X1E CRD, QCP and Devkit when no card
> is inserted for PCIe3.
> 
> Fix this by checking the QPHY_PCS_POWER_DOWN_CONTROL register in addition
> to QPHY_START_CTRL. If the PHY is powered down with the register, it
> doesn't conform to the expectations for using the "no_csr" reset, so we
> fully re-initialize with the normal reset sequence.
> 
> Cc: stable@vger.kernel.org
> Fixes: 0cc22f5a861c ("phy: qcom: qmp-pcie: Add PHY register retention support")
> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> index 95830dcfdec9b1f68fd55d1cc3c102985cfafcc1..6a469a8f5ae7eba6e4d1d702efaae1c658c4321e 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> @@ -4339,10 +4339,12 @@ 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;
> -	bool phy_initialized = !!(readl(pcs + cfg->regs[QPHY_START_CTRL]));
>  	int ret;
>  
> -	qmp->skip_init = qmp->nocsr_reset && phy_initialized;
> +	qmp->skip_init = qmp->nocsr_reset &&
> +		readl(pcs + cfg->regs[QPHY_START_CTRL]) &&
> +		readl(pcs + cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL]);

I think it would be good to ensure the value matches platform config
expectations, i.e. !(val & cfg->pwrdn_ctrl)

Konrad

Re: [PATCH] phy: qcom: qmp-pcie: Fix PHY initialization when powered down by firmware

Posted by Stephan Gerhold 1 month, 3 weeks ago

On Wed, Aug 13, 2025 at 12:13:31PM +0200, Konrad Dybcio wrote:
> On 8/12/25 6:30 PM, Stephan Gerhold wrote:
> > Commit 0cc22f5a861c ("phy: qcom: qmp-pcie: Add PHY register retention
> > support") added support for using the "no_csr" reset to skip configuration
> > of the PHY if the init sequence was already applied by the boot firmware.
> > The expectation is that the PHY is only turned on/off by using the "no_csr"
> > reset, instead of powering it down and re-programming it after a full
> > reset.
> > 
> > The boot firmware on X1E does not fully conform to this expectation: If the
> > PCIe3 link fails to come up (e.g. because no PCIe card is inserted), the
> > firmware powers down the PHY using the QPHY_PCS_POWER_DOWN_CONTROL
> > register. The QPHY_START_CTRL register is kept as-is, so the driver assumes
> > the PHY is already initialized and skips the configuration/power up
> > sequence. The PHY won't come up again without clearing the
> > QPHY_PCS_POWER_DOWN_CONTROL, so eventually initialization fails:
> > 
> >   qcom-qmp-pcie-phy 1be0000.phy: phy initialization timed-out
> >   phy phy-1be0000.phy.0: phy poweron failed --> -110
> >   qcom-pcie 1bd0000.pcie: cannot initialize host
> >   qcom-pcie 1bd0000.pcie: probe with driver qcom-pcie failed with error -110
> > 
> > This can be reliably reproduced on the X1E CRD, QCP and Devkit when no card
> > is inserted for PCIe3.
> > 
> > Fix this by checking the QPHY_PCS_POWER_DOWN_CONTROL register in addition
> > to QPHY_START_CTRL. If the PHY is powered down with the register, it
> > doesn't conform to the expectations for using the "no_csr" reset, so we
> > fully re-initialize with the normal reset sequence.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 0cc22f5a861c ("phy: qcom: qmp-pcie: Add PHY register retention support")
> > Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
> > ---
> >  drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > index 95830dcfdec9b1f68fd55d1cc3c102985cfafcc1..6a469a8f5ae7eba6e4d1d702efaae1c658c4321e 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > @@ -4339,10 +4339,12 @@ 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;
> > -	bool phy_initialized = !!(readl(pcs + cfg->regs[QPHY_START_CTRL]));
> >  	int ret;
> >  
> > -	qmp->skip_init = qmp->nocsr_reset && phy_initialized;
> > +	qmp->skip_init = qmp->nocsr_reset &&
> > +		readl(pcs + cfg->regs[QPHY_START_CTRL]) &&
> > +		readl(pcs + cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL]);
> 
> I think it would be good to ensure the value matches platform config
> expectations, i.e. !(val & cfg->pwrdn_ctrl)
> 

I think ((val & cfg->pwrdn_ctrl) == cfg->pwrdn_ctrl) is what you want,
to check if all the bits we would set are actually set? That sounds
reasonable, I'll send a v2 with that soon.

I'll make the same change for QPHY_START_CTRL, to have it consistent
(checking for SERDES_START | PCS_START, which is what we would set).

Thanks,
Stephan

Re: [PATCH] phy: qcom: qmp-pcie: Fix PHY initialization when powered down by firmware

Posted by Konrad Dybcio 1 month, 3 weeks ago

On 8/13/25 2:04 PM, Stephan Gerhold wrote:
> On Wed, Aug 13, 2025 at 12:13:31PM +0200, Konrad Dybcio wrote:
>> On 8/12/25 6:30 PM, Stephan Gerhold wrote:
>>> Commit 0cc22f5a861c ("phy: qcom: qmp-pcie: Add PHY register retention
>>> support") added support for using the "no_csr" reset to skip configuration
>>> of the PHY if the init sequence was already applied by the boot firmware.
>>> The expectation is that the PHY is only turned on/off by using the "no_csr"
>>> reset, instead of powering it down and re-programming it after a full
>>> reset.
>>>
>>> The boot firmware on X1E does not fully conform to this expectation: If the
>>> PCIe3 link fails to come up (e.g. because no PCIe card is inserted), the
>>> firmware powers down the PHY using the QPHY_PCS_POWER_DOWN_CONTROL
>>> register. The QPHY_START_CTRL register is kept as-is, so the driver assumes
>>> the PHY is already initialized and skips the configuration/power up
>>> sequence. The PHY won't come up again without clearing the
>>> QPHY_PCS_POWER_DOWN_CONTROL, so eventually initialization fails:
>>>
>>>   qcom-qmp-pcie-phy 1be0000.phy: phy initialization timed-out
>>>   phy phy-1be0000.phy.0: phy poweron failed --> -110
>>>   qcom-pcie 1bd0000.pcie: cannot initialize host
>>>   qcom-pcie 1bd0000.pcie: probe with driver qcom-pcie failed with error -110
>>>
>>> This can be reliably reproduced on the X1E CRD, QCP and Devkit when no card
>>> is inserted for PCIe3.
>>>
>>> Fix this by checking the QPHY_PCS_POWER_DOWN_CONTROL register in addition
>>> to QPHY_START_CTRL. If the PHY is powered down with the register, it
>>> doesn't conform to the expectations for using the "no_csr" reset, so we
>>> fully re-initialize with the normal reset sequence.
>>>
>>> Cc: stable@vger.kernel.org
>>> Fixes: 0cc22f5a861c ("phy: qcom: qmp-pcie: Add PHY register retention support")
>>> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
>>> ---
>>>  drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>> index 95830dcfdec9b1f68fd55d1cc3c102985cfafcc1..6a469a8f5ae7eba6e4d1d702efaae1c658c4321e 100644
>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>> @@ -4339,10 +4339,12 @@ 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;
>>> -	bool phy_initialized = !!(readl(pcs + cfg->regs[QPHY_START_CTRL]));
>>>  	int ret;
>>>  
>>> -	qmp->skip_init = qmp->nocsr_reset && phy_initialized;
>>> +	qmp->skip_init = qmp->nocsr_reset &&
>>> +		readl(pcs + cfg->regs[QPHY_START_CTRL]) &&
>>> +		readl(pcs + cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL]);
>>
>> I think it would be good to ensure the value matches platform config
>> expectations, i.e. !(val & cfg->pwrdn_ctrl)
>>
> 
> I think ((val & cfg->pwrdn_ctrl) == cfg->pwrdn_ctrl) is what you want,
> to check if all the bits we would set are actually set? That sounds
> reasonable, I'll send a v2 with that soon.

We can do that, currently the register (at least on 8750 that I randomly
checked) doesn't have any other bits..

> I'll make the same change for QPHY_START_CTRL, to have it consistent
> (checking for SERDES_START | PCS_START, which is what we would set).

Sounds reasonable

Konrad