Re: [PATCH v4 13/14] cpuidle: psci: Add support for PM domains by using genpd

[Sudeep Holla <sudeep.holla@arm.com>]

On Fri, Dec 20, 2019 at 12:27:39PM +0100, Ulf Hansson wrote:
> On Fri, 20 Dec 2019 at 11:07, Sudeep Holla <sudeep.holla@arm.com> wrote:

[...]

> >
> > Even if you don't create all these genpd domains, it is still degraded
> > mode and we are anyway not changing that. Let me know if my understanding
> > is wrong here.
>
> Your understanding is wrong.
>
> If I remove the genpds because psci_set_osi_mode() fails, then in the
> current suggested initialization path, that will lead to that the
> entire cpuidle-psci driver will fail to initiate (which is because
> psci_dt_attach_cpu() returns an error). In other words, only WFI state
> will be used by cpuidle as there will be no cpuidle driver registered
> at all.
>
> That would not be an acceptable behaviour, as it would make the
> situation worse than today.
>
> What we want in this scenario is to keep using all the idle states for
> the CPUs, but ignores those for the cluster. That we both agree on,
> right?
>

Yes, I agree and understand that. I was assuming as part of this change
you will fixup psci_dt_cpu_init_idle not to return error but just allow
CPU level idle. Sorry if that was not clear, I was always assuming that.

> >
> > I am sure, DTB may get copied to different platform and the firmware may
> > not support OSI. I know we have logs, but creating and leaving those
> > genpd domains unused will be just confusing. Please change that.
>
> We are not creating any genpds unless OSI mode is supported. We do not
> even try to attach CPUs to the PM domains, unless OSI mode is
> supported. So this should already work according to your expectations
> and previous requests.
>

Yes I understand, but checking if "OSI mode is supported" is not same as
"setting OSI mode". Until OSI mode is set, it is default/PC mode, so we
need to work based on that assumption.

> To address your concern about removing genpds when psci_set_osi_mode()
> fails, we also need to address the problems we get when calling
> psci_dt_attach_cpu(). There are two viable options as I see it.
>

Shouldn't that fail ? Sorry, I might be missing something.

> 1. Prevent calling psci_dt_attach_cpu() altogether when
> psci_set_osi_mode() failed. This means another function needs to be
> shared from cpuidle-psci-domain.c to let cpuidle-psci.c know about it.
>

If we don't create any genpd, will psci_dt_attach_cpu fail ?

> 2. We can let psci_dt_attach_cpu() return NULL, when
> psci_set_osi_mode() failed - as this information is already known by
> cpuidle-psci-domain.c.
>

Yes I was making all the arguments/discussion based on that. Do you see
any issues with that ? Any races possible ?

> I vote for option 2, but what do you think?
>

Me too from the time I started the discussion, I assume a lot and
don't put this into words in the email.

--
Regards,
Sudeep