[PATCH 0/3] Amazon's Annapurna Labs POS Driver

[PATCH 0/3] Amazon's Annapurna Labs POS Driver

[Talel Shenhar]

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.


Talel Shenhar (3):
  dt-bindings: soc: al-pos: Amazon's Annapurna Labs POS
  soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
  arm64: alpine: select AL_POS

 .../bindings/soc/amazon/amazon,al-pos.txt          |  18 +++
 MAINTAINERS                                        |   6 +
 arch/arm64/Kconfig.platforms                       |   1 +
 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 +++++++++++++++++++++
 8 files changed, 162 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

[PATCH 1/3] dt-bindings: soc: al-pos: Amazon's Annapurna Labs POS

[Talel Shenhar]

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

[PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver

[Talel Shenhar]

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

[PATCH 3/3] arm64: alpine: select AL_POS

[Talel Shenhar]

Amazon's Annapurna Labs SoCs uses al-pos driver, enable it.

Signed-off-by: Talel Shenhar <talel@amazon.com>
---
 arch/arm64/Kconfig.platforms | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 4778c77..bd86b15 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -25,6 +25,7 @@ config ARCH_SUNXI
 config ARCH_ALPINE
 	bool "Annapurna Labs Alpine platform"
 	select ALPINE_MSI if PCI
+	select AL_POS
 	help
 	  This enables support for the Annapurna Labs Alpine
 	  Soc family.
-- 
2.7.4

Re: [PATCH 3/3] arm64: alpine: select AL_POS

[Arnd Bergmann]

On Mon, Sep 9, 2019 at 11:14 AM Talel Shenhar <talel@amazon.com> wrote:
>
> Amazon's Annapurna Labs SoCs uses al-pos driver, enable it.
>
> Signed-off-by: Talel Shenhar <talel@amazon.com>
> ---
>  arch/arm64/Kconfig.platforms | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> index 4778c77..bd86b15 100644
> --- a/arch/arm64/Kconfig.platforms
> +++ b/arch/arm64/Kconfig.platforms
> @@ -25,6 +25,7 @@ config ARCH_SUNXI
>  config ARCH_ALPINE
>         bool "Annapurna Labs Alpine platform"
>         select ALPINE_MSI if PCI
> +       select AL_POS
>         help
>           This enables support for the Annapurna Labs Alpine
>           Soc family.

Generally I think this kind of thing should go into the defconfig
rather than being hard-selected. There might be users that
want to not enable the driver.

       Arnd

Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver

[Arnd Bergmann]

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

Re: [PATCH 3/3] arm64: alpine: select AL_POS

[Shenhar, Talel]

On 9/9/2019 12:40 PM, Arnd Bergmann wrote:
> On Mon, Sep 9, 2019 at 11:14 AM Talel Shenhar <talel@amazon.com> wrote:
>> Amazon's Annapurna Labs SoCs uses al-pos driver, enable it.
>>
>> Signed-off-by: Talel Shenhar <talel@amazon.com>
>> ---
>>   arch/arm64/Kconfig.platforms | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
>> index 4778c77..bd86b15 100644
>> --- a/arch/arm64/Kconfig.platforms
>> +++ b/arch/arm64/Kconfig.platforms
>> @@ -25,6 +25,7 @@ config ARCH_SUNXI
>>   config ARCH_ALPINE
>>          bool "Annapurna Labs Alpine platform"
>>          select ALPINE_MSI if PCI
>> +       select AL_POS
>>          help
>>            This enables support for the Annapurna Labs Alpine
>>            Soc family.
> Generally I think this kind of thing should go into the defconfig
> rather than being hard-selected. There might be users that
> want to not enable the driver.
>
>         Arnd

The reason for selecting it is because this is a driver that we will 
always want for ARCH_ALPINE.

Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver

[Shenhar, Talel]

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

Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver

[kbuild test robot]

[-- 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 --]

Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver

[Arnd Bergmann]

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

Re: [PATCH 3/3] arm64: alpine: select AL_POS

[Arnd Bergmann]

On Mon, Sep 9, 2019 at 12:17 PM Shenhar, Talel <talel@amazon.com> wrote:
> On 9/9/2019 12:40 PM, Arnd Bergmann wrote:
> > On Mon, Sep 9, 2019 at 11:14 AM Talel Shenhar <talel@amazon.com> wrote:
> >> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> >> index 4778c77..bd86b15 100644
> >> --- a/arch/arm64/Kconfig.platforms
> >> +++ b/arch/arm64/Kconfig.platforms
> >> @@ -25,6 +25,7 @@ config ARCH_SUNXI
> >>   config ARCH_ALPINE
> >>          bool "Annapurna Labs Alpine platform"
> >>          select ALPINE_MSI if PCI
> >> +       select AL_POS
> >>          help
> >>            This enables support for the Annapurna Labs Alpine
> >>            Soc family.
> > Generally I think this kind of thing should go into the defconfig
> > rather than being hard-selected. There might be users that
> > want to not enable the driver.
>
> The reason for selecting it is because this is a driver that we will
> always want for ARCH_ALPINE.

Can you put the exact requirement (other than "we want this")
in the changelog text then? It's still not clear to me what breaks
without this driver.

        Arnd

Re: [PATCH 3/3] arm64: alpine: select AL_POS

[Shenhar, Talel]

On 9/9/2019 4:45 PM, Arnd Bergmann wrote:
> On Mon, Sep 9, 2019 at 12:17 PM Shenhar, Talel <talel@amazon.com> wrote:
>> On 9/9/2019 12:40 PM, Arnd Bergmann wrote:
>>> On Mon, Sep 9, 2019 at 11:14 AM Talel Shenhar <talel@amazon.com> wrote:
>>>> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
>>>> index 4778c77..bd86b15 100644
>>>> --- a/arch/arm64/Kconfig.platforms
>>>> +++ b/arch/arm64/Kconfig.platforms
>>>> @@ -25,6 +25,7 @@ config ARCH_SUNXI
>>>>    config ARCH_ALPINE
>>>>           bool "Annapurna Labs Alpine platform"
>>>>           select ALPINE_MSI if PCI
>>>> +       select AL_POS
>>>>           help
>>>>             This enables support for the Annapurna Labs Alpine
>>>>             Soc family.
>>> Generally I think this kind of thing should go into the defconfig
>>> rather than being hard-selected. There might be users that
>>> want to not enable the driver.
>> The reason for selecting it is because this is a driver that we will
>> always want for ARCH_ALPINE.
> Can you put the exact requirement (other than "we want this")
> in the changelog text then? It's still not clear to me what breaks
> without this driver.
>
>          Arnd

Its not that something will get broken. its error event detector for POS 
events which allows seeing bad accesses to registers.

What is the general rule of which configs to put under select and which 
under defconfig?

I was thinking that "general" SoC support is good under select - those 
things that we always want.

And specific features, e.g. RAID support or features that supported only 
on specific HW shall go under defconfig.


Similar, I see ARCH_LAYERSCAPE selecting EDAC_SUPPORT.


Will love to hear the general rule for select vs defconfig.

Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver

[Shenhar, Talel]

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?

Re: [PATCH 3/3] arm64: alpine: select AL_POS

[Arnd Bergmann]

On Mon, Sep 9, 2019 at 3:59 PM Shenhar, Talel <talel@amazon.com> wrote:
> On 9/9/2019 4:45 PM, Arnd Bergmann wrote:
>
> Its not that something will get broken. its error event detector for POS
> events which allows seeing bad accesses to registers.
>
> What is the general rule of which configs to put under select and which
> under defconfig?
>
> I was thinking that "general" SoC support is good under select - those
> things that we always want.

I generally want as little as possible to be selected, basically only
things that are required for linking the kernel and booting it without
potentially destroying the hardware.

In particular, I want most drivers to be enabled as loadable modules
if possible. When you have general-purpose distributions support
your platform, there is no need to have this module built-in while
running on a different chip, even if you always want to load the
module when it's running on yours.

> And specific features, e.g. RAID support or features that supported only
> on specific HW shall go under defconfig.
>
> Similar, I see ARCH_LAYERSCAPE selecting EDAC_SUPPORT.

I think this was done to avoid a link failure. It's also possible that this
is a mistake and just did not get caught in review.

       Arnd

Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver

[Arnd Bergmann]

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

Re: [PATCH 3/3] arm64: alpine: select AL_POS

[Shenhar, Talel]

On 9/9/2019 6:08 PM, Arnd Bergmann wrote:
> On Mon, Sep 9, 2019 at 3:59 PM Shenhar, Talel <talel@amazon.com> wrote:
>> On 9/9/2019 4:45 PM, Arnd Bergmann wrote:
>>
>> Its not that something will get broken. its error event detector for POS
>> events which allows seeing bad accesses to registers.
>>
>> What is the general rule of which configs to put under select and which
>> under defconfig?
>>
>> I was thinking that "general" SoC support is good under select - those
>> things that we always want.
> I generally want as little as possible to be selected, basically only
> things that are required for linking the kernel and booting it without
> potentially destroying the hardware.
>
> In particular, I want most drivers to be enabled as loadable modules
> if possible. When you have general-purpose distributions support
> your platform, there is no need to have this module built-in while
> running on a different chip, even if you always want to load the
> module when it's running on yours.
>
>> And specific features, e.g. RAID support or features that supported only
>> on specific HW shall go under defconfig.
>>
>> Similar, I see ARCH_LAYERSCAPE selecting EDAC_SUPPORT.
> I think this was done to avoid a link failure. It's also possible that this
> is a mistake and just did not get caught in review.
>
>         Arnd


I see.

Will remove this from v2.

Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver

[Shenhar, Talel]

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