Message-ID: <a041186e-fd7d-0bd2-eb67-8e6000b50c32@linaro.org>
Date:   Mon, 6 Mar 2023 15:25:50 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH v7 2/5] arm64: dts: qcom: Add msm8939 SoC
Content-Language: en-US
To:     Stephan Gerhold <stephan@gerhold.net>
Cc:     agross@kernel.org, andersson@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>
References: <20230223153655.262783-1-bryan.odonoghue@linaro.org>
 <20230223153655.262783-3-bryan.odonoghue@linaro.org>
 <ZAYA4cdlpIfjdqt2@gerhold.net>
From:   Bryan O'Donoghue <bryan.odonoghue@linaro.org>
In-Reply-To: <ZAYA4cdlpIfjdqt2@gerhold.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 06/03/2023 15:04, Stephan Gerhold wrote:
> Hi Bryan,
> 
> Thanks for making the changes. I only have some minor nitpicks now:

>> +
>> +			tsens_base1: base1@a0 {
>> +				reg = <0xa0 0x1>;
>> +				bits = <0 8>;
>> +			};
>> +
>> +			tsens_s5_p1: s5-p1@a1 {
>> +				reg = <0xa1 0x1>;
>> +				bits = <0 6>;
>> +			};
>> [...]
>> +		tsens: thermal-sensor@4a9000 {
>> +			compatible = "qcom,msm8939-tsens", "qcom,tsens-v0_1";
>> +			reg = <0x004a9000 0x1000>, /* TM */
>> +			      <0x004a8000 0x1000>; /* SROT */
>> +			nvmem-cells = <&tsens_mode>,
>> +				      <&tsens_base1>, <&tsens_base2>,
>> +				      <&tsens_s0_p1>, <&tsens_s0_p2>,
>> +				      <&tsens_s1_p1>, <&tsens_s1_p2>,
>> +				      <&tsens_s2_p1>, <&tsens_s2_p2>,
>> +				      <&tsens_s3_p1>, <&tsens_s3_p2>,
>> +				      <&tsens_s4_p1>, <&tsens_s4_p2>,
>> +				      <&tsens_s4_p1>, <&tsens_s4_p2>,
>> +				      <&tsens_s5_p1>, <&tsens_s5_p2>,
>> +				      <&tsens_s6_p1>, <&tsens_s6_p2>,
>> +				      <&tsens_s7_p1>, <&tsens_s7_p2>,
>> +				      <&tsens_s8_p1>, <&tsens_s8_p2>;
>> +			nvmem-cell-names = "mode",
>> +					   "base1", "base2",
>> +					   "s0_p1", "s0_p2",
>> +					   "s1_p1", "s1_p2",
>> +					   "s2_p1", "s2_p2",
>> +					   "s3_p1", "s3_p2",
>> +					   "s4_p1", "s4_p2",
>> +					   "s4_p1", "s4_p2",
> 
> s4_p1/p2 are specified twice, is this on purpose or accidental?

On purpose I believe.

@Dmitry ?

> 
>> +					   "s5_p1", "s5_p2",
>> +					   "s6_p1", "s6_p2",
>> +					   "s7_p1", "s7_p2",
>> +					   "s8_p1", "s8_p2";
>> +			#qcom,sensors = <9>;
>> +			interrupts = <GIC_SPI 184 IRQ_TYPE_LEVEL_HIGH>;
>> +			interrupt-names = "uplow";
>> +			#thermal-sensor-cells = <1>;
>> +		};
>> [...]
>> +		usb: usb@78d9000 {
>> +			compatible = "qcom,ci-hdrc";
>> +			reg = <0x078d9000 0x200>,
>> +			      <0x078d9200 0x200>;
>> +			interrupts = <GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>,
>> +				     <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;
>> +			clocks = <&gcc GCC_USB_HS_AHB_CLK>,
>> +				 <&gcc GCC_USB_HS_SYSTEM_CLK>;
>> +			clock-names = "iface", "core";
>> +			assigned-clocks = <&gcc GCC_USB_HS_SYSTEM_CLK>;
>> +			assigned-clock-rates = <80000000>;
>> +			resets = <&gcc GCC_USB_HS_BCR>;
>> +			reset-names = "core";
>> +			#reset-cells = <1>;
>> +			phy_type = "ulpi";
>> +			dr_mode = "otg";
> 
> Please add
> 
> 			hnp-disable;
> 			srp-disable;
> 			adp-disable;
> 
> here for consistency with msm8916.dtsi. These are needed for correct
> behavior if you enable CONFIG_USB_OTG_FSM, see commit bfd5d21abcd5c
> ("arm64: dts: qcom: msm8916: Move common USB properties to msm8916.dtsi")
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bfd5d21abcd5c7941ad79b594f5f42e27496eb28
> 

Hmm, I haven't noticed any problems with OTG - then again, I'm looking 
at my kernel config and +# CONFIG_USB_OTG_FSM is not set