| Message ID | 20200604004331.669936-5-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/7] arm64: dts: qcom: pm8009: Add base dts file | expand |
On 04-06-20, 03:43, Dmitry Baryshkov wrote: > Add temperature alarm and thermal zone configuration to all three > pm8150 instances. Configuration is largely based on the msm-4.19 tree. > These alarms use main adc of the pmic. Separate temperature adc is not > supported yet. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > arch/arm64/boot/dts/qcom/pm8150.dtsi | 41 +++++++++++++++++++++++-- > arch/arm64/boot/dts/qcom/pm8150b.dtsi | 43 +++++++++++++++++++++++++-- > arch/arm64/boot/dts/qcom/pm8150l.dtsi | 43 +++++++++++++++++++++++++-- > 3 files changed, 119 insertions(+), 8 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/pm8150.dtsi b/arch/arm64/boot/dts/qcom/pm8150.dtsi > index c0b197458665..fee2db42f4cb 100644 > --- a/arch/arm64/boot/dts/qcom/pm8150.dtsi > +++ b/arch/arm64/boot/dts/qcom/pm8150.dtsi > @@ -30,6 +30,15 @@ pwrkey { > }; > }; > > + pm8150_temp: temp-alarm@2400 { > + compatible = "qcom,spmi-temp-alarm"; > + reg = <0x2400>; > + interrupts = <0x0 0x24 0x0 IRQ_TYPE_EDGE_BOTH>; > + io-channels = <&pm8150_adc ADC5_DIE_TEMP>; > + io-channel-names = "thermal"; > + #thermal-sensor-cells = <0>; > + }; > + > pm8150_adc: adc@3100 { > compatible = "qcom,spmi-adc5"; > reg = <0x3100>; > @@ -38,8 +47,6 @@ pm8150_adc: adc@3100 { > #io-channel-cells = <1>; > interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>; > > - status = "disabled"; > - This should not be removed, rather than this please add enabled in you board dts file > ref-gnd@0 { > reg = <ADC5_REF_GND>; > qcom,pre-scaling = <1 1>; > @@ -85,3 +92,33 @@ pmic@1 { > #size-cells = <0>; > }; > }; > + > +&thermal_zones { > + pm8150_temp { > + polling-delay-passive = <0>; > + polling-delay = <0>; > + > + thermal-sensors = <&pm8150_temp>; > + > + trips { > + trip0 { > + temperature = <95000>; > + hysteresis = <0>; > + type = "passive"; > + }; > + > + trip1 { > + temperature = <115000>; > + hysteresis = <0>; > + type = "passive"; > + }; > + > + trip2 { > + temperature = <145000>; > + hysteresis = <0>; > + type = "passive"; > + }; > + }; > + > + }; Not sure about this, Amit..? Should this also not be in board dts? Similar comments on similar ones for rest of the patch as well..
On 04/06/2020 13:47, Vinod Koul wrote: > On 04-06-20, 03:43, Dmitry Baryshkov wrote: >> Add temperature alarm and thermal zone configuration to all three >> pm8150 instances. Configuration is largely based on the msm-4.19 tree. >> These alarms use main adc of the pmic. Separate temperature adc is not >> supported yet. >> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> arch/arm64/boot/dts/qcom/pm8150.dtsi | 41 +++++++++++++++++++++++-- >> arch/arm64/boot/dts/qcom/pm8150b.dtsi | 43 +++++++++++++++++++++++++-- >> arch/arm64/boot/dts/qcom/pm8150l.dtsi | 43 +++++++++++++++++++++++++-- >> 3 files changed, 119 insertions(+), 8 deletions(-) >> >> diff --git a/arch/arm64/boot/dts/qcom/pm8150.dtsi b/arch/arm64/boot/dts/qcom/pm8150.dtsi >> index c0b197458665..fee2db42f4cb 100644 >> --- a/arch/arm64/boot/dts/qcom/pm8150.dtsi >> +++ b/arch/arm64/boot/dts/qcom/pm8150.dtsi >> @@ -30,6 +30,15 @@ pwrkey { >> }; >> }; >> >> + pm8150_temp: temp-alarm@2400 { >> + compatible = "qcom,spmi-temp-alarm"; >> + reg = <0x2400>; >> + interrupts = <0x0 0x24 0x0 IRQ_TYPE_EDGE_BOTH>; >> + io-channels = <&pm8150_adc ADC5_DIE_TEMP>; >> + io-channel-names = "thermal"; >> + #thermal-sensor-cells = <0>; >> + }; >> + >> pm8150_adc: adc@3100 { >> compatible = "qcom,spmi-adc5"; >> reg = <0x3100>; >> @@ -38,8 +47,6 @@ pm8150_adc: adc@3100 { >> #io-channel-cells = <1>; >> interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>; >> >> - status = "disabled"; >> - > > This should not be removed, rather than this please add enabled in you > board dts file > >> ref-gnd@0 { >> reg = <ADC5_REF_GND>; >> qcom,pre-scaling = <1 1>; >> @@ -85,3 +92,33 @@ pmic@1 { >> #size-cells = <0>; >> }; >> }; >> + >> +&thermal_zones { >> + pm8150_temp { >> + polling-delay-passive = <0>; >> + polling-delay = <0>; >> + >> + thermal-sensors = <&pm8150_temp>; >> + >> + trips { >> + trip0 { >> + temperature = <95000>; >> + hysteresis = <0>; >> + type = "passive"; >> + }; >> + >> + trip1 { >> + temperature = <115000>; >> + hysteresis = <0>; >> + type = "passive"; >> + }; >> + >> + trip2 { >> + temperature = <145000>; >> + hysteresis = <0>; >> + type = "passive"; >> + }; >> + }; >> + >> + }; > > Not sure about this, Amit..? Should this also not be in board dts? > > Similar comments on similar ones for rest of the patch as well.. I'm not so sure. This part of the configuration seems generic to me. Unlike adc-tm config, which definitely goes to the board file. I can split this into a separate pm8150-temp.dtsi file. Does that sound better?
Sorry missed ccing Amit, done now. On 04-06-20, 18:03, Dmitry Baryshkov wrote: > On 04/06/2020 13:47, Vinod Koul wrote: > > On 04-06-20, 03:43, Dmitry Baryshkov wrote: > > > pm8150_adc: adc@3100 { > > > compatible = "qcom,spmi-adc5"; > > > reg = <0x3100>; > > > @@ -38,8 +47,6 @@ pm8150_adc: adc@3100 { > > > #io-channel-cells = <1>; > > > interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>; > > > - status = "disabled"; > > > - > > > > This should not be removed, rather than this please add enabled in you > > board dts file ... > > > +&thermal_zones { > > > + pm8150_temp { > > > + polling-delay-passive = <0>; > > > + polling-delay = <0>; > > > + > > > + thermal-sensors = <&pm8150_temp>; > > > + > > > + trips { > > > + trip0 { > > > + temperature = <95000>; > > > + hysteresis = <0>; > > > + type = "passive"; > > > + }; > > > + > > > + trip1 { > > > + temperature = <115000>; > > > + hysteresis = <0>; > > > + type = "passive"; > > > + }; > > > + > > > + trip2 { > > > + temperature = <145000>; > > > + hysteresis = <0>; > > > + type = "passive"; > > > + }; > > > + }; > > > + > > > + }; > > > > Not sure about this, Amit..? Should this also not be in board dts? > > > > Similar comments on similar ones for rest of the patch as well.. > > I'm not so sure. This part of the configuration seems generic to me. Unlike > adc-tm config, which definitely goes to the board file. I think the temperature values may be board specific, Amit can confirm that. If that is the case then this belongs to board dts, otherwise here :) > I can split this into a separate pm8150-temp.dtsi file. Does that sound > better? That might make it worse, we don't do splitting.
Hello, On Fri, 5 Jun 2020 at 07:40, Vinod Koul <vkoul@kernel.org> wrote: > > > Sorry missed ccing Amit, done now. > > On 04-06-20, 18:03, Dmitry Baryshkov wrote: > > On 04/06/2020 13:47, Vinod Koul wrote: > > > On 04-06-20, 03:43, Dmitry Baryshkov wrote: > > > > > pm8150_adc: adc@3100 { > > > > compatible = "qcom,spmi-adc5"; > > > > reg = <0x3100>; > > > > @@ -38,8 +47,6 @@ pm8150_adc: adc@3100 { > > > > #io-channel-cells = <1>; > > > > interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>; > > > > - status = "disabled"; > > > > - > > > > > > This should not be removed, rather than this please add enabled in you > > > board dts file Compare this with pm8998.dtsi. It has all nodes enabled by default. > > > > +&thermal_zones { > > > > + pm8150_temp { > > > > + polling-delay-passive = <0>; > > > > + polling-delay = <0>; > > > > + > > > > + thermal-sensors = <&pm8150_temp>; > > > > + > > > > + trips { > > > > + trip0 { > > > > + temperature = <95000>; > > > > + hysteresis = <0>; > > > > + type = "passive"; > > > > + }; > > > > + > > > > + trip1 { > > > > + temperature = <115000>; > > > > + hysteresis = <0>; > > > > + type = "passive"; > > > > + }; > > > > + > > > > + trip2 { > > > > + temperature = <145000>; > > > > + hysteresis = <0>; > > > > + type = "passive"; > > > > + }; > > > > + }; > > > > + > > > > + }; > > > > > > Not sure about this, Amit..? Should this also not be in board dts? > > > > > > Similar comments on similar ones for rest of the patch as well.. > > > > I'm not so sure. This part of the configuration seems generic to me. Unlike > > adc-tm config, which definitely goes to the board file. > > I think the temperature values may be board specific, Amit can confirm > that. If that is the case then this belongs to board dts, otherwise here :) Again, pm8998 has these thermal values in the dtsi file. In V2 I will update these three files to follow pm8998.dtsi.
On Fri, Jun 5, 2020 at 10:10 AM Vinod Koul <vkoul@kernel.org> wrote: > > > Sorry missed ccing Amit, done now. > > On 04-06-20, 18:03, Dmitry Baryshkov wrote: > > On 04/06/2020 13:47, Vinod Koul wrote: > > > On 04-06-20, 03:43, Dmitry Baryshkov wrote: > > > > > pm8150_adc: adc@3100 { > > > > compatible = "qcom,spmi-adc5"; > > > > reg = <0x3100>; > > > > @@ -38,8 +47,6 @@ pm8150_adc: adc@3100 { > > > > #io-channel-cells = <1>; > > > > interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>; > > > > - status = "disabled"; > > > > - > > > > > > This should not be removed, rather than this please add enabled in you > > > board dts file Is the default disabled for a reason? I'd expect the default to be enabled and then board-specific dts to disable a specific adc if needed. > ... > > > > > +&thermal_zones { > > > > + pm8150_temp { > > > > + polling-delay-passive = <0>; > > > > + polling-delay = <0>; > > > > + > > > > + thermal-sensors = <&pm8150_temp>; > > > > + > > > > + trips { > > > > + trip0 { > > > > + temperature = <95000>; > > > > + hysteresis = <0>; > > > > + type = "passive"; > > > > + }; > > > > + > > > > + trip1 { > > > > + temperature = <115000>; > > > > + hysteresis = <0>; > > > > + type = "passive"; Since there is not cooling map associated with these two trips i.e. no mitigation action, this trip is informational. So make it of type "hot". Is there really a need for two passive trip points? Just one at 115 should be enough? > > > > + }; > > > > + > > > > + trip2 { > > > > + temperature = <145000>; Are you sure about this? That is a very toasty temperature. :-) > > > > + hysteresis = <0>; > > > > + type = "passive"; The last trip should typically be of type "critical". That is the temperature at which the system will initiate a shutdown. > > > > + }; > > > > + }; > > > > + > > > > + }; > > > > > > Not sure about this, Amit..? Should this also not be in board dts? > > > > > > Similar comments on similar ones for rest of the patch as well.. > > > > I'm not so sure. This part of the configuration seems generic to me. Unlike > > adc-tm config, which definitely goes to the board file. > > I think the temperature values may be board specific, Amit can confirm > that. If that is the case then this belongs to board dts, otherwise here :) While the temp values can be board-specific e.g. if the same SoC is used in a mobile phone and a laptop, the thresholds rarely change, in my experience. I think they can stay in the pmic dtsi file and any specific board can override if necessary. > > I can split this into a separate pm8150-temp.dtsi file. Does that sound > > better? > > That might make it worse, we don't do splitting. Right, let's not split it. > > -- > ~Vinod
diff --git a/arch/arm64/boot/dts/qcom/pm8150.dtsi b/arch/arm64/boot/dts/qcom/pm8150.dtsi index c0b197458665..fee2db42f4cb 100644 --- a/arch/arm64/boot/dts/qcom/pm8150.dtsi +++ b/arch/arm64/boot/dts/qcom/pm8150.dtsi @@ -30,6 +30,15 @@ pwrkey { }; }; + pm8150_temp: temp-alarm@2400 { + compatible = "qcom,spmi-temp-alarm"; + reg = <0x2400>; + interrupts = <0x0 0x24 0x0 IRQ_TYPE_EDGE_BOTH>; + io-channels = <&pm8150_adc ADC5_DIE_TEMP>; + io-channel-names = "thermal"; + #thermal-sensor-cells = <0>; + }; + pm8150_adc: adc@3100 { compatible = "qcom,spmi-adc5"; reg = <0x3100>; @@ -38,8 +47,6 @@ pm8150_adc: adc@3100 { #io-channel-cells = <1>; interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>; - status = "disabled"; - ref-gnd@0 { reg = <ADC5_REF_GND>; qcom,pre-scaling = <1 1>; @@ -85,3 +92,33 @@ pmic@1 { #size-cells = <0>; }; }; + +&thermal_zones { + pm8150_temp { + polling-delay-passive = <0>; + polling-delay = <0>; + + thermal-sensors = <&pm8150_temp>; + + trips { + trip0 { + temperature = <95000>; + hysteresis = <0>; + type = "passive"; + }; + + trip1 { + temperature = <115000>; + hysteresis = <0>; + type = "passive"; + }; + + trip2 { + temperature = <145000>; + hysteresis = <0>; + type = "passive"; + }; + }; + + }; +}; diff --git a/arch/arm64/boot/dts/qcom/pm8150b.dtsi b/arch/arm64/boot/dts/qcom/pm8150b.dtsi index 40b5d75a4a1d..e93d16f2d1e0 100644 --- a/arch/arm64/boot/dts/qcom/pm8150b.dtsi +++ b/arch/arm64/boot/dts/qcom/pm8150b.dtsi @@ -22,7 +22,16 @@ power-on@800 { status = "disabled"; }; - adc@3100 { + pm8150b_temp: temp-alarm@2400 { + compatible = "qcom,spmi-temp-alarm"; + reg = <0x2400>; + interrupts = <0x2 0x24 0x0 IRQ_TYPE_EDGE_BOTH>; + io-channels = <&pm8150b_adc ADC5_DIE_TEMP>; + io-channel-names = "thermal"; + #thermal-sensor-cells = <0>; + }; + + pm8150b_adc: adc@3100 { compatible = "qcom,spmi-adc5"; reg = <0x3100>; #address-cells = <1>; @@ -30,8 +39,6 @@ adc@3100 { #io-channel-cells = <1>; interrupts = <0x2 0x31 0x0 IRQ_TYPE_EDGE_RISING>; - status = "disabled"; - ref-gnd@0 { reg = <ADC5_REF_GND>; qcom,pre-scaling = <1 1>; @@ -74,3 +81,33 @@ pmic@3 { #size-cells = <0>; }; }; + +&thermal_zones { + pm8150b_temp { + polling-delay-passive = <0>; + polling-delay = <0>; + + thermal-sensors = <&pm8150b_temp>; + + trips { + trip0 { + temperature = <95000>; + hysteresis = <0>; + type = "passive"; + }; + + trip1 { + temperature = <115000>; + hysteresis = <0>; + type = "passive"; + }; + + trip2 { + temperature = <145000>; + hysteresis = <0>; + type = "passive"; + }; + }; + + }; +}; diff --git a/arch/arm64/boot/dts/qcom/pm8150l.dtsi b/arch/arm64/boot/dts/qcom/pm8150l.dtsi index cf05e0685d10..1edf87c95a27 100644 --- a/arch/arm64/boot/dts/qcom/pm8150l.dtsi +++ b/arch/arm64/boot/dts/qcom/pm8150l.dtsi @@ -22,7 +22,16 @@ power-on@800 { status = "disabled"; }; - adc@3100 { + pm8150l_temp: temp-alarm@2400 { + compatible = "qcom,spmi-temp-alarm"; + reg = <0x2400>; + interrupts = <0x4 0x24 0x0 IRQ_TYPE_EDGE_BOTH>; + io-channels = <&pm8150l_adc ADC5_DIE_TEMP>; + io-channel-names = "thermal"; + #thermal-sensor-cells = <0>; + }; + + pm8150l_adc: adc@3100 { compatible = "qcom,spmi-adc5"; reg = <0x3100>; #address-cells = <1>; @@ -30,8 +39,6 @@ adc@3100 { #io-channel-cells = <1>; interrupts = <0x4 0x31 0x0 IRQ_TYPE_EDGE_RISING>; - status = "disabled"; - ref-gnd@0 { reg = <ADC5_REF_GND>; qcom,pre-scaling = <1 1>; @@ -68,3 +75,33 @@ pmic@5 { #size-cells = <0>; }; }; + +&thermal_zones { + pm8150l_temp { + polling-delay-passive = <0>; + polling-delay = <0>; + + thermal-sensors = <&pm8150l_temp>; + + trips { + trip0 { + temperature = <95000>; + hysteresis = <0>; + type = "passive"; + }; + + trip1 { + temperature = <115000>; + hysteresis = <0>; + type = "passive"; + }; + + trip2 { + temperature = <145000>; + hysteresis = <0>; + type = "passive"; + }; + }; + + }; +};
Add temperature alarm and thermal zone configuration to all three pm8150 instances. Configuration is largely based on the msm-4.19 tree. These alarms use main adc of the pmic. Separate temperature adc is not supported yet. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- arch/arm64/boot/dts/qcom/pm8150.dtsi | 41 +++++++++++++++++++++++-- arch/arm64/boot/dts/qcom/pm8150b.dtsi | 43 +++++++++++++++++++++++++-- arch/arm64/boot/dts/qcom/pm8150l.dtsi | 43 +++++++++++++++++++++++++-- 3 files changed, 119 insertions(+), 8 deletions(-)