From: Joseph Lo <josephl@nvidia.com>
To: Sivaram Nair <sivaramn@nvidia.com>
Cc: Alexandre Courbot <gnurou@gmail.com>,
Stephen Warren <swarren@wwwdotorg.org>,
Thierry Reding <thierry.reding@gmail.com>,
"linux-tegra@vger.kernel.org" <linux-tegra@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
Rob Herring <robh+dt@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Peter De Schrijver <pdeschrijver@nvidia.com>,
Matthew Longnecker <MLongnecker@nvidia.com>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
Jassi Brar <jassisinghbrar@gmail.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Catalin Marinas <catalin.marinas@arm.com>,
Will Deacon <will.deacon@arm.com>
Subject: Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver
Date: Mon, 18 Jul 2016 16:58:12 +0800 [thread overview]
Message-ID: <578C9A24.7090205@nvidia.com> (raw)
In-Reply-To: <20160707213313.GB9897@kickseed.nvidia.com>
On 07/08/2016 05:33 AM, Sivaram Nair wrote:
> 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?
>
I will try to refactor the driver to fix the inner loop issue by
separating the mbox channel with different HSP modules. And hook
different sub channels for different masters.
Thanks,
-Joseph
next prev parent reply other threads:[~2016-07-18 8:58 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-07-05 9:04 [PATCH V2 00/10] arm64: tegra: add BPMP support Joseph Lo
2016-07-05 9:04 ` [PATCH V2 01/10] Documentation: dt-bindings: mailbox: tegra: Add binding for HSP mailbox Joseph Lo
2016-07-06 17:02 ` Stephen Warren
2016-07-07 6:24 ` Joseph Lo
2016-07-07 18:13 ` Sivaram Nair
2016-07-07 18:35 ` Stephen Warren
2016-07-07 18:44 ` Sivaram Nair
2016-07-11 14:14 ` Rob Herring
2016-07-11 16:08 ` Stephen Warren
2016-07-18 23:13 ` Stephen Warren
2016-07-19 7:09 ` Joseph Lo
2016-07-05 9:04 ` [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver Joseph Lo
2016-07-06 7:05 ` Alexandre Courbot
2016-07-06 9:06 ` Joseph Lo
2016-07-06 12:23 ` Alexandre Courbot
2016-07-07 6:37 ` Joseph Lo
2016-07-07 21:33 ` Sivaram Nair
2016-07-18 8:58 ` Joseph Lo [this message]
2016-07-06 16:50 ` Stephen Warren
2016-07-07 6:49 ` Joseph Lo
2016-07-07 21:10 ` Sivaram Nair
2016-07-18 8:51 ` Joseph Lo
2016-07-05 9:04 ` [PATCH V2 03/10] Documentation: dt-bindings: firmware: tegra: add bindings of the BPMP Joseph Lo
2016-07-06 11:42 ` Alexandre Courbot
2016-07-07 6:25 ` Joseph Lo
2016-07-06 17:03 ` Stephen Warren
2016-07-07 6:26 ` Joseph Lo
2016-07-11 14:22 ` Rob Herring
2016-07-11 16:05 ` Stephen Warren
2016-07-18 7:44 ` Joseph Lo
2016-07-18 16:18 ` Stephen Warren
2016-07-13 19:41 ` Stephen Warren
2016-07-18 6:42 ` Joseph Lo
2016-07-05 9:04 ` [PATCH V2 04/10] firmware: tegra: add IVC library Joseph Lo
2016-07-07 11:16 ` Alexandre Courbot
2016-07-09 23:45 ` Paul Gortmaker
2016-07-05 9:04 ` [PATCH V2 05/10] firmware: tegra: add BPMP support Joseph Lo
2016-07-06 11:39 ` Alexandre Courbot
2016-07-06 16:39 ` Stephen Warren
2016-07-06 16:47 ` Matt Longnecker
2016-07-07 2:24 ` Alexandre Courbot
2016-07-07 8:17 ` Joseph Lo
2016-07-07 10:18 ` Alexandre Courbot
2016-07-07 19:55 ` Stephen Warren
2016-07-08 20:19 ` Sivaram Nair
2016-07-08 17:55 ` Sivaram Nair
2016-07-05 9:04 ` [PATCH V2 06/10] soc/tegra: Add Tegra186 support Joseph Lo
2016-07-05 9:04 ` [PATCH V2 07/10] arm64: defconfig: Enable Tegra186 SoC Joseph Lo
2016-07-05 9:04 ` [PATCH V2 08/10] arm64: dts: tegra: Add Tegra186 support Joseph Lo
2016-07-05 9:04 ` [PATCH V2 09/10] arm64: dts: tegra: Add NVIDIA Tegra186 P3310 main board support Joseph Lo
2016-07-05 9:04 ` [PATCH V2 10/10] arm64: dts: tegra: Add NVIDIA P2771 " Joseph Lo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=578C9A24.7090205@nvidia.com \
--to=josephl@nvidia.com \
--cc=MLongnecker@nvidia.com \
--cc=catalin.marinas@arm.com \
--cc=devicetree@vger.kernel.org \
--cc=gnurou@gmail.com \
--cc=jassisinghbrar@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=pdeschrijver@nvidia.com \
--cc=robh+dt@kernel.org \
--cc=sivaramn@nvidia.com \
--cc=swarren@wwwdotorg.org \
--cc=thierry.reding@gmail.com \
--cc=will.deacon@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).