Re: [PATCH V8] EDAC: Add EDAC driver for loongson memory controller

[Zhao Qunqin]

在 2024/11/13 下午12:14, Huacai Chen 写道:
> Hi, Qunqin
>
> On Wed, Nov 13, 2024 at 11:23 AM Zhao Qunqin <zhaoqunqin@xxxxxxxxxxx> wrote:
> > Add ECC support for Loongson SoC DDR controller. This
> > driver reports single bit errors (CE) only.
> >
> > Only ACPI firmware is supported.
> Fair enough, if there is only ACPI firmware support EDAC, we don't
> need to bother FDT.
OK
> > Signed-off-by: Zhao Qunqin <zhaoqunqin@xxxxxxxxxxx>
> > ---
> > Changes in v8:
> >          - Used readl() instead of readq()
> >          - Used acpi_device_id instead of of_device_id, then removed
> >            dt-bindings
> >
> > Changes in v7:
> >          - Fixed sparse's "incorrect type in assignment"
> >          - Cleaned up coding style
> >
> > Changes in v6:
> >          - Changed the Kconfig name to CONFIG_EDAC_LOONGSON
> >
> > Changes in v5:
> >          - Dropepd the loongson_ prefix from all static functions.
> >          - Aligned function arguments on the opening brace.
> >          - Dropepd useless comments and useless wrapper. Dropped side
> >            comments.
> >          - Reordered variable declarations.
> >
> > Changes in v4:
> >          - None
> >
> > Changes in v3:
> >          - Addressed review comments raised by Krzysztof and Huacai
> >
> > Changes in v2:
> >          - Addressed review comments raised by Krzysztof
> >
> >   MAINTAINERS                  |   6 ++
> >   arch/loongarch/Kconfig       |   1 +
> >   drivers/edac/Kconfig         |   8 ++
> >   drivers/edac/Makefile        |   1 +
> >   drivers/edac/loongson_edac.c | 155 +++++++++++++++++++++++++++++++++++
> >   5 files changed, 171 insertions(+)
> >   create mode 100644 drivers/edac/loongson_edac.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index e9659a5a7..b36a45051 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -13397,6 +13397,12 @@ S:     Maintained
> >   F:     Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> >   F:     drivers/thermal/loongson2_thermal.c
> >
> > +LOONGSON EDAC DRIVER
> > +M:     Zhao Qunqin <zhaoqunqin@xxxxxxxxxxx>
> > +L:     linux-edac@xxxxxxxxxxxxxxx
> > +S:     Maintained
> > +F:     drivers/edac/loongson_edac.c
> > +
> >   LSILOGIC MPT FUSION DRIVERS (FC/SAS/SPI)
> >   M:     Sathya Prakash <sathya.prakash@xxxxxxxxxxxx>
> >   M:     Sreekanth Reddy <sreekanth.reddy@xxxxxxxxxxxx>
> > diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
> > index bb35c34f8..10b9ba587 100644
> > --- a/arch/loongarch/Kconfig
> > +++ b/arch/loongarch/Kconfig
> > @@ -185,6 +185,7 @@ config LOONGARCH
> >          select PCI_MSI_ARCH_FALLBACKS
> >          select PCI_QUIRKS
> >          select PERF_USE_VMALLOC
> > +       select EDAC_SUPPORT
> Please use alpha-betical order, which means "select EDAC_SUPPORT"
> should be before "select EFI".
Will fix
> >          select RTC_LIB
> >          select SPARSE_IRQ
> >          select SYSCTL_ARCH_UNALIGN_ALLOW
> > diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> > index 81af6c344..4dce2b92a 100644
> > --- a/drivers/edac/Kconfig
> > +++ b/drivers/edac/Kconfig
> > @@ -564,5 +564,13 @@ config EDAC_VERSAL
> >            Support injecting both correctable and uncorrectable errors
> >            for debugging purposes.
> >
> > +config EDAC_LOONGSON
> > +       tristate "Loongson Memory Controller"
> > +       depends on (LOONGARCH && ACPI) || COMPILE_TEST
> > +       help
> > +         Support for error detection and correction on the Loongson
> > +         family memory controller. This driver reports single bit
> > +         errors (CE) only. Loongson-3A5000/3C5000/3D5000/3A6000/3C6000
> > +         are compatible.
> >
> >   endif # EDAC
> > diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
> > index faf310eec..f8bdbc895 100644
> > --- a/drivers/edac/Makefile
> > +++ b/drivers/edac/Makefile
> > @@ -88,3 +88,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..340722db1
> > --- /dev/null
> > +++ b/drivers/edac/loongson_edac.c
> > @@ -0,0 +1,155 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2024 Loongson Technology Corporation Limited.
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/edac.h>
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> Use alpha-betical order too, which means exchange module.h and init.h.
Will fix
> > +#include <linux/platform_device.h>
> > +#include "edac_module.h"
> > +
> > +#define ECC_CS_COUNT_REG       0x18
> > +
> > +struct loongson_edac_pvt {
> > +       void __iomem *ecc_base;
> > +       int last_ce_count;
> > +};
> > +
> > +static int read_ecc(struct mem_ctl_info *mci)
> > +{
> > +       struct loongson_edac_pvt *pvt = mci->pvt_info;
> > +       u32 ecc;
> > +       int cs;
> > +
> > +       if (!pvt->ecc_base)
> > +               return pvt->last_ce_count;
> > +
> > +       ecc = readl(pvt->ecc_base + ECC_CS_COUNT_REG);
> If the register is actually 64bit, it is better to use readq, and add
> "depends on 64BIT" after config EDAC_LOONGSON.

It seems that including "<linux/io-64-nonatomic-lo-hi.h>" this  file and 
directly using reqdq()  is a better option.

Best regards