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

2014-12-21 Thread Siarhei Siamashka 
On Fri, 19 Dec 2014 17:40:27 +0100
Hans de Goede hdego...@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 hdego...@redhat.com wrote:
 
  The A23 (sun8i) requires different values for these then sun6i, so make 
  them
  function parameters.
 
  Signed-off-by: Hans de Goede hdego...@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  2400 * k * max_n / m) {
m = 1;
if (clk  2400 * 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 */
 
 
 
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2014-12-19 Thread Siarhei Siamashka 
On Tue, 16 Dec 2014 21:31:34 +0100
Hans de Goede hdego...@redhat.com wrote:

 The A23 (sun8i) requires different values for these then sun6i, so make them
 function parameters.

 Signed-off-by: Hans de Goede hdego...@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)?

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 */



-- 
Best regards,
Siarhei Siamashka
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2014-12-19 Thread Hans de Goede 

Hi,

On 19-12-14 11:03, Siarhei Siamashka wrote:

On Tue, 16 Dec 2014 21:31:34 +0100
Hans de Goede hdego...@redhat.com wrote:


The A23 (sun8i) requires different values for these then sun6i, so make them
function parameters.

Signed-off-by: Hans de Goede hdego...@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. AFAIK using other k and m factors with the same end-result should
work fine. But doing things this way follows the KISS principle and I'm
a great fan of KISS.

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 */





___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2014-12-18 Thread Ian Campbell 
On Tue, 2014-12-16 at 21:31 +0100, Hans de Goede wrote:
 The A23 (sun8i) requires different values for these then sun6i, so make them
 function parameters.
 
 Signed-off-by: Hans de Goede hdego...@redhat.com

Acked-by: Ian Campbell i...@hellion.org.uk


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2014-12-16 Thread Hans de Goede 
The A23 (sun8i) requires different values for these then sun6i, so make them
function parameters.

Signed-off-by: Hans de Goede hdego...@redhat.com
---
 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 */
-- 
2.1.0

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot