[PATCH v8 0/2] Add the driver for Intel Keem Bay SoC timer block

[PATCH v8 0/2] Add the driver for Intel Keem Bay SoC timer block

[shruthi.sanil]

From: Shruthi Sanil <shruthi.sanil@intel.com>

The timer block supports 1 64-bit free running counter
and 8 32-bit general purpose timers.

Patch 1 holds the device tree binding documentation.
Patch 2 holds the device driver.

This driver is tested on the Keem Bay evaluation module board.

Changes since v7:
- Added back the compatible string "intel,keembay-gpt-creg"
  as an enum to the mfd device node in the device tree bindings.
- As the timer is used as a broadcast timer during CPU idle,
  only one timer is needed. Hence updated the driver accordingly
  incorporating the review comments.

Changes since v6:
- Removed the unused compatible string from the mfd device node
  to fix the error thrown by the make dt-binding command.

Changes since v5:
- Created a MFD device for the common configuration register
  in the device tree bindings.
- Updated the timer driver with the MFD framework to access the
  common configuration register.

Changes since v4:
- Updated the description in the device tree bindings.
- Updated the unit address of all the timers and counter
  in the device tree binding.

Changes since v3:
- Update in KConfig file to support COMPILE_TEST for Keem Bay timer.
- Update in device tree bindings to remove status field.
- Update in device tree bindings to remove 64-bit address space for
  the child nodes by using non-empty ranges.

Changes since v2:
- Add multi timer support.
- Update in the device tree binding to support multi timers.
- Code optimization.

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.

Shruthi Sanil (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   | 128 ++++++++++
 MAINTAINERS                                   |   6 +
 drivers/clocksource/Kconfig                   |  11 +
 drivers/clocksource/Makefile                  |   1 +
 drivers/clocksource/timer-keembay.c           | 230 ++++++++++++++++++
 5 files changed, 376 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/intel,keembay-timer.yaml
 create mode 100644 drivers/clocksource/timer-keembay.c


base-commit: cfb92440ee71adcc2105b0890bb01ac3cddb8507
-- 
2.17.1

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

[shruthi.sanil]

From: Shruthi Sanil <shruthi.sanil@intel.com>

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

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Shruthi Sanil <shruthi.sanil@intel.com>
---
 .../bindings/timer/intel,keembay-timer.yaml   | 128 ++++++++++++++++++
 1 file changed, 128 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..9e6d46ecc2dc
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/intel,keembay-timer.yaml
@@ -0,0 +1,128 @@
+# 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:
+  - Shruthi Sanil <shruthi.sanil@intel.com>
+
+description: |
+  The Intel Keem Bay timer driver supports 1 free running counter and 8 timers.
+  Each timer is capable of generating inividual interrupt.
+  Both the features are enabled through the timer general config register.
+
+  The parent node represents the common general configuration details and
+  the child nodes represents the counter and timers.
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - intel,keembay-gpt-creg
+          - const: simple-mfd
+
+  reg:
+    description: General configuration register address and length.
+    maxItems: 1
+
+  ranges: true
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - ranges
+  - "#address-cells"
+  - "#size-cells"
+
+patternProperties:
+  "^counter@[0-9a-f]+$":
+    description: Properties for Intel Keem Bay counter.
+    type: object
+    properties:
+      compatible:
+        oneOf:
+          - items:
+              - enum:
+                  - intel,keembay-counter
+
+      reg:
+        maxItems: 1
+
+      clocks:
+        maxItems: 1
+
+    required:
+      - compatible
+      - reg
+      - clocks
+
+  "^timer@[0-9a-f]+$":
+    description: Properties for Intel Keem Bay timer
+    type: object
+    properties:
+      compatible:
+        oneOf:
+          - items:
+              - enum:
+                  - intel,keembay-timer
+
+      reg:
+        maxItems: 1
+
+      interrupts:
+        maxItems: 1
+
+      clocks:
+        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
+
+    soc {
+        #address-cells = <0x2>;
+        #size-cells = <0x2>;
+
+        gpt@20331000 {
+            compatible = "intel,keembay-gpt-creg", "simple-mfd";
+            reg = <0x0 0x20331000 0x0 0xc>;
+            ranges = <0x0 0x0 0x20330000 0xF0>;
+            #address-cells = <0x1>;
+            #size-cells = <0x1>;
+
+            counter@e8 {
+                compatible = "intel,keembay-counter";
+                reg = <0xe8 0x8>;
+                clocks = <&scmi_clk KEEM_BAY_A53_TIM>;
+            };
+
+            timer@30 {
+                compatible = "intel,keembay-timer";
+                reg = <0x30 0xc>;
+                interrupts = <GIC_SPI 0x5 IRQ_TYPE_LEVEL_HIGH>;
+                clocks = <&scmi_clk KEEM_BAY_A53_TIM>;
+            };
+        };
+    };
+
+...
-- 
2.17.1

[PATCH v8 2/2] clocksource: Add Intel Keem Bay timer support

[shruthi.sanil]

From: Shruthi Sanil <shruthi.sanil@intel.com>

The Intel Keem Bay timer driver supports clocksource and clockevent
features for the timer IP used in Intel Keem Bay SoC.
The timer block supports 1 free running counter and 8 timers.
The free running counter can be used as a clocksource and
the timers can be used as clockevent. Each timer is capable of
generating individual interrupt.
Both the features are enabled through the timer general config register.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Shruthi Sanil <shruthi.sanil@intel.com>
---
 MAINTAINERS                         |   6 +
 drivers/clocksource/Kconfig         |  11 ++
 drivers/clocksource/Makefile        |   1 +
 drivers/clocksource/timer-keembay.c | 230 ++++++++++++++++++++++++++++
 4 files changed, 248 insertions(+)
 create mode 100644 drivers/clocksource/timer-keembay.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 777cd6fa2b3d..73c0029dcdf7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9796,6 +9796,12 @@ F:	drivers/crypto/keembay/keembay-ocs-hcu-core.c
 F:	drivers/crypto/keembay/ocs-hcu.c
 F:	drivers/crypto/keembay/ocs-hcu.h
 
+INTEL KEEM BAY TIMER DRIVER
+M:	Shruthi Sanil <shruthi.sanil@intel.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/timer/intel,keembay-timer.yaml
+F:	drivers/clocksource/timer-keembay.c
+
 INTEL THUNDER BAY EMMC PHY DRIVER
 M:	Nandhini Srikandan <nandhini.srikandan@intel.com>
 M:	Rashmi A <rashmi.a@intel.com>
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index cfb8ea0df3b1..65b6cf916e5a 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -721,4 +721,15 @@ config MICROCHIP_PIT64B
 	  modes and high resolution. It is used as a clocksource
 	  and a clockevent.
 
+config KEEMBAY_TIMER
+	bool "Intel Keem Bay timer"
+	depends on ARCH_KEEMBAY || COMPILE_TEST
+	select TIMER_OF
+	help
+	  This option enables the support for the Intel Keem Bay
+	  general purpose timer and free running counter driver.
+	  Each timer can generate an individual interrupt and
+	  supports oneshot and periodic modes.
+	  The 64-bit counter can be used as a clock source.
+
 endmenu
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index fa5f624eadb6..dff6458ef9e5 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -89,3 +89,4 @@ obj-$(CONFIG_GX6605S_TIMER)		+= timer-gx6605s.o
 obj-$(CONFIG_HYPERV_TIMER)		+= hyperv_timer.o
 obj-$(CONFIG_MICROCHIP_PIT64B)		+= timer-microchip-pit64b.o
 obj-$(CONFIG_MSC313E_TIMER)		+= timer-msc313e.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..230609c06a26
--- /dev/null
+++ b/drivers/clocksource/timer-keembay.c
@@ -0,0 +1,230 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Intel Keem Bay Timer driver
+ *
+ * Copyright (C) 2020 Intel Corporation
+ */
+
+#include <linux/bitops.h>
+#include <linux/interrupt.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/sizes.h>
+#include <linux/slab.h>
+#include <linux/regmap.h>
+
+#include "timer-of.h"
+
+/* Timer register offset */
+#define TIM_CNT_VAL_OFFSET		0x0
+#define TIM_RELOAD_VAL_OFFSET		0x4
+#define TIM_CONFIG_OFFSET		0x8
+
+/* Bit fields of timer general config register */
+#define TIM_CONFIG_PRESCALER_ENABLE	BIT(2)
+#define TIM_CONFIG_COUNTER_ENABLE	BIT(0)
+
+/* Bit fields of timer config register */
+#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_GEN_MASK			GENMASK(31, 12)
+#define TIM_RATING			200
+#define TIM_CLKSRC_MASK_BITS		64
+
+#define TIMER_NAME_SIZE			25
+
+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_disable(void __iomem *base)
+{
+	writel(0x0, 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 inline void keembay_timer_clear_pending_int(void __iomem *base)
+{
+	u32 val;
+
+	val = readl(base + TIM_CONFIG_OFFSET);
+	val &= ~TIM_CONFIG_INTERRUPT_PENDING;
+	writel(val, base + TIM_CONFIG_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);
+	void __iomem *tim_base = timer_of_base(to);
+
+	keembay_timer_disable(tim_base);
+	keembay_timer_update_counter(tim_base, evt);
+	keembay_timer_enable(tim_base, 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);
+	void __iomem *tim_base = timer_of_base(to);
+
+	keembay_timer_disable(tim_base);
+	keembay_timer_update_counter(tim_base, timer_of_period(to));
+	keembay_timer_enable(tim_base, 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);
+	void __iomem *tim_base = timer_of_base(to);
+	u32 val;
+
+	val = readl(tim_base + TIM_CONFIG_OFFSET);
+
+	if (val & TIM_CONFIG_RESTART) {
+		/* Clear interrupt for periodic timer*/
+		keembay_timer_clear_pending_int(tim_base);
+	} else {
+		/* Disable the timer for one shot timer */
+		keembay_timer_disable(tim_base);
+	}
+
+	evt->event_handler(evt);
+
+	return IRQ_HANDLED;
+}
+
+static int __init keembay_clockevent_init(struct device_node *np)
+{
+	struct timer_of *keembay_ce_to;
+	struct regmap *regmap;
+	int ret;
+	u32 val;
+
+	regmap = device_node_to_regmap(np->parent);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	ret = regmap_read(regmap, TIM_CONFIG_OFFSET, &val);
+	if (ret)
+		return ret;
+
+	/* Prescaler bit must be enabled for the timer to function */
+	if (!(val & TIM_CONFIG_PRESCALER_ENABLE)) {
+		pr_err("%pOF: Prescaler is not enabled\n", np);
+		ret = -ENODEV;
+	}
+
+	keembay_ce_to = kzalloc(sizeof(*keembay_ce_to), GFP_KERNEL);
+	if (!keembay_ce_to)
+		ret = -ENOMEM;
+
+	keembay_ce_to->flags = TIMER_OF_IRQ | TIMER_OF_BASE | TIMER_OF_CLOCK;
+	keembay_ce_to->clkevt.name = "keembay_timer";
+	keembay_ce_to->clkevt.cpumask = cpu_possible_mask;
+	keembay_ce_to->clkevt.features = CLOCK_EVT_FEAT_PERIODIC |
+					 CLOCK_EVT_FEAT_ONESHOT  |
+					 CLOCK_EVT_FEAT_DYNIRQ;
+	keembay_ce_to->clkevt.rating = TIM_RATING;
+	keembay_ce_to->clkevt.set_next_event = keembay_timer_set_next_event;
+	keembay_ce_to->clkevt.set_state_periodic = keembay_timer_periodic;
+	keembay_ce_to->clkevt.set_state_shutdown = keembay_timer_shutdown;
+	keembay_ce_to->of_irq.handler = keembay_timer_isr;
+	keembay_ce_to->of_irq.flags = IRQF_TIMER;
+
+	ret = timer_of_init(np, keembay_ce_to);
+	if (ret)
+		goto err_keembay_ce_to_free;
+
+	ret = regmap_read(regmap, TIM_RELOAD_VAL_OFFSET, &val);
+	if (ret)
+		goto err_keembay_ce_to_free;
+
+	keembay_ce_to->of_clk.rate = keembay_ce_to->of_clk.rate / (val + 1);
+
+	clockevents_config_and_register(&keembay_ce_to->clkevt,
+					timer_of_rate(keembay_ce_to),
+					1,
+					U32_MAX);
+
+	return 0;
+
+err_keembay_ce_to_free:
+	kfree(keembay_ce_to);
+
+	return ret;
+}
+
+static struct timer_of keembay_cs_to = {
+	.flags	= TIMER_OF_BASE | TIMER_OF_CLOCK,
+};
+
+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_MASK_BITS),
+	.flags	= CLOCK_SOURCE_IS_CONTINUOUS |
+		  CLOCK_SOURCE_SUSPEND_NONSTOP,
+};
+
+static int __init keembay_clocksource_init(struct device_node *np)
+{
+	struct regmap *regmap;
+	u32 val;
+	int ret;
+
+	regmap = device_node_to_regmap(np->parent);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	ret = regmap_read(regmap, TIM_CONFIG_OFFSET, &val);
+	if (ret)
+		return ret;
+
+	/* Free Running Counter bit must be enabled for counter to function */
+	if (!(val & TIM_CONFIG_COUNTER_ENABLE)) {
+		pr_err("%pOF: free running counter is not enabled\n", np);
+		return -ENODEV;
+	}
+
+	ret = timer_of_init(np, &keembay_cs_to);
+	if (ret)
+		return ret;
+
+	return clocksource_register_hz(&keembay_counter, timer_of_rate(&keembay_cs_to));
+}
+
+TIMER_OF_DECLARE(keembay_clockevent, "intel,keembay-timer", keembay_clockevent_init);
+TIMER_OF_DECLARE(keembay_clocksource, "intel,keembay-counter", keembay_clocksource_init);
-- 
2.17.1

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

[Rob Herring]

On Tue, Feb 22, 2022 at 03:26:53PM +0530, shruthi.sanil@intel.com wrote:
> From: Shruthi Sanil <shruthi.sanil@intel.com>
> 
> Add Device Tree bindings for the Timer IP, which can be used as
> clocksource and clockevent device in the Intel Keem Bay SoC.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Shruthi Sanil <shruthi.sanil@intel.com>
> ---
>  .../bindings/timer/intel,keembay-timer.yaml   | 128 ++++++++++++++++++
>  1 file changed, 128 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..9e6d46ecc2dc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/intel,keembay-timer.yaml
> @@ -0,0 +1,128 @@
> +# 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:
> +  - Shruthi Sanil <shruthi.sanil@intel.com>
> +
> +description: |
> +  The Intel Keem Bay timer driver supports 1 free running counter and 8 timers.

Bindings describe the h/w, not what drivers support.

> +  Each timer is capable of generating inividual interrupt.
> +  Both the features are enabled through the timer general config register.
> +
> +  The parent node represents the common general configuration details and
> +  the child nodes represents the counter and timers.
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:

Don't need oneOf with only 1 entry.

> +          - enum:
> +              - intel,keembay-gpt-creg
> +          - const: simple-mfd
> +
> +  reg:
> +    description: General configuration register address and length.
> +    maxItems: 1
> +
> +  ranges: true
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - ranges
> +  - "#address-cells"
> +  - "#size-cells"
> +
> +patternProperties:
> +  "^counter@[0-9a-f]+$":
> +    description: Properties for Intel Keem Bay counter.
> +    type: object
> +    properties:
> +      compatible:
> +        oneOf:

Don't need oneOf.

> +          - items:
> +              - enum:
> +                  - intel,keembay-counter
> +
> +      reg:
> +        maxItems: 1
> +
> +      clocks:
> +        maxItems: 1
> +
> +    required:
> +      - compatible
> +      - reg
> +      - clocks
> +
> +  "^timer@[0-9a-f]+$":
> +    description: Properties for Intel Keem Bay timer
> +    type: object
> +    properties:
> +      compatible:
> +        oneOf:
> +          - items:
> +              - enum:
> +                  - intel,keembay-timer
> +
> +      reg:
> +        maxItems: 1
> +
> +      interrupts:
> +        maxItems: 1
> +
> +      clocks:
> +        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
> +
> +    soc {
> +        #address-cells = <0x2>;
> +        #size-cells = <0x2>;
> +
> +        gpt@20331000 {
> +            compatible = "intel,keembay-gpt-creg", "simple-mfd";

It looks like you are splitting things based on Linux implementation 
details. Does this h/w block have different combinations of timers and 
counters? If not, then you don't need the child nodes at all. There's 
plenty of h/w blocks that get used as both a clocksource and clockevent.

Maybe I already raised this, but assume I don't remember and this patch 
needs to address any questions I already asked.

> +            reg = <0x0 0x20331000 0x0 0xc>;
> +            ranges = <0x0 0x0 0x20330000 0xF0>;
> +            #address-cells = <0x1>;
> +            #size-cells = <0x1>;
> +
> +            counter@e8 {
> +                compatible = "intel,keembay-counter";
> +                reg = <0xe8 0x8>;
> +                clocks = <&scmi_clk KEEM_BAY_A53_TIM>;
> +            };
> +
> +            timer@30 {
> +                compatible = "intel,keembay-timer";
> +                reg = <0x30 0xc>;
> +                interrupts = <GIC_SPI 0x5 IRQ_TYPE_LEVEL_HIGH>;
> +                clocks = <&scmi_clk KEEM_BAY_A53_TIM>;
> +            };
> +        };
> +    };
> +
> +...
> -- 
> 2.17.1
> 
> 

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

[Sanil, Shruthi]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Wednesday, February 23, 2022 4:44 AM
> To: Sanil, Shruthi <shruthi.sanil@intel.com>
> Cc: daniel.lezcano@linaro.org; tglx@linutronix.de; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org;
> andriy.shevchenko@linux.intel.com; mgross@linux.intel.com; Thokala,
> Srikanth <srikanth.thokala@intel.com>; Raja Subramanian, Lakshmi Bai
> <lakshmi.bai.raja.subramanian@intel.com>; Sangannavar, Mallikarjunappa
> <mallikarjunappa.sangannavar@intel.com>
> Subject: Re: [PATCH v8 1/2] dt-bindings: timer: Add bindings for Intel Keem
> Bay SoC Timer
> 
> On Tue, Feb 22, 2022 at 03:26:53PM +0530, shruthi.sanil@intel.com wrote:
> > From: Shruthi Sanil <shruthi.sanil@intel.com>
> >
> > Add Device Tree bindings for the Timer IP, which can be used as
> > clocksource and clockevent device in the Intel Keem Bay SoC.
> >
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> > Signed-off-by: Shruthi Sanil <shruthi.sanil@intel.com>
> > ---
> >  .../bindings/timer/intel,keembay-timer.yaml   | 128 ++++++++++++++++++
> >  1 file changed, 128 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..9e6d46ecc2dc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/timer/intel,keembay-
> timer.yaml
> > @@ -0,0 +1,128 @@
> > +# 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:
> > +  - Shruthi Sanil <shruthi.sanil@intel.com>
> > +
> > +description: |
> > +  The Intel Keem Bay timer driver supports 1 free running counter and 8
> timers.
> 
> Bindings describe the h/w, not what drivers support.

Sorry for adding the word driver here.
Actually the Keem Bay timer IP has 1 free running counter and 8 timers.
I'll correct the description explaining clearly that the above are the H/W details.

> 
> > +  Each timer is capable of generating inividual interrupt.
> > +  Both the features are enabled through the timer general config register.
> > +
> > +  The parent node represents the common general configuration details
> > + and  the child nodes represents the counter and timers.
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - items:
> 
> Don't need oneOf with only 1 entry.

OK, I'll correct it in my next patch.

> 
> > +          - enum:
> > +              - intel,keembay-gpt-creg
> > +          - const: simple-mfd
> > +
> > +  reg:
> > +    description: General configuration register address and length.
> > +    maxItems: 1
> > +
> > +  ranges: true
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - ranges
> > +  - "#address-cells"
> > +  - "#size-cells"
> > +
> > +patternProperties:
> > +  "^counter@[0-9a-f]+$":
> > +    description: Properties for Intel Keem Bay counter.
> > +    type: object
> > +    properties:
> > +      compatible:
> > +        oneOf:
> 
> Don't need oneOf.

OK, I'll correct it in my next patch.

> 
> > +          - items:
> > +              - enum:
> > +                  - intel,keembay-counter
> > +
> > +      reg:
> > +        maxItems: 1
> > +
> > +      clocks:
> > +        maxItems: 1
> > +
> > +    required:
> > +      - compatible
> > +      - reg
> > +      - clocks
> > +
> > +  "^timer@[0-9a-f]+$":
> > +    description: Properties for Intel Keem Bay timer
> > +    type: object
> > +    properties:
> > +      compatible:
> > +        oneOf:
> > +          - items:
> > +              - enum:
> > +                  - intel,keembay-timer
> > +
> > +      reg:
> > +        maxItems: 1
> > +
> > +      interrupts:
> > +        maxItems: 1
> > +
> > +      clocks:
> > +        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
> > +
> > +    soc {
> > +        #address-cells = <0x2>;
> > +        #size-cells = <0x2>;
> > +
> > +        gpt@20331000 {
> > +            compatible = "intel,keembay-gpt-creg", "simple-mfd";
> 
> It looks like you are splitting things based on Linux implementation details.
> Does this h/w block have different combinations of timers and counters? If
> not, then you don't need the child nodes at all. There's plenty of h/w blocks
> that get used as both a clocksource and clockevent.
> 

Yes, the Timer H/W block has 1 free running counter and 8 timers.
These timers and counter are enabled using a common configuration register.
Hence we have a parent node which has the details of this common configuration register
And the child nodes with their register and interrupt details.

> Maybe I already raised this, but assume I don't remember and this patch
> needs to address any questions I already asked.

All the review comments given by you in the series of this patch are addressed.

In the example below, I have just added the details of one timer.
But in actual we have a total of 8 timers.
I can update all the 8 if that's required to be updated.

> 
> > +            reg = <0x0 0x20331000 0x0 0xc>;
> > +            ranges = <0x0 0x0 0x20330000 0xF0>;
> > +            #address-cells = <0x1>;
> > +            #size-cells = <0x1>;
> > +
> > +            counter@e8 {
> > +                compatible = "intel,keembay-counter";
> > +                reg = <0xe8 0x8>;
> > +                clocks = <&scmi_clk KEEM_BAY_A53_TIM>;
> > +            };
> > +
> > +            timer@30 {
> > +                compatible = "intel,keembay-timer";
> > +                reg = <0x30 0xc>;
> > +                interrupts = <GIC_SPI 0x5 IRQ_TYPE_LEVEL_HIGH>;
> > +                clocks = <&scmi_clk KEEM_BAY_A53_TIM>;
> > +            };
> > +        };
> > +    };
> > +
> > +...
> > --
> > 2.17.1
> >
> >

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

[Andy Shevchenko]

On Tue, Feb 22, 2022 at 05:13:41PM -0600, Rob Herring wrote:
> On Tue, Feb 22, 2022 at 03:26:53PM +0530, shruthi.sanil@intel.com wrote:
> > From: Shruthi Sanil <shruthi.sanil@intel.com>
> > 
> > Add Device Tree bindings for the Timer IP, which can be used as
> > clocksource and clockevent device in the Intel Keem Bay SoC.

...

> > +    soc {
> > +        #address-cells = <0x2>;
> > +        #size-cells = <0x2>;
> > +
> > +        gpt@20331000 {
> > +            compatible = "intel,keembay-gpt-creg", "simple-mfd";
> 
> It looks like you are splitting things based on Linux implementation
> details. Does this h/w block have different combinations of timers and
> counters? If not, then you don't need the child nodes at all. There's
> plenty of h/w blocks that get used as both a clocksource and clockevent.
> 
> Maybe I already raised this, but assume I don't remember and this patch
> needs to address any questions I already asked.

I dunno if I mentioned that hardware seems to have 5 or so devices behind
the block, so ideally it should be one device node that represents the global
register spaces and several children nodes.

However, I am not familiar with the established practices in DT world, but
above seems to me the right thing to do since it describes the hardware as
is (without any linuxisms).

> > +            reg = <0x0 0x20331000 0x0 0xc>;
> > +            ranges = <0x0 0x0 0x20330000 0xF0>;
> > +            #address-cells = <0x1>;
> > +            #size-cells = <0x1>;
> > +
> > +            counter@e8 {
> > +                compatible = "intel,keembay-counter";
> > +                reg = <0xe8 0x8>;
> > +                clocks = <&scmi_clk KEEM_BAY_A53_TIM>;
> > +            };
> > +
> > +            timer@30 {
> > +                compatible = "intel,keembay-timer";
> > +                reg = <0x30 0xc>;
> > +                interrupts = <GIC_SPI 0x5 IRQ_TYPE_LEVEL_HIGH>;
> > +                clocks = <&scmi_clk KEEM_BAY_A53_TIM>;
> > +            };
> > +        };
> > +    };

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v8 2/2] clocksource: Add Intel Keem Bay timer support

[Sanil, Shruthi]

Hello Daniel and Thomas,

Could you please help review this patch?


Regards,
Shruthi

> -----Original Message-----
> From: Sanil, Shruthi <shruthi.sanil@intel.com>
> Sent: Tuesday, February 22, 2022 3:27 PM
> To: daniel.lezcano@linaro.org; tglx@linutronix.de; robh+dt@kernel.org;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org
> Cc: andriy.shevchenko@linux.intel.com; mgross@linux.intel.com; Thokala,
> Srikanth <srikanth.thokala@intel.com>; Raja Subramanian, Lakshmi Bai
> <lakshmi.bai.raja.subramanian@intel.com>; Sangannavar, Mallikarjunappa
> <mallikarjunappa.sangannavar@intel.com>; Sanil, Shruthi
> <shruthi.sanil@intel.com>
> Subject: [PATCH v8 2/2] clocksource: Add Intel Keem Bay timer support
> 
> From: Shruthi Sanil <shruthi.sanil@intel.com>
> 
> The Intel Keem Bay timer driver supports clocksource and clockevent
> features for the timer IP used in Intel Keem Bay SoC.
> The timer block supports 1 free running counter and 8 timers.
> The free running counter can be used as a clocksource and the timers can be
> used as clockevent. Each timer is capable of generating individual interrupt.
> Both the features are enabled through the timer general config register.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Shruthi Sanil <shruthi.sanil@intel.com>
> ---
>  MAINTAINERS                         |   6 +
>  drivers/clocksource/Kconfig         |  11 ++
>  drivers/clocksource/Makefile        |   1 +
>  drivers/clocksource/timer-keembay.c | 230
> ++++++++++++++++++++++++++++
>  4 files changed, 248 insertions(+)
>  create mode 100644 drivers/clocksource/timer-keembay.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 777cd6fa2b3d..73c0029dcdf7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9796,6 +9796,12 @@ F:	drivers/crypto/keembay/keembay-ocs-hcu-
> core.c
>  F:	drivers/crypto/keembay/ocs-hcu.c
>  F:	drivers/crypto/keembay/ocs-hcu.h
> 
> +INTEL KEEM BAY TIMER DRIVER
> +M:	Shruthi Sanil <shruthi.sanil@intel.com>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/timer/intel,keembay-
> timer.yaml
> +F:	drivers/clocksource/timer-keembay.c
> +
>  INTEL THUNDER BAY EMMC PHY DRIVER
>  M:	Nandhini Srikandan <nandhini.srikandan@intel.com>
>  M:	Rashmi A <rashmi.a@intel.com>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig index
> cfb8ea0df3b1..65b6cf916e5a 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -721,4 +721,15 @@ config MICROCHIP_PIT64B
>  	  modes and high resolution. It is used as a clocksource
>  	  and a clockevent.
> 
> +config KEEMBAY_TIMER
> +	bool "Intel Keem Bay timer"
> +	depends on ARCH_KEEMBAY || COMPILE_TEST
> +	select TIMER_OF
> +	help
> +	  This option enables the support for the Intel Keem Bay
> +	  general purpose timer and free running counter driver.
> +	  Each timer can generate an individual interrupt and
> +	  supports oneshot and periodic modes.
> +	  The 64-bit counter can be used as a clock source.
> +
>  endmenu
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index fa5f624eadb6..dff6458ef9e5 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -89,3 +89,4 @@ obj-$(CONFIG_GX6605S_TIMER)		+= timer-
> gx6605s.o
>  obj-$(CONFIG_HYPERV_TIMER)		+= hyperv_timer.o
>  obj-$(CONFIG_MICROCHIP_PIT64B)		+= timer-microchip-pit64b.o
>  obj-$(CONFIG_MSC313E_TIMER)		+= timer-msc313e.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..230609c06a26
> --- /dev/null
> +++ b/drivers/clocksource/timer-keembay.c
> @@ -0,0 +1,230 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Intel Keem Bay Timer driver
> + *
> + * Copyright (C) 2020 Intel Corporation  */
> +
> +#include <linux/bitops.h>
> +#include <linux/interrupt.h>
> +#include <linux/io-64-nonatomic-lo-hi.h> #include <linux/mfd/syscon.h>
> +#include <linux/module.h> #include <linux/of_address.h> #include
> +<linux/sizes.h> #include <linux/slab.h> #include <linux/regmap.h>
> +
> +#include "timer-of.h"
> +
> +/* Timer register offset */
> +#define TIM_CNT_VAL_OFFSET		0x0
> +#define TIM_RELOAD_VAL_OFFSET		0x4
> +#define TIM_CONFIG_OFFSET		0x8
> +
> +/* Bit fields of timer general config register */
> +#define TIM_CONFIG_PRESCALER_ENABLE	BIT(2)
> +#define TIM_CONFIG_COUNTER_ENABLE	BIT(0)
> +
> +/* Bit fields of timer config register */
> +#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_GEN_MASK			GENMASK(31, 12)
> +#define TIM_RATING			200
> +#define TIM_CLKSRC_MASK_BITS		64
> +
> +#define TIMER_NAME_SIZE			25
> +
> +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_disable(void __iomem *base) {
> +	writel(0x0, 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 inline void keembay_timer_clear_pending_int(void __iomem *base)
> +{
> +	u32 val;
> +
> +	val = readl(base + TIM_CONFIG_OFFSET);
> +	val &= ~TIM_CONFIG_INTERRUPT_PENDING;
> +	writel(val, base + TIM_CONFIG_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);
> +	void __iomem *tim_base = timer_of_base(to);
> +
> +	keembay_timer_disable(tim_base);
> +	keembay_timer_update_counter(tim_base, evt);
> +	keembay_timer_enable(tim_base, 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);
> +	void __iomem *tim_base = timer_of_base(to);
> +
> +	keembay_timer_disable(tim_base);
> +	keembay_timer_update_counter(tim_base, timer_of_period(to));
> +	keembay_timer_enable(tim_base, 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);
> +	void __iomem *tim_base = timer_of_base(to);
> +	u32 val;
> +
> +	val = readl(tim_base + TIM_CONFIG_OFFSET);
> +
> +	if (val & TIM_CONFIG_RESTART) {
> +		/* Clear interrupt for periodic timer*/
> +		keembay_timer_clear_pending_int(tim_base);
> +	} else {
> +		/* Disable the timer for one shot timer */
> +		keembay_timer_disable(tim_base);
> +	}
> +
> +	evt->event_handler(evt);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int __init keembay_clockevent_init(struct device_node *np) {
> +	struct timer_of *keembay_ce_to;
> +	struct regmap *regmap;
> +	int ret;
> +	u32 val;
> +
> +	regmap = device_node_to_regmap(np->parent);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	ret = regmap_read(regmap, TIM_CONFIG_OFFSET, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* Prescaler bit must be enabled for the timer to function */
> +	if (!(val & TIM_CONFIG_PRESCALER_ENABLE)) {
> +		pr_err("%pOF: Prescaler is not enabled\n", np);
> +		ret = -ENODEV;
> +	}
> +
> +	keembay_ce_to = kzalloc(sizeof(*keembay_ce_to), GFP_KERNEL);
> +	if (!keembay_ce_to)
> +		ret = -ENOMEM;
> +
> +	keembay_ce_to->flags = TIMER_OF_IRQ | TIMER_OF_BASE |
> TIMER_OF_CLOCK;
> +	keembay_ce_to->clkevt.name = "keembay_timer";
> +	keembay_ce_to->clkevt.cpumask = cpu_possible_mask;
> +	keembay_ce_to->clkevt.features = CLOCK_EVT_FEAT_PERIODIC |
> +					 CLOCK_EVT_FEAT_ONESHOT  |
> +					 CLOCK_EVT_FEAT_DYNIRQ;
> +	keembay_ce_to->clkevt.rating = TIM_RATING;
> +	keembay_ce_to->clkevt.set_next_event =
> keembay_timer_set_next_event;
> +	keembay_ce_to->clkevt.set_state_periodic =
> keembay_timer_periodic;
> +	keembay_ce_to->clkevt.set_state_shutdown =
> keembay_timer_shutdown;
> +	keembay_ce_to->of_irq.handler = keembay_timer_isr;
> +	keembay_ce_to->of_irq.flags = IRQF_TIMER;
> +
> +	ret = timer_of_init(np, keembay_ce_to);
> +	if (ret)
> +		goto err_keembay_ce_to_free;
> +
> +	ret = regmap_read(regmap, TIM_RELOAD_VAL_OFFSET, &val);
> +	if (ret)
> +		goto err_keembay_ce_to_free;
> +
> +	keembay_ce_to->of_clk.rate = keembay_ce_to->of_clk.rate / (val +
> 1);
> +
> +	clockevents_config_and_register(&keembay_ce_to->clkevt,
> +					timer_of_rate(keembay_ce_to),
> +					1,
> +					U32_MAX);
> +
> +	return 0;
> +
> +err_keembay_ce_to_free:
> +	kfree(keembay_ce_to);
> +
> +	return ret;
> +}
> +
> +static struct timer_of keembay_cs_to = {
> +	.flags	= TIMER_OF_BASE | TIMER_OF_CLOCK,
> +};
> +
> +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_MASK_BITS),
> +	.flags	= CLOCK_SOURCE_IS_CONTINUOUS |
> +		  CLOCK_SOURCE_SUSPEND_NONSTOP,
> +};
> +
> +static int __init keembay_clocksource_init(struct device_node *np) {
> +	struct regmap *regmap;
> +	u32 val;
> +	int ret;
> +
> +	regmap = device_node_to_regmap(np->parent);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	ret = regmap_read(regmap, TIM_CONFIG_OFFSET, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* Free Running Counter bit must be enabled for counter to function
> */
> +	if (!(val & TIM_CONFIG_COUNTER_ENABLE)) {
> +		pr_err("%pOF: free running counter is not enabled\n", np);
> +		return -ENODEV;
> +	}
> +
> +	ret = timer_of_init(np, &keembay_cs_to);
> +	if (ret)
> +		return ret;
> +
> +	return clocksource_register_hz(&keembay_counter,
> +timer_of_rate(&keembay_cs_to)); }
> +
> +TIMER_OF_DECLARE(keembay_clockevent, "intel,keembay-timer",
> +keembay_clockevent_init); TIMER_OF_DECLARE(keembay_clocksource,
> +"intel,keembay-counter", keembay_clocksource_init);
> --
> 2.17.1

Re: [PATCH v8 2/2] clocksource: Add Intel Keem Bay timer support

[Daniel Lezcano]

On 22/02/2022 10:56, shruthi.sanil@intel.com wrote:
> From: Shruthi Sanil <shruthi.sanil@intel.com>
> 
> The Intel Keem Bay timer driver supports clocksource and clockevent
> features for the timer IP used in Intel Keem Bay SoC.
> The timer block supports 1 free running counter and 8 timers.
> The free running counter can be used as a clocksource and
> the timers can be used as clockevent. Each timer is capable of
> generating individual interrupt.
> Both the features are enabled through the timer general config register.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Shruthi Sanil <shruthi.sanil@intel.com>
> ---
>   MAINTAINERS                         |   6 +
>   drivers/clocksource/Kconfig         |  11 ++
>   drivers/clocksource/Makefile        |   1 +
>   drivers/clocksource/timer-keembay.c | 230 ++++++++++++++++++++++++++++
>   4 files changed, 248 insertions(+)
>   create mode 100644 drivers/clocksource/timer-keembay.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 777cd6fa2b3d..73c0029dcdf7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9796,6 +9796,12 @@ F:	drivers/crypto/keembay/keembay-ocs-hcu-core.c
>   F:	drivers/crypto/keembay/ocs-hcu.c
>   F:	drivers/crypto/keembay/ocs-hcu.h
>   
> +INTEL KEEM BAY TIMER DRIVER
> +M:	Shruthi Sanil <shruthi.sanil@intel.com>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/timer/intel,keembay-timer.yaml
> +F:	drivers/clocksource/timer-keembay.c
> +
>   INTEL THUNDER BAY EMMC PHY DRIVER
>   M:	Nandhini Srikandan <nandhini.srikandan@intel.com>
>   M:	Rashmi A <rashmi.a@intel.com>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index cfb8ea0df3b1..65b6cf916e5a 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -721,4 +721,15 @@ config MICROCHIP_PIT64B
>   	  modes and high resolution. It is used as a clocksource
>   	  and a clockevent.
>   
> +config KEEMBAY_TIMER
> +	bool "Intel Keem Bay timer"
> +	depends on ARCH_KEEMBAY || COMPILE_TEST
> +	select TIMER_OF
> +	help
> +	  This option enables the support for the Intel Keem Bay
> +	  general purpose timer and free running counter driver.
> +	  Each timer can generate an individual interrupt and
> +	  supports oneshot and periodic modes.
> +	  The 64-bit counter can be used as a clock source.
> +
>   endmenu
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index fa5f624eadb6..dff6458ef9e5 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -89,3 +89,4 @@ obj-$(CONFIG_GX6605S_TIMER)		+= timer-gx6605s.o
>   obj-$(CONFIG_HYPERV_TIMER)		+= hyperv_timer.o
>   obj-$(CONFIG_MICROCHIP_PIT64B)		+= timer-microchip-pit64b.o
>   obj-$(CONFIG_MSC313E_TIMER)		+= timer-msc313e.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..230609c06a26
> --- /dev/null
> +++ b/drivers/clocksource/timer-keembay.c
> @@ -0,0 +1,230 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Intel Keem Bay Timer driver
> + *
> + * Copyright (C) 2020 Intel Corporation
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/interrupt.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/sizes.h>
> +#include <linux/slab.h>
> +#include <linux/regmap.h>
> +
> +#include "timer-of.h"
> +
> +/* Timer register offset */
> +#define TIM_CNT_VAL_OFFSET		0x0
> +#define TIM_RELOAD_VAL_OFFSET		0x4
> +#define TIM_CONFIG_OFFSET		0x8
> +
> +/* Bit fields of timer general config register */
> +#define TIM_CONFIG_PRESCALER_ENABLE	BIT(2)
> +#define TIM_CONFIG_COUNTER_ENABLE	BIT(0)
> +
> +/* Bit fields of timer config register */
> +#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_GEN_MASK			GENMASK(31, 12)
> +#define TIM_RATING			200
> +#define TIM_CLKSRC_MASK_BITS		64
> +
> +#define TIMER_NAME_SIZE			25
> +
> +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_disable(void __iomem *base)
> +{
> +	writel(0x0, 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 inline void keembay_timer_clear_pending_int(void __iomem *base)
> +{
> +	u32 val;
> +
> +	val = readl(base + TIM_CONFIG_OFFSET);
> +	val &= ~TIM_CONFIG_INTERRUPT_PENDING;
> +	writel(val, base + TIM_CONFIG_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);
> +	void __iomem *tim_base = timer_of_base(to);
> +
> +	keembay_timer_disable(tim_base);
> +	keembay_timer_update_counter(tim_base, evt);
> +	keembay_timer_enable(tim_base, 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);
> +	void __iomem *tim_base = timer_of_base(to);
> +
> +	keembay_timer_disable(tim_base);
> +	keembay_timer_update_counter(tim_base, timer_of_period(to));
> +	keembay_timer_enable(tim_base, 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);
> +	void __iomem *tim_base = timer_of_base(to);
> +	u32 val;
> +
> +	val = readl(tim_base + TIM_CONFIG_OFFSET);
> +
> +	if (val & TIM_CONFIG_RESTART) {
> +		/* Clear interrupt for periodic timer*/

nit: comment format is:

/*
  * my comment
  */

One line comment format is usually for the network subsystem

> +		keembay_timer_clear_pending_int(tim_base);
> +	} else {
> +		/* Disable the timer for one shot timer */

comment format

> +		keembay_timer_disable(tim_base);
> +	}
> +
> +	evt->event_handler(evt);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int __init keembay_clockevent_init(struct device_node *np)
> +{
> +	struct timer_of *keembay_ce_to;
> +	struct regmap *regmap;
> +	int ret;
> +	u32 val;
> +
> +	regmap = device_node_to_regmap(np->parent);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	ret = regmap_read(regmap, TIM_CONFIG_OFFSET, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* Prescaler bit must be enabled for the timer to function */

comment format

> +	if (!(val & TIM_CONFIG_PRESCALER_ENABLE)) {
> +		pr_err("%pOF: Prescaler is not enabled\n", np);
> +		ret = -ENODEV;
> +	}

Why bail out instead of enabling the prescalar ?

> +	keembay_ce_to = kzalloc(sizeof(*keembay_ce_to), GFP_KERNEL);
> +	if (!keembay_ce_to)
> +		ret = -ENOMEM;

As your convenience but I suggest global static variable initialization, 
that will simplify the init function regarding the error path.

> +	keembay_ce_to->flags = TIMER_OF_IRQ | TIMER_OF_BASE | TIMER_OF_CLOCK;
> +	keembay_ce_to->clkevt.name = "keembay_timer";
> +	keembay_ce_to->clkevt.cpumask = cpu_possible_mask;
> +	keembay_ce_to->clkevt.features = CLOCK_EVT_FEAT_PERIODIC |
> +					 CLOCK_EVT_FEAT_ONESHOT  |
> +					 CLOCK_EVT_FEAT_DYNIRQ;
> +	keembay_ce_to->clkevt.rating = TIM_RATING;
> +	keembay_ce_to->clkevt.set_next_event = keembay_timer_set_next_event;
> +	keembay_ce_to->clkevt.set_state_periodic = keembay_timer_periodic;
> +	keembay_ce_to->clkevt.set_state_shutdown = keembay_timer_shutdown;
> +	keembay_ce_to->of_irq.handler = keembay_timer_isr;
> +	keembay_ce_to->of_irq.flags = IRQF_TIMER;
> +
> +	ret = timer_of_init(np, keembay_ce_to);
> +	if (ret)
> +		goto err_keembay_ce_to_free;
> +
> +	ret = regmap_read(regmap, TIM_RELOAD_VAL_OFFSET, &val);
> +	if (ret)
> +		goto err_keembay_ce_to_free;
> +
> +	keembay_ce_to->of_clk.rate = keembay_ce_to->of_clk.rate / (val + 1);
> +
> +	clockevents_config_and_register(&keembay_ce_to->clkevt,
> +					timer_of_rate(keembay_ce_to),
> +					1,
> +					U32_MAX);
> +
> +	return 0;
> +
> +err_keembay_ce_to_free:
> +	kfree(keembay_ce_to);
> +
> +	return ret;
> +}
> +
> +static struct timer_of keembay_cs_to = {
> +	.flags	= TIMER_OF_BASE | TIMER_OF_CLOCK,
> +};
> +
> +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_MASK_BITS),
> +	.flags	= CLOCK_SOURCE_IS_CONTINUOUS |
> +		  CLOCK_SOURCE_SUSPEND_NONSTOP,
> +};
> +
> +static int __init keembay_clocksource_init(struct device_node *np)
> +{
> +	struct regmap *regmap;
> +	u32 val;
> +	int ret;
> +
> +	regmap = device_node_to_regmap(np->parent);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	ret = regmap_read(regmap, TIM_CONFIG_OFFSET, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* Free Running Counter bit must be enabled for counter to function */

comment format

> +	if (!(val & TIM_CONFIG_COUNTER_ENABLE)) {
> +		pr_err("%pOF: free running counter is not enabled\n", np);
> +		return -ENODEV;
> +	}

Same comment as above. Why not enable the counter ?

Other than that, the driver is having a good shape


-- 
<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 v8 2/2] clocksource: Add Intel Keem Bay timer support

[Sanil, Shruthi]

> -----Original Message-----
> From: Daniel Lezcano <daniel.lezcano@linaro.org>
> Sent: Wednesday, March 2, 2022 2:39 AM
> To: Sanil, Shruthi <shruthi.sanil@intel.com>; tglx@linutronix.de;
> robh+dt@kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org
> Cc: andriy.shevchenko@linux.intel.com; mgross@linux.intel.com; Thokala,
> Srikanth <srikanth.thokala@intel.com>; Raja Subramanian, Lakshmi Bai
> <lakshmi.bai.raja.subramanian@intel.com>; Sangannavar, Mallikarjunappa
> <mallikarjunappa.sangannavar@intel.com>
> Subject: Re: [PATCH v8 2/2] clocksource: Add Intel Keem Bay timer support
> 
> On 22/02/2022 10:56, shruthi.sanil@intel.com wrote:
> > From: Shruthi Sanil <shruthi.sanil@intel.com>
> >
> > The Intel Keem Bay timer driver supports clocksource and clockevent
> > features for the timer IP used in Intel Keem Bay SoC.
> > The timer block supports 1 free running counter and 8 timers.
> > The free running counter can be used as a clocksource and the timers
> > can be used as clockevent. Each timer is capable of generating
> > individual interrupt.
> > Both the features are enabled through the timer general config register.
> >
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> > Signed-off-by: Shruthi Sanil <shruthi.sanil@intel.com>
> > ---
> >   MAINTAINERS                         |   6 +
> >   drivers/clocksource/Kconfig         |  11 ++
> >   drivers/clocksource/Makefile        |   1 +
> >   drivers/clocksource/timer-keembay.c | 230
> ++++++++++++++++++++++++++++
> >   4 files changed, 248 insertions(+)
> >   create mode 100644 drivers/clocksource/timer-keembay.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > 777cd6fa2b3d..73c0029dcdf7 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9796,6 +9796,12 @@ F:	drivers/crypto/keembay/keembay-ocs-hcu-
> core.c
> >   F:	drivers/crypto/keembay/ocs-hcu.c
> >   F:	drivers/crypto/keembay/ocs-hcu.h
> >
> > +INTEL KEEM BAY TIMER DRIVER
> > +M:	Shruthi Sanil <shruthi.sanil@intel.com>
> > +S:	Maintained
> > +F:	Documentation/devicetree/bindings/timer/intel,keembay-
> timer.yaml
> > +F:	drivers/clocksource/timer-keembay.c
> > +
> >   INTEL THUNDER BAY EMMC PHY DRIVER
> >   M:	Nandhini Srikandan <nandhini.srikandan@intel.com>
> >   M:	Rashmi A <rashmi.a@intel.com>
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index cfb8ea0df3b1..65b6cf916e5a 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -721,4 +721,15 @@ config MICROCHIP_PIT64B
> >   	  modes and high resolution. It is used as a clocksource
> >   	  and a clockevent.
> >
> > +config KEEMBAY_TIMER
> > +	bool "Intel Keem Bay timer"
> > +	depends on ARCH_KEEMBAY || COMPILE_TEST
> > +	select TIMER_OF
> > +	help
> > +	  This option enables the support for the Intel Keem Bay
> > +	  general purpose timer and free running counter driver.
> > +	  Each timer can generate an individual interrupt and
> > +	  supports oneshot and periodic modes.
> > +	  The 64-bit counter can be used as a clock source.
> > +
> >   endmenu
> > diff --git a/drivers/clocksource/Makefile
> > b/drivers/clocksource/Makefile index fa5f624eadb6..dff6458ef9e5 100644
> > --- a/drivers/clocksource/Makefile
> > +++ b/drivers/clocksource/Makefile
> > @@ -89,3 +89,4 @@ obj-$(CONFIG_GX6605S_TIMER)		+=
> timer-gx6605s.o
> >   obj-$(CONFIG_HYPERV_TIMER)		+= hyperv_timer.o
> >   obj-$(CONFIG_MICROCHIP_PIT64B)		+= timer-microchip-pit64b.o
> >   obj-$(CONFIG_MSC313E_TIMER)		+= timer-msc313e.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..230609c06a26
> > --- /dev/null
> > +++ b/drivers/clocksource/timer-keembay.c
> > @@ -0,0 +1,230 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Intel Keem Bay Timer driver
> > + *
> > + * Copyright (C) 2020 Intel Corporation  */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io-64-nonatomic-lo-hi.h> #include
> > +<linux/mfd/syscon.h> #include <linux/module.h> #include
> > +<linux/of_address.h> #include <linux/sizes.h> #include <linux/slab.h>
> > +#include <linux/regmap.h>
> > +
> > +#include "timer-of.h"
> > +
> > +/* Timer register offset */
> > +#define TIM_CNT_VAL_OFFSET		0x0
> > +#define TIM_RELOAD_VAL_OFFSET		0x4
> > +#define TIM_CONFIG_OFFSET		0x8
> > +
> > +/* Bit fields of timer general config register */
> > +#define TIM_CONFIG_PRESCALER_ENABLE	BIT(2)
> > +#define TIM_CONFIG_COUNTER_ENABLE	BIT(0)
> > +
> > +/* Bit fields of timer config register */
> > +#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_GEN_MASK			GENMASK(31, 12)
> > +#define TIM_RATING			200
> > +#define TIM_CLKSRC_MASK_BITS		64
> > +
> > +#define TIMER_NAME_SIZE			25
> > +
> > +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_disable(void __iomem *base) {
> > +	writel(0x0, 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 inline void keembay_timer_clear_pending_int(void __iomem
> > +*base) {
> > +	u32 val;
> > +
> > +	val = readl(base + TIM_CONFIG_OFFSET);
> > +	val &= ~TIM_CONFIG_INTERRUPT_PENDING;
> > +	writel(val, base + TIM_CONFIG_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);
> > +	void __iomem *tim_base = timer_of_base(to);
> > +
> > +	keembay_timer_disable(tim_base);
> > +	keembay_timer_update_counter(tim_base, evt);
> > +	keembay_timer_enable(tim_base, 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);
> > +	void __iomem *tim_base = timer_of_base(to);
> > +
> > +	keembay_timer_disable(tim_base);
> > +	keembay_timer_update_counter(tim_base, timer_of_period(to));
> > +	keembay_timer_enable(tim_base, 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);
> > +	void __iomem *tim_base = timer_of_base(to);
> > +	u32 val;
> > +
> > +	val = readl(tim_base + TIM_CONFIG_OFFSET);
> > +
> > +	if (val & TIM_CONFIG_RESTART) {
> > +		/* Clear interrupt for periodic timer*/
> 
> nit: comment format is:
> 
> /*
>   * my comment
>   */
> 
> One line comment format is usually for the network subsystem

OK. I'll update the comment format.

> 
> > +		keembay_timer_clear_pending_int(tim_base);
> > +	} else {
> > +		/* Disable the timer for one shot timer */
> 
> comment format

I'll update the comment format.

> 
> > +		keembay_timer_disable(tim_base);
> > +	}
> > +
> > +	evt->event_handler(evt);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int __init keembay_clockevent_init(struct device_node *np) {
> > +	struct timer_of *keembay_ce_to;
> > +	struct regmap *regmap;
> > +	int ret;
> > +	u32 val;
> > +
> > +	regmap = device_node_to_regmap(np->parent);
> > +	if (IS_ERR(regmap))
> > +		return PTR_ERR(regmap);
> > +
> > +	ret = regmap_read(regmap, TIM_CONFIG_OFFSET, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Prescaler bit must be enabled for the timer to function */
> 
> comment format

I'll update the comment format.

> 
> > +	if (!(val & TIM_CONFIG_PRESCALER_ENABLE)) {
> > +		pr_err("%pOF: Prescaler is not enabled\n", np);
> > +		ret = -ENODEV;
> > +	}
> 
> Why bail out instead of enabling the prescalar ?

Because it is a secure register and it would be updated by the bootloader.

> 
> > +	keembay_ce_to = kzalloc(sizeof(*keembay_ce_to), GFP_KERNEL);
> > +	if (!keembay_ce_to)
> > +		ret = -ENOMEM;
> 
> As your convenience but I suggest global static variable initialization, that will
> simplify the init function regarding the error path.

Yes, I can update this to have a global static variable.
This was updated like this considering multiple timers.

> 
> > +	keembay_ce_to->flags = TIMER_OF_IRQ | TIMER_OF_BASE |
> TIMER_OF_CLOCK;
> > +	keembay_ce_to->clkevt.name = "keembay_timer";
> > +	keembay_ce_to->clkevt.cpumask = cpu_possible_mask;
> > +	keembay_ce_to->clkevt.features = CLOCK_EVT_FEAT_PERIODIC |
> > +					 CLOCK_EVT_FEAT_ONESHOT  |
> > +					 CLOCK_EVT_FEAT_DYNIRQ;
> > +	keembay_ce_to->clkevt.rating = TIM_RATING;
> > +	keembay_ce_to->clkevt.set_next_event =
> keembay_timer_set_next_event;
> > +	keembay_ce_to->clkevt.set_state_periodic =
> keembay_timer_periodic;
> > +	keembay_ce_to->clkevt.set_state_shutdown =
> keembay_timer_shutdown;
> > +	keembay_ce_to->of_irq.handler = keembay_timer_isr;
> > +	keembay_ce_to->of_irq.flags = IRQF_TIMER;
> > +
> > +	ret = timer_of_init(np, keembay_ce_to);
> > +	if (ret)
> > +		goto err_keembay_ce_to_free;
> > +
> > +	ret = regmap_read(regmap, TIM_RELOAD_VAL_OFFSET, &val);
> > +	if (ret)
> > +		goto err_keembay_ce_to_free;
> > +
> > +	keembay_ce_to->of_clk.rate = keembay_ce_to->of_clk.rate / (val +
> 1);
> > +
> > +	clockevents_config_and_register(&keembay_ce_to->clkevt,
> > +					timer_of_rate(keembay_ce_to),
> > +					1,
> > +					U32_MAX);
> > +
> > +	return 0;
> > +
> > +err_keembay_ce_to_free:
> > +	kfree(keembay_ce_to);
> > +
> > +	return ret;
> > +}
> > +
> > +static struct timer_of keembay_cs_to = {
> > +	.flags	= TIMER_OF_BASE | TIMER_OF_CLOCK,
> > +};
> > +
> > +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_MASK_BITS),
> > +	.flags	= CLOCK_SOURCE_IS_CONTINUOUS |
> > +		  CLOCK_SOURCE_SUSPEND_NONSTOP,
> > +};
> > +
> > +static int __init keembay_clocksource_init(struct device_node *np) {
> > +	struct regmap *regmap;
> > +	u32 val;
> > +	int ret;
> > +
> > +	regmap = device_node_to_regmap(np->parent);
> > +	if (IS_ERR(regmap))
> > +		return PTR_ERR(regmap);
> > +
> > +	ret = regmap_read(regmap, TIM_CONFIG_OFFSET, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Free Running Counter bit must be enabled for counter to function
> > +*/
> 
> comment format

I'll update the comment format.

> 
> > +	if (!(val & TIM_CONFIG_COUNTER_ENABLE)) {
> > +		pr_err("%pOF: free running counter is not enabled\n", np);
> > +		return -ENODEV;
> > +	}
> 
> Same comment as above. Why not enable the counter ?

Because it is a secure register and it would be updated by the bootloader.

> 
> Other than that, the driver is having a good shape

Thank You!

Regards,
Shruthi

> 
> 
> --
> <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 v8 2/2] clocksource: Add Intel Keem Bay timer support

[Daniel Lezcano]

On 02/03/2022 11:12, Sanil, Shruthi wrote:

[ ... ]

>>> +	if (!(val & TIM_CONFIG_PRESCALER_ENABLE)) {
>>> +		pr_err("%pOF: Prescaler is not enabled\n", np);
>>> +		ret = -ENODEV;
>>> +	}
>>
>> Why bail out instead of enabling the prescalar ?
> 
> Because it is a secure register and it would be updated by the bootloader.
Should it be considered as a firmware bug ?

-- 
<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 v8 2/2] clocksource: Add Intel Keem Bay timer support

[Andy Shevchenko]

On Tue, Mar 01, 2022 at 10:09:06PM +0100, Daniel Lezcano wrote:
> On 22/02/2022 10:56, shruthi.sanil@intel.com wrote:

> > +		/* Clear interrupt for periodic timer*/
> 
> nit: comment format is:
> 
> /*
>  * my comment
>  */
> 
> One line comment format is usually for the network subsystem

Huh?
Any pointers to the documentation, please?

> > +		keembay_timer_clear_pending_int(tim_base);
> > +	} else {
> > +		/* Disable the timer for one shot timer */
> 
> comment format
> 
> > +		keembay_timer_disable(tim_base);
> > +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 2/2] clocksource: Add Intel Keem Bay timer support

[andriy.shevchenko]

On Wed, Mar 02, 2022 at 10:12:49AM +0000, Sanil, Shruthi wrote:
> > From: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Sent: Wednesday, March 2, 2022 2:39 AM
> > Subject: Re: [PATCH v8 2/2] clocksource: Add Intel Keem Bay timer support
> > On 22/02/2022 10:56, shruthi.sanil@intel.com wrote:

...

> > One line comment format is usually for the network subsystem
> 
> OK. I'll update the comment format.

Hold on, we need a proof from documentation.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 2/2] clocksource: Add Intel Keem Bay timer support

[Daniel Lezcano]

On 02/03/2022 14:53, Andy Shevchenko wrote:
> On Tue, Mar 01, 2022 at 10:09:06PM +0100, Daniel Lezcano wrote:
>> On 22/02/2022 10:56, shruthi.sanil@intel.com wrote:
> 
>>> +		/* Clear interrupt for periodic timer*/
>>
>> nit: comment format is:
>>
>> /*
>>   * my comment
>>   */
>>
>> One line comment format is usually for the network subsystem
> 
> Huh?
> Any pointers to the documentation, please?

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst#n598

Well actually it is for multi line, so I may have confused with these 
one line comments.

On the other hand having one line comment telling what does the function 
right after is not really useful. The function names are self-explanatory.

>>> +		keembay_timer_clear_pending_int(tim_base);
>>> +	} else {
>>> +		/* Disable the timer for one shot timer */
>>
>> comment format
>>
>>> +		keembay_timer_disable(tim_base);
>>> +	}
> 


-- 
<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 v8 2/2] clocksource: Add Intel Keem Bay timer support

[Andy Shevchenko]

On Wed, Mar 02, 2022 at 04:58:08PM +0100, Daniel Lezcano wrote:
> On 02/03/2022 14:53, Andy Shevchenko wrote:
> > On Tue, Mar 01, 2022 at 10:09:06PM +0100, Daniel Lezcano wrote:
> > > On 22/02/2022 10:56, shruthi.sanil@intel.com wrote:

...

> > > One line comment format is usually for the network subsystem

> > Any pointers to the documentation, please?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst#n598
> 
> Well actually it is for multi line, so I may have confused with these one
> line comments.

Seems so.

> On the other hand having one line comment telling what does the function
> right after is not really useful. The function names are self-explanatory.

Agree on this.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v8 2/2] clocksource: Add Intel Keem Bay timer support

[Sanil, Shruthi]

> -----Original Message-----
> From: Daniel Lezcano <daniel.lezcano@linaro.org>
> Sent: Wednesday, March 2, 2022 3:54 PM
> To: Sanil, Shruthi <shruthi.sanil@intel.com>; tglx@linutronix.de;
> robh+dt@kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org
> Cc: andriy.shevchenko@linux.intel.com; mgross@linux.intel.com; Thokala,
> Srikanth <srikanth.thokala@intel.com>; Raja Subramanian, Lakshmi Bai
> <lakshmi.bai.raja.subramanian@intel.com>; Sangannavar, Mallikarjunappa
> <mallikarjunappa.sangannavar@intel.com>
> Subject: Re: [PATCH v8 2/2] clocksource: Add Intel Keem Bay timer support
> 
> On 02/03/2022 11:12, Sanil, Shruthi wrote:
> 
> [ ... ]
> 
> >>> +	if (!(val & TIM_CONFIG_PRESCALER_ENABLE)) {
> >>> +		pr_err("%pOF: Prescaler is not enabled\n", np);
> >>> +		ret = -ENODEV;
> >>> +	}
> >>
> >> Why bail out instead of enabling the prescalar ?
> >
> > Because it is a secure register and it would be updated by the bootloader.
> Should it be considered as a firmware bug ?

No. This is a common driver across products in the series and enablement of this bit depends on the project requirements.
Hence to be sure from driver, we added this check to avoid initialization of the driver in the case where it cannot be functional.

> 
> --
> <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 v8 2/2] clocksource: Add Intel Keem Bay timer support

[Daniel Lezcano]

On 02/03/2022 17:07, Sanil, Shruthi wrote:
>> -----Original Message----- From: Daniel Lezcano
>> <daniel.lezcano@linaro.org> Sent: Wednesday, March 2, 2022 3:54 PM 
>> To: Sanil, Shruthi <shruthi.sanil@intel.com>; tglx@linutronix.de; 
>> robh+dt@kernel.org; linux-kernel@vger.kernel.org; 
>> devicetree@vger.kernel.org Cc: andriy.shevchenko@linux.intel.com;
>> mgross@linux.intel.com; Thokala, Srikanth
>> <srikanth.thokala@intel.com>; Raja Subramanian, Lakshmi Bai 
>> <lakshmi.bai.raja.subramanian@intel.com>; Sangannavar,
>> Mallikarjunappa <mallikarjunappa.sangannavar@intel.com> Subject:
>> Re: [PATCH v8 2/2] clocksource: Add Intel Keem Bay timer support
>> 
>> On 02/03/2022 11:12, Sanil, Shruthi wrote:
>> 
>> [ ... ]
>> 
>>>>> +	if (!(val & TIM_CONFIG_PRESCALER_ENABLE)) { +
>>>>> pr_err("%pOF: Prescaler is not enabled\n", np); +		ret =
>>>>> -ENODEV; +	}
>>>> 
>>>> Why bail out instead of enabling the prescalar ?
>>> 
>>> Because it is a secure register and it would be updated by the
>>> bootloader.
>> Should it be considered as a firmware bug ?
> 
> No. This is a common driver across products in the series and
> enablement of this bit depends on the project requirements. Hence to
> be sure from driver, we added this check to avoid initialization of
> the driver in the case where it cannot be functional.

I'm not sure to get the meaning of 'project requirements' but (for my 
understanding) why not describe the timer in the DT for such projects?


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

RE: [PATCH v8 2/2] clocksource: Add Intel Keem Bay timer support

[Sanil, Shruthi]

Regards,
Shruthi

> -----Original Message-----
> From: Daniel Lezcano <daniel.lezcano@linaro.org>
> Sent: Wednesday, March 2, 2022 9:57 PM
> To: Sanil, Shruthi <shruthi.sanil@intel.com>; tglx@linutronix.de;
> robh+dt@kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org
> Cc: andriy.shevchenko@linux.intel.com; mgross@linux.intel.com; Thokala,
> Srikanth <srikanth.thokala@intel.com>; Raja Subramanian, Lakshmi Bai
> <lakshmi.bai.raja.subramanian@intel.com>; Sangannavar, Mallikarjunappa
> <mallikarjunappa.sangannavar@intel.com>
> Subject: Re: [PATCH v8 2/2] clocksource: Add Intel Keem Bay timer support
> 
> On 02/03/2022 17:07, Sanil, Shruthi wrote:
> >> -----Original Message----- From: Daniel Lezcano
> >> <daniel.lezcano@linaro.org> Sent: Wednesday, March 2, 2022 3:54 PM
> >> To: Sanil, Shruthi <shruthi.sanil@intel.com>; tglx@linutronix.de;
> >> robh+dt@kernel.org; linux-kernel@vger.kernel.org;
> >> devicetree@vger.kernel.org Cc: andriy.shevchenko@linux.intel.com;
> >> mgross@linux.intel.com; Thokala, Srikanth
> >> <srikanth.thokala@intel.com>; Raja Subramanian, Lakshmi Bai
> >> <lakshmi.bai.raja.subramanian@intel.com>; Sangannavar,
> >> Mallikarjunappa <mallikarjunappa.sangannavar@intel.com> Subject:
> >> Re: [PATCH v8 2/2] clocksource: Add Intel Keem Bay timer support
> >>
> >> On 02/03/2022 11:12, Sanil, Shruthi wrote:
> >>
> >> [ ... ]
> >>
> >>>>> +	if (!(val & TIM_CONFIG_PRESCALER_ENABLE)) { +
> >>>>> pr_err("%pOF: Prescaler is not enabled\n", np); +		ret =
> >>>>> -ENODEV; +	}
> >>>>
> >>>> Why bail out instead of enabling the prescalar ?
> >>>
> >>> Because it is a secure register and it would be updated by the
> >>> bootloader.
> >> Should it be considered as a firmware bug ?
> >
> > No. This is a common driver across products in the series and
> > enablement of this bit depends on the project requirements. Hence to
> > be sure from driver, we added this check to avoid initialization of
> > the driver in the case where it cannot be functional.
> 
> I'm not sure to get the meaning of 'project requirements' but (for my
> understanding) why not describe the timer in the DT for such projects?
> 

OK, I understand your point now. We can control the driver initialization from device tree binding rather than add a check in the driver.
But isn't it good to have a check, if enabling of the bit is missed out in the FW? This can help in debugging.

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

Re: [PATCH v8 2/2] clocksource: Add Intel Keem Bay timer support

[Daniel Lezcano]

On 03/03/2022 07:18, Sanil, Shruthi wrote:

[ ... ]

>>>>>>> +	if (!(val & TIM_CONFIG_PRESCALER_ENABLE)) { + 
>>>>>>> pr_err("%pOF: Prescaler is not enabled\n", np); +		ret = 
>>>>>>> -ENODEV; +	}
>>>>>> 
>>>>>> Why bail out instead of enabling the prescalar ?
>>>>> 
>>>>> Because it is a secure register and it would be updated by
>>>>> the bootloader.
>>>> Should it be considered as a firmware bug ?
>>> 
>>> No. This is a common driver across products in the series and 
>>> enablement of this bit depends on the project requirements. Hence
>>> to be sure from driver, we added this check to avoid
>>> initialization of the driver in the case where it cannot be
>>> functional.
>> 
>> I'm not sure to get the meaning of 'project requirements' but (for
>> my understanding) why not describe the timer in the DT for such
>> projects?
>> 
> 
> OK, I understand your point now. We can control the driver
> initialization from device tree binding rather than add a check in
> the driver. But isn't it good to have a check, if enabling of the bit
> is missed out in the FW? This can help in debugging.

So if the description is in the DT but the prescaler bit is not enabled 
then the firmware is buggy, IIUC. Yeah, this check would help, may be 
add more context in the error message, eg. "Firmware has not enabled the 
prescaler bit" or something like that

Thanks for the clarification

   -- D.



-- 
<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 v8 2/2] clocksource: Add Intel Keem Bay timer support

[Sanil, Shruthi]

> -----Original Message-----
> From: Daniel Lezcano <daniel.lezcano@linaro.org>
> Sent: Thursday, March 3, 2022 3:48 PM
> To: Sanil, Shruthi <shruthi.sanil@intel.com>; tglx@linutronix.de;
> robh+dt@kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org
> Cc: andriy.shevchenko@linux.intel.com; mgross@linux.intel.com; Thokala,
> Srikanth <srikanth.thokala@intel.com>; Raja Subramanian, Lakshmi Bai
> <lakshmi.bai.raja.subramanian@intel.com>; Sangannavar, Mallikarjunappa
> <mallikarjunappa.sangannavar@intel.com>
> Subject: Re: [PATCH v8 2/2] clocksource: Add Intel Keem Bay timer support
> 
> On 03/03/2022 07:18, Sanil, Shruthi wrote:
> 
> [ ... ]
> 
> >>>>>>> +	if (!(val & TIM_CONFIG_PRESCALER_ENABLE)) { +
> >>>>>>> pr_err("%pOF: Prescaler is not enabled\n", np); +		ret =
> >>>>>>> -ENODEV; +	}
> >>>>>>
> >>>>>> Why bail out instead of enabling the prescalar ?
> >>>>>
> >>>>> Because it is a secure register and it would be updated by the
> >>>>> bootloader.
> >>>> Should it be considered as a firmware bug ?
> >>>
> >>> No. This is a common driver across products in the series and
> >>> enablement of this bit depends on the project requirements. Hence to
> >>> be sure from driver, we added this check to avoid initialization of
> >>> the driver in the case where it cannot be functional.
> >>
> >> I'm not sure to get the meaning of 'project requirements' but (for my
> >> understanding) why not describe the timer in the DT for such
> >> projects?
> >>
> >
> > OK, I understand your point now. We can control the driver
> > initialization from device tree binding rather than add a check in the
> > driver. But isn't it good to have a check, if enabling of the bit is
> > missed out in the FW? This can help in debugging.
> 
> So if the description is in the DT but the prescaler bit is not enabled then the
> firmware is buggy, IIUC. Yeah, this check would help, may be add more
Yes, right. It would mean the FW is buggy.

> context in the error message, eg. "Firmware has not enabled the prescaler
> bit" or something like that
> 
> Thanks for the clarification

Yes, Sure. I'll update the comment accordingly.
Thank you 😊

> 
>    -- D.
> 
> 
> 
> --
> <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 v8 2/2] clocksource: Add Intel Keem Bay timer support

[andriy.shevchenko]

On Thu, Mar 03, 2022 at 11:17:33AM +0100, Daniel Lezcano wrote:
> On 03/03/2022 07:18, Sanil, Shruthi wrote:

> > > > > > > > +	if (!(val & TIM_CONFIG_PRESCALER_ENABLE)) { +
> > > > > > > > pr_err("%pOF: Prescaler is not enabled\n", np);
> > > > > > > > +		ret = -ENODEV; +	}
> > > > > > > 
> > > > > > > Why bail out instead of enabling the prescalar ?
> > > > > > 
> > > > > > Because it is a secure register and it would be updated by
> > > > > > the bootloader.
> > > > > Should it be considered as a firmware bug ?
> > > > 
> > > > No. This is a common driver across products in the series and
> > > > enablement of this bit depends on the project requirements.
> > > > Hence
> > > > to be sure from driver, we added this check to avoid
> > > > initialization of the driver in the case where it cannot be
> > > > functional.
> > > 
> > > I'm not sure to get the meaning of 'project requirements' but (for
> > > my understanding) why not describe the timer in the DT for such
> > > projects?
> > > 
> > 
> > OK, I understand your point now. We can control the driver
> > initialization from device tree binding rather than add a check in
> > the driver. But isn't it good to have a check, if enabling of the bit
> > is missed out in the FW? This can help in debugging.
> 
> So if the description is in the DT but the prescaler bit is not enabled then
> the firmware is buggy, IIUC. Yeah, this check would help, may be add more
> context in the error message, eg. "Firmware has not enabled the prescaler
> bit" or something like that

For this we also use a FW_BUG prefix to printf()-like functions.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 2/2] clocksource: Add Intel Keem Bay timer support

[andriy.shevchenko]

On Thu, Mar 03, 2022 at 10:23:43AM +0000, Sanil, Shruthi wrote:
> > From: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Sent: Thursday, March 3, 2022 3:48 PM
> > On 03/03/2022 07:18, Sanil, Shruthi wrote:

...

> > So if the description is in the DT but the prescaler bit is not enabled then the
> > firmware is buggy, IIUC. Yeah, this check would help, may be add more
> Yes, right. It would mean the FW is buggy.

Make use of FW_BUG prefix instead of "Firmware" in the message.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v8 2/2] clocksource: Add Intel Keem Bay timer support

[Sanil, Shruthi]

> -----Original Message-----
> From: andriy.shevchenko@linux.intel.com
> <andriy.shevchenko@linux.intel.com>
> Sent: Thursday, March 3, 2022 4:18 PM
> To: Sanil, Shruthi <shruthi.sanil@intel.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>; tglx@linutronix.de;
> robh+dt@kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; mgross@linux.intel.com; Thokala, Srikanth
> <srikanth.thokala@intel.com>; Raja Subramanian, Lakshmi Bai
> <lakshmi.bai.raja.subramanian@intel.com>; Sangannavar, Mallikarjunappa
> <mallikarjunappa.sangannavar@intel.com>
> Subject: Re: [PATCH v8 2/2] clocksource: Add Intel Keem Bay timer support
> 
> On Thu, Mar 03, 2022 at 10:23:43AM +0000, Sanil, Shruthi wrote:
> > > From: Daniel Lezcano <daniel.lezcano@linaro.org>
> > > Sent: Thursday, March 3, 2022 3:48 PM On 03/03/2022 07:18, Sanil,
> > > Shruthi wrote:
> 
> ...
> 
> > > So if the description is in the DT but the prescaler bit is not
> > > enabled then the firmware is buggy, IIUC. Yeah, this check would
> > > help, may be add more
> > Yes, right. It would mean the FW is buggy.
> 
> Make use of FW_BUG prefix instead of "Firmware" in the message.

Sure. I'll update accordingly.

> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Re: [PATCH v8 2/2] clocksource: Add Intel Keem Bay timer support

[Daniel Lezcano]

On 03/03/2022 11:47, andriy.shevchenko@linux.intel.com wrote:
> On Thu, Mar 03, 2022 at 11:17:33AM +0100, Daniel Lezcano wrote:
>> On 03/03/2022 07:18, Sanil, Shruthi wrote:
> 
>>>>>>>>> +	if (!(val & TIM_CONFIG_PRESCALER_ENABLE)) { +
>>>>>>>>> pr_err("%pOF: Prescaler is not enabled\n", np);
>>>>>>>>> +		ret = -ENODEV; +	}
>>>>>>>>
>>>>>>>> Why bail out instead of enabling the prescalar ?
>>>>>>>
>>>>>>> Because it is a secure register and it would be updated by
>>>>>>> the bootloader.
>>>>>> Should it be considered as a firmware bug ?
>>>>>
>>>>> No. This is a common driver across products in the series and
>>>>> enablement of this bit depends on the project requirements.
>>>>> Hence
>>>>> to be sure from driver, we added this check to avoid
>>>>> initialization of the driver in the case where it cannot be
>>>>> functional.
>>>>
>>>> I'm not sure to get the meaning of 'project requirements' but (for
>>>> my understanding) why not describe the timer in the DT for such
>>>> projects?
>>>>
>>>
>>> OK, I understand your point now. We can control the driver
>>> initialization from device tree binding rather than add a check in
>>> the driver. But isn't it good to have a check, if enabling of the bit
>>> is missed out in the FW? This can help in debugging.
>>
>> So if the description is in the DT but the prescaler bit is not enabled then
>> the firmware is buggy, IIUC. Yeah, this check would help, may be add more
>> context in the error message, eg. "Firmware has not enabled the prescaler
>> bit" or something like that
> 
> For this we also use a FW_BUG prefix to printf()-like functions.

Thanks for the information, I was unaware of this prefix.

Good to know ;)



-- 
<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 v8 1/2] dt-bindings: timer: Add bindings for Intel Keem Bay SoC Timer

[Rob Herring]

On Wed, Feb 23, 2022 at 5:31 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Feb 22, 2022 at 05:13:41PM -0600, Rob Herring wrote:
> > On Tue, Feb 22, 2022 at 03:26:53PM +0530, shruthi.sanil@intel.com wrote:
> > > From: Shruthi Sanil <shruthi.sanil@intel.com>
> > >
> > > Add Device Tree bindings for the Timer IP, which can be used as
> > > clocksource and clockevent device in the Intel Keem Bay SoC.
>
> ...
>
> > > +    soc {
> > > +        #address-cells = <0x2>;
> > > +        #size-cells = <0x2>;
> > > +
> > > +        gpt@20331000 {
> > > +            compatible = "intel,keembay-gpt-creg", "simple-mfd";
> >
> > It looks like you are splitting things based on Linux implementation
> > details. Does this h/w block have different combinations of timers and
> > counters? If not, then you don't need the child nodes at all. There's
> > plenty of h/w blocks that get used as both a clocksource and clockevent.
> >
> > Maybe I already raised this, but assume I don't remember and this patch
> > needs to address any questions I already asked.
>
> I dunno if I mentioned that hardware seems to have 5 or so devices behind
> the block, so ideally it should be one device node that represents the global
> register spaces and several children nodes.

Is it 5 devices or 9 devices?

> However, I am not familiar with the established practices in DT world, but
> above seems to me the right thing to do since it describes the hardware as
> is (without any linuxisms).

The Linuxism in these cases defining 1 node per driver because that's
what is convenient for automatic probing. That appears to be exactly
the case here. The red flag is nodes with a compatible and nothing
else. The next question is whether the sub-devices are blocks that
will be assembled in varying combinations and quantities. If not, then
not much point subdividing the h/w blocks.

There's also many cases of having multiple 'identical' timers and
wanting to encode which timer gets assigned to clocksource vs.
clockevent. But those 'identical' timers aren't if you care about
which timer gets assigned where. I *think* that's not the case here
unless you are trying to pick the timer for the clockevent by not
defining the other timers.

Without having a complete picture of what's in 'gpt-creg', I can't
give better advice.

Rob

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

[Andy Shevchenko]

On Mon, Mar 07, 2022 at 04:33:23PM -0600, Rob Herring wrote:
> On Wed, Feb 23, 2022 at 5:31 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Tue, Feb 22, 2022 at 05:13:41PM -0600, Rob Herring wrote:
> > > On Tue, Feb 22, 2022 at 03:26:53PM +0530, shruthi.sanil@intel.com wrote:
> > > > From: Shruthi Sanil <shruthi.sanil@intel.com>
> > > >
> > > > Add Device Tree bindings for the Timer IP, which can be used as
> > > > clocksource and clockevent device in the Intel Keem Bay SoC.
> >
> > ...
> >
> > > > +    soc {
> > > > +        #address-cells = <0x2>;
> > > > +        #size-cells = <0x2>;
> > > > +
> > > > +        gpt@20331000 {
> > > > +            compatible = "intel,keembay-gpt-creg", "simple-mfd";
> > >
> > > It looks like you are splitting things based on Linux implementation
> > > details. Does this h/w block have different combinations of timers and
> > > counters? If not, then you don't need the child nodes at all. There's
> > > plenty of h/w blocks that get used as both a clocksource and clockevent.
> > >
> > > Maybe I already raised this, but assume I don't remember and this patch
> > > needs to address any questions I already asked.
> >
> > I dunno if I mentioned that hardware seems to have 5 or so devices behind
> > the block, so ideally it should be one device node that represents the global
> > register spaces and several children nodes.
> 
> Is it 5 devices or 9 devices?

5 devices, one of which is a timer block out of 8 timers.
You may count them as 12 altogether.

> > However, I am not familiar with the established practices in DT world, but
> > above seems to me the right thing to do since it describes the hardware as
> > is (without any linuxisms).
> 
> The Linuxism in these cases defining 1 node per driver because that's
> what is convenient for automatic probing. That appears to be exactly
> the case here. The red flag is nodes with a compatible and nothing
> else. The next question is whether the sub-devices are blocks that
> will be assembled in varying combinations and quantities. If not, then
> not much point subdividing the h/w blocks.

AFAIU the hardware architecture the amount of timers is dependent on
the IP synthesis configuration. On this platform it's 8, but it may be
1 or 2, for example.

> There's also many cases of having multiple 'identical' timers and
> wanting to encode which timer gets assigned to clocksource vs.
> clockevent. But those 'identical' timers aren't if you care about
> which timer gets assigned where. I *think* that's not the case here
> unless you are trying to pick the timer for the clockevent by not
> defining the other timers.
> 
> Without having a complete picture of what's in 'gpt-creg', I can't
> give better advice.

I guess they need to share TRM, if possible, to show what this
block is.

-- 
With Best Regards,
Andy Shevchenko

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

[Sanil, Shruthi]

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Tuesday, March 8, 2022 3:44 PM
> To: Rob Herring <robh@kernel.org>
> Cc: Sanil, Shruthi <shruthi.sanil@intel.com>; Daniel Lezcano
> <daniel.lezcano@linaro.org>; Thomas Gleixner <tglx@linutronix.de>; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org; Mark Gross
> <mgross@linux.intel.com>; Thokala, Srikanth <srikanth.thokala@intel.com>;
> Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>;
> Sangannavar, Mallikarjunappa <mallikarjunappa.sangannavar@intel.com>
> Subject: Re: [PATCH v8 1/2] dt-bindings: timer: Add bindings for Intel Keem
> Bay SoC Timer
> 
> On Mon, Mar 07, 2022 at 04:33:23PM -0600, Rob Herring wrote:
> > On Wed, Feb 23, 2022 at 5:31 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Tue, Feb 22, 2022 at 05:13:41PM -0600, Rob Herring wrote:
> > > > On Tue, Feb 22, 2022 at 03:26:53PM +0530, shruthi.sanil@intel.com
> wrote:
> > > > > From: Shruthi Sanil <shruthi.sanil@intel.com>
> > > > >
> > > > > Add Device Tree bindings for the Timer IP, which can be used as
> > > > > clocksource and clockevent device in the Intel Keem Bay SoC.
> > >
> > > ...
> > >
> > > > > +    soc {
> > > > > +        #address-cells = <0x2>;
> > > > > +        #size-cells = <0x2>;
> > > > > +
> > > > > +        gpt@20331000 {
> > > > > +            compatible = "intel,keembay-gpt-creg",
> > > > > + "simple-mfd";
> > > >
> > > > It looks like you are splitting things based on Linux
> > > > implementation details. Does this h/w block have different
> > > > combinations of timers and counters? If not, then you don't need
> > > > the child nodes at all. There's plenty of h/w blocks that get used as both
> a clocksource and clockevent.
> > > >
> > > > Maybe I already raised this, but assume I don't remember and this
> > > > patch needs to address any questions I already asked.
> > >
> > > I dunno if I mentioned that hardware seems to have 5 or so devices
> > > behind the block, so ideally it should be one device node that
> > > represents the global register spaces and several children nodes.
> >
> > Is it 5 devices or 9 devices?
> 
> 5 devices, one of which is a timer block out of 8 timers.
> You may count them as 12 altogether.
> 
> > > However, I am not familiar with the established practices in DT
> > > world, but above seems to me the right thing to do since it
> > > describes the hardware as is (without any linuxisms).
> >
> > The Linuxism in these cases defining 1 node per driver because that's
> > what is convenient for automatic probing. That appears to be exactly
> > the case here. The red flag is nodes with a compatible and nothing
> > else. The next question is whether the sub-devices are blocks that
> > will be assembled in varying combinations and quantities. If not, then
> > not much point subdividing the h/w blocks.
> 
> AFAIU the hardware architecture the amount of timers is dependent on the
> IP synthesis configuration. On this platform it's 8, but it may be
> 1 or 2, for example.

Yes, the number of timers can vary between platforms.
For eg., Intel Keem Bay SoC has 8 timers where as in Intel Thunder Bay SoC has 6 timers.

> 
> > There's also many cases of having multiple 'identical' timers and
> > wanting to encode which timer gets assigned to clocksource vs.
> > clockevent. But those 'identical' timers aren't if you care about
> > which timer gets assigned where. I *think* that's not the case here
> > unless you are trying to pick the timer for the clockevent by not
> > defining the other timers.
> >
> > Without having a complete picture of what's in 'gpt-creg', I can't
> > give better advice.
> 
> I guess they need to share TRM, if possible, to show what this block is.
> 

I would like to explain briefly about the Timer IP in the Keem Bay Soc.
The Timers block contains 8 general purpose timers, a free running counter. Each general purpose timer can generate an individual interrupt to the interrupt controller.
The timer block consists of secure and non-secure timers. Hence there are secure and non-secure registers in separate address banks.
The secure register bank consists of the common control register where the timers and counters need to be enabled.
From the driver we try to check if these bits are enabled to continue with the initialization of the driver.
Hence we need to pass the base address of both the address banks to the driver from the DTB.
The control register is common for both timer and counter. Hence we went for parent child module in DTB. 'gpt-creg' represents this control register.

> --
> With Best Regards,
> Andy Shevchenko
> 

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

[Sanil, Shruthi]

Hi Rob,

I tried to the technical manual that could be shared publicly, but couldn't find one which had the timer IP details.
In the email below I have tried to answer your question. Could you please let me know if I was able to answer your question?
Can we try to discuss and close it in the best possible way? Need your help in taking this patch forward?

Regards,
Shruthi

> -----Original Message-----
> From: Sanil, Shruthi
> Sent: Friday, March 18, 2022 11:07 AM
> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Rob Herring
> <robh@kernel.org>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>; Thomas Gleixner
> <tglx@linutronix.de>; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; Mark Gross <mgross@linux.intel.com>;
> Thokala, Srikanth <srikanth.thokala@intel.com>; Raja Subramanian, Lakshmi
> Bai <lakshmi.bai.raja.subramanian@intel.com>; Sangannavar,
> Mallikarjunappa <mallikarjunappa.sangannavar@intel.com>
> Subject: RE: [PATCH v8 1/2] dt-bindings: timer: Add bindings for Intel Keem
> Bay SoC Timer
> 
> > -----Original Message-----
> > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Sent: Tuesday, March 8, 2022 3:44 PM
> > To: Rob Herring <robh@kernel.org>
> > Cc: Sanil, Shruthi <shruthi.sanil@intel.com>; Daniel Lezcano
> > <daniel.lezcano@linaro.org>; Thomas Gleixner <tglx@linutronix.de>;
> > linux- kernel@vger.kernel.org; devicetree@vger.kernel.org; Mark Gross
> > <mgross@linux.intel.com>; Thokala, Srikanth
> > <srikanth.thokala@intel.com>; Raja Subramanian, Lakshmi Bai
> > <lakshmi.bai.raja.subramanian@intel.com>;
> > Sangannavar, Mallikarjunappa <mallikarjunappa.sangannavar@intel.com>
> > Subject: Re: [PATCH v8 1/2] dt-bindings: timer: Add bindings for Intel
> > Keem Bay SoC Timer
> >
> > On Mon, Mar 07, 2022 at 04:33:23PM -0600, Rob Herring wrote:
> > > On Wed, Feb 23, 2022 at 5:31 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > On Tue, Feb 22, 2022 at 05:13:41PM -0600, Rob Herring wrote:
> > > > > On Tue, Feb 22, 2022 at 03:26:53PM +0530,
> > > > > shruthi.sanil@intel.com
> > wrote:
> > > > > > From: Shruthi Sanil <shruthi.sanil@intel.com>
> > > > > >
> > > > > > Add Device Tree bindings for the Timer IP, which can be used
> > > > > > as clocksource and clockevent device in the Intel Keem Bay SoC.
> > > >
> > > > ...
> > > >
> > > > > > +    soc {
> > > > > > +        #address-cells = <0x2>;
> > > > > > +        #size-cells = <0x2>;
> > > > > > +
> > > > > > +        gpt@20331000 {
> > > > > > +            compatible = "intel,keembay-gpt-creg",
> > > > > > + "simple-mfd";
> > > > >
> > > > > It looks like you are splitting things based on Linux
> > > > > implementation details. Does this h/w block have different
> > > > > combinations of timers and counters? If not, then you don't need
> > > > > the child nodes at all. There's plenty of h/w blocks that get
> > > > > used as both
> > a clocksource and clockevent.
> > > > >
> > > > > Maybe I already raised this, but assume I don't remember and
> > > > > this patch needs to address any questions I already asked.
> > > >
> > > > I dunno if I mentioned that hardware seems to have 5 or so devices
> > > > behind the block, so ideally it should be one device node that
> > > > represents the global register spaces and several children nodes.
> > >
> > > Is it 5 devices or 9 devices?
> >
> > 5 devices, one of which is a timer block out of 8 timers.
> > You may count them as 12 altogether.
> >
> > > > However, I am not familiar with the established practices in DT
> > > > world, but above seems to me the right thing to do since it
> > > > describes the hardware as is (without any linuxisms).
> > >
> > > The Linuxism in these cases defining 1 node per driver because
> > > that's what is convenient for automatic probing. That appears to be
> > > exactly the case here. The red flag is nodes with a compatible and
> > > nothing else. The next question is whether the sub-devices are
> > > blocks that will be assembled in varying combinations and
> > > quantities. If not, then not much point subdividing the h/w blocks.
> >
> > AFAIU the hardware architecture the amount of timers is dependent on
> > the IP synthesis configuration. On this platform it's 8, but it may be
> > 1 or 2, for example.
> 
> Yes, the number of timers can vary between platforms.
> For eg., Intel Keem Bay SoC has 8 timers where as in Intel Thunder Bay SoC
> has 6 timers.
> 
> >
> > > There's also many cases of having multiple 'identical' timers and
> > > wanting to encode which timer gets assigned to clocksource vs.
> > > clockevent. But those 'identical' timers aren't if you care about
> > > which timer gets assigned where. I *think* that's not the case here
> > > unless you are trying to pick the timer for the clockevent by not
> > > defining the other timers.
> > >
> > > Without having a complete picture of what's in 'gpt-creg', I can't
> > > give better advice.
> >
> > I guess they need to share TRM, if possible, to show what this block is.
> >
> 
> I would like to explain briefly about the Timer IP in the Keem Bay Soc.
> The Timers block contains 8 general purpose timers, a free running counter.
> Each general purpose timer can generate an individual interrupt to the
> interrupt controller.
> The timer block consists of secure and non-secure timers. Hence there are
> secure and non-secure registers in separate address banks.
> The secure register bank consists of the common control register where the
> timers and counters need to be enabled.
> From the driver we try to check if these bits are enabled to continue with the
> initialization of the driver.
> Hence we need to pass the base address of both the address banks to the
> driver from the DTB.
> The control register is common for both timer and counter. Hence we went
> for parent child module in DTB. 'gpt-creg' represents this control register.
> 
> > --
> > With Best Regards,
> > Andy Shevchenko
> >

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

[Sanil, Shruthi]

Hi Rob,

> -----Original Message-----
> From: Sanil, Shruthi
> Sent: Friday, March 18, 2022 11:07 AM
> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Rob Herring
> <robh@kernel.org>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>; Thomas Gleixner
> <tglx@linutronix.de>; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; Mark Gross <mgross@linux.intel.com>;
> Thokala, Srikanth <srikanth.thokala@intel.com>; Raja Subramanian, Lakshmi
> Bai <lakshmi.bai.raja.subramanian@intel.com>; Sangannavar,
> Mallikarjunappa <mallikarjunappa.sangannavar@intel.com>
> Subject: RE: [PATCH v8 1/2] dt-bindings: timer: Add bindings for Intel Keem
> Bay SoC Timer
> 
> > -----Original Message-----
> > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Sent: Tuesday, March 8, 2022 3:44 PM
> > To: Rob Herring <robh@kernel.org>
> > Cc: Sanil, Shruthi <shruthi.sanil@intel.com>; Daniel Lezcano
> > <daniel.lezcano@linaro.org>; Thomas Gleixner <tglx@linutronix.de>;
> > linux- kernel@vger.kernel.org; devicetree@vger.kernel.org; Mark Gross
> > <mgross@linux.intel.com>; Thokala, Srikanth
> > <srikanth.thokala@intel.com>; Raja Subramanian, Lakshmi Bai
> > <lakshmi.bai.raja.subramanian@intel.com>;
> > Sangannavar, Mallikarjunappa <mallikarjunappa.sangannavar@intel.com>
> > Subject: Re: [PATCH v8 1/2] dt-bindings: timer: Add bindings for Intel
> > Keem Bay SoC Timer
> >
> > On Mon, Mar 07, 2022 at 04:33:23PM -0600, Rob Herring wrote:
> > > On Wed, Feb 23, 2022 at 5:31 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > On Tue, Feb 22, 2022 at 05:13:41PM -0600, Rob Herring wrote:
> > > > > On Tue, Feb 22, 2022 at 03:26:53PM +0530,
> > > > > shruthi.sanil@intel.com
> > wrote:
> > > > > > From: Shruthi Sanil <shruthi.sanil@intel.com>
> > > > > >
> > > > > > Add Device Tree bindings for the Timer IP, which can be used
> > > > > > as clocksource and clockevent device in the Intel Keem Bay SoC.
> > > >
> > > > ...
> > > >
> > > > > > +    soc {
> > > > > > +        #address-cells = <0x2>;
> > > > > > +        #size-cells = <0x2>;
> > > > > > +
> > > > > > +        gpt@20331000 {
> > > > > > +            compatible = "intel,keembay-gpt-creg",
> > > > > > + "simple-mfd";
> > > > >
> > > > > It looks like you are splitting things based on Linux
> > > > > implementation details. Does this h/w block have different
> > > > > combinations of timers and counters? If not, then you don't need
> > > > > the child nodes at all. There's plenty of h/w blocks that get
> > > > > used as both
> > a clocksource and clockevent.
> > > > >
> > > > > Maybe I already raised this, but assume I don't remember and
> > > > > this patch needs to address any questions I already asked.
> > > >
> > > > I dunno if I mentioned that hardware seems to have 5 or so devices
> > > > behind the block, so ideally it should be one device node that
> > > > represents the global register spaces and several children nodes.
> > >
> > > Is it 5 devices or 9 devices?
> >
> > 5 devices, one of which is a timer block out of 8 timers.
> > You may count them as 12 altogether.
> >
> > > > However, I am not familiar with the established practices in DT
> > > > world, but above seems to me the right thing to do since it
> > > > describes the hardware as is (without any linuxisms).
> > >
> > > The Linuxism in these cases defining 1 node per driver because
> > > that's what is convenient for automatic probing. That appears to be
> > > exactly the case here. The red flag is nodes with a compatible and
> > > nothing else. The next question is whether the sub-devices are
> > > blocks that will be assembled in varying combinations and
> > > quantities. If not, then not much point subdividing the h/w blocks.
> >
> > AFAIU the hardware architecture the amount of timers is dependent on
> > the IP synthesis configuration. On this platform it's 8, but it may be
> > 1 or 2, for example.
> 
> Yes, the number of timers can vary between platforms.
> For eg., Intel Keem Bay SoC has 8 timers where as in Intel Thunder Bay SoC
> has 6 timers.
> 
> >
> > > There's also many cases of having multiple 'identical' timers and
> > > wanting to encode which timer gets assigned to clocksource vs.
> > > clockevent. But those 'identical' timers aren't if you care about
> > > which timer gets assigned where. I *think* that's not the case here
> > > unless you are trying to pick the timer for the clockevent by not
> > > defining the other timers.
> > >
> > > Without having a complete picture of what's in 'gpt-creg', I can't
> > > give better advice.
> >
> > I guess they need to share TRM, if possible, to show what this block is.
> >
> 
> I would like to explain briefly about the Timer IP in the Keem Bay Soc.
> The Timers block contains 8 general purpose timers, a free running counter.
> Each general purpose timer can generate an individual interrupt to the
> interrupt controller.
> The timer block consists of secure and non-secure timers. Hence there are
> secure and non-secure registers in separate address banks.
> The secure register bank consists of the common control register where the
> timers and counters need to be enabled.
> From the driver we try to check if these bits are enabled to continue with the
> initialization of the driver.
> Hence we need to pass the base address of both the address banks to the
> driver from the DTB.
> The control register is common for both timer and counter. Hence we went
> for parent child module in DTB. 'gpt-creg' represents this control register.
> 

I tried to get the technical manual that could be shared publicly, but couldn't find one which has the timer IP details.
In the previous reply above, I have tried to explain the details of the Timer IP. Could you please let me know if I was able to answer your question?

I think the concern point here is the common register that needs to be accessed by the timer during initialization. Same address cannot be defined in multiple timer nodes and also we cannot have this in the driver too as offset because this offset is changing from platform to platform. Hence we have gone with the parent child approach. I need your help here on whats the best possible way to do this?

I need your support in taking this patch forward?

> > --
> > With Best Regards,
> > Andy Shevchenko
> >