| Message ID | 1629282424-4070-1-git-send-email-mkrishn@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/4] dt-bindings: msm: add DT bindings for sc7280 | expand |
On Wed, 18 Aug 2021 15:57:01 +0530, Krishna Manikandan wrote: > MSM Mobile Display Subsystem (MDSS) encapsulates sub-blocks > like DPU display controller, DSI, EDP etc. Add required DPU > device tree bindings for SC7280. > > Signed-off-by: Krishna Manikandan <mkrishn@codeaurora.org> > --- > .../bindings/display/msm/dpu-sc7280.yaml | 228 +++++++++++++++++++++ > 1 file changed, 228 insertions(+) > create mode 100644 Documentation/devicetree/bindings/display/msm/dpu-sc7280.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-sc7280.example.dts:19:18: fatal error: dt-bindings/clock/qcom,dispcc-sc7280.h: No such file or directory 19 | #include <dt-bindings/clock/qcom,dispcc-sc7280.h> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated. make[1]: *** [scripts/Makefile.lib:380: Documentation/devicetree/bindings/display/msm/dpu-sc7280.example.dt.yaml] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1419: dt_binding_check] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1517976 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.
Quoting Krishna Manikandan (2021-08-18 03:27:02) > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi > index 53a21d0..fd7ff1c 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > @@ -5,6 +5,7 @@ > * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved. > */ > > +#include <dt-bindings/clock/qcom,dispcc-sc7280.h> > #include <dt-bindings/clock/qcom,gcc-sc7280.h> > #include <dt-bindings/clock/qcom,rpmh.h> > #include <dt-bindings/interconnect/qcom,sc7280.h> > @@ -1424,6 +1425,90 @@ > #power-domain-cells = <1>; > }; > > + mdss: mdss@ae00000 { subsystem@ae00000 > + compatible = "qcom,sc7280-mdss"; > + reg = <0 0x0ae00000 0 0x1000>; > + reg-names = "mdss"; > + > + power-domains = <&dispcc DISP_CC_MDSS_CORE_GDSC>; > + > + clocks = <&gcc GCC_DISP_AHB_CLK>, > + <&dispcc DISP_CC_MDSS_AHB_CLK>, > + <&dispcc DISP_CC_MDSS_MDP_CLK>; > + clock-names = "iface", "ahb", "core"; > + > + assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>; > + assigned-clock-rates = <300000000>; > + > + interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-controller; > + #interrupt-cells = <1>; > + > + interconnects = <&mmss_noc MASTER_MDP0 0 &mc_virt SLAVE_EBI1 0>; > + interconnect-names = "mdp0-mem"; > + > + iommus = <&apps_smmu 0x900 0x402>; > + > + #address-cells = <2>; > + #size-cells = <2>; > + ranges; > + > + status = "disabled"; > + > + mdp: mdp@ae01000 { display-controller@ae01000 > + compatible = "qcom,sc7280-dpu"; > + reg = <0 0x0ae01000 0 0x8f030>, > + <0 0x0aeb0000 0 0x2008>; > + reg-names = "mdp", "vbif"; > + > + clocks = <&gcc GCC_DISP_HF_AXI_CLK>, > + <&gcc GCC_DISP_SF_AXI_CLK>, > + <&dispcc DISP_CC_MDSS_AHB_CLK>, > + <&dispcc DISP_CC_MDSS_MDP_LUT_CLK>, > + <&dispcc DISP_CC_MDSS_MDP_CLK>, > + <&dispcc DISP_CC_MDSS_VSYNC_CLK>; > + clock-names = "bus", "nrt_bus", "iface", "lut", "core", > + "vsync"; One line per string please. > + assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>, > + <&dispcc DISP_CC_MDSS_VSYNC_CLK>, > + <&dispcc DISP_CC_MDSS_AHB_CLK>; > + assigned-clock-rates = <300000000>, > + <19200000>, > + <19200000>; > + operating-points-v2 = <&mdp_opp_table>; > + power-domains = <&rpmhpd SC7280_CX>; > + > + interrupt-parent = <&mdss>; > + interrupts = <0>; > + > + status = "disabled"; > + > + mdp_opp_table: mdp-opp-table { mdp_opp_table: opp-table { > + compatible = "operating-points-v2"; > + > + opp-200000000 { > + opp-hz = /bits/ 64 <200000000>; > + required-opps = <&rpmhpd_opp_low_svs>; > + }; > + > + opp-300000000 { > + opp-hz = /bits/ 64 <300000000>; > + required-opps = <&rpmhpd_opp_svs>; > + }; > + > + opp-380000000 { > + opp-hz = /bits/ 64 <380000000>; > + required-opps = <&rpmhpd_opp_svs_l1>; > + }; > + > + opp-506666667 { > + opp-hz = /bits/ 64 <506666667>; > + required-opps = <&rpmhpd_opp_nom>; > + }; > + }; > + }; > + }; > +
Quoting Krishna Manikandan (2021-08-18 03:27:04) > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi > index aadf55d..5be318e 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > @@ -1412,7 +1412,7 @@ > reg = <0 0xaf00000 0 0x20000>; > clocks = <&rpmhcc RPMH_CXO_CLK>, > <&gcc GCC_DISP_GPLL0_CLK_SRC>, > - <0>, <0>, <0>, <0>, <0>, <0>; > + <0>, <0>, <0>, <0>, <&edp_phy 0>, <&edp_phy 1>; > clock-names = "bi_tcxo", "gcc_disp_gpll0_clk", > "dsi0_phy_pll_out_byteclk", > "dsi0_phy_pll_out_dsiclk", > @@ -1493,6 +1493,12 @@ > remote-endpoint = <&dsi0_in>; > }; > }; Newline here please. > + port@1 { > + reg = <1>; > + dpu_intf5_out: endpoint { > + remote-endpoint = <&edp_in>; > + }; > + }; > }; > > mdp_opp_table: mdp-opp-table { > @@ -1608,6 +1614,101 @@ > > status = "disabled"; > }; > + > + msm_edp: edp@aea0000 { > + status = "disabled"; Please pick a place to put status disabled. I don't know what qcom maintainers want, but please be consistent. > + compatible = "qcom,sc7280-edp"; > + reg = <0 0xaea0000 0 0x200>, > + <0 0xaea0200 0 0x200>, > + <0 0xaea0400 0 0xc00>, > + <0 0xaea1000 0 0x400>; > + > + interrupt-parent = <&mdss>; > + interrupts = <14 IRQ_TYPE_NONE>; Drop flags. > + > + clocks = <&rpmhcc RPMH_CXO_CLK>, > + <&gcc GCC_EDP_CLKREF_EN>, > + <&dispcc DISP_CC_MDSS_AHB_CLK>, > + <&dispcc DISP_CC_MDSS_EDP_AUX_CLK>, > + <&dispcc DISP_CC_MDSS_EDP_LINK_CLK>, > + <&dispcc DISP_CC_MDSS_EDP_LINK_INTF_CLK>, > + <&dispcc DISP_CC_MDSS_EDP_PIXEL_CLK>; > + clock-names = "core_xo", "core_ref", > + "core_iface", "core_aux", "ctrl_link", > + "ctrl_link_iface", "stream_pixel"; One line per string please. > + #clock-cells = <1>; > + assigned-clocks = <&dispcc DISP_CC_MDSS_EDP_LINK_CLK_SRC>, > + <&dispcc DISP_CC_MDSS_EDP_PIXEL_CLK_SRC>; > + assigned-clock-parents = <&edp_phy 0>, <&edp_phy 1>; > + > + phys = <&edp_phy>; > + phy-names = "dp"; > + > + vdda-1p2-supply = <&vreg_l6b_1p2>; > + vdda-0p9-supply = <&vreg_l10c_0p8>; Can this be done here? Probably needs to move to the board dts/dtsi file. > + operating-points-v2 = <&edp_opp_table>; > + power-domains = <&rpmhpd SC7280_CX>; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&edp_hot_plug_det>, <&edp_panel_power_on>; > + > + panel-bklt-gpio = <&pm8350c_gpios 7 GPIO_ACTIVE_HIGH>; > + panel-pwm-gpio = <&pm8350c_gpios 8 GPIO_ACTIVE_HIGH>; Please no panel-bklt-gpio and panel-pwm-gpio properties. > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + port@0 { > + reg = <0>; > + edp_in: endpoint { > + remote-endpoint = <&dpu_intf5_out>; > + }; > + }; > + }; > + > + edp_opp_table: edp-opp-table { edp_opp_table: opp-table { > + compatible = "operating-points-v2"; > + > + opp-160000000 { > + opp-hz = /bits/ 64 <160000000>; > + required-opps = <&rpmhpd_opp_low_svs>; > + }; > + > + opp-270000000 { > + opp-hz = /bits/ 64 <270000000>; > + required-opps = <&rpmhpd_opp_svs>; > + }; > + > + opp-540000000 { > + opp-hz = /bits/ 64 <540000000>; > + required-opps = <&rpmhpd_opp_nom>; > + }; > + > + opp-810000000 { > + opp-hz = /bits/ 64 <810000000>; > + required-opps = <&rpmhpd_opp_nom>; > + }; > + }; > + }; > + > + edp_phy: phy@aec2000 { Good job on using phy! > + status = "disabled"; > + compatible = "qcom,sc7280-edp-phy"; > + reg = <0 0xaec2a00 0 0x19c>, > + <0 0xaec2200 0 0xa0>, > + <0 0xaec2600 0 0xa0>, > + <0 0xaec2000 0 0x1c0>; > + > + clocks = <&rpmhcc RPMH_CXO_CLK>, > + <&gcc GCC_EDP_CLKREF_EN>; > + clock-names = "aux", "cfg_ahb"; > + > + vdda-pll-supply = <&vreg_l6b_1p2>; > + vdda-phy-supply = <&vreg_l10c_0p8>; Again, still question the ability to put this here vs. in IDP file. > + > + #clock-cells = <1>; > + #phy-cells = <0>; > + }; > }; > > pdc: interrupt-controller@b220000 { > @@ -1704,6 +1805,30 @@ > function = "qup13"; > }; > > + edp_hot_plug_det: edp-hot-plug-det { > + pinmux { > + pins = "gpio60"; > + function = "edp_hot"; > + }; > + pinconf { > + pins = "gpio60"; > + bias-pull-down; > + input-enable; > + }; Move pinconf stuff to board file (and combine nodes as mka stated). > + }; > + > + edp_panel_power_on: edp-panel-power-on { > + pinmux { > + pins = "gpio80"; > + function = "gpio"; > + }; > + pinconf { > + pins = "gpio80"; > + bias-disable; > + output-high; > + }; > + }; > + > sdc1_on: sdc1-on { > clk { > pins = "sdc1_clk"; > -- > 2.7.4 >
On 2021-10-05 07:21, Stephen Boyd wrote: > Quoting mkrishn@codeaurora.org (2021-09-30 23:39:07) >> On 2021-09-30 23:28, Stephen Boyd wrote: >> > Quoting mkrishn@codeaurora.org (2021-09-30 04:56:59) >> >> On 2021-08-19 01:27, Stephen Boyd wrote: >> >> > Quoting Krishna Manikandan (2021-08-18 03:27:02) >> >> >> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi >> >> >> b/arch/arm64/boot/dts/qcom/sc7280.dtsi >> >> >> index 53a21d0..fd7ff1c 100644 >> >> >> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi >> >> >> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi >> >> >> + >> >> >> + status = "disabled"; >> >> >> + >> >> >> + mdp: mdp@ae01000 { >> >> > >> >> > display-controller@ae01000 >> >> >> >> Stephen, >> >> In the current driver code, there is a substring comparison for >> >> "mdp" >> >> in device node name as part of probe sequence. If "mdp" is not present >> >> in the node name, it will >> >> return an error resulting in probe failure. Can we continue using >> >> mdp >> >> as nodename instead of display controller? >> >> >> > >> > Can we fix the driver to not look for node names and look for >> > compatible >> > strings instead? It took me a minute to find compare_name_mdp() in >> > drivers/gpu/drm/msm/msm_drv.c to understand what you're talking about. >> > Perhaps looking for qcom,mdp5 in there will be sufficient instead of >> > looking at the node name. >> >> Sure Stephen. I will make the necessary changes in msm_drv.c to look >> for >> compatible string instead of node name. >> Can I include these two changes (changing mdp--> display controller >> and >> msm_drv.c changes) in a separate series ? >> > > Sure. So you'll send the drm driver change now and we'll get the DT > change after that with the more generic node name? Yes Stephen.I have raised the change to fix the driver issue. https://patchwork.kernel.org/project/linux-arm-msm/patch/1633427071-19523-1-git-send-email-mkrishn@codeaurora.org/ Regards, Krishna
diff --git a/Documentation/devicetree/bindings/display/msm/dpu-sc7280.yaml b/Documentation/devicetree/bindings/display/msm/dpu-sc7280.yaml new file mode 100644 index 0000000..3d256c0 --- /dev/null +++ b/Documentation/devicetree/bindings/display/msm/dpu-sc7280.yaml @@ -0,0 +1,228 @@ +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/msm/dpu-sc7280.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm Display DPU dt properties for SC7280 target + +maintainers: + - Krishna Manikandan <mkrishn@codeaurora.org> + +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 SC7280 target. + +properties: + compatible: + items: + - const: qcom,sc7280-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: + items: + - description: Interconnect path specifying the port ids for data bus + + interconnect-names: + const: mdp0-mem + +patternProperties: + "^display-controller@[0-9a-f]+$": + type: object + description: Node containing the properties of DPU. + + properties: + compatible: + items: + - const: qcom,sc7280-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. + + properties: + port@0: + $ref: /schemas/graph.yaml#/properties/port + description: DPU_INTF1 (DSI) + + port@1: + $ref: /schemas/graph.yaml#/properties/port + description: DPU_INTF5 (EDP) + + required: + - port@0 + + 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 <dt-bindings/clock/qcom,dispcc-sc7280.h> + #include <dt-bindings/clock/qcom,gcc-sc7280.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/interconnect/qcom,sc7280.h> + #include <dt-bindings/power/qcom-rpmpd.h> + + display-subsystem@ae00000 { + #address-cells = <1>; + #size-cells = <1>; + compatible = "qcom,sc7280-mdss"; + reg = <0xae00000 0x1000>; + reg-names = "mdss"; + power-domains = <&dispcc DISP_CC_MDSS_CORE_GDSC>; + clocks = <&gcc GCC_DISP_AHB_CLK>, + <&dispcc DISP_CC_MDSS_AHB_CLK>, + <&dispcc DISP_CC_MDSS_MDP_CLK>; + clock-names = "iface", "ahb", "core"; + + interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>; + interrupt-controller; + #interrupt-cells = <1>; + + interconnects = <&mmss_noc MASTER_MDP0 &mc_virt SLAVE_EBI1>; + interconnect-names = "mdp0-mem"; + + iommus = <&apps_smmu 0x900 0x402>; + ranges; + + display-controller@ae01000 { + compatible = "qcom,sc7280-dpu"; + reg = <0x0ae01000 0x8f000>, + <0x0aeb0000 0x2008>; + + reg-names = "mdp", "vbif"; + + clocks = <&gcc GCC_DISP_HF_AXI_CLK>, + <&gcc GCC_DISP_SF_AXI_CLK>, + <&dispcc DISP_CC_MDSS_AHB_CLK>, + <&dispcc DISP_CC_MDSS_MDP_LUT_CLK>, + <&dispcc DISP_CC_MDSS_MDP_CLK>, + <&dispcc DISP_CC_MDSS_VSYNC_CLK>; + clock-names = "bus", "nrt_bus", "iface", "lut", "core", + "vsync"; + + interrupt-parent = <&mdss>; + interrupts = <0>; + power-domains = <&rpmhpd SC7280_CX>; + operating-points-v2 = <&mdp_opp_table>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + dpu_intf1_out: endpoint { + remote-endpoint = <&dsi0_in>; + }; + }; + + port@1 { + reg = <1>; + dpu_intf5_out: endpoint { + remote-endpoint = <&edp_in>; + }; + }; + }; + }; + }; +...
MSM Mobile Display Subsystem (MDSS) encapsulates sub-blocks like DPU display controller, DSI, EDP etc. Add required DPU device tree bindings for SC7280. Signed-off-by: Krishna Manikandan <mkrishn@codeaurora.org> --- .../bindings/display/msm/dpu-sc7280.yaml | 228 +++++++++++++++++++++ 1 file changed, 228 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/msm/dpu-sc7280.yaml