Re: [PATCH 05/10] phy: qcom-ipq806x-usb: fix the SS PHY TEST_POWERDOWN bit

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch corrects the bit offset for `SSUSB_CTRL_TEST_POWERDOWN` while failing to actually fix the PHY powerdown issue due to a pre-existing copy-paste error that clears the bit instead of setting it. Furthermore, a naive correction of this error will expose a missing cleanup in the init sequence.
--

--- Patch [5]: [PATCH 05/10] phy: qcom-ipq806x-usb: fix the SS PHY TEST_POWERDOWN bit ---
commit c8781c769e4ee40df09815570d904fe2655dc82f
Author: Dmitry Baryshkov <[email protected]>

phy: qcom-ipq806x-usb: fix the SS PHY TEST_POWERDOWN bit
    
The SS PHY exit sequence tries to put the PHY into retention via the
TEST_POWERDOWN bit of SS_PHY_CTRL, but defines that bit as BIT(27). In
the DWC3 QSCRATCH SS_PHY_CTRL register TEST_POWERDOWN is bit 26 (see
the downstream phy-msm-ssusb driver for the same IP generation), so the
sequence pokes an unrelated bit and the PHY is never powered down.

> diff --git a/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c b/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c
> index 21b59d4ca8d2a..cf77e0a66e20d 100644
> --- a/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c
> +++ b/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c
> @@ -51,7 +51,7 @@
>  
>  /* PHY_CTRL_REG */
>  #define SSUSB_CTRL_REF_USE_PAD		BIT(28)
> -#define SSUSB_CTRL_TEST_POWERDOWN	BIT(27)
> +#define SSUSB_CTRL_TEST_POWERDOWN	BIT(26)

[Severity: High]
This isn't a bug introduced by this patch, but while correcting the bit
offset for SSUSB_CTRL_TEST_POWERDOWN, the PHY powerdown issue remains
because the exit sequence clears the bit instead of setting it.

Looking at qcom_ipq806x_usb_ss_phy_exit() in 
drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c, it calls:

	usb_phy_write_readback(phy_dwc3, SSUSB_PHY_CTRL_REG,
			       SSUSB_CTRL_TEST_POWERDOWN, 0x0);

The usb_phy_write_readback() function clears the masked bits when passed 0x0:

	tmp &= ~mask;		/* retain other bits */
	write_val = tmp | val;

Should the third call in qcom_ipq806x_usb_ss_phy_exit() pass
SSUSB_CTRL_TEST_POWERDOWN as the value to actually set the bit?

Furthermore, if the exit sequence is fixed to set the bit, will the PHY
fail to exit retention on resume? In qcom_ipq806x_usb_ss_phy_init(), the
register is read, modified, and written back without clearing the
TEST_POWERDOWN bit:

	data = readl(phy_dwc3->base + SSUSB_PHY_CTRL_REG);
	writel(data | SSUSB_CTRL_SS_PHY_RESET,
	       phy_dwc3->base + SSUSB_PHY_CTRL_REG);

Since data is never updated to clear SSUSB_CTRL_TEST_POWERDOWN, would it
stay set and prevent the PHY from exiting retention mode?

>  #define SSUSB_CTRL_LANE0_PWR_PRESENT	BIT(24)
>  #define SSUSB_CTRL_SS_PHY_EN		BIT(8)
>  #define SSUSB_CTRL_SS_PHY_RESET		BIT(7)

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

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