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

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

[Daniel Lezcano]

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"
+	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
diff --git a/drivers/soc/qcom/dtpm.c b/drivers/soc/qcom/dtpm.c
new file mode 100644
index 000000000000..c15283f59494
--- /dev/null
+++ b/drivers/soc/qcom/dtpm.c
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2021 Linaro Limited
+ *
+ * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
+ *
+ * DTPM hierarchy description
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/dtpm.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+static struct dtpm_node __initdata sdm845_hierarchy[] = {
+	[0]{ .name = "sdm845" },
+	[1]{ .name = "package",
+	     .parent = &sdm845_hierarchy[0] },
+	[2]{ .name = "/cpus/cpu@0",
+	     .type = DTPM_NODE_DT,
+	     .parent = &sdm845_hierarchy[1] },
+	[3]{ .name = "/cpus/cpu@100",
+	     .type = DTPM_NODE_DT,
+	     .parent = &sdm845_hierarchy[1] },
+	[4]{ .name = "/cpus/cpu@200",
+	     .type = DTPM_NODE_DT,
+	     .parent = &sdm845_hierarchy[1] },
+	[5]{ .name = "/cpus/cpu@300",
+	     .type = DTPM_NODE_DT,
+	     .parent = &sdm845_hierarchy[1] },
+	[6]{ .name = "/cpus/cpu@400",
+	     .type = DTPM_NODE_DT,
+	     .parent = &sdm845_hierarchy[1] },
+	[7]{ .name = "/cpus/cpu@500",
+	     .type = DTPM_NODE_DT,
+	     .parent = &sdm845_hierarchy[1] },
+	[8]{ .name = "/cpus/cpu@600",
+	     .type = DTPM_NODE_DT,
+	     .parent = &sdm845_hierarchy[1] },
+	[9]{ .name = "/cpus/cpu@700",
+	     .type = DTPM_NODE_DT,
+	     .parent = &sdm845_hierarchy[1] },
+	[10]{ .name = "/soc@0/gpu@5000000",
+	     .type = DTPM_NODE_DT,
+	     .parent = &sdm845_hierarchy[1] },
+	[11]{ },
+};
+
+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");
+
-- 
2.25.1

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

[Steev Klimaszewski]

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



> +	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
> diff --git a/drivers/soc/qcom/dtpm.c b/drivers/soc/qcom/dtpm.c
> new file mode 100644
> index 000000000000..c15283f59494
> --- /dev/null
> +++ b/drivers/soc/qcom/dtpm.c
> @@ -0,0 +1,65 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2021 Linaro Limited
> + *
> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
> + *
> + * DTPM hierarchy description
> + */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/dtpm.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +static struct dtpm_node __initdata sdm845_hierarchy[] = {
> +	[0]{ .name = "sdm845" },
> +	[1]{ .name = "package",
> +	     .parent = &sdm845_hierarchy[0] },
> +	[2]{ .name = "/cpus/cpu@0",
> +	     .type = DTPM_NODE_DT,
> +	     .parent = &sdm845_hierarchy[1] },
> +	[3]{ .name = "/cpus/cpu@100",
> +	     .type = DTPM_NODE_DT,
> +	     .parent = &sdm845_hierarchy[1] },
> +	[4]{ .name = "/cpus/cpu@200",
> +	     .type = DTPM_NODE_DT,
> +	     .parent = &sdm845_hierarchy[1] },
> +	[5]{ .name = "/cpus/cpu@300",
> +	     .type = DTPM_NODE_DT,
> +	     .parent = &sdm845_hierarchy[1] },
> +	[6]{ .name = "/cpus/cpu@400",
> +	     .type = DTPM_NODE_DT,
> +	     .parent = &sdm845_hierarchy[1] },
> +	[7]{ .name = "/cpus/cpu@500",
> +	     .type = DTPM_NODE_DT,
> +	     .parent = &sdm845_hierarchy[1] },
> +	[8]{ .name = "/cpus/cpu@600",
> +	     .type = DTPM_NODE_DT,
> +	     .parent = &sdm845_hierarchy[1] },
> +	[9]{ .name = "/cpus/cpu@700",
> +	     .type = DTPM_NODE_DT,
> +	     .parent = &sdm845_hierarchy[1] },
> +	[10]{ .name = "/soc@0/gpu@5000000",
> +	     .type = DTPM_NODE_DT,
> +	     .parent = &sdm845_hierarchy[1] },
> +	[11]{ },
> +};
> +
> +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?

-- Steev

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

[Daniel Lezcano]

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

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

[Steev Klimaszewski]

Hi Daniel,

On 12/18/21 2:11 PM, Daniel Lezcano wrote:
> Hi Steev,
>
> thanks for taking the time to test the series.

My C630 is my daily driver and main computer, so I don't mind testing 
things to improve its usage at all.


> <snip>
> 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.
Makes sense, I just wasn't entirely sure if it was on purpose or not.
>>> +    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
> [ ... ]
I noticed this as well, and was going to ask if it shouldn't be named 
qcom_dtpm, but I don't think it matters since it would be in 
/lib/modules/$kver/kernel/drivers/soc/qcom ?
>>> +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
>
So, I'm definitely not the greatest of kernel hackers, just enough 
knowledge to be dangerous and I know how to apply patches properly.... 
I'm not able to actually get this working.  I've tried adding it with a 
few different numbers, and any time i try to add the d-p-c, I get

Dec 18 15:00:49 limitless kernel: [   57.394503] adreno 5000000.gpu: EM: 
invalid perf. state: -22
Dec 18 15:00:49 limitless kernel: [   57.394515] dtpm_devfreq: No energy 
model available for '5000000.gpu'
Dec 18 15:00:49 limitless kernel: [   57.394519] dtpm: Failed to setup 
'/soc@0/gpu@5000000': -22

-- steev

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

[Daniel Lezcano]

On 19/12/2021 19:44, Steev Klimaszewski wrote:
> Hi Daniel,
> 
> On 12/18/21 2:11 PM, Daniel Lezcano wrote:
>> Hi Steev,
>>
>> thanks for taking the time to test the series.
> 
> My C630 is my daily driver and main computer, so I don't mind testing
> things to improve its usage at all.
> 
> 
>> <snip>
>> 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.
> Makes sense, I just wasn't entirely sure if it was on purpose or not.
>>>> +    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
>> [ ... ]
> I noticed this as well, and was going to ask if it shouldn't be named
> qcom_dtpm, but I don't think it matters since it would be in
> /lib/modules/$kver/kernel/drivers/soc/qcom ?

Right

>>>> +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
>>
> So, I'm definitely not the greatest of kernel hackers, just enough
> knowledge to be dangerous and I know how to apply patches properly....
> I'm not able to actually get this working.  I've tried adding it with a
> few different numbers, and any time i try to add the d-p-c, I get
> 
> Dec 18 15:00:49 limitless kernel: [   57.394503] adreno 5000000.gpu: EM:
> invalid perf. state: -22
> Dec 18 15:00:49 limitless kernel: [   57.394515] dtpm_devfreq: No energy
> model available for '5000000.gpu'
> Dec 18 15:00:49 limitless kernel: [   57.394519] dtpm: Failed to setup
> '/soc@0/gpu@5000000': -22

I've been through the code and I suspect something is missing in the
mainline kernel for devfreq vs energy model. Not related to DTPM actually.

I'll double check if that could be added beside this series


-- 
<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

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

[Bjorn Andersson]

On Sat 18 Dec 05:00 PST 2021, 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"
> +	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
> diff --git a/drivers/soc/qcom/dtpm.c b/drivers/soc/qcom/dtpm.c
> new file mode 100644
> index 000000000000..c15283f59494
> --- /dev/null
> +++ b/drivers/soc/qcom/dtpm.c
> @@ -0,0 +1,65 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2021 Linaro Limited
> + *
> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
> + *
> + * DTPM hierarchy description
> + */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/dtpm.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +static struct dtpm_node __initdata sdm845_hierarchy[] = {
> +	[0]{ .name = "sdm845" },

Why is the index signifiant here?
Doesn't this imply risk that we forget one element, which will be
thereby implicitly be left initialized as {} and hence denote
termination of the list?

> +	[1]{ .name = "package",
> +	     .parent = &sdm845_hierarchy[0] },
> +	[2]{ .name = "/cpus/cpu@0",
> +	     .type = DTPM_NODE_DT,
> +	     .parent = &sdm845_hierarchy[1] },
> +	[3]{ .name = "/cpus/cpu@100",
> +	     .type = DTPM_NODE_DT,
> +	     .parent = &sdm845_hierarchy[1] },
> +	[4]{ .name = "/cpus/cpu@200",
> +	     .type = DTPM_NODE_DT,
> +	     .parent = &sdm845_hierarchy[1] },
> +	[5]{ .name = "/cpus/cpu@300",
> +	     .type = DTPM_NODE_DT,
> +	     .parent = &sdm845_hierarchy[1] },
> +	[6]{ .name = "/cpus/cpu@400",
> +	     .type = DTPM_NODE_DT,
> +	     .parent = &sdm845_hierarchy[1] },
> +	[7]{ .name = "/cpus/cpu@500",
> +	     .type = DTPM_NODE_DT,
> +	     .parent = &sdm845_hierarchy[1] },
> +	[8]{ .name = "/cpus/cpu@600",
> +	     .type = DTPM_NODE_DT,
> +	     .parent = &sdm845_hierarchy[1] },
> +	[9]{ .name = "/cpus/cpu@700",
> +	     .type = DTPM_NODE_DT,
> +	     .parent = &sdm845_hierarchy[1] },
> +	[10]{ .name = "/soc@0/gpu@5000000",

It worries me that we encode the textual structure of the dts in the
kernel. E.g. for quite a while this was "/soc/gpu@5000000", so if this
landed a year ago this driver would have prevented us from correcting
the dts.

Another concern is that not all busses in the system are capable of
36-bit wide addresses, so it's plausible that we might one day have to
create a more accurate representation of the address space. Maybe not on
SDM845, but this would force us to be inconsistent.

Regards,
Bjorn

> +	     .type = DTPM_NODE_DT,
> +	     .parent = &sdm845_hierarchy[1] },
> +	[11]{ },
> +};
> +
> +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");
> +
> -- 
> 2.25.1
> 

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

[Daniel Lezcano]

Hi Bjorn,

On 07/01/2022 20:27, Bjorn Andersson wrote:

[ ... ]

>> +#include <linux/dtpm.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +
>> +static struct dtpm_node __initdata sdm845_hierarchy[] = {
>> +	[0]{ .name = "sdm845" },
> 
> Why is the index signifiant here?
> Doesn't this imply risk that we forget one element, which will be
> thereby implicitly be left initialized as {} and hence denote
> termination of the list?

Yes, that is possible. The other annotation is also possible. The index
helps to refer from the .parent field.

That said nothing forces to use the index, so it is a matter of taste.

>> +	[1]{ .name = "package",
>> +	     .parent = &sdm845_hierarchy[0] },
>> +	[2]{ .name = "/cpus/cpu@0",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[3]{ .name = "/cpus/cpu@100",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[4]{ .name = "/cpus/cpu@200",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[5]{ .name = "/cpus/cpu@300",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[6]{ .name = "/cpus/cpu@400",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[7]{ .name = "/cpus/cpu@500",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[8]{ .name = "/cpus/cpu@600",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[9]{ .name = "/cpus/cpu@700",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[10]{ .name = "/soc@0/gpu@5000000",
> 
> It worries me that we encode the textual structure of the dts in the
> kernel. E.g. for quite a while this was "/soc/gpu@5000000", so if this
> landed a year ago this driver would have prevented us from correcting
> the dts.

Why ? The change should be reflected in the driver also, no ?

> Another concern is that not all busses in the system are capable of
> 36-bit wide addresses, so it's plausible that we might one day have to
> create a more accurate representation of the address space. Maybe not on
> SDM845, but this would force us to be inconsistent.

Sorry, I'm missing the point :/

If a change is done in the DT, the code using the description must be
changed accordingly, no?


> Regards,
> Bjorn
> 
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[11]{ },
>> +};
>> +
>> +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");
>> +
>> -- 
>> 2.25.1
>>


-- 
<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

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

[Bjorn Andersson]

On Fri 07 Jan 14:07 PST 2022, Daniel Lezcano wrote:

> 
> Hi Bjorn,
> 
> On 07/01/2022 20:27, Bjorn Andersson wrote:
> 
> [ ... ]
> 
> >> +#include <linux/dtpm.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/platform_device.h>
> >> +
> >> +static struct dtpm_node __initdata sdm845_hierarchy[] = {
> >> +	[0]{ .name = "sdm845" },
> > 
> > Why is the index signifiant here?
> > Doesn't this imply risk that we forget one element, which will be
> > thereby implicitly be left initialized as {} and hence denote
> > termination of the list?
> 
> Yes, that is possible. The other annotation is also possible. The index
> helps to refer from the .parent field.
> 
> That said nothing forces to use the index, so it is a matter of taste.
> 
> >> +	[1]{ .name = "package",
> >> +	     .parent = &sdm845_hierarchy[0] },
> >> +	[2]{ .name = "/cpus/cpu@0",
> >> +	     .type = DTPM_NODE_DT,
> >> +	     .parent = &sdm845_hierarchy[1] },
> >> +	[3]{ .name = "/cpus/cpu@100",
> >> +	     .type = DTPM_NODE_DT,
> >> +	     .parent = &sdm845_hierarchy[1] },
> >> +	[4]{ .name = "/cpus/cpu@200",
> >> +	     .type = DTPM_NODE_DT,
> >> +	     .parent = &sdm845_hierarchy[1] },
> >> +	[5]{ .name = "/cpus/cpu@300",
> >> +	     .type = DTPM_NODE_DT,
> >> +	     .parent = &sdm845_hierarchy[1] },
> >> +	[6]{ .name = "/cpus/cpu@400",
> >> +	     .type = DTPM_NODE_DT,
> >> +	     .parent = &sdm845_hierarchy[1] },
> >> +	[7]{ .name = "/cpus/cpu@500",
> >> +	     .type = DTPM_NODE_DT,
> >> +	     .parent = &sdm845_hierarchy[1] },
> >> +	[8]{ .name = "/cpus/cpu@600",
> >> +	     .type = DTPM_NODE_DT,
> >> +	     .parent = &sdm845_hierarchy[1] },
> >> +	[9]{ .name = "/cpus/cpu@700",
> >> +	     .type = DTPM_NODE_DT,
> >> +	     .parent = &sdm845_hierarchy[1] },
> >> +	[10]{ .name = "/soc@0/gpu@5000000",
> > 
> > It worries me that we encode the textual structure of the dts in the
> > kernel. E.g. for quite a while this was "/soc/gpu@5000000", so if this
> > landed a year ago this driver would have prevented us from correcting
> > the dts.
> 
> Why ? The change should be reflected in the driver also, no ?
> 

There was no update needed to change /soc to /soc@0, but with this
driver in place we would need to do that.

The problem is that the life cycle of the DTB is different from Linux
and we promise our users that the kernel will be backwards compatible
with existing DTBs (at least for a reasonable amount of time).

So if we made a change in the kernel to turn the incorrect
"/soc/gpu@5000000" into "/soc@0/gpu@5000000" we would no longer find a
match if you try to boot with yesterday's DTB.

> > Another concern is that not all busses in the system are capable of
> > 36-bit wide addresses, so it's plausible that we might one day have to
> > create a more accurate representation of the address space. Maybe not on
> > SDM845, but this would force us to be inconsistent.
> 
> Sorry, I'm missing the point :/
> 
> If a change is done in the DT, the code using the description must be
> changed accordingly, no?
> 

No, the kernel should continue to function with the old DTB.

Consider when your Linux distro gives you a new kernel version on your
computer, that shouldn't require an upgrade of "BIOS" in order to boot
the new kernel.

Regards,
Bjorn

> 
> > Regards,
> > Bjorn
> > 
> >> +	     .type = DTPM_NODE_DT,
> >> +	     .parent = &sdm845_hierarchy[1] },
> >> +	[11]{ },
> >> +};
> >> +
> >> +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");
> >> +
> >> -- 
> >> 2.25.1
> >>
> 
> 
> -- 
> <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