Re: [PATCH v2 06/11] dt-bindings: display/msm: move qcom,sc7180-mdss schema to mdss.yaml

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

On 11/08/2022 11:25, Krzysztof Kozlowski wrote:
> On 10/07/2022 12:00, Dmitry Baryshkov wrote:
> > Move schema for qcom,sc7180-mdss from dpu-sc7180.yaml to mdss.yaml so
> > that the dpu file describes only the DPU schema.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> >   .../bindings/display/msm/dpu-sc7180.yaml      | 149 +++++-------------
> >   .../devicetree/bindings/display/msm/mdss.yaml |  45 +++++-
> >   2 files changed, 80 insertions(+), 114 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/display/msm/dpu-sc7180.yaml b/Documentation/devicetree/bindings/display/msm/dpu-sc7180.yaml
> > index d3c3e4b07897..9d4ec0b60c25 100644
> > --- a/Documentation/devicetree/bindings/display/msm/dpu-sc7180.yaml
> > +++ b/Documentation/devicetree/bindings/display/msm/dpu-sc7180.yaml
> > @@ -10,151 +10,78 @@ maintainers:
> >     - Krishna Manikandan <quic_mkrishn@xxxxxxxxxxx>
> >   
> >   description: |
> > -  Device tree bindings for MSM Mobile Display Subsystem(MDSS) that encapsulates
> > -  sub-blocks like DPU display controller, DSI and DP interfaces etc. Device tree
> > -  bindings of MDSS and DPU are mentioned for SC7180 target.
> > +  Device tree bindings for the DPU display controller for SC7180 target.
> >   
> >   properties:
> >     compatible:
> >       items:
> > -      - const: qcom,sc7180-mdss
> > +      - const: qcom,sc7180-dpu
> >   
> >     reg:
> > -    maxItems: 1
> > +    items:
> > +      - description: Address offset and size for mdp register set
> > +      - description: Address offset and size for vbif register set
> >   
> >     reg-names:
> > -    const: mdss
> > -
> > -  power-domains:
> > -    maxItems: 1
> > +    items:
> > +      - const: mdp
> > +      - const: vbif
> >   
> >     clocks:
> >       items:
> > -      - description: Display AHB clock from gcc
> > -      - description: Display AHB clock from dispcc
> > +      - description: Display hf axi clock
> > +      - description: Display ahb clock
> > +      - description: Display rotator clock
> > +      - description: Display lut clock
> >         - description: Display core clock
> > +      - description: Display vsync clock
> >   
> >     clock-names:
> >       items:
> > +      - const: bus
> >         - const: iface
> > -      - const: ahb
> > +      - const: rot
> > +      - const: lut
> >         - const: core
> > +      - const: vsync
> >   
> >     interrupts:
> >       maxItems: 1
> >   
> > -  interrupt-controller: true
> > -
> > -  "#address-cells": true
> > -
> > -  "#size-cells": true
> > -
> > -  "#interrupt-cells":
> > -    const: 1
> > -
> > -  iommus:
> > -    items:
> > -      - description: Phandle to apps_smmu node with SID mask for Hard-Fail port0
> > -
> > -  ranges: true
> > -
> > -  interconnects:
> > -    items:
> > -      - description: Interconnect path specifying the port ids for data bus
> > -
> > -  interconnect-names:
> > -    const: mdp0-mem
> > +  power-domains:
> > +    maxItems: 1
> >   
> > -  resets:
> > -    items:
> > -      - description: MDSS_CORE reset
> > +  operating-points-v2: true
> >   
> > -patternProperties:
> > -  "^display-controller@[0-9a-f]+$":
> > -    type: object
> > -    description: Node containing the properties of DPU.
> > +  ports:
> > +    $ref: /schemas/graph.yaml#/properties/ports
> > +    description: |
> > +      Contains the list of output ports from DPU device. These ports
> > +      connect to interfaces that are external to the DPU hardware,
> > +      such as DSI, DP etc. Each output port contains an endpoint that
> > +      describes how it is connected to an external interface.
> >   
> >       properties:
> > -      compatible:
> > -        items:
> > -          - const: qcom,sc7180-dpu
> > -
> > -      reg:
> > -        items:
> > -          - description: Address offset and size for mdp register set
> > -          - description: Address offset and size for vbif register set
> > -
> > -      reg-names:
> > -        items:
> > -          - const: mdp
> > -          - const: vbif
> > -
> > -      clocks:
> > -        items:
> > -          - description: Display hf axi clock
> > -          - description: Display ahb clock
> > -          - description: Display rotator clock
> > -          - description: Display lut clock
> > -          - description: Display core clock
> > -          - description: Display vsync clock
> > -
> > -      clock-names:
> > -        items:
> > -          - const: bus
> > -          - const: iface
> > -          - const: rot
> > -          - const: lut
> > -          - const: core
> > -          - const: vsync
> > -
> > -      interrupts:
> > -        maxItems: 1
> > -
> > -      power-domains:
> > -        maxItems: 1
> > -
> > -      operating-points-v2: true
> > -
> > -      ports:
> > -        $ref: /schemas/graph.yaml#/properties/ports
> > -        description: |
> > -          Contains the list of output ports from DPU device. These ports
> > -          connect to interfaces that are external to the DPU hardware,
> > -          such as DSI, DP etc. Each output port contains an endpoint that
> > -          describes how it is connected to an external interface.
> > -
> > -        properties:
> > -          port@0:
> > -            $ref: /schemas/graph.yaml#/properties/port
> > -            description: DPU_INTF1 (DSI1)
> > -
> > -          port@2:
> > -            $ref: /schemas/graph.yaml#/properties/port
> > -            description: DPU_INTF0 (DP)
> > -
> > -        required:
> > -          - port@0
> > +      port@0:
> > +        $ref: /schemas/graph.yaml#/properties/port
> > +        description: DPU_INTF1 (DSI1)
> > +
> > +      port@2:
> > +        $ref: /schemas/graph.yaml#/properties/port
> > +        description: DPU_INTF0 (DP)
> >   
> >       required:
> > -      - compatible
> > -      - reg
> > -      - reg-names
> > -      - clocks
> > -      - interrupts
> > -      - power-domains
> > -      - operating-points-v2
> > -      - ports
> > +      - port@0
> >   
> >   required:
> >     - compatible
> >     - reg
> >     - reg-names
> > -  - power-domains
> >     - clocks
> >     - interrupts
> > -  - interrupt-controller
> > -  - iommus
> > -  - ranges
> > +  - power-domains
> > +  - operating-points-v2
> > +  - ports
> >   
> >   additionalProperties: false
> >   
> > diff --git a/Documentation/devicetree/bindings/display/msm/mdss.yaml b/Documentation/devicetree/bindings/display/msm/mdss.yaml
> > index 7d4ab3d71d2d..98f1f2501291 100644
> > --- a/Documentation/devicetree/bindings/display/msm/mdss.yaml
> > +++ b/Documentation/devicetree/bindings/display/msm/mdss.yaml
> > @@ -17,6 +17,7 @@ description:
> >   properties:
> >     compatible:
> >       enum:
> > +      - qcom,sc7180-mdss
> >         - qcom,sdm845-mdss
> >         - qcom,mdss
> >   
> > @@ -64,20 +65,21 @@ properties:
> >         - description: MDSS_CORE reset
> >   
> >     interconnects:
> > -    minItems: 2
> > +    minItems: 1
> >       items:
> >         - description: MDP port 0
> >         - description: MDP port 1
> >         - description: Rotator
> >   
> >     interconnect-names:
> > -    minItems: 2
> > +    minItems: 1
> >       items:
> >         - const: mdp0-mem
> >         - const: mdp1-mem
> >         - const: rotator-mem
> >   
> >     iommus:
> > +    minItems: 1
> >       items:
> >         - description: Phandle to apps_smmu node with SID mask for Hard-Fail port0
> >         - description: Phandle to apps_smmu node with SID mask for Hard-Fail port1
> > @@ -129,9 +131,11 @@ allOf:
> >               - const: mdss
> >   
> >           interconnects:
> > +          minItems: 1
> >             maxItems: 2
> >   
> >           interconnect-names:
> > +          minItems: 1
> >             maxItems: 2
> >   
> >         required:
> > @@ -157,6 +161,29 @@ allOf:
> >           iommus:
> >             minItems: 2
> >   
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - qcom,sc7180-mdss
> > +    then:
> > +      properties:
> > +        clocks:
> > +          items:
> > +            - description: Display AHB clock from gcc
> > +            - description: Display AHB clock from dispcc
> > +            - description: Display core clock
> > +
> > +        clock-names:
> > +          items:
> > +            - const: iface
> > +            - const: ahb
> > +            - const: core
> > +
> > +        iommus:
> > +          maxItems: 1
> > +
> >   required:
> >     - compatible
> >     - reg
> > @@ -177,7 +204,19 @@ patternProperties:
> >       # TODO: add reference once the mdp5 is converted
> >   
> >     "^display-controller@(0|[1-9a-f][0-9a-f]*)$":
> > -    $ref: dpu-sdm845.yaml
> > +    oneOf:
> > +      - $ref: dpu-sc7180.yaml
> > +      - $ref: dpu-sdm845.yaml
>
> I don't think this is good approach. These are strictly tied, so you
> should have rather per SoC MDSS schema pulling in:
> 1. common MDSS
> 2. specific children schemas
>
> This makes it quite obvious and specific. Your setup now allows putting
> sc7180-dpu under sdm845-dpu.

Then we should probably also use very specific $ref and compat check for 
all other entries in this file too. I understand your concern, however 
I'd prefer to have the working schema which is not tight enough compared 
to set of schema files that completely do not work. And current 
dpu-*yaml do not work as they allow just the display-controller beneath 
the mdss node.

If you absolutely insist on me making the binding tight enough, I'll 
rework the bindings to clearly specify which SoC has which nodes.

> Best regards,
> Krzysztof

--
With best wishes
Dmitry