2018-10-08 12:52:38

by Marek Szyprowski

[permalink] [raw]
Subject: [PATCH 0/7] Proper arch timer support for Exynos5433-based TM2(e) boards

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



2018-10-08 12:51:19

by Marek Szyprowski

[permalink] [raw]
Subject: [PATCH 7/7] arm64: platform: Add enable Exynos Multi-Core Timer driver

Exynos MCT driver is required even if ARM Architected Timer driver
is used to properly configure common timer hardware.

Signed-off-by: Marek Szyprowski <[email protected]>
---
arch/arm64/Kconfig.platforms | 1 +
1 file changed, 1 insertion(+)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 51bc479334a4..7cc687580fad 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -62,6 +62,7 @@ config ARCH_BRCMSTB
config ARCH_EXYNOS
bool "ARMv8 based Samsung Exynos SoC family"
select COMMON_CLK_SAMSUNG
+ select CLKSRC_EXYNOS_MCT
select EXYNOS_PM_DOMAINS if PM_GENERIC_DOMAINS
select EXYNOS_PMU
select HAVE_S3C2410_WATCHDOG if WATCHDOG
--
2.17.1


2018-10-08 12:51:27

by Marek Szyprowski

[permalink] [raw]
Subject: [PATCH 6/7] arm64: dts: exynos: Mark arch_timer device as not-fw-configured on TM2(e)

TM2(e) boards use old firmware, which some PCSI requirements and doesn't
properly configure arch virtual timers, so add property indicating this
state. This enables respective workarounds in the timer driver.

Signed-off-by: Marek Szyprowski <[email protected]>
---
arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi | 1 +
1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi b/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
index 2606c2ab6456..ded0fb6c5936 100644
--- a/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
@@ -1265,6 +1265,7 @@
};

&timer {
+ arm,cpu-registers-not-fw-configured;
clock-frequency = <24000000>;
};

--
2.17.1


2018-10-08 12:51:36

by Marek Szyprowski

[permalink] [raw]
Subject: [PATCH 5/7] arm64: dts: exynos: Move arch-timer node to right place

Move ARM architected timer device-tree node to the beginning of 'soc'
node, to group it together with other ARM CPU core devices (like PMU).

Signed-off-by: Marek Szyprowski <[email protected]>
---
arch/arm64/boot/dts/exynos/exynos5433.dtsi | 23 +++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
index 77297d66642c..eee3d1d1b52f 100644
--- a/arch/arm64/boot/dts/exynos/exynos5433.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
@@ -290,6 +290,18 @@
interrupt-affinity = <&cpu4>, <&cpu5>, <&cpu6>, <&cpu7>;
};

+ timer: timer {
+ compatible = "arm,armv8-timer";
+ interrupts = <GIC_PPI 13
+ (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>,
+ <GIC_PPI 14
+ (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>,
+ <GIC_PPI 11
+ (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>,
+ <GIC_PPI 10
+ (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>;
+ };
+
sysram@2020000 {
compatible = "mmio-sram";
reg = <0x02020000 0x5c000>;
@@ -1811,17 +1823,6 @@
};
};

- timer: timer {
- compatible = "arm,armv8-timer";
- interrupts = <GIC_PPI 13
- (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>,
- <GIC_PPI 14
- (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>,
- <GIC_PPI 11
- (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>,
- <GIC_PPI 10
- (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>;
- };
};

#include "exynos5433-bus.dtsi"
--
2.17.1


2018-10-08 12:51:41

by Marek Szyprowski

[permalink] [raw]
Subject: [PATCH 2/7] clocksource: exynos_mct: Add arch_timer cooperation mode for ARM64

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 <[email protected]>
---
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 =
@@ -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);
+ }
+ }
+
/* 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


2018-10-08 12:51:49

by Marek Szyprowski

[permalink] [raw]
Subject: [PATCH 4/7] clocksource: arch_timer: Add support for not-fw-configured timer on ARM64

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 <[email protected]>
---
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.
+
config ARM_GLOBAL_TIMER
bool "Support for the ARM global timer" if COMPILE_TEST
select TIMER_OF if OF
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 9a7d4dc00b6e..b6f109654daf 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -423,6 +423,14 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
.read_cntvct_el0 = arm64_1188873_read_cntvct_el0,
},
#endif
+#ifdef CONFIG_ARCH_TIMER_REGISTERS_NOT_FW_CONFIGURED
+ {
+ .match_type = ate_match_dt,
+ .id = "arm,cpu-registers-not-fw-configured",
+ .desc = "broken CPU firmware (timer registers not configured)",
+ .read_cntvct_el0 = arch_counter_get_cntpct,
+ },
+#endif
};

typedef bool (*ate_match_fn_t)(const struct arch_timer_erratum_workaround *,
@@ -1234,9 +1242,10 @@ static int __init arch_timer_of_init(struct device_node *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_uses_ppi = ARCH_TIMER_PHYS_SECURE_PPI;
+ if (of_property_read_bool(np, "arm,cpu-registers-not-fw-configured"))
+ arch_timer_uses_ppi = IS_ENABLED(CONFIG_ARM64) ?
+ ARCH_TIMER_PHYS_NONSECURE_PPI :
+ ARCH_TIMER_PHYS_SECURE_PPI;
else
arch_timer_uses_ppi = arch_timer_select_ppi();

--
2.17.1


2018-10-08 12:52:21

by Marek Szyprowski

[permalink] [raw]
Subject: [PATCH 1/7] clocksource: exynos_mct: Remove dead code

Exynos Multi-Core Timer driver is used only on device-tree based
systems, so remove non-dt related code.

Signed-off-by: Marek Szyprowski <[email protected]>
---
drivers/clocksource/exynos_mct.c | 4 ----
1 file changed, 4 deletions(-)

diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index 7a244b681876..43b335ff4a96 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -581,11 +581,7 @@ static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
* timer irqs are specified after the four global timer
* irqs are specified.
*/
-#ifdef CONFIG_OF
nr_irqs = of_irq_count(np);
-#else
- nr_irqs = 0;
-#endif
for (i = MCT_L0_IRQ; i < nr_irqs; i++)
mct_irqs[i] = irq_of_parse_and_map(np, i);

--
2.17.1


2018-10-08 12:52:44

by Marek Szyprowski

[permalink] [raw]
Subject: [PATCH 3/7] clocksource: Change CPU hotplug priority of exynos_mct driver

Exynos Multi-Core Timer driver (exynos_mct) must be started before
ARM Architected Timers (arch_timer), because both timers share common
hardware block and turning on MCT is needed to get ARM Architected
Timer working properly.

Signed-off-by: Marek Szyprowski <[email protected]>
---
include/linux/cpuhotplug.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index caf40ad0bbc6..5d9e4a6ea299 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -115,10 +115,10 @@ enum cpuhp_state {
CPUHP_AP_PERF_ARM_ACPI_STARTING,
CPUHP_AP_PERF_ARM_STARTING,
CPUHP_AP_ARM_L2X0_STARTING,
+ CPUHP_AP_EXYNOS4_MCT_TIMER_STARTING,
CPUHP_AP_ARM_ARCH_TIMER_STARTING,
CPUHP_AP_ARM_GLOBAL_TIMER_STARTING,
CPUHP_AP_JCORE_TIMER_STARTING,
- CPUHP_AP_EXYNOS4_MCT_TIMER_STARTING,
CPUHP_AP_ARM_TWD_STARTING,
CPUHP_AP_QCOM_TIMER_STARTING,
CPUHP_AP_ARMADA_TIMER_STARTING,
--
2.17.1


2018-10-08 13:17:59

by Marc Zyngier

[permalink] [raw]
Subject: Re: [PATCH 4/7] clocksource: arch_timer: Add support for not-fw-configured timer on ARM64

+ 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 <[email protected]>
> ---
> 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...

2018-10-09 08:12:39

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 1/7] clocksource: exynos_mct: Remove dead code

On Mon, 8 Oct 2018 at 14:50, Marek Szyprowski <[email protected]> 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 <[email protected]>

Best regards,
Krzysztof

2018-10-09 08:20:45

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 3/7] clocksource: Change CPU hotplug priority of exynos_mct driver

On Mon, 8 Oct 2018 at 14:50, Marek Szyprowski <[email protected]> wrote:
>
> Exynos Multi-Core Timer driver (exynos_mct) must be started before
> ARM Architected Timers (arch_timer), because both timers share common
> hardware block and turning on MCT is needed to get ARM Architected
> Timer working properly.
>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> include/linux/cpuhotplug.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

2018-10-09 08:21:17

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 2/7] clocksource: exynos_mct: Add arch_timer cooperation mode for ARM64

On Mon, 8 Oct 2018 at 14:50, Marek Szyprowski <[email protected]> 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 <[email protected]>
> ---
> 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
>

2018-10-15 12:13:25

by Marek Szyprowski

[permalink] [raw]
Subject: Re: [PATCH 4/7] clocksource: arch_timer: Add support for not-fw-configured timer on ARM64

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 <[email protected]>
>> ---
>>   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


2018-10-15 12:29:26

by Daniel Lezcano

[permalink] [raw]
Subject: Re: [PATCH 4/7] clocksource: arch_timer: Add support for not-fw-configured timer on ARM64

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 <[email protected]>
>>> ---
>>>   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