Re: [PATCHv3 3/3] edac: altera: Add EDAC support for Altera SDRAM

[Thor Thayer]

On Thu, May 8, 2014 at 7:05 AM, Borislav Petkov <bp@xxxxxxxxx> wrote:
> On Mon, May 05, 2014 at 05:52:17PM -0500, tthayer@xxxxxxxxxx wrote:
>> From: Thor Thayer <tthayer@xxxxxxxxxx>
>
> Missing commit message.

Whoops. I don't know what happened there. I'll fix it.

>
>> ---
>> v2: Use the SDRAM controller registers to calculate memory size
>>     instead of the Device Tree. Update To & Cc list. Add maintainer
>>     information.
>>
>> v3: EDAC driver cleanup based on comments from Mailing list.
>>
>> Signed-off-by: Thor Thayer <tthayer@xxxxxxxxxx>
>> ---
>>  MAINTAINERS                |    5 +
>>  drivers/edac/Kconfig       |    9 +
>>  drivers/edac/Makefile      |    2 +
>>  drivers/edac/altera_edac.c |  411 ++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 427 insertions(+)
>>  create mode 100644 drivers/edac/altera_edac.c
>>

<snip>

>> --- /dev/null
>> +++ b/drivers/edac/altera_edac.c
>> @@ -0,0 +1,411 @@
>> +/*
>> + *  Copyright Altera Corporation (C) 2014. All rights reserved.
>> + *  Copyright 2011-2012 Calxeda, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + *
>> + * This file is subject to the terms and conditions of the GNU General Public
>> + * License.  See the file "COPYING" in the main directory of this archive
>> + * for more details.
>
> I'm guessing your lawyers want this boilerplate after all?
>

Yes. Their reasoning is that they want to retain the rights and
warranty language with the file (just in case the COPYING file
changes).

> ...
>
>> +static irqreturn_t altr_sdram_mc_err_handler(int irq, void *dev_id)
>> +{
>> +     struct mem_ctl_info *mci = dev_id;
>> +     struct altr_sdram_mc_data *drvdata = mci->pvt_info;
>> +     u32 status = 0, err_count = 0, err_addr = 0;
>> +
>> +     /* Error Address is shared by both SBE & DBE */
>> +     regmap_read(drvdata->mc_vbase, ERRADDR, &err_addr);
>> +
>> +     regmap_read(drvdata->mc_vbase, DRAMSTS, &status);
>> +
>> +     if (status & DRAMSTS_DBEERR) {
>> +             regmap_read(drvdata->mc_vbase, DBECOUNT, &err_count);
>> +             edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, err_count,
>> +                                  err_addr >> PAGE_SHIFT,
>> +                                  err_addr & ~PAGE_MASK, 0,
>> +                                  0, 0, -1, mci->ctl_name, "");
>
> So, are we setting edac_mc_panic_on_ue now or what are we planning to do
> for uncorrectable errors?

Yes. I tested using edac_core.edac_mc_panic_on_ue=1 from the command
line and it worked fine. I'll add a comment to clarify. BTW, thanks
for your help on that.

>
>> +     if (status & DRAMSTS_SBEERR) {
>> +             regmap_read(drvdata->mc_vbase, SBECOUNT, &err_count);

<snip>

>> +     if (count == 3) {
>> +             edac_printk(KERN_ALERT, EDAC_MC,
>> +                         "EDAC Inject Double bit error\n");
>> +             regmap_write(drvdata->mc_vbase, CTLCFG,
>> +                          (read_reg | CTLCFG_GEN_DB_ERR));
>> +     } else {
>> +             edac_printk(KERN_ALERT, EDAC_MC,
>> +                         "EDAC Inject Single bit error\n");
>> +             regmap_write(drvdata->mc_vbase, CTLCFG,
>> +                          (read_reg | CTLCFG_GEN_SB_ERR));
>
> You can remove the "EDAC " string from those printks above as
> edac_printk already adds the prefix.

Great. Will do.

>
>> +     }
>> +
>> +     ptemp[0] = 0x5A5A5A5A;
>> +     ptemp[1] = 0xA5A5A5A5;
>> +     regmap_write(drvdata->mc_vbase, CTLCFG, read_reg);
>> +     /* Ensure it has been written out */
>> +     wmb();
>> +
>> +     reg = ptemp[0];
>> +     read_reg = ptemp[1];
>> +     /* Force Read */
>> +     rmb();
>
> Have you checked whether those reads don't get optimized away by the
> compiler? I know, you need them for triggering the error condition.
>
> Also, you should add a comment here to explain why the reads are being
> done.

I considered using "volatile" variables, but decided against it after
I read Documentation/volatile-considered-harmful.txt and my situation
doesn't fit into the exemptions. Is there a better way to handle this?

I'll add the comment.

Thanks for reviewing and your comments.

Thor

>
> --
> Regards/Gruss,
>     Boris.
>
> Sent from a fat crate under my desk. Formatting is fine.
> --
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/