Re: [PATCH v10 18/27] drivers: firmware: psci: Add support for PM domains using genpd

[Ulf Hansson]

[...]

> > -obj-$(CONFIG_ARM_PSCI_FW)    += psci.o
> > +obj-$(CONFIG_ARM_PSCI_FW)    += psci.o psci_pm_domain.o
>
> Same comment as 17/27.
>
> +obj-$(CONFIG_PSCI_IDLE) += psci_pm_domain.o

Let's discuss that in patch 17 to agree on the way forward.

[...]

> > +static int psci_pd_power_off(struct generic_pm_domain *pd)
> > +{
> > +     struct genpd_power_state *state = &pd->states[pd->state_idx];
> > +     u32 *pd_state;
> > +     u32 composite_pd_state;
> > +
> > +     /* If we have failed to enable OSI mode, then abort power off. */
> > +     if (psci_has_osi_support() && !osi_mode_enabled)
> > +             return -EBUSY;
>
> I'm not sure EBUSY is the best error code to describe this situation.
> May be ENOTSUP ?

I see your point. However, -EBUSY is the correct code for this case as
it tells genpd that the PM domain could not power off, but needs to
stay powered on.

To be clear, genpd treat -EBUSY, in a bit special way.

>
> However, how possible is it to pass in this function if the OSI mode was
> not enabled ?

If we fail to enable OSI mode, we keep having the CPUs attached to
PSCI PM PM domains. Although, as we are then running in platform
coordinated mode, there is no point allowing a cluster idle state.

The option would be to convert to the flattened model in case we fail
to enable OSI mode, but that error path is going to be rather
complicated (we need to detach CPUs, unregister domains and providers,
etc). Instead the assumption is that it simply shouldn't fail when we
decide to try. If it does, it's likely something need to be fixed
anyways.

>
> > +     if (!state->data)
> > +             return 0;
> > +
> > +     /* When OSI mode is enabled, set the corresponding domain state. */
> > +     pd_state = state->data;
> > +     composite_pd_state = *pd_state | psci_get_domain_state();
> > +     psci_set_domain_state(composite_pd_state);
> > +
> > +     return 0;
> > +}
> > +
> > +static int psci_pd_parse_state_nodes(struct genpd_power_state *states,
> > +                             int state_count)
>
> __init ?

Yes, probably. I will check and see if/where it could makes sense.
Thanks for pointing it out!

[...]

Kind regards
Uffe