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 devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html