| Message ID | 1386331027-26065-1-git-send-email-broonie@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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.
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
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).
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
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.
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 {