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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] phy: qcom: missing SIDDQ clear in 6nm PHY post-tuning
- [Low] build: missing <linux/bitfield.h> for FIELD_PREP_CONST
--

commit bd845c9b97c7b3b1c9ec07fec6daa12f9b0766d5
Author: Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>

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

Add the 6 nm 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 2e37b39dabbaa..3ed4292fc8c30 100644
> --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> @@ -29,12 +29,15 @@
>  
>  #define USB2_PHY_USB_PHY_UTMI_CTRL5		(0x50)
>  #define POR					BIT(1)
> +#define ATERESET				BIT(0)
>  
>  #define USB2_PHY_USB_PHY_HS_PHY_CTRL_COMMON0	(0x54)
>  #define SIDDQ					BIT(2)
>  #define RETENABLEN				BIT(3)
>  #define FSEL_MASK				GENMASK(6, 4)
>  #define FSEL_DEFAULT				(0x3 << 4)
> +#define FSEL_24MHZ				FIELD_PREP_CONST(FSEL_MASK, 2)

[Severity: Low]
Is it possible this could cause build failures on architectures where
linux/bitfield.h is not transitively included? Relying on transitive
header inclusions might break when implicit include chains differ.

[ ... ]

> @@ -418,6 +442,32 @@ static const struct phy_reg_config hs_5nm_phy_post_tuning[] = {
>  	{ USB2_PHY_USB_PHY_CFG0, UTMI_PHY_CMN_CTRL_OVERRIDE_EN, 0 },
>  };
>  
> +static const struct phy_reg_config hs_6nm_phy_pre_tuning[] = {
> +	{ 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 },
> +	{ USB2_PHY_USB_PHY_HS_PHY_CTRL_COMMON1, VBUSVLDEXTSEL0, VBUSVLDEXTSEL0 },
> +	{ USB2_PHY_USB_PHY_HS_PHY_CTRL1, VBUSVLDEXT0, VBUSVLDEXT0 },
> +};
> +
> +static const struct phy_reg_config hs_6nm_phy_post_tuning[] = {
> +	{ USB2_PHY_USB_PHY_HS_PHY_CTRL_COMMON2, VREGBYPASS, VREGBYPASS },
> +	{ USB2_PHY_USB_PHY_UTMI_CTRL5, ATERESET, 0 },
> +	{ USB2_PHY_USB_PHY_HS_PHY_TEST1, TESTDATAOUTSEL, 0 },
> +	{ USB2_PHY_USB_PHY_HS_PHY_TEST1, TESTCLK, 0 },
> +	{ USB2_PHY_USB_PHY_HS_PHY_CTRL_COMMON0, VATESTENB_MASK, 0 },

[Severity: Medium]
Does this sequence intentionally omit clearing the SIDDQ deep sleep bit?

If the analog PHY defaults to deep sleep (SIDDQ=1), omitting the clear
operation could leave the PHY powered down and non-functional during boot
or interface initialization.

Looking at hs_5nm_phy_post_tuning(), it explicitly clears the SIDDQ bit:
    { USB2_PHY_USB_PHY_HS_PHY_CTRL_COMMON0, SIDDQ, 0 }

Could the 6nm hardware behavior identically match the 5nm/7nm and other
Synopsys PHYs where SIDDQ defaults to 1, requiring it to be cleared here?

> +	{ USB2_PHY_USB_PHY_HS_PHY_TEST0, TESTDATAIN_MASK, 0 },
> +	{ USB2_PHY_USB_PHY_HS_PHY_CTRL2, USB2_SUSPEND_N_SEL | USB2_SUSPEND_N,
> +					 USB2_SUSPEND_N_SEL | USB2_SUSPEND_N },
> +	{ USB2_PHY_USB_PHY_UTMI_CTRL0, SLEEPM, SLEEPM },
> +	{ USB2_PHY_USB_PHY_UTMI_CTRL5, POR, 0 },
> +	{ USB2_PHY_USB_PHY_HS_PHY_CTRL2, USB2_SUSPEND_N_SEL, 0 },
> +	{ USB2_PHY_USB_PHY_CFG0, UTMI_PHY_CMN_CTRL_OVERRIDE_EN, 0 },
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006-phy-v5-0-cbe9330a6449@oss.qualcomm.com?part=6

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