From: Johan Hovold <johan+linaro@kernel.org>
commit 7a7d86d14d073dfa3429c550667a8e78b99edbd4 upstream.
The PHY is powered on during phy-init by setting the SW_PWRDN bit in the
COM_POWER_DOWN_CTRL register and then setting the same bit in the in the
PCS_POWER_DOWN_CONTROL register that belongs to the USB part of the
PHY.
Currently, whether power on succeeds depends on probe order and having
the USB part of the PHY be initialised first. In case the DP part of the
PHY is instead initialised first, the intended power on of the USB block
results in a corrupted DP_PHY register (e.g. DP_PHY_AUX_CFG8).
Add a pointer to the USB part of the PHY to the driver data and use that
to power on the PHY also if the DP part of the PHY is initialised first.
Fixes: 52e013d0bffa ("phy: qcom-qmp: Add support for DP in USB3+DP combo phy")
Cc: stable@vger.kernel.org # 5.10
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
Link: https://lore.kernel.org/r/20221114081346.5116-5-johan+linaro@kernel.org
Signed-off-by: Vinod Koul <vkoul@kernel.org>
[swboyd@chromium.org: Backport to pre-split driver]
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
drivers/phy/qualcomm/phy-qcom-qmp.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index c6f860ce3d99..9fda6d283f20 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -2919,6 +2919,7 @@ struct qcom_qmp {
struct regulator_bulk_data *vregs;
struct qmp_phy **phys;
+ struct qmp_phy *usb_phy;
struct mutex phy_mutex;
int init_count;
@@ -4554,7 +4555,7 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy)
struct qcom_qmp *qmp = qphy->qmp;
const struct qmp_phy_cfg *cfg = qphy->cfg;
void __iomem *serdes = qphy->serdes;
- void __iomem *pcs = qphy->pcs;
+ struct qmp_phy *usb_phy = qmp->usb_phy;
void __iomem *dp_com = qmp->dp_com;
int ret, i;
@@ -4620,13 +4621,13 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy)
qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
SW_PWRDN);
} else {
- if (cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL])
- qphy_setbits(pcs,
- cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
- cfg->pwrdn_ctrl);
+ if (usb_phy->cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL])
+ qphy_setbits(usb_phy->pcs,
+ usb_phy->cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
+ usb_phy->cfg->pwrdn_ctrl);
else
- qphy_setbits(pcs, QPHY_POWER_DOWN_CONTROL,
- cfg->pwrdn_ctrl);
+ qphy_setbits(usb_phy->pcs, QPHY_POWER_DOWN_CONTROL,
+ usb_phy->cfg->pwrdn_ctrl);
}
mutex_unlock(&qmp->phy_mutex);
@@ -5769,6 +5770,9 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev)
goto err_node_put;
}
+ if (cfg->type == PHY_TYPE_USB3)
+ qmp->usb_phy = qmp->phys[id];
+
/*
* Register the pipe clock provided by phy.
* See function description to see details of this pipe clock.
@@ -5791,6 +5795,9 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev)
id++;
}
+ if (!qmp->usb_phy)
+ return -EINVAL;
+
phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
if (!IS_ERR(phy_provider))
dev_info(dev, "Registered Qcom-QMP phy\n");
--
https://chromeos.devOn Thu, Jan 12, 2023 at 04:54:04PM -0800, Stephen Boyd wrote:
> From: Johan Hovold <johan+linaro@kernel.org>
>
> commit 7a7d86d14d073dfa3429c550667a8e78b99edbd4 upstream.
>
> The PHY is powered on during phy-init by setting the SW_PWRDN bit in the
> COM_POWER_DOWN_CTRL register and then setting the same bit in the in the
> PCS_POWER_DOWN_CONTROL register that belongs to the USB part of the
> PHY.
>
> Currently, whether power on succeeds depends on probe order and having
> the USB part of the PHY be initialised first. In case the DP part of the
> PHY is instead initialised first, the intended power on of the USB block
> results in a corrupted DP_PHY register (e.g. DP_PHY_AUX_CFG8).
>
> Add a pointer to the USB part of the PHY to the driver data and use that
> to power on the PHY also if the DP part of the PHY is initialised first.
>
> Fixes: 52e013d0bffa ("phy: qcom-qmp: Add support for DP in USB3+DP combo phy")
> Cc: stable@vger.kernel.org # 5.10
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> Link: https://lore.kernel.org/r/20221114081346.5116-5-johan+linaro@kernel.org
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> [swboyd@chromium.org: Backport to pre-split driver]
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
> drivers/phy/qualcomm/phy-qcom-qmp.c | 21 ++++++++++++++-------
> 1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index c6f860ce3d99..9fda6d283f20 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -2919,6 +2919,7 @@ struct qcom_qmp {
> struct regulator_bulk_data *vregs;
>
> struct qmp_phy **phys;
> + struct qmp_phy *usb_phy;
>
> struct mutex phy_mutex;
> int init_count;
> @@ -4554,7 +4555,7 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy)
> struct qcom_qmp *qmp = qphy->qmp;
> const struct qmp_phy_cfg *cfg = qphy->cfg;
> void __iomem *serdes = qphy->serdes;
> - void __iomem *pcs = qphy->pcs;
> + struct qmp_phy *usb_phy = qmp->usb_phy;
> void __iomem *dp_com = qmp->dp_com;
> int ret, i;
>
> @@ -4620,13 +4621,13 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy)
> qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
> SW_PWRDN);
> } else {
> - if (cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL])
> - qphy_setbits(pcs,
> - cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
> - cfg->pwrdn_ctrl);
> + if (usb_phy->cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL])
> + qphy_setbits(usb_phy->pcs,
> + usb_phy->cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
> + usb_phy->cfg->pwrdn_ctrl);
> else
> - qphy_setbits(pcs, QPHY_POWER_DOWN_CONTROL,
> - cfg->pwrdn_ctrl);
> + qphy_setbits(usb_phy->pcs, QPHY_POWER_DOWN_CONTROL,
> + usb_phy->cfg->pwrdn_ctrl);
This bit looks like it requires some more work in order not to break
PCIe and UFS PHYs, which lack the USB registers.
> }
>
> mutex_unlock(&qmp->phy_mutex);
> @@ -5769,6 +5770,9 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev)
> goto err_node_put;
> }
>
> + if (cfg->type == PHY_TYPE_USB3)
> + qmp->usb_phy = qmp->phys[id];
> +
> /*
> * Register the pipe clock provided by phy.
> * See function description to see details of this pipe clock.
> @@ -5791,6 +5795,9 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev)
> id++;
> }
>
> + if (!qmp->usb_phy)
> + return -EINVAL;
> +
This will break probe of PCIe and UFS PHYs unless you only check this
for combo PHYs.
Perhaps you can rename the usb_phy pointer to something more generic and
set it also for PCIe and UFS so that it always points to the register
block that should be used for QPHY_PCS_POWER_DOWN_CONTROL.
> phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> if (!IS_ERR(phy_provider))
> dev_info(dev, "Registered Qcom-QMP phy\n");
Johan
Quoting Johan Hovold (2023-01-13 00:07:32)
> On Thu, Jan 12, 2023 at 04:54:04PM -0800, Stephen Boyd wrote:
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > index c6f860ce3d99..9fda6d283f20 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > @@ -4620,13 +4621,13 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy)
> > qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
> > SW_PWRDN);
> > } else {
> > - if (cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL])
> > - qphy_setbits(pcs,
> > - cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
> > - cfg->pwrdn_ctrl);
> > + if (usb_phy->cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL])
> > + qphy_setbits(usb_phy->pcs,
> > + usb_phy->cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
> > + usb_phy->cfg->pwrdn_ctrl);
> > else
> > - qphy_setbits(pcs, QPHY_POWER_DOWN_CONTROL,
> > - cfg->pwrdn_ctrl);
> > + qphy_setbits(usb_phy->pcs, QPHY_POWER_DOWN_CONTROL,
> > + usb_phy->cfg->pwrdn_ctrl);
>
> This bit looks like it requires some more work in order not to break
> PCIe and UFS PHYs, which lack the USB registers.
Good point. I don't have PCIe or UFS PHYs so I missed this.
>
> > }
> >
> > mutex_unlock(&qmp->phy_mutex);
> > @@ -5769,6 +5770,9 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev)
> > goto err_node_put;
> > }
> >
> > + if (cfg->type == PHY_TYPE_USB3)
I changed this to by 'cfg->type != PHY_TYPE_DP'
> > + qmp->usb_phy = qmp->phys[id];
> > +
> > /*
> > * Register the pipe clock provided by phy.
> > * See function description to see details of this pipe clock.
> > @@ -5791,6 +5795,9 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev)
> > id++;
> > }
> >
> > + if (!qmp->usb_phy)
> > + return -EINVAL;
> > +
>
> This will break probe of PCIe and UFS PHYs unless you only check this
> for combo PHYs.
>
> Perhaps you can rename the usb_phy pointer to something more generic and
> set it also for PCIe and UFS so that it always points to the register
> block that should be used for QPHY_PCS_POWER_DOWN_CONTROL.
I worry that future backports will be complicated if they use 'usb_phy'.
Maybe I'll just leave this as is.
© 2016 - 2025 Red Hat, Inc.