Re: [PATCH v2 1/2] devicetree: add binding for generic mmio clocksource

["Måns Rullgård" <mans@mansr.com>]

Rob Herring <robherring2@gmail.com> writes:

> On Wed, Oct 7, 2015 at 11:47 AM, Måns Rullgård <mans@mansr.com> wrote:
>> Mark Rutland <mark.rutland@arm.com> writes:
>>
>>> On Wed, Oct 07, 2015 at 04:37:13PM +0100, Mans Rullgard wrote:
>>>> This adds a DT binding for a generic mmio clocksource as implemented
>>>> by clocksource_mmio_init().
>>>>
>>>> Signed-off-by: Mans Rullgard <mans@mansr.com>
>>>> ---
>>>> Changed in v2:
>>>> - added sched_clock support
>>>> ---
>>>>  .../devicetree/bindings/timer/clocksource-mmio.txt | 28 ++++++++++++++++++++++
>>>>  1 file changed, 28 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/timer/clocksource-mmio.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/timer/clocksource-mmio.txt b/Documentation/devicetree/bindings/timer/clocksource-mmio.txt
>>>> new file mode 100644
>>>> index 0000000..cfb3601
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/timer/clocksource-mmio.txt
>>>> @@ -0,0 +1,28 @@
>>>> +Generic MMIO clocksource
>>>> +
>>>> +Required properties:
>>>> +
>>>> +- compatible: should be "clocksource-mmio"
>
> I'm doubtful this matches any h/w. This would assume there is no other
> control like enabling, reset, clock dividers, etc.

I know it matches real hardware.

> Who is your user of this?

Various Sigma Designs chips have such counters.  I figured others might
as well, and it seemed silly to tie such generic functionality to a
specific chip family.

>> I had a bad feeling about this.  How would you suggest determining a
>> suitable value from actual hardware parameters?
>
> I wouldn't. I think there is general agreement that rating is broken.

So hardcoding something like 300 would be OK?

>>>> +- linux,sched-clock: boolean, register clocksource as sched_clock
>>>
>>> Likewise, this property doesn't belong in the DT for the same reasons as
>>> clocksource-rating.
>>
>> What would be a proper way to select a sched_clock source?  I realise
>> it's a Linux-specific thing and DT is supposed to be generic, but the
>> information must be provided somehow.
>
> The kernel already has some logic to do this. Most number of bits
> followed by highest frequency will be the winning sched_clock. You
> might also want to look at things like always on or not.

The problem is that sched_clock_register() doesn't take a pointer to be
passed back to the read_sched_clock callback like most interfaces of
this type do.  This means the callback must use global variables set up
before the register call, but at that time there's no way of knowing
which one will be used.  If there were a way of getting a pointer to the
callback, it would be a simple matter of registering all instances and
letting the kernel choose which to use.

-- 
Måns Rullgård
mans@mansr.com