[PATCH v3 06/15] ARM: shmobile: koelsch-reference: Initialize CPG device

[PATCH v3 06/15] ARM: shmobile: koelsch-reference: Initialize CPG device

[Laurent Pinchart]

On multiplatform kernels clocks are handled by the CCF CPG driver. It
must be explicitly initialized by a call to rcar_gen2_clocks_init() with
the value of the boot mode pins.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 arch/arm/mach-shmobile/board-koelsch-reference.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-shmobile/board-koelsch-reference.c b/arch/arm/mach-shmobile/board-koelsch-reference.c
index a804a17..788ccdf 100644
--- a/arch/arm/mach-shmobile/board-koelsch-reference.c
+++ b/arch/arm/mach-shmobile/board-koelsch-reference.c
@@ -19,7 +19,7 @@
  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  */
 
-#include <linux/clk-provider.h>
+#include <linux/clk/shmobile.h>
 #include <linux/kernel.h>
 #include <linux/of_platform.h>
 #include <mach/common.h>
@@ -27,11 +27,17 @@
 #include <mach/r8a7791.h>
 #include <asm/mach/arch.h>
 
-static void __init koelsch_add_standard_devices(void)
+static void __init koelsch_init_time(void)
 {
 #ifdef CONFIG_COMMON_CLK
-	of_clk_init(NULL);
-#else
+	rcar_gen2_clocks_init(rcar_gen2_read_mode_pins());
+#endif
+	rcar_gen2_timer_init();
+}
+
+static void __init koelsch_add_standard_devices(void)
+{
+#ifndef CONFIG_COMMON_CLK
 	r8a7791_clock_init();
 #endif
 	r8a7791_add_dt_devices();
@@ -46,7 +52,7 @@ static const char * const koelsch_boards_compat_dt[] __initconst = {
 DT_MACHINE_START(KOELSCH_DT, "koelsch")
 	.smp		= smp_ops(r8a7791_smp_ops),
 	.init_early	= r8a7791_init_early,
-	.init_time	= rcar_gen2_timer_init,
+	.init_time	= koelsch_init_time,
 	.init_machine	= koelsch_add_standard_devices,
 	.init_late	= shmobile_init_late,
 	.dt_compat	= koelsch_boards_compat_dt,
-- 
1.8.3.2

Re: [PATCH v3 06/15] ARM: shmobile: koelsch-reference: Initialize CPG device

[Magnus Damm]

On Thu, Nov 28, 2013 at 10:03 AM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
> On multiplatform kernels clocks are handled by the CCF CPG driver. It
> must be explicitly initialized by a call to rcar_gen2_clocks_init() with
> the value of the boot mode pins.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Hi Laurent,

Thanks for your efforts. I think this series looks very nice in
general. Please see below for two questions.

> --- a/arch/arm/mach-shmobile/board-koelsch-reference.c
> +++ b/arch/arm/mach-shmobile/board-koelsch-reference.c
> @@ -46,7 +52,7 @@ static const char * const koelsch_boards_compat_dt[] __initconst = {
>  DT_MACHINE_START(KOELSCH_DT, "koelsch")
>         .smp            = smp_ops(r8a7791_smp_ops),
>         .init_early     = r8a7791_init_early,
> -       .init_time      = rcar_gen2_timer_init,
> +       .init_time      = koelsch_init_time,
>         .init_machine   = koelsch_add_standard_devices,
>         .init_late      = shmobile_init_late,
>         .dt_compat      = koelsch_boards_compat_dt,

What is the reason that you need to initialize CCF at ->init_time()?
We used to initialize timers early in the legacy board case but for DT
we've so far been able to initialize timers late. Is it really time to
go back again? =)

As for initializing the CCF, if we go down the route of early CCF
init, can't you put your CCF init call at one shared location in for
instance rcar_gen2_timer_init()?

Thanks,

/ magnus

Re: [PATCH v3 06/15] ARM: shmobile: koelsch-reference: Initialize CPG device

[Laurent Pinchart]

Hi Magnus,

On Thursday 28 November 2013 14:32:18 Magnus Damm wrote:
> On Thu, Nov 28, 2013 at 10:03 AM, Laurent Pinchart wrote:
> > On multiplatform kernels clocks are handled by the CCF CPG driver. It
> > must be explicitly initialized by a call to rcar_gen2_clocks_init() with
> > the value of the boot mode pins.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> 
> Hi Laurent,
> 
> Thanks for your efforts. I think this series looks very nice in
> general. Please see below for two questions.
> 
> > --- a/arch/arm/mach-shmobile/board-koelsch-reference.c
> > +++ b/arch/arm/mach-shmobile/board-koelsch-reference.c
> > @@ -46,7 +52,7 @@ static const char * const koelsch_boards_compat_dt[]
> > __initconst = {> 
> >  DT_MACHINE_START(KOELSCH_DT, "koelsch")
> >  
> >         .smp            = smp_ops(r8a7791_smp_ops),
> >         .init_early     = r8a7791_init_early,
> > 
> > -       .init_time      = rcar_gen2_timer_init,
> > +       .init_time      = koelsch_init_time,
> > 
> >         .init_machine   = koelsch_add_standard_devices,
> >         .init_late      = shmobile_init_late,
> >         .dt_compat      = koelsch_boards_compat_dt,
> 
> What is the reason that you need to initialize CCF at ->init_time()?
> We used to initialize timers early in the legacy board case but for DT
> we've so far been able to initialize timers late. Is it really time to
> go back again? =)

rcar_gen2_timer_init() calls clocksource_of_init(), and our clocksource 
devices need clocks, so CCF needs to be initialized before that.

> As for initializing the CCF, if we go down the route of early CCF
> init, can't you put your CCF init call at one shared location in for
> instance rcar_gen2_timer_init()?

That's a good idea, I will do that.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 06/15] ARM: shmobile: koelsch-reference: Initialize CPG device

[Magnus Damm]

Hi Laurent,

On Fri, Nov 29, 2013 at 2:17 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> On Thursday 28 November 2013 14:32:18 Magnus Damm wrote:
>> On Thu, Nov 28, 2013 at 10:03 AM, Laurent Pinchart wrote:
>> > On multiplatform kernels clocks are handled by the CCF CPG driver. It
>> > must be explicitly initialized by a call to rcar_gen2_clocks_init() with
>> > the value of the boot mode pins.
>> >
>> > Signed-off-by: Laurent Pinchart
>> > <laurent.pinchart+renesas@ideasonboard.com>
>>
>> Hi Laurent,
>>
>> Thanks for your efforts. I think this series looks very nice in
>> general. Please see below for two questions.
>>
>> > --- a/arch/arm/mach-shmobile/board-koelsch-reference.c
>> > +++ b/arch/arm/mach-shmobile/board-koelsch-reference.c
>> > @@ -46,7 +52,7 @@ static const char * const koelsch_boards_compat_dt[]
>> > __initconst = {>
>> >  DT_MACHINE_START(KOELSCH_DT, "koelsch")
>> >
>> >         .smp            = smp_ops(r8a7791_smp_ops),
>> >         .init_early     = r8a7791_init_early,
>> >
>> > -       .init_time      = rcar_gen2_timer_init,
>> > +       .init_time      = koelsch_init_time,
>> >
>> >         .init_machine   = koelsch_add_standard_devices,
>> >         .init_late      = shmobile_init_late,
>> >         .dt_compat      = koelsch_boards_compat_dt,
>>
>> What is the reason that you need to initialize CCF at ->init_time()?
>> We used to initialize timers early in the legacy board case but for DT
>> we've so far been able to initialize timers late. Is it really time to
>> go back again? =)
>
> rcar_gen2_timer_init() calls clocksource_of_init(), and our clocksource
> devices need clocks, so CCF needs to be initialized before that.

Well, I can see where you are coming from, and your assumption sounds
sane. But I don't think your logic is a matching reality! =)

Our timer drivers like CMT, TMU, MTU2 and STI are all using the good
old regular platform device driver model for probe(). They do not rely
on the clocksource_of_init() probe mechanism. The reason for this is
that they were written before clocksource_of_init() was introduced.
Actually, I'm sure there is a reason behind it, but I don't see the
point of this duplicated OF-specific interface at all, but that's a
different story.

The ARM drivers like TWD and arch timer do however rely on
clocksource_of_init(), but they can all be used without the clock
framework unless I'm mistaken, so you don't have any dependency there.

It is my opinion that we should use the regular driver model for our
timers. If there is some issue with that then we should for instance
be able to use deferred probing to remedy that.

Some of our legacy board code is using early platform devices for
early timers. But all DT reference board code with legacy clocks is
setting up timers late. Because of this I believe it is very possible
to init the timers late with CCF as well.

So from my point of view we should avoid enabling CCF early, it just
makes things complicated with no apparent upside.

>> As for initializing the CCF, if we go down the route of early CCF
>> init, can't you put your CCF init call at one shared location in for
>> instance rcar_gen2_timer_init()?
>
> That's a good idea, I will do that.

Thanks. Since I believe it is possible to use late CCF init then I
much prefer using that. But feel free to prove me wrong!

Cheers,

/ magnus

Re: [PATCH v3 06/15] ARM: shmobile: koelsch-reference: Initialize CPG device

[Laurent Pinchart]

Hi Magnus,

On Friday 29 November 2013 10:43:31 Magnus Damm wrote:
> On Fri, Nov 29, 2013 at 2:17 AM, Laurent Pinchart wrote:
> > On Thursday 28 November 2013 14:32:18 Magnus Damm wrote:
> >> On Thu, Nov 28, 2013 at 10:03 AM, Laurent Pinchart wrote:
> >> > On multiplatform kernels clocks are handled by the CCF CPG driver. It
> >> > must be explicitly initialized by a call to rcar_gen2_clocks_init()
> >> > with the value of the boot mode pins.
> >> > 
> >> > Signed-off-by: Laurent Pinchart
> >> > <laurent.pinchart+renesas@ideasonboard.com>
> >> 
> >> Hi Laurent,
> >> 
> >> Thanks for your efforts. I think this series looks very nice in
> >> general. Please see below for two questions.
> >> 
> >> > --- a/arch/arm/mach-shmobile/board-koelsch-reference.c
> >> > +++ b/arch/arm/mach-shmobile/board-koelsch-reference.c
> >> > @@ -46,7 +52,7 @@ static const char * const koelsch_boards_compat_dt[]
> >> > __initconst = {
> >> > 
> >> >  DT_MACHINE_START(KOELSCH_DT, "koelsch")
> >> >         .smp            = smp_ops(r8a7791_smp_ops),
> >> >         .init_early     = r8a7791_init_early,
> >> > -       .init_time      = rcar_gen2_timer_init,
> >> > +       .init_time      = koelsch_init_time,
> >> >         .init_machine   = koelsch_add_standard_devices,
> >> >         .init_late      = shmobile_init_late,
> >> >         .dt_compat      = koelsch_boards_compat_dt,
> >> 
> >> What is the reason that you need to initialize CCF at ->init_time()?
> >> We used to initialize timers early in the legacy board case but for DT
> >> we've so far been able to initialize timers late. Is it really time to
> >> go back again? =)
> > 
> > rcar_gen2_timer_init() calls clocksource_of_init(), and our clocksource
> > devices need clocks, so CCF needs to be initialized before that.
> 
> Well, I can see where you are coming from, and your assumption sounds sane.
> But I don't think your logic is a matching reality! =)
> 
> Our timer drivers like CMT, TMU, MTU2 and STI are all using the good old
> regular platform device driver model for probe(). They do not rely on the
> clocksource_of_init() probe mechanism. The reason for this is that they were
> written before clocksource_of_init() was introduced. Actually, I'm sure
> there is a reason behind it, but I don't see the point of this duplicated
> OF-specific interface at all, but that's a different story.

With a way to pass the boot mode to the CPG driver without using an explicit 
function call from board code to driver code (and a way to generalize our arch 
timer setup code, see below), of_clk_init() and clocksource_of_init() would 
allow getting rid of the init_time callback function completely as the ARM 
core calls those two functions when the init_time callback is NULL.

> The ARM drivers like TWD and arch timer do however rely on
> clocksource_of_init(), but they can all be used without the clock framework
> unless I'm mistaken, so you don't have any dependency there.

As far as I know that's correct.

Speaking of the arch timer, is the initialization code in 
rcar_gen2_timer_init() really specific to our SoCs, or could it be made more 
generic and moved to drivers/clocksource/arm_arch_timer.c somehow ?

> It is my opinion that we should use the regular driver model for our timers.
> If there is some issue with that then we should for instance be able to use
> deferred probing to remedy that.
> 
> Some of our legacy board code is using early platform devices for early
> timers.

Why is that ?

> But all DT reference board code with legacy clocks is setting up timers
> late. Because of this I believe it is very possible to init the timers late
> with CCF as well.
> 
> So from my point of view we should avoid enabling CCF early, it just makes
> things complicated with no apparent upside.
> 
> >> As for initializing the CCF, if we go down the route of early CCF
> >> init, can't you put your CCF init call at one shared location in for
> >> instance rcar_gen2_timer_init()?
> > 
> > That's a good idea, I will do that.
> 
> Thanks. Since I believe it is possible to use late CCF init then I much
> prefer using that. But feel free to prove me wrong!

Initializing CCF at init_time time matches what other ARM platforms do, and 
allows putting the rcar_gen2_clocks_init() call in a shared location. Even if 
not strictly required, it looks to me like it is a good place to initialize 
CCF.

-- 
Regards,

Laurent Pinchart