[PATCH] arm: rockchip: rk3399: Program PLL clock for DDR at 50 MHz in documented range

[Kever Yang]

Hi Xavier,

     Thanks for your patch.

On 2022/7/16 18:31, Xavier Drudis Ferran wrote:
> The original code set up the DDR clock to 48 MHz, not 50MHz as
> requested, and did it in a way that didn't satisfy the Application
> Notes in RK3399 TRM [1]. 2.9.2.B says:
>
>     PLL frequency range requirement
>     [...]
>     FOUTVCO: 800MHz to 3.2GHz
>
> 2.9.2.A :
>     PLL output frequency configuration
>     [...]
>     FOUTVCO = FREF / REFDIV * FBDIV
>     FOUTPOSTDIV = FOUTVCO / POSTDIV1 / POSTDIV2
>
> FREF = 24 MHz
>
> The original code gives FOUTVCO: 24MHz/1 * 12 = 288MHz < 800MHz
> And the resulting FOUTPOSTDIV is 288MHz / 3 / 2 = 48MHz
> but the requested frequency was 50MHz
>
> Note:
> 2.7.2 Detail Register Description
> PMUCRU_PPLL_CON0 says
>
>     fbdiv
>     Feedback Divide Value
>     Valid divider settings are:
>     [16, 3200] in integer mode
>
> So .fbdiv = 12 wouldn't be right. But 2.9.2.C says:
>
>     PLL setting consideration
>     [...]
>     The following settings are valid for FBDIV:
>     DSMPD=1 (Integer Mode):
>     12,13,14,16-4095 (practical value is limited to 3200, 2400, or 1600
>     (FVCOMAX / FREFMIN))
>     [...]
>
> So .fbdiv = 12 would be right.
>
> In any case FOUTVCO is still wrong. I thank YouMin Chen for
> confirmation and explanation.
>
> Despite documentation, I don't seem to be able to reproduce a
> practical problem with the wrong FOUTVCO. When I initially found it I
> thought some problems with detecting the RAM capacity in my Rock Pi 4B
> could be related to it and my patch seemed to help. But since I'm no
> longer able to reproduce the issue, it works with or without this
> patch. And meanwhile a patch[2] by Lee Jones and YouMin Chen addresses
> this issue. Btw, shouldn't that be commited?
>
> So this patches solves no visible problem.  Yet, to prevent future
> problems, I think it'd be best to stick to spec.
>
> An alternative to this patch could be
>
>      {.refdiv = 1, .fbdiv = 75, .postdiv1 = 6, .postdiv2 = 6};
>
> This would theoretically consume more power and yield less jitter,
> according to 2.9.2.C :
>
>     PLL setting consideration
>     [...]
>     For lowest power operation, the minimum VCO and FREF frequencies
>     should be used. For minimum jitter operation, the highest VCO and
>     FREF frequencies should be used.
>     [...]
>
> But I haven't tried it because I don't think it matters much. 50MHz
> for DDR is only shortly used by TPL at RAM init. Normal operation is
> at 800MHz.  Maybe it's better to use less power until later when more
> complex software can control batteries or charging or whatever ?
>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Philipp Tomsich <philipp.tomsich at vrull.eu>
> Cc: Kever Yang <kever.yang at rock-chips.com>
> Cc: Lukasz Majewski <lukma at denx.de>
> Cc: Sean Anderson <seanga2 at gmail.com>
>
> Link: [1] https://www.rockchip.fr/Rockchip%20RK3399%20TRM%20V1.4%20Part1.pdf

This should be use below address instead:

https://opensource.rock-chips.com/images/e/ee/Rockchip_RK3399TRM_V1.4_Part1-20170408.pdf

>
> Link: [2] https://patchwork.ozlabs.org/project/uboot/list/?series=305766
>
> Signed-off-by: Xavier Drudis Ferran <xdrudis at tinet.cat>

Reviewed-by: Kever Yang <kever.yang at rock-chips.com>

Thanks,
- Kever
> ---
>   drivers/clk/rockchip/clk_rk3399.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/rockchip/clk_rk3399.c b/drivers/clk/rockchip/clk_rk3399.c
> index 7d31a9f22a..4762462b04 100644
> --- a/drivers/clk/rockchip/clk_rk3399.c
> +++ b/drivers/clk/rockchip/clk_rk3399.c
> @@ -840,7 +840,7 @@ static ulong rk3399_ddr_set_clk(struct rockchip_cru *cru,
>   	switch (set_rate) {
>   	case 50 * MHz:
>   		dpll_cfg = (struct pll_div)
> -		{.refdiv = 1, .fbdiv = 12, .postdiv1 = 3, .postdiv2 = 2};
> +		{.refdiv = 2, .fbdiv = 75, .postdiv1 = 3, .postdiv2 = 6};
>   		break;
>   	case 200 * MHz:
>   		dpll_cfg = (struct pll_div)