Re: [PATCH v15 2/4] dt-bindings: msm: dsi: add yaml schemas for DSI bindings

[mkrishn@codeaurora.org]

On 2021-04-08 20:33, Rob Herring wrote:
> On Mon, Apr 05, 2021 at 04:36:08PM +0530, Krishna Manikandan wrote:
>> Add YAML schema for the device tree bindings for DSI
>> 
>> Signed-off-by: Krishna Manikandan <mkrishn@codeaurora.org>
>> 
>> Changes in v1:
>>     - Separate dsi controller bindings to a separate patch (Stephen 
>> Boyd)
>>     - Merge dsi-common-controller.yaml and dsi-controller-main.yaml to
>>       a single file (Stephen Boyd)
>>     - Drop supply entries and definitions from properties (Stephen 
>> Boyd)
>>     - Modify phy-names property for dsi controller (Stephen Boyd)
>>     - Remove boolean from description (Stephen Boyd)
>>     - Drop pinctrl properties as they are standard entries (Stephen 
>> Boyd)
>>     - Modify the description for ports property and keep the reference
>>       to the generic binding where this is defined (Stephen Boyd)
>>     - Add description to clock names (Stephen Boyd)
>>     - Correct the indendation (Stephen Boyd)
>>     - Drop the label for display dt nodes and correct the node
>>       name (Stephen Boyd)
>> 
>> Changes in v2:
>>     - Drop maxItems for clock (Stephen Boyd)
>>     - Drop qcom,mdss-mdp-transfer-time-us as it is not used in 
>> upstream
>>       dt file (Stephen Boyd)
>>     - Keep child node directly under soc node (Stephen Boyd)
>>     - Drop qcom,sync-dual-dsi as it is not used in upstream dt
>> 
>> Changes in v3:
>>     - Add description for register property (Stephen Boyd)
>> 
>> Changes in v4:
>>     - Add maxItems for phys property (Stephen Boyd)
>>     - Add maxItems for reg property (Stephen Boyd)
>>     - Add reference for data-lanes property (Stephen Boyd)
>>     - Remove soc from example (Stephen Boyd)
>> 
>> Changes in v5:
>>     - Modify title and description (Stephen Boyd)
>>     - Add required properties for ports node (Stephen Boyd)
>>     - Add data-lanes in the example (Stephen Boyd)
>>     - Drop qcom,master-dsi property (Stephen Boyd)
>> 
>> Changes in v6:
>>     - Add required properties for port@0, port@1 and corresponding
>>       endpoints (Stephen Boyd)
>>     - Add address-cells and size-cells for ports (Stephen Boyd)
>>     - Use additionalProperties instead of unevaluatedProperties 
>> (Stephen Boyd)
>> ---
>>  .../bindings/display/msm/dsi-controller-main.yaml  | 213 
>> ++++++++++++++++++
>>  .../devicetree/bindings/display/msm/dsi.txt        | 249 
>> ---------------------
>>  2 files changed, 213 insertions(+), 249 deletions(-)
>>  create mode 100644 
>> Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
>>  delete mode 100644 
>> Documentation/devicetree/bindings/display/msm/dsi.txt
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml 
>> b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
>> new file mode 100644
>> index 0000000..7858524
>> --- /dev/null
>> +++ 
>> b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
>> @@ -0,0 +1,213 @@
>> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: 
>> http://devicetree.org/schemas/display/msm/dsi-controller-main.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Display DSI controller
>> +
>> +maintainers:
>> +  - Krishna Manikandan <mkrishn@codeaurora.org>
>> +
>> +allOf:
>> +  - $ref: "../dsi-controller.yaml#"
>> +
>> +properties:
>> +  compatible:
>> +    items:
>> +      - const: qcom,mdss-dsi-ctrl
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  reg-names:
>> +    const: dsi_ctrl
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    items:
>> +      - description: Display byte clock
>> +      - description: Display byte interface clock
>> +      - description: Display pixel clock
>> +      - description: Display escape clock
>> +      - description: Display AHB clock
>> +      - description: Display AXI clock
>> +
>> +  clock-names:
>> +    items:
>> +      - const: byte
>> +      - const: byte_intf
>> +      - const: pixel
>> +      - const: core
>> +      - const: iface
>> +      - const: bus
>> +
>> +  phys:
>> +    maxItems: 1
>> +
>> +  phy-names:
>> +    const: dsi
>> +
>> +  "#address-cells": true
>> +
>> +  "#size-cells": true
>> +
>> +  syscon-sfpb:
>> +    description: A phandle to mmss_sfpb syscon node (only for DSIv2).
>> +    $ref: "/schemas/types.yaml#/definitions/phandle"
>> +
>> +  qcom,dual-dsi-mode:
>> +    type: boolean
>> +    description: |
>> +      Indicates if the DSI controller is driving a panel which needs
>> +      2 DSI links.
>> +
>> +  ports:
> 
> Same issues in this one.
> 
>> +    $ref: "/schemas/graph.yaml#/properties/port"
>> +    type: object
>> +    description: |
>> +      Contains DSI controller input and output ports as children, 
>> each
>> +      containing one endpoint subnode.
>> +
>> +    properties:
>> +      port@0:
>> +        type: object
>> +        description: |
>> +          Input endpoints of the controller.
>> +
>> +        properties:
>> +          reg:
>> +            const: 0
>> +
>> +          endpoint:
>> +            type: object
>> +            properties:
>> +              remote-endpoint:
> 
> Don't need to describe this, the common schema does.
> 
>> +                description: |
>> +                  For port@1, set to phandle of the connected 
>> panel/bridge's
>> +                  input endpoint. For port@0, set to the MDP 
>> interface output.
>> +
>> +              data-lanes:
>> +                $ref: "/schemas/media/video-interfaces.yaml#"
> 
> Not how this reference works. Look at other examples.
> 
>> +                description: |
>> +                  This describes how the physical DSI data lanes are 
>> mapped
>> +                  to the logical lanes on the given platform. The 
>> value contained in
>> +                  index n describes what physical lane is mapped to 
>> the logical lane n
>> +                  (DATAn, where n lies between 0 and 3). The clock 
>> lane position is fixed
>> +                  and can't be changed. Hence, they aren't a part of 
>> the DT bindings.
>> +
>> +                items:
>> +                  - const: 0
>> +                  - const: 1
>> +                  - const: 2
>> +                  - const: 3
> 
> If this is the only possible value, why does it need to be in DT?
Hi Rob,
These are the possible values:
-    <0 1 2 3>
-    <1 2 3 0>
-    <2 3 0 1>
-    <3 0 1 2>
-    <0 3 2 1>
-    <1 0 3 2>
-    <2 1 0 3>
-    <3 2 1 0>

Shall I follow the below mentioned approach for defining these values ?
oneOf:
   - items:
     - const: 0
     - const: 1
     - const: 2
     - const: 3
   - items:
     - const: 1
     - const: 2
     - const: 3
     - const: 0
   - items:
     - const: 2
     - const: 3
     - const: 0
     - const: 1
   - items:
     - const: 3
     - const: 0
     - const: 1
     - const: 2
   - items:
     - const: 0
     - const: 3
     - const: 2
     - const: 1
   - items:
     - const: 1
     - const: 0
     - const: 3
     - const: 2
   - items:
     - const: 2
     - const: 1
     - const: 0
     - const: 3
   - items:
     - const: 3
     - const: 2
     - const: 1
     - const: 0

Thanks,
Krishna
> 
>> +
>> +            required:
>> +              - remote-endpoint
>> +
>> +        required:
>> +          - reg
>> +          - endpoint
>> +
>> +      port@1:
>> +        type: object
>> +        description: |
>> +          Output endpoints of the controller.
>> +        properties:
>> +          reg:
>> +            const: 1
>> +
>> +          endpoint:
>> +            type: object
>> +            properties:
>> +              remote-endpoint: true
>> +              data-lanes:
>> +                items:
>> +                  - const: 0
>> +                  - const: 1
>> +                  - const: 2
>> +                  - const: 3
>> +
>> +            required:
>> +              - remote-endpoint
>> +              - data-lanes
>> +
>> +        required:
>> +          - reg
>> +          - endpoint
>> +
>> +    required:
>> +      - port@0
>> +      - port@1
>> +      - "#address-cells"
>> +      - "#size-cells"
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - reg-names
>> +  - interrupts
>> +  - clocks
>> +  - clock-names
>> +  - phys
>> +  - phy-names
>> +  - ports
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +     #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +     #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
>> +     #include <dt-bindings/clock/qcom,gcc-sdm845.h>
>> +
>> +     dsi@ae94000 {
>> +           compatible = "qcom,mdss-dsi-ctrl";
>> +           reg = <0x0ae94000 0x400>;
>> +           reg-names = "dsi_ctrl";
>> +
>> +           #address-cells = <1>;
>> +           #size-cells = <0>;
>> +
>> +           interrupt-parent = <&mdss>;
>> +           interrupts = <4>;
>> +
>> +           clocks = <&dispcc DISP_CC_MDSS_BYTE0_CLK>,
>> +                    <&dispcc DISP_CC_MDSS_BYTE0_INTF_CLK>,
>> +                    <&dispcc DISP_CC_MDSS_PCLK0_CLK>,
>> +                    <&dispcc DISP_CC_MDSS_ESC0_CLK>,
>> +                    <&dispcc DISP_CC_MDSS_AHB_CLK>,
>> +                    <&dispcc DISP_CC_MDSS_AXI_CLK>;
>> +           clock-names = "byte",
>> +                         "byte_intf",
>> +                         "pixel",
>> +                         "core",
>> +                         "iface",
>> +                         "bus";
>> +
>> +           phys = <&dsi0_phy>;
>> +           phy-names = "dsi";
>> +
>> +           ports {
>> +                  #address-cells = <1>;
>> +                  #size-cells = <0>;
>> +
>> +                  port@0 {
>> +                          reg = <0>;
>> +                          dsi0_in: endpoint {
>> +                                   remote-endpoint = 
>> <&dpu_intf1_out>;
>> +                          };
>> +                  };
>> +
>> +                  port@1 {
>> +                          reg = <1>;
>> +                          dsi0_out: endpoint {
>> +                                   remote-endpoint = <&sn65dsi86_in>;
>> +                                   data-lanes = <0 1 2 3>;
>> +                          };
>> +                  };
>> +           };
>> +     };
>> +...
>> diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt 
>> b/Documentation/devicetree/bindings/display/msm/dsi.txt
>> deleted file mode 100644
>> index b9a64d3..0000000
>> --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
>> +++ /dev/null
>> @@ -1,249 +0,0 @@
>> -Qualcomm Technologies Inc. adreno/snapdragon DSI output
>> -
>> -DSI Controller:
>> -Required properties:
>> -- compatible:
>> -  * "qcom,mdss-dsi-ctrl"
>> -- reg: Physical base address and length of the registers of 
>> controller
>> -- reg-names: The names of register regions. The following regions are 
>> required:
>> -  * "dsi_ctrl"
>> -- interrupts: The interrupt signal from the DSI block.
>> -- power-domains: Should be <&mmcc MDSS_GDSC>.
>> -- clocks: Phandles to device clocks.
>> -- clock-names: the following clocks are required:
>> -  * "mdp_core"
>> -  * "iface"
>> -  * "bus"
>> -  * "core_mmss"
>> -  * "byte"
>> -  * "pixel"
>> -  * "core"
>> -  For DSIv2, we need an additional clock:
>> -   * "src"
>> -  For DSI6G v2.0 onwards, we need also need the clock:
>> -   * "byte_intf"
>> -- assigned-clocks: Parents of "byte" and "pixel" for the given 
>> platform.
>> -- assigned-clock-parents: The Byte clock and Pixel clock PLL outputs 
>> provided
>> -  by a DSI PHY block. See [1] for details on clock bindings.
>> -- vdd-supply: phandle to vdd regulator device node
>> -- vddio-supply: phandle to vdd-io regulator device node
>> -- vdda-supply: phandle to vdda regulator device node
>> -- phys: phandle to DSI PHY device node
>> -- phy-names: the name of the corresponding PHY device
>> -- syscon-sfpb: A phandle to mmss_sfpb syscon node (only for DSIv2)
>> -- ports: Contains 2 DSI controller ports as child nodes. Each port 
>> contains
>> -  an endpoint subnode as defined in [2] and [3].
>> -
>> -Optional properties:
>> -- panel@0: Node of panel connected to this DSI controller.
>> -  See files in [4] for each supported panel.
>> -- qcom,dual-dsi-mode: Boolean value indicating if the DSI controller 
>> is
>> -  driving a panel which needs 2 DSI links.
>> -- qcom,master-dsi: Boolean value indicating if the DSI controller is 
>> driving
>> -  the master link of the 2-DSI panel.
>> -- qcom,sync-dual-dsi: Boolean value indicating if the DSI controller 
>> is
>> -  driving a 2-DSI panel whose 2 links need receive command 
>> simultaneously.
>> -- pinctrl-names: the pin control state names; should contain 
>> "default"
>> -- pinctrl-0: the default pinctrl state (active)
>> -- pinctrl-n: the "sleep" pinctrl state
>> -- ports: contains DSI controller input and output ports as children, 
>> each
>> -  containing one endpoint subnode.
>> -
>> -  DSI Endpoint properties:
>> -  - remote-endpoint: For port@0, set to phandle of the connected 
>> panel/bridge's
>> -    input endpoint. For port@1, set to the MDP interface output. See 
>> [2] for
>> -    device graph info.
>> -
>> -  - data-lanes: this describes how the physical DSI data lanes are 
>> mapped
>> -    to the logical lanes on the given platform. The value contained 
>> in
>> -    index n describes what physical lane is mapped to the logical 
>> lane n
>> -    (DATAn, where n lies between 0 and 3). The clock lane position is 
>> fixed
>> -    and can't be changed. Hence, they aren't a part of the DT 
>> bindings. See
>> -    [3] for more info on the data-lanes property.
>> -
>> -    For example:
>> -
>> -    data-lanes = <3 0 1 2>;
>> -
>> -    The above mapping describes that the logical data lane DATA0 is 
>> mapped to
>> -    the physical data lane DATA3, logical DATA1 to physical DATA0, 
>> logic DATA2
>> -    to phys DATA1 and logic DATA3 to phys DATA2.
>> -
>> -    There are only a limited number of physical to logical mappings 
>> possible:
>> -    <0 1 2 3>
>> -    <1 2 3 0>
>> -    <2 3 0 1>
>> -    <3 0 1 2>
>> -    <0 3 2 1>
>> -    <1 0 3 2>
>> -    <2 1 0 3>
>> -    <3 2 1 0>
> 
> You've dropped all these?