From: Doug Anderson <[email protected]>
Some 32-bit (ARMv7) systems are architected like this:
* The firmware doesn't know and doesn't care about hypervisor mode and
we don't want to add the complexity of hypervisor there.
* The firmware isn't involved in SMP bringup or resume.
* The ARCH timer come up with an uninitialized offset (CNTVOFF)
between the virtual and physical counters. Each core gets a
different random offset.
* The device boots in "Secure SVC" mode.
* Nothing has touched the reset value of CNTHCTL.PL1PCEN or
CNTHCTL.PL1PCTEN (both default to 1 at reset)
On systems like the above, it doesn't make sense to use the virtual
counter. There's nobody managing the offset and each time a core goes
down and comes back up it will get reinitialized to some other random
value.
This adds an optional property which can inform the kernel of this
situation, and firmware is free to remove the property if it is going
to initialize the CNTVOFF registers when each CPU comes out of reset.
Currently, the best course of action in this case is to use the
physical timer, which is why it is important that CNTHCTL hasn't been
changed from its reset value and it's a reasonable assumption given
that the firmware has never entered HYP mode.
Note that it's been said that on ARMv8 systems the firmware and
kernel really can't be architected as described above. That means
using the physical timer like this really only makes sense for ARMv7
systems.
Signed-off-by: Doug Anderson <[email protected]>
Signed-off-by: Sonny Rao <[email protected]>
Reviewed-by: Mark Rutland <[email protected]>
---
Changes in v2:
- Add "#ifdef CONFIG_ARM" as per Will Deacon
Changes in v3:
- change property name to arm,cntvoff-not-fw-configured and specify
that the value of CNTHCTL.PL1PC(T)EN must still be the reset value
of 1 as per Mark Rutland
Changes in v4:
- change property name to arm,cpu-registers-not-fw-configured and
specify that all cpu registers must have architected reset values
per Mark Rutland
- change from "#ifdef CONFIG_ARM" to "if (IS_ENABLED(CONFIG_ARM))" per
Arnd Bergmann
---
Documentation/devicetree/bindings/arm/arch_timer.txt | 8 ++++++++
drivers/clocksource/arm_arch_timer.c | 8 ++++++++
2 files changed, 16 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt
index 37b2caf..256b4d8 100644
--- a/Documentation/devicetree/bindings/arm/arch_timer.txt
+++ b/Documentation/devicetree/bindings/arm/arch_timer.txt
@@ -22,6 +22,14 @@ to deliver its interrupts via SPIs.
- always-on : a boolean property. If present, the timer is powered through an
always-on power domain, therefore it never loses context.
+** Optional properties:
+
+- arm,cpu-registers-not-fw-configured : Firmware does not initialize
+ any of the generic timer CPU registers, which contain their
+ architecturally-defined reset values. Only supported for 32-bit
+ systems which follow the ARMv7 architected reset values.
+
+
Example:
timer {
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 8daf056..799139f 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -654,6 +654,14 @@ static void __init arch_timer_init(struct device_node *np)
arch_timer_detect_rate(NULL, np);
/*
+ * If we cannot rely on firmware initializing the timer registers then
+ * we should use the physical timers instead.
+ */
+ if (IS_ENABLED(CONFIG_ARM) &&
+ of_property_read_bool(np, "arm,cpu-registers-not-fw-configured"))
+ arch_timer_use_virtual = false;
+
+ /*
* If HYP mode is available, we know that the physical timer
* has been configured to be accessible from PL1. Use it, so
* that a guest can use the virtual timer instead.
--
1.8.3.2
Daniel,
On Wed, Oct 8, 2014 at 12:33 AM, Sonny Rao <[email protected]> wrote:
> From: Doug Anderson <[email protected]>
>
> Some 32-bit (ARMv7) systems are architected like this:
>
> * The firmware doesn't know and doesn't care about hypervisor mode and
> we don't want to add the complexity of hypervisor there.
>
> * The firmware isn't involved in SMP bringup or resume.
>
> * The ARCH timer come up with an uninitialized offset (CNTVOFF)
> between the virtual and physical counters. Each core gets a
> different random offset.
>
> * The device boots in "Secure SVC" mode.
>
> * Nothing has touched the reset value of CNTHCTL.PL1PCEN or
> CNTHCTL.PL1PCTEN (both default to 1 at reset)
>
> On systems like the above, it doesn't make sense to use the virtual
> counter. There's nobody managing the offset and each time a core goes
> down and comes back up it will get reinitialized to some other random
> value.
>
> This adds an optional property which can inform the kernel of this
> situation, and firmware is free to remove the property if it is going
> to initialize the CNTVOFF registers when each CPU comes out of reset.
>
> Currently, the best course of action in this case is to use the
> physical timer, which is why it is important that CNTHCTL hasn't been
> changed from its reset value and it's a reasonable assumption given
> that the firmware has never entered HYP mode.
>
> Note that it's been said that on ARMv8 systems the firmware and
> kernel really can't be architected as described above. That means
> using the physical timer like this really only makes sense for ARMv7
> systems.
>
> Signed-off-by: Doug Anderson <[email protected]>
> Signed-off-by: Sonny Rao <[email protected]>
> Reviewed-by: Mark Rutland <[email protected]>
> ---
> Changes in v2:
> - Add "#ifdef CONFIG_ARM" as per Will Deacon
>
> Changes in v3:
> - change property name to arm,cntvoff-not-fw-configured and specify
> that the value of CNTHCTL.PL1PC(T)EN must still be the reset value
> of 1 as per Mark Rutland
>
> Changes in v4:
> - change property name to arm,cpu-registers-not-fw-configured and
> specify that all cpu registers must have architected reset values
> per Mark Rutland
> - change from "#ifdef CONFIG_ARM" to "if (IS_ENABLED(CONFIG_ARM))" per
> Arnd Bergmann
> ---
> Documentation/devicetree/bindings/arm/arch_timer.txt | 8 ++++++++
> drivers/clocksource/arm_arch_timer.c | 8 ++++++++
> 2 files changed, 16 insertions(+)
Do you know what the status of this patch is? This patch and Sonny's
patch at <https://patchwork.kernel.org/patch/5051901/> are needed on
Rockchip rk3288 for some specific things:
1. To make SMP happy with coreboot.
2. To (I assume) make SMP happy after S2R (no matter which firmware is
used since I don't think anyone has PSCI for rk3288).
...we still need a DTS entry atop these patches, but that's trivial to
add once these land.
Thanks!
On 11/20/2014 12:01 AM, Doug Anderson wrote:
> Daniel,
>
> On Wed, Oct 8, 2014 at 12:33 AM, Sonny Rao <[email protected]> wrote:
>> From: Doug Anderson <[email protected]>
>>
>> Some 32-bit (ARMv7) systems are architected like this:
>>
>> * The firmware doesn't know and doesn't care about hypervisor mode and
>> we don't want to add the complexity of hypervisor there.
>>
>> * The firmware isn't involved in SMP bringup or resume.
>>
>> * The ARCH timer come up with an uninitialized offset (CNTVOFF)
>> between the virtual and physical counters. Each core gets a
>> different random offset.
>>
>> * The device boots in "Secure SVC" mode.
>>
>> * Nothing has touched the reset value of CNTHCTL.PL1PCEN or
>> CNTHCTL.PL1PCTEN (both default to 1 at reset)
>>
>> On systems like the above, it doesn't make sense to use the virtual
>> counter. There's nobody managing the offset and each time a core goes
>> down and comes back up it will get reinitialized to some other random
>> value.
>>
>> This adds an optional property which can inform the kernel of this
>> situation, and firmware is free to remove the property if it is going
>> to initialize the CNTVOFF registers when each CPU comes out of reset.
>>
>> Currently, the best course of action in this case is to use the
>> physical timer, which is why it is important that CNTHCTL hasn't been
>> changed from its reset value and it's a reasonable assumption given
>> that the firmware has never entered HYP mode.
>>
>> Note that it's been said that on ARMv8 systems the firmware and
>> kernel really can't be architected as described above. That means
>> using the physical timer like this really only makes sense for ARMv7
>> systems.
>>
>> Signed-off-by: Doug Anderson <[email protected]>
>> Signed-off-by: Sonny Rao <[email protected]>
>> Reviewed-by: Mark Rutland <[email protected]>
>> ---
>> Changes in v2:
>> - Add "#ifdef CONFIG_ARM" as per Will Deacon
>>
>> Changes in v3:
>> - change property name to arm,cntvoff-not-fw-configured and specify
>> that the value of CNTHCTL.PL1PC(T)EN must still be the reset value
>> of 1 as per Mark Rutland
>>
>> Changes in v4:
>> - change property name to arm,cpu-registers-not-fw-configured and
>> specify that all cpu registers must have architected reset values
>> per Mark Rutland
>> - change from "#ifdef CONFIG_ARM" to "if (IS_ENABLED(CONFIG_ARM))" per
>> Arnd Bergmann
>> ---
>> Documentation/devicetree/bindings/arm/arch_timer.txt | 8 ++++++++
>> drivers/clocksource/arm_arch_timer.c | 8 ++++++++
>> 2 files changed, 16 insertions(+)
>
> Do you know what the status of this patch is? This patch and Sonny's
> patch at <https://patchwork.kernel.org/patch/5051901/> are needed on
> Rockchip rk3288 for some specific things:
>
> 1. To make SMP happy with coreboot.
>
> 2. To (I assume) make SMP happy after S2R (no matter which firmware is
> used since I don't think anyone has PSCI for rk3288).
>
> ...we still need a DTS entry atop these patches, but that's trivial to
> add once these land.
Hi Doug,
sounds like I forgot it. Thanks for the heads up.
Applied for 3.19.
-- Daniel
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Hi Doug, Olof,
IIUC, it sounds like this patch is needed from some other patches in
arm-soc. Olof was proposing to take this patch through its tree to
facilitate the integration.
Olof, is it this patch you were worried about ?
Thanks
-- Daniel
On 11/20/2014 12:01 AM, Doug Anderson wrote:
> Daniel,
>
> On Wed, Oct 8, 2014 at 12:33 AM, Sonny Rao <[email protected]> wrote:
>> From: Doug Anderson <[email protected]>
>>
>> Some 32-bit (ARMv7) systems are architected like this:
>>
>> * The firmware doesn't know and doesn't care about hypervisor mode and
>> we don't want to add the complexity of hypervisor there.
>>
>> * The firmware isn't involved in SMP bringup or resume.
>>
>> * The ARCH timer come up with an uninitialized offset (CNTVOFF)
>> between the virtual and physical counters. Each core gets a
>> different random offset.
>>
>> * The device boots in "Secure SVC" mode.
>>
>> * Nothing has touched the reset value of CNTHCTL.PL1PCEN or
>> CNTHCTL.PL1PCTEN (both default to 1 at reset)
>>
>> On systems like the above, it doesn't make sense to use the virtual
>> counter. There's nobody managing the offset and each time a core goes
>> down and comes back up it will get reinitialized to some other random
>> value.
>>
>> This adds an optional property which can inform the kernel of this
>> situation, and firmware is free to remove the property if it is going
>> to initialize the CNTVOFF registers when each CPU comes out of reset.
>>
>> Currently, the best course of action in this case is to use the
>> physical timer, which is why it is important that CNTHCTL hasn't been
>> changed from its reset value and it's a reasonable assumption given
>> that the firmware has never entered HYP mode.
>>
>> Note that it's been said that on ARMv8 systems the firmware and
>> kernel really can't be architected as described above. That means
>> using the physical timer like this really only makes sense for ARMv7
>> systems.
>>
>> Signed-off-by: Doug Anderson <[email protected]>
>> Signed-off-by: Sonny Rao <[email protected]>
>> Reviewed-by: Mark Rutland <[email protected]>
>> ---
>> Changes in v2:
>> - Add "#ifdef CONFIG_ARM" as per Will Deacon
>>
>> Changes in v3:
>> - change property name to arm,cntvoff-not-fw-configured and specify
>> that the value of CNTHCTL.PL1PC(T)EN must still be the reset value
>> of 1 as per Mark Rutland
>>
>> Changes in v4:
>> - change property name to arm,cpu-registers-not-fw-configured and
>> specify that all cpu registers must have architected reset values
>> per Mark Rutland
>> - change from "#ifdef CONFIG_ARM" to "if (IS_ENABLED(CONFIG_ARM))" per
>> Arnd Bergmann
>> ---
>> Documentation/devicetree/bindings/arm/arch_timer.txt | 8 ++++++++
>> drivers/clocksource/arm_arch_timer.c | 8 ++++++++
>> 2 files changed, 16 insertions(+)
>
> Do you know what the status of this patch is? This patch and Sonny's
> patch at <https://patchwork.kernel.org/patch/5051901/> are needed on
> Rockchip rk3288 for some specific things:
>
> 1. To make SMP happy with coreboot.
>
> 2. To (I assume) make SMP happy after S2R (no matter which firmware is
> used since I don't think anyone has PSCI for rk3288).
>
> ...we still need a DTS entry atop these patches, but that's trivial to
> add once these land.
>
> Thanks!
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Hi Daniel,
Am Mittwoch, 26. November 2014, 12:51:08 schrieb Daniel Lezcano:
> Hi Doug, Olof,
>
> IIUC, it sounds like this patch is needed from some other patches in
> arm-soc. Olof was proposing to take this patch through its tree to
> facilitate the integration.
>
> Olof, is it this patch you were worried about ?
I think this is one of two patches in question.
"clocksource: arch_timer: Fix code to use physical timers when requested" [0]
would be the second one.
And the patch for arm-soc that Olof means would be "ARM: dts: rk3288: add
arm,cpu-registers-not-fw-configured" [1].
Heiko
[0]
https://git.linaro.org/people/daniel.lezcano/linux.git/commit/04f71c2cae54dc26b2a236c787ea8d56c174150b
[1] https://lkml.org/lkml/2014/11/25/975
>
> Thanks
>
> -- Daniel
>
> On 11/20/2014 12:01 AM, Doug Anderson wrote:
> > Daniel,
> >
> > On Wed, Oct 8, 2014 at 12:33 AM, Sonny Rao <[email protected]> wrote:
> >> From: Doug Anderson <[email protected]>
> >>
> >> Some 32-bit (ARMv7) systems are architected like this:
> >>
> >> * The firmware doesn't know and doesn't care about hypervisor mode and
> >>
> >> we don't want to add the complexity of hypervisor there.
> >>
> >> * The firmware isn't involved in SMP bringup or resume.
> >>
> >> * The ARCH timer come up with an uninitialized offset (CNTVOFF)
> >>
> >> between the virtual and physical counters. Each core gets a
> >> different random offset.
> >>
> >> * The device boots in "Secure SVC" mode.
> >>
> >> * Nothing has touched the reset value of CNTHCTL.PL1PCEN or
> >>
> >> CNTHCTL.PL1PCTEN (both default to 1 at reset)
> >>
> >> On systems like the above, it doesn't make sense to use the virtual
> >> counter. There's nobody managing the offset and each time a core goes
> >> down and comes back up it will get reinitialized to some other random
> >> value.
> >>
> >> This adds an optional property which can inform the kernel of this
> >> situation, and firmware is free to remove the property if it is going
> >> to initialize the CNTVOFF registers when each CPU comes out of reset.
> >>
> >> Currently, the best course of action in this case is to use the
> >> physical timer, which is why it is important that CNTHCTL hasn't been
> >> changed from its reset value and it's a reasonable assumption given
> >> that the firmware has never entered HYP mode.
> >>
> >> Note that it's been said that on ARMv8 systems the firmware and
> >> kernel really can't be architected as described above. That means
> >> using the physical timer like this really only makes sense for ARMv7
> >> systems.
> >>
> >> Signed-off-by: Doug Anderson <[email protected]>
> >> Signed-off-by: Sonny Rao <[email protected]>
> >> Reviewed-by: Mark Rutland <[email protected]>
> >> ---
> >> Changes in v2:
> >> - Add "#ifdef CONFIG_ARM" as per Will Deacon
> >>
> >> Changes in v3:
> >> - change property name to arm,cntvoff-not-fw-configured and specify
> >>
> >> that the value of CNTHCTL.PL1PC(T)EN must still be the reset value
> >> of 1 as per Mark Rutland
> >>
> >> Changes in v4:
> >> - change property name to arm,cpu-registers-not-fw-configured and
> >>
> >> specify that all cpu registers must have architected reset values
> >> per Mark Rutland
> >>
> >> - change from "#ifdef CONFIG_ARM" to "if (IS_ENABLED(CONFIG_ARM))" per
> >>
> >> Arnd Bergmann
> >>
> >> ---
> >>
> >> Documentation/devicetree/bindings/arm/arch_timer.txt | 8 ++++++++
> >> drivers/clocksource/arm_arch_timer.c | 8 ++++++++
> >> 2 files changed, 16 insertions(+)
> >
> > Do you know what the status of this patch is? This patch and Sonny's
> > patch at <https://patchwork.kernel.org/patch/5051901/> are needed on
> > Rockchip rk3288 for some specific things:
> >
> > 1. To make SMP happy with coreboot.
> >
> > 2. To (I assume) make SMP happy after S2R (no matter which firmware is
> > used since I don't think anyone has PSCI for rk3288).
> >
> > ...we still need a DTS entry atop these patches, but that's trivial to
> > add once these land.
> >
> > Thanks!
On 11/26/2014 01:06 PM, Heiko Stübner wrote:
> Hi Daniel,
>
> Am Mittwoch, 26. November 2014, 12:51:08 schrieb Daniel Lezcano:
>> Hi Doug, Olof,
>>
>> IIUC, it sounds like this patch is needed from some other patches in
>> arm-soc. Olof was proposing to take this patch through its tree to
>> facilitate the integration.
>>
>> Olof, is it this patch you were worried about ?
>
> I think this is one of two patches in question.
>
> "clocksource: arch_timer: Fix code to use physical timers when requested" [0]
> would be the second one.
>
> And the patch for arm-soc that Olof means would be "ARM: dts: rk3288: add
> arm,cpu-registers-not-fw-configured" [1].
Ok, so IIUC, "clocksource: arch_timer: Fix code to use physical timers
when requested" should go via arm's tree, right ?
> Heiko
>
> [0]
> https://git.linaro.org/people/daniel.lezcano/linux.git/commit/04f71c2cae54dc26b2a236c787ea8d56c174150b
> [1] https://lkml.org/lkml/2014/11/25/975
>
>>
>> Thanks
>>
>> -- Daniel
>>
>> On 11/20/2014 12:01 AM, Doug Anderson wrote:
>>> Daniel,
>>>
>>> On Wed, Oct 8, 2014 at 12:33 AM, Sonny Rao <[email protected]> wrote:
>>>> From: Doug Anderson <[email protected]>
>>>>
>>>> Some 32-bit (ARMv7) systems are architected like this:
>>>>
>>>> * The firmware doesn't know and doesn't care about hypervisor mode and
>>>>
>>>> we don't want to add the complexity of hypervisor there.
>>>>
>>>> * The firmware isn't involved in SMP bringup or resume.
>>>>
>>>> * The ARCH timer come up with an uninitialized offset (CNTVOFF)
>>>>
>>>> between the virtual and physical counters. Each core gets a
>>>> different random offset.
>>>>
>>>> * The device boots in "Secure SVC" mode.
>>>>
>>>> * Nothing has touched the reset value of CNTHCTL.PL1PCEN or
>>>>
>>>> CNTHCTL.PL1PCTEN (both default to 1 at reset)
>>>>
>>>> On systems like the above, it doesn't make sense to use the virtual
>>>> counter. There's nobody managing the offset and each time a core goes
>>>> down and comes back up it will get reinitialized to some other random
>>>> value.
>>>>
>>>> This adds an optional property which can inform the kernel of this
>>>> situation, and firmware is free to remove the property if it is going
>>>> to initialize the CNTVOFF registers when each CPU comes out of reset.
>>>>
>>>> Currently, the best course of action in this case is to use the
>>>> physical timer, which is why it is important that CNTHCTL hasn't been
>>>> changed from its reset value and it's a reasonable assumption given
>>>> that the firmware has never entered HYP mode.
>>>>
>>>> Note that it's been said that on ARMv8 systems the firmware and
>>>> kernel really can't be architected as described above. That means
>>>> using the physical timer like this really only makes sense for ARMv7
>>>> systems.
>>>>
>>>> Signed-off-by: Doug Anderson <[email protected]>
>>>> Signed-off-by: Sonny Rao <[email protected]>
>>>> Reviewed-by: Mark Rutland <[email protected]>
>>>> ---
>>>> Changes in v2:
>>>> - Add "#ifdef CONFIG_ARM" as per Will Deacon
>>>>
>>>> Changes in v3:
>>>> - change property name to arm,cntvoff-not-fw-configured and specify
>>>>
>>>> that the value of CNTHCTL.PL1PC(T)EN must still be the reset value
>>>> of 1 as per Mark Rutland
>>>>
>>>> Changes in v4:
>>>> - change property name to arm,cpu-registers-not-fw-configured and
>>>>
>>>> specify that all cpu registers must have architected reset values
>>>> per Mark Rutland
>>>>
>>>> - change from "#ifdef CONFIG_ARM" to "if (IS_ENABLED(CONFIG_ARM))" per
>>>>
>>>> Arnd Bergmann
>>>>
>>>> ---
>>>>
>>>> Documentation/devicetree/bindings/arm/arch_timer.txt | 8 ++++++++
>>>> drivers/clocksource/arm_arch_timer.c | 8 ++++++++
>>>> 2 files changed, 16 insertions(+)
>>>
>>> Do you know what the status of this patch is? This patch and Sonny's
>>> patch at <https://patchwork.kernel.org/patch/5051901/> are needed on
>>> Rockchip rk3288 for some specific things:
>>>
>>> 1. To make SMP happy with coreboot.
>>>
>>> 2. To (I assume) make SMP happy after S2R (no matter which firmware is
>>> used since I don't think anyone has PSCI for rk3288).
>>>
>>> ...we still need a DTS entry atop these patches, but that's trivial to
>>> add once these land.
>>>
>>> Thanks!
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Am Mittwoch, 26. November 2014, 13:30:58 schrieb Daniel Lezcano:
> On 11/26/2014 01:06 PM, Heiko St?bner wrote:
> > Hi Daniel,
> >
> > Am Mittwoch, 26. November 2014, 12:51:08 schrieb Daniel Lezcano:
> >> Hi Doug, Olof,
> >>
> >> IIUC, it sounds like this patch is needed from some other patches in
> >> arm-soc. Olof was proposing to take this patch through its tree to
> >> facilitate the integration.
> >>
> >> Olof, is it this patch you were worried about ?
> >
> > I think this is one of two patches in question.
> >
> > "clocksource: arch_timer: Fix code to use physical timers when requested"
> > [0] would be the second one.
> >
> > And the patch for arm-soc that Olof means would be "ARM: dts: rk3288: add
> > arm,cpu-registers-not-fw-configured" [1].
>
> Ok, so IIUC, "clocksource: arch_timer: Fix code to use physical timers
> when requested" should go via arm's tree, right ?
If I'm reading Olof's irc-comments from yesterday correctly, that is right and
the 3 patches should go in together:
- "clocksource: arch_timer: Fix code to use physical timers
when requested" fixes the use of physical timers in general
- "clocksource: arch_timer: Allow the device tree to specify uninitialized
timer registers" allows this to be set from dt
- "ARM: dts: rk3288: add arm,cpu-registers-not-fw-configured" enables this on
rk3288
Heiko
On 10/08/2014 09:33 AM, Sonny Rao wrote:
> From: Doug Anderson <[email protected]>
>
> Some 32-bit (ARMv7) systems are architected like this:
>
> * The firmware doesn't know and doesn't care about hypervisor mode and
> we don't want to add the complexity of hypervisor there.
>
> * The firmware isn't involved in SMP bringup or resume.
>
> * The ARCH timer come up with an uninitialized offset (CNTVOFF)
> between the virtual and physical counters. Each core gets a
> different random offset.
>
> * The device boots in "Secure SVC" mode.
>
> * Nothing has touched the reset value of CNTHCTL.PL1PCEN or
> CNTHCTL.PL1PCTEN (both default to 1 at reset)
>
> On systems like the above, it doesn't make sense to use the virtual
> counter. There's nobody managing the offset and each time a core goes
> down and comes back up it will get reinitialized to some other random
> value.
>
> This adds an optional property which can inform the kernel of this
> situation, and firmware is free to remove the property if it is going
> to initialize the CNTVOFF registers when each CPU comes out of reset.
>
> Currently, the best course of action in this case is to use the
> physical timer, which is why it is important that CNTHCTL hasn't been
> changed from its reset value and it's a reasonable assumption given
> that the firmware has never entered HYP mode.
>
> Note that it's been said that on ARMv8 systems the firmware and
> kernel really can't be architected as described above. That means
> using the physical timer like this really only makes sense for ARMv7
> systems.
>
> Signed-off-by: Doug Anderson <[email protected]>
> Signed-off-by: Sonny Rao <[email protected]>
> Reviewed-by: Mark Rutland <[email protected]>
Acked-by: Daniel Lezcano <[email protected]>
I would be nice to have Catalin's ack.
Thanks
-- Daniel
> ---
> Changes in v2:
> - Add "#ifdef CONFIG_ARM" as per Will Deacon
>
> Changes in v3:
> - change property name to arm,cntvoff-not-fw-configured and specify
> that the value of CNTHCTL.PL1PC(T)EN must still be the reset value
> of 1 as per Mark Rutland
>
> Changes in v4:
> - change property name to arm,cpu-registers-not-fw-configured and
> specify that all cpu registers must have architected reset values
> per Mark Rutland
> - change from "#ifdef CONFIG_ARM" to "if (IS_ENABLED(CONFIG_ARM))" per
> Arnd Bergmann
> ---
> Documentation/devicetree/bindings/arm/arch_timer.txt | 8 ++++++++
> drivers/clocksource/arm_arch_timer.c | 8 ++++++++
> 2 files changed, 16 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt
> index 37b2caf..256b4d8 100644
> --- a/Documentation/devicetree/bindings/arm/arch_timer.txt
> +++ b/Documentation/devicetree/bindings/arm/arch_timer.txt
> @@ -22,6 +22,14 @@ to deliver its interrupts via SPIs.
> - always-on : a boolean property. If present, the timer is powered through an
> always-on power domain, therefore it never loses context.
>
> +** Optional properties:
> +
> +- arm,cpu-registers-not-fw-configured : Firmware does not initialize
> + any of the generic timer CPU registers, which contain their
> + architecturally-defined reset values. Only supported for 32-bit
> + systems which follow the ARMv7 architected reset values.
> +
> +
> Example:
>
> timer {
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 8daf056..799139f 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -654,6 +654,14 @@ static void __init arch_timer_init(struct device_node *np)
> arch_timer_detect_rate(NULL, np);
>
> /*
> + * If we cannot rely on firmware initializing the timer registers then
> + * we should use the physical timers instead.
> + */
> + if (IS_ENABLED(CONFIG_ARM) &&
> + of_property_read_bool(np, "arm,cpu-registers-not-fw-configured"))
> + arch_timer_use_virtual = false;
> +
> + /*
> * If HYP mode is available, we know that the physical timer
> * has been configured to be accessible from PL1. Use it, so
> * that a guest can use the virtual timer instead.
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 11/26/2014 01:48 PM, Heiko Stübner wrote:
> Am Mittwoch, 26. November 2014, 13:30:58 schrieb Daniel Lezcano:
>> On 11/26/2014 01:06 PM, Heiko Stübner wrote:
>>> Hi Daniel,
>>>
>>> Am Mittwoch, 26. November 2014, 12:51:08 schrieb Daniel Lezcano:
>>>> Hi Doug, Olof,
>>>>
>>>> IIUC, it sounds like this patch is needed from some other patches in
>>>> arm-soc. Olof was proposing to take this patch through its tree to
>>>> facilitate the integration.
>>>>
>>>> Olof, is it this patch you were worried about ?
>>>
>>> I think this is one of two patches in question.
>>>
>>> "clocksource: arch_timer: Fix code to use physical timers when requested"
>>> [0] would be the second one.
>>>
>>> And the patch for arm-soc that Olof means would be "ARM: dts: rk3288: add
>>> arm,cpu-registers-not-fw-configured" [1].
>>
>> Ok, so IIUC, "clocksource: arch_timer: Fix code to use physical timers
>> when requested" should go via arm's tree, right ?
>
> If I'm reading Olof's irc-comments from yesterday correctly, that is right and
> the 3 patches should go in together:
>
> - "clocksource: arch_timer: Fix code to use physical timers
> when requested" fixes the use of physical timers in general
> - "clocksource: arch_timer: Allow the device tree to specify uninitialized
> timer registers" allows this to be set from dt
> - "ARM: dts: rk3288: add arm,cpu-registers-not-fw-configured" enables this on
> rk3288
Ok, then I drop them from my tree and will let Olof to handle them.
Thanks !
-- Daniel
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Am Mittwoch, 26. November 2014, 13:49:57 schrieb Daniel Lezcano:
> On 11/26/2014 01:48 PM, Heiko St?bner wrote:
> > Am Mittwoch, 26. November 2014, 13:30:58 schrieb Daniel Lezcano:
> >> On 11/26/2014 01:06 PM, Heiko St?bner wrote:
> >>> Hi Daniel,
> >>>
> >>> Am Mittwoch, 26. November 2014, 12:51:08 schrieb Daniel Lezcano:
> >>>> Hi Doug, Olof,
> >>>>
> >>>> IIUC, it sounds like this patch is needed from some other patches in
> >>>> arm-soc. Olof was proposing to take this patch through its tree to
> >>>> facilitate the integration.
> >>>>
> >>>> Olof, is it this patch you were worried about ?
> >>>
> >>> I think this is one of two patches in question.
> >>>
> >>> "clocksource: arch_timer: Fix code to use physical timers when
> >>> requested"
> >>> [0] would be the second one.
> >>>
> >>> And the patch for arm-soc that Olof means would be "ARM: dts: rk3288:
> >>> add
> >>> arm,cpu-registers-not-fw-configured" [1].
> >>
> >> Ok, so IIUC, "clocksource: arch_timer: Fix code to use physical timers
> >> when requested" should go via arm's tree, right ?
> >
> > If I'm reading Olof's irc-comments from yesterday correctly, that is right
> > and the 3 patches should go in together:
> >
> > - "clocksource: arch_timer: Fix code to use physical timers
> >
> > when requested" fixes the use of physical timers in general
> >
> > - "clocksource: arch_timer: Allow the device tree to specify uninitialized
> >
> > timer registers" allows this to be set from dt
> >
> > - "ARM: dts: rk3288: add arm,cpu-registers-not-fw-configured" enables this
> > on>
> > rk3288
>
> Ok, then I drop them from my tree and will let Olof to handle them.
But maybe you could give them an Ack :-)
Heiko
On 11/26/2014 01:55 PM, Heiko Stübner wrote:
> Am Mittwoch, 26. November 2014, 13:49:57 schrieb Daniel Lezcano:
>> On 11/26/2014 01:48 PM, Heiko Stübner wrote:
>>> Am Mittwoch, 26. November 2014, 13:30:58 schrieb Daniel Lezcano:
>>>> On 11/26/2014 01:06 PM, Heiko Stübner wrote:
>>>>> Hi Daniel,
>>>>>
>>>>> Am Mittwoch, 26. November 2014, 12:51:08 schrieb Daniel Lezcano:
>>>>>> Hi Doug, Olof,
>>>>>>
>>>>>> IIUC, it sounds like this patch is needed from some other patches in
>>>>>> arm-soc. Olof was proposing to take this patch through its tree to
>>>>>> facilitate the integration.
>>>>>>
>>>>>> Olof, is it this patch you were worried about ?
>>>>>
>>>>> I think this is one of two patches in question.
>>>>>
>>>>> "clocksource: arch_timer: Fix code to use physical timers when
>>>>> requested"
>>>>> [0] would be the second one.
>>>>>
>>>>> And the patch for arm-soc that Olof means would be "ARM: dts: rk3288:
>>>>> add
>>>>> arm,cpu-registers-not-fw-configured" [1].
>>>>
>>>> Ok, so IIUC, "clocksource: arch_timer: Fix code to use physical timers
>>>> when requested" should go via arm's tree, right ?
>>>
>>> If I'm reading Olof's irc-comments from yesterday correctly, that is right
>>> and the 3 patches should go in together:
>>>
>>> - "clocksource: arch_timer: Fix code to use physical timers
>>>
>>> when requested" fixes the use of physical timers in general
>>>
>>> - "clocksource: arch_timer: Allow the device tree to specify uninitialized
>>>
>>> timer registers" allows this to be set from dt
>>>
>>> - "ARM: dts: rk3288: add arm,cpu-registers-not-fw-configured" enables this
>>> on>
>>> rk3288
>>
>> Ok, then I drop them from my tree and will let Olof to handle them.
>
> But maybe you could give them an Ack :-)
Already done :)
-- Daniel
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On Wed, 2014-10-08 at 15:33 +0800, Sonny Rao wrote:
> From: Doug Anderson <[email protected]>
>
> Some 32-bit (ARMv7) systems are architected like this:
>
> * The firmware doesn't know and doesn't care about hypervisor mode and
> we don't want to add the complexity of hypervisor there.
>
> * The firmware isn't involved in SMP bringup or resume.
>
> * The ARCH timer come up with an uninitialized offset (CNTVOFF)
> between the virtual and physical counters. Each core gets a
> different random offset.
>
> * The device boots in "Secure SVC" mode.
>
> * Nothing has touched the reset value of CNTHCTL.PL1PCEN or
> CNTHCTL.PL1PCTEN (both default to 1 at reset)
>
> On systems like the above, it doesn't make sense to use the virtual
> counter. There's nobody managing the offset and each time a core goes
> down and comes back up it will get reinitialized to some other random
> value.
>
> This adds an optional property which can inform the kernel of this
> situation, and firmware is free to remove the property if it is going
> to initialize the CNTVOFF registers when each CPU comes out of reset.
>
> Currently, the best course of action in this case is to use the
> physical timer, which is why it is important that CNTHCTL hasn't been
> changed from its reset value and it's a reasonable assumption given
> that the firmware has never entered HYP mode.
>
> Note that it's been said that on ARMv8 systems the firmware and
> kernel really can't be architected as described above. That means
> using the physical timer like this really only makes sense for ARMv7
> systems.
>
> Signed-off-by: Doug Anderson <[email protected]>
> Signed-off-by: Sonny Rao <[email protected]>
> Reviewed-by: Mark Rutland <[email protected]>
> ---
> Changes in v2:
> - Add "#ifdef CONFIG_ARM" as per Will Deacon
>
> Changes in v3:
> - change property name to arm,cntvoff-not-fw-configured and specify
> that the value of CNTHCTL.PL1PC(T)EN must still be the reset value
> of 1 as per Mark Rutland
>
> Changes in v4:
> - change property name to arm,cpu-registers-not-fw-configured and
> specify that all cpu registers must have architected reset values
> per Mark Rutland
> - change from "#ifdef CONFIG_ARM" to "if (IS_ENABLED(CONFIG_ARM))" per
> Arnd Bergmann
> ---
> Documentation/devicetree/bindings/arm/arch_timer.txt | 8 ++++++++
> drivers/clocksource/arm_arch_timer.c | 8 ++++++++
> 2 files changed, 16 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt
> index 37b2caf..256b4d8 100644
> --- a/Documentation/devicetree/bindings/arm/arch_timer.txt
> +++ b/Documentation/devicetree/bindings/arm/arch_timer.txt
> @@ -22,6 +22,14 @@ to deliver its interrupts via SPIs.
> - always-on : a boolean property. If present, the timer is powered through an
> always-on power domain, therefore it never loses context.
>
> +** Optional properties:
> +
> +- arm,cpu-registers-not-fw-configured : Firmware does not initialize
> + any of the generic timer CPU registers, which contain their
> + architecturally-defined reset values. Only supported for 32-bit
> + systems which follow the ARMv7 architected reset values.
> +
> +
Hi,
Sorry for the (very) late reply.
I just realize today MT8135 need this and the other patch [1] to boot
SMP correctly. I've applied both patches and they works fine. Thanks :)
However, I'm not sure if we really need to add new property.
arm_arch_timer driver will only use virtual timer when virtual PPI
interrupt is provided, so the following patch to timer dtsi will also
works. I think if the firmware doesn't support virtual timer, it make
sense to not supply virtual interrupt.
timer {
compatible = "arm,armv7-timer";
interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_LOW>,
- <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>,
- <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW>,
- <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>;
+ <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>;
clock-frequency = <13000000>;
};
Joe.C
[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305436.html
Yingjoe,
On Wed, Nov 26, 2014 at 6:41 AM, Yingjoe Chen <[email protected]> wrote:
> Sorry for the (very) late reply.
> I just realize today MT8135 need this and the other patch [1] to boot
> SMP correctly. I've applied both patches and they works fine. Thanks :)
Excellent. It's helpful to include a Tested-by: tag in your email.
You'd have a line with just "Tested-by: Yingjoe Chen
<[email protected]>"
> However, I'm not sure if we really need to add new property.
> arm_arch_timer driver will only use virtual timer when virtual PPI
> interrupt is provided, so the following patch to timer dtsi will also
> works. I think if the firmware doesn't support virtual timer, it make
> sense to not supply virtual interrupt.
>
> timer {
> compatible = "arm,armv7-timer";
> interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_LOW>,
> - <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>,
> - <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW>,
> - <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>;
> + <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>;
> clock-frequency = <13000000>;
> };
Once you have Sonny's patch then I believe that the above would work.
However we rejected something like this because device tree is
supposed to describe the hardware. The hardware really does provide
the virtual timer interrupts and they really are at PPI 11 and PPI 10.
It's just that firmware doesn't handle things properly so they can't
be used.
NOTE: If we add the "arm,cpu-registers-not-fw-configured" to the
device tree and firmware actually works out how to configure things
(like if somehow has firmware that has a hypervisor) then it can
easily remove this device tree property before calling through to the
kernel. It would be much harder for the firmware to add back in the
"PPI 11" and "PPI 10" entries to the timer.
-Doug
Hi,
On Wed, 2014-11-26 at 08:14 -0800, Doug Anderson wrote:
> Yingjoe,
>
> On Wed, Nov 26, 2014 at 6:41 AM, Yingjoe Chen <[email protected]> wrote:
> > Sorry for the (very) late reply.
> > I just realize today MT8135 need this and the other patch [1] to boot
> > SMP correctly. I've applied both patches and they works fine. Thanks :)
>
> Excellent. It's helpful to include a Tested-by: tag in your email.
> You'd have a line with just "Tested-by: Yingjoe Chen
> <[email protected]>"
sure, here's my tested-by for the 2 patches
Tested-by: Yingjoe Chen <[email protected]>
I'll remember to add it next time :)
> > However, I'm not sure if we really need to add new property.
> > arm_arch_timer driver will only use virtual timer when virtual PPI
> > interrupt is provided, so the following patch to timer dtsi will also
> > works. I think if the firmware doesn't support virtual timer, it make
> > sense to not supply virtual interrupt.
> >
> > timer {
> > compatible = "arm,armv7-timer";
> > interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_LOW>,
> > - <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>,
> > - <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW>,
> > - <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>;
> > + <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>;
> > clock-frequency = <13000000>;
> > };
>
> Once you have Sonny's patch then I believe that the above would work.
> However we rejected something like this because device tree is
> supposed to describe the hardware. The hardware really does provide
> the virtual timer interrupts and they really are at PPI 11 and PPI 10.
> It's just that firmware doesn't handle things properly so they can't
> be used.
>
> NOTE: If we add the "arm,cpu-registers-not-fw-configured" to the
> device tree and firmware actually works out how to configure things
> (like if somehow has firmware that has a hypervisor) then it can
> easily remove this device tree property before calling through to the
> kernel. It would be much harder for the firmware to add back in the
> "PPI 11" and "PPI 10" entries to the timer.
>
> -Doug
I see your point, that's good for me then.
Thanks.
Joe.C
On Wed, Nov 26, 2014 at 12:49:42PM +0000, Daniel Lezcano wrote:
> On 10/08/2014 09:33 AM, Sonny Rao wrote:
> > From: Doug Anderson <[email protected]>
> >
> > Some 32-bit (ARMv7) systems are architected like this:
> >
> > * The firmware doesn't know and doesn't care about hypervisor mode and
> > we don't want to add the complexity of hypervisor there.
> >
> > * The firmware isn't involved in SMP bringup or resume.
> >
> > * The ARCH timer come up with an uninitialized offset (CNTVOFF)
> > between the virtual and physical counters. Each core gets a
> > different random offset.
> >
> > * The device boots in "Secure SVC" mode.
> >
> > * Nothing has touched the reset value of CNTHCTL.PL1PCEN or
> > CNTHCTL.PL1PCTEN (both default to 1 at reset)
> >
> > On systems like the above, it doesn't make sense to use the virtual
> > counter. There's nobody managing the offset and each time a core goes
> > down and comes back up it will get reinitialized to some other random
> > value.
> >
> > This adds an optional property which can inform the kernel of this
> > situation, and firmware is free to remove the property if it is going
> > to initialize the CNTVOFF registers when each CPU comes out of reset.
> >
> > Currently, the best course of action in this case is to use the
> > physical timer, which is why it is important that CNTHCTL hasn't been
> > changed from its reset value and it's a reasonable assumption given
> > that the firmware has never entered HYP mode.
> >
> > Note that it's been said that on ARMv8 systems the firmware and
> > kernel really can't be architected as described above. That means
> > using the physical timer like this really only makes sense for ARMv7
> > systems.
> >
> > Signed-off-by: Doug Anderson <[email protected]>
> > Signed-off-by: Sonny Rao <[email protected]>
> > Reviewed-by: Mark Rutland <[email protected]>
>
> Acked-by: Daniel Lezcano <[email protected]>
>
> I would be nice to have Catalin's ack.
FWIW:
Acked-by: Catalin Marinas <[email protected]>
On Wed, Oct 08, 2014 at 12:33:47AM -0700, Sonny Rao wrote:
> From: Doug Anderson <[email protected]>
>
> Some 32-bit (ARMv7) systems are architected like this:
>
> * The firmware doesn't know and doesn't care about hypervisor mode and
> we don't want to add the complexity of hypervisor there.
>
> * The firmware isn't involved in SMP bringup or resume.
>
> * The ARCH timer come up with an uninitialized offset (CNTVOFF)
> between the virtual and physical counters. Each core gets a
> different random offset.
>
> * The device boots in "Secure SVC" mode.
>
> * Nothing has touched the reset value of CNTHCTL.PL1PCEN or
> CNTHCTL.PL1PCTEN (both default to 1 at reset)
>
> On systems like the above, it doesn't make sense to use the virtual
> counter. There's nobody managing the offset and each time a core goes
> down and comes back up it will get reinitialized to some other random
> value.
>
> This adds an optional property which can inform the kernel of this
> situation, and firmware is free to remove the property if it is going
> to initialize the CNTVOFF registers when each CPU comes out of reset.
>
> Currently, the best course of action in this case is to use the
> physical timer, which is why it is important that CNTHCTL hasn't been
> changed from its reset value and it's a reasonable assumption given
> that the firmware has never entered HYP mode.
>
> Note that it's been said that on ARMv8 systems the firmware and
> kernel really can't be architected as described above. That means
> using the physical timer like this really only makes sense for ARMv7
> systems.
>
> Signed-off-by: Doug Anderson <[email protected]>
> Signed-off-by: Sonny Rao <[email protected]>
> Reviewed-by: Mark Rutland <[email protected]>
> ---
> Changes in v2:
> - Add "#ifdef CONFIG_ARM" as per Will Deacon
>
> Changes in v3:
> - change property name to arm,cntvoff-not-fw-configured and specify
> that the value of CNTHCTL.PL1PC(T)EN must still be the reset value
> of 1 as per Mark Rutland
>
> Changes in v4:
> - change property name to arm,cpu-registers-not-fw-configured and
> specify that all cpu registers must have architected reset values
> per Mark Rutland
> - change from "#ifdef CONFIG_ARM" to "if (IS_ENABLED(CONFIG_ARM))" per
> Arnd Bergmann
Applied to next/drivers (and next/dt for rk3288 dependency). Thanks, all!
-Olof