Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file

[Chanwoo Choi]

On 1/9/20 12:44 AM, Lukasz Luba wrote:
> Hi,
> 
> On 1/8/20 2:20 PM, Dmitry Osipenko wrote:
>> 08.01.2020 00:48, Bjorn Andersson ÐÐÑÐÑ:
>>> On Tue 07 Jan 01:05 PST 2020, Chanwoo Choi wrote:
>>>
>>>> Add new devfreq_transitions debugfs file to track the frequency transitions
>>>> of all devfreq devices for the simple profiling as following:
>>>> - /sys/kernel/debug/devfreq/devfreq_transitions
>>>>
>>>> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
>>>> in Kconfig in order to save the transition history.
>>>>
>>>> [Detailed description of each field of 'devfreq_transitions' debugfs file]
>>>> - time_msÂÂÂ : Change time of frequency transition. (unit: millisecond)
>>>> - dev_nameÂÂÂ : Device name of h/w.
>>>> - devÂÂÂÂÂÂÂ : Device name made by devfreq core.
>>>> - parent_devÂÂÂ : If devfreq device uses the passive governor,
>>>> ÂÂÂÂÂÂÂÂÂ show parent devfreq device name.
>>>> - load_%ÂÂÂ : If devfreq device uses the simple_ondemand governor,
>>>> ÂÂÂÂÂÂÂÂÂ load is used by governor whene deciding the new frequency.
>>>> ÂÂÂÂÂÂÂÂÂ (unit: percentage)
>>>> - old_freq_hzÂÂÂ : Frequency before changing. (unit: hz)
>>>> - new_freq_hzÂÂÂ : Frequency after changed. (unit: hz)
>>>>
>>>> [For example on Exynos5422-based Odroid-XU3 board]
>>>> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
>>>> time_ms dev_name dev parent_dev load_% old_freq_hz new_freq_hz
>>>> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
>>>> 14600ÂÂÂÂÂ soc:bus_nocÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ devfreq2ÂÂ devfreq1ÂÂ 0ÂÂÂÂÂ 100000000ÂÂÂ 67000000
>>>> 14600ÂÂÂÂÂ soc:bus_fsys_apbÂÂÂÂÂÂÂÂÂÂÂÂÂÂ devfreq3ÂÂ devfreq1ÂÂ 0ÂÂÂÂÂ 200000000ÂÂÂ 100000000
>>>> 14600ÂÂÂÂÂ soc:bus_fsysÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ devfreq4ÂÂ devfreq1ÂÂ 0ÂÂÂÂÂ 200000000ÂÂÂ 100000000
>>>> 14600ÂÂÂÂÂ soc:bus_fsys2ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ devfreq5ÂÂ devfreq1ÂÂ 0ÂÂÂÂÂ 150000000ÂÂÂ 75000000
>>>> 14602ÂÂÂÂÂ soc:bus_mfcÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ devfreq6ÂÂ devfreq1ÂÂ 0ÂÂÂÂÂ 222000000ÂÂÂ 96000000
>>>> 14602ÂÂÂÂÂ soc:bus_genÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ devfreq7ÂÂ devfreq1ÂÂ 0ÂÂÂÂÂ 267000000ÂÂÂ 89000000
>>>> 14602ÂÂÂÂÂ soc:bus_g2dÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ devfreq9ÂÂ devfreq1ÂÂ 0ÂÂÂÂÂ 300000000ÂÂÂ 84000000
>>>> 14602ÂÂÂÂÂ soc:bus_g2d_acpÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ devfreq10Â devfreq1ÂÂ 0ÂÂÂÂÂ 267000000ÂÂÂ 67000000
>>>> 14602ÂÂÂÂÂ soc:bus_jpegÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ devfreq11Â devfreq1ÂÂ 0ÂÂÂÂÂ 300000000ÂÂÂ 75000000
>>>> 14602ÂÂÂÂÂ soc:bus_jpeg_apbÂÂÂÂÂÂÂÂÂÂÂÂÂÂ devfreq12Â devfreq1ÂÂ 0ÂÂÂÂÂ 167000000ÂÂÂ 84000000
>>>> 14603ÂÂÂÂÂ soc:bus_disp1_fimdÂÂÂÂÂÂÂÂÂÂÂÂ devfreq13Â devfreq1ÂÂ 0ÂÂÂÂÂ 200000000ÂÂÂ 120000000
>>>> 14603ÂÂÂÂÂ soc:bus_disp1ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ devfreq14Â devfreq1ÂÂ 0ÂÂÂÂÂ 300000000ÂÂÂ 120000000
>>>> 14606ÂÂÂÂÂ soc:bus_gscl_scalerÂÂÂÂÂÂÂÂÂÂÂ devfreq15Â devfreq1ÂÂ 0ÂÂÂÂÂ 300000000ÂÂÂ 150000000
>>>> 14606ÂÂÂÂÂ soc:bus_msclÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ devfreq16Â devfreq1ÂÂ 0ÂÂÂÂÂ 333000000ÂÂÂ 84000000
>>>> 14608ÂÂÂÂÂ soc:bus_wcoreÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ devfreq1ÂÂÂÂÂÂÂÂÂÂÂÂÂ 9ÂÂÂÂÂ 333000000ÂÂÂ 84000000
>>>> 14783ÂÂÂÂÂ 10c20000.memory-controllerÂÂÂÂ devfreq0ÂÂÂÂÂÂÂÂÂÂÂÂÂ 35ÂÂÂÂ 825000000ÂÂÂ 633000000
>>>> 15873ÂÂÂÂÂ soc:bus_wcoreÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ devfreq1ÂÂÂÂÂÂÂÂÂÂÂÂÂ 41ÂÂÂÂ 84000000ÂÂÂÂ 400000000
>>>> 15873ÂÂÂÂÂ soc:bus_nocÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ devfreq2ÂÂ devfreq1ÂÂ 0ÂÂÂÂÂ 67000000ÂÂÂÂ 100000000
>>>> [snip]
>>>>
>>>
>>> Wouldn't it make more sense to expose this through the tracing
>>> framework - like many other subsystems does?
>>
>> I think devfreq core already has some tracing support and indeed it
>> should be better to extend it rather than duplicate.
>>
> 
> In my opinion this debugfs interface should be considered as a helpful
> validation entry point. We had some issues with wrong bootloader
> configurations in clock tree, where some frequencies could not be set
> in the kernel. Similar useful description can be find in clock subsystem
> where there is clock tree summary file.
> 
> It is much cheaper to poke a few files in debug dir by some automated
> test than starting tracing, provoking desired code flow in the
> devfreq for every device, paring the results... A simple boot test
> which reads only these new files can be enough to rise the flag.
> Secondly the tracing is not always compiled.
> 
> It could capture old/wrong bootloaders which pinned devices
> improperly to PLLs or wrong DT values in OPP table.
> (a workaround for Odroid xu4 patchset:
> https://protect2.fireeye.com/url?k=364d2fbb-6b9993d3-364ca4f4-0cc47a3356b2-98db7e7cf023414c&u=https://lkml.org/lkml/2019/7/15/276
> )
> 
> Chanwoo what do think about some sanity check summary?
> It could be presented in a 3rd file: 'devfreq_sanity', which
> could report if the devices could set their registered OPPs
> and got the same values, i.e. set 166MHz --> set to 150MHz
> in reality. If a config option i.e. DEVFREQ_SANITY is set
> then during the registration of a new device it checks OPPs
> if they are possible to set. It could be done before assigning
> the governor for the device and results present in of of your files.

Firstly, I'm welcoming to add new debugging feature.

As you suggested, it is required for the OPP control.
But, I'm not sure that it have to be verified in either OPP or devfreq.
Or it have be verified when adding the OPP table into devicetree file.

If we add this debugging feature, I think 'resource_sanity' as following path.
/sys/kernel/debug/devfreq/devfreqX/resource_sanity (not fixed name)
- show the sanity result of regulator voltage and frequency

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics