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

[Quentin Schulz via U-Boot <u-boot@lists.u-boot-project.org>]

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