[PATCH v4] clocksource: arch_timer: Allow the device tree to specify uninitialized timer registers

[Sonny Rao]

From: Doug Anderson <dian...@chromium.org>

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 <dian...@chromium.org>
Signed-off-by: Sonny Rao <sonn...@chromium.org>
Reviewed-by: Mark Rutland <mark.r...@arm.com>
---
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Doug Anderson]

Daniel,

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!

[Daniel Lezcano]

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

[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 ?

Thanks

-- Daniel

On 11/20/2014 12:01 AM, Doug Anderson wrote:

<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

[Heiko Stübner]

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

[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 ?

<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

[Daniel Lezcano]

Acked-by: Daniel Lezcano <daniel....@linaro.org>

I would be nice to have Catalin's ack.

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

[Heiko Stübner]

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

[Heiko Stübner]

Am Mittwoch, 26. November 2014, 13:49:57 schrieb Daniel Lezcano:

> Ok, then I drop them from my tree and will let Olof to handle them.

But maybe you could give them an Ack :-)

[Daniel Lezcano]

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

[Daniel Lezcano]

On 11/26/2014 01:48 PM, Heiko Stübner wrote:

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

[Yingjoe Chen]

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

[Doug Anderson]

Yingjoe,

On Wed, Nov 26, 2014 at 6:41 AM, Yingjoe Chen <yingjo...@mediatek.com> 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
<yingjo...@mediatek.com>"

> 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

[Yingjoe Chen]

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 <yingjo...@mediatek.com> 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
> <yingjo...@mediatek.com>"

sure, here's my tested-by for the 2 patches

Tested-by: Yingjoe Chen <yingjo...@mediatek.com>

I'll remember to add it next time :)

I see your point, that's good for me then.
Thanks.

Joe.C

[Catalin Marinas]

FWIW:

Acked-by: Catalin Marinas <catalin...@arm.com>

[Olof Johansson]

Applied to next/drivers (and next/dt for rk3288 dependency). Thanks, all!


-Olof