| Message ID | 1476467276-75094-4-git-send-email-lina.iyer@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 14/10/16 18:47, Lina Iyer wrote: > This patch allows domains to define idle states in the DT. SoC's can > define domain idle states in DT using the "domain-idle-states" property > of the domain provider. Add API to read the idle states from DT that can > be set in the genpd object. > > This patch is based on the original patch by Marc Titinger. > > Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > --- > drivers/base/power/domain.c | 94 +++++++++++++++++++++++++++++++++++++++++++++ > include/linux/pm_domain.h | 8 ++++ > 2 files changed, 102 insertions(+) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 37ab7f1..9af75ba 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1916,6 +1916,100 @@ out: > return ret ? -EPROBE_DEFER : 0; > } > EXPORT_SYMBOL_GPL(genpd_dev_pm_attach); > + > +static const struct of_device_id idle_state_match[] = { > + { .compatible = "arm,idle-state", }, > + { } > +}; > + I still think it's better to have another compatible to serve this purpose. We don't want to end up creating genpd domains just because they are "arm,idle-state" compatible IMO ? I agree you can prevent it checking for OSC mode support in the firmware. But I want to understand if you have any strong reasons for avoiding that approach.
Hi Sudeep, On Mon, Oct 24 2016 at 07:39 -0600, Sudeep Holla wrote: > > >On 14/10/16 18:47, Lina Iyer wrote: >>This patch allows domains to define idle states in the DT. SoC's can >>define domain idle states in DT using the "domain-idle-states" property >>of the domain provider. Add API to read the idle states from DT that can >>be set in the genpd object. >> >>This patch is based on the original patch by Marc Titinger. >> >>Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com> >>Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >>Signed-off-by: Lina Iyer <lina.iyer@linaro.org> >>--- >> drivers/base/power/domain.c | 94 +++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/pm_domain.h | 8 ++++ >> 2 files changed, 102 insertions(+) >> >>diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c >>index 37ab7f1..9af75ba 100644 >>--- a/drivers/base/power/domain.c >>+++ b/drivers/base/power/domain.c >>@@ -1916,6 +1916,100 @@ out: >> return ret ? -EPROBE_DEFER : 0; >> } >> EXPORT_SYMBOL_GPL(genpd_dev_pm_attach); >>+ >>+static const struct of_device_id idle_state_match[] = { >>+ { .compatible = "arm,idle-state", }, >>+ { } >>+}; >>+ > >I still think it's better to have another compatible to serve this >purpose. We don't want to end up creating genpd domains just because >they are "arm,idle-state" compatible IMO ? > >I agree you can prevent it checking for OSC mode support in the >firmware. But I want to understand if you have any strong reasons for >avoiding that approach. > Why are you still held up with OSI/PC PSCI modes? I repeat again this series is not about any of that, it is just about PM domains. PM domains have idle states and the idle-state description is similar in definition to arm,idle-state and therefore uses the same compatible. There is no point re-defining something that already exists in the kernel. I was able to find the original thread, where we discussed this [1]. I suggest, you read about PM domains and its idle states and understand this series in the context of PM domains. Thanks, Lina [1]. http://www.serverphorums.com/read.php?12,1303996
On 24/10/16 17:48, Lina Iyer wrote: > Hi Sudeep, > > On Mon, Oct 24 2016 at 07:39 -0600, Sudeep Holla wrote: >> >> >> On 14/10/16 18:47, Lina Iyer wrote: >>> This patch allows domains to define idle states in the DT. SoC's can >>> define domain idle states in DT using the "domain-idle-states" property >>> of the domain provider. Add API to read the idle states from DT that can >>> be set in the genpd object. >>> >>> This patch is based on the original patch by Marc Titinger. >>> >>> Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com> >>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org> >>> --- >>> drivers/base/power/domain.c | 94 >>> +++++++++++++++++++++++++++++++++++++++++++++ >>> include/linux/pm_domain.h | 8 ++++ >>> 2 files changed, 102 insertions(+) >>> >>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c >>> index 37ab7f1..9af75ba 100644 >>> --- a/drivers/base/power/domain.c >>> +++ b/drivers/base/power/domain.c >>> @@ -1916,6 +1916,100 @@ out: >>> return ret ? -EPROBE_DEFER : 0; >>> } >>> EXPORT_SYMBOL_GPL(genpd_dev_pm_attach); >>> + >>> +static const struct of_device_id idle_state_match[] = { >>> + { .compatible = "arm,idle-state", }, >>> + { } >>> +}; >>> + >> >> I still think it's better to have another compatible to serve this >> purpose. We don't want to end up creating genpd domains just because >> they are "arm,idle-state" compatible IMO ? >> >> I agree you can prevent it checking for OSC mode support in the >> firmware. But I want to understand if you have any strong reasons for >> avoiding that approach. >> > Why are you still held up with OSI/PC PSCI modes? I am just pointing that out to make sure you are not defining these DT bindings with just QCOM platform and OSC in consideration. I am thinking about how can it be used/extended in other use-cases. > I repeat again this > series is not about any of that, it is just about PM domains. I completely understand that, no argument on that. What I worry is that if a system(in future) represents this power domains and domain idles states and doesn't want to create a genpd, do we have to handle it differently or even the way your CPUidle series would do or can we say those states need to specify that they are compatible with the new feature(the one being added in this series) with a new compatible. I prefer the latter than the former to avoid all possible workarounds in future. > PM domains > have idle states and the idle-state description is similar in definition > to arm,idle-state and therefore uses the same compatible. There is no > point re-defining something that already exists in the kernel. > Yes I understand that but "arm,idle-states" were not defined with this feature in mind and hence I am bit worried if it could cause any issue especially if deprecate cpu-idle-states and move to this model completely. I really don't like this mix and hence I am raising the concern here. I am trying to ease that migration. > I was able to find the original thread, where we discussed this [1]. > > I suggest, you read about PM domains and its idle states and understand > this series in the context of PM domains. > Sure, will do that. Thanks for pointing that out. But the concern I am raising is entirely different. I am asking if this re-use will cause any issue in future as pointed out above. I re-iterate that I understand this series is independent of the CPUIdle and hence asking why not make it completely independent by just adding the new compatible. I am *not asking to redefine something completely*. What I am saying is *just* to add new compatible that may(for cpu devices)/may not(for other/non-CPU devices) be used along with "arm,idle-state". I may be too paranoid here but I think that's safer. It helps to skip creating of genpd if required for some domains as I had explained before.
On 24/10/16 17:48, Lina Iyer wrote: > Hi Sudeep, [..] > > I was able to find the original thread, where we discussed this [1]. > > [1]. http://www.serverphorums.com/read.php?12,1303996 > I went through the discussion and that's valid discussion and I agree. But what am looking for is backward compatibility and not exactly same as the discussion and I agree that can be solved in different way. I just feel that's one easily way to solve such issues. I am open to suggestions.
On Mon, Oct 24 2016 at 11:27 -0600, Sudeep Holla wrote: > > >On 24/10/16 17:48, Lina Iyer wrote: >>Hi Sudeep, >> >>On Mon, Oct 24 2016 at 07:39 -0600, Sudeep Holla wrote: >>> >>> >>>On 14/10/16 18:47, Lina Iyer wrote: >>>>This patch allows domains to define idle states in the DT. SoC's can >>>>define domain idle states in DT using the "domain-idle-states" property >>>>of the domain provider. Add API to read the idle states from DT that can >>>>be set in the genpd object. >>>> >>>>This patch is based on the original patch by Marc Titinger. >>>> >>>>Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com> >>>>Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >>>>Signed-off-by: Lina Iyer <lina.iyer@linaro.org> >>>>--- >>>>drivers/base/power/domain.c | 94 >>>>+++++++++++++++++++++++++++++++++++++++++++++ >>>>include/linux/pm_domain.h | 8 ++++ >>>>2 files changed, 102 insertions(+) >>>> >>>>diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c >>>>index 37ab7f1..9af75ba 100644 >>>>--- a/drivers/base/power/domain.c >>>>+++ b/drivers/base/power/domain.c >>>>@@ -1916,6 +1916,100 @@ out: >>>> return ret ? -EPROBE_DEFER : 0; >>>>} >>>>EXPORT_SYMBOL_GPL(genpd_dev_pm_attach); >>>>+ >>>>+static const struct of_device_id idle_state_match[] = { >>>>+ { .compatible = "arm,idle-state", }, >>>>+ { } >>>>+}; >>>>+ >>> >>>I still think it's better to have another compatible to serve this >>>purpose. We don't want to end up creating genpd domains just because >>>they are "arm,idle-state" compatible IMO ? >>> >>>I agree you can prevent it checking for OSC mode support in the >>>firmware. But I want to understand if you have any strong reasons for >>>avoiding that approach. >>> >>Why are you still held up with OSI/PC PSCI modes? > >I am just pointing that out to make sure you are not defining these >DT bindings with just QCOM platform and OSC in consideration. > >I am thinking about how can it be used/extended in other use-cases. > >>I repeat again this >>series is not about any of that, it is just about PM domains. > >I completely understand that, no argument on that. What I worry is that >if a system(in future) represents this power domains and domain idles >states and doesn't want to create a genpd, do we have to handle it >differently or even the way your CPUidle series would do or can we say >those states need to specify that they are compatible with the new >feature(the one being added in this series) with a new compatible. >I prefer the latter than the former to avoid all possible workarounds >in future. > >>PM domains >>have idle states and the idle-state description is similar in definition >>to arm,idle-state and therefore uses the same compatible. There is no >>point re-defining something that already exists in the kernel. >> > >Yes I understand that but "arm,idle-states" were not defined with this >feature in mind and hence I am bit worried if it could cause any issue >especially if deprecate cpu-idle-states and move to this model >completely. I really don't like this mix and hence I am raising the >concern here. I am trying to ease that migration. > >>I was able to find the original thread, where we discussed this [1]. >> >>I suggest, you read about PM domains and its idle states and understand >>this series in the context of PM domains. >> > >Sure, will do that. Thanks for pointing that out. But the concern I am >raising is entirely different. I am asking if this re-use will cause any >issue in future as pointed out above. > >I re-iterate that I understand this series is independent of the CPUIdle >and hence asking why not make it completely independent by just adding >the new compatible. > >I am *not asking to redefine something completely*. What I am saying is >*just* to add new compatible that may(for cpu devices)/may not(for >other/non-CPU devices) be used along with "arm,idle-state". > I am fine with that, as long as the compatible string is an alternate for "arm,idle-state" it shouldn't be a problem. Any recommendations? Thanks, Lina >I may be too paranoid here but I think that's safer. It helps to skip >creating of genpd if required for some domains as I had explained before. > >-- >Regards, >Sudeep
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 37ab7f1..9af75ba 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -1916,6 +1916,100 @@ out: return ret ? -EPROBE_DEFER : 0; } EXPORT_SYMBOL_GPL(genpd_dev_pm_attach); + +static const struct of_device_id idle_state_match[] = { + { .compatible = "arm,idle-state", }, + { } +}; + +static int genpd_parse_state(struct genpd_power_state *genpd_state, + struct device_node *state_node) +{ + int err; + u32 residency; + u32 entry_latency, exit_latency; + const struct of_device_id *match_id; + + match_id = of_match_node(idle_state_match, state_node); + if (!match_id) + return -EINVAL; + + err = of_property_read_u32(state_node, "entry-latency-us", + &entry_latency); + if (err) { + pr_debug(" * %s missing entry-latency-us property\n", + state_node->full_name); + return -EINVAL; + } + + err = of_property_read_u32(state_node, "exit-latency-us", + &exit_latency); + if (err) { + pr_debug(" * %s missing exit-latency-us property\n", + state_node->full_name); + return -EINVAL; + } + + err = of_property_read_u32(state_node, "min-residency-us", &residency); + if (!err) + genpd_state->residency_ns = 1000 * residency; + + genpd_state->power_on_latency_ns = 1000 * exit_latency; + genpd_state->power_off_latency_ns = 1000 * entry_latency; + + return 0; +} + +/** + * of_genpd_parse_idle_states: Return array of idle states for the genpd. + * + * @dn: The genpd device node + * @states: The pointer to which the state array will be saved. + * @n: The count of elements in the array returned from this function. + * + * Returns the device states parsed from the OF node. The memory for the states + * is allocated by this function and is the responsibility of the caller to + * free the memory after use. + */ +int of_genpd_parse_idle_states(struct device_node *dn, + struct genpd_power_state **states, int *n) +{ + struct genpd_power_state *st; + struct device_node *np; + int i = 0; + int err, ret; + int count; + struct of_phandle_iterator it; + + count = of_count_phandle_with_args(dn, "domain-idle-states", NULL); + if (!count) + return -EINVAL; + + st = kcalloc(count, sizeof(*st), GFP_KERNEL); + if (!st) + return -ENOMEM; + + /* Loop over the phandles until all the requested entry is found */ + of_for_each_phandle(&it, err, dn, "domain-idle-states", NULL, 0) { + np = it.node; + ret = genpd_parse_state(&st[i++], np); + if (ret) { + pr_err + ("Parsing idle state node %s failed with err %d\n", + np->full_name, ret); + of_node_put(np); + kfree(st); + return ret; + } + } + + *n = count; + *states = st; + + return 0; +} +EXPORT_SYMBOL_GPL(of_genpd_parse_idle_states); + #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */ diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index f4492eb..b489496 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -205,6 +205,8 @@ extern int of_genpd_add_device(struct of_phandle_args *args, extern int of_genpd_add_subdomain(struct of_phandle_args *parent, struct of_phandle_args *new_subdomain); extern struct generic_pm_domain *of_genpd_remove_last(struct device_node *np); +extern int of_genpd_parse_idle_states(struct device_node *dn, + struct genpd_power_state **states, int *n); int genpd_dev_pm_attach(struct device *dev); #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */ @@ -234,6 +236,12 @@ static inline int of_genpd_add_subdomain(struct of_phandle_args *parent, return -ENODEV; } +static inline int of_genpd_parse_idle_states(struct device_node *dn, + struct genpd_power_state **states, int *n) +{ + return -ENODEV; +} + static inline int genpd_dev_pm_attach(struct device *dev) { return -ENODEV;