[U-Boot] [PATCH 09/14] sun6i: Add k and m parameters to clock_set_pll5()

[Siarhei Siamashka]

On Fri, 19 Dec 2014 17:40:27 +0100
Hans de Goede <hdegoede at redhat.com> wrote:

> Hi,
> 
> On 19-12-14 11:03, Siarhei Siamashka wrote:
> > On Tue, 16 Dec 2014 21:31:34 +0100
> > Hans de Goede <hdegoede at redhat.com> wrote:
> >
> >> The A23 (sun8i) requires different values for these then sun6i, so make them
> >> function parameters.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> >
> > What happens if A23 does not get these special k and m parameters, but
> > the 'clock_set_pll5' function picks some other values for them (with
> > the same resulting target clock speed)?
> 
> The major difference is that on the A23 pll5 must be set to dram_clk / 2,
> where as on A31 it needs to be set to dram_clk * 2. By codifying k and m
> so that they do the * 2 on A31 and / 2 on A23 we can make any multiple of
> 24 MHz as DRAM clk without needing any other code to figure out the optimal
> k and m.

Wouldn't such other code in 'clock_set_pll5' just look like this
(adds only a few extra instructions with an optimizing compiler):

    const int max_n = 32;
    int k = 1, m = 2;
    if (clk > 24000000 * k * max_n / m) {
        m = 1;
        if (clk > 24000000 * k * max_n / m)
            k = 2;
    }

As an additional bonus, you get 12 MHz clock frequency selection
granularity on A31 for the DRAM clock speeds up to 384 MHz. And
keep the current 24 MHz granularity for anything larger than that.

> AFAIK using other k and m factors with the same end-result should
> work fine.

This statement seems to somewhat contradict with your original commit
message. Should the commit message be updated?

> But doing things this way follows the KISS principle and I'm
> a great fan of KISS.

By instead spreading the complexity all across the sources? So that each
caller code needs to make assumptions about the valid clock frequency
range and statically pick the right multiplier/divisor parameters as
an additional burden?

In what way is it simple?


> 
> Regards,
> 
> Hans
> 
> 
> 
> >
> > And if they are really required for A23, then why exposing them as the
> > function arguments instead of hiding this implementation detail inside
> > of the 'clock_set_pll5' function?
> >
> >> ---
> >>   arch/arm/cpu/armv7/sunxi/clock_sun6i.c        | 4 +---
> >>   arch/arm/cpu/armv7/sunxi/dram_sun6i.c         | 2 +-
> >>   arch/arm/include/asm/arch-sunxi/clock_sun6i.h | 2 +-
> >>   3 files changed, 3 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/arch/arm/cpu/armv7/sunxi/clock_sun6i.c b/arch/arm/cpu/armv7/sunxi/clock_sun6i.c
> >> index 193e314..8ef19df 100644
> >> --- a/arch/arm/cpu/armv7/sunxi/clock_sun6i.c
> >> +++ b/arch/arm/cpu/armv7/sunxi/clock_sun6i.c
> >> @@ -144,12 +144,10 @@ void clock_set_pll3(unsigned int clk)
> >>   	       &ccm->pll3_cfg);
> >>   }
> >>
> >> -void clock_set_pll5(unsigned int clk, bool sigma_delta_enable)
> >> +void clock_set_pll5(unsigned int clk, int k, int m, bool sigma_delta_enable)
> >>   {
> >>   	struct sunxi_ccm_reg * const ccm =
> >>   		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
> >> -	const int k = 2;
> >> -	const int m = 1;
> >>
> >>   	if (sigma_delta_enable)
> >>   		writel(CCM_PLL5_PATTERN, &ccm->pll5_pattern_cfg);
> >> diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
> >> index bc6428a..a8bbdfd 100644
> >> --- a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
> >> +++ b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
> >> @@ -46,7 +46,7 @@ static void mctl_sys_init(void)
> >>   		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
> >>   	const int dram_clk_div = 2;
> >>
> >> -	clock_set_pll5(DRAM_CLK * dram_clk_div, false);
> >> +	clock_set_pll5(DRAM_CLK * dram_clk_div, 2, 1, false);
> >>
> >>   	clrsetbits_le32(&ccm->dram_clk_cfg, CCM_DRAMCLK_CFG_DIV0_MASK,
> >>   		CCM_DRAMCLK_CFG_DIV0(dram_clk_div) | CCM_DRAMCLK_CFG_RST |
> >> diff --git a/arch/arm/include/asm/arch-sunxi/clock_sun6i.h b/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
> >> index f807af3..7d61216 100644
> >> --- a/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
> >> +++ b/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
> >> @@ -311,6 +311,6 @@ struct sunxi_ccm_reg {
> >>   #define CCM_DE_CTRL_PLL10		(5 << 24)
> >>   #define CCM_DE_CTRL_GATE		(1 << 31)
> >>
> >> -void clock_set_pll5(unsigned int clk, bool sigma_delta_enable);
> >> +void clock_set_pll5(unsigned int clk, int k, int m, bool sigma_delta_enable);
> >>
> >>   #endif /* _SUNXI_CLOCK_SUN6I_H */
> >
> >
> >