| Message ID | 1379625868-5395-1-git-send-email-robherring2@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Just a quick drive-by. Sorry, I don't know the history of previous review cycles. On Thu, Sep 19, 2013 at 2:24 PM, Rob Herring <robherring2@gmail.com> wrote: > Main node optional properties: > > - - cpu_suspend : Function ID for CPU_SUSPEND operation > + - cpu_suspend[-<32|64] : Function ID for CPU_SUSPEND operation > + > + - cpu_off : Function ID for CPU_OFF operation > + > + - cpu_on[-<32|64] : Function ID for CPU_ON operation > + > + - affinity_info[-<32|64] : Function ID for AFFINITY_INFO operation > > - - cpu_off : Function ID for CPU_OFF operation > + - migrate[-<32|64] : Function ID for MIGRATE operation > > - - cpu_on : Function ID for CPU_ON operation > + - migrate_info_type : Function ID for MIGRATE_INFO_TYPE operation > > - - migrate : Function ID for MIGRATE operation > + - migrate_info_up_cpu[-<32|64] : Function ID for MIGRATE_INFO_UP_CPU operation > > + - system_reset : Function ID for SYSTEM_RESET operation > + > + - system_off : Function ID for SYSTEM_OFF operation All of these should use dashes instead of underscores. I also wonder if it would be better to move them into a subnode to keep the namespace a bit cleaner. > +Some functions have have separate IDs for 32-bit and 64-bit calling > +conventions. These separate function IDs are described with function names with > +"-64" and "-32" suffixes (e.g. cpu_on-64). Where a function name does not have > +a suffix, the ID may be used with either calling convention depending on the > +CPU state -- AArch32 callers should use the 32-bit calling convention, and > +AArch64 callers should use the 64-bit calling convention. Why not just make them a possible two-element property with <32 64>, or if only one element, same on both? Seems cleaner. -Olof
On Fri, Sep 20, 2013 at 05:36:51AM +0100, Olof Johansson wrote: > Hi, Hi Olof, > > Just a quick drive-by. Sorry, I don't know the history of previous > review cycles. > > > On Thu, Sep 19, 2013 at 2:24 PM, Rob Herring <robherring2@gmail.com> wrote: > > > Main node optional properties: > > > > - - cpu_suspend : Function ID for CPU_SUSPEND operation > > + - cpu_suspend[-<32|64] : Function ID for CPU_SUSPEND operation > > + > > + - cpu_off : Function ID for CPU_OFF operation > > + > > + - cpu_on[-<32|64] : Function ID for CPU_ON operation > > + > > + - affinity_info[-<32|64] : Function ID for AFFINITY_INFO operation > > > > - - cpu_off : Function ID for CPU_OFF operation > > + - migrate[-<32|64] : Function ID for MIGRATE operation > > > > - - cpu_on : Function ID for CPU_ON operation > > + - migrate_info_type : Function ID for MIGRATE_INFO_TYPE operation > > > > - - migrate : Function ID for MIGRATE operation > > + - migrate_info_up_cpu[-<32|64] : Function ID for MIGRATE_INFO_UP_CPU operation > > > > + - system_reset : Function ID for SYSTEM_RESET operation > > + > > + - system_off : Function ID for SYSTEM_OFF operation > > All of these should use dashes instead of underscores. I also don't like the underscores, but they're an unfortunate historical artifact that we can't get rid of (at least for cpu_on, cpu_off, and cpu_suspend). Both Xen and kvmtool are already passing DTs to guests which have underscores in the property names, so it's both a boot ABI and a userspace ABI. One aim with this update is to ensure that the new binding maintains a level of forward compatibility for OSs -- the PSCI 0.2 binding needs to be a superset of the existing PSCI binding to allow Xen and KVM to provide PSCI 0.2 functionality without sacrificing the ability to boot older guests (kvmtool could be given a flag to choose what version of PSCI to provide, but I'm not sure that can be done for Xen). Given that, I'm not sure if it makes sense to force the rest to have dashes rather than underscores -- it'll leave the binding more inconsistent. > > I also wonder if it would be better to move them into a subnode to > keep the namespace a bit cleaner. For the compatibility reasons above, I don't think we can do this. At least the existing IDs (cpu_on, cpu_off, and cpu_suspend) would need to be described in the psci node rather than a child. > > > +Some functions have have separate IDs for 32-bit and 64-bit calling > > +conventions. These separate function IDs are described with function names with > > +"-64" and "-32" suffixes (e.g. cpu_on-64). Where a function name does not have > > +a suffix, the ID may be used with either calling convention depending on the > > +CPU state -- AArch32 callers should use the 32-bit calling convention, and > > +AArch64 callers should use the 64-bit calling convention. > > Why not just make them a possible two-element property with <32 64>, > or if only one element, same on both? Seems cleaner. A 64-bit platform may not have 32-bit call support, and we wouldn't be able to describe that with a <32 64> property format. As it is valid for a 64-bit OS to make 32-bit calls, we need to not describe a 32-bit call ID in that case. Cheers, Mark.
On Fri, Sep 20, 2013 at 3:20 AM, Mark Rutland <mark.rutland@arm.com> wrote: > On Fri, Sep 20, 2013 at 05:36:51AM +0100, Olof Johansson wrote: >> Hi, > > Hi Olof, > >> >> Just a quick drive-by. Sorry, I don't know the history of previous >> review cycles. >> >> >> On Thu, Sep 19, 2013 at 2:24 PM, Rob Herring <robherring2@gmail.com> wrote: >> >> > Main node optional properties: >> > >> > - - cpu_suspend : Function ID for CPU_SUSPEND operation >> > + - cpu_suspend[-<32|64] : Function ID for CPU_SUSPEND operation >> > + >> > + - cpu_off : Function ID for CPU_OFF operation >> > + >> > + - cpu_on[-<32|64] : Function ID for CPU_ON operation >> > + >> > + - affinity_info[-<32|64] : Function ID for AFFINITY_INFO operation >> > >> > - - cpu_off : Function ID for CPU_OFF operation >> > + - migrate[-<32|64] : Function ID for MIGRATE operation >> > >> > - - cpu_on : Function ID for CPU_ON operation >> > + - migrate_info_type : Function ID for MIGRATE_INFO_TYPE operation >> > >> > - - migrate : Function ID for MIGRATE operation >> > + - migrate_info_up_cpu[-<32|64] : Function ID for MIGRATE_INFO_UP_CPU operation >> > >> > + - system_reset : Function ID for SYSTEM_RESET operation >> > + >> > + - system_off : Function ID for SYSTEM_OFF operation >> >> All of these should use dashes instead of underscores. > > I also don't like the underscores, but they're an unfortunate historical > artifact that we can't get rid of (at least for cpu_on, cpu_off, and > cpu_suspend). We need to keep those three old ones supported in the driver, but the newer binding can use dashes instead, as long as the kernel also handles the old binding. > Both Xen and kvmtool are already passing DTs to guests which have > underscores in the property names, so it's both a boot ABI and a > userspace ABI. Again, as mentioned on IRC: We've said that bindings are not yet stable until we lock them down, so we will need to adjust accordingly. Once we lock them down, they are stable and part of ABI. But we really don't want to deal with the bad bindings that people came up with without much review, and that's what the whole idea behind cleaning up and then locking them down is all about. > One aim with this update is to ensure that the new binding maintains a > level of forward compatibility for OSs -- the PSCI 0.2 binding needs to > be a superset of the existing PSCI binding to allow Xen and KVM to > provide PSCI 0.2 functionality without sacrificing the ability to boot > older guests (kvmtool could be given a flag to choose what version of > PSCI to provide, but I'm not sure that can be done for Xen). I strongly disagree. The very fact that you don't have the guts to call it 1.0 or 2.0 means it's nowhere near stable. :) So, let's take this opportunity to clean up the binding and be done with it. Existing software that can handle the old binding can do so in the future too, but new software (and new device trees) should use the newer binding. > Given that, I'm not sure if it makes sense to force the rest to have > dashes rather than underscores -- it'll leave the binding more > inconsistent. Again, don't mix -- switch everyone over. But make the kernel implementation handle dashes for the three legacy properties. >> I also wonder if it would be better to move them into a subnode to >> keep the namespace a bit cleaner. > > For the compatibility reasons above, I don't think we can do this. At > least the existing IDs (cpu_on, cpu_off, and cpu_suspend) would need to > be described in the psci node rather than a child. Again, we can change the binding as long as the kernel and the tools know what to do with the old bad binding. >> > +Some functions have have separate IDs for 32-bit and 64-bit calling >> > +conventions. These separate function IDs are described with function names with >> > +"-64" and "-32" suffixes (e.g. cpu_on-64). Where a function name does not have >> > +a suffix, the ID may be used with either calling convention depending on the >> > +CPU state -- AArch32 callers should use the 32-bit calling convention, and >> > +AArch64 callers should use the 64-bit calling convention. >> >> Why not just make them a possible two-element property with <32 64>, >> or if only one element, same on both? Seems cleaner. > > A 64-bit platform may not have 32-bit call support, and we wouldn't be > able to describe that with a <32 64> property format. As it is valid for > a 64-bit OS to make 32-bit calls, we need to not describe a 32-bit call > ID in that case. Unless you declare one call id value to mean invalid, and use that to indicate where it's not supported. Seems slightly cleaner but I'm not going to pick a fight over it. ;-) Either way is fine. -Olof
On Wed, Sep 25, 2013 at 12:15 PM, Olof Johansson <olof@lixom.net> wrote: > On Fri, Sep 20, 2013 at 3:20 AM, Mark Rutland <mark.rutland@arm.com> wrote: >> On Fri, Sep 20, 2013 at 05:36:51AM +0100, Olof Johansson wrote: >>> Hi, >> >> Hi Olof, >> >>> >>> Just a quick drive-by. Sorry, I don't know the history of previous >>> review cycles. >>> >>> >>> On Thu, Sep 19, 2013 at 2:24 PM, Rob Herring <robherring2@gmail.com> wrote: >>> >>> > Main node optional properties: >>> > >>> > - - cpu_suspend : Function ID for CPU_SUSPEND operation >>> > + - cpu_suspend[-<32|64] : Function ID for CPU_SUSPEND operation >>> > + >>> > + - cpu_off : Function ID for CPU_OFF operation >>> > + >>> > + - cpu_on[-<32|64] : Function ID for CPU_ON operation >>> > + >>> > + - affinity_info[-<32|64] : Function ID for AFFINITY_INFO operation >>> > >>> > - - cpu_off : Function ID for CPU_OFF operation >>> > + - migrate[-<32|64] : Function ID for MIGRATE operation >>> > >>> > - - cpu_on : Function ID for CPU_ON operation >>> > + - migrate_info_type : Function ID for MIGRATE_INFO_TYPE operation >>> > >>> > - - migrate : Function ID for MIGRATE operation >>> > + - migrate_info_up_cpu[-<32|64] : Function ID for MIGRATE_INFO_UP_CPU operation >>> > >>> > + - system_reset : Function ID for SYSTEM_RESET operation >>> > + >>> > + - system_off : Function ID for SYSTEM_OFF operation >>> >>> All of these should use dashes instead of underscores. >> >> I also don't like the underscores, but they're an unfortunate historical >> artifact that we can't get rid of (at least for cpu_on, cpu_off, and >> cpu_suspend). > > We need to keep those three old ones supported in the driver, but the > newer binding can use dashes instead, as long as the kernel also > handles the old binding. > >> Both Xen and kvmtool are already passing DTs to guests which have >> underscores in the property names, so it's both a boot ABI and a >> userspace ABI. > > Again, as mentioned on IRC: We've said that bindings are not yet > stable until we lock them down, so we will need to adjust accordingly. > Once we lock them down, they are stable and part of ABI. But we really > don't want to deal with the bad bindings that people came up with > without much review, and that's what the whole idea behind cleaning up > and then locking them down is all about. For some of us, bindings are already in production. Until we have some way to document stable vs. unstable, it is not up to anyone but the users of a binding to define the stability. If the users of a binding can tolerate the change, then it is unstable and can change. Otherwise, it has to be treated as stable especially when there are objections to changing it. One way or another, I need to support system_reset and system_off because that is what is in firmware. Yes, it should have all been documented here first, but it is not like I'm just making shit up and documenting after the fact. The binding is documented to some extent in the PSCI document itself. All this boils down to using '_' vs. '-', right? Come on. I'm all for consistency, but let's be practical. There are plenty of documented examples that use '_' starting with device_type. Rob
On Wed, Sep 25, 2013 at 12:20 PM, Rob Herring <robherring2@gmail.com> wrote: > On Wed, Sep 25, 2013 at 12:15 PM, Olof Johansson <olof@lixom.net> wrote: >> On Fri, Sep 20, 2013 at 3:20 AM, Mark Rutland <mark.rutland@arm.com> wrote: >>> On Fri, Sep 20, 2013 at 05:36:51AM +0100, Olof Johansson wrote: >>>> Hi, >>> >>> Hi Olof, >>> >>>> >>>> Just a quick drive-by. Sorry, I don't know the history of previous >>>> review cycles. >>>> >>>> >>>> On Thu, Sep 19, 2013 at 2:24 PM, Rob Herring <robherring2@gmail.com> wrote: >>>> >>>> > Main node optional properties: >>>> > >>>> > - - cpu_suspend : Function ID for CPU_SUSPEND operation >>>> > + - cpu_suspend[-<32|64] : Function ID for CPU_SUSPEND operation >>>> > + >>>> > + - cpu_off : Function ID for CPU_OFF operation >>>> > + >>>> > + - cpu_on[-<32|64] : Function ID for CPU_ON operation >>>> > + >>>> > + - affinity_info[-<32|64] : Function ID for AFFINITY_INFO operation >>>> > >>>> > - - cpu_off : Function ID for CPU_OFF operation >>>> > + - migrate[-<32|64] : Function ID for MIGRATE operation >>>> > >>>> > - - cpu_on : Function ID for CPU_ON operation >>>> > + - migrate_info_type : Function ID for MIGRATE_INFO_TYPE operation >>>> > >>>> > - - migrate : Function ID for MIGRATE operation >>>> > + - migrate_info_up_cpu[-<32|64] : Function ID for MIGRATE_INFO_UP_CPU operation >>>> > >>>> > + - system_reset : Function ID for SYSTEM_RESET operation >>>> > + >>>> > + - system_off : Function ID for SYSTEM_OFF operation >>>> >>>> All of these should use dashes instead of underscores. >>> >>> I also don't like the underscores, but they're an unfortunate historical >>> artifact that we can't get rid of (at least for cpu_on, cpu_off, and >>> cpu_suspend). >> >> We need to keep those three old ones supported in the driver, but the >> newer binding can use dashes instead, as long as the kernel also >> handles the old binding. >> >>> Both Xen and kvmtool are already passing DTs to guests which have >>> underscores in the property names, so it's both a boot ABI and a >>> userspace ABI. >> >> Again, as mentioned on IRC: We've said that bindings are not yet >> stable until we lock them down, so we will need to adjust accordingly. >> Once we lock them down, they are stable and part of ABI. But we really >> don't want to deal with the bad bindings that people came up with >> without much review, and that's what the whole idea behind cleaning up >> and then locking them down is all about. > > For some of us, bindings are already in production. Until we have some > way to document stable vs. unstable, it is not up to anyone but the > users of a binding to define the stability. If the users of a binding > can tolerate the change, then it is unstable and can change. Ok, so why review bindings at all then? Just implement what you want, ship it and force it on the rest of us for ever. > All this boils down to using '_' vs. '-', right? Come on. I'm all for > consistency, but let's be practical. There are plenty of documented > examples that use '_' starting with device_type. Next property I add will be called Device_Type then, if we're willing to not give a shit about best practices any more. Thanks for setting the direction and holding the bar high, Rob! -Olof
On Wed, Sep 25, 2013 at 2:34 PM, Olof Johansson <olof@lixom.net> wrote: > On Wed, Sep 25, 2013 at 12:20 PM, Rob Herring <robherring2@gmail.com> wrote: >> On Wed, Sep 25, 2013 at 12:15 PM, Olof Johansson <olof@lixom.net> wrote: >>> On Fri, Sep 20, 2013 at 3:20 AM, Mark Rutland <mark.rutland@arm.com> wrote: >>>> On Fri, Sep 20, 2013 at 05:36:51AM +0100, Olof Johansson wrote: >>>>> Hi, >>>> >>>> Hi Olof, >>>> >>>>> >>>>> Just a quick drive-by. Sorry, I don't know the history of previous >>>>> review cycles. >>>>> >>>>> >>>>> On Thu, Sep 19, 2013 at 2:24 PM, Rob Herring <robherring2@gmail.com> wrote: >>>>> >>>>> > Main node optional properties: >>>>> > >>>>> > - - cpu_suspend : Function ID for CPU_SUSPEND operation >>>>> > + - cpu_suspend[-<32|64] : Function ID for CPU_SUSPEND operation >>>>> > + >>>>> > + - cpu_off : Function ID for CPU_OFF operation >>>>> > + >>>>> > + - cpu_on[-<32|64] : Function ID for CPU_ON operation >>>>> > + >>>>> > + - affinity_info[-<32|64] : Function ID for AFFINITY_INFO operation >>>>> > >>>>> > - - cpu_off : Function ID for CPU_OFF operation >>>>> > + - migrate[-<32|64] : Function ID for MIGRATE operation >>>>> > >>>>> > - - cpu_on : Function ID for CPU_ON operation >>>>> > + - migrate_info_type : Function ID for MIGRATE_INFO_TYPE operation >>>>> > >>>>> > - - migrate : Function ID for MIGRATE operation >>>>> > + - migrate_info_up_cpu[-<32|64] : Function ID for MIGRATE_INFO_UP_CPU operation >>>>> > >>>>> > + - system_reset : Function ID for SYSTEM_RESET operation >>>>> > + >>>>> > + - system_off : Function ID for SYSTEM_OFF operation >>>>> >>>>> All of these should use dashes instead of underscores. >>>> >>>> I also don't like the underscores, but they're an unfortunate historical >>>> artifact that we can't get rid of (at least for cpu_on, cpu_off, and >>>> cpu_suspend). >>> >>> We need to keep those three old ones supported in the driver, but the >>> newer binding can use dashes instead, as long as the kernel also >>> handles the old binding. >>> >>>> Both Xen and kvmtool are already passing DTs to guests which have >>>> underscores in the property names, so it's both a boot ABI and a >>>> userspace ABI. >>> >>> Again, as mentioned on IRC: We've said that bindings are not yet >>> stable until we lock them down, so we will need to adjust accordingly. >>> Once we lock them down, they are stable and part of ABI. But we really >>> don't want to deal with the bad bindings that people came up with >>> without much review, and that's what the whole idea behind cleaning up >>> and then locking them down is all about. >> >> For some of us, bindings are already in production. Until we have some >> way to document stable vs. unstable, it is not up to anyone but the >> users of a binding to define the stability. If the users of a binding >> can tolerate the change, then it is unstable and can change. > > Ok, so why review bindings at all then? Just implement what you want, > ship it and force it on the rest of us for ever. Dealing with 2 versions of PSCI bindings rather than simply extending the existing binding will be more pain for the kernel, kvmtool, qemu and xen than any benefit of using a hyphen will provide. This binding was written by Will Deacon and acked by Arnd and Nicolas not to mention the follow-up patches to use it over the last 9 months. Hardly a bunch of newbies that don't understand DT. That binding had 3 postings. No one complained about the underscores then. It is precisely because it has been reviewed that I don't think we should be changing it. I don't see simply adding additional function ids as grounds for redoing the binding. The 32/64 bit calling convention handling may be a different discussion which is fine with me. Dealing with that was not my goal here. >> All this boils down to using '_' vs. '-', right? Come on. I'm all for >> consistency, but let's be practical. There are plenty of documented >> examples that use '_' starting with device_type. > > Next property I add will be called Device_Type then, if we're willing > to not give a shit about best practices any more. Thanks for setting > the direction and holding the bar high, Rob! That's fine, just mark it unstable and we can change it later. I'll sit quiet for 9 months first and then complain about it. I'll be waiting for your patch to change all the '_' to '-' in all the binding docs and dts files. Since everything is unstable, that shouldn't be a problem to change. :) Rob
On Wed, Sep 25, 2013 at 2:34 PM, Olof Johansson <olof@lixom.net> wrote: > On Wed, Sep 25, 2013 at 12:20 PM, Rob Herring <robherring2@gmail.com> wrote: >> On Wed, Sep 25, 2013 at 12:15 PM, Olof Johansson <olof@lixom.net> wrote: >>> On Fri, Sep 20, 2013 at 3:20 AM, Mark Rutland <mark.rutland@arm.com> wrote: >>>> On Fri, Sep 20, 2013 at 05:36:51AM +0100, Olof Johansson wrote: >>>>> Hi, >> >> For some of us, bindings are already in production. Until we have some >> way to document stable vs. unstable, it is not up to anyone but the >> users of a binding to define the stability. If the users of a binding >> can tolerate the change, then it is unstable and can change. > > Ok, so why review bindings at all then? Just implement what you want, > ship it and force it on the rest of us for ever. > >> All this boils down to using '_' vs. '-', right? Come on. I'm all for >> consistency, but let's be practical. There are plenty of documented >> examples that use '_' starting with device_type. > > Next property I add will be called Device_Type then, if we're willing > to not give a shit about best practices any more. Thanks for setting > the direction and holding the bar high, Rob! Rob, Olof, The unwritten law regarding new OF properties is that they should be using '-' and not '_'. The only reason device_type and a couple others still exist is because of the legacy of Sun OpenPROM/OpenBoot which preceded OpenFirmware and standardization - it stayed that way for compatibility. There shouldn't be an underscore in any new property name, even if IEEE1275 and then PAPR and then ePAPR completely ignored it the issue, mostly because by defining that _ is invalid, they would never be able to use device_type. And while the use of device_type being completely removed was proposed for the first few versions of PAPR and the PowerPC device tree bindings that preceded it, it was obviously a stupid idea. ~ What this binding achieves is the ability to override the numbers from the docs (which can be hardcoded in the kernel since they are hardcoded in the documentation) with custom numbers is all the binding needs, and the numbering is entirely down to the documentation too - as per the SMC convention and the latest PSCI, that magic 0x40000000 bit is what makes it 64-bit so no need to append or prepend any weird values to the property names or function names. Looking at it completely logically and from a conservative (resource usage) standpoint: The original binding as implemented makes sense as the interface was poorly defined and needed help, as the difference between the original PSCI proposal and the 0.2 revision is the *addition* of fixed function IDs and the SMC calling convention. There is no need to define every PSCI function call number as a property and associated value when for a version of the PSCI spec, that value is locked down. The API is locked down. Adding new functions properties to the DT doesn't make sense if you have an old kernel. If you supply the value from the docs you're wasting space in the DT, and potentially supplying more information than the OS kernel will know what to do with anyway. It follows that in the case where someone overrides a function call with their own custom number for their own custom magic implementation, this is by very definition *not PSCI 0.2 anymore* and therefore the compatible property would be invalid anyway.. Which makes a device tree binding beyond.. psci { compatible = "arm,psci-0.2"; method = "[sh]vc"; }; .. a complete overthinking of the problem. The original binding can stick around in it's limited fashion, but I would have preferred the above, since the property names have underscores (poo!) and don't match the documentation anyway - it is a binding for binding's sake. What it should have been is psci { compatible = "arm,psci"; method = "hvc"; function-ids = 0xf00, 0xf01, 0xf02, 0xf03; function-names = "CPU_ON", "CPU_OFF", "CPU_SUSPEND", "CPU_MIGRATE"; }; Now the function names match the docs, the strcmp code matching those strings will match the docs, and there's no reason to even discuss what those property values would even need to be in the binding, since they are in the original PSCI documentation. They could exist in a binding only to remind people of the 4 functions that even existed. Too late now, though. Device trees aren't for offloading documentation data into, if we can at all help it. Where data is fixed and documented, the documentation is canonical and the device tree needn't replicate it for the benefit of the operating system. Rob wrote: > This binding was written by Will Deacon and acked by Arnd and Nicolas not to mention the follow-up patches to use it over the last 9 months. Hardly a bunch of newbies that don't understand DT. I would have expected better from Arnd who was there for the original arch/ppc -> arch/powerpc thing and SLOF and PAPR and ePAPR and probably had every real PowerPC OpenFirmware platform on his desk at some point. I seem to recall you were around too - as was I. We should have all caught it.. nobody did. Even when people do, sometimes people won't listen anyway ;) There are great examples of weirdo bindings making it into device trees in the deep dark past of arch/ppc -> arch/powerpc while nobody was looking, or shipped on some IBM blade before it was even a specification. Sometimes things slip through the cracks. The fact that there is no standards body or approval committee anymore (besides the self-regulating agile monster we have now) doesn't help. Even the Power.org TSCs didn't do a lot. Until such a thing exists, we just have to do our best to fix stuff after the fact, and until there *is* a standards committee or maybe some kind of validation tool for obvious errors (underscores in properties) or some kind of special syntax definition like an XML schema that things can be validated against... plus a review process where the driver author can be drilled about why that property needs to be this way or what the benefit is for providing it.. device trees as ABI is a moronic gesture. Where device trees are in flux, ways need to be standardized to fix device trees before Linux is loaded. This is *easy as pie* on a real OpenFirmware box. In U-Boot it is a bit of a bitch to do, but it's definitely possible to completely rework the FDT in memory once loaded and add and remove and rename properties.. this ends up being a distro problem, but the distro guys don't want to own it.
On Thu, 19 Sep 2013, Rob Herring wrote: > From: Rob Herring <rob.herring@calxeda.com> > > The PSCI spec from ARM has been updated to 0.2 version. Update the > binding document to reflect the spec changes. For the binding, the > major changes are the addition of psci_version, affinity_info, > migrate_info_type, migrate_info_up_cpu, system_reset and system_off > functions. The recommended function ID numbering has also changed. > > This update also defines 32 and 64 bit calling conventions. The calling > convention is defined on a per function ID basis. > > Signed-off-by: Rob Herring <rob.herring@calxeda.com> > Cc: Dave Martin <Dave.Martin@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Ian Campbell <Ian.Campbell@citrix.com> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Cc: Marc Zyngier <Marc.Zyngier@arm.com> > Cc: Christoffer Dall <christoffer.dall@linaro.org>, > Cc: Charles Garcia-Tobin <Charles.Garcia-Tobin@arm.com> > Cc: Matt Sealey <neko@bakuhatsu.net> > Cc: devicetree@vger.kernel.org Rob, I noticed that this patch hasn't gone upstream yet. Is the interface still under discussion? I am thinking of implementing the Xen side of it. > v4: Reword last paragraph concerning calling convention used for > functions without -32 or -64 suffix. > > v3: This version implements Mark's suggested binding with -32 and -64 > suffixes. > > Documentation/devicetree/bindings/arm/psci.txt | 64 +++++++++++++++++++------- > 1 file changed, 48 insertions(+), 16 deletions(-) > > diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt > index 433afe9..8675ec2 100644 > --- a/Documentation/devicetree/bindings/arm/psci.txt > +++ b/Documentation/devicetree/bindings/arm/psci.txt > @@ -1,16 +1,17 @@ > * Power State Coordination Interface (PSCI) > > Firmware implementing the PSCI functions described in ARM document number > -ARM DEN 0022A ("Power State Coordination Interface System Software on ARM > +ARM DEN 0022B ("Power State Coordination Interface System Software on ARM > processors") can be used by Linux to initiate various CPU-centric power > operations. > > -Issue A of the specification describes functions for CPU suspend, hotplug > -and migration of secure software. > +Issue B of the specification describes functions for CPU suspend, hotplug, > +migration of secure software, and system level reset and powerdown. > > Functions are invoked by trapping to the privilege level of the PSCI > firmware (specified as part of the binding below) and passing arguments > -in a manner similar to that specified by AAPCS: > +as defined in ARM document "SMC Calling Convention" (ARM DEN 0028) in a manner > +similar to that specified by AAPCS: > > r0 => 32-bit Function ID / return value > {r1 - r3} => Parameters > @@ -21,10 +22,10 @@ to #0. > > Main node required properties: > > - - compatible : Must be "arm,psci" > + - compatible : Must contain "arm,psci-0.2" or "arm,psci" > > - - method : The method of calling the PSCI firmware. Permitted > - values are: > + - method : The method defines the calling convention for the PSCI > + firmware. Permitted values are: > > "smc" : SMC #0, with the register assignments specified > in this binding. > @@ -32,24 +33,55 @@ Main node required properties: > "hvc" : HVC #0, with the register assignments specified > in this binding. > > + - psci_version : Function ID for PSCI_VERSION operation. Required for > + "arm,psci-0.2" compatible version or later. > + > Main node optional properties: > > - - cpu_suspend : Function ID for CPU_SUSPEND operation > + - cpu_suspend[-<32|64] : Function ID for CPU_SUSPEND operation > + > + - cpu_off : Function ID for CPU_OFF operation > + > + - cpu_on[-<32|64] : Function ID for CPU_ON operation > + > + - affinity_info[-<32|64] : Function ID for AFFINITY_INFO operation > > - - cpu_off : Function ID for CPU_OFF operation > + - migrate[-<32|64] : Function ID for MIGRATE operation > > - - cpu_on : Function ID for CPU_ON operation > + - migrate_info_type : Function ID for MIGRATE_INFO_TYPE operation > > - - migrate : Function ID for MIGRATE operation > + - migrate_info_up_cpu[-<32|64] : Function ID for MIGRATE_INFO_UP_CPU operation > > + - system_reset : Function ID for SYSTEM_RESET operation > + > + - system_off : Function ID for SYSTEM_OFF operation > + > +Some functions have have separate IDs for 32-bit and 64-bit calling > +conventions. These separate function IDs are described with function names with > +"-64" and "-32" suffixes (e.g. cpu_on-64). Where a function name does not have > +a suffix, the ID may be used with either calling convention depending on the > +CPU state -- AArch32 callers should use the 32-bit calling convention, and > +AArch64 callers should use the 64-bit calling convention. > > Example: > > psci { > - compatible = "arm,psci"; > + compatible = "arm,psci-0.2"; > method = "smc"; > - cpu_suspend = <0x95c10000>; > - cpu_off = <0x95c10001>; > - cpu_on = <0x95c10002>; > - migrate = <0x95c10003>; > + psci_version = <0x84000000>; > + cpu_suspend-32 = <0x84000001>; > + cpu_suspend-64 = <0xc4000001>; > + cpu_off = <0x84000002>; > + cpu_on-32 = <0x84000003>; > + cpu_on-64 = <0xc4000003>; > + affinity_info-32 = <0x84000004>; > + affinity_info-64 = <0xc4000004>; > + migrate-32 = <0x84000005>; > + migrate-64 = <0xc4000005>; > + migrate_info_type = <0x84000006>; > + migrate_info_up_cpu-32 = <0x84000007>; > + migrate_info_up_cpu-64 = <0xc4000007>; > + system_off = <0x84000008>; > + system_reset = <0x84000009>; > }; > + > -- > 1.8.1.2 >
On 11/15/2013 08:48 AM, Stefano Stabellini wrote: > On Thu, 19 Sep 2013, Rob Herring wrote: >> From: Rob Herring <rob.herring@calxeda.com> >> >> The PSCI spec from ARM has been updated to 0.2 version. Update the >> binding document to reflect the spec changes. For the binding, the >> major changes are the addition of psci_version, affinity_info, >> migrate_info_type, migrate_info_up_cpu, system_reset and system_off >> functions. The recommended function ID numbering has also changed. >> >> This update also defines 32 and 64 bit calling conventions. The calling >> convention is defined on a per function ID basis. >> >> Signed-off-by: Rob Herring <rob.herring@calxeda.com> >> Cc: Dave Martin <Dave.Martin@arm.com> >> Cc: Mark Rutland <mark.rutland@arm.com> >> Cc: Ian Campbell <Ian.Campbell@citrix.com> >> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >> Cc: Marc Zyngier <Marc.Zyngier@arm.com> >> Cc: Christoffer Dall <christoffer.dall@linaro.org>, >> Cc: Charles Garcia-Tobin <Charles.Garcia-Tobin@arm.com> >> Cc: Matt Sealey <neko@bakuhatsu.net> >> Cc: devicetree@vger.kernel.org > > Rob, > I noticed that this patch hasn't gone upstream yet. Is the interface > still under discussion? > I am thinking of implementing the Xen side of it. Since we can't seem to agree on how to extend the binding, I plan to go back to something more simple which is simply adding 2 optional properties to the existing "arm,psci" binding. This would be enough to meet the needs of KVM, Xen, and highbank. A future binding can deal with the whole 32 and 64-bit calling convention issue. Rob
On Fri, 15 Nov 2013, Rob Herring wrote: > On 11/15/2013 08:48 AM, Stefano Stabellini wrote: > > On Thu, 19 Sep 2013, Rob Herring wrote: > >> From: Rob Herring <rob.herring@calxeda.com> > >> > >> The PSCI spec from ARM has been updated to 0.2 version. Update the > >> binding document to reflect the spec changes. For the binding, the > >> major changes are the addition of psci_version, affinity_info, > >> migrate_info_type, migrate_info_up_cpu, system_reset and system_off > >> functions. The recommended function ID numbering has also changed. > >> > >> This update also defines 32 and 64 bit calling conventions. The calling > >> convention is defined on a per function ID basis. > >> > >> Signed-off-by: Rob Herring <rob.herring@calxeda.com> > >> Cc: Dave Martin <Dave.Martin@arm.com> > >> Cc: Mark Rutland <mark.rutland@arm.com> > >> Cc: Ian Campbell <Ian.Campbell@citrix.com> > >> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > >> Cc: Marc Zyngier <Marc.Zyngier@arm.com> > >> Cc: Christoffer Dall <christoffer.dall@linaro.org>, > >> Cc: Charles Garcia-Tobin <Charles.Garcia-Tobin@arm.com> > >> Cc: Matt Sealey <neko@bakuhatsu.net> > >> Cc: devicetree@vger.kernel.org > > > > Rob, > > I noticed that this patch hasn't gone upstream yet. Is the interface > > still under discussion? > > I am thinking of implementing the Xen side of it. > > Since we can't seem to agree on how to extend the binding, I plan to go > back to something more simple which is simply adding 2 optional > properties to the existing "arm,psci" binding. This would be enough to > meet the needs of KVM, Xen, and highbank. A future binding can deal with > the whole 32 and 64-bit calling convention issue. OK. TBH Xen doesn't need anything more than what is already specified in "arm,psci". However it would have been nice to fully implement the latest PSCI spec.
diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt index 433afe9..8675ec2 100644 --- a/Documentation/devicetree/bindings/arm/psci.txt +++ b/Documentation/devicetree/bindings/arm/psci.txt @@ -1,16 +1,17 @@ * Power State Coordination Interface (PSCI) Firmware implementing the PSCI functions described in ARM document number -ARM DEN 0022A ("Power State Coordination Interface System Software on ARM +ARM DEN 0022B ("Power State Coordination Interface System Software on ARM processors") can be used by Linux to initiate various CPU-centric power operations. -Issue A of the specification describes functions for CPU suspend, hotplug -and migration of secure software. +Issue B of the specification describes functions for CPU suspend, hotplug, +migration of secure software, and system level reset and powerdown. Functions are invoked by trapping to the privilege level of the PSCI firmware (specified as part of the binding below) and passing arguments -in a manner similar to that specified by AAPCS: +as defined in ARM document "SMC Calling Convention" (ARM DEN 0028) in a manner +similar to that specified by AAPCS: r0 => 32-bit Function ID / return value {r1 - r3} => Parameters @@ -21,10 +22,10 @@ to #0. Main node required properties: - - compatible : Must be "arm,psci" + - compatible : Must contain "arm,psci-0.2" or "arm,psci" - - method : The method of calling the PSCI firmware. Permitted - values are: + - method : The method defines the calling convention for the PSCI + firmware. Permitted values are: "smc" : SMC #0, with the register assignments specified in this binding. @@ -32,24 +33,55 @@ Main node required properties: "hvc" : HVC #0, with the register assignments specified in this binding. + - psci_version : Function ID for PSCI_VERSION operation. Required for + "arm,psci-0.2" compatible version or later. + Main node optional properties: - - cpu_suspend : Function ID for CPU_SUSPEND operation + - cpu_suspend[-<32|64] : Function ID for CPU_SUSPEND operation + + - cpu_off : Function ID for CPU_OFF operation + + - cpu_on[-<32|64] : Function ID for CPU_ON operation + + - affinity_info[-<32|64] : Function ID for AFFINITY_INFO operation - - cpu_off : Function ID for CPU_OFF operation + - migrate[-<32|64] : Function ID for MIGRATE operation - - cpu_on : Function ID for CPU_ON operation + - migrate_info_type : Function ID for MIGRATE_INFO_TYPE operation - - migrate : Function ID for MIGRATE operation + - migrate_info_up_cpu[-<32|64] : Function ID for MIGRATE_INFO_UP_CPU operation + - system_reset : Function ID for SYSTEM_RESET operation + + - system_off : Function ID for SYSTEM_OFF operation + +Some functions have have separate IDs for 32-bit and 64-bit calling +conventions. These separate function IDs are described with function names with +"-64" and "-32" suffixes (e.g. cpu_on-64). Where a function name does not have +a suffix, the ID may be used with either calling convention depending on the +CPU state -- AArch32 callers should use the 32-bit calling convention, and +AArch64 callers should use the 64-bit calling convention. Example: psci { - compatible = "arm,psci"; + compatible = "arm,psci-0.2"; method = "smc"; - cpu_suspend = <0x95c10000>; - cpu_off = <0x95c10001>; - cpu_on = <0x95c10002>; - migrate = <0x95c10003>; + psci_version = <0x84000000>; + cpu_suspend-32 = <0x84000001>; + cpu_suspend-64 = <0xc4000001>; + cpu_off = <0x84000002>; + cpu_on-32 = <0x84000003>; + cpu_on-64 = <0xc4000003>; + affinity_info-32 = <0x84000004>; + affinity_info-64 = <0xc4000004>; + migrate-32 = <0x84000005>; + migrate-64 = <0xc4000005>; + migrate_info_type = <0x84000006>; + migrate_info_up_cpu-32 = <0x84000007>; + migrate_info_up_cpu-64 = <0xc4000007>; + system_off = <0x84000008>; + system_reset = <0x84000009>; }; +