* [PATCH 1/2] EDAC: Add Aspeed AST2500 EDAC driver
From: Stefan Schaeckeler @ 2019-01-15 17:57 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20190110095049.GB17621@zn.tnic>
Hello Boris,
Thank you for your feedback.
> From: Borislav Petkov <bp@alien8.de>
>
> On Sun, Dec 16, 2018 at 10:01:56PM -0800, Stefan Schaeckeler wrote:
> > From: Stefan M Schaeckeler <sschaeck@cisco.com>
> >
> > Add support for the Aspeed AST2500 SoC EDAC driver.
> >
> > Signed-off-by: Stefan M Schaeckeler <sschaeck@cisco.com>
> > ---
> > 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 000000000000..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
^ permalink raw reply
* [PATCH] misc: aspeed-lpc-ctrl: make memory-region optional
From: Vijay Khemka @ 2019-01-15 1:51 UTC (permalink / raw)
To: linux-aspeed
Makiing memory-region as optional parameter in device tree if
user needs to use memory-region then define in devicetree.
Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
drivers/misc/aspeed-lpc-ctrl.c | 30 ++++++++++++++++++------------
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c
index a024f8042259..20507f0764fb 100644
--- a/drivers/misc/aspeed-lpc-ctrl.c
+++ b/drivers/misc/aspeed-lpc-ctrl.c
@@ -90,6 +90,10 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
if (map.window_id != 0)
return -EINVAL;
+ /* If memory-region is not described in device tree */
+ if (!lpc_ctrl->mem_size)
+ return -EINVAL;
+
map.size = lpc_ctrl->mem_size;
return copy_to_user(p, &map, sizeof(map)) ? -EFAULT : 0;
@@ -129,6 +133,10 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
addr = lpc_ctrl->pnor_base;
size = lpc_ctrl->pnor_size;
} else if (map.window_type == ASPEED_LPC_CTRL_WINDOW_MEMORY) {
+ /* If memory-region is not described in device tree */
+ if (!lpc_ctrl->mem_size)
+ return -EINVAL;
+
addr = lpc_ctrl->mem_base;
size = lpc_ctrl->mem_size;
} else {
@@ -214,22 +222,20 @@ static int aspeed_lpc_ctrl_probe(struct platform_device *pdev)
dev_set_drvdata(&pdev->dev, lpc_ctrl);
+ /* If memory-region is described in device tree then store */
node = of_parse_phandle(dev->of_node, "memory-region", 0);
- if (!node) {
- dev_err(dev, "Didn't find reserved memory\n");
- return -EINVAL;
- }
+ if (node) {
+ rc = of_address_to_resource(node, 0, &resm);
+ of_node_put(node);
+ if (rc) {
+ dev_err(dev, "Couldn't address to resource for reserved memory\n");
+ return -ENOMEM;
+ }
- rc = of_address_to_resource(node, 0, &resm);
- of_node_put(node);
- if (rc) {
- dev_err(dev, "Couldn't address to resource for reserved memory\n");
- return -ENOMEM;
+ lpc_ctrl->mem_size = resource_size(&resm);
+ lpc_ctrl->mem_base = resm.start;
}
- lpc_ctrl->mem_size = resource_size(&resm);
- lpc_ctrl->mem_base = resm.start;
-
lpc_ctrl->regmap = syscon_node_to_regmap(
pdev->dev.parent->of_node);
if (IS_ERR(lpc_ctrl->regmap)) {
--
2.17.1
^ permalink raw reply related
* [Potential Spoof] Re: [Potential Spoof] Re: [PATCH v2 4/4] ARM: dts: aspeed: Add lpc ctrl for Facebook
From: Vijay Khemka @ 2019-01-14 19:28 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1547419180.2315773.1633559160.3F7D29CF@webmail.messagingengine.com>
?On 1/13/19, 2:39 PM, "Andrew Jeffery" <andrew@aj.id.au> wrote:
Hi Vijay,
Sorry for providing an opinion so late, however:
On Sat, 12 Jan 2019, at 11:03, Vijay Khemka wrote:
> Joel,
> Please merge these patches as it is required by facebook platform.
>
> Regards
> -Vijay
>
> On 1/7/19, 11:25 AM, "Linux-aspeed on behalf of Vijay Khemka" <linux-
> aspeed-bounces+vijaykhemka=fb.com at lists.ozlabs.org on behalf of
> vijaykhemka at fb.com> wrote:
>
> Please merge these patches in upstream kernel.
>
> Regards
> -Vijay
>
> On 12/20/18, 10:06 AM, "Linux-aspeed on behalf of Vijay Khemka"
> <linux-aspeed-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of
> vijaykhemka at fb.com> wrote:
>
> Joel, Can you please take care of these patches merge.
>
> On 12/17/18, 12:04 PM, "Vijay Khemka" <vijaykhemka@fb.com> wrote:
>
> Added lpc ctrl device to enable LPC clock in Facebook
> Tiogapass device tree.
>
> Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
> ---
> .../boot/dts/aspeed-bmc-facebook-tiogapass.dts | 17 ++++++
> +++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-
> tiogapass.dts b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> index 73e58a821613..ef7875b54562 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> @@ -22,6 +22,17 @@
> reg = <0x80000000 0x20000000>;
> };
>
> + reserved-memory {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + flash_memory: region at 98000000 {
> + no-map;
> + reg = <0x98000000 0x00001000>; /* 4K */
> + };
> + };
> +
> iio-hwmon {
> compatible = "iio-hwmon";
> io-channels = <&adc 0>, <&adc 1>, <&adc 2>, <&adc 3>,
> @@ -54,6 +65,12 @@
> };
> };
>
> +&lpc_ctrl {
> + status = "okay";
> + memory-region = <&flash_memory>;
> + flash = <&spi1>;
> +};
> +
> &uart1 {
> // Host Console
> status = "okay";
> --
> 2.17.1
I understand you just want to make your system work, but this is a
broken way to do it. And that's not your fault - we should really fix
the driver. I think the memory-region phandle should be optional
and the driver should simply limit the options available to the ioctl
to just the flash region if the memory node is not present (i.e. return
an error if the memory region is requested but not described in the
devicetree).
Let me fix driver itself first.
Andrew
^ permalink raw reply
* [Potential Spoof] Re: [Potential Spoof] Re: [PATCH v2 4/4] ARM: dts: aspeed: Add lpc ctrl for Facebook
From: Andrew Jeffery @ 2019-01-13 22:39 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <5972B57A-23D5-43F4-9476-229379B96C7C@fb.com>
Hi Vijay,
Sorry for providing an opinion so late, however:
On Sat, 12 Jan 2019, at 11:03, Vijay Khemka wrote:
> Joel,
> Please merge these patches as it is required by facebook platform.
>
> Regards
> -Vijay
>
> ?On 1/7/19, 11:25 AM, "Linux-aspeed on behalf of Vijay Khemka" <linux-
> aspeed-bounces+vijaykhemka=fb.com at lists.ozlabs.org on behalf of
> vijaykhemka at fb.com> wrote:
>
> Please merge these patches in upstream kernel.
>
> Regards
> -Vijay
>
> On 12/20/18, 10:06 AM, "Linux-aspeed on behalf of Vijay Khemka"
> <linux-aspeed-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of
> vijaykhemka at fb.com> wrote:
>
> Joel, Can you please take care of these patches merge.
>
> On 12/17/18, 12:04 PM, "Vijay Khemka" <vijaykhemka@fb.com> wrote:
>
> Added lpc ctrl device to enable LPC clock in Facebook
> Tiogapass device tree.
>
> Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
> ---
> .../boot/dts/aspeed-bmc-facebook-tiogapass.dts | 17 ++++++
> +++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-
> tiogapass.dts b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> index 73e58a821613..ef7875b54562 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> @@ -22,6 +22,17 @@
> reg = <0x80000000 0x20000000>;
> };
>
> + reserved-memory {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + flash_memory: region at 98000000 {
> + no-map;
> + reg = <0x98000000 0x00001000>; /* 4K */
> + };
> + };
> +
> iio-hwmon {
> compatible = "iio-hwmon";
> io-channels = <&adc 0>, <&adc 1>, <&adc 2>, <&adc 3>,
> @@ -54,6 +65,12 @@
> };
> };
>
> +&lpc_ctrl {
> + status = "okay";
> + memory-region = <&flash_memory>;
> + flash = <&spi1>;
> +};
> +
> &uart1 {
> // Host Console
> status = "okay";
> --
> 2.17.1
I understand you just want to make your system work, but this is a
broken way to do it. And that's not your fault - we should really fix
the driver. I think the memory-region phandle should be optional
and the driver should simply limit the options available to the ioctl
to just the flash region if the memory node is not present (i.e. return
an error if the memory region is requested but not described in the
devicetree).
Andrew
^ permalink raw reply
* phosphor-ipmi-host.service broken
From: Andrew Jeffery @ 2019-01-13 22:11 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <F7A6D855-479B-48FD-98C0-FEAAB372B52A@fb.com>
Hi Vijay,
The linux-aspeed at lists.ozlabs.org list is for kernel patches going upstream - Joel uses it as a way to track relevant patches in patchwork. For openbmc discussions (i.e. userspace questions like this) please use openbmc at lists.ozlabs.org
On Thu, 10 Jan 2019, at 05:12, Vijay Khemka wrote:
> I see new changes in this phosphor-ipmi-host.service file where it needs
> org.openbmc.HostIpmi.service service to be running and this
> org.openbmc.HostIpmi.service service depends on phosphor-ipmi-
> host.service file. So it is cyclic dependency which in turn not letting
> run any of these service.
We've fixed this: https://gerrit.openbmc-project.xyz/#/c/openbmc/meta-phosphor/+/16856/
Sorry for breaking things - I had forgotten about the KCS backend.
Andrew
^ permalink raw reply
* [Potential Spoof] Re: [Potential Spoof] Re: [PATCH v2 4/4] ARM: dts: aspeed: Add lpc ctrl for Facebook
From: Vijay Khemka @ 2019-01-12 0:33 UTC (permalink / raw)
To: linux-aspeed
Joel,
Please merge these patches as it is required by facebook platform.
Regards
-Vijay
?On 1/7/19, 11:25 AM, "Linux-aspeed on behalf of Vijay Khemka" <linux-aspeed-bounces+vijaykhemka=fb.com at lists.ozlabs.org on behalf of vijaykhemka@fb.com> wrote:
Please merge these patches in upstream kernel.
Regards
-Vijay
On 12/20/18, 10:06 AM, "Linux-aspeed on behalf of Vijay Khemka" <linux-aspeed-bounces+vijaykhemka=fb.com at lists.ozlabs.org on behalf of vijaykhemka@fb.com> wrote:
Joel, Can you please take care of these patches merge.
On 12/17/18, 12:04 PM, "Vijay Khemka" <vijaykhemka@fb.com> wrote:
Added lpc ctrl device to enable LPC clock in Facebook
Tiogapass device tree.
Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
.../boot/dts/aspeed-bmc-facebook-tiogapass.dts | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index 73e58a821613..ef7875b54562 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -22,6 +22,17 @@
reg = <0x80000000 0x20000000>;
};
+ reserved-memory {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ flash_memory: region at 98000000 {
+ no-map;
+ reg = <0x98000000 0x00001000>; /* 4K */
+ };
+ };
+
iio-hwmon {
compatible = "iio-hwmon";
io-channels = <&adc 0>, <&adc 1>, <&adc 2>, <&adc 3>,
@@ -54,6 +65,12 @@
};
};
+&lpc_ctrl {
+ status = "okay";
+ memory-region = <&flash_memory>;
+ flash = <&spi1>;
+};
+
&uart1 {
// Host Console
status = "okay";
--
2.17.1
^ permalink raw reply
* [PATCH 1/2] EDAC: Add Aspeed AST2500 EDAC driver
From: Borislav Petkov @ 2019-01-10 9:50 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1545026517-64069-2-git-send-email-schaecsn@gmx.net>
On Sun, Dec 16, 2018 at 10:01:56PM -0800, Stefan Schaeckeler wrote:
> From: Stefan M Schaeckeler <sschaeck@cisco.com>
>
> Add support for the Aspeed AST2500 SoC EDAC driver.
>
> Signed-off-by: Stefan M Schaeckeler <sschaeck@cisco.com>
> ---
> 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-edac at vger.kernel.org
> S: Maintained
> F: drivers/edac/amd64_edac*
>
> +EDAC-AST2500
> +M: Stefan Schaeckeler <sschaeck@cisco.com>
> +S: Supported
> +F: drivers/edac/aspeed_edac.c
> +F: Documentation/devicetree/bindings/edac/aspeed-sdram-edac.txt
> +
> EDAC-CALXEDA
> M: Robert Richter <rric@kernel.org>
> L: linux-edac at 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 = <0x80000000 0>;
> };
>
> + edac: sdram at 1e6e0000 {
> + compatible = "aspeed,ast2500-sdram-edac";
> + reg = <0x1e6e0000 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 000000000000..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 <linux/edac.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/stop_machine.h>
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +#include <linux/regmap.h>
> +#include <asm/page.h>
> +#include "edac_module.h"
> +
> +
> +#define DRV_NAME "aspeed-edac"
> +
> +
> +/* registers */
no need for that comment
> +#define ASPEED_MCR_PROT 0x00 /* protection key register */
> +#define ASPEED_MCR_CONF 0x04 /* 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_REC 0x5c /* address of last recoverable error */
> +#define ASPEED_MCR_LAST ASPEED_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 + 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:
> + case ASPEED_MCR_ADDR_UNREC:
> + case ASPEED_MCR_ADDR_REC:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +
> +static const struct regmap_config aspeed_edac_regmap_config = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> + .max_register = ASPEED_MCR_LAST,
> + .reg_write = aspeed_edac_regmap_reg_write,
> + .reg_read = aspeed_edac_regmap_reg_read,
> + .volatile_reg = aspeed_edac_regmap_is_volatile,
> + .fast_io = true,
> +};
> +
> +
> +static struct regmap *aspeed_edac_regmap;
Put that definition at the top of the file, under the #defines.
> +
> +
> +static void aspeed_edac_count_rec(struct mem_ctl_info *mci,
> + u8 rec_cnt,
> + u32 rec_addr)
> +{
> + struct csrow_info *csrow = mci->csrows[0];
> + u32 page, offset, syndrome;
> +
> + if (rec_cnt > 0) {
Save an indentation level:
if (!rec_cnt)
return;
> + /* report first few errors (if there are) */
> + /* note: no addresses are recorded */
> + if (rec_cnt > 1) {
> + page = 0; /* not available */
> + offset = 0; /* not available */
> + syndrome = 0; /* not available */
Do a single comment, over the lines, not sideways. Ditto for the rest of
the driver - side comments make the code harder to read.
> + edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
> + rec_cnt-1, page, offset,
> + syndrome, 0, 0, -1,
> + "address(es) not available", "");
> + }
> +
> + /* report last error */
> + /* note: rec_addr is the last recoverable error addr */
> + page = rec_addr >> PAGE_SHIFT;
> + offset = rec_addr & ~PAGE_MASK;
> + syndrome = 0; /* not available */
> + edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, 1,
> + csrow->first_page + page, offset, syndrome,
> + 0, 0, -1, "", "");
> + }
> +}
> +
> +
> +static void aspeed_edac_count_un_rec(struct mem_ctl_info *mci,
> + u8 un_rec_cnt,
> + u32 un_rec_addr)
> +{
> + struct csrow_info *csrow = mci->csrows[0];
> + u32 page, offset, syndrome;
> +
> + if (un_rec_cnt > 0) {
As above: save an indentation level by flipping the check.
> + /* report 1. error */
> + /* note: un_rec_addr is the first unrecoverable error addr */
> + page = un_rec_addr >> PAGE_SHIFT;
> + offset = un_rec_addr & ~PAGE_MASK;
> + syndrome = 0; /* not available */
> + edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, 1,
> + csrow->first_page + page, offset, syndrome,
> + 0, 0, -1, "", "");
> +
> + /* report further errors (if there are) */
> + /* note: no addresses are recorded */
> + if (un_rec_cnt > 1) {
> + page = 0; /* not available */
> + offset = 0; /* not available */
> + syndrome = 0; /* not available */
> + edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
> + un_rec_cnt-1, page, offset,
> + syndrome, 0, 0, -1,
> + "address(es) not available", "");
> + }
> + }
> +}
> +
> +
> +static void aspeed_edac_enable_interrupts(void)
> +{
> +
> + regmap_update_bits(aspeed_edac_regmap, ASPEED_MCR_INTR_CTRL,
> + ASPEED_MCR_INTR_CTRL_ENABLE,
> + ASPEED_MCR_INTR_CTRL_ENABLE);
> +}
A function which has a single statement and is called only once, looks
a bit useless to me. Just put the regmap_update_bits() call at the call
site with a comment (which you already have).
> +
> +
> +static void aspeed_edac_disable_interrupts(void)
> +{
> + regmap_update_bits(aspeed_edac_regmap, ASPEED_MCR_INTR_CTRL,
> + ASPEED_MCR_INTR_CTRL_ENABLE, 0);
> +}
Ditto.
> +
> +
> +static void aspeed_edac_clear_interrupts(void)
> +{
> + regmap_update_bits(aspeed_edac_regmap, ASPEED_MCR_INTR_CTRL,
> + ASPEED_MCR_INTR_CTRL_CLEAR,
> + ASPEED_MCR_INTR_CTRL_CLEAR);
> +
> + regmap_update_bits(aspeed_edac_regmap, ASPEED_MCR_INTR_CTRL,
> + ASPEED_MCR_INTR_CTRL_CLEAR, 0);
> +}
Ditto.
> +
> +
> +static irqreturn_t aspeed_edac_isr(int irq, void *arg)
> +{
> + u8 rec_cnt, un_rec_cnt;
> + u32 rec_addr, un_rec_addr;
> + struct mem_ctl_info *mci = arg;
> + u32 reg50, reg5c, reg58;
> +
> + regmap_read(aspeed_edac_regmap, ASPEED_MCR_INTR_CTRL, ®50);
> + dev_dbg(mci->pdev, "received edac interrupt w/ mmc register 50: 0x%x\n",
> + reg50);
> +
> + /* collect data about recoverable and unrecoverable errors */
> + rec_cnt = (reg50 & ASPEED_MCR_INTR_CTRL_CNT_REC) >> 16;
> + un_rec_cnt = (reg50 & ASPEED_MCR_INTR_CTRL_CNT_UNREC) >> 12;
> +
> + dev_dbg(mci->pdev, "%d recoverable interrupts and %d unrecoverable interrupts\n",
> + rec_cnt, un_rec_cnt);
> +
> + regmap_read(aspeed_edac_regmap, ASPEED_MCR_ADDR_UNREC, ®58);
> + un_rec_addr = reg58 >> 4;
> +
> + regmap_read(aspeed_edac_regmap, ASPEED_MCR_ADDR_REC, ®5c);
> + rec_addr = reg5c >> 4;
> +
> + /* clear interrupt flags and error counters: */
> + aspeed_edac_clear_interrupts();
> +
> + /* process recoverable and unrecoverable errors */
> + if (rec_cnt > 0)
You check rec_cnt here *and* in the function? One can never be sure huh? :-)
> + aspeed_edac_count_rec(mci, rec_cnt, rec_addr);
> +
> + if (un_rec_cnt > 0)
> + aspeed_edac_count_un_rec(mci, un_rec_cnt, un_rec_addr);
Ditto. Just do the checks in the respective functions.
> +
> + if ((rec_cnt == 0) && (un_rec_cnt == 0))
Do
if (!rec_cnt && !un_rec_cnt)
> + dev_dbg(mci->pdev, "received edac interrupt, but did not find any ecc counters\n");
s/ecc/ECC/g
> +
> + regmap_read(aspeed_edac_regmap, ASPEED_MCR_INTR_CTRL, ®50);
> + dev_dbg(mci->pdev, "edac interrupt handled. mmc reg 50 is now: 0x%x\n",
> + reg50);
> +
> + return IRQ_HANDLED;
> +}
> +
> +
> +static int aspeed_edac_config_irq(void *ctx,
> + struct platform_device *pdev)
Let that line stick out.
> +{
> + int irq;
> + int rc;
> +
> + /* register interrupt handler */
> +
> + irq = platform_get_irq(pdev, 0);
> + dev_dbg(&pdev->dev, "got irq %d\n", irq);
> + if (!irq)
> + return -ENODEV;
> +
> + rc = devm_request_irq(&pdev->dev, irq, aspeed_edac_isr,
> + IRQF_TRIGGER_HIGH, DRV_NAME, ctx);
> + if (rc) {
> + dev_err(&pdev->dev, "unable to request irq %d\n", irq);
> + return rc;
> + }
> +
> + /* enable interrupts */
> + aspeed_edac_enable_interrupts();
> +
> + return 0;
> +}
> +
> +
> +static int aspeed_edac_init_csrows(struct mem_ctl_info *mci)
> +{
> + struct csrow_info *csrow = mci->csrows[0];
> + struct dimm_info *dimm;
> + struct device_node *np;
> + u32 nr_pages, dram_type;
> + struct resource r;
> + u32 reg04;
> + int rc;
> +
> + /* retrieve info about physical memory from device tree */
> + np = of_find_node_by_path("/memory");
> +
Superfluous new line.
> + if (!np) {
> + dev_err(mci->pdev, "dt: missing /memory node\n");
> + return -ENODEV;
> + };
> +
> + rc = of_address_to_resource(np, 0, &r);
> +
> + of_node_put(np);
> +
> + if (rc) {
> + dev_err(mci->pdev, "dt: failed requesting resource for /memory node\n");
> + return rc;
> + };
> +
> + dev_dbg(mci->pdev, "dt: /memory node resources: first page r.start=0x%x, resource_size=0x%x, PAGE_SHIFT macro=0x%x\n",
> + r.start, resource_size(&r), PAGE_SHIFT);
> +
> + csrow->first_page = r.start >> PAGE_SHIFT;
> + nr_pages = resource_size(&r) >> PAGE_SHIFT;
> + csrow->last_page = csrow->first_page + nr_pages - 1;
> +
> + regmap_read(aspeed_edac_regmap, ASPEED_MCR_CONF, ®04);
> + dram_type = (reg04 & ASPEED_MCR_CONF_DRAM_TYPE) ? MEM_DDR4 : MEM_DDR3;
> +
> + dimm = csrow->channels[0]->dimm;
> + dimm->mtype = dram_type;
> + dimm->edac_mode = EDAC_SECDED;
> + dimm->nr_pages = nr_pages / csrow->nr_channels;
> +
> + dev_dbg(mci->pdev, "initialized dimm with first_page=0x%lx and nr_pages=0x%x\n",
> + csrow->first_page, nr_pages);
> +
> + return 0;
> +}
> +
> +
> +static int aspeed_edac_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + void __iomem *regs;
> + struct resource *res;
> + struct mem_ctl_info *mci;
> + struct edac_mc_layer layers[2];
> + struct device_node *np;
> + u32 reg04;
> + int rc;
Please sort function local variables declaration in a reverse christmas
tree order:
<type A> longest_variable_name;
<type B> shorter_var_name;
<type C> even_shorter;
<type D> i;
Do that in all functions.
> +
> + /* setup regmap */
> + np = dev->of_node;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENOENT;
> +
> + regs = devm_ioremap_resource(dev, res);
> + if (IS_ERR(regs))
> + return PTR_ERR(regs);
> +
> + aspeed_edac_regmap = devm_regmap_init(dev, NULL, (__force void *)regs,
> + &aspeed_edac_regmap_config);
> + if (IS_ERR(aspeed_edac_regmap))
> + return PTR_ERR(aspeed_edac_regmap);
> +
> + /* bail out if ECC mode is not configured */
> + regmap_read(aspeed_edac_regmap, ASPEED_MCR_CONF, ®04);
> + if (!(reg04 & ASPEED_MCR_CONF_ECC)) {
> + dev_err(&pdev->dev, "ECC mode is not configured in u-boot\n");
> + return -EPERM;
> + }
> +
> + edac_op_state = EDAC_OPSTATE_INT;
> +
> + /* allocate & init EDAC MC data structure */
> + layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
> + layers[0].size = 1;
> + layers[0].is_virt_csrow = true;
> + layers[1].type = EDAC_MC_LAYER_CHANNEL;
> + layers[1].size = 1;
> + layers[1].is_virt_csrow = false;
> +
> + mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, 0);
> + if (mci == NULL)
if (!mci)
> + return -ENOMEM;
> +
> + mci->pdev = &pdev->dev;
> + mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR4;
> + mci->edac_ctl_cap = EDAC_FLAG_SECDED;
> + mci->edac_cap = EDAC_FLAG_SECDED;
> + mci->scrub_cap = SCRUB_FLAG_HW_SRC;
> + mci->scrub_mode = SCRUB_HW_SRC;
> + mci->mod_name = DRV_NAME;
> + mci->ctl_name = "MIC";
> + mci->dev_name = dev_name(&pdev->dev);
> +
> + rc = aspeed_edac_init_csrows(mci);
> + if (rc) {
> + dev_err(&pdev->dev, "failed to init csrows\n");
> + goto probe_exit02;
> + }
> +
> + platform_set_drvdata(pdev, mci);
> +
> + /* register with edac core */
> + rc = edac_mc_add_mc(mci);
> + if (rc) {
> + dev_err(&pdev->dev, "failed to register with EDAC core\n");
> + goto probe_exit02;
> + }
> +
> + /* register interrupt handler and enable interrupts */
> + rc = aspeed_edac_config_irq(mci, pdev);
> + if (rc) {
> + dev_err(&pdev->dev, "failed setting up irq\n");
> + goto probe_exit01;
> + }
> +
> + return 0;
> +
> +probe_exit01:
> + edac_mc_del_mc(&pdev->dev);
> +probe_exit02:
> + edac_mc_free(mci);
> + return rc;
> +}
...
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply
* [PATCH v3 3/4] ARM: dts: dps650ab: add power supply
From: Xiaoting Liu @ 2019-01-10 2:29 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <cover.1546918172.git.xiaoting.liu@hxt-semitech.com>
Add Delta Electronics power supply DPS-650-AB.
Signed-off-by: Xiaoting Liu <xiaoting.liu@hxt-semitech.com>
---
v3:
Modify patch subject and commit message.
Add delta to compatible property.
---
v2:
No change.
---
arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts b/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts
index bdfd8c9f3a7c..521afbea2c5b 100644
--- a/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts
@@ -173,6 +173,16 @@
};
};
};
+
+ dps650ab at 58 {
+ compatible = "delta,dps650ab";
+ reg = <0x58>;
+ };
+
+ dps650ab at 59 {
+ compatible = "delta,dps650ab";
+ reg = <0x59>;
+ };
};
&i2c9 {
--
1.8.3.1
^ permalink raw reply related
* phosphor-ipmi-host.service broken
From: Vijay Khemka @ 2019-01-09 18:42 UTC (permalink / raw)
To: linux-aspeed
I see new changes in this phosphor-ipmi-host.service file where it needs org.openbmc.HostIpmi.service service to be running and this org.openbmc.HostIpmi.service service depends on phosphor-ipmi-host.service file. So it is cyclic dependency which in turn not letting run any of these service.
Regards
-Vijay
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20190109/36a4d397/attachment.html>
^ permalink raw reply
* [Potential Spoof] Re: [PATCH v2 4/4] ARM: dts: aspeed: Add lpc ctrl for Facebook
From: Vijay Khemka @ 2019-01-07 19:18 UTC (permalink / raw)
To: linux-aspeed
Please merge these patches in upstream kernel.
Regards
-Vijay
?On 12/20/18, 10:06 AM, "Linux-aspeed on behalf of Vijay Khemka" <linux-aspeed-bounces+vijaykhemka=fb.com at lists.ozlabs.org on behalf of vijaykhemka@fb.com> wrote:
Joel, Can you please take care of these patches merge.
On 12/17/18, 12:04 PM, "Vijay Khemka" <vijaykhemka@fb.com> wrote:
Added lpc ctrl device to enable LPC clock in Facebook
Tiogapass device tree.
Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
.../boot/dts/aspeed-bmc-facebook-tiogapass.dts | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index 73e58a821613..ef7875b54562 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -22,6 +22,17 @@
reg = <0x80000000 0x20000000>;
};
+ reserved-memory {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ flash_memory: region at 98000000 {
+ no-map;
+ reg = <0x98000000 0x00001000>; /* 4K */
+ };
+ };
+
iio-hwmon {
compatible = "iio-hwmon";
io-channels = <&adc 0>, <&adc 1>, <&adc 2>, <&adc 3>,
@@ -54,6 +65,12 @@
};
};
+&lpc_ctrl {
+ status = "okay";
+ memory-region = <&flash_memory>;
+ flash = <&spi1>;
+};
+
&uart1 {
// Host Console
status = "okay";
--
2.17.1
^ permalink raw reply
* [PATCH v2 3/4] dts: dps650ab: add dps650ab device tree
From: Xiaoting Liu @ 2019-01-07 11:05 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <cover.1546851856.git.xiaoting.liu@hxt-semitech.com>
Add dps650ab device tree to support power supply dps650ab driver
Signed-off-by: Xiaoting Liu <xiaoting.liu@hxt-semitech.com>
---
arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts b/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts
index bdfd8c9f3a7c..e7e59da6c9ae 100644
--- a/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts
@@ -173,6 +173,16 @@
};
};
};
+
+ dps650ab at 58 {
+ compatible = "dps650ab";
+ reg = <0x58>;
+ };
+
+ dps650ab at 59 {
+ compatible = "dps650ab";
+ reg = <0x59>;
+ };
};
&i2c9 {
--
1.8.3.1
This email is intended only for the named addressee. It may contain information that is confidential/private, legally privileged, or copyright-protected, and you should handle it accordingly. If you are not the intended recipient, you do not have legal rights to retain, copy, or distribute this email or its contents, and should promptly delete the email and all electronic copies in your system; do not retain copies in any media. If you have received this email in error, please notify the sender promptly. Thank you.
^ permalink raw reply related
* [PATCH 3/4] dts: dps650ab: add dps650ab device tree
From: Xiaoting Liu @ 2019-01-04 9:03 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <cover.1546591275.git.xiaoting.liu@hxt-semitech.com>
Add dps650ab device tree to support power supply dps650ab driver.
Signed-off-by: Xiaoting Liu <xiaoting.liu@hxt-semitech.com>
---
arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts b/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts
index bdfd8c9f3a7c..e7e59da6c9ae 100644
--- a/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-arm-stardragon4800-rep2.dts
@@ -173,6 +173,16 @@
};
};
};
+
+ dps650ab at 58 {
+ compatible = "dps650ab";
+ reg = <0x58>;
+ };
+
+ dps650ab at 59 {
+ compatible = "dps650ab";
+ reg = <0x59>;
+ };
};
&i2c9 {
--
1.8.3.1
This email is intended only for the named addressee. It may contain information that is confidential/private, legally privileged, or copyright-protected, and you should handle it accordingly. If you are not the intended recipient, you do not have legal rights to retain, copy, or distribute this email or its contents, and should promptly delete the email and all electronic copies in your system; do not retain copies in any media. If you have received this email in error, please notify the sender promptly. Thank you.
^ permalink raw reply related
* [PATCH 1/2] EDAC: Add Aspeed AST2500 EDAC driver
From: Boris Petkov @ 2018-12-31 13:53 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20181231132000.3894E6E85603@corona.crabdance.com>
On December 31, 2018 3:20:00 PM GMT+02:00, Stefan Schaeckeler <schaecsn@gmx.net> 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.
^ permalink raw reply
* [PATCH 1/2] EDAC: Add Aspeed AST2500 EDAC driver
From: Stefan Schaeckeler @ 2018-12-31 13:20 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1545026517-64069-2-git-send-email-schaecsn@gmx.net>
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 <schaecsn@gmx.net>
>
> From: Stefan M Schaeckeler <sschaeck@cisco.com>
>
> Add support for the Aspeed AST2500 SoC EDAC driver.
>
> Signed-off-by: Stefan M Schaeckeler <sschaeck@cisco.com>
> ---
> 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-edac at vger.kernel.org
> S: Maintained
> F: drivers/edac/amd64_edac*
>
> +EDAC-AST2500
> +M: Stefan Schaeckeler <sschaeck@cisco.com>
> +S: Supported
> +F: drivers/edac/aspeed_edac.c
> +F: Documentation/devicetree/bindings/edac/aspeed-sdram-edac.txt
> +
> EDAC-CALXEDA
> M: Robert Richter <rric@kernel.org>
> L: linux-edac at 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 = <0x80000000 0>;
> };
>
> + edac: sdram at 1e6e0000 {
> + compatible = "aspeed,ast2500-sdram-edac";
> + reg = <0x1e6e0000 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 000000000000..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 <linux/edac.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/stop_machine.h>
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +#include <linux/regmap.h>
> +#include <asm/page.h>
Including asm/page.h does not seem to be necessary.
> +#include "edac_module.h"
> +
> +
> +#define DRV_NAME "aspeed-edac"
> +
> +
> +/* registers */
> +#define ASPEED_MCR_PROT 0x00 /* protection key register */
> +#define ASPEED_MCR_CONF 0x04 /* 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_REC 0x5c /* address of last recoverable error */
> +#define ASPEED_MCR_LAST ASPEED_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:
> + case ASPEED_MCR_ADDR_UNREC:
> + case ASPEED_MCR_ADDR_REC:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +
> +static const struct regmap_config aspeed_edac_regmap_config = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> + .max_register = ASPEED_MCR_LAST,
> + .reg_write = aspeed_edac_regmap_reg_write,
> + .reg_read = aspeed_edac_regmap_reg_read,
> + .volatile_reg = aspeed_edac_regmap_is_volatile,
> + .fast_io = true,
> +};
> +
> +
> +static struct regmap *aspeed_edac_regmap;
> +
> +
> +static void aspeed_edac_count_rec(struct mem_ctl_info *mci,
> + u8 rec_cnt,
> + u32 rec_addr)
> +{
> + struct csrow_info *csrow = mci->csrows[0];
> + u32 page, offset, syndrome;
> +
> + if (rec_cnt > 0) {
> + /* report first few errors (if there are) */
> + /* note: no addresses are recorded */
> + if (rec_cnt > 1) {
> + page = 0; /* not available */
> + offset = 0; /* not available */
> + syndrome = 0; /* not available */
> + edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
> + rec_cnt-1, page, offset,
> + syndrome, 0, 0, -1,
> + "address(es) not available", "");
> + }
> +
> + /* report last error */
> + /* note: rec_addr is the last recoverable error addr */
> + page = rec_addr >> PAGE_SHIFT;
> + offset = rec_addr & ~PAGE_MASK;
> + syndrome = 0; /* not available */
> + edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, 1,
> + csrow->first_page + page, offset, syndrome,
> + 0, 0, -1, "", "");
> + }
> +}
> +
> +
> +static void aspeed_edac_count_un_rec(struct mem_ctl_info *mci,
> + u8 un_rec_cnt,
> + u32 un_rec_addr)
> +{
> + struct csrow_info *csrow = mci->csrows[0];
> + u32 page, offset, syndrome;
> +
> + if (un_rec_cnt > 0) {
> + /* report 1. error */
> + /* note: un_rec_addr is the first unrecoverable error addr */
> + page = un_rec_addr >> PAGE_SHIFT;
> + offset = un_rec_addr & ~PAGE_MASK;
> + syndrome = 0; /* not available */
> + edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, 1,
> + csrow->first_page + page, offset, syndrome,
> + 0, 0, -1, "", "");
> +
> + /* report further errors (if there are) */
> + /* note: no addresses are recorded */
> + if (un_rec_cnt > 1) {
> + page = 0; /* not available */
> + offset = 0; /* not available */
> + syndrome = 0; /* not available */
> + edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
> + un_rec_cnt-1, page, offset,
> + syndrome, 0, 0, -1,
> + "address(es) not available", "");
> + }
> + }
> +}
> +
> +
> +static void aspeed_edac_enable_interrupts(void)
> +{
> +
> + regmap_update_bits(aspeed_edac_regmap, ASPEED_MCR_INTR_CTRL,
> + ASPEED_MCR_INTR_CTRL_ENABLE,
> + ASPEED_MCR_INTR_CTRL_ENABLE);
> +}
> +
> +
> +static void aspeed_edac_disable_interrupts(void)
> +{
> + regmap_update_bits(aspeed_edac_regmap, ASPEED_MCR_INTR_CTRL,
> + ASPEED_MCR_INTR_CTRL_ENABLE, 0);
> +}
> +
> +
> +static void aspeed_edac_clear_interrupts(void)
> +{
> + regmap_update_bits(aspeed_edac_regmap, ASPEED_MCR_INTR_CTRL,
> + ASPEED_MCR_INTR_CTRL_CLEAR,
> + ASPEED_MCR_INTR_CTRL_CLEAR);
> +
> + regmap_update_bits(aspeed_edac_regmap, ASPEED_MCR_INTR_CTRL,
> + ASPEED_MCR_INTR_CTRL_CLEAR, 0);
> +}
> +
> +
> +static irqreturn_t aspeed_edac_isr(int irq, void *arg)
> +{
> + u8 rec_cnt, un_rec_cnt;
> + u32 rec_addr, un_rec_addr;
> + struct mem_ctl_info *mci = arg;
> + u32 reg50, reg5c, reg58;
> +
> + regmap_read(aspeed_edac_regmap, ASPEED_MCR_INTR_CTRL, ®50);
> + dev_dbg(mci->pdev, "received edac interrupt w/ mmc register 50: 0x%x\n",
> + reg50);
> +
> + /* collect data about recoverable and unrecoverable errors */
> + rec_cnt = (reg50 & ASPEED_MCR_INTR_CTRL_CNT_REC) >> 16;
> + un_rec_cnt = (reg50 & ASPEED_MCR_INTR_CTRL_CNT_UNREC) >> 12;
> +
> + dev_dbg(mci->pdev, "%d recoverable interrupts and %d unrecoverable interrupts\n",
> + rec_cnt, un_rec_cnt);
> +
> + regmap_read(aspeed_edac_regmap, ASPEED_MCR_ADDR_UNREC, ®58);
> + un_rec_addr = reg58 >> 4;
Now I better understand reg58. This is from aspeed docs:
- - - snip - - -
31:30 reserved (0)
29:4 Address of first un-recoverable ECC error
3:0 reserved (0)
- - - snip - - -
Bits 3:0 are indeed part of the address, but their values are not known and so
simply hardcoded to 0000. This is because there is one ECC byte for every 8
bytes and so the precision is 8-byte.
In other words, the whole register holds the address of the first
un-recoverable ECC error and so the shift must be removed:
un_rec_addr = reg58;
Sanity check: an address space of 29-4+1 bits would be quite small. The correct
address space is 30 bits.
> + regmap_read(aspeed_edac_regmap, ASPEED_MCR_ADDR_REC, ®5c);
> + rec_addr = reg5c >> 4;
Same as above.
> + /* clear interrupt flags and error counters: */
> + aspeed_edac_clear_interrupts();
> +
> + /* process recoverable and unrecoverable errors */
> + if (rec_cnt > 0)
> + aspeed_edac_count_rec(mci, rec_cnt, rec_addr);
> +
> + if (un_rec_cnt > 0)
> + aspeed_edac_count_un_rec(mci, un_rec_cnt, un_rec_addr);
> +
> + if ((rec_cnt == 0) && (un_rec_cnt == 0))
> + dev_dbg(mci->pdev, "received edac interrupt, but did not find any ecc counters\n");
> +
> + regmap_read(aspeed_edac_regmap, ASPEED_MCR_INTR_CTRL, ®50);
> + dev_dbg(mci->pdev, "edac interrupt handled. mmc reg 50 is now: 0x%x\n",
> + reg50);
> +
> + return IRQ_HANDLED;
> +}
> +
> +
> +static int aspeed_edac_config_irq(void *ctx,
> + struct platform_device *pdev)
> +{
> + int irq;
> + int rc;
> +
> + /* register interrupt handler */
> +
> + irq = platform_get_irq(pdev, 0);
> + dev_dbg(&pdev->dev, "got irq %d\n", irq);
> + if (!irq)
> + return -ENODEV;
> +
> + rc = devm_request_irq(&pdev->dev, irq, aspeed_edac_isr,
> + IRQF_TRIGGER_HIGH, DRV_NAME, ctx);
> + if (rc) {
> + dev_err(&pdev->dev, "unable to request irq %d\n", irq);
> + return rc;
> + }
> +
> + /* enable interrupts */
> + aspeed_edac_enable_interrupts();
> +
> + return 0;
> +}
> +
> +
> +static int aspeed_edac_init_csrows(struct mem_ctl_info *mci)
> +{
> + struct csrow_info *csrow = mci->csrows[0];
> + struct dimm_info *dimm;
> + struct device_node *np;
> + u32 nr_pages, dram_type;
> + struct resource r;
> + u32 reg04;
> + int rc;
> +
> + /* retrieve info about physical memory from device tree */
> + np = of_find_node_by_path("/memory");
> +
> + if (!np) {
> + dev_err(mci->pdev, "dt: missing /memory node\n");
> + return -ENODEV;
> + };
> +
> + rc = of_address_to_resource(np, 0, &r);
> +
> + of_node_put(np);
> +
> + if (rc) {
> + dev_err(mci->pdev, "dt: failed requesting resource for /memory node\n");
> + return rc;
> + };
> +
> + dev_dbg(mci->pdev, "dt: /memory node resources: first page r.start=0x%x, resource_size=0x%x, PAGE_SHIFT macro=0x%x\n",
> + r.start, resource_size(&r), PAGE_SHIFT);
> +
> + csrow->first_page = r.start >> PAGE_SHIFT;
> + nr_pages = resource_size(&r) >> PAGE_SHIFT;
> + csrow->last_page = csrow->first_page + nr_pages - 1;
> +
> + regmap_read(aspeed_edac_regmap, ASPEED_MCR_CONF, ®04);
> + dram_type = (reg04 & ASPEED_MCR_CONF_DRAM_TYPE) ? MEM_DDR4 : MEM_DDR3;
> +
> + dimm = csrow->channels[0]->dimm;
> + dimm->mtype = dram_type;
> + dimm->edac_mode = EDAC_SECDED;
> + dimm->nr_pages = nr_pages / csrow->nr_channels;
> +
> + dev_dbg(mci->pdev, "initialized dimm with first_page=0x%lx and nr_pages=0x%x\n",
> + csrow->first_page, nr_pages);
> +
> + return 0;
> +}
> +
> +
> +static int aspeed_edac_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + void __iomem *regs;
> + struct resource *res;
> + struct mem_ctl_info *mci;
> + struct edac_mc_layer layers[2];
> + struct device_node *np;
> + u32 reg04;
> + int rc;
> +
> + /* setup regmap */
> + np = dev->of_node;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENOENT;
> +
> + regs = devm_ioremap_resource(dev, res);
> + if (IS_ERR(regs))
> + return PTR_ERR(regs);
> +
> + aspeed_edac_regmap = devm_regmap_init(dev, NULL, (__force void *)regs,
> + &aspeed_edac_regmap_config);
> + if (IS_ERR(aspeed_edac_regmap))
> + return PTR_ERR(aspeed_edac_regmap);
> +
> + /* bail out if ECC mode is not configured */
> + regmap_read(aspeed_edac_regmap, ASPEED_MCR_CONF, ®04);
> + if (!(reg04 & ASPEED_MCR_CONF_ECC)) {
> + dev_err(&pdev->dev, "ECC mode is not configured in u-boot\n");
> + return -EPERM;
> + }
> +
> + edac_op_state = EDAC_OPSTATE_INT;
> +
> + /* allocate & init EDAC MC data structure */
> + layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
> + layers[0].size = 1;
> + layers[0].is_virt_csrow = true;
> + layers[1].type = EDAC_MC_LAYER_CHANNEL;
> + layers[1].size = 1;
> + layers[1].is_virt_csrow = false;
> +
> + mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, 0);
> + if (mci == NULL)
> + return -ENOMEM;
> +
> + mci->pdev = &pdev->dev;
> + mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR4;
> + mci->edac_ctl_cap = EDAC_FLAG_SECDED;
> + mci->edac_cap = EDAC_FLAG_SECDED;
> + mci->scrub_cap = SCRUB_FLAG_HW_SRC;
> + mci->scrub_mode = SCRUB_HW_SRC;
> + mci->mod_name = DRV_NAME;
> + mci->ctl_name = "MIC";
> + mci->dev_name = dev_name(&pdev->dev);
> +
> + rc = aspeed_edac_init_csrows(mci);
> + if (rc) {
> + dev_err(&pdev->dev, "failed to init csrows\n");
> + goto probe_exit02;
> + }
> +
> + platform_set_drvdata(pdev, mci);
> +
> + /* register with edac core */
> + rc = edac_mc_add_mc(mci);
> + if (rc) {
> + dev_err(&pdev->dev, "failed to register with EDAC core\n");
> + goto probe_exit02;
> + }
> +
> + /* register interrupt handler and enable interrupts */
> + rc = aspeed_edac_config_irq(mci, pdev);
> + if (rc) {
> + dev_err(&pdev->dev, "failed setting up irq\n");
> + goto probe_exit01;
> + }
> +
> + return 0;
> +
> +probe_exit01:
> + edac_mc_del_mc(&pdev->dev);
> +probe_exit02:
> + edac_mc_free(mci);
> + return rc;
> +}
> +
> +
> +static int aspeed_edac_remove(struct platform_device *pdev)
> +{
> + struct mem_ctl_info *mci;
> +
> + /* disable interrupts */
> + aspeed_edac_disable_interrupts();
> +
> + /* free resources */
> + mci = edac_mc_del_mc(&pdev->dev);
> + if (mci)
> + edac_mc_free(mci);
> +
> + return 0;
> +}
> +
> +
> +static const struct of_device_id aspeed_edac_of_match[] = {
> + { .compatible = "aspeed,ast2500-sdram-edac" },
> + {},
> +};
> +
> +
> +static struct platform_driver aspeed_edac_driver = {
> + .driver = {
> + .name = DRV_NAME,
> + .of_match_table = aspeed_edac_of_match
> + },
> + .probe = aspeed_edac_probe,
> + .remove = aspeed_edac_remove
> +};
> +
> +
> +static int __init aspeed_edac_init(void)
> +{
> + return platform_driver_register(&aspeed_edac_driver);
> +}
> +
> +
> +static void __exit aspeed_edac_exit(void)
> +{
> + platform_driver_unregister(&aspeed_edac_driver);
> +}
> +
> +
> +module_init(aspeed_edac_init);
> +module_exit(aspeed_edac_exit);
> +
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Stefan Schaeckeler <sschaeck@cisco.com>");
> +MODULE_DESCRIPTION("Aspeed AST2500 EDAC driver");
> +MODULE_VERSION("1.0");
> --
> 2.19.1
>
Stefan
^ permalink raw reply
* [PATCH 2/2] dt-bindings: edac: Aspeed AST2500
From: schaecsn @ 2018-12-29 18:30 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20181227220906.GA14320@bogus>
Hello Rob,
> From: Rob Herring <robh@kernel.org>
>
> On Sun, Dec 16, 2018 at 10:01:57PM -0800, Stefan Schaeckeler wrote:
> > From: Stefan M Schaeckeler <sschaeck@cisco.com>
> >
> > Add support for the Aspeed AST2500 SoC EDAC driver.
> >
> > Signed-off-by: Stefan M Schaeckeler <sschaeck@cisco.com>
> > ---
> > .../bindings/edac/aspeed-sdram-edac.txt | 34 +++++++++++++++++++
> > 1 file changed, 34 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/edac/aspeed-sdram-edac.txt
> >
> > diff --git a/Documentation/devicetree/bindings/edac/aspeed-sdram-edac.txt b/Documentation/devicetree/bindings/edac/aspeed-sdram-edac.txt
> > new file mode 100644
> > index 000000000000..57ba852883c7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/edac/aspeed-sdram-edac.txt
> > @@ -0,0 +1,34 @@
> > +Aspeed AST2500 SoC EDAC device driver
>
> Bindings are for h/w, not drivers
Changed "device driver" to "node".
I will also change the commit message to perhaps "Add support for EDAC on the
Aspeed AST2500 SoC."
> > +The Aspeed AST2500 SoC supports DDR3 and DDR4 memory with and without ECC (error
> > +correction check).
> > +
> > +The memory controller supports SECDED (single bit error correction, double bit
> > +error detection) and single bit error auto scrubbing by reserving 8 bits for
> > +every 64 bit word (effectively reducing available memory to 8/9).
> > +
> > +First, ECC must be configured in u-boot. Then, this driver will expose error
> > +counters via the edac kernel framework.
>
> Please reword this to not be u-boot or kernel specific.
The previous paragraph is now: Note, the bootloader must configure ECC mode in
the memory controller.
> Maybe this node is enabled in the bootloader or the OS can just read the
> registers to see if ECC is enabled. The latter is more future proof if
> you need to access the DDR ctrl registers for other reasons.
The driver's probe function has a sanity check. It consults the memory
controller for enabled ECC mode:
/* bail out if ECC mode is not configured */
regmap_read(aspeed_edac_regmap, ASPEED_MCR_CONF, ®04);
if (!(reg04 & ASPEED_MCR_CONF_ECC)) {
dev_err(&pdev->dev, "ECC mode is not configured in u-boot\n");
return -EPERM;
}
> > +A note on memory organization in ECC mode: every 512 bytes are followed by 64
> > +bytes of ECC codes.
>
> That sounds strange. Normally, the memory would be 72-bits wide to hold
> the ECC byte for each 64-bit chunk. It would be inefficient to access
> the ECC byte in a discontiguous location.
When a word is loaded from memory, the corresponding ECC word needs to be
loaded as well (both words can and will be cached). Performance relies on
temporal and spatial locality. That can fire back, of course.
> In any case, none of this is really important for the binding.
I will move above and below paragraph into Kconfig.
> > The address remapping is done in hardware and is fully
> > +transparent to firmware and software. Because of this, ECC mode must be
> > +configured in u-boot as part of the memory initialization as one can not switch
> > +from one mode to another when executing in memory.
> > +
> > +
> > +
> > +Required properties:
> > +- compatible: should be "aspeed,ast2500-sdram-edac"
> > +- reg: sdram controller register set should be <0x1e6e0000 0x174>
> > +- interrupts: should be AVIC interrupt #0
> > +
> > +
> > +Example:
> > +
> > + edac: sdram at 1e6e0000 {
> > + compatible = "aspeed,ast2500-sdram-edac";
> > + reg = <0x1e6e0000 0x174>;
> > + interrupts = <0>;
> > + status = "okay";
>
> Don't show status in examples.
Removed.
>
> > + };
> > --
> > 2.19.1
To wrap it up, for the next patchset, I will generate a diff for below text
- - - snip - - -
Aspeed AST2500 SoC EDAC node
The Aspeed AST2500 SoC supports DDR3 and DDR4 memory with and without ECC (error
correction check).
The memory controller supports SECDED (single bit error correction, double bit
error detection) and single bit error auto scrubbing by reserving 8 bits for
every 64 bit word (effectively reducing available memory to 8/9).
Note, the bootloader must configure ECC mode in the memory controller.
Required properties:
- compatible: should be "aspeed,ast2500-sdram-edac"
- reg: sdram controller register set should be <0x1e6e0000 0x174>
- interrupts: should be AVIC interrupt #0
Example:
edac: sdram at 1e6e0000 {
compatible = "aspeed,ast2500-sdram-edac";
reg = <0x1e6e0000 0x174>;
interrupts = <0>;
};
- - - snip - - -
Stefan
^ permalink raw reply
* [PATCH 2/2] dt-bindings: edac: Aspeed AST2500
From: Rob Herring @ 2018-12-27 22:09 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1545026517-64069-3-git-send-email-schaecsn@gmx.net>
On Sun, Dec 16, 2018 at 10:01:57PM -0800, Stefan Schaeckeler wrote:
> From: Stefan M Schaeckeler <sschaeck@cisco.com>
>
> Add support for the Aspeed AST2500 SoC EDAC driver.
>
> Signed-off-by: Stefan M Schaeckeler <sschaeck@cisco.com>
> ---
> .../bindings/edac/aspeed-sdram-edac.txt | 34 +++++++++++++++++++
> 1 file changed, 34 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/edac/aspeed-sdram-edac.txt
>
> diff --git a/Documentation/devicetree/bindings/edac/aspeed-sdram-edac.txt b/Documentation/devicetree/bindings/edac/aspeed-sdram-edac.txt
> new file mode 100644
> index 000000000000..57ba852883c7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/edac/aspeed-sdram-edac.txt
> @@ -0,0 +1,34 @@
> +Aspeed AST2500 SoC EDAC device driver
Bindings are for h/w, not drivers
> +
> +The Aspeed AST2500 SoC supports DDR3 and DDR4 memory with and without ECC (error
> +correction check).
> +
> +The memory controller supports SECDED (single bit error correction, double bit
> +error detection) and single bit error auto scrubbing by reserving 8 bits for
> +every 64 bit word (effectively reducing available memory to 8/9).
> +
> +First, ECC must be configured in u-boot. Then, this driver will expose error
> +counters via the edac kernel framework.
Please reword this to not be u-boot or kernel specific.
Maybe this node is enabled in the bootloader or the OS can just read the
registers to see if ECC is enabled. The latter is more future proof if
you need to access the DDR ctrl registers for other reasons.
> +
> +A note on memory organization in ECC mode: every 512 bytes are followed by 64
> +bytes of ECC codes.
That sounds strange. Normally, the memory would be 72-bits wide to hold
the ECC byte for each 64-bit chunk. It would be inefficient to access
the ECC byte in a discontiguous location. In any case, none of this is
really important for the binding.
> The address remapping is done in hardware and is fully
> +transparent to firmware and software. Because of this, ECC mode must be
> +configured in u-boot as part of the memory initialization as one can not switch
> +from one mode to another when executing in memory.
> +
> +
> +
> +Required properties:
> +- compatible: should be "aspeed,ast2500-sdram-edac"
> +- reg: sdram controller register set should be <0x1e6e0000 0x174>
> +- interrupts: should be AVIC interrupt #0
> +
> +
> +Example:
> +
> + edac: sdram at 1e6e0000 {
> + compatible = "aspeed,ast2500-sdram-edac";
> + reg = <0x1e6e0000 0x174>;
> + interrupts = <0>;
> + status = "okay";
Don't show status in examples.
> + };
> --
> 2.19.1
>
^ permalink raw reply
* [PATCH] gpio: aspeed: remove duplicated statement
From: Linus Walleij @ 2018-12-21 10:14 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20181212215305.3852170-1-taoren@fb.com>
On Wed, Dec 12, 2018 at 10:53 PM Tao Ren <taoren@fb.com> wrote:
> Remove duplicated assignment statement from aspeed_gpio_probe() function.
>
> Signed-off-by: Tao Ren <taoren@fb.com>
Patch applied with Joel's ACK.
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH v2 4/4] ARM: dts: aspeed: Add lpc ctrl for Facebook
From: Vijay Khemka @ 2018-12-20 18:05 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20181217200405.2904551-4-vijaykhemka@fb.com>
Joel, Can you please take care of these patches merge.
?On 12/17/18, 12:04 PM, "Vijay Khemka" <vijaykhemka@fb.com> wrote:
Added lpc ctrl device to enable LPC clock in Facebook
Tiogapass device tree.
Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
.../boot/dts/aspeed-bmc-facebook-tiogapass.dts | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index 73e58a821613..ef7875b54562 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -22,6 +22,17 @@
reg = <0x80000000 0x20000000>;
};
+ reserved-memory {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ flash_memory: region at 98000000 {
+ no-map;
+ reg = <0x98000000 0x00001000>; /* 4K */
+ };
+ };
+
iio-hwmon {
compatible = "iio-hwmon";
io-channels = <&adc 0>, <&adc 1>, <&adc 2>, <&adc 3>,
@@ -54,6 +65,12 @@
};
};
+&lpc_ctrl {
+ status = "okay";
+ memory-region = <&flash_memory>;
+ flash = <&spi1>;
+};
+
&uart1 {
// Host Console
status = "okay";
--
2.17.1
^ permalink raw reply
* [PATCH 0/7] v4.19-stable randconfig fixes
From: Sasha Levin @ 2018-12-18 16:20 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20181218151230.GA30844@kroah.com>
On Tue, Dec 18, 2018 at 04:12:30PM +0100, Greg Kroah-Hartman wrote:
>On Mon, Dec 17, 2018 at 07:20:28PM -0500, Sasha Levin wrote:
>> On Fri, Dec 14, 2018 at 11:10:05PM +0100, Arnd Bergmann wrote:
>> > Hi Greg,
>> >
>> > I did some randconfig testing on linux-4.19 arm/arm64/x86. So far I needed
>> > 27 patches, most of which are also still needed in mainline Linux. I
>> > had submitted some before, and others were not submitted previously
>> > for some reason. I'll try to get those fixed in mainline and then
>> > make sure we get them into 4.19 as well.
>> >
>> > This series for now contains four patches that did make it into mainline:
>> >
>> > 2e6ae11dd0d1 ("slimbus: ngd: mark PM functions as __maybe_unused")
>> > 33f49571d750 ("staging: olpc_dcon: add a missing dependency")
>> > 0eeec01488da ("scsi: raid_attrs: fix unused variable warning")
>> > 11d4afd4ff66 ("sched/pelt: Fix warning and clean up IRQ PELT config")
>> >
>> > Feel free to either cherry-pick those from mainline or apply the
>> > patch from this series, whichever works best for you.
>> >
>> > The other three patches are for warnings in code that got removed in
>> > mainline kernels:
>> >
>> > 3e9efc3299dd ("i2c: aspeed: Handle master/slave combined irq events properly")
>> > 972910948fb6 ("ARM: dts: qcom: Remove Arrow SD600 eval board")
>> > effec874792f ("drm/msm/dpu: Remove dpu_dbg")
>> >
>> > My feeling was that it's safer to just address the warning by fixing
>> > the code correctly in each of these cases, but if you disagree,
>> > applying the mainline change should work equally well, so decide
>> > for yourself.
>>
>> Thanks Arnd, I took the series as is.
>>
>> We really need to discuss how -stable deals with removed code upstream.
>> For some cases, we should probably follow suit and remove it from
>> -stable as well (I'm mostly thinking dodgy code with potential security
>> issues).
>
>It would be nice to do that at times (like lustre and ipx), but it's
>good to keep that code around as maybe someone is using it? I don't
>know, it's a tough call...
Yeah, it's really a case-by-case basis. I'm really concerned about an
unmaintained piece of code in stable kernels, no one actually fixes bug
in it.
With the example here where that eval board was removed because no one
was using it is a great example of code we should be removing from
stable trees as well. If no one is using it - great! but if someone
does, then the removal should be reverted upstream as well.
--
Thanks,
Sasha
^ permalink raw reply
* [PATCH 0/7] v4.19-stable randconfig fixes
From: Greg Kroah-Hartman @ 2018-12-18 15:12 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20181218002028.GX2746@sasha-vm>
On Mon, Dec 17, 2018 at 07:20:28PM -0500, Sasha Levin wrote:
> On Fri, Dec 14, 2018 at 11:10:05PM +0100, Arnd Bergmann wrote:
> > Hi Greg,
> >
> > I did some randconfig testing on linux-4.19 arm/arm64/x86. So far I needed
> > 27 patches, most of which are also still needed in mainline Linux. I
> > had submitted some before, and others were not submitted previously
> > for some reason. I'll try to get those fixed in mainline and then
> > make sure we get them into 4.19 as well.
> >
> > This series for now contains four patches that did make it into mainline:
> >
> > 2e6ae11dd0d1 ("slimbus: ngd: mark PM functions as __maybe_unused")
> > 33f49571d750 ("staging: olpc_dcon: add a missing dependency")
> > 0eeec01488da ("scsi: raid_attrs: fix unused variable warning")
> > 11d4afd4ff66 ("sched/pelt: Fix warning and clean up IRQ PELT config")
> >
> > Feel free to either cherry-pick those from mainline or apply the
> > patch from this series, whichever works best for you.
> >
> > The other three patches are for warnings in code that got removed in
> > mainline kernels:
> >
> > 3e9efc3299dd ("i2c: aspeed: Handle master/slave combined irq events properly")
> > 972910948fb6 ("ARM: dts: qcom: Remove Arrow SD600 eval board")
> > effec874792f ("drm/msm/dpu: Remove dpu_dbg")
> >
> > My feeling was that it's safer to just address the warning by fixing
> > the code correctly in each of these cases, but if you disagree,
> > applying the mainline change should work equally well, so decide
> > for yourself.
>
> Thanks Arnd, I took the series as is.
>
> We really need to discuss how -stable deals with removed code upstream.
> For some cases, we should probably follow suit and remove it from
> -stable as well (I'm mostly thinking dodgy code with potential security
> issues).
It would be nice to do that at times (like lustre and ipx), but it's
good to keep that code around as maybe someone is using it? I don't
know, it's a tough call...
thanks,
greg k-h
^ permalink raw reply
* [PATCH 0/7] v4.19-stable randconfig fixes
From: Sasha Levin @ 2018-12-18 0:20 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20181214221023.3878670-1-arnd@arndb.de>
On Fri, Dec 14, 2018 at 11:10:05PM +0100, Arnd Bergmann wrote:
>Hi Greg,
>
>I did some randconfig testing on linux-4.19 arm/arm64/x86. So far I needed
>27 patches, most of which are also still needed in mainline Linux. I
>had submitted some before, and others were not submitted previously
>for some reason. I'll try to get those fixed in mainline and then
>make sure we get them into 4.19 as well.
>
>This series for now contains four patches that did make it into mainline:
>
>2e6ae11dd0d1 ("slimbus: ngd: mark PM functions as __maybe_unused")
>33f49571d750 ("staging: olpc_dcon: add a missing dependency")
>0eeec01488da ("scsi: raid_attrs: fix unused variable warning")
>11d4afd4ff66 ("sched/pelt: Fix warning and clean up IRQ PELT config")
>
>Feel free to either cherry-pick those from mainline or apply the
>patch from this series, whichever works best for you.
>
>The other three patches are for warnings in code that got removed in
>mainline kernels:
>
>3e9efc3299dd ("i2c: aspeed: Handle master/slave combined irq events properly")
>972910948fb6 ("ARM: dts: qcom: Remove Arrow SD600 eval board")
>effec874792f ("drm/msm/dpu: Remove dpu_dbg")
>
>My feeling was that it's safer to just address the warning by fixing
>the code correctly in each of these cases, but if you disagree,
>applying the mainline change should work equally well, so decide
>for yourself.
Thanks Arnd, I took the series as is.
We really need to discuss how -stable deals with removed code upstream.
For some cases, we should probably follow suit and remove it from
-stable as well (I'm mostly thinking dodgy code with potential security
issues).
--
Thanks,
Sasha
^ permalink raw reply
* [PATCH v2 4/4] ARM: dts: aspeed: Add lpc ctrl for Facebook
From: Vijay Khemka @ 2018-12-17 20:04 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20181217200405.2904551-1-vijaykhemka@fb.com>
Added lpc ctrl device to enable LPC clock in Facebook
Tiogapass device tree.
Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
.../boot/dts/aspeed-bmc-facebook-tiogapass.dts | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index 73e58a821613..ef7875b54562 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -22,6 +22,17 @@
reg = <0x80000000 0x20000000>;
};
+ reserved-memory {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ flash_memory: region at 98000000 {
+ no-map;
+ reg = <0x98000000 0x00001000>; /* 4K */
+ };
+ };
+
iio-hwmon {
compatible = "iio-hwmon";
io-channels = <&adc 0>, <&adc 1>, <&adc 2>, <&adc 3>,
@@ -54,6 +65,12 @@
};
};
+&lpc_ctrl {
+ status = "okay";
+ memory-region = <&flash_memory>;
+ flash = <&spi1>;
+};
+
&uart1 {
// Host Console
status = "okay";
--
2.17.1
^ permalink raw reply related
* [PATCH v2 3/4] ARM: dts: aspeed: Add KCS for Facebook
From: Vijay Khemka @ 2018-12-17 20:04 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20181217200405.2904551-1-vijaykhemka@fb.com>
Added kcs device in Facebook Tiogapass device tree.
Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index 64039dddd853..73e58a821613 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -67,6 +67,18 @@
status = "okay";
};
+&kcs2 {
+ // BMC KCS channel 2
+ status = "okay";
+ kcs_addr = <0xca8>;
+};
+
+&kcs3 {
+ // BMC KCS channel 3
+ status = "okay";
+ kcs_addr = <0xca2>;
+};
+
&mac0 {
status = "okay";
--
2.17.1
^ permalink raw reply related
* [PATCH v2 2/4] ARM: dts: aspeed: Add KCS support for LPC BMC
From: Vijay Khemka @ 2018-12-17 20:04 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20181217200405.2904551-1-vijaykhemka@fb.com>
Added kcs device support for lpc BMC.
Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
arch/arm/boot/dts/aspeed-g5.dtsi | 33 +++++++++++++++++++++++++++++++-
1 file changed, 32 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index d107459fc0f8..2743f400aa29 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -330,8 +330,32 @@
ranges = <0x0 0x1e789000 0x1000>;
lpc_bmc: lpc-bmc at 0 {
- compatible = "aspeed,ast2500-lpc-bmc";
+ compatible = "aspeed,ast2500-lpc-bmc", "simple-mfd", "syscon";
reg = <0x0 0x80>;
+ reg-io-width = <4>;
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x0 0x80>;
+
+ kcs1: kcs1 at 0 {
+ compatible = "aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <1>;
+ status = "disabled";
+ };
+ kcs2: kcs2 at 0 {
+ compatible = "aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <2>;
+ status = "disabled";
+ };
+ kcs3: kcs3 at 0 {
+ compatible = "aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <3>;
+ status = "disabled";
+ };
};
lpc_host: lpc-host at 80 {
@@ -343,6 +367,13 @@
#size-cells = <1>;
ranges = <0x0 0x80 0x1e0>;
+ kcs4: kcs4 at 0 {
+ compatible = "aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <4>;
+ status = "disabled";
+ };
+
lpc_ctrl: lpc-ctrl at 0 {
compatible = "aspeed,ast2500-lpc-ctrl";
reg = <0x0 0x80>;
--
2.17.1
^ permalink raw reply related
* [PATCH v2 1/4] ARM: dts: aspeed: Add sensors devices for Facebook
From: Vijay Khemka @ 2018-12-17 20:04 UTC (permalink / raw)
To: linux-aspeed
Added ADC and other sensor devices in Facebook Tiogapass device tree.
Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
.../dts/aspeed-bmc-facebook-tiogapass.dts | 23 +++++++++++++++++--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index f8e7b71af7e6..64039dddd853 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -21,6 +21,17 @@
memory at 80000000 {
reg = <0x80000000 0x20000000>;
};
+
+ iio-hwmon {
+ compatible = "iio-hwmon";
+ io-channels = <&adc 0>, <&adc 1>, <&adc 2>, <&adc 3>,
+ <&adc 4>, <&adc 5>, <&adc 6>;
+ };
+
+ iio-hwmon-battery {
+ compatible = "iio-hwmon";
+ io-channels = <&adc 7>;
+ };
};
&fmc {
@@ -64,6 +75,10 @@
use-ncsi;
};
+&adc {
+ status = "okay";
+};
+
&i2c0 {
status = "okay";
//Airmax Conn B, CPU0 PIROM, CPU1 PIROM
@@ -122,6 +137,10 @@
&i2c8 {
status = "okay";
+ tmp421 at 1f {
+ compatible = "ti,tmp421";
+ reg = <0x1f>;
+ };
//Mezz Sensor SMBus
};
@@ -140,7 +159,7 @@
};
fan at 1 {
- reg = <0x00>;
- aspeed,fan-tach-ch = /bits/ 8 <0x01>;
+ reg = <0x01>;
+ aspeed,fan-tach-ch = /bits/ 8 <0x02>;
};
};
--
2.17.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox