| Message ID | 20140828170306.GQ14650@leverpostej |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, Aug 28, 2014 at 10:03 AM, Mark Rutland <mark.rutland@arm.com> wrote: > On Thu, Aug 28, 2014 at 05:28:22PM +0100, Olof Johansson wrote: >> On Thu, Aug 28, 2014 at 2:48 AM, Mark Rutland <mark.rutland@arm.com> wrote: >> > Hi, >> > >> >> > + cpus { >> >> > + #address-cells = <2>; >> >> > + #size-cells = <0>; >> >> >> >> Why size-cells=2? Can you not fit a cpuid in 32 bits? >> > >> > As of commit 72aea393a2e7 (arm64: smp: honour #address-size when parsing >> > CPU reg property) Linux can handle single-cell cpu node reg entries >> > where /cpus/#address-cells = <1>. >> > >> > I can't make any guarantees about other code (e.g. bootloaders) which >> > might try to do things with cpu nodes, YMMV. >> >> Ok. If address-cells is kept at 2 the unit address needs to be changed >> to "0,0". So one or the other has to be changed. > > I'm happy either way. > > I'm not sure the rest of the tree had "0," prefixes on all of the > unit-addresses for 64-bit addresses that were under 4GB, and I'm not > sure that existing dts consistently do that either. > > Do we want to enforce that for all 64-bit unit-addresses? Yeah, I believe that's the only valid format for a 2-address-cell unit address. >> > [...] >> > >> >> > + hsi2c_2: hsi2c@14E60000 { >> >> >> >> I much prefer lowercase hex in unit addresses (and reg entries) below. I >> >> know 32-bit uses uppercase, but let's switch going forward here. >> > >> > My preference also; I'm happy to enforce that on new dts. >> > >> > [...] >> > >> >> > + timer { >> >> > + compatible = "arm,armv8-timer"; >> >> > + interrupts = <1 13 0xff01>, >> >> > + <1 14 0xff01>, >> >> > + <1 11 0xff01>, >> >> > + <1 10 0xff01>; >> >> > + clock-frequency = <24000000>; >> >> > + use-clocksource-only; >> >> > + use-physical-timer; >> >> >> >> These two properties are not standard, and I would expect any 64-bit >> >> platform to come with PSCI such that you have a way to initialize the >> >> virtual timers. >> > >> > Likewise with clock-frequency. It's not a full workaround, and it's not >> > hard to initialise CNTFRQ on each CPU. >> >> Technically clock-frequency is documented, but not recommended to be >> used unless needed for working around firmware that doesn't setup the >> register value. :) > > True. > >> In this case it's likely a cargo cult carry over from 5250 where the >> CNTFRQ requirement happened around the same time as we were working on >> it so that generation firmware lacked support for it -- it should >> since then have been fixed properly. > > It's probably unhelpful that the documentation isn't explicit about > that. On that front, how about the patch below? > > Mark. > > ---->8---- > From 67104ad5a56e4c18f9c41f06af028b7561740afd Mon Sep 17 00:00:00 2001 > From: Mark Rutland <mark.rutland@arm.com> > Date: Thu, 28 Aug 2014 17:41:03 +0100 > Subject: [PATCH] Doc: dt: arch_timer: discourage clock-frequency use > > The ARM Generic Timer (AKA the architected timer, arm_arch_timer) > features a CPU register (CNTFRQ) which firmware is intended to > initialize, and non-secure software can read to determine the frequency > of the timer. On CPUs with secure state, this register cannot be written > from non-secure states. > > The firmware of early SoCs featuring the timer did not correctly > initialize CNTFRQ correctly on all CPUs, requiring the frequency to be > described in DT as a workaround. This workaround is not complete however > as CNTFRQ is exposed to all software in a privileged non-secure mode, > including KVM guests. The firmware and DTs for recent SoCs have followed > the example set by these early SoCs. > > This patch updates the arch timer binding documentation to make it > clearer that the use of the clock-frequency property is a poor > work-around. The MMIO generic timer binding is similarly updated, though > this is less of a concern as there is generally no need to expose the > MMIO timers to guest OSs. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Marc Zyngier <marc.zyngier@arm.com> With caps fixed: Acked-by: Olof Johansson <olof@lixom.net> > --- > Documentation/devicetree/bindings/arm/arch_timer.txt | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt > index 37b2caf..5ca3f95 100644 > --- a/Documentation/devicetree/bindings/arm/arch_timer.txt > +++ b/Documentation/devicetree/bindings/arm/arch_timer.txt > @@ -17,7 +17,10 @@ to deliver its interrupts via SPIs. > - interrupts : Interrupt list for secure, non-secure, virtual and > hypervisor timers, in that order. > > -- clock-frequency : The frequency of the main counter, in Hz. Optional. > +- clock-frequency : The frequency of the main counter, in Hz. Should be present > + only where necessary to work around BROKEN firmware which does not configure No need to do broken in all caps. In reality I don't expect it to make a difference on people complying or not. :) > + CNTFRQ on all CPUs to a uniform correct value. Use of this property is > + STRONGLY DISCOURAGED; fix your firmware unless absolutely impossible. Same here. > > - always-on : a boolean property. If present, the timer is powered through an > always-on power domain, therefore it never loses context. > @@ -38,7 +41,8 @@ Example: > > - compatible : Should at least contain "arm,armv7-timer-mem". > > -- clock-frequency : The frequency of the main counter, in Hz. Optional. > +- clock-frequency : The frequency of the main counter, in Hz. Should be present > + only when firmware has not configured the MMIO CNTFRQ registers. > > - reg : The control frame base address. > > -- > 1.9.1 > -- 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 28/08/14 18:03, Mark Rutland wrote: > From 67104ad5a56e4c18f9c41f06af028b7561740afd Mon Sep 17 00:00:00 2001 > From: Mark Rutland <mark.rutland@arm.com> > Date: Thu, 28 Aug 2014 17:41:03 +0100 > Subject: [PATCH] Doc: dt: arch_timer: discourage clock-frequency use > > The ARM Generic Timer (AKA the architected timer, arm_arch_timer) > features a CPU register (CNTFRQ) which firmware is intended to > initialize, and non-secure software can read to determine the frequency > of the timer. On CPUs with secure state, this register cannot be written > from non-secure states. > > The firmware of early SoCs featuring the timer did not correctly > initialize CNTFRQ correctly on all CPUs, requiring the frequency to be > described in DT as a workaround. This workaround is not complete however > as CNTFRQ is exposed to all software in a privileged non-secure mode, > including KVM guests. The firmware and DTs for recent SoCs have followed I believe Xen is also affected by this. > the example set by these early SoCs. > > This patch updates the arch timer binding documentation to make it > clearer that the use of the clock-frequency property is a poor > work-around. The MMIO generic timer binding is similarly updated, though > this is less of a concern as there is generally no need to expose the > MMIO timers to guest OSs. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Marc Zyngier <marc.zyngier@arm.com> Short of more explicit threats: Acked-by: Marc Zyngier <marc.zyngier@arm.com> > --- > Documentation/devicetree/bindings/arm/arch_timer.txt | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt > index 37b2caf..5ca3f95 100644 > --- a/Documentation/devicetree/bindings/arm/arch_timer.txt > +++ b/Documentation/devicetree/bindings/arm/arch_timer.txt > @@ -17,7 +17,10 @@ to deliver its interrupts via SPIs. > - interrupts : Interrupt list for secure, non-secure, virtual and > hypervisor timers, in that order. > > -- clock-frequency : The frequency of the main counter, in Hz. Optional. > +- clock-frequency : The frequency of the main counter, in Hz. Should be present > + only where necessary to work around BROKEN firmware which does not configure > + CNTFRQ on all CPUs to a uniform correct value. Use of this property is > + STRONGLY DISCOURAGED; fix your firmware unless absolutely impossible. > > - always-on : a boolean property. If present, the timer is powered through an > always-on power domain, therefore it never loses context. > @@ -38,7 +41,8 @@ Example: > > - compatible : Should at least contain "arm,armv7-timer-mem". > > -- clock-frequency : The frequency of the main counter, in Hz. Optional. > +- clock-frequency : The frequency of the main counter, in Hz. Should be present > + only when firmware has not configured the MMIO CNTFRQ registers. > > - reg : The control frame base address. > >
On Thu, Aug 28, 2014 at 06:27:04PM +0100, Marc Zyngier wrote: > On 28/08/14 18:03, Mark Rutland wrote: > > > From 67104ad5a56e4c18f9c41f06af028b7561740afd Mon Sep 17 00:00:00 2001 > > From: Mark Rutland <mark.rutland@arm.com> > > Date: Thu, 28 Aug 2014 17:41:03 +0100 > > Subject: [PATCH] Doc: dt: arch_timer: discourage clock-frequency use > > > > The ARM Generic Timer (AKA the architected timer, arm_arch_timer) > > features a CPU register (CNTFRQ) which firmware is intended to > > initialize, and non-secure software can read to determine the frequency > > of the timer. On CPUs with secure state, this register cannot be written > > from non-secure states. > > > > The firmware of early SoCs featuring the timer did not correctly > > initialize CNTFRQ correctly on all CPUs, requiring the frequency to be > > described in DT as a workaround. This workaround is not complete however > > as CNTFRQ is exposed to all software in a privileged non-secure mode, > > including KVM guests. The firmware and DTs for recent SoCs have followed > > I believe Xen is also affected by this. True. s/KVM/KVM\/Xen/, then? > > the example set by these early SoCs. > > > > This patch updates the arch timer binding documentation to make it > > clearer that the use of the clock-frequency property is a poor > > work-around. The MMIO generic timer binding is similarly updated, though > > this is less of a concern as there is generally no need to expose the > > MMIO timers to guest OSs. > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > Cc: Marc Zyngier <marc.zyngier@arm.com> > > Short of more explicit threats: > > Acked-by: Marc Zyngier <marc.zyngier@arm.com> Cheers. Mark. > > > --- > > Documentation/devicetree/bindings/arm/arch_timer.txt | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt > > index 37b2caf..5ca3f95 100644 > > --- a/Documentation/devicetree/bindings/arm/arch_timer.txt > > +++ b/Documentation/devicetree/bindings/arm/arch_timer.txt > > @@ -17,7 +17,10 @@ to deliver its interrupts via SPIs. > > - interrupts : Interrupt list for secure, non-secure, virtual and > > hypervisor timers, in that order. > > > > -- clock-frequency : The frequency of the main counter, in Hz. Optional. > > +- clock-frequency : The frequency of the main counter, in Hz. Should be present > > + only where necessary to work around BROKEN firmware which does not configure > > + CNTFRQ on all CPUs to a uniform correct value. Use of this property is > > + STRONGLY DISCOURAGED; fix your firmware unless absolutely impossible. > > > > - always-on : a boolean property. If present, the timer is powered through an > > always-on power domain, therefore it never loses context. > > @@ -38,7 +41,8 @@ Example: > > > > - compatible : Should at least contain "arm,armv7-timer-mem". > > > > -- clock-frequency : The frequency of the main counter, in Hz. Optional. > > +- clock-frequency : The frequency of the main counter, in Hz. Should be present > > + only when firmware has not configured the MMIO CNTFRQ registers. > > > > - reg : The control frame base address. > > > > > > > -- > Jazz is not dead. It just smells funny... -- 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 Thu, Aug 28, 2014 at 12:27 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 28/08/14 18:03, Mark Rutland wrote: > >> From 67104ad5a56e4c18f9c41f06af028b7561740afd Mon Sep 17 00:00:00 2001 >> From: Mark Rutland <mark.rutland@arm.com> >> Date: Thu, 28 Aug 2014 17:41:03 +0100 >> Subject: [PATCH] Doc: dt: arch_timer: discourage clock-frequency use >> >> The ARM Generic Timer (AKA the architected timer, arm_arch_timer) >> features a CPU register (CNTFRQ) which firmware is intended to >> initialize, and non-secure software can read to determine the frequency >> of the timer. On CPUs with secure state, this register cannot be written >> from non-secure states. >> >> The firmware of early SoCs featuring the timer did not correctly >> initialize CNTFRQ correctly on all CPUs, requiring the frequency to be >> described in DT as a workaround. This workaround is not complete however >> as CNTFRQ is exposed to all software in a privileged non-secure mode, >> including KVM guests. The firmware and DTs for recent SoCs have followed > > I believe Xen is also affected by this. > >> the example set by these early SoCs. >> >> This patch updates the arch timer binding documentation to make it >> clearer that the use of the clock-frequency property is a poor >> work-around. The MMIO generic timer binding is similarly updated, though >> this is less of a concern as there is generally no need to expose the >> MMIO timers to guest OSs. >> >> Signed-off-by: Mark Rutland <mark.rutland@arm.com> >> Cc: Marc Zyngier <marc.zyngier@arm.com> > > Short of more explicit threats: Why not also add WARNs (and mark for stable). People tend to notice and fix those. If not the vendors, those pesky customers always complain (the same ones that get concerned about BogoMIPS values being too low). 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 28/08/14 18:30, Mark Rutland wrote: > On Thu, Aug 28, 2014 at 06:27:04PM +0100, Marc Zyngier wrote: >> On 28/08/14 18:03, Mark Rutland wrote: >> >>> From 67104ad5a56e4c18f9c41f06af028b7561740afd Mon Sep 17 00:00:00 2001 >>> From: Mark Rutland <mark.rutland@arm.com> >>> Date: Thu, 28 Aug 2014 17:41:03 +0100 >>> Subject: [PATCH] Doc: dt: arch_timer: discourage clock-frequency use >>> >>> The ARM Generic Timer (AKA the architected timer, arm_arch_timer) >>> features a CPU register (CNTFRQ) which firmware is intended to >>> initialize, and non-secure software can read to determine the frequency >>> of the timer. On CPUs with secure state, this register cannot be written >>> from non-secure states. >>> >>> The firmware of early SoCs featuring the timer did not correctly >>> initialize CNTFRQ correctly on all CPUs, requiring the frequency to be >>> described in DT as a workaround. This workaround is not complete however >>> as CNTFRQ is exposed to all software in a privileged non-secure mode, >>> including KVM guests. The firmware and DTs for recent SoCs have followed >> >> I believe Xen is also affected by this. > > True. > > s/KVM/KVM\/Xen/, then? Yup. Or "including guests running under a hypervisor", I expect this to be such a fundamental problem that all hypervisors will trip over on that one (Jailhouse definitely does). Thanks, M.
On Thu, Aug 28, 2014 at 06:19:00PM +0100, Olof Johansson wrote: > On Thu, Aug 28, 2014 at 10:03 AM, Mark Rutland <mark.rutland@arm.com> wrote: > > On Thu, Aug 28, 2014 at 05:28:22PM +0100, Olof Johansson wrote: > >> On Thu, Aug 28, 2014 at 2:48 AM, Mark Rutland <mark.rutland@arm.com> wrote: > >> > Hi, > >> > > >> >> > + cpus { > >> >> > + #address-cells = <2>; > >> >> > + #size-cells = <0>; > >> >> > >> >> Why size-cells=2? Can you not fit a cpuid in 32 bits? > >> > > >> > As of commit 72aea393a2e7 (arm64: smp: honour #address-size when parsing > >> > CPU reg property) Linux can handle single-cell cpu node reg entries > >> > where /cpus/#address-cells = <1>. > >> > > >> > I can't make any guarantees about other code (e.g. bootloaders) which > >> > might try to do things with cpu nodes, YMMV. > >> > >> Ok. If address-cells is kept at 2 the unit address needs to be changed > >> to "0,0". So one or the other has to be changed. > > > > I'm happy either way. > > > > I'm not sure the rest of the tree had "0," prefixes on all of the > > unit-addresses for 64-bit addresses that were under 4GB, and I'm not > > sure that existing dts consistently do that either. > > > > Do we want to enforce that for all 64-bit unit-addresses? > > Yeah, I believe that's the only valid format for a 2-address-cell unit address. Fair enough. I didn't spot this explicitly mentioned anywhere in ePAPR, but the examples match. I should probably re-jig that checkpatch test I had for unit-addresses. > >> > [...] > >> > > >> >> > + hsi2c_2: hsi2c@14E60000 { > >> >> > >> >> I much prefer lowercase hex in unit addresses (and reg entries) below. I > >> >> know 32-bit uses uppercase, but let's switch going forward here. > >> > > >> > My preference also; I'm happy to enforce that on new dts. > >> > > >> > [...] > >> > > >> >> > + timer { > >> >> > + compatible = "arm,armv8-timer"; > >> >> > + interrupts = <1 13 0xff01>, > >> >> > + <1 14 0xff01>, > >> >> > + <1 11 0xff01>, > >> >> > + <1 10 0xff01>; > >> >> > + clock-frequency = <24000000>; > >> >> > + use-clocksource-only; > >> >> > + use-physical-timer; > >> >> > >> >> These two properties are not standard, and I would expect any 64-bit > >> >> platform to come with PSCI such that you have a way to initialize the > >> >> virtual timers. > >> > > >> > Likewise with clock-frequency. It's not a full workaround, and it's not > >> > hard to initialise CNTFRQ on each CPU. > >> > >> Technically clock-frequency is documented, but not recommended to be > >> used unless needed for working around firmware that doesn't setup the > >> register value. :) > > > > True. > > > >> In this case it's likely a cargo cult carry over from 5250 where the > >> CNTFRQ requirement happened around the same time as we were working on > >> it so that generation firmware lacked support for it -- it should > >> since then have been fixed properly. > > > > It's probably unhelpful that the documentation isn't explicit about > > that. On that front, how about the patch below? > > > > Mark. > > > > ---->8---- > > From 67104ad5a56e4c18f9c41f06af028b7561740afd Mon Sep 17 00:00:00 2001 > > From: Mark Rutland <mark.rutland@arm.com> > > Date: Thu, 28 Aug 2014 17:41:03 +0100 > > Subject: [PATCH] Doc: dt: arch_timer: discourage clock-frequency use > > > > The ARM Generic Timer (AKA the architected timer, arm_arch_timer) > > features a CPU register (CNTFRQ) which firmware is intended to > > initialize, and non-secure software can read to determine the frequency > > of the timer. On CPUs with secure state, this register cannot be written > > from non-secure states. > > > > The firmware of early SoCs featuring the timer did not correctly > > initialize CNTFRQ correctly on all CPUs, requiring the frequency to be > > described in DT as a workaround. This workaround is not complete however > > as CNTFRQ is exposed to all software in a privileged non-secure mode, > > including KVM guests. The firmware and DTs for recent SoCs have followed > > the example set by these early SoCs. > > > > This patch updates the arch timer binding documentation to make it > > clearer that the use of the clock-frequency property is a poor > > work-around. The MMIO generic timer binding is similarly updated, though > > this is less of a concern as there is generally no need to expose the > > MMIO timers to guest OSs. > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > Cc: Marc Zyngier <marc.zyngier@arm.com> > > With caps fixed: > > Acked-by: Olof Johansson <olof@lixom.net> Cheers. > > --- > > Documentation/devicetree/bindings/arm/arch_timer.txt | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt > > index 37b2caf..5ca3f95 100644 > > --- a/Documentation/devicetree/bindings/arm/arch_timer.txt > > +++ b/Documentation/devicetree/bindings/arm/arch_timer.txt > > @@ -17,7 +17,10 @@ to deliver its interrupts via SPIs. > > - interrupts : Interrupt list for secure, non-secure, virtual and > > hypervisor timers, in that order. > > > > -- clock-frequency : The frequency of the main counter, in Hz. Optional. > > +- clock-frequency : The frequency of the main counter, in Hz. Should be present > > + only where necessary to work around BROKEN firmware which does not configure > > No need to do broken in all caps. In reality I don't expect it to make > a difference on people complying or not. :) Sure. I'll save the caps for replies to violators ;) Mark. > > + CNTFRQ on all CPUs to a uniform correct value. Use of this property is > > + STRONGLY DISCOURAGED; fix your firmware unless absolutely impossible. > > Same here. > > > > > - always-on : a boolean property. If present, the timer is powered through an > > always-on power domain, therefore it never loses context. > > @@ -38,7 +41,8 @@ Example: > > > > - compatible : Should at least contain "arm,armv7-timer-mem". > > > > -- clock-frequency : The frequency of the main counter, in Hz. Optional. > > +- clock-frequency : The frequency of the main counter, in Hz. Should be present > > + only when firmware has not configured the MMIO CNTFRQ registers. > > > > - reg : The control frame base address. > > > > -- > > 1.9.1 > > > -- 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 Thu, Aug 28, 2014 at 06:33:13PM +0100, Rob Herring wrote: > On Thu, Aug 28, 2014 at 12:27 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: > > On 28/08/14 18:03, Mark Rutland wrote: > > > >> From 67104ad5a56e4c18f9c41f06af028b7561740afd Mon Sep 17 00:00:00 2001 > >> From: Mark Rutland <mark.rutland@arm.com> > >> Date: Thu, 28 Aug 2014 17:41:03 +0100 > >> Subject: [PATCH] Doc: dt: arch_timer: discourage clock-frequency use > >> > >> The ARM Generic Timer (AKA the architected timer, arm_arch_timer) > >> features a CPU register (CNTFRQ) which firmware is intended to > >> initialize, and non-secure software can read to determine the frequency > >> of the timer. On CPUs with secure state, this register cannot be written > >> from non-secure states. > >> > >> The firmware of early SoCs featuring the timer did not correctly > >> initialize CNTFRQ correctly on all CPUs, requiring the frequency to be > >> described in DT as a workaround. This workaround is not complete however > >> as CNTFRQ is exposed to all software in a privileged non-secure mode, > >> including KVM guests. The firmware and DTs for recent SoCs have followed > > > > I believe Xen is also affected by this. > > > >> the example set by these early SoCs. > >> > >> This patch updates the arch timer binding documentation to make it > >> clearer that the use of the clock-frequency property is a poor > >> work-around. The MMIO generic timer binding is similarly updated, though > >> this is less of a concern as there is generally no need to expose the > >> MMIO timers to guest OSs. > >> > >> Signed-off-by: Mark Rutland <mark.rutland@arm.com> > >> Cc: Marc Zyngier <marc.zyngier@arm.com> > > > > Short of more explicit threats: > > Why not also add WARNs (and mark for stable). People tend to notice > and fix those. If not the vendors, those pesky customers always > complain (the same ones that get concerned about BogoMIPS values being > too low). I had a go a while back but it was a bit painful becuase the MMIO and cpu timer code is intertwined, and a clock-frequency property on the MMIO timers isn't all that problematic (though admittedly still a workaround for FW not initialising something it was intended to). I was hoping I'd have the chance to split the driver in two first, so as to sidestep that. I'll take another look. 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 Thu, Aug 28, 2014 at 06:37:19PM +0100, Marc Zyngier wrote: > On 28/08/14 18:30, Mark Rutland wrote: > > On Thu, Aug 28, 2014 at 06:27:04PM +0100, Marc Zyngier wrote: > >> On 28/08/14 18:03, Mark Rutland wrote: > >> > >>> From 67104ad5a56e4c18f9c41f06af028b7561740afd Mon Sep 17 00:00:00 2001 > >>> From: Mark Rutland <mark.rutland@arm.com> > >>> Date: Thu, 28 Aug 2014 17:41:03 +0100 > >>> Subject: [PATCH] Doc: dt: arch_timer: discourage clock-frequency use > >>> > >>> The ARM Generic Timer (AKA the architected timer, arm_arch_timer) > >>> features a CPU register (CNTFRQ) which firmware is intended to > >>> initialize, and non-secure software can read to determine the frequency > >>> of the timer. On CPUs with secure state, this register cannot be written > >>> from non-secure states. > >>> > >>> The firmware of early SoCs featuring the timer did not correctly > >>> initialize CNTFRQ correctly on all CPUs, requiring the frequency to be > >>> described in DT as a workaround. This workaround is not complete however > >>> as CNTFRQ is exposed to all software in a privileged non-secure mode, > >>> including KVM guests. The firmware and DTs for recent SoCs have followed > >> > >> I believe Xen is also affected by this. > > > > True. > > > > s/KVM/KVM\/Xen/, then? > > Yup. Or "including guests running under a hypervisor" Ah, that sounds better. I'll use that for the next posting. > I expect this to be such a fundamental problem that all hypervisors > will trip over on that one (Jailhouse definitely does). Yeah, this is a generic problem. 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
Hi Mark, On Thu, Aug 28, 2014 at 7:39 PM, Mark Rutland <mark.rutland@arm.com> wrote: >> >> Ok. If address-cells is kept at 2 the unit address needs to be changed >> >> to "0,0". So one or the other has to be changed. >> > >> > I'm happy either way. >> > >> > I'm not sure the rest of the tree had "0," prefixes on all of the >> > unit-addresses for 64-bit addresses that were under 4GB, and I'm not >> > sure that existing dts consistently do that either. >> > >> > Do we want to enforce that for all 64-bit unit-addresses? >> >> Yeah, I believe that's the only valid format for a 2-address-cell unit address. > > Fair enough. I didn't spot this explicitly mentioned anywhere in ePAPR, > but the examples match. I couldn't find much about how the unit-addresses should really look like. Power_ePAPR_APPROVED_v1.1.pdf: "The unit-address component of the name is specific to the bus type on which the node sits. It consists of one or more ASCII characters from the set of characters in Table 2-1. The unit-address must match the first address specified in the reg property of the node. If the node has no reg property, the @ and unit-address must be omitted and the node-name alone differentiates the node from other nodes at the same level in the tree. The binding for a particular bus may specify additional, more specific requirements for the format of reg and the unit-address." "Table 2.1" contains lot of characters, definitely not limited to hex numbers. Also nothing about (not) needing a "0x" prefix. > I should probably re-jig that checkpatch test I had for unit-addresses. It would be great if dtc started complaining about unit-addresses not matching the first reg property. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- 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 Thu, Aug 28, 2014 at 12:19 PM, Olof Johansson <olof@lixom.net> wrote: > On Thu, Aug 28, 2014 at 10:03 AM, Mark Rutland <mark.rutland@arm.com> wrote: >> On Thu, Aug 28, 2014 at 05:28:22PM +0100, Olof Johansson wrote: >>> On Thu, Aug 28, 2014 at 2:48 AM, Mark Rutland <mark.rutland@arm.com> wrote: >>> > Hi, >>> > >>> >> > + cpus { >>> >> > + #address-cells = <2>; >>> >> > + #size-cells = <0>; >>> >> >>> >> Why size-cells=2? Can you not fit a cpuid in 32 bits? >>> > >>> > As of commit 72aea393a2e7 (arm64: smp: honour #address-size when parsing >>> > CPU reg property) Linux can handle single-cell cpu node reg entries >>> > where /cpus/#address-cells = <1>. >>> > >>> > I can't make any guarantees about other code (e.g. bootloaders) which >>> > might try to do things with cpu nodes, YMMV. >>> >>> Ok. If address-cells is kept at 2 the unit address needs to be changed >>> to "0,0". So one or the other has to be changed. >> >> I'm happy either way. >> >> I'm not sure the rest of the tree had "0," prefixes on all of the >> unit-addresses for 64-bit addresses that were under 4GB, and I'm not >> sure that existing dts consistently do that either. >> >> Do we want to enforce that for all 64-bit unit-addresses? > > Yeah, I believe that's the only valid format for a 2-address-cell unit address. But we don't do leading 0's anywhere else like single cell unit addresses. Buses expressed with ranges and offsets are one example. Also, I2C addresses have a 32-bit size in DT yet are only 8-bit and we don't do leading zero's there. 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 Thu, Aug 28, 2014 at 06:47:00PM +0100, Geert Uytterhoeven wrote: > Hi Mark, > > On Thu, Aug 28, 2014 at 7:39 PM, Mark Rutland <mark.rutland@arm.com> wrote: > >> >> Ok. If address-cells is kept at 2 the unit address needs to be changed > >> >> to "0,0". So one or the other has to be changed. > >> > > >> > I'm happy either way. > >> > > >> > I'm not sure the rest of the tree had "0," prefixes on all of the > >> > unit-addresses for 64-bit addresses that were under 4GB, and I'm not > >> > sure that existing dts consistently do that either. > >> > > >> > Do we want to enforce that for all 64-bit unit-addresses? > >> > >> Yeah, I believe that's the only valid format for a 2-address-cell unit address. > > > > Fair enough. I didn't spot this explicitly mentioned anywhere in ePAPR, > > but the examples match. > > I couldn't find much about how the unit-addresses should really look like. > > Power_ePAPR_APPROVED_v1.1.pdf: > "The unit-address component of the name is specific to the bus type on > which the node sits. It consists > of one or more ASCII characters from the set of characters in Table > 2-1. The unit-address must > match the first address specified in the reg property of the node. If > the node has no reg property, the > @ and unit-address must be omitted and the node-name alone > differentiates the node from other nodes > at the same level in the tree. The binding for a particular bus may > specify additional, more specific > requirements for the format of reg and the unit-address." > > "Table 2.1" contains lot of characters, definitely not limited to hex numbers. > Also nothing about (not) needing a "0x" prefix. This is unfortunate. I guess this was assumed to be implied by way of the examples. :/ > > I should probably re-jig that checkpatch test I had for unit-addresses. > > It would be great if dtc started complaining about unit-addresses not > matching the first reg property. Agreed. When I last tried I thought that required more complex parsing than could be done with a regex. That said, I'd forgotten that properties must come before child nodes, so I though I had to at least balance '{' and '}' for children. I guess all we need to do is find a line beginning with '\s*reg\s*=\s*<' before the next '{' or '}'. Maybe this will be easier than previously thought. :) 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 Thu, Aug 28, 2014 at 10:54 AM, Rob Herring <robh@kernel.org> wrote: > On Thu, Aug 28, 2014 at 12:19 PM, Olof Johansson <olof@lixom.net> wrote: >> On Thu, Aug 28, 2014 at 10:03 AM, Mark Rutland <mark.rutland@arm.com> wrote: >>> On Thu, Aug 28, 2014 at 05:28:22PM +0100, Olof Johansson wrote: >>>> On Thu, Aug 28, 2014 at 2:48 AM, Mark Rutland <mark.rutland@arm.com> wrote: >>>> > Hi, >>>> > >>>> >> > + cpus { >>>> >> > + #address-cells = <2>; >>>> >> > + #size-cells = <0>; >>>> >> >>>> >> Why size-cells=2? Can you not fit a cpuid in 32 bits? >>>> > >>>> > As of commit 72aea393a2e7 (arm64: smp: honour #address-size when parsing >>>> > CPU reg property) Linux can handle single-cell cpu node reg entries >>>> > where /cpus/#address-cells = <1>. >>>> > >>>> > I can't make any guarantees about other code (e.g. bootloaders) which >>>> > might try to do things with cpu nodes, YMMV. >>>> >>>> Ok. If address-cells is kept at 2 the unit address needs to be changed >>>> to "0,0". So one or the other has to be changed. >>> >>> I'm happy either way. >>> >>> I'm not sure the rest of the tree had "0," prefixes on all of the >>> unit-addresses for 64-bit addresses that were under 4GB, and I'm not >>> sure that existing dts consistently do that either. >>> >>> Do we want to enforce that for all 64-bit unit-addresses? >> >> Yeah, I believe that's the only valid format for a 2-address-cell unit address. > > But we don't do leading 0's anywhere else like single cell unit > addresses. Buses expressed with ranges and offsets are one example. > Also, I2C addresses have a 32-bit size in DT yet are only 8-bit and we > don't do leading zero's there. Ok, I'm happily proven wrong here, also by confirming how this is done on "real" OF. According to benh: 15:20 <benh> ojn: 0,0 is not quite right, it's supposed to be used when the two numbers are different things, like device,fn on PCI The same is true for >2^32 unit addresses, they just use the one integer instead of x,y. So, I take back all I've said on this in the last 72 hours. :) It looks like we might need to revisit some of the 32-bit DTs. Simon, drop the series you had. :) -Olof -- 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
diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt index 37b2caf..5ca3f95 100644 --- a/Documentation/devicetree/bindings/arm/arch_timer.txt +++ b/Documentation/devicetree/bindings/arm/arch_timer.txt @@ -17,7 +17,10 @@ to deliver its interrupts via SPIs. - interrupts : Interrupt list for secure, non-secure, virtual and hypervisor timers, in that order. -- clock-frequency : The frequency of the main counter, in Hz. Optional. +- clock-frequency : The frequency of the main counter, in Hz. Should be present + only where necessary to work around BROKEN firmware which does not configure + CNTFRQ on all CPUs to a uniform correct value. Use of this property is + STRONGLY DISCOURAGED; fix your firmware unless absolutely impossible. - always-on : a boolean property. If present, the timer is powered through an always-on power domain, therefore it never loses context. @@ -38,7 +41,8 @@ Example: - compatible : Should at least contain "arm,armv7-timer-mem". -- clock-frequency : The frequency of the main counter, in Hz. Optional. +- clock-frequency : The frequency of the main counter, in Hz. Should be present + only when firmware has not configured the MMIO CNTFRQ registers. - reg : The control frame base address.