| Message ID | 20240126-sc8280xp-tsens2_3-v2-1-8504d18828de@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] arm64: dts: qcom: sc8280xp: Introduce additional tsens instances | expand |
On Fri, Jan 26, 2024 at 07:12:45AM -0800, Bjorn Andersson wrote: > The SC8280XP contains two additional tsens instances, providing among > other things thermal measurements for the GPU. > > Add these and a GPU thermal-zone. > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- > Changes in v2: > - Drop TM/SROT comments > - Remove polling delays, rely on interrupts > - Link to v1: https://lore.kernel.org/r/20240118-sc8280xp-tsens2_3-v1-1-e86bce14f6bf@quicinc.com > --- > arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 37 ++++++++++++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > index febf28356ff8..7bfbb1bd8f4a 100644 > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > @@ -4033,6 +4033,28 @@ tsens1: thermal-sensor@c265000 { > #thermal-sensor-cells = <1>; > }; > > + tsens2: thermal-sensor@c251000 { > + compatible = "qcom,sc8280xp-tsens", "qcom,tsens-v2"; > + reg = <0 0x0c251000 0 0x1ff>, > + <0 0x0c224000 0 0x8>; > + #qcom,sensors = <11>; > + interrupts-extended = <&pdc 122 IRQ_TYPE_LEVEL_HIGH>, > + <&pdc 124 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "uplow", "critical"; > + #thermal-sensor-cells = <1>; > + }; > + > + tsens3: thermal-sensor@c252000 { > + compatible = "qcom,sc8280xp-tsens", "qcom,tsens-v2"; > + reg = <0 0x0c252000 0 0x1ff>, > + <0 0x0c225000 0 0x8>; > + #qcom,sensors = <5>; > + interrupts-extended = <&pdc 123 IRQ_TYPE_LEVEL_HIGH>, > + <&pdc 125 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "uplow", "critical"; > + #thermal-sensor-cells = <1>; > + }; These should go before tsens0 based on the unit address. > + > aoss_qmp: power-management@c300000 { > compatible = "qcom,sc8280xp-aoss-qmp", "qcom,aoss-qmp"; > reg = <0 0x0c300000 0 0x400>; > @@ -5212,6 +5234,21 @@ cpu-crit { > }; > }; > > + gpu-thermal { > + polling-delay-passive = <0>; > + polling-delay = <0>; > + > + thermal-sensors = <&tsens2 2>; > + > + trips { > + cpu-crit { > + temperature = <110000>; > + hysteresis = <1000>; > + type = "critical"; > + }; > + }; > + }; Shall you submit a follow-on patch to set the polling delays to zero for the other thermal zones (cpu, cluster, mem) so that we don't poll for those? Looks good to me otherwise: Reviewed-by: Johan Hovold <johan+linaro@kernel.org> Johan
On Fri, Jan 26, 2024 at 05:36:10PM +0100, Johan Hovold wrote: > On Fri, Jan 26, 2024 at 07:12:45AM -0800, Bjorn Andersson wrote: > > The SC8280XP contains two additional tsens instances, providing among > > other things thermal measurements for the GPU. > > > > Add these and a GPU thermal-zone. > > > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > > --- > > Changes in v2: > > - Drop TM/SROT comments > > - Remove polling delays, rely on interrupts > > - Link to v1: https://lore.kernel.org/r/20240118-sc8280xp-tsens2_3-v1-1-e86bce14f6bf@quicinc.com > > --- > > arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 37 ++++++++++++++++++++++++++++++++++ > > 1 file changed, 37 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > > index febf28356ff8..7bfbb1bd8f4a 100644 > > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > > @@ -4033,6 +4033,28 @@ tsens1: thermal-sensor@c265000 { > > #thermal-sensor-cells = <1>; > > }; > > > > + tsens2: thermal-sensor@c251000 { > > + compatible = "qcom,sc8280xp-tsens", "qcom,tsens-v2"; > > + reg = <0 0x0c251000 0 0x1ff>, > > + <0 0x0c224000 0 0x8>; > > + #qcom,sensors = <11>; > > + interrupts-extended = <&pdc 122 IRQ_TYPE_LEVEL_HIGH>, > > + <&pdc 124 IRQ_TYPE_LEVEL_HIGH>; > > + interrupt-names = "uplow", "critical"; > > + #thermal-sensor-cells = <1>; > > + }; > > + > > + tsens3: thermal-sensor@c252000 { > > + compatible = "qcom,sc8280xp-tsens", "qcom,tsens-v2"; > > + reg = <0 0x0c252000 0 0x1ff>, > > + <0 0x0c225000 0 0x8>; > > + #qcom,sensors = <5>; > > + interrupts-extended = <&pdc 123 IRQ_TYPE_LEVEL_HIGH>, > > + <&pdc 125 IRQ_TYPE_LEVEL_HIGH>; > > + interrupt-names = "uplow", "critical"; > > + #thermal-sensor-cells = <1>; > > + }; > > These should go before tsens0 based on the unit address. > You're right, thanks for spotting that. > > + > > aoss_qmp: power-management@c300000 { > > compatible = "qcom,sc8280xp-aoss-qmp", "qcom,aoss-qmp"; > > reg = <0 0x0c300000 0 0x400>; > > @@ -5212,6 +5234,21 @@ cpu-crit { > > }; > > }; > > > > + gpu-thermal { > > + polling-delay-passive = <0>; > > + polling-delay = <0>; > > + > > + thermal-sensors = <&tsens2 2>; > > + > > + trips { > > + cpu-crit { > > + temperature = <110000>; > > + hysteresis = <1000>; > > + type = "critical"; > > + }; > > + }; > > + }; > > Shall you submit a follow-on patch to set the polling delays to zero > for the other thermal zones (cpu, cluster, mem) so that we don't poll > for those? > I optimistically interpreted Konrad's response as a promise by him to do so ;) I do like his patch which remove the poll-properties for non-polling mode. Would be nice to not first change the values to 0 and then remove the properties... > Looks good to me otherwise: > > Reviewed-by: Johan Hovold <johan+linaro@kernel.org> > Thanks, Bjorn > Johan
On Fri, Jan 26, 2024 at 08:51:13AM -0800, Bjorn Andersson wrote: > On Fri, Jan 26, 2024 at 05:36:10PM +0100, Johan Hovold wrote: > > Shall you submit a follow-on patch to set the polling delays to zero > > for the other thermal zones (cpu, cluster, mem) so that we don't poll > > for those? > > I optimistically interpreted Konrad's response as a promise by him to do > so ;) > > I do like his patch which remove the poll-properties for non-polling > mode. Would be nice to not first change the values to 0 and then remove > the properties... No, that should not be an issue as it allows us to get rid of the polling without waiting for a binding update which may or may not materialise in 6.9-rc1. But whoever updates those properties need to do some proper testing to make sure that those interrupts really work. Johan
On 26.01.2024 16:12, Bjorn Andersson wrote: > The SC8280XP contains two additional tsens instances, providing among > other things thermal measurements for the GPU. > > Add these and a GPU thermal-zone. > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- > Changes in v2: > - Drop TM/SROT comments > - Remove polling delays, rely on interrupts > - Link to v1: https://lore.kernel.org/r/20240118-sc8280xp-tsens2_3-v1-1-e86bce14f6bf@quicinc.com > --- > arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 37 ++++++++++++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > index febf28356ff8..7bfbb1bd8f4a 100644 > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > @@ -4033,6 +4033,28 @@ tsens1: thermal-sensor@c265000 { > #thermal-sensor-cells = <1>; > }; > > + tsens2: thermal-sensor@c251000 { > + compatible = "qcom,sc8280xp-tsens", "qcom,tsens-v2"; > + reg = <0 0x0c251000 0 0x1ff>, > + <0 0x0c224000 0 0x8>; > + #qcom,sensors = <11>; > + interrupts-extended = <&pdc 122 IRQ_TYPE_LEVEL_HIGH>, > + <&pdc 124 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "uplow", "critical"; > + #thermal-sensor-cells = <1>; > + }; > + > + tsens3: thermal-sensor@c252000 { > + compatible = "qcom,sc8280xp-tsens", "qcom,tsens-v2"; > + reg = <0 0x0c252000 0 0x1ff>, > + <0 0x0c225000 0 0x8>; > + #qcom,sensors = <5>; > + interrupts-extended = <&pdc 123 IRQ_TYPE_LEVEL_HIGH>, > + <&pdc 125 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "uplow", "critical"; > + #thermal-sensor-cells = <1>; With the sorting issue that Johan mentioned resolved: Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> Konrad
On 26.01.2024 18:00, Johan Hovold wrote: > On Fri, Jan 26, 2024 at 08:51:13AM -0800, Bjorn Andersson wrote: >> On Fri, Jan 26, 2024 at 05:36:10PM +0100, Johan Hovold wrote: > >>> Shall you submit a follow-on patch to set the polling delays to zero >>> for the other thermal zones (cpu, cluster, mem) so that we don't poll >>> for those? >> >> I optimistically interpreted Konrad's response as a promise by him to do >> so ;) >> >> I do like his patch which remove the poll-properties for non-polling >> mode. Would be nice to not first change the values to 0 and then remove >> the properties... That was my intention as well.. > > No, that should not be an issue as it allows us to get rid of the > polling without waiting for a binding update which may or may not > materialise in 6.9-rc1. If you really insist, I may do that, but if the thermal guys act on it quickly and we negotiate an immutable branch, we can simply but atop it, saving the submitter timeof(patchset), the reviewers timeof(verify), the build bots timeof(builds) and the applier timeof(pick-build-push).. > > But whoever updates those properties need to do some proper testing to > make sure that those interrupts really work. They seem to, check /proc/interrupts before and after adding an e.g. 45degC trip point on one of the CPU thermal zones, they fire aplenty. Konrad
On Fri, Jan 26, 2024 at 10:23:41PM +0100, Konrad Dybcio wrote: > On 26.01.2024 18:00, Johan Hovold wrote: > > On Fri, Jan 26, 2024 at 08:51:13AM -0800, Bjorn Andersson wrote: > >> On Fri, Jan 26, 2024 at 05:36:10PM +0100, Johan Hovold wrote: > > > >>> Shall you submit a follow-on patch to set the polling delays to zero > >>> for the other thermal zones (cpu, cluster, mem) so that we don't poll > >>> for those? > >> > >> I optimistically interpreted Konrad's response as a promise by him to do > >> so ;) > >> > >> I do like his patch which remove the poll-properties for non-polling > >> mode. Would be nice to not first change the values to 0 and then remove > >> the properties... > > That was my intention as well.. > > > > > No, that should not be an issue as it allows us to get rid of the > > polling without waiting for a binding update which may or may not > > materialise in 6.9-rc1. > > If you really insist, I may do that, but if the thermal guys act on it > quickly and we negotiate an immutable branch, we can simply but atop it, > saving the submitter timeof(patchset), the reviewers timeof(verify), the > build bots timeof(builds) and the applier timeof(pick-build-push).. Why would introduce such a dependency for really no good reason? Submit a patch based on the current binding, then when/if your proposed binding update hits mainline, you can send a *single* patch dropping the parameters from all qualcomm dtsi. Updating the binding is a separate and lower priority task. In fact, it may not even be desirable at all as an omission of adding these parameters could then lead to broken thermal management on platforms where the interrupts do not work. Having an explicit poll-delay of zero at least gives people a reason to think about it when merging a new platform. But again, that's a separate discussion. Don't make this patch depend on that. > > But whoever updates those properties need to do some proper testing to > > make sure that those interrupts really work. > > They seem to, check /proc/interrupts before and after adding an e.g. 45degC > trip point on one of the CPU thermal zones, they fire aplenty. That's not proper testing. Add/enable debugging in the thermal driver and make sure that you trigger precisely once when passing the threshold in both directions. Johan
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi index febf28356ff8..7bfbb1bd8f4a 100644 --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi @@ -4033,6 +4033,28 @@ tsens1: thermal-sensor@c265000 { #thermal-sensor-cells = <1>; }; + tsens2: thermal-sensor@c251000 { + compatible = "qcom,sc8280xp-tsens", "qcom,tsens-v2"; + reg = <0 0x0c251000 0 0x1ff>, + <0 0x0c224000 0 0x8>; + #qcom,sensors = <11>; + interrupts-extended = <&pdc 122 IRQ_TYPE_LEVEL_HIGH>, + <&pdc 124 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "uplow", "critical"; + #thermal-sensor-cells = <1>; + }; + + tsens3: thermal-sensor@c252000 { + compatible = "qcom,sc8280xp-tsens", "qcom,tsens-v2"; + reg = <0 0x0c252000 0 0x1ff>, + <0 0x0c225000 0 0x8>; + #qcom,sensors = <5>; + interrupts-extended = <&pdc 123 IRQ_TYPE_LEVEL_HIGH>, + <&pdc 125 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "uplow", "critical"; + #thermal-sensor-cells = <1>; + }; + aoss_qmp: power-management@c300000 { compatible = "qcom,sc8280xp-aoss-qmp", "qcom,aoss-qmp"; reg = <0 0x0c300000 0 0x400>; @@ -5212,6 +5234,21 @@ cpu-crit { }; }; + gpu-thermal { + polling-delay-passive = <0>; + polling-delay = <0>; + + thermal-sensors = <&tsens2 2>; + + trips { + cpu-crit { + temperature = <110000>; + hysteresis = <1000>; + type = "critical"; + }; + }; + }; + mem-thermal { polling-delay-passive = <250>; polling-delay = <1000>;
The SC8280XP contains two additional tsens instances, providing among other things thermal measurements for the GPU. Add these and a GPU thermal-zone. Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> --- Changes in v2: - Drop TM/SROT comments - Remove polling delays, rely on interrupts - Link to v1: https://lore.kernel.org/r/20240118-sc8280xp-tsens2_3-v1-1-e86bce14f6bf@quicinc.com --- arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 37 ++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) --- base-commit: 943b9f0ab2cfbaea148dd6ac279957eb08b96904 change-id: 20240118-sc8280xp-tsens2_3-a5fd9a48d655 Best regards,