Message-ID: <451416a9-3caa-50d7-832d-9188a53e76ab@linaro.org>
Date:   Fri, 7 Feb 2020 10:31:18 +0100
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Amit Kucheria <amit.kucheria@...durent.com>
Cc:     Vasily Khoruzhick <anarsoul@...il.com>,
        Yangtao Li <tiny.windzz@...il.com>,
        Zhang Rui <rui.zhang@...el.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Maxime Ripard <mripard@...nel.org>,
        Chen-Yu Tsai <wens@...e.org>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux PM list <linux-pm@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        lakml <linux-arm-kernel@...ts.infradead.org>,
        Ondřej Jirman <megous@...ous.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v8 0/7] add thermal sensor driver for A64, A83T, H3, H5,
 H6, R40

On 06/02/2020 20:23, Amit Kucheria wrote:
> On Thu, Feb 6, 2020 at 10:16 PM Daniel Lezcano
> <daniel.lezcano@...aro.org> wrote:
>>
>>
>> Hi Amit,
>>
>> On 06/02/2020 15:13, Amit Kucheria wrote:
>>> Hi Vasily,
>>>
>>> For this entire series, the DTS files don't contain any trip points.
>>> Did I miss some other series?
>>>
>>> At a minimum, you should add some "hot" or "critical" trip points
>>> since then don't require a cooling-map with throttling actions. If you
>>> have "passive" trip points, then you need to provide cooling-maps.
>>
>> Except I'm misunderstanding the bindings, a thermal zone must define
>> these required properties:
>>
>> - polling-delay
>> - polling-delay-passive
>> - thermal-sensors
>> - trips
>> - cooling-maps
> 
> Right, except for the cooling-maps. Those are exempted if there is the
> trip type is not passive. That is my understanding of the existing
> bindings.

The binding is ambiguous.

For me it states the cooling maps must be defined as it is a required
node of the thermal zone.

We may not have an active or passive cooling device for the thermal
zone, thus we can not comply with the dt binding and strictly speaking
we shouldn't add this thermal zone.

But the logic of having a 'hot' or a 'critical' trip point without a
cooling device is correct.

As we move this binding to a schema, we shall clarify the cooling-maps
is required if there are active or passive trip points otherwise it is
optional.


> Trip type critical triggers a shutdown and trip type hot only triggers
> a notification - see thermal_core.c:handle_critical_trips(). So we
> only need cooling maps for passive trip types.
> 
>>> Since this series has been merged, could you please follow up with a
>>> fixup series to add the trip points?
>>>
>>> Regards,
>>> Amit
>>> p.s. We should catch all this automatically, I'll send out yaml
>>> bindings for the thermal framework soon that should catch this stuff.
>>
>> +1
>>
>> There was a small discussion about converting the binding to a schema:
>>
>> https://www.spinics.net/lists/devicetree/msg332424.html
> 
> 
> Aah, I missed that. I started working on something last week that
> looks similar to your discussion. Pushed a WIP branch here[1], it
> looks like I had a similar idea on how to split the bindings. Hope to
> finish this up tomorrow for an RFC.

Great, thanks for taking care of that.


> [1] https://github.com/idlethread/linux/commits/up/thermal/yaml-conversion-v1
> 
>>> On Thu, Dec 19, 2019 at 10:58 PM Vasily Khoruzhick <anarsoul@...il.com> wrote:
>>>>
>>>> This patchset adds driver for thermal sensor in A64, A83T, H3, H5,
>>>> H6 and R40 SoCs.
>>>>
>>>> v8:
>>>>         - [vasily] Address more Maxime's comments for dt-schema
>>>>         - [vasily] Add myself to MAINTAINERS for the driver and schema
>>>>         - [vasily] Round calibration data size to word boundary for H6 and A64
>>>>         - [vasily] Change offset for A64 since it reports too low temp otherwise.
>>>>                    Likely conversion formula in user manual is not correct.
>>>>
>>>> v7:
>>>>         - [vasily] Address Maxime's comments for dt-schema
>>>>         - [vasily] Move common part of H3 and H5 dts into sunxi-h3-h5.dtsi
>>>>         - [vasily] Add Maxime's a-b to the driver patch
>>>>
>>>> v6:
>>>>         - [ondrej, vasily] Squash all driver related changes into a
>>>>                            single patch
>>>>         - [ondrej] Rename calib -> calibration
>>>>         - [ondrej] Fix thermal zone registration check
>>>>         - [ondrej] Lower rate of sensor data interrupts to 4/sec/sensor
>>>>         - [ondrej] Rework scale/offset values, H6 calibration
>>>>         - [ondrej] Explicitly set mod clock to 24 MHz
>>>>         - [ondrej] Set undocumented bits in CTRL0 for H6
>>>>         - [ondrej] Add support for A83T
>>>>         - [ondrej] Add dts changes for A83T, H3, H5, H6
>>>>         - [vasily] Add dts changes for A64
>>>>         - [vasily] Address Maxime's comments for YAML scheme
>>>>         - [vasily] Make .calc_temp callback mandatory
>>>>         - [vasily] Set .max_register in regmap config, so regs can be
>>>>                    inspected using debugfs
>>>>
>>>> Ondrej Jirman (4):
>>>>   ARM: dts: sun8i-a83t: Add thermal sensor and thermal zones
>>>>   ARM: dts: sun8i-h3: Add thermal sensor and thermal zones
>>>>   arm64: dts: allwinner: h5: Add thermal sensor and thermal zones
>>>>   arm64: dts: allwinner: h6: Add thermal sensor and thermal zones
>>>>
>>>> Vasily Khoruzhick (1):
>>>>   arm64: dts: allwinner: a64: Add thermal sensors and thermal zones
>>>>
>>>> Yangtao Li (2):
>>>>   thermal: sun8i: add thermal driver for H6/H5/H3/A64/A83T/R40
>>>>   dt-bindings: thermal: add YAML schema for sun8i-thermal driver
>>>>     bindings
>>>>
>>>>  .../thermal/allwinner,sun8i-a83t-ths.yaml     | 160 +++++
>>>>  MAINTAINERS                                   |   8 +
>>>>  arch/arm/boot/dts/sun8i-a83t.dtsi             |  36 +
>>>>  arch/arm/boot/dts/sun8i-h3.dtsi               |  20 +
>>>>  arch/arm/boot/dts/sunxi-h3-h5.dtsi            |   6 +
>>>>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi |  42 ++
>>>>  arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi  |  26 +
>>>>  arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi  |  33 +
>>>>  drivers/thermal/Kconfig                       |  14 +
>>>>  drivers/thermal/Makefile                      |   1 +
>>>>  drivers/thermal/sun8i_thermal.c               | 639 ++++++++++++++++++
>>>>  11 files changed, 985 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml
>>>>  create mode 100644 drivers/thermal/sun8i_thermal.c
>>>>
>>>> --
>>>> 2.24.1
>>>>
>>
>>
>> --
>>  <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
>>


-- 
 <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

Powered by blists - more mailing lists