[PATCH v3 0/2] powerpc: define and implement MPIC message register support

[PATCH v3 0/2] powerpc: define and implement MPIC message register support

[Meador Inge]

This patch set defines a binding for FSL MPIC message registers and implements
an API for accessing those message registers.  Testing was done on a MPC8572DS
in an Linux-Linux AMP setup using OpenMCAPI (www.openmcapi.org) to communicate
between OS instances.  The message register API is used by the OpenMCAPI shared 
memory driver to send notifications between cores.

* v3 - Clarified a point in the binding concerning the length of the
       'interrupts' property.  Pointed out by Scott Wood.

* v2 - Incorporate feedback from Scott Wood
   * Make binding less implementation specific.
   * Add 'mpic-' prefix to message register node properties and aliases.
   * Remove 'interrupt-parent' from binding.
   * Fixed some example bugs with receive masks.

Signed-off-by: Meador Inge <meador_inge-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
Cc: Hollis Blanchard <hollis_blanchard-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>

Meador Inge (2):
  powerpc: document the FSL MPIC message register binding
  powerpc: add support for MPIC message register API

 .../devicetree/bindings/powerpc/fsl/mpic-msgr.txt  |   62 +++++
 arch/powerpc/include/asm/mpic_msgr.h               |   35 +++
 arch/powerpc/platforms/Kconfig                     |    8 +
 arch/powerpc/sysdev/Makefile                       |    3 +-
 arch/powerpc/sysdev/mpic_msgr.c                    |  279 ++++++++++++++++++++
 5 files changed, 386 insertions(+), 1 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/powerpc/fsl/mpic-msgr.txt
 create mode 100644 arch/powerpc/include/asm/mpic_msgr.h
 create mode 100644 arch/powerpc/sysdev/mpic_msgr.c

[PATCH v3 0/2] powerpc: define and implement MPIC message register support

[Meador Inge]

This patch set defines a binding for FSL MPIC message registers and implements
an API for accessing those message registers.  Testing was done on a MPC8572DS
in an Linux-Linux AMP setup using OpenMCAPI (www.openmcapi.org) to communicate
between OS instances.  The message register API is used by the OpenMCAPI shared 
memory driver to send notifications between cores.

* v3 - Clarified a point in the binding concerning the length of the
       'interrupts' property.  Pointed out by Scott Wood.

* v2 - Incorporate feedback from Scott Wood
   * Make binding less implementation specific.
   * Add 'mpic-' prefix to message register node properties and aliases.
   * Remove 'interrupt-parent' from binding.
   * Fixed some example bugs with receive masks.

Signed-off-by: Meador Inge <meador_inge@mentor.com>
Cc: Hollis Blanchard <hollis_blanchard@mentor.com>

Meador Inge (2):
  powerpc: document the FSL MPIC message register binding
  powerpc: add support for MPIC message register API

 .../devicetree/bindings/powerpc/fsl/mpic-msgr.txt  |   62 +++++
 arch/powerpc/include/asm/mpic_msgr.h               |   35 +++
 arch/powerpc/platforms/Kconfig                     |    8 +
 arch/powerpc/sysdev/Makefile                       |    3 +-
 arch/powerpc/sysdev/mpic_msgr.c                    |  279 ++++++++++++++++++++
 5 files changed, 386 insertions(+), 1 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/powerpc/fsl/mpic-msgr.txt
 create mode 100644 arch/powerpc/include/asm/mpic_msgr.h
 create mode 100644 arch/powerpc/sysdev/mpic_msgr.c

[PATCH v3 1/2] powerpc: document the FSL MPIC message register binding

[Meador Inge]

This binding documents how the message register blocks found in some FSL
MPIC implementations shall be represented in a device tree.

Signed-off-by: Meador Inge <meador_inge@mentor.com>
Cc: Hollis Blanchard <hollis_blanchard@mentor.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Scott Wood <scottwood@freescale.com>
---
 .../devicetree/bindings/powerpc/fsl/mpic-msgr.txt  |   62 ++++++++++++++++++++
 1 files changed, 62 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/powerpc/fsl/mpic-msgr.txt

diff --git a/Documentation/devicetree/bindings/powerpc/fsl/mpic-msgr.txt b/Documentation/devicetree/bindings/powerpc/fsl/mpic-msgr.txt
new file mode 100644
index 0000000..e1c19b0
--- /dev/null
+++ b/Documentation/devicetree/bindings/powerpc/fsl/mpic-msgr.txt
@@ -0,0 +1,62 @@
+* FSL MPIC Message Registers
+
+This binding specifies what properties must be available in the device tree
+representation of the message register blocks found in some FSL MPIC
+implementations.
+
+Required properties:
+
+    - compatible: Specifies the compatibility list for the message register
+      block.  The type shall be <string> and the value shall be of the form
+      "fsl,mpic-v<version>-msgr", where <version> is the version number of
+      the MPIC containing the message registers.
+
+    - reg: Specifies the base physical address(s) and size(s) of the
+      message register block's addressable register space.  The type shall be
+      <prop-encoded-array>.
+
+    - interrupts: Specifies a list of interrupt source and level-sense pairs.
+      The type shall be <prop-encoded-array>.  The length shall be equal to
+      the number of registers that are available for receiving interrupts.
+
+Optional properties:
+
+    - mpic-msgr-receive-mask: Specifies what registers in the containing block
+      are allowed to receive interrupts.  The value is a bit mask where a set
+      bit at bit 'n' indicates that message register 'n' can receive interrupts.
+      The type shall be <prop-encoded-array>.  If not present, then all of
+      the message registers in the block are available.
+
+Aliases:
+
+    An alias should be created for every message register block.  They are not
+    required, though.  However, a particular implementation of this binding
+    may require aliases to be present.  Aliases are of the form
+    'mpic-msgr-block<n>', where <n> is an integer specifying the block's number.
+    Numbers shall start at 0.
+
+Example:
+
+	aliases {
+		mpic-msgr-block0 = &mpic_msgr_block0;
+		mpic-msgr-block1 = &mpic_msgr_block1;
+	};
+
+	mpic_msgr_block0: mpic-msgr-block@41400 {
+		compatible = "fsl,mpic-v3.1-msgr";
+		reg = <0x41400 0x200>;
+		// Message registers 0 and 2 in this block can receive interrupts on
+		// sources 0xb0 and 0xb2, respectively.
+		interrupts = <0xb0 2 0xb2 2>;
+		mpic-msgr-receive-mask = <0x5>;
+	};
+
+	mpic_msgr_block1: mpic-msgr-block@42400 {
+		compatible = "fsl,mpic-v3.1-msgr";
+		reg = <0x42400 0x200>;
+		// Message registers 0 and 2 in this block can receive interrupts on
+		// sources 0xb4 and 0xb6, respectively.
+		interrupts = <0xb4 2 0xb6 2>;
+		mpic-msgr-receive-mask = <0x5>;
+	};
+
-- 
1.7.0.4

Re: [PATCH v3 1/2] powerpc: document the FSL MPIC message register binding

[Scott Wood]

On Tue, 31 May 2011 14:19:02 -0500
Meador Inge <meador_inge@mentor.com> wrote:

> This binding documents how the message register blocks found in some FSL
> MPIC implementations shall be represented in a device tree.
> 
> Signed-off-by: Meador Inge <meador_inge@mentor.com>
> Cc: Hollis Blanchard <hollis_blanchard@mentor.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Scott Wood <scottwood@freescale.com>
> ---
>  .../devicetree/bindings/powerpc/fsl/mpic-msgr.txt  |   62 ++++++++++++++++++++
>  1 files changed, 62 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/powerpc/fsl/mpic-msgr.txt

ACK

-Scott

Re: [PATCH v3 1/2] powerpc: document the FSL MPIC message register binding

[Meador Inge]

On 05/31/2011 02:23 PM, Scott Wood wrote:

> On Tue, 31 May 2011 14:19:02 -0500
> Meador Inge <meador_inge@mentor.com> wrote:
> 
>> This binding documents how the message register blocks found in some FSL
>> MPIC implementations shall be represented in a device tree.
>>
>> Signed-off-by: Meador Inge <meador_inge@mentor.com>
>> Cc: Hollis Blanchard <hollis_blanchard@mentor.com>
>> Cc: Grant Likely <grant.likely@secretlab.ca>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Cc: Scott Wood <scottwood@freescale.com>
>> ---
>>  .../devicetree/bindings/powerpc/fsl/mpic-msgr.txt  |   62 ++++++++++++++++++++
>>  1 files changed, 62 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/powerpc/fsl/mpic-msgr.txt
> 
> ACK

Thanks.  Any comments on the implementation side:
http://patchwork.ozlabs.org/patch/98075/ ?

-- 
Meador Inge
CodeSourcery / Mentor Embedded
http://www.mentor.com/embedded-software

[PATCH v3 2/2] powerpc: add support for MPIC message register API

[Meador Inge]

Some MPIC implementations contain one or more blocks of message registers
that are used to send messages between cores via IPIs.  A simple API has
been added to access (get/put, read, write, etc ...) these message registers.
The available message registers are initially discovered via nodes in the
device tree.  A separate commit contains a binding for the message register
nodes.

Signed-off-by: Meador Inge <meador_inge-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
Cc: Hollis Blanchard <hollis_blanchard-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
---
 arch/powerpc/include/asm/mpic_msgr.h |   35 +++++
 arch/powerpc/platforms/Kconfig       |    8 +
 arch/powerpc/sysdev/Makefile         |    3 +-
 arch/powerpc/sysdev/mpic_msgr.c      |  279 ++++++++++++++++++++++++++++++++++
 4 files changed, 324 insertions(+), 1 deletions(-)
 create mode 100644 arch/powerpc/include/asm/mpic_msgr.h
 create mode 100644 arch/powerpc/sysdev/mpic_msgr.c

diff --git a/arch/powerpc/include/asm/mpic_msgr.h b/arch/powerpc/include/asm/mpic_msgr.h
new file mode 100644
index 0000000..370dcb4
--- /dev/null
+++ b/arch/powerpc/include/asm/mpic_msgr.h
@@ -0,0 +1,35 @@
+/*
+ * Copyright 2011-2012, Meador Inge, Mentor Graphics Corporation.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2 of the
+ * License.
+ *
+ */
+
+#ifndef _ASM_MPIC_MSGR_H
+#define _ASM_MPIC_MSGR_H
+
+#include <linux/types.h>
+
+struct mpic_msgr {
+	u32 __iomem *addr;
+	u32 __iomem *mer;
+	u32 __iomem	*msr;
+	int irq;
+	atomic_t in_use;
+	int num;
+};
+
+extern struct mpic_msgr* mpic_msgr_get(unsigned int reg_num);
+extern void mpic_msgr_put(struct mpic_msgr* msgr);
+extern void mpic_msgr_enable(struct mpic_msgr *msgr);
+extern void mpic_msgr_disable(struct mpic_msgr *msgr);
+extern void mpic_msgr_write(struct mpic_msgr *msgr, u32 message);
+extern u32 mpic_msgr_read(struct mpic_msgr *msgr);
+extern void mpic_msgr_clear(struct mpic_msgr *msgr);
+extern void mpic_msgr_set_destination(struct mpic_msgr *msgr, u32 cpu_num);
+extern int mpic_msgr_get_irq(struct mpic_msgr *msgr);
+
+#endif
diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
index f7b0772..4d65593 100644
--- a/arch/powerpc/platforms/Kconfig
+++ b/arch/powerpc/platforms/Kconfig
@@ -78,6 +78,14 @@ config MPIC_WEIRD
 	bool
 	default n
 
+config MPIC_MSGR
+	bool "MPIC message register support"
+	depends on MPIC
+	default n
+	help
+	  Enables support for the MPIC message registers.  These
+	  registers are used for inter-processor communication.
+
 config PPC_I8259
 	bool
 	default n
diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile
index 1e0c933..6d40185 100644
--- a/arch/powerpc/sysdev/Makefile
+++ b/arch/powerpc/sysdev/Makefile
@@ -3,7 +3,8 @@ subdir-ccflags-$(CONFIG_PPC_WERROR) := -Werror
 ccflags-$(CONFIG_PPC64)		:= -mno-minimal-toc
 
 mpic-msi-obj-$(CONFIG_PCI_MSI)	+= mpic_msi.o mpic_u3msi.o mpic_pasemi_msi.o
-obj-$(CONFIG_MPIC)		+= mpic.o $(mpic-msi-obj-y)
+mpic-msgr-obj-$(CONFIG_MPIC_MSGR)	+= mpic_msgr.o
+obj-$(CONFIG_MPIC)		+= mpic.o $(mpic-msi-obj-y) $(mpic-msgr-obj-y)
 fsl-msi-obj-$(CONFIG_PCI_MSI)	+= fsl_msi.o
 obj-$(CONFIG_PPC_MSI_BITMAP)	+= msi_bitmap.o
 
diff --git a/arch/powerpc/sysdev/mpic_msgr.c b/arch/powerpc/sysdev/mpic_msgr.c
new file mode 100644
index 0000000..bfa0612
--- /dev/null
+++ b/arch/powerpc/sysdev/mpic_msgr.c
@@ -0,0 +1,279 @@
+/*
+ * Copyright 2011-2012, Meador Inge, Mentor Graphics Corporation.
+ *
+ * Some ideas based on un-pushed work done by Vivek Mahajan, Jason Jin, and
+ * Mingkai Hu from Freescale Semiconductor, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2 of the
+ * License.
+ *
+ */
+
+#include <linux/list.h>
+#include <linux/of_platform.h>
+#include <linux/errno.h>
+#include <asm/prom.h>
+#include <asm/hw_irq.h>
+#include <asm/ppc-pci.h>
+#include <asm/mpic_msgr.h>
+
+#define MPIC_MSGR_REGISTERS_PER_BLOCK 4
+#define MSGR_INUSE 0
+#define MSGR_FREE 1
+
+/* Internal structure used *only* for IO mapping register blocks. */
+struct mpic_msgr_block {
+	struct msgr {
+		u32 msgr;
+		u8 res[12];
+	} msgrs[MPIC_MSGR_REGISTERS_PER_BLOCK];
+	u8 res0[192];
+	u32 mer;
+	u8 res1[12];
+	u32 msr;
+};
+
+static struct mpic_msgr **mpic_msgrs = 0;
+static unsigned int mpic_msgr_count = 0;
+
+struct mpic_msgr* mpic_msgr_get(unsigned int reg_num)
+{
+	struct mpic_msgr* msgr;
+
+	if (reg_num >= mpic_msgr_count)
+		return ERR_PTR(-ENODEV);
+
+	msgr = mpic_msgrs[reg_num];
+
+	if (atomic_cmpxchg(&msgr->in_use, MSGR_FREE, MSGR_INUSE) == MSGR_FREE)
+		return msgr;
+
+	return ERR_PTR(-EBUSY);
+}
+EXPORT_SYMBOL(mpic_msgr_get);
+
+void mpic_msgr_put(struct mpic_msgr* msgr)
+{
+	atomic_set(&msgr->in_use, MSGR_FREE);
+}
+EXPORT_SYMBOL(mpic_msgr_put);
+
+void mpic_msgr_enable(struct mpic_msgr *msgr)
+{
+	out_be32(msgr->mer, in_be32(msgr->mer) | (1 << msgr->num));
+}
+EXPORT_SYMBOL(mpic_msgr_enable);
+
+void mpic_msgr_disable(struct mpic_msgr *msgr)
+{
+	out_be32(msgr->mer, in_be32(msgr->mer) & ~(1 << msgr->num));
+}
+EXPORT_SYMBOL(mpic_msgr_disable);
+
+void mpic_msgr_write(struct mpic_msgr *msgr, u32 message)
+{
+	out_be32(msgr->addr, message);
+}
+EXPORT_SYMBOL(mpic_msgr_write);
+
+u32 mpic_msgr_read(struct mpic_msgr *msgr)
+{
+	return in_be32(msgr->addr);
+}
+EXPORT_SYMBOL(mpic_msgr_read);
+
+void mpic_msgr_clear(struct mpic_msgr *msgr)
+{
+	(void) mpic_msgr_read(msgr);
+}
+EXPORT_SYMBOL(mpic_msgr_clear);
+
+void mpic_msgr_set_destination(struct mpic_msgr *msgr, u32 cpu_num)
+{
+	out_be32(msgr->addr, 1 << cpu_num);
+}
+EXPORT_SYMBOL(mpic_msgr_set_destination);
+
+int mpic_msgr_get_irq(struct mpic_msgr *msgr)
+{
+	return msgr->irq;
+}
+EXPORT_SYMBOL(mpic_msgr_get_irq);
+
+/* The following three functions are used to compute the order and number of
+ * the message register blocks.  They are clearly very inefficent.  However,
+ * they are called *only* a few times during device initialization.
+ */
+static unsigned int mpic_msgr_number_of_blocks(void)
+{
+	unsigned int count;
+	struct device_node *aliases;
+
+	count = 0;
+	aliases = of_find_node_by_name(NULL, "aliases");
+
+	if (aliases) {
+		char buf[32];
+
+		for (;;) {
+			snprintf(buf, sizeof(buf), "mpic-msgr-block%d", count);
+			if (!of_find_property(aliases, buf, NULL))
+				break;
+
+			count += 1;
+		}
+	}
+
+	return count;
+}
+
+static unsigned int mpic_msgr_number_of_registers(void)
+{
+	return mpic_msgr_number_of_blocks() * MPIC_MSGR_REGISTERS_PER_BLOCK;
+}
+
+static int mpic_msgr_block_number(struct device_node *node)
+{
+	struct device_node *aliases;
+	unsigned int index, number_of_blocks;
+	char buf[64];
+
+	number_of_blocks = mpic_msgr_number_of_blocks();
+	aliases = of_find_node_by_name(NULL, "aliases");
+	if (!aliases)
+		return -1;
+
+	for (index = 0; index < number_of_blocks; ++index) {
+		struct property *prop;
+
+		snprintf(buf, sizeof(buf), "mpic-msgr-block%d", index);
+		prop = of_find_property(aliases, buf, NULL);
+		if (node == of_find_node_by_path(prop->value))
+			break;
+	}
+
+	return index == number_of_blocks ? -1 : index;
+}
+
+/* The probe function for a single message register block.
+ */
+static __devinit int mpic_msgr_probe(struct platform_device *dev)
+{
+	struct mpic_msgr_block __iomem *msgr_block;
+	int block_number;
+	struct resource rsrc;
+	unsigned int i;
+	unsigned int irq_index;
+	struct device_node *np = dev->dev.of_node;
+	unsigned int receive_mask;
+	const unsigned int *prop;
+
+	if (!np) {
+		dev_err(&dev->dev, "Device OF-Node is NULL");
+		return -EFAULT;
+	}
+
+	/* Allocate the message register array upon the first device
+	 * registered.
+	 */
+	if (!mpic_msgrs) {
+		mpic_msgr_count = mpic_msgr_number_of_registers();
+		dev_info(&dev->dev, "Found %d message registers\n", mpic_msgr_count);
+
+		mpic_msgrs = kzalloc(sizeof(struct mpic_msgr) * mpic_msgr_count,
+							 GFP_KERNEL);
+		if (!mpic_msgrs) {
+			dev_err(&dev->dev, "No memory for message register blocks\n");
+			return -ENOMEM;
+		}
+	}
+	dev_info(&dev->dev, "Of-device full name %s\n", np->full_name);
+
+	/* IO map the message register block. */
+	of_address_to_resource(np, 0, &rsrc);
+	msgr_block = ioremap(rsrc.start, rsrc.end - rsrc.start);
+	if (!msgr_block) {
+		dev_err(&dev->dev, "Failed to iomap MPIC message registers");
+		return -EFAULT;
+	}
+
+	/* Ensure the block has a defined order. */
+	block_number = mpic_msgr_block_number(np);
+	if (block_number < 0) {
+		dev_err(&dev->dev, "Failed to find message register block alias\n");
+		return -ENODEV;
+	}
+	dev_info(&dev->dev, "Setting up message register block %d\n", block_number);
+
+	/* Grab the receive mask which specifies what registers can receive
+	 * interrupts.
+	 */
+	prop = of_get_property(np, "mpic-msgr-receive-mask", NULL);
+	receive_mask = (prop) ? *prop : 0xF;
+
+	/* Build up the appropriate message register data structures. */
+	for (i = 0, irq_index = 0; i < MPIC_MSGR_REGISTERS_PER_BLOCK; ++i) {
+		struct mpic_msgr *msgr;
+		unsigned int reg_number;
+
+		msgr = kzalloc(sizeof(struct mpic_msgr), GFP_KERNEL);
+		if (!msgr) {
+			dev_err(&dev->dev, "No memory for message register\n");
+			return -ENOMEM;
+		}
+
+		reg_number = block_number * MPIC_MSGR_REGISTERS_PER_BLOCK + i;
+		msgr->addr = &msgr_block->msgrs[i].msgr;
+		msgr->mer = &msgr_block->mer;
+		msgr->msr = &msgr_block->msr;
+		atomic_set(&msgr->in_use, MSGR_FREE);
+		msgr->num = reg_number;
+
+		if (receive_mask & (1 << i)) {
+			struct resource irq;
+
+			if (of_irq_to_resource(np, irq_index, &irq) == NO_IRQ) {
+				dev_err(&dev->dev, "Missing interrupt specifier");
+				kfree(msgr);
+				return -EFAULT;
+			}
+			msgr->irq = irq.start;
+			irq_index += 1;
+		} else {
+			msgr->irq = NO_IRQ;
+		}
+
+		mpic_msgrs[reg_number] = msgr;
+		mpic_msgr_disable(msgr);
+		dev_info(&dev->dev, "Register %d initialized: irq %d\n",
+				 msgr->num, msgr->irq);
+	
+	}
+
+	return 0;
+}
+
+static const struct of_device_id mpic_msgr_ids[] = {
+	{
+		.compatible = "fsl,mpic-v3.1-msgr",
+		.data = NULL,
+	},
+	{}
+};
+
+static struct platform_driver mpic_msgr_driver = {
+	.driver = {
+		.name = "mpic-msgr",
+		.owner = THIS_MODULE,
+		.of_match_table = mpic_msgr_ids,
+	},
+	.probe = mpic_msgr_probe,
+};
+
+static __init int mpic_msgr_init(void)
+{
+	return platform_driver_register(&mpic_msgr_driver);
+}
+subsys_initcall(mpic_msgr_init);
-- 
1.7.0.4

[PATCH v3 2/2] powerpc: add support for MPIC message register API

[Meador Inge]

Some MPIC implementations contain one or more blocks of message registers
that are used to send messages between cores via IPIs.  A simple API has
been added to access (get/put, read, write, etc ...) these message registers.
The available message registers are initially discovered via nodes in the
device tree.  A separate commit contains a binding for the message register
nodes.

Signed-off-by: Meador Inge <meador_inge@mentor.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Hollis Blanchard <hollis_blanchard@mentor.com>
---
 arch/powerpc/include/asm/mpic_msgr.h |   35 +++++
 arch/powerpc/platforms/Kconfig       |    8 +
 arch/powerpc/sysdev/Makefile         |    3 +-
 arch/powerpc/sysdev/mpic_msgr.c      |  279 ++++++++++++++++++++++++++++++++++
 4 files changed, 324 insertions(+), 1 deletions(-)
 create mode 100644 arch/powerpc/include/asm/mpic_msgr.h
 create mode 100644 arch/powerpc/sysdev/mpic_msgr.c

diff --git a/arch/powerpc/include/asm/mpic_msgr.h b/arch/powerpc/include/asm/mpic_msgr.h
new file mode 100644
index 0000000..370dcb4
--- /dev/null
+++ b/arch/powerpc/include/asm/mpic_msgr.h
@@ -0,0 +1,35 @@
+/*
+ * Copyright 2011-2012, Meador Inge, Mentor Graphics Corporation.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2 of the
+ * License.
+ *
+ */
+
+#ifndef _ASM_MPIC_MSGR_H
+#define _ASM_MPIC_MSGR_H
+
+#include <linux/types.h>
+
+struct mpic_msgr {
+	u32 __iomem *addr;
+	u32 __iomem *mer;
+	u32 __iomem	*msr;
+	int irq;
+	atomic_t in_use;
+	int num;
+};
+
+extern struct mpic_msgr* mpic_msgr_get(unsigned int reg_num);
+extern void mpic_msgr_put(struct mpic_msgr* msgr);
+extern void mpic_msgr_enable(struct mpic_msgr *msgr);
+extern void mpic_msgr_disable(struct mpic_msgr *msgr);
+extern void mpic_msgr_write(struct mpic_msgr *msgr, u32 message);
+extern u32 mpic_msgr_read(struct mpic_msgr *msgr);
+extern void mpic_msgr_clear(struct mpic_msgr *msgr);
+extern void mpic_msgr_set_destination(struct mpic_msgr *msgr, u32 cpu_num);
+extern int mpic_msgr_get_irq(struct mpic_msgr *msgr);
+
+#endif
diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
index f7b0772..4d65593 100644
--- a/arch/powerpc/platforms/Kconfig
+++ b/arch/powerpc/platforms/Kconfig
@@ -78,6 +78,14 @@ config MPIC_WEIRD
 	bool
 	default n
 
+config MPIC_MSGR
+	bool "MPIC message register support"
+	depends on MPIC
+	default n
+	help
+	  Enables support for the MPIC message registers.  These
+	  registers are used for inter-processor communication.
+
 config PPC_I8259
 	bool
 	default n
diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile
index 1e0c933..6d40185 100644
--- a/arch/powerpc/sysdev/Makefile
+++ b/arch/powerpc/sysdev/Makefile
@@ -3,7 +3,8 @@ subdir-ccflags-$(CONFIG_PPC_WERROR) := -Werror
 ccflags-$(CONFIG_PPC64)		:= -mno-minimal-toc
 
 mpic-msi-obj-$(CONFIG_PCI_MSI)	+= mpic_msi.o mpic_u3msi.o mpic_pasemi_msi.o
-obj-$(CONFIG_MPIC)		+= mpic.o $(mpic-msi-obj-y)
+mpic-msgr-obj-$(CONFIG_MPIC_MSGR)	+= mpic_msgr.o
+obj-$(CONFIG_MPIC)		+= mpic.o $(mpic-msi-obj-y) $(mpic-msgr-obj-y)
 fsl-msi-obj-$(CONFIG_PCI_MSI)	+= fsl_msi.o
 obj-$(CONFIG_PPC_MSI_BITMAP)	+= msi_bitmap.o
 
diff --git a/arch/powerpc/sysdev/mpic_msgr.c b/arch/powerpc/sysdev/mpic_msgr.c
new file mode 100644
index 0000000..bfa0612
--- /dev/null
+++ b/arch/powerpc/sysdev/mpic_msgr.c
@@ -0,0 +1,279 @@
+/*
+ * Copyright 2011-2012, Meador Inge, Mentor Graphics Corporation.
+ *
+ * Some ideas based on un-pushed work done by Vivek Mahajan, Jason Jin, and
+ * Mingkai Hu from Freescale Semiconductor, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2 of the
+ * License.
+ *
+ */
+
+#include <linux/list.h>
+#include <linux/of_platform.h>
+#include <linux/errno.h>
+#include <asm/prom.h>
+#include <asm/hw_irq.h>
+#include <asm/ppc-pci.h>
+#include <asm/mpic_msgr.h>
+
+#define MPIC_MSGR_REGISTERS_PER_BLOCK 4
+#define MSGR_INUSE 0
+#define MSGR_FREE 1
+
+/* Internal structure used *only* for IO mapping register blocks. */
+struct mpic_msgr_block {
+	struct msgr {
+		u32 msgr;
+		u8 res[12];
+	} msgrs[MPIC_MSGR_REGISTERS_PER_BLOCK];
+	u8 res0[192];
+	u32 mer;
+	u8 res1[12];
+	u32 msr;
+};
+
+static struct mpic_msgr **mpic_msgrs = 0;
+static unsigned int mpic_msgr_count = 0;
+
+struct mpic_msgr* mpic_msgr_get(unsigned int reg_num)
+{
+	struct mpic_msgr* msgr;
+
+	if (reg_num >= mpic_msgr_count)
+		return ERR_PTR(-ENODEV);
+
+	msgr = mpic_msgrs[reg_num];
+
+	if (atomic_cmpxchg(&msgr->in_use, MSGR_FREE, MSGR_INUSE) == MSGR_FREE)
+		return msgr;
+
+	return ERR_PTR(-EBUSY);
+}
+EXPORT_SYMBOL(mpic_msgr_get);
+
+void mpic_msgr_put(struct mpic_msgr* msgr)
+{
+	atomic_set(&msgr->in_use, MSGR_FREE);
+}
+EXPORT_SYMBOL(mpic_msgr_put);
+
+void mpic_msgr_enable(struct mpic_msgr *msgr)
+{
+	out_be32(msgr->mer, in_be32(msgr->mer) | (1 << msgr->num));
+}
+EXPORT_SYMBOL(mpic_msgr_enable);
+
+void mpic_msgr_disable(struct mpic_msgr *msgr)
+{
+	out_be32(msgr->mer, in_be32(msgr->mer) & ~(1 << msgr->num));
+}
+EXPORT_SYMBOL(mpic_msgr_disable);
+
+void mpic_msgr_write(struct mpic_msgr *msgr, u32 message)
+{
+	out_be32(msgr->addr, message);
+}
+EXPORT_SYMBOL(mpic_msgr_write);
+
+u32 mpic_msgr_read(struct mpic_msgr *msgr)
+{
+	return in_be32(msgr->addr);
+}
+EXPORT_SYMBOL(mpic_msgr_read);
+
+void mpic_msgr_clear(struct mpic_msgr *msgr)
+{
+	(void) mpic_msgr_read(msgr);
+}
+EXPORT_SYMBOL(mpic_msgr_clear);
+
+void mpic_msgr_set_destination(struct mpic_msgr *msgr, u32 cpu_num)
+{
+	out_be32(msgr->addr, 1 << cpu_num);
+}
+EXPORT_SYMBOL(mpic_msgr_set_destination);
+
+int mpic_msgr_get_irq(struct mpic_msgr *msgr)
+{
+	return msgr->irq;
+}
+EXPORT_SYMBOL(mpic_msgr_get_irq);
+
+/* The following three functions are used to compute the order and number of
+ * the message register blocks.  They are clearly very inefficent.  However,
+ * they are called *only* a few times during device initialization.
+ */
+static unsigned int mpic_msgr_number_of_blocks(void)
+{
+	unsigned int count;
+	struct device_node *aliases;
+
+	count = 0;
+	aliases = of_find_node_by_name(NULL, "aliases");
+
+	if (aliases) {
+		char buf[32];
+
+		for (;;) {
+			snprintf(buf, sizeof(buf), "mpic-msgr-block%d", count);
+			if (!of_find_property(aliases, buf, NULL))
+				break;
+
+			count += 1;
+		}
+	}
+
+	return count;
+}
+
+static unsigned int mpic_msgr_number_of_registers(void)
+{
+	return mpic_msgr_number_of_blocks() * MPIC_MSGR_REGISTERS_PER_BLOCK;
+}
+
+static int mpic_msgr_block_number(struct device_node *node)
+{
+	struct device_node *aliases;
+	unsigned int index, number_of_blocks;
+	char buf[64];
+
+	number_of_blocks = mpic_msgr_number_of_blocks();
+	aliases = of_find_node_by_name(NULL, "aliases");
+	if (!aliases)
+		return -1;
+
+	for (index = 0; index < number_of_blocks; ++index) {
+		struct property *prop;
+
+		snprintf(buf, sizeof(buf), "mpic-msgr-block%d", index);
+		prop = of_find_property(aliases, buf, NULL);
+		if (node == of_find_node_by_path(prop->value))
+			break;
+	}
+
+	return index == number_of_blocks ? -1 : index;
+}
+
+/* The probe function for a single message register block.
+ */
+static __devinit int mpic_msgr_probe(struct platform_device *dev)
+{
+	struct mpic_msgr_block __iomem *msgr_block;
+	int block_number;
+	struct resource rsrc;
+	unsigned int i;
+	unsigned int irq_index;
+	struct device_node *np = dev->dev.of_node;
+	unsigned int receive_mask;
+	const unsigned int *prop;
+
+	if (!np) {
+		dev_err(&dev->dev, "Device OF-Node is NULL");
+		return -EFAULT;
+	}
+
+	/* Allocate the message register array upon the first device
+	 * registered.
+	 */
+	if (!mpic_msgrs) {
+		mpic_msgr_count = mpic_msgr_number_of_registers();
+		dev_info(&dev->dev, "Found %d message registers\n", mpic_msgr_count);
+
+		mpic_msgrs = kzalloc(sizeof(struct mpic_msgr) * mpic_msgr_count,
+							 GFP_KERNEL);
+		if (!mpic_msgrs) {
+			dev_err(&dev->dev, "No memory for message register blocks\n");
+			return -ENOMEM;
+		}
+	}
+	dev_info(&dev->dev, "Of-device full name %s\n", np->full_name);
+
+	/* IO map the message register block. */
+	of_address_to_resource(np, 0, &rsrc);
+	msgr_block = ioremap(rsrc.start, rsrc.end - rsrc.start);
+	if (!msgr_block) {
+		dev_err(&dev->dev, "Failed to iomap MPIC message registers");
+		return -EFAULT;
+	}
+
+	/* Ensure the block has a defined order. */
+	block_number = mpic_msgr_block_number(np);
+	if (block_number < 0) {
+		dev_err(&dev->dev, "Failed to find message register block alias\n");
+		return -ENODEV;
+	}
+	dev_info(&dev->dev, "Setting up message register block %d\n", block_number);
+
+	/* Grab the receive mask which specifies what registers can receive
+	 * interrupts.
+	 */
+	prop = of_get_property(np, "mpic-msgr-receive-mask", NULL);
+	receive_mask = (prop) ? *prop : 0xF;
+
+	/* Build up the appropriate message register data structures. */
+	for (i = 0, irq_index = 0; i < MPIC_MSGR_REGISTERS_PER_BLOCK; ++i) {
+		struct mpic_msgr *msgr;
+		unsigned int reg_number;
+
+		msgr = kzalloc(sizeof(struct mpic_msgr), GFP_KERNEL);
+		if (!msgr) {
+			dev_err(&dev->dev, "No memory for message register\n");
+			return -ENOMEM;
+		}
+
+		reg_number = block_number * MPIC_MSGR_REGISTERS_PER_BLOCK + i;
+		msgr->addr = &msgr_block->msgrs[i].msgr;
+		msgr->mer = &msgr_block->mer;
+		msgr->msr = &msgr_block->msr;
+		atomic_set(&msgr->in_use, MSGR_FREE);
+		msgr->num = reg_number;
+
+		if (receive_mask & (1 << i)) {
+			struct resource irq;
+
+			if (of_irq_to_resource(np, irq_index, &irq) == NO_IRQ) {
+				dev_err(&dev->dev, "Missing interrupt specifier");
+				kfree(msgr);
+				return -EFAULT;
+			}
+			msgr->irq = irq.start;
+			irq_index += 1;
+		} else {
+			msgr->irq = NO_IRQ;
+		}
+
+		mpic_msgrs[reg_number] = msgr;
+		mpic_msgr_disable(msgr);
+		dev_info(&dev->dev, "Register %d initialized: irq %d\n",
+				 msgr->num, msgr->irq);
+	
+	}
+
+	return 0;
+}
+
+static const struct of_device_id mpic_msgr_ids[] = {
+	{
+		.compatible = "fsl,mpic-v3.1-msgr",
+		.data = NULL,
+	},
+	{}
+};
+
+static struct platform_driver mpic_msgr_driver = {
+	.driver = {
+		.name = "mpic-msgr",
+		.owner = THIS_MODULE,
+		.of_match_table = mpic_msgr_ids,
+	},
+	.probe = mpic_msgr_probe,
+};
+
+static __init int mpic_msgr_init(void)
+{
+	return platform_driver_register(&mpic_msgr_driver);
+}
+subsys_initcall(mpic_msgr_init);
-- 
1.7.0.4

Re: [PATCH v3 2/2] powerpc: add support for MPIC message register API

[Benjamin Herrenschmidt]

On Tue, 2011-05-31 at 14:19 -0500, Meador Inge wrote:
> Some MPIC implementations contain one or more blocks of message registers
> that are used to send messages between cores via IPIs.  A simple API has
> been added to access (get/put, read, write, etc ...) these message registers.
> The available message registers are initially discovered via nodes in the
> device tree.  A separate commit contains a binding for the message register
> nodes.

Ok, so I finally got to look at that in a bit more details...
> +#ifndef _ASM_MPIC_MSGR_H
> +#define _ASM_MPIC_MSGR_H
> +
> +#include <linux/types.h>
> +
> +struct mpic_msgr {
> +	u32 __iomem *addr;
> +	u32 __iomem *mer;
> +	u32 __iomem	*msr;
> +	int irq;
> +	atomic_t in_use;
> +	int num;
> +};

General comment... I'm really not fan of "msgr", I'd rather see
"mpic_message_*", it's a tad more verbose but looks a lot better, no ?

Also do you need those 3 iomem pointers ? Not just one with fixed
offsets ? Or do they come from vastly different sources ?

atomic_t in_use looks fishy, but let's see how you use it...

> +extern struct mpic_msgr* mpic_msgr_get(unsigned int reg_num);
> +extern void mpic_msgr_put(struct mpic_msgr* msgr);
> +extern void mpic_msgr_enable(struct mpic_msgr *msgr);
> +extern void mpic_msgr_disable(struct mpic_msgr *msgr);
> +extern void mpic_msgr_write(struct mpic_msgr *msgr, u32 message);
> +extern u32 mpic_msgr_read(struct mpic_msgr *msgr);
> +extern void mpic_msgr_clear(struct mpic_msgr *msgr);
> +extern void mpic_msgr_set_destination(struct mpic_msgr *msgr, u32 cpu_num);
> +extern int mpic_msgr_get_irq(struct mpic_msgr *msgr);

Documentation of the API please.

> +#endif
> diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
> index f7b0772..4d65593 100644
> --- a/arch/powerpc/platforms/Kconfig
> +++ b/arch/powerpc/platforms/Kconfig
> @@ -78,6 +78,14 @@ config MPIC_WEIRD
>  	bool
>  	default n
>  
> +config MPIC_MSGR
> +	bool "MPIC message register support"
> +	depends on MPIC
> +	default n
> +	help
> +	  Enables support for the MPIC message registers.  These
> +	  registers are used for inter-processor communication.
> +
>  config PPC_I8259
>  	bool
>  	default n
> diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile
> index 1e0c933..6d40185 100644
> --- a/arch/powerpc/sysdev/Makefile
> +++ b/arch/powerpc/sysdev/Makefile
> @@ -3,7 +3,8 @@ subdir-ccflags-$(CONFIG_PPC_WERROR) := -Werror
>  ccflags-$(CONFIG_PPC64)		:= -mno-minimal-toc
>  
>  mpic-msi-obj-$(CONFIG_PCI_MSI)	+= mpic_msi.o mpic_u3msi.o mpic_pasemi_msi.o
> -obj-$(CONFIG_MPIC)		+= mpic.o $(mpic-msi-obj-y)
> +mpic-msgr-obj-$(CONFIG_MPIC_MSGR)	+= mpic_msgr.o
> +obj-$(CONFIG_MPIC)		+= mpic.o $(mpic-msi-obj-y) $(mpic-msgr-obj-y)
>  fsl-msi-obj-$(CONFIG_PCI_MSI)	+= fsl_msi.o
>  obj-$(CONFIG_PPC_MSI_BITMAP)	+= msi_bitmap.o
>  
> diff --git a/arch/powerpc/sysdev/mpic_msgr.c b/arch/powerpc/sysdev/mpic_msgr.c
> new file mode 100644
> index 0000000..bfa0612
> --- /dev/null
> +++ b/arch/powerpc/sysdev/mpic_msgr.c
> @@ -0,0 +1,279 @@
> +/*
> + * Copyright 2011-2012, Meador Inge, Mentor Graphics Corporation.
> + *
> + * Some ideas based on un-pushed work done by Vivek Mahajan, Jason Jin, and
> + * Mingkai Hu from Freescale Semiconductor, Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2 of the
> + * License.
> + *
> + */
> +
> +#include <linux/list.h>
> +#include <linux/of_platform.h>
> +#include <linux/errno.h>
> +#include <asm/prom.h>
> +#include <asm/hw_irq.h>
> +#include <asm/ppc-pci.h>
> +#include <asm/mpic_msgr.h>
> +
> +#define MPIC_MSGR_REGISTERS_PER_BLOCK 4
> +#define MSGR_INUSE 0
> +#define MSGR_FREE 1
> +
> +/* Internal structure used *only* for IO mapping register blocks. */
> +struct mpic_msgr_block {
> +	struct msgr {
> +		u32 msgr;
> +		u8 res[12];
> +	} msgrs[MPIC_MSGR_REGISTERS_PER_BLOCK];
> +	u8 res0[192];
> +	u32 mer;
> +	u8 res1[12];
> +	u32 msr;
> +};

So this represent HW registers ? Please make it clear in the comment.
I'm not a terrible fan of using structures to map HW especially with so
few registers.

> +static struct mpic_msgr **mpic_msgrs = 0;
> +static unsigned int mpic_msgr_count = 0;
> +
> +struct mpic_msgr* mpic_msgr_get(unsigned int reg_num)
> +{
> +	struct mpic_msgr* msgr;
> +
> +	if (reg_num >= mpic_msgr_count)
> +		return ERR_PTR(-ENODEV);
> +
> +	msgr = mpic_msgrs[reg_num];

No locking on the array access, might be ok if those things are never
plugged in/out I suppose...

> +	if (atomic_cmpxchg(&msgr->in_use, MSGR_FREE, MSGR_INUSE) == MSGR_FREE)
> +		return msgr;
> +
> +	return ERR_PTR(-EBUSY);
> +}
> +EXPORT_SYMBOL(mpic_msgr_get);

So how are those things intended to be used ? Clients get a fixed
"register" number to use ? It looks like this stuff would have been
better off using some kind of allocator no ? A bitmap would have done
the job... or do you really need to have some kind of fixed numbering
associated with clients ?

> +void mpic_msgr_put(struct mpic_msgr* msgr)
> +{
> +	atomic_set(&msgr->in_use, MSGR_FREE);
> +}
> +EXPORT_SYMBOL(mpic_msgr_put);

Shouldn't it be disabled too while at it ?

> +void mpic_msgr_enable(struct mpic_msgr *msgr)
> +{
> +	out_be32(msgr->mer, in_be32(msgr->mer) | (1 << msgr->num));
> +}
> +EXPORT_SYMBOL(mpic_msgr_enable);

Why are all those exported non-GPL ? We have a policy of making new
in-kernel APIs generally GPL only.

> +void mpic_msgr_disable(struct mpic_msgr *msgr)
> +{
> +	out_be32(msgr->mer, in_be32(msgr->mer) & ~(1 << msgr->num));
> +}
> +EXPORT_SYMBOL(mpic_msgr_disable);

I see read-modify-write cycles here with no synchronization/locking
whatsoever... Might be ok as long as you document it in the doc of the
API, ie, the caller is required to synchronize.

> +void mpic_msgr_write(struct mpic_msgr *msgr, u32 message)
> +{
> +	out_be32(msgr->addr, message);
> +}
> +EXPORT_SYMBOL(mpic_msgr_write);
> +
> +u32 mpic_msgr_read(struct mpic_msgr *msgr)
> +{
> +	return in_be32(msgr->addr);
> +}
> +EXPORT_SYMBOL(mpic_msgr_read);

Those look like significant bloat/overhead for what is just an MMIO
wrapper on a register access, why not make them static inline's
instead ?

> +void mpic_msgr_clear(struct mpic_msgr *msgr)
> +{
> +	(void) mpic_msgr_read(msgr);
> +}
> +EXPORT_SYMBOL(mpic_msgr_clear);
> +
> +void mpic_msgr_set_destination(struct mpic_msgr *msgr, u32 cpu_num)
> +{
> +	out_be32(msgr->addr, 1 << cpu_num);
> +}
> +EXPORT_SYMBOL(mpic_msgr_set_destination);

Please make it clear that this takes a HW CPU number. How is that going
to be used ? Wouldn't you be better off having a __ variant taking the
HW CPU number an the "public" variant doing the conversion from a Linux
number ?

> +int mpic_msgr_get_irq(struct mpic_msgr *msgr)
> +{
> +	return msgr->irq;
> +}
> +EXPORT_SYMBOL(mpic_msgr_get_irq);
> +
> +/* The following three functions are used to compute the order and number of
> + * the message register blocks.  They are clearly very inefficent.  However,
> + * they are called *only* a few times during device initialization.
> + */
> +static unsigned int mpic_msgr_number_of_blocks(void)
> +{
> +	unsigned int count;
> +	struct device_node *aliases;
> +
> +	count = 0;
> +	aliases = of_find_node_by_name(NULL, "aliases");
> +
> +	if (aliases) {
> +		char buf[32];
> +
> +		for (;;) {
> +			snprintf(buf, sizeof(buf), "mpic-msgr-block%d", count);
> +			if (!of_find_property(aliases, buf, NULL))
> +				break;
> +
> +			count += 1;
> +		}
> +	}
> +
> +	return count;
> +}

I don't like relying on aliases as a way to figure out what HW is
present, that's not right. You should find the actual devices themselves
based on their compatible properties.

All of that stuff also assume these things are never going to be
hot-plugged. IE. will they never go in/out of partition in hypervisors
(or even be virtualized) ?

> +static unsigned int mpic_msgr_number_of_registers(void)
> +{
> +	return mpic_msgr_number_of_blocks() * MPIC_MSGR_REGISTERS_PER_BLOCK;
> +}
> +
> +static int mpic_msgr_block_number(struct device_node *node)
> +{
> +	struct device_node *aliases;
> +	unsigned int index, number_of_blocks;
> +	char buf[64];
> +
> +	number_of_blocks = mpic_msgr_number_of_blocks();
> +	aliases = of_find_node_by_name(NULL, "aliases");
> +	if (!aliases)
> +		return -1;
> +
> +	for (index = 0; index < number_of_blocks; ++index) {
> +		struct property *prop;
> +
> +		snprintf(buf, sizeof(buf), "mpic-msgr-block%d", index);
> +		prop = of_find_property(aliases, buf, NULL);
> +		if (node == of_find_node_by_path(prop->value))
> +			break;
> +	}
> +
> +	return index == number_of_blocks ? -1 : index;
> +}
> +
> +/* The probe function for a single message register block.
> + */
> +static __devinit int mpic_msgr_probe(struct platform_device *dev)
> +{
> +	struct mpic_msgr_block __iomem *msgr_block;
> +	int block_number;
> +	struct resource rsrc;
> +	unsigned int i;
> +	unsigned int irq_index;
> +	struct device_node *np = dev->dev.of_node;
> +	unsigned int receive_mask;
> +	const unsigned int *prop;
> +
> +	if (!np) {
> +		dev_err(&dev->dev, "Device OF-Node is NULL");
> +		return -EFAULT;
> +	}
> +
> +	/* Allocate the message register array upon the first device
> +	 * registered.
> +	 */
> +	if (!mpic_msgrs) {
> +		mpic_msgr_count = mpic_msgr_number_of_registers();
> +		dev_info(&dev->dev, "Found %d message registers\n", mpic_msgr_count);
> +
> +		mpic_msgrs = kzalloc(sizeof(struct mpic_msgr) * mpic_msgr_count,
> +							 GFP_KERNEL);
> +		if (!mpic_msgrs) {
> +			dev_err(&dev->dev, "No memory for message register blocks\n");
> +			return -ENOMEM;
> +		}
> +	}
> +	dev_info(&dev->dev, "Of-device full name %s\n", np->full_name);

Ok, so we have another scheme of:

 - Count all devices in the system of a given type
 - Assign them numbers
 - API uses number

That sucks... unless you have an allocator. And even then.

I'd rather clients use something like struct mpic_msgr (or msg_reg or
message_reg) as the "handle" to one of these things.

It can be obtained via an allocator or a device tree parsing routine if
there's a fixed relationship between clients and registers, I don't
really know how that stuff is to be used, but in any case, the whole
thing stinks as it is.

> +	/* IO map the message register block. */
> +	of_address_to_resource(np, 0, &rsrc);
> +	msgr_block = ioremap(rsrc.start, rsrc.end - rsrc.start);
> +	if (!msgr_block) {
> +		dev_err(&dev->dev, "Failed to iomap MPIC message registers");
> +		return -EFAULT;
> +	}
> +
> +	/* Ensure the block has a defined order. */
> +	block_number = mpic_msgr_block_number(np);
> +	if (block_number < 0) {
> +		dev_err(&dev->dev, "Failed to find message register block alias\n");
> +		return -ENODEV;
> +	}
> +	dev_info(&dev->dev, "Setting up message register block %d\n", block_number);
> +
> +	/* Grab the receive mask which specifies what registers can receive
> +	 * interrupts.
> +	 */
> +	prop = of_get_property(np, "mpic-msgr-receive-mask", NULL);
> +	receive_mask = (prop) ? *prop : 0xF;
> +
> +	/* Build up the appropriate message register data structures. */
> +	for (i = 0, irq_index = 0; i < MPIC_MSGR_REGISTERS_PER_BLOCK; ++i) {
> +		struct mpic_msgr *msgr;
> +		unsigned int reg_number;
> +
> +		msgr = kzalloc(sizeof(struct mpic_msgr), GFP_KERNEL);
> +		if (!msgr) {
> +			dev_err(&dev->dev, "No memory for message register\n");
> +			return -ENOMEM;
> +		}
> +
> +		reg_number = block_number * MPIC_MSGR_REGISTERS_PER_BLOCK + i;
> +		msgr->addr = &msgr_block->msgrs[i].msgr;
> +		msgr->mer = &msgr_block->mer;
> +		msgr->msr = &msgr_block->msr;
> +		atomic_set(&msgr->in_use, MSGR_FREE);
> +		msgr->num = reg_number;
> +
> +		if (receive_mask & (1 << i)) {
> +			struct resource irq;
> +
> +			if (of_irq_to_resource(np, irq_index, &irq) == NO_IRQ) {
> +				dev_err(&dev->dev, "Missing interrupt specifier");
> +				kfree(msgr);
> +				return -EFAULT;
> +			}
> +			msgr->irq = irq.start;
> +			irq_index += 1;
> +		} else {
> +			msgr->irq = NO_IRQ;
> +		}
> +
> +		mpic_msgrs[reg_number] = msgr;
> +		mpic_msgr_disable(msgr);
> +		dev_info(&dev->dev, "Register %d initialized: irq %d\n",
> +				 msgr->num, msgr->irq);
> +	
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mpic_msgr_ids[] = {
> +	{
> +		.compatible = "fsl,mpic-v3.1-msgr",
> +		.data = NULL,
> +	},
> +	{}
> +};
> +
> +static struct platform_driver mpic_msgr_driver = {
> +	.driver = {
> +		.name = "mpic-msgr",
> +		.owner = THIS_MODULE,
> +		.of_match_table = mpic_msgr_ids,
> +	},
> +	.probe = mpic_msgr_probe,
> +};
> +
> +static __init int mpic_msgr_init(void)
> +{
> +	return platform_driver_register(&mpic_msgr_driver);
> +}
> +subsys_initcall(mpic_msgr_init);

Ben.

Re: [PATCH v3 2/2] powerpc: add support for MPIC message register API

[Benjamin Herrenschmidt]

On Tue, 2011-05-31 at 14:19 -0500, Meador Inge wrote:
> Some MPIC implementations contain one or more blocks of message registers
> that are used to send messages between cores via IPIs.  A simple API has
> been added to access (get/put, read, write, etc ...) these message registers.
> The available message registers are initially discovered via nodes in the
> device tree.  A separate commit contains a binding for the message register
> nodes.

Ok, so I finally got to look at that in a bit more details...
> +#ifndef _ASM_MPIC_MSGR_H
> +#define _ASM_MPIC_MSGR_H
> +
> +#include <linux/types.h>
> +
> +struct mpic_msgr {
> +	u32 __iomem *addr;
> +	u32 __iomem *mer;
> +	u32 __iomem	*msr;
> +	int irq;
> +	atomic_t in_use;
> +	int num;
> +};

General comment... I'm really not fan of "msgr", I'd rather see
"mpic_message_*", it's a tad more verbose but looks a lot better, no ?

Also do you need those 3 iomem pointers ? Not just one with fixed
offsets ? Or do they come from vastly different sources ?

atomic_t in_use looks fishy, but let's see how you use it...

> +extern struct mpic_msgr* mpic_msgr_get(unsigned int reg_num);
> +extern void mpic_msgr_put(struct mpic_msgr* msgr);
> +extern void mpic_msgr_enable(struct mpic_msgr *msgr);
> +extern void mpic_msgr_disable(struct mpic_msgr *msgr);
> +extern void mpic_msgr_write(struct mpic_msgr *msgr, u32 message);
> +extern u32 mpic_msgr_read(struct mpic_msgr *msgr);
> +extern void mpic_msgr_clear(struct mpic_msgr *msgr);
> +extern void mpic_msgr_set_destination(struct mpic_msgr *msgr, u32 cpu_num);
> +extern int mpic_msgr_get_irq(struct mpic_msgr *msgr);

Documentation of the API please.

> +#endif
> diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
> index f7b0772..4d65593 100644
> --- a/arch/powerpc/platforms/Kconfig
> +++ b/arch/powerpc/platforms/Kconfig
> @@ -78,6 +78,14 @@ config MPIC_WEIRD
>  	bool
>  	default n
>  
> +config MPIC_MSGR
> +	bool "MPIC message register support"
> +	depends on MPIC
> +	default n
> +	help
> +	  Enables support for the MPIC message registers.  These
> +	  registers are used for inter-processor communication.
> +
>  config PPC_I8259
>  	bool
>  	default n
> diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile
> index 1e0c933..6d40185 100644
> --- a/arch/powerpc/sysdev/Makefile
> +++ b/arch/powerpc/sysdev/Makefile
> @@ -3,7 +3,8 @@ subdir-ccflags-$(CONFIG_PPC_WERROR) := -Werror
>  ccflags-$(CONFIG_PPC64)		:= -mno-minimal-toc
>  
>  mpic-msi-obj-$(CONFIG_PCI_MSI)	+= mpic_msi.o mpic_u3msi.o mpic_pasemi_msi.o
> -obj-$(CONFIG_MPIC)		+= mpic.o $(mpic-msi-obj-y)
> +mpic-msgr-obj-$(CONFIG_MPIC_MSGR)	+= mpic_msgr.o
> +obj-$(CONFIG_MPIC)		+= mpic.o $(mpic-msi-obj-y) $(mpic-msgr-obj-y)
>  fsl-msi-obj-$(CONFIG_PCI_MSI)	+= fsl_msi.o
>  obj-$(CONFIG_PPC_MSI_BITMAP)	+= msi_bitmap.o
>  
> diff --git a/arch/powerpc/sysdev/mpic_msgr.c b/arch/powerpc/sysdev/mpic_msgr.c
> new file mode 100644
> index 0000000..bfa0612
> --- /dev/null
> +++ b/arch/powerpc/sysdev/mpic_msgr.c
> @@ -0,0 +1,279 @@
> +/*
> + * Copyright 2011-2012, Meador Inge, Mentor Graphics Corporation.
> + *
> + * Some ideas based on un-pushed work done by Vivek Mahajan, Jason Jin, and
> + * Mingkai Hu from Freescale Semiconductor, Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2 of the
> + * License.
> + *
> + */
> +
> +#include <linux/list.h>
> +#include <linux/of_platform.h>
> +#include <linux/errno.h>
> +#include <asm/prom.h>
> +#include <asm/hw_irq.h>
> +#include <asm/ppc-pci.h>
> +#include <asm/mpic_msgr.h>
> +
> +#define MPIC_MSGR_REGISTERS_PER_BLOCK 4
> +#define MSGR_INUSE 0
> +#define MSGR_FREE 1
> +
> +/* Internal structure used *only* for IO mapping register blocks. */
> +struct mpic_msgr_block {
> +	struct msgr {
> +		u32 msgr;
> +		u8 res[12];
> +	} msgrs[MPIC_MSGR_REGISTERS_PER_BLOCK];
> +	u8 res0[192];
> +	u32 mer;
> +	u8 res1[12];
> +	u32 msr;
> +};

So this represent HW registers ? Please make it clear in the comment.
I'm not a terrible fan of using structures to map HW especially with so
few registers.

> +static struct mpic_msgr **mpic_msgrs = 0;
> +static unsigned int mpic_msgr_count = 0;
> +
> +struct mpic_msgr* mpic_msgr_get(unsigned int reg_num)
> +{
> +	struct mpic_msgr* msgr;
> +
> +	if (reg_num >= mpic_msgr_count)
> +		return ERR_PTR(-ENODEV);
> +
> +	msgr = mpic_msgrs[reg_num];

No locking on the array access, might be ok if those things are never
plugged in/out I suppose...

> +	if (atomic_cmpxchg(&msgr->in_use, MSGR_FREE, MSGR_INUSE) == MSGR_FREE)
> +		return msgr;
> +
> +	return ERR_PTR(-EBUSY);
> +}
> +EXPORT_SYMBOL(mpic_msgr_get);

So how are those things intended to be used ? Clients get a fixed
"register" number to use ? It looks like this stuff would have been
better off using some kind of allocator no ? A bitmap would have done
the job... or do you really need to have some kind of fixed numbering
associated with clients ?

> +void mpic_msgr_put(struct mpic_msgr* msgr)
> +{
> +	atomic_set(&msgr->in_use, MSGR_FREE);
> +}
> +EXPORT_SYMBOL(mpic_msgr_put);

Shouldn't it be disabled too while at it ?

> +void mpic_msgr_enable(struct mpic_msgr *msgr)
> +{
> +	out_be32(msgr->mer, in_be32(msgr->mer) | (1 << msgr->num));
> +}
> +EXPORT_SYMBOL(mpic_msgr_enable);

Why are all those exported non-GPL ? We have a policy of making new
in-kernel APIs generally GPL only.

> +void mpic_msgr_disable(struct mpic_msgr *msgr)
> +{
> +	out_be32(msgr->mer, in_be32(msgr->mer) & ~(1 << msgr->num));
> +}
> +EXPORT_SYMBOL(mpic_msgr_disable);

I see read-modify-write cycles here with no synchronization/locking
whatsoever... Might be ok as long as you document it in the doc of the
API, ie, the caller is required to synchronize.

> +void mpic_msgr_write(struct mpic_msgr *msgr, u32 message)
> +{
> +	out_be32(msgr->addr, message);
> +}
> +EXPORT_SYMBOL(mpic_msgr_write);
> +
> +u32 mpic_msgr_read(struct mpic_msgr *msgr)
> +{
> +	return in_be32(msgr->addr);
> +}
> +EXPORT_SYMBOL(mpic_msgr_read);

Those look like significant bloat/overhead for what is just an MMIO
wrapper on a register access, why not make them static inline's
instead ?

> +void mpic_msgr_clear(struct mpic_msgr *msgr)
> +{
> +	(void) mpic_msgr_read(msgr);
> +}
> +EXPORT_SYMBOL(mpic_msgr_clear);
> +
> +void mpic_msgr_set_destination(struct mpic_msgr *msgr, u32 cpu_num)
> +{
> +	out_be32(msgr->addr, 1 << cpu_num);
> +}
> +EXPORT_SYMBOL(mpic_msgr_set_destination);

Please make it clear that this takes a HW CPU number. How is that going
to be used ? Wouldn't you be better off having a __ variant taking the
HW CPU number an the "public" variant doing the conversion from a Linux
number ?

> +int mpic_msgr_get_irq(struct mpic_msgr *msgr)
> +{
> +	return msgr->irq;
> +}
> +EXPORT_SYMBOL(mpic_msgr_get_irq);
> +
> +/* The following three functions are used to compute the order and number of
> + * the message register blocks.  They are clearly very inefficent.  However,
> + * they are called *only* a few times during device initialization.
> + */
> +static unsigned int mpic_msgr_number_of_blocks(void)
> +{
> +	unsigned int count;
> +	struct device_node *aliases;
> +
> +	count = 0;
> +	aliases = of_find_node_by_name(NULL, "aliases");
> +
> +	if (aliases) {
> +		char buf[32];
> +
> +		for (;;) {
> +			snprintf(buf, sizeof(buf), "mpic-msgr-block%d", count);
> +			if (!of_find_property(aliases, buf, NULL))
> +				break;
> +
> +			count += 1;
> +		}
> +	}
> +
> +	return count;
> +}

I don't like relying on aliases as a way to figure out what HW is
present, that's not right. You should find the actual devices themselves
based on their compatible properties.

All of that stuff also assume these things are never going to be
hot-plugged. IE. will they never go in/out of partition in hypervisors
(or even be virtualized) ?

> +static unsigned int mpic_msgr_number_of_registers(void)
> +{
> +	return mpic_msgr_number_of_blocks() * MPIC_MSGR_REGISTERS_PER_BLOCK;
> +}
> +
> +static int mpic_msgr_block_number(struct device_node *node)
> +{
> +	struct device_node *aliases;
> +	unsigned int index, number_of_blocks;
> +	char buf[64];
> +
> +	number_of_blocks = mpic_msgr_number_of_blocks();
> +	aliases = of_find_node_by_name(NULL, "aliases");
> +	if (!aliases)
> +		return -1;
> +
> +	for (index = 0; index < number_of_blocks; ++index) {
> +		struct property *prop;
> +
> +		snprintf(buf, sizeof(buf), "mpic-msgr-block%d", index);
> +		prop = of_find_property(aliases, buf, NULL);
> +		if (node == of_find_node_by_path(prop->value))
> +			break;
> +	}
> +
> +	return index == number_of_blocks ? -1 : index;
> +}
> +
> +/* The probe function for a single message register block.
> + */
> +static __devinit int mpic_msgr_probe(struct platform_device *dev)
> +{
> +	struct mpic_msgr_block __iomem *msgr_block;
> +	int block_number;
> +	struct resource rsrc;
> +	unsigned int i;
> +	unsigned int irq_index;
> +	struct device_node *np = dev->dev.of_node;
> +	unsigned int receive_mask;
> +	const unsigned int *prop;
> +
> +	if (!np) {
> +		dev_err(&dev->dev, "Device OF-Node is NULL");
> +		return -EFAULT;
> +	}
> +
> +	/* Allocate the message register array upon the first device
> +	 * registered.
> +	 */
> +	if (!mpic_msgrs) {
> +		mpic_msgr_count = mpic_msgr_number_of_registers();
> +		dev_info(&dev->dev, "Found %d message registers\n", mpic_msgr_count);
> +
> +		mpic_msgrs = kzalloc(sizeof(struct mpic_msgr) * mpic_msgr_count,
> +							 GFP_KERNEL);
> +		if (!mpic_msgrs) {
> +			dev_err(&dev->dev, "No memory for message register blocks\n");
> +			return -ENOMEM;
> +		}
> +	}
> +	dev_info(&dev->dev, "Of-device full name %s\n", np->full_name);

Ok, so we have another scheme of:

 - Count all devices in the system of a given type
 - Assign them numbers
 - API uses number

That sucks... unless you have an allocator. And even then.

I'd rather clients use something like struct mpic_msgr (or msg_reg or
message_reg) as the "handle" to one of these things.

It can be obtained via an allocator or a device tree parsing routine if
there's a fixed relationship between clients and registers, I don't
really know how that stuff is to be used, but in any case, the whole
thing stinks as it is.

> +	/* IO map the message register block. */
> +	of_address_to_resource(np, 0, &rsrc);
> +	msgr_block = ioremap(rsrc.start, rsrc.end - rsrc.start);
> +	if (!msgr_block) {
> +		dev_err(&dev->dev, "Failed to iomap MPIC message registers");
> +		return -EFAULT;
> +	}
> +
> +	/* Ensure the block has a defined order. */
> +	block_number = mpic_msgr_block_number(np);
> +	if (block_number < 0) {
> +		dev_err(&dev->dev, "Failed to find message register block alias\n");
> +		return -ENODEV;
> +	}
> +	dev_info(&dev->dev, "Setting up message register block %d\n", block_number);
> +
> +	/* Grab the receive mask which specifies what registers can receive
> +	 * interrupts.
> +	 */
> +	prop = of_get_property(np, "mpic-msgr-receive-mask", NULL);
> +	receive_mask = (prop) ? *prop : 0xF;
> +
> +	/* Build up the appropriate message register data structures. */
> +	for (i = 0, irq_index = 0; i < MPIC_MSGR_REGISTERS_PER_BLOCK; ++i) {
> +		struct mpic_msgr *msgr;
> +		unsigned int reg_number;
> +
> +		msgr = kzalloc(sizeof(struct mpic_msgr), GFP_KERNEL);
> +		if (!msgr) {
> +			dev_err(&dev->dev, "No memory for message register\n");
> +			return -ENOMEM;
> +		}
> +
> +		reg_number = block_number * MPIC_MSGR_REGISTERS_PER_BLOCK + i;
> +		msgr->addr = &msgr_block->msgrs[i].msgr;
> +		msgr->mer = &msgr_block->mer;
> +		msgr->msr = &msgr_block->msr;
> +		atomic_set(&msgr->in_use, MSGR_FREE);
> +		msgr->num = reg_number;
> +
> +		if (receive_mask & (1 << i)) {
> +			struct resource irq;
> +
> +			if (of_irq_to_resource(np, irq_index, &irq) == NO_IRQ) {
> +				dev_err(&dev->dev, "Missing interrupt specifier");
> +				kfree(msgr);
> +				return -EFAULT;
> +			}
> +			msgr->irq = irq.start;
> +			irq_index += 1;
> +		} else {
> +			msgr->irq = NO_IRQ;
> +		}
> +
> +		mpic_msgrs[reg_number] = msgr;
> +		mpic_msgr_disable(msgr);
> +		dev_info(&dev->dev, "Register %d initialized: irq %d\n",
> +				 msgr->num, msgr->irq);
> +	
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mpic_msgr_ids[] = {
> +	{
> +		.compatible = "fsl,mpic-v3.1-msgr",
> +		.data = NULL,
> +	},
> +	{}
> +};
> +
> +static struct platform_driver mpic_msgr_driver = {
> +	.driver = {
> +		.name = "mpic-msgr",
> +		.owner = THIS_MODULE,
> +		.of_match_table = mpic_msgr_ids,
> +	},
> +	.probe = mpic_msgr_probe,
> +};
> +
> +static __init int mpic_msgr_init(void)
> +{
> +	return platform_driver_register(&mpic_msgr_driver);
> +}
> +subsys_initcall(mpic_msgr_init);

Ben.

Re: [openmcapi-dev] Re: [PATCH v3 2/2] powerpc: add support for MPIC message register API

[Meador Inge]

On 06/17/2011 12:33 AM, Benjamin Herrenschmidt wrote:

> On Tue, 2011-05-31 at 14:19 -0500, Meador Inge wrote:
>> Some MPIC implementations contain one or more blocks of message registers
>> that are used to send messages between cores via IPIs.  A simple API has
>> been added to access (get/put, read, write, etc ...) these message registers.
>> The available message registers are initially discovered via nodes in the
>> device tree.  A separate commit contains a binding for the message register
>> nodes.
> 
> Ok, so I finally got to look at that in a bit more details...
>> +#ifndef _ASM_MPIC_MSGR_H
>> +#define _ASM_MPIC_MSGR_H
>> +
>> +#include <linux/types.h>
>> +
>> +struct mpic_msgr {
>> +	u32 __iomem *addr;
>> +	u32 __iomem *mer;
>> +	u32 __iomem	*msr;
>> +	int irq;
>> +	atomic_t in_use;
>> +	int num;
>> +};
> 
> General comment... I'm really not fan of "msgr", I'd rather see
> "mpic_message_*", it's a tad more verbose but looks a lot better, no ?

It reads a little better.  I got 'msgr' from the HW manual.  Also, 'msgr'
is used in the device tree and file names too.  So I would like to use
'msgr' or 'message' everywhere.  I am OK with 'msgr'.

> Also do you need those 3 iomem pointers ? Not just one with fixed
> offsets ? Or do they come from vastly different sources ?

I can drop the 'msr' field as it is not used anymore.  I wouldn't say the
others are vastly different, but they are not right next to each other.  On the
MPC8572DS, for example, there is one MER for every message register block.  The
first block starts at offset 0x1400 (MSGR1), 0x1410 (MSGR2), 0x1420 (MSGR2),
0x1430 (MSGR3).  So you could do the math to get the MER from the MSGR.

> atomic_t in_use looks fishy, but let's see how you use it...
> 
>> +extern struct mpic_msgr* mpic_msgr_get(unsigned int reg_num);
>> +extern void mpic_msgr_put(struct mpic_msgr* msgr);
>> +extern void mpic_msgr_enable(struct mpic_msgr *msgr);
>> +extern void mpic_msgr_disable(struct mpic_msgr *msgr);
>> +extern void mpic_msgr_write(struct mpic_msgr *msgr, u32 message);
>> +extern u32 mpic_msgr_read(struct mpic_msgr *msgr);
>> +extern void mpic_msgr_clear(struct mpic_msgr *msgr);
>> +extern void mpic_msgr_set_destination(struct mpic_msgr *msgr, u32 cpu_num);
>> +extern int mpic_msgr_get_irq(struct mpic_msgr *msgr);
> 
> Documentation of the API please.

Will do.

>> +#endif
>> diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
>> index f7b0772..4d65593 100644
>> --- a/arch/powerpc/platforms/Kconfig
>> +++ b/arch/powerpc/platforms/Kconfig
>> @@ -78,6 +78,14 @@ config MPIC_WEIRD
>>  	bool
>>  	default n
>>  
>> +config MPIC_MSGR
>> +	bool "MPIC message register support"
>> +	depends on MPIC
>> +	default n
>> +	help
>> +	  Enables support for the MPIC message registers.  These
>> +	  registers are used for inter-processor communication.
>> +
>>  config PPC_I8259
>>  	bool
>>  	default n
>> diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile
>> index 1e0c933..6d40185 100644
>> --- a/arch/powerpc/sysdev/Makefile
>> +++ b/arch/powerpc/sysdev/Makefile
>> @@ -3,7 +3,8 @@ subdir-ccflags-$(CONFIG_PPC_WERROR) := -Werror
>>  ccflags-$(CONFIG_PPC64)		:= -mno-minimal-toc
>>  
>>  mpic-msi-obj-$(CONFIG_PCI_MSI)	+= mpic_msi.o mpic_u3msi.o mpic_pasemi_msi.o
>> -obj-$(CONFIG_MPIC)		+= mpic.o $(mpic-msi-obj-y)
>> +mpic-msgr-obj-$(CONFIG_MPIC_MSGR)	+= mpic_msgr.o
>> +obj-$(CONFIG_MPIC)		+= mpic.o $(mpic-msi-obj-y) $(mpic-msgr-obj-y)
>>  fsl-msi-obj-$(CONFIG_PCI_MSI)	+= fsl_msi.o
>>  obj-$(CONFIG_PPC_MSI_BITMAP)	+= msi_bitmap.o
>>  
>> diff --git a/arch/powerpc/sysdev/mpic_msgr.c b/arch/powerpc/sysdev/mpic_msgr.c
>> new file mode 100644
>> index 0000000..bfa0612
>> --- /dev/null
>> +++ b/arch/powerpc/sysdev/mpic_msgr.c
>> @@ -0,0 +1,279 @@
>> +/*
>> + * Copyright 2011-2012, Meador Inge, Mentor Graphics Corporation.
>> + *
>> + * Some ideas based on un-pushed work done by Vivek Mahajan, Jason Jin, and
>> + * Mingkai Hu from Freescale Semiconductor, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; version 2 of the
>> + * License.
>> + *
>> + */
>> +
>> +#include <linux/list.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/errno.h>
>> +#include <asm/prom.h>
>> +#include <asm/hw_irq.h>
>> +#include <asm/ppc-pci.h>
>> +#include <asm/mpic_msgr.h>
>> +
>> +#define MPIC_MSGR_REGISTERS_PER_BLOCK 4
>> +#define MSGR_INUSE 0
>> +#define MSGR_FREE 1
>> +
>> +/* Internal structure used *only* for IO mapping register blocks. */
>> +struct mpic_msgr_block {
>> +	struct msgr {
>> +		u32 msgr;
>> +		u8 res[12];
>> +	} msgrs[MPIC_MSGR_REGISTERS_PER_BLOCK];
>> +	u8 res0[192];
>> +	u32 mer;
>> +	u8 res1[12];
>> +	u32 msr;
>> +};
> 
> So this represent HW registers ? Please make it clear in the comment.
> I'm not a terrible fan of using structures to map HW especially with so
> few registers.

Yes, HW registers.  I will change the comment.

>> +	if (atomic_cmpxchg(&msgr->in_use, MSGR_FREE, MSGR_INUSE) == MSGR_FREE)
>> +		return msgr;
>> +
>> +	return ERR_PTR(-EBUSY);
>> +}
>> +EXPORT_SYMBOL(mpic_msgr_get);
> 
> So how are those things intended to be used ? Clients get a fixed
> "register" number to use ? It looks like this stuff would have been
> better off using some kind of allocator no ? A bitmap would have done
> the job... or do you really need to have some kind of fixed numbering
> associated with clients ?

A fixed numbering scheme really is needed.  We are using these registers
to communicate b/w different kernels in an AMP system.  In order
to do this each kernel has to be using the same numbering so that they
can communicate.

>> +void mpic_msgr_put(struct mpic_msgr* msgr)
>> +{
>> +	atomic_set(&msgr->in_use, MSGR_FREE);
>> +}
>> +EXPORT_SYMBOL(mpic_msgr_put);
> 
> Shouldn't it be disabled too while at it ?

Yes.

>> +void mpic_msgr_enable(struct mpic_msgr *msgr)
>> +{
>> +	out_be32(msgr->mer, in_be32(msgr->mer) | (1 << msgr->num));
>> +}
>> +EXPORT_SYMBOL(mpic_msgr_enable);
> 
> Why are all those exported non-GPL ? We have a policy of making new
> in-kernel APIs generally GPL only.

Oversight on my part, I will fix it.

>> +void mpic_msgr_disable(struct mpic_msgr *msgr)
>> +{
>> +	out_be32(msgr->mer, in_be32(msgr->mer) & ~(1 << msgr->num));
>> +}
>> +EXPORT_SYMBOL(mpic_msgr_disable);
> 
> I see read-modify-write cycles here with no synchronization/locking
> whatsoever... Might be ok as long as you document it in the doc of the
> API, ie, the caller is required to synchronize.

I will add locking.

>> +void mpic_msgr_write(struct mpic_msgr *msgr, u32 message)
>> +{
>> +	out_be32(msgr->addr, message);
>> +}
>> +EXPORT_SYMBOL(mpic_msgr_write);
>> +
>> +u32 mpic_msgr_read(struct mpic_msgr *msgr)
>> +{
>> +	return in_be32(msgr->addr);
>> +}
>> +EXPORT_SYMBOL(mpic_msgr_read);
> 
> Those look like significant bloat/overhead for what is just an MMIO
> wrapper on a register access, why not make them static inline's
> instead ?

I will make them 'static inline'.

>> +int mpic_msgr_get_irq(struct mpic_msgr *msgr)
>> +{
>> +	return msgr->irq;
>> +}
>> +EXPORT_SYMBOL(mpic_msgr_get_irq);
>> +
>> +/* The following three functions are used to compute the order and number of
>> + * the message register blocks.  They are clearly very inefficent.  However,
>> + * they are called *only* a few times during device initialization.
>> + */
>> +static unsigned int mpic_msgr_number_of_blocks(void)
>> +{
>> +	unsigned int count;
>> +	struct device_node *aliases;
>> +
>> +	count = 0;
>> +	aliases = of_find_node_by_name(NULL, "aliases");
>> +
>> +	if (aliases) {
>> +		char buf[32];
>> +
>> +		for (;;) {
>> +			snprintf(buf, sizeof(buf), "mpic-msgr-block%d", count);
>> +			if (!of_find_property(aliases, buf, NULL))
>> +				break;
>> +
>> +			count += 1;
>> +		}
>> +	}
>> +
>> +	return count;
>> +}
> 
> I don't like relying on aliases as a way to figure out what HW is
> present, that's not right. You should find the actual devices themselves
> based on their compatible properties.

In this case, the device is a block of message registers.  These are found by
probing the compatible properties.  However, once a _block_ is found the
register within it are numbered using the found aliases.  This is
due to the need for a fixed number scheme as mentioned previously.

Why a fixed numbering scheme is needed is always this first question about this
patch.  So, I should add a comment explaining all of this in the implementation.

> Ok, so we have another scheme of:
> 
>  - Count all devices in the system of a given type
>  - Assign them numbers
>  - API uses number
> 
> That sucks... unless you have an allocator. And even then.
> 
> I'd rather clients use something like struct mpic_msgr (or msg_reg or
> message_reg) as the "handle" to one of these things.

Most of the APIs *do* use a 'mpic_msgr' handle.  The only one that doesn't
is 'mpic_msgr_get', which takes the register number to retrieve.  We discussed
in the beginning having an allocator, but it didn't fit well with the AMP use
case we are working on.  There was a recent discussion on how a dynamic
allocator could be added in addition to the static one in PATCH v0:
http://patchwork.ozlabs.org/patch/92037/.

Thanks a lot for the review Ben.  Much appreciated.

-- 
Meador Inge
CodeSourcery / Mentor Embedded
http://www.mentor.com/embedded-software

Re: [PATCH v3 2/2] powerpc: add support for MPIC message register API

[Scott Wood]

On Fri, 17 Jun 2011 15:33:04 +1000
Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:

> On Tue, 2011-05-31 at 14:19 -0500, Meador Inge wrote:
> > +void mpic_msgr_enable(struct mpic_msgr *msgr)
> > +{
> > +	out_be32(msgr->mer, in_be32(msgr->mer) | (1 << msgr->num));
> > +}
> > +EXPORT_SYMBOL(mpic_msgr_enable);
> 
> Why are all those exported non-GPL ? We have a policy of making new
> in-kernel APIs generally GPL only.

>From Documentation/DocBook/kernel-hacking.tmpl:

  <sect1 id="sym-exportsymbols-gpl">
   <title><function>EXPORT_SYMBOL_GPL()</function>   
    <filename class="headerfile">include/linux/module.h</filename></title>
                                                     
   <para>                                            
    Similar to <function>EXPORT_SYMBOL()</function> except that the
    symbols exported by <function>EXPORT_SYMBOL_GPL()</function> can
    only be seen by modules with a                   
    <function>MODULE_LICENSE()</function> that specifies a GPL
    compatible license.  It implies that the function is considered
    an internal implementation issue, and not really an interface.
   </para>                                           
  </sect1>

When did this change from "considered an internal implementation issue, and
not really an interface" to "all new interfaces"?

-Scott

Re: [PATCH v3 2/2] powerpc: add support for MPIC message register API

[Scott Wood]

On Fri, 17 Jun 2011 15:33:04 +1000
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Tue, 2011-05-31 at 14:19 -0500, Meador Inge wrote:
> > +void mpic_msgr_enable(struct mpic_msgr *msgr)
> > +{
> > +	out_be32(msgr->mer, in_be32(msgr->mer) | (1 << msgr->num));
> > +}
> > +EXPORT_SYMBOL(mpic_msgr_enable);
>=20
> Why are all those exported non-GPL ? We have a policy of making new
> in-kernel APIs generally GPL only.

=46rom Documentation/DocBook/kernel-hacking.tmpl:

  <sect1 id=3D"sym-exportsymbols-gpl">
   <title><function>EXPORT_SYMBOL_GPL()</function>  =20
    <filename class=3D"headerfile">include/linux/module.h</filename></title>
                                                    =20
   <para>                                           =20
    Similar to <function>EXPORT_SYMBOL()</function> except that the
    symbols exported by <function>EXPORT_SYMBOL_GPL()</function> can
    only be seen by modules with a                  =20
    <function>MODULE_LICENSE()</function> that specifies a GPL
    compatible license.  It implies that the function is considered
    an internal implementation issue, and not really an interface.
   </para>                                          =20
  </sect1>

When did this change from "considered an internal implementation issue, and
not really an interface" to "all new interfaces"?

-Scott

Re: [PATCH v3 2/2] powerpc: add support for MPIC message register API

[Benjamin Herrenschmidt]

On Fri, 2011-06-17 at 11:58 -0500, Scott Wood wrote:
> When did this change from "considered an internal implementation
> issue, and not really an interface" to "all new interfaces"?

Interesting blurb... that's not everybody's opinion and I would argue
that supporting AMP kernels isn't something we want to do with closed
source crap.

Ben.

Re: [PATCH v3 2/2] powerpc: add support for MPIC message register API

[Scott Wood]

On Sat, 18 Jun 2011 08:58:53 +1000
Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:

> On Fri, 2011-06-17 at 11:58 -0500, Scott Wood wrote:
> > When did this change from "considered an internal implementation
> > issue, and not really an interface" to "all new interfaces"?
> 
> Interesting blurb... that's not everybody's opinion and I would argue
> that supporting AMP kernels isn't something we want to do with closed
> source crap.

I'm not advocating "closed source crap", just that if something is
"policy" (as opposed to opinion), it'd be nice if the documentation
actually matched.

-Scott

Re: [PATCH v3 2/2] powerpc: add support for MPIC message register API

[Scott Wood]

On Sat, 18 Jun 2011 08:58:53 +1000
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Fri, 2011-06-17 at 11:58 -0500, Scott Wood wrote:
> > When did this change from "considered an internal implementation
> > issue, and not really an interface" to "all new interfaces"?
> 
> Interesting blurb... that's not everybody's opinion and I would argue
> that supporting AMP kernels isn't something we want to do with closed
> source crap.

I'm not advocating "closed source crap", just that if something is
"policy" (as opposed to opinion), it'd be nice if the documentation
actually matched.

-Scott

Re: [PATCH v3 2/2] powerpc: add support for MPIC message register API

[Benjamin Herrenschmidt]

On Mon, 2011-06-20 at 10:59 -0500, Scott Wood wrote:
> On Sat, 18 Jun 2011 08:58:53 +1000
> Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:
> 
> > On Fri, 2011-06-17 at 11:58 -0500, Scott Wood wrote:
> > > When did this change from "considered an internal implementation
> > > issue, and not really an interface" to "all new interfaces"?
> > 
> > Interesting blurb... that's not everybody's opinion and I would argue
> > that supporting AMP kernels isn't something we want to do with closed
> > source crap.
> 
> I'm not advocating "closed source crap", just that if something is
> "policy" (as opposed to opinion), it'd be nice if the documentation
> actually matched.

Well, in Linux, the line between opinion and policy is quite blurred.

I don't know for sure what Linus himself thinks here and various other
maintainers have expressed various opinions as well. As far as I'm
concerned, I don't see the point in encouraging binary junk, especially
for low level interfaces like this one.

Cheers,
Ben.

Re: [PATCH v3 2/2] powerpc: add support for MPIC message register API

[Benjamin Herrenschmidt]

On Mon, 2011-06-20 at 10:59 -0500, Scott Wood wrote:
> On Sat, 18 Jun 2011 08:58:53 +1000
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> > On Fri, 2011-06-17 at 11:58 -0500, Scott Wood wrote:
> > > When did this change from "considered an internal implementation
> > > issue, and not really an interface" to "all new interfaces"?
> > 
> > Interesting blurb... that's not everybody's opinion and I would argue
> > that supporting AMP kernels isn't something we want to do with closed
> > source crap.
> 
> I'm not advocating "closed source crap", just that if something is
> "policy" (as opposed to opinion), it'd be nice if the documentation
> actually matched.

Well, in Linux, the line between opinion and policy is quite blurred.

I don't know for sure what Linus himself thinks here and various other
maintainers have expressed various opinions as well. As far as I'm
concerned, I don't see the point in encouraging binary junk, especially
for low level interfaces like this one.

Cheers,
Ben.
 

Re: [openmcapi-dev] Re: [PATCH v3 2/2] powerpc: add support for MPIC message register API

[Meador Inge]

On 06/17/2011 12:33 AM, Benjamin Herrenschmidt wrote:

> On Tue, 2011-05-31 at 14:19 -0500, Meador Inge wrote:
>> Some MPIC implementations contain one or more blocks of message registers
>> that are used to send messages between cores via IPIs.  A simple API has
>> been added to access (get/put, read, write, etc ...) these message registers.
>> The available message registers are initially discovered via nodes in the
>> device tree.  A separate commit contains a binding for the message register
>> nodes.

<snip>

> 
> Ok, so we have another scheme of:
> 
>  - Count all devices in the system of a given type
>  - Assign them numbers
>  - API uses number
> 
> That sucks... unless you have an allocator. And even then.
> 
> I'd rather clients use something like struct mpic_msgr (or msg_reg or
> message_reg) as the "handle" to one of these things.
> 
> It can be obtained via an allocator or a device tree parsing routine if
> there's a fixed relationship between clients and registers, I don't
> really know how that stuff is to be used, but in any case, the whole
> thing stinks as it is.

Ben,

I posted a more detailed response a few days back:
http://patchwork.ozlabs.org/patch/98075/.  In
that response, I tried to put forth the rationale
for allocating the registers statically due to
the AMP use case.  With that in mind, do you
still disagree with the design?  If so, do
you have any suggestions for how it could be
better?

-- 
Meador Inge
CodeSourcery / Mentor Embedded
http://www.mentor.com/embedded-software

Re: [openmcapi-dev] Re: [PATCH v3 2/2] powerpc: add support for MPIC message register API

[Benjamin Herrenschmidt]

On Wed, 2011-06-29 at 22:04 -0500, Meador Inge wrote:
> I posted a more detailed response a few days back:
> http://patchwork.ozlabs.org/patch/98075/.  In
> that response, I tried to put forth the rationale
> for allocating the registers statically due to
> the AMP use case.  With that in mind, do you
> still disagree with the design?  If so, do
> you have any suggestions for how it could be
> better? 

No, not really, please repost with my other comments addressed.

Cheers,
Ben.

Re: [openmcapi-dev] Re: [PATCH v3 2/2] powerpc: add support for MPIC message register API

[Benjamin Herrenschmidt]

On Wed, 2011-06-29 at 22:04 -0500, Meador Inge wrote:
> I posted a more detailed response a few days back:
> http://patchwork.ozlabs.org/patch/98075/.  In
> that response, I tried to put forth the rationale
> for allocating the registers statically due to
> the AMP use case.  With that in mind, do you
> still disagree with the design?  If so, do
> you have any suggestions for how it could be
> better? 

No, not really, please repost with my other comments addressed.

Cheers,
Ben.

Re: [openmcapi-dev] [PATCH v3 0/2] powerpc: define and implement MPIC message register support

[Meador Inge]

On 05/31/2011 02:19 PM, Meador Inge wrote:

> This patch set defines a binding for FSL MPIC message registers and implements
> an API for accessing those message registers.  Testing was done on a MPC8572DS
> in an Linux-Linux AMP setup using OpenMCAPI (www.openmcapi.org) to communicate
> between OS instances.  The message register API is used by the OpenMCAPI shared 
> memory driver to send notifications between cores.
> 
> * v3 - Clarified a point in the binding concerning the length of the
>        'interrupts' property.  Pointed out by Scott Wood.
> 
> * v2 - Incorporate feedback from Scott Wood
>    * Make binding less implementation specific.
>    * Add 'mpic-' prefix to message register node properties and aliases.
>    * Remove 'interrupt-parent' from binding.
>    * Fixed some example bugs with receive masks.
> 
> Signed-off-by: Meador Inge <meador_inge-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> Cc: Hollis Blanchard <hollis_blanchard-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> 
> Meador Inge (2):
>   powerpc: document the FSL MPIC message register binding

The binding has been acked (Thanks Scott).

>   powerpc: add support for MPIC message register API

Scott, Kumar, are you all OK with the
implementation: http://patchwork.ozlabs.org/patch/98075/ ?


-- 
Meador Inge
CodeSourcery / Mentor Embedded
http://www.mentor.com/embedded-software

Re: [openmcapi-dev] [PATCH v3 0/2] powerpc: define and implement MPIC message register support

[Meador Inge]

On 05/31/2011 02:19 PM, Meador Inge wrote:

> This patch set defines a binding for FSL MPIC message registers and implements
> an API for accessing those message registers.  Testing was done on a MPC8572DS
> in an Linux-Linux AMP setup using OpenMCAPI (www.openmcapi.org) to communicate
> between OS instances.  The message register API is used by the OpenMCAPI shared 
> memory driver to send notifications between cores.
> 
> * v3 - Clarified a point in the binding concerning the length of the
>        'interrupts' property.  Pointed out by Scott Wood.
> 
> * v2 - Incorporate feedback from Scott Wood
>    * Make binding less implementation specific.
>    * Add 'mpic-' prefix to message register node properties and aliases.
>    * Remove 'interrupt-parent' from binding.
>    * Fixed some example bugs with receive masks.
> 
> Signed-off-by: Meador Inge <meador_inge@mentor.com>
> Cc: Hollis Blanchard <hollis_blanchard@mentor.com>
> 
> Meador Inge (2):
>   powerpc: document the FSL MPIC message register binding

The binding has been acked (Thanks Scott).

>   powerpc: add support for MPIC message register API

Scott, Kumar, are you all OK with the
implementation: http://patchwork.ozlabs.org/patch/98075/ ?


-- 
Meador Inge
CodeSourcery / Mentor Embedded
http://www.mentor.com/embedded-software