linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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).