Re: [PATCH v6 20/41] ARM: da830: add new clock init using common clock framework

2018-02-05 Thread Sekhar Nori 
On Friday 02 February 2018 11:33 PM, David Lechner wrote:
> On 02/02/2018 08:12 AM, Sekhar Nori wrote:
>> On Saturday 20 January 2018 10:43 PM, David Lechner wrote:
>>>   void __init da830_init_time(void)
>>>   {
>>> +#ifdef CONFIG_COMMON_CLK
>>> +    void __iomem *pll0, *psc0, *psc1;
>>> +    struct clk *clk;
>>> +
>>> +    pll0 = ioremap(DA8XX_PLL0_BASE, SZ_4K);
>>> +    psc0 = ioremap(DA8XX_PSC0_BASE, SZ_4K);
>>> +    psc1 = ioremap(DA8XX_PSC1_BASE, SZ_4K);
>>> +
>>> +    da8xx_register_cfgchip();
>>> +
>>> +    clk_register_fixed_rate(NULL, "ref_clk", NULL, 0, DA830_REF_FREQ);
>>> +
>>> +    da830_pll_clk_init(pll0);
>>> +
>>> +    da830_psc_clk_init(psc0, psc1);
>>> +
>>
>>> +    clk = clk_register_fixed_factor(NULL, "i2c0", "pll0_aux_clk", 0,
>>> 1, 1);
>>> +    clk_register_clkdev(clk, NULL, "i2c_davinci.1");
>>> +
>>> +    clk = clk_register_fixed_factor(NULL, "timer0", "pll0_aux_clk",
>>> 0, 1, 1);
>>> +    clk_register_clkdev(clk, "timer0", NULL);
>>> +
>>> +    clk = clk_register_fixed_factor(NULL, "timer1", "pll0_aux_clk",
>>> 0, 1, 1);
>>> +    clk_register_clkdev(clk, NULL, "davinci-wdt");
>>
>> Isn't this better done in da830_pll_clk_init() ? I think we can get rid
>> of the dummy fixed factor clock too and directly use the pll0_auxclk.
> 
> 
> I considered it, but I kind of like keeping the fixed factor clocks for
> debugging purposes. If you just have "pll0_auxclk" the enable count is
> not helpful because you don't know which driver did the enabling.

I think it is better to more or less reflect the hardware here. We would
not be doing this in the DT case, for example.

I see your point on debugging. Such code can perhaps be temporarily
introduced if really debugging such an issue. This will be the case with
any shared clock.

Thanks,
Sekhar

Re: [PATCH v6 20/41] ARM: da830: add new clock init using common clock framework

2018-02-05 Thread Sekhar Nori 
On Friday 02 February 2018 11:33 PM, David Lechner wrote:
> On 02/02/2018 08:12 AM, Sekhar Nori wrote:
>> On Saturday 20 January 2018 10:43 PM, David Lechner wrote:
>>>   void __init da830_init_time(void)
>>>   {
>>> +#ifdef CONFIG_COMMON_CLK
>>> +    void __iomem *pll0, *psc0, *psc1;
>>> +    struct clk *clk;
>>> +
>>> +    pll0 = ioremap(DA8XX_PLL0_BASE, SZ_4K);
>>> +    psc0 = ioremap(DA8XX_PSC0_BASE, SZ_4K);
>>> +    psc1 = ioremap(DA8XX_PSC1_BASE, SZ_4K);
>>> +
>>> +    da8xx_register_cfgchip();
>>> +
>>> +    clk_register_fixed_rate(NULL, "ref_clk", NULL, 0, DA830_REF_FREQ);
>>> +
>>> +    da830_pll_clk_init(pll0);
>>> +
>>> +    da830_psc_clk_init(psc0, psc1);
>>> +
>>
>>> +    clk = clk_register_fixed_factor(NULL, "i2c0", "pll0_aux_clk", 0,
>>> 1, 1);
>>> +    clk_register_clkdev(clk, NULL, "i2c_davinci.1");
>>> +
>>> +    clk = clk_register_fixed_factor(NULL, "timer0", "pll0_aux_clk",
>>> 0, 1, 1);
>>> +    clk_register_clkdev(clk, "timer0", NULL);
>>> +
>>> +    clk = clk_register_fixed_factor(NULL, "timer1", "pll0_aux_clk",
>>> 0, 1, 1);
>>> +    clk_register_clkdev(clk, NULL, "davinci-wdt");
>>
>> Isn't this better done in da830_pll_clk_init() ? I think we can get rid
>> of the dummy fixed factor clock too and directly use the pll0_auxclk.
> 
> 
> I considered it, but I kind of like keeping the fixed factor clocks for
> debugging purposes. If you just have "pll0_auxclk" the enable count is
> not helpful because you don't know which driver did the enabling.

I think it is better to more or less reflect the hardware here. We would
not be doing this in the DT case, for example.

I see your point on debugging. Such code can perhaps be temporarily
introduced if really debugging such an issue. This will be the case with
any shared clock.

Thanks,
Sekhar

Re: [PATCH v6 20/41] ARM: da830: add new clock init using common clock framework

2018-02-02 Thread David Lechner 

On 02/02/2018 08:12 AM, Sekhar Nori wrote:

On Saturday 20 January 2018 10:43 PM, David Lechner wrote:

  void __init da830_init_time(void)
  {
+#ifdef CONFIG_COMMON_CLK
+   void __iomem *pll0, *psc0, *psc1;
+   struct clk *clk;
+
+   pll0 = ioremap(DA8XX_PLL0_BASE, SZ_4K);
+   psc0 = ioremap(DA8XX_PSC0_BASE, SZ_4K);
+   psc1 = ioremap(DA8XX_PSC1_BASE, SZ_4K);
+
+   da8xx_register_cfgchip();
+
+   clk_register_fixed_rate(NULL, "ref_clk", NULL, 0, DA830_REF_FREQ);
+
+   da830_pll_clk_init(pll0);
+
+   da830_psc_clk_init(psc0, psc1);
+



+   clk = clk_register_fixed_factor(NULL, "i2c0", "pll0_aux_clk", 0, 1, 1);
+   clk_register_clkdev(clk, NULL, "i2c_davinci.1");
+
+   clk = clk_register_fixed_factor(NULL, "timer0", "pll0_aux_clk", 0, 1, 
1);
+   clk_register_clkdev(clk, "timer0", NULL);
+
+   clk = clk_register_fixed_factor(NULL, "timer1", "pll0_aux_clk", 0, 1, 
1);
+   clk_register_clkdev(clk, NULL, "davinci-wdt");


Isn't this better done in da830_pll_clk_init() ? I think we can get rid
of the dummy fixed factor clock too and directly use the pll0_auxclk.



I considered it, but I kind of like keeping the fixed factor clocks for
debugging purposes. If you just have "pll0_auxclk" the enable count is
not helpful because you don't know which driver did the enabling.

On the other hand, once things are working, you don't really need to
do any debugging.



That reminds me, is "pll0_aux_clk" above correct, or should it be
"pll0_auxclk" like in da830_pll_clk_init()?


Yes, it should be "pll0_auxclk".




+
+   clk = clk_register_fixed_factor(NULL, "rmii", "pll0_sysclk7", 0, 1, 1);
+   clk_register_clkdev(clk, "rmii", NULL);


I don't see any driver looking for this clock using con_id "rmii". I
know this came from existing code. But its most likely a vestige and can
be dropped.

Thanks,
Sekhar

Re: [PATCH v6 20/41] ARM: da830: add new clock init using common clock framework

2018-02-02 Thread David Lechner 

On 02/02/2018 08:12 AM, Sekhar Nori wrote:

On Saturday 20 January 2018 10:43 PM, David Lechner wrote:

  void __init da830_init_time(void)
  {
+#ifdef CONFIG_COMMON_CLK
+   void __iomem *pll0, *psc0, *psc1;
+   struct clk *clk;
+
+   pll0 = ioremap(DA8XX_PLL0_BASE, SZ_4K);
+   psc0 = ioremap(DA8XX_PSC0_BASE, SZ_4K);
+   psc1 = ioremap(DA8XX_PSC1_BASE, SZ_4K);
+
+   da8xx_register_cfgchip();
+
+   clk_register_fixed_rate(NULL, "ref_clk", NULL, 0, DA830_REF_FREQ);
+
+   da830_pll_clk_init(pll0);
+
+   da830_psc_clk_init(psc0, psc1);
+



+   clk = clk_register_fixed_factor(NULL, "i2c0", "pll0_aux_clk", 0, 1, 1);
+   clk_register_clkdev(clk, NULL, "i2c_davinci.1");
+
+   clk = clk_register_fixed_factor(NULL, "timer0", "pll0_aux_clk", 0, 1, 
1);
+   clk_register_clkdev(clk, "timer0", NULL);
+
+   clk = clk_register_fixed_factor(NULL, "timer1", "pll0_aux_clk", 0, 1, 
1);
+   clk_register_clkdev(clk, NULL, "davinci-wdt");


Isn't this better done in da830_pll_clk_init() ? I think we can get rid
of the dummy fixed factor clock too and directly use the pll0_auxclk.



I considered it, but I kind of like keeping the fixed factor clocks for
debugging purposes. If you just have "pll0_auxclk" the enable count is
not helpful because you don't know which driver did the enabling.

On the other hand, once things are working, you don't really need to
do any debugging.



That reminds me, is "pll0_aux_clk" above correct, or should it be
"pll0_auxclk" like in da830_pll_clk_init()?


Yes, it should be "pll0_auxclk".




+
+   clk = clk_register_fixed_factor(NULL, "rmii", "pll0_sysclk7", 0, 1, 1);
+   clk_register_clkdev(clk, "rmii", NULL);


I don't see any driver looking for this clock using con_id "rmii". I
know this came from existing code. But its most likely a vestige and can
be dropped.

Thanks,
Sekhar

Re: [PATCH v6 20/41] ARM: da830: add new clock init using common clock framework

2018-02-02 Thread Sekhar Nori 
On Saturday 20 January 2018 10:43 PM, David Lechner wrote:
>  void __init da830_init_time(void)
>  {
> +#ifdef CONFIG_COMMON_CLK
> + void __iomem *pll0, *psc0, *psc1;
> + struct clk *clk;
> +
> + pll0 = ioremap(DA8XX_PLL0_BASE, SZ_4K);
> + psc0 = ioremap(DA8XX_PSC0_BASE, SZ_4K);
> + psc1 = ioremap(DA8XX_PSC1_BASE, SZ_4K);
> +
> + da8xx_register_cfgchip();
> +
> + clk_register_fixed_rate(NULL, "ref_clk", NULL, 0, DA830_REF_FREQ);
> +
> + da830_pll_clk_init(pll0);
> +
> + da830_psc_clk_init(psc0, psc1);
> +

> + clk = clk_register_fixed_factor(NULL, "i2c0", "pll0_aux_clk", 0, 1, 1);
> + clk_register_clkdev(clk, NULL, "i2c_davinci.1");
> +
> + clk = clk_register_fixed_factor(NULL, "timer0", "pll0_aux_clk", 0, 1, 
> 1);
> + clk_register_clkdev(clk, "timer0", NULL);
> +
> + clk = clk_register_fixed_factor(NULL, "timer1", "pll0_aux_clk", 0, 1, 
> 1);
> + clk_register_clkdev(clk, NULL, "davinci-wdt");

Isn't this better done in da830_pll_clk_init() ? I think we can get rid
of the dummy fixed factor clock too and directly use the pll0_auxclk.
That reminds me, is "pll0_aux_clk" above correct, or should it be
"pll0_auxclk" like in da830_pll_clk_init()?

> +
> + clk = clk_register_fixed_factor(NULL, "rmii", "pll0_sysclk7", 0, 1, 1);
> + clk_register_clkdev(clk, "rmii", NULL);

I don't see any driver looking for this clock using con_id "rmii". I
know this came from existing code. But its most likely a vestige and can
be dropped.

Thanks,
Sekhar

Re: [PATCH v6 20/41] ARM: da830: add new clock init using common clock framework

2018-02-02 Thread Sekhar Nori 
On Saturday 20 January 2018 10:43 PM, David Lechner wrote:
>  void __init da830_init_time(void)
>  {
> +#ifdef CONFIG_COMMON_CLK
> + void __iomem *pll0, *psc0, *psc1;
> + struct clk *clk;
> +
> + pll0 = ioremap(DA8XX_PLL0_BASE, SZ_4K);
> + psc0 = ioremap(DA8XX_PSC0_BASE, SZ_4K);
> + psc1 = ioremap(DA8XX_PSC1_BASE, SZ_4K);
> +
> + da8xx_register_cfgchip();
> +
> + clk_register_fixed_rate(NULL, "ref_clk", NULL, 0, DA830_REF_FREQ);
> +
> + da830_pll_clk_init(pll0);
> +
> + da830_psc_clk_init(psc0, psc1);
> +

> + clk = clk_register_fixed_factor(NULL, "i2c0", "pll0_aux_clk", 0, 1, 1);
> + clk_register_clkdev(clk, NULL, "i2c_davinci.1");
> +
> + clk = clk_register_fixed_factor(NULL, "timer0", "pll0_aux_clk", 0, 1, 
> 1);
> + clk_register_clkdev(clk, "timer0", NULL);
> +
> + clk = clk_register_fixed_factor(NULL, "timer1", "pll0_aux_clk", 0, 1, 
> 1);
> + clk_register_clkdev(clk, NULL, "davinci-wdt");

Isn't this better done in da830_pll_clk_init() ? I think we can get rid
of the dummy fixed factor clock too and directly use the pll0_auxclk.
That reminds me, is "pll0_aux_clk" above correct, or should it be
"pll0_auxclk" like in da830_pll_clk_init()?

> +
> + clk = clk_register_fixed_factor(NULL, "rmii", "pll0_sysclk7", 0, 1, 1);
> + clk_register_clkdev(clk, "rmii", NULL);

I don't see any driver looking for this clock using con_id "rmii". I
know this came from existing code. But its most likely a vestige and can
be dropped.

Thanks,
Sekhar

Re: [PATCH v6 20/41] ARM: da830: add new clock init using common clock framework

2018-01-22 Thread David Lechner 

On 01/20/2018 11:13 AM, David Lechner wrote:

This adds the new board-specific clock init in mach-davinci/da830.c
using the new common clock framework drivers.

The #ifdefs are needed to prevent compile errors until the entire
ARCH_DAVINCI is converted.

Also clean up the #includes since we are adding some here.

Signed-off-by: David Lechner 
---

v6 changes:
- add blank lines between function calls
- include da8xx_register_cfgchip()

  arch/arm/mach-davinci/da830.c | 49 +--
  1 file changed, 43 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-davinci/da830.c b/arch/arm/mach-davinci/da830.c


...


+   clk = clk_register_fixed_factor(NULL, "i2c0", "pll0_aux_clk", 0, 1, 1);


Should be "pll0_auxclk" instead of "pll0_aux_clk" here and 2 more below.


+   clk_register_clkdev(clk, NULL, "i2c_davinci.1");
+
+   clk = clk_register_fixed_factor(NULL, "timer0", "pll0_aux_clk", 0, 1, 
1);
+   clk_register_clkdev(clk, "timer0", NULL);
+
+   clk = clk_register_fixed_factor(NULL, "timer1", "pll0_aux_clk", 0, 1, 
1);
+   clk_register_clkdev(clk, NULL, "davinci-wdt");
+
+   clk = clk_register_fixed_factor(NULL, "rmii", "pll0_sysclk7", 0, 1, 1);
+   clk_register_clkdev(clk, "rmii", NULL);
+#else
da8xx_register_cfgchip();
davinci_clk_init(da830_clks);
+#endif
davinci_timer_init();
  }

Re: [PATCH v6 20/41] ARM: da830: add new clock init using common clock framework

2018-01-22 Thread David Lechner 

On 01/20/2018 11:13 AM, David Lechner wrote:

This adds the new board-specific clock init in mach-davinci/da830.c
using the new common clock framework drivers.

The #ifdefs are needed to prevent compile errors until the entire
ARCH_DAVINCI is converted.

Also clean up the #includes since we are adding some here.

Signed-off-by: David Lechner 
---

v6 changes:
- add blank lines between function calls
- include da8xx_register_cfgchip()

  arch/arm/mach-davinci/da830.c | 49 +--
  1 file changed, 43 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-davinci/da830.c b/arch/arm/mach-davinci/da830.c


...


+   clk = clk_register_fixed_factor(NULL, "i2c0", "pll0_aux_clk", 0, 1, 1);


Should be "pll0_auxclk" instead of "pll0_aux_clk" here and 2 more below.


+   clk_register_clkdev(clk, NULL, "i2c_davinci.1");
+
+   clk = clk_register_fixed_factor(NULL, "timer0", "pll0_aux_clk", 0, 1, 
1);
+   clk_register_clkdev(clk, "timer0", NULL);
+
+   clk = clk_register_fixed_factor(NULL, "timer1", "pll0_aux_clk", 0, 1, 
1);
+   clk_register_clkdev(clk, NULL, "davinci-wdt");
+
+   clk = clk_register_fixed_factor(NULL, "rmii", "pll0_sysclk7", 0, 1, 1);
+   clk_register_clkdev(clk, "rmii", NULL);
+#else
da8xx_register_cfgchip();
davinci_clk_init(da830_clks);
+#endif
davinci_timer_init();
  }

[PATCH v6 20/41] ARM: da830: add new clock init using common clock framework

2018-01-20 Thread David Lechner 
This adds the new board-specific clock init in mach-davinci/da830.c
using the new common clock framework drivers.

The #ifdefs are needed to prevent compile errors until the entire
ARCH_DAVINCI is converted.

Also clean up the #includes since we are adding some here.

Signed-off-by: David Lechner 
---

v6 changes:
- add blank lines between function calls
- include da8xx_register_cfgchip()

 arch/arm/mach-davinci/da830.c | 49 +--
 1 file changed, 43 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-davinci/da830.c b/arch/arm/mach-davinci/da830.c
index 39de5a6..0b6d0f3 100644
--- a/arch/arm/mach-davinci/da830.c
+++ b/arch/arm/mach-davinci/da830.c
@@ -8,23 +8,29 @@
  * is licensed "as is" without any warranty of any kind, whether express
  * or implied.
  */
+#include 
+#include 
+#include 
+#include 
 #include 
 #include 
-#include 
 #include 
 
 #include 
 
-#include "psc.h"
-#include 
-#include 
 #include 
-#include 
+#include 
 #include 
+#include 
+#include 
 
-#include "clock.h"
 #include "mux.h"
 
+#ifndef CONFIG_COMMON_CLK
+#include "clock.h"
+#include "psc.h"
+#endif
+
 /* Offsets of the 8 compare registers on the da830 */
 #define DA830_CMP12_0  0x60
 #define DA830_CMP12_1  0x64
@@ -37,6 +43,7 @@
 
 #define DA830_REF_FREQ 2400
 
+#ifndef CONFIG_COMMON_CLK
 static struct pll_data pll0_data = {
.num= 1,
.phys_base  = DA8XX_PLL0_BASE,
@@ -432,6 +439,7 @@ static struct clk_lookup da830_clks[] = {
CLK(NULL,   "rmii", _clk),
CLK(NULL,   NULL,   NULL),
 };
+#endif
 
 /*
  * Device specific mux setup
@@ -1223,7 +1231,36 @@ void __init da830_init(void)
 
 void __init da830_init_time(void)
 {
+#ifdef CONFIG_COMMON_CLK
+   void __iomem *pll0, *psc0, *psc1;
+   struct clk *clk;
+
+   pll0 = ioremap(DA8XX_PLL0_BASE, SZ_4K);
+   psc0 = ioremap(DA8XX_PSC0_BASE, SZ_4K);
+   psc1 = ioremap(DA8XX_PSC1_BASE, SZ_4K);
+
+   da8xx_register_cfgchip();
+
+   clk_register_fixed_rate(NULL, "ref_clk", NULL, 0, DA830_REF_FREQ);
+
+   da830_pll_clk_init(pll0);
+
+   da830_psc_clk_init(psc0, psc1);
+
+   clk = clk_register_fixed_factor(NULL, "i2c0", "pll0_aux_clk", 0, 1, 1);
+   clk_register_clkdev(clk, NULL, "i2c_davinci.1");
+
+   clk = clk_register_fixed_factor(NULL, "timer0", "pll0_aux_clk", 0, 1, 
1);
+   clk_register_clkdev(clk, "timer0", NULL);
+
+   clk = clk_register_fixed_factor(NULL, "timer1", "pll0_aux_clk", 0, 1, 
1);
+   clk_register_clkdev(clk, NULL, "davinci-wdt");
+
+   clk = clk_register_fixed_factor(NULL, "rmii", "pll0_sysclk7", 0, 1, 1);
+   clk_register_clkdev(clk, "rmii", NULL);
+#else
da8xx_register_cfgchip();
davinci_clk_init(da830_clks);
+#endif
davinci_timer_init();
 }
-- 
2.7.4

[PATCH v6 20/41] ARM: da830: add new clock init using common clock framework

2018-01-20 Thread David Lechner 
This adds the new board-specific clock init in mach-davinci/da830.c
using the new common clock framework drivers.

The #ifdefs are needed to prevent compile errors until the entire
ARCH_DAVINCI is converted.

Also clean up the #includes since we are adding some here.

Signed-off-by: David Lechner 
---

v6 changes:
- add blank lines between function calls
- include da8xx_register_cfgchip()

 arch/arm/mach-davinci/da830.c | 49 +--
 1 file changed, 43 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-davinci/da830.c b/arch/arm/mach-davinci/da830.c
index 39de5a6..0b6d0f3 100644
--- a/arch/arm/mach-davinci/da830.c
+++ b/arch/arm/mach-davinci/da830.c
@@ -8,23 +8,29 @@
  * is licensed "as is" without any warranty of any kind, whether express
  * or implied.
  */
+#include 
+#include 
+#include 
+#include 
 #include 
 #include 
-#include 
 #include 
 
 #include 
 
-#include "psc.h"
-#include 
-#include 
 #include 
-#include 
+#include 
 #include 
+#include 
+#include 
 
-#include "clock.h"
 #include "mux.h"
 
+#ifndef CONFIG_COMMON_CLK
+#include "clock.h"
+#include "psc.h"
+#endif
+
 /* Offsets of the 8 compare registers on the da830 */
 #define DA830_CMP12_0  0x60
 #define DA830_CMP12_1  0x64
@@ -37,6 +43,7 @@
 
 #define DA830_REF_FREQ 2400
 
+#ifndef CONFIG_COMMON_CLK
 static struct pll_data pll0_data = {
.num= 1,
.phys_base  = DA8XX_PLL0_BASE,
@@ -432,6 +439,7 @@ static struct clk_lookup da830_clks[] = {
CLK(NULL,   "rmii", _clk),
CLK(NULL,   NULL,   NULL),
 };
+#endif
 
 /*
  * Device specific mux setup
@@ -1223,7 +1231,36 @@ void __init da830_init(void)
 
 void __init da830_init_time(void)
 {
+#ifdef CONFIG_COMMON_CLK
+   void __iomem *pll0, *psc0, *psc1;
+   struct clk *clk;
+
+   pll0 = ioremap(DA8XX_PLL0_BASE, SZ_4K);
+   psc0 = ioremap(DA8XX_PSC0_BASE, SZ_4K);
+   psc1 = ioremap(DA8XX_PSC1_BASE, SZ_4K);
+
+   da8xx_register_cfgchip();
+
+   clk_register_fixed_rate(NULL, "ref_clk", NULL, 0, DA830_REF_FREQ);
+
+   da830_pll_clk_init(pll0);
+
+   da830_psc_clk_init(psc0, psc1);
+
+   clk = clk_register_fixed_factor(NULL, "i2c0", "pll0_aux_clk", 0, 1, 1);
+   clk_register_clkdev(clk, NULL, "i2c_davinci.1");
+
+   clk = clk_register_fixed_factor(NULL, "timer0", "pll0_aux_clk", 0, 1, 
1);
+   clk_register_clkdev(clk, "timer0", NULL);
+
+   clk = clk_register_fixed_factor(NULL, "timer1", "pll0_aux_clk", 0, 1, 
1);
+   clk_register_clkdev(clk, NULL, "davinci-wdt");
+
+   clk = clk_register_fixed_factor(NULL, "rmii", "pll0_sysclk7", 0, 1, 1);
+   clk_register_clkdev(clk, "rmii", NULL);
+#else
da8xx_register_cfgchip();
davinci_clk_init(da830_clks);
+#endif
davinci_timer_init();
 }
-- 
2.7.4