Re: [PATCH v1 1/2] Loongarch: EDAC driver for loongson memory controller

[Krzysztof Kozlowski]

On 21/08/2024 08:47, Zhao Qunqin wrote:
> From: zhaoqunqin <zhaoqunqin@xxxxxxxxxxx>
> 
> Report single bit errors (CE) only
> 
> Signed-off-by: zhaoqunqin <zhaoqunqin@xxxxxxxxxxx>
> ---
>  arch/loongarch/Kconfig       |   2 +
>  drivers/edac/Kconfig         |  10 ++
>  drivers/edac/Makefile        |   1 +
>  drivers/edac/loongson_edac.c | 208 +++++++++++++++++++++++++++++++++++
>  4 files changed, 221 insertions(+)
>  create mode 100644 drivers/edac/loongson_edac.c
> 
> diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
> index ddc042895..59d47053f 100644
> --- a/arch/loongarch/Kconfig
> +++ b/arch/loongarch/Kconfig
> @@ -179,6 +179,8 @@ config LOONGARCH
>  	select PCI_QUIRKS
>  	select PERF_USE_VMALLOC
>  	select RTC_LIB
> +	select EDAC_SUPPORT
> +	select EDAC

Nope, you should not select user-visible sumbols.

>  	select SPARSE_IRQ
>  	select SYSCTL_ARCH_UNALIGN_ALLOW
>  	select SYSCTL_ARCH_UNALIGN_NO_WARN
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index 16c8de505..60b1997f0 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -573,5 +573,15 @@ config EDAC_VERSAL
>  	  Support injecting both correctable and uncorrectable errors
>  	  for debugging purposes.
>  
> +config EDAC_LOONGSON
> +	tristate "Loongson EDAC"
> +	depends on LOONGARCH

Missing compile test

> +	default m
> +	help
> +	  Support for error detection and correction on the loongson memory
> +	  controller.
> +
> +	  Report single bit errors (CE) only.
> +

Why double line? Drop

>  
>  endif # EDAC
> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
> index 4edfb83ff..d6f2cfe7e 100644
> --- a/drivers/edac/Makefile
> +++ b/drivers/edac/Makefile
> @@ -89,3 +89,4 @@ obj-$(CONFIG_EDAC_DMC520)		+= dmc520_edac.o
>  obj-$(CONFIG_EDAC_NPCM)			+= npcm_edac.o
>  obj-$(CONFIG_EDAC_ZYNQMP)		+= zynqmp_edac.o
>  obj-$(CONFIG_EDAC_VERSAL)		+= versal_edac.o
> +obj-$(CONFIG_EDAC_LOONGSON)		+= loongson_edac.o
> diff --git a/drivers/edac/loongson_edac.c b/drivers/edac/loongson_edac.c
> new file mode 100644
> index 000000000..c639c11ed
> --- /dev/null
> +++ b/drivers/edac/loongson_edac.c
> @@ -0,0 +1,208 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2024 Loongson Technology Corporation Limited.
> + */
> +
> +#include <linux/edac.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +
> +#include "edac_module.h"
> +
> +enum ecc_index {
> +	ECC_SET = 0,
> +	ECC_RESERVED,
> +	ECC_COUNT,
> +	ECC_CS_COUNT,
> +	ECC_CODE,
> +	ECC_ADDR,
> +	ECC_DATA0,
> +	ECC_DATA1,
> +	ECC_DATA2,
> +	ECC_DATA3,
> +};
> +
> +static long idx;

Drop, racy and useless. If you need ID, then use IDR but first explain
what purpose does it serve.

> +
> +struct loongson_edac_pvt {
> +	volatile u64 *ecc_base;
> +	int last_ce_count;
> +};
> +
> +static void loongson_update_ce_count(struct mem_ctl_info *mci,
> +					int chan,
> +					int new)
> +{
> +	int add;
> +	struct loongson_edac_pvt *pvt = mci->pvt_info;
> +
> +	add = new - pvt->last_ce_count;
> +
> +	/* Store the new values */
> +	pvt->last_ce_count = new;
> +
> +	/* device resume or any other exceptions*/
> +	if (add < 0)
> +		return;
> +
> +	/*updated the edac core */
> +	if (add != 0) {
> +		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, add,
> +					0, 0, 0,
> +					chan, 0, -1, "error", "");
> +		edac_mc_printk(mci, KERN_INFO, "add: %d", add);
> +	}
> +}
> +
> +static int loongson_read_ecc(struct mem_ctl_info *mci)
> +{
> +	u64 ecc;
> +	int cs = 0;
> +	struct loongson_edac_pvt *pvt = mci->pvt_info;
> +
> +	if (!pvt->ecc_base)
> +		return pvt->last_ce_count;
> +
> +	ecc = pvt->ecc_base[ECC_CS_COUNT];
> +	cs += ecc & 0xff;		// cs0
> +	cs += (ecc >> 8) & 0xff;	// cs1
> +	cs += (ecc >> 16) & 0xff;	// cs2
> +	cs += (ecc >> 24) & 0xff;	// cs3
> +
> +	return cs;
> +}
> +
> +static void loongson_edac_check(struct mem_ctl_info *mci)
> +{
> +	loongson_update_ce_count(mci, 0, loongson_read_ecc(mci));
> +}
> +
> +static int get_dimm_config(struct mem_ctl_info *mci)
> +{
> +	u32 size, npages;
> +	struct dimm_info *dimm;
> +
> +	/* size not used */
> +	size = -1;
> +	npages = MiB_TO_PAGES(size);
> +
> +	dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms, mci->n_layers,
> +			0, 0, 0);
> +	dimm->nr_pages = npages;
> +	snprintf(dimm->label, sizeof(dimm->label),
> +			"MC#%uChannel#%u_DIMM#%u",
> +			mci->mc_idx, 0, 0);
> +	dimm->grain = 8;
> +
> +	return 0;
> +}
> +
> +static void loongson_pvt_init(struct mem_ctl_info *mci, u64 *vbase)
> +{
> +	struct loongson_edac_pvt *pvt = mci->pvt_info;
> +
> +	pvt->ecc_base = vbase;
> +	pvt->last_ce_count = loongson_read_ecc(mci);
> +}
> +
> +static int loongson_edac_probe(struct platform_device *pdev)
> +{
> +	struct resource *rs;
> +	struct mem_ctl_info *mci;
> +	struct edac_mc_layer layers[2];
> +	struct loongson_edac_pvt *pvt;
> +	u64 *vbase = NULL;
> +
> +	rs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!rs)
> +		return -EINVAL;
> +	if (rs->start) {
> +		vbase = devm_ioremap(&pdev->dev, rs->start, resource_size(rs));

Why you cannot use wrapper over these two calls - devm_ioremap_resource?

> +		if (!vbase)
> +			return -ENOMEM;
> +	}
> +
> +	/* allocate a new MC control structure */
> +	layers[0].type = EDAC_MC_LAYER_CHANNEL;
> +	layers[0].size = 1;
> +	layers[0].is_virt_csrow = false;
> +	layers[1].type = EDAC_MC_LAYER_SLOT;
> +	layers[1].size = 1;
> +	layers[1].is_virt_csrow = true;
> +	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, sizeof(*pvt));
> +	if (mci == NULL)
> +		return -ENOMEM;
> +
> +	edac_dbg(0, "MC: mci = %p\n", mci);
> +
> +	mci->mc_idx = idx++;
> +	mci->mtype_cap = MEM_FLAG_RDDR4;
> +	mci->edac_ctl_cap = EDAC_FLAG_NONE;
> +	mci->edac_cap = EDAC_FLAG_NONE;
> +	mci->mod_name = "loongson_edac.c";
> +	mci->ctl_name = "loongson_edac_ctl";
> +	mci->dev_name = "loongson_edac_dev";
> +	mci->ctl_page_to_phys = NULL;
> +	mci->pdev = &pdev->dev;
> +	mci->error_desc.grain = 8;
> +	/* Set the function pointer to an actual operation function */
> +	mci->edac_check = loongson_edac_check;
> +
> +	loongson_pvt_init(mci, vbase);
> +	get_dimm_config(mci);
> +
> +	if (edac_mc_add_mc(mci)) {
> +		edac_dbg(0, "MC: failed edac_mc_add_mc()\n");
> +		edac_mc_free(mci);
> +	}
> +	return 0;
> +}
> +
> +static int loongson_edac_remove(struct platform_device *pdev)
> +{
> +	struct mem_ctl_info *mci = edac_mc_del_mc(&pdev->dev);
> +
> +	if (mci) {
> +		edac_mc_free(mci);
> +		return 0;
> +	}
> +	return -ENODEV;
> +}
> +
> +static const struct of_device_id loongson_edac_of_match[] = {
> +	{ .compatible = "loongson,ls-mc-edac", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, loongson_edac_of_match);
> +
> +static struct platform_driver loongson_edac_driver = {
> +	.probe		= loongson_edac_probe,
> +	.remove		= loongson_edac_remove,
> +	.driver		= {
> +		.name	= "ls-mc-edac",
> +		.owner = THIS_MODULE,

Drop... that's ancient code.

> +		.of_match_table = loongson_edac_of_match,
> +	},
> +};
> +
> +static int __init loongson_edac_init(void)
> +{
> +	/* poll only */
> +	edac_op_state = EDAC_OPSTATE_POLL;

Drop, unused. Clean your driver before posting it.

> +
> +	return platform_driver_register(&loongson_edac_driver);
> +}
> +
> +static void __exit loongson_edac_exit(void)
> +{
> +	platform_driver_unregister(&loongson_edac_driver);
> +}
> +
> +module_init(loongson_edac_init);
> +module_exit(loongson_edac_exit);

module_platform_driver

> +module_param(edac_op_state, int, 0444);

Drop

> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Zhao Qunqin <zhaoqunqin@xxxxxxxxxxx>\n");
> +MODULE_DESCRIPTION("EDAC driver for loongson memory controller");

Best regards,
Krzysztof