[PATCH 11/15] clk: rockchip: rk3576: Fix trivial clock configuration errors

[PATCH 11/15] clk: rockchip: rk3576: Fix trivial clock configuration errors

[Jonas Karlman]

The RK3576 clock driver has a few trivial copy-paste mistakes in its
clock handling.

Fix the trivial clock configuration errors:
- use correct VPLL mode reg
- rename and use PHP_PLL_CON macro
- set correct parent for ACLK_TOP clocks
- avoid overriding the selected I2C parent clock
- stop CLK_I2C8 from falling through into CLK_I2C9
- use correct SARADC and TSADC clksel regs
- use correct parent pll rate for UART clocks
- align BPLL configuration to match other PLLs
- remove unused BPLL_CON macro

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 .../include/asm/arch-rockchip/cru_rk3576.h    |  5 ++--
 drivers/clk/rockchip/clk_rk3576.c             | 23 +++++++++----------
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/arch/arm/include/asm/arch-rockchip/cru_rk3576.h b/arch/arm/include/asm/arch-rockchip/cru_rk3576.h
index fb77fbd7307a..41e225245843 100644
--- a/arch/arm/include/asm/arch-rockchip/cru_rk3576.h
+++ b/arch/arm/include/asm/arch-rockchip/cru_rk3576.h
@@ -127,24 +127,23 @@ struct pll_rate_table {
 #define RK3576_SDMMC_CON0		0xC30
 #define RK3576_SDMMC_CON1		0xC34
 
+#define RK3576_PHP_PLL_CON(x)		((x) * 0x4 + RK3576_PHP_CRU_BASE)
 #define RK3576_PHP_CLKSEL_CON(x)	((x) * 0x4 + RK3576_PHP_CRU_BASE + 0x300)
 #define RK3576_PHP_CLKGATE_CON(x)	((x) * 0x4 + RK3576_PHP_CRU_BASE + 0x800)
 #define RK3576_PHP_SOFTRST_CON(x)	((x) * 0x4 + RK3576_PHP_CRU_BASE + 0xa00)
 
-#define RK3576_PMU_PLL_CON(x)		((x) * 0x4 + RK3576_PHP_CRU_BASE)
 #define RK3576_PMU_CLKSEL_CON(x)	((x) * 0x4 + RK3576_PMU_CRU_BASE + 0x300)
 #define RK3576_PMU_CLKGATE_CON(x)	((x) * 0x4 + RK3576_PMU_CRU_BASE + 0x800)
 #define RK3576_PMU_SOFTRST_CON(x)	((x) * 0x4 + RK3576_PMU_CRU_BASE + 0xa00)
 
+#define RK3576_LPLL_CON(x)		((x) * 0x4 + RK3576_CCI_CRU_BASE)
 #define RK3576_CCI_CLKSEL_CON(x)	((x) * 0x4 + RK3576_CCI_CRU_BASE + 0x300)
 #define RK3576_CCI_CLKGATE_CON(x)	((x) * 0x4 + RK3576_CCI_CRU_BASE + 0x800)
 #define RK3576_CCI_SOFTRST_CON(x)	((x) * 0x4 + RK3576_CCI_CRU_BASE + 0xa00)
 
-#define RK3576_BPLL_CON(x)		((x) * 0x4 + RK3576_BIGCORE_CRU_BASE)
 #define RK3576_BIGCORE_CLKSEL_CON(x)	((x) * 0x4 + RK3576_BIGCORE_CRU_BASE + 0x300)
 #define RK3576_BIGCORE_CLKGATE_CON(x)	((x) * 0x4 + RK3576_BIGCORE_CRU_BASE + 0x800)
 #define RK3576_BIGCORE_SOFTRST_CON(x)	((x) * 0x4 + RK3576_BIGCORE_CRU_BASE + 0xa00)
-#define RK3576_LPLL_CON(x)		((x) * 0x4 + RK3576_CCI_CRU_BASE)
 #define RK3576_LITCORE_CLKSEL_CON(x)	((x) * 0x4 + RK3576_LITCORE_CRU_BASE + 0x300)
 #define RK3576_LITCORE_CLKGATE_CON(x)	((x) * 0x4 + RK3576_LITCORE_CRU_BASE + 0x800)
 #define RK3576_LITCORE_SOFTRST_CON(x)	((x) * 0x4 + RK3576_LITCORE_CRU_BASE + 0xa00)
diff --git a/drivers/clk/rockchip/clk_rk3576.c b/drivers/clk/rockchip/clk_rk3576.c
index 92bde425b0ee..75b705ffba2f 100644
--- a/drivers/clk/rockchip/clk_rk3576.c
+++ b/drivers/clk/rockchip/clk_rk3576.c
@@ -44,19 +44,18 @@ static struct rockchip_pll_rate_table rk3576_24m_pll_rates[] = {
 
 static struct rockchip_pll_clock rk3576_pll_clks[] = {
 	[BPLL] = PLL(pll_rk3588, PLL_BPLL, RK3576_PLL_CON(0),
-		      RK3576_BPLL_MODE_CON0, 0, 15, 0,
-		      rk3576_24m_pll_rates),
+		     RK3576_BPLL_MODE_CON0, 0, 15, 0, rk3576_24m_pll_rates),
 	[LPLL] = PLL(pll_rk3588, PLL_LPLL, RK3576_LPLL_CON(16),
 		     RK3576_LPLL_MODE_CON0, 0, 15, 0, rk3576_24m_pll_rates),
 	[VPLL] = PLL(pll_rk3588, PLL_VPLL, RK3576_PLL_CON(88),
-		      RK3576_LPLL_MODE_CON0, 4, 15, 0, rk3576_24m_pll_rates),
+		     RK3576_MODE_CON0, 4, 15, 0, rk3576_24m_pll_rates),
 	[AUPLL] = PLL(pll_rk3588, PLL_AUPLL, RK3576_PLL_CON(96),
 		      RK3576_MODE_CON0, 6, 15, 0, rk3576_24m_pll_rates),
 	[CPLL] = PLL(pll_rk3588, PLL_CPLL, RK3576_PLL_CON(104),
 		     RK3576_MODE_CON0, 8, 15, 0, rk3576_24m_pll_rates),
 	[GPLL] = PLL(pll_rk3588, PLL_GPLL, RK3576_PLL_CON(112),
 		     RK3576_MODE_CON0, 2, 15, 0, rk3576_24m_pll_rates),
-	[PPLL] = PLL(pll_rk3588, PLL_PPLL, RK3576_PMU_PLL_CON(128),
+	[PPLL] = PLL(pll_rk3588, PLL_PPLL, RK3576_PHP_PLL_CON(128),
 		     RK3576_MODE_CON0, 10, 15, ROCKCHIP_PLL_FIXED_MODE,
 		     rk3576_24m_pll_rates),
 };
@@ -320,8 +319,7 @@ static ulong rk3576_top_set_clk(struct rk3576_clk_priv *priv,
 		rk_clrsetreg(&cru->clksel_con[10],
 			     ACLK_TOP_MID_DIV_MASK |
 			     ACLK_TOP_MID_SEL_MASK,
-			     (ACLK_TOP_MID_SEL_GPLL <<
-			      ACLK_TOP_MID_SEL_SHIFT) |
+			     (src_clk << ACLK_TOP_MID_SEL_SHIFT) |
 			     (src_clk_div - 1) << ACLK_TOP_MID_DIV_SHIFT);
 		break;
 	case PCLK_TOP_ROOT:
@@ -429,7 +427,7 @@ static ulong rk3576_i2c_set_clk(struct rk3576_clk_priv *priv, ulong clk_id,
 		src_clk = CLK_I2C_SEL_200M;
 	else if (rate >= 99 * MHz)
 		src_clk = CLK_I2C_SEL_100M;
-	if (rate >= 50 * MHz)
+	else if (rate >= 50 * MHz)
 		src_clk = CLK_I2C_SEL_50M;
 	else
 		src_clk = CLK_I2C_SEL_OSC;
@@ -470,6 +468,7 @@ static ulong rk3576_i2c_set_clk(struct rk3576_clk_priv *priv, ulong clk_id,
 	case CLK_I2C8:
 		rk_clrsetreg(&cru->clksel_con[57], CLK_I2C8_SEL_MASK,
 			     src_clk << CLK_I2C8_SEL_SHIFT);
+		break;
 	case CLK_I2C9:
 		rk_clrsetreg(&cru->clksel_con[58], CLK_I2C9_SEL_MASK,
 			     src_clk << CLK_I2C9_SEL_SHIFT);
@@ -691,7 +690,7 @@ static ulong rk3576_adc_set_clk(struct rk3576_clk_priv *priv,
 		} else {
 			src_clk_div = DIV_ROUND_UP(priv->gpll_hz, rate);
 			assert(src_clk_div - 1 <= 255);
-			rk_clrsetreg(&cru->clksel_con[59],
+			rk_clrsetreg(&cru->clksel_con[58],
 				     CLK_SARADC_SEL_MASK |
 				     CLK_SARADC_DIV_MASK,
 				     (CLK_SARADC_SEL_GPLL <<
@@ -703,7 +702,7 @@ static ulong rk3576_adc_set_clk(struct rk3576_clk_priv *priv,
 	case CLK_TSADC:
 		src_clk_div = DIV_ROUND_UP(OSC_HZ, rate);
 		assert(src_clk_div - 1 <= 255);
-		rk_clrsetreg(&cru->clksel_con[58],
+		rk_clrsetreg(&cru->clksel_con[59],
 			     CLK_TSADC_DIV_MASK,
 			     (src_clk_div - 1) <<
 			     CLK_TSADC_DIV_SHIFT);
@@ -1715,7 +1714,7 @@ static ulong rk3576_uart_frac_set_rate(struct rk3576_clk_priv *priv,
 		p_rate = OSC_HZ;
 	} else {
 		clk_src = CLK_UART_SRC_SEL_GPLL;
-		p_rate = priv->cpll_hz;
+		p_rate = priv->gpll_hz;
 	}
 
 	rational_best_approximation(rate, p_rate, GENMASK(16 - 1, 0),
@@ -1843,7 +1842,7 @@ static ulong rk3576_uart_set_rate(struct rk3576_clk_priv *priv,
 		div = DIV_ROUND_UP(priv->gpll_hz, rate);
 	} else if (!(priv->cpll_hz % rate)) {
 		clk_src = CLK_UART_SEL_CPLL;
-		div = DIV_ROUND_UP(priv->gpll_hz, rate);
+		div = DIV_ROUND_UP(priv->cpll_hz, rate);
 	} else if (!(rk3576_uart_frac_get_rate(priv, CLK_UART_FRAC_0) % rate)) {
 		clk_src = CLK_UART_SEL_FRAC0;
 		div = DIV_ROUND_UP(rk3576_uart_frac_get_rate(priv, CLK_UART_FRAC_0), rate);
@@ -2458,7 +2457,7 @@ static int rk3576_clk_probe(struct udevice *dev)
 	       RK3576_SCRU_BASE + RK3576_MODE_CON0);
 	/* fix ppll\aupll\cpll */
 	writel(BITS_WITH_WMASK(2, 0x7U, 6),
-	       RK3576_CRU_BASE + RK3576_PMU_PLL_CON(129));
+	       RK3576_CRU_BASE + RK3576_PHP_PLL_CON(129));
 	writel(BITS_WITH_WMASK(2, 0x7U, 6),
 	       RK3576_CRU_BASE + RK3576_PLL_CON(97));
 	writel(BITS_WITH_WMASK(2, 0x7U, 6),
-- 
2.54.0

Re: [PATCH 11/15] clk: rockchip: rk3576: Fix trivial clock configuration errors

[Quentin Schulz via U-Boot]

Hi Jonas,

On 7/30/26 4:13 PM, Jonas Karlman wrote:
> The RK3576 clock driver has a few trivial copy-paste mistakes in its
> clock handling.
> 
> Fix the trivial clock configuration errors:
> - use correct VPLL mode reg
> - rename and use PHP_PLL_CON macro
> - set correct parent for ACLK_TOP clocks
> - avoid overriding the selected I2C parent clock
> - stop CLK_I2C8 from falling through into CLK_I2C9
> - use correct SARADC and TSADC clksel regs
> - use correct parent pll rate for UART clocks
> - align BPLL configuration to match other PLLs
> - remove unused BPLL_CON macro
> 

Please split those into separate commits. All changes are fine 
individually. See small remark below for a change I believe would help 
with reading the code more easily.

> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>   .../include/asm/arch-rockchip/cru_rk3576.h    |  5 ++--
>   drivers/clk/rockchip/clk_rk3576.c             | 23 +++++++++----------
>   2 files changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch-rockchip/cru_rk3576.h b/arch/arm/include/asm/arch-rockchip/cru_rk3576.h
> index fb77fbd7307a..41e225245843 100644
> --- a/arch/arm/include/asm/arch-rockchip/cru_rk3576.h
> +++ b/arch/arm/include/asm/arch-rockchip/cru_rk3576.h
> @@ -127,24 +127,23 @@ struct pll_rate_table {
>   #define RK3576_SDMMC_CON0		0xC30
>   #define RK3576_SDMMC_CON1		0xC34
>   
> +#define RK3576_PHP_PLL_CON(x)		((x) * 0x4 + RK3576_PHP_CRU_BASE)

Same remark as for the RK3588 patch, please add 0x200 so we can use 
RK3576_PHP_PLL_CON(0) when we want to interact with PHPTOPCRU_PPLL_CON0.

>   #define RK3576_PHP_CLKSEL_CON(x)	((x) * 0x4 + RK3576_PHP_CRU_BASE + 0x300)
>   #define RK3576_PHP_CLKGATE_CON(x)	((x) * 0x4 + RK3576_PHP_CRU_BASE + 0x800)
>   #define RK3576_PHP_SOFTRST_CON(x)	((x) * 0x4 + RK3576_PHP_CRU_BASE + 0xa00)
>   
> -#define RK3576_PMU_PLL_CON(x)		((x) * 0x4 + RK3576_PHP_CRU_BASE)
>   #define RK3576_PMU_CLKSEL_CON(x)	((x) * 0x4 + RK3576_PMU_CRU_BASE + 0x300)
>   #define RK3576_PMU_CLKGATE_CON(x)	((x) * 0x4 + RK3576_PMU_CRU_BASE + 0x800)
>   #define RK3576_PMU_SOFTRST_CON(x)	((x) * 0x4 + RK3576_PMU_CRU_BASE + 0xa00)
>   
> +#define RK3576_LPLL_CON(x)		((x) * 0x4 + RK3576_CCI_CRU_BASE)

Please add 0x40 so we can do RK3576_LPLL_CON(0) to interact with 
CCICRU_LPLL_CON0.

>   #define RK3576_CCI_CLKSEL_CON(x)	((x) * 0x4 + RK3576_CCI_CRU_BASE + 0x300)
>   #define RK3576_CCI_CLKGATE_CON(x)	((x) * 0x4 + RK3576_CCI_CRU_BASE + 0x800)
>   #define RK3576_CCI_SOFTRST_CON(x)	((x) * 0x4 + RK3576_CCI_CRU_BASE + 0xa00)
>   
> -#define RK3576_BPLL_CON(x)		((x) * 0x4 + RK3576_BIGCORE_CRU_BASE)
>   #define RK3576_BIGCORE_CLKSEL_CON(x)	((x) * 0x4 + RK3576_BIGCORE_CRU_BASE + 0x300)
>   #define RK3576_BIGCORE_CLKGATE_CON(x)	((x) * 0x4 + RK3576_BIGCORE_CRU_BASE + 0x800)
>   #define RK3576_BIGCORE_SOFTRST_CON(x)	((x) * 0x4 + RK3576_BIGCORE_CRU_BASE + 0xa00)
> -#define RK3576_LPLL_CON(x)		((x) * 0x4 + RK3576_CCI_CRU_BASE)
>   #define RK3576_LITCORE_CLKSEL_CON(x)	((x) * 0x4 + RK3576_LITCORE_CRU_BASE + 0x300)
>   #define RK3576_LITCORE_CLKGATE_CON(x)	((x) * 0x4 + RK3576_LITCORE_CRU_BASE + 0x800)
>   #define RK3576_LITCORE_SOFTRST_CON(x)	((x) * 0x4 + RK3576_LITCORE_CRU_BASE + 0xa00)
> diff --git a/drivers/clk/rockchip/clk_rk3576.c b/drivers/clk/rockchip/clk_rk3576.c
> index 92bde425b0ee..75b705ffba2f 100644
> --- a/drivers/clk/rockchip/clk_rk3576.c
> +++ b/drivers/clk/rockchip/clk_rk3576.c
> @@ -44,19 +44,18 @@ static struct rockchip_pll_rate_table rk3576_24m_pll_rates[] = {
>   
>   static struct rockchip_pll_clock rk3576_pll_clks[] = {
>   	[BPLL] = PLL(pll_rk3588, PLL_BPLL, RK3576_PLL_CON(0),
> -		      RK3576_BPLL_MODE_CON0, 0, 15, 0,
> -		      rk3576_24m_pll_rates),
> +		     RK3576_BPLL_MODE_CON0, 0, 15, 0, rk3576_24m_pll_rates),
>   	[LPLL] = PLL(pll_rk3588, PLL_LPLL, RK3576_LPLL_CON(16),
>   		     RK3576_LPLL_MODE_CON0, 0, 15, 0, rk3576_24m_pll_rates),
>   	[VPLL] = PLL(pll_rk3588, PLL_VPLL, RK3576_PLL_CON(88),
> -		      RK3576_LPLL_MODE_CON0, 4, 15, 0, rk3576_24m_pll_rates),
> +		     RK3576_MODE_CON0, 4, 15, 0, rk3576_24m_pll_rates),
>   	[AUPLL] = PLL(pll_rk3588, PLL_AUPLL, RK3576_PLL_CON(96),
>   		      RK3576_MODE_CON0, 6, 15, 0, rk3576_24m_pll_rates),
>   	[CPLL] = PLL(pll_rk3588, PLL_CPLL, RK3576_PLL_CON(104),
>   		     RK3576_MODE_CON0, 8, 15, 0, rk3576_24m_pll_rates),
>   	[GPLL] = PLL(pll_rk3588, PLL_GPLL, RK3576_PLL_CON(112),
>   		     RK3576_MODE_CON0, 2, 15, 0, rk3576_24m_pll_rates),
> -	[PPLL] = PLL(pll_rk3588, PLL_PPLL, RK3576_PMU_PLL_CON(128),
> +	[PPLL] = PLL(pll_rk3588, PLL_PPLL, RK3576_PHP_PLL_CON(128),
>   		     RK3576_MODE_CON0, 10, 15, ROCKCHIP_PLL_FIXED_MODE,

RK3576_MODE_CON0 is incorrect here, but since ROCKCHIP_PLL_FIXED_MODE is 
set, this won't be used as far as I could tell. I'm wondering whether we 
should have a new macros that wouldn't force us to define something 
necessarily incorrect. Something for later though.

Cheers,
Quentin

Re: [PATCH 11/15] clk: rockchip: rk3576: Fix trivial clock configuration errors

[pcb]

Hi Jonas, Quentin, 

We are an embedded design house building products on the RK3576, and this series lines up with several clock-related issues we have hit during our own U-Boot bring-up. 
Thanks for putting it together ― and for the honest note that some of these fixes are only compile-tested. That is exactly the gap we can help close. 
On the RK3576 side specifically, the fixes in patch 11 that matter most to us in production are the VPLL mode register correction (we rely on VPLL for display clocks) 
and the PPLL base fix from PMU domain to PHP domain (PPLL feeds the DRAM controller, so a wrong register window here is the kind of thing that only shows up as intermittent instability rather than a clean failure). 

Happy to share more concrete observations if useful. On Quentin's point about RK3576_MODE_CON0 being incorrect for PPLL but masked by ROCKCHIP_PLL_FIXED_MODE: I agree a new macro is the right long-term direction, 
and I would add one more angle ― the same latent hazard exists in the RK3588 PPLL entry, so whatever we settle on should be applied to both drivers in the same series. 
In the meantime, a compile-time assertion in the PLL() macro (e.g. BUILD_BUG_ON when FIXED_MODE is set but a mode register is still required) would prevent future contributors from silently reintroducing the wrong address.
If useful, we can draft that as a follow-up patch. Finally, if a v2 is not already in flight: we would be happy to test patches 4, 5, and 11 on our RK3576 reference hardware (DDR init, VPLL/display bring-up, and I2C parent selection are all exercised during our regular boot testing) and provide Tested-by tags plus any rate mismatches we observe. Best regards,

Owen
Boardcon Embedded Design Limited
https://www.boardcon.com