Re: [PATCH 06/10] phy: qcom-ipq806x-usb: fix the PHY_PARAM_CTRL1 field masks

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

Pre-existing issues:
- [High] The patch fixes overlapping off-by-one field masks for PHY_PARAM_CTRL1, but fails to address identical off-by-one overlaps in RX_OVRD_IN_HI and TX_OVRD_DRV_LO register masks which cause active initialization bugs.
- [High] Missing clock cleanup on error paths in qcom_ipq806x_usb_ss_phy_init().
- [High] TEST_POWERDOWN is incorrectly cleared instead of being set during PHY exit.
- [High] Missing -EPROBE_DEFER handling for the optional xo_clk.
- [Medium] qcom_ipq806x_usb_ss_phy_init() incorrectly passes a fully assembled register value to usb_phy_write_readback(), causing spurious readback verification failures.
--

--- Patch [6]: [PATCH 06/10] phy: qcom-ipq806x-usb: fix the PHY_PARAM_CTRL1 field masks ---
commit cc9548cb27cc0af73d58ff5dfbb1d43ff53c1b2b
Author: Dmitry Baryshkov <[email protected]>

phy: qcom-ipq806x-usb: fix the PHY_PARAM_CTRL1 field masks

This patch aligns the PHY_PARAM_CTRL1 field masks with their documented 
positions to prevent values from landing shifted. The previous off-by-one 
masks corrupted neighboring fields and caused spurious write-readback 
failures during PHY initialization.

> diff --git a/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c b/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c
> index cf77e0a66e20d..3d3e9d6da298c 100644
> --- a/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c
> +++ b/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c
> @@ -68,15 +68,17 @@
>  #define SSPHY_MPLL_VALUE		0
>  
>  /* QSCRATCH PHY_PARAM_CTRL1 fields */
> -#define PHY_PARAM_CTRL1_TX_FULL_SWING_MASK	GENMASK(26, 19)
> -#define PHY_PARAM_CTRL1_TX_DEEMPH_6DB_MASK	GENMASK(19, 13)
> -#define PHY_PARAM_CTRL1_TX_DEEMPH_3_5DB_MASK	GENMASK(13, 7)
> -#define PHY_PARAM_CTRL1_LOS_BIAS_MASK		GENMASK(7, 2)
> +#define PHY_PARAM_CTRL1_TX_FULL_SWING_MASK	GENMASK(26, 20)
> +#define PHY_PARAM_CTRL1_TX_DEEMPH_6DB_MASK	GENMASK(19, 14)
> +#define PHY_PARAM_CTRL1_TX_DEEMPH_3_5DB_MASK	GENMASK(13, 8)
> +#define PHY_PARAM_CTRL1_LOS_LEVEL_MASK		GENMASK(7, 3)
> +#define PHY_PARAM_CTRL1_LOS_BIAS_MASK		GENMASK(2, 0)
>  
>  #define PHY_PARAM_CTRL1_MASK				\
>  		(PHY_PARAM_CTRL1_TX_FULL_SWING_MASK |	\
>  		 PHY_PARAM_CTRL1_TX_DEEMPH_6DB_MASK |	\
>  		 PHY_PARAM_CTRL1_TX_DEEMPH_3_5DB_MASK |	\
> +		 PHY_PARAM_CTRL1_LOS_LEVEL_MASK | \
>  		 PHY_PARAM_CTRL1_LOS_BIAS_MASK)

[Severity: High]
This is a pre-existing issue, but while fixing the overlapping field masks for 
PHY_PARAM_CTRL1, are there identical overlaps in the RX_OVRD_IN_HI and 
TX_OVRD_DRV_LO masks that break the initialization logic?

For example:

#define RX_OVRD_IN_HI_RX_EQ_MASK		GENMASK(10, 7)
#define RX_OVRD_IN_HI_RX_EQ_EN_OVRD		BIT(7)

Because these masks overlap at bit 7, does qcom_ipq806x_usb_ss_phy_init() 
silently disable the override right after enabling it?

	data |= RX_OVRD_IN_HI_RX_EQ_EN_OVRD;
	data &= ~RX_OVRD_IN_HI_RX_EQ_MASK;
	data |= RX_OVRD_IN_HI_RX_EQ(phy_dwc3->rx_eq);

[ ... ]

> @@ -436,7 +440,8 @@ static int qcom_ipq806x_usb_ss_phy_init(struct phy *phy)
>  	data |= PHY_PARAM_CTRL1_TX_FULL_SWING(0x6e) |
>  		PHY_PARAM_CTRL1_TX_DEEMPH_6DB(0x20) |
>  		PHY_PARAM_CTRL1_TX_DEEMPH_3_5DB(phy_dwc3->tx_deamp_3_5db) |
> -		PHY_PARAM_CTRL1_LOS_BIAS(0x9);
> +		PHY_PARAM_CTRL1_LOS_LEVEL(0x9) |
> +		PHY_PARAM_CTRL1_LOS_BIAS(0x0);
>  
>  	usb_phy_write_readback(phy_dwc3, SSUSB_PHY_PARAM_CTRL_1,
>  			       PHY_PARAM_CTRL1_MASK, data);

[Severity: Medium]
This isn't a bug introduced by this patch, but does this call to 
usb_phy_write_readback() always fail spuriously on probe?

It looks like we are passing a fully assembled register value (`data`) as `val`
instead of passing just the masked bits:

	data = readl(phy_dwc3->base + SSUSB_PHY_PARAM_CTRL_1);
	data &= ~PHY_PARAM_CTRL1_MASK;
	data |= PHY_PARAM_CTRL1_TX_FULL_SWING(0x6e) | ...
	usb_phy_write_readback(phy_dwc3, SSUSB_PHY_PARAM_CTRL_1,
			       PHY_PARAM_CTRL1_MASK, data);

Inside usb_phy_write_readback(), it uses the provided `mask` when checking `val`:

	/* Read back to see if val was written */
	tmp = readl(phy_dwc3->base + offset);
	tmp &= mask;		/* clear other bits */

	if (tmp != val)
		dev_err(...);

Since `data` contains unmasked bits from the initial readl() and `tmp` masks
them out, won't this check unconditionally fail if any unmasked bits are non-zero?

[Severity: High]
This is a pre-existing issue, but does qcom_ipq806x_usb_ss_phy_init() leak 
the clock references if it fails? 

When a hardware read/write fails, the error path returns without unwinding 
the enabled clocks:

err_phy_trans:
	return ret;

Since the PHY core doesn't call phy_exit() when phy_init() fails, it seems 
xo_clk and ref_clk remain permanently prepared and enabled?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does the PHY exit sequence in 
qcom_ipq806x_usb_ss_phy_exit() disable PHY retention instead of enabling it?

The comment indicates we want to set TEST_POWERDOWN:

	/* 3. Set TEST_POWERED_DOWN in PHY_CTRL_REG to enable PHY retention */
	...
	usb_phy_write_readback(phy_dwc3, SSUSB_PHY_CTRL_REG,
			       SSUSB_CTRL_TEST_POWERDOWN, 0x0);

Since 0x0 is passed as the `val`, usb_phy_write_readback() clears the 
SSUSB_CTRL_TEST_POWERDOWN bit rather than setting it. Should this pass 
SSUSB_CTRL_TEST_POWERDOWN as the value?

[ ... ]

[Severity: High]
This is a pre-existing issue, but does qcom_ipq806x_usb_phy_probe() handle 
probe deferral correctly for the optional xo_clk?

	phy_dwc3->xo_clk = devm_clk_get(phy_dwc3->dev, "xo");
	if (IS_ERR(phy_dwc3->xo_clk)) {
		dev_dbg(phy_dwc3->dev, "cannot get TCXO clock\n");
		phy_dwc3->xo_clk = NULL;
	}

devm_clk_get() returns -EPROBE_DEFER when the clock is not yet ready. By 
checking IS_ERR and masking all errors to NULL, wouldn't the driver fail to 
defer its probe and silently operate without the TCXO clock if it probes 
before the clock provider is ready?

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

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