Re: [PATCH v3 7/7] arm64: dts: qcom: Add mx power domain as thermal warming device.

Re: [PATCH v3 7/7] arm64: dts: qcom: Add mx power domain as thermal warming device.

[Rob Herring]

On Wed, Oct 16, 2019 at 03:37:21PM -0400, Thara Gopinath wrote:
> RPMh hosts mx power domain that can be used to warm up the SoC.
> Add sub-node to rpmhpd node for mx to be recognized
> as thermal warming device on sdm845.
> 
> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 0222f48..0671c8a 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -3788,6 +3788,11 @@
>  						opp-level = <RPMH_REGULATOR_LEVEL_TURBO_L1>;
>  					};
>  				};
> +
> +				mx_cdev: mx {
> +					#cooling-cells = <2>;
> +					.name = "mx";

Copy this from C code?

> +				};
>  			};
>  
>  			rsc_hlos: interconnect {
> -- 
> 2.1.4
> 

Re: [PATCH v3 6/7] dt-bindings: soc: qcom: Extend RPMh power controller binding to describe thermal warming device

[Rob Herring]

On Thu, Oct 17, 2019 at 12:10:15PM -0400, Thara Gopinath wrote:
> On 10/17/2019 11:43 AM, Ulf Hansson wrote:
> > On Thu, 17 Oct 2019 at 17:28, Thara Gopinath <thara.gopinath@linaro.org> wrote:
> >>
> >> Hello Ulf,
> >> Thanks for the review!
> >>
> >> On 10/17/2019 05:04 AM, Ulf Hansson wrote:
> >>> On Wed, 16 Oct 2019 at 21:37, Thara Gopinath <thara.gopinath@linaro.org> wrote:
> >>>>
> >>>> RPMh power controller hosts mx domain that can be used as thermal
> >>>> warming device. Add a sub-node to specify this.
> >>>>
> >>>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> >>>> ---
> >>>>  Documentation/devicetree/bindings/power/qcom,rpmpd.txt | 10 ++++++++++
> >>>>  1 file changed, 10 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> >>>> index eb35b22..fff695d 100644
> >>>> --- a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> >>>> +++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> >>>> @@ -18,6 +18,16 @@ Required Properties:
> >>>>  Refer to <dt-bindings/power/qcom-rpmpd.h> for the level values for
> >>>>  various OPPs for different platforms as well as Power domain indexes
> >>>>
> >>>> += SUBNODES
> >>>> +RPMh alsp hosts power domains that can behave as thermal warming device.
> >>>> +These are expressed as subnodes of the RPMh. The name of the node is used
> >>>> +to identify the power domain and must therefor be "mx".
> >>>> +
> >>>> +- #cooling-cells:
> >>>> +       Usage: optional
> >>>> +       Value type: <u32>
> >>>> +       Definition: must be 2
> >>>> +
> >>>
> >>> Just wanted to express a minor thought about this. In general we use
> >>> subnodes of PM domain providers to represent the topology of PM
> >>> domains (subdomains), this is something different, which I guess is
> >>> fine.
> >>>
> >>> I assume the #cooling-cells is here tells us this is not a PM domain
> >>> provider, but a "cooling device provider"?
> >> Yep.
> >>>
> >>> Also, I wonder if it would be fine to specify "power-domains" here,
> >>> rather than using "name" as I think that is kind of awkward!?
> >> Do you mean "power-domain-names" ? I am using this to match against the
> >> genpd names defined in the provider driver.
> > 
> > No. If you are using "power-domains" it means that you allow to
> > describe the specifier for the provider.
> Yep. But won't this look funny in DT ? The provider node will have a sub
> node with a power domain referencing to itself Like below: Is this ok ?
> 
> rpmhpd: power-controller {
>                                 compatible = "qcom,sdm845-rpmhpd";
>                                 #power-domain-cells = <1>;
> 
> 			...
> 			...
> 				mx_cdev: mx {
>                                         #cooling-cells = <2>;
>                                         power-domains = <&rpmhpd	SDM845_MX>;
>                                 };
> 				

The whole concept here seems all wrong to me. Isn't it what's in the 
power domain that's the cooling device. A CPU power domain is not a 
cooling device, the CPU is. Or we wouldn't make a clock a cooling 
device, but what the clock drives.

Rob

Re: [PATCH v3 6/7] dt-bindings: soc: qcom: Extend RPMh power controller binding to describe thermal warming device

[Ulf Hansson]

On Tue, 29 Oct 2019 at 02:36, Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Oct 17, 2019 at 12:10:15PM -0400, Thara Gopinath wrote:
> > On 10/17/2019 11:43 AM, Ulf Hansson wrote:
> > > On Thu, 17 Oct 2019 at 17:28, Thara Gopinath <thara.gopinath@linaro.org> wrote:
> > >>
> > >> Hello Ulf,
> > >> Thanks for the review!
> > >>
> > >> On 10/17/2019 05:04 AM, Ulf Hansson wrote:
> > >>> On Wed, 16 Oct 2019 at 21:37, Thara Gopinath <thara.gopinath@linaro.org> wrote:
> > >>>>
> > >>>> RPMh power controller hosts mx domain that can be used as thermal
> > >>>> warming device. Add a sub-node to specify this.
> > >>>>
> > >>>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> > >>>> ---
> > >>>>  Documentation/devicetree/bindings/power/qcom,rpmpd.txt | 10 ++++++++++
> > >>>>  1 file changed, 10 insertions(+)
> > >>>>
> > >>>> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> > >>>> index eb35b22..fff695d 100644
> > >>>> --- a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> > >>>> +++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> > >>>> @@ -18,6 +18,16 @@ Required Properties:
> > >>>>  Refer to <dt-bindings/power/qcom-rpmpd.h> for the level values for
> > >>>>  various OPPs for different platforms as well as Power domain indexes
> > >>>>
> > >>>> += SUBNODES
> > >>>> +RPMh alsp hosts power domains that can behave as thermal warming device.
> > >>>> +These are expressed as subnodes of the RPMh. The name of the node is used
> > >>>> +to identify the power domain and must therefor be "mx".
> > >>>> +
> > >>>> +- #cooling-cells:
> > >>>> +       Usage: optional
> > >>>> +       Value type: <u32>
> > >>>> +       Definition: must be 2
> > >>>> +
> > >>>
> > >>> Just wanted to express a minor thought about this. In general we use
> > >>> subnodes of PM domain providers to represent the topology of PM
> > >>> domains (subdomains), this is something different, which I guess is
> > >>> fine.
> > >>>
> > >>> I assume the #cooling-cells is here tells us this is not a PM domain
> > >>> provider, but a "cooling device provider"?
> > >> Yep.
> > >>>
> > >>> Also, I wonder if it would be fine to specify "power-domains" here,
> > >>> rather than using "name" as I think that is kind of awkward!?
> > >> Do you mean "power-domain-names" ? I am using this to match against the
> > >> genpd names defined in the provider driver.
> > >
> > > No. If you are using "power-domains" it means that you allow to
> > > describe the specifier for the provider.
> > Yep. But won't this look funny in DT ? The provider node will have a sub
> > node with a power domain referencing to itself Like below: Is this ok ?
> >
> > rpmhpd: power-controller {
> >                                 compatible = "qcom,sdm845-rpmhpd";
> >                                 #power-domain-cells = <1>;
> >
> >                       ...
> >                       ...
> >                               mx_cdev: mx {
> >                                         #cooling-cells = <2>;
> >                                         power-domains = <&rpmhpd      SDM845_MX>;
> >                                 };
> >
>
> The whole concept here seems all wrong to me. Isn't it what's in the
> power domain that's the cooling device. A CPU power domain is not a
> cooling device, the CPU is. Or we wouldn't make a clock a cooling
> device, but what the clock drives.

Well, I don't think that's entirely correct description either.

As I see it, it's really the actual PM domain (that manages voltages
for a power island), that needs to stay in full power state and
increase its voltage level, as to warm up some of the silicon. It's
not a regular device, but more a characteristics of how the PM domain
can be used.

Kind regards
Uffe

Re: [PATCH v3 6/7] dt-bindings: soc: qcom: Extend RPMh power controller binding to describe thermal warming device

[Rob Herring]

On Tue, Oct 29, 2019 at 5:07 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 29 Oct 2019 at 02:36, Rob Herring <robh@kernel.org> wrote:
> >
> > On Thu, Oct 17, 2019 at 12:10:15PM -0400, Thara Gopinath wrote:
> > > On 10/17/2019 11:43 AM, Ulf Hansson wrote:
> > > > On Thu, 17 Oct 2019 at 17:28, Thara Gopinath <thara.gopinath@linaro.org> wrote:
> > > >>
> > > >> Hello Ulf,
> > > >> Thanks for the review!
> > > >>
> > > >> On 10/17/2019 05:04 AM, Ulf Hansson wrote:
> > > >>> On Wed, 16 Oct 2019 at 21:37, Thara Gopinath <thara.gopinath@linaro.org> wrote:
> > > >>>>
> > > >>>> RPMh power controller hosts mx domain that can be used as thermal
> > > >>>> warming device. Add a sub-node to specify this.
> > > >>>>
> > > >>>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> > > >>>> ---
> > > >>>>  Documentation/devicetree/bindings/power/qcom,rpmpd.txt | 10 ++++++++++
> > > >>>>  1 file changed, 10 insertions(+)
> > > >>>>
> > > >>>> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> > > >>>> index eb35b22..fff695d 100644
> > > >>>> --- a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> > > >>>> +++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> > > >>>> @@ -18,6 +18,16 @@ Required Properties:
> > > >>>>  Refer to <dt-bindings/power/qcom-rpmpd.h> for the level values for
> > > >>>>  various OPPs for different platforms as well as Power domain indexes
> > > >>>>
> > > >>>> += SUBNODES
> > > >>>> +RPMh alsp hosts power domains that can behave as thermal warming device.
> > > >>>> +These are expressed as subnodes of the RPMh. The name of the node is used
> > > >>>> +to identify the power domain and must therefor be "mx".
> > > >>>> +
> > > >>>> +- #cooling-cells:
> > > >>>> +       Usage: optional
> > > >>>> +       Value type: <u32>
> > > >>>> +       Definition: must be 2
> > > >>>> +
> > > >>>
> > > >>> Just wanted to express a minor thought about this. In general we use
> > > >>> subnodes of PM domain providers to represent the topology of PM
> > > >>> domains (subdomains), this is something different, which I guess is
> > > >>> fine.
> > > >>>
> > > >>> I assume the #cooling-cells is here tells us this is not a PM domain
> > > >>> provider, but a "cooling device provider"?
> > > >> Yep.
> > > >>>
> > > >>> Also, I wonder if it would be fine to specify "power-domains" here,
> > > >>> rather than using "name" as I think that is kind of awkward!?
> > > >> Do you mean "power-domain-names" ? I am using this to match against the
> > > >> genpd names defined in the provider driver.
> > > >
> > > > No. If you are using "power-domains" it means that you allow to
> > > > describe the specifier for the provider.
> > > Yep. But won't this look funny in DT ? The provider node will have a sub
> > > node with a power domain referencing to itself Like below: Is this ok ?
> > >
> > > rpmhpd: power-controller {
> > >                                 compatible = "qcom,sdm845-rpmhpd";
> > >                                 #power-domain-cells = <1>;
> > >
> > >                       ...
> > >                       ...
> > >                               mx_cdev: mx {
> > >                                         #cooling-cells = <2>;
> > >                                         power-domains = <&rpmhpd      SDM845_MX>;
> > >                                 };
> > >
> >
> > The whole concept here seems all wrong to me. Isn't it what's in the
> > power domain that's the cooling device. A CPU power domain is not a
> > cooling device, the CPU is. Or we wouldn't make a clock a cooling
> > device, but what the clock drives.
>
> Well, I don't think that's entirely correct description either.
>
> As I see it, it's really the actual PM domain (that manages voltages
> for a power island), that needs to stay in full power state and
> increase its voltage level, as to warm up some of the silicon. It's
> not a regular device, but more a characteristics of how the PM domain
> can be used.

First I've heard of Si needing warming...

I think I'd just expect the power domain provider to know which
domains to power on then.

Rob

Re: [PATCH v3 6/7] dt-bindings: soc: qcom: Extend RPMh power controller binding to describe thermal warming device

[Ulf Hansson]

On Tue, 29 Oct 2019 at 21:16, Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Oct 29, 2019 at 5:07 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Tue, 29 Oct 2019 at 02:36, Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Thu, Oct 17, 2019 at 12:10:15PM -0400, Thara Gopinath wrote:
> > > > On 10/17/2019 11:43 AM, Ulf Hansson wrote:
> > > > > On Thu, 17 Oct 2019 at 17:28, Thara Gopinath <thara.gopinath@linaro.org> wrote:
> > > > >>
> > > > >> Hello Ulf,
> > > > >> Thanks for the review!
> > > > >>
> > > > >> On 10/17/2019 05:04 AM, Ulf Hansson wrote:
> > > > >>> On Wed, 16 Oct 2019 at 21:37, Thara Gopinath <thara.gopinath@linaro.org> wrote:
> > > > >>>>
> > > > >>>> RPMh power controller hosts mx domain that can be used as thermal
> > > > >>>> warming device. Add a sub-node to specify this.
> > > > >>>>
> > > > >>>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> > > > >>>> ---
> > > > >>>>  Documentation/devicetree/bindings/power/qcom,rpmpd.txt | 10 ++++++++++
> > > > >>>>  1 file changed, 10 insertions(+)
> > > > >>>>
> > > > >>>> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> > > > >>>> index eb35b22..fff695d 100644
> > > > >>>> --- a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> > > > >>>> +++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> > > > >>>> @@ -18,6 +18,16 @@ Required Properties:
> > > > >>>>  Refer to <dt-bindings/power/qcom-rpmpd.h> for the level values for
> > > > >>>>  various OPPs for different platforms as well as Power domain indexes
> > > > >>>>
> > > > >>>> += SUBNODES
> > > > >>>> +RPMh alsp hosts power domains that can behave as thermal warming device.
> > > > >>>> +These are expressed as subnodes of the RPMh. The name of the node is used
> > > > >>>> +to identify the power domain and must therefor be "mx".
> > > > >>>> +
> > > > >>>> +- #cooling-cells:
> > > > >>>> +       Usage: optional
> > > > >>>> +       Value type: <u32>
> > > > >>>> +       Definition: must be 2
> > > > >>>> +
> > > > >>>
> > > > >>> Just wanted to express a minor thought about this. In general we use
> > > > >>> subnodes of PM domain providers to represent the topology of PM
> > > > >>> domains (subdomains), this is something different, which I guess is
> > > > >>> fine.
> > > > >>>
> > > > >>> I assume the #cooling-cells is here tells us this is not a PM domain
> > > > >>> provider, but a "cooling device provider"?
> > > > >> Yep.
> > > > >>>
> > > > >>> Also, I wonder if it would be fine to specify "power-domains" here,
> > > > >>> rather than using "name" as I think that is kind of awkward!?
> > > > >> Do you mean "power-domain-names" ? I am using this to match against the
> > > > >> genpd names defined in the provider driver.
> > > > >
> > > > > No. If you are using "power-domains" it means that you allow to
> > > > > describe the specifier for the provider.
> > > > Yep. But won't this look funny in DT ? The provider node will have a sub
> > > > node with a power domain referencing to itself Like below: Is this ok ?
> > > >
> > > > rpmhpd: power-controller {
> > > >                                 compatible = "qcom,sdm845-rpmhpd";
> > > >                                 #power-domain-cells = <1>;
> > > >
> > > >                       ...
> > > >                       ...
> > > >                               mx_cdev: mx {
> > > >                                         #cooling-cells = <2>;
> > > >                                         power-domains = <&rpmhpd      SDM845_MX>;
> > > >                                 };
> > > >
> > >
> > > The whole concept here seems all wrong to me. Isn't it what's in the
> > > power domain that's the cooling device. A CPU power domain is not a
> > > cooling device, the CPU is. Or we wouldn't make a clock a cooling
> > > device, but what the clock drives.
> >
> > Well, I don't think that's entirely correct description either.
> >
> > As I see it, it's really the actual PM domain (that manages voltages
> > for a power island), that needs to stay in full power state and
> > increase its voltage level, as to warm up some of the silicon. It's
> > not a regular device, but more a characteristics of how the PM domain
> > can be used.
>
> First I've heard of Si needing warming...

I guess people go to cooler places with their devices. :-)

>
> I think I'd just expect the power domain provider to know which
> domains to power on then.

Yeah, I agree. This seems reasonable.

Thanks!

Kind regards
Uffe

Re: [PATCH v3 7/7] arm64: dts: qcom: Add mx power domain as thermal warming device.

[Thara Gopinath]

On 10/28/2019 09:31 PM, Rob Herring wrote:
> On Wed, Oct 16, 2019 at 03:37:21PM -0400, Thara Gopinath wrote:
>> RPMh hosts mx power domain that can be used to warm up the SoC.
>> Add sub-node to rpmhpd node for mx to be recognized
>> as thermal warming device on sdm845.
>>
>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
>> ---
>>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> index 0222f48..0671c8a 100644
>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> @@ -3788,6 +3788,11 @@
>>  						opp-level = <RPMH_REGULATOR_LEVEL_TURBO_L1>;
>>  					};
>>  				};
>> +
>> +				mx_cdev: mx {
>> +					#cooling-cells = <2>;
>> +					.name = "mx";
> 
> Copy this from C code?
Hi Rob,

What do you mean ?

> 
>> +				};
>>  			};
>>  
>>  			rsc_hlos: interconnect {
>> -- 
>> 2.1.4
>>


-- 
Warm Regards
Thara

Re: [PATCH v3 6/7] dt-bindings: soc: qcom: Extend RPMh power controller binding to describe thermal warming device

[Thara Gopinath]

Hi Rob,

Thanks for the review.

On 10/29/2019 04:16 PM, Rob Herring wrote:
> On Tue, Oct 29, 2019 at 5:07 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>
>> On Tue, 29 Oct 2019 at 02:36, Rob Herring <robh@kernel.org> wrote:
>>>
>>> On Thu, Oct 17, 2019 at 12:10:15PM -0400, Thara Gopinath wrote:
>>>> On 10/17/2019 11:43 AM, Ulf Hansson wrote:
>>>>> On Thu, 17 Oct 2019 at 17:28, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>>>>>>
>>>>>> Hello Ulf,
>>>>>> Thanks for the review!
>>>>>>
>>>>>> On 10/17/2019 05:04 AM, Ulf Hansson wrote:
>>>>>>> On Wed, 16 Oct 2019 at 21:37, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>>>>>>>>
>>>>>>>> RPMh power controller hosts mx domain that can be used as thermal
>>>>>>>> warming device. Add a sub-node to specify this.
>>>>>>>>
>>>>>>>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
>>>>>>>> ---
>>>>>>>>  Documentation/devicetree/bindings/power/qcom,rpmpd.txt | 10 ++++++++++
>>>>>>>>  1 file changed, 10 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
>>>>>>>> index eb35b22..fff695d 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
>>>>>>>> +++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
>>>>>>>> @@ -18,6 +18,16 @@ Required Properties:
>>>>>>>>  Refer to <dt-bindings/power/qcom-rpmpd.h> for the level values for
>>>>>>>>  various OPPs for different platforms as well as Power domain indexes
>>>>>>>>
>>>>>>>> += SUBNODES
>>>>>>>> +RPMh alsp hosts power domains that can behave as thermal warming device.
>>>>>>>> +These are expressed as subnodes of the RPMh. The name of the node is used
>>>>>>>> +to identify the power domain and must therefor be "mx".
>>>>>>>> +
>>>>>>>> +- #cooling-cells:
>>>>>>>> +       Usage: optional
>>>>>>>> +       Value type: <u32>
>>>>>>>> +       Definition: must be 2
>>>>>>>> +
>>>>>>>
>>>>>>> Just wanted to express a minor thought about this. In general we use
>>>>>>> subnodes of PM domain providers to represent the topology of PM
>>>>>>> domains (subdomains), this is something different, which I guess is
>>>>>>> fine.
>>>>>>>
>>>>>>> I assume the #cooling-cells is here tells us this is not a PM domain
>>>>>>> provider, but a "cooling device provider"?
>>>>>> Yep.
>>>>>>>
>>>>>>> Also, I wonder if it would be fine to specify "power-domains" here,
>>>>>>> rather than using "name" as I think that is kind of awkward!?
>>>>>> Do you mean "power-domain-names" ? I am using this to match against the
>>>>>> genpd names defined in the provider driver.
>>>>>
>>>>> No. If you are using "power-domains" it means that you allow to
>>>>> describe the specifier for the provider.
>>>> Yep. But won't this look funny in DT ? The provider node will have a sub
>>>> node with a power domain referencing to itself Like below: Is this ok ?
>>>>
>>>> rpmhpd: power-controller {
>>>>                                 compatible = "qcom,sdm845-rpmhpd";
>>>>                                 #power-domain-cells = <1>;
>>>>
>>>>                       ...
>>>>                       ...
>>>>                               mx_cdev: mx {
>>>>                                         #cooling-cells = <2>;
>>>>                                         power-domains = <&rpmhpd      SDM845_MX>;
>>>>                                 };
>>>>
>>>
>>> The whole concept here seems all wrong to me. Isn't it what's in the
>>> power domain that's the cooling device. A CPU power domain is not a
>>> cooling device, the CPU is. Or we wouldn't make a clock a cooling
>>> device, but what the clock drives.
>>
>> Well, I don't think that's entirely correct description either.
>>
>> As I see it, it's really the actual PM domain (that manages voltages
>> for a power island), that needs to stay in full power state and
>> increase its voltage level, as to warm up some of the silicon. It's
>> not a regular device, but more a characteristics of how the PM domain
>> can be used.
> 
> First I've heard of Si needing warming...
Cold regions and non-closing of circuits is what I am told.
> 
> I think I'd just expect the power domain provider to know which
> domains to power on then.
I will just retain #cooling-cells in the power domain provider and let
the driver identify the actual power domains.

> 
> Rob
> 


-- 
Warm Regards
Thara

Re: [PATCH v3 3/7] thermal: core: Add late init hook to cooling device ops

[Amit Kucheria]

On Thu, Oct 17, 2019 at 1:07 AM Thara Gopinath
<thara.gopinath@linaro.org> wrote:
>
> Add a hook in thermal_cooling_device_ops to be called after
> the cooling device has been initialized and registered
> but before binding it to a thermal zone.
>
> In this patch series it is used to hook up a power domain
> to the device pointer of cooling device.
>
> It can be used for any other relevant late initializations
> of a cooling device as well.

Please describe WHY this hook is needed.

> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> ---
>  drivers/thermal/thermal_core.c | 13 +++++++++++++
>  include/linux/thermal.h        |  1 +
>  2 files changed, 14 insertions(+)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 886e8fa..c2ecb73 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -994,6 +994,19 @@ __thermal_cooling_device_register(struct device_node *np,
>         list_add(&cdev->node, &thermal_cdev_list);
>         mutex_unlock(&thermal_list_lock);
>
> +       /* Call into cdev late initialization if defined */
> +       if (cdev->ops->late_init) {
> +               result = cdev->ops->late_init(cdev);
> +               if (result) {
> +                       ida_simple_remove(&thermal_cdev_ida, cdev->id);
> +                       put_device(&cdev->device);
> +                       mutex_lock(&thermal_list_lock);
> +                       list_del(&cdev->node);
> +                       mutex_unlock(&thermal_list_lock);
> +                       return ERR_PTR(result);
> +               }
> +       }
> +
>         /* Update binding information for 'this' new cdev */
>         bind_cdev(cdev);
>
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index e45659c..e94b3de 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -125,6 +125,7 @@ struct thermal_cooling_device_ops {
>                            struct thermal_zone_device *, unsigned long, u32 *);
>         int (*power2state)(struct thermal_cooling_device *,
>                            struct thermal_zone_device *, u32, unsigned long *);
> +       int (*late_init)(struct thermal_cooling_device *);
>  };
>
>  struct thermal_cooling_device {
> --
> 2.1.4
>

Re: [PATCH v3 3/7] thermal: core: Add late init hook to cooling device ops

[Thara Gopinath]

On 12/03/2019 12:04 PM, Amit Kucheria wrote:
> On Thu, Oct 17, 2019 at 1:07 AM Thara Gopinath
> <thara.gopinath@linaro.org> wrote:
>>
>> Add a hook in thermal_cooling_device_ops to be called after
>> the cooling device has been initialized and registered
>> but before binding it to a thermal zone.
>>
>> In this patch series it is used to hook up a power domain
>> to the device pointer of cooling device.
>>
>> It can be used for any other relevant late initializations
>> of a cooling device as well.
> 
> Please describe WHY this hook is needed.
Hi Amit,

Thanks for the review. This patch is no longer relevant.
I had posted a v4 of this series based on other review comments[1]

1. https://lkml.org/lkml/2019/11/20/355

-- 
Warm Regards
Thara

Re: [PATCH v3 3/7] thermal: core: Add late init hook to cooling device ops

[Amit Kucheria]

On Tue, Dec 3, 2019 at 10:34 PM Amit Kucheria
<amit.kucheria@verdurent.com> wrote:
>
> On Thu, Oct 17, 2019 at 1:07 AM Thara Gopinath
> <thara.gopinath@linaro.org> wrote:
> >
> > Add a hook in thermal_cooling_device_ops to be called after
> > the cooling device has been initialized and registered
> > but before binding it to a thermal zone.
> >
> > In this patch series it is used to hook up a power domain
> > to the device pointer of cooling device.
> >
> > It can be used for any other relevant late initializations
> > of a cooling device as well.
>
> Please describe WHY this hook is needed.

Just noticed you dropped this for v4. Nevermind.

> > Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> > ---
> >  drivers/thermal/thermal_core.c | 13 +++++++++++++
> >  include/linux/thermal.h        |  1 +
> >  2 files changed, 14 insertions(+)
> >
> > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> > index 886e8fa..c2ecb73 100644
> > --- a/drivers/thermal/thermal_core.c
> > +++ b/drivers/thermal/thermal_core.c
> > @@ -994,6 +994,19 @@ __thermal_cooling_device_register(struct device_node *np,
> >         list_add(&cdev->node, &thermal_cdev_list);
> >         mutex_unlock(&thermal_list_lock);
> >
> > +       /* Call into cdev late initialization if defined */
> > +       if (cdev->ops->late_init) {
> > +               result = cdev->ops->late_init(cdev);
> > +               if (result) {
> > +                       ida_simple_remove(&thermal_cdev_ida, cdev->id);
> > +                       put_device(&cdev->device);
> > +                       mutex_lock(&thermal_list_lock);
> > +                       list_del(&cdev->node);
> > +                       mutex_unlock(&thermal_list_lock);
> > +                       return ERR_PTR(result);
> > +               }
> > +       }
> > +
> >         /* Update binding information for 'this' new cdev */
> >         bind_cdev(cdev);
> >
> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > index e45659c..e94b3de 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -125,6 +125,7 @@ struct thermal_cooling_device_ops {
> >                            struct thermal_zone_device *, unsigned long, u32 *);
> >         int (*power2state)(struct thermal_cooling_device *,
> >                            struct thermal_zone_device *, u32, unsigned long *);
> > +       int (*late_init)(struct thermal_cooling_device *);
> >  };
> >
> >  struct thermal_cooling_device {
> > --
> > 2.1.4
> >