Re: [PATCH 1/3] dt-bindings: display/msm: Add binding for SC8280XP MDSS
On Wed, 10 Aug 2022 21:01:19 -0700, Bjorn Andersson wrote: > Add binding for the display subsystem and display processing unit in the > Qualcomm SC8280XP platform. > > Signed-off-by: Bjorn Andersson > --- > .../bindings/display/msm/dpu-sc8280xp.yaml| 284 ++ > 1 file changed, 284 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/display/msm/dpu-sc8280xp.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: Documentation/devicetree/bindings/display/msm/dpu-sc8280xp.example.dts:21:18: fatal error: dt-bindings/clock/qcom,dispcc-sc8280xp.h: No such file or directory 21 | #include | ^~ compilation terminated. make[1]: *** [scripts/Makefile.lib:383: Documentation/devicetree/bindings/display/msm/dpu-sc8280xp.example.dtb] Error 1 make[1]: *** Waiting for unfinished jobs make: *** [Makefile:1404: dt_binding_check] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/ This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
Re: [PATCH 1/3] dt-bindings: display/msm: Add binding for SC8280XP MDSS
On 11/08/2022 11:04, Krzysztof Kozlowski wrote: >> >> additionalProperties:false on this level >> >> which will point to missing properties (e.g. opp-table) > > I'll fix existing bindings which have similar issue. Hm, I think Dmitry is already working on this: https://lore.kernel.org/all/20220710090040.35193-5-dmitry.barysh...@linaro.org/ so your patches should be on top of his. Best regards, Krzysztof
Re: [PATCH 1/3] dt-bindings: display/msm: Add binding for SC8280XP MDSS
On 11/08/2022 10:56, Krzysztof Kozlowski wrote:
> On 11/08/2022 07:01, Bjorn Andersson wrote:
>> Add binding for the display subsystem and display processing unit in the
>> Qualcomm SC8280XP platform.
>>
>> Signed-off-by: Bjorn Andersson
>> ---
>> .../bindings/display/msm/dpu-sc8280xp.yaml| 284 ++
>> 1 file changed, 284 insertions(+)
>> create mode 100644
>> Documentation/devicetree/bindings/display/msm/dpu-sc8280xp.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/display/msm/dpu-sc8280xp.yaml
>> b/Documentation/devicetree/bindings/display/msm/dpu-sc8280xp.yaml
>> new file mode 100644
>> index ..6c25943e639c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/msm/dpu-sc8280xp.yaml
>
> qcom prefix is needed (also when file is in msm subdir)
>
> The file name should be based on compatible, so "qcom,sc8280xp-mdss.yaml"
>
>> @@ -0,0 +1,284 @@
>> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/msm/dpu-sc8280xp.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Display Processing Unit for SC8280XP
>> +
>> +maintainers:
>> + - Bjorn Andersson
>> +
>> +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 SC8280XP.
>
> s/Device tree bindings//
> so just:
>
> SC8280XP MSM Mobile Display Subsystem (MDSS) that encapsulates
> sub-blocks like DPU display controller, DSI and DP interfaces etc.
>
>> +
>> +properties:
>> + compatible:
>> +const: qcom,sc8280xp-mdss
>> +
>> + reg:
>> +maxItems: 1
>> +
>> + reg-names:
>> +const: mdss
>
> You do not need reg names for one item, especially if the name is kind
> of obvious... unless you re-use existing driver which needs it? Then
> maybe let's change the driver to take first element?
OK, I see the driver expects this. It seems it is legacy from
87729e2a7871 ("drm/msm: unify MDSS drivers") times. So it could be
changed to grab first element always (older MDSS with three reg items
still has mdss_phys at first item).
>
>> +
>> + power-domains:
>> +maxItems: 1
>> +
>> + 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
>> +
>> + interrupts:
>> +maxItems: 1
>> +
>> + interrupt-controller: true
>> +
>> + "#address-cells": true
>> +
>> + "#size-cells": true
>
> I see other DPU bindings also specify both as "true". Why not a fixed
> number (const)?
>
>> +
>> + "#interrupt-cells":
>> +const: 1
>> +
>> + iommus:
>> +items:
>> + - description: Phandle to apps_smmu node with SID mask for Hard-Fail
>> port0
>> +
>> + ranges: true
>> +
>> + interconnects:
>> +minItems: 2
>
> No need for minItems in such case.
>
>> +maxItems: 2
>> +
>> + interconnect-names:
>> +items:
>> + - const: mdp0-mem
>> + - const: mdp1-mem
>> +
>> + resets:
>> +items:
>> + - description: MDSS_CORE reset
>> +
>> +patternProperties:
>> + "^display-controller@[0-9a-f]+$":
>> +type: object
>> +description: Node containing the properties of DPU.
>
> additionalProperties:false on this level
>
> which will point to missing properties (e.g. opp-table)
I'll fix existing bindings which have similar issue.
Best regards,
Krzysztof
Re: [PATCH 1/3] dt-bindings: display/msm: Add binding for SC8280XP MDSS
On 11/08/2022 07:01, Bjorn Andersson wrote: > Add binding for the display subsystem and display processing unit in the > Qualcomm SC8280XP platform. > > Signed-off-by: Bjorn Andersson > --- > .../bindings/display/msm/dpu-sc8280xp.yaml| 284 ++ > 1 file changed, 284 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/display/msm/dpu-sc8280xp.yaml > > diff --git a/Documentation/devicetree/bindings/display/msm/dpu-sc8280xp.yaml > b/Documentation/devicetree/bindings/display/msm/dpu-sc8280xp.yaml > new file mode 100644 > index ..6c25943e639c > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/msm/dpu-sc8280xp.yaml qcom prefix is needed (also when file is in msm subdir) The file name should be based on compatible, so "qcom,sc8280xp-mdss.yaml" > @@ -0,0 +1,284 @@ > +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/msm/dpu-sc8280xp.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm Display Processing Unit for SC8280XP > + > +maintainers: > + - Bjorn Andersson > + > +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 SC8280XP. s/Device tree bindings// so just: SC8280XP MSM Mobile Display Subsystem (MDSS) that encapsulates sub-blocks like DPU display controller, DSI and DP interfaces etc. > + > +properties: > + compatible: > +const: qcom,sc8280xp-mdss > + > + reg: > +maxItems: 1 > + > + reg-names: > +const: mdss You do not need reg names for one item, especially if the name is kind of obvious... unless you re-use existing driver which needs it? Then maybe let's change the driver to take first element? > + > + power-domains: > +maxItems: 1 > + > + 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 > + > + interrupts: > +maxItems: 1 > + > + interrupt-controller: true > + > + "#address-cells": true > + > + "#size-cells": true I see other DPU bindings also specify both as "true". Why not a fixed number (const)? > + > + "#interrupt-cells": > +const: 1 > + > + iommus: > +items: > + - description: Phandle to apps_smmu node with SID mask for Hard-Fail > port0 > + > + ranges: true > + > + interconnects: > +minItems: 2 No need for minItems in such case. > +maxItems: 2 > + > + interconnect-names: > +items: > + - const: mdp0-mem > + - const: mdp1-mem > + > + resets: > +items: > + - description: MDSS_CORE reset > + > +patternProperties: > + "^display-controller@[0-9a-f]+$": > +type: object > +description: Node containing the properties of DPU. additionalProperties:false on this level which will point to missing properties (e.g. opp-table) > + > +properties: > + compatible: > +const: qcom,sc8280xp-dpu > + Best regards, Krzysztof
[PATCH 1/3] dt-bindings: display/msm: Add binding for SC8280XP MDSS
Add binding for the display subsystem and display processing unit in the
Qualcomm SC8280XP platform.
Signed-off-by: Bjorn Andersson
---
.../bindings/display/msm/dpu-sc8280xp.yaml| 284 ++
1 file changed, 284 insertions(+)
create mode 100644
Documentation/devicetree/bindings/display/msm/dpu-sc8280xp.yaml
diff --git a/Documentation/devicetree/bindings/display/msm/dpu-sc8280xp.yaml
b/Documentation/devicetree/bindings/display/msm/dpu-sc8280xp.yaml
new file mode 100644
index ..6c25943e639c
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/msm/dpu-sc8280xp.yaml
@@ -0,0 +1,284 @@
+# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/msm/dpu-sc8280xp.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Display Processing Unit for SC8280XP
+
+maintainers:
+ - Bjorn Andersson
+
+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 SC8280XP.
+
+properties:
+ compatible:
+const: qcom,sc8280xp-mdss
+
+ reg:
+maxItems: 1
+
+ reg-names:
+const: mdss
+
+ power-domains:
+maxItems: 1
+
+ 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
+
+ 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:
+minItems: 2
+maxItems: 2
+
+ interconnect-names:
+items:
+ - const: mdp0-mem
+ - const: mdp1-mem
+
+ resets:
+items:
+ - description: MDSS_CORE reset
+
+patternProperties:
+ "^display-controller@[0-9a-f]+$":
+type: object
+description: Node containing the properties of DPU.
+
+properties:
+ compatible:
+const: qcom,sc8280xp-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 sf axi clock
+ - description: Display ahb clock
+ - description: Display lut clock
+ - description: Display core clock
+ - description: Display vsync clock
+
+ clock-names:
+items:
+ - const: bus
+ - const: nrt_bus
+ - const: iface
+ - 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.
+
+patternProperties:
+ '^port@[0-8]$':
+$ref: /schemas/graph.yaml#/properties/port
+description: DPU interfaces
+
+required:
+ - compatible
+ - reg
+ - reg-names
+ - clocks
+ - interrupts
+ - power-domains
+ - operating-points-v2
+ - ports
+
+required:
+ - compatible
+ - reg
+ - reg-names
+ - power-domains
+ - clocks
+ - interrupts
+ - interrupt-controller
+ - iommus
+ - ranges
+
+additionalProperties: false
+
+examples:
+ - |
+#include
+#include
+#include
+#include
+#include
+
+display-subsystem@ae0 {
+ compatible = "qcom,sc8280xp-mdss";
+ reg = <0x0ae0 0x1000>;
+ reg-names = "mdss";
+
+ power-domains = < MDSS_GDSC>;
+
+ clocks = < GCC_DISP_AHB_CLK>,
+ < DISP_CC_MDSS_AHB_CLK>,
+ < DISP_CC_MDSS_MDP_CLK>;
+ clock-names = "iface",
+"ahb",
+"core";
+
+ assigned-clocks = < DISP_CC_MDSS_MDP_CLK>;
+ assigned-clock-rates = <46000>;
+
+ resets = < DISP_CC_MDSS_CORE_BCR>;
+
+ interrupts = ;
+ interrupt-controller;
+ #interrupt-cells = <1>;
+
+ interconnects = <_noc MASTER_MDP0 0 _virt SLAVE_EBI1 0>,
+ <_noc MASTER_MDP1 0 _virt SLAVE_EBI1 0>;
+ interconnect-names = "mdp0-mem", "mdp1-mem";
+
+ iommus = <_smmu 0x1000 0x402>;
+
+ #address-cells = <1>;
+
