[PATCH] ARM: cpu: Document and tweak clock-frequency property

[PATCH] ARM: cpu: Document and tweak clock-frequency property

[Mark Brown]

From: Mark Brown <broonie@linaro.org>

The ARMv7 topology code uses the ePAPR specified mandatory clock-frequency
property to determine the relative performances of the CPUs along with the
CPU type. However with FDT we don't update to take account of the current
speed and if the cores are not running at full speed on boot then a device
tree which is accurate on boot can provide incorrect information about the
relative performances of the cores.

Document the current usage both to override ePAPR and to make the binding
within the kernel more complete. Ideally the kernel would use information
from the CPU frequency scaling drivers here but they may in turn consider
this property and such changes are likely to be part of the energy aware
scheduling work so not immediately available.

Signed-off-by: Mark Brown <broonie@linaro.org>
---
 Documentation/devicetree/bindings/arm/cpus.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
index 91304353eea4..e3726f6bca92 100644
--- a/Documentation/devicetree/bindings/arm/cpus.txt
+++ b/Documentation/devicetree/bindings/arm/cpus.txt
@@ -191,6 +191,15 @@ nodes to be present and contain the properties described below.
 			  property identifying a 64-bit zero-initialised
 			  memory location.
 
+	- clock-frequency
+		Usage: required
+		Value type: <u32> or <u64>
+		Definition:
+			This is specified in ePAPR as the current clock
+			frequency of the CPU.  When used with these
+			extensions it should reflect the maximum clock
+			frequency for the CPU.
+
 Example 1 (dual-cluster big.LITTLE system 32-bit):
 
 	cpus {
-- 
1.8.5.1

[PATCH] ARM: cpu: Document and tweak clock-frequency property

[Rob Herring]

On 12/06/2013 05:57 AM, Mark Brown wrote:
> From: Mark Brown <broonie@linaro.org>
> 
> The ARMv7 topology code uses the ePAPR specified mandatory clock-frequency
> property to determine the relative performances of the CPUs along with the
> CPU type. However with FDT we don't update to take account of the current
> speed and if the cores are not running at full speed on boot then a device
> tree which is accurate on boot can provide incorrect information about the
> relative performances of the cores.
> 
> Document the current usage both to override ePAPR and to make the binding
> within the kernel more complete. Ideally the kernel would use information
> from the CPU frequency scaling drivers here but they may in turn consider
> this property and such changes are likely to be part of the energy aware
> scheduling work so not immediately available.
> 
> Signed-off-by: Mark Brown <broonie@linaro.org>
> ---
>  Documentation/devicetree/bindings/arm/cpus.txt | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
> index 91304353eea4..e3726f6bca92 100644
> --- a/Documentation/devicetree/bindings/arm/cpus.txt
> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> @@ -191,6 +191,15 @@ nodes to be present and contain the properties described below.
>  			  property identifying a 64-bit zero-initialised
>  			  memory location.
>  
> +	- clock-frequency
> +		Usage: required

This breaks compatibility. It may be required for a feature in the
kernel to work, but should not be required in general. Perhaps we need
"optional/recommended" or "optional/required for new designs". Or we
could say required only with heterogeneous cores.

> +		Value type: <u32> or <u64>

How do I determine the size? I think generally this property which is
used in multiple bindings is always u32. Of course, that won't work for
our 5GHz parts next year.

> +		Definition:
> +			This is specified in ePAPR as the current clock
> +			frequency of the CPU.  When used with these
> +			extensions it should reflect the maximum clock
> +			frequency for the CPU.

What does extensions mean? cpu topology nodes?

It is useful to have a standard way to determine the current cpu
frequency. I've been asked for this several times on highbank. This
could be cpufreq, but there is not always a driver loaded. lshw already
has support for reading the frequency using this property. So I'm not
real sure deviating from the ePAPR is a good idea.

If a cpu only supports 1 frequency, then clock-frequency will always
reflect the current and max freq. If a cpu supports multiple
frequencies, then it should have an OPP table with those frequencies. We
should then get max frequency from the OPP table rather than
clock-frequency. It is clear that clock-frequency is insufficient to
describe everything we need.

Rob

[PATCH] ARM: cpu: Document and tweak clock-frequency property

[Mark Brown]

On Sat, Dec 07, 2013 at 12:36:35PM -0600, Rob Herring wrote:
> On 12/06/2013 05:57 AM, Mark Brown wrote:

> > +	- clock-frequency
> > +		Usage: required

> This breaks compatibility. It may be required for a feature in the

It doesn't, it's listed as mandatory in ePAPR section 3.7.1 which is
already part of our bindings - this is merely copying that information
into the binding document in the kernel so that it's more discoverable 
and so that we can tweak the definition to reflect reality a bit more
closely.

> kernel to work, but should not be required in general. Perhaps we need
> "optional/recommended" or "optional/required for new designs". Or we
> could say required only with heterogeneous cores.

For practical purposes a robust implementation should make it optional
(as the current ARMv7 one does) but that doesn't change the spec.

> > +		Value type: <u32> or <u64>

> How do I determine the size? I think generally this property which is
> used in multiple bindings is always u32. Of course, that won't work for
> our 5GHz parts next year.

Beats me.  Actually now I recheck the spec it should be a prop encoded
array consisting of a single element that's either u32 or u64, I guess
the array encodes the information.

> > +		Definition:
> > +			This is specified in ePAPR as the current clock
> > +			frequency of the CPU.  When used with these
> > +			extensions it should reflect the maximum clock
> > +			frequency for the CPU.

> What does extensions mean? cpu topology nodes?

No, it means the document being modified - the same language is used
throughout the document to refer to the extensions it's defining on top
of the base ePAPR CPU bindings.

> It is useful to have a standard way to determine the current cpu
> frequency. I've been asked for this several times on highbank. This
> could be cpufreq, but there is not always a driver loaded. lshw already
> has support for reading the frequency using this property. So I'm not
> real sure deviating from the ePAPR is a good idea.

That'd be nice but it's not terribly practical to do it in DT given that
we have a static DT.  As soon as something does change the frequency the
information goes bad, and I don't know how this is going to play with
things like kexec.  One could spec that the core always needs to be
started at top speed though that doesn't seem helpful.

> If a cpu only supports 1 frequency, then clock-frequency will always
> reflect the current and max freq. If a cpu supports multiple
> frequencies, then it should have an OPP table with those frequencies. We
> should then get max frequency from the OPP table rather than
> clock-frequency. It is clear that clock-frequency is insufficient to
> describe everything we need.

Right, though encoding the operating points into DT isn't always going
to be useful.  What I'm trying to do here is both reflect the existing
ARMv7 usage and make the property a bit more useful.

If defining it to be the maximum frequency isn't going to fly we should
probably just override ePAPR to say it's optional and implementations
should not rely on its accuracy.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131208/92f116da/attachment.sig>

[PATCH] ARM: cpu: Document and tweak clock-frequency property

[Peter Maydell]

On 8 December 2013 16:19, Mark Brown <broonie@kernel.org> wrote:
> On Sat, Dec 07, 2013 at 12:36:35PM -0600, Rob Herring wrote:
>> On 12/06/2013 05:57 AM, Mark Brown wrote:
>
>> > +   - clock-frequency
>> > +           Usage: required
>
>> This breaks compatibility. It may be required for a feature in the
>
> It doesn't, it's listed as mandatory in ePAPR section 3.7.1 which is
> already part of our bindings - this is merely copying that information
> into the binding document in the kernel so that it's more discoverable
> and so that we can tweak the definition to reflect reality a bit more
> closely.

Sorry, but there is already shipping software (kvmtool
and QEMU) which isn't emitting clock-frequency properties
for cpu nodes, based on what you documented in the kernel
doc file, which says:
"The ARM architecture, in accordance with the ePAPR, requires
the cpus and cpu nodes to be present and contain the properties
described below."

not "must contain the properties described below and also
any others that the ePAPR spec says are mandatory".

So I'm afraid you're stuck with this being an optional property.

(It's also not at all clear what a virtual machine's devicetree
should set the clock-frequency properties to anyway...)

thanks
-- PMM

[PATCH] ARM: cpu: Document and tweak clock-frequency property

[Mark Brown]

On Sun, Dec 08, 2013 at 04:38:58PM +0000, Peter Maydell wrote:

> Sorry, but there is already shipping software (kvmtool
> and QEMU) which isn't emitting clock-frequency properties
> for cpu nodes, based on what you documented in the kernel
> doc file, which says:

I didn't document anything here except this patch, I was just trying to
reconcile the implementation with the documentation.

> "The ARM architecture, in accordance with the ePAPR, requires
> the cpus and cpu nodes to be present and contain the properties
> described below."

> not "must contain the properties described below and also
> any others that the ePAPR spec says are mandatory".

I think that's a fairly tortured way of reading the language there to be
honest (and doesn't reflect the actual deployed code reading the binding
which does use this property without documentation outside ePAPR and does
warn if it's absent).  If that really is how we want to read things then
we probably ought to delete the reference to ePAPR both here and in the
other binding documentation we have and fork the specs.

> So I'm afraid you're stuck with this being an optional property.

Like I say I think a reasonable and robust implementation shouldn't
reject a device tree with it missing but that doesn't stop the device
tree being out of spec.  This is also the existing kernel behaviour for
this property so we're stuck with it anyway and my goal here was to
minimise our deviation from the spec so I introduced the minimum
practical change in the process of copying it in for discoverability.

This sort of situation is going to become more and more common as people
actually look at device trees in production; the kernel will have to be
robust against device trees that it previously accepted even if they are
out of spec (and should just generally be robust in parsing).  We've got
to understand that the kernel will fill the role Windows does for PCs -
things that run well enough with existing kernels are going to end up
being released regardless of spec conformance.  Kernels should be
liberal in what they accept, DTs should be conservative in what they
contain and both need to understand that the other is going to get it
wrong some of the time.

> (It's also not at all clear what a virtual machine's devicetree
> should set the clock-frequency properties to anyway...)

Yes, it's a poorly considered property all round.  Most currently
available silicon has variable clocks for the cores which is an issue
with a fixed DT like FDT provides and like you say for simulators and so
on it's meaningless.

Ideally someone with the time/enthuisiasm will get this dealt with more
sensibly in a future revision of ePAPR.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131208/ba398e33/attachment.sig>

[PATCH] ARM: cpu: Document and tweak clock-frequency property

[Peter Maydell]

On 8 December 2013 19:22, Mark Brown <broonie@kernel.org> wrote:
> On Sun, Dec 08, 2013 at 04:38:58PM +0000, Peter Maydell wrote:
>
>> Sorry, but there is already shipping software (kvmtool
>> and QEMU) which isn't emitting clock-frequency properties
>> for cpu nodes, based on what you documented in the kernel
>> doc file, which says:
>
> I didn't document anything here except this patch, I was just trying to
> reconcile the implementation with the documentation.

I mean "you" in the sense of "the Linux kernel developers in
general" :-)

>> "The ARM architecture, in accordance with the ePAPR, requires
>> the cpus and cpu nodes to be present and contain the properties
>> described below."
>
>> not "must contain the properties described below and also
>> any others that the ePAPR spec says are mandatory".
>
> I think that's a fairly tortured way of reading the language there to be
> honest

It's the way that I read it, and presumably also the way it was read
by  Marc Z when he wrote the kvmtool device tree writing code, and
also I would assume the way it was read by the people who wrote the
random three or four dts files in arch/arm/boot/dts that I checked,
since none of them include clock-frequency properties as far as I
can see. If half a dozen different people all produced device
trees with cpu nodes without a clock-frequency property then
I think it's a bit hard to claim that it requires a tortured reading
of the documentation to decide that it's not mandatory.

> (and doesn't reflect the actual deployed code reading the binding
> which does use this property without documentation outside ePAPR and does
> warn if it's absent)

This code should be fixed, then... (I'm pretty sure the kernel
has not always warned about the absence of this property,
or there would not be such a wide prevalence of DTs and
DT generating code which omitted it.)

>> So I'm afraid you're stuck with this being an optional property.
>
> Like I say I think a reasonable and robust implementation shouldn't
> reject a device tree with it missing but that doesn't stop the device
> tree being out of spec.  This is also the existing kernel behaviour for
> this property so we're stuck with it anyway and my goal here was to
> minimise our deviation from the spec so I introduced the minimum
> practical change in the process of copying it in for discoverability.
>
> This sort of situation is going to become more and more common as people
> actually look at device trees in production; the kernel will have to be
> robust against device trees that it previously accepted even if they are
> out of spec (and should just generally be robust in parsing).  We've got
> to understand that the kernel will fill the role Windows does for PCs -
> things that run well enough with existing kernels are going to end up
> being released regardless of spec conformance.  Kernels should be
> liberal in what they accept, DTs should be conservative in what they
> contain and both need to understand that the other is going to get it
> wrong some of the time.

I agree generally with this, and I think I would also include that
the kernel should not start warning about things which it has
not warned about in the past, because this basically manifests
as "user experiences warning which they can't do much about";
it's directed at the wrong target and much too late.

If you want to make properties mandatory then you should
have some sort of setup for validating an entire device tree
including all the properties which the kernel doesn't happen
to use yet or doesn't use in every configuration. That can
then feel free to warn copiously about any missing mandatory
properties, obviously.

>> (It's also not at all clear what a virtual machine's devicetree
>> should set the clock-frequency properties to anyway...)
>
> Yes, it's a poorly considered property all round.  Most currently
> available silicon has variable clocks for the cores which is an issue
> with a fixed DT like FDT provides and like you say for simulators and so
> on it's meaningless.

So what would we lose by making it genuinely optional and
just silently doing something reasonable if it's not set?

thanks
-- PMM

[PATCH] ARM: cpu: Document and tweak clock-frequency property

[Mark Brown]

On Sun, Dec 08, 2013 at 07:51:59PM +0000, Peter Maydell wrote:
> On 8 December 2013 19:22, Mark Brown <broonie@kernel.org> wrote:
> > On Sun, Dec 08, 2013 at 04:38:58PM +0000, Peter Maydell wrote:

> >> "The ARM architecture, in accordance with the ePAPR, requires
> >> the cpus and cpu nodes to be present and contain the properties
> >> described below."

> >> not "must contain the properties described below and also
> >> any others that the ePAPR spec says are mandatory".

> > I think that's a fairly tortured way of reading the language there to be
> > honest

> It's the way that I read it, and presumably also the way it was read
> by  Marc Z when he wrote the kvmtool device tree writing code, and
> also I would assume the way it was read by the people who wrote the
> random three or four dts files in arch/arm/boot/dts that I checked,
> since none of them include clock-frequency properties as far as I
> can see. If half a dozen different people all produced device
> trees with cpu nodes without a clock-frequency property then
> I think it's a bit hard to claim that it requires a tortured reading
> of the documentation to decide that it's not mandatory.

I think you're assuming that people are reading the binding documents at
all when they write things and that should they look at them they will
follow any references to ePAPR (which requires a signup to download) and
then pay attention to the requirements annotations (this is done a lot
more clearly in most of the in-tree documentation) all without making
any mistakes.  I think each of these assumptions is optimistic.

> > (and doesn't reflect the actual deployed code reading the binding
> > which does use this property without documentation outside ePAPR and does
> > warn if it's absent)

> This code should be fixed, then... (I'm pretty sure the kernel
> has not always warned about the absence of this property,
> or there would not be such a wide prevalence of DTs and
> DT generating code which omitted it.)

Too late for that, it's shipping in stable kernels.

> > being released regardless of spec conformance.  Kernels should be
> > liberal in what they accept, DTs should be conservative in what they
> > contain and both need to understand that the other is going to get it
> > wrong some of the time.

> I agree generally with this, and I think I would also include that
> the kernel should not start warning about things which it has
> not warned about in the past, because this basically manifests
> as "user experiences warning which they can't do much about";
> it's directed at the wrong target and much too late.

I think it's reasonable to warn on things, especially in cases where
it's risky or difficult to try to figure out what to do without the
property.  At this point most people can update their DTs and obviously
there's an awful lot of cases where the only people who would ever see
warnings are definitely people in a position to do so (things like
consumer electronics for example).

There does come a point where it's just nitpicking and not helpful but
if it has a substantial effect on functionality then it's useful.  In
this case suppressing the warning for non-asymmetric systems might be
sensible.

> If you want to make properties mandatory then you should
> have some sort of setup for validating an entire device tree
> including all the properties which the kernel doesn't happen
> to use yet or doesn't use in every configuration. That can
> then feel free to warn copiously about any missing mandatory
> properties, obviously.

That's one of the goals with the DT validation software that people like
Tomasz are working on.  Sadly it doesn't exist yet, it would definitely
make life a lot easier.

> >> (It's also not at all clear what a virtual machine's devicetree
> >> should set the clock-frequency properties to anyway...)

> > Yes, it's a poorly considered property all round.  Most currently
> > available silicon has variable clocks for the cores which is an issue
> > with a fixed DT like FDT provides and like you say for simulators and so
> > on it's meaningless.

> So what would we lose by making it genuinely optional and
> just silently doing something reasonable if it's not set?

For all practical purposes it is currently optional but the spec says
it is mandatory.  I would rather err on the side of not changing the
documentation in case someone does work based on ePAPR and/or an old
kernel and since doing that keeps the spec more stable even if we do
implement in a more tolerant fashion within Linux (as we should).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131208/4887bbb5/attachment.sig>

[PATCH] ARM: cpu: Document and tweak clock-frequency property

[Peter Maydell]

On 8 December 2013 21:50, Mark Brown <broonie@kernel.org> wrote:
> On Sun, Dec 08, 2013 at 07:51:59PM +0000, Peter Maydell wrote:
>> On 8 December 2013 19:22, Mark Brown <broonie@kernel.org> wrote:
>> > (and doesn't reflect the actual deployed code reading the binding
>> > which does use this property without documentation outside ePAPR and does
>> > warn if it's absent)
>
>> This code should be fixed, then... (I'm pretty sure the kernel
>> has not always warned about the absence of this property,
>> or there would not be such a wide prevalence of DTs and
>> DT generating code which omitted it.)
>
> Too late for that, it's shipping in stable kernels.

> There does come a point where it's just nitpicking and not helpful but
> if it has a substantial effect on functionality then it's useful.  In
> this case suppressing the warning for non-asymmetric systems might be
> sensible.

Hmm, so "mandatory for non-symmetric [I assume you mean
that and not really 'non-asymmetric'?], otherwise optional" ?
I think that would be reasonable and preserve backwards
compatibility.

> For all practical purposes it is currently optional but the spec says
> it is mandatory.  I would rather err on the side of not changing the
> documentation in case someone does work based on ePAPR and/or an old
> kernel and since doing that keeps the spec more stable even if we do
> implement in a more tolerant fashion within Linux (as we should).

As I say, I don't think your specification currently does say
it is mandatory. If the documentation doesn't clearly list
it as a mandatory parameter, and a large number of
people writing DTS files or DT generation code haven't
put it in, and the kernel didn't complain about it not being
present for a long long time, then de facto it is optional,
and you should make your documentation conform with reality
and fix bugs where the kernel isn't coping with that.

thanks
-- PMM

[PATCH] ARM: cpu: Document and tweak clock-frequency property

[Mark Brown]

On Sun, Dec 08, 2013 at 10:55:28PM +0000, Peter Maydell wrote:
> On 8 December 2013 21:50, Mark Brown <broonie@kernel.org> wrote:

> > There does come a point where it's just nitpicking and not helpful but
> > if it has a substantial effect on functionality then it's useful.  In
> > this case suppressing the warning for non-asymmetric systems might be
> > sensible.

> Hmm, so "mandatory for non-symmetric [I assume you mean
> that and not really 'non-asymmetric'?], otherwise optional" ?
> I think that would be reasonable and preserve backwards
> compatibility.

No, I really mean asymmetric - I'm talking about the cases where we
suppress the warning.

> > For all practical purposes it is currently optional but the spec says
> > it is mandatory.  I would rather err on the side of not changing the
> > documentation in case someone does work based on ePAPR and/or an old
> > kernel and since doing that keeps the spec more stable even if we do
> > implement in a more tolerant fashion within Linux (as we should).

> As I say, I don't think your specification currently does say
> it is mandatory. If the documentation doesn't clearly list
> it as a mandatory parameter, and a large number of

Like I say I don't think that's a sensible interpretation and that if it
is what we want to do then someone's got to find the time to copy all
the bindings out of the spec into the kernel.

> people writing DTS files or DT generation code haven't
> put it in, and the kernel didn't complain about it not being
> present for a long long time, then de facto it is optional,
> and you should make your documentation conform with reality
> and fix bugs where the kernel isn't coping with that.

The kernel currently copes fine with this, welcome to the world of
writing things down in specifications.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131209/5b2adbd4/attachment.sig>