[PATCH 1/2] clk: rockchip: clk-pll: Fix CRU_xx_CON2 register usage

[PATCH 1/2] clk: rockchip: clk-pll: Fix CRU_xx_CON2 register usage

[Alexander Shiyan]

According to RK3588 TRM, CRU_(CPLL/GPLL/etc)_CON2 register
(rate-k value) does not use highword write enable mask.
Lets fix this.

Signed-off-by: Alexander Shiyan <eagle.alexander923@gmail.com>
---
 drivers/clk/rockchip/clk-pll.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
index 2c2abb3b4210..77ba4d6e7b5f 100644
--- a/drivers/clk/rockchip/clk-pll.c
+++ b/drivers/clk/rockchip/clk-pll.c
@@ -959,7 +959,7 @@ static int rockchip_rk3588_pll_set_params(struct rockchip_clk_pll *pll,
 		       HIWORD_UPDATE(rate->s, RK3588_PLLCON1_S_MASK, RK3588_PLLCON1_S_SHIFT),
 		       pll->reg_base + RK3399_PLLCON(1));
 
-	writel_relaxed(HIWORD_UPDATE(rate->k, RK3588_PLLCON2_K_MASK, RK3588_PLLCON2_K_SHIFT),
+	writel_relaxed((rate->k & RK3588_PLLCON2_K_MASK) << RK3588_PLLCON2_K_SHIFT,
 		       pll->reg_base + RK3399_PLLCON(2));
 
 	/* set pll power up */
-- 
2.39.1

[PATCH 2/2] clk: rockchip: clk-pll: Typo fix

[Alexander Shiyan]

Fixed RK3399 -> RK3588 typo.
No functional changes.

Signed-off-by: Alexander Shiyan <eagle.alexander923@gmail.com>
---
 drivers/clk/rockchip/clk-pll.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
index 77ba4d6e7b5f..6c5039bd661f 100644
--- a/drivers/clk/rockchip/clk-pll.c
+++ b/drivers/clk/rockchip/clk-pll.c
@@ -949,18 +949,18 @@ static int rockchip_rk3588_pll_set_params(struct rockchip_clk_pll *pll,
 	/* set pll power down */
 	writel(HIWORD_UPDATE(RK3588_PLLCON1_PWRDOWN,
 			     RK3588_PLLCON1_PWRDOWN, 0),
-	       pll->reg_base + RK3399_PLLCON(1));
+	       pll->reg_base + RK3588_PLLCON(1));
 
 	/* update pll values */
 	writel_relaxed(HIWORD_UPDATE(rate->m, RK3588_PLLCON0_M_MASK, RK3588_PLLCON0_M_SHIFT),
-		       pll->reg_base + RK3399_PLLCON(0));
+		       pll->reg_base + RK3588_PLLCON(0));
 
 	writel_relaxed(HIWORD_UPDATE(rate->p, RK3588_PLLCON1_P_MASK, RK3588_PLLCON1_P_SHIFT) |
 		       HIWORD_UPDATE(rate->s, RK3588_PLLCON1_S_MASK, RK3588_PLLCON1_S_SHIFT),
-		       pll->reg_base + RK3399_PLLCON(1));
+		       pll->reg_base + RK3588_PLLCON(1));
 
 	writel_relaxed((rate->k & RK3588_PLLCON2_K_MASK) << RK3588_PLLCON2_K_SHIFT,
-		       pll->reg_base + RK3399_PLLCON(2));
+		       pll->reg_base + RK3588_PLLCON(2));
 
 	/* set pll power up */
 	writel(HIWORD_UPDATE(0, RK3588_PLLCON1_PWRDOWN, 0),
-- 
2.39.1

Re: [PATCH 1/2] clk: rockchip: clk-pll: Fix CRU_xx_CON2 register usage

[Heiko Stübner]

Hi Alexander,

Am Dienstag, 8. April 2025, 08:31:25 Mitteleuropäische Sommerzeit schrieb Alexander Shiyan:
> According to RK3588 TRM, CRU_(CPLL/GPLL/etc)_CON2 register
> (rate-k value) does not use highword write enable mask.
> Lets fix this.
> 
> Signed-off-by: Alexander Shiyan <eagle.alexander923@gmail.com>

The commit message doesn't say, but did you check this on actual
hardware too?

Sometimes there is a disconnect between the TRM and actual hardware,
so the actual real-life situation should be checked.


As for a test-case, any write without write-mask to a register that
would require a write-mask would not come through.
So with your patch applied, does the register value change after
the write below when reading it back again?


Thanks
Heiko


> ---
>  drivers/clk/rockchip/clk-pll.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
> index 2c2abb3b4210..77ba4d6e7b5f 100644
> --- a/drivers/clk/rockchip/clk-pll.c
> +++ b/drivers/clk/rockchip/clk-pll.c
> @@ -959,7 +959,7 @@ static int rockchip_rk3588_pll_set_params(struct rockchip_clk_pll *pll,
>  		       HIWORD_UPDATE(rate->s, RK3588_PLLCON1_S_MASK, RK3588_PLLCON1_S_SHIFT),
>  		       pll->reg_base + RK3399_PLLCON(1));
>  
> -	writel_relaxed(HIWORD_UPDATE(rate->k, RK3588_PLLCON2_K_MASK, RK3588_PLLCON2_K_SHIFT),
> +	writel_relaxed((rate->k & RK3588_PLLCON2_K_MASK) << RK3588_PLLCON2_K_SHIFT,
>  		       pll->reg_base + RK3399_PLLCON(2));
>  
>  	/* set pll power up */
> 

Re: [PATCH 1/2] clk: rockchip: clk-pll: Fix CRU_xx_CON2 register usage

[Alexander Shiyan]

Hello.

Yes, everything works as expected:

# mw 0xfd7c01a8 0x00001234
# md 0xfd7c01a8+4
fd7c01a8: 00001234
# mw 0xfd7c01a8 0x00005678
# md 0xfd7c01a8+4
fd7c01a8: 00005678

вт, 8 апр. 2025 г. в 10:31, Heiko Stübner <heiko@sntech.de>:
>
> Hi Alexander,
>
> Am Dienstag, 8. April 2025, 08:31:25 Mitteleuropäische Sommerzeit schrieb Alexander Shiyan:
> > According to RK3588 TRM, CRU_(CPLL/GPLL/etc)_CON2 register
> > (rate-k value) does not use highword write enable mask.
> > Lets fix this.
> >
> > Signed-off-by: Alexander Shiyan <eagle.alexander923@gmail.com>
>
> The commit message doesn't say, but did you check this on actual
> hardware too?
>
> Sometimes there is a disconnect between the TRM and actual hardware,
> so the actual real-life situation should be checked.
>
>
> As for a test-case, any write without write-mask to a register that
> would require a write-mask would not come through.
> So with your patch applied, does the register value change after
> the write below when reading it back again?
>
>
> Thanks
> Heiko
>
>
> > ---
> >  drivers/clk/rockchip/clk-pll.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
> > index 2c2abb3b4210..77ba4d6e7b5f 100644
> > --- a/drivers/clk/rockchip/clk-pll.c
> > +++ b/drivers/clk/rockchip/clk-pll.c
> > @@ -959,7 +959,7 @@ static int rockchip_rk3588_pll_set_params(struct rockchip_clk_pll *pll,
> >                      HIWORD_UPDATE(rate->s, RK3588_PLLCON1_S_MASK, RK3588_PLLCON1_S_SHIFT),
> >                      pll->reg_base + RK3399_PLLCON(1));
> >
> > -     writel_relaxed(HIWORD_UPDATE(rate->k, RK3588_PLLCON2_K_MASK, RK3588_PLLCON2_K_SHIFT),
> > +     writel_relaxed((rate->k & RK3588_PLLCON2_K_MASK) << RK3588_PLLCON2_K_SHIFT,
> >                      pll->reg_base + RK3399_PLLCON(2));
> >
> >       /* set pll power up */
> >
>
>
>
>