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

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

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
>

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>;
> > +    };

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