diff mbox series

[v6,01/12] dt-bindings: display/msm: split qcom,mdss bindings

Message ID 20220901102312.2005553-2-dmitry.baryshkov@linaro.org
State Superseded
Headers show
Series dt-bindings: display/msm: rework MDSS and DPU bindings | expand

Commit Message

Dmitry Baryshkov Sept. 1, 2022, 10:23 a.m. UTC
Split Mobile Display SubSystem (MDSS) root node bindings to the separate
yaml file. Changes to the existing (txt) schema:
 - Added optional "vbif_nrt_phys" region used by msm8996
 - Made "bus" and "vsync" clocks optional (they are not used by some
   platforms)
 - Added (optional) "core" clock added recently to the mdss driver
 - Added optional resets property referencing MDSS reset
 - Defined child nodes pointing to corresponding reference schema.
 - Dropped the "lut" clock. It was added to the schema by mistake (it is
   a part of mdp4 schema, not the mdss).

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 .../devicetree/bindings/display/msm/mdp5.txt  |  30 +---
 .../devicetree/bindings/display/msm/mdss.yaml | 166 ++++++++++++++++++
 2 files changed, 167 insertions(+), 29 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/msm/mdss.yaml

Comments

Rob Herring Sept. 8, 2022, 7:37 p.m. UTC | #1
On Thu, Sep 08, 2022 at 03:37:38PM +0200, Krzysztof Kozlowski wrote:
> On 01/09/2022 12:23, Dmitry Baryshkov wrote:
> > Split Mobile Display SubSystem (MDSS) root node bindings to the separate
> > yaml file. Changes to the existing (txt) schema:
> >  - Added optional "vbif_nrt_phys" region used by msm8996
> >  - Made "bus" and "vsync" clocks optional (they are not used by some
> >    platforms)
> >  - Added (optional) "core" clock added recently to the mdss driver
> >  - Added optional resets property referencing MDSS reset
> >  - Defined child nodes pointing to corresponding reference schema.
> >  - Dropped the "lut" clock. It was added to the schema by mistake (it is
> >    a part of mdp4 schema, not the mdss).
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  .../devicetree/bindings/display/msm/mdp5.txt  |  30 +---
> >  .../devicetree/bindings/display/msm/mdss.yaml | 166 ++++++++++++++++++
> >  2 files changed, 167 insertions(+), 29 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/display/msm/mdss.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/display/msm/mdp5.txt b/Documentation/devicetree/bindings/display/msm/mdp5.txt
> > index 43d11279c925..65d03c58dee6 100644
> > --- a/Documentation/devicetree/bindings/display/msm/mdp5.txt
> > +++ b/Documentation/devicetree/bindings/display/msm/mdp5.txt
> > @@ -2,37 +2,9 @@ Qualcomm adreno/snapdragon MDP5 display controller
> >  
> >  Description:
> >  
> > -This is the bindings documentation for the Mobile Display Subsytem(MDSS) that
> > -encapsulates sub-blocks like MDP5, DSI, HDMI, eDP etc, and the MDP5 display
> > +This is the bindings documentation for the MDP5 display
> >  controller found in SoCs like MSM8974, APQ8084, MSM8916, MSM8994 and MSM8996.
> >  
> > -MDSS:
> > -Required properties:
> > -- compatible:
> > -  * "qcom,mdss" - MDSS
> > -- reg: Physical base address and length of the controller's registers.
> > -- reg-names: The names of register regions. The following regions are required:
> > -  * "mdss_phys"
> > -  * "vbif_phys"
> > -- interrupts: The interrupt signal from MDSS.
> > -- interrupt-controller: identifies the node as an interrupt controller.
> > -- #interrupt-cells: specifies the number of cells needed to encode an interrupt
> > -  source, should be 1.
> > -- power-domains: a power domain consumer specifier according to
> > -  Documentation/devicetree/bindings/power/power_domain.txt
> > -- clocks: device clocks. See ../clocks/clock-bindings.txt for details.
> > -- clock-names: the following clocks are required.
> > -  * "iface"
> > -  * "bus"
> > -  * "vsync"
> > -- #address-cells: number of address cells for the MDSS children. Should be 1.
> > -- #size-cells: Should be 1.
> > -- ranges: parent bus address space is the same as the child bus address space.
> > -
> > -Optional properties:
> > -- clock-names: the following clocks are optional:
> > -  * "lut"
> > -
> >  MDP5:
> >  Required properties:
> >  - compatible:
> > diff --git a/Documentation/devicetree/bindings/display/msm/mdss.yaml b/Documentation/devicetree/bindings/display/msm/mdss.yaml
> > new file mode 100644
> > index 000000000000..8860fc55cca5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/msm/mdss.yaml
> > @@ -0,0 +1,166 @@
> > +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/msm/mdss.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm Mobile Display SubSystem (MDSS)
> > +
> > +maintainers:
> > +  - Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > +  - Rob Clark <robdclark@gmail.com>
> > +
> > +description:
> > +  This is the bindings documentation for the Mobile Display Subsytem(MDSS) that
> > +  encapsulates sub-blocks like MDP5, DSI, HDMI, eDP, etc.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - qcom,mdss
> > +
> > +  reg:
> > +    minItems: 2
> > +    maxItems: 3
> > +
> > +  reg-names:
> > +    minItems: 2
> > +    items:
> > +      - const: mdss_phys
> > +      - const: vbif_phys
> > +      - const: vbif_nrt_phys
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  interrupt-controller:
> > +    true
> 
> If there is going to be v7 - please make it one line.
> 
> > +
> > +  "#interrupt-cells":
> > +    const: 1
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +    description: |
> > +      The MDSS power domain provided by GCC
> > +
> > +  clocks:
> > +    minItems: 1
> > +    items:
> > +      - description: Display abh clock
> > +      - description: Display axi clock
> > +      - description: Display vsync clock
> > +
> > +  clock-names:
> > +    minItems: 1
> > +    items:
> > +      - const: iface
> > +      - const: bus
> > +      - const: vsync
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 1
> > +
> > +  ranges:
> > +    true
> 
> Ditto.
> 
> > +
> > +  resets:
> > +    items:
> > +      - description: MDSS_CORE reset
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - interrupts
> > +  - interrupt-controller
> > +  - "#interrupt-cells"
> > +  - power-domains
> > +  - clocks
> > +  - clock-names
> > +  - "#address-cells"
> > +  - "#size-cells"
> > +  - ranges
> > +
> > +patternProperties:
> > +  "^mdp@[1-9a-f][0-9a-f]*$":
> > +    type: object
> > +    properties:
> > +      compatible:
> > +        const: qcom,mdp5
> > +
> > +  "^dsi@[1-9a-f][0-9a-f]*$":
> > +    type: object
> > +    properties:
> > +      compatible:
> > +        const: qcom,mdss-dsi-ctrl
> 
> This should be ref to dsi-controller-main.yaml... or based on previous
> Rob's feedback you dropped it everywhere in children?

I don't think I said. I thought about it some, as yes, we normally have 
done as you suggested. The downside is with a ref we'll do the whole
validation of the child node twice (unless the referenced schema has a 
'select: false') whereas here only 'compatible' is validated twice. This 
way also complicates checking for unevaluatedProperties/additionalProperties.

So maybe better to keep with the 'normal' way and make this a ref. 

Rob
Rob Herring Sept. 9, 2022, 10:23 p.m. UTC | #2
On Thu, Sep 8, 2022 at 3:20 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 08/09/2022 22:37, Rob Herring wrote:
> > On Thu, Sep 08, 2022 at 03:37:38PM +0200, Krzysztof Kozlowski wrote:
> >> On 01/09/2022 12:23, Dmitry Baryshkov wrote:
> >>> Split Mobile Display SubSystem (MDSS) root node bindings to the separate
> >>> yaml file. Changes to the existing (txt) schema:
> >>>   - Added optional "vbif_nrt_phys" region used by msm8996
> >>>   - Made "bus" and "vsync" clocks optional (they are not used by some
> >>>     platforms)
> >>>   - Added (optional) "core" clock added recently to the mdss driver
> >>>   - Added optional resets property referencing MDSS reset
> >>>   - Defined child nodes pointing to corresponding reference schema.
> >>>   - Dropped the "lut" clock. It was added to the schema by mistake (it is
> >>>     a part of mdp4 schema, not the mdss).
> >>>
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>> ---
> >>>   .../devicetree/bindings/display/msm/mdp5.txt  |  30 +---
> >>>   .../devicetree/bindings/display/msm/mdss.yaml | 166 ++++++++++++++++++
> >>>   2 files changed, 167 insertions(+), 29 deletions(-)
> >>>   create mode 100644 Documentation/devicetree/bindings/display/msm/mdss.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/display/msm/mdp5.txt b/Documentation/devicetree/bindings/display/msm/mdp5.txt
> >>> index 43d11279c925..65d03c58dee6 100644
> >>> --- a/Documentation/devicetree/bindings/display/msm/mdp5.txt
> >>> +++ b/Documentation/devicetree/bindings/display/msm/mdp5.txt
> >>> @@ -2,37 +2,9 @@ Qualcomm adreno/snapdragon MDP5 display controller
> >>>
> >>>   Description:
> >>>
> >>> -This is the bindings documentation for the Mobile Display Subsytem(MDSS) that
> >>> -encapsulates sub-blocks like MDP5, DSI, HDMI, eDP etc, and the MDP5 display
> >>> +This is the bindings documentation for the MDP5 display
> >>>   controller found in SoCs like MSM8974, APQ8084, MSM8916, MSM8994 and MSM8996.
> >>>
> >>> -MDSS:
> >>> -Required properties:
> >>> -- compatible:
> >>> -  * "qcom,mdss" - MDSS
> >>> -- reg: Physical base address and length of the controller's registers.
> >>> -- reg-names: The names of register regions. The following regions are required:
> >>> -  * "mdss_phys"
> >>> -  * "vbif_phys"
> >>> -- interrupts: The interrupt signal from MDSS.
> >>> -- interrupt-controller: identifies the node as an interrupt controller.
> >>> -- #interrupt-cells: specifies the number of cells needed to encode an interrupt
> >>> -  source, should be 1.
> >>> -- power-domains: a power domain consumer specifier according to
> >>> -  Documentation/devicetree/bindings/power/power_domain.txt
> >>> -- clocks: device clocks. See ../clocks/clock-bindings.txt for details.
> >>> -- clock-names: the following clocks are required.
> >>> -  * "iface"
> >>> -  * "bus"
> >>> -  * "vsync"
> >>> -- #address-cells: number of address cells for the MDSS children. Should be 1.
> >>> -- #size-cells: Should be 1.
> >>> -- ranges: parent bus address space is the same as the child bus address space.
> >>> -
> >>> -Optional properties:
> >>> -- clock-names: the following clocks are optional:
> >>> -  * "lut"
> >>> -
> >>>   MDP5:
> >>>   Required properties:
> >>>   - compatible:
> >>> diff --git a/Documentation/devicetree/bindings/display/msm/mdss.yaml b/Documentation/devicetree/bindings/display/msm/mdss.yaml
> >>> new file mode 100644
> >>> index 000000000000..8860fc55cca5
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/display/msm/mdss.yaml
> >>> @@ -0,0 +1,166 @@
> >>> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/display/msm/mdss.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Qualcomm Mobile Display SubSystem (MDSS)
> >>> +
> >>> +maintainers:
> >>> +  - Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>> +  - Rob Clark <robdclark@gmail.com>
> >>> +
> >>> +description:
> >>> +  This is the bindings documentation for the Mobile Display Subsytem(MDSS) that
> >>> +  encapsulates sub-blocks like MDP5, DSI, HDMI, eDP, etc.
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    enum:
> >>> +      - qcom,mdss
> >>> +
> >>> +  reg:
> >>> +    minItems: 2
> >>> +    maxItems: 3
> >>> +
> >>> +  reg-names:
> >>> +    minItems: 2
> >>> +    items:
> >>> +      - const: mdss_phys
> >>> +      - const: vbif_phys
> >>> +      - const: vbif_nrt_phys
> >>> +
> >>> +  interrupts:
> >>> +    maxItems: 1
> >>> +
> >>> +  interrupt-controller:
> >>> +    true
> >>
> >> If there is going to be v7 - please make it one line.
> >>
> >>> +
> >>> +  "#interrupt-cells":
> >>> +    const: 1
> >>> +
> >>> +  power-domains:
> >>> +    maxItems: 1
> >>> +    description: |
> >>> +      The MDSS power domain provided by GCC
> >>> +
> >>> +  clocks:
> >>> +    minItems: 1
> >>> +    items:
> >>> +      - description: Display abh clock
> >>> +      - description: Display axi clock
> >>> +      - description: Display vsync clock
> >>> +
> >>> +  clock-names:
> >>> +    minItems: 1
> >>> +    items:
> >>> +      - const: iface
> >>> +      - const: bus
> >>> +      - const: vsync
> >>> +
> >>> +  "#address-cells":
> >>> +    const: 1
> >>> +
> >>> +  "#size-cells":
> >>> +    const: 1
> >>> +
> >>> +  ranges:
> >>> +    true
> >>
> >> Ditto.
> >>
> >>> +
> >>> +  resets:
> >>> +    items:
> >>> +      - description: MDSS_CORE reset
> >>> +
> >>> +required:
> >>> +  - compatible
> >>> +  - reg
> >>> +  - reg-names
> >>> +  - interrupts
> >>> +  - interrupt-controller
> >>> +  - "#interrupt-cells"
> >>> +  - power-domains
> >>> +  - clocks
> >>> +  - clock-names
> >>> +  - "#address-cells"
> >>> +  - "#size-cells"
> >>> +  - ranges
> >>> +
> >>> +patternProperties:
> >>> +  "^mdp@[1-9a-f][0-9a-f]*$":
> >>> +    type: object
> >>> +    properties:
> >>> +      compatible:
> >>> +        const: qcom,mdp5
> >>> +
> >>> +  "^dsi@[1-9a-f][0-9a-f]*$":
> >>> +    type: object
> >>> +    properties:
> >>> +      compatible:
> >>> +        const: qcom,mdss-dsi-ctrl
> >>
> >> This should be ref to dsi-controller-main.yaml... or based on previous
> >> Rob's feedback you dropped it everywhere in children?
> >
> > I don't think I said. I thought about it some, as yes, we normally have
> > done as you suggested. The downside is with a ref we'll do the whole
> > validation of the child node twice (unless the referenced schema has a
> > 'select: false') whereas here only 'compatible' is validated twice. This
> > way also complicates checking for unevaluatedProperties/additionalProperties.
> >
> > So maybe better to keep with the 'normal' way and make this a ref.
>
> Well.. I tried using $ref in the previous iteration, but then I faced
> the fact that I can not use it. Using the $ref the node gets validated
> even if it is disabled, and we do not want to do this. The nodes are
> usually split in way that regulators are specified in the board DT file.
> Thus disabled dsi/dsi-phy nodes do not get all the required regulators.
> And dt-validate happily dumps tons of warnings for disabled nodes. Thus
> I ended up with the compatible rather than $ref.

Only warnings about required properties? Those are supposed to get
filtered out if the node is disabled. Maybe being in a $ref doesn't
work.

Rob
Krzysztof Kozlowski Sept. 11, 2022, 11:27 a.m. UTC | #3
On 10/09/2022 14:54, Dmitry Baryshkov wrote:
>>
>> However I think there is no such problem, as Dmitry said, that ref
>> changes anything. There will be always failure - either from parent
>> schema (using $ref) or from device child schema (the one which actually
>> misses the property).
> 
> Initially I stumbled upon this issue with the dsi and dsi_phy nodes
> for msm8996 devices. If I have $ref here, dsi1/dsi1_phy nodes will
> emit warnings regarding the missing -supply properties despite nodes
> being disabled. If I use `compatible' here, the schema checks pass.
> Thus I'd prefer to leave `compatible' here. Not to mention that it
> also allows specifying a tighter binding than just using the $ref.

I don't think we understood each other. I claim that error will be there
anyway, just from different schema. So your change fixes nothing in
total schema check...


Best regards,
Krzysztof
Dmitry Baryshkov Sept. 11, 2022, 1:45 p.m. UTC | #4
On Sun, 11 Sept 2022 at 14:27, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 10/09/2022 14:54, Dmitry Baryshkov wrote:
> >>
> >> However I think there is no such problem, as Dmitry said, that ref
> >> changes anything. There will be always failure - either from parent
> >> schema (using $ref) or from device child schema (the one which actually
> >> misses the property).
> >
> > Initially I stumbled upon this issue with the dsi and dsi_phy nodes
> > for msm8996 devices. If I have $ref here, dsi1/dsi1_phy nodes will
> > emit warnings regarding the missing -supply properties despite nodes
> > being disabled. If I use `compatible' here, the schema checks pass.
> > Thus I'd prefer to leave `compatible' here. Not to mention that it
> > also allows specifying a tighter binding than just using the $ref.
>
> I don't think we understood each other. I claim that error will be there
> anyway, just from different schema. So your change fixes nothing in
> total schema check...

If the node is disabled, there will be no different schema check.
Krzysztof Kozlowski Sept. 11, 2022, 1:57 p.m. UTC | #5
On 11/09/2022 15:45, Dmitry Baryshkov wrote:
> On Sun, 11 Sept 2022 at 14:27, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 10/09/2022 14:54, Dmitry Baryshkov wrote:
>>>>
>>>> However I think there is no such problem, as Dmitry said, that ref
>>>> changes anything. There will be always failure - either from parent
>>>> schema (using $ref) or from device child schema (the one which actually
>>>> misses the property).
>>>
>>> Initially I stumbled upon this issue with the dsi and dsi_phy nodes
>>> for msm8996 devices. If I have $ref here, dsi1/dsi1_phy nodes will
>>> emit warnings regarding the missing -supply properties despite nodes
>>> being disabled. If I use `compatible' here, the schema checks pass.
>>> Thus I'd prefer to leave `compatible' here. Not to mention that it
>>> also allows specifying a tighter binding than just using the $ref.
>>
>> I don't think we understood each other. I claim that error will be there
>> anyway, just from different schema. So your change fixes nothing in
>> total schema check...
> 
> If the node is disabled, there will be no different schema check.

As I wrote before, there was.

Best regards,
Krzysztof
Krzysztof Kozlowski Sept. 11, 2022, 6:32 p.m. UTC | #6
On 11/09/2022 19:45, Dmitry Baryshkov wrote:
> On Sun, 11 Sept 2022 at 16:57, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 11/09/2022 15:45, Dmitry Baryshkov wrote:
>>> On Sun, 11 Sept 2022 at 14:27, Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>
>>>> On 10/09/2022 14:54, Dmitry Baryshkov wrote:
>>>>>>
>>>>>> However I think there is no such problem, as Dmitry said, that ref
>>>>>> changes anything. There will be always failure - either from parent
>>>>>> schema (using $ref) or from device child schema (the one which actually
>>>>>> misses the property).
>>>>>
>>>>> Initially I stumbled upon this issue with the dsi and dsi_phy nodes
>>>>> for msm8996 devices. If I have $ref here, dsi1/dsi1_phy nodes will
>>>>> emit warnings regarding the missing -supply properties despite nodes
>>>>> being disabled. If I use `compatible' here, the schema checks pass.
>>>>> Thus I'd prefer to leave `compatible' here. Not to mention that it
>>>>> also allows specifying a tighter binding than just using the $ref.
>>>>
>>>> I don't think we understood each other. I claim that error will be there
>>>> anyway, just from different schema. So your change fixes nothing in
>>>> total schema check...
>>>
>>> If the node is disabled, there will be no different schema check.
>>
>> As I wrote before, there was.
> 
> The following results were captured with the following command, with
> most of the DSI and MDSS schema files fixed, using the following
> command:
> $ PATH=~/.local/bin/:$PATH make -C ../build-64/ ARCH=arm64
> qcom/sda660-inforce-ifc6560.dtb  CHECK_DTBS=y
> DT_SCHEMA_FILES=display/msm
> 
> As you can see from the example below, when using 'compatible' I'm
> getting warnings just for the gpu@5000000 node, while using $ref I
> also got warnings for the dsi-phy@c996400 node (disabled in the DT
> file).
> For your reference the tree in question is uploaded to the:
>     https://git.linaro.org/people/dmitry.baryshkov/kernel.git msm-mdss-yaml

I did not say anything about msm-mdss. I said you will get errors from
child schema anyway.

	From schema:
/home/krzk/dev/linux/linux/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml

/home/krzk/dev/linux/linux/out/arch/arm64/boot/dts/qcom/sda660-inforce-ifc6560.dtb:
dsi@c994000: clock-names: ['mdp_core', 'byte', 'byte_intf', 'mnoc',
'iface', 'bus', 'core_mmss', 'pixel', 'core'] is too long

	From schema:
/home/krzk/dev/linux/linux/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml

/home/krzk/dev/linux/linux/out/arch/arm64/boot/dts/qcom/sda660-inforce-ifc6560.dtb:
dsi@c994000: 'vdda-supply' does not match any of the regexes:
'pinctrl-[0-9]+'



If your child schema fails, the referencing schema fails as well...


Best regards,
Krzysztof
Krzysztof Kozlowski Sept. 11, 2022, 6:55 p.m. UTC | #7
On 11/09/2022 20:36, Krzysztof Kozlowski wrote:

>> /home/krzk/dev/linux/linux/out/arch/arm64/boot/dts/qcom/sda660-inforce-ifc6560.dtb:
>> dsi@c994000: 'vdda-supply' does not match any of the regexes:
>> 'pinctrl-[0-9]+'
>>
>>
>>
>> If your child schema fails, the referencing schema fails as well...
> 
> 
> Although now with DSI-PHY I cannot reproduce it and I am pretty sure I
> reproduced it with DPU controllers after modifying the DTS to lack a
> property... Hmmm
> 
I think I have a fix for this in DT schema.

Best regards,
Krzysztof
Dmitry Baryshkov Sept. 15, 2022, 11:50 a.m. UTC | #8
On 11/09/2022 22:19, Krzysztof Kozlowski wrote:
> On 11/09/2022 20:36, Krzysztof Kozlowski wrote:
> 
>>> If your child schema fails, the referencing schema fails as well...
>>
>>
>> Although now with DSI-PHY I cannot reproduce it and I am pretty sure I
>> reproduced it with DPU controllers after modifying the DTS to lack a
>> property... Hmmm
> 
> https://github.com/devicetree-org/dt-schema/pull/82

Thanks for the quick fix!

However I think I'd still stick to the compatible binding for two reasons:
  - It doesn't evaluate schema twice for these nodes
  - It allows us to tightly link child nodes with the parent compatible, 
which I think, was one of the points raised several revisions ago.
Krzysztof Kozlowski Sept. 15, 2022, 2:33 p.m. UTC | #9
On 15/09/2022 12:50, Dmitry Baryshkov wrote:
> On 11/09/2022 22:19, Krzysztof Kozlowski wrote:
>> On 11/09/2022 20:36, Krzysztof Kozlowski wrote:
>>
>>>> If your child schema fails, the referencing schema fails as well...
>>>
>>>
>>> Although now with DSI-PHY I cannot reproduce it and I am pretty sure I
>>> reproduced it with DPU controllers after modifying the DTS to lack a
>>> property... Hmmm
>>
>> https://github.com/devicetree-org/dt-schema/pull/82
> 
> Thanks for the quick fix!
> 
> However I think I'd still stick to the compatible binding for two reasons:
>   - It doesn't evaluate schema twice for these nodes
>   - It allows us to tightly link child nodes with the parent compatible, 
> which I think, was one of the points raised several revisions ago.

Yes, true. The referenced schema sometimes accepts few compatibles and
this gives strict matching without additional complexity.


Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/msm/mdp5.txt b/Documentation/devicetree/bindings/display/msm/mdp5.txt
index 43d11279c925..65d03c58dee6 100644
--- a/Documentation/devicetree/bindings/display/msm/mdp5.txt
+++ b/Documentation/devicetree/bindings/display/msm/mdp5.txt
@@ -2,37 +2,9 @@  Qualcomm adreno/snapdragon MDP5 display controller
 
 Description:
 
-This is the bindings documentation for the Mobile Display Subsytem(MDSS) that
-encapsulates sub-blocks like MDP5, DSI, HDMI, eDP etc, and the MDP5 display
+This is the bindings documentation for the MDP5 display
 controller found in SoCs like MSM8974, APQ8084, MSM8916, MSM8994 and MSM8996.
 
-MDSS:
-Required properties:
-- compatible:
-  * "qcom,mdss" - MDSS
-- reg: Physical base address and length of the controller's registers.
-- reg-names: The names of register regions. The following regions are required:
-  * "mdss_phys"
-  * "vbif_phys"
-- interrupts: The interrupt signal from MDSS.
-- interrupt-controller: identifies the node as an interrupt controller.
-- #interrupt-cells: specifies the number of cells needed to encode an interrupt
-  source, should be 1.
-- power-domains: a power domain consumer specifier according to
-  Documentation/devicetree/bindings/power/power_domain.txt
-- clocks: device clocks. See ../clocks/clock-bindings.txt for details.
-- clock-names: the following clocks are required.
-  * "iface"
-  * "bus"
-  * "vsync"
-- #address-cells: number of address cells for the MDSS children. Should be 1.
-- #size-cells: Should be 1.
-- ranges: parent bus address space is the same as the child bus address space.
-
-Optional properties:
-- clock-names: the following clocks are optional:
-  * "lut"
-
 MDP5:
 Required properties:
 - compatible:
diff --git a/Documentation/devicetree/bindings/display/msm/mdss.yaml b/Documentation/devicetree/bindings/display/msm/mdss.yaml
new file mode 100644
index 000000000000..8860fc55cca5
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/msm/mdss.yaml
@@ -0,0 +1,166 @@ 
+# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/msm/mdss.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Mobile Display SubSystem (MDSS)
+
+maintainers:
+  - Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
+  - Rob Clark <robdclark@gmail.com>
+
+description:
+  This is the bindings documentation for the Mobile Display Subsytem(MDSS) that
+  encapsulates sub-blocks like MDP5, DSI, HDMI, eDP, etc.
+
+properties:
+  compatible:
+    enum:
+      - qcom,mdss
+
+  reg:
+    minItems: 2
+    maxItems: 3
+
+  reg-names:
+    minItems: 2
+    items:
+      - const: mdss_phys
+      - const: vbif_phys
+      - const: vbif_nrt_phys
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-controller:
+    true
+
+  "#interrupt-cells":
+    const: 1
+
+  power-domains:
+    maxItems: 1
+    description: |
+      The MDSS power domain provided by GCC
+
+  clocks:
+    minItems: 1
+    items:
+      - description: Display abh clock
+      - description: Display axi clock
+      - description: Display vsync clock
+
+  clock-names:
+    minItems: 1
+    items:
+      - const: iface
+      - const: bus
+      - const: vsync
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 1
+
+  ranges:
+    true
+
+  resets:
+    items:
+      - description: MDSS_CORE reset
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - interrupts
+  - interrupt-controller
+  - "#interrupt-cells"
+  - power-domains
+  - clocks
+  - clock-names
+  - "#address-cells"
+  - "#size-cells"
+  - ranges
+
+patternProperties:
+  "^mdp@[1-9a-f][0-9a-f]*$":
+    type: object
+    properties:
+      compatible:
+        const: qcom,mdp5
+
+  "^dsi@[1-9a-f][0-9a-f]*$":
+    type: object
+    properties:
+      compatible:
+        const: qcom,mdss-dsi-ctrl
+
+  "^dsi-phy@[1-9a-f][0-9a-f]*$":
+    type: object
+    properties:
+      compatible:
+        enum:
+          - qcom,dsi-phy-14nm
+          - qcom,dsi-phy-14nm-660
+          - qcom,dsi-phy-20nm
+          - qcom,dsi-phy-28nm-hpm
+          - qcom,dsi-phy-28nm-lp
+
+  "^hdmi-phy@[1-9a-f][0-9a-f]*$":
+    type: object
+    properties:
+      compatible:
+        enum:
+          - qcom,hdmi-phy-8084
+          - qcom,hdmi-phy-8660
+          - qcom,hdmi-phy-8960
+          - qcom,hdmi-phy-8974
+          - qcom,hdmi-phy-8996
+
+  "^hdmi-tx@[1-9a-f][0-9a-f]*$":
+    type: object
+    properties:
+      compatible:
+        enum:
+          - qcom,hdmi-tx-8084
+          - qcom,hdmi-tx-8660
+          - qcom,hdmi-tx-8960
+          - qcom,hdmi-tx-8974
+          - qcom,hdmi-tx-8994
+          - qcom,hdmi-tx-8996
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,gcc-msm8916.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    mdss@1a00000 {
+      compatible = "qcom,mdss";
+      reg = <0x1a00000 0x1000>,
+            <0x1ac8000 0x3000>;
+      reg-names = "mdss_phys", "vbif_phys";
+
+      power-domains = <&gcc MDSS_GDSC>;
+
+      clocks = <&gcc GCC_MDSS_AHB_CLK>,
+               <&gcc GCC_MDSS_AXI_CLK>,
+               <&gcc GCC_MDSS_VSYNC_CLK>;
+      clock-names = "iface",
+                    "bus",
+                    "vsync";
+
+      interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
+
+      interrupt-controller;
+      #interrupt-cells = <1>;
+
+      #address-cells = <1>;
+      #size-cells = <1>;
+      ranges;
+
+    };
+...