diff mbox series

[v4,3/5] dt-bindings: display: ti, j721e-dss: Add dt-schema yaml binding

Message ID 89db418c91689beb6e63e0c3c99b39655948b429.1576704528.git.jsarha@ti.com (mailing list archive)
State New, archived
Headers show
Series drm/tidss: New driver for TI Keystone platform Display SubSystem | expand

Commit Message

Jyri Sarha Dec. 19, 2019, 8:23 a.m. UTC
Add dt-schema yaml bindig for J721E DSS, J721E version TI Keystone
Display SubSystem.

Version history:

v2: no change

v3: - reg-names: "wp" -> "wb"
    - Add ports node
    - Add includes to dts example
    - reindent dts example

v4: - Add descriptions to reg, clocks, and interrups properties
    - Remove minItems when its value is the same as maxItems value

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 .../bindings/display/ti/ti,j721e-dss.yaml     | 209 ++++++++++++++++++
 1 file changed, 209 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml

Comments

Maxime Ripard Dec. 19, 2019, 8:38 a.m. UTC | #1
Hi,

On Thu, Dec 19, 2019 at 10:23:17AM +0200, Jyri Sarha wrote:
> Add dt-schema yaml bindig for J721E DSS, J721E version TI Keystone
> Display SubSystem.
>
> Version history:
>
> v2: no change
>
> v3: - reg-names: "wp" -> "wb"
>     - Add ports node
>     - Add includes to dts example
>     - reindent dts example
>
> v4: - Add descriptions to reg, clocks, and interrups properties
>     - Remove minItems when its value is the same as maxItems value
>
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  .../bindings/display/ti/ti,j721e-dss.yaml     | 209 ++++++++++++++++++
>  1 file changed, 209 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
> new file mode 100644
> index 000000000000..cd68c4294f9a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
> @@ -0,0 +1,209 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2019 Texas Instruments Incorporated
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/display/ti/ti,j721e-dss.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Texas Instruments J721E Display Subsystem
> +
> +maintainers:
> +  - Jyri Sarha <jsarha@ti.com>
> +  - Tomi Valkeinen <tomi.valkeinen@ti.com>
> +
> +description: |
> +  The J721E TI Keystone Display SubSystem with four output ports and
> +  four video planes. There is two full video planes and two "lite
> +  planes" without scaling support. The video ports can be connected to
> +  the SoC's DPI pins or to integrated display bridges on the SoC.
> +
> +properties:
> +  compatible:
> +    const: ti,j721e-dss
> +
> +  reg:
> +    maxItems: 17
> +    description: |
> +      Addresses to each DSS memory region described in the SoC's TRM.
> +      The reg-names refer to memory regions as follows:
> +      reg-names: Region Name in TRM:     Description:
> +      common_m   DSS0_DISPC_0_COMMON_M   DSS Master common register area
> +      common_s0  DSS0_DISPC_0_COMMON_SO  DSS Shared common register area 0
> +      common_s1  DSS0_DISPC_0_COMMON_S1  DSS Shared common register area 1
> +      common_s2  DSS0_DISPC_0_COMMON_S2  DSS Shared common register area 2
> +      vidl1      DSS0_VIDL1              VIDL1 light video plane 1
> +      vidl2      DSS0_VIDL2              VIDL2 light video plane 2
> +      vid1       DSS0_VID1               VID1 video plane 1
> +      vid2       DSS0_VID2               VID1 video plane 2
> +      ovr1       DSS0_OVR1               OVR1 overlay manager for vp1
> +      ovr2       DSS0_OVR2               OVR2 overlay manager for vp2
> +      ovr3       DSS0_OVR3               OVR1 overlay manager for vp3
> +      ovr4       DSS0_OVR4               OVR2 overlay manager for vp4
> +      vp1        DSS0_VP1                VP1 video port 1
> +      vp2        DSS0_VP2                VP1 video port 2
> +      vp3        DSS0_VP3                VP1 video port 3
> +      vp4        DSS0_VP4                VP1 video port 4
> +      wp         DSS0_WB                 Write Back registers

I guess it applies to all your schemas in that patch series, but you
could just do something like

reg:
  items:
    - description: DSS Master common register area
    - description: DSS Shared common register area 0
    - description: DSS Shared common register area 1

...

That way, you wouldn't have to worry about the maxItems, and you end
up doing pretty much that already in the description

> +  reg-names:
> +    items:
> +      - const: common_m
> +      - const: common_s0
> +      - const: common_s1
> +      - const: common_s2
> +      - const: vidl1
> +      - const: vidl2
> +      - const: vid1
> +      - const: vid2
> +      - const: ovr1
> +      - const: ovr2
> +      - const: ovr3
> +      - const: ovr4
> +      - const: vp1
> +      - const: vp2
> +      - const: vp3
> +      - const: vp4
> +      - const: wb
> +
> +  clocks:
> +    maxItems: 5
> +    description:
> +      phandles to clock nodes for DSS functional clock (fck) and video
> +      port 1, 2, 3 and 4 pixel clocks (vp1, vp2, vp3, vp4).
> +
> +  clock-names:
> +    items:
> +      - const: fck
> +      - const: vp1
> +      - const: vp2
> +      - const: vp3
> +      - const: vp4
> +
> +  interrupts:
> +    maxItems: 4
> +    description:
> +      Interrupt descriptions for common irq registers in common_m,
> +      common_m0, common_m1, and common_m2, sections.

Same story here, but the names don't match interrupt-names. I guess
describing what those interrupts actually are would be great: you just
define how the driver calls them, but not what they are actually doing
or representing.

I'm guessing that would end up in something like that:

interrupts:
  items:
    - description: DSS Master interrupt
    - description: DSS Shared 0 interrupt
    - description: DSS Shared 1 interrupt
    - description: DSS Shared 2 interrupt

Maxime
Peter Ujfalusi Dec. 19, 2019, 1:39 p.m. UTC | #2
Hi Jyri,

On 19/12/2019 10.23, Jyri Sarha wrote:
> Add dt-schema yaml bindig for J721E DSS, J721E version TI Keystone
> Display SubSystem.
> 
> Version history:
> 
> v2: no change
> 
> v3: - reg-names: "wp" -> "wb"
>     - Add ports node
>     - Add includes to dts example
>     - reindent dts example
> 
> v4: - Add descriptions to reg, clocks, and interrups properties
>     - Remove minItems when its value is the same as maxItems value
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  .../bindings/display/ti/ti,j721e-dss.yaml     | 209 ++++++++++++++++++
>  1 file changed, 209 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
> new file mode 100644
> index 000000000000..cd68c4294f9a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
> @@ -0,0 +1,209 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2019 Texas Instruments Incorporated
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/display/ti/ti,j721e-dss.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Texas Instruments J721E Display Subsystem
> +
> +maintainers:
> +  - Jyri Sarha <jsarha@ti.com>
> +  - Tomi Valkeinen <tomi.valkeinen@ti.com>
> +
> +description: |
> +  The J721E TI Keystone Display SubSystem with four output ports and
> +  four video planes. There is two full video planes and two "lite
> +  planes" without scaling support. The video ports can be connected to
> +  the SoC's DPI pins or to integrated display bridges on the SoC.
> +
> +properties:
> +  compatible:
> +    const: ti,j721e-dss
> +
> +  reg:
> +    maxItems: 17
> +    description: |
> +      Addresses to each DSS memory region described in the SoC's TRM.
> +      The reg-names refer to memory regions as follows:
> +      reg-names: Region Name in TRM:     Description:
> +      common_m   DSS0_DISPC_0_COMMON_M   DSS Master common register area
> +      common_s0  DSS0_DISPC_0_COMMON_SO  DSS Shared common register area 0
> +      common_s1  DSS0_DISPC_0_COMMON_S1  DSS Shared common register area 1
> +      common_s2  DSS0_DISPC_0_COMMON_S2  DSS Shared common register area 2
> +      vidl1      DSS0_VIDL1              VIDL1 light video plane 1
> +      vidl2      DSS0_VIDL2              VIDL2 light video plane 2
> +      vid1       DSS0_VID1               VID1 video plane 1
> +      vid2       DSS0_VID2               VID1 video plane 2
> +      ovr1       DSS0_OVR1               OVR1 overlay manager for vp1
> +      ovr2       DSS0_OVR2               OVR2 overlay manager for vp2
> +      ovr3       DSS0_OVR3               OVR1 overlay manager for vp3
> +      ovr4       DSS0_OVR4               OVR2 overlay manager for vp4
> +      vp1        DSS0_VP1                VP1 video port 1
> +      vp2        DSS0_VP2                VP1 video port 2
> +      vp3        DSS0_VP3                VP1 video port 3
> +      vp4        DSS0_VP4                VP1 video port 4
> +      wp         DSS0_WB                 Write Back registers

'wp' is back ;)

> +
> +  reg-names:
> +    items:
> +      - const: common_m
> +      - const: common_s0
> +      - const: common_s1
> +      - const: common_s2
> +      - const: vidl1
> +      - const: vidl2
> +      - const: vid1
> +      - const: vid2
> +      - const: ovr1
> +      - const: ovr2
> +      - const: ovr3
> +      - const: ovr4
> +      - const: vp1
> +      - const: vp2
> +      - const: vp3
> +      - const: vp4
> +      - const: wb

imho the description you put under the 'reg' would be more natural here,
where you actually specifying the names of the register ranges.

> +
> +  clocks:
> +    maxItems: 5
> +    description:
> +      phandles to clock nodes for DSS functional clock (fck) and video
> +      port 1, 2, 3 and 4 pixel clocks (vp1, vp2, vp3, vp4).
> +
> +  clock-names:
> +    items:
> +      - const: fck
> +      - const: vp1
> +      - const: vp2
> +      - const: vp3
> +      - const: vp4

Same comment, it is more natural to have the description coupled with
what you are describing.

> +
> +  interrupts:
> +    maxItems: 4
> +    description:
> +      Interrupt descriptions for common irq registers in common_m,
> +      common_m0, common_m1, and common_m2, sections.
> +
> +  interrupt-names:
> +    items:
> +      - const: common_m
> +      - const: common_s0
> +      - const: common_s1
> +      - const: common_s2
> +
> +  power-domains:
> +    maxItems: 1
> +    description: phandle to the associated power domain
> +
> +  ports:
> +    type: object
> +    description:
> +      Ports as described in Documentation/devictree/bindings/graph.txt
> +    properties:
> +      "#address-cells":
> +        const: 1
> +
> +      "#size-cells":
> +        const: 0
> +
> +      port@0:
> +        type: object
> +        description:
> +          The output port node form video port 1
> +
> +      port@1:
> +        type: object
> +        description:
> +          The output port node from video port 2
> +
> +      port@2:
> +        type: object
> +        description:
> +          The output port node from video port 3
> +
> +      port@3:
> +        type: object
> +        description:
> +          The output port node from video port 4
> +
> +    required:
> +      - "#address-cells"
> +      - "#size-cells"
> +
> +  max-memory-bandwidth:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Input memory (from main memory to dispc) bandwidth limit in
> +      bytes per second
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - clocks
> +  - clock-names
> +  - interrupts
> +  - interrupt-names
> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/soc/ti,sci_pm_domain.h>
> +
> +    dss: dss@04a00000 {
> +            compatible = "ti,j721e-dss";
> +            reg =   <0x00 0x04a00000 0x00 0x10000>, /* common_m */
> +                    <0x00 0x04a10000 0x00 0x10000>, /* common_s0*/
> +                    <0x00 0x04b00000 0x00 0x10000>, /* common_s1*/
> +                    <0x00 0x04b10000 0x00 0x10000>, /* common_s2*/
> +                    <0x00 0x04a20000 0x00 0x10000>, /* vidl1 */
> +                    <0x00 0x04a30000 0x00 0x10000>, /* vidl2 */
> +                    <0x00 0x04a50000 0x00 0x10000>, /* vid1 */
> +                    <0x00 0x04a60000 0x00 0x10000>, /* vid2 */
> +                    <0x00 0x04a70000 0x00 0x10000>, /* ovr1 */
> +                    <0x00 0x04a90000 0x00 0x10000>, /* ovr2 */
> +                    <0x00 0x04ab0000 0x00 0x10000>, /* ovr3 */
> +                    <0x00 0x04ad0000 0x00 0x10000>, /* ovr4 */
> +                    <0x00 0x04a80000 0x00 0x10000>, /* vp1 */
> +                    <0x00 0x04aa0000 0x00 0x10000>, /* vp2 */
> +                    <0x00 0x04ac0000 0x00 0x10000>, /* vp3 */
> +                    <0x00 0x04ae0000 0x00 0x10000>, /* vp4 */
> +                    <0x00 0x04af0000 0x00 0x10000>; /* wb */
> +            reg-names = "common_m", "common_s0",
> +                    "common_s1", "common_s2",
> +                    "vidl1", "vidl2","vid1","vid2",
> +                    "ovr1", "ovr2", "ovr3", "ovr4",
> +                    "vp1", "vp2", "vp3", "vp4",
> +                    "wb";
> +            clocks =        <&k3_clks 152 0>,
> +                            <&k3_clks 152 1>,
> +                            <&k3_clks 152 4>,
> +                            <&k3_clks 152 9>,
> +                            <&k3_clks 152 13>;
> +            clock-names = "fck", "vp1", "vp2", "vp3", "vp4";
> +            power-domains = <&k3_pds 152 TI_SCI_PD_EXCLUSIVE>;
> +            interrupts =    <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>,
> +                            <GIC_SPI 603 IRQ_TYPE_LEVEL_HIGH>,
> +                            <GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>,
> +                            <GIC_SPI 605 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names =       "common_m",
> +                                    "common_s0",
> +                                    "common_s1",
> +                                    "common_s2";
> +            ports {
> +                    #address-cells = <1>;
> +                    #size-cells = <0>;
> +                    port@0 {
> +                            reg = <0>;
> +
> +                            dpi_out_0: endpoint {
> +                                    remote-endpoint = <&dp_bridge_input>;
> +                            };
> +                    };
> +            };
> +    };
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Jyri Sarha Dec. 19, 2019, 2:01 p.m. UTC | #3
On 19/12/2019 10:38, Maxime Ripard wrote:
> Hi,
> 
> On Thu, Dec 19, 2019 at 10:23:17AM +0200, Jyri Sarha wrote:
>> Add dt-schema yaml bindig for J721E DSS, J721E version TI Keystone
>> Display SubSystem.
>>
>> Version history:
>>
>> v2: no change
>>
>> v3: - reg-names: "wp" -> "wb"
>>     - Add ports node
>>     - Add includes to dts example
>>     - reindent dts example
>>
>> v4: - Add descriptions to reg, clocks, and interrups properties
>>     - Remove minItems when its value is the same as maxItems value
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> ---
>>  .../bindings/display/ti/ti,j721e-dss.yaml     | 209 ++++++++++++++++++
>>  1 file changed, 209 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
>> new file mode 100644
>> index 000000000000..cd68c4294f9a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
>> @@ -0,0 +1,209 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +# Copyright 2019 Texas Instruments Incorporated
>> +%YAML 1.2
>> +---
>> +$id: "http://devicetree.org/schemas/display/ti/ti,j721e-dss.yaml#"
>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>> +
>> +title: Texas Instruments J721E Display Subsystem
>> +
>> +maintainers:
>> +  - Jyri Sarha <jsarha@ti.com>
>> +  - Tomi Valkeinen <tomi.valkeinen@ti.com>
>> +
>> +description: |
>> +  The J721E TI Keystone Display SubSystem with four output ports and
>> +  four video planes. There is two full video planes and two "lite
>> +  planes" without scaling support. The video ports can be connected to
>> +  the SoC's DPI pins or to integrated display bridges on the SoC.
>> +
>> +properties:
>> +  compatible:
>> +    const: ti,j721e-dss
>> +
>> +  reg:
>> +    maxItems: 17
>> +    description: |
>> +      Addresses to each DSS memory region described in the SoC's TRM.
>> +      The reg-names refer to memory regions as follows:
>> +      reg-names: Region Name in TRM:     Description:
>> +      common_m   DSS0_DISPC_0_COMMON_M   DSS Master common register area
>> +      common_s0  DSS0_DISPC_0_COMMON_SO  DSS Shared common register area 0
>> +      common_s1  DSS0_DISPC_0_COMMON_S1  DSS Shared common register area 1
>> +      common_s2  DSS0_DISPC_0_COMMON_S2  DSS Shared common register area 2
>> +      vidl1      DSS0_VIDL1              VIDL1 light video plane 1
>> +      vidl2      DSS0_VIDL2              VIDL2 light video plane 2
>> +      vid1       DSS0_VID1               VID1 video plane 1
>> +      vid2       DSS0_VID2               VID1 video plane 2
>> +      ovr1       DSS0_OVR1               OVR1 overlay manager for vp1
>> +      ovr2       DSS0_OVR2               OVR2 overlay manager for vp2
>> +      ovr3       DSS0_OVR3               OVR1 overlay manager for vp3
>> +      ovr4       DSS0_OVR4               OVR2 overlay manager for vp4
>> +      vp1        DSS0_VP1                VP1 video port 1
>> +      vp2        DSS0_VP2                VP1 video port 2
>> +      vp3        DSS0_VP3                VP1 video port 3
>> +      vp4        DSS0_VP4                VP1 video port 4
>> +      wp         DSS0_WB                 Write Back registers
> 
> I guess it applies to all your schemas in that patch series, but you
> could just do something like
> 
> reg:
>   items:
>     - description: DSS Master common register area
>     - description: DSS Shared common register area 0
>     - description: DSS Shared common register area 1
> 

Ok, thanks. I was not sure if you can do that (still a newbie with
yaml). What do you think about Peter Ujfalusi's suggestion of putting
the descriptions to reg-names (and clock-names and  interrupt-names)?
e.g. something like this:

  reg-names:
    items:
      - const: common_m
      - description: DSS Master common register area
      - const: common_s0
      - description: DSS Master common register area
...

Or is that even allowed?


> ...
> 
> That way, you wouldn't have to worry about the maxItems, and you end
> up doing pretty much that already in the description
> 
>> +  reg-names:
>> +    items:
>> +      - const: common_m
>> +      - const: common_s0
>> +      - const: common_s1
>> +      - const: common_s2
>> +      - const: vidl1
>> +      - const: vidl2
>> +      - const: vid1
>> +      - const: vid2
>> +      - const: ovr1
>> +      - const: ovr2
>> +      - const: ovr3
>> +      - const: ovr4
>> +      - const: vp1
>> +      - const: vp2
>> +      - const: vp3
>> +      - const: vp4
>> +      - const: wb
>> +
>> +  clocks:
>> +    maxItems: 5
>> +    description:
>> +      phandles to clock nodes for DSS functional clock (fck) and video
>> +      port 1, 2, 3 and 4 pixel clocks (vp1, vp2, vp3, vp4).
>> +
>> +  clock-names:
>> +    items:
>> +      - const: fck
>> +      - const: vp1
>> +      - const: vp2
>> +      - const: vp3
>> +      - const: vp4
>> +
>> +  interrupts:
>> +    maxItems: 4
>> +    description:
>> +      Interrupt descriptions for common irq registers in common_m,
>> +      common_m0, common_m1, and common_m2, sections.
> 
> Same story here, but the names don't match interrupt-names. I guess
> describing what those interrupts actually are would be great: you just
> define how the driver calls them, but not what they are actually doing
> or representing.
> 
> I'm guessing that would end up in something like that:
> 
> interrupts:
>   items:
>     - description: DSS Master interrupt
>     - description: DSS Shared 0 interrupt
>     - description: DSS Shared 1 interrupt
>     - description: DSS Shared 2 interrupt
> 
> Maxime
>
Jyri Sarha Dec. 19, 2019, 7:45 p.m. UTC | #4
On 19/12/2019 16:01, Jyri Sarha wrote:
> On 19/12/2019 10:38, Maxime Ripard wrote:
>> Hi,
>>
>> On Thu, Dec 19, 2019 at 10:23:17AM +0200, Jyri Sarha wrote:
>>> Add dt-schema yaml bindig for J721E DSS, J721E version TI Keystone
>>> Display SubSystem.
>>>
>>> Version history:
>>>
>>> v2: no change
>>>
>>> v3: - reg-names: "wp" -> "wb"
>>>     - Add ports node
>>>     - Add includes to dts example
>>>     - reindent dts example
>>>
>>> v4: - Add descriptions to reg, clocks, and interrups properties
>>>     - Remove minItems when its value is the same as maxItems value
>>>
>>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>>> ---
>>>  .../bindings/display/ti/ti,j721e-dss.yaml     | 209 ++++++++++++++++++
>>>  1 file changed, 209 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
>>> new file mode 100644
>>> index 000000000000..cd68c4294f9a
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
>>> @@ -0,0 +1,209 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +# Copyright 2019 Texas Instruments Incorporated
>>> +%YAML 1.2
>>> +---
>>> +$id: "http://devicetree.org/schemas/display/ti/ti,j721e-dss.yaml#"
>>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>>> +
>>> +title: Texas Instruments J721E Display Subsystem
>>> +
>>> +maintainers:
>>> +  - Jyri Sarha <jsarha@ti.com>
>>> +  - Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> +
>>> +description: |
>>> +  The J721E TI Keystone Display SubSystem with four output ports and
>>> +  four video planes. There is two full video planes and two "lite
>>> +  planes" without scaling support. The video ports can be connected to
>>> +  the SoC's DPI pins or to integrated display bridges on the SoC.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: ti,j721e-dss
>>> +
>>> +  reg:
>>> +    maxItems: 17
>>> +    description: |
>>> +      Addresses to each DSS memory region described in the SoC's TRM.
>>> +      The reg-names refer to memory regions as follows:
>>> +      reg-names: Region Name in TRM:     Description:
>>> +      common_m   DSS0_DISPC_0_COMMON_M   DSS Master common register area
>>> +      common_s0  DSS0_DISPC_0_COMMON_SO  DSS Shared common register area 0
>>> +      common_s1  DSS0_DISPC_0_COMMON_S1  DSS Shared common register area 1
>>> +      common_s2  DSS0_DISPC_0_COMMON_S2  DSS Shared common register area 2
>>> +      vidl1      DSS0_VIDL1              VIDL1 light video plane 1
>>> +      vidl2      DSS0_VIDL2              VIDL2 light video plane 2
>>> +      vid1       DSS0_VID1               VID1 video plane 1
>>> +      vid2       DSS0_VID2               VID1 video plane 2
>>> +      ovr1       DSS0_OVR1               OVR1 overlay manager for vp1
>>> +      ovr2       DSS0_OVR2               OVR2 overlay manager for vp2
>>> +      ovr3       DSS0_OVR3               OVR1 overlay manager for vp3
>>> +      ovr4       DSS0_OVR4               OVR2 overlay manager for vp4
>>> +      vp1        DSS0_VP1                VP1 video port 1
>>> +      vp2        DSS0_VP2                VP1 video port 2
>>> +      vp3        DSS0_VP3                VP1 video port 3
>>> +      vp4        DSS0_VP4                VP1 video port 4
>>> +      wp         DSS0_WB                 Write Back registers
>> I guess it applies to all your schemas in that patch series, but you
>> could just do something like
>>
>> reg:
>>   items:
>>     - description: DSS Master common register area
>>     - description: DSS Shared common register area 0
>>     - description: DSS Shared common register area 1
>>
> Ok, thanks. I was not sure if you can do that (still a newbie with
> yaml). What do you think about Peter Ujfalusi's suggestion of putting
> the descriptions to reg-names (and clock-names and  interrupt-names)?
> e.g. something like this:
> 
>   reg-names:
>     items:
>       - const: common_m
>       - description: DSS Master common register area
>       - const: common_s0
>       - description: DSS Master common register area
> ...
> 
> Or is that even allowed?
> 

Hmmm, it looks like make dt_binding_check does not like that. I guess I
go with your suggestion, but with reg-name reference in the description.

Best regards,
Jyri
Maxime Ripard Dec. 20, 2019, 7:45 a.m. UTC | #5
Hi,

On Thu, Dec 19, 2019 at 04:01:58PM +0200, Jyri Sarha wrote:
> On 19/12/2019 10:38, Maxime Ripard wrote:
> > On Thu, Dec 19, 2019 at 10:23:17AM +0200, Jyri Sarha wrote:
> >> Add dt-schema yaml bindig for J721E DSS, J721E version TI Keystone
> >> Display SubSystem.
> >>
> >> Version history:
> >>
> >> v2: no change
> >>
> >> v3: - reg-names: "wp" -> "wb"
> >>     - Add ports node
> >>     - Add includes to dts example
> >>     - reindent dts example
> >>
> >> v4: - Add descriptions to reg, clocks, and interrups properties
> >>     - Remove minItems when its value is the same as maxItems value
> >>
> >> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> >> ---
> >>  .../bindings/display/ti/ti,j721e-dss.yaml     | 209 ++++++++++++++++++
> >>  1 file changed, 209 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
> >> new file mode 100644
> >> index 000000000000..cd68c4294f9a
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
> >> @@ -0,0 +1,209 @@
> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >> +# Copyright 2019 Texas Instruments Incorporated
> >> +%YAML 1.2
> >> +---
> >> +$id: "http://devicetree.org/schemas/display/ti/ti,j721e-dss.yaml#"
> >> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> >> +
> >> +title: Texas Instruments J721E Display Subsystem
> >> +
> >> +maintainers:
> >> +  - Jyri Sarha <jsarha@ti.com>
> >> +  - Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> +
> >> +description: |
> >> +  The J721E TI Keystone Display SubSystem with four output ports and
> >> +  four video planes. There is two full video planes and two "lite
> >> +  planes" without scaling support. The video ports can be connected to
> >> +  the SoC's DPI pins or to integrated display bridges on the SoC.
> >> +
> >> +properties:
> >> +  compatible:
> >> +    const: ti,j721e-dss
> >> +
> >> +  reg:
> >> +    maxItems: 17
> >> +    description: |
> >> +      Addresses to each DSS memory region described in the SoC's TRM.
> >> +      The reg-names refer to memory regions as follows:
> >> +      reg-names: Region Name in TRM:     Description:
> >> +      common_m   DSS0_DISPC_0_COMMON_M   DSS Master common register area
> >> +      common_s0  DSS0_DISPC_0_COMMON_SO  DSS Shared common register area 0
> >> +      common_s1  DSS0_DISPC_0_COMMON_S1  DSS Shared common register area 1
> >> +      common_s2  DSS0_DISPC_0_COMMON_S2  DSS Shared common register area 2
> >> +      vidl1      DSS0_VIDL1              VIDL1 light video plane 1
> >> +      vidl2      DSS0_VIDL2              VIDL2 light video plane 2
> >> +      vid1       DSS0_VID1               VID1 video plane 1
> >> +      vid2       DSS0_VID2               VID1 video plane 2
> >> +      ovr1       DSS0_OVR1               OVR1 overlay manager for vp1
> >> +      ovr2       DSS0_OVR2               OVR2 overlay manager for vp2
> >> +      ovr3       DSS0_OVR3               OVR1 overlay manager for vp3
> >> +      ovr4       DSS0_OVR4               OVR2 overlay manager for vp4
> >> +      vp1        DSS0_VP1                VP1 video port 1
> >> +      vp2        DSS0_VP2                VP1 video port 2
> >> +      vp3        DSS0_VP3                VP1 video port 3
> >> +      vp4        DSS0_VP4                VP1 video port 4
> >> +      wp         DSS0_WB                 Write Back registers
> >
> > I guess it applies to all your schemas in that patch series, but you
> > could just do something like
> >
> > reg:
> >   items:
> >     - description: DSS Master common register area
> >     - description: DSS Shared common register area 0
> >     - description: DSS Shared common register area 1
> >
>
> Ok, thanks. I was not sure if you can do that (still a newbie with
> yaml). What do you think about Peter Ujfalusi's suggestion of putting
> the descriptions to reg-names (and clock-names and  interrupt-names)?
> e.g. something like this:
>
>   reg-names:
>     items:
>       - const: common_m
>       - description: DSS Master common register area
>       - const: common_s0
>       - description: DSS Master common register area

No, you can't really intertwin them like this, this would mean that
you would expect twice the number of items.

You could do something like this:
reg-names:
  items:
    - const: common_m
      description: DSS Master common register area
    - const: common_s0
      description: DSS Master common register area

But on a more fundamental level, the register area is stored under
regs, while reg-names is just a label for that area, so having the
description under reg, and the labels under reg-names makes more sense
(well, to me at least :))

Maxiem
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
new file mode 100644
index 000000000000..cd68c4294f9a
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
@@ -0,0 +1,209 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2019 Texas Instruments Incorporated
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/display/ti/ti,j721e-dss.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Texas Instruments J721E Display Subsystem
+
+maintainers:
+  - Jyri Sarha <jsarha@ti.com>
+  - Tomi Valkeinen <tomi.valkeinen@ti.com>
+
+description: |
+  The J721E TI Keystone Display SubSystem with four output ports and
+  four video planes. There is two full video planes and two "lite
+  planes" without scaling support. The video ports can be connected to
+  the SoC's DPI pins or to integrated display bridges on the SoC.
+
+properties:
+  compatible:
+    const: ti,j721e-dss
+
+  reg:
+    maxItems: 17
+    description: |
+      Addresses to each DSS memory region described in the SoC's TRM.
+      The reg-names refer to memory regions as follows:
+      reg-names: Region Name in TRM:     Description:
+      common_m   DSS0_DISPC_0_COMMON_M   DSS Master common register area
+      common_s0  DSS0_DISPC_0_COMMON_SO  DSS Shared common register area 0
+      common_s1  DSS0_DISPC_0_COMMON_S1  DSS Shared common register area 1
+      common_s2  DSS0_DISPC_0_COMMON_S2  DSS Shared common register area 2
+      vidl1      DSS0_VIDL1              VIDL1 light video plane 1
+      vidl2      DSS0_VIDL2              VIDL2 light video plane 2
+      vid1       DSS0_VID1               VID1 video plane 1
+      vid2       DSS0_VID2               VID1 video plane 2
+      ovr1       DSS0_OVR1               OVR1 overlay manager for vp1
+      ovr2       DSS0_OVR2               OVR2 overlay manager for vp2
+      ovr3       DSS0_OVR3               OVR1 overlay manager for vp3
+      ovr4       DSS0_OVR4               OVR2 overlay manager for vp4
+      vp1        DSS0_VP1                VP1 video port 1
+      vp2        DSS0_VP2                VP1 video port 2
+      vp3        DSS0_VP3                VP1 video port 3
+      vp4        DSS0_VP4                VP1 video port 4
+      wp         DSS0_WB                 Write Back registers
+
+  reg-names:
+    items:
+      - const: common_m
+      - const: common_s0
+      - const: common_s1
+      - const: common_s2
+      - const: vidl1
+      - const: vidl2
+      - const: vid1
+      - const: vid2
+      - const: ovr1
+      - const: ovr2
+      - const: ovr3
+      - const: ovr4
+      - const: vp1
+      - const: vp2
+      - const: vp3
+      - const: vp4
+      - const: wb
+
+  clocks:
+    maxItems: 5
+    description:
+      phandles to clock nodes for DSS functional clock (fck) and video
+      port 1, 2, 3 and 4 pixel clocks (vp1, vp2, vp3, vp4).
+
+  clock-names:
+    items:
+      - const: fck
+      - const: vp1
+      - const: vp2
+      - const: vp3
+      - const: vp4
+
+  interrupts:
+    maxItems: 4
+    description:
+      Interrupt descriptions for common irq registers in common_m,
+      common_m0, common_m1, and common_m2, sections.
+
+  interrupt-names:
+    items:
+      - const: common_m
+      - const: common_s0
+      - const: common_s1
+      - const: common_s2
+
+  power-domains:
+    maxItems: 1
+    description: phandle to the associated power domain
+
+  ports:
+    type: object
+    description:
+      Ports as described in Documentation/devictree/bindings/graph.txt
+    properties:
+      "#address-cells":
+        const: 1
+
+      "#size-cells":
+        const: 0
+
+      port@0:
+        type: object
+        description:
+          The output port node form video port 1
+
+      port@1:
+        type: object
+        description:
+          The output port node from video port 2
+
+      port@2:
+        type: object
+        description:
+          The output port node from video port 3
+
+      port@3:
+        type: object
+        description:
+          The output port node from video port 4
+
+    required:
+      - "#address-cells"
+      - "#size-cells"
+
+  max-memory-bandwidth:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Input memory (from main memory to dispc) bandwidth limit in
+      bytes per second
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - clocks
+  - clock-names
+  - interrupts
+  - interrupt-names
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/soc/ti,sci_pm_domain.h>
+
+    dss: dss@04a00000 {
+            compatible = "ti,j721e-dss";
+            reg =   <0x00 0x04a00000 0x00 0x10000>, /* common_m */
+                    <0x00 0x04a10000 0x00 0x10000>, /* common_s0*/
+                    <0x00 0x04b00000 0x00 0x10000>, /* common_s1*/
+                    <0x00 0x04b10000 0x00 0x10000>, /* common_s2*/
+                    <0x00 0x04a20000 0x00 0x10000>, /* vidl1 */
+                    <0x00 0x04a30000 0x00 0x10000>, /* vidl2 */
+                    <0x00 0x04a50000 0x00 0x10000>, /* vid1 */
+                    <0x00 0x04a60000 0x00 0x10000>, /* vid2 */
+                    <0x00 0x04a70000 0x00 0x10000>, /* ovr1 */
+                    <0x00 0x04a90000 0x00 0x10000>, /* ovr2 */
+                    <0x00 0x04ab0000 0x00 0x10000>, /* ovr3 */
+                    <0x00 0x04ad0000 0x00 0x10000>, /* ovr4 */
+                    <0x00 0x04a80000 0x00 0x10000>, /* vp1 */
+                    <0x00 0x04aa0000 0x00 0x10000>, /* vp2 */
+                    <0x00 0x04ac0000 0x00 0x10000>, /* vp3 */
+                    <0x00 0x04ae0000 0x00 0x10000>, /* vp4 */
+                    <0x00 0x04af0000 0x00 0x10000>; /* wb */
+            reg-names = "common_m", "common_s0",
+                    "common_s1", "common_s2",
+                    "vidl1", "vidl2","vid1","vid2",
+                    "ovr1", "ovr2", "ovr3", "ovr4",
+                    "vp1", "vp2", "vp3", "vp4",
+                    "wb";
+            clocks =        <&k3_clks 152 0>,
+                            <&k3_clks 152 1>,
+                            <&k3_clks 152 4>,
+                            <&k3_clks 152 9>,
+                            <&k3_clks 152 13>;
+            clock-names = "fck", "vp1", "vp2", "vp3", "vp4";
+            power-domains = <&k3_pds 152 TI_SCI_PD_EXCLUSIVE>;
+            interrupts =    <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>,
+                            <GIC_SPI 603 IRQ_TYPE_LEVEL_HIGH>,
+                            <GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>,
+                            <GIC_SPI 605 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names =       "common_m",
+                                    "common_s0",
+                                    "common_s1",
+                                    "common_s2";
+            ports {
+                    #address-cells = <1>;
+                    #size-cells = <0>;
+                    port@0 {
+                            reg = <0>;
+
+                            dpi_out_0: endpoint {
+                                    remote-endpoint = <&dp_bridge_input>;
+                            };
+                    };
+            };
+    };