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

[Michal Wilczynski]

On 9/1/26 20:14, Jonas Karlman wrote:
> 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.

Good to know, I've left them alone for now since I don't have the board
to test it, but it should be fairly easy to convert them as well.

> 
>>
>> 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@xxxxxxxxxxx>
>> ---
>>  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.

will do in v3

> 
>>  }
>>  
>>  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.

I think I can restore this debug info in the helper for v3

> 
>>  
>>  	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.

will do for v3.

Thank you for your review, as mentioned before I don't have rockchip
hardware so a Tested-by tag for after v3 is sent would be very welcome.

> 
> 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) {
>>
> 
> 

Best regards,
-- 
Michal Wilczynski <m.wilczynski@xxxxxxxxxxx>