Re: [PATCH 4/5] phy: qcom: snps-femto-v2: Move PHY init registers to config tables

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The mask and value arguments for the `USB2_PHY_USB_PHY_REFCLK_CTRL` register write are swapped in the `hs_5nm_phy_pre` configuration table.
- [High] The `USB2_AUTO_RESUME` bit is never cleared in `qcom_snps_hsphy_suspend()` due to an incorrect mask of 0.
--

--- Patch [4]: [PATCH 4/5] phy: qcom: snps-femto-v2: Move PHY init registers to config tables ---
commit 5e719820e4a23178ca8190ffc88ff2f741947939
Author: Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>

phy: qcom: snps-femto-v2: Move PHY init registers to config tables

This commit refactors the SNPS Femto v2 PHY driver to use per-compatible
configuration tables for fixed pre- and post-override register sequences.
This allows the driver to share the common sequence while adding SoC-specific
sequences for future platforms.

> diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> index eb0b0f61d98e0..a34027b215f9b 100644
> --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
[ ... ]
> @@ -383,13 +395,42 @@ static const struct override_param_map sc7280_snps_7nm_phy[] = {
>  	{},
>  };
>  
> +static const struct phy_reg_config hs_5nm_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_HS_PHY_CTRL_COMMON0, FSEL_MASK, 0 },
> +	{ USB2_PHY_USB_PHY_HS_PHY_CTRL_COMMON1, PLLBTUNE, PLLBTUNE },
> +	{ USB2_PHY_USB_PHY_REFCLK_CTRL, REFCLK_SEL_DEFAULT, REFCLK_SEL_MASK },

[Severity: High]
This is a pre-existing issue, but are the mask and value arguments swapped
here?

The phy_reg_config structure expects the order { offset, mask, val }. By
passing REFCLK_SEL_DEFAULT as the mask and REFCLK_SEL_MASK as the value,
qcom_snps_hsphy_write_mask() will evaluate:

    reg &= ~REFCLK_SEL_DEFAULT;
    reg |= REFCLK_SEL_MASK & REFCLK_SEL_DEFAULT;

Because REFCLK_SEL_DEFAULT is 0x2 and REFCLK_SEL_MASK is 0x3, bit 0 is excluded
from the mask and remains unchanged. If bit 0 was set by the bootloader, the
final register value incorrectly becomes 0x3 instead of 0x2, selecting the
wrong reference clock.

(Notice that in another patch of this series, hs_6nm_phy_pre correctly uses
REFCLK_SEL_MASK as the mask, but hs_5nm_phy_pre was not corrected here.)

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

[ ... ]

While looking at how qcom_snps_hsphy_write_mask() is used in the driver, I
noticed another pre-existing issue in qcom_snps_hsphy_suspend(), which is not
modified by this patch:

[Severity: High]
This isn't a bug introduced by this patch, but does qcom_snps_hsphy_suspend()
fail to clear the USB2_AUTO_RESUME bit?

drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c:qcom_snps_hsphy_suspend() {
    ...
    qcom_snps_hsphy_write_mask(hsphy->base,
                               USB2_PHY_USB_PHY_HS_PHY_CTRL2,
                               0, USB2_AUTO_RESUME);
    ...
}

By passing 0 as the mask, qcom_snps_hsphy_write_mask() will do:

    reg &= ~0;
    reg |= USB2_AUTO_RESUME & 0;

This leaves the register entirely unaffected, and the auto-resume bit is
left set indefinitely instead of being cleared.

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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy