[{"id":1760777,"web_url":"http://patchwork.ozlabs.org/comment/1760777/","msgid":"<1b062d84-7335-8553-39c7-2e60973b4396@arm.com>","list_archive_url":null,"date":"2017-08-31T09:28:01","subject":"Re: [PATCH 05/13] irqchip: add initial support for ompic","submitter":{"id":7353,"url":"http://patchwork.ozlabs.org/api/people/7353/","name":"Marc Zyngier","email":"marc.zyngier@arm.com"},"content":"On 30/08/17 22:58, Stafford Horne wrote:\n> From: Stefan Kristiansson \n> \n> IPI driver for OpenRISC Multicore programmable interrupt controller as\n> described in the Multicore support section of the OpenRISC 1.2\n> proposed architecture specification:\n> \n> https://github.com/stffrdhrn/doc/raw/arch-1.2-proposal/openrisc-arch-1.2-rev0.pdf\n> \n> Each OpenRISC core contains a full interrupt controller which is used in\n> the SMP architecture for interrupt balancing. This IPI device is the\n> only external device required for enabling SMP on OpenRISC.\n> \n> Pending ops are stored in a memory bit mask which can allow multiple\n> pending operations to be set and serviced at a time. This is mostly\n> borrowed from the alpha IPI implementation.\n> \n> Signed-off-by: Stefan Kristiansson \n> [shorne@gmail.com: converted ops to bitmask, wrote commit message]\n> Signed-off-by: Stafford Horne \n> ---\n> .../bindings/interrupt-controller/ompic.txt | 22 ++++\n> arch/openrisc/Kconfig | 1 +\n> drivers/irqchip/Kconfig | 4 +\n> drivers/irqchip/Makefile | 1 +\n> drivers/irqchip/irq-ompic.c | 117 +++++++++++++++++++++\n> 5 files changed, 145 insertions(+)\n> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ompic.txt\n> create mode 100644 drivers/irqchip/irq-ompic.c\n> \n> diff --git a/Documentation/devicetree/bindings/interrupt-controller/ompic.txt b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt\n> new file mode 100644\n> index 000000000000..4176ecc3366d\n> --- /dev/null\n> +++ b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt\n> @@ -0,0 +1,22 @@\n> +OpenRISC Multicore Programmable Interrupt Controller\n> +\n> +Required properties:\n> +\n> +- compatible : This should be \"ompic\"\n> +- reg : Specifies base physical address and size of the register space. The\n> + size can be arbitrary based on the number of cores the controller has\n> + been configured to handle, typically 8 bytes per core.\n> +- interrupt-controller : Identifies the node as an interrupt controller\n> +- #interrupt-cells : Specifies the number of cells needed to encode an\n> + interrupt source. The value shall be 1.\n> +- interrupts : Specifies the interrupt line to which the ompic is wired.\n> +\n> +Example:\n> +\n> +ompic: ompic {\n> +\tcompatible = \"ompic\";\n> +\treg = <0x98000000 16>;\n> +\t#interrupt-cells = <1>;\n> +\tinterrupt-controller;\n> +\tinterrupts = <1>;\n> +};\n> diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig\n> index 214c837ce597..dd7e55e7e42d 100644\n> --- a/arch/openrisc/Kconfig\n> +++ b/arch/openrisc/Kconfig\n> @@ -30,6 +30,7 @@ config OPENRISC\n> \tselect NO_BOOTMEM\n> \tselect ARCH_USE_QUEUED_SPINLOCKS\n> \tselect ARCH_USE_QUEUED_RWLOCKS\n> +\tselect OMPIC if SMP\n> \n> config CPU_BIG_ENDIAN\n> \tdef_bool y\n> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig\n> index f1fd5f44d1d4..3fa60e6667a7 100644\n> --- a/drivers/irqchip/Kconfig\n> +++ b/drivers/irqchip/Kconfig\n> @@ -145,6 +145,10 @@ config CLPS711X_IRQCHIP\n> \tselect SPARSE_IRQ\n> \tdefault y\n> \n> +config OMPIC\n> +\tbool\n> +\tselect IRQ_DOMAIN\n\nWhy do you need to select IRQ_DOMAIN? This driver doesn't seem to use any...\n\n> +\n> config OR1K_PIC\n> \tbool\n> \tselect IRQ_DOMAIN\n> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile\n> index e88d856cc09c..123047d7a20d 100644\n> --- a/drivers/irqchip/Makefile\n> +++ b/drivers/irqchip/Makefile\n> @@ -17,6 +17,7 @@ obj-$(CONFIG_DW_APB_ICTL)\t\t+= irq-dw-apb-ictl.o\n> obj-$(CONFIG_METAG)\t\t\t+= irq-metag-ext.o\n> obj-$(CONFIG_METAG_PERFCOUNTER_IRQS)\t+= irq-metag.o\n> obj-$(CONFIG_CLPS711X_IRQCHIP)\t\t+= irq-clps711x.o\n> +obj-$(CONFIG_OMPIC)\t\t\t+= irq-ompic.o\n> obj-$(CONFIG_OR1K_PIC)\t\t\t+= irq-or1k-pic.o\n> obj-$(CONFIG_ORION_IRQCHIP)\t\t+= irq-orion.o\n> obj-$(CONFIG_OMAP_IRQCHIP)\t\t+= irq-omap-intc.o\n> diff --git a/drivers/irqchip/irq-ompic.c b/drivers/irqchip/irq-ompic.c\n> new file mode 100644\n> index 000000000000..438819f8a5a7\n> --- /dev/null\n> +++ b/drivers/irqchip/irq-ompic.c\n> @@ -0,0 +1,117 @@\n> +/*\n> + * Open Multi-Processor Interrupt Controller driver\n> + *\n> + * Copyright (C) 2014 Stefan Kristiansson \n> + *\n> + * This file is licensed under the terms of the GNU General Public License\n> + * version 2. This program is licensed \"as is\" without any warranty of any\n> + * kind, whether express or implied.\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n\nDon't think you need this.\n\n> +#include \n\nNor this.\n\n> +\n> +#include \n> +\n> +#define OMPIC_IPI_BASE\t\t\t0x0\n> +#define OMPIC_IPI_CTRL(cpu)\t\t(OMPIC_IPI_BASE + 0x0 + (cpu)*8)\n> +#define OMPIC_IPI_STAT(cpu)\t\t(OMPIC_IPI_BASE + 0x4 + (cpu)*8)\n\nIn the DT binding you say that \"size can be arbitrary based on the\nnumber of cores the controller has been configured to handle, typically\n8 bytes per core\". Here, this is 8 bytes, always, which is not exactly\nthe same. What is the architectural value, if any? If there is none,\nthen the per-core size should either come from DT or some other mean\n(register?).\n\n> +\n> +#define OMPIC_IPI_CTRL_IRQ_ACK\t\t(1 << 31)\n> +#define OMPIC_IPI_CTRL_IRQ_GEN\t\t(1 << 30)\n> +#define OMPIC_IPI_CTRL_DST(cpu)\t\t(((cpu) & 0x3fff) << 16)\n> +\n> +#define OMPIC_IPI_STAT_IRQ_PENDING\t(1 << 30)\n> +\n> +#define OMPIC_IPI_DATA(x)\t\t((x) & 0xffff)\n> +\n> +static struct {\n> +\tunsigned long ops;\n> +} ipi_data[NR_CPUS];\n\nIn general, a per cpu data structure is better expressed as a percpu\ndata structure (yes, I'm in a funny mood this morning). Otherwise,\nyou're going to thrash more than just the receiver and the sender, but\nalso all the other CPUs that have their data in the same cache line.\n\n> +\n> +static void __iomem *ompic_base;\n> +\n> +static inline u32 ompic_readreg(void __iomem *base, loff_t offset)\n> +{\n> +\treturn ioread32be(base + offset);\n> +}\n> +\n> +static void ompic_writereg(void __iomem *base, loff_t offset, u32 data)\n> +{\n> +\tiowrite32be(data, base + offset);\n> +}\n> +\n> +#ifdef CONFIG_SMP\n\nThis code is only selected when CONFIG_SMP=y.\n\n> +void ompic_raise_softirq(const struct cpumask *mask, unsigned int irq)\n> +{\n\nWhat is \"irq\" here? How is it guaranteed to fit in an unsigned long?\n\n> +\tunsigned int dst_cpu;\n> +\tunsigned int src_cpu = smp_processor_id();\n> +\n> +\tfor_each_cpu(dst_cpu, mask) {\n> +\t\tset_bit(irq, &ipi_data[dst_cpu].ops);\n> +\n> +\t\tompic_writereg(ompic_base, OMPIC_IPI_CTRL(src_cpu),\n> +\t\t\t OMPIC_IPI_CTRL_IRQ_GEN |\n> +\t\t\t OMPIC_IPI_CTRL_DST(dst_cpu) |\n> +\t\t\t OMPIC_IPI_DATA(1));\n\nWhat guarantees that the action of set_bit can be observed by the target\nCPU? set-bit gives you atomicity, but no barrier.\n\n> +\t}\n> +}\n> +#endif\n> +\n> +irqreturn_t ompic_ipi_handler(int irq, void *dev_id)\n> +{\n> +\tunsigned int cpu = smp_processor_id();\n> +\tunsigned long *pending_ops = &ipi_data[cpu].ops;\n> +\tunsigned long ops;\n> +\n> +\tompic_writereg(ompic_base, OMPIC_IPI_CTRL(cpu), OMPIC_IPI_CTRL_IRQ_ACK);\n> +\twhile ((ops = xchg(pending_ops, 0)) != 0) {\n\nBarrier again?\n\n> +\t\tdo {\n> +\t\t\tunsigned long ipi;\n> +\n> +\t\t\tipi = ops & -ops;\n> +\t\t\tops &= ~ipi;\n> +\t\t\tipi = __ffs(ipi);\n\nThis feels pointlessly convoluted. Is there any reason why you can't\nwrite it as:\n\n\t\t\tipi = __ffs(ops);\n\t\t\tops &= ~(1UL << ipi);\n\nwhich feels like a much more common idiom?\n\n> +\n> +\t\t\thandle_IPI(ipi);\n> +\t\t} while (ops);\n> +\t}\n> +\n> +\treturn IRQ_HANDLED;\n> +}\n> +\n> +static struct irqaction ompi_ipi_irqaction = {\n> +\t.handler = ompic_ipi_handler,\n> +\t.flags = IRQF_PERCPU,\n> +\t.name = \"ompic_ipi\",\n> +};\n> +\n> +#ifdef CONFIG_OF\n\nThis code is useless if you don't have CONFIG_OF. So either you make the\ndriver depend on CONFIG_OF, or you actively select it. And given that\nCONFIG_OPENRISC already selects CONFIG_OF, you can happily get rid of\nall the #ifdefery here.\n\n> +int __init ompic_of_init(struct device_node *node, struct device_node *parent)\n> +{\n> +\tint irq;\n> +\n> +\tif (WARN_ON(!node))\n> +\t\treturn -ENODEV;\n\nHow do you end-up here if node == NULL?\n\n> +\n> +\tmemset(ipi_data, 0, sizeof(ipi_data));\n\nThe kernel should do this for you already.\n\n> +\n> +\tompic_base = of_iomap(node, 0);\n> +\n> +\tirq = irq_of_parse_and_map(node, 0);\n> +\tsetup_irq(irq, &ompi_ipi_irqaction);\n> +\n> +#ifdef CONFIG_SMP\n> +\tset_smp_cross_call(ompic_raise_softirq);\n> +#endif\n> +\n> +\treturn 0;\n> +}\n> +IRQCHIP_DECLARE(ompic, \"ompic\", ompic_of_init);\n> +#endif\n> \n\nThanks,\n\n\tM.","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xjcVf3HWDz9sRW\n\tfor ;\n\tThu, 31 Aug 2017 19:28:10 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751023AbdHaJ2J (ORCPT\n\t);\n\tThu, 31 Aug 2017 05:28:09 -0400","from foss.arm.com ([217.140.101.70]:53040 \"EHLO foss.arm.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1750968AbdHaJ2I (ORCPT );\n\tThu, 31 Aug 2017 05:28:08 -0400","from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249])\n\tby usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id A942515A2;\n\tThu, 31 Aug 2017 02:28:07 -0700 (PDT)","from [10.1.207.16] (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249])\n\tby usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id\n\tAE9543F540; Thu, 31 Aug 2017 02:28:03 -0700 (PDT)"],"Subject":"Re: [PATCH 05/13] irqchip: add initial support for ompic","To":"Stafford Horne , LKML ","Cc":"Openrisc ,\n\tStefan Kristiansson ,\n\tThomas Gleixner ,\n\tJason Cooper , \n\tRob Herring , Mark Rutland ,\n\tJonas Bonn , devicetree@vger.kernel.org","References":"\n\t<538699c64b5601e8800b77da29f7951bf23f57ce.1504129273.git.shorne@gmail.com>","From":"Marc Zyngier ","Organization":"ARM Ltd","Message-ID":"<1b062d84-7335-8553-39c7-2e60973b4396@arm.com>","Date":"Thu, 31 Aug 2017 10:28:01 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<538699c64b5601e8800b77da29f7951bf23f57ce.1504129273.git.shorne@gmail.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1760830,"web_url":"http://patchwork.ozlabs.org/comment/1760830/","msgid":"<20170831105940.GE15031@leverpostej>","list_archive_url":null,"date":"2017-08-31T10:59:40","subject":"Re: [PATCH 05/13] irqchip: add initial support for ompic","submitter":{"id":8806,"url":"http://patchwork.ozlabs.org/api/people/8806/","name":"Mark Rutland","email":"mark.rutland@arm.com"},"content":"On Thu, Aug 31, 2017 at 06:58:36AM +0900, Stafford Horne wrote:\n> From: Stefan Kristiansson \n> \n> IPI driver for OpenRISC Multicore programmable interrupt controller as\n> described in the Multicore support section of the OpenRISC 1.2\n> proposed architecture specification:\n> \n> https://github.com/stffrdhrn/doc/raw/arch-1.2-proposal/openrisc-arch-1.2-rev0.pdf\n> \n> Each OpenRISC core contains a full interrupt controller which is used in\n> the SMP architecture for interrupt balancing. This IPI device is the\n> only external device required for enabling SMP on OpenRISC.\n\nI'm a little confused. Is this device the whole \"ompic\", a sub-device\nthereof, or an independent IP block connected to it?\n\n> Pending ops are stored in a memory bit mask which can allow multiple\n> pending operations to be set and serviced at a time. This is mostly\n> borrowed from the alpha IPI implementation.\n> \n> Signed-off-by: Stefan Kristiansson \n> [shorne@gmail.com: converted ops to bitmask, wrote commit message]\n> Signed-off-by: Stafford Horne \n> ---\n> .../bindings/interrupt-controller/ompic.txt | 22 ++++\n> arch/openrisc/Kconfig | 1 +\n> drivers/irqchip/Kconfig | 4 +\n> drivers/irqchip/Makefile | 1 +\n> drivers/irqchip/irq-ompic.c | 117 +++++++++++++++++++++\n> 5 files changed, 145 insertions(+)\n> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ompic.txt\n> create mode 100644 drivers/irqchip/irq-ompic.c\n> \n> diff --git a/Documentation/devicetree/bindings/interrupt-controller/ompic.txt b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt\n> new file mode 100644\n> index 000000000000..4176ecc3366d\n> --- /dev/null\n> +++ b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt\n> @@ -0,0 +1,22 @@\n> +OpenRISC Multicore Programmable Interrupt Controller\n> +\n> +Required properties:\n> +\n> +- compatible : This should be \"ompic\"\n\nThis needs a vendor prefix.\n\nPresumably, this should be \"opencores,ompic\"? (pending whether this is\nactually the whole ompic, as above).\n\n> +- reg : Specifies base physical address and size of the register space. The\n> + size can be arbitrary based on the number of cores the controller has\n> + been configured to handle, typically 8 bytes per core.\n\nHow are those regions correlated with CPUs?\n\ne.g. is there a fixed relationship with a physical CPU id, or some\nmechanism by which the relationship can be probed dynamically?\n\n> +- interrupt-controller : Identifies the node as an interrupt controller\n> +- #interrupt-cells : Specifies the number of cells needed to encode an\n> + interrupt source. The value shall be 1.\n\nNo flags? Does this not need edge/level configuration or active high/low\nconfiguration?\n\n> +- interrupts : Specifies the interrupt line to which the ompic is wired.\n\nWhat is this interrupt used for?\n\nIs this some percpu interrupt used to proxy the IPI? It feels very odd\nto assume such a thing from the parent irqchip. Surely this is\nintimately coupled with that?\n\n> +\n> +Example:\n> +\n> +ompic: ompic {\n> +\tcompatible = \"ompic\";\n> +\treg = <0x98000000 16>;\n> +\t#interrupt-cells = <1>;\n> +\tinterrupt-controller;\n> +\tinterrupts = <1>;\n> +};\n\n[...]\n\n> +static struct {\n> +\tunsigned long ops;\n> +} ipi_data[NR_CPUS];\n> +\n> +static void __iomem *ompic_base;\n\nIs there guaranteed to only be one of these system-wide?\n\n[...]\n\n> +void ompic_raise_softirq(const struct cpumask *mask, unsigned int irq)\n> +{\n> +\tunsigned int dst_cpu;\n> +\tunsigned int src_cpu = smp_processor_id();\n> +\n> +\tfor_each_cpu(dst_cpu, mask) {\n> +\t\tset_bit(irq, &ipi_data[dst_cpu].ops);\n> +\n> +\t\tompic_writereg(ompic_base, OMPIC_IPI_CTRL(src_cpu),\n> +\t\t\t OMPIC_IPI_CTRL_IRQ_GEN |\n> +\t\t\t OMPIC_IPI_CTRL_DST(dst_cpu) |\n> +\t\t\t OMPIC_IPI_DATA(1));\n> +\t}\n\nHere you assume that the mapping is big enough to cover these registers,\nbut the ompic_of_init() function didn't sanity-check the size, so this\nis not guaranteed.\n\n[...]\n\n> +int __init ompic_of_init(struct device_node *node, struct device_node *parent)\n> +{\n> +\tint irq;\n> +\n> +\tif (WARN_ON(!node))\n> +\t\treturn -ENODEV;\n\nGiven this function is only invoked if the kernel found a node with a\nmatching compatible string, how can this possibly happen?\n\n> +\tmemset(ipi_data, 0, sizeof(ipi_data));\n\nAs ipi_data was static, it is already zeroed.\n\n> +\n> +\tompic_base = of_iomap(node, 0);\n\nWhat if this failed?\n\n> +\n> +\tirq = irq_of_parse_and_map(node, 0);\n\nWhat if this is missing?\n\n> +\tsetup_irq(irq, &ompi_ipi_irqaction);\n\nAs covered earlier, I;m confused by this. I'd expect that your root\nirqchip would handle the IPIs, and hence need to probe nothing from the\nDT for those.\n\nHere we're assuming the IRQ is wired up to some per-cpu interrupt that\nwe can treat as an IPI, and that the parent irqchip handles that\nappropriately, which seems very odd.\n\nThanks,\nMark.\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xjfZ13fGYz9sRW\n\tfor ;\n\tThu, 31 Aug 2017 21:01:13 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751236AbdHaLA7 (ORCPT\n\t);\n\tThu, 31 Aug 2017 07:00:59 -0400","from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:54342 \"EHLO\n\tfoss.arm.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751053AbdHaLA6 (ORCPT );\n\tThu, 31 Aug 2017 07:00:58 -0400","from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249])\n\tby usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 9469980D;\n\tThu, 31 Aug 2017 04:00:58 -0700 (PDT)","from leverpostej (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249])\n\tby usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id\n\t9462E3F58F; Thu, 31 Aug 2017 04:00:56 -0700 (PDT)"],"Date":"Thu, 31 Aug 2017 11:59:40 +0100","From":"Mark Rutland ","To":"Stafford Horne ","Cc":"LKML ,\n\tOpenrisc ,\n\tStefan Kristiansson ,\n\tThomas Gleixner ,\n\tJason Cooper , \n\tMarc Zyngier , Rob Herring ,\n\tJonas Bonn , devicetree@vger.kernel.org","Subject":"Re: [PATCH 05/13] irqchip: add initial support for ompic","Message-ID":"<20170831105940.GE15031@leverpostej>","References":"\n\t<538699c64b5601e8800b77da29f7951bf23f57ce.1504129273.git.shorne@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<538699c64b5601e8800b77da29f7951bf23f57ce.1504129273.git.shorne@gmail.com>","User-Agent":"Mutt/1.5.21 (2010-09-15)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1761420,"web_url":"http://patchwork.ozlabs.org/comment/1761420/","msgid":"<20170901012449.GG2609@lianli.shorne-pla.net>","list_archive_url":null,"date":"2017-09-01T01:24:49","subject":"Re: [PATCH 05/13] irqchip: add initial support for ompic","submitter":{"id":68420,"url":"http://patchwork.ozlabs.org/api/people/68420/","name":"Stafford Horne","email":"shorne@gmail.com"},"content":"On Thu, Aug 31, 2017 at 10:28:01AM +0100, Marc Zyngier wrote:\n> On 30/08/17 22:58, Stafford Horne wrote:\n> > From: Stefan Kristiansson \n> > \n> > IPI driver for OpenRISC Multicore programmable interrupt controller as\n> > described in the Multicore support section of the OpenRISC 1.2\n> > proposed architecture specification:\n> > \n> > https://github.com/stffrdhrn/doc/raw/arch-1.2-proposal/openrisc-arch-1.2-rev0.pdf\n> > \n> > Each OpenRISC core contains a full interrupt controller which is used in\n> > the SMP architecture for interrupt balancing. This IPI device is the\n> > only external device required for enabling SMP on OpenRISC.\n> > \n> > Pending ops are stored in a memory bit mask which can allow multiple\n> > pending operations to be set and serviced at a time. This is mostly\n> > borrowed from the alpha IPI implementation.\n> > \n> > Signed-off-by: Stefan Kristiansson \n> > [shorne@gmail.com: converted ops to bitmask, wrote commit message]\n> > Signed-off-by: Stafford Horne \n> > ---\n> > .../bindings/interrupt-controller/ompic.txt | 22 ++++\n> > arch/openrisc/Kconfig | 1 +\n> > drivers/irqchip/Kconfig | 4 +\n> > drivers/irqchip/Makefile | 1 +\n> > drivers/irqchip/irq-ompic.c | 117 +++++++++++++++++++++\n> > 5 files changed, 145 insertions(+)\n> > create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ompic.txt\n> > create mode 100644 drivers/irqchip/irq-ompic.c\n> > \n> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/ompic.txt b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt\n> > new file mode 100644\n> > index 000000000000..4176ecc3366d\n> > --- /dev/null\n> > +++ b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt\n> > @@ -0,0 +1,22 @@\n> > +OpenRISC Multicore Programmable Interrupt Controller\n> > +\n> > +Required properties:\n> > +\n> > +- compatible : This should be \"ompic\"\n> > +- reg : Specifies base physical address and size of the register space. The\n> > + size can be arbitrary based on the number of cores the controller has\n> > + been configured to handle, typically 8 bytes per core.\n> > +- interrupt-controller : Identifies the node as an interrupt controller\n> > +- #interrupt-cells : Specifies the number of cells needed to encode an\n> > + interrupt source. The value shall be 1.\n> > +- interrupts : Specifies the interrupt line to which the ompic is wired.\n> > +\n> > +Example:\n> > +\n> > +ompic: ompic {\n> > +\tcompatible = \"ompic\";\n> > +\treg = <0x98000000 16>;\n> > +\t#interrupt-cells = <1>;\n> > +\tinterrupt-controller;\n> > +\tinterrupts = <1>;\n> > +};\n> > diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig\n> > index 214c837ce597..dd7e55e7e42d 100644\n> > --- a/arch/openrisc/Kconfig\n> > +++ b/arch/openrisc/Kconfig\n> > @@ -30,6 +30,7 @@ config OPENRISC\n> > \tselect NO_BOOTMEM\n> > \tselect ARCH_USE_QUEUED_SPINLOCKS\n> > \tselect ARCH_USE_QUEUED_RWLOCKS\n> > +\tselect OMPIC if SMP\n> > \n> > config CPU_BIG_ENDIAN\n> > \tdef_bool y\n> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig\n> > index f1fd5f44d1d4..3fa60e6667a7 100644\n> > --- a/drivers/irqchip/Kconfig\n> > +++ b/drivers/irqchip/Kconfig\n> > @@ -145,6 +145,10 @@ config CLPS711X_IRQCHIP\n> > \tselect SPARSE_IRQ\n> > \tdefault y\n> > \n> > +config OMPIC\n> > +\tbool\n> > +\tselect IRQ_DOMAIN\n> \n> Why do you need to select IRQ_DOMAIN? This driver doesn't seem to use any...\n\nRight, I will look to remove that.\n\n> > +\n> > config OR1K_PIC\n> > \tbool\n> > \tselect IRQ_DOMAIN\n> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile\n> > index e88d856cc09c..123047d7a20d 100644\n> > --- a/drivers/irqchip/Makefile\n> > +++ b/drivers/irqchip/Makefile\n> > @@ -17,6 +17,7 @@ obj-$(CONFIG_DW_APB_ICTL)\t\t+= irq-dw-apb-ictl.o\n> > obj-$(CONFIG_METAG)\t\t\t+= irq-metag-ext.o\n> > obj-$(CONFIG_METAG_PERFCOUNTER_IRQS)\t+= irq-metag.o\n> > obj-$(CONFIG_CLPS711X_IRQCHIP)\t\t+= irq-clps711x.o\n> > +obj-$(CONFIG_OMPIC)\t\t\t+= irq-ompic.o\n> > obj-$(CONFIG_OR1K_PIC)\t\t\t+= irq-or1k-pic.o\n> > obj-$(CONFIG_ORION_IRQCHIP)\t\t+= irq-orion.o\n> > obj-$(CONFIG_OMAP_IRQCHIP)\t\t+= irq-omap-intc.o\n> > diff --git a/drivers/irqchip/irq-ompic.c b/drivers/irqchip/irq-ompic.c\n> > new file mode 100644\n> > index 000000000000..438819f8a5a7\n> > --- /dev/null\n> > +++ b/drivers/irqchip/irq-ompic.c\n> > @@ -0,0 +1,117 @@\n> > +/*\n> > + * Open Multi-Processor Interrupt Controller driver\n> > + *\n> > + * Copyright (C) 2014 Stefan Kristiansson \n> > + *\n> > + * This file is licensed under the terms of the GNU General Public License\n> > + * version 2. This program is licensed \"as is\" without any warranty of any\n> > + * kind, whether express or implied.\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> \n> Don't think you need this.\n> \n> > +#include \n> \n> Nor this.\n\nOK on both.\n\n> > +\n> > +#include \n> > +\n> > +#define OMPIC_IPI_BASE\t\t\t0x0\n> > +#define OMPIC_IPI_CTRL(cpu)\t\t(OMPIC_IPI_BASE + 0x0 + (cpu)*8)\n> > +#define OMPIC_IPI_STAT(cpu)\t\t(OMPIC_IPI_BASE + 0x4 + (cpu)*8)\n> \n> In the DT binding you say that \"size can be arbitrary based on the\n> number of cores the controller has been configured to handle, typically\n> 8 bytes per core\". Here, this is 8 bytes, always, which is not exactly\n> the same. What is the architectural value, if any? If there is none,\n> then the per-core size should either come from DT or some other mean\n> (register?).\n\nI mean the address space is 8 bytes x number-of-cores. Thats what I meant\nby arbitrary, I guess its better for me to be explicit. There is no\nregister that we can check to confirm the configuration of ompic. But I\nguess we can check the CPU NUMCORES register and compare it to the DT\naddress space to do a sanity check.\n\n> > +\n> > +#define OMPIC_IPI_CTRL_IRQ_ACK\t\t(1 << 31)\n> > +#define OMPIC_IPI_CTRL_IRQ_GEN\t\t(1 << 30)\n> > +#define OMPIC_IPI_CTRL_DST(cpu)\t\t(((cpu) & 0x3fff) << 16)\n> > +\n> > +#define OMPIC_IPI_STAT_IRQ_PENDING\t(1 << 30)\n> > +\n> > +#define OMPIC_IPI_DATA(x)\t\t((x) & 0xffff)\n> > +\n> > +static struct {\n> > +\tunsigned long ops;\n> > +} ipi_data[NR_CPUS];\n> \n> In general, a per cpu data structure is better expressed as a percpu\n> data structure (yes, I'm in a funny mood this morning). Otherwise,\n> you're going to thrash more than just the receiver and the sender, but\n> also all the other CPUs that have their data in the same cache line.\n\nRight, that makes sense, I am not sure why that was done this way. I think\nI borrowed from alpha which has the extra __cacheline_aligned annotations.\nI forgot to come back and fix this. Ill do as you suggest, thank you.\n\nExcerpt from alpha:\n\nstatic struct {\n unsigned long bits ____cacheline_aligned;\n} ipi_data[NR_CPUS] __cacheline_aligned;\n\n> > +\n> > +static void __iomem *ompic_base;\n> > +\n> > +static inline u32 ompic_readreg(void __iomem *base, loff_t offset)\n> > +{\n> > +\treturn ioread32be(base + offset);\n> > +}\n> > +\n> > +static void ompic_writereg(void __iomem *base, loff_t offset, u32 data)\n> > +{\n> > +\tiowrite32be(data, base + offset);\n> > +}\n> > +\n> > +#ifdef CONFIG_SMP\n> \n> This code is only selected when CONFIG_SMP=y.\n\nYes, that is right, as below:\n\n set_smp_cross_call(ompic_raise_softirq);\n\nThe set_smp_cross_call() function from smp.c is only defined for smp. Do\nyou think thats wrong or needed extra comments? This is similar to other\nchips in irqchip/ for archs which use set_smp_cross_call().\n\n> > +void ompic_raise_softirq(const struct cpumask *mask, unsigned int irq)\n> > +{\n> \n> What is \"irq\" here? How is it guaranteed to fit in an unsigned long?\n\nHere irq is the `enum ipi_msg_type` which is guaranteed to fit in unsigned\nlong. Porbably its better to rename as msg or ipi_msg?\n\n> > +\tunsigned int dst_cpu;\n> > +\tunsigned int src_cpu = smp_processor_id();\n> > +\n> > +\tfor_each_cpu(dst_cpu, mask) {\n> > +\t\tset_bit(irq, &ipi_data[dst_cpu].ops);\n> > +\n> > +\t\tompic_writereg(ompic_base, OMPIC_IPI_CTRL(src_cpu),\n> > +\t\t\t OMPIC_IPI_CTRL_IRQ_GEN |\n> > +\t\t\t OMPIC_IPI_CTRL_DST(dst_cpu) |\n> > +\t\t\t OMPIC_IPI_DATA(1));\n> \n> What guarantees that the action of set_bit can be observed by the target\n> CPU? set-bit gives you atomicity, but no barrier.\n\nThe bit will not be read by target CPUs until after the ompic_writereg()\nwhich will trigger the target CPU interrupt to be raised. OpenRISC stores\nare ordered.\n\nThis will work on OpenRISC, but should I be thinking of other architectures\nas well for all drivers? Or do you think this will be an issue on\nOpenRISC?\n\n> > +\t}\n> > +}\n> > +#endif\n> > +\n> > +irqreturn_t ompic_ipi_handler(int irq, void *dev_id)\n> > +{\n> > +\tunsigned int cpu = smp_processor_id();\n> > +\tunsigned long *pending_ops = &ipi_data[cpu].ops;\n> > +\tunsigned long ops;\n> > +\n> > +\tompic_writereg(ompic_base, OMPIC_IPI_CTRL(cpu), OMPIC_IPI_CTRL_IRQ_ACK);\n> > +\twhile ((ops = xchg(pending_ops, 0)) != 0) {\n> \n> Barrier again?\n\nThanks, but some concern of above.\n\n> > +\t\tdo {\n> > +\t\t\tunsigned long ipi;\n> > +\n> > +\t\t\tipi = ops & -ops;\n> > +\t\t\tops &= ~ipi;\n> > +\t\t\tipi = __ffs(ipi);\n> \n> This feels pointlessly convoluted. Is there any reason why you can't\n> write it as:\n> \n> \t\t\tipi = __ffs(ops);\n> \t\t\tops &= ~(1UL << ipi);\n> \n> which feels like a much more common idiom?\n\nRight, this should work, not sure what I was thinking above right now. Let\nme give that a try.\n\n> > +\n> > +\t\t\thandle_IPI(ipi);\n> > +\t\t} while (ops);\n> > +\t}\n> > +\n> > +\treturn IRQ_HANDLED;\n> > +}\n> > +\n> > +static struct irqaction ompi_ipi_irqaction = {\n> > +\t.handler = ompic_ipi_handler,\n> > +\t.flags = IRQF_PERCPU,\n> > +\t.name = \"ompic_ipi\",\n> > +};\n> > +\n> > +#ifdef CONFIG_OF\n> \n> This code is useless if you don't have CONFIG_OF. So either you make the\n> driver depend on CONFIG_OF, or you actively select it. And given that\n> CONFIG_OPENRISC already selects CONFIG_OF, you can happily get rid of\n> all the #ifdefery here.\n\nThanks, right.\n\n> > +int __init ompic_of_init(struct device_node *node, struct device_node *parent)\n> > +{\n> > +\tint irq;\n> > +\n> > +\tif (WARN_ON(!node))\n> > +\t\treturn -ENODEV;\n> \n> How do you end-up here if node == NULL?\n\nRight.\n\n> > +\n> > +\tmemset(ipi_data, 0, sizeof(ipi_data));\n> \n> The kernel should do this for you already.\n\nRight.\n\n> > +\n> > +\tompic_base = of_iomap(node, 0);\n> > +\n> > +\tirq = irq_of_parse_and_map(node, 0);\n> > +\tsetup_irq(irq, &ompi_ipi_irqaction);\n> > +\n> > +#ifdef CONFIG_SMP\n> > +\tset_smp_cross_call(ompic_raise_softirq);\n> > +#endif\n> > +\n> > +\treturn 0;\n> > +}\n> > +IRQCHIP_DECLARE(ompic, \"ompic\", ompic_of_init);\n> > +#endif\n> >\n\nThank you for the feedback, I will clean this up and resubmit with the\ncomments on the other thread.\n\nIn terms of commit path, do you think its ok for this to go in via the\nOpenRISC arch path?\n\n-Stafford\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"R8DfC4NY\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xk1kb4gBnz9sMN\n\tfor ;\n\tFri, 1 Sep 2017 11:24:55 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1750955AbdIABYy (ORCPT\n\t);\n\tThu, 31 Aug 2017 21:24:54 -0400","from mail-pg0-f66.google.com ([74.125.83.66]:34216 \"EHLO\n\tmail-pg0-f66.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1750868AbdIABYx (ORCPT\n\t); Thu, 31 Aug 2017 21:24:53 -0400","by mail-pg0-f66.google.com with SMTP id 63so795110pgc.1;\n\tThu, 31 Aug 2017 18:24:53 -0700 (PDT)","from localhost (g206.61-45-91.ppp.wakwak.ne.jp. [61.45.91.206])\n\tby smtp.gmail.com with ESMTPSA id\n\tu195sm1041655pgb.30.2017.08.31.18.24.51\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tThu, 31 Aug 2017 18:24:52 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:in-reply-to:user-agent;\n\tbh=9cH2nKpxFvdsm4PI0MBbJonZcHgUYB7u74BFlrBKYi4=;\n\tb=R8DfC4NYo/sYhjwTWVIbzJLUpAJvHS2M6Eb3yIBi3K7/qxG9yhMwFLap27GpRx7JmF\n\tWFoTjtGXt6L8gOoUMwywHvjeSj/2d8iUoiKPoQE2S8YuxiEXozeG3K4GQwEpqMktkpIr\n\tLM8ixFgamIfw0ceEFdcAnJ/e6O5bZCfkCVL4K3SuWzMEDHvwLJ3YV51op0K02PCeXOE7\n\t3cs8a6NZcaI4Ji5wXN4xB7A7t33OmbVEilST4r+KbEslRttWRvHf+m2lGtWBwtUmtusV\n\tALQoR9G1Q2dtRNQrmyrKsYmb4sSLoNC/VXTGrJ6boF93v3hohTfJdDXr6AeVfTglxuKc\n\tAArg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:in-reply-to:user-agent;\n\tbh=9cH2nKpxFvdsm4PI0MBbJonZcHgUYB7u74BFlrBKYi4=;\n\tb=rSX9yGIxl9HjziVrMa6IcTvF6n3VIF8yzAM29S23XoJOdtDEfrLe1V6Gz7ZHanK9Si\n\t2EOnbfMpdFk7OvWk8ljdb7lXeBv3uixV1f0jBHwyae/2jUABXCuzsdZRtqW+JJceK1zU\n\tCnQrdsxvY6fsiM4Qh4S0j+Dl5apPWzVoyNq6Eqd2ecuEY5YvHP6ZZzB7RhPktOLHvR1j\n\t9wNEc210Z/kSG0RejSaCt4aUu3AVSJjOdhCg6fN/xLBhbJa5vG33qR3v2KO0V7t1fbXQ\n\t7WZT64VgmNMVS+lwglgWEc1kKS8w5PvMR8tWXTfr+grh1YkMrS7rC3YY+LuB4EgIzOUS\n\tDtDw==","X-Gm-Message-State":"AHPjjUhBVEhdqE6tQDq4ESikw4ROpqsESc19z0PE2ySzCAQFiD0WrqHb\n\tP0Nm5/vbpPDOgQ==","X-Google-Smtp-Source":"ADKCNb6iYf2qz5XZaJBkWrHwlF1smUaIuU7rP+MlicGt9T6+Va0LJ/OVaIL/riwb2uByBJpDPJHAtg==","X-Received":"by 10.99.126.91 with SMTP id o27mr416081pgn.287.1504229092634;\n\tThu, 31 Aug 2017 18:24:52 -0700 (PDT)","Date":"Fri, 1 Sep 2017 10:24:49 +0900","From":"Stafford Horne ","To":"Marc Zyngier ","Cc":"LKML ,\n\tOpenrisc ,\n\tStefan Kristiansson ,\n\tThomas Gleixner ,\n\tJason Cooper , \n\tRob Herring , Mark Rutland ,\n\tJonas Bonn , devicetree@vger.kernel.org","Subject":"Re: [PATCH 05/13] irqchip: add initial support for ompic","Message-ID":"<20170901012449.GG2609@lianli.shorne-pla.net>","References":"\n\t<538699c64b5601e8800b77da29f7951bf23f57ce.1504129273.git.shorne@gmail.com>\n\t<1b062d84-7335-8553-39c7-2e60973b4396@arm.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<1b062d84-7335-8553-39c7-2e60973b4396@arm.com>","User-Agent":"Mutt/1.8.3 (2017-05-23)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1761717,"web_url":"http://patchwork.ozlabs.org/comment/1761717/","msgid":"<20170901135924.GK2609@lianli.shorne-pla.net>","list_archive_url":null,"date":"2017-09-01T13:59:24","subject":"Re: [PATCH 05/13] irqchip: add initial support for ompic","submitter":{"id":68420,"url":"http://patchwork.ozlabs.org/api/people/68420/","name":"Stafford Horne","email":"shorne@gmail.com"},"content":"On Thu, Aug 31, 2017 at 11:59:40AM +0100, Mark Rutland wrote:\n> On Thu, Aug 31, 2017 at 06:58:36AM +0900, Stafford Horne wrote:\n> > From: Stefan Kristiansson \n> > \n> > IPI driver for OpenRISC Multicore programmable interrupt controller as\n> > described in the Multicore support section of the OpenRISC 1.2\n> > proposed architecture specification:\n> > \n> > https://github.com/stffrdhrn/doc/raw/arch-1.2-proposal/openrisc-arch-1.2-rev0.pdf\n> > \n> > Each OpenRISC core contains a full interrupt controller which is used in\n> > the SMP architecture for interrupt balancing. This IPI device is the\n> > only external device required for enabling SMP on OpenRISC.\n> \n> I'm a little confused. Is this device the whole \"ompic\", a sub-device\n> thereof, or an independent IP block connected to it?\n\nThis device *is* the ompic, it currently just handles IPI communication,\nbut was originally thought it should do more, i.e. distribute device\ninterrupts, hence the name. For now, the ompic device only handles IPI.\n\nPerhaps we can change the name? If you think that's needed I can discuss\nwith Stefan (the creator).\n\nThis is documented in the pdf, but here is an ascii diagram to help.\n\nNotes\n\n o The ompic generates a level interrupt to the CPU PIC when a message is\n ready. Messages are delivered via the memory bus.\n o The ompic does not have any interrupt input lines.\n o The ompic must be wired to the same irq line on each core.\n o Devices must be wired to the same irq line on each core.\n o This seems strange, but in the end very little extra hardware is needed\n to enable smp.\n\n +---------+ +---------+\n | CPU | | CPU |\n | Core 0 |<==\\ (memory access) /==>| Core 1 |\n | [ PIC ]| | | | [ PIC ]|\n +----^-^--+ | | +----^-^--+\n | | v v | |\n <====|=|=================================|=|==> (memory bus)\n | | ^ ^ | |\n (ipi | +------|---------+--------|-------|-+ (device irq)\n irq | | | | |\n core0)| +------|---------|--------|-------+ (ipi irq core1)\n | | | | |\n +----o-o-+ | +--------+ |\n | ompic |<===/ | Device |<===/\n | IPI | +--------+\n +--------+\n\n> > Pending ops are stored in a memory bit mask which can allow multiple\n> > pending operations to be set and serviced at a time. This is mostly\n> > borrowed from the alpha IPI implementation.\n> > \n> > Signed-off-by: Stefan Kristiansson \n> > [shorne@gmail.com: converted ops to bitmask, wrote commit message]\n> > Signed-off-by: Stafford Horne \n> > ---\n> > .../bindings/interrupt-controller/ompic.txt | 22 ++++\n> > arch/openrisc/Kconfig | 1 +\n> > drivers/irqchip/Kconfig | 4 +\n> > drivers/irqchip/Makefile | 1 +\n> > drivers/irqchip/irq-ompic.c | 117 +++++++++++++++++++++\n> > 5 files changed, 145 insertions(+)\n> > create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ompic.txt\n> > create mode 100644 drivers/irqchip/irq-ompic.c\n> > \n> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/ompic.txt b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt\n> > new file mode 100644\n> > index 000000000000..4176ecc3366d\n> > --- /dev/null\n> > +++ b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt\n> > @@ -0,0 +1,22 @@\n> > +OpenRISC Multicore Programmable Interrupt Controller\n> > +\n> > +Required properties:\n> > +\n> > +- compatible : This should be \"ompic\"\n> \n> This needs a vendor prefix.\n> \n> Presumably, this should be \"opencores,ompic\"? (pending whether this is\n> actually the whole ompic, as above).\n\nAs discussed above this is the ompic.\n\nIn terms of vendor, this and further openrisc development is no longer\nassociated with opencores, that company just hosts the old opencores.org\nwebsite but is not assisting in openrisc development any longer. Perhaps\n\"openrisc,ompic\" or \"openrisc,ipi\".\n\nThis also I would like to get final say from Stefan, but any suggestions\nare welcome.\n\n> > +- reg : Specifies base physical address and size of the register space. The\n> > + size can be arbitrary based on the number of cores the controller has\n> > + been configured to handle, typically 8 bytes per core.\n> \n> How are those regions correlated with CPUs?\n> \n> e.g. is there a fixed relationship with a physical CPU id, or some\n> mechanism by which the relationship can be probed dynamically?\n\nThere should only be one region.\n\ncore id 0 is associated with registers at address 0x0 and 0x4\ncore id 1 is associated with registers at address 0x8 and 0xc\netc.\n\nThis is can't be probed. Should this be documented here? That seems more\nof something that is internal to the driver. I think the only thing some\none would want to set in TD is the register space size which should be be 8\nx number-of-cores.\n\n> > +- interrupt-controller : Identifies the node as an interrupt controller\n> > +- #interrupt-cells : Specifies the number of cells needed to encode an\n> > + interrupt source. The value shall be 1.\n> \n> No flags? Does this not need edge/level configuration or active high/low\n> configuration?\n\nNo flags, this maybe I am confused on, the ompic does not receive any\ninterrupts. The input to the ompic are register writes which raise/clear\ninterrupts on the target CPUs. So maybe it should not define an\n'interrupt-controller' or '#interrupt-cells' tag. But then should it be an\nirqchip? Since its facilitates the IPI I think this is the right place.\n\n> > +- interrupts : Specifies the interrupt line to which the ompic is wired.\n> \n> What is this interrupt used for?\n> \n> Is this some percpu interrupt used to proxy the IPI? It feels very odd\n> to assume such a thing from the parent irqchip. Surely this is\n> intimately coupled with that?\n\nThis represents which IRQ line on each cpu the IPI is connected to (it must\nbe connected to the same IRQ line on each CPU.\n\ni.e.\n interrupt-parent = <&pic>;\n\n> > +\n> > +Example:\n> > +\n> > +ompic: ompic {\n> > +\tcompatible = \"ompic\";\n> > +\treg = <0x98000000 16>;\n> > +\t#interrupt-cells = <1>;\n> > +\tinterrupt-controller;\n> > +\tinterrupts = <1>;\n> > +};\n> \n> [...]\n> \n> > +static struct {\n> > +\tunsigned long ops;\n> > +} ipi_data[NR_CPUS];\n> > +\n> > +static void __iomem *ompic_base;\n> \n> Is there guaranteed to only be one of these system-wide?\n\nYes, that should be guaranteed, but I can look for moving this into a per\ndevice structure.\n\n> [...]\n> \n> > +void ompic_raise_softirq(const struct cpumask *mask, unsigned int irq)\n> > +{\n> > +\tunsigned int dst_cpu;\n> > +\tunsigned int src_cpu = smp_processor_id();\n> > +\n> > +\tfor_each_cpu(dst_cpu, mask) {\n> > +\t\tset_bit(irq, &ipi_data[dst_cpu].ops);\n> > +\n> > +\t\tompic_writereg(ompic_base, OMPIC_IPI_CTRL(src_cpu),\n> > +\t\t\t OMPIC_IPI_CTRL_IRQ_GEN |\n> > +\t\t\t OMPIC_IPI_CTRL_DST(dst_cpu) |\n> > +\t\t\t OMPIC_IPI_DATA(1));\n> > +\t}\n> \n> Here you assume that the mapping is big enough to cover these registers,\n> but the ompic_of_init() function didn't sanity-check the size, so this\n> is not guaranteed.\n\nOK, I will add some sanity checking.\n\n> [...]\n> \n> > +int __init ompic_of_init(struct device_node *node, struct device_node *parent)\n> > +{\n> > +\tint irq;\n> > +\n> > +\tif (WARN_ON(!node))\n> > +\t\treturn -ENODEV;\n> \n> Given this function is only invoked if the kernel found a node with a\n> matching compatible string, how can this possibly happen?\n\nRight, I think now. Will fix.\n\n> > +\tmemset(ipi_data, 0, sizeof(ipi_data));\n> \n> As ipi_data was static, it is already zeroed.\n\nRight, will fix.\n\n> > +\n> > +\tompic_base = of_iomap(node, 0);\n> \n> What if this failed?\n\nRight will fix.\n\n> > +\n> > +\tirq = irq_of_parse_and_map(node, 0);\n> \n> What if this is missing?\n\nOK, I will do some error checking.\n\n> \n> > +\tsetup_irq(irq, &ompi_ipi_irqaction);\n> \n> As covered earlier, I;m confused by this. I'd expect that your root\n> irqchip would handle the IPIs, and hence need to probe nothing from the\n> DT for those.\n> \n> Here we're assuming the IRQ is wired up to some per-cpu interrupt that\n> we can treat as an IPI, and that the parent irqchip handles that\n> appropriately, which seems very odd.\n\nI hope the comments above help to clarify this. By appropriately you mean\nlevel/edge active high/low? The ompic and openrisc internal pic have been\ndesigned to be compatible.\n\nThanks for the review. Between this and the comments from Marc I have a\nbit of work to do. Please let me know if you have any further questions.\n\n-Stafford\n\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"rSzjLZuM\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xkLTH401Rz9t32\n\tfor ;\n\tFri, 1 Sep 2017 23:59:31 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752033AbdIAN7a (ORCPT\n\t);\n\tFri, 1 Sep 2017 09:59:30 -0400","from mail-pf0-f193.google.com ([209.85.192.193]:37329 \"EHLO\n\tmail-pf0-f193.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751966AbdIAN72 (ORCPT\n\t); Fri, 1 Sep 2017 09:59:28 -0400","by mail-pf0-f193.google.com with SMTP id a2so170583pfj.4;\n\tFri, 01 Sep 2017 06:59:28 -0700 (PDT)","from localhost (g206.61-45-91.ppp.wakwak.ne.jp. [61.45.91.206])\n\tby smtp.gmail.com with ESMTPSA id\n\tp190sm482904pfb.39.2017.09.01.06.59.26\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tFri, 01 Sep 2017 06:59:27 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:in-reply-to:user-agent;\n\tbh=PWfuCmj5ZdLvmE6A6MNE5BV/f5h09YAkJ1HxFLhsOik=;\n\tb=rSzjLZuM1HFaZ5uB8gKB9mwn7wkpX2qJw0/8En6Ixm5AdKucamK8atyMQ7tOq/1EEF\n\tN4QrfFodx+/jjVXdVJ9xPYXsrj5l2lIImvTfdv7pe5tCqz6dtPCgoGjV6kdCVMOwouSL\n\tHdrQRuyXKi9Y5xbQEexSMUtVDs23kW/VwpsK9YNQ5J6il7+FWFWH4MOA/Rfal4OBCUc6\n\tpwgfrHfjFaU+9eexzhsMGDpjZ3skG+6BqZV1SB92jIT8dVcE4YASsF22ugGA+2IX8zeL\n\t26o6FKEDgUgReGb0O0Vk7S5Sgvv7OuqLQapuIbn/f67azlh7RM0JnaPWWVu5eDbkoGOC\n\t3j5Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:in-reply-to:user-agent;\n\tbh=PWfuCmj5ZdLvmE6A6MNE5BV/f5h09YAkJ1HxFLhsOik=;\n\tb=WNXl6A1Ac5xHIJLcJdAo4tefh7jzsy/G1Euljo2tH0W9rwlnanumkqQ6G9GRJc04x5\n\toXBfxL2p2Umnfm2M3NZiFm11aKoYtTB1z6XykBLlLqD1HpCWuVxz6x/O8945hs4gACgV\n\tIncLT/LemsToUeOeD8QifMmjn+td6VxG9ha6fQFJvWQooZoo2ZrqKSecGCJoSSqe4OQk\n\tfFrsc5d1Ur9tsQojC9b8XmCLyPFwFyHWZ7vvgZzR2MRZmcIMcYoNxX8ErKYXCWR2uowL\n\tWkgn/Se8sJ78V2wSij1IgrmeUMq48BTU3JL/AWooHdd/XJyGo4E/zr1dKF1fzV+ENgSI\n\tswNw==","X-Gm-Message-State":"AHPjjUiI6h4h2NSwjFvnKVxyJm7jvFhB8ooGlRPiQ3OGWcgi8Azyev+Y\n\t3gCHV6/G1n0dUs3f/BLLCA==","X-Google-Smtp-Source":"ADKCNb7qDnzby7/4H7WUqo8EqSXNFT1TqIDID1NQ2gr46HOX+Q8xuMy6YJiscKjb2D3edcLn+HC+RQ==","X-Received":"by 10.99.43.132 with SMTP id r126mr2516780pgr.241.1504274367698; \n\tFri, 01 Sep 2017 06:59:27 -0700 (PDT)","Date":"Fri, 1 Sep 2017 22:59:24 +0900","From":"Stafford Horne ","To":"Mark Rutland ","Cc":"LKML ,\n\tOpenrisc ,\n\tStefan Kristiansson ,\n\tThomas Gleixner ,\n\tJason Cooper , \n\tMarc Zyngier , Rob Herring ,\n\tJonas Bonn , devicetree@vger.kernel.org","Subject":"Re: [PATCH 05/13] irqchip: add initial support for ompic","Message-ID":"<20170901135924.GK2609@lianli.shorne-pla.net>","References":"\n\t<538699c64b5601e8800b77da29f7951bf23f57ce.1504129273.git.shorne@gmail.com>\n\t<20170831105940.GE15031@leverpostej>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170831105940.GE15031@leverpostej>","User-Agent":"Mutt/1.8.3 (2017-05-23)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1761859,"web_url":"http://patchwork.ozlabs.org/comment/1761859/","msgid":"<97879c84-3ce8-b2bf-d438-679a69b60774@arm.com>","list_archive_url":null,"date":"2017-09-01T17:25:01","subject":"Re: [PATCH 05/13] irqchip: add initial support for ompic","submitter":{"id":7353,"url":"http://patchwork.ozlabs.org/api/people/7353/","name":"Marc Zyngier","email":"marc.zyngier@arm.com"},"content":"On 01/09/17 02:24, Stafford Horne wrote:\n> On Thu, Aug 31, 2017 at 10:28:01AM +0100, Marc Zyngier wrote:\n>> On 30/08/17 22:58, Stafford Horne wrote:\n>>> From: Stefan Kristiansson \n>>>\n>>> IPI driver for OpenRISC Multicore programmable interrupt controller as\n>>> described in the Multicore support section of the OpenRISC 1.2\n>>> proposed architecture specification:\n>>>\n>>> https://github.com/stffrdhrn/doc/raw/arch-1.2-proposal/openrisc-arch-1.2-rev0.pdf\n>>>\n>>> Each OpenRISC core contains a full interrupt controller which is used in\n>>> the SMP architecture for interrupt balancing. This IPI device is the\n>>> only external device required for enabling SMP on OpenRISC.\n>>>\n>>> Pending ops are stored in a memory bit mask which can allow multiple\n>>> pending operations to be set and serviced at a time. This is mostly\n>>> borrowed from the alpha IPI implementation.\n>>>\n>>> Signed-off-by: Stefan Kristiansson \n>>> [shorne@gmail.com: converted ops to bitmask, wrote commit message]\n>>> Signed-off-by: Stafford Horne \n>>> ---\n>>> .../bindings/interrupt-controller/ompic.txt | 22 ++++\n>>> arch/openrisc/Kconfig | 1 +\n>>> drivers/irqchip/Kconfig | 4 +\n>>> drivers/irqchip/Makefile | 1 +\n>>> drivers/irqchip/irq-ompic.c | 117 +++++++++++++++++++++\n>>> 5 files changed, 145 insertions(+)\n>>> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ompic.txt\n>>> create mode 100644 drivers/irqchip/irq-ompic.c\n>>>\n>>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/ompic.txt b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt\n>>> new file mode 100644\n>>> index 000000000000..4176ecc3366d\n>>> --- /dev/null\n>>> +++ b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt\n>>> @@ -0,0 +1,22 @@\n>>> +OpenRISC Multicore Programmable Interrupt Controller\n>>> +\n>>> +Required properties:\n>>> +\n>>> +- compatible : This should be \"ompic\"\n>>> +- reg : Specifies base physical address and size of the register space. The\n>>> + size can be arbitrary based on the number of cores the controller has\n>>> + been configured to handle, typically 8 bytes per core.\n>>> +- interrupt-controller : Identifies the node as an interrupt controller\n>>> +- #interrupt-cells : Specifies the number of cells needed to encode an\n>>> + interrupt source. The value shall be 1.\n>>> +- interrupts : Specifies the interrupt line to which the ompic is wired.\n>>> +\n>>> +Example:\n>>> +\n>>> +ompic: ompic {\n>>> +\tcompatible = \"ompic\";\n>>> +\treg = <0x98000000 16>;\n>>> +\t#interrupt-cells = <1>;\n>>> +\tinterrupt-controller;\n>>> +\tinterrupts = <1>;\n>>> +};\n>>> diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig\n>>> index 214c837ce597..dd7e55e7e42d 100644\n>>> --- a/arch/openrisc/Kconfig\n>>> +++ b/arch/openrisc/Kconfig\n>>> @@ -30,6 +30,7 @@ config OPENRISC\n>>> \tselect NO_BOOTMEM\n>>> \tselect ARCH_USE_QUEUED_SPINLOCKS\n>>> \tselect ARCH_USE_QUEUED_RWLOCKS\n>>> +\tselect OMPIC if SMP\n>>> \n>>> config CPU_BIG_ENDIAN\n>>> \tdef_bool y\n>>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig\n>>> index f1fd5f44d1d4..3fa60e6667a7 100644\n>>> --- a/drivers/irqchip/Kconfig\n>>> +++ b/drivers/irqchip/Kconfig\n>>> @@ -145,6 +145,10 @@ config CLPS711X_IRQCHIP\n>>> \tselect SPARSE_IRQ\n>>> \tdefault y\n>>> \n>>> +config OMPIC\n>>> +\tbool\n>>> +\tselect IRQ_DOMAIN\n>>\n>> Why do you need to select IRQ_DOMAIN? This driver doesn't seem to use any...\n> \n> Right, I will look to remove that.\n> \n>>> +\n>>> config OR1K_PIC\n>>> \tbool\n>>> \tselect IRQ_DOMAIN\n>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile\n>>> index e88d856cc09c..123047d7a20d 100644\n>>> --- a/drivers/irqchip/Makefile\n>>> +++ b/drivers/irqchip/Makefile\n>>> @@ -17,6 +17,7 @@ obj-$(CONFIG_DW_APB_ICTL)\t\t+= irq-dw-apb-ictl.o\n>>> obj-$(CONFIG_METAG)\t\t\t+= irq-metag-ext.o\n>>> obj-$(CONFIG_METAG_PERFCOUNTER_IRQS)\t+= irq-metag.o\n>>> obj-$(CONFIG_CLPS711X_IRQCHIP)\t\t+= irq-clps711x.o\n>>> +obj-$(CONFIG_OMPIC)\t\t\t+= irq-ompic.o\n>>> obj-$(CONFIG_OR1K_PIC)\t\t\t+= irq-or1k-pic.o\n>>> obj-$(CONFIG_ORION_IRQCHIP)\t\t+= irq-orion.o\n>>> obj-$(CONFIG_OMAP_IRQCHIP)\t\t+= irq-omap-intc.o\n>>> diff --git a/drivers/irqchip/irq-ompic.c b/drivers/irqchip/irq-ompic.c\n>>> new file mode 100644\n>>> index 000000000000..438819f8a5a7\n>>> --- /dev/null\n>>> +++ b/drivers/irqchip/irq-ompic.c\n>>> @@ -0,0 +1,117 @@\n>>> +/*\n>>> + * Open Multi-Processor Interrupt Controller driver\n>>> + *\n>>> + * Copyright (C) 2014 Stefan Kristiansson \n>>> + *\n>>> + * This file is licensed under the terms of the GNU General Public License\n>>> + * version 2. This program is licensed \"as is\" without any warranty of any\n>>> + * kind, whether express or implied.\n>>> + */\n>>> +\n>>> +#include \n>>> +#include \n>>> +#include \n>>> +#include \n>>> +#include \n>>> +#include \n>>> +#include \n>>\n>> Don't think you need this.\n>>\n>>> +#include \n>>\n>> Nor this.\n> \n> OK on both.\n> \n>>> +\n>>> +#include \n>>> +\n>>> +#define OMPIC_IPI_BASE\t\t\t0x0\n>>> +#define OMPIC_IPI_CTRL(cpu)\t\t(OMPIC_IPI_BASE + 0x0 + (cpu)*8)\n>>> +#define OMPIC_IPI_STAT(cpu)\t\t(OMPIC_IPI_BASE + 0x4 + (cpu)*8)\n>>\n>> In the DT binding you say that \"size can be arbitrary based on the\n>> number of cores the controller has been configured to handle, typically\n>> 8 bytes per core\". Here, this is 8 bytes, always, which is not exactly\n>> the same. What is the architectural value, if any? If there is none,\n>> then the per-core size should either come from DT or some other mean\n>> (register?).\n> \n> I mean the address space is 8 bytes x number-of-cores. Thats what I meant\n> by arbitrary, I guess its better for me to be explicit. There is no\n> register that we can check to confirm the configuration of ompic. But I\n> guess we can check the CPU NUMCORES register and compare it to the DT\n> address space to do a sanity check.\n\nThanks for the explanation. So the code is OK, but the DT should be more\nrigorous in saying that there is 8 bytes per CPU. And yes to the check\nif that can be done at this stage.\n\n> \n>>> +\n>>> +#define OMPIC_IPI_CTRL_IRQ_ACK\t\t(1 << 31)\n>>> +#define OMPIC_IPI_CTRL_IRQ_GEN\t\t(1 << 30)\n>>> +#define OMPIC_IPI_CTRL_DST(cpu)\t\t(((cpu) & 0x3fff) << 16)\n>>> +\n>>> +#define OMPIC_IPI_STAT_IRQ_PENDING\t(1 << 30)\n>>> +\n>>> +#define OMPIC_IPI_DATA(x)\t\t((x) & 0xffff)\n>>> +\n>>> +static struct {\n>>> +\tunsigned long ops;\n>>> +} ipi_data[NR_CPUS];\n>>\n>> In general, a per cpu data structure is better expressed as a percpu\n>> data structure (yes, I'm in a funny mood this morning). Otherwise,\n>> you're going to thrash more than just the receiver and the sender, but\n>> also all the other CPUs that have their data in the same cache line.\n> \n> Right, that makes sense, I am not sure why that was done this way. I think\n> I borrowed from alpha which has the extra __cacheline_aligned annotations.\n> I forgot to come back and fix this. Ill do as you suggest, thank you.\n> \n> Excerpt from alpha:\n> \n> static struct {\n> unsigned long bits ____cacheline_aligned;\n> } ipi_data[NR_CPUS] __cacheline_aligned;\n\nYup, __cacheline_aligned is key here, as it will have the same effect as\nthe percpu stuff from that point of view.\n\n>>> +\n>>> +static void __iomem *ompic_base;\n>>> +\n>>> +static inline u32 ompic_readreg(void __iomem *base, loff_t offset)\n>>> +{\n>>> +\treturn ioread32be(base + offset);\n>>> +}\n>>> +\n>>> +static void ompic_writereg(void __iomem *base, loff_t offset, u32 data)\n>>> +{\n>>> +\tiowrite32be(data, base + offset);\n>>> +}\n>>> +\n>>> +#ifdef CONFIG_SMP\n>>\n>> This code is only selected when CONFIG_SMP=y.\n> \n> Yes, that is right, as below:\n> \n> set_smp_cross_call(ompic_raise_softirq);\n> \n> The set_smp_cross_call() function from smp.c is only defined for smp. Do\n> you think thats wrong or needed extra comments? This is similar to other\n> chips in irqchip/ for archs which use set_smp_cross_call().\n\nOther irqchips can often be compiled for both SMP and !SMP, hence the\nneed of a #ifdef. In your case, this driver is only compiled when SMP is\nselected, so you're pretty much guaranteed that this symbol is available.\n\n> \n>>> +void ompic_raise_softirq(const struct cpumask *mask, unsigned int irq)\n>>> +{\n>>\n>> What is \"irq\" here? How is it guaranteed to fit in an unsigned long?\n> \n> Here irq is the `enum ipi_msg_type` which is guaranteed to fit in unsigned\n> long. Porbably its better to rename as msg or ipi_msg?\n\nYou should certainly make sure your \"enum ipi_msg_type\" is capped at the\nnumber of bits of unsigned long. And yes to ipi_msg, which is a better\nname than irq. Also, you could change the types of ompic_raise_softirq\nand set_smp_cross_call, so that you use the enum instead of an int here.\n\n> \n>>> +\tunsigned int dst_cpu;\n>>> +\tunsigned int src_cpu = smp_processor_id();\n>>> +\n>>> +\tfor_each_cpu(dst_cpu, mask) {\n>>> +\t\tset_bit(irq, &ipi_data[dst_cpu].ops);\n>>> +\n>>> +\t\tompic_writereg(ompic_base, OMPIC_IPI_CTRL(src_cpu),\n>>> +\t\t\t OMPIC_IPI_CTRL_IRQ_GEN |\n>>> +\t\t\t OMPIC_IPI_CTRL_DST(dst_cpu) |\n>>> +\t\t\t OMPIC_IPI_DATA(1));\n>>\n>> What guarantees that the action of set_bit can be observed by the target\n>> CPU? set-bit gives you atomicity, but no barrier.\n> \n> The bit will not be read by target CPUs until after the ompic_writereg()\n> which will trigger the target CPU interrupt to be raised. OpenRISC stores\n> are ordered.\n\nAre they really ordered irrespective of the memory type (one is\ncacheable RAM, the other is a device...)?\n\nAnd assuming they are (which I find a bit odd), that doesn't necessarily\nguarantee that the interrupt will only be delivered once the effect of\nset_bit can be seen on the target CPU...\n\n> This will work on OpenRISC, but should I be thinking of other architectures\n> as well for all drivers? Or do you think this will be an issue on\n> OpenRISC?\n\nI definitely think this could be an issue with OpenRISC, but only\nsomeone familiar with the OpenRISC architecture can say whether I'm\nright or wrong. I'm just guessing at the moment.\n\n[...]\n\n> Thank you for the feedback, I will clean this up and resubmit with the\n> comments on the other thread.\n> \n> In terms of commit path, do you think its ok for this to go in via the\n> OpenRISC arch path?\n\nSure, that's fine by me.\n\n\tM.","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xkR2X51fgz9t32\n\tfor ;\n\tSat, 2 Sep 2017 03:25:08 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751998AbdIARZG (ORCPT\n\t);\n\tFri, 1 Sep 2017 13:25:06 -0400","from foss.arm.com ([217.140.101.70]:41884 \"EHLO foss.arm.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751863AbdIARZF (ORCPT );\n\tFri, 1 Sep 2017 13:25:05 -0400","from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249])\n\tby usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id EC9162B;\n\tFri, 1 Sep 2017 10:25:04 -0700 (PDT)","from [10.1.207.16] (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249])\n\tby usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id\n\t435F13F483; Fri, 1 Sep 2017 10:25:03 -0700 (PDT)"],"Subject":"Re: [PATCH 05/13] irqchip: add initial support for ompic","To":"Stafford Horne ","Cc":"LKML ,\n\tOpenrisc ,\n\tStefan Kristiansson ,\n\tThomas Gleixner ,\n\tJason Cooper , \n\tRob Herring , Mark Rutland ,\n\tJonas Bonn , devicetree@vger.kernel.org","References":"\n\t<538699c64b5601e8800b77da29f7951bf23f57ce.1504129273.git.shorne@gmail.com>\n\t<1b062d84-7335-8553-39c7-2e60973b4396@arm.com>\n\t<20170901012449.GG2609@lianli.shorne-pla.net>","From":"Marc Zyngier ","Organization":"ARM Ltd","Message-ID":"<97879c84-3ce8-b2bf-d438-679a69b60774@arm.com>","Date":"Fri, 1 Sep 2017 18:25:01 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170901012449.GG2609@lianli.shorne-pla.net>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1762335,"web_url":"http://patchwork.ozlabs.org/comment/1762335/","msgid":"<20170903221236.GM2609@lianli.shorne-pla.net>","list_archive_url":null,"date":"2017-09-03T22:12:36","subject":"Re: [PATCH 05/13] irqchip: add initial support for ompic","submitter":{"id":68420,"url":"http://patchwork.ozlabs.org/api/people/68420/","name":"Stafford Horne","email":"shorne@gmail.com"},"content":"On Fri, Sep 01, 2017 at 06:25:01PM +0100, Marc Zyngier wrote:\n> On 01/09/17 02:24, Stafford Horne wrote:\n> > On Thu, Aug 31, 2017 at 10:28:01AM +0100, Marc Zyngier wrote:\n> >> On 30/08/17 22:58, Stafford Horne wrote:\n> >>> From: Stefan Kristiansson \n> >>>\n> >>> IPI driver for OpenRISC Multicore programmable interrupt controller as\n> >>> described in the Multicore support section of the OpenRISC 1.2\n> >>> proposed architecture specification:\n> >>>\n> >>> https://github.com/stffrdhrn/doc/raw/arch-1.2-proposal/openrisc-arch-1.2-rev0.pdf\n> >>>\n> >>> Each OpenRISC core contains a full interrupt controller which is used in\n> >>> the SMP architecture for interrupt balancing. This IPI device is the\n> >>> only external device required for enabling SMP on OpenRISC.\n> >>>\n> >>> Pending ops are stored in a memory bit mask which can allow multiple\n> >>> pending operations to be set and serviced at a time. This is mostly\n> >>> borrowed from the alpha IPI implementation.\n> >>>\n> >>> Signed-off-by: Stefan Kristiansson \n> >>> [shorne@gmail.com: converted ops to bitmask, wrote commit message]\n> >>> Signed-off-by: Stafford Horne \n> >>> ---\n> >>> .../bindings/interrupt-controller/ompic.txt | 22 ++++\n> >>> arch/openrisc/Kconfig | 1 +\n> >>> drivers/irqchip/Kconfig | 4 +\n> >>> drivers/irqchip/Makefile | 1 +\n> >>> drivers/irqchip/irq-ompic.c | 117 +++++++++++++++++++++\n> >>> 5 files changed, 145 insertions(+)\n> >>> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ompic.txt\n> >>> create mode 100644 drivers/irqchip/irq-ompic.c\n> >>>\n> >>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/ompic.txt b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt\n> >>> new file mode 100644\n> >>> index 000000000000..4176ecc3366d\n> >>> --- /dev/null\n> >>> +++ b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt\n> >>> @@ -0,0 +1,22 @@\n> >>> +OpenRISC Multicore Programmable Interrupt Controller\n> >>> +\n> >>> +Required properties:\n> >>> +\n> >>> +- compatible : This should be \"ompic\"\n> >>> +- reg : Specifies base physical address and size of the register space. The\n> >>> + size can be arbitrary based on the number of cores the controller has\n> >>> + been configured to handle, typically 8 bytes per core.\n> >>> +- interrupt-controller : Identifies the node as an interrupt controller\n> >>> +- #interrupt-cells : Specifies the number of cells needed to encode an\n> >>> + interrupt source. The value shall be 1.\n> >>> +- interrupts : Specifies the interrupt line to which the ompic is wired.\n> >>> +\n> >>> +Example:\n> >>> +\n> >>> +ompic: ompic {\n> >>> +\tcompatible = \"ompic\";\n> >>> +\treg = <0x98000000 16>;\n> >>> +\t#interrupt-cells = <1>;\n> >>> +\tinterrupt-controller;\n> >>> +\tinterrupts = <1>;\n> >>> +};\n> >>> diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig\n> >>> index 214c837ce597..dd7e55e7e42d 100644\n> >>> --- a/arch/openrisc/Kconfig\n> >>> +++ b/arch/openrisc/Kconfig\n> >>> @@ -30,6 +30,7 @@ config OPENRISC\n> >>> \tselect NO_BOOTMEM\n> >>> \tselect ARCH_USE_QUEUED_SPINLOCKS\n> >>> \tselect ARCH_USE_QUEUED_RWLOCKS\n> >>> +\tselect OMPIC if SMP\n> >>> \n> >>> config CPU_BIG_ENDIAN\n> >>> \tdef_bool y\n> >>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig\n> >>> index f1fd5f44d1d4..3fa60e6667a7 100644\n> >>> --- a/drivers/irqchip/Kconfig\n> >>> +++ b/drivers/irqchip/Kconfig\n> >>> @@ -145,6 +145,10 @@ config CLPS711X_IRQCHIP\n> >>> \tselect SPARSE_IRQ\n> >>> \tdefault y\n> >>> \n> >>> +config OMPIC\n> >>> +\tbool\n> >>> +\tselect IRQ_DOMAIN\n> >>\n> >> Why do you need to select IRQ_DOMAIN? This driver doesn't seem to use any...\n> > \n> > Right, I will look to remove that.\n> > \n> >>> +\n> >>> config OR1K_PIC\n> >>> \tbool\n> >>> \tselect IRQ_DOMAIN\n> >>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile\n> >>> index e88d856cc09c..123047d7a20d 100644\n> >>> --- a/drivers/irqchip/Makefile\n> >>> +++ b/drivers/irqchip/Makefile\n> >>> @@ -17,6 +17,7 @@ obj-$(CONFIG_DW_APB_ICTL)\t\t+= irq-dw-apb-ictl.o\n> >>> obj-$(CONFIG_METAG)\t\t\t+= irq-metag-ext.o\n> >>> obj-$(CONFIG_METAG_PERFCOUNTER_IRQS)\t+= irq-metag.o\n> >>> obj-$(CONFIG_CLPS711X_IRQCHIP)\t\t+= irq-clps711x.o\n> >>> +obj-$(CONFIG_OMPIC)\t\t\t+= irq-ompic.o\n> >>> obj-$(CONFIG_OR1K_PIC)\t\t\t+= irq-or1k-pic.o\n> >>> obj-$(CONFIG_ORION_IRQCHIP)\t\t+= irq-orion.o\n> >>> obj-$(CONFIG_OMAP_IRQCHIP)\t\t+= irq-omap-intc.o\n> >>> diff --git a/drivers/irqchip/irq-ompic.c b/drivers/irqchip/irq-ompic.c\n> >>> new file mode 100644\n> >>> index 000000000000..438819f8a5a7\n> >>> --- /dev/null\n> >>> +++ b/drivers/irqchip/irq-ompic.c\n> >>> @@ -0,0 +1,117 @@\n> >>> +/*\n> >>> + * Open Multi-Processor Interrupt Controller driver\n> >>> + *\n> >>> + * Copyright (C) 2014 Stefan Kristiansson \n> >>> + *\n> >>> + * This file is licensed under the terms of the GNU General Public License\n> >>> + * version 2. This program is licensed \"as is\" without any warranty of any\n> >>> + * kind, whether express or implied.\n> >>> + */\n> >>> +\n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>\n> >> Don't think you need this.\n> >>\n> >>> +#include \n> >>\n> >> Nor this.\n> > \n> > OK on both.\n> > \n> >>> +\n> >>> +#include \n> >>> +\n> >>> +#define OMPIC_IPI_BASE\t\t\t0x0\n> >>> +#define OMPIC_IPI_CTRL(cpu)\t\t(OMPIC_IPI_BASE + 0x0 + (cpu)*8)\n> >>> +#define OMPIC_IPI_STAT(cpu)\t\t(OMPIC_IPI_BASE + 0x4 + (cpu)*8)\n> >>\n> >> In the DT binding you say that \"size can be arbitrary based on the\n> >> number of cores the controller has been configured to handle, typically\n> >> 8 bytes per core\". Here, this is 8 bytes, always, which is not exactly\n> >> the same. What is the architectural value, if any? If there is none,\n> >> then the per-core size should either come from DT or some other mean\n> >> (register?).\n> > \n> > I mean the address space is 8 bytes x number-of-cores. Thats what I meant\n> > by arbitrary, I guess its better for me to be explicit. There is no\n> > register that we can check to confirm the configuration of ompic. But I\n> > guess we can check the CPU NUMCORES register and compare it to the DT\n> > address space to do a sanity check.\n> \n> Thanks for the explanation. So the code is OK, but the DT should be more\n> rigorous in saying that there is 8 bytes per CPU. And yes to the check\n> if that can be done at this stage.\n\nI will update that.\n\n> > \n> >>> +\n> >>> +#define OMPIC_IPI_CTRL_IRQ_ACK\t\t(1 << 31)\n> >>> +#define OMPIC_IPI_CTRL_IRQ_GEN\t\t(1 << 30)\n> >>> +#define OMPIC_IPI_CTRL_DST(cpu)\t\t(((cpu) & 0x3fff) << 16)\n> >>> +\n> >>> +#define OMPIC_IPI_STAT_IRQ_PENDING\t(1 << 30)\n> >>> +\n> >>> +#define OMPIC_IPI_DATA(x)\t\t((x) & 0xffff)\n> >>> +\n> >>> +static struct {\n> >>> +\tunsigned long ops;\n> >>> +} ipi_data[NR_CPUS];\n> >>\n> >> In general, a per cpu data structure is better expressed as a percpu\n> >> data structure (yes, I'm in a funny mood this morning). Otherwise,\n> >> you're going to thrash more than just the receiver and the sender, but\n> >> also all the other CPUs that have their data in the same cache line.\n> > \n> > Right, that makes sense, I am not sure why that was done this way. I think\n> > I borrowed from alpha which has the extra __cacheline_aligned annotations.\n> > I forgot to come back and fix this. Ill do as you suggest, thank you.\n> > \n> > Excerpt from alpha:\n> > \n> > static struct {\n> > unsigned long bits ____cacheline_aligned;\n> > } ipi_data[NR_CPUS] __cacheline_aligned;\n> \n> Yup, __cacheline_aligned is key here, as it will have the same effect as\n> the percpu stuff from that point of view.\n\nRight, I think in this case I rather use the percpu API rather then\ndepending on how well our compiler implements the __cacheline_aligned\nannotations.\n\n> >>> +\n> >>> +static void __iomem *ompic_base;\n> >>> +\n> >>> +static inline u32 ompic_readreg(void __iomem *base, loff_t offset)\n> >>> +{\n> >>> +\treturn ioread32be(base + offset);\n> >>> +}\n> >>> +\n> >>> +static void ompic_writereg(void __iomem *base, loff_t offset, u32 data)\n> >>> +{\n> >>> +\tiowrite32be(data, base + offset);\n> >>> +}\n> >>> +\n> >>> +#ifdef CONFIG_SMP\n> >>\n> >> This code is only selected when CONFIG_SMP=y.\n> > \n> > Yes, that is right, as below:\n> > \n> > set_smp_cross_call(ompic_raise_softirq);\n> > \n> > The set_smp_cross_call() function from smp.c is only defined for smp. Do\n> > you think thats wrong or needed extra comments? This is similar to other\n> > chips in irqchip/ for archs which use set_smp_cross_call().\n> \n> Other irqchips can often be compiled for both SMP and !SMP, hence the\n> need of a #ifdef. In your case, this driver is only compiled when SMP is\n> selected, so you're pretty much guaranteed that this symbol is available.\n\nRight, I'll remove it.\n\n> > \n> >>> +void ompic_raise_softirq(const struct cpumask *mask, unsigned int irq)\n> >>> +{\n> >>\n> >> What is \"irq\" here? How is it guaranteed to fit in an unsigned long?\n> > \n> > Here irq is the `enum ipi_msg_type` which is guaranteed to fit in unsigned\n> > long. Porbably its better to rename as msg or ipi_msg?\n> \n> You should certainly make sure your \"enum ipi_msg_type\" is capped at the\n> number of bits of unsigned long. And yes to ipi_msg, which is a better\n> name than irq. Also, you could change the types of ompic_raise_softirq\n> and set_smp_cross_call, so that you use the enum instead of an int here.\n\nOK.\n\nI had a go at changing the type to the enum, but I realize that would\nrequire moving the enum definition into our asm/smp.h which I rather not do\nat the moment for sake of keeping it private inside of smp.c. This follows\nwhat other architectures do as well.\n\n> > \n> >>> +\tunsigned int dst_cpu;\n> >>> +\tunsigned int src_cpu = smp_processor_id();\n> >>> +\n> >>> +\tfor_each_cpu(dst_cpu, mask) {\n> >>> +\t\tset_bit(irq, &ipi_data[dst_cpu].ops);\n> >>> +\n> >>> +\t\tompic_writereg(ompic_base, OMPIC_IPI_CTRL(src_cpu),\n> >>> +\t\t\t OMPIC_IPI_CTRL_IRQ_GEN |\n> >>> +\t\t\t OMPIC_IPI_CTRL_DST(dst_cpu) |\n> >>> +\t\t\t OMPIC_IPI_DATA(1));\n> >>\n> >> What guarantees that the action of set_bit can be observed by the target\n> >> CPU? set-bit gives you atomicity, but no barrier.\n> > \n> > The bit will not be read by target CPUs until after the ompic_writereg()\n> > which will trigger the target CPU interrupt to be raised. OpenRISC stores\n> > are ordered.\n> \n> Are they really ordered irrespective of the memory type (one is\n> cacheable RAM, the other is a device...)?\n> \n> And assuming they are (which I find a bit odd), that doesn't necessarily\n> guarantee that the interrupt will only be delivered once the effect of\n> set_bit can be seen on the target CPU...\n\nOpenRISC might be a bit odd here, but I think this is correct. On OpenRISC\nthe atomic instructions also imply a pipeline flush for stores and loads\n(l.swa/l.lwa). This is highlighted in the architecture spec section 7.3 [0].\n\n[0] https://github.com/stffrdhrn/doc/raw/arch-1.2-proposal/openrisc-arch-1.2-rev0.pdf\n\n-Stafford\n\n> > This will work on OpenRISC, but should I be thinking of other architectures\n> > as well for all drivers? Or do you think this will be an issue on\n> > OpenRISC?\n> \n> I definitely think this could be an issue with OpenRISC, but only\n> someone familiar with the OpenRISC architecture can say whether I'm\n> right or wrong. I'm just guessing at the moment.\n> \n> [...]\n> \n> > Thank you for the feedback, I will clean this up and resubmit with the\n> > comments on the other thread.\n> > \n> > In terms of commit path, do you think its ok for this to go in via the\n> > OpenRISC arch path?\n> \n> Sure, that's fine by me.\n> \n> \tM.\n> -- \n> Jazz is not dead. It just smells funny...\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"WhqZLnTG\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xlnKQ1pXtz9sPs\n\tfor ;\n\tMon, 4 Sep 2017 08:12:42 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753020AbdICWMk (ORCPT\n\t);\n\tSun, 3 Sep 2017 18:12:40 -0400","from mail-pf0-f194.google.com ([209.85.192.194]:33839 \"EHLO\n\tmail-pf0-f194.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752974AbdICWMj (ORCPT\n\t); Sun, 3 Sep 2017 18:12:39 -0400","by mail-pf0-f194.google.com with SMTP id v22so3313505pfk.1;\n\tSun, 03 Sep 2017 15:12:39 -0700 (PDT)","from localhost (g186.58-98-166.ppp.wakwak.ne.jp. [58.98.166.186])\n\tby smtp.gmail.com with ESMTPSA id\n\ti186sm8586920pfg.81.2017.09.03.15.12.37\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tSun, 03 Sep 2017 15:12:37 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:in-reply-to:user-agent;\n\tbh=YgCkOxCOaAqnD2EiD7ONwi/noE4AIGWA+aqb0BGkvfI=;\n\tb=WhqZLnTGHnS44wBi2WRnFXrJNVJ63s5TWjbte2L+wm/yiWu64SD9kMs/hKiCFvTQgn\n\tIPOqvyiTbTsUdV/dKqAWLSNIZ12xLrTIWMGnLo6YIigzVr+IKbGFNN+CMFPNvBPkSNLv\n\tBqm8j6Y1gLMXscH2kXby/327gKi9pwxjTDexXeXl2HCxaYfYMJdvBJGNdQvWpEkaYJdC\n\tr9aAmfh+E3DsZ+Vg7FfDka+v4XBjdqv2Zujf8AbThF3WMkB30PRuv3XTiEpojFhBvKe+\n\t89HLN7SPBzD0T91bHTrVvcAMg2Y2jxgREbiLKlntRaTnIazb+nIREMFdKoQPIiA9bmK/\n\tx+5A==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:in-reply-to:user-agent;\n\tbh=YgCkOxCOaAqnD2EiD7ONwi/noE4AIGWA+aqb0BGkvfI=;\n\tb=lH6lw16J2k5VbU4MCs4A7ljofHhQBbv+BNPHcHsDBT3U9kn+Qi0YkYW1hJ/9CZTFy0\n\t8/Garxd8wSy/kkVRiTc1bqTDdLQ/fEcNPDBiLrks5jGjBfMjwNx4rBdnfcRpg82zFNpm\n\tcI07rgfl4L3ByDJocYJvoG6advKCfhtpweTUQJTxYRY+0wr9Fp5Ss54T/W7Eg8CbMOS2\n\tkeu2PN+MsrPZwiPtJd0Zh7FfxWIQGdsBAdNn05qRSZAHNg8jgCtcOoBrpD1EqwKdfwis\n\tlZ6SDxnmzipRhgroCIBdfPpGT/fKdItivL2W06Th11+5tg7vNyLlrJrw7Ti9GoOk9dGq\n\tNw7g==","X-Gm-Message-State":"AHPjjUiMCNjnVkmgz8nSgJH7S7UyJrzSP0HCZrg+XWhW55cxynvfshv+\n\ttKyC7H9pEApWeFH3s0GB8w==","X-Google-Smtp-Source":"ADKCNb6ie+iahmSy1sfHl5SVsTmekWjjTfBvp52nuCbwuKcHV0tDjmmqMZbViD+8PN5SEOgvUIUWtw==","X-Received":"by 10.101.88.65 with SMTP id s1mr10180581pgr.35.1504476758560;\n\tSun, 03 Sep 2017 15:12:38 -0700 (PDT)","Date":"Mon, 4 Sep 2017 07:12:36 +0900","From":"Stafford Horne ","To":"Marc Zyngier ","Cc":"LKML ,\n\tOpenrisc ,\n\tStefan Kristiansson ,\n\tThomas Gleixner ,\n\tJason Cooper , \n\tRob Herring , Mark Rutland ,\n\tJonas Bonn , devicetree@vger.kernel.org","Subject":"Re: [PATCH 05/13] irqchip: add initial support for ompic","Message-ID":"<20170903221236.GM2609@lianli.shorne-pla.net>","References":"\n\t<538699c64b5601e8800b77da29f7951bf23f57ce.1504129273.git.shorne@gmail.com>\n\t<1b062d84-7335-8553-39c7-2e60973b4396@arm.com>\n\t<20170901012449.GG2609@lianli.shorne-pla.net>\n\t<97879c84-3ce8-b2bf-d438-679a69b60774@arm.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<97879c84-3ce8-b2bf-d438-679a69b60774@arm.com>","User-Agent":"Mutt/1.8.3 (2017-05-23)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1762480,"web_url":"http://patchwork.ozlabs.org/comment/1762480/","msgid":"","list_archive_url":null,"date":"2017-09-04T07:35:19","subject":"Re: [PATCH 05/13] irqchip: add initial support for ompic","submitter":{"id":7353,"url":"http://patchwork.ozlabs.org/api/people/7353/","name":"Marc Zyngier","email":"marc.zyngier@arm.com"},"content":"On 03/09/17 23:12, Stafford Horne wrote:\n> On Fri, Sep 01, 2017 at 06:25:01PM +0100, Marc Zyngier wrote:\n>> On 01/09/17 02:24, Stafford Horne wrote:\n>>> On Thu, Aug 31, 2017 at 10:28:01AM +0100, Marc Zyngier wrote:\n>>>> On 30/08/17 22:58, Stafford Horne wrote:\n>>>>> From: Stefan Kristiansson \n\n[...]\n\n>>>>> +\tunsigned int dst_cpu;\n>>>>> +\tunsigned int src_cpu = smp_processor_id();\n>>>>> +\n>>>>> +\tfor_each_cpu(dst_cpu, mask) {\n>>>>> +\t\tset_bit(irq, &ipi_data[dst_cpu].ops);\n>>>>> +\n>>>>> +\t\tompic_writereg(ompic_base, OMPIC_IPI_CTRL(src_cpu),\n>>>>> +\t\t\t OMPIC_IPI_CTRL_IRQ_GEN |\n>>>>> +\t\t\t OMPIC_IPI_CTRL_DST(dst_cpu) |\n>>>>> +\t\t\t OMPIC_IPI_DATA(1));\n>>>>\n>>>> What guarantees that the action of set_bit can be observed by the target\n>>>> CPU? set-bit gives you atomicity, but no barrier.\n>>>\n>>> The bit will not be read by target CPUs until after the ompic_writereg()\n>>> which will trigger the target CPU interrupt to be raised. OpenRISC stores\n>>> are ordered.\n>>\n>> Are they really ordered irrespective of the memory type (one is\n>> cacheable RAM, the other is a device...)?\n>>\n>> And assuming they are (which I find a bit odd), that doesn't necessarily\n>> guarantee that the interrupt will only be delivered once the effect of\n>> set_bit can be seen on the target CPU...\n> \n> OpenRISC might be a bit odd here, but I think this is correct. On OpenRISC\n> the atomic instructions also imply a pipeline flush for stores and loads\n> (l.swa/l.lwa). This is highlighted in the architecture spec section 7.3 [0].\n\nOK, fair enough. This is quite unusual (and I'm prepared to bet that\nthis will be relaxed in future evolutions of the architecture), but\nthat's indeed the gospel.\n\nPlease add a comment between set_bit and writereg so that we know we've\nbeen through this discussion already.\n\nThanks,\n\n\tM.","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xm1pj1KKRz9s82\n\tfor ;\n\tMon, 4 Sep 2017 17:35:25 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753254AbdIDHfX (ORCPT\n\t);\n\tMon, 4 Sep 2017 03:35:23 -0400","from foss.arm.com ([217.140.101.70]:54822 \"EHLO foss.arm.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751949AbdIDHfX (ORCPT );\n\tMon, 4 Sep 2017 03:35:23 -0400","from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249])\n\tby usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 81A8280D;\n\tMon, 4 Sep 2017 00:35:22 -0700 (PDT)","from [10.1.206.41] (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249])\n\tby usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id\n\tBB0E63F58F; Mon, 4 Sep 2017 00:35:20 -0700 (PDT)"],"Subject":"Re: [PATCH 05/13] irqchip: add initial support for ompic","To":"Stafford Horne ","Cc":"LKML ,\n\tOpenrisc ,\n\tStefan Kristiansson ,\n\tThomas Gleixner ,\n\tJason Cooper , \n\tRob Herring , Mark Rutland ,\n\tJonas Bonn , devicetree@vger.kernel.org","References":"\n\t<538699c64b5601e8800b77da29f7951bf23f57ce.1504129273.git.shorne@gmail.com>\n\t<1b062d84-7335-8553-39c7-2e60973b4396@arm.com>\n\t<20170901012449.GG2609@lianli.shorne-pla.net>\n\t<97879c84-3ce8-b2bf-d438-679a69b60774@arm.com>\n\t<20170903221236.GM2609@lianli.shorne-pla.net>","From":"Marc Zyngier ","Organization":"ARM Ltd","Message-ID":"","Date":"Mon, 4 Sep 2017 08:35:19 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170903221236.GM2609@lianli.shorne-pla.net>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}}]