Date:   Tue, 30 Mar 2021 10:50:54 +0200
From:   Maxime Ripard <maxime@...no.tech>
To:     Samuel Holland <samuel@...lland.org>
Cc:     Andre Przywara <andre.przywara@....com>,
        Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec <jernej.skrabec@...l.net>,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-sunxi@...ts.linux.dev,
        linux-sunxi@...glegroups.com
Subject: Re: [RFC PATCH] arm64: dts: allwinner: a64/h5: Add CPU idle states

Hi Samuel,

On Tue, Mar 23, 2021 at 11:44:50PM -0500, Samuel Holland wrote:
> On 3/22/21 8:56 PM, Andre Przywara wrote:
> >> I'm sending this patch as an RFC because it raises questions about how
> >> we handle firmware versioning. How far back does (or should) our support
> >> for old TF-A and Crust versions go?
> >>
> >> cpuidle has a problem that without working firmware support, CPUs will
> >> enter idle states and be unable to wake up. As a result, the system will
> >> hang at some point during boot, usually before getting to userspace.
> >>
> >> For over a year[0], TF-A has exposed the PSCI CPU_SUSPEND function when
> >> a SCPI implementation is present[1]. Implementing CPU_SUSPEND is
> >> required for implementing SYSTEM_SUSPEND[2], even if CPU_SUSPEND is not
> >> itself used for anything. 
> >>
> >> However, there was no code to actually wake up a CPU once it called the
> >> CPU_SUSPEND function, because I could not find the register providing
> >> the necessary information. The fact that CPU_SUSPEND was broken affected
> >> nobody, because nothing ever called it -- there were no idle states in
> >> the DTS. In hindsight, what I should have done was always return failure
> >> from sunxi_validate_power_state(), but that ship has long sailed.
> >>
> >> I finally found the elusive register and implemented the wakeup code
> >> earlier this month[3]. So now, CPU_SUSPEND actually works, if all of
> >> your firmware is up to date, and cpuidle works if you add the states in
> >> your device tree.
> >>
> >> Unfortunately, there is currently nothing verifying that compatibility.
> >> So you can get into four possible scenarios:
> >>   1) No idle states in DTS, any firmware => Linux works, with baseline
> >>      power consumption.
> >>   2) Idle states added to DTS, no Crust/SCPI => Linux works, but every
> >>      attempt to enter an idle state is rejected because CPU_SUSPEND is
> >>      not hooked up. So power consumption increases by a sizable amount.
> >>   3) Idle states added to DTS, "old" Crust/SCPI (before [3]) => Linux
> >>      fails to boot, because CPUs never return from idle states.
> >>   4) Idle states added to DTS, "new" Crust/SCPI (after [3]) => Linux
> >>      works, with improved power consumption compared to the baseline.
> >>
> >> Obviously, we want to prevent scenario 3 if possible.
> > 
> > So I think the core of the problem is that the DT describes some
> > firmware feature, but we have the DT bundled with the kernel, not the
> > firmware.
> 
> I would say the core problem is that the firmware lies about supporting
> PSCI CPU_SUSPEND. Linux shouldn't be calling CPU_SUSPEND if the firmware
> declares it as unavailable, regardless of what is in the DTS.

I would say we have two core problems :)

> (Technically, per the PSCI standard, CPU_SUSPEND is a mandatory
> function, but a quick survey of the TF-A platforms shows it is far from
> universally implemented.)

U-boot also implements CPU_SUSPEND but will return -1 if you don't have
an implementation. I guess that at the moment we shouldn't be reporting
it as supported if we don't

But I also agree with Andre here, we shouldn't list cpuidles
capabilities in the DTS if we don't always have them either.

> > So is there any way we can detect an older crust version in U-Boot,
> > then remove any potential idle states from the DT?
> 
> Let's assume that we are using a functioning SoC (H3) or the secure fuse
> is blown (A64) and therefore U-Boot cannot access SRAM A2. I can think
> of three ways it can learn about crust:
> 
> a) PSCI_FEATURES (e.g. is CPU_SUSPEND supported)
> b) Metadata in the FIT image
> c) Custom SMCs
> 
> TF-A has some additional methods available:
> 
> d) The SCPI-reported firmware version
> e) The magic number at the beginning of the firmware binary
> 
> > Granted, this requires recent U-Boot as well, but at least we could try
> > to mitigate the worst case a bit?
> 
> If we're okay with modifying firmware to solve this problem, then I
> propose the following solution:
> 
> 1) Version bump crust or change its magic number.
> 2) Modify TF-A to only report CPU_SUSPEND as available if it detects the
>    new crust version. This would involve conditionally setting
>    sunxi_scpi_psci_ops.validate_power_state, and updating psci_setup.c
>    to also check for .validate_power_state when setting psci_caps.
> 3) Modify the Linux PSCI client to respect PSCI_FEATURES when setting
>    psci_ops.cpu_suspend. cpuidle-psci checks for this function before
>    setting up idle states.
> 4) Finally, after some time, add the idle states to the DTS.
> 
> In fact, this solution solves both scenarios 2 and 3, because it also
> takes care of the native PM implementation, which doesn't implement
> CPU_SUSPEND at all.
> 
> Does that sound workable?

If we can add the DT node at boot in crust (or in TF-A), then we don't
really need PSCI_FEATURES, and it would magically work once the firmware
is updated. It looks like a solid plan otherwise

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists