arm64: dts: renesas: r8a77970: add thermal support
diff mbox series

Message ID 51491585-16fb-93b1-bb1a-09e89683f2f0@cogentembedded.com
State Superseded
Delegated to: Simon Horman
Headers show
Series
  • arm64: dts: renesas: r8a77970: add thermal support
Related show

Commit Message

Sergei Shtylyov Oct. 5, 2018, 7:25 p.m. UTC
Describe THS/CIVM in the R8A77970 device trees.

Based on the original (and large) patches by Vladimir Barinov.

Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
This patch is against the 'renesas-devel-20181004-v4.19-rc6' tag of Simon
Horman's 'renesas.git' repo.

The thermal driver/bindings patches have been posted yesterday...

 arch/arm64/boot/dts/renesas/r8a77970.dtsi |   32 ++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Comments

Sergei Shtylyov Oct. 5, 2018, 7:33 p.m. UTC | #1
On 10/05/2018 10:25 PM, Sergei Shtylyov wrote:

> Describe THS/CIVM in the R8A77970 device trees.

   Damn, should be singular "tree"! :-/

> Based on the original (and large) patches by Vladimir Barinov.
> 
> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

[...]

MBR, Sergei
Simon Horman Oct. 8, 2018, 7:55 a.m. UTC | #2
On Fri, Oct 05, 2018 at 10:25:47PM +0300, Sergei Shtylyov wrote:
> Describe THS/CIVM in the R8A77970 device trees.
> 
> Based on the original (and large) patches by Vladimir Barinov.
> 
> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
> This patch is against the 'renesas-devel-20181004-v4.19-rc6' tag of Simon
> Horman's 'renesas.git' repo.
> 
> The thermal driver/bindings patches have been posted yesterday...
> 
>  arch/arm64/boot/dts/renesas/r8a77970.dtsi |   32 ++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> Index: renesas/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> ===================================================================
> --- renesas.orig/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> +++ renesas/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> @@ -300,6 +300,19 @@
>  			#power-domain-cells = <1>;
>  		};
>  
> +		thermal: thermal@e6190000 {
> +			compatible = "renesas,thermal-r8a77970";
> +			reg =  <0 0xe6190000 0 0x14

What is the motivation for 0x14, to me 0x10 seems like a more natural size
for the register window.

Otherwise the patch looks good to me.

> +				0 0xe6190100 0 0x38>;
> +			interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&cpg CPG_MOD 522>;
> +			power-domains = <&sysc R8A77970_PD_ALWAYS_ON>;
> +			resets = <&cpg 522>;
> +			#thermal-sensor-cells = <0>;
> +		};
> +
>  		intc_ex: interrupt-controller@e61c0000 {
>  			compatible = "renesas,intc-ex-r8a77970", "renesas,irqc";
>  			#interrupt-cells = <2>;
> @@ -1033,6 +1046,25 @@
>  		};
>  	};
>  
> +	thermal-zones {
> +		cpu-thermal {
> +			polling-delay-passive = <250>;
> +			polling-delay = <1000>;
> +			thermal-sensors = <&thermal>;
> +
> +			trips {
> +				cpu-crit {
> +					temperature = <120000>;
> +					hysteresis = <2000>;
> +					type = "critical";
> +				};
> +			};
> +
> +			cooling-maps {
> +			};
> +		};
> +	};
> +
>  	timer {
>  		compatible = "arm,armv8-timer";
>  		interrupts-extended = <&gic GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>,
>
Geert Uytterhoeven Oct. 8, 2018, 8:12 a.m. UTC | #3
Hi Sergei,

On Fri, Oct 5, 2018 at 9:26 PM Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Describe THS/CIVM in the R8A77970 device trees.
>
> Based on the original (and large) patches by Vladimir Barinov.
>
> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>
> ---
> This patch is against the 'renesas-devel-20181004-v4.19-rc6' tag of Simon
> Horman's 'renesas.git' repo.
>
> The thermal driver/bindings patches have been posted yesterday...
>
>  arch/arm64/boot/dts/renesas/r8a77970.dtsi |   32 ++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> Index: renesas/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> ===================================================================
> --- renesas.orig/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> +++ renesas/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> @@ -300,6 +300,19 @@
>                         #power-domain-cells = <1>;
>                 };
>
> +               thermal: thermal@e6190000 {
> +                       compatible = "renesas,thermal-r8a77970";
> +                       reg =  <0 0xe6190000 0 0x14

0x14 was appropriate for R-Mobile APE6...


> +                               0 0xe6190100 0 0x38>;

What about the CIVM status register? DT describes hardware, not driver
limitations.

> +                       interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>,
> +                                    <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>,
> +                                    <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
> +                       clocks = <&cpg CPG_MOD 522>;
> +                       power-domains = <&sysc R8A77970_PD_ALWAYS_ON>;
> +                       resets = <&cpg 522>;
> +                       #thermal-sensor-cells = <0>;
> +               };
> +
>                 intc_ex: interrupt-controller@e61c0000 {
>                         compatible = "renesas,intc-ex-r8a77970", "renesas,irqc";
>                         #interrupt-cells = <2>;

The rest looks good to me, so with the above fixed:

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert
Sergei Shtylyov Oct. 8, 2018, 4:35 p.m. UTC | #4
On 10/08/2018 11:12 AM, Geert Uytterhoeven wrote:

>> Describe THS/CIVM in the R8A77970 device trees.
>>
>> Based on the original (and large) patches by Vladimir Barinov.
>>
>> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> ---
>> This patch is against the 'renesas-devel-20181004-v4.19-rc6' tag of Simon
>> Horman's 'renesas.git' repo.
>>
>> The thermal driver/bindings patches have been posted yesterday...
>>
>>  arch/arm64/boot/dts/renesas/r8a77970.dtsi |   32 ++++++++++++++++++++++++++++++
>>  1 file changed, 32 insertions(+)
>>
>> Index: renesas/arch/arm64/boot/dts/renesas/r8a77970.dtsi
>> ===================================================================
>> --- renesas.orig/arch/arm64/boot/dts/renesas/r8a77970.dtsi
>> +++ renesas/arch/arm64/boot/dts/renesas/r8a77970.dtsi
>> @@ -300,6 +300,19 @@
>>                         #power-domain-cells = <1>;
>>                 };
>>
>> +               thermal: thermal@e6190000 {
>> +                       compatible = "renesas,thermal-r8a77970";
>> +                       reg =  <0 0xe6190000 0 0x14
> 
> 0x14 was appropriate for R-Mobile APE6...

   Copy&paste is to blame here, I guess... I'll fix to 0x10.

> 
>> +                               0 0xe6190100 0 0x38>;
> 
> What about the CIVM status register? DT describes hardware, not driver
> limitations.

   I wasn't sure whether to put it into a separate "reg" tuple (which would confuse
the driver) or not. After looking into the manual again, I'm going to extend the
2nd "reg" tuple...

>> +                       interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>,
>> +                                    <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>,
>> +                                    <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
>> +                       clocks = <&cpg CPG_MOD 522>;
>> +                       power-domains = <&sysc R8A77970_PD_ALWAYS_ON>;
>> +                       resets = <&cpg 522>;
>> +                       #thermal-sensor-cells = <0>;
>> +               };
>> +
>>                 intc_ex: interrupt-controller@e61c0000 {
>>                         compatible = "renesas,intc-ex-r8a77970", "renesas,irqc";
>>                         #interrupt-cells = <2>;
> 
> The rest looks good to me, so with the above fixed:
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

   Thanks :-)

> Gr{oetje,eeting}s,
> 
>                         Geert

MBR, Sergei
Geert Uytterhoeven Oct. 8, 2018, 4:40 p.m. UTC | #5
Hi Sergei,

On Mon, Oct 8, 2018 at 6:35 PM Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> On 10/08/2018 11:12 AM, Geert Uytterhoeven wrote:
> >> Describe THS/CIVM in the R8A77970 device trees.
> >> Based on the original (and large) patches by Vladimir Barinov.
> >>
> >> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >>
> >> ---
> >> This patch is against the 'renesas-devel-20181004-v4.19-rc6' tag of Simon
> >> Horman's 'renesas.git' repo.
> >>
> >> The thermal driver/bindings patches have been posted yesterday...
> >>
> >>  arch/arm64/boot/dts/renesas/r8a77970.dtsi |   32 ++++++++++++++++++++++++++++++
> >>  1 file changed, 32 insertions(+)
> >>
> >> Index: renesas/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> >> ===================================================================
> >> --- renesas.orig/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> >> +++ renesas/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> >> @@ -300,6 +300,19 @@
> >>                         #power-domain-cells = <1>;
> >>                 };
> >>
> >> +               thermal: thermal@e6190000 {
> >> +                       compatible = "renesas,thermal-r8a77970";
> >> +                       reg =  <0 0xe6190000 0 0x14
> >
> > 0x14 was appropriate for R-Mobile APE6...
>
>    Copy&paste is to blame here, I guess... I'll fix to 0x10.

OK.

> >> +                               0 0xe6190100 0 0x38>;
> >
> > What about the CIVM status register? DT describes hardware, not driver
> > limitations.
>
>    I wasn't sure whether to put it into a separate "reg" tuple (which would confuse
> the driver) or not. After looking into the manual again, I'm going to extend the
> 2nd "reg" tuple...

The CIVM Status Register indicates the chip internal voltage.
As such it's not a per-channel property, and IMHO doesn't belong in the second
tuple (e.g. R-Mobile APE6 has 3 channels).

Perhaps extending the bindings to handle more reg tuples, possibly using
reg-names?

Gr{oetje,eeting}s,

                        Geert
Sergei Shtylyov Oct. 8, 2018, 6:04 p.m. UTC | #6
Hello!

On 10/08/2018 07:40 PM, Geert Uytterhoeven wrote:

>>>> Describe THS/CIVM in the R8A77970 device trees.
>>>> Based on the original (and large) patches by Vladimir Barinov.
>>>>
>>>> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>>>
>>>> ---
>>>> This patch is against the 'renesas-devel-20181004-v4.19-rc6' tag of Simon
>>>> Horman's 'renesas.git' repo.
>>>>
>>>> The thermal driver/bindings patches have been posted yesterday...
>>>>
>>>>  arch/arm64/boot/dts/renesas/r8a77970.dtsi |   32 ++++++++++++++++++++++++++++++
>>>>  1 file changed, 32 insertions(+)
>>>>
>>>> Index: renesas/arch/arm64/boot/dts/renesas/r8a77970.dtsi
>>>> ===================================================================
>>>> --- renesas.orig/arch/arm64/boot/dts/renesas/r8a77970.dtsi
>>>> +++ renesas/arch/arm64/boot/dts/renesas/r8a77970.dtsi
>>>> @@ -300,6 +300,19 @@
>>>>                         #power-domain-cells = <1>;
>>>>                 };
>>>>
>>>> +               thermal: thermal@e6190000 {
>>>> +                       compatible = "renesas,thermal-r8a77970";
>>>> +                       reg =  <0 0xe6190000 0 0x14
>>>
>>> 0x14 was appropriate for R-Mobile APE6...
>>
>>    Copy&paste is to blame here, I guess... I'll fix to 0x10.
> 
> OK.
> 
>>>> +                               0 0xe6190100 0 0x38>;
>>>
>>> What about the CIVM status register? DT describes hardware, not driver
>>> limitations.
>>
>>    I wasn't sure whether to put it into a separate "reg" tuple (which would confuse
>> the driver) or not. After looking into the manual again, I'm going to extend the
>> 2nd "reg" tuple...
> 
> The CIVM Status Register indicates the chip internal voltage.
> As such it's not a per-channel property, and IMHO doesn't belong in the second
> tuple

   I was looking a the block diagrams (both in the chapters 10A and 10B) and I got a totally different impression...

> (e.g. R-Mobile APE6 has 3 channels).

   The driver handles that alright. It's the adding the CIVM as a separate tuple
that would break it.

> Perhaps extending the bindings to handle more reg tuples, possibly using
> reg-names?

   You mean teaching the driver about one more kind of "reg" tuples? I would like
to avoid that of course -- due to the need to still handle the old DTs as well...

> Gr{oetje,eeting}s,
> 
>                         Geert
> 

MBR, Sergei
Geert Uytterhoeven Oct. 10, 2018, 7:10 a.m. UTC | #7
Hi Sergei,

On Mon, Oct 8, 2018 at 8:04 PM Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> On 10/08/2018 07:40 PM, Geert Uytterhoeven wrote:
> >>>> Describe THS/CIVM in the R8A77970 device trees.
> >>>> Based on the original (and large) patches by Vladimir Barinov.

> >>>> +                               0 0xe6190100 0 0x38>;
> >>>
> >>> What about the CIVM status register? DT describes hardware, not driver
> >>> limitations.
> >>
> >>    I wasn't sure whether to put it into a separate "reg" tuple (which would confuse
> >> the driver) or not. After looking into the manual again, I'm going to extend the
> >> 2nd "reg" tuple...
> >
> > The CIVM Status Register indicates the chip internal voltage.
> > As such it's not a per-channel property, and IMHO doesn't belong in the second
> > tuple
>
>    I was looking a the block diagrams (both in the chapters 10A and 10B) and I got a totally different impression...

I stand corrected. So extending the second reg block is fine.

Gr{oetje,eeting}s,

                        Geert

Patch
diff mbox series

Index: renesas/arch/arm64/boot/dts/renesas/r8a77970.dtsi
===================================================================
--- renesas.orig/arch/arm64/boot/dts/renesas/r8a77970.dtsi
+++ renesas/arch/arm64/boot/dts/renesas/r8a77970.dtsi
@@ -300,6 +300,19 @@ 
 			#power-domain-cells = <1>;
 		};
 
+		thermal: thermal@e6190000 {
+			compatible = "renesas,thermal-r8a77970";
+			reg =  <0 0xe6190000 0 0x14
+				0 0xe6190100 0 0x38>;
+			interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD 522>;
+			power-domains = <&sysc R8A77970_PD_ALWAYS_ON>;
+			resets = <&cpg 522>;
+			#thermal-sensor-cells = <0>;
+		};
+
 		intc_ex: interrupt-controller@e61c0000 {
 			compatible = "renesas,intc-ex-r8a77970", "renesas,irqc";
 			#interrupt-cells = <2>;
@@ -1033,6 +1046,25 @@ 
 		};
 	};
 
+	thermal-zones {
+		cpu-thermal {
+			polling-delay-passive = <250>;
+			polling-delay = <1000>;
+			thermal-sensors = <&thermal>;
+
+			trips {
+				cpu-crit {
+					temperature = <120000>;
+					hysteresis = <2000>;
+					type = "critical";
+				};
+			};
+
+			cooling-maps {
+			};
+		};
+	};
+
 	timer {
 		compatible = "arm,armv8-timer";
 		interrupts-extended = <&gic GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>,