Re: [PATCH 3/4] soc: bcm: brcmstb: Added support for PSCI system suspend operations

From: Florian Fainelli
Date: Thu Feb 03 2022 - 13:45:31 EST

Hi Mark,

On 2/3/2022 4:09 AM, Mark Rutland wrote:
Hi,

On Fri, Jan 21, 2022 at 07:54:20PM -0800, Florian Fainelli wrote:
Add support for the Broadcom STB system suspend operations which
leverage the standard PSCI functions and uses the
psci_cpu_suspend_enter() operation to power off the system with or
without retention ("echo standby > /sys/power/state").

What exactly does this enable that can't be achieved with the existing PSCI
driver as-is?

When you say "retention", what specifically do you mean? Retention of CPU
state? DRAM contents?

We already have SYSTEM_SUSPEND for states where DRAM content is retained but
CPU (and some system state) is lost, and IIUC we can do suspend-to-idle with
CPU_SUSPEND states.

interface necessary?

The system reset path also supports a special "powercycle" mode which
signals to the ARM Trusted Firmware that an external PMIC chip must
force the SoC into a power cycle.

How does that compare to the regular SYSTEM_RESET call?

The PSCI spec says of SYSTEM_RESET:

| This function provides a method for performing a system cold reset. To the
| caller, the behavior is equivalent to a hardware power-cycle sequence.

... so I don't follow how this is different, unless this platform's
SYSTEM_RESET implementation is *not* actually performing a system cold reset?

If that *doesn't* perform a cold rest, it seems like a bug?

I think you answered that by looking at the code down below and the use case was to define a vendor specific method of resetting the chip. This is something that we could sort of always override one way or the other by registering our own power off notifier callback with a higher priority to make it take precedence, assuming that the platform is indeed brcmstb so we don't override other people's systems, too.


As much as possible extensions were built using the SIP namespace rather
than the standard PSCI namespace, however compatibility with the
standard PSCI implementation is retained when CONFIG_BRCMSTB_PM is not
selected.

I really don't like this, because it seems to be creating a parallel
infrastructure for doing things that can *already* be done with standard PSCI
driver. The actual PSCI bits just seem to be playing about with the power_state
value, which we should be able to do in the regular PSCI driver, and the
SIP-specific functions seem to have nothing to do with PSCI.

The implementation is standard in the sense that no PSCI function call had to be modified in a non-standard way for system wide suspend/resume operations to work, but yet the mix of SiP and PSCI is not properly used to differentiate the platform as you reported.


At the least there needs to be a much better explanation of why this is
necessary, but overall I'd very much like to have *any* vendor specific code
for suspend states, and if there are limitations in the standard PSCI driver we
go and address those. Otherwise we're going to gain a plethora of
vendor-specific suspend implementations, which is exactly what PSCI was
intended to avoid in the first place.

Entirely fair.


I have some specific comments below, but even with those addressed, I don't
think this is the right approach, and as things stand, NAK to this.

That is fair, I think I have a clearer view of how to support some of our uses cases by extending the existing PSCI in ways that is hopefully acceptable.

[snip]

+static int brcmstb_psci_integ_region_reset_all(void)
+{
+ return invoke_psci_fn(SIP_FUNC_INTEG_REGION_RESET_ALL, 0, 0, 0);
+}

What's all this? Below I see the phrase "integrity checking regions", but only
the brcmstb_psci_integ_region_reset_all() function is used, and it's not clear
what this is supposed to be for.

Will remove that. We have a set of function calls here that allow us to define which specific areas of DRAM are to be hash checked during suspend, and then hash checked again during resume. This is used both as a debugging tool to spot faulty board designs where DRAM power is not retained as it should as well as a security counter measure.


+static int brcmstb_psci_sys_reset(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ const char *cmd = data;
+ /*
+ * reset_type[31] = 0 (architectural)
+ * reset_type[30:0] = 0 (SYSTEM_WARM_RESET)
+ * cookie = 0 (ignored by the implementation)
+ */
+ uint32_t reboot_type = 0;
+
+ if ((action == REBOOT_COLD || action == REBOOT_WARM ||
+ action == REBOOT_SOFT) &&
+ brcmstb_psci_system_reset2_supported) {
+ if (cmd && !strcmp(cmd, "powercycle"))
+ reboot_type = BIT(31) | 1;
+ invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), reboot_type, 0, 0);
+ } else {
+ invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
+ }
+
+ return NOTIFY_DONE;
+}

If there are a bunch of specific SYSTEM_RESET2 values we want to expose, I'd
rather we described those generically in the DT, and somehow handle that in the
generic driver.

OK, it that seems appropriate, I will propose something that extends the DT binding and standard PSCI implementation.

[snip]

+
+static struct notifier_block brcmstb_psci_nb = {
+ .notifier_call = brcmstb_psci_panic_notify,
+};

This appears to have nothing to do with idle/suspend states (and so might be OK
on its own if you need it, but it should be in a separate patch with some
justification).

Correct this is orthogonal and this is just a signal to the firmware that kernel has panicked.

[snip]


Immediately after this switch, we know we there is *a* SMCCCv1.1+
implementation, but we have no idea *which* implementation that is. It could be
Broadcom's brcmstb implementation, it could be KVM's implementation, or anyone
else's...

+
+ /* Check the revision of monitor */
+ if (invoke_psci_fn == __invoke_psci_fn_hvc)
+ arm_smccc_hvc(SIP_SVC_REVISION,
+ 0, 0, 0, 0, 0, 0, 0, &res);
+ else
+ arm_smccc_smc(SIP_SVC_REVISION,
+ 0, 0, 0, 0, 0, 0, 0, &res);

This tells us the SIP interface's revision (if implemented), but not that the
SIP is Broadcom, and we still don't know that this is the brcmstb
implementation specifically.

Good point.

[snip]


+ pr_info("Using PSCI based system PM (full featured)\n");

This should be explicit with something like "Overriding stnadard PSCI
functionaliy with brcmstb-specific code".

As it stands this is at best meaningless, and at worst misleading and
disparaging of standard PSCI.

This is meaningless and a left over from our downstream tree where it is used to determine the generational level of the implementation, I will definitively remove it.

[snip]

+#define SIP_SVC_REVISION \
+ ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+ IS_ENABLED(CONFIG_64BIT), \
+ ARM_SMCCC_OWNER_SIP, \
+ 0xFF02)

Looking at the SMCCC spec, isn't the "general service query" REVISION call
0xFF03? 0xFF02 is reserved.

Sure is, whoever wrote that probably did not know it at the time (not me!).
--
Florian

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature