[PATCH v2 0/6] Mailbox: Provide support STi based platforms

[PATCH v2 0/6] Mailbox: Provide support STi based platforms

[Lee Jones]

ST's platforms currently support a maximum of 5 Mailboxes, one for
each of the supported co-processors situated on the platform.  Each
Mailbox is divided up into 4 instances which consist of 32 channels.
Messages are passed between the application and co-processors using
shared memory areas.

Also included in the set is an example Client which should be generic
enough to use as a testing environment for all Mailbox IPs which use
shared memory to pass messages.  With a small change, it should also
be able to test more extravagant implementations too.

v1 => v2:
 - New MACRO() to obtain base address for a given instance
 - Move locking into the structure it protects
 - Stop checking for 'ready' state when sending data
 - Don't clear channel data (that belongs to the API)
 - #define register offsets instead of providing via pdata
 - Register driver with module_platform_driver()

Lee Jones (6):
  mailbox: dt: Supply bindings for ST's Mailbox IP
  mailbox: dt-bindings: Add shared [driver <=> device tree] defines
  mailbox: Add support for ST's Mailbox IP
  ARM: STi: stih407-family: Add nodes for Mailbox
  mailbox: Add generic mechanism for testing Mailbox Controllers
  ARM: STi: DT: STiH407: Enable Mailbox testing facility

 .../devicetree/bindings/mailbox/sti-mailbox.txt    |  53 +++
 arch/arm/boot/dts/stih407-family.dtsi              |  40 ++
 drivers/mailbox/Kconfig                            |  14 +
 drivers/mailbox/Makefile                           |   4 +
 drivers/mailbox/mailbox-sti.c                      | 527 +++++++++++++++++++++
 drivers/mailbox/mailbox-test.c                     | 210 ++++++++
 include/dt-bindings/mailbox/mailbox.h              |  14 +
 7 files changed, 862 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/sti-mailbox.txt
 create mode 100644 drivers/mailbox/mailbox-sti.c
 create mode 100644 drivers/mailbox/mailbox-test.c
 create mode 100644 include/dt-bindings/mailbox/mailbox.h

-- 
1.9.1

[PATCH v2 1/6] mailbox: dt: Supply bindings for ST's Mailbox IP

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 .../devicetree/bindings/mailbox/sti-mailbox.txt    | 53 ++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/sti-mailbox.txt

diff --git a/Documentation/devicetree/bindings/mailbox/sti-mailbox.txt b/Documentation/devicetree/bindings/mailbox/sti-mailbox.txt
new file mode 100644
index 0000000..3390e3e
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/sti-mailbox.txt
@@ -0,0 +1,53 @@
+ST Microelectronics Mailbox Driver
+
+Each ST Mailbox IP currently consists of 4 instances of 32 channels.  Messages
+are passed between Application and Remote processors using shared memory.
+
+Controller
+----------
+
+Required properties:
+- compatible		: Should be "st,stih407-mailbox"
+- reg			: Offset and length of the device's register set
+- mbox-name		: Name of the mailbox
+- #mbox-cells:		: Must be 3
+			  <&phandle instance channel direction>
+			    phandle   : Label name of controller
+			    instance  : Instance number
+			    channel   : Channel number
+			    direction : Data flow direction; MBOX_TX or MBOX_RX
+
+Optional properties
+- interrupts		: Contains the IRQ line for a Rx mailbox
+			    [NB: The absence of this property denotes Tx only]
+
+Example:
+
+mailbox0: mailbox@0  {
+	compatible	= "st,stih407-mailbox";
+	reg		= <0x8f00000 0x1000>;
+	interrupts	= <GIC_SPI 1 IRQ_TYPE_NONE>;
+	#mbox-cells	= <3>;
+	mbox-name	= "a9";
+};
+
+Client
+------
+
+Required properties:
+- compatible		: Many (See the client docs)
+- reg			: Shared (between Application and Remote) memory address
+- mboxes		: Standard property to specify a Mailbox (See ./mailbox.txt)
+			  Cells must match 'mbox-cells' (See Controller docs above)
+
+Optional properties
+- mbox-names		: Name given to channels seen in the 'mboxes' property.
+
+Example:
+
+mailbox_test {
+	compatible	= "mailbox_test";
+	reg		= <0x[shared_memory_address], [shared_memory_size]>;
+	mboxes		= <&mailbox2 0 1 MBOX_TX>, <&mailbox0 2 1 MBOX_RX>;
+	mbox-names	= "tx",	"rx";
+};
-- 
1.9.1

[PATCH v2 2/6] mailbox: dt-bindings: Add shared [driver <=> device tree] defines

[Lee Jones]

This header is currently only used for defines pertaining to data
direction i.e. Rx, Tx or Loopback.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 include/dt-bindings/mailbox/mailbox.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 include/dt-bindings/mailbox/mailbox.h

diff --git a/include/dt-bindings/mailbox/mailbox.h b/include/dt-bindings/mailbox/mailbox.h
new file mode 100644
index 0000000..82e929a
--- /dev/null
+++ b/include/dt-bindings/mailbox/mailbox.h
@@ -0,0 +1,14 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __MAILBOX_CONTROLLER_DT_BINDINGS_H
+#define __MAILBOX_CONTROLLER_DT_BINDINGS_H
+
+#define MBOX_TX		0x1
+#define MBOX_RX		0x2
+#define MBOX_LOOPBACK	(MBOX_RX | MBOX_TX)
+
+#endif /* __MAILBOX_CONTROLLER_DT_BINDINGS_H */
-- 
1.9.1

[PATCH v2 3/6] mailbox: Add support for ST's Mailbox IP

[Lee Jones]

ST's platforms currently support a maximum of 5 Mailboxes, one for
each of the supported co-processors situated on the platform.  Each
Mailbox is divided up into 4 instances which consist of 32 channels.
Messages are passed between the application and co-processors using
shared memory areas.  It is the Client's responsibility to manage
these areas.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mailbox/Kconfig       |   7 +
 drivers/mailbox/Makefile      |   2 +
 drivers/mailbox/mailbox-sti.c | 527 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 536 insertions(+)
 create mode 100644 drivers/mailbox/mailbox-sti.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index e269f08..2cc4c39 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -70,4 +70,11 @@ config BCM2835_MBOX
 	  the services of the Videocore. Say Y here if you want to use the
 	  BCM2835 Mailbox.
 
+config STI_MBOX
+	tristate "STI Mailbox framework support"
+	depends on ARCH_STI && OF
+	help
+	  Mailbox implementation for STMicroelectonics family chips with
+	  hardware for interprocessor communication.
+
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 8e6d822..7cb4766 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -13,3 +13,5 @@ obj-$(CONFIG_PCC)		+= pcc.o
 obj-$(CONFIG_ALTERA_MBOX)	+= mailbox-altera.o
 
 obj-$(CONFIG_BCM2835_MBOX)	+= bcm2835-mailbox.o
+
+obj-$(CONFIG_STI_MBOX)		+= mailbox-sti.o
diff --git a/drivers/mailbox/mailbox-sti.c b/drivers/mailbox/mailbox-sti.c
new file mode 100644
index 0000000..54679d8
--- /dev/null
+++ b/drivers/mailbox/mailbox-sti.c
@@ -0,0 +1,527 @@
+/*
+ * STi Mailbox
+ *
+ * Copyright (C) 2015 ST Microelectronics
+ *
+ * Author: Lee Jones <lee.jones@linaro.org> for ST Microelectronics
+ *
+ * Based on the original driver written by;
+ *   Alexandre Torgue, Olivier Lebreton and Loic Pallardy
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <dt-bindings/mailbox/mailbox.h>
+
+#include "mailbox.h"
+
+#define STI_MBOX_INST_MAX	4      /* RAM saving: Max supported instances */
+#define STI_MBOX_CHAN_MAX	20     /* RAM saving: Max supported channels  */
+
+#define STI_IRQ_VAL_OFFSET	0x04   /* Read interrupt status	              */
+#define STI_IRQ_SET_OFFSET	0x24   /* Generate a Tx channel interrupt     */
+#define STI_IRQ_CLR_OFFSET	0x44   /* Clear pending Rx interrupts	      */
+#define STI_ENA_VAL_OFFSET	0x64   /* Read enable status		      */
+#define STI_ENA_SET_OFFSET	0x84   /* Enable a channel		      */
+#define STI_ENA_CLR_OFFSET	0xa4   /* Disable a channel		      */
+
+#define MBOX_BASE(mdev, inst)   ((mdev)->base + (inst * 4))
+
+/**
+ * STi Mailbox device data
+ *
+ * An IP Mailbox is currently composed of 4 instances
+ * Each instance is currently composed of 32 channels
+ * This means that we have 128 channels per Mailbox
+ * A channel an be used for TX or RX
+ *
+ * @dev:	Device to which it is attached
+ * @mbox:	Representation of a communication channel controller
+ * @base:	Base address of the register mapping region
+ * @name:	Name of the mailbox
+ * @enabled:	Local copy of enabled channels
+ * @lock:	Mutex protecting enabled status
+ */
+struct sti_mbox_device {
+	struct device		*dev;
+	struct mbox_controller	*mbox;
+	void __iomem		*base;
+	const char		*name;
+	u32			enabled[STI_MBOX_INST_MAX];
+	spinlock_t		lock;
+};
+
+/**
+ * STi Mailbox platform specfic configuration
+ *
+ * @num_inst:	Maximum number of instances in one HW Mailbox
+ * @num_chan:	Maximum number of channel per instance
+ */
+struct sti_mbox_pdata {
+	unsigned int		num_inst;
+	unsigned int		num_chan;
+};
+
+/**
+ * STi Mailbox allocated channel information
+ *
+ * @mdev:	Pointer to parent Mailbox device
+ * @instance:	Instance number channel resides in
+ * @channel:	Channel number pertaining to this container
+ * @direction:	Direction which data will travel in through the channel (Tx/Rx)
+ */
+struct sti_channel {
+	struct sti_mbox_device	*mdev;
+	unsigned int		instance;
+	unsigned int		channel;
+	unsigned int		direction;
+};
+
+static inline bool sti_mbox_channel_is_enabled(struct mbox_chan *chan)
+{
+	struct sti_channel *chan_info = chan->con_priv;
+	struct sti_mbox_device *mdev = chan_info->mdev;
+	unsigned int instance = chan_info->instance;
+	unsigned int channel = chan_info->channel;
+	unsigned long flags;
+
+	spin_lock_irqsave(&mdev->lock, flags);
+	return mdev->enabled[instance] & BIT(channel);
+	spin_unlock_irqrestore(&mdev->lock, flags);
+}
+
+static inline
+struct mbox_chan *sti_mbox_to_channel(struct mbox_controller *mbox,
+				      unsigned int instance,
+				      unsigned int channel)
+{
+	struct sti_channel *chan_info;
+	int i;
+
+	for (i = 0; i < mbox->num_chans; i++) {
+		chan_info = mbox->chans[i].con_priv;
+		if (chan_info &&
+		    chan_info->instance == instance &&
+		    chan_info->channel == channel)
+			return &mbox->chans[i];
+	}
+
+	dev_err(mbox->dev,
+		"Channel not registered: instance: %d channel: %d\n",
+		instance, channel);
+
+	return NULL;
+}
+
+static void sti_mbox_enable_channel(struct mbox_chan *chan)
+{
+	struct sti_channel *chan_info = chan->con_priv;
+	struct sti_mbox_device *mdev = chan_info->mdev;
+	unsigned int instance = chan_info->instance;
+	unsigned int channel = chan_info->channel;
+	unsigned long flags;
+	void __iomem *base = MBOX_BASE(mdev, instance);
+
+	spin_lock_irqsave(&mdev->lock, flags);
+	mdev->enabled[instance] |= BIT(channel);
+	writel_relaxed(BIT(channel), base + STI_ENA_SET_OFFSET);
+	spin_unlock_irqrestore(&mdev->lock, flags);
+}
+
+static void sti_mbox_disable_channel(struct mbox_chan *chan)
+{
+	struct sti_channel *chan_info = chan->con_priv;
+	struct sti_mbox_device *mdev = chan_info->mdev;
+	unsigned int instance = chan_info->instance;
+	unsigned int channel = chan_info->channel;
+	unsigned long flags;
+	void __iomem *base = MBOX_BASE(mdev, instance);
+
+	spin_lock_irqsave(&mdev->lock, flags);
+	mdev->enabled[instance] &= ~BIT(channel);
+	writel_relaxed(BIT(channel), base + STI_ENA_CLR_OFFSET);
+	spin_unlock_irqrestore(&mdev->lock, flags);
+}
+
+static void sti_mbox_clear_irq(struct mbox_chan *chan)
+{
+	struct sti_channel *chan_info = chan->con_priv;
+	struct sti_mbox_device *mdev = chan_info->mdev;
+	unsigned int instance = chan_info->instance;
+	unsigned int channel = chan_info->channel;
+	void __iomem *base = MBOX_BASE(mdev, instance);
+
+	writel_relaxed(BIT(channel), base + STI_IRQ_CLR_OFFSET);
+}
+
+static struct mbox_chan *sti_mbox_irq_to_channel(struct sti_mbox_device *mdev,
+						 unsigned int instance)
+{
+	struct mbox_controller *mbox = mdev->mbox;
+	struct mbox_chan *chan = NULL;
+	unsigned int channel;
+	unsigned long bits;
+	void __iomem *base = MBOX_BASE(mdev, instance);
+
+	bits = readl_relaxed(base + STI_IRQ_VAL_OFFSET);
+	if (!bits)
+		/* No IRQs fired in specified instance */
+		return NULL;
+
+	/* An IRQ has fired, find the associated channel */
+	for (channel = 0; bits; channel++) {
+		if (!test_and_clear_bit(channel, &bits))
+			continue;
+
+		chan = sti_mbox_to_channel(mbox, instance, channel);
+		if (chan) {
+			dev_dbg(mbox->dev,
+				"IRQ fired on instance: %d channel: %d\n",
+				instance, channel);
+			break;
+		}
+	}
+
+	return chan;
+}
+
+static irqreturn_t sti_mbox_thread_handler(int irq, void *data)
+{
+	struct sti_mbox_device *mdev = data;
+	struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
+	struct mbox_chan *chan;
+	unsigned int instance;
+
+	for (instance = 0; instance < pdata->num_inst; instance++) {
+keep_looking:
+		chan = sti_mbox_irq_to_channel(mdev, instance);
+		if (!chan)
+			continue;
+
+		mbox_chan_received_data(chan, NULL);
+		sti_mbox_clear_irq(chan);
+		sti_mbox_enable_channel(chan);
+		goto keep_looking;
+	}
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t sti_mbox_irq_handler(int irq, void *data)
+{
+	struct sti_mbox_device *mdev = data;
+	struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
+	struct sti_channel *chan_info;
+	struct mbox_chan *chan;
+	unsigned int instance;
+	int ret = IRQ_NONE;
+
+	for (instance = 0; instance < pdata->num_inst; instance++) {
+		chan = sti_mbox_irq_to_channel(mdev, instance);
+		if (!chan)
+			continue;
+		chan_info = chan->con_priv;
+
+		if (!sti_mbox_channel_is_enabled(chan)) {
+			dev_warn(mdev->dev,
+				 "Unexpected IRQ: %s\n"
+				 "  instance: %d: channel: %d [enabled: %x]\n",
+				 mdev->name, chan_info->instance,
+				 chan_info->channel, mdev->enabled[instance]);
+
+			/* Only handle IRQ if no other valid IRQs were found */
+			if (ret == IRQ_NONE)
+				ret = IRQ_HANDLED;
+			continue;
+		}
+
+		sti_mbox_disable_channel(chan);
+		ret = IRQ_WAKE_THREAD;
+	}
+
+	if (ret == IRQ_NONE)
+		dev_err(mdev->dev, "Spurious IRQ - was a channel requested?\n");
+
+	return ret;
+}
+
+static bool sti_mbox_tx_is_ready(struct mbox_chan *chan)
+{
+	struct sti_channel *chan_info = chan->con_priv;
+	struct sti_mbox_device *mdev = chan_info->mdev;
+	unsigned int instance = chan_info->instance;
+	unsigned int channel = chan_info->channel;
+	void __iomem *base = MBOX_BASE(mdev, instance);
+
+	if (!(chan_info->direction & MBOX_TX))
+		return false;
+
+	if (!(readl_relaxed(base + STI_ENA_VAL_OFFSET) & BIT(channel))) {
+		dev_dbg(mdev->dev, "Mbox: %s: inst: %d, chan: %d disabled\n",
+			mdev->name, instance, channel);
+		return false;
+	}
+
+	if (readl_relaxed(base + STI_IRQ_VAL_OFFSET) & BIT(channel)) {
+		dev_dbg(mdev->dev, "Mbox: %s: inst: %d, chan: %d not ready\n",
+			mdev->name, instance, channel);
+		return false;
+	}
+
+	return true;
+}
+
+static int sti_mbox_send_data(struct mbox_chan *chan, void *data)
+{
+	struct sti_channel *chan_info = chan->con_priv;
+	struct sti_mbox_device *mdev = chan_info->mdev;
+	unsigned int instance = chan_info->instance;
+	unsigned int channel = chan_info->channel;
+	void __iomem *base = MBOX_BASE(mdev, instance);
+
+	/* Send event to co-processor */
+	writel_relaxed(BIT(channel), base + STI_IRQ_SET_OFFSET);
+
+	dev_dbg(mdev->dev,
+		"Sent via Mailbox %s: instance: %d channel: %d\n",
+		mdev->name, instance, channel);
+
+	return 0;
+}
+
+static int sti_mbox_startup_chan(struct mbox_chan *chan)
+{
+	sti_mbox_clear_irq(chan);
+	sti_mbox_enable_channel(chan);
+
+	return 0;
+}
+
+static void sti_mbox_shutdown_chan(struct mbox_chan *chan)
+{
+	struct sti_channel *chan_info = chan->con_priv;
+	struct mbox_controller *mbox = chan_info->mdev->mbox;
+	int i;
+
+	for (i = 0; i < mbox->num_chans; i++)
+		if (chan == &mbox->chans[i])
+			break;
+
+	if (mbox->num_chans == i) {
+		dev_warn(mbox->dev, "Request to free non-existent channel\n");
+		return;
+	}
+
+	/* Reset channel */
+	sti_mbox_disable_channel(chan);
+	sti_mbox_clear_irq(chan);
+	chan->con_priv = NULL;
+}
+
+static struct mbox_chan *sti_mbox_xlate(struct mbox_controller *mbox,
+					const struct of_phandle_args *spec)
+{
+	struct sti_mbox_device *mdev = dev_get_drvdata(mbox->dev);
+	struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
+	struct sti_channel *chan_info;
+	struct mbox_chan *chan = NULL;
+	unsigned int instance  = spec->args[0];
+	unsigned int channel   = spec->args[1];
+	unsigned int direction = spec->args[2];
+	int i;
+
+	/* Bounds checking */
+	if (instance >= pdata->num_inst || channel  >= pdata->num_chan) {
+		dev_err(mbox->dev,
+			"Invalid channel requested instance: %d channel: %d\n",
+			instance, channel);
+		return NULL;
+	}
+
+	for (i = 0; i < mbox->num_chans; i++) {
+		chan_info = mbox->chans[i].con_priv;
+
+		/* Is requested channel free? */
+		if (direction != MBOX_LOOPBACK &&
+		    chan_info &&
+		    mbox->dev == chan_info->mdev->dev &&
+		    instance == chan_info->instance &&
+		    channel == chan_info->channel) {
+			dev_err(mbox->dev, "Channel in use\n");
+			return NULL;
+		}
+
+		/*
+		 * Find the first free slot, then continue checking
+		 * to see if requested channel is in use
+		 */
+		if (!chan && !chan_info)
+			chan = &mbox->chans[i];
+	}
+
+	if (!chan) {
+		dev_err(mbox->dev, "No free channels left\n");
+		return NULL;
+	}
+
+	chan_info = devm_kzalloc(mbox->dev, sizeof(*chan_info), GFP_KERNEL);
+	if (!chan_info)
+		return NULL;
+
+	chan_info->mdev		= mdev;
+	chan_info->instance	= instance;
+	chan_info->channel	= channel;
+	chan_info->direction	= direction;
+
+	chan->con_priv = chan_info;
+
+	dev_info(mbox->dev,
+		 "Mbox: %s: Created channel:\n"
+		 "  instance: %d channel: %d direction: %s\n",
+		 mdev->name, instance, channel,
+		direction == MBOX_LOOPBACK ? "Loopback" :
+		direction == MBOX_TX ? "Tx" : "Rx");
+
+	return chan;
+}
+
+static struct mbox_chan_ops sti_mbox_ops = {
+	.startup	= sti_mbox_startup_chan,
+	.shutdown	= sti_mbox_shutdown_chan,
+	.send_data	= sti_mbox_send_data,
+	.last_tx_done	= sti_mbox_tx_is_ready,
+};
+
+static const struct sti_mbox_pdata mbox_stih407_pdata = {
+	.num_inst	= 4,
+	.num_chan	= 32,
+};
+
+static const struct of_device_id sti_mailbox_match[] = {
+	{
+		.compatible = "st,stih407-mailbox",
+		.data = (void *)&mbox_stih407_pdata
+	},
+	{ }
+};
+
+static int sti_mbox_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+	struct mbox_controller *mbox;
+	struct sti_mbox_device *mdev;
+	struct device_node *np = pdev->dev.of_node;
+	struct mbox_chan *chans;
+	struct resource *res;
+	int irq;
+	int ret;
+
+	match = of_match_device(sti_mailbox_match, &pdev->dev);
+	if (!match) {
+		dev_err(&pdev->dev, "No configuration found\n");
+		return -ENODEV;
+	}
+	pdev->dev.platform_data = (struct sti_mbox_pdata *) match->data;
+
+	mdev = devm_kzalloc(&pdev->dev, sizeof(*mdev), GFP_KERNEL);
+	if (!mdev)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, mdev);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mdev->base = devm_ioremap_resource(&pdev->dev, res);
+	if (!mdev->base)
+		return -ENOMEM;
+
+	ret = of_property_read_string(np, "mbox-name", &mdev->name);
+	if (ret)
+		mdev->name = np->full_name;
+
+	mbox = devm_kzalloc(&pdev->dev, sizeof(*mbox), GFP_KERNEL);
+	if (!mbox)
+		return -ENOMEM;
+
+	chans = devm_kzalloc(&pdev->dev,
+			     sizeof(*chans) * STI_MBOX_CHAN_MAX, GFP_KERNEL);
+	if (!chans)
+		return -ENOMEM;
+
+	mdev->dev		= &pdev->dev;
+	mdev->mbox		= mbox;
+
+	spin_lock_init(&mdev->lock);
+
+	/* STi Mailbox does not have a Tx-Done or Tx-Ready IRQ */
+	mbox->txdone_irq	= false;
+	mbox->txdone_poll	= true;
+	mbox->txpoll_period	= 100;
+	mbox->ops		= &sti_mbox_ops;
+	mbox->dev		= mdev->dev;
+	mbox->of_xlate		= sti_mbox_xlate;
+	mbox->chans		= chans;
+	mbox->num_chans		= STI_MBOX_CHAN_MAX;
+
+	ret = mbox_controller_register(mbox);
+	if (ret)
+		return ret;
+
+	/* It's okay for Tx Mailboxes to not supply IRQs */
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_info(&pdev->dev,
+			 "%s: Registered Tx only Mailbox\n", mdev->name);
+		return 0;
+	}
+
+	ret = devm_request_threaded_irq(&pdev->dev, irq,
+					sti_mbox_irq_handler,
+					sti_mbox_thread_handler,
+					IRQF_ONESHOT, mdev->name, mdev);
+	if (ret) {
+		dev_err(&pdev->dev, "Can't claim IRQ %d\n", irq);
+		mbox_controller_unregister(mbox);
+		return -EINVAL;
+	}
+
+	dev_info(&pdev->dev, "%s: Registered Tx/Rx Mailbox\n", mdev->name);
+
+	return 0;
+}
+
+static int sti_mbox_remove(struct platform_device *pdev)
+{
+	struct sti_mbox_device *mdev = platform_get_drvdata(pdev);
+
+	mbox_controller_unregister(mdev->mbox);
+
+	return 0;
+}
+
+static struct platform_driver sti_mbox_driver = {
+	.probe = sti_mbox_probe,
+	.remove = sti_mbox_remove,
+	.driver = {
+		.name = "sti-mailbox",
+		.of_match_table = sti_mailbox_match,
+	},
+};
+module_platform_driver(sti_mbox_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("STMicroelectronics Mailbox Controller");
+MODULE_AUTHOR("Lee Jones <lee.jones@linaro.org");
+MODULE_ALIAS("platform:mailbox-sti");
-- 
1.9.1

[PATCH v2 4/6] ARM: STi: stih407-family: Add nodes for Mailbox

[Lee Jones]

This patch supplies the Mailbox Controller nodes.  In order to
request channels, these nodes will be referenced by Mailbox
Client nodes.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih407-family.dtsi | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index 838b812..c061d11 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -11,6 +11,7 @@
 #include <dt-bindings/phy/phy.h>
 #include <dt-bindings/reset-controller/stih407-resets.h>
 #include <dt-bindings/interrupt-controller/irq-st.h>
+#include <dt-bindings/mailbox/mailbox.h>
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
@@ -565,5 +566,38 @@
 						  <&phy_port2 PHY_TYPE_USB3>;
 			};
 		};
+
+		mailbox0: mailbox@0  {
+			compatible	= "st,stih407-mailbox";
+			reg		= <0x8f00000 0x1000>;
+			interrupts	= <GIC_SPI 1 IRQ_TYPE_NONE>;
+			#mbox-cells	= <3>;
+			mbox-name	= "a9";
+			status		= "okay";
+		};
+
+		mailbox1: mailbox@1 {
+			compatible	= "st,stih407-mailbox";
+			reg		= <0x8f01000 0x1000>;
+			#mbox-cells	= <3>;
+			mbox-name	= "st231_gp_1";
+			status		= "okay";
+		};
+
+		mailbox2: mailbox@2 {
+			compatible	= "st,stih407-mailbox";
+			reg		= <0x8f02000 0x1000>;
+			#mbox-cells	= <3>;
+			mbox-name	= "st231_gp_0";
+			status		= "okay";
+		};
+
+		mailbox3: mailbox@3 {
+			compatible	= "st,stih407-mailbox";
+			reg		= <0x8f03000 0x1000>;
+			#mbox-cells	= <3>;
+			mbox-name	= "st231_audio_video";
+			status		= "okay";
+		};
 	};
 };
-- 
1.9.1

[PATCH v2 5/6] mailbox: Add generic mechanism for testing Mailbox Controllers

[Lee Jones]

This particular Client implementation uses shared memory in order
to pass messages between Mailbox users; however, it can be easily
hacked to support any type of Controller.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mailbox/Kconfig        |   7 ++
 drivers/mailbox/Makefile       |   2 +
 drivers/mailbox/mailbox-test.c | 210 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 219 insertions(+)
 create mode 100644 drivers/mailbox/mailbox-test.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 2cc4c39..7720bde 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -77,4 +77,11 @@ config STI_MBOX
 	  Mailbox implementation for STMicroelectonics family chips with
 	  hardware for interprocessor communication.
 
+config MAILBOX_TEST
+	tristate "Mailbox Test Client"
+	depends on OF
+	help
+	  Test client to help with testing new Controller driver
+	  implementations.
+
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 7cb4766..92435ef 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -2,6 +2,8 @@
 
 obj-$(CONFIG_MAILBOX)		+= mailbox.o
 
+obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
+
 obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
 
 obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
diff --git a/drivers/mailbox/mailbox-test.c b/drivers/mailbox/mailbox-test.c
new file mode 100644
index 0000000..10bfe3a
--- /dev/null
+++ b/drivers/mailbox/mailbox-test.c
@@ -0,0 +1,210 @@
+/*
+ * Copyright (C) 2015 ST Microelectronics
+ *
+ * Author: Lee Jones <lee.jones@linaro.org>
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+
+#define MBOX_MAX_MSG_LEN 128
+
+static struct dentry *root_debugfs_dir;
+
+struct mbox_test_device {
+	struct device		*dev;
+	struct mbox_chan	*tx_channel;
+	struct mbox_chan	*rx_channel;
+	void __iomem		*mmio;
+
+};
+
+static ssize_t mbox_test_write(struct file *filp,
+			       const char __user *userbuf,
+			       size_t count, loff_t *ppos)
+{
+	struct mbox_test_device *tdev = filp->private_data;
+	char *message;
+	int ret;
+
+	if (count > MBOX_MAX_MSG_LEN) {
+		dev_err(tdev->dev,
+			"Message length %d greater than max allowed %d\n",
+			count, MBOX_MAX_MSG_LEN);
+		return -EINVAL;
+	}
+
+	message = kzalloc(count, GFP_KERNEL);
+	if (!message)
+		return -ENOMEM;
+
+	ret = copy_from_user(message, userbuf, count);
+	if (ret)
+		return -EFAULT;
+
+	print_hex_dump(KERN_ERR, "Client: Sending: ", DUMP_PREFIX_ADDRESS,
+		       16, 1, message, 16, true);
+
+	ret = mbox_send_message(tdev->tx_channel, message);
+	if (ret < 0)
+		dev_err(tdev->dev, "Failed to send message via mailbox\n");
+
+	kfree(message);
+
+	return ret < 0 ? ret : count;
+}
+
+static const struct file_operations mbox_test_ops = {
+	.write	= mbox_test_write,
+	.open	= simple_open,
+	.llseek	= generic_file_llseek,
+};
+
+static int mbox_test_add_debugfs(struct platform_device *pdev,
+				 struct mbox_test_device *tdev)
+{
+	if (!debugfs_initialized())
+		return 0;
+
+	root_debugfs_dir = debugfs_create_dir("mailbox", NULL);
+	if (!root_debugfs_dir) {
+		dev_err(&pdev->dev, "Failed to create Mailbox debugfs\n");
+		return -EINVAL;
+	}
+
+	debugfs_create_file("send-message", 0200, root_debugfs_dir,
+			    tdev, &mbox_test_ops);
+
+	return 0;
+}
+
+static void mbox_test_receive_message(struct mbox_client *client, void *message)
+{
+	struct mbox_test_device *tdev = dev_get_drvdata(client->dev);
+
+	if (!tdev->mmio) {
+		dev_info(tdev->dev, "Client: Recived something [read mmio]\n");
+		return;
+	}
+
+	print_hex_dump(KERN_ERR, "Client: from co-proc: ", DUMP_PREFIX_ADDRESS,
+		       16, 1, tdev->mmio, MBOX_MAX_MSG_LEN, true);
+}
+
+static void mbox_test_prepare_message(struct mbox_client *client, void *message)
+{
+	struct mbox_test_device *tdev = dev_get_drvdata(client->dev);
+
+	if (tdev->mmio)
+		memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
+}
+
+static void mbox_test_message_sent(struct mbox_client *client,
+				   void *message, int r)
+{
+	if (r)
+		dev_warn(client->dev,
+			 "Client: Message could not be sent: %d\n", r);
+	else
+		dev_info(client->dev,
+			 "Client: Message sent\n");
+}
+
+static struct mbox_chan *
+mbox_test_request_channel(struct platform_device *pdev, const char *name)
+{
+	struct mbox_client *client;
+
+	client = devm_kzalloc(&pdev->dev, sizeof(*client), GFP_KERNEL);
+	if (!client)
+		return ERR_PTR(-ENOMEM);
+
+	client->dev		= &pdev->dev;
+	client->rx_callback	= mbox_test_receive_message;
+	client->tx_prepare	= mbox_test_prepare_message;
+	client->tx_done		= mbox_test_message_sent;
+	client->tx_block	= true;
+	client->knows_txdone	= false;
+	client->tx_tout		= 500;
+
+	return mbox_request_channel_byname(client, name);
+}
+
+static int mbox_test_probe(struct platform_device *pdev)
+{
+	struct mbox_test_device *tdev;
+	struct resource *res;
+	int ret;
+
+	tdev = devm_kzalloc(&pdev->dev, sizeof(*tdev), GFP_KERNEL);
+	if (!tdev)
+		return -ENOMEM;
+
+	/* It's okay for MMIO to be NULL */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	tdev->mmio = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(tdev->mmio))
+		tdev->mmio = NULL;
+
+	tdev->tx_channel = mbox_test_request_channel(pdev, "tx");
+	if (IS_ERR(tdev->tx_channel))
+		return PTR_ERR(tdev->tx_channel);
+
+	tdev->rx_channel = mbox_test_request_channel(pdev, "rx");
+	if (IS_ERR(tdev->rx_channel))
+		return PTR_ERR(tdev->rx_channel);
+
+	tdev->dev = &pdev->dev;
+	platform_set_drvdata(pdev, tdev);
+
+	ret = mbox_test_add_debugfs(pdev, tdev);
+	if (ret)
+		return ret;
+
+	dev_info(&pdev->dev, "Successfully registered\n");
+
+	return 0;
+}
+
+static int mbox_test_remove(struct platform_device *pdev)
+{
+	struct mbox_test_device *tdev = platform_get_drvdata(pdev);
+
+	debugfs_remove_recursive(root_debugfs_dir);
+
+	mbox_free_channel(tdev->tx_channel);
+	mbox_free_channel(tdev->rx_channel);
+
+	return 0;
+}
+
+static const struct of_device_id mbox_test_match[] = {
+	{ .compatible = "mailbox_test" },
+	{},
+};
+
+static struct platform_driver mbox_test_driver = {
+	.driver = {
+		.name = "mailbox_sti_test",
+		.of_match_table = mbox_test_match,
+	},
+	.probe  = mbox_test_probe,
+	.remove = mbox_test_remove,
+};
+module_platform_driver(mbox_test_driver);
+
+MODULE_DESCRIPTION("Generic Mailbox Testing Facility");
+MODULE_AUTHOR("Lee Jones <lee.jones@linaro.org");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCH v2 6/6] ARM: STi: DT: STiH407: Enable Mailbox testing facility

[Lee Jones]

This patch supplies a Client node to enable the Mailbox testing
facility.  It will be used to send and receive messages from any
given co-processor in order to test the STi Mailbox Controller
driver.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih407-family.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index c061d11..df7fe80 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -567,6 +567,12 @@
 			};
 		};
 
+		mailbox_test {
+			compatible = "mailbox_test";
+			mboxes = <&mailbox2 1 31 MBOX_TX>, <&mailbox0 1 31 MBOX_RX>;
+			mbox-names = "tx", "rx";
+		};
+
 		mailbox0: mailbox@0  {
 			compatible	= "st,stih407-mailbox";
 			reg		= <0x8f00000 0x1000>;
-- 
1.9.1

Re: [PATCH v2 3/6] mailbox: Add support for ST's Mailbox IP

[Paul Bolle]

On ma, 2015-07-27 at 10:44 +0100, Lee Jones wrote:
> --- /dev/null
> +++ b/drivers/mailbox/mailbox-sti.c

> +static int sti_mbox_probe(struct platform_device *pdev)
> +{
> +	[...]
> +
> +	match = of_match_device(sti_mailbox_match, &pdev->dev);
> +	if (!match) {
> +		dev_err(&pdev->dev, "No configuration found\n");
> +		return -ENODEV;
> +	}
> +	pdev->dev.platform_data = (struct sti_mbox_pdata *) match->data;
> +
> +	[...]
> +}

> +static struct platform_driver sti_mbox_driver = {
> +	.probe = sti_mbox_probe,
> +	.remove = sti_mbox_remove,
> +	.driver = {
> +		.name = "sti-mailbox",
> +		.of_match_table = sti_mailbox_match,
> +	},
> +};
> +module_platform_driver(sti_mbox_driver);

> +MODULE_ALIAS("platform:mailbox-sti");

It seems this alias is only useful if there's a corresponding struct
platform_device with a "mailbox-sti" .name. I couldn't spot that
platform_device.

Besides, if I read sti_max_probe() correctly, without OF support loading
this module won't accomplish much. So what would another way of
autoloading this module buy you?

Thanks,


Paul Bolle

Re: [PATCH v2 3/6] mailbox: Add support for ST's Mailbox IP

[Lee Jones]

On Wed, 29 Jul 2015, Paul Bolle wrote:

> On ma, 2015-07-27 at 10:44 +0100, Lee Jones wrote:
> > --- /dev/null
> > +++ b/drivers/mailbox/mailbox-sti.c
> 
> > +static int sti_mbox_probe(struct platform_device *pdev)
> > +{
> > +	[...]
> > +
> > +	match = of_match_device(sti_mailbox_match, &pdev->dev);
> > +	if (!match) {
> > +		dev_err(&pdev->dev, "No configuration found\n");
> > +		return -ENODEV;
> > +	}
> > +	pdev->dev.platform_data = (struct sti_mbox_pdata *) match->data;
> > +
> > +	[...]
> > +}
> 
> > +static struct platform_driver sti_mbox_driver = {
> > +	.probe = sti_mbox_probe,
> > +	.remove = sti_mbox_remove,
> > +	.driver = {
> > +		.name = "sti-mailbox",
> > +		.of_match_table = sti_mailbox_match,
> > +	},
> > +};
> > +module_platform_driver(sti_mbox_driver);
> 
> > +MODULE_ALIAS("platform:mailbox-sti");
> 
> It seems this alias is only useful if there's a corresponding struct
> platform_device with a "mailbox-sti" .name. I couldn't spot that
> platform_device.
> 
> Besides, if I read sti_max_probe() correctly, without OF support loading
> this module won't accomplish much. So what would another way of
> autoloading this module buy you?

I think this line can be safely removed.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 3/6] mailbox: Add support for ST's Mailbox IP

[Paul Bolle]

On do, 2015-07-30 at 12:45 +0100, Lee Jones wrote:
> On Wed, 29 Jul 2015, Paul Bolle wrote:
> > Besides, if I read sti_max_probe() correctly, without OF support
> > loading
> > this module won't accomplish much. So what would another way of
> > autoloading this module buy you?
> 
> I think this line can be safely removed.

Looking at this patch a little more I notice there's no line reading
     MODULE_DEVICE_TABLE(of, sti_mailbox_match);

The three mailbox drivers that currently support OF do have such a line.
So "another way of autoloading" was rather sloppy as now I don't see how
this driver, if build as a module, will get auto-loaded. Maybe there's
an auto-loading mechanism that I'm not aware of here. Or perhaps auto
-loading is not needed.

Similar question for patch 5/6 and the lack of a line reading
    MODULE_DEVICE_TABLE(of, mbox_test_match);

Thanks,


Paul Bolle

Re: [PATCH v2 3/6] mailbox: Add support for ST's Mailbox IP

[Lee Jones]

On Thu, 30 Jul 2015, Paul Bolle wrote:

> On do, 2015-07-30 at 12:45 +0100, Lee Jones wrote:
> > On Wed, 29 Jul 2015, Paul Bolle wrote:
> > > Besides, if I read sti_max_probe() correctly, without OF support
> > > loading
> > > this module won't accomplish much. So what would another way of
> > > autoloading this module buy you?
> > 
> > I think this line can be safely removed.
> 
> Looking at this patch a little more I notice there's no line reading
>      MODULE_DEVICE_TABLE(of, sti_mailbox_match);
> 
> The three mailbox drivers that currently support OF do have such a line.
> So "another way of autoloading" was rather sloppy as now I don't see how
> this driver, if build as a module, will get auto-loaded. Maybe there's
> an auto-loading mechanism that I'm not aware of here. Or perhaps auto
> -loading is not needed.
> 
> Similar question for patch 5/6 and the lack of a line reading
>     MODULE_DEVICE_TABLE(of, mbox_test_match);

Yes, both of those lines should be present.

FYW, I don't test these drivers as modules and have no desire to.  I
always build everything into a single kernel binary, so any feedback
regarding the module enabling code is appreciated.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 5/6] mailbox: Add generic mechanism for testing Mailbox Controllers

[Jassi Brar]

On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:
> This particular Client implementation uses shared memory in order
> to pass messages between Mailbox users; however, it can be easily
> hacked to support any type of Controller.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/mailbox/Kconfig        |   7 ++
>  drivers/mailbox/Makefile       |   2 +
>  drivers/mailbox/mailbox-test.c | 210 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 219 insertions(+)
>  create mode 100644 drivers/mailbox/mailbox-test.c
>
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index 2cc4c39..7720bde 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -77,4 +77,11 @@ config STI_MBOX
>           Mailbox implementation for STMicroelectonics family chips with
>           hardware for interprocessor communication.
>
> +config MAILBOX_TEST
> +       tristate "Mailbox Test Client"
> +       depends on OF
> +       help
> +         Test client to help with testing new Controller driver
> +         implementations.
> +
>  endif
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index 7cb4766..92435ef 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -2,6 +2,8 @@
>
>  obj-$(CONFIG_MAILBOX)          += mailbox.o
>
> +obj-$(CONFIG_MAILBOX_TEST)     += mailbox-test.o
> +
>  obj-$(CONFIG_ARM_MHU)  += arm_mhu.o
>
>  obj-$(CONFIG_PL320_MBOX)       += pl320-ipc.o
> diff --git a/drivers/mailbox/mailbox-test.c b/drivers/mailbox/mailbox-test.c
> new file mode 100644
> index 0000000..10bfe3a
> --- /dev/null
> +++ b/drivers/mailbox/mailbox-test.c
> @@ -0,0 +1,210 @@
> +/*
> + * Copyright (C) 2015 ST Microelectronics
> + *
> + * Author: Lee Jones <lee.jones@linaro.org>
> + *
> + * 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; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/debugfs.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +
> +#define MBOX_MAX_MSG_LEN 128
> +
> +static struct dentry *root_debugfs_dir;
> +
> +struct mbox_test_device {
> +       struct device           *dev;
> +       struct mbox_chan        *tx_channel;
> +       struct mbox_chan        *rx_channel;
> +       void __iomem            *mmio;
> +
> +};
> +
> +static ssize_t mbox_test_write(struct file *filp,
> +                              const char __user *userbuf,
> +                              size_t count, loff_t *ppos)
> +{
> +       struct mbox_test_device *tdev = filp->private_data;
> +       char *message;
> +       int ret;
> +
> +       if (count > MBOX_MAX_MSG_LEN) {
> +               dev_err(tdev->dev,
> +                       "Message length %d greater than max allowed %d\n",
> +                       count, MBOX_MAX_MSG_LEN);
> +               return -EINVAL;
> +       }
> +
> +       message = kzalloc(count, GFP_KERNEL);
> +       if (!message)
> +               return -ENOMEM;
> +
> +       ret = copy_from_user(message, userbuf, count);
> +       if (ret)
> +               return -EFAULT;
> +
> +       print_hex_dump(KERN_ERR, "Client: Sending: ", DUMP_PREFIX_ADDRESS,
> +                      16, 1, message, 16, true);
> +
> +       ret = mbox_send_message(tdev->tx_channel, message);
> +       if (ret < 0)
> +               dev_err(tdev->dev, "Failed to send message via mailbox\n");
> +
> +       kfree(message);
> +
> +       return ret < 0 ? ret : count;
> +}
> +
> +static const struct file_operations mbox_test_ops = {
> +       .write  = mbox_test_write,
> +       .open   = simple_open,
> +       .llseek = generic_file_llseek,
> +};
> +
> +static int mbox_test_add_debugfs(struct platform_device *pdev,
> +                                struct mbox_test_device *tdev)
> +{
> +       if (!debugfs_initialized())
> +               return 0;
> +
> +       root_debugfs_dir = debugfs_create_dir("mailbox", NULL);
> +       if (!root_debugfs_dir) {
> +               dev_err(&pdev->dev, "Failed to create Mailbox debugfs\n");
> +               return -EINVAL;
> +       }
> +
> +       debugfs_create_file("send-message", 0200, root_debugfs_dir,
> +                           tdev, &mbox_test_ops);
> +
> +       return 0;
> +}
> +
> +static void mbox_test_receive_message(struct mbox_client *client, void *message)
> +{
> +       struct mbox_test_device *tdev = dev_get_drvdata(client->dev);
> +
> +       if (!tdev->mmio) {
> +               dev_info(tdev->dev, "Client: Recived something [read mmio]\n");
> +               return;
> +       }
> +
> +       print_hex_dump(KERN_ERR, "Client: from co-proc: ", DUMP_PREFIX_ADDRESS,
> +                      16, 1, tdev->mmio, MBOX_MAX_MSG_LEN, true);
> +}
> +
> +static void mbox_test_prepare_message(struct mbox_client *client, void *message)
> +{
> +       struct mbox_test_device *tdev = dev_get_drvdata(client->dev);
> +
> +       if (tdev->mmio)
> +               memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
>
This is unlikely to work. Those platforms that need to send a 2 part
message, they do :
(a) Signal/Command/Target code via some controller register (via
mbox_send_message).
(b) Setup the payload in Shared-Memory (via tx_prepare).

This test driver assumes both are the same. I think you have to
declare something like

struct mbox_test_message { /* same for TX and RX */
          unsigned sig_len;
          void *signal;               /* rx/tx via mailbox api */
          unsigned pl_len;
          void *payload;           /* rx/tx via shared-memory */
};

> +
> +static void mbox_test_message_sent(struct mbox_client *client,
> +                                  void *message, int r)
> +{
> +       if (r)
> +               dev_warn(client->dev,
> +                        "Client: Message could not be sent: %d\n", r);
> +       else
> +               dev_info(client->dev,
> +                        "Client: Message sent\n");
> +}
> +
> +static struct mbox_chan *
> +mbox_test_request_channel(struct platform_device *pdev, const char *name)
> +{
> +       struct mbox_client *client;
> +
> +       client = devm_kzalloc(&pdev->dev, sizeof(*client), GFP_KERNEL);
> +       if (!client)
> +               return ERR_PTR(-ENOMEM);
> +
> +       client->dev             = &pdev->dev;
> +       client->rx_callback     = mbox_test_receive_message;
> +       client->tx_prepare      = mbox_test_prepare_message;
> +       client->tx_done         = mbox_test_message_sent;
> +       client->tx_block        = true;
> +       client->knows_txdone    = false;
> +       client->tx_tout         = 500;
> +
> +       return mbox_request_channel_byname(client, name);
> +}
> +
> +static int mbox_test_probe(struct platform_device *pdev)
> +{
> +       struct mbox_test_device *tdev;
> +       struct resource *res;
> +       int ret;
> +
> +       tdev = devm_kzalloc(&pdev->dev, sizeof(*tdev), GFP_KERNEL);
> +       if (!tdev)
> +               return -ENOMEM;
> +
> +       /* It's okay for MMIO to be NULL */
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       tdev->mmio = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(tdev->mmio))
> +               tdev->mmio = NULL;
> +
> +       tdev->tx_channel = mbox_test_request_channel(pdev, "tx");
> +       if (IS_ERR(tdev->tx_channel))
> +               return PTR_ERR(tdev->tx_channel);
> +
> +       tdev->rx_channel = mbox_test_request_channel(pdev, "rx");
> +       if (IS_ERR(tdev->rx_channel))
> +               return PTR_ERR(tdev->rx_channel);
> +
Should it really fail on TX or RX only clients?
It takes write from userspace but shouldn't it also provide data
received to the userspace?

Cheers.
Jassi

Re: [PATCH v2 2/6] mailbox: dt-bindings: Add shared [driver <=> device tree] defines

[Jassi Brar]

On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:
> This header is currently only used for defines pertaining to data
> direction i.e. Rx, Tx or Loopback.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  include/dt-bindings/mailbox/mailbox.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>  create mode 100644 include/dt-bindings/mailbox/mailbox.h
>
> diff --git a/include/dt-bindings/mailbox/mailbox.h b/include/dt-bindings/mailbox/mailbox.h
> new file mode 100644
> index 0000000..82e929a
> --- /dev/null
> +++ b/include/dt-bindings/mailbox/mailbox.h
> @@ -0,0 +1,14 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef __MAILBOX_CONTROLLER_DT_BINDINGS_H
> +#define __MAILBOX_CONTROLLER_DT_BINDINGS_H
> +
> +#define MBOX_TX                0x1
> +#define MBOX_RX                0x2
> +#define MBOX_LOOPBACK  (MBOX_RX | MBOX_TX)
> +
Not sure I understand 'loopback'.  Does it mean h/w has some
'loopback' mode for testing purposes? Or it simply means the
controller can send as well as receive messages?

Thanks.

Re: [PATCH v2 2/6] mailbox: dt-bindings: Add shared [driver <=> device tree] defines

[Lee Jones]

On Mon, 10 Aug 2015, Jassi Brar wrote:
> On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > This header is currently only used for defines pertaining to data
> > direction i.e. Rx, Tx or Loopback.
> >
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  include/dt-bindings/mailbox/mailbox.h | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >  create mode 100644 include/dt-bindings/mailbox/mailbox.h
> >
> > diff --git a/include/dt-bindings/mailbox/mailbox.h b/include/dt-bindings/mailbox/mailbox.h
> > new file mode 100644
> > index 0000000..82e929a
> > --- /dev/null
> > +++ b/include/dt-bindings/mailbox/mailbox.h
> > @@ -0,0 +1,14 @@
> > +/*
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#ifndef __MAILBOX_CONTROLLER_DT_BINDINGS_H
> > +#define __MAILBOX_CONTROLLER_DT_BINDINGS_H
> > +
> > +#define MBOX_TX                0x1
> > +#define MBOX_RX                0x2
> > +#define MBOX_LOOPBACK  (MBOX_RX | MBOX_TX)
> > +
> Not sure I understand 'loopback'.  Does it mean h/w has some
> 'loopback' mode for testing purposes? Or it simply means the
> controller can send as well as receive messages?

'loopback' allows firmware to conduct some early function tests.
However, channels are simplex, so we provide protection against
multiple allocation of single channel.  By allocating a LOOPBACK
channel we over-ride this protection and allow a single channel to be
allocated twice, once for Rx and the other for Tx.

This mode was born out of a _real_ use-case.  Some of ST's firmware
running on products actually uses the loopback feature.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 2/6] mailbox: dt-bindings: Add shared [driver <=> device tree] defines

[Jassi Brar]

On Mon, Aug 10, 2015 at 1:54 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Mon, 10 Aug 2015, Jassi Brar wrote:
>> On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> > This header is currently only used for defines pertaining to data
>> > direction i.e. Rx, Tx or Loopback.
>> >
>> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
>> > ---
>> >  include/dt-bindings/mailbox/mailbox.h | 14 ++++++++++++++
>> >  1 file changed, 14 insertions(+)
>> >  create mode 100644 include/dt-bindings/mailbox/mailbox.h
>> >
>> > diff --git a/include/dt-bindings/mailbox/mailbox.h b/include/dt-bindings/mailbox/mailbox.h
>> > new file mode 100644
>> > index 0000000..82e929a
>> > --- /dev/null
>> > +++ b/include/dt-bindings/mailbox/mailbox.h
>> > @@ -0,0 +1,14 @@
>> > +/*
>> > + * This program is free software; you can redistribute it and/or modify
>> > + * it under the terms of the GNU General Public License version 2 as
>> > + * published by the Free Software Foundation.
>> > + */
>> > +
>> > +#ifndef __MAILBOX_CONTROLLER_DT_BINDINGS_H
>> > +#define __MAILBOX_CONTROLLER_DT_BINDINGS_H
>> > +
>> > +#define MBOX_TX                0x1
>> > +#define MBOX_RX                0x2
>> > +#define MBOX_LOOPBACK  (MBOX_RX | MBOX_TX)
>> > +
>> Not sure I understand 'loopback'.  Does it mean h/w has some
>> 'loopback' mode for testing purposes? Or it simply means the
>> controller can send as well as receive messages?
>
> 'loopback' allows firmware to conduct some early function tests.
> However, channels are simplex, so we provide protection against
> multiple allocation of single channel.  By allocating a LOOPBACK
> channel we over-ride this protection and allow a single channel to be
> allocated twice, once for Rx and the other for Tx.
>
So basically hardware is half-duplex, not simplex. I think maybe you
should simply allow for RX and TX always. It should work. Just
handover any received data before send_data (reflecting the h/w
limitation). That way you don't need any such special flag.

Re: [PATCH v2 2/6] mailbox: dt-bindings: Add shared [driver <=> device tree] defines

[Lee Jones]

On Mon, 10 Aug 2015, Jassi Brar wrote:

> On Mon, Aug 10, 2015 at 1:54 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Mon, 10 Aug 2015, Jassi Brar wrote:
> >> On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >> > This header is currently only used for defines pertaining to data
> >> > direction i.e. Rx, Tx or Loopback.
> >> >
> >> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> >> > ---
> >> >  include/dt-bindings/mailbox/mailbox.h | 14 ++++++++++++++
> >> >  1 file changed, 14 insertions(+)
> >> >  create mode 100644 include/dt-bindings/mailbox/mailbox.h
> >> >
> >> > diff --git a/include/dt-bindings/mailbox/mailbox.h b/include/dt-bindings/mailbox/mailbox.h
> >> > new file mode 100644
> >> > index 0000000..82e929a
> >> > --- /dev/null
> >> > +++ b/include/dt-bindings/mailbox/mailbox.h
> >> > @@ -0,0 +1,14 @@
> >> > +/*
> >> > + * This program is free software; you can redistribute it and/or modify
> >> > + * it under the terms of the GNU General Public License version 2 as
> >> > + * published by the Free Software Foundation.
> >> > + */
> >> > +
> >> > +#ifndef __MAILBOX_CONTROLLER_DT_BINDINGS_H
> >> > +#define __MAILBOX_CONTROLLER_DT_BINDINGS_H
> >> > +
> >> > +#define MBOX_TX                0x1
> >> > +#define MBOX_RX                0x2
> >> > +#define MBOX_LOOPBACK  (MBOX_RX | MBOX_TX)
> >> > +
> >> Not sure I understand 'loopback'.  Does it mean h/w has some
> >> 'loopback' mode for testing purposes? Or it simply means the
> >> controller can send as well as receive messages?
> >
> > 'loopback' allows firmware to conduct some early function tests.
> > However, channels are simplex, so we provide protection against
> > multiple allocation of single channel.  By allocating a LOOPBACK
> > channel we over-ride this protection and allow a single channel to be
> > allocated twice, once for Rx and the other for Tx.
> >
> So basically hardware is half-duplex, not simplex. I think maybe you
> should simply allow for RX and TX always. It should work. Just
> handover any received data before send_data (reflecting the h/w
> limitation). That way you don't need any such special flag.

Unfortunately no, that's not correct.  Only Mailbox 0 is half-duplex.
The others are simplex (Rx only).  Ideally I'd like to keep the
LOOPBACK flag, as it's easier to figure out if what someone is
attempting to do is actually valid.  It is only ever used for
'loopback' testing anyway, so it's a perfectly reasonable
description of the channel configuration.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 2/6] mailbox: dt-bindings: Add shared [driver <=> device tree] defines

[Jassi Brar]

On Wed, Aug 12, 2015 at 2:23 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Mon, 10 Aug 2015, Jassi Brar wrote:
>
>> On Mon, Aug 10, 2015 at 1:54 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> > On Mon, 10 Aug 2015, Jassi Brar wrote:
>> >> On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> >> > This header is currently only used for defines pertaining to data
>> >> > direction i.e. Rx, Tx or Loopback.
>> >> >
>> >> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
>> >> > ---
>> >> >  include/dt-bindings/mailbox/mailbox.h | 14 ++++++++++++++
>> >> >  1 file changed, 14 insertions(+)
>> >> >  create mode 100644 include/dt-bindings/mailbox/mailbox.h
>> >> >
>> >> > diff --git a/include/dt-bindings/mailbox/mailbox.h b/include/dt-bindings/mailbox/mailbox.h
>> >> > new file mode 100644
>> >> > index 0000000..82e929a
>> >> > --- /dev/null
>> >> > +++ b/include/dt-bindings/mailbox/mailbox.h
>> >> > @@ -0,0 +1,14 @@
>> >> > +/*
>> >> > + * This program is free software; you can redistribute it and/or modify
>> >> > + * it under the terms of the GNU General Public License version 2 as
>> >> > + * published by the Free Software Foundation.
>> >> > + */
>> >> > +
>> >> > +#ifndef __MAILBOX_CONTROLLER_DT_BINDINGS_H
>> >> > +#define __MAILBOX_CONTROLLER_DT_BINDINGS_H
>> >> > +
>> >> > +#define MBOX_TX                0x1
>> >> > +#define MBOX_RX                0x2
>> >> > +#define MBOX_LOOPBACK  (MBOX_RX | MBOX_TX)
>> >> > +
>> >> Not sure I understand 'loopback'.  Does it mean h/w has some
>> >> 'loopback' mode for testing purposes? Or it simply means the
>> >> controller can send as well as receive messages?
>> >
>> > 'loopback' allows firmware to conduct some early function tests.
>> > However, channels are simplex, so we provide protection against
>> > multiple allocation of single channel.  By allocating a LOOPBACK
>> > channel we over-ride this protection and allow a single channel to be
>> > allocated twice, once for Rx and the other for Tx.
>> >
>> So basically hardware is half-duplex, not simplex. I think maybe you
>> should simply allow for RX and TX always. It should work. Just
>> handover any received data before send_data (reflecting the h/w
>> limitation). That way you don't need any such special flag.
>
> Unfortunately no, that's not correct.  Only Mailbox 0 is half-duplex.
> The others are simplex (Rx only).
>
Assuming that is indeed the case (though code and comments suggest
otherwise), it is still not a matter of choice for clients to 'make' a
channel RX or TX or RXTX. That is the property/constraint of the
controller and the controller driver should simply check for channel
ID to be zero in send_data() and return error if its non-zero.

>  Ideally I'd like to keep the
> LOOPBACK flag, as it's easier to figure out if what someone is
> attempting to do is actually valid.
>
I am not for such paranoia. Provider drivers is not the place to check
for valid user data.
The controller driver should simply reject send_data() request on any
mailbox > 0 while the consumer driver should scream for attention
because that's where the problem is.

Please note, what I suggest will only make the code simpler while not
breaking anything.

Thanks.

Re: [PATCH v2 5/6] mailbox: Add generic mechanism for testing Mailbox Controllers

[Lee Jones]

On Mon, 10 Aug 2015, Jassi Brar wrote:

> On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > This particular Client implementation uses shared memory in order
> > to pass messages between Mailbox users; however, it can be easily
> > hacked to support any type of Controller.
> >
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/mailbox/Kconfig        |   7 ++
> >  drivers/mailbox/Makefile       |   2 +
> >  drivers/mailbox/mailbox-test.c | 210 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 219 insertions(+)
> >  create mode 100644 drivers/mailbox/mailbox-test.c
> >
> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> > index 2cc4c39..7720bde 100644
> > --- a/drivers/mailbox/Kconfig
> > +++ b/drivers/mailbox/Kconfig
> > @@ -77,4 +77,11 @@ config STI_MBOX
> >           Mailbox implementation for STMicroelectonics family chips with
> >           hardware for interprocessor communication.
> >
> > +config MAILBOX_TEST
> > +       tristate "Mailbox Test Client"
> > +       depends on OF
> > +       help
> > +         Test client to help with testing new Controller driver
> > +         implementations.
> > +
> >  endif
> > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > index 7cb4766..92435ef 100644
> > --- a/drivers/mailbox/Makefile
> > +++ b/drivers/mailbox/Makefile
> > @@ -2,6 +2,8 @@
> >
> >  obj-$(CONFIG_MAILBOX)          += mailbox.o
> >
> > +obj-$(CONFIG_MAILBOX_TEST)     += mailbox-test.o
> > +
> >  obj-$(CONFIG_ARM_MHU)  += arm_mhu.o
> >
> >  obj-$(CONFIG_PL320_MBOX)       += pl320-ipc.o
> > diff --git a/drivers/mailbox/mailbox-test.c b/drivers/mailbox/mailbox-test.c
> > new file mode 100644
> > index 0000000..10bfe3a
> > --- /dev/null
> > +++ b/drivers/mailbox/mailbox-test.c
> > @@ -0,0 +1,210 @@
> > +/*
> > + * Copyright (C) 2015 ST Microelectronics
> > + *
> > + * Author: Lee Jones <lee.jones@linaro.org>
> > + *
> > + * 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; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#include <linux/debugfs.h>
> > +#include <linux/err.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mailbox_client.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/uaccess.h>
> > +
> > +#define MBOX_MAX_MSG_LEN 128
> > +
> > +static struct dentry *root_debugfs_dir;
> > +
> > +struct mbox_test_device {
> > +       struct device           *dev;
> > +       struct mbox_chan        *tx_channel;
> > +       struct mbox_chan        *rx_channel;
> > +       void __iomem            *mmio;
> > +
> > +};
> > +
> > +static ssize_t mbox_test_write(struct file *filp,
> > +                              const char __user *userbuf,
> > +                              size_t count, loff_t *ppos)
> > +{
> > +       struct mbox_test_device *tdev = filp->private_data;
> > +       char *message;
> > +       int ret;
> > +
> > +       if (count > MBOX_MAX_MSG_LEN) {
> > +               dev_err(tdev->dev,
> > +                       "Message length %d greater than max allowed %d\n",
> > +                       count, MBOX_MAX_MSG_LEN);
> > +               return -EINVAL;
> > +       }
> > +
> > +       message = kzalloc(count, GFP_KERNEL);
> > +       if (!message)
> > +               return -ENOMEM;
> > +
> > +       ret = copy_from_user(message, userbuf, count);
> > +       if (ret)
> > +               return -EFAULT;
> > +
> > +       print_hex_dump(KERN_ERR, "Client: Sending: ", DUMP_PREFIX_ADDRESS,
> > +                      16, 1, message, 16, true);
> > +
> > +       ret = mbox_send_message(tdev->tx_channel, message);
> > +       if (ret < 0)
> > +               dev_err(tdev->dev, "Failed to send message via mailbox\n");
> > +
> > +       kfree(message);
> > +
> > +       return ret < 0 ? ret : count;
> > +}
> > +
> > +static const struct file_operations mbox_test_ops = {
> > +       .write  = mbox_test_write,
> > +       .open   = simple_open,
> > +       .llseek = generic_file_llseek,
> > +};
> > +
> > +static int mbox_test_add_debugfs(struct platform_device *pdev,
> > +                                struct mbox_test_device *tdev)
> > +{
> > +       if (!debugfs_initialized())
> > +               return 0;
> > +
> > +       root_debugfs_dir = debugfs_create_dir("mailbox", NULL);
> > +       if (!root_debugfs_dir) {
> > +               dev_err(&pdev->dev, "Failed to create Mailbox debugfs\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       debugfs_create_file("send-message", 0200, root_debugfs_dir,
> > +                           tdev, &mbox_test_ops);
> > +
> > +       return 0;
> > +}
> > +
> > +static void mbox_test_receive_message(struct mbox_client *client, void *message)
> > +{
> > +       struct mbox_test_device *tdev = dev_get_drvdata(client->dev);
> > +
> > +       if (!tdev->mmio) {
> > +               dev_info(tdev->dev, "Client: Recived something [read mmio]\n");
> > +               return;
> > +       }
> > +
> > +       print_hex_dump(KERN_ERR, "Client: from co-proc: ", DUMP_PREFIX_ADDRESS,
> > +                      16, 1, tdev->mmio, MBOX_MAX_MSG_LEN, true);
> > +}
> > +
> > +static void mbox_test_prepare_message(struct mbox_client *client, void *message)
> > +{
> > +       struct mbox_test_device *tdev = dev_get_drvdata(client->dev);
> > +
> > +       if (tdev->mmio)
> > +               memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
> >
> This is unlikely to work. Those platforms that need to send a 2 part
> message, they do :
> (a) Signal/Command/Target code via some controller register (via
> mbox_send_message).
> (b) Setup the payload in Shared-Memory (via tx_prepare).
> 
> This test driver assumes both are the same. I think you have to
> declare something like

This driver assumes that the framework will call client->tx_prepare()
first, which satisfies (b).  It then assumes controller->send_data()
will be invoked, which will send the platform specific
signal/command/target code, which then satisfies (a).

In what way does it assume they are the same?

> struct mbox_test_message { /* same for TX and RX */
>           unsigned sig_len;
>           void *signal;               /* rx/tx via mailbox api */
>           unsigned pl_len;
>           void *payload;           /* rx/tx via shared-memory */
> };

How do you think this should be set and use these?

> > +
> > +static void mbox_test_message_sent(struct mbox_client *client,
> > +                                  void *message, int r)
> > +{
> > +       if (r)
> > +               dev_warn(client->dev,
> > +                        "Client: Message could not be sent: %d\n", r);
> > +       else
> > +               dev_info(client->dev,
> > +                        "Client: Message sent\n");
> > +}
> > +
> > +static struct mbox_chan *
> > +mbox_test_request_channel(struct platform_device *pdev, const char *name)
> > +{
> > +       struct mbox_client *client;
> > +
> > +       client = devm_kzalloc(&pdev->dev, sizeof(*client), GFP_KERNEL);
> > +       if (!client)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       client->dev             = &pdev->dev;
> > +       client->rx_callback     = mbox_test_receive_message;
> > +       client->tx_prepare      = mbox_test_prepare_message;
> > +       client->tx_done         = mbox_test_message_sent;
> > +       client->tx_block        = true;
> > +       client->knows_txdone    = false;
> > +       client->tx_tout         = 500;
> > +
> > +       return mbox_request_channel_byname(client, name);
> > +}
> > +
> > +static int mbox_test_probe(struct platform_device *pdev)
> > +{
> > +       struct mbox_test_device *tdev;
> > +       struct resource *res;
> > +       int ret;
> > +
> > +       tdev = devm_kzalloc(&pdev->dev, sizeof(*tdev), GFP_KERNEL);
> > +       if (!tdev)
> > +               return -ENOMEM;
> > +
> > +       /* It's okay for MMIO to be NULL */
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       tdev->mmio = devm_ioremap_resource(&pdev->dev, res);
> > +       if (IS_ERR(tdev->mmio))
> > +               tdev->mmio = NULL;
> > +
> > +       tdev->tx_channel = mbox_test_request_channel(pdev, "tx");
> > +       if (IS_ERR(tdev->tx_channel))
> > +               return PTR_ERR(tdev->tx_channel);
> > +
> > +       tdev->rx_channel = mbox_test_request_channel(pdev, "rx");
> > +       if (IS_ERR(tdev->rx_channel))
> > +               return PTR_ERR(tdev->rx_channel);
> > +
> Should it really fail on TX or RX only clients?

Good question.  Probably not, but I guess we'd need a flag for that.

> It takes write from userspace but shouldn't it also provide data
> received to the userspace?

Currently we only print the returning message.  If you want me to put
it in a userspace file too, that's not an issue.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 2/6] mailbox: dt-bindings: Add shared [driver <=> device tree] defines

[Lee Jones]

On Wed, 12 Aug 2015, Jassi Brar wrote:

> On Wed, Aug 12, 2015 at 2:23 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Mon, 10 Aug 2015, Jassi Brar wrote:
> >
> >> On Mon, Aug 10, 2015 at 1:54 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >> > On Mon, 10 Aug 2015, Jassi Brar wrote:
> >> >> On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >> >> > This header is currently only used for defines pertaining to data
> >> >> > direction i.e. Rx, Tx or Loopback.
> >> >> >
> >> >> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> >> >> > ---
> >> >> >  include/dt-bindings/mailbox/mailbox.h | 14 ++++++++++++++
> >> >> >  1 file changed, 14 insertions(+)
> >> >> >  create mode 100644 include/dt-bindings/mailbox/mailbox.h
> >> >> >
> >> >> > diff --git a/include/dt-bindings/mailbox/mailbox.h b/include/dt-bindings/mailbox/mailbox.h
> >> >> > new file mode 100644
> >> >> > index 0000000..82e929a
> >> >> > --- /dev/null
> >> >> > +++ b/include/dt-bindings/mailbox/mailbox.h
> >> >> > @@ -0,0 +1,14 @@
> >> >> > +/*
> >> >> > + * This program is free software; you can redistribute it and/or modify
> >> >> > + * it under the terms of the GNU General Public License version 2 as
> >> >> > + * published by the Free Software Foundation.
> >> >> > + */
> >> >> > +
> >> >> > +#ifndef __MAILBOX_CONTROLLER_DT_BINDINGS_H
> >> >> > +#define __MAILBOX_CONTROLLER_DT_BINDINGS_H
> >> >> > +
> >> >> > +#define MBOX_TX                0x1
> >> >> > +#define MBOX_RX                0x2
> >> >> > +#define MBOX_LOOPBACK  (MBOX_RX | MBOX_TX)
> >> >> > +
> >> >> Not sure I understand 'loopback'.  Does it mean h/w has some
> >> >> 'loopback' mode for testing purposes? Or it simply means the
> >> >> controller can send as well as receive messages?
> >> >
> >> > 'loopback' allows firmware to conduct some early function tests.
> >> > However, channels are simplex, so we provide protection against
> >> > multiple allocation of single channel.  By allocating a LOOPBACK
> >> > channel we over-ride this protection and allow a single channel to be
> >> > allocated twice, once for Rx and the other for Tx.
> >> >
> >> So basically hardware is half-duplex, not simplex. I think maybe you
> >> should simply allow for RX and TX always. It should work. Just
> >> handover any received data before send_data (reflecting the h/w
> >> limitation). That way you don't need any such special flag.
> >
> > Unfortunately no, that's not correct.  Only Mailbox 0 is half-duplex.
> > The others are simplex (Rx only).
> >
> Assuming that is indeed the case (though code and comments suggest
> otherwise),

Sorry, that should have read "Tx only".  Only the A9 (Mbox 0) can Rx.

> it is still not a matter of choice for clients to 'make' a
> channel RX or TX or RXTX. That is the property/constraint of the
> controller and the controller driver should simply check for channel
> ID to be zero in send_data() and return error if its non-zero.

The client is not 'making' the channel Rx or Tx.  It's requesting a
specific Rx or Tx channel.  It's the controller's responsibility to
tell the client if the configuration it has requested is invalid or
already in use.

You're suggesting I remove configuration checking during
mbox_request_channel(), which is not a good suggestion.  That will
fool the client into thinking it has successfully obtained a channel
with the configuration it desires, only to fail later when it attempts
to actually send a message.  Either that, or it will be waiting
forever for a message that it will never receive because it has
selected a Tx only channel to receive from.

That doesn't sound reasonable to me.

> >  Ideally I'd like to keep the
> > LOOPBACK flag, as it's easier to figure out if what someone is
> > attempting to do is actually valid.
> >
> I am not for such paranoia. Provider drivers is not the place to check
> for valid user data.

No one is checking data here.  We are checking channel confirmation,
which is absolutely a controller's responsibility.

> The controller driver should simply reject send_data() request on any
> mailbox > 0 while the consumer driver should scream for attention
> because that's where the problem is.

That would make debugging very difficult, due to the fact that we've
already insinuated that the client had successfully received the
channel/configuration it desired, when in actual fact, the provider of
that channel would know full well that it was actually invalid.

> Please note, what I suggest will only make the code simpler while not
> breaking anything.

Simpler yes.  We could remove all error checking and the driver would
be simpler still, but the fact remains that it would not be doing its
job and debugging would become neigh impossible.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 5/6] mailbox: Add generic mechanism for testing Mailbox Controllers

[Jassi Brar]

On Wed, Aug 12, 2015 at 3:53 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Mon, 10 Aug 2015, Jassi Brar wrote:
>
>> On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> > This particular Client implementation uses shared memory in order
>> > to pass messages between Mailbox users; however, it can be easily
>> > hacked to support any type of Controller.
>> >
>> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
>> > ---
>> >  drivers/mailbox/Kconfig        |   7 ++
>> >  drivers/mailbox/Makefile       |   2 +
>> >  drivers/mailbox/mailbox-test.c | 210 +++++++++++++++++++++++++++++++++++++++++
>> >  3 files changed, 219 insertions(+)
>> >  create mode 100644 drivers/mailbox/mailbox-test.c
>> >
>> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
>> > index 2cc4c39..7720bde 100644
>> > --- a/drivers/mailbox/Kconfig
>> > +++ b/drivers/mailbox/Kconfig
>> > @@ -77,4 +77,11 @@ config STI_MBOX
>> >           Mailbox implementation for STMicroelectonics family chips with
>> >           hardware for interprocessor communication.
>> >
>> > +config MAILBOX_TEST
>> > +       tristate "Mailbox Test Client"
>> > +       depends on OF
>> > +       help
>> > +         Test client to help with testing new Controller driver
>> > +         implementations.
>> > +
>> >  endif
>> > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
>> > index 7cb4766..92435ef 100644
>> > --- a/drivers/mailbox/Makefile
>> > +++ b/drivers/mailbox/Makefile
>> > @@ -2,6 +2,8 @@
>> >
>> >  obj-$(CONFIG_MAILBOX)          += mailbox.o
>> >
>> > +obj-$(CONFIG_MAILBOX_TEST)     += mailbox-test.o
>> > +
>> >  obj-$(CONFIG_ARM_MHU)  += arm_mhu.o
>> >
>> >  obj-$(CONFIG_PL320_MBOX)       += pl320-ipc.o
>> > diff --git a/drivers/mailbox/mailbox-test.c b/drivers/mailbox/mailbox-test.c
>> > new file mode 100644
>> > index 0000000..10bfe3a
>> > --- /dev/null
>> > +++ b/drivers/mailbox/mailbox-test.c
>> > @@ -0,0 +1,210 @@
>> > +/*
>> > + * Copyright (C) 2015 ST Microelectronics
>> > + *
>> > + * Author: Lee Jones <lee.jones@linaro.org>
>> > + *
>> > + * 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; either version 2 of the License, or
>> > + * (at your option) any later version.
>> > + */
>> > +
>> > +#include <linux/debugfs.h>
>> > +#include <linux/err.h>
>> > +#include <linux/kernel.h>
>> > +#include <linux/mailbox_client.h>
>> > +#include <linux/module.h>
>> > +#include <linux/of.h>
>> > +#include <linux/platform_device.h>
>> > +#include <linux/slab.h>
>> > +#include <linux/uaccess.h>
>> > +
>> > +#define MBOX_MAX_MSG_LEN 128
>> > +
>> > +static struct dentry *root_debugfs_dir;
>> > +
>> > +struct mbox_test_device {
>> > +       struct device           *dev;
>> > +       struct mbox_chan        *tx_channel;
>> > +       struct mbox_chan        *rx_channel;
>> > +       void __iomem            *mmio;
>> > +
>> > +};
>> > +
>> > +static ssize_t mbox_test_write(struct file *filp,
>> > +                              const char __user *userbuf,
>> > +                              size_t count, loff_t *ppos)
>> > +{
>> > +       struct mbox_test_device *tdev = filp->private_data;
>> > +       char *message;
>> > +       int ret;
>> > +
>> > +       if (count > MBOX_MAX_MSG_LEN) {
>> > +               dev_err(tdev->dev,
>> > +                       "Message length %d greater than max allowed %d\n",
>> > +                       count, MBOX_MAX_MSG_LEN);
>> > +               return -EINVAL;
>> > +       }
>> > +
>> > +       message = kzalloc(count, GFP_KERNEL);
>> > +       if (!message)
>> > +               return -ENOMEM;
>> > +
>> > +       ret = copy_from_user(message, userbuf, count);
>> > +       if (ret)
>> > +               return -EFAULT;
>> > +
>> > +       print_hex_dump(KERN_ERR, "Client: Sending: ", DUMP_PREFIX_ADDRESS,
>> > +                      16, 1, message, 16, true);
>> > +
>> > +       ret = mbox_send_message(tdev->tx_channel, message);
>> > +       if (ret < 0)
>> > +               dev_err(tdev->dev, "Failed to send message via mailbox\n");
>> > +
>> > +       kfree(message);
>> > +
>> > +       return ret < 0 ? ret : count;
>> > +}
>> > +
>> > +static const struct file_operations mbox_test_ops = {
>> > +       .write  = mbox_test_write,
>> > +       .open   = simple_open,
>> > +       .llseek = generic_file_llseek,
>> > +};
>> > +
>> > +static int mbox_test_add_debugfs(struct platform_device *pdev,
>> > +                                struct mbox_test_device *tdev)
>> > +{
>> > +       if (!debugfs_initialized())
>> > +               return 0;
>> > +
>> > +       root_debugfs_dir = debugfs_create_dir("mailbox", NULL);
>> > +       if (!root_debugfs_dir) {
>> > +               dev_err(&pdev->dev, "Failed to create Mailbox debugfs\n");
>> > +               return -EINVAL;
>> > +       }
>> > +
>> > +       debugfs_create_file("send-message", 0200, root_debugfs_dir,
>> > +                           tdev, &mbox_test_ops);
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static void mbox_test_receive_message(struct mbox_client *client, void *message)
>> > +{
>> > +       struct mbox_test_device *tdev = dev_get_drvdata(client->dev);
>> > +
>> > +       if (!tdev->mmio) {
>> > +               dev_info(tdev->dev, "Client: Recived something [read mmio]\n");
>> > +               return;
>> > +       }
>> > +
>> > +       print_hex_dump(KERN_ERR, "Client: from co-proc: ", DUMP_PREFIX_ADDRESS,
>> > +                      16, 1, tdev->mmio, MBOX_MAX_MSG_LEN, true);
>> > +}
>> > +
>> > +static void mbox_test_prepare_message(struct mbox_client *client, void *message)
>> > +{
>> > +       struct mbox_test_device *tdev = dev_get_drvdata(client->dev);
>> > +
>> > +       if (tdev->mmio)
>> > +               memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
>> >
>> This is unlikely to work. Those platforms that need to send a 2 part
>> message, they do :
>> (a) Signal/Command/Target code via some controller register (via
>> mbox_send_message).
>> (b) Setup the payload in Shared-Memory (via tx_prepare).
>>
>> This test driver assumes both are the same. I think you have to
>> declare something like
>
> This driver assumes that the framework will call client->tx_prepare()
> first, which satisfies (b).  It then assumes controller->send_data()
> will be invoked, which will send the platform specific
> signal/command/target code, which then satisfies (a).
>
Yeah, but what would be that code? Who provides that?

> In what way does it assume they are the same?
>
notice the 'message' pointer in
mbox_send_message(tdev->tx_channel, message);
    and
memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);

>> struct mbox_test_message { /* same for TX and RX */
>>           unsigned sig_len;
>>           void *signal;               /* rx/tx via mailbox api */
>>           unsigned pl_len;
>>           void *payload;           /* rx/tx via shared-memory */
>> };
>
> How do you think this should be set and use these?
>
The userspace would want to specify the command code (32bits or not)
that would be passed via the fifo/register of the controller. So we
need signal[]

The data to be passed via share-memory could be provided by userspace
via the payload[] array.


>> > +static void mbox_test_message_sent(struct mbox_client *client,
>> > +                                  void *message, int r)
>> > +{
>> > +       if (r)
>> > +               dev_warn(client->dev,
>> > +                        "Client: Message could not be sent: %d\n", r);
>> > +       else
>> > +               dev_info(client->dev,
>> > +                        "Client: Message sent\n");
>> > +}
>> > +
>> > +static struct mbox_chan *
>> > +mbox_test_request_channel(struct platform_device *pdev, const char *name)
>> > +{
>> > +       struct mbox_client *client;
>> > +
>> > +       client = devm_kzalloc(&pdev->dev, sizeof(*client), GFP_KERNEL);
>> > +       if (!client)
>> > +               return ERR_PTR(-ENOMEM);
>> > +
>> > +       client->dev             = &pdev->dev;
>> > +       client->rx_callback     = mbox_test_receive_message;
>> > +       client->tx_prepare      = mbox_test_prepare_message;
>> > +       client->tx_done         = mbox_test_message_sent;
>> > +       client->tx_block        = true;
>> > +       client->knows_txdone    = false;
>> > +       client->tx_tout         = 500;
>> > +
>> > +       return mbox_request_channel_byname(client, name);
>> > +}
>> > +
>> > +static int mbox_test_probe(struct platform_device *pdev)
>> > +{
>> > +       struct mbox_test_device *tdev;
>> > +       struct resource *res;
>> > +       int ret;
>> > +
>> > +       tdev = devm_kzalloc(&pdev->dev, sizeof(*tdev), GFP_KERNEL);
>> > +       if (!tdev)
>> > +               return -ENOMEM;
>> > +
>> > +       /* It's okay for MMIO to be NULL */
>> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> > +       tdev->mmio = devm_ioremap_resource(&pdev->dev, res);
>> > +       if (IS_ERR(tdev->mmio))
>> > +               tdev->mmio = NULL;
>> > +
>> > +       tdev->tx_channel = mbox_test_request_channel(pdev, "tx");
>> > +       if (IS_ERR(tdev->tx_channel))
>> > +               return PTR_ERR(tdev->tx_channel);
>> > +
>> > +       tdev->rx_channel = mbox_test_request_channel(pdev, "rx");
>> > +       if (IS_ERR(tdev->rx_channel))
>> > +               return PTR_ERR(tdev->rx_channel);
>> > +
>> Should it really fail on TX or RX only clients?
>
> Good question.  Probably not, but I guess we'd need a flag for that.
>
Just assume 1 channel. You can't make it receive if the controller
can't do rx, so the userspace will simply block on read requests. And
you can't make the controller send, if it can't. So any write attempt
will simply return with an error.
 And we are going to need very platform specific knowledge to execute
the tests. It's not like some loop enumerating all possible
combinations of parameters that the api must pass... so we are good.

>> It takes write from userspace but shouldn't it also provide data
>> received to the userspace?
>
> Currently we only print the returning message.  If you want me to put
> it in a userspace file too, that's not an issue.
>
Ideally we should support 'read' just like write.

thanks.

Re: [PATCH v2 5/6] mailbox: Add generic mechanism for testing Mailbox Controllers

[Lee Jones]

On Thu, 13 Aug 2015, Jassi Brar wrote:

> On Wed, Aug 12, 2015 at 3:53 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Mon, 10 Aug 2015, Jassi Brar wrote:
> >
> >> On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >> > This particular Client implementation uses shared memory in order
> >> > to pass messages between Mailbox users; however, it can be easily
> >> > hacked to support any type of Controller.
> >> >
> >> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> >> > ---
> >> >  drivers/mailbox/Kconfig        |   7 ++
> >> >  drivers/mailbox/Makefile       |   2 +
> >> >  drivers/mailbox/mailbox-test.c | 210 +++++++++++++++++++++++++++++++++++++++++
> >> >  3 files changed, 219 insertions(+)
> >> >  create mode 100644 drivers/mailbox/mailbox-test.c
> >> >
> >> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> >> > index 2cc4c39..7720bde 100644
> >> > --- a/drivers/mailbox/Kconfig
> >> > +++ b/drivers/mailbox/Kconfig
> >> > @@ -77,4 +77,11 @@ config STI_MBOX
> >> >           Mailbox implementation for STMicroelectonics family chips with
> >> >           hardware for interprocessor communication.
> >> >
> >> > +config MAILBOX_TEST
> >> > +       tristate "Mailbox Test Client"
> >> > +       depends on OF
> >> > +       help
> >> > +         Test client to help with testing new Controller driver
> >> > +         implementations.
> >> > +
> >> >  endif
> >> > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> >> > index 7cb4766..92435ef 100644
> >> > --- a/drivers/mailbox/Makefile
> >> > +++ b/drivers/mailbox/Makefile
> >> > @@ -2,6 +2,8 @@
> >> >
> >> >  obj-$(CONFIG_MAILBOX)          += mailbox.o
> >> >
> >> > +obj-$(CONFIG_MAILBOX_TEST)     += mailbox-test.o
> >> > +
> >> >  obj-$(CONFIG_ARM_MHU)  += arm_mhu.o
> >> >
> >> >  obj-$(CONFIG_PL320_MBOX)       += pl320-ipc.o
> >> > diff --git a/drivers/mailbox/mailbox-test.c b/drivers/mailbox/mailbox-test.c
> >> > new file mode 100644
> >> > index 0000000..10bfe3a
> >> > --- /dev/null
> >> > +++ b/drivers/mailbox/mailbox-test.c
> >> > @@ -0,0 +1,210 @@
> >> > +/*
> >> > + * Copyright (C) 2015 ST Microelectronics
> >> > + *
> >> > + * Author: Lee Jones <lee.jones@linaro.org>
> >> > + *
> >> > + * 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; either version 2 of the License, or
> >> > + * (at your option) any later version.
> >> > + */
> >> > +
> >> > +#include <linux/debugfs.h>
> >> > +#include <linux/err.h>
> >> > +#include <linux/kernel.h>
> >> > +#include <linux/mailbox_client.h>
> >> > +#include <linux/module.h>
> >> > +#include <linux/of.h>
> >> > +#include <linux/platform_device.h>
> >> > +#include <linux/slab.h>
> >> > +#include <linux/uaccess.h>
> >> > +
> >> > +#define MBOX_MAX_MSG_LEN 128
> >> > +
> >> > +static struct dentry *root_debugfs_dir;
> >> > +
> >> > +struct mbox_test_device {
> >> > +       struct device           *dev;
> >> > +       struct mbox_chan        *tx_channel;
> >> > +       struct mbox_chan        *rx_channel;
> >> > +       void __iomem            *mmio;
> >> > +
> >> > +};
> >> > +
> >> > +static ssize_t mbox_test_write(struct file *filp,
> >> > +                              const char __user *userbuf,
> >> > +                              size_t count, loff_t *ppos)
> >> > +{
> >> > +       struct mbox_test_device *tdev = filp->private_data;
> >> > +       char *message;
> >> > +       int ret;
> >> > +
> >> > +       if (count > MBOX_MAX_MSG_LEN) {
> >> > +               dev_err(tdev->dev,
> >> > +                       "Message length %d greater than max allowed %d\n",
> >> > +                       count, MBOX_MAX_MSG_LEN);
> >> > +               return -EINVAL;
> >> > +       }
> >> > +
> >> > +       message = kzalloc(count, GFP_KERNEL);
> >> > +       if (!message)
> >> > +               return -ENOMEM;
> >> > +
> >> > +       ret = copy_from_user(message, userbuf, count);
> >> > +       if (ret)
> >> > +               return -EFAULT;
> >> > +
> >> > +       print_hex_dump(KERN_ERR, "Client: Sending: ", DUMP_PREFIX_ADDRESS,
> >> > +                      16, 1, message, 16, true);
> >> > +
> >> > +       ret = mbox_send_message(tdev->tx_channel, message);
> >> > +       if (ret < 0)
> >> > +               dev_err(tdev->dev, "Failed to send message via mailbox\n");
> >> > +
> >> > +       kfree(message);
> >> > +
> >> > +       return ret < 0 ? ret : count;
> >> > +}
> >> > +
> >> > +static const struct file_operations mbox_test_ops = {
> >> > +       .write  = mbox_test_write,
> >> > +       .open   = simple_open,
> >> > +       .llseek = generic_file_llseek,
> >> > +};
> >> > +
> >> > +static int mbox_test_add_debugfs(struct platform_device *pdev,
> >> > +                                struct mbox_test_device *tdev)
> >> > +{
> >> > +       if (!debugfs_initialized())
> >> > +               return 0;
> >> > +
> >> > +       root_debugfs_dir = debugfs_create_dir("mailbox", NULL);
> >> > +       if (!root_debugfs_dir) {
> >> > +               dev_err(&pdev->dev, "Failed to create Mailbox debugfs\n");
> >> > +               return -EINVAL;
> >> > +       }
> >> > +
> >> > +       debugfs_create_file("send-message", 0200, root_debugfs_dir,
> >> > +                           tdev, &mbox_test_ops);
> >> > +
> >> > +       return 0;
> >> > +}
> >> > +
> >> > +static void mbox_test_receive_message(struct mbox_client *client, void *message)
> >> > +{
> >> > +       struct mbox_test_device *tdev = dev_get_drvdata(client->dev);
> >> > +
> >> > +       if (!tdev->mmio) {
> >> > +               dev_info(tdev->dev, "Client: Recived something [read mmio]\n");
> >> > +               return;
> >> > +       }
> >> > +
> >> > +       print_hex_dump(KERN_ERR, "Client: from co-proc: ", DUMP_PREFIX_ADDRESS,
> >> > +                      16, 1, tdev->mmio, MBOX_MAX_MSG_LEN, true);
> >> > +}
> >> > +
> >> > +static void mbox_test_prepare_message(struct mbox_client *client, void *message)
> >> > +{
> >> > +       struct mbox_test_device *tdev = dev_get_drvdata(client->dev);
> >> > +
> >> > +       if (tdev->mmio)
> >> > +               memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
> >> >
> >> This is unlikely to work. Those platforms that need to send a 2 part
> >> message, they do :
> >> (a) Signal/Command/Target code via some controller register (via
> >> mbox_send_message).
> >> (b) Setup the payload in Shared-Memory (via tx_prepare).
> >>
> >> This test driver assumes both are the same. I think you have to
> >> declare something like
> >
> > This driver assumes that the framework will call client->tx_prepare()
> > first, which satisfies (b).  It then assumes controller->send_data()
> > will be invoked, which will send the platform specific
> > signal/command/target code, which then satisfies (a).
> >
> Yeah, but what would be that code? Who provides that?
> 
> > In what way does it assume they are the same?
> >
> notice the 'message' pointer in
> mbox_send_message(tdev->tx_channel, message);
>     and
> memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
> 
> >> struct mbox_test_message { /* same for TX and RX */
> >>           unsigned sig_len;
> >>           void *signal;               /* rx/tx via mailbox api */
> >>           unsigned pl_len;
> >>           void *payload;           /* rx/tx via shared-memory */
> >> };
> >
> > How do you think this should be set and use these?
> >
> The userspace would want to specify the command code (32bits or not)
> that would be passed via the fifo/register of the controller. So we
> need signal[]
> 
> The data to be passed via share-memory could be provided by userspace
> via the payload[] array.

Okay, so would the solution be two userspace files 'signal' and
'message', so in the case of a client specified signal we can write it
into there first.

echo 255  > signal
echo test > message

... or in the case where no signal is required, or the controller
driver taking care of it, we just don't write anything to signal?

Just for clarification, in the case where a signal is required, do
clients usually pass that through mbox_send_message() too?

  mbox_send_message(tdev->tx_channel, signal);
  mbox_send_message(tdev->tx_channel, message);

... or vice versa.

> >> > +static void mbox_test_message_sent(struct mbox_client *client,
> >> > +                                  void *message, int r)
> >> > +{
> >> > +       if (r)
> >> > +               dev_warn(client->dev,
> >> > +                        "Client: Message could not be sent: %d\n", r);
> >> > +       else
> >> > +               dev_info(client->dev,
> >> > +                        "Client: Message sent\n");
> >> > +}
> >> > +
> >> > +static struct mbox_chan *
> >> > +mbox_test_request_channel(struct platform_device *pdev, const char *name)
> >> > +{
> >> > +       struct mbox_client *client;
> >> > +
> >> > +       client = devm_kzalloc(&pdev->dev, sizeof(*client), GFP_KERNEL);
> >> > +       if (!client)
> >> > +               return ERR_PTR(-ENOMEM);
> >> > +
> >> > +       client->dev             = &pdev->dev;
> >> > +       client->rx_callback     = mbox_test_receive_message;
> >> > +       client->tx_prepare      = mbox_test_prepare_message;
> >> > +       client->tx_done         = mbox_test_message_sent;
> >> > +       client->tx_block        = true;
> >> > +       client->knows_txdone    = false;
> >> > +       client->tx_tout         = 500;
> >> > +
> >> > +       return mbox_request_channel_byname(client, name);
> >> > +}
> >> > +
> >> > +static int mbox_test_probe(struct platform_device *pdev)
> >> > +{
> >> > +       struct mbox_test_device *tdev;
> >> > +       struct resource *res;
> >> > +       int ret;
> >> > +
> >> > +       tdev = devm_kzalloc(&pdev->dev, sizeof(*tdev), GFP_KERNEL);
> >> > +       if (!tdev)
> >> > +               return -ENOMEM;
> >> > +
> >> > +       /* It's okay for MMIO to be NULL */
> >> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> > +       tdev->mmio = devm_ioremap_resource(&pdev->dev, res);
> >> > +       if (IS_ERR(tdev->mmio))
> >> > +               tdev->mmio = NULL;
> >> > +
> >> > +       tdev->tx_channel = mbox_test_request_channel(pdev, "tx");
> >> > +       if (IS_ERR(tdev->tx_channel))
> >> > +               return PTR_ERR(tdev->tx_channel);
> >> > +
> >> > +       tdev->rx_channel = mbox_test_request_channel(pdev, "rx");
> >> > +       if (IS_ERR(tdev->rx_channel))
> >> > +               return PTR_ERR(tdev->rx_channel);
> >> > +
> >> Should it really fail on TX or RX only clients?
> >
> > Good question.  Probably not, but I guess we'd need a flag for that.
> >
> Just assume 1 channel. You can't make it receive if the controller
> can't do rx, so the userspace will simply block on read requests. And
> you can't make the controller send, if it can't. So any write attempt
> will simply return with an error.
>  And we are going to need very platform specific knowledge to execute
> the tests. It's not like some loop enumerating all possible
> combinations of parameters that the api must pass... so we are good.

I'm guessing most tests will be some kind of send-reply test, so we'd
need two channels for that.  I can make it so the driver does not fail
if 'tx' or 'rx' is missing, but will fail if both are.

Bare in mind that this test driver isn't designed to cover all of the
corner cases, but more designed as a helpful boiler plate where it
will tick some use-case boxes, but is clear enough to be hacked around
by other vendors who have different requirements.

With regards to you comments on userspace; I suggest that whoever is
testing the controller will know it and will not attempt to check for
a reply from a controller (or co-proc) which is write-only.

> >> It takes write from userspace but shouldn't it also provide data
> >> received to the userspace?
> >
> > Currently we only print the returning message.  If you want me to put
> > it in a userspace file too, that's not an issue.
> >
> Ideally we should support 'read' just like write.

Sure, I can do that.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 5/6] mailbox: Add generic mechanism for testing Mailbox Controllers

[Jassi Brar]

On Thu, Aug 13, 2015 at 2:49 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Thu, 13 Aug 2015, Jassi Brar wrote:

>> >> > +
>> >> > +static void mbox_test_prepare_message(struct mbox_client *client, void *message)
>> >> > +{
>> >> > +       struct mbox_test_device *tdev = dev_get_drvdata(client->dev);
>> >> > +
>> >> > +       if (tdev->mmio)
>> >> > +               memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
>> >> >
>> >> This is unlikely to work. Those platforms that need to send a 2 part
>> >> message, they do :
>> >> (a) Signal/Command/Target code via some controller register (via
>> >> mbox_send_message).
>> >> (b) Setup the payload in Shared-Memory (via tx_prepare).
>> >>
>> >> This test driver assumes both are the same. I think you have to
>> >> declare something like
>> >
>> > This driver assumes that the framework will call client->tx_prepare()
>> > first, which satisfies (b).  It then assumes controller->send_data()
>> > will be invoked, which will send the platform specific
>> > signal/command/target code, which then satisfies (a).
>> >
>> Yeah, but what would be that code? Who provides that?
>>
>> > In what way does it assume they are the same?
>> >
>> notice the 'message' pointer in
>> mbox_send_message(tdev->tx_channel, message);
>>     and
>> memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
>>
>> >> struct mbox_test_message { /* same for TX and RX */
>> >>           unsigned sig_len;
>> >>           void *signal;               /* rx/tx via mailbox api */
>> >>           unsigned pl_len;
>> >>           void *payload;           /* rx/tx via shared-memory */
>> >> };
>> >
>> > How do you think this should be set and use these?
>> >
>> The userspace would want to specify the command code (32bits or not)
>> that would be passed via the fifo/register of the controller. So we
>> need signal[]
>>
>> The data to be passed via share-memory could be provided by userspace
>> via the payload[] array.
>
> Okay, so would the solution be two userspace files 'signal' and
> 'message', so in the case of a client specified signal we can write it
> into there first.
>
> echo 255  > signal
> echo test > message
>
> ... or in the case where no signal is required, or the controller
> driver taking care of it, we just don't write anything to signal?
>
file_operations.write() should parse signal and message, coming from
userspace, into a local structure before routing them via
mbox_send_message and tx_prepare respectively.

> Just for clarification, in the case where a signal is required, do
> clients usually pass that through mbox_send_message() too?
>
>   mbox_send_message(tdev->tx_channel, signal);
>   mbox_send_message(tdev->tx_channel, message);
>
No. Command/Signal code is passed via mbox_send_message(signal). The
payload is passed via Shared-Memory during tx_prepare(message)


>> >> > +static void mbox_test_message_sent(struct mbox_client *client,
>> >> > +                                  void *message, int r)
>> >> > +{
>> >> > +       if (r)
>> >> > +               dev_warn(client->dev,
>> >> > +                        "Client: Message could not be sent: %d\n", r);
>> >> > +       else
>> >> > +               dev_info(client->dev,
>> >> > +                        "Client: Message sent\n");
>> >> > +}
>> >> > +
>> >> > +static struct mbox_chan *
>> >> > +mbox_test_request_channel(struct platform_device *pdev, const char *name)
>> >> > +{
>> >> > +       struct mbox_client *client;
>> >> > +
>> >> > +       client = devm_kzalloc(&pdev->dev, sizeof(*client), GFP_KERNEL);
>> >> > +       if (!client)
>> >> > +               return ERR_PTR(-ENOMEM);
>> >> > +
>> >> > +       client->dev             = &pdev->dev;
>> >> > +       client->rx_callback     = mbox_test_receive_message;
>> >> > +       client->tx_prepare      = mbox_test_prepare_message;
>> >> > +       client->tx_done         = mbox_test_message_sent;
>> >> > +       client->tx_block        = true;
>> >> > +       client->knows_txdone    = false;
>> >> > +       client->tx_tout         = 500;
>> >> > +
>> >> > +       return mbox_request_channel_byname(client, name);
>> >> > +}
>> >> > +
>> >> > +static int mbox_test_probe(struct platform_device *pdev)
>> >> > +{
>> >> > +       struct mbox_test_device *tdev;
>> >> > +       struct resource *res;
>> >> > +       int ret;
>> >> > +
>> >> > +       tdev = devm_kzalloc(&pdev->dev, sizeof(*tdev), GFP_KERNEL);
>> >> > +       if (!tdev)
>> >> > +               return -ENOMEM;
>> >> > +
>> >> > +       /* It's okay for MMIO to be NULL */
>> >> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> >> > +       tdev->mmio = devm_ioremap_resource(&pdev->dev, res);
>> >> > +       if (IS_ERR(tdev->mmio))
>> >> > +               tdev->mmio = NULL;
>> >> > +
>> >> > +       tdev->tx_channel = mbox_test_request_channel(pdev, "tx");
>> >> > +       if (IS_ERR(tdev->tx_channel))
>> >> > +               return PTR_ERR(tdev->tx_channel);
>> >> > +
>> >> > +       tdev->rx_channel = mbox_test_request_channel(pdev, "rx");
>> >> > +       if (IS_ERR(tdev->rx_channel))
>> >> > +               return PTR_ERR(tdev->rx_channel);
>> >> > +
>> >> Should it really fail on TX or RX only clients?
>> >
>> > Good question.  Probably not, but I guess we'd need a flag for that.
>> >
>> Just assume 1 channel. You can't make it receive if the controller
>> can't do rx, so the userspace will simply block on read requests. And
>> you can't make the controller send, if it can't. So any write attempt
>> will simply return with an error.
>>  And we are going to need very platform specific knowledge to execute
>> the tests. It's not like some loop enumerating all possible
>> combinations of parameters that the api must pass... so we are good.
>
> I'm guessing most tests will be some kind of send-reply test, so we'd
> need two channels for that.  I can make it so the driver does not fail
> if 'tx' or 'rx' is missing, but will fail if both are.
>
I have seen more channels to be rx+tx, than rx/tx only. And for latter
case the user should run 2 test threads, one each for RX and TX.

> Bare in mind that this test driver isn't designed to cover all of the
> corner cases, but more designed as a helpful boiler plate where it
> will tick some use-case boxes, but is clear enough to be hacked around
> by other vendors who have different requirements.
>
> With regards to you comments on userspace; I suggest that whoever is
> testing the controller will know it and will not attempt to check for
> a reply from a controller (or co-proc) which is write-only.
>
That's actually my point :)
When the userspace already has all that platform specific knowledge,
why check for 'valid' usage in controller driver? Simply reject any
request that the controller isn't capable of (which is practically
never supposed to happen).

Thanks.

Re: [PATCH v2 5/6] mailbox: Add generic mechanism for testing Mailbox Controllers

[Lee Jones]

On Thu, 13 Aug 2015, Jassi Brar wrote:

> On Thu, Aug 13, 2015 at 2:49 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Thu, 13 Aug 2015, Jassi Brar wrote:
> 
> >> >> > +
> >> >> > +static void mbox_test_prepare_message(struct mbox_client *client, void *message)
> >> >> > +{
> >> >> > +       struct mbox_test_device *tdev = dev_get_drvdata(client->dev);
> >> >> > +
> >> >> > +       if (tdev->mmio)
> >> >> > +               memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
> >> >> >
> >> >> This is unlikely to work. Those platforms that need to send a 2 part
> >> >> message, they do :
> >> >> (a) Signal/Command/Target code via some controller register (via
> >> >> mbox_send_message).
> >> >> (b) Setup the payload in Shared-Memory (via tx_prepare).
> >> >>
> >> >> This test driver assumes both are the same. I think you have to
> >> >> declare something like
> >> >
> >> > This driver assumes that the framework will call client->tx_prepare()
> >> > first, which satisfies (b).  It then assumes controller->send_data()
> >> > will be invoked, which will send the platform specific
> >> > signal/command/target code, which then satisfies (a).
> >> >
> >> Yeah, but what would be that code? Who provides that?
> >>
> >> > In what way does it assume they are the same?
> >> >
> >> notice the 'message' pointer in
> >> mbox_send_message(tdev->tx_channel, message);
> >>     and
> >> memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
> >>
> >> >> struct mbox_test_message { /* same for TX and RX */
> >> >>           unsigned sig_len;
> >> >>           void *signal;               /* rx/tx via mailbox api */
> >> >>           unsigned pl_len;
> >> >>           void *payload;           /* rx/tx via shared-memory */
> >> >> };
> >> >
> >> > How do you think this should be set and use these?
> >> >
> >> The userspace would want to specify the command code (32bits or not)
> >> that would be passed via the fifo/register of the controller. So we
> >> need signal[]
> >>
> >> The data to be passed via share-memory could be provided by userspace
> >> via the payload[] array.
> >
> > Okay, so would the solution be two userspace files 'signal' and
> > 'message', so in the case of a client specified signal we can write it
> > into there first.
> >
> > echo 255  > signal
> > echo test > message
> >
> > ... or in the case where no signal is required, or the controller
> > driver taking care of it, we just don't write anything to signal?
> >
> file_operations.write() should parse signal and message, coming from
> userspace, into a local structure before routing them via
> mbox_send_message and tx_prepare respectively.

Okay.  So before I code this up we should agree on syntax.

How would you like to represent the break between signal and message?
Obviously ' ' would be a bad idea, as some clients may want to send
messages with white space contained.  How about '\t' or '\n'?

> > Just for clarification, in the case where a signal is required, do
> > clients usually pass that through mbox_send_message() too?
> >
> >   mbox_send_message(tdev->tx_channel, signal);
> >   mbox_send_message(tdev->tx_channel, message);
> >
> No. Command/Signal code is passed via mbox_send_message(signal). The
> payload is passed via Shared-Memory during tx_prepare(message)

Okay, I see.  Thanks for the clarification.

> >> >> > +static void mbox_test_message_sent(struct mbox_client *client,
> >> >> > +                                  void *message, int r)
> >> >> > +{
> >> >> > +       if (r)
> >> >> > +               dev_warn(client->dev,
> >> >> > +                        "Client: Message could not be sent: %d\n", r);
> >> >> > +       else
> >> >> > +               dev_info(client->dev,
> >> >> > +                        "Client: Message sent\n");
> >> >> > +}
> >> >> > +
> >> >> > +static struct mbox_chan *
> >> >> > +mbox_test_request_channel(struct platform_device *pdev, const char *name)
> >> >> > +{
> >> >> > +       struct mbox_client *client;
> >> >> > +
> >> >> > +       client = devm_kzalloc(&pdev->dev, sizeof(*client), GFP_KERNEL);
> >> >> > +       if (!client)
> >> >> > +               return ERR_PTR(-ENOMEM);
> >> >> > +
> >> >> > +       client->dev             = &pdev->dev;
> >> >> > +       client->rx_callback     = mbox_test_receive_message;
> >> >> > +       client->tx_prepare      = mbox_test_prepare_message;
> >> >> > +       client->tx_done         = mbox_test_message_sent;
> >> >> > +       client->tx_block        = true;
> >> >> > +       client->knows_txdone    = false;
> >> >> > +       client->tx_tout         = 500;
> >> >> > +
> >> >> > +       return mbox_request_channel_byname(client, name);
> >> >> > +}
> >> >> > +
> >> >> > +static int mbox_test_probe(struct platform_device *pdev)
> >> >> > +{
> >> >> > +       struct mbox_test_device *tdev;
> >> >> > +       struct resource *res;
> >> >> > +       int ret;
> >> >> > +
> >> >> > +       tdev = devm_kzalloc(&pdev->dev, sizeof(*tdev), GFP_KERNEL);
> >> >> > +       if (!tdev)
> >> >> > +               return -ENOMEM;
> >> >> > +
> >> >> > +       /* It's okay for MMIO to be NULL */
> >> >> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> >> > +       tdev->mmio = devm_ioremap_resource(&pdev->dev, res);
> >> >> > +       if (IS_ERR(tdev->mmio))
> >> >> > +               tdev->mmio = NULL;
> >> >> > +
> >> >> > +       tdev->tx_channel = mbox_test_request_channel(pdev, "tx");
> >> >> > +       if (IS_ERR(tdev->tx_channel))
> >> >> > +               return PTR_ERR(tdev->tx_channel);
> >> >> > +
> >> >> > +       tdev->rx_channel = mbox_test_request_channel(pdev, "rx");
> >> >> > +       if (IS_ERR(tdev->rx_channel))
> >> >> > +               return PTR_ERR(tdev->rx_channel);
> >> >> > +
> >> >> Should it really fail on TX or RX only clients?
> >> >
> >> > Good question.  Probably not, but I guess we'd need a flag for that.
> >> >
> >> Just assume 1 channel. You can't make it receive if the controller
> >> can't do rx, so the userspace will simply block on read requests. And
> >> you can't make the controller send, if it can't. So any write attempt
> >> will simply return with an error.
> >>  And we are going to need very platform specific knowledge to execute
> >> the tests. It's not like some loop enumerating all possible
> >> combinations of parameters that the api must pass... so we are good.
> >
> > I'm guessing most tests will be some kind of send-reply test, so we'd
> > need two channels for that.  I can make it so the driver does not fail
> > if 'tx' or 'rx' is missing, but will fail if both are.
> >
> I have seen more channels to be rx+tx, than rx/tx only. And for latter
> case the user should run 2 test threads, one each for RX and TX.

Okay, makes sense to me.

> > Bare in mind that this test driver isn't designed to cover all of the
> > corner cases, but more designed as a helpful boiler plate where it
> > will tick some use-case boxes, but is clear enough to be hacked around
> > by other vendors who have different requirements.
> >
> > With regards to you comments on userspace; I suggest that whoever is
> > testing the controller will know it and will not attempt to check for
> > a reply from a controller (or co-proc) which is write-only.
> >
> That's actually my point :)
> When the userspace already has all that platform specific knowledge,
> why check for 'valid' usage in controller driver? Simply reject any
> request that the controller isn't capable of (which is practically
> never supposed to happen).

This conversation doesn't really belong in this thread; but seeing as
you bought it up ...

I absolutely and fundamentally disagree that the controller shouldn't
do error checking during mbox_request_channel(). It is
mbox_request_channel() that should fail on a known bad configuration,
rather than pass back a known rotten apple and wait for that it to be
eaten (appreciate I've gone a little overboard with the analogies)
before flagging it up.  And for what purpose?  To make the controller
driver a little simpler?  I do hope that you do see sense on this one
and allow me to keep the error checking in the driver!

If you have a reply to this, please do so one the driver thread, so
it's easier to follow.

Secondly, a reply more akin to this thread.  The guys using _this_
driver are more likely to be Controller authors.  These are the chaps
I was speaking about that are going to know the platform specifics of
the controller.  The error checking in mbox_request_channel() is
designed for 'client' authors, who are more likely to be requesting
invalid configurations.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 5/6] mailbox: Add generic mechanism for testing Mailbox Controllers

[Jassi Brar]

On Thu, Aug 13, 2015 at 3:53 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Thu, 13 Aug 2015, Jassi Brar wrote:
>
>> On Thu, Aug 13, 2015 at 2:49 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> > On Thu, 13 Aug 2015, Jassi Brar wrote:
>>
>> >> >> > +
>> >> >> > +static void mbox_test_prepare_message(struct mbox_client *client, void *message)
>> >> >> > +{
>> >> >> > +       struct mbox_test_device *tdev = dev_get_drvdata(client->dev);
>> >> >> > +
>> >> >> > +       if (tdev->mmio)
>> >> >> > +               memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
>> >> >> >
>> >> >> This is unlikely to work. Those platforms that need to send a 2 part
>> >> >> message, they do :
>> >> >> (a) Signal/Command/Target code via some controller register (via
>> >> >> mbox_send_message).
>> >> >> (b) Setup the payload in Shared-Memory (via tx_prepare).
>> >> >>
>> >> >> This test driver assumes both are the same. I think you have to
>> >> >> declare something like
>> >> >
>> >> > This driver assumes that the framework will call client->tx_prepare()
>> >> > first, which satisfies (b).  It then assumes controller->send_data()
>> >> > will be invoked, which will send the platform specific
>> >> > signal/command/target code, which then satisfies (a).
>> >> >
>> >> Yeah, but what would be that code? Who provides that?
>> >>
>> >> > In what way does it assume they are the same?
>> >> >
>> >> notice the 'message' pointer in
>> >> mbox_send_message(tdev->tx_channel, message);
>> >>     and
>> >> memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
>> >>
>> >> >> struct mbox_test_message { /* same for TX and RX */
>> >> >>           unsigned sig_len;
>> >> >>           void *signal;               /* rx/tx via mailbox api */
>> >> >>           unsigned pl_len;
>> >> >>           void *payload;           /* rx/tx via shared-memory */
>> >> >> };
>> >> >
>> >> > How do you think this should be set and use these?
>> >> >
>> >> The userspace would want to specify the command code (32bits or not)
>> >> that would be passed via the fifo/register of the controller. So we
>> >> need signal[]
>> >>
>> >> The data to be passed via share-memory could be provided by userspace
>> >> via the payload[] array.
>> >
>> > Okay, so would the solution be two userspace files 'signal' and
>> > 'message', so in the case of a client specified signal we can write it
>> > into there first.
>> >
>> > echo 255  > signal
>> > echo test > message
>> >
>> > ... or in the case where no signal is required, or the controller
>> > driver taking care of it, we just don't write anything to signal?
>> >
>> file_operations.write() should parse signal and message, coming from
>> userspace, into a local structure before routing them via
>> mbox_send_message and tx_prepare respectively.
>
> Okay.  So before I code this up we should agree on syntax.
>
> How would you like to represent the break between signal and message?
> Obviously ' ' would be a bad idea, as some clients may want to send
> messages with white space contained.  How about '\t' or '\n'?
>
Yeah, I am not a fan of markers and flags either.

Maybe we should share with userspace
  struct mbox_test_message { /* same for TX and RX */
           unsigned sig_len;
           void __user *signal;        /* rx/tx via mailbox api */
           unsigned pl_len;
           void __user *payload;    /* rx/tx via shared-memory */
  };

First copy_from_user the structure of length sizof(struct
mbox_test_message) and then copy_from_user length sig_len and pl_len
from signal[] and payload[] respectively (usually ioctls would carry
such data).

Re: [PATCH v2 5/6] mailbox: Add generic mechanism for testing Mailbox Controllers

[Lee Jones]

On Thu, 13 Aug 2015, Jassi Brar wrote:

> On Thu, Aug 13, 2015 at 3:53 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Thu, 13 Aug 2015, Jassi Brar wrote:
> >
> >> On Thu, Aug 13, 2015 at 2:49 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >> > On Thu, 13 Aug 2015, Jassi Brar wrote:
> >>
> >> >> >> > +
> >> >> >> > +static void mbox_test_prepare_message(struct mbox_client *client, void *message)
> >> >> >> > +{
> >> >> >> > +       struct mbox_test_device *tdev = dev_get_drvdata(client->dev);
> >> >> >> > +
> >> >> >> > +       if (tdev->mmio)
> >> >> >> > +               memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
> >> >> >> >
> >> >> >> This is unlikely to work. Those platforms that need to send a 2 part
> >> >> >> message, they do :
> >> >> >> (a) Signal/Command/Target code via some controller register (via
> >> >> >> mbox_send_message).
> >> >> >> (b) Setup the payload in Shared-Memory (via tx_prepare).
> >> >> >>
> >> >> >> This test driver assumes both are the same. I think you have to
> >> >> >> declare something like
> >> >> >
> >> >> > This driver assumes that the framework will call client->tx_prepare()
> >> >> > first, which satisfies (b).  It then assumes controller->send_data()
> >> >> > will be invoked, which will send the platform specific
> >> >> > signal/command/target code, which then satisfies (a).
> >> >> >
> >> >> Yeah, but what would be that code? Who provides that?
> >> >>
> >> >> > In what way does it assume they are the same?
> >> >> >
> >> >> notice the 'message' pointer in
> >> >> mbox_send_message(tdev->tx_channel, message);
> >> >>     and
> >> >> memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
> >> >>
> >> >> >> struct mbox_test_message { /* same for TX and RX */
> >> >> >>           unsigned sig_len;
> >> >> >>           void *signal;               /* rx/tx via mailbox api */
> >> >> >>           unsigned pl_len;
> >> >> >>           void *payload;           /* rx/tx via shared-memory */
> >> >> >> };
> >> >> >
> >> >> > How do you think this should be set and use these?
> >> >> >
> >> >> The userspace would want to specify the command code (32bits or not)
> >> >> that would be passed via the fifo/register of the controller. So we
> >> >> need signal[]
> >> >>
> >> >> The data to be passed via share-memory could be provided by userspace
> >> >> via the payload[] array.
> >> >
> >> > Okay, so would the solution be two userspace files 'signal' and
> >> > 'message', so in the case of a client specified signal we can write it
> >> > into there first.
> >> >
> >> > echo 255  > signal
> >> > echo test > message
> >> >
> >> > ... or in the case where no signal is required, or the controller
> >> > driver taking care of it, we just don't write anything to signal?
> >> >
> >> file_operations.write() should parse signal and message, coming from
> >> userspace, into a local structure before routing them via
> >> mbox_send_message and tx_prepare respectively.
> >
> > Okay.  So before I code this up we should agree on syntax.
> >
> > How would you like to represent the break between signal and message?
> > Obviously ' ' would be a bad idea, as some clients may want to send
> > messages with white space contained.  How about '\t' or '\n'?
> >
> Yeah, I am not a fan of markers and flags either.
> 
> Maybe we should share with userspace
>   struct mbox_test_message { /* same for TX and RX */
>            unsigned sig_len;
>            void __user *signal;        /* rx/tx via mailbox api */
>            unsigned pl_len;
>            void __user *payload;    /* rx/tx via shared-memory */
>   };
> 
> First copy_from_user the structure of length sizof(struct
> mbox_test_message) and then copy_from_user length sig_len and pl_len
> from signal[] and payload[] respectively (usually ioctls would carry
> such data).

Simplicity and ease of use should be the goals here.  Testers should
not have to write applications to use this driver.  Can we come up
with a simple/effective method that uses SYSFS/DEBUGFS please?

The easiest way I can think of which avoids markers/separators AND the
requirement for users to have to write applications is to have two
files, 'signal' and 'message' as mentioned before.  Once both are
populated I can get this driver to draft the message appropriately and
fire it off.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 5/6] mailbox: Add generic mechanism for testing Mailbox Controllers

[Jassi Brar]

On Thu, Aug 13, 2015 at 4:30 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Thu, 13 Aug 2015, Jassi Brar wrote:
>
>> On Thu, Aug 13, 2015 at 3:53 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> > On Thu, 13 Aug 2015, Jassi Brar wrote:
>> >
>> >> On Thu, Aug 13, 2015 at 2:49 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> >> > On Thu, 13 Aug 2015, Jassi Brar wrote:
>> >>
>> >> >> >> > +
>> >> >> >> > +static void mbox_test_prepare_message(struct mbox_client *client, void *message)
>> >> >> >> > +{
>> >> >> >> > +       struct mbox_test_device *tdev = dev_get_drvdata(client->dev);
>> >> >> >> > +
>> >> >> >> > +       if (tdev->mmio)
>> >> >> >> > +               memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
>> >> >> >> >
>> >> >> >> This is unlikely to work. Those platforms that need to send a 2 part
>> >> >> >> message, they do :
>> >> >> >> (a) Signal/Command/Target code via some controller register (via
>> >> >> >> mbox_send_message).
>> >> >> >> (b) Setup the payload in Shared-Memory (via tx_prepare).
>> >> >> >>
>> >> >> >> This test driver assumes both are the same. I think you have to
>> >> >> >> declare something like
>> >> >> >
>> >> >> > This driver assumes that the framework will call client->tx_prepare()
>> >> >> > first, which satisfies (b).  It then assumes controller->send_data()
>> >> >> > will be invoked, which will send the platform specific
>> >> >> > signal/command/target code, which then satisfies (a).
>> >> >> >
>> >> >> Yeah, but what would be that code? Who provides that?
>> >> >>
>> >> >> > In what way does it assume they are the same?
>> >> >> >
>> >> >> notice the 'message' pointer in
>> >> >> mbox_send_message(tdev->tx_channel, message);
>> >> >>     and
>> >> >> memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
>> >> >>
>> >> >> >> struct mbox_test_message { /* same for TX and RX */
>> >> >> >>           unsigned sig_len;
>> >> >> >>           void *signal;               /* rx/tx via mailbox api */
>> >> >> >>           unsigned pl_len;
>> >> >> >>           void *payload;           /* rx/tx via shared-memory */
>> >> >> >> };
>> >> >> >
>> >> >> > How do you think this should be set and use these?
>> >> >> >
>> >> >> The userspace would want to specify the command code (32bits or not)
>> >> >> that would be passed via the fifo/register of the controller. So we
>> >> >> need signal[]
>> >> >>
>> >> >> The data to be passed via share-memory could be provided by userspace
>> >> >> via the payload[] array.
>> >> >
>> >> > Okay, so would the solution be two userspace files 'signal' and
>> >> > 'message', so in the case of a client specified signal we can write it
>> >> > into there first.
>> >> >
>> >> > echo 255  > signal
>> >> > echo test > message
>> >> >
>> >> > ... or in the case where no signal is required, or the controller
>> >> > driver taking care of it, we just don't write anything to signal?
>> >> >
>> >> file_operations.write() should parse signal and message, coming from
>> >> userspace, into a local structure before routing them via
>> >> mbox_send_message and tx_prepare respectively.
>> >
>> > Okay.  So before I code this up we should agree on syntax.
>> >
>> > How would you like to represent the break between signal and message?
>> > Obviously ' ' would be a bad idea, as some clients may want to send
>> > messages with white space contained.  How about '\t' or '\n'?
>> >
>> Yeah, I am not a fan of markers and flags either.
>>
>> Maybe we should share with userspace
>>   struct mbox_test_message { /* same for TX and RX */
>>            unsigned sig_len;
>>            void __user *signal;        /* rx/tx via mailbox api */
>>            unsigned pl_len;
>>            void __user *payload;    /* rx/tx via shared-memory */
>>   };
>>
>> First copy_from_user the structure of length sizof(struct
>> mbox_test_message) and then copy_from_user length sig_len and pl_len
>> from signal[] and payload[] respectively (usually ioctls would carry
>> such data).
>
> Simplicity and ease of use should be the goals here.  Testers should
> not have to write applications to use this driver.  Can we come up
> with a simple/effective method that uses SYSFS/DEBUGFS please?
>
> The easiest way I can think of which avoids markers/separators AND the
> requirement for users to have to write applications is to have two
> files, 'signal' and 'message' as mentioned before.  Once both are
> populated I can get this driver to draft the message appropriately and
> fire it off.
>
And then write to more files for RX data? ... which should also be in
the form of 'signal' and 'message'.

BTW like for spi there is a stock application in
Documentation/spi/spidev_test.c we could do the same?

Re: [PATCH v2 5/6] mailbox: Add generic mechanism for testing Mailbox Controllers

[Lee Jones]

On Thu, 13 Aug 2015, Jassi Brar wrote:

> On Thu, Aug 13, 2015 at 4:30 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Thu, 13 Aug 2015, Jassi Brar wrote:
> >
> >> On Thu, Aug 13, 2015 at 3:53 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >> > On Thu, 13 Aug 2015, Jassi Brar wrote:
> >> >
> >> >> On Thu, Aug 13, 2015 at 2:49 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >> >> > On Thu, 13 Aug 2015, Jassi Brar wrote:
> >> >>
> >> >> >> >> > +
> >> >> >> >> > +static void mbox_test_prepare_message(struct mbox_client *client, void *message)
> >> >> >> >> > +{
> >> >> >> >> > +       struct mbox_test_device *tdev = dev_get_drvdata(client->dev);
> >> >> >> >> > +
> >> >> >> >> > +       if (tdev->mmio)
> >> >> >> >> > +               memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
> >> >> >> >> >
> >> >> >> >> This is unlikely to work. Those platforms that need to send a 2 part
> >> >> >> >> message, they do :
> >> >> >> >> (a) Signal/Command/Target code via some controller register (via
> >> >> >> >> mbox_send_message).
> >> >> >> >> (b) Setup the payload in Shared-Memory (via tx_prepare).
> >> >> >> >>
> >> >> >> >> This test driver assumes both are the same. I think you have to
> >> >> >> >> declare something like
> >> >> >> >
> >> >> >> > This driver assumes that the framework will call client->tx_prepare()
> >> >> >> > first, which satisfies (b).  It then assumes controller->send_data()
> >> >> >> > will be invoked, which will send the platform specific
> >> >> >> > signal/command/target code, which then satisfies (a).
> >> >> >> >
> >> >> >> Yeah, but what would be that code? Who provides that?
> >> >> >>
> >> >> >> > In what way does it assume they are the same?
> >> >> >> >
> >> >> >> notice the 'message' pointer in
> >> >> >> mbox_send_message(tdev->tx_channel, message);
> >> >> >>     and
> >> >> >> memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
> >> >> >>
> >> >> >> >> struct mbox_test_message { /* same for TX and RX */
> >> >> >> >>           unsigned sig_len;
> >> >> >> >>           void *signal;               /* rx/tx via mailbox api */
> >> >> >> >>           unsigned pl_len;
> >> >> >> >>           void *payload;           /* rx/tx via shared-memory */
> >> >> >> >> };
> >> >> >> >
> >> >> >> > How do you think this should be set and use these?
> >> >> >> >
> >> >> >> The userspace would want to specify the command code (32bits or not)
> >> >> >> that would be passed via the fifo/register of the controller. So we
> >> >> >> need signal[]
> >> >> >>
> >> >> >> The data to be passed via share-memory could be provided by userspace
> >> >> >> via the payload[] array.
> >> >> >
> >> >> > Okay, so would the solution be two userspace files 'signal' and
> >> >> > 'message', so in the case of a client specified signal we can write it
> >> >> > into there first.
> >> >> >
> >> >> > echo 255  > signal
> >> >> > echo test > message
> >> >> >
> >> >> > ... or in the case where no signal is required, or the controller
> >> >> > driver taking care of it, we just don't write anything to signal?
> >> >> >
> >> >> file_operations.write() should parse signal and message, coming from
> >> >> userspace, into a local structure before routing them via
> >> >> mbox_send_message and tx_prepare respectively.
> >> >
> >> > Okay.  So before I code this up we should agree on syntax.
> >> >
> >> > How would you like to represent the break between signal and message?
> >> > Obviously ' ' would be a bad idea, as some clients may want to send
> >> > messages with white space contained.  How about '\t' or '\n'?
> >> >
> >> Yeah, I am not a fan of markers and flags either.
> >>
> >> Maybe we should share with userspace
> >>   struct mbox_test_message { /* same for TX and RX */
> >>            unsigned sig_len;
> >>            void __user *signal;        /* rx/tx via mailbox api */
> >>            unsigned pl_len;
> >>            void __user *payload;    /* rx/tx via shared-memory */
> >>   };
> >>
> >> First copy_from_user the structure of length sizof(struct
> >> mbox_test_message) and then copy_from_user length sig_len and pl_len
> >> from signal[] and payload[] respectively (usually ioctls would carry
> >> such data).
> >
> > Simplicity and ease of use should be the goals here.  Testers should
> > not have to write applications to use this driver.  Can we come up
> > with a simple/effective method that uses SYSFS/DEBUGFS please?
> >
> > The easiest way I can think of which avoids markers/separators AND the
> > requirement for users to have to write applications is to have two
> > files, 'signal' and 'message' as mentioned before.  Once both are
> > populated I can get this driver to draft the message appropriately and
> > fire it off.
> >
> And then write to more files for RX data? ... which should also be in
> the form of 'signal' and 'message'.
> 
> BTW like for spi there is a stock application in
> Documentation/spi/spidev_test.c we could do the same?

Coming from personal experience, testing drivers with (even stock)
applications is much more painful that simply writing/reading
(cat/echo) to a file in SYSFS/DEBUGFS.  Particularly if people are
using initramfs or thelike.  If it is possible to use SYSFS/DEBUGFS,
which it is in this case, then I believe that's the route we could go
down.

In answer to your question; we only need those two files.  The reply
can be placed back into 'message' and can be read from there.

Simple to use, simple to code (on both sides), elegant, no overhead.

Win, win, win, win.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 5/6] mailbox: Add generic mechanism for testing Mailbox Controllers

[Jassi Brar]

On Thu, Aug 13, 2015 at 5:10 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Thu, 13 Aug 2015, Jassi Brar wrote:
>
>> On Thu, Aug 13, 2015 at 4:30 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> > On Thu, 13 Aug 2015, Jassi Brar wrote:
>> >
>> >> On Thu, Aug 13, 2015 at 3:53 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> >> > On Thu, 13 Aug 2015, Jassi Brar wrote:
>> >> >
>> >> >> On Thu, Aug 13, 2015 at 2:49 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> >> >> > On Thu, 13 Aug 2015, Jassi Brar wrote:
>> >> >>
>> >> >> >> >> > +
>> >> >> >> >> > +static void mbox_test_prepare_message(struct mbox_client *client, void *message)
>> >> >> >> >> > +{
>> >> >> >> >> > +       struct mbox_test_device *tdev = dev_get_drvdata(client->dev);
>> >> >> >> >> > +
>> >> >> >> >> > +       if (tdev->mmio)
>> >> >> >> >> > +               memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
>> >> >> >> >> >
>> >> >> >> >> This is unlikely to work. Those platforms that need to send a 2 part
>> >> >> >> >> message, they do :
>> >> >> >> >> (a) Signal/Command/Target code via some controller register (via
>> >> >> >> >> mbox_send_message).
>> >> >> >> >> (b) Setup the payload in Shared-Memory (via tx_prepare).
>> >> >> >> >>
>> >> >> >> >> This test driver assumes both are the same. I think you have to
>> >> >> >> >> declare something like
>> >> >> >> >
>> >> >> >> > This driver assumes that the framework will call client->tx_prepare()
>> >> >> >> > first, which satisfies (b).  It then assumes controller->send_data()
>> >> >> >> > will be invoked, which will send the platform specific
>> >> >> >> > signal/command/target code, which then satisfies (a).
>> >> >> >> >
>> >> >> >> Yeah, but what would be that code? Who provides that?
>> >> >> >>
>> >> >> >> > In what way does it assume they are the same?
>> >> >> >> >
>> >> >> >> notice the 'message' pointer in
>> >> >> >> mbox_send_message(tdev->tx_channel, message);
>> >> >> >>     and
>> >> >> >> memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
>> >> >> >>
>> >> >> >> >> struct mbox_test_message { /* same for TX and RX */
>> >> >> >> >>           unsigned sig_len;
>> >> >> >> >>           void *signal;               /* rx/tx via mailbox api */
>> >> >> >> >>           unsigned pl_len;
>> >> >> >> >>           void *payload;           /* rx/tx via shared-memory */
>> >> >> >> >> };
>> >> >> >> >
>> >> >> >> > How do you think this should be set and use these?
>> >> >> >> >
>> >> >> >> The userspace would want to specify the command code (32bits or not)
>> >> >> >> that would be passed via the fifo/register of the controller. So we
>> >> >> >> need signal[]
>> >> >> >>
>> >> >> >> The data to be passed via share-memory could be provided by userspace
>> >> >> >> via the payload[] array.
>> >> >> >
>> >> >> > Okay, so would the solution be two userspace files 'signal' and
>> >> >> > 'message', so in the case of a client specified signal we can write it
>> >> >> > into there first.
>> >> >> >
>> >> >> > echo 255  > signal
>> >> >> > echo test > message
>> >> >> >
>> >> >> > ... or in the case where no signal is required, or the controller
>> >> >> > driver taking care of it, we just don't write anything to signal?
>> >> >> >
>> >> >> file_operations.write() should parse signal and message, coming from
>> >> >> userspace, into a local structure before routing them via
>> >> >> mbox_send_message and tx_prepare respectively.
>> >> >
>> >> > Okay.  So before I code this up we should agree on syntax.
>> >> >
>> >> > How would you like to represent the break between signal and message?
>> >> > Obviously ' ' would be a bad idea, as some clients may want to send
>> >> > messages with white space contained.  How about '\t' or '\n'?
>> >> >
>> >> Yeah, I am not a fan of markers and flags either.
>> >>
>> >> Maybe we should share with userspace
>> >>   struct mbox_test_message { /* same for TX and RX */
>> >>            unsigned sig_len;
>> >>            void __user *signal;        /* rx/tx via mailbox api */
>> >>            unsigned pl_len;
>> >>            void __user *payload;    /* rx/tx via shared-memory */
>> >>   };
>> >>
>> >> First copy_from_user the structure of length sizof(struct
>> >> mbox_test_message) and then copy_from_user length sig_len and pl_len
>> >> from signal[] and payload[] respectively (usually ioctls would carry
>> >> such data).
>> >
>> > Simplicity and ease of use should be the goals here.  Testers should
>> > not have to write applications to use this driver.  Can we come up
>> > with a simple/effective method that uses SYSFS/DEBUGFS please?
>> >
>> > The easiest way I can think of which avoids markers/separators AND the
>> > requirement for users to have to write applications is to have two
>> > files, 'signal' and 'message' as mentioned before.  Once both are
>> > populated I can get this driver to draft the message appropriately and
>> > fire it off.
>> >
>> And then write to more files for RX data? ... which should also be in
>> the form of 'signal' and 'message'.
>>
>> BTW like for spi there is a stock application in
>> Documentation/spi/spidev_test.c we could do the same?
>
> Coming from personal experience, testing drivers with (even stock)
> applications is much more painful that simply writing/reading
> (cat/echo) to a file in SYSFS/DEBUGFS.  Particularly if people are
> using initramfs or thelike.  If it is possible to use SYSFS/DEBUGFS,
> which it is in this case, then I believe that's the route we could go
> down.
>
Well, where could sysfs/debugfs not be used? :)  Anyways I am ok if
prefer debugfs.

> In answer to your question; we only need those two files.  The reply
> can be placed back into 'message' and can be read from there.
>
Testing shouldn't be restricted to 'send command and receive reply'.
What if Linux only listens to broadcasts from the remote? Who knows
someone might want to (ab)use this test client to implement userspace
handler of remote commands?
So please see RX to be independent of TX.

Thanks.

Re: [PATCH v2 5/6] mailbox: Add generic mechanism for testing Mailbox Controllers

[Lee Jones]

On Thu, 13 Aug 2015, Jassi Brar wrote:

> On Thu, Aug 13, 2015 at 5:10 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Thu, 13 Aug 2015, Jassi Brar wrote:
> >
> >> On Thu, Aug 13, 2015 at 4:30 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >> > On Thu, 13 Aug 2015, Jassi Brar wrote:
> >> >
> >> >> On Thu, Aug 13, 2015 at 3:53 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >> >> > On Thu, 13 Aug 2015, Jassi Brar wrote:
> >> >> >
> >> >> >> On Thu, Aug 13, 2015 at 2:49 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >> >> >> > On Thu, 13 Aug 2015, Jassi Brar wrote:
> >> >> >>
> >> >> >> >> >> > +
> >> >> >> >> >> > +static void mbox_test_prepare_message(struct mbox_client *client, void *message)
> >> >> >> >> >> > +{
> >> >> >> >> >> > +       struct mbox_test_device *tdev = dev_get_drvdata(client->dev);
> >> >> >> >> >> > +
> >> >> >> >> >> > +       if (tdev->mmio)
> >> >> >> >> >> > +               memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
> >> >> >> >> >> >
> >> >> >> >> >> This is unlikely to work. Those platforms that need to send a 2 part
> >> >> >> >> >> message, they do :
> >> >> >> >> >> (a) Signal/Command/Target code via some controller register (via
> >> >> >> >> >> mbox_send_message).
> >> >> >> >> >> (b) Setup the payload in Shared-Memory (via tx_prepare).
> >> >> >> >> >>
> >> >> >> >> >> This test driver assumes both are the same. I think you have to
> >> >> >> >> >> declare something like
> >> >> >> >> >
> >> >> >> >> > This driver assumes that the framework will call client->tx_prepare()
> >> >> >> >> > first, which satisfies (b).  It then assumes controller->send_data()
> >> >> >> >> > will be invoked, which will send the platform specific
> >> >> >> >> > signal/command/target code, which then satisfies (a).
> >> >> >> >> >
> >> >> >> >> Yeah, but what would be that code? Who provides that?
> >> >> >> >>
> >> >> >> >> > In what way does it assume they are the same?
> >> >> >> >> >
> >> >> >> >> notice the 'message' pointer in
> >> >> >> >> mbox_send_message(tdev->tx_channel, message);
> >> >> >> >>     and
> >> >> >> >> memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
> >> >> >> >>
> >> >> >> >> >> struct mbox_test_message { /* same for TX and RX */
> >> >> >> >> >>           unsigned sig_len;
> >> >> >> >> >>           void *signal;               /* rx/tx via mailbox api */
> >> >> >> >> >>           unsigned pl_len;
> >> >> >> >> >>           void *payload;           /* rx/tx via shared-memory */
> >> >> >> >> >> };
> >> >> >> >> >
> >> >> >> >> > How do you think this should be set and use these?
> >> >> >> >> >
> >> >> >> >> The userspace would want to specify the command code (32bits or not)
> >> >> >> >> that would be passed via the fifo/register of the controller. So we
> >> >> >> >> need signal[]
> >> >> >> >>
> >> >> >> >> The data to be passed via share-memory could be provided by userspace
> >> >> >> >> via the payload[] array.
> >> >> >> >
> >> >> >> > Okay, so would the solution be two userspace files 'signal' and
> >> >> >> > 'message', so in the case of a client specified signal we can write it
> >> >> >> > into there first.
> >> >> >> >
> >> >> >> > echo 255  > signal
> >> >> >> > echo test > message
> >> >> >> >
> >> >> >> > ... or in the case where no signal is required, or the controller
> >> >> >> > driver taking care of it, we just don't write anything to signal?
> >> >> >> >
> >> >> >> file_operations.write() should parse signal and message, coming from
> >> >> >> userspace, into a local structure before routing them via
> >> >> >> mbox_send_message and tx_prepare respectively.
> >> >> >
> >> >> > Okay.  So before I code this up we should agree on syntax.
> >> >> >
> >> >> > How would you like to represent the break between signal and message?
> >> >> > Obviously ' ' would be a bad idea, as some clients may want to send
> >> >> > messages with white space contained.  How about '\t' or '\n'?
> >> >> >
> >> >> Yeah, I am not a fan of markers and flags either.
> >> >>
> >> >> Maybe we should share with userspace
> >> >>   struct mbox_test_message { /* same for TX and RX */
> >> >>            unsigned sig_len;
> >> >>            void __user *signal;        /* rx/tx via mailbox api */
> >> >>            unsigned pl_len;
> >> >>            void __user *payload;    /* rx/tx via shared-memory */
> >> >>   };
> >> >>
> >> >> First copy_from_user the structure of length sizof(struct
> >> >> mbox_test_message) and then copy_from_user length sig_len and pl_len
> >> >> from signal[] and payload[] respectively (usually ioctls would carry
> >> >> such data).
> >> >
> >> > Simplicity and ease of use should be the goals here.  Testers should
> >> > not have to write applications to use this driver.  Can we come up
> >> > with a simple/effective method that uses SYSFS/DEBUGFS please?
> >> >
> >> > The easiest way I can think of which avoids markers/separators AND the
> >> > requirement for users to have to write applications is to have two
> >> > files, 'signal' and 'message' as mentioned before.  Once both are
> >> > populated I can get this driver to draft the message appropriately and
> >> > fire it off.
> >> >
> >> And then write to more files for RX data? ... which should also be in
> >> the form of 'signal' and 'message'.
> >>
> >> BTW like for spi there is a stock application in
> >> Documentation/spi/spidev_test.c we could do the same?
> >
> > Coming from personal experience, testing drivers with (even stock)
> > applications is much more painful that simply writing/reading
> > (cat/echo) to a file in SYSFS/DEBUGFS.  Particularly if people are
> > using initramfs or thelike.  If it is possible to use SYSFS/DEBUGFS,
> > which it is in this case, then I believe that's the route we could go
> > down.
> >
> Well, where could sysfs/debugfs not be used? :)  Anyways I am ok if
> prefer debugfs.

You know what I mean; CPIO et. al.

> > In answer to your question; we only need those two files.  The reply
> > can be placed back into 'message' and can be read from there.
> >
> Testing shouldn't be restricted to 'send command and receive reply'.
> What if Linux only listens to broadcasts from the remote? Who knows
> someone might want to (ab)use this test client to implement userspace
> handler of remote commands?
> So please see RX to be independent of TX.

Sure.

Now just agree with me that mbox_request_chan() should fail on request
of a known bad configuration request and I can code all this up and
re-submit. :D

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 5/6] mailbox: Add generic mechanism for testing Mailbox Controllers

[Jassi Brar]

On Thu, Aug 13, 2015 at 6:37 PM, Lee Jones <lee.jones@linaro.org> wrote:
>
> Now just agree with me that mbox_request_chan() should fail on request
> of a known bad configuration request and I can code all this up and
> re-submit. :D
>
You make me look like a jerk :(   My problem is not with validation as
such. I see problem in the way you implement that makes validation
necessary. I'll explain step-by-step in the driver post.

Re: [PATCH v2 5/6] mailbox: Add generic mechanism for testing Mailbox Controllers

[Lee Jones]

On Thu, 13 Aug 2015, Jassi Brar wrote:

> On Thu, Aug 13, 2015 at 6:37 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >
> > Now just agree with me that mbox_request_chan() should fail on request
> > of a known bad configuration request and I can code all this up and
> > re-submit. :D
> >
> You make me look like a jerk :(   My problem is not with validation as
> such. I see problem in the way you implement that makes validation
> necessary. I'll explain step-by-step in the driver post.

That wasn't the intention, don't be so sensitive. ;)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 3/6] mailbox: Add support for ST's Mailbox IP

[Jassi Brar]

On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:

> +
> +static bool sti_mbox_tx_is_ready(struct mbox_chan *chan)
> +{
> +       struct sti_channel *chan_info = chan->con_priv;
> +       struct sti_mbox_device *mdev = chan_info->mdev;
> +       unsigned int instance = chan_info->instance;
> +       unsigned int channel = chan_info->channel;
> +       void __iomem *base = MBOX_BASE(mdev, instance);
> +
> +       if (!(chan_info->direction & MBOX_TX))
> +               return false;
>
Here the 'direction' is gotten via DT node of the client i.e, you
expect consumer drivers to tell the provider what its limitations are?

IMO if some physical channel can't do TX then that should be either
hardcoded inside the controller driver or learnt via DT node of the
_controller_.


> +static struct mbox_chan *sti_mbox_xlate(struct mbox_controller *mbox,
> +                                       const struct of_phandle_args *spec)
> +{
> +       struct sti_mbox_device *mdev = dev_get_drvdata(mbox->dev);
> +       struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
> +       struct sti_channel *chan_info;
> +       struct mbox_chan *chan = NULL;
> +       unsigned int instance  = spec->args[0];
> +       unsigned int channel   = spec->args[1];
> +       unsigned int direction = spec->args[2];
> +       int i;
> +
> +       /* Bounds checking */
> +       if (instance >= pdata->num_inst || channel  >= pdata->num_chan) {
> +               dev_err(mbox->dev,
> +                       "Invalid channel requested instance: %d channel: %d\n",
> +                       instance, channel);
> +               return NULL;
return ERR_PTR(-EINVAL)

> +       }
> +
> +       for (i = 0; i < mbox->num_chans; i++) {
> +               chan_info = mbox->chans[i].con_priv;
> +
> +               /* Is requested channel free? */
> +               if (direction != MBOX_LOOPBACK &&
>
Consider this example when 2 clients ask for same physical channel but
in different modes.
           mboxes = <&mboxA 0 1 MBOX_TX>;
           mboxes = <&mboxA 0 1 MBOX_LOOPBACK>;

You happily assign 2 virtual channels backed by one physical channel
{mboxA, 0, 1}. The 2 clients think they can freely do startup(),
shutdown() and send_data() on the channels. But obviously we are
screwed with races like
   client1.startup()
    -> client2.startup()
        -> client2.send_data()
            -> client2.shutdown()
                -> client1.send_data()  XXXX

 Now you can shove in some more checks to 'fix' the race OR you can
simply expose only physical channels. Practically no client would ever
ask it to do what it can't, and for the hypothetical possibility that
some does, just return error.

> +                   chan_info &&
> +                   mbox->dev == chan_info->mdev->dev &&
> +                   instance == chan_info->instance &&
> +                   channel == chan_info->channel) {
> +                       dev_err(mbox->dev, "Channel in use\n");
> +                       return NULL;
return ERR_PTR(-EBUSY)

> +               }
> +
> +               /*
> +                * Find the first free slot, then continue checking
> +                * to see if requested channel is in use
> +                */
> +               if (!chan && !chan_info)
> +                       chan = &mbox->chans[i];
> +       }
> +
> +       if (!chan) {
> +               dev_err(mbox->dev, "No free channels left\n");
> +               return NULL;
return ERR_PTR(-EBUSY)

> +       }
> +
> +       chan_info = devm_kzalloc(mbox->dev, sizeof(*chan_info), GFP_KERNEL);
> +       if (!chan_info)
> +               return NULL;
return ERR_PTR(-ENOMEM)

Thanks.

Re: [PATCH v2 3/6] mailbox: Add support for ST's Mailbox IP

[Lee Jones]

On Thu, 13 Aug 2015, Jassi Brar wrote:
> On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:
> 
> > +
> > +static bool sti_mbox_tx_is_ready(struct mbox_chan *chan)
> > +{
> > +       struct sti_channel *chan_info = chan->con_priv;
> > +       struct sti_mbox_device *mdev = chan_info->mdev;
> > +       unsigned int instance = chan_info->instance;
> > +       unsigned int channel = chan_info->channel;
> > +       void __iomem *base = MBOX_BASE(mdev, instance);
> > +
> > +       if (!(chan_info->direction & MBOX_TX))
> > +               return false;
> >
> Here the 'direction' is gotten via DT node of the client i.e, you
> expect consumer drivers to tell the provider what its limitations are?
> 
> IMO if some physical channel can't do TX then that should be either
> hardcoded inside the controller driver or learnt via DT node of the
> _controller_.

That's a fair point.  I need to create a new property similar to the
already existing 'read-only'.  I guess 'tx-only' is equivalent.

> > +static struct mbox_chan *sti_mbox_xlate(struct mbox_controller *mbox,
> > +                                       const struct of_phandle_args *spec)
> > +{
> > +       struct sti_mbox_device *mdev = dev_get_drvdata(mbox->dev);
> > +       struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
> > +       struct sti_channel *chan_info;
> > +       struct mbox_chan *chan = NULL;
> > +       unsigned int instance  = spec->args[0];
> > +       unsigned int channel   = spec->args[1];
> > +       unsigned int direction = spec->args[2];
> > +       int i;
> > +
> > +       /* Bounds checking */
> > +       if (instance >= pdata->num_inst || channel  >= pdata->num_chan) {
> > +               dev_err(mbox->dev,
> > +                       "Invalid channel requested instance: %d channel: %d\n",
> > +                       instance, channel);
> > +               return NULL;
> return ERR_PTR(-EINVAL)

I can handle all these, no problem.

> > +       }
> > +
> > +       for (i = 0; i < mbox->num_chans; i++) {
> > +               chan_info = mbox->chans[i].con_priv;
> > +
> > +               /* Is requested channel free? */
> > +               if (direction != MBOX_LOOPBACK &&
> >
> Consider this example when 2 clients ask for same physical channel but
> in different modes.
>            mboxes = <&mboxA 0 1 MBOX_TX>;
>            mboxes = <&mboxA 0 1 MBOX_LOOPBACK>;
> 
> You happily assign 2 virtual channels backed by one physical channel
> {mboxA, 0, 1}. The 2 clients think they can freely do startup(),
> shutdown() and send_data() on the channels. But obviously we are
> screwed with races like
>    client1.startup()
>     -> client2.startup()
>         -> client2.send_data()
>             -> client2.shutdown()
>                 -> client1.send_data()  XXXX

Good catch and a fair point.  As you say, it's unlikely to happen, but
I would like to prevent it in any case.

>  Now you can shove in some more checks to 'fix' the race OR you can
> simply expose only physical channels.

We can't expose all of the channels.  There are too many and would
take up too much *unused* memory.

I don't want to have an endless stream of checks either, but we should
try to cover the bases.  I think smarter (rather than more) checks is
the answer.  I'll have a think about it.

> Practically no client would ever
> ask it to do what it can't, and for the hypothetical possibility that
> some does, just return error.

Right.  Smarter error checking here and returning an error on a bad
config is what I plan to do.

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 3/6] mailbox: Add support for ST's Mailbox IP

[Jassi Brar]

On Fri, Aug 14, 2015 at 12:03 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Thu, 13 Aug 2015, Jassi Brar wrote:
>> On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:
>>
>> > +
>> > +static bool sti_mbox_tx_is_ready(struct mbox_chan *chan)
>> > +{
>> > +       struct sti_channel *chan_info = chan->con_priv;
>> > +       struct sti_mbox_device *mdev = chan_info->mdev;
>> > +       unsigned int instance = chan_info->instance;
>> > +       unsigned int channel = chan_info->channel;
>> > +       void __iomem *base = MBOX_BASE(mdev, instance);
>> > +
>> > +       if (!(chan_info->direction & MBOX_TX))
>> > +               return false;
>> >
>> Here the 'direction' is gotten via DT node of the client i.e, you
>> expect consumer drivers to tell the provider what its limitations are?
>>
>> IMO if some physical channel can't do TX then that should be either
>> hardcoded inside the controller driver or learnt via DT node of the
>> _controller_.
>
> That's a fair point.
.

>  I need to create a new property similar to the
> already existing 'read-only'.  I guess 'tx-only' is equivalent.
>
Just to be clear, if you must have such a property it should come from
the _controller_ node.

However at one point you said,  "Only the A9 (Mbox 0) can Rx."
  Which sounds like the 'simplex' constraint is not coming from the
mailbox controller but from the remote endpoints that don't RX+TX
except for one of them. That makes more sense than a controller with
differently capable physical channels. If that is indeed the
situation, then the controller is actually 'duplex' and there should
be no tx-only/rx-only property anywhere. Everything automatically
falls into place because client drivers are written for specific
targets and, unless you write some code, there can be no TX call to a
remote that doesn't listen.

>> > +
>> > +       for (i = 0; i < mbox->num_chans; i++) {
>> > +               chan_info = mbox->chans[i].con_priv;
>> > +
>> > +               /* Is requested channel free? */
>> > +               if (direction != MBOX_LOOPBACK &&
>> >
>> Consider this example when 2 clients ask for same physical channel but
>> in different modes.
>>            mboxes = <&mboxA 0 1 MBOX_TX>;
>>            mboxes = <&mboxA 0 1 MBOX_LOOPBACK>;
>>
>> You happily assign 2 virtual channels backed by one physical channel
>> {mboxA, 0, 1}. The 2 clients think they can freely do startup(),
>> shutdown() and send_data() on the channels. But obviously we are
>> screwed with races like
>>    client1.startup()
>>     -> client2.startup()
>>         -> client2.send_data()
>>             -> client2.shutdown()
>>                 -> client1.send_data()  XXXX
>
> Good catch and a fair point.  As you say, it's unlikely to happen, but
> I would like to prevent it in any case.
>
No, such races are a practical problem. We must own them.

I was talking about problems that arise because someone wrote bad
DT... those are not 'practical' problems because there are too many
ways to screw up with bad DT properties that if we try to check for
them we'll go insane.

>>  Now you can shove in some more checks to 'fix' the race OR you can
>> simply expose only physical channels.
>
> We can't expose all of the channels.  There are too many and would
> take up too much *unused* memory.
>
I am aware of that. I said expose _only_ physical channels, not _all_ :)

Cheers!

Re: [PATCH v2 3/6] mailbox: Add support for ST's Mailbox IP

[Lee Jones]

On Fri, 14 Aug 2015, Jassi Brar wrote:

> On Fri, Aug 14, 2015 at 12:03 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Thu, 13 Aug 2015, Jassi Brar wrote:
> >> On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >>
> >> > +
> >> > +static bool sti_mbox_tx_is_ready(struct mbox_chan *chan)
> >> > +{
> >> > +       struct sti_channel *chan_info = chan->con_priv;
> >> > +       struct sti_mbox_device *mdev = chan_info->mdev;
> >> > +       unsigned int instance = chan_info->instance;
> >> > +       unsigned int channel = chan_info->channel;
> >> > +       void __iomem *base = MBOX_BASE(mdev, instance);
> >> > +
> >> > +       if (!(chan_info->direction & MBOX_TX))
> >> > +               return false;
> >> >
> >> Here the 'direction' is gotten via DT node of the client i.e, you
> >> expect consumer drivers to tell the provider what its limitations are?
> >>
> >> IMO if some physical channel can't do TX then that should be either
> >> hardcoded inside the controller driver or learnt via DT node of the
> >> _controller_.
> >
> > That's a fair point.
> .
> 
> >  I need to create a new property similar to the
> > already existing 'read-only'.  I guess 'tx-only' is equivalent.
> >
> Just to be clear, if you must have such a property it should come from
> the _controller_ node.

Correct.  That's the plan.

> However at one point you said,  "Only the A9 (Mbox 0) can Rx."
>   Which sounds like the 'simplex' constraint is not coming from the
> mailbox controller but from the remote endpoints that don't RX+TX
> except for one of them. That makes more sense than a controller with
> differently capable physical channels. If that is indeed the
> situation, then the controller is actually 'duplex' and there should
> be no tx-only/rx-only property anywhere. Everything automatically
> falls into place because client drivers are written for specific
> targets and, unless you write some code, there can be no TX call to a
> remote that doesn't listen.

Unfortunately it's a restriction of the hardware (or the controller as
you call it, although it's not really a controller).  There is only
one IRQ for Rx'ing and that's wired up to the A9's mailbox (Mailbox
0).  If one of the remote processors attempted to send a message
through any of the other mailboxes (other than the Mailbox 0), then no
one would hear the doorbell ring and the message would go unserviced.

> >> > +
> >> > +       for (i = 0; i < mbox->num_chans; i++) {
> >> > +               chan_info = mbox->chans[i].con_priv;
> >> > +
> >> > +               /* Is requested channel free? */
> >> > +               if (direction != MBOX_LOOPBACK &&
> >> >
> >> Consider this example when 2 clients ask for same physical channel but
> >> in different modes.
> >>            mboxes = <&mboxA 0 1 MBOX_TX>;
> >>            mboxes = <&mboxA 0 1 MBOX_LOOPBACK>;
> >>
> >> You happily assign 2 virtual channels backed by one physical channel
> >> {mboxA, 0, 1}. The 2 clients think they can freely do startup(),
> >> shutdown() and send_data() on the channels. But obviously we are
> >> screwed with races like
> >>    client1.startup()
> >>     -> client2.startup()
> >>         -> client2.send_data()
> >>             -> client2.shutdown()
> >>                 -> client1.send_data()  XXXX
> >
> > Good catch and a fair point.  As you say, it's unlikely to happen, but
> > I would like to prevent it in any case.
> >
> No, such races are a practical problem. We must own them.
> 
> I was talking about problems that arise because someone wrote bad
> DT... those are not 'practical' problems because there are too many
> ways to screw up with bad DT properties that if we try to check for
> them we'll go insane.

The only thing I'm having trouble with protecting at the moment is
other clients _also_ requesting a LOOPBACK channel.  I would like to
check which clients have already requested one/them, however that
information is not available until _after_ xlate() has been called,
which is pretty frustrating.  Perhaps I'll put a comment in instead.

> >>  Now you can shove in some more checks to 'fix' the race OR you can
> >> simply expose only physical channels.
> >
> > We can't expose all of the channels.  There are too many and would
> > take up too much *unused* memory.
> >
> I am aware of that. I said expose _only_ physical channels, not _all_ :)

It's impossible to know which physical channels will be used by
clients and subsequently which physical channels to expose.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog