[05/13] irqchip: add initial support for ompic

From: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>

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@saunalahti.fi>
[shorne@gmail.com: converted ops to bitmask, wrote commit message]
Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 .../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

On 30/08/17 22:58, Stafford Horne wrote:
> From: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
> 
> 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@saunalahti.fi>
> [shorne@gmail.com: converted ops to bitmask, wrote commit message]
> Signed-off-by: Stafford Horne <shorne@gmail.com>
> ---
>  .../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...

> +
>  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@saunalahti.fi>
> + *
> + * 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.

> +
> +#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?).

> +
> +#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.

> +
> +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.

> +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?

> +	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.

> +	}
> +}
> +#endif
> +
> +irqreturn_t ompic_ipi_handler(int irq, void *dev_id)
> +{
> +	unsigned int cpu = smp_processor_id();
> +	unsigned long *pending_ops = &ipi_data[cpu].ops;
> +	unsigned long ops;
> +
> +	ompic_writereg(ompic_base, OMPIC_IPI_CTRL(cpu), OMPIC_IPI_CTRL_IRQ_ACK);
> +	while ((ops = xchg(pending_ops, 0)) != 0) {

Barrier again?

> +		do {
> +			unsigned long ipi;
> +
> +			ipi = ops & -ops;
> +			ops &= ~ipi;
> +			ipi = __ffs(ipi);

This feels pointlessly convoluted. Is there any reason why you can't
write it as:

			ipi = __ffs(ops);
			ops &= ~(1UL << ipi);

which feels like a much more common idiom?

> +
> +			handle_IPI(ipi);
> +		} while (ops);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static struct irqaction ompi_ipi_irqaction = {
> +	.handler =      ompic_ipi_handler,
> +	.flags =        IRQF_PERCPU,
> +	.name =         "ompic_ipi",
> +};
> +
> +#ifdef CONFIG_OF

This code is useless if you don't have CONFIG_OF. So either you make the
driver depend on CONFIG_OF, or you actively select it. And given that
CONFIG_OPENRISC already selects CONFIG_OF, you can happily get rid of
all the #ifdefery here.

> +int __init ompic_of_init(struct device_node *node, struct device_node *parent)
> +{
> +	int irq;
> +
> +	if (WARN_ON(!node))
> +		return -ENODEV;

How do you end-up here if node == NULL?

> +
> +	memset(ipi_data, 0, sizeof(ipi_data));

The kernel should do this for you already.

> +
> +	ompic_base = of_iomap(node, 0);
> +
> +	irq = irq_of_parse_and_map(node, 0);
> +	setup_irq(irq, &ompi_ipi_irqaction);
> +
> +#ifdef CONFIG_SMP
> +	set_smp_cross_call(ompic_raise_softirq);
> +#endif
> +
> +	return 0;
> +}
> +IRQCHIP_DECLARE(ompic, "ompic", ompic_of_init);
> +#endif
> 

Thanks,

	M.

On Thu, Aug 31, 2017 at 06:58:36AM +0900, Stafford Horne wrote:
> From: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
> 
> 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.

I'm a little confused. Is this device the whole "ompic", a sub-device
thereof, or an independent IP block connected to it?

> 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@saunalahti.fi>
> [shorne@gmail.com: converted ops to bitmask, wrote commit message]
> Signed-off-by: Stafford Horne <shorne@gmail.com>
> ---
>  .../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"

This needs a vendor prefix.

Presumably, this should be "opencores,ompic"? (pending whether this is
actually the whole ompic, as above).

> +- 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.

How are those regions correlated with CPUs?

e.g. is there a fixed relationship with a physical CPU id, or some
mechanism by which the relationship can be probed dynamically?

> +- 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.

No flags? Does this not need edge/level configuration or active high/low
configuration?

> +- interrupts : Specifies the interrupt line to which the ompic is wired.

What is this interrupt used for?

Is this some percpu interrupt used to proxy the IPI? It feels very odd
to assume such a thing from the parent irqchip. Surely this is
intimately coupled with that?

> +
> +Example:
> +
> +ompic: ompic {
> +	compatible = "ompic";
> +	reg = <0x98000000 16>;
> +	#interrupt-cells = <1>;
> +	interrupt-controller;
> +	interrupts = <1>;
> +};

[...]

> +static struct {
> +	unsigned long ops;
> +} ipi_data[NR_CPUS];
> +
> +static void __iomem *ompic_base;

Is there guaranteed to only be one of these system-wide?

[...]

> +void ompic_raise_softirq(const struct cpumask *mask, unsigned int irq)
> +{
> +	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));
> +	}

Here you assume that the mapping is big enough to cover these registers,
but the ompic_of_init() function didn't sanity-check the size, so this
is not guaranteed.

[...]

> +int __init ompic_of_init(struct device_node *node, struct device_node *parent)
> +{
> +	int irq;
> +
> +	if (WARN_ON(!node))
> +		return -ENODEV;

Given this function is only invoked if the kernel found a node with a
matching compatible string, how can this possibly happen?

> +	memset(ipi_data, 0, sizeof(ipi_data));

As ipi_data was static, it is already zeroed.

> +
> +	ompic_base = of_iomap(node, 0);

What if this failed?

> +
> +	irq = irq_of_parse_and_map(node, 0);

What if this is missing?

> +	setup_irq(irq, &ompi_ipi_irqaction);

As covered earlier, I;m confused by this. I'd expect that your root
irqchip would handle the IPIs, and hence need to probe nothing from the
DT for those.

Here we're assuming the IRQ is wired up to some per-cpu interrupt that
we can treat as an IPI, and that the parent irqchip handles that
appropriately, which seems very odd.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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@saunalahti.fi>
> > 
> > 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@saunalahti.fi>
> > [shorne@gmail.com: converted ops to bitmask, wrote commit message]
> > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > ---
> >  .../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@saunalahti.fi>
> > + *
> > + * 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.

> > +
> > +#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;

> > +
> > +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().

> > +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?

> > +	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.

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?

> > +	}
> > +}
> > +#endif
> > +
> > +irqreturn_t ompic_ipi_handler(int irq, void *dev_id)
> > +{
> > +	unsigned int cpu = smp_processor_id();
> > +	unsigned long *pending_ops = &ipi_data[cpu].ops;
> > +	unsigned long ops;
> > +
> > +	ompic_writereg(ompic_base, OMPIC_IPI_CTRL(cpu), OMPIC_IPI_CTRL_IRQ_ACK);
> > +	while ((ops = xchg(pending_ops, 0)) != 0) {
> 
> Barrier again?

Thanks, but some concern of above.

> > +		do {
> > +			unsigned long ipi;
> > +
> > +			ipi = ops & -ops;
> > +			ops &= ~ipi;
> > +			ipi = __ffs(ipi);
> 
> This feels pointlessly convoluted. Is there any reason why you can't
> write it as:
> 
> 			ipi = __ffs(ops);
> 			ops &= ~(1UL << ipi);
> 
> which feels like a much more common idiom?

Right, this should work, not sure what I was thinking above right now.  Let
me give that a try.

> > +
> > +			handle_IPI(ipi);
> > +		} while (ops);
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static struct irqaction ompi_ipi_irqaction = {
> > +	.handler =      ompic_ipi_handler,
> > +	.flags =        IRQF_PERCPU,
> > +	.name =         "ompic_ipi",
> > +};
> > +
> > +#ifdef CONFIG_OF
> 
> This code is useless if you don't have CONFIG_OF. So either you make the
> driver depend on CONFIG_OF, or you actively select it. And given that
> CONFIG_OPENRISC already selects CONFIG_OF, you can happily get rid of
> all the #ifdefery here.

Thanks, right.

> > +int __init ompic_of_init(struct device_node *node, struct device_node *parent)
> > +{
> > +	int irq;
> > +
> > +	if (WARN_ON(!node))
> > +		return -ENODEV;
> 
> How do you end-up here if node == NULL?

Right.

> > +
> > +	memset(ipi_data, 0, sizeof(ipi_data));
> 
> The kernel should do this for you already.

Right.

> > +
> > +	ompic_base = of_iomap(node, 0);
> > +
> > +	irq = irq_of_parse_and_map(node, 0);
> > +	setup_irq(irq, &ompi_ipi_irqaction);
> > +
> > +#ifdef CONFIG_SMP
> > +	set_smp_cross_call(ompic_raise_softirq);
> > +#endif
> > +
> > +	return 0;
> > +}
> > +IRQCHIP_DECLARE(ompic, "ompic", ompic_of_init);
> > +#endif
> >

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?

-Stafford
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Aug 31, 2017 at 11:59:40AM +0100, Mark Rutland wrote:
> On Thu, Aug 31, 2017 at 06:58:36AM +0900, Stafford Horne wrote:
> > From: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
> > 
> > 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.
> 
> I'm a little confused. Is this device the whole "ompic", a sub-device
> thereof, or an independent IP block connected to it?

This device *is* the ompic, it currently just handles IPI communication,
but was originally thought it should do more, i.e. distribute device
interrupts, hence the name.  For now, the ompic device only handles IPI.

Perhaps we can change the name?  If you think that's needed I can discuss
with Stefan (the creator).

This is documented in the pdf, but here is an ascii diagram to help.

Notes

 o The ompic generates a level interrupt to the CPU PIC when a message is
   ready.  Messages are delivered via the memory bus.
 o The ompic does not have any interrupt input lines.
 o The ompic must be wired to the same irq line on each core.
 o Devices must be wired to the same irq line on each core.
 o This seems strange, but in the end very little extra hardware is needed
   to enable smp.

  +---------+                         +---------+
  | CPU     |                         | CPU     |
  |  Core 0 |<==\ (memory access) /==>|  Core 1 |
  |  [ PIC ]|   |                 |   |  [ PIC ]|
  +----^-^--+   |                 |   +----^-^--+
       | |      v                 v        | |
  <====|=|=================================|=|==> (memory bus)
       | |      ^                  ^       | |
 (ipi  | +------|---------+--------|-------|-+ (device irq)
  irq  |        |         |        |       |
 core0)| +------|---------|--------|-------+ (ipi irq core1)
       | |      |         |        |
  +----o-o-+    |    +--------+    |
  | ompic  |<===/    | Device |<===/
  |  IPI   |         +--------+
  +--------+

> > 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@saunalahti.fi>
> > [shorne@gmail.com: converted ops to bitmask, wrote commit message]
> > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > ---
> >  .../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"
> 
> This needs a vendor prefix.
> 
> Presumably, this should be "opencores,ompic"? (pending whether this is
> actually the whole ompic, as above).

As discussed above this is the ompic.

In terms of vendor, this and further openrisc development is no longer
associated with opencores, that company just hosts the old opencores.org
website but is not assisting in openrisc development any longer.  Perhaps
"openrisc,ompic" or "openrisc,ipi".

This also I would like to get final say from Stefan, but any suggestions
are welcome.

> > +- 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.
> 
> How are those regions correlated with CPUs?
> 
> e.g. is there a fixed relationship with a physical CPU id, or some
> mechanism by which the relationship can be probed dynamically?

There should only be one region.

core id 0 is associated with registers at address 0x0 and 0x4
core id 1 is associated with registers at address 0x8 and 0xc
etc.

This is can't be probed.  Should this be documented here?  That seems more
of something that is internal to the driver. I think the only thing some
one would want to set in TD is the register space size which should be be 8
x number-of-cores.

> > +- 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.
> 
> No flags? Does this not need edge/level configuration or active high/low
> configuration?

No flags, this maybe I am confused on, the ompic does not receive any
interrupts.  The input to the ompic are register writes which raise/clear
interrupts on the target CPUs.  So maybe it should not define an
'interrupt-controller' or '#interrupt-cells' tag.  But then should it be an
irqchip?  Since its facilitates the IPI I think this is the right place.

> > +- interrupts : Specifies the interrupt line to which the ompic is wired.
> 
> What is this interrupt used for?
> 
> Is this some percpu interrupt used to proxy the IPI? It feels very odd
> to assume such a thing from the parent irqchip. Surely this is
> intimately coupled with that?

This represents which IRQ line on each cpu the IPI is connected to (it must
be connected to the same IRQ line on each CPU.

i.e.
 interrupt-parent = <&pic>;

> > +
> > +Example:
> > +
> > +ompic: ompic {
> > +	compatible = "ompic";
> > +	reg = <0x98000000 16>;
> > +	#interrupt-cells = <1>;
> > +	interrupt-controller;
> > +	interrupts = <1>;
> > +};
> 
> [...]
> 
> > +static struct {
> > +	unsigned long ops;
> > +} ipi_data[NR_CPUS];
> > +
> > +static void __iomem *ompic_base;
> 
> Is there guaranteed to only be one of these system-wide?

Yes, that should be guaranteed, but I can look for moving this into a per
device structure.

> [...]
> 
> > +void ompic_raise_softirq(const struct cpumask *mask, unsigned int irq)
> > +{
> > +	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));
> > +	}
> 
> Here you assume that the mapping is big enough to cover these registers,
> but the ompic_of_init() function didn't sanity-check the size, so this
> is not guaranteed.

OK, I will add some sanity checking.

> [...]
> 
> > +int __init ompic_of_init(struct device_node *node, struct device_node *parent)
> > +{
> > +	int irq;
> > +
> > +	if (WARN_ON(!node))
> > +		return -ENODEV;
> 
> Given this function is only invoked if the kernel found a node with a
> matching compatible string, how can this possibly happen?

Right, I think now.  Will fix.

> > +	memset(ipi_data, 0, sizeof(ipi_data));
> 
> As ipi_data was static, it is already zeroed.

Right, will fix.

> > +
> > +	ompic_base = of_iomap(node, 0);
> 
> What if this failed?

Right will fix.

> > +
> > +	irq = irq_of_parse_and_map(node, 0);
> 
> What if this is missing?

OK, I will do some error checking.

> 
> > +	setup_irq(irq, &ompi_ipi_irqaction);
> 
> As covered earlier, I;m confused by this. I'd expect that your root
> irqchip would handle the IPIs, and hence need to probe nothing from the
> DT for those.
> 
> Here we're assuming the IRQ is wired up to some per-cpu interrupt that
> we can treat as an IPI, and that the parent irqchip handles that
> appropriately, which seems very odd.

I hope the comments above help to clarify this.  By appropriately you mean
level/edge active high/low?  The ompic and openrisc internal pic have been
designed to be compatible.

Thanks for the review.  Between this and the comments from Marc I have a
bit of work to do.  Please let me know if you have any further questions.

-Stafford

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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@saunalahti.fi>
>>>
>>> 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@saunalahti.fi>
>>> [shorne@gmail.com: converted ops to bitmask, wrote commit message]
>>> Signed-off-by: Stafford Horne <shorne@gmail.com>
>>> ---
>>>  .../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@saunalahti.fi>
>>> + *
>>> + * 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.

On Fri, Sep 01, 2017 at 06:25:01PM +0100, Marc Zyngier wrote:
> 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@saunalahti.fi>
> >>>
> >>> 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@saunalahti.fi>
> >>> [shorne@gmail.com: converted ops to bitmask, wrote commit message]
> >>> Signed-off-by: Stafford Horne <shorne@gmail.com>
> >>> ---
> >>>  .../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@saunalahti.fi>
> >>> + *
> >>> + * 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.

I will update that.

> > 
> >>> +
> >>> +#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.

Right, I think in this case I rather use the percpu API rather then
depending on how well our compiler implements the __cacheline_aligned
annotations.

> >>> +
> >>> +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.

Right, I'll remove it.

> > 
> >>> +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.

OK.

I had a go at changing the type to the enum, but I realize that would
require moving the enum definition into our asm/smp.h which I rather not do
at the moment for sake of keeping it private inside of smp.c.  This follows
what other architectures do as well.

> > 
> >>> +	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...

OpenRISC might be a bit odd here, but I think this is correct.  On OpenRISC
the atomic instructions also imply a pipeline flush for stores and loads
(l.swa/l.lwa).  This is highlighted in the architecture spec section 7.3 [0].

[0] https://github.com/stffrdhrn/doc/raw/arch-1.2-proposal/openrisc-arch-1.2-rev0.pdf

-Stafford

> > 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 03/09/17 23:12, Stafford Horne wrote:
> On Fri, Sep 01, 2017 at 06:25:01PM +0100, Marc Zyngier wrote:
>> 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@saunalahti.fi>

[...]

>>>>> +	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...
> 
> OpenRISC might be a bit odd here, but I think this is correct.  On OpenRISC
> the atomic instructions also imply a pipeline flush for stores and loads
> (l.swa/l.lwa).  This is highlighted in the architecture spec section 7.3 [0].

OK, fair enough. This is quite unusual (and I'm prepared to bet that
this will be relaxed in future evolutions of the architecture), but
that's indeed the gospel.

Please add a comment between set_bit and writereg so that we know we've
been through this discussion already.

Thanks,

	M.