Re: [PATCH v9 5/5] phy: qualcomm: add MSM8974 HDMI PHY support

[Manivannan Sadhasivam]

On Wed, May 13, 2026 at 09:14:08PM +0300, Dmitry Baryshkov wrote:
> Add support for HDMI PHY on Qualcomm MSM8974 / APQ8074 platforms.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> Acked-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxxxxxxxx>
> ---
>  drivers/phy/qualcomm/phy-qcom-hdmi-28hpm.c | 282 +++++++++++++++++++++++++++++
>  1 file changed, 282 insertions(+)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-hdmi-28hpm.c b/drivers/phy/qualcomm/phy-qcom-hdmi-28hpm.c
> index 720757f8f393..7d398060b3a3 100644
> --- a/drivers/phy/qualcomm/phy-qcom-hdmi-28hpm.c
> +++ b/drivers/phy/qualcomm/phy-qcom-hdmi-28hpm.c
> @@ -6,10 +6,13 @@
>   * Author: Rob Clark <robdclark@xxxxxxxxx>
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/delay.h>
>  #include <linux/iopoll.h>
> +#include <linux/math64.h>
>  
>  #include "phy-qcom-hdmi-preqmp.h"
> +#include "phy-qcom-uniphy.h"
>  
>  #define REG_HDMI_8x74_ANA_CFG0					0x00000000
>  #define REG_HDMI_8x74_ANA_CFG1					0x00000004
> @@ -31,8 +34,282 @@
>  #define REG_HDMI_8x74_BIST_PATN3				0x00000048
>  #define REG_HDMI_8x74_STATUS					0x0000005c
>  
> +#define HDMI_8974_VCO_MAX_FREQ 1800000000UL
> +#define HDMI_8974_VCO_MIN_FREQ  600000000UL
> +
> +#define HDMI_8974_COMMON_DIV 5
> +
> +static inline void write16(u16 val, void __iomem *reg)

Same comment as previous patch. No 'inline' keyword in .c files.

> +{
> +	writel(val & 0xff, reg);
> +	writel(val >> 8, reg + 4);
> +}
> +
> +static inline void write24(u32 val, void __iomem *reg)
> +{
> +	writel(val & 0xff, reg);
> +	writel((val >> 8) & 0xff, reg + 4);
> +	writel(val >> 16, reg + 8);
> +}
> +
> +static inline u32 read24(void __iomem *reg)
> +{
> +	u32 val = readl(reg);
> +
> +	val |= readl(reg + 4) << 8;
> +	val |= readl(reg + 8) << 16;
> +
> +	return val;
> +}
> +
> +static void qcom_uniphy_setup(void __iomem *base, unsigned int ref_freq,
> +			      bool sdm_mode,
> +			      bool ref_freq_mult_2,
> +			      bool dither,
> +			      unsigned int refclk_div,
> +			      unsigned int vco_freq)

Nit: You can align these arguments better.

> +{
> +	unsigned int int_ref_freq = ref_freq * (ref_freq_mult_2 ? 2 : 1);
> +	unsigned int div_in_freq = vco_freq / refclk_div;
> +	unsigned int dc_offset = div_in_freq / int_ref_freq - 1;
> +	unsigned int sdm_freq_seed;
> +	unsigned int val;
> +	u64 tmp = div_in_freq % int_ref_freq;
> +
> +	tmp *= 0x10000;
> +	sdm_freq_seed = div_u64(tmp, int_ref_freq);
> +
> +	val = FIELD_PREP(UNIPHY_PLL_REFCLK_DBLR, ref_freq_mult_2) |
> +	      FIELD_PREP(UNIPHY_PLL_REFCLK_DIV, refclk_div - 1);
> +	writel(val, base + UNIPHY_PLL_REFCLK_CFG);
> +
> +	if (sdm_mode) {
> +		writel(0, base + UNIPHY_PLL_SDM_CFG0);
> +		writel(FIELD_PREP(UNIPHY_PLL_SDM_DITHER_EN, dither) | dc_offset,
> +		       base + UNIPHY_PLL_SDM_CFG1);
> +		write24(sdm_freq_seed, base + UNIPHY_PLL_SDM_CFG2);
> +	} else {
> +		writel(UNIPHY_PLL_SDM_BYP | dc_offset, base + UNIPHY_PLL_SDM_CFG0);
> +		writel(0, base + UNIPHY_PLL_SDM_CFG1);
> +		write24(0, base + UNIPHY_PLL_SDM_CFG2);
> +	}
> +
> +	write16(mult_frac(ref_freq, 5, 1000000), base + UNIPHY_PLL_CAL_CFG8);
> +	write16(vco_freq / 16, base + UNIPHY_PLL_CAL_CFG10);
> +}
> +
> +static unsigned long qcom_uniphy_recalc(void __iomem *base, unsigned long parent_rate)
> +{
> +	unsigned long rate;
> +	u32 refclk_cfg;
> +	u32 dc_offset;
> +	u64 fraq_n;
> +	u32 val;
> +
> +	refclk_cfg = readl(base + UNIPHY_PLL_REFCLK_CFG);
> +	if (refclk_cfg & UNIPHY_PLL_REFCLK_DBLR)
> +		parent_rate *= 2;
> +
> +	val = readl(base + UNIPHY_PLL_SDM_CFG0);
> +	if (FIELD_GET(UNIPHY_PLL_SDM_BYP, val)) {
> +		dc_offset = FIELD_GET(UNIPHY_PLL_SDM_BYP_DIV, val);
> +		fraq_n = 0;
> +	} else {
> +		dc_offset = FIELD_GET(UNIPHY_PLL_SDM_DC_OFFSET,
> +				      readl(base + UNIPHY_PLL_SDM_CFG1));
> +		fraq_n = read24(base + UNIPHY_PLL_SDM_CFG2);
> +	}
> +
> +	rate = (dc_offset + 1) * parent_rate;
> +	rate += div_u64(fraq_n * parent_rate, 0x10000);
> +
> +	rate *= FIELD_GET(UNIPHY_PLL_REFCLK_DIV, refclk_cfg) + 1;
> +
> +	return rate;
> +}
> +
> +static const unsigned int qcom_hdmi_8974_divs[] = {1, 2, 4, 6};
> +
> +static unsigned long qcom_hdmi_8974_pll_recalc_rate(struct clk_hw *hw,
> +						    unsigned long parent_rate)
> +{
> +	struct qcom_hdmi_preqmp_phy *hdmi_phy = hw_clk_to_phy(hw);
> +	u32 div_idx = readl(hdmi_phy->pll_reg + UNIPHY_PLL_POSTDIV1_CFG);
> +	unsigned long rate = qcom_uniphy_recalc(hdmi_phy->pll_reg, parent_rate);
> +
> +	return rate / HDMI_8974_COMMON_DIV / qcom_hdmi_8974_divs[div_idx & 0x3];
> +}
> +
> +static int qcom_hdmi_8974_pll_determine_rate(struct clk_hw *hw,
> +					     struct clk_rate_request *req)
> +{
> +	unsigned long min_freq = HDMI_8974_VCO_MIN_FREQ / HDMI_8974_COMMON_DIV;
> +	unsigned long max_freq = HDMI_8974_VCO_MAX_FREQ / HDMI_8974_COMMON_DIV;
> +
> +	req->rate = clamp(req->rate, min_freq / 6, max_freq);
> +
> +	return 0;
> +}
> +
> +static const struct clk_ops qcom_hdmi_8974_pll_ops = {
> +	.recalc_rate = qcom_hdmi_8974_pll_recalc_rate,
> +	.determine_rate = qcom_hdmi_8974_pll_determine_rate,
> +};
> +
> +static int qcom_hdmi_msm8974_phy_find_div(unsigned long long pixclk)
> +{
> +	unsigned long long min_freq = HDMI_8974_VCO_MIN_FREQ / HDMI_8974_COMMON_DIV;
> +	int i;
> +
> +	if (pixclk > HDMI_8974_VCO_MAX_FREQ / HDMI_8974_COMMON_DIV)
> +		return -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(qcom_hdmi_8974_divs); i++) {
> +		if (pixclk >= min_freq / qcom_hdmi_8974_divs[i])
> +			return i;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int qcom_hdmi_msm8974_phy_pll_set_rate(struct qcom_hdmi_preqmp_phy *hdmi_phy)
> +{
> +	unsigned long long pixclk = hdmi_phy->hdmi_opts.tmds_char_rate;
> +	unsigned long vco_rate;
> +	unsigned int div;
> +	int div_idx = 0;
> +
> +	div_idx = qcom_hdmi_msm8974_phy_find_div(pixclk);
> +	if (WARN_ON(div_idx < 0))
> +		return div_idx;
> +
> +	div = qcom_hdmi_8974_divs[div_idx];
> +	vco_rate = pixclk * HDMI_8974_COMMON_DIV * div;
> +
> +	writel(0x81, hdmi_phy->phy_reg + REG_HDMI_8x74_GLB_CFG);
> +
> +	writel(0x01, hdmi_phy->pll_reg + UNIPHY_PLL_GLB_CFG);
> +	writel(0x19, hdmi_phy->pll_reg + UNIPHY_PLL_VCOLPF_CFG);
> +	writel(0x0e, hdmi_phy->pll_reg + UNIPHY_PLL_LPFR_CFG);
> +	writel(0x20, hdmi_phy->pll_reg + UNIPHY_PLL_LPFC1_CFG);
> +	writel(0x0d, hdmi_phy->pll_reg + UNIPHY_PLL_LPFC2_CFG);
> +
> +	qcom_uniphy_setup(hdmi_phy->pll_reg, 19200000, true, true, true, 1, vco_rate);
> +
> +	writel(0x10, hdmi_phy->pll_reg + UNIPHY_PLL_LKDET_CFG0);
> +	writel(0x1a, hdmi_phy->pll_reg + UNIPHY_PLL_LKDET_CFG1);
> +	writel(0x05, hdmi_phy->pll_reg + UNIPHY_PLL_LKDET_CFG2);
> +
> +	writel(div_idx, hdmi_phy->pll_reg + UNIPHY_PLL_POSTDIV1_CFG);
> +
> +	writel(0x00, hdmi_phy->pll_reg + UNIPHY_PLL_POSTDIV2_CFG);
> +	writel(0x00, hdmi_phy->pll_reg + UNIPHY_PLL_POSTDIV3_CFG);
> +	writel(0x01, hdmi_phy->pll_reg + UNIPHY_PLL_CAL_CFG2);
> +
> +	writel(0x1f, hdmi_phy->phy_reg + REG_HDMI_8x74_PD_CTRL0);
> +	udelay(50);

You've added a lot of delays in-between register writes. Delay between register
writes makes me nervous because there is no guarantee that the previous write
before delay would've reached the hardware before the next write after delay. So
the delay might become irrelevant and can cause nasty timing issues if these
delays need to be honored.

So I'd double check and add a readl() before the delay to flush the previous
writes.

- Mani

-- 
மணிவண்ணன் சதாசிவம்