mbox series

[00/15] thermal: qcom: tsens: Add interrupt support

Message ID cover.1564091601.git.amit.kucheria@linaro.org
Headers show
Series thermal: qcom: tsens: Add interrupt support | expand

Message

Amit Kucheria July 25, 2019, 10:18 p.m. UTC
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.

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

Comments

Amit Kucheria July 26, 2019, 11:10 a.m. UTC | #1
(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
Brian Masney July 29, 2019, 9:07 a.m. UTC | #2
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
Amit Kucheria July 29, 2019, 9:29 a.m. UTC | #3
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.
Amit Kucheria Aug. 8, 2019, 1:04 p.m. UTC | #4
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

>
Rob Herring Aug. 16, 2019, 9:36 p.m. UTC | #5
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

>
Amit Kucheria Aug. 16, 2019, 10:02 p.m. UTC | #6
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

> >
Stephen Boyd Aug. 17, 2019, 4:01 a.m. UTC | #7
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>
Stephen Boyd Aug. 17, 2019, 4:02 a.m. UTC | #8
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>
Stephen Boyd Aug. 17, 2019, 4:03 a.m. UTC | #9
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>
Amit Kucheria Aug. 19, 2019, 7:10 a.m. UTC | #10
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

> > > >
Daniel Lezcano Aug. 19, 2019, 11:55 a.m. UTC | #11
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
Rob Herring Aug. 19, 2019, 1:07 p.m. UTC | #12
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
Daniel Lezcano Aug. 19, 2019, 1:19 p.m. UTC | #13
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