Re: [PATCH V2 0/6] thermal: exynos: Add kernel thermal support for exynos platform

[Zhang Rui <rui.zhang@intel.com>]

On 三, 2012-04-11 at 18:17 +0530, Amit Kachhap wrote:
> Hi Rui,
> 
> Thanks for looking into the patches.
> 
> On 10 April 2012 06:28, Zhang Rui <rui.zhang@intel.com> wrote:
> > Hi, Amit,
> >
> > On 三, 2012-04-04 at 10:02 +0530, Amit Kachhap wrote:
> >> Hi Len/Rui,
> >>
> >> Any comment or feedback from your side about the status of this patch?
> >> Is it merge-able or major re-work is needed? I have fixed most of the
> >> comments in this patchset and currently working on some of the minor
> >> comments received and will submit them shortly.
> >>
> > Sorry for the late response.
> >
> > First of all, it makes sense to me to introduce the generic cpufrq
> > cooling implementation.
> ok thanks
> > But I still have some questions.
> > I think the key reason why THERMAL_TRIP_STATE_INSTANCE is introduced is
> > that the MONIROR_ZONE and WARN_ZONE on exynos4 can not fit into the
> > current passive handling in the generic thermal layer well, right?
> > e.g. there is no tc1/tc2 on exynos4.
> >
> > If yes, is it possible that we can enhance the passive cooling to
> > support the generic processor cooling?
> > say, introduce another way to throttle the processor in
> > thermal_zone_device_passive when tc1 and tc2 are not available?
> 
> I agree that this new trip type code can be moved into passive trip
> type when tc1 and tc2 are 0. but this is special type of cooling
> devices behaviour where only instances of the same cooling device is
> binded to a trip point. The order of mapping is the only
> differentiating criteria and there are some checks used to implement
> this like
> 1) The trip points should be in ascending order.(This is missing in my
> original patch, so I added below)
> 2) The set_cur_state has to be called for the exact temp range so
> get_cur_state(&state) and set_cur_state(state ++/state--) logic is not
> possible.
> 3) set_cur_state is called as set_cur_state(cdev_instance).

Do you really need two THERMAL_TRIP_STATE_INSTANCE trip points?

I'm not sure if my understanding is right, but IMO, we can have one
THERMAL_TRIP_STATE_INSTANCE only, for both MONIROR_ZONE and WARN_ZONE,
and the trip temperature equals MONIROR_ZONE.
The cpufreq cooling device will work in the same way, no?

thanks,
rui

> There is a chance that people might confuse that these checks are
> applicable for passive trip types also which is not the case here.
> 
> @@ -1187,6 +1228,21 @@ struct thermal_zone_device
> *thermal_zone_device_register(char *type,
>                 tz->ops->get_trip_type(tz, count, &trip_type);
>                 if (trip_type == THERMAL_TRIP_PASSIVE)
>                         passive = 1;
> +               /*
> +                * For THERMAL_TRIP_STATE_INSTANCE trips, thermal zone should
> +                * be in ascending order.
> +               */
> +               if (trip_type == THERMAL_TRIP_STATE_INSTANCE) {
> +                       tz->ops->get_trip_temp(tz, count, &trip_temp);
> +                       if (first_trip_temp == 0)
> +                               first_trip_temp = trip_temp;
> +                       else if (first_trip_temp < trip_temp)
> +                               first_trip_temp = trip_temp;
> +                       else if (first_trip_temp > trip_temp) {
> +                               pr_warn("Zone trip points should be in
> ascending order\n");
> +                               goto unregister;
> +                       }
> +               }
>         }
> 
>         if (!passive)
> 
> Anyway there is other alternative where this new trip type is not
> needed and I can just use the existing trip type THERMAL_TRIP_ACTIVE
> and create 2 separate cooling devices for MONITOR_ZONE and WARN_ZONE.
> I had thought to make this generic and just to manage with 1 cooling
> device.
> What is your view?
> 
> Thanks,
> Amit Daniel
> 
> 
> >
> > thanks,
> > rui
> >
> >> Regards,
> >> Amit Daniel
> >>
> >> On 19 March 2012 11:47, Amit Daniel Kachhap <amit.kachhap@linaro.org> wrote:
> >> > Changes since V1:
> >> > *Moved the sensor driver to driver/thermal folder from driver/hwmon folder
> >> >  as suggested by Mark Brown and Guenter Roeck
> >> > *Added notifier support to notify the registered drivers of any cpu cooling
> >> >  action. The driver can modify the default cooling behaviour(eg set different
> >> >  max clip frequency).
> >> > *The percentage based frequency replaced with absolute clipped frequency.
> >> > *Some more conditional checks when setting max frequency.
> >> > *Renamed the new trip type THERMAL_TRIP_STATE_ACTIVE to
> >> >  THERMAL_TRIP_STATE_INSTANCE
> >> > *Many review comments from R, Durgadoss <durgadoss.r@intel.com> and
> >> >  eduardo.valentin@ti.com implemented.
> >> > *Removed cooling stats through debugfs patch
> >> > *The V1 based can be found here,
> >> >  https://lkml.org/lkml/2012/2/22/123
> >> >  http://lkml.org/lkml/2012/3/3/32
> >> >
> >> > Changes since RFC:
> >> > *Changed the cpu cooling registration/unregistration API's to instance based
> >> > *Changed the STATE_ACTIVE trip type to pass correct instance id
> >> > *Adding support to restore back the policy->max_freq after doing frequency
> >> >  clipping.
> >> > *Moved the trip cooling stats from sysfs node to debugfs node as suggested
> >> >  by Greg KH greg@kroah.com
> >> > *Incorporated several review comments from eduardo.valentin@ti.com
> >> > *Moved the Temperature sensor driver from driver/hwmon/ to driver/mfd
> >> >  as discussed with Guenter Roeck <guenter.roeck@ericsson.com> and
> >> >  Donggeun Kim <dg77.kim@samsung.com> (https://lkml.org/lkml/2012/1/5/7)
> >> > *Some changes according to the changes in common cpu cooling APIs
> >> > *The RFC based patches can be found here,
> >> >  https://lkml.org/lkml/2011/12/13/186
> >> >  https://lkml.org/lkml/2011/12/21/169
> >> >
> >> >
> >> > Brief Description:
> >> >
> >> > 1) The generic cooling devices code is placed inside driver/thermal/* as
> >> > placing inside acpi folder will need un-necessary enabling of acpi code. This
> >> > codes is architecture independent.
> >> >
> >> > 2) This patchset adds a new trip type THERMAL_TRIP_STATE_INSTANCE which passes
> >> > cooling device instance number and may be helpful for cpufreq cooling devices
> >> > to take the correct cooling action. This trip type avoids the temperature
> >> > comparision check again inside the cooling handler.
> >> >
> >> > 3) This patchset adds generic cpu cooling low level implementation through
> >> > frequency clipping and cpu hotplug. In future, other cpu related cooling
> >> > devices may be added here. An ACPI version of this already exists
> >> > (drivers/acpi/processor_thermal.c). But this will be useful for platforms
> >> > like ARM using the generic thermal interface along with the generic cpu
> >> > cooling devices. The cooling device registration API's return cooling device
> >> > pointers which can be easily binded with the thermal zone trip points.
> >> > The important APIs exposed are,
> >> >   a)struct thermal_cooling_device *cpufreq_cooling_register(
> >> >        struct freq_clip_table *tab_ptr, unsigned int tab_size,
> >> >        const struct cpumask *mask_val)
> >> >   b)void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
> >> >
> >> > 4) Samsung exynos platform thermal implementation is done using the generic
> >> > cpu cooling APIs and the new trip type. The temperature sensor driver present
> >> > in the hwmon folder(registered as hwmon driver) is moved to thermal folder
> >> > and registered as a thermal driver.
> >> >
> >> > All this patchset is based on Kernel version 3.3-rc7
> >> >
> >> > A simple data/control flow diagrams is shown below,
> >> >
> >> > Core Linux thermal <----->  Exynos thermal interface <----- Temperature Sensor
> >> >          |                             |
> >> >         \|/                            |
> >> >  Cpufreq cooling device <---------------
> >> >
> >> >
> >> > Amit Daniel Kachhap (6):
> >> >  thermal: Add a new trip type to use cooling device instance number
> >> >  thermal: Add generic cpufreq cooling implementation
> >> >  thermal: Add generic cpuhotplug cooling implementation
> >> >  hwmon: exynos4: Move thermal sensor driver to driver/thermal
> >> >    directory
> >> >  thermal: exynos4: Register the tmu sensor with the kernel thermal
> >> >    layer
> >> >  ARM: exynos4: Add thermal sensor driver platform device support
> >> >
> >> >  Documentation/hwmon/exynos4_tmu           |   81 ---
> >> >  Documentation/thermal/cpu-cooling-api.txt |   76 +++
> >> >  Documentation/thermal/exynos4_tmu         |   52 ++
> >> >  Documentation/thermal/sysfs-api.txt       |    4 +-
> >> >  arch/arm/mach-exynos/Kconfig              |   11 +
> >> >  arch/arm/mach-exynos/Makefile             |    1 +
> >> >  arch/arm/mach-exynos/clock.c              |    4 +
> >> >  arch/arm/mach-exynos/dev-tmu.c            |   39 ++
> >> >  arch/arm/mach-exynos/include/mach/irqs.h  |    2 +
> >> >  arch/arm/mach-exynos/include/mach/map.h   |    1 +
> >> >  arch/arm/mach-exynos/mach-origen.c        |    1 +
> >> >  arch/arm/plat-samsung/include/plat/devs.h |    1 +
> >> >  drivers/hwmon/Kconfig                     |   10 -
> >> >  drivers/hwmon/Makefile                    |    1 -
> >> >  drivers/hwmon/exynos4_tmu.c               |  514 -------------------
> >> >  drivers/thermal/Kconfig                   |   21 +
> >> >  drivers/thermal/Makefile                  |    2 +
> >> >  drivers/thermal/cpu_cooling.c             |  529 +++++++++++++++++++
> >> >  drivers/thermal/exynos4_thermal.c         |  790 +++++++++++++++++++++++++++++
> >> >  drivers/thermal/thermal_sys.c             |   45 ++-
> >> >  include/linux/cpu_cooling.h               |   78 +++
> >> >  include/linux/platform_data/exynos4_tmu.h |    7 +
> >> >  include/linux/thermal.h                   |    1 +
> >> >  23 files changed, 1660 insertions(+), 611 deletions(-)
> >> >  delete mode 100644 Documentation/hwmon/exynos4_tmu
> >> >  create mode 100644 Documentation/thermal/cpu-cooling-api.txt
> >> >  create mode 100644 Documentation/thermal/exynos4_tmu
> >> >  create mode 100644 arch/arm/mach-exynos/dev-tmu.c
> >> >  delete mode 100644 drivers/hwmon/exynos4_tmu.c
> >> >  create mode 100644 drivers/thermal/cpu_cooling.c
> >> >  create mode 100644 drivers/thermal/exynos4_thermal.c
> >> >  create mode 100644 include/linux/cpu_cooling.h
> >> >
> >
> >


_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-pm