Re: [PATCH v5 6/6] qcom/soc/drivers: Add DTPM description for sdm845

[Daniel Lezcano <daniel.lezcano@linaro.org>]

Hi Steev,

thanks for taking the time to test the series.

On 18/12/2021 20:47, Steev Klimaszewski wrote:
> Hi Daniel,
> 
> On 12/18/21 7:00 AM, Daniel Lezcano wrote:
>> The DTPM framework does support now the hierarchy description.
>>
>> The platform specific code can call the hierarchy creation function
>> with an array of struct dtpm_node pointing to their parents.
>>
>> This patch provides a description of the big and Little CPUs and the
>> GPU and tie them together under a virtual package name. Only sdm845 is
>> described.
>>
>> The description could be extended in the future with the memory
>> controller with devfreq if it has the energy information.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>   drivers/soc/qcom/Kconfig  |  9 ++++++
>>   drivers/soc/qcom/Makefile |  1 +
>>   drivers/soc/qcom/dtpm.c   | 65 +++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 75 insertions(+)
>>   create mode 100644 drivers/soc/qcom/dtpm.c
>>
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index e718b8735444..f21c1df2f2f9 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -228,4 +228,13 @@ config QCOM_APR
>>         application processor and QDSP6. APR is
>>         used by audio driver to configure QDSP6
>>         ASM, ADM and AFE modules.
>> +
>> +config QCOM_DTPM
>> +    tristate "Qualcomm DTPM hierarchy"
> 
> Testing this on a Lenovo Yoga C630 here and...
> 
> 
> Should this be tristate?  Is it actually possible to unload the module
> once it's loaded?
> 
> Here I have DTPM=y, DTPM_CPU=y, DTPM_DEVFREQ=y, QCOM_DTPM=m
> 
> But if I attempt to modprobe -r dtpm,
> 
> modprobe: ERROR: ../libkmod/libkmod-module.c:799
> kmod_module_remove_module() could not remove 'dtpm': Device or resource
> busy

Yes, the module is designed to be loaded only. I did not wanted to add
more complexity in the driver as unloading it is not the priority ATM.
We need this to be a module in order to load it after the other devices.

>> +    depends on DTPM
>> +    help
>> +     Describe the hierarchy for the Dynamic Thermal Power
>> +     Management tree on this platform. That will create all the
>> +     power capping capable devices.
>> +
>>   endmenu
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index 70d5de69fd7b..cf38496c3f61 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -28,3 +28,4 @@ obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
>>   obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
>>   obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
>>   obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) +=    kryo-l2-accessors.o
>> +obj-$(CONFIG_QCOM_DTPM) += dtpm.o

[ ... ]

>> +static struct of_device_id __initdata sdm845_dtpm_match_table[] = {
>> +        { .compatible = "qcom,sdm845", .data = sdm845_hierarchy },
>> +        {},
>> +};
>> +
>> +static int __init sdm845_dtpm_init(void)
>> +{
>> +    return dtpm_create_hierarchy(sdm845_dtpm_match_table);
>> +}
>> +late_initcall(sdm845_dtpm_init);
>> +
>> +MODULE_DESCRIPTION("Qualcomm DTPM driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:dtpm");
>> +MODULE_AUTHOR("Daniel Lezcano <daniel.lezcano@kernel.org");
>> +
> 
> It does seem to work aside from not being able to modprobe -r the
> module. Although I do see
> 
> [   35.849622] dtpm: Registered dtpm node 'sdm845' / 0-0 uW,
> [   35.849652] dtpm: Registered dtpm node 'package' / 0-0 uW,
> [   35.849676] dtpm: Registered dtpm node 'cpu0-cpufreq' / 40000-436000 uW,
> [   35.849702] dtpm: Registered dtpm node 'cpu4-cpufreq' /
> 520000-5828000 uW,
> [   35.849734] dtpm_devfreq: No energy model available for '5000000.gpu'
> [   35.849738] dtpm: Failed to setup '/soc@0/gpu@5000000': -22
> 
> If the devfreq issue with the gpu isn't expected, are we missing
> something for the c630?

Yes, the energy model is missing for the GPU, very likely the
'dynamic-power-coefficient' property is missing in the gpu section.

A quick test could be to add a value like 800. The resulting power
numbers will be wrong but it should be possible to act on the
performance by using these wrong power numbers.

  -- Daniel

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog