From: Javi Merino <javi.merino@arm.com>
To: Leo Yan <leo.yan@linaro.org>
Cc: Eduardo Valentin <edubezval@gmail.com>,
Wei Xu <xuwei5@hisilicon.com>, Rob Herring <robh+dt@kernel.org>,
Pawel Moll <pawel.moll@arm.com>,
Mark Rutland <mark.rutland@arm.com>,
Ian Campbell <ijc+devicetree@hellion.org.uk>,
Kumar Gala <galak@codeaurora.org>,
Catalin Marinas <catalin.marinas@arm.com>,
Will Deacon <will.deacon@arm.com>,
Zhang Rui <rui.zhang@intel.com>,
kongxinwei <kong.kongxinwei@hisilicon.com>,
Punit Agrawal <Punit.Agrawal@arm.com>,
linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org
Subject: Re: [PATCH v2 1/5] thermal: change "hysteresis" as optional property
Date: Fri, 4 Mar 2016 11:57:53 +0000 [thread overview]
Message-ID: <20160304115752.GA13894@e104805> (raw)
In-Reply-To: <20160304030349.GA11012@leoy-linaro>
On Fri, Mar 04, 2016 at 11:03:49AM +0800, Leo Yan wrote:
> Hi Eduardo,
>
> On Thu, Mar 03, 2016 at 08:29:44AM -0800, Eduardo Valentin wrote:
> > Hi Leo,
> >
> > On Fri, Feb 26, 2016 at 11:43:43AM +0800, Leo Yan wrote:
> > > The property "hysteresis" is mandatory for trip points, so if without
> > > it the thermal zone cannot register successfully. But "hysteresis" is
> > > ignored in the thermal subsystem and only inquired by several thermal
> > > sensor drivers.
> >
> > I am not sure this a good direction to go. Remember that Linux
> > implementation not necessarily has to be the implication of the DT
> > binding. Hysteresis is a property that plays a significant role on
> > thermal control systems, which in many cases avoid overshooting cooling
> > actions. Having the DT writer to explicitly set it to 0 means that zone
> > does not suffer of overshooting and does not need hysteresis.
>
> After review current code, the "hysteresis" is used to calculate
> temperature falling threshold with a more conservative value; so that
> finally avoid overshooting issue.
>
> Please confirm if is my understanding correct or not?
>
> > If the Linux thermal subsystem has a problem with handling hysteresis, I
> > would rather fix Linux code than relaxing the DT binding. Or if you
> > still believe hysteresis is really optional, I would prefer to see a
> > better justification than "Linux ignores it".
I see it the other way round, Is hysteresis a property that, without
it, the thermal code can't configure itself so it fails to create the
trip point? The current code goes "There is no hysteresis for this
property, I don't know how to set up this trip point!". I think we
can do better than this.
> If we think about power allocator governor, PID's two parameters are
> also used to dismiss overshooting issue: one is k_po (proportional
> term), another is k_i (integral term). So that means after we apply
> power allocator governor, we don't need parameter "hysteresis" due PID
> algorithm can automatically dismiss potential errors.
I disagree. We shouldn't base DT decisions based on only one
governor in linux. Having said that, AFAICS all governors currently
ignore hysteresis.
Cheers,
Javi
WARNING: multiple messages have this Message-ID (diff)
From: javi.merino@arm.com (Javi Merino)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2 1/5] thermal: change "hysteresis" as optional property
Date: Fri, 4 Mar 2016 11:57:53 +0000 [thread overview]
Message-ID: <20160304115752.GA13894@e104805> (raw)
In-Reply-To: <20160304030349.GA11012@leoy-linaro>
On Fri, Mar 04, 2016 at 11:03:49AM +0800, Leo Yan wrote:
> Hi Eduardo,
>
> On Thu, Mar 03, 2016 at 08:29:44AM -0800, Eduardo Valentin wrote:
> > Hi Leo,
> >
> > On Fri, Feb 26, 2016 at 11:43:43AM +0800, Leo Yan wrote:
> > > The property "hysteresis" is mandatory for trip points, so if without
> > > it the thermal zone cannot register successfully. But "hysteresis" is
> > > ignored in the thermal subsystem and only inquired by several thermal
> > > sensor drivers.
> >
> > I am not sure this a good direction to go. Remember that Linux
> > implementation not necessarily has to be the implication of the DT
> > binding. Hysteresis is a property that plays a significant role on
> > thermal control systems, which in many cases avoid overshooting cooling
> > actions. Having the DT writer to explicitly set it to 0 means that zone
> > does not suffer of overshooting and does not need hysteresis.
>
> After review current code, the "hysteresis" is used to calculate
> temperature falling threshold with a more conservative value; so that
> finally avoid overshooting issue.
>
> Please confirm if is my understanding correct or not?
>
> > If the Linux thermal subsystem has a problem with handling hysteresis, I
> > would rather fix Linux code than relaxing the DT binding. Or if you
> > still believe hysteresis is really optional, I would prefer to see a
> > better justification than "Linux ignores it".
I see it the other way round, Is hysteresis a property that, without
it, the thermal code can't configure itself so it fails to create the
trip point? The current code goes "There is no hysteresis for this
property, I don't know how to set up this trip point!". I think we
can do better than this.
> If we think about power allocator governor, PID's two parameters are
> also used to dismiss overshooting issue: one is k_po (proportional
> term), another is k_i (integral term). So that means after we apply
> power allocator governor, we don't need parameter "hysteresis" due PID
> algorithm can automatically dismiss potential errors.
I disagree. We shouldn't base DT decisions based on only one
governor in linux. Having said that, AFAICS all governors currently
ignore hysteresis.
Cheers,
Javi
next prev parent reply other threads:[~2016-03-04 11:57 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-26 3:43 [PATCH v2 0/5] thermal: hisilicon: enable power allocator for Hi6220 Leo Yan
2016-02-26 3:43 ` Leo Yan
2016-02-26 3:43 ` Leo Yan
2016-02-26 3:43 ` [PATCH v2 1/5] thermal: change "hysteresis" as optional property Leo Yan
2016-02-26 3:43 ` Leo Yan
[not found] ` <1456458227-12950-2-git-send-email-leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2016-03-03 10:45 ` Javi Merino
2016-03-03 10:45 ` Javi Merino
2016-03-03 10:45 ` Javi Merino
2016-03-03 16:29 ` Eduardo Valentin
2016-03-03 16:29 ` Eduardo Valentin
2016-03-03 16:29 ` Eduardo Valentin
2016-03-04 3:03 ` Leo Yan
2016-03-04 3:03 ` Leo Yan
2016-03-04 11:57 ` Javi Merino [this message]
2016-03-04 11:57 ` Javi Merino
2016-03-08 13:57 ` Leo Yan
2016-03-08 13:57 ` Leo Yan
2016-03-08 20:55 ` Eduardo Valentin
2016-03-08 20:55 ` Eduardo Valentin
2016-03-09 11:10 ` Javi Merino
2016-03-09 11:10 ` Javi Merino
2016-03-20 15:40 ` Leo Yan
2016-03-20 15:40 ` Leo Yan
2016-02-26 3:43 ` [PATCH v2 2/5] thermal: hisilicon: support to use any sensor Leo Yan
2016-02-26 3:43 ` Leo Yan
2016-02-26 3:43 ` [PATCH v2 3/5] thermal: hisilicon: fix IRQ imbalance enabling Leo Yan
2016-02-26 3:43 ` Leo Yan
2016-02-26 3:43 ` [PATCH v2 4/5] arm64: dts: register Hi6220's thermal sensor Leo Yan
2016-02-26 3:43 ` Leo Yan
2016-02-26 3:43 ` [PATCH v2 5/5] arm64: dts: register Hi6220's thermal zone for power allocator Leo Yan
2016-02-26 3:43 ` Leo Yan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160304115752.GA13894@e104805 \
--to=javi.merino@arm.com \
--cc=Punit.Agrawal@arm.com \
--cc=catalin.marinas@arm.com \
--cc=devicetree@vger.kernel.org \
--cc=edubezval@gmail.com \
--cc=galak@codeaurora.org \
--cc=ijc+devicetree@hellion.org.uk \
--cc=kong.kongxinwei@hisilicon.com \
--cc=leo.yan@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=pawel.moll@arm.com \
--cc=robh+dt@kernel.org \
--cc=rui.zhang@intel.com \
--cc=will.deacon@arm.com \
--cc=xuwei5@hisilicon.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.