| Message ID | 1412753627-28287-1-git-send-email-sonnyrao@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Daniel, On Wed, Oct 8, 2014 at 12:33 AM, Sonny Rao <sonnyrao@chromium.org> wrote: > From: Doug Anderson <dianders@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 <dianders@chromium.org> > Signed-off-by: Sonny Rao <sonnyrao@chromium.org> > Reviewed-by: Mark Rutland <mark.rutland@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(+) 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 <sonnyrao@chromium.org> wrote: >> From: Doug Anderson <dianders@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 <dianders@chromium.org> >> Signed-off-by: Sonny Rao <sonnyrao@chromium.org> >> Reviewed-by: Mark Rutland <mark.rutland@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(+) > > 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
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 <sonnyrao@chromium.org> wrote: >> From: Doug Anderson <dianders@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 <dianders@chromium.org> >> Signed-off-by: Sonny Rao <sonnyrao@chromium.org> >> Reviewed-by: Mark Rutland <mark.rutland@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(+) > > 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! >
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 <sonnyrao@chromium.org> wrote: > >> From: Doug Anderson <dianders@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 <dianders@chromium.org> > >> Signed-off-by: Sonny Rao <sonnyrao@chromium.org> > >> Reviewed-by: Mark Rutland <mark.rutland@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(+) > > > > 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 <sonnyrao@chromium.org> wrote: >>>> From: Doug Anderson <dianders@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 <dianders@chromium.org> >>>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org> >>>> Reviewed-by: Mark Rutland <mark.rutland@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(+) >>> >>> 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! >
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 <dianders@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 <dianders@chromium.org> > Signed-off-by: Sonny Rao <sonnyrao@chromium.org> > Reviewed-by: Mark Rutland <mark.rutland@arm.com> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org> 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. >
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
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
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 Wed, 2014-10-08 at 15:33 +0800, Sonny Rao wrote: > From: Doug Anderson <dianders@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 <dianders@chromium.org> > Signed-off-by: Sonny Rao <sonnyrao@chromium.org> > Reviewed-by: Mark Rutland <mark.rutland@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. > + > + 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 <yingjoe.chen@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 <yingjoe.chen@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
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 <yingjoe.chen@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 > <yingjoe.chen@mediatek.com>" sure, here's my tested-by for the 2 patches Tested-by: Yingjoe Chen <yingjoe.chen@mediatek.com> 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 <dianders@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 <dianders@chromium.org> > > Signed-off-by: Sonny Rao <sonnyrao@chromium.org> > > Reviewed-by: Mark Rutland <mark.rutland@arm.com> > > Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org> > > I would be nice to have Catalin's ack. FWIW: Acked-by: Catalin Marinas <catalin.marinas@arm.com>
On Wed, Oct 08, 2014 at 12:33:47AM -0700, Sonny Rao wrote: > From: Doug Anderson <dianders@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 <dianders@chromium.org> > Signed-off-by: Sonny Rao <sonnyrao@chromium.org> > Reviewed-by: Mark Rutland <mark.rutland@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 Applied to next/drivers (and next/dt for rk3288 dependency). Thanks, all! -Olof
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.