| Message ID | 20181204052119.806-1-rnayak@codeaurora.org |
|---|---|
| Headers | show |
| Series | Add power domain driver for corners on msm8996/sdm845 | expand |
On 04-12-18, 10:51, Rajendra Nayak wrote: > Add the DT node for the rpmhpd powercontroller. > > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > --- > arch/arm64/boot/dts/qcom/sdm845.dtsi | 51 ++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
On 04-12-18, 10:51, Rajendra Nayak wrote: > Specify the active + sleep and active-only MX power domains as > the parents of the corresponding CX power domains. This will ensure that > performance state requests on CX automatically generate equivalent requests > on MX power domains. > > This is used to enforce a requirement that exists for various > hardware blocks on SDM845 that MX performance state >= CX performance > state for a given operating frequency. > > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > --- > This patch is dependent on the series from > Viresh [1] which adds support to propogate performance states across the > power domain hierarchy which is still being reviewed. > > [1] https://lkml.org/lkml/2018/11/26/333 > > drivers/soc/qcom/rpmhpd.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Overall looks good to me, just some nitpicks around modules and includes. Quoting Rajendra Nayak (2018-12-03 21:21:14) > The Power domains for corners just pass the performance state set by the > consumers to the RPM (Remote Power manager) which then takes care > of setting the appropriate voltage on the corresponding rails to > meet the performance needs. > > We add all Power domain data needed on msm8996 here. This driver can easily > be extended by adding data for other qualcomm SoCs as well. > Why is "Power" capitalized in power domain? > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile > index f25b54cd6cf8..f1b25fdcf2ad 100644 > --- a/drivers/soc/qcom/Makefile > +++ b/drivers/soc/qcom/Makefile > @@ -21,3 +21,4 @@ obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o > obj-$(CONFIG_QCOM_APR) += apr.o > obj-$(CONFIG_QCOM_LLCC) += llcc-slice.o > obj-$(CONFIG_QCOM_SDM845_LLCC) += llcc-sdm845.o > +obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o > diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c > new file mode 100644 > index 000000000000..a0b9f122d793 > --- /dev/null > +++ b/drivers/soc/qcom/rpmpd.c > @@ -0,0 +1,294 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. */ > + > +#include <linux/err.h> > +#include <linux/export.h> And what is exported? > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/module.h> Is it a module? It's only bool so I don't think so. > +#include <linux/mutex.h> > +#include <linux/pm_domain.h> > +#include <linux/mfd/qcom_rpm.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/soc/qcom/smd-rpm.h> > + > +#include <dt-bindings/mfd/qcom-rpm.h> > +#include <dt-bindings/power/qcom-rpmpd.h> > + > +#define domain_to_rpmpd(domain) container_of(domain, struct rpmpd, pd) > + > +/* Resource types */ > +#define RPMPD_SMPA 0x61706d73 > +#define RPMPD_LDOA 0x616f646c > + > +/* Operation Keys */ > +#define KEY_CORNER 0x6e726f63 /* corn */ > +#define KEY_ENABLE 0x6e657773 /* swen */ > +#define KEY_FLOOR_CORNER 0x636676 /* vfc */ > + > +#define DEFINE_RPMPD_CORN_SMPA(_platform, _name, _active, r_id) \ > + static struct rpmpd _platform##_##_active; \ > + static struct rpmpd _platform##_##_name = { \ > + .pd = { .name = #_name, }, \ > + .peer = &_platform##_##_active, \ > + .res_type = RPMPD_SMPA, \ > + .res_id = r_id, \ > + .key = KEY_CORNER, \ > + }; \ > + static struct rpmpd _platform##_##_active = { \ > + .pd = { .name = #_active, }, \ > + .peer = &_platform##_##_name, \ > + .active_only = true, \ > + .res_type = RPMPD_SMPA, \ > + .res_id = r_id, \ > + .key = KEY_CORNER, \ > + } > + > +#define DEFINE_RPMPD_CORN_LDOA(_platform, _name, r_id) \ > + static struct rpmpd _platform##_##_name = { \ > + .pd = { .name = #_name, }, \ > + .res_type = RPMPD_LDOA, \ > + .res_id = r_id, \ > + .key = KEY_CORNER, \ > + } > + > +#define DEFINE_RPMPD_VFC(_platform, _name, r_id, r_type) \ > + static struct rpmpd _platform##_##_name = { \ > + .pd = { .name = #_name, }, \ > + .res_type = r_type, \ > + .res_id = r_id, \ > + .key = KEY_FLOOR_CORNER, \ > + } > + > +#define DEFINE_RPMPD_VFC_SMPA(_platform, _name, r_id) \ > + DEFINE_RPMPD_VFC(_platform, _name, r_id, RPMPD_SMPA) > + > +#define DEFINE_RPMPD_VFC_LDOA(_platform, _name, r_id) \ > + DEFINE_RPMPD_VFC(_platform, _name, r_id, RPMPD_LDOA) > + > +struct rpmpd_req { > + __le32 key; > + __le32 nbytes; > + __le32 value; > +}; > + > +struct rpmpd { > + struct generic_pm_domain pd; > + struct rpmpd *peer; > + const bool active_only; > + unsigned int corner; > + bool enabled; > + const char *res_name; > + const int res_type; > + const int res_id; > + struct qcom_smd_rpm *rpm; > + __le32 key; > +}; > + > +struct rpmpd_desc { > + struct rpmpd **rpmpds; > + size_t num_pds; > +}; > + > +static DEFINE_MUTEX(rpmpd_lock); > + > +/* msm8996 RPM Power domains */ > +DEFINE_RPMPD_CORN_SMPA(msm8996, vddcx, vddcx_ao, 1); > +DEFINE_RPMPD_CORN_SMPA(msm8996, vddmx, vddmx_ao, 2); > +DEFINE_RPMPD_CORN_LDOA(msm8996, vddsscx, 26); Mmm.. CORN... > + > +DEFINE_RPMPD_VFC_SMPA(msm8996, vddcx_vfc, 1); > +DEFINE_RPMPD_VFC_LDOA(msm8996, vddsscx_vfc, 26); > + > +static struct rpmpd *msm8996_rpmpds[] = { > + [MSM8996_VDDCX] = &msm8996_vddcx, > + [MSM8996_VDDCX_AO] = &msm8996_vddcx_ao, > + [MSM8996_VDDCX_VFC] = &msm8996_vddcx_vfc, > + [MSM8996_VDDMX] = &msm8996_vddmx, > + [MSM8996_VDDMX_AO] = &msm8996_vddmx_ao, > + [MSM8996_VDDSSCX] = &msm8996_vddsscx, > + [MSM8996_VDDSSCX_VFC] = &msm8996_vddsscx_vfc, > +}; > + [...] > + } > + > + return of_genpd_add_provider_onecell(pdev->dev.of_node, data); > +} > + > +static struct platform_driver rpmpd_driver = { > + .driver = { > + .name = "qcom-rpmpd", > + .of_match_table = rpmpd_match_table, suppress bind attributes here? > + }, > + .probe = rpmpd_probe, Because there isn't a remove function to tear down the genpds. > +}; > + > +static int __init rpmpd_init(void) > +{ > + return platform_driver_register(&rpmpd_driver); > +} > +core_initcall(rpmpd_init); > + > +static void __exit rpmpd_exit(void) > +{ > + platform_driver_unregister(&rpmpd_driver); > +} > +module_exit(rpmpd_exit); > + > +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. RPM Power Domain Driver"); > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform:qcom-rpmpd"); Is this MODULE_ALIAS required? I thought this wasn't useful because it's auto-generated or something like that. Also, this is bool so these can all go away unless it becomes tristate.
Quoting Rajendra Nayak (2018-12-03 21:21:15) > @@ -221,6 +224,47 @@ static int rpmpd_power_off(struct generic_pm_domain *domain) > return ret; > } > > +static int rpmpd_set_performance(struct generic_pm_domain *domain, > + unsigned int state) > +{ > + int ret = 0; > + struct rpmpd *pd = domain_to_rpmpd(domain); > + > + mutex_lock(&rpmpd_lock); > + > + if (state > MAX_RPMPD_STATE) > + goto out; Does this need to be under the mutex lock? Doesn't look like it. > + > + pd->corner = state; > + > + if (!pd->enabled && (pd->key != KEY_FLOOR_CORNER)) Please drop useless parenthesis. > + goto out; > + > + ret = rpmpd_aggregate_corner(pd); > + > +out: > + mutex_unlock(&rpmpd_lock); > + > + return ret; > +} > + > +static unsigned int rpmpd_get_performance(struct generic_pm_domain *genpd, > + struct dev_pm_opp *opp) > +{ > + struct device_node *np; > + unsigned int corner = 0; > + > + np = dev_pm_opp_get_of_node(opp); > + if (of_property_read_u32(np, "qcom,level", &corner)) { > + pr_err("%s: missing 'qcom,level' property\n", __func__); We leak np reference here, assuming dev_pm_opp_get_of_node() returns an of_node_get() pointer to the caller. > + return 0; > + } > + > + of_node_put(np); This same code exists twice. Perhaps a helper needs to exist for qcom_rpm_get_performance() to pull the number out of the DT. > + > + return corner; > +}
Quoting Rajendra Nayak (2018-12-03 21:21:18) > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi > index b72bdb0a31a5..a6d0cd8d17b0 100644 > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi > @@ -1324,6 +1325,56 @@ > compatible = "qcom,sdm845-rpmh-clk"; > #clock-cells = <1>; > }; > + > + rpmhpd: power-controller { > + compatible = "qcom,sdm845-rpmhpd"; > + #power-domain-cells = <1>; > + operating-points-v2 = <&rpmhpd_opp_table>; > + }; > + > + rpmhpd_opp_table: opp-table { This table should go somewhere else? I don't understand why it's in the rpmh node because it's not an rpmh device. Does it go to the root? Or does it go under rpmhpd itself? I'm not sure.
On 12/5/2018 4:42 AM, Stephen Boyd wrote: > Overall looks good to me, just some nitpicks around modules and > includes. Thanks for the review, I will fix up all your concerns below and respin soon. > > Quoting Rajendra Nayak (2018-12-03 21:21:14) >> The Power domains for corners just pass the performance state set by the >> consumers to the RPM (Remote Power manager) which then takes care >> of setting the appropriate voltage on the corresponding rails to >> meet the performance needs. >> >> We add all Power domain data needed on msm8996 here. This driver can easily >> be extended by adding data for other qualcomm SoCs as well. >> > > Why is "Power" capitalized in power domain? > >> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile >> index f25b54cd6cf8..f1b25fdcf2ad 100644 >> --- a/drivers/soc/qcom/Makefile >> +++ b/drivers/soc/qcom/Makefile >> @@ -21,3 +21,4 @@ obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o >> obj-$(CONFIG_QCOM_APR) += apr.o >> obj-$(CONFIG_QCOM_LLCC) += llcc-slice.o >> obj-$(CONFIG_QCOM_SDM845_LLCC) += llcc-sdm845.o >> +obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o >> diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c >> new file mode 100644 >> index 000000000000..a0b9f122d793 >> --- /dev/null >> +++ b/drivers/soc/qcom/rpmpd.c >> @@ -0,0 +1,294 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. */ >> + >> +#include <linux/err.h> >> +#include <linux/export.h> > > And what is exported? > >> +#include <linux/init.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> > > Is it a module? It's only bool so I don't think so. > >> +#include <linux/mutex.h> >> +#include <linux/pm_domain.h> >> +#include <linux/mfd/qcom_rpm.h> >> +#include <linux/of.h> >> +#include <linux/of_device.h> >> +#include <linux/platform_device.h> >> +#include <linux/soc/qcom/smd-rpm.h> >> + >> +#include <dt-bindings/mfd/qcom-rpm.h> >> +#include <dt-bindings/power/qcom-rpmpd.h> >> + >> +#define domain_to_rpmpd(domain) container_of(domain, struct rpmpd, pd) >> + >> +/* Resource types */ >> +#define RPMPD_SMPA 0x61706d73 >> +#define RPMPD_LDOA 0x616f646c >> + >> +/* Operation Keys */ >> +#define KEY_CORNER 0x6e726f63 /* corn */ >> +#define KEY_ENABLE 0x6e657773 /* swen */ >> +#define KEY_FLOOR_CORNER 0x636676 /* vfc */ >> + >> +#define DEFINE_RPMPD_CORN_SMPA(_platform, _name, _active, r_id) \ >> + static struct rpmpd _platform##_##_active; \ >> + static struct rpmpd _platform##_##_name = { \ >> + .pd = { .name = #_name, }, \ >> + .peer = &_platform##_##_active, \ >> + .res_type = RPMPD_SMPA, \ >> + .res_id = r_id, \ >> + .key = KEY_CORNER, \ >> + }; \ >> + static struct rpmpd _platform##_##_active = { \ >> + .pd = { .name = #_active, }, \ >> + .peer = &_platform##_##_name, \ >> + .active_only = true, \ >> + .res_type = RPMPD_SMPA, \ >> + .res_id = r_id, \ >> + .key = KEY_CORNER, \ >> + } >> + >> +#define DEFINE_RPMPD_CORN_LDOA(_platform, _name, r_id) \ >> + static struct rpmpd _platform##_##_name = { \ >> + .pd = { .name = #_name, }, \ >> + .res_type = RPMPD_LDOA, \ >> + .res_id = r_id, \ >> + .key = KEY_CORNER, \ >> + } >> + >> +#define DEFINE_RPMPD_VFC(_platform, _name, r_id, r_type) \ >> + static struct rpmpd _platform##_##_name = { \ >> + .pd = { .name = #_name, }, \ >> + .res_type = r_type, \ >> + .res_id = r_id, \ >> + .key = KEY_FLOOR_CORNER, \ >> + } >> + >> +#define DEFINE_RPMPD_VFC_SMPA(_platform, _name, r_id) \ >> + DEFINE_RPMPD_VFC(_platform, _name, r_id, RPMPD_SMPA) >> + >> +#define DEFINE_RPMPD_VFC_LDOA(_platform, _name, r_id) \ >> + DEFINE_RPMPD_VFC(_platform, _name, r_id, RPMPD_LDOA) >> + >> +struct rpmpd_req { >> + __le32 key; >> + __le32 nbytes; >> + __le32 value; >> +}; >> + >> +struct rpmpd { >> + struct generic_pm_domain pd; >> + struct rpmpd *peer; >> + const bool active_only; >> + unsigned int corner; >> + bool enabled; >> + const char *res_name; >> + const int res_type; >> + const int res_id; >> + struct qcom_smd_rpm *rpm; >> + __le32 key; >> +}; >> + >> +struct rpmpd_desc { >> + struct rpmpd **rpmpds; >> + size_t num_pds; >> +}; >> + >> +static DEFINE_MUTEX(rpmpd_lock); >> + >> +/* msm8996 RPM Power domains */ >> +DEFINE_RPMPD_CORN_SMPA(msm8996, vddcx, vddcx_ao, 1); >> +DEFINE_RPMPD_CORN_SMPA(msm8996, vddmx, vddmx_ao, 2); >> +DEFINE_RPMPD_CORN_LDOA(msm8996, vddsscx, 26); > > Mmm.. CORN... > >> + >> +DEFINE_RPMPD_VFC_SMPA(msm8996, vddcx_vfc, 1); >> +DEFINE_RPMPD_VFC_LDOA(msm8996, vddsscx_vfc, 26); >> + >> +static struct rpmpd *msm8996_rpmpds[] = { >> + [MSM8996_VDDCX] = &msm8996_vddcx, >> + [MSM8996_VDDCX_AO] = &msm8996_vddcx_ao, >> + [MSM8996_VDDCX_VFC] = &msm8996_vddcx_vfc, >> + [MSM8996_VDDMX] = &msm8996_vddmx, >> + [MSM8996_VDDMX_AO] = &msm8996_vddmx_ao, >> + [MSM8996_VDDSSCX] = &msm8996_vddsscx, >> + [MSM8996_VDDSSCX_VFC] = &msm8996_vddsscx_vfc, >> +}; >> + > [...] >> + } >> + >> + return of_genpd_add_provider_onecell(pdev->dev.of_node, data); >> +} >> + >> +static struct platform_driver rpmpd_driver = { >> + .driver = { >> + .name = "qcom-rpmpd", >> + .of_match_table = rpmpd_match_table, > > suppress bind attributes here? > >> + }, >> + .probe = rpmpd_probe, > > Because there isn't a remove function to tear down the genpds. > >> +}; >> + >> +static int __init rpmpd_init(void) >> +{ >> + return platform_driver_register(&rpmpd_driver); >> +} >> +core_initcall(rpmpd_init); >> + >> +static void __exit rpmpd_exit(void) >> +{ >> + platform_driver_unregister(&rpmpd_driver); >> +} >> +module_exit(rpmpd_exit); >> + >> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. RPM Power Domain Driver"); >> +MODULE_LICENSE("GPL v2"); >> +MODULE_ALIAS("platform:qcom-rpmpd"); > > Is this MODULE_ALIAS required? I thought this wasn't useful because it's > auto-generated or something like that. Also, this is bool so these can > all go away unless it becomes tristate. >
On 12/5/2018 4:44 AM, Stephen Boyd wrote: > Quoting Rajendra Nayak (2018-12-03 21:21:15) >> @@ -221,6 +224,47 @@ static int rpmpd_power_off(struct generic_pm_domain *domain) >> return ret; >> } >> >> +static int rpmpd_set_performance(struct generic_pm_domain *domain, >> + unsigned int state) >> +{ >> + int ret = 0; >> + struct rpmpd *pd = domain_to_rpmpd(domain); >> + >> + mutex_lock(&rpmpd_lock); >> + >> + if (state > MAX_RPMPD_STATE) >> + goto out; > > Does this need to be under the mutex lock? Doesn't look like it. Nope, will move it out. > >> + >> + pd->corner = state; >> + >> + if (!pd->enabled && (pd->key != KEY_FLOOR_CORNER)) > > Please drop useless parenthesis. sure > >> + goto out; >> + >> + ret = rpmpd_aggregate_corner(pd); >> + >> +out: >> + mutex_unlock(&rpmpd_lock); >> + >> + return ret; >> +} >> + >> +static unsigned int rpmpd_get_performance(struct generic_pm_domain *genpd, >> + struct dev_pm_opp *opp) >> +{ >> + struct device_node *np; >> + unsigned int corner = 0; >> + >> + np = dev_pm_opp_get_of_node(opp); >> + if (of_property_read_u32(np, "qcom,level", &corner)) { >> + pr_err("%s: missing 'qcom,level' property\n", __func__); > > We leak np reference here, assuming dev_pm_opp_get_of_node() returns an > of_node_get() pointer to the caller. good catch, will fix. > >> + return 0; >> + } >> + >> + of_node_put(np); > > This same code exists twice. Perhaps a helper needs to exist for > qcom_rpm_get_performance() to pull the number out of the DT. Sure I can make both drivers use a common helper instead of duplicating it. > >> + >> + return corner; >> +}
On 12/5/2018 4:46 AM, Stephen Boyd wrote: > Quoting Rajendra Nayak (2018-12-03 21:21:18) >> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi >> index b72bdb0a31a5..a6d0cd8d17b0 100644 >> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi >> @@ -1324,6 +1325,56 @@ >> compatible = "qcom,sdm845-rpmh-clk"; >> #clock-cells = <1>; >> }; >> + >> + rpmhpd: power-controller { >> + compatible = "qcom,sdm845-rpmhpd"; >> + #power-domain-cells = <1>; >> + operating-points-v2 = <&rpmhpd_opp_table>; >> + }; >> + >> + rpmhpd_opp_table: opp-table { > > This table should go somewhere else? I don't understand why it's in the > rpmh node because it's not an rpmh device. Does it go to the root? Or > does it go under rpmhpd itself? I'm not sure. I could move it to root perhaps, we seem to do that atleast in the case of GPU. The power domain bindings (Documentation/devicetree/bindings/power/power_domain.txt) seem to suggest it can't be under the power-controller node itself.
On 12/5/2018 12:33 PM, Rajendra Nayak wrote: >> >>> + return 0; >>> + } >>> + >>> + of_node_put(np); >> >> This same code exists twice. Perhaps a helper needs to exist for >> qcom_rpm_get_performance() to pull the number out of the DT. > > Sure I can make both drivers use a common helper instead of duplicating it. which would mean I will need to create a new file just to define the common helper. Does that seem like an overkill?
Quoting Rajendra Nayak (2018-12-05 02:11:22) > > On 12/5/2018 12:33 PM, Rajendra Nayak wrote: > >> > >>> + return 0; > >>> + } > >>> + > >>> + of_node_put(np); > >> > >> This same code exists twice. Perhaps a helper needs to exist for > >> qcom_rpm_get_performance() to pull the number out of the DT. > > > > Sure I can make both drivers use a common helper instead of duplicating it. > > which would mean I will need to create a new file just to define the > common helper. Does that seem like an overkill? Maybe put it in the genpd code and let it take a const char *name argument that picks out the property that drivers want to look at? That way other OPP properties can be picked out with a simple call to the function but it's generic enough to be used in other places.
On Wed, Dec 05, 2018 at 12:37:29PM +0530, Rajendra Nayak wrote: > > > On 12/5/2018 4:46 AM, Stephen Boyd wrote: > > Quoting Rajendra Nayak (2018-12-03 21:21:18) > > > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi > > > index b72bdb0a31a5..a6d0cd8d17b0 100644 > > > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi > > > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi > > > @@ -1324,6 +1325,56 @@ > > > compatible = "qcom,sdm845-rpmh-clk"; > > > #clock-cells = <1>; > > > }; > > > + > > > + rpmhpd: power-controller { > > > + compatible = "qcom,sdm845-rpmhpd"; > > > + #power-domain-cells = <1>; > > > + operating-points-v2 = <&rpmhpd_opp_table>; > > > + }; > > > + > > > + rpmhpd_opp_table: opp-table { > > > > This table should go somewhere else? I don't understand why it's in the > > rpmh node because it's not an rpmh device. Does it go to the root? Or > > does it go under rpmhpd itself? I'm not sure. > > I could move it to root perhaps, we seem to do that atleast in the case of > GPU. The power domain bindings (Documentation/devicetree/bindings/power/power_domain.txt) > seem to suggest it can't be under the power-controller node itself. Why not? I don't see anything forbidding that like already having some other type of child nodes. It's a little weird to have operating-points-v2 point to a child node, but that will still work. Rob