[PATCH v4 1/5] mailbox: Make startup and shutdown ops optional

[PATCH v4 1/5] mailbox: Make startup and shutdown ops optional

[Bjorn Andersson]

Some mailbox hardware doesn't have to perform any additional operations
on startup of shutdown, so make these optional.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v3:
- New patch

 drivers/mailbox/mailbox.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 4671f8a12872..c88de953394a 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -137,6 +137,20 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
 	return HRTIMER_NORESTART;
 }
 
+static int mbox_startup(struct mbox_chan *chan)
+{
+	if (chan->mbox->ops->startup)
+		return chan->mbox->ops->startup(chan);
+
+	return 0;
+}
+
+static void mbox_shutdown(struct mbox_chan *chan)
+{
+	if (chan->mbox->ops->shutdown)
+		chan->mbox->ops->shutdown(chan);
+}
+
 /**
  * mbox_chan_received_data - A way for controller driver to push data
  *				received from remote to the upper layer.
@@ -352,7 +366,7 @@ struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
 
 	spin_unlock_irqrestore(&chan->lock, flags);
 
-	ret = chan->mbox->ops->startup(chan);
+	ret = mbox_startup(chan);
 	if (ret) {
 		dev_err(dev, "Unable to startup the chan (%d)\n", ret);
 		mbox_free_channel(chan);
@@ -405,7 +419,7 @@ void mbox_free_channel(struct mbox_chan *chan)
 	if (!chan || !chan->cl)
 		return;
 
-	chan->mbox->ops->shutdown(chan);
+	mbox_shutdown(chan);
 
 	/* The queued TX requests are simply aborted, no callbacks are made */
 	spin_lock_irqsave(&chan->lock, flags);
-- 
2.12.0

[PATCH v4 2/5] dt-bindings: mailbox: Introduce Qualcomm APCS global binding

[Bjorn Andersson]

Introduce a binding for the Qualcomm APCS global block, exposing a
mailbox for invoking interrupts on remote processors in the system.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v3:
- Transition to mailbox binding

Changes since v2:
- New binding

 .../bindings/mailbox/qcom,apcs-kpss-global.txt     | 46 ++++++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt

diff --git a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
new file mode 100644
index 000000000000..eaa9e780f412
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
@@ -0,0 +1,46 @@
+Binding for the Qualcomm APCS global block
+==========================================
+
+This binding describes the APCS "global" block found in various Qualcomm
+platforms.
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: must be one of:
+		    "qcom,msm8916-apcs-kpss-global",
+		    "qcom,msm8996-apcs-hmss-global"
+
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: must specify the base address and size of the global block
+
+- #mbox-cells:
+	Usage: required
+	Value type: <u32>
+	Definition: as described in mailbox.txt, must be 1
+
+
+= EXAMPLE
+The following example describes the APCS HMSS found in MSM8996 and part of the
+GLINK RPM referencing the "rpm_hlos" doorbell therein.
+
+	apcs_glb: apcs-glb@9820000 {
+		compatible = "qcom,msm8996-apcs-hmss-global";
+		reg = <0x9820000 0x1000>;
+
+		#mbox-cells = <1>;
+	};
+
+	rpm-glink {
+		compatible = "qcom,glink-rpm";
+
+		interrupts = <GIC_SPI 168 IRQ_TYPE_EDGE_RISING>;
+
+		qcom,rpm-msg-ram = <&rpm_msg_ram>;
+
+		mboxes = <&apcs_glb 0>;
+		mbox-names = "rpm_hlos";
+	};
+
-- 
2.12.0

[PATCH v4 3/5] soc: qcom: Introduce APCS IPC driver

[Bjorn Andersson]

This implements a driver that exposes the IPC bits found in the APCS
Global block in various Qualcomm platforms. The bits are used to signal
inter-processor communication signals from the application CPU to other
masters.

The driver implements the "doorbell" binding and could be used as basis
for a new Linux framework, if found useful outside Qualcomm.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v3:
- Migrate to mailbox framework

Changes since v2:
- New driver

 drivers/mailbox/Kconfig                 |   8 ++
 drivers/mailbox/Makefile                |   2 +
 drivers/mailbox/qcom-apcs-ipc-mailbox.c | 128 ++++++++++++++++++++++++++++++++
 3 files changed, 138 insertions(+)
 create mode 100644 drivers/mailbox/qcom-apcs-ipc-mailbox.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index ceff415f201c..fffc64da61f9 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -124,6 +124,14 @@ config MAILBOX_TEST
 	  Test client to help with testing new Controller driver
 	  implementations.
 
+config QCOM_APCS_IPC
+	tristate "Qualcomm APCS IPC driver"
+	depends on ARCH_QCOM
+	help
+	  Say y here to enable support for the APCS IPC doorbell driver,
+	  providing an interface for invoking the inter-process communication
+	  signals from the application processor to other masters.
+
 config TEGRA_HSP_MBOX
 	bool "Tegra HSP (Hardware Synchronization Primitives) Driver"
 	depends on ARCH_TEGRA_186_SOC
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 7dde4f609ae8..cc718c79669a 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -30,4 +30,6 @@ obj-$(CONFIG_HI6220_MBOX)	+= hi6220-mailbox.o
 
 obj-$(CONFIG_BCM_PDC_MBOX)	+= bcm-pdc-mailbox.o
 
+obj-$(CONFIG_QCOM_APCS_IPC)	+= qcom-apcs-ipc-mailbox.o
+
 obj-$(CONFIG_TEGRA_HSP_MBOX)	+= tegra-hsp.o
diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
new file mode 100644
index 000000000000..41e31c66c7aa
--- /dev/null
+++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
@@ -0,0 +1,128 @@
+/*
+ * Copyright (c) 2017, Linaro Ltd
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/mailbox_controller.h>
+
+#define QCOM_APCS_IPC_BITS	32
+
+struct qcom_apcs_ipc {
+	struct device *dev;
+
+	struct mbox_controller mbox;
+	struct mbox_chan mbox_chans[QCOM_APCS_IPC_BITS];
+
+	void __iomem *base;
+	unsigned long offset;
+};
+
+static int qcom_apcs_ipc_send_data(struct mbox_chan *chan, void *data)
+{
+	struct qcom_apcs_ipc *apcs = container_of(chan->mbox,
+						  struct qcom_apcs_ipc, mbox);
+	unsigned long idx = (unsigned long)chan->con_priv;
+
+	writel(BIT(idx), apcs->base + apcs->offset);
+
+	return 0;
+}
+
+static const struct mbox_chan_ops qcom_apcs_ipc_ops = {
+	.send_data = qcom_apcs_ipc_send_data,
+};
+
+static int qcom_apcs_ipc_probe(struct platform_device *pdev)
+{
+	struct qcom_apcs_ipc *apcs;
+	struct resource *res;
+	unsigned long i;
+	int ret;
+
+	apcs = devm_kzalloc(&pdev->dev, sizeof(*apcs), GFP_KERNEL);
+	if (!apcs)
+		return -ENOMEM;
+
+	apcs->dev = &pdev->dev;
+	apcs->offset = (unsigned long)of_device_get_match_data(&pdev->dev);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	apcs->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(apcs->base))
+		return PTR_ERR(apcs->base);
+
+	/* Initialize channel identifiers */
+	for (i = 0; i < ARRAY_SIZE(apcs->mbox_chans); i++)
+		apcs->mbox_chans[i].con_priv = (void *)i;
+
+	apcs->mbox.dev = &pdev->dev;
+	apcs->mbox.ops = &qcom_apcs_ipc_ops;
+	apcs->mbox.chans = apcs->mbox_chans;
+	apcs->mbox.num_chans = ARRAY_SIZE(apcs->mbox_chans);
+
+	ret = mbox_controller_register(&apcs->mbox);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register APCS IPC controller\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, apcs);
+
+	return 0;
+}
+
+static int qcom_apcs_ipc_remove(struct platform_device *pdev)
+{
+	struct qcom_apcs_ipc *apcs = platform_get_drvdata(pdev);
+
+	mbox_controller_unregister(&apcs->mbox);
+
+	return 0;
+}
+
+/* .data is the offset of the ipc register within the global block */
+static const struct of_device_id qcom_apcs_ipc_of_match[] = {
+	{ .compatible = "qcom,msm8916-apcs-kpss-global", .data = (void *)8 },
+	{ .compatible = "qcom,msm8996-apcs-hmss-global", .data = (void *)16 },
+	{}
+};
+MODULE_DEVICE_TABLE(of, qcom_apcs_ipc_of_match);
+
+static struct platform_driver qcom_apcs_ipc_driver = {
+	.probe = qcom_apcs_ipc_probe,
+	.remove = qcom_apcs_ipc_remove,
+	.driver = {
+		.name = "qcom_apcs_ipc",
+		.of_match_table = qcom_apcs_ipc_of_match,
+	},
+};
+
+static int __init qcom_apcs_ipc_init(void)
+{
+	return platform_driver_register(&qcom_apcs_ipc_driver);
+}
+postcore_initcall(qcom_apcs_ipc_init);
+
+static void __exit qcom_apcs_ipc_exit(void)
+{
+	platform_driver_unregister(&qcom_apcs_ipc_driver);
+}
+module_exit(qcom_apcs_ipc_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Qualcomm APCS IPC driver");
-- 
2.12.0

[PATCH v4 4/5] soc: qcom: Add device tree binding for GLINK RPM

[Bjorn Andersson]

Add device tree binding documentation for the Qualcomm GLINK RPM, used
for communication with the Resource Power Management subsystem in
various Qualcomm SoCs.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v3:
- Transition doorbell to mailbox framework

Changes since v2:
- Replace qcom,ipc syscon with a "doorbell"

Changes since v1:
- None

 .../devicetree/bindings/soc/qcom/qcom,glink.txt    | 73 ++++++++++++++++++++++
 1 file changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
new file mode 100644
index 000000000000..e32198df0e9c
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
@@ -0,0 +1,73 @@
+Qualcomm RPM GLINK binding
+
+This binding describes the Qualcomm RPM GLINK, a fifo based mechanism for
+communication with the Resource Power Management system on various Qualcomm
+platforms.
+
+- compatible:
+	Usage: required
+	Value type: <stringlist>
+	Definition: must be "qcom,glink-rpm"
+
+- interrupts:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: should specify the IRQ used by the remote processor to
+		    signal this processor about communication related events
+
+- qcom,rpm-msg-ram:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: handle to RPM message memory resource
+
+- mboxes:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: reference to the "rpm_hlos" mailbox in APCS, as described
+		    in mailbox/mailbox.txt
+
+= GLINK DEVICES
+Each subnode of the GLINK node represent function tied to a virtual
+communication channel. The name of the nodes are not important. The properties
+of these nodes are defined by the individual bindings for the specific function
+- but must contain the following property:
+
+- qcom,glink-channels:
+	Usage: required
+	Value type: <stringlist>
+	Definition: a list of channels tied to this function, used for matching
+		    the function to a set of virtual channels
+
+= EXAMPLE
+The following example represents the GLINK RPM node on a MSM8996 device, with
+the function for the "rpm_request" channel defined, which is used for
+regualtors and root clocks.
+
+	apcs_glb: apcs-glb@9820000 {
+		compatible = "qcom,msm8996-apcs-hmss-global";
+		reg = <0x9820000 0x1000>;
+
+		#mbox-cells = <1>;
+	};
+
+	rpm_msg_ram: memory@68000 {
+		compatible = "qcom,rpm-msg-ram";
+		reg = <0x68000 0x6000>;
+	};
+
+	rpm-glink {
+		compatible = "qcom,glink-rpm";
+
+		interrupts = <GIC_SPI 168 IRQ_TYPE_EDGE_RISING>;
+
+		qcom,rpm-msg-ram = <&rpm_msg_ram>;
+
+		mboxes = <&apcs_glb 0>;
+
+		rpm-requests {
+			compatible = "qcom,rpm-msm8996";
+			qcom,glink-channels = "rpm_requests";
+
+			...
+		};
+	};
-- 
2.12.0

[PATCH v4 5/5] rpmsg: Introduce Qualcomm RPM glink driver

[Bjorn Andersson]

This introduces a basic driver for communicating over "native glink"
with the RPM found in Qualcomm platforms.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v3:
- Transition doorbell to mailbox framework

Changes since v2:
- Replace syscon with apcs_ipc doorbell implementation

Changes since v1:
- Depend on HAS_IOMEM, for UM build failure
- Wrap read/write indices on >= size, to keep the values valid when message
  aligns with end of fifo.

 drivers/rpmsg/Kconfig          |   10 +
 drivers/rpmsg/Makefile         |    1 +
 drivers/rpmsg/qcom_glink_rpm.c | 1230 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 1241 insertions(+)
 create mode 100644 drivers/rpmsg/qcom_glink_rpm.c

diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
index f12ac0b28263..449d43511c92 100644
--- a/drivers/rpmsg/Kconfig
+++ b/drivers/rpmsg/Kconfig
@@ -13,6 +13,16 @@ config RPMSG_CHAR
 	  in /dev. They make it possible for user-space programs to send and
 	  receive rpmsg packets.
 
+config RPMSG_QCOM_GLINK_RPM
+	tristate "Qualcomm RPM Glink driver"
+	select RPMSG
+	depends on HAS_IOMEM
+	depends on QCOM_APCS_IPC
+	help
+	  Say y here to enable support for the GLINK RPM communication driver,
+	  which serves as a channel for communication with the RPM in GLINK
+	  enabled systems.
+
 config RPMSG_QCOM_SMD
 	tristate "Qualcomm Shared Memory Driver (SMD)"
 	depends on QCOM_SMEM
diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
index fae9a6d548fb..28cc19088cc0 100644
--- a/drivers/rpmsg/Makefile
+++ b/drivers/rpmsg/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_RPMSG)		+= rpmsg_core.o
 obj-$(CONFIG_RPMSG_CHAR)	+= rpmsg_char.o
+obj-$(CONFIG_RPMSG_QCOM_GLINK_RPM) += qcom_glink_rpm.o
 obj-$(CONFIG_RPMSG_QCOM_SMD)	+= qcom_smd.o
 obj-$(CONFIG_RPMSG_VIRTIO)	+= virtio_rpmsg_bus.o
diff --git a/drivers/rpmsg/qcom_glink_rpm.c b/drivers/rpmsg/qcom_glink_rpm.c
new file mode 100644
index 000000000000..e7d718387a1e
--- /dev/null
+++ b/drivers/rpmsg/qcom_glink_rpm.c
@@ -0,0 +1,1230 @@
+/*
+ * Copyright (c) 2016-2017, Linaro Ltd
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/idr.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/list.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/rpmsg.h>
+#include <linux/slab.h>
+#include <linux/workqueue.h>
+#include <linux/mailbox_client.h>
+
+#include "rpmsg_internal.h"
+
+#define RPM_TOC_SIZE		256
+#define RPM_TOC_MAGIC		0x67727430 /* grt0 */
+#define RPM_TOC_MAX_ENTRIES	((RPM_TOC_SIZE - sizeof(struct rpm_toc)) / \
+				 sizeof(struct rpm_toc_entry))
+
+#define RPM_TX_FIFO_ID		0x61703272 /* ap2r */
+#define RPM_RX_FIFO_ID		0x72326170 /* r2ap */
+
+#define GLINK_NAME_SIZE		32
+
+#define RPM_GLINK_CID_MIN	1
+#define RPM_GLINK_CID_MAX	65536
+
+struct rpm_toc_entry {
+	__le32 id;
+	__le32 offset;
+	__le32 size;
+} __packed;
+
+struct rpm_toc {
+	__le32 magic;
+	__le32 count;
+
+	struct rpm_toc_entry entries[];
+} __packed;
+
+struct glink_msg {
+	__le16 cmd;
+	__le16 param1;
+	__le32 param2;
+	u8 data[];
+} __packed;
+
+struct glink_rpm_pipe {
+	void __iomem *tail;
+	void __iomem *head;
+
+	void __iomem *fifo;
+
+	size_t length;
+};
+
+/**
+ * struct glink_defer_cmd - deferred incoming control message
+ * @node:	list node
+ * @msg:	message header
+ * data:	payload of the message
+ *
+ * Copy of a received control message, to be added to @rx_queue and processed
+ * by @rx_work of @glink_rpm.
+ */
+struct glink_defer_cmd {
+	struct list_head node;
+
+	struct glink_msg msg;
+	u8 data[];
+};
+
+/**
+ * struct glink_rpm - driver context, relates to one remote subsystem
+ * @dev:	reference to the associated struct device
+ * @doorbell:	"rpm_hlos" ipc doorbell
+ * @rx_pipe:	pipe object for receive FIFO
+ * @tx_pipe:	pipe object for transmit FIFO
+ * @irq:	IRQ for signaling incoming events
+ * @rx_work:	worker for handling received control messages
+ * @rx_lock:	protects the @rx_queue
+ * @rx_queue:	queue of received control messages to be processed in @rx_work
+ * @tx_lock:	synchronizes operations on the tx fifo
+ * @idr_lock:	synchronizes @lcids and @rcids modifications
+ * @lcids:	idr of all channels with a known local channel id
+ * @rcids:	idr of all channels with a known remote channel id
+ */
+struct glink_rpm {
+	struct device *dev;
+
+	struct mbox_client mbox_client;
+	struct mbox_chan *mbox_chan;
+
+	struct glink_rpm_pipe rx_pipe;
+	struct glink_rpm_pipe tx_pipe;
+
+	int irq;
+
+	struct work_struct rx_work;
+	spinlock_t rx_lock;
+	struct list_head rx_queue;
+
+	struct mutex tx_lock;
+
+	struct mutex idr_lock;
+	struct idr lcids;
+	struct idr rcids;
+};
+
+enum {
+	GLINK_STATE_CLOSED,
+	GLINK_STATE_OPENING,
+	GLINK_STATE_OPEN,
+	GLINK_STATE_CLOSING,
+};
+
+/**
+ * struct glink_channel - internal representation of a channel
+ * @rpdev:	rpdev reference, only used for primary endpoints
+ * @ept:	rpmsg endpoint this channel is associated with
+ * @glink:	glink_rpm context handle
+ * @refcount:	refcount for the channel object
+ * @recv_lock:	guard for @ept.cb
+ * @name:	unique channel name/identifier
+ * @lcid:	channel id, in local space
+ * @rcid:	channel id, in remote space
+ * @buf:	receive buffer, for gathering fragments
+ * @buf_offset:	write offset in @buf
+ * @buf_size:	size of current @buf
+ * @open_ack:	completed once remote has acked the open-request
+ * @open_req:	completed once open-request has been received
+ */
+struct glink_channel {
+	struct rpmsg_endpoint ept;
+
+	struct rpmsg_device *rpdev;
+	struct glink_rpm *glink;
+
+	struct kref refcount;
+
+	spinlock_t recv_lock;
+
+	char *name;
+	unsigned int lcid;
+	unsigned int rcid;
+
+	void *buf;
+	int buf_offset;
+	int buf_size;
+
+	struct completion open_ack;
+	struct completion open_req;
+};
+
+#define to_glink_channel(_ept) container_of(_ept, struct glink_channel, ept)
+
+static const struct rpmsg_endpoint_ops glink_endpoint_ops;
+
+#define RPM_CMD_VERSION			0
+#define RPM_CMD_VERSION_ACK		1
+#define RPM_CMD_OPEN			2
+#define RPM_CMD_CLOSE			3
+#define RPM_CMD_OPEN_ACK		4
+#define RPM_CMD_TX_DATA			9
+#define RPM_CMD_CLOSE_ACK		11
+#define RPM_CMD_TX_DATA_CONT		12
+#define RPM_CMD_READ_NOTIF		13
+
+#define GLINK_FEATURE_INTENTLESS	BIT(1)
+
+static struct glink_channel *glink_rpm_alloc_channel(struct glink_rpm *glink,
+						     const char *name)
+{
+	struct glink_channel *channel;
+
+	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
+	if (!channel)
+		return ERR_PTR(-ENOMEM);
+
+	/* Setup glink internal glink_channel data */
+	spin_lock_init(&channel->recv_lock);
+	channel->glink = glink;
+	channel->name = kstrdup(name, GFP_KERNEL);
+
+	init_completion(&channel->open_req);
+	init_completion(&channel->open_ack);
+
+	kref_init(&channel->refcount);
+
+	return channel;
+}
+
+static void glink_rpm_channel_release(struct kref *ref)
+{
+	struct glink_channel *channel = container_of(ref, struct glink_channel,
+						     refcount);
+
+	kfree(channel->name);
+	kfree(channel);
+}
+
+static size_t glink_rpm_rx_avail(struct glink_rpm *glink)
+{
+	struct glink_rpm_pipe *pipe = &glink->rx_pipe;
+	unsigned int head;
+	unsigned int tail;
+
+	head = readl(pipe->head);
+	tail = readl(pipe->tail);
+
+	if (head < tail)
+		return pipe->length - tail + head;
+	else
+		return head - tail;
+}
+
+static void glink_rpm_rx_peak(struct glink_rpm *glink,
+			      void *data, size_t count)
+{
+	struct glink_rpm_pipe *pipe = &glink->rx_pipe;
+	unsigned int tail;
+	size_t len;
+
+	tail = readl(pipe->tail);
+
+	len = min_t(size_t, count, pipe->length - tail);
+	if (len) {
+		__ioread32_copy(data, pipe->fifo + tail,
+				len / sizeof(u32));
+	}
+
+	if (len != count) {
+		__ioread32_copy(data + len, pipe->fifo,
+				(count - len) / sizeof(u32));
+	}
+}
+
+static void glink_rpm_rx_advance(struct glink_rpm *glink,
+				 size_t count)
+{
+	struct glink_rpm_pipe *pipe = &glink->rx_pipe;
+	unsigned int tail;
+
+	tail = readl(pipe->tail);
+
+	tail += count;
+	if (tail >= pipe->length)
+		tail -= pipe->length;
+
+	writel(tail, pipe->tail);
+}
+
+static size_t glink_rpm_tx_avail(struct glink_rpm *glink)
+{
+	struct glink_rpm_pipe *pipe = &glink->tx_pipe;
+	unsigned int head;
+	unsigned int tail;
+
+	head = readl(pipe->head);
+	tail = readl(pipe->tail);
+
+	if (tail <= head)
+		return pipe->length - head + tail;
+	else
+		return tail - head;
+}
+
+static unsigned int glink_rpm_tx_write(struct glink_rpm *glink,
+				       unsigned int head,
+				       const void *data, size_t count)
+{
+	struct glink_rpm_pipe *pipe = &glink->tx_pipe;
+	size_t len;
+
+	len = min_t(size_t, count, pipe->length - head);
+	if (len) {
+		__iowrite32_copy(pipe->fifo + head, data,
+				 len / sizeof(u32));
+	}
+
+	if (len != count) {
+		__iowrite32_copy(pipe->fifo, data + len,
+				 (count - len) / sizeof(u32));
+	}
+
+	head += count;
+	if (head >= pipe->length)
+		head -= pipe->length;
+
+	return head;
+}
+
+static int glink_rpm_tx(struct glink_rpm *glink,
+			const void *hdr, size_t hlen,
+			const void *data, size_t dlen, bool wait)
+{
+	struct glink_rpm_pipe *pipe = &glink->tx_pipe;
+	unsigned int head;
+	unsigned int tlen = hlen + dlen;
+	int ret;
+
+	/* Reject packets that are too big */
+	if (tlen >= glink->tx_pipe.length)
+		return -EINVAL;
+
+	if (WARN(tlen % 8, "Unaligned TX request"))
+		return -EINVAL;
+
+	ret = mutex_lock_interruptible(&glink->tx_lock);
+	if (ret)
+		return ret;
+
+	while (glink_rpm_tx_avail(glink) < tlen) {
+		if (!wait) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		msleep(10);
+	}
+
+	head = readl(pipe->head);
+	head = glink_rpm_tx_write(glink, head, hdr, hlen);
+	head = glink_rpm_tx_write(glink, head, data, dlen);
+	writel(head, pipe->head);
+
+	mbox_send_message(glink->mbox_chan, NULL);
+
+out:
+	mutex_unlock(&glink->tx_lock);
+
+	return ret;
+}
+
+static int glink_rpm_send_version(struct glink_rpm *glink)
+{
+	struct glink_msg msg;
+
+	msg.cmd = cpu_to_le16(RPM_CMD_VERSION);
+	msg.param1 = cpu_to_le16(1);
+	msg.param2 = cpu_to_le32(GLINK_FEATURE_INTENTLESS);
+
+	return glink_rpm_tx(glink, &msg, sizeof(msg), NULL, 0, true);
+}
+
+static void glink_rpm_send_version_ack(struct glink_rpm *glink)
+{
+	struct glink_msg msg;
+
+	msg.cmd = cpu_to_le16(RPM_CMD_VERSION_ACK);
+	msg.param1 = cpu_to_le16(1);
+	msg.param2 = cpu_to_le32(0);
+
+	glink_rpm_tx(glink, &msg, sizeof(msg), NULL, 0, true);
+}
+
+static void glink_rpm_send_open_ack(struct glink_rpm *glink,
+					 struct glink_channel *channel)
+{
+	struct glink_msg msg;
+
+	msg.cmd = cpu_to_le16(RPM_CMD_OPEN_ACK);
+	msg.param1 = cpu_to_le16(channel->rcid);
+	msg.param2 = cpu_to_le32(0);
+
+	glink_rpm_tx(glink, &msg, sizeof(msg), NULL, 0, true);
+}
+
+/**
+ * glink_rpm_send_open_req() - send a RPM_CMD_OPEN request to the remote
+ * @glink:
+ * @channel:
+ *
+ * Allocates a local channel id and sends a RPM_CMD_OPEN message to the remote.
+ * Will return with refcount held, regardless of outcome.
+ *
+ * Returns 0 on success, negative errno otherwise.
+ */
+static int glink_rpm_send_open_req(struct glink_rpm *glink,
+					 struct glink_channel *channel)
+{
+	struct {
+		struct glink_msg msg;
+		u8 name[GLINK_NAME_SIZE];
+	} __packed req;
+	int name_len = strlen(channel->name) + 1;
+	int req_len = ALIGN(sizeof(req.msg) + name_len, 8);
+	int ret;
+
+	kref_get(&channel->refcount);
+
+	mutex_lock(&glink->idr_lock);
+	ret = idr_alloc_cyclic(&glink->lcids, channel,
+			       RPM_GLINK_CID_MIN, RPM_GLINK_CID_MAX, GFP_KERNEL);
+	mutex_unlock(&glink->idr_lock);
+	if (ret < 0)
+		return ret;
+
+	channel->lcid = ret;
+
+	req.msg.cmd = cpu_to_le16(RPM_CMD_OPEN);
+	req.msg.param1 = cpu_to_le16(channel->lcid);
+	req.msg.param2 = cpu_to_le32(name_len);
+	strcpy(req.name, channel->name);
+
+	ret = glink_rpm_tx(glink, &req, req_len, NULL, 0, true);
+	if (ret)
+		goto remove_idr;
+
+	return 0;
+
+remove_idr:
+	mutex_lock(&glink->idr_lock);
+	idr_remove(&glink->lcids, channel->lcid);
+	channel->lcid = 0;
+	mutex_unlock(&glink->idr_lock);
+
+	return ret;
+}
+
+static void glink_rpm_send_close_req(struct glink_rpm *glink,
+					  struct glink_channel *channel)
+{
+	struct glink_msg req;
+
+	req.cmd = cpu_to_le16(RPM_CMD_CLOSE);
+	req.param1 = cpu_to_le16(channel->lcid);
+	req.param2 = 0;
+
+	glink_rpm_tx(glink, &req, sizeof(req), NULL, 0, true);
+}
+
+static void glink_rpm_send_close_ack(struct glink_rpm *glink, unsigned int rcid)
+{
+	struct glink_msg req;
+
+	req.cmd = cpu_to_le16(RPM_CMD_CLOSE_ACK);
+	req.param1 = cpu_to_le16(rcid);
+	req.param2 = 0;
+
+	glink_rpm_tx(glink, &req, sizeof(req), NULL, 0, true);
+}
+
+static int glink_rpm_rx_defer(struct glink_rpm *glink, size_t extra)
+{
+	struct glink_defer_cmd *dcmd;
+
+	extra = ALIGN(extra, 8);
+
+	if (glink_rpm_rx_avail(glink) < sizeof(struct glink_msg) + extra) {
+		dev_dbg(glink->dev, "Insufficient data in rx fifo");
+		return -ENXIO;
+	}
+
+	dcmd = kzalloc(sizeof(*dcmd) + extra, GFP_ATOMIC);
+	if (!dcmd)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&dcmd->node);
+
+	glink_rpm_rx_peak(glink, &dcmd->msg, sizeof(dcmd->msg) + extra);
+
+	spin_lock(&glink->rx_lock);
+	list_add_tail(&dcmd->node, &glink->rx_queue);
+	spin_unlock(&glink->rx_lock);
+
+	schedule_work(&glink->rx_work);
+	glink_rpm_rx_advance(glink, sizeof(dcmd->msg) + extra);
+
+	return 0;
+}
+
+static int glink_rpm_rx_data(struct glink_rpm *glink, size_t avail)
+{
+	struct glink_channel *channel;
+	struct {
+		struct glink_msg msg;
+		__le32 chunk_size;
+		__le32 left_size;
+	} __packed hdr;
+	unsigned int chunk_size;
+	unsigned int left_size;
+	unsigned int rcid;
+
+	if (avail < sizeof(hdr)) {
+		dev_dbg(glink->dev, "Not enough data in fifo\n");
+		return -EAGAIN;
+	}
+
+	glink_rpm_rx_peak(glink, &hdr, sizeof(hdr));
+	chunk_size = le32_to_cpu(hdr.chunk_size);
+	left_size = le32_to_cpu(hdr.left_size);
+
+	if (avail < sizeof(hdr) + chunk_size) {
+		dev_dbg(glink->dev, "Payload not yet in fifo\n");
+		return -EAGAIN;
+	}
+
+	if (WARN(chunk_size % 4, "Incoming data must be word aligned\n"))
+		return -EINVAL;
+
+	rcid = le16_to_cpu(hdr.msg.param1);
+	channel = idr_find(&glink->rcids, rcid);
+	if (!channel) {
+		dev_dbg(glink->dev, "Data on non-existing channel\n");
+
+		/* Drop the message */
+		glink_rpm_rx_advance(glink, ALIGN(sizeof(hdr) + chunk_size, 8));
+		return 0;
+	}
+
+	/* Might have an ongoing, fragmented, message to append */
+	if (!channel->buf) {
+		channel->buf = kmalloc(chunk_size + left_size, GFP_ATOMIC);
+		if (!channel->buf)
+			return -ENOMEM;
+
+		channel->buf_size = chunk_size + left_size;
+		channel->buf_offset = 0;
+	}
+
+	glink_rpm_rx_advance(glink, sizeof(hdr));
+
+	if (channel->buf_size - channel->buf_offset < chunk_size) {
+		dev_err(glink->dev, "Insufficient space in input buffer\n");
+
+		/* The packet header lied, drop payload */
+		glink_rpm_rx_advance(glink, chunk_size);
+		return -ENOMEM;
+	}
+
+	glink_rpm_rx_peak(glink, channel->buf + channel->buf_offset, chunk_size);
+	channel->buf_offset += chunk_size;
+
+	/* Handle message when no fragments remain to be received */
+	if (!left_size) {
+		spin_lock(&channel->recv_lock);
+		if (channel->ept.cb) {
+			channel->ept.cb(channel->ept.rpdev,
+					channel->buf,
+					channel->buf_offset,
+					channel->ept.priv,
+					RPMSG_ADDR_ANY);
+		}
+		spin_unlock(&channel->recv_lock);
+
+		kfree(channel->buf);
+		channel->buf = NULL;
+		channel->buf_size = 0;
+	}
+
+	/* Each message starts at 8 byte aligned address */
+	glink_rpm_rx_advance(glink, ALIGN(chunk_size, 8));
+
+	return 0;
+}
+
+static int glink_rpm_rx_open_ack(struct glink_rpm *glink, unsigned int lcid)
+{
+	struct glink_channel *channel;
+
+	channel = idr_find(&glink->lcids, lcid);
+	if (!channel) {
+		dev_err(glink->dev, "Invalid open ack packet\n");
+		return -EINVAL;
+	}
+
+	complete(&channel->open_ack);
+
+	return 0;
+}
+
+static irqreturn_t glink_rpm_intr(int irq, void *data)
+{
+	struct glink_rpm *glink = data;
+	struct glink_msg msg;
+	unsigned int param1;
+	unsigned int param2;
+	unsigned int avail;
+	unsigned int cmd;
+	int ret;
+
+	for (;;) {
+		avail = glink_rpm_rx_avail(glink);
+		if (avail < sizeof(msg))
+			break;
+
+		glink_rpm_rx_peak(glink, &msg, sizeof(msg));
+
+		cmd = le16_to_cpu(msg.cmd);
+		param1 = le16_to_cpu(msg.param1);
+		param2 = le32_to_cpu(msg.param2);
+
+		switch (cmd) {
+		case RPM_CMD_VERSION:
+		case RPM_CMD_VERSION_ACK:
+		case RPM_CMD_CLOSE:
+		case RPM_CMD_CLOSE_ACK:
+			ret = glink_rpm_rx_defer(glink, 0);
+			break;
+		case RPM_CMD_OPEN_ACK:
+			ret = glink_rpm_rx_open_ack(glink, param1);
+			glink_rpm_rx_advance(glink, ALIGN(sizeof(msg), 8));
+			break;
+		case RPM_CMD_OPEN:
+			ret = glink_rpm_rx_defer(glink, param2);
+			break;
+		case RPM_CMD_TX_DATA:
+		case RPM_CMD_TX_DATA_CONT:
+			ret = glink_rpm_rx_data(glink, avail);
+			break;
+		case RPM_CMD_READ_NOTIF:
+			glink_rpm_rx_advance(glink, ALIGN(sizeof(msg), 8));
+			mbox_send_message(glink->mbox_chan, NULL);
+
+			ret = 0;
+			break;
+		default:
+			dev_err(glink->dev, "unhandled rx cmd: %d\n", cmd);
+			ret = -EINVAL;
+			break;
+		}
+
+		if (ret)
+			break;
+	}
+
+	return IRQ_HANDLED;
+}
+
+/* Locally initiated rpmsg_create_ept */
+static struct glink_channel *glink_rpm_create_local(struct glink_rpm *glink,
+						    const char *name)
+{
+	struct glink_channel *channel;
+	int ret;
+
+	channel = glink_rpm_alloc_channel(glink, name);
+	if (IS_ERR(channel))
+		return ERR_CAST(channel);
+
+	ret = glink_rpm_send_open_req(glink, channel);
+	if (ret)
+		goto release_channel;
+
+	ret = wait_for_completion_timeout(&channel->open_ack, 5 * HZ);
+	if (!ret)
+		goto err_timeout;
+
+	ret = wait_for_completion_timeout(&channel->open_req, 5 * HZ);
+	if (!ret)
+		goto err_timeout;
+
+	glink_rpm_send_open_ack(glink, channel);
+
+	return channel;
+
+err_timeout:
+	/* glink_rpm_send_open_req() did register the channel in lcids*/
+	mutex_lock(&glink->idr_lock);
+	idr_remove(&glink->lcids, channel->lcid);
+	mutex_unlock(&glink->idr_lock);
+
+release_channel:
+	/* Release glink_rpm_send_open_req() reference */
+	kref_put(&channel->refcount, glink_rpm_channel_release);
+	/* Release glink_rpm_alloc_channel() reference */
+	kref_put(&channel->refcount, glink_rpm_channel_release);
+
+	return ERR_PTR(-ETIMEDOUT);
+}
+
+/* Remote initiated rpmsg_create_ept */
+static int glink_rpm_create_remote(struct glink_rpm *glink,
+				   struct glink_channel *channel)
+{
+	int ret;
+
+	glink_rpm_send_open_ack(glink, channel);
+
+	ret = glink_rpm_send_open_req(glink, channel);
+	if (ret)
+		goto close_link;
+
+	ret = wait_for_completion_timeout(&channel->open_ack, 5 * HZ);
+	if (!ret) {
+		ret = -ETIMEDOUT;
+		goto close_link;
+	}
+
+	return 0;
+
+close_link:
+	/*
+	 * Send a close request to "undo" our open-ack. The close-ack will
+	 * release the last reference.
+	 */
+	glink_rpm_send_close_req(glink, channel);
+
+	/* Release glink_rpm_send_open_req() reference */
+	kref_put(&channel->refcount, glink_rpm_channel_release);
+
+	return ret;
+}
+
+static struct rpmsg_endpoint *glink_rpm_create_ept(struct rpmsg_device *rpdev,
+						  rpmsg_rx_cb_t cb, void *priv,
+						  struct rpmsg_channel_info chinfo)
+{
+	struct glink_channel *parent = to_glink_channel(rpdev->ept);
+	struct glink_channel *channel;
+	struct glink_rpm *glink = parent->glink;
+	struct rpmsg_endpoint *ept;
+	const char *name = chinfo.name;
+	int cid;
+	int ret;
+
+	idr_for_each_entry(&glink->rcids, channel, cid) {
+		if (!strcmp(channel->name, name))
+			break;
+	}
+
+	if (!channel) {
+		channel = glink_rpm_create_local(glink, name);
+		if (IS_ERR(channel))
+			return NULL;
+	} else {
+		ret = glink_rpm_create_remote(glink, channel);
+		if (ret)
+			return NULL;
+	}
+
+	ept = &channel->ept;
+	ept->rpdev = rpdev;
+	ept->cb = cb;
+	ept->priv = priv;
+	ept->ops = &glink_endpoint_ops;
+
+	return ept;
+}
+
+static void glink_rpm_destroy_ept(struct rpmsg_endpoint *ept)
+{
+	struct glink_channel *channel = to_glink_channel(ept);
+	struct glink_rpm *glink = channel->glink;
+	unsigned long flags;
+
+	spin_lock_irqsave(&channel->recv_lock, flags);
+	channel->ept.cb = NULL;
+	spin_unlock_irqrestore(&channel->recv_lock, flags);
+
+	/* Decouple the potential rpdev from the channel */
+	channel->rpdev = NULL;
+
+	glink_rpm_send_close_req(glink, channel);
+}
+
+static int __glink_rpm_send(struct glink_channel *channel,
+			     void *data, int len, bool wait)
+{
+	struct glink_rpm *glink = channel->glink;
+	struct {
+		struct glink_msg msg;
+		__le32 chunk_size;
+		__le32 left_size;
+	} __packed req;
+
+	if (WARN(len % 8, "RPM GLINK expects 8 byte aligned messages\n"))
+		return -EINVAL;
+
+	req.msg.cmd = cpu_to_le16(RPM_CMD_TX_DATA);
+	req.msg.param1 = cpu_to_le16(channel->lcid);
+	req.msg.param2 = cpu_to_le32(channel->rcid);
+	req.chunk_size = cpu_to_le32(len);
+	req.left_size = cpu_to_le32(0);
+
+	return glink_rpm_tx(glink, &req, sizeof(req), data, len, wait);
+}
+
+static int glink_rpm_send(struct rpmsg_endpoint *ept, void *data, int len)
+{
+	struct glink_channel *channel = to_glink_channel(ept);
+
+	return __glink_rpm_send(channel, data, len, true);
+}
+
+static int glink_rpm_trysend(struct rpmsg_endpoint *ept, void *data, int len)
+{
+	struct glink_channel *channel = to_glink_channel(ept);
+
+	return __glink_rpm_send(channel, data, len, false);
+}
+
+/*
+ * Finds the device_node for the glink child interested in this channel.
+ */
+static struct device_node *glink_rpm_match_channel(struct device_node *node,
+						    const char *channel)
+{
+	struct device_node *child;
+	const char *name;
+	const char *key;
+	int ret;
+
+	for_each_available_child_of_node(node, child) {
+		key = "qcom,glink-channels";
+		ret = of_property_read_string(child, key, &name);
+		if (ret)
+			continue;
+
+		if (strcmp(name, channel) == 0)
+			return child;
+	}
+
+	return NULL;
+}
+
+static const struct rpmsg_device_ops glink_device_ops = {
+	.create_ept = glink_rpm_create_ept,
+};
+
+static const struct rpmsg_endpoint_ops glink_endpoint_ops = {
+	.destroy_ept = glink_rpm_destroy_ept,
+	.send = glink_rpm_send,
+	.trysend = glink_rpm_trysend,
+};
+
+static void glink_rpm_rpdev_release(struct device *dev)
+{
+	struct rpmsg_device *rpdev = to_rpmsg_device(dev);
+	struct glink_channel *channel = to_glink_channel(rpdev->ept);
+
+	channel->rpdev = NULL;
+	kfree(rpdev);
+}
+
+static int glink_rpm_rx_open(struct glink_rpm *glink, unsigned int rcid,
+			     char *name)
+{
+	struct glink_channel *channel;
+	struct rpmsg_device *rpdev;
+	bool create_device = false;
+	int lcid;
+	int ret;
+
+	idr_for_each_entry(&glink->lcids, channel, lcid) {
+		if (!strcmp(channel->name, name))
+			break;
+	}
+
+	if (!channel) {
+		channel = glink_rpm_alloc_channel(glink, name);
+		if (IS_ERR(channel))
+			return PTR_ERR(channel);
+
+		/* The opening dance was initiated by the remote */
+		create_device = true;
+	}
+
+	mutex_lock(&glink->idr_lock);
+	ret = idr_alloc(&glink->rcids, channel, rcid, rcid + 1, GFP_KERNEL);
+	if (ret < 0) {
+		dev_err(glink->dev, "Unable to insert channel into rcid list\n");
+		mutex_unlock(&glink->idr_lock);
+		goto free_channel;
+	}
+	channel->rcid = ret;
+	mutex_unlock(&glink->idr_lock);
+
+	complete(&channel->open_req);
+
+	if (create_device) {
+		rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
+		if (!rpdev) {
+			ret = -ENOMEM;
+			goto rcid_remove;
+		}
+
+		rpdev->ept = &channel->ept;
+		strncpy(rpdev->id.name, name, RPMSG_NAME_SIZE);
+		rpdev->src = RPMSG_ADDR_ANY;
+		rpdev->dst = RPMSG_ADDR_ANY;
+		rpdev->ops = &glink_device_ops;
+
+		rpdev->dev.of_node = glink_rpm_match_channel(glink->dev->of_node, name);
+		rpdev->dev.parent = glink->dev;
+		rpdev->dev.release = glink_rpm_rpdev_release;
+
+		ret = rpmsg_register_device(rpdev);
+		if (ret)
+			goto free_rpdev;
+
+		channel->rpdev = rpdev;
+	}
+
+	return 0;
+
+free_rpdev:
+	kfree(rpdev);
+rcid_remove:
+	mutex_lock(&glink->idr_lock);
+	idr_remove(&glink->rcids, channel->rcid);
+	channel->rcid = 0;
+	mutex_unlock(&glink->idr_lock);
+free_channel:
+	/* Release the reference, iff we took it */
+	if (create_device)
+		kref_put(&channel->refcount, glink_rpm_channel_release);
+
+	return ret;
+}
+
+static void glink_rpm_rx_close(struct glink_rpm *glink, unsigned int rcid)
+{
+	struct rpmsg_channel_info chinfo;
+	struct glink_channel *channel;
+
+	channel = idr_find(&glink->rcids, rcid);
+	if (WARN(!channel, "close request on unknown channel\n"))
+		return;
+
+	if (channel->rpdev) {
+		strncpy(chinfo.name, channel->name, sizeof(chinfo.name));
+		chinfo.src = RPMSG_ADDR_ANY;
+		chinfo.dst = RPMSG_ADDR_ANY;
+
+		rpmsg_unregister_device(glink->dev, &chinfo);
+	}
+
+	glink_rpm_send_close_ack(glink, channel->rcid);
+
+	mutex_lock(&glink->idr_lock);
+	idr_remove(&glink->rcids, channel->rcid);
+	channel->rcid = 0;
+	mutex_unlock(&glink->idr_lock);
+
+	kref_put(&channel->refcount, glink_rpm_channel_release);
+}
+
+static void glink_rpm_rx_close_ack(struct glink_rpm *glink, unsigned int lcid)
+{
+	struct glink_channel *channel;
+
+	channel = idr_find(&glink->lcids, lcid);
+	if (WARN(!channel, "close ack on unknown channel\n"))
+		return;
+
+	mutex_lock(&glink->idr_lock);
+	idr_remove(&glink->lcids, channel->lcid);
+	channel->lcid = 0;
+	mutex_unlock(&glink->idr_lock);
+
+	kref_put(&channel->refcount, glink_rpm_channel_release);
+}
+
+static void glink_rpm_work(struct work_struct *work)
+{
+	struct glink_rpm *glink = container_of(work, struct glink_rpm, rx_work);
+	struct glink_defer_cmd *dcmd;
+	struct glink_msg *msg;
+	unsigned long flags;
+	unsigned int param1;
+	unsigned int param2;
+	unsigned int cmd;
+
+	for (;;) {
+		spin_lock_irqsave(&glink->rx_lock, flags);
+		if (list_empty(&glink->rx_queue)) {
+			spin_unlock_irqrestore(&glink->rx_lock, flags);
+			break;
+		}
+		dcmd = list_first_entry(&glink->rx_queue, struct glink_defer_cmd, node);
+		list_del(&dcmd->node);
+		spin_unlock_irqrestore(&glink->rx_lock, flags);
+
+		msg = &dcmd->msg;
+		cmd = le16_to_cpu(msg->cmd);
+		param1 = le16_to_cpu(msg->param1);
+		param2 = le32_to_cpu(msg->param2);
+
+		switch (cmd) {
+		case RPM_CMD_VERSION:
+			glink_rpm_send_version_ack(glink);
+			break;
+		case RPM_CMD_VERSION_ACK:
+			break;
+		case RPM_CMD_OPEN:
+			glink_rpm_rx_open(glink, param1, msg->data);
+			break;
+		case RPM_CMD_CLOSE:
+			glink_rpm_rx_close(glink, param1);
+			break;
+		case RPM_CMD_CLOSE_ACK:
+			glink_rpm_rx_close_ack(glink, param1);
+			break;
+		default:
+			WARN(1, "Unknown defer object %d\n", cmd);
+			break;
+		}
+
+		kfree(dcmd);
+	}
+}
+
+static int glink_rpm_parse_toc(struct device *dev,
+			       void __iomem *msg_ram,
+			       size_t msg_ram_size,
+			       struct glink_rpm_pipe *rx,
+			       struct glink_rpm_pipe *tx)
+{
+	struct rpm_toc *toc;
+	int num_entries;
+	unsigned int id;
+	size_t offset;
+	size_t size;
+	void *buf;
+	int i;
+
+	buf = kzalloc(RPM_TOC_SIZE, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	__ioread32_copy(buf, msg_ram + msg_ram_size - RPM_TOC_SIZE,
+			RPM_TOC_SIZE / sizeof(u32));
+
+	toc = buf;
+
+	if (le32_to_cpu(toc->magic) != RPM_TOC_MAGIC) {
+		dev_err(dev, "RPM TOC has invalid magic\n");
+		goto err_inval;
+	}
+
+	num_entries = le32_to_cpu(toc->count);
+	if (num_entries > RPM_TOC_MAX_ENTRIES) {
+		dev_err(dev, "Invalid number of toc entries\n");
+		goto err_inval;
+	}
+
+	for (i = 0; i < num_entries; i++) {
+		id = le32_to_cpu(toc->entries[i].id);
+		offset = le32_to_cpu(toc->entries[i].offset);
+		size = le32_to_cpu(toc->entries[i].size);
+
+		if (offset > msg_ram_size || offset + size > msg_ram_size) {
+			dev_err(dev, "TOC entry with invalid size\n");
+			continue;
+		}
+
+		switch (id) {
+		case RPM_RX_FIFO_ID:
+			rx->length = size;
+
+			rx->tail = msg_ram + offset;
+			rx->head = msg_ram + offset + sizeof(u32);
+			rx->fifo = msg_ram + offset + 2 * sizeof(u32);
+			break;
+		case RPM_TX_FIFO_ID:
+			tx->length = size;
+
+			tx->tail = msg_ram + offset;
+			tx->head = msg_ram + offset + sizeof(u32);
+			tx->fifo = msg_ram + offset + 2 * sizeof(u32);
+			break;
+		}
+	}
+
+	if (!rx->fifo || !tx->fifo) {
+		dev_err(dev, "Unable to find rx and tx descriptors\n");
+		goto err_inval;
+	}
+
+	kfree(buf);
+	return 0;
+
+err_inval:
+	kfree(buf);
+	return -EINVAL;
+}
+
+static int glink_rpm_probe(struct platform_device *pdev)
+{
+	struct glink_rpm *glink;
+	struct device_node *np;
+	void __iomem *msg_ram;
+	size_t msg_ram_size;
+	struct device *dev = &pdev->dev;
+	struct resource r;
+	int irq;
+	int ret;
+
+	glink = devm_kzalloc(dev, sizeof(*glink), GFP_KERNEL);
+	if (!glink)
+		return -ENOMEM;
+
+	glink->dev = dev;
+
+	mutex_init(&glink->tx_lock);
+	spin_lock_init(&glink->rx_lock);
+	INIT_LIST_HEAD(&glink->rx_queue);
+	INIT_WORK(&glink->rx_work, glink_rpm_work);
+
+	mutex_init(&glink->idr_lock);
+	idr_init(&glink->lcids);
+	idr_init(&glink->rcids);
+
+	glink->mbox_client.dev = &pdev->dev;
+	glink->mbox_chan = mbox_request_channel(&glink->mbox_client, 0);
+	if (IS_ERR(glink->mbox_chan)) {
+		if (PTR_ERR(glink->mbox_chan) != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "failed to acquire IPC channel\n");
+		return PTR_ERR(glink->mbox_chan);
+	}
+
+	np = of_parse_phandle(dev->of_node, "qcom,rpm-msg-ram", 0);
+	ret = of_address_to_resource(np, 0, &r);
+	of_node_put(np);
+	if (ret)
+		return ret;
+
+	msg_ram = devm_ioremap(dev, r.start, resource_size(&r));
+	msg_ram_size = resource_size(&r);
+	if (!msg_ram)
+		return -ENOMEM;
+
+	ret = glink_rpm_parse_toc(dev, msg_ram, msg_ram_size,
+				  &glink->rx_pipe, &glink->tx_pipe);
+	if (ret)
+		return ret;
+
+	writel(0, glink->tx_pipe.head);
+	writel(0, glink->rx_pipe.tail);
+
+	irq = platform_get_irq(pdev, 0);
+	ret = devm_request_irq(dev, irq,
+			       glink_rpm_intr,
+			       IRQF_NO_SUSPEND | IRQF_SHARED,
+			       "glink-rpm", glink);
+	if (ret) {
+		dev_err(dev, "Failed to request IRQ\n");
+		return ret;
+	}
+
+	glink->irq = irq;
+
+	ret = glink_rpm_send_version(glink);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, glink);
+
+	return 0;
+}
+
+static int glink_rpm_remove_device(struct device *dev, void *data)
+{
+	device_unregister(dev);
+
+	return 0;
+}
+
+static int glink_rpm_remove(struct platform_device *pdev)
+{
+	struct glink_rpm *glink = platform_get_drvdata(pdev);
+	struct glink_channel *channel;
+	int cid;
+	int ret;
+
+	disable_irq(glink->irq);
+	cancel_work_sync(&glink->rx_work);
+
+	ret = device_for_each_child(glink->dev, NULL, glink_rpm_remove_device);
+	if (ret)
+		dev_warn(glink->dev, "Can't remove GLINK devices: %d\n", ret);
+
+	/* Release any defunct local channels, waiting for close-ack */
+	idr_for_each_entry(&glink->lcids, channel, cid)
+		kref_put(&channel->refcount, glink_rpm_channel_release);
+
+	idr_destroy(&glink->lcids);
+	idr_destroy(&glink->rcids);
+
+	return 0;
+}
+
+static const struct of_device_id glink_rpm_of_match[] = {
+	{ .compatible = "qcom,glink-rpm" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, glink_rpm_of_match);
+
+static struct platform_driver glink_rpm_driver = {
+	.probe = glink_rpm_probe,
+	.remove = glink_rpm_remove,
+	.driver = {
+		.name = "qcom_glink_rpm",
+		.of_match_table = glink_rpm_of_match,
+	},
+};
+
+static int __init glink_rpm_init(void)
+{
+	return platform_driver_register(&glink_rpm_driver);
+}
+subsys_initcall(glink_rpm_init);
+
+static void __exit glink_rpm_exit(void)
+{
+	platform_driver_unregister(&glink_rpm_driver);
+}
+module_exit(glink_rpm_exit);
+
+MODULE_AUTHOR("Bjorn Andersson <bjorn.andersson@linaro.org>");
+MODULE_DESCRIPTION("Qualcomm GLINK RPM driver");
+MODULE_LICENSE("GPL v2");
-- 
2.12.0

Re: [PATCH v4 1/5] mailbox: Make startup and shutdown ops optional

[Sudeep Holla]

On 04/05/17 21:05, Bjorn Andersson wrote:
> Some mailbox hardware doesn't have to perform any additional operations
> on startup of shutdown, so make these optional.
> 

Quite handy. I can you this to get rid of empty shutdown that I added in[1]

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

-- 
Regards,
Sudeep

[1] https://lkml.org/lkml/2017/5/2/315

Re: [PATCH v4 3/5] soc: qcom: Introduce APCS IPC driver

[Jassi Brar]

On Fri, May 5, 2017 at 1:35 AM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:

> +
> +static int qcom_apcs_ipc_send_data(struct mbox_chan *chan, void *data)
> +{
> +       struct qcom_apcs_ipc *apcs = container_of(chan->mbox,
> +                                                 struct qcom_apcs_ipc, mbox);
> +       unsigned long idx = (unsigned long)chan->con_priv;
> +
> +       writel(BIT(idx), apcs->base + apcs->offset);
> +
When/how does this bit get ever cleared again?
You may want to add last_tx_done() callback to check if this bit is
cleared before you can send the next interrupt. And set
txdone_poll/irq accordingly.

Thanks

Re: [PATCH v4 1/5] mailbox: Make startup and shutdown ops optional

[Jassi Brar]

On Fri, May 5, 2017 at 1:35 AM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> Some mailbox hardware doesn't have to perform any additional operations
> on startup of shutdown, so make these optional.
>
Thanks, makes sense.

> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> index 4671f8a12872..c88de953394a 100644
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -137,6 +137,20 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
>         return HRTIMER_NORESTART;
>  }
>
> +static int mbox_startup(struct mbox_chan *chan)
> +{
> +       if (chan->mbox->ops->startup)
> +               return chan->mbox->ops->startup(chan);
> +
> +       return 0;
> +}
> +
> +static void mbox_shutdown(struct mbox_chan *chan)
> +{
> +       if (chan->mbox->ops->shutdown)
> +               chan->mbox->ops->shutdown(chan);
> +}
> +
These functions are going to be called from exactly one place. So
maybe we simply do the check before startup/shutdown calls?

thanks.

Re: [PATCH v4 1/5] mailbox: Make startup and shutdown ops optional

[Bjorn Andersson]

On Fri 05 May 03:33 PDT 2017, Jassi Brar wrote:

> On Fri, May 5, 2017 at 1:35 AM, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> > Some mailbox hardware doesn't have to perform any additional operations
> > on startup of shutdown, so make these optional.
> >
> Thanks, makes sense.
> 

Thanks

> > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> > index 4671f8a12872..c88de953394a 100644
> > --- a/drivers/mailbox/mailbox.c
> > +++ b/drivers/mailbox/mailbox.c
> > @@ -137,6 +137,20 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
> >         return HRTIMER_NORESTART;
> >  }
> >
> > +static int mbox_startup(struct mbox_chan *chan)
> > +{
> > +       if (chan->mbox->ops->startup)
> > +               return chan->mbox->ops->startup(chan);
> > +
> > +       return 0;
> > +}
> > +
> > +static void mbox_shutdown(struct mbox_chan *chan)
> > +{
> > +       if (chan->mbox->ops->shutdown)
> > +               chan->mbox->ops->shutdown(chan);
> > +}
> > +
> These functions are going to be called from exactly one place. So
> maybe we simply do the check before startup/shutdown calls?
> 

I didn't want to add another indentation level at the bottom of
mbox_request_channel(), but now that I check again it still would stay
below 80 chars and doesn't look too unreasonable.

The shutdown() was moved out just to get some symmetry.

I'll inline the two checks and resend this together with the driver,
once we have concluded on your question/comment there.

Regards,
Bjorn

Re: [PATCH v4 3/5] soc: qcom: Introduce APCS IPC driver

[Bjorn Andersson]

On Fri 05 May 03:26 PDT 2017, Jassi Brar wrote:

> On Fri, May 5, 2017 at 1:35 AM, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> 
> > +
> > +static int qcom_apcs_ipc_send_data(struct mbox_chan *chan, void *data)
> > +{
> > +       struct qcom_apcs_ipc *apcs = container_of(chan->mbox,
> > +                                                 struct qcom_apcs_ipc, mbox);
> > +       unsigned long idx = (unsigned long)chan->con_priv;
> > +
> > +       writel(BIT(idx), apcs->base + apcs->offset);
> > +
> When/how does this bit get ever cleared again?
> You may want to add last_tx_done() callback to check if this bit is
> cleared before you can send the next interrupt. And set
> txdone_poll/irq accordingly.
> 

It's a write-only register, writing a bit fires off an edge triggered
interrupt on the specific remote processor, which will ack the
associated IRQ status and handle the interrupt.

As the "message" is just a notification to the other side that it needs
to act on "something", there's no harm in notifying it multiple times
before it has a chance to ack the IRQ and a write after that will be
seen as a separate interrupt.

Regards,
Bjorn

Re: [PATCH v4 3/5] soc: qcom: Introduce APCS IPC driver

[Jassi Brar]

On Sat, May 6, 2017 at 12:07 AM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Fri 05 May 03:26 PDT 2017, Jassi Brar wrote:
>
>> On Fri, May 5, 2017 at 1:35 AM, Bjorn Andersson
>> <bjorn.andersson@linaro.org> wrote:
>>
>> > +
>> > +static int qcom_apcs_ipc_send_data(struct mbox_chan *chan, void *data)
>> > +{
>> > +       struct qcom_apcs_ipc *apcs = container_of(chan->mbox,
>> > +                                                 struct qcom_apcs_ipc, mbox);
>> > +       unsigned long idx = (unsigned long)chan->con_priv;
>> > +
>> > +       writel(BIT(idx), apcs->base + apcs->offset);
>> > +
>> When/how does this bit get ever cleared again?
>> You may want to add last_tx_done() callback to check if this bit is
>> cleared before you can send the next interrupt. And set
>> txdone_poll/irq accordingly.
>>
>
> It's a write-only register, writing a bit fires off an edge triggered
> interrupt on the specific remote processor, which will ack the
> associated IRQ status and handle the interrupt.
>
> As the "message" is just a notification to the other side that it needs
> to act on "something", there's no harm in notifying it multiple times
> before it has a chance to ack the IRQ and a write after that will be
> seen as a separate interrupt.
>
What causes it to return to '0'?

I think the driver should wait for it to become 0 before writing 1.
For example, the protocol has a command that says to remote cpu to
increase the voltage supply by 0.1v. This command is filled in a
structure and laid out in the shared memory before you ring the
'doorbell'.  In this situation you don't want the remote cpu to act
twice on the same command. Also for a new command, you don't want to
overwrite the last command packet before remote cpu has consumed it.

Re: [PATCH v4 3/5] soc: qcom: Introduce APCS IPC driver

[Jeffrey Hugo]

On 5/5/2017 1:22 PM, Jassi Brar wrote:
> On Sat, May 6, 2017 at 12:07 AM, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
>> On Fri 05 May 03:26 PDT 2017, Jassi Brar wrote:
>>
>>> On Fri, May 5, 2017 at 1:35 AM, Bjorn Andersson
>>> <bjorn.andersson@linaro.org> wrote:
>>>
>>>> +
>>>> +static int qcom_apcs_ipc_send_data(struct mbox_chan *chan, void *data)
>>>> +{
>>>> +       struct qcom_apcs_ipc *apcs = container_of(chan->mbox,
>>>> +                                                 struct qcom_apcs_ipc, mbox);
>>>> +       unsigned long idx = (unsigned long)chan->con_priv;
>>>> +
>>>> +       writel(BIT(idx), apcs->base + apcs->offset);
>>>> +
>>> When/how does this bit get ever cleared again?
>>> You may want to add last_tx_done() callback to check if this bit is
>>> cleared before you can send the next interrupt. And set
>>> txdone_poll/irq accordingly.
>>>
>>
>> It's a write-only register, writing a bit fires off an edge triggered
>> interrupt on the specific remote processor, which will ack the
>> associated IRQ status and handle the interrupt.
>>
>> As the "message" is just a notification to the other side that it needs
>> to act on "something", there's no harm in notifying it multiple times
>> before it has a chance to ack the IRQ and a write after that will be
>> seen as a separate interrupt.
>>
> What causes it to return to '0'?

Technically nothing.  This is not a traditional register.  Its an 
address that is modeled as a register that the hardware knows, when 1 is 
written to the specific bit, the hardware is to take a specific action 
(generate the interrupt).

As Bjorn stated, its a write only address.  The hardware documentation 
states not to read it.  On most platforms, if you attempt to read the 
address, you will always get 0, although that is not guaranteed, and 
there have been platforms where attempting to read the address will 
result in a bus hang.

Conceptually, the hardware automatically resets it to "0" in the next 
clock cycle.  Software doesn't need to care.

> 
> I think the driver should wait for it to become 0 before writing 1.
> For example, the protocol has a command that says to remote cpu to
> increase the voltage supply by 0.1v. This command is filled in a
> structure and laid out in the shared memory before you ring the
> 'doorbell'.  In this situation you don't want the remote cpu to act
> twice on the same command. Also for a new command, you don't want to
> overwrite the last command packet before remote cpu has consumed it.

That doesn't apply here.  The doorbell in this case is a signal for the 
remote processor to go look at the FIFO in shared memory.  Since 
interrupts can be lost (ie the local processor sends multiple interrupts 
before the remote processor can service them, so the GIC drops them 
since the interrupt is already pending), the remote processor is 
required to drain the FIFO when it processes the interrupt.

There is no way to determine if the remote processor has observed a 
message, that does not involve pretty trivial race conditions.

I invented this protocol.  From what I've seen (granted I did not do a 
thorough review), Bjorn is doing the correct thing here.

-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v4 3/5] soc: qcom: Introduce APCS IPC driver

[Jassi Brar]

On Sat, May 6, 2017 at 1:23 AM, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
> On 5/5/2017 1:22 PM, Jassi Brar wrote:
>>
>> On Sat, May 6, 2017 at 12:07 AM, Bjorn Andersson
>> <bjorn.andersson@linaro.org> wrote:
>>>
>
> There is no way to determine if the remote processor has observed a message,
> that does not involve pretty trivial race conditions.
>
Thanks for chiming in.
How is it supposed to work if a client queues more than one request?
How do you know when it's ok to overwrite the FIFO and send the next
command?
  Usually if h/w doesn't indicate, we cook up some ack packet for each
command. Otherwise the protocol seems badly broken.

 If there is really nothing that can be done to check delivery of a
message, I'll pick the driver as such. Best of luck :)

Re: [PATCH v4 3/5] soc: qcom: Introduce APCS IPC driver

[Jeffrey Hugo]

On 5/5/2017 2:22 PM, Jassi Brar wrote:
> On Sat, May 6, 2017 at 1:23 AM, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
>> On 5/5/2017 1:22 PM, Jassi Brar wrote:
>>>
>>> On Sat, May 6, 2017 at 12:07 AM, Bjorn Andersson
>>> <bjorn.andersson@linaro.org> wrote:
>>>>
>>
>> There is no way to determine if the remote processor has observed a message,
>> that does not involve pretty trivial race conditions.
>>
> Thanks for chiming in.
> How is it supposed to work if a client queues more than one request?
> How do you know when it's ok to overwrite the FIFO and send the next
> command?
>    Usually if h/w doesn't indicate, we cook up some ack packet for each
> command. Otherwise the protocol seems badly broken.
> 
>   If there is really nothing that can be done to check delivery of a
> message, I'll pick the driver as such. Best of luck :)

The protocol is designed so that we don't need an ack, or confirmation 
of delivery.  Such details are left to the higher level protocol, if 
needed.

The transmitter owns the "head" pointer in the fifo, and the receiver 
owns the "tail" pointer.  The fifo is empty if those pointers are the 
same value.  When the receiver has copied data out of the fifo, it 
advances the tail pointer.  The transmitter must ensure that the head 
pointer never meets the tail pointer through wrap around.

You can kind of check delivery based on the position of the tail 
pointer, but I can tell you from experience, you never really want to do 
that as it never tells you what you really want to know.

-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v4 3/5] soc: qcom: Introduce APCS IPC driver

[Bjorn Andersson]

On Fri 05 May 13:22 PDT 2017, Jassi Brar wrote:

> On Sat, May 6, 2017 at 1:23 AM, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
> > On 5/5/2017 1:22 PM, Jassi Brar wrote:
> >>
> >> On Sat, May 6, 2017 at 12:07 AM, Bjorn Andersson
> >> <bjorn.andersson@linaro.org> wrote:
> >>>
> >
> > There is no way to determine if the remote processor has observed a message,
> > that does not involve pretty trivial race conditions.
> >
> Thanks for chiming in.
> How is it supposed to work if a client queues more than one request?

One such example is found in patch 5 in this series. There are two FIFOs
in shared memory, one in each direction. Each fifo has a index-pair
associated; a write-index is used by the writer to inform the reader
where the valid data in the ring buffer ends and a read-index indicates
to the writer how far behind the read is.

The writer will just push data into the FIFO, each time firing off an
APCS IPC interrupt and when the remote interrupt handler runs it will
consume all the messages from the read-index to the write-index. All
without the need for the reader to signal the writer that it has
received the interrupts.

In the event that the write-index catches up with the read-index a
dedicated flag is set which will cause the reader to signal that the
read-index is updated - allowing the writer to sleep waiting for room in
the FIFO.

> How do you know when it's ok to overwrite the FIFO and send the next
> command?

As long as there's enough space between the write-index and the
read-index we can write.

And we don't need any locks, because the writer is the only one changing
the write-index and the reader is the only one changing the read index.

>   Usually if h/w doesn't indicate, we cook up some ack packet for each
> command. Otherwise the protocol seems badly broken.
> 
>  If there is really nothing that can be done to check delivery of a
> message, I'll pick the driver as such. Best of luck :)

The protocols implemented on top will guarantee this, as needed, so it's
fine :)


I'll update patch 1 and repost patch 1 through 3 for you to merge in the
mailbox tree, patch 4 and 5 I'll pull into the rpmsg tree once I have
received some additional feedback. (We don't have any build time
dependencies between the two, so it's better to merge each driver
through their respective tree)

Thanks,
Bjorn

Re: [PATCH v4 3/5] soc: qcom: Introduce APCS IPC driver

[Jassi Brar]

On Sat, May 6, 2017 at 6:49 AM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Fri 05 May 13:22 PDT 2017, Jassi Brar wrote:

>> How is it supposed to work if a client queues more than one request?
>
> One such example is found in patch 5 in this series. There are two FIFOs
> in shared memory, one in each direction. Each fifo has a index-pair
> associated; a write-index is used by the writer to inform the reader
> where the valid data in the ring buffer ends and a read-index indicates
> to the writer how far behind the read is.
>
> The writer will just push data into the FIFO, each time firing off an
> APCS IPC interrupt and when the remote interrupt handler runs it will
> consume all the messages from the read-index to the write-index. All
> without the need for the reader to signal the writer that it has
> received the interrupts.
>
> In the event that the write-index catches up with the read-index a
> dedicated flag is set which will cause the reader to signal that the
> read-index is updated - allowing the writer to sleep waiting for room in
> the FIFO.
>
Interesting.Just for my enlightenment...

  Where does the writer sleep in the driver? I see it simply sets the
bit and leave.
Such a flag (or head and tail pointers matching) should be checked in
last_tx_done()

If you think RPM will _always_ be ready to accept new messages (though
we have seen that doesn't hold in some situations), then you don't
need last_tx_done. The client should call mbox_client_txdone() after
mbox_send_message().

thnx

Re: [PATCH v4 3/5] soc: qcom: Introduce APCS IPC driver

[Bjorn Andersson]

On Fri 05 May 21:48 PDT 2017, Jassi Brar wrote:

> On Sat, May 6, 2017 at 6:49 AM, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> > On Fri 05 May 13:22 PDT 2017, Jassi Brar wrote:
> 
> >> How is it supposed to work if a client queues more than one request?
> >
> > One such example is found in patch 5 in this series. There are two FIFOs
> > in shared memory, one in each direction. Each fifo has a index-pair
> > associated; a write-index is used by the writer to inform the reader
> > where the valid data in the ring buffer ends and a read-index indicates
> > to the writer how far behind the read is.
> >
> > The writer will just push data into the FIFO, each time firing off an
> > APCS IPC interrupt and when the remote interrupt handler runs it will
> > consume all the messages from the read-index to the write-index. All
> > without the need for the reader to signal the writer that it has
> > received the interrupts.
> >
> > In the event that the write-index catches up with the read-index a
> > dedicated flag is set which will cause the reader to signal that the
> > read-index is updated - allowing the writer to sleep waiting for room in
> > the FIFO.
> >
> Interesting.Just for my enlightenment...
> 
>   Where does the writer sleep in the driver? I see it simply sets the
> bit and leave.
> Such a flag (or head and tail pointers matching) should be checked in
> last_tx_done()
> 

In the case of the FIFO based communication the flow control is
implemented one layer up. You can see this in glink_rpm_tx() (in patch
5/5), where we check to see if there's enough room between the writer
and the reader to store the new message. 

The remote side will process messages and increment the read index,
which will break us out of the loop.


Note that its possible to write any number of messages before invoking
the APCS IPC and there's no harm (beyond wasting a little bit of power)
in invoking it when there's no data written.

> If you think RPM will _always_ be ready to accept new messages (though
> we have seen that doesn't hold in some situations), then you don't
> need last_tx_done.

Whether the remote processor is ready to accept a new message or not is
irrelevant in the sense of APCS IPC. When a client sends the signal over
the APCS IPC it has already determined that there was space, filled in
the shared memory buffers and is now simply invoking the remote handler.


The APCS IPC register serves the basis for all inter-processor
communication in a Qualcomm platform, so it's not only the RPM driver
discussed earlier that uses this. It's also used for other non-FIFO
based communication channels, where the signalled information either
isn't acked at all or acked on a system-level.

But regardless of the protocol implemented ontop, the purpose of the
APCS IPC bit is _only_ to invoke some remote handler to consume some
data, somewhere - the event in itself does not carry any information.

> The client should call mbox_client_txdone() after
> mbox_send_message().

So every time we call mbox_send_message() from any of the client drivers
we also needs to call mbox_client_txdone()?

This seems like an awkward side effect of using the mailbox framework -
which has to be spread out in at least 6 different client drivers :(

Regards,
Bjorn

Re: [PATCH v4 3/5] soc: qcom: Introduce APCS IPC driver

[Jassi Brar]

On Mon, May 8, 2017 at 11:24 AM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Fri 05 May 21:48 PDT 2017, Jassi Brar wrote:
>
> The APCS IPC register serves the basis for all inter-processor
> communication in a Qualcomm platform, so it's not only the RPM driver
> discussed earlier that uses this. It's also used for other non-FIFO
> based communication channels, where the signalled information either
> isn't acked at all or acked on a system-level.
>
Something has to indicate consumption of data or "requested action
taken". Otherwise the protocol is design-wise broken.

> But regardless of the protocol implemented ontop, the purpose of the
> APCS IPC bit is _only_ to invoke some remote handler to consume some
> data, somewhere - the event in itself does not carry any information.
>
Yes, every platform that uses shared-memory works like that. However
there is always something that tells if the command has been acted
upon by the remote. In your case that is the read-pointer movement.

>> The client should call mbox_client_txdone() after
>> mbox_send_message().
>
> So every time we call mbox_send_message() from any of the client drivers
> we also needs to call mbox_client_txdone()?
>
Yes.

> This seems like an awkward side effect of using the mailbox framework -
> which has to be spread out in at least 6 different client drivers :(
>
No. Mailbox or whatever you implement - you must (and do) tick the
state machine to keep the messages moving.
  Best designs have some interrupt occurring when the message has been
consumed by the remote. Some designs have a flag set which needs to be
polled to detect completion. Very few (like yours) that support
neither irq nor polling, have to be driven by the upper protocol layer
by some ack packet (or tracking read/write pointers like you do).
These three cases are denoted by TXDONE_BY_IRQ, TXDONE_BY_POLL and
TXDONE_BY_ACK respectively.

  If no client driver will ever submit a message if there is no space
in FIFO, then you can specify TXDONE_BY_POLL and have last_tx_done()
always return true. That way you don't need to call
mbox_client_txdone().

Re: [PATCH v4 4/5] soc: qcom: Add device tree binding for GLINK RPM

[Rob Herring]

On Thu, May 04, 2017 at 01:05:38PM -0700, Bjorn Andersson wrote:
> Add device tree binding documentation for the Qualcomm GLINK RPM, used
> for communication with the Resource Power Management subsystem in
> various Qualcomm SoCs.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v3:
> - Transition doorbell to mailbox framework
> 
> Changes since v2:
> - Replace qcom,ipc syscon with a "doorbell"
> 
> Changes since v1:
> - None
> 
>  .../devicetree/bindings/soc/qcom/qcom,glink.txt    | 73 ++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
> new file mode 100644
> index 000000000000..e32198df0e9c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
> @@ -0,0 +1,73 @@
> +Qualcomm RPM GLINK binding
> +
> +This binding describes the Qualcomm RPM GLINK, a fifo based mechanism for
> +communication with the Resource Power Management system on various Qualcomm
> +platforms.
> +
> +- compatible:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: must be "qcom,glink-rpm"
> +
> +- interrupts:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: should specify the IRQ used by the remote processor to
> +		    signal this processor about communication related events
> +
> +- qcom,rpm-msg-ram:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: handle to RPM message memory resource
> +
> +- mboxes:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: reference to the "rpm_hlos" mailbox in APCS, as described
> +		    in mailbox/mailbox.txt
> +
> += GLINK DEVICES
> +Each subnode of the GLINK node represent function tied to a virtual
> +communication channel. The name of the nodes are not important. The properties
> +of these nodes are defined by the individual bindings for the specific function
> +- but must contain the following property:
> +
> +- qcom,glink-channels:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: a list of channels tied to this function, used for matching
> +		    the function to a set of virtual channels
> +
> += EXAMPLE
> +The following example represents the GLINK RPM node on a MSM8996 device, with
> +the function for the "rpm_request" channel defined, which is used for
> +regualtors and root clocks.
> +
> +	apcs_glb: apcs-glb@9820000 {

mailbox@... Or does this block do other things?

Otherwise,

Acked-by: Rob Herring <robh@kernel.org>


> +		compatible = "qcom,msm8996-apcs-hmss-global";
> +		reg = <0x9820000 0x1000>;
> +
> +		#mbox-cells = <1>;
> +	};
> +
> +	rpm_msg_ram: memory@68000 {
> +		compatible = "qcom,rpm-msg-ram";
> +		reg = <0x68000 0x6000>;
> +	};
> +
> +	rpm-glink {
> +		compatible = "qcom,glink-rpm";
> +
> +		interrupts = <GIC_SPI 168 IRQ_TYPE_EDGE_RISING>;
> +
> +		qcom,rpm-msg-ram = <&rpm_msg_ram>;
> +
> +		mboxes = <&apcs_glb 0>;
> +
> +		rpm-requests {
> +			compatible = "qcom,rpm-msm8996";
> +			qcom,glink-channels = "rpm_requests";
> +
> +			...
> +		};
> +	};
> -- 
> 2.12.0
> 

Re: [PATCH v4 4/5] soc: qcom: Add device tree binding for GLINK RPM

[Bjorn Andersson]

On Mon 08 May 10:06 PDT 2017, Rob Herring wrote:

> On Thu, May 04, 2017 at 01:05:38PM -0700, Bjorn Andersson wrote:
> > Add device tree binding documentation for the Qualcomm GLINK RPM, used
> > for communication with the Resource Power Management subsystem in
> > various Qualcomm SoCs.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> > 
> > Changes since v3:
> > - Transition doorbell to mailbox framework
> > 
> > Changes since v2:
> > - Replace qcom,ipc syscon with a "doorbell"
> > 
> > Changes since v1:
> > - None
> > 
> >  .../devicetree/bindings/soc/qcom/qcom,glink.txt    | 73 ++++++++++++++++++++++
> >  1 file changed, 73 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
> > new file mode 100644
> > index 000000000000..e32198df0e9c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
> > @@ -0,0 +1,73 @@
> > +Qualcomm RPM GLINK binding
> > +
> > +This binding describes the Qualcomm RPM GLINK, a fifo based mechanism for
> > +communication with the Resource Power Management system on various Qualcomm
> > +platforms.
> > +
> > +- compatible:
> > +	Usage: required
> > +	Value type: <stringlist>
> > +	Definition: must be "qcom,glink-rpm"
> > +
> > +- interrupts:
> > +	Usage: required
> > +	Value type: <prop-encoded-array>
> > +	Definition: should specify the IRQ used by the remote processor to
> > +		    signal this processor about communication related events
> > +
> > +- qcom,rpm-msg-ram:
> > +	Usage: required
> > +	Value type: <prop-encoded-array>
> > +	Definition: handle to RPM message memory resource
> > +
> > +- mboxes:
> > +	Usage: required
> > +	Value type: <prop-encoded-array>
> > +	Definition: reference to the "rpm_hlos" mailbox in APCS, as described
> > +		    in mailbox/mailbox.txt
> > +
> > += GLINK DEVICES
> > +Each subnode of the GLINK node represent function tied to a virtual
> > +communication channel. The name of the nodes are not important. The properties
> > +of these nodes are defined by the individual bindings for the specific function
> > +- but must contain the following property:
> > +
> > +- qcom,glink-channels:
> > +	Usage: required
> > +	Value type: <stringlist>
> > +	Definition: a list of channels tied to this function, used for matching
> > +		    the function to a set of virtual channels
> > +
> > += EXAMPLE
> > +The following example represents the GLINK RPM node on a MSM8996 device, with
> > +the function for the "rpm_request" channel defined, which is used for
> > +regualtors and root clocks.
> > +
> > +	apcs_glb: apcs-glb@9820000 {
> 
> mailbox@... Or does this block do other things?
> 

So far it's just a mailbox, so I'll update this.

> Otherwise,
> 
> Acked-by: Rob Herring <robh@kernel.org>
> 

Thanks,
Bjorn

Re: [PATCH v4 3/5] soc: qcom: Introduce APCS IPC driver

[Bjorn Andersson]

On Sun 07 May 23:47 PDT 2017, Jassi Brar wrote:

> On Mon, May 8, 2017 at 11:24 AM, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> > On Fri 05 May 21:48 PDT 2017, Jassi Brar wrote:
> >
> > The APCS IPC register serves the basis for all inter-processor
> > communication in a Qualcomm platform, so it's not only the RPM driver
> > discussed earlier that uses this. It's also used for other non-FIFO
> > based communication channels, where the signalled information either
> > isn't acked at all or acked on a system-level.
> >
> Something has to indicate consumption of data or "requested action
> taken". Otherwise the protocol is design-wise broken.
> 

The SMD and GLINK protocols work by providing two independent one-way
pipes that higher levels can use to send and receive messages. When some
driver pushes a message into the transmit-pipe we check if there's
space, then write the message, signal the remote (APCS IPC) and then
return.

It's then up to the higher level driver and protocol what to do next. In
some cases it will expect that there will appear a packet on the
incoming pipe indicating that the action was taken, but there's nothing
in the communication path enforcing this.

By this you can have strictly one-way notifications, two-way
stop-and-wait style communication or multiple-messages-in-flight
communication over the same transport mechanism.


The remote will update the read index (in shared memory) as it consumes
the data from the FIFO, but under normal circumstances there are no
reason for it to actively notify the sender or for the sender to wait
for it to be consumed.

> > But regardless of the protocol implemented ontop, the purpose of the
> > APCS IPC bit is _only_ to invoke some remote handler to consume some
> > data, somewhere - the event in itself does not carry any information.
> >
> Yes, every platform that uses shared-memory works like that. However
> there is always something that tells if the command has been acted
> upon by the remote. In your case that is the read-pointer movement.
> 

In a straight forward stop-and-wait flow control based setup like the
version of RPM previously discussed this makes a lot of sense. But there
are a multitude of different protocols using this mechanism to signal
that something has happened.

> >> The client should call mbox_client_txdone() after
> >> mbox_send_message().
> >
> > So every time we call mbox_send_message() from any of the client drivers
> > we also needs to call mbox_client_txdone()?
> >
> Yes.
> 
> > This seems like an awkward side effect of using the mailbox framework -
> > which has to be spread out in at least 6 different client drivers :(
> >
> No. Mailbox or whatever you implement - you must (and do) tick the
> state machine to keep the messages moving.

But the state you have in the other mailbox drivers is not a concern of
the APCS IPC.

>   Best designs have some interrupt occurring when the message has been
> consumed by the remote. Some designs have a flag set which needs to be
> polled to detect completion. Very few (like yours) that support
> neither irq nor polling, have to be driven by the upper protocol layer
> by some ack packet (or tracking read/write pointers like you do).
> These three cases are denoted by TXDONE_BY_IRQ, TXDONE_BY_POLL and
> TXDONE_BY_ACK respectively.
> 

You're confusing the APCS IPC with the larger communication mechanism,
flow control is taken care of in some higher layer - if it's needed at
all.

This is why I suggested that this is a doorbell, rather than a mailbox.
Your argumentation of how a mailbox should work makes perfect sense, but
it's not how the Qualcomm IPC works.

>   If no client driver will ever submit a message if there is no space
> in FIFO, then you can specify TXDONE_BY_POLL and have last_tx_done()
> always return true. That way you don't need to call
> mbox_client_txdone().

Clients of the APCS IPC will never post a message to the mailbox, it's
non-blocking and the "transaction" is done when the operation returns.
All the other parts of a "non-broken protocol" is a concern of some
other part of the software stack.


Setting TXDONE_BY_POLL and specifying a dummy last_tx_done() comes with
a crazy overhead. To set a single bit in a register we will take the
channel spinlock 4 times, start a timer, iterate over all registered
channels and the client must be marked as blocking so we will get at
least 2 additional context switches.

Regards,
Bjorn

Re: [PATCH v4 3/5] soc: qcom: Introduce APCS IPC driver

[Jassi Brar]

On Tue, May 9, 2017 at 12:41 AM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Sun 07 May 23:47 PDT 2017, Jassi Brar wrote:
>
>> On Mon, May 8, 2017 at 11:24 AM, Bjorn Andersson
>> <bjorn.andersson@linaro.org> wrote:
>> > On Fri 05 May 21:48 PDT 2017, Jassi Brar wrote:
>> >
>> > The APCS IPC register serves the basis for all inter-processor
>> > communication in a Qualcomm platform, so it's not only the RPM driver
>> > discussed earlier that uses this. It's also used for other non-FIFO
>> > based communication channels, where the signalled information either
>> > isn't acked at all or acked on a system-level.
>> >
>> Something has to indicate consumption of data or "requested action
>> taken". Otherwise the protocol is design-wise broken.
>>
>
> The SMD and GLINK protocols work by providing two independent one-way
> pipes that higher levels can use to send and receive messages. When some
> driver pushes a message into the transmit-pipe we check if there's
> space, then write the message, signal the remote (APCS IPC) and then
> return.
>
"we check if there's space"  -> this is what mailbox api tries to do
with last_tx_done before starting the next message.


>> >> The client should call mbox_client_txdone() after
>> >> mbox_send_message().
>> >
>> > So every time we call mbox_send_message() from any of the client drivers
>> > we also needs to call mbox_client_txdone()?
>> >
>> Yes.
>>
>> > This seems like an awkward side effect of using the mailbox framework -
>> > which has to be spread out in at least 6 different client drivers :(
>> >
>> No. Mailbox or whatever you implement - you must (and do) tick the
>> state machine to keep the messages moving.
>
> But the state you have in the other mailbox drivers is not a concern of
> the APCS IPC.
>
No, as you say above you check for space before writing the next
message, this is what I call ticking the state machine.


>>   Best designs have some interrupt occurring when the message has been
>> consumed by the remote. Some designs have a flag set which needs to be
>> polled to detect completion. Very few (like yours) that support
>> neither irq nor polling, have to be driven by the upper protocol layer
>> by some ack packet (or tracking read/write pointers like you do).
>> These three cases are denoted by TXDONE_BY_IRQ, TXDONE_BY_POLL and
>> TXDONE_BY_ACK respectively.
>>
>
> You're confusing the APCS IPC with the larger communication mechanism,
>
Maybe. I am not versed with QCom technologies like RPM, SMD, GLINK, APCS etc.
Controller driver is what physically transmits a signal to remote.
Users above the mailbox api are client drivers.

>
> This is why I suggested that this is a doorbell, rather than a mailbox.
> Your argumentation of how a mailbox should work makes perfect sense, but
> it's not how the Qualcomm IPC works.
>
Mailbox framework is designed to support, what you call doorbell type
of communication, just fine. There is no need to define another class.

>
> Setting TXDONE_BY_POLL and specifying a dummy last_tx_done() comes with
> a crazy overhead. To set a single bit in a register we will take the
> channel spinlock 4 times, start a timer, iterate over all registered
> channels and the client must be marked as blocking so we will get at
> least 2 additional context switches.
>
How often does the platform send messages for it to be a considerable load?
BTW, this is an option only if your client driver doesn't want to
explicitly tick the state machine by calling mbox_client_txdone()...
which I think should be done in the first place.

thanks

Re: [PATCH v4 3/5] soc: qcom: Introduce APCS IPC driver

[Bjorn Andersson]

On Tue 09 May 09:41 PDT 2017, Jassi Brar wrote:

> On Tue, May 9, 2017 at 12:41 AM, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> > On Sun 07 May 23:47 PDT 2017, Jassi Brar wrote:
> >
> >> On Mon, May 8, 2017 at 11:24 AM, Bjorn Andersson
> >> <bjorn.andersson@linaro.org> wrote:
> >> > On Fri 05 May 21:48 PDT 2017, Jassi Brar wrote:
> >> >
> >> > The APCS IPC register serves the basis for all inter-processor
> >> > communication in a Qualcomm platform, so it's not only the RPM driver
> >> > discussed earlier that uses this. It's also used for other non-FIFO
> >> > based communication channels, where the signalled information either
> >> > isn't acked at all or acked on a system-level.
> >> >
> >> Something has to indicate consumption of data or "requested action
> >> taken". Otherwise the protocol is design-wise broken.
> >>
> >
> > The SMD and GLINK protocols work by providing two independent one-way
> > pipes that higher levels can use to send and receive messages. When some
> > driver pushes a message into the transmit-pipe we check if there's
> > space, then write the message, signal the remote (APCS IPC) and then
> > return.
> >
> "we check if there's space"  -> this is what mailbox api tries to do
> with last_tx_done before starting the next message.
> 

The space we're looking for is in a higher level in the protocol stack,
the APCS IPC doesn't have a space concern.

> 
> >> >> The client should call mbox_client_txdone() after
> >> >> mbox_send_message().
> >> >
> >> > So every time we call mbox_send_message() from any of the client drivers
> >> > we also needs to call mbox_client_txdone()?
> >> >
> >> Yes.
> >>
> >> > This seems like an awkward side effect of using the mailbox framework -
> >> > which has to be spread out in at least 6 different client drivers :(
> >> >
> >> No. Mailbox or whatever you implement - you must (and do) tick the
> >> state machine to keep the messages moving.
> >
> > But the state you have in the other mailbox drivers is not a concern of
> > the APCS IPC.
> >
> No, as you say above you check for space before writing the next
> message, this is what I call ticking the state machine.
> 

Sure, but you're talking about the mailbox state machine. The APCS IPC
doesn't have states.  The _only_ thing that the APCS IPC provides is a
mechanism for informing the other side that "hey there's something to
do". So it doesn't matter if there's already a pending "hey there's
something to do", because adding another will still only be "hey there's
something to do".

I'm just trying to describe the big picture, but you keep confusing the
mailbox/doorbell responsibilities with the client's responsibilities.

> 
> >>   Best designs have some interrupt occurring when the message has been
> >> consumed by the remote. Some designs have a flag set which needs to be
> >> polled to detect completion. Very few (like yours) that support
> >> neither irq nor polling, have to be driven by the upper protocol layer
> >> by some ack packet (or tracking read/write pointers like you do).
> >> These three cases are denoted by TXDONE_BY_IRQ, TXDONE_BY_POLL and
> >> TXDONE_BY_ACK respectively.
> >>
> >
> > You're confusing the APCS IPC with the larger communication mechanism,
> >
> Maybe. I am not versed with QCom technologies like RPM, SMD, GLINK, APCS etc.
> Controller driver is what physically transmits a signal to remote.
> Users above the mailbox api are client drivers.
> 

Correct, and the signal that we're trying to transmit is "hey there's
_something_ to do", nothing else.


Relate this to when Fedex drops a packet at your door; first he checks
there's space on the porch, then he puts the packet there, he rings the
doorbell and then he walks away. You are free to "answer" the doorbell
now or at any point in the future. He doesn't have to stand there are
wait and it doesn't matter if he rings the bell multiple times - you
will still only check for packages once (regardless of how many he left).

In the case of him wanting your signature, then that's an implementation
detail of the Fedex guy, it has nothing to do with how you wire your
doorbell!

> >
> > This is why I suggested that this is a doorbell, rather than a mailbox.
> > Your argumentation of how a mailbox should work makes perfect sense, but
> > it's not how the Qualcomm IPC works.
> >
> Mailbox framework is designed to support, what you call doorbell type
> of communication, just fine. There is no need to define another class.
> 

Okay good.

> >
> > Setting TXDONE_BY_POLL and specifying a dummy last_tx_done() comes with
> > a crazy overhead. To set a single bit in a register we will take the
> > channel spinlock 4 times, start a timer, iterate over all registered
> > channels and the client must be marked as blocking so we will get at
> > least 2 additional context switches.
> >
> How often does the platform send messages for it to be a considerable load?

This is the basis for all inter-processor communication in the Qualcomm
platforms, so the above list of extra hoops to jump through is not
acceptable.

> BTW, this is an option only if your client driver doesn't want to
> explicitly tick the state machine by calling mbox_client_txdone()...
> which I think should be done in the first place.
> 

There is no state of the APCS IPC, so the overhead is created by the
mailbox framework.


The part where this piece of hardware differs from the other mailboxes
is that TX is done as send_data() returns and in the realm of the
mailbox there is no such thing as "tx done". So how about we extend the
framework to handle stateless and message-less doorbells?

Regards,
Bjorn

Re: [PATCH v4 3/5] soc: qcom: Introduce APCS IPC driver

[Jassi Brar]

On Wed, May 10, 2017 at 12:41 AM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Tue 09 May 09:41 PDT 2017, Jassi Brar wrote:


>>
>> >> >> The client should call mbox_client_txdone() after
>> >> >> mbox_send_message().
>> >> >
>> >> > So every time we call mbox_send_message() from any of the client drivers
>> >> > we also needs to call mbox_client_txdone()?
>> >> >
>> >> Yes.
>> >>
>> >> > This seems like an awkward side effect of using the mailbox framework -
>> >> > which has to be spread out in at least 6 different client drivers :(
>> >> >
>> >> No. Mailbox or whatever you implement - you must (and do) tick the
>> >> state machine to keep the messages moving.
>> >
>> > But the state you have in the other mailbox drivers is not a concern of
>> > the APCS IPC.
>> >
>> No, as you say above you check for space before writing the next
>> message, this is what I call ticking the state machine.
>>
>
> Sure, but you're talking about the mailbox state machine. The APCS IPC
> doesn't have states.  The _only_ thing that the APCS IPC provides is a
> mechanism for informing the other side that "hey there's something to
> do". So it doesn't matter if there's already a pending "hey there's
> something to do", because adding another will still only be "hey there's
> something to do".
>
> I'm just trying to describe the big picture, but you keep confusing the
> mailbox/doorbell responsibilities with the client's responsibilities.
>
I think I do understand the bigger picture...

The client driver sets up data packet in SHM and submit a "doorbell"
to be ringed. The controller driver simply sets some bit to trigger an
irq on the remote side (doorbell). And before submitting a "doorbell"
the client makes sure there is some space for data packet to be
written. Right?  You see, in the big picture you do have a
state-machine.

                 [Message to send]
                               |
                               |
           |-------------->|
           | No             |
           |                   |
           |___[Space Available?]
                               |
                               |Yes
                               |
                               |
                  [ Setup Data in SHM]
                               |
                              V
                    [Ring Doorbell]


Mailbox framework supports this whole picture. There is even a
callback (tx_prepare) to setup data packet just before the doorbell is
to be rung.

>> BTW, this is an option only if your client driver doesn't want to
>> explicitly tick the state machine by calling mbox_client_txdone()...
>> which I think should be done in the first place.
>>
>
> There is no state of the APCS IPC, so the overhead is created by the
> mailbox framework.
>
Overhead remains the same if you move the check from your client
drivers to last_tx_done.
OR your client driver, rightfully, drive the state machine by calling
mbox_client_txdone() like other platforms.

                 [Message to send]<----------|
                               |                             |
                               |                             |
           |-------------->|                             |
           | No             |                             |
           |                   |                             |
           |___[Space Available?]             |
                               |                             |
                               |Yes                       |
                               |                             |
                              V                             |
                  [Setup Data in SHM]           |
                               |                              |
                              V                              |
                 mbox_send_message()        |
                               |                               |
                              V                              |
                 mbox_client_txdone()           |
                               |                              |
                               V______________|


> The part where this piece of hardware differs from the other mailboxes
> is that TX is done as send_data() returns and in the realm of the
> mailbox there is no such thing as "tx done". So how about we extend the
> framework to handle stateless and message-less doorbells?
>
This is a very common usecase. It would be unfair to other platforms
to modify the API just because you find it awkward to call
mbox_client_txdone() right after mbox_send_message(). For example,
drivers/firmware/tegra/bpmp.c
I'd much rather have mbox_send_message_and_tick() than implant a new api.

Thanks.

Re: [PATCH v4 3/5] soc: qcom: Introduce APCS IPC driver

[Bjorn Andersson]

On Tue 09 May 19:33 PDT 2017, Jassi Brar wrote:

> On Wed, May 10, 2017 at 12:41 AM, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> > On Tue 09 May 09:41 PDT 2017, Jassi Brar wrote:
[..]
> > The part where this piece of hardware differs from the other mailboxes
> > is that TX is done as send_data() returns and in the realm of the
> > mailbox there is no such thing as "tx done". So how about we extend the
> > framework to handle stateless and message-less doorbells?
> >
> This is a very common usecase. It would be unfair to other platforms
> to modify the API just because you find it awkward to call
> mbox_client_txdone() right after mbox_send_message(). For example,
> drivers/firmware/tegra/bpmp.c
> I'd much rather have mbox_send_message_and_tick() than implant a new api.
> 

I wasn't proposing to implement a new API; for mailbox controllers with
tx method set to POLL the client will only have call mbox_send_data()
nothing else. So I proposed [1] yesterday, that will make the apcs
controller behave just like this case.


Looking at it again, making sure that tx method is TXDONE_BY_ACK should
make mbox_client_txdone() essentially a nop, only locking the channel
spinlock twice and then returning, as send_data() didn't leave any data
in the queue. Is my understanding of this correct?

So please let me know what you think about [1], if you don't like it
I'll fix the things pointed to by Stephen and we'll have to live with
the two calls.

[1] https://patchwork.kernel.org/patch/9718931/

Regards,
Bjorn

Re: [PATCH v4 3/5] soc: qcom: Introduce APCS IPC driver

[Jassi Brar]

On Thu, May 11, 2017 at 12:30 AM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Tue 09 May 19:33 PDT 2017, Jassi Brar wrote:
>
>> On Wed, May 10, 2017 at 12:41 AM, Bjorn Andersson
>> <bjorn.andersson@linaro.org> wrote:
>> > On Tue 09 May 09:41 PDT 2017, Jassi Brar wrote:
> [..]
>> > The part where this piece of hardware differs from the other mailboxes
>> > is that TX is done as send_data() returns and in the realm of the
>> > mailbox there is no such thing as "tx done". So how about we extend the
>> > framework to handle stateless and message-less doorbells?
>> >
>> This is a very common usecase. It would be unfair to other platforms
>> to modify the API just because you find it awkward to call
>> mbox_client_txdone() right after mbox_send_message(). For example,
>> drivers/firmware/tegra/bpmp.c
>> I'd much rather have mbox_send_message_and_tick() than implant a new api.
>>
>
> I wasn't proposing to implement a new API
>
Introducing mbox_controller.txdone_none for underlying mailbox
controllers is indeed a new API.

>; for mailbox controllers with
> tx method set to POLL the client will only have call mbox_send_data()
> nothing else. So I proposed [1] yesterday, that will make the apcs
> controller behave just like this case.
>
mbox_send_data()    for POLL usecase
mbox_send_data()+mbox_client_txdone()   for your usecase.

> Looking at it again, making sure that tx method is TXDONE_BY_ACK should
> make mbox_client_txdone() essentially a nop, only locking the channel
> spinlock twice and then returning, as send_data() didn't leave any data
> in the queue. Is my understanding of this correct?
>
Not exactly. tx_tick() frees the busy flag 'chan->active_req'.

> So please let me know what you think about [1], if you don't like it
> I'll fix the things pointed to by Stephen and we'll have to live with
> the two calls.
>
My last reply was about [1]. Other platforms call
mbox_send_data()+mbox_client_txdone() see
drivers/firmware/tegra/bpmp.c, but you want to introduce another API
in the innards of the framework. If we must do it, it should be done
above the framework by introducing

void mbox_send_message_and_tick(struct mbox_chan *chan, void *mssg)
           OR
void mbox_ring_doorbell(struct mbox_chan *chan, void *mssg)
{
   (void)mbox_send_message(chan, mssg);
   mbox_client_txdone(chan, 0);
}

Re: [PATCH v4 3/5] soc: qcom: Introduce APCS IPC driver

[Bjorn Andersson]

On Wed 10 May 19:07 PDT 2017, Jassi Brar wrote:

> On Thu, May 11, 2017 at 12:30 AM, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> > On Tue 09 May 19:33 PDT 2017, Jassi Brar wrote:
[..]
> > So please let me know what you think about [1], if you don't like it
> > I'll fix the things pointed to by Stephen and we'll have to live with
> > the two calls.
> >
> My last reply was about [1]. Other platforms call
> mbox_send_data()+mbox_client_txdone() see
> drivers/firmware/tegra/bpmp.c, but you want to introduce another API
> in the innards of the framework.

Okay, lets go with that then. I will incorporate the changes requested
by Stephen and post a final version and then add the
mbox_client_txdone() in the clients.

> If we must do it, it should be done
> above the framework by introducing
> 
> void mbox_send_message_and_tick(struct mbox_chan *chan, void *mssg)
>            OR
> void mbox_ring_doorbell(struct mbox_chan *chan, void *mssg)
> {
>    (void)mbox_send_message(chan, mssg);
>    mbox_client_txdone(chan, 0);
> }

This sounds reasonable, but I would prefer that we get the two drivers
merged - so I suggest that we deal with that later, when we see if its
worth the effort.

Regards,
Bjorn

Re: [PATCH v4 3/5] soc: qcom: Introduce APCS IPC driver

[Jassi Brar]

On Sat, May 13, 2017 at 4:18 AM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Wed 10 May 19:07 PDT 2017, Jassi Brar wrote:
>
>> On Thu, May 11, 2017 at 12:30 AM, Bjorn Andersson
>> <bjorn.andersson@linaro.org> wrote:
>> > On Tue 09 May 19:33 PDT 2017, Jassi Brar wrote:
> [..]
>> > So please let me know what you think about [1], if you don't like it
>> > I'll fix the things pointed to by Stephen and we'll have to live with
>> > the two calls.
>> >
>> My last reply was about [1]. Other platforms call
>> mbox_send_data()+mbox_client_txdone() see
>> drivers/firmware/tegra/bpmp.c, but you want to introduce another API
>> in the innards of the framework.
>
> Okay, lets go with that then. I will incorporate the changes requested
> by Stephen and post a final version and then add the
> mbox_client_txdone() in the clients.
>
OK cool. And please update the commit log discarding
"The driver implements the "doorbell" binding and could be used as basis
for a new Linux framework, if found useful outside Qualcomm."

Cheers!