* [PATCH v5 2/2] EDAC: add EDAC driver for DMC520
@ 2019-07-25 0:49 Lei Wang
2019-08-07 14:40 ` Borislav Petkov
0 siblings, 1 reply; 6+ messages in thread
From: Lei Wang @ 2019-07-25 0:49 UTC (permalink / raw)
To: bp, james.morse, robh+dt, mark.rutland
Cc: devicetree, linux-kernel, mchehab, linux-edac, sashal, hangl,
lewan, ruizhao, Lei Wang
New driver supports error detection and correction on the devices with ARM
DMC-520 memory controller.
Signed-off-by: Lei Wang <leiwang_git@outlook.com>
Reviewed-by: James Morse <james.morse@arm.com>
---
Changes in v5:
- When enable configured interrupts, do not potentially disable
other irrelevant interrupts.
- fix return type of dmc520_get_scrub_type to enum scrub_type
- Use capital letters for enum
- Retrieve memory width from dmc register format_control
- use cheaper spin_lock API.
- Remove compatible line for "brcm,dmc-520"
- Add James' Reviewed-by
---
MAINTAINERS | 6 +
drivers/edac/Kconfig | 7 +
drivers/edac/Makefile | 1 +
drivers/edac/dmc520_edac.c | 632 +++++++++++++++++++++++++++++++++++++
4 files changed, 646 insertions(+)
create mode 100644 drivers/edac/dmc520_edac.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 77eae44bf5de..7c1ac8bc8ea1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5703,6 +5703,12 @@ F: Documentation/driver-api/edac.rst
F: drivers/edac/
F: include/linux/edac.h
+EDAC-DMC520
+M: Lei Wang <lewan@microsoft.com>
+L: linux-edac@vger.kernel.org
+S: Supported
+F: drivers/edac/dmc520_edac.c
+
EDAC-E752X
M: Mark Gross <mark.gross@intel.com>
L: linux-edac@vger.kernel.org
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 200c04ce5b0e..7fde5aea0c1a 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -510,4 +510,11 @@ config EDAC_ASPEED
First, ECC must be configured in the bootloader. Then, this driver
will expose error counters via the EDAC kernel framework.
+config EDAC_DMC520
+ tristate "ARM DMC-520 ECC"
+ depends on ARM64
+ help
+ Support for error detection and correction on the
+ SoCs with ARM DMC-520 DRAM controller.
+
endif # EDAC
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index 165ca65e1a3a..e0d98f5b2045 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -85,3 +85,4 @@ obj-$(CONFIG_EDAC_XGENE) += xgene_edac.o
obj-$(CONFIG_EDAC_TI) += ti_edac.o
obj-$(CONFIG_EDAC_QCOM) += qcom_edac.o
obj-$(CONFIG_EDAC_ASPEED) += aspeed_edac.o
+obj-$(CONFIG_EDAC_DMC520) += dmc520_edac.o
diff --git a/drivers/edac/dmc520_edac.c b/drivers/edac/dmc520_edac.c
new file mode 100644
index 000000000000..aeb8d84405cf
--- /dev/null
+++ b/drivers/edac/dmc520_edac.c
@@ -0,0 +1,632 @@
+// SPDX-License-Identifier: GPL-2.0
+/* EDAC driver for DMC-520 */
+
+
+#include <linux/bitfield.h>
+#include <linux/edac.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include "edac_mc.h"
+
+/* DMC-520 registers */
+#define REG_OFFSET_FEATURE_CONFIG 0x130
+#define REG_OFFSET_ECC_ERRC_COUNT_31_00 0x158
+#define REG_OFFSET_ECC_ERRC_COUNT_63_32 0x15C
+#define REG_OFFSET_ECC_ERRD_COUNT_31_00 0x160
+#define REG_OFFSET_ECC_ERRD_COUNT_63_32 0x164
+#define REG_OFFSET_INTERRUPT_CONTROL 0x500
+#define REG_OFFSET_INTERRUPT_CLR 0x508
+#define REG_OFFSET_INTERRUPT_STATUS 0x510
+#define REG_OFFSET_DRAM_ECC_ERRC_INT_INFO_31_00 0x528
+#define REG_OFFSET_DRAM_ECC_ERRC_INT_INFO_63_32 0x52C
+#define REG_OFFSET_DRAM_ECC_ERRD_INT_INFO_31_00 0x530
+#define REG_OFFSET_DRAM_ECC_ERRD_INT_INFO_63_32 0x534
+#define REG_OFFSET_ADDRESS_CONTROL_NOW 0x1010
+#define REG_OFFSET_MEMORY_TYPE_NOW 0x1128
+#define REG_OFFSET_SCRUB_CONTROL0_NOW 0x1170
+#define REG_OFFSET_FORMAT_CONTROL 0x18
+
+/* DMC-520 types, masks and bitfields */
+#define DRAM_ECC_INT_CE_BIT BIT(2)
+#define DRAM_ECC_INT_UE_BIT BIT(3)
+#define ALL_INT_MASK GENMASK(9, 0)
+#define MEMORY_WIDTH_MASK GENMASK(1, 0)
+#define SCRUB_TRIGGER0_NEXT_MASK GENMASK(1, 0)
+#define REG_FIELD_DRAM_ECC_ENABLED GENMASK(1, 0)
+#define REG_FIELD_MEMORY_TYPE GENMASK(2, 0)
+#define REG_FIELD_DEVICE_WIDTH GENMASK(9, 8)
+#define REG_FIELD_ADDRESS_CONTROL_COL GENMASK(2, 0)
+#define REG_FIELD_ADDRESS_CONTROL_ROW GENMASK(10, 8)
+#define REG_FIELD_ADDRESS_CONTROL_BANK GENMASK(18, 16)
+#define REG_FIELD_ADDRESS_CONTROL_RANK GENMASK(25, 24)
+#define REG_FIELD_ERR_INFO_LOW_VALID BIT(0)
+#define REG_FIELD_ERR_INFO_LOW_COL GENMASK(10, 1)
+#define REG_FIELD_ERR_INFO_LOW_ROW GENMASK(28, 11)
+#define REG_FIELD_ERR_INFO_LOW_RANK GENMASK(31, 29)
+#define REG_FIELD_ERR_INFO_HIGH_BANK GENMASK(3, 0)
+#define REG_FIELD_ERR_INFO_HIGH_VALID BIT(31)
+
+#define DRAM_ADDRESS_CONTROL_MIN_COL_BITS 8
+#define DRAM_ADDRESS_CONTROL_MIN_ROW_BITS 11
+
+#define DMC520_SCRUB_TRIGGER_ERR_DETECT 2
+#define DMC520_SCRUB_TRIGGER_IDLE 3
+
+/* Driver settings */
+/* The max-length message would be: "rank:7 bank:15 row:262143 col:1023".
+ * Max length is 34. Using a 40-size buffer is enough.
+ */
+#define EDAC_MSG_BUF_SIZE 40
+#define EDAC_MOD_NAME "dmc520-edac"
+#define EDAC_CTL_NAME "dmc520"
+
+/* the data bus width for the attached memory chips. */
+enum dmc520_mem_width {
+ MEM_WIDTH_X32 = 2,
+ MEM_WIDTH_X64 = 3
+};
+
+/* memory type */
+enum dmc520_mem_type {
+ MEM_TYPE_DDR3 = 1,
+ MEM_TYPE_DDR4 = 2
+};
+
+/* memory device width */
+enum dmc520_dev_width {
+ DEV_WIDTH_X4 = 0,
+ DEV_WIDTH_X8 = 1,
+ DEV_WIDTH_X16 = 2
+};
+
+struct ecc_error_info {
+ u32 col;
+ u32 row;
+ u32 bank;
+ u32 rank;
+};
+
+/* The EDAC driver private data */
+struct dmc520_edac {
+ void __iomem *reg_base;
+ u32 nintr;
+ u32 interrupt_mask_all;
+ spinlock_t ecc_lock;
+ u32 interrupt_masks[0];
+};
+
+static int dmc520_mc_idx;
+
+static irqreturn_t
+dmc520_edac_dram_all_isr(int irq, struct mem_ctl_info *mci, u32 interrupt_mask);
+
+#define DECLARE_ISR(index) \
+static irqreturn_t dmc520_isr_##index (int irq, void *data) \
+{ \
+ struct mem_ctl_info *mci; \
+ struct dmc520_edac *edac; \
+ mci = data; \
+ edac = mci->pvt_info; \
+ return dmc520_edac_dram_all_isr(irq, mci, edac->interrupt_masks[index]); \
+}
+
+DECLARE_ISR(0)
+DECLARE_ISR(1)
+/* More DECLARE_ISR(index) can be added to support more interrupt lines. */
+
+irq_handler_t dmc520_isr_array[] = {
+ dmc520_isr_0,
+ dmc520_isr_1
+ /* More dmc520_isr_index can be added to support more interrupt lines. */
+};
+
+static u32 dmc520_read_reg(struct dmc520_edac *edac, u32 offset)
+{
+ return readl(edac->reg_base + offset);
+}
+
+static void dmc520_write_reg(struct dmc520_edac *edac, u32 val, u32 offset)
+{
+ writel(val, edac->reg_base + offset);
+}
+
+static u32 dmc520_calc_dram_ecc_error(u32 value)
+{
+ u32 total = 0;
+
+ /* Each rank's error counter takes one byte */
+ while (value > 0) {
+ total += (value & 0xFF);
+ value >>= 8;
+ }
+ return total;
+}
+
+static u32 dmc520_get_dram_ecc_error_count(struct dmc520_edac *edac,
+ bool is_ce)
+{
+ u32 reg_offset_low, reg_offset_high;
+ u32 err_low, err_high;
+ u32 err_count;
+
+ reg_offset_low = is_ce ? REG_OFFSET_ECC_ERRC_COUNT_31_00 :
+ REG_OFFSET_ECC_ERRD_COUNT_31_00;
+ reg_offset_high = is_ce ? REG_OFFSET_ECC_ERRC_COUNT_63_32 :
+ REG_OFFSET_ECC_ERRD_COUNT_63_32;
+
+ err_low = dmc520_read_reg(edac, reg_offset_low);
+ err_high = dmc520_read_reg(edac, reg_offset_high);
+ /* Reset error counters */
+ dmc520_write_reg(edac, 0, reg_offset_low);
+ dmc520_write_reg(edac, 0, reg_offset_high);
+
+ err_count = dmc520_calc_dram_ecc_error(err_low) +
+ dmc520_calc_dram_ecc_error(err_high);
+
+ return err_count;
+}
+
+static void dmc520_get_dram_ecc_error_info(struct dmc520_edac *edac,
+ bool is_ce,
+ struct ecc_error_info *info)
+{
+ u32 reg_offset_low, reg_offset_high;
+ u32 reg_val_low, reg_val_high;
+ bool valid;
+
+ reg_offset_low = is_ce ? REG_OFFSET_DRAM_ECC_ERRC_INT_INFO_31_00 :
+ REG_OFFSET_DRAM_ECC_ERRD_INT_INFO_31_00;
+ reg_offset_high = is_ce ? REG_OFFSET_DRAM_ECC_ERRC_INT_INFO_63_32 :
+ REG_OFFSET_DRAM_ECC_ERRD_INT_INFO_63_32;
+
+ reg_val_low = dmc520_read_reg(edac, reg_offset_low);
+ reg_val_high = dmc520_read_reg(edac, reg_offset_high);
+
+ valid = (FIELD_GET(REG_FIELD_ERR_INFO_LOW_VALID, reg_val_low) != 0) &&
+ (FIELD_GET(REG_FIELD_ERR_INFO_HIGH_VALID, reg_val_high) != 0);
+
+ if (valid) {
+ info->col = FIELD_GET(REG_FIELD_ERR_INFO_LOW_COL, reg_val_low);
+ info->row = FIELD_GET(REG_FIELD_ERR_INFO_LOW_ROW, reg_val_low);
+ info->rank = FIELD_GET(REG_FIELD_ERR_INFO_LOW_RANK, reg_val_low);
+ info->bank = FIELD_GET(REG_FIELD_ERR_INFO_HIGH_BANK, reg_val_high);
+ } else {
+ memset(info, 0, sizeof(struct ecc_error_info));
+ }
+}
+
+static bool dmc520_is_ecc_enabled(void __iomem *reg_base)
+{
+ u32 reg_val = readl(reg_base + REG_OFFSET_FEATURE_CONFIG);
+
+ return (FIELD_GET(REG_FIELD_DRAM_ECC_ENABLED, reg_val) != 0);
+}
+
+static enum scrub_type dmc520_get_scrub_type(struct dmc520_edac *edac)
+{
+ enum scrub_type type = SCRUB_NONE;
+ u32 reg_val, scrub_cfg;
+
+ reg_val = dmc520_read_reg(edac, REG_OFFSET_SCRUB_CONTROL0_NOW);
+ scrub_cfg = FIELD_GET(SCRUB_TRIGGER0_NEXT_MASK, reg_val);
+
+ if (DMC520_SCRUB_TRIGGER_ERR_DETECT == scrub_cfg ||
+ DMC520_SCRUB_TRIGGER_IDLE == scrub_cfg)
+ type = SCRUB_HW_PROG;
+
+ return type;
+}
+
+/* Get the memory data bus width, in number of bytes. */
+static u32 dmc520_get_memory_width(struct dmc520_edac *edac)
+{
+ static u32 mem_width;
+ u32 reg_val;
+ enum dmc520_mem_width mem_width_field;
+
+ if (mem_width == 0) {
+ reg_val = dmc520_read_reg(edac, REG_OFFSET_FORMAT_CONTROL);
+ mem_width_field = FIELD_GET(MEMORY_WIDTH_MASK, reg_val);
+
+ if (mem_width_field == MEM_WIDTH_X32)
+ mem_width = 4; /* 32-bits, 4 bytes */
+ else if (mem_width_field == MEM_WIDTH_X64)
+ mem_width = 8; /* 64-bits, 8 bytes */
+ }
+
+ return mem_width;
+}
+
+static enum mem_type dmc520_get_mtype(struct dmc520_edac *edac)
+{
+ enum mem_type mt = MEM_UNKNOWN;
+ u32 reg_val;
+ enum dmc520_mem_type type;
+
+ reg_val = dmc520_read_reg(edac, REG_OFFSET_MEMORY_TYPE_NOW);
+ type = FIELD_GET(REG_FIELD_MEMORY_TYPE, reg_val);
+
+ switch (type) {
+ case MEM_TYPE_DDR3:
+ mt = MEM_DDR3;
+ break;
+
+ case MEM_TYPE_DDR4:
+ mt = MEM_DDR4;
+ break;
+ }
+
+ return mt;
+}
+
+static enum dev_type dmc520_get_dtype(struct dmc520_edac *edac)
+{
+ enum dev_type dt = DEV_UNKNOWN;
+ u32 reg_val;
+ enum dmc520_dev_width device_width;
+
+ reg_val = dmc520_read_reg(edac, REG_OFFSET_MEMORY_TYPE_NOW);
+ device_width = FIELD_GET(REG_FIELD_DEVICE_WIDTH, reg_val);
+
+ switch (device_width) {
+ case DEV_WIDTH_X4:
+ dt = DEV_X4;
+ break;
+
+ case DEV_WIDTH_X8:
+ dt = DEV_X8;
+ break;
+
+ case DEV_WIDTH_X16:
+ dt = DEV_X16;
+ break;
+ }
+
+ return dt;
+}
+
+static u32 dmc520_get_rank_count(void __iomem *reg_base)
+{
+ u32 reg_val, rank_bits;
+
+ reg_val = readl(reg_base + REG_OFFSET_ADDRESS_CONTROL_NOW);
+ rank_bits = FIELD_GET(REG_FIELD_ADDRESS_CONTROL_RANK, reg_val);
+
+ return (1 << rank_bits);
+}
+
+static u64 dmc520_get_rank_size(struct dmc520_edac *edac)
+{
+ u32 reg_val, col_bits, row_bits, bank_bits;
+
+ reg_val = dmc520_read_reg(edac, REG_OFFSET_ADDRESS_CONTROL_NOW);
+
+ col_bits = FIELD_GET(REG_FIELD_ADDRESS_CONTROL_COL, reg_val) +
+ DRAM_ADDRESS_CONTROL_MIN_COL_BITS;
+ row_bits = FIELD_GET(REG_FIELD_ADDRESS_CONTROL_ROW, reg_val) +
+ DRAM_ADDRESS_CONTROL_MIN_ROW_BITS;
+ bank_bits = FIELD_GET(REG_FIELD_ADDRESS_CONTROL_BANK, reg_val);
+
+ return (u64)dmc520_get_memory_width(edac) << (col_bits + row_bits + bank_bits);
+}
+
+static void dmc520_handle_dram_ecc_errors(struct mem_ctl_info *mci,
+ bool is_ce)
+{
+ struct ecc_error_info info;
+ struct dmc520_edac *edac;
+ u32 cnt;
+ char message[EDAC_MSG_BUF_SIZE];
+
+ edac = mci->pvt_info;
+ dmc520_get_dram_ecc_error_info(edac, is_ce, &info);
+
+ cnt = dmc520_get_dram_ecc_error_count(edac, is_ce);
+
+ if (cnt > 0) {
+ snprintf(message, ARRAY_SIZE(message),
+ "rank:%d bank:%d row:%d col:%d",
+ info.rank, info.bank,
+ info.row, info.col);
+
+ spin_lock(&edac->ecc_lock);
+ edac_mc_handle_error((is_ce ? HW_EVENT_ERR_CORRECTED :
+ HW_EVENT_ERR_UNCORRECTED),
+ mci, cnt, 0, 0, 0, info.rank, -1, -1,
+ message, "");
+ spin_unlock(&edac->ecc_lock);
+ }
+}
+
+static irqreturn_t dmc520_edac_dram_ecc_isr(int irq, struct mem_ctl_info *mci, bool is_ce)
+{
+ u32 i_mask;
+ struct dmc520_edac *edac;
+
+ edac = mci->pvt_info;
+
+ i_mask = is_ce ? DRAM_ECC_INT_CE_BIT : DRAM_ECC_INT_UE_BIT;
+
+ dmc520_handle_dram_ecc_errors(mci, is_ce);
+
+ dmc520_write_reg(edac, i_mask, REG_OFFSET_INTERRUPT_CLR);
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t
+dmc520_edac_dram_all_isr(int irq, struct mem_ctl_info *mci, u32 interrupt_mask)
+{
+ struct dmc520_edac *edac;
+ u32 status;
+ irqreturn_t irq_ret = IRQ_NONE;
+
+ edac = mci->pvt_info;
+
+ status = dmc520_read_reg(edac, REG_OFFSET_INTERRUPT_STATUS);
+
+ if ((interrupt_mask & DRAM_ECC_INT_CE_BIT) &&
+ (status & DRAM_ECC_INT_CE_BIT))
+ irq_ret = dmc520_edac_dram_ecc_isr(irq, mci, true);
+
+ if ((interrupt_mask & DRAM_ECC_INT_UE_BIT) &&
+ (status & DRAM_ECC_INT_UE_BIT))
+ irq_ret = dmc520_edac_dram_ecc_isr(irq, mci, false);
+
+ /* If in the future there are more supported interrupts in a different
+ * platform, more condition statements can be added here for each
+ * interrupt flag, together with its corresponding isr implementations.
+ */
+
+ return irq_ret;
+}
+
+static void dmc520_init_csrow(struct mem_ctl_info *mci)
+{
+ struct csrow_info *csi;
+ struct dimm_info *dimm;
+ int row, ch;
+ enum dev_type dt;
+ enum mem_type mt;
+ u64 rs;
+ u32 pages_per_rank;
+ struct dmc520_edac *edac = mci->pvt_info;
+
+ dt = dmc520_get_dtype(edac);
+ mt = dmc520_get_mtype(edac);
+ rs = dmc520_get_rank_size(edac);
+ pages_per_rank = rs >> PAGE_SHIFT;
+
+ for (row = 0; row < mci->nr_csrows; row++) {
+ csi = mci->csrows[row];
+
+ for (ch = 0; ch < csi->nr_channels; ch++) {
+ dimm = csi->channels[ch]->dimm;
+ dimm->grain = dmc520_get_memory_width(edac);
+ dimm->dtype = dt;
+ dimm->mtype = mt;
+ dimm->edac_mode = EDAC_FLAG_SECDED;
+ dimm->nr_pages = pages_per_rank / csi->nr_channels;
+ }
+ }
+}
+
+static int dmc520_edac_probe(struct platform_device *pdev)
+{
+ struct device *dev;
+ struct dmc520_edac *edac;
+ struct mem_ctl_info *mci;
+ struct edac_mc_layer layers[1];
+ int ret, intr_index, nintr, nintr_registered = 0;
+ struct resource *res;
+ void __iomem *reg_base;
+ u32 reg_val;
+
+ /* Parsing the device node */
+ dev = &pdev->dev;
+
+ nintr = of_property_count_u32_elems(dev->of_node, "interrupt-config");
+ if (nintr <= 0) {
+ edac_printk(KERN_ERR, EDAC_MOD_NAME,
+ "Invalid device node configuration: at least one interrupt "
+ "line & config is expected.\n");
+ return -EINVAL;
+ }
+
+ if (nintr > ARRAY_SIZE(dmc520_isr_array)) {
+ edac_printk(KERN_ERR, EDAC_MOD_NAME,
+ "Invalid device node configuration: # of interrupt config "
+ "elements (%d) can not exceed %ld.\n",
+ nintr, ARRAY_SIZE(dmc520_isr_array));
+ return -EINVAL;
+ }
+
+ /* Initialize dmc520 edac */
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ reg_base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(reg_base))
+ return PTR_ERR(reg_base);
+
+ if (!dmc520_is_ecc_enabled(reg_base))
+ return -ENXIO;
+
+ layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
+ layers[0].size = dmc520_get_rank_count(reg_base);
+ layers[0].is_virt_csrow = true;
+
+ mci = edac_mc_alloc(dmc520_mc_idx++, ARRAY_SIZE(layers), layers,
+ sizeof(struct dmc520_edac) + sizeof(u32) * nintr);
+ if (!mci) {
+ edac_printk(KERN_ERR, EDAC_MOD_NAME,
+ "Failed to allocate memory for mc instance\n");
+ return -ENOMEM;
+ }
+
+ edac = mci->pvt_info;
+ edac->reg_base = reg_base;
+ edac->nintr = nintr;
+ edac->interrupt_mask_all = 0;
+ spin_lock_init(&edac->ecc_lock);
+
+ ret = of_property_read_u32_array(dev->of_node, "interrupt-config",
+ edac->interrupt_masks, nintr);
+ if (ret) {
+ edac_printk(KERN_ERR, EDAC_MOD_NAME,
+ "Failed to get interrupt-config arrays.\n");
+ goto err_free_mc;
+ }
+
+ for (intr_index = 0; intr_index < nintr; ++intr_index) {
+ if (edac->interrupt_mask_all & edac->interrupt_masks[intr_index]) {
+ edac_printk(KERN_ERR, EDAC_MC,
+ "interrupt-config error: "
+ "element %d's interrupt mask %d has overlap.\n",
+ intr_index, edac->interrupt_masks[intr_index]);
+ goto err_free_mc;
+ }
+
+ edac->interrupt_mask_all |= edac->interrupt_masks[intr_index];
+ }
+
+ if ((edac->interrupt_mask_all | ALL_INT_MASK) != ALL_INT_MASK) {
+ edac_printk(KERN_WARNING, EDAC_MOD_NAME,
+ "interrupt-config warning: "
+ "interrupt mask (0x%x) is not supported by dmc520 (0x%lx).\n",
+ edac->interrupt_mask_all, ALL_INT_MASK);
+ }
+ edac->interrupt_mask_all &= ALL_INT_MASK;
+
+ platform_set_drvdata(pdev, mci);
+
+ mci->pdev = dev;
+ mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR4;
+ mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
+ mci->edac_cap = EDAC_FLAG_SECDED;
+ mci->scrub_cap = SCRUB_FLAG_HW_SRC;
+ mci->scrub_mode = dmc520_get_scrub_type(edac);
+ mci->ctl_name = EDAC_CTL_NAME;
+ mci->dev_name = dev_name(mci->pdev);
+ mci->mod_name = EDAC_MOD_NAME;
+ mci->ctl_page_to_phys = NULL;
+
+ edac_op_state = EDAC_OPSTATE_INT;
+
+ dmc520_init_csrow(mci);
+
+ ret = edac_mc_add_mc(mci);
+ if (ret) {
+ edac_printk(KERN_ERR, EDAC_MOD_NAME,
+ "Failed to register with EDAC core\n");
+ goto err_free_mc;
+ }
+
+ /* Clear interrupts, not affecting other unrelated interrupts */
+ reg_val = dmc520_read_reg(edac, REG_OFFSET_INTERRUPT_CONTROL);
+ dmc520_write_reg(edac, reg_val & (~(edac->interrupt_mask_all)),
+ REG_OFFSET_INTERRUPT_CONTROL);
+ dmc520_write_reg(edac, edac->interrupt_mask_all,
+ REG_OFFSET_INTERRUPT_CLR);
+
+ for (intr_index = 0; intr_index < nintr; ++intr_index) {
+ int irq_id = platform_get_irq(pdev, intr_index);
+
+ if (irq_id < 0) {
+ edac_printk(KERN_ERR, EDAC_MC,
+ "Failed to get irq #%d\n", intr_index);
+ ret = -ENODEV;
+ goto err_free_irq;
+ }
+
+ ret = devm_request_irq(&pdev->dev, irq_id,
+ dmc520_isr_array[intr_index], IRQF_SHARED,
+ dev_name(&pdev->dev), mci);
+ if (ret < 0) {
+ edac_printk(KERN_ERR, EDAC_MC,
+ "Failed to request irq %d\n", irq_id);
+ goto err_free_irq;
+ }
+
+ ++nintr_registered;
+ }
+
+ /* Reset DRAM CE/UE counters */
+ if (edac->interrupt_mask_all & DRAM_ECC_INT_CE_BIT)
+ dmc520_get_dram_ecc_error_count(edac, true);
+
+ if (edac->interrupt_mask_all & DRAM_ECC_INT_UE_BIT)
+ dmc520_get_dram_ecc_error_count(edac, false);
+
+ /* Enable interrupts, not affecting other unrelated interrupts */
+ dmc520_write_reg(edac,
+ reg_val | edac->interrupt_mask_all,
+ REG_OFFSET_INTERRUPT_CONTROL);
+
+ return 0;
+
+err_free_irq:
+ for (intr_index = 0; intr_index < nintr_registered; ++intr_index) {
+ int irq_id = platform_get_irq(pdev, intr_index);
+
+ devm_free_irq(&pdev->dev, irq_id, mci);
+ }
+ edac_mc_del_mc(&pdev->dev);
+err_free_mc:
+ edac_mc_free(mci);
+
+ return ret;
+}
+
+static int dmc520_edac_remove(struct platform_device *pdev)
+{
+ struct dmc520_edac *edac;
+ struct mem_ctl_info *mci;
+ u32 reg_val, intr_index;
+
+ mci = platform_get_drvdata(pdev);
+ edac = mci->pvt_info;
+
+ /* Disable interrupts */
+ reg_val = dmc520_read_reg(edac, REG_OFFSET_INTERRUPT_CONTROL);
+ dmc520_write_reg(edac, reg_val & (~(edac->interrupt_mask_all)),
+ REG_OFFSET_INTERRUPT_CONTROL);
+
+ /* free irq's */
+ for (intr_index = 0; intr_index < edac->nintr; ++intr_index) {
+ int irq_id = platform_get_irq(pdev, intr_index);
+
+ devm_free_irq(&pdev->dev, irq_id, mci);
+ }
+
+ edac_mc_del_mc(&pdev->dev);
+ edac_mc_free(mci);
+
+ return 0;
+}
+
+static const struct of_device_id dmc520_edac_driver_id[] = {
+ { .compatible = "arm,dmc-520", },
+ { /* end of table */ }
+};
+
+MODULE_DEVICE_TABLE(of, dmc520_edac_driver_id);
+
+static struct platform_driver dmc520_edac_driver = {
+ .driver = {
+ .name = "dmc520",
+ .of_match_table = dmc520_edac_driver_id,
+ },
+
+ .probe = dmc520_edac_probe,
+ .remove = dmc520_edac_remove
+};
+
+module_platform_driver(dmc520_edac_driver);
+
+MODULE_AUTHOR(
+ "Rui Zhao <ruizhao@microsoft.com>, Lei Wang <lewan@microsoft.com>");
+MODULE_DESCRIPTION("DMC-520 ECC driver");
+MODULE_LICENSE("GPL v2");
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v5 2/2] EDAC: add EDAC driver for DMC520
2019-07-25 0:49 [PATCH v5 2/2] EDAC: add EDAC driver for DMC520 Lei Wang
@ 2019-08-07 14:40 ` Borislav Petkov
2019-08-13 1:05 ` Lei Wang
0 siblings, 1 reply; 6+ messages in thread
From: Borislav Petkov @ 2019-08-07 14:40 UTC (permalink / raw)
To: Lei Wang
Cc: james.morse, robh+dt, mark.rutland, devicetree, linux-kernel,
mchehab, linux-edac, sashal, hangl, lewan, ruizhao
On Thu, Jul 25, 2019 at 12:49:27AM +0000, Lei Wang wrote:
> New driver supports error detection and correction on the devices with ARM
> DMC-520 memory controller.
>
> Signed-off-by: Lei Wang <leiwang_git@outlook.com>
> Reviewed-by: James Morse <james.morse@arm.com>
> ---
> Changes in v5:
> - When enable configured interrupts, do not potentially disable
> other irrelevant interrupts.
> - fix return type of dmc520_get_scrub_type to enum scrub_type
> - Use capital letters for enum
> - Retrieve memory width from dmc register format_control
> - use cheaper spin_lock API.
> - Remove compatible line for "brcm,dmc-520"
> - Add James' Reviewed-by
> ---
> MAINTAINERS | 6 +
> drivers/edac/Kconfig | 7 +
> drivers/edac/Makefile | 1 +
> drivers/edac/dmc520_edac.c | 632 +++++++++++++++++++++++++++++++++++++
> 4 files changed, 646 insertions(+)
> create mode 100644 drivers/edac/dmc520_edac.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 77eae44bf5de..7c1ac8bc8ea1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5703,6 +5703,12 @@ F: Documentation/driver-api/edac.rst
> F: drivers/edac/
> F: include/linux/edac.h
>
> +EDAC-DMC520
> +M: Lei Wang <lewan@microsoft.com>
> +L: linux-edac@vger.kernel.org
> +S: Supported
> +F: drivers/edac/dmc520_edac.c
> +
> EDAC-E752X
> M: Mark Gross <mark.gross@intel.com>
> L: linux-edac@vger.kernel.org
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index 200c04ce5b0e..7fde5aea0c1a 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -510,4 +510,11 @@ config EDAC_ASPEED
> First, ECC must be configured in the bootloader. Then, this driver
> will expose error counters via the EDAC kernel framework.
>
> +config EDAC_DMC520
> + tristate "ARM DMC-520 ECC"
> + depends on ARM64
> + help
> + Support for error detection and correction on the
> + SoCs with ARM DMC-520 DRAM controller.
> +
> endif # EDAC
> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
> index 165ca65e1a3a..e0d98f5b2045 100644
> --- a/drivers/edac/Makefile
> +++ b/drivers/edac/Makefile
> @@ -85,3 +85,4 @@ obj-$(CONFIG_EDAC_XGENE) += xgene_edac.o
> obj-$(CONFIG_EDAC_TI) += ti_edac.o
> obj-$(CONFIG_EDAC_QCOM) += qcom_edac.o
> obj-$(CONFIG_EDAC_ASPEED) += aspeed_edac.o
> +obj-$(CONFIG_EDAC_DMC520) += dmc520_edac.o
> diff --git a/drivers/edac/dmc520_edac.c b/drivers/edac/dmc520_edac.c
> new file mode 100644
> index 000000000000..aeb8d84405cf
> --- /dev/null
> +++ b/drivers/edac/dmc520_edac.c
> @@ -0,0 +1,632 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* EDAC driver for DMC-520 */
That comment is kinda useless.
> +#include <linux/bitfield.h>
> +#include <linux/edac.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include "edac_mc.h"
> +
> +/* DMC-520 registers */
> +#define REG_OFFSET_FEATURE_CONFIG 0x130
> +#define REG_OFFSET_ECC_ERRC_COUNT_31_00 0x158
> +#define REG_OFFSET_ECC_ERRC_COUNT_63_32 0x15C
> +#define REG_OFFSET_ECC_ERRD_COUNT_31_00 0x160
> +#define REG_OFFSET_ECC_ERRD_COUNT_63_32 0x164
> +#define REG_OFFSET_INTERRUPT_CONTROL 0x500
> +#define REG_OFFSET_INTERRUPT_CLR 0x508
> +#define REG_OFFSET_INTERRUPT_STATUS 0x510
> +#define REG_OFFSET_DRAM_ECC_ERRC_INT_INFO_31_00 0x528
> +#define REG_OFFSET_DRAM_ECC_ERRC_INT_INFO_63_32 0x52C
> +#define REG_OFFSET_DRAM_ECC_ERRD_INT_INFO_31_00 0x530
> +#define REG_OFFSET_DRAM_ECC_ERRD_INT_INFO_63_32 0x534
> +#define REG_OFFSET_ADDRESS_CONTROL_NOW 0x1010
> +#define REG_OFFSET_MEMORY_TYPE_NOW 0x1128
> +#define REG_OFFSET_SCRUB_CONTROL0_NOW 0x1170
> +#define REG_OFFSET_FORMAT_CONTROL 0x18
> +
> +/* DMC-520 types, masks and bitfields */
> +#define DRAM_ECC_INT_CE_BIT BIT(2)
> +#define DRAM_ECC_INT_UE_BIT BIT(3)
> +#define ALL_INT_MASK GENMASK(9, 0)
> +#define MEMORY_WIDTH_MASK GENMASK(1, 0)
> +#define SCRUB_TRIGGER0_NEXT_MASK GENMASK(1, 0)
> +#define REG_FIELD_DRAM_ECC_ENABLED GENMASK(1, 0)
> +#define REG_FIELD_MEMORY_TYPE GENMASK(2, 0)
> +#define REG_FIELD_DEVICE_WIDTH GENMASK(9, 8)
> +#define REG_FIELD_ADDRESS_CONTROL_COL GENMASK(2, 0)
> +#define REG_FIELD_ADDRESS_CONTROL_ROW GENMASK(10, 8)
> +#define REG_FIELD_ADDRESS_CONTROL_BANK GENMASK(18, 16)
> +#define REG_FIELD_ADDRESS_CONTROL_RANK GENMASK(25, 24)
> +#define REG_FIELD_ERR_INFO_LOW_VALID BIT(0)
> +#define REG_FIELD_ERR_INFO_LOW_COL GENMASK(10, 1)
> +#define REG_FIELD_ERR_INFO_LOW_ROW GENMASK(28, 11)
> +#define REG_FIELD_ERR_INFO_LOW_RANK GENMASK(31, 29)
> +#define REG_FIELD_ERR_INFO_HIGH_BANK GENMASK(3, 0)
> +#define REG_FIELD_ERR_INFO_HIGH_VALID BIT(31)
> +
> +#define DRAM_ADDRESS_CONTROL_MIN_COL_BITS 8
> +#define DRAM_ADDRESS_CONTROL_MIN_ROW_BITS 11
> +
> +#define DMC520_SCRUB_TRIGGER_ERR_DETECT 2
> +#define DMC520_SCRUB_TRIGGER_IDLE 3
One tab too many. Keep them all vertically aligned.
> +
> +/* Driver settings */
> +/* The max-length message would be: "rank:7 bank:15 row:262143 col:1023".
> + * Max length is 34. Using a 40-size buffer is enough.
> + */
> +#define EDAC_MSG_BUF_SIZE 40
That define is driver-local only. Prefix it with "DMC520_" or so, not
with "EDAC_".
> +#define EDAC_MOD_NAME "dmc520-edac"
> +#define EDAC_CTL_NAME "dmc520"
> +
> +/* the data bus width for the attached memory chips. */
> +enum dmc520_mem_width {
> + MEM_WIDTH_X32 = 2,
> + MEM_WIDTH_X64 = 3
> +};
> +
> +/* memory type */
> +enum dmc520_mem_type {
> + MEM_TYPE_DDR3 = 1,
> + MEM_TYPE_DDR4 = 2
> +};
> +
> +/* memory device width */
> +enum dmc520_dev_width {
> + DEV_WIDTH_X4 = 0,
> + DEV_WIDTH_X8 = 1,
> + DEV_WIDTH_X16 = 2
> +};
> +
> +struct ecc_error_info {
> + u32 col;
> + u32 row;
> + u32 bank;
> + u32 rank;
> +};
> +
> +/* The EDAC driver private data */
> +struct dmc520_edac {
> + void __iomem *reg_base;
> + u32 nintr;
> + u32 interrupt_mask_all;
> + spinlock_t ecc_lock;
> + u32 interrupt_masks[0];
> +};
> +
> +static int dmc520_mc_idx;
> +
> +static irqreturn_t
> +dmc520_edac_dram_all_isr(int irq, struct mem_ctl_info *mci, u32 interrupt_mask);
> +
> +#define DECLARE_ISR(index) \
> +static irqreturn_t dmc520_isr_##index (int irq, void *data) \
Please integrate scripts/checkpatch.pl into your patch creation
workflow. Some of the warnings/errors *actually* make sense.
WARNING: space prohibited between function name and open parenthesis '('
#200: FILE: drivers/edac/dmc520_edac.c:108:
+static irqreturn_t dmc520_isr_##index (int irq, void *data) \
> +{ \
> + struct mem_ctl_info *mci; \
> + struct dmc520_edac *edac; \
> + mci = data; \
> + edac = mci->pvt_info; \
> + return dmc520_edac_dram_all_isr(irq, mci, edac->interrupt_masks[index]); \
> +}
> +
> +DECLARE_ISR(0)
> +DECLARE_ISR(1)
> +/* More DECLARE_ISR(index) can be added to support more interrupt lines. */
> +
> +irq_handler_t dmc520_isr_array[] = {
> + dmc520_isr_0,
> + dmc520_isr_1
> + /* More dmc520_isr_index can be added to support more interrupt lines. */
What are those comments really for?
> +};
> +
> +static u32 dmc520_read_reg(struct dmc520_edac *edac, u32 offset)
> +{
> + return readl(edac->reg_base + offset);
> +}
> +
> +static void dmc520_write_reg(struct dmc520_edac *edac, u32 val, u32 offset)
> +{
> + writel(val, edac->reg_base + offset);
> +}
> +
> +static u32 dmc520_calc_dram_ecc_error(u32 value)
> +{
> + u32 total = 0;
> +
> + /* Each rank's error counter takes one byte */
> + while (value > 0) {
> + total += (value & 0xFF);
> + value >>= 8;
> + }
> + return total;
> +}
> +
> +static u32 dmc520_get_dram_ecc_error_count(struct dmc520_edac *edac,
> + bool is_ce)
> +{
> + u32 reg_offset_low, reg_offset_high;
> + u32 err_low, err_high;
> + u32 err_count;
> +
> + reg_offset_low = is_ce ? REG_OFFSET_ECC_ERRC_COUNT_31_00 :
> + REG_OFFSET_ECC_ERRD_COUNT_31_00;
> + reg_offset_high = is_ce ? REG_OFFSET_ECC_ERRC_COUNT_63_32 :
> + REG_OFFSET_ECC_ERRD_COUNT_63_32;
> +
> + err_low = dmc520_read_reg(edac, reg_offset_low);
> + err_high = dmc520_read_reg(edac, reg_offset_high);
> + /* Reset error counters */
> + dmc520_write_reg(edac, 0, reg_offset_low);
> + dmc520_write_reg(edac, 0, reg_offset_high);
> +
> + err_count = dmc520_calc_dram_ecc_error(err_low) +
> + dmc520_calc_dram_ecc_error(err_high);
> +
> + return err_count;
> +}
> +
> +static void dmc520_get_dram_ecc_error_info(struct dmc520_edac *edac,
> + bool is_ce,
> + struct ecc_error_info *info)
> +{
> + u32 reg_offset_low, reg_offset_high;
> + u32 reg_val_low, reg_val_high;
> + bool valid;
> +
> + reg_offset_low = is_ce ? REG_OFFSET_DRAM_ECC_ERRC_INT_INFO_31_00 :
> + REG_OFFSET_DRAM_ECC_ERRD_INT_INFO_31_00;
> + reg_offset_high = is_ce ? REG_OFFSET_DRAM_ECC_ERRC_INT_INFO_63_32 :
> + REG_OFFSET_DRAM_ECC_ERRD_INT_INFO_63_32;
> +
> + reg_val_low = dmc520_read_reg(edac, reg_offset_low);
> + reg_val_high = dmc520_read_reg(edac, reg_offset_high);
> +
> + valid = (FIELD_GET(REG_FIELD_ERR_INFO_LOW_VALID, reg_val_low) != 0) &&
> + (FIELD_GET(REG_FIELD_ERR_INFO_HIGH_VALID, reg_val_high) != 0);
> +
> + if (valid) {
> + info->col = FIELD_GET(REG_FIELD_ERR_INFO_LOW_COL, reg_val_low);
> + info->row = FIELD_GET(REG_FIELD_ERR_INFO_LOW_ROW, reg_val_low);
> + info->rank = FIELD_GET(REG_FIELD_ERR_INFO_LOW_RANK, reg_val_low);
> + info->bank = FIELD_GET(REG_FIELD_ERR_INFO_HIGH_BANK, reg_val_high);
> + } else {
> + memset(info, 0, sizeof(struct ecc_error_info));
> + }
> +}
> +
> +static bool dmc520_is_ecc_enabled(void __iomem *reg_base)
> +{
> + u32 reg_val = readl(reg_base + REG_OFFSET_FEATURE_CONFIG);
> +
> + return (FIELD_GET(REG_FIELD_DRAM_ECC_ENABLED, reg_val) != 0);
No need for that " != 0" at the end.
> +}
> +
> +static enum scrub_type dmc520_get_scrub_type(struct dmc520_edac *edac)
> +{
> + enum scrub_type type = SCRUB_NONE;
> + u32 reg_val, scrub_cfg;
> +
> + reg_val = dmc520_read_reg(edac, REG_OFFSET_SCRUB_CONTROL0_NOW);
> + scrub_cfg = FIELD_GET(SCRUB_TRIGGER0_NEXT_MASK, reg_val);
> +
> + if (DMC520_SCRUB_TRIGGER_ERR_DETECT == scrub_cfg ||
> + DMC520_SCRUB_TRIGGER_IDLE == scrub_cfg)
> + type = SCRUB_HW_PROG;
More from checkpatch:
WARNING: Comparisons should place the constant on the right side of the test
#309: FILE: drivers/edac/dmc520_edac.c:217:
+ if (DMC520_SCRUB_TRIGGER_ERR_DETECT == scrub_cfg ||
WARNING: Comparisons should place the constant on the right side of the test
#310: FILE: drivers/edac/dmc520_edac.c:218:
+ DMC520_SCRUB_TRIGGER_IDLE == scrub_cfg)
> +
> + return type;
> +}
> +
> +/* Get the memory data bus width, in number of bytes. */
> +static u32 dmc520_get_memory_width(struct dmc520_edac *edac)
> +{
> + static u32 mem_width;
> + u32 reg_val;
> + enum dmc520_mem_width mem_width_field;
Please sort function local variables declaration in a reverse christmas
tree order:
<type A> longest_variable_name;
<type B> shorter_var_name;
<type C> even_shorter;
<type D> i;
Here and in the other functions.
> +
> + if (mem_width == 0) {
Flip that check and save yourself an indentation level:
if (mem_width)
return;
reg_val = ...
> + reg_val = dmc520_read_reg(edac, REG_OFFSET_FORMAT_CONTROL);
> + mem_width_field = FIELD_GET(MEMORY_WIDTH_MASK, reg_val);
> +
> + if (mem_width_field == MEM_WIDTH_X32)
> + mem_width = 4; /* 32-bits, 4 bytes */
> + else if (mem_width_field == MEM_WIDTH_X64)
> + mem_width = 8; /* 64-bits, 8 bytes */
Please no side-comments. And those you can directly remove as they're
useless - your defines and variables should be named properly so that
there is no need for a comment here additionally.
> + }
> +
> + return mem_width;
> +}
> +
> +static enum mem_type dmc520_get_mtype(struct dmc520_edac *edac)
> +{
> + enum mem_type mt = MEM_UNKNOWN;
> + u32 reg_val;
> + enum dmc520_mem_type type;
> +
> + reg_val = dmc520_read_reg(edac, REG_OFFSET_MEMORY_TYPE_NOW);
> + type = FIELD_GET(REG_FIELD_MEMORY_TYPE, reg_val);
> +
> + switch (type) {
> + case MEM_TYPE_DDR3:
> + mt = MEM_DDR3;
> + break;
> +
> + case MEM_TYPE_DDR4:
> + mt = MEM_DDR4;
> + break;
> + }
> +
> + return mt;
> +}
> +
> +static enum dev_type dmc520_get_dtype(struct dmc520_edac *edac)
> +{
> + enum dev_type dt = DEV_UNKNOWN;
> + u32 reg_val;
> + enum dmc520_dev_width device_width;
> +
> + reg_val = dmc520_read_reg(edac, REG_OFFSET_MEMORY_TYPE_NOW);
> + device_width = FIELD_GET(REG_FIELD_DEVICE_WIDTH, reg_val);
> +
> + switch (device_width) {
> + case DEV_WIDTH_X4:
> + dt = DEV_X4;
> + break;
> +
> + case DEV_WIDTH_X8:
> + dt = DEV_X8;
> + break;
> +
> + case DEV_WIDTH_X16:
> + dt = DEV_X16;
> + break;
> + }
> +
> + return dt;
> +}
> +
> +static u32 dmc520_get_rank_count(void __iomem *reg_base)
> +{
> + u32 reg_val, rank_bits;
> +
> + reg_val = readl(reg_base + REG_OFFSET_ADDRESS_CONTROL_NOW);
> + rank_bits = FIELD_GET(REG_FIELD_ADDRESS_CONTROL_RANK, reg_val);
> +
> + return (1 << rank_bits);
return BIT(rank_bits);
> +}
> +
> +static u64 dmc520_get_rank_size(struct dmc520_edac *edac)
> +{
> + u32 reg_val, col_bits, row_bits, bank_bits;
> +
> + reg_val = dmc520_read_reg(edac, REG_OFFSET_ADDRESS_CONTROL_NOW);
> +
> + col_bits = FIELD_GET(REG_FIELD_ADDRESS_CONTROL_COL, reg_val) +
> + DRAM_ADDRESS_CONTROL_MIN_COL_BITS;
> + row_bits = FIELD_GET(REG_FIELD_ADDRESS_CONTROL_ROW, reg_val) +
> + DRAM_ADDRESS_CONTROL_MIN_ROW_BITS;
> + bank_bits = FIELD_GET(REG_FIELD_ADDRESS_CONTROL_BANK, reg_val);
> +
> + return (u64)dmc520_get_memory_width(edac) << (col_bits + row_bits + bank_bits);
> +}
> +
> +static void dmc520_handle_dram_ecc_errors(struct mem_ctl_info *mci,
> + bool is_ce)
> +{
> + struct ecc_error_info info;
> + struct dmc520_edac *edac;
> + u32 cnt;
> + char message[EDAC_MSG_BUF_SIZE];
> +
> + edac = mci->pvt_info;
> + dmc520_get_dram_ecc_error_info(edac, is_ce, &info);
> +
> + cnt = dmc520_get_dram_ecc_error_count(edac, is_ce);
> +
> + if (cnt > 0) {
Save an indentation level:
if (!cnt)
return;
snprintf...
> + snprintf(message, ARRAY_SIZE(message),
> + "rank:%d bank:%d row:%d col:%d",
> + info.rank, info.bank,
> + info.row, info.col);
> +
> + spin_lock(&edac->ecc_lock);
> + edac_mc_handle_error((is_ce ? HW_EVENT_ERR_CORRECTED :
> + HW_EVENT_ERR_UNCORRECTED),
> + mci, cnt, 0, 0, 0, info.rank, -1, -1,
> + message, "");
> + spin_unlock(&edac->ecc_lock);
> + }
> +}
> +
> +static irqreturn_t dmc520_edac_dram_ecc_isr(int irq, struct mem_ctl_info *mci, bool is_ce)
> +{
> + u32 i_mask;
> + struct dmc520_edac *edac;
> +
> + edac = mci->pvt_info;
> +
> + i_mask = is_ce ? DRAM_ECC_INT_CE_BIT : DRAM_ECC_INT_UE_BIT;
> +
> + dmc520_handle_dram_ecc_errors(mci, is_ce);
> +
> + dmc520_write_reg(edac, i_mask, REG_OFFSET_INTERRUPT_CLR);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t
> +dmc520_edac_dram_all_isr(int irq, struct mem_ctl_info *mci, u32 interrupt_mask)
> +{
> + struct dmc520_edac *edac;
> + u32 status;
> + irqreturn_t irq_ret = IRQ_NONE;
> +
> + edac = mci->pvt_info;
> +
> + status = dmc520_read_reg(edac, REG_OFFSET_INTERRUPT_STATUS);
> +
> + if ((interrupt_mask & DRAM_ECC_INT_CE_BIT) &&
> + (status & DRAM_ECC_INT_CE_BIT))
> + irq_ret = dmc520_edac_dram_ecc_isr(irq, mci, true);
> +
> + if ((interrupt_mask & DRAM_ECC_INT_UE_BIT) &&
> + (status & DRAM_ECC_INT_UE_BIT))
> + irq_ret = dmc520_edac_dram_ecc_isr(irq, mci, false);
> +
> + /* If in the future there are more supported interrupts in a different
> + * platform, more condition statements can be added here for each
> + * interrupt flag, together with its corresponding isr implementations.
> + */
This comment above is useless.
Also, kernel comments style is:
/*
* A sentence ending with a full-stop.
* Another sentence. ...
* More sentences. ...
*/
> +
> + return irq_ret;
> +}
> +
> +static void dmc520_init_csrow(struct mem_ctl_info *mci)
> +{
> + struct csrow_info *csi;
> + struct dimm_info *dimm;
> + int row, ch;
> + enum dev_type dt;
> + enum mem_type mt;
> + u64 rs;
> + u32 pages_per_rank;
> + struct dmc520_edac *edac = mci->pvt_info;
> +
> + dt = dmc520_get_dtype(edac);
> + mt = dmc520_get_mtype(edac);
> + rs = dmc520_get_rank_size(edac);
> + pages_per_rank = rs >> PAGE_SHIFT;
> +
> + for (row = 0; row < mci->nr_csrows; row++) {
> + csi = mci->csrows[row];
> +
> + for (ch = 0; ch < csi->nr_channels; ch++) {
> + dimm = csi->channels[ch]->dimm;
> + dimm->grain = dmc520_get_memory_width(edac);
> + dimm->dtype = dt;
> + dimm->mtype = mt;
> + dimm->edac_mode = EDAC_FLAG_SECDED;
> + dimm->nr_pages = pages_per_rank / csi->nr_channels;
> + }
> + }
> +}
> +
> +static int dmc520_edac_probe(struct platform_device *pdev)
> +{
> + struct device *dev;
> + struct dmc520_edac *edac;
> + struct mem_ctl_info *mci;
> + struct edac_mc_layer layers[1];
> + int ret, intr_index, nintr, nintr_registered = 0;
> + struct resource *res;
> + void __iomem *reg_base;
> + u32 reg_val;
> +
> + /* Parsing the device node */
> + dev = &pdev->dev;
> +
> + nintr = of_property_count_u32_elems(dev->of_node, "interrupt-config");
> + if (nintr <= 0) {
> + edac_printk(KERN_ERR, EDAC_MOD_NAME,
> + "Invalid device node configuration: at least one interrupt "
> + "line & config is expected.\n");
More from checkpatch:
WARNING: quoted string split across lines
#528: FILE: drivers/edac/dmc520_edac.c:436:
+ "Invalid device node configuration: at least one interrupt "
+ "line & config is expected.\n");
WARNING: quoted string split across lines
#535: FILE: drivers/edac/dmc520_edac.c:443:
+ "Invalid device node configuration: # of interrupt config "
+ "elements (%d) can not exceed %ld.\n",
WARNING: quoted string split across lines
#579: FILE: drivers/edac/dmc520_edac.c:487:
+ "interrupt-config error: "
+ "element %d's interrupt mask %d has overlap.\n",
WARNING: quoted string split across lines
#590: FILE: drivers/edac/dmc520_edac.c:498:
+ "interrupt-config warning: "
+ "interrupt mask (0x%x) is not supported by dmc520 (0x%lx).\n",
> + return -EINVAL;
> + }
> +
> + if (nintr > ARRAY_SIZE(dmc520_isr_array)) {
> + edac_printk(KERN_ERR, EDAC_MOD_NAME,
> + "Invalid device node configuration: # of interrupt config "
> + "elements (%d) can not exceed %ld.\n",
> + nintr, ARRAY_SIZE(dmc520_isr_array));
> + return -EINVAL;
> + }
> +
> + /* Initialize dmc520 edac */
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + reg_base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(reg_base))
> + return PTR_ERR(reg_base);
> +
> + if (!dmc520_is_ecc_enabled(reg_base))
> + return -ENXIO;
> +
> + layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
> + layers[0].size = dmc520_get_rank_count(reg_base);
> + layers[0].is_virt_csrow = true;
> +
> + mci = edac_mc_alloc(dmc520_mc_idx++, ARRAY_SIZE(layers), layers,
> + sizeof(struct dmc520_edac) + sizeof(u32) * nintr);
> + if (!mci) {
> + edac_printk(KERN_ERR, EDAC_MOD_NAME,
> + "Failed to allocate memory for mc instance\n");
> + return -ENOMEM;
> + }
> +
> + edac = mci->pvt_info;
> + edac->reg_base = reg_base;
> + edac->nintr = nintr;
> + edac->interrupt_mask_all = 0;
> + spin_lock_init(&edac->ecc_lock);
Move that checking...
> +
> + ret = of_property_read_u32_array(dev->of_node, "interrupt-config",
> + edac->interrupt_masks, nintr);
> + if (ret) {
> + edac_printk(KERN_ERR, EDAC_MOD_NAME,
> + "Failed to get interrupt-config arrays.\n");
> + goto err_free_mc;
> + }
> +
> + for (intr_index = 0; intr_index < nintr; ++intr_index) {
> + if (edac->interrupt_mask_all & edac->interrupt_masks[intr_index]) {
> + edac_printk(KERN_ERR, EDAC_MC,
> + "interrupt-config error: "
> + "element %d's interrupt mask %d has overlap.\n",
> + intr_index, edac->interrupt_masks[intr_index]);
> + goto err_free_mc;
> + }
> +
> + edac->interrupt_mask_all |= edac->interrupt_masks[intr_index];
> + }
> +
> + if ((edac->interrupt_mask_all | ALL_INT_MASK) != ALL_INT_MASK) {
> + edac_printk(KERN_WARNING, EDAC_MOD_NAME,
> + "interrupt-config warning: "
> + "interrupt mask (0x%x) is not supported by dmc520 (0x%lx).\n",
> + edac->interrupt_mask_all, ALL_INT_MASK);
> + }
> + edac->interrupt_mask_all &= ALL_INT_MASK;
> +
> + platform_set_drvdata(pdev, mci);
... up-to-here before the edac_mc_alloc() call so that you don't have to
needlessly goto err_free_mc and free the memory you just allocated if
any of those properties reading fails. IOW, do all your hw init first
and then call edac_mc_alloc(), when you have everything needed.
> +
> + mci->pdev = dev;
> + mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR4;
> + mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
> + mci->edac_cap = EDAC_FLAG_SECDED;
> + mci->scrub_cap = SCRUB_FLAG_HW_SRC;
> + mci->scrub_mode = dmc520_get_scrub_type(edac);
> + mci->ctl_name = EDAC_CTL_NAME;
> + mci->dev_name = dev_name(mci->pdev);
> + mci->mod_name = EDAC_MOD_NAME;
> + mci->ctl_page_to_phys = NULL;
> +
> + edac_op_state = EDAC_OPSTATE_INT;
> +
> + dmc520_init_csrow(mci);
> +
> + ret = edac_mc_add_mc(mci);
> + if (ret) {
> + edac_printk(KERN_ERR, EDAC_MOD_NAME,
> + "Failed to register with EDAC core\n");
> + goto err_free_mc;
> + }
> +
> + /* Clear interrupts, not affecting other unrelated interrupts */
> + reg_val = dmc520_read_reg(edac, REG_OFFSET_INTERRUPT_CONTROL);
> + dmc520_write_reg(edac, reg_val & (~(edac->interrupt_mask_all)),
> + REG_OFFSET_INTERRUPT_CONTROL);
> + dmc520_write_reg(edac, edac->interrupt_mask_all,
> + REG_OFFSET_INTERRUPT_CLR);
> +
> + for (intr_index = 0; intr_index < nintr; ++intr_index) {
> + int irq_id = platform_get_irq(pdev, intr_index);
> +
> + if (irq_id < 0) {
> + edac_printk(KERN_ERR, EDAC_MC,
> + "Failed to get irq #%d\n", intr_index);
> + ret = -ENODEV;
> + goto err_free_irq;
> + }
> +
> + ret = devm_request_irq(&pdev->dev, irq_id,
> + dmc520_isr_array[intr_index], IRQF_SHARED,
> + dev_name(&pdev->dev), mci);
> + if (ret < 0) {
> + edac_printk(KERN_ERR, EDAC_MC,
> + "Failed to request irq %d\n", irq_id);
> + goto err_free_irq;
> + }
> +
> + ++nintr_registered;
> + }
The same with this IRQ requesting and you'll save yourself the err_free_irq too.
> +
> + /* Reset DRAM CE/UE counters */
> + if (edac->interrupt_mask_all & DRAM_ECC_INT_CE_BIT)
> + dmc520_get_dram_ecc_error_count(edac, true);
> +
> + if (edac->interrupt_mask_all & DRAM_ECC_INT_UE_BIT)
> + dmc520_get_dram_ecc_error_count(edac, false);
> +
> + /* Enable interrupts, not affecting other unrelated interrupts */
> + dmc520_write_reg(edac,
> + reg_val | edac->interrupt_mask_all,
> + REG_OFFSET_INTERRUPT_CONTROL);
Align at opening brace:
dmc520_write_reg(edac, reg_val | edac->interrupt_mask_all,
REG_OFFSET_INTERRUPT_CONTROL);
> +
> + return 0;
> +
> +err_free_irq:
> + for (intr_index = 0; intr_index < nintr_registered; ++intr_index) {
> + int irq_id = platform_get_irq(pdev, intr_index);
> +
> + devm_free_irq(&pdev->dev, irq_id, mci);
> + }
> + edac_mc_del_mc(&pdev->dev);
> +err_free_mc:
> + edac_mc_free(mci);
> +
> + return ret;
> +}
> +
> +static int dmc520_edac_remove(struct platform_device *pdev)
> +{
> + struct dmc520_edac *edac;
> + struct mem_ctl_info *mci;
> + u32 reg_val, intr_index;
> +
> + mci = platform_get_drvdata(pdev);
> + edac = mci->pvt_info;
> +
> + /* Disable interrupts */
> + reg_val = dmc520_read_reg(edac, REG_OFFSET_INTERRUPT_CONTROL);
> + dmc520_write_reg(edac, reg_val & (~(edac->interrupt_mask_all)),
> + REG_OFFSET_INTERRUPT_CONTROL);
> +
> + /* free irq's */
> + for (intr_index = 0; intr_index < edac->nintr; ++intr_index) {
> + int irq_id = platform_get_irq(pdev, intr_index);
> +
> + devm_free_irq(&pdev->dev, irq_id, mci);
> + }
> +
> + edac_mc_del_mc(&pdev->dev);
> + edac_mc_free(mci);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id dmc520_edac_driver_id[] = {
> + { .compatible = "arm,dmc-520", },
> + { /* end of table */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, dmc520_edac_driver_id);
> +
> +static struct platform_driver dmc520_edac_driver = {
> + .driver = {
> + .name = "dmc520",
> + .of_match_table = dmc520_edac_driver_id,
> + },
> +
> + .probe = dmc520_edac_probe,
> + .remove = dmc520_edac_remove
> +};
> +
> +module_platform_driver(dmc520_edac_driver);
> +
> +MODULE_AUTHOR(
No linebreaks like that - let it stick out over 80 cols.
> + "Rui Zhao <ruizhao@microsoft.com>, Lei Wang <lewan@microsoft.com>");
> +MODULE_DESCRIPTION("DMC-520 ECC driver");
> +MODULE_LICENSE("GPL v2");
> --
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v5 2/2] EDAC: add EDAC driver for DMC520
2019-08-07 14:40 ` Borislav Petkov
@ 2019-08-13 1:05 ` Lei Wang
2019-08-19 9:31 ` Borislav Petkov
0 siblings, 1 reply; 6+ messages in thread
From: Lei Wang @ 2019-08-13 1:05 UTC (permalink / raw)
To: Borislav Petkov
Cc: james.morse, robh+dt, mark.rutland, devicetree, linux-kernel,
mchehab, linux-edac, sashal, hangl, lewan, ruizhao
Thanks Borislav! I addressed most of your comments. There are a few that
need your further feedback. Thanks!
>> +++ b/drivers/edac/dmc520_edac.c
>> @@ -0,0 +1,632 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* EDAC driver for DMC-520 */
>
> That comment is kinda useless.
>
Added some more comments for the file:
/*
* dmc520_edac.c, EDAC driver for DMC-520 memory controller
*
* Authors: Rui Zhao <ruizhao@microsoft.com>
* Lei Wang <lewan@microsoft.com>
*/
>
>> +{ \
>> + struct mem_ctl_info *mci; \
>> + struct dmc520_edac *edac; \
>> + mci = data; \
>> + edac = mci->pvt_info; \
>> + return dmc520_edac_dram_all_isr(irq, mci, edac->interrupt_masks[index]); \
>> +}
>> +
>> +DECLARE_ISR(0)
>> +DECLARE_ISR(1)
>> +/* More DECLARE_ISR(index) can be added to support more interrupt lines. */
>> +
>> +irq_handler_t dmc520_isr_array[] = {
>> + dmc520_isr_0,
>> + dmc520_isr_1
>> + /* More dmc520_isr_index can be added to support more interrupt lines. */
>
> What are those comments really for?
>
These comments tell how to potentially expand the driver functions to
support more interrupts besides what are already here.
>> +
>> + /* If in the future there are more supported interrupts in a different
>> + * platform, more condition statements can be added here for each
>> + * interrupt flag, together with its corresponding isr implementations.
>> + */
>
> This comment above is useless.
>
> Also, kernel comments style is:
>
> /*
> * A sentence ending with a full-stop.
> * Another sentence. ...
> * More sentences. ...
> */
>
As above comments, this comment is to guide potential future adding to
this driver to support other interrupts. I modified the comment format.
>> +
>> + mci = edac_mc_alloc(dmc520_mc_idx++, ARRAY_SIZE(layers), layers,
>> + sizeof(struct dmc520_edac) + sizeof(u32) * nintr);
>> + if (!mci) {
>> + edac_printk(KERN_ERR, EDAC_MOD_NAME,
>> + "Failed to allocate memory for mc instance\n");
>> + return -ENOMEM;
>> + }
>> +
>> + edac = mci->pvt_info;
>> + edac->reg_base = reg_base;
>> + edac->nintr = nintr;
>> + edac->interrupt_mask_all = 0;
>> + spin_lock_init(&edac->ecc_lock);
>
> Move that checking...
>
>> +
>> + ret = of_property_read_u32_array(dev->of_node, "interrupt-config",
>> + edac->interrupt_masks, nintr);
>> + if (ret) {
>> + edac_printk(KERN_ERR, EDAC_MOD_NAME,
>> + "Failed to get interrupt-config arrays.\n");
>> + goto err_free_mc;
>> + }
>> +
>> + for (intr_index = 0; intr_index < nintr; ++intr_index) {
>> + if (edac->interrupt_mask_all & edac->interrupt_masks[intr_index]) {
>> + edac_printk(KERN_ERR, EDAC_MC,
>> + "interrupt-config error: "
>> + "element %d's interrupt mask %d has overlap.\n",
>> + intr_index, edac->interrupt_masks[intr_index]);
>> + goto err_free_mc;
>> + }
>> +
>> + edac->interrupt_mask_all |= edac->interrupt_masks[intr_index];
>> + }
>> +
>> + if ((edac->interrupt_mask_all | ALL_INT_MASK) != ALL_INT_MASK) {
>> + edac_printk(KERN_WARNING, EDAC_MOD_NAME,
>> + "interrupt-config warning: "
>> + "interrupt mask (0x%x) is not supported by dmc520 (0x%lx).\n",
>> + edac->interrupt_mask_all, ALL_INT_MASK);
>> + }
>> + edac->interrupt_mask_all &= ALL_INT_MASK;
>> +
>> + platform_set_drvdata(pdev, mci);
>
> ... up-to-here before the edac_mc_alloc() call so that you don't have to
> needlessly goto err_free_mc and free the memory you just allocated if
> any of those properties reading fails. IOW, do all your hw init first
> and then call edac_mc_alloc(), when you have everything needed.
>
After edac_mc_alloc(), if succeeds, the above code updates dmc520_edac
struct data. If moving edac_mc_alloc as suggested, I will need to use
local variables to store the data, and then after edac_mc_alloc() update
the dmc520_edac. By ordering this way, the dmc520_edac can be updated
directly.
Planning not fix.
>> +
>> + mci->pdev = dev;
>> + mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR4;
>> + mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
>> + mci->edac_cap = EDAC_FLAG_SECDED;
>> + mci->scrub_cap = SCRUB_FLAG_HW_SRC;
>> + mci->scrub_mode = dmc520_get_scrub_type(edac);
>> + mci->ctl_name = EDAC_CTL_NAME;
>> + mci->dev_name = dev_name(mci->pdev);
>> + mci->mod_name = EDAC_MOD_NAME;
>> + mci->ctl_page_to_phys = NULL;
>> +
>> + edac_op_state = EDAC_OPSTATE_INT;
>> +
>> + dmc520_init_csrow(mci);
>> +
>> + ret = edac_mc_add_mc(mci);
>> + if (ret) {
>> + edac_printk(KERN_ERR, EDAC_MOD_NAME,
>> + "Failed to register with EDAC core\n");
>> + goto err_free_mc;
>> + }
>> +
>> + /* Clear interrupts, not affecting other unrelated interrupts */
>> + reg_val = dmc520_read_reg(edac, REG_OFFSET_INTERRUPT_CONTROL);
>> + dmc520_write_reg(edac, reg_val & (~(edac->interrupt_mask_all)),
>> + REG_OFFSET_INTERRUPT_CONTROL);
>> + dmc520_write_reg(edac, edac->interrupt_mask_all,
>> + REG_OFFSET_INTERRUPT_CLR);
>> +
>> + for (intr_index = 0; intr_index < nintr; ++intr_index) {
>> + int irq_id = platform_get_irq(pdev, intr_index);
>> +
>> + if (irq_id < 0) {
>> + edac_printk(KERN_ERR, EDAC_MC,
>> + "Failed to get irq #%d\n", intr_index);
>> + ret = -ENODEV;
>> + goto err_free_irq;
>> + }
>> +
>> + ret = devm_request_irq(&pdev->dev, irq_id,
>> + dmc520_isr_array[intr_index], IRQF_SHARED,
>> + dev_name(&pdev->dev), mci);
>> + if (ret < 0) {
>> + edac_printk(KERN_ERR, EDAC_MC,
>> + "Failed to request irq %d\n", irq_id);
>> + goto err_free_irq;
>> + }
>> +
>> + ++nintr_registered;
>> + }
>
> The same with this IRQ requesting and you'll save yourself the err_free_irq too.
Do you mean having an array to keep all the irq_id, and then only
devm_request_irq on them if all of the platform_get_irq are success? Yes
that was one option I was thinking too when writing this part. But the #
of interrupts are not fixed and are read from the dt configuration and I
didn't want to use a fixed MAX array size for it. Also it adds a local
array to store the irq_id's. So eventually I favored this
implementation: platform_get_irq one interrupt, and devm_request_irq one
interrupt, in a loop.
Planning not fix.
Thanks again!
-Lei
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v5 2/2] EDAC: add EDAC driver for DMC520
2019-08-13 1:05 ` Lei Wang
@ 2019-08-19 9:31 ` Borislav Petkov
2019-08-27 1:49 ` Lei Wang
0 siblings, 1 reply; 6+ messages in thread
From: Borislav Petkov @ 2019-08-19 9:31 UTC (permalink / raw)
To: Lei Wang
Cc: james.morse, robh+dt, mark.rutland, devicetree, linux-kernel,
mchehab, linux-edac, sashal, hangl, lewan, ruizhao
On Tue, Aug 13, 2019 at 01:05:02AM +0000, Lei Wang wrote:
> Added some more comments for the file:
>
> /*
> * dmc520_edac.c, EDAC driver for DMC-520 memory controller
^^^^^^^^^^^^^
Filename is redundant.
> These comments tell how to potentially expand the driver functions to
> support more interrupts besides what are already here.
I can read that - the question is why are they there and for whom? For
your future colleagues who'll take over this driver or what is those
comments' purpose?
> As above comments, this comment is to guide potential future adding to
> this driver to support other interrupts.
See question above.
> After edac_mc_alloc(), if succeeds, the above code updates dmc520_edac
> struct data. If moving edac_mc_alloc as suggested, I will need to use
> local variables to store the data,
Yes, do that pls.
> Do you mean having an array to keep all the irq_id, and then only
> devm_request_irq on them if all of the platform_get_irq are success?
No, move it before edac_mc_alloc().
In general, do *all* initialization of your hardware first and only
then, when it succeeds, allocate the EDAC structures.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v5 2/2] EDAC: add EDAC driver for DMC520
2019-08-19 9:31 ` Borislav Petkov
@ 2019-08-27 1:49 ` Lei Wang
2019-08-27 7:53 ` Borislav Petkov
0 siblings, 1 reply; 6+ messages in thread
From: Lei Wang @ 2019-08-27 1:49 UTC (permalink / raw)
To: Borislav Petkov
Cc: james.morse, robh+dt, mark.rutland, devicetree, linux-kernel,
mchehab, linux-edac, sashal, hangl, lewan, ruizhao,
scott.branden, yuqing.shen, ray.jui
>> These comments tell how to potentially expand the driver functions to
>> support more interrupts besides what are already here.
>
> I can read that - the question is why are they there and for whom? For
> your future colleagues who'll take over this driver or what is those
> comments' purpose?
>
Yes, this is to help open source developers who potentially might want
to expand this driver, most likely colleagues, and possibly other
developers.
>> After edac_mc_alloc(), if succeeds, the above code updates dmc520_edac
>> struct data. If moving edac_mc_alloc as suggested, I will need to use
>> local variables to store the data,
>
> Yes, do that pls.
>
>> Do you mean having an array to keep all the irq_id, and then only
>> devm_request_irq on them if all of the platform_get_irq are success?
>
> No, move it before edac_mc_alloc().
>
> In general, do *all* initialization of your hardware first and only
> then, when it succeeds, allocate the EDAC structures.
>
Thanks! I'll address this feedback.
-Lei
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v5 2/2] EDAC: add EDAC driver for DMC520
2019-08-27 1:49 ` Lei Wang
@ 2019-08-27 7:53 ` Borislav Petkov
0 siblings, 0 replies; 6+ messages in thread
From: Borislav Petkov @ 2019-08-27 7:53 UTC (permalink / raw)
To: Lei Wang
Cc: james.morse, robh+dt, mark.rutland, devicetree, linux-kernel,
mchehab, linux-edac, sashal, hangl, lewan, ruizhao,
scott.branden, yuqing.shen, ray.jui
On Tue, Aug 27, 2019 at 01:49:29AM +0000, Lei Wang wrote:
> Yes, this is to help open source developers who potentially might want
> to expand this driver, most likely colleagues, and possibly other
> developers.
Then please put that info in the comment at the beginning of the driver
- not some random single sentences about what can be done, interspersed
throughout the code. It needs to be visible at a first glance.
Also, if you want this to be a longer doc, I wouldn't mind at all having
it in Documentation/edac/ (which doesn't exist yet) and then point
people to it from the comment at the beginning of the driver.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-08-27 7:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-25 0:49 [PATCH v5 2/2] EDAC: add EDAC driver for DMC520 Lei Wang
2019-08-07 14:40 ` Borislav Petkov
2019-08-13 1:05 ` Lei Wang
2019-08-19 9:31 ` Borislav Petkov
2019-08-27 1:49 ` Lei Wang
2019-08-27 7:53 ` Borislav Petkov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.