[PATCH V2 0/2] mailbox: Introduce Texas Instrument's message manager driver

[PATCH V2 0/2] mailbox: Introduce Texas Instrument's message manager driver

[Nishanth Menon]

Hi,

The following v2 of the series provides a base driver for the support
of TI's message manager hardware block which facilitates communication
between multiple compute engines(or processors) with a central system
controller (called PMMC in K2G SoC). Keystone family of TI processors
incorporate this solution starting with K2G[1].

The hardware specification is available here:
http://www.ti.com/lit/ug/spruhy8/spruhy8.pdf Chapter 8.1 (Message
Manager - NOTE: complete chapter update is estimated to be around
March '16).

Starting with K2G SoC, TI Keystone architecture is moving towards an
architecture similar to ARM Juno platform with a centralized system
controller where all system control takes place - all power management
and system control functions are now centralized in this system
controller. A protocol called TI-SCI (TI System Control Interface)
is built on top of this mailbox driver providing for communication
protocol (similar to SCPI drivers/firmware/arm_scpi.c) Follow on
patches (once the message manager is accepted) will enable the same.
However, an 4.1 kernel based vendor kernel implementation is available
at [2].

The following series is functional in TI vendor kernel, however, was
tested on K2G EVM using the following test fragment:
http://pastebin.ubuntu.com/15209502/

Boot log: http://pastebin.ubuntu.com/15209436/

baseline: v4.5-rc1

Changes since v1:
	- dt binding has been majorly upgraded
	- driver updates for the bindings
v1: http://marc.info/?l=devicetree&m=145469015018996&w=2

Nishanth Menon (2):
  Documentation: dt: mailbox: Add TI Message Manager
  mailbox: Introduce TI message manager driver

 .../bindings/mailbox/ti,message-manager.txt        |  50 ++
 drivers/mailbox/Kconfig                            |  11 +
 drivers/mailbox/Makefile                           |   2 +
 drivers/mailbox/ti-msgmgr.c                        | 657 +++++++++++++++++++++
 include/linux/ti-msgmgr.h                          |  35 ++
 5 files changed, 755 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/ti,message-manager.txt
 create mode 100644 drivers/mailbox/ti-msgmgr.c
 create mode 100644 include/linux/ti-msgmgr.h

[1] K2G support
https://patchwork.kernel.org/patch/8196481/
https://patchwork.kernel.org/patch/8196491/
https://patchwork.kernel.org/patch/8196471/

[2] TISCI support(for reference): (mailbox client driver)
http://git.ti.com/ti-linux-kernel/ti-linux-kernel/blobs/ti-lsk-linux-4.1.y/Documentation/devicetree/bindings/arm/keystone/ti,sci.txt

http://git.ti.com/ti-linux-kernel/ti-linux-kernel/blobs/ti-lsk-linux-4.1.y/drivers/firmware/ti_sci.c
http://git.ti.com/ti-linux-kernel/ti-linux-kernel/blobs/ti-lsk-linux-4.1.y/drivers/firmware/ti_sci.h
http://git.ti.com/ti-linux-kernel/ti-linux-kernel/blobs/ti-lsk-linux-4.1.y/include/linux/ti_sci_protocol.h
The above protocol is used for clock, generic powerdomain, reset
etc..
-- 
2.7.0

[PATCH V2 1/2] Documentation: dt: mailbox: Add TI Message Manager

[Nishanth Menon]

Message Manager is a hardware block used to communicate with various
processor systems within certain Texas Instrument's Keystone
generation SoCs.

This hardware engine is used to transfer messages from various compute
entities(or processors) within the SoC. It is designed to be self
contained without needing software initialization for operation.

Signed-off-by: Nishanth Menon <nm@ti.com>
---
Changes in V2: major refactor of binding - Thanks for all the review from Rob and Jassi
	- Generic compatible property is dropped
	- no more child nodes, instead interrupt information is provided as part
	  of the the message manager node it self - part of data moved into
	  driver based on compatible property.
	- mbox-cells is 2 (passes queue_id and proxy_id) -added elaboration as well
	- change in interrupt names

V1: https://patchwork.kernel.org/patch/8237111/

 .../bindings/mailbox/ti,message-manager.txt        | 50 ++++++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/ti,message-manager.txt

diff --git a/Documentation/devicetree/bindings/mailbox/ti,message-manager.txt b/Documentation/devicetree/bindings/mailbox/ti,message-manager.txt
new file mode 100644
index 000000000000..b449d025049f
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/ti,message-manager.txt
@@ -0,0 +1,50 @@
+Texas Instruments' Message Manager Driver
+========================================
+
+The Texas Instruments' Message Manager is a mailbox controller that has
+configurable queues selectable at SoC(System on Chip) integration. The Message
+manager is broken up into queues in different address regions that are called
+"proxies" - each instance is unidirectional and is instantiated at SoC
+integration level to indicate receive or transmit path.
+
+Message Manager Device Node:
+===========================
+Required properties:
+--------------------
+- compatible:		Shall be: "ti,k2g-message-manager"
+- reg-names 		queue_proxy_region - Map the queue proxy region.
+			queue_state_debug_region - Map the queue state debug
+			region.
+- reg:			Contains the register map per reg-names.
+- #mbox-cells		Shall be 2. Contains the queue ID and proxy ID in that
+		        order referring to the transfer path.
+- interrupt-names:	Contains interrupt names matching the rx transfer path
+			for a given SoC. Receive interrupts shall be of the
+			format: "rx_<QID>_<PID>".
+			For ti,k2g-message-manager, this shall contain:
+				"rx_005_002", "rx_057_002"
+- interrupts:		Contains the interrupt information corresponding to
+			interrupt-names property.
+
+Example(K2G):
+------------
+
+	msgmgr: msgmgr@02a00000 {
+		compatible = "ti,k2g-message-manager";
+		#mbox-cells = <2>;
+		reg-names = "queue_proxy_region", "queue_state_debug_region";
+		reg = <0x02a00000 0x400000>, <0x028c3400 0x400>;
+		interrupt-names = "rx_005", "rx_057";
+		interrupts = <GIC_SPI 324 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 327 IRQ_TYPE_LEVEL_HIGH>;
+	};
+
+	pmmc: pmmc {
+		[...]
+		mbox-names = "rx", "tx";
+		# RX queue ID is 5, proxy ID is 2
+		# TX queue ID is 0, proxy ID is 0
+		mboxes= <&msgmgr 5 2>,
+			<&msgmgr 0 0>;
+		[...]
+	};
-- 
2.7.0

Re: [PATCH V2 1/2] Documentation: dt: mailbox: Add TI Message Manager

[Rob Herring]

On Fri, Feb 26, 2016 at 04:24:11PM -0600, Nishanth Menon wrote:
> Message Manager is a hardware block used to communicate with various
> processor systems within certain Texas Instrument's Keystone
> generation SoCs.
> 
> This hardware engine is used to transfer messages from various compute
> entities(or processors) within the SoC. It is designed to be self
> contained without needing software initialization for operation.
> 
> Signed-off-by: Nishanth Menon <nm-l0cyMroinI0@public.gmane.org>
> ---
> Changes in V2: major refactor of binding - Thanks for all the review from Rob and Jassi
> 	- Generic compatible property is dropped
> 	- no more child nodes, instead interrupt information is provided as part
> 	  of the the message manager node it self - part of data moved into
> 	  driver based on compatible property.
> 	- mbox-cells is 2 (passes queue_id and proxy_id) -added elaboration as well
> 	- change in interrupt names
> 
> V1: https://patchwork.kernel.org/patch/8237111/
> 
>  .../bindings/mailbox/ti,message-manager.txt        | 50 ++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/ti,message-manager.txt

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2 2/2] mailbox: Introduce TI message manager driver

[Nishanth Menon]

Support for TI Message Manager Module. This hardware block manages a
bunch of hardware queues meant for communication between processor
entities.

Clients sitting on top of this would manage the required protocol
for communicating with the counterpart entities.

For more details on TI Message Manager hardware block, see documentation
that will is available here: http://www.ti.com/lit/ug/spruhy8/spruhy8.pdf
Chapter 8.1(Message Manager)

Signed-off-by: Nishanth Menon <nm@ti.com>
---
V2: Changes since V1:
	- Major refactoring of code for binding changes
	- moved list of valid K2G queueus into driver
	- split up probe into sub functions for easier maintenance

V1: https://patchwork.kernel.org/patch/8237171/

 drivers/mailbox/Kconfig     |  11 +
 drivers/mailbox/Makefile    |   2 +
 drivers/mailbox/ti-msgmgr.c | 657 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/ti-msgmgr.h   |  35 +++
 4 files changed, 705 insertions(+)
 create mode 100644 drivers/mailbox/ti-msgmgr.c
 create mode 100644 include/linux/ti-msgmgr.h

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 546d05f4358a..5da85ae48a59 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -78,6 +78,17 @@ config STI_MBOX
 	  Mailbox implementation for STMicroelectonics family chips with
 	  hardware for interprocessor communication.
 
+config TI_MESSAGE_MANAGER
+	tristate "Texas Instruments Message Manager Driver"
+	depends on ARCH_KEYSTONE
+	help
+	  An implementation of Message Manager slave driver for Keystone
+	  architecture SoCs from Texas Instruments. Message Manager is a
+	  communication entity found on few of Texas Instrument's keystone
+	  architecture SoCs. These may be used for communication between
+	  multiple processors within the SoC. Select this driver if your
+	  platform has support for the hardware block.
+
 config MAILBOX_TEST
 	tristate "Mailbox Test Client"
 	depends on OF
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 92435ef11f26..a9c2899e44ff 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -17,3 +17,5 @@ obj-$(CONFIG_ALTERA_MBOX)	+= mailbox-altera.o
 obj-$(CONFIG_BCM2835_MBOX)	+= bcm2835-mailbox.o
 
 obj-$(CONFIG_STI_MBOX)		+= mailbox-sti.o
+
+obj-$(CONFIG_TI_MESSAGE_MANAGER) += ti-msgmgr.o
diff --git a/drivers/mailbox/ti-msgmgr.c b/drivers/mailbox/ti-msgmgr.c
new file mode 100644
index 000000000000..1f4afa11ba88
--- /dev/null
+++ b/drivers/mailbox/ti-msgmgr.c
@@ -0,0 +1,657 @@
+/*
+ * Texas Instruments' Message Manager Driver
+ *
+ * Copyright (C) 2015-2016 Texas Instruments Incorporated - http://www.ti.com/
+ *	Nishanth Menon
+ *
+ * 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.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#define pr_fmt(fmt) "%s: " fmt, __func__
+
+#include <linux/device.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_device.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/ti-msgmgr.h>
+
+#define Q_DATA_OFFSET(proxy, queue, reg)	\
+		     ((0x10000 * (proxy)) + (0x80 * (queue)) + ((reg) * 4))
+#define Q_STATE_OFFSET(queue)			((queue) * 0x4)
+#define Q_STATE_ENTRY_COUNT_MASK		(0xFFF000)
+
+/**
+ * struct ti_msgmgr_valid_queue_desc - SoC valid queues meant for this processor
+ * @queue_id:	Queue Number for this path
+ * @proxy_id:	Proxy ID representing the processor in SoC
+ * @is_tx:	Is this a receive path?
+ */
+struct ti_msgmgr_valid_queue_desc {
+	u8 queue_id;
+	u8 proxy_id;
+	bool is_tx;
+};
+
+/**
+ * struct ti_msgmgr_desc - Description of message manager integration
+ * @queue_count:	Number of Queues
+ * @max_message_size:	Message size in bytes
+ * @max_messages:	Number of messages
+ * @q_slices:		Number of queue engines
+ * @q_proxies:		Number of queue proxies per page
+ * @data_first_reg:	First data register for proxy data region
+ * @data_last_reg:	Last data register for proxy data region
+ * @tx_polled:		Do I need to use polled mechanism for tx
+ * @tx_poll_timeout_ms: Timeout in ms if polled
+ * @valid_queues:	List of Valid queues that the processor can access
+ * @num_valid_queues:	Number of valid queues
+ *
+ * This structure is used in of match data to describe how integration
+ * for a specific compatible SoC is done.
+ */
+struct ti_msgmgr_desc {
+	u8 queue_count;
+	u8 max_message_size;
+	u8 max_messages;
+	u8 q_slices;
+	u8 q_proxies;
+	u8 data_first_reg;
+	u8 data_last_reg;
+	bool tx_polled;
+	int tx_poll_timeout_ms;
+	const struct ti_msgmgr_valid_queue_desc *valid_queues;
+	int num_valid_queues;
+};
+
+/**
+ * struct ti_queue_inst - Description of a queue instance
+ * @name:	Queue Name
+ * @queue_id:	Queue Identifier as mapped on SoC
+ * @proxy_id:	Proxy Identifier as mapped on SoC
+ * @irq:	IRQ for Rx Queue
+ * @is_tx:	'true' if transmit queue, else, 'false'
+ * @queue_buff_start: First register of Data Buffer
+ * @queue_buff_end: Last (or confirmation) register of Data buffer
+ * @queue_state: Queue status register
+ * @chan:	Mailbox channel
+ * @rx_buff:	Receive buffer pointer allocated at probe, max_message_size
+ */
+struct ti_queue_inst {
+	char name[30];
+	u8 queue_id;
+	u8 proxy_id;
+	int irq;
+	bool is_tx;
+	void __iomem *queue_buff_start;
+	void __iomem *queue_buff_end;
+	void __iomem *queue_state;
+	struct mbox_chan *chan;
+	u32 *rx_buff;
+};
+
+/**
+ * struct ti_msgmgr_inst - Description of a Message Manager Instance
+ * @dev:	device pointer corresponding to the Message Manager instance
+ * @desc:	Description of the SoC integration
+ * @queue_proxy_region:	Queue proxy region where queue buffers are located
+ * @queue_state_debug_region:	Queue status register regions
+ * @num_valid_queues:	Number of valid queues defined for the processor
+ *		Note: other queues are probably reserved for other processors
+ *		in the SoC.
+ * @qinsts:	Array of valid Queue Instances for the Processor
+ * @mbox:	Mailbox Controller
+ * @chans:	Array for channels corresponding to the Queue Instances.
+ */
+struct ti_msgmgr_inst {
+	struct device *dev;
+	const struct ti_msgmgr_desc *desc;
+	void __iomem *queue_proxy_region;
+	void __iomem *queue_state_debug_region;
+	u8 num_valid_queues;
+	struct ti_queue_inst *qinsts;
+	struct mbox_controller mbox;
+	struct mbox_chan *chans;
+};
+
+/**
+ * ti_msgmgr_queue_get_num_messages() - Get the number of pending messages
+ * @qinst:	Queue instance for which we check the number of pending messages
+ *
+ * Return: number of messages pending in the queue (0 == no pending messages)
+ */
+static inline int ti_msgmgr_queue_get_num_messages(struct ti_queue_inst *qinst)
+{
+	u32 val;
+
+	/*
+	 * We cannot use relaxed operation here - update may happen
+	 * real-time.
+	 */
+	val = readl(qinst->queue_state) & Q_STATE_ENTRY_COUNT_MASK;
+	val >>= __ffs(Q_STATE_ENTRY_COUNT_MASK);
+
+	return val;
+}
+
+/**
+ * ti_msgmgr_queue_rx_interrupt() - Interrupt handler for receive Queue
+ * @irq:	Interrupt number
+ * @p:		Channel Pointer
+ *
+ * Return: -EINVAL if there is no instance
+ * IRQ_NONE if the interrupt is not ours.
+ * IRQ_HANDLED if the rx interrupt was successfully handled.
+ */
+static irqreturn_t ti_msgmgr_queue_rx_interrupt(int irq, void *p)
+{
+	struct mbox_chan *chan = p;
+	struct device *dev = chan->mbox->dev;
+	struct ti_msgmgr_inst *inst = dev_get_drvdata(dev);
+	struct ti_queue_inst *qinst = chan->con_priv;
+	const struct ti_msgmgr_desc *desc;
+	int msg_count, num_words;
+	struct ti_msgmgr_message message;
+	void __iomem *data_reg;
+	u32 *word_data;
+
+	if (WARN_ON(!inst)) {
+		dev_err(dev, "no platform drv data??\n");
+		return -EINVAL;
+	}
+
+	/* Do I have an invalid interrupt source? */
+	if (qinst->is_tx) {
+		dev_err(dev, "Cannot handle rx interrupt on tx channel %s\n",
+			qinst->name);
+		return IRQ_NONE;
+	}
+
+	/* Do I actually have messages to read? */
+	msg_count = ti_msgmgr_queue_get_num_messages(qinst);
+	if (!msg_count) {
+		/* Shared IRQ? */
+		dev_dbg(dev, "Spurious event - 0 pending data!\n");
+		return IRQ_NONE;
+	}
+
+	/*
+	 * I have no idea about the protocol being used to communicate with the
+	 * remote producer - 0 could be valid data, so I wont make a judgement
+	 * of how many bytes I should be reading. Let the client figure this
+	 * out.. I just read the full message and pass it on..
+	 */
+	desc = inst->desc;
+	message.len = desc->max_message_size;
+	message.buf = (u8 *)qinst->rx_buff;
+
+	/*
+	 * NOTE about register access involved here:
+	 * the hardware block is implemented with 32bit access operations and no
+	 * support for data splitting.  We don't want the hardware to misbehave
+	 * with sub 32bit access - For example: if the last register read is
+	 * split into byte wise access, it can result in the queue getting
+	 * stuck or indeterminate behavior. An out of order read operation may
+	 * result in weird data results as well.
+	 * Hence, we do not use memcpy_fromio or __ioread32_copy here, instead
+	 * we depend on readl for the purpose.
+	 *
+	 * Also note that the final register read automatically marks the
+	 * queue message as read.
+	 */
+	for (data_reg = qinst->queue_buff_start, word_data = qinst->rx_buff,
+	     num_words = (desc->max_message_size / sizeof(u32));
+	     num_words; num_words--, data_reg += sizeof(u32), word_data++)
+		*word_data = readl(data_reg);
+
+	/*
+	 * Last register read automatically clears the IRQ if only 1 message
+	 * is pending - so send the data up the stack..
+	 * NOTE: Client is expected to be as optimal as possible, since
+	 * we invoke the handler in IRQ context.
+	 */
+	mbox_chan_received_data(chan, (void *)&message);
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * ti_msgmgr_queue_peek_data() - Peek to see if there are any rx messages.
+ * @chan:	Channel Pointer
+ *
+ * Return: 'true' if there is pending rx data, 'false' if there is none.
+ */
+static bool ti_msgmgr_queue_peek_data(struct mbox_chan *chan)
+{
+	struct ti_queue_inst *qinst = chan->con_priv;
+	int msg_count;
+
+	if (qinst->is_tx)
+		return false;
+
+	msg_count = ti_msgmgr_queue_get_num_messages(qinst);
+
+	return msg_count ? true : false;
+}
+
+/**
+ * ti_msgmgr_last_tx_done() - See if all the tx messages are sent
+ * @chan:	Channel pointer
+ *
+ * Return: 'true' is no pending tx data, 'false' if there are any.
+ */
+static bool ti_msgmgr_last_tx_done(struct mbox_chan *chan)
+{
+	struct ti_queue_inst *qinst = chan->con_priv;
+	int msg_count;
+
+	if (!qinst->is_tx)
+		return false;
+
+	msg_count = ti_msgmgr_queue_get_num_messages(qinst);
+
+	/* if we have any messages pending.. */
+	return msg_count ? false : true;
+}
+
+/**
+ * ti_msgmgr_send_data() - Send data
+ * @chan:	Channel Pointer
+ * @data:	ti_msgmgr_message * Message Pointer
+ *
+ * Return: 0 if all goes good, else appropriate error messages.
+ */
+static int ti_msgmgr_send_data(struct mbox_chan *chan, void *data)
+{
+	struct device *dev = chan->mbox->dev;
+	struct ti_msgmgr_inst *inst = dev_get_drvdata(dev);
+	const struct ti_msgmgr_desc *desc;
+	struct ti_queue_inst *qinst = chan->con_priv;
+	int msg_count, num_words, trail_bytes;
+	struct ti_msgmgr_message *message = data;
+	void __iomem *data_reg;
+	u32 *word_data;
+
+	if (WARN_ON(!inst)) {
+		dev_err(dev, "no platform drv data??\n");
+		return -EINVAL;
+	}
+	desc = inst->desc;
+
+	if (desc->max_message_size < message->len) {
+		dev_err(dev, "Queue %s message length %d > max %d\n",
+			qinst->name, message->len, desc->max_message_size);
+		return -EINVAL;
+	}
+
+	/* Are we able to send this or not? */
+	msg_count = ti_msgmgr_queue_get_num_messages(qinst);
+	if (msg_count >= desc->max_messages) {
+		dev_warn(dev, "Queue %s is full (%d messages)\n", qinst->name,
+			 msg_count);
+		return -EBUSY;
+	}
+
+	/*
+	 * NOTE about register access involved here:
+	 * the hardware block is implemented with 32bit access operations and no
+	 * support for data splitting.  We don't want the hardware to misbehave
+	 * with sub 32bit access - For example: if the last register write is
+	 * split into byte wise access, it can result in the queue getting
+	 * stuck or dummy messages being transmitted or indeterminate behavior.
+	 * The same can occur if out of order operations take place.
+	 * Hence, we do not use memcpy_toio or ___iowrite32_copy here, instead
+	 * we use writel which ensures the sequencing we need.
+	 */
+	for (data_reg = qinst->queue_buff_start,
+	     num_words = message->len / sizeof(u32),
+	     word_data = (u32 *)message->buf;
+	     num_words; num_words--, data_reg += sizeof(u32), word_data++)
+		writel(*word_data, data_reg);
+
+	trail_bytes = message->len % sizeof(u32);
+	if (trail_bytes) {
+		u32 data_trail = *word_data;
+
+		/* Ensure all unused data is 0 */
+		data_trail &= 0xFFFFFFFF >> (8 * (sizeof(u32) - trail_bytes));
+		writel(data_trail, data_reg);
+		data_reg++;
+	}
+	/*
+	 * 'data_reg' indicates next register to write. If we did not already
+	 * write on tx complete reg(last reg), we must do so for transmit
+	 */
+	if (data_reg <= qinst->queue_buff_end)
+		writel(0, qinst->queue_buff_end);
+
+	return 0;
+}
+
+/**
+ * ti_msgmgr_queue_startup() - Startup queue
+ * @chan:	Channel pointer
+ *
+ * Return: 0 if all goes good, else return corresponding error message
+ */
+static int ti_msgmgr_queue_startup(struct mbox_chan *chan)
+{
+	struct ti_queue_inst *qinst = chan->con_priv;
+	struct device *dev = chan->mbox->dev;
+	int ret;
+
+	if (!qinst->is_tx) {
+		/*
+		 * With the expectation that the IRQ might be shared in SoC
+		 */
+		ret = request_irq(qinst->irq, ti_msgmgr_queue_rx_interrupt,
+				  IRQF_SHARED, qinst->name, chan);
+		if (ret) {
+			dev_err(dev, "Unable to get IRQ %d on %s(res=%d)\n",
+				qinst->irq, qinst->name, ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * ti_msgmgr_queue_shutdown() - Shutdown the queue
+ * @chan:	Channel pointer
+ */
+static void ti_msgmgr_queue_shutdown(struct mbox_chan *chan)
+{
+	struct ti_queue_inst *qinst = chan->con_priv;
+
+	if (!qinst->is_tx)
+		free_irq(qinst->irq, chan);
+}
+
+/**
+ * ti_msgmgr_of_xlate() - Translation of phandle to queue
+ * @mbox:	Mailbox controller
+ * @p:		phandle pointer
+ *
+ * Return: Mailbox channel corresponding to the queue, else return error
+ * pointer.
+ */
+static struct mbox_chan *ti_msgmgr_of_xlate(struct mbox_controller *mbox,
+					    const struct of_phandle_args *p)
+{
+	struct ti_msgmgr_inst *inst;
+	int req_qid, req_pid;
+	struct ti_queue_inst *qinst;
+	int i;
+
+	inst = container_of(mbox, struct ti_msgmgr_inst, mbox);
+	if (WARN_ON(!inst))
+		return ERR_PTR(-EINVAL);
+
+	/* #mbox-cells is 2 */
+	if (p->args_count != 2) {
+		dev_err(inst->dev, "Invalid arguments in dt[%d] instead of 2\n",
+			p->args_count);
+		return ERR_PTR(-EINVAL);
+	}
+	req_qid = p->args[0];
+	req_pid = p->args[1];
+
+	for (qinst = inst->qinsts, i = 0; i < inst->num_valid_queues;
+	     i++, qinst++) {
+		if (req_qid == qinst->queue_id && req_pid == qinst->proxy_id)
+			return qinst->chan;
+	}
+
+	dev_err(inst->dev, "Queue ID %d, Proxy ID %d is wrong on %s\n",
+		req_qid, req_pid, p->np->name);
+	return ERR_PTR(-ENOENT);
+}
+
+/**
+ * ti_msgmgr_queue_setup() - Setup data structures for each queue instance
+ * @idx:	index of the queue
+ * @dev:	pointer to the message manager device
+ * @np:		pointer to the of node
+ * @inst:	Queue instance pointer
+ * @d:		Message Manager instance description data
+ * @qd:		Queue description data
+ * @qinst:	Queue instance pointer
+ * @chan:	pointer to mailbox channel
+ *
+ * Return: 0 if all went well, else return corresponding error
+ */
+static int ti_msgmgr_queue_setup(int idx, struct device *dev,
+				 struct device_node *np,
+				 struct ti_msgmgr_inst *inst,
+				 const struct ti_msgmgr_desc *d,
+				 const struct ti_msgmgr_valid_queue_desc *qd,
+				 struct ti_queue_inst *qinst,
+				 struct mbox_chan *chan)
+{
+	qinst->proxy_id = qd->proxy_id;
+	qinst->queue_id = qd->queue_id;
+
+	if (qinst->queue_id > d->queue_count) {
+		dev_err(dev, "Queue Data [idx=%d] queuid %d > %d\n",
+			idx, qinst->queue_id, d->queue_count);
+		return -ERANGE;
+	}
+
+	qinst->is_tx = qd->is_tx;
+	snprintf(qinst->name, sizeof(qinst->name), "%s %s_%03d_%03d",
+		 dev_name(dev), qinst->is_tx ? "tx" : "rx", qinst->queue_id,
+		 qinst->proxy_id);
+
+	if (!qinst->is_tx) {
+		char of_rx_irq_name[7];
+
+		snprintf(of_rx_irq_name, sizeof(of_rx_irq_name),
+			 "rx_%03d", qinst->queue_id);
+
+		qinst->irq = of_irq_get_byname(np, of_rx_irq_name);
+		if (qinst->irq < 0) {
+			dev_crit(dev,
+				 "[%d]QID %d PID %d:No IRQ[%s]: %d\n",
+				 idx, qinst->queue_id, qinst->proxy_id,
+				 of_rx_irq_name, qinst->irq);
+			return qinst->irq;
+		}
+		/* Allocate usage buffer for rx */
+		qinst->rx_buff = devm_kzalloc(dev,
+					      d->max_message_size, GFP_KERNEL);
+		if (!qinst->rx_buff)
+			return -ENOMEM;
+	}
+
+	qinst->queue_buff_start = inst->queue_proxy_region +
+	    Q_DATA_OFFSET(qinst->proxy_id, qinst->queue_id, d->data_first_reg);
+	qinst->queue_buff_end = inst->queue_proxy_region +
+	    Q_DATA_OFFSET(qinst->proxy_id, qinst->queue_id, d->data_last_reg);
+	qinst->queue_state = inst->queue_state_debug_region +
+	    Q_STATE_OFFSET(qinst->queue_id);
+	qinst->chan = chan;
+
+	chan->con_priv = qinst;
+
+	dev_dbg(dev, "[%d] qidx=%d pidx=%d irq=%d q_s=%p q_e = %p\n",
+		idx, qinst->queue_id, qinst->proxy_id, qinst->irq,
+		qinst->queue_buff_start, qinst->queue_buff_end);
+	return 0;
+}
+
+/* Queue operations */
+static const struct mbox_chan_ops ti_msgmgr_chan_ops = {
+	.startup = ti_msgmgr_queue_startup,
+	.shutdown = ti_msgmgr_queue_shutdown,
+	.peek_data = ti_msgmgr_queue_peek_data,
+	.last_tx_done = ti_msgmgr_last_tx_done,
+	.send_data = ti_msgmgr_send_data,
+};
+
+/* Keystone K2G SoC integration details */
+static const struct ti_msgmgr_valid_queue_desc k2g_valid_queues[] = {
+	{.queue_id = 0, .proxy_id = 0, .is_tx = true,},
+	{.queue_id = 1, .proxy_id = 0, .is_tx = true,},
+	{.queue_id = 2, .proxy_id = 0, .is_tx = true,},
+	{.queue_id = 3, .proxy_id = 0, .is_tx = true,},
+	{.queue_id = 5, .proxy_id = 2, .is_tx = false,},
+	{.queue_id = 56, .proxy_id = 1, .is_tx = true,},
+	{.queue_id = 57, .proxy_id = 2, .is_tx = false,},
+	{.queue_id = 58, .proxy_id = 3, .is_tx = true,},
+	{.queue_id = 59, .proxy_id = 4, .is_tx = true,},
+	{.queue_id = 60, .proxy_id = 5, .is_tx = true,},
+	{.queue_id = 61, .proxy_id = 6, .is_tx = true,},
+};
+
+static const struct ti_msgmgr_desc k2g_desc = {
+	.queue_count = 64,
+	.max_message_size = 64,
+	.max_messages = 128,
+	.q_slices = 1,
+	.q_proxies = 1,
+	.data_first_reg = 16,
+	.data_last_reg = 31,
+	.tx_polled = false,
+	.valid_queues = k2g_valid_queues,
+	.num_valid_queues = ARRAY_SIZE(k2g_valid_queues),
+};
+
+static const struct of_device_id ti_msgmgr_of_match[] = {
+	{.compatible = "ti,k2g-message-manager", .data = &k2g_desc},
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ti_msgmgr_of_match);
+
+static int ti_msgmgr_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct of_device_id *of_id;
+	struct device_node *np;
+	struct resource *res;
+	const struct ti_msgmgr_desc *desc;
+	struct ti_msgmgr_inst *inst;
+	struct ti_queue_inst *qinst;
+	struct mbox_controller *mbox;
+	struct mbox_chan *chans;
+	int queue_count;
+	int i;
+	int ret = -EINVAL;
+	const struct ti_msgmgr_valid_queue_desc *queue_desc;
+
+	if (!dev->of_node) {
+		dev_err(dev, "no OF information\n");
+		return -EINVAL;
+	}
+	np = dev->of_node;
+
+	of_id = of_match_device(ti_msgmgr_of_match, dev);
+	if (!of_id) {
+		dev_err(dev, "OF data missing\n");
+		return -EINVAL;
+	}
+	desc = of_id->data;
+
+	inst = devm_kzalloc(dev, sizeof(*inst), GFP_KERNEL);
+	if (!inst)
+		return -ENOMEM;
+
+	inst->dev = dev;
+	inst->desc = desc;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+					   "queue_proxy_region");
+	inst->queue_proxy_region = devm_ioremap_resource(dev, res);
+	if (IS_ERR(inst->queue_proxy_region))
+		return PTR_ERR(inst->queue_proxy_region);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+					   "queue_state_debug_region");
+	inst->queue_state_debug_region = devm_ioremap_resource(dev, res);
+	if (IS_ERR(inst->queue_state_debug_region))
+		return PTR_ERR(inst->queue_state_debug_region);
+
+	dev_dbg(dev, "proxy region=%p, queue_state=%p\n",
+		inst->queue_proxy_region, inst->queue_state_debug_region);
+
+	queue_count = desc->num_valid_queues;
+	if (!queue_count || queue_count > desc->queue_count) {
+		dev_crit(dev, "Invalid Number of queues %d. Max %d\n",
+			 queue_count, desc->queue_count);
+		return -ERANGE;
+	}
+	inst->num_valid_queues = queue_count;
+
+	qinst = devm_kzalloc(dev, sizeof(*qinst) * queue_count, GFP_KERNEL);
+	if (!qinst)
+		return -ENOMEM;
+	inst->qinsts = qinst;
+
+	chans = devm_kzalloc(dev, sizeof(*chans) * queue_count, GFP_KERNEL);
+	if (!chans)
+		return -ENOMEM;
+	inst->chans = chans;
+
+	for (i = 0, queue_desc = desc->valid_queues;
+	     i < queue_count; i++, qinst++, chans++, queue_desc++) {
+		ret = ti_msgmgr_queue_setup(i, dev, np, inst,
+					    desc, queue_desc, qinst, chans);
+		if (ret)
+			return ret;
+	}
+
+	mbox = &inst->mbox;
+	mbox->dev = dev;
+	mbox->ops = &ti_msgmgr_chan_ops;
+	mbox->chans = inst->chans;
+	mbox->num_chans = inst->num_valid_queues;
+	mbox->txdone_irq = false;
+	mbox->txdone_poll = desc->tx_polled;
+	if (desc->tx_polled)
+		mbox->txpoll_period = desc->tx_poll_timeout_ms;
+	mbox->of_xlate = ti_msgmgr_of_xlate;
+
+	platform_set_drvdata(pdev, inst);
+	ret = mbox_controller_register(mbox);
+	if (ret)
+		dev_err(dev, "Failed to register mbox_controller(%d)\n", ret);
+
+	return ret;
+}
+
+static int ti_msgmgr_remove(struct platform_device *pdev)
+{
+	struct ti_msgmgr_inst *inst;
+
+	inst = platform_get_drvdata(pdev);
+	mbox_controller_unregister(&inst->mbox);
+
+	return 0;
+}
+
+static struct platform_driver ti_msgmgr_driver = {
+	.probe = ti_msgmgr_probe,
+	.remove = ti_msgmgr_remove,
+	.driver = {
+		   .name = "ti-msgmgr",
+		   .of_match_table = of_match_ptr(ti_msgmgr_of_match),
+	},
+};
+module_platform_driver(ti_msgmgr_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("TI message manager driver");
+MODULE_AUTHOR("Nishanth Menon");
+MODULE_ALIAS("platform:ti-msgmgr");
diff --git a/include/linux/ti-msgmgr.h b/include/linux/ti-msgmgr.h
new file mode 100644
index 000000000000..eac8e0c6fe11
--- /dev/null
+++ b/include/linux/ti-msgmgr.h
@@ -0,0 +1,35 @@
+/*
+ * Texas Instruments' Message Manager
+ *
+ * Copyright (C) 2015-2016 Texas Instruments Incorporated - http://www.ti.com/
+ *	Nishanth Menon
+ *
+ * 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.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef TI_MSGMGR_H
+#define TI_MSGMGR_H
+
+/**
+ * struct ti_msgmgr_message - Message Manager structure
+ * @len: Length of data in the Buffer
+ * @buf: Buffer pointer
+ *
+ * This is the structure for data used in mbox_send_message
+ * the length of data buffer used depends on the SoC integration
+ * parameters - each message may be 64, 128 bytes long depending
+ * on SoC. Client is supposed to be aware of this.
+ */
+struct ti_msgmgr_message {
+	size_t len;
+	u8 *buf;
+};
+
+#endif /* TI_MSGMGR_H */
-- 
2.7.0

Re: [PATCH V2 2/2] mailbox: Introduce TI message manager driver

[Jassi Brar]

On Sat, Feb 27, 2016 at 3:54 AM, Nishanth Menon <nm@ti.com> wrote:
> Support for TI Message Manager Module. This hardware block manages a
> bunch of hardware queues meant for communication between processor
> entities.
>
> Clients sitting on top of this would manage the required protocol
> for communicating with the counterpart entities.
>
> For more details on TI Message Manager hardware block, see documentation
> that will is available here: http://www.ti.com/lit/ug/spruhy8/spruhy8.pdf
> Chapter 8.1(Message Manager)
>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
> V2: Changes since V1:
>         - Major refactoring of code for binding changes
>         - moved list of valid K2G queueus into driver
>         - split up probe into sub functions for easier maintenance
>
> V1: https://patchwork.kernel.org/patch/8237171/
>
>  drivers/mailbox/Kconfig     |  11 +
>  drivers/mailbox/Makefile    |   2 +
>  drivers/mailbox/ti-msgmgr.c | 657
Do you want to call it something more specific than 'msgmgr from TI'?
Maybe its about Keystone, like mailbox-keystone.c ?


> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/ti-msgmgr.h>
>
This seems a bit bold. I think  include/linux/soc/ti/ is the right place.


> +       /* Do I actually have messages to read? */
> +       msg_count = ti_msgmgr_queue_get_num_messages(qinst);
> +       if (!msg_count) {
> +               /* Shared IRQ? */
> +               dev_dbg(dev, "Spurious event - 0 pending data!\n");
> +               return IRQ_NONE;
> +       }
> +
> +       /*
> +        * I have no idea about the protocol being used to communicate with the
> +        * remote producer - 0 could be valid data, so I wont make a judgement
> +        * of how many bytes I should be reading. Let the client figure this
> +        * out.. I just read the full message and pass it on..
> +        */
Exactly. And similarly when you send data, you should not have more
than one message in transit. Now please see my note in
ti_msgmgr_send_data()


> +static int ti_msgmgr_send_data(struct mbox_chan *chan, void *data)
> +{
> +       struct device *dev = chan->mbox->dev;
> +       struct ti_msgmgr_inst *inst = dev_get_drvdata(dev);
> +       const struct ti_msgmgr_desc *desc;
> +       struct ti_queue_inst *qinst = chan->con_priv;
> +       int msg_count, num_words, trail_bytes;
> +       struct ti_msgmgr_message *message = data;
> +       void __iomem *data_reg;
> +       u32 *word_data;
> +
> +       if (WARN_ON(!inst)) {
> +               dev_err(dev, "no platform drv data??\n");
> +               return -EINVAL;
> +       }
> +       desc = inst->desc;
> +
> +       if (desc->max_message_size < message->len) {
> +               dev_err(dev, "Queue %s message length %d > max %d\n",
> +                       qinst->name, message->len, desc->max_message_size);
> +               return -EINVAL;
> +       }
> +
> +       /* Are we able to send this or not? */
> +       msg_count = ti_msgmgr_queue_get_num_messages(qinst);
> +       if (msg_count >= desc->max_messages) {
> +               dev_warn(dev, "Queue %s is full (%d messages)\n", qinst->name,
> +                        msg_count);
> +               return -EBUSY;
> +       }
This seems fishy. mailbox api always submit 1 'complete' message to be
sent and checks for completion by last_tx_done() before calling
send_data() again. Controller drivers are not supposed to queue
messages - mailbox core does. So you should never be unable to send a
message.


> +       /*
> +        * NOTE about register access involved here:
> +        * the hardware block is implemented with 32bit access operations and no
> +        * support for data splitting.  We don't want the hardware to misbehave
> +        * with sub 32bit access - For example: if the last register write is
> +        * split into byte wise access, it can result in the queue getting
> +        * stuck or dummy messages being transmitted or indeterminate behavior.
> +        * The same can occur if out of order operations take place.
> +        * Hence, we do not use memcpy_toio or ___iowrite32_copy here, instead
> +        * we use writel which ensures the sequencing we need.
> +        */
.... deja-vu ?


> +
> +/* Keystone K2G SoC integration details */
> +static const struct ti_msgmgr_valid_queue_desc k2g_valid_queues[] = {
> +       {.queue_id = 0, .proxy_id = 0, .is_tx = true,},
> +       {.queue_id = 1, .proxy_id = 0, .is_tx = true,},
> +       {.queue_id = 2, .proxy_id = 0, .is_tx = true,},
> +       {.queue_id = 3, .proxy_id = 0, .is_tx = true,},
> +       {.queue_id = 5, .proxy_id = 2, .is_tx = false,},
> +       {.queue_id = 56, .proxy_id = 1, .is_tx = true,},
> +       {.queue_id = 57, .proxy_id = 2, .is_tx = false,},
> +       {.queue_id = 58, .proxy_id = 3, .is_tx = true,},
> +       {.queue_id = 59, .proxy_id = 4, .is_tx = true,},
> +       {.queue_id = 60, .proxy_id = 5, .is_tx = true,},
> +       {.queue_id = 61, .proxy_id = 6, .is_tx = true,},
> +};
> +
> +static const struct ti_msgmgr_desc k2g_desc = {
> +       .queue_count = 64,
> +       .max_message_size = 64,
> +       .max_messages = 128,
> +       .q_slices = 1,
> +       .q_proxies = 1,
> +       .data_first_reg = 16,
> +       .data_last_reg = 31,
> +       .tx_polled = false,
> +       .valid_queues = k2g_valid_queues,
> +       .num_valid_queues = ARRAY_SIZE(k2g_valid_queues),
> +};
If these parameters are very configurable, maybe they should be in DT?

Thanks.

Re: [PATCH V2 2/2] mailbox: Introduce TI message manager driver

[Nishanth Menon]

Hi Jassi,

Thanks for reviewing the patch.
On 03/03/2016 11:18 PM, Jassi Brar wrote:

[...]

>>
>>  drivers/mailbox/Kconfig     |  11 +
>>  drivers/mailbox/Makefile    |   2 +
>>  drivers/mailbox/ti-msgmgr.c | 657

> Do you want to call it something more specific than 'msgmgr from TI'?
> Maybe its about Keystone, like mailbox-keystone.c ?

Here is the rationale for choosing the name:
There are more than 1 IPC in TI SoCs and even within Keystone SoC.
Further, it is indeed called message manager hardware block and used
across multiple SoC families (even though it is originated by keystone
architecture). we do have a reuse of the omap-mailbox in a future
keystone device (in addition to message manager), so using ti-mailbox is
more appropriate for omap-mailbox, but anyways.. further renaming this
as keystone-mailbox will confuse poor users when the new SoC comes in..

We do have a lot of cross pollination of hardware blocks across TI
architectures, and "keystone-" prefix does not really fit the usage.
hence the usage of vendor-hardwareblock name.

> 
> 
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/ti-msgmgr.h>
>>
> This seems a bit bold. I think  include/linux/soc/ti/ is the right place.

Sure, I can. Since I followed include/linux/omap-mailbox.h, Would you
suggest all files such as include/linux/omap* be moved to
include/linux/soc/ti/ ? I guess that is not pertinent to this specific
patch, but I am curious..


>> +       /* Do I actually have messages to read? */
>> +       msg_count = ti_msgmgr_queue_get_num_messages(qinst);
>> +       if (!msg_count) {
>> +               /* Shared IRQ? */
>> +               dev_dbg(dev, "Spurious event - 0 pending data!\n");
>> +               return IRQ_NONE;
>> +       }
>> +
>> +       /*
>> +        * I have no idea about the protocol being used to communicate with the
>> +        * remote producer - 0 could be valid data, so I wont make a judgement
>> +        * of how many bytes I should be reading. Let the client figure this
>> +        * out.. I just read the full message and pass it on..
>> +        */
> Exactly. And similarly when you send data, you should not have more
> than one message in transit. Now please see my note in
> ti_msgmgr_send_data()
> 
> 
>> +static int ti_msgmgr_send_data(struct mbox_chan *chan, void *data)
>> +{
>> +       struct device *dev = chan->mbox->dev;
>> +       struct ti_msgmgr_inst *inst = dev_get_drvdata(dev);
>> +       const struct ti_msgmgr_desc *desc;
>> +       struct ti_queue_inst *qinst = chan->con_priv;
>> +       int msg_count, num_words, trail_bytes;
>> +       struct ti_msgmgr_message *message = data;
>> +       void __iomem *data_reg;
>> +       u32 *word_data;
>> +
>> +       if (WARN_ON(!inst)) {
>> +               dev_err(dev, "no platform drv data??\n");
>> +               return -EINVAL;
>> +       }
>> +       desc = inst->desc;
>> +
>> +       if (desc->max_message_size < message->len) {
>> +               dev_err(dev, "Queue %s message length %d > max %d\n",
>> +                       qinst->name, message->len, desc->max_message_size);
>> +               return -EINVAL;
>> +       }
>> +
>> +       /* Are we able to send this or not? */
>> +       msg_count = ti_msgmgr_queue_get_num_messages(qinst);
>> +       if (msg_count >= desc->max_messages) {
>> +               dev_warn(dev, "Queue %s is full (%d messages)\n", qinst->name,
>> +                        msg_count);
>> +               return -EBUSY;
>> +       }
> This seems fishy. mailbox api always submit 1 'complete' message to be
> sent and checks for completion by last_tx_done() before calling
> send_data() again. Controller drivers are not supposed to queue
> messages - mailbox core does. So you should never be unable to send a
> message.


OK-> to explain this, few reasons: (queue messages check and usage of
last_tx_done are kind of intertwined answer..
a) we need to remember that the message manager has a shared RAM.
multiple transmitter over other queues can be sharing the same.
unfortunately, we dont get a threshold kind of interrupt or status that
I am able to exploit in the current incarnation of the solution. The
best we can do in the full system is to constrain the number of messages
that are max pending simultaneously in each of the queue from various
transmitters in the SoC.
b) last_tx_done() is checked if TXDONE_BY_POLL, not if TXDONE_BY_ACK
right? which is how this'd work since txdone_poll is false -> that is
how we want this mechanism to work once the far end is ready for next
message, it acks. I do see your point about being tied to protocol - I
dont like it either.. in fact, I'd prefer that client registration
mention what kind of handshaking is necessary, but: a) that is not how
mailbox framework is constructed at the moment(we state txdone_poll at
mailbox registration, not at client usage) and b) I have no real need
for multiple clients to users of message manager who actually need
non-ACK usage - even for the foreseeable future (at least 1 next
generation of SoC) - if such a need does arise in the future, I will
have to rework framework and make this capability at the registration
time of the client - allowing each client path to use different
mechanisms on hardware such as these that need it.
c) message manager can actually queue more than one message(depending on
client capability). Even though, at this point, we are not really
capable of doing it(again from what I can see for immediate future),
mailbox framework by checking last_tx_done forces a single message
sequencing - which is not really exploiting the capability of the
hardware - in theory, we should be able to queue max num messages, hit
cpuidle and snooze away while the remote entity chomp away data at it's
own pace and finally give us a notification back - but again, we can
argue it is indeed protocol dependent, so setting txdone_poll to false
actually enables that to be done in user. Again - i have no immediate
need for any queued multiple transfer needs yet.. even if i need to, in
the future, it can easily be done by the client by maintaining code as
is - txdone_poll is false.

> 
> 
>> +       /*
>> +        * NOTE about register access involved here:
>> +        * the hardware block is implemented with 32bit access operations and no
>> +        * support for data splitting.  We don't want the hardware to misbehave
>> +        * with sub 32bit access - For example: if the last register write is
>> +        * split into byte wise access, it can result in the queue getting
>> +        * stuck or dummy messages being transmitted or indeterminate behavior.
>> +        * The same can occur if out of order operations take place.
>> +        * Hence, we do not use memcpy_toio or ___iowrite32_copy here, instead
>> +        * we use writel which ensures the sequencing we need.
>> +        */
> .... deja-vu ?

Tell me about it.. but then, surprises like these do pop in once in a
while.. sigh..

>> +
>> +/* Keystone K2G SoC integration details */
>> +static const struct ti_msgmgr_valid_queue_desc k2g_valid_queues[] = {
>> +       {.queue_id = 0, .proxy_id = 0, .is_tx = true,},
>> +       {.queue_id = 1, .proxy_id = 0, .is_tx = true,},
>> +       {.queue_id = 2, .proxy_id = 0, .is_tx = true,},
>> +       {.queue_id = 3, .proxy_id = 0, .is_tx = true,},
>> +       {.queue_id = 5, .proxy_id = 2, .is_tx = false,},
>> +       {.queue_id = 56, .proxy_id = 1, .is_tx = true,},
>> +       {.queue_id = 57, .proxy_id = 2, .is_tx = false,},
>> +       {.queue_id = 58, .proxy_id = 3, .is_tx = true,},
>> +       {.queue_id = 59, .proxy_id = 4, .is_tx = true,},
>> +       {.queue_id = 60, .proxy_id = 5, .is_tx = true,},
>> +       {.queue_id = 61, .proxy_id = 6, .is_tx = true,},
>> +};
>> +
>> +static const struct ti_msgmgr_desc k2g_desc = {
>> +       .queue_count = 64,
>> +       .max_message_size = 64,
>> +       .max_messages = 128,
>> +       .q_slices = 1,
>> +       .q_proxies = 1,
>> +       .data_first_reg = 16,
>> +       .data_last_reg = 31,
>> +       .tx_polled = false,
>> +       .valid_queues = k2g_valid_queues,
>> +       .num_valid_queues = ARRAY_SIZE(k2g_valid_queues),
>> +};
> If these parameters are very configurable, maybe they should be in DT?

These are SoC integration specific data. based on DT, we will only have
SoC specific compatible property in DT. Since  the data is tied to SoC
integration, there is no benefit of keeping these in DT.

-- 
Regards,
Nishanth Menon

Re: [PATCH V2 2/2] mailbox: Introduce TI message manager driver

[Jassi Brar]

On Fri, Mar 4, 2016 at 8:05 PM, Nishanth Menon <nm-l0cyMroinI0@public.gmane.org> wrote:
> Hi Jassi,
>
> Thanks for reviewing the patch.
> On 03/03/2016 11:18 PM, Jassi Brar wrote:
>
> [...]
>
>>>
>>>  drivers/mailbox/Kconfig     |  11 +
>>>  drivers/mailbox/Makefile    |   2 +
>>>  drivers/mailbox/ti-msgmgr.c | 657
>
>> Do you want to call it something more specific than 'msgmgr from TI'?
>> Maybe its about Keystone, like mailbox-keystone.c ?
>
> Here is the rationale for choosing the name:
> There are more than 1 IPC in TI SoCs and even within Keystone SoC.
> Further, it is indeed called message manager hardware block and used
> across multiple SoC families (even though it is originated by keystone
> architecture). we do have a reuse of the omap-mailbox in a future
> keystone device (in addition to message manager), so using ti-mailbox is
> more appropriate for omap-mailbox, but anyways.. further renaming this
> as keystone-mailbox will confuse poor users when the new SoC comes in..
>
> We do have a lot of cross pollination of hardware blocks across TI
> architectures, and "keystone-" prefix does not really fit the usage.
> hence the usage of vendor-hardwareblock name.
>
OK, I leave that to you to call it whatever you think is right.

>>
>>> +#include <linux/module.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_irq.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/ti-msgmgr.h>
>>>
>> This seems a bit bold. I think  include/linux/soc/ti/ is the right place.
>
> Sure, I can. Since I followed c, Would you
> suggest all files such as include/linux/omap* be moved to
> include/linux/soc/ti/ ? I guess that is not pertinent to this specific
> patch, but I am curious..
>
include/linux/omap-mailbox.h predates mailbox api.
But yes, I do think include/linux is not the right place for platform
specific headers. include/linux/soc/ is.

>
>>> +       /* Do I actually have messages to read? */
>>> +       msg_count = ti_msgmgr_queue_get_num_messages(qinst);
>>> +       if (!msg_count) {
>>> +               /* Shared IRQ? */
>>> +               dev_dbg(dev, "Spurious event - 0 pending data!\n");
>>> +               return IRQ_NONE;
>>> +       }
>>> +
>>> +       /*
>>> +        * I have no idea about the protocol being used to communicate with the
>>> +        * remote producer - 0 could be valid data, so I wont make a judgement
>>> +        * of how many bytes I should be reading. Let the client figure this
>>> +        * out.. I just read the full message and pass it on..
>>> +        */
>> Exactly. And similarly when you send data, you should not have more
>> than one message in transit. Now please see my note in
>> ti_msgmgr_send_data()
>>
>>
>>> +static int ti_msgmgr_send_data(struct mbox_chan *chan, void *data)
>>> +{
>>> +       struct device *dev = chan->mbox->dev;
>>> +       struct ti_msgmgr_inst *inst = dev_get_drvdata(dev);
>>> +       const struct ti_msgmgr_desc *desc;
>>> +       struct ti_queue_inst *qinst = chan->con_priv;
>>> +       int msg_count, num_words, trail_bytes;
>>> +       struct ti_msgmgr_message *message = data;
>>> +       void __iomem *data_reg;
>>> +       u32 *word_data;
>>> +
>>> +       if (WARN_ON(!inst)) {
>>> +               dev_err(dev, "no platform drv data??\n");
>>> +               return -EINVAL;
>>> +       }
>>> +       desc = inst->desc;
>>> +
>>> +       if (desc->max_message_size < message->len) {
>>> +               dev_err(dev, "Queue %s message length %d > max %d\n",
>>> +                       qinst->name, message->len, desc->max_message_size);
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       /* Are we able to send this or not? */
>>> +       msg_count = ti_msgmgr_queue_get_num_messages(qinst);
>>> +       if (msg_count >= desc->max_messages) {
>>> +               dev_warn(dev, "Queue %s is full (%d messages)\n", qinst->name,
>>> +                        msg_count);
>>> +               return -EBUSY;
>>> +       }
>> This seems fishy. mailbox api always submit 1 'complete' message to be
>> sent and checks for completion by last_tx_done() before calling
>> send_data() again. Controller drivers are not supposed to queue
>> messages - mailbox core does. So you should never be unable to send a
>> message.
>
>
> OK-> to explain this, few reasons: (queue messages check and usage of
> last_tx_done are kind of intertwined answer..
> a) we need to remember that the message manager has a shared RAM.
> multiple transmitter over other queues can be sharing the same.
> unfortunately, we dont get a threshold kind of interrupt or status that
> I am able to exploit in the current incarnation of the solution. The
> best we can do in the full system is to constrain the number of messages
> that are max pending simultaneously in each of the queue from various
> transmitters in the SoC.
> b) last_tx_done() is checked if TXDONE_BY_POLL, not if TXDONE_BY_ACK
> right? which is how this'd work since txdone_poll is false -> that is
> how we want this mechanism to work once the far end is ready for next
> message, it acks. I do see your point about being tied to protocol - I
> dont like it either.. in fact, I'd prefer that client registration
> mention what kind of handshaking is necessary, but: a) that is not how
> mailbox framework is constructed at the moment(we state txdone_poll at
> mailbox registration, not at client usage) and b) I have no real need
> for multiple clients to users of message manager who actually need
> non-ACK usage - even for the foreseeable future (at least 1 next
> generation of SoC) - if such a need does arise in the future, I will
> have to rework framework and make this capability at the registration
> time of the client - allowing each client path to use different
> mechanisms on hardware such as these that need it.
> c) message manager can actually queue more than one message(depending on
> client capability). Even though, at this point, we are not really
> capable of doing it(again from what I can see for immediate future),
> mailbox framework by checking last_tx_done forces a single message
> sequencing - which is not really exploiting the capability of the
> hardware - in theory, we should be able to queue max num messages, hit
> cpuidle and snooze away while the remote entity chomp away data at it's
> own pace and finally give us a notification back - but again, we can
> argue it is indeed protocol dependent, so setting txdone_poll to false
> actually enables that to be done in user. Again - i have no immediate
> need for any queued multiple transfer needs yet.. even if i need to, in
> the future, it can easily be done by the client by maintaining code as
> is - txdone_poll is false.
>
All I suggest is that the controller does not queue more than 1
message at a time, which means the controller driver allows for
maximum possible resources taken by a message.
The buffering is already done by the core, and if for your 'batch
dispatch' thing the client could simply flush them to remote by
pretending it got the ack (which is no worse than simply sending all
messages to remote without caring if the first was successful or not).


>>
>>> +       /*
>>> +        * NOTE about register access involved here:
>>> +        * the hardware block is implemented with 32bit access operations and no
>>> +        * support for data splitting.  We don't want the hardware to misbehave
>>> +        * with sub 32bit access - For example: if the last register write is
>>> +        * split into byte wise access, it can result in the queue getting
>>> +        * stuck or dummy messages being transmitted or indeterminate behavior.
>>> +        * The same can occur if out of order operations take place.
>>> +        * Hence, we do not use memcpy_toio or ___iowrite32_copy here, instead
>>> +        * we use writel which ensures the sequencing we need.
>>> +        */
>> .... deja-vu ?
>
> Tell me about it.. but then, surprises like these do pop in once in a
> while.. sigh..
>
I meant you have same para for read() , so maybe omit this one.

>>> +
>>> +/* Keystone K2G SoC integration details */
>>> +static const struct ti_msgmgr_valid_queue_desc k2g_valid_queues[] = {
>>> +       {.queue_id = 0, .proxy_id = 0, .is_tx = true,},
>>> +       {.queue_id = 1, .proxy_id = 0, .is_tx = true,},
>>> +       {.queue_id = 2, .proxy_id = 0, .is_tx = true,},
>>> +       {.queue_id = 3, .proxy_id = 0, .is_tx = true,},
>>> +       {.queue_id = 5, .proxy_id = 2, .is_tx = false,},
>>> +       {.queue_id = 56, .proxy_id = 1, .is_tx = true,},
>>> +       {.queue_id = 57, .proxy_id = 2, .is_tx = false,},
>>> +       {.queue_id = 58, .proxy_id = 3, .is_tx = true,},
>>> +       {.queue_id = 59, .proxy_id = 4, .is_tx = true,},
>>> +       {.queue_id = 60, .proxy_id = 5, .is_tx = true,},
>>> +       {.queue_id = 61, .proxy_id = 6, .is_tx = true,},
>>> +};
>>> +
>>> +static const struct ti_msgmgr_desc k2g_desc = {
>>> +       .queue_count = 64,
>>> +       .max_message_size = 64,
>>> +       .max_messages = 128,
>>> +       .q_slices = 1,
>>> +       .q_proxies = 1,
>>> +       .data_first_reg = 16,
>>> +       .data_last_reg = 31,
>>> +       .tx_polled = false,
>>> +       .valid_queues = k2g_valid_queues,
>>> +       .num_valid_queues = ARRAY_SIZE(k2g_valid_queues),
>>> +};
>> If these parameters are very configurable, maybe they should be in DT?
>
> These are SoC integration specific data. based on DT, we will only have
> SoC specific compatible property in DT. Since  the data is tied to SoC
> integration, there is no benefit of keeping these in DT.
>
Well, yes if the numbers don't change with very often.

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

Re: [PATCH V2 2/2] mailbox: Introduce TI message manager driver

[Nishanth Menon]

On 03/07/2016 12:31 PM, Jassi Brar wrote:
> On Fri, Mar 4, 2016 at 8:05 PM, Nishanth Menon <nm@ti.com> wrote:
>> Hi Jassi,
>>
>> Thanks for reviewing the patch.
>> On 03/03/2016 11:18 PM, Jassi Brar wrote:
>>
>> [...]
>>
>>>>
>>>>  drivers/mailbox/Kconfig     |  11 +
>>>>  drivers/mailbox/Makefile    |   2 +
>>>>  drivers/mailbox/ti-msgmgr.c | 657
>>
>>> Do you want to call it something more specific than 'msgmgr from TI'?
>>> Maybe its about Keystone, like mailbox-keystone.c ?
>>
>> Here is the rationale for choosing the name:
>> There are more than 1 IPC in TI SoCs and even within Keystone SoC.
>> Further, it is indeed called message manager hardware block and used
>> across multiple SoC families (even though it is originated by keystone
>> architecture). we do have a reuse of the omap-mailbox in a future
>> keystone device (in addition to message manager), so using ti-mailbox is
>> more appropriate for omap-mailbox, but anyways.. further renaming this
>> as keystone-mailbox will confuse poor users when the new SoC comes in..
>>
>> We do have a lot of cross pollination of hardware blocks across TI
>> architectures, and "keystone-" prefix does not really fit the usage.
>> hence the usage of vendor-hardwareblock name.
>>
> OK, I leave that to you to call it whatever you think is right.

thanks.

> 
>>>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_irq.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/ti-msgmgr.h>
>>>>
>>> This seems a bit bold. I think  include/linux/soc/ti/ is the right place.
>>
>> Sure, I can. Since I followed c, Would you
>> suggest all files such as include/linux/omap* be moved to
>> include/linux/soc/ti/ ? I guess that is not pertinent to this specific
>> patch, but I am curious..
>>
> include/linux/omap-mailbox.h predates mailbox api.
> But yes, I do think include/linux is not the right place for platform
> specific headers. include/linux/soc/ is.

Will  fix this.

>>
>>>> +       /* Do I actually have messages to read? */
>>>> +       msg_count = ti_msgmgr_queue_get_num_messages(qinst);
>>>> +       if (!msg_count) {
>>>> +               /* Shared IRQ? */
>>>> +               dev_dbg(dev, "Spurious event - 0 pending data!\n");
>>>> +               return IRQ_NONE;
>>>> +       }
>>>> +
>>>> +       /*
>>>> +        * I have no idea about the protocol being used to communicate with the
>>>> +        * remote producer - 0 could be valid data, so I wont make a judgement
>>>> +        * of how many bytes I should be reading. Let the client figure this
>>>> +        * out.. I just read the full message and pass it on..
>>>> +        */
>>> Exactly. And similarly when you send data, you should not have more
>>> than one message in transit. Now please see my note in
>>> ti_msgmgr_send_data()
>>>
>>>
>>>> +static int ti_msgmgr_send_data(struct mbox_chan *chan, void *data)
>>>> +{
>>>> +       struct device *dev = chan->mbox->dev;
>>>> +       struct ti_msgmgr_inst *inst = dev_get_drvdata(dev);
>>>> +       const struct ti_msgmgr_desc *desc;
>>>> +       struct ti_queue_inst *qinst = chan->con_priv;
>>>> +       int msg_count, num_words, trail_bytes;
>>>> +       struct ti_msgmgr_message *message = data;
>>>> +       void __iomem *data_reg;
>>>> +       u32 *word_data;
>>>> +
>>>> +       if (WARN_ON(!inst)) {
>>>> +               dev_err(dev, "no platform drv data??\n");
>>>> +               return -EINVAL;
>>>> +       }
>>>> +       desc = inst->desc;
>>>> +
>>>> +       if (desc->max_message_size < message->len) {
>>>> +               dev_err(dev, "Queue %s message length %d > max %d\n",
>>>> +                       qinst->name, message->len, desc->max_message_size);
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       /* Are we able to send this or not? */
>>>> +       msg_count = ti_msgmgr_queue_get_num_messages(qinst);
>>>> +       if (msg_count >= desc->max_messages) {
>>>> +               dev_warn(dev, "Queue %s is full (%d messages)\n", qinst->name,
>>>> +                        msg_count);
>>>> +               return -EBUSY;
>>>> +       }
>>> This seems fishy. mailbox api always submit 1 'complete' message to be
>>> sent and checks for completion by last_tx_done() before calling
>>> send_data() again. Controller drivers are not supposed to queue
>>> messages - mailbox core does. So you should never be unable to send a
>>> message.
>>
>>
>> OK-> to explain this, few reasons: (queue messages check and usage of
>> last_tx_done are kind of intertwined answer..
>> a) we need to remember that the message manager has a shared RAM.
>> multiple transmitter over other queues can be sharing the same.
>> unfortunately, we dont get a threshold kind of interrupt or status that
>> I am able to exploit in the current incarnation of the solution. The
>> best we can do in the full system is to constrain the number of messages
>> that are max pending simultaneously in each of the queue from various
>> transmitters in the SoC.
>> b) last_tx_done() is checked if TXDONE_BY_POLL, not if TXDONE_BY_ACK
>> right? which is how this'd work since txdone_poll is false -> that is
>> how we want this mechanism to work once the far end is ready for next
>> message, it acks. I do see your point about being tied to protocol - I
>> dont like it either.. in fact, I'd prefer that client registration
>> mention what kind of handshaking is necessary, but: a) that is not how
>> mailbox framework is constructed at the moment(we state txdone_poll at
>> mailbox registration, not at client usage) and b) I have no real need
>> for multiple clients to users of message manager who actually need
>> non-ACK usage - even for the foreseeable future (at least 1 next
>> generation of SoC) - if such a need does arise in the future, I will
>> have to rework framework and make this capability at the registration
>> time of the client - allowing each client path to use different
>> mechanisms on hardware such as these that need it.
>> c) message manager can actually queue more than one message(depending on
>> client capability). Even though, at this point, we are not really
>> capable of doing it(again from what I can see for immediate future),
>> mailbox framework by checking last_tx_done forces a single message
>> sequencing - which is not really exploiting the capability of the
>> hardware - in theory, we should be able to queue max num messages, hit
>> cpuidle and snooze away while the remote entity chomp away data at it's
>> own pace and finally give us a notification back - but again, we can
>> argue it is indeed protocol dependent, so setting txdone_poll to false
>> actually enables that to be done in user. Again - i have no immediate
>> need for any queued multiple transfer needs yet.. even if i need to, in
>> the future, it can easily be done by the client by maintaining code as
>> is - txdone_poll is false.
>>
> All I suggest is that the controller does not queue more than 1
> message at a time, which means the controller driver allows for
> maximum possible resources taken by a message.
> The buffering is already done by the core, and if for your 'batch
> dispatch' thing the client could simply flush them to remote by
> pretending it got the ack (which is no worse than simply sending all
> messages to remote without caring if the first was successful or not).

Are you suggesting to set txdone_poll is true? the controller is quite
capable of queueing more than 1 message at a time. This the reason for
letting the client choose the mode of operation - use ack mechanism for
operation. client can choose to ignore the buffering in the controller,
as you mentioned, but then, why force txdone_poll to true and deny the
usage of the queue capability of the hardware?

>>>> +       /*
>>>> +        * NOTE about register access involved here:
>>>> +        * the hardware block is implemented with 32bit access operations and no
>>>> +        * support for data splitting.  We don't want the hardware to misbehave
>>>> +        * with sub 32bit access - For example: if the last register write is
>>>> +        * split into byte wise access, it can result in the queue getting
>>>> +        * stuck or dummy messages being transmitted or indeterminate behavior.
>>>> +        * The same can occur if out of order operations take place.
>>>> +        * Hence, we do not use memcpy_toio or ___iowrite32_copy here, instead
>>>> +        * we use writel which ensures the sequencing we need.
>>>> +        */
>>> .... deja-vu ?
>>
>> Tell me about it.. but then, surprises like these do pop in once in a
>> while.. sigh..
>>
> I meant you have same para for read() , so maybe omit this one.

Aaah.. OK. i will add something trivial as "similar constraints as read"..

>>>> +
>>>> +/* Keystone K2G SoC integration details */
>>>> +static const struct ti_msgmgr_valid_queue_desc k2g_valid_queues[] = {
>>>> +       {.queue_id = 0, .proxy_id = 0, .is_tx = true,},
>>>> +       {.queue_id = 1, .proxy_id = 0, .is_tx = true,},
>>>> +       {.queue_id = 2, .proxy_id = 0, .is_tx = true,},
>>>> +       {.queue_id = 3, .proxy_id = 0, .is_tx = true,},
>>>> +       {.queue_id = 5, .proxy_id = 2, .is_tx = false,},
>>>> +       {.queue_id = 56, .proxy_id = 1, .is_tx = true,},
>>>> +       {.queue_id = 57, .proxy_id = 2, .is_tx = false,},
>>>> +       {.queue_id = 58, .proxy_id = 3, .is_tx = true,},
>>>> +       {.queue_id = 59, .proxy_id = 4, .is_tx = true,},
>>>> +       {.queue_id = 60, .proxy_id = 5, .is_tx = true,},
>>>> +       {.queue_id = 61, .proxy_id = 6, .is_tx = true,},
>>>> +};
>>>> +
>>>> +static const struct ti_msgmgr_desc k2g_desc = {
>>>> +       .queue_count = 64,
>>>> +       .max_message_size = 64,
>>>> +       .max_messages = 128,
>>>> +       .q_slices = 1,
>>>> +       .q_proxies = 1,
>>>> +       .data_first_reg = 16,
>>>> +       .data_last_reg = 31,
>>>> +       .tx_polled = false,
>>>> +       .valid_queues = k2g_valid_queues,
>>>> +       .num_valid_queues = ARRAY_SIZE(k2g_valid_queues),
>>>> +};
>>> If these parameters are very configurable, maybe they should be in DT?
>>
>> These are SoC integration specific data. based on DT, we will only have
>> SoC specific compatible property in DT. Since  the data is tied to SoC
>> integration, there is no benefit of keeping these in DT.
>>
> Well, yes if the numbers don't change with very often.

Hopefully not.

-- 
Regards,
Nishanth Menon

Re: [PATCH V2 2/2] mailbox: Introduce TI message manager driver

[Jassi Brar]

On Tue, Mar 8, 2016 at 2:18 AM, Nishanth Menon <nm-l0cyMroinI0@public.gmane.org> wrote:
> On 03/07/2016 12:31 PM, Jassi Brar wrote:
>> On Fri, Mar 4, 2016 at 8:05 PM, Nishanth Menon <nm-l0cyMroinI0@public.gmane.org> wrote:
>>>>
>>>>> +static int ti_msgmgr_send_data(struct mbox_chan *chan, void *data)
>>>>> +{
>>>>> +       struct device *dev = chan->mbox->dev;
>>>>> +       struct ti_msgmgr_inst *inst = dev_get_drvdata(dev);
>>>>> +       const struct ti_msgmgr_desc *desc;
>>>>> +       struct ti_queue_inst *qinst = chan->con_priv;
>>>>> +       int msg_count, num_words, trail_bytes;
>>>>> +       struct ti_msgmgr_message *message = data;
>>>>> +       void __iomem *data_reg;
>>>>> +       u32 *word_data;
>>>>> +
>>>>> +       if (WARN_ON(!inst)) {
>>>>> +               dev_err(dev, "no platform drv data??\n");
>>>>> +               return -EINVAL;
>>>>> +       }
>>>>> +       desc = inst->desc;
>>>>> +
>>>>> +       if (desc->max_message_size < message->len) {
>>>>> +               dev_err(dev, "Queue %s message length %d > max %d\n",
>>>>> +                       qinst->name, message->len, desc->max_message_size);
>>>>> +               return -EINVAL;
>>>>> +       }
>>>>> +
>>>>> +       /* Are we able to send this or not? */
>>>>> +       msg_count = ti_msgmgr_queue_get_num_messages(qinst);
>>>>> +       if (msg_count >= desc->max_messages) {
>>>>> +               dev_warn(dev, "Queue %s is full (%d messages)\n", qinst->name,
>>>>> +                        msg_count);
>>>>> +               return -EBUSY;
>>>>> +       }
>>>> This seems fishy. mailbox api always submit 1 'complete' message to be
>>>> sent and checks for completion by last_tx_done() before calling
>>>> send_data() again. Controller drivers are not supposed to queue
>>>> messages - mailbox core does. So you should never be unable to send a
>>>> message.
>>>
>>>
>>> OK-> to explain this, few reasons: (queue messages check and usage of
>>> last_tx_done are kind of intertwined answer..
>>> a) we need to remember that the message manager has a shared RAM.
>>> multiple transmitter over other queues can be sharing the same.
>>> unfortunately, we dont get a threshold kind of interrupt or status that
>>> I am able to exploit in the current incarnation of the solution. The
>>> best we can do in the full system is to constrain the number of messages
>>> that are max pending simultaneously in each of the queue from various
>>> transmitters in the SoC.
>>> b) last_tx_done() is checked if TXDONE_BY_POLL, not if TXDONE_BY_ACK
>>> right? which is how this'd work since txdone_poll is false -> that is
>>> how we want this mechanism to work once the far end is ready for next
>>> message, it acks. I do see your point about being tied to protocol - I
>>> dont like it either.. in fact, I'd prefer that client registration
>>> mention what kind of handshaking is necessary, but: a) that is not how
>>> mailbox framework is constructed at the moment(we state txdone_poll at
>>> mailbox registration, not at client usage) and b) I have no real need
>>> for multiple clients to users of message manager who actually need
>>> non-ACK usage - even for the foreseeable future (at least 1 next
>>> generation of SoC) - if such a need does arise in the future, I will
>>> have to rework framework and make this capability at the registration
>>> time of the client - allowing each client path to use different
>>> mechanisms on hardware such as these that need it.
>>> c) message manager can actually queue more than one message(depending on
>>> client capability). Even though, at this point, we are not really
>>> capable of doing it(again from what I can see for immediate future),
>>> mailbox framework by checking last_tx_done forces a single message
>>> sequencing - which is not really exploiting the capability of the
>>> hardware - in theory, we should be able to queue max num messages, hit
>>> cpuidle and snooze away while the remote entity chomp away data at it's
>>> own pace and finally give us a notification back - but again, we can
>>> argue it is indeed protocol dependent, so setting txdone_poll to false
>>> actually enables that to be done in user. Again - i have no immediate
>>> need for any queued multiple transfer needs yet.. even if i need to, in
>>> the future, it can easily be done by the client by maintaining code as
>>> is - txdone_poll is false.
>>>
>> All I suggest is that the controller does not queue more than 1
>> message at a time, which means the controller driver allows for
>> maximum possible resources taken by a message.
>> The buffering is already done by the core, and if for your 'batch
>> dispatch' thing the client could simply flush them to remote by
>> pretending it got the ack (which is no worse than simply sending all
>> messages to remote without caring if the first was successful or not).
>
> Are you suggesting to set txdone_poll is true?
No.

> the controller is quite
> capable of queueing more than 1 message at a time. This the reason for
> letting the client choose the mode of operation - use ack mechanism for
> operation. client can choose to ignore the buffering in the controller,
> as you mentioned, but then, why force txdone_poll to true and deny the
> usage of the queue capability of the hardware?
>
irq/poll/ack whatever you use, there is no valid reason to buffer
messages in the controller driver. Please let me know what usecase you
have in mind that must have messages buffered in controller driver and
not core.

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

Re: [PATCH V2 2/2] mailbox: Introduce TI message manager driver

[Nishanth Menon]

Jassi,

On Tue, Mar 8, 2016 at 1:10 AM, Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Mar 8, 2016 at 2:18 AM, Nishanth Menon <nm-l0cyMroinI0@public.gmane.org> wrote:
>> On 03/07/2016 12:31 PM, Jassi Brar wrote:
>>> On Fri, Mar 4, 2016 at 8:05 PM, Nishanth Menon <nm-l0cyMroinI0@public.gmane.org> wrote:
>>>>>
>>>>>> +static int ti_msgmgr_send_data(struct mbox_chan *chan, void *data)
>>>>>> +{
>>>>>> +       struct device *dev = chan->mbox->dev;
>>>>>> +       struct ti_msgmgr_inst *inst = dev_get_drvdata(dev);
>>>>>> +       const struct ti_msgmgr_desc *desc;
>>>>>> +       struct ti_queue_inst *qinst = chan->con_priv;
>>>>>> +       int msg_count, num_words, trail_bytes;
>>>>>> +       struct ti_msgmgr_message *message = data;
>>>>>> +       void __iomem *data_reg;
>>>>>> +       u32 *word_data;
>>>>>> +
>>>>>> +       if (WARN_ON(!inst)) {
>>>>>> +               dev_err(dev, "no platform drv data??\n");
>>>>>> +               return -EINVAL;
>>>>>> +       }
>>>>>> +       desc = inst->desc;
>>>>>> +
>>>>>> +       if (desc->max_message_size < message->len) {
>>>>>> +               dev_err(dev, "Queue %s message length %d > max %d\n",
>>>>>> +                       qinst->name, message->len, desc->max_message_size);
>>>>>> +               return -EINVAL;
>>>>>> +       }
>>>>>> +
>>>>>> +       /* Are we able to send this or not? */
>>>>>> +       msg_count = ti_msgmgr_queue_get_num_messages(qinst);
>>>>>> +       if (msg_count >= desc->max_messages) {
>>>>>> +               dev_warn(dev, "Queue %s is full (%d messages)\n", qinst->name,
>>>>>> +                        msg_count);
>>>>>> +               return -EBUSY;
>>>>>> +       }
>>>>> This seems fishy. mailbox api always submit 1 'complete' message to be
>>>>> sent and checks for completion by last_tx_done() before calling
>>>>> send_data() again. Controller drivers are not supposed to queue
>>>>> messages - mailbox core does. So you should never be unable to send a
>>>>> message.
>>>>
>>>>
>>>> OK-> to explain this, few reasons: (queue messages check and usage of
>>>> last_tx_done are kind of intertwined answer..
>>>> a) we need to remember that the message manager has a shared RAM.
>>>> multiple transmitter over other queues can be sharing the same.
>>>> unfortunately, we dont get a threshold kind of interrupt or status that
>>>> I am able to exploit in the current incarnation of the solution. The
>>>> best we can do in the full system is to constrain the number of messages
>>>> that are max pending simultaneously in each of the queue from various
>>>> transmitters in the SoC.
>>>> b) last_tx_done() is checked if TXDONE_BY_POLL, not if TXDONE_BY_ACK
>>>> right? which is how this'd work since txdone_poll is false -> that is
>>>> how we want this mechanism to work once the far end is ready for next
>>>> message, it acks. I do see your point about being tied to protocol - I
>>>> dont like it either.. in fact, I'd prefer that client registration
>>>> mention what kind of handshaking is necessary, but: a) that is not how
>>>> mailbox framework is constructed at the moment(we state txdone_poll at
>>>> mailbox registration, not at client usage) and b) I have no real need
>>>> for multiple clients to users of message manager who actually need
>>>> non-ACK usage - even for the foreseeable future (at least 1 next
>>>> generation of SoC) - if such a need does arise in the future, I will
>>>> have to rework framework and make this capability at the registration
>>>> time of the client - allowing each client path to use different
>>>> mechanisms on hardware such as these that need it.
>>>> c) message manager can actually queue more than one message(depending on
>>>> client capability). Even though, at this point, we are not really
>>>> capable of doing it(again from what I can see for immediate future),
>>>> mailbox framework by checking last_tx_done forces a single message
>>>> sequencing - which is not really exploiting the capability of the
>>>> hardware - in theory, we should be able to queue max num messages, hit
>>>> cpuidle and snooze away while the remote entity chomp away data at it's
>>>> own pace and finally give us a notification back - but again, we can
>>>> argue it is indeed protocol dependent, so setting txdone_poll to false
>>>> actually enables that to be done in user. Again - i have no immediate
>>>> need for any queued multiple transfer needs yet.. even if i need to, in
>>>> the future, it can easily be done by the client by maintaining code as
>>>> is - txdone_poll is false.
>>>>
>>> All I suggest is that the controller does not queue more than 1
>>> message at a time, which means the controller driver allows for
>>> maximum possible resources taken by a message.
>>> The buffering is already done by the core, and if for your 'batch
>>> dispatch' thing the client could simply flush them to remote by
>>> pretending it got the ack (which is no worse than simply sending all
>>> messages to remote without caring if the first was successful or not).
>>
>> Are you suggesting to set txdone_poll is true?
> No.
>
>> the controller is quite
>> capable of queueing more than 1 message at a time. This the reason for
>> letting the client choose the mode of operation - use ack mechanism for
>> operation. client can choose to ignore the buffering in the controller,
>> as you mentioned, but then, why force txdone_poll to true and deny the
>> usage of the queue capability of the hardware?
>>
> irq/poll/ack whatever you use, there is no valid reason to buffer
> messages in the controller driver. Please let me know what usecase you
> have in mind that must have messages buffered in controller driver and
> not core.

I am confused, I am _not_ buffering any tx data in the controller
driver - rx data is stored in a temp buffer to send up the stack -
that is just regular practise, right?. In tx, I just check to ensure
that the queue has'nt run out prior to transmission since the hardware
is capable of queueing - ok, in a single transmitter system it is
probably a little overkill, but we would like to function in multiple
producer SoC as well. What am I missing here?

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

Re: [PATCH V2 2/2] mailbox: Introduce TI message manager driver

[Jassi Brar]

On Tue, Mar 8, 2016 at 8:07 PM, Nishanth Menon <nm-l0cyMroinI0@public.gmane.org> wrote:
> Jassi,
>
> On Tue, Mar 8, 2016 at 1:10 AM, Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Tue, Mar 8, 2016 at 2:18 AM, Nishanth Menon <nm-l0cyMroinI0@public.gmane.org> wrote:
>>> On 03/07/2016 12:31 PM, Jassi Brar wrote:
>>>> On Fri, Mar 4, 2016 at 8:05 PM, Nishanth Menon <nm-l0cyMroinI0@public.gmane.org> wrote:
>>>>>>
>>>>>>> +static int ti_msgmgr_send_data(struct mbox_chan *chan, void *data)
>>>>>>> +{
>>>>>>> +       struct device *dev = chan->mbox->dev;
>>>>>>> +       struct ti_msgmgr_inst *inst = dev_get_drvdata(dev);
>>>>>>> +       const struct ti_msgmgr_desc *desc;
>>>>>>> +       struct ti_queue_inst *qinst = chan->con_priv;
>>>>>>> +       int msg_count, num_words, trail_bytes;
>>>>>>> +       struct ti_msgmgr_message *message = data;
>>>>>>> +       void __iomem *data_reg;
>>>>>>> +       u32 *word_data;
>>>>>>> +
>>>>>>> +       if (WARN_ON(!inst)) {
>>>>>>> +               dev_err(dev, "no platform drv data??\n");
>>>>>>> +               return -EINVAL;
>>>>>>> +       }
>>>>>>> +       desc = inst->desc;
>>>>>>> +
>>>>>>> +       if (desc->max_message_size < message->len) {
>>>>>>> +               dev_err(dev, "Queue %s message length %d > max %d\n",
>>>>>>> +                       qinst->name, message->len, desc->max_message_size);
>>>>>>> +               return -EINVAL;
>>>>>>> +       }
>>>>>>> +
>>>>>>> +       /* Are we able to send this or not? */
>>>>>>> +       msg_count = ti_msgmgr_queue_get_num_messages(qinst);
>>>>>>> +       if (msg_count >= desc->max_messages) {
>>>>>>> +               dev_warn(dev, "Queue %s is full (%d messages)\n", qinst->name,
>>>>>>> +                        msg_count);
>>>>>>> +               return -EBUSY;
>>>>>>> +       }
>>>>>> This seems fishy. mailbox api always submit 1 'complete' message to be
>>>>>> sent and checks for completion by last_tx_done() before calling
>>>>>> send_data() again. Controller drivers are not supposed to queue
>>>>>> messages - mailbox core does. So you should never be unable to send a
>>>>>> message.
>>>>>
>>>>>
>>>>> OK-> to explain this, few reasons: (queue messages check and usage of
>>>>> last_tx_done are kind of intertwined answer..
>>>>> a) we need to remember that the message manager has a shared RAM.
>>>>> multiple transmitter over other queues can be sharing the same.
>>>>> unfortunately, we dont get a threshold kind of interrupt or status that
>>>>> I am able to exploit in the current incarnation of the solution. The
>>>>> best we can do in the full system is to constrain the number of messages
>>>>> that are max pending simultaneously in each of the queue from various
>>>>> transmitters in the SoC.
>>>>> b) last_tx_done() is checked if TXDONE_BY_POLL, not if TXDONE_BY_ACK
>>>>> right? which is how this'd work since txdone_poll is false -> that is
>>>>> how we want this mechanism to work once the far end is ready for next
>>>>> message, it acks. I do see your point about being tied to protocol - I
>>>>> dont like it either.. in fact, I'd prefer that client registration
>>>>> mention what kind of handshaking is necessary, but: a) that is not how
>>>>> mailbox framework is constructed at the moment(we state txdone_poll at
>>>>> mailbox registration, not at client usage) and b) I have no real need
>>>>> for multiple clients to users of message manager who actually need
>>>>> non-ACK usage - even for the foreseeable future (at least 1 next
>>>>> generation of SoC) - if such a need does arise in the future, I will
>>>>> have to rework framework and make this capability at the registration
>>>>> time of the client - allowing each client path to use different
>>>>> mechanisms on hardware such as these that need it.
>>>>> c) message manager can actually queue more than one message(depending on
>>>>> client capability). Even though, at this point, we are not really
>>>>> capable of doing it(again from what I can see for immediate future),
>>>>> mailbox framework by checking last_tx_done forces a single message
>>>>> sequencing - which is not really exploiting the capability of the
>>>>> hardware - in theory, we should be able to queue max num messages, hit
>>>>> cpuidle and snooze away while the remote entity chomp away data at it's
>>>>> own pace and finally give us a notification back - but again, we can
>>>>> argue it is indeed protocol dependent, so setting txdone_poll to false
>>>>> actually enables that to be done in user. Again - i have no immediate
>>>>> need for any queued multiple transfer needs yet.. even if i need to, in
>>>>> the future, it can easily be done by the client by maintaining code as
>>>>> is - txdone_poll is false.
>>>>>
>>>> All I suggest is that the controller does not queue more than 1
>>>> message at a time, which means the controller driver allows for
>>>> maximum possible resources taken by a message.
>>>> The buffering is already done by the core, and if for your 'batch
>>>> dispatch' thing the client could simply flush them to remote by
>>>> pretending it got the ack (which is no worse than simply sending all
>>>> messages to remote without caring if the first was successful or not).
>>>
>>> Are you suggesting to set txdone_poll is true?
>> No.
>>
>>> the controller is quite
>>> capable of queueing more than 1 message at a time. This the reason for
>>> letting the client choose the mode of operation - use ack mechanism for
>>> operation. client can choose to ignore the buffering in the controller,
>>> as you mentioned, but then, why force txdone_poll to true and deny the
>>> usage of the queue capability of the hardware?
>>>
>> irq/poll/ack whatever you use, there is no valid reason to buffer
>> messages in the controller driver. Please let me know what usecase you
>> have in mind that must have messages buffered in controller driver and
>> not core.
>
> I am confused, I am _not_ buffering any tx data in the controller
> driver - rx data is stored in a temp buffer to send up the stack -
> that is just regular practise, right?
>
right.

> In tx, I just check to ensure
> that the queue has'nt run out prior to transmission since the hardware
> is capable of queueing - ok, in a single transmitter system it is
> probably a little overkill, but we would like to function in multiple
> producer SoC as well. What am I missing here?
>
In send_data() you have ...

+       /* Are we able to send this or not? */
+       msg_count = ti_msgmgr_queue_get_num_messages(qinst);
+       if (msg_count >= desc->max_messages) {
+               dev_warn(dev, "Queue %s is full (%d messages)\n", qinst->name,
+                        msg_count);
+               return -EBUSY;
+       }

That is, you check if there are some messages in the TX-Queue already.
I am not sure how you could hit this and if that is legit.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 2/2] mailbox: Introduce TI message manager driver

[Nishanth Menon]

On Tue, Mar 15, 2016 at 12:31 AM, Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Mar 8, 2016 at 8:07 PM, Nishanth Menon <nm-l0cyMroinI0@public.gmane.org> wrote:
>> Jassi,
>>
>> On Tue, Mar 8, 2016 at 1:10 AM, Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On Tue, Mar 8, 2016 at 2:18 AM, Nishanth Menon <nm-l0cyMroinI0@public.gmane.org> wrote:
>>>> On 03/07/2016 12:31 PM, Jassi Brar wrote:
>>>>> On Fri, Mar 4, 2016 at 8:05 PM, Nishanth Menon <nm-l0cyMroinI0@public.gmane.org> wrote:
>>>>>>>
>>>>>>>> +static int ti_msgmgr_send_data(struct mbox_chan *chan, void *data)
>>>>>>>> +{
>>>>>>>> +       struct device *dev = chan->mbox->dev;
>>>>>>>> +       struct ti_msgmgr_inst *inst = dev_get_drvdata(dev);
>>>>>>>> +       const struct ti_msgmgr_desc *desc;
>>>>>>>> +       struct ti_queue_inst *qinst = chan->con_priv;
>>>>>>>> +       int msg_count, num_words, trail_bytes;
>>>>>>>> +       struct ti_msgmgr_message *message = data;
>>>>>>>> +       void __iomem *data_reg;
>>>>>>>> +       u32 *word_data;
>>>>>>>> +
>>>>>>>> +       if (WARN_ON(!inst)) {
>>>>>>>> +               dev_err(dev, "no platform drv data??\n");
>>>>>>>> +               return -EINVAL;
>>>>>>>> +       }
>>>>>>>> +       desc = inst->desc;
>>>>>>>> +
>>>>>>>> +       if (desc->max_message_size < message->len) {
>>>>>>>> +               dev_err(dev, "Queue %s message length %d > max %d\n",
>>>>>>>> +                       qinst->name, message->len, desc->max_message_size);
>>>>>>>> +               return -EINVAL;
>>>>>>>> +       }
>>>>>>>> +
>>>>>>>> +       /* Are we able to send this or not? */
>>>>>>>> +       msg_count = ti_msgmgr_queue_get_num_messages(qinst);
>>>>>>>> +       if (msg_count >= desc->max_messages) {
>>>>>>>> +               dev_warn(dev, "Queue %s is full (%d messages)\n", qinst->name,
>>>>>>>> +                        msg_count);
>>>>>>>> +               return -EBUSY;
>>>>>>>> +       }
>>>>>>> This seems fishy. mailbox api always submit 1 'complete' message to be
>>>>>>> sent and checks for completion by last_tx_done() before calling
>>>>>>> send_data() again. Controller drivers are not supposed to queue
>>>>>>> messages - mailbox core does. So you should never be unable to send a
>>>>>>> message.
>>>>>>
>>>>>>
>>>>>> OK-> to explain this, few reasons: (queue messages check and usage of
>>>>>> last_tx_done are kind of intertwined answer..
>>>>>> a) we need to remember that the message manager has a shared RAM.
>>>>>> multiple transmitter over other queues can be sharing the same.
>>>>>> unfortunately, we dont get a threshold kind of interrupt or status that
>>>>>> I am able to exploit in the current incarnation of the solution. The
>>>>>> best we can do in the full system is to constrain the number of messages
>>>>>> that are max pending simultaneously in each of the queue from various
>>>>>> transmitters in the SoC.
>>>>>> b) last_tx_done() is checked if TXDONE_BY_POLL, not if TXDONE_BY_ACK
>>>>>> right? which is how this'd work since txdone_poll is false -> that is
>>>>>> how we want this mechanism to work once the far end is ready for next
>>>>>> message, it acks. I do see your point about being tied to protocol - I
>>>>>> dont like it either.. in fact, I'd prefer that client registration
>>>>>> mention what kind of handshaking is necessary, but: a) that is not how
>>>>>> mailbox framework is constructed at the moment(we state txdone_poll at
>>>>>> mailbox registration, not at client usage) and b) I have no real need
>>>>>> for multiple clients to users of message manager who actually need
>>>>>> non-ACK usage - even for the foreseeable future (at least 1 next
>>>>>> generation of SoC) - if such a need does arise in the future, I will
>>>>>> have to rework framework and make this capability at the registration
>>>>>> time of the client - allowing each client path to use different
>>>>>> mechanisms on hardware such as these that need it.
>>>>>> c) message manager can actually queue more than one message(depending on
>>>>>> client capability). Even though, at this point, we are not really
>>>>>> capable of doing it(again from what I can see for immediate future),
>>>>>> mailbox framework by checking last_tx_done forces a single message
>>>>>> sequencing - which is not really exploiting the capability of the
>>>>>> hardware - in theory, we should be able to queue max num messages, hit
>>>>>> cpuidle and snooze away while the remote entity chomp away data at it's
>>>>>> own pace and finally give us a notification back - but again, we can
>>>>>> argue it is indeed protocol dependent, so setting txdone_poll to false
>>>>>> actually enables that to be done in user. Again - i have no immediate
>>>>>> need for any queued multiple transfer needs yet.. even if i need to, in
>>>>>> the future, it can easily be done by the client by maintaining code as
>>>>>> is - txdone_poll is false.
>>>>>>
>>>>> All I suggest is that the controller does not queue more than 1
>>>>> message at a time, which means the controller driver allows for
>>>>> maximum possible resources taken by a message.
>>>>> The buffering is already done by the core, and if for your 'batch
>>>>> dispatch' thing the client could simply flush them to remote by
>>>>> pretending it got the ack (which is no worse than simply sending all
>>>>> messages to remote without caring if the first was successful or not).
>>>>
>>>> Are you suggesting to set txdone_poll is true?
>>> No.
>>>
>>>> the controller is quite
>>>> capable of queueing more than 1 message at a time. This the reason for
>>>> letting the client choose the mode of operation - use ack mechanism for
>>>> operation. client can choose to ignore the buffering in the controller,
>>>> as you mentioned, but then, why force txdone_poll to true and deny the
>>>> usage of the queue capability of the hardware?
>>>>
>>> irq/poll/ack whatever you use, there is no valid reason to buffer
>>> messages in the controller driver. Please let me know what usecase you
>>> have in mind that must have messages buffered in controller driver and
>>> not core.
>>
>> I am confused, I am _not_ buffering any tx data in the controller
>> driver - rx data is stored in a temp buffer to send up the stack -
>> that is just regular practise, right?
>>
> right.
>
>> In tx, I just check to ensure
>> that the queue has'nt run out prior to transmission since the hardware
>> is capable of queueing - ok, in a single transmitter system it is
>> probably a little overkill, but we would like to function in multiple
>> producer SoC as well. What am I missing here?
>>
> In send_data() you have ...
>
> +       /* Are we able to send this or not? */
> +       msg_count = ti_msgmgr_queue_get_num_messages(qinst);
> +       if (msg_count >= desc->max_messages) {
> +               dev_warn(dev, "Queue %s is full (%d messages)\n", qinst->name,
> +                        msg_count);
> +               return -EBUSY;
> +       }
>
> That is, you check if there are some messages in the TX-Queue already.
> I am not sure how you could hit this and if that is legit.

Alright, i will drop this check since it is causing a lot more
confusion that that is worth. we can introduce it when we finally do
hit an issue eventually with multiple processors trying to transmit on
the same queue manager. that is not a concern at the very immediate
time, so we should be good to drop.

please let me know if you are ok with this.

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

Re: [PATCH V2 2/2] mailbox: Introduce TI message manager driver

[Jassi Brar]

On Tue, Mar 15, 2016 at 10:35 PM, Nishanth Menon <nm@ti.com> wrote:
> On Tue, Mar 15, 2016 at 12:31 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>> On Tue, Mar 8, 2016 at 8:07 PM, Nishanth Menon <nm@ti.com> wrote:
>>> Jassi,
>>>
>>> On Tue, Mar 8, 2016 at 1:10 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>> On Tue, Mar 8, 2016 at 2:18 AM, Nishanth Menon <nm@ti.com> wrote:
>>>>> On 03/07/2016 12:31 PM, Jassi Brar wrote:
>>>>>> On Fri, Mar 4, 2016 at 8:05 PM, Nishanth Menon <nm@ti.com> wrote:
>>>>>>>>
>>>>>>>>> +static int ti_msgmgr_send_data(struct mbox_chan *chan, void *data)
>>>>>>>>> +{
>>>>>>>>> +       struct device *dev = chan->mbox->dev;
>>>>>>>>> +       struct ti_msgmgr_inst *inst = dev_get_drvdata(dev);
>>>>>>>>> +       const struct ti_msgmgr_desc *desc;
>>>>>>>>> +       struct ti_queue_inst *qinst = chan->con_priv;
>>>>>>>>> +       int msg_count, num_words, trail_bytes;
>>>>>>>>> +       struct ti_msgmgr_message *message = data;
>>>>>>>>> +       void __iomem *data_reg;
>>>>>>>>> +       u32 *word_data;
>>>>>>>>> +
>>>>>>>>> +       if (WARN_ON(!inst)) {
>>>>>>>>> +               dev_err(dev, "no platform drv data??\n");
>>>>>>>>> +               return -EINVAL;
>>>>>>>>> +       }
>>>>>>>>> +       desc = inst->desc;
>>>>>>>>> +
>>>>>>>>> +       if (desc->max_message_size < message->len) {
>>>>>>>>> +               dev_err(dev, "Queue %s message length %d > max %d\n",
>>>>>>>>> +                       qinst->name, message->len, desc->max_message_size);
>>>>>>>>> +               return -EINVAL;
>>>>>>>>> +       }
>>>>>>>>> +
>>>>>>>>> +       /* Are we able to send this or not? */
>>>>>>>>> +       msg_count = ti_msgmgr_queue_get_num_messages(qinst);
>>>>>>>>> +       if (msg_count >= desc->max_messages) {
>>>>>>>>> +               dev_warn(dev, "Queue %s is full (%d messages)\n", qinst->name,
>>>>>>>>> +                        msg_count);
>>>>>>>>> +               return -EBUSY;
>>>>>>>>> +       }
>>>>>>>> This seems fishy. mailbox api always submit 1 'complete' message to be
>>>>>>>> sent and checks for completion by last_tx_done() before calling
>>>>>>>> send_data() again. Controller drivers are not supposed to queue
>>>>>>>> messages - mailbox core does. So you should never be unable to send a
>>>>>>>> message.
>>>>>>>
>>>>>>>
>>>>>>> OK-> to explain this, few reasons: (queue messages check and usage of
>>>>>>> last_tx_done are kind of intertwined answer..
>>>>>>> a) we need to remember that the message manager has a shared RAM.
>>>>>>> multiple transmitter over other queues can be sharing the same.
>>>>>>> unfortunately, we dont get a threshold kind of interrupt or status that
>>>>>>> I am able to exploit in the current incarnation of the solution. The
>>>>>>> best we can do in the full system is to constrain the number of messages
>>>>>>> that are max pending simultaneously in each of the queue from various
>>>>>>> transmitters in the SoC.
>>>>>>> b) last_tx_done() is checked if TXDONE_BY_POLL, not if TXDONE_BY_ACK
>>>>>>> right? which is how this'd work since txdone_poll is false -> that is
>>>>>>> how we want this mechanism to work once the far end is ready for next
>>>>>>> message, it acks. I do see your point about being tied to protocol - I
>>>>>>> dont like it either.. in fact, I'd prefer that client registration
>>>>>>> mention what kind of handshaking is necessary, but: a) that is not how
>>>>>>> mailbox framework is constructed at the moment(we state txdone_poll at
>>>>>>> mailbox registration, not at client usage) and b) I have no real need
>>>>>>> for multiple clients to users of message manager who actually need
>>>>>>> non-ACK usage - even for the foreseeable future (at least 1 next
>>>>>>> generation of SoC) - if such a need does arise in the future, I will
>>>>>>> have to rework framework and make this capability at the registration
>>>>>>> time of the client - allowing each client path to use different
>>>>>>> mechanisms on hardware such as these that need it.
>>>>>>> c) message manager can actually queue more than one message(depending on
>>>>>>> client capability). Even though, at this point, we are not really
>>>>>>> capable of doing it(again from what I can see for immediate future),
>>>>>>> mailbox framework by checking last_tx_done forces a single message
>>>>>>> sequencing - which is not really exploiting the capability of the
>>>>>>> hardware - in theory, we should be able to queue max num messages, hit
>>>>>>> cpuidle and snooze away while the remote entity chomp away data at it's
>>>>>>> own pace and finally give us a notification back - but again, we can
>>>>>>> argue it is indeed protocol dependent, so setting txdone_poll to false
>>>>>>> actually enables that to be done in user. Again - i have no immediate
>>>>>>> need for any queued multiple transfer needs yet.. even if i need to, in
>>>>>>> the future, it can easily be done by the client by maintaining code as
>>>>>>> is - txdone_poll is false.
>>>>>>>
>>>>>> All I suggest is that the controller does not queue more than 1
>>>>>> message at a time, which means the controller driver allows for
>>>>>> maximum possible resources taken by a message.
>>>>>> The buffering is already done by the core, and if for your 'batch
>>>>>> dispatch' thing the client could simply flush them to remote by
>>>>>> pretending it got the ack (which is no worse than simply sending all
>>>>>> messages to remote without caring if the first was successful or not).
>>>>>
>>>>> Are you suggesting to set txdone_poll is true?
>>>> No.
>>>>
>>>>> the controller is quite
>>>>> capable of queueing more than 1 message at a time. This the reason for
>>>>> letting the client choose the mode of operation - use ack mechanism for
>>>>> operation. client can choose to ignore the buffering in the controller,
>>>>> as you mentioned, but then, why force txdone_poll to true and deny the
>>>>> usage of the queue capability of the hardware?
>>>>>
>>>> irq/poll/ack whatever you use, there is no valid reason to buffer
>>>> messages in the controller driver. Please let me know what usecase you
>>>> have in mind that must have messages buffered in controller driver and
>>>> not core.
>>>
>>> I am confused, I am _not_ buffering any tx data in the controller
>>> driver - rx data is stored in a temp buffer to send up the stack -
>>> that is just regular practise, right?
>>>
>> right.
>>
>>> In tx, I just check to ensure
>>> that the queue has'nt run out prior to transmission since the hardware
>>> is capable of queueing - ok, in a single transmitter system it is
>>> probably a little overkill, but we would like to function in multiple
>>> producer SoC as well. What am I missing here?
>>>
>> In send_data() you have ...
>>
>> +       /* Are we able to send this or not? */
>> +       msg_count = ti_msgmgr_queue_get_num_messages(qinst);
>> +       if (msg_count >= desc->max_messages) {
>> +               dev_warn(dev, "Queue %s is full (%d messages)\n", qinst->name,
>> +                        msg_count);
>> +               return -EBUSY;
>> +       }
>>
>> That is, you check if there are some messages in the TX-Queue already.
>> I am not sure how you could hit this and if that is legit.
>
> Alright, i will drop this check since it is causing a lot more
> confusion
>
It's confusing because you check ti_msgmgr_queue_get_num_messages()
also in ti_msgmgr_last_tx_done() which doesn't make sense because the
former accounts for messages from other senders also (as you say there
could be multiple senders).

> that that is worth. we can introduce it when we finally do
> hit an issue eventually with multiple processors trying to transmit on
> the same queue manager. that is not a concern at the very immediate
> time, so we should be good to drop.
>
> please let me know if you are ok with this.
>
I am ok with whatever you assert is needed for your platform. I just
point out what I think are inconsistencies in your assumptions. I'll
pick the next revision however it is.

Thanks.

Re: [PATCH V2 2/2] mailbox: Introduce TI message manager driver

[Nishanth Menon]

Hi Jassi,

On 03/16/2016 12:16 AM, Jassi Brar wrote:

[...]
>> Alright, i will drop this check since it is causing a lot more
>> confusion
>>
> It's confusing because you check ti_msgmgr_queue_get_num_messages()
> also in ti_msgmgr_last_tx_done() which doesn't make sense because the
> former accounts for messages from other senders also (as you say there
> could be multiple senders).


True -> I will drop it for now. I will see if the case I was trying to
protect is actually possible to be hit in the first place. And if proven
to be required, I will introduce it back with a better explanation and
the usecase where this is needed.

>> that that is worth. we can introduce it when we finally do
>> hit an issue eventually with multiple processors trying to transmit on
>> the same queue manager. that is not a concern at the very immediate
>> time, so we should be good to drop.
>>
>> please let me know if you are ok with this.
>>
> I am ok with whatever you assert is needed for your platform. I just
> point out what I think are inconsistencies in your assumptions. I'll
> pick the next revision however it is.


Thanks for your patience and guidance with this series. I have tried to
incorporate all the alignment we have had on this thread as part of
V3[1] of the series.


[1] http://marc.info/?l=linux-arm-kernel&m=145817434531691&w=2

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