| Message ID | 1711741835-10044-1-git-send-email-quic_khsieh@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v3] phy/qcom-qmp-combo: propagate correct return value at phy_power_on() | expand |
Dmitry, Any more comments? On 3/29/2024 12:50 PM, Kuogee Hsieh wrote: > Currently qmp_combo_dp_power_on() always return 0 in regardless of > return value of cfg->configure_dp_phy(). This patch propagate > return value of cfg->configure_dp_phy() all the way back to caller. > > Changes in V3: > -- add v2 changes log > > Changes in V2: > -- add Fixes tag > -- add dev_err() to qmp_v3_configure_dp_phy() > -- add dev_err() to qmp_v4_configure_dp_phy() > > Fixes: 52e013d0bffa ("phy: qcom-qmp: Add support for DP in USB3+DP combo phy") > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > --- > drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > index 36632fa..513d99d 100644 > --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > @@ -2343,8 +2343,10 @@ static int qmp_v3_configure_dp_phy(struct qmp_combo *qmp) > writel(0x05, qmp->dp_dp_phy + QSERDES_V3_DP_PHY_TX2_TX3_LANE_CTL); > > ret = qmp_combo_configure_dp_clocks(qmp); > - if (ret) > + if (ret) { > + dev_err(qmp->dev, "dp phy configure failed, err=%d\n", ret); > return ret; > + } > > writel(0x04, qmp->dp_dp_phy + QSERDES_DP_PHY_AUX_CFG2); > writel(0x01, qmp->dp_dp_phy + QSERDES_DP_PHY_CFG); > @@ -2519,8 +2521,10 @@ static int qmp_v4_configure_dp_phy(struct qmp_combo *qmp) > int ret; > > ret = qmp_v456_configure_dp_phy(qmp); > - if (ret < 0) > + if (ret < 0) { > + dev_err(qmp->dev, "dp phy configure failed, err=%d\n", ret); > return ret; > + } > > /* > * At least for 7nm DP PHY this has to be done after enabling link > @@ -2754,6 +2758,7 @@ static int qmp_combo_dp_power_on(struct phy *phy) > const struct qmp_phy_cfg *cfg = qmp->cfg; > void __iomem *tx = qmp->dp_tx; > void __iomem *tx2 = qmp->dp_tx2; > + int ret; > > mutex_lock(&qmp->phy_mutex); > > @@ -2766,11 +2771,11 @@ static int qmp_combo_dp_power_on(struct phy *phy) > cfg->configure_dp_tx(qmp); > > /* Configure link rate, swing, etc. */ > - cfg->configure_dp_phy(qmp); > + ret = cfg->configure_dp_phy(qmp); > > mutex_unlock(&qmp->phy_mutex); > > - return 0; > + return ret; > } > > static int qmp_combo_dp_power_off(struct phy *phy)
On Wed, Apr 03, 2024 at 10:22:37AM -0700, Kuogee Hsieh wrote: > Dmitry, > > Any more comments? > > On 3/29/2024 12:50 PM, Kuogee Hsieh wrote: > > Currently qmp_combo_dp_power_on() always return 0 in regardless of > > return value of cfg->configure_dp_phy(). This patch propagate > > return value of cfg->configure_dp_phy() all the way back to caller. > > > > Changes in V3: > > -- add v2 changes log > > > > Changes in V2: > > -- add Fixes tag > > -- add dev_err() to qmp_v3_configure_dp_phy() > > -- add dev_err() to qmp_v4_configure_dp_phy() > > > > Fixes: 52e013d0bffa ("phy: qcom-qmp: Add support for DP in USB3+DP combo phy") > > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > > --- > > drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 13 +++++++++---- > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > > index 36632fa..513d99d 100644 > > --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > > @@ -2343,8 +2343,10 @@ static int qmp_v3_configure_dp_phy(struct qmp_combo *qmp) > > writel(0x05, qmp->dp_dp_phy + QSERDES_V3_DP_PHY_TX2_TX3_LANE_CTL); > > ret = qmp_combo_configure_dp_clocks(qmp); > > - if (ret) > > + if (ret) { > > + dev_err(qmp->dev, "dp phy configure failed, err=%d\n", ret); > > return ret; > > + } dev_err() calls are not related to the fix itself. Please split them to a separate patch. > > writel(0x04, qmp->dp_dp_phy + QSERDES_DP_PHY_AUX_CFG2); > > writel(0x01, qmp->dp_dp_phy + QSERDES_DP_PHY_CFG); > > @@ -2519,8 +2521,10 @@ static int qmp_v4_configure_dp_phy(struct qmp_combo *qmp) > > int ret; > > ret = qmp_v456_configure_dp_phy(qmp); > > - if (ret < 0) > > + if (ret < 0) { > > + dev_err(qmp->dev, "dp phy configure failed, err=%d\n", ret); > > return ret; > > + } > > /* > > * At least for 7nm DP PHY this has to be done after enabling link > > @@ -2754,6 +2758,7 @@ static int qmp_combo_dp_power_on(struct phy *phy) > > const struct qmp_phy_cfg *cfg = qmp->cfg; > > void __iomem *tx = qmp->dp_tx; > > void __iomem *tx2 = qmp->dp_tx2; > > + int ret; > > mutex_lock(&qmp->phy_mutex); > > @@ -2766,11 +2771,11 @@ static int qmp_combo_dp_power_on(struct phy *phy) > > cfg->configure_dp_tx(qmp); > > /* Configure link rate, swing, etc. */ > > - cfg->configure_dp_phy(qmp); > > + ret = cfg->configure_dp_phy(qmp); > > mutex_unlock(&qmp->phy_mutex); > > - return 0; > > + return ret; > > } > > static int qmp_combo_dp_power_off(struct phy *phy)
Quoting Kuogee Hsieh (2024-03-29 12:50:35) > Currently qmp_combo_dp_power_on() always return 0 in regardless of > return value of cfg->configure_dp_phy(). This patch propagate > return value of cfg->configure_dp_phy() all the way back to caller. Is this found via code inspection or because the phy is failing to power on sometimes? I ask because I'm looking at a DP bug on Trogdor with chromeos' v6.6 based kernel and wondering if this is related. Also, is the call to phy_power_on() going to be checked in the DP driver? $ git grep -n phy_power_on -- drivers/gpu/drm/msm/dp/ drivers/gpu/drm/msm/dp/dp_ctrl.c:1453: phy_power_on(phy);
On 4/3/2024 12:51 PM, Stephen Boyd wrote: > Quoting Kuogee Hsieh (2024-03-29 12:50:35) >> Currently qmp_combo_dp_power_on() always return 0 in regardless of >> return value of cfg->configure_dp_phy(). This patch propagate >> return value of cfg->configure_dp_phy() all the way back to caller. > > Is this found via code inspection or because the phy is failing to power > on sometimes? I ask because I'm looking at a DP bug on Trogdor with > chromeos' v6.6 based kernel and wondering if this is related. > No, we actually hit an issue. This issue was originally reported as a link training issue while bringing up DP on x1e80100. While debugging that we noticed that we should not have even proceeded to link training because the PLL was not getting locked and it was failing silently since there are no other error prints (and hence the second part of the patch to improve the error logs), and we do not return any error code from this driver, we could not catch the PLL unlocked issue. > Also, is the call to phy_power_on() going to be checked in > the DP driver? > > $ git grep -n phy_power_on -- drivers/gpu/drm/msm/dp/ > drivers/gpu/drm/msm/dp/dp_ctrl.c:1453: phy_power_on(phy); Yes, this is a good point. We should also post the patch to add the error checking in DP driver to fail if phy_power_on fails.
Quoting Abhinav Kumar (2024-04-03 12:58:50) > > > On 4/3/2024 12:51 PM, Stephen Boyd wrote: > > Quoting Kuogee Hsieh (2024-03-29 12:50:35) > >> Currently qmp_combo_dp_power_on() always return 0 in regardless of > >> return value of cfg->configure_dp_phy(). This patch propagate > >> return value of cfg->configure_dp_phy() all the way back to caller. > > > > Is this found via code inspection or because the phy is failing to power > > on sometimes? I ask because I'm looking at a DP bug on Trogdor with > > chromeos' v6.6 based kernel and wondering if this is related. > > > > No, we actually hit an issue. This issue was originally reported as a > link training issue while bringing up DP on x1e80100. > > While debugging that we noticed that we should not have even proceeded > to link training because the PLL was not getting locked and it was > failing silently since there are no other error prints (and hence the > second part of the patch to improve the error logs), and we do not > return any error code from this driver, we could not catch the PLL > unlocked issue. Did link training succeed in that case and the screen was black? Also, did you figure out why the PLL failed to lock? I sometimes see reports now with an "Unexpected interrupt:" message from the DP driver and the interrupt is the PLL unlocked one (DP_INTR_PLL_UNLOCKED). > > > Also, is the call to phy_power_on() going to be checked in > > the DP driver? > > > > $ git grep -n phy_power_on -- drivers/gpu/drm/msm/dp/ > > drivers/gpu/drm/msm/dp/dp_ctrl.c:1453: phy_power_on(phy); > > Yes, this is a good point. We should also post the patch to add the > error checking in DP driver to fail if phy_power_on fails. Sounds great, thanks.
On 4/3/2024 1:04 PM, Stephen Boyd wrote: > Quoting Abhinav Kumar (2024-04-03 12:58:50) >> >> >> On 4/3/2024 12:51 PM, Stephen Boyd wrote: >>> Quoting Kuogee Hsieh (2024-03-29 12:50:35) >>>> Currently qmp_combo_dp_power_on() always return 0 in regardless of >>>> return value of cfg->configure_dp_phy(). This patch propagate >>>> return value of cfg->configure_dp_phy() all the way back to caller. >>> >>> Is this found via code inspection or because the phy is failing to power >>> on sometimes? I ask because I'm looking at a DP bug on Trogdor with >>> chromeos' v6.6 based kernel and wondering if this is related. >>> >> >> No, we actually hit an issue. This issue was originally reported as a >> link training issue while bringing up DP on x1e80100. >> >> While debugging that we noticed that we should not have even proceeded >> to link training because the PLL was not getting locked and it was >> failing silently since there are no other error prints (and hence the >> second part of the patch to improve the error logs), and we do not >> return any error code from this driver, we could not catch the PLL >> unlocked issue. > > Did link training succeed in that case and the screen was black? Also, > did you figure out why the PLL failed to lock? I sometimes see reports > now with an "Unexpected interrupt:" message from the DP driver and the > interrupt is the PLL unlocked one (DP_INTR_PLL_UNLOCKED). > No the link training had failed. Yes, root-cause was that the PLL registers were misconfigured in the x1e80100 DP PHY for HBR2. Once we programmed the correct values it worked. This was specific to x1e80100. Yes, Doug mentioned this to me on IRC that this issue is still there. Surprising because I thought we had pushed https://patchwork.freedesktop.org/patch/551847/ long ago and it was fixed. It certainly did that time when I had tested this. >> >>> Also, is the call to phy_power_on() going to be checked in >>> the DP driver? >>> >>> $ git grep -n phy_power_on -- drivers/gpu/drm/msm/dp/ >>> drivers/gpu/drm/msm/dp/dp_ctrl.c:1453: phy_power_on(phy); >> >> Yes, this is a good point. We should also post the patch to add the >> error checking in DP driver to fail if phy_power_on fails. > > Sounds great, thanks.
Quoting Abhinav Kumar (2024-04-03 14:28:52) > > > On 4/3/2024 1:04 PM, Stephen Boyd wrote: > > Quoting Abhinav Kumar (2024-04-03 12:58:50) > >> > >> > >> On 4/3/2024 12:51 PM, Stephen Boyd wrote: > >>> Quoting Kuogee Hsieh (2024-03-29 12:50:35) > >>>> Currently qmp_combo_dp_power_on() always return 0 in regardless of > >>>> return value of cfg->configure_dp_phy(). This patch propagate > >>>> return value of cfg->configure_dp_phy() all the way back to caller. > >>> > >>> Is this found via code inspection or because the phy is failing to power > >>> on sometimes? I ask because I'm looking at a DP bug on Trogdor with > >>> chromeos' v6.6 based kernel and wondering if this is related. > >>> > >> > >> No, we actually hit an issue. This issue was originally reported as a > >> link training issue while bringing up DP on x1e80100. > >> > >> While debugging that we noticed that we should not have even proceeded > >> to link training because the PLL was not getting locked and it was > >> failing silently since there are no other error prints (and hence the > >> second part of the patch to improve the error logs), and we do not > >> return any error code from this driver, we could not catch the PLL > >> unlocked issue. > > > > Did link training succeed in that case and the screen was black? Also, > > did you figure out why the PLL failed to lock? I sometimes see reports > > now with an "Unexpected interrupt:" message from the DP driver and the > > interrupt is the PLL unlocked one (DP_INTR_PLL_UNLOCKED). > > > > No the link training had failed. > > Yes, root-cause was that the PLL registers were misconfigured in the > x1e80100 DP PHY for HBR2. Once we programmed the correct values it > worked. This was specific to x1e80100. Ah ok, so that's what the x1e80100 patch is about. > > Yes, Doug mentioned this to me on IRC that this issue is still there. > Surprising because I thought we had pushed > https://patchwork.freedesktop.org/patch/551847/ long ago and it was > fixed. It certainly did that time when I had tested this. I see it on v6.6 and it is also on v5.15.y (stable kernel) so that has been picked back. Somehow the aux interrupt is still happening though when the PLL isn't locked. Maybe that interrupt bit should be masked in most cases and only unmasked when something in the driver is going to care about it.
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c index 36632fa..513d99d 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c @@ -2343,8 +2343,10 @@ static int qmp_v3_configure_dp_phy(struct qmp_combo *qmp) writel(0x05, qmp->dp_dp_phy + QSERDES_V3_DP_PHY_TX2_TX3_LANE_CTL); ret = qmp_combo_configure_dp_clocks(qmp); - if (ret) + if (ret) { + dev_err(qmp->dev, "dp phy configure failed, err=%d\n", ret); return ret; + } writel(0x04, qmp->dp_dp_phy + QSERDES_DP_PHY_AUX_CFG2); writel(0x01, qmp->dp_dp_phy + QSERDES_DP_PHY_CFG); @@ -2519,8 +2521,10 @@ static int qmp_v4_configure_dp_phy(struct qmp_combo *qmp) int ret; ret = qmp_v456_configure_dp_phy(qmp); - if (ret < 0) + if (ret < 0) { + dev_err(qmp->dev, "dp phy configure failed, err=%d\n", ret); return ret; + } /* * At least for 7nm DP PHY this has to be done after enabling link @@ -2754,6 +2758,7 @@ static int qmp_combo_dp_power_on(struct phy *phy) const struct qmp_phy_cfg *cfg = qmp->cfg; void __iomem *tx = qmp->dp_tx; void __iomem *tx2 = qmp->dp_tx2; + int ret; mutex_lock(&qmp->phy_mutex); @@ -2766,11 +2771,11 @@ static int qmp_combo_dp_power_on(struct phy *phy) cfg->configure_dp_tx(qmp); /* Configure link rate, swing, etc. */ - cfg->configure_dp_phy(qmp); + ret = cfg->configure_dp_phy(qmp); mutex_unlock(&qmp->phy_mutex); - return 0; + return ret; } static int qmp_combo_dp_power_off(struct phy *phy)
Currently qmp_combo_dp_power_on() always return 0 in regardless of return value of cfg->configure_dp_phy(). This patch propagate return value of cfg->configure_dp_phy() all the way back to caller. Changes in V3: -- add v2 changes log Changes in V2: -- add Fixes tag -- add dev_err() to qmp_v3_configure_dp_phy() -- add dev_err() to qmp_v4_configure_dp_phy() Fixes: 52e013d0bffa ("phy: qcom-qmp: Add support for DP in USB3+DP combo phy") Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> --- drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)