On Fri, 19 Dec 2014 17:40:27 +0100 Hans de Goede hdegoede@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@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@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 */