* [PATCH] clocksource: Add driver for the Ingenic JZ47xx OST
@ 2019-08-09 12:38 Paul Cercueil
2019-08-16 14:54 ` Daniel Lezcano
0 siblings, 1 reply; 8+ messages in thread
From: Paul Cercueil @ 2019-08-09 12:38 UTC (permalink / raw)
To: Daniel Lezcano, Thomas Gleixner
Cc: od, linux-kernel, Maarten ter Huurne, Paul Cercueil,
Mathieu Malaterre, Artur Rojek
From: Maarten ter Huurne <maarten@treewalker.org>
OST is the OS Timer, a 64-bit timer/counter with buffered reading.
SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and
JZ4780 have a 64-bit OST.
This driver will register both a clocksource and a sched_clock to the
system.
Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Tested-by: Mathieu Malaterre <malat@debian.org>
Tested-by: Artur Rojek <contact@artur-rojek.eu>
---
Hi,
This patch comes from a bigger patchset that was cut in smaller pieces
for easier integration to mainline.
(The patchset was https://lkml.org/lkml/2019/3/27/1837)
The only change is the use of device_node_to_regmap(), which was added
in a prior patchset now merged in the MIPS tree.
For that reason this patch is based on the ingenic-tcu-v5.4 branch of
the MIPS tree
(git://git.kernel.org/pub/scm/linux/kernel/git/mips/linux.git).
Thanks,
-Paul
drivers/clocksource/Kconfig | 8 ++
drivers/clocksource/Makefile | 1 +
drivers/clocksource/ingenic-ost.c | 221 ++++++++++++++++++++++++++++++
3 files changed, 230 insertions(+)
create mode 100644 drivers/clocksource/ingenic-ost.c
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index a9cdc2c4f8bd..3b1503082a23 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -696,4 +696,12 @@ config INGENIC_TIMER
help
Support for the timer/counter unit of the Ingenic JZ SoCs.
+config INGENIC_OST
+ bool "Ingenic JZ47xx Operating System Timer"
+ depends on MIPS || COMPILE_TEST
+ depends on COMMON_CLK
+ select MFD_SYSCON
+ help
+ Support for the OS Timer of the Ingenic JZ4770 or similar SoC.
+
endmenu
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 4dfe4225ece7..6bc97a6fd229 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -80,6 +80,7 @@ obj-$(CONFIG_ASM9260_TIMER) += asm9260_timer.o
obj-$(CONFIG_H8300_TMR8) += h8300_timer8.o
obj-$(CONFIG_H8300_TMR16) += h8300_timer16.o
obj-$(CONFIG_H8300_TPU) += h8300_tpu.o
+obj-$(CONFIG_INGENIC_OST) += ingenic-ost.o
obj-$(CONFIG_INGENIC_TIMER) += ingenic-timer.o
obj-$(CONFIG_CLKSRC_ST_LPC) += clksrc_st_lpc.o
obj-$(CONFIG_X86_NUMACHIP) += numachip.o
diff --git a/drivers/clocksource/ingenic-ost.c b/drivers/clocksource/ingenic-ost.c
new file mode 100644
index 000000000000..c708d5d7dd15
--- /dev/null
+++ b/drivers/clocksource/ingenic-ost.c
@@ -0,0 +1,221 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * JZ47xx SoCs TCU Operating System Timer driver
+ *
+ * Copyright (C) 2016 Maarten ter Huurne <maarten@treewalker.org>
+ * Copyright (C) 2018 Paul Cercueil <paul@crapouillou.net>
+ */
+
+#include <linux/clk.h>
+#include <linux/clocksource.h>
+#include <linux/mfd/ingenic-tcu.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/regmap.h>
+#include <linux/sched_clock.h>
+
+#define TCU_OST_TCSR_MASK 0xffc0
+#define TCU_OST_TCSR_CNT_MD BIT(15)
+
+#define TCU_OST_CHANNEL 15
+
+struct ingenic_ost_soc_info {
+ bool is64bit;
+};
+
+struct ingenic_ost {
+ struct regmap *map;
+ struct clk *clk;
+
+ struct clocksource cs;
+};
+
+static struct ingenic_ost *ingenic_ost;
+
+static u64 notrace ingenic_ost_read_cntl(void)
+{
+ u32 val;
+
+ regmap_read(ingenic_ost->map, TCU_REG_OST_CNTL, &val);
+
+ return val;
+}
+
+static u64 notrace ingenic_ost_read_cnth(void)
+{
+ u32 val;
+
+ regmap_read(ingenic_ost->map, TCU_REG_OST_CNTH, &val);
+
+ return val;
+}
+
+static u64 notrace ingenic_ost_clocksource_read64(struct clocksource *cs)
+{
+ u32 val1, val2;
+ u64 count, recount;
+ s64 diff;
+
+ /*
+ * The buffering of the upper 32 bits of the timer prevents wrong
+ * results from the bottom 32 bits overflowing due to the timer ticking
+ * along. However, it does not prevent wrong results from simultaneous
+ * reads of the timer, which could reset the buffer mid-read.
+ * Since this kind of wrong read can happen only when the bottom bits
+ * overflow, there will be minutes between wrong reads, so if we read
+ * twice in succession, at least one of the reads will be correct.
+ */
+
+ /* Bypass the regmap here as we must return as soon as possible */
+ regmap_read(ingenic_ost->map, TCU_REG_OST_CNTL, &val1);
+ regmap_read(ingenic_ost->map, TCU_REG_OST_CNTHBUF, &val2);
+ count = (u64)val1 | (u64)val2 << 32;
+
+ regmap_read(ingenic_ost->map, TCU_REG_OST_CNTL, &val1);
+ regmap_read(ingenic_ost->map, TCU_REG_OST_CNTHBUF, &val2);
+ recount = (u64)val1 | (u64)val2 << 32;
+
+ /*
+ * A wrong read will produce a result that is 1<<32 too high: the bottom
+ * part from before overflow and the upper part from after overflow.
+ * Therefore, the lower value of the two reads is the correct value.
+ */
+
+ diff = (s64)(recount - count);
+ if (unlikely(diff < 0))
+ count = recount;
+
+ return count;
+}
+
+static u64 notrace ingenic_ost_clocksource_read32(struct clocksource *cs)
+{
+ return ingenic_ost_read_cnth();
+}
+
+static int __init ingenic_ost_probe(struct platform_device *pdev)
+{
+ const struct ingenic_ost_soc_info *soc_info;
+ struct device *dev = &pdev->dev;
+ struct ingenic_ost *ost;
+ struct clocksource *cs;
+ unsigned long rate, flags;
+ int err;
+
+ soc_info = device_get_match_data(dev);
+ if (!soc_info)
+ return -EINVAL;
+
+ ost = devm_kzalloc(dev, sizeof(*ost), GFP_KERNEL);
+ if (!ost)
+ return -ENOMEM;
+
+ ingenic_ost = ost;
+
+ ost->map = device_node_to_regmap(dev->parent->of_node);
+ if (!ost->map) {
+ dev_err(dev, "regmap not found\n");
+ return -EINVAL;
+ }
+
+ ost->clk = devm_clk_get(dev, "ost");
+ if (IS_ERR(ost->clk))
+ return PTR_ERR(ost->clk);
+
+ err = clk_prepare_enable(ost->clk);
+ if (err)
+ return err;
+
+ /* Clear counter high/low registers */
+ if (soc_info->is64bit)
+ regmap_write(ost->map, TCU_REG_OST_CNTL, 0);
+ regmap_write(ost->map, TCU_REG_OST_CNTH, 0);
+
+ /* Don't reset counter at compare value. */
+ regmap_update_bits(ost->map, TCU_REG_OST_TCSR,
+ TCU_OST_TCSR_MASK, TCU_OST_TCSR_CNT_MD);
+
+ rate = clk_get_rate(ost->clk);
+
+ /* Enable OST TCU channel */
+ regmap_write(ost->map, TCU_REG_TESR, BIT(TCU_OST_CHANNEL));
+
+ cs = &ost->cs;
+ cs->name = "ingenic-ost";
+ cs->rating = 320;
+ cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
+
+ if (soc_info->is64bit) {
+ cs->mask = CLOCKSOURCE_MASK(64);
+ cs->read = ingenic_ost_clocksource_read64;
+ } else {
+ cs->mask = CLOCKSOURCE_MASK(32);
+ cs->read = ingenic_ost_clocksource_read32;
+ }
+
+ err = clocksource_register_hz(cs, rate);
+ if (err) {
+ dev_err(dev, "clocksource registration failed: %d\n", err);
+ clk_disable_unprepare(ost->clk);
+ return err;
+ }
+
+ /* Cannot register a sched_clock with interrupts on */
+ local_irq_save(flags);
+ if (soc_info->is64bit)
+ sched_clock_register(ingenic_ost_read_cntl, 32, rate);
+ else
+ sched_clock_register(ingenic_ost_read_cnth, 32, rate);
+ local_irq_restore(flags);
+
+ return 0;
+}
+
+static int __maybe_unused ingenic_ost_suspend(struct device *dev)
+{
+ struct ingenic_ost *ost = dev_get_drvdata(dev);
+
+ clk_disable(ost->clk);
+
+ return 0;
+}
+
+static int __maybe_unused ingenic_ost_resume(struct device *dev)
+{
+ struct ingenic_ost *ost = dev_get_drvdata(dev);
+
+ return clk_enable(ost->clk);
+}
+
+static const struct dev_pm_ops __maybe_unused ingenic_ost_pm_ops = {
+ /* _noirq: We want the OST clock to be gated last / ungated first */
+ .suspend_noirq = ingenic_ost_suspend,
+ .resume_noirq = ingenic_ost_resume,
+};
+
+static const struct ingenic_ost_soc_info jz4725b_ost_soc_info = {
+ .is64bit = false,
+};
+
+static const struct ingenic_ost_soc_info jz4770_ost_soc_info = {
+ .is64bit = true,
+};
+
+static const struct of_device_id ingenic_ost_of_match[] = {
+ { .compatible = "ingenic,jz4725b-ost", .data = &jz4725b_ost_soc_info, },
+ { .compatible = "ingenic,jz4770-ost", .data = &jz4770_ost_soc_info, },
+ { }
+};
+
+static struct platform_driver ingenic_ost_driver = {
+ .driver = {
+ .name = "ingenic-ost",
+#ifdef CONFIG_PM_SUSPEND
+ .pm = &ingenic_ost_pm_ops,
+#endif
+ .of_match_table = ingenic_ost_of_match,
+ },
+};
+builtin_platform_driver_probe(ingenic_ost_driver, ingenic_ost_probe);
--
2.21.0.593.g511ec345e18
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] clocksource: Add driver for the Ingenic JZ47xx OST
2019-08-09 12:38 [PATCH] clocksource: Add driver for the Ingenic JZ47xx OST Paul Cercueil
@ 2019-08-16 14:54 ` Daniel Lezcano
2019-11-07 15:56 ` Paul Cercueil
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Lezcano @ 2019-08-16 14:54 UTC (permalink / raw)
To: Paul Cercueil, Thomas Gleixner
Cc: od, linux-kernel, Maarten ter Huurne, Mathieu Malaterre, Artur Rojek
On 09/08/2019 14:38, Paul Cercueil wrote:
> From: Maarten ter Huurne <maarten@treewalker.org>
>
> OST is the OS Timer, a 64-bit timer/counter with buffered reading.
>
> SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and
> JZ4780 have a 64-bit OST.
>
> This driver will register both a clocksource and a sched_clock to the
> system.
>
> Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Tested-by: Mathieu Malaterre <malat@debian.org>
> Tested-by: Artur Rojek <contact@artur-rojek.eu>
> ---
>
[ ... ]
> + err = clocksource_register_hz(cs, rate);
> + if (err) {
> + dev_err(dev, "clocksource registration failed: %d\n", err);
> + clk_disable_unprepare(ost->clk);
> + return err;
> + }
> +
> + /* Cannot register a sched_clock with interrupts on */
Aren't they already disabled?
> + local_irq_save(flags);
> + if (soc_info->is64bit)
> + sched_clock_register(ingenic_ost_read_cntl, 32, rate);
> + else
> + sched_clock_register(ingenic_ost_read_cnth, 32, rate);
> + local_irq_restore(flags);
> +
> + return 0;
> +}
> +
--
<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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] clocksource: Add driver for the Ingenic JZ47xx OST
2019-08-16 14:54 ` Daniel Lezcano
@ 2019-11-07 15:56 ` Paul Cercueil
2019-11-07 19:39 ` Daniel Lezcano
0 siblings, 1 reply; 8+ messages in thread
From: Paul Cercueil @ 2019-11-07 15:56 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Thomas Gleixner, od, linux-kernel, Maarten ter Huurne,
Mathieu Malaterre, Artur Rojek
Hi Daniel,
On 2019-08-16 16:54, Daniel Lezcano wrote:
> On 09/08/2019 14:38, Paul Cercueil wrote:
>> From: Maarten ter Huurne <maarten@treewalker.org>
>>
>> OST is the OS Timer, a 64-bit timer/counter with buffered reading.
>>
>> SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and
>> JZ4780 have a 64-bit OST.
>>
>> This driver will register both a clocksource and a sched_clock to the
>> system.
>>
>> Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>> Tested-by: Mathieu Malaterre <malat@debian.org>
>> Tested-by: Artur Rojek <contact@artur-rojek.eu>
>> ---
>>
>
> [ ... ]
>
>> + err = clocksource_register_hz(cs, rate);
>> + if (err) {
>> + dev_err(dev, "clocksource registration failed: %d\n", err);
>> + clk_disable_unprepare(ost->clk);
>> + return err;
>> + }
>> +
>> + /* Cannot register a sched_clock with interrupts on */
>
> Aren't they already disabled?
Sorry for the late reply.
No, they are not already disabled; this is what I get if I comment out
the local_irq_save()/local_irq_restore():
[ 0.361014] clocksource: ingenic-ost: mask: 0xffffffff max_cycles:
0xffffffff, max_idle_ns: 159271703898 ns
[ 0.361515] clocksource: Switched to clocksource ingenic-ost
[ 0.361686] ------------[ cut here ]------------
[ 0.361893] WARNING: CPU: 0 PID: 1 at kernel/time/sched_clock.c:179
sched_clock_register+0x7c/0x2e4
[ 0.362174] CPU: 0 PID: 1 Comm: swapper Not tainted 5.4.0-rc5+ #461
[ 0.362330] Stack : 80744558 80069b44 80770000 00000000 00000000
00dfd7a7 806e6db4 8106bb74
[ 0.362619] 806f0000 81067ca4 806f31c7 80769478 00000020
10000400 8106bb20 00dfd7a7
[ 0.362906] 00000000 00000000 80780000 00000000 00000007
00000001 00000049 3563722d
[ 0.363191] 8106ba61 00000000 ffffffff 00000010 806f0000
00000000 00000000 806f0000
[ 0.363477] 00000020 00000000 80714534 80770000 00000002
80319154 00000000 80770000
[ 0.363762] ...
[ 0.363906] Call Trace:
[ 0.364087] [<8001af14>] show_stack+0x40/0x128
[ 0.364289] [<8002fd88>] __warn+0xb8/0xe0
[ 0.364478] [<8002fe14>] warn_slowpath_fmt+0x64/0xc0
[ 0.364678] [<8072b1c8>] sched_clock_register+0x7c/0x2e4
[ 0.364895] [<8073c874>] ingenic_ost_probe+0x224/0x248
[ 0.365090] [<803d5394>] platform_drv_probe+0x40/0x94
[ 0.365526] [<803d362c>] really_probe+0x104/0x374
[ 0.365743] [<803d3ff0>] device_driver_attach+0x78/0x80
[ 0.365938] [<803d4070>] __driver_attach+0x78/0x118
[ 0.366129] [<803d1700>] bus_for_each_dev+0x7c/0xc8
[ 0.366318] [<803d226c>] bus_add_driver+0x1bc/0x204
[ 0.366513] [<803d4878>] driver_register+0x84/0x14c
[ 0.366717] [<8073a144>] __platform_driver_probe+0x98/0x140
[ 0.366931] [<80724e38>] do_one_initcall+0x84/0x1b4
[ 0.367126] [<807250cc>] kernel_init_freeable+0x164/0x240
[ 0.367318] [<805df75c>] kernel_init+0x10/0xf8
[ 0.367510] [<8001542c>] ret_from_kernel_thread+0x14/0x1c
[ 0.367722] ---[ end trace 7fedf00408fa3bed ]---
[ 0.367985] sched_clock: 32 bits at 12MHz, resolution 83ns, wraps
every 178956970966ns
At kernel/time/sched_clock.c:179 there is:
WARN_ON(!irqs_disabled());
>> + local_irq_save(flags);
>> + if (soc_info->is64bit)
>> + sched_clock_register(ingenic_ost_read_cntl, 32, rate);
>> + else
>> + sched_clock_register(ingenic_ost_read_cnth, 32, rate);
>> + local_irq_restore(flags);
>> +
>> + return 0;
>> +}
>> +
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] clocksource: Add driver for the Ingenic JZ47xx OST
2019-11-07 15:56 ` Paul Cercueil
@ 2019-11-07 19:39 ` Daniel Lezcano
2019-11-07 19:57 ` Paul Cercueil
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Lezcano @ 2019-11-07 19:39 UTC (permalink / raw)
To: Paul Cercueil
Cc: Thomas Gleixner, od, linux-kernel, Maarten ter Huurne,
Mathieu Malaterre, Artur Rojek
On 07/11/2019 16:56, Paul Cercueil wrote:
> Hi Daniel,
>
> On 2019-08-16 16:54, Daniel Lezcano wrote:
>> On 09/08/2019 14:38, Paul Cercueil wrote:
>>> From: Maarten ter Huurne <maarten@treewalker.org>
>>>
>>> OST is the OS Timer, a 64-bit timer/counter with buffered reading.
>>>
>>> SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and
>>> JZ4780 have a 64-bit OST.
>>>
>>> This driver will register both a clocksource and a sched_clock to the
>>> system.
>>>
>>> Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>> Tested-by: Mathieu Malaterre <malat@debian.org>
>>> Tested-by: Artur Rojek <contact@artur-rojek.eu>
>>> ---
>>>
>>
>> [ ... ]
>>
>>> + err = clocksource_register_hz(cs, rate);
>>> + if (err) {
>>> + dev_err(dev, "clocksource registration failed: %d\n", err);
>>> + clk_disable_unprepare(ost->clk);
>>> + return err;
>>> + }
>>> +
>>> + /* Cannot register a sched_clock with interrupts on */
>>
>> Aren't they already disabled?
>
> Sorry for the late reply.
>
> No, they are not already disabled; this is what I get if I comment out
> the local_irq_save()/local_irq_restore():
>
> [ 0.361014] clocksource: ingenic-ost: mask: 0xffffffff max_cycles:
> 0xffffffff, max_idle_ns: 159271703898 ns
> [ 0.361515] clocksource: Switched to clocksource ingenic-ost
> [ 0.361686] ------------[ cut here ]------------
> [ 0.361893] WARNING: CPU: 0 PID: 1 at kernel/time/sched_clock.c:179
> sched_clock_register+0x7c/0x2e4
> [ 0.362174] CPU: 0 PID: 1 Comm: swapper Not tainted 5.4.0-rc5+ #461
> [ 0.362330] Stack : 80744558 80069b44 80770000 00000000 00000000
> 00dfd7a7 806e6db4 8106bb74
> [ 0.362619] 806f0000 81067ca4 806f31c7 80769478 00000020
> 10000400 8106bb20 00dfd7a7
> [ 0.362906] 00000000 00000000 80780000 00000000 00000007
> 00000001 00000049 3563722d
> [ 0.363191] 8106ba61 00000000 ffffffff 00000010 806f0000
> 00000000 00000000 806f0000
> [ 0.363477] 00000020 00000000 80714534 80770000 00000002
> 80319154 00000000 80770000
> [ 0.363762] ...
> [ 0.363906] Call Trace:
> [ 0.364087] [<8001af14>] show_stack+0x40/0x128
> [ 0.364289] [<8002fd88>] __warn+0xb8/0xe0
> [ 0.364478] [<8002fe14>] warn_slowpath_fmt+0x64/0xc0
> [ 0.364678] [<8072b1c8>] sched_clock_register+0x7c/0x2e4
> [ 0.364895] [<8073c874>] ingenic_ost_probe+0x224/0x248
> [ 0.365090] [<803d5394>] platform_drv_probe+0x40/0x94
> [ 0.365526] [<803d362c>] really_probe+0x104/0x374
> [ 0.365743] [<803d3ff0>] device_driver_attach+0x78/0x80
> [ 0.365938] [<803d4070>] __driver_attach+0x78/0x118
> [ 0.366129] [<803d1700>] bus_for_each_dev+0x7c/0xc8
> [ 0.366318] [<803d226c>] bus_add_driver+0x1bc/0x204
> [ 0.366513] [<803d4878>] driver_register+0x84/0x14c
> [ 0.366717] [<8073a144>] __platform_driver_probe+0x98/0x140
> [ 0.366931] [<80724e38>] do_one_initcall+0x84/0x1b4
> [ 0.367126] [<807250cc>] kernel_init_freeable+0x164/0x240
> [ 0.367318] [<805df75c>] kernel_init+0x10/0xf8
> [ 0.367510] [<8001542c>] ret_from_kernel_thread+0x14/0x1c
> [ 0.367722] ---[ end trace 7fedf00408fa3bed ]---
> [ 0.367985] sched_clock: 32 bits at 12MHz, resolution 83ns, wraps
> every 178956970966ns
>
> At kernel/time/sched_clock.c:179 there is:
> WARN_ON(!irqs_disabled());
That is strange, no drivers is doing that and no warning is appearing.
Isn't missing a local_irq_disable in the code path in the stack above?
>>> + local_irq_save(flags);
>>> + if (soc_info->is64bit)
>>> + sched_clock_register(ingenic_ost_read_cntl, 32, rate);
>>> + else
>>> + sched_clock_register(ingenic_ost_read_cnth, 32, rate);
>>> + local_irq_restore(flags);
>>> +
>>> + return 0;
>>> +}
>>> +
--
<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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] clocksource: Add driver for the Ingenic JZ47xx OST
2019-11-07 19:39 ` Daniel Lezcano
@ 2019-11-07 19:57 ` Paul Cercueil
2019-11-07 20:22 ` Daniel Lezcano
0 siblings, 1 reply; 8+ messages in thread
From: Paul Cercueil @ 2019-11-07 19:57 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Thomas Gleixner, od, linux-kernel, Maarten ter Huurne,
Mathieu Malaterre, Artur Rojek
Le jeu., nov. 7, 2019 at 20:39, Daniel Lezcano
<daniel.lezcano@linaro.org> a écrit :
> On 07/11/2019 16:56, Paul Cercueil wrote:
>> Hi Daniel,
>>
>> On 2019-08-16 16:54, Daniel Lezcano wrote:
>>> On 09/08/2019 14:38, Paul Cercueil wrote:
>>>> From: Maarten ter Huurne <maarten@treewalker.org>
>>>>
>>>> OST is the OS Timer, a 64-bit timer/counter with buffered reading.
>>>>
>>>> SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and
>>>> JZ4780 have a 64-bit OST.
>>>>
>>>> This driver will register both a clocksource and a sched_clock to
>>>> the
>>>> system.
>>>>
>>>> Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
>>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>> Tested-by: Mathieu Malaterre <malat@debian.org>
>>>> Tested-by: Artur Rojek <contact@artur-rojek.eu>
>>>> ---
>>>>
>>>
>>> [ ... ]
>>>
>>>> + err = clocksource_register_hz(cs, rate);
>>>> + if (err) {
>>>> + dev_err(dev, "clocksource registration failed: %d\n",
>>>> err);
>>>> + clk_disable_unprepare(ost->clk);
>>>> + return err;
>>>> + }
>>>> +
>>>> + /* Cannot register a sched_clock with interrupts on */
>>>
>>> Aren't they already disabled?
>>
>> Sorry for the late reply.
>>
>> No, they are not already disabled; this is what I get if I comment
>> out
>> the local_irq_save()/local_irq_restore():
>>
>> [ 0.361014] clocksource: ingenic-ost: mask: 0xffffffff
>> max_cycles:
>> 0xffffffff, max_idle_ns: 159271703898 ns
>> [ 0.361515] clocksource: Switched to clocksource ingenic-ost
>> [ 0.361686] ------------[ cut here ]------------
>> [ 0.361893] WARNING: CPU: 0 PID: 1 at
>> kernel/time/sched_clock.c:179
>> sched_clock_register+0x7c/0x2e4
>> [ 0.362174] CPU: 0 PID: 1 Comm: swapper Not tainted 5.4.0-rc5+
>> #461
>> [ 0.362330] Stack : 80744558 80069b44 80770000 00000000 00000000
>> 00dfd7a7 806e6db4 8106bb74
>> [ 0.362619] 806f0000 81067ca4 806f31c7 80769478 00000020
>> 10000400 8106bb20 00dfd7a7
>> [ 0.362906] 00000000 00000000 80780000 00000000 00000007
>> 00000001 00000049 3563722d
>> [ 0.363191] 8106ba61 00000000 ffffffff 00000010 806f0000
>> 00000000 00000000 806f0000
>> [ 0.363477] 00000020 00000000 80714534 80770000 00000002
>> 80319154 00000000 80770000
>> [ 0.363762] ...
>> [ 0.363906] Call Trace:
>> [ 0.364087] [<8001af14>] show_stack+0x40/0x128
>> [ 0.364289] [<8002fd88>] __warn+0xb8/0xe0
>> [ 0.364478] [<8002fe14>] warn_slowpath_fmt+0x64/0xc0
>> [ 0.364678] [<8072b1c8>] sched_clock_register+0x7c/0x2e4
>> [ 0.364895] [<8073c874>] ingenic_ost_probe+0x224/0x248
>> [ 0.365090] [<803d5394>] platform_drv_probe+0x40/0x94
>> [ 0.365526] [<803d362c>] really_probe+0x104/0x374
>> [ 0.365743] [<803d3ff0>] device_driver_attach+0x78/0x80
>> [ 0.365938] [<803d4070>] __driver_attach+0x78/0x118
>> [ 0.366129] [<803d1700>] bus_for_each_dev+0x7c/0xc8
>> [ 0.366318] [<803d226c>] bus_add_driver+0x1bc/0x204
>> [ 0.366513] [<803d4878>] driver_register+0x84/0x14c
>> [ 0.366717] [<8073a144>] __platform_driver_probe+0x98/0x140
>> [ 0.366931] [<80724e38>] do_one_initcall+0x84/0x1b4
>> [ 0.367126] [<807250cc>] kernel_init_freeable+0x164/0x240
>> [ 0.367318] [<805df75c>] kernel_init+0x10/0xf8
>> [ 0.367510] [<8001542c>] ret_from_kernel_thread+0x14/0x1c
>> [ 0.367722] ---[ end trace 7fedf00408fa3bed ]---
>> [ 0.367985] sched_clock: 32 bits at 12MHz, resolution 83ns, wraps
>> every 178956970966ns
>>
>> At kernel/time/sched_clock.c:179 there is:
>> WARN_ON(!irqs_disabled());
>
> That is strange, no drivers is doing that and no warning is appearing.
>
> Isn't missing a local_irq_disable in the code path in the stack above?
I think it comes to the fact that the other drivers are probed much
earlier in the boot process, while this one is probed as a regular
platform device driver.
>
>>>> + local_irq_save(flags);
>>>> + if (soc_info->is64bit)
>>>> + sched_clock_register(ingenic_ost_read_cntl, 32, rate);
>>>> + else
>>>> + sched_clock_register(ingenic_ost_read_cnth, 32, rate);
>>>> + local_irq_restore(flags);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>
>
> --
> <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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] clocksource: Add driver for the Ingenic JZ47xx OST
2019-11-07 19:57 ` Paul Cercueil
@ 2019-11-07 20:22 ` Daniel Lezcano
2019-11-07 20:34 ` Paul Cercueil
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Lezcano @ 2019-11-07 20:22 UTC (permalink / raw)
To: Paul Cercueil
Cc: Thomas Gleixner, od, linux-kernel, Maarten ter Huurne,
Mathieu Malaterre, Artur Rojek
On 07/11/2019 20:57, Paul Cercueil wrote:
>
>
> Le jeu., nov. 7, 2019 at 20:39, Daniel Lezcano
> <daniel.lezcano@linaro.org> a écrit :
>> On 07/11/2019 16:56, Paul Cercueil wrote:
>>> Hi Daniel,
>>>
>>> On 2019-08-16 16:54, Daniel Lezcano wrote:
>>>> On 09/08/2019 14:38, Paul Cercueil wrote:
>>>>> From: Maarten ter Huurne <maarten@treewalker.org>
>>>>>
>>>>> OST is the OS Timer, a 64-bit timer/counter with buffered reading.
>>>>>
>>>>> SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and
>>>>> JZ4780 have a 64-bit OST.
>>>>>
>>>>> This driver will register both a clocksource and a sched_clock to the
>>>>> system.
>>>>>
>>>>> Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
>>>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>>> Tested-by: Mathieu Malaterre <malat@debian.org>
>>>>> Tested-by: Artur Rojek <contact@artur-rojek.eu>
>>>>> ---
>>>>>
>>>>
>>>> [ ... ]
>>>>
>>>>> + err = clocksource_register_hz(cs, rate);
>>>>> + if (err) {
>>>>> + dev_err(dev, "clocksource registration failed: %d\n", err);
>>>>> + clk_disable_unprepare(ost->clk);
>>>>> + return err;
>>>>> + }
>>>>> +
>>>>> + /* Cannot register a sched_clock with interrupts on */
>>>>
>>>> Aren't they already disabled?
>>>
>>> Sorry for the late reply.
>>>
>>> No, they are not already disabled; this is what I get if I comment out
>>> the local_irq_save()/local_irq_restore():
>>>
>>> [ 0.361014] clocksource: ingenic-ost: mask: 0xffffffff max_cycles:
>>> 0xffffffff, max_idle_ns: 159271703898 ns
>>> [ 0.361515] clocksource: Switched to clocksource ingenic-ost
>>> [ 0.361686] ------------[ cut here ]------------
>>> [ 0.361893] WARNING: CPU: 0 PID: 1 at kernel/time/sched_clock.c:179
>>> sched_clock_register+0x7c/0x2e4
>>> [ 0.362174] CPU: 0 PID: 1 Comm: swapper Not tainted 5.4.0-rc5+ #461
>>> [ 0.362330] Stack : 80744558 80069b44 80770000 00000000 00000000
>>> 00dfd7a7 806e6db4 8106bb74
>>> [ 0.362619] 806f0000 81067ca4 806f31c7 80769478 00000020
>>> 10000400 8106bb20 00dfd7a7
>>> [ 0.362906] 00000000 00000000 80780000 00000000 00000007
>>> 00000001 00000049 3563722d
>>> [ 0.363191] 8106ba61 00000000 ffffffff 00000010 806f0000
>>> 00000000 00000000 806f0000
>>> [ 0.363477] 00000020 00000000 80714534 80770000 00000002
>>> 80319154 00000000 80770000
>>> [ 0.363762] ...
>>> [ 0.363906] Call Trace:
>>> [ 0.364087] [<8001af14>] show_stack+0x40/0x128
>>> [ 0.364289] [<8002fd88>] __warn+0xb8/0xe0
>>> [ 0.364478] [<8002fe14>] warn_slowpath_fmt+0x64/0xc0
>>> [ 0.364678] [<8072b1c8>] sched_clock_register+0x7c/0x2e4
>>> [ 0.364895] [<8073c874>] ingenic_ost_probe+0x224/0x248
>>> [ 0.365090] [<803d5394>] platform_drv_probe+0x40/0x94
>>> [ 0.365526] [<803d362c>] really_probe+0x104/0x374
>>> [ 0.365743] [<803d3ff0>] device_driver_attach+0x78/0x80
>>> [ 0.365938] [<803d4070>] __driver_attach+0x78/0x118
>>> [ 0.366129] [<803d1700>] bus_for_each_dev+0x7c/0xc8
>>> [ 0.366318] [<803d226c>] bus_add_driver+0x1bc/0x204
>>> [ 0.366513] [<803d4878>] driver_register+0x84/0x14c
>>> [ 0.366717] [<8073a144>] __platform_driver_probe+0x98/0x140
>>> [ 0.366931] [<80724e38>] do_one_initcall+0x84/0x1b4
>>> [ 0.367126] [<807250cc>] kernel_init_freeable+0x164/0x240
>>> [ 0.367318] [<805df75c>] kernel_init+0x10/0xf8
>>> [ 0.367510] [<8001542c>] ret_from_kernel_thread+0x14/0x1c
>>> [ 0.367722] ---[ end trace 7fedf00408fa3bed ]---
>>> [ 0.367985] sched_clock: 32 bits at 12MHz, resolution 83ns, wraps
>>> every 178956970966ns
>>>
>>> At kernel/time/sched_clock.c:179 there is:
>>> WARN_ON(!irqs_disabled());
>>
>> That is strange, no drivers is doing that and no warning is appearing.
>>
>> Isn't missing a local_irq_disable in the code path in the stack above?
>
> I think it comes to the fact that the other drivers are probed much
> earlier in the boot process, while this one is probed as a regular
> platform device driver.
There are other drivers doing and nobody is disabling the interrupt:
em_sti.c:static struct platform_driver em_sti_device_driver = {
ingenic-timer.c:static struct platform_driver ingenic_tcu_driver = {
sh_cmt.c:static struct platform_driver sh_cmt_device_driver = {
sh_mtu2.c:static struct platform_driver sh_mtu2_device_driver = {
sh_tmu.c:static struct platform_driver sh_tmu_device_driver = {
timer-ti-dm.c:static struct platform_driver omap_dm_timer_driver = {
>>>>> + local_irq_save(flags);
>>>>> + if (soc_info->is64bit)
>>>>> + sched_clock_register(ingenic_ost_read_cntl, 32, rate);
>>>>> + else
>>>>> + sched_clock_register(ingenic_ost_read_cnth, 32, rate);
>>>>> + local_irq_restore(flags);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>
>>
>> --
>> <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
>>
>
>
--
<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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] clocksource: Add driver for the Ingenic JZ47xx OST
2019-11-07 20:22 ` Daniel Lezcano
@ 2019-11-07 20:34 ` Paul Cercueil
2019-12-10 9:19 ` Daniel Lezcano
0 siblings, 1 reply; 8+ messages in thread
From: Paul Cercueil @ 2019-11-07 20:34 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Thomas Gleixner, od, linux-kernel, Maarten ter Huurne,
Mathieu Malaterre, Artur Rojek
Le jeu., nov. 7, 2019 at 21:22, Daniel Lezcano
<daniel.lezcano@linaro.org> a écrit :
> On 07/11/2019 20:57, Paul Cercueil wrote:
>>
>>
>> Le jeu., nov. 7, 2019 at 20:39, Daniel Lezcano
>> <daniel.lezcano@linaro.org> a écrit :
>>> On 07/11/2019 16:56, Paul Cercueil wrote:
>>>> Hi Daniel,
>>>>
>>>> On 2019-08-16 16:54, Daniel Lezcano wrote:
>>>>> On 09/08/2019 14:38, Paul Cercueil wrote:
>>>>>> From: Maarten ter Huurne <maarten@treewalker.org>
>>>>>>
>>>>>> OST is the OS Timer, a 64-bit timer/counter with buffered
>>>>>> reading.
>>>>>>
>>>>>> SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770
>>>>>> and
>>>>>> JZ4780 have a 64-bit OST.
>>>>>>
>>>>>> This driver will register both a clocksource and a sched_clock
>>>>>> to the
>>>>>> system.
>>>>>>
>>>>>> Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
>>>>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>>>> Tested-by: Mathieu Malaterre <malat@debian.org>
>>>>>> Tested-by: Artur Rojek <contact@artur-rojek.eu>
>>>>>> ---
>>>>>>
>>>>>
>>>>> [ ... ]
>>>>>
>>>>>> + err = clocksource_register_hz(cs, rate);
>>>>>> + if (err) {
>>>>>> + dev_err(dev, "clocksource registration failed: %d\n",
>>>>>> err);
>>>>>> + clk_disable_unprepare(ost->clk);
>>>>>> + return err;
>>>>>> + }
>>>>>> +
>>>>>> + /* Cannot register a sched_clock with interrupts on */
>>>>>
>>>>> Aren't they already disabled?
>>>>
>>>> Sorry for the late reply.
>>>>
>>>> No, they are not already disabled; this is what I get if I
>>>> comment out
>>>> the local_irq_save()/local_irq_restore():
>>>>
>>>> [ 0.361014] clocksource: ingenic-ost: mask: 0xffffffff
>>>> max_cycles:
>>>> 0xffffffff, max_idle_ns: 159271703898 ns
>>>> [ 0.361515] clocksource: Switched to clocksource ingenic-ost
>>>> [ 0.361686] ------------[ cut here ]------------
>>>> [ 0.361893] WARNING: CPU: 0 PID: 1 at
>>>> kernel/time/sched_clock.c:179
>>>> sched_clock_register+0x7c/0x2e4
>>>> [ 0.362174] CPU: 0 PID: 1 Comm: swapper Not tainted
>>>> 5.4.0-rc5+ #461
>>>> [ 0.362330] Stack : 80744558 80069b44 80770000 00000000
>>>> 00000000
>>>> 00dfd7a7 806e6db4 8106bb74
>>>> [ 0.362619] 806f0000 81067ca4 806f31c7 80769478
>>>> 00000020
>>>> 10000400 8106bb20 00dfd7a7
>>>> [ 0.362906] 00000000 00000000 80780000 00000000
>>>> 00000007
>>>> 00000001 00000049 3563722d
>>>> [ 0.363191] 8106ba61 00000000 ffffffff 00000010
>>>> 806f0000
>>>> 00000000 00000000 806f0000
>>>> [ 0.363477] 00000020 00000000 80714534 80770000
>>>> 00000002
>>>> 80319154 00000000 80770000
>>>> [ 0.363762] ...
>>>> [ 0.363906] Call Trace:
>>>> [ 0.364087] [<8001af14>] show_stack+0x40/0x128
>>>> [ 0.364289] [<8002fd88>] __warn+0xb8/0xe0
>>>> [ 0.364478] [<8002fe14>] warn_slowpath_fmt+0x64/0xc0
>>>> [ 0.364678] [<8072b1c8>] sched_clock_register+0x7c/0x2e4
>>>> [ 0.364895] [<8073c874>] ingenic_ost_probe+0x224/0x248
>>>> [ 0.365090] [<803d5394>] platform_drv_probe+0x40/0x94
>>>> [ 0.365526] [<803d362c>] really_probe+0x104/0x374
>>>> [ 0.365743] [<803d3ff0>] device_driver_attach+0x78/0x80
>>>> [ 0.365938] [<803d4070>] __driver_attach+0x78/0x118
>>>> [ 0.366129] [<803d1700>] bus_for_each_dev+0x7c/0xc8
>>>> [ 0.366318] [<803d226c>] bus_add_driver+0x1bc/0x204
>>>> [ 0.366513] [<803d4878>] driver_register+0x84/0x14c
>>>> [ 0.366717] [<8073a144>] __platform_driver_probe+0x98/0x140
>>>> [ 0.366931] [<80724e38>] do_one_initcall+0x84/0x1b4
>>>> [ 0.367126] [<807250cc>] kernel_init_freeable+0x164/0x240
>>>> [ 0.367318] [<805df75c>] kernel_init+0x10/0xf8
>>>> [ 0.367510] [<8001542c>] ret_from_kernel_thread+0x14/0x1c
>>>> [ 0.367722] ---[ end trace 7fedf00408fa3bed ]---
>>>> [ 0.367985] sched_clock: 32 bits at 12MHz, resolution 83ns,
>>>> wraps
>>>> every 178956970966ns
>>>>
>>>> At kernel/time/sched_clock.c:179 there is:
>>>> WARN_ON(!irqs_disabled());
>>>
>>> That is strange, no drivers is doing that and no warning is
>>> appearing.
>>>
>>> Isn't missing a local_irq_disable in the code path in the stack
>>> above?
>>
>> I think it comes to the fact that the other drivers are probed much
>> earlier in the boot process, while this one is probed as a regular
>> platform device driver.
>
>
> There are other drivers doing and nobody is disabling the interrupt:
>
> em_sti.c:static struct platform_driver em_sti_device_driver = {
> ingenic-timer.c:static struct platform_driver ingenic_tcu_driver = {
> sh_cmt.c:static struct platform_driver sh_cmt_device_driver = {
> sh_mtu2.c:static struct platform_driver sh_mtu2_device_driver = {
> sh_tmu.c:static struct platform_driver sh_tmu_device_driver = {
> timer-ti-dm.c:static struct platform_driver omap_dm_timer_driver = {
Yes, but they don't register a sched_clock at all (except for
ingenic-timer, but it does probe using TIMER_OF_DECLARE), and this
warning is in sched_clock_register().
>
>>>>>> + local_irq_save(flags);
>>>>>> + if (soc_info->is64bit)
>>>>>> + sched_clock_register(ingenic_ost_read_cntl, 32, rate);
>>>>>> + else
>>>>>> + sched_clock_register(ingenic_ost_read_cnth, 32, rate);
>>>>>> + local_irq_restore(flags);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>
>>>
>>> --
>>> <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
>>>
>>
>>
>
>
> --
> <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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] clocksource: Add driver for the Ingenic JZ47xx OST
2019-11-07 20:34 ` Paul Cercueil
@ 2019-12-10 9:19 ` Daniel Lezcano
0 siblings, 0 replies; 8+ messages in thread
From: Daniel Lezcano @ 2019-12-10 9:19 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Paul Cercueil, od, linux-kernel, Maarten ter Huurne,
Mathieu Malaterre, Artur Rojek
On 07/11/2019 21:34, Paul Cercueil wrote:
>
>
> Le jeu., nov. 7, 2019 at 21:22, Daniel Lezcano
> <daniel.lezcano@linaro.org> a écrit :
>> On 07/11/2019 20:57, Paul Cercueil wrote:
>>>
>>>
>>> Le jeu., nov. 7, 2019 at 20:39, Daniel Lezcano
>>> <daniel.lezcano@linaro.org> a écrit :
>>>> On 07/11/2019 16:56, Paul Cercueil wrote:
>>>>> Hi Daniel,
>>>>>
>>>>> On 2019-08-16 16:54, Daniel Lezcano wrote:
>>>>>> On 09/08/2019 14:38, Paul Cercueil wrote:
>>>>>>> From: Maarten ter Huurne <maarten@treewalker.org>
>>>>>>>
>>>>>>> OST is the OS Timer, a 64-bit timer/counter with buffered reading.
>>>>>>>
>>>>>>> SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and
>>>>>>> JZ4780 have a 64-bit OST.
>>>>>>>
>>>>>>> This driver will register both a clocksource and a sched_clock
>>>>>>> to the
>>>>>>> system.
>>>>>>>
>>>>>>> Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
>>>>>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>>>>> Tested-by: Mathieu Malaterre <malat@debian.org>
>>>>>>> Tested-by: Artur Rojek <contact@artur-rojek.eu>
>>>>>>> ---
>>>>>>>
>>>>>>
>>>>>> [ ... ]
>>>>>>
>>>>>>> + err = clocksource_register_hz(cs, rate);
>>>>>>> + if (err) {
>>>>>>> + dev_err(dev, "clocksource registration failed: %d\n",
>>>>>>> err);
>>>>>>> + clk_disable_unprepare(ost->clk);
>>>>>>> + return err;
>>>>>>> + }
>>>>>>> +
>>>>>>> + /* Cannot register a sched_clock with interrupts on */
>>>>>>
>>>>>> Aren't they already disabled?
>>>>>
>>>>> Sorry for the late reply.
>>>>>
>>>>> No, they are not already disabled; this is what I get if I
>>>>> comment out
>>>>> the local_irq_save()/local_irq_restore():
>>>>>
>>>>> [ 0.361014] clocksource: ingenic-ost: mask: 0xffffffff
>>>>> max_cycles:
>>>>> 0xffffffff, max_idle_ns: 159271703898 ns
>>>>> [ 0.361515] clocksource: Switched to clocksource ingenic-ost
>>>>> [ 0.361686] ------------[ cut here ]------------
>>>>> [ 0.361893] WARNING: CPU: 0 PID: 1 at
>>>>> kernel/time/sched_clock.c:179
>>>>> sched_clock_register+0x7c/0x2e4
>>>>> [ 0.362174] CPU: 0 PID: 1 Comm: swapper Not tainted 5.4.0-rc5+
>>>>> #461
>>>>> [ 0.362330] Stack : 80744558 80069b44 80770000 00000000 00000000
>>>>> 00dfd7a7 806e6db4 8106bb74
>>>>> [ 0.362619] 806f0000 81067ca4 806f31c7 80769478 00000020
>>>>> 10000400 8106bb20 00dfd7a7
>>>>> [ 0.362906] 00000000 00000000 80780000 00000000 00000007
>>>>> 00000001 00000049 3563722d
>>>>> [ 0.363191] 8106ba61 00000000 ffffffff 00000010 806f0000
>>>>> 00000000 00000000 806f0000
>>>>> [ 0.363477] 00000020 00000000 80714534 80770000 00000002
>>>>> 80319154 00000000 80770000
>>>>> [ 0.363762] ...
>>>>> [ 0.363906] Call Trace:
>>>>> [ 0.364087] [<8001af14>] show_stack+0x40/0x128
>>>>> [ 0.364289] [<8002fd88>] __warn+0xb8/0xe0
>>>>> [ 0.364478] [<8002fe14>] warn_slowpath_fmt+0x64/0xc0
>>>>> [ 0.364678] [<8072b1c8>] sched_clock_register+0x7c/0x2e4
>>>>> [ 0.364895] [<8073c874>] ingenic_ost_probe+0x224/0x248
>>>>> [ 0.365090] [<803d5394>] platform_drv_probe+0x40/0x94
>>>>> [ 0.365526] [<803d362c>] really_probe+0x104/0x374
>>>>> [ 0.365743] [<803d3ff0>] device_driver_attach+0x78/0x80
>>>>> [ 0.365938] [<803d4070>] __driver_attach+0x78/0x118
>>>>> [ 0.366129] [<803d1700>] bus_for_each_dev+0x7c/0xc8
>>>>> [ 0.366318] [<803d226c>] bus_add_driver+0x1bc/0x204
>>>>> [ 0.366513] [<803d4878>] driver_register+0x84/0x14c
>>>>> [ 0.366717] [<8073a144>] __platform_driver_probe+0x98/0x140
>>>>> [ 0.366931] [<80724e38>] do_one_initcall+0x84/0x1b4
>>>>> [ 0.367126] [<807250cc>] kernel_init_freeable+0x164/0x240
>>>>> [ 0.367318] [<805df75c>] kernel_init+0x10/0xf8
>>>>> [ 0.367510] [<8001542c>] ret_from_kernel_thread+0x14/0x1c
>>>>> [ 0.367722] ---[ end trace 7fedf00408fa3bed ]---
>>>>> [ 0.367985] sched_clock: 32 bits at 12MHz, resolution 83ns, wraps
>>>>> every 178956970966ns
>>>>>
>>>>> At kernel/time/sched_clock.c:179 there is:
>>>>> WARN_ON(!irqs_disabled());
>>>>
>>>> That is strange, no drivers is doing that and no warning is appearing.
>>>>
>>>> Isn't missing a local_irq_disable in the code path in the stack above?
>>>
>>> I think it comes to the fact that the other drivers are probed much
>>> earlier in the boot process, while this one is probed as a regular
>>> platform device driver.
>>
>>
>> There are other drivers doing and nobody is disabling the interrupt:
>>
>> em_sti.c:static struct platform_driver em_sti_device_driver = {
>> ingenic-timer.c:static struct platform_driver ingenic_tcu_driver = {
>> sh_cmt.c:static struct platform_driver sh_cmt_device_driver = {
>> sh_mtu2.c:static struct platform_driver sh_mtu2_device_driver = {
>> sh_tmu.c:static struct platform_driver sh_tmu_device_driver = {
>> timer-ti-dm.c:static struct platform_driver omap_dm_timer_driver = {
>
> Yes, but they don't register a sched_clock at all (except for
> ingenic-timer, but it does probe using TIMER_OF_DECLARE), and this
> warning is in sched_clock_register().
>
>>
>>>>>>> + local_irq_save(flags);
>>>>>>> + if (soc_info->is64bit)
>>>>>>> + sched_clock_register(ingenic_ost_read_cntl, 32, rate);
>>>>>>> + else
>>>>>>> + sched_clock_register(ingenic_ost_read_cnth, 32, rate);
>>>>>>> + local_irq_restore(flags);
>>>>>>> +
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +
Thomas,
the local_irq_save/restore calls shouldn't be in the
sched_clock_register() function instead of showing a warning?
--
<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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-12-10 9:19 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-09 12:38 [PATCH] clocksource: Add driver for the Ingenic JZ47xx OST Paul Cercueil
2019-08-16 14:54 ` Daniel Lezcano
2019-11-07 15:56 ` Paul Cercueil
2019-11-07 19:39 ` Daniel Lezcano
2019-11-07 19:57 ` Paul Cercueil
2019-11-07 20:22 ` Daniel Lezcano
2019-11-07 20:34 ` Paul Cercueil
2019-12-10 9:19 ` Daniel Lezcano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).