Re: [PATCH v2 12/15] phy: rockchip: inno-hdmi: Use the common Innosilicon PHY helpers

[Jonas Karlman <jonas@kwiboo.se>]

Hi Michal,

On 8/28/2026 3:47 PM, Michal Wilczynski wrote:
> The RK3328 pre-PLL programming, its table lookup and its recalc_rate and
> determine_rate implementations are the generic Innosilicon ones, so drop
> the local copies and use the shared helpers instead.

RK3528 and RK3538/RK3539 also seems to use the generic Innosilicon ones,
so we should hopefully also be able to use the helpers for those SoCs.

> 
> The RK3228 pre-PLL sits at different register addresses, so it keeps its
> own register level code and only shares the table lookup.
> 
> The register writes, their order and the values written are unchanged.
> No functional change intended.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  drivers/phy/rockchip/Kconfig                  |   1 +
>  drivers/phy/rockchip/phy-rockchip-inno-hdmi.c | 127 +++++---------------------
>  2 files changed, 25 insertions(+), 103 deletions(-)
> 
> diff --git a/drivers/phy/rockchip/Kconfig b/drivers/phy/rockchip/Kconfig
> index 14698571b607590dc0e7a29f3b39eb9a5d4e3d25..9732c5be87717dc3ddbea1c60080952336a07af9 100644
> --- a/drivers/phy/rockchip/Kconfig
> +++ b/drivers/phy/rockchip/Kconfig
> @@ -34,6 +34,7 @@ config PHY_ROCKCHIP_INNO_HDMI
>  	depends on COMMON_CLK
>  	depends on HAS_IOMEM
>  	select GENERIC_PHY
> +	select PHY_INNO_HDMI
>  	help
>  	  Enable this to support the Rockchip Innosilicon HDMI PHY.
>  
> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c b/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c
> index 1483907413faa5ce1dc8614db05e6e929410404d..fae766607b0ea120b3adb36001387062f39a5c00 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c
> @@ -17,6 +17,7 @@
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
> +#include <linux/phy/inno-hdmi-phy.h>
>  #include <linux/phy/phy.h>
>  #include <linux/slab.h>
>  
> @@ -240,6 +241,8 @@ struct inno_hdmi_phy {
>  	const struct inno_hdmi_phy_drv_data *plat_data;
>  	int chip_version;
>  
> +	struct inno_hdmi_phy_pre_pll pre_pll;
> +
>  	/* clk provider */
>  	struct clk_hw hw;
>  	struct clk *phyclk;
> @@ -247,21 +250,6 @@ struct inno_hdmi_phy {
>  	unsigned long tmdsclock;
>  };
>  
> -struct pre_pll_config {
> -	unsigned long pixclock;
> -	unsigned long tmdsclock;
> -	u8 prediv;
> -	u16 fbdiv;
> -	u8 tmds_div_a;
> -	u8 tmds_div_b;
> -	u8 tmds_div_c;
> -	u8 pclk_div_a;
> -	u8 pclk_div_b;
> -	u8 pclk_div_c;
> -	u8 pclk_div_d;
> -	u8 vco_div_5_en;
> -	u32 fracdiv;
> -};
>  
>  struct post_pll_config {
>  	unsigned long tmdsclock;
> @@ -290,7 +278,7 @@ struct inno_hdmi_phy_drv_data {
>  	const struct phy_config		*phy_cfg_table;
>  };
>  
> -static const struct pre_pll_config pre_pll_cfg_table[] = {
> +static const struct inno_hdmi_phy_pre_pll_config pre_pll_cfg_table[] = {
>  	{ 25175000,  25175000,  3,  125, 3, 1, 1,  1, 3, 3,  4, 0, 0xe00000},
>  	{ 25175000,  31468750,  1,   41, 0, 3, 3,  1, 3, 3,  4, 0, 0xf5554f},
>  	{ 27000000,  27000000,  1,   36, 0, 3, 3,  1, 2, 3,  4, 0,      0x0},
> @@ -673,20 +661,12 @@ static const struct phy_ops inno_hdmi_phy_ops = {
>  };
>  
>  static const
> -struct pre_pll_config *inno_hdmi_phy_get_pre_pll_cfg(struct inno_hdmi_phy *inno,
> -						     unsigned long rate)
> +struct inno_hdmi_phy_pre_pll_config *inno_hdmi_phy_get_pre_pll_cfg(struct inno_hdmi_phy *inno,
> +								   unsigned long rate)
>  {
> -	const struct pre_pll_config *cfg = pre_pll_cfg_table;
>  	unsigned long tmdsclock = inno_hdmi_phy_get_tmdsclk(inno, rate);
>  
> -	for (; cfg->pixclock != 0; cfg++)
> -		if (cfg->pixclock == rate && cfg->tmdsclock == tmdsclock)
> -			break;
> -
> -	if (cfg->pixclock == 0)
> -		return ERR_PTR(-EINVAL);
> -
> -	return cfg;
> +	return inno_hdmi_phy_pre_pll_lookup(&inno->pre_pll, rate, tmdsclock);

Please fully remove the local inno_hdmi_phy_get_pre_pll_cfg() helper and
call inno_hdmi_phy_pre_pll_lookup() in its place.

>  }
>  
>  static int inno_hdmi_phy_rk3228_clk_is_prepared(struct clk_hw *hw)
> @@ -752,7 +732,7 @@ unsigned long inno_hdmi_phy_rk3228_clk_recalc_rate(struct clk_hw *hw,
>  static int inno_hdmi_phy_rk3228_clk_determine_rate(struct clk_hw *hw,
>  						   struct clk_rate_request *req)
>  {
> -	const struct pre_pll_config *cfg = pre_pll_cfg_table;
> +	const struct inno_hdmi_phy_pre_pll_config *cfg = pre_pll_cfg_table;
>  
>  	req->rate = (req->rate / 1000) * 1000;
>  
> @@ -773,7 +753,7 @@ static int inno_hdmi_phy_rk3228_clk_set_rate(struct clk_hw *hw,
>  					     unsigned long parent_rate)
>  {
>  	struct inno_hdmi_phy *inno = to_inno_hdmi_phy(hw);
> -	const struct pre_pll_config *cfg;
> +	const struct inno_hdmi_phy_pre_pll_config *cfg;
>  	unsigned long tmdsclock = inno_hdmi_phy_get_tmdsclk(inno, rate);
>  	u32 v;
>  	int ret;
> @@ -870,39 +850,11 @@ unsigned long inno_hdmi_phy_rk3328_clk_recalc_rate(struct clk_hw *hw,
>  						   unsigned long parent_rate)
>  {
>  	struct inno_hdmi_phy *inno = to_inno_hdmi_phy(hw);
> -	unsigned long frac;
> -	u8 nd, no_a, no_b, no_d;
> -	u64 vco;
> -	u16 nf;
> -
> -	nd = inno_read(inno, 0xa1) & RK3328_PRE_PLL_PRE_DIV_MASK;
> -	nf = ((inno_read(inno, 0xa2) & RK3328_PRE_PLL_FB_DIV_11_8_MASK) << 8);
> -	nf |= inno_read(inno, 0xa3);
> -	vco = parent_rate * nf;
> -
> -	if (!(inno_read(inno, 0xa2) & RK3328_PRE_PLL_FRAC_DIV_DISABLE)) {
> -		frac = inno_read(inno, 0xd3) |
> -		       (inno_read(inno, 0xd2) << 8) |
> -		       (inno_read(inno, 0xd1) << 16);
> -		vco += DIV_ROUND_CLOSEST(parent_rate * frac, (1 << 24));
> -	}
>  
> -	if (inno_read(inno, 0xa0) & RK3328_PCLK_VCO_DIV_5_MASK) {
> -		do_div(vco, nd * 5);
> -	} else {
> -		no_a = inno_read(inno, 0xa5) & RK3328_PRE_PLL_PCLK_DIV_A_MASK;
> -		no_b = inno_read(inno, 0xa5) & RK3328_PRE_PLL_PCLK_DIV_B_MASK;
> -		no_b >>= RK3328_PRE_PLL_PCLK_DIV_B_SHIFT;
> -		no_b += 2;
> -		no_d = inno_read(inno, 0xa6) & RK3328_PRE_PLL_PCLK_DIV_D_MASK;
> -
> -		do_div(vco, (nd * (no_a == 1 ? no_b : no_a) * no_d * 2));
> -	}
> +	inno->pixclock = inno_hdmi_phy_pre_pll_recalc_rate(&inno->pre_pll,
> +							   parent_rate);
>  
> -	inno->pixclock = DIV_ROUND_CLOSEST((unsigned long)vco, 1000) * 1000;
> -
> -	dev_dbg(inno->dev, "%s rate %lu vco %llu\n",
> -		__func__, inno->pixclock, vco);
> +	dev_dbg(inno->dev, "%s rate %lu\n", __func__, inno->pixclock);

VCO should be between 1.4-3.2 GHz, we now loose this debug information.
This is a change that probably should be noted in the commit message.

>  
>  	return inno->pixclock;
>  }
> @@ -910,20 +862,9 @@ unsigned long inno_hdmi_phy_rk3328_clk_recalc_rate(struct clk_hw *hw,
>  static int inno_hdmi_phy_rk3328_clk_determine_rate(struct clk_hw *hw,
>  						   struct clk_rate_request *req)
>  {
> -	const struct pre_pll_config *cfg = pre_pll_cfg_table;
> -
> -	req->rate = (req->rate / 1000) * 1000;
> -
> -	for (; cfg->pixclock != 0; cfg++)
> -		if (cfg->pixclock == req->rate)
> -			break;
> -
> -	if (cfg->pixclock == 0)
> -		return -EINVAL;
> -
> -	req->rate = cfg->pixclock;
> +	struct inno_hdmi_phy *inno = to_inno_hdmi_phy(hw);
>  
> -	return 0;
> +	return inno_hdmi_phy_pre_pll_determine_rate(&inno->pre_pll, req);
>  }
>  
>  static int inno_hdmi_phy_rk3328_clk_set_rate(struct clk_hw *hw,
> @@ -931,9 +872,8 @@ static int inno_hdmi_phy_rk3328_clk_set_rate(struct clk_hw *hw,
>  					     unsigned long parent_rate)
>  {
>  	struct inno_hdmi_phy *inno = to_inno_hdmi_phy(hw);
> -	const struct pre_pll_config *cfg;
> +	const struct inno_hdmi_phy_pre_pll_config *cfg;
>  	unsigned long tmdsclock = inno_hdmi_phy_get_tmdsclk(inno, rate);
> -	u32 val;
>  	int ret;
>  
>  	dev_dbg(inno->dev, "%s rate %lu tmdsclk %lu\n",
> @@ -946,35 +886,11 @@ static int inno_hdmi_phy_rk3328_clk_set_rate(struct clk_hw *hw,
>  	if (IS_ERR(cfg))
>  		return PTR_ERR(cfg);
>  
> -	inno_update_bits(inno, 0xa0, RK3328_PRE_PLL_POWER_DOWN,
> -			 RK3328_PRE_PLL_POWER_DOWN);
> -
> -	/* Configure pre-pll */
> -	inno_update_bits(inno, 0xa0, RK3328_PCLK_VCO_DIV_5_MASK,
> -			 RK3328_PCLK_VCO_DIV_5(cfg->vco_div_5_en));
> -	inno_write(inno, 0xa1, RK3328_PRE_PLL_PRE_DIV(cfg->prediv));
> -
> -	val = RK3328_SPREAD_SPECTRUM_MOD_DISABLE;
> -	if (!cfg->fracdiv)
> -		val |= RK3328_PRE_PLL_FRAC_DIV_DISABLE;
> -	inno_write(inno, 0xa2, RK3328_PRE_PLL_FB_DIV_11_8(cfg->fbdiv) | val);
> -	inno_write(inno, 0xa3, RK3328_PRE_PLL_FB_DIV_7_0(cfg->fbdiv));
> -	inno_write(inno, 0xa5, RK3328_PRE_PLL_PCLK_DIV_A(cfg->pclk_div_a) |
> -		   RK3328_PRE_PLL_PCLK_DIV_B(cfg->pclk_div_b));
> -	inno_write(inno, 0xa6, RK3328_PRE_PLL_PCLK_DIV_C(cfg->pclk_div_c) |
> -		   RK3328_PRE_PLL_PCLK_DIV_D(cfg->pclk_div_d));
> -	inno_write(inno, 0xa4, RK3328_PRE_PLL_TMDSCLK_DIV_C(cfg->tmds_div_c) |
> -		   RK3328_PRE_PLL_TMDSCLK_DIV_A(cfg->tmds_div_a) |
> -		   RK3328_PRE_PLL_TMDSCLK_DIV_B(cfg->tmds_div_b));
> -	inno_write(inno, 0xd3, RK3328_PRE_PLL_FRAC_DIV_7_0(cfg->fracdiv));
> -	inno_write(inno, 0xd2, RK3328_PRE_PLL_FRAC_DIV_15_8(cfg->fracdiv));
> -	inno_write(inno, 0xd1, RK3328_PRE_PLL_FRAC_DIV_23_16(cfg->fracdiv));
> +	inno_hdmi_phy_pre_pll_power_down(&inno->pre_pll, true);
> +	inno_hdmi_phy_pre_pll_configure(&inno->pre_pll, cfg);
> +	inno_hdmi_phy_pre_pll_power_down(&inno->pre_pll, false);
>  
> -	inno_update_bits(inno, 0xa0, RK3328_PRE_PLL_POWER_DOWN, 0);

This should likely also remove all now unused RK3328 pre_pll related
macros.

Regards,
Jonas

> -
> -	/* Wait for Pre-PLL lock */
> -	ret = inno_poll(inno, 0xa9, val, val & RK3328_PRE_PLL_LOCK_STATUS,
> -			1000, 10000);
> +	ret = inno_hdmi_phy_pre_pll_wait_locked(&inno->pre_pll, 10000);
>  	if (ret) {
>  		dev_err(inno->dev, "Pre-PLL locking failed\n");
>  		return ret;
> @@ -1373,6 +1289,11 @@ static int inno_hdmi_phy_probe(struct platform_device *pdev)
>  	if (IS_ERR(inno->regmap))
>  		return PTR_ERR(inno->regmap);
>  
> +	/* The PHY block starts the register space on these SoCs. */
> +	inno->pre_pll.regmap = inno->regmap;
> +	inno->pre_pll.offset = 0;
> +	inno->pre_pll.table = pre_pll_cfg_table;
> +
>  	/* only the newer rk3328 hdmiphy has an interrupt */
>  	inno->irq = platform_get_irq(pdev, 0);
>  	if (inno->irq > 0) {
>