Hello Rui,
On Mon, Aug 20, 2012 at 5:16 AM, Zhang Rui wrote:
> On δΊ”, 2012-08-17 at 11:56 +0300, Valentin, Eduardo wrote:
>> Hello,
>
>> >>> +
>> >>> +
>> >>> +1.2 CPU cooling action notifier register/unregister interface
>> >>> +1.2.1 int cputherm_register_notifier(struct notifier_block *nb,
>> >>> + unsigned int list)
>> >>> +
>> >>> +This interface registers a driver with cpu cooling layer. The
>> >>> driver will
>> >>> +be notified when any cpu cooling action is called.
>> >>> +
>> >>> +nb: notifier function to register
>> >>> +list: CPUFREQ_COOLING_START or CPUFREQ_COOLING_STOP
>> >>> +
>> >>> +1.2.2 int cputherm_unregister_notifier(struct notifier_block *nb,
>> >>> + unsigned int list)
>> >>> +
>> >>> +This interface registers a driver with cpu cooling layer. The
>> >>> driver will
>> >>> +be notified when any cpu cooling action is called.
>> >>> +
>> >>> +nb: notifier function to register
>> >>> +list: CPUFREQ_COOLING_START or CPUFREQ_COOLING_STOP
>> >>
>> >> what are these two APIs used for?
>> >> I did not see they are used in your patch set, do I miss something?
>> > No currently they are not used by my patches. I added them on request
>> > from Eduardo and others
>>
>> Yeah, this was a suggestion as we didn't really know how the FW part
>> would evolve by that time.
>>
>> The reasoning is to allow any interested user (in kernel) to be
>> notified when max frequency changes.
>
> in this case, the cooling device should be updated.
> Say all the target of the thermal_instances of a cooling devices is 0,
> which means they want the cpu to run at maximum frequency, when the max
> frequency changes, we should set the processor to the new max frequency
> as well.
> Using notification is not a good idea as user can not handle this
> without interacting with the thermal framework.
>
> IMO, we should introduce a new API to handle this, rather than just
> notifications to users.
Agreed. The context is actually much wider than the CPU cooling. In
fact, cooling co-processors is actually where things gets a bit
complicated. The idea behind this type of handshaking is to allow the
affected subsystem to change their actual setup when max performance
is not allowed anymore.
>
>> Actually, the use case behind
>> this is to allow such users to perform some handshaking or stop their
>> activities or even change some paramenters, in case the max frequency
>> would change.
>
> It is the cooling device driver that notice this change first, and it
> should invoke something like thermal_cooling_device_update/rebind() to
> tell the thermal framework this change.
>
Yeah. Ideally, the framework needs to be aware of cooling device state
change. Today, as we have pretty much a broadcast/unidirectional type
of messaging, the framework/policy always assumes the cooling devices
will be in sync with what it is dictated by the policy.
>> Ideally it would be possible to nack the cooling
>> transition. But that is yet a wider discussion. So far we don't have
>> users for this.
>>
> yep.
> I thought about this before, but I'd prefer to do this when there is a
> real user. Or else, we are kind of over-designing here.
> how about removing this piece of code for now?
Agreed. I second you that this problem is a much wider issue and needs
improvement in the FW itself and how the cooling devices are designed.
>
> thanks,
> rui
>
>> >>
>> >>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> >>> index 7dd8c34..996003b 100644
>> >>> --- a/drivers/thermal/Kconfig
>> >>> +++ b/drivers/thermal/Kconfig
>> >>> @@ -19,6 +19,17 @@ config THERMAL_HWMON
>> >>> depends on HWMON=y || HWMON=THERMAL
>> >>> default y
>> >>>
>> >>> +config CPU_THERMAL
>> >>> + bool "generic cpu cooling support"
>> >>> + depends on THERMAL && CPU_FREQ
>> >>> + help
>> >>> + This implements the generic cpu cooling mechanism through
>> >>> frequency
>> >>> + reduction, cpu hotplug and any other ways of reducing
>> >>> temperature. An
>> >>> + ACPI version of this already
>> >>> exists(drivers/acpi/processor_thermal.c).
>> >>> + This will be useful for platforms using the generic thermal
>> >>> interface
>> >>> + and not the ACPI interface.
>> >>> + If you want this support, you should say Y here.
>> >>> +
>> >>> config SPEAR_THERMAL
>> >>> bool "SPEAr thermal sensor driver"
>> >>> depends on THERMAL
>> >>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>> >>> index fd9369a..aae59ad 100644
>> >>> --- a/drivers/thermal/Makefile
>> >>> +++ b/drivers/thermal/Makefile
>> >>> @@ -3,5 +3,6 @@
>> >>> #
>> >>>
>> >>> obj-$(CONFIG_THERMAL)+= thermal_sys.o
>> >>> +obj-$(CONFIG_CPU_THERMAL)+= cpu_cooling.o
>> >>> obj-$(CONFIG_SPEAR_THERMAL) += spear_thermal.o
>> >>> obj-$(CONFIG_RCAR_THERMAL) += rcar_thermal.o
>> >>> diff --git a/drivers/thermal/cpu_cooling.c
>> >>> b/drivers/thermal
