Re: [PATCH 1/2] dt-bindings: timer: Update TI timer to yaml

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 12/04/2022 15:16, Tony Lindgren wrote:
> Let's update the TI timer binding to use yaml. As this binding is specific
> to the TI dual-mode timers also known as dm-timers, let's use file name
> ti,timer-dm.yaml to avoid confusion with other timers.
> 
> We also correct the issue with the old binding that was out of date for
> several properties.
> 
> The am43 related timers are undocumented, but compatible with the am3
> timers. Let's add the am43 timers too.
> 
> The dm814 and dm816 timers are missing, let's add them.
> 
> Some timers on some SoCs are dual mapped, like the ABE timers on omap4
> and 5. The reg property maxItems must be updated to 2.
> 
> The timer clocks can be managed by the parent interconnect target module
> with no clocks assigned for the timer node. And in some cases the SoC may
> need to configure additional clocks for the timer in addition to the
> functional clock.
> 
> The clock names are optional and not specific to the comptible property.
> For example, dra7 timers on l3 interconnect do not need clock-names,while
> the timers on dra7 l4 interconnect need them with both being compatible
> with ti,omap5430-timer.
> 
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Keerthy <j-keerthy@ti.com>
> Cc: Nishanth Menon <nm@ti.com>
> Cc: Vignesh Raghavendra <vigneshr@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  .../bindings/pwm/pwm-omap-dmtimer.txt         |   2 +-
>  .../bindings/timer/ti,timer-dm.yaml           | 161 ++++++++++++++++++
>  .../devicetree/bindings/timer/ti,timer.txt    |  44 -----
>  3 files changed, 162 insertions(+), 45 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/timer/ti,timer-dm.yaml
>  delete mode 100644 Documentation/devicetree/bindings/timer/ti,timer.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-omap-dmtimer.txt b/Documentation/devicetree/bindings/pwm/pwm-omap-dmtimer.txt
> --- a/Documentation/devicetree/bindings/pwm/pwm-omap-dmtimer.txt
> +++ b/Documentation/devicetree/bindings/pwm/pwm-omap-dmtimer.txt
> @@ -2,7 +2,7 @@
>  
>  Required properties:
>  - compatible: Shall contain "ti,omap-dmtimer-pwm".
> -- ti,timers: phandle to PWM capable OMAP timer. See timer/ti,timer.txt for info
> +- ti,timers: phandle to PWM capable OMAP timer. See timer/ti,timer-dm.yaml for info
>    about these timers.
>  - #pwm-cells: Should be 3. See pwm.yaml in this directory for a description of
>    the cells format.
> diff --git a/Documentation/devicetree/bindings/timer/ti,timer-dm.yaml b/Documentation/devicetree/bindings/timer/ti,timer-dm.yaml
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/ti,timer-dm.yaml
> @@ -0,0 +1,161 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/timer/ti,timer-dm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TI dual-mode timer
> +
> +maintainers:
> +  - Tony Lindgren <tony@atomide.com>
> +
> +description: |
> +  The TI dual-mode timer is a general purpose timer with PWM capabilities.
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - ti,am335x-timer
> +              - ti,am335x-timer-1ms
> +              - ti,dm814-timer
> +              - ti,dm816-timer
> +              - ti,omap2420-timer
> +              - ti,omap3430-timer
> +              - ti,omap4430-timer
> +              - ti,omap5430-timer
> +      - items:
> +          - const: ti,am4372-timer
> +          - const: ti,am335x-timer
> +      - items:
> +          - const: ti,am4372-timer-1ms
> +          - const: ti,am335x-timer-1ms
> +
> +  reg:
> +    minItems: 1
> +    maxItems: 2
> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 2
> +
> +  clock-names:
> +    minItems: 1
> +    maxItems: 2
> +
> +  interrupts:
> +    description:
> +      Interrupt if available. The timer PWM features may be usable
> +      in a limited way even without interrupts.
> +    maxItems: 1
> +
> +  ti,timer-alwon:
> +    description:
> +      Timer is always enabled when the SoC is powered. Note that some SoCs like
> +      am335x can suspend to PM coprocessor RTC only mode and in that case the
> +      SoC power is cut including timers.
> +    type: boolean
> +
> +  ti,timer-dsp:
> +    description:
> +      Timer is routable to the DSP in addition to the operating system.
> +    type: boolean
> +
> +  ti,timer-pwm:
> +    description:
> +      Timer has been wired for PWM capability.
> +    type: boolean
> +
> +  ti,timer-secure:
> +    description:
> +      Timer access has been limited to secure mode only.
> +    type: boolean
> +
> +  ti,hwmods:
> +    description:
> +      Name of the HWMOD associated with timer. This is for legacy
> +      omap2/3 platforms only.
> +    $ref: /schemas/types.yaml#/definitions/string
> +    deprecated: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - ti,omap3430-timer
> +              - ti,omap4430-timer
> +              - ti,omap5430-timer
> +    then:
> +      properties:
> +        reg:
> +          oneOf:
> +            - items:
> +                - description: IO address
> +            - items:
> +                - description: CPU to L4 ABE IO address
> +                - description: L3 to L4 ABE IO address
> +        clocks:
> +          oneOf:
> +            - items:
> +                - description: Functional clock
> +            - items:
> +                - description: Functional clock
> +                - description: System clock

I think this can be made simpler, without oneOf:

  clocks:
    items:
      - description: Functional clock
      - description: System clock
    minItems: 1


and similar for clock-names.

> +        clock-names:
> +          oneOf:
> +            - items:
> +                - const: fck
> +            - items:
> +                - const: fck
> +                - const: timer_sys_ck
> +    else:
> +      properties:
> +        reg:
> +          items:
> +            - description: IO address
> +        clocks:
> +          items:
> +            - description: Functional clock
> +        clock-names:
> +          items:
> +            - const: fck

Hmmm, in your previous version I had impression that first clock is not
the same on every flavor. Now it looks the same, so clocks you could
simplify even more:
1. Define full list in main properties (not in allOf) with minItems:1
2. Use maxItems:1, for this "else:" case.

This could greatly reduce amount of code you need and keep the actual
definition of clocks in main properties (not in allOf; allOf are only to
constrain it).

Best regards,
Krzysztof