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

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

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"
> +- reg: the physical address of the counter register
> +- reg-io-width: size of counter register in bytes, should be 2 or 4

Can this not be inferred from the reg?

What about 8 byte counters?

> +- clocks: phandle to the source clock

Is the frequency expected to be exactly the source clock frequency? I
imagine it's possible for there to be a divisor.

We can add properties for that later, but we should be explcit as to
what we currently expect the relationship between the clock and the
clocksource to be.

> +- clocksource-bits: number of valid bits
> +- clocksource-rating: rating of the clocksource

NAK. This has no meaning w.r.t. the hardware. This should not be in the
DT. If there are criteria that bias this (e.g. frequency, latency), they
should eitehr be describedi nteh DT or determined dynamically.

> +Optional properties:
> +
> +- clocksource-counts-down: indicates that counter counts down
> +- label: name of the clocksource
> +- 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.

Mark.

> +
> +Example:
> +
> +clocksource {
> +	compatible = "clocksource-mmio";
> +	reg = <0x10000 4>;
> +	reg-io-width = <4>;
> +	clocksource-bits = <32>;
> +	clocksource-rating = <300>;
> +	clocks = <&clk>;
> +	linux,sched_clock;
> +}
> -- 
> 2.5.3
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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"
>> +- reg: the physical address of the counter register
>> +- reg-io-width: size of counter register in bytes, should be 2 or 4
>
> Can this not be inferred from the reg?

You're right, it can.

> What about 8 byte counters?

The existing code only supports 2 or 4, but that of course doesn't
matter here.

>> +- clocks: phandle to the source clock
>
> Is the frequency expected to be exactly the source clock frequency? I
> imagine it's possible for there to be a divisor.

There could of course be, though there isn't in the hardware I'm dealing
with.  Is specifying it here preferable using a fixed-factor-clock?

> We can add properties for that later, but we should be explcit as to
> what we currently expect the relationship between the clock and the
> clocksource to be.
>
>> +- clocksource-bits: number of valid bits
>> +- clocksource-rating: rating of the clocksource
>
> NAK. This has no meaning w.r.t. the hardware. This should not be in the
> DT. If there are criteria that bias this (e.g. frequency, latency), they
> should either be described in the DT or determined dynamically.

I had a bad feeling about this.  How would you suggest determining a
suitable value from actual hardware parameters?

>> +- 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.

On Wed, Oct 07, 2015 at 05:47:12PM +0100, Måns Rullgård 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"
> >> +- reg: the physical address of the counter register
> >> +- reg-io-width: size of counter register in bytes, should be 2 or 4
> >
> > Can this not be inferred from the reg?
> 
> You're right, it can.
> 
> > What about 8 byte counters?
> 
> The existing code only supports 2 or 4, but that of course doesn't
> matter here.
> 
> >> +- clocks: phandle to the source clock
> >
> > Is the frequency expected to be exactly the source clock frequency? I
> > imagine it's possible for there to be a divisor.
> 
> There could of course be, though there isn't in the hardware I'm dealing
> with.  Is specifying it here preferable using a fixed-factor-clock?

I'm not actually sure; I guess it would be ok to do so.

For now we should just explicitly state that the clocksource is assumed
to tick at the rate of the clock.

> > We can add properties for that later, but we should be explcit as to
> > what we currently expect the relationship between the clock and the
> > clocksource to be.
> >
> >> +- clocksource-bits: number of valid bits
> >> +- clocksource-rating: rating of the clocksource
> >
> > NAK. This has no meaning w.r.t. the hardware. This should not be in the
> > DT. If there are criteria that bias this (e.g. frequency, latency), they
> > should either be described in the DT or determined dynamically.
> 
> I had a bad feeling about this.  How would you suggest determining a
> suitable value from actual hardware parameters?

I don't have a good answer to that given the rating is semi-arbitrary,
but that's also the reason I don't want to see it in the DT.

Can we not just choose a fixed number in the driver for now? Likely
something lower than the architected timers (400 currently).

> >> +- 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.

I think that any source above a certain rate and below a certain
latency, which does not lose state, should be registered (and the best
gets chosen dynamically).

Come to think of it, what's the expected behaviour of this source w.r.t.
power management? I expect that the kernel needs to leave the clock
enabled at all times for this to possibly work.

Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.

Who is your user of this?

>>> +- reg: the physical address of the counter register
>>> +- reg-io-width: size of counter register in bytes, should be 2 or 4
>>
>> Can this not be inferred from the reg?
>
> You're right, it can.
>
>> What about 8 byte counters?
>
> The existing code only supports 2 or 4, but that of course doesn't
> matter here.
>
>>> +- clocks: phandle to the source clock
>>
>> Is the frequency expected to be exactly the source clock frequency? I
>> imagine it's possible for there to be a divisor.
>
> There could of course be, though there isn't in the hardware I'm dealing
> with.  Is specifying it here preferable using a fixed-factor-clock?
>
>> We can add properties for that later, but we should be explcit as to
>> what we currently expect the relationship between the clock and the
>> clocksource to be.
>>
>>> +- clocksource-bits: number of valid bits
>>> +- clocksource-rating: rating of the clocksource
>>
>> NAK. This has no meaning w.r.t. the hardware. This should not be in the
>> DT. If there are criteria that bias this (e.g. frequency, latency), they
>> should either be described in the DT or determined dynamically.
>
> 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.

This has come up a few times before in context of given a pile of
timer h/w how do you select one. Chosen properties have been one
example (I think that actually went in on Integrator, but we since
removed it). Think about what the properties of a timer are and then
you can decide based on properties for those. OMAP timers are a good
example.

>
>>> +- 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.

Rob

>
> --
> Måns Rullgård
> mans@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Rutland <mark.rutland@arm.com> writes:

>> >> +- clocks: phandle to the source clock
>> >
>> > Is the frequency expected to be exactly the source clock frequency? I
>> > imagine it's possible for there to be a divisor.
>> 
>> There could of course be, though there isn't in the hardware I'm dealing
>> with.  Is specifying it here preferable using a fixed-factor-clock?
>
> I'm not actually sure; I guess it would be ok to do so.
>
> For now we should just explicitly state that the clocksource is assumed
> to tick at the rate of the clock.

OK, I'll come up with a clearer wording.

>> > We can add properties for that later, but we should be explcit as to
>> > what we currently expect the relationship between the clock and the
>> > clocksource to be.
>> >
>> >> +- clocksource-bits: number of valid bits
>> >> +- clocksource-rating: rating of the clocksource
>> >
>> > NAK. This has no meaning w.r.t. the hardware. This should not be in the
>> > DT. If there are criteria that bias this (e.g. frequency, latency), they
>> > should either be described in the DT or determined dynamically.
>> 
>> I had a bad feeling about this.  How would you suggest determining a
>> suitable value from actual hardware parameters?
>
> I don't have a good answer to that given the rating is semi-arbitrary,
> but that's also the reason I don't want to see it in the DT.
>
> Can we not just choose a fixed number in the driver for now? Likely
> something lower than the architected timers (400 currently).

Fine with me.

>> >> +- 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.
>
> I think that any source above a certain rate and below a certain
> latency, which does not lose state, should be registered (and the best
> gets chosen dynamically).
>
> Come to think of it, what's the expected behaviour of this source w.r.t.
> power management? I expect that the kernel needs to leave the clock
> enabled at all times for this to possibly work.

The platform I'm dealing with has a 32-bit register counting cycles of
the external clock input which never stops.  It also has a few counters
with a configurable clock source, and those might indeed stop so should
probably not be used for this.

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> writes:

> Rob Herring <robherring2@gmail.com> writes:
>
>> On Wed, Oct 7, 2015 at 11:47 AM, Måns Rullgård <mans@mansr.com> wrote:
>>> 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.

Anyone got a comment on this?  Do I have to send a patch adding this
before anyone will tell me why it's a bad idea?  (That method almost
always works.)

+Stephen who has worked on this code.

On Fri, Oct 9, 2015 at 11:19 AM, Måns Rullgård <mans@mansr.com> wrote:
> Måns Rullgård <mans@mansr.com> writes:
>
>> Rob Herring <robherring2@gmail.com> writes:
>>
>>> On Wed, Oct 7, 2015 at 11:47 AM, Måns Rullgård <mans@mansr.com> wrote:
>>>> 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.
>
> Anyone got a comment on this?  Do I have to send a patch adding this
> before anyone will tell me why it's a bad idea?  (That method almost
> always works.)

Adding a ptr to the callback seems fine to me.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 10/09, Rob Herring wrote:
> +Stephen who has worked on this code.
> 
> On Fri, Oct 9, 2015 at 11:19 AM, Måns Rullgård <mans@mansr.com> wrote:
> > Måns Rullgård <mans@mansr.com> writes:
> >
> >> Rob Herring <robherring2@gmail.com> writes:
> >>
> >>> On Wed, Oct 7, 2015 at 11:47 AM, Måns Rullgård <mans@mansr.com> wrote:
> >>>> 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.
> >
> > Anyone got a comment on this?  Do I have to send a patch adding this
> > before anyone will tell me why it's a bad idea?  (That method almost
> > always works.)
> 
> Adding a ptr to the callback seems fine to me.
> 

Does that mean a flag day? Urgh. Pain. I'm not opposed to adding
a pointer, in fact it might be better for performance so that we
don't take a cache miss in read() functions that need to load
some pointer. We were talking about that problem a few months
ago, but nothing came of it.

Stephen Boyd <sboyd@codeaurora.org> writes:

> On 10/09, Rob Herring wrote:
>> +Stephen who has worked on this code.
>> 
>> On Fri, Oct 9, 2015 at 11:19 AM, Måns Rullgård <mans@mansr.com> wrote:
>> > Måns Rullgård <mans@mansr.com> writes:
>> >
>> >> Rob Herring <robherring2@gmail.com> writes:
>> >>
>> >>> On Wed, Oct 7, 2015 at 11:47 AM, Måns Rullgård <mans@mansr.com> wrote:
>> >>>> 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.
>> >
>> > Anyone got a comment on this?  Do I have to send a patch adding this
>> > before anyone will tell me why it's a bad idea?  (That method almost
>> > always works.)
>> 
>> Adding a ptr to the callback seems fine to me.
>> 
>
> Does that mean a flag day? Urgh. Pain. I'm not opposed to adding
> a pointer, in fact it might be better for performance so that we
> don't take a cache miss in read() functions that need to load
> some pointer. We were talking about that problem a few months
> ago, but nothing came of it.

Flag day in what sense?  There aren't all that many users of the
interface (56, to be precise), and sched_clock_register() isn't
exported.  Verifying the change will be a minor pain, but I don't see
why it should have any major consequences.  Obviously I'd just set the
pointer to null for existing users and leave it for the respective
maintainers to make proper use of it where sensible.

Stephen Boyd <sboyd@codeaurora.org> writes:

> On 10/09, Rob Herring wrote:
>> +Stephen who has worked on this code.
>> 
>> On Fri, Oct 9, 2015 at 11:19 AM, Måns Rullgård <mans@mansr.com> wrote:
>> > Måns Rullgård <mans@mansr.com> writes:
>> >
>> >> Rob Herring <robherring2@gmail.com> writes:
>> >>
>> >>> On Wed, Oct 7, 2015 at 11:47 AM, Måns Rullgård <mans@mansr.com> wrote:
>> >>>> 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.
>> >
>> > Anyone got a comment on this?  Do I have to send a patch adding this
>> > before anyone will tell me why it's a bad idea?  (That method almost
>> > always works.)
>> 
>> Adding a ptr to the callback seems fine to me.
>> 
>
> Does that mean a flag day? Urgh. Pain. I'm not opposed to adding
> a pointer, in fact it might be better for performance so that we
> don't take a cache miss in read() functions that need to load
> some pointer. We were talking about that problem a few months
> ago, but nothing came of it.

I've sent a patch.  Let the flames begin.

On 10/09, Måns Rullgård wrote:
> Stephen Boyd <sboyd@codeaurora.org> writes:
> 
> > On 10/09, Rob Herring wrote:
> >> 
> >> Adding a ptr to the callback seems fine to me.
> >> 
> >
> > Does that mean a flag day? Urgh. Pain. I'm not opposed to adding
> > a pointer, in fact it might be better for performance so that we
> > don't take a cache miss in read() functions that need to load
> > some pointer. We were talking about that problem a few months
> > ago, but nothing came of it.
> 
> Flag day in what sense?  There aren't all that many users of the
> interface (56, to be precise), and sched_clock_register() isn't
> exported. 

That's exactly what a flag day is. Lots of coordination, lots of
acks, etc. Last time when I changed the registration API I made a
new registration API, moved every caller over one by one, and
then deleted the old registration API. That's how you manage a
flag day.

We could probably do the same thing again with two different
types of registration APIs so that we move over users one by one.
The old registration API would be wrapped with a sched_clock
local function that doesn't pass the pointer it gets called with,
while the new registration API would fill in the function pointer
that we call directly from the core code. The double function
call is probably bad for performance, so I guess we should get
rid of it and always pass the pointer to the callback. But this
is at least a method to convert everything gradually without
breaking users that may be going through different trees.

> Verifying the change will be a minor pain, but I don't see
> why it should have any major consequences.  Obviously I'd just set the
> pointer to null for existing users and leave it for the respective
> maintainers to make proper use of it where sensible.
> 

Sure.

On 10/09, Måns Rullgård wrote:
> Stephen Boyd <sboyd@codeaurora.org> writes:
> >
> > Does that mean a flag day? Urgh. Pain. I'm not opposed to adding
> > a pointer, in fact it might be better for performance so that we
> > don't take a cache miss in read() functions that need to load
> > some pointer. We were talking about that problem a few months
> > ago, but nothing came of it.
> 
> I've sent a patch.  Let the flames begin.
> 

I never got it. Was I Cced?