Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <28e1df7a-6577-bf39-9739-d0a047b36f12@linaro.org>
Date:   Tue, 17 Jan 2023 22:48:37 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Subject: Re: [PATCH v3 5/8] arm64: dts: qcom: Add msm8939 SoC
Content-Language: en-US
To:     Bjorn Andersson <andersson@kernel.org>
Cc:     agross@kernel.org, konrad.dybcio@linaro.org, djakov@kernel.org,
        robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        linux-arm-msm@vger.kernel.org, linux-pm@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        benl@squareup.com, shawn.guo@linaro.org, fabien.parent@linaro.org,
        leo.yan@linaro.org, dmitry.baryshkov@linaro.org,
        Jun Nie <jun.nie@linaro.org>,
        James Willcox <jwillcox@squareup.com>,
        Joseph Gates <jgates@squareup.com>,
        Max Chen <mchen@squareup.com>, Zac Crosby <zac@squareup.com>,
        Vincent Knecht <vincent.knecht@mailoo.org>,
        Stephan Gerhold <stephan@gerhold.net>
References: <20230117024846.1367794-1-bryan.odonoghue@linaro.org>
 <20230117024846.1367794-6-bryan.odonoghue@linaro.org>
 <20230117205800.cqexxwxmtupapy7e@builder.lan>
From:   Bryan O'Donoghue <bryan.odonoghue@linaro.org>
In-Reply-To: <20230117205800.cqexxwxmtupapy7e@builder.lan>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@linaro.org>
>> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
>> Co-developed-by: Jun Nie <jun.nie@linaro.org>
>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>> Co-developed-by: Benjamin Li <benl@squareup.com>
>> Signed-off-by: Benjamin Li <benl@squareup.com>
>> Co-developed-by: James Willcox <jwillcox@squareup.com>
>> Signed-off-by: James Willcox <jwillcox@squareup.com>
>> Co-developed-by: Leo Yan <leo.yan@linaro.org>
>> Signed-off-by: Leo Yan <leo.yan@linaro.org>
>> Co-developed-by: Joseph Gates <jgates@squareup.com>
>> Signed-off-by: Joseph Gates <jgates@squareup.com>
>> Co-developed-by: Max Chen <mchen@squareup.com>
>> Signed-off-by: Max Chen <mchen@squareup.com>
>> Co-developed-by: Zac Crosby <zac@squareup.com>
>> Signed-off-by: Zac Crosby <zac@squareup.com>
>> Co-developed-by: Vincent Knecht <vincent.knecht@mailoo.org>
>> Signed-off-by: Vincent Knecht <vincent.knecht@mailoo.org>
>> Co-developed-by: Stephan Gerhold <stephan@gerhold.net>
>> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> 
> 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