Re: [PATCH v3 5/8] arm64: dts: qcom: Add msm8939 SoC

From: Bryan O'Donoghue
Date: Tue Jan 17 2023 - 18:43:40 EST

On 17/01/2023 20:58, Bjorn Andersson wrote:
On Tue, Jan 17, 2023 at 02:48:43AM +0000, Bryan O'Donoghue wrote:
Add msm8939 a derivative SoC of msm8916. This SoC contains a number of key
differences to msm8916.

- big.LITTLE Octa Core - quad 1.5GHz + quad 1.0GHz
- DRAM 1x800 LPDDR3
- Camera 4+4 lane CSI
- Venus @ 1080p60 HEVC
- DSI x 2
- Adreno A405
- WiFi wcn3660/wcn3680b 802.11ac

Co-developed-by: Shawn Guo <shawn.guo@xxxxxxxxxx>
Signed-off-by: Shawn Guo <shawn.guo@xxxxxxxxxx>
Co-developed-by: Jun Nie <jun.nie@xxxxxxxxxx>
Signed-off-by: Jun Nie <jun.nie@xxxxxxxxxx>
Co-developed-by: Benjamin Li <benl@xxxxxxxxxxxx>
Signed-off-by: Benjamin Li <benl@xxxxxxxxxxxx>
Co-developed-by: James Willcox <jwillcox@xxxxxxxxxxxx>
Signed-off-by: James Willcox <jwillcox@xxxxxxxxxxxx>
Co-developed-by: Leo Yan <leo.yan@xxxxxxxxxx>
Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
Co-developed-by: Joseph Gates <jgates@xxxxxxxxxxxx>
Signed-off-by: Joseph Gates <jgates@xxxxxxxxxxxx>
Co-developed-by: Max Chen <mchen@xxxxxxxxxxxx>
Signed-off-by: Max Chen <mchen@xxxxxxxxxxxx>
Co-developed-by: Zac Crosby <zac@xxxxxxxxxxxx>
Signed-off-by: Zac Crosby <zac@xxxxxxxxxxxx>
Co-developed-by: Vincent Knecht <vincent.knecht@xxxxxxxxxx>
Signed-off-by: Vincent Knecht <vincent.knecht@xxxxxxxxxx>
Co-developed-by: Stephan Gerhold <stephan@xxxxxxxxxxx>
Signed-off-by: Stephan Gerhold <stephan@xxxxxxxxxxx>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>

Just to make sure when I get the question, you all co-developed this
patch, right?

A long list but a fair one.

---
arch/arm64/boot/dts/qcom/msm8939.dtsi | 2393 +++++++++++++++++++++++++
1 file changed, 2393 insertions(+)
create mode 100644 arch/arm64/boot/dts/qcom/msm8939.dtsi

diff --git a/arch/arm64/boot/dts/qcom/msm8939.dtsi b/arch/arm64/boot/dts/qcom/msm8939.dtsi
new file mode 100644
index 0000000000000..8cd358a9fe623
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/msm8939.dtsi
@@ -0,0 +1,2393 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2020-2023, Linaro Limited
+ */
+
+#include <dt-bindings/clock/qcom,gcc-msm8939.h>
+#include <dt-bindings/clock/qcom,rpmcc.h>
+#include <dt-bindings/interconnect/qcom,msm8939.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/power/qcom-rpmpd.h>
+#include <dt-bindings/reset/qcom,gcc-msm8939.h>
+#include <dt-bindings/thermal/thermal.h>
+
+/ {
+ interrupt-parent = <&intc>;
+
+ #address-cells = <2>;
+ #size-cells = <2>;

Why do you use a default of 2? In particular since you reduce it to 1 in
/soc...

You asked that before, and I took a note of the answer but, then because I was away from the main machine when I sent V2, I didn't have the log.

Here's what I wrote down.

" - address-cells/size-cells = 1 in /soc - Bjorn
I experimentally changed address/cell sizes to 2
I'm finding that lk chokes "

So AFAIR LK was unhappy about changing the top level address/size cells to <1> <1> and converting the /soc address/size cells to <2> <2> caused a number of breakages during boot.

To be honest, this pattern is copied from the msm8916.dtsi original. msm8953.dtsi has the same thing. msm8994 too, and 8998.

If you think it needs changing, then I'll have to see what can be done with soc@{} entries.


+
+ clocks {
+ xo_board: xo-board {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <19200000>;
+ };
+
+ sleep_clk: sleep-clk {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <32768>;
+ };
+ };
[..]
+ smp2p-hexagon {

To avoid having people start sending patches that changes the sort order
as soon as I merge this, could you please sort your nodes by address
(not applicable for this one), then by node name alphabetically, then by
label alphabetically.

ah. I sorted the contents of soc. I missed the upper level groupings.


+ compatible = "qcom,smp2p";
+ qcom,smem = <435>, <428>;
+
+ interrupts = <GIC_SPI 27 IRQ_TYPE_EDGE_RISING>;
+
+ mboxes = <&apcs1_mbox 14>;
+
+ qcom,local-pid = <0>;
+ qcom,remote-pid = <1>;
+
+ hexagon_smp2p_out: master-kernel {
+ qcom,entry-name = "master-kernel";
+
+ #qcom,smem-state-cells = <1>;
+ };
+
+ hexagon_smp2p_in: slave-kernel {
+ qcom,entry-name = "slave-kernel";
+
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ #address-cells = <0>;
+ #size-cells = <0>;
+ };
+ };
+
+ memory@80000000 {
+ device_type = "memory";
+ /* We expect the bootloader to fill in the reg */
+ reg = <0x0 0x80000000 0x0 0x0>;
+ };
+
[..]
+ soc: soc@0 {
[..]
+ pronto: remoteproc@a204000 {
+ compatible = "qcom,pronto-v2-pil", "qcom,pronto";
+ reg = <0x0a204000 0x2000>,
+ <0x0a202000 0x1000>,
+ <0x0a21b000 0x3000>;
+ reg-names = "ccu", "dxe", "pmu";
+
+ interrupts-extended = <&intc 0 149 IRQ_TYPE_EDGE_RISING>,
+ <&wcnss_smp2p_in 0 IRQ_TYPE_EDGE_RISING>,
+ <&wcnss_smp2p_in 1 IRQ_TYPE_EDGE_RISING>,
+ <&wcnss_smp2p_in 2 IRQ_TYPE_EDGE_RISING>,
+ <&wcnss_smp2p_in 3 IRQ_TYPE_EDGE_RISING>;
+ interrupt-names = "wdog", "fatal", "ready", "handover", "stop-ack";
+
+ memory-region = <&wcnss_mem>;
+
+ power-domains = <&rpmpd MSM8939_VDDCX>,
+ <&rpmpd MSM8939_VDDMX_AO>;

The purpose of the remoteproc driver's vote is to keep the rails powered
while we're booting the remote, in the event that Linux decides to
suspend and turn of the power rails while we're waiting...

Once the remote pulls the "handover" interrupt, it signals that it has
cast the necessary votes and need no more hand-holding.

So it's unlikely that _AO is the right choice here.

Yes, it's probably just VDDMX isn't it.

I'll change that.

---
bod