| Message ID | 20220503115557.53370-5-phil.edworthy@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add new Renesas RZ/V2M SoC and Renesas RZ/V2M EVK support | expand |
Hi Phil, This is the only patch from this series that I've received, and judging by the CC list this hasn't gone to either LKML or LAKML, so I'm missing the surrounding context for this. Looking on lore, this is part of: https://lore.kernel.org/linux-devicetree/20220503115557.53370-1-phil.edworthy@renesas.com/T/#t ... which is adding support for an arm64 SoC. On Tue, May 03, 2022 at 12:55:49PM +0100, Phil Edworthy wrote: > Some SoCs use a gated clock for the timer and the means to reset the timer. > Hence add these as optional. The clock feeding the architected timer is supposed to be in an always-on clock domain, and is supopsed to be enabled before running any Normal World software. The arm64 kernel *requires* that this is enabled prior to entry. If the kernel ever has to touch either the clock or reset, then there are phases where the counter will not function correctly, which is simply broken. Given that, I do not think this should be in the DT, and instead the clock should be marked as critical in the provider node (and the reset should never be touched). Thanks, Mark. > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > --- > .../devicetree/bindings/timer/arm,arch_timer.yaml | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml b/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml > index df8ce87fd54b..20cd90fc7015 100644 > --- a/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml > +++ b/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml > @@ -64,6 +64,13 @@ properties: > CNTFRQ on all CPUs to a uniform correct value. Use of this property is > strongly discouraged; fix your firmware unless absolutely impossible. > > + clocks: > + description: Optional clock for the timer. > + maxItems: 1 > + > + resets: > + maxItems: 1 > + > always-on: > type: boolean > description: If present, the timer is powered through an always-on power > -- > 2.32.0 >
On 2022-05-03 12:55, Phil Edworthy wrote: > Some SoCs use a gated clock for the timer and the means to reset the > timer. > Hence add these as optional. The architecture is crystal clear on the subject: the counter is in an always-on domain. Why should this be visible to SW? Also, reseting the counter breaks the guaranteed monotonicity we rely on. Worse case, this belongs to the boot firmware, not the kernel, and I don't think this should be described in the DT. M. > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > --- > .../devicetree/bindings/timer/arm,arch_timer.yaml | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git > a/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml > b/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml > index df8ce87fd54b..20cd90fc7015 100644 > --- a/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml > +++ b/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml > @@ -64,6 +64,13 @@ properties: > CNTFRQ on all CPUs to a uniform correct value. Use of this > property is > strongly discouraged; fix your firmware unless absolutely > impossible. > > + clocks: > + description: Optional clock for the timer. > + maxItems: 1 > + > + resets: > + maxItems: 1 > + > always-on: > type: boolean > description: If present, the timer is powered through an always-on > power
Hi Marc, On Tue, May 3, 2022 at 3:12 PM Marc Zyngier <maz@kernel.org> wrote: > On 2022-05-03 12:55, Phil Edworthy wrote: > > Some SoCs use a gated clock for the timer and the means to reset the > > timer. > > Hence add these as optional. > > The architecture is crystal clear on the subject: the counter > is in an always-on domain. Why should this be visible to SW? > Also, reseting the counter breaks the guaranteed monotonicity > we rely on. The DT bindings do state: always-on: type: boolean description: If present, the timer is powered through an always-on power domain, therefore it never loses context. and (surprisingly?) the absence of this property seems to be the norm... And: arm,no-tick-in-suspend: type: boolean description: The main counter does not tick when the system is in low-power system suspend on some SoCs. This behavior does not match the Architecture Reference Manual's specification that the system counter "must be implemented in an always-on power domain." So there's already precedent for clocks that can be disabled. > Worse case, this belongs to the boot firmware, not the kernel, > and I don't think this should be described in the DT. "DT describes hardware, not software policy"? > > --- a/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml > > +++ b/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml > > @@ -64,6 +64,13 @@ properties: > > CNTFRQ on all CPUs to a uniform correct value. Use of this > > property is > > strongly discouraged; fix your firmware unless absolutely > > impossible. > > > > + clocks: > > + description: Optional clock for the timer. > > + maxItems: 1 > > + > > + resets: > > + maxItems: 1 > > + > > always-on: > > type: boolean > > description: If present, the timer is powered through an always-on > > power 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
Hi Geert, On Tue, May 03, 2022 at 04:22:35PM +0200, Geert Uytterhoeven wrote: > On Tue, May 3, 2022 at 3:12 PM Marc Zyngier <maz@kernel.org> wrote: > > On 2022-05-03 12:55, Phil Edworthy wrote: > > > Some SoCs use a gated clock for the timer and the means to reset the > > > timer. > > > Hence add these as optional. > > > > The architecture is crystal clear on the subject: the counter > > is in an always-on domain. Why should this be visible to SW? > > Also, reseting the counter breaks the guaranteed monotonicity > > we rely on. > > The DT bindings do state: > > always-on: > type: boolean > description: If present, the timer is powered through an always-on power > domain, therefore it never loses context. > > and (surprisingly?) the absence of this property seems to be the > norm... That's the *timer* (i.e. the comparator logic within each CPU which fires an interrupt), not the *counter* (i.e. the incrementing value fed by a clock). What this is trying to say is whether that can be relied upon to cause a wakeup while the CPU is in a low-power state, or whether it cannot (and hence SW needs to use another timer for the wakeup). It's legitimate for each timer to not be in an always-on power domain because it is part of each CPU, whereas the counter is global to the system. We can clear up the wording here since it's apparently confusing. > And: > > arm,no-tick-in-suspend: > type: boolean > description: The main counter does not tick when the system is in > low-power system suspend on some SoCs. This behavior does not match the > Architecture Reference Manual's specification that the system > counter "must > be implemented in an always-on power domain." This is admittedly a workaround for an integration bug, but it's quite different and only affects the time jump that can be observed when going into suspend an exiting from it. Whenever software is running the counter is incrementing. > So there's already precedent for clocks that can be disabled. There's precedent for the clock being disabled in a specific deep sleep state, not when SW is actively running. > > Worse case, this belongs to the boot firmware, not the kernel, > > and I don't think this should be described in the DT. > > "DT describes hardware, not software policy"? It's still describing the HW. There's plenty of other always-on stuff that we don't describe because for all intents and purposes it is always on. Note that this being always-on isn't just a Linux thing; that affects plenty of other SW which may run and it's an *architectural property* that's apparently being violated. Thanks, Mark.
Geert, On Tue, 03 May 2022 15:22:35 +0100, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Marc, > > On Tue, May 3, 2022 at 3:12 PM Marc Zyngier <maz@kernel.org> wrote: > > On 2022-05-03 12:55, Phil Edworthy wrote: > > > Some SoCs use a gated clock for the timer and the means to reset the > > > timer. > > > Hence add these as optional. > > > > The architecture is crystal clear on the subject: the counter > > is in an always-on domain. Why should this be visible to SW? > > Also, reseting the counter breaks the guaranteed monotonicity > > we rely on. > > The DT bindings do state: > > always-on: > type: boolean > description: If present, the timer is powered through an always-on power > domain, therefore it never loses context. > > and (surprisingly?) the absence of this property seems to be the > norm... *timer* is the key word. And counter != timer. What your HW has is a gate on the *counter* which is illegal if observable from NS SW. > > And: > > arm,no-tick-in-suspend: > type: boolean > description: The main counter does not tick when the system is in > low-power system suspend on some SoCs. This behavior does not match the > Architecture Reference Manual's specification that the system > counter "must > be implemented in an always-on power domain." > > So there's already precedent for clocks that can be disabled. No, this is only the case in *suspend*, as the name of the property vaguely hints at. And that's a property for a bug. In your case, the clock can be controlled arbitrarily, which is even worse. > > > Worse case, this belongs to the boot firmware, not the kernel, > > and I don't think this should be described in the DT. > > "DT describes hardware, not software policy"? I'm happy to spread "always-on" properties all over the shop, but that's not helping. The HW spec says it in bold letters: the counter is always running, and doesn't jump backward. I can't imagine how secure SW will behave when you reset its counter... :-/ M.
Hi Marc and Mark, On 03 May 2022 16:57 Marc Zyngier wrote: > On Tue, 03 May 2022 15:22:35 +0100, Geert Uytterhoeven wrote: > > On Tue, May 3, 2022 at 3:12 PM Marc Zyngier wrote: > > > On 2022-05-03 12:55, Phil Edworthy wrote: > > > > Some SoCs use a gated clock for the timer and the means to reset the > > > > timer. > > > > Hence add these as optional. > > > > > > The architecture is crystal clear on the subject: the counter > > > is in an always-on domain. Why should this be visible to SW? > > > Also, reseting the counter breaks the guaranteed monotonicity > > > we rely on. > > > > The DT bindings do state: > > > > always-on: > > type: boolean > > description: If present, the timer is powered through an always-on > power > > domain, therefore it never loses context. > > > > and (surprisingly?) the absence of this property seems to be the > > norm... > > *timer* is the key word. And counter != timer. What your HW has is a > gate on the *counter* which is illegal if observable from NS SW. Ok, thanks for your feedback. We'll pretend this clock gate and reset doesn't exist and drop this patch. > > > > And: > > > > arm,no-tick-in-suspend: > > type: boolean > > description: The main counter does not tick when the system is in > > low-power system suspend on some SoCs. This behavior does not > match the > > Architecture Reference Manual's specification that the system > > counter "must > > be implemented in an always-on power domain." > > > > So there's already precedent for clocks that can be disabled. > > No, this is only the case in *suspend*, as the name of the property > vaguely hints at. And that's a property for a bug. In your case, the > clock can be controlled arbitrarily, which is even worse. > > > > > > Worse case, this belongs to the boot firmware, not the kernel, > > > and I don't think this should be described in the DT. > > > > "DT describes hardware, not software policy"? > > I'm happy to spread "always-on" properties all over the shop, but > that's not helping. The HW spec says it in bold letters: the counter > is always running, and doesn't jump backward. I can't imagine how > secure SW will behave when you reset its counter... :-/ Thanks Phil
On Tue, May 03, 2022 at 02:11:59PM +0100, Mark Rutland wrote: > Hi Phil, > > This is the only patch from this series that I've received, and judging > by the CC list this hasn't gone to either LKML or LAKML, so I'm missing > the surrounding context for this. > > Looking on lore, this is part of: > > https://lore.kernel.org/linux-devicetree/20220503115557.53370-1-phil.edworthy@renesas.com/T/#t > > ... which is adding support for an arm64 SoC. > > On Tue, May 03, 2022 at 12:55:49PM +0100, Phil Edworthy wrote: > > Some SoCs use a gated clock for the timer and the means to reset the timer. > > Hence add these as optional. > > The clock feeding the architected timer is supposed to be in an > always-on clock domain, and is supopsed to be enabled before running any > Normal World software. > > The arm64 kernel *requires* that this is enabled prior to entry. If the > kernel ever has to touch either the clock or reset, then there are > phases where the counter will not function correctly, which is simply > broken. > > Given that, I do not think this should be in the DT, and instead the > clock should be marked as critical in the provider node (and the reset > should never be touched). That is not yet an accepted DT property, but is currently on the list for review[1]. If that's something people need, chime in. More than 1 person needing something is always better. Rob [1] https://lore.kernel.org/all/20220428110107.149524-1-marex@denx.de/
Hi Rob, On Thu, May 5, 2022 at 12:33 AM Rob Herring <robh@kernel.org> wrote: > On Tue, May 03, 2022 at 02:11:59PM +0100, Mark Rutland wrote: > > This is the only patch from this series that I've received, and judging > > by the CC list this hasn't gone to either LKML or LAKML, so I'm missing > > the surrounding context for this. > > > > Looking on lore, this is part of: > > > > https://lore.kernel.org/linux-devicetree/20220503115557.53370-1-phil.edworthy@renesas.com/T/#t > > > > ... which is adding support for an arm64 SoC. > > > > On Tue, May 03, 2022 at 12:55:49PM +0100, Phil Edworthy wrote: > > > Some SoCs use a gated clock for the timer and the means to reset the timer. > > > Hence add these as optional. > > > > The clock feeding the architected timer is supposed to be in an > > always-on clock domain, and is supopsed to be enabled before running any > > Normal World software. > > > > The arm64 kernel *requires* that this is enabled prior to entry. If the > > kernel ever has to touch either the clock or reset, then there are > > phases where the counter will not function correctly, which is simply > > broken. > > > > Given that, I do not think this should be in the DT, and instead the > > clock should be marked as critical in the provider node (and the reset > > should never be touched). > > That is not yet an accepted DT property, but is currently on the list > for review[1]. If that's something people need, chime in. More than 1 > person needing something is always better. I am aware of[1]. AFAIU, that is meant for clocks that need to stay enabled for external reasons (external hardware driven by on-SoC clock). For internal reasons (e.g. arch-timer), CLK_IS_CRITICAL is fine. > [1] https://lore.kernel.org/all/20220428110107.149524-1-marex@denx.de/ 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
diff --git a/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml b/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml index df8ce87fd54b..20cd90fc7015 100644 --- a/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml +++ b/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml @@ -64,6 +64,13 @@ properties: CNTFRQ on all CPUs to a uniform correct value. Use of this property is strongly discouraged; fix your firmware unless absolutely impossible. + clocks: + description: Optional clock for the timer. + maxItems: 1 + + resets: + maxItems: 1 + always-on: type: boolean description: If present, the timer is powered through an always-on power
Some SoCs use a gated clock for the timer and the means to reset the timer. Hence add these as optional. Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> --- .../devicetree/bindings/timer/arm,arch_timer.yaml | 7 +++++++ 1 file changed, 7 insertions(+)