[PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver
Joseph Lo
josephl at nvidia.com
Mon Jul 18 01:51:05 PDT 2016
On 07/08/2016 05:10 AM, Sivaram Nair wrote:
> 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 at 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?
Ah, this should be MAX_NUM_HSP_DB_CHAN now.
But the mbox driver still needs a max channel number, I will check how
to enhance it properly with multiple HSP modules support in the same driver.
Maybe 4 channels for SM, AS, SS and DB. And the sub channels for
different functions under them. Then it may able to fix the double loop
issue in the hsp_db_irq function.
>
>> +#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?
Yes, will fix them.
>
>> +};
>> +
>> +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?
Because we only support DB right now, there is only one member in the
union. We can add something like sm_chan here when we need to support
that later.
>
>> +
>> +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.
OK, will fix this.
>
>> +};
>> +
>> +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)?
Yes.
>
>> + 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.
This should be MAX_NUM_HSP_DB_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:
>
> 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...?
OK, will fix both.
>
>> + 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()?
Yes, true.
>
>> + 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)?
Should be OK.
>
>> + 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()
?
I think it's different usage.
>
>> +
>> + 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;
>> + }
Thanks,
-Joseph
More information about the linux-arm-kernel
mailing list