From: Sudeep Holla <sudeep.holla@arm.com> To: Ulf Hansson <ulf.hansson@linaro.org> Cc: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>, Mark Rutland <mark.rutland@arm.com>, Linux ARM <linux-arm-kernel@lists.infradead.org>, "Rafael J . Wysocki" <rjw@rjwysocki.net>, Daniel Lezcano <daniel.lezcano@linaro.org>, "Raju P . L . S . S . S . N" <rplsssn@codeaurora.org>, Amit Kucheria <amit.kucheria@linaro.org>, Bjorn Andersson <bjorn.andersson@linaro.org>, Stephen Boyd <sboyd@kernel.org>, Niklas Cassel <niklas.cassel@linaro.org>, Tony Lindgren <tony@atomide.com>, Kevin Hilman <khilman@kernel.org>, Lina Iyer <ilina@codeaurora.org>, Viresh Kumar <viresh.kumar@linaro.org>, Vincent Guittot <vincent.guittot@linaro.org>, Geert Uytterhoeven <geert+renesas@glider.be>, Souvik Chakravarty <souvik.chakravarty@arm.com>, Sudeep Holla <sudeep.holla@arm.com>, Linux PM <linux-pm@vger.kernel.org>, linux-arm-msm <linux-arm-msm@vger.kernel.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Lina Iyer <lina.iyer@linaro.org> Subject: Re: [PATCH 09/18] drivers: firmware: psci: Add support for PM domains using genpd Date: Mon, 10 Jun 2019 11:59:03 +0100 [thread overview] Message-ID: <20190610105903.GC26602@e107155-lin> (raw) In-Reply-To: <CAPDyKFq3FFZEAEKrPfvBPUpAGKaTo05zS0-5sfgBjGFhRZ0b=w@mail.gmail.com> On Mon, Jun 10, 2019 at 12:21:41PM +0200, Ulf Hansson wrote: > On Fri, 7 Jun 2019 at 17:27, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Mon, May 13, 2019 at 09:22:51PM +0200, Ulf Hansson wrote: > > > When the hierarchical CPU topology layout is used in DT, we need to setup > > > the corresponding PM domain data structures, as to allow a CPU and a group > > > of CPUs to be power managed accordingly. Let's enable this by deploying > > > support through the genpd interface. > > > > > > Additionally, when the OS initiated mode is supported by the PSCI FW, let's > > > also parse the domain idle states DT bindings as to make genpd responsible > > > for the state selection, when the states are compatible with > > > "domain-idle-state". Otherwise, when only Platform Coordinated mode is > > > supported, we rely solely on the state selection to be managed through the > > > regular cpuidle framework. > > > > > > If the initialization of the PM domain data structures succeeds and the OS > > > initiated mode is supported, we try to switch to it. In case it fails, > > > let's fall back into a degraded mode, rather than bailing out and returning > > > an error code. > > > > > > Due to that the OS initiated mode may become enabled, we need to adjust to > > > maintain backwards compatibility for a kernel started through a kexec call. > > > Do this by explicitly switch to Platform Coordinated mode during boot. > > > > > > Finally, the actual initialization of the PM domain data structures, is > > > done via calling the new shared function, psci_dt_init_pm_domains(). > > > However, this is implemented by subsequent changes. > > > > > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org> > > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > --- > > > > > > Changes: > > > - Simplify code setting domain_state at power off. > > > - Use the genpd ->free_state() callback to manage freeing of states. > > > - Fixup a bogus while loop. > > > > > > --- > > > drivers/firmware/psci/Makefile | 2 +- > > > drivers/firmware/psci/psci.c | 7 +- > > > drivers/firmware/psci/psci.h | 5 + > > > drivers/firmware/psci/psci_pm_domain.c | 268 +++++++++++++++++++++++++ > > > 4 files changed, 280 insertions(+), 2 deletions(-) > > > create mode 100644 drivers/firmware/psci/psci_pm_domain.c > > > [...] > > > + > > > +static int psci_pd_parse_states(struct device_node *np, > > > + struct genpd_power_state **states, int *state_count) > > > +{ > > > + int ret; > > > + > > > + /* Parse the domain idle states. */ > > > + ret = of_genpd_parse_idle_states(np, states, state_count); > > > + if (ret) > > > + return ret; > > > + > > > > > > Lots of things here in this file are not psci specific. They can be > > moved as generic CPU PM domain support. > > What exactly do you mean by CPU PM domain support? > > The current split is based upon how the generic PM domain (genpd) > supports CPU devices (see GENPD_FLAG_CPU_DOMAIN), which is already > available. > > I agree that finding the right balance between what can be made > generic and driver specific is not always obvious. Often it's better > to start with having more things in the driver code, then move things > into a common framework, later on, when that turns out to make sense. > Indeed, I agree. But when reviewing this time I thought it should be possible to push generic stuff into existing dt_idle_driver. I must admit that I haven't thought much in details, just thought of expressing the idea and see. But yes it's difficult to find the balance but at the same time we need reasons to have these in psci :) > > > > > + /* Fill out the PSCI specifics for each found state. */ > > > + ret = psci_pd_parse_state_nodes(*states, *state_count); > > > + if (ret) > > > + kfree(*states); > > > + > > > > Things like above are PSCI. > > > > I am trying to see if we can do something to achieve partitions like we > > have today: psci.c just has PSCI specific stuff and dt_idle_states.c > > deals with generic idle stuff. > > I am open to any suggestions. Although, I am not sure I understand > your comment and nor the reason to why you want me to change. > > So, what is the problem with having the code that you refer to, inside > drivers/firmware/psci/psci_pm_domain.c? Can't we just start with that > and see how it plays? > I need to think how to partition this well. I don't have suggestions right away, but I need to get convinced what we have here is best we can do or come up with a better solution. I didn't like it as is at this time. -- Regards, Sudeep
WARNING: multiple messages have this Message-ID (diff)
From: Sudeep Holla <sudeep.holla@arm.com> To: Ulf Hansson <ulf.hansson@linaro.org> Cc: Mark Rutland <mark.rutland@arm.com>, Geert Uytterhoeven <geert+renesas@glider.be>, Tony Lindgren <tony@atomide.com>, Viresh Kumar <viresh.kumar@linaro.org>, Bjorn Andersson <bjorn.andersson@linaro.org>, Lina Iyer <ilina@codeaurora.org>, Lina Iyer <lina.iyer@linaro.org>, Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>, Kevin Hilman <khilman@kernel.org>, Daniel Lezcano <daniel.lezcano@linaro.org>, "Raju P . L . S . S . S . N" <rplsssn@codeaurora.org>, Souvik Chakravarty <souvik.chakravarty@arm.com>, Linux PM <linux-pm@vger.kernel.org>, linux-arm-msm <linux-arm-msm@vger.kernel.org>, Linux ARM <linux-arm-kernel@lists.infradead.org>, Stephen Boyd <sboyd@kernel.org>, "Rafael J . Wysocki" <rjw@rjwysocki.net>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Amit Kucheria <amit.kucheria@linaro.org>, Sudeep Holla <sudeep.holla@arm.com>, Niklas Cassel <niklas.cassel@linaro.org> Subject: Re: [PATCH 09/18] drivers: firmware: psci: Add support for PM domains using genpd Date: Mon, 10 Jun 2019 11:59:03 +0100 [thread overview] Message-ID: <20190610105903.GC26602@e107155-lin> (raw) In-Reply-To: <CAPDyKFq3FFZEAEKrPfvBPUpAGKaTo05zS0-5sfgBjGFhRZ0b=w@mail.gmail.com> On Mon, Jun 10, 2019 at 12:21:41PM +0200, Ulf Hansson wrote: > On Fri, 7 Jun 2019 at 17:27, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Mon, May 13, 2019 at 09:22:51PM +0200, Ulf Hansson wrote: > > > When the hierarchical CPU topology layout is used in DT, we need to setup > > > the corresponding PM domain data structures, as to allow a CPU and a group > > > of CPUs to be power managed accordingly. Let's enable this by deploying > > > support through the genpd interface. > > > > > > Additionally, when the OS initiated mode is supported by the PSCI FW, let's > > > also parse the domain idle states DT bindings as to make genpd responsible > > > for the state selection, when the states are compatible with > > > "domain-idle-state". Otherwise, when only Platform Coordinated mode is > > > supported, we rely solely on the state selection to be managed through the > > > regular cpuidle framework. > > > > > > If the initialization of the PM domain data structures succeeds and the OS > > > initiated mode is supported, we try to switch to it. In case it fails, > > > let's fall back into a degraded mode, rather than bailing out and returning > > > an error code. > > > > > > Due to that the OS initiated mode may become enabled, we need to adjust to > > > maintain backwards compatibility for a kernel started through a kexec call. > > > Do this by explicitly switch to Platform Coordinated mode during boot. > > > > > > Finally, the actual initialization of the PM domain data structures, is > > > done via calling the new shared function, psci_dt_init_pm_domains(). > > > However, this is implemented by subsequent changes. > > > > > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org> > > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > --- > > > > > > Changes: > > > - Simplify code setting domain_state at power off. > > > - Use the genpd ->free_state() callback to manage freeing of states. > > > - Fixup a bogus while loop. > > > > > > --- > > > drivers/firmware/psci/Makefile | 2 +- > > > drivers/firmware/psci/psci.c | 7 +- > > > drivers/firmware/psci/psci.h | 5 + > > > drivers/firmware/psci/psci_pm_domain.c | 268 +++++++++++++++++++++++++ > > > 4 files changed, 280 insertions(+), 2 deletions(-) > > > create mode 100644 drivers/firmware/psci/psci_pm_domain.c > > > [...] > > > + > > > +static int psci_pd_parse_states(struct device_node *np, > > > + struct genpd_power_state **states, int *state_count) > > > +{ > > > + int ret; > > > + > > > + /* Parse the domain idle states. */ > > > + ret = of_genpd_parse_idle_states(np, states, state_count); > > > + if (ret) > > > + return ret; > > > + > > > > > > Lots of things here in this file are not psci specific. They can be > > moved as generic CPU PM domain support. > > What exactly do you mean by CPU PM domain support? > > The current split is based upon how the generic PM domain (genpd) > supports CPU devices (see GENPD_FLAG_CPU_DOMAIN), which is already > available. > > I agree that finding the right balance between what can be made > generic and driver specific is not always obvious. Often it's better > to start with having more things in the driver code, then move things > into a common framework, later on, when that turns out to make sense. > Indeed, I agree. But when reviewing this time I thought it should be possible to push generic stuff into existing dt_idle_driver. I must admit that I haven't thought much in details, just thought of expressing the idea and see. But yes it's difficult to find the balance but at the same time we need reasons to have these in psci :) > > > > > + /* Fill out the PSCI specifics for each found state. */ > > > + ret = psci_pd_parse_state_nodes(*states, *state_count); > > > + if (ret) > > > + kfree(*states); > > > + > > > > Things like above are PSCI. > > > > I am trying to see if we can do something to achieve partitions like we > > have today: psci.c just has PSCI specific stuff and dt_idle_states.c > > deals with generic idle stuff. > > I am open to any suggestions. Although, I am not sure I understand > your comment and nor the reason to why you want me to change. > > So, what is the problem with having the code that you refer to, inside > drivers/firmware/psci/psci_pm_domain.c? Can't we just start with that > and see how it plays? > I need to think how to partition this well. I don't have suggestions right away, but I need to get convinced what we have here is best we can do or come up with a better solution. I didn't like it as is at this time. -- Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-06-10 10:59 UTC|newest] Thread overview: 155+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-05-13 19:22 [PATCH 00/18] ARM/ARM64: Support hierarchical CPU arrangement for PSCI Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-05-13 19:22 ` [PATCH 01/18] dt: psci: Update DT bindings to support hierarchical PSCI states Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-07-19 11:29 ` Lorenzo Pieralisi 2019-07-19 11:29 ` Lorenzo Pieralisi 2019-05-13 19:22 ` [PATCH 02/18] of: base: Add of_get_cpu_state_node() to get idle states for a CPU node Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-05-13 19:22 ` [PATCH 03/18] cpuidle: dt: Support hierarchical CPU idle states Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-05-13 19:22 ` [PATCH 04/18] ARM/ARM64: cpuidle: Let back-end init ops take the driver as input Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-06-07 15:00 ` Sudeep Holla 2019-06-07 15:00 ` Sudeep Holla 2019-06-10 10:20 ` Ulf Hansson 2019-06-10 10:20 ` Ulf Hansson 2019-05-13 19:22 ` [PATCH 05/18] drivers: firmware: psci: Simplify state node parsing Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-06-07 15:01 ` Sudeep Holla 2019-06-07 15:01 ` Sudeep Holla 2019-05-13 19:22 ` [PATCH 06/18] drivers: firmware: psci: Support hierarchical CPU idle states Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-06-07 15:03 ` Sudeep Holla 2019-06-07 15:03 ` Sudeep Holla 2019-05-13 19:22 ` [PATCH 07/18] drivers: firmware: psci: Prepare to use OS initiated suspend mode Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-06-07 15:17 ` Sudeep Holla 2019-06-07 15:17 ` Sudeep Holla 2019-06-10 10:21 ` Ulf Hansson 2019-06-10 10:21 ` Ulf Hansson 2019-06-10 10:42 ` Sudeep Holla 2019-06-10 10:42 ` Sudeep Holla 2019-07-16 14:53 ` Sudeep Holla 2019-07-16 14:53 ` Sudeep Holla 2019-05-13 19:22 ` [PATCH 08/18] drivers: firmware: psci: Prepare to support PM domains Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-06-07 15:21 ` Sudeep Holla 2019-06-07 15:21 ` Sudeep Holla 2019-05-13 19:22 ` [PATCH 09/18] drivers: firmware: psci: Add support for PM domains using genpd Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-06-07 15:27 ` Sudeep Holla 2019-06-07 15:27 ` Sudeep Holla 2019-06-10 10:21 ` Ulf Hansson 2019-06-10 10:21 ` Ulf Hansson 2019-06-10 10:59 ` Sudeep Holla [this message] 2019-06-10 10:59 ` Sudeep Holla 2019-07-16 15:05 ` Sudeep Holla 2019-07-16 15:05 ` Sudeep Holla 2019-07-18 11:04 ` Ulf Hansson 2019-07-18 11:04 ` Ulf Hansson 2019-07-18 13:19 ` Sudeep Holla 2019-07-18 13:19 ` Sudeep Holla 2019-07-18 17:57 ` Lina Iyer 2019-07-18 17:57 ` Lina Iyer 2019-07-19 9:45 ` Sudeep Holla 2019-07-19 9:45 ` Sudeep Holla 2019-05-13 19:22 ` [PATCH 10/18] drivers: firmware: psci: Add hierarchical domain idle states converter Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-07-09 15:31 ` Lorenzo Pieralisi 2019-07-09 15:31 ` Lorenzo Pieralisi 2019-07-16 8:45 ` Ulf Hansson 2019-07-16 8:45 ` Ulf Hansson 2019-07-16 14:51 ` Lorenzo Pieralisi 2019-07-16 14:51 ` Lorenzo Pieralisi 2019-07-18 11:43 ` Ulf Hansson 2019-07-18 11:43 ` Ulf Hansson 2019-07-18 13:36 ` Lorenzo Pieralisi 2019-07-18 13:36 ` Lorenzo Pieralisi 2019-05-13 19:22 ` [PATCH 11/18] drivers: firmware: psci: Introduce psci_dt_topology_init() Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-05-13 19:22 ` [PATCH 12/18] drivers: firmware: psci: Add a helper to attach a CPU to its PM domain Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-05-13 19:22 ` [PATCH 13/18] drivers: firmware: psci: Attach the CPU's device " Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-05-13 19:22 ` [PATCH 14/18] drivers: firmware: psci: Manage runtime PM in the idle path for CPUs Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-07-16 15:53 ` Lorenzo Pieralisi 2019-07-16 15:53 ` Lorenzo Pieralisi 2019-07-18 10:35 ` Ulf Hansson 2019-07-18 10:35 ` Ulf Hansson 2019-07-18 13:30 ` Lorenzo Pieralisi 2019-07-18 13:30 ` Lorenzo Pieralisi 2019-07-18 16:54 ` Ulf Hansson 2019-07-18 16:54 ` Ulf Hansson 2019-07-18 17:41 ` Lina Iyer 2019-07-18 17:41 ` Lina Iyer 2019-07-18 21:49 ` Ulf Hansson 2019-07-18 21:49 ` Ulf Hansson 2019-07-19 10:02 ` Lorenzo Pieralisi 2019-07-19 10:02 ` Lorenzo Pieralisi 2019-05-13 19:22 ` [PATCH 15/18] drivers: firmware: psci: Support CPU hotplug for the hierarchical model Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-06-07 15:31 ` Sudeep Holla 2019-06-07 15:31 ` Sudeep Holla 2019-06-10 10:21 ` Ulf Hansson 2019-06-10 10:21 ` Ulf Hansson 2019-06-10 11:02 ` Sudeep Holla 2019-06-10 11:02 ` Sudeep Holla 2019-05-13 19:22 ` [PATCH 16/18] arm64: kernel: Respect the hierarchical CPU topology in DT for PSCI Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-05-13 19:22 ` [PATCH 17/18] arm64: dts: Convert to the hierarchical CPU topology layout for MSM8916 Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-05-13 19:22 ` Ulf Hansson 2019-07-16 14:47 ` Sudeep Holla 2019-07-16 14:47 ` Sudeep Holla 2019-07-16 20:36 ` Lina Iyer 2019-07-16 20:36 ` Lina Iyer 2019-07-17 17:18 ` Sudeep Holla 2019-07-17 17:18 ` Sudeep Holla 2019-05-13 19:23 ` [PATCH 18/18] arm64: dts: hikey: Convert to the hierarchical CPU topology layout Ulf Hansson 2019-05-13 19:23 ` Ulf Hansson 2019-05-13 19:23 ` Ulf Hansson 2019-07-16 14:47 ` Sudeep Holla 2019-07-16 14:47 ` Sudeep Holla 2019-07-18 10:48 ` Ulf Hansson 2019-07-18 10:48 ` Ulf Hansson 2019-07-18 13:11 ` Sudeep Holla 2019-07-18 13:11 ` Sudeep Holla 2019-05-14 8:08 ` [PATCH 00/18] ARM/ARM64: Support hierarchical CPU arrangement for PSCI Rafael J. Wysocki 2019-05-14 8:08 ` Rafael J. Wysocki 2019-05-14 8:08 ` Rafael J. Wysocki 2019-05-14 8:58 ` Ulf Hansson 2019-05-14 8:58 ` Ulf Hansson 2019-05-14 8:58 ` Ulf Hansson 2019-06-07 15:42 ` Sudeep Holla 2019-06-07 15:42 ` Sudeep Holla 2019-06-07 19:34 ` Bjorn Andersson 2019-06-07 19:34 ` Bjorn Andersson 2019-06-10 10:32 ` Sudeep Holla 2019-06-10 10:32 ` Sudeep Holla 2019-06-10 15:54 ` Ulf Hansson 2019-06-10 15:54 ` Ulf Hansson 2019-06-10 17:16 ` Lorenzo Pieralisi 2019-06-10 17:16 ` Lorenzo Pieralisi 2019-06-10 18:57 ` Ulf Hansson 2019-06-10 18:57 ` Ulf Hansson 2019-06-18 11:56 ` Ulf Hansson 2019-06-18 11:56 ` Ulf Hansson 2019-06-07 11:19 ` Ulf Hansson 2019-06-07 11:19 ` Ulf Hansson
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190610105903.GC26602@e107155-lin \ --to=sudeep.holla@arm.com \ --cc=Lorenzo.Pieralisi@arm.com \ --cc=amit.kucheria@linaro.org \ --cc=bjorn.andersson@linaro.org \ --cc=daniel.lezcano@linaro.org \ --cc=geert+renesas@glider.be \ --cc=ilina@codeaurora.org \ --cc=khilman@kernel.org \ --cc=lina.iyer@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=niklas.cassel@linaro.org \ --cc=rjw@rjwysocki.net \ --cc=rplsssn@codeaurora.org \ --cc=sboyd@kernel.org \ --cc=souvik.chakravarty@arm.com \ --cc=tony@atomide.com \ --cc=ulf.hansson@linaro.org \ --cc=vincent.guittot@linaro.org \ --cc=viresh.kumar@linaro.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.