* [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
@ 2019-09-09 9:10 ` Talel Shenhar
0 siblings, 0 replies; 51+ messages in thread
From: Talel Shenhar @ 2019-09-09 9:10 UTC (permalink / raw)
To: robh+dt, mark.rutland, mchehab+samsung, davem, gregkh,
nicolas.ferre, tglx, arnd, venture, linus.walleij, olof, mripard,
ssantosh, paul.kocialkowski, mjourdan, catalin.marinas, will,
talel, devicetree, linux-kernel, linux-arm-kernel
Cc: barakw, hhhawa, benh, jonnyc, ronenk, hanochu, dwmw
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 | 6 +++
drivers/soc/Kconfig | 1 +
drivers/soc/Makefile | 1 +
drivers/soc/amazon/Kconfig | 5 ++
drivers/soc/amazon/Makefile | 1 +
drivers/soc/amazon/al_pos.c | 129 ++++++++++++++++++++++++++++++++++++++++++++
6 files changed, 143 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..627af40 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -751,6 +751,12 @@ 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>
+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..6d0bdff
--- /dev/null
+++ b/drivers/soc/amazon/al_pos.c
@@ -0,0 +1,129 @@
+// 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>
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Talel Shenhar");
+MODULE_DESCRIPTION("Amazon's Annapurna Labs POS driver");
+
+/* 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_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_1);
+ if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1))
+ return IRQ_NONE;
+
+ log0 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_0);
+ writel_relaxed(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;
+ struct resource *resource;
+ int ret;
+
+ pos = devm_kzalloc(&pdev->dev, sizeof(*pos), GFP_KERNEL);
+ if (!pos)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, pos);
+ pos->pdev = pdev;
+
+ resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ pos->mmio_base = devm_ioremap_resource(&pdev->dev, resource);
+ 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 = irq_of_parse_and_map(pdev->dev.of_node, 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);
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 51+ messages in thread
* [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
@ 2019-09-09 9:10 ` Talel Shenhar
0 siblings, 0 replies; 51+ messages in thread
From: Talel Shenhar @ 2019-09-09 9:10 UTC (permalink / raw)
To: robh+dt, mark.rutland, mchehab+samsung, davem, gregkh,
nicolas.ferre, tglx, arnd, venture, linus.walleij, olof, mripard,
ssantosh, paul.kocialkowski, mjourdan, catalin.marinas, will,
talel, devicetree, linux-kernel, linux-arm-kernel
Cc: dwmw, benh, hhhawa, ronenk, jonnyc, hanochu, barakw
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 | 6 +++
drivers/soc/Kconfig | 1 +
drivers/soc/Makefile | 1 +
drivers/soc/amazon/Kconfig | 5 ++
drivers/soc/amazon/Makefile | 1 +
drivers/soc/amazon/al_pos.c | 129 ++++++++++++++++++++++++++++++++++++++++++++
6 files changed, 143 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..627af40 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -751,6 +751,12 @@ 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>
+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..6d0bdff
--- /dev/null
+++ b/drivers/soc/amazon/al_pos.c
@@ -0,0 +1,129 @@
+// 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>
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Talel Shenhar");
+MODULE_DESCRIPTION("Amazon's Annapurna Labs POS driver");
+
+/* 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_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_1);
+ if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1))
+ return IRQ_NONE;
+
+ log0 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_0);
+ writel_relaxed(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;
+ struct resource *resource;
+ int ret;
+
+ pos = devm_kzalloc(&pdev->dev, sizeof(*pos), GFP_KERNEL);
+ if (!pos)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, pos);
+ pos->pdev = pdev;
+
+ resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ pos->mmio_base = devm_ioremap_resource(&pdev->dev, resource);
+ 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 = irq_of_parse_and_map(pdev->dev.of_node, 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);
--
2.7.4
^ permalink raw reply related [flat|nested] 51+ messages in thread
* Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
2019-09-09 9:10 ` Talel Shenhar
(?)
@ 2019-09-09 9:44 ` Arnd Bergmann
-1 siblings, 0 replies; 51+ messages in thread
From: Arnd Bergmann @ 2019-09-09 9:44 UTC (permalink / raw)
To: Talel Shenhar
Cc: Rob Herring, Mark Rutland, Mauro Carvalho Chehab, David Miller,
gregkh, Nicolas Ferre, Thomas Gleixner, Patrick Venture,
Linus Walleij, Olof Johansson, Maxime Ripard, Santosh Shilimkar,
paul.kocialkowski, mjourdan, Catalin Marinas, Will Deacon, DTML,
linux-kernel, Linux ARM, David Woodhouse, Benjamin Herrenschmidt,
hhhawa, ronenk, jonnyc, hanochu, barakw
On Mon, Sep 9, 2019 at 11:14 AM 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>
Looks ok overall, juts a few minor comments:
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Talel Shenhar");
> +MODULE_DESCRIPTION("Amazon's Annapurna Labs POS driver");
These usually go to the end of the file.
> + log1 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_1);
> + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1))
> + return IRQ_NONE;
> +
> + log0 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_0);
> + writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1);
Why do you require _relaxed() accessors here? Please add a comment
explaining that, or use the regular readl()/writel().
> + resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + pos->mmio_base = devm_ioremap_resource(&pdev->dev, resource);
This can be simplified to devm_platform_ioremap_resource().
> + pos->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
And this is usually written as platform_get_irq()
Arnd
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
@ 2019-09-09 9:44 ` Arnd Bergmann
0 siblings, 0 replies; 51+ messages in thread
From: Arnd Bergmann @ 2019-09-09 9:44 UTC (permalink / raw)
To: Talel Shenhar
Cc: Mark Rutland, mjourdan, Catalin Marinas, Linus Walleij,
linux-kernel, jonnyc, Mauro Carvalho Chehab, ronenk, Will Deacon,
Benjamin Herrenschmidt, DTML, Maxime Ripard, Rob Herring,
Santosh Shilimkar, Thomas Gleixner, hanochu, Linux ARM, barakw,
hhhawa, gregkh, paul.kocialkowski, Patrick Venture,
Olof Johansson, David Miller, David Woodhouse
On Mon, Sep 9, 2019 at 11:14 AM 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>
Looks ok overall, juts a few minor comments:
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Talel Shenhar");
> +MODULE_DESCRIPTION("Amazon's Annapurna Labs POS driver");
These usually go to the end of the file.
> + log1 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_1);
> + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1))
> + return IRQ_NONE;
> +
> + log0 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_0);
> + writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1);
Why do you require _relaxed() accessors here? Please add a comment
explaining that, or use the regular readl()/writel().
> + resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + pos->mmio_base = devm_ioremap_resource(&pdev->dev, resource);
This can be simplified to devm_platform_ioremap_resource().
> + pos->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
And this is usually written as platform_get_irq()
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
@ 2019-09-09 9:44 ` Arnd Bergmann
0 siblings, 0 replies; 51+ messages in thread
From: Arnd Bergmann @ 2019-09-09 9:44 UTC (permalink / raw)
To: Talel Shenhar
Cc: Rob Herring, Mark Rutland, Mauro Carvalho Chehab, David Miller,
gregkh, Nicolas Ferre, Thomas Gleixner, Patrick Venture,
Linus Walleij, Olof Johansson, Maxime Ripard, Santosh Shilimkar,
paul.kocialkowski, mjourdan, Catalin Marinas, Will Deacon, DTML,
linux-kernel, Linux ARM
On Mon, Sep 9, 2019 at 11:14 AM 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>
Looks ok overall, juts a few minor comments:
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Talel Shenhar");
> +MODULE_DESCRIPTION("Amazon's Annapurna Labs POS driver");
These usually go to the end of the file.
> + log1 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_1);
> + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1))
> + return IRQ_NONE;
> +
> + log0 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_0);
> + writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1);
Why do you require _relaxed() accessors here? Please add a comment
explaining that, or use the regular readl()/writel().
> + resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + pos->mmio_base = devm_ioremap_resource(&pdev->dev, resource);
This can be simplified to devm_platform_ioremap_resource().
> + pos->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
And this is usually written as platform_get_irq()
Arnd
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
2019-09-09 9:44 ` Arnd Bergmann
(?)
@ 2019-09-09 11:12 ` Shenhar, Talel
-1 siblings, 0 replies; 51+ messages in thread
From: Shenhar, Talel @ 2019-09-09 11:12 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Rob Herring, Mark Rutland, Mauro Carvalho Chehab, David Miller,
gregkh, Nicolas Ferre, Thomas Gleixner, Patrick Venture,
Linus Walleij, Olof Johansson, Maxime Ripard, Santosh Shilimkar,
paul.kocialkowski, mjourdan, Catalin Marinas, Will Deacon, DTML,
linux-kernel, Linux ARM, David Woodhouse, Benjamin Herrenschmidt,
hhhawa, ronenk, jonnyc, hanochu, barakw
On 9/9/2019 12:44 PM, Arnd Bergmann wrote:
> On Mon, Sep 9, 2019 at 11:14 AM 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>
> Looks ok overall, juts a few minor comments:
Thanks.
>
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR("Talel Shenhar");
>> +MODULE_DESCRIPTION("Amazon's Annapurna Labs POS driver");
> These usually go to the end of the file.
Ack, Will move them as part of v2.
>
>> + log1 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_1);
>> + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1))
>> + return IRQ_NONE;
>> +
>> + log0 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_0);
>> + writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1);
> Why do you require _relaxed() accessors here? Please add a comment
> explaining that, or use the regular readl()/writel().
I don't think commenting is needed here as there is nothing special in
this type of access.
I don't see this is common to comment the use of the _relaxed accessors.
This driver is for SoC using arm64 cpu.
If one uses the non-relaxed version of readl while running on arm64, he
shall cause read barrier, which is then doing dsm(ld).. This barrier is
not needed here, so we spare the use of the more heavy readl in favor of
the less "harmful" one.
Let me know what you think.
>
>> + resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + pos->mmio_base = devm_ioremap_resource(&pdev->dev, resource);
> This can be simplified to devm_platform_ioremap_resource().
Ack, Will simplify them in v2.
>
>> + pos->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> And this is usually written as platform_get_irq()
Ack, Will replace them with platform_get_irq() in v2.
>
> Arnd
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
@ 2019-09-09 11:12 ` Shenhar, Talel
0 siblings, 0 replies; 51+ messages in thread
From: Shenhar, Talel @ 2019-09-09 11:12 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Mark Rutland, mjourdan, Catalin Marinas, Linus Walleij,
linux-kernel, jonnyc, Mauro Carvalho Chehab, ronenk, Will Deacon,
Benjamin Herrenschmidt, DTML, Maxime Ripard, Rob Herring,
Santosh Shilimkar, Thomas Gleixner, hanochu, Linux ARM, barakw,
hhhawa, gregkh, paul.kocialkowski, Patrick Venture,
Olof Johansson, David Miller, David Woodhouse
On 9/9/2019 12:44 PM, Arnd Bergmann wrote:
> On Mon, Sep 9, 2019 at 11:14 AM 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>
> Looks ok overall, juts a few minor comments:
Thanks.
>
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR("Talel Shenhar");
>> +MODULE_DESCRIPTION("Amazon's Annapurna Labs POS driver");
> These usually go to the end of the file.
Ack, Will move them as part of v2.
>
>> + log1 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_1);
>> + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1))
>> + return IRQ_NONE;
>> +
>> + log0 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_0);
>> + writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1);
> Why do you require _relaxed() accessors here? Please add a comment
> explaining that, or use the regular readl()/writel().
I don't think commenting is needed here as there is nothing special in
this type of access.
I don't see this is common to comment the use of the _relaxed accessors.
This driver is for SoC using arm64 cpu.
If one uses the non-relaxed version of readl while running on arm64, he
shall cause read barrier, which is then doing dsm(ld).. This barrier is
not needed here, so we spare the use of the more heavy readl in favor of
the less "harmful" one.
Let me know what you think.
>
>> + resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + pos->mmio_base = devm_ioremap_resource(&pdev->dev, resource);
> This can be simplified to devm_platform_ioremap_resource().
Ack, Will simplify them in v2.
>
>> + pos->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> And this is usually written as platform_get_irq()
Ack, Will replace them with platform_get_irq() in v2.
>
> Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
@ 2019-09-09 11:12 ` Shenhar, Talel
0 siblings, 0 replies; 51+ messages in thread
From: Shenhar, Talel @ 2019-09-09 11:12 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Rob Herring, Mark Rutland, Mauro Carvalho Chehab, David Miller,
gregkh, Nicolas Ferre, Thomas Gleixner, Patrick Venture,
Linus Walleij, Olof Johansson, Maxime Ripard, Santosh Shilimkar,
paul.kocialkowski, mjourdan, Catalin Marinas, Will Deacon, DTML,
linux-kernel, Linux ARM
On 9/9/2019 12:44 PM, Arnd Bergmann wrote:
> On Mon, Sep 9, 2019 at 11:14 AM 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>
> Looks ok overall, juts a few minor comments:
Thanks.
>
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR("Talel Shenhar");
>> +MODULE_DESCRIPTION("Amazon's Annapurna Labs POS driver");
> These usually go to the end of the file.
Ack, Will move them as part of v2.
>
>> + log1 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_1);
>> + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1))
>> + return IRQ_NONE;
>> +
>> + log0 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_0);
>> + writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1);
> Why do you require _relaxed() accessors here? Please add a comment
> explaining that, or use the regular readl()/writel().
I don't think commenting is needed here as there is nothing special in
this type of access.
I don't see this is common to comment the use of the _relaxed accessors.
This driver is for SoC using arm64 cpu.
If one uses the non-relaxed version of readl while running on arm64, he
shall cause read barrier, which is then doing dsm(ld).. This barrier is
not needed here, so we spare the use of the more heavy readl in favor of
the less "harmful" one.
Let me know what you think.
>
>> + resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + pos->mmio_base = devm_ioremap_resource(&pdev->dev, resource);
> This can be simplified to devm_platform_ioremap_resource().
Ack, Will simplify them in v2.
>
>> + pos->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> And this is usually written as platform_get_irq()
Ack, Will replace them with platform_get_irq() in v2.
>
> Arnd
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
2019-09-09 11:12 ` Shenhar, Talel
(?)
@ 2019-09-09 13:41 ` Arnd Bergmann
-1 siblings, 0 replies; 51+ messages in thread
From: Arnd Bergmann @ 2019-09-09 13:41 UTC (permalink / raw)
To: Shenhar, Talel
Cc: Rob Herring, Mark Rutland, Mauro Carvalho Chehab, David Miller,
gregkh, Nicolas Ferre, Thomas Gleixner, Patrick Venture,
Linus Walleij, Olof Johansson, Maxime Ripard, Santosh Shilimkar,
paul.kocialkowski, mjourdan, Catalin Marinas, Will Deacon, DTML,
linux-kernel, Linux ARM, David Woodhouse, Benjamin Herrenschmidt,
hhhawa, ronenk, jonnyc, hanochu, barakw
On Mon, Sep 9, 2019 at 1:13 PM Shenhar, Talel <talel@amazon.com> wrote:
> On 9/9/2019 12:44 PM, Arnd Bergmann wrote:
> > On Mon, Sep 9, 2019 at 11:14 AM Talel Shenhar <talel@amazon.com> wrote:
> >> + writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1);
> > Why do you require _relaxed() accessors here? Please add a comment
> > explaining that, or use the regular readl()/writel().
>
> I don't think commenting is needed here as there is nothing special in
> this type of access.
>
> I don't see this is common to comment the use of the _relaxed accessors.
I usually mention it in driver reviews, but most authors revert back
to the normal accessors when there is no difference.
> This driver is for SoC using arm64 cpu.
>
> If one uses the non-relaxed version of readl while running on arm64, he
> shall cause read barrier, which is then doing dsm(ld).. This barrier is
> not needed here, so we spare the use of the more heavy readl in favor of
> the less "harmful" one.
>
> Let me know what you think.
If the barrier causes no harm, just leave it in to keep the code more
readable. Most developers don't need to know the difference between
the two, so using the less common interface just makes the reader
curious about why it was picked.
Avoiding the barrier can make a huge performance difference in a
hot code path, but the downside is that it can behave in unexpected
ways if the same code is run on a different CPU architecture that
does not have the exact same rules about what _relaxed() means.
In fact, replacing a 'readl()' with 'readl_relaxed() + rmb()' can lead
to slower rather than faster code when the explicit barrier is heavier
than the implied one (e.g. on x86), or readl_relaxed() does not skip
the barrier.
The general rule with kernel interfaces when you have two versions
that both do what you want is to pick the one with the shorter name.
See spin_lock()/spin_lock_irqsave(), ioremap()/ioremap_nocache(),
or ktime_get()/ktime_get_clocktai_ts64(). (yes, there are also
exceptions)
Arnd
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
@ 2019-09-09 13:41 ` Arnd Bergmann
0 siblings, 0 replies; 51+ messages in thread
From: Arnd Bergmann @ 2019-09-09 13:41 UTC (permalink / raw)
To: Shenhar, Talel
Cc: Mark Rutland, mjourdan, Catalin Marinas, Linus Walleij,
linux-kernel, jonnyc, Mauro Carvalho Chehab, ronenk, Will Deacon,
Benjamin Herrenschmidt, DTML, Maxime Ripard, Rob Herring,
Santosh Shilimkar, Thomas Gleixner, hanochu, Linux ARM, barakw,
hhhawa, gregkh, paul.kocialkowski, Patrick Venture,
Olof Johansson, David Miller, David Woodhouse
On Mon, Sep 9, 2019 at 1:13 PM Shenhar, Talel <talel@amazon.com> wrote:
> On 9/9/2019 12:44 PM, Arnd Bergmann wrote:
> > On Mon, Sep 9, 2019 at 11:14 AM Talel Shenhar <talel@amazon.com> wrote:
> >> + writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1);
> > Why do you require _relaxed() accessors here? Please add a comment
> > explaining that, or use the regular readl()/writel().
>
> I don't think commenting is needed here as there is nothing special in
> this type of access.
>
> I don't see this is common to comment the use of the _relaxed accessors.
I usually mention it in driver reviews, but most authors revert back
to the normal accessors when there is no difference.
> This driver is for SoC using arm64 cpu.
>
> If one uses the non-relaxed version of readl while running on arm64, he
> shall cause read barrier, which is then doing dsm(ld).. This barrier is
> not needed here, so we spare the use of the more heavy readl in favor of
> the less "harmful" one.
>
> Let me know what you think.
If the barrier causes no harm, just leave it in to keep the code more
readable. Most developers don't need to know the difference between
the two, so using the less common interface just makes the reader
curious about why it was picked.
Avoiding the barrier can make a huge performance difference in a
hot code path, but the downside is that it can behave in unexpected
ways if the same code is run on a different CPU architecture that
does not have the exact same rules about what _relaxed() means.
In fact, replacing a 'readl()' with 'readl_relaxed() + rmb()' can lead
to slower rather than faster code when the explicit barrier is heavier
than the implied one (e.g. on x86), or readl_relaxed() does not skip
the barrier.
The general rule with kernel interfaces when you have two versions
that both do what you want is to pick the one with the shorter name.
See spin_lock()/spin_lock_irqsave(), ioremap()/ioremap_nocache(),
or ktime_get()/ktime_get_clocktai_ts64(). (yes, there are also
exceptions)
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
@ 2019-09-09 13:41 ` Arnd Bergmann
0 siblings, 0 replies; 51+ messages in thread
From: Arnd Bergmann @ 2019-09-09 13:41 UTC (permalink / raw)
To: Shenhar, Talel
Cc: Rob Herring, Mark Rutland, Mauro Carvalho Chehab, David Miller,
gregkh, Nicolas Ferre, Thomas Gleixner, Patrick Venture,
Linus Walleij, Olof Johansson, Maxime Ripard, Santosh Shilimkar,
paul.kocialkowski, mjourdan, Catalin Marinas, Will Deacon, DTML,
linux-kernel, Linux ARM
On Mon, Sep 9, 2019 at 1:13 PM Shenhar, Talel <talel@amazon.com> wrote:
> On 9/9/2019 12:44 PM, Arnd Bergmann wrote:
> > On Mon, Sep 9, 2019 at 11:14 AM Talel Shenhar <talel@amazon.com> wrote:
> >> + writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1);
> > Why do you require _relaxed() accessors here? Please add a comment
> > explaining that, or use the regular readl()/writel().
>
> I don't think commenting is needed here as there is nothing special in
> this type of access.
>
> I don't see this is common to comment the use of the _relaxed accessors.
I usually mention it in driver reviews, but most authors revert back
to the normal accessors when there is no difference.
> This driver is for SoC using arm64 cpu.
>
> If one uses the non-relaxed version of readl while running on arm64, he
> shall cause read barrier, which is then doing dsm(ld).. This barrier is
> not needed here, so we spare the use of the more heavy readl in favor of
> the less "harmful" one.
>
> Let me know what you think.
If the barrier causes no harm, just leave it in to keep the code more
readable. Most developers don't need to know the difference between
the two, so using the less common interface just makes the reader
curious about why it was picked.
Avoiding the barrier can make a huge performance difference in a
hot code path, but the downside is that it can behave in unexpected
ways if the same code is run on a different CPU architecture that
does not have the exact same rules about what _relaxed() means.
In fact, replacing a 'readl()' with 'readl_relaxed() + rmb()' can lead
to slower rather than faster code when the explicit barrier is heavier
than the implied one (e.g. on x86), or readl_relaxed() does not skip
the barrier.
The general rule with kernel interfaces when you have two versions
that both do what you want is to pick the one with the shorter name.
See spin_lock()/spin_lock_irqsave(), ioremap()/ioremap_nocache(),
or ktime_get()/ktime_get_clocktai_ts64(). (yes, there are also
exceptions)
Arnd
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
2019-09-09 13:41 ` Arnd Bergmann
(?)
@ 2019-09-09 14:11 ` Shenhar, Talel
-1 siblings, 0 replies; 51+ messages in thread
From: Shenhar, Talel @ 2019-09-09 14:11 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Rob Herring, Mark Rutland, Mauro Carvalho Chehab, David Miller,
gregkh, Nicolas Ferre, Thomas Gleixner, Patrick Venture,
Linus Walleij, Olof Johansson, Maxime Ripard, Santosh Shilimkar,
paul.kocialkowski, mjourdan, Catalin Marinas, Will Deacon, DTML,
linux-kernel, Linux ARM, David Woodhouse, Benjamin Herrenschmidt,
hhhawa, ronenk, jonnyc, hanochu, barakw
On 9/9/2019 4:41 PM, Arnd Bergmann wrote:
> On Mon, Sep 9, 2019 at 1:13 PM Shenhar, Talel <talel@amazon.com> wrote:
>> On 9/9/2019 12:44 PM, Arnd Bergmann wrote:
>>> On Mon, Sep 9, 2019 at 11:14 AM Talel Shenhar <talel@amazon.com> wrote:
>>>> + writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1);
>>> Why do you require _relaxed() accessors here? Please add a comment
>>> explaining that, or use the regular readl()/writel().
>> I don't think commenting is needed here as there is nothing special in
>> this type of access.
>>
>> I don't see this is common to comment the use of the _relaxed accessors.
> I usually mention it in driver reviews, but most authors revert back
> to the normal accessors when there is no difference.
>
>> This driver is for SoC using arm64 cpu.
>>
>> If one uses the non-relaxed version of readl while running on arm64, he
>> shall cause read barrier, which is then doing dsm(ld).. This barrier is
>> not needed here, so we spare the use of the more heavy readl in favor of
>> the less "harmful" one.
>>
>> Let me know what you think.
> If the barrier causes no harm, just leave it in to keep the code more
> readable. Most developers don't need to know the difference between
> the two, so using the less common interface just makes the reader
> curious about why it was picked.
>
> Avoiding the barrier can make a huge performance difference in a
> hot code path, but the downside is that it can behave in unexpected
> ways if the same code is run on a different CPU architecture that
> does not have the exact same rules about what _relaxed() means.
>
> In fact, replacing a 'readl()' with 'readl_relaxed() + rmb()' can lead
> to slower rather than faster code when the explicit barrier is heavier
> than the implied one (e.g. on x86), or readl_relaxed() does not skip
> the barrier.
>
> The general rule with kernel interfaces when you have two versions
> that both do what you want is to pick the one with the shorter name.
> See spin_lock()/spin_lock_irqsave(), ioremap()/ioremap_nocache(),
> or ktime_get()/ktime_get_clocktai_ts64(). (yes, there are also
> exceptions)
>
> Arnd
Thanks for the detailed response.
In current implementation of v1, I am not doing any read barrier, Hence,
using the non-relaxed will add unneeded memory barrier.
I have no strong objection moving to the non-relaxed version and have an
unneeded memory barrier, as this path is not "hot" one.
Beside of avoiding the unneeded memory barrier, I would be happy to keep
common behavior for our drivers:
e.g.
https://github.com/torvalds/linux/blob/master/drivers/irqchip/irq-al-fic.c#L49
So what do you think we should go with? relaxed or non-relaxed?
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
@ 2019-09-09 14:11 ` Shenhar, Talel
0 siblings, 0 replies; 51+ messages in thread
From: Shenhar, Talel @ 2019-09-09 14:11 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Mark Rutland, mjourdan, Catalin Marinas, Linus Walleij,
linux-kernel, jonnyc, Mauro Carvalho Chehab, ronenk, Will Deacon,
Benjamin Herrenschmidt, DTML, Maxime Ripard, Rob Herring,
Santosh Shilimkar, Thomas Gleixner, hanochu, Linux ARM, barakw,
hhhawa, gregkh, paul.kocialkowski, Patrick Venture,
Olof Johansson, David Miller, David Woodhouse
On 9/9/2019 4:41 PM, Arnd Bergmann wrote:
> On Mon, Sep 9, 2019 at 1:13 PM Shenhar, Talel <talel@amazon.com> wrote:
>> On 9/9/2019 12:44 PM, Arnd Bergmann wrote:
>>> On Mon, Sep 9, 2019 at 11:14 AM Talel Shenhar <talel@amazon.com> wrote:
>>>> + writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1);
>>> Why do you require _relaxed() accessors here? Please add a comment
>>> explaining that, or use the regular readl()/writel().
>> I don't think commenting is needed here as there is nothing special in
>> this type of access.
>>
>> I don't see this is common to comment the use of the _relaxed accessors.
> I usually mention it in driver reviews, but most authors revert back
> to the normal accessors when there is no difference.
>
>> This driver is for SoC using arm64 cpu.
>>
>> If one uses the non-relaxed version of readl while running on arm64, he
>> shall cause read barrier, which is then doing dsm(ld).. This barrier is
>> not needed here, so we spare the use of the more heavy readl in favor of
>> the less "harmful" one.
>>
>> Let me know what you think.
> If the barrier causes no harm, just leave it in to keep the code more
> readable. Most developers don't need to know the difference between
> the two, so using the less common interface just makes the reader
> curious about why it was picked.
>
> Avoiding the barrier can make a huge performance difference in a
> hot code path, but the downside is that it can behave in unexpected
> ways if the same code is run on a different CPU architecture that
> does not have the exact same rules about what _relaxed() means.
>
> In fact, replacing a 'readl()' with 'readl_relaxed() + rmb()' can lead
> to slower rather than faster code when the explicit barrier is heavier
> than the implied one (e.g. on x86), or readl_relaxed() does not skip
> the barrier.
>
> The general rule with kernel interfaces when you have two versions
> that both do what you want is to pick the one with the shorter name.
> See spin_lock()/spin_lock_irqsave(), ioremap()/ioremap_nocache(),
> or ktime_get()/ktime_get_clocktai_ts64(). (yes, there are also
> exceptions)
>
> Arnd
Thanks for the detailed response.
In current implementation of v1, I am not doing any read barrier, Hence,
using the non-relaxed will add unneeded memory barrier.
I have no strong objection moving to the non-relaxed version and have an
unneeded memory barrier, as this path is not "hot" one.
Beside of avoiding the unneeded memory barrier, I would be happy to keep
common behavior for our drivers:
e.g.
https://github.com/torvalds/linux/blob/master/drivers/irqchip/irq-al-fic.c#L49
So what do you think we should go with? relaxed or non-relaxed?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
@ 2019-09-09 14:11 ` Shenhar, Talel
0 siblings, 0 replies; 51+ messages in thread
From: Shenhar, Talel @ 2019-09-09 14:11 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Rob Herring, Mark Rutland, Mauro Carvalho Chehab, David Miller,
gregkh, Nicolas Ferre, Thomas Gleixner, Patrick Venture,
Linus Walleij, Olof Johansson, Maxime Ripard, Santosh Shilimkar,
paul.kocialkowski, mjourdan, Catalin Marinas, Will Deacon, DTML,
linux-kernel, Linux ARM
On 9/9/2019 4:41 PM, Arnd Bergmann wrote:
> On Mon, Sep 9, 2019 at 1:13 PM Shenhar, Talel <talel@amazon.com> wrote:
>> On 9/9/2019 12:44 PM, Arnd Bergmann wrote:
>>> On Mon, Sep 9, 2019 at 11:14 AM Talel Shenhar <talel@amazon.com> wrote:
>>>> + writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1);
>>> Why do you require _relaxed() accessors here? Please add a comment
>>> explaining that, or use the regular readl()/writel().
>> I don't think commenting is needed here as there is nothing special in
>> this type of access.
>>
>> I don't see this is common to comment the use of the _relaxed accessors.
> I usually mention it in driver reviews, but most authors revert back
> to the normal accessors when there is no difference.
>
>> This driver is for SoC using arm64 cpu.
>>
>> If one uses the non-relaxed version of readl while running on arm64, he
>> shall cause read barrier, which is then doing dsm(ld).. This barrier is
>> not needed here, so we spare the use of the more heavy readl in favor of
>> the less "harmful" one.
>>
>> Let me know what you think.
> If the barrier causes no harm, just leave it in to keep the code more
> readable. Most developers don't need to know the difference between
> the two, so using the less common interface just makes the reader
> curious about why it was picked.
>
> Avoiding the barrier can make a huge performance difference in a
> hot code path, but the downside is that it can behave in unexpected
> ways if the same code is run on a different CPU architecture that
> does not have the exact same rules about what _relaxed() means.
>
> In fact, replacing a 'readl()' with 'readl_relaxed() + rmb()' can lead
> to slower rather than faster code when the explicit barrier is heavier
> than the implied one (e.g. on x86), or readl_relaxed() does not skip
> the barrier.
>
> The general rule with kernel interfaces when you have two versions
> that both do what you want is to pick the one with the shorter name.
> See spin_lock()/spin_lock_irqsave(), ioremap()/ioremap_nocache(),
> or ktime_get()/ktime_get_clocktai_ts64(). (yes, there are also
> exceptions)
>
> Arnd
Thanks for the detailed response.
In current implementation of v1, I am not doing any read barrier, Hence,
using the non-relaxed will add unneeded memory barrier.
I have no strong objection moving to the non-relaxed version and have an
unneeded memory barrier, as this path is not "hot" one.
Beside of avoiding the unneeded memory barrier, I would be happy to keep
common behavior for our drivers:
e.g.
https://github.com/torvalds/linux/blob/master/drivers/irqchip/irq-al-fic.c#L49
So what do you think we should go with? relaxed or non-relaxed?
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
2019-09-09 14:11 ` Shenhar, Talel
(?)
@ 2019-09-09 15:16 ` Arnd Bergmann
-1 siblings, 0 replies; 51+ messages in thread
From: Arnd Bergmann @ 2019-09-09 15:16 UTC (permalink / raw)
To: Shenhar, Talel
Cc: Rob Herring, Mark Rutland, Mauro Carvalho Chehab, David Miller,
gregkh, Nicolas Ferre, Thomas Gleixner, Patrick Venture,
Linus Walleij, Olof Johansson, Maxime Ripard, Santosh Shilimkar,
paul.kocialkowski, mjourdan, Catalin Marinas, Will Deacon, DTML,
linux-kernel, Linux ARM, David Woodhouse, Benjamin Herrenschmidt,
hhhawa, ronenk, jonnyc, hanochu, barakw
On Mon, Sep 9, 2019 at 4:11 PM Shenhar, Talel <talel@amazon.com> wrote:
> On 9/9/2019 4:41 PM, Arnd Bergmann wrote:
>
> In current implementation of v1, I am not doing any read barrier, Hence,
> using the non-relaxed will add unneeded memory barrier.
>
> I have no strong objection moving to the non-relaxed version and have an
> unneeded memory barrier, as this path is not "hot" one.
Ok, then please add it.
> Beside of avoiding the unneeded memory barrier, I would be happy to keep
> common behavior for our drivers:
>
> e.g.
>
> https://github.com/torvalds/linux/blob/master/drivers/irqchip/irq-al-fic.c#L49
>
>
> So what do you think we should go with? relaxed or non-relaxed?
The al_fic_set_trigger() function is clearly a slow-path and should use the
non-relaxed functions. In case of al_fic_irq_handler(), the extra barrier
might introduce a measurable overhead, but at the same time I'm
not sure if that one is correct without the barrier:
If you have an MSI-type interrupt for notifying a device driver of
a DMA completion, there might not be any other barrier between
the arrival of the MSI message and the CPU accessing the data.
Depending on how strict the hardware implements MSI and how
the IRQ is chained, this could lead to data corruption.
If the interrupt is only used for level or edge triggered interrupts,
this is ok since you already need another register read in
the driver before it can safely access a DMA buffer.
In either case, if you can prove that it's safe to use the relaxed
version here and you think that it may help, it would be good to
add a comment explaining the reasoning.
Arnd
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
@ 2019-09-09 15:16 ` Arnd Bergmann
0 siblings, 0 replies; 51+ messages in thread
From: Arnd Bergmann @ 2019-09-09 15:16 UTC (permalink / raw)
To: Shenhar, Talel
Cc: Mark Rutland, mjourdan, Catalin Marinas, Linus Walleij,
linux-kernel, jonnyc, Mauro Carvalho Chehab, ronenk, Will Deacon,
Benjamin Herrenschmidt, DTML, Maxime Ripard, Rob Herring,
Santosh Shilimkar, Thomas Gleixner, hanochu, Linux ARM, barakw,
hhhawa, gregkh, paul.kocialkowski, Patrick Venture,
Olof Johansson, David Miller, David Woodhouse
On Mon, Sep 9, 2019 at 4:11 PM Shenhar, Talel <talel@amazon.com> wrote:
> On 9/9/2019 4:41 PM, Arnd Bergmann wrote:
>
> In current implementation of v1, I am not doing any read barrier, Hence,
> using the non-relaxed will add unneeded memory barrier.
>
> I have no strong objection moving to the non-relaxed version and have an
> unneeded memory barrier, as this path is not "hot" one.
Ok, then please add it.
> Beside of avoiding the unneeded memory barrier, I would be happy to keep
> common behavior for our drivers:
>
> e.g.
>
> https://github.com/torvalds/linux/blob/master/drivers/irqchip/irq-al-fic.c#L49
>
>
> So what do you think we should go with? relaxed or non-relaxed?
The al_fic_set_trigger() function is clearly a slow-path and should use the
non-relaxed functions. In case of al_fic_irq_handler(), the extra barrier
might introduce a measurable overhead, but at the same time I'm
not sure if that one is correct without the barrier:
If you have an MSI-type interrupt for notifying a device driver of
a DMA completion, there might not be any other barrier between
the arrival of the MSI message and the CPU accessing the data.
Depending on how strict the hardware implements MSI and how
the IRQ is chained, this could lead to data corruption.
If the interrupt is only used for level or edge triggered interrupts,
this is ok since you already need another register read in
the driver before it can safely access a DMA buffer.
In either case, if you can prove that it's safe to use the relaxed
version here and you think that it may help, it would be good to
add a comment explaining the reasoning.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
@ 2019-09-09 15:16 ` Arnd Bergmann
0 siblings, 0 replies; 51+ messages in thread
From: Arnd Bergmann @ 2019-09-09 15:16 UTC (permalink / raw)
To: Shenhar, Talel
Cc: Rob Herring, Mark Rutland, Mauro Carvalho Chehab, David Miller,
gregkh, Nicolas Ferre, Thomas Gleixner, Patrick Venture,
Linus Walleij, Olof Johansson, Maxime Ripard, Santosh Shilimkar,
paul.kocialkowski, mjourdan, Catalin Marinas, Will Deacon, DTML,
linux-kernel, Linux ARM
On Mon, Sep 9, 2019 at 4:11 PM Shenhar, Talel <talel@amazon.com> wrote:
> On 9/9/2019 4:41 PM, Arnd Bergmann wrote:
>
> In current implementation of v1, I am not doing any read barrier, Hence,
> using the non-relaxed will add unneeded memory barrier.
>
> I have no strong objection moving to the non-relaxed version and have an
> unneeded memory barrier, as this path is not "hot" one.
Ok, then please add it.
> Beside of avoiding the unneeded memory barrier, I would be happy to keep
> common behavior for our drivers:
>
> e.g.
>
> https://github.com/torvalds/linux/blob/master/drivers/irqchip/irq-al-fic.c#L49
>
>
> So what do you think we should go with? relaxed or non-relaxed?
The al_fic_set_trigger() function is clearly a slow-path and should use the
non-relaxed functions. In case of al_fic_irq_handler(), the extra barrier
might introduce a measurable overhead, but at the same time I'm
not sure if that one is correct without the barrier:
If you have an MSI-type interrupt for notifying a device driver of
a DMA completion, there might not be any other barrier between
the arrival of the MSI message and the CPU accessing the data.
Depending on how strict the hardware implements MSI and how
the IRQ is chained, this could lead to data corruption.
If the interrupt is only used for level or edge triggered interrupts,
this is ok since you already need another register read in
the driver before it can safely access a DMA buffer.
In either case, if you can prove that it's safe to use the relaxed
version here and you think that it may help, it would be good to
add a comment explaining the reasoning.
Arnd
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
2019-09-09 15:16 ` Arnd Bergmann
(?)
@ 2019-09-10 6:21 ` Shenhar, Talel
-1 siblings, 0 replies; 51+ messages in thread
From: Shenhar, Talel @ 2019-09-10 6:21 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Rob Herring, Mark Rutland, Mauro Carvalho Chehab, David Miller,
gregkh, Nicolas Ferre, Thomas Gleixner, Patrick Venture,
Linus Walleij, Olof Johansson, Maxime Ripard, Santosh Shilimkar,
paul.kocialkowski, mjourdan, Catalin Marinas, Will Deacon, DTML,
linux-kernel, Linux ARM, David Woodhouse, Benjamin Herrenschmidt,
hhhawa, ronenk, jonnyc, hanochu, barakw
On 9/9/2019 6:16 PM, Arnd Bergmann wrote:
> On Mon, Sep 9, 2019 at 4:11 PM Shenhar, Talel <talel@amazon.com> wrote:
>> On 9/9/2019 4:41 PM, Arnd Bergmann wrote:
>>
>> In current implementation of v1, I am not doing any read barrier, Hence,
>> using the non-relaxed will add unneeded memory barrier.
>>
>> I have no strong objection moving to the non-relaxed version and have an
>> unneeded memory barrier, as this path is not "hot" one.
> Ok, then please add it.
ok, shall be part of v2
>
>> Beside of avoiding the unneeded memory barrier, I would be happy to keep
>> common behavior for our drivers:
>>
>> e.g.
>>
>> https://github.com/torvalds/linux/blob/master/drivers/irqchip/irq-al-fic.c#L49
>>
>>
>> So what do you think we should go with? relaxed or non-relaxed?
> The al_fic_set_trigger() function is clearly a slow-path and should use the
> non-relaxed functions. In case of al_fic_irq_handler(), the extra barrier
> might introduce a measurable overhead, but at the same time I'm
> not sure if that one is correct without the barrier:
>
> If you have an MSI-type interrupt for notifying a device driver of
> a DMA completion, there might not be any other barrier between
> the arrival of the MSI message and the CPU accessing the data.
> Depending on how strict the hardware implements MSI and how
> the IRQ is chained, this could lead to data corruption.
>
> If the interrupt is only used for level or edge triggered interrupts,
> this is ok since you already need another register read in
> the driver before it can safely access a DMA buffer.
>
> In either case, if you can prove that it's safe to use the relaxed
> version here and you think that it may help, it would be good to
> add a comment explaining the reasoning.
Decided to go with the non-relaxed version as this is not hot path and
likely be more clear to the common reader to have non relaxed version.
>
> Arnd
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
@ 2019-09-10 6:21 ` Shenhar, Talel
0 siblings, 0 replies; 51+ messages in thread
From: Shenhar, Talel @ 2019-09-10 6:21 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Mark Rutland, mjourdan, Catalin Marinas, Linus Walleij,
linux-kernel, jonnyc, Mauro Carvalho Chehab, ronenk, Will Deacon,
Benjamin Herrenschmidt, DTML, Maxime Ripard, Rob Herring,
Santosh Shilimkar, Thomas Gleixner, hanochu, Linux ARM, barakw,
hhhawa, gregkh, paul.kocialkowski, Patrick Venture,
Olof Johansson, David Miller, David Woodhouse
On 9/9/2019 6:16 PM, Arnd Bergmann wrote:
> On Mon, Sep 9, 2019 at 4:11 PM Shenhar, Talel <talel@amazon.com> wrote:
>> On 9/9/2019 4:41 PM, Arnd Bergmann wrote:
>>
>> In current implementation of v1, I am not doing any read barrier, Hence,
>> using the non-relaxed will add unneeded memory barrier.
>>
>> I have no strong objection moving to the non-relaxed version and have an
>> unneeded memory barrier, as this path is not "hot" one.
> Ok, then please add it.
ok, shall be part of v2
>
>> Beside of avoiding the unneeded memory barrier, I would be happy to keep
>> common behavior for our drivers:
>>
>> e.g.
>>
>> https://github.com/torvalds/linux/blob/master/drivers/irqchip/irq-al-fic.c#L49
>>
>>
>> So what do you think we should go with? relaxed or non-relaxed?
> The al_fic_set_trigger() function is clearly a slow-path and should use the
> non-relaxed functions. In case of al_fic_irq_handler(), the extra barrier
> might introduce a measurable overhead, but at the same time I'm
> not sure if that one is correct without the barrier:
>
> If you have an MSI-type interrupt for notifying a device driver of
> a DMA completion, there might not be any other barrier between
> the arrival of the MSI message and the CPU accessing the data.
> Depending on how strict the hardware implements MSI and how
> the IRQ is chained, this could lead to data corruption.
>
> If the interrupt is only used for level or edge triggered interrupts,
> this is ok since you already need another register read in
> the driver before it can safely access a DMA buffer.
>
> In either case, if you can prove that it's safe to use the relaxed
> version here and you think that it may help, it would be good to
> add a comment explaining the reasoning.
Decided to go with the non-relaxed version as this is not hot path and
likely be more clear to the common reader to have non relaxed version.
>
> Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
@ 2019-09-10 6:21 ` Shenhar, Talel
0 siblings, 0 replies; 51+ messages in thread
From: Shenhar, Talel @ 2019-09-10 6:21 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Rob Herring, Mark Rutland, Mauro Carvalho Chehab, David Miller,
gregkh, Nicolas Ferre, Thomas Gleixner, Patrick Venture,
Linus Walleij, Olof Johansson, Maxime Ripard, Santosh Shilimkar,
paul.kocialkowski, mjourdan, Catalin Marinas, Will Deacon, DTML,
linux-kernel, Linux ARM
On 9/9/2019 6:16 PM, Arnd Bergmann wrote:
> On Mon, Sep 9, 2019 at 4:11 PM Shenhar, Talel <talel@amazon.com> wrote:
>> On 9/9/2019 4:41 PM, Arnd Bergmann wrote:
>>
>> In current implementation of v1, I am not doing any read barrier, Hence,
>> using the non-relaxed will add unneeded memory barrier.
>>
>> I have no strong objection moving to the non-relaxed version and have an
>> unneeded memory barrier, as this path is not "hot" one.
> Ok, then please add it.
ok, shall be part of v2
>
>> Beside of avoiding the unneeded memory barrier, I would be happy to keep
>> common behavior for our drivers:
>>
>> e.g.
>>
>> https://github.com/torvalds/linux/blob/master/drivers/irqchip/irq-al-fic.c#L49
>>
>>
>> So what do you think we should go with? relaxed or non-relaxed?
> The al_fic_set_trigger() function is clearly a slow-path and should use the
> non-relaxed functions. In case of al_fic_irq_handler(), the extra barrier
> might introduce a measurable overhead, but at the same time I'm
> not sure if that one is correct without the barrier:
>
> If you have an MSI-type interrupt for notifying a device driver of
> a DMA completion, there might not be any other barrier between
> the arrival of the MSI message and the CPU accessing the data.
> Depending on how strict the hardware implements MSI and how
> the IRQ is chained, this could lead to data corruption.
>
> If the interrupt is only used for level or edge triggered interrupts,
> this is ok since you already need another register read in
> the driver before it can safely access a DMA buffer.
>
> In either case, if you can prove that it's safe to use the relaxed
> version here and you think that it may help, it would be good to
> add a comment explaining the reasoning.
Decided to go with the non-relaxed version as this is not hot path and
likely be more clear to the common reader to have non relaxed version.
>
> Arnd
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
2019-09-09 9:10 ` Talel Shenhar
(?)
@ 2019-09-09 11:51 ` kbuild test robot
-1 siblings, 0 replies; 51+ messages in thread
From: kbuild test robot @ 2019-09-09 11:51 UTC (permalink / raw)
To: Talel Shenhar
Cc: kbuild-all, robh+dt, mark.rutland, mchehab+samsung, davem,
gregkh, nicolas.ferre, tglx, arnd, venture, linus.walleij, olof,
mripard, ssantosh, paul.kocialkowski, mjourdan, catalin.marinas,
will, talel, devicetree, linux-kernel, linux-arm-kernel, dwmw,
benh, hhhawa, ronenk, jonnyc, hanochu, barakw
[-- Attachment #1: Type: text/plain, Size: 2407 bytes --]
Hi Talel,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[cannot apply to v5.3-rc8 next-20190904]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Talel-Shenhar/Amazon-s-Annapurna-Labs-POS-Driver/20190909-180243
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/soc/amazon/al_pos.c: In function 'al_pos_irq_handler':
>> drivers/soc/amazon/al_pos.c:56:57: warning: left shift count >= width of type [-Wshift-count-overflow]
addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32);
^~
vim +56 drivers/soc/amazon/al_pos.c
37
38 static irqreturn_t al_pos_irq_handler(int irq, void *info)
39 {
40 struct platform_device *pdev = info;
41 struct al_pos *pos = platform_get_drvdata(pdev);
42 u32 log1;
43 u32 log0;
44 u64 addr;
45 u16 request_id;
46 u8 bresp;
47
48 log1 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_1);
49 if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1))
50 return IRQ_NONE;
51
52 log0 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_0);
53 writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1);
54
55 addr = FIELD_GET(AL_POS_ERROR_LOG_0_ADDR_LOW, log0);
> 56 addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32);
57 request_id = FIELD_GET(AL_POS_ERROR_LOG_1_REQUEST_ID, log1);
58 bresp = FIELD_GET(AL_POS_ERROR_LOG_1_BRESP, log1);
59
60 dev_err(&pdev->dev, "addr=0x%llx request_id=0x%x bresp=0x%x\n",
61 addr, request_id, bresp);
62
63 if (al_pos_panic)
64 panic("POS");
65
66 return IRQ_HANDLED;
67 }
68
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50923 bytes --]
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
@ 2019-09-09 11:51 ` kbuild test robot
0 siblings, 0 replies; 51+ messages in thread
From: kbuild test robot @ 2019-09-09 11:51 UTC (permalink / raw)
To: Talel Shenhar
Cc: mark.rutland, mjourdan, catalin.marinas, linus.walleij,
linux-kernel, jonnyc, mchehab+samsung, ronenk, will, hanochu,
benh, devicetree, arnd, mripard, robh+dt, ssantosh, tglx, talel,
linux-arm-kernel, barakw, hhhawa, gregkh, paul.kocialkowski,
kbuild-all, venture, olof, davem, dwmw
[-- Attachment #1: Type: text/plain, Size: 2407 bytes --]
Hi Talel,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[cannot apply to v5.3-rc8 next-20190904]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Talel-Shenhar/Amazon-s-Annapurna-Labs-POS-Driver/20190909-180243
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/soc/amazon/al_pos.c: In function 'al_pos_irq_handler':
>> drivers/soc/amazon/al_pos.c:56:57: warning: left shift count >= width of type [-Wshift-count-overflow]
addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32);
^~
vim +56 drivers/soc/amazon/al_pos.c
37
38 static irqreturn_t al_pos_irq_handler(int irq, void *info)
39 {
40 struct platform_device *pdev = info;
41 struct al_pos *pos = platform_get_drvdata(pdev);
42 u32 log1;
43 u32 log0;
44 u64 addr;
45 u16 request_id;
46 u8 bresp;
47
48 log1 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_1);
49 if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1))
50 return IRQ_NONE;
51
52 log0 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_0);
53 writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1);
54
55 addr = FIELD_GET(AL_POS_ERROR_LOG_0_ADDR_LOW, log0);
> 56 addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32);
57 request_id = FIELD_GET(AL_POS_ERROR_LOG_1_REQUEST_ID, log1);
58 bresp = FIELD_GET(AL_POS_ERROR_LOG_1_BRESP, log1);
59
60 dev_err(&pdev->dev, "addr=0x%llx request_id=0x%x bresp=0x%x\n",
61 addr, request_id, bresp);
62
63 if (al_pos_panic)
64 panic("POS");
65
66 return IRQ_HANDLED;
67 }
68
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50923 bytes --]
[-- Attachment #3: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
@ 2019-09-09 11:51 ` kbuild test robot
0 siblings, 0 replies; 51+ messages in thread
From: kbuild test robot @ 2019-09-09 11:51 UTC (permalink / raw)
Cc: mark.rutland, mjourdan, catalin.marinas, linus.walleij,
linux-kernel, jonnyc, mchehab+samsung, ronenk, will, hanochu,
benh, devicetree, arnd, mripard, robh+dt, ssantosh, tglx, talel,
linux-arm-kernel, barakw, hhhawa, gregkh, paul.kocialkowski,
kbuild-all, venture, olof, davem, dwmw
[-- Attachment #1: Type: text/plain, Size: 2407 bytes --]
Hi Talel,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[cannot apply to v5.3-rc8 next-20190904]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Talel-Shenhar/Amazon-s-Annapurna-Labs-POS-Driver/20190909-180243
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/soc/amazon/al_pos.c: In function 'al_pos_irq_handler':
>> drivers/soc/amazon/al_pos.c:56:57: warning: left shift count >= width of type [-Wshift-count-overflow]
addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32);
^~
vim +56 drivers/soc/amazon/al_pos.c
37
38 static irqreturn_t al_pos_irq_handler(int irq, void *info)
39 {
40 struct platform_device *pdev = info;
41 struct al_pos *pos = platform_get_drvdata(pdev);
42 u32 log1;
43 u32 log0;
44 u64 addr;
45 u16 request_id;
46 u8 bresp;
47
48 log1 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_1);
49 if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1))
50 return IRQ_NONE;
51
52 log0 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_0);
53 writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1);
54
55 addr = FIELD_GET(AL_POS_ERROR_LOG_0_ADDR_LOW, log0);
> 56 addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32);
57 request_id = FIELD_GET(AL_POS_ERROR_LOG_1_REQUEST_ID, log1);
58 bresp = FIELD_GET(AL_POS_ERROR_LOG_1_BRESP, log1);
59
60 dev_err(&pdev->dev, "addr=0x%llx request_id=0x%x bresp=0x%x\n",
61 addr, request_id, bresp);
62
63 if (al_pos_panic)
64 panic("POS");
65
66 return IRQ_HANDLED;
67 }
68
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50923 bytes --]
[-- Attachment #3: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 51+ messages in thread