Re: [PATCH] EDAC/altera: Warm Reset option for Stratix10 peripheral DBE

From: Thor Thayer
Date: Tue Jun 04 2019 - 17:43:56 EST

Hi James,

On 6/4/19 12:23 PM, James Morse wrote:
Hi Thor,

(CC: +Mark, Lorenzo and Sudeep for PSCI.
How should SYSTEM_RESET2 be used for a vendor-specific reset?

The original patch is:

Thank you for reviewing my patch and bringing in additional expertise.

On 03/06/2019 21:37, thor.thayer@xxxxxxxxxxxxxxx wrote:
From: Thor Thayer <thor.thayer@xxxxxxxxxxxxxxx>

The Stratix10 peripheral FIFO memories can recover from double
bit errors with a warm reset instead of a cold reset.
Add the option of a warm reset for peripheral (USB, Ethernet)

CPU memories such as SDRAM and OCRAM require a cold reset for
Filter on whether the error is a SDRAM/OCRAM or a peripheral
FIFO memory to determine which reset to use when the warm
reset option is configured.

... so you want to make different SMC calls on each CPU after panic()?

Yes. The registers needed for a warm reset need to be setup in EL3 (U-Boot). As part of this, I needed the other CPUs (3 of the 4) to idle in WFI.

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index 8816f74a22b4..179601f14b48 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -2036,6 +2036,19 @@ static const struct irq_domain_ops a10_eccmgr_ic_ops = {
/* panic routine issues reboot on non-zero panic_timeout */
extern int panic_timeout;
+/* EL3 SMC call to setup CPUs for warm reset */
+void panic_smp_self_stop(void)
+ struct arm_smccc_res result;
+ __cpu_disable();
+ cpu_relax();
+ S10_WARM_RESET_WFI_FLAG, 0, 0, 0, 0, 0, &result);


panic_smp_self_stop() isn't for drivers to override: only the arch code.
__cpu_disable() is only for the cpu-hotplug machinery. Nothing else should touch it.

Isn't this thing only called if another CPU out there is panic()ing too?

OK. I wasn't aware of that intention. Thanks.

On an SError, all the CPUs except the first one end up calling panic_smp_self_stop().

All the CPUs are calling the panic routine which follows this path:
arm64_serror_panic() -> nmi_panic()

In nmi_panic()
If this is the first CPU to reach nmi_panic(), call panic()
otherwise, call nmi_panic_self_stop() -> panic_smp_self_stop()

panic_smp_self_stop() is a while(1) cpu_relax() which is a yield but I needed a WFI.

I think one of the problems here is arm64 leaves secondary CPUs running after panic().
This would be better fixed by using the appropriate cpu_ops[]->cpu_die() call in arm64's

Yes. I will look into this.

As for passing platform-specific options, PSCI[0] has a 'reset_type' for SYSTEM_RESET2,
which looks suspiciously like what you want here. I'm not sure how its expected to be
used... hopefully the PSCI maintainers can give us some pointers.

(The existing support is commit 4302e381a870 ("firmware/psci: add support for SYSTEM_RESET2"))

Is it possible for firmware to do both the cold/warm reset work when SYSTEM_RESET is
called? This would mean you don't have to care here and there are fewer choices to be made
If not, is there anything left behind that can give it the hint? Like non-zero error
counters for the USB/Ethernet devices?

Hmm. I'll look into this. I was trying to handle more in Linux so that I could do a best effort of printout notification of the error, etc for the logs (even though I know getting this far or printing out isn't guaranteed)

@@ -2067,14 +2080,28 @@ static int s10_edac_dberr_handler(struct notifier_block *this,
S10_SYSMGR_UE_ADDR_OFST, err_addr);
edac_printk(KERN_ERR, EDAC_DEVICE,
- "EDAC: [Fatal DBE on %s @ 0x%08X]\n",
- ed->edac_dev_name, err_addr);
+ "EDAC: [Fatal DBE on %s [CPU=%d] @ 0x%08X]\n",
+ ed->edac_dev_name, raw_smp_processor_id(),
+ err_addr);
/* Notify the System through SMC. Reboot delay = 1 second */
+ /* Handle peripheral FIFO DBE as Warm Resets */
+ if (dberror & S10_COLD_RESET_MASK) {

+ panic_timeout = 1;

Isn't this value supposed to be provided on the kernel commandline? Surely this prevents
debug using the commandline option to increase the delay?

(I see you already change it)

I see your point but I only override the value when I have an ECC DBE panic.

+ arm_smccc_smc(INTEL_SIP_SMC_ECC_DBE, dberror, 0, 0, 0,
+ 0, 0, 0, &result);
+ } else {
+ arm_smccc_smc(INTEL_SIP_SMC_ECC_DBE,
+ S10_WARM_RESET_WFI_FLAG | dberror, 0, 0,
+ 0, 0, 0, 0, &result);
+ }
panic_timeout = 1;
arm_smccc_smc(INTEL_SIP_SMC_ECC_DBE, dberror, 0, 0, 0, 0,
0, 0, &result);

What do these SMC do? Are they equivalent to the PSCI CPU online/offline calls?

panic() notifiers aren't robust as they can be skipped if kdump is loaded.

These are calling into the firmware to 1) notify the firmware of an upcoming cold reset in the case of cold reset or 2) call the warm reset handler in the firmware.

I missed that limitation of the panic() notifiers.

Thanks for the insightful comments!