Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver

From: Sivaram Nair
Date: Thu Jul 07 2016 - 17:10:43 EST

Next message: Sivaram Nair: "Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver"
Previous message: Rafael J. Wysocki: "Re: [PATCH v8 0/6] ACPI / processor_idle: Add ACPI v6.0 LPI support"
In reply to: Joseph Lo: "Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver"
Next in thread: Joseph Lo: "[PATCH V2 08/10] arm64: dts: tegra: Add Tegra186 support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html

Next message: Sivaram Nair: "Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver"
Previous message: Rafael J. Wysocki: "Re: [PATCH v8 0/6] ACPI / processor_idle: Add ACPI v6.0 LPI support"
In reply to: Joseph Lo: "Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver"
Next in thread: Joseph Lo: "[PATCH V2 08/10] arm64: dts: tegra: Add Tegra186 support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]