Re: [PATCH 2/2] dt-bindings: timer: convert rockchip,rk-timer.txt to YAML

[Ezequiel Garcia <ezequiel@collabora.com>]

On Thu, 2021-04-29 at 11:25 -0500, Rob Herring wrote:
> On Sat, Apr 24, 2021 at 04:19:46PM -0300, Ezequiel Garcia wrote:
> > Convert Rockchip Timer dt-bindings to YAML.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  .../bindings/timer/rockchip,rk-timer.txt      | 27 --------
> >  .../bindings/timer/rockchip,rk-timer.yaml     | 67 +++++++++++++++++++
> >  2 files changed, 67 insertions(+), 27 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
> >  create mode 100644 Documentation/devicetree/bindings/timer/rockchip,rk-timer.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt b/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
> > deleted file mode 100644
> > index d65fdce7c7f0..000000000000
> > --- a/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
> > +++ /dev/null
> > @@ -1,27 +0,0 @@
> > -Rockchip rk timer
> > -
> > -Required properties:
> > -- compatible: should be:
> > -  "rockchip,rv1108-timer", "rockchip,rk3288-timer": for Rockchip RV1108
> > -  "rockchip,rk3036-timer", "rockchip,rk3288-timer": for Rockchip RK3036
> > -  "rockchip,rk3066-timer", "rockchip,rk3288-timer": for Rockchip RK3066
> > -  "rockchip,rk3188-timer", "rockchip,rk3288-timer": for Rockchip RK3188
> > -  "rockchip,rk3228-timer", "rockchip,rk3288-timer": for Rockchip RK3228
> > -  "rockchip,rk3229-timer", "rockchip,rk3288-timer": for Rockchip RK3229
> > -  "rockchip,rk3288-timer": for Rockchip RK3288
> > -  "rockchip,rk3368-timer", "rockchip,rk3288-timer": for Rockchip RK3368
> > -  "rockchip,rk3399-timer": for Rockchip RK3399
> > -- reg: base address of the timer register starting with TIMERS CONTROL register
> > -- interrupts: should contain the interrupts for Timer0
> > -- clocks : must contain an entry for each entry in clock-names
> > -- clock-names : must include the following entries:
> > -  "timer", "pclk"
> > -
> > -Example:
> > -       timer: timer@ff810000 {
> > -               compatible = "rockchip,rk3288-timer";
> > -               reg = <0xff810000 0x20>;
> > -               interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
> > -               clocks = <&xin24m>, <&cru PCLK_TIMER>;
> > -               clock-names = "timer", "pclk";
> > -       };
> > diff --git a/Documentation/devicetree/bindings/timer/rockchip,rk-timer.yaml b/Documentation/devicetree/bindings/timer/rockchip,rk-timer.yaml
> > new file mode 100644
> > index 000000000000..f1bc3ac7abc8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/timer/rockchip,rk-timer.yaml
> > @@ -0,0 +1,67 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/timer/rockchip,rk-timer.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Rockchip Timer Device Tree Bindings
> > +
> > +maintainers:
> > +  - Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> This should be someone that knows the h/w and cares about Rockchip.
> 

Daniel wrote the driver, so I figured he'd care :)
If not, perhaps Heiko (if he agrees)?

> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - const: rockchip,rk3288-timer
> > +      - const: rockchip,rk3399-timer
> > +      - items:
> > +          - enum:
> > +            - rockchip,rv1108-timer
> > +            - rockchip,rk3036-timer
> > +            - rockchip,rk3066-timer
> > +            - rockchip,rk3188-timer
> > +            - rockchip,rk3228-timer
> > +            - rockchip,rk3229-timer
> > +            - rockchip,rk3288-timer
> > +            - rockchip,rk3368-timer
> > +            - rockchip,px30-timer
> > +          - const: rockchip,rk3288-timer
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    minItems: 2
> > +    maxItems: 2
> > +
> > +  clock-names:
> > +    items:
> > +      enum:
> > +        - timer
> > +        - pclk
> 
> We can't define the order here? We should fix dts files if they are 
> inconsistent.
> 

The driver requests the clocks by name, and unfortunately DTS
rely on that. We can change all the DTSI, but wouldn't that
be too much trouble for something that is currently working fine?

Why is the order important?

> > +    minItems: 2
> > +    maxItems: 2
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +  - clock-names
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/clock/rk3288-cru.h>
> > +
> > +    timer: timer@ff810000 {
> > +        compatible = "rockchip,rk3288-timer";
> > +        reg = <0xff810000 0x20>;
> > +        interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
> > +        clocks = <&xin24m>, <&cru PCLK_TIMER>;
> > +        clock-names = "timer", "pclk";
> > +    };
> > -- 
> > 2.30.0
> >