On Thu, Aug 14, 2025 at 05:26:05PM -0500, Bjorn Andersson wrote:
> On Thu, Aug 14, 2025 at 11:27:10AM +0200, 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.
> >
> > Also check the register more carefully to ensure all of the bits we expect
> > are actually set. A simple !!(readl()) is not enough, because the PHY might
> > be only partially set up with some of the expected bits set.
> >
> > Cc: stable@xxxxxxxxxxxxxxx
> > Fixes: 0cc22f5a861c ("phy: qcom: qmp-pcie: Add PHY register retention support")
> > Signed-off-by: Stephan Gerhold <stephan.gerhold@xxxxxxxxxx>
> > ---
> > Changes in v2:
> > - Ensure that all expected bits are set (Konrad)
> > - Link to v1: https://lore.kernel.org/r/20250812-phy-qcom-qmp-pcie-nocsr-fix-v1-1-9a7d0a5d2b46@xxxxxxxxxx
> > ---
> > drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 14 ++++++++++++--
> > 1 file changed, 12 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..80973527fafcb294273dff1864828532dab738db 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > @@ -3067,6 +3067,14 @@ struct qmp_pcie {
> > struct clk_fixed_rate aux_clk_fixed;
> > };
> >
> > +static bool qphy_checkbits(const void __iomem *base, u32 offset, u32 val)
> > +{
> > + u32 reg;
> > +
> > + reg = readl(base + offset);
> > + return (reg & val) == val;
> > +}
> > +
> > static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
> > {
> > u32 reg;
> > @@ -4339,10 +4347,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 &&
> > + qphy_checkbits(pcs, cfg->regs[QPHY_START_CTRL], SERDES_START | PCS_START) &&
> > + qphy_checkbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], cfg->pwrdn_ctrl);
>
> IMHO the "phy_initialized" variable does provide valuable context to
> what those (now) two lines represents. That is particularly relevant as
> the second one is active low...so at least I need to think a bit extra
> to understand what's going on.
>
I dropped the "phy_initialized" variable mainly because it didn't "look
good" together with the line wrapping of the two new longer lines. :-)
Perhaps it would already help to reuse and clarify the comment block
below, like this?
Thanks,
Stephan
@@ -4339,16 +4347,21 @@ 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;
/*
- * We need to check the existence of init sequences in two cases:
- * 1. The PHY doesn't support no_csr reset.
- * 2. The PHY supports no_csr reset but isn't initialized by bootloader.
- * As we can't skip init in these two cases.
+ * We can skip PHY initialization if all of the following conditions
+ * are met:
+ * 1. The PHY supports the nocsr_reset that preserves the PHY config.
+ * 2. The PHY was started (and not powered down again) by the
+ * bootloader, with all of the expected bits set correctly.
+ * In this case, we can continue without having the init sequence
+ * defined in the driver.
*/
+ qmp->skip_init = qmp->nocsr_reset &&
+ qphy_checkbits(pcs, cfg->regs[QPHY_START_CTRL], SERDES_START | PCS_START) &&
+ qphy_checkbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], cfg->pwrdn_ctrl);
+
if (!qmp->skip_init && !cfg->tbls.serdes_num) {
dev_err(qmp->dev, "Init sequence not available\n");
return -ENODATA;
