On Thu, Feb 26, 2015 at 05:30:00PM -0400, Eduardo Valentin wrote: > On Thu, Feb 26, 2015 at 07:00:33PM +0000, Javi Merino wrote: > > It's useful for tuning to be able to edit thermal_zone_parameters from > > userspace. Export them to the thermal_zone sysfs so that they can be > > easily changed. > > > > Cc: Zhang Rui <[email protected]> > > Cc: Eduardo Valentin <[email protected]> > > Signed-off-by: Javi Merino <[email protected]> > > --- > > Documentation/thermal/sysfs-api.txt | 52 +++++++++++++++++ > > drivers/thermal/thermal_core.c | 110 > > ++++++++++++++++++++++++++++++++++++ > > 2 files changed, 162 insertions(+) > > > > diff --git a/Documentation/thermal/sysfs-api.txt > > b/Documentation/thermal/sysfs-api.txt > > index fc7dfe10778b..7d44d7f1a71b 100644 > > --- a/Documentation/thermal/sysfs-api.txt > > +++ b/Documentation/thermal/sysfs-api.txt > > @@ -184,6 +184,12 @@ Thermal zone device sys I/F, created once it's > > registered: > > |---trip_point_[0-*]_type: Trip point type > > |---trip_point_[0-*]_hyst: Hysteresis value for this trip point > > |---emul_temp: Emulated temperature set node > > + |---sustainable_power: Sustainable dissipatable power > > + |---k_po: Proportional term during temperature > > overshoot > > + |---k_pu: Proportional term during temperature > > undershoot > > + |---k_i: PID's integral term in the power allocator > > gov > > + |---k_d: PID's derivative term in the power > > allocator > > + |---integral_cutoff: Offset above which errors are accumulated > > Can this be under a specific directory? > > I thought of something like > /sys/class/thermal/thermal_zoneX/governor_params/<gov spec files> > > The above node can be handled by the governor code. > > > > > > Thermal cooling device sys I/F, created once it's registered: > > /sys/class/thermal/cooling_device[0-*]: > > @@ -307,6 +313,52 @@ emul_temp > > because userland can easily disable the thermal policy by simply > > flooding this sysfs node with low temperature values. > > > > +sustainable_power > > + An estimate of the sustained power that can be dissipated by > > + the thermal zone. Used by the power allocator governor. For > > + more information see Documentation/thermal/power_allocator.txt > > + Unit: milliwatts > > + RW, Optional > > + > > +k_po > > + The proportional term of the power allocator governor's PID > > + controller during temperature overshoot. Temperature overshoot > > + is when the current temperature is above the "desired > > + temperature" trip point. For more information see > > + Documentation/thermal/power_allocator.txt > > + RW, Optional > > + > > +k_pu > > + The proportional term of the power allocator governor's PID > > + controller during temperature undershoot. Temperature undershoot > > + is when the current temperature is below the "desired > > + temperature" trip point. For more information see > > + Documentation/thermal/power_allocator.txt > > + RW, Optional > > + > > +k_i > > + The integral term of the power allocator governor's PID > > + controller. This term allows the PID controller to compensate > > + for long term drift. For more information see > > + Documentation/thermal/power_allocator.txt > > + RW, Optional > > + > > +k_d > > + The derivative term of the power allocator governor's PID > > + controller. For more information see > > + Documentation/thermal/power_allocator.txt > > + RW, Optional > > + > > +integral_cutoff > > + Temperature offset from the desired temperature trip point > > + above which the integral term of the power allocator > > + governor's PID controller starts accumulating errors. For > > + example, if integral_cutoff is 0, then the integral term only > > + accumulates error when temperature is above the desired > > + temperature trip point. For more information see > > + Documentation/thermal/power_allocator.txt > > + RW, Optional > > + > > ***************************** > > * Cooling device attributes * > > ***************************** > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > > index df9ba3bf55dc..228e93f8a146 100644 > > --- a/drivers/thermal/thermal_core.c > > +++ b/drivers/thermal/thermal_core.c > > @@ -873,6 +873,111 @@ emul_temp_store(struct device *dev, struct > > device_attribute *attr, > > static DEVICE_ATTR(emul_temp, S_IWUSR, NULL, emul_temp_store); > > #endif/*CONFIG_THERMAL_EMULATION*/ > > > > +#ifdef CONFIG_THERMAL_GOV_POWER_ALLOCATOR
What bugged me in the first place was the fact that we are doing this
#ifdef here. But in fact, it is not really necessary, as the parameters
are stored in tzp, and they will be there regardless of the config
status.
> > +
> > +static ssize_t
> > +sustainable_power_show(struct device *dev, struct device_attribute
> > *devattr,
> > + char *buf)
> > +{
> > + struct thermal_zone_device *tz = to_thermal_zone(dev);
> > +
> > + if (tz->tzp)
> > + return sprintf(buf, "%u\n", tz->tzp->sustainable_power);
> > + else
> > + return -EIO;
> > +}
> > +
> > +static ssize_t
> > +sustainable_power_store(struct device *dev, struct device_attribute
> > *devattr,
> > + const char *buf, size_t count)
> > +{
> > + struct thermal_zone_device *tz = to_thermal_zone(dev);
> > + u32 sustainable_power;
> > +
> > + if (!tz->tzp)
> > + return -EIO;
> > +
> > + if (kstrtou32(buf, 10, &sustainable_power))
> > + return -EINVAL;
> > +
> > + tz->tzp->sustainable_power = sustainable_power;
> > +
> > + return count;
> > +}
> > +static DEVICE_ATTR(sustainable_power, S_IWUSR | S_IRUGO,
> > sustainable_power_show,
> > + sustainable_power_store);
> > +
> > +#define create_s32_tzp_attr(name) \
> > + static ssize_t \
> > + name##_show(struct device *dev, struct device_attribute *devattr, \
> > + char *buf) \
> > + { \
> > + struct thermal_zone_device *tz = to_thermal_zone(dev); \
> > + \
> > + if (tz->tzp) \
> > + return sprintf(buf, "%u\n", tz->tzp->name); \
> > + else \
> > + return -EIO; \
> > + } \
> > + \
> > + static ssize_t \
> > + name##_store(struct device *dev, struct device_attribute *devattr, \
> > + const char *buf, size_t count) \
> > + { \
> > + struct thermal_zone_device *tz = to_thermal_zone(dev); \
> > + s32 value; \
> > + \
> > + if (!tz->tzp) \
> > + return -EIO; \
> > + \
> > + if (kstrtos32(buf, 10, &value)) \
> > + return -EINVAL; \
> > + \
> > + tz->tzp->name = value; \
> > + \
> > + return count; \
> > + } \
> > + static DEVICE_ATTR(name, S_IWUSR | S_IRUGO, name##_show, name##_store)
> > +
> > +create_s32_tzp_attr(k_po);
> > +create_s32_tzp_attr(k_pu);
> > +create_s32_tzp_attr(k_i);
> > +create_s32_tzp_attr(k_d);
> > +create_s32_tzp_attr(integral_cutoff);
> > +#undef create_s32_tzp_attr
> > +
> > +static struct device_attribute *dev_tzp_attrs[] = {
> > + &dev_attr_sustainable_power,
> > + &dev_attr_k_po,
> > + &dev_attr_k_pu,
> > + &dev_attr_k_i,
> > + &dev_attr_k_d,
> > + &dev_attr_integral_cutoff,
> > +};
> > +
> > +static int create_power_allocator_tzp_attrs(struct device *dev)
I would rename this to thermal_create_zone_params_attrs and remove the
ifdefiry. If you are not exposing the complete info under tzp, then make
a comment about it.
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(dev_tzp_attrs); i++) {
> > + int ret;
> > + struct device_attribute *dev_attr = dev_tzp_attrs[i];
> > +
> > + ret = device_create_file(dev, dev_attr);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +#else /* !CONFIG_THERMAL_GOV_POWER_ALLOCATOR */
> > +static int create_power_allocator_tzp_attrs(struct device *dev)
> > +{
> > + return 0;
> > +}
> > +#endif
> > +
>
> It is better if this code is not part of thermal_core. The governor
> specific code must be in the governor code. Durga has done a pretty good
> job splitting governor code out of thermal_core.c. I don't think we want
> to get them back.
Unless you really want to expose something that is only inside the
governor data struture, you can ignore the above comment.
>
>
> > /**
> > * power_actor_get_max_power() - get the maximum power that a cdev can
> > consume
> > * @cdev: pointer to &thermal_cooling_device
> > @@ -1712,6 +1817,11 @@ struct thermal_zone_device
> > *thermal_zone_device_register(const char *type,
> > if (result)
> > goto unregister;
> >
> > + /* Add power_allocator specific thermal zone params */
> > + result = create_power_allocator_tzp_attrs(&tz->device);
> > + if (result)
> > + goto unregister;
>
> Here you could create the governor_params and have a callback from
> governor to populate it.
ditto.
Of course, assuming all we are doing is exposing what is in
thermal_zone_params, which is generic enough to be in thermal_core.c.
>
> > +
> > /* Update 'this' zone's governor information */
> > mutex_lock(&thermal_governor_lock);
> >
> > --
> > 1.9.1
> >
signature.asc
Description: Digital signature
