| Message ID | 1607670585-26438-2-git-send-email-mkrishn@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v9,1/2] dt-bindings: msm: disp: add yaml schemas for DPU and DSI bindings | expand |
On 2020-12-11 14:36, Stephen Boyd wrote: > Quoting Krishna Manikandan (2020-12-10 23:09:45) >> Changes in v10: >> - Change title of this patch as it does not contain PLL bindings >> anymore >> - Remove redundant properties >> - Remove use of IRQ flag >> - Fix ports property >> >> Previous Change log: >> https://lkml.kernel.org/lkml/1597066683-6044-2-git-send-email-mkrishn@codeaurora.org/ > > Why can't that be put in here instead of making me click the link? > Thanks Stephen for reviews. I just wanted to use space efficiently. But if it helps in reviews, I will bring all the previous change logs back. >> diff --git >> a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml >> b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml >> new file mode 100644 >> index 0000000..bc80632 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml >> @@ -0,0 +1,120 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > Why two spaces between 'only and 'OR'? > >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/display/msm/dp-controller.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: MSM Display Port Controller. > > Drop the full-stop? > >> + >> +maintainers: >> + - Chandan Uddaraju <chandanu@codeaurora.org> >> + - Vara Reddy <varar@codeaurora.org> >> + - Tanmay Shah <tanmay@codeaurora.org> >> + >> +description: | >> + Device tree bindings for DisplayPort host controller for MSM >> targets >> + that are compatible with VESA DisplayPort interface specification. >> + >> +properties: >> + compatible: >> + enum: >> + - qcom,sc7180-dp >> + >> + reg: >> + maxItems: 1 >> + >> + interrupts: >> + maxItems: 1 >> + >> + clocks: >> + items: >> + - description: AHB clock to enable register access >> + - description: Display Port AUX clock >> + - description: Display Port Link clock >> + - description: Link interface clock between DP and PHY >> + - description: Display Port Pixel clock >> + >> + clock-names: >> + items: >> + - const: core_iface >> + - const: core_aux >> + - const: ctrl_link >> + - const: ctrl_link_iface >> + - const: stream_pixel > > There should be a dp-phy too, so 'phy' and 'phy-names' property. I see > a > 'power-domains' property downstream, so please add that too. And also > '#sound-dai-cells". > >> + >> + ports: >> + type: object >> + description: | >> + A ports node with endpoint definitions as defined in >> + Documentation/devicetree/bindings/media/video-interfaces.txt. >> + properties: >> + "#address-cells": >> + const: 1 >> + >> + "#size-cells": >> + const: 0 >> + >> + port@0: >> + type: object >> + description: Input endpoint of the controller. >> + >> + port@1: >> + type: object >> + description: Output endpoint of the controller. > > This should have a required section too, and state that all the above > properties are required. > >> + >> + additionalProperties: false >> + >> +required: >> + - compatible >> + - reg >> + - interrupts >> + - clocks >> + - clock-names >> + - ports >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + #include <dt-bindings/clock/qcom,dispcc-sc7180.h> >> + >> + mdss: mdss { > > Can drop the label. Also this node name is bad! Should be > 'subsystem@<reg>' I guess? And then the reg can just be the first > address in a reg property that is remapped for the children nodes. > I didn't get this part: "And then the reg can just be the first address in a reg property that is remapped for children nodes." Does this mean, I should add reg property for mdss node as well? Node mdss address is 0x0ae00000. So something like: reg = <0 0x0ae00000 0 0x1000>; >> + #address-cells = <2>; >> + #size-cells = <2>; >> + interrupt-controller; >> + >> + msm_dp: displayport-controller@ae90000 { > > Can drop the label. > >> + compatible = "qcom,sc7180-dp"; >> + reg = <0 0xae90000 0 0x1400>; >> + interrupt-parent = <&mdss>; >> + interrupts = <12>; >> + clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>, >> + <&dispcc DISP_CC_MDSS_DP_AUX_CLK>, >> + <&dispcc DISP_CC_MDSS_DP_LINK_CLK>, >> + <&dispcc DISP_CC_MDSS_DP_LINK_INTF_CLK>, >> + <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK>; >> + clock-names = "core_iface", "core_aux", >> + "ctrl_link", >> + "ctrl_link_iface", "stream_pixel"; >> + >> + ports { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + port@0 { >> + reg = <0>; >> + dp_in: endpoint { > > Can drop the label. > >> + remote-endpoint = <&dpu_intf0_out>; >> + }; >> + }; >> + >> + port@1 { >> + reg = <1>; >> + dp_out: endpoint { > > Can drop the label and show some remote-endpoint connection? > I am not sure what should I include here as connection. IIRC, Robh suggested It should connect to the Type-C connector through some sort of mixing node. However, I see only typec connector node at: Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml. Should we use that directly or should we use ec node? Can we do something like: remote-endpoint = <&typec> or <&ec> Rest of the comments looks good and I will address them in next patch. Thanks. >> + }; >> + }; >> + }; >> + }; >> + }; >> +...
Quoting Tanmay Shah (2020-12-17 14:52:54) > On 2020-12-11 14:36, Stephen Boyd wrote: > > Quoting Krishna Manikandan (2020-12-10 23:09:45) > >> Changes in v10: > >> - Change title of this patch as it does not contain PLL bindings > >> anymore > >> - Remove redundant properties > >> - Remove use of IRQ flag > >> - Fix ports property > >> > >> Previous Change log: > >> https://lkml.kernel.org/lkml/1597066683-6044-2-git-send-email-mkrishn@codeaurora.org/ > > > > Why can't that be put in here instead of making me click the link? > > > > Thanks Stephen for reviews. > > I just wanted to use space efficiently. But if it helps in reviews, > I will bring all the previous change logs back. Or it can be left under the cut (---). This is probably going to merge through the binding tree so it doesn't follow drm practice of putting version history in the commit text. > > >> diff --git > >> a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > >> b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > >> new file mode 100644 > >> index 0000000..bc80632 > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml [...] > >> + - | > >> + #include <dt-bindings/interrupt-controller/arm-gic.h> > >> + #include <dt-bindings/clock/qcom,dispcc-sc7180.h> > >> + > >> + mdss: mdss { > > > > Can drop the label. Also this node name is bad! Should be > > 'subsystem@<reg>' I guess? And then the reg can just be the first > > address in a reg property that is remapped for the children nodes. > > > > I didn't get this part: > "And then the reg can just be the first address in a reg property that > is remapped for children nodes." > > Does this mean, I should add reg property for mdss node as well? Node > mdss address is 0x0ae00000. So something like: > > reg = <0 0x0ae00000 0 0x1000>; > Yes and then a ranges property so that child nodes can figure out their reg property appropriately. > > > >> + #address-cells = <2>; > >> + #size-cells = <2>; > >> + interrupt-controller; [...] > >> + remote-endpoint = <&dpu_intf0_out>; > >> + }; > >> + }; > >> + > >> + port@1 { > >> + reg = <1>; > >> + dp_out: endpoint { > > > > Can drop the label and show some remote-endpoint connection? > > > > I am not sure what should I include here as connection. IIRC, Robh > suggested It should connect to the Type-C connector through some sort of > mixing node. However, I see only typec connector node at: > Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml. > Should we use that directly or should we use ec node? > > Can we do something like: > > remote-endpoint = <&typec> or <&ec> Sure. I suppose <&typec> would work.
diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml new file mode 100644 index 0000000..bc80632 --- /dev/null +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml @@ -0,0 +1,120 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/msm/dp-controller.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: MSM Display Port Controller. + +maintainers: + - Chandan Uddaraju <chandanu@codeaurora.org> + - Vara Reddy <varar@codeaurora.org> + - Tanmay Shah <tanmay@codeaurora.org> + +description: | + Device tree bindings for DisplayPort host controller for MSM targets + that are compatible with VESA DisplayPort interface specification. + +properties: + compatible: + enum: + - qcom,sc7180-dp + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + clocks: + items: + - description: AHB clock to enable register access + - description: Display Port AUX clock + - description: Display Port Link clock + - description: Link interface clock between DP and PHY + - description: Display Port Pixel clock + + clock-names: + items: + - const: core_iface + - const: core_aux + - const: ctrl_link + - const: ctrl_link_iface + - const: stream_pixel + + ports: + type: object + description: | + A ports node with endpoint definitions as defined in + Documentation/devicetree/bindings/media/video-interfaces.txt. + properties: + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + + port@0: + type: object + description: Input endpoint of the controller. + + port@1: + type: object + description: Output endpoint of the controller. + + additionalProperties: false + +required: + - compatible + - reg + - interrupts + - clocks + - clock-names + - ports + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/clock/qcom,dispcc-sc7180.h> + + mdss: mdss { + #address-cells = <2>; + #size-cells = <2>; + interrupt-controller; + + msm_dp: displayport-controller@ae90000 { + compatible = "qcom,sc7180-dp"; + reg = <0 0xae90000 0 0x1400>; + interrupt-parent = <&mdss>; + interrupts = <12>; + clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>, + <&dispcc DISP_CC_MDSS_DP_AUX_CLK>, + <&dispcc DISP_CC_MDSS_DP_LINK_CLK>, + <&dispcc DISP_CC_MDSS_DP_LINK_INTF_CLK>, + <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK>; + clock-names = "core_iface", "core_aux", + "ctrl_link", + "ctrl_link_iface", "stream_pixel"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + dp_in: endpoint { + remote-endpoint = <&dpu_intf0_out>; + }; + }; + + port@1 { + reg = <1>; + dp_out: endpoint { + }; + }; + }; + }; + }; +... diff --git a/Documentation/devicetree/bindings/display/msm/dpu-sc7180.yaml b/Documentation/devicetree/bindings/display/msm/dpu-sc7180.yaml index 8397ea8..9549d8f 100644 --- a/Documentation/devicetree/bindings/display/msm/dpu-sc7180.yaml +++ b/Documentation/devicetree/bindings/display/msm/dpu-sc7180.yaml @@ -132,6 +132,9 @@ patternProperties: port@1: type: object description: DPU_INTF2 (DSI2) + port@2: + type: object + description: DPU_INTF0 (DP) assigned-clocks: maxItems: 4 @@ -228,6 +231,13 @@ examples: remote-endpoint = <&dsi0_in>; }; }; + + port@2 { + reg = <2>; + dpu_intf0_out: endpoint { + remote-endpoint = <&dp_in>; + }; + }; }; }; };