diff mbox series

[v3,1/4] arm64: dts: qcom: sc7180: Don't enable lpass clocks by default

Message ID 20230315154311.37299-2-nikita@trvn.ru
State New
Headers
Series Add Acer Aspire 1 |

Commit Message

Nikita Travkin March 15, 2023, 3:43 p.m. UTC 
  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(+)

Comments

Konrad Dybcio March 15, 2023, 4:12 p.m. UTC | #1 
On 15.03.2023 16:43, 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>
> ---
+CC Doug

Please confirm whether this also applies to IDP (in which case
this would have been a bugfix).

Konrad
>  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 */
>  		};
>  
>  		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 */
>  		};
>  	};
>
Doug Anderson March 16, 2023, 12:06 a.m. UTC | #2 
Hi,

On Wed, Mar 15, 2023 at 9:12 AM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
>
>
> On 15.03.2023 16:43, 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>
> > ---
> +CC Doug
>
> Please confirm whether this also applies to IDP (in which case
> this would have been a bugfix).

Thanks for the CC! Actually, Judy (and maybe Matthias) might be
better. The audio stuff makes my head spin a little bit and I know
they've kept track of it much better than I have. If they don't have
time, I can dig more into it myself. I also added Srinivasa just in
case.

In general, though, I think people at Google don't interact with IDP
very much. It's a reference board from Qualcomm and not the right form
factor. It also doesn't support most of the tooling that folks at
Google on the ChromeOS team expect. I don't know if audio was ever
really supported on IDP. Probably not since there's no "sound" node in
the IDP device tree file.


> Konrad
> >  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 */
> >               };
> >
> >               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 */
> >               };
> >       };
> >
Matthias Kaehlcke March 16, 2023, 12:23 a.m. UTC | #3 
On Wed, Mar 15, 2023 at 05:06:04PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Wed, Mar 15, 2023 at 9:12 AM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >
> >
> >
> > On 15.03.2023 16:43, 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>
> > > ---
> > +CC Doug
> >
> > Please confirm whether this also applies to IDP (in which case
> > this would have been a bugfix).
> 
> Thanks for the CC! Actually, Judy (and maybe Matthias) might be
> better. The audio stuff makes my head spin a little bit and I know
> they've kept track of it much better than I have. If they don't have
> time, I can dig more into it myself. I also added Srinivasa just in
> case.
>
> In general, though, I think people at Google don't interact with IDP
> very much. It's a reference board from Qualcomm and not the right form
> factor. It also doesn't support most of the tooling that folks at
> Google on the ChromeOS team expect. I don't know if audio was ever
> really supported on IDP. Probably not since there's no "sound" node in
> the IDP device tree file.

I don't know about the history of audio on the IDP either, I share Doug's
assessment that it looks like it was never supported.

> > Konrad
> > >  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 */
> > >               };
> > >
> > >               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 */
> > >               };
> > >       };
> > >
Matthias Kaehlcke March 16, 2023, 12:28 a.m. UTC | #4 
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".

>  		};
>  
>  		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
>
Nikita Travkin March 16, 2023, 3:42 a.m. UTC | #5 
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
>>
Matthias Kaehlcke March 16, 2023, 5:28 p.m. UTC | #6 
On Thu, Mar 16, 2023 at 08:42:03AM +0500, Nikita Travkin wrote:
> 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 the clarification!
diff mbox series

Patch

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 */
 		};
 
 		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 */
 		};
 	};