| Message ID | 1629282424-4070-4-git-send-email-mkrishn@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v1,1/4] dt-bindings: msm: add DT bindings for sc7280 | expand |
On Wed, Aug 18, 2021 at 03:57:04PM +0530, Krishna Manikandan wrote: > From: Sankeerth Billakanti <sbillaka@codeaurora.org> > > Add edp controller and phy DT nodes for sc7280. > > Signed-off-by: Sankeerth Billakanti <sbillaka@codeaurora.org> > --- > arch/arm64/boot/dts/qcom/sc7280.dtsi | 127 ++++++++++++++++++++++++++++++++++- > 1 file changed, 126 insertions(+), 1 deletion(-) > > 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>; > }; > }; > + 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"; > + 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>; > + > + 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"; > + #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>; These regulators are defined in the board .dts (sc7280-idp.dts), hence the SoC .dtsi shouldn't depend on them. My impression is that pm7325.dtsi and pm8350c.dtsi should include definitions for regulators that supply basic SoC blocks. If the configuration can vary depending on the SoC there could be SoC specific includes for each PMIC. If a board uses a different configuration it could overwrite the PMIC .dtsi settings. > + 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>; The pins are board specific, hence they shouldn't be configured in the .dtsi of the SoC. > + > + 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 { > + 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 { > + 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>; > + > + #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; > + }; There should be no separate 'pinmux' and 'pinconf' nodes, see other entries in this section. > + }; > + > + edp_panel_power_on: edp-panel-power-on { > + pinmux { > + pins = "gpio80"; > + function = "gpio"; > + }; > + pinconf { > + pins = "gpio80"; > + bias-disable; > + output-high; > + }; > + }; IIUC this could be any GPIO and hence should be configured by the board file.
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 >
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>; }; }; + 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"; + 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>; + + 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"; + #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>; + 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>; + + 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 { + 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 { + 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>; + + #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; + }; + }; + + 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";