[PATCH v5 0/6] powercap/drivers/dtpm: Create the dtpm hierarchy

[PATCH v5 0/6] powercap/drivers/dtpm: Create the dtpm hierarchy

[Daniel Lezcano]

The DTPM hierarchy is the base to build on top of it a power budget
allocator. It reflects the power consumption of the group of devices
and allows to cap their power.

The core code is there but there is no way to describe the desired
hierarchy yet.

A first proposal introduced the description through configfs [1] but
was rejected [2].

A second proposal based on the device tree with a binding similar to
the power domains [3] was proposed but finally rejected [4].

This fifth version delegates the hierarchy creation to the SoC with a
specific and self-encapsulated code using an array to describe the tree. The
SoC DTPM driver defines an array of nodes pointing to their parents.  The
hierarchy description can integrate a DT node and in the future a SCMI node,
that means the description can mix different type of nodes.

In addition to the hierarchy creation, the devfreq dtpm support is also
integrated into this series.

This series was tested on a rock960 (revision B - rk3399 based) and a
db845c (Qualcomm sdm845 based).

[1] https://lore.kernel.org/all/20210401183654.27214-1-daniel.lezcano@linaro.org/
[2] https://lore.kernel.org/all/YGYg6ZeZ1181%2FpXk@kroah.com/
[3] https://lore.kernel.org/all/20211205231558.779698-1-daniel.lezcano@linaro.org/
[4] https://lore.kernel.org/all/YbfFapsmsjs4qnsg@robh.at.kernel.org/

Changelog:
   V5:
   - Remove DT bindings
   - Added description with an array
   - Added simple description for rk3399 and sdm845
   - Moved dtpm table to the data section
   
   V4:
   - Added missing powerzone-cells
   - Changed powerzone name to comply with the pattern property

   V3:
   - Remove GPU section as no power is available (yet)
   - Remove '#powerzone-cells' conforming to the bindings change
   - Removed required property 'compatible'
   - Removed powerzone-cells from the topmost node
   - Removed powerzone-cells from cpus 'consumers' in example
   - Set additionnal property to false

   V2:
   - Added pattern properties and stick to powerzone-*
   - Added required property compatible and powerzone-cells
   - Added additionnal property
   - Added compatible
   - Renamed to 'powerzones'
   - Added missing powerzone-cells to the topmost node
   - Fixed errors reported by 'make DT_CHECKER_FLAGS=-m dt_binding_check'
   - Move description in the SoC dtsi specific file
   - Fixed missing prototype warning reported by lkp@

   V1: Initial post

Daniel Lezcano (6):
  powercap/drivers/dtpm: Move dtpm table from init to data section
  powercap/drivers/dtpm: Add hierarchy creation
  powercap/drivers/dtpm: Add CPU DT initialization support
  powercap/drivers/dtpm: Add dtpm devfreq with energy model support
  rockchip/soc/drivers: Add DTPM description for rk3399
  qcom/soc/drivers: Add DTPM description for sdm845

 drivers/powercap/Kconfig          |   8 ++
 drivers/powercap/Makefile         |   1 +
 drivers/powercap/dtpm.c           | 155 ++++++++++++++++++++++-
 drivers/powercap/dtpm_cpu.c       |  37 ++++--
 drivers/powercap/dtpm_devfreq.c   | 201 ++++++++++++++++++++++++++++++
 drivers/soc/qcom/Kconfig          |   9 ++
 drivers/soc/qcom/Makefile         |   1 +
 drivers/soc/qcom/dtpm.c           |  65 ++++++++++
 drivers/soc/rockchip/Kconfig      |   8 ++
 drivers/soc/rockchip/Makefile     |   1 +
 drivers/soc/rockchip/dtpm.c       |  56 +++++++++
 include/asm-generic/vmlinux.lds.h |   4 +-
 include/linux/dtpm.h              |  21 +++-
 13 files changed, 551 insertions(+), 16 deletions(-)
 create mode 100644 drivers/powercap/dtpm_devfreq.c
 create mode 100644 drivers/soc/qcom/dtpm.c
 create mode 100644 drivers/soc/rockchip/dtpm.c

-- 
2.25.1

[PATCH v5 1/6] powercap/drivers/dtpm: Move dtpm table from init to data section

[Daniel Lezcano]

The dtpm table is used to let the different dtpm backends to register
their setup callbacks in a single place and preventing to export
multiple functions all around the kernel. That allows the dtpm code to
be self-encapsulated.

The dtpm hierarchy will be passed as a parameter by a platform
specific code and that will lead to the creation of the different dtpm
nodes.

The function creating the hierarchy could be called from a module at
init time or when it is loaded. However, at this moment the table is
already freed as it belongs to the init section and the creation will
lead to a invalid memory access.

Fix this by moving the table to the data section.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 include/asm-generic/vmlinux.lds.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 42f3866bca69..50d494d94d6c 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -362,7 +362,8 @@
 	BRANCH_PROFILE()						\
 	TRACE_PRINTKS()							\
 	BPF_RAW_TP()							\
-	TRACEPOINT_STR()
+	TRACEPOINT_STR()						\
+	DTPM_TABLE()
 
 /*
  * Data section helpers
@@ -723,7 +724,6 @@
 	ACPI_PROBE_TABLE(irqchip)					\
 	ACPI_PROBE_TABLE(timer)						\
 	THERMAL_TABLE(governor)						\
-	DTPM_TABLE()							\
 	EARLYCON_TABLE()						\
 	LSM_TABLE()							\
 	EARLY_LSM_TABLE()						\
-- 
2.25.1

Re: [PATCH v5 1/6] powercap/drivers/dtpm: Move dtpm table from init to data section

[Ulf Hansson]

On Sat, 18 Dec 2021 at 14:00, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> The dtpm table is used to let the different dtpm backends to register
> their setup callbacks in a single place and preventing to export
> multiple functions all around the kernel. That allows the dtpm code to
> be self-encapsulated.

Well, that's not entirely true. The dtpm code and its backends (or
ops, whatever we call them) are already maintained from a single
place, the /drivers/powercap/* directory. I assume we intend to keep
it like this going forward too, right?

That is also what patch4 with the devfreq backend continues to conform to.

>
> The dtpm hierarchy will be passed as a parameter by a platform
> specific code and that will lead to the creation of the different dtpm
> nodes.
>
> The function creating the hierarchy could be called from a module at
> init time or when it is loaded. However, at this moment the table is
> already freed as it belongs to the init section and the creation will
> lead to a invalid memory access.
>
> Fix this by moving the table to the data section.

With the above said, I find it a bit odd to put a table in the data
section like this. Especially, since the only remaining argument for
why, is to avoid exporting functions, which isn't needed anyway.

I mean, it would be silly if we should continue to put subsystem
specific tables in here, to just let them contain a set of subsystem
specific callbacks.

>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Kind regards
Uffe

> ---
>  include/asm-generic/vmlinux.lds.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index 42f3866bca69..50d494d94d6c 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -362,7 +362,8 @@
>         BRANCH_PROFILE()                                                \
>         TRACE_PRINTKS()                                                 \
>         BPF_RAW_TP()                                                    \
> -       TRACEPOINT_STR()
> +       TRACEPOINT_STR()                                                \
> +       DTPM_TABLE()
>
>  /*
>   * Data section helpers
> @@ -723,7 +724,6 @@
>         ACPI_PROBE_TABLE(irqchip)                                       \
>         ACPI_PROBE_TABLE(timer)                                         \
>         THERMAL_TABLE(governor)                                         \
> -       DTPM_TABLE()                                                    \
>         EARLYCON_TABLE()                                                \
>         LSM_TABLE()                                                     \
>         EARLY_LSM_TABLE()                                               \
> --
> 2.25.1
>

Re: [PATCH v5 1/6] powercap/drivers/dtpm: Move dtpm table from init to data section

[Daniel Lezcano]

Hi Ulf,

thanks for your comments

On 31/12/2021 14:33, Ulf Hansson wrote:
> On Sat, 18 Dec 2021 at 14:00, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> The dtpm table is used to let the different dtpm backends to register
>> their setup callbacks in a single place and preventing to export
>> multiple functions all around the kernel. That allows the dtpm code to
>> be self-encapsulated.
> 
> Well, that's not entirely true. The dtpm code and its backends (or
> ops, whatever we call them) are already maintained from a single
> place, the /drivers/powercap/* directory. I assume we intend to keep
> it like this going forward too, right?

Hopefully we can add more devices like the battery or the backlight, but
I'm not sure they will be in drivers/powercap.

> That is also what patch4 with the devfreq backend continues to conform to.
> 
>>
>> The dtpm hierarchy will be passed as a parameter by a platform
>> specific code and that will lead to the creation of the different dtpm
>> nodes.
>>
>> The function creating the hierarchy could be called from a module at
>> init time or when it is loaded. However, at this moment the table is
>> already freed as it belongs to the init section and the creation will
>> lead to a invalid memory access.
>>
>> Fix this by moving the table to the data section.
> 
> With the above said, I find it a bit odd to put a table in the data
> section like this. Especially, since the only remaining argument for
> why, is to avoid exporting functions, which isn't needed anyway.
> 
> I mean, it would be silly if we should continue to put subsystem
> specific tables in here, to just let them contain a set of subsystem
> specific callbacks.

Do you have an alternative without introducing cyclic dependencies ?



>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> Kind regards
> Uffe
> 
>> ---
>>  include/asm-generic/vmlinux.lds.h | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
>> index 42f3866bca69..50d494d94d6c 100644
>> --- a/include/asm-generic/vmlinux.lds.h
>> +++ b/include/asm-generic/vmlinux.lds.h
>> @@ -362,7 +362,8 @@
>>         BRANCH_PROFILE()                                                \
>>         TRACE_PRINTKS()                                                 \
>>         BPF_RAW_TP()                                                    \
>> -       TRACEPOINT_STR()
>> +       TRACEPOINT_STR()                                                \
>> +       DTPM_TABLE()
>>
>>  /*
>>   * Data section helpers
>> @@ -723,7 +724,6 @@
>>         ACPI_PROBE_TABLE(irqchip)                                       \
>>         ACPI_PROBE_TABLE(timer)                                         \
>>         THERMAL_TABLE(governor)                                         \
>> -       DTPM_TABLE()                                                    \
>>         EARLYCON_TABLE()                                                \
>>         LSM_TABLE()                                                     \
>>         EARLY_LSM_TABLE()                                               \
>> --
>> 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 1/6] powercap/drivers/dtpm: Move dtpm table from init to data section

[Daniel Lezcano]

On 31/12/2021 14:33, Ulf Hansson wrote:
> On Sat, 18 Dec 2021 at 14:00, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> The dtpm table is used to let the different dtpm backends to register
>> their setup callbacks in a single place and preventing to export
>> multiple functions all around the kernel. That allows the dtpm code to
>> be self-encapsulated.
> 
> Well, that's not entirely true. The dtpm code and its backends (or
> ops, whatever we call them) are already maintained from a single
> place, the /drivers/powercap/* directory. I assume we intend to keep
> it like this going forward too, right?
> 
> That is also what patch4 with the devfreq backend continues to conform to.
> 
>>
>> The dtpm hierarchy will be passed as a parameter by a platform
>> specific code and that will lead to the creation of the different dtpm
>> nodes.
>>
>> The function creating the hierarchy could be called from a module at
>> init time or when it is loaded. However, at this moment the table is
>> already freed as it belongs to the init section and the creation will
>> lead to a invalid memory access.
>>
>> Fix this by moving the table to the data section.
> 
> With the above said, I find it a bit odd to put a table in the data
> section like this. Especially, since the only remaining argument for
> why, is to avoid exporting functions, which isn't needed anyway.
> 
> I mean, it would be silly if we should continue to put subsystem
> specific tables in here, to just let them contain a set of subsystem
> specific callbacks.

So I tried to change the approach and right now I was not able to find
an alternative keeping the code self-encapsulate and without introducing
cyclic dependencies.

I suggest to keep the patch as it is and double check if it makes sense
to change it after adding more dtpm backends

Alternatively I can copy the table to a dynamically allocated table.


>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> Kind regards
> Uffe
> 
>> ---
>>  include/asm-generic/vmlinux.lds.h | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
>> index 42f3866bca69..50d494d94d6c 100644
>> --- a/include/asm-generic/vmlinux.lds.h
>> +++ b/include/asm-generic/vmlinux.lds.h
>> @@ -362,7 +362,8 @@
>>         BRANCH_PROFILE()                                                \
>>         TRACE_PRINTKS()                                                 \
>>         BPF_RAW_TP()                                                    \
>> -       TRACEPOINT_STR()
>> +       TRACEPOINT_STR()                                                \
>> +       DTPM_TABLE()
>>
>>  /*
>>   * Data section helpers
>> @@ -723,7 +724,6 @@
>>         ACPI_PROBE_TABLE(irqchip)                                       \
>>         ACPI_PROBE_TABLE(timer)                                         \
>>         THERMAL_TABLE(governor)                                         \
>> -       DTPM_TABLE()                                                    \
>>         EARLYCON_TABLE()                                                \
>>         LSM_TABLE()                                                     \
>>         EARLY_LSM_TABLE()                                               \
>> --
>> 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 1/6] powercap/drivers/dtpm: Move dtpm table from init to data section

[Ulf Hansson]

On Fri, 7 Jan 2022 at 14:15, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 31/12/2021 14:33, Ulf Hansson wrote:
> > On Sat, 18 Dec 2021 at 14:00, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >>
> >> The dtpm table is used to let the different dtpm backends to register
> >> their setup callbacks in a single place and preventing to export
> >> multiple functions all around the kernel. That allows the dtpm code to
> >> be self-encapsulated.
> >
> > Well, that's not entirely true. The dtpm code and its backends (or
> > ops, whatever we call them) are already maintained from a single
> > place, the /drivers/powercap/* directory. I assume we intend to keep
> > it like this going forward too, right?
> >
> > That is also what patch4 with the devfreq backend continues to conform to.
> >
> >>
> >> The dtpm hierarchy will be passed as a parameter by a platform
> >> specific code and that will lead to the creation of the different dtpm
> >> nodes.
> >>
> >> The function creating the hierarchy could be called from a module at
> >> init time or when it is loaded. However, at this moment the table is
> >> already freed as it belongs to the init section and the creation will
> >> lead to a invalid memory access.
> >>
> >> Fix this by moving the table to the data section.
> >
> > With the above said, I find it a bit odd to put a table in the data
> > section like this. Especially, since the only remaining argument for
> > why, is to avoid exporting functions, which isn't needed anyway.
> >
> > I mean, it would be silly if we should continue to put subsystem
> > specific tables in here, to just let them contain a set of subsystem
> > specific callbacks.
>
> So I tried to change the approach and right now I was not able to find
> an alternative keeping the code self-encapsulate and without introducing
> cyclic dependencies.
>
> I suggest to keep the patch as it is and double check if it makes sense
> to change it after adding more dtpm backends
>
> Alternatively I can copy the table to a dynamically allocated table.

I am not sure I understand the problem. You don't need a "table of
callbacks" at all, at least to start with.

Instead, what you need is to make a call to a function, or actually
one call per supported dtpm type from dtpm_setup_dt() (introduced in
patch2).

For CPUs, you would simply call dtpm_cpu_setup() (introduced in
patch3) from dtpm_setup_dt(), rather than walking the dtpm table an
invoking the ->setup() callback.

Did that make sense to you?

Going forward, when we decide to introduce the option to add/remove
support for dtpm types dynamically, you can then convert to a
dynamically allocated table.

[...]

Kind regards
Uffe

Re: [PATCH v5 1/6] powercap/drivers/dtpm: Move dtpm table from init to data section

[Daniel Lezcano]

On 07/01/2022 15:49, Ulf Hansson wrote:
> On Fri, 7 Jan 2022 at 14:15, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> On 31/12/2021 14:33, Ulf Hansson wrote:
>>> On Sat, 18 Dec 2021 at 14:00, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> The dtpm table is used to let the different dtpm backends to register
>>>> their setup callbacks in a single place and preventing to export
>>>> multiple functions all around the kernel. That allows the dtpm code to
>>>> be self-encapsulated.
>>>
>>> Well, that's not entirely true. The dtpm code and its backends (or
>>> ops, whatever we call them) are already maintained from a single
>>> place, the /drivers/powercap/* directory. I assume we intend to keep
>>> it like this going forward too, right?
>>>
>>> That is also what patch4 with the devfreq backend continues to conform to.
>>>
>>>>
>>>> The dtpm hierarchy will be passed as a parameter by a platform
>>>> specific code and that will lead to the creation of the different dtpm
>>>> nodes.
>>>>
>>>> The function creating the hierarchy could be called from a module at
>>>> init time or when it is loaded. However, at this moment the table is
>>>> already freed as it belongs to the init section and the creation will
>>>> lead to a invalid memory access.
>>>>
>>>> Fix this by moving the table to the data section.
>>>
>>> With the above said, I find it a bit odd to put a table in the data
>>> section like this. Especially, since the only remaining argument for
>>> why, is to avoid exporting functions, which isn't needed anyway.
>>>
>>> I mean, it would be silly if we should continue to put subsystem
>>> specific tables in here, to just let them contain a set of subsystem
>>> specific callbacks.
>>
>> So I tried to change the approach and right now I was not able to find
>> an alternative keeping the code self-encapsulate and without introducing
>> cyclic dependencies.
>>
>> I suggest to keep the patch as it is and double check if it makes sense
>> to change it after adding more dtpm backends
>>
>> Alternatively I can copy the table to a dynamically allocated table.
> 
> I am not sure I understand the problem. You don't need a "table of
> callbacks" at all, at least to start with.
> 
> Instead, what you need is to make a call to a function, or actually
> one call per supported dtpm type from dtpm_setup_dt() (introduced in
> patch2).
> 
> For CPUs, you would simply call dtpm_cpu_setup() (introduced in
> patch3) from dtpm_setup_dt(), rather than walking the dtpm table an
> invoking the ->setup() callback.
>
> Did that make sense to you?

Yeah, I already got the point ;)

I'll convert it to something else, and we will see in the future if that
needs to be converted back to the table.


> Going forward, when we decide to introduce the option to add/remove
> support for dtpm types dynamically, you can then convert to a
> dynamically allocated table.
> 
> [...]
> 
> Kind regards
> Uffe
> 


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

[PATCH v5 2/6] powercap/drivers/dtpm: Add hierarchy creation

[Daniel Lezcano]

The DTPM framework is available but without a way to configure it.

This change provides a way to create a hierarchy of DTPM node where
the power consumption reflects the sum of the children's power
consumption.

It is up to the platform to specify an array of dtpm nodes where each
element has a pointer to its parent, except the top most one. The type
of the node gives the indication of which initialization callback to
call. At this time, we can create a virtual node, where its purpose is
to be a parent in the hierarchy, and a DT node where the name
describes its path.

In order to ensure a nice self-encapsulation, the DTPM table
descriptors contains a couple of initialization functions, one to
setup the DTPM backend and one to initialize it up. With this
approach, the DTPM framework has a very few material to export.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/powercap/Kconfig    |   1 +
 drivers/powercap/dtpm.c     | 155 ++++++++++++++++++++++++++++++++++--
 drivers/powercap/dtpm_cpu.c |   2 +-
 include/linux/dtpm.h        |  21 ++++-
 4 files changed, 171 insertions(+), 8 deletions(-)

diff --git a/drivers/powercap/Kconfig b/drivers/powercap/Kconfig
index 8242e8c5ed77..b1ca339957e3 100644
--- a/drivers/powercap/Kconfig
+++ b/drivers/powercap/Kconfig
@@ -46,6 +46,7 @@ config IDLE_INJECT
 
 config DTPM
 	bool "Power capping for Dynamic Thermal Power Management (EXPERIMENTAL)"
+	depends on OF
 	help
 	  This enables support for the power capping for the dynamic
 	  thermal power management userspace engine.
diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
index 0fe70687c198..1611c86de5f5 100644
--- a/drivers/powercap/dtpm.c
+++ b/drivers/powercap/dtpm.c
@@ -23,6 +23,7 @@
 #include <linux/powercap.h>
 #include <linux/slab.h>
 #include <linux/mutex.h>
+#include <linux/of.h>
 
 #define DTPM_POWER_LIMIT_FLAG 0
 
@@ -461,19 +462,163 @@ int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent)
 	return 0;
 }
 
-static int __init init_dtpm(void)
+static struct dtpm *dtpm_setup_virtual(const struct dtpm_node *hierarchy,
+				       struct dtpm *parent)
+{
+	struct dtpm *dtpm;
+	int ret;
+
+	dtpm = kzalloc(sizeof(*dtpm), GFP_KERNEL);
+	if (!dtpm)
+		return ERR_PTR(-ENOMEM);
+	dtpm_init(dtpm, NULL);
+
+	ret = dtpm_register(hierarchy->name, dtpm, parent);
+	if (ret) {
+		pr_err("Failed to register dtpm node '%s': %d\n",
+		       hierarchy->name, ret);
+		kfree(dtpm);
+		return ERR_PTR(ret);
+	}
+
+	return dtpm;
+}
+
+static struct dtpm *dtpm_setup_dt(const struct dtpm_node *hierarchy,
+				  struct dtpm *parent)
+{
+	struct dtpm_descr *dtpm_descr;
+	struct device_node *np;
+	int ret;
+
+	np = of_find_node_by_path(hierarchy->name);
+	if (!np) {
+		pr_err("Failed to find '%s'\n", hierarchy->name);
+		return ERR_PTR(-ENXIO);
+	}
+
+	for_each_dtpm_table(dtpm_descr) {
+
+		ret = dtpm_descr->setup(parent, np);
+		if (ret) {
+			pr_err("Failed to setup '%s': %d\n", hierarchy->name, ret);
+			of_node_put(np);
+			return ERR_PTR(ret);
+		}
+
+		of_node_put(np);
+	}
+
+	/*
+	 * By returning a NULL pointer, we let know the caller there
+	 * is no child for us as we are a leaf of the tree
+	 */
+	return NULL;
+}
+
+typedef struct dtpm * (*dtpm_node_callback_t)(const struct dtpm_node *, struct dtpm *);
+
+dtpm_node_callback_t dtpm_node_callback[] = {
+	[DTPM_NODE_VIRTUAL] = dtpm_setup_virtual,
+	[DTPM_NODE_DT] = dtpm_setup_dt,
+};
+
+static int dtpm_for_each_child(const struct dtpm_node *hierarchy,
+			       const struct dtpm_node *it, struct dtpm *parent)
+{
+	struct dtpm *dtpm;
+	int i, ret;
+
+	for (i = 0; hierarchy[i].name; i++) {
+
+		if (hierarchy[i].parent != it)
+			continue;
+
+		dtpm = dtpm_node_callback[hierarchy[i].type](&hierarchy[i], parent);
+		if (!dtpm || IS_ERR(dtpm))
+			continue;
+
+		ret = dtpm_for_each_child(hierarchy, &hierarchy[i], dtpm);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * dtpm_create_hierarchy - Create the dtpm hierarchy
+ * @hierarchy: An array of struct dtpm_node describing the hierarchy
+ *
+ * The function is called by the platform specific code with the
+ * description of the different node in the hierarchy. It creates the
+ * tree in the sysfs filesystem under the powercap dtpm entry.
+ *
+ * The expected tree has the format:
+ *
+ * struct dtpm_node hierarchy[] = {
+ *	[0] { .name = "topmost" },
+ *      [1] { .name = "package", .parent = &hierarchy[0] },
+ *      [2] { .name = "/cpus/cpu0", .type = DTPM_NODE_DT, .parent = &hierarchy[1] },
+ *      [3] { .name = "/cpus/cpu1", .type = DTPM_NODE_DT, .parent = &hierarchy[1] },
+ *      [4] { .name = "/cpus/cpu2", .type = DTPM_NODE_DT, .parent = &hierarchy[1] },
+ *      [5] { .name = "/cpus/cpu3", .type = DTPM_NODE_DT, .parent = &hierarchy[1] },
+ *	[6] { }
+ * };
+ *
+ * The last element is always an empty one and marks the end of the
+ * array.
+ *
+ * Return: zero on success, a negative value in case of error. Errors
+ * are reported back from the underlying functions.
+ */
+int dtpm_create_hierarchy(struct of_device_id *dtpm_match_table)
 {
+	const struct of_device_id *match;
+	const struct dtpm_node *hierarchy;
 	struct dtpm_descr *dtpm_descr;
+	struct device_node *np;
+	int ret;
+
+	np = of_find_node_by_path("/");
+	if (!np)
+		return -ENODEV;
+
+	match = of_match_node(dtpm_match_table, np);
 
+	of_node_put(np);
+
+	if (!match)
+		return -ENODEV;
+
+	hierarchy = match->data;
+	if (!hierarchy)
+		return -EFAULT;
+
+	ret = dtpm_for_each_child(hierarchy, NULL, NULL);
+	if (ret)
+		return ret;
+	
+	for_each_dtpm_table(dtpm_descr) {
+
+		if (!dtpm_descr->init)
+			continue;
+
+		dtpm_descr->init();
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dtpm_create_hierarchy);
+
+static int __init init_dtpm(void)
+{
 	pct = powercap_register_control_type(NULL, "dtpm", NULL);
 	if (IS_ERR(pct)) {
 		pr_err("Failed to register control type\n");
 		return PTR_ERR(pct);
 	}
 
-	for_each_dtpm_table(dtpm_descr)
-		dtpm_descr->init();
-
 	return 0;
 }
-late_initcall(init_dtpm);
+fs_initcall_sync(init_dtpm);
diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
index b740866b228d..6bffb44c75aa 100644
--- a/drivers/powercap/dtpm_cpu.c
+++ b/drivers/powercap/dtpm_cpu.c
@@ -269,4 +269,4 @@ static int __init dtpm_cpu_init(void)
 	return 0;
 }
 
-DTPM_DECLARE(dtpm_cpu, dtpm_cpu_init);
+DTPM_DECLARE(dtpm_cpu, dtpm_cpu_init, NULL);
diff --git a/include/linux/dtpm.h b/include/linux/dtpm.h
index d37e5d06a357..5a6b31eaf7e4 100644
--- a/include/linux/dtpm.h
+++ b/include/linux/dtpm.h
@@ -32,23 +32,39 @@ struct dtpm_ops {
 	void (*release)(struct dtpm *);
 };
 
+struct device_node;
+
 typedef int (*dtpm_init_t)(void);
+typedef int (*dtpm_setup_t)(struct dtpm *, struct device_node *);
 
 struct dtpm_descr {
 	dtpm_init_t init;
+	dtpm_setup_t setup;
+};
+
+enum DTPM_NODE_TYPE {
+	DTPM_NODE_VIRTUAL = 0,
+	DTPM_NODE_DT,
+};
+
+struct dtpm_node {
+	enum DTPM_NODE_TYPE type;
+	const char *name;
+	struct dtpm_node *parent;
 };
 
 /* Init section thermal table */
 extern struct dtpm_descr __dtpm_table[];
 extern struct dtpm_descr __dtpm_table_end[];
 
-#define DTPM_TABLE_ENTRY(name, __init)				\
+#define DTPM_TABLE_ENTRY(name, __init, __setup)			\
 	static struct dtpm_descr __dtpm_table_entry_##name	\
 	__used __section("__dtpm_table") = {			\
 		.init = __init,					\
+		.setup = __setup,				\
 	}
 
-#define DTPM_DECLARE(name, init)	DTPM_TABLE_ENTRY(name, init)
+#define DTPM_DECLARE(name, init, setup)	DTPM_TABLE_ENTRY(name, init, setup)
 
 #define for_each_dtpm_table(__dtpm)	\
 	for (__dtpm = __dtpm_table;	\
@@ -70,4 +86,5 @@ void dtpm_unregister(struct dtpm *dtpm);
 
 int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent);
 
+int dtpm_create_hierarchy(struct of_device_id *dtpm_match_table);
 #endif
-- 
2.25.1

Re: [PATCH v5 2/6] powercap/drivers/dtpm: Add hierarchy creation

[Ulf Hansson]

On Sat, 18 Dec 2021 at 14:00, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> The DTPM framework is available but without a way to configure it.
>
> This change provides a way to create a hierarchy of DTPM node where
> the power consumption reflects the sum of the children's power
> consumption.
>
> It is up to the platform to specify an array of dtpm nodes where each
> element has a pointer to its parent, except the top most one. The type
> of the node gives the indication of which initialization callback to
> call. At this time, we can create a virtual node, where its purpose is
> to be a parent in the hierarchy, and a DT node where the name
> describes its path.
>
> In order to ensure a nice self-encapsulation, the DTPM table
> descriptors contains a couple of initialization functions, one to
> setup the DTPM backend and one to initialize it up. With this
> approach, the DTPM framework has a very few material to export.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/powercap/Kconfig    |   1 +
>  drivers/powercap/dtpm.c     | 155 ++++++++++++++++++++++++++++++++++--
>  drivers/powercap/dtpm_cpu.c |   2 +-
>  include/linux/dtpm.h        |  21 ++++-
>  4 files changed, 171 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/powercap/Kconfig b/drivers/powercap/Kconfig
> index 8242e8c5ed77..b1ca339957e3 100644
> --- a/drivers/powercap/Kconfig
> +++ b/drivers/powercap/Kconfig
> @@ -46,6 +46,7 @@ config IDLE_INJECT
>
>  config DTPM
>         bool "Power capping for Dynamic Thermal Power Management (EXPERIMENTAL)"
> +       depends on OF
>         help
>           This enables support for the power capping for the dynamic
>           thermal power management userspace engine.
> diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
> index 0fe70687c198..1611c86de5f5 100644
> --- a/drivers/powercap/dtpm.c
> +++ b/drivers/powercap/dtpm.c
> @@ -23,6 +23,7 @@
>  #include <linux/powercap.h>
>  #include <linux/slab.h>
>  #include <linux/mutex.h>
> +#include <linux/of.h>
>
>  #define DTPM_POWER_LIMIT_FLAG 0
>
> @@ -461,19 +462,163 @@ int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent)
>         return 0;
>  }
>
> -static int __init init_dtpm(void)
> +static struct dtpm *dtpm_setup_virtual(const struct dtpm_node *hierarchy,
> +                                      struct dtpm *parent)
> +{
> +       struct dtpm *dtpm;
> +       int ret;
> +
> +       dtpm = kzalloc(sizeof(*dtpm), GFP_KERNEL);
> +       if (!dtpm)
> +               return ERR_PTR(-ENOMEM);
> +       dtpm_init(dtpm, NULL);
> +
> +       ret = dtpm_register(hierarchy->name, dtpm, parent);
> +       if (ret) {
> +               pr_err("Failed to register dtpm node '%s': %d\n",
> +                      hierarchy->name, ret);
> +               kfree(dtpm);
> +               return ERR_PTR(ret);
> +       }
> +
> +       return dtpm;
> +}
> +
> +static struct dtpm *dtpm_setup_dt(const struct dtpm_node *hierarchy,
> +                                 struct dtpm *parent)
> +{
> +       struct dtpm_descr *dtpm_descr;
> +       struct device_node *np;
> +       int ret;
> +
> +       np = of_find_node_by_path(hierarchy->name);
> +       if (!np) {
> +               pr_err("Failed to find '%s'\n", hierarchy->name);
> +               return ERR_PTR(-ENXIO);
> +       }
> +
> +       for_each_dtpm_table(dtpm_descr) {
> +
> +               ret = dtpm_descr->setup(parent, np);

This will unconditionally call the ->setup callback() for each dtpm
desc in the dtpm table. At this point the ->setup() callback has not
been assigned by anyone that uses DTPM_DECLARE(), so if this would be
called, it would trigger a NULL pointer dereference error.

On the other hand, we don't have someone calling
dtpm_create_hierarchy() yet, so this code doesn't get exercised, but
it still looks a bit odd to me. Maybe squashing patch2 and patch3 is
an option?

> +               if (ret) {
> +                       pr_err("Failed to setup '%s': %d\n", hierarchy->name, ret);
> +                       of_node_put(np);
> +                       return ERR_PTR(ret);
> +               }
> +
> +               of_node_put(np);

This will be called for every loop in the dtpm table. This is wrong,
you only want to call it once, outside the loop.

> +       }
> +
> +       /*
> +        * By returning a NULL pointer, we let know the caller there
> +        * is no child for us as we are a leaf of the tree
> +        */
> +       return NULL;
> +}
> +
> +typedef struct dtpm * (*dtpm_node_callback_t)(const struct dtpm_node *, struct dtpm *);
> +
> +dtpm_node_callback_t dtpm_node_callback[] = {
> +       [DTPM_NODE_VIRTUAL] = dtpm_setup_virtual,
> +       [DTPM_NODE_DT] = dtpm_setup_dt,
> +};
> +
> +static int dtpm_for_each_child(const struct dtpm_node *hierarchy,
> +                              const struct dtpm_node *it, struct dtpm *parent)
> +{
> +       struct dtpm *dtpm;
> +       int i, ret;
> +
> +       for (i = 0; hierarchy[i].name; i++) {
> +
> +               if (hierarchy[i].parent != it)
> +                       continue;
> +
> +               dtpm = dtpm_node_callback[hierarchy[i].type](&hierarchy[i], parent);
> +               if (!dtpm || IS_ERR(dtpm))
> +                       continue;
> +
> +               ret = dtpm_for_each_child(hierarchy, &hierarchy[i], dtpm);

Why do you need to recursively call dtpm_for_each_child() here?

Is there a restriction on how the dtpm core code manages adding
children/parents?

> +               if (ret)
> +                       return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +/**
> + * dtpm_create_hierarchy - Create the dtpm hierarchy
> + * @hierarchy: An array of struct dtpm_node describing the hierarchy
> + *
> + * The function is called by the platform specific code with the
> + * description of the different node in the hierarchy. It creates the
> + * tree in the sysfs filesystem under the powercap dtpm entry.
> + *
> + * The expected tree has the format:
> + *
> + * struct dtpm_node hierarchy[] = {
> + *     [0] { .name = "topmost" },

For clarity, I think we should also specify DTPM_NODE_VIRTUAL here.

> + *      [1] { .name = "package", .parent = &hierarchy[0] },

Ditto.

> + *      [2] { .name = "/cpus/cpu0", .type = DTPM_NODE_DT, .parent = &hierarchy[1] },
> + *      [3] { .name = "/cpus/cpu1", .type = DTPM_NODE_DT, .parent = &hierarchy[1] },
> + *      [4] { .name = "/cpus/cpu2", .type = DTPM_NODE_DT, .parent = &hierarchy[1] },
> + *      [5] { .name = "/cpus/cpu3", .type = DTPM_NODE_DT, .parent = &hierarchy[1] },
> + *     [6] { }
> + * };
> + *
> + * The last element is always an empty one and marks the end of the
> + * array.
> + *
> + * Return: zero on success, a negative value in case of error. Errors
> + * are reported back from the underlying functions.
> + */
> +int dtpm_create_hierarchy(struct of_device_id *dtpm_match_table)
>  {
> +       const struct of_device_id *match;
> +       const struct dtpm_node *hierarchy;
>         struct dtpm_descr *dtpm_descr;
> +       struct device_node *np;
> +       int ret;
> +
> +       np = of_find_node_by_path("/");
> +       if (!np)
> +               return -ENODEV;
> +
> +       match = of_match_node(dtpm_match_table, np);
>
> +       of_node_put(np);
> +
> +       if (!match)
> +               return -ENODEV;
> +
> +       hierarchy = match->data;
> +       if (!hierarchy)
> +               return -EFAULT;
> +
> +       ret = dtpm_for_each_child(hierarchy, NULL, NULL);
> +       if (ret)
> +               return ret;
> +
> +       for_each_dtpm_table(dtpm_descr) {
> +
> +               if (!dtpm_descr->init)
> +                       continue;
> +
> +               dtpm_descr->init();
> +       }
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(dtpm_create_hierarchy);
> +
> +static int __init init_dtpm(void)
> +{
>         pct = powercap_register_control_type(NULL, "dtpm", NULL);
>         if (IS_ERR(pct)) {
>                 pr_err("Failed to register control type\n");
>                 return PTR_ERR(pct);
>         }

It looks like powercap_register_control_type() should be able to be
called from dtpm_create_hierarchy(). In this way we can simply drop
the initcall below, altogether.

Of course, that assumes that dtpm_create_hierachy() is being called
from a regular module_platform_driver() path - or at least from a
later initcall than fs_initcall(), which is when the "powercap_class"
is being registered. But that sounds like a reasonable assumption we
should be able to make, no?

>
> -       for_each_dtpm_table(dtpm_descr)
> -               dtpm_descr->init();
> -
>         return 0;
>  }
> -late_initcall(init_dtpm);
> +fs_initcall_sync(init_dtpm);
> diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
> index b740866b228d..6bffb44c75aa 100644
> --- a/drivers/powercap/dtpm_cpu.c
> +++ b/drivers/powercap/dtpm_cpu.c
> @@ -269,4 +269,4 @@ static int __init dtpm_cpu_init(void)
>         return 0;
>  }
>
> -DTPM_DECLARE(dtpm_cpu, dtpm_cpu_init);
> +DTPM_DECLARE(dtpm_cpu, dtpm_cpu_init, NULL);
> diff --git a/include/linux/dtpm.h b/include/linux/dtpm.h
> index d37e5d06a357..5a6b31eaf7e4 100644
> --- a/include/linux/dtpm.h
> +++ b/include/linux/dtpm.h
> @@ -32,23 +32,39 @@ struct dtpm_ops {
>         void (*release)(struct dtpm *);
>  };
>
> +struct device_node;
> +
>  typedef int (*dtpm_init_t)(void);
> +typedef int (*dtpm_setup_t)(struct dtpm *, struct device_node *);
>
>  struct dtpm_descr {
>         dtpm_init_t init;
> +       dtpm_setup_t setup;
> +};
> +
> +enum DTPM_NODE_TYPE {
> +       DTPM_NODE_VIRTUAL = 0,
> +       DTPM_NODE_DT,
> +};
> +
> +struct dtpm_node {
> +       enum DTPM_NODE_TYPE type;
> +       const char *name;
> +       struct dtpm_node *parent;
>  };
>
>  /* Init section thermal table */
>  extern struct dtpm_descr __dtpm_table[];
>  extern struct dtpm_descr __dtpm_table_end[];
>
> -#define DTPM_TABLE_ENTRY(name, __init)                         \
> +#define DTPM_TABLE_ENTRY(name, __init, __setup)                        \
>         static struct dtpm_descr __dtpm_table_entry_##name      \
>         __used __section("__dtpm_table") = {                    \
>                 .init = __init,                                 \
> +               .setup = __setup,                               \
>         }
>
> -#define DTPM_DECLARE(name, init)       DTPM_TABLE_ENTRY(name, init)
> +#define DTPM_DECLARE(name, init, setup)        DTPM_TABLE_ENTRY(name, init, setup)
>
>  #define for_each_dtpm_table(__dtpm)    \
>         for (__dtpm = __dtpm_table;     \
> @@ -70,4 +86,5 @@ void dtpm_unregister(struct dtpm *dtpm);
>
>  int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent);
>
> +int dtpm_create_hierarchy(struct of_device_id *dtpm_match_table);

To start simple, I think dtpm_create_hiearchy() is the sufficient
interface to add at this point.

However, it's quite likely that it's going to be called from a regular
module (SoC specific platform driver), which means it needs to manage
->remove() operations too. Anyway, I am fine if we look into that as
improvements on top of the $subject series.

>  #endif
> --
> 2.25.1
>

Kind regards
Uffe

Re: [PATCH v5 2/6] powercap/drivers/dtpm: Add hierarchy creation

[Daniel Lezcano]

On 31/12/2021 14:45, Ulf Hansson wrote:

[ ... ]

>> +static struct dtpm *dtpm_setup_dt(const struct dtpm_node *hierarchy,
>> +                                 struct dtpm *parent)
>> +{
>> +       struct dtpm_descr *dtpm_descr;
>> +       struct device_node *np;
>> +       int ret;
>> +
>> +       np = of_find_node_by_path(hierarchy->name);
>> +       if (!np) {
>> +               pr_err("Failed to find '%s'\n", hierarchy->name);
>> +               return ERR_PTR(-ENXIO);
>> +       }
>> +
>> +       for_each_dtpm_table(dtpm_descr) {
>> +
>> +               ret = dtpm_descr->setup(parent, np);
> 
> This will unconditionally call the ->setup callback() for each dtpm
> desc in the dtpm table. At this point the ->setup() callback has not
> been assigned by anyone that uses DTPM_DECLARE(), so if this would be
> called, it would trigger a NULL pointer dereference error.
> 
> On the other hand, we don't have someone calling
> dtpm_create_hierarchy() yet, so this code doesn't get exercised, but

Yes, that is the reason why the test is not here.

> it still looks a bit odd to me. Maybe squashing patch2 and patch3 is
> an option?
Sure

>> +               if (ret) {
>> +                       pr_err("Failed to setup '%s': %d\n", hierarchy->name, ret);
>> +                       of_node_put(np);
>> +                       return ERR_PTR(ret);
>> +               }
>> +
>> +               of_node_put(np);
> 
> This will be called for every loop in the dtpm table. This is wrong,
> you only want to call it once, outside the loop.

Right, good catch

>> +       }
>> +
>> +       /*
>> +        * By returning a NULL pointer, we let know the caller there
>> +        * is no child for us as we are a leaf of the tree
>> +        */
>> +       return NULL;
>> +}
>> +
>> +typedef struct dtpm * (*dtpm_node_callback_t)(const struct dtpm_node *, struct dtpm *);
>> +
>> +dtpm_node_callback_t dtpm_node_callback[] = {
>> +       [DTPM_NODE_VIRTUAL] = dtpm_setup_virtual,
>> +       [DTPM_NODE_DT] = dtpm_setup_dt,
>> +};
>> +
>> +static int dtpm_for_each_child(const struct dtpm_node *hierarchy,
>> +                              const struct dtpm_node *it, struct dtpm *parent)
>> +{
>> +       struct dtpm *dtpm;
>> +       int i, ret;
>> +
>> +       for (i = 0; hierarchy[i].name; i++) {
>> +
>> +               if (hierarchy[i].parent != it)
>> +                       continue;
>> +
>> +               dtpm = dtpm_node_callback[hierarchy[i].type](&hierarchy[i], parent);
>> +               if (!dtpm || IS_ERR(dtpm))
>> +                       continue;
>> +
>> +               ret = dtpm_for_each_child(hierarchy, &hierarchy[i], dtpm);
> 
> Why do you need to recursively call dtpm_for_each_child() here?
> 
> Is there a restriction on how the dtpm core code manages adding
> children/parents?

[ ... ]

The recursive call is needed given the structure of the tree in an array
in order to connect with the parent.


>> + *
>> + * struct dtpm_node hierarchy[] = {
>> + *     [0] { .name = "topmost" },
> 
> For clarity, I think we should also specify DTPM_NODE_VIRTUAL here.
> 
>> + *      [1] { .name = "package", .parent = &hierarchy[0] },
> 
> Ditto.

Sure

[ ... ]

>> +static int __init init_dtpm(void)
>> +{
>>         pct = powercap_register_control_type(NULL, "dtpm", NULL);
>>         if (IS_ERR(pct)) {
>>                 pr_err("Failed to register control type\n");
>>                 return PTR_ERR(pct);
>>         }
> 
> It looks like powercap_register_control_type() should be able to be
> called from dtpm_create_hierarchy(). In this way we can simply drop
> the initcall below, altogether.
>
> Of course, that assumes that dtpm_create_hierachy() is being called
> from a regular module_platform_driver() path - or at least from a
> later initcall than fs_initcall(), which is when the "powercap_class"
> is being registered. But that sounds like a reasonable assumption we
> should be able to make, no?

Yes, agree. Good suggestion, I will do the change.

[ ... ]

>>  int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent);
>>
>> +int dtpm_create_hierarchy(struct of_device_id *dtpm_match_table);
> 
> To start simple, I think dtpm_create_hiearchy() is the sufficient
> interface to add at this point.
> 
> However, it's quite likely that it's going to be called from a regular
> module (SoC specific platform driver), which means it needs to manage
> ->remove() operations too. Anyway, I am fine if we look into that as
> improvements on top of the $subject series.

Yes, ATM, the modules can not be unloaded on purpose. The removal can be
added later


-- 
<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 2/6] powercap/drivers/dtpm: Add hierarchy creation

[Ulf Hansson]

[...]

> >> +static int dtpm_for_each_child(const struct dtpm_node *hierarchy,
> >> +                              const struct dtpm_node *it, struct dtpm *parent)
> >> +{
> >> +       struct dtpm *dtpm;
> >> +       int i, ret;
> >> +
> >> +       for (i = 0; hierarchy[i].name; i++) {
> >> +
> >> +               if (hierarchy[i].parent != it)
> >> +                       continue;
> >> +
> >> +               dtpm = dtpm_node_callback[hierarchy[i].type](&hierarchy[i], parent);
> >> +               if (!dtpm || IS_ERR(dtpm))
> >> +                       continue;
> >> +
> >> +               ret = dtpm_for_each_child(hierarchy, &hierarchy[i], dtpm);
> >
> > Why do you need to recursively call dtpm_for_each_child() here?
> >
> > Is there a restriction on how the dtpm core code manages adding
> > children/parents?
>
> [ ... ]
>
> The recursive call is needed given the structure of the tree in an array
> in order to connect with the parent.

Right, I believe I understand what you are trying to do here, but I am
not sure if this is the best approach to do this. Maybe it is.

The problem is that we are also allocating memory for a dtpm and we
call dtpm_register() on it in this execution path - and this memory
doesn't get freed up nor unregistered, if any of the later recursive
calls to dtpm_for_each_child() fails.

The point is, it looks like it can get rather messy with the recursive
calls to cope with the error path. Maybe it's easier to store the
allocated dtpms in a list somewhere and use this to also find a
reference of a parent?

Later on, when we may decide to implement "dtpm_destroy_hierarchy()"
(or whatever we would call such interface), you probably need a list
of the allocated dtpms anyway, don't you think?

[...]

Kind regards
Uffe

Re: [PATCH v5 2/6] powercap/drivers/dtpm: Add hierarchy creation

[Daniel Lezcano]

On 07/01/2022 16:54, Ulf Hansson wrote:
> [...]
> 
>>>> +static int dtpm_for_each_child(const struct dtpm_node *hierarchy,
>>>> +                              const struct dtpm_node *it, struct dtpm *parent)
>>>> +{
>>>> +       struct dtpm *dtpm;
>>>> +       int i, ret;
>>>> +
>>>> +       for (i = 0; hierarchy[i].name; i++) {
>>>> +
>>>> +               if (hierarchy[i].parent != it)
>>>> +                       continue;
>>>> +
>>>> +               dtpm = dtpm_node_callback[hierarchy[i].type](&hierarchy[i], parent);
>>>> +               if (!dtpm || IS_ERR(dtpm))
>>>> +                       continue;
>>>> +
>>>> +               ret = dtpm_for_each_child(hierarchy, &hierarchy[i], dtpm);
>>>
>>> Why do you need to recursively call dtpm_for_each_child() here?
>>>
>>> Is there a restriction on how the dtpm core code manages adding
>>> children/parents?
>>
>> [ ... ]
>>
>> The recursive call is needed given the structure of the tree in an array
>> in order to connect with the parent.
> 
> Right, I believe I understand what you are trying to do here, but I am
> not sure if this is the best approach to do this. Maybe it is.
> 
> The problem is that we are also allocating memory for a dtpm and we
> call dtpm_register() on it in this execution path - and this memory
> doesn't get freed up nor unregistered, if any of the later recursive
> calls to dtpm_for_each_child() fails.
> 
> The point is, it looks like it can get rather messy with the recursive
> calls to cope with the error path. Maybe it's easier to store the
> allocated dtpms in a list somewhere and use this to also find a
> reference of a parent?

I think it is better to continue the construction with other nodes even
some of them failed to create, it should be a non critical issue. As an
analogy, if one thermal zone fails to create, the other thermal zones
are not removed.

In addition, that should allow multiple nodes description for different
DT setup for the same platform. That should fix the issue pointed by Bjorn.

> Later on, when we may decide to implement "dtpm_destroy_hierarchy()"
> (or whatever we would call such interface), you probably need a list
> of the allocated dtpms anyway, don't you think?

No it is not necessary, the dtpms list is the dtpm tree itself and it
can be destroyed recursively.


-- 
<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 2/6] powercap/drivers/dtpm: Add hierarchy creation

[Ulf Hansson]

On Mon, 10 Jan 2022 at 16:55, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 07/01/2022 16:54, Ulf Hansson wrote:
> > [...]
> >
> >>>> +static int dtpm_for_each_child(const struct dtpm_node *hierarchy,
> >>>> +                              const struct dtpm_node *it, struct dtpm *parent)
> >>>> +{
> >>>> +       struct dtpm *dtpm;
> >>>> +       int i, ret;
> >>>> +
> >>>> +       for (i = 0; hierarchy[i].name; i++) {
> >>>> +
> >>>> +               if (hierarchy[i].parent != it)
> >>>> +                       continue;
> >>>> +
> >>>> +               dtpm = dtpm_node_callback[hierarchy[i].type](&hierarchy[i], parent);
> >>>> +               if (!dtpm || IS_ERR(dtpm))
> >>>> +                       continue;
> >>>> +
> >>>> +               ret = dtpm_for_each_child(hierarchy, &hierarchy[i], dtpm);
> >>>
> >>> Why do you need to recursively call dtpm_for_each_child() here?
> >>>
> >>> Is there a restriction on how the dtpm core code manages adding
> >>> children/parents?
> >>
> >> [ ... ]
> >>
> >> The recursive call is needed given the structure of the tree in an array
> >> in order to connect with the parent.
> >
> > Right, I believe I understand what you are trying to do here, but I am
> > not sure if this is the best approach to do this. Maybe it is.
> >
> > The problem is that we are also allocating memory for a dtpm and we
> > call dtpm_register() on it in this execution path - and this memory
> > doesn't get freed up nor unregistered, if any of the later recursive
> > calls to dtpm_for_each_child() fails.
> >
> > The point is, it looks like it can get rather messy with the recursive
> > calls to cope with the error path. Maybe it's easier to store the
> > allocated dtpms in a list somewhere and use this to also find a
> > reference of a parent?
>
> I think it is better to continue the construction with other nodes even
> some of them failed to create, it should be a non critical issue. As an
> analogy, if one thermal zone fails to create, the other thermal zones
> are not removed.

Well, what if it fails because its "consumer part" is waiting for some
resource to become available?

Maybe the devfreq driver/subsystem isn't available yet and causes
-EPROBE_DEFER, for example. Perhaps this isn't the way the dtpm
registration works currently, but sure it's worth considering when
going forward, no?

In any case, papering over the error seems quite scary to me. I would
much prefer if we instead could propagate the error code correctly to
the caller of dtpm_create_hierarchy(), to allow it to retry if
necessary.

>
> In addition, that should allow multiple nodes description for different
> DT setup for the same platform. That should fix the issue pointed by Bjorn.
>
> > Later on, when we may decide to implement "dtpm_destroy_hierarchy()"
> > (or whatever we would call such interface), you probably need a list
> > of the allocated dtpms anyway, don't you think?
>
> No it is not necessary, the dtpms list is the dtpm tree itself and it
> can be destroyed recursively.

I could quite figure out how that should work though, but I assume
that is what the ->release() callback in the struct dtpm_ops is there
to help with, in some way.

Kind regards
Uffe

Re: [PATCH v5 2/6] powercap/drivers/dtpm: Add hierarchy creation

[Daniel Lezcano]

On 11/01/2022 09:28, Ulf Hansson wrote:
> On Mon, 10 Jan 2022 at 16:55, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> On 07/01/2022 16:54, Ulf Hansson wrote:
>>> [...]
>>>
>>>>>> +static int dtpm_for_each_child(const struct dtpm_node *hierarchy,
>>>>>> +                              const struct dtpm_node *it, struct dtpm *parent)
>>>>>> +{
>>>>>> +       struct dtpm *dtpm;
>>>>>> +       int i, ret;
>>>>>> +
>>>>>> +       for (i = 0; hierarchy[i].name; i++) {
>>>>>> +
>>>>>> +               if (hierarchy[i].parent != it)
>>>>>> +                       continue;
>>>>>> +
>>>>>> +               dtpm = dtpm_node_callback[hierarchy[i].type](&hierarchy[i], parent);
>>>>>> +               if (!dtpm || IS_ERR(dtpm))
>>>>>> +                       continue;
>>>>>> +
>>>>>> +               ret = dtpm_for_each_child(hierarchy, &hierarchy[i], dtpm);
>>>>>
>>>>> Why do you need to recursively call dtpm_for_each_child() here?
>>>>>
>>>>> Is there a restriction on how the dtpm core code manages adding
>>>>> children/parents?
>>>>
>>>> [ ... ]
>>>>
>>>> The recursive call is needed given the structure of the tree in an array
>>>> in order to connect with the parent.
>>>
>>> Right, I believe I understand what you are trying to do here, but I am
>>> not sure if this is the best approach to do this. Maybe it is.
>>>
>>> The problem is that we are also allocating memory for a dtpm and we
>>> call dtpm_register() on it in this execution path - and this memory
>>> doesn't get freed up nor unregistered, if any of the later recursive
>>> calls to dtpm_for_each_child() fails.
>>>
>>> The point is, it looks like it can get rather messy with the recursive
>>> calls to cope with the error path. Maybe it's easier to store the
>>> allocated dtpms in a list somewhere and use this to also find a
>>> reference of a parent?
>>
>> I think it is better to continue the construction with other nodes even
>> some of them failed to create, it should be a non critical issue. As an
>> analogy, if one thermal zone fails to create, the other thermal zones
>> are not removed.
> 
> Well, what if it fails because its "consumer part" is waiting for some
> resource to become available?
> 
> Maybe the devfreq driver/subsystem isn't available yet and causes
> -EPROBE_DEFER, for example. Perhaps this isn't the way the dtpm
> registration works currently, but sure it's worth considering when
> going forward, no?

It should be solved by the fact that the DTPM description is a module
and loaded after the system booted. The module loading ordering is
solved by userspace.

I agree, we could improve that but it is way too complex to be addressed
in a single series and should be part of a specific change IMO.

> In any case, papering over the error seems quite scary to me. I would
> much prefer if we instead could propagate the error code correctly to
> the caller of dtpm_create_hierarchy(), to allow it to retry if
> necessary.

It is really something we should be able to address later.

>> In addition, that should allow multiple nodes description for different
>> DT setup for the same platform. That should fix the issue pointed by Bjorn.
>>
>>> Later on, when we may decide to implement "dtpm_destroy_hierarchy()"
>>> (or whatever we would call such interface), you probably need a list
>>> of the allocated dtpms anyway, don't you think?
>>
>> No it is not necessary, the dtpms list is the dtpm tree itself and it
>> can be destroyed recursively.
> 
> I could quite figure out how that should work though, but I assume
> that is what the ->release() callback in the struct dtpm_ops is there
> to help with, in some way.
> 
> Kind regards
> Uffe
> 


-- 
<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 2/6] powercap/drivers/dtpm: Add hierarchy creation

[Ulf Hansson]

On Tue, 11 Jan 2022 at 18:52, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 11/01/2022 09:28, Ulf Hansson wrote:
> > On Mon, 10 Jan 2022 at 16:55, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 07/01/2022 16:54, Ulf Hansson wrote:
> >>> [...]
> >>>
> >>>>>> +static int dtpm_for_each_child(const struct dtpm_node *hierarchy,
> >>>>>> +                              const struct dtpm_node *it, struct dtpm *parent)
> >>>>>> +{
> >>>>>> +       struct dtpm *dtpm;
> >>>>>> +       int i, ret;
> >>>>>> +
> >>>>>> +       for (i = 0; hierarchy[i].name; i++) {
> >>>>>> +
> >>>>>> +               if (hierarchy[i].parent != it)
> >>>>>> +                       continue;
> >>>>>> +
> >>>>>> +               dtpm = dtpm_node_callback[hierarchy[i].type](&hierarchy[i], parent);
> >>>>>> +               if (!dtpm || IS_ERR(dtpm))
> >>>>>> +                       continue;
> >>>>>> +
> >>>>>> +               ret = dtpm_for_each_child(hierarchy, &hierarchy[i], dtpm);
> >>>>>
> >>>>> Why do you need to recursively call dtpm_for_each_child() here?
> >>>>>
> >>>>> Is there a restriction on how the dtpm core code manages adding
> >>>>> children/parents?
> >>>>
> >>>> [ ... ]
> >>>>
> >>>> The recursive call is needed given the structure of the tree in an array
> >>>> in order to connect with the parent.
> >>>
> >>> Right, I believe I understand what you are trying to do here, but I am
> >>> not sure if this is the best approach to do this. Maybe it is.
> >>>
> >>> The problem is that we are also allocating memory for a dtpm and we
> >>> call dtpm_register() on it in this execution path - and this memory
> >>> doesn't get freed up nor unregistered, if any of the later recursive
> >>> calls to dtpm_for_each_child() fails.
> >>>
> >>> The point is, it looks like it can get rather messy with the recursive
> >>> calls to cope with the error path. Maybe it's easier to store the
> >>> allocated dtpms in a list somewhere and use this to also find a
> >>> reference of a parent?
> >>
> >> I think it is better to continue the construction with other nodes even
> >> some of them failed to create, it should be a non critical issue. As an
> >> analogy, if one thermal zone fails to create, the other thermal zones
> >> are not removed.
> >
> > Well, what if it fails because its "consumer part" is waiting for some
> > resource to become available?
> >
> > Maybe the devfreq driver/subsystem isn't available yet and causes
> > -EPROBE_DEFER, for example. Perhaps this isn't the way the dtpm
> > registration works currently, but sure it's worth considering when
> > going forward, no?
>
> It should be solved by the fact that the DTPM description is a module
> and loaded after the system booted. The module loading ordering is
> solved by userspace.

Ideally, yes. However, drivers/subsystems in the kernel should respect
-EPROBE_DEFER. It's good practice to do that.

>
> I agree, we could improve that but it is way too complex to be addressed
> in a single series and should be part of a specific change IMO.

It's not my call to make, but I don't agree, sorry.

In my opinion, plain error handling to avoid leaking memory isn't
something that should be addressed later. At least if the problems are
already spotted during review.

>
> > In any case, papering over the error seems quite scary to me. I would
> > much prefer if we instead could propagate the error code correctly to
> > the caller of dtpm_create_hierarchy(), to allow it to retry if
> > necessary.
>
> It is really something we should be able to address later.
>

[...]

Kind regards
Uffe

Re: [PATCH v5 2/6] powercap/drivers/dtpm: Add hierarchy creation

[Daniel Lezcano]

On 12/01/2022 13:00, Ulf Hansson wrote:
> On Tue, 11 Jan 2022 at 18:52, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> On 11/01/2022 09:28, Ulf Hansson wrote:
>>> On Mon, 10 Jan 2022 at 16:55, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> On 07/01/2022 16:54, Ulf Hansson wrote:
>>>>> [...]
>>>>>
>>>>>>>> +static int dtpm_for_each_child(const struct dtpm_node *hierarchy,
>>>>>>>> +                              const struct dtpm_node *it, struct dtpm *parent)
>>>>>>>> +{
>>>>>>>> +       struct dtpm *dtpm;
>>>>>>>> +       int i, ret;
>>>>>>>> +
>>>>>>>> +       for (i = 0; hierarchy[i].name; i++) {
>>>>>>>> +
>>>>>>>> +               if (hierarchy[i].parent != it)
>>>>>>>> +                       continue;
>>>>>>>> +
>>>>>>>> +               dtpm = dtpm_node_callback[hierarchy[i].type](&hierarchy[i], parent);
>>>>>>>> +               if (!dtpm || IS_ERR(dtpm))
>>>>>>>> +                       continue;
>>>>>>>> +
>>>>>>>> +               ret = dtpm_for_each_child(hierarchy, &hierarchy[i], dtpm);
>>>>>>>
>>>>>>> Why do you need to recursively call dtpm_for_each_child() here?
>>>>>>>
>>>>>>> Is there a restriction on how the dtpm core code manages adding
>>>>>>> children/parents?
>>>>>>
>>>>>> [ ... ]
>>>>>>
>>>>>> The recursive call is needed given the structure of the tree in an array
>>>>>> in order to connect with the parent.
>>>>>
>>>>> Right, I believe I understand what you are trying to do here, but I am
>>>>> not sure if this is the best approach to do this. Maybe it is.
>>>>>
>>>>> The problem is that we are also allocating memory for a dtpm and we
>>>>> call dtpm_register() on it in this execution path - and this memory
>>>>> doesn't get freed up nor unregistered, if any of the later recursive
>>>>> calls to dtpm_for_each_child() fails.
>>>>>
>>>>> The point is, it looks like it can get rather messy with the recursive
>>>>> calls to cope with the error path. Maybe it's easier to store the
>>>>> allocated dtpms in a list somewhere and use this to also find a
>>>>> reference of a parent?
>>>>
>>>> I think it is better to continue the construction with other nodes even
>>>> some of them failed to create, it should be a non critical issue. As an
>>>> analogy, if one thermal zone fails to create, the other thermal zones
>>>> are not removed.
>>>
>>> Well, what if it fails because its "consumer part" is waiting for some
>>> resource to become available?
>>>
>>> Maybe the devfreq driver/subsystem isn't available yet and causes
>>> -EPROBE_DEFER, for example. Perhaps this isn't the way the dtpm
>>> registration works currently, but sure it's worth considering when
>>> going forward, no?
>>
>> It should be solved by the fact that the DTPM description is a module
>> and loaded after the system booted. The module loading ordering is
>> solved by userspace.
> 
> Ideally, yes. However, drivers/subsystems in the kernel should respect
> -EPROBE_DEFER. It's good practice to do that.

Certainly.

However, it does not make sense because dtpm is not a device and I don't
see a device returning EPROBE_DEFER right now.

Wanting to handle EPROBE_DEFER will make the code a gaz factory:
 - shall we destroy the hierarchy each time a device is returning a
EPROBE_DEFER ?
     * yes : then we need to recreate it every time we recall it and we
end with an empty tree in case of error
     * no : we have to keep track of what was created or not, in order
to attach the newly device to the tree with a the parent, etc ...

So an incredible complexity for actually having no device returning
EPROBE_DEFER.

In addition, let's imagine one of the component like cpufreq is a
module, no EPROBE_DEFER handling will prevent the description being
created before the cpufreq driver is loaded.

But... I agree the hierarchy creation function should be called after
all the devices were created. For that, I think the kernel is providing
what is needed:

1. We compile the SoC specific dtpm always as a module

	depends on ... && m

2. In the module we add the dependencies to other modules

	MODULE_SOFTDEP(post: panfrost)

And with that, all dependencies are explicitly described and the
hierarchy creation is safe.

Does it make sense ?



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

[PATCH v5 3/6] powercap/drivers/dtpm: Add CPU DT initialization support

[Daniel Lezcano]

Based on the previous DT changes in the core code, use the 'setup'
callback to initialize the CPU DTPM backend.

Code is reorganized to stick to the DTPM table description. No
functional changes.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/powercap/dtpm_cpu.c | 37 ++++++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
index 6bffb44c75aa..ca605911523b 100644
--- a/drivers/powercap/dtpm_cpu.c
+++ b/drivers/powercap/dtpm_cpu.c
@@ -21,6 +21,7 @@
 #include <linux/cpuhotplug.h>
 #include <linux/dtpm.h>
 #include <linux/energy_model.h>
+#include <linux/of.h>
 #include <linux/pm_qos.h>
 #include <linux/slab.h>
 #include <linux/units.h>
@@ -176,6 +177,17 @@ static int cpuhp_dtpm_cpu_offline(unsigned int cpu)
 }
 
 static int cpuhp_dtpm_cpu_online(unsigned int cpu)
+{
+	struct dtpm_cpu *dtpm_cpu;
+
+	dtpm_cpu = per_cpu(dtpm_per_cpu, cpu);
+	if (dtpm_cpu)
+		return dtpm_update_power(&dtpm_cpu->dtpm);
+
+	return 0;
+}
+
+static int __dtpm_cpu_setup(int cpu, struct dtpm *parent)
 {
 	struct dtpm_cpu *dtpm_cpu;
 	struct cpufreq_policy *policy;
@@ -183,6 +195,10 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
 	char name[CPUFREQ_NAME_LEN];
 	int ret = -ENOMEM;
 
+	dtpm_cpu = per_cpu(dtpm_per_cpu, cpu);
+	if (dtpm_cpu)
+		return 0;
+
 	policy = cpufreq_cpu_get(cpu);
 	if (!policy)
 		return 0;
@@ -191,10 +207,6 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
 	if (!pd)
 		return -EINVAL;
 
-	dtpm_cpu = per_cpu(dtpm_per_cpu, cpu);
-	if (dtpm_cpu)
-		return dtpm_update_power(&dtpm_cpu->dtpm);
-
 	dtpm_cpu = kzalloc(sizeof(*dtpm_cpu), GFP_KERNEL);
 	if (!dtpm_cpu)
 		return -ENOMEM;
@@ -207,7 +219,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
 
 	snprintf(name, sizeof(name), "cpu%d-cpufreq", dtpm_cpu->cpu);
 
-	ret = dtpm_register(name, &dtpm_cpu->dtpm, NULL);
+	ret = dtpm_register(name, &dtpm_cpu->dtpm, parent);
 	if (ret)
 		goto out_kfree_dtpm_cpu;
 
@@ -231,7 +243,18 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
 	return ret;
 }
 
-static int __init dtpm_cpu_init(void)
+static int dtpm_cpu_setup(struct dtpm *dtpm, struct device_node *np)
+{
+	int cpu;
+
+	cpu = of_cpu_node_to_id(np);
+	if (cpu < 0)
+		return 0;
+
+	return __dtpm_cpu_setup(cpu, dtpm);
+}
+
+static int dtpm_cpu_init(void)
 {
 	int ret;
 
@@ -269,4 +292,4 @@ static int __init dtpm_cpu_init(void)
 	return 0;
 }
 
-DTPM_DECLARE(dtpm_cpu, dtpm_cpu_init, NULL);
+DTPM_DECLARE(dtpm_cpu, dtpm_cpu_init, dtpm_cpu_setup);
-- 
2.25.1

Re: [PATCH v5 3/6] powercap/drivers/dtpm: Add CPU DT initialization support

[Ulf Hansson]

On Sat, 18 Dec 2021 at 14:00, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> Based on the previous DT changes in the core code, use the 'setup'
> callback to initialize the CPU DTPM backend.
>
> Code is reorganized to stick to the DTPM table description. No
> functional changes.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

As I stated for patch2 an option that might be preferred, could be to
squash this one into it.

Nevertheless, feel free to add:

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/powercap/dtpm_cpu.c | 37 ++++++++++++++++++++++++++++++-------
>  1 file changed, 30 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
> index 6bffb44c75aa..ca605911523b 100644
> --- a/drivers/powercap/dtpm_cpu.c
> +++ b/drivers/powercap/dtpm_cpu.c
> @@ -21,6 +21,7 @@
>  #include <linux/cpuhotplug.h>
>  #include <linux/dtpm.h>
>  #include <linux/energy_model.h>
> +#include <linux/of.h>
>  #include <linux/pm_qos.h>
>  #include <linux/slab.h>
>  #include <linux/units.h>
> @@ -176,6 +177,17 @@ static int cpuhp_dtpm_cpu_offline(unsigned int cpu)
>  }
>
>  static int cpuhp_dtpm_cpu_online(unsigned int cpu)
> +{
> +       struct dtpm_cpu *dtpm_cpu;
> +
> +       dtpm_cpu = per_cpu(dtpm_per_cpu, cpu);
> +       if (dtpm_cpu)
> +               return dtpm_update_power(&dtpm_cpu->dtpm);
> +
> +       return 0;
> +}
> +
> +static int __dtpm_cpu_setup(int cpu, struct dtpm *parent)
>  {
>         struct dtpm_cpu *dtpm_cpu;
>         struct cpufreq_policy *policy;
> @@ -183,6 +195,10 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
>         char name[CPUFREQ_NAME_LEN];
>         int ret = -ENOMEM;
>
> +       dtpm_cpu = per_cpu(dtpm_per_cpu, cpu);
> +       if (dtpm_cpu)
> +               return 0;
> +
>         policy = cpufreq_cpu_get(cpu);
>         if (!policy)
>                 return 0;
> @@ -191,10 +207,6 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
>         if (!pd)
>                 return -EINVAL;
>
> -       dtpm_cpu = per_cpu(dtpm_per_cpu, cpu);
> -       if (dtpm_cpu)
> -               return dtpm_update_power(&dtpm_cpu->dtpm);
> -
>         dtpm_cpu = kzalloc(sizeof(*dtpm_cpu), GFP_KERNEL);
>         if (!dtpm_cpu)
>                 return -ENOMEM;
> @@ -207,7 +219,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
>
>         snprintf(name, sizeof(name), "cpu%d-cpufreq", dtpm_cpu->cpu);
>
> -       ret = dtpm_register(name, &dtpm_cpu->dtpm, NULL);
> +       ret = dtpm_register(name, &dtpm_cpu->dtpm, parent);
>         if (ret)
>                 goto out_kfree_dtpm_cpu;
>
> @@ -231,7 +243,18 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
>         return ret;
>  }
>
> -static int __init dtpm_cpu_init(void)
> +static int dtpm_cpu_setup(struct dtpm *dtpm, struct device_node *np)
> +{
> +       int cpu;
> +
> +       cpu = of_cpu_node_to_id(np);
> +       if (cpu < 0)
> +               return 0;
> +
> +       return __dtpm_cpu_setup(cpu, dtpm);
> +}
> +
> +static int dtpm_cpu_init(void)
>  {
>         int ret;
>
> @@ -269,4 +292,4 @@ static int __init dtpm_cpu_init(void)
>         return 0;
>  }
>
> -DTPM_DECLARE(dtpm_cpu, dtpm_cpu_init, NULL);
> +DTPM_DECLARE(dtpm_cpu, dtpm_cpu_init, dtpm_cpu_setup);
> --
> 2.25.1
>

[PATCH v5 4/6] powercap/drivers/dtpm: Add dtpm devfreq with energy model support

[Daniel Lezcano]

Currently the dtpm supports the CPUs via cpufreq and the energy
model. This change provides the same for the device which supports
devfreq.

Each device supporting devfreq and having an energy model can be added
to the hierarchy.

The concept is the same as the cpufreq DTPM support: the QoS is used
to aggregate the requests and the energy model gives the value of the
instantaneous power consumption ponderated by the load of the device.

Cc: Chanwoo Choi <cwchoi00@gmail.com>
Cc: Lukasz Luba <lukasz.luba@arm.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/powercap/Kconfig        |   7 ++
 drivers/powercap/Makefile       |   1 +
 drivers/powercap/dtpm_devfreq.c | 201 ++++++++++++++++++++++++++++++++
 3 files changed, 209 insertions(+)
 create mode 100644 drivers/powercap/dtpm_devfreq.c

diff --git a/drivers/powercap/Kconfig b/drivers/powercap/Kconfig
index b1ca339957e3..515e3ceb3393 100644
--- a/drivers/powercap/Kconfig
+++ b/drivers/powercap/Kconfig
@@ -57,4 +57,11 @@ config DTPM_CPU
 	help
 	  This enables support for CPU power limitation based on
 	  energy model.
+
+config DTPM_DEVFREQ
+	bool "Add device power capping based on the energy model"
+	depends on DTPM && ENERGY_MODEL
+	help
+	  This enables support for device power limitation based on
+	  energy model.
 endif
diff --git a/drivers/powercap/Makefile b/drivers/powercap/Makefile
index fabcf388a8d3..494617cdad88 100644
--- a/drivers/powercap/Makefile
+++ b/drivers/powercap/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_DTPM) += dtpm.o
 obj-$(CONFIG_DTPM_CPU) += dtpm_cpu.o
+obj-$(CONFIG_DTPM_DEVFREQ) += dtpm_devfreq.o
 obj-$(CONFIG_POWERCAP)	+= powercap_sys.o
 obj-$(CONFIG_INTEL_RAPL_CORE) += intel_rapl_common.o
 obj-$(CONFIG_INTEL_RAPL) += intel_rapl_msr.o
diff --git a/drivers/powercap/dtpm_devfreq.c b/drivers/powercap/dtpm_devfreq.c
new file mode 100644
index 000000000000..fd3817f71f44
--- /dev/null
+++ b/drivers/powercap/dtpm_devfreq.c
@@ -0,0 +1,201 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2021 Linaro Limited
+ *
+ * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
+ *
+ * The devfreq device combined with the energy model and the load can
+ * give an estimation of the power consumption as well as limiting the
+ * power.
+ *
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/cpumask.h>
+#include <linux/devfreq.h>
+#include <linux/dtpm.h>
+#include <linux/energy_model.h>
+#include <linux/of.h>
+#include <linux/pm_qos.h>
+#include <linux/slab.h>
+#include <linux/units.h>
+
+struct dtpm_devfreq {
+	struct dtpm dtpm;
+	struct dev_pm_qos_request qos_req;
+	struct devfreq *devfreq;
+};
+
+static struct dtpm_devfreq *to_dtpm_devfreq(struct dtpm *dtpm)
+{
+	return container_of(dtpm, struct dtpm_devfreq, dtpm);
+}
+
+static int update_pd_power_uw(struct dtpm *dtpm)
+{
+	struct dtpm_devfreq *dtpm_devfreq = to_dtpm_devfreq(dtpm);
+	struct devfreq *devfreq = dtpm_devfreq->devfreq;
+	struct device *dev = devfreq->dev.parent;
+	struct em_perf_domain *pd = em_pd_get(dev);
+
+	dtpm->power_min = pd->table[0].power;
+	dtpm->power_min *= MICROWATT_PER_MILLIWATT;
+
+	dtpm->power_max = pd->table[pd->nr_perf_states - 1].power;
+	dtpm->power_max *= MICROWATT_PER_MILLIWATT;
+
+	return 0;
+}
+
+static u64 set_pd_power_limit(struct dtpm *dtpm, u64 power_limit)
+{
+	struct dtpm_devfreq *dtpm_devfreq = to_dtpm_devfreq(dtpm);
+	struct devfreq *devfreq = dtpm_devfreq->devfreq;
+	struct device *dev = devfreq->dev.parent;
+	struct em_perf_domain *pd = em_pd_get(dev);
+	unsigned long freq;
+	u64 power;
+	int i;
+
+	for (i = 0; i < pd->nr_perf_states; i++) {
+
+		power = pd->table[i].power * MICROWATT_PER_MILLIWATT;
+		if (power > power_limit)
+			break;
+	}
+
+	freq = pd->table[i - 1].frequency;
+
+	dev_pm_qos_update_request(&dtpm_devfreq->qos_req, freq);
+
+	power_limit = pd->table[i - 1].power * MICROWATT_PER_MILLIWATT;
+
+	return power_limit;
+}
+
+static void _normalize_load(struct devfreq_dev_status *status)
+{
+	if (status->total_time > 0xfffff) {
+		status->total_time >>= 10;
+		status->busy_time >>= 10;
+	}
+
+	status->busy_time <<= 10;
+	status->busy_time /= status->total_time ? : 1;
+
+	status->busy_time = status->busy_time ? : 1;
+	status->total_time = 1024;
+}
+
+static u64 get_pd_power_uw(struct dtpm *dtpm)
+{
+	struct dtpm_devfreq *dtpm_devfreq = to_dtpm_devfreq(dtpm);
+	struct devfreq *devfreq = dtpm_devfreq->devfreq;
+	struct device *dev = devfreq->dev.parent;
+	struct em_perf_domain *pd = em_pd_get(dev);
+	struct devfreq_dev_status status;
+	unsigned long freq;
+	u64 power;
+	int i;
+
+	mutex_lock(&devfreq->lock);
+	status = devfreq->last_status;
+	mutex_unlock(&devfreq->lock);
+
+	freq = DIV_ROUND_UP(status.current_frequency, HZ_PER_KHZ);
+	_normalize_load(&status);
+
+	for (i = 0; i < pd->nr_perf_states; i++) {
+
+		if (pd->table[i].frequency < freq)
+			continue;
+
+		power = pd->table[i].power * MICROWATT_PER_MILLIWATT;
+		power *= status.busy_time;
+		power >>= 10;
+
+		return power;
+	}
+
+	return 0;
+}
+
+static void pd_release(struct dtpm *dtpm)
+{
+	struct dtpm_devfreq *dtpm_devfreq = to_dtpm_devfreq(dtpm);
+
+	if (dev_pm_qos_request_active(&dtpm_devfreq->qos_req))
+		dev_pm_qos_remove_request(&dtpm_devfreq->qos_req);
+
+	kfree(dtpm_devfreq);
+}
+
+static struct dtpm_ops dtpm_ops = {
+	.set_power_uw = set_pd_power_limit,
+	.get_power_uw = get_pd_power_uw,
+	.update_power_uw = update_pd_power_uw,
+	.release = pd_release,
+};
+
+static int __dtpm_devfreq_setup(struct devfreq *devfreq, struct dtpm *parent)
+{
+	struct device *dev = devfreq->dev.parent;
+	struct dtpm_devfreq *dtpm_devfreq;
+	struct em_perf_domain *pd;
+	int ret = -ENOMEM;
+
+	pd = em_pd_get(dev);
+	if (!pd) {
+		ret = dev_pm_opp_of_register_em(dev, NULL);
+		if (ret) {
+			pr_err("No energy model available for '%s'\n", dev_name(dev));
+			return -EINVAL;
+		}
+	}
+
+	dtpm_devfreq = kzalloc(sizeof(*dtpm_devfreq), GFP_KERNEL);
+	if (!dtpm_devfreq)
+		return -ENOMEM;
+
+	dtpm_init(&dtpm_devfreq->dtpm, &dtpm_ops);
+
+	dtpm_devfreq->devfreq = devfreq;
+
+	ret = dtpm_register(dev_name(dev), &dtpm_devfreq->dtpm, parent);
+	if (ret) {
+		pr_err("Failed to register '%s': %d\n", dev_name(dev), ret);
+		goto out_dtpm_devfreq;
+	}
+
+	ret = dev_pm_qos_add_request(dev, &dtpm_devfreq->qos_req,
+				     DEV_PM_QOS_MAX_FREQUENCY,
+				     PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
+	if (ret) {
+		pr_err("Failed to add QoS request: %d\n", ret);
+		goto out_dtpm_unregister;
+	}
+
+	dtpm_update_power(&dtpm_devfreq->dtpm);
+
+	return 0;
+
+out_dtpm_unregister:
+	dtpm_unregister(&dtpm_devfreq->dtpm);
+out_dtpm_devfreq:
+	kfree(dtpm_devfreq);
+
+	return ret;
+}
+
+static int dtpm_devfreq_setup(struct dtpm *dtpm, struct device_node *np)
+{
+	struct devfreq *devfreq;
+
+	devfreq = devfreq_get_devfreq_by_node(np);
+	if (IS_ERR(devfreq))
+		return 0;
+
+	return __dtpm_devfreq_setup(devfreq, dtpm);
+}
+
+DTPM_DECLARE(dtpm_dev, NULL, dtpm_devfreq_setup);
-- 
2.25.1

[PATCH v5 5/6] rockchip/soc/drivers: Add DTPM description for rk3399

[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 parent.

This patch provides a description of the big and Little CPUs and the
GPU and tie them together under a virtual package name. Only rk3399 is
described now.

The description could be extended in the future with the memory
controller with devfreq if it has the energy information.

The hierarchy uses the GPU devfreq with the panfrost driver, and this
one could be loaded as a module. If the hierarchy is created before
the panfrost driver is loaded, it will fail. For this reason the
Kconfig option depends on the panfrost Kconfig's option. If this one
is compiled as a module, automatically the dtpm hierarchy code will be
a module also. Module loading ordering will fix this chicken-egg
problem.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/soc/rockchip/Kconfig  |  8 +++++
 drivers/soc/rockchip/Makefile |  1 +
 drivers/soc/rockchip/dtpm.c   | 56 +++++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+)
 create mode 100644 drivers/soc/rockchip/dtpm.c

diff --git a/drivers/soc/rockchip/Kconfig b/drivers/soc/rockchip/Kconfig
index 25eb2c1e31bb..a88fe6d3064a 100644
--- a/drivers/soc/rockchip/Kconfig
+++ b/drivers/soc/rockchip/Kconfig
@@ -34,4 +34,12 @@ config ROCKCHIP_PM_DOMAINS
 
           If unsure, say N.
 
+config ROCKCHIP_DTPM
+	tristate "Rockchip DTPM hierarchy"
+	depends on DTPM && DRM_PANFROST
+	help
+	 Describe the hierarchy for the Dynamic Thermal Power
+	 Management tree on this platform. That will create all the
+	 power capping capable devices.
+
 endif
diff --git a/drivers/soc/rockchip/Makefile b/drivers/soc/rockchip/Makefile
index 875032f7344e..05f31a4e743c 100644
--- a/drivers/soc/rockchip/Makefile
+++ b/drivers/soc/rockchip/Makefile
@@ -5,3 +5,4 @@
 obj-$(CONFIG_ROCKCHIP_GRF) += grf.o
 obj-$(CONFIG_ROCKCHIP_IODOMAIN) += io-domain.o
 obj-$(CONFIG_ROCKCHIP_PM_DOMAINS) += pm_domains.o
+obj-$(CONFIG_ROCKCHIP_DTPM) += dtpm.o
diff --git a/drivers/soc/rockchip/dtpm.c b/drivers/soc/rockchip/dtpm.c
new file mode 100644
index 000000000000..77edc565c110
--- /dev/null
+++ b/drivers/soc/rockchip/dtpm.c
@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2021 Linaro Limited
+ *
+ * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
+ *
+ * DTPM hierarchy description
+ */
+#include <linux/dtpm.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+static struct dtpm_node __initdata rk3399_hierarchy[] = {
+	[0]{ .name = "rk3399" },
+	[1]{ .name = "package",
+	     .parent = &rk3399_hierarchy[0] },
+	[2]{ .name = "/cpus/cpu@0",
+	     .type = DTPM_NODE_DT,
+	     .parent = &rk3399_hierarchy[1] },
+	[3]{ .name = "/cpus/cpu@1",
+	     .type = DTPM_NODE_DT,
+	     .parent = &rk3399_hierarchy[1] },
+	[4]{ .name = "/cpus/cpu@2",
+	     .type = DTPM_NODE_DT,
+	     .parent = &rk3399_hierarchy[1] },
+	[5]{ .name = "/cpus/cpu@3",
+	     .type = DTPM_NODE_DT,
+	     .parent = &rk3399_hierarchy[1] },
+	[6]{ .name = "/cpus/cpu@100",
+	     .type = DTPM_NODE_DT,
+	     .parent = &rk3399_hierarchy[1] },
+	[7]{ .name = "/cpus/cpu@101",
+	     .type = DTPM_NODE_DT,
+	     .parent = &rk3399_hierarchy[1] },
+	[8]{ .name = "rockchip,rk3399-mali",
+	     .type = DTPM_NODE_DT,
+	     .parent = &rk3399_hierarchy[1] },
+	[9]{ },
+};
+
+static struct of_device_id __initdata rockchip_dtpm_match_table[] = {
+        { .compatible = "rockchip,rk3399", .data = rk3399_hierarchy },
+        {},
+};
+
+static int __init rockchip_dtpm_init(void)
+{
+	return dtpm_create_hierarchy(rockchip_dtpm_match_table);
+}
+late_initcall(rockchip_dtpm_init);
+
+MODULE_DESCRIPTION("Rockchip DTPM driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:dtpm");
+MODULE_AUTHOR("Daniel Lezcano <daniel.lezcano@kernel.org");
-- 
2.25.1

Re: [PATCH v5 5/6] rockchip/soc/drivers: Add DTPM description for rk3399

[Ulf Hansson]

Hi Daniel, Rob,

On Sat, 18 Dec 2021 at 14:00, Daniel Lezcano <daniel.lezcano@linaro.org> 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 parent.
>
> This patch provides a description of the big and Little CPUs and the
> GPU and tie them together under a virtual package name. Only rk3399 is
> described now.
>
> The description could be extended in the future with the memory
> controller with devfreq if it has the energy information.
>
> The hierarchy uses the GPU devfreq with the panfrost driver, and this
> one could be loaded as a module. If the hierarchy is created before
> the panfrost driver is loaded, it will fail. For this reason the
> Kconfig option depends on the panfrost Kconfig's option. If this one
> is compiled as a module, automatically the dtpm hierarchy code will be
> a module also. Module loading ordering will fix this chicken-egg
> problem.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/soc/rockchip/Kconfig  |  8 +++++
>  drivers/soc/rockchip/Makefile |  1 +
>  drivers/soc/rockchip/dtpm.c   | 56 +++++++++++++++++++++++++++++++++++
>  3 files changed, 65 insertions(+)
>  create mode 100644 drivers/soc/rockchip/dtpm.c
>
> diff --git a/drivers/soc/rockchip/Kconfig b/drivers/soc/rockchip/Kconfig
> index 25eb2c1e31bb..a88fe6d3064a 100644
> --- a/drivers/soc/rockchip/Kconfig
> +++ b/drivers/soc/rockchip/Kconfig
> @@ -34,4 +34,12 @@ config ROCKCHIP_PM_DOMAINS
>
>            If unsure, say N.
>
> +config ROCKCHIP_DTPM
> +       tristate "Rockchip DTPM hierarchy"
> +       depends on DTPM && DRM_PANFROST
> +       help
> +        Describe the hierarchy for the Dynamic Thermal Power
> +        Management tree on this platform. That will create all the
> +        power capping capable devices.
> +
>  endif
> diff --git a/drivers/soc/rockchip/Makefile b/drivers/soc/rockchip/Makefile
> index 875032f7344e..05f31a4e743c 100644
> --- a/drivers/soc/rockchip/Makefile
> +++ b/drivers/soc/rockchip/Makefile
> @@ -5,3 +5,4 @@
>  obj-$(CONFIG_ROCKCHIP_GRF) += grf.o
>  obj-$(CONFIG_ROCKCHIP_IODOMAIN) += io-domain.o
>  obj-$(CONFIG_ROCKCHIP_PM_DOMAINS) += pm_domains.o
> +obj-$(CONFIG_ROCKCHIP_DTPM) += dtpm.o
> diff --git a/drivers/soc/rockchip/dtpm.c b/drivers/soc/rockchip/dtpm.c
> new file mode 100644
> index 000000000000..77edc565c110
> --- /dev/null
> +++ b/drivers/soc/rockchip/dtpm.c
> @@ -0,0 +1,56 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2021 Linaro Limited
> + *
> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
> + *
> + * DTPM hierarchy description
> + */
> +#include <linux/dtpm.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +static struct dtpm_node __initdata rk3399_hierarchy[] = {
> +       [0]{ .name = "rk3399" },
> +       [1]{ .name = "package",
> +            .parent = &rk3399_hierarchy[0] },
> +       [2]{ .name = "/cpus/cpu@0",
> +            .type = DTPM_NODE_DT,
> +            .parent = &rk3399_hierarchy[1] },
> +       [3]{ .name = "/cpus/cpu@1",
> +            .type = DTPM_NODE_DT,
> +            .parent = &rk3399_hierarchy[1] },
> +       [4]{ .name = "/cpus/cpu@2",
> +            .type = DTPM_NODE_DT,
> +            .parent = &rk3399_hierarchy[1] },
> +       [5]{ .name = "/cpus/cpu@3",
> +            .type = DTPM_NODE_DT,
> +            .parent = &rk3399_hierarchy[1] },
> +       [6]{ .name = "/cpus/cpu@100",
> +            .type = DTPM_NODE_DT,
> +            .parent = &rk3399_hierarchy[1] },
> +       [7]{ .name = "/cpus/cpu@101",
> +            .type = DTPM_NODE_DT,
> +            .parent = &rk3399_hierarchy[1] },
> +       [8]{ .name = "rockchip,rk3399-mali",
> +            .type = DTPM_NODE_DT,
> +            .parent = &rk3399_hierarchy[1] },
> +       [9]{ },
> +};

I will not object to this, as in the end this seems like what we need
to do, unless we can describe things through generic DT bindings for
DTPM. Right?

Although, if the above is correct, I need to stress that I am kind of
worried that this doesn't really scale. We would need to copy lots of
information from the DTS files into platform specific c-files, to be
able to describe the DTPM hierarchy.

> +
> +static struct of_device_id __initdata rockchip_dtpm_match_table[] = {
> +        { .compatible = "rockchip,rk3399", .data = rk3399_hierarchy },
> +        {},
> +};
> +
> +static int __init rockchip_dtpm_init(void)
> +{
> +       return dtpm_create_hierarchy(rockchip_dtpm_match_table);
> +}
> +late_initcall(rockchip_dtpm_init);
> +
> +MODULE_DESCRIPTION("Rockchip DTPM driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:dtpm");
> +MODULE_AUTHOR("Daniel Lezcano <daniel.lezcano@kernel.org");
> --
> 2.25.1
>

Kind regards
Uffe

Re: [PATCH v5 5/6] rockchip/soc/drivers: Add DTPM description for rk3399

[Geert Uytterhoeven]

On Fri, Dec 31, 2021 at 2:58 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Sat, 18 Dec 2021 at 14:00, Daniel Lezcano <daniel.lezcano@linaro.org> 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 parent.
> >
> > This patch provides a description of the big and Little CPUs and the
> > GPU and tie them together under a virtual package name. Only rk3399 is
> > described now.
> >
> > The description could be extended in the future with the memory
> > controller with devfreq if it has the energy information.
> >
> > The hierarchy uses the GPU devfreq with the panfrost driver, and this
> > one could be loaded as a module. If the hierarchy is created before
> > the panfrost driver is loaded, it will fail. For this reason the
> > Kconfig option depends on the panfrost Kconfig's option. If this one
> > is compiled as a module, automatically the dtpm hierarchy code will be
> > a module also. Module loading ordering will fix this chicken-egg
> > problem.
> >
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> > --- /dev/null
> > +++ b/drivers/soc/rockchip/dtpm.c
> > @@ -0,0 +1,56 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright 2021 Linaro Limited
> > + *
> > + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
> > + *
> > + * DTPM hierarchy description
> > + */
> > +#include <linux/dtpm.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +
> > +static struct dtpm_node __initdata rk3399_hierarchy[] = {
> > +       [0]{ .name = "rk3399" },
> > +       [1]{ .name = "package",
> > +            .parent = &rk3399_hierarchy[0] },
> > +       [2]{ .name = "/cpus/cpu@0",
> > +            .type = DTPM_NODE_DT,
> > +            .parent = &rk3399_hierarchy[1] },
> > +       [3]{ .name = "/cpus/cpu@1",
> > +            .type = DTPM_NODE_DT,
> > +            .parent = &rk3399_hierarchy[1] },
> > +       [4]{ .name = "/cpus/cpu@2",
> > +            .type = DTPM_NODE_DT,
> > +            .parent = &rk3399_hierarchy[1] },
> > +       [5]{ .name = "/cpus/cpu@3",
> > +            .type = DTPM_NODE_DT,
> > +            .parent = &rk3399_hierarchy[1] },
> > +       [6]{ .name = "/cpus/cpu@100",
> > +            .type = DTPM_NODE_DT,
> > +            .parent = &rk3399_hierarchy[1] },
> > +       [7]{ .name = "/cpus/cpu@101",
> > +            .type = DTPM_NODE_DT,
> > +            .parent = &rk3399_hierarchy[1] },
> > +       [8]{ .name = "rockchip,rk3399-mali",
> > +            .type = DTPM_NODE_DT,
> > +            .parent = &rk3399_hierarchy[1] },
> > +       [9]{ },
> > +};
>
> I will not object to this, as in the end this seems like what we need
> to do, unless we can describe things through generic DT bindings for
> DTPM. Right?
>
> Although, if the above is correct, I need to stress that I am kind of
> worried that this doesn't really scale. We would need to copy lots of
> information from the DTS files into platform specific c-files, to be
> able to describe the DTPM hierarchy.

The description in rk3399_hierarchy[] looks fairly similar to a
power-domains hierarchy, like we have in e.g. the various
drivers/soc/renesas/r8*-sysc.c files.  One big difference is that the
latter do not hardcode the node paths in the driver, but use power
domain indices, referenced from DT in power-domains properties.

Perhaps a similar approach can be used for DTPM?
Does DTPM differ a lot from PM Domains? If not, perhaps no new
properties are needed, and power-domains/#power-domain-cells can be
used as is?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v5 5/6] rockchip/soc/drivers: Add DTPM description for rk3399

[Daniel Lezcano]

Hi Geert,

thanks for your feedback

On 04/01/2022 10:29, Geert Uytterhoeven wrote:
> On Fri, Dec 31, 2021 at 2:58 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On Sat, 18 Dec 2021 at 14:00, Daniel Lezcano <daniel.lezcano@linaro.org> 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 parent.
>>>
>>> This patch provides a description of the big and Little CPUs and the
>>> GPU and tie them together under a virtual package name. Only rk3399 is
>>> described now.
>>>
>>> The description could be extended in the future with the memory
>>> controller with devfreq if it has the energy information.
>>>
>>> The hierarchy uses the GPU devfreq with the panfrost driver, and this
>>> one could be loaded as a module. If the hierarchy is created before
>>> the panfrost driver is loaded, it will fail. For this reason the
>>> Kconfig option depends on the panfrost Kconfig's option. If this one
>>> is compiled as a module, automatically the dtpm hierarchy code will be
>>> a module also. Module loading ordering will fix this chicken-egg
>>> problem.
>>>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
>>> --- /dev/null
>>> +++ b/drivers/soc/rockchip/dtpm.c
>>> @@ -0,0 +1,56 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright 2021 Linaro Limited
>>> + *
>>> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> + *
>>> + * DTPM hierarchy description
>>> + */
>>> +#include <linux/dtpm.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_device.h>
>>> +
>>> +static struct dtpm_node __initdata rk3399_hierarchy[] = {
>>> +       [0]{ .name = "rk3399" },
>>> +       [1]{ .name = "package",
>>> +            .parent = &rk3399_hierarchy[0] },
>>> +       [2]{ .name = "/cpus/cpu@0",
>>> +            .type = DTPM_NODE_DT,
>>> +            .parent = &rk3399_hierarchy[1] },
>>> +       [3]{ .name = "/cpus/cpu@1",
>>> +            .type = DTPM_NODE_DT,
>>> +            .parent = &rk3399_hierarchy[1] },
>>> +       [4]{ .name = "/cpus/cpu@2",
>>> +            .type = DTPM_NODE_DT,
>>> +            .parent = &rk3399_hierarchy[1] },
>>> +       [5]{ .name = "/cpus/cpu@3",
>>> +            .type = DTPM_NODE_DT,
>>> +            .parent = &rk3399_hierarchy[1] },
>>> +       [6]{ .name = "/cpus/cpu@100",
>>> +            .type = DTPM_NODE_DT,
>>> +            .parent = &rk3399_hierarchy[1] },
>>> +       [7]{ .name = "/cpus/cpu@101",
>>> +            .type = DTPM_NODE_DT,
>>> +            .parent = &rk3399_hierarchy[1] },
>>> +       [8]{ .name = "rockchip,rk3399-mali",
>>> +            .type = DTPM_NODE_DT,
>>> +            .parent = &rk3399_hierarchy[1] },
>>> +       [9]{ },
>>> +};
>>
>> I will not object to this, as in the end this seems like what we need
>> to do, unless we can describe things through generic DT bindings for
>> DTPM. Right?
>>
>> Although, if the above is correct, I need to stress that I am kind of
>> worried that this doesn't really scale. We would need to copy lots of
>> information from the DTS files into platform specific c-files, to be
>> able to describe the DTPM hierarchy.
> 
> The description in rk3399_hierarchy[] looks fairly similar to a
> power-domains hierarchy, like we have in e.g. the various
> drivers/soc/renesas/r8*-sysc.c files.  One big difference is that the
> latter do not hardcode the node paths in the driver, but use power
> domain indices, referenced from DT in power-domains properties.
> 
> Perhaps a similar approach can be used for DTPM?
> Does DTPM differ a lot from PM Domains? 

Yes they differ. A DTPM node is a powerzone, a place where we can get
and set the power.

That is the reason why initially a separate binding was proposed.

> If not, perhaps no new
> properties are needed, and power-domains/#power-domain-cells can be
> used as is?




-- 
<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 5/6] rockchip/soc/drivers: Add DTPM description for rk3399

[Daniel Lezcano]

On 31/12/2021 14:57, Ulf Hansson wrote:

[ ... ]

>> +static struct dtpm_node __initdata rk3399_hierarchy[] = {
>> +       [0]{ .name = "rk3399" },
>> +       [1]{ .name = "package",
>> +            .parent = &rk3399_hierarchy[0] },
>> +       [2]{ .name = "/cpus/cpu@0",
>> +            .type = DTPM_NODE_DT,
>> +            .parent = &rk3399_hierarchy[1] },
>> +       [3]{ .name = "/cpus/cpu@1",
>> +            .type = DTPM_NODE_DT,
>> +            .parent = &rk3399_hierarchy[1] },
>> +       [4]{ .name = "/cpus/cpu@2",
>> +            .type = DTPM_NODE_DT,
>> +            .parent = &rk3399_hierarchy[1] },
>> +       [5]{ .name = "/cpus/cpu@3",
>> +            .type = DTPM_NODE_DT,
>> +            .parent = &rk3399_hierarchy[1] },
>> +       [6]{ .name = "/cpus/cpu@100",
>> +            .type = DTPM_NODE_DT,
>> +            .parent = &rk3399_hierarchy[1] },
>> +       [7]{ .name = "/cpus/cpu@101",
>> +            .type = DTPM_NODE_DT,
>> +            .parent = &rk3399_hierarchy[1] },
>> +       [8]{ .name = "rockchip,rk3399-mali",
>> +            .type = DTPM_NODE_DT,
>> +            .parent = &rk3399_hierarchy[1] },
>> +       [9]{ },
>> +};
> 
> I will not object to this, as in the end this seems like what we need
> to do, unless we can describe things through generic DT bindings for
> DTPM. Right?

Yes, as asked by Rob, we should try to describe in the kernel first.

> Although, if the above is correct, I need to stress that I am kind of
> worried that this doesn't really scale. We would need to copy lots of
> information from the DTS files into platform specific c-files, to be
> able to describe the DTPM hierarchy.

TBH I don't think it is a lot and it is a __initdata. At least we should
begin with something and see later how to consolidate if it is needed, no?


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

[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

Re: [PATCH v5 0/6] powercap/drivers/dtpm: Create the dtpm hierarchy

[Daniel Lezcano]

Hi all,

any comments on this series. Is it fine if I merge it without patch 6/6?

Heiko, is the description fine for you in the SoC specific code?

Thanks
  -- Daniel

On 18/12/2021 14:00, Daniel Lezcano wrote:
> The DTPM hierarchy is the base to build on top of it a power budget
> allocator. It reflects the power consumption of the group of devices
> and allows to cap their power.
> 
> The core code is there but there is no way to describe the desired
> hierarchy yet.
> 
> A first proposal introduced the description through configfs [1] but
> was rejected [2].
> 
> A second proposal based on the device tree with a binding similar to
> the power domains [3] was proposed but finally rejected [4].
> 
> This fifth version delegates the hierarchy creation to the SoC with a
> specific and self-encapsulated code using an array to describe the tree. The
> SoC DTPM driver defines an array of nodes pointing to their parents.  The
> hierarchy description can integrate a DT node and in the future a SCMI node,
> that means the description can mix different type of nodes.
> 
> In addition to the hierarchy creation, the devfreq dtpm support is also
> integrated into this series.
> 
> This series was tested on a rock960 (revision B - rk3399 based) and a
> db845c (Qualcomm sdm845 based).
> 
> [1] https://lore.kernel.org/all/20210401183654.27214-1-daniel.lezcano@linaro.org/
> [2] https://lore.kernel.org/all/YGYg6ZeZ1181%2FpXk@kroah.com/
> [3] https://lore.kernel.org/all/20211205231558.779698-1-daniel.lezcano@linaro.org/
> [4] https://lore.kernel.org/all/YbfFapsmsjs4qnsg@robh.at.kernel.org/
> 
> Changelog:
>    V5:
>    - Remove DT bindings
>    - Added description with an array
>    - Added simple description for rk3399 and sdm845
>    - Moved dtpm table to the data section
>    
>    V4:
>    - Added missing powerzone-cells
>    - Changed powerzone name to comply with the pattern property
> 
>    V3:
>    - Remove GPU section as no power is available (yet)
>    - Remove '#powerzone-cells' conforming to the bindings change
>    - Removed required property 'compatible'
>    - Removed powerzone-cells from the topmost node
>    - Removed powerzone-cells from cpus 'consumers' in example
>    - Set additionnal property to false
> 
>    V2:
>    - Added pattern properties and stick to powerzone-*
>    - Added required property compatible and powerzone-cells
>    - Added additionnal property
>    - Added compatible
>    - Renamed to 'powerzones'
>    - Added missing powerzone-cells to the topmost node
>    - Fixed errors reported by 'make DT_CHECKER_FLAGS=-m dt_binding_check'
>    - Move description in the SoC dtsi specific file
>    - Fixed missing prototype warning reported by lkp@
> 
>    V1: Initial post
> 
> Daniel Lezcano (6):
>   powercap/drivers/dtpm: Move dtpm table from init to data section
>   powercap/drivers/dtpm: Add hierarchy creation
>   powercap/drivers/dtpm: Add CPU DT initialization support
>   powercap/drivers/dtpm: Add dtpm devfreq with energy model support
>   rockchip/soc/drivers: Add DTPM description for rk3399
>   qcom/soc/drivers: Add DTPM description for sdm845
> 
>  drivers/powercap/Kconfig          |   8 ++
>  drivers/powercap/Makefile         |   1 +
>  drivers/powercap/dtpm.c           | 155 ++++++++++++++++++++++-
>  drivers/powercap/dtpm_cpu.c       |  37 ++++--
>  drivers/powercap/dtpm_devfreq.c   | 201 ++++++++++++++++++++++++++++++
>  drivers/soc/qcom/Kconfig          |   9 ++
>  drivers/soc/qcom/Makefile         |   1 +
>  drivers/soc/qcom/dtpm.c           |  65 ++++++++++
>  drivers/soc/rockchip/Kconfig      |   8 ++
>  drivers/soc/rockchip/Makefile     |   1 +
>  drivers/soc/rockchip/dtpm.c       |  56 +++++++++
>  include/asm-generic/vmlinux.lds.h |   4 +-
>  include/linux/dtpm.h              |  21 +++-
>  13 files changed, 551 insertions(+), 16 deletions(-)
>  create mode 100644 drivers/powercap/dtpm_devfreq.c
>  create mode 100644 drivers/soc/qcom/dtpm.c
>  create mode 100644 drivers/soc/rockchip/dtpm.c
> 


-- 
<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 0/6] powercap/drivers/dtpm: Create the dtpm hierarchy

[Ulf Hansson]

On Thu, 23 Dec 2021 at 14:20, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> Hi all,
>
> any comments on this series. Is it fine if I merge it without patch 6/6?
>
> Heiko, is the description fine for you in the SoC specific code?

FYI, I intend to have a closer look at the series next week. Happy holidays!

Kind regards
Uffe

>
> Thanks
>   -- Daniel
>
> On 18/12/2021 14:00, Daniel Lezcano wrote:
> > The DTPM hierarchy is the base to build on top of it a power budget
> > allocator. It reflects the power consumption of the group of devices
> > and allows to cap their power.
> >
> > The core code is there but there is no way to describe the desired
> > hierarchy yet.
> >
> > A first proposal introduced the description through configfs [1] but
> > was rejected [2].
> >
> > A second proposal based on the device tree with a binding similar to
> > the power domains [3] was proposed but finally rejected [4].
> >
> > This fifth version delegates the hierarchy creation to the SoC with a
> > specific and self-encapsulated code using an array to describe the tree. The
> > SoC DTPM driver defines an array of nodes pointing to their parents.  The
> > hierarchy description can integrate a DT node and in the future a SCMI node,
> > that means the description can mix different type of nodes.
> >
> > In addition to the hierarchy creation, the devfreq dtpm support is also
> > integrated into this series.
> >
> > This series was tested on a rock960 (revision B - rk3399 based) and a
> > db845c (Qualcomm sdm845 based).
> >
> > [1] https://lore.kernel.org/all/20210401183654.27214-1-daniel.lezcano@linaro.org/
> > [2] https://lore.kernel.org/all/YGYg6ZeZ1181%2FpXk@kroah.com/
> > [3] https://lore.kernel.org/all/20211205231558.779698-1-daniel.lezcano@linaro.org/
> > [4] https://lore.kernel.org/all/YbfFapsmsjs4qnsg@robh.at.kernel.org/
> >
> > Changelog:
> >    V5:
> >    - Remove DT bindings
> >    - Added description with an array
> >    - Added simple description for rk3399 and sdm845
> >    - Moved dtpm table to the data section
> >
> >    V4:
> >    - Added missing powerzone-cells
> >    - Changed powerzone name to comply with the pattern property
> >
> >    V3:
> >    - Remove GPU section as no power is available (yet)
> >    - Remove '#powerzone-cells' conforming to the bindings change
> >    - Removed required property 'compatible'
> >    - Removed powerzone-cells from the topmost node
> >    - Removed powerzone-cells from cpus 'consumers' in example
> >    - Set additionnal property to false
> >
> >    V2:
> >    - Added pattern properties and stick to powerzone-*
> >    - Added required property compatible and powerzone-cells
> >    - Added additionnal property
> >    - Added compatible
> >    - Renamed to 'powerzones'
> >    - Added missing powerzone-cells to the topmost node
> >    - Fixed errors reported by 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> >    - Move description in the SoC dtsi specific file
> >    - Fixed missing prototype warning reported by lkp@
> >
> >    V1: Initial post
> >
> > Daniel Lezcano (6):
> >   powercap/drivers/dtpm: Move dtpm table from init to data section
> >   powercap/drivers/dtpm: Add hierarchy creation
> >   powercap/drivers/dtpm: Add CPU DT initialization support
> >   powercap/drivers/dtpm: Add dtpm devfreq with energy model support
> >   rockchip/soc/drivers: Add DTPM description for rk3399
> >   qcom/soc/drivers: Add DTPM description for sdm845
> >
> >  drivers/powercap/Kconfig          |   8 ++
> >  drivers/powercap/Makefile         |   1 +
> >  drivers/powercap/dtpm.c           | 155 ++++++++++++++++++++++-
> >  drivers/powercap/dtpm_cpu.c       |  37 ++++--
> >  drivers/powercap/dtpm_devfreq.c   | 201 ++++++++++++++++++++++++++++++
> >  drivers/soc/qcom/Kconfig          |   9 ++
> >  drivers/soc/qcom/Makefile         |   1 +
> >  drivers/soc/qcom/dtpm.c           |  65 ++++++++++
> >  drivers/soc/rockchip/Kconfig      |   8 ++
> >  drivers/soc/rockchip/Makefile     |   1 +
> >  drivers/soc/rockchip/dtpm.c       |  56 +++++++++
> >  include/asm-generic/vmlinux.lds.h |   4 +-
> >  include/linux/dtpm.h              |  21 +++-
> >  13 files changed, 551 insertions(+), 16 deletions(-)
> >  create mode 100644 drivers/powercap/dtpm_devfreq.c
> >  create mode 100644 drivers/soc/qcom/dtpm.c
> >  create mode 100644 drivers/soc/rockchip/dtpm.c
> >
>
>
> --
> <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 0/6] powercap/drivers/dtpm: Create the dtpm hierarchy

[Daniel Lezcano]

Hi Ulf,

On 23/12/2021 14:32, Ulf Hansson wrote:
> On Thu, 23 Dec 2021 at 14:20, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> Hi all,
>>
>> any comments on this series. Is it fine if I merge it without patch 6/6?
>>
>> Heiko, is the description fine for you in the SoC specific code?
> 
> FYI, I intend to have a closer look at the series next week. Happy holidays!

Ah, ok. Thank you for letting me know.

I'll wait for your feedback then

Thanks

  -- Daniel


>>
>> On 18/12/2021 14:00, Daniel Lezcano wrote:
>>> The DTPM hierarchy is the base to build on top of it a power budget
>>> allocator. It reflects the power consumption of the group of devices
>>> and allows to cap their power.
>>>
>>> The core code is there but there is no way to describe the desired
>>> hierarchy yet.
>>>
>>> A first proposal introduced the description through configfs [1] but
>>> was rejected [2].
>>>
>>> A second proposal based on the device tree with a binding similar to
>>> the power domains [3] was proposed but finally rejected [4].
>>>
>>> This fifth version delegates the hierarchy creation to the SoC with a
>>> specific and self-encapsulated code using an array to describe the tree. The
>>> SoC DTPM driver defines an array of nodes pointing to their parents.  The
>>> hierarchy description can integrate a DT node and in the future a SCMI node,
>>> that means the description can mix different type of nodes.
>>>
>>> In addition to the hierarchy creation, the devfreq dtpm support is also
>>> integrated into this series.
>>>
>>> This series was tested on a rock960 (revision B - rk3399 based) and a
>>> db845c (Qualcomm sdm845 based).
>>>
>>> [1] https://lore.kernel.org/all/20210401183654.27214-1-daniel.lezcano@linaro.org/
>>> [2] https://lore.kernel.org/all/YGYg6ZeZ1181%2FpXk@kroah.com/
>>> [3] https://lore.kernel.org/all/20211205231558.779698-1-daniel.lezcano@linaro.org/
>>> [4] https://lore.kernel.org/all/YbfFapsmsjs4qnsg@robh.at.kernel.org/
>>>
>>> Changelog:
>>>    V5:
>>>    - Remove DT bindings
>>>    - Added description with an array
>>>    - Added simple description for rk3399 and sdm845
>>>    - Moved dtpm table to the data section
>>>
>>>    V4:
>>>    - Added missing powerzone-cells
>>>    - Changed powerzone name to comply with the pattern property
>>>
>>>    V3:
>>>    - Remove GPU section as no power is available (yet)
>>>    - Remove '#powerzone-cells' conforming to the bindings change
>>>    - Removed required property 'compatible'
>>>    - Removed powerzone-cells from the topmost node
>>>    - Removed powerzone-cells from cpus 'consumers' in example
>>>    - Set additionnal property to false
>>>
>>>    V2:
>>>    - Added pattern properties and stick to powerzone-*
>>>    - Added required property compatible and powerzone-cells
>>>    - Added additionnal property
>>>    - Added compatible
>>>    - Renamed to 'powerzones'
>>>    - Added missing powerzone-cells to the topmost node
>>>    - Fixed errors reported by 'make DT_CHECKER_FLAGS=-m dt_binding_check'
>>>    - Move description in the SoC dtsi specific file
>>>    - Fixed missing prototype warning reported by lkp@
>>>
>>>    V1: Initial post
>>>
>>> Daniel Lezcano (6):
>>>   powercap/drivers/dtpm: Move dtpm table from init to data section
>>>   powercap/drivers/dtpm: Add hierarchy creation
>>>   powercap/drivers/dtpm: Add CPU DT initialization support
>>>   powercap/drivers/dtpm: Add dtpm devfreq with energy model support
>>>   rockchip/soc/drivers: Add DTPM description for rk3399
>>>   qcom/soc/drivers: Add DTPM description for sdm845
>>>
>>>  drivers/powercap/Kconfig          |   8 ++
>>>  drivers/powercap/Makefile         |   1 +
>>>  drivers/powercap/dtpm.c           | 155 ++++++++++++++++++++++-
>>>  drivers/powercap/dtpm_cpu.c       |  37 ++++--
>>>  drivers/powercap/dtpm_devfreq.c   | 201 ++++++++++++++++++++++++++++++
>>>  drivers/soc/qcom/Kconfig          |   9 ++
>>>  drivers/soc/qcom/Makefile         |   1 +
>>>  drivers/soc/qcom/dtpm.c           |  65 ++++++++++
>>>  drivers/soc/rockchip/Kconfig      |   8 ++
>>>  drivers/soc/rockchip/Makefile     |   1 +
>>>  drivers/soc/rockchip/dtpm.c       |  56 +++++++++
>>>  include/asm-generic/vmlinux.lds.h |   4 +-
>>>  include/linux/dtpm.h              |  21 +++-
>>>  13 files changed, 551 insertions(+), 16 deletions(-)
>>>  create mode 100644 drivers/powercap/dtpm_devfreq.c
>>>  create mode 100644 drivers/soc/qcom/dtpm.c
>>>  create mode 100644 drivers/soc/rockchip/dtpm.c
>>>
>>
>>
>> --
>> <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


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