Message-ID: <20240807103245593-0700.eberman@hu-eberman-lv.qualcomm.com>
Date: Wed, 7 Aug 2024 11:10:50 -0700
From: Elliot Berman <quic_eberman@...cinc.com>
To: Lorenzo Pieralisi <lpieralisi@...nel.org>
CC: Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio
	<konrad.dybcio@...aro.org>,
        Sebastian Reichel <sre@...nel.org>, Rob Herring
	<robh@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>, Vinod Koul <vkoul@...nel.org>,
        Andy Yan
	<andy.yan@...k-chips.com>,
        Mark Rutland <mark.rutland@....com>,
        "Bartosz
 Golaszewski" <bartosz.golaszewski@...aro.org>,
        Satya Durga Srinivasu Prabhala
	<quic_satyap@...cinc.com>,
        Melody Olvera <quic_molvera@...cinc.com>,
        Shivendra Pratap <quic_spratap@...cinc.com>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        Florian Fainelli <florian.fainelli@...adcom.com>,
        <linux-pm@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH v5 3/4] firmware: psci: Read and use vendor reset types

On Wed, Aug 07, 2024 at 05:02:38PM +0200, Lorenzo Pieralisi wrote:
> On Mon, Jun 17, 2024 at 10:18:09AM -0700, Elliot Berman wrote:
> > SoC vendors have different types of resets and are controlled through
> > various registers. For instance, Qualcomm chipsets can reboot to a
> > "download mode" that allows a RAM dump to be collected. Another example
> > is they also support writing a cookie that can be read by bootloader
> > during next boot. PSCI offers a mechanism, SYSTEM_RESET2, for these
> > vendor reset types to be implemented without requiring drivers for every
> > register/cookie.
> > 
> > Add support in PSCI to statically map reboot mode commands from
> > userspace to a vendor reset and cookie value using the device tree.
> > 
> > A separate initcall is needed to parse the devicetree, instead of using
> > psci_dt_init because mm isn't sufficiently set up to allocate memory.
> > 
> > Reboot mode framework is close but doesn't quite fit with the
> > design and requirements for PSCI SYSTEM_RESET2. Some of these issues can
> > be solved but doesn't seem reasonable in sum:
> >  1. reboot mode registers against the reboot_notifier_list, which is too
> >     early to call SYSTEM_RESET2.
> 
> Please define "too early" (apologies if it has been explained before).
> 

My understanding is that reboot_notifier_list handlers shouldn't
actually be rebooting the system. I see it's generally used for one last
operation to put system into safer state. A few examples that are quick
to write based on what I see today: watchdogs disable themselves, PMUs
get torn down, xenbus tries to abort any requests. There are a couple
examples of drivers that *do* immediately reboot, but those seem to be
outlier. None of the reboot mode framework clients currently trigger
restart itself: they all set some cookies to give hint to firmware or
hardware what to do.

The restart_handler_list is documented to be a list of handlers that
should try to really restart the system. PSCI driver currently registers
against this.

> >     PSCI would need to remember the reset type from the reboot-mode
> >     framework callback and use it psci_sys_reset.
> >  2. reboot mode assumes only one cookie/parameter is described in the
> >     device tree. SYSTEM_RESET2 uses 2: one for the type and one for
> >     cookie.
> 
> That's surmountable I suppose.
> 
> >  3. psci cpuidle driver already registers a driver against the
> >     arm,psci-1.0 compatible. Refactoring would be needed to have both a
> >     cpuidle and reboot-mode driver.
> 
> We could put together a PSCI "parent" driver that creates child platform
> devices for idle and reboot drivers to match (which actually is not
> really pretty but it would make more sense than matching the idle
> driver only to the psci compatible string, which is what current code
> does).
> 
> > Tested-by: Florian Fainelli <florian.fainelli@...adcom.com>
> > Signed-off-by: Elliot Berman <quic_eberman@...cinc.com>
> > ---
> >  drivers/firmware/psci/psci.c | 92 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 92 insertions(+)
> > 
> > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> > index d9629ff87861..e672b33b71d1 100644
> > --- a/drivers/firmware/psci/psci.c
> > +++ b/drivers/firmware/psci/psci.c
> > @@ -29,6 +29,8 @@
> >  #include <asm/smp_plat.h>
> >  #include <asm/suspend.h>
> >  
> > +#define REBOOT_PREFIX "mode-"
> > +
> >  /*
> >   * While a 64-bit OS can make calls with SMC32 calling conventions, for some
> >   * calls it is necessary to use SMC64 to pass or return 64-bit values.
> > @@ -79,6 +81,14 @@ struct psci_0_1_function_ids get_psci_0_1_function_ids(void)
> >  static u32 psci_cpu_suspend_feature;
> >  static bool psci_system_reset2_supported;
> >  
> > +struct psci_reset_param {
> > +	const char *mode;
> > +	u32 reset_type;
> > +	u32 cookie;
> > +};
> > +static struct psci_reset_param *psci_reset_params;
> > +static size_t num_psci_reset_params;
> > +
> >  static inline bool psci_has_ext_power_state(void)
> >  {
> >  	return psci_cpu_suspend_feature &
> > @@ -305,9 +315,29 @@ static int get_set_conduit_method(const struct device_node *np)
> >  	return 0;
> >  }
> >  
> > +static void psci_vendor_sys_reset2(unsigned long action, void *data)
> 
> 'action' is unused and therefore it is not really needed.
> 
> > +{
> > +	const char *cmd = data;
> > +	unsigned long ret;
> > +	size_t i;
> > +
> > +	for (i = 0; i < num_psci_reset_params; i++) {
> > +		if (!strcmp(psci_reset_params[i].mode, cmd)) {
> > +			ret = invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2),
> > +					     psci_reset_params[i].reset_type,
> > +					     psci_reset_params[i].cookie, 0);
> > +			pr_err("failed to perform reset \"%s\": %ld\n",
> > +				cmd, (long)ret);
> > +		}
> > +	}
> > +}
> > +
> >  static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
> >  			  void *data)
> >  {
> > +	if (data && num_psci_reset_params)
> 
> So, reboot_mode here is basically ignored; if there is a vendor defined
> reset, we fire it off.
> 
> I think Mark mentioned his concerns earlier related to REBOOT_* mode and
> reset type (granted, the context was different):
> 
> https://lore.kernel.org/all/20200320120105.GA36658@C02TD0UTHF1T.local/
> 
> I would like to understand if this is the right thing to do before
> accepting this patchset.
> 

I don't have any concerns to move this part below checking reboot_mode.
Or, I could add reboot_mode == REBOOT_COLD check.

I'm not sure how best to define the behavior if user sets
reboot_mode = REBOOT_WARM and then user does "reboot bootloader". IMO,
"REBOOT_WARM" is at odds with the "bootloader" command. We can have one
take precedence over the other. I chose for the vendor resets to take
priority, since if userspace is providing specific command at reboot
time, that's probably what they want.

Let me know your thoughts and I'm happy to change the behavior around.


Thanks,
Elliot

Powered by blists - more mailing lists