Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1436857 > unrolled thread
| Started by | Joseph Lo <josephl@nvidia.com> |
|---|---|
| First post | 2016-07-05 11:10 +0200 |
| Last post | 2016-07-07 08:30 +0200 |
| Articles | 20 on this page of 30 — 6 participants |
Back to article view | Back to linux.kernel
[PATCH V2 00/10] arm64: tegra: add BPMP support Joseph Lo <josephl@nvidia.com> - 2016-07-05 11:10 +0200
[PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver Joseph Lo <josephl@nvidia.com> - 2016-07-05 11:10 +0200
Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver Alexandre Courbot <gnurou@gmail.com> - 2016-07-06 09:10 +0200
Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver Joseph Lo <josephl@nvidia.com> - 2016-07-06 11:10 +0200
Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver Alexandre Courbot <gnurou@gmail.com> - 2016-07-06 14:30 +0200
Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver Joseph Lo <josephl@nvidia.com> - 2016-07-07 08:40 +0200
Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver Sivaram Nair <sivaramn@nvidia.com> - 2016-07-07 23:40 +0200
Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver Stephen Warren <swarren@wwwdotorg.org> - 2016-07-06 19:00 +0200
Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver Joseph Lo <josephl@nvidia.com> - 2016-07-07 08:50 +0200
Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver Sivaram Nair <sivaramn@nvidia.com> - 2016-07-07 23:20 +0200
[PATCH V2 07/10] arm64: defconfig: Enable Tegra186 SoC Joseph Lo <josephl@nvidia.com> - 2016-07-05 11:10 +0200
[PATCH V2 09/10] arm64: dts: tegra: Add NVIDIA Tegra186 P3310 main board support Joseph Lo <josephl@nvidia.com> - 2016-07-05 11:10 +0200
[PATCH V2 10/10] arm64: dts: tegra: Add NVIDIA P2771 board support Joseph Lo <josephl@nvidia.com> - 2016-07-05 11:10 +0200
[PATCH V2 04/10] firmware: tegra: add IVC library Joseph Lo <josephl@nvidia.com> - 2016-07-05 11:10 +0200
Re: [PATCH V2 04/10] firmware: tegra: add IVC library Alexandre Courbot <gnurou@gmail.com> - 2016-07-07 13:20 +0200
Re: [PATCH V2 04/10] firmware: tegra: add IVC library Paul Gortmaker <paul.gortmaker@windriver.com> - 2016-07-10 01:50 +0200
[PATCH V2 08/10] arm64: dts: tegra: Add Tegra186 support Joseph Lo <josephl@nvidia.com> - 2016-07-05 11:10 +0200
[PATCH V2 06/10] soc/tegra: Add Tegra186 support Joseph Lo <josephl@nvidia.com> - 2016-07-05 11:10 +0200
Re: [PATCH V2 03/10] Documentation: dt-bindings: firmware: tegra: add bindings of the BPMP Alexandre Courbot <gnurou@gmail.com> - 2016-07-06 13:50 +0200
Re: [PATCH V2 03/10] Documentation: dt-bindings: firmware: tegra: add bindings of the BPMP Joseph Lo <josephl@nvidia.com> - 2016-07-07 08:30 +0200
Re: [PATCH V2 05/10] firmware: tegra: add BPMP support Alexandre Courbot <gnurou@gmail.com> - 2016-07-06 14:00 +0200
Re: [PATCH V2 05/10] firmware: tegra: add BPMP support Stephen Warren <swarren@wwwdotorg.org> - 2016-07-06 18:50 +0200
Re: [PATCH V2 05/10] firmware: tegra: add BPMP support Matt Longnecker <mlongnecker@nvidia.com> - 2016-07-06 18:50 +0200
Re: [PATCH V2 05/10] firmware: tegra: add BPMP support Alexandre Courbot <gnurou@gmail.com> - 2016-07-07 04:30 +0200
Re: [PATCH V2 05/10] firmware: tegra: add BPMP support Joseph Lo <josephl@nvidia.com> - 2016-07-07 10:20 +0200
Re: [PATCH V2 05/10] firmware: tegra: add BPMP support Alexandre Courbot <gnurou@gmail.com> - 2016-07-07 12:20 +0200
Re: [PATCH V2 05/10] firmware: tegra: add BPMP support Stephen Warren <swarren@wwwdotorg.org> - 2016-07-07 22:00 +0200
Re: [PATCH V2 05/10] firmware: tegra: add BPMP support Sivaram Nair <sivaramn@nvidia.com> - 2016-07-08 22:30 +0200
Re: [PATCH V2 03/10] Documentation: dt-bindings: firmware: tegra: add bindings of the BPMP Stephen Warren <swarren@wwwdotorg.org> - 2016-07-06 19:10 +0200
Re: [PATCH V2 03/10] Documentation: dt-bindings: firmware: tegra: add bindings of the BPMP Joseph Lo <josephl@nvidia.com> - 2016-07-07 08:30 +0200
Page 1 of 2 [1] 2 Next page →
| From | Joseph Lo <josephl@nvidia.com> |
|---|---|
| Date | 2016-07-05 11:10 +0200 |
| Subject | [PATCH V2 00/10] arm64: tegra: add BPMP support |
| Message-ID | <rRuwF-MJ-15@gated-at.bofh.it> |
Hi,
This series introduce the first announced Boot and Power Management Processor
(BPMP) for the new generation Tegra SoCs, which is designed for boot
process handling and offloading the power management tasks from the CPU.
We also add some very initial and basic support for Tegra186 SoC, which
supports debug console and initrd for initial bring up currently. More drivers
and functions can be supported based on this later.
Thanks,
Joseph
Changes in V2
- revise the HSP mailbox and bpmp DT binding documents
- fix the HSP mailbox driver according to the binding update
- update the dts files to represent the binding update
Joseph Lo (10):
Documentation: dt-bindings: mailbox: tegra: Add binding for HSP
mailbox
mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives)
driver
Documentation: dt-bindings: firmware: tegra: add bindings of the BPMP
firmware: tegra: add IVC library
firmware: tegra: add BPMP support
soc/tegra: Add Tegra186 support
arm64: defconfig: Enable Tegra186 SoC
arm64: dts: tegra: Add Tegra186 support
arm64: dts: tegra: Add NVIDIA Tegra186 P3310 main board support
arm64: dts: tegra: Add NVIDIA P2771 board support
.../bindings/firmware/nvidia,tegra186-bpmp.txt | 77 +
.../bindings/mailbox/nvidia,tegra186-hsp.txt | 51 +
arch/arm64/boot/dts/nvidia/Makefile | 1 +
arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dts | 8 +
arch/arm64/boot/dts/nvidia/tegra186-p3310.dtsi | 34 +
arch/arm64/boot/dts/nvidia/tegra186.dtsi | 77 +
arch/arm64/configs/defconfig | 1 +
drivers/firmware/Kconfig | 1 +
drivers/firmware/Makefile | 1 +
drivers/firmware/tegra/Kconfig | 25 +
drivers/firmware/tegra/Makefile | 2 +
drivers/firmware/tegra/bpmp.c | 713 +++++++++
drivers/firmware/tegra/ivc.c | 659 ++++++++
drivers/mailbox/Kconfig | 9 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/tegra-hsp.c | 418 +++++
drivers/soc/tegra/Kconfig | 14 +
include/dt-bindings/clock/tegra186-clock.h | 940 ++++++++++++
include/dt-bindings/mailbox/tegra186-hsp.h | 23 +
include/dt-bindings/reset/tegra186-reset.h | 217 +++
include/soc/tegra/bpmp.h | 29 +
include/soc/tegra/bpmp_abi.h | 1601 ++++++++++++++++++++
include/soc/tegra/ivc.h | 102 ++
23 files changed, 5005 insertions(+)
create mode 100644 Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.txt
create mode 100644 Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt
create mode 100644 arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dts
create mode 100644 arch/arm64/boot/dts/nvidia/tegra186-p3310.dtsi
create mode 100644 arch/arm64/boot/dts/nvidia/tegra186.dtsi
create mode 100644 drivers/firmware/tegra/Kconfig
create mode 100644 drivers/firmware/tegra/Makefile
create mode 100644 drivers/firmware/tegra/bpmp.c
create mode 100644 drivers/firmware/tegra/ivc.c
create mode 100644 drivers/mailbox/tegra-hsp.c
create mode 100644 include/dt-bindings/clock/tegra186-clock.h
create mode 100644 include/dt-bindings/mailbox/tegra186-hsp.h
create mode 100644 include/dt-bindings/reset/tegra186-reset.h
create mode 100644 include/soc/tegra/bpmp.h
create mode 100644 include/soc/tegra/bpmp_abi.h
create mode 100644 include/soc/tegra/ivc.h
--
2.9.0
[toc] | [next] | [standalone]
| From | Joseph Lo <josephl@nvidia.com> |
|---|---|
| Date | 2016-07-05 11:10 +0200 |
| Subject | [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver |
| Message-ID | <rRuwG-MJ-27@gated-at.bofh.it> |
| In reply to | #1436857 |
The Tegra HSP mailbox driver implements the signaling doorbell-based
interprocessor communication (IPC) for remote processors currently. The
HSP HW modules support some different features for that, which are
shared mailboxes, shared semaphores, arbitrated semaphores, and
doorbells. And there are multiple HSP HW instances on the chip. So the
driver is extendable to support more features for different IPC
requirement.
The driver of remote processor can use it as a mailbox client and deal
with the IPC protocol to synchronize the data communications.
Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
Changes in V2:
- Update the driver to support the binding changes in V2
- it's extendable to support multiple HSP sub-modules on the same HSP HW block
now.
---
drivers/mailbox/Kconfig | 9 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/tegra-hsp.c | 418 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 429 insertions(+)
create mode 100644 drivers/mailbox/tegra-hsp.c
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 5305923752d2..fe584cb54720 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -114,6 +114,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 0be3e742bb7d..26d8f91c7fea 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -25,3 +25,5 @@ obj-$(CONFIG_TI_MESSAGE_MANAGER) += ti-msgmgr.o
obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
obj-$(CONFIG_HI6220_MBOX) += hi6220-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..93c3ef58f29f
--- /dev/null
+++ b/drivers/mailbox/tegra-hsp.c
@@ -0,0 +1,418 @@
+/*
+ * 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 <dt-bindings/mailbox/tegra186-hsp.h>
+
+#define HSP_INT_DIMENSIONING 0x380
+#define HSP_nSM_OFFSET 0
+#define HSP_nSS_OFFSET 4
+#define HSP_nAS_OFFSET 8
+#define HSP_nDB_OFFSET 12
+#define HSP_nSI_OFFSET 16
+#define HSP_nINT_MASK 0xf
+
+#define HSP_DB_REG_TRIGGER 0x0
+#define HSP_DB_REG_ENABLE 0x4
+#define HSP_DB_REG_RAW 0x8
+#define HSP_DB_REG_PENDING 0xc
+
+#define HSP_DB_CCPLEX 1
+#define HSP_DB_BPMP 3
+
+#define MAX_NUM_HSP_CHAN 32
+#define MAX_NUM_HSP_DB 7
+
+#define hsp_db_offset(i, d) \
+ (d->base + ((1 + (d->nr_sm >> 1) + d->nr_ss + d->nr_as) << 16) + \
+ (i) * 0x100)
+
+struct tegra_hsp_db_chan {
+ int master_id;
+ int db_id;
+};
+
+struct tegra_hsp_mbox_chan {
+ int type;
+ union {
+ struct tegra_hsp_db_chan db_chan;
+ };
+};
+
+struct tegra_hsp_mbox {
+ struct mbox_controller *mbox;
+ void __iomem *base;
+ void __iomem *db_base[MAX_NUM_HSP_DB];
+ int db_irq;
+ int nr_sm;
+ int nr_as;
+ int nr_ss;
+ int nr_db;
+ int nr_si;
+ spinlock_t lock;
+};
+
+static inline u32 hsp_readl(void __iomem *base, int reg)
+{
+ return readl(base + reg);
+}
+
+static inline void hsp_writel(void __iomem *base, int reg, u32 val)
+{
+ writel(val, base + reg);
+ readl(base + reg);
+}
+
+static int hsp_db_can_ring(void __iomem *db_base)
+{
+ u32 reg;
+
+ reg = hsp_readl(db_base, HSP_DB_REG_ENABLE);
+
+ return !!(reg & BIT(HSP_DB_MASTER_CCPLEX));
+}
+
+static irqreturn_t hsp_db_irq(int irq, void *p)
+{
+ struct tegra_hsp_mbox *hsp_mbox = p;
+ ulong val;
+ int master_id;
+
+ val = (ulong)hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX],
+ HSP_DB_REG_PENDING);
+ hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_PENDING, val);
+
+ spin_lock(&hsp_mbox->lock);
+ for_each_set_bit(master_id, &val, MAX_NUM_HSP_CHAN) {
+ struct mbox_chan *chan;
+ struct tegra_hsp_mbox_chan *mchan;
+ int i;
+
+ for (i = 0; i < MAX_NUM_HSP_CHAN; i++) {
+ chan = &hsp_mbox->mbox->chans[i];
+
+ if (!chan->con_priv)
+ continue;
+
+ mchan = chan->con_priv;
+ if (mchan->type == HSP_MBOX_TYPE_DB &&
+ mchan->db_chan.master_id == master_id)
+ break;
+ chan = NULL;
+ }
+
+ if (chan)
+ mbox_chan_received_data(chan, NULL);
+ }
+ spin_unlock(&hsp_mbox->lock);
+
+ return IRQ_HANDLED;
+}
+
+static int hsp_db_send_data(struct mbox_chan *chan, void *data)
+{
+ struct tegra_hsp_mbox_chan *mchan = chan->con_priv;
+ struct tegra_hsp_db_chan *db_chan = &mchan->db_chan;
+ struct tegra_hsp_mbox *hsp_mbox = dev_get_drvdata(chan->mbox->dev);
+
+ hsp_writel(hsp_mbox->db_base[db_chan->db_id], HSP_DB_REG_TRIGGER, 1);
+
+ return 0;
+}
+
+static int hsp_db_startup(struct mbox_chan *chan)
+{
+ struct tegra_hsp_mbox_chan *mchan = chan->con_priv;
+ struct tegra_hsp_db_chan *db_chan = &mchan->db_chan;
+ struct tegra_hsp_mbox *hsp_mbox = dev_get_drvdata(chan->mbox->dev);
+ u32 val;
+ unsigned long flag;
+
+ if (db_chan->master_id >= MAX_NUM_HSP_CHAN) {
+ dev_err(chan->mbox->dev, "invalid HSP chan: master ID: %d\n",
+ db_chan->master_id);
+ return -EINVAL;
+ }
+
+ spin_lock_irqsave(&hsp_mbox->lock, flag);
+ val = hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_ENABLE);
+ val |= BIT(db_chan->master_id);
+ hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_ENABLE, val);
+ spin_unlock_irqrestore(&hsp_mbox->lock, flag);
+
+ if (!hsp_db_can_ring(hsp_mbox->db_base[db_chan->db_id]))
+ return -ENODEV;
+
+ return 0;
+}
+
+static void hsp_db_shutdown(struct mbox_chan *chan)
+{
+ struct tegra_hsp_mbox_chan *mchan = chan->con_priv;
+ struct tegra_hsp_db_chan *db_chan = &mchan->db_chan;
+ struct tegra_hsp_mbox *hsp_mbox = dev_get_drvdata(chan->mbox->dev);
+ u32 val;
+ unsigned long flag;
+
+ spin_lock_irqsave(&hsp_mbox->lock, flag);
+ val = hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_ENABLE);
+ val &= ~BIT(db_chan->master_id);
+ hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_ENABLE, val);
+ spin_unlock_irqrestore(&hsp_mbox->lock, flag);
+}
+
+static bool hsp_db_last_tx_done(struct mbox_chan *chan)
+{
+ return true;
+}
+
+static int tegra_hsp_db_init(struct tegra_hsp_mbox *hsp_mbox,
+ struct mbox_chan *mchan, int master_id)
+{
+ struct platform_device *pdev = to_platform_device(hsp_mbox->mbox->dev);
+ struct tegra_hsp_mbox_chan *hsp_mbox_chan;
+ int ret;
+
+ if (!hsp_mbox->db_irq) {
+ int i;
+
+ hsp_mbox->db_irq = platform_get_irq_byname(pdev, "doorbell");
+ ret = devm_request_irq(&pdev->dev, hsp_mbox->db_irq,
+ hsp_db_irq, IRQF_NO_SUSPEND,
+ dev_name(&pdev->dev), hsp_mbox);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < MAX_NUM_HSP_DB; i++)
+ hsp_mbox->db_base[i] = hsp_db_offset(i, hsp_mbox);
+ }
+
+ hsp_mbox_chan = devm_kzalloc(&pdev->dev, sizeof(*hsp_mbox_chan),
+ GFP_KERNEL);
+ if (!hsp_mbox_chan)
+ return -ENOMEM;
+
+ hsp_mbox_chan->type = HSP_MBOX_TYPE_DB;
+ hsp_mbox_chan->db_chan.master_id = master_id;
+ switch (master_id) {
+ case HSP_DB_MASTER_BPMP:
+ hsp_mbox_chan->db_chan.db_id = HSP_DB_BPMP;
+ break;
+ default:
+ hsp_mbox_chan->db_chan.db_id = MAX_NUM_HSP_DB;
+ break;
+ }
+
+ mchan->con_priv = hsp_mbox_chan;
+
+ return 0;
+}
+
+static int hsp_send_data(struct mbox_chan *chan, void *data)
+{
+ struct tegra_hsp_mbox_chan *hsp_mbox_chan = chan->con_priv;
+ int ret = 0;
+
+ switch (hsp_mbox_chan->type) {
+ case HSP_MBOX_TYPE_DB:
+ ret = hsp_db_send_data(chan, data);
+ break;
+ default:
+ break;
+ }
+
+ return ret;
+}
+
+static int hsp_startup(struct mbox_chan *chan)
+{
+ struct tegra_hsp_mbox_chan *hsp_mbox_chan = chan->con_priv;
+ int ret = 0;
+
+ switch (hsp_mbox_chan->type) {
+ case HSP_MBOX_TYPE_DB:
+ ret = hsp_db_startup(chan);
+ break;
+ default:
+ break;
+ }
+
+ return ret;
+}
+
+static void hsp_shutdown(struct mbox_chan *chan)
+{
+ struct tegra_hsp_mbox_chan *hsp_mbox_chan = chan->con_priv;
+
+ switch (hsp_mbox_chan->type) {
+ case HSP_MBOX_TYPE_DB:
+ hsp_db_shutdown(chan);
+ break;
+ default:
+ break;
+ }
+
+ chan->con_priv = NULL;
+}
+
+static bool hsp_last_tx_done(struct mbox_chan *chan)
+{
+ struct tegra_hsp_mbox_chan *hsp_mbox_chan = chan->con_priv;
+ bool ret = true;
+
+ switch (hsp_mbox_chan->type) {
+ case HSP_MBOX_TYPE_DB:
+ ret = hsp_db_last_tx_done(chan);
+ break;
+ default:
+ break;
+ }
+
+ return ret;
+}
+
+static const struct mbox_chan_ops tegra_hsp_ops = {
+ .send_data = hsp_send_data,
+ .startup = hsp_startup,
+ .shutdown = hsp_shutdown,
+ .last_tx_done = hsp_last_tx_done,
+};
+
+static const struct of_device_id tegra_hsp_match[] = {
+ { .compatible = "nvidia,tegra186-hsp" },
+ { }
+};
+
+static struct mbox_chan *
+of_hsp_mbox_xlate(struct mbox_controller *mbox,
+ const struct of_phandle_args *sp)
+{
+ int mbox_id = sp->args[0];
+ int hsp_type = (mbox_id >> 16) & 0xf;
+ int master_id = mbox_id & 0xff;
+ struct tegra_hsp_mbox *hsp_mbox = dev_get_drvdata(mbox->dev);
+ struct mbox_chan *free_chan;
+ int i, ret = 0;
+
+ spin_lock(&hsp_mbox->lock);
+
+ for (i = 0; i < mbox->num_chans; i++) {
+ free_chan = &mbox->chans[i];
+ if (!free_chan->con_priv)
+ break;
+ free_chan = NULL;
+ }
+
+ if (!free_chan) {
+ spin_unlock(&hsp_mbox->lock);
+ return ERR_PTR(-EFAULT);
+ }
+
+ switch (hsp_type) {
+ case HSP_MBOX_TYPE_DB:
+ ret = tegra_hsp_db_init(hsp_mbox, free_chan, master_id);
+ break;
+ default:
+ break;
+ }
+
+ spin_unlock(&hsp_mbox->lock);
+
+ if (ret)
+ free_chan = ERR_PTR(-EFAULT);
+
+ return free_chan;
+}
+
+static int tegra_hsp_probe(struct platform_device *pdev)
+{
+ struct tegra_hsp_mbox *hsp_mbox;
+ struct resource *res;
+ int ret = 0;
+ u32 reg;
+
+ hsp_mbox = devm_kzalloc(&pdev->dev, sizeof(*hsp_mbox), GFP_KERNEL);
+ if (!hsp_mbox)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ hsp_mbox->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(hsp_mbox->base))
+ return PTR_ERR(hsp_mbox->base);
+
+ reg = hsp_readl(hsp_mbox->base, HSP_INT_DIMENSIONING);
+ hsp_mbox->nr_sm = (reg >> HSP_nSM_OFFSET) & HSP_nINT_MASK;
+ hsp_mbox->nr_ss = (reg >> HSP_nSS_OFFSET) & HSP_nINT_MASK;
+ hsp_mbox->nr_as = (reg >> HSP_nAS_OFFSET) & HSP_nINT_MASK;
+ hsp_mbox->nr_db = (reg >> HSP_nDB_OFFSET) & HSP_nINT_MASK;
+ hsp_mbox->nr_si = (reg >> HSP_nSI_OFFSET) & HSP_nINT_MASK;
+
+ hsp_mbox->mbox = devm_kzalloc(&pdev->dev,
+ sizeof(*hsp_mbox->mbox), GFP_KERNEL);
+ if (!hsp_mbox->mbox)
+ return -ENOMEM;
+
+ hsp_mbox->mbox->chans =
+ devm_kcalloc(&pdev->dev, MAX_NUM_HSP_CHAN,
+ sizeof(*hsp_mbox->mbox->chans), GFP_KERNEL);
+ if (!hsp_mbox->mbox->chans)
+ return -ENOMEM;
+
+ hsp_mbox->mbox->of_xlate = of_hsp_mbox_xlate;
+ hsp_mbox->mbox->num_chans = MAX_NUM_HSP_CHAN;
+ hsp_mbox->mbox->dev = &pdev->dev;
+ hsp_mbox->mbox->txdone_irq = false;
+ hsp_mbox->mbox->txdone_poll = false;
+ hsp_mbox->mbox->ops = &tegra_hsp_ops;
+ platform_set_drvdata(pdev, hsp_mbox);
+
+ ret = mbox_controller_register(hsp_mbox->mbox);
+ if (ret) {
+ pr_err("tegra-hsp mbox: fail to register mailbox %d.\n", ret);
+ return ret;
+ }
+
+ spin_lock_init(&hsp_mbox->lock);
+
+ return 0;
+}
+
+static int tegra_hsp_remove(struct platform_device *pdev)
+{
+ struct tegra_hsp_mbox *hsp_mbox = platform_get_drvdata(pdev);
+
+ if (hsp_mbox->mbox)
+ mbox_controller_unregister(hsp_mbox->mbox);
+
+ return 0;
+}
+
+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.9.0
[toc] | [prev] | [next] | [standalone]
| From | Alexandre Courbot <gnurou@gmail.com> |
|---|---|
| Date | 2016-07-06 09:10 +0200 |
| Subject | Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver |
| Message-ID | <rRP86-64B-21@gated-at.bofh.it> |
| In reply to | #1436858 |
On Tue, Jul 5, 2016 at 6:04 PM, Joseph Lo <josephl@nvidia.com> wrote:
> The Tegra HSP mailbox driver implements the signaling doorbell-based
> interprocessor communication (IPC) for remote processors currently. The
> HSP HW modules support some different features for that, which are
> shared mailboxes, shared semaphores, arbitrated semaphores, and
> doorbells. And there are multiple HSP HW instances on the chip. So the
> driver is extendable to support more features for different IPC
> requirement.
>
> The driver of remote processor can use it as a mailbox client and deal
> with the IPC protocol to synchronize the data communications.
>
> Signed-off-by: Joseph Lo <josephl@nvidia.com>
> ---
> Changes in V2:
> - Update the driver to support the binding changes in V2
> - it's extendable to support multiple HSP sub-modules on the same HSP HW block
> now.
> ---
> drivers/mailbox/Kconfig | 9 +
> drivers/mailbox/Makefile | 2 +
> drivers/mailbox/tegra-hsp.c | 418 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 429 insertions(+)
> create mode 100644 drivers/mailbox/tegra-hsp.c
>
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index 5305923752d2..fe584cb54720 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -114,6 +114,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"
Space missing before the opening parenthesis (same in the patch title btw).
> + 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.
Since this option is selected automatically by ARCH_TEGRA_186_SOC, you
should probably drop the last 2 sentences.
> +
> 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 0be3e742bb7d..26d8f91c7fea 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -25,3 +25,5 @@ obj-$(CONFIG_TI_MESSAGE_MANAGER) += ti-msgmgr.o
> obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
>
> obj-$(CONFIG_HI6220_MBOX) += hi6220-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..93c3ef58f29f
> --- /dev/null
> +++ b/drivers/mailbox/tegra-hsp.c
> @@ -0,0 +1,418 @@
> +/*
> + * 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 <dt-bindings/mailbox/tegra186-hsp.h>
> +
> +#define HSP_INT_DIMENSIONING 0x380
> +#define HSP_nSM_OFFSET 0
> +#define HSP_nSS_OFFSET 4
> +#define HSP_nAS_OFFSET 8
> +#define HSP_nDB_OFFSET 12
> +#define HSP_nSI_OFFSET 16
Would be nice to have comments to understand what SM, SS, AS, etc.
stand for (Shared Mailboxes, Shared Semaphores, Arbitrated Semaphores
but you need to look at the patch description to understand that). A
top-of-file comment explaning the necessary concepts to read this code
would do the trick.
> +#define HSP_nINT_MASK 0xf
> +
> +#define HSP_DB_REG_TRIGGER 0x0
> +#define HSP_DB_REG_ENABLE 0x4
> +#define HSP_DB_REG_RAW 0x8
> +#define HSP_DB_REG_PENDING 0xc
> +
> +#define HSP_DB_CCPLEX 1
> +#define HSP_DB_BPMP 3
Maybe turn this into enum and use that type for
tegra_hsp_db_chan::db_id? Also have MAX_NUM_HSP_DB here, since it is
related to these values?
> +
> +#define MAX_NUM_HSP_CHAN 32
> +#define MAX_NUM_HSP_DB 7
> +
> +#define hsp_db_offset(i, d) \
> + (d->base + ((1 + (d->nr_sm >> 1) + d->nr_ss + d->nr_as) << 16) + \
> + (i) * 0x100)
> +
> +struct tegra_hsp_db_chan {
> + int master_id;
> + int db_id;
> +};
> +
> +struct tegra_hsp_mbox_chan {
> + int type;
> + union {
> + struct tegra_hsp_db_chan db_chan;
> + };
> +};
> +
> +struct tegra_hsp_mbox {
> + struct mbox_controller *mbox;
> + void __iomem *base;
> + void __iomem *db_base[MAX_NUM_HSP_DB];
> + int db_irq;
> + int nr_sm;
> + int nr_as;
> + int nr_ss;
> + int nr_db;
> + int nr_si;
> + spinlock_t lock;
> +};
> +
> +static inline u32 hsp_readl(void __iomem *base, int reg)
> +{
> + return readl(base + reg);
> +}
> +
> +static inline void hsp_writel(void __iomem *base, int reg, u32 val)
> +{
> + writel(val, base + reg);
> + readl(base + reg);
> +}
> +
> +static int hsp_db_can_ring(void __iomem *db_base)
> +{
> + u32 reg;
> +
> + reg = hsp_readl(db_base, HSP_DB_REG_ENABLE);
> +
> + return !!(reg & BIT(HSP_DB_MASTER_CCPLEX));
> +}
> +
> +static irqreturn_t hsp_db_irq(int irq, void *p)
> +{
> + struct tegra_hsp_mbox *hsp_mbox = p;
> + ulong val;
> + int master_id;
> +
> + val = (ulong)hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX],
> + HSP_DB_REG_PENDING);
> + hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_PENDING, val);
> +
> + spin_lock(&hsp_mbox->lock);
> + for_each_set_bit(master_id, &val, MAX_NUM_HSP_CHAN) {
> + struct mbox_chan *chan;
> + struct tegra_hsp_mbox_chan *mchan;
> + int i;
> +
> + for (i = 0; i < MAX_NUM_HSP_CHAN; i++) {
I wonder if this could not be optimized. You are doing a double loop
on MAX_NUM_HSP_CHAN to look for an identical master_id. Since it seems
like the same master_id cannot be used twice (considering that the
inner loop only processes the first match), couldn't you just select
the free channel in of_hsp_mbox_xlate() by doing
&mbox->chans[master_id] (and returning an error if it is already
used), then simply getting chan as &hsp_mbox->mbox->chans[master_id]
instead of having the inner loop below? That would remove the need for
the second loop.
If having two channels use the same master_id is a valid scenario,
then all matches on master_id should probably be processed, not just
the first one.
> + chan = &hsp_mbox->mbox->chans[i];
> +
> + if (!chan->con_priv)
> + continue;
> +
> + mchan = chan->con_priv;
> + if (mchan->type == HSP_MBOX_TYPE_DB &&
> + mchan->db_chan.master_id == master_id)
> + break;
> + chan = NULL;
> + }
> +
> + if (chan)
> + mbox_chan_received_data(chan, NULL);
> + }
> + spin_unlock(&hsp_mbox->lock);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int hsp_db_send_data(struct mbox_chan *chan, void *data)
> +{
> + struct tegra_hsp_mbox_chan *mchan = chan->con_priv;
> + struct tegra_hsp_db_chan *db_chan = &mchan->db_chan;
> + struct tegra_hsp_mbox *hsp_mbox = dev_get_drvdata(chan->mbox->dev);
> +
> + hsp_writel(hsp_mbox->db_base[db_chan->db_id], HSP_DB_REG_TRIGGER, 1);
> +
> + return 0;
> +}
> +
> +static int hsp_db_startup(struct mbox_chan *chan)
> +{
> + struct tegra_hsp_mbox_chan *mchan = chan->con_priv;
> + struct tegra_hsp_db_chan *db_chan = &mchan->db_chan;
> + struct tegra_hsp_mbox *hsp_mbox = dev_get_drvdata(chan->mbox->dev);
> + u32 val;
> + unsigned long flag;
> +
> + if (db_chan->master_id >= MAX_NUM_HSP_CHAN) {
> + dev_err(chan->mbox->dev, "invalid HSP chan: master ID: %d\n",
> + db_chan->master_id);
> + return -EINVAL;
> + }
> +
> + spin_lock_irqsave(&hsp_mbox->lock, flag);
> + val = hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_ENABLE);
> + val |= BIT(db_chan->master_id);
> + hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_ENABLE, val);
> + spin_unlock_irqrestore(&hsp_mbox->lock, flag);
> +
> + if (!hsp_db_can_ring(hsp_mbox->db_base[db_chan->db_id]))
> + return -ENODEV;
> +
> + return 0;
> +}
> +
> +static void hsp_db_shutdown(struct mbox_chan *chan)
> +{
> + struct tegra_hsp_mbox_chan *mchan = chan->con_priv;
> + struct tegra_hsp_db_chan *db_chan = &mchan->db_chan;
> + struct tegra_hsp_mbox *hsp_mbox = dev_get_drvdata(chan->mbox->dev);
> + u32 val;
> + unsigned long flag;
> +
> + spin_lock_irqsave(&hsp_mbox->lock, flag);
> + val = hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_ENABLE);
> + val &= ~BIT(db_chan->master_id);
> + hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_ENABLE, val);
> + spin_unlock_irqrestore(&hsp_mbox->lock, flag);
> +}
> +
> +static bool hsp_db_last_tx_done(struct mbox_chan *chan)
> +{
> + return true;
> +}
> +
> +static int tegra_hsp_db_init(struct tegra_hsp_mbox *hsp_mbox,
> + struct mbox_chan *mchan, int master_id)
> +{
> + struct platform_device *pdev = to_platform_device(hsp_mbox->mbox->dev);
> + struct tegra_hsp_mbox_chan *hsp_mbox_chan;
> + int ret;
> +
> + if (!hsp_mbox->db_irq) {
> + int i;
> +
> + hsp_mbox->db_irq = platform_get_irq_byname(pdev, "doorbell");
Getting the IRQ sounds more like a job for probe() - I don't see the
benefit of lazy-doing it?
> + ret = devm_request_irq(&pdev->dev, hsp_mbox->db_irq,
> + hsp_db_irq, IRQF_NO_SUSPEND,
> + dev_name(&pdev->dev), hsp_mbox);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < MAX_NUM_HSP_DB; i++)
> + hsp_mbox->db_base[i] = hsp_db_offset(i, hsp_mbox);
Same here, cannot this be moved into probe()?
> + }
> +
> + hsp_mbox_chan = devm_kzalloc(&pdev->dev, sizeof(*hsp_mbox_chan),
> + GFP_KERNEL);
> + if (!hsp_mbox_chan)
> + return -ENOMEM;
> +
> + hsp_mbox_chan->type = HSP_MBOX_TYPE_DB;
> + hsp_mbox_chan->db_chan.master_id = master_id;
> + switch (master_id) {
> + case HSP_DB_MASTER_BPMP:
> + hsp_mbox_chan->db_chan.db_id = HSP_DB_BPMP;
> + break;
> + default:
> + hsp_mbox_chan->db_chan.db_id = MAX_NUM_HSP_DB;
> + break;
> + }
> +
> + mchan->con_priv = hsp_mbox_chan;
> +
> + return 0;
> +}
> +
> +static int hsp_send_data(struct mbox_chan *chan, void *data)
> +{
> + struct tegra_hsp_mbox_chan *hsp_mbox_chan = chan->con_priv;
> + int ret = 0;
> +
> + switch (hsp_mbox_chan->type) {
> + case HSP_MBOX_TYPE_DB:
> + ret = hsp_db_send_data(chan, data);
> + break;
> + default:
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static int hsp_startup(struct mbox_chan *chan)
> +{
> + struct tegra_hsp_mbox_chan *hsp_mbox_chan = chan->con_priv;
> + int ret = 0;
> +
> + switch (hsp_mbox_chan->type) {
> + case HSP_MBOX_TYPE_DB:
> + ret = hsp_db_startup(chan);
> + break;
> + default:
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static void hsp_shutdown(struct mbox_chan *chan)
> +{
> + struct tegra_hsp_mbox_chan *hsp_mbox_chan = chan->con_priv;
> +
> + switch (hsp_mbox_chan->type) {
> + case HSP_MBOX_TYPE_DB:
> + hsp_db_shutdown(chan);
> + break;
> + default:
> + break;
> + }
> +
> + chan->con_priv = NULL;
> +}
> +
> +static bool hsp_last_tx_done(struct mbox_chan *chan)
> +{
> + struct tegra_hsp_mbox_chan *hsp_mbox_chan = chan->con_priv;
> + bool ret = true;
> +
> + switch (hsp_mbox_chan->type) {
> + case HSP_MBOX_TYPE_DB:
> + ret = hsp_db_last_tx_done(chan);
> + break;
> + default:
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static const struct mbox_chan_ops tegra_hsp_ops = {
> + .send_data = hsp_send_data,
> + .startup = hsp_startup,
> + .shutdown = hsp_shutdown,
> + .last_tx_done = hsp_last_tx_done,
> +};
> +
> +static const struct of_device_id tegra_hsp_match[] = {
> + { .compatible = "nvidia,tegra186-hsp" },
> + { }
> +};
> +
> +static struct mbox_chan *
> +of_hsp_mbox_xlate(struct mbox_controller *mbox,
> + const struct of_phandle_args *sp)
> +{
> + int mbox_id = sp->args[0];
> + int hsp_type = (mbox_id >> 16) & 0xf;
> + int master_id = mbox_id & 0xff;
> + struct tegra_hsp_mbox *hsp_mbox = dev_get_drvdata(mbox->dev);
> + struct mbox_chan *free_chan;
> + int i, ret = 0;
> +
> + spin_lock(&hsp_mbox->lock);
> +
> + for (i = 0; i < mbox->num_chans; i++) {
> + free_chan = &mbox->chans[i];
> + if (!free_chan->con_priv)
> + break;
> + free_chan = NULL;
> + }
> +
> + if (!free_chan) {
> + spin_unlock(&hsp_mbox->lock);
> + return ERR_PTR(-EFAULT);
> + }
> +
> + switch (hsp_type) {
> + case HSP_MBOX_TYPE_DB:
> + ret = tegra_hsp_db_init(hsp_mbox, free_chan, master_id);
Maybe move tegra_hsp_db_init's definition closer to this function,
since it is only used here? Having related functions close to one
another makes it easier to understand the code.
> + break;
> + default:
This looks like an error condition - it should probably be reported,
and maybe even an error returned. If you do it here you probably don't
need to do the same check in hsp_send_data() and following functions.
> + break;
> + }
> +
> + spin_unlock(&hsp_mbox->lock);
> +
> + if (ret)
> + free_chan = ERR_PTR(-EFAULT);
> +
> + return free_chan;
> +}
> +
> +static int tegra_hsp_probe(struct platform_device *pdev)
> +{
> + struct tegra_hsp_mbox *hsp_mbox;
> + struct resource *res;
> + int ret = 0;
> + u32 reg;
> +
> + hsp_mbox = devm_kzalloc(&pdev->dev, sizeof(*hsp_mbox), GFP_KERNEL);
> + if (!hsp_mbox)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + hsp_mbox->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(hsp_mbox->base))
> + return PTR_ERR(hsp_mbox->base);
> +
> + reg = hsp_readl(hsp_mbox->base, HSP_INT_DIMENSIONING);
> + hsp_mbox->nr_sm = (reg >> HSP_nSM_OFFSET) & HSP_nINT_MASK;
> + hsp_mbox->nr_ss = (reg >> HSP_nSS_OFFSET) & HSP_nINT_MASK;
> + hsp_mbox->nr_as = (reg >> HSP_nAS_OFFSET) & HSP_nINT_MASK;
> + hsp_mbox->nr_db = (reg >> HSP_nDB_OFFSET) & HSP_nINT_MASK;
> + hsp_mbox->nr_si = (reg >> HSP_nSI_OFFSET) & HSP_nINT_MASK;
Maybe have a HSP_NR(type, reg) that expands to (reg >> HSP_n ## TYPE
## _OFFSET) & HSP_nINT_MASK) to simplify this?
> +
> + hsp_mbox->mbox = devm_kzalloc(&pdev->dev,
> + sizeof(*hsp_mbox->mbox), GFP_KERNEL);
> + if (!hsp_mbox->mbox)
> + return -ENOMEM;
> +
> + hsp_mbox->mbox->chans =
> + devm_kcalloc(&pdev->dev, MAX_NUM_HSP_CHAN,
> + sizeof(*hsp_mbox->mbox->chans), GFP_KERNEL);
> + if (!hsp_mbox->mbox->chans)
> + return -ENOMEM;
> +
> + hsp_mbox->mbox->of_xlate = of_hsp_mbox_xlate;
> + hsp_mbox->mbox->num_chans = MAX_NUM_HSP_CHAN;
> + hsp_mbox->mbox->dev = &pdev->dev;
> + hsp_mbox->mbox->txdone_irq = false;
> + hsp_mbox->mbox->txdone_poll = false;
> + hsp_mbox->mbox->ops = &tegra_hsp_ops;
> + platform_set_drvdata(pdev, hsp_mbox);
> +
> + ret = mbox_controller_register(hsp_mbox->mbox);
> + if (ret) {
> + pr_err("tegra-hsp mbox: fail to register mailbox %d.\n", ret);
> + return ret;
> + }
> +
> + spin_lock_init(&hsp_mbox->lock);
> +
> + return 0;
> +}
> +
> +static int tegra_hsp_remove(struct platform_device *pdev)
> +{
> + struct tegra_hsp_mbox *hsp_mbox = platform_get_drvdata(pdev);
> +
> + if (hsp_mbox->mbox)
> + mbox_controller_unregister(hsp_mbox->mbox);
> +
> + return 0;
> +}
> +
> +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.9.0
>
[toc] | [prev] | [next] | [standalone]
| From | Joseph Lo <josephl@nvidia.com> |
|---|---|
| Date | 2016-07-06 11:10 +0200 |
| Subject | Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver |
| Message-ID | <rRR0d-7fr-19@gated-at.bofh.it> |
| In reply to | #1437503 |
On 07/06/2016 03:05 PM, Alexandre Courbot wrote:
> On Tue, Jul 5, 2016 at 6:04 PM, Joseph Lo <josephl@nvidia.com> wrote:
>> The Tegra HSP mailbox driver implements the signaling doorbell-based
>> interprocessor communication (IPC) for remote processors currently. The
>> HSP HW modules support some different features for that, which are
>> shared mailboxes, shared semaphores, arbitrated semaphores, and
>> doorbells. And there are multiple HSP HW instances on the chip. So the
>> driver is extendable to support more features for different IPC
>> requirement.
>>
>> The driver of remote processor can use it as a mailbox client and deal
>> with the IPC protocol to synchronize the data communications.
>>
>> Signed-off-by: Joseph Lo <josephl@nvidia.com>
>> ---
>> Changes in V2:
>> - Update the driver to support the binding changes in V2
>> - it's extendable to support multiple HSP sub-modules on the same HSP HW block
>> now.
>> ---
>> drivers/mailbox/Kconfig | 9 +
>> drivers/mailbox/Makefile | 2 +
>> drivers/mailbox/tegra-hsp.c | 418 ++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 429 insertions(+)
>> create mode 100644 drivers/mailbox/tegra-hsp.c
>>
>> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
>> index 5305923752d2..fe584cb54720 100644
>> --- a/drivers/mailbox/Kconfig
>> +++ b/drivers/mailbox/Kconfig
>> @@ -114,6 +114,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"
>
> Space missing before the opening parenthesis (same in the patch title btw).
Okay.
>
>> + 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.
>
> Since this option is selected automatically by ARCH_TEGRA_186_SOC, you
> should probably drop the last 2 sentences.
Okay.
>
>> +
>> 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 0be3e742bb7d..26d8f91c7fea 100644
>> --- a/drivers/mailbox/Makefile
>> +++ b/drivers/mailbox/Makefile
>> @@ -25,3 +25,5 @@ obj-$(CONFIG_TI_MESSAGE_MANAGER) += ti-msgmgr.o
>> obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
>>
>> obj-$(CONFIG_HI6220_MBOX) += hi6220-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..93c3ef58f29f
>> --- /dev/null
>> +++ b/drivers/mailbox/tegra-hsp.c
>> @@ -0,0 +1,418 @@
>> +/*
>> + * 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 <dt-bindings/mailbox/tegra186-hsp.h>
>> +
>> +#define HSP_INT_DIMENSIONING 0x380
>> +#define HSP_nSM_OFFSET 0
>> +#define HSP_nSS_OFFSET 4
>> +#define HSP_nAS_OFFSET 8
>> +#define HSP_nDB_OFFSET 12
>> +#define HSP_nSI_OFFSET 16
>
> Would be nice to have comments to understand what SM, SS, AS, etc.
> stand for (Shared Mailboxes, Shared Semaphores, Arbitrated Semaphores
> but you need to look at the patch description to understand that). A
> top-of-file comment explaning the necessary concepts to read this code
> would do the trick.
Yes, will fix that.
>
>> +#define HSP_nINT_MASK 0xf
>> +
>> +#define HSP_DB_REG_TRIGGER 0x0
>> +#define HSP_DB_REG_ENABLE 0x4
>> +#define HSP_DB_REG_RAW 0x8
>> +#define HSP_DB_REG_PENDING 0xc
>> +
>> +#define HSP_DB_CCPLEX 1
>> +#define HSP_DB_BPMP 3
>
> Maybe turn this into enum and use that type for
> tegra_hsp_db_chan::db_id? Also have MAX_NUM_HSP_DB here, since it is
> related to these values?
Okay.
>
>> +
>> +#define MAX_NUM_HSP_CHAN 32
>> +#define MAX_NUM_HSP_DB 7
>> +
>> +#define hsp_db_offset(i, d) \
>> + (d->base + ((1 + (d->nr_sm >> 1) + d->nr_ss + d->nr_as) << 16) + \
>> + (i) * 0x100)
>> +
>> +struct tegra_hsp_db_chan {
>> + int master_id;
>> + int db_id;
>> +};
>> +
>> +struct tegra_hsp_mbox_chan {
>> + int type;
>> + union {
>> + struct tegra_hsp_db_chan db_chan;
>> + };
>> +};
>> +
>> +struct tegra_hsp_mbox {
>> + struct mbox_controller *mbox;
>> + void __iomem *base;
>> + void __iomem *db_base[MAX_NUM_HSP_DB];
>> + int db_irq;
>> + int nr_sm;
>> + int nr_as;
>> + int nr_ss;
>> + int nr_db;
>> + int nr_si;
>> + spinlock_t lock;
>> +};
>> +
>> +static inline u32 hsp_readl(void __iomem *base, int reg)
>> +{
>> + return readl(base + reg);
>> +}
>> +
>> +static inline void hsp_writel(void __iomem *base, int reg, u32 val)
>> +{
>> + writel(val, base + reg);
>> + readl(base + reg);
>> +}
>> +
>> +static int hsp_db_can_ring(void __iomem *db_base)
>> +{
>> + u32 reg;
>> +
>> + reg = hsp_readl(db_base, HSP_DB_REG_ENABLE);
>> +
>> + return !!(reg & BIT(HSP_DB_MASTER_CCPLEX));
>> +}
>> +
>> +static irqreturn_t hsp_db_irq(int irq, void *p)
>> +{
>> + struct tegra_hsp_mbox *hsp_mbox = p;
>> + ulong val;
>> + int master_id;
>> +
>> + val = (ulong)hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX],
>> + HSP_DB_REG_PENDING);
>> + hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_PENDING, val);
>> +
>> + spin_lock(&hsp_mbox->lock);
>> + for_each_set_bit(master_id, &val, MAX_NUM_HSP_CHAN) {
>> + struct mbox_chan *chan;
>> + struct tegra_hsp_mbox_chan *mchan;
>> + int i;
>> +
>> + for (i = 0; i < MAX_NUM_HSP_CHAN; i++) {
>
> I wonder if this could not be optimized. You are doing a double loop
> on MAX_NUM_HSP_CHAN to look for an identical master_id. Since it seems
> like the same master_id cannot be used twice (considering that the
> inner loop only processes the first match), couldn't you just select
> the free channel in of_hsp_mbox_xlate() by doing
> &mbox->chans[master_id] (and returning an error if it is already
> used), then simply getting chan as &hsp_mbox->mbox->chans[master_id]
> instead of having the inner loop below? That would remove the need for
> the second loop.
That was exactly what I did in the V1, which only supported one HSP
sub-module per HSP HW block. So we can just use the master_id as the
mbox channel ID.
Meanwhile, the V2 is purposed to support multiple HSP sub-modules to be
running on the same HSP HW block. The "ID" between different modules
could be conflict. So I dropped the mechanism that used the master_id as
the mbox channel ID.
Instead, the channel is allocated at the time, when the client is bound
to one of the HSP sub-modules. And we store the "ID" information into
the private mbox channel data, which can help us to figure out which
mbox channel should response to the interrupt.
In the doorbell case, because all the DB clients are shared the same DB
IRQ at the CPU side. So in the ISR, we need to figure out the IRQ
source, which is the master_id that the IRQ came from. This is the outer
loop. The inner loop, we figure out which channel should response to by
checking the type and ID.
And I think it should be pretty quick, because we only check the set bit
from the pending register. And finding the matching channel.
>
> If having two channels use the same master_id is a valid scenario,
> then all matches on master_id should probably be processed, not just
> the first one.
Each DB channel should have different master_id.
>
>> + chan = &hsp_mbox->mbox->chans[i];
>> +
>> + if (!chan->con_priv)
>> + continue;
>> +
>> + mchan = chan->con_priv;
>> + if (mchan->type == HSP_MBOX_TYPE_DB &&
>> + mchan->db_chan.master_id == master_id)
>> + break;
>> + chan = NULL;
>> + }
>> +
>> + if (chan)
>> + mbox_chan_received_data(chan, NULL);
>> + }
>> + spin_unlock(&hsp_mbox->lock);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int hsp_db_send_data(struct mbox_chan *chan, void *data)
>> +{
>> + struct tegra_hsp_mbox_chan *mchan = chan->con_priv;
>> + struct tegra_hsp_db_chan *db_chan = &mchan->db_chan;
>> + struct tegra_hsp_mbox *hsp_mbox = dev_get_drvdata(chan->mbox->dev);
>> +
>> + hsp_writel(hsp_mbox->db_base[db_chan->db_id], HSP_DB_REG_TRIGGER, 1);
>> +
>> + return 0;
>> +}
>> +
>> +static int hsp_db_startup(struct mbox_chan *chan)
>> +{
>> + struct tegra_hsp_mbox_chan *mchan = chan->con_priv;
>> + struct tegra_hsp_db_chan *db_chan = &mchan->db_chan;
>> + struct tegra_hsp_mbox *hsp_mbox = dev_get_drvdata(chan->mbox->dev);
>> + u32 val;
>> + unsigned long flag;
>> +
>> + if (db_chan->master_id >= MAX_NUM_HSP_CHAN) {
>> + dev_err(chan->mbox->dev, "invalid HSP chan: master ID: %d\n",
>> + db_chan->master_id);
>> + return -EINVAL;
>> + }
>> +
>> + spin_lock_irqsave(&hsp_mbox->lock, flag);
>> + val = hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_ENABLE);
>> + val |= BIT(db_chan->master_id);
>> + hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_ENABLE, val);
>> + spin_unlock_irqrestore(&hsp_mbox->lock, flag);
>> +
>> + if (!hsp_db_can_ring(hsp_mbox->db_base[db_chan->db_id]))
>> + return -ENODEV;
>> +
>> + return 0;
>> +}
>> +
>> +static void hsp_db_shutdown(struct mbox_chan *chan)
>> +{
>> + struct tegra_hsp_mbox_chan *mchan = chan->con_priv;
>> + struct tegra_hsp_db_chan *db_chan = &mchan->db_chan;
>> + struct tegra_hsp_mbox *hsp_mbox = dev_get_drvdata(chan->mbox->dev);
>> + u32 val;
>> + unsigned long flag;
>> +
>> + spin_lock_irqsave(&hsp_mbox->lock, flag);
>> + val = hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_ENABLE);
>> + val &= ~BIT(db_chan->master_id);
>> + hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_ENABLE, val);
>> + spin_unlock_irqrestore(&hsp_mbox->lock, flag);
>> +}
>> +
>> +static bool hsp_db_last_tx_done(struct mbox_chan *chan)
>> +{
>> + return true;
>> +}
>> +
>> +static int tegra_hsp_db_init(struct tegra_hsp_mbox *hsp_mbox,
>> + struct mbox_chan *mchan, int master_id)
>> +{
>> + struct platform_device *pdev = to_platform_device(hsp_mbox->mbox->dev);
>> + struct tegra_hsp_mbox_chan *hsp_mbox_chan;
>> + int ret;
>> +
>> + if (!hsp_mbox->db_irq) {
>> + int i;
>> +
>> + hsp_mbox->db_irq = platform_get_irq_byname(pdev, "doorbell");
>
> Getting the IRQ sounds more like a job for probe() - I don't see the
> benefit of lazy-doing it?
We only need the IRQ when the client is requesting the DB service. For
other HSP sub-modules, they are using different IRQ. So I didn't do that
at probe time.
>
>> + ret = devm_request_irq(&pdev->dev, hsp_mbox->db_irq,
>> + hsp_db_irq, IRQF_NO_SUSPEND,
>> + dev_name(&pdev->dev), hsp_mbox);
>> + if (ret)
>> + return ret;
>> +
>> + for (i = 0; i < MAX_NUM_HSP_DB; i++)
>> + hsp_mbox->db_base[i] = hsp_db_offset(i, hsp_mbox);
>
> Same here, cannot this be moved into probe()?
Same as above, only needed when the client requests it.
>
>> + }
>> +
>> + hsp_mbox_chan = devm_kzalloc(&pdev->dev, sizeof(*hsp_mbox_chan),
>> + GFP_KERNEL);
>> + if (!hsp_mbox_chan)
>> + return -ENOMEM;
>> +
>> + hsp_mbox_chan->type = HSP_MBOX_TYPE_DB;
>> + hsp_mbox_chan->db_chan.master_id = master_id;
>> + switch (master_id) {
>> + case HSP_DB_MASTER_BPMP:
>> + hsp_mbox_chan->db_chan.db_id = HSP_DB_BPMP;
>> + break;
>> + default:
>> + hsp_mbox_chan->db_chan.db_id = MAX_NUM_HSP_DB;
>> + break;
>> + }
>> +
>> + mchan->con_priv = hsp_mbox_chan;
>> +
>> + return 0;
>> +}
>> +
>> +static int hsp_send_data(struct mbox_chan *chan, void *data)
>> +{
>> + struct tegra_hsp_mbox_chan *hsp_mbox_chan = chan->con_priv;
>> + int ret = 0;
>> +
>> + switch (hsp_mbox_chan->type) {
>> + case HSP_MBOX_TYPE_DB:
>> + ret = hsp_db_send_data(chan, data);
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int hsp_startup(struct mbox_chan *chan)
>> +{
>> + struct tegra_hsp_mbox_chan *hsp_mbox_chan = chan->con_priv;
>> + int ret = 0;
>> +
>> + switch (hsp_mbox_chan->type) {
>> + case HSP_MBOX_TYPE_DB:
>> + ret = hsp_db_startup(chan);
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static void hsp_shutdown(struct mbox_chan *chan)
>> +{
>> + struct tegra_hsp_mbox_chan *hsp_mbox_chan = chan->con_priv;
>> +
>> + switch (hsp_mbox_chan->type) {
>> + case HSP_MBOX_TYPE_DB:
>> + hsp_db_shutdown(chan);
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> + chan->con_priv = NULL;
>> +}
>> +
>> +static bool hsp_last_tx_done(struct mbox_chan *chan)
>> +{
>> + struct tegra_hsp_mbox_chan *hsp_mbox_chan = chan->con_priv;
>> + bool ret = true;
>> +
>> + switch (hsp_mbox_chan->type) {
>> + case HSP_MBOX_TYPE_DB:
>> + ret = hsp_db_last_tx_done(chan);
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static const struct mbox_chan_ops tegra_hsp_ops = {
>> + .send_data = hsp_send_data,
>> + .startup = hsp_startup,
>> + .shutdown = hsp_shutdown,
>> + .last_tx_done = hsp_last_tx_done,
>> +};
>> +
>> +static const struct of_device_id tegra_hsp_match[] = {
>> + { .compatible = "nvidia,tegra186-hsp" },
>> + { }
>> +};
>> +
>> +static struct mbox_chan *
>> +of_hsp_mbox_xlate(struct mbox_controller *mbox,
>> + const struct of_phandle_args *sp)
>> +{
>> + int mbox_id = sp->args[0];
>> + int hsp_type = (mbox_id >> 16) & 0xf;
>> + int master_id = mbox_id & 0xff;
>> + struct tegra_hsp_mbox *hsp_mbox = dev_get_drvdata(mbox->dev);
>> + struct mbox_chan *free_chan;
>> + int i, ret = 0;
>> +
>> + spin_lock(&hsp_mbox->lock);
>> +
>> + for (i = 0; i < mbox->num_chans; i++) {
>> + free_chan = &mbox->chans[i];
>> + if (!free_chan->con_priv)
>> + break;
>> + free_chan = NULL;
>> + }
>> +
>> + if (!free_chan) {
>> + spin_unlock(&hsp_mbox->lock);
>> + return ERR_PTR(-EFAULT);
>> + }
>> +
>> + switch (hsp_type) {
>> + case HSP_MBOX_TYPE_DB:
>> + ret = tegra_hsp_db_init(hsp_mbox, free_chan, master_id);
>
> Maybe move tegra_hsp_db_init's definition closer to this function,
> since it is only used here? Having related functions close to one
> another makes it easier to understand the code.
Okay.
>
>> + break;
>> + default:
>
> This looks like an error condition - it should probably be reported,
> and maybe even an error returned. If you do it here you probably don't
> need to do the same check in hsp_send_data() and following functions.
Yes, will fix.
>
>> + break;
>> + }
>> +
>> + spin_unlock(&hsp_mbox->lock);
>> +
>> + if (ret)
>> + free_chan = ERR_PTR(-EFAULT);
>> +
>> + return free_chan;
>> +}
>> +
>> +static int tegra_hsp_probe(struct platform_device *pdev)
>> +{
>> + struct tegra_hsp_mbox *hsp_mbox;
>> + struct resource *res;
>> + int ret = 0;
>> + u32 reg;
>> +
>> + hsp_mbox = devm_kzalloc(&pdev->dev, sizeof(*hsp_mbox), GFP_KERNEL);
>> + if (!hsp_mbox)
>> + return -ENOMEM;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + hsp_mbox->base = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(hsp_mbox->base))
>> + return PTR_ERR(hsp_mbox->base);
>> +
>> + reg = hsp_readl(hsp_mbox->base, HSP_INT_DIMENSIONING);
>> + hsp_mbox->nr_sm = (reg >> HSP_nSM_OFFSET) & HSP_nINT_MASK;
>> + hsp_mbox->nr_ss = (reg >> HSP_nSS_OFFSET) & HSP_nINT_MASK;
>> + hsp_mbox->nr_as = (reg >> HSP_nAS_OFFSET) & HSP_nINT_MASK;
>> + hsp_mbox->nr_db = (reg >> HSP_nDB_OFFSET) & HSP_nINT_MASK;
>> + hsp_mbox->nr_si = (reg >> HSP_nSI_OFFSET) & HSP_nINT_MASK;
>
> Maybe have a HSP_NR(type, reg) that expands to (reg >> HSP_n ## TYPE
> ## _OFFSET) & HSP_nINT_MASK) to simplify this?
Good suggestion, will fix.
Thanks,
-Joseph
>
>> +
>> + hsp_mbox->mbox = devm_kzalloc(&pdev->dev,
>> + sizeof(*hsp_mbox->mbox), GFP_KERNEL);
>> + if (!hsp_mbox->mbox)
>> + return -ENOMEM;
>> +
>> + hsp_mbox->mbox->chans =
>> + devm_kcalloc(&pdev->dev, MAX_NUM_HSP_CHAN,
>> + sizeof(*hsp_mbox->mbox->chans), GFP_KERNEL);
>> + if (!hsp_mbox->mbox->chans)
>> + return -ENOMEM;
>> +
>> + hsp_mbox->mbox->of_xlate = of_hsp_mbox_xlate;
>> + hsp_mbox->mbox->num_chans = MAX_NUM_HSP_CHAN;
>> + hsp_mbox->mbox->dev = &pdev->dev;
>> + hsp_mbox->mbox->txdone_irq = false;
>> + hsp_mbox->mbox->txdone_poll = false;
>> + hsp_mbox->mbox->ops = &tegra_hsp_ops;
>> + platform_set_drvdata(pdev, hsp_mbox);
>> +
>> + ret = mbox_controller_register(hsp_mbox->mbox);
>> + if (ret) {
>> + pr_err("tegra-hsp mbox: fail to register mailbox %d.\n", ret);
>> + return ret;
>> + }
>> +
>> + spin_lock_init(&hsp_mbox->lock);
>> +
>> + return 0;
>> +}
>> +
>> +static int tegra_hsp_remove(struct platform_device *pdev)
>> +{
>> + struct tegra_hsp_mbox *hsp_mbox = platform_get_drvdata(pdev);
>> +
>> + if (hsp_mbox->mbox)
>> + mbox_controller_unregister(hsp_mbox->mbox);
>> +
>> + return 0;
>> +}
>> +
>> +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.9.0
>>
[toc] | [prev] | [next] | [standalone]
| From | Alexandre Courbot <gnurou@gmail.com> |
|---|---|
| Date | 2016-07-06 14:30 +0200 |
| Subject | Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver |
| Message-ID | <rRU7M-I3-1@gated-at.bofh.it> |
| In reply to | #1437577 |
On Wed, Jul 6, 2016 at 6:06 PM, Joseph Lo <josephl@nvidia.com> wrote:
> On 07/06/2016 03:05 PM, Alexandre Courbot wrote:
>>
>> On Tue, Jul 5, 2016 at 6:04 PM, Joseph Lo <josephl@nvidia.com> wrote:
>>>
>>> The Tegra HSP mailbox driver implements the signaling doorbell-based
>>> interprocessor communication (IPC) for remote processors currently. The
>>> HSP HW modules support some different features for that, which are
>>> shared mailboxes, shared semaphores, arbitrated semaphores, and
>>> doorbells. And there are multiple HSP HW instances on the chip. So the
>>> driver is extendable to support more features for different IPC
>>> requirement.
>>>
>>> The driver of remote processor can use it as a mailbox client and deal
>>> with the IPC protocol to synchronize the data communications.
>>>
>>> Signed-off-by: Joseph Lo <josephl@nvidia.com>
>>> ---
>>> Changes in V2:
>>> - Update the driver to support the binding changes in V2
>>> - it's extendable to support multiple HSP sub-modules on the same HSP HW
>>> block
>>> now.
>>> ---
>>> drivers/mailbox/Kconfig | 9 +
>>> drivers/mailbox/Makefile | 2 +
>>> drivers/mailbox/tegra-hsp.c | 418
>>> ++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 429 insertions(+)
>>> create mode 100644 drivers/mailbox/tegra-hsp.c
>>>
>>> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
>>> index 5305923752d2..fe584cb54720 100644
>>> --- a/drivers/mailbox/Kconfig
>>> +++ b/drivers/mailbox/Kconfig
>>> @@ -114,6 +114,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"
>>
>>
>> Space missing before the opening parenthesis (same in the patch title
>> btw).
>
> Okay.
>>
>>
>>> + 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.
>>
>>
>> Since this option is selected automatically by ARCH_TEGRA_186_SOC, you
>> should probably drop the last 2 sentences.
>
> Okay.
>
>>
>>> +
>>> 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 0be3e742bb7d..26d8f91c7fea 100644
>>> --- a/drivers/mailbox/Makefile
>>> +++ b/drivers/mailbox/Makefile
>>> @@ -25,3 +25,5 @@ obj-$(CONFIG_TI_MESSAGE_MANAGER) += ti-msgmgr.o
>>> obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
>>>
>>> obj-$(CONFIG_HI6220_MBOX) += hi6220-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..93c3ef58f29f
>>> --- /dev/null
>>> +++ b/drivers/mailbox/tegra-hsp.c
>>> @@ -0,0 +1,418 @@
>>> +/*
>>> + * 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 <dt-bindings/mailbox/tegra186-hsp.h>
>>> +
>>> +#define HSP_INT_DIMENSIONING 0x380
>>> +#define HSP_nSM_OFFSET 0
>>> +#define HSP_nSS_OFFSET 4
>>> +#define HSP_nAS_OFFSET 8
>>> +#define HSP_nDB_OFFSET 12
>>> +#define HSP_nSI_OFFSET 16
>>
>>
>> Would be nice to have comments to understand what SM, SS, AS, etc.
>> stand for (Shared Mailboxes, Shared Semaphores, Arbitrated Semaphores
>> but you need to look at the patch description to understand that). A
>> top-of-file comment explaning the necessary concepts to read this code
>> would do the trick.
>
> Yes, will fix that.
>>
>>
>>> +#define HSP_nINT_MASK 0xf
>>> +
>>> +#define HSP_DB_REG_TRIGGER 0x0
>>> +#define HSP_DB_REG_ENABLE 0x4
>>> +#define HSP_DB_REG_RAW 0x8
>>> +#define HSP_DB_REG_PENDING 0xc
>>> +
>>> +#define HSP_DB_CCPLEX 1
>>> +#define HSP_DB_BPMP 3
>>
>>
>> Maybe turn this into enum and use that type for
>> tegra_hsp_db_chan::db_id? Also have MAX_NUM_HSP_DB here, since it is
>> related to these values?
>
> Okay.
>
>>
>>> +
>>> +#define MAX_NUM_HSP_CHAN 32
>>> +#define MAX_NUM_HSP_DB 7
>>> +
>>> +#define hsp_db_offset(i, d) \
>>> + (d->base + ((1 + (d->nr_sm >> 1) + d->nr_ss + d->nr_as) << 16) +
>>> \
>>> + (i) * 0x100)
>>> +
>>> +struct tegra_hsp_db_chan {
>>> + int master_id;
>>> + int db_id;
>>> +};
>>> +
>>> +struct tegra_hsp_mbox_chan {
>>> + int type;
>>> + union {
>>> + struct tegra_hsp_db_chan db_chan;
>>> + };
>>> +};
>>> +
>>> +struct tegra_hsp_mbox {
>>> + struct mbox_controller *mbox;
>>> + void __iomem *base;
>>> + void __iomem *db_base[MAX_NUM_HSP_DB];
>>> + int db_irq;
>>> + int nr_sm;
>>> + int nr_as;
>>> + int nr_ss;
>>> + int nr_db;
>>> + int nr_si;
>>> + spinlock_t lock;
>>> +};
>>> +
>>> +static inline u32 hsp_readl(void __iomem *base, int reg)
>>> +{
>>> + return readl(base + reg);
>>> +}
>>> +
>>> +static inline void hsp_writel(void __iomem *base, int reg, u32 val)
>>> +{
>>> + writel(val, base + reg);
>>> + readl(base + reg);
>>> +}
>>> +
>>> +static int hsp_db_can_ring(void __iomem *db_base)
>>> +{
>>> + u32 reg;
>>> +
>>> + reg = hsp_readl(db_base, HSP_DB_REG_ENABLE);
>>> +
>>> + return !!(reg & BIT(HSP_DB_MASTER_CCPLEX));
>>> +}
>>> +
>>> +static irqreturn_t hsp_db_irq(int irq, void *p)
>>> +{
>>> + struct tegra_hsp_mbox *hsp_mbox = p;
>>> + ulong val;
>>> + int master_id;
>>> +
>>> + val = (ulong)hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX],
>>> + HSP_DB_REG_PENDING);
>>> + hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_PENDING,
>>> val);
>>> +
>>> + spin_lock(&hsp_mbox->lock);
>>> + for_each_set_bit(master_id, &val, MAX_NUM_HSP_CHAN) {
>>> + struct mbox_chan *chan;
>>> + struct tegra_hsp_mbox_chan *mchan;
>>> + int i;
>>> +
>>> + for (i = 0; i < MAX_NUM_HSP_CHAN; i++) {
>>
>>
>> I wonder if this could not be optimized. You are doing a double loop
>> on MAX_NUM_HSP_CHAN to look for an identical master_id. Since it seems
>> like the same master_id cannot be used twice (considering that the
>> inner loop only processes the first match), couldn't you just select
>> the free channel in of_hsp_mbox_xlate() by doing
>> &mbox->chans[master_id] (and returning an error if it is already
>> used), then simply getting chan as &hsp_mbox->mbox->chans[master_id]
>> instead of having the inner loop below? That would remove the need for
>> the second loop.
>
>
> That was exactly what I did in the V1, which only supported one HSP
> sub-module per HSP HW block. So we can just use the master_id as the mbox
> channel ID.
>
> Meanwhile, the V2 is purposed to support multiple HSP sub-modules to be
> running on the same HSP HW block. The "ID" between different modules could
> be conflict. So I dropped the mechanism that used the master_id as the mbox
> channel ID.
>
> Instead, the channel is allocated at the time, when the client is bound to
> one of the HSP sub-modules. And we store the "ID" information into the
> private mbox channel data, which can help us to figure out which mbox
> channel should response to the interrupt.
>
> In the doorbell case, because all the DB clients are shared the same DB IRQ
> at the CPU side. So in the ISR, we need to figure out the IRQ source, which
> is the master_id that the IRQ came from. This is the outer loop. The inner
> loop, we figure out which channel should response to by checking the type
> and ID.
>
> And I think it should be pretty quick, because we only check the set bit
> from the pending register. And finding the matching channel.
Yeah, I am not worried about the CPU time (although in interrupt
context, we always should), but rather about whether the code could be
simplified.
Ah, I think I get it. You want to be able to receive interrupts from
the same master, but not necessarily for the doorbell function.
Because of this you cannot use master_id as the index for the channel.
Am I understanding correctly?
>
>>
>> If having two channels use the same master_id is a valid scenario,
>> then all matches on master_id should probably be processed, not just
>> the first one.
>
> Each DB channel should have different master_id.
>
>
>>
>>> + chan = &hsp_mbox->mbox->chans[i];
>>> +
>>> + if (!chan->con_priv)
>>> + continue;
>>> +
>>> + mchan = chan->con_priv;
>>> + if (mchan->type == HSP_MBOX_TYPE_DB &&
>>> + mchan->db_chan.master_id == master_id)
>>> + break;
>>> + chan = NULL;
>>> + }
>>> +
>>> + if (chan)
>>> + mbox_chan_received_data(chan, NULL);
>>> + }
>>> + spin_unlock(&hsp_mbox->lock);
>>> +
>>> + return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int hsp_db_send_data(struct mbox_chan *chan, void *data)
>>> +{
>>> + struct tegra_hsp_mbox_chan *mchan = chan->con_priv;
>>> + struct tegra_hsp_db_chan *db_chan = &mchan->db_chan;
>>> + struct tegra_hsp_mbox *hsp_mbox =
>>> dev_get_drvdata(chan->mbox->dev);
>>> +
>>> + hsp_writel(hsp_mbox->db_base[db_chan->db_id], HSP_DB_REG_TRIGGER,
>>> 1);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int hsp_db_startup(struct mbox_chan *chan)
>>> +{
>>> + struct tegra_hsp_mbox_chan *mchan = chan->con_priv;
>>> + struct tegra_hsp_db_chan *db_chan = &mchan->db_chan;
>>> + struct tegra_hsp_mbox *hsp_mbox =
>>> dev_get_drvdata(chan->mbox->dev);
>>> + u32 val;
>>> + unsigned long flag;
>>> +
>>> + if (db_chan->master_id >= MAX_NUM_HSP_CHAN) {
>>> + dev_err(chan->mbox->dev, "invalid HSP chan: master ID:
>>> %d\n",
>>> + db_chan->master_id);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + spin_lock_irqsave(&hsp_mbox->lock, flag);
>>> + val = hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX],
>>> HSP_DB_REG_ENABLE);
>>> + val |= BIT(db_chan->master_id);
>>> + hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_ENABLE,
>>> val);
>>> + spin_unlock_irqrestore(&hsp_mbox->lock, flag);
>>> +
>>> + if (!hsp_db_can_ring(hsp_mbox->db_base[db_chan->db_id]))
>>> + return -ENODEV;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void hsp_db_shutdown(struct mbox_chan *chan)
>>> +{
>>> + struct tegra_hsp_mbox_chan *mchan = chan->con_priv;
>>> + struct tegra_hsp_db_chan *db_chan = &mchan->db_chan;
>>> + struct tegra_hsp_mbox *hsp_mbox =
>>> dev_get_drvdata(chan->mbox->dev);
>>> + u32 val;
>>> + unsigned long flag;
>>> +
>>> + spin_lock_irqsave(&hsp_mbox->lock, flag);
>>> + val = hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX],
>>> HSP_DB_REG_ENABLE);
>>> + val &= ~BIT(db_chan->master_id);
>>> + hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_ENABLE,
>>> val);
>>> + spin_unlock_irqrestore(&hsp_mbox->lock, flag);
>>> +}
>>> +
>>> +static bool hsp_db_last_tx_done(struct mbox_chan *chan)
>>> +{
>>> + return true;
>>> +}
>>> +
>>> +static int tegra_hsp_db_init(struct tegra_hsp_mbox *hsp_mbox,
>>> + struct mbox_chan *mchan, int master_id)
>>> +{
>>> + struct platform_device *pdev =
>>> to_platform_device(hsp_mbox->mbox->dev);
>>> + struct tegra_hsp_mbox_chan *hsp_mbox_chan;
>>> + int ret;
>>> +
>>> + if (!hsp_mbox->db_irq) {
>>> + int i;
>>> +
>>> + hsp_mbox->db_irq = platform_get_irq_byname(pdev,
>>> "doorbell");
>>
>>
>> Getting the IRQ sounds more like a job for probe() - I don't see the
>> benefit of lazy-doing it?
>
>
> We only need the IRQ when the client is requesting the DB service. For other
> HSP sub-modules, they are using different IRQ. So I didn't do that at probe
> time.
Ok, but probe() is where resources should be acquired... and at the
very least DT properties be looked up. In this case there is no hard
requirement for doing it elsewhere.
Is this interrupt absolutely required? Or can we tolerate to not use
the doorbell service? In the first case, the driver should fail during
probe(), not sometime later. In the second case, you should still get
all the interrupts in probe(), then disable them if they are not
needed, and check in this function whether db_irq is a valid interrupt
number to decide whether or not we can use doorbell.
>
>>
>>> + ret = devm_request_irq(&pdev->dev, hsp_mbox->db_irq,
>>> + hsp_db_irq, IRQF_NO_SUSPEND,
>>> + dev_name(&pdev->dev), hsp_mbox);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + for (i = 0; i < MAX_NUM_HSP_DB; i++)
>>> + hsp_mbox->db_base[i] = hsp_db_offset(i,
>>> hsp_mbox);
>>
>>
>> Same here, cannot this be moved into probe()?
>
> Same as above, only needed when the client requests it.
But you don't waste any resources by doing it preemptively in probe().
So let's keep related code in the same place.
[toc] | [prev] | [next] | [standalone]
| From | Joseph Lo <josephl@nvidia.com> |
|---|---|
| Date | 2016-07-07 08:40 +0200 |
| Subject | Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver |
| Message-ID | <rSb8B-3fF-9@gated-at.bofh.it> |
| In reply to | #1437672 |
On 07/06/2016 08:23 PM, Alexandre Courbot wrote:
> On Wed, Jul 6, 2016 at 6:06 PM, Joseph Lo <josephl@nvidia.com> wrote:
>> On 07/06/2016 03:05 PM, Alexandre Courbot wrote:
>>>
>>> On Tue, Jul 5, 2016 at 6:04 PM, Joseph Lo <josephl@nvidia.com> wrote:
>>>>
>>>> The Tegra HSP mailbox driver implements the signaling doorbell-based
>>>> interprocessor communication (IPC) for remote processors currently. The
>>>> HSP HW modules support some different features for that, which are
>>>> shared mailboxes, shared semaphores, arbitrated semaphores, and
>>>> doorbells. And there are multiple HSP HW instances on the chip. So the
>>>> driver is extendable to support more features for different IPC
>>>> requirement.
>>>>
>>>> The driver of remote processor can use it as a mailbox client and deal
>>>> with the IPC protocol to synchronize the data communications.
>>>>
>>>> Signed-off-by: Joseph Lo <josephl@nvidia.com>
>>>> ---
>>>> Changes in V2:
>>>> - Update the driver to support the binding changes in V2
>>>> - it's extendable to support multiple HSP sub-modules on the same HSP HW
>>>> block
>>>> now.
>>>> ---
>>>> drivers/mailbox/Kconfig | 9 +
>>>> drivers/mailbox/Makefile | 2 +
>>>> drivers/mailbox/tegra-hsp.c | 418
>>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 429 insertions(+)
>>>> create mode 100644 drivers/mailbox/tegra-hsp.c
>>>>
>>>> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
>>>> index 5305923752d2..fe584cb54720 100644
>>>> --- a/drivers/mailbox/Kconfig
>>>> +++ b/drivers/mailbox/Kconfig
>>>> @@ -114,6 +114,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"
>>>
>>>
>>> Space missing before the opening parenthesis (same in the patch title
>>> btw).
>>
>> Okay.
>>>
>>>
>>>> + 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.
>>>
>>>
>>> Since this option is selected automatically by ARCH_TEGRA_186_SOC, you
>>> should probably drop the last 2 sentences.
>>
>> Okay.
>>
>>>
>>>> +
>>>> 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 0be3e742bb7d..26d8f91c7fea 100644
>>>> --- a/drivers/mailbox/Makefile
>>>> +++ b/drivers/mailbox/Makefile
>>>> @@ -25,3 +25,5 @@ obj-$(CONFIG_TI_MESSAGE_MANAGER) += ti-msgmgr.o
>>>> obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
>>>>
>>>> obj-$(CONFIG_HI6220_MBOX) += hi6220-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..93c3ef58f29f
>>>> --- /dev/null
>>>> +++ b/drivers/mailbox/tegra-hsp.c
>>>> @@ -0,0 +1,418 @@
>>>> +/*
>>>> + * 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 <dt-bindings/mailbox/tegra186-hsp.h>
>>>> +
>>>> +#define HSP_INT_DIMENSIONING 0x380
>>>> +#define HSP_nSM_OFFSET 0
>>>> +#define HSP_nSS_OFFSET 4
>>>> +#define HSP_nAS_OFFSET 8
>>>> +#define HSP_nDB_OFFSET 12
>>>> +#define HSP_nSI_OFFSET 16
>>>
>>>
>>> Would be nice to have comments to understand what SM, SS, AS, etc.
>>> stand for (Shared Mailboxes, Shared Semaphores, Arbitrated Semaphores
>>> but you need to look at the patch description to understand that). A
>>> top-of-file comment explaning the necessary concepts to read this code
>>> would do the trick.
>>
>> Yes, will fix that.
>>>
>>>
>>>> +#define HSP_nINT_MASK 0xf
>>>> +
>>>> +#define HSP_DB_REG_TRIGGER 0x0
>>>> +#define HSP_DB_REG_ENABLE 0x4
>>>> +#define HSP_DB_REG_RAW 0x8
>>>> +#define HSP_DB_REG_PENDING 0xc
>>>> +
>>>> +#define HSP_DB_CCPLEX 1
>>>> +#define HSP_DB_BPMP 3
>>>
>>>
>>> Maybe turn this into enum and use that type for
>>> tegra_hsp_db_chan::db_id? Also have MAX_NUM_HSP_DB here, since it is
>>> related to these values?
>>
>> Okay.
>>
>>>
>>>> +
>>>> +#define MAX_NUM_HSP_CHAN 32
>>>> +#define MAX_NUM_HSP_DB 7
>>>> +
>>>> +#define hsp_db_offset(i, d) \
>>>> + (d->base + ((1 + (d->nr_sm >> 1) + d->nr_ss + d->nr_as) << 16) +
>>>> \
>>>> + (i) * 0x100)
>>>> +
>>>> +struct tegra_hsp_db_chan {
>>>> + int master_id;
>>>> + int db_id;
>>>> +};
>>>> +
>>>> +struct tegra_hsp_mbox_chan {
>>>> + int type;
>>>> + union {
>>>> + struct tegra_hsp_db_chan db_chan;
>>>> + };
>>>> +};
>>>> +
>>>> +struct tegra_hsp_mbox {
>>>> + struct mbox_controller *mbox;
>>>> + void __iomem *base;
>>>> + void __iomem *db_base[MAX_NUM_HSP_DB];
>>>> + int db_irq;
>>>> + int nr_sm;
>>>> + int nr_as;
>>>> + int nr_ss;
>>>> + int nr_db;
>>>> + int nr_si;
>>>> + spinlock_t lock;
>>>> +};
>>>> +
>>>> +static inline u32 hsp_readl(void __iomem *base, int reg)
>>>> +{
>>>> + return readl(base + reg);
>>>> +}
>>>> +
>>>> +static inline void hsp_writel(void __iomem *base, int reg, u32 val)
>>>> +{
>>>> + writel(val, base + reg);
>>>> + readl(base + reg);
>>>> +}
>>>> +
>>>> +static int hsp_db_can_ring(void __iomem *db_base)
>>>> +{
>>>> + u32 reg;
>>>> +
>>>> + reg = hsp_readl(db_base, HSP_DB_REG_ENABLE);
>>>> +
>>>> + return !!(reg & BIT(HSP_DB_MASTER_CCPLEX));
>>>> +}
>>>> +
>>>> +static irqreturn_t hsp_db_irq(int irq, void *p)
>>>> +{
>>>> + struct tegra_hsp_mbox *hsp_mbox = p;
>>>> + ulong val;
>>>> + int master_id;
>>>> +
>>>> + val = (ulong)hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX],
>>>> + HSP_DB_REG_PENDING);
>>>> + hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_PENDING,
>>>> val);
>>>> +
>>>> + spin_lock(&hsp_mbox->lock);
>>>> + for_each_set_bit(master_id, &val, MAX_NUM_HSP_CHAN) {
>>>> + struct mbox_chan *chan;
>>>> + struct tegra_hsp_mbox_chan *mchan;
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < MAX_NUM_HSP_CHAN; i++) {
>>>
>>>
>>> I wonder if this could not be optimized. You are doing a double loop
>>> on MAX_NUM_HSP_CHAN to look for an identical master_id. Since it seems
>>> like the same master_id cannot be used twice (considering that the
>>> inner loop only processes the first match), couldn't you just select
>>> the free channel in of_hsp_mbox_xlate() by doing
>>> &mbox->chans[master_id] (and returning an error if it is already
>>> used), then simply getting chan as &hsp_mbox->mbox->chans[master_id]
>>> instead of having the inner loop below? That would remove the need for
>>> the second loop.
>>
>>
>> That was exactly what I did in the V1, which only supported one HSP
>> sub-module per HSP HW block. So we can just use the master_id as the mbox
>> channel ID.
>>
>> Meanwhile, the V2 is purposed to support multiple HSP sub-modules to be
>> running on the same HSP HW block. The "ID" between different modules could
>> be conflict. So I dropped the mechanism that used the master_id as the mbox
>> channel ID.
>>
>> Instead, the channel is allocated at the time, when the client is bound to
>> one of the HSP sub-modules. And we store the "ID" information into the
>> private mbox channel data, which can help us to figure out which mbox
>> channel should response to the interrupt.
>>
>> In the doorbell case, because all the DB clients are shared the same DB IRQ
>> at the CPU side. So in the ISR, we need to figure out the IRQ source, which
>> is the master_id that the IRQ came from. This is the outer loop. The inner
>> loop, we figure out which channel should response to by checking the type
>> and ID.
>>
>> And I think it should be pretty quick, because we only check the set bit
>> from the pending register. And finding the matching channel.
>
> Yeah, I am not worried about the CPU time (although in interrupt
> context, we always should), but rather about whether the code could be
> simplified.
>
> Ah, I think I get it. You want to be able to receive interrupts from
> the same master, but not necessarily for the doorbell function.
> Because of this you cannot use master_id as the index for the channel.
> Am I understanding correctly?
Yes, the DB clients trigger the IRQ through the same master
(HSP_DB_CCPLEX) with it's master_id. We (CPU) can check the ID to know
who is requesting the HSP mbox service. Each ID is unique under the DB
module.
But the ID could be conflict when the HSP mbox driver are working with
multiple HSP sub-function under the same HSP HW block. So we can't just
match the ID to the HSP mbox channel ID.
>
>>
>>>
>>> If having two channels use the same master_id is a valid scenario,
>>> then all matches on master_id should probably be processed, not just
>>> the first one.
>>
>> Each DB channel should have different master_id.
>>
>>
>>>
>>>> + chan = &hsp_mbox->mbox->chans[i];
>>>> +
>>>> + if (!chan->con_priv)
>>>> + continue;
>>>> +
>>>> + mchan = chan->con_priv;
>>>> + if (mchan->type == HSP_MBOX_TYPE_DB &&
>>>> + mchan->db_chan.master_id == master_id)
>>>> + break;
>>>> + chan = NULL;
>>>> + }
>>>> +
>>>> + if (chan)
>>>> + mbox_chan_received_data(chan, NULL);
>>>> + }
>>>> + spin_unlock(&hsp_mbox->lock);
>>>> +
>>>> + return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static int hsp_db_send_data(struct mbox_chan *chan, void *data)
>>>> +{
>>>> + struct tegra_hsp_mbox_chan *mchan = chan->con_priv;
>>>> + struct tegra_hsp_db_chan *db_chan = &mchan->db_chan;
>>>> + struct tegra_hsp_mbox *hsp_mbox =
>>>> dev_get_drvdata(chan->mbox->dev);
>>>> +
>>>> + hsp_writel(hsp_mbox->db_base[db_chan->db_id], HSP_DB_REG_TRIGGER,
>>>> 1);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int hsp_db_startup(struct mbox_chan *chan)
>>>> +{
>>>> + struct tegra_hsp_mbox_chan *mchan = chan->con_priv;
>>>> + struct tegra_hsp_db_chan *db_chan = &mchan->db_chan;
>>>> + struct tegra_hsp_mbox *hsp_mbox =
>>>> dev_get_drvdata(chan->mbox->dev);
>>>> + u32 val;
>>>> + unsigned long flag;
>>>> +
>>>> + if (db_chan->master_id >= MAX_NUM_HSP_CHAN) {
>>>> + dev_err(chan->mbox->dev, "invalid HSP chan: master ID:
>>>> %d\n",
>>>> + db_chan->master_id);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + spin_lock_irqsave(&hsp_mbox->lock, flag);
>>>> + val = hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX],
>>>> HSP_DB_REG_ENABLE);
>>>> + val |= BIT(db_chan->master_id);
>>>> + hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_ENABLE,
>>>> val);
>>>> + spin_unlock_irqrestore(&hsp_mbox->lock, flag);
>>>> +
>>>> + if (!hsp_db_can_ring(hsp_mbox->db_base[db_chan->db_id]))
>>>> + return -ENODEV;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void hsp_db_shutdown(struct mbox_chan *chan)
>>>> +{
>>>> + struct tegra_hsp_mbox_chan *mchan = chan->con_priv;
>>>> + struct tegra_hsp_db_chan *db_chan = &mchan->db_chan;
>>>> + struct tegra_hsp_mbox *hsp_mbox =
>>>> dev_get_drvdata(chan->mbox->dev);
>>>> + u32 val;
>>>> + unsigned long flag;
>>>> +
>>>> + spin_lock_irqsave(&hsp_mbox->lock, flag);
>>>> + val = hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX],
>>>> HSP_DB_REG_ENABLE);
>>>> + val &= ~BIT(db_chan->master_id);
>>>> + hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_ENABLE,
>>>> val);
>>>> + spin_unlock_irqrestore(&hsp_mbox->lock, flag);
>>>> +}
>>>> +
>>>> +static bool hsp_db_last_tx_done(struct mbox_chan *chan)
>>>> +{
>>>> + return true;
>>>> +}
>>>> +
>>>> +static int tegra_hsp_db_init(struct tegra_hsp_mbox *hsp_mbox,
>>>> + struct mbox_chan *mchan, int master_id)
>>>> +{
>>>> + struct platform_device *pdev =
>>>> to_platform_device(hsp_mbox->mbox->dev);
>>>> + struct tegra_hsp_mbox_chan *hsp_mbox_chan;
>>>> + int ret;
>>>> +
>>>> + if (!hsp_mbox->db_irq) {
>>>> + int i;
>>>> +
>>>> + hsp_mbox->db_irq = platform_get_irq_byname(pdev,
>>>> "doorbell");
>>>
>>>
>>> Getting the IRQ sounds more like a job for probe() - I don't see the
>>> benefit of lazy-doing it?
>>
>>
>> We only need the IRQ when the client is requesting the DB service. For other
>> HSP sub-modules, they are using different IRQ. So I didn't do that at probe
>> time.
>
> Ok, but probe() is where resources should be acquired... and at the
> very least DT properties be looked up. In this case there is no hard
> requirement for doing it elsewhere.
>
> Is this interrupt absolutely required? Or can we tolerate to not use
> the doorbell service? In the first case, the driver should fail during
> probe(), not sometime later. In the second case, you should still get
> all the interrupts in probe(), then disable them if they are not
> needed, and check in this function whether db_irq is a valid interrupt
> number to decide whether or not we can use doorbell.
Ah, I understand your concern now. It should be ok to move to probe().
Will fix that.
>
>>
>>>
>>>> + ret = devm_request_irq(&pdev->dev, hsp_mbox->db_irq,
>>>> + hsp_db_irq, IRQF_NO_SUSPEND,
>>>> + dev_name(&pdev->dev), hsp_mbox);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + for (i = 0; i < MAX_NUM_HSP_DB; i++)
>>>> + hsp_mbox->db_base[i] = hsp_db_offset(i,
>>>> hsp_mbox);
>>>
>>>
>>> Same here, cannot this be moved into probe()?
>>
>> Same as above, only needed when the client requests it.
>
> But you don't waste any resources by doing it preemptively in probe().
> So let's keep related code in the same place.
Okay.
Thanks,
-Joseph
[toc] | [prev] | [next] | [standalone]
| From | Sivaram Nair <sivaramn@nvidia.com> |
|---|---|
| Date | 2016-07-07 23:40 +0200 |
| Subject | Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver |
| Message-ID | <rSpbA-40A-23@gated-at.bofh.it> |
| In reply to | #1438195 |
On Thu, Jul 07, 2016 at 02:37:27PM +0800, Joseph Lo wrote:
> On 07/06/2016 08:23 PM, Alexandre Courbot wrote:
> >On Wed, Jul 6, 2016 at 6:06 PM, Joseph Lo <josephl@nvidia.com> wrote:
> >>On 07/06/2016 03:05 PM, Alexandre Courbot wrote:
> >>>
> >>>On Tue, Jul 5, 2016 at 6:04 PM, Joseph Lo <josephl@nvidia.com> wrote:
> >>>>
> >>>>The Tegra HSP mailbox driver implements the signaling doorbell-based
> >>>>interprocessor communication (IPC) for remote processors currently. The
> >>>>HSP HW modules support some different features for that, which are
> >>>>shared mailboxes, shared semaphores, arbitrated semaphores, and
> >>>>doorbells. And there are multiple HSP HW instances on the chip. So the
> >>>>driver is extendable to support more features for different IPC
> >>>>requirement.
> >>>>
> >>>>The driver of remote processor can use it as a mailbox client and deal
> >>>>with the IPC protocol to synchronize the data communications.
> >>>>
> >>>>Signed-off-by: Joseph Lo <josephl@nvidia.com>
> >>>>---
> >>>>Changes in V2:
> >>>>- Update the driver to support the binding changes in V2
> >>>>- it's extendable to support multiple HSP sub-modules on the same HSP HW
> >>>>block
> >>>> now.
> >>>>---
> >>>> drivers/mailbox/Kconfig | 9 +
> >>>> drivers/mailbox/Makefile | 2 +
> >>>> drivers/mailbox/tegra-hsp.c | 418
> >>>>++++++++++++++++++++++++++++++++++++++++++++
> >>>> 3 files changed, 429 insertions(+)
> >>>> create mode 100644 drivers/mailbox/tegra-hsp.c
> >>>>
> >>>>diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> >>>>index 5305923752d2..fe584cb54720 100644
> >>>>--- a/drivers/mailbox/Kconfig
> >>>>+++ b/drivers/mailbox/Kconfig
> >>>>@@ -114,6 +114,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"
> >>>
> >>>
> >>>Space missing before the opening parenthesis (same in the patch title
> >>>btw).
> >>
> >>Okay.
> >>>
> >>>
> >>>>+ 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.
> >>>
> >>>
> >>>Since this option is selected automatically by ARCH_TEGRA_186_SOC, you
> >>>should probably drop the last 2 sentences.
> >>
> >>Okay.
> >>
> >>>
> >>>>+
> >>>> 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 0be3e742bb7d..26d8f91c7fea 100644
> >>>>--- a/drivers/mailbox/Makefile
> >>>>+++ b/drivers/mailbox/Makefile
> >>>>@@ -25,3 +25,5 @@ obj-$(CONFIG_TI_MESSAGE_MANAGER) += ti-msgmgr.o
> >>>> obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
> >>>>
> >>>> obj-$(CONFIG_HI6220_MBOX) += hi6220-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..93c3ef58f29f
> >>>>--- /dev/null
> >>>>+++ b/drivers/mailbox/tegra-hsp.c
> >>>>@@ -0,0 +1,418 @@
> >>>>+/*
> >>>>+ * 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 <dt-bindings/mailbox/tegra186-hsp.h>
> >>>>+
> >>>>+#define HSP_INT_DIMENSIONING 0x380
> >>>>+#define HSP_nSM_OFFSET 0
> >>>>+#define HSP_nSS_OFFSET 4
> >>>>+#define HSP_nAS_OFFSET 8
> >>>>+#define HSP_nDB_OFFSET 12
> >>>>+#define HSP_nSI_OFFSET 16
> >>>
> >>>
> >>>Would be nice to have comments to understand what SM, SS, AS, etc.
> >>>stand for (Shared Mailboxes, Shared Semaphores, Arbitrated Semaphores
> >>>but you need to look at the patch description to understand that). A
> >>>top-of-file comment explaning the necessary concepts to read this code
> >>>would do the trick.
> >>
> >>Yes, will fix that.
> >>>
> >>>
> >>>>+#define HSP_nINT_MASK 0xf
> >>>>+
> >>>>+#define HSP_DB_REG_TRIGGER 0x0
> >>>>+#define HSP_DB_REG_ENABLE 0x4
> >>>>+#define HSP_DB_REG_RAW 0x8
> >>>>+#define HSP_DB_REG_PENDING 0xc
> >>>>+
> >>>>+#define HSP_DB_CCPLEX 1
> >>>>+#define HSP_DB_BPMP 3
> >>>
> >>>
> >>>Maybe turn this into enum and use that type for
> >>>tegra_hsp_db_chan::db_id? Also have MAX_NUM_HSP_DB here, since it is
> >>>related to these values?
> >>
> >>Okay.
> >>
> >>>
> >>>>+
> >>>>+#define MAX_NUM_HSP_CHAN 32
> >>>>+#define MAX_NUM_HSP_DB 7
> >>>>+
> >>>>+#define hsp_db_offset(i, d) \
> >>>>+ (d->base + ((1 + (d->nr_sm >> 1) + d->nr_ss + d->nr_as) << 16) +
> >>>>\
> >>>>+ (i) * 0x100)
> >>>>+
> >>>>+struct tegra_hsp_db_chan {
> >>>>+ int master_id;
> >>>>+ int db_id;
> >>>>+};
> >>>>+
> >>>>+struct tegra_hsp_mbox_chan {
> >>>>+ int type;
> >>>>+ union {
> >>>>+ struct tegra_hsp_db_chan db_chan;
> >>>>+ };
> >>>>+};
> >>>>+
> >>>>+struct tegra_hsp_mbox {
> >>>>+ struct mbox_controller *mbox;
> >>>>+ void __iomem *base;
> >>>>+ void __iomem *db_base[MAX_NUM_HSP_DB];
> >>>>+ int db_irq;
> >>>>+ int nr_sm;
> >>>>+ int nr_as;
> >>>>+ int nr_ss;
> >>>>+ int nr_db;
> >>>>+ int nr_si;
> >>>>+ spinlock_t lock;
> >>>>+};
> >>>>+
> >>>>+static inline u32 hsp_readl(void __iomem *base, int reg)
> >>>>+{
> >>>>+ return readl(base + reg);
> >>>>+}
> >>>>+
> >>>>+static inline void hsp_writel(void __iomem *base, int reg, u32 val)
> >>>>+{
> >>>>+ writel(val, base + reg);
> >>>>+ readl(base + reg);
> >>>>+}
> >>>>+
> >>>>+static int hsp_db_can_ring(void __iomem *db_base)
> >>>>+{
> >>>>+ u32 reg;
> >>>>+
> >>>>+ reg = hsp_readl(db_base, HSP_DB_REG_ENABLE);
> >>>>+
> >>>>+ return !!(reg & BIT(HSP_DB_MASTER_CCPLEX));
> >>>>+}
> >>>>+
> >>>>+static irqreturn_t hsp_db_irq(int irq, void *p)
> >>>>+{
> >>>>+ struct tegra_hsp_mbox *hsp_mbox = p;
> >>>>+ ulong val;
> >>>>+ int master_id;
> >>>>+
> >>>>+ val = (ulong)hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX],
> >>>>+ HSP_DB_REG_PENDING);
> >>>>+ hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_PENDING,
> >>>>val);
> >>>>+
> >>>>+ spin_lock(&hsp_mbox->lock);
> >>>>+ for_each_set_bit(master_id, &val, MAX_NUM_HSP_CHAN) {
> >>>>+ struct mbox_chan *chan;
> >>>>+ struct tegra_hsp_mbox_chan *mchan;
> >>>>+ int i;
> >>>>+
> >>>>+ for (i = 0; i < MAX_NUM_HSP_CHAN; i++) {
> >>>
> >>>
> >>>I wonder if this could not be optimized. You are doing a double loop
> >>>on MAX_NUM_HSP_CHAN to look for an identical master_id. Since it seems
> >>>like the same master_id cannot be used twice (considering that the
> >>>inner loop only processes the first match), couldn't you just select
> >>>the free channel in of_hsp_mbox_xlate() by doing
> >>>&mbox->chans[master_id] (and returning an error if it is already
> >>>used), then simply getting chan as &hsp_mbox->mbox->chans[master_id]
> >>>instead of having the inner loop below? That would remove the need for
> >>>the second loop.
> >>
> >>
> >>That was exactly what I did in the V1, which only supported one HSP
> >>sub-module per HSP HW block. So we can just use the master_id as the mbox
> >>channel ID.
> >>
> >>Meanwhile, the V2 is purposed to support multiple HSP sub-modules to be
> >>running on the same HSP HW block. The "ID" between different modules could
> >>be conflict. So I dropped the mechanism that used the master_id as the mbox
> >>channel ID.
> >>
> >>Instead, the channel is allocated at the time, when the client is bound to
> >>one of the HSP sub-modules. And we store the "ID" information into the
> >>private mbox channel data, which can help us to figure out which mbox
> >>channel should response to the interrupt.
> >>
> >>In the doorbell case, because all the DB clients are shared the same DB IRQ
> >>at the CPU side. So in the ISR, we need to figure out the IRQ source, which
> >>is the master_id that the IRQ came from. This is the outer loop. The inner
> >>loop, we figure out which channel should response to by checking the type
> >>and ID.
> >>
> >>And I think it should be pretty quick, because we only check the set bit
> >>from the pending register. And finding the matching channel.
> >
> >Yeah, I am not worried about the CPU time (although in interrupt
> >context, we always should), but rather about whether the code could be
> >simplified.
> >
> >Ah, I think I get it. You want to be able to receive interrupts from
> >the same master, but not necessarily for the doorbell function.
> >Because of this you cannot use master_id as the index for the channel.
> >Am I understanding correctly?
>
> Yes, the DB clients trigger the IRQ through the same master
> (HSP_DB_CCPLEX) with it's master_id. We (CPU) can check the ID to
> know who is requesting the HSP mbox service. Each ID is unique under
> the DB module.
>
> But the ID could be conflict when the HSP mbox driver are working
> with multiple HSP sub-function under the same HSP HW block. So we
> can't just match the ID to the HSP mbox channel ID.
Joseph, can you think about any other sub-function that uses the same
master ids (& those that does not have their own irqs)? I wonder if we
are over-engineering this. I think the hsp_db_startup() and
hsp_db_shutdown() does not support sharing masters - _startup() by one
followed by _shutdown() from another will mask the interrupt. If there
is infact other potential sub-functions, I would imagine this will
translate to other values of the tegra_hsp_mbox_chan.type than
HSP_MBOX_TYPE_DB? If yes, then you should be able to remove need of this
inner loop by having per-sub-function mboxes or by combining 'type' and
'master_id' to make single index value?
>
> >
> >>
> >>>
> >>>If having two channels use the same master_id is a valid scenario,
> >>>then all matches on master_id should probably be processed, not just
> >>>the first one.
> >>
> >>Each DB channel should have different master_id.
> >>
> >>
> >>>
> >>>>+ chan = &hsp_mbox->mbox->chans[i];
> >>>>+
> >>>>+ if (!chan->con_priv)
> >>>>+ continue;
> >>>>+
> >>>>+ mchan = chan->con_priv;
> >>>>+ if (mchan->type == HSP_MBOX_TYPE_DB &&
> >>>>+ mchan->db_chan.master_id == master_id)
> >>>>+ break;
> >>>>+ chan = NULL;
> >>>>+ }
> >>>>+
> >>>>+ if (chan)
> >>>>+ mbox_chan_received_data(chan, NULL);
> >>>>+ }
> >>>>+ spin_unlock(&hsp_mbox->lock);
> >>>>+
> >>>>+ return IRQ_HANDLED;
> >>>>+}
> >>>>+
> >>>>+static int hsp_db_send_data(struct mbox_chan *chan, void *data)
> >>>>+{
> >>>>+ struct tegra_hsp_mbox_chan *mchan = chan->con_priv;
> >>>>+ struct tegra_hsp_db_chan *db_chan = &mchan->db_chan;
> >>>>+ struct tegra_hsp_mbox *hsp_mbox =
> >>>>dev_get_drvdata(chan->mbox->dev);
> >>>>+
> >>>>+ hsp_writel(hsp_mbox->db_base[db_chan->db_id], HSP_DB_REG_TRIGGER,
> >>>>1);
> >>>>+
> >>>>+ return 0;
> >>>>+}
> >>>>+
> >>>>+static int hsp_db_startup(struct mbox_chan *chan)
> >>>>+{
> >>>>+ struct tegra_hsp_mbox_chan *mchan = chan->con_priv;
> >>>>+ struct tegra_hsp_db_chan *db_chan = &mchan->db_chan;
> >>>>+ struct tegra_hsp_mbox *hsp_mbox =
> >>>>dev_get_drvdata(chan->mbox->dev);
> >>>>+ u32 val;
> >>>>+ unsigned long flag;
> >>>>+
> >>>>+ if (db_chan->master_id >= MAX_NUM_HSP_CHAN) {
> >>>>+ dev_err(chan->mbox->dev, "invalid HSP chan: master ID:
> >>>>%d\n",
> >>>>+ db_chan->master_id);
> >>>>+ return -EINVAL;
> >>>>+ }
> >>>>+
> >>>>+ spin_lock_irqsave(&hsp_mbox->lock, flag);
> >>>>+ val = hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX],
> >>>>HSP_DB_REG_ENABLE);
> >>>>+ val |= BIT(db_chan->master_id);
> >>>>+ hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_ENABLE,
> >>>>val);
> >>>>+ spin_unlock_irqrestore(&hsp_mbox->lock, flag);
> >>>>+
> >>>>+ if (!hsp_db_can_ring(hsp_mbox->db_base[db_chan->db_id]))
> >>>>+ return -ENODEV;
> >>>>+
> >>>>+ return 0;
> >>>>+}
> >>>>+
> >>>>+static void hsp_db_shutdown(struct mbox_chan *chan)
> >>>>+{
> >>>>+ struct tegra_hsp_mbox_chan *mchan = chan->con_priv;
> >>>>+ struct tegra_hsp_db_chan *db_chan = &mchan->db_chan;
> >>>>+ struct tegra_hsp_mbox *hsp_mbox =
> >>>>dev_get_drvdata(chan->mbox->dev);
> >>>>+ u32 val;
> >>>>+ unsigned long flag;
> >>>>+
> >>>>+ spin_lock_irqsave(&hsp_mbox->lock, flag);
> >>>>+ val = hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX],
> >>>>HSP_DB_REG_ENABLE);
> >>>>+ val &= ~BIT(db_chan->master_id);
> >>>>+ hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_ENABLE,
> >>>>val);
> >>>>+ spin_unlock_irqrestore(&hsp_mbox->lock, flag);
> >>>>+}
> >>>>+
> >>>>+static bool hsp_db_last_tx_done(struct mbox_chan *chan)
> >>>>+{
> >>>>+ return true;
> >>>>+}
> >>>>+
> >>>>+static int tegra_hsp_db_init(struct tegra_hsp_mbox *hsp_mbox,
> >>>>+ struct mbox_chan *mchan, int master_id)
> >>>>+{
> >>>>+ struct platform_device *pdev =
> >>>>to_platform_device(hsp_mbox->mbox->dev);
> >>>>+ struct tegra_hsp_mbox_chan *hsp_mbox_chan;
> >>>>+ int ret;
> >>>>+
> >>>>+ if (!hsp_mbox->db_irq) {
> >>>>+ int i;
> >>>>+
> >>>>+ hsp_mbox->db_irq = platform_get_irq_byname(pdev,
> >>>>"doorbell");
> >>>
> >>>
> >>>Getting the IRQ sounds more like a job for probe() - I don't see the
> >>>benefit of lazy-doing it?
> >>
> >>
> >>We only need the IRQ when the client is requesting the DB service. For other
> >>HSP sub-modules, they are using different IRQ. So I didn't do that at probe
> >>time.
> >
> >Ok, but probe() is where resources should be acquired... and at the
> >very least DT properties be looked up. In this case there is no hard
> >requirement for doing it elsewhere.
> >
> >Is this interrupt absolutely required? Or can we tolerate to not use
> >the doorbell service? In the first case, the driver should fail during
> >probe(), not sometime later. In the second case, you should still get
> >all the interrupts in probe(), then disable them if they are not
> >needed, and check in this function whether db_irq is a valid interrupt
> >number to decide whether or not we can use doorbell.
>
> Ah, I understand your concern now. It should be ok to move to
> probe(). Will fix that.
>
> >
> >>
> >>>
> >>>>+ ret = devm_request_irq(&pdev->dev, hsp_mbox->db_irq,
> >>>>+ hsp_db_irq, IRQF_NO_SUSPEND,
> >>>>+ dev_name(&pdev->dev), hsp_mbox);
> >>>>+ if (ret)
> >>>>+ return ret;
> >>>>+
> >>>>+ for (i = 0; i < MAX_NUM_HSP_DB; i++)
> >>>>+ hsp_mbox->db_base[i] = hsp_db_offset(i,
> >>>>hsp_mbox);
> >>>
> >>>
> >>>Same here, cannot this be moved into probe()?
> >>
> >>Same as above, only needed when the client requests it.
> >
> >But you don't waste any resources by doing it preemptively in probe().
> >So let's keep related code in the same place.
>
> Okay.
>
> Thanks,
> -Joseph
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[toc] | [prev] | [next] | [standalone]
| From | Stephen Warren <swarren@wwwdotorg.org> |
|---|---|
| Date | 2016-07-06 19:00 +0200 |
| Subject | Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver |
| Message-ID | <rRYl3-3dx-1@gated-at.bofh.it> |
| In reply to | #1437577 |
On 07/06/2016 03:06 AM, Joseph Lo wrote:
> On 07/06/2016 03:05 PM, Alexandre Courbot wrote:
>> On Tue, Jul 5, 2016 at 6:04 PM, Joseph Lo <josephl@nvidia.com> wrote:
>>> The Tegra HSP mailbox driver implements the signaling doorbell-based
>>> interprocessor communication (IPC) for remote processors currently. The
>>> HSP HW modules support some different features for that, which are
>>> shared mailboxes, shared semaphores, arbitrated semaphores, and
>>> doorbells. And there are multiple HSP HW instances on the chip. So the
>>> driver is extendable to support more features for different IPC
>>> requirement.
>>>
>>> The driver of remote processor can use it as a mailbox client and deal
>>> with the IPC protocol to synchronize the data communications.
>>> diff --git a/drivers/mailbox/tegra-hsp.c b/drivers/mailbox/tegra-hsp.c
>>> +static irqreturn_t hsp_db_irq(int irq, void *p)
>>> +{
>>> + struct tegra_hsp_mbox *hsp_mbox = p;
>>> + ulong val;
>>> + int master_id;
>>> +
>>> + val = (ulong)hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX],
>>> + HSP_DB_REG_PENDING);
>>> + hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX],
>>> HSP_DB_REG_PENDING, val);
>>> +
>>> + spin_lock(&hsp_mbox->lock);
>>> + for_each_set_bit(master_id, &val, MAX_NUM_HSP_CHAN) {
>>> + struct mbox_chan *chan;
>>> + struct tegra_hsp_mbox_chan *mchan;
>>> + int i;
>>> +
>>> + for (i = 0; i < MAX_NUM_HSP_CHAN; i++) {
>>
>> I wonder if this could not be optimized. You are doing a double loop
>> on MAX_NUM_HSP_CHAN to look for an identical master_id. Since it seems
>> like the same master_id cannot be used twice (considering that the
>> inner loop only processes the first match), couldn't you just select
>> the free channel in of_hsp_mbox_xlate() by doing
>> &mbox->chans[master_id] (and returning an error if it is already
>> used), then simply getting chan as &hsp_mbox->mbox->chans[master_id]
>> instead of having the inner loop below? That would remove the need for
>> the second loop.
>
> That was exactly what I did in the V1, which only supported one HSP
> sub-module per HSP HW block. So we can just use the master_id as the
> mbox channel ID.
>
> Meanwhile, the V2 is purposed to support multiple HSP sub-modules to be
> running on the same HSP HW block. The "ID" between different modules
> could be conflict. So I dropped the mechanism that used the master_id as
> the mbox channel ID.
I haven't looked at the code in this patch since I'm mainly concerned
about the DT bindings. However, I will say that nothing in the change to
the mailbox specifier in DT should have required /any/ changes to the
code, except to add a single check to validate that the "mailbox type"
encoded into the top 16 bits of the mailbox ID were 0, and hence
represented a doorbell rather than anything else. Any enhancements to
support other mailbox types could have happened later, and I doubt would
require anything dynamic even then.
>>> +static int tegra_hsp_db_init(struct tegra_hsp_mbox *hsp_mbox,
>>> + struct mbox_chan *mchan, int master_id)
>>> +{
>>> + struct platform_device *pdev = to_platform_device(hsp_mbox->mbox->dev);
>>> + struct tegra_hsp_mbox_chan *hsp_mbox_chan;
>>> + int ret;
>>> +
>>> + if (!hsp_mbox->db_irq) {
>>> + int i;
>>> +
>>> + hsp_mbox->db_irq = platform_get_irq_byname(pdev, "doorbell");
>>
>> Getting the IRQ sounds more like a job for probe() - I don't see the
>> benefit of lazy-doing it?
>
> We only need the IRQ when the client is requesting the DB service. For
> other HSP sub-modules, they are using different IRQ. So I didn't do that
> at probe time.
All resources provided by other devices/drivers must be acquired at
probe time, since that's the only time it's possible to defer probe if
the provider of the resource is not available.
If you don't follow that rule, what happens is:
1) This driver probes.
2) Some other driver calls tegra_hsp_db_init(), and it fails since the
provider of the IRQ is not yet available. This likely ends up returning
something other than -EPROBE_DEFER since the HSP driver was found
successfully (thus there is no deferred probe situation as far as the
mailbox core is concerned), it's just that the mailbox channel
lookup/init/... failed.
3) The other driver's probe() fails due to this, but since the error
wasn't a probe deferral, the other driver's probe() is never retried.
[toc] | [prev] | [next] | [standalone]
| From | Joseph Lo <josephl@nvidia.com> |
|---|---|
| Date | 2016-07-07 08:50 +0200 |
| Subject | Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver |
| Message-ID | <rSbii-3kT-19@gated-at.bofh.it> |
| In reply to | #1437846 |
On 07/07/2016 12:50 AM, Stephen Warren wrote:
> On 07/06/2016 03:06 AM, Joseph Lo wrote:
>> On 07/06/2016 03:05 PM, Alexandre Courbot wrote:
>>> On Tue, Jul 5, 2016 at 6:04 PM, Joseph Lo <josephl@nvidia.com> wrote:
>>>> The Tegra HSP mailbox driver implements the signaling doorbell-based
>>>> interprocessor communication (IPC) for remote processors currently. The
>>>> HSP HW modules support some different features for that, which are
>>>> shared mailboxes, shared semaphores, arbitrated semaphores, and
>>>> doorbells. And there are multiple HSP HW instances on the chip. So the
>>>> driver is extendable to support more features for different IPC
>>>> requirement.
>>>>
>>>> The driver of remote processor can use it as a mailbox client and deal
>>>> with the IPC protocol to synchronize the data communications.
>
>>>> diff --git a/drivers/mailbox/tegra-hsp.c b/drivers/mailbox/tegra-hsp.c
>
>>>> +static irqreturn_t hsp_db_irq(int irq, void *p)
>>>> +{
>>>> + struct tegra_hsp_mbox *hsp_mbox = p;
>>>> + ulong val;
>>>> + int master_id;
>>>> +
>>>> + val = (ulong)hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX],
>>>> + HSP_DB_REG_PENDING);
>>>> + hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX],
>>>> HSP_DB_REG_PENDING, val);
>>>> +
>>>> + spin_lock(&hsp_mbox->lock);
>>>> + for_each_set_bit(master_id, &val, MAX_NUM_HSP_CHAN) {
>>>> + struct mbox_chan *chan;
>>>> + struct tegra_hsp_mbox_chan *mchan;
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < MAX_NUM_HSP_CHAN; i++) {
>>>
>>> I wonder if this could not be optimized. You are doing a double loop
>>> on MAX_NUM_HSP_CHAN to look for an identical master_id. Since it seems
>>> like the same master_id cannot be used twice (considering that the
>>> inner loop only processes the first match), couldn't you just select
>>> the free channel in of_hsp_mbox_xlate() by doing
>>> &mbox->chans[master_id] (and returning an error if it is already
>>> used), then simply getting chan as &hsp_mbox->mbox->chans[master_id]
>>> instead of having the inner loop below? That would remove the need for
>>> the second loop.
>>
>> That was exactly what I did in the V1, which only supported one HSP
>> sub-module per HSP HW block. So we can just use the master_id as the
>> mbox channel ID.
>>
>> Meanwhile, the V2 is purposed to support multiple HSP sub-modules to be
>> running on the same HSP HW block. The "ID" between different modules
>> could be conflict. So I dropped the mechanism that used the master_id as
>> the mbox channel ID.
>
> I haven't looked at the code in this patch since I'm mainly concerned
> about the DT bindings. However, I will say that nothing in the change to
> the mailbox specifier in DT should have required /any/ changes to the
> code, except to add a single check to validate that the "mailbox type"
> encoded into the top 16 bits of the mailbox ID were 0, and hence
> represented a doorbell rather than anything else. Any enhancements to
> support other mailbox types could have happened later, and I doubt would
> require anything dynamic even then.
Yes, I only add the code for that change. Maybe some glue code for the
extend-ability to support more HSP modules in the future.
>
>>>> +static int tegra_hsp_db_init(struct tegra_hsp_mbox *hsp_mbox,
>>>> + struct mbox_chan *mchan, int master_id)
>>>> +{
>>>> + struct platform_device *pdev =
>>>> to_platform_device(hsp_mbox->mbox->dev);
>>>> + struct tegra_hsp_mbox_chan *hsp_mbox_chan;
>>>> + int ret;
>>>> +
>>>> + if (!hsp_mbox->db_irq) {
>>>> + int i;
>>>> +
>>>> + hsp_mbox->db_irq = platform_get_irq_byname(pdev,
>>>> "doorbell");
>>>
>>> Getting the IRQ sounds more like a job for probe() - I don't see the
>>> benefit of lazy-doing it?
>>
>> We only need the IRQ when the client is requesting the DB service. For
>> other HSP sub-modules, they are using different IRQ. So I didn't do that
>> at probe time.
>
> All resources provided by other devices/drivers must be acquired at
> probe time, since that's the only time it's possible to defer probe if
> the provider of the resource is not available.
>
> If you don't follow that rule, what happens is:
>
> 1) This driver probes.
>
> 2) Some other driver calls tegra_hsp_db_init(), and it fails since the
> provider of the IRQ is not yet available. This likely ends up returning
> something other than -EPROBE_DEFER since the HSP driver was found
> successfully (thus there is no deferred probe situation as far as the
> mailbox core is concerned), it's just that the mailbox channel
> lookup/init/... failed.
>
> 3) The other driver's probe() fails due to this, but since the error
> wasn't a probe deferral, the other driver's probe() is never retried.
Agree, will fix this.
Thanks,
-Joseph
[toc] | [prev] | [next] | [standalone]
| From | Sivaram Nair <sivaramn@nvidia.com> |
|---|---|
| Date | 2016-07-07 23:20 +0200 |
| Subject | Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver |
| Message-ID | <rSoSe-3SM-11@gated-at.bofh.it> |
| In reply to | #1436858 |
On Tue, Jul 05, 2016 at 05:04:23PM +0800, Joseph Lo wrote:
> The Tegra HSP mailbox driver implements the signaling doorbell-based
> interprocessor communication (IPC) for remote processors currently. The
> HSP HW modules support some different features for that, which are
> shared mailboxes, shared semaphores, arbitrated semaphores, and
> doorbells. And there are multiple HSP HW instances on the chip. So the
> driver is extendable to support more features for different IPC
> requirement.
>
> The driver of remote processor can use it as a mailbox client and deal
> with the IPC protocol to synchronize the data communications.
>
> Signed-off-by: Joseph Lo <josephl@nvidia.com>
> ---
> Changes in V2:
> - Update the driver to support the binding changes in V2
> - it's extendable to support multiple HSP sub-modules on the same HSP HW block
> now.
> ---
> drivers/mailbox/Kconfig | 9 +
> drivers/mailbox/Makefile | 2 +
> drivers/mailbox/tegra-hsp.c | 418 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 429 insertions(+)
> create mode 100644 drivers/mailbox/tegra-hsp.c
>
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index 5305923752d2..fe584cb54720 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -114,6 +114,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 0be3e742bb7d..26d8f91c7fea 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -25,3 +25,5 @@ obj-$(CONFIG_TI_MESSAGE_MANAGER) += ti-msgmgr.o
> obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
>
> obj-$(CONFIG_HI6220_MBOX) += hi6220-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..93c3ef58f29f
> --- /dev/null
> +++ b/drivers/mailbox/tegra-hsp.c
> @@ -0,0 +1,418 @@
> +/*
> + * 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 <dt-bindings/mailbox/tegra186-hsp.h>
> +
> +#define HSP_INT_DIMENSIONING 0x380
> +#define HSP_nSM_OFFSET 0
> +#define HSP_nSS_OFFSET 4
> +#define HSP_nAS_OFFSET 8
> +#define HSP_nDB_OFFSET 12
> +#define HSP_nSI_OFFSET 16
> +#define HSP_nINT_MASK 0xf
> +
> +#define HSP_DB_REG_TRIGGER 0x0
> +#define HSP_DB_REG_ENABLE 0x4
> +#define HSP_DB_REG_RAW 0x8
> +#define HSP_DB_REG_PENDING 0xc
> +
> +#define HSP_DB_CCPLEX 1
> +#define HSP_DB_BPMP 3
> +
> +#define MAX_NUM_HSP_CHAN 32
Is this an arbitrarily chosen number?
> +#define MAX_NUM_HSP_DB 7
> +
> +#define hsp_db_offset(i, d) \
> + (d->base + ((1 + (d->nr_sm >> 1) + d->nr_ss + d->nr_as) << 16) + \
> + (i) * 0x100)
> +
> +struct tegra_hsp_db_chan {
> + int master_id;
> + int db_id;
These should be unsigned?
> +};
> +
> +struct tegra_hsp_mbox_chan {
> + int type;
This too...
> + union {
> + struct tegra_hsp_db_chan db_chan;
> + };
> +};
Why do we need to use a union?
> +
> +struct tegra_hsp_mbox {
> + struct mbox_controller *mbox;
> + void __iomem *base;
> + void __iomem *db_base[MAX_NUM_HSP_DB];
> + int db_irq;
> + int nr_sm;
> + int nr_as;
> + int nr_ss;
> + int nr_db;
> + int nr_si;
> + spinlock_t lock;
You might need to change this to a mutex - see below.
> +};
> +
> +static inline u32 hsp_readl(void __iomem *base, int reg)
> +{
> + return readl(base + reg);
> +}
> +
> +static inline void hsp_writel(void __iomem *base, int reg, u32 val)
> +{
> + writel(val, base + reg);
> + readl(base + reg);
> +}
> +
> +static int hsp_db_can_ring(void __iomem *db_base)
> +{
> + u32 reg;
> +
> + reg = hsp_readl(db_base, HSP_DB_REG_ENABLE);
> +
> + return !!(reg & BIT(HSP_DB_MASTER_CCPLEX));
> +}
> +
> +static irqreturn_t hsp_db_irq(int irq, void *p)
> +{
> + struct tegra_hsp_mbox *hsp_mbox = p;
> + ulong val;
This should be u32 and...
> + int master_id;
> +
> + val = (ulong)hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX],
> + HSP_DB_REG_PENDING);
the cast should/can be removed (hsp_readl and hsp_writel both use u32)?
> + hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_PENDING, val);
> +
> + spin_lock(&hsp_mbox->lock);
> + for_each_set_bit(master_id, &val, MAX_NUM_HSP_CHAN) {
> + struct mbox_chan *chan;
> + struct tegra_hsp_mbox_chan *mchan;
> + int i;
> +
> + for (i = 0; i < MAX_NUM_HSP_CHAN; i++) {
> + chan = &hsp_mbox->mbox->chans[i];
> +
> + if (!chan->con_priv)
> + continue;
> +
> + mchan = chan->con_priv;
> + if (mchan->type == HSP_MBOX_TYPE_DB &&
> + mchan->db_chan.master_id == master_id)
> + break;
> + chan = NULL;
> + }
Like Alexandre, I didn't like this use of inner loop as well. But I will
add my comment to the other thread.
> +
> + if (chan)
> + mbox_chan_received_data(chan, NULL);
> + }
> + spin_unlock(&hsp_mbox->lock);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int hsp_db_send_data(struct mbox_chan *chan, void *data)
> +{
> + struct tegra_hsp_mbox_chan *mchan = chan->con_priv;
> + struct tegra_hsp_db_chan *db_chan = &mchan->db_chan;
> + struct tegra_hsp_mbox *hsp_mbox = dev_get_drvdata(chan->mbox->dev);
> +
> + hsp_writel(hsp_mbox->db_base[db_chan->db_id], HSP_DB_REG_TRIGGER, 1);
> +
> + return 0;
> +}
> +
> +static int hsp_db_startup(struct mbox_chan *chan)
> +{
> + struct tegra_hsp_mbox_chan *mchan = chan->con_priv;
> + struct tegra_hsp_db_chan *db_chan = &mchan->db_chan;
> + struct tegra_hsp_mbox *hsp_mbox = dev_get_drvdata(chan->mbox->dev);
> + u32 val;
> + unsigned long flag;
> +
> + if (db_chan->master_id >= MAX_NUM_HSP_CHAN) {
Is this a valid check? IIUC, MAX_NUM_HSP_CHAN is independent of the
number of masters.
> + dev_err(chan->mbox->dev, "invalid HSP chan: master ID: %d\n",
> + db_chan->master_id);
> + return -EINVAL;
> + }
> +
> + spin_lock_irqsave(&hsp_mbox->lock, flag);
> + val = hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_ENABLE);
> + val |= BIT(db_chan->master_id);
> + hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_ENABLE, val);
> + spin_unlock_irqrestore(&hsp_mbox->lock, flag);
> +
> + if (!hsp_db_can_ring(hsp_mbox->db_base[db_chan->db_id]))
> + return -ENODEV;
> +
> + return 0;
> +}
> +
> +static void hsp_db_shutdown(struct mbox_chan *chan)
> +{
> + struct tegra_hsp_mbox_chan *mchan = chan->con_priv;
> + struct tegra_hsp_db_chan *db_chan = &mchan->db_chan;
> + struct tegra_hsp_mbox *hsp_mbox = dev_get_drvdata(chan->mbox->dev);
> + u32 val;
> + unsigned long flag;
> +
> + spin_lock_irqsave(&hsp_mbox->lock, flag);
> + val = hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_ENABLE);
> + val &= ~BIT(db_chan->master_id);
> + hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX], HSP_DB_REG_ENABLE, val);
> + spin_unlock_irqrestore(&hsp_mbox->lock, flag);
> +}
> +
> +static bool hsp_db_last_tx_done(struct mbox_chan *chan)
> +{
> + return true;
> +}
> +
> +static int tegra_hsp_db_init(struct tegra_hsp_mbox *hsp_mbox,
> + struct mbox_chan *mchan, int master_id)
> +{
> + struct platform_device *pdev = to_platform_device(hsp_mbox->mbox->dev);
> + struct tegra_hsp_mbox_chan *hsp_mbox_chan;
> + int ret;
> +
> + if (!hsp_mbox->db_irq) {
> + int i;
> +
> + hsp_mbox->db_irq = platform_get_irq_byname(pdev, "doorbell");
> + ret = devm_request_irq(&pdev->dev, hsp_mbox->db_irq,
> + hsp_db_irq, IRQF_NO_SUSPEND,
> + dev_name(&pdev->dev), hsp_mbox);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < MAX_NUM_HSP_DB; i++)
> + hsp_mbox->db_base[i] = hsp_db_offset(i, hsp_mbox);
> + }
> +
> + hsp_mbox_chan = devm_kzalloc(&pdev->dev, sizeof(*hsp_mbox_chan),
> + GFP_KERNEL);
> + if (!hsp_mbox_chan)
> + return -ENOMEM;
> +
> + hsp_mbox_chan->type = HSP_MBOX_TYPE_DB;
> + hsp_mbox_chan->db_chan.master_id = master_id;
> + switch (master_id) {
> + case HSP_DB_MASTER_BPMP:
> + hsp_mbox_chan->db_chan.db_id = HSP_DB_BPMP;
> + break;
> + default:
> + hsp_mbox_chan->db_chan.db_id = MAX_NUM_HSP_DB;
> + break;
> + }
> +
> + mchan->con_priv = hsp_mbox_chan;
> +
> + return 0;
> +}
> +
> +static int hsp_send_data(struct mbox_chan *chan, void *data)
> +{
> + struct tegra_hsp_mbox_chan *hsp_mbox_chan = chan->con_priv;
> + int ret = 0;
> +
> + switch (hsp_mbox_chan->type) {
> + case HSP_MBOX_TYPE_DB:
> + ret = hsp_db_send_data(chan, data);
> + break;
> + default:
Should you return an error here?
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static int hsp_startup(struct mbox_chan *chan)
> +{
> + struct tegra_hsp_mbox_chan *hsp_mbox_chan = chan->con_priv;
> + int ret = 0;
> +
> + switch (hsp_mbox_chan->type) {
> + case HSP_MBOX_TYPE_DB:
> + ret = hsp_db_startup(chan);
> + break;
> + default:
And here too...?
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static void hsp_shutdown(struct mbox_chan *chan)
> +{
> + struct tegra_hsp_mbox_chan *hsp_mbox_chan = chan->con_priv;
> +
> + switch (hsp_mbox_chan->type) {
> + case HSP_MBOX_TYPE_DB:
> + hsp_db_shutdown(chan);
> + break;
> + default:
> + break;
> + }
> +
> + chan->con_priv = NULL;
> +}
> +
> +static bool hsp_last_tx_done(struct mbox_chan *chan)
> +{
> + struct tegra_hsp_mbox_chan *hsp_mbox_chan = chan->con_priv;
> + bool ret = true;
> +
> + switch (hsp_mbox_chan->type) {
> + case HSP_MBOX_TYPE_DB:
> + ret = hsp_db_last_tx_done(chan);
hsp_db_last_tx_done() return true - so we might as well make this parent
function to return true and remove hsp_db_last_tx_done()?
> + break;
> + default:
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static const struct mbox_chan_ops tegra_hsp_ops = {
> + .send_data = hsp_send_data,
> + .startup = hsp_startup,
> + .shutdown = hsp_shutdown,
> + .last_tx_done = hsp_last_tx_done,
> +};
> +
> +static const struct of_device_id tegra_hsp_match[] = {
> + { .compatible = "nvidia,tegra186-hsp" },
> + { }
> +};
> +
> +static struct mbox_chan *
> +of_hsp_mbox_xlate(struct mbox_controller *mbox,
> + const struct of_phandle_args *sp)
> +{
> + int mbox_id = sp->args[0];
> + int hsp_type = (mbox_id >> 16) & 0xf;
Wouldn't it be nicer if the shift and mask constants are made defines in
the DT bindings header (tegra186-hsp.h)?
> + int master_id = mbox_id & 0xff;
> + struct tegra_hsp_mbox *hsp_mbox = dev_get_drvdata(mbox->dev);
> + struct mbox_chan *free_chan;
> + int i, ret = 0;
> +
> + spin_lock(&hsp_mbox->lock);
If you must use spin locks, you will have to use the irqsave/restore
veriants in this function (called from thread context).
> +
> + for (i = 0; i < mbox->num_chans; i++) {
> + free_chan = &mbox->chans[i];
> + if (!free_chan->con_priv)
> + break;
> + free_chan = NULL;
> + }
> +
> + if (!free_chan) {
> + spin_unlock(&hsp_mbox->lock);
> + return ERR_PTR(-EFAULT);
> + }
IMO, it will be cleaner & simpler if you move the above code (doing the
lookup) into a separate function that returns free_chan - and you can
reuse that in hsp_db_irq()
> +
> + switch (hsp_type) {
> + case HSP_MBOX_TYPE_DB:
> + ret = tegra_hsp_db_init(hsp_mbox, free_chan, master_id);
tegra_hsp_db_init() uses devm_kzalloc and you are doing this holding a
spinlock.
> + break;
> + default:
Not returning error here will also cause resource leak (free_chan).
> + break;
> + }
> +
> + spin_unlock(&hsp_mbox->lock);
> +
> + if (ret)
> + free_chan = ERR_PTR(-EFAULT);
> +
> + return free_chan;
> +}
> +
> +static int tegra_hsp_probe(struct platform_device *pdev)
> +{
> + struct tegra_hsp_mbox *hsp_mbox;
> + struct resource *res;
> + int ret = 0;
> + u32 reg;
> +
> + hsp_mbox = devm_kzalloc(&pdev->dev, sizeof(*hsp_mbox), GFP_KERNEL);
> + if (!hsp_mbox)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + hsp_mbox->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(hsp_mbox->base))
> + return PTR_ERR(hsp_mbox->base);
> +
> + reg = hsp_readl(hsp_mbox->base, HSP_INT_DIMENSIONING);
> + hsp_mbox->nr_sm = (reg >> HSP_nSM_OFFSET) & HSP_nINT_MASK;
> + hsp_mbox->nr_ss = (reg >> HSP_nSS_OFFSET) & HSP_nINT_MASK;
> + hsp_mbox->nr_as = (reg >> HSP_nAS_OFFSET) & HSP_nINT_MASK;
> + hsp_mbox->nr_db = (reg >> HSP_nDB_OFFSET) & HSP_nINT_MASK;
> + hsp_mbox->nr_si = (reg >> HSP_nSI_OFFSET) & HSP_nINT_MASK;
> +
> + hsp_mbox->mbox = devm_kzalloc(&pdev->dev,
> + sizeof(*hsp_mbox->mbox), GFP_KERNEL);
> + if (!hsp_mbox->mbox)
> + return -ENOMEM;
> +
> + hsp_mbox->mbox->chans =
> + devm_kcalloc(&pdev->dev, MAX_NUM_HSP_CHAN,
> + sizeof(*hsp_mbox->mbox->chans), GFP_KERNEL);
> + if (!hsp_mbox->mbox->chans)
> + return -ENOMEM;
> +
> + hsp_mbox->mbox->of_xlate = of_hsp_mbox_xlate;
> + hsp_mbox->mbox->num_chans = MAX_NUM_HSP_CHAN;
> + hsp_mbox->mbox->dev = &pdev->dev;
> + hsp_mbox->mbox->txdone_irq = false;
> + hsp_mbox->mbox->txdone_poll = false;
> + hsp_mbox->mbox->ops = &tegra_hsp_ops;
> + platform_set_drvdata(pdev, hsp_mbox);
> +
> + ret = mbox_controller_register(hsp_mbox->mbox);
> + if (ret) {
> + pr_err("tegra-hsp mbox: fail to register mailbox %d.\n", ret);
> + return ret;
> + }
> +
> + spin_lock_init(&hsp_mbox->lock);
> +
> + return 0;
> +}
> +
> +static int tegra_hsp_remove(struct platform_device *pdev)
> +{
> + struct tegra_hsp_mbox *hsp_mbox = platform_get_drvdata(pdev);
> +
> + if (hsp_mbox->mbox)
> + mbox_controller_unregister(hsp_mbox->mbox);
> +
> + return 0;
> +}
> +
> +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.9.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[toc] | [prev] | [next] | [standalone]
| From | Joseph Lo <josephl@nvidia.com> |
|---|---|
| Date | 2016-07-05 11:10 +0200 |
| Subject | [PATCH V2 07/10] arm64: defconfig: Enable Tegra186 SoC |
| Message-ID | <rRuwG-MJ-31@gated-at.bofh.it> |
| In reply to | #1436857 |
Enable Tegra186 SoC. Signed-off-by: Joseph Lo <josephl@nvidia.com> --- Changes in V2: - None --- arch/arm64/configs/defconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig index e69051098435..64d767ec142c 100644 --- a/arch/arm64/configs/defconfig +++ b/arch/arm64/configs/defconfig @@ -326,6 +326,7 @@ CONFIG_QCOM_SMEM=y CONFIG_QCOM_SMD=y CONFIG_QCOM_SMD_RPM=y CONFIG_ARCH_TEGRA_132_SOC=y +CONFIG_ARCH_TEGRA_186_SOC=y CONFIG_ARCH_TEGRA_210_SOC=y CONFIG_EXTCON_USB_GPIO=y CONFIG_PWM=y -- 2.9.0
[toc] | [prev] | [next] | [standalone]
| From | Joseph Lo <josephl@nvidia.com> |
|---|---|
| Date | 2016-07-05 11:10 +0200 |
| Subject | [PATCH V2 09/10] arm64: dts: tegra: Add NVIDIA Tegra186 P3310 main board support |
| Message-ID | <rRuwG-MJ-21@gated-at.bofh.it> |
| In reply to | #1436857 |
Add NVIDIA Tegra186 P3310 main board support, which is a chip module
with DRAM, nonvolatile storage, WiFi, ethernet and PMIC chips on it. It
also needs an IO board and hooks on it to represent as an application
platform.
Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
Changes in V2:
- update according to the binding fix in V2
---
arch/arm64/boot/dts/nvidia/tegra186-p3310.dtsi | 34 ++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
create mode 100644 arch/arm64/boot/dts/nvidia/tegra186-p3310.dtsi
diff --git a/arch/arm64/boot/dts/nvidia/tegra186-p3310.dtsi b/arch/arm64/boot/dts/nvidia/tegra186-p3310.dtsi
new file mode 100644
index 000000000000..f5238866d321
--- /dev/null
+++ b/arch/arm64/boot/dts/nvidia/tegra186-p3310.dtsi
@@ -0,0 +1,34 @@
+#include "tegra186.dtsi"
+
+/ {
+ model = "NVIDIA Tegra186 P3310 main Board";
+ compatible = "nvidia,p3301", "nvidia,tegra186";
+
+ aliases {
+ serial0 = &uarta;
+ };
+
+ chosen {
+ bootargs = "earlycon console=ttyS0,115200n8";
+ stdout-path = "serial0:115200n8";
+ };
+
+ memory {
+ device_type = "memory";
+ reg = <0x0 0x80000000 0x2 0x00000000>;
+ };
+
+ serial@03100000 {
+ // HACK: before clk driver ready
+ clock-frequency = <408000000>;
+ status = "okay";
+ };
+
+ hsp@03c00000 {
+ status = "okay";
+ };
+
+ bpmp {
+ status = "okay";
+ };
+};
--
2.9.0
[toc] | [prev] | [next] | [standalone]
| From | Joseph Lo <josephl@nvidia.com> |
|---|---|
| Date | 2016-07-05 11:10 +0200 |
| Subject | [PATCH V2 10/10] arm64: dts: tegra: Add NVIDIA P2771 board support |
| Message-ID | <rRuwG-MJ-33@gated-at.bofh.it> |
| In reply to | #1436857 |
Add NVIDIA Tegra186 P2771 board support, which is a reference
development board with P2597 I/O board and P3310 chip module on it.
Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
Changes in V2:
- None
---
arch/arm64/boot/dts/nvidia/Makefile | 1 +
arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dts | 8 ++++++++
2 files changed, 9 insertions(+)
create mode 100644 arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dts
diff --git a/arch/arm64/boot/dts/nvidia/Makefile b/arch/arm64/boot/dts/nvidia/Makefile
index 0f7cdf3e05c1..67234f3dc795 100644
--- a/arch/arm64/boot/dts/nvidia/Makefile
+++ b/arch/arm64/boot/dts/nvidia/Makefile
@@ -1,4 +1,5 @@
dtb-$(CONFIG_ARCH_TEGRA_132_SOC) += tegra132-norrin.dtb
+dtb-$(CONFIG_ARCH_TEGRA_186_SOC) += tegra186-p2771-0000.dtb
dtb-$(CONFIG_ARCH_TEGRA_210_SOC) += tegra210-p2371-0000.dtb
dtb-$(CONFIG_ARCH_TEGRA_210_SOC) += tegra210-p2371-2180.dtb
dtb-$(CONFIG_ARCH_TEGRA_210_SOC) += tegra210-p2571.dtb
diff --git a/arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dts b/arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dts
new file mode 100644
index 000000000000..66b936389fa7
--- /dev/null
+++ b/arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dts
@@ -0,0 +1,8 @@
+/dts-v1/;
+
+#include "tegra186-p3310.dtsi"
+
+/ {
+ model = "NVIDIA Tegra186 P2771-0000 Board";
+ compatible = "nvidia,p2771-0000", "nvidia,tegra186";
+};
--
2.9.0
[toc] | [prev] | [next] | [standalone]
| From | Joseph Lo <josephl@nvidia.com> |
|---|---|
| Date | 2016-07-05 11:10 +0200 |
| Subject | [PATCH V2 04/10] firmware: tegra: add IVC library |
| Message-ID | <rRuwG-MJ-37@gated-at.bofh.it> |
| In reply to | #1436857 |
The Inter-VM communication (IVC) is a communication protocol, which is
designed for interprocessor communication (IPC) or the communication
between the hypervisor and the virtual machine with a guest OS on it. So
it can be translated as inter-virtual memory or inter-virtual machine
communication. The message channels are maintained on the DRAM or SRAM
and the data coherency should be considered. Or the data could be
corrupted or out of date when the remote client checking it.
Inside the IVC, it maintains memory-based descriptors for the TX/RX
channels and the coherency issue of the counter and payloads. So the
clients can use it to send/receive messages to/from remote ones.
We introduce it as a library for the firmware drivers, which can use it
for IPC.
Based-on-the-work-by:
Peter Newman <pnewman@nvidia.com>
Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
Changes in V2:
- None
---
drivers/firmware/Kconfig | 1 +
drivers/firmware/Makefile | 1 +
drivers/firmware/tegra/Kconfig | 13 +
drivers/firmware/tegra/Makefile | 1 +
drivers/firmware/tegra/ivc.c | 659 ++++++++++++++++++++++++++++++++++++++++
include/soc/tegra/ivc.h | 102 +++++++
6 files changed, 777 insertions(+)
create mode 100644 drivers/firmware/tegra/Kconfig
create mode 100644 drivers/firmware/tegra/Makefile
create mode 100644 drivers/firmware/tegra/ivc.c
create mode 100644 include/soc/tegra/ivc.h
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 5e618058defe..bbd64ae8c4c6 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -200,5 +200,6 @@ config HAVE_ARM_SMCCC
source "drivers/firmware/broadcom/Kconfig"
source "drivers/firmware/google/Kconfig"
source "drivers/firmware/efi/Kconfig"
+source "drivers/firmware/tegra/Kconfig"
endmenu
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 474bada56fcd..9a4df8171cc4 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -24,3 +24,4 @@ obj-y += broadcom/
obj-$(CONFIG_GOOGLE_FIRMWARE) += google/
obj-$(CONFIG_EFI) += efi/
obj-$(CONFIG_UEFI_CPER) += efi/
+obj-y += tegra/
diff --git a/drivers/firmware/tegra/Kconfig b/drivers/firmware/tegra/Kconfig
new file mode 100644
index 000000000000..1fa3e4e136a5
--- /dev/null
+++ b/drivers/firmware/tegra/Kconfig
@@ -0,0 +1,13 @@
+menu "Tegra firmware driver"
+
+config TEGRA_IVC
+ bool "Tegra IVC protocol"
+ depends on ARCH_TEGRA
+ help
+ IVC (Inter-VM Communication) protocol is part of the IPC
+ (Inter Processor Communication) framework on Tegra. It maintains the
+ data and the different commuication channels in SysRAM or RAM and
+ keeps the content is synchronization between host CPU and remote
+ processors.
+
+endmenu
diff --git a/drivers/firmware/tegra/Makefile b/drivers/firmware/tegra/Makefile
new file mode 100644
index 000000000000..92e2153e8173
--- /dev/null
+++ b/drivers/firmware/tegra/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_TEGRA_IVC) += ivc.o
diff --git a/drivers/firmware/tegra/ivc.c b/drivers/firmware/tegra/ivc.c
new file mode 100644
index 000000000000..3e736bb9915a
--- /dev/null
+++ b/drivers/firmware/tegra/ivc.c
@@ -0,0 +1,659 @@
+/*
+ * Copyright (c) 2014-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/module.h>
+
+#include <soc/tegra/ivc.h>
+
+#define IVC_ALIGN 64
+
+#ifdef CONFIG_SMP
+
+static inline void ivc_rmb(void)
+{
+ smp_rmb();
+}
+
+static inline void ivc_wmb(void)
+{
+ smp_wmb();
+}
+
+static inline void ivc_mb(void)
+{
+ smp_mb();
+}
+
+#else
+
+static inline void ivc_rmb(void)
+{
+ rmb();
+}
+
+static inline void ivc_wmb(void)
+{
+ wmb();
+}
+
+static inline void ivc_mb(void)
+{
+ mb();
+}
+
+#endif
+
+/*
+ * IVC channel reset protocol.
+ *
+ * Each end uses its tx_channel.state to indicate its synchronization state.
+ */
+enum ivc_state {
+ /*
+ * This value is zero for backwards compatibility with services that
+ * assume channels to be initially zeroed. Such channels are in an
+ * initially valid state, but cannot be asynchronously reset, and must
+ * maintain a valid state at all times.
+ *
+ * The transmitting end can enter the established state from the sync or
+ * ack state when it observes the receiving endpoint in the ack or
+ * established state, indicating that has cleared the counters in our
+ * rx_channel.
+ */
+ ivc_state_established = 0,
+
+ /*
+ * If an endpoint is observed in the sync state, the remote endpoint is
+ * allowed to clear the counters it owns asynchronously with respect to
+ * the current endpoint. Therefore, the current endpoint is no longer
+ * allowed to communicate.
+ */
+ ivc_state_sync,
+
+ /*
+ * When the transmitting end observes the receiving end in the sync
+ * state, it can clear the w_count and r_count and transition to the ack
+ * state. If the remote endpoint observes us in the ack state, it can
+ * return to the established state once it has cleared its counters.
+ */
+ ivc_state_ack
+};
+
+/*
+ * This structure is divided into two-cache aligned parts, the first is only
+ * written through the tx_channel pointer, while the second is only written
+ * through the rx_channel pointer. This delineates ownership of the cache lines,
+ * which is critical to performance and necessary in non-cache coherent
+ * implementations.
+ */
+struct ivc_channel_header {
+ union {
+ struct {
+ /* fields owned by the transmitting end */
+ uint32_t w_count;
+ uint32_t state;
+ };
+ uint8_t w_align[IVC_ALIGN];
+ };
+ union {
+ /* fields owned by the receiving end */
+ uint32_t r_count;
+ uint8_t r_align[IVC_ALIGN];
+ };
+};
+
+static inline void ivc_invalidate_counter(struct ivc *ivc,
+ dma_addr_t handle)
+{
+ if (!ivc->peer_device)
+ return;
+ dma_sync_single_for_cpu(ivc->peer_device, handle, IVC_ALIGN,
+ DMA_FROM_DEVICE);
+}
+
+static inline void ivc_flush_counter(struct ivc *ivc, dma_addr_t handle)
+{
+ if (!ivc->peer_device)
+ return;
+ dma_sync_single_for_device(ivc->peer_device, handle, IVC_ALIGN,
+ DMA_TO_DEVICE);
+}
+
+static inline int ivc_channel_empty(struct ivc *ivc,
+ struct ivc_channel_header *ch)
+{
+ /*
+ * This function performs multiple checks on the same values with
+ * security implications, so create snapshots with ACCESS_ONCE() to
+ * ensure that these checks use the same values.
+ */
+ uint32_t w_count = ACCESS_ONCE(ch->w_count);
+ uint32_t r_count = ACCESS_ONCE(ch->r_count);
+
+ /*
+ * Perform an over-full check to prevent denial of service attacks where
+ * a server could be easily fooled into believing that there's an
+ * extremely large number of frames ready, since receivers are not
+ * expected to check for full or over-full conditions.
+ *
+ * Although the channel isn't empty, this is an invalid case caused by
+ * a potentially malicious peer, so returning empty is safer, because it
+ * gives the impression that the channel has gone silent.
+ */
+ if (w_count - r_count > ivc->nframes)
+ return 1;
+
+ return w_count == r_count;
+}
+
+static inline int ivc_channel_full(struct ivc *ivc,
+ struct ivc_channel_header *ch)
+{
+ /*
+ * Invalid cases where the counters indicate that the queue is over
+ * capacity also appear full.
+ */
+ return ACCESS_ONCE(ch->w_count) - ACCESS_ONCE(ch->r_count)
+ >= ivc->nframes;
+}
+
+static inline uint32_t ivc_channel_avail_count(struct ivc *ivc,
+ struct ivc_channel_header *ch)
+{
+ /*
+ * This function isn't expected to be used in scenarios where an
+ * over-full situation can lead to denial of service attacks. See the
+ * comment in ivc_channel_empty() for an explanation about special
+ * over-full considerations.
+ */
+ return ACCESS_ONCE(ch->w_count) - ACCESS_ONCE(ch->r_count);
+}
+
+static inline void ivc_advance_tx(struct ivc *ivc)
+{
+ ACCESS_ONCE(ivc->tx_channel->w_count) =
+ ACCESS_ONCE(ivc->tx_channel->w_count) + 1;
+
+ if (ivc->w_pos == ivc->nframes - 1)
+ ivc->w_pos = 0;
+ else
+ ivc->w_pos++;
+}
+
+static inline void ivc_advance_rx(struct ivc *ivc)
+{
+ ACCESS_ONCE(ivc->rx_channel->r_count) =
+ ACCESS_ONCE(ivc->rx_channel->r_count) + 1;
+
+ if (ivc->r_pos == ivc->nframes - 1)
+ ivc->r_pos = 0;
+ else
+ ivc->r_pos++;
+}
+
+static inline int ivc_check_read(struct ivc *ivc)
+{
+ /*
+ * tx_channel->state is set locally, so it is not synchronized with
+ * state from the remote peer. The remote peer cannot reset its
+ * transmit counters until we've acknowledged its synchronization
+ * request, so no additional synchronization is required because an
+ * asynchronous transition of rx_channel->state to ivc_state_ack is not
+ * allowed.
+ */
+ if (ivc->tx_channel->state != ivc_state_established)
+ return -ECONNRESET;
+
+ /*
+ * Avoid unnecessary invalidations when performing repeated accesses to
+ * an IVC channel by checking the old queue pointers first.
+ * Synchronization is only necessary when these pointers indicate empty
+ * or full.
+ */
+ if (!ivc_channel_empty(ivc, ivc->rx_channel))
+ return 0;
+
+ ivc_invalidate_counter(ivc, ivc->rx_handle +
+ offsetof(struct ivc_channel_header, w_count));
+ return ivc_channel_empty(ivc, ivc->rx_channel) ? -ENOMEM : 0;
+}
+
+static inline int ivc_check_write(struct ivc *ivc)
+{
+ if (ivc->tx_channel->state != ivc_state_established)
+ return -ECONNRESET;
+
+ if (!ivc_channel_full(ivc, ivc->tx_channel))
+ return 0;
+
+ ivc_invalidate_counter(ivc, ivc->tx_handle +
+ offsetof(struct ivc_channel_header, r_count));
+ return ivc_channel_full(ivc, ivc->tx_channel) ? -ENOMEM : 0;
+}
+
+static void *ivc_frame_pointer(struct ivc *ivc, struct ivc_channel_header *ch,
+ uint32_t frame)
+{
+ BUG_ON(frame >= ivc->nframes);
+ return (void *)((uintptr_t)(ch + 1) + ivc->frame_size * frame);
+}
+
+static inline dma_addr_t ivc_frame_handle(struct ivc *ivc,
+ dma_addr_t channel_handle, uint32_t frame)
+{
+ BUG_ON(!ivc->peer_device);
+ BUG_ON(frame >= ivc->nframes);
+ return channel_handle + sizeof(struct ivc_channel_header) +
+ ivc->frame_size * frame;
+}
+
+static inline void ivc_invalidate_frame(struct ivc *ivc,
+ dma_addr_t channel_handle, unsigned frame, int offset, int len)
+{
+ if (!ivc->peer_device)
+ return;
+ dma_sync_single_for_cpu(ivc->peer_device,
+ ivc_frame_handle(ivc, channel_handle, frame) + offset,
+ len, DMA_FROM_DEVICE);
+}
+
+static inline void ivc_flush_frame(struct ivc *ivc, dma_addr_t channel_handle,
+ unsigned frame, int offset, int len)
+{
+ if (!ivc->peer_device)
+ return;
+ dma_sync_single_for_device(ivc->peer_device,
+ ivc_frame_handle(ivc, channel_handle, frame) + offset,
+ len, DMA_TO_DEVICE);
+}
+
+/* directly peek at the next frame rx'ed */
+void *tegra_ivc_read_get_next_frame(struct ivc *ivc)
+{
+ int result = ivc_check_read(ivc);
+ if (result)
+ return ERR_PTR(result);
+
+ /*
+ * Order observation of w_pos potentially indicating new data before
+ * data read.
+ */
+ ivc_rmb();
+
+ ivc_invalidate_frame(ivc, ivc->rx_handle, ivc->r_pos, 0,
+ ivc->frame_size);
+ return ivc_frame_pointer(ivc, ivc->rx_channel, ivc->r_pos);
+}
+EXPORT_SYMBOL(tegra_ivc_read_get_next_frame);
+
+int tegra_ivc_read_advance(struct ivc *ivc)
+{
+ /*
+ * No read barriers or synchronization here: the caller is expected to
+ * have already observed the channel non-empty. This check is just to
+ * catch programming errors.
+ */
+ int result = ivc_check_read(ivc);
+ if (result)
+ return result;
+
+ ivc_advance_rx(ivc);
+ ivc_flush_counter(ivc, ivc->rx_handle +
+ offsetof(struct ivc_channel_header, r_count));
+
+ /*
+ * Ensure our write to r_pos occurs before our read from w_pos.
+ */
+ ivc_mb();
+
+ /*
+ * Notify only upon transition from full to non-full.
+ * The available count can only asynchronously increase, so the
+ * worst possible side-effect will be a spurious notification.
+ */
+ ivc_invalidate_counter(ivc, ivc->rx_handle +
+ offsetof(struct ivc_channel_header, w_count));
+
+ if (ivc_channel_avail_count(ivc, ivc->rx_channel) == ivc->nframes - 1)
+ ivc->notify(ivc);
+
+ return 0;
+}
+EXPORT_SYMBOL(tegra_ivc_read_advance);
+
+/* directly poke at the next frame to be tx'ed */
+void *tegra_ivc_write_get_next_frame(struct ivc *ivc)
+{
+ int result = ivc_check_write(ivc);
+ if (result)
+ return ERR_PTR(result);
+
+ return ivc_frame_pointer(ivc, ivc->tx_channel, ivc->w_pos);
+}
+EXPORT_SYMBOL(tegra_ivc_write_get_next_frame);
+
+/* advance the tx buffer */
+int tegra_ivc_write_advance(struct ivc *ivc)
+{
+ int result = ivc_check_write(ivc);
+ if (result)
+ return result;
+
+ ivc_flush_frame(ivc, ivc->tx_handle, ivc->w_pos, 0, ivc->frame_size);
+
+ /*
+ * Order any possible stores to the frame before update of w_pos.
+ */
+ ivc_wmb();
+
+ ivc_advance_tx(ivc);
+ ivc_flush_counter(ivc, ivc->tx_handle +
+ offsetof(struct ivc_channel_header, w_count));
+
+ /*
+ * Ensure our write to w_pos occurs before our read from r_pos.
+ */
+ ivc_mb();
+
+ /*
+ * Notify only upon transition from empty to non-empty.
+ * The available count can only asynchronously decrease, so the
+ * worst possible side-effect will be a spurious notification.
+ */
+ ivc_invalidate_counter(ivc, ivc->tx_handle +
+ offsetof(struct ivc_channel_header, r_count));
+
+ if (ivc_channel_avail_count(ivc, ivc->tx_channel) == 1)
+ ivc->notify(ivc);
+
+ return 0;
+}
+EXPORT_SYMBOL(tegra_ivc_write_advance);
+
+void tegra_ivc_channel_reset(struct ivc *ivc)
+{
+ ivc->tx_channel->state = ivc_state_sync;
+ ivc_flush_counter(ivc, ivc->tx_handle +
+ offsetof(struct ivc_channel_header, w_count));
+ ivc->notify(ivc);
+}
+EXPORT_SYMBOL(tegra_ivc_channel_reset);
+
+/*
+ * ===============================================================
+ * IVC State Transition Table - see tegra_ivc_channel_notified()
+ * ===============================================================
+ *
+ * local remote action
+ * ----- ------ -----------------------------------
+ * SYNC EST <none>
+ * SYNC ACK reset counters; move to EST; notify
+ * SYNC SYNC reset counters; move to ACK; notify
+ * ACK EST move to EST; notify
+ * ACK ACK move to EST; notify
+ * ACK SYNC reset counters; move to ACK; notify
+ * EST EST <none>
+ * EST ACK <none>
+ * EST SYNC reset counters; move to ACK; notify
+ *
+ * ===============================================================
+ */
+
+int tegra_ivc_channel_notified(struct ivc *ivc)
+{
+ enum ivc_state peer_state;
+
+ /* Copy the receiver's state out of shared memory. */
+ ivc_invalidate_counter(ivc, ivc->rx_handle +
+ offsetof(struct ivc_channel_header, w_count));
+ peer_state = ACCESS_ONCE(ivc->rx_channel->state);
+
+ if (peer_state == ivc_state_sync) {
+ /*
+ * Order observation of ivc_state_sync before stores clearing
+ * tx_channel.
+ */
+ ivc_rmb();
+
+ /*
+ * Reset tx_channel counters. The remote end is in the SYNC
+ * state and won't make progress until we change our state,
+ * so the counters are not in use at this time.
+ */
+ ivc->tx_channel->w_count = 0;
+ ivc->rx_channel->r_count = 0;
+
+ ivc->w_pos = 0;
+ ivc->r_pos = 0;
+
+ /*
+ * Ensure that counters appear cleared before new state can be
+ * observed.
+ */
+ ivc_wmb();
+
+ /*
+ * Move to ACK state. We have just cleared our counters, so it
+ * is now safe for the remote end to start using these values.
+ */
+ ivc->tx_channel->state = ivc_state_ack;
+ ivc_flush_counter(ivc, ivc->tx_handle +
+ offsetof(struct ivc_channel_header, w_count));
+
+ /*
+ * Notify remote end to observe state transition.
+ */
+ ivc->notify(ivc);
+
+ } else if (ivc->tx_channel->state == ivc_state_sync &&
+ peer_state == ivc_state_ack) {
+ /*
+ * Order observation of ivc_state_sync before stores clearing
+ * tx_channel.
+ */
+ ivc_rmb();
+
+ /*
+ * Reset tx_channel counters. The remote end is in the ACK
+ * state and won't make progress until we change our state,
+ * so the counters are not in use at this time.
+ */
+ ivc->tx_channel->w_count = 0;
+ ivc->rx_channel->r_count = 0;
+
+ ivc->w_pos = 0;
+ ivc->r_pos = 0;
+
+ /*
+ * Ensure that counters appear cleared before new state can be
+ * observed.
+ */
+ ivc_wmb();
+
+ /*
+ * Move to ESTABLISHED state. We know that the remote end has
+ * already cleared its counters, so it is safe to start
+ * writing/reading on this channel.
+ */
+ ivc->tx_channel->state = ivc_state_established;
+ ivc_flush_counter(ivc, ivc->tx_handle +
+ offsetof(struct ivc_channel_header, w_count));
+
+ /*
+ * Notify remote end to observe state transition.
+ */
+ ivc->notify(ivc);
+
+ } else if (ivc->tx_channel->state == ivc_state_ack) {
+ /*
+ * At this point, we have observed the peer to be in either
+ * the ACK or ESTABLISHED state. Next, order observation of
+ * peer state before storing to tx_channel.
+ */
+ ivc_rmb();
+
+ /*
+ * Move to ESTABLISHED state. We know that we have previously
+ * cleared our counters, and we know that the remote end has
+ * cleared its counters, so it is safe to start writing/reading
+ * on this channel.
+ */
+ ivc->tx_channel->state = ivc_state_established;
+ ivc_flush_counter(ivc, ivc->tx_handle +
+ offsetof(struct ivc_channel_header, w_count));
+
+ /*
+ * Notify remote end to observe state transition.
+ */
+ ivc->notify(ivc);
+
+ } else {
+ /*
+ * There is no need to handle any further action. Either the
+ * channel is already fully established, or we are waiting for
+ * the remote end to catch up with our current state. Refer
+ * to the diagram in "IVC State Transition Table" above.
+ */
+ }
+
+ return ivc->tx_channel->state == ivc_state_established ? 0 : -EAGAIN;
+}
+EXPORT_SYMBOL(tegra_ivc_channel_notified);
+
+size_t tegra_ivc_align(size_t size)
+{
+ return (size + (IVC_ALIGN - 1)) & ~(IVC_ALIGN - 1);
+}
+EXPORT_SYMBOL(tegra_ivc_align);
+
+unsigned tegra_ivc_total_queue_size(unsigned queue_size)
+{
+ if (queue_size & (IVC_ALIGN - 1)) {
+ pr_err("%s: queue_size (%u) must be %u-byte aligned\n",
+ __func__, queue_size, IVC_ALIGN);
+ return 0;
+ }
+ return queue_size + sizeof(struct ivc_channel_header);
+}
+EXPORT_SYMBOL(tegra_ivc_total_queue_size);
+
+static int check_ivc_params(uintptr_t queue_base1, uintptr_t queue_base2,
+ unsigned nframes, unsigned frame_size)
+{
+ BUG_ON(offsetof(struct ivc_channel_header, w_count) & (IVC_ALIGN - 1));
+ BUG_ON(offsetof(struct ivc_channel_header, r_count) & (IVC_ALIGN - 1));
+ BUG_ON(sizeof(struct ivc_channel_header) & (IVC_ALIGN - 1));
+
+ if ((uint64_t)nframes * (uint64_t)frame_size >= 0x100000000) {
+ pr_err("nframes * frame_size overflows\n");
+ return -EINVAL;
+ }
+
+ /*
+ * The headers must at least be aligned enough for counters
+ * to be accessed atomically.
+ */
+ if (queue_base1 & (IVC_ALIGN - 1)) {
+ pr_err("ivc channel start not aligned: %lx\n", queue_base1);
+ return -EINVAL;
+ }
+ if (queue_base2 & (IVC_ALIGN - 1)) {
+ pr_err("ivc channel start not aligned: %lx\n", queue_base2);
+ return -EINVAL;
+ }
+
+ if (frame_size & (IVC_ALIGN - 1)) {
+ pr_err("frame size not adequately aligned: %u\n", frame_size);
+ return -EINVAL;
+ }
+
+ if (queue_base1 < queue_base2) {
+ if (queue_base1 + frame_size * nframes > queue_base2) {
+ pr_err("queue regions overlap: %lx + %x, %x\n",
+ queue_base1, frame_size,
+ frame_size * nframes);
+ return -EINVAL;
+ }
+ } else {
+ if (queue_base2 + frame_size * nframes > queue_base1) {
+ pr_err("queue regions overlap: %lx + %x, %x\n",
+ queue_base2, frame_size,
+ frame_size * nframes);
+ return -EINVAL;
+ }
+ }
+
+ return 0;
+}
+
+int tegra_ivc_init(struct ivc *ivc, uintptr_t rx_base, dma_addr_t rx_handle,
+ uintptr_t tx_base, dma_addr_t tx_handle, unsigned nframes,
+ unsigned frame_size, struct device *peer_device,
+ void (*notify)(struct ivc *))
+{
+ size_t queue_size;
+
+ int result = check_ivc_params(rx_base, tx_base, nframes, frame_size);
+ if (result)
+ return result;
+
+ BUG_ON(!ivc);
+ BUG_ON(!notify);
+
+ queue_size = tegra_ivc_total_queue_size(nframes * frame_size);
+
+ /*
+ * All sizes that can be returned by communication functions should
+ * fit in an int.
+ */
+ if (frame_size > INT_MAX)
+ return -E2BIG;
+
+ ivc->rx_channel = (struct ivc_channel_header *)rx_base;
+ ivc->tx_channel = (struct ivc_channel_header *)tx_base;
+
+ if (peer_device) {
+ if (rx_handle != DMA_ERROR_CODE) {
+ ivc->rx_handle = rx_handle;
+ ivc->tx_handle = tx_handle;
+ } else {
+ ivc->rx_handle = dma_map_single(peer_device,
+ ivc->rx_channel, queue_size, DMA_BIDIRECTIONAL);
+ if (ivc->rx_handle == DMA_ERROR_CODE)
+ return -ENOMEM;
+
+ ivc->tx_handle = dma_map_single(peer_device,
+ ivc->tx_channel, queue_size, DMA_BIDIRECTIONAL);
+ if (ivc->tx_handle == DMA_ERROR_CODE) {
+ dma_unmap_single(peer_device, ivc->rx_handle,
+ queue_size, DMA_BIDIRECTIONAL);
+ return -ENOMEM;
+ }
+ }
+ }
+
+ ivc->notify = notify;
+ ivc->frame_size = frame_size;
+ ivc->nframes = nframes;
+ ivc->peer_device = peer_device;
+
+ /*
+ * These values aren't necessarily correct until the channel has been
+ * reset.
+ */
+ ivc->w_pos = 0;
+ ivc->r_pos = 0;
+
+ return 0;
+}
+EXPORT_SYMBOL(tegra_ivc_init);
diff --git a/include/soc/tegra/ivc.h b/include/soc/tegra/ivc.h
new file mode 100644
index 000000000000..1762fbee3fa2
--- /dev/null
+++ b/include/soc/tegra/ivc.h
@@ -0,0 +1,102 @@
+/*
+ * 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.
+ */
+
+#ifndef __TEGRA_IVC_H
+
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/types.h>
+
+struct ivc_channel_header;
+
+struct ivc {
+ struct ivc_channel_header *rx_channel, *tx_channel;
+ uint32_t w_pos, r_pos;
+
+ void (*notify)(struct ivc *);
+ uint32_t nframes, frame_size;
+
+ struct device *peer_device;
+ dma_addr_t rx_handle, tx_handle;
+};
+
+/**
+ * tegra_ivc_read_get_next_frame - Peek at the next frame to receive
+ * @ivc pointer of the IVC channel
+ *
+ * Peek at the next frame to be received, without removing it from
+ * the queue.
+ *
+ * Returns a pointer to the frame, or an error encoded pointer.
+ */
+void *tegra_ivc_read_get_next_frame(struct ivc *ivc);
+
+/**
+ * tegra_ivc_read_advance - Advance the read queue
+ * @ivc pointer of the IVC channel
+ *
+ * Advance the read queue
+ *
+ * Returns 0, or a negative error value if failed.
+ */
+int tegra_ivc_read_advance(struct ivc *ivc);
+
+/**
+ * tegra_ivc_write_get_next_frame - Poke at the next frame to transmit
+ * @ivc pointer of the IVC channel
+ *
+ * Get access to the next frame.
+ *
+ * Returns a pointer to the frame, or an error encoded pointer.
+ */
+void *tegra_ivc_write_get_next_frame(struct ivc *ivc);
+
+/**
+ * tegra_ivc_write_advance - Advance the write queue
+ * @ivc pointer of the IVC channel
+ *
+ * Advance the write queue
+ *
+ * Returns 0, or a negative error value if failed.
+ */
+int tegra_ivc_write_advance(struct ivc *ivc);
+
+/**
+ * tegra_ivc_channel_notified - handle internal messages
+ * @ivc pointer of the IVC channel
+ *
+ * This function must be called following every notification.
+ *
+ * Returns 0 if the channel is ready for communication, or -EAGAIN if a channel
+ * reset is in progress.
+ */
+int tegra_ivc_channel_notified(struct ivc *ivc);
+
+/**
+ * tegra_ivc_channel_reset - initiates a reset of the shared memory state
+ * @ivc pointer of the IVC channel
+ *
+ * This function must be called after a channel is reserved before it is used
+ * for communication. The channel will be ready for use when a subsequent call
+ * to notify the remote of the channel reset.
+ */
+void tegra_ivc_channel_reset(struct ivc *ivc);
+
+size_t tegra_ivc_align(size_t size);
+unsigned tegra_ivc_total_queue_size(unsigned queue_size);
+int tegra_ivc_init(struct ivc *ivc, uintptr_t rx_base, dma_addr_t rx_handle,
+ uintptr_t tx_base, dma_addr_t tx_handle, unsigned nframes,
+ unsigned frame_size, struct device *peer_device,
+ void (*notify)(struct ivc *));
+
+#endif /* __TEGRA_IVC_H */
--
2.9.0
[toc] | [prev] | [next] | [standalone]
| From | Alexandre Courbot <gnurou@gmail.com> |
|---|---|
| Date | 2016-07-07 13:20 +0200 |
| Subject | Re: [PATCH V2 04/10] firmware: tegra: add IVC library |
| Message-ID | <rSfvz-69Q-1@gated-at.bofh.it> |
| In reply to | #1436863 |
On Tue, Jul 5, 2016 at 6:04 PM, Joseph Lo <josephl@nvidia.com> wrote:
> The Inter-VM communication (IVC) is a communication protocol, which is
> designed for interprocessor communication (IPC) or the communication
> between the hypervisor and the virtual machine with a guest OS on it. So
> it can be translated as inter-virtual memory or inter-virtual machine
> communication. The message channels are maintained on the DRAM or SRAM
> and the data coherency should be considered. Or the data could be
> corrupted or out of date when the remote client checking it.
>
> Inside the IVC, it maintains memory-based descriptors for the TX/RX
> channels and the coherency issue of the counter and payloads. So the
> clients can use it to send/receive messages to/from remote ones.
>
> We introduce it as a library for the firmware drivers, which can use it
> for IPC.
>
> Based-on-the-work-by:
> Peter Newman <pnewman@nvidia.com>
>
> Signed-off-by: Joseph Lo <josephl@nvidia.com>
> ---
> Changes in V2:
> - None
> ---
> drivers/firmware/Kconfig | 1 +
> drivers/firmware/Makefile | 1 +
> drivers/firmware/tegra/Kconfig | 13 +
> drivers/firmware/tegra/Makefile | 1 +
> drivers/firmware/tegra/ivc.c | 659 ++++++++++++++++++++++++++++++++++++++++
> include/soc/tegra/ivc.h | 102 +++++++
> 6 files changed, 777 insertions(+)
> create mode 100644 drivers/firmware/tegra/Kconfig
> create mode 100644 drivers/firmware/tegra/Makefile
> create mode 100644 drivers/firmware/tegra/ivc.c
> create mode 100644 include/soc/tegra/ivc.h
>
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 5e618058defe..bbd64ae8c4c6 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -200,5 +200,6 @@ config HAVE_ARM_SMCCC
> source "drivers/firmware/broadcom/Kconfig"
> source "drivers/firmware/google/Kconfig"
> source "drivers/firmware/efi/Kconfig"
> +source "drivers/firmware/tegra/Kconfig"
>
> endmenu
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index 474bada56fcd..9a4df8171cc4 100644
> --- a/drivers/firmware/Makefile
> +++ b/drivers/firmware/Makefile
> @@ -24,3 +24,4 @@ obj-y += broadcom/
> obj-$(CONFIG_GOOGLE_FIRMWARE) += google/
> obj-$(CONFIG_EFI) += efi/
> obj-$(CONFIG_UEFI_CPER) += efi/
> +obj-y += tegra/
> diff --git a/drivers/firmware/tegra/Kconfig b/drivers/firmware/tegra/Kconfig
> new file mode 100644
> index 000000000000..1fa3e4e136a5
> --- /dev/null
> +++ b/drivers/firmware/tegra/Kconfig
> @@ -0,0 +1,13 @@
> +menu "Tegra firmware driver"
> +
> +config TEGRA_IVC
> + bool "Tegra IVC protocol"
> + depends on ARCH_TEGRA
> + help
> + IVC (Inter-VM Communication) protocol is part of the IPC
> + (Inter Processor Communication) framework on Tegra. It maintains the
> + data and the different commuication channels in SysRAM or RAM and
> + keeps the content is synchronization between host CPU and remote
> + processors.
> +
> +endmenu
> diff --git a/drivers/firmware/tegra/Makefile b/drivers/firmware/tegra/Makefile
> new file mode 100644
> index 000000000000..92e2153e8173
> --- /dev/null
> +++ b/drivers/firmware/tegra/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_TEGRA_IVC) += ivc.o
> diff --git a/drivers/firmware/tegra/ivc.c b/drivers/firmware/tegra/ivc.c
> new file mode 100644
> index 000000000000..3e736bb9915a
> --- /dev/null
> +++ b/drivers/firmware/tegra/ivc.c
> @@ -0,0 +1,659 @@
> +/*
> + * Copyright (c) 2014-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/module.h>
> +
> +#include <soc/tegra/ivc.h>
> +
> +#define IVC_ALIGN 64
> +
> +#ifdef CONFIG_SMP
> +
> +static inline void ivc_rmb(void)
> +{
> + smp_rmb();
> +}
> +
> +static inline void ivc_wmb(void)
> +{
> + smp_wmb();
> +}
> +
> +static inline void ivc_mb(void)
> +{
> + smp_mb();
> +}
> +
> +#else
> +
> +static inline void ivc_rmb(void)
> +{
> + rmb();
> +}
> +
> +static inline void ivc_wmb(void)
> +{
> + wmb();
> +}
> +
> +static inline void ivc_mb(void)
> +{
> + mb();
> +}
> +
> +#endif
> +
> +/*
> + * IVC channel reset protocol.
> + *
> + * Each end uses its tx_channel.state to indicate its synchronization state.
> + */
> +enum ivc_state {
> + /*
> + * This value is zero for backwards compatibility with services that
> + * assume channels to be initially zeroed. Such channels are in an
> + * initially valid state, but cannot be asynchronously reset, and must
> + * maintain a valid state at all times.
> + *
> + * The transmitting end can enter the established state from the sync or
> + * ack state when it observes the receiving endpoint in the ack or
> + * established state, indicating that has cleared the counters in our
> + * rx_channel.
> + */
> + ivc_state_established = 0,
> +
> + /*
> + * If an endpoint is observed in the sync state, the remote endpoint is
> + * allowed to clear the counters it owns asynchronously with respect to
> + * the current endpoint. Therefore, the current endpoint is no longer
> + * allowed to communicate.
> + */
> + ivc_state_sync,
> +
> + /*
> + * When the transmitting end observes the receiving end in the sync
> + * state, it can clear the w_count and r_count and transition to the ack
> + * state. If the remote endpoint observes us in the ack state, it can
> + * return to the established state once it has cleared its counters.
> + */
> + ivc_state_ack
> +};
> +
> +/*
> + * This structure is divided into two-cache aligned parts, the first is only
Should read "two cache-aligned" maybe?
> + * written through the tx_channel pointer, while the second is only written
> + * through the rx_channel pointer. This delineates ownership of the cache lines,
> + * which is critical to performance and necessary in non-cache coherent
> + * implementations.
> + */
> +struct ivc_channel_header {
> + union {
> + struct {
> + /* fields owned by the transmitting end */
Fields? According to the context I would say "frames?"
> + uint32_t w_count;
> + uint32_t state;
> + };
> + uint8_t w_align[IVC_ALIGN];
> + };
> + union {
> + /* fields owned by the receiving end */
Same here.
> + uint32_t r_count;
> + uint8_t r_align[IVC_ALIGN];
> + };
> +};
> +
> +static inline void ivc_invalidate_counter(struct ivc *ivc,
> + dma_addr_t handle)
> +{
> + if (!ivc->peer_device)
> + return;
> + dma_sync_single_for_cpu(ivc->peer_device, handle, IVC_ALIGN,
> + DMA_FROM_DEVICE);
> +}
> +
> +static inline void ivc_flush_counter(struct ivc *ivc, dma_addr_t handle)
> +{
> + if (!ivc->peer_device)
> + return;
> + dma_sync_single_for_device(ivc->peer_device, handle, IVC_ALIGN,
> + DMA_TO_DEVICE);
> +}
> +
> +static inline int ivc_channel_empty(struct ivc *ivc,
> + struct ivc_channel_header *ch)
This function should probably return bool.
> +{
> + /*
> + * This function performs multiple checks on the same values with
> + * security implications, so create snapshots with ACCESS_ONCE() to
> + * ensure that these checks use the same values.
> + */
> + uint32_t w_count = ACCESS_ONCE(ch->w_count);
> + uint32_t r_count = ACCESS_ONCE(ch->r_count);
> +
> + /*
> + * Perform an over-full check to prevent denial of service attacks where
> + * a server could be easily fooled into believing that there's an
> + * extremely large number of frames ready, since receivers are not
> + * expected to check for full or over-full conditions.
> + *
> + * Although the channel isn't empty, this is an invalid case caused by
> + * a potentially malicious peer, so returning empty is safer, because it
> + * gives the impression that the channel has gone silent.
> + */
> + if (w_count - r_count > ivc->nframes)
> + return 1;
> +
> + return w_count == r_count;
> +}
> +
> +static inline int ivc_channel_full(struct ivc *ivc,
> + struct ivc_channel_header *ch)
And this one too.
> +{
> + /*
> + * Invalid cases where the counters indicate that the queue is over
> + * capacity also appear full.
> + */
> + return ACCESS_ONCE(ch->w_count) - ACCESS_ONCE(ch->r_count)
> + >= ivc->nframes;
> +}
> +
> +static inline uint32_t ivc_channel_avail_count(struct ivc *ivc,
> + struct ivc_channel_header *ch)
> +{
> + /*
> + * This function isn't expected to be used in scenarios where an
> + * over-full situation can lead to denial of service attacks. See the
> + * comment in ivc_channel_empty() for an explanation about special
> + * over-full considerations.
> + */
> + return ACCESS_ONCE(ch->w_count) - ACCESS_ONCE(ch->r_count);
> +}
> +
> +static inline void ivc_advance_tx(struct ivc *ivc)
> +{
> + ACCESS_ONCE(ivc->tx_channel->w_count) =
> + ACCESS_ONCE(ivc->tx_channel->w_count) + 1;
> +
> + if (ivc->w_pos == ivc->nframes - 1)
> + ivc->w_pos = 0;
> + else
> + ivc->w_pos++;
> +}
> +
> +static inline void ivc_advance_rx(struct ivc *ivc)
> +{
> + ACCESS_ONCE(ivc->rx_channel->r_count) =
> + ACCESS_ONCE(ivc->rx_channel->r_count) + 1;
> +
> + if (ivc->r_pos == ivc->nframes - 1)
> + ivc->r_pos = 0;
> + else
> + ivc->r_pos++;
> +}
> +
> +static inline int ivc_check_read(struct ivc *ivc)
> +{
> + /*
> + * tx_channel->state is set locally, so it is not synchronized with
> + * state from the remote peer. The remote peer cannot reset its
> + * transmit counters until we've acknowledged its synchronization
> + * request, so no additional synchronization is required because an
> + * asynchronous transition of rx_channel->state to ivc_state_ack is not
> + * allowed.
> + */
> + if (ivc->tx_channel->state != ivc_state_established)
> + return -ECONNRESET;
> +
> + /*
> + * Avoid unnecessary invalidations when performing repeated accesses to
> + * an IVC channel by checking the old queue pointers first.
> + * Synchronization is only necessary when these pointers indicate empty
> + * or full.
> + */
> + if (!ivc_channel_empty(ivc, ivc->rx_channel))
> + return 0;
> +
> + ivc_invalidate_counter(ivc, ivc->rx_handle +
> + offsetof(struct ivc_channel_header, w_count));
> + return ivc_channel_empty(ivc, ivc->rx_channel) ? -ENOMEM : 0;
> +}
> +
> +static inline int ivc_check_write(struct ivc *ivc)
> +{
> + if (ivc->tx_channel->state != ivc_state_established)
> + return -ECONNRESET;
> +
> + if (!ivc_channel_full(ivc, ivc->tx_channel))
> + return 0;
> +
> + ivc_invalidate_counter(ivc, ivc->tx_handle +
> + offsetof(struct ivc_channel_header, r_count));
> + return ivc_channel_full(ivc, ivc->tx_channel) ? -ENOMEM : 0;
> +}
> +
> +static void *ivc_frame_pointer(struct ivc *ivc, struct ivc_channel_header *ch,
> + uint32_t frame)
> +{
> + BUG_ON(frame >= ivc->nframes);
> + return (void *)((uintptr_t)(ch + 1) + ivc->frame_size * frame);
> +}
> +
> +static inline dma_addr_t ivc_frame_handle(struct ivc *ivc,
> + dma_addr_t channel_handle, uint32_t frame)
> +{
> + BUG_ON(!ivc->peer_device);
> + BUG_ON(frame >= ivc->nframes);
> + return channel_handle + sizeof(struct ivc_channel_header) +
> + ivc->frame_size * frame;
> +}
> +
> +static inline void ivc_invalidate_frame(struct ivc *ivc,
> + dma_addr_t channel_handle, unsigned frame, int offset, int len)
> +{
> + if (!ivc->peer_device)
> + return;
> + dma_sync_single_for_cpu(ivc->peer_device,
> + ivc_frame_handle(ivc, channel_handle, frame) + offset,
> + len, DMA_FROM_DEVICE);
> +}
> +
> +static inline void ivc_flush_frame(struct ivc *ivc, dma_addr_t channel_handle,
> + unsigned frame, int offset, int len)
> +{
> + if (!ivc->peer_device)
> + return;
> + dma_sync_single_for_device(ivc->peer_device,
> + ivc_frame_handle(ivc, channel_handle, frame) + offset,
> + len, DMA_TO_DEVICE);
> +}
> +
> +/* directly peek at the next frame rx'ed */
> +void *tegra_ivc_read_get_next_frame(struct ivc *ivc)
> +{
> + int result = ivc_check_read(ivc);
> + if (result)
> + return ERR_PTR(result);
> +
> + /*
> + * Order observation of w_pos potentially indicating new data before
> + * data read.
> + */
> + ivc_rmb();
> +
> + ivc_invalidate_frame(ivc, ivc->rx_handle, ivc->r_pos, 0,
> + ivc->frame_size);
> + return ivc_frame_pointer(ivc, ivc->rx_channel, ivc->r_pos);
> +}
> +EXPORT_SYMBOL(tegra_ivc_read_get_next_frame);
> +
> +int tegra_ivc_read_advance(struct ivc *ivc)
> +{
> + /*
> + * No read barriers or synchronization here: the caller is expected to
> + * have already observed the channel non-empty. This check is just to
> + * catch programming errors.
> + */
> + int result = ivc_check_read(ivc);
> + if (result)
> + return result;
> +
> + ivc_advance_rx(ivc);
> + ivc_flush_counter(ivc, ivc->rx_handle +
> + offsetof(struct ivc_channel_header, r_count));
This function is called quite a few times, and every time you have
this cumbersome offsetof. In practice you can only flush one of two
things: the write counter (first 64 bits of the header) or the read
counter (second part). Maybe you can specify which one you want to
flush through an extra argument to ivc_flush_counter (say, enum {
COUNTER_RD, COUNTER_WR }), and perform the offsetof there according to
the value of the argument? Same for ivc_invalidate_counter.
> +
> + /*
> + * Ensure our write to r_pos occurs before our read from w_pos.
> + */
> + ivc_mb();
> +
> + /*
> + * Notify only upon transition from full to non-full.
> + * The available count can only asynchronously increase, so the
> + * worst possible side-effect will be a spurious notification.
> + */
> + ivc_invalidate_counter(ivc, ivc->rx_handle +
> + offsetof(struct ivc_channel_header, w_count));
> +
> + if (ivc_channel_avail_count(ivc, ivc->rx_channel) == ivc->nframes - 1)
> + ivc->notify(ivc);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(tegra_ivc_read_advance);
> +
> +/* directly poke at the next frame to be tx'ed */
> +void *tegra_ivc_write_get_next_frame(struct ivc *ivc)
> +{
> + int result = ivc_check_write(ivc);
> + if (result)
> + return ERR_PTR(result);
> +
> + return ivc_frame_pointer(ivc, ivc->tx_channel, ivc->w_pos);
> +}
> +EXPORT_SYMBOL(tegra_ivc_write_get_next_frame);
> +
> +/* advance the tx buffer */
> +int tegra_ivc_write_advance(struct ivc *ivc)
> +{
> + int result = ivc_check_write(ivc);
> + if (result)
> + return result;
> +
> + ivc_flush_frame(ivc, ivc->tx_handle, ivc->w_pos, 0, ivc->frame_size);
> +
> + /*
> + * Order any possible stores to the frame before update of w_pos.
> + */
> + ivc_wmb();
> +
> + ivc_advance_tx(ivc);
> + ivc_flush_counter(ivc, ivc->tx_handle +
> + offsetof(struct ivc_channel_header, w_count));
> +
> + /*
> + * Ensure our write to w_pos occurs before our read from r_pos.
> + */
> + ivc_mb();
> +
> + /*
> + * Notify only upon transition from empty to non-empty.
> + * The available count can only asynchronously decrease, so the
> + * worst possible side-effect will be a spurious notification.
> + */
> + ivc_invalidate_counter(ivc, ivc->tx_handle +
> + offsetof(struct ivc_channel_header, r_count));
> +
> + if (ivc_channel_avail_count(ivc, ivc->tx_channel) == 1)
> + ivc->notify(ivc);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(tegra_ivc_write_advance);
> +
> +void tegra_ivc_channel_reset(struct ivc *ivc)
> +{
> + ivc->tx_channel->state = ivc_state_sync;
> + ivc_flush_counter(ivc, ivc->tx_handle +
> + offsetof(struct ivc_channel_header, w_count));
> + ivc->notify(ivc);
> +}
> +EXPORT_SYMBOL(tegra_ivc_channel_reset);
> +
> +/*
> + * ===============================================================
> + * IVC State Transition Table - see tegra_ivc_channel_notified()
> + * ===============================================================
> + *
> + * local remote action
> + * ----- ------ -----------------------------------
> + * SYNC EST <none>
> + * SYNC ACK reset counters; move to EST; notify
> + * SYNC SYNC reset counters; move to ACK; notify
> + * ACK EST move to EST; notify
> + * ACK ACK move to EST; notify
> + * ACK SYNC reset counters; move to ACK; notify
> + * EST EST <none>
> + * EST ACK <none>
> + * EST SYNC reset counters; move to ACK; notify
> + *
> + * ===============================================================
> + */
> +
> +int tegra_ivc_channel_notified(struct ivc *ivc)
> +{
> + enum ivc_state peer_state;
> +
> + /* Copy the receiver's state out of shared memory. */
> + ivc_invalidate_counter(ivc, ivc->rx_handle +
> + offsetof(struct ivc_channel_header, w_count));
> + peer_state = ACCESS_ONCE(ivc->rx_channel->state);
> +
> + if (peer_state == ivc_state_sync) {
> + /*
> + * Order observation of ivc_state_sync before stores clearing
> + * tx_channel.
> + */
> + ivc_rmb();
> +
> + /*
> + * Reset tx_channel counters. The remote end is in the SYNC
> + * state and won't make progress until we change our state,
> + * so the counters are not in use at this time.
> + */
> + ivc->tx_channel->w_count = 0;
> + ivc->rx_channel->r_count = 0;
> +
> + ivc->w_pos = 0;
> + ivc->r_pos = 0;
> +
> + /*
> + * Ensure that counters appear cleared before new state can be
> + * observed.
> + */
> + ivc_wmb();
> +
> + /*
> + * Move to ACK state. We have just cleared our counters, so it
> + * is now safe for the remote end to start using these values.
> + */
> + ivc->tx_channel->state = ivc_state_ack;
> + ivc_flush_counter(ivc, ivc->tx_handle +
> + offsetof(struct ivc_channel_header, w_count));
> +
> + /*
> + * Notify remote end to observe state transition.
> + */
> + ivc->notify(ivc);
> +
> + } else if (ivc->tx_channel->state == ivc_state_sync &&
> + peer_state == ivc_state_ack) {
> + /*
> + * Order observation of ivc_state_sync before stores clearing
> + * tx_channel.
> + */
> + ivc_rmb();
> +
> + /*
> + * Reset tx_channel counters. The remote end is in the ACK
> + * state and won't make progress until we change our state,
> + * so the counters are not in use at this time.
> + */
> + ivc->tx_channel->w_count = 0;
> + ivc->rx_channel->r_count = 0;
> +
> + ivc->w_pos = 0;
> + ivc->r_pos = 0;
> +
> + /*
> + * Ensure that counters appear cleared before new state can be
> + * observed.
> + */
> + ivc_wmb();
> +
> + /*
> + * Move to ESTABLISHED state. We know that the remote end has
> + * already cleared its counters, so it is safe to start
> + * writing/reading on this channel.
> + */
> + ivc->tx_channel->state = ivc_state_established;
> + ivc_flush_counter(ivc, ivc->tx_handle +
> + offsetof(struct ivc_channel_header, w_count));
> +
> + /*
> + * Notify remote end to observe state transition.
> + */
> + ivc->notify(ivc);
> +
> + } else if (ivc->tx_channel->state == ivc_state_ack) {
> + /*
> + * At this point, we have observed the peer to be in either
> + * the ACK or ESTABLISHED state. Next, order observation of
> + * peer state before storing to tx_channel.
> + */
> + ivc_rmb();
> +
> + /*
> + * Move to ESTABLISHED state. We know that we have previously
> + * cleared our counters, and we know that the remote end has
> + * cleared its counters, so it is safe to start writing/reading
> + * on this channel.
> + */
> + ivc->tx_channel->state = ivc_state_established;
> + ivc_flush_counter(ivc, ivc->tx_handle +
> + offsetof(struct ivc_channel_header, w_count));
> +
> + /*
> + * Notify remote end to observe state transition.
> + */
> + ivc->notify(ivc);
> +
> + } else {
> + /*
> + * There is no need to handle any further action. Either the
> + * channel is already fully established, or we are waiting for
> + * the remote end to catch up with our current state. Refer
> + * to the diagram in "IVC State Transition Table" above.
> + */
> + }
> +
> + return ivc->tx_channel->state == ivc_state_established ? 0 : -EAGAIN;
> +}
> +EXPORT_SYMBOL(tegra_ivc_channel_notified);
> +
> +size_t tegra_ivc_align(size_t size)
> +{
> + return (size + (IVC_ALIGN - 1)) & ~(IVC_ALIGN - 1);
return ALIGN(size, IVC_ALIGN)?
> +}
> +EXPORT_SYMBOL(tegra_ivc_align);
> +
> +unsigned tegra_ivc_total_queue_size(unsigned queue_size)
> +{
> + if (queue_size & (IVC_ALIGN - 1)) {
> + pr_err("%s: queue_size (%u) must be %u-byte aligned\n",
> + __func__, queue_size, IVC_ALIGN);
> + return 0;
> + }
> + return queue_size + sizeof(struct ivc_channel_header);
> +}
> +EXPORT_SYMBOL(tegra_ivc_total_queue_size);
> +
> +static int check_ivc_params(uintptr_t queue_base1, uintptr_t queue_base2,
> + unsigned nframes, unsigned frame_size)
> +{
> + BUG_ON(offsetof(struct ivc_channel_header, w_count) & (IVC_ALIGN - 1));
> + BUG_ON(offsetof(struct ivc_channel_header, r_count) & (IVC_ALIGN - 1));
> + BUG_ON(sizeof(struct ivc_channel_header) & (IVC_ALIGN - 1));
These checks are done on purely static data, we don't need to do it
here, for each channel... If there a way to have them performed at
compilation time instead? Since these constraints must be enforced,
they should also be specified as a comment to struct
ivc_channel_header to avoid unwanted modifications.
Mmm, or thinking twice, since the condition can be evaluated at
compilation time, maybe the compiler will optimize these out entirely?
In that case, a better place to do this would be tegra_ivc_init() - we
want the failure to be reported as early as possible.
> +
> + if ((uint64_t)nframes * (uint64_t)frame_size >= 0x100000000) {
> + pr_err("nframes * frame_size overflows\n");
> + return -EINVAL;
> + }
> +
> + /*
> + * The headers must at least be aligned enough for counters
> + * to be accessed atomically.
> + */
> + if (queue_base1 & (IVC_ALIGN - 1)) {
> + pr_err("ivc channel start not aligned: %lx\n", queue_base1);
> + return -EINVAL;
> + }
> + if (queue_base2 & (IVC_ALIGN - 1)) {
> + pr_err("ivc channel start not aligned: %lx\n", queue_base2);
> + return -EINVAL;
> + }
> +
> + if (frame_size & (IVC_ALIGN - 1)) {
> + pr_err("frame size not adequately aligned: %u\n", frame_size);
> + return -EINVAL;
> + }
> +
> + if (queue_base1 < queue_base2) {
> + if (queue_base1 + frame_size * nframes > queue_base2) {
> + pr_err("queue regions overlap: %lx + %x, %x\n",
> + queue_base1, frame_size,
> + frame_size * nframes);
> + return -EINVAL;
> + }
> + } else {
> + if (queue_base2 + frame_size * nframes > queue_base1) {
> + pr_err("queue regions overlap: %lx + %x, %x\n",
> + queue_base2, frame_size,
> + frame_size * nframes);
> + return -EINVAL;
> + }
> + }
> +
> + return 0;
> +}
> +
> +int tegra_ivc_init(struct ivc *ivc, uintptr_t rx_base, dma_addr_t rx_handle,
> + uintptr_t tx_base, dma_addr_t tx_handle, unsigned nframes,
> + unsigned frame_size, struct device *peer_device,
> + void (*notify)(struct ivc *))
> +{
> + size_t queue_size;
> +
> + int result = check_ivc_params(rx_base, tx_base, nframes, frame_size);
> + if (result)
> + return result;
> +
> + BUG_ON(!ivc);
> + BUG_ON(!notify);
Why BUG_ON and not return -EINVAL? Is this really unrecoverable?
Doesn't seem so, and later in this function you return error
conditions for other errors...
> +
> + queue_size = tegra_ivc_total_queue_size(nframes * frame_size);
> +
> + /*
> + * All sizes that can be returned by communication functions should
> + * fit in an int.
> + */
> + if (frame_size > INT_MAX)
> + return -E2BIG;
> +
> + ivc->rx_channel = (struct ivc_channel_header *)rx_base;
> + ivc->tx_channel = (struct ivc_channel_header *)tx_base;
> +
> + if (peer_device) {
> + if (rx_handle != DMA_ERROR_CODE) {
This looks more complicated than it needs to be - in practice,
tx_handle and rx_handle are always DMA_ERROR_CODE when you call this
function from the BPMP code, and this block will never get used.
Furthermore, ivc_flush_*() and ivc_invalidate_*() only rely on
peer_device to decide whether they need to call dma_sync. Calling
these functions on an unmapped page is an invalid use of the DMA API,
so the case where rx_handle != DMA_ERROR_CODE is invalid anyway.
Isn't it possible to remove these rx_handle/tx_handle arguments
altogether and thus simplify this function?
> + ivc->rx_handle = rx_handle;
> + ivc->tx_handle = tx_handle;
> + } else {
> + ivc->rx_handle = dma_map_single(peer_device,
> + ivc->rx_channel, queue_size, DMA_BIDIRECTIONAL);
> + if (ivc->rx_handle == DMA_ERROR_CODE)
> + return -ENOMEM;
> +
> + ivc->tx_handle = dma_map_single(peer_device,
> + ivc->tx_channel, queue_size, DMA_BIDIRECTIONAL);
> + if (ivc->tx_handle == DMA_ERROR_CODE) {
> + dma_unmap_single(peer_device, ivc->rx_handle,
> + queue_size, DMA_BIDIRECTIONAL);
> + return -ENOMEM;
> + }
When do we unmap these pages btw? I know the BPMP driver is probably
never going to be unloaded, but we should probably have
tegra_ivc_cleanup somewhere since other potential users may make use
of it.
> + }
> + }
> +
> + ivc->notify = notify;
> + ivc->frame_size = frame_size;
> + ivc->nframes = nframes;
> + ivc->peer_device = peer_device;
> +
> + /*
> + * These values aren't necessarily correct until the channel has been
> + * reset.
> + */
> + ivc->w_pos = 0;
> + ivc->r_pos = 0;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(tegra_ivc_init);
> diff --git a/include/soc/tegra/ivc.h b/include/soc/tegra/ivc.h
> new file mode 100644
> index 000000000000..1762fbee3fa2
> --- /dev/null
> +++ b/include/soc/tegra/ivc.h
> @@ -0,0 +1,102 @@
> +/*
> + * 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.
> + */
> +
> +#ifndef __TEGRA_IVC_H
> +
> +#include <linux/device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/types.h>
> +
> +struct ivc_channel_header;
> +
> +struct ivc {
I'd rename this to tegra_ivc to avoid potential name collisions (and
since all using functions are prefixed with tegra_ivc).
> + struct ivc_channel_header *rx_channel, *tx_channel;
> + uint32_t w_pos, r_pos;
> +
> + void (*notify)(struct ivc *);
> + uint32_t nframes, frame_size;
> +
> + struct device *peer_device;
> + dma_addr_t rx_handle, tx_handle;
> +};
> +
> +/**
> + * tegra_ivc_read_get_next_frame - Peek at the next frame to receive
> + * @ivc pointer of the IVC channel
> + *
> + * Peek at the next frame to be received, without removing it from
> + * the queue.
> + *
> + * Returns a pointer to the frame, or an error encoded pointer.
> + */
> +void *tegra_ivc_read_get_next_frame(struct ivc *ivc);
> +
> +/**
> + * tegra_ivc_read_advance - Advance the read queue
> + * @ivc pointer of the IVC channel
> + *
> + * Advance the read queue
> + *
> + * Returns 0, or a negative error value if failed.
> + */
> +int tegra_ivc_read_advance(struct ivc *ivc);
> +
> +/**
> + * tegra_ivc_write_get_next_frame - Poke at the next frame to transmit
> + * @ivc pointer of the IVC channel
> + *
> + * Get access to the next frame.
> + *
> + * Returns a pointer to the frame, or an error encoded pointer.
> + */
> +void *tegra_ivc_write_get_next_frame(struct ivc *ivc);
> +
> +/**
> + * tegra_ivc_write_advance - Advance the write queue
> + * @ivc pointer of the IVC channel
> + *
> + * Advance the write queue
> + *
> + * Returns 0, or a negative error value if failed.
> + */
> +int tegra_ivc_write_advance(struct ivc *ivc);
> +
> +/**
> + * tegra_ivc_channel_notified - handle internal messages
> + * @ivc pointer of the IVC channel
> + *
> + * This function must be called following every notification.
> + *
> + * Returns 0 if the channel is ready for communication, or -EAGAIN if a channel
> + * reset is in progress.
> + */
> +int tegra_ivc_channel_notified(struct ivc *ivc);
> +
> +/**
> + * tegra_ivc_channel_reset - initiates a reset of the shared memory state
> + * @ivc pointer of the IVC channel
> + *
> + * This function must be called after a channel is reserved before it is used
> + * for communication. The channel will be ready for use when a subsequent call
> + * to notify the remote of the channel reset.
> + */
> +void tegra_ivc_channel_reset(struct ivc *ivc);
> +
> +size_t tegra_ivc_align(size_t size);
> +unsigned tegra_ivc_total_queue_size(unsigned queue_size);
> +int tegra_ivc_init(struct ivc *ivc, uintptr_t rx_base, dma_addr_t rx_handle,
> + uintptr_t tx_base, dma_addr_t tx_handle, unsigned nframes,
> + unsigned frame_size, struct device *peer_device,
> + void (*notify)(struct ivc *));
> +
> +#endif /* __TEGRA_IVC_H */
> --
> 2.9.0
>
[toc] | [prev] | [next] | [standalone]
| From | Paul Gortmaker <paul.gortmaker@windriver.com> |
|---|---|
| Date | 2016-07-10 01:50 +0200 |
| Subject | Re: [PATCH V2 04/10] firmware: tegra: add IVC library |
| Message-ID | <rTaat-16A-7@gated-at.bofh.it> |
| In reply to | #1436863 |
On Tue, Jul 5, 2016 at 5:04 AM, Joseph Lo <josephl@nvidia.com> wrote: > The Inter-VM communication (IVC) is a communication protocol, which is > designed for interprocessor communication (IPC) or the communication > between the hypervisor and the virtual machine with a guest OS on it. So > it can be translated as inter-virtual memory or inter-virtual machine > communication. The message channels are maintained on the DRAM or SRAM > and the data coherency should be considered. Or the data could be > corrupted or out of date when the remote client checking it. > > Inside the IVC, it maintains memory-based descriptors for the TX/RX > channels and the coherency issue of the counter and payloads. So the > clients can use it to send/receive messages to/from remote ones. > > We introduce it as a library for the firmware drivers, which can use it > for IPC. > > Based-on-the-work-by: > Peter Newman <pnewman@nvidia.com> > > Signed-off-by: Joseph Lo <josephl@nvidia.com> > --- > Changes in V2: > - None > --- > drivers/firmware/Kconfig | 1 + > drivers/firmware/Makefile | 1 + > drivers/firmware/tegra/Kconfig | 13 + > drivers/firmware/tegra/Makefile | 1 + > drivers/firmware/tegra/ivc.c | 659 ++++++++++++++++++++++++++++++++++++++++ > include/soc/tegra/ivc.h | 102 +++++++ > 6 files changed, 777 insertions(+) > create mode 100644 drivers/firmware/tegra/Kconfig > create mode 100644 drivers/firmware/tegra/Makefile > create mode 100644 drivers/firmware/tegra/ivc.c > create mode 100644 include/soc/tegra/ivc.h > > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig > index 5e618058defe..bbd64ae8c4c6 100644 > --- a/drivers/firmware/Kconfig > +++ b/drivers/firmware/Kconfig > @@ -200,5 +200,6 @@ config HAVE_ARM_SMCCC > source "drivers/firmware/broadcom/Kconfig" > source "drivers/firmware/google/Kconfig" > source "drivers/firmware/efi/Kconfig" > +source "drivers/firmware/tegra/Kconfig" > > endmenu > diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile > index 474bada56fcd..9a4df8171cc4 100644 > --- a/drivers/firmware/Makefile > +++ b/drivers/firmware/Makefile > @@ -24,3 +24,4 @@ obj-y += broadcom/ > obj-$(CONFIG_GOOGLE_FIRMWARE) += google/ > obj-$(CONFIG_EFI) += efi/ > obj-$(CONFIG_UEFI_CPER) += efi/ > +obj-y += tegra/ > diff --git a/drivers/firmware/tegra/Kconfig b/drivers/firmware/tegra/Kconfig > new file mode 100644 > index 000000000000..1fa3e4e136a5 > --- /dev/null > +++ b/drivers/firmware/tegra/Kconfig > @@ -0,0 +1,13 @@ > +menu "Tegra firmware driver" > + > +config TEGRA_IVC > + bool "Tegra IVC protocol" If this driver is not tristate, then why does the driver include the module.h header below? > + depends on ARCH_TEGRA > + help > + IVC (Inter-VM Communication) protocol is part of the IPC > + (Inter Processor Communication) framework on Tegra. It maintains the > + data and the different commuication channels in SysRAM or RAM and > + keeps the content is synchronization between host CPU and remote > + processors. > + > +endmenu > diff --git a/drivers/firmware/tegra/Makefile b/drivers/firmware/tegra/Makefile > new file mode 100644 > index 000000000000..92e2153e8173 > --- /dev/null > +++ b/drivers/firmware/tegra/Makefile > @@ -0,0 +1 @@ > +obj-$(CONFIG_TEGRA_IVC) += ivc.o > diff --git a/drivers/firmware/tegra/ivc.c b/drivers/firmware/tegra/ivc.c > new file mode 100644 > index 000000000000..3e736bb9915a > --- /dev/null > +++ b/drivers/firmware/tegra/ivc.c > @@ -0,0 +1,659 @@ > +/* > + * Copyright (c) 2014-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/module.h> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ I'm sure it "works" since module.h includes nearly everything else, but that is less than ideal for exactly the same reason. Thanks, Paul. -- > + > +#include <soc/tegra/ivc.h> > + > +#define IVC_ALIGN 64 > +
[toc] | [prev] | [next] | [standalone]
| From | Joseph Lo <josephl@nvidia.com> |
|---|---|
| Date | 2016-07-05 11:10 +0200 |
| Subject | [PATCH V2 08/10] arm64: dts: tegra: Add Tegra186 support |
| Message-ID | <rRuwG-MJ-43@gated-at.bofh.it> |
| In reply to | #1436857 |
This adds the initial support of Tegra186 SoC, which can help to bring
up the debug console and initrd for further developing.
Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
Changes in V2:
- update the file according the HSP and BPMP binding fix in V2
---
arch/arm64/boot/dts/nvidia/tegra186.dtsi | 77 ++++++++++++++++++++++++++++++++
1 file changed, 77 insertions(+)
create mode 100644 arch/arm64/boot/dts/nvidia/tegra186.dtsi
diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
new file mode 100644
index 000000000000..57badd5de9b4
--- /dev/null
+++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
@@ -0,0 +1,77 @@
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/mailbox/tegra186-hsp.h>
+
+/ {
+ compatible = "nvidia,tegra186";
+ interrupt-parent = <&gic>;
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ uarta: serial@03100000 {
+ compatible = "nvidia,tegra186-uart", "nvidia,tegra20-uart";
+ reg = <0x0 0x03100000 0x0 0x40>;
+ reg-shift = <2>;
+ interrupts = <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>;
+ status = "disabled";
+ };
+
+ gic: interrupt-controller@03881000 {
+ compatible = "arm,gic-400";
+ #interrupt-cells = <3>;
+ interrupt-controller;
+ reg = <0x0 0x03881000 0x0 0x1000>,
+ <0x0 0x03882000 0x0 0x2000>;
+ interrupts = <GIC_PPI 9
+ (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
+ interrupt-parent = <&gic>;
+ };
+
+ hsp_top0: hsp@03c00000 {
+ compatible = "nvidia,tegra186-hsp";
+ reg = <0x0 0x03c00000 0x0 0xa0000>;
+ interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "doorbell";
+ #mbox-cells = <1>;
+ status = "disabled";
+ };
+
+ sysram@30000000 {
+ compatible = "nvidia,tegra186-sysram", "mmio-ram";
+ reg = <0x0 0x30000000 0x0 0x4ffff>;
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges = <0 0x0 0x0 0x30000000 0x0 0x4ffff>;
+
+ cpu_bpmp_tx: bpmp_shmem@4e000 {
+ compatible = "nvidia,tegra186-bpmp-shmem";
+ reg = <0x0 0x4e000 0x0 0x1000>;
+ };
+
+ cpu_bpmp_rx: bpmp_shmem@4f000 {
+ compatible = "nvidia,tegra186-bpmp-shmem";
+ reg = <0x0 0x4f000 0x0 0x1000>;
+ };
+ };
+
+ bpmp {
+ compatible = "nvidia,tegra186-bpmp";
+ mboxes = <&hsp_top0 HSP_MBOX_ID(DB, HSP_DB_MASTER_BPMP)>;
+ shmem = <&cpu_bpmp_tx &cpu_bpmp_rx>;
+ #clock-cells = <1>;
+ #reset-cells = <1>;
+ status = "disabled";
+ };
+
+ timer {
+ compatible = "arm,armv8-timer";
+ interrupts = <GIC_PPI 13
+ (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+ <GIC_PPI 14
+ (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+ <GIC_PPI 11
+ (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+ <GIC_PPI 10
+ (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
+ interrupt-parent = <&gic>;
+ };
+};
--
2.9.0
[toc] | [prev] | [next] | [standalone]
| From | Joseph Lo <josephl@nvidia.com> |
|---|---|
| Date | 2016-07-05 11:10 +0200 |
| Subject | [PATCH V2 06/10] soc/tegra: Add Tegra186 support |
| Message-ID | <rRuwG-MJ-41@gated-at.bofh.it> |
| In reply to | #1436857 |
The Tegra186 has a combination of Denver and Cortex-A57 CPU cores and GPUs with Pascal architecture on it. It features with ADSP with Cortex-A9 CPU for audio processing, hardware video encoder/decoder with multi-format support, ISP for image capture processing and BPMP for the power managements. Signed-off-by: Joseph Lo <josephl@nvidia.com> --- Changes in V2: - None --- drivers/soc/tegra/Kconfig | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/drivers/soc/tegra/Kconfig b/drivers/soc/tegra/Kconfig index 03089ad2fc65..88a71dfd466c 100644 --- a/drivers/soc/tegra/Kconfig +++ b/drivers/soc/tegra/Kconfig @@ -61,6 +61,20 @@ config ARCH_TEGRA_132_SOC but contains an NVIDIA Denver CPU complex in place of Tegra124's "4+1" Cortex-A15 CPU complex. +config ARCH_TEGRA_186_SOC + bool "NVIDIA Tegra186 SoC" + select MAILBOX + select TEGRA_BPMP + select TEGRA_HSP_MBOX + select TEGRA_IVC + help + Enable support for the NVIDIA Tegar186 SoC. The Tegra186 has a + combination of Denver and Cortex-A57 CPU cores and GPUs with Pascal + architecture on it. It features with ADSP with Cortex-A9 CPU for + audio processing, hardware video encoder/decoder with multi-format + support, ISP for image capture processing and BPMP for the power + managements. + config ARCH_TEGRA_210_SOC bool "NVIDIA Tegra210 SoC" select PINCTRL_TEGRA210 -- 2.9.0
[toc] | [prev] | [next] | [standalone]
| From | Alexandre Courbot <gnurou@gmail.com> |
|---|---|
| Date | 2016-07-06 13:50 +0200 |
| Subject | Re: [PATCH V2 03/10] Documentation: dt-bindings: firmware: tegra: add bindings of the BPMP |
| Message-ID | <rRTv3-fc-3@gated-at.bofh.it> |
| In reply to | #1436857 |
On Tue, Jul 5, 2016 at 6:04 PM, Joseph Lo <josephl@nvidia.com> wrote:
> The BPMP is a specific processor in Tegra chip, which is designed for
> booting process handling and offloading the power management, clock
> management, and reset control tasks from the CPU. The binding document
> defines the resources that would be used by the BPMP firmware driver,
> which can create the interprocessor communication (IPC) between the CPU
> and BPMP.
>
> Signed-off-by: Joseph Lo <josephl@nvidia.com>
> ---
> Changes in V2:
> - update the message that the BPMP is clock and reset control provider
> - add tegra186-clock.h and tegra186-reset.h header files
> - revise the description of the required properties
> ---
> .../bindings/firmware/nvidia,tegra186-bpmp.txt | 77 ++
> include/dt-bindings/clock/tegra186-clock.h | 940 +++++++++++++++++++++
> include/dt-bindings/reset/tegra186-reset.h | 217 +++++
> 3 files changed, 1234 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.txt
> create mode 100644 include/dt-bindings/clock/tegra186-clock.h
> create mode 100644 include/dt-bindings/reset/tegra186-reset.h
>
> diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.txt b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.txt
> new file mode 100644
> index 000000000000..4d0b6eba56c5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.txt
> @@ -0,0 +1,77 @@
> +NVIDIA Tegra Boot and Power Management Processor (BPMP)
> +
> +The BPMP is a specific processor in Tegra chip, which is designed for
> +booting process handling and offloading the power management, clock
> +management, and reset control tasks from the CPU. The binding document
> +defines the resources that would be used by the BPMP firmware driver,
> +which can create the interprocessor communication (IPC) between the CPU
> +and BPMP.
> +
> +Required properties:
> +- name : Should be bpmp
> +- compatible
> + Array of strings
> + One of:
> + - "nvidia,tegra186-bpmp"
> +- mboxes : The phandle of mailbox controller and the mailbox specifier.
> +- shmem : List of the phandle of the TX and RX shared memory area that
> + the IPC between CPU and BPMP is based on.
> +- #clock-cells : Should be 1.
> +- #reset-cells : Should be 1.
> +
> +This node is a mailbox consumer. See the following files for details of
> +the mailbox subsystem, and the specifiers implemented by the relevant
> +provider(s):
> +
> +- Documentation/devicetree/bindings/mailbox/mailbox.txt
> +- Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt
> +
> +This node is a clock and reset provider. See the following files for
> +general documentation of those features, and the specifiers implemented
> +by this node:
> +
> +- Documentation/devicetree/bindings/clock/clock-bindings.txt
> +- include/dt-bindings/clock/tegra186-clock.h
> +- Documentation/devicetree/bindings/reset/reset.txt
> +- include/dt-bindings/reset/tegra186-reset.h
> +
> +The shared memory bindings for BPMP
> +-----------------------------------
> +
> +The shared memory area for the IPC TX and RX between CPU and BPMP are
> +predefined and work on top of sysram, which is an SRAM inside the chip.
> +
> +See "Documentation/devicetree/bindings/sram/sram.txt" for the bindings.
> +
> +Example:
> +
> +hsp_top0: hsp@03c00000 {
> + ...
> + #mbox-cells = <1>;
> +};
> +
> +sysram@30000000 {
> + compatible = "nvidia,tegra186-sysram", "mmio-ram";
Shouldn't the second compatible be "mmio-sram"?
If so, then you have the same typo in tegra186.dtsi as well.
[toc] | [prev] | [next] | [standalone]
| From | Joseph Lo <josephl@nvidia.com> |
|---|---|
| Date | 2016-07-07 08:30 +0200 |
| Subject | Re: [PATCH V2 03/10] Documentation: dt-bindings: firmware: tegra: add bindings of the BPMP |
| Message-ID | <rSaYV-3ar-11@gated-at.bofh.it> |
| In reply to | #1437654 |
On 07/06/2016 07:42 PM, Alexandre Courbot wrote:
> On Tue, Jul 5, 2016 at 6:04 PM, Joseph Lo <josephl@nvidia.com> wrote:
>> The BPMP is a specific processor in Tegra chip, which is designed for
>> booting process handling and offloading the power management, clock
>> management, and reset control tasks from the CPU. The binding document
>> defines the resources that would be used by the BPMP firmware driver,
>> which can create the interprocessor communication (IPC) between the CPU
>> and BPMP.
>>
>> Signed-off-by: Joseph Lo <josephl@nvidia.com>
>> ---
>> Changes in V2:
>> - update the message that the BPMP is clock and reset control provider
>> - add tegra186-clock.h and tegra186-reset.h header files
>> - revise the description of the required properties
>> ---
>> .../bindings/firmware/nvidia,tegra186-bpmp.txt | 77 ++
>> include/dt-bindings/clock/tegra186-clock.h | 940 +++++++++++++++++++++
>> include/dt-bindings/reset/tegra186-reset.h | 217 +++++
>> 3 files changed, 1234 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.txt
>> create mode 100644 include/dt-bindings/clock/tegra186-clock.h
>> create mode 100644 include/dt-bindings/reset/tegra186-reset.h
>>
>> diff --git a/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.txt b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.txt
>> new file mode 100644
>> index 000000000000..4d0b6eba56c5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/firmware/nvidia,tegra186-bpmp.txt
>> @@ -0,0 +1,77 @@
>> +NVIDIA Tegra Boot and Power Management Processor (BPMP)
>> +
>> +The BPMP is a specific processor in Tegra chip, which is designed for
>> +booting process handling and offloading the power management, clock
>> +management, and reset control tasks from the CPU. The binding document
>> +defines the resources that would be used by the BPMP firmware driver,
>> +which can create the interprocessor communication (IPC) between the CPU
>> +and BPMP.
>> +
>> +Required properties:
>> +- name : Should be bpmp
>> +- compatible
>> + Array of strings
>> + One of:
>> + - "nvidia,tegra186-bpmp"
>> +- mboxes : The phandle of mailbox controller and the mailbox specifier.
>> +- shmem : List of the phandle of the TX and RX shared memory area that
>> + the IPC between CPU and BPMP is based on.
>> +- #clock-cells : Should be 1.
>> +- #reset-cells : Should be 1.
>> +
>> +This node is a mailbox consumer. See the following files for details of
>> +the mailbox subsystem, and the specifiers implemented by the relevant
>> +provider(s):
>> +
>> +- Documentation/devicetree/bindings/mailbox/mailbox.txt
>> +- Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt
>> +
>> +This node is a clock and reset provider. See the following files for
>> +general documentation of those features, and the specifiers implemented
>> +by this node:
>> +
>> +- Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +- include/dt-bindings/clock/tegra186-clock.h
>> +- Documentation/devicetree/bindings/reset/reset.txt
>> +- include/dt-bindings/reset/tegra186-reset.h
>> +
>> +The shared memory bindings for BPMP
>> +-----------------------------------
>> +
>> +The shared memory area for the IPC TX and RX between CPU and BPMP are
>> +predefined and work on top of sysram, which is an SRAM inside the chip.
>> +
>> +See "Documentation/devicetree/bindings/sram/sram.txt" for the bindings.
>> +
>> +Example:
>> +
>> +hsp_top0: hsp@03c00000 {
>> + ...
>> + #mbox-cells = <1>;
>> +};
>> +
>> +sysram@30000000 {
>> + compatible = "nvidia,tegra186-sysram", "mmio-ram";
>
> Shouldn't the second compatible be "mmio-sram"?
>
> If so, then you have the same typo in tegra186.dtsi as well.
>
Good catch, will fix.
Thanks,
-Joseph
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web