by Thor Thayer

[permalink] [raw]

Subject: Re: [PATCHv5 4/5] edac: altera: Add Altera L2 Cache and OCRAM EDAC Support

On 12/02/2014 09:25 AM, Mark Rutland wrote:
> Hi,
>
>> +/* MPU L2 Register Defines */
>> +#define ALTR_MPUL2_CONTROL_OFFSET 0x100
>> +#define ALTR_MPUL2_CTL_CACHE_EN_MASK BIT(0)
>
> These are just standard PL310 register definitions, no?

Yes.

> [...]
>
>> +static void *ocram_alloc_mem(size_t size, void **other)
>> +{
>> + struct device_node *np;
>> + struct gen_pool *gp;
>> + void *sram_addr;
>> +
>> + np = of_find_compatible_node(NULL, NULL, "altr,ocram-edac");
>> + if (!np)
>> + return NULL;
>
> Don't you need to have a corresponding of_node_put?
>
> Is this ever called before the code above has probed? Can't you keep
> this information around rather than probing it every time?
>

This is only used for testing so it happens infrequently and always
after the probe has finished.

These error injection functions will not be part of the standard
distribution for our board so it was cleaner to leave it out.

>> +
>> + gp = of_get_named_gen_pool(np, "iram", 0);
>> + if (!gp)
>> + return NULL;
>> +
>> + sram_addr = (void *)gen_pool_alloc(gp, size);
>> + if (!sram_addr)
>> + return NULL;
>> +
>> + memset(sram_addr, 0, size);
>
> Is it safe to do a memset to the sram? How is it mapped?
>

I checked the gen_pool and the memory may not be contiguous. I will fix
this. Thanks!

> [...]
>
>> +static void *l2_alloc_mem(size_t size, void **other)
>> +{
>> + struct device *dev = *other;
>> + void *ptemp = devm_kzalloc(dev, size, GFP_KERNEL);
>> +
>> + if (!ptemp)
>> + return NULL;
>> +
>> + /* Make sure everything is written out */
>> + wmb();
>> + flush_cache_all();
>
> Doesn't this just flush out _to_ the L2 (and not beyond)? Even then I
> don't think that's safe with the MMU enabled.
>
> Why is the flush necessary?
>

flush_cache_all() maps to flush_kern_all() which cleans all the cache
levels up to the level of coherency which includes L2 in it.

This flush ensures the data is in a known state (zero) before I inject
the errors for testing L2 ECC.

> [...]
>
>> +static int altr_l2_dependencies(struct platform_device *pdev,
>> + void __iomem *base)
>> +{
>> + u32 control;
>> + struct regmap *l2_vbase;
>> +
>> + control = readl(base) & ALTR_L2_ECC_EN_MASK;
>> + if (!control) {
>> + edac_printk(KERN_ERR, EDAC_DEVICE,
>> + "L2: No ECC present, or ECC disabled\n");
>> + return -ENODEV;
>> + }
>> +
>> + l2_vbase = syscon_regmap_lookup_by_compatible("arm,pl310-cache");
>> + if (IS_ERR(l2_vbase)) {
>> + edac_printk(KERN_ERR, EDAC_DEVICE,
>> + "L2:regmap for arm,pl310-cache lookup failed.\n");
>> + return -ENODEV;
>> + }
>
> I must NAK any use of the L2 as a syscon device. It's simply not a
> register file that was intended to be shared. I appreciate that you only
> want to check something very simple, but we should use a higher level
> API for that (and we should add one if we do not already have one)
>

OK. I didn't know of a way to read that register otherwise. I will
rework this.

>> +
>> + regmap_read(l2_vbase, ALTR_MPUL2_CONTROL_OFFSET, &control);
>> + if (!(control & ALTR_MPUL2_CTL_CACHE_EN_MASK)) {
>> + edac_printk(KERN_ERR, EDAC_DEVICE, "L2: Cache disabled\n");
>> + return -ENODEV;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +const struct edac_device_prv_data l2ecc_data = {
>> + .setup = altr_l2_dependencies,
>> + .ce_clear_mask = 0,
>> + .ue_clear_mask = 0,
>> +#ifdef CONFIG_EDAC_DEBUG
>> + .eccmgr_sysfs_attr = altr_l2_sysfs_attributes,
>> + .alloc_mem = l2_alloc_mem,
>> + .free_mem = l2_free_mem,
>> + .ecc_enable_mask = ALTR_L2_ECC_EN_MASK,
>> + .ce_set_mask = (ALTR_L2_ECC_EN_MASK | ALTR_L2_ECC_INJS_MASK),
>> + .ue_set_mask = (ALTR_L2_ECC_EN_MASK | ALTR_L2_ECC_INJD_MASK),
>> + .trig_alloc_sz = ALTR_TRIG_L2C_BYTE_SIZE,
>> +#endif
>> +};
>> +
>> +#endif /* #ifdef CONFIG_EDAC_ALTERA_L2C */
>> +
>> MODULE_LICENSE("GPL v2");
>> MODULE_AUTHOR("Thor Thayer");
>> -MODULE_DESCRIPTION("EDAC Driver for Altera SDRAM Controller");
>> +MODULE_DESCRIPTION("EDAC Driver for Altera Memories");
>> --
>> 1.7.9.5
>>
>>

2014-12-02 17:56:42

by Thor Thayer

[permalink] [raw]

Subject: Re: [PATCHv5 5/5] arm: dts: Add Altera L2 Cache and OCRAM EDAC

On 12/02/2014 08:57 AM, Mark Rutland wrote:
> On Wed, Nov 12, 2014 at 12:14:23AM +0000, [email protected] wrote:
>> From: Thor Thayer <[email protected]>
>>
>> Adding the device tree entries and bindings needed to support
>> the Altera L2 cache and On-Chip RAM EDAC. This patch relies upon
>> an earlier patch to declare and setup On-chip RAM properly.
>> http://www.spinics.net/lists/devicetree/msg51117.html
>>
>> Signed-off-by: Thor Thayer <[email protected]>

<snip>

>> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-l2-edac.txt
>> @@ -0,0 +1,15 @@
>> +Altera SoCFPGA L2 cache Error Detection and Correction [EDAC]
>> +
>> +Required Properties:
>> +- compatible : Should be "altr,l2-edac"
>> +- reg : Address and size for ECC error interrupt clear registers.
>> +- interrupts : Should be single bit error interrupt, then double bit error
>> + interrupt. Note the rising edge type.
>> +
>> +Example:
>> +
>> + l2edac@ffd08140 {
>> + compatible = "altr,l2-edac";
>> + reg = <0xffd08140 0x4>;
>> + interrupts = <0 36 1>, <0 37 1>;
>> + };
>
> Judging by the size of the reg entry, this is part of a larger block
> (the same one the OCRAM EDAC lives in). Why isn't that larger block
> described?
>
> EDAC is a Linux subsystem name, but typically not the HW block name.
> What HW block does this live in?
>

Yes, this register is part of the ECC block of registers. In order to
get each probe function to run, the L2 EDAC and OCRAM EDAC had to be at
the top level of the device tree. If they are children of a parent node,
the probe functions aren't executed.
Is there a better way or example I should follow?

>> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt
>> new file mode 100644
>> index 0000000..31ab205
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt

<snip>

>> +
>> L2: l2-cache@fffef000 {
>> - compatible = "arm,pl310-cache";
>> + compatible = "arm,pl310-cache", "syscon";
>
> NAK.
>
> Why are you marking the PL310 as a syscon device? It is most definitely
> _NOT_ a shared set of registers lumped together.
>

Unfortunately, the register is locked for exclusive access by the L2
cache driver. I read your comment on patch 4 and will find a better way.

> Thanks,
> Mark.
>