| Message ID | 20210424191946.69978-1-ezequiel@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] arm64: dts: rockchip: add timer0 clocks on rk3368 | expand |
On Sat, 24 Apr 2021 16:19:46 -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 > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: ./Documentation/devicetree/bindings/timer/rockchip,rk-timer.yaml:19:13: [warning] wrong indentation: expected 14 but found 12 (indentation) dtschema/dtc warnings/errors: See https://patchwork.ozlabs.org/patch/1469995 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
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. > + > +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. > + 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 >
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 > >
On Thu, Apr 29, 2021 at 12:03 PM Ezequiel Garcia <ezequiel@collabora.com> wrote: > > 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 :) Ah, then that's fine I guess. Given he is also the subsystem maintainer I was confused. > 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. That's good, then the OS doesn't care if you change it. Of course, I didn't have to look to tell that. If the order varied, the OS would have to use the names. > We can change all the DTSI, but wouldn't that > be too much trouble for something that is currently working fine? It's not *all*. At least half are correct. Pick the order that's the majority. > Why is the order important? Because that's the DT convention. Why is random order important? I don't really care so much on an individual binding, but overall if defining the order doesn't cost anything then we should. The more order is not defined, the more people are going to copy those examples. Yes, there's a cost on fixing dts files, but if we're writing schema to pass on existing dts files, what's the point of schema? Rob
diff --git a/arch/arm64/boot/dts/rockchip/rk3368.dtsi b/arch/arm64/boot/dts/rockchip/rk3368.dtsi index 242f821a90ba..548be81c9d3f 100644 --- a/arch/arm64/boot/dts/rockchip/rk3368.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3368.dtsi @@ -668,6 +668,8 @@ timer0: timer@ff810000 { compatible = "rockchip,rk3368-timer", "rockchip,rk3288-timer"; reg = <0x0 0xff810000 0x0 0x20>; interrupts = <GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&cru PCLK_TIMER0>, <&cru SCLK_TIMER00>; + clock-names = "pclk", "timer"; }; spdif: spdif@ff880000 {
The timer driver requires pclk and sclk clocks to be present in the device tree node, so add them. Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- arch/arm64/boot/dts/rockchip/rk3368.dtsi | 2 ++ 1 file changed, 2 insertions(+)