[PATCH v2 0/2] Add drivers for Intel Keem Bay SoC timer block

[PATCH v2 0/2] Add drivers for Intel Keem Bay SoC timer block

[vijayakannan.ayyathurai]

From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>

Hi,

This patch set adds the driver for Intel Keem Bay SoC timer block, which
contains 32-bit general purpose timers, a 64 bit free running counter.

Patch 1 holds the Device Tree binding documentation and
Patch 2 holds the Device Driver for clocksource and clockevent.

It was tested on the Keem Bay evaluation module board.

Thanks,
Vijay

Changes since v1:
 - Add support for KEEMBAY_TIMER to get selected through Kconfig.platforms.
 - Add CLOCK_EVT_FEAT_DYNIRQ as part of clockevent feature.
 - Avoid overlapping reg regions across 2 device nodes.
 - Simplify 2 device nodes as 1 because both are from same IP block.
 - Adapt the driver code according to the new simplified devicetree.

Vijayakannan Ayyathurai (2):
  dt-bindings: timer: Add bindings for Intel Keem Bay SoC timer
  clocksource: Add Intel Keem Bay Timer Support

 .../bindings/timer/intel,keembay-timer.yaml   |  52 ++++
 arch/arm64/Kconfig.platforms                  |   1 +
 drivers/clocksource/Kconfig                   |  10 +
 drivers/clocksource/Makefile                  |   1 +
 drivers/clocksource/timer-keembay.c           | 225 ++++++++++++++++++
 5 files changed, 289 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/intel,keembay-timer.yaml
 create mode 100644 drivers/clocksource/timer-keembay.c


base-commit: 5c8fe583cce542aa0b84adc939ce85293de36e5e
prerequisite-patch-id: bbb340c3a34059ea6960e8b96f5ad3bf0b4ae7b0
prerequisite-patch-id: b71b548284a57a38ce96d9bb523eadfccabd6e02
-- 
2.17.1

[PATCH v2 1/2] dt-bindings: timer: Add bindings for Intel Keem Bay SoC timer

[vijayakannan.ayyathurai]

From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>

Add Device Tree bindings for the Timer IP, which used as clocksource and
clockevent device in the Intel Keem Bay SoC.

Acked-by: Mark Gross <mgross@linux.intel.com>
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
---
 .../bindings/timer/intel,keembay-timer.yaml   | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/intel,keembay-timer.yaml

diff --git a/Documentation/devicetree/bindings/timer/intel,keembay-timer.yaml b/Documentation/devicetree/bindings/timer/intel,keembay-timer.yaml
new file mode 100644
index 000000000000..197493336ac2
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/intel,keembay-timer.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/timer/intel,keembay-timer.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Intel Keem Bay SoC Timers
+
+maintainers:
+  - Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
+  - Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
+
+description:
+  Intel Keem Bay SoC Timers block contains 8 32-bit general purpose timers,
+  a free running 64-bit counter, a random number generator and a watchdog
+  timer. Each gpt can generate an individual interrupt.
+
+properties:
+  compatible:
+    enum:
+      - intel,keembay-timer
+
+  reg:
+    maxItems: 3
+
+  clocks:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #define KEEM_BAY_A53_TIM
+
+    timer@20330010 {
+        compatible = "intel,keembay-timer";
+        reg = <0x20330010 0xc>,
+              <0x203300e8 0xc>,
+              <0x20331000 0xc>;
+        clocks = <&scmi_clk KEEM_BAY_A53_TIM>;
+        interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
+    };
-- 
2.17.1

Re: [PATCH v2 1/2] dt-bindings: timer: Add bindings for Intel Keem Bay SoC timer

[Rob Herring]

On Wed, 30 Dec 2020 14:25:26 +0800, vijayakannan.ayyathurai@intel.com wrote:
> From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> 
> Add Device Tree bindings for the Timer IP, which used as clocksource and
> clockevent device in the Intel Keem Bay SoC.
> 
> Acked-by: Mark Gross <mgross@linux.intel.com>
> Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> ---
>  .../bindings/timer/intel,keembay-timer.yaml   | 52 +++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/timer/intel,keembay-timer.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/timer/intel,keembay-timer.example.dts:32.3-33.1 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:344: Documentation/devicetree/bindings/timer/intel,keembay-timer.example.dt.yaml] Error 1
make: *** [Makefile:1370: dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1421313

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

RE: [PATCH v2 1/2] dt-bindings: timer: Add bindings for Intel Keem Bay SoC timer

[Ayyathurai, Vijayakannan]

Hi Rob,

> From: Rob Herring <robh@kernel.org>
> > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> >
> > Add Device Tree bindings for the Timer IP, which used as clocksource and
> > clockevent device in the Intel Keem Bay SoC.
> >
> > Acked-by: Mark Gross <mgross@linux.intel.com>
> > Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Vijayakannan Ayyathurai
> <vijayakannan.ayyathurai@intel.com>
> > ---
> >  .../bindings/timer/intel,keembay-timer.yaml   | 52 +++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> >  create mode 100644
> Documentation/devicetree/bindings/timer/intel,keembay-timer.yaml
> >
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> Error: Documentation/devicetree/bindings/timer/intel,keembay-
> timer.example.dts:32.3-33.1 syntax error
> FATAL ERROR: Unable to parse input tree
> make[1]: *** [scripts/Makefile.lib:344:
> Documentation/devicetree/bindings/timer/intel,keembay-
> timer.example.dt.yaml] Error 1
> make: *** [Makefile:1370: dt_binding_check] Error 2
> 
> See https://patchwork.ozlabs.org/patch/1421313
> 
> This check can fail if there are any dependencies. The base for a patch
> series is generally the most recent rc1.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit.

Thanks for the review. Let me check again and re-submit in the next version.

Thanks,
Vijay

[PATCH v2 2/2] clocksource: Add Intel Keem Bay Timer Support

[vijayakannan.ayyathurai]

From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>

Add generic clocksource and clockevent driver for the timer IP used
in Intel Keem Bay SoC.

One free running Counter used as a clocksource device and one Timer
used as a clockevent device. Both are enabled through TIM_GEN_CONFIG
register. This register is in the DT resource index 2.

Timer and Counter base register is in the DT resource index 0 and 1
respectively. This register map/unmap handled by TIMER OF api.

Acked-by: Mark Gross <mgross@linux.intel.com>
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
---
 arch/arm64/Kconfig.platforms        |   1 +
 drivers/clocksource/Kconfig         |  10 ++
 drivers/clocksource/Makefile        |   1 +
 drivers/clocksource/timer-keembay.c | 225 ++++++++++++++++++++++++++++
 4 files changed, 237 insertions(+)
 create mode 100644 drivers/clocksource/timer-keembay.c

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 6eecdef538bd..12c0c39a27ff 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -146,6 +146,7 @@ config ARCH_HISI
 
 config ARCH_KEEMBAY
 	bool "Keem Bay SoC"
+	select KEEMBAY_TIMER
 	help
 	  This enables support for Intel Movidius SoC code-named Keem Bay.
 
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 14c7c4712478..cebe774fe104 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -728,4 +728,14 @@ config MICROCHIP_PIT64B
 	  modes and high resolution. It is used as a clocksource
 	  and a clockevent.
 
+config KEEMBAY_TIMER
+	bool "Intel Keembay timer driver"
+	depends on ARCH_KEEMBAY || (ARM64 && COMPILE_TEST)
+	select TIMER_OF
+	help
+	  This option enables the support for the Intel Keembay general
+	  purpose timer and free running counter driver. Each timer can
+	  generate an individual interrupt and the 64 bit counter can also
+	  be used as one of the clock source.
+
 endmenu
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 3c75cbbf8533..584628a56c76 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -93,3 +93,4 @@ obj-$(CONFIG_CSKY_MP_TIMER)		+= timer-mp-csky.o
 obj-$(CONFIG_GX6605S_TIMER)		+= timer-gx6605s.o
 obj-$(CONFIG_HYPERV_TIMER)		+= hyperv_timer.o
 obj-$(CONFIG_MICROCHIP_PIT64B)		+= timer-microchip-pit64b.o
+obj-$(CONFIG_KEEMBAY_TIMER)		+= timer-keembay.o
diff --git a/drivers/clocksource/timer-keembay.c b/drivers/clocksource/timer-keembay.c
new file mode 100644
index 000000000000..f5465b907ba4
--- /dev/null
+++ b/drivers/clocksource/timer-keembay.c
@@ -0,0 +1,225 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Intel Keembay Timer driver
+ *
+ * Copyright (C) 2020 Intel Corporation
+ */
+
+#include <linux/bits.h>
+#include <linux/interrupt.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/sizes.h>
+
+#include "timer-of.h"
+
+/* Registers offset */
+#define TIM_CNT_VAL_OFFSET		0
+#define TIM_RELOAD_VAL_OFFSET		SZ_4
+#define TIM_CONFIG_OFFSET		SZ_8
+
+/* Bit fields of TIM_GEN_CONFIG register */
+#define TIM_CONFIG_PRESCALER_ENABLE	BIT(2)
+
+/* Bit fields of TIM_CONFIG registers */
+#define TIM_CONFIG_INTERRUPT_PENDING	BIT(4)
+#define TIM_CONFIG_INTERRUPT_ENABLE	BIT(2)
+#define TIM_CONFIG_RESTART		BIT(1)
+#define TIM_CONFIG_ENABLE		BIT(0)
+
+#define TIM_RATING			200
+#define TIM_CLKSRC_BITS			SZ_64
+
+struct keembay_init_data {
+	struct timer_of	*cfg;
+	void __iomem	*base;
+	u32		mask;
+};
+
+static inline void keembay_timer_disable(void __iomem *base)
+{
+	writel(0, base + TIM_CONFIG_OFFSET);
+}
+
+static inline void keembay_timer_enable(void __iomem *base, u32 flags)
+{
+	writel(TIM_CONFIG_ENABLE | flags, base + TIM_CONFIG_OFFSET);
+}
+
+static inline void keembay_timer_update_counter(void __iomem *base, u32 val)
+{
+	writel(val, base + TIM_CNT_VAL_OFFSET);
+	writel(val, base + TIM_RELOAD_VAL_OFFSET);
+}
+
+static int keembay_timer_set_next_event(unsigned long evt, struct clock_event_device *ce)
+{
+	u32 flags = TIM_CONFIG_INTERRUPT_ENABLE;
+	struct timer_of *to = to_timer_of(ce);
+
+	keembay_timer_disable(timer_of_base(to));
+	keembay_timer_update_counter(timer_of_base(to), evt);
+	keembay_timer_enable(timer_of_base(to), flags);
+
+	return 0;
+}
+
+static int keembay_timer_periodic(struct clock_event_device *ce)
+{
+	u32 flags = TIM_CONFIG_INTERRUPT_ENABLE | TIM_CONFIG_RESTART;
+	struct timer_of *to = to_timer_of(ce);
+
+	keembay_timer_disable(timer_of_base(to));
+	keembay_timer_update_counter(timer_of_base(to), timer_of_period(to));
+	keembay_timer_enable(timer_of_base(to), flags);
+
+	return 0;
+}
+
+static int keembay_timer_shutdown(struct clock_event_device *ce)
+{
+	struct timer_of *to = to_timer_of(ce);
+
+	keembay_timer_disable(timer_of_base(to));
+
+	return 0;
+}
+
+static irqreturn_t keembay_timer_isr(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+	struct timer_of *to = to_timer_of(evt);
+	u32 val;
+
+	val = readl(timer_of_base(to) + TIM_CONFIG_OFFSET);
+	val &= ~TIM_CONFIG_INTERRUPT_PENDING;
+	writel(val, timer_of_base(to) + TIM_CONFIG_OFFSET);
+
+	if (clockevent_state_oneshot(evt))
+		keembay_timer_disable(timer_of_base(to));
+
+	evt->event_handler(evt);
+
+	return IRQ_HANDLED;
+}
+
+static int __init keembay_timer_setup(struct device_node *np, struct keembay_init_data *data)
+{
+	struct timer_of *to = data->cfg;
+	u32 val;
+
+	val = readl(data->base + TIM_CONFIG_OFFSET);
+	if (!(val & data->mask))
+		return -ENODEV;
+
+	return timer_of_init(np, to);
+}
+
+static void keembay_timer_cleanup(struct device_node *np, struct keembay_init_data *data)
+{
+	iounmap(data->base);
+}
+
+static struct timer_of keembay_ce_to = {
+	.flags	= TIMER_OF_IRQ | TIMER_OF_BASE | TIMER_OF_CLOCK,
+	.clkevt = {
+		.name			= "keembay_sys_clkevt",
+		.features		= CLOCK_EVT_FEAT_PERIODIC |
+					  CLOCK_EVT_FEAT_ONESHOT  |
+					  CLOCK_EVT_FEAT_DYNIRQ,
+		.rating			= TIM_RATING,
+		.set_next_event		= keembay_timer_set_next_event,
+		.set_state_periodic	= keembay_timer_periodic,
+		.set_state_shutdown	= keembay_timer_shutdown,
+	},
+	.of_base = {
+		.index = 0,
+	},
+	.of_irq = {
+		.handler = keembay_timer_isr,
+		.flags = IRQF_TIMER | IRQF_IRQPOLL,
+	},
+};
+
+static int __init keembay_clockevent_init(struct device_node *np,
+					  struct keembay_init_data *data)
+{
+	u32 val;
+	int ret;
+
+	data->mask = TIM_CONFIG_PRESCALER_ENABLE;
+	data->cfg = &keembay_ce_to;
+	ret = keembay_timer_setup(np, data);
+	if (ret)
+		return ret;
+
+	val = readl(data->base + TIM_RELOAD_VAL_OFFSET);
+
+	keembay_ce_to.clkevt.cpumask = cpumask_of(0);
+	keembay_ce_to.of_clk.rate = keembay_ce_to.of_clk.rate / (val + 1);
+
+	keembay_timer_disable(timer_of_base(&keembay_ce_to));
+
+	clockevents_config_and_register(&keembay_ce_to.clkevt,
+					timer_of_rate(&keembay_ce_to), 1, U32_MAX);
+	return 0;
+}
+
+static struct timer_of keembay_cs_to = {
+	.flags	= TIMER_OF_BASE | TIMER_OF_CLOCK,
+	.of_base = {
+		.index = 1,
+	},
+};
+
+static u64 notrace keembay_clocksource_read(struct clocksource *cs)
+{
+	return lo_hi_readq(timer_of_base(&keembay_cs_to));
+}
+
+static struct clocksource keembay_counter = {
+	.name			= "keembay_sys_counter",
+	.rating			= TIM_RATING,
+	.read			= keembay_clocksource_read,
+	.mask			= CLOCKSOURCE_MASK(TIM_CLKSRC_BITS),
+	.flags			= CLOCK_SOURCE_IS_CONTINUOUS |
+				  CLOCK_SOURCE_SUSPEND_NONSTOP,
+};
+
+static int __init keembay_clocksource_init(struct device_node *np,
+					   struct keembay_init_data *data)
+{
+	int ret;
+
+	data->mask = TIM_CONFIG_ENABLE;
+	data->cfg = &keembay_cs_to;
+	ret = keembay_timer_setup(np, data);
+	if (ret)
+		return ret;
+
+	return clocksource_register_hz(&keembay_counter, timer_of_rate(&keembay_cs_to));
+}
+
+static int __init keembay_timer_init(struct device_node *np)
+{
+	struct keembay_init_data data;
+	int ret;
+
+	data.base = of_iomap(np, 2);
+	if (!data.base)
+		return -ENXIO;
+
+	ret = keembay_clocksource_init(np, &data);
+	if (ret)
+		goto exit;
+
+	ret = keembay_clockevent_init(np, &data);
+
+exit:
+	keembay_timer_cleanup(np, &data);
+
+	return ret;
+}
+
+TIMER_OF_DECLARE(keembay_timer, "intel,keembay-timer", keembay_timer_init);
-- 
2.17.1

Re: [PATCH v2 2/2] clocksource: Add Intel Keem Bay Timer Support

[Daniel Lezcano]

On 30/12/2020 07:25, vijayakannan.ayyathurai@intel.com wrote:
> From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>

[ ... ]

> +static struct timer_of keembay_ce_to = {
> +	.flags	= TIMER_OF_IRQ | TIMER_OF_BASE | TIMER_OF_CLOCK,
> +	.clkevt = {
> +		.name			= "keembay_sys_clkevt",
> +		.features		= CLOCK_EVT_FEAT_PERIODIC |
> +					  CLOCK_EVT_FEAT_ONESHOT  |
> +					  CLOCK_EVT_FEAT_DYNIRQ,
> +		.rating			= TIM_RATING,
> +		.set_next_event		= keembay_timer_set_next_event,
> +		.set_state_periodic	= keembay_timer_periodic,
> +		.set_state_shutdown	= keembay_timer_shutdown,
> +	},
> +	.of_base = {
> +		.index = 0,
> +	},
> +	.of_irq = {
> +		.handler = keembay_timer_isr,
> +		.flags = IRQF_TIMER | IRQF_IRQPOLL,

Is the IRQPOLL flag really needed here ?

> +	},
> +};
> +
> +static int __init keembay_clockevent_init(struct device_node *np,
> +					  struct keembay_init_data *data)
> +{
> +	u32 val;
> +	int ret;
> +
> +	data->mask = TIM_CONFIG_PRESCALER_ENABLE;
> +	data->cfg = &keembay_ce_to;
> +	ret = keembay_timer_setup(np, data);
> +	if (ret)
> +		return ret;
> +
> +	val = readl(data->base + TIM_RELOAD_VAL_OFFSET);
> +
> +	keembay_ce_to.clkevt.cpumask = cpumask_of(0);
> +	keembay_ce_to.of_clk.rate = keembay_ce_to.of_clk.rate / (val + 1);
> +
> +	keembay_timer_disable(timer_of_base(&keembay_ce_to));
> +
> +	clockevents_config_and_register(&keembay_ce_to.clkevt,
> +					timer_of_rate(&keembay_ce_to), 1, U32_MAX);
> +	return 0;
> +}
> +
> +static struct timer_of keembay_cs_to = {
> +	.flags	= TIMER_OF_BASE | TIMER_OF_CLOCK,
> +	.of_base = {
> +		.index = 1,
> +	},
> +};
> +
> +static u64 notrace keembay_clocksource_read(struct clocksource *cs)
> +{
> +	return lo_hi_readq(timer_of_base(&keembay_cs_to));
> +}
> +
> +static struct clocksource keembay_counter = {
> +	.name			= "keembay_sys_counter",
> +	.rating			= TIM_RATING,
> +	.read			= keembay_clocksource_read,
> +	.mask			= CLOCKSOURCE_MASK(TIM_CLKSRC_BITS),
> +	.flags			= CLOCK_SOURCE_IS_CONTINUOUS |
> +				  CLOCK_SOURCE_SUSPEND_NONSTOP,
> +};
> +
> +static int __init keembay_clocksource_init(struct device_node *np,
> +					   struct keembay_init_data *data)
> +{
> +	int ret;
> +
> +	data->mask = TIM_CONFIG_ENABLE;
> +	data->cfg = &keembay_cs_to;
> +	ret = keembay_timer_setup(np, data);
> +	if (ret)
> +		return ret;
> +
> +	return clocksource_register_hz(&keembay_counter, timer_of_rate(&keembay_cs_to));
> +}
> +
> +static int __init keembay_timer_init(struct device_node *np)
> +{
> +	struct keembay_init_data data;
> +	int ret;
> +
> +	data.base = of_iomap(np, 2);
> +	if (!data.base)
> +		return -ENXIO;
> +
> +	ret = keembay_clocksource_init(np, &data);
> +	if (ret)
> +		goto exit;
> +
> +	ret = keembay_clockevent_init(np, &data);

Is this missing ?

	if (ret)
		goto exit;

	return 0;

> +
> +exit:
> +	keembay_timer_cleanup(np, &data);
> +
> +	return ret;
> +}
> +
> +TIMER_OF_DECLARE(keembay_timer, "intel,keembay-timer", keembay_timer_init);
> 


-- 
<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

RE: [PATCH v2 2/2] clocksource: Add Intel Keem Bay Timer Support

[Ayyathurai, Vijayakannan]

Hi Daniel,

> From: Daniel Lezcano <daniel.lezcano@linaro.org>
> > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> 
> [ ... ]
> 
> > +static struct timer_of keembay_ce_to = {
> > +	.flags	= TIMER_OF_IRQ | TIMER_OF_BASE | TIMER_OF_CLOCK,
> > +	.clkevt = {
> > +		.name			= "keembay_sys_clkevt",
> > +		.features		= CLOCK_EVT_FEAT_PERIODIC |
> > +					  CLOCK_EVT_FEAT_ONESHOT  |
> > +					  CLOCK_EVT_FEAT_DYNIRQ,
> > +		.rating			= TIM_RATING,
> > +		.set_next_event		=
> keembay_timer_set_next_event,
> > +		.set_state_periodic	= keembay_timer_periodic,
> > +		.set_state_shutdown	= keembay_timer_shutdown,
> > +	},
> > +	.of_base = {
> > +		.index = 0,
> > +	},
> > +	.of_irq = {
> > +		.handler = keembay_timer_isr,
> > +		.flags = IRQF_TIMER | IRQF_IRQPOLL,
> 
> Is the IRQPOLL flag really needed here ?
> 

Not really needed. I will remove this redundant Flag in my next version.

> > +static int __init keembay_timer_init(struct device_node *np)
> > +{
> > +	struct keembay_init_data data;
> > +	int ret;
> > +
> > +	data.base = of_iomap(np, 2);
> > +	if (!data.base)
> > +		return -ENXIO;
> > +
> > +	ret = keembay_clocksource_init(np, &data);
> > +	if (ret)
> > +		goto exit;
> > +
> > +	ret = keembay_clockevent_init(np, &data);
> 
> Is this missing ?
> 

Yes. Either case it goes to the exit path. So I thought of avoiding this error handling code.

> 	if (ret)
> 		goto exit;
> 
> 	return 0;
> 
> > +
> > +exit:
> > +	keembay_timer_cleanup(np, &data);
> > +
> > +	return ret;
> > +}
> > +
> > +TIMER_OF_DECLARE(keembay_timer, "intel,keembay-timer",
> keembay_timer_init);
> >
> 

Thanks,
Vijay

Re: [PATCH v2 2/2] clocksource: Add Intel Keem Bay Timer Support

[andriy.shevchenko]

On Tue, Jan 19, 2021 at 02:56:36AM +0000, Ayyathurai, Vijayakannan wrote:

...

> > > +	data.base = of_iomap(np, 2);
> > > +	if (!data.base)
> > > +		return -ENXIO;
> > > +
> > > +	ret = keembay_clocksource_init(np, &data);
> > > +	if (ret)
> > > +		goto exit;
> > > +
> > > +	ret = keembay_clockevent_init(np, &data);
> > 
> > Is this missing ?
> > 
> 
> Yes. Either case it goes to the exit path. So I thought of avoiding this error handling code.

The point is that in success you probably won't call keembay_timer_cleanup().

> > 	if (ret)
> > 		goto exit;
> > 
> > 	return 0;
> > 
> > > +exit:
> > > +	keembay_timer_cleanup(np, &data);
> > > +
> > > +	return ret;
> > > +}

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2 2/2] clocksource: Add Intel Keem Bay Timer Support

[Ayyathurai, Vijayakannan]

Hi Andy,

> From: andriy.shevchenko@linux.intel.com
> 
> > > > +	data.base = of_iomap(np, 2);
> > > > +	if (!data.base)
> > > > +		return -ENXIO;
> > > > +
> > > > +	ret = keembay_clocksource_init(np, &data);
> > > > +	if (ret)
> > > > +		goto exit;
> > > > +
> > > > +	ret = keembay_clockevent_init(np, &data);
> > >
> > > Is this missing ?
> > >
> >
> > Yes. Either case it goes to the exit path. So I thought of avoiding this error
> handling code.
> 
> The point is that in success you probably won't call keembay_timer_cleanup().
> 

Yes. You are right, if I use this error handling code.

> > > 	if (ret)
> > > 		goto exit;
> > >
> > > 	return 0;
> > >
> > > > +exit:
> > > > +	keembay_timer_cleanup(np, &data);
> > > > +
> > > > +	return ret;
> > > > +}
> 
Thanks,
Vijay

RE: [PATCH v2 0/2] Add drivers for Intel Keem Bay SoC timer block

[Ayyathurai, Vijayakannan]

Hi,

> From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> 
> Changes since v1:
>  - Add support for KEEMBAY_TIMER to get selected through Kconfig.platforms.
>  - Add CLOCK_EVT_FEAT_DYNIRQ as part of clockevent feature.
>  - Avoid overlapping reg regions across 2 device nodes.
>  - Simplify 2 device nodes as 1 because both are from same IP block.
>  - Adapt the driver code according to the new simplified devicetree.
> 
> Vijayakannan Ayyathurai (2):
>   dt-bindings: timer: Add bindings for Intel Keem Bay SoC timer
>   clocksource: Add Intel Keem Bay Timer Support

Kindly help us to review this updated patch(v2) set.

Thanks,
Vijay

Re: [PATCH v2 0/2] Add drivers for Intel Keem Bay SoC timer block

[Daniel Lezcano]

On 13/01/2021 11:54, Ayyathurai, Vijayakannan wrote:
> Hi,
> 
>> From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
>>
>> Changes since v1:
>>  - Add support for KEEMBAY_TIMER to get selected through Kconfig.platforms.
>>  - Add CLOCK_EVT_FEAT_DYNIRQ as part of clockevent feature.
>>  - Avoid overlapping reg regions across 2 device nodes.
>>  - Simplify 2 device nodes as 1 because both are from same IP block.
>>  - Adapt the driver code according to the new simplified devicetree.
>>
>> Vijayakannan Ayyathurai (2):
>>   dt-bindings: timer: Add bindings for Intel Keem Bay SoC timer
>>   clocksource: Add Intel Keem Bay Timer Support
> 
> Kindly help us to review this updated patch(v2) set.

Review in progress ... :)


-- 
<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

RE: [PATCH v2 0/2] Add drivers for Intel Keem Bay SoC timer block

[Ayyathurai, Vijayakannan]

Hi Daniel,

> From: Daniel Lezcano <daniel.lezcano@linaro.org>
> >> From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> >>
> >> Changes since v1:
> >>  - Add support for KEEMBAY_TIMER to get selected through
> Kconfig.platforms.
> >>  - Add CLOCK_EVT_FEAT_DYNIRQ as part of clockevent feature.
> >>  - Avoid overlapping reg regions across 2 device nodes.
> >>  - Simplify 2 device nodes as 1 because both are from same IP block.
> >>  - Adapt the driver code according to the new simplified devicetree.
> >>
> >> Vijayakannan Ayyathurai (2):
> >>   dt-bindings: timer: Add bindings for Intel Keem Bay SoC timer
> >>   clocksource: Add Intel Keem Bay Timer Support
> >
> > Kindly help us to review this updated patch(v2) set.
> 
> Review in progress ... :)
> 

Thank you for the Review. 

Thanks,
Vijay