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 <[email protected]> > --- > 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) { >
