From: leo.yan@linaro.org (Leo Yan)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 1/3] thermal: hisilicon: add new hisilicon thermal sensor driver
Date: Fri, 20 Mar 2015 22:53:53 +0800 [thread overview]
Message-ID: <20150320145353.GA20461@leoy-linaro> (raw)
In-Reply-To: <20150320112414.GD14766@leverpostej>
On Fri, Mar 20, 2015 at 11:24:14AM +0000, Mark Rutland wrote:
> > > > + ret = of_property_read_u32(np, "hisilicon,tsensor-thres-temp",
> > > > + &sensor->thres_temp);
> > > > + if (ret) {
> > > > + dev_err(&pdev->dev, "failed to get thres value %d: %d\n",
> > > > + index, ret);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + ret = of_property_read_u32(np, "hisilicon,tsensor-reset-temp",
> > > > + &sensor->reset_temp);
> > > > + if (ret) {
> > > > + dev_err(&pdev->dev, "failed to get reset value %d: %d\n",
> > > > + index, ret);
> > > > + return ret;
> > > > + }
> > >
> > > I see now that these properties result in the HW being programmed. You
> > > should figure out how to reconcile these with thermal-zone trip points
> > > rather than having parallel properties.
> >
> > Set "tsensor-thres-temp" to register so that if thermal reaches the
> > threshold, the sensor will trigger h/w interrupt.
> >
> > Set "tsensor-reset-temp" to register so that if thermal reaches the
> > reset value, the sensor will assert SoC reset signal to trigger h/w
> > reset.
>
> I understand this.
>
> > This is different w/t thermal-zone trip points, the trip points are
> > used for timer polling.
>
> That may be the case in the code as it stands today, but per the binding
> the trip points are the temperatures at which an action is to be taken.
>
> The thermal-zone has poilling-delay and polling-delay-passive, but
> there's no reason you couldn't also use the interrupt to handle the
> "hot" trip-point, adn the reset at the "critical" trip-point. All that's
> missing is the plumbing in order to do so.
>
> So please co-ordinate with the thermal framework to do that.
Let's dig further more for this point, so that we can get more specific
gudiance and have a good preparation for next version's patch set.
After i reviewed the thermal framework code, currently have one smooth
way to co-ordinate the trip points w/t thermal framework: use the function
*thermal_zone_device_register()* to register sensor, and can use the
callback function .get_trip_temp to tell thermal framework for the
trip points' temperature.
For hisi thermal case, now the driver is using the function
*thermal_zone_of_sensor_register* to register sensor, but use this way
i have not found there have standard APIs which can be used by sensor
driver to get the trip points info from thermal framework.
I may miss something for thermal framework, so if have existed APIs to
get the trip points, could pls point out?
> > Do u think below modification is more reasonable?
> >
> > - Set "tsensor-thres-temp" = 700000, which equal to thermal-zone
> > passive trip point, so that we can use h/w interrupt to update the
> > thermal value immediately, rather than using polling method w/t long
> > delay;
> >
> > - Set "tsensor-reset-temp" = <100000>, which is higher than
> > thermal-zone critical trip point, so that the s/w reset method has
> > higher priority than h/w reset; we also can easily know the reset is
> > caused by thermal framework; "tsensor-reset-temp" is only used to
> > protect h/w circuit.
>
> As mentioned above, I think that you should co-ordinate with the thermal
> framework. You're worknig around limitations inthe code as it stands
> today rather than solving the fundamental issue.
>
> > > > + if (of_property_read_bool(np, "hisilicon,tsensor-bind-irq")) {
> > > > +
> > > > + if (data->irq_bind_sensor != -1)
> > > > + dev_warn(&pdev->dev, "irq has bound to index %d\n",
> > > > + data->irq_bind_sensor);
> > > > +
> > > > + /* bind irq to this sensor */
> > > > + data->irq_bind_sensor = index;
> > > > + }
> > >
> > > I don't see why this should be specified in the DT. Why do you believe
> > > it should?
> >
> > The thermal sensor module has four sensors, but have only one
> > interrupt signal; This interrupt can only be used by one sensor;
> > So want to use dts to bind the interrupt with one selected sensor.
>
> That's not all that great, though I'm not exactly sure how the kernel
> would select the best sensor to measure with. It would be good if you
> could talk to the thermal maintainers w.r.t. this.
This will be decided by the silicon, right? Every soc has different
combination with cpu/gpu/vpu, so which part is hottest, this maybe
highly dependent on individual SoC.
S/W just need provide the flexibility so that later can choose
the interrupt to bind with the sensor within the hottest part.
> > > > +static int hisi_thermal_probe(struct platform_device *pdev)
> > > > +{
> > > > + struct hisi_thermal_data *data;
> > > > + struct resource *res;
> > > > + int i;
> > > > + int ret;
> > > > +
> > > > + if (!cpufreq_get_current_driver()) {
> > > > + dev_dbg(&pdev->dev, "no cpufreq driver!");
> > > > + return -EPROBE_DEFER;
> > > > + }
> > >
> > > Surely we care about not burning out the board even if we don't have
> > > cpufreq?
> > >
> > > Is there any ordering guarantee between the probing of this driver and
> > > cpufreq?
> >
> > Yes, here need binding the thermal sensor w/t cpu cooling device,
> > and cpu cooling device is based on cpufreq driver.
>
> Sure, but if you don't have a cooling device you still want the critical
> temperature reset and so on, no?
Okay, let's remove this dependency.
Thanks,
Leo Yan
next prev parent reply other threads:[~2015-03-20 14:53 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-19 7:57 [PATCH 0/3] 96boards: add thermal senor support to hikey board kongxinwei
2015-03-19 7:57 ` [PATCH 1/3] thermal: hisilicon: add new hisilicon thermal sensor driver kongxinwei
2015-03-19 14:17 ` Mark Rutland
2015-03-20 6:06 ` Leo Yan
2015-03-20 11:24 ` Mark Rutland
2015-03-20 14:53 ` Leo Yan [this message]
2015-03-20 15:55 ` Mark Rutland
2015-03-23 4:46 ` Leo Yan
2015-03-23 8:30 ` kongxinwei
2015-03-20 15:27 ` Xinwei Kong
2015-03-20 7:37 ` kongxinwei
2015-03-19 7:57 ` [PATCH 2/3] dts: hi6220: enable thermal sensor for hisilicon SoC kongxinwei
2015-03-19 7:57 ` [PATCH 3/3] dt-bindings: Document the hi6220 thermal sensor bindings kongxinwei
2015-03-19 14:08 ` Mark Rutland
2015-03-19 13:59 ` [PATCH 0/3] 96boards: add thermal senor support to hikey board Mark Rutland
2015-03-20 3:10 ` kongxinwei
2015-03-20 6:13 ` Leo Yan
2015-03-20 7:41 ` kongxinwei
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=20150320145353.GA20461@leoy-linaro \
--to=leo.yan@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).