| Message ID | 20211218130014.4037640-7-daniel.lezcano@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | None | expand |
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
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
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
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
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 >
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 >>
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
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"); +
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