Re: [PATCH v2 1/2] EDAC: add EDAC driver for DMC520

Re: [PATCH v2 1/2] EDAC: add EDAC driver for DMC520

[Borislav Petkov]

Just some initial comments. The general direction of this should be
looked at by James.

On Thu, Mar 07, 2019 at 01:24:01AM +0000, Rui Zhao wrote:
> From: Rui Zhao <ruizhao@microsoft.com>
> 
> New driver supports error detection and correction on
> the devices with ARM DMC-520 memory controller.
> 
> Signed-off-by: Rui Zhao <ruizhao@microsoft.com>
> ---
>  MAINTAINERS                |   6 +
>  drivers/edac/Kconfig       |   7 +
>  drivers/edac/Makefile      |   1 +
>  drivers/edac/dmc520_edac.c | 619 +++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 633 insertions(+)
>  create mode 100644 drivers/edac/dmc520_edac.c

...

> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
> index 716096d..793d64f 100644
> --- a/drivers/edac/Makefile
> +++ b/drivers/edac/Makefile
> @@ -78,3 +78,4 @@ obj-$(CONFIG_EDAC_SYNOPSYS)		+= synopsys_edac.o
>  obj-$(CONFIG_EDAC_XGENE)		+= xgene_edac.o
>  obj-$(CONFIG_EDAC_TI)			+= ti_edac.o
>  obj-$(CONFIG_EDAC_QCOM)			+= qcom_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 0000000..c70ce4e
> --- /dev/null
> +++ b/drivers/edac/dmc520_edac.c
> @@ -0,0 +1,619 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +// EDAC driver for DMC-520

No "//" commenting in .c files. Make sure to have a cursory look at

Documentation/process/submitting-patches.rst

Also, this patch doesn't apply - please redo it against:

https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=for-next

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v2 1/2] EDAC: add EDAC driver for DMC520

[James Morse]

Hi guys,

On 23/03/2019 09:23, Borislav Petkov wrote:
> On Thu, Mar 07, 2019 at 01:24:01AM +0000, Rui Zhao wrote:
>> From: Rui Zhao <ruizhao@microsoft.com>
>>
>> New driver supports error detection and correction on
>> the devices with ARM DMC-520 memory controller.

A question/suggestion on the direction...

Could we avoid probing the driver based on the root hardware compatible?
Could we use the device/chip/platform specific one instead?


We want to avoid per-function edac drivers. If ${my_chip} has edac support for L3 and
memory, I should have a ${my_chip}_edac driver that pulls in the appropriate L3 and memory
code, and presents a sensible view to edac_core.

Thinking out loud...

You have:

>> +static const struct of_device_id dmc520_edac_driver_id[] = {
>> +	{ .compatible = "arm,dmc-520", },
>> +	{ /* end of table */ }
>> +};
>> +

If you wanted to add another device with edac support, we'd ask you to create
${your_chip}_edac driver and pull in the DMC520 and the other device.

But probing the 'arm,dmc-520' compatible like this leaves us in a tricky place if someone
else does this: ${their_device} probes the dmc520 like this too, but they can't stop it on
their platform as it will break yours...


It's normal to have a specific compatible, vexpress has:
| compatible = "arm,vexpress,v2f-2xv6,ca7x3", "arm,vexpress,v2f-2xv6", "arm,vexpress";

Could we do the same here:
| compatible = "vendor,soc-name-dmc520", "arm,dmc-520";

or even:
| compatible = "microsoft,product-name-dmc520", "arm,dmc-520";
if there is some firmware/board configuration that means vendor/soc isn't precise enough.

Then we always probe the driver from "vendor,soc-name-dmc520", never from "arm,dmc520".

This means we grow a list of vendor/soc-name that are using this driver, but if one of
them wants to support a second edac device, we can remove their vendor/soc-name from the
list without affecting anyone else.


Thanks,

James

Re: [PATCH v2 1/2] EDAC: add EDAC driver for DMC520

[James Morse]

Hi Rui,

On 07/03/2019 01:24, Rui Zhao wrote:
> From: Rui Zhao <ruizhao@microsoft.com>
> New driver supports error detection and correction on
> the devices with ARM DMC-520 memory controller.


> diff --git a/drivers/edac/dmc520_edac.c b/drivers/edac/dmc520_edac.c
> new file mode 100644
> index 0000000..c70ce4e
> --- /dev/null
> +++ b/drivers/edac/dmc520_edac.c
> @@ -0,0 +1,619 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +// EDAC driver for DMC-520

[..]

> +// DMC-520 types, masks and bitfields
> +#define MEMORY_TYPE_LPDDR3			0
> +#define MEMORY_TYPE_DDR3			1
> +#define MEMORY_TYPE_DDR4			2
> +#define MEMORY_TYPE_LPDDR4			3

(this might be better as an enum as it hints to the compiler that only these
values can ever happen. An example of where this is handy further down...)


> +#define MEMORY_DEV_WIDTH_X4			0
> +#define MEMORY_DEV_WIDTH_X8			1
> +#define MEMORY_DEV_WIDTH_X16			2
> +#define MEMORY_DEV_WIDTH_X32			3

(this might be better as an enum)


> +struct dmc520_edac {
> +	void __iomem *reg_base;

> +	char message[EDAC_MSG_BUF_SIZE];

Does there need to be an irq-safe lock for this mesage buffer?
Your snprintf() into it in dmc520_handle_dram_ecc_errors(), which is called from both
dmc520_edac_dram_ue_isr() and dmc520_edac_dram_ce_isr()... if these are wired up as
different irqs, they could happen on different CPUs concurrently...


> +};


> +static bool 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 (info) {

I see one caller, dmc520_handle_dram_ecc_errors(), which has info on the stack... is it
possible for info to be NULL here?


> +		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));
> +		}
> +	}
> +
> +	return valid;
> +}

> +static enum mem_type dmc520_get_mtype(struct dmc520_edac *edac)
> +{
> +	enum mem_type mt;
> +	u32 reg_val, type;
> +
> +	reg_val = dmc520_read_reg(edac, REG_OFFSET_MEMORY_TYPE_NOW);
> +	type = FIELD_GET(REG_FIELD_MEMORY_TYPE, reg_val);
> +
> +	switch (type) {
> +	case MEMORY_TYPE_LPDDR3:
> +	case MEMORY_TYPE_DDR3:
> +		mt = MEM_DDR3;
> +		break;
> +
> +	case MEMORY_TYPE_DDR4:
> +	case MEMORY_TYPE_LPDDR4:
> +	default:

Is this default just to shut the compiler warning up?
If so, you could use an enum and the compiler won't warn for a switch() that has all the
cases covered.


> +		mt = MEM_DDR4;
> +		break;
> +	}
> +	return mt;
> +}


> +static irqreturn_t dmc520_edac_dram_ce_isr(int irq, void *data)
> +{
> +	return dmc520_edac_dram_ecc_isr(irq, data, true);
> +}
> +
> +static irqreturn_t dmc520_edac_dram_ue_isr(int irq, void *data)
> +{
> +	return dmc520_edac_dram_ecc_isr(irq, data, false);
> +}

How come these calls aren't folded into the one-size fits all irq handler below?
If the hardware always sets the status register, does the irqchip driver need to call a
different helper?


> +static irqreturn_t dmc520_edac_all_isr(int irq, void *data)
> +{
> +	struct mem_ctl_info *mci;
> +	struct dmc520_edac *edac;
> +	u32 status;
> +
> +	mci = data;
> +	edac = mci->pvt_info;
> +
> +	status = dmc520_read_reg(edac, REG_OFFSET_INTERRUPT_STATUS);
> +
> +	if (status & DRAM_ECC_INT_CE_MASK)
> +		dmc520_edac_dram_ce_isr(irq, data);
> +
> +	if (status & DRAM_ECC_INT_UE_MASK)
> +		dmc520_edac_dram_ue_isr(irq, data);
> +
> +	// Other interrupt handlers can be added
> +
> +	return IRQ_HANDLED;


Shouldn't there be a way of getting IRQ_NONE out of this?


> +}

[...]


> +static int count_set_bits(u32 n)
> +{
> +	int count;
> +
> +	for (count = 0; n != 0; n &= (n - 1))
> +		count++;
> +
> +	return count;
> +}

Isn't this what hweight() does?



> +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, irq, nintr;
> +	struct resource *res;
> +	void __iomem *reg_base;
> +	u32 status, current_bit, interrupt_mask;
> +	bool interrupt_shared;
> +
> +	dev = &pdev->dev;
> +	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);
> +
> +	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));
> +	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;
> +
> +	if (!dmc520_is_ecc_enabled(edac)) {
> +		edac_printk(KERN_ERR, EDAC_MOD_NAME, "ECC not enabled\n");
> +		ret = -ENXIO;
> +		goto err;
> +	}
> +
> +	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_is_scrub_configured(edac) ?
> +			  SCRUB_HW_SRC : SCRUB_NONE;
> +	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;
> +	}
> +
> +	interrupt_shared = false;
> +	interrupt_mask = DRAM_ECC_INT_CE_MASK | DRAM_ECC_INT_UE_MASK;
> +
> +	if (dev->of_node) {
> +		if (of_find_property(dev->of_node, "interrupt-shared", NULL))
> +			interrupt_shared = true;
> +
> +		if (of_property_read_u32(dev->of_node,
> +					 "interrupt-mask",
> +					 &interrupt_mask)) {
> +			edac_printk(KERN_INFO, EDAC_MOD_NAME,
> +				    "Use default interrupt mask 0x%X\n",
> +				    interrupt_mask);
> +		}
> +
> +		interrupt_mask &= ALL_INT_MASK;
> +	}
> +
> +	// Clear interrupts
> +	status = dmc520_read_reg(edac, REG_OFFSET_INTERRUPT_STATUS);
> +	dmc520_write_reg(edac, 0, REG_OFFSET_INTERRUPT_CONTROL);
> +	dmc520_write_reg(edac, ALL_INT_MASK, REG_OFFSET_INTERRUPT_CLR);
> +
> +	// Reset DRAM CE/UE counters
> +	if (interrupt_mask & DRAM_ECC_INT_CE_MASK)
> +		dmc520_get_dram_ecc_error_count(edac, true);
> +
> +	if (interrupt_mask & DRAM_ECC_INT_UE_MASK)
> +		dmc520_get_dram_ecc_error_count(edac, false);
> +
> +	nintr = count_set_bits(interrupt_mask);
> +	if (nintr == 0) {
> +		edac_printk(KERN_ERR, EDAC_MC,
> +			    "Invalid interrupt mask 0x%X\n",
> +			    interrupt_mask);
> +		ret = -EINVAL;
> +		goto err;

This is after edac_mc_add_mc(), and on your remove path you have a edac_mc_del_mc(). Is
that necessary here? Do you need an err_del_mc?



> +	}
> +
> +	current_bit = BIT(0);
> +	for (irq = 0; irq < nintr; ++irq) {
> +		irq_handler_t edac_isr;
> +		int irq_id = platform_get_irq(pdev, irq);
> +
> +		if (irq_id < 0) {
> +			edac_printk(KERN_ERR, EDAC_MC,
> +				    "Failed to get %s irq\n",
> +				    irq == 0 ? "CE" : "UE");
> +			ret = -ENODEV;
> +			goto err;

err_del_mc?


> +		}
> +
> +		if (interrupt_shared) {
> +			edac_isr = dmc520_edac_all_isr;
> +		} else {
> +			while (current_bit & ALL_INT_MASK) {
> +				if (current_bit & interrupt_mask)
> +					break;
> +
> +				current_bit <<= 1;
> +			}
> +
> +			if (current_bit & DRAM_ECC_INT_CE_MASK) {
> +				edac_isr = dmc520_edac_dram_ce_isr;
> +			} else if (current_bit & DRAM_ECC_INT_UE_MASK) {
> +				edac_isr = dmc520_edac_dram_ue_isr;
> +			} else {
> +				edac_printk(KERN_ERR, EDAC_MC,
> +					    "Invalid interrupt bit 0x%X\n",
> +					    current_bit);
> +				ret = -EINVAL;
> +				goto err;

err_del_mc?

> +			}
> +
> +			current_bit <<= 1;
> +		}
> +
> +		ret = devm_request_irq(&pdev->dev,
> +				       irq_id,
> +				       edac_isr,
> +				       0,
> +				       dev_name(&pdev->dev),
> +				       mci);
> +		if (ret < 0) {
> +			edac_printk(KERN_ERR, EDAC_MC,
> +				    "Failed to request irq %d\n", irq_id);
> +			goto err;

err_del_mc?

> +		}
> +
> +		// Only one irq for all interrupts
> +		if (interrupt_shared)
> +			break;
> +	}
> +
> +	// Enable interrupts
> +	dmc520_write_reg(edac, interrupt_mask, REG_OFFSET_INTERRUPT_CONTROL);
> +
> +	return 0;
> +
> +err:
> +	edac_mc_free(mci);
> +
> +	return ret;
> +}


[...]

> +MODULE_AUTHOR("Rui Zhao <ruizhao@microsoft.com>");
> +MODULE_DESCRIPTION("DMC-520 ECC driver");

> +MODULE_LICENSE("GPL v2");

This doesn't match your SPDX tag above:
| // SPDX-License-Identifier: GPL-2.0+

Can I use a later version?



Thanks,

James

Re: [PATCH v2 1/2] EDAC: add EDAC driver for DMC520

[Borislav Petkov]

On Mon, Mar 25, 2019 at 06:27:52PM +0000, James Morse wrote:
> or even:
> | compatible = "microsoft,product-name-dmc520", "arm,dmc-520";
> if there is some firmware/board configuration that means vendor/soc isn't precise enough.

Yap, makes sense to me. If dmc-520 is a memory controller IP, then it
should be a generic, library-like compilation unit which the platform
driver, i.e., <microsoft-platform-name>_edac.c or so links with.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

RE: [PATCH v2 1/2] EDAC: add EDAC driver for DMC520

[Lei Wang (BSP)]

Hi James/Borislav,

I addressed your comments and sent out " [PATCH v3 2/2] EDAC: add EDAC driver for DMC520" for review. Patch is against for-next.

More details please see inline below with "[Lei]" tag. Thanks!

-Lei

-----Original Message-----
From: James Morse <james.morse@arm.com> 
Sent: Monday, March 25, 2019 11:32 AM
To: Rui Zhao <ruizhao@outlook.com>
Cc: bp@alien8.de; robh+dt@kernel.org; mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-edac@vger.kernel.org; okaya@kernel.org; mchehab@kernel.org; will.deacon@arm.com; sashal@kernel.org; Hang Li <hangl@microsoft.com>; Lei Wang (BSP) <Wang.Lei@microsoft.com>; Rui Zhao <ruizhao@microsoft.com>
Subject: Re: [PATCH v2 1/2] EDAC: add EDAC driver for DMC520

Hi Rui,

On 07/03/2019 01:24, Rui Zhao wrote:
> From: Rui Zhao <ruizhao@microsoft.com> New driver supports error 
> detection and correction on the devices with ARM DMC-520 memory 
> controller.


> diff --git a/drivers/edac/dmc520_edac.c b/drivers/edac/dmc520_edac.c 
> new file mode 100644 index 0000000..c70ce4e
> --- /dev/null
> +++ b/drivers/edac/dmc520_edac.c
> @@ -0,0 +1,619 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +// EDAC driver for DMC-520

[..]

> +// DMC-520 types, masks and bitfields
> +#define MEMORY_TYPE_LPDDR3			0
> +#define MEMORY_TYPE_DDR3			1
> +#define MEMORY_TYPE_DDR4			2
> +#define MEMORY_TYPE_LPDDR4			3

(this might be better as an enum as it hints to the compiler that only these values can ever happen. An example of where this is handy further down...)


> +#define MEMORY_DEV_WIDTH_X4			0
> +#define MEMORY_DEV_WIDTH_X8			1
> +#define MEMORY_DEV_WIDTH_X16			2
> +#define MEMORY_DEV_WIDTH_X32			3

(this might be better as an enum)

[Lei] We confirmed with Silicon Vendor the latest register definitions for type and dev width. 

> +struct dmc520_edac {
> +	void __iomem *reg_base;

> +	char message[EDAC_MSG_BUF_SIZE];

Does there need to be an irq-safe lock for this mesage buffer?
Your snprintf() into it in dmc520_handle_dram_ecc_errors(), which is called from both
dmc520_edac_dram_ue_isr() and dmc520_edac_dram_ce_isr()... if these are wired up as different irqs, they could happen on different CPUs concurrently...

[Lei] We modified message to be allocated on stack.

> +};


> +static bool 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 (info) {

I see one caller, dmc520_handle_dram_ecc_errors(), which has info on the stack... is it possible for info to be NULL here?

[Lei] This is not changed since it's fine to check NULL here.

> +		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));
> +		}
> +	}
> +
> +	return valid;
> +}

> +static enum mem_type dmc520_get_mtype(struct dmc520_edac *edac) {
> +	enum mem_type mt;
> +	u32 reg_val, type;
> +
> +	reg_val = dmc520_read_reg(edac, REG_OFFSET_MEMORY_TYPE_NOW);
> +	type = FIELD_GET(REG_FIELD_MEMORY_TYPE, reg_val);
> +
> +	switch (type) {
> +	case MEMORY_TYPE_LPDDR3:
> +	case MEMORY_TYPE_DDR3:
> +		mt = MEM_DDR3;
> +		break;
> +
> +	case MEMORY_TYPE_DDR4:
> +	case MEMORY_TYPE_LPDDR4:
> +	default:

Is this default just to shut the compiler warning up?
If so, you could use an enum and the compiler won't warn for a switch() that has all the cases covered.

[Lei] This was addressed in new PATCH.

> +		mt = MEM_DDR4;
> +		break;
> +	}
> +	return mt;
> +}


> +static irqreturn_t dmc520_edac_dram_ce_isr(int irq, void *data) {
> +	return dmc520_edac_dram_ecc_isr(irq, data, true); }
> +
> +static irqreturn_t dmc520_edac_dram_ue_isr(int irq, void *data) {
> +	return dmc520_edac_dram_ecc_isr(irq, data, false); }

How come these calls aren't folded into the one-size fits all irq handler below?
If the hardware always sets the status register, does the irqchip driver need to call a different helper?

[Lei] I refactored the way how isr is registered. Now there is a one-to-one mapping from each interrupt line 
to its supported interrupt-mask. It's more flexible to adapt to other possible platform implementations for future.

> +static irqreturn_t dmc520_edac_all_isr(int irq, void *data) {
> +	struct mem_ctl_info *mci;
> +	struct dmc520_edac *edac;
> +	u32 status;
> +
> +	mci = data;
> +	edac = mci->pvt_info;
> +
> +	status = dmc520_read_reg(edac, REG_OFFSET_INTERRUPT_STATUS);
> +
> +	if (status & DRAM_ECC_INT_CE_MASK)
> +		dmc520_edac_dram_ce_isr(irq, data);
> +
> +	if (status & DRAM_ECC_INT_UE_MASK)
> +		dmc520_edac_dram_ue_isr(irq, data);
> +
> +	// Other interrupt handlers can be added
> +
> +	return IRQ_HANDLED;


Shouldn't there be a way of getting IRQ_NONE out of this?

[Lei] Addressed.

> +}

[...]


> +static int count_set_bits(u32 n)
> +{
> +	int count;
> +
> +	for (count = 0; n != 0; n &= (n - 1))
> +		count++;
> +
> +	return count;
> +}

Isn't this what hweight() does?

[Lei] Addressed.

> +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, irq, nintr;
> +	struct resource *res;
> +	void __iomem *reg_base;
> +	u32 status, current_bit, interrupt_mask;
> +	bool interrupt_shared;
> +
> +	dev = &pdev->dev;
> +	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);
> +
> +	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));
> +	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;
> +
> +	if (!dmc520_is_ecc_enabled(edac)) {
> +		edac_printk(KERN_ERR, EDAC_MOD_NAME, "ECC not enabled\n");
> +		ret = -ENXIO;
> +		goto err;
> +	}
> +
> +	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_is_scrub_configured(edac) ?
> +			  SCRUB_HW_SRC : SCRUB_NONE;
> +	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;
> +	}
> +
> +	interrupt_shared = false;
> +	interrupt_mask = DRAM_ECC_INT_CE_MASK | DRAM_ECC_INT_UE_MASK;
> +
> +	if (dev->of_node) {
> +		if (of_find_property(dev->of_node, "interrupt-shared", NULL))
> +			interrupt_shared = true;
> +
> +		if (of_property_read_u32(dev->of_node,
> +					 "interrupt-mask",
> +					 &interrupt_mask)) {
> +			edac_printk(KERN_INFO, EDAC_MOD_NAME,
> +				    "Use default interrupt mask 0x%X\n",
> +				    interrupt_mask);
> +		}
> +
> +		interrupt_mask &= ALL_INT_MASK;
> +	}
> +
> +	// Clear interrupts
> +	status = dmc520_read_reg(edac, REG_OFFSET_INTERRUPT_STATUS);
> +	dmc520_write_reg(edac, 0, REG_OFFSET_INTERRUPT_CONTROL);
> +	dmc520_write_reg(edac, ALL_INT_MASK, REG_OFFSET_INTERRUPT_CLR);
> +
> +	// Reset DRAM CE/UE counters
> +	if (interrupt_mask & DRAM_ECC_INT_CE_MASK)
> +		dmc520_get_dram_ecc_error_count(edac, true);
> +
> +	if (interrupt_mask & DRAM_ECC_INT_UE_MASK)
> +		dmc520_get_dram_ecc_error_count(edac, false);
> +
> +	nintr = count_set_bits(interrupt_mask);
> +	if (nintr == 0) {
> +		edac_printk(KERN_ERR, EDAC_MC,
> +			    "Invalid interrupt mask 0x%X\n",
> +			    interrupt_mask);
> +		ret = -EINVAL;
> +		goto err;

This is after edac_mc_add_mc(), and on your remove path you have a edac_mc_del_mc(). Is that necessary here? Do you need an err_del_mc?

[Lei] Addressed.

> +	}
> +
> +	current_bit = BIT(0);
> +	for (irq = 0; irq < nintr; ++irq) {
> +		irq_handler_t edac_isr;
> +		int irq_id = platform_get_irq(pdev, irq);
> +
> +		if (irq_id < 0) {
> +			edac_printk(KERN_ERR, EDAC_MC,
> +				    "Failed to get %s irq\n",
> +				    irq == 0 ? "CE" : "UE");
> +			ret = -ENODEV;
> +			goto err;

err_del_mc?

> +		}
> +
> +		if (interrupt_shared) {
> +			edac_isr = dmc520_edac_all_isr;
> +		} else {
> +			while (current_bit & ALL_INT_MASK) {
> +				if (current_bit & interrupt_mask)
> +					break;
> +
> +				current_bit <<= 1;
> +			}
> +
> +			if (current_bit & DRAM_ECC_INT_CE_MASK) {
> +				edac_isr = dmc520_edac_dram_ce_isr;
> +			} else if (current_bit & DRAM_ECC_INT_UE_MASK) {
> +				edac_isr = dmc520_edac_dram_ue_isr;
> +			} else {
> +				edac_printk(KERN_ERR, EDAC_MC,
> +					    "Invalid interrupt bit 0x%X\n",
> +					    current_bit);
> +				ret = -EINVAL;
> +				goto err;

err_del_mc?

> +			}
> +
> +			current_bit <<= 1;
> +		}
> +
> +		ret = devm_request_irq(&pdev->dev,
> +				       irq_id,
> +				       edac_isr,
> +				       0,
> +				       dev_name(&pdev->dev),
> +				       mci);
> +		if (ret < 0) {
> +			edac_printk(KERN_ERR, EDAC_MC,
> +				    "Failed to request irq %d\n", irq_id);
> +			goto err;

err_del_mc?

> +		}
> +
> +		// Only one irq for all interrupts
> +		if (interrupt_shared)
> +			break;
> +	}
> +
> +	// Enable interrupts
> +	dmc520_write_reg(edac, interrupt_mask, 
> +REG_OFFSET_INTERRUPT_CONTROL);
> +
> +	return 0;
> +
> +err:
> +	edac_mc_free(mci);
> +
> +	return ret;
> +}


[...]

> +MODULE_AUTHOR("Rui Zhao <ruizhao@microsoft.com>");
> +MODULE_DESCRIPTION("DMC-520 ECC driver");

> +MODULE_LICENSE("GPL v2");

This doesn't match your SPDX tag above:
| // SPDX-License-Identifier: GPL-2.0+

Can I use a later version?

[Lei] Addressed

Thanks,

James