Re: [PATCH 1/3] thermal: hisilicon: add new hisilicon thermal sensor driver

[Xinwei Kong]

On 2015å03æ20æ 19:24, 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.

In order to co-ordinate with the thermal framework, We will fix "tsensor-thres-temp"
and "tsensor-reset-temp" value in the dts. This temperatue will higher than thermal
zone trip-point value. During polling delay time if SoC temperature is above thermal
zone trip-point value and below "tsensor-thres-temp" value,the systerm will use this
thermal framework ways to realize basic function such as cpu cooling device. Otherwise
it will use interrput mode to cause some functions.

This interrupt mode help thermal framework way to complete function, in high temperature
situation this interrupt is better than other mode. I think that it can work well.

Xinwei

> > 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.
>
> > > > +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?
>
> Mark.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/