subject:"\[PATCH 1\/2\] EDAC\: Add Aspeed AST2500 EDAC driver"

Re: [PATCH 1/2] EDAC: Add Aspeed AST2500 EDAC driver

2019-01-16 Thread Borislav Petkov

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

2019-01-15 Thread Stefan Schaeckeler

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

2019-01-10 Thread 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/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

2018-12-31 Thread Boris Petkov

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

2018-12-31 Thread Stefan Schaeckeler

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

2018-12-16 Thread 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 
+#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:
+

Re: [PATCH 1/2] EDAC: Add Aspeed AST2500 EDAC driver

Re: [PATCH 1/2] EDAC: Add Aspeed AST2500 EDAC driver

Re: [PATCH 1/2] EDAC: Add Aspeed AST2500 EDAC driver

Re: [PATCH 1/2] EDAC: Add Aspeed AST2500 EDAC driver

Re: [PATCH 1/2] EDAC: Add Aspeed AST2500 EDAC driver

[PATCH 1/2] EDAC: Add Aspeed AST2500 EDAC driver

6 matches

Site Navigation

Mail list logo

Footer information