On Wed, Jan 27, 2016 at 10:13:20AM -0600, tthayer@xxxxxxxxxxxxxxxxxxxxx wrote: > From: Thor Thayer <tthayer@xxxxxxxxxxxxxxxxxxxxx> > > Adding L2 Cache and On-Chip RAM EDAC support for the > Altera SoCs using the EDAC device model. The SDRAM > controller is using the Memory Controller model. > > Each type of ECC is individually configurable. > > Signed-off-by: Thor Thayer <tthayer@xxxxxxxxxxxxxxxxxxxxx> > --- > v9: Improve device tree node release. Free managed resources > on error path. Fix ocram memory leak. > v8: Remove MASK from single bit mask names. > s/altr,edac/altr,socfpga-ecc-manager > Use debugfs instead of sysfs. > Add chip family name to match string. > Fix header year. > Fix build dependencies & change commit accordingly. > s/CONFIG_EDAC_ALTERA_MC/CONFIG_EDAC_ALTERA > v7: s/of_get_named_gen_pool/of_gen_pool_get > Remove #ifdef for EDAC_DEBUG > Use -ENODEV instead of EPROBE_DEFER > v6: Convert to nested EDAC in device tree. Force L2 cache > on for L2Cache ECC & remove L2 cache syscon for checking > enable bit. Update year in header. > v5: No change. > v4: Change mask defines to use BIT(). > Fix comment style to agree with kernel coding style. > Better printk description for read != write in trigger. > Remove SysFS debugging message. > Better dci->mod_name > Move gen_pool pointer assignment to end of function. > Invert logic to reduce indent in ocram depenency check. > Change from dev_err() to edac_printk() > Replace magic numbers with defines & comments. > Improve error injection test. > Change Makefile intermediary name to altr (from alt) > v3: Move OCRAM and L2 cache EDAC functions into altera_edac.c > instead of separate files. > v2: Fix L2 dependency comments. > --- > drivers/edac/Kconfig | 26 ++- > drivers/edac/Makefile | 2 +- > drivers/edac/altera_edac.c | 487 +++++++++++++++++++++++++++++++++++++++++++- > 3 files changed, 507 insertions(+), 8 deletions(-) I'm still waiting for the people on CC to confirm the DT changes. A couple of comments on the EDAC bits below. .... > +static irqreturn_t altr_edac_device_handler(int irq, void *dev_id) > +{ > + struct edac_device_ctl_info *dci = dev_id; > + struct altr_edac_device_dev *drvdata = dci->pvt_info; > + const struct edac_device_prv_data *priv = drvdata->data; > + > + if (irq == drvdata->sb_irq) { > + if (priv->ce_clear_mask) > + writel(priv->ce_clear_mask, drvdata->base); > + edac_device_handle_ce(dci, 0, 0, drvdata->edac_dev_name); > + } > + if (irq == drvdata->db_irq) { > + if (priv->ue_clear_mask) > + writel(priv->ue_clear_mask, drvdata->base); > + edac_device_handle_ue(dci, 0, 0, drvdata->edac_dev_name); > + panic("\nEDAC:ECC_DEVICE[Uncorrectable errors]\n"); > + } And irq is guaranteed to always be ->sb_irq or ->db_irq? Otherwise, you can do else WARN_ON(1); just in case. > + > + return IRQ_HANDLED; > +} ... > +/* > + * altr_edac_device_probe() > + * This is a generic EDAC device driver that will support > + * various Altera memory devices such as the L2 cache ECC and > + * OCRAM ECC as well as the memories for other peripherals. > + * Module specific initialization is done by passing the > + * function index in the device tree. > + */ > +static int altr_edac_device_probe(struct platform_device *pdev) > +{ > + struct edac_device_ctl_info *dci; > + struct altr_edac_device_dev *drvdata; > + struct resource *r; > + int res = 0; > + struct device_node *np = pdev->dev.of_node; > + char *ecc_name = (char *)np->name; > + static int dev_instance; > + struct dentry *debugfs; > + > + if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) { > + edac_printk(KERN_ERR, EDAC_DEVICE, > + "Unable to open devm\n"); > + return -ENOMEM; > + } > + > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!r) { > + edac_printk(KERN_ERR, EDAC_DEVICE, > + "Unable to get mem resource\n"); > + res = -ENODEV; > + goto fail; > + } > + > + if (!devm_request_mem_region(&pdev->dev, r->start, resource_size(r), > + dev_name(&pdev->dev))) { > + edac_printk(KERN_ERR, EDAC_DEVICE, > + "%s:Error requesting mem region\n", ecc_name); > + res = -EBUSY; > + goto fail; > + } > + > + dci = edac_device_alloc_ctl_info(sizeof(*drvdata), ecc_name, > + 1, ecc_name, 1, 0, NULL, 0, > + dev_instance++); > + > + if (!dci) { > + edac_printk(KERN_ERR, EDAC_DEVICE, > + "%s: Unable to allocate EDAC device\n", ecc_name); > + res = -ENOMEM; > + goto fail; > + } > + > + drvdata = dci->pvt_info; > + dci->dev = &pdev->dev; > + platform_set_drvdata(pdev, dci); > + drvdata->edac_dev_name = ecc_name; > + > + drvdata->base = devm_ioremap(&pdev->dev, r->start, resource_size(r)); > + if (!drvdata->base) > + goto fail1; > + > + /* Get driver specific data for this EDAC device */ > + drvdata->data = of_match_node(altr_edac_device_of_match, np)->data; > + > + /* Check specific dependencies for the module */ > + if (drvdata->data->setup) { > + res = drvdata->data->setup(pdev, drvdata->base); > + if (res < 0) > + goto fail1; > + } > + > + drvdata->sb_irq = platform_get_irq(pdev, 0); > + res = devm_request_irq(&pdev->dev, drvdata->sb_irq, > + altr_edac_device_handler, > + 0, dev_name(&pdev->dev), dci); > + if (res < 0) > + goto fail1; > + > + drvdata->db_irq = platform_get_irq(pdev, 1); > + res = devm_request_irq(&pdev->dev, drvdata->db_irq, > + altr_edac_device_handler, > + 0, dev_name(&pdev->dev), dci); > + if (res < 0) > + goto fail1; > + > + dci->mod_name = "Altera ECC Manager"; > + dci->dev_name = drvdata->edac_dev_name; > + > + debugfs = edac_debugfs_create_dir(ecc_name); > + if (debugfs) > + altr_create_edacdev_dbgfs(dci, drvdata->data, debugfs); > + > + if (edac_device_add_device(dci)) <--- if you end up here and debugfs nodes have been created, you need to destroy them here. You probably could change edac_debugfs_exit() to call debugfs_remove_recursive() and make sure your driver calls it. Right now we're calling edac_debugfs_exit() in edac_exit() and I *think* that is ok for a platform device driver as this one but I haven't actually *verified* that. > + goto fail1; > + > + devres_close_group(&pdev->dev, NULL); > + > + return 0; > + > +fail1: > + edac_device_free_ctl_info(dci); > +fail: > + devres_release_group(&pdev->dev, NULL); > + edac_printk(KERN_ERR, EDAC_DEVICE, > + "%s:Error setting up EDAC device: %d\n", ecc_name, res); > + > + return res; > +} > + > +static int altr_edac_device_remove(struct platform_device *pdev) > +{ > + struct edac_device_ctl_info *dci = platform_get_drvdata(pdev); > + > + edac_device_del_device(&pdev->dev); > + edac_device_free_ctl_info(dci); > + > + return 0; > +} > + > +static struct platform_driver altr_edac_device_driver = { > + .probe = altr_edac_device_probe, > + .remove = altr_edac_device_remove, > + .driver = { > + .name = "altr_edac_device", > + .of_match_table = altr_edac_device_of_match, > + }, > +}; > +module_platform_driver(altr_edac_device_driver); > + > +/*********************** OCRAM EDAC Device Functions *********************/ > + > +#ifdef CONFIG_EDAC_ALTERA_OCRAM > + > +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,socfpga-ocram-ecc"); > + if (!np) > + return NULL; > + > + gp = of_gen_pool_get(np, "iram", 0); > + of_node_put(np); > + if (!gp) > + return NULL; > + > + sram_addr = (void *)gen_pool_alloc(gp, size); > + if (!sram_addr) > + return NULL; > + > + memset(sram_addr, 0, size); > + wmb(); /* Ensure data is written out */ > + > + *other = gp; /* Remember this handle for freeing later */ Please put comments ontop of the line they're referring to. Those side-things are cluttering the code unnecessarily. > + > + return sram_addr; > +} > + > +static void ocram_free_mem(void *p, size_t size, void *other) > +{ > + gen_pool_free((struct gen_pool *)other, (u32)p, size); > +} > + > +/* > + * altr_ocram_dependencies() > + * Test for OCRAM cache ECC dependencies upon entry because > + * platform specific startup should have initialized the > + * On-Chip RAM memory and enabled the ECC. > + * Can't turn on ECC here because accessing un-initialized > + * memory will cause CE/UE errors possibly causing an ABORT. > + */ > +static int altr_ocram_dependencies(struct platform_device *pdev, A verb in the name? altr_ocram_test_deps or altr_ocram_check_deps or so? > + void __iomem *base) > +{ > + u32 control; > + > + control = readl(base) & ALTR_OCR_ECC_EN; > + if (control) > + return 0; > + > + edac_printk(KERN_ERR, EDAC_DEVICE, > + "OCRAM: No ECC present or ECC disabled.\n"); > + return -ENODEV; > +} > + > +const struct edac_device_prv_data ocramecc_data = { > + .setup = altr_ocram_dependencies, > + .ce_clear_mask = (ALTR_OCR_ECC_EN | ALTR_OCR_ECC_SERR), > + .ue_clear_mask = (ALTR_OCR_ECC_EN | ALTR_OCR_ECC_DERR), > + .dbgfs_name = "altr_ocram_trigger", > + .alloc_mem = ocram_alloc_mem, > + .free_mem = ocram_free_mem, > + .ecc_enable_mask = ALTR_OCR_ECC_EN, > + .ce_set_mask = (ALTR_OCR_ECC_EN | ALTR_OCR_ECC_INJS), > + .ue_set_mask = (ALTR_OCR_ECC_EN | ALTR_OCR_ECC_INJD), > + .trig_alloc_sz = ALTR_TRIG_OCRAM_BYTE_SIZE, > +}; > + > +#endif /* CONFIG_EDAC_ALTERA_OCRAM */ > + > +/********************* L2 Cache EDAC Device Functions ********************/ > + > +#ifdef CONFIG_EDAC_ALTERA_L2C > + > +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(); See, it reads much better with the comment ontop. :) > + > + /* > + * Clean all cache levels up to LoC (includes L2) > + * This ensures the corrupted data is written into > + * L2 cache for readback test (which causes ECC error). > + */ > + flush_cache_all(); > + > + return ptemp; > +} > + > +static void l2_free_mem(void *p, size_t size, void *other) > +{ > + struct device *dev = other; > + > + if (dev && p) > + devm_kfree(dev, p); > +} > + > +/* > + * altr_l2_dependencies() > + * Test for L2 cache ECC dependencies upon entry because > + * platform specific startup should have initialized the L2 > + * memory and enabled the ECC. > + * Bail if ECC is not enabled. > + * Note that L2 Cache Enable is forced at build time. > + */ > +static int altr_l2_dependencies(struct platform_device *pdev, Here too, pls add a verb in the function name. > + void __iomem *base) > +{ > + u32 control; > + > + control = readl(base) & ALTR_L2_ECC_EN; > + if (!control) { > + edac_printk(KERN_ERR, EDAC_DEVICE, > + "L2: No ECC present, or ECC disabled\n"); > + return -ENODEV; > + } > + > + return 0; > +} -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- 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