Re: [PATCH 3/3] arm64: dts: register Hi6220's thermal zone for power allocator

From: Leo Yan
Date: Wed Feb 24 2016 - 21:42:18 EST


Hi Javi,

Thanks for review.

On Wed, Feb 24, 2016 at 06:28:57PM +0000, Javi Merino wrote:
> On Sat, Feb 20, 2016 at 11:52:09PM +0800, Leo Yan wrote:
> > With profiling Hi6220's power modeling so get dynamic coefficient and
> > sustainable power. So pass these parameters from DT.
> >
> > Now enable power allocator wit only one actor for CPU part, so directly
> > use cluster0's thermal sensor for monitoring temperature.
> >
> > Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
> > ---
> > arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 36 +++++++++++++++++++++++++++++++
> > 1 file changed, 36 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> > index 50ba1b0..3608a3e 100644
> > --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> > +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> > @@ -6,6 +6,7 @@
> >
> > #include <dt-bindings/interrupt-controller/arm-gic.h>
> > #include <dt-bindings/clock/hi6220-clock.h>
> > +#include <dt-bindings/thermal/thermal.h>
> >
> > / {
> > compatible = "hisilicon,hi6220";
> > @@ -87,6 +88,7 @@
> > cooling-max-level = <0>;
> > #cooling-cells = <2>; /* min followed by max */
> > cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
> > + dynamic-power-coefficient = <311>;
> > };
> >
> > cpu1: cpu@1 {
> > @@ -322,5 +324,39 @@
> > clock-names = "thermal_clk";
> > #thermal-sensor-cells = <1>;
> > };
> > +
> > + thermal-zones {
> > +
> > + cls0: cls0 {
> > + polling-delay = <1000>;
> > + polling-delay-passive = <100>;
> > + sustainable-power = <3326>;
> > +
> > + /* sensor ID */
> > + thermal-sensors = <&tsensor 2>;
> > +
> > + trips {
> > + threshold: trip-point@0 {
> > + temperature = <65000>;
> > + hysteresis = <1000>;
>
> As far as I know, hysteresis is ignored in the thermal subsystem right
> now, so you could remove it from both trip points.

Will remove it.

> > + type = "passive";
> > + };
> > +
> > + target: trip-point@1 {
> > + temperature = <75000>;
> > + hysteresis = <1000>;
> > + type = "passive";
> > + };
> > + };
> > +
> > + cooling-maps {
> > + map0 {
> > + trip = <&target>;
> > + contribution = <1024>;
>
> As Hikey has only one voltage domain, you only have one cpu cooling
> device. "contribution" is only useful when you have more than one
> cooling device, as it's relative to the other cooling device's
> contribution. You can remove contribution from here.
>
> Other than this minor stuff, it looks good to me. FWIW,
>
> Reviewed-by: Javi Merino <javi.merino@xxxxxxx>

Thanks for review. Will sent out new version.

Thanks,
Leo Yan

> > + cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> > + };
> > + };
> > + };
> > + };
> > };
> > };