Re: [PATCH 08/10] phy: qcom-ipq806x-usb: support the MSM8974 SS PHY parameters

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditional modification of shared IPQ806x HS PHY initialization introduces a hardware regression and contradicts the commit message.
- [Low] The PHY_PARAM_CTRL1_MASK macro is left as unused dead code.
--

--- Patch [8]: [PATCH 08/10] phy: qcom-ipq806x-usb: support the MSM8974 SS PHY parameters ---
commit ba82cb741a57672fd8113542520d510104c224d2
Author: Dmitry Baryshkov <[email protected]>

phy: qcom-ipq806x-usb: support the MSM8974 SS PHY parameters

This commit adds platform data for the MSM8974 compatible in the IPQ806x USB
PHY driver. It modifies qcom_ipq806x_usb_ss_phy_init() to use dynamic SS PHY
parameters instead of hardcoding IPQ806x-specific values. This ensures the
correct launch amplitudes are used for MSM8974.

[Severity: High]
Does this code unconditionally apply the active-low clamp disable bit?

drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c:qcom_ipq806x_usb_hs_phy_init() {
	val = HSUSB_CTRL_DPSEHV_CLAMP | HSUSB_CTRL_DMSEHV_CLAMP |
		HSUSB_CTRL_RETENABLEN  | HSUSB_CTRL_COMMONONN |
		HSUSB_CTRL_OTGSESSVLD_CLAMP | HSUSB_CTRL_ID_HV_CLAMP |
		HSUSB_CTRL_UTMI_OTG_VBUS_VALID | HSUSB_CTRL_UTMI_CLK_EN |
		HSUSB_CTRL_CLAMP_EN | HSUSB_CLAMP_MPM_DPSE_DMSE_EN_N |
		phy_dwc3->drvdata->fsel;
    ...
	writel(val, phy_dwc3->base + HSUSB_PHY_CTRL_REG);
    ...
}

This completely overwrites the PHY_CTRL register for all platforms. Can this
introduce a hardware regression for older IPQ806x hardware by sending an
unintended active-low clamp-disable bit?

Also, does this contradict the commit message's claim that MSM8974 must not
overwrite the whole PHY_CTRL register?

Finally, are there redundant, duplicated bit definitions for BIT(26) in the
same file?

drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c {
    ...
#define HSUSB_CLAMP_MPM_DPSE_DMSE_EN_N	BIT(26)
    ...
}

> diff --git a/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c b/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c
> index 86e8e869a743f..35abe9ae05429 100644
> --- a/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c
> +++ b/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c
[ ... ]
> @@ -433,18 +438,22 @@ static int qcom_ipq806x_usb_ss_phy_init(struct phy *phy)
>  	 * TX_DEEMPH_3_5DB [13:8] set based on SoC version
>  	 * LOS_BIAS [7:3] to 9
>  	 */
> -	data = readl(phy_dwc3->base + SSUSB_PHY_PARAM_CTRL_1);
> -
> -	data &= ~PHY_PARAM_CTRL1_MASK;

[Severity: Low]
Is the PHY_PARAM_CTRL1_MASK macro left as unused dead code now that its usage
has been removed here?

> -
> -	data |= PHY_PARAM_CTRL1_TX_FULL_SWING(0x6e) |
> -		PHY_PARAM_CTRL1_TX_DEEMPH_6DB(0x20) |
> -		PHY_PARAM_CTRL1_TX_DEEMPH_3_5DB(phy_dwc3->tx_deamp_3_5db) |
> -		PHY_PARAM_CTRL1_LOS_LEVEL(0x9) |
> -		PHY_PARAM_CTRL1_LOS_BIAS(0x0);
> +	mask = PHY_PARAM_CTRL1_TX_FULL_SWING_MASK |
> +	       PHY_PARAM_CTRL1_TX_DEEMPH_3_5DB_MASK |
> +	       PHY_PARAM_CTRL1_LOS_LEVEL_MASK |
> +	       PHY_PARAM_CTRL1_LOS_BIAS_MASK;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy