* [PATCH v2 0/3] Amazon's Annapurna Labs POS Driver @ 2019-09-10 19:05 ` Talel Shenhar 0 siblings, 0 replies; 26+ messages in thread From: Talel Shenhar @ 2019-09-10 19:05 UTC (permalink / raw) To: robh+dt, marc.zyngier, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, talel, linux-kernel, devicetree The Amazon's Annapurna Labs SoCs includes Point Of Serialization error logging unit that reports an error in case of write error (e.g. attempt to write to a read only register). This patch series introduces the support for this unit. Changes since v1: ================= - move MODULE_ to the end of the file - simplified resource remapping devm_platform_ioremap_resource() - use platform_get_irq() instead of irq_of_parse_and_map() - removed the use of _relaxed accessor in favor to the regular ones - removed driver selected based on arch - added casting to u64 before left shifting (reported by kbuild test robot) Talel Shenhar (3): dt-bindings: soc: al-pos: Amazon's Annapurna Labs POS soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver soc: amazon: al-pos: cast to u64 before left shifting .../bindings/soc/amazon/amazon,al-pos.txt | 18 +++ MAINTAINERS | 7 ++ drivers/soc/Kconfig | 1 + drivers/soc/Makefile | 1 + drivers/soc/amazon/Kconfig | 5 + drivers/soc/amazon/Makefile | 1 + drivers/soc/amazon/al_pos.c | 127 +++++++++++++++++++++ 7 files changed, 160 insertions(+) create mode 100644 Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt create mode 100644 drivers/soc/amazon/Kconfig create mode 100644 drivers/soc/amazon/Makefile create mode 100644 drivers/soc/amazon/al_pos.c -- 2.7.4 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 0/3] Amazon's Annapurna Labs POS Driver @ 2019-09-10 19:05 ` Talel Shenhar 0 siblings, 0 replies; 26+ messages in thread From: Talel Shenhar @ 2019-09-10 19:05 UTC (permalink / raw) To: robh+dt, marc.zyngier, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, talel, linux-kernel, devicetree The Amazon's Annapurna Labs SoCs includes Point Of Serialization error logging unit that reports an error in case of write error (e.g. attempt to write to a read only register). This patch series introduces the support for this unit. Changes since v1: ================= - move MODULE_ to the end of the file - simplified resource remapping devm_platform_ioremap_resource() - use platform_get_irq() instead of irq_of_parse_and_map() - removed the use of _relaxed accessor in favor to the regular ones - removed driver selected based on arch - added casting to u64 before left shifting (reported by kbuild test robot) Talel Shenhar (3): dt-bindings: soc: al-pos: Amazon's Annapurna Labs POS soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver soc: amazon: al-pos: cast to u64 before left shifting .../bindings/soc/amazon/amazon,al-pos.txt | 18 +++ MAINTAINERS | 7 ++ drivers/soc/Kconfig | 1 + drivers/soc/Makefile | 1 + drivers/soc/amazon/Kconfig | 5 + drivers/soc/amazon/Makefile | 1 + drivers/soc/amazon/al_pos.c | 127 +++++++++++++++++++++ 7 files changed, 160 insertions(+) create mode 100644 Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt create mode 100644 drivers/soc/amazon/Kconfig create mode 100644 drivers/soc/amazon/Makefile create mode 100644 drivers/soc/amazon/al_pos.c -- 2.7.4 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 1/3] dt-bindings: soc: al-pos: Amazon's Annapurna Labs POS 2019-09-10 19:05 ` Talel Shenhar @ 2019-09-10 19:05 ` Talel Shenhar -1 siblings, 0 replies; 26+ messages in thread From: Talel Shenhar @ 2019-09-10 19:05 UTC (permalink / raw) To: robh+dt, marc.zyngier, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, talel, linux-kernel, devicetree Document Amazon's Annapurna Labs POS SoC binding. Signed-off-by: Talel Shenhar <talel@amazon.com> --- .../devicetree/bindings/soc/amazon/amazon,al-pos.txt | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt diff --git a/Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt b/Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt new file mode 100644 index 00000000..035cc571 --- /dev/null +++ b/Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt @@ -0,0 +1,18 @@ +Amazon's Annapurna Labs POS + +POS node is defined to describe the Point Of Serialization (POS) logger +unit. + +Required properties: +- compatible: Shall be "amazon,al-pos". +- reg: POS logger resources. +- interrupts: should contain the interrupt for pos error event. + +Example: + +al_pos { + compatible = "amazon,al-pos"; + reg = <0x0 0xf0070084 0x0 0x00000008>; + interrupt-parent = <&amazon_system_fabric>; + interrupts = <24 IRQ_TYPE_LEVEL_HIGH>; +}; -- 2.7.4 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 1/3] dt-bindings: soc: al-pos: Amazon's Annapurna Labs POS @ 2019-09-10 19:05 ` Talel Shenhar 0 siblings, 0 replies; 26+ messages in thread From: Talel Shenhar @ 2019-09-10 19:05 UTC (permalink / raw) To: robh+dt, marc.zyngier, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, talel, linux-kernel, devicetree Document Amazon's Annapurna Labs POS SoC binding. Signed-off-by: Talel Shenhar <talel@amazon.com> --- .../devicetree/bindings/soc/amazon/amazon,al-pos.txt | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt diff --git a/Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt b/Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt new file mode 100644 index 00000000..035cc571 --- /dev/null +++ b/Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt @@ -0,0 +1,18 @@ +Amazon's Annapurna Labs POS + +POS node is defined to describe the Point Of Serialization (POS) logger +unit. + +Required properties: +- compatible: Shall be "amazon,al-pos". +- reg: POS logger resources. +- interrupts: should contain the interrupt for pos error event. + +Example: + +al_pos { + compatible = "amazon,al-pos"; + reg = <0x0 0xf0070084 0x0 0x00000008>; + interrupt-parent = <&amazon_system_fabric>; + interrupts = <24 IRQ_TYPE_LEVEL_HIGH>; +}; -- 2.7.4 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: soc: al-pos: Amazon's Annapurna Labs POS 2019-09-10 19:05 ` Talel Shenhar (?) @ 2019-09-18 13:32 ` Rob Herring 2019-09-18 13:44 ` Shenhar, Talel -1 siblings, 1 reply; 26+ messages in thread From: Rob Herring @ 2019-09-18 13:32 UTC (permalink / raw) To: Talel Shenhar Cc: marc.zyngier, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, linux-kernel, devicetree On Tue, Sep 10, 2019 at 10:05:08PM +0300, Talel Shenhar wrote: > Document Amazon's Annapurna Labs POS SoC binding. > > Signed-off-by: Talel Shenhar <talel@amazon.com> > --- > .../devicetree/bindings/soc/amazon/amazon,al-pos.txt | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > create mode 100644 Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt Please convert to DT schema. > > diff --git a/Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt b/Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt > new file mode 100644 > index 00000000..035cc571 > --- /dev/null > +++ b/Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt > @@ -0,0 +1,18 @@ > +Amazon's Annapurna Labs POS > + > +POS node is defined to describe the Point Of Serialization (POS) logger > +unit. > + > +Required properties: > +- compatible: Shall be "amazon,al-pos". > +- reg: POS logger resources. > +- interrupts: should contain the interrupt for pos error event. > + > +Example: > + > +al_pos { Needs a unit-address. > + compatible = "amazon,al-pos"; > + reg = <0x0 0xf0070084 0x0 0x00000008>; > + interrupt-parent = <&amazon_system_fabric>; > + interrupts = <24 IRQ_TYPE_LEVEL_HIGH>; > +}; > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: soc: al-pos: Amazon's Annapurna Labs POS 2019-09-18 13:32 ` Rob Herring @ 2019-09-18 13:44 ` Shenhar, Talel 0 siblings, 0 replies; 26+ messages in thread From: Shenhar, Talel @ 2019-09-18 13:44 UTC (permalink / raw) To: Rob Herring Cc: marc.zyngier, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, linux-kernel, devicetree Tnx Rob for the review. Shall be part of v3. Waiting for responses from Arnd and James and will publish v3. On 9/18/2019 4:35 PM, Rob Herring wrote: > On Tue, Sep 10, 2019 at 10:05:08PM +0300, Talel Shenhar wrote: >> Document Amazon's Annapurna Labs POS SoC binding. >> >> Signed-off-by: Talel Shenhar <talel@amazon.com> >> --- >> .../devicetree/bindings/soc/amazon/amazon,al-pos.txt | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt > Please convert to DT schema. > >> diff --git a/Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt b/Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt >> new file mode 100644 >> index 00000000..035cc571 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt >> @@ -0,0 +1,18 @@ >> +Amazon's Annapurna Labs POS >> + >> +POS node is defined to describe the Point Of Serialization (POS) logger >> +unit. >> + >> +Required properties: >> +- compatible: Shall be "amazon,al-pos". >> +- reg: POS logger resources. >> +- interrupts: should contain the interrupt for pos error event. >> + >> +Example: >> + >> +al_pos { > Needs a unit-address. > >> + compatible = "amazon,al-pos"; >> + reg = <0x0 0xf0070084 0x0 0x00000008>; >> + interrupt-parent = <&amazon_system_fabric>; >> + interrupts = <24 IRQ_TYPE_LEVEL_HIGH>; >> +}; >> -- >> 2.7.4 >> ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: soc: al-pos: Amazon's Annapurna Labs POS @ 2019-09-18 13:44 ` Shenhar, Talel 0 siblings, 0 replies; 26+ messages in thread From: Shenhar, Talel @ 2019-09-18 13:44 UTC (permalink / raw) To: Rob Herring Cc: marc.zyngier, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, linux-kernel, devicetree Tnx Rob for the review. Shall be part of v3. Waiting for responses from Arnd and James and will publish v3. On 9/18/2019 4:35 PM, Rob Herring wrote: > On Tue, Sep 10, 2019 at 10:05:08PM +0300, Talel Shenhar wrote: >> Document Amazon's Annapurna Labs POS SoC binding. >> >> Signed-off-by: Talel Shenhar <talel@amazon.com> >> --- >> .../devicetree/bindings/soc/amazon/amazon,al-pos.txt | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt > Please convert to DT schema. > >> diff --git a/Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt b/Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt >> new file mode 100644 >> index 00000000..035cc571 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt >> @@ -0,0 +1,18 @@ >> +Amazon's Annapurna Labs POS >> + >> +POS node is defined to describe the Point Of Serialization (POS) logger >> +unit. >> + >> +Required properties: >> +- compatible: Shall be "amazon,al-pos". >> +- reg: POS logger resources. >> +- interrupts: should contain the interrupt for pos error event. >> + >> +Example: >> + >> +al_pos { > Needs a unit-address. > >> + compatible = "amazon,al-pos"; >> + reg = <0x0 0xf0070084 0x0 0x00000008>; >> + interrupt-parent = <&amazon_system_fabric>; >> + interrupts = <24 IRQ_TYPE_LEVEL_HIGH>; >> +}; >> -- >> 2.7.4 >> ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver 2019-09-10 19:05 ` Talel Shenhar @ 2019-09-10 19:05 ` Talel Shenhar -1 siblings, 0 replies; 26+ messages in thread From: Talel Shenhar @ 2019-09-10 19:05 UTC (permalink / raw) To: robh+dt, marc.zyngier, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, talel, linux-kernel, devicetree The Amazon's Annapurna Labs SoCs includes Point Of Serialization error logging unit that reports an error in case write error (e.g. attempt to write to a read only register). This patch introduces the support for this unit. Signed-off-by: Talel Shenhar <talel@amazon.com> --- MAINTAINERS | 7 +++ drivers/soc/Kconfig | 1 + drivers/soc/Makefile | 1 + drivers/soc/amazon/Kconfig | 5 ++ drivers/soc/amazon/Makefile | 1 + drivers/soc/amazon/al_pos.c | 127 ++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 142 insertions(+) create mode 100644 drivers/soc/amazon/Kconfig create mode 100644 drivers/soc/amazon/Makefile create mode 100644 drivers/soc/amazon/al_pos.c diff --git a/MAINTAINERS b/MAINTAINERS index e7a47b5..8c3a070 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -751,6 +751,13 @@ F: drivers/tty/serial/altera_jtaguart.c F: include/linux/altera_uart.h F: include/linux/altera_jtaguart.h +AMAZON ANNAPURNA LABS POS +M: Talel Shenhar <talel@amazon.com> +M: Talel Shenhar <talelshenhar@gmail.com> +S: Maintained +F: Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt +F: drivers/soc/amazon/al_pos.c + AMAZON ANNAPURNA LABS THERMAL MMIO DRIVER M: Talel Shenhar <talel@amazon.com> S: Maintained diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig index 833e04a..913a6b1 100644 --- a/drivers/soc/Kconfig +++ b/drivers/soc/Kconfig @@ -2,6 +2,7 @@ menu "SOC (System On Chip) specific Drivers" source "drivers/soc/actions/Kconfig" +source "drivers/soc/amazon/Kconfig" source "drivers/soc/amlogic/Kconfig" source "drivers/soc/aspeed/Kconfig" source "drivers/soc/atmel/Kconfig" diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile index 2ec3550..c1c5c64 100644 --- a/drivers/soc/Makefile +++ b/drivers/soc/Makefile @@ -6,6 +6,7 @@ obj-$(CONFIG_ARCH_ACTIONS) += actions/ obj-$(CONFIG_SOC_ASPEED) += aspeed/ obj-$(CONFIG_ARCH_AT91) += atmel/ +obj-y += amazon/ obj-y += bcm/ obj-$(CONFIG_ARCH_DOVE) += dove/ obj-$(CONFIG_MACH_DOVE) += dove/ diff --git a/drivers/soc/amazon/Kconfig b/drivers/soc/amazon/Kconfig new file mode 100644 index 00000000..fdd4cdd --- /dev/null +++ b/drivers/soc/amazon/Kconfig @@ -0,0 +1,5 @@ +config AL_POS + bool "Amazon's Annapurna Labs POS driver" + depends on ARCH_ALPINE || COMPILE_TEST + help + Include support for the SoC POS error capability. diff --git a/drivers/soc/amazon/Makefile b/drivers/soc/amazon/Makefile new file mode 100644 index 00000000..a31441a --- /dev/null +++ b/drivers/soc/amazon/Makefile @@ -0,0 +1 @@ +obj-$(CONFIG_AL_POS) += al_pos.o diff --git a/drivers/soc/amazon/al_pos.c b/drivers/soc/amazon/al_pos.c new file mode 100644 index 00000000..a865111 --- /dev/null +++ b/drivers/soc/amazon/al_pos.c @@ -0,0 +1,127 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. + */ +#include <linux/bitfield.h> +#include <linux/interrupt.h> +#include <linux/module.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/of_platform.h> + +/* Registers Offset */ +#define AL_POS_ERROR_LOG_1 0x0 +#define AL_POS_ERROR_LOG_0 0x4 + +/* Registers Fields */ +#define AL_POS_ERROR_LOG_1_VALID GENMASK(31, 31) +#define AL_POS_ERROR_LOG_1_BRESP GENMASK(18, 17) +#define AL_POS_ERROR_LOG_1_REQUEST_ID GENMASK(16, 8) +#define AL_POS_ERROR_LOG_1_ADDR_HIGH GENMASK(7, 0) + +#define AL_POS_ERROR_LOG_0_ADDR_LOW GENMASK(31, 0) + +static int al_pos_panic; +module_param(al_pos_panic, int, 0); +MODULE_PARM_DESC(al_pos_panic, "Defines if POS error is causing panic()"); + +struct al_pos { + struct platform_device *pdev; + void __iomem *mmio_base; + int irq; +}; + +static irqreturn_t al_pos_irq_handler(int irq, void *info) +{ + struct platform_device *pdev = info; + struct al_pos *pos = platform_get_drvdata(pdev); + u32 log1; + u32 log0; + u64 addr; + u16 request_id; + u8 bresp; + + log1 = readl(pos->mmio_base + AL_POS_ERROR_LOG_1); + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1)) + return IRQ_NONE; + + log0 = readl(pos->mmio_base + AL_POS_ERROR_LOG_0); + writel(0, pos->mmio_base + AL_POS_ERROR_LOG_1); + + addr = FIELD_GET(AL_POS_ERROR_LOG_0_ADDR_LOW, log0); + addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32); + request_id = FIELD_GET(AL_POS_ERROR_LOG_1_REQUEST_ID, log1); + bresp = FIELD_GET(AL_POS_ERROR_LOG_1_BRESP, log1); + + dev_err(&pdev->dev, "addr=0x%llx request_id=0x%x bresp=0x%x\n", + addr, request_id, bresp); + + if (al_pos_panic) + panic("POS"); + + return IRQ_HANDLED; +} + +static int al_pos_probe(struct platform_device *pdev) +{ + struct al_pos *pos; + int ret; + + pos = devm_kzalloc(&pdev->dev, sizeof(*pos), GFP_KERNEL); + if (!pos) + return -ENOMEM; + + platform_set_drvdata(pdev, pos); + pos->pdev = pdev; + + pos->mmio_base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(pos->mmio_base)) { + dev_err(&pdev->dev, "failed to ioremap memory (%ld)\n", + PTR_ERR(pos->mmio_base)); + return PTR_ERR(pos->mmio_base); + } + + pos->irq = platform_get_irq(pdev, 0); + if (pos->irq <= 0) { + dev_err(&pdev->dev, "fail to parse and map irq\n"); + return -EINVAL; + } + + ret = devm_request_irq(&pdev->dev, + pos->irq, + al_pos_irq_handler, + 0, + pdev->name, + pdev); + if (ret != 0) { + dev_err(&pdev->dev, + "failed to register to irq %d (%d)\n", + pos->irq, ret); + return ret; + } + + dev_info(&pdev->dev, "successfully loaded\n"); + + return 0; +} + +static const struct of_device_id al_pos_of_match[] = { + { .compatible = "amazon,al-pos", }, + {}, +}; + +MODULE_DEVICE_TABLE(of, al_pos_of_match); + +static struct platform_driver al_pos_driver = { + .probe = al_pos_probe, + .driver = { + .name = "al-pos", + .of_match_table = al_pos_of_match, + }, +}; + +module_platform_driver(al_pos_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_AUTHOR("Talel Shenhar"); +MODULE_DESCRIPTION("Amazon's Annapurna Labs POS driver"); -- 2.7.4 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver @ 2019-09-10 19:05 ` Talel Shenhar 0 siblings, 0 replies; 26+ messages in thread From: Talel Shenhar @ 2019-09-10 19:05 UTC (permalink / raw) To: robh+dt, marc.zyngier, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, talel, linux-kernel, devicetree The Amazon's Annapurna Labs SoCs includes Point Of Serialization error logging unit that reports an error in case write error (e.g. attempt to write to a read only register). This patch introduces the support for this unit. Signed-off-by: Talel Shenhar <talel@amazon.com> --- MAINTAINERS | 7 +++ drivers/soc/Kconfig | 1 + drivers/soc/Makefile | 1 + drivers/soc/amazon/Kconfig | 5 ++ drivers/soc/amazon/Makefile | 1 + drivers/soc/amazon/al_pos.c | 127 ++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 142 insertions(+) create mode 100644 drivers/soc/amazon/Kconfig create mode 100644 drivers/soc/amazon/Makefile create mode 100644 drivers/soc/amazon/al_pos.c diff --git a/MAINTAINERS b/MAINTAINERS index e7a47b5..8c3a070 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -751,6 +751,13 @@ F: drivers/tty/serial/altera_jtaguart.c F: include/linux/altera_uart.h F: include/linux/altera_jtaguart.h +AMAZON ANNAPURNA LABS POS +M: Talel Shenhar <talel@amazon.com> +M: Talel Shenhar <talelshenhar@gmail.com> +S: Maintained +F: Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt +F: drivers/soc/amazon/al_pos.c + AMAZON ANNAPURNA LABS THERMAL MMIO DRIVER M: Talel Shenhar <talel@amazon.com> S: Maintained diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig index 833e04a..913a6b1 100644 --- a/drivers/soc/Kconfig +++ b/drivers/soc/Kconfig @@ -2,6 +2,7 @@ menu "SOC (System On Chip) specific Drivers" source "drivers/soc/actions/Kconfig" +source "drivers/soc/amazon/Kconfig" source "drivers/soc/amlogic/Kconfig" source "drivers/soc/aspeed/Kconfig" source "drivers/soc/atmel/Kconfig" diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile index 2ec3550..c1c5c64 100644 --- a/drivers/soc/Makefile +++ b/drivers/soc/Makefile @@ -6,6 +6,7 @@ obj-$(CONFIG_ARCH_ACTIONS) += actions/ obj-$(CONFIG_SOC_ASPEED) += aspeed/ obj-$(CONFIG_ARCH_AT91) += atmel/ +obj-y += amazon/ obj-y += bcm/ obj-$(CONFIG_ARCH_DOVE) += dove/ obj-$(CONFIG_MACH_DOVE) += dove/ diff --git a/drivers/soc/amazon/Kconfig b/drivers/soc/amazon/Kconfig new file mode 100644 index 00000000..fdd4cdd --- /dev/null +++ b/drivers/soc/amazon/Kconfig @@ -0,0 +1,5 @@ +config AL_POS + bool "Amazon's Annapurna Labs POS driver" + depends on ARCH_ALPINE || COMPILE_TEST + help + Include support for the SoC POS error capability. diff --git a/drivers/soc/amazon/Makefile b/drivers/soc/amazon/Makefile new file mode 100644 index 00000000..a31441a --- /dev/null +++ b/drivers/soc/amazon/Makefile @@ -0,0 +1 @@ +obj-$(CONFIG_AL_POS) += al_pos.o diff --git a/drivers/soc/amazon/al_pos.c b/drivers/soc/amazon/al_pos.c new file mode 100644 index 00000000..a865111 --- /dev/null +++ b/drivers/soc/amazon/al_pos.c @@ -0,0 +1,127 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. + */ +#include <linux/bitfield.h> +#include <linux/interrupt.h> +#include <linux/module.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/of_platform.h> + +/* Registers Offset */ +#define AL_POS_ERROR_LOG_1 0x0 +#define AL_POS_ERROR_LOG_0 0x4 + +/* Registers Fields */ +#define AL_POS_ERROR_LOG_1_VALID GENMASK(31, 31) +#define AL_POS_ERROR_LOG_1_BRESP GENMASK(18, 17) +#define AL_POS_ERROR_LOG_1_REQUEST_ID GENMASK(16, 8) +#define AL_POS_ERROR_LOG_1_ADDR_HIGH GENMASK(7, 0) + +#define AL_POS_ERROR_LOG_0_ADDR_LOW GENMASK(31, 0) + +static int al_pos_panic; +module_param(al_pos_panic, int, 0); +MODULE_PARM_DESC(al_pos_panic, "Defines if POS error is causing panic()"); + +struct al_pos { + struct platform_device *pdev; + void __iomem *mmio_base; + int irq; +}; + +static irqreturn_t al_pos_irq_handler(int irq, void *info) +{ + struct platform_device *pdev = info; + struct al_pos *pos = platform_get_drvdata(pdev); + u32 log1; + u32 log0; + u64 addr; + u16 request_id; + u8 bresp; + + log1 = readl(pos->mmio_base + AL_POS_ERROR_LOG_1); + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1)) + return IRQ_NONE; + + log0 = readl(pos->mmio_base + AL_POS_ERROR_LOG_0); + writel(0, pos->mmio_base + AL_POS_ERROR_LOG_1); + + addr = FIELD_GET(AL_POS_ERROR_LOG_0_ADDR_LOW, log0); + addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32); + request_id = FIELD_GET(AL_POS_ERROR_LOG_1_REQUEST_ID, log1); + bresp = FIELD_GET(AL_POS_ERROR_LOG_1_BRESP, log1); + + dev_err(&pdev->dev, "addr=0x%llx request_id=0x%x bresp=0x%x\n", + addr, request_id, bresp); + + if (al_pos_panic) + panic("POS"); + + return IRQ_HANDLED; +} + +static int al_pos_probe(struct platform_device *pdev) +{ + struct al_pos *pos; + int ret; + + pos = devm_kzalloc(&pdev->dev, sizeof(*pos), GFP_KERNEL); + if (!pos) + return -ENOMEM; + + platform_set_drvdata(pdev, pos); + pos->pdev = pdev; + + pos->mmio_base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(pos->mmio_base)) { + dev_err(&pdev->dev, "failed to ioremap memory (%ld)\n", + PTR_ERR(pos->mmio_base)); + return PTR_ERR(pos->mmio_base); + } + + pos->irq = platform_get_irq(pdev, 0); + if (pos->irq <= 0) { + dev_err(&pdev->dev, "fail to parse and map irq\n"); + return -EINVAL; + } + + ret = devm_request_irq(&pdev->dev, + pos->irq, + al_pos_irq_handler, + 0, + pdev->name, + pdev); + if (ret != 0) { + dev_err(&pdev->dev, + "failed to register to irq %d (%d)\n", + pos->irq, ret); + return ret; + } + + dev_info(&pdev->dev, "successfully loaded\n"); + + return 0; +} + +static const struct of_device_id al_pos_of_match[] = { + { .compatible = "amazon,al-pos", }, + {}, +}; + +MODULE_DEVICE_TABLE(of, al_pos_of_match); + +static struct platform_driver al_pos_driver = { + .probe = al_pos_probe, + .driver = { + .name = "al-pos", + .of_match_table = al_pos_of_match, + }, +}; + +module_platform_driver(al_pos_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_AUTHOR("Talel Shenhar"); +MODULE_DESCRIPTION("Amazon's Annapurna Labs POS driver"); -- 2.7.4 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver 2019-09-10 19:05 ` Talel Shenhar @ 2019-09-11 14:15 ` Marc Zyngier -1 siblings, 0 replies; 26+ messages in thread From: Marc Zyngier @ 2019-09-11 14:15 UTC (permalink / raw) To: Talel Shenhar Cc: robh+dt, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, linux-kernel, devicetree, James Morse [+James] Hi Talel, On Tue, 10 Sep 2019 20:05:09 +0100, Talel Shenhar <talel@amazon.com> wrote: > > The Amazon's Annapurna Labs SoCs includes Point Of Serialization error > logging unit that reports an error in case write error (e.g. attempt to > write to a read only register). > This patch introduces the support for this unit. > > Signed-off-by: Talel Shenhar <talel@amazon.com> > --- > MAINTAINERS | 7 +++ > drivers/soc/Kconfig | 1 + > drivers/soc/Makefile | 1 + > drivers/soc/amazon/Kconfig | 5 ++ > drivers/soc/amazon/Makefile | 1 + > drivers/soc/amazon/al_pos.c | 127 ++++++++++++++++++++++++++++++++++++++++++++ > 6 files changed, 142 insertions(+) > create mode 100644 drivers/soc/amazon/Kconfig > create mode 100644 drivers/soc/amazon/Makefile > create mode 100644 drivers/soc/amazon/al_pos.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index e7a47b5..8c3a070 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -751,6 +751,13 @@ F: drivers/tty/serial/altera_jtaguart.c > F: include/linux/altera_uart.h > F: include/linux/altera_jtaguart.h > > +AMAZON ANNAPURNA LABS POS > +M: Talel Shenhar <talel@amazon.com> > +M: Talel Shenhar <talelshenhar@gmail.com> > +S: Maintained > +F: Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt > +F: drivers/soc/amazon/al_pos.c > + > AMAZON ANNAPURNA LABS THERMAL MMIO DRIVER > M: Talel Shenhar <talel@amazon.com> > S: Maintained > diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig > index 833e04a..913a6b1 100644 > --- a/drivers/soc/Kconfig > +++ b/drivers/soc/Kconfig > @@ -2,6 +2,7 @@ > menu "SOC (System On Chip) specific Drivers" > > source "drivers/soc/actions/Kconfig" > +source "drivers/soc/amazon/Kconfig" > source "drivers/soc/amlogic/Kconfig" > source "drivers/soc/aspeed/Kconfig" > source "drivers/soc/atmel/Kconfig" > diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile > index 2ec3550..c1c5c64 100644 > --- a/drivers/soc/Makefile > +++ b/drivers/soc/Makefile > @@ -6,6 +6,7 @@ > obj-$(CONFIG_ARCH_ACTIONS) += actions/ > obj-$(CONFIG_SOC_ASPEED) += aspeed/ > obj-$(CONFIG_ARCH_AT91) += atmel/ > +obj-y += amazon/ > obj-y += bcm/ > obj-$(CONFIG_ARCH_DOVE) += dove/ > obj-$(CONFIG_MACH_DOVE) += dove/ > diff --git a/drivers/soc/amazon/Kconfig b/drivers/soc/amazon/Kconfig > new file mode 100644 > index 00000000..fdd4cdd > --- /dev/null > +++ b/drivers/soc/amazon/Kconfig > @@ -0,0 +1,5 @@ > +config AL_POS > + bool "Amazon's Annapurna Labs POS driver" Some would say that the name is ever slightly unfortunate... > + depends on ARCH_ALPINE || COMPILE_TEST > + help > + Include support for the SoC POS error capability. > diff --git a/drivers/soc/amazon/Makefile b/drivers/soc/amazon/Makefile > new file mode 100644 > index 00000000..a31441a > --- /dev/null > +++ b/drivers/soc/amazon/Makefile > @@ -0,0 +1 @@ > +obj-$(CONFIG_AL_POS) += al_pos.o > diff --git a/drivers/soc/amazon/al_pos.c b/drivers/soc/amazon/al_pos.c > new file mode 100644 > index 00000000..a865111 > --- /dev/null > +++ b/drivers/soc/amazon/al_pos.c > @@ -0,0 +1,127 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. > + */ > +#include <linux/bitfield.h> > +#include <linux/interrupt.h> > +#include <linux/module.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > +#include <linux/of_platform.h> > + > +/* Registers Offset */ > +#define AL_POS_ERROR_LOG_1 0x0 > +#define AL_POS_ERROR_LOG_0 0x4 > + > +/* Registers Fields */ > +#define AL_POS_ERROR_LOG_1_VALID GENMASK(31, 31) > +#define AL_POS_ERROR_LOG_1_BRESP GENMASK(18, 17) > +#define AL_POS_ERROR_LOG_1_REQUEST_ID GENMASK(16, 8) > +#define AL_POS_ERROR_LOG_1_ADDR_HIGH GENMASK(7, 0) > + > +#define AL_POS_ERROR_LOG_0_ADDR_LOW GENMASK(31, 0) > + > +static int al_pos_panic; > +module_param(al_pos_panic, int, 0); > +MODULE_PARM_DESC(al_pos_panic, "Defines if POS error is causing panic()"); > + > +struct al_pos { > + struct platform_device *pdev; > + void __iomem *mmio_base; > + int irq; > +}; > + > +static irqreturn_t al_pos_irq_handler(int irq, void *info) > +{ > + struct platform_device *pdev = info; > + struct al_pos *pos = platform_get_drvdata(pdev); > + u32 log1; > + u32 log0; > + u64 addr; > + u16 request_id; > + u8 bresp; > + > + log1 = readl(pos->mmio_base + AL_POS_ERROR_LOG_1); Do you actually need the implied barriers? I'd expect that relaxed accesses should be enough. > + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1)) > + return IRQ_NONE; > + > + log0 = readl(pos->mmio_base + AL_POS_ERROR_LOG_0); > + writel(0, pos->mmio_base + AL_POS_ERROR_LOG_1); > + > + addr = FIELD_GET(AL_POS_ERROR_LOG_0_ADDR_LOW, log0); > + addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32); > + request_id = FIELD_GET(AL_POS_ERROR_LOG_1_REQUEST_ID, log1); > + bresp = FIELD_GET(AL_POS_ERROR_LOG_1_BRESP, log1); > + > + dev_err(&pdev->dev, "addr=0x%llx request_id=0x%x bresp=0x%x\n", > + addr, request_id, bresp); What is this information? How do we make use of it? Given that this is asynchronous, how do we correlate it to the offending software? > + > + if (al_pos_panic) > + panic("POS"); > + > + return IRQ_HANDLED; > +} > + > +static int al_pos_probe(struct platform_device *pdev) > +{ > + struct al_pos *pos; > + int ret; > + > + pos = devm_kzalloc(&pdev->dev, sizeof(*pos), GFP_KERNEL); > + if (!pos) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, pos); > + pos->pdev = pdev; > + > + pos->mmio_base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(pos->mmio_base)) { > + dev_err(&pdev->dev, "failed to ioremap memory (%ld)\n", > + PTR_ERR(pos->mmio_base)); > + return PTR_ERR(pos->mmio_base); > + } > + > + pos->irq = platform_get_irq(pdev, 0); > + if (pos->irq <= 0) { > + dev_err(&pdev->dev, "fail to parse and map irq\n"); > + return -EINVAL; > + } > + > + ret = devm_request_irq(&pdev->dev, > + pos->irq, > + al_pos_irq_handler, > + 0, > + pdev->name, > + pdev); > + if (ret != 0) { > + dev_err(&pdev->dev, > + "failed to register to irq %d (%d)\n", > + pos->irq, ret); > + return ret; > + } > + > + dev_info(&pdev->dev, "successfully loaded\n"); > + > + return 0; > +} > + > +static const struct of_device_id al_pos_of_match[] = { > + { .compatible = "amazon,al-pos", }, > + {}, > +}; > + > +MODULE_DEVICE_TABLE(of, al_pos_of_match); > + > +static struct platform_driver al_pos_driver = { > + .probe = al_pos_probe, > + .driver = { > + .name = "al-pos", > + .of_match_table = al_pos_of_match, > + }, > +}; > + > +module_platform_driver(al_pos_driver); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_AUTHOR("Talel Shenhar"); > +MODULE_DESCRIPTION("Amazon's Annapurna Labs POS driver"); > -- > 2.7.4 > The whole think looks to me like a poor man's EDAC handling, and I'd expect to be plugged in that subsystem instead. Any reason why this isn't the case? It would certainly make the handling uniform for the user. Thanks, M. -- Jazz is not dead, it just smells funny. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver @ 2019-09-11 14:15 ` Marc Zyngier 0 siblings, 0 replies; 26+ messages in thread From: Marc Zyngier @ 2019-09-11 14:15 UTC (permalink / raw) To: Talel Shenhar Cc: robh+dt, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, linux-kernel, devicetree, James Morse [+James] Hi Talel, On Tue, 10 Sep 2019 20:05:09 +0100, Talel Shenhar <talel@amazon.com> wrote: > > The Amazon's Annapurna Labs SoCs includes Point Of Serialization error > logging unit that reports an error in case write error (e.g. attempt to > write to a read only register). > This patch introduces the support for this unit. > > Signed-off-by: Talel Shenhar <talel@amazon.com> > --- > MAINTAINERS | 7 +++ > drivers/soc/Kconfig | 1 + > drivers/soc/Makefile | 1 + > drivers/soc/amazon/Kconfig | 5 ++ > drivers/soc/amazon/Makefile | 1 + > drivers/soc/amazon/al_pos.c | 127 ++++++++++++++++++++++++++++++++++++++++++++ > 6 files changed, 142 insertions(+) > create mode 100644 drivers/soc/amazon/Kconfig > create mode 100644 drivers/soc/amazon/Makefile > create mode 100644 drivers/soc/amazon/al_pos.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index e7a47b5..8c3a070 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -751,6 +751,13 @@ F: drivers/tty/serial/altera_jtaguart.c > F: include/linux/altera_uart.h > F: include/linux/altera_jtaguart.h > > +AMAZON ANNAPURNA LABS POS > +M: Talel Shenhar <talel@amazon.com> > +M: Talel Shenhar <talelshenhar@gmail.com> > +S: Maintained > +F: Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt > +F: drivers/soc/amazon/al_pos.c > + > AMAZON ANNAPURNA LABS THERMAL MMIO DRIVER > M: Talel Shenhar <talel@amazon.com> > S: Maintained > diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig > index 833e04a..913a6b1 100644 > --- a/drivers/soc/Kconfig > +++ b/drivers/soc/Kconfig > @@ -2,6 +2,7 @@ > menu "SOC (System On Chip) specific Drivers" > > source "drivers/soc/actions/Kconfig" > +source "drivers/soc/amazon/Kconfig" > source "drivers/soc/amlogic/Kconfig" > source "drivers/soc/aspeed/Kconfig" > source "drivers/soc/atmel/Kconfig" > diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile > index 2ec3550..c1c5c64 100644 > --- a/drivers/soc/Makefile > +++ b/drivers/soc/Makefile > @@ -6,6 +6,7 @@ > obj-$(CONFIG_ARCH_ACTIONS) += actions/ > obj-$(CONFIG_SOC_ASPEED) += aspeed/ > obj-$(CONFIG_ARCH_AT91) += atmel/ > +obj-y += amazon/ > obj-y += bcm/ > obj-$(CONFIG_ARCH_DOVE) += dove/ > obj-$(CONFIG_MACH_DOVE) += dove/ > diff --git a/drivers/soc/amazon/Kconfig b/drivers/soc/amazon/Kconfig > new file mode 100644 > index 00000000..fdd4cdd > --- /dev/null > +++ b/drivers/soc/amazon/Kconfig > @@ -0,0 +1,5 @@ > +config AL_POS > + bool "Amazon's Annapurna Labs POS driver" Some would say that the name is ever slightly unfortunate... > + depends on ARCH_ALPINE || COMPILE_TEST > + help > + Include support for the SoC POS error capability. > diff --git a/drivers/soc/amazon/Makefile b/drivers/soc/amazon/Makefile > new file mode 100644 > index 00000000..a31441a > --- /dev/null > +++ b/drivers/soc/amazon/Makefile > @@ -0,0 +1 @@ > +obj-$(CONFIG_AL_POS) += al_pos.o > diff --git a/drivers/soc/amazon/al_pos.c b/drivers/soc/amazon/al_pos.c > new file mode 100644 > index 00000000..a865111 > --- /dev/null > +++ b/drivers/soc/amazon/al_pos.c > @@ -0,0 +1,127 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. > + */ > +#include <linux/bitfield.h> > +#include <linux/interrupt.h> > +#include <linux/module.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > +#include <linux/of_platform.h> > + > +/* Registers Offset */ > +#define AL_POS_ERROR_LOG_1 0x0 > +#define AL_POS_ERROR_LOG_0 0x4 > + > +/* Registers Fields */ > +#define AL_POS_ERROR_LOG_1_VALID GENMASK(31, 31) > +#define AL_POS_ERROR_LOG_1_BRESP GENMASK(18, 17) > +#define AL_POS_ERROR_LOG_1_REQUEST_ID GENMASK(16, 8) > +#define AL_POS_ERROR_LOG_1_ADDR_HIGH GENMASK(7, 0) > + > +#define AL_POS_ERROR_LOG_0_ADDR_LOW GENMASK(31, 0) > + > +static int al_pos_panic; > +module_param(al_pos_panic, int, 0); > +MODULE_PARM_DESC(al_pos_panic, "Defines if POS error is causing panic()"); > + > +struct al_pos { > + struct platform_device *pdev; > + void __iomem *mmio_base; > + int irq; > +}; > + > +static irqreturn_t al_pos_irq_handler(int irq, void *info) > +{ > + struct platform_device *pdev = info; > + struct al_pos *pos = platform_get_drvdata(pdev); > + u32 log1; > + u32 log0; > + u64 addr; > + u16 request_id; > + u8 bresp; > + > + log1 = readl(pos->mmio_base + AL_POS_ERROR_LOG_1); Do you actually need the implied barriers? I'd expect that relaxed accesses should be enough. > + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1)) > + return IRQ_NONE; > + > + log0 = readl(pos->mmio_base + AL_POS_ERROR_LOG_0); > + writel(0, pos->mmio_base + AL_POS_ERROR_LOG_1); > + > + addr = FIELD_GET(AL_POS_ERROR_LOG_0_ADDR_LOW, log0); > + addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32); > + request_id = FIELD_GET(AL_POS_ERROR_LOG_1_REQUEST_ID, log1); > + bresp = FIELD_GET(AL_POS_ERROR_LOG_1_BRESP, log1); > + > + dev_err(&pdev->dev, "addr=0x%llx request_id=0x%x bresp=0x%x\n", > + addr, request_id, bresp); What is this information? How do we make use of it? Given that this is asynchronous, how do we correlate it to the offending software? > + > + if (al_pos_panic) > + panic("POS"); > + > + return IRQ_HANDLED; > +} > + > +static int al_pos_probe(struct platform_device *pdev) > +{ > + struct al_pos *pos; > + int ret; > + > + pos = devm_kzalloc(&pdev->dev, sizeof(*pos), GFP_KERNEL); > + if (!pos) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, pos); > + pos->pdev = pdev; > + > + pos->mmio_base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(pos->mmio_base)) { > + dev_err(&pdev->dev, "failed to ioremap memory (%ld)\n", > + PTR_ERR(pos->mmio_base)); > + return PTR_ERR(pos->mmio_base); > + } > + > + pos->irq = platform_get_irq(pdev, 0); > + if (pos->irq <= 0) { > + dev_err(&pdev->dev, "fail to parse and map irq\n"); > + return -EINVAL; > + } > + > + ret = devm_request_irq(&pdev->dev, > + pos->irq, > + al_pos_irq_handler, > + 0, > + pdev->name, > + pdev); > + if (ret != 0) { > + dev_err(&pdev->dev, > + "failed to register to irq %d (%d)\n", > + pos->irq, ret); > + return ret; > + } > + > + dev_info(&pdev->dev, "successfully loaded\n"); > + > + return 0; > +} > + > +static const struct of_device_id al_pos_of_match[] = { > + { .compatible = "amazon,al-pos", }, > + {}, > +}; > + > +MODULE_DEVICE_TABLE(of, al_pos_of_match); > + > +static struct platform_driver al_pos_driver = { > + .probe = al_pos_probe, > + .driver = { > + .name = "al-pos", > + .of_match_table = al_pos_of_match, > + }, > +}; > + > +module_platform_driver(al_pos_driver); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_AUTHOR("Talel Shenhar"); > +MODULE_DESCRIPTION("Amazon's Annapurna Labs POS driver"); > -- > 2.7.4 > The whole think looks to me like a poor man's EDAC handling, and I'd expect to be plugged in that subsystem instead. Any reason why this isn't the case? It would certainly make the handling uniform for the user. Thanks, M. -- Jazz is not dead, it just smells funny. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [UNVERIFIED SENDER] Re: [PATCH v2 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver 2019-09-11 14:15 ` Marc Zyngier @ 2019-09-12 6:50 ` Shenhar, Talel -1 siblings, 0 replies; 26+ messages in thread From: Shenhar, Talel @ 2019-09-12 6:50 UTC (permalink / raw) To: Marc Zyngier Cc: robh+dt, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, linux-kernel, devicetree, James Morse Hi Marc, On 9/11/2019 5:15 PM, Marc Zyngier wrote: > [+James] > > Hi Talel, > > On Tue, 10 Sep 2019 20:05:09 +0100, > Talel Shenhar <talel@amazon.com> wrote: > >> + log1 = readl(pos->mmio_base + AL_POS_ERROR_LOG_1); > Do you actually need the implied barriers? I'd expect that relaxed > accesses should be enough. You are correct. Barriers are not needed, In v1 this driver indeed used _relaxed versions. Due to request coming from Arnd in v1 patch series I removed it. As this is not data path I had no strong objection for removing it. > >> + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1)) >> + return IRQ_NONE; >> + >> + log0 = readl(pos->mmio_base + AL_POS_ERROR_LOG_0); >> + writel(0, pos->mmio_base + AL_POS_ERROR_LOG_1); >> + >> + addr = FIELD_GET(AL_POS_ERROR_LOG_0_ADDR_LOW, log0); >> + addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32); >> + request_id = FIELD_GET(AL_POS_ERROR_LOG_1_REQUEST_ID, log1); >> + bresp = FIELD_GET(AL_POS_ERROR_LOG_1_BRESP, log1); >> + >> + dev_err(&pdev->dev, "addr=0x%llx request_id=0x%x bresp=0x%x\n", >> + addr, request_id, bresp); > What is this information? How do we make use of it? Given that this is > asynchronous, how do we correlate it to the offending software? Indeed this information arriving from the HW is asynchronous. There is no direct method to get the offending software. There are all kinds of hacks we do to find the offending software once we find this error. most of the time its a new patch introduced but some of the time is just digging. > The whole think looks to me like a poor man's EDAC handling, and I'd > expect to be plugged in that subsystem instead. Any reason why this > isn't the case? It would certainly make the handling uniform for the > user. This logic was not plugged into EDAC as there is no "Correctable" error here. its just error event. Not all errors are EDAC in the sense of Error Detection And *Correction*. There are no correctable errors for this driver. So plugging it under EDAC seems like abusing the EDAC system. Now that I've emphasize the reason for not putting this under EDAC, what do you think? should this "only uncorrectable event" driver should be part of EDAC? Thanks, Talel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [UNVERIFIED SENDER] Re: [PATCH v2 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver @ 2019-09-12 6:50 ` Shenhar, Talel 0 siblings, 0 replies; 26+ messages in thread From: Shenhar, Talel @ 2019-09-12 6:50 UTC (permalink / raw) To: Marc Zyngier Cc: robh+dt, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, linux-kernel, devicetree, James Morse Hi Marc, On 9/11/2019 5:15 PM, Marc Zyngier wrote: > [+James] > > Hi Talel, > > On Tue, 10 Sep 2019 20:05:09 +0100, > Talel Shenhar <talel@amazon.com> wrote: > >> + log1 = readl(pos->mmio_base + AL_POS_ERROR_LOG_1); > Do you actually need the implied barriers? I'd expect that relaxed > accesses should be enough. You are correct. Barriers are not needed, In v1 this driver indeed used _relaxed versions. Due to request coming from Arnd in v1 patch series I removed it. As this is not data path I had no strong objection for removing it. > >> + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1)) >> + return IRQ_NONE; >> + >> + log0 = readl(pos->mmio_base + AL_POS_ERROR_LOG_0); >> + writel(0, pos->mmio_base + AL_POS_ERROR_LOG_1); >> + >> + addr = FIELD_GET(AL_POS_ERROR_LOG_0_ADDR_LOW, log0); >> + addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32); >> + request_id = FIELD_GET(AL_POS_ERROR_LOG_1_REQUEST_ID, log1); >> + bresp = FIELD_GET(AL_POS_ERROR_LOG_1_BRESP, log1); >> + >> + dev_err(&pdev->dev, "addr=0x%llx request_id=0x%x bresp=0x%x\n", >> + addr, request_id, bresp); > What is this information? How do we make use of it? Given that this is > asynchronous, how do we correlate it to the offending software? Indeed this information arriving from the HW is asynchronous. There is no direct method to get the offending software. There are all kinds of hacks we do to find the offending software once we find this error. most of the time its a new patch introduced but some of the time is just digging. > The whole think looks to me like a poor man's EDAC handling, and I'd > expect to be plugged in that subsystem instead. Any reason why this > isn't the case? It would certainly make the handling uniform for the > user. This logic was not plugged into EDAC as there is no "Correctable" error here. its just error event. Not all errors are EDAC in the sense of Error Detection And *Correction*. There are no correctable errors for this driver. So plugging it under EDAC seems like abusing the EDAC system. Now that I've emphasize the reason for not putting this under EDAC, what do you think? should this "only uncorrectable event" driver should be part of EDAC? Thanks, Talel ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [UNVERIFIED SENDER] Re: [PATCH v2 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver 2019-09-12 6:50 ` Shenhar, Talel @ 2019-09-12 8:50 ` Marc Zyngier -1 siblings, 0 replies; 26+ messages in thread From: Marc Zyngier @ 2019-09-12 8:50 UTC (permalink / raw) To: Shenhar, Talel Cc: robh+dt, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, linux-kernel, devicetree, James Morse On Thu, 12 Sep 2019 07:50:03 +0100, "Shenhar, Talel" <talel@amazon.com> wrote: > > Hi Marc, > > > On 9/11/2019 5:15 PM, Marc Zyngier wrote: > > [+James] > > > > Hi Talel, > > > > On Tue, 10 Sep 2019 20:05:09 +0100, > > Talel Shenhar <talel@amazon.com> wrote: > > > >> + log1 = readl(pos->mmio_base + AL_POS_ERROR_LOG_1); > > Do you actually need the implied barriers? I'd expect that relaxed > > accesses should be enough. > > You are correct. Barriers are not needed, In v1 this driver indeed > used _relaxed versions. > > Due to request coming from Arnd in v1 patch series I removed it. As > this is not data path I had no strong objection for removing it. Independently from whether this has any material impact on performance (this obviously isn't a hot path, unless it can be directly generated by userspace or a guest), I believe it is important to use the right type of accessor, if only because code gets copied around... Others would probably argue that this is the very reason why we should always use the "safe" option... > > > > >> + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1)) > >> + return IRQ_NONE; > >> + > >> + log0 = readl(pos->mmio_base + AL_POS_ERROR_LOG_0); > >> + writel(0, pos->mmio_base + AL_POS_ERROR_LOG_1); > >> + > >> + addr = FIELD_GET(AL_POS_ERROR_LOG_0_ADDR_LOW, log0); > >> + addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32); > >> + request_id = FIELD_GET(AL_POS_ERROR_LOG_1_REQUEST_ID, log1); > >> + bresp = FIELD_GET(AL_POS_ERROR_LOG_1_BRESP, log1); > >> + > >> + dev_err(&pdev->dev, "addr=0x%llx request_id=0x%x bresp=0x%x\n", > >> + addr, request_id, bresp); > > What is this information? How do we make use of it? Given that this is > > asynchronous, how do we correlate it to the offending software? > > Indeed this information arriving from the HW is asynchronous. > > There is no direct method to get the offending software. > > There are all kinds of hacks we do to find the offending software once > we find this error. most of the time its a new patch introduced but > some of the time is just digging. OK, so that the moment, this is more of a debug tool than anything else, right? > > The whole think looks to me like a poor man's EDAC handling, and I'd > > expect to be plugged in that subsystem instead. Any reason why this > > isn't the case? It would certainly make the handling uniform for the > > user. > > This logic was not plugged into EDAC as there is no "Correctable" > error here. its just error event. Not all errors are EDAC in the sense > of Error Detection And *Correction*. There are no correctable errors > for this driver. I'd argue the opposite! Because you obviously don't let a read-only register being written to, the error has been corrected, and you signal the correction status. > So plugging it under EDAC seems like abusing the EDAC system. > > Now that I've emphasize the reason for not putting this under EDAC, > what do you think? should this "only uncorrectable event" driver > should be part of EDAC? My choice would be to plug it into the EDAC subsystem, and report all interrupts as "Corrected" events. Optionally, and only if you are debugging something that requires it, report the error as "Uncorrectable", in which case the EDAC subsystem should trigger a panic. At least you'd get the infrastructure, logging and tooling that the EDAC subsystem offers (parsing the kernel log doesn't really count). Thanks, M. -- Jazz is not dead, it just smells funny. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [UNVERIFIED SENDER] Re: [PATCH v2 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver @ 2019-09-12 8:50 ` Marc Zyngier 0 siblings, 0 replies; 26+ messages in thread From: Marc Zyngier @ 2019-09-12 8:50 UTC (permalink / raw) To: Shenhar, Talel Cc: robh+dt, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, linux-kernel, devicetree, James Morse On Thu, 12 Sep 2019 07:50:03 +0100, "Shenhar, Talel" <talel@amazon.com> wrote: > > Hi Marc, > > > On 9/11/2019 5:15 PM, Marc Zyngier wrote: > > [+James] > > > > Hi Talel, > > > > On Tue, 10 Sep 2019 20:05:09 +0100, > > Talel Shenhar <talel@amazon.com> wrote: > > > >> + log1 = readl(pos->mmio_base + AL_POS_ERROR_LOG_1); > > Do you actually need the implied barriers? I'd expect that relaxed > > accesses should be enough. > > You are correct. Barriers are not needed, In v1 this driver indeed > used _relaxed versions. > > Due to request coming from Arnd in v1 patch series I removed it. As > this is not data path I had no strong objection for removing it. Independently from whether this has any material impact on performance (this obviously isn't a hot path, unless it can be directly generated by userspace or a guest), I believe it is important to use the right type of accessor, if only because code gets copied around... Others would probably argue that this is the very reason why we should always use the "safe" option... > > > > >> + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1)) > >> + return IRQ_NONE; > >> + > >> + log0 = readl(pos->mmio_base + AL_POS_ERROR_LOG_0); > >> + writel(0, pos->mmio_base + AL_POS_ERROR_LOG_1); > >> + > >> + addr = FIELD_GET(AL_POS_ERROR_LOG_0_ADDR_LOW, log0); > >> + addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32); > >> + request_id = FIELD_GET(AL_POS_ERROR_LOG_1_REQUEST_ID, log1); > >> + bresp = FIELD_GET(AL_POS_ERROR_LOG_1_BRESP, log1); > >> + > >> + dev_err(&pdev->dev, "addr=0x%llx request_id=0x%x bresp=0x%x\n", > >> + addr, request_id, bresp); > > What is this information? How do we make use of it? Given that this is > > asynchronous, how do we correlate it to the offending software? > > Indeed this information arriving from the HW is asynchronous. > > There is no direct method to get the offending software. > > There are all kinds of hacks we do to find the offending software once > we find this error. most of the time its a new patch introduced but > some of the time is just digging. OK, so that the moment, this is more of a debug tool than anything else, right? > > The whole think looks to me like a poor man's EDAC handling, and I'd > > expect to be plugged in that subsystem instead. Any reason why this > > isn't the case? It would certainly make the handling uniform for the > > user. > > This logic was not plugged into EDAC as there is no "Correctable" > error here. its just error event. Not all errors are EDAC in the sense > of Error Detection And *Correction*. There are no correctable errors > for this driver. I'd argue the opposite! Because you obviously don't let a read-only register being written to, the error has been corrected, and you signal the correction status. > So plugging it under EDAC seems like abusing the EDAC system. > > Now that I've emphasize the reason for not putting this under EDAC, > what do you think? should this "only uncorrectable event" driver > should be part of EDAC? My choice would be to plug it into the EDAC subsystem, and report all interrupts as "Corrected" events. Optionally, and only if you are debugging something that requires it, report the error as "Uncorrectable", in which case the EDAC subsystem should trigger a panic. At least you'd get the infrastructure, logging and tooling that the EDAC subsystem offers (parsing the kernel log doesn't really count). Thanks, M. -- Jazz is not dead, it just smells funny. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [UNVERIFIED SENDER] Re: [UNVERIFIED SENDER] Re: [PATCH v2 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver 2019-09-12 8:50 ` Marc Zyngier @ 2019-09-12 9:19 ` Shenhar, Talel -1 siblings, 0 replies; 26+ messages in thread From: Shenhar, Talel @ 2019-09-12 9:19 UTC (permalink / raw) To: Marc Zyngier Cc: robh+dt, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, linux-kernel, devicetree, James Morse On 9/12/2019 11:50 AM, Marc Zyngier wrote: > On Thu, 12 Sep 2019 07:50:03 +0100, > "Shenhar, Talel" <talel@amazon.com> wrote: >> Hi Marc, >> >> >> On 9/11/2019 5:15 PM, Marc Zyngier wrote: >>> [+James] >>> >>> Hi Talel, >>> >>> On Tue, 10 Sep 2019 20:05:09 +0100, >>> Talel Shenhar <talel@amazon.com> wrote: >>> >>>> + log1 = readl(pos->mmio_base + AL_POS_ERROR_LOG_1); >>> Do you actually need the implied barriers? I'd expect that relaxed >>> accesses should be enough. >> You are correct. Barriers are not needed, In v1 this driver indeed >> used _relaxed versions. >> >> Due to request coming from Arnd in v1 patch series I removed it. As >> this is not data path I had no strong objection for removing it. > Independently from whether this has any material impact on performance > (this obviously isn't a hot path, unless it can be directly generated > by userspace or a guest), I believe it is important to use the right > type of accessor, if only because code gets copied around... Others > would probably argue that this is the very reason why we should always > use the "safe" option... Arnld, would love your inputs before publishing v3 to avoid ping-pong. > >>>> + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1)) >>>> + return IRQ_NONE; >>>> + >>>> + log0 = readl(pos->mmio_base + AL_POS_ERROR_LOG_0); >>>> + writel(0, pos->mmio_base + AL_POS_ERROR_LOG_1); >>>> + >>>> + addr = FIELD_GET(AL_POS_ERROR_LOG_0_ADDR_LOW, log0); >>>> + addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32); >>>> + request_id = FIELD_GET(AL_POS_ERROR_LOG_1_REQUEST_ID, log1); >>>> + bresp = FIELD_GET(AL_POS_ERROR_LOG_1_BRESP, log1); >>>> + >>>> + dev_err(&pdev->dev, "addr=0x%llx request_id=0x%x bresp=0x%x\n", >>>> + addr, request_id, bresp); >>> What is this information? How do we make use of it? Given that this is >>> asynchronous, how do we correlate it to the offending software? >> Indeed this information arriving from the HW is asynchronous. >> >> There is no direct method to get the offending software. >> >> There are all kinds of hacks we do to find the offending software once >> we find this error. most of the time its a new patch introduced but >> some of the time is just digging. > OK, so that the moment, this is more of a debug tool than anything > else, right? Not sure what do you mean by debug tool. this is used to capture iliigle access and allow panic() based on them or simple logging. > >>> The whole think looks to me like a poor man's EDAC handling, and I'd >>> expect to be plugged in that subsystem instead. Any reason why this >>> isn't the case? It would certainly make the handling uniform for the >>> user. >> This logic was not plugged into EDAC as there is no "Correctable" >> error here. its just error event. Not all errors are EDAC in the sense >> of Error Detection And *Correction*. There are no correctable errors >> for this driver. > I'd argue the opposite! Because you obviously don't let a read-only > register being written to, the error has been corrected, and you > signal the correction status. Not the meaning of corrected from my point of view - the system as a whole (sw&hw) are not working state. a driver thinks it configured the system to do A while the system doesn't really do that. and the critical part is that the driver that did operation A doesn't even have a way to know it. So I would not call this corrected. > >> So plugging it under EDAC seems like abusing the EDAC system. >> >> Now that I've emphasize the reason for not putting this under EDAC, >> what do you think? should this "only uncorrectable event" driver >> should be part of EDAC? > My choice would be to plug it into the EDAC subsystem, and report all > interrupts as "Corrected" events. Optionally, and only if you are > debugging something that requires it, report the error as > "Uncorrectable", in which case the EDAC subsystem should trigger a > panic. > > At least you'd get the infrastructure, logging and tooling that the > EDAC subsystem offers (parsing the kernel log doesn't really count). I see what you say. However, I don't see too much added value in plugging this to EDAC and feel like it would abuse EDAC framework. James, will love your input from EDAC point of view, does it make sense to plug un-correctable only event to EDAC? > > Thanks, > > M. > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [UNVERIFIED SENDER] Re: [UNVERIFIED SENDER] Re: [PATCH v2 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver @ 2019-09-12 9:19 ` Shenhar, Talel 0 siblings, 0 replies; 26+ messages in thread From: Shenhar, Talel @ 2019-09-12 9:19 UTC (permalink / raw) To: Marc Zyngier Cc: robh+dt, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, linux-kernel, devicetree, James Morse On 9/12/2019 11:50 AM, Marc Zyngier wrote: > On Thu, 12 Sep 2019 07:50:03 +0100, > "Shenhar, Talel" <talel@amazon.com> wrote: >> Hi Marc, >> >> >> On 9/11/2019 5:15 PM, Marc Zyngier wrote: >>> [+James] >>> >>> Hi Talel, >>> >>> On Tue, 10 Sep 2019 20:05:09 +0100, >>> Talel Shenhar <talel@amazon.com> wrote: >>> >>>> + log1 = readl(pos->mmio_base + AL_POS_ERROR_LOG_1); >>> Do you actually need the implied barriers? I'd expect that relaxed >>> accesses should be enough. >> You are correct. Barriers are not needed, In v1 this driver indeed >> used _relaxed versions. >> >> Due to request coming from Arnd in v1 patch series I removed it. As >> this is not data path I had no strong objection for removing it. > Independently from whether this has any material impact on performance > (this obviously isn't a hot path, unless it can be directly generated > by userspace or a guest), I believe it is important to use the right > type of accessor, if only because code gets copied around... Others > would probably argue that this is the very reason why we should always > use the "safe" option... Arnld, would love your inputs before publishing v3 to avoid ping-pong. > >>>> + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1)) >>>> + return IRQ_NONE; >>>> + >>>> + log0 = readl(pos->mmio_base + AL_POS_ERROR_LOG_0); >>>> + writel(0, pos->mmio_base + AL_POS_ERROR_LOG_1); >>>> + >>>> + addr = FIELD_GET(AL_POS_ERROR_LOG_0_ADDR_LOW, log0); >>>> + addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32); >>>> + request_id = FIELD_GET(AL_POS_ERROR_LOG_1_REQUEST_ID, log1); >>>> + bresp = FIELD_GET(AL_POS_ERROR_LOG_1_BRESP, log1); >>>> + >>>> + dev_err(&pdev->dev, "addr=0x%llx request_id=0x%x bresp=0x%x\n", >>>> + addr, request_id, bresp); >>> What is this information? How do we make use of it? Given that this is >>> asynchronous, how do we correlate it to the offending software? >> Indeed this information arriving from the HW is asynchronous. >> >> There is no direct method to get the offending software. >> >> There are all kinds of hacks we do to find the offending software once >> we find this error. most of the time its a new patch introduced but >> some of the time is just digging. > OK, so that the moment, this is more of a debug tool than anything > else, right? Not sure what do you mean by debug tool. this is used to capture iliigle access and allow panic() based on them or simple logging. > >>> The whole think looks to me like a poor man's EDAC handling, and I'd >>> expect to be plugged in that subsystem instead. Any reason why this >>> isn't the case? It would certainly make the handling uniform for the >>> user. >> This logic was not plugged into EDAC as there is no "Correctable" >> error here. its just error event. Not all errors are EDAC in the sense >> of Error Detection And *Correction*. There are no correctable errors >> for this driver. > I'd argue the opposite! Because you obviously don't let a read-only > register being written to, the error has been corrected, and you > signal the correction status. Not the meaning of corrected from my point of view - the system as a whole (sw&hw) are not working state. a driver thinks it configured the system to do A while the system doesn't really do that. and the critical part is that the driver that did operation A doesn't even have a way to know it. So I would not call this corrected. > >> So plugging it under EDAC seems like abusing the EDAC system. >> >> Now that I've emphasize the reason for not putting this under EDAC, >> what do you think? should this "only uncorrectable event" driver >> should be part of EDAC? > My choice would be to plug it into the EDAC subsystem, and report all > interrupts as "Corrected" events. Optionally, and only if you are > debugging something that requires it, report the error as > "Uncorrectable", in which case the EDAC subsystem should trigger a > panic. > > At least you'd get the infrastructure, logging and tooling that the > EDAC subsystem offers (parsing the kernel log doesn't really count). I see what you say. However, I don't see too much added value in plugging this to EDAC and feel like it would abuse EDAC framework. James, will love your input from EDAC point of view, does it make sense to plug un-correctable only event to EDAC? > > Thanks, > > M. > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [UNVERIFIED SENDER] Re: [UNVERIFIED SENDER] Re: [PATCH v2 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver 2019-09-12 9:19 ` Shenhar, Talel (?) @ 2019-09-19 14:42 ` James Morse 2019-09-22 6:55 ` Shenhar, Talel -1 siblings, 1 reply; 26+ messages in thread From: James Morse @ 2019-09-19 14:42 UTC (permalink / raw) To: Shenhar, Talel Cc: Marc Zyngier, robh+dt, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, linux-kernel, devicetree Hi guys, On 12/09/2019 10:19, Shenhar, Talel wrote: > On 9/12/2019 11:50 AM, Marc Zyngier wrote: >> On Thu, 12 Sep 2019 07:50:03 +0100, >> "Shenhar, Talel" <talel@amazon.com> wrote: >>> On 9/11/2019 5:15 PM, Marc Zyngier wrote: >>>> On Tue, 10 Sep 2019 20:05:09 +0100, >>>> Talel Shenhar <talel@amazon.com> wrote: >>>>> + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1)) >>>>> + return IRQ_NONE; >>>>> + >>>>> + log0 = readl(pos->mmio_base + AL_POS_ERROR_LOG_0); >>>>> + writel(0, pos->mmio_base + AL_POS_ERROR_LOG_1); >>>>> + >>>>> + addr = FIELD_GET(AL_POS_ERROR_LOG_0_ADDR_LOW, log0); >>>>> + addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32); >>>>> + request_id = FIELD_GET(AL_POS_ERROR_LOG_1_REQUEST_ID, log1); >>>>> + bresp = FIELD_GET(AL_POS_ERROR_LOG_1_BRESP, log1); >>>>> + >>>>> + dev_err(&pdev->dev, "addr=0x%llx request_id=0x%x bresp=0x%x\n", >>>>> + addr, request_id, bresp); >>>> What is this information? How do we make use of it? Given that this is >>>> asynchronous, how do we correlate it to the offending software? >>> Indeed this information arriving from the HW is asynchronous. >>> >>> There is no direct method to get the offending software. >>> >>> There are all kinds of hacks we do to find the offending software once >>> we find this error. most of the time its a new patch introduced but >>> some of the time is just digging. >> OK, so that the moment, this is more of a debug tool than anything >> else, right? > Not sure what do you mean by debug tool. this is used to capture iliigle access and allow > panic() based on them or simple logging. Plumbing this into edac as a 'device' gives you the existing/standard interface for user-space. For example the 'panic_on_ue' that is exposed via sysfs, this saves you having another interface to toggle it for your driver. You can then use the existing distro tools to drive/monitor/sample it. >>>> The whole think looks to me like a poor man's EDAC handling, and I'd >>>> expect to be plugged in that subsystem instead. Any reason why this >>>> isn't the case? It would certainly make the handling uniform for the >>>> user. >>> This logic was not plugged into EDAC as there is no "Correctable" >>> error here. its just error event. Not all errors are EDAC in the sense >>> of Error Detection And *Correction*. There are no correctable errors >>> for this driver. >> I'd argue the opposite! Because you obviously don't let a read-only >> register being written to, the error has been corrected, and you >> signal the correction status. > Not the meaning of corrected from my point of view - the system as a whole (sw&hw) are not > working state. a driver thinks it configured the system to do A while the system doesn't > really do that. and the critical part is that the driver that did operation A doesn't even > have a way to know it. > > So I would not call this corrected. I don't think corrected/uncorrected helps here. If the register is read-only, and software writes to it, its a software bug. (from the v8.2's RAS extensions view, its somewhere between unrecoverable and uncontained) >>> So plugging it under EDAC seems like abusing the EDAC system. If EDAC doesn't do what you need, it can always be extended. >>> Now that I've emphasize the reason for not putting this under EDAC, >>> what do you think? should this "only uncorrectable event" driver >>> should be part of EDAC? Sure, (its for memory controllers, but:) enum edac_type has a EDAC_EC: "Error Checking, no correction". This wouldn't be the only device that only reports uncorrectable errors. >> My choice would be to plug it into the EDAC subsystem, and report all >> interrupts as "Corrected" events. Optionally, and only if you are >> debugging something that requires it, report the error as >> "Uncorrectable", in which case the EDAC subsystem should trigger a >> panic. >> At least you'd get the infrastructure, logging and tooling that the >> EDAC subsystem offers (parsing the kernel log doesn't really count). > I see what you say. However, I don't see too much added value in plugging this to EDAC and > feel like it would abuse EDAC framework. > James, will love your input from EDAC point of view, does it make sense to plug > un-correctable only event to EDAC? I think this device is an example of something like a "Fabric switch units" in Documentation/driver-api/edac.rst. It makes sense that it should be described as a 'device' to edac. You can then use the existing user-space tools to control/report/monitor the values. Thanks, James ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [UNVERIFIED SENDER] Re: [UNVERIFIED SENDER] Re: [PATCH v2 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver 2019-09-19 14:42 ` James Morse @ 2019-09-22 6:55 ` Shenhar, Talel 0 siblings, 0 replies; 26+ messages in thread From: Shenhar, Talel @ 2019-09-22 6:55 UTC (permalink / raw) To: James Morse Cc: Marc Zyngier, robh+dt, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, linux-kernel, devicetree On 9/19/2019 5:42 PM, James Morse wrote: > Hi guys, > > On 12/09/2019 10:19, Shenhar, Talel wrote: >> On 9/12/2019 11:50 AM, Marc Zyngier wrote: >>> On Thu, 12 Sep 2019 07:50:03 +0100, >>> "Shenhar, Talel" <talel@amazon.com> wrote: >>>> On 9/11/2019 5:15 PM, Marc Zyngier wrote: >>>>> On Tue, 10 Sep 2019 20:05:09 +0100, >>>>> Talel Shenhar <talel@amazon.com> wrote: >>>>> >> James, will love your input from EDAC point of view, does it make sense to plug >> un-correctable only event to EDAC? > I think this device is an example of something like a "Fabric switch units" in > Documentation/driver-api/edac.rst. It makes sense that it should be described as a > 'device' to edac. You can then use the existing user-space tools to control/report/monitor > the values. Thank you James, Will port this logic to be edac device. > > > Thanks, > > James ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [UNVERIFIED SENDER] Re: [UNVERIFIED SENDER] Re: [PATCH v2 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver @ 2019-09-22 6:55 ` Shenhar, Talel 0 siblings, 0 replies; 26+ messages in thread From: Shenhar, Talel @ 2019-09-22 6:55 UTC (permalink / raw) To: James Morse Cc: Marc Zyngier, robh+dt, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, linux-kernel, devicetree On 9/19/2019 5:42 PM, James Morse wrote: > Hi guys, > > On 12/09/2019 10:19, Shenhar, Talel wrote: >> On 9/12/2019 11:50 AM, Marc Zyngier wrote: >>> On Thu, 12 Sep 2019 07:50:03 +0100, >>> "Shenhar, Talel" <talel@amazon.com> wrote: >>>> On 9/11/2019 5:15 PM, Marc Zyngier wrote: >>>>> On Tue, 10 Sep 2019 20:05:09 +0100, >>>>> Talel Shenhar <talel@amazon.com> wrote: >>>>> >> James, will love your input from EDAC point of view, does it make sense to plug >> un-correctable only event to EDAC? > I think this device is an example of something like a "Fabric switch units" in > Documentation/driver-api/edac.rst. It makes sense that it should be described as a > 'device' to edac. You can then use the existing user-space tools to control/report/monitor > the values. Thank you James, Will port this logic to be edac device. > > > Thanks, > > James ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 3/3] soc: amazon: al-pos: cast to u64 before left shifting 2019-09-10 19:05 ` Talel Shenhar @ 2019-09-10 19:05 ` Talel Shenhar -1 siblings, 0 replies; 26+ messages in thread From: Talel Shenhar @ 2019-09-10 19:05 UTC (permalink / raw) To: robh+dt, marc.zyngier, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, talel, linux-kernel, devicetree Fix wrap around for pos errors on addresses above 32 bit. Reported-by: kbuild test robot <lkp@intel.com> Signed-off-by: Talel Shenhar <talel@amazon.com> --- drivers/soc/amazon/al_pos.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/soc/amazon/al_pos.c b/drivers/soc/amazon/al_pos.c index a865111..e95e1fc 100644 --- a/drivers/soc/amazon/al_pos.c +++ b/drivers/soc/amazon/al_pos.c @@ -49,7 +49,7 @@ static irqreturn_t al_pos_irq_handler(int irq, void *info) writel(0, pos->mmio_base + AL_POS_ERROR_LOG_1); addr = FIELD_GET(AL_POS_ERROR_LOG_0_ADDR_LOW, log0); - addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32); + addr |= (((u64)FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1)) << 32); request_id = FIELD_GET(AL_POS_ERROR_LOG_1_REQUEST_ID, log1); bresp = FIELD_GET(AL_POS_ERROR_LOG_1_BRESP, log1); -- 2.7.4 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 3/3] soc: amazon: al-pos: cast to u64 before left shifting @ 2019-09-10 19:05 ` Talel Shenhar 0 siblings, 0 replies; 26+ messages in thread From: Talel Shenhar @ 2019-09-10 19:05 UTC (permalink / raw) To: robh+dt, marc.zyngier, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, talel, linux-kernel, devicetree Fix wrap around for pos errors on addresses above 32 bit. Reported-by: kbuild test robot <lkp@intel.com> Signed-off-by: Talel Shenhar <talel@amazon.com> --- drivers/soc/amazon/al_pos.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/soc/amazon/al_pos.c b/drivers/soc/amazon/al_pos.c index a865111..e95e1fc 100644 --- a/drivers/soc/amazon/al_pos.c +++ b/drivers/soc/amazon/al_pos.c @@ -49,7 +49,7 @@ static irqreturn_t al_pos_irq_handler(int irq, void *info) writel(0, pos->mmio_base + AL_POS_ERROR_LOG_1); addr = FIELD_GET(AL_POS_ERROR_LOG_0_ADDR_LOW, log0); - addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32); + addr |= (((u64)FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1)) << 32); request_id = FIELD_GET(AL_POS_ERROR_LOG_1_REQUEST_ID, log1); bresp = FIELD_GET(AL_POS_ERROR_LOG_1_BRESP, log1); -- 2.7.4 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2 3/3] soc: amazon: al-pos: cast to u64 before left shifting 2019-09-10 19:05 ` Talel Shenhar @ 2019-09-11 14:18 ` Marc Zyngier -1 siblings, 0 replies; 26+ messages in thread From: Marc Zyngier @ 2019-09-11 14:18 UTC (permalink / raw) To: Talel Shenhar Cc: robh+dt, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, linux-kernel, devicetree On Tue, 10 Sep 2019 20:05:10 +0100, Talel Shenhar <talel@amazon.com> wrote: > > Fix wrap around for pos errors on addresses above 32 bit. > > Reported-by: kbuild test robot <lkp@intel.com> > Signed-off-by: Talel Shenhar <talel@amazon.com> > --- > drivers/soc/amazon/al_pos.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/soc/amazon/al_pos.c b/drivers/soc/amazon/al_pos.c > index a865111..e95e1fc 100644 > --- a/drivers/soc/amazon/al_pos.c > +++ b/drivers/soc/amazon/al_pos.c > @@ -49,7 +49,7 @@ static irqreturn_t al_pos_irq_handler(int irq, void *info) > writel(0, pos->mmio_base + AL_POS_ERROR_LOG_1); > > addr = FIELD_GET(AL_POS_ERROR_LOG_0_ADDR_LOW, log0); > - addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32); > + addr |= (((u64)FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1)) << 32); > request_id = FIELD_GET(AL_POS_ERROR_LOG_1_REQUEST_ID, log1); > bresp = FIELD_GET(AL_POS_ERROR_LOG_1_BRESP, log1); > > -- > 2.7.4 > This fix should be squashed into the previous patch. Thanks, M. -- Jazz is not dead, it just smells funny. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 3/3] soc: amazon: al-pos: cast to u64 before left shifting @ 2019-09-11 14:18 ` Marc Zyngier 0 siblings, 0 replies; 26+ messages in thread From: Marc Zyngier @ 2019-09-11 14:18 UTC (permalink / raw) To: Talel Shenhar Cc: robh+dt, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, linux-kernel, devicetree On Tue, 10 Sep 2019 20:05:10 +0100, Talel Shenhar <talel@amazon.com> wrote: > > Fix wrap around for pos errors on addresses above 32 bit. > > Reported-by: kbuild test robot <lkp@intel.com> > Signed-off-by: Talel Shenhar <talel@amazon.com> > --- > drivers/soc/amazon/al_pos.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/soc/amazon/al_pos.c b/drivers/soc/amazon/al_pos.c > index a865111..e95e1fc 100644 > --- a/drivers/soc/amazon/al_pos.c > +++ b/drivers/soc/amazon/al_pos.c > @@ -49,7 +49,7 @@ static irqreturn_t al_pos_irq_handler(int irq, void *info) > writel(0, pos->mmio_base + AL_POS_ERROR_LOG_1); > > addr = FIELD_GET(AL_POS_ERROR_LOG_0_ADDR_LOW, log0); > - addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32); > + addr |= (((u64)FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1)) << 32); > request_id = FIELD_GET(AL_POS_ERROR_LOG_1_REQUEST_ID, log1); > bresp = FIELD_GET(AL_POS_ERROR_LOG_1_BRESP, log1); > > -- > 2.7.4 > This fix should be squashed into the previous patch. Thanks, M. -- Jazz is not dead, it just smells funny. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [UNVERIFIED SENDER] Re: [PATCH v2 3/3] soc: amazon: al-pos: cast to u64 before left shifting 2019-09-11 14:18 ` Marc Zyngier @ 2019-09-12 6:51 ` Shenhar, Talel -1 siblings, 0 replies; 26+ messages in thread From: Shenhar, Talel @ 2019-09-12 6:51 UTC (permalink / raw) To: Marc Zyngier Cc: robh+dt, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, linux-kernel, devicetree On 9/11/2019 5:18 PM, Marc Zyngier wrote: > On Tue, 10 Sep 2019 20:05:10 +0100, > Talel Shenhar <talel@amazon.com> wrote: >> Fix wrap around for pos errors on addresses above 32 bit. >> >> Reported-by: kbuild test robot <lkp@intel.com> >> Signed-off-by: Talel Shenhar <talel@amazon.com> >> --- >> drivers/soc/amazon/al_pos.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/soc/amazon/al_pos.c b/drivers/soc/amazon/al_pos.c >> index a865111..e95e1fc 100644 >> --- a/drivers/soc/amazon/al_pos.c >> +++ b/drivers/soc/amazon/al_pos.c >> @@ -49,7 +49,7 @@ static irqreturn_t al_pos_irq_handler(int irq, void *info) >> writel(0, pos->mmio_base + AL_POS_ERROR_LOG_1); >> >> addr = FIELD_GET(AL_POS_ERROR_LOG_0_ADDR_LOW, log0); >> - addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32); >> + addr |= (((u64)FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1)) << 32); >> request_id = FIELD_GET(AL_POS_ERROR_LOG_1_REQUEST_ID, log1); >> bresp = FIELD_GET(AL_POS_ERROR_LOG_1_BRESP, log1); >> >> -- >> 2.7.4 >> > This fix should be squashed into the previous patch. Sure, Shall be part of v3. > > Thanks, > > M. > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [UNVERIFIED SENDER] Re: [PATCH v2 3/3] soc: amazon: al-pos: cast to u64 before left shifting @ 2019-09-12 6:51 ` Shenhar, Talel 0 siblings, 0 replies; 26+ messages in thread From: Shenhar, Talel @ 2019-09-12 6:51 UTC (permalink / raw) To: Marc Zyngier Cc: robh+dt, tglx, jason, mark.rutland, nicolas.ferre, mchehab+samsung, shawn.lin, gregkh, dwmw, benh, linux-kernel, devicetree On 9/11/2019 5:18 PM, Marc Zyngier wrote: > On Tue, 10 Sep 2019 20:05:10 +0100, > Talel Shenhar <talel@amazon.com> wrote: >> Fix wrap around for pos errors on addresses above 32 bit. >> >> Reported-by: kbuild test robot <lkp@intel.com> >> Signed-off-by: Talel Shenhar <talel@amazon.com> >> --- >> drivers/soc/amazon/al_pos.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/soc/amazon/al_pos.c b/drivers/soc/amazon/al_pos.c >> index a865111..e95e1fc 100644 >> --- a/drivers/soc/amazon/al_pos.c >> +++ b/drivers/soc/amazon/al_pos.c >> @@ -49,7 +49,7 @@ static irqreturn_t al_pos_irq_handler(int irq, void *info) >> writel(0, pos->mmio_base + AL_POS_ERROR_LOG_1); >> >> addr = FIELD_GET(AL_POS_ERROR_LOG_0_ADDR_LOW, log0); >> - addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32); >> + addr |= (((u64)FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1)) << 32); >> request_id = FIELD_GET(AL_POS_ERROR_LOG_1_REQUEST_ID, log1); >> bresp = FIELD_GET(AL_POS_ERROR_LOG_1_BRESP, log1); >> >> -- >> 2.7.4 >> > This fix should be squashed into the previous patch. Sure, Shall be part of v3. > > Thanks, > > M. > ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2019-09-22 6:55 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-10 19:05 [PATCH v2 0/3] Amazon's Annapurna Labs POS Driver Talel Shenhar 2019-09-10 19:05 ` Talel Shenhar 2019-09-10 19:05 ` [PATCH v2 1/3] dt-bindings: soc: al-pos: Amazon's Annapurna Labs POS Talel Shenhar 2019-09-10 19:05 ` Talel Shenhar 2019-09-18 13:32 ` Rob Herring 2019-09-18 13:44 ` Shenhar, Talel 2019-09-18 13:44 ` Shenhar, Talel 2019-09-10 19:05 ` [PATCH v2 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver Talel Shenhar 2019-09-10 19:05 ` Talel Shenhar 2019-09-11 14:15 ` Marc Zyngier 2019-09-11 14:15 ` Marc Zyngier 2019-09-12 6:50 ` [UNVERIFIED SENDER] " Shenhar, Talel 2019-09-12 6:50 ` Shenhar, Talel 2019-09-12 8:50 ` Marc Zyngier 2019-09-12 8:50 ` Marc Zyngier 2019-09-12 9:19 ` [UNVERIFIED SENDER] " Shenhar, Talel 2019-09-12 9:19 ` Shenhar, Talel 2019-09-19 14:42 ` James Morse 2019-09-22 6:55 ` Shenhar, Talel 2019-09-22 6:55 ` Shenhar, Talel 2019-09-10 19:05 ` [PATCH v2 3/3] soc: amazon: al-pos: cast to u64 before left shifting Talel Shenhar 2019-09-10 19:05 ` Talel Shenhar 2019-09-11 14:18 ` Marc Zyngier 2019-09-11 14:18 ` Marc Zyngier 2019-09-12 6:51 ` [UNVERIFIED SENDER] " Shenhar, Talel 2019-09-12 6:51 ` Shenhar, Talel
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.