2018-10-17 13:44:08

by Marek Szyprowski

[permalink] [raw]
Subject: [PATCH v3 0/6] 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


Changelog:

v3:
- added patch, which splits resources and interrupts allocation
- simplified arch timer cooperation mode
- dropped CPU hotplug priority change patch, it is not really needed
- removed more non-dt dead code

v2:
- dropped arch timer patch, it will be discussed separately
- fixed issues pointed by Krzysztof Kozlowski

v1: https://patchwork.kernel.org/project/linux-samsung-soc/list/?series=27965&state=*&archive=both
- initial version


Patch summary:

Marek Szyprowski (6):
clocksource: exynos_mct: Remove dead code
clocksource: exynos_mct: Fix error path in timer resources
initialization
clocksource: exynos_mct: Refactor resources allocation
clocksource: exynos_mct: Add arch_timer cooperation mode for ARM64
arm64: dts: exynos: Move arch-timer node to right place
arm64: platform: Add enable Exynos Multi-Core Timer driver

arch/arm64/Kconfig.platforms | 1 +
arch/arm64/boot/dts/exynos/exynos5433.dtsi | 23 +++---
drivers/clocksource/exynos_mct.c | 81 ++++++++++++++--------
3 files changed, 67 insertions(+), 38 deletions(-)

--
2.17.1



2018-10-17 13:42:56

by Marek Szyprowski

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

On Exynos SoCs enabling MCT driver is required even if ARM Architected
Timer driver is used to for managing timer hardware and clock source
events.

Signed-off-by: Marek Szyprowski <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Reviewed-by: Chanwoo Choi <[email protected]>
Tested-by: Chanwoo Choi <[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-17 13:43:04

by Marek Szyprowski

[permalink] [raw]
Subject: [PATCH v3 4/6] 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 (global system counter). 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 MCT driver only enables its clocks and starts
global timer. Everything else will be handled by arch_timer driver.

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

diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index 02ad55db390b..1b19a4f03929 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -606,6 +606,15 @@ static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
if (ret)
return ret;

+ 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) {
+ of_node_put(np);
+ exynos4_mct_frc_start();
+ return 0;
+ }
+ }

ret = exynos4_timer_interrupts(np, int_type);
if (ret)
--
2.17.1


2018-10-17 13:43:13

by Marek Szyprowski

[permalink] [raw]
Subject: [PATCH v3 3/6] clocksource: exynos_mct: Refactor resources allocation

Move interrupts allocation from exynos4_timer_resources() into separate
function together with the interrupt number parsing code from
mct_init_dt(), so the code for managing interrupts is kept together.
While touching exynos4_timer_resources() function, move of_iomap() to it.
No functional changes.

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

diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index 49413900b24c..02ad55db390b 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -502,11 +502,14 @@ static int exynos4_mct_dying_cpu(unsigned int cpu)
return 0;
}

-static int __init exynos4_timer_resources(struct device_node *np, void __iomem *base)
+static int __init exynos4_timer_resources(struct device_node *np)
{
- int err, cpu;
struct clk *mct_clk, *tick_clk;

+ reg_base = of_iomap(np, 0);
+ if (!reg_base)
+ panic("%s: unable to ioremap mct address space\n", __func__);
+
tick_clk = of_clk_get_by_name(np, "fin_pll");
if (IS_ERR(tick_clk))
panic("%s: unable to determine tick clock rate\n", __func__);
@@ -517,9 +520,27 @@ static int __init exynos4_timer_resources(struct device_node *np, void __iomem *
panic("%s: unable to retrieve mct clock instance\n", __func__);
clk_prepare_enable(mct_clk);

- reg_base = base;
- if (!reg_base)
- panic("%s: unable to ioremap mct address space\n", __func__);
+ return 0;
+}
+
+static int __init exynos4_timer_interrupts(struct device_node *np,
+ unsigned int int_type)
+{
+ int i, err, cpu;
+
+ mct_int_type = int_type;
+
+ /* This driver uses only one global timer interrupt */
+ mct_irqs[MCT_G0_IRQ] = irq_of_parse_and_map(np, MCT_G0_IRQ);
+
+ /*
+ * Find out the number of local irqs specified. The local
+ * timer irqs are specified after the four global timer
+ * irqs are specified.
+ */
+ nr_irqs = of_irq_count(np);
+ for (i = MCT_L0_IRQ; i < nr_irqs; i++)
+ mct_irqs[i] = irq_of_parse_and_map(np, i);

if (mct_int_type == MCT_INT_PPI) {

@@ -579,24 +600,14 @@ static int __init exynos4_timer_resources(struct device_node *np, void __iomem *

static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
{
- u32 nr_irqs, i;
int ret;

- mct_int_type = int_type;
-
- /* This driver uses only one global timer interrupt */
- mct_irqs[MCT_G0_IRQ] = irq_of_parse_and_map(np, MCT_G0_IRQ);
+ ret = exynos4_timer_resources(np);
+ if (ret)
+ return ret;

- /*
- * Find out the number of local irqs specified. The local
- * timer irqs are specified after the four global timer
- * irqs are specified.
- */
- nr_irqs = of_irq_count(np);
- for (i = MCT_L0_IRQ; i < nr_irqs; i++)
- mct_irqs[i] = irq_of_parse_and_map(np, i);

- ret = exynos4_timer_resources(np, of_iomap(np, 0));
+ ret = exynos4_timer_interrupts(np, int_type);
if (ret)
return ret;

--
2.17.1


2018-10-17 13:43:51

by Marek Szyprowski

[permalink] [raw]
Subject: [PATCH v3 2/6] clocksource: exynos_mct: Fix error path in timer resources initialization

While freeing interrupt handlers in error path, don't assume that all
requested interrupts are per-processor interrupts and properly release
standard interrupts too.

Reported-by: Krzysztof Kozlowski <[email protected]>
Fixes: 56a94f13919c ("clocksource: exynos_mct: Avoid blocking calls in the cpu hotplug notifier")
Signed-off-by: Marek Szyprowski <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Reviewed-by: Chanwoo Choi <[email protected]>
---
drivers/clocksource/exynos_mct.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index ef18bbf8d20c..49413900b24c 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -561,7 +561,19 @@ static int __init exynos4_timer_resources(struct device_node *np, void __iomem *
return 0;

out_irq:
- free_percpu_irq(mct_irqs[MCT_L0_IRQ], &percpu_mct_tick);
+ if (mct_int_type == MCT_INT_PPI) {
+ free_percpu_irq(mct_irqs[MCT_L0_IRQ], &percpu_mct_tick);
+ } else {
+ for_each_possible_cpu(cpu) {
+ struct mct_clock_event_device *pcpu_mevt =
+ per_cpu_ptr(&percpu_mct_tick, cpu);
+
+ if (pcpu_mevt->evt.irq != -1) {
+ free_irq(pcpu_mevt->evt.irq, pcpu_mevt);
+ pcpu_mevt->evt.irq = -1;
+ }
+ }
+ }
return err;
}

--
2.17.1


2018-10-17 13:44:41

by Marek Szyprowski

[permalink] [raw]
Subject: [PATCH v3 5/6] 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]>
Reviewed-by: Chanwoo Choi <[email protected]>
Tested-by: Chanwoo Choi <[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 2131f12364cb..fa20eb3495b3 100644
--- a/arch/arm64/boot/dts/exynos/exynos5433.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
@@ -255,6 +255,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)>;
+ };
+
chipid@10000000 {
compatible = "samsung,exynos4210-chipid";
reg = <0x10000000 0x100>;
@@ -1750,17 +1762,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-17 13:44:53

by Marek Szyprowski

[permalink] [raw]
Subject: [PATCH v3 1/6] 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. In case of !CONFIG_OF
the code is anyway equal because of_irq_count() has a stub
returning 0. Device node pointer is always provided when driver
has been probed from device tree.

Signed-off-by: Marek Szyprowski <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Reviewed-by: Chanwoo Choi <[email protected]>
---
drivers/clocksource/exynos_mct.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index 7a244b681876..ef18bbf8d20c 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -507,13 +507,12 @@ static int __init exynos4_timer_resources(struct device_node *np, void __iomem *
int err, cpu;
struct clk *mct_clk, *tick_clk;

- tick_clk = np ? of_clk_get_by_name(np, "fin_pll") :
- clk_get(NULL, "fin_pll");
+ tick_clk = of_clk_get_by_name(np, "fin_pll");
if (IS_ERR(tick_clk))
panic("%s: unable to determine tick clock rate\n", __func__);
clk_rate = clk_get_rate(tick_clk);

- mct_clk = np ? of_clk_get_by_name(np, "mct") : clk_get(NULL, "mct");
+ mct_clk = of_clk_get_by_name(np, "mct");
if (IS_ERR(mct_clk))
panic("%s: unable to retrieve mct clock instance\n", __func__);
clk_prepare_enable(mct_clk);
@@ -581,11 +580,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-17 14:30:54

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v3 3/6] clocksource: exynos_mct: Refactor resources allocation

On Wed, 17 Oct 2018 at 15:42, Marek Szyprowski <[email protected]> wrote:
>
> Move interrupts allocation from exynos4_timer_resources() into separate
> function together with the interrupt number parsing code from
> mct_init_dt(), so the code for managing interrupts is kept together.
> While touching exynos4_timer_resources() function, move of_iomap() to it.
> No functional changes.
>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> drivers/clocksource/exynos_mct.c | 49 +++++++++++++++++++-------------
> 1 file changed, 30 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index 49413900b24c..02ad55db390b 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -502,11 +502,14 @@ static int exynos4_mct_dying_cpu(unsigned int cpu)
> return 0;
> }
>
> -static int __init exynos4_timer_resources(struct device_node *np, void __iomem *base)
> +static int __init exynos4_timer_resources(struct device_node *np)
> {
> - int err, cpu;
> struct clk *mct_clk, *tick_clk;
>
> + reg_base = of_iomap(np, 0);
> + if (!reg_base)
> + panic("%s: unable to ioremap mct address space\n", __func__);
> +
> tick_clk = of_clk_get_by_name(np, "fin_pll");
> if (IS_ERR(tick_clk))
> panic("%s: unable to determine tick clock rate\n", __func__);
> @@ -517,9 +520,27 @@ static int __init exynos4_timer_resources(struct device_node *np, void __iomem *
> panic("%s: unable to retrieve mct clock instance\n", __func__);
> clk_prepare_enable(mct_clk);
>
> - reg_base = base;
> - if (!reg_base)
> - panic("%s: unable to ioremap mct address space\n", __func__);
> + return 0;
> +}
> +
> +static int __init exynos4_timer_interrupts(struct device_node *np,
> + unsigned int int_type)
> +{
> + int i, err, cpu;
> +
> + mct_int_type = int_type;

This does not look good. Before, assignment was done before call to
exynos4_timer_resources(). Now, if I follow the path correctly, it
will be after. Therefore the exynos4_timer_resources() will use wrong
value. This should pop up during tests.

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);
> +
> + /*
> + * Find out the number of local irqs specified. The local
> + * timer irqs are specified after the four global timer
> + * irqs are specified.
> + */
> + nr_irqs = of_irq_count(np);
> + for (i = MCT_L0_IRQ; i < nr_irqs; i++)
> + mct_irqs[i] = irq_of_parse_and_map(np, i);
>
> if (mct_int_type == MCT_INT_PPI) {
>
> @@ -579,24 +600,14 @@ static int __init exynos4_timer_resources(struct device_node *np, void __iomem *
>
> static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
> {
> - u32 nr_irqs, i;
> int ret;
>
> - mct_int_type = int_type;
> -
> - /* This driver uses only one global timer interrupt */
> - mct_irqs[MCT_G0_IRQ] = irq_of_parse_and_map(np, MCT_G0_IRQ);
> + ret = exynos4_timer_resources(np);
> + if (ret)
> + return ret;
>
> - /*
> - * Find out the number of local irqs specified. The local
> - * timer irqs are specified after the four global timer
> - * irqs are specified.
> - */
> - nr_irqs = of_irq_count(np);
> - for (i = MCT_L0_IRQ; i < nr_irqs; i++)
> - mct_irqs[i] = irq_of_parse_and_map(np, i);
>
> - ret = exynos4_timer_resources(np, of_iomap(np, 0));
> + ret = exynos4_timer_interrupts(np, int_type);
> if (ret)
> return ret;
>
> --
> 2.17.1
>

2018-10-18 03:52:02

by Chanwoo Choi

[permalink] [raw]
Subject: Re: [PATCH v3 4/6] clocksource: exynos_mct: Add arch_timer cooperation mode for ARM64

Hi Marek,

On 2018년 10월 17일 22:41, Marek Szyprowski 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 (global system counter). 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 MCT driver only enables its clocks and starts
> global timer. Everything else will be handled by arch_timer driver.
>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> drivers/clocksource/exynos_mct.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index 02ad55db390b..1b19a4f03929 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -606,6 +606,15 @@ static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
> if (ret)
> return ret;
>
> + 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) {
> + of_node_put(np);
> + exynos4_mct_frc_start();
> + return 0;
> + }
> + }
>
> ret = exynos4_timer_interrupts(np, int_type);
> if (ret)
>

I tested it on Exynos5433-based TM2 board.

Reviewed-by: Chanwoo Choi <[email protected]>
Tested-by: Chanwoo Choi <[email protected]>


--
Best Regards,
Chanwoo Choi
Samsung Electronics

2018-10-18 04:00:16

by Chanwoo Choi

[permalink] [raw]
Subject: Re: [PATCH v3 3/6] clocksource: exynos_mct: Refactor resources allocation

Hi Marek,

I tested it for both kernel booting and cpu hotplugging
on Exynos5433-based TM2 board.

Reviewed-by: Chanwoo Choi <[email protected]>
Tested-by: Chanwoo Choi <[email protected]>

On 2018년 10월 17일 22:41, Marek Szyprowski wrote:
> Move interrupts allocation from exynos4_timer_resources() into separate
> function together with the interrupt number parsing code from
> mct_init_dt(), so the code for managing interrupts is kept together.
> While touching exynos4_timer_resources() function, move of_iomap() to it.
> No functional changes.
>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> drivers/clocksource/exynos_mct.c | 49 +++++++++++++++++++-------------
> 1 file changed, 30 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index 49413900b24c..02ad55db390b 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -502,11 +502,14 @@ static int exynos4_mct_dying_cpu(unsigned int cpu)
> return 0;
> }
>
> -static int __init exynos4_timer_resources(struct device_node *np, void __iomem *base)
> +static int __init exynos4_timer_resources(struct device_node *np)
> {
> - int err, cpu;
> struct clk *mct_clk, *tick_clk;
>
> + reg_base = of_iomap(np, 0);
> + if (!reg_base)
> + panic("%s: unable to ioremap mct address space\n", __func__);
> +
> tick_clk = of_clk_get_by_name(np, "fin_pll");
> if (IS_ERR(tick_clk))
> panic("%s: unable to determine tick clock rate\n", __func__);
> @@ -517,9 +520,27 @@ static int __init exynos4_timer_resources(struct device_node *np, void __iomem *
> panic("%s: unable to retrieve mct clock instance\n", __func__);
> clk_prepare_enable(mct_clk);
>
> - reg_base = base;
> - if (!reg_base)
> - panic("%s: unable to ioremap mct address space\n", __func__);
> + return 0;
> +}
> +
> +static int __init exynos4_timer_interrupts(struct device_node *np,
> + unsigned int int_type)
> +{
> + int i, err, cpu;
> +
> + mct_int_type = int_type;
> +
> + /* This driver uses only one global timer interrupt */
> + mct_irqs[MCT_G0_IRQ] = irq_of_parse_and_map(np, MCT_G0_IRQ);
> +
> + /*
> + * Find out the number of local irqs specified. The local
> + * timer irqs are specified after the four global timer
> + * irqs are specified.
> + */
> + nr_irqs = of_irq_count(np);
> + for (i = MCT_L0_IRQ; i < nr_irqs; i++)
> + mct_irqs[i] = irq_of_parse_and_map(np, i);
>
> if (mct_int_type == MCT_INT_PPI) {
>
> @@ -579,24 +600,14 @@ static int __init exynos4_timer_resources(struct device_node *np, void __iomem *
>
> static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
> {
> - u32 nr_irqs, i;
> int ret;
>
> - mct_int_type = int_type;
> -
> - /* This driver uses only one global timer interrupt */
> - mct_irqs[MCT_G0_IRQ] = irq_of_parse_and_map(np, MCT_G0_IRQ);
> + ret = exynos4_timer_resources(np);
> + if (ret)
> + return ret;
>
> - /*
> - * Find out the number of local irqs specified. The local
> - * timer irqs are specified after the four global timer
> - * irqs are specified.
> - */
> - nr_irqs = of_irq_count(np);
> - for (i = MCT_L0_IRQ; i < nr_irqs; i++)
> - mct_irqs[i] = irq_of_parse_and_map(np, i);
>
> - ret = exynos4_timer_resources(np, of_iomap(np, 0));
> + ret = exynos4_timer_interrupts(np, int_type);
> if (ret)
> return ret;
>
>


--
Best Regards,
Chanwoo Choi
Samsung Electronics

2018-10-18 09:40:53

by Marek Szyprowski

[permalink] [raw]
Subject: Re: [PATCH v3 3/6] clocksource: exynos_mct: Refactor resources allocation

Hi Krzysztof,

On 2018-10-17 16:29, Krzysztof Kozlowski wrote:
> On Wed, 17 Oct 2018 at 15:42, Marek Szyprowski <[email protected]> wrote:
>> Move interrupts allocation from exynos4_timer_resources() into separate
>> function together with the interrupt number parsing code from
>> mct_init_dt(), so the code for managing interrupts is kept together.
>> While touching exynos4_timer_resources() function, move of_iomap() to it.
>> No functional changes.
>>
>> Signed-off-by: Marek Szyprowski <[email protected]>
>> ---
>> drivers/clocksource/exynos_mct.c | 49 +++++++++++++++++++-------------
>> 1 file changed, 30 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
>> index 49413900b24c..02ad55db390b 100644
>> --- a/drivers/clocksource/exynos_mct.c
>> +++ b/drivers/clocksource/exynos_mct.c
>> @@ -502,11 +502,14 @@ static int exynos4_mct_dying_cpu(unsigned int cpu)
>> return 0;
>> }
>>
>> -static int __init exynos4_timer_resources(struct device_node *np, void __iomem *base)
>> +static int __init exynos4_timer_resources(struct device_node *np)
>> {
>> - int err, cpu;
>> struct clk *mct_clk, *tick_clk;
>>
>> + reg_base = of_iomap(np, 0);
>> + if (!reg_base)
>> + panic("%s: unable to ioremap mct address space\n", __func__);
>> +
>> tick_clk = of_clk_get_by_name(np, "fin_pll");
>> if (IS_ERR(tick_clk))
>> panic("%s: unable to determine tick clock rate\n", __func__);
>> @@ -517,9 +520,27 @@ static int __init exynos4_timer_resources(struct device_node *np, void __iomem *
>> panic("%s: unable to retrieve mct clock instance\n", __func__);
>> clk_prepare_enable(mct_clk);
>>
>> - reg_base = base;
>> - if (!reg_base)
>> - panic("%s: unable to ioremap mct address space\n", __func__);
>> + return 0;
>> +}
>> +
>> +static int __init exynos4_timer_interrupts(struct device_node *np,
>> + unsigned int int_type)
>> +{
>> + int i, err, cpu;
>> +
>> + mct_int_type = int_type;
> This does not look good. Before, assignment was done before call to
> exynos4_timer_resources(). Now, if I follow the path correctly, it
> will be after. Therefore the exynos4_timer_resources() will use wrong
> value. This should pop up during tests.

After refactoring exynos4_timer_resource() doesn't use int_type, but you
are right, there is one item missing: nr_irqs as local variable, what
causes the driver to mess with the global one, what is fatal to the
system. I will send a fixed version in a few minutes.

> ...

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland