This series add GPT timer support for mt8173. This is based on v4.2-rc1
and Matthias' next branch (for dts parts).
The first 2 patches comes from 'Add SMP bringup support for mt65xx socs'
series [1]. I decide to move these 2 patches to this series, since it
is more relevent here. They are changed based on Matthias' and Daniel's
comments.
[1] http://lists.infradead.org/pipermail/linux-mediatek/2015-May/000714.html
Daniel Kurtz (1):
arm64: dts: mt8173: add timer node
Yingjoe Chen (4):
clocksource: mediatek: do not enable GPT_CLK_EVT when setup
clocksource: mediatek: Use GPT as sched clock source
arm64: mediatek: enable MTK_TIMER
clk: mediatek: add 13mhz clock for MT8173
arch/arm64/Kconfig | 1 +
arch/arm64/boot/dts/mediatek/mt8173.dtsi | 9 +++++++++
drivers/clk/mediatek/clk-mt8173.c | 5 +++++
drivers/clocksource/mtk_timer.c | 26 ++++++++++++++++++++------
include/dt-bindings/clock/mt8173-clk.h | 3 ++-
5 files changed, 37 insertions(+), 7 deletions(-)
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 | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
index 68ab423..237c20b 100644
--- a/drivers/clocksource/mtk_timer.c
+++ b/drivers/clocksource/mtk_timer.c
@@ -156,9 +156,11 @@ 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(struct mtk_clock_event_device *evt, u8 timer,
+ u8 option, bool enable)
{
+ u32 val;
+
writel(TIMER_CTRL_CLEAR | TIMER_CTRL_DISABLE,
evt->gpt_base + TIMER_CTRL_REG(timer));
@@ -167,8 +169,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 +239,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, true);
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, false);
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 237c20b..ae95b29 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_sched_reg __read_mostly;
+
+static u64 notrace mtk_read_sched_clock(void)
+{
+ return readl_relaxed(gpt_sched_reg);
+}
+
static inline struct mtk_clock_event_device *to_mtk_clk(
struct clock_event_device *c)
{
@@ -242,6 +250,8 @@ static void __init mtk_timer_init(struct device_node *node)
mtk_timer_setup(evt, GPT_CLK_SRC, TIMER_CTRL_OP_FREERUN, true);
clocksource_mmio_init(evt->gpt_base + TIMER_CNT_REG(GPT_CLK_SRC),
node->name, rate, 300, 32, clocksource_mmio_readl_up);
+ gpt_sched_reg = evt->gpt_base + TIMER_CNT_REG(GPT_CLK_SRC);
+ sched_clock_register(mtk_read_sched_clock, 32, rate);
/* Configure clock event */
mtk_timer_setup(evt, GPT_CLK_EVT, TIMER_CTRL_OP_REPEAT, false);
--
1.8.1.1.dirty
Enable MTK_TIMER for MediaTek plaform, which will be used as
schedule clock.
Signed-off-by: Yingjoe Chen <[email protected]>
---
arch/arm64/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 0f6edb1..5934f51 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -193,6 +193,7 @@ config ARCH_MEDIATEK
bool "Mediatek MT65xx & MT81xx ARMv8 SoC"
select ARM_GIC
select PINCTRL
+ select MTK_TIMER
help
Support for Mediatek MT65xx & MT81xx ARMv8 SoCs
--
1.8.1.1.dirty
Add 13mhz clock used by GPT timer in infracfg.
Signed-off-by: Yingjoe Chen <[email protected]>
---
drivers/clk/mediatek/clk-mt8173.c | 5 +++++
include/dt-bindings/clock/mt8173-clk.h | 3 ++-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c
index 4b9e04c..540c5c3 100644
--- a/drivers/clk/mediatek/clk-mt8173.c
+++ b/drivers/clk/mediatek/clk-mt8173.c
@@ -618,6 +618,10 @@ static const struct mtk_gate infra_clks[] __initconst = {
GATE_ICG(CLK_INFRA_PMICWRAP, "infra_pmicwrap", "axi_sel", 23),
};
+static const struct mtk_fixed_factor infra_divs[] __initconst = {
+ FACTOR(CLK_INFRA_CLK_13M, "clk13m", "clk26m", 1, 2),
+};
+
static const struct mtk_gate_regs peri0_cg_regs = {
.set_ofs = 0x0008,
.clr_ofs = 0x0010,
@@ -737,6 +741,7 @@ static void __init mtk_infrasys_init(struct device_node *node)
mtk_clk_register_gates(node, infra_clks, ARRAY_SIZE(infra_clks),
clk_data);
+ mtk_clk_register_factors(infra_divs, ARRAY_SIZE(infra_divs), clk_data);
r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
if (r)
diff --git a/include/dt-bindings/clock/mt8173-clk.h b/include/dt-bindings/clock/mt8173-clk.h
index 4ad76ed..fa2a2bb 100644
--- a/include/dt-bindings/clock/mt8173-clk.h
+++ b/include/dt-bindings/clock/mt8173-clk.h
@@ -187,7 +187,8 @@
#define CLK_INFRA_CEC 9
#define CLK_INFRA_PMICSPI 10
#define CLK_INFRA_PMICWRAP 11
-#define CLK_INFRA_NR_CLK 12
+#define CLK_INFRA_CLK_13M 12
+#define CLK_INFRA_NR_CLK 13
/* PERI_SYS */
--
1.8.1.1.dirty
From: Daniel Kurtz <[email protected]>
Add device node to enable GPT timer. This timer will be
used as sched clock source.
Signed-off-by: Daniel Kurtz <[email protected]>
Signed-off-by: Eddie Huang <[email protected]>
Signed-off-by: Yingjoe Chen <[email protected]>
---
arch/arm64/boot/dts/mediatek/mt8173.dtsi | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
index 0696f8f..04bdd8f 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
@@ -219,6 +219,15 @@
reg = <0 0x10007000 0 0x100>;
};
+ timer: timer@10008000 {
+ compatible = "mediatek,mt8173-timer",
+ "mediatek,mt6577-timer";
+ reg = <0 0x10008000 0 0x1000>;
+ interrupts = <GIC_SPI 144 IRQ_TYPE_LEVEL_LOW>;
+ clocks = <&infracfg CLK_INFRA_CLK_13M>,
+ <&topckgen CLK_TOP_RTC_SEL>;
+ };
+
pwrap: pwrap@1000d000 {
compatible = "mediatek,mt8173-pwrap";
reg = <0 0x1000d000 0 0x1000>;
--
1.8.1.1.dirty
On 07/13, Yingjoe Chen wrote:
> Add 13mhz clock used by GPT timer in infracfg.
>
> Signed-off-by: Yingjoe Chen <[email protected]>
> ---
Acked-by: Stephen Boyd <[email protected]>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 07/13, 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: Stephen Boyd <[email protected]>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Mon, Jul 13, 2015 at 5:32 PM, Yingjoe Chen <[email protected]> wrote:
> From: Daniel Kurtz <[email protected]>
>
> Add device node to enable GPT timer. This timer will be
> used as sched clock source.
>
> Signed-off-by: Daniel Kurtz <[email protected]>
> Signed-off-by: Eddie Huang <[email protected]>
> Signed-off-by: Yingjoe Chen <[email protected]>
This binding needs documentation.
> ---
> arch/arm64/boot/dts/mediatek/mt8173.dtsi | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> index 0696f8f..04bdd8f 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> @@ -219,6 +219,15 @@
> reg = <0 0x10007000 0 0x100>;
> };
>
> + timer: timer@10008000 {
> + compatible = "mediatek,mt8173-timer",
> + "mediatek,mt6577-timer";
> + reg = <0 0x10008000 0 0x1000>;
> + interrupts = <GIC_SPI 144 IRQ_TYPE_LEVEL_LOW>;
> + clocks = <&infracfg CLK_INFRA_CLK_13M>,
> + <&topckgen CLK_TOP_RTC_SEL>;
Why two clocks? The driver only uses one.
Please use a clock-names property.
Thanks,
-Dan
> + };
> +
> pwrap: pwrap@1000d000 {
> compatible = "mediatek,mt8173-pwrap";
> reg = <0 0x1000d000 0 0x1000>;
> --
> 1.8.1.1.dirty
>
On Tue, Jul 14, 2015 at 12:26 PM, Daniel Kurtz <[email protected]> wrote:
> On Mon, Jul 13, 2015 at 5:32 PM, Yingjoe Chen <[email protected]> wrote:
>> From: Daniel Kurtz <[email protected]>
>>
>> Add device node to enable GPT timer. This timer will be
>> used as sched clock source.
>>
>> Signed-off-by: Daniel Kurtz <[email protected]>
>> Signed-off-by: Eddie Huang <[email protected]>
>> Signed-off-by: Yingjoe Chen <[email protected]>
>
> This binding needs documentation.
Whoops. I just found it at:
Documentation/devicetree/bindings/timer/mediatek,mtk-timer.txt
>
>> ---
>> arch/arm64/boot/dts/mediatek/mt8173.dtsi | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>> index 0696f8f..04bdd8f 100644
>> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>> @@ -219,6 +219,15 @@
>> reg = <0 0x10007000 0 0x100>;
>> };
>>
>> + timer: timer@10008000 {
>> + compatible = "mediatek,mt8173-timer",
>> + "mediatek,mt6577-timer";
>> + reg = <0 0x10008000 0 0x1000>;
>> + interrupts = <GIC_SPI 144 IRQ_TYPE_LEVEL_LOW>;
>> + clocks = <&infracfg CLK_INFRA_CLK_13M>,
>> + <&topckgen CLK_TOP_RTC_SEL>;
>
> Why two clocks? The driver only uses one.
> Please use a clock-names property.
>
> Thanks,
> -Dan
>
>> + };
>> +
>> pwrap: pwrap@1000d000 {
>> compatible = "mediatek,mt8173-pwrap";
>> reg = <0 0x1000d000 0 0x1000>;
>> --
>> 1.8.1.1.dirty
>>
On Mon, Jul 13, 2015 at 5:32 PM, Yingjoe Chen <[email protected]> wrote:
> Add 13mhz clock used by GPT timer in infracfg.
>
> Signed-off-by: Yingjoe Chen <[email protected]>
> ---
> drivers/clk/mediatek/clk-mt8173.c | 5 +++++
> include/dt-bindings/clock/mt8173-clk.h | 3 ++-
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c
> index 4b9e04c..540c5c3 100644
> --- a/drivers/clk/mediatek/clk-mt8173.c
> +++ b/drivers/clk/mediatek/clk-mt8173.c
> @@ -618,6 +618,10 @@ static const struct mtk_gate infra_clks[] __initconst = {
> GATE_ICG(CLK_INFRA_PMICWRAP, "infra_pmicwrap", "axi_sel", 23),
> };
>
> +static const struct mtk_fixed_factor infra_divs[] __initconst = {
> + FACTOR(CLK_INFRA_CLK_13M, "clk13m", "clk26m", 1, 2),
> +};
> +
> static const struct mtk_gate_regs peri0_cg_regs = {
> .set_ofs = 0x0008,
> .clr_ofs = 0x0010,
> @@ -737,6 +741,7 @@ static void __init mtk_infrasys_init(struct device_node *node)
>
> mtk_clk_register_gates(node, infra_clks, ARRAY_SIZE(infra_clks),
> clk_data);
> + mtk_clk_register_factors(infra_divs, ARRAY_SIZE(infra_divs), clk_data);
>
> r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
> if (r)
> diff --git a/include/dt-bindings/clock/mt8173-clk.h b/include/dt-bindings/clock/mt8173-clk.h
> index 4ad76ed..fa2a2bb 100644
> --- a/include/dt-bindings/clock/mt8173-clk.h
> +++ b/include/dt-bindings/clock/mt8173-clk.h
> @@ -187,7 +187,8 @@
> #define CLK_INFRA_CEC 9
> #define CLK_INFRA_PMICSPI 10
> #define CLK_INFRA_PMICWRAP 11
> -#define CLK_INFRA_NR_CLK 12
> +#define CLK_INFRA_CLK_13M 12
> +#define CLK_INFRA_NR_CLK 13
Note: this one conflicts slightly with pi-cheng's patch that adds CPU
mux clocks for cpufreq driver, since they both add more INFRA clocks:
https://patchwork.kernel.org/patch/6721511/
>
> /* PERI_SYS */
>
> --
> 1.8.1.1.dirty
>
Hi Yingjoe,
On Mon, Jul 13, 2015 at 5:32 PM, Yingjoe Chen <[email protected]> wrote:
> 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 | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
> index 68ab423..237c20b 100644
> --- a/drivers/clocksource/mtk_timer.c
> +++ b/drivers/clocksource/mtk_timer.c
> @@ -156,9 +156,11 @@ 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(struct mtk_clock_event_device *evt, u8 timer,
> + u8 option, bool enable)
This function can be: __init
Other than this tiny nit, and the small potential conflict in patch 4,
this whole series is:
Reviewed-by: Daniel Kurtz <[email protected]>
(I do think it is a bit strange that the mediatek,mt6577-timer binding
does not use "clock-names", but that is independent of this patch
set).
Thanks!
> {
> + u32 val;
> +
> writel(TIMER_CTRL_CLEAR | TIMER_CTRL_DISABLE,
> evt->gpt_base + TIMER_CTRL_REG(timer));
>
> @@ -167,8 +169,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 +239,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, true);
> 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, false);
> clockevents_config_and_register(&evt->dev, rate, 0x3,
> 0xffffffff);
>
> --
> 1.8.1.1.dirty
>
On Monday, July 13, 2015 05:32:46 PM 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: Matthias Brugger <[email protected]>
On Monday, July 13, 2015 05:32:47 PM Yingjoe Chen wrote:
> Enable MTK_TIMER for MediaTek plaform, which will be used as
> schedule clock.
>
> Signed-off-by: Yingjoe Chen <[email protected]>
> ---
Applied, thanks.
On Monday, July 13, 2015 05:32:45 PM Yingjoe Chen wrote:
> 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]>
> ---
Acked-by: Matthias Brugger <[email protected]>
On Fri, 17 Jul 2015, Matthias Brugger wrote:
> On Monday, July 13, 2015 05:32:47 PM Yingjoe Chen wrote:
> > Enable MTK_TIMER for MediaTek plaform, which will be used as
> > schedule clock.
> >
> > Signed-off-by: Yingjoe Chen <[email protected]>
> > ---
>
> Applied, thanks.
So you enable the current code w/o the modifications required for this
to work?
Thanks,
tglx
On Saturday, July 18, 2015 09:31:53 AM Thomas Gleixner wrote:
> On Fri, 17 Jul 2015, Matthias Brugger wrote:
> > On Monday, July 13, 2015 05:32:47 PM Yingjoe Chen wrote:
> > > Enable MTK_TIMER for MediaTek plaform, which will be used as
> > > schedule clock.
> > >
> > > Signed-off-by: Yingjoe Chen <[email protected]>
> > > ---
> >
> > Applied, thanks.
>
> So you enable the current code w/o the modifications required for this
> to work?
>
Well as the timer is not defined in dts it should not break the board...
But yes you are right, I will drop this commit for now.
Thanks,
Matthias
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.
Acked-by: Matthias Brugger <[email protected]>
Reviewed-by: Daniel Kurtz <[email protected]>
Signed-off-by: Yingjoe Chen <[email protected]>
---
Update to my patch [1], added __init as Daniel suggest. This is the
only patch that need to change in that series, so I only sent this one.
http://lists.infradead.org/pipermail/linux-mediatek/2015-July/001545.html
drivers/clocksource/mtk_timer.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
index 68ab423..2ba5b66 100644
--- a/drivers/clocksource/mtk_timer.c
+++ b/drivers/clocksource/mtk_timer.c
@@ -156,9 +156,11 @@ 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 __init mtk_timer_setup(struct mtk_clock_event_device *evt,
+ u8 timer, u8 option, bool enable)
{
+ u32 val;
+
writel(TIMER_CTRL_CLEAR | TIMER_CTRL_DISABLE,
evt->gpt_base + TIMER_CTRL_REG(timer));
@@ -167,8 +169,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 +239,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, true);
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, false);
clockevents_config_and_register(&evt->dev, rate, 0x3,
0xffffffff);
--
1.8.1.1.dirty
On Tue, 2015-07-14 at 15:39 +0800, Daniel Kurtz wrote:
> Hi Yingjoe,
>
> On Mon, Jul 13, 2015 at 5:32 PM, Yingjoe Chen <[email protected]> wrote:
> > 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 | 16 ++++++++++------
> > 1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
> > index 68ab423..237c20b 100644
> > --- a/drivers/clocksource/mtk_timer.c
> > +++ b/drivers/clocksource/mtk_timer.c
> > @@ -156,9 +156,11 @@ 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(struct mtk_clock_event_device *evt, u8 timer,
> > + u8 option, bool enable)
>
> This function can be: __init
>
> Other than this tiny nit, and the small potential conflict in patch 4,
> this whole series is:
>
> Reviewed-by: Daniel Kurtz <[email protected]>
>
> (I do think it is a bit strange that the mediatek,mt6577-timer binding
> does not use "clock-names", but that is independent of this patch
> set).
>
Hi Daniel,
Thanks for your review.
I added __init as you suggested, and Pi-Cheng already sent an updated
version of his patch to resolve the conflict[1].
Joe.C
[1]
http://lists.infradead.org/pipermail/linux-mediatek/2015-July/001592.html
On Wed, 2015-07-22 at 16:14 +0800, Yingjoe Chen wrote:
> 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.
>
> Acked-by: Matthias Brugger <[email protected]>
> Reviewed-by: Daniel Kurtz <[email protected]>
> Signed-off-by: Yingjoe Chen <[email protected]>
> ---
> Update to my patch [1], added __init as Daniel suggest. This is the
> only patch that need to change in that series, so I only sent this one.
>
> http://lists.infradead.org/pipermail/linux-mediatek/2015-July/001545.html
Hi Daniel, Thomas,
Any suggestions for mtk_timer fixes in this series?
Should I resend and add tags from the reviewers?
Joe.C
On Mon, Jul 13, 2015 at 5:32 PM, Yingjoe Chen <[email protected]> wrote:
> This series add GPT timer support for mt8173. This is based on v4.2-rc1
> and Matthias' next branch (for dts parts).
>
> The first 2 patches comes from 'Add SMP bringup support for mt65xx socs'
> series [1]. I decide to move these 2 patches to this series, since it
> is more relevent here. They are changed based on Matthias' and Daniel's
> comments.
>
> [1] http://lists.infradead.org/pipermail/linux-mediatek/2015-May/000714.html
>
> Daniel Kurtz (1):
> arm64: dts: mt8173: add timer node
>
> Yingjoe Chen (4):
> clocksource: mediatek: do not enable GPT_CLK_EVT when setup
> clocksource: mediatek: Use GPT as sched clock source
> arm64: mediatek: enable MTK_TIMER
> clk: mediatek: add 13mhz clock for MT8173
Was this mediatek clocksource patch set get forgotten again?
All 5 patches have have been reviewed, and I think the ones that need
it have been Ack'ed by Matthias and/or Stephen.
What is the plan for merging them?
Thanks,
-Dan
>
> arch/arm64/Kconfig | 1 +
> arch/arm64/boot/dts/mediatek/mt8173.dtsi | 9 +++++++++
> drivers/clk/mediatek/clk-mt8173.c | 5 +++++
> drivers/clocksource/mtk_timer.c | 26 ++++++++++++++++++++------
> include/dt-bindings/clock/mt8173-clk.h | 3 ++-
> 5 files changed, 37 insertions(+), 7 deletions(-)
>
On 08/11/2015 05:54 PM, Daniel Kurtz wrote:
> On Mon, Jul 13, 2015 at 5:32 PM, Yingjoe Chen <[email protected]> wrote:
>> This series add GPT timer support for mt8173. This is based on v4.2-rc1
>> and Matthias' next branch (for dts parts).
>>
>> The first 2 patches comes from 'Add SMP bringup support for mt65xx socs'
>> series [1]. I decide to move these 2 patches to this series, since it
>> is more relevent here. They are changed based on Matthias' and Daniel's
>> comments.
>>
>> [1] http://lists.infradead.org/pipermail/linux-mediatek/2015-May/000714.html
>>
>> Daniel Kurtz (1):
>> arm64: dts: mt8173: add timer node
>>
>> Yingjoe Chen (4):
>> clocksource: mediatek: do not enable GPT_CLK_EVT when setup
>> clocksource: mediatek: Use GPT as sched clock source
>> arm64: mediatek: enable MTK_TIMER
>> clk: mediatek: add 13mhz clock for MT8173
>
> Was this mediatek clocksource patch set get forgotten again?
Yep. Sounds like it felt at the wrong moment.
> All 5 patches have have been reviewed, and I think the ones that need
> it have been Ack'ed by Matthias and/or Stephen.
> What is the plan for merging them?
So if I am not wrong we have two patches I can take in my tree as a
couple of fixes (1 and 2), right ?
--
<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
On Wed, Aug 12, 2015 at 8:32 PM, Daniel Lezcano
<[email protected]> wrote:
> On 08/11/2015 05:54 PM, Daniel Kurtz wrote:
>>
>> On Mon, Jul 13, 2015 at 5:32 PM, Yingjoe Chen <[email protected]>
>> wrote:
>>>
>>> This series add GPT timer support for mt8173. This is based on v4.2-rc1
>>> and Matthias' next branch (for dts parts).
>>>
>>> The first 2 patches comes from 'Add SMP bringup support for mt65xx socs'
>>> series [1]. I decide to move these 2 patches to this series, since it
>>> is more relevent here. They are changed based on Matthias' and Daniel's
>>> comments.
>>>
>>> [1]
>>> http://lists.infradead.org/pipermail/linux-mediatek/2015-May/000714.html
>>>
>>> Daniel Kurtz (1):
>>> arm64: dts: mt8173: add timer node
>>>
>>> Yingjoe Chen (4):
>>> clocksource: mediatek: do not enable GPT_CLK_EVT when setup
>>> clocksource: mediatek: Use GPT as sched clock source
>>> arm64: mediatek: enable MTK_TIMER
>>> clk: mediatek: add 13mhz clock for MT8173
>>
>>
>> Was this mediatek clocksource patch set get forgotten again?
>
>
> Yep. Sounds like it felt at the wrong moment.
>
>> All 5 patches have have been reviewed, and I think the ones that need
>> it have been Ack'ed by Matthias and/or Stephen.
>> What is the plan for merging them?
>
>
> So if I am not wrong we have two patches I can take in my tree as a couple
> of fixes (1 and 2), right ?
Yes.
Patches 3 & 5 are for Matthias, and patch 4 is Ack'ed by Stephen Boyd,
so I assume Matthias can pick that one up too.
Matthias, is that correct?
Or does Patch 4 need to go in via a clock maintainer's tree?
-Dan
On 07/22/2015 10:14 AM, Yingjoe Chen wrote:
> 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.
>
> Acked-by: Matthias Brugger <[email protected]>
> Reviewed-by: Daniel Kurtz <[email protected]>
> Signed-off-by: Yingjoe Chen <[email protected]>
> ---
>
> Update to my patch [1], added __init as Daniel suggest. This is the
> only patch that need to change in that series, so I only sent this one.
>
> http://lists.infradead.org/pipermail/linux-mediatek/2015-July/001545.html
>
> drivers/clocksource/mtk_timer.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
> index 68ab423..2ba5b66 100644
> --- a/drivers/clocksource/mtk_timer.c
> +++ b/drivers/clocksource/mtk_timer.c
> @@ -156,9 +156,11 @@ 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 __init mtk_timer_setup(struct mtk_clock_event_device *evt,
> + u8 timer, u8 option, bool enable)
> {
> + u32 val;
> +
> writel(TIMER_CTRL_CLEAR | TIMER_CTRL_DISABLE,
> evt->gpt_base + TIMER_CTRL_REG(timer));
>
> @@ -167,8 +169,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));
Instead of the 'enable' new option, I prefer a test with 'timer' with a
comment:
/*
* the timer hw is broken in that way ... bla bla, so we only
* enable the clocksource ...
*/
if (timer == GPT_CLK_SRC)
val |= TIMER_CTRL_ENABLE;
That said, can you have a look at commit 1096be08 ?
"clockevents: sun5i: Fix setup_irq init sequence"
first and check if moving the interrupt request after the
clockevents_config_and_register could fix your issue.
> }
>
> static void mtk_timer_enable_irq(struct mtk_clock_event_device *evt, u8 timer)
> @@ -235,12 +239,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, true);
> 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, false);
> clockevents_config_and_register(&evt->dev, rate, 0x3,
> 0xffffffff);
--
<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
On Thu, 2015-08-13 at 10:35 +0200, Daniel Lezcano wrote:
> On 07/22/2015 10:14 AM, Yingjoe Chen wrote:
> > 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.
> >
> > Acked-by: Matthias Brugger <[email protected]>
> > Reviewed-by: Daniel Kurtz <[email protected]>
> > Signed-off-by: Yingjoe Chen <[email protected]>
> > ---
> >
> > Update to my patch [1], added __init as Daniel suggest. This is the
> > only patch that need to change in that series, so I only sent this one.
> >
> > http://lists.infradead.org/pipermail/linux-mediatek/2015-July/001545.html
> >
> > drivers/clocksource/mtk_timer.c | 16 ++++++++++------
> > 1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
> > index 68ab423..2ba5b66 100644
> > --- a/drivers/clocksource/mtk_timer.c
> > +++ b/drivers/clocksource/mtk_timer.c
> > @@ -156,9 +156,11 @@ 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 __init mtk_timer_setup(struct mtk_clock_event_device *evt,
> > + u8 timer, u8 option, bool enable)
> > {
> > + u32 val;
> > +
> > writel(TIMER_CTRL_CLEAR | TIMER_CTRL_DISABLE,
> > evt->gpt_base + TIMER_CTRL_REG(timer));
> >
> > @@ -167,8 +169,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));
>
> Instead of the 'enable' new option, I prefer a test with 'timer' with a
> comment:
>
> /*
> * the timer hw is broken in that way ... bla bla, so we only
> * enable the clocksource ...
> */
> if (timer == GPT_CLK_SRC)
> val |= TIMER_CTRL_ENABLE;
Hi Daniel,
Thanks for your review.
Since this bug happens to anyone using interrupt, I'm not sure checking
timer and only enable it for GPT_CLK_SRC is easier to read. Anyway, I'll
change to this in next version.
> That said, can you have a look at commit 1096be08 ?
> "clockevents: sun5i: Fix setup_irq init sequence"
>
> first and check if moving the interrupt request after the
> clockevents_config_and_register could fix your issue.
I've tested this before, see:
http://lists.infradead.org/pipermail/linux-mediatek/2015-May/000539.html
http://lists.infradead.org/pipermail/linux-mediatek/2015-May/000551.html
Joe.C
On 08/17/2015 04:10 PM, Yingjoe Chen wrote:
> On Thu, 2015-08-13 at 10:35 +0200, Daniel Lezcano wrote:
>> On 07/22/2015 10:14 AM, Yingjoe Chen wrote:
>>> 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.
It "seems" ?
>>> When irq is enabled afterward, we see
>>> spurious interrupt.
Doesn't have the firmware something to do with that ?
I have a mtk 8173 board I can use next week. How do you reproduce the
issue ?
>>> Change init flow to only enable GPT_CLK_SRC at mtk_timer_init.
>>>
>>> Acked-by: Matthias Brugger <[email protected]>
>>> Reviewed-by: Daniel Kurtz <[email protected]>
>>> Signed-off-by: Yingjoe Chen <[email protected]>
>>> ---
>>>
>>> Update to my patch [1], added __init as Daniel suggest. This is the
>>> only patch that need to change in that series, so I only sent this one.
>>>
>>> http://lists.infradead.org/pipermail/linux-mediatek/2015-July/001545.html
>>>
>>> drivers/clocksource/mtk_timer.c | 16 ++++++++++------
>>> 1 file changed, 10 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
>>> index 68ab423..2ba5b66 100644
>>> --- a/drivers/clocksource/mtk_timer.c
>>> +++ b/drivers/clocksource/mtk_timer.c
>>> @@ -156,9 +156,11 @@ 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 __init mtk_timer_setup(struct mtk_clock_event_device *evt,
>>> + u8 timer, u8 option, bool enable)
>>> {
>>> + u32 val;
>>> +
>>> writel(TIMER_CTRL_CLEAR | TIMER_CTRL_DISABLE,
>>> evt->gpt_base + TIMER_CTRL_REG(timer));
>>>
>>> @@ -167,8 +169,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));
>>
>> Instead of the 'enable' new option, I prefer a test with 'timer' with a
>> comment:
>>
>> /*
>> * the timer hw is broken in that way ... bla bla, so we only
>> * enable the clocksource ...
>> */
>> if (timer == GPT_CLK_SRC)
>> val |= TIMER_CTRL_ENABLE;
>
> Hi Daniel,
>
> Thanks for your review.
> Since this bug happens to anyone using interrupt,
Can you elaborate ? I don't get the point.
> I'm not sure checking
> timer and only enable it for GPT_CLK_SRC is easier to read. Anyway, I'll
> change to this in next version.
>> That said, can you have a look at commit 1096be08 ?
>> "clockevents: sun5i: Fix setup_irq init sequence"
>>
>> first and check if moving the interrupt request after the
>> clockevents_config_and_register could fix your issue.
>
> I've tested this before, see:
>
> http://lists.infradead.org/pipermail/linux-mediatek/2015-May/000539.html
> http://lists.infradead.org/pipermail/linux-mediatek/2015-May/000551.html
I could take or ack this patch trusting it fixes the issue but there are
some points that need clarifications.
- Does the spurious interrupt occurs *every* time ? at each boot ?
The previous patches were supposed to fix the issue but they actually
didn't. So how is tested the patch ?
Regarding the different fixes for this problem, it sounds like you are
proceeding by trial and error.
Please give a more detailed analysis of the problem and especially check
the timer is not altered by the firmware leaving it in a transient state
or whatever.
-- Daniel
--
<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
On Thu, 2015-08-20 at 16:28 +0200, Daniel Lezcano wrote:
> On 08/17/2015 04:10 PM, Yingjoe Chen wrote:
> > On Thu, 2015-08-13 at 10:35 +0200, Daniel Lezcano wrote:
> >> On 07/22/2015 10:14 AM, Yingjoe Chen wrote:
> >>> 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.
>
> It "seems" ?
Hi,
Datasheet doesn't mention detail. So I did some experiments, playing
around with registers. Based on my observation, I think this is what
happens:
For each GPT timer, it has ENABLE, IRQ_EN, IRQ status, IRQ_ACK, counter
& compare.
When mtk_timer_init calls mtk_timer_setup to setup GPT_CLK_EVT, it
enable the timer but didn't set counter or compare. Both counter &
compare is zero on reset, so GPT immediately raise IRQ status. IRQ_EN is
still disabled now, so it didn't trigger interrupt right away.
At end of mtk_timer_init, it calls mtk_timer_enable_irq to enable irq.
Since IRQ status is 1 now, GPT trigger interrupt immediately. The
interrupt is serviced by mtk_timer_interrupt. Since this is not an
expected event, evt->dev.event_handler will be NULL and system crashed
in the handler.
> >>> When irq is enabled afterward, we see
> >>> spurious interrupt.
>
> Doesn't have the firmware something to do with that ?
We have 6 GPT on mt8173, mtk timer use 2 of them. The spurious interrupt
only happens on GPT_CLK_EVT (GPT1). Our firmware didn't touch that one,
so it is in reset default when mtk timer driver try to enable it.
> I have a mtk 8173 board I can use next week. How do you reproduce the
> issue ?
>
> >>> Change init flow to only enable GPT_CLK_SRC at mtk_timer_init.
> >>>
> >>> Acked-by: Matthias Brugger <[email protected]>
> >>> Reviewed-by: Daniel Kurtz <[email protected]>
> >>> Signed-off-by: Yingjoe Chen <[email protected]>
> >>> ---
> >>>
> >>> Update to my patch [1], added __init as Daniel suggest. This is the
> >>> only patch that need to change in that series, so I only sent this one.
> >>>
> >>> http://lists.infradead.org/pipermail/linux-mediatek/2015-July/001545.html
> >>>
> >>> drivers/clocksource/mtk_timer.c | 16 ++++++++++------
> >>> 1 file changed, 10 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
> >>> index 68ab423..2ba5b66 100644
> >>> --- a/drivers/clocksource/mtk_timer.c
> >>> +++ b/drivers/clocksource/mtk_timer.c
> >>> @@ -156,9 +156,11 @@ 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 __init mtk_timer_setup(struct mtk_clock_event_device *evt,
> >>> + u8 timer, u8 option, bool enable)
> >>> {
> >>> + u32 val;
> >>> +
> >>> writel(TIMER_CTRL_CLEAR | TIMER_CTRL_DISABLE,
> >>> evt->gpt_base + TIMER_CTRL_REG(timer));
> >>>
> >>> @@ -167,8 +169,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));
> >>
> >> Instead of the 'enable' new option, I prefer a test with 'timer' with a
> >> comment:
> >>
> >> /*
> >> * the timer hw is broken in that way ... bla bla, so we only
> >> * enable the clocksource ...
> >> */
> >> if (timer == GPT_CLK_SRC)
> >> val |= TIMER_CTRL_ENABLE;
> >
> > Hi Daniel,
> >
> > Thanks for your review.
> > Since this bug happens to anyone using interrupt,
>
> Can you elaborate ? I don't get the point.
>
> > I'm not sure checking
> > timer and only enable it for GPT_CLK_SRC is easier to read. Anyway, I'll
> > change to this in next version.
> >> That said, can you have a look at commit 1096be08 ?
> >> "clockevents: sun5i: Fix setup_irq init sequence"
> >>
> >> first and check if moving the interrupt request after the
> >> clockevents_config_and_register could fix your issue.
> >
> > I've tested this before, see:
> >
> > http://lists.infradead.org/pipermail/linux-mediatek/2015-May/000539.html
> > http://lists.infradead.org/pipermail/linux-mediatek/2015-May/000551.html
>
> I could take or ack this patch trusting it fixes the issue but there are
> some points that need clarifications.
>
> - Does the spurious interrupt occurs *every* time ? at each boot ?
Yes. If you applied this series to enable mtk timer without this fix on
mt8173 or mt8135 you can reproduce it. It occurs for every boot.
It crash before uart driver is ready, so you'll have to use earlycon to
see the crash log.
> The previous patches were supposed to fix the issue but they actually
> didn't. So how is tested the patch ?
I'm not sure. I believe Matthias test that one on mt6589, maybe it have
different behavior, or different timing.
> Regarding the different fixes for this problem, it sounds like you are
> proceeding by trial and error.
>
> Please give a more detailed analysis of the problem and especially check
> the timer is not altered by the firmware leaving it in a transient state
> or whatever.
See above for my analysis.
Thanks.
Joe.C
On 08/21/2015 04:39 PM, Yingjoe Chen wrote:
[ ... ]
>> - Does the spurious interrupt occurs *every* time ? at each boot ?
>
> Yes. If you applied this series to enable mtk timer without this fix on
> mt8173 or mt8135 you can reproduce it. It occurs for every boot.
>
> It crash before uart driver is ready, so you'll have to use earlycon to
> see the crash log.
Can you give me the earlycon params ?
Thanks.
-- Daniel
--
<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
After analysis done by Yingjoe Chen, the timer appears to have a pending
interrupt when it is enabled.
Fix this by acknowledging the pending interrupt when enabling the timer
interrupt.
Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/clocksource/mtk_timer.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
index 4cd16fb..13543a8 100644
--- a/drivers/clocksource/mtk_timer.c
+++ b/drivers/clocksource/mtk_timer.c
@@ -156,14 +156,6 @@ 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)
-{
- /* Disable all interrupts */
- writel(0x0, evt->gpt_base + GPT_IRQ_EN_REG);
- /* Acknowledge all interrupts */
- writel(0x3f, evt->gpt_base + GPT_IRQ_ACK_REG);
-}
-
static void
mtk_timer_setup(struct mtk_clock_event_device *evt, u8 timer, u8 option)
{
@@ -183,6 +175,9 @@ static void mtk_timer_enable_irq(struct mtk_clock_event_device *evt, u8 timer)
{
u32 val;
+ /* Acknowledge all spurious pending interrupts */
+ writel(0x3f, evt->gpt_base + GPT_IRQ_ACK_REG);
+
val = readl(evt->gpt_base + GPT_IRQ_EN_REG);
writel(val | GPT_IRQ_ENABLE(timer),
evt->gpt_base + GPT_IRQ_EN_REG);
@@ -232,8 +227,6 @@ static void __init mtk_timer_init(struct device_node *node)
}
rate = clk_get_rate(clk);
- mtk_timer_global_reset(evt);
-
if (request_irq(evt->dev.irq, mtk_timer_interrupt,
IRQF_TIMER | IRQF_IRQPOLL, "mtk_timer", evt)) {
pr_warn("failed to setup irq %d\n", evt->dev.irq);
--
1.9.1
On Mon, 2015-08-24 at 09:51 +0200, Daniel Lezcano wrote:
> On 08/21/2015 04:39 PM, Yingjoe Chen wrote:
>
> [ ... ]
>
> >> - Does the spurious interrupt occurs *every* time ? at each boot ?
> >
> > Yes. If you applied this series to enable mtk timer without this fix on
> > mt8173 or mt8135 you can reproduce it. It occurs for every boot.
> >
> > It crash before uart driver is ready, so you'll have to use earlycon to
> > see the crash log.
>
> Can you give me the earlycon params ?
Hi Daniel,
You probably already figure this out. anyway I'm using this to enable
earlycon:
Add 'earlycon' in bootargs, and in device tree chosen part, add
"linux,stdout-path=&uart0;"
Joe.C
On Mon, 2015-08-24 at 15:30 +0200, Daniel Lezcano wrote:
> After analysis done by Yingjoe Chen, the timer appears to have a pending
> interrupt when it is enabled.
>
> Fix this by acknowledging the pending interrupt when enabling the timer
> interrupt.
>
> Signed-off-by: Daniel Lezcano <[email protected]>
Hi Daniel,
Thanks for your patch, this can fix the boot issue.
Tested-by: Yingjoe Chen <[email protected]>
> ---
> drivers/clocksource/mtk_timer.c | 13 +++----------
> 1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
> index 4cd16fb..13543a8 100644
> --- a/drivers/clocksource/mtk_timer.c
> +++ b/drivers/clocksource/mtk_timer.c
> @@ -156,14 +156,6 @@ 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)
> -{
> - /* Disable all interrupts */
> - writel(0x0, evt->gpt_base + GPT_IRQ_EN_REG);
> - /* Acknowledge all interrupts */
> - writel(0x3f, evt->gpt_base + GPT_IRQ_ACK_REG);
> -}
> -
> static void
> mtk_timer_setup(struct mtk_clock_event_device *evt, u8 timer, u8 option)
> {
> @@ -183,6 +175,9 @@ static void mtk_timer_enable_irq(struct mtk_clock_event_device *evt, u8 timer)
> {
> u32 val;
>
> + /* Acknowledge all spurious pending interrupts */
> + writel(0x3f, evt->gpt_base + GPT_IRQ_ACK_REG);
This should use tab to indent.
> +
> val = readl(evt->gpt_base + GPT_IRQ_EN_REG);
> writel(val | GPT_IRQ_ENABLE(timer),
> evt->gpt_base + GPT_IRQ_EN_REG);
> @@ -232,8 +227,6 @@ static void __init mtk_timer_init(struct device_node *node)
> }
> rate = clk_get_rate(clk);
>
> - mtk_timer_global_reset(evt);
> -
I think we should keep this one, or at least disable irq first in
mtk_timer_enable_irq. MT8173 firmware didn't use this GPT, but I think
it is a good ideat to do it just in case firmware in some other platform
use it.
Joe.C
After analysis done by Yingjoe Chen, the timer appears to have a pending
interrupt when it is enabled.
Fix this by acknowledging the pending interrupt when enabling the timer
interrupt.
Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/clocksource/mtk_timer.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
index 4cd16fb..505f53d 100644
--- a/drivers/clocksource/mtk_timer.c
+++ b/drivers/clocksource/mtk_timer.c
@@ -156,14 +156,6 @@ 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)
-{
- /* Disable all interrupts */
- writel(0x0, evt->gpt_base + GPT_IRQ_EN_REG);
- /* Acknowledge all interrupts */
- writel(0x3f, evt->gpt_base + GPT_IRQ_ACK_REG);
-}
-
static void
mtk_timer_setup(struct mtk_clock_event_device *evt, u8 timer, u8 option)
{
@@ -183,6 +175,12 @@ static void mtk_timer_enable_irq(struct mtk_clock_event_device *evt, u8 timer)
{
u32 val;
+ /* Disable all interrupts */
+ writel(0x0, evt->gpt_base + GPT_IRQ_EN_REG);
+
+ /* Acknowledge all spurious pending interrupts */
+ writel(0x3f, evt->gpt_base + GPT_IRQ_ACK_REG);
+
val = readl(evt->gpt_base + GPT_IRQ_EN_REG);
writel(val | GPT_IRQ_ENABLE(timer),
evt->gpt_base + GPT_IRQ_EN_REG);
@@ -232,8 +230,6 @@ static void __init mtk_timer_init(struct device_node *node)
}
rate = clk_get_rate(clk);
- mtk_timer_global_reset(evt);
-
if (request_irq(evt->dev.irq, mtk_timer_interrupt,
IRQF_TIMER | IRQF_IRQPOLL, "mtk_timer", evt)) {
pr_warn("failed to setup irq %d\n", evt->dev.irq);
--
1.9.1
On Mon, 2015-08-24 at 23:57 +0200, Daniel Lezcano wrote:
> After analysis done by Yingjoe Chen, the timer appears to have a pending
> interrupt when it is enabled.
>
> Fix this by acknowledging the pending interrupt when enabling the timer
> interrupt.
>
> Signed-off-by: Daniel Lezcano <[email protected]>
> ---
> drivers/clocksource/mtk_timer.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/clocksource/mtk_timer.c b/drivers/clocksource/mtk_timer.c
> index 4cd16fb..505f53d 100644
> --- a/drivers/clocksource/mtk_timer.c
> +++ b/drivers/clocksource/mtk_timer.c
> @@ -156,14 +156,6 @@ 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)
> -{
> - /* Disable all interrupts */
> - writel(0x0, evt->gpt_base + GPT_IRQ_EN_REG);
> - /* Acknowledge all interrupts */
> - writel(0x3f, evt->gpt_base + GPT_IRQ_ACK_REG);
> -}
> -
> static void
> mtk_timer_setup(struct mtk_clock_event_device *evt, u8 timer, u8 option)
> {
> @@ -183,6 +175,12 @@ static void mtk_timer_enable_irq(struct mtk_clock_event_device *evt, u8 timer)
> {
> u32 val;
>
> + /* Disable all interrupts */
> + writel(0x0, evt->gpt_base + GPT_IRQ_EN_REG);
> +
> + /* Acknowledge all spurious pending interrupts */
> + writel(0x3f, evt->gpt_base + GPT_IRQ_ACK_REG);
> +
> val = readl(evt->gpt_base + GPT_IRQ_EN_REG);
> writel(val | GPT_IRQ_ENABLE(timer),
> evt->gpt_base + GPT_IRQ_EN_REG);
> @@ -232,8 +230,6 @@ static void __init mtk_timer_init(struct device_node *node)
> }
> rate = clk_get_rate(clk);
>
> - mtk_timer_global_reset(evt);
> -
> if (request_irq(evt->dev.irq, mtk_timer_interrupt,
> IRQF_TIMER | IRQF_IRQPOLL, "mtk_timer", evt)) {
> pr_warn("failed to setup irq %d\n", evt->dev.irq);
Hi Daniel,
Thanks for your patch, tested on mt8173
Tested-by: Yingjoe Chen <[email protected]>
Joe.C
On 07/13/2015 11:32 AM, Yingjoe Chen wrote:
> This series add GPT timer support for mt8173. This is based on v4.2-rc1
> and Matthias' next branch (for dts parts).
>
> The first 2 patches comes from 'Add SMP bringup support for mt65xx socs'
> series [1]. I decide to move these 2 patches to this series, since it
> is more relevent here. They are changed based on Matthias' and Daniel's
> comments.
>
> [1] http://lists.infradead.org/pipermail/linux-mediatek/2015-May/000714.html
>
> Daniel Kurtz (1):
> arm64: dts: mt8173: add timer node
>
> Yingjoe Chen (4):
> clocksource: mediatek: do not enable GPT_CLK_EVT when setup
> clocksource: mediatek: Use GPT as sched clock source
> arm64: mediatek: enable MTK_TIMER
> clk: mediatek: add 13mhz clock for MT8173
>
> arch/arm64/Kconfig | 1 +
> arch/arm64/boot/dts/mediatek/mt8173.dtsi | 9 +++++++++
> drivers/clk/mediatek/clk-mt8173.c | 5 +++++
> drivers/clocksource/mtk_timer.c | 26 ++++++++++++++++++++------
> include/dt-bindings/clock/mt8173-clk.h | 3 ++-
> 5 files changed, 37 insertions(+), 7 deletions(-)
Who will take this patchset ? I can take the patch 2 if needed.
--
<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
On Wed, 2015-08-26 at 09:37 +0200, Daniel Lezcano wrote:
> On 07/13/2015 11:32 AM, Yingjoe Chen wrote:
> > This series add GPT timer support for mt8173. This is based on v4.2-rc1
> > and Matthias' next branch (for dts parts).
> >
> > The first 2 patches comes from 'Add SMP bringup support for mt65xx socs'
> > series [1]. I decide to move these 2 patches to this series, since it
> > is more relevent here. They are changed based on Matthias' and Daniel's
> > comments.
> >
> > [1] http://lists.infradead.org/pipermail/linux-mediatek/2015-May/000714.html
> >
> > Daniel Kurtz (1):
> > arm64: dts: mt8173: add timer node
> >
> > Yingjoe Chen (4):
> > clocksource: mediatek: do not enable GPT_CLK_EVT when setup
> > clocksource: mediatek: Use GPT as sched clock source
> > arm64: mediatek: enable MTK_TIMER
> > clk: mediatek: add 13mhz clock for MT8173
> >
> > arch/arm64/Kconfig | 1 +
> > arch/arm64/boot/dts/mediatek/mt8173.dtsi | 9 +++++++++
> > drivers/clk/mediatek/clk-mt8173.c | 5 +++++
> > drivers/clocksource/mtk_timer.c | 26 ++++++++++++++++++++------
> > include/dt-bindings/clock/mt8173-clk.h | 3 ++-
> > 5 files changed, 37 insertions(+), 7 deletions(-)
>
> Who will take this patchset ? I can take the patch 2 if needed.
>
>
Hi Daniel,
Please take patch 2 with your fix. Thanks.
I think patch 3(enable MTK_TIMER in Kconfig) and 5 (add timer node)
should go through Matthias' tree to arm-soc and patch 4(add 13mhz clock)
should go through James' mtk-clk tree to clk maintainer.
Joe.C
On 08/25/2015 03:21 PM, Yingjoe Chen wrote:
> Tested-by: Yingjoe Chen<[email protected]>
Applied to my tree for 4.4.
-- Daniel
--
<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