Re: [PATCH 5.15.y 3/4] phy: qcom-qmp-combo: fix broken power on

From: Johan Hovold <johan@kernel.org>
To: Stephen Boyd <swboyd@chromium.org>
Cc: stable@vger.kernel.org, Johan Hovold <johan+linaro@kernel.org>,
	linux-kernel@vger.kernel.org, patches@lists.linux.dev,
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	Vinod Koul <vkoul@kernel.org>
Subject: Re: [PATCH 5.15.y 3/4] phy: qcom-qmp-combo: fix broken power on
Date: Fri, 13 Jan 2023 09:07:32 +0100	[thread overview]
Message-ID: <Y8ERRMXng+kL+Vtc@hovoldconsulting.com> (raw)
In-Reply-To: <20230113005405.3992011-4-swboyd@chromium.org>

On 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