Re: [Bisected] altera_edac crash on a system without ECC

[Thor Thayer <thor.thayer@linux.intel.com>]

Hi Aaro,

On 12/4/19 7:25 AM, Aaro Koskinen wrote:
> Hi,
> 
> On Tue, Dec 03, 2019 at 05:09:48PM -0600, Thor Thayer wrote:
>> On 11/29/19 10:57 AM, Aaro Koskinen wrote:
>>> Hi,
>>>
>>> I tried booting v5.4 mainline kernel on a stratix10 board with ECC
>>> disabled, and the altera-edac driver (with only SDRAM enabled) is
>>> now crashing the system instead of failing the probe with "No ECC/ECC
>>> disabled".
>>>
>> I apologize for the late reply. I was on vacation.
>>
>> ECC disabled means the sof/jic that you're loading has ECC disabled,
>> correct?
> 
> Yes.
> 
>>> This seems to have started with commit 08f08bfb7b4c ("EDAC, altera:
>>> Merge Stratix10 into the Arria10 SDRAM probe routine"). With the change,
>>> looks like sdram probe no longer uses SMC calls and instead accesses
>>> the registers directly. The crash looks like this:
>>
>> I haven't seen this. I'd expect both ECC enabled and disabled to fail with
>> the dumps you have below since they'd both need to use the regmap functions.
> 
> With ECC enabled it doesn't fail, as the direct register access appears
> to work then (I also checked by reading 0xf8011101 from userspace -
> and it works without an abort).
> 
>> Yes, this does look like it is using the register accesses instead of the
>> SMC call. Line 2206 sets the SMC call after determining from the if()
>> statement if it is a Stratix10 or Arria10 and from below it seems to take
>> the Arria10 path.
> 
> But that's setting the ecc_mgr_map. I think that altr_check_ecc_deps
> and altr_sdram_probe use a different mapping. Before commit 08f08bfb7b4c
> there was S10 specific altr_s10_sdram_probe() that took care of the SMC,
> but I cannot see how the current code doing that unless I'm missing some
> special magic.
> 
>> The altr_check_ecc_deps() call is checking whether ECC is enabled so the
>> probe should fail.
>>
>> I suspect the device tree. Can you verify the following node is in your
>> device tree?
>>
>> 	sdramedac {
>> 		compatible = "altr,sdram-edac-s10";
>> 		altr,sdr-syscon = <&sdr>;
>> 		interrupts = <16 4>;
>> 	};
> 
> Yes, I'm using the in-tree socfpga_stratix10.dtsi.
> 
> A.
> 
This appears to be a setup problem in U-Boot where U-Boot only allows 
access to the ECC registers if ECC is enabled. However, the Linux EDAC 
driver reads the ECC registers to determine if ECC is enabled or not.

A patch that always allows access to the ECC registers has been posted 
to U-Boot.
https://patchwork.ozlabs.org/patch/1205274/

Thanks,

Thor