Re: [PATCH 02/14] clocksource/drivers/timer-ti-dm: Add clockevent and clocksource support

[Rob Herring <robh@kernel.org>]

On Thu, Apr 30, 2020 at 10:31 AM Tony Lindgren <tony@atomide.com> wrote:
>
> * Rob Herring <robh@kernel.org> [200430 14:01]:
> > On Mon, Apr 27, 2020 at 08:23:29AM -0700, Tony Lindgren wrote:
> > > * Daniel Lezcano <daniel.lezcano@linaro.org> [200427 15:03]:
> > > > On 27/04/2020 16:31, Tony Lindgren wrote:
> > > > > Hi,
> > > > >
> > > > > * Daniel Lezcano <daniel.lezcano@linaro.org> [200427 09:19]:
> > > > >> On 17/04/2020 18:55, Tony Lindgren wrote:
> > > > >>> --- a/Documentation/devicetree/bindings/timer/ti,timer.txt
> > > > >>> +++ b/Documentation/devicetree/bindings/timer/ti,timer.txt
> > > > >>> @@ -14,6 +14,8 @@ Required properties:
> > > > >>>                       ti,omap5430-timer (applicable to OMAP543x devices)
> > > > >>>                       ti,am335x-timer (applicable to AM335x devices)
> > > > >>>                       ti,am335x-timer-1ms (applicable to AM335x devices)
> > > > >>> +                     ti,dmtimer-clockevent (when used as for clockevent)
> > > > >>> +                     ti,dmtimer-clocksource (when used as for clocksource)
> > > > >>
> > > > >> Please, submit a separate patch for this.
> > > > >>
> > > > >> Before you resend as is, this will be nacked as clocksource / clockevent
> > > > >> is not a hardware description but a Linux thing.
> > > > >>
> > > > >> Finding a way to characterize that from the DT is an endless discussion
> > > > >> since years, so I suggest to use a single property for the timer eg
> > > > >> <ti,dmtimer> and initialize the clocksource and the clockevent in the
> > > > >> driver.
> > > > >
> > > > > Hmm good point. We still need to specify which timer is a clocksource
> > > > > and which one a clockevent somehow.
> > > > >
> > > > > Maybe we could have a generic properties like the clock framework such as:
> > > > >
> > > > > assigned-system-clocksource
> > > > > assigned-system-clockevent
> > > >
> > > > I think that will be the same problem :/
> > >
> > > Seems like other SoCs have the same issue too with multiple timers
> > > to configure.
> > >
> > > > Is it possible to check the interrupt for the clockevent ? A timer node
> > > > with the interrrupt is the clockevent, without it is a clocksource.
> > >
> > > OK let's try that. So the configuration would become then:
> > >
> > > compatible = "ti,dmtimer;   /* reserved for system timers */
> > > /delete-property/interrupts;        /* ok so it's a clocksource */
> > > /delete-property/interrupts-extended;
> >
> > That's not really what was meant.
>
> OK, so let's figure out something better then.
>
> > Let's say you have N timers. Either every timer is exactly the same and
> > the OS can just assign them however it wants or there is some difference
> > in the h/w making certain timer better for certain functions. Describe
> > that difference. It could be clock rate, number of counter bits, always
> > on, secure mode access only, has or doesn't have output compare or input
> > capture, etc.
>
> Hmm. Trying to detect this automatically will get messy. For example,
> we have few omap3 boards with the following options that also need to
> consider if the separate 32KiHz counter is available:
>
> 1. The best case scenario
>
> ti,omap-counter32k clocksource
> ti,sysc-omap2-timer ti,timer-alwon clockevent (timer1)
>
> 2. Boards relying on internal clock with unusable 32k counter
>
> ti,sysc-omap2-timer ti,timer-alwon clocksource (timer12)
> ti,sysc-omap2-timer clockevent (typically gpt2)
>
> In the second case, the 32k counter is unusable, and timer1
> is unusable with the external 32k always on clock. But timer1
> can be used with the system clock just fine for other purposes.
> So ideally we would not tag timer1 as disabled :)

I'm perfectly fine with a 'broken 32k clk' type property.

Though I think the compatibility story is not good changing DT for
stuff needed to boot and needed early in boot. It's one thing to break
something not required to get a system booted.

> For the second case, we could remove ti,timer-alwon property
> for timer1, and tag the 32k counter as disabled as the source
> clock is unreliable. Then somewhere in the code we would need
> to check if ti,omap-counter32k is availabe, then check if
> timer1 is always-on, then use timer12 if not a secure device
> like n900.

IIRC, there's some OMAP timer properties for secure vs. non-secure.
(It's not the first time we've had this discussion on TI timers.)

> If the board wants to use the system clock as the source for
> a higher resolution with assigned-clock-parents, then we'd need
> to ignore the always-on property and not use the 32k counter as
> the clocksource. Basically to somehow figure out that a higher
> resolution configuration is preferred over a
> low-power configuration.

That could be something you want to pick at run-time.

> So what's your take on just adding the generic properties for
> assigned-system-clocksource and clockevent?

I'm tired of discussing this for 10 years...

Rob