Message-ID: <20170329134134.GJ2123@mai>
Date:   Wed, 29 Mar 2017 15:41:34 +0200
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Mark Rutland <mark.rutland@....com>
Cc:     Rob Herring <robh@...nel.org>,
        Alexander Kochetkov <al.kochet@...il.com>,
        Heiko Stuebner <heiko@...ech.de>, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-rockchip@...ts.infradead.org,
        Thomas Gleixner <tglx@...utronix.de>,
        Russell King <linux@...linux.org.uk>,
        Caesar Wang <wxt@...k-chips.com>,
        Huang Tao <huangtao@...k-chips.com>
Subject: Re: [PATCH v7 1/7] clocksource/drivers/clksrc-evt-probe: Describe
 with the DT both the clocksource and the clockevent

On Wed, Mar 29, 2017 at 01:57:14PM +0100, Mark Rutland wrote:
> On Wed, Mar 29, 2017 at 02:36:38PM +0200, Daniel Lezcano wrote:
> > On Wed, Mar 29, 2017 at 11:49:11AM +0100, Mark Rutland wrote:
> > > On Wed, Mar 29, 2017 at 11:22:10AM +0200, Daniel Lezcano wrote:
> > > > On Tue, Mar 28, 2017 at 08:51:46PM -0500, Rob Herring wrote:
> > > > > On Wed, Mar 22, 2017 at 06:48:28PM +0300, Alexander Kochetkov wrote:
> > 
> > You can have several timers on the system and may want to use the clockevents
> > from one IP block and the clocksource from another IP block. For example, in
> > the case of a bogus clocksource.
> 
> I understand this. However, what I was trying to say is that *how* we
> use a particular device should be a software decision. I have a more
> concrete suggestion on that below.
> 
> > Moreover there are some drivers with a node for a clocksource and
> > another one for the clockevent, and the driver is assuming the clockevent is
> > defined first and then the clocksource.
> > 
> > eg.
> > 
> > arch/arc/boot/dts/abilis_tb10x.dtsi:
> > 
> >         /* TIMER0 with interrupt for clockevent */
> >         timer0 {
> >                 compatible = "snps,arc-timer";
> >                 interrupts = <3>;
> >                 interrupt-parent = <&intc>;
> >                 clocks = <&cpu_clk>;
> >         };
> > 
> >         /* TIMER1 for free running clocksource */
> >         timer1 {
> >                 compatible = "snps,arc-timer";
> >                 clocks = <&cpu_clk>;
> >         };
> > 
> > drivers/clocksource/arc_timer.c:
> > 
> > static int __init arc_of_timer_init(struct device_node *np)
> > {
> >         static int init_count = 0;
> >         int ret;
> > 
> >         if (!init_count) {
> >                 init_count = 1;
> >                 ret = arc_clockevent_setup(np);
> >         } else {
> >                 ret = arc_cs_setup_timer1(np);
> >         }
> > 
> >         return ret;
> > }
> > 
> > Even if that works, it is a fragile mechanism.
> > 
> > So the purpose of these changes is to provide a stronger timer declaration in
> > order to clearly split in the kernel a clocksource and a clockevent
> > initialization.
> 
> I agree that this pattern is not nice. However, I think that splitting
> devices as this level makes the problem *worse*.
> 
> Users care that they have a clocksource and a clockevent device. They
> do not care *which* particular device is used as either. The comments in
> the DT above are at best misleading.

Agree.

And the driver is assuming the first node is the clockevent and the second one
is the clocksource. If the DT invert these nodes, that breaks the driver.

> What we need is for the kernel to understand that devices can be both
> clockevent and clocksource (perhaps mutually exclusively), such that the
> kernel can decide how to make use of devices.
> 
> That way, for the above the kernel can figure out that timer0 could be
> used as clocksource or clockevent, while timer1 can only be used as a
> clocksource due to the lack of an interrupt. Thus, it can choose to use
> timer0 as a clockevent, and timer1 and a clocksource.

Well, 'interrupt' gives an indication the timer can be used as a clockevent and
clocksource, not the clockevent only.

If we take the case of the rockchip, the arm_arch_timer clocksource is stopped
when the CPU is clock gated. So specifically, we don't want to use this
clocksource but we want to use the arch clockevents because they are better.

> > > > > > With this approach, we allow a mechanism to clearly define a clocksource or a
> > > > > > clockevent without aerobatics we can find around in some drivers:
> > > > > > 	timer-sp804.c, arc-timer.c, dw_apb_timer_of.c, mps2-timer.c,
> > > > > > 	renesas-ostm.c, time-efm32.c, time-lpc32xx.c.
> > > > > 
> > > > > These all already have bindings and work. What problem are you trying to 
> > > > > solve other than restructuring Linux?
> > > > 
> > > > Yes, there is already the bindings, but that force to do some hackish
> > > > initialization.
> > > 
> > > Here, you are forcing hackish DT changes that do not truly describe HW.
> > > How is that better?
> > 
> > So if this is hackish DT changes, then the existing DTs should be fixed, no?
> 
> Yes.
> 
> For the above snippet, the only thing that needs to change is the
> comment.
> 
> > > > I would like to give the opportunity to declare separately a clocksource and a
> > > > clockevent, in order to have full control of how this is initialized.
> > > 
> > > To me it sounds like what we need is Linux infrastructure that allows
> > > one to register a device as having both clockevent/clocksource
> > > functionality.
> > 
> > That was the idea. Create a macro CLOCKEVENT_OF and CLOCKSOURCE_OF both of them
> > calling their respective init routines. And in addition a TIMER_OF doing both
> > CLOCKEVENT_OF and CLOCKSOURCE_OF.
> > 
> > It is the DT which does not allow to do this separation.
> > 
> > Would be the following approach more acceptable ?
> > 
> > 1. Replace all CLOCKSOURCE_OF by TIMER_OF (just renaming)
> 
> I am fine with this renaming.
> 
> > 2. A node can have a clockevent and|or a clocksource attributes
> 
> As above, this should not be in the DT given it's describing a
> (Linux-specific) SW policy and not a HW detail.
> 
> So I must disagree with this.

IIUC my discussion with Rob, an attribute is acceptable (btw if
'clocksource'|'clockevent' names are too Linux specific (+1), what
about a more generic name like 'tick' and 'time' ?).

> > 3. The timer_probe pass a flag to the driver's init function, so this one knows
> >    if it should invoke the clockevent/clocksource init functions.
> >    No attribute defaults to clocksource|clockevent.
> > 
> > That would be backward compatible and will let to create drivers with clutch
> > activated device via DT. Also, it will give the opportunity to the existing
> > drivers to change consolidate their initialization routines.
> 
> I think that if anything, we need a combined clocksource+clockevent
> device that we register to the core code. That means all
> clocksource/clockevent drivers have a consolidated routine.
> 
> Subsequently, core code should determine how specifically to use the
> device (e.g. based on what other devices are registered, and their
> capabilities).

IMO, the core code is complex enough and that may imply more heuristics.
Regarding the number of timers, I do believe it is much better to simply tell
which one we want to use via the DT (assuming the DT is a description of the
desired hardware, not all the available hardware).

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Powered by blists - more mailing lists