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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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