| Message ID | 1447799871-56374-22-git-send-email-lina.iyer@linaro.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Andy Gross |
| Headers | show |
Hi Lina, On Tue, Nov 17, 2015 at 03:37:45PM -0700, Lina Iyer wrote: > Architectures that support CPU domain control in the firmware specify > the domain heirarchy as part of the topology nodes. Parse and initialize > domains from the topology node for such architectures. > > Cc: Rob Herring <robherring2@gmail.com> > Cc: Stephen Boyd <sboyd@codeaurora.org> > Cc: Kevin Hilman <khilman@linaro.org> > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > --- > drivers/base/power/cpu-pd.c | 76 +++++++++++++++++++++++++++++++++++++++++++++ > include/linux/cpu-pd.h | 1 + > 2 files changed, 77 insertions(+) > > diff --git a/drivers/base/power/cpu-pd.c b/drivers/base/power/cpu-pd.c > index e331ae6..2872c18 100644 > --- a/drivers/base/power/cpu-pd.c > +++ b/drivers/base/power/cpu-pd.c > @@ -429,3 +429,79 @@ int of_attach_cpu_pm_domain(struct device_node *dn) > return 0; > } > EXPORT_SYMBOL(of_attach_cpu_pm_domain); > + > +static int of_parse_cpu_pd(struct device_node *cluster, > + const struct cpu_pd_ops *ops) > +{ > + struct device_node *domain_node; > + struct generic_pm_domain *genpd; > + char name[10]; > + struct device_node *c; > + int i, ret; > + > + for (i = 0; ; i++) { > + snprintf(name, sizeof(name), "cluster%d", i); > + c = of_get_child_by_name(cluster, name); > + if (!c) > + break; > + > + domain_node = of_parse_phandle(c, "cluster", 0); > + if (!domain_node) > + return -1; > + > + /* Initialize CPU PM domain domain at this level */ > + genpd = of_init_cpu_pm_domain(domain_node, ops); > + if (IS_ERR(genpd)) > + return -1; > + > + /* Initialize and attach child domains */ > + ret = of_parse_cpu_pd(c, ops); > + > + /* > + * Attach the domain to its parent after reading > + * the children, so the mask of CPUs in this domain > + * are setup correctly. > + */ > + if (!ret) > + of_attach_cpu_pm_domain(domain_node); > + > + of_node_put(c); > + if (ret != 0) > + return ret; > + } > + > + return 0; > +} > + > +/** > + * of_setup_cpu_domain_topology() - Setup the CPU domains from the CPU > + * topology node in DT. > + * > + * @ops: The PM domain suspend/resume ops for all the domains in the topology > + */ > +int of_setup_cpu_domain_topology(const struct cpu_pd_ops *ops) > +{ > + struct device_node *cn, *map; > + int ret = 0; > + > + cn = of_find_node_by_path("/cpus"); > + if (!cn) { > + pr_err("No CPU information found in DT\n"); > + return 0; > + } > + > + map = of_get_child_by_name(cn, "cpu-map"); > + if (!map) > + goto out; I commented on this before, is this reliance on cpu-map necessary ? Could not you just rely on the "power-domains" phandle in the cpu nodes to build the cpumask for a specific power domain ? I think you should try to decouple the concept of power domain from the cpu-map cluster and I think this would also simplify your code in the process. So to sum it up, I'd suggest you build the power domain cpumask by enumerating the cpus pointing at a specific power-domain node. Lorenzo > + > + ret = of_parse_cpu_pd(map, ops); > + if (ret != 0) > + goto out_map; > + > +out_map: > + of_node_put(map); > +out: > + of_node_put(cn); > + return ret; > +} > +EXPORT_SYMBOL(of_setup_cpu_domain_topology); > diff --git a/include/linux/cpu-pd.h b/include/linux/cpu-pd.h > index 489ee2f..e8290db 100644 > --- a/include/linux/cpu-pd.h > +++ b/include/linux/cpu-pd.h > @@ -32,4 +32,5 @@ struct cpu_pm_domain { > struct generic_pm_domain *of_init_cpu_pm_domain(struct device_node *dn, > const struct cpu_pd_ops *ops); > int of_attach_cpu_pm_domain(struct device_node *dn); > +int of_setup_cpu_domain_topology(const struct cpu_pd_ops *ops); > #endif /* __CPU_PD_H__ */ > -- > 2.1.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 07 2015 at 07:53 -0700, Lorenzo Pieralisi wrote: >Hi Lina, > >On Tue, Nov 17, 2015 at 03:37:45PM -0700, Lina Iyer wrote: >> Architectures that support CPU domain control in the firmware specify >> the domain heirarchy as part of the topology nodes. Parse and initialize >> domains from the topology node for such architectures. >> >> Cc: Rob Herring <robherring2@gmail.com> >> Cc: Stephen Boyd <sboyd@codeaurora.org> >> Cc: Kevin Hilman <khilman@linaro.org> >> Cc: Ulf Hansson <ulf.hansson@linaro.org> >> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >> Signed-off-by: Lina Iyer <lina.iyer@linaro.org> >> --- >> drivers/base/power/cpu-pd.c | 76 +++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/cpu-pd.h | 1 + >> 2 files changed, 77 insertions(+) >> >> diff --git a/drivers/base/power/cpu-pd.c b/drivers/base/power/cpu-pd.c >> index e331ae6..2872c18 100644 >> --- a/drivers/base/power/cpu-pd.c >> +++ b/drivers/base/power/cpu-pd.c >> @@ -429,3 +429,79 @@ int of_attach_cpu_pm_domain(struct device_node *dn) >> return 0; >> } >> EXPORT_SYMBOL(of_attach_cpu_pm_domain); >> + >> +static int of_parse_cpu_pd(struct device_node *cluster, >> + const struct cpu_pd_ops *ops) >> +{ >> + struct device_node *domain_node; >> + struct generic_pm_domain *genpd; >> + char name[10]; >> + struct device_node *c; >> + int i, ret; >> + >> + for (i = 0; ; i++) { >> + snprintf(name, sizeof(name), "cluster%d", i); >> + c = of_get_child_by_name(cluster, name); >> + if (!c) >> + break; >> + >> + domain_node = of_parse_phandle(c, "cluster", 0); >> + if (!domain_node) >> + return -1; >> + >> + /* Initialize CPU PM domain domain at this level */ >> + genpd = of_init_cpu_pm_domain(domain_node, ops); >> + if (IS_ERR(genpd)) >> + return -1; >> + >> + /* Initialize and attach child domains */ >> + ret = of_parse_cpu_pd(c, ops); >> + >> + /* >> + * Attach the domain to its parent after reading >> + * the children, so the mask of CPUs in this domain >> + * are setup correctly. >> + */ >> + if (!ret) >> + of_attach_cpu_pm_domain(domain_node); >> + >> + of_node_put(c); >> + if (ret != 0) >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +/** >> + * of_setup_cpu_domain_topology() - Setup the CPU domains from the CPU >> + * topology node in DT. >> + * >> + * @ops: The PM domain suspend/resume ops for all the domains in the topology >> + */ >> +int of_setup_cpu_domain_topology(const struct cpu_pd_ops *ops) >> +{ >> + struct device_node *cn, *map; >> + int ret = 0; >> + >> + cn = of_find_node_by_path("/cpus"); >> + if (!cn) { >> + pr_err("No CPU information found in DT\n"); >> + return 0; >> + } >> + >> + map = of_get_child_by_name(cn, "cpu-map"); >> + if (!map) >> + goto out; > >I commented on this before, is this reliance on cpu-map necessary ? >Could not you just rely on the "power-domains" phandle in the cpu >nodes to build the cpumask for a specific power domain ? I think >you should try to decouple the concept of power domain from the cpu-map >cluster and I think this would also simplify your code in the process. > Sorry, I missed seeing your comment on this earlier. Hmm, it can be done, but I felt out of place to define just power domains that have no devices in Linux, since they are handled in PSCI, but also have no relation to PSCI. Hence, I felt cpu-map to be a good place for the cluster domain. But I am not strongly inclined. I can remove them from the cpu-map and keep them as separate nodes. However, it would be nice to have a way to say these nodes are PSCI controlled. Thanks, Lina >So to sum it up, I'd suggest you build the power domain cpumask by >enumerating the cpus pointing at a specific power-domain node. > >> + >> + ret = of_parse_cpu_pd(map, ops); >> + if (ret != 0) >> + goto out_map; >> + >> +out_map: >> + of_node_put(map); >> +out: >> + of_node_put(cn); >> + return ret; >> +} >> +EXPORT_SYMBOL(of_setup_cpu_domain_topology); >> diff --git a/include/linux/cpu-pd.h b/include/linux/cpu-pd.h >> index 489ee2f..e8290db 100644 >> --- a/include/linux/cpu-pd.h >> +++ b/include/linux/cpu-pd.h >> @@ -32,4 +32,5 @@ struct cpu_pm_domain { >> struct generic_pm_domain *of_init_cpu_pm_domain(struct device_node *dn, >> const struct cpu_pd_ops *ops); >> int of_attach_cpu_pm_domain(struct device_node *dn); >> +int of_setup_cpu_domain_topology(const struct cpu_pd_ops *ops); >> #endif /* __CPU_PD_H__ */ >> -- >> 2.1.4 >> -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 08, 2015 at 11:05:36AM -0700, Lina Iyer wrote: [...] > On Mon, Dec 07 2015 at 07:53 -0700, Lorenzo Pieralisi wrote: > >>+/** > >>+ * of_setup_cpu_domain_topology() - Setup the CPU domains from the CPU > >>+ * topology node in DT. > >>+ * > >>+ * @ops: The PM domain suspend/resume ops for all the domains in the topology > >>+ */ > >>+int of_setup_cpu_domain_topology(const struct cpu_pd_ops *ops) > >>+{ > >>+ struct device_node *cn, *map; > >>+ int ret = 0; > >>+ > >>+ cn = of_find_node_by_path("/cpus"); > >>+ if (!cn) { > >>+ pr_err("No CPU information found in DT\n"); > >>+ return 0; > >>+ } > >>+ > >>+ map = of_get_child_by_name(cn, "cpu-map"); > >>+ if (!map) > >>+ goto out; > > > >I commented on this before, is this reliance on cpu-map necessary ? > >Could not you just rely on the "power-domains" phandle in the cpu > >nodes to build the cpumask for a specific power domain ? I think > >you should try to decouple the concept of power domain from the cpu-map > >cluster and I think this would also simplify your code in the process. > > > Sorry, I missed seeing your comment on this earlier. > > Hmm, it can be done, but I felt out of place to define just power > domains that have no devices in Linux, since they are handled in PSCI, > but also have no relation to PSCI. Hence, I felt cpu-map to be a good > place for the cluster domain. But I am not strongly inclined. I can > remove them from the cpu-map and keep them as separate nodes. However, > it would be nice to have a way to say these nodes are PSCI controlled. I do not agree with the way you described the system. Here is how I see it. DT must model HW, and in HW your cpus will be part of a power domain(s) that of course can be hierarchical. Every cpus has a phandle to the power domain it belongs into, and to group them as "cluster" you should follow the power domains hierarchy (ie if you have a cluster power domain, it will contain the core power domains, through the hierarchy it is easy to detect what cpus are part of the cluster power domain by detecting which cpus are part of its children). It is much simpler than the current solution I think, you get rid of cpu-map dependency (honestly it is a bit weird what you did, with cpu-map containing a cluster phandle to a power domain and cpu phandles to cpu nodes, and the DT bindings you posted does not seem to match your dts). For PSCI, nothing prevents you from defining the same bindings except that the power domain(s) is not explicitly controlled by the OS, still, the DT would always describe the HW and you can use it to detect the power domain topology and which cpus are part of a given power domain. Please let me know what you think, thanks. Lorenzo > > Thanks, > Lina > > >So to sum it up, I'd suggest you build the power domain cpumask by > >enumerating the cpus pointing at a specific power-domain node. > > > > >>+ > >>+ ret = of_parse_cpu_pd(map, ops); > >>+ if (ret != 0) > >>+ goto out_map; > >>+ > >>+out_map: > >>+ of_node_put(map); > >>+out: > >>+ of_node_put(cn); > >>+ return ret; > >>+} > >>+EXPORT_SYMBOL(of_setup_cpu_domain_topology); > >>diff --git a/include/linux/cpu-pd.h b/include/linux/cpu-pd.h > >>index 489ee2f..e8290db 100644 > >>--- a/include/linux/cpu-pd.h > >>+++ b/include/linux/cpu-pd.h > >>@@ -32,4 +32,5 @@ struct cpu_pm_domain { > >> struct generic_pm_domain *of_init_cpu_pm_domain(struct device_node *dn, > >> const struct cpu_pd_ops *ops); > >> int of_attach_cpu_pm_domain(struct device_node *dn); > >>+int of_setup_cpu_domain_topology(const struct cpu_pd_ops *ops); > >> #endif /* __CPU_PD_H__ */ > >>-- > >>2.1.4 > >> > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 10, 2015 at 7:11 PM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Tue, Dec 08, 2015 at 11:05:36AM -0700, Lina Iyer wrote: > > [...] > >> On Mon, Dec 07 2015 at 07:53 -0700, Lorenzo Pieralisi wrote: >> >>+/** >> >>+ * of_setup_cpu_domain_topology() - Setup the CPU domains from the CPU >> >>+ * topology node in DT. >> >>+ * >> >>+ * @ops: The PM domain suspend/resume ops for all the domains in the topology >> >>+ */ >> >>+int of_setup_cpu_domain_topology(const struct cpu_pd_ops *ops) >> >>+{ >> >>+ struct device_node *cn, *map; >> >>+ int ret = 0; >> >>+ >> >>+ cn = of_find_node_by_path("/cpus"); >> >>+ if (!cn) { >> >>+ pr_err("No CPU information found in DT\n"); >> >>+ return 0; >> >>+ } >> >>+ >> >>+ map = of_get_child_by_name(cn, "cpu-map"); >> >>+ if (!map) >> >>+ goto out; >> > >> >I commented on this before, is this reliance on cpu-map necessary ? >> >Could not you just rely on the "power-domains" phandle in the cpu >> >nodes to build the cpumask for a specific power domain ? I think >> >you should try to decouple the concept of power domain from the cpu-map >> >cluster and I think this would also simplify your code in the process. >> > >> Sorry, I missed seeing your comment on this earlier. >> >> Hmm, it can be done, but I felt out of place to define just power >> domains that have no devices in Linux, since they are handled in PSCI, >> but also have no relation to PSCI. Hence, I felt cpu-map to be a good >> place for the cluster domain. But I am not strongly inclined. I can >> remove them from the cpu-map and keep them as separate nodes. However, >> it would be nice to have a way to say these nodes are PSCI controlled. > > I do not agree with the way you described the system. > > Here is how I see it. DT must model HW, and in HW your cpus will be part > of a power domain(s) that of course can be hierarchical. Every cpus has a > phandle to the power domain it belongs into, and to group them as "cluster" > you should follow the power domains hierarchy (ie if you have a cluster > power domain, it will contain the core power domains, through the > hierarchy it is easy to detect what cpus are part of the cluster power > domain by detecting which cpus are part of its children). It is much > simpler than the current solution I think, you get rid of cpu-map > dependency (honestly it is a bit weird what you did, with cpu-map > containing a cluster phandle to a power domain and cpu phandles to > cpu nodes, and the DT bindings you posted does not seem to match your > dts). > > For PSCI, nothing prevents you from defining the same bindings except > that the power domain(s) is not explicitly controlled by the OS, still, > the DT would always describe the HW and you can use it to detect the > power domain topology and which cpus are part of a given power domain. > > Please let me know what you think, thanks. +1 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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 11 2015 at 02:04 -0700, Geert Uytterhoeven wrote: >On Thu, Dec 10, 2015 at 7:11 PM, Lorenzo Pieralisi ><lorenzo.pieralisi@arm.com> wrote: >> On Tue, Dec 08, 2015 at 11:05:36AM -0700, Lina Iyer wrote: >> >> [...] >> >>> On Mon, Dec 07 2015 at 07:53 -0700, Lorenzo Pieralisi wrote: >>> >>+/** >>> >>+ * of_setup_cpu_domain_topology() - Setup the CPU domains from the CPU >>> >>+ * topology node in DT. >>> >>+ * >>> >>+ * @ops: The PM domain suspend/resume ops for all the domains in the topology >>> >>+ */ >>> >>+int of_setup_cpu_domain_topology(const struct cpu_pd_ops *ops) >>> >>+{ >>> >>+ struct device_node *cn, *map; >>> >>+ int ret = 0; >>> >>+ >>> >>+ cn = of_find_node_by_path("/cpus"); >>> >>+ if (!cn) { >>> >>+ pr_err("No CPU information found in DT\n"); >>> >>+ return 0; >>> >>+ } >>> >>+ >>> >>+ map = of_get_child_by_name(cn, "cpu-map"); >>> >>+ if (!map) >>> >>+ goto out; >>> > >>> >I commented on this before, is this reliance on cpu-map necessary ? >>> >Could not you just rely on the "power-domains" phandle in the cpu >>> >nodes to build the cpumask for a specific power domain ? I think >>> >you should try to decouple the concept of power domain from the cpu-map >>> >cluster and I think this would also simplify your code in the process. >>> > >>> Sorry, I missed seeing your comment on this earlier. >>> >>> Hmm, it can be done, but I felt out of place to define just power >>> domains that have no devices in Linux, since they are handled in PSCI, >>> but also have no relation to PSCI. Hence, I felt cpu-map to be a good >>> place for the cluster domain. But I am not strongly inclined. I can >>> remove them from the cpu-map and keep them as separate nodes. However, >>> it would be nice to have a way to say these nodes are PSCI controlled. >> >> I do not agree with the way you described the system. >> >> Here is how I see it. DT must model HW, and in HW your cpus will be part >> of a power domain(s) that of course can be hierarchical. Every cpus has a >> phandle to the power domain it belongs into, and to group them as "cluster" >> you should follow the power domains hierarchy (ie if you have a cluster >> power domain, it will contain the core power domains, through the >> hierarchy it is easy to detect what cpus are part of the cluster power >> domain by detecting which cpus are part of its children). It is much >> simpler than the current solution I think, you get rid of cpu-map >> dependency (honestly it is a bit weird what you did, with cpu-map >> containing a cluster phandle to a power domain and cpu phandles to >> cpu nodes, and the DT bindings you posted does not seem to match your >> dts). >> >> For PSCI, nothing prevents you from defining the same bindings except >> that the power domain(s) is not explicitly controlled by the OS, still, >> the DT would always describe the HW and you can use it to detect the >> power domain topology and which cpus are part of a given power domain. >> >> Please let me know what you think, thanks. > >+1 > I have a version written up that builds up the hierarchy from the CPU nodes. It doesnt use cpu-map. Will be part of the next submission. Thanks, Lina -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/base/power/cpu-pd.c b/drivers/base/power/cpu-pd.c index e331ae6..2872c18 100644 --- a/drivers/base/power/cpu-pd.c +++ b/drivers/base/power/cpu-pd.c @@ -429,3 +429,79 @@ int of_attach_cpu_pm_domain(struct device_node *dn) return 0; } EXPORT_SYMBOL(of_attach_cpu_pm_domain); + +static int of_parse_cpu_pd(struct device_node *cluster, + const struct cpu_pd_ops *ops) +{ + struct device_node *domain_node; + struct generic_pm_domain *genpd; + char name[10]; + struct device_node *c; + int i, ret; + + for (i = 0; ; i++) { + snprintf(name, sizeof(name), "cluster%d", i); + c = of_get_child_by_name(cluster, name); + if (!c) + break; + + domain_node = of_parse_phandle(c, "cluster", 0); + if (!domain_node) + return -1; + + /* Initialize CPU PM domain domain at this level */ + genpd = of_init_cpu_pm_domain(domain_node, ops); + if (IS_ERR(genpd)) + return -1; + + /* Initialize and attach child domains */ + ret = of_parse_cpu_pd(c, ops); + + /* + * Attach the domain to its parent after reading + * the children, so the mask of CPUs in this domain + * are setup correctly. + */ + if (!ret) + of_attach_cpu_pm_domain(domain_node); + + of_node_put(c); + if (ret != 0) + return ret; + } + + return 0; +} + +/** + * of_setup_cpu_domain_topology() - Setup the CPU domains from the CPU + * topology node in DT. + * + * @ops: The PM domain suspend/resume ops for all the domains in the topology + */ +int of_setup_cpu_domain_topology(const struct cpu_pd_ops *ops) +{ + struct device_node *cn, *map; + int ret = 0; + + cn = of_find_node_by_path("/cpus"); + if (!cn) { + pr_err("No CPU information found in DT\n"); + return 0; + } + + map = of_get_child_by_name(cn, "cpu-map"); + if (!map) + goto out; + + ret = of_parse_cpu_pd(map, ops); + if (ret != 0) + goto out_map; + +out_map: + of_node_put(map); +out: + of_node_put(cn); + return ret; +} +EXPORT_SYMBOL(of_setup_cpu_domain_topology); diff --git a/include/linux/cpu-pd.h b/include/linux/cpu-pd.h index 489ee2f..e8290db 100644 --- a/include/linux/cpu-pd.h +++ b/include/linux/cpu-pd.h @@ -32,4 +32,5 @@ struct cpu_pm_domain { struct generic_pm_domain *of_init_cpu_pm_domain(struct device_node *dn, const struct cpu_pd_ops *ops); int of_attach_cpu_pm_domain(struct device_node *dn); +int of_setup_cpu_domain_topology(const struct cpu_pd_ops *ops); #endif /* __CPU_PD_H__ */
Architectures that support CPU domain control in the firmware specify the domain heirarchy as part of the topology nodes. Parse and initialize domains from the topology node for such architectures. Cc: Rob Herring <robherring2@gmail.com> Cc: Stephen Boyd <sboyd@codeaurora.org> Cc: Kevin Hilman <khilman@linaro.org> Cc: Ulf Hansson <ulf.hansson@linaro.org> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Signed-off-by: Lina Iyer <lina.iyer@linaro.org> --- drivers/base/power/cpu-pd.c | 76 +++++++++++++++++++++++++++++++++++++++++++++ include/linux/cpu-pd.h | 1 + 2 files changed, 77 insertions(+)