[4/6] phy: qcom-qmp-combo: fix broken power on

The PHY is powered on during phy-init by setting the SW_PRWDN 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
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

On 11/11/2022 11:42, Johan Hovold wrote:
> The PHY is powered on during phy-init by setting the SW_PRWDN bit in the

Nit: SW_PWRDN

> 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
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

I can only hope that at some point in your cleanup this hack is going to 
be removed.
Nevertheless, I don't see a good way to do this at this moment. Thus:

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
>   drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 40c25a0ead23..17707f68d482 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -932,6 +932,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;
> @@ -1911,7 +1912,7 @@ static int qmp_combo_com_init(struct qmp_phy *qphy)
>   {
>   	struct qcom_qmp *qmp = qphy->qmp;
>   	const struct qmp_phy_cfg *cfg = qphy->cfg;
> -	void __iomem *pcs = qphy->pcs;
> +	struct qmp_phy *usb_phy = qmp->usb_phy;
>   	void __iomem *dp_com = qmp->dp_com;
>   	int ret;
>   
> @@ -1963,7 +1964,8 @@ static int qmp_combo_com_init(struct qmp_phy *qphy)
>   	qphy_clrbits(dp_com, QPHY_V3_DP_COM_SWI_CTRL, 0x03);
>   	qphy_clrbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET);
>   
> -	qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
> +	qphy_setbits(usb_phy->pcs, usb_phy->cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
> +			SW_PWRDN);
>   
>   	mutex_unlock(&qmp->phy_mutex);
>   
> @@ -2831,6 +2833,8 @@ static int qmp_combo_probe(struct platform_device *pdev)
>   				goto err_node_put;
>   			}
>   
> +			qmp->usb_phy = qmp->phys[id];
> +
>   			/*
>   			 * Register the pipe clock provided by phy.
>   			 * See function description to see details of this pipe clock.
> @@ -2846,6 +2850,9 @@ static int qmp_combo_probe(struct platform_device *pdev)
>   		id++;
>   	}
>   
> +	if (!qmp->usb_phy)
> +		return -EINVAL;
> +
>   	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
>   
>   	return PTR_ERR_OR_ZERO(phy_provider);

On Sat, Nov 12, 2022 at 09:15:43AM +0300, Dmitry Baryshkov wrote:
> On 11/11/2022 11:42, Johan Hovold wrote:
> > The PHY is powered on during phy-init by setting the SW_PRWDN bit in the
> 
> Nit: SW_PWRDN
> 
> > 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
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> 
> I can only hope that at some point in your cleanup this hack is going to 
> be removed.
> Nevertheless, I don't see a good way to do this at this moment. Thus:

Not sure why you're calling this a hack. This is how the hardware works
and pretending that this PHY is some kind of MFD with completely
independent components is partly what resulted in this mess.

Accessing the USB registers by means of a hard-coded index in the PHY
array as is done in the runtime PM callbacks is a hack (see patch 5/6),
adding a dedicated pointer is not.

> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Johan