[PATCH 0/2] Fix potential merge conflict for dw_apb_timer_of

[Heiko Stübner]

Am Dienstag, 18. Juni 2013, 17:38:35 schrieb Heiko Stübner:
> Hi Pavel,
> 
> Am Dienstag, 18. Juni 2013, 17:02:44 schrieb Pavel Machek:
> > Hi!
> > 
> > > >The following 2 patches will eliminate the need for the patch in John
> > > >Stultz's tree. If there is to be merge of the 2 trees, then the
> > > >patch:
> > > >
> > > >dw_apb_timer_of.c: Remove parts that were picoxcell-specific
> > > >
> > > >can be removed from John's tree to avoid a merge-conflict.
> > > >
> > > >Based on arm-soc/for-next:
> > > >
> > > >PATCH[1/2] - Rename "dw-apb-timer-osc" and "dw-apb-timer-sp" bindings
> > > >to just "dw-apb-timer"
> > > >PATCH[2/2] - Fix user/system reporting by fixing read_sched_clock()
> > > 
> > > Pavel/Jamie: Can you take a look at these too and make sure these cover
> > > what you were doing.
> > 
> > [It seems like Heiko Stübner was not aware of patches in the clock
> > tree, so did pretty much equivalent patch.]
> 
> Correct ... I was going after what was in linux-next and the tip.git [which
> I also only saw recently at all] does not seem to be part of it.
> 
> > Dinh's changes look good to me, but
> > 
> > [PATCH v2 4/4] clocksource: dw_apb_timer_of: use clocksource_of_init
> > 
> > does not exactly look nice: (I'm sorry I did not see original series,
> > before it was merged to -soc.). The function counts number of times it
> > was called, and behaves differently in each case. It is not very
> > traditional kernel code at the very least.
> > 
> > +static int num_called;
> > +static void __init dw_apb_timer_init(struct device_node *timer)
> > 
> >  {
> > 
> > -       struct device_node *event_timer, *source_timer;
> > -
> > -       event_timer = of_find_matching_node(NULL, osctimer_ids);
> > -       if (!event_timer)
> > -               panic("No timer for clockevent");
> > -       add_clockevent(event_timer);
> > -
> > -       source_timer = of_find_matching_node(event_timer,
> > osctimer_ids);
> > -       if (!source_timer)
> > -               panic("No timer for clocksource");
> > -       add_clocksource(source_timer);
> > -
> > -       of_node_put(source_timer);
> > +       switch (num_called) {
> > +       case 0:
> > +               pr_debug("%s: found clockevent timer\n", __func__);
> > +               add_clockevent(timer);
> > +               of_node_put(timer);
> > +               break;
> > +       case 1:
> > +               pr_debug("%s: found clocksource timer\n", __func__);
> > +               add_clocksource(timer);
> > +               of_node_put(timer);
> > +               init_sched_clock();
> > +               break;
> > +       default:
> > +               break;
> > +       }
> > 
> > -       init_sched_clock();
> > +       num_called++;
> > 
> >  }
> > 
> > Heiko, can you take a look at John Stultz tree? We modified this area
> > already... I understand you only have one timer on your silicon?

also it seems like not being able to use the apb_timer as sched_clock will 
hurt my platform too.

I've tried to use the arm_arch_timer, but when the arch_timer_get_cntfrq() 
function gets called, I only get an "undefined instruction" Oops for the 
executed asm in there.

As there is no manual available for the SoC, I can only guess that it doesn't 
contain such a component. This is fueled additionally by the PPI part of the 
gic only having 3 interrupt sources [there is small excerpt of the soc-manual 
floating around that contains this information], with one already being the 
twd interrupt, while the arm_arch_timer seems to require 4 itself.

Therefore it would cool, if we could keep the sched_clock functionality 
(provided by the clocksource timer) around somehow.


Heiko



> nope, my silicon has actually three timers of this type (all of them of the
> "snps,dw-apb-timer-osc" type ... which did change it seems).
> 
> But the clocksource also needs to provide the sched_clock on it.
> 
> Due to the multiple matching I came up with the numbering, because the of-
> clocksource must match the timer ips multiple times and needs to use one as
> clockevent and one as clocksource.
> 
> > Would perhaps parameter on dw_apb_timer_init telling it what to do be
> > better solution? I don't like the "num_called" variable too much...
> 
> The problem I see is, how do you want to distinguish between the timer used
> as clockevent and the one used as clocksource. The ip blocks are the same,
> so the dt binding must also be the same, as it only describes the
> hardware.
> 
> And the
> CLOCKSOURCE_OF_DECLARE(apb_timer, "snps,dw-apb-timer-osc",
> dw_apb_timer_init); of course also matches against all the timer nodes in
> the dt.