Date:   Thu, 3 Feb 2022 10:45:23 -0800
From:   Florian Fainelli <florian.fainelli@...adcom.com>
To:     Mark Rutland <mark.rutland@....com>,
        Florian Fainelli <f.fainelli@...il.com>
Cc:     linux-arm-kernel@...ts.infradead.org,
        "maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE" 
        <bcm-kernel-feedback-list@...adcom.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/4] soc: bcm: brcmstb: Added support for PSCI system
 suspend operations

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

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4221 bytes)

Powered by blists - more mailing lists