Re: [PATCH 02/17] clocksource: davinci-timer: new driver

pon., 14 sty 2019 o 13:20 Sekhar Nori [off-list ref] napisał(a):

Hi Bartosz,

On 11/01/19 10:51 PM, Bartosz Golaszewski wrote:

quoted

From: Bartosz Golaszewski <redacted>

Currently the clocksource and clockevent support for davinci platforms
lives in mach-davinci. It hard-codes many things, used global variables,
implements functionalities unused by any platform and has code fragments
scattered across many (often unrelated) files.

Implement a new, modern and simplified timer driver and put it into
drivers/clocksource. We still need to support legacy board files so
export a config structure and a function that allows machine code to
register the timer.

We don't check the return values of regmap reads and writes since with
mmio it's only likely to fail due to programmer's errors.

We also don't bother freeing resources on errors in
davinci_timer_register() as the system won't boot without a timer anyway.

Signed-off-by: Bartosz Golaszewski <redacted>

With this series, DA830 fails to boot. Rest of the devices are okay from
boot perspective.

DA830 is pretty unique because it uses the same timer-half for both
clocksource and clockevent. May be you can set the same configuration on
your DA850 to see the same issue? Else, I will enable low-level debug
and try to provide more debug data.

I can't boot da850 with the same config as da830 (0x60 compare
register, compare irq 74) even with the old timer code. Just to make
sure: does da830 boot fine with mainline v5.0-rc2?

Some minor comments below from quick look:

quoted

diff --git a/drivers/clocksource/timer-davinci.c b/drivers/clocksource/timer-davinci.c
new file mode 100644
index 000000000000..7282a1fda80f
--- /dev/null
+++ b/drivers/clocksource/timer-davinci.c

@@ -0,0 +1,415 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * TI DaVinci clocksource driver
+ *
+ * Copyright (C) 2019 Texas Instruments
+ * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
+ * (with some parts adopted from code by Kevin Hilman <khilman@baylibre.com>)

Did you really intend GPL v2 or later? The original code referred to
above is marked 2.0 only.

I'm not sure. Should it be GPLv2 only? I'm not well versed with licensing.

Bart

quoted

+ */
+
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/clocksource.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/regmap.h>
+#include <linux/sched_clock.h>
+
+#include <clocksource/timer-davinci.h>
+
+#define DAVINCI_TIMER_REG_TIM12                      0x10
+#define DAVINCI_TIMER_REG_TIM34                      0x14
+#define DAVINCI_TIMER_REG_PRD12                      0x18
+#define DAVINCI_TIMER_REG_PRD34                      0x1c
+#define DAVINCI_TIMER_REG_TCR                        0x20
+#define DAVINCI_TIMER_REG_TGCR                       0x24
+
+#define DAVINCI_TIMER_TIMMODE_MASK           0x0000000c
+#define DAVINCI_TIMER_RESET_MASK             0x00000003
+#define DAVINCI_TIMER_TIMMODE_32BIT_UNCHAINED        0x00000004
+#define DAVINCI_TIMER_UNRESET                        0x00000003

I think these are more readable if using BIT() and GENMASK()

Thanks,
Sekhar

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel