From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sivaram Nair Subject: Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver Date: Thu, 7 Jul 2016 14:10:16 -0700 Message-ID: <20160707211016.GA9897@kickseed.nvidia.com> References: <20160705090431.5852-1-josephl@nvidia.com> <20160705090431.5852-3-josephl@nvidia.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Return-path: In-Reply-To: <20160705090431.5852-3-josephl@nvidia.com> Content-Disposition: inline Sender: linux-kernel-owner@vger.kernel.org To: Joseph Lo Cc: Stephen Warren , Thierry Reding , Alexandre Courbot , linux-tegra@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Rob Herring , Mark Rutland , Peter De Schrijver , Matthew Longnecker , devicetree@vger.kernel.org, Jassi Brar , linux-kernel@vger.kernel.org, Catalin Marinas , Will Deacon List-Id: linux-tegra@vger.kernel.org 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 > --- > 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 > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752844AbcGGVKb (ORCPT ); Thu, 7 Jul 2016 17:10:31 -0400 Received: from hqemgate15.nvidia.com ([216.228.121.64]:4333 "EHLO hqemgate15.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751206AbcGGVKT (ORCPT ); Thu, 7 Jul 2016 17:10:19 -0400 X-PGP-Universal: processed; by hqpgpgate101.nvidia.com on Thu, 07 Jul 2016 14:10:17 -0700 Date: Thu, 7 Jul 2016 14:10:16 -0700 From: Sivaram Nair To: Joseph Lo CC: Stephen Warren , Thierry Reding , Alexandre Courbot , , , Rob Herring , Mark Rutland , Peter De Schrijver , Matthew Longnecker , , Jassi Brar , , Catalin Marinas , Will Deacon Subject: Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver Message-ID: <20160707211016.GA9897@kickseed.nvidia.com> References: <20160705090431.5852-1-josephl@nvidia.com> <20160705090431.5852-3-josephl@nvidia.com> MIME-Version: 1.0 In-Reply-To: <20160705090431.5852-3-josephl@nvidia.com> X-NVConfidentiality: public User-Agent: Mutt/1.5.21 (2010-09-15) Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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 > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: sivaramn@nvidia.com (Sivaram Nair) Date: Thu, 7 Jul 2016 14:10:16 -0700 Subject: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver In-Reply-To: <20160705090431.5852-3-josephl@nvidia.com> References: <20160705090431.5852-1-josephl@nvidia.com> <20160705090431.5852-3-josephl@nvidia.com> Message-ID: <20160707211016.GA9897@kickseed.nvidia.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 > --- > 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 > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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 at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html