Re: [PATCH v2] dt-bindings: display: Convert etnaviv to json-schema

[Rob Herring <robh+dt@kernel.org>]

On Tue, Jan 28, 2020 at 1:58 PM Benjamin GAIGNARD
<benjamin.gaignard@st.com> wrote:
>
>
> On 1/28/20 8:35 PM, Rob Herring wrote:
> > On Tue, Jan 28, 2020 at 6:31 AM Benjamin GAIGNARD
> > <benjamin.gaignard@st.com> wrote:
> >>
> >> On 1/28/20 1:06 PM, Maxime Ripard wrote:
> >>> Hi Benjamin,
> >>>
> >>> On Tue, Jan 28, 2020 at 09:20:13AM +0100, Benjamin Gaignard wrote:
> >>>> Convert etnaviv bindings to yaml format.
> >>>>
> >>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> >>>> ---
> >>>>    .../bindings/display/etnaviv/etnaviv-drm.txt       | 36 -----------
> >>>>    .../devicetree/bindings/gpu/vivante,gc.yaml        | 72 ++++++++++++++++++++++
> >>>>    2 files changed, 72 insertions(+), 36 deletions(-)
> >>>>    delete mode 100644 Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt
> >>>>    create mode 100644 Documentation/devicetree/bindings/gpu/vivante,gc.yaml
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt b/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt
> >>>> deleted file mode 100644
> >>>> index 8def11b16a24..000000000000
> >>>> --- a/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt
> >>>> +++ /dev/null
> >>>> @@ -1,36 +0,0 @@
> >>>> -Vivante GPU core devices
> >>>> -========================
> >>>> -
> >>>> -Required properties:
> >>>> -- compatible: Should be "vivante,gc"
> >>>> -  A more specific compatible is not needed, as the cores contain chip
> >>>> -  identification registers at fixed locations, which provide all the
> >>>> -  necessary information to the driver.
> >>>> -- reg: should be register base and length as documented in the
> >>>> -  datasheet
> >>>> -- interrupts: Should contain the cores interrupt line
> >>>> -- clocks: should contain one clock for entry in clock-names
> >>>> -  see Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>> -- clock-names:
> >>>> -   - "bus":    AXI/master interface clock
> >>>> -   - "reg":    AHB/slave interface clock
> >>>> -               (only required if GPU can gate slave interface independently)
> >>>> -   - "core":   GPU core clock
> >>>> -   - "shader": Shader clock (only required if GPU has feature PIPE_3D)
> >>>> -
> >>>> -Optional properties:
> >>>> -- power-domains: a power domain consumer specifier according to
> >>>> -  Documentation/devicetree/bindings/power/power_domain.txt
> >>>> -
> >>>> -example:
> >>>> -
> >>>> -gpu_3d: gpu@130000 {
> >>>> -    compatible = "vivante,gc";
> >>>> -    reg = <0x00130000 0x4000>;
> >>>> -    interrupts = <0 9 IRQ_TYPE_LEVEL_HIGH>;
> >>>> -    clocks = <&clks IMX6QDL_CLK_GPU3D_AXI>,
> >>>> -             <&clks IMX6QDL_CLK_GPU3D_CORE>,
> >>>> -             <&clks IMX6QDL_CLK_GPU3D_SHADER>;
> >>>> -    clock-names = "bus", "core", "shader";
> >>>> -    power-domains = <&gpc 1>;
> >>>> -};
> >>>> diff --git a/Documentation/devicetree/bindings/gpu/vivante,gc.yaml b/Documentation/devicetree/bindings/gpu/vivante,gc.yaml
> >>>> new file mode 100644
> >>>> index 000000000000..c4f549c0d750
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/gpu/vivante,gc.yaml
> >>>> @@ -0,0 +1,72 @@
> >>>> +# SPDX-License-Identifier: GPL-2.0
> >>>> +%YAML 1.2
> >>>> +---
> >>>> +$id: http://devicetree.org/schemas/gpu/vivante,gc.yaml#
> >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>> +
> >>>> +title: Vivante GPU Bindings
> >>>> +
> >>>> +description: Vivante GPU core devices
> >>>> +
> >>>> +maintainers:
> >>>> +  -  Lucas Stach <l.stach@pengutronix.de>
> >>>> +
> >>>> +properties:
> >>>> +  compatible:
> >>>> +    const: vivante,gc
> >>>> +
> >>>> +  reg:
> >>>> +    maxItems: 1
> >>>> +
> >>>> +  interrupts:
> >>>> +    maxItems: 1
> >>>> +
> >>>> +  clocks:
> >>>> +    items:
> >>>> +      - description: AXI/master interface clock
> >>>> +      - description: GPU core clock
> >>>> +      - description: Shader clock (only required if GPU has feature PIPE_3D)
> >>>> +      - description: AHB/slave interface clock (only required if GPU can gate slave interface independently)
> >>> Can you have an AHB slave interface clock without a shader clock?
> >> No because the items in the list are ordered so you need to have, in
> >> order: "bus", "core", "shader", "reg"
> >>
> >> If it is needed to allow any number of clock in any order I could write
> >> it like this:
> > Yes, but I prefer we don't allow any order if we don't have to. Did
> > you run this schema against dtbs_check or just audit the dts files
> > with vivante?
>
> Both, I found these mix of reg-names:
>
> "core"
>
> "bus","core"
>
> "bus","core","shader"

You missed a couple:

arch/arc/boot/dts/hsdk.dts-                     clock-names = "bus",
"reg", "core", "shader";
arch/arm/boot/dts/dove.dtsi-                            clock-names = "core";
arch/arm/boot/dts/imx6q.dtsi-                   clock-names = "bus", "core";
arch/arm/boot/dts/imx6qdl.dtsi-                 clock-names = "bus",
"core", "shader";
arch/arm/boot/dts/imx6qdl.dtsi-                 clock-names = "bus", "core";
arch/arm/boot/dts/imx6sl.dtsi-                  clock-names = "bus", "core";
arch/arm/boot/dts/imx6sl.dtsi-                  clock-names = "bus", "core";
arch/arm/boot/dts/imx6sx.dtsi-                  clock-names = "bus",
"core", "shader";
arch/arm/boot/dts/stm32mp157c.dtsi-                     clock-names =
"bus" ,"core";
arch/arm64/boot/dts/freescale/imx8mq.dtsi-
clock-names = "core", "shader", "bus", "reg";

imx8mq is probably new enough to change if we wanted to.

I guess just do an enum...

Rob