This series add SMP brinup support for mediatek SoCs. This is based
on v4.1-rc1.
There are 2 similar but different SMP bringup up methods on Mediatek
mt65xx and mt81xx. On MT8135 & MT8127, system boots with a trustzone
firmware. Others, like MT6589, doesn't have trustzone, and run kernel
directly in secure world.
Patch 1,2 are preparing patch, cleanup/fix in secondary_startup_arm
Patch 3~5 fix issues in mtk_timer(GPT) and enable arch timer support.
Patch 6,7 add support for cpu enable-method "mediatek,mt65xx-smp" and
"mediatek,mt81xx-tz-smp", which support Mediatek SMP bringup for non-TZ
and TZ platform.
Patch 8,9 finally enable SMP bringup for mt8135 and mt8127.
Changes in v2:
- Fix boot issue for THUMB2 kernel.
- Not enable GPT_CLK_EVT when setup to fix GPT spurious interrupt issue
- Change platsmp.c according to Matthias' suggestion
v1:
http://lists.infradead.org/pipermail/linux-mediatek/2015-May/000528.html
Matthias Brugger (1):
ARM: mediatek: enable gpt6 on boot up to make arch timer working
Yingjoe Chen (8):
ARM: correct secondary_startup_arm mode
ARM: add secondary_startup_arm prototype in header file
clocksource: mediatek: do not enable GPT_CLK_EVT when setup
clocksource: mediatek: Use GPT as sched clock source
devicetree: bindings: add new SMP enable method Mediatek SoC
ARM: mediatek: add smp bringup code
ARM: dts: mt8135: enable basic SMP bringup for mt8135
ARM: dts: mt8127: enable basic SMP bringup for mt8127
Documentation/devicetree/bindings/arm/cpus.txt | 2 +
arch/arm/boot/dts/mt8127.dtsi | 16 +++
arch/arm/boot/dts/mt8135.dtsi | 16 +++
arch/arm/include/asm/smp.h | 1 +
arch/arm/kernel/head.S | 2 +-
arch/arm/mach-mediatek/Makefile | 3 +
arch/arm/mach-mediatek/mediatek.c | 29 +++++
arch/arm/mach-mediatek/platsmp.c | 144 +++++++++++++++++++++++++
drivers/clocksource/mtk_timer.c | 25 ++++-
9 files changed, 232 insertions(+), 6 deletions(-)
create mode 100644 arch/arm/mach-mediatek/platsmp.c
secondary_startup_arm is used as ARM mode secondary start up function
when ther kernel is compiled in THUMB mode, however the label itself
is still in .thumb mode. readelf shows:
160979: c020a581 120 FUNC GLOBAL DEFAULT 2 secondary_startup_arm
Make sure the label is in ARM mode as well.
Signed-off-by: Yingjoe Chen <[email protected]>
---
arch/arm/kernel/head.S | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
index 3637973..58ee8a2 100644
--- a/arch/arm/kernel/head.S
+++ b/arch/arm/kernel/head.S
@@ -346,8 +346,8 @@ __turn_mmu_on_loc:
#if defined(CONFIG_SMP)
.text
-ENTRY(secondary_startup_arm)
.arm
+ENTRY(secondary_startup_arm)
THUMB( adr r9, BSYM(1f) ) @ Kernel is entered in ARM.
THUMB( bx r9 ) @ If this is a Thumb-2 kernel,
THUMB( .thumb ) @ switch to Thumb now.
--
1.8.1.1.dirty
Put secondary_startup_arm() prototype in arch/arm/include/asm/smp.h
so users doesn't have to add extern prototype in their code.
Signed-off-by: Yingjoe Chen <[email protected]>
---
arch/arm/include/asm/smp.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/include/asm/smp.h b/arch/arm/include/asm/smp.h
index 18f5a55..f5cffb7 100644
--- a/arch/arm/include/asm/smp.h
+++ b/arch/arm/include/asm/smp.h
@@ -69,6 +69,7 @@ struct secondary_data {
extern struct secondary_data secondary_data;
extern volatile int pen_release;
extern void secondary_startup(void);
+extern void secondary_startup_arm(void);
extern int __cpu_disable(void);
--
1.8.1.1.dirty
Spurious mtk timer interrupt is noticed at boot and cause kernel
crash. It seems if GPT is enabled, it will latch irq status even
when its IRQ is disabled. When irq is enabled afterward, we see
spurious interrupt.
Change init flow to only enable GPT_CLK_SRC at mtk_timer_init.
Signed-off-by: Yingjoe Chen <[email protected]>
---
drivers/clocksource/mtk_timer.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
index 68ab423..91206f9 100644
--- a/drivers/clocksource/mtk_timer.c
+++ b/drivers/clocksource/mtk_timer.c
@@ -157,8 +157,11 @@ static void mtk_timer_global_reset(struct mtk_clock_event_device *evt)
}
static void
-mtk_timer_setup(struct mtk_clock_event_device *evt, u8 timer, u8 option)
+mtk_timer_setup(struct mtk_clock_event_device *evt,
+ u8 timer, u8 option, u8 enable)
{
+ u32 val;
+
writel(TIMER_CTRL_CLEAR | TIMER_CTRL_DISABLE,
evt->gpt_base + TIMER_CTRL_REG(timer));
@@ -167,8 +170,10 @@ mtk_timer_setup(struct mtk_clock_event_device *evt, u8 timer, u8 option)
writel(0x0, evt->gpt_base + TIMER_CMP_REG(timer));
- writel(TIMER_CTRL_OP(option) | TIMER_CTRL_ENABLE,
- evt->gpt_base + TIMER_CTRL_REG(timer));
+ val = TIMER_CTRL_OP(option);
+ if (enable)
+ val |= TIMER_CTRL_ENABLE;
+ writel(val, evt->gpt_base + TIMER_CTRL_REG(timer));
}
static void mtk_timer_enable_irq(struct mtk_clock_event_device *evt, u8 timer)
@@ -235,12 +240,12 @@ static void __init mtk_timer_init(struct device_node *node)
evt->ticks_per_jiffy = DIV_ROUND_UP(rate, HZ);
/* Configure clock source */
- mtk_timer_setup(evt, GPT_CLK_SRC, TIMER_CTRL_OP_FREERUN);
+ mtk_timer_setup(evt, GPT_CLK_SRC, TIMER_CTRL_OP_FREERUN, 1);
clocksource_mmio_init(evt->gpt_base + TIMER_CNT_REG(GPT_CLK_SRC),
node->name, rate, 300, 32, clocksource_mmio_readl_up);
/* Configure clock event */
- mtk_timer_setup(evt, GPT_CLK_EVT, TIMER_CTRL_OP_REPEAT);
+ mtk_timer_setup(evt, GPT_CLK_EVT, TIMER_CTRL_OP_REPEAT, 0);
clockevents_config_and_register(&evt->dev, rate, 0x3,
0xffffffff);
--
1.8.1.1.dirty
When cpu is in deep idle, arch timer will stop counting. Setup GPT as
sched clock source so it can keep counting in idle.
Signed-off-by: Yingjoe Chen <[email protected]>
---
drivers/clocksource/mtk_timer.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
index 91206f9..fe7cf72 100644
--- a/drivers/clocksource/mtk_timer.c
+++ b/drivers/clocksource/mtk_timer.c
@@ -24,6 +24,7 @@
#include <linux/of.h>
#include <linux/of_address.h>
#include <linux/of_irq.h>
+#include <linux/sched_clock.h>
#include <linux/slab.h>
#define GPT_IRQ_EN_REG 0x00
@@ -59,6 +60,13 @@ struct mtk_clock_event_device {
struct clock_event_device dev;
};
+static void __iomem *gpt_base __read_mostly;
+
+static u64 notrace mtk_read_sched_clock(void)
+{
+ return readl_relaxed(gpt_base + TIMER_CNT_REG(GPT_CLK_SRC));
+}
+
static inline struct mtk_clock_event_device *to_mtk_clk(
struct clock_event_device *c)
{
@@ -243,6 +251,8 @@ static void __init mtk_timer_init(struct device_node *node)
mtk_timer_setup(evt, GPT_CLK_SRC, TIMER_CTRL_OP_FREERUN, 1);
clocksource_mmio_init(evt->gpt_base + TIMER_CNT_REG(GPT_CLK_SRC),
node->name, rate, 300, 32, clocksource_mmio_readl_up);
+ gpt_base = evt->gpt_base;
+ sched_clock_register(mtk_read_sched_clock, 32, rate);
/* Configure clock event */
mtk_timer_setup(evt, GPT_CLK_EVT, TIMER_CTRL_OP_REPEAT, 0);
--
1.8.1.1.dirty
From: Matthias Brugger <[email protected]>
We enable GTP6 which ungates the arch timer clock.
In the future this should be done in the bootloader.
Signed-off-by: Matthias Brugger <[email protected]>
Signed-off-by: Yingjoe Chen <[email protected]>
---
arch/arm/mach-mediatek/mediatek.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/arch/arm/mach-mediatek/mediatek.c b/arch/arm/mach-mediatek/mediatek.c
index a954900..6b38d67 100644
--- a/arch/arm/mach-mediatek/mediatek.c
+++ b/arch/arm/mach-mediatek/mediatek.c
@@ -16,6 +16,34 @@
*/
#include <linux/init.h>
#include <asm/mach/arch.h>
+#include <linux/of.h>
+#include <linux/clk-provider.h>
+#include <linux/clocksource.h>
+
+
+#define GPT6_CON_MT65xx 0x10008060
+#define GPT_ENABLE 0x31
+
+static void __init mediatek_timer_init(void)
+{
+ void __iomem *gpt_base = 0;
+
+ if (of_machine_is_compatible("mediatek,mt6589") ||
+ of_machine_is_compatible("mediatek,mt8135") ||
+ of_machine_is_compatible("mediatek,mt8127")) {
+ /* turn on GPT6 which ungates arch timer clocks */
+ gpt_base = ioremap(GPT6_CON_MT65xx, 0x04);
+ }
+
+ /* enabel clock and set to free-run */
+ if (gpt_base) {
+ writel(GPT_ENABLE, gpt_base);
+ iounmap(gpt_base);
+ }
+
+ of_clk_init(NULL);
+ clocksource_of_init();
+};
static const char * const mediatek_board_dt_compat[] = {
"mediatek,mt6589",
@@ -27,4 +55,5 @@ static const char * const mediatek_board_dt_compat[] = {
DT_MACHINE_START(MEDIATEK_DT, "Mediatek Cortex-A7 (Device Tree)")
.dt_compat = mediatek_board_dt_compat,
+ .init_time = mediatek_timer_init,
MACHINE_END
--
1.8.1.1.dirty
This commit add new cpu enable method "mediatek,mt65xx-smp" and
"mediatek,mt81xx-tz-smp".
Signed-off-by: Yingjoe Chen <[email protected]>
---
Documentation/devicetree/bindings/arm/cpus.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
index 6aa331d..ac2903d 100644
--- a/Documentation/devicetree/bindings/arm/cpus.txt
+++ b/Documentation/devicetree/bindings/arm/cpus.txt
@@ -194,6 +194,8 @@ nodes to be present and contain the properties described below.
"marvell,armada-380-smp"
"marvell,armada-390-smp"
"marvell,armada-xp-smp"
+ "mediatek,mt65xx-smp"
+ "mediatek,mt81xx-tz-smp"
"qcom,gcc-msm8660"
"qcom,kpss-acc-v1"
"qcom,kpss-acc-v2"
--
1.8.1.1.dirty
Add support for booting secondary CPUs on mt6589, mt8127
and mt8135.
Signed-off-by: Yingjoe Chen <[email protected]>
---
arch/arm/mach-mediatek/Makefile | 3 +
arch/arm/mach-mediatek/platsmp.c | 144 +++++++++++++++++++++++++++++++++++++++
2 files changed, 147 insertions(+)
create mode 100644 arch/arm/mach-mediatek/platsmp.c
diff --git a/arch/arm/mach-mediatek/Makefile b/arch/arm/mach-mediatek/Makefile
index 43e619f..2116460 100644
--- a/arch/arm/mach-mediatek/Makefile
+++ b/arch/arm/mach-mediatek/Makefile
@@ -1 +1,4 @@
+ifeq ($(CONFIG_SMP),y)
+obj-$(CONFIG_ARCH_MEDIATEK) += platsmp.o
+endif
obj-$(CONFIG_ARCH_MEDIATEK) += mediatek.o
diff --git a/arch/arm/mach-mediatek/platsmp.c b/arch/arm/mach-mediatek/platsmp.c
new file mode 100644
index 0000000..b3cf75f
--- /dev/null
+++ b/arch/arm/mach-mediatek/platsmp.c
@@ -0,0 +1,144 @@
+/*
+ * arch/arm/mach-mediatek/platsmp.c
+ *
+ * Copyright (c) 2014 Mediatek Inc.
+ * Author: Shunli Wang <[email protected]>
+ * Yingjoe Chen <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+#include <linux/io.h>
+#include <linux/memblock.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/string.h>
+#include <linux/threads.h>
+
+#define MTK_MAX_CPU 8
+#define MTK_SMP_REG_SIZE 0x1000
+
+struct mtk_smp_boot_info {
+ unsigned long smp_base;
+ unsigned int jump_reg;
+ unsigned int core_keys[MTK_MAX_CPU - 1];
+ unsigned int core_regs[MTK_MAX_CPU - 1];
+};
+
+static const struct mtk_smp_boot_info mtk_mt8135_tz_boot = {
+ 0x80002000, 0x3fc,
+ { 0x534c4131, 0x4c415332, 0x41534c33 },
+ { 0x3f8, 0x3f8, 0x3f8 },
+};
+
+static const struct mtk_smp_boot_info mtk_mt6589_boot = {
+ 0x10002000, 0x34,
+ { 0x534c4131, 0x4c415332, 0x41534c33 },
+ { 0x38, 0x3c, 0x40 },
+};
+
+static const struct of_device_id mtk_tz_smp_boot_infos[] __initconst = {
+ { .compatible = "mediatek,mt8135", .data = &mtk_mt8135_tz_boot },
+ { .compatible = "mediatek,mt8127", .data = &mtk_mt8135_tz_boot },
+};
+
+static const struct of_device_id mtk_smp_boot_infos[] __initconst = {
+ { .compatible = "mediatek,mt6589", .data = &mtk_mt6589_boot },
+};
+
+static void __iomem *mtk_smp_base;
+static const struct mtk_smp_boot_info *mtk_smp_info;
+
+static int mtk_boot_secondary(unsigned int cpu, struct task_struct *idle)
+{
+ if (!mtk_smp_base)
+ return -EINVAL;
+
+ if (!mtk_smp_info->core_keys[cpu-1])
+ return -EINVAL;
+
+ writel_relaxed(mtk_smp_info->core_keys[cpu-1],
+ mtk_smp_base + mtk_smp_info->core_regs[cpu-1]);
+
+ arch_send_wakeup_ipi_mask(cpumask_of(cpu));
+
+ return 0;
+}
+
+static void __init __mtk_smp_prepare_cpus(unsigned int max_cpus, int trustzone)
+{
+ int i, num;
+ const struct of_device_id *infos;
+
+ if (trustzone) {
+ num = ARRAY_SIZE(mtk_tz_smp_boot_infos);
+ infos = mtk_tz_smp_boot_infos;
+ } else {
+ num = ARRAY_SIZE(mtk_smp_boot_infos);
+ infos = mtk_smp_boot_infos;
+ }
+
+ /* Find smp boot info for this SoC */
+ for (i = 0; i < num; i++) {
+ if (of_machine_is_compatible(infos[i].compatible)) {
+ mtk_smp_info = infos[i].data;
+ break;
+ }
+ }
+
+ if (!mtk_smp_info) {
+ pr_err("%s: Device is not supported\n", __func__);
+ return;
+ }
+
+ if (trustzone) {
+ if (memblock_reserve(mtk_smp_info->smp_base, MTK_SMP_REG_SIZE)) {
+ pr_err("%s: Can't reserve smp memory\n", __func__);
+ return;
+ }
+ mtk_smp_base = phys_to_virt(mtk_smp_info->smp_base);
+ } else {
+ mtk_smp_base = ioremap(mtk_smp_info->smp_base, MTK_SMP_REG_SIZE);
+ if (!mtk_smp_base) {
+ pr_err("%s: Can't remap %lx\n", __func__,
+ mtk_smp_info->smp_base);
+ return;
+ }
+ }
+
+ /*
+ * write the address of slave startup address into the system-wide
+ * jump register
+ */
+ writel_relaxed(virt_to_phys(secondary_startup_arm),
+ mtk_smp_base + mtk_smp_info->jump_reg);
+}
+
+static void __init mtk_tz_smp_prepare_cpus(unsigned int max_cpus)
+{
+ __mtk_smp_prepare_cpus(max_cpus, 1);
+}
+
+static void __init mtk_smp_prepare_cpus(unsigned int max_cpus)
+{
+ __mtk_smp_prepare_cpus(max_cpus, 0);
+}
+
+static struct smp_operations mt81xx_tz_smp_ops __initdata = {
+ .smp_prepare_cpus = mtk_tz_smp_prepare_cpus,
+ .smp_boot_secondary = mtk_boot_secondary,
+};
+CPU_METHOD_OF_DECLARE(mt81xx_tz_smp, "mediatek,mt81xx-tz-smp", &mt81xx_tz_smp_ops);
+
+static struct smp_operations mt65xx_smp_ops __initdata = {
+ .smp_prepare_cpus = mtk_smp_prepare_cpus,
+ .smp_boot_secondary = mtk_boot_secondary,
+};
+CPU_METHOD_OF_DECLARE(mt65xx_smp, "mediatek,mt65xx-smp", &mt65xx_smp_ops);
--
1.8.1.1.dirty
Add arch timer node to enable arch-timer support. MT8135 firmware
doesn't correctly setup arch-timer frequency and CNTVOFF, add
properties to workaround this.
This also set cpu enable-method to enable SMP.
Signed-off-by: Yingjoe Chen <[email protected]>
---
arch/arm/boot/dts/mt8135.dtsi | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/arch/arm/boot/dts/mt8135.dtsi b/arch/arm/boot/dts/mt8135.dtsi
index a161e99..cc8a8c9 100644
--- a/arch/arm/boot/dts/mt8135.dtsi
+++ b/arch/arm/boot/dts/mt8135.dtsi
@@ -43,6 +43,7 @@
cpus {
#address-cells = <1>;
#size-cells = <0>;
+ enable-method = "mediatek,mt81xx-tz-smp";
cpu0: cpu@0 {
device_type = "cpu";
@@ -95,6 +96,21 @@
};
+ timer {
+ compatible = "arm,armv7-timer";
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) |
+ IRQ_TYPE_LEVEL_LOW)>,
+ <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) |
+ IRQ_TYPE_LEVEL_LOW)>,
+ <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) |
+ IRQ_TYPE_LEVEL_LOW)>,
+ <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) |
+ IRQ_TYPE_LEVEL_LOW)>;
+ clock-frequency = <13000000>;
+ arm,cpu-registers-not-fw-configured;
+ };
+
soc {
#address-cells = <2>;
#size-cells = <2>;
--
1.8.1.1.dirty
Add arch timer node to enable arch-timer support. MT8127 firmware
doesn't correctly setup arch-timer frequency and CNTVOFF, add
properties to workaround this.
This also set cpu enable-method to enable SMP.
Signed-off-by: Yingjoe Chen <[email protected]>
---
arch/arm/boot/dts/mt8127.dtsi | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/arch/arm/boot/dts/mt8127.dtsi b/arch/arm/boot/dts/mt8127.dtsi
index aaa7862..7c2090d 100644
--- a/arch/arm/boot/dts/mt8127.dtsi
+++ b/arch/arm/boot/dts/mt8127.dtsi
@@ -23,6 +23,7 @@
cpus {
#address-cells = <1>;
#size-cells = <0>;
+ enable-method = "mediatek,mt81xx-tz-smp";
cpu@0 {
device_type = "cpu";
@@ -72,6 +73,21 @@
};
};
+ timer {
+ compatible = "arm,armv7-timer";
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) |
+ IRQ_TYPE_LEVEL_LOW)>,
+ <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) |
+ IRQ_TYPE_LEVEL_LOW)>,
+ <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) |
+ IRQ_TYPE_LEVEL_LOW)>,
+ <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) |
+ IRQ_TYPE_LEVEL_LOW)>;
+ clock-frequency = <13000000>;
+ arm,cpu-registers-not-fw-configured;
+ };
+
soc {
#address-cells = <2>;
#size-cells = <2>;
--
1.8.1.1.dirty
2015-05-16 9:58 GMT+02:00 Yingjoe Chen <[email protected]>:
> Put secondary_startup_arm() prototype in arch/arm/include/asm/smp.h
> so users doesn't have to add extern prototype in their code.
>
> Signed-off-by: Yingjoe Chen <[email protected]>
> ---
> arch/arm/include/asm/smp.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/include/asm/smp.h b/arch/arm/include/asm/smp.h
> index 18f5a55..f5cffb7 100644
> --- a/arch/arm/include/asm/smp.h
> +++ b/arch/arm/include/asm/smp.h
> @@ -69,6 +69,7 @@ struct secondary_data {
> extern struct secondary_data secondary_data;
> extern volatile int pen_release;
> extern void secondary_startup(void);
> +extern void secondary_startup_arm(void);
Reviewed-by: Matthias Brugger <[email protected]>
--
motzblog.wordpress.com
2015-05-16 9:58 GMT+02:00 Yingjoe Chen <[email protected]>:
> secondary_startup_arm is used as ARM mode secondary start up function
> when ther kernel is compiled in THUMB mode, however the label itself
> is still in .thumb mode. readelf shows:
>
> 160979: c020a581 120 FUNC GLOBAL DEFAULT 2 secondary_startup_arm
>
> Make sure the label is in ARM mode as well.
>
> Signed-off-by: Yingjoe Chen <[email protected]>
> ---
> arch/arm/kernel/head.S | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
> index 3637973..58ee8a2 100644
> --- a/arch/arm/kernel/head.S
> +++ b/arch/arm/kernel/head.S
> @@ -346,8 +346,8 @@ __turn_mmu_on_loc:
>
> #if defined(CONFIG_SMP)
> .text
> -ENTRY(secondary_startup_arm)
> .arm
> +ENTRY(secondary_startup_arm)
> THUMB( adr r9, BSYM(1f) ) @ Kernel is entered in ARM.
> THUMB( bx r9 ) @ If this is a Thumb-2 kernel,
> THUMB( .thumb ) @ switch to Thumb now.
This fixes and issue we had with SMP on mt6589.
Tested-by: Matthias Brugger <[email protected]>
--
motzblog.wordpress.com
On Sat, May 16, 2015 at 11:08:36AM +0200, Matthias Brugger wrote:
> 2015-05-16 9:58 GMT+02:00 Yingjoe Chen <[email protected]>:
> > secondary_startup_arm is used as ARM mode secondary start up function
> > when ther kernel is compiled in THUMB mode, however the label itself
> > is still in .thumb mode. readelf shows:
> >
> > 160979: c020a581 120 FUNC GLOBAL DEFAULT 2 secondary_startup_arm
> >
> > Make sure the label is in ARM mode as well.
> >
> > Signed-off-by: Yingjoe Chen <[email protected]>
> > ---
> > arch/arm/kernel/head.S | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
> > index 3637973..58ee8a2 100644
> > --- a/arch/arm/kernel/head.S
> > +++ b/arch/arm/kernel/head.S
> > @@ -346,8 +346,8 @@ __turn_mmu_on_loc:
> >
> > #if defined(CONFIG_SMP)
> > .text
> > -ENTRY(secondary_startup_arm)
> > .arm
> > +ENTRY(secondary_startup_arm)
> > THUMB( adr r9, BSYM(1f) ) @ Kernel is entered in ARM.
> > THUMB( bx r9 ) @ If this is a Thumb-2 kernel,
> > THUMB( .thumb ) @ switch to Thumb now.
>
> This fixes and issue we had with SMP on mt6589.
>
> Tested-by: Matthias Brugger <[email protected]>
Please ensure that this patch ends up in the patch system so it doesn't
get forgotten about, thanks.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
On Sat, May 16, 2015 at 11:02:40AM +0200, Matthias Brugger wrote:
> 2015-05-16 9:58 GMT+02:00 Yingjoe Chen <[email protected]>:
> > Put secondary_startup_arm() prototype in arch/arm/include/asm/smp.h
> > so users doesn't have to add extern prototype in their code.
> >
> > Signed-off-by: Yingjoe Chen <[email protected]>
> > ---
> > arch/arm/include/asm/smp.h | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/arm/include/asm/smp.h b/arch/arm/include/asm/smp.h
> > index 18f5a55..f5cffb7 100644
> > --- a/arch/arm/include/asm/smp.h
> > +++ b/arch/arm/include/asm/smp.h
> > @@ -69,6 +69,7 @@ struct secondary_data {
> > extern struct secondary_data secondary_data;
> > extern volatile int pen_release;
> > extern void secondary_startup(void);
> > +extern void secondary_startup_arm(void);
>
> Reviewed-by: Matthias Brugger <[email protected]>
This change should also end up in the patch system as its core ARM stuff,
especially if it's not scheduled for merging through arm-soc.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
On Sat, 2015-05-16 at 22:19 +0100, Russell King - ARM Linux wrote:
> On Sat, May 16, 2015 at 11:02:40AM +0200, Matthias Brugger wrote:
> > 2015-05-16 9:58 GMT+02:00 Yingjoe Chen <[email protected]>:
> > > Put secondary_startup_arm() prototype in arch/arm/include/asm/smp.h
> > > so users doesn't have to add extern prototype in their code.
> > >
> > > Signed-off-by: Yingjoe Chen <[email protected]>
> > > ---
> > > arch/arm/include/asm/smp.h | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/arch/arm/include/asm/smp.h b/arch/arm/include/asm/smp.h
> > > index 18f5a55..f5cffb7 100644
> > > --- a/arch/arm/include/asm/smp.h
> > > +++ b/arch/arm/include/asm/smp.h
> > > @@ -69,6 +69,7 @@ struct secondary_data {
> > > extern struct secondary_data secondary_data;
> > > extern volatile int pen_release;
> > > extern void secondary_startup(void);
> > > +extern void secondary_startup_arm(void);
> >
> > Reviewed-by: Matthias Brugger <[email protected]>
>
> This change should also end up in the patch system as its core ARM stuff,
> especially if it's not scheduled for merging through arm-soc.
>
Russell,
Thanks. I just put these 2 patches into the patch system.
Since patch 7/9 depends on this patch, I think we should merge this
patch(2/9) through arm-soc tree to prevent build fail.
Joe.C
On 05/16/15 00:58, Yingjoe Chen wrote:
> diff --git a/arch/arm/mach-mediatek/mediatek.c b/arch/arm/mach-mediatek/mediatek.c
> index a954900..6b38d67 100644
> --- a/arch/arm/mach-mediatek/mediatek.c
> +++ b/arch/arm/mach-mediatek/mediatek.c
> @@ -16,6 +16,34 @@
> */
> #include <linux/init.h>
> #include <asm/mach/arch.h>
> +#include <linux/of.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clocksource.h>
> +
> +
> +#define GPT6_CON_MT65xx 0x10008060
> +#define GPT_ENABLE 0x31
> +
> +static void __init mediatek_timer_init(void)
> +{
> + void __iomem *gpt_base = 0;
= NULL ?
> +
> + if (of_machine_is_compatible("mediatek,mt6589") ||
> + of_machine_is_compatible("mediatek,mt8135") ||
> + of_machine_is_compatible("mediatek,mt8127")) {
> + /* turn on GPT6 which ungates arch timer clocks */
> + gpt_base = ioremap(GPT6_CON_MT65xx, 0x04);
> + }
> +
> + /* enabel clock and set to free-run */
s/enabel/enable/
> + if (gpt_base) {
> + writel(GPT_ENABLE, gpt_base);
> + iounmap(gpt_base);
> + }
> +
Why not join the two conditions together?
> + of_clk_init(NULL);
> + clocksource_of_init();
> +};
>
> static const char * const mediatek_board_dt_compat[] = {
> "mediatek,mt6589",
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
2015-05-19 21:51 GMT+02:00 Stephen Boyd <[email protected]>:
> On 05/16/15 00:58, Yingjoe Chen wrote:
>> diff --git a/arch/arm/mach-mediatek/mediatek.c b/arch/arm/mach-mediatek/mediatek.c
>> index a954900..6b38d67 100644
>> --- a/arch/arm/mach-mediatek/mediatek.c
>> +++ b/arch/arm/mach-mediatek/mediatek.c
>> @@ -16,6 +16,34 @@
>> */
>> #include <linux/init.h>
>> #include <asm/mach/arch.h>
>> +#include <linux/of.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/clocksource.h>
>> +
>> +
>> +#define GPT6_CON_MT65xx 0x10008060
>> +#define GPT_ENABLE 0x31
>> +
>> +static void __init mediatek_timer_init(void)
>> +{
>> + void __iomem *gpt_base = 0;
>
> = NULL ?
>
>> +
>> + if (of_machine_is_compatible("mediatek,mt6589") ||
>> + of_machine_is_compatible("mediatek,mt8135") ||
>> + of_machine_is_compatible("mediatek,mt8127")) {
>> + /* turn on GPT6 which ungates arch timer clocks */
>> + gpt_base = ioremap(GPT6_CON_MT65xx, 0x04);
>> + }
>> +
>> + /* enabel clock and set to free-run */
>
> s/enabel/enable/
>
>> + if (gpt_base) {
>> + writel(GPT_ENABLE, gpt_base);
>> + iounmap(gpt_base);
>> + }
>> +
>
> Why not join the two conditions together?
The initial patch supposed that on different SoCs GPT6_CON register
would be mapped on different addresses, but that seems not the case.
So yes, we can join the two conditions together.
Yingjoe, can you add the changes in the next series or do you want me to do it?
Thanks,
Matthias
--
motzblog.wordpress.com
On Wed, 2015-05-20 at 10:43 +0200, Matthias Brugger wrote:
> 2015-05-19 21:51 GMT+02:00 Stephen Boyd <[email protected]>:
> > On 05/16/15 00:58, Yingjoe Chen wrote:
> >> diff --git a/arch/arm/mach-mediatek/mediatek.c b/arch/arm/mach-mediatek/mediatek.c
> >> index a954900..6b38d67 100644
> >> --- a/arch/arm/mach-mediatek/mediatek.c
> >> +++ b/arch/arm/mach-mediatek/mediatek.c
> >> @@ -16,6 +16,34 @@
> >> */
> >> #include <linux/init.h>
> >> #include <asm/mach/arch.h>
> >> +#include <linux/of.h>
> >> +#include <linux/clk-provider.h>
> >> +#include <linux/clocksource.h>
> >> +
> >> +
> >> +#define GPT6_CON_MT65xx 0x10008060
> >> +#define GPT_ENABLE 0x31
> >> +
> >> +static void __init mediatek_timer_init(void)
> >> +{
> >> + void __iomem *gpt_base = 0;
> >
> > = NULL ?
> >
> >> +
> >> + if (of_machine_is_compatible("mediatek,mt6589") ||
> >> + of_machine_is_compatible("mediatek,mt8135") ||
> >> + of_machine_is_compatible("mediatek,mt8127")) {
> >> + /* turn on GPT6 which ungates arch timer clocks */
> >> + gpt_base = ioremap(GPT6_CON_MT65xx, 0x04);
> >> + }
> >> +
> >> + /* enabel clock and set to free-run */
> >
> > s/enabel/enable/
> >
> >> + if (gpt_base) {
> >> + writel(GPT_ENABLE, gpt_base);
> >> + iounmap(gpt_base);
> >> + }
> >> +
> >
> > Why not join the two conditions together?
>
> The initial patch supposed that on different SoCs GPT6_CON register
> would be mapped on different addresses, but that seems not the case.
> So yes, we can join the two conditions together.
>
> Yingjoe, can you add the changes in the next series or do you want me to do it?
Hi,
I'll change these in next series. Thanks.
Joe.C
2015-05-16 9:58 GMT+02:00 Yingjoe Chen <[email protected]>:
> Spurious mtk timer interrupt is noticed at boot and cause kernel
> crash. It seems if GPT is enabled, it will latch irq status even
> when its IRQ is disabled. When irq is enabled afterward, we see
> spurious interrupt.
> Change init flow to only enable GPT_CLK_SRC at mtk_timer_init.
>
> Signed-off-by: Yingjoe Chen <[email protected]>
> ---
> drivers/clocksource/mtk_timer.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
> index 68ab423..91206f9 100644
> --- a/drivers/clocksource/mtk_timer.c
> +++ b/drivers/clocksource/mtk_timer.c
> @@ -157,8 +157,11 @@ static void mtk_timer_global_reset(struct mtk_clock_event_device *evt)
> }
>
> static void
> -mtk_timer_setup(struct mtk_clock_event_device *evt, u8 timer, u8 option)
> +mtk_timer_setup(struct mtk_clock_event_device *evt,
> + u8 timer, u8 option, u8 enable)
Nicer would be:
static void mtk_timer_setup(struct mtk_clock_event_device *evt, u8 timer,
u8 option, u8 enable)
Apart from that parameter enable should be of type bool.
Thanks,
Matthias
--
motzblog.wordpress.com
2015-05-16 9:58 GMT+02:00 Yingjoe Chen <[email protected]>:
> When cpu is in deep idle, arch timer will stop counting. Setup GPT as
> sched clock source so it can keep counting in idle.
>
> Signed-off-by: Yingjoe Chen <[email protected]>
> ---
> drivers/clocksource/mtk_timer.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
> index 91206f9..fe7cf72 100644
> --- a/drivers/clocksource/mtk_timer.c
> +++ b/drivers/clocksource/mtk_timer.c
> @@ -24,6 +24,7 @@
> #include <linux/of.h>
> #include <linux/of_address.h>
> #include <linux/of_irq.h>
> +#include <linux/sched_clock.h>
> #include <linux/slab.h>
>
> #define GPT_IRQ_EN_REG 0x00
> @@ -59,6 +60,13 @@ struct mtk_clock_event_device {
> struct clock_event_device dev;
> };
>
> +static void __iomem *gpt_base __read_mostly;
> +
> +static u64 notrace mtk_read_sched_clock(void)
> +{
> + return readl_relaxed(gpt_base + TIMER_CNT_REG(GPT_CLK_SRC));
> +}
> +
> static inline struct mtk_clock_event_device *to_mtk_clk(
> struct clock_event_device *c)
> {
> @@ -243,6 +251,8 @@ static void __init mtk_timer_init(struct device_node *node)
> mtk_timer_setup(evt, GPT_CLK_SRC, TIMER_CTRL_OP_FREERUN, 1);
> clocksource_mmio_init(evt->gpt_base + TIMER_CNT_REG(GPT_CLK_SRC),
> node->name, rate, 300, 32, clocksource_mmio_readl_up);
> + gpt_base = evt->gpt_base;
This is really hacky. We should clean up the code and provide
mtk_clock_event_device globally.
Please add the patch below, which does exactly this.
---- 8< ---------------- >8 ------
>From 631e7bf4e5d9456d0bb4a29b2dee4b84e8c052bd Mon Sep 17 00:00:00 2001
From: Matthias Brugger <[email protected]>
Date: Wed, 20 May 2015 12:43:16 +0200
Subject: [PATCH] clocksource: mediatek: Define mtk_clock_event_device globally
Sched clock code, especially sched_clock_register does not allow to pass a
pointer to actual_read_sched_clock. So if in the driver the register base
address is not globally defined, we are not able to read the scheduler
clock register. This patch sets the mtk_clock_event_device struct globally
for the driver, to be able to read the register.
Signed-off-by: Matthias Brugger <[email protected]>
---
drivers/clocksource/mtk_timer.c | 50 +++++++++++++++--------------------------
1 file changed, 18 insertions(+), 32 deletions(-)
diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
index 68ab423..c5f5b40 100644
--- a/drivers/clocksource/mtk_timer.c
+++ b/drivers/clocksource/mtk_timer.c
@@ -59,13 +59,9 @@ struct mtk_clock_event_device {
struct clock_event_device dev;
};
-static inline struct mtk_clock_event_device *to_mtk_clk(
- struct clock_event_device *c)
-{
- return container_of(c, struct mtk_clock_event_device, dev);
-}
+struct mtk_clock_event_device *evt;
-static void mtk_clkevt_time_stop(struct mtk_clock_event_device *evt, u8 timer)
+static void mtk_clkevt_time_stop(u8 timer)
{
u32 val;
@@ -74,14 +70,12 @@ static void mtk_clkevt_time_stop(struct
mtk_clock_event_device *evt, u8 timer)
TIMER_CTRL_REG(timer));
}
-static void mtk_clkevt_time_setup(struct mtk_clock_event_device *evt,
- unsigned long delay, u8 timer)
+static void mtk_clkevt_time_setup(unsigned long delay, u8 timer)
{
writel(delay, evt->gpt_base + TIMER_CMP_REG(timer));
}
-static void mtk_clkevt_time_start(struct mtk_clock_event_device *evt,
- bool periodic, u8 timer)
+static void mtk_clkevt_time_start(bool periodic, u8 timer)
{
u32 val;
@@ -105,14 +99,12 @@ static void mtk_clkevt_time_start(struct
mtk_clock_event_device *evt,
static void mtk_clkevt_mode(enum clock_event_mode mode,
struct clock_event_device *clk)
{
- struct mtk_clock_event_device *evt = to_mtk_clk(clk);
-
- mtk_clkevt_time_stop(evt, GPT_CLK_EVT);
+ mtk_clkevt_time_stop(GPT_CLK_EVT);
switch (mode) {
case CLOCK_EVT_MODE_PERIODIC:
- mtk_clkevt_time_setup(evt, evt->ticks_per_jiffy, GPT_CLK_EVT);
- mtk_clkevt_time_start(evt, true, GPT_CLK_EVT);
+ mtk_clkevt_time_setup(evt->ticks_per_jiffy, GPT_CLK_EVT);
+ mtk_clkevt_time_start(true, GPT_CLK_EVT);
break;
case CLOCK_EVT_MODE_ONESHOT:
/* Timer is enabled in set_next_event */
@@ -128,19 +120,15 @@ static void mtk_clkevt_mode(enum clock_event_mode mode,
static int mtk_clkevt_next_event(unsigned long event,
struct clock_event_device *clk)
{
- struct mtk_clock_event_device *evt = to_mtk_clk(clk);
-
- mtk_clkevt_time_stop(evt, GPT_CLK_EVT);
- mtk_clkevt_time_setup(evt, event, GPT_CLK_EVT);
- mtk_clkevt_time_start(evt, false, GPT_CLK_EVT);
+ mtk_clkevt_time_stop(GPT_CLK_EVT);
+ mtk_clkevt_time_setup(event, GPT_CLK_EVT);
+ mtk_clkevt_time_start(false, GPT_CLK_EVT);
return 0;
}
static irqreturn_t mtk_timer_interrupt(int irq, void *dev_id)
{
- struct mtk_clock_event_device *evt = dev_id;
-
/* Acknowledge timer0 irq */
writel(GPT_IRQ_ACK(GPT_CLK_EVT), evt->gpt_base + GPT_IRQ_ACK_REG);
evt->dev.event_handler(&evt->dev);
@@ -148,7 +136,7 @@ static irqreturn_t mtk_timer_interrupt(int irq,
void *dev_id)
return IRQ_HANDLED;
}
-static void mtk_timer_global_reset(struct mtk_clock_event_device *evt)
+static inline void mtk_timer_global_reset(void)
{
/* Disable all interrupts */
writel(0x0, evt->gpt_base + GPT_IRQ_EN_REG);
@@ -156,8 +144,7 @@ static void mtk_timer_global_reset(struct
mtk_clock_event_device *evt)
writel(0x3f, evt->gpt_base + GPT_IRQ_ACK_REG);
}
-static void
-mtk_timer_setup(struct mtk_clock_event_device *evt, u8 timer, u8 option)
+static void mtk_timer_setup(u8 timer, u8 option)
{
writel(TIMER_CTRL_CLEAR | TIMER_CTRL_DISABLE,
evt->gpt_base + TIMER_CTRL_REG(timer));
@@ -171,7 +158,7 @@ mtk_timer_setup(struct mtk_clock_event_device
*evt, u8 timer, u8 option)
evt->gpt_base + TIMER_CTRL_REG(timer));
}
-static void mtk_timer_enable_irq(struct mtk_clock_event_device *evt, u8 timer)
+static void mtk_timer_enable_irq(u8 timer)
{
u32 val;
@@ -182,7 +169,6 @@ static void mtk_timer_enable_irq(struct
mtk_clock_event_device *evt, u8 timer)
static void __init mtk_timer_init(struct device_node *node)
{
- struct mtk_clock_event_device *evt;
struct resource res;
unsigned long rate = 0;
struct clk *clk;
@@ -224,10 +210,10 @@ static void __init mtk_timer_init(struct
device_node *node)
}
rate = clk_get_rate(clk);
- mtk_timer_global_reset(evt);
+ mtk_timer_global_reset();
if (request_irq(evt->dev.irq, mtk_timer_interrupt,
- IRQF_TIMER | IRQF_IRQPOLL, "mtk_timer", evt)) {
+ IRQF_TIMER | IRQF_IRQPOLL, "mtk_timer", NULL)) {
pr_warn("failed to setup irq %d\n", evt->dev.irq);
goto err_clk_disable;
}
@@ -235,16 +221,16 @@ static void __init mtk_timer_init(struct
device_node *node)
evt->ticks_per_jiffy = DIV_ROUND_UP(rate, HZ);
/* Configure clock source */
- mtk_timer_setup(evt, GPT_CLK_SRC, TIMER_CTRL_OP_FREERUN);
+ mtk_timer_setup(GPT_CLK_SRC, TIMER_CTRL_OP_FREERUN);
clocksource_mmio_init(evt->gpt_base + TIMER_CNT_REG(GPT_CLK_SRC),
node->name, rate, 300, 32, clocksource_mmio_readl_up);
/* Configure clock event */
- mtk_timer_setup(evt, GPT_CLK_EVT, TIMER_CTRL_OP_REPEAT);
+ mtk_timer_setup(GPT_CLK_EVT, TIMER_CTRL_OP_REPEAT);
clockevents_config_and_register(&evt->dev, rate, 0x3,
0xffffffff);
- mtk_timer_enable_irq(evt, GPT_CLK_EVT);
+ mtk_timer_enable_irq(GPT_CLK_EVT);
return;
--
1.9.1
On Wed, 2015-05-20 at 13:02 +0200, Matthias Brugger wrote:
> 2015-05-16 9:58 GMT+02:00 Yingjoe Chen <[email protected]>:
> > When cpu is in deep idle, arch timer will stop counting. Setup GPT as
> > sched clock source so it can keep counting in idle.
> >
> > Signed-off-by: Yingjoe Chen <[email protected]>
> > ---
> > drivers/clocksource/mtk_timer.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
> > index 91206f9..fe7cf72 100644
> > --- a/drivers/clocksource/mtk_timer.c
> > +++ b/drivers/clocksource/mtk_timer.c
> > @@ -24,6 +24,7 @@
> > #include <linux/of.h>
> > #include <linux/of_address.h>
> > #include <linux/of_irq.h>
> > +#include <linux/sched_clock.h>
> > #include <linux/slab.h>
> >
> > #define GPT_IRQ_EN_REG 0x00
> > @@ -59,6 +60,13 @@ struct mtk_clock_event_device {
> > struct clock_event_device dev;
> > };
> >
> > +static void __iomem *gpt_base __read_mostly;
> > +
> > +static u64 notrace mtk_read_sched_clock(void)
> > +{
> > + return readl_relaxed(gpt_base + TIMER_CNT_REG(GPT_CLK_SRC));
> > +}
> > +
> > static inline struct mtk_clock_event_device *to_mtk_clk(
> > struct clock_event_device *c)
> > {
> > @@ -243,6 +251,8 @@ static void __init mtk_timer_init(struct device_node *node)
> > mtk_timer_setup(evt, GPT_CLK_SRC, TIMER_CTRL_OP_FREERUN, 1);
> > clocksource_mmio_init(evt->gpt_base + TIMER_CNT_REG(GPT_CLK_SRC),
> > node->name, rate, 300, 32, clocksource_mmio_readl_up);
> > + gpt_base = evt->gpt_base;
>
> This is really hacky. We should clean up the code and provide
> mtk_clock_event_device globally.
> Please add the patch below, which does exactly this.
> ---- 8< ---------------- >8 ------
> From 631e7bf4e5d9456d0bb4a29b2dee4b84e8c052bd Mon Sep 17 00:00:00 2001
> From: Matthias Brugger <[email protected]>
> Date: Wed, 20 May 2015 12:43:16 +0200
> Subject: [PATCH] clocksource: mediatek: Define mtk_clock_event_device globally
>
> Sched clock code, especially sched_clock_register does not allow to pass a
> pointer to actual_read_sched_clock. So if in the driver the register base
> address is not globally defined, we are not able to read the scheduler
> clock register. This patch sets the mtk_clock_event_device struct globally
> for the driver, to be able to read the register.
Hi,
I'm not sure using a global device pointer is any better.
Actually, almost every user of sched_clock_register need to keep a
global register base address. Does it make sense to fix this in
sched_clock_register?
> Signed-off-by: Matthias Brugger <[email protected]>
> ---
> drivers/clocksource/mtk_timer.c | 50 +++++++++++++++--------------------------
> 1 file changed, 18 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
> index 68ab423..c5f5b40 100644
> --- a/drivers/clocksource/mtk_timer.c
> +++ b/drivers/clocksource/mtk_timer.c
> @@ -59,13 +59,9 @@ struct mtk_clock_event_device {
> struct clock_event_device dev;
> };
>
> -static inline struct mtk_clock_event_device *to_mtk_clk(
> - struct clock_event_device *c)
> -{
> - return container_of(c, struct mtk_clock_event_device, dev);
> -}
> +struct mtk_clock_event_device *evt;
The name is too short even if we make it static:
static struct mtk_clock_event_device *evt __read_mostly;
Joe.C
2015-05-20 16:05 GMT+02:00 Yingjoe Chen <[email protected]>:
> On Wed, 2015-05-20 at 13:02 +0200, Matthias Brugger wrote:
>> 2015-05-16 9:58 GMT+02:00 Yingjoe Chen <[email protected]>:
>> > When cpu is in deep idle, arch timer will stop counting. Setup GPT as
>> > sched clock source so it can keep counting in idle.
>> >
>> > Signed-off-by: Yingjoe Chen <[email protected]>
>> > ---
>> > drivers/clocksource/mtk_timer.c | 10 ++++++++++
>> > 1 file changed, 10 insertions(+)
>> >
>> > diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
>> > index 91206f9..fe7cf72 100644
>> > --- a/drivers/clocksource/mtk_timer.c
>> > +++ b/drivers/clocksource/mtk_timer.c
>> > @@ -24,6 +24,7 @@
>> > #include <linux/of.h>
>> > #include <linux/of_address.h>
>> > #include <linux/of_irq.h>
>> > +#include <linux/sched_clock.h>
>> > #include <linux/slab.h>
>> >
>> > #define GPT_IRQ_EN_REG 0x00
>> > @@ -59,6 +60,13 @@ struct mtk_clock_event_device {
>> > struct clock_event_device dev;
>> > };
>> >
>> > +static void __iomem *gpt_base __read_mostly;
>> > +
>> > +static u64 notrace mtk_read_sched_clock(void)
>> > +{
>> > + return readl_relaxed(gpt_base + TIMER_CNT_REG(GPT_CLK_SRC));
>> > +}
>> > +
>> > static inline struct mtk_clock_event_device *to_mtk_clk(
>> > struct clock_event_device *c)
>> > {
>> > @@ -243,6 +251,8 @@ static void __init mtk_timer_init(struct device_node *node)
>> > mtk_timer_setup(evt, GPT_CLK_SRC, TIMER_CTRL_OP_FREERUN, 1);
>> > clocksource_mmio_init(evt->gpt_base + TIMER_CNT_REG(GPT_CLK_SRC),
>> > node->name, rate, 300, 32, clocksource_mmio_readl_up);
>> > + gpt_base = evt->gpt_base;
>>
>> This is really hacky. We should clean up the code and provide
>> mtk_clock_event_device globally.
>> Please add the patch below, which does exactly this.
>> ---- 8< ---------------- >8 ------
>> From 631e7bf4e5d9456d0bb4a29b2dee4b84e8c052bd Mon Sep 17 00:00:00 2001
>> From: Matthias Brugger <[email protected]>
>> Date: Wed, 20 May 2015 12:43:16 +0200
>> Subject: [PATCH] clocksource: mediatek: Define mtk_clock_event_device globally
>>
>> Sched clock code, especially sched_clock_register does not allow to pass a
>> pointer to actual_read_sched_clock. So if in the driver the register base
>> address is not globally defined, we are not able to read the scheduler
>> clock register. This patch sets the mtk_clock_event_device struct globally
>> for the driver, to be able to read the register.
>
> Hi,
>
> I'm not sure using a global device pointer is any better.
Why not?
>
> Actually, almost every user of sched_clock_register need to keep a
> global register base address. Does it make sense to fix this in
> sched_clock_register?
>
Not sure about that. Normally you have just one timer in your system,
so you don't get any problems to declare driver private structs
globally on a per driver basis.
Looking on other drivers I have seen a bit of everything, no struct at
all, a global pointer to the driver private struct, only iomem
pointers global and the rest of the driver private struct as is.
It seems that there is no uniform way so up to you. Just please don't
use two times the exactly same iomem pointer, one in the driver
private struct and one for the scheduler clock. :)
Thanks,
Matthias
--
motzblog.wordpress.com
On 05/20, Matthias Brugger wrote:
> 2015-05-20 16:05 GMT+02:00 Yingjoe Chen <[email protected]>:
> > On Wed, 2015-05-20 at 13:02 +0200, Matthias Brugger wrote:
> >> 2015-05-16 9:58 GMT+02:00 Yingjoe Chen <[email protected]>:
> >> > When cpu is in deep idle, arch timer will stop counting. Setup GPT as
> >> > sched clock source so it can keep counting in idle.
> >> >
> >> > Signed-off-by: Yingjoe Chen <[email protected]>
> >> > ---
> >> > drivers/clocksource/mtk_timer.c | 10 ++++++++++
> >> > 1 file changed, 10 insertions(+)
> >> >
> >> > diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
> >> > index 91206f9..fe7cf72 100644
> >> > --- a/drivers/clocksource/mtk_timer.c
> >> > +++ b/drivers/clocksource/mtk_timer.c
> >> > @@ -24,6 +24,7 @@
> >> > #include <linux/of.h>
> >> > #include <linux/of_address.h>
> >> > #include <linux/of_irq.h>
> >> > +#include <linux/sched_clock.h>
> >> > #include <linux/slab.h>
> >> >
> >> > #define GPT_IRQ_EN_REG 0x00
> >> > @@ -59,6 +60,13 @@ struct mtk_clock_event_device {
> >> > struct clock_event_device dev;
> >> > };
> >> >
> >> > +static void __iomem *gpt_base __read_mostly;
> >> > +
> >> > +static u64 notrace mtk_read_sched_clock(void)
> >> > +{
> >> > + return readl_relaxed(gpt_base + TIMER_CNT_REG(GPT_CLK_SRC));
> >> > +}
> >> > +
> >> > static inline struct mtk_clock_event_device *to_mtk_clk(
> >> > struct clock_event_device *c)
> >> > {
> >> > @@ -243,6 +251,8 @@ static void __init mtk_timer_init(struct device_node *node)
> >> > mtk_timer_setup(evt, GPT_CLK_SRC, TIMER_CTRL_OP_FREERUN, 1);
> >> > clocksource_mmio_init(evt->gpt_base + TIMER_CNT_REG(GPT_CLK_SRC),
> >> > node->name, rate, 300, 32, clocksource_mmio_readl_up);
> >> > + gpt_base = evt->gpt_base;
> >>
> >> This is really hacky. We should clean up the code and provide
> >> mtk_clock_event_device globally.
> >> Please add the patch below, which does exactly this.
> >> ---- 8< ---------------- >8 ------
> >> From 631e7bf4e5d9456d0bb4a29b2dee4b84e8c052bd Mon Sep 17 00:00:00 2001
> >> From: Matthias Brugger <[email protected]>
> >> Date: Wed, 20 May 2015 12:43:16 +0200
> >> Subject: [PATCH] clocksource: mediatek: Define mtk_clock_event_device globally
> >>
> >> Sched clock code, especially sched_clock_register does not allow to pass a
> >> pointer to actual_read_sched_clock. So if in the driver the register base
> >> address is not globally defined, we are not able to read the scheduler
> >> clock register. This patch sets the mtk_clock_event_device struct globally
> >> for the driver, to be able to read the register.
> >
> > Hi,
> >
> > I'm not sure using a global device pointer is any better.
>
> Why not?
>
> >
> > Actually, almost every user of sched_clock_register need to keep a
> > global register base address. Does it make sense to fix this in
> > sched_clock_register?
> >
>
> Not sure about that. Normally you have just one timer in your system,
> so you don't get any problems to declare driver private structs
> globally on a per driver basis.
> Looking on other drivers I have seen a bit of everything, no struct at
> all, a global pointer to the driver private struct, only iomem
> pointers global and the rest of the driver private struct as is.
> It seems that there is no uniform way so up to you. Just please don't
> use two times the exactly same iomem pointer, one in the driver
> private struct and one for the scheduler clock. :)
>
Typically the point of doing the container_of design on the
clockevent is to keep cache locality of all the data. This way,
when the clockevent is programmed it's cheap pointer math on a
pointer that's hot in the cache instead of a global pointer
dereference and likely cache miss to find the data necessary.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Wed, May 20, 2015 at 7:02 PM, Matthias Brugger
<[email protected]> wrote:
> 2015-05-16 9:58 GMT+02:00 Yingjoe Chen <[email protected]>:
>> When cpu is in deep idle, arch timer will stop counting. Setup GPT as
>> sched clock source so it can keep counting in idle.
>>
>> Signed-off-by: Yingjoe Chen <[email protected]>
>> ---
>> drivers/clocksource/mtk_timer.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
>> index 91206f9..fe7cf72 100644
>> --- a/drivers/clocksource/mtk_timer.c
>> +++ b/drivers/clocksource/mtk_timer.c
>> @@ -24,6 +24,7 @@
>> #include <linux/of.h>
>> #include <linux/of_address.h>
>> #include <linux/of_irq.h>
>> +#include <linux/sched_clock.h>
>> #include <linux/slab.h>
>>
>> #define GPT_IRQ_EN_REG 0x00
>> @@ -59,6 +60,13 @@ struct mtk_clock_event_device {
>> struct clock_event_device dev;
>> };
>>
>> +static void __iomem *gpt_base __read_mostly;
>> +
>> +static u64 notrace mtk_read_sched_clock(void)
>> +{
>> + return readl_relaxed(gpt_base + TIMER_CNT_REG(GPT_CLK_SRC));
>> +}
>> +
>> static inline struct mtk_clock_event_device *to_mtk_clk(
>> struct clock_event_device *c)
>> {
>> @@ -243,6 +251,8 @@ static void __init mtk_timer_init(struct device_node *node)
>> mtk_timer_setup(evt, GPT_CLK_SRC, TIMER_CTRL_OP_FREERUN, 1);
>> clocksource_mmio_init(evt->gpt_base + TIMER_CNT_REG(GPT_CLK_SRC),
>> node->name, rate, 300, 32, clocksource_mmio_readl_up);
>> + gpt_base = evt->gpt_base;
>
> This is really hacky. We should clean up the code and provide
> mtk_clock_event_device globally.
> Please add the patch below, which does exactly this.
I don't think this is so hacky.
In light of Stephen's comment about the benefit of using
container_of() to extract gpt_base from the passed in struct
clock_event_device in the other routines, what is the benefit of
making more of mtk_clock_event_device global?
I think what Yingjoe has implemented is short and sweet.
Reviewed-by: Daniel Kurtz <[email protected]>
On Friday, July 03, 2015 09:48:42 PM Daniel Kurtz wrote:
> On Wed, May 20, 2015 at 7:02 PM, Matthias Brugger
>
> <[email protected]> wrote:
> > 2015-05-16 9:58 GMT+02:00 Yingjoe Chen <[email protected]>:
> >> When cpu is in deep idle, arch timer will stop counting. Setup GPT as
> >> sched clock source so it can keep counting in idle.
> >>
> >> Signed-off-by: Yingjoe Chen <[email protected]>
> >> ---
> >>
> >> drivers/clocksource/mtk_timer.c | 10 ++++++++++
> >> 1 file changed, 10 insertions(+)
> >>
> >> diff --git a/drivers/clocksource/mtk_timer.c
> >> b/drivers/clocksource/mtk_timer.c index 91206f9..fe7cf72 100644
> >> --- a/drivers/clocksource/mtk_timer.c
> >> +++ b/drivers/clocksource/mtk_timer.c
> >> @@ -24,6 +24,7 @@
> >>
> >> #include <linux/of.h>
> >> #include <linux/of_address.h>
> >> #include <linux/of_irq.h>
> >>
> >> +#include <linux/sched_clock.h>
> >>
> >> #include <linux/slab.h>
> >>
> >> #define GPT_IRQ_EN_REG 0x00
> >>
> >> @@ -59,6 +60,13 @@ struct mtk_clock_event_device {
> >>
> >> struct clock_event_device dev;
> >>
> >> };
> >>
> >> +static void __iomem *gpt_base __read_mostly;
> >> +
> >> +static u64 notrace mtk_read_sched_clock(void)
> >> +{
> >> + return readl_relaxed(gpt_base + TIMER_CNT_REG(GPT_CLK_SRC));
> >> +}
> >> +
> >>
> >> static inline struct mtk_clock_event_device *to_mtk_clk(
> >>
> >> struct clock_event_device *c)
> >>
> >> {
> >>
> >> @@ -243,6 +251,8 @@ static void __init mtk_timer_init(struct device_node
> >> *node)>>
> >> mtk_timer_setup(evt, GPT_CLK_SRC, TIMER_CTRL_OP_FREERUN, 1);
> >> clocksource_mmio_init(evt->gpt_base + TIMER_CNT_REG(GPT_CLK_SRC),
> >>
> >> node->name, rate, 300, 32,
> >> clocksource_mmio_readl_up);
> >>
> >> + gpt_base = evt->gpt_base;
> >
> > This is really hacky. We should clean up the code and provide
> > mtk_clock_event_device globally.
> > Please add the patch below, which does exactly this.
>
> I don't think this is so hacky.
> In light of Stephen's comment about the benefit of using
> container_of() to extract gpt_base from the passed in struct
> clock_event_device in the other routines, what is the benefit of
> making more of mtk_clock_event_device global?
> I think what Yingjoe has implemented is short and sweet.
>
Huh, this patch got somehow forgotten.
Ok, just one comment. I would prefer to rename the global gpt_base to
gpt_sched_base or something similar and set the pointer + offset directly
mtk_timer_init.
Thanks,
Matthias
On Fri, 2015-07-03 at 17:51 +0200, Matthias Brugger wrote:
> On Friday, July 03, 2015 09:48:42 PM Daniel Kurtz wrote:
<...>
> > >> @@ -243,6 +251,8 @@ static void __init mtk_timer_init(struct device_node
> > >> *node)>>
> > >> mtk_timer_setup(evt, GPT_CLK_SRC, TIMER_CTRL_OP_FREERUN, 1);
> > >> clocksource_mmio_init(evt->gpt_base + TIMER_CNT_REG(GPT_CLK_SRC),
> > >>
> > >> node->name, rate, 300, 32,
> > >> clocksource_mmio_readl_up);
> > >>
> > >> + gpt_base = evt->gpt_base;
> > >
> > > This is really hacky. We should clean up the code and provide
> > > mtk_clock_event_device globally.
> > > Please add the patch below, which does exactly this.
> >
> > I don't think this is so hacky.
> > In light of Stephen's comment about the benefit of using
> > container_of() to extract gpt_base from the passed in struct
> > clock_event_device in the other routines, what is the benefit of
> > making more of mtk_clock_event_device global?
> > I think what Yingjoe has implemented is short and sweet.
> >
>
> Huh, this patch got somehow forgotten.
> Ok, just one comment. I would prefer to rename the global gpt_base to
> gpt_sched_base or something similar and set the pointer + offset directly
> mtk_timer_init.
In next version, I will it change to:
static void __iomem *gpt_sched_reg __read_mostly;
and in mtk_timer_init:
+ gpt_sched_reg = evt->gpt_base + TIMER_CNT_REG(GPT_CLK_SRC);
Joe.C
On 05/16/2015 09:58 AM, Yingjoe Chen wrote:
> When cpu is in deep idle, arch timer will stop counting. Setup GPT as
> sched clock source so it can keep counting in idle.
>
> Signed-off-by: Yingjoe Chen <[email protected]>
> ---
Acked-by: Daniel Lezcano <[email protected]>
--
<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