MIME-Version: 1.0
Date:   Thu, 16 Mar 2023 08:42:03 +0500
From:   Nikita Travkin <nikita@trvn.ru>
To:     Matthias Kaehlcke <mka@chromium.org>
Cc:     agross@kernel.org, andersson@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, konrad.dybcio@linaro.org,
        linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, ~postmarketos/upstreaming@lists.sr.ht
Subject: Re: [PATCH v3 1/4] arm64: dts: qcom: sc7180: Don't enable lpass
 clocks by default
In-Reply-To: <ZBJivckYK+JrGdG2@google.com>
References: <20230315154311.37299-1-nikita@trvn.ru>
 <20230315154311.37299-2-nikita@trvn.ru> <ZBJivckYK+JrGdG2@google.com>
Message-ID: <67077784fcf9aa928be9a6503e3d1e83@trvn.ru>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

Matthias Kaehlcke писал(а) 16.03.2023 05:28:
> On Wed, Mar 15, 2023 at 08:43:08PM +0500, Nikita Travkin wrote:
>> lpass clocks are usually blocked from HLOS by the firmware and
>> instead are managed by the ADSP. Mark them as reserved and explicitly
>> enable in the CrOS boards that have special, cooperative firmware.
>>
>> Signed-off-by: Nikita Travkin <nikita@trvn.ru>
>> ---
>>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 8 ++++++++
>>  arch/arm64/boot/dts/qcom/sc7180.dtsi         | 4 ++++
>>  2 files changed, 12 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> index 423630c4d02c..26def6e12723 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> @@ -785,6 +785,14 @@ alc5682: codec@1a {
>>  	};
>>  };
>>
>> +&lpasscc {
>> +	status = "okay";
>> +};
>> +
>> +&lpass_hm {
>> +	status = "okay";
>> +};
>> +
>>  &lpass_cpu {
>>  	status = "okay";
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> index 53f0076f20f6..f0de177981f9 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> @@ -3623,6 +3623,8 @@ lpasscc: clock-controller@62d00000 {
>>  			power-domains = <&lpass_hm LPASS_CORE_HM_GDSCR>;
>>  			#clock-cells = <1>;
>>  			#power-domain-cells = <1>;
>> +
>> +			status = "reserved"; /* Controlled by ADSP */
> 
> Why not "disabled"? I see "reserved" is used in a couple of other
> instances, but it doesn't appear in Documentation/devicetree/bindings/,
> so it doesn't seem to have a well defined meaning that distinguishes it
> from "disabled".

Hi, the standard properties like this one are defined in the
Devicetree specification, notably chapter 2.3.4 "status" says:

  "reserved" Indicates that the device is operational, but should
  not be used. Typically this is used for devices that are controlled
  by another software component, such as platform firmware.

On the contrary,

  "disabled" Indicates that the device is not presently operational (...)

Since the hardware is operational but is controlled by "foreign"
firmware, I believe "reserved" state is more correct here.

Thanks for looking at the patch!
Nikita

> 
>>  		};
>>
>>  		lpass_cpu: lpass@62d87000 {
>> @@ -3671,6 +3673,8 @@ lpass_hm: clock-controller@63000000 {
>>
>>  			#clock-cells = <1>;
>>  			#power-domain-cells = <1>;
>> +
>> +			status = "reserved"; /* Controlled by ADSP */
>>  		};
>>  	};
>>
>> --
>> 2.39.2
>>