Re: [PATCH 07/15] clk: rockchip: rk3588: Fix trivial clock configuration errors

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi Jonas,

On 7/30/26 4:12 PM, Jonas Karlman wrote:
> The RK3588 clock driver has a few trivial copy-paste mistakes in its
> clock handling.
> 
> Fix the trivial clock configuration errors:
> - remove duplicate PLL_CON/MODE_CON defines

RK3588_MODE_CON is not duplicated. However, it is unused 
(RK3588_MODE_CON0 was preferred) so its removal is fine.

> - rename and use PHP_PLL_CON macro
> - use correct parent rate for TSADC clock
> - use correct parent rate for UART clocks
> - align BxPLL configuration to match other PLLs
> 

Please no cosmetic changes in a patch that has logic changes, split them 
in their own commit.

For the rest, same remark as the previous patch, when you need to list 
unrelated things, you need to split in separate commits. The individual 
changes are mostly fine, see my remark for the PHP_PLL_CON macro a few 
lines after.

> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>   arch/arm/include/asm/arch-rockchip/cru_rk3588.h |  5 +----
>   drivers/clk/rockchip/clk_rk3588.c               | 12 +++++-------
>   2 files changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch-rockchip/cru_rk3588.h b/arch/arm/include/asm/arch-rockchip/cru_rk3588.h
> index afce8a44af3b..39295adeb38a 100644
> --- a/arch/arm/include/asm/arch-rockchip/cru_rk3588.h
> +++ b/arch/arm/include/asm/arch-rockchip/cru_rk3588.h
> @@ -105,9 +105,6 @@ struct pll_rate_table {
>   	unsigned int k;
>   };
>   
> -#define RK3588_PLL_CON(x)		((x) * 0x4)
> -#define RK3588_MODE_CON			0x280
> -

Fixes: 165d258386a7 ("arm: rockchip: Add cru header for rk3588")

>   #define RK3588_PHP_CRU_BASE		0x8000
>   #define RK3588_PMU_CRU_BASE		0x30000
>   #define RK3588_BIGCORE0_CRU_BASE	0x50000
> @@ -129,10 +126,10 @@ struct pll_rate_table {
>   #define RK3588_SDMMC_CON0		0xC30
>   #define RK3588_SDMMC_CON1		0xC34
>   
> +#define RK3588_PHP_PLL_CON(x)		((x) * 0x4 + RK3588_PHP_CRU_BASE)

Shouldn't that be PHP_PPLL_CON(x)?

Also, please take this opportunity to actually fix the offset and have 
0x200 added to it already as otherwise we need to write

RK3588_PHP_PLL_CON(128)

when wanting to interact with PHPTOPCRU_PPLL_CON0, and I would much prefer

RK3588_PHP_PLL_CON(0)

instead.

Fixes: 165d258386a7 ("arm: rockchip: Add cru header for rk3588")

>   #define RK3588_PHP_CLKGATE_CON(x)	((x) * 0x4 + RK3588_PHP_CRU_BASE + 0x800)
>   #define RK3588_PHP_SOFTRST_CON(x)	((x) * 0x4 + RK3588_PHP_CRU_BASE + 0xa00)
>   
> -#define RK3588_PMU_PLL_CON(x)		((x) * 0x4 + RK3588_PHP_CRU_BASE)
>   #define RK3588_PMU_CLKSEL_CON(x)	((x) * 0x4 + RK3588_PMU_CRU_BASE + 0x300)
>   #define RK3588_PMU_CLKGATE_CON(x)	((x) * 0x4 + RK3588_PMU_CRU_BASE + 0x800)
>   #define RK3588_PMU_SOFTRST_CON(x)	((x) * 0x4 + RK3588_PMU_CRU_BASE + 0xa00)
> diff --git a/drivers/clk/rockchip/clk_rk3588.c b/drivers/clk/rockchip/clk_rk3588.c
> index b9fd4bec311b..157a02e011a5 100644
> --- a/drivers/clk/rockchip/clk_rk3588.c
> +++ b/drivers/clk/rockchip/clk_rk3588.c
> @@ -44,11 +44,9 @@ static struct rockchip_pll_rate_table rk3588_pll_rates[] = {
>   
>   static struct rockchip_pll_clock rk3588_pll_clks[] = {
>   	[B0PLL] = PLL(pll_rk3588, PLL_B0PLL, RK3588_B0_PLL_CON(0),
> -		      RK3588_B0_PLL_MODE_CON, 0, 15, 0,
> -		      rk3588_pll_rates),
> +		      RK3588_B0_PLL_MODE_CON, 0, 15, 0, rk3588_pll_rates),
>   	[B1PLL] = PLL(pll_rk3588, PLL_B1PLL, RK3588_B1_PLL_CON(8),
> -		      RK3588_B1_PLL_MODE_CON, 0, 15, 0,
> -		      rk3588_pll_rates),
> +		      RK3588_B1_PLL_MODE_CON, 0, 15, 0, rk3588_pll_rates),
>   	[LPLL] = PLL(pll_rk3588, PLL_LPLL, RK3588_LPLL_CON(16),
>   		     RK3588_LPLL_MODE_CON, 0, 15, 0, rk3588_pll_rates),
>   	[V0PLL] = PLL(pll_rk3588, PLL_V0PLL, RK3588_PLL_CON(88),
> @@ -61,7 +59,7 @@ static struct rockchip_pll_clock rk3588_pll_clks[] = {
>   		     RK3588_MODE_CON0, 2, 15, 0, rk3588_pll_rates),
>   	[NPLL] = PLL(pll_rk3588, PLL_NPLL, RK3588_PLL_CON(120),
>   		     RK3588_MODE_CON0, 0, 15, 0, rk3588_pll_rates),
> -	[PPLL] = PLL(pll_rk3588, PLL_PPLL, RK3588_PMU_PLL_CON(128),
> +	[PPLL] = PLL(pll_rk3588, PLL_PPLL, RK3588_PHP_PLL_CON(128),
>   		     RK3588_MODE_CON0, 10, 15, ROCKCHIP_PLL_FIXED_MODE,
>   		     rk3588_pll_rates),
>   #ifdef CONFIG_XPL_BUILD
> @@ -655,7 +653,7 @@ static ulong rk3588_adc_get_clk(struct rk3588_clk_priv *priv, ulong clk_id)
>   		if (sel == CLK_TSADC_SEL_24M)
>   			prate = OSC_HZ;
>   		else
> -			prate = 100 * MHz;
> +			prate = priv->gpll_hz;

We also have a few cases of *PLL_HZ being used instead of priv->*pll_hz. 
Technically, they should be the same based on the implementation  in 
rk3588_clk_init() but we're not being consistent. In any case, not a 
blocker for this here.

Fixes: 7a474df74023 ("clk: rockchip: Add rk3588 clk support")

>   		return DIV_TO_RATE(prate, div);
>   	default:
>   		return -ENOENT;
> @@ -1343,7 +1341,7 @@ static ulong rk3588_uart_set_rate(struct rk3588_clk_priv *priv,
>   	} else if (priv->cpll_hz % rate == 0) {
>   		clk_src = CLK_UART_SRC_SEL_CPLL;
>   		uart_src = CLK_UART_SEL_SRC;
> -		div = DIV_ROUND_UP(priv->gpll_hz, rate);
> +		div = DIV_ROUND_UP(priv->cpll_hz, rate);

Fixes: 7a474df74023 ("clk: rockchip: Add rk3588 clk support")

>   	} else if (rate == OSC_HZ) {
>   		clk_src = CLK_UART_SRC_SEL_GPLL;
>   		uart_src = CLK_UART_SEL_XIN24M;