| Message ID | 20181008125009.3721-1-m.szyprowski@samsung.com |
|---|---|
| Headers | show |
| Series | Proper arch timer support for Exynos5433-based TM2(e) boards | expand |
+ Mark Rutland Hi Marek, On 08/10/18 13:50, Marek Szyprowski wrote: > Use common infrastructure for ARM Architected Timers erratum to enable > support for systems with broken CPU firmware (timer registers not > properly configured). This mode has been already availabled on ARM > (32bits) architecture. This enables to run Linux kernel on ARM64 boards > using physical architected timers instead of the virtual ones. Examples > of such system with broken firmware are Samsung Exynos5433 SoC based > TM2(e) boards, which is already deployed for years and updating firmware > is not possible. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/clocksource/Kconfig | 11 +++++++++++ > drivers/clocksource/arm_arch_timer.c | 15 ++++++++++++--- > 2 files changed, 23 insertions(+), 3 deletions(-) > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > index a11f4ba98b05..a30752579b03 100644 > --- a/drivers/clocksource/Kconfig > +++ b/drivers/clocksource/Kconfig > @@ -364,6 +364,17 @@ config ARM64_ERRATUM_858921 > The workaround will be dynamically enabled when an affected > core is detected. > > +config ARCH_TIMER_REGISTERS_NOT_FW_CONFIGURED > + bool "Workaround for arch timer registers not configured by firmware" > + default y > + select ARM_ARCH_TIMER_OOL_WORKAROUND > + depends on ARM_ARCH_TIMER && ARM64 > + help > + This option enables a workaround for boards, on which arch timer > + registers are not properly configured by the board firmware. > + The workaround will be dynamically enabled when an affected > + board is detected. > + I'm sorry, but I'm strongly pushing back on this. This horrible hack was accepted with the express condition that it would be limited to ARMv7 platforms (on the ground that we never really documented the arch timer boot requirements on that version of the architecture), and would never proliferate on arm64. From day 1, we established what the boot protocol was, and we mandated that either: - kernel is entered at EL2 on all CPUs - cntvoff_el2 is zeroed on all CPUs and we've got most people to fix their firmware, or live with the consequences. If these machines cannot receive a non-broken firmware, what are the odds that they will receive a mainline kernel? Thanks, M. -- Jazz is not dead. It just smells funny...
On Mon, 8 Oct 2018 at 14:50, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > Exynos Multi-Core Timer driver is used only on device-tree based > systems, so remove non-dt related code. You can also mention that in case of !CONFIG_OF the code is anyway equal because of_irq_count() has a stub returning 0. Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> Best regards, Krzysztof
On Mon, 8 Oct 2018 at 14:50, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > To get ARM Architected Timers working on Samsung Exynos SoCs, one has to > first configure and enable Exynos Multi-Core Timer, because they both > share some common hardware blocks. This patch adds a mode of cooperation > with arch_timer driver, so kernel can use CP15 based timer interface via > arch_timer driver, which is mandatory on ARM64. In such mode driver only > configures MCT registers and starts the timer but don't register any > clocksource or events in the system. Those are left to be handled by > arch_timer driver. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/clocksource/exynos_mct.c | 28 ++++++++++++++++++++++------ > 1 file changed, 22 insertions(+), 6 deletions(-) > > diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c > index 43b335ff4a96..05b201ed8ef5 100644 > --- a/drivers/clocksource/exynos_mct.c > +++ b/drivers/clocksource/exynos_mct.c > @@ -57,6 +57,7 @@ > #define TICK_BASE_CNT 1 > > enum { > + MCT_INT_NONE = 0, > MCT_INT_SPI, > MCT_INT_PPI > }; > @@ -238,6 +239,9 @@ static int __init exynos4_clocksource_init(void) > { > exynos4_mct_frc_start(); > > + if (!mct_int_type) > + return 0; > + > #if defined(CONFIG_ARM) > exynos4_delay_timer.read_current_timer = &exynos4_read_current_timer; > exynos4_delay_timer.freq = clk_rate; > @@ -343,6 +347,9 @@ static struct irqaction mct_comp_event_irq = { > > static int exynos4_clockevent_init(void) > { > + if (!mct_int_type) > + return 0; > + > mct_comp_device.cpumask = cpumask_of(0); > clockevents_config_and_register(&mct_comp_device, clk_rate, > 0xf, 0xffffffff); > @@ -476,12 +483,12 @@ static int exynos4_mct_starting_cpu(unsigned int cpu) > > irq_force_affinity(evt->irq, cpumask_of(cpu)); > enable_irq(evt->irq); > - } else { > + } else if (mct_int_type == MCT_INT_PPI) { > enable_percpu_irq(mct_irqs[MCT_L0_IRQ], 0); > } > - clockevents_config_and_register(evt, clk_rate / (TICK_BASE_CNT + 1), > - 0xf, 0x7fffffff); > - > + if (mct_int_type) > + clockevents_config_and_register(evt, > + clk_rate / (TICK_BASE_CNT + 1), 0xf, 0x7fffffff); > return 0; > } > > @@ -496,7 +503,7 @@ static int exynos4_mct_dying_cpu(unsigned int cpu) > if (evt->irq != -1) > disable_irq_nosync(evt->irq); > exynos4_mct_write(0x1, mevt->base + MCT_L_INT_CSTAT_OFFSET); > - } else { > + } else if (mct_int_type == MCT_INT_PPI) { > disable_percpu_irq(mct_irqs[MCT_L0_IRQ]); > } > return 0; > @@ -529,7 +536,7 @@ static int __init exynos4_timer_resources(struct device_node *np, void __iomem * > &percpu_mct_tick); > WARN(err, "MCT: can't request IRQ %d (%d)\n", > mct_irqs[MCT_L0_IRQ], err); > - } else { > + } else if (mct_int_type == MCT_INT_SPI) { > for_each_possible_cpu(cpu) { > int mct_irq = mct_irqs[MCT_L0_IRQ + cpu]; > struct mct_clock_event_device *pcpu_mevt = The error path of cpuhp_setup_state() does not look correct because request_percpu_irq() was not called. The error path looks suspicious also for MCT_INT_SPI path - the IRQs were allocated with regular request_irq but are freed with percpu version. > @@ -573,6 +580,15 @@ static int __init mct_init_dt(struct device_node *np, unsigned int int_type) > > mct_int_type = int_type; > > + if (IS_ENABLED(CONFIG_ARM64) && IS_ENABLED(CONFIG_ARM_ARCH_TIMER)) { > + struct device_node *np = of_find_compatible_node(NULL, NULL, > + "arm,armv8-timer"); > + if (np) { > + mct_int_type = MCT_INT_NONE; > + of_node_put(np); In this case, later the mct_irqs are being assigned but totally not used. Are they needed? Best regards, Krzysztof > + } > + } > + > /* This driver uses only one global timer interrupt */ > mct_irqs[MCT_G0_IRQ] = irq_of_parse_and_map(np, MCT_G0_IRQ); > > -- > 2.17.1 >
Hi All, On 2018-10-08 15:17, Marc Zyngier wrote: > + Mark Rutland > > Hi Marek, > > On 08/10/18 13:50, Marek Szyprowski wrote: >> Use common infrastructure for ARM Architected Timers erratum to enable >> support for systems with broken CPU firmware (timer registers not >> properly configured). This mode has been already availabled on ARM >> (32bits) architecture. This enables to run Linux kernel on ARM64 boards >> using physical architected timers instead of the virtual ones. Examples >> of such system with broken firmware are Samsung Exynos5433 SoC based >> TM2(e) boards, which is already deployed for years and updating firmware >> is not possible. >> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> --- >> drivers/clocksource/Kconfig | 11 +++++++++++ >> drivers/clocksource/arm_arch_timer.c | 15 ++++++++++++--- >> 2 files changed, 23 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig >> index a11f4ba98b05..a30752579b03 100644 >> --- a/drivers/clocksource/Kconfig >> +++ b/drivers/clocksource/Kconfig >> @@ -364,6 +364,17 @@ config ARM64_ERRATUM_858921 >> The workaround will be dynamically enabled when an affected >> core is detected. >> +config ARCH_TIMER_REGISTERS_NOT_FW_CONFIGURED >> + bool "Workaround for arch timer registers not configured by >> firmware" >> + default y >> + select ARM_ARCH_TIMER_OOL_WORKAROUND >> + depends on ARM_ARCH_TIMER && ARM64 >> + help >> + This option enables a workaround for boards, on which arch timer >> + registers are not properly configured by the board firmware. >> + The workaround will be dynamically enabled when an affected >> + board is detected. >> + > > I'm sorry, but I'm strongly pushing back on this. > > This horrible hack was accepted with the express condition that it > would be limited to ARMv7 platforms (on the ground that we never > really documented the arch timer boot requirements on that version of > the architecture), and would never proliferate on arm64. From day 1, > we established what the boot protocol was, and we mandated that either: > > - kernel is entered at EL2 on all CPUs > - cntvoff_el2 is zeroed on all CPUs > > and we've got most people to fix their firmware, or live with the > consequences. If these machines cannot receive a non-broken firmware, > what are the odds that they will receive a mainline kernel? Well, I know that the firmware is broken, but I cannot do anything about it. On the other hand updating kernel is still possible and mainline runs fine on TM2(e) boards. I will send v2 without this patch then. Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
On 15/10/2018 14:12, Marek Szyprowski wrote: > Hi All, > > On 2018-10-08 15:17, Marc Zyngier wrote: >> + Mark Rutland >> >> Hi Marek, >> >> On 08/10/18 13:50, Marek Szyprowski wrote: >>> Use common infrastructure for ARM Architected Timers erratum to enable >>> support for systems with broken CPU firmware (timer registers not >>> properly configured). This mode has been already availabled on ARM >>> (32bits) architecture. This enables to run Linux kernel on ARM64 boards >>> using physical architected timers instead of the virtual ones. Examples >>> of such system with broken firmware are Samsung Exynos5433 SoC based >>> TM2(e) boards, which is already deployed for years and updating firmware >>> is not possible. >>> >>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>> --- >>> drivers/clocksource/Kconfig | 11 +++++++++++ >>> drivers/clocksource/arm_arch_timer.c | 15 ++++++++++++--- >>> 2 files changed, 23 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig >>> index a11f4ba98b05..a30752579b03 100644 >>> --- a/drivers/clocksource/Kconfig >>> +++ b/drivers/clocksource/Kconfig >>> @@ -364,6 +364,17 @@ config ARM64_ERRATUM_858921 >>> The workaround will be dynamically enabled when an affected >>> core is detected. >>> +config ARCH_TIMER_REGISTERS_NOT_FW_CONFIGURED >>> + bool "Workaround for arch timer registers not configured by >>> firmware" >>> + default y >>> + select ARM_ARCH_TIMER_OOL_WORKAROUND >>> + depends on ARM_ARCH_TIMER && ARM64 >>> + help >>> + This option enables a workaround for boards, on which arch timer >>> + registers are not properly configured by the board firmware. >>> + The workaround will be dynamically enabled when an affected >>> + board is detected. >>> + >> >> I'm sorry, but I'm strongly pushing back on this. >> >> This horrible hack was accepted with the express condition that it >> would be limited to ARMv7 platforms (on the ground that we never >> really documented the arch timer boot requirements on that version of >> the architecture), and would never proliferate on arm64. From day 1, >> we established what the boot protocol was, and we mandated that either: >> >> - kernel is entered at EL2 on all CPUs >> - cntvoff_el2 is zeroed on all CPUs >> >> and we've got most people to fix their firmware, or live with the >> consequences. If these machines cannot receive a non-broken firmware, >> what are the odds that they will receive a mainline kernel? > > Well, I know that the firmware is broken, but I cannot do anything about it. > On the other hand updating kernel is still possible and mainline runs fine > on TM2(e) boards. I will send v2 without this patch then. I second Marc's opinion. -- <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
Dear All, This patchset is an attempt to submit the last piece of missing code to have proper support for Exynos5433 SoCs based TM2(e) boards. It performs a cleanup of timer configuration, which so far needed various out-of-tree workarounds. The fixes provided by this patchset are also needed for add proper support for system suspend/resume. Best regards Marek Szyprowski Samsung R&D Institute Poland Patch summary: Marek Szyprowski (7): clocksource: exynos_mct: Remove dead code clocksource: exynos_mct: Add arch_timer cooperation mode for ARM64 clocksource: Change CPU hotplug priority of exynos_mct driver clocksource: arch_timer: Add support for not-fw-configured timer on ARM64 arm64: dts: exynos: Move arch-timer node to right place arm64: dts: exynos: Mark arch_timer device as not-fw-configured on TM2(e) arm64: platform: Add enable Exynos Multi-Core Timer driver arch/arm64/Kconfig.platforms | 1 + .../dts/exynos/exynos5433-tm2-common.dtsi | 1 + arch/arm64/boot/dts/exynos/exynos5433.dtsi | 23 ++++++------- drivers/clocksource/Kconfig | 11 +++++++ drivers/clocksource/arm_arch_timer.c | 15 +++++++-- drivers/clocksource/exynos_mct.c | 32 +++++++++++++------ include/linux/cpuhotplug.h | 2 +- 7 files changed, 60 insertions(+), 25 deletions(-) -- 2.17.1