Re: [PATCH v9 2/5] phy: qualcomm: hdmi-28lpm: provide dynamic configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] VCO calibration frequency is calculated with the wrong unit, causing severe truncation and corrupting calibration registers.
- [High] Integer overflow in the `dc_offset` calculation for SDM bypass mode silently overlaps with control bits and severely misconfigures the PLL multiplier.
--

commit 26d0a45f7fc0a0f6484ab8dc0e93e6b81f47871e
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

phy: qualcomm: hdmi-28lpm: provide dynamic configuration

This commit replaces the fixed value configuration tables for the HDMI PHY
with values calculated at runtime.

> diff --git a/drivers/phy/qualcomm/phy-qcom-hdmi-28lpm.c b/drivers/phy/qualcomm/phy-qcom-hdmi-28lpm.c
> index f1e7113e10bda..90d3331313c05 100644
> --- a/drivers/phy/qualcomm/phy-qcom-hdmi-28lpm.c
> +++ b/drivers/phy/qualcomm/phy-qcom-hdmi-28lpm.c

[ ... ]

> @@ -407,6 +202,70 @@ static unsigned long qcom_28lpm_recalc(struct qcom_hdmi_preqmp_phy *hdmi_phy,
>  	return rate;
>  }
>  
> +/* This function is close to UNIPHY, but it has slighly different fields */
> +static int qcom_28lpm_set_rate(struct qcom_hdmi_preqmp_phy *hdmi_phy, unsigned long parent_rate,
> +			       unsigned long vco_freq, u32 div_idx)
> +{
> +	unsigned int pixclk = hdmi_phy->hdmi_opts.tmds_char_rate;
> +	unsigned int int_ref_freq;
> +	unsigned int dc_offset;
> +	unsigned int sdm_freq_seed;
> +	unsigned int val;
> +	bool sdm_mode = false;
> +	u32 refclk_cfg;
> +	u32 lf_cfg0;
> +	u32 lf_cfg1;
> +	u64 tmp;
> +
> +	dev_dbg(hdmi_phy->dev, "rate=%u, div = %d, vco = %lu", pixclk, div_idx, vco_freq);
> +
> +	if (vco_freq % (parent_rate / 2) == 0) {
> +		refclk_cfg = FIELD_PREP(HDMI_8960_PHY_CLK0_DIV, 1);
> +		int_ref_freq = parent_rate / 2;
> +	} else {
> +		refclk_cfg = HDMI_8960_PHY_DBLR_EN;
> +		int_ref_freq = parent_rate * 2;
> +		sdm_mode = true;
> +	}
> +
> +	dc_offset = vco_freq / int_ref_freq - 1;

Could dc_offset overflow the 6-bit hardware field in SDM bypass mode?

If a high VCO frequency is requested (e.g., a 216 MHz pixel clock yielding a
1080 MHz VCO), int_ref_freq is 13.5 MHz. The calculation for dc_offset
becomes 1080 / 13.5 - 1 = 79 (0x4F).

[ ... ]

> +	if (sdm_mode)
> +		writel(dc_offset,
> +		       hdmi_phy->pll_reg + REG_HDMI_8960_PHY_PLL_SDM_CFG0);
> +	else
> +		writel(HDMI_8960_PHY_SDM_BYP | dc_offset,
> +		       hdmi_phy->pll_reg + REG_HDMI_8960_PHY_PLL_SDM_CFG0);

This value exceeds the 6-bit boundary. Bit 6 (0x40) overlaps directly with
the HDMI_8960_PHY_SDM_BYP control bit, so the hardware divider would only
receive the truncated lower 5 bits (0x0F = 15). The hardware would therefore
synthesize a VCO of 216 MHz instead of the expected 1080 MHz.

Does the bypass selection logic earlier need an additional constraint to fall
back to fractional SDM mode when the divider limit is exceeded?

> +
> +	writel(HDMI_8960_PHY_DITHER | dc_offset,
> +	       hdmi_phy->pll_reg + REG_HDMI_8960_PHY_PLL_SDM_CFG1);
> +
> +	write24(sdm_freq_seed, hdmi_phy->pll_reg + REG_HDMI_8960_PHY_PLL_SDM_CFG2);
> +
> +	write16(vco_freq / 1000, hdmi_phy->pll_reg + REG_HDMI_8960_PHY_PLL_VCOCAL_CFG0);

Is dividing by 1000 yielding kHz instead of the expected MHz here?

Since vco_freq is in Hz, for a typical 1080p pixel clock (148.5 MHz),
vco_freq is 742,500,000 Hz. Dividing by 1000 results in 742,500 (0x0B5454).
Because write16 takes a u16 argument, this value would silently truncate to
0x5454, writing 0x54 to both VCOCAL_CFG0 and VCOCAL_CFG1.

The previous fixed configuration tables explicitly programmed 0x02E6 (742) for
this resolution, indicating the hardware expects the value in MHz.

Should this calculation be vco_freq / 1000000 instead?

> +
> +	writel(0x3b, hdmi_phy->pll_reg + REG_HDMI_8960_PHY_PLL_VCOCAL_CFG2);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513-fd-hdmi-phy-v9-0-ca98c72f1f9f@oss.qualcomm.com?part=2

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