* Re: [PATCH v3 06/15] ARM: shmobile: koelsch-reference: Initialize CPG device
2013-11-28 1:03 [PATCH v3 06/15] ARM: shmobile: koelsch-reference: Initialize CPG device Laurent Pinchart
@ 2013-11-28 5:32 ` Magnus Damm
2013-11-28 17:17 ` Laurent Pinchart
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Magnus Damm @ 2013-11-28 5:32 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3 06/15] ARM: shmobile: koelsch-reference: Initialize CPG device
2013-11-28 1:03 [PATCH v3 06/15] ARM: shmobile: koelsch-reference: Initialize CPG device Laurent Pinchart
2013-11-28 5:32 ` Magnus Damm
@ 2013-11-28 17:17 ` Laurent Pinchart
2013-11-29 1:43 ` Magnus Damm
2013-11-29 15:21 ` Laurent Pinchart
3 siblings, 0 replies; 5+ messages in thread
From: Laurent Pinchart @ 2013-11-28 17:17 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3 06/15] ARM: shmobile: koelsch-reference: Initialize CPG device
2013-11-28 1:03 [PATCH v3 06/15] ARM: shmobile: koelsch-reference: Initialize CPG device Laurent Pinchart
2013-11-28 5:32 ` Magnus Damm
2013-11-28 17:17 ` Laurent Pinchart
@ 2013-11-29 1:43 ` Magnus Damm
2013-11-29 15:21 ` Laurent Pinchart
3 siblings, 0 replies; 5+ messages in thread
From: Magnus Damm @ 2013-11-29 1:43 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3 06/15] ARM: shmobile: koelsch-reference: Initialize CPG device
2013-11-28 1:03 [PATCH v3 06/15] ARM: shmobile: koelsch-reference: Initialize CPG device Laurent Pinchart
` (2 preceding siblings ...)
2013-11-29 1:43 ` Magnus Damm
@ 2013-11-29 15:21 ` Laurent Pinchart
3 siblings, 0 replies; 5+ messages in thread
From: Laurent Pinchart @ 2013-11-29 15:21 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 5+ messages in thread