On 01/09/17 02:24, Stafford Horne wrote: > On Thu, Aug 31, 2017 at 10:28:01AM +0100, Marc Zyngier wrote: >> On 30/08/17 22:58, Stafford Horne wrote: >>> From: Stefan Kristiansson <stefan.kristiansson@xxxxxxxxxxxxx> >>> >>> IPI driver for OpenRISC Multicore programmable interrupt controller as >>> described in the Multicore support section of the OpenRISC 1.2 >>> proposed architecture specification: >>> >>> https://github.com/stffrdhrn/doc/raw/arch-1.2-proposal/openrisc-arch-1.2-rev0.pdf >>> >>> Each OpenRISC core contains a full interrupt controller which is used in >>> the SMP architecture for interrupt balancing. This IPI device is the >>> only external device required for enabling SMP on OpenRISC. >>> >>> Pending ops are stored in a memory bit mask which can allow multiple >>> pending operations to be set and serviced at a time. This is mostly >>> borrowed from the alpha IPI implementation. >>> >>> Signed-off-by: Stefan Kristiansson <stefan.kristiansson@xxxxxxxxxxxxx> >>> [shorne@xxxxxxxxx: converted ops to bitmask, wrote commit message] >>> Signed-off-by: Stafford Horne <shorne@xxxxxxxxx> >>> --- >>> .../bindings/interrupt-controller/ompic.txt | 22 ++++ >>> arch/openrisc/Kconfig | 1 + >>> drivers/irqchip/Kconfig | 4 + >>> drivers/irqchip/Makefile | 1 + >>> drivers/irqchip/irq-ompic.c | 117 +++++++++++++++++++++ >>> 5 files changed, 145 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ompic.txt >>> create mode 100644 drivers/irqchip/irq-ompic.c >>> >>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/ompic.txt b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt >>> new file mode 100644 >>> index 000000000000..4176ecc3366d >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/interrupt-controller/ompic.txt >>> @@ -0,0 +1,22 @@ >>> +OpenRISC Multicore Programmable Interrupt Controller >>> + >>> +Required properties: >>> + >>> +- compatible : This should be "ompic" >>> +- reg : Specifies base physical address and size of the register space. The >>> + size can be arbitrary based on the number of cores the controller has >>> + been configured to handle, typically 8 bytes per core. >>> +- interrupt-controller : Identifies the node as an interrupt controller >>> +- #interrupt-cells : Specifies the number of cells needed to encode an >>> + interrupt source. The value shall be 1. >>> +- interrupts : Specifies the interrupt line to which the ompic is wired. >>> + >>> +Example: >>> + >>> +ompic: ompic { >>> + compatible = "ompic"; >>> + reg = <0x98000000 16>; >>> + #interrupt-cells = <1>; >>> + interrupt-controller; >>> + interrupts = <1>; >>> +}; >>> diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig >>> index 214c837ce597..dd7e55e7e42d 100644 >>> --- a/arch/openrisc/Kconfig >>> +++ b/arch/openrisc/Kconfig >>> @@ -30,6 +30,7 @@ config OPENRISC >>> select NO_BOOTMEM >>> select ARCH_USE_QUEUED_SPINLOCKS >>> select ARCH_USE_QUEUED_RWLOCKS >>> + select OMPIC if SMP >>> >>> config CPU_BIG_ENDIAN >>> def_bool y >>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig >>> index f1fd5f44d1d4..3fa60e6667a7 100644 >>> --- a/drivers/irqchip/Kconfig >>> +++ b/drivers/irqchip/Kconfig >>> @@ -145,6 +145,10 @@ config CLPS711X_IRQCHIP >>> select SPARSE_IRQ >>> default y >>> >>> +config OMPIC >>> + bool >>> + select IRQ_DOMAIN >> >> Why do you need to select IRQ_DOMAIN? This driver doesn't seem to use any... > > Right, I will look to remove that. > >>> + >>> config OR1K_PIC >>> bool >>> select IRQ_DOMAIN >>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile >>> index e88d856cc09c..123047d7a20d 100644 >>> --- a/drivers/irqchip/Makefile >>> +++ b/drivers/irqchip/Makefile >>> @@ -17,6 +17,7 @@ obj-$(CONFIG_DW_APB_ICTL) += irq-dw-apb-ictl.o >>> obj-$(CONFIG_METAG) += irq-metag-ext.o >>> obj-$(CONFIG_METAG_PERFCOUNTER_IRQS) += irq-metag.o >>> obj-$(CONFIG_CLPS711X_IRQCHIP) += irq-clps711x.o >>> +obj-$(CONFIG_OMPIC) += irq-ompic.o >>> obj-$(CONFIG_OR1K_PIC) += irq-or1k-pic.o >>> obj-$(CONFIG_ORION_IRQCHIP) += irq-orion.o >>> obj-$(CONFIG_OMAP_IRQCHIP) += irq-omap-intc.o >>> diff --git a/drivers/irqchip/irq-ompic.c b/drivers/irqchip/irq-ompic.c >>> new file mode 100644 >>> index 000000000000..438819f8a5a7 >>> --- /dev/null >>> +++ b/drivers/irqchip/irq-ompic.c >>> @@ -0,0 +1,117 @@ >>> +/* >>> + * Open Multi-Processor Interrupt Controller driver >>> + * >>> + * Copyright (C) 2014 Stefan Kristiansson <stefan.kristiansson@xxxxxxxxxxxxx> >>> + * >>> + * This file is licensed under the terms of the GNU General Public License >>> + * version 2. This program is licensed "as is" without any warranty of any >>> + * kind, whether express or implied. >>> + */ >>> + >>> +#include <linux/io.h> >>> +#include <linux/interrupt.h> >>> +#include <linux/smp.h> >>> +#include <linux/of.h> >>> +#include <linux/of_irq.h> >>> +#include <linux/of_address.h> >>> +#include <linux/irqchip/chained_irq.h> >> >> Don't think you need this. >> >>> +#include <linux/delay.h> >> >> Nor this. > > OK on both. > >>> + >>> +#include <linux/irqchip.h> >>> + >>> +#define OMPIC_IPI_BASE 0x0 >>> +#define OMPIC_IPI_CTRL(cpu) (OMPIC_IPI_BASE + 0x0 + (cpu)*8) >>> +#define OMPIC_IPI_STAT(cpu) (OMPIC_IPI_BASE + 0x4 + (cpu)*8) >> >> In the DT binding you say that "size can be arbitrary based on the >> number of cores the controller has been configured to handle, typically >> 8 bytes per core". Here, this is 8 bytes, always, which is not exactly >> the same. What is the architectural value, if any? If there is none, >> then the per-core size should either come from DT or some other mean >> (register?). > > I mean the address space is 8 bytes x number-of-cores. Thats what I meant > by arbitrary, I guess its better for me to be explicit. There is no > register that we can check to confirm the configuration of ompic. But I > guess we can check the CPU NUMCORES register and compare it to the DT > address space to do a sanity check. Thanks for the explanation. So the code is OK, but the DT should be more rigorous in saying that there is 8 bytes per CPU. And yes to the check if that can be done at this stage. > >>> + >>> +#define OMPIC_IPI_CTRL_IRQ_ACK (1 << 31) >>> +#define OMPIC_IPI_CTRL_IRQ_GEN (1 << 30) >>> +#define OMPIC_IPI_CTRL_DST(cpu) (((cpu) & 0x3fff) << 16) >>> + >>> +#define OMPIC_IPI_STAT_IRQ_PENDING (1 << 30) >>> + >>> +#define OMPIC_IPI_DATA(x) ((x) & 0xffff) >>> + >>> +static struct { >>> + unsigned long ops; >>> +} ipi_data[NR_CPUS]; >> >> In general, a per cpu data structure is better expressed as a percpu >> data structure (yes, I'm in a funny mood this morning). Otherwise, >> you're going to thrash more than just the receiver and the sender, but >> also all the other CPUs that have their data in the same cache line. > > Right, that makes sense, I am not sure why that was done this way. I think > I borrowed from alpha which has the extra __cacheline_aligned annotations. > I forgot to come back and fix this. Ill do as you suggest, thank you. > > Excerpt from alpha: > > static struct { > unsigned long bits ____cacheline_aligned; > } ipi_data[NR_CPUS] __cacheline_aligned; Yup, __cacheline_aligned is key here, as it will have the same effect as the percpu stuff from that point of view. >>> + >>> +static void __iomem *ompic_base; >>> + >>> +static inline u32 ompic_readreg(void __iomem *base, loff_t offset) >>> +{ >>> + return ioread32be(base + offset); >>> +} >>> + >>> +static void ompic_writereg(void __iomem *base, loff_t offset, u32 data) >>> +{ >>> + iowrite32be(data, base + offset); >>> +} >>> + >>> +#ifdef CONFIG_SMP >> >> This code is only selected when CONFIG_SMP=y. > > Yes, that is right, as below: > > set_smp_cross_call(ompic_raise_softirq); > > The set_smp_cross_call() function from smp.c is only defined for smp. Do > you think thats wrong or needed extra comments? This is similar to other > chips in irqchip/ for archs which use set_smp_cross_call(). Other irqchips can often be compiled for both SMP and !SMP, hence the need of a #ifdef. In your case, this driver is only compiled when SMP is selected, so you're pretty much guaranteed that this symbol is available. > >>> +void ompic_raise_softirq(const struct cpumask *mask, unsigned int irq) >>> +{ >> >> What is "irq" here? How is it guaranteed to fit in an unsigned long? > > Here irq is the `enum ipi_msg_type` which is guaranteed to fit in unsigned > long. Porbably its better to rename as msg or ipi_msg? You should certainly make sure your "enum ipi_msg_type" is capped at the number of bits of unsigned long. And yes to ipi_msg, which is a better name than irq. Also, you could change the types of ompic_raise_softirq and set_smp_cross_call, so that you use the enum instead of an int here. > >>> + unsigned int dst_cpu; >>> + unsigned int src_cpu = smp_processor_id(); >>> + >>> + for_each_cpu(dst_cpu, mask) { >>> + set_bit(irq, &ipi_data[dst_cpu].ops); >>> + >>> + ompic_writereg(ompic_base, OMPIC_IPI_CTRL(src_cpu), >>> + OMPIC_IPI_CTRL_IRQ_GEN | >>> + OMPIC_IPI_CTRL_DST(dst_cpu) | >>> + OMPIC_IPI_DATA(1)); >> >> What guarantees that the action of set_bit can be observed by the target >> CPU? set-bit gives you atomicity, but no barrier. > > The bit will not be read by target CPUs until after the ompic_writereg() > which will trigger the target CPU interrupt to be raised. OpenRISC stores > are ordered. Are they really ordered irrespective of the memory type (one is cacheable RAM, the other is a device...)? And assuming they are (which I find a bit odd), that doesn't necessarily guarantee that the interrupt will only be delivered once the effect of set_bit can be seen on the target CPU... > This will work on OpenRISC, but should I be thinking of other architectures > as well for all drivers? Or do you think this will be an issue on > OpenRISC? I definitely think this could be an issue with OpenRISC, but only someone familiar with the OpenRISC architecture can say whether I'm right or wrong. I'm just guessing at the moment. [...] > Thank you for the feedback, I will clean this up and resubmit with the > comments on the other thread. > > In terms of commit path, do you think its ok for this to go in via the > OpenRISC arch path? Sure, that's fine by me. M. -- Jazz is not dead. It just smells funny... -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
