Re: [PATCH v2 10/13] cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains

From: Ulf Hansson <ulf.hansson@linaro.org>
To: Sudeep Holla <sudeep.holla@arm.com>
Cc: "Rafael J . Wysocki" <rjw@rjwysocki.net>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Lina Iyer <ilina@codeaurora.org>,
	Linux PM <linux-pm@vger.kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Stephen Boyd <sboyd@kernel.org>, Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Kevin Hilman <khilman@kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	Lina Iyer <lina.iyer@linaro.org>
Subject: Re: [PATCH v2 10/13] cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains
Date: Mon, 18 Nov 2019 14:37:41 +0100	[thread overview]
Message-ID: <CAPDyKFraEhFVm27YG0fVkjT0-oBBxFpfiBY4zS+1TMy=0F6GRQ@mail.gmail.com> (raw)
In-Reply-To: <20191115173053.GE27170@bogus>

On Fri, 15 Nov 2019 at 18:30, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Tue, Oct 29, 2019 at 05:44:35PM +0100, Ulf Hansson wrote:
> > The per CPU variable psci_power_state, contains an array of fixed values,
> > which reflects the corresponding arm,psci-suspend-param parsed from DT, for
> > each of the available CPU idle states.
> >
> > This isn't sufficient when using the hierarchical CPU topology in DT, in
> > combination with having PSCI OS initiated (OSI) mode enabled. More
> > precisely, in OSI mode, Linux is responsible of telling the PSCI FW what
> > idle state the cluster (a group of CPUs) should enter, while in PSCI
> > Platform Coordinated (PC) mode, each CPU independently votes for an idle
> > state of the cluster.
> >
> > For this reason, introduce a per CPU variable called domain_state and
> > implement two helper functions to read/write its value. Then let the
> > domain_state take precedence over the regular selected state, when entering
> > and idle state.
> >
> > Finally, let's also avoid sprinkling the existing non-OSI path with
> > operations being specific for OSI.
> >
>
> Mostly looks good.

Thanks!

> I am still wondering if we can keep all OSI related
> info in the newly created structure and have psci_states outside it as
> before. And I was think psci_enter_idle_state_pc and psci_enter_idle_state_osi
> instead of single psci_enter_idle_state and assign/initialise state->enter
> based on the mode chosen. I had to closer look now and looks like enter
> is initialised in generic dt_idle_states. That said, what you have in this
> patch also looks OK to me, was just trying to avoid access to the new
> structure all together and keep the PC mode patch almost same as before
> when suspending. I will see what Lorenzo thinks about this.

I did explore that approach a bit, but found it easier to go with what
I propose here. The most important point, in my view, is that in this
suggested approach only one if-check, "if (!data->dev)", is added to
the PC mode path compared to the original path. I think this should be
fine, right!?

In any case, if you prefer any other solution, just tell me and I adapt to it.

Now, I am looking forward to hear from Lorenzo.

Kind regards
Uffe