[PATCH v4 0/2] mailbox: Add Tegra HSP driver

[PATCH v4 0/2] mailbox: Add Tegra HSP driver

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Hi Jassi,

These patches have gone through a number of revisions now and I've
tested them to the point where I'm confident that they work well.

Given the chain of drivers that depends on this, I think it'd be good if
I took this through the Tegra tree with your Acked-by, so that I can
deal with the dependencies within one tree. I can provide a stable tag
for you to pull into the mailbox tree.

Thanks,
Thierry

Joseph Lo (1):
  dt-bindings: mailbox: Add Tegra HSP binding

Thierry Reding (1):
  mailbox: Add Tegra HSP driver

 .../bindings/mailbox/nvidia,tegra186-hsp.txt       |  52 ++
 drivers/mailbox/Kconfig                            |   9 +
 drivers/mailbox/Makefile                           |   2 +
 drivers/mailbox/tegra-hsp.c                        | 542 +++++++++++++++++++++
 include/dt-bindings/mailbox/tegra186-hsp.h         |  24 +
 5 files changed, 629 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt
 create mode 100644 drivers/mailbox/tegra-hsp.c
 create mode 100644 include/dt-bindings/mailbox/tegra186-hsp.h

-- 
2.10.2

[PATCH v4 1/2] dt-bindings: mailbox: Add Tegra HSP binding

[Thierry Reding]

From: Joseph Lo <josephl@nvidia.com>

Add DT binding for the Hardware Synchronization Primitives (HSP). The
HSP is designed for the processors to share resources and communicate
with one another. A set of hardware synchronization primitives for
interprocessor communication (IPC) is provided. IPC protocols can use
use these hardware synchronization primitives when operating between
processors in an AMP configuration.

Signed-off-by: Joseph Lo <josephl@nvidia.com>
Acked-by: Stephen Warren <swarren@nvidia.com>
Acked-by: Rob Herring <robh@kernel.org>
Acked-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 .../bindings/mailbox/nvidia,tegra186-hsp.txt       | 52 ++++++++++++++++++++++
 include/dt-bindings/mailbox/tegra186-hsp.h         | 24 ++++++++++
 2 files changed, 76 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt
 create mode 100644 include/dt-bindings/mailbox/tegra186-hsp.h

diff --git a/Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt b/Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt
new file mode 100644
index 000000000000..b99d25fc2f26
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt
@@ -0,0 +1,52 @@
+NVIDIA Tegra Hardware Synchronization Primitives (HSP)
+
+The HSP modules are used for the processors to share resources and communicate
+together. It provides a set of hardware synchronization primitives for
+interprocessor communication. So the interprocessor communication (IPC)
+protocols can use hardware synchronization primitives, when operating between
+two processors not in an SMP relationship.
+
+The features that HSP supported are shared mailboxes, shared semaphores,
+arbitrated semaphores and doorbells.
+
+Required properties:
+- name : Should be hsp
+- compatible
+    Array of strings.
+    one of:
+    - "nvidia,tegra186-hsp"
+- reg : Offset and length of the register set for the device.
+- interrupt-names
+    Array of strings.
+    Contains a list of names for the interrupts described by the interrupt
+    property. May contain the following entries, in any order:
+    - "doorbell"
+    Users of this binding MUST look up entries in the interrupt property
+    by name, using this interrupt-names property to do so.
+- interrupts
+    Array of interrupt specifiers.
+    Must contain one entry per entry in the interrupt-names property,
+    in a matching order.
+- #mbox-cells : Should be 2.
+
+The mbox specifier of the "mboxes" property in the client node should
+contain two data. The first one should be the HSP type and the second
+one should be the ID that the client is going to use. Those information
+can be found in the following file.
+
+- <dt-bindings/mailbox/tegra186-hsp.h>.
+
+Example:
+
+hsp_top0: hsp@3c00000 {
+	compatible = "nvidia,tegra186-hsp";
+	reg = <0x0 0x03c00000 0x0 0xa0000>;
+	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
+	interrupt-names = "doorbell";
+	#mbox-cells = <2>;
+};
+
+client {
+	...
+	mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_DB TEGRA_HSP_DB_MASTER_XXX>;
+};
diff --git a/include/dt-bindings/mailbox/tegra186-hsp.h b/include/dt-bindings/mailbox/tegra186-hsp.h
new file mode 100644
index 000000000000..f5d66e5f5f10
--- /dev/null
+++ b/include/dt-bindings/mailbox/tegra186-hsp.h
@@ -0,0 +1,24 @@
+/*
+ * This header provides constants for binding nvidia,tegra186-hsp.
+ */
+
+#ifndef _DT_BINDINGS_MAILBOX_TEGRA186_HSP_H
+#define _DT_BINDINGS_MAILBOX_TEGRA186_HSP_H
+
+/*
+ * These define the type of mailbox that is to be used (doorbell, shared
+ * mailbox, shared semaphore or arbitrated semaphore).
+ */
+#define TEGRA_HSP_MBOX_TYPE_DB 0x0
+#define TEGRA_HSP_MBOX_TYPE_SM 0x1
+#define TEGRA_HSP_MBOX_TYPE_SS 0x2
+#define TEGRA_HSP_MBOX_TYPE_AS 0x3
+
+/*
+ * These defines represent the bit associated with the given master ID in the
+ * doorbell registers.
+ */
+#define TEGRA_HSP_DB_MASTER_CCPLEX 17
+#define TEGRA_HSP_DB_MASTER_BPMP 19
+
+#endif
-- 
2.10.2

[PATCH v4 2/2] mailbox: Add Tegra HSP driver

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This driver exposes a mailbox interface for interprocessor communication
using the Hardware Synchronization Primitives (HSP) module's doorbell
mechanism. There are multiple HSP instances and they provide additional
features such as shared mailboxes, shared and arbitrated semaphores.

A driver for a remote processor can use the mailbox client provided by
the HSP driver and build an IPC protocol on top of this synchronization
mechanism.

Based on work by Joseph Lo <josephl@nvidia.com>.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v4:
- check for doorbell ring before enabling new master
- add missing space in Kconfig description
- remove unnecessary barriers
- simplify doorbell lookup
- remove debug leftovers

Changes in v3:
- use a more object oriented design
- fix some locking issues

 drivers/mailbox/Kconfig     |   9 +
 drivers/mailbox/Makefile    |   2 +
 drivers/mailbox/tegra-hsp.c | 542 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 553 insertions(+)
 create mode 100644 drivers/mailbox/tegra-hsp.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 11eebfe8a4cb..ceff415f201c 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -124,6 +124,15 @@ config MAILBOX_TEST
 	  Test client to help with testing new Controller driver
 	  implementations.
 
+config TEGRA_HSP_MBOX
+	bool "Tegra HSP (Hardware Synchronization Primitives) Driver"
+	depends on ARCH_TEGRA_186_SOC
+	help
+	  The Tegra HSP driver is used for the interprocessor communication
+	  between different remote processors and host processors on Tegra186
+	  and later SoCs. Say Y here if you want to have this support.
+	  If unsure say N.
+
 config XGENE_SLIMPRO_MBOX
 	tristate "APM SoC X-Gene SLIMpro Mailbox Controller"
 	depends on ARCH_XGENE
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index ace6fed8fea9..7dde4f609ae8 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -29,3 +29,5 @@ obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
 obj-$(CONFIG_HI6220_MBOX)	+= hi6220-mailbox.o
 
 obj-$(CONFIG_BCM_PDC_MBOX)	+= bcm-pdc-mailbox.o
+
+obj-$(CONFIG_TEGRA_HSP_MBOX)	+= tegra-hsp.o
diff --git a/drivers/mailbox/tegra-hsp.c b/drivers/mailbox/tegra-hsp.c
new file mode 100644
index 000000000000..17b256a52855
--- /dev/null
+++ b/drivers/mailbox/tegra-hsp.c
@@ -0,0 +1,542 @@
+/*
+ * Copyright (c) 2016, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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/interrupt.h>
+#include <linux/io.h>
+#include <linux/mailbox_controller.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include <dt-bindings/mailbox/tegra186-hsp.h>
+
+#define HSP_INT_DIMENSIONING	0x380
+#define HSP_nSM_SHIFT		0
+#define HSP_nSS_SHIFT		4
+#define HSP_nAS_SHIFT		8
+#define HSP_nDB_SHIFT		12
+#define HSP_nSI_SHIFT		16
+#define HSP_nINT_MASK		0xf
+
+#define HSP_DB_TRIGGER	0x0
+#define HSP_DB_ENABLE	0x4
+#define HSP_DB_RAW	0x8
+#define HSP_DB_PENDING	0xc
+
+#define HSP_DB_CCPLEX		1
+#define HSP_DB_BPMP		3
+#define HSP_DB_MAX		7
+
+struct tegra_hsp_channel;
+struct tegra_hsp;
+
+struct tegra_hsp_channel_ops {
+	int (*send_data)(struct tegra_hsp_channel *channel, void *data);
+	int (*startup)(struct tegra_hsp_channel *channel);
+	void (*shutdown)(struct tegra_hsp_channel *channel);
+	bool (*last_tx_done)(struct tegra_hsp_channel *channel);
+};
+
+struct tegra_hsp_channel {
+	struct tegra_hsp *hsp;
+	const struct tegra_hsp_channel_ops *ops;
+	struct mbox_chan *chan;
+	void __iomem *regs;
+};
+
+static struct tegra_hsp_channel *to_tegra_hsp_channel(struct mbox_chan *chan)
+{
+	return chan->con_priv;
+}
+
+struct tegra_hsp_doorbell {
+	struct tegra_hsp_channel channel;
+	struct list_head list;
+	const char *name;
+	unsigned int master;
+	unsigned int index;
+};
+
+static struct tegra_hsp_doorbell *
+to_tegra_hsp_doorbell(struct tegra_hsp_channel *channel)
+{
+	if (!channel)
+		return NULL;
+
+	return container_of(channel, struct tegra_hsp_doorbell, channel);
+}
+
+struct tegra_hsp_db_map {
+	const char *name;
+	unsigned int master;
+	unsigned int index;
+};
+
+struct tegra_hsp_soc {
+	const struct tegra_hsp_db_map *map;
+};
+
+struct tegra_hsp {
+	const struct tegra_hsp_soc *soc;
+	struct mbox_controller mbox;
+	void __iomem *regs;
+	unsigned int irq;
+	unsigned int num_sm;
+	unsigned int num_as;
+	unsigned int num_ss;
+	unsigned int num_db;
+	unsigned int num_si;
+	spinlock_t lock;
+
+	struct list_head doorbells;
+};
+
+static inline struct tegra_hsp *
+to_tegra_hsp(struct mbox_controller *mbox)
+{
+	return container_of(mbox, struct tegra_hsp, mbox);
+}
+
+static inline u32 tegra_hsp_readl(struct tegra_hsp *hsp, unsigned int offset)
+{
+	return readl(hsp->regs + offset);
+}
+
+static inline void tegra_hsp_writel(struct tegra_hsp *hsp, u32 value,
+				    unsigned int offset)
+{
+	writel(value, hsp->regs + offset);
+}
+
+static inline u32 tegra_hsp_channel_readl(struct tegra_hsp_channel *channel,
+					  unsigned int offset)
+{
+	return readl(channel->regs + offset);
+}
+
+static inline void tegra_hsp_channel_writel(struct tegra_hsp_channel *channel,
+					    u32 value, unsigned int offset)
+{
+	writel(value, channel->regs + offset);
+}
+
+static bool tegra_hsp_doorbell_can_ring(struct tegra_hsp_doorbell *db)
+{
+	u32 value;
+
+	value = tegra_hsp_channel_readl(&db->channel, HSP_DB_ENABLE);
+
+	return (value & BIT(TEGRA_HSP_DB_MASTER_CCPLEX)) != 0;
+}
+
+static struct tegra_hsp_doorbell *
+__tegra_hsp_doorbell_get(struct tegra_hsp *hsp, unsigned int master)
+{
+	struct tegra_hsp_doorbell *entry;
+
+	list_for_each_entry(entry, &hsp->doorbells, list)
+		if (entry->master == master)
+			return entry;
+
+	return NULL;
+}
+
+static struct tegra_hsp_doorbell *
+tegra_hsp_doorbell_get(struct tegra_hsp *hsp, unsigned int master)
+{
+	struct tegra_hsp_doorbell *db;
+	unsigned long flags;
+
+	spin_lock_irqsave(&hsp->lock, flags);
+	db = __tegra_hsp_doorbell_get(hsp, master);
+	spin_unlock_irqrestore(&hsp->lock, flags);
+
+	return db;
+}
+
+static irqreturn_t tegra_hsp_doorbell_irq(int irq, void *data)
+{
+	struct tegra_hsp *hsp = data;
+	struct tegra_hsp_doorbell *db;
+	unsigned long master, value;
+
+	db = tegra_hsp_doorbell_get(hsp, TEGRA_HSP_DB_MASTER_CCPLEX);
+	if (!db)
+		return IRQ_NONE;
+
+	value = tegra_hsp_channel_readl(&db->channel, HSP_DB_PENDING);
+	tegra_hsp_channel_writel(&db->channel, value, HSP_DB_PENDING);
+
+	spin_lock(&hsp->lock);
+
+	for_each_set_bit(master, &value, hsp->mbox.num_chans) {
+		struct tegra_hsp_doorbell *db;
+
+		db = __tegra_hsp_doorbell_get(hsp, master);
+		/*
+		 * Depending on the bootloader chain, the CCPLEX doorbell will
+		 * have some doorbells enabled, which means that requesting an
+		 * interrupt will immediately fire.
+		 *
+		 * In that case, db->channel.chan will still be NULL here and
+		 * cause a crash if not properly guarded.
+		 *
+		 * It remains to be seen if ignoring the doorbell in that case
+		 * is the correct solution.
+		 */
+		if (db && db->channel.chan)
+			mbox_chan_received_data(db->channel.chan, NULL);
+	}
+
+	spin_unlock(&hsp->lock);
+
+	return IRQ_HANDLED;
+}
+
+static int tegra_hsp_doorbell_send_data(struct tegra_hsp_channel *channel,
+					void *data)
+{
+	tegra_hsp_channel_writel(channel, 1, HSP_DB_TRIGGER);
+
+	return 0;
+}
+
+static int tegra_hsp_doorbell_startup(struct tegra_hsp_channel *channel)
+{
+	struct tegra_hsp_doorbell *db = to_tegra_hsp_doorbell(channel);
+	struct tegra_hsp *hsp = channel->hsp;
+	struct tegra_hsp_doorbell *ccplex;
+	unsigned long flags;
+	u32 value;
+
+	if (db->master >= hsp->mbox.num_chans) {
+		dev_err(hsp->mbox.dev,
+			"invalid master ID %u for HSP channel\n",
+			db->master);
+		return -EINVAL;
+	}
+
+	ccplex = tegra_hsp_doorbell_get(hsp, TEGRA_HSP_DB_MASTER_CCPLEX);
+	if (!ccplex)
+		return -ENODEV;
+
+	if (!tegra_hsp_doorbell_can_ring(db))
+		return -ENODEV;
+
+	spin_lock_irqsave(&hsp->lock, flags);
+
+	value = tegra_hsp_channel_readl(&ccplex->channel, HSP_DB_ENABLE);
+	value |= BIT(db->master);
+	tegra_hsp_channel_writel(&ccplex->channel, value, HSP_DB_ENABLE);
+
+	spin_unlock_irqrestore(&hsp->lock, flags);
+
+	return 0;
+}
+
+static void tegra_hsp_doorbell_shutdown(struct tegra_hsp_channel *channel)
+{
+	struct tegra_hsp_doorbell *db = to_tegra_hsp_doorbell(channel);
+	struct tegra_hsp *hsp = channel->hsp;
+	struct tegra_hsp_doorbell *ccplex;
+	unsigned long flags;
+	u32 value;
+
+	ccplex = tegra_hsp_doorbell_get(hsp, TEGRA_HSP_DB_MASTER_CCPLEX);
+	if (!ccplex)
+		return;
+
+	spin_lock_irqsave(&hsp->lock, flags);
+
+	value = tegra_hsp_channel_readl(&ccplex->channel, HSP_DB_ENABLE);
+	value &= ~BIT(db->master);
+	tegra_hsp_channel_writel(&ccplex->channel, value, HSP_DB_ENABLE);
+
+	spin_unlock_irqrestore(&hsp->lock, flags);
+}
+
+static bool tegra_hsp_doorbell_last_tx_done(struct tegra_hsp_channel *channel)
+{
+	return true;
+}
+
+static const struct tegra_hsp_channel_ops tegra_hsp_doorbell_ops = {
+	.send_data = tegra_hsp_doorbell_send_data,
+	.startup = tegra_hsp_doorbell_startup,
+	.shutdown = tegra_hsp_doorbell_shutdown,
+	.last_tx_done = tegra_hsp_doorbell_last_tx_done,
+};
+
+static struct tegra_hsp_channel *
+tegra_hsp_doorbell_create(struct tegra_hsp *hsp, const char *name,
+			  unsigned int master, unsigned int index)
+{
+	struct tegra_hsp_doorbell *db;
+	unsigned int offset;
+	unsigned long flags;
+
+	db = kzalloc(sizeof(*db), GFP_KERNEL);
+	if (!db)
+		return ERR_PTR(-ENOMEM);
+
+	offset = (1 + (hsp->num_sm / 2) + hsp->num_ss + hsp->num_as) << 16;
+	offset += index * 0x100;
+
+	db->channel.ops = &tegra_hsp_doorbell_ops;
+	db->channel.regs = hsp->regs + offset;
+	db->channel.hsp = hsp;
+
+	db->name = kstrdup_const(name, GFP_KERNEL);
+	db->master = master;
+	db->index = index;
+
+	spin_lock_irqsave(&hsp->lock, flags);
+	list_add_tail(&db->list, &hsp->doorbells);
+	spin_unlock_irqrestore(&hsp->lock, flags);
+
+	return &db->channel;
+}
+
+static void __tegra_hsp_doorbell_destroy(struct tegra_hsp_doorbell *db)
+{
+	list_del(&db->list);
+	kfree_const(db->name);
+	kfree(db);
+}
+
+static int tegra_hsp_send_data(struct mbox_chan *chan, void *data)
+{
+	struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan);
+
+	return channel->ops->send_data(channel, data);
+}
+
+static int tegra_hsp_startup(struct mbox_chan *chan)
+{
+	struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan);
+
+	return channel->ops->startup(channel);
+}
+
+static void tegra_hsp_shutdown(struct mbox_chan *chan)
+{
+	struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan);
+
+	return channel->ops->shutdown(channel);
+}
+
+static bool tegra_hsp_last_tx_done(struct mbox_chan *chan)
+{
+	struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan);
+
+	return channel->ops->last_tx_done(channel);
+}
+
+static const struct mbox_chan_ops tegra_hsp_ops = {
+	.send_data = tegra_hsp_send_data,
+	.startup = tegra_hsp_startup,
+	.shutdown = tegra_hsp_shutdown,
+	.last_tx_done = tegra_hsp_last_tx_done,
+};
+
+static struct mbox_chan *of_tegra_hsp_xlate(struct mbox_controller *mbox,
+					    const struct of_phandle_args *args)
+{
+	struct tegra_hsp_channel *channel = ERR_PTR(-ENODEV);
+	struct tegra_hsp *hsp = to_tegra_hsp(mbox);
+	unsigned int type = args->args[0];
+	unsigned int master = args->args[1];
+	struct tegra_hsp_doorbell *db;
+	struct mbox_chan *chan;
+	unsigned long flags;
+	unsigned int i;
+
+	switch (type) {
+	case TEGRA_HSP_MBOX_TYPE_DB:
+		db = tegra_hsp_doorbell_get(hsp, master);
+		if (db)
+			channel = &db->channel;
+
+		break;
+
+	default:
+		break;
+	}
+
+	if (IS_ERR(channel))
+		return ERR_CAST(channel);
+
+	spin_lock_irqsave(&hsp->lock, flags);
+
+	for (i = 0; i < hsp->mbox.num_chans; i++) {
+		chan = &hsp->mbox.chans[i];
+		if (!chan->con_priv) {
+			chan->con_priv = channel;
+			channel->chan = chan;
+			break;
+		}
+
+		chan = NULL;
+	}
+
+	spin_unlock_irqrestore(&hsp->lock, flags);
+
+	return chan ?: ERR_PTR(-EBUSY);
+}
+
+static void tegra_hsp_remove_doorbells(struct tegra_hsp *hsp)
+{
+	struct tegra_hsp_doorbell *db;
+	unsigned long flags;
+
+	spin_lock_irqsave(&hsp->lock, flags);
+
+	list_for_each_entry(db, &hsp->doorbells, list)
+		__tegra_hsp_doorbell_destroy(db);
+
+	spin_unlock_irqrestore(&hsp->lock, flags);
+}
+
+static int tegra_hsp_add_doorbells(struct tegra_hsp *hsp)
+{
+	const struct tegra_hsp_db_map *map = hsp->soc->map;
+	struct tegra_hsp_channel *channel;
+
+	while (map->name) {
+		channel = tegra_hsp_doorbell_create(hsp, map->name,
+						    map->master, map->index);
+		if (IS_ERR(channel)) {
+			tegra_hsp_remove_doorbells(hsp);
+			return PTR_ERR(channel);
+		}
+
+		map++;
+	}
+
+	return 0;
+}
+
+static int tegra_hsp_probe(struct platform_device *pdev)
+{
+	struct tegra_hsp *hsp;
+	struct resource *res;
+	u32 value;
+	int err;
+
+	hsp = devm_kzalloc(&pdev->dev, sizeof(*hsp), GFP_KERNEL);
+	if (!hsp)
+		return -ENOMEM;
+
+	hsp->soc = of_device_get_match_data(&pdev->dev);
+	INIT_LIST_HEAD(&hsp->doorbells);
+	spin_lock_init(&hsp->lock);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	hsp->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(hsp->regs))
+		return PTR_ERR(hsp->regs);
+
+	value = tegra_hsp_readl(hsp, HSP_INT_DIMENSIONING);
+	hsp->num_sm = (value >> HSP_nSM_SHIFT) & HSP_nINT_MASK;
+	hsp->num_ss = (value >> HSP_nSS_SHIFT) & HSP_nINT_MASK;
+	hsp->num_as = (value >> HSP_nAS_SHIFT) & HSP_nINT_MASK;
+	hsp->num_db = (value >> HSP_nDB_SHIFT) & HSP_nINT_MASK;
+	hsp->num_si = (value >> HSP_nSI_SHIFT) & HSP_nINT_MASK;
+
+	err = platform_get_irq_byname(pdev, "doorbell");
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to get doorbell IRQ: %d\n", err);
+		return err;
+	}
+
+	hsp->irq = err;
+
+	hsp->mbox.of_xlate = of_tegra_hsp_xlate;
+	hsp->mbox.num_chans = 32;
+	hsp->mbox.dev = &pdev->dev;
+	hsp->mbox.txdone_irq = false;
+	hsp->mbox.txdone_poll = false;
+	hsp->mbox.ops = &tegra_hsp_ops;
+
+	hsp->mbox.chans = devm_kcalloc(&pdev->dev, hsp->mbox.num_chans,
+					sizeof(*hsp->mbox.chans),
+					GFP_KERNEL);
+	if (!hsp->mbox.chans)
+		return -ENOMEM;
+
+	err = tegra_hsp_add_doorbells(hsp);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to add doorbells: %d\n", err);
+		return err;
+	}
+
+	platform_set_drvdata(pdev, hsp);
+
+	err = mbox_controller_register(&hsp->mbox);
+	if (err) {
+		dev_err(&pdev->dev, "failed to register mailbox: %d\n", err);
+		tegra_hsp_remove_doorbells(hsp);
+		return err;
+	}
+
+	err = devm_request_irq(&pdev->dev, hsp->irq, tegra_hsp_doorbell_irq,
+			       IRQF_NO_SUSPEND, dev_name(&pdev->dev), hsp);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to request IRQ#%u: %d\n",
+			hsp->irq, err);
+		return err;
+	}
+
+	return 0;
+}
+
+static int tegra_hsp_remove(struct platform_device *pdev)
+{
+	struct tegra_hsp *hsp = platform_get_drvdata(pdev);
+
+	mbox_controller_unregister(&hsp->mbox);
+	tegra_hsp_remove_doorbells(hsp);
+
+	return 0;
+}
+
+static const struct tegra_hsp_db_map tegra186_hsp_db_map[] = {
+	{ "ccplex", TEGRA_HSP_DB_MASTER_CCPLEX, HSP_DB_CCPLEX, },
+	{ "bpmp",   TEGRA_HSP_DB_MASTER_BPMP,   HSP_DB_BPMP,   },
+	{ /* sentinel */ }
+};
+
+static const struct tegra_hsp_soc tegra186_hsp_soc = {
+	.map = tegra186_hsp_db_map,
+};
+
+static const struct of_device_id tegra_hsp_match[] = {
+	{ .compatible = "nvidia,tegra186-hsp", .data = &tegra186_hsp_soc },
+	{ }
+};
+
+static struct platform_driver tegra_hsp_driver = {
+	.driver = {
+		.name = "tegra-hsp",
+		.of_match_table = tegra_hsp_match,
+	},
+	.probe = tegra_hsp_probe,
+	.remove = tegra_hsp_remove,
+};
+
+static int __init tegra_hsp_init(void)
+{
+	return platform_driver_register(&tegra_hsp_driver);
+}
+core_initcall(tegra_hsp_init);
-- 
2.10.2

Re: [PATCH v4 2/2] mailbox: Add Tegra HSP driver

[Jassi Brar]

On Tue, Nov 15, 2016 at 9:18 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:

....
> +
> +struct tegra_hsp_channel;
> +struct tegra_hsp;
> +
> +struct tegra_hsp_channel_ops {
> +       int (*send_data)(struct tegra_hsp_channel *channel, void *data);
> +       int (*startup)(struct tegra_hsp_channel *channel);
> +       void (*shutdown)(struct tegra_hsp_channel *channel);
> +       bool (*last_tx_done)(struct tegra_hsp_channel *channel);
> +};
> +
> +struct tegra_hsp_channel {
> +       struct tegra_hsp *hsp;
> +       const struct tegra_hsp_channel_ops *ops;
> +       struct mbox_chan *chan;
> +       void __iomem *regs;
> +};
> +
> +static struct tegra_hsp_channel *to_tegra_hsp_channel(struct mbox_chan *chan)
> +{
> +       return chan->con_priv;
> +}
> +
It seems
       channel = to_tegra_hsp_channel(chan);
is no simpler ritual than
       channel = chan->con_priv;   ?

> +struct tegra_hsp_doorbell {
> +       struct tegra_hsp_channel channel;
> +       struct list_head list;
> +       const char *name;
> +       unsigned int master;
> +       unsigned int index;
> +};
> +
> +static struct tegra_hsp_doorbell *
> +to_tegra_hsp_doorbell(struct tegra_hsp_channel *channel)
> +{
> +       if (!channel)
> +               return NULL;
> +
> +       return container_of(channel, struct tegra_hsp_doorbell, channel);
> +}
> +
But you don't check for NULL returned, before dereferencing the pointer 'db'

> +struct tegra_hsp_db_map {
> +       const char *name;
> +       unsigned int master;
> +       unsigned int index;
> +};
> +
> +struct tegra_hsp_soc {
> +       const struct tegra_hsp_db_map *map;
> +};
> +
> +struct tegra_hsp {
> +       const struct tegra_hsp_soc *soc;
> +       struct mbox_controller mbox;
> +       void __iomem *regs;
> +       unsigned int irq;
> +       unsigned int num_sm;
> +       unsigned int num_as;
> +       unsigned int num_ss;
> +       unsigned int num_db;
> +       unsigned int num_si;
> +       spinlock_t lock;
> +
> +       struct list_head doorbells;
> +};
> +
> +static inline struct tegra_hsp *
> +to_tegra_hsp(struct mbox_controller *mbox)
> +{
> +       return container_of(mbox, struct tegra_hsp, mbox);
> +}
> +
> +static inline u32 tegra_hsp_readl(struct tegra_hsp *hsp, unsigned int offset)
> +{
> +       return readl(hsp->regs + offset);
> +}
> +
> +static inline void tegra_hsp_writel(struct tegra_hsp *hsp, u32 value,
> +                                   unsigned int offset)
> +{
> +       writel(value, hsp->regs + offset);
> +}
> +
> +static inline u32 tegra_hsp_channel_readl(struct tegra_hsp_channel *channel,
> +                                         unsigned int offset)
> +{
> +       return readl(channel->regs + offset);
> +}
> +
> +static inline void tegra_hsp_channel_writel(struct tegra_hsp_channel *channel,
> +                                           u32 value, unsigned int offset)
> +{
> +       writel(value, channel->regs + offset);
> +}
> +
> +static bool tegra_hsp_doorbell_can_ring(struct tegra_hsp_doorbell *db)
> +{
> +       u32 value;
> +
> +       value = tegra_hsp_channel_readl(&db->channel, HSP_DB_ENABLE);
> +
> +       return (value & BIT(TEGRA_HSP_DB_MASTER_CCPLEX)) != 0;
> +}
> +
> +static struct tegra_hsp_doorbell *
> +__tegra_hsp_doorbell_get(struct tegra_hsp *hsp, unsigned int master)
> +{
> +       struct tegra_hsp_doorbell *entry;
> +
> +       list_for_each_entry(entry, &hsp->doorbells, list)
> +               if (entry->master == master)
> +                       return entry;
> +
> +       return NULL;
> +}
> +
> +static struct tegra_hsp_doorbell *
> +tegra_hsp_doorbell_get(struct tegra_hsp *hsp, unsigned int master)
> +{
> +       struct tegra_hsp_doorbell *db;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&hsp->lock, flags);
> +       db = __tegra_hsp_doorbell_get(hsp, master);
> +       spin_unlock_irqrestore(&hsp->lock, flags);
> +
> +       return db;
> +}
> +
.....

> +
> +static int tegra_hsp_doorbell_send_data(struct tegra_hsp_channel *channel,
> +                                       void *data)
> +{
> +       tegra_hsp_channel_writel(channel, 1, HSP_DB_TRIGGER);
> +
> +       return 0;
> +}
> +
> +static int tegra_hsp_doorbell_startup(struct tegra_hsp_channel *channel)
> +{
> +       struct tegra_hsp_doorbell *db = to_tegra_hsp_doorbell(channel);
> +       struct tegra_hsp *hsp = channel->hsp;
> +       struct tegra_hsp_doorbell *ccplex;
> +       unsigned long flags;
> +       u32 value;
> +
> +       if (db->master >= hsp->mbox.num_chans) {
> +               dev_err(hsp->mbox.dev,
> +                       "invalid master ID %u for HSP channel\n",
> +                       db->master);
> +               return -EINVAL;
> +       }
> +
> +       ccplex = tegra_hsp_doorbell_get(hsp, TEGRA_HSP_DB_MASTER_CCPLEX);
> +       if (!ccplex)
> +               return -ENODEV;
> +
> +       if (!tegra_hsp_doorbell_can_ring(db))
> +               return -ENODEV;
> +
> +       spin_lock_irqsave(&hsp->lock, flags);
> +
> +       value = tegra_hsp_channel_readl(&ccplex->channel, HSP_DB_ENABLE);
> +       value |= BIT(db->master);
> +       tegra_hsp_channel_writel(&ccplex->channel, value, HSP_DB_ENABLE);
> +
> +       spin_unlock_irqrestore(&hsp->lock, flags);
> +
> +       return 0;
> +}
> +
> +static void tegra_hsp_doorbell_shutdown(struct tegra_hsp_channel *channel)
> +{
> +       struct tegra_hsp_doorbell *db = to_tegra_hsp_doorbell(channel);
> +       struct tegra_hsp *hsp = channel->hsp;
> +       struct tegra_hsp_doorbell *ccplex;
> +       unsigned long flags;
> +       u32 value;
> +
> +       ccplex = tegra_hsp_doorbell_get(hsp, TEGRA_HSP_DB_MASTER_CCPLEX);
> +       if (!ccplex)
> +               return;
> +
> +       spin_lock_irqsave(&hsp->lock, flags);
> +
> +       value = tegra_hsp_channel_readl(&ccplex->channel, HSP_DB_ENABLE);
> +       value &= ~BIT(db->master);
> +       tegra_hsp_channel_writel(&ccplex->channel, value, HSP_DB_ENABLE);
> +
> +       spin_unlock_irqrestore(&hsp->lock, flags);
> +}
> +
> +static bool tegra_hsp_doorbell_last_tx_done(struct tegra_hsp_channel *channel)
> +{
> +       return true;
> +}
Just curious, is the IPC done instantly after writing HSP_DB_TRIGGER
bit? Usually there is at least some bit that stays (un)set as a 'busy
flag'.

> +
> +static const struct tegra_hsp_channel_ops tegra_hsp_doorbell_ops = {
> +       .send_data = tegra_hsp_doorbell_send_data,
> +       .startup = tegra_hsp_doorbell_startup,
> +       .shutdown = tegra_hsp_doorbell_shutdown,
> +       .last_tx_done = tegra_hsp_doorbell_last_tx_done,
> +};
> +
....

> +static int tegra_hsp_send_data(struct mbox_chan *chan, void *data)
> +{
> +       struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan);
> +
> +       return channel->ops->send_data(channel, data);
> +}
> +
> +static int tegra_hsp_startup(struct mbox_chan *chan)
> +{
> +       struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan);
> +
> +       return channel->ops->startup(channel);
> +}
> +
> +static void tegra_hsp_shutdown(struct mbox_chan *chan)
> +{
> +       struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan);
> +
> +       return channel->ops->shutdown(channel);
> +}
> +
> +static bool tegra_hsp_last_tx_done(struct mbox_chan *chan)
> +{
> +       struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan);
> +
> +       return channel->ops->last_tx_done(channel);
> +}
> +
> +static const struct mbox_chan_ops tegra_hsp_ops = {
> +       .send_data = tegra_hsp_send_data,
> +       .startup = tegra_hsp_startup,
> +       .shutdown = tegra_hsp_shutdown,
> +       .last_tx_done = tegra_hsp_last_tx_done,
> +};
> +
These 4 above seem overkill. Why not directly use tegra_hsp_doorbell_xxx() ?

Cheers!

Re: [PATCH v4 2/2] mailbox: Add Tegra HSP driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5294 bytes --]

On Wed, Nov 16, 2016 at 10:58:07AM +0530, Jassi Brar wrote:
> On Tue, Nov 15, 2016 at 9:18 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> 
> ....
> > +
> > +struct tegra_hsp_channel;
> > +struct tegra_hsp;
> > +
> > +struct tegra_hsp_channel_ops {
> > +       int (*send_data)(struct tegra_hsp_channel *channel, void *data);
> > +       int (*startup)(struct tegra_hsp_channel *channel);
> > +       void (*shutdown)(struct tegra_hsp_channel *channel);
> > +       bool (*last_tx_done)(struct tegra_hsp_channel *channel);
> > +};
> > +
> > +struct tegra_hsp_channel {
> > +       struct tegra_hsp *hsp;
> > +       const struct tegra_hsp_channel_ops *ops;
> > +       struct mbox_chan *chan;
> > +       void __iomem *regs;
> > +};
> > +
> > +static struct tegra_hsp_channel *to_tegra_hsp_channel(struct mbox_chan *chan)
> > +{
> > +       return chan->con_priv;
> > +}
> > +
> It seems
>        channel = to_tegra_hsp_channel(chan);
> is no simpler ritual than
>        channel = chan->con_priv;   ?

Yes, that's true. I've dropped the to_tegra_hsp_channel() inline in
favour of using the chan->con_priv directly.

> > +struct tegra_hsp_doorbell {
> > +       struct tegra_hsp_channel channel;
> > +       struct list_head list;
> > +       const char *name;
> > +       unsigned int master;
> > +       unsigned int index;
> > +};
> > +
> > +static struct tegra_hsp_doorbell *
> > +to_tegra_hsp_doorbell(struct tegra_hsp_channel *channel)
> > +{
> > +       if (!channel)
> > +               return NULL;
> > +
> > +       return container_of(channel, struct tegra_hsp_doorbell, channel);
> > +}
> > +
> But you don't check for NULL returned, before dereferencing the pointer 'db'

In all the call sites where this is used the channel is guaranteed not
to be NULL, hence no checking is necessary. However the function here
could potentially be used in other cases where no such guarantees can
be given and checking the !channel above is merely there to avoid
casting to a non-NULL pointer from a NULL pointer.

I've run occasionally into this issue because container_of() will simply
perform arithmetic on the pointer given, so passing channel as NULL
would convert to some very large pointer that can no longer be easily
discerned from an invalid pointer.

So this is primarily a safety feature, and one that I'd prefer to keep
just to avoid running into issues down the road when the function gets
used under different circumstances.

> > +static bool tegra_hsp_doorbell_last_tx_done(struct tegra_hsp_channel *channel)
> > +{
> > +       return true;
> > +}
> Just curious, is the IPC done instantly after writing HSP_DB_TRIGGER
> bit? Usually there is at least some bit that stays (un)set as a 'busy
> flag'.

I don't think there's a bit like that for doorbells. The way that these
doorbells are used is in combination with a shared memory IPC protocol.
Two processors will communicate by writing to and reading from what is
essentially a ring buffer in shared memory. The doorbells are merely a
means of communicating their peer that a new entry is available in the
shared memory.

> > +static const struct tegra_hsp_channel_ops tegra_hsp_doorbell_ops = {
> > +       .send_data = tegra_hsp_doorbell_send_data,
> > +       .startup = tegra_hsp_doorbell_startup,
> > +       .shutdown = tegra_hsp_doorbell_shutdown,
> > +       .last_tx_done = tegra_hsp_doorbell_last_tx_done,
> > +};
> > +
> ....
> 
> > +static int tegra_hsp_send_data(struct mbox_chan *chan, void *data)
> > +{
> > +       struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan);
> > +
> > +       return channel->ops->send_data(channel, data);
> > +}
> > +
> > +static int tegra_hsp_startup(struct mbox_chan *chan)
> > +{
> > +       struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan);
> > +
> > +       return channel->ops->startup(channel);
> > +}
> > +
> > +static void tegra_hsp_shutdown(struct mbox_chan *chan)
> > +{
> > +       struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan);
> > +
> > +       return channel->ops->shutdown(channel);
> > +}
> > +
> > +static bool tegra_hsp_last_tx_done(struct mbox_chan *chan)
> > +{
> > +       struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan);
> > +
> > +       return channel->ops->last_tx_done(channel);
> > +}
> > +
> > +static const struct mbox_chan_ops tegra_hsp_ops = {
> > +       .send_data = tegra_hsp_send_data,
> > +       .startup = tegra_hsp_startup,
> > +       .shutdown = tegra_hsp_shutdown,
> > +       .last_tx_done = tegra_hsp_last_tx_done,
> > +};
> > +
> These 4 above seem overkill. Why not directly use tegra_hsp_doorbell_xxx() ?

This is in preparation for supporting the other synchronization
primitives that the HSP IP block exposes. Some of them use different
programming and semantics, hence why we want to have this second level
of abstraction. It will allow us to share some of the code between the
different primitives once their implementations are added.

If you feel really strongly about it, I suppose I could eliminate it,
but I suspect that we'd want to add them back in after support for the
other primitives is added.

Thanks,
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v4 2/2] mailbox: Add Tegra HSP driver

[Jassi Brar]

On Wed, Nov 16, 2016 at 8:38 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Wed, Nov 16, 2016 at 10:58:07AM +0530, Jassi Brar wrote:
>> On Tue, Nov 15, 2016 at 9:18 PM, Thierry Reding
>> <thierry.reding@gmail.com> wrote:
>>
>> ....
>> > +
>> > +struct tegra_hsp_channel;
>> > +struct tegra_hsp;
>> > +
>> > +struct tegra_hsp_channel_ops {
>> > +       int (*send_data)(struct tegra_hsp_channel *channel, void *data);
>> > +       int (*startup)(struct tegra_hsp_channel *channel);
>> > +       void (*shutdown)(struct tegra_hsp_channel *channel);
>> > +       bool (*last_tx_done)(struct tegra_hsp_channel *channel);
>> > +};
>> > +
>> > +struct tegra_hsp_channel {
>> > +       struct tegra_hsp *hsp;
>> > +       const struct tegra_hsp_channel_ops *ops;
>> > +       struct mbox_chan *chan;
>> > +       void __iomem *regs;
>> > +};
>> > +
>> > +static struct tegra_hsp_channel *to_tegra_hsp_channel(struct mbox_chan *chan)
>> > +{
>> > +       return chan->con_priv;
>> > +}
>> > +
>> It seems
>>        channel = to_tegra_hsp_channel(chan);
>> is no simpler ritual than
>>        channel = chan->con_priv;   ?
>
> Yes, that's true. I've dropped the to_tegra_hsp_channel() inline in
> favour of using the chan->con_priv directly.
>
>> > +struct tegra_hsp_doorbell {
>> > +       struct tegra_hsp_channel channel;
>> > +       struct list_head list;
>> > +       const char *name;
>> > +       unsigned int master;
>> > +       unsigned int index;
>> > +};
>> > +
>> > +static struct tegra_hsp_doorbell *
>> > +to_tegra_hsp_doorbell(struct tegra_hsp_channel *channel)
>> > +{
>> > +       if (!channel)
>> > +               return NULL;
>> > +
>> > +       return container_of(channel, struct tegra_hsp_doorbell, channel);
>> > +}
>> > +
>> But you don't check for NULL returned, before dereferencing the pointer 'db'
>
> In all the call sites where this is used the channel is guaranteed not
> to be NULL, hence no checking is necessary. However the function here
> could potentially be used in other cases where no such guarantees can
> be given and checking the !channel above is merely there to avoid
> casting to a non-NULL pointer from a NULL pointer.
>
> I've run occasionally into this issue because container_of() will simply
> perform arithmetic on the pointer given, so passing channel as NULL
> would convert to some very large pointer that can no longer be easily
> discerned from an invalid pointer.
>
> So this is primarily a safety feature, and one that I'd prefer to keep
> just to avoid running into issues down the road when the function gets
> used under different circumstances.
>
>> > +static bool tegra_hsp_doorbell_last_tx_done(struct tegra_hsp_channel *channel)
>> > +{
>> > +       return true;
>> > +}
>> Just curious, is the IPC done instantly after writing HSP_DB_TRIGGER
>> bit? Usually there is at least some bit that stays (un)set as a 'busy
>> flag'.
>
> I don't think there's a bit like that for doorbells. The way that these
> doorbells are used is in combination with a shared memory IPC protocol.
> Two processors will communicate by writing to and reading from what is
> essentially a ring buffer in shared memory. The doorbells are merely a
> means of communicating their peer that a new entry is available in the
> shared memory.
>
For such protocols, we have the TXDONE_BY_ACK. I assume your client
drivers will drive the state-machine. Otherwise, you risk overrunning
the ring-buffer in SHM, but not caring if the first filled buffer was
actually consumed by the remote (just like ALSA ring buffer).

>> > +static const struct tegra_hsp_channel_ops tegra_hsp_doorbell_ops = {
>> > +       .send_data = tegra_hsp_doorbell_send_data,
>> > +       .startup = tegra_hsp_doorbell_startup,
>> > +       .shutdown = tegra_hsp_doorbell_shutdown,
>> > +       .last_tx_done = tegra_hsp_doorbell_last_tx_done,
>> > +};
>> > +
>> ....
>>
>> > +static int tegra_hsp_send_data(struct mbox_chan *chan, void *data)
>> > +{
>> > +       struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan);
>> > +
>> > +       return channel->ops->send_data(channel, data);
>> > +}
>> > +
>> > +static int tegra_hsp_startup(struct mbox_chan *chan)
>> > +{
>> > +       struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan);
>> > +
>> > +       return channel->ops->startup(channel);
>> > +}
>> > +
>> > +static void tegra_hsp_shutdown(struct mbox_chan *chan)
>> > +{
>> > +       struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan);
>> > +
>> > +       return channel->ops->shutdown(channel);
>> > +}
>> > +
>> > +static bool tegra_hsp_last_tx_done(struct mbox_chan *chan)
>> > +{
>> > +       struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan);
>> > +
>> > +       return channel->ops->last_tx_done(channel);
>> > +}
>> > +
>> > +static const struct mbox_chan_ops tegra_hsp_ops = {
>> > +       .send_data = tegra_hsp_send_data,
>> > +       .startup = tegra_hsp_startup,
>> > +       .shutdown = tegra_hsp_shutdown,
>> > +       .last_tx_done = tegra_hsp_last_tx_done,
>> > +};
>> > +
>> These 4 above seem overkill. Why not directly use tegra_hsp_doorbell_xxx() ?
>
> This is in preparation for supporting the other synchronization
> primitives that the HSP IP block exposes. Some of them use different
> programming and semantics, hence why we want to have this second level
> of abstraction. It will allow us to share some of the code between the
> different primitives once their implementations are added.
>
OK, but until then this, and the above NULL check, will look silly.
Usually we add only necessary code at any time.

Thanks.

[PATCH v5] mailbox: Add Tegra HSP driver

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This driver exposes a mailbox interface for interprocessor communication
using the Hardware Synchronization Primitives (HSP) module's doorbell
mechanism. There are multiple HSP instances and they provide additional
features such as shared mailboxes, shared and arbitrated semaphores.

A driver for a remote processor can use the mailbox client provided by
the HSP driver and build an IPC protocol on top of this synchronization
mechanism.

Based on work by Joseph Lo <josephl@nvidia.com>.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v5:
- remove unused ->last_tx_done() callback
- get rid of extra layer of indirection
- drop useless to_tegra_hsp_channel()

Changes in v4:
- check for doorbell ring before enabling new master
- add missing space in Kconfig description
- remove unnecessary barriers
- simplify doorbell lookup
- remove debug leftovers

Changes in v3:
- use a more object oriented design
- fix some locking issues

 drivers/mailbox/Kconfig     |   9 +
 drivers/mailbox/Makefile    |   2 +
 drivers/mailbox/tegra-hsp.c | 479 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 490 insertions(+)
 create mode 100644 drivers/mailbox/tegra-hsp.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 11eebfe8a4cb..ceff415f201c 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -124,6 +124,15 @@ config MAILBOX_TEST
 	  Test client to help with testing new Controller driver
 	  implementations.
 
+config TEGRA_HSP_MBOX
+	bool "Tegra HSP (Hardware Synchronization Primitives) Driver"
+	depends on ARCH_TEGRA_186_SOC
+	help
+	  The Tegra HSP driver is used for the interprocessor communication
+	  between different remote processors and host processors on Tegra186
+	  and later SoCs. Say Y here if you want to have this support.
+	  If unsure say N.
+
 config XGENE_SLIMPRO_MBOX
 	tristate "APM SoC X-Gene SLIMpro Mailbox Controller"
 	depends on ARCH_XGENE
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index ace6fed8fea9..7dde4f609ae8 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -29,3 +29,5 @@ obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
 obj-$(CONFIG_HI6220_MBOX)	+= hi6220-mailbox.o
 
 obj-$(CONFIG_BCM_PDC_MBOX)	+= bcm-pdc-mailbox.o
+
+obj-$(CONFIG_TEGRA_HSP_MBOX)	+= tegra-hsp.o
diff --git a/drivers/mailbox/tegra-hsp.c b/drivers/mailbox/tegra-hsp.c
new file mode 100644
index 000000000000..3724f2fcd371
--- /dev/null
+++ b/drivers/mailbox/tegra-hsp.c
@@ -0,0 +1,479 @@
+/*
+ * Copyright (c) 2016, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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/interrupt.h>
+#include <linux/io.h>
+#include <linux/mailbox_controller.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include <dt-bindings/mailbox/tegra186-hsp.h>
+
+#define HSP_INT_DIMENSIONING	0x380
+#define HSP_nSM_SHIFT		0
+#define HSP_nSS_SHIFT		4
+#define HSP_nAS_SHIFT		8
+#define HSP_nDB_SHIFT		12
+#define HSP_nSI_SHIFT		16
+#define HSP_nINT_MASK		0xf
+
+#define HSP_DB_TRIGGER	0x0
+#define HSP_DB_ENABLE	0x4
+#define HSP_DB_RAW	0x8
+#define HSP_DB_PENDING	0xc
+
+#define HSP_DB_CCPLEX		1
+#define HSP_DB_BPMP		3
+#define HSP_DB_MAX		7
+
+struct tegra_hsp_channel;
+struct tegra_hsp;
+
+struct tegra_hsp_channel {
+	struct tegra_hsp *hsp;
+	struct mbox_chan *chan;
+	void __iomem *regs;
+};
+
+struct tegra_hsp_doorbell {
+	struct tegra_hsp_channel channel;
+	struct list_head list;
+	const char *name;
+	unsigned int master;
+	unsigned int index;
+};
+
+struct tegra_hsp_db_map {
+	const char *name;
+	unsigned int master;
+	unsigned int index;
+};
+
+struct tegra_hsp_soc {
+	const struct tegra_hsp_db_map *map;
+};
+
+struct tegra_hsp {
+	const struct tegra_hsp_soc *soc;
+	struct mbox_controller mbox;
+	void __iomem *regs;
+	unsigned int irq;
+	unsigned int num_sm;
+	unsigned int num_as;
+	unsigned int num_ss;
+	unsigned int num_db;
+	unsigned int num_si;
+	spinlock_t lock;
+
+	struct list_head doorbells;
+};
+
+static inline struct tegra_hsp *
+to_tegra_hsp(struct mbox_controller *mbox)
+{
+	return container_of(mbox, struct tegra_hsp, mbox);
+}
+
+static inline u32 tegra_hsp_readl(struct tegra_hsp *hsp, unsigned int offset)
+{
+	return readl(hsp->regs + offset);
+}
+
+static inline void tegra_hsp_writel(struct tegra_hsp *hsp, u32 value,
+				    unsigned int offset)
+{
+	writel(value, hsp->regs + offset);
+}
+
+static inline u32 tegra_hsp_channel_readl(struct tegra_hsp_channel *channel,
+					  unsigned int offset)
+{
+	return readl(channel->regs + offset);
+}
+
+static inline void tegra_hsp_channel_writel(struct tegra_hsp_channel *channel,
+					    u32 value, unsigned int offset)
+{
+	writel(value, channel->regs + offset);
+}
+
+static bool tegra_hsp_doorbell_can_ring(struct tegra_hsp_doorbell *db)
+{
+	u32 value;
+
+	value = tegra_hsp_channel_readl(&db->channel, HSP_DB_ENABLE);
+
+	return (value & BIT(TEGRA_HSP_DB_MASTER_CCPLEX)) != 0;
+}
+
+static struct tegra_hsp_doorbell *
+__tegra_hsp_doorbell_get(struct tegra_hsp *hsp, unsigned int master)
+{
+	struct tegra_hsp_doorbell *entry;
+
+	list_for_each_entry(entry, &hsp->doorbells, list)
+		if (entry->master == master)
+			return entry;
+
+	return NULL;
+}
+
+static struct tegra_hsp_doorbell *
+tegra_hsp_doorbell_get(struct tegra_hsp *hsp, unsigned int master)
+{
+	struct tegra_hsp_doorbell *db;
+	unsigned long flags;
+
+	spin_lock_irqsave(&hsp->lock, flags);
+	db = __tegra_hsp_doorbell_get(hsp, master);
+	spin_unlock_irqrestore(&hsp->lock, flags);
+
+	return db;
+}
+
+static irqreturn_t tegra_hsp_doorbell_irq(int irq, void *data)
+{
+	struct tegra_hsp *hsp = data;
+	struct tegra_hsp_doorbell *db;
+	unsigned long master, value;
+
+	db = tegra_hsp_doorbell_get(hsp, TEGRA_HSP_DB_MASTER_CCPLEX);
+	if (!db)
+		return IRQ_NONE;
+
+	value = tegra_hsp_channel_readl(&db->channel, HSP_DB_PENDING);
+	tegra_hsp_channel_writel(&db->channel, value, HSP_DB_PENDING);
+
+	spin_lock(&hsp->lock);
+
+	for_each_set_bit(master, &value, hsp->mbox.num_chans) {
+		struct tegra_hsp_doorbell *db;
+
+		db = __tegra_hsp_doorbell_get(hsp, master);
+		/*
+		 * Depending on the bootloader chain, the CCPLEX doorbell will
+		 * have some doorbells enabled, which means that requesting an
+		 * interrupt will immediately fire.
+		 *
+		 * In that case, db->channel.chan will still be NULL here and
+		 * cause a crash if not properly guarded.
+		 *
+		 * It remains to be seen if ignoring the doorbell in that case
+		 * is the correct solution.
+		 */
+		if (db && db->channel.chan)
+			mbox_chan_received_data(db->channel.chan, NULL);
+	}
+
+	spin_unlock(&hsp->lock);
+
+	return IRQ_HANDLED;
+}
+
+static struct tegra_hsp_channel *
+tegra_hsp_doorbell_create(struct tegra_hsp *hsp, const char *name,
+			  unsigned int master, unsigned int index)
+{
+	struct tegra_hsp_doorbell *db;
+	unsigned int offset;
+	unsigned long flags;
+
+	db = kzalloc(sizeof(*db), GFP_KERNEL);
+	if (!db)
+		return ERR_PTR(-ENOMEM);
+
+	offset = (1 + (hsp->num_sm / 2) + hsp->num_ss + hsp->num_as) << 16;
+	offset += index * 0x100;
+
+	db->channel.regs = hsp->regs + offset;
+	db->channel.hsp = hsp;
+
+	db->name = kstrdup_const(name, GFP_KERNEL);
+	db->master = master;
+	db->index = index;
+
+	spin_lock_irqsave(&hsp->lock, flags);
+	list_add_tail(&db->list, &hsp->doorbells);
+	spin_unlock_irqrestore(&hsp->lock, flags);
+
+	return &db->channel;
+}
+
+static void __tegra_hsp_doorbell_destroy(struct tegra_hsp_doorbell *db)
+{
+	list_del(&db->list);
+	kfree_const(db->name);
+	kfree(db);
+}
+
+static int tegra_hsp_doorbell_send_data(struct mbox_chan *chan, void *data)
+{
+	struct tegra_hsp_doorbell *db = chan->con_priv;
+
+	tegra_hsp_channel_writel(&db->channel, 1, HSP_DB_TRIGGER);
+
+	return 0;
+}
+
+static int tegra_hsp_doorbell_startup(struct mbox_chan *chan)
+{
+	struct tegra_hsp_doorbell *db = chan->con_priv;
+	struct tegra_hsp *hsp = db->channel.hsp;
+	struct tegra_hsp_doorbell *ccplex;
+	unsigned long flags;
+	u32 value;
+
+	if (db->master >= hsp->mbox.num_chans) {
+		dev_err(hsp->mbox.dev,
+			"invalid master ID %u for HSP channel\n",
+			db->master);
+		return -EINVAL;
+	}
+
+	ccplex = tegra_hsp_doorbell_get(hsp, TEGRA_HSP_DB_MASTER_CCPLEX);
+	if (!ccplex)
+		return -ENODEV;
+
+	if (!tegra_hsp_doorbell_can_ring(db))
+		return -ENODEV;
+
+	spin_lock_irqsave(&hsp->lock, flags);
+
+	value = tegra_hsp_channel_readl(&ccplex->channel, HSP_DB_ENABLE);
+	value |= BIT(db->master);
+	tegra_hsp_channel_writel(&ccplex->channel, value, HSP_DB_ENABLE);
+
+	spin_unlock_irqrestore(&hsp->lock, flags);
+
+	return 0;
+}
+
+static void tegra_hsp_doorbell_shutdown(struct mbox_chan *chan)
+{
+	struct tegra_hsp_doorbell *db = chan->con_priv;
+	struct tegra_hsp *hsp = db->channel.hsp;
+	struct tegra_hsp_doorbell *ccplex;
+	unsigned long flags;
+	u32 value;
+
+	ccplex = tegra_hsp_doorbell_get(hsp, TEGRA_HSP_DB_MASTER_CCPLEX);
+	if (!ccplex)
+		return;
+
+	spin_lock_irqsave(&hsp->lock, flags);
+
+	value = tegra_hsp_channel_readl(&ccplex->channel, HSP_DB_ENABLE);
+	value &= ~BIT(db->master);
+	tegra_hsp_channel_writel(&ccplex->channel, value, HSP_DB_ENABLE);
+
+	spin_unlock_irqrestore(&hsp->lock, flags);
+}
+
+static const struct mbox_chan_ops tegra_hsp_doorbell_ops = {
+	.send_data = tegra_hsp_doorbell_send_data,
+	.startup = tegra_hsp_doorbell_startup,
+	.shutdown = tegra_hsp_doorbell_shutdown,
+};
+
+static struct mbox_chan *of_tegra_hsp_xlate(struct mbox_controller *mbox,
+					    const struct of_phandle_args *args)
+{
+	struct tegra_hsp_channel *channel = ERR_PTR(-ENODEV);
+	struct tegra_hsp *hsp = to_tegra_hsp(mbox);
+	unsigned int type = args->args[0];
+	unsigned int master = args->args[1];
+	struct tegra_hsp_doorbell *db;
+	struct mbox_chan *chan;
+	unsigned long flags;
+	unsigned int i;
+
+	switch (type) {
+	case TEGRA_HSP_MBOX_TYPE_DB:
+		db = tegra_hsp_doorbell_get(hsp, master);
+		if (db)
+			channel = &db->channel;
+
+		break;
+
+	default:
+		break;
+	}
+
+	if (IS_ERR(channel))
+		return ERR_CAST(channel);
+
+	spin_lock_irqsave(&hsp->lock, flags);
+
+	for (i = 0; i < hsp->mbox.num_chans; i++) {
+		chan = &hsp->mbox.chans[i];
+		if (!chan->con_priv) {
+			chan->con_priv = channel;
+			channel->chan = chan;
+			break;
+		}
+
+		chan = NULL;
+	}
+
+	spin_unlock_irqrestore(&hsp->lock, flags);
+
+	return chan ?: ERR_PTR(-EBUSY);
+}
+
+static void tegra_hsp_remove_doorbells(struct tegra_hsp *hsp)
+{
+	struct tegra_hsp_doorbell *db;
+	unsigned long flags;
+
+	spin_lock_irqsave(&hsp->lock, flags);
+
+	list_for_each_entry(db, &hsp->doorbells, list)
+		__tegra_hsp_doorbell_destroy(db);
+
+	spin_unlock_irqrestore(&hsp->lock, flags);
+}
+
+static int tegra_hsp_add_doorbells(struct tegra_hsp *hsp)
+{
+	const struct tegra_hsp_db_map *map = hsp->soc->map;
+	struct tegra_hsp_channel *channel;
+
+	while (map->name) {
+		channel = tegra_hsp_doorbell_create(hsp, map->name,
+						    map->master, map->index);
+		if (IS_ERR(channel)) {
+			tegra_hsp_remove_doorbells(hsp);
+			return PTR_ERR(channel);
+		}
+
+		map++;
+	}
+
+	return 0;
+}
+
+static int tegra_hsp_probe(struct platform_device *pdev)
+{
+	struct tegra_hsp *hsp;
+	struct resource *res;
+	u32 value;
+	int err;
+
+	hsp = devm_kzalloc(&pdev->dev, sizeof(*hsp), GFP_KERNEL);
+	if (!hsp)
+		return -ENOMEM;
+
+	hsp->soc = of_device_get_match_data(&pdev->dev);
+	INIT_LIST_HEAD(&hsp->doorbells);
+	spin_lock_init(&hsp->lock);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	hsp->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(hsp->regs))
+		return PTR_ERR(hsp->regs);
+
+	value = tegra_hsp_readl(hsp, HSP_INT_DIMENSIONING);
+	hsp->num_sm = (value >> HSP_nSM_SHIFT) & HSP_nINT_MASK;
+	hsp->num_ss = (value >> HSP_nSS_SHIFT) & HSP_nINT_MASK;
+	hsp->num_as = (value >> HSP_nAS_SHIFT) & HSP_nINT_MASK;
+	hsp->num_db = (value >> HSP_nDB_SHIFT) & HSP_nINT_MASK;
+	hsp->num_si = (value >> HSP_nSI_SHIFT) & HSP_nINT_MASK;
+
+	err = platform_get_irq_byname(pdev, "doorbell");
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to get doorbell IRQ: %d\n", err);
+		return err;
+	}
+
+	hsp->irq = err;
+
+	hsp->mbox.of_xlate = of_tegra_hsp_xlate;
+	hsp->mbox.num_chans = 32;
+	hsp->mbox.dev = &pdev->dev;
+	hsp->mbox.txdone_irq = false;
+	hsp->mbox.txdone_poll = false;
+	hsp->mbox.ops = &tegra_hsp_doorbell_ops;
+
+	hsp->mbox.chans = devm_kcalloc(&pdev->dev, hsp->mbox.num_chans,
+					sizeof(*hsp->mbox.chans),
+					GFP_KERNEL);
+	if (!hsp->mbox.chans)
+		return -ENOMEM;
+
+	err = tegra_hsp_add_doorbells(hsp);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to add doorbells: %d\n", err);
+		return err;
+	}
+
+	platform_set_drvdata(pdev, hsp);
+
+	err = mbox_controller_register(&hsp->mbox);
+	if (err) {
+		dev_err(&pdev->dev, "failed to register mailbox: %d\n", err);
+		tegra_hsp_remove_doorbells(hsp);
+		return err;
+	}
+
+	err = devm_request_irq(&pdev->dev, hsp->irq, tegra_hsp_doorbell_irq,
+			       IRQF_NO_SUSPEND, dev_name(&pdev->dev), hsp);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to request IRQ#%u: %d\n",
+			hsp->irq, err);
+		return err;
+	}
+
+	return 0;
+}
+
+static int tegra_hsp_remove(struct platform_device *pdev)
+{
+	struct tegra_hsp *hsp = platform_get_drvdata(pdev);
+
+	mbox_controller_unregister(&hsp->mbox);
+	tegra_hsp_remove_doorbells(hsp);
+
+	return 0;
+}
+
+static const struct tegra_hsp_db_map tegra186_hsp_db_map[] = {
+	{ "ccplex", TEGRA_HSP_DB_MASTER_CCPLEX, HSP_DB_CCPLEX, },
+	{ "bpmp",   TEGRA_HSP_DB_MASTER_BPMP,   HSP_DB_BPMP,   },
+	{ /* sentinel */ }
+};
+
+static const struct tegra_hsp_soc tegra186_hsp_soc = {
+	.map = tegra186_hsp_db_map,
+};
+
+static const struct of_device_id tegra_hsp_match[] = {
+	{ .compatible = "nvidia,tegra186-hsp", .data = &tegra186_hsp_soc },
+	{ }
+};
+
+static struct platform_driver tegra_hsp_driver = {
+	.driver = {
+		.name = "tegra-hsp",
+		.of_match_table = tegra_hsp_match,
+	},
+	.probe = tegra_hsp_probe,
+	.remove = tegra_hsp_remove,
+};
+
+static int __init tegra_hsp_init(void)
+{
+	return platform_driver_register(&tegra_hsp_driver);
+}
+core_initcall(tegra_hsp_init);
-- 
2.10.2

Re: [PATCH v4 2/2] mailbox: Add Tegra HSP driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6590 bytes --]

On Wed, Nov 16, 2016 at 09:00:19PM +0530, Jassi Brar wrote:
> On Wed, Nov 16, 2016 at 8:38 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Wed, Nov 16, 2016 at 10:58:07AM +0530, Jassi Brar wrote:
> >> On Tue, Nov 15, 2016 at 9:18 PM, Thierry Reding
> >> <thierry.reding@gmail.com> wrote:
> >>
> >> ....
> >> > +
> >> > +struct tegra_hsp_channel;
> >> > +struct tegra_hsp;
> >> > +
> >> > +struct tegra_hsp_channel_ops {
> >> > +       int (*send_data)(struct tegra_hsp_channel *channel, void *data);
> >> > +       int (*startup)(struct tegra_hsp_channel *channel);
> >> > +       void (*shutdown)(struct tegra_hsp_channel *channel);
> >> > +       bool (*last_tx_done)(struct tegra_hsp_channel *channel);
> >> > +};
> >> > +
> >> > +struct tegra_hsp_channel {
> >> > +       struct tegra_hsp *hsp;
> >> > +       const struct tegra_hsp_channel_ops *ops;
> >> > +       struct mbox_chan *chan;
> >> > +       void __iomem *regs;
> >> > +};
> >> > +
> >> > +static struct tegra_hsp_channel *to_tegra_hsp_channel(struct mbox_chan *chan)
> >> > +{
> >> > +       return chan->con_priv;
> >> > +}
> >> > +
> >> It seems
> >>        channel = to_tegra_hsp_channel(chan);
> >> is no simpler ritual than
> >>        channel = chan->con_priv;   ?
> >
> > Yes, that's true. I've dropped the to_tegra_hsp_channel() inline in
> > favour of using the chan->con_priv directly.
> >
> >> > +struct tegra_hsp_doorbell {
> >> > +       struct tegra_hsp_channel channel;
> >> > +       struct list_head list;
> >> > +       const char *name;
> >> > +       unsigned int master;
> >> > +       unsigned int index;
> >> > +};
> >> > +
> >> > +static struct tegra_hsp_doorbell *
> >> > +to_tegra_hsp_doorbell(struct tegra_hsp_channel *channel)
> >> > +{
> >> > +       if (!channel)
> >> > +               return NULL;
> >> > +
> >> > +       return container_of(channel, struct tegra_hsp_doorbell, channel);
> >> > +}
> >> > +
> >> But you don't check for NULL returned, before dereferencing the pointer 'db'
> >
> > In all the call sites where this is used the channel is guaranteed not
> > to be NULL, hence no checking is necessary. However the function here
> > could potentially be used in other cases where no such guarantees can
> > be given and checking the !channel above is merely there to avoid
> > casting to a non-NULL pointer from a NULL pointer.
> >
> > I've run occasionally into this issue because container_of() will simply
> > perform arithmetic on the pointer given, so passing channel as NULL
> > would convert to some very large pointer that can no longer be easily
> > discerned from an invalid pointer.
> >
> > So this is primarily a safety feature, and one that I'd prefer to keep
> > just to avoid running into issues down the road when the function gets
> > used under different circumstances.
> >
> >> > +static bool tegra_hsp_doorbell_last_tx_done(struct tegra_hsp_channel *channel)
> >> > +{
> >> > +       return true;
> >> > +}
> >> Just curious, is the IPC done instantly after writing HSP_DB_TRIGGER
> >> bit? Usually there is at least some bit that stays (un)set as a 'busy
> >> flag'.
> >
> > I don't think there's a bit like that for doorbells. The way that these
> > doorbells are used is in combination with a shared memory IPC protocol.
> > Two processors will communicate by writing to and reading from what is
> > essentially a ring buffer in shared memory. The doorbells are merely a
> > means of communicating their peer that a new entry is available in the
> > shared memory.
> >
> For such protocols, we have the TXDONE_BY_ACK. I assume your client
> drivers will drive the state-machine. Otherwise, you risk overrunning
> the ring-buffer in SHM, but not caring if the first filled buffer was
> actually consumed by the remote (just like ALSA ring buffer).

I did some digging and it turns out that the driver is already using
TXDONE_BY_ACK (it sets .txdone_irq = false and .txdone_poll = false)
and indeed users are driving the TX state machine by calling the
mbox_client_txdone() function where appropriate.

Given the above the .last_tx_done() callback is completely unused so
I've just dropped it.

> >> > +static const struct tegra_hsp_channel_ops tegra_hsp_doorbell_ops = {
> >> > +       .send_data = tegra_hsp_doorbell_send_data,
> >> > +       .startup = tegra_hsp_doorbell_startup,
> >> > +       .shutdown = tegra_hsp_doorbell_shutdown,
> >> > +       .last_tx_done = tegra_hsp_doorbell_last_tx_done,
> >> > +};
> >> > +
> >> ....
> >>
> >> > +static int tegra_hsp_send_data(struct mbox_chan *chan, void *data)
> >> > +{
> >> > +       struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan);
> >> > +
> >> > +       return channel->ops->send_data(channel, data);
> >> > +}
> >> > +
> >> > +static int tegra_hsp_startup(struct mbox_chan *chan)
> >> > +{
> >> > +       struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan);
> >> > +
> >> > +       return channel->ops->startup(channel);
> >> > +}
> >> > +
> >> > +static void tegra_hsp_shutdown(struct mbox_chan *chan)
> >> > +{
> >> > +       struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan);
> >> > +
> >> > +       return channel->ops->shutdown(channel);
> >> > +}
> >> > +
> >> > +static bool tegra_hsp_last_tx_done(struct mbox_chan *chan)
> >> > +{
> >> > +       struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan);
> >> > +
> >> > +       return channel->ops->last_tx_done(channel);
> >> > +}
> >> > +
> >> > +static const struct mbox_chan_ops tegra_hsp_ops = {
> >> > +       .send_data = tegra_hsp_send_data,
> >> > +       .startup = tegra_hsp_startup,
> >> > +       .shutdown = tegra_hsp_shutdown,
> >> > +       .last_tx_done = tegra_hsp_last_tx_done,
> >> > +};
> >> > +
> >> These 4 above seem overkill. Why not directly use tegra_hsp_doorbell_xxx() ?
> >
> > This is in preparation for supporting the other synchronization
> > primitives that the HSP IP block exposes. Some of them use different
> > programming and semantics, hence why we want to have this second level
> > of abstraction. It will allow us to share some of the code between the
> > different primitives once their implementations are added.
> >
> OK, but until then this, and the above NULL check, will look silly.
> Usually we add only necessary code at any time.

I've removed the additional level of indirection, we can always add this
back when/if required. As a side-effect the to_tegra_hsp_doorbell() goes
away because it is unused, thereby removing your last concern as well.

I just sent out a v5 that should address all of your comments.

Thanks,
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v5] mailbox: Add Tegra HSP driver

[Jassi Brar]

On Wed, Nov 16, 2016 at 11:11 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> This driver exposes a mailbox interface for interprocessor communication
> using the Hardware Synchronization Primitives (HSP) module's doorbell
> mechanism. There are multiple HSP instances and they provide additional
> features such as shared mailboxes, shared and arbitrated semaphores.
>
> A driver for a remote processor can use the mailbox client provided by
> the HSP driver and build an IPC protocol on top of this synchronization
> mechanism.
>
> Based on work by Joseph Lo <josephl@nvidia.com>.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Acked-by: Jassi Brar <jaswinder.singh@linaro.org>

Re: [PATCH v5] mailbox: Add Tegra HSP driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

On Fri, Nov 18, 2016 at 02:28:07PM +0530, Jassi Brar wrote:
> On Wed, Nov 16, 2016 at 11:11 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > From: Thierry Reding <treding@nvidia.com>
> >
> > This driver exposes a mailbox interface for interprocessor communication
> > using the Hardware Synchronization Primitives (HSP) module's doorbell
> > mechanism. There are multiple HSP instances and they provide additional
> > features such as shared mailboxes, shared and arbitrated semaphores.
> >
> > A driver for a remote processor can use the mailbox client provided by
> > the HSP driver and build an IPC protocol on top of this synchronization
> > mechanism.
> >
> > Based on work by Joseph Lo <josephl@nvidia.com>.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> 
> Acked-by: Jassi Brar <jaswinder.singh@linaro.org>

Thanks!

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]