Re: [PATCH 1/2] EDAC: Add Aspeed AST2500 EDAC driver
On Tue, Jan 15, 2019 at 09:57:48AM -0800, Stefan Schaeckeler wrote: > That's interesting. I did a grep over all 16944 GPL licensed files with an > SPDX > identifier. > > 785 of them have a license text while 16159 don't. Goes to show that we're still in the process of converting stuff to SPDX. > When stripping off aspeed_edac_, some static function names will become quite > "bare-bone": > > aspeed_edac_init(), aspeed_edac_exit(), aspeed_edac_probe(), > aspeed_edac_remove(), aspeed_edac_of_match(), aspeed_edac_isr(), > aspeed_edac_config_irq(). So namespaced function names we normally use for globally visible symbols and those are not but only driver-specific. So, for example, aspeed_edac_config_irq() is a mouthful, at least to me, and not needed. config_irq(), OTOH, is clear at a very quick glance. The others, aspeed_edac_init(), etc, you could call aspeed_init(), aspeed_exit() or so. But since you're going to be stare at that code, this was just a suggestion. Your call. :) > Does your suggestion also apply to static variables? E.g. aspeed_edac_regmap, > aspeed_edac_regmap_config, aspeed_edac_driver? Also, here some variable names > would become quite "bare-bone". Same as above. HTH. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH 1/2] EDAC: Add Aspeed AST2500 EDAC driver
Hello Boris, Thank you for your feedback. > From: Borislav Petkov > > On Sun, Dec 16, 2018 at 10:01:56PM -0800, Stefan Schaeckeler wrote: > > From: Stefan M Schaeckeler > > > > Add support for the Aspeed AST2500 SoC EDAC driver. > > > > Signed-off-by: Stefan M Schaeckeler > > --- > > MAINTAINERS | 6 + > > arch/arm/boot/dts/aspeed-g5.dtsi | 7 + > > drivers/edac/Kconfig | 7 + > > drivers/edac/Makefile| 1 + > > drivers/edac/aspeed_edac.c | 457 +++ > > 5 files changed, 478 insertions(+) > > create mode 100644 drivers/edac/aspeed_edac.c > > I couldn't see anything out of the ordinary - only nitpicks below. [...] > > diff --git a/drivers/edac/aspeed_edac.c b/drivers/edac/aspeed_edac.c > > new file mode 100644 > > index ..d6ed119909eb > > --- /dev/null > > +++ b/drivers/edac/aspeed_edac.c > > @@ -0,0 +1,457 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright 2018 Cisco Systems > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License > > + * as published by the Free Software Foundation; either version > > + * 2 of the License, or (at your option) any later version. > > You have the SPDX license identifier - no need for that text. That's interesting. I did a grep over all 16944 GPL licensed files with an SPDX identifier. 785 of them have a license text while 16159 don't. I will remove mine. > > +static int aspeed_edac_regmap_reg_write(void *context, unsigned int reg, > > + unsigned int val) > > All the static functions don't need the "aspeed_edac" prefix. When stripping off aspeed_edac_, some static function names will become quite "bare-bone": aspeed_edac_init(), aspeed_edac_exit(), aspeed_edac_probe(), aspeed_edac_remove(), aspeed_edac_of_match(), aspeed_edac_isr(), aspeed_edac_config_irq(). Does your suggestion also apply to static variables? E.g. aspeed_edac_regmap, aspeed_edac_regmap_config, aspeed_edac_driver? Also, here some variable names would become quite "bare-bone". Stefan
Re: [PATCH 1/2] EDAC: Add Aspeed AST2500 EDAC driver
On Sun, Dec 16, 2018 at 10:01:56PM -0800, Stefan Schaeckeler wrote: > From: Stefan M Schaeckeler > > Add support for the Aspeed AST2500 SoC EDAC driver. > > Signed-off-by: Stefan M Schaeckeler > --- > MAINTAINERS | 6 + > arch/arm/boot/dts/aspeed-g5.dtsi | 7 + > drivers/edac/Kconfig | 7 + > drivers/edac/Makefile| 1 + > drivers/edac/aspeed_edac.c | 457 +++ > 5 files changed, 478 insertions(+) > create mode 100644 drivers/edac/aspeed_edac.c I couldn't see anything out of the ordinary - only nitpicks below. > diff --git a/MAINTAINERS b/MAINTAINERS > index 3318f30903b2..1feb92b14029 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -5315,6 +5315,12 @@ L: linux-e...@vger.kernel.org > S: Maintained > F: drivers/edac/amd64_edac* > > +EDAC-AST2500 > +M: Stefan Schaeckeler > +S: Supported > +F: drivers/edac/aspeed_edac.c > +F: Documentation/devicetree/bindings/edac/aspeed-sdram-edac.txt > + > EDAC-CALXEDA > M: Robert Richter > L: linux-e...@vger.kernel.org > diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi > b/arch/arm/boot/dts/aspeed-g5.dtsi > index d107459fc0f8..b4e479ab5a2d 100644 > --- a/arch/arm/boot/dts/aspeed-g5.dtsi > +++ b/arch/arm/boot/dts/aspeed-g5.dtsi > @@ -47,6 +47,13 @@ > reg = <0x8000 0>; > }; > > + edac: sdram@1e6e { > + compatible = "aspeed,ast2500-sdram-edac"; > + reg = <0x1e6e 0x174>; > + interrupts = <0>; > + status = "disabled"; > + }; > + > ahb { > compatible = "simple-bus"; > #address-cells = <1>; > diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig > index 41c9ccdd20d6..67834430b0a1 100644 > --- a/drivers/edac/Kconfig > +++ b/drivers/edac/Kconfig > @@ -475,4 +475,11 @@ config EDAC_QCOM > For debugging issues having to do with stability and overall system > health, you should probably say 'Y' here. > > +config EDAC_ASPEED > + tristate "Aspeed AST 2500 SoC" > + depends on MACH_ASPEED_G5 > + help > + Support for error detection and correction on the > + Aspeed AST 2500 SoC. > + > endif # EDAC > diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile > index 716096d08ea0..e1f23d4ff860 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_ASPEED)+= aspeed_edac.o > diff --git a/drivers/edac/aspeed_edac.c b/drivers/edac/aspeed_edac.c > new file mode 100644 > index ..d6ed119909eb > --- /dev/null > +++ b/drivers/edac/aspeed_edac.c > @@ -0,0 +1,457 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright 2018 Cisco Systems > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; either version > + * 2 of the License, or (at your option) any later version. You have the SPDX license identifier - no need for that text. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include "edac_module.h" > + > + > +#define DRV_NAME "aspeed-edac" > + > + > +/* registers */ no need for that comment > +#define ASPEED_MCR_PROT0x00 /* protection key register */ > +#define ASPEED_MCR_CONF0x04 /* configuration register */ > +#define ASPEED_MCR_INTR_CTRL 0x50 /* interrupt control/status register */ > +#define ASPEED_MCR_ADDR_UNREC 0x58 /* address of first un-recoverable error > */ > +#define ASPEED_MCR_ADDR_REC0x5c /* address of last recoverable error */ > +#define ASPEED_MCR_LASTASPEED_MCR_ADDR_REC > + > + > +/* bits and masks */ ditto > +#define ASPEED_MCR_PROT_PASSWD 0xfc600309 > +#define ASPEED_MCR_CONF_DRAM_TYPE BIT(4) > +#define ASPEED_MCR_CONF_ECC BIT(7) > +#define ASPEED_MCR_INTR_CTRL_CLEAR BIT(31) > +#define ASPEED_MCR_INTR_CTRL_CNT_REC GENMASK(23, 16) > +#define ASPEED_MCR_INTR_CTRL_CNT_UNREC GENMASK(15, 12) > +#define ASPEED_MCR_INTR_CTRL_ENABLE (BIT(0) | BIT(1)) > + > + > + > +static int aspeed_edac_regmap_reg_write(void *context, unsigned int reg, > + unsigned int val) All the static functions don't need the "aspeed_edac" prefix. > +{ > + void __iomem *regs = (void __iomem *)context; > + > + /* enable write to MCR register set */ > + writel(ASPEED_MCR_PROT_PASSWD, regs + ASPEED_MCR_PROT); > + > + writel(val, regs + reg); > + > + /* disable write to MCR register set */ > + writel(~ASPEED_MCR_PROT_PASSWD, regs
Re: [PATCH 1/2] EDAC: Add Aspeed AST2500 EDAC driver
On December 31, 2018 3:20:00 PM GMT+02:00, Stefan Schaeckeler wrote: >Let me start with reviewing my own driver. Perhaps someone could review >it as well, please? Someone will review it when the merge window and vacations are over. -- Sent from a small device: formatting sux and brevity is inevitable.
Re: [PATCH 1/2] EDAC: Add Aspeed AST2500 EDAC driver
Let me start with reviewing my own driver. Perhaps someone could review it as well, please? I found a cosmetic issue and a bug. See inline. > From: Stefan Schaeckeler > > From: Stefan M Schaeckeler > > Add support for the Aspeed AST2500 SoC EDAC driver. > > Signed-off-by: Stefan M Schaeckeler > --- > MAINTAINERS | 6 + > arch/arm/boot/dts/aspeed-g5.dtsi | 7 + > drivers/edac/Kconfig | 7 + > drivers/edac/Makefile| 1 + > drivers/edac/aspeed_edac.c | 457 +++ > 5 files changed, 478 insertions(+) > create mode 100644 drivers/edac/aspeed_edac.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 3318f30903b2..1feb92b14029 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -5315,6 +5315,12 @@ L: linux-e...@vger.kernel.org > S: Maintained > F: drivers/edac/amd64_edac* > > +EDAC-AST2500 > +M: Stefan Schaeckeler > +S: Supported > +F: drivers/edac/aspeed_edac.c > +F: Documentation/devicetree/bindings/edac/aspeed-sdram-edac.txt > + > EDAC-CALXEDA > M: Robert Richter > L: linux-e...@vger.kernel.org > diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi > b/arch/arm/boot/dts/aspeed-g5.dtsi > index d107459fc0f8..b4e479ab5a2d 100644 > --- a/arch/arm/boot/dts/aspeed-g5.dtsi > +++ b/arch/arm/boot/dts/aspeed-g5.dtsi > @@ -47,6 +47,13 @@ > reg = <0x8000 0>; > }; > > + edac: sdram@1e6e { > + compatible = "aspeed,ast2500-sdram-edac"; > + reg = <0x1e6e 0x174>; > + interrupts = <0>; > + status = "disabled"; > + }; > + > ahb { > compatible = "simple-bus"; > #address-cells = <1>; > diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig > index 41c9ccdd20d6..67834430b0a1 100644 > --- a/drivers/edac/Kconfig > +++ b/drivers/edac/Kconfig > @@ -475,4 +475,11 @@ config EDAC_QCOM > For debugging issues having to do with stability and overall system > health, you should probably say 'Y' here. > > +config EDAC_ASPEED > + tristate "Aspeed AST 2500 SoC" > + depends on MACH_ASPEED_G5 > + help > + Support for error detection and correction on the > + Aspeed AST 2500 SoC. > + > endif # EDAC > diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile > index 716096d08ea0..e1f23d4ff860 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_ASPEED)+= aspeed_edac.o > diff --git a/drivers/edac/aspeed_edac.c b/drivers/edac/aspeed_edac.c > new file mode 100644 > index ..d6ed119909eb > --- /dev/null > +++ b/drivers/edac/aspeed_edac.c > @@ -0,0 +1,457 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright 2018 Cisco Systems > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; either version > + * 2 of the License, or (at your option) any later version. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include Including asm/page.h does not seem to be necessary. > +#include "edac_module.h" > + > + > +#define DRV_NAME "aspeed-edac" > + > + > +/* registers */ > +#define ASPEED_MCR_PROT0x00 /* protection key register */ > +#define ASPEED_MCR_CONF0x04 /* configuration register */ > +#define ASPEED_MCR_INTR_CTRL 0x50 /* interrupt control/status register */ > +#define ASPEED_MCR_ADDR_UNREC 0x58 /* address of first un-recoverable error > */ > +#define ASPEED_MCR_ADDR_REC0x5c /* address of last recoverable error */ > +#define ASPEED_MCR_LASTASPEED_MCR_ADDR_REC > + > + > +/* bits and masks */ > +#define ASPEED_MCR_PROT_PASSWD 0xfc600309 > +#define ASPEED_MCR_CONF_DRAM_TYPE BIT(4) > +#define ASPEED_MCR_CONF_ECC BIT(7) > +#define ASPEED_MCR_INTR_CTRL_CLEAR BIT(31) > +#define ASPEED_MCR_INTR_CTRL_CNT_REC GENMASK(23, 16) > +#define ASPEED_MCR_INTR_CTRL_CNT_UNREC GENMASK(15, 12) > +#define ASPEED_MCR_INTR_CTRL_ENABLE (BIT(0) | BIT(1)) > + > + > + > +static int aspeed_edac_regmap_reg_write(void *context, unsigned int reg, > + unsigned int val) > +{ > + void __iomem *regs = (void __iomem *)context; > + > + /* enable write to MCR register set */ > + writel(ASPEED_MCR_PROT_PASSWD, regs + ASPEED_MCR_PROT); > + > + writel(val, regs + reg); > + > + /* disable write to MCR register set */ > + writel(~ASPEED_MCR_PROT_PASSWD, regs + ASPEED_MCR_PROT); > + > + return 0; > +} > + > + > +static
[PATCH 1/2] EDAC: Add Aspeed AST2500 EDAC driver
From: Stefan M Schaeckeler Add support for the Aspeed AST2500 SoC EDAC driver. Signed-off-by: Stefan M Schaeckeler --- MAINTAINERS | 6 + arch/arm/boot/dts/aspeed-g5.dtsi | 7 + drivers/edac/Kconfig | 7 + drivers/edac/Makefile| 1 + drivers/edac/aspeed_edac.c | 457 +++ 5 files changed, 478 insertions(+) create mode 100644 drivers/edac/aspeed_edac.c diff --git a/MAINTAINERS b/MAINTAINERS index 3318f30903b2..1feb92b14029 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5315,6 +5315,12 @@ L: linux-e...@vger.kernel.org S: Maintained F: drivers/edac/amd64_edac* +EDAC-AST2500 +M: Stefan Schaeckeler +S: Supported +F: drivers/edac/aspeed_edac.c +F: Documentation/devicetree/bindings/edac/aspeed-sdram-edac.txt + EDAC-CALXEDA M: Robert Richter L: linux-e...@vger.kernel.org diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi index d107459fc0f8..b4e479ab5a2d 100644 --- a/arch/arm/boot/dts/aspeed-g5.dtsi +++ b/arch/arm/boot/dts/aspeed-g5.dtsi @@ -47,6 +47,13 @@ reg = <0x8000 0>; }; + edac: sdram@1e6e { + compatible = "aspeed,ast2500-sdram-edac"; + reg = <0x1e6e 0x174>; + interrupts = <0>; + status = "disabled"; + }; + ahb { compatible = "simple-bus"; #address-cells = <1>; diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig index 41c9ccdd20d6..67834430b0a1 100644 --- a/drivers/edac/Kconfig +++ b/drivers/edac/Kconfig @@ -475,4 +475,11 @@ config EDAC_QCOM For debugging issues having to do with stability and overall system health, you should probably say 'Y' here. +config EDAC_ASPEED + tristate "Aspeed AST 2500 SoC" + depends on MACH_ASPEED_G5 + help + Support for error detection and correction on the + Aspeed AST 2500 SoC. + endif # EDAC diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile index 716096d08ea0..e1f23d4ff860 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_ASPEED) += aspeed_edac.o diff --git a/drivers/edac/aspeed_edac.c b/drivers/edac/aspeed_edac.c new file mode 100644 index ..d6ed119909eb --- /dev/null +++ b/drivers/edac/aspeed_edac.c @@ -0,0 +1,457 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright 2018 Cisco Systems + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version + * 2 of the License, or (at your option) any later version. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "edac_module.h" + + +#define DRV_NAME "aspeed-edac" + + +/* registers */ +#define ASPEED_MCR_PROT0x00 /* protection key register */ +#define ASPEED_MCR_CONF0x04 /* configuration register */ +#define ASPEED_MCR_INTR_CTRL 0x50 /* interrupt control/status register */ +#define ASPEED_MCR_ADDR_UNREC 0x58 /* address of first un-recoverable error */ +#define ASPEED_MCR_ADDR_REC0x5c /* address of last recoverable error */ +#define ASPEED_MCR_LASTASPEED_MCR_ADDR_REC + + +/* bits and masks */ +#define ASPEED_MCR_PROT_PASSWD 0xfc600309 +#define ASPEED_MCR_CONF_DRAM_TYPE BIT(4) +#define ASPEED_MCR_CONF_ECC BIT(7) +#define ASPEED_MCR_INTR_CTRL_CLEAR BIT(31) +#define ASPEED_MCR_INTR_CTRL_CNT_REC GENMASK(23, 16) +#define ASPEED_MCR_INTR_CTRL_CNT_UNREC GENMASK(15, 12) +#define ASPEED_MCR_INTR_CTRL_ENABLE (BIT(0) | BIT(1)) + + + +static int aspeed_edac_regmap_reg_write(void *context, unsigned int reg, + unsigned int val) +{ + void __iomem *regs = (void __iomem *)context; + + /* enable write to MCR register set */ + writel(ASPEED_MCR_PROT_PASSWD, regs + ASPEED_MCR_PROT); + + writel(val, regs + reg); + + /* disable write to MCR register set */ + writel(~ASPEED_MCR_PROT_PASSWD, regs + ASPEED_MCR_PROT); + + return 0; +} + + +static int aspeed_edac_regmap_reg_read(void *context, unsigned int reg, + unsigned int *val) +{ + void __iomem *regs = (void __iomem *)context; + + *val = readl(regs + reg); + + return 0; +} + +static bool aspeed_edac_regmap_is_volatile(struct device *dev, + unsigned int reg) +{ + switch (reg) { + case ASPEED_MCR_PROT: + case ASPEED_MCR_INTR_CTRL: +