diff mbox series

[v5,1/3] arm64: dts: qcom: sdm845: Add dpu to sdm845 dts file

Message ID 1543876054-12998-1-git-send-email-jsanka@codeaurora.org (mailing list archive)
State Not Applicable, archived
Headers show
Series [v5,1/3] arm64: dts: qcom: sdm845: Add dpu to sdm845 dts file | expand

Commit Message

Jeykumar Sankaran Dec. 3, 2018, 10:27 p.m. UTC 
DPU is short for the Display Processing Unit. It is the display
controller on Qualcomm SDM845 chips.

This change adds MDSS and DSI nodes to enable display on the
target device.

Changes in v2:
	 - Beefed up commit message
 	 - Use SoC specific compatibles for mdss and dpu (Rob H)
	 - Use assigned-clocks to set initial clock frequency(Rob H)
Changes in v3:
	 - added IOMMU node
 	 - Fix device naming (remove _phys)
	 - Use correct IRQ_TYPE in interrupt specifiers
Changes in v4:
 	 - move mdss node to preserve the unit address sort order
 	 - remove _clk suffix from dsi clocks
	 (both the comments are from Doug Anderson)
Changes in v5:
	- Keep the device status "disabled" by default (Bjorn Andersson)
	- Use MDSS_GDSC macro (Jordan)
	- Fix phy-names (Jordan)
	- List reg ranges in numerical order (Jordan)

Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 203 +++++++++++++++++++++++++++++++++++
 1 file changed, 203 insertions(+)

Comments

Doug Anderson Dec. 4, 2018, 12:57 a.m. UTC | #1 
Hi,

On Mon, Dec 3, 2018 at 2:27 PM Jeykumar Sankaran <jsanka@codeaurora.org> wrote:
> +                       dsi0: dsi@ae94000 {
> +                               compatible = "qcom,mdss-dsi-ctrl";
> +                               reg = <0xae94000 0x400>;
> +                               reg-names = "dsi_ctrl";
> +
> +                               interrupt-parent = <&mdss>;
> +                               interrupts = <4 IRQ_TYPE_LEVEL_HIGH>;
> +
> +                               clocks = <&dispcc DISP_CC_MDSS_BYTE0_CLK>,
> +                                        <&dispcc DISP_CC_MDSS_BYTE0_INTF_CLK>,
> +                                        <&dispcc DISP_CC_MDSS_PCLK0_CLK>,
> +                                        <&dispcc DISP_CC_MDSS_ESC0_CLK>,
> +                                        <&dispcc DISP_CC_MDSS_AHB_CLK>,
> +                                        <&dispcc DISP_CC_MDSS_AXI_CLK>;
> +                               clock-names = "byte",
> +                                             "byte_intf",
> +                                             "pixel",
> +                                             "core",
> +                                             "iface",
> +                                             "bus";
> +
> +                               phys = <&dsi0_phy>;
> +                               phy-names = "dsi0";

I'm pretty sure that this should just be "dsi" and the one below in
dsi1 should also be called "dsi".  +Jordan should confirm.


> +                       dsi1: dsi@ae96000 {
> +                               compatible = "qcom,mdss-dsi-ctrl";
> +                               reg = <0xae96000 0x400>;
> +                               reg-names = "dsi_ctrl";
> +
> +                               interrupt-parent = <&mdss>;
> +                               interrupts = <5 IRQ_TYPE_LEVEL_HIGH>;
> +
> +                               clocks = <&dispcc DISP_CC_MDSS_BYTE1_CLK>,
> +                                        <&dispcc DISP_CC_MDSS_BYTE1_INTF_CLK>,
> +                                        <&dispcc DISP_CC_MDSS_PCLK1_CLK>,
> +                                        <&dispcc DISP_CC_MDSS_ESC1_CLK>,
> +                                        <&dispcc DISP_CC_MDSS_AHB_CLK>,
> +                                        <&dispcc DISP_CC_MDSS_AXI_CLK>;
> +                               clock-names = "byte",
> +                                             "byte_intf",
> +                                             "pixel",
> +                                             "core",
> +                                             "iface",
> +                                             "bus";
> +
> +                               phys = <&dsi1_phy>;
> +                               phy-names = "dsi1";
> +
> +                               status = "disabled";

This "disabled" is causing me problems.  I don't actually need "dsi1"
but if I don't enable "dsi1" then my display doesn't come up.  :(  I
ran out of time to debug but I wonder if this is this the standard
thing where DRM needs to wait for all the components to probe until it
can finish?  If nobody on this list just knows I'll dig tomorrow and
confirm that my memory isn't faulty and see what we've done about this
in the past.



One last note: it's pretty weird that you sent out only 1/3 and not
2/3 and 3/3.  If you're not ready to send out MTP stuff yet then you
should send out v6 as just a singleton patch.


-Doug
Jeykumar Sankaran Dec. 4, 2018, 2:41 a.m. UTC | #2 
On 2018-12-03 16:57, Doug Anderson wrote:
> Hi,
> 
> On Mon, Dec 3, 2018 at 2:27 PM Jeykumar Sankaran 
> <jsanka@codeaurora.org>
> wrote:
>> +                       dsi0: dsi@ae94000 {
>> +                               compatible = "qcom,mdss-dsi-ctrl";
>> +                               reg = <0xae94000 0x400>;
>> +                               reg-names = "dsi_ctrl";
>> +
>> +                               interrupt-parent = <&mdss>;
>> +                               interrupts = <4 IRQ_TYPE_LEVEL_HIGH>;
>> +
>> +                               clocks = <&dispcc 
>> DISP_CC_MDSS_BYTE0_CLK>,
>> +                                        <&dispcc
>> DISP_CC_MDSS_BYTE0_INTF_CLK>,
>> +                                        <&dispcc 
>> DISP_CC_MDSS_PCLK0_CLK>,
>> +                                        <&dispcc 
>> DISP_CC_MDSS_ESC0_CLK>,
>> +                                        <&dispcc 
>> DISP_CC_MDSS_AHB_CLK>,
>> +                                        <&dispcc 
>> DISP_CC_MDSS_AXI_CLK>;
>> +                               clock-names = "byte",
>> +                                             "byte_intf",
>> +                                             "pixel",
>> +                                             "core",
>> +                                             "iface",
>> +                                             "bus";
>> +
>> +                               phys = <&dsi0_phy>;
>> +                               phy-names = "dsi0";
> 
> I'm pretty sure that this should just be "dsi" and the one below in
> dsi1 should also be called "dsi".  +Jordan should confirm.
> 
Makes sense. I can fix that!
> 
>> +                       dsi1: dsi@ae96000 {
>> +                               compatible = "qcom,mdss-dsi-ctrl";
>> +                               reg = <0xae96000 0x400>;
>> +                               reg-names = "dsi_ctrl";
>> +
>> +                               interrupt-parent = <&mdss>;
>> +                               interrupts = <5 IRQ_TYPE_LEVEL_HIGH>;
>> +
>> +                               clocks = <&dispcc 
>> DISP_CC_MDSS_BYTE1_CLK>,
>> +                                        <&dispcc
>> DISP_CC_MDSS_BYTE1_INTF_CLK>,
>> +                                        <&dispcc 
>> DISP_CC_MDSS_PCLK1_CLK>,
>> +                                        <&dispcc 
>> DISP_CC_MDSS_ESC1_CLK>,
>> +                                        <&dispcc 
>> DISP_CC_MDSS_AHB_CLK>,
>> +                                        <&dispcc 
>> DISP_CC_MDSS_AXI_CLK>;
>> +                               clock-names = "byte",
>> +                                             "byte_intf",
>> +                                             "pixel",
>> +                                             "core",
>> +                                             "iface",
>> +                                             "bus";
>> +
>> +                               phys = <&dsi1_phy>;
>> +                               phy-names = "dsi1";
>> +
>> +                               status = "disabled";
> 
> This "disabled" is causing me problems.  I don't actually need "dsi1"
> but if I don't enable "dsi1" then my display doesn't come up.  :(  I
> ran out of time to debug but I wonder if this is this the standard
> thing where DRM needs to wait for all the components to probe until it
> can finish?  If nobody on this list just knows I'll dig tomorrow and
> confirm that my memory isn't faulty and see what we've done about this
> in the past.
> 
https://patchwork.kernel.org/patch/10467895/

Can you try out with this change (reviewed but not merged yet). It 
validates
the nodes before adding to the DSI list.
> 
> 
> One last note: it's pretty weird that you sent out only 1/3 and not
> 2/3 and 3/3.  If you're not ready to send out MTP stuff yet then you
> should send out v6 as just a singleton patch.
Yes. I was trying to separate this one out as an independent change. 
Sandeep
is working on the comments on removing the pinctrl nodes and updated
mtp nodes. He should be posting 2/3 and 3/3 in the next couple of days.

Thanks,
Jeykumar S.

> 
> 
> -Doug
Doug Anderson Dec. 4, 2018, 6:08 p.m. UTC | #3 
Hi,

On Mon, Dec 3, 2018 at 6:41 PM Jeykumar Sankaran <jsanka@codeaurora.org> wrote:
> >> +                       dsi1: dsi@ae96000 {
> >> +                               compatible = "qcom,mdss-dsi-ctrl";
> >> +                               reg = <0xae96000 0x400>;
> >> +                               reg-names = "dsi_ctrl";
> >> +
> >> +                               interrupt-parent = <&mdss>;
> >> +                               interrupts = <5 IRQ_TYPE_LEVEL_HIGH>;
> >> +
> >> +                               clocks = <&dispcc
> >> DISP_CC_MDSS_BYTE1_CLK>,
> >> +                                        <&dispcc
> >> DISP_CC_MDSS_BYTE1_INTF_CLK>,
> >> +                                        <&dispcc
> >> DISP_CC_MDSS_PCLK1_CLK>,
> >> +                                        <&dispcc
> >> DISP_CC_MDSS_ESC1_CLK>,
> >> +                                        <&dispcc
> >> DISP_CC_MDSS_AHB_CLK>,
> >> +                                        <&dispcc
> >> DISP_CC_MDSS_AXI_CLK>;
> >> +                               clock-names = "byte",
> >> +                                             "byte_intf",
> >> +                                             "pixel",
> >> +                                             "core",
> >> +                                             "iface",
> >> +                                             "bus";
> >> +
> >> +                               phys = <&dsi1_phy>;
> >> +                               phy-names = "dsi1";
> >> +
> >> +                               status = "disabled";
> >
> > This "disabled" is causing me problems.  I don't actually need "dsi1"
> > but if I don't enable "dsi1" then my display doesn't come up.  :(  I
> > ran out of time to debug but I wonder if this is this the standard
> > thing where DRM needs to wait for all the components to probe until it
> > can finish?  If nobody on this list just knows I'll dig tomorrow and
> > confirm that my memory isn't faulty and see what we've done about this
> > in the past.
> >
> https://patchwork.kernel.org/patch/10467895/
>
> Can you try out with this change (reviewed but not merged yet). It
> validates
> the nodes before adding to the DSI list.

No, that doesn't fix it.  I also don't see your printout.

OK, found the problem and posted a patch.  See
<https://lkml.kernel.org/r/20181204180441.218160-1-dianders@chromium.org>.
Please test and review if you are able.


> > One last note: it's pretty weird that you sent out only 1/3 and not
> > 2/3 and 3/3.  If you're not ready to send out MTP stuff yet then you
> > should send out v6 as just a singleton patch.
> Yes. I was trying to separate this one out as an independent change.
> Sandeep
> is working on the comments on removing the pinctrl nodes and updated
> mtp nodes. He should be posting 2/3 and 3/3 in the next couple of days.

OK, good to know.  Probably 2/3 and 3/3 will be squashed anyway since
(as I suggested in the review) they should both be touching the MTP
device tree file.

...in any case, you should send yours out as a singleton patch and
then you don't have to guess how many patches might or might not be
sent out later.  Sandeep can send out his patch and say it depends on
yours.

-Doug
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 1419b00..a16f1fe 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1256,6 +1256,209 @@ 
 			};
 		};
 
+		mdss: mdss@ae00000 {
+			compatible = "qcom,sdm845-mdss";
+			reg = <0xae00000 0x1000>;
+			reg-names = "mdss";
+
+			power-domains = <&dispcc MDSS_GDSC>;
+
+			clocks = <&gcc GCC_DISP_AHB_CLK>,
+				 <&gcc GCC_DISP_AXI_CLK>,
+				 <&dispcc DISP_CC_MDSS_MDP_CLK>;
+			clock-names = "iface", "bus", "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>;
+
+			iommus = <&apps_smmu 0x880 0x8>,
+			         <&apps_smmu 0xc80 0x8>;
+
+			status = "disabled";
+
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+
+			mdss_mdp: mdp@ae01000 {
+				compatible = "qcom,sdm845-dpu";
+				reg = <0x0ae01000 0x8f000>,
+				      <0x0aeb0000 0x2008>;
+				reg-names = "mdp", "vbif";
+
+				clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
+					 <&dispcc DISP_CC_MDSS_AXI_CLK>,
+					 <&dispcc DISP_CC_MDSS_MDP_CLK>,
+					 <&dispcc DISP_CC_MDSS_VSYNC_CLK>;
+				clock-names = "iface", "bus", "core", "vsync";
+
+				assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>,
+						  <&dispcc DISP_CC_MDSS_VSYNC_CLK>;
+				assigned-clock-rates = <300000000>,
+						       <19200000>;
+
+				interrupt-parent = <&mdss>;
+				interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+
+				status = "disabled";
+
+				ports {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					port@0 {
+						reg = <0>;
+						dpu_intf1_out: endpoint {
+							remote-endpoint = <&dsi0_in>;
+						};
+					};
+
+					port@1 {
+						reg = <1>;
+						dpu_intf2_out: endpoint {
+							remote-endpoint = <&dsi1_in>;
+						};
+					};
+				};
+			};
+
+			dsi0: dsi@ae94000 {
+				compatible = "qcom,mdss-dsi-ctrl";
+				reg = <0xae94000 0x400>;
+				reg-names = "dsi_ctrl";
+
+				interrupt-parent = <&mdss>;
+				interrupts = <4 IRQ_TYPE_LEVEL_HIGH>;
+
+				clocks = <&dispcc DISP_CC_MDSS_BYTE0_CLK>,
+					 <&dispcc DISP_CC_MDSS_BYTE0_INTF_CLK>,
+					 <&dispcc DISP_CC_MDSS_PCLK0_CLK>,
+					 <&dispcc DISP_CC_MDSS_ESC0_CLK>,
+					 <&dispcc DISP_CC_MDSS_AHB_CLK>,
+					 <&dispcc DISP_CC_MDSS_AXI_CLK>;
+				clock-names = "byte",
+					      "byte_intf",
+					      "pixel",
+					      "core",
+					      "iface",
+					      "bus";
+
+				phys = <&dsi0_phy>;
+				phy-names = "dsi0";
+
+				status = "disabled";
+
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				ports {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					port@0 {
+						reg = <0>;
+						dsi0_in: endpoint {
+							remote-endpoint = <&dpu_intf1_out>;
+						};
+					};
+
+					port@1 {
+						reg = <1>;
+						dsi0_out: endpoint {
+						};
+					};
+				};
+			};
+
+			dsi0_phy: dsi-phy@ae94400 {
+				compatible = "qcom,dsi-phy-10nm";
+				reg = <0xae94400 0x200>,
+				      <0xae94600 0x280>,
+				      <0xae94a00 0x1e0>;
+				reg-names = "dsi_phy",
+					    "dsi_phy_lane",
+					    "dsi_pll";
+
+				#clock-cells = <1>;
+				#phy-cells = <0>;
+
+				clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>;
+				clock-names = "iface";
+
+				status = "disabled";
+			};
+
+			dsi1: dsi@ae96000 {
+				compatible = "qcom,mdss-dsi-ctrl";
+				reg = <0xae96000 0x400>;
+				reg-names = "dsi_ctrl";
+
+				interrupt-parent = <&mdss>;
+				interrupts = <5 IRQ_TYPE_LEVEL_HIGH>;
+
+				clocks = <&dispcc DISP_CC_MDSS_BYTE1_CLK>,
+					 <&dispcc DISP_CC_MDSS_BYTE1_INTF_CLK>,
+					 <&dispcc DISP_CC_MDSS_PCLK1_CLK>,
+					 <&dispcc DISP_CC_MDSS_ESC1_CLK>,
+					 <&dispcc DISP_CC_MDSS_AHB_CLK>,
+					 <&dispcc DISP_CC_MDSS_AXI_CLK>;
+				clock-names = "byte",
+					      "byte_intf",
+					      "pixel",
+					      "core",
+					      "iface",
+					      "bus";
+
+				phys = <&dsi1_phy>;
+				phy-names = "dsi1";
+
+				status = "disabled";
+
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				ports {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					port@0 {
+						reg = <0>;
+						dsi1_in: endpoint {
+							remote-endpoint = <&dpu_intf2_out>;
+						};
+					};
+
+					port@1 {
+						reg = <1>;
+						dsi1_out: endpoint {
+						};
+					};
+				};
+			};
+
+			dsi1_phy: dsi-phy@ae96400 {
+				compatible = "qcom,dsi-phy-10nm";
+				reg = <0xae96400 0x200>,
+				      <0xae96600 0x280>,
+				      <0xae96a00 0x10e>;
+				reg-names = "dsi_phy",
+					    "dsi_phy_lane",
+					    "dsi_pll";
+
+				#clock-cells = <1>;
+				#phy-cells = <0>;
+
+				clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>;
+				clock-names = "iface";
+
+				status = "disabled";
+			};
+		};
+
 		dispcc: clock-controller@af00000 {
 			compatible = "qcom,sdm845-dispcc";
 			reg = <0xaf00000 0x10000>;