Re: [PATCH 04/10] phy: qcom-ipq806x-usb: add MSM8974 HS PHY support

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The MSM8974 initialization sequence reuses the shared IPQ806x sequence instead of providing its own, directly contradicting the commit message and unconditionally overwriting the PHY_CTRL register.
- [High] Setting the active-low HSUSB_CLAMP_MPM_DPSE_DMSE_EN_N bit disables the DP/DM MPM clamps, contradicting the commit message.
- [Low] Duplicate and redundant macro definitions for BIT(26) in the PHY_CTRL register.
--

--- Patch [4]: [PATCH 04/10] phy: qcom-ipq806x-usb: add MSM8974 HS PHY support ---
commit 90246a8322244753151c5fb6777bd8d442742a72
Author: Dmitry Baryshkov <[email protected]>

phy: qcom-ipq806x-usb: add MSM8974 HS PHY support

This commit adds support for the MSM8974 DWC3 wrapper by introducing a new
compatible string and driver data. It claims to implement a dedicated init
sequence for MSM8974 to preserve reference frequency selection straps.

> Add the qcom,msm8974-usb-phy-hs compatible with its own init
> sequence: unlike the IPQ806x one, it must not overwrite the whole
> PHY_CTRL register, as the power-on values of the reference frequency
> selection straps have to be preserved. Only enable the UTMI clock and
> the DP/DM MPM clamps, signal VBUS valid towards the link, and disable
> retention.

> diff --git a/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c b/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c
> --- a/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c
> +++ b/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c
> @@ -17,6 +17,7 @@
>  #define HSUSB_PHY_CTRL_REG		(0x10)
>  
>  /* PHY_CTRL_REG */
> +#define HSUSB_CLAMP_MPM_DPSE_DMSE_EN_N	BIT(26)
>  #define HSUSB_CTRL_DMSEHV_CLAMP		BIT(24)
>  #define HSUSB_CTRL_USB2_SUSPEND		BIT(23)
>  #define HSUSB_CTRL_UTMI_CLK_EN		BIT(21)
[ ... ]
> @@ -27,9 +28,13 @@
>  #define HSUSB_CTRL_ID_HV_CLAMP		BIT(9)
>  #define HSUSB_CTRL_OTGSESSVLD_CLAMP	BIT(8)
>  #define HSUSB_CTRL_CLAMP_EN		BIT(7)
> +#define HSUSB_CTRL_CLAMP_MPM_DPSE_DMSE	BIT(26)

[Severity: Low]
This isn't a bug, but does this bit need to be defined twice? It was just
defined as HSUSB_CLAMP_MPM_DPSE_DMSE_EN_N a few lines above.

[ ... ]
> @@ -273,7 +283,8 @@ static int qcom_ipq806x_usb_hs_phy_init(struct phy *phy)
>  		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 | 0x70;
> +		HSUSB_CTRL_CLAMP_EN | HSUSB_CLAMP_MPM_DPSE_DMSE_EN_N |

[Severity: High]
The commit message mentions we should "enable the DP/DM MPM clamps". Since
this bit has an _EN_N suffix, standardly indicating it is active-low, does
setting it to 1 here actually disable the clamps instead of enabling them?

> +		phy_dwc3->drvdata->fsel;
>  
>  	/* use core clock if external reference is not present */
>  	if (!phy_dwc3->xo_clk)
> @@ -282,9 +293,14 @@ static int qcom_ipq806x_usb_hs_phy_init(struct phy *phy)
>  	writel(val, phy_dwc3->base + HSUSB_PHY_CTRL_REG);

[Severity: High]
The commit message states that the MSM8974 init sequence "must not
overwrite the whole PHY_CTRL register". Does unconditionally writing to
this register with writel() break the preservation of the power-on values
for the reference frequency selection straps?

[ ... ]
> @@ -458,6 +474,18 @@ static const struct phy_drvdata qcom_ipq806x_usb_hs_drvdata = {
>  		.owner		= THIS_MODULE,
>  	},
>  	.clk_rate = 60000000,
> +	.fsel = 0x70,
> +};
> +
> +static const struct phy_drvdata qcom_msm8974_usb_hs_drvdata = {
> +	.ops = {
> +		.init		= qcom_ipq806x_usb_hs_phy_init,

[Severity: High]
The commit message indicates that MSM8974 was intended to have "its own init
sequence", but here it is assigned the shared qcom_ipq806x_usb_hs_phy_init
function. Should this point to a dedicated initialization function for
MSM8974 to avoid the writel() register overwrite mentioned above?

> +		.exit		= qcom_ipq806x_usb_hs_phy_exit,
> +		.owner		= THIS_MODULE,
> +	},

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

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