diff mbox series

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

Message ID 2938028520edbd0140805a22bdacd0b30c45b2df.1606377035.git.vijayakannan.ayyathurai@intel.com
State Changes Requested
Headers show
Series Add drivers for Intel Keem Bay SoC timer block | expand

Checks

Context Check Description
robh/dt-meta-schema success
robh/checkpatch success

Commit Message

Ayyathurai, Vijayakannan Nov. 26, 2020, 10:34 a.m. UTC
From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>

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

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

Comments

Rob Herring Dec. 8, 2020, 4:12 p.m. UTC | #1
On Thu, Nov 26, 2020 at 06:34:08PM +0800, vijayakannan.ayyathurai@intel.com wrote:
> From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> 
> Add Device Tree bindings for the Timer IP, which used as clocksource and
> clockevent in the Intel Keem Bay SoC.
> 
> Signed-off-by: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> Acked-by: Mark Gross <mgross@linux.intel.com>
> Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  .../bindings/timer/intel,keembay-timer.yaml   | 72 +++++++++++++++++++
>  1 file changed, 72 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..396a698967ca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/intel,keembay-timer.yaml
> @@ -0,0 +1,72 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/timer/intel,keembay-timer.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Intel Keem Bay SoC Timers
> +
> +maintainers:
> +  - Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> +
> +description:
> +  Intel Keem Bay SoC Timers block contains 8 32-bit general purpose timers,
> +  a free running 64-bit counter, a random number generator and a watchdog
> +  timer. Each gpt can generate an individual interrupt.
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          enum:
> +            - intel,keembay-timer
> +            - intel,keembay-counter
> +
> +  reg:
> +    maxItems: 2
> +
> +  clocks:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - intel,keembay-timer
> +    then:
> +      properties:
> +        interrupts:
> +          maxItems: 1
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #define KEEM_BAY_A53_TIM
> +
> +    timer@20330010 {
> +        compatible = "intel,keembay-timer";
> +        reg = <0x20330010 0xc>,
> +              <0x20331000 0xc>;
> +        interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> +        clocks = <&scmi_clk KEEM_BAY_A53_TIM>;
> +    };
> +
> +    counter@203300e8 {
> +        compatible = "intel,keembay-counter";
> +        reg = <0x203300e8 0xc>,
> +              <0x20331000 0xc>;

You have overlapping reg regions here. Don't do that. Define the DT 
in terms of the h/w, not how you want to split things for Linux.

It looks like a single h/w block providing multiple functions.

> +        clocks = <&scmi_clk KEEM_BAY_A53_TIM>;
> +    };
> -- 
> 2.17.1
>
Andy Shevchenko Dec. 8, 2020, 6:12 p.m. UTC | #2
On Tue, Dec 08, 2020 at 10:12:47AM -0600, Rob Herring wrote:
> On Thu, Nov 26, 2020 at 06:34:08PM +0800, vijayakannan.ayyathurai@intel.com wrote:
> > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> > 
> > Add Device Tree bindings for the Timer IP, which used as clocksource and
> > clockevent in the Intel Keem Bay SoC.

...

> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #define KEEM_BAY_A53_TIM
> > +
> > +    timer@20330010 {
> > +        compatible = "intel,keembay-timer";
> > +        reg = <0x20330010 0xc>,
> > +              <0x20331000 0xc>;
> > +        interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> > +        clocks = <&scmi_clk KEEM_BAY_A53_TIM>;
> > +    };
> > +
> > +    counter@203300e8 {
> > +        compatible = "intel,keembay-counter";
> > +        reg = <0x203300e8 0xc>,
> > +              <0x20331000 0xc>;
> 
> You have overlapping reg regions here. Don't do that. Define the DT 
> in terms of the h/w, not how you want to split things for Linux.
> 
> It looks like a single h/w block providing multiple functions.

Actually a good catch.

Perhaps it needs to have a parent device that provides three resources (one
common and one per each of two functions) and in the driver it should consume
them accordingly. Though I'm not an expert in DT, does it sound correct from
your perspective?

> > +        clocks = <&scmi_clk KEEM_BAY_A53_TIM>;
> > +    };
Ayyathurai, Vijayakannan Dec. 16, 2020, 7:16 p.m. UTC | #3
Hi Rob,

> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> On Tue, Dec 08, 2020 at 10:12:47AM -0600, Rob Herring wrote:
> > > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> > >
> > > Add Device Tree bindings for the Timer IP, which used as clocksource and
> > > clockevent in the Intel Keem Bay SoC.
> 
> ...
> 
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > +    #define KEEM_BAY_A53_TIM
> > > +
> > > +    timer@20330010 {
> > > +        compatible = "intel,keembay-timer";
> > > +        reg = <0x20330010 0xc>,
> > > +              <0x20331000 0xc>;
> > > +        interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> > > +        clocks = <&scmi_clk KEEM_BAY_A53_TIM>;
> > > +    };
> > > +
> > > +    counter@203300e8 {
> > > +        compatible = "intel,keembay-counter";
> > > +        reg = <0x203300e8 0xc>,
> > > +              <0x20331000 0xc>;
> >
> > You have overlapping reg regions here. Don't do that. Define the DT
> > in terms of the h/w, not how you want to split things for Linux.
> >
> > It looks like a single h/w block providing multiple functions.
> 
> Actually a good catch.
> 
> Perhaps it needs to have a parent device that provides three resources (one
> common and one per each of two functions) and in the driver it should
> consume
> them accordingly. Though I'm not an expert in DT, does it sound correct from
> your perspective?
> 

May I know your feedback for the below way, which Andy suggested.
I will adapt the driver based on this in the next version.

timer@20331000 {
    compatible = "intel,keembay-timer";
    reg = <20331000 0xc>;
    
    gpt@20330010 {
        compatible = "intel,keembay-gpt";
        reg = <20330010 0xc>;
        interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
        clocks = <&scmi_clk KEEM_BAY_A53_TIM>;
    };

    counter@203300e8 {
        compatible = "intel,keembay-counter";
        reg = <203300e8 0xc>;
        clocks = <&scmi_clk KEEM_BAY_A53_TIM>;
    };
};

Thanks,
Vijay
diff mbox series

Patch

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