Message ID | cover.1564091601.git.amit.kucheria@linaro.org |
---|---|
Headers | show |
Series | thermal: qcom: tsens: Add interrupt support | expand |
(Resending from a desktop client because mobile gmail apparently sends html that gets rejected by all lists) On Fri, Jul 26, 2019 at 4:06 PM Brian Masney <masneyb@onstation.org> wrote: > > Hi Amit, > > On Fri, Jul 26, 2019 at 03:48:35AM +0530, Amit Kucheria wrote: > > Add interrupt support to TSENS. The first 6 patches are general fixes and > > cleanups to the driver before interrupt support is introduced. > > > > This series has been developed against qcs404 and sdm845 and then tested on > > msm8916. Testing on msm8998 and msm8974 would be appreciated since I don't > > have hardware handy. Further, I plan to test on msm8996 and also submit to > > kernelci. > > > > I'm sending this out for more review to get help with testing. > > I can test this on msm8974 for you using a Nexus 5. Here's what I've > done so far: Thanks. I was hoping that would be the case given all your effort getting Nexus 5 supported. :-) > The device tree nodes appear in sysfs: > > / # ls -1 /sys/class/thermal/ > cooling_device0 > cooling_device1 > thermal_zone0 > thermal_zone1 > thermal_zone2 > thermal_zone3 > thermal_zone4 > thermal_zone5 > thermal_zone6 > thermal_zone7 > thermal_zone8 > thermal_zone9 Looks good. What are the contents of the files inside the two cooling_device directories? The output of the following command would be nice: $ grep "" cooling_device?/* > The various temperatures were in the upper 40s and I threw some work at > all four CPU cores to warm up the phone and watched the various > temperatures rise: > > / # for i in $(seq 0 9) ; do > > TYPE=$(cat /sys/class/thermal/thermal_zone$i/type) > > TEMP=$(cat /sys/class/thermal/thermal_zone$i/temp) > > echo "$TYPE = $TEMP" > > done > cpu-thermal0 = 66000 > cpu-thermal1 = 66000 > cpu-thermal2 = 66000 > cpu-thermal3 = 66000 > q6-dsp-thermal = 60000 > modemtx-thermal = 57000 > video-thermal = 61000 > wlan-thermal = 65000 > gpu-thermal-top = 61000 > gpu-thermal-bottom = 59000 > > To test the interrupt support, I lowered all of the temperature trips to > 51C but I'm not sure where to read that notification. I assume one of > the cooling devices or a governor should be started? Sorry but I haven't > done any work in the thermal subsystem yet and I'm short on time this > morning to investigate right now. For now, just checking if the tsens interrupt in /proc/interrupts fires should be fine. I have another patch to add some information to debugs that I'll send at some point. How well does cpufreq work on 8974? I haven't looked at it yet but we'll need it for thermal throttling. > Brian
On Sat, Jul 27, 2019 at 12:58:54PM +0530, Amit Kucheria wrote: > On Fri, Jul 26, 2019 at 4:59 PM Brian Masney <masneyb@onstation.org> wrote: > > On Fri, Jul 26, 2019 at 04:40:16PM +0530, Amit Kucheria wrote: > > > How well does cpufreq work on 8974? I haven't looked at it yet but > > > we'll need it for thermal throttling. > > > > I'm not sure how to tell if the frequency is dynamically changed during > > runtime on arm. x86-64 shows this information in /proc/cpuinfo. Here's > > the /proc/cpuinfo on the Nexus 5: > > Nah. /proc/cpuinfo won't show what we need. > > Try the following: > > $ grep "" /sys/devices/system/cpu/cpufreq/policy?/* > > More specifically, the following files have the information you need. > Run watch -n1 on them. > > $ grep "" /sys/devices/system/cpu/cpufreq/policy?/scaling_*_freq There's no cpufreq directory on msm8974: # ls -1 /sys/devices/system/cpu/ cpu0 cpu1 cpu2 cpu3 cpuidle hotplug isolated kernel_max modalias offline online possible power present smt uevent I'm using qcom_defconfig. Brian
On Mon, Jul 29, 2019 at 2:33 PM Luca Weiss <luca@z3ntu.xyz> wrote: > > On Freitag, 26. Juli 2019 00:18:47 CEST Amit Kucheria wrote: > > Register upper-lower interrupt for the tsens controller. > > > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > > --- > > Cc: masneyb@onstation.org > > > > arch/arm/boot/dts/qcom-msm8974.dtsi | 36 +++++++++++++++-------------- > > 1 file changed, 19 insertions(+), 17 deletions(-) > > > > Hi, the title of this patch should be "arm" and not "arm64". Good catch! Copy-paste error, will fix.
On Fri, Jul 26, 2019 at 3:48 AM Amit Kucheria <amit.kucheria@linaro.org> wrote: > > Add interrupt support to TSENS. The first 6 patches are general fixes and > cleanups to the driver before interrupt support is introduced. > > This series has been developed against qcs404 and sdm845 and then tested on > msm8916. Testing on msm8998 and msm8974 would be appreciated since I don't > have hardware handy. Further, I plan to test on msm8996 and also submit to > kernelci. Gentle nudge for reviews. This has now been successfully tested on 8974 (along with 8916, qcs404, sdm845). Testing on msm8998 would be much appreciated. > I'm sending this out for more review to get help with testing. > > Amit Kucheria (15): > drivers: thermal: tsens: Get rid of id field in tsens_sensor > drivers: thermal: tsens: Simplify code flow in tsens_probe > drivers: thermal: tsens: Add __func__ identifier to debug statements > drivers: thermal: tsens: Add debugfs support > arm: dts: msm8974: thermal: Add thermal zones for each sensor > arm64: dts: msm8916: thermal: Fixup HW ids for cpu sensors > dt: thermal: tsens: Document interrupt support in tsens driver > arm64: dts: sdm845: thermal: Add interrupt support > arm64: dts: msm8996: thermal: Add interrupt support > arm64: dts: msm8998: thermal: Add interrupt support > arm64: dts: qcs404: thermal: Add interrupt support > arm64: dts: msm8974: thermal: Add interrupt support > arm64: dts: msm8916: thermal: Add interrupt support > drivers: thermal: tsens: Create function to return sign-extended > temperature > drivers: thermal: tsens: Add interrupt support > > .../bindings/thermal/qcom-tsens.txt | 5 + > arch/arm/boot/dts/qcom-msm8974.dtsi | 108 +++- > arch/arm64/boot/dts/qcom/msm8916.dtsi | 26 +- > arch/arm64/boot/dts/qcom/msm8996.dtsi | 60 +- > arch/arm64/boot/dts/qcom/msm8998.dtsi | 82 +-- > arch/arm64/boot/dts/qcom/qcs404.dtsi | 42 +- > arch/arm64/boot/dts/qcom/sdm845.dtsi | 88 +-- > drivers/thermal/qcom/tsens-8960.c | 4 +- > drivers/thermal/qcom/tsens-common.c | 610 +++++++++++++++++- > drivers/thermal/qcom/tsens-v0_1.c | 11 + > drivers/thermal/qcom/tsens-v1.c | 29 + > drivers/thermal/qcom/tsens-v2.c | 18 + > drivers/thermal/qcom/tsens.c | 52 +- > drivers/thermal/qcom/tsens.h | 285 +++++++- > 14 files changed, 1214 insertions(+), 206 deletions(-) > > -- > 2.17.1 >
On Fri, Jul 26, 2019 at 03:48:42AM +0530, Amit Kucheria wrote: > Define two new required properties to define interrupts and > interrupt-names for tsens. > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > --- > Documentation/devicetree/bindings/thermal/qcom-tsens.txt | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt > index 673cc1831ee9..3d3dd5dc6d36 100644 > --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt > +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt > @@ -22,6 +22,8 @@ Required properties: > > - #thermal-sensor-cells : Should be 1. See ./thermal.txt for a description. > - #qcom,sensors: Number of sensors in tsens block > +- interrupts: Interrupts generated from Always-On subsystem (AOSS) > +- interrupt-names: The name of the interrupt e.g. "tsens0", "tsens1" How many interrupts? A name with just indices isn't too useful. > - Refer to Documentation/devicetree/bindings/nvmem/nvmem.txt to know how to specify > nvmem cells > > @@ -40,6 +42,9 @@ tsens0: thermal-sensor@c263000 { > reg = <0xc263000 0x1ff>, /* TM */ > <0xc222000 0x1ff>; /* SROT */ > #qcom,sensors = <13>; > + interrupts = <GIC_SPI 506 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "tsens0"; > + > #thermal-sensor-cells = <1>; > }; > > -- > 2.17.1 >
On Sat, Aug 17, 2019 at 3:06 AM Rob Herring <robh@kernel.org> wrote: > > On Fri, Jul 26, 2019 at 03:48:42AM +0530, Amit Kucheria wrote: > > Define two new required properties to define interrupts and > > interrupt-names for tsens. > > > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > > --- > > Documentation/devicetree/bindings/thermal/qcom-tsens.txt | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt > > index 673cc1831ee9..3d3dd5dc6d36 100644 > > --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt > > +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt > > @@ -22,6 +22,8 @@ Required properties: > > > > - #thermal-sensor-cells : Should be 1. See ./thermal.txt for a description. > > - #qcom,sensors: Number of sensors in tsens block > > +- interrupts: Interrupts generated from Always-On subsystem (AOSS) > > +- interrupt-names: The name of the interrupt e.g. "tsens0", "tsens1" > > How many interrupts? A name with just indices isn't too useful. Depending on the version of the tsens IP, there can be 1 (upper/lower threshold), 2 (upper/lower + critical threshold) or 3 (upper/lower + critical + zero degree) interrupts. This patch series only introduces support for a single interrupt (upper/lower). I used the names tsens0, tsens1 to encapsulate the controller instance since some SoCs have 1 controller, others have two. So we'll end up with something like the following in DT: tsens0: thermal-sensor@c263000 { compatible = "qcom,sdm845-tsens", "qcom,tsens-v2"; reg = <0 0x0c263000 0 0x1ff>, /* TM */ <0 0x0c222000 0 0x1ff>; /* SROT */ #qcom,sensors = <13>; interrupts = <GIC_SPI 506 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 508 IRQ_TYPE_LEVEL_HIGH>; interrupt-names = "tsens0", "tsens0-critical"; #thermal-sensor-cells = <1>; }; tsens1: thermal-sensor@c265000 { compatible = "qcom,sdm845-tsens", "qcom,tsens-v2"; reg = <0 0x0c265000 0 0x1ff>, /* TM */ <0 0x0c223000 0 0x1ff>; /* SROT */ #qcom,sensors = <8>; interrupts = <GIC_SPI 507 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 509 IRQ_TYPE_LEVEL_HIGH>; interrupt-names = "tsens1", "tsens1-critical"; #thermal-sensor-cells = <1>; } Does that work? Regards, Amit > > - Refer to Documentation/devicetree/bindings/nvmem/nvmem.txt to know how to specify > > nvmem cells > > > > @@ -40,6 +42,9 @@ tsens0: thermal-sensor@c263000 { > > reg = <0xc263000 0x1ff>, /* TM */ > > <0xc222000 0x1ff>; /* SROT */ > > #qcom,sensors = <13>; > > + interrupts = <GIC_SPI 506 IRQ_TYPE_LEVEL_HIGH>; > > + interrupt-names = "tsens0"; > > + > > #thermal-sensor-cells = <1>; > > }; > > > > -- > > 2.17.1 > >
Quoting Amit Kucheria (2019-07-25 15:18:36) > There are two fields - id and hw_id - to track what sensor an action was > to performed on. This was because the sensors connected to a TSENS IP > might not be contiguous i.e. 1, 2, 4, 5 with 3 being skipped. > > This causes confusion in the code which uses hw_id sometimes and id > other times (tsens_get_temp, tsens_get_trend). > > Switch to only using the hw_id field to track the physical ID of the > sensor. When we iterate through all the sensors connected to an IP > block, we use an index i to loop through the list of sensors, and then > return the actual hw_id that is registered on that index. > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > --- Nice cleanup! Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Quoting Amit Kucheria (2019-07-25 15:18:37) > Move platform_set_drvdata up to avoid an extra 'if (ret)' check after > the call to tsens_register. > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > --- Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Quoting Amit Kucheria (2019-07-25 15:18:38) > Printing the function name when enabling debugging makes logs easier to > read. > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > --- Reviewed-by: Stephen Boyd <swboyd@chromium.org>
On Sun, Aug 18, 2019 at 12:55 AM Rob Herring <robh@kernel.org> wrote: > > On Fri, Aug 16, 2019 at 5:02 PM Amit Kucheria <amit.kucheria@linaro.org> wrote: > > > > On Sat, Aug 17, 2019 at 3:06 AM Rob Herring <robh@kernel.org> wrote: > > > > > > On Fri, Jul 26, 2019 at 03:48:42AM +0530, Amit Kucheria wrote: > > > > Define two new required properties to define interrupts and > > > > interrupt-names for tsens. > > > > > > > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > > > > --- > > > > Documentation/devicetree/bindings/thermal/qcom-tsens.txt | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt > > > > index 673cc1831ee9..3d3dd5dc6d36 100644 > > > > --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt > > > > +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt > > > > @@ -22,6 +22,8 @@ Required properties: > > > > > > > > - #thermal-sensor-cells : Should be 1. See ./thermal.txt for a description. > > > > - #qcom,sensors: Number of sensors in tsens block > > > > +- interrupts: Interrupts generated from Always-On subsystem (AOSS) > > > > +- interrupt-names: The name of the interrupt e.g. "tsens0", "tsens1" > > > > > > How many interrupts? A name with just indices isn't too useful. > > > > Depending on the version of the tsens IP, there can be 1 (upper/lower > > threshold), 2 (upper/lower + critical threshold) or 3 (upper/lower + > > critical + zero degree) interrupts. This patch series only introduces > > support for a single interrupt (upper/lower). > > I would expect a different compatible for each possibility. We're currently using the 'qcom,tsens-v1' and 'qcom,tsens-v2' compatibles to broadly capture the feature (and register map) differences. By defining the following, I should be able to check at runtime (using platform_get_irq_by_name() as suggested) if a particular interrupt type is available on the platform, no? So do we really require three different compatibles? interrupt-names = "uplow", "crit", "cold" [1] Respin of older SoC with a newer version of IP > > I used the names tsens0, tsens1 to encapsulate the controller instance > > since some SoCs have 1 controller, others have two. So we'll end up > > with something like the following in DT: > > That's not really how *-names is supposed to work. The name is for > identifying what is at each index. Or to put it another way, a driver > should be able to use platform_get_irq_by_name(). So 'critical', > 'zero' and something for the first one. Fair point. I'll rework it to use "uplow", "crit" and "cold" or something to the effect. Is there another way I get the controller instance index in the name in /proc/interrupts? > > tsens0: thermal-sensor@c263000 { > > compatible = "qcom,sdm845-tsens", "qcom,tsens-v2"; > > reg = <0 0x0c263000 0 0x1ff>, /* TM */ > > <0 0x0c222000 0 0x1ff>; /* SROT */ > > #qcom,sensors = <13>; > > interrupts = <GIC_SPI 506 IRQ_TYPE_LEVEL_HIGH>, > > <GIC_SPI 508 IRQ_TYPE_LEVEL_HIGH>; > > interrupt-names = "tsens0", "tsens0-critical"; > > #thermal-sensor-cells = <1>; > > }; > > > > tsens1: thermal-sensor@c265000 { > > compatible = "qcom,sdm845-tsens", "qcom,tsens-v2"; > > reg = <0 0x0c265000 0 0x1ff>, /* TM */ > > <0 0x0c223000 0 0x1ff>; /* SROT */ > > #qcom,sensors = <8>; > > interrupts = <GIC_SPI 507 IRQ_TYPE_LEVEL_HIGH>, > > <GIC_SPI 509 IRQ_TYPE_LEVEL_HIGH>; > > interrupt-names = "tsens1", "tsens1-critical"; > > #thermal-sensor-cells = <1>; > > } > > > > Does that work? > > > > Regards, > > Amit > > > > > > - Refer to Documentation/devicetree/bindings/nvmem/nvmem.txt to know how to specify > > > > nvmem cells > > > > > > > > @@ -40,6 +42,9 @@ tsens0: thermal-sensor@c263000 { > > > > reg = <0xc263000 0x1ff>, /* TM */ > > > > <0xc222000 0x1ff>; /* SROT */ > > > > #qcom,sensors = <13>; > > > > + interrupts = <GIC_SPI 506 IRQ_TYPE_LEVEL_HIGH>; > > > > + interrupt-names = "tsens0"; > > > > + > > > > #thermal-sensor-cells = <1>; > > > > }; > > > > > > > > -- > > > > 2.17.1 > > > >
On 26/07/2019 00:18, Amit Kucheria wrote: > There are two fields - id and hw_id - to track what sensor an action was > to performed on. This was because the sensors connected to a TSENS IP > might not be contiguous i.e. 1, 2, 4, 5 with 3 being skipped. > > This causes confusion in the code which uses hw_id sometimes and id > other times (tsens_get_temp, tsens_get_trend). > > Switch to only using the hw_id field to track the physical ID of the > sensor. When we iterate through all the sensors connected to an IP > block, we use an index i to loop through the list of sensors, and then > return the actual hw_id that is registered on that index. > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org> -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
On Mon, Aug 19, 2019 at 2:10 AM Amit Kucheria <amit.kucheria@linaro.org> wrote: > > On Sun, Aug 18, 2019 at 12:55 AM Rob Herring <robh@kernel.org> wrote: > > > > On Fri, Aug 16, 2019 at 5:02 PM Amit Kucheria <amit.kucheria@linaro.org> wrote: > > > > > > On Sat, Aug 17, 2019 at 3:06 AM Rob Herring <robh@kernel.org> wrote: > > > > > > > > On Fri, Jul 26, 2019 at 03:48:42AM +0530, Amit Kucheria wrote: > > > > > Define two new required properties to define interrupts and > > > > > interrupt-names for tsens. > > > > > > > > > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > > > > > --- > > > > > Documentation/devicetree/bindings/thermal/qcom-tsens.txt | 5 +++++ > > > > > 1 file changed, 5 insertions(+) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt > > > > > index 673cc1831ee9..3d3dd5dc6d36 100644 > > > > > --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt > > > > > +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt > > > > > @@ -22,6 +22,8 @@ Required properties: > > > > > > > > > > - #thermal-sensor-cells : Should be 1. See ./thermal.txt for a description. > > > > > - #qcom,sensors: Number of sensors in tsens block > > > > > +- interrupts: Interrupts generated from Always-On subsystem (AOSS) > > > > > +- interrupt-names: The name of the interrupt e.g. "tsens0", "tsens1" > > > > > > > > How many interrupts? A name with just indices isn't too useful. > > > > > > Depending on the version of the tsens IP, there can be 1 (upper/lower > > > threshold), 2 (upper/lower + critical threshold) or 3 (upper/lower + > > > critical + zero degree) interrupts. This patch series only introduces > > > support for a single interrupt (upper/lower). > > > > I would expect a different compatible for each possibility. > > We're currently using the 'qcom,tsens-v1' and 'qcom,tsens-v2' > compatibles to broadly capture the feature (and register map) > differences. > > By defining the following, I should be able to check at runtime (using > platform_get_irq_by_name() as suggested) if a particular interrupt > type is available on the platform, no? So do we really require three > different compatibles? Yes and no. I would assume the SoC specific compatibles would meet this, but the driver can ignore that and just use platform_get_irq_by_name() or count the number of interrupts. > interrupt-names = "uplow", "crit", "cold" > > [1] Respin of older SoC with a newer version of IP > > > > I used the names tsens0, tsens1 to encapsulate the controller instance > > > since some SoCs have 1 controller, others have two. So we'll end up > > > with something like the following in DT: > > > > That's not really how *-names is supposed to work. The name is for > > identifying what is at each index. Or to put it another way, a driver > > should be able to use platform_get_irq_by_name(). So 'critical', > > 'zero' and something for the first one. > > Fair point. I'll rework it to use "uplow", "crit" and "cold" or > something to the effect. > > Is there another way I get the controller instance index in the name > in /proc/interrupts? Not sure offhand. Add the dev_name() into the IRQ name perhaps. Rob
On 26/07/2019 00:18, Amit Kucheria wrote: > Printing the function name when enabling debugging makes logs easier to > read. > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > --- Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org> -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog