Messages in this thread | | | Subject | Re: [PATCH V4 4/4] thermal/drivers/cpu_cooling: Rename to cpufreq_cooling | From | Martin Kepplinger <> | Date | Tue, 10 Dec 2019 09:57:33 +0100 |
| |
On 09.12.19 20:29, Daniel Lezcano wrote: > On 09/12/2019 13:03, Daniel Lezcano wrote: >> On 09/12/2019 10:54, Martin Kepplinger wrote: >>> >>> >>> On 06.12.19 15:15, Daniel Lezcano wrote: >>>> On 06/12/2019 12:33, Martin Kepplinger wrote: >>>>> I tested this on the librem5-devkit and see the >>>>> cooling devices in sysfs. I configure ARM_PSCI_CPUIDLE, not ARM_CPUIDLE and >>>>> add the patch below in register the cooling device there. "psci_idle" >>>>> is listed as the cpuidle_driver. >>>>> >>>>> That's what I'm running, in case you want to see it all: >>>>> https://source.puri.sm/martin.kepplinger/linux-next/commits/next-20191205/librem5_cpuidle_mainline_atf >>>>> >>>>> so I add a trip temperature description like this: >>>>> https://source.puri.sm/martin.kepplinger/linux-next/commit/361f49f93ae2c477fd012790831cabd0ed976660 >>>>> >>>>> When I let the SoC heat up, cpuidle cooling won't kick it. In sysfs: >>>>> >>>>> catting the relevant files in /sys/class/thermal after heating up, >>>>> if that makes sense: >>>>> >>>>> 87000 >>>>> 85000 >>>>> 85000 >>>>> thermal-cpufreq-0 >>>>> 1 >>>>> thermal-idle-0 >>>>> 0 >>>>> thermal-idle-1 >>>>> 0 >>>>> thermal-idle-2 >>>>> 0 >>>>> thermal-idle-3 >>>>> 0 >>>>> >>>>> with ARM_CPUIDLE instead of ARM_PSCI_CPUIDLE (and registering the cooling dev >>>>> during cpuidle-arm.c init) I won't have a cpuidle driver and thus no cpu-sleep >>>>> state at all. >>>>> >>>>> Can you see where the problem here lies? >>>> >>>> Yes, I removed the registration via the DT. >>>> >>>> Can you try the following: >>>> >>>> diff --git a/drivers/cpuidle/dt_idle_states.c >>>> b/drivers/cpuidle/dt_idle_states.c >>>> index d06d21a9525d..01367ddec49a 100644 >>>> --- a/drivers/cpuidle/dt_idle_states.c >>>> +++ b/drivers/cpuidle/dt_idle_states.c >>>> @@ -13,6 +13,7 @@ >>>> #include <linux/errno.h> >>>> #include <linux/kernel.h> >>>> #include <linux/module.h> >>>> +#include <linux/cpu_cooling.h> >>>> #include <linux/of.h> >>>> #include <linux/of_device.h> >>>> >>>> @@ -205,6 +206,9 @@ int dt_init_idle_driver(struct cpuidle_driver *drv, >>>> err = -EINVAL; >>>> break; >>>> } >>>> + >>>> + cpuidle_of_cooling_register(state_node, drv); >>>> + >>>> of_node_put(state_node); >>>> } >>>> >>>> That's a hack for the moment. >>>> >>> >>> thanks. I could test that successfully. The only question would be: Is >>> is intentional how "non-aggressive" the cooling driver cools? I would >>> have expected it to basically inject more idle cycles earlier. I'd set >>> 75 degrees as trip point and at 85 degress is would only inject about 30 >>> (of 100). > > By the way, how many CPUs are injecting idle cycle when the mitigation > happens ?
all 4 are injecting the same.
> >>> You describe the "config values" in question in the documentation, but >>> I'm not sure what's the correct way to change them. >> >> That is difficult to say without knowing the board behavior. Are you >> able to profile the temperature with the load? How fast the temperature >> increases? The aggressive behavior of the cooling device will depend on >> the governor which depends on the slope of the temperature increase and >> the sampling. >> >> Can you give the pointer to the git tree with the DT definition of your >> board?
https://source.puri.sm/martin.kepplinger/linux-next/blob/next-20191205/librem5_cpuidle_mainline_atf/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts
you can browse in that branch.
>> >> You can try by changing the idle duration to 10ms instead of the default >> 4ms.
where is that set?
>> >> You can also change the cooling states in the DT <&state 20 70>, so it >> will begin to mitigate at state 20. But I wouldn't recommend that.
where would we assign that? I'm not sure who reads that -.- it's still something to consider, but a longer idle duration makes more sense, yes.
>> >> Do you have the energy power model, so we can try with the IPA governor? >> >>
thanks for the reminder. I'd look at that later.
martin
| |