Re: [PATCH 5/5] phy: qcom: snps-femto-v2: Add IPQ9650 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The `hs_5nm_phy_pre` array incorrectly swaps the mask and value parameters for `USB2_PHY_USB_PHY_REFCLK_CTRL`, resulting in incomplete bit clearing.
--

--- Patch [5]: [PATCH 5/5] phy: qcom: snps-femto-v2: Add IPQ9650 support ---
commit 5cb516a1d2d452ce8939f49955704babc0701171
Author: Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>

phy: qcom: snps-femto-v2: Add IPQ9650 support

This commit adds the 6nm SNPS femto USB2 PHY configuration needed by
IPQ9650. The PHYs on this SoC require a dedicated init sequence and override
parameters before they can be enabled.

> diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> index a34027b215f9b..7e8b346c61896 100644
> --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c

[ ... ]

> @@ -418,6 +442,34 @@ static const struct phy_reg_config hs_5nm_phy_post[] = {
>  	{ UINT_MAX, 0, 0 },
>  };
>  
> +static const struct phy_reg_config hs_6nm_phy_pre[] = {
> +	{ USB2_PHY_USB_PHY_CFG0, UTMI_PHY_CMN_CTRL_OVERRIDE_EN, UTMI_PHY_CMN_CTRL_OVERRIDE_EN },
> +	{ USB2_PHY_USB_PHY_UTMI_CTRL5, POR, POR },
> +	{ USB2_PHY_USB_PHY_FSEL_SEL, FSEL_SEL, FSEL_SEL },
> +	{ USB2_PHY_USB_PHY_HS_PHY_CTRL_COMMON0, FSEL_MASK, FSEL_24MHZ },
> +	{ USB2_PHY_USB_PHY_HS_PHY_CTRL_COMMON1, PLLBTUNE, PLLBTUNE },
> +	{ USB2_PHY_USB_PHY_REFCLK_CTRL, REFCLK_SEL_MASK, REFCLK_SEL_DEFAULT },

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing it I noticed a
discrepancy between the new hs_6nm_phy_pre array and the existing
hs_5nm_phy_pre array.

The newly introduced hs_6nm_phy_pre array correctly orders the mask and value
parameters:

    { USB2_PHY_USB_PHY_REFCLK_CTRL, REFCLK_SEL_MASK, REFCLK_SEL_DEFAULT },

However, the existing hs_5nm_phy_pre array appears to have the mask and value
swapped:

    drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c:hs_5nm_phy_pre[]
    { USB2_PHY_USB_PHY_REFCLK_CTRL, REFCLK_SEL_DEFAULT, REFCLK_SEL_MASK },

Since the phy_reg_config struct expects { offset, mask, val }, passing
REFCLK_SEL_DEFAULT (0x2) as the mask and REFCLK_SEL_MASK (0x3) as the value
means qcom_snps_hsphy_write_mask() will execute:

    reg &= ~0x2
    reg |= (0x3 & 0x2)

This sets bit 1 to 1 but fails to clear bit 0.

If bit 0 was already set by the bootloader, the register will hold 0x3
instead of 0x2, which could result in an incorrect reference clock
configuration and prevent the USB PHY PLL from locking.

Does this swapped mask and value in hs_5nm_phy_pre need to be corrected in a
separate patch?

> +	{ USB2_PHY_USB_PHY_HS_PHY_CTRL_COMMON1, VBUSVLDEXTSEL0, VBUSVLDEXTSEL0 },
> +	{ USB2_PHY_USB_PHY_HS_PHY_CTRL1, VBUSVLDEXT0, VBUSVLDEXT0 },
> +	{ UINT_MAX, 0, 0 },
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-phy-v1-0-257d68a3210b@oss.qualcomm.com?part=5