On Fri, 15 Nov 2019 at 18:16, Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
>
> On Tue, Oct 29, 2019 at 05:44:34PM +0100, Ulf Hansson wrote:
> > In order to enable a CPU to be power managed through its PM domain, let's
> > try to attach it by calling psci_dt_attach_cpu() during the cpuidle
> > initialization.
> >
> > psci_dt_attach_cpu() returns a pointer to the attached struct device, which
> > later should be used for runtime PM, hence we need to store it somewhere.
> > Rather than adding yet another per CPU variable, let's create a per CPU
> > struct to collect the relevant per CPU variables.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> > ---
> >
> > Changes in v2:
> > - Rebased.
> >
> > ---
> > drivers/cpuidle/cpuidle-psci.c | 24 ++++++++++++++++++++----
> > 1 file changed, 20 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > index 830995b8a56f..167249d0493f 100644
> > --- a/drivers/cpuidle/cpuidle-psci.c
> > +++ b/drivers/cpuidle/cpuidle-psci.c
> > @@ -20,14 +20,20 @@
> >
> > #include <asm/cpuidle.h>
> >
> > +#include "cpuidle-psci.h"
> > #include "dt_idle_states.h"
> >
> > -static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
> > +struct psci_cpuidle_data {
> > + u32 *psci_states;
> > + struct device *dev;
> > +};
> > +
> > +static DEFINE_PER_CPU_READ_MOSTLY(struct psci_cpuidle_data, psci_cpuidle_data);
> >
> > static int psci_enter_idle_state(struct cpuidle_device *dev,
> > struct cpuidle_driver *drv, int idx)
> > {
> > - u32 *state = __this_cpu_read(psci_power_state);
> > + u32 *state = __this_cpu_read(psci_cpuidle_data.psci_states);
> >
> > return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter,
> > idx, state[idx]);
> > @@ -78,7 +84,9 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
> > {
> > int i, ret = 0;
> > u32 *psci_states;
> > + struct device *dev;
> > struct device_node *state_node;
> > + struct psci_cpuidle_data *data = per_cpu_ptr(&psci_cpuidle_data, cpu);
> >
> > state_count++; /* Add WFI state too */
> > psci_states = kcalloc(state_count, sizeof(*psci_states), GFP_KERNEL);
> > @@ -104,8 +112,16 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
> > goto free_mem;
> > }
> >
> > - /* Idle states parsed correctly, initialize per-cpu pointer */
> > - per_cpu(psci_power_state, cpu) = psci_states;
> > + dev = psci_dt_attach_cpu(cpu);
>
> Why do we need to call psci_dt_attach_cpu for even PC mode and ...
>
> > + if (IS_ERR(dev)) {
> > + ret = PTR_ERR(dev);
> > + goto free_mem;
> > + }
> > +
> > + data->dev = dev;
> > +
>
> ... assign NULL above. I don't see the need for one. Default it will be
> NULL anyway and we can call psci_dt_attach_cpu only if psci_has_osi_support()
Are you sure it's NULL as default? It's a pointer, within a static
initiated struct.
"static DEFINE_PER_CPU_READ_MOSTLY(struct psci_cpuidle_data,
psci_cpuidle_data);"
>
> I will look through further patches to see if it make sense or not.
So, the check for psci_has_osi_support() is done in
psci_dt_attach_cpu(), which returns "NULL" if OSI isn't supported.
The idea with this approach is also to keep the common code in
psci_dt_cpu_init_idle() (or the entire cpuidle-psci.c actually), as
transparent as possible, to whether PSCI OSI-mode is supported or not.
Of course, if you insist, I am open to change in any way you prefer.
Kind regards
Uffe
