Hi,
This is my another attempt to extend mailbox framework to support
doorbell mode mailbox hardware. It also adds doorbell support to ARM
MHU driver.
It makes no sense to create additional abstraction to deal with such
hardware as we end up re-implementing all the queuing mechanism in
the current framework as well as a set of new APIs which are similar
to the existing mailbox APIs.
Few mailbox releated terminologies I would like to get clarified.
Here are definations in my opinion, please chime in and correct it if I
got them wrong.
1. *message* - a self-contain entity which is sent by a sender to a
receiver, can be formed of one or more transfers
2. *transfers* - can nothing more than an interrupt, but it can also have
an associated data payload
3. *message protocol* - a protocol which defines the format of messages
that are sent between sender and receiver
4. *transport protocol* - a protocol which defines how transfers are
indicated by the sender to the receiver, using the mailbox and
the location of the payload, if applicable.
5. *channel* - used to pass messages between the sender and receiver.
A mailbox controller can implement configurable channels
depending upon the transport protocol.
6. *in-band* - A transfer payload sent using a mailbox channel
7. *out-band* - A transfer payload sent using a method other than a mailbox
channel, for example shared memory.
Brief hardware description about MHU
====================================
For each of the interrupt signals, the MHU drives the signal using a 32-bit
register, with all 32 bits logically ORed together. The MHU provides a set of
registers to enable software to SET, CLEAR, and check the STATUS of each of
the bits of this register independently. The use of 32 bits for each interrupt
line enables software to provide more information about the source of the
interrupt. For example, each bit of the register can be associated with a type
of event that can contribute to raising the interrupt.
Types of MHU Transport Protocols
================================
1. Doorbell - The doorbell transport protocol generates just an signal to
the receiver that sender has made a transfer.
2. Single-word - Each transfer is a single word transferred in-band.
3. Multi-word - Each transfer is made of two or more words, possible in
newer versions of MHU
Choice of Transport Protocol
============================
The choice is completely platform specific and it can be based on
usecases, number of available instances of mailbox,..etc
Add support for doorbell/signal mode controllers
================================================
Some mailbox controllers are lack FIFOs or memory to transmit data or
they may need to be configured in doorbell mode based on platform
choice. They typically contains single doorbell registers to just signal
the remote. The actually data is transmitted/shared using some shared
memory which is not part of the mailbox.
Such controllers don't need to transmit any data, they just transmit
the signal. In such controllers the data pointer passed to
mbox_send_message is passed to client via it's tx_prepare callback.
Controller doesn't need any data to be passed from the client.
So the idea is to introduce the new API send_signal to support such
doorbell/signal mode in mailbox controllers. This is useful to avoid
another layer of abstraction as typically multiple channels can be
multiplexied into single register.
Problem with ARM MHU driver in upstream
=======================================
It is designed to use the 32-bit registers(particularly SET) to send
32-bit data from one processor(AP) to another remote processor(SCP).
Basically it supports only single-word transfer protocol.
How is this related to SCMI ?
============================
Since SCMI was designed to replace all the existing vendor protocols,
it needs to be generic and the initial design/upstream drivers use
mailbox APIs as the standard interface to be able to talk/work with
any mailbox hardware controller. The SCMI specification itself was
designed based on ACPI PCC, which uses some shared memory for payload
and mailbox hardware just to signal the payload.
And this is very well aligned with the MHU hardware and the way firmware
team have used each bit as a separate channel.
So what's the problem then ?
============================
The mailbox APIs are not designed to cope with such doorbell mode of
mailbox operation. The mbox_send_data expects to send a (void *)data to
the controller and the interpretation of the same is left to the
controller and the protocol running a particular platform.
The main problem is that the strategy falls apart if one wants to use a
standard protocol like SCMI on variety of controllers.
Since the choice of transport protocol is platform dependent, each
mailbox controller driver can choose the protocol based on the platform
information from DT/ACPI.
Proposed solution
=================
The idea is to extend the mailbox APIs to support such a doorbell mode
of mailbox operation. The controller driver with the help of platform
firmware(DT for example) identify the mode of operation chosen by the
platform.
Why not add an additional abstraction on top of MHU/any mailbox controller ?
===========================================================================
As suggested during the review, I did attempt to build an abstraction
on top of mailbox driver using mailbox APIs. But soon ran into some
of the following issues/observations:
1. The resulting abstraction will look exactly like mailbox APIs, just
adding layers of indirection. It not only looks very ugly but also
duplicate queueing and other APIs that already exist in the mailbox
framework.
2. Not scalable as every controller that has similar issue to address
need to come up with different abstraction that suits it
3. It gets very ugly/complicated to represent this abstraction in DT
as this will be representation of some software construct and not
real hardware.
4. Performance hit as the abstraction layer gets serialised with one
Summary
=======
The mode in which a mailbox controller is configured to work is platform
specific and platform via DT/ACPI will inform about the same to OS.
The mailbox controller driver in OS need to support different/all modes
of transport possible and statically configure to one of the mode based
on the input from platform supplied information in DT.
--
Regards,
Sudeep
Sudeep Holla (6):
mailbox: add support for doorbell/signal mode controllers
mailbox: arm_mhu: reorder header inclusion and drop unneeded ones
dt-bindings: mailbox: add bindings to support ARM MHU doorbells
mailbox: arm_mhu: migrate to threaded irq handler
mailbox: arm_mhu: re-factor data structure to add doorbell support
mailbox: arm_mhu: add full support for the doorbells
.../devicetree/bindings/mailbox/arm-mhu.txt | 39 +-
drivers/mailbox/arm_mhu.c | 381 +++++++++++++++---
drivers/mailbox/mailbox.c | 11 +-
include/linux/mailbox_controller.h | 11 +
4 files changed, 389 insertions(+), 53 deletions(-)
--
2.17.1
In preparation to introduce support for doorbells which require the
interrupt handlers to be threaded, this patch moves the existing
interrupt handler to threaded handler.
Also it moves out the registering and freeing of the handlers from
the mailbox startup and shutdown methods. This also is required to
support doorbells.
Cc: Jassi Brar <[email protected]>
Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/mailbox/arm_mhu.c | 46 +++++++++++++++++++--------------------
1 file changed, 22 insertions(+), 24 deletions(-)
diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c
index 747cab1090ff..98838d5ae108 100644
--- a/drivers/mailbox/arm_mhu.c
+++ b/drivers/mailbox/arm_mhu.c
@@ -84,33 +84,16 @@ static int mhu_startup(struct mbox_chan *chan)
{
struct mhu_link *mlink = chan->con_priv;
u32 val;
- int ret;
val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS);
- ret = request_irq(mlink->irq, mhu_rx_interrupt,
- IRQF_SHARED, "mhu_link", chan);
- if (ret) {
- dev_err(chan->mbox->dev,
- "Unable to acquire IRQ %d\n", mlink->irq);
- return ret;
- }
-
return 0;
}
-static void mhu_shutdown(struct mbox_chan *chan)
-{
- struct mhu_link *mlink = chan->con_priv;
-
- free_irq(mlink->irq, chan);
-}
-
static const struct mbox_chan_ops mhu_ops = {
.send_data = mhu_send_data,
.startup = mhu_startup,
- .shutdown = mhu_shutdown,
.last_tx_done = mhu_last_tx_done,
};
@@ -132,13 +115,6 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
return PTR_ERR(mhu->base);
}
- for (i = 0; i < MHU_CHANS; i++) {
- mhu->chan[i].con_priv = &mhu->mlink[i];
- mhu->mlink[i].irq = adev->irq[i];
- mhu->mlink[i].rx_reg = mhu->base + mhu_reg[i];
- mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + TX_REG_OFFSET;
- }
-
mhu->mbox.dev = dev;
mhu->mbox.chans = &mhu->chan[0];
mhu->mbox.num_chans = MHU_CHANS;
@@ -155,6 +131,28 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
return err;
}
+ for (i = 0; i < MHU_CHANS; i++) {
+ int irq = mhu->mlink[i].irq = adev->irq[i];
+
+ if (irq <= 0) {
+ dev_dbg(dev, "No IRQ found for Channel %d\n", i);
+ continue;
+ }
+
+ mhu->chan[i].con_priv = &mhu->mlink[i];
+ mhu->mlink[i].rx_reg = mhu->base + mhu_reg[i];
+ mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + TX_REG_OFFSET;
+
+ err = devm_request_threaded_irq(dev, irq, NULL,
+ mhu_rx_interrupt, IRQF_ONESHOT,
+ "mhu_link", &mhu->chan[i]);
+ if (err) {
+ dev_err(dev, "Can't claim IRQ %d\n", irq);
+ mbox_controller_unregister(&mhu->mbox);
+ return err;
+ }
+ }
+
dev_info(dev, "ARM MHU Mailbox registered\n");
return 0;
}
--
2.17.1
We now have the basic infrastructure and binding to support doorbells
on ARM MHU controller.
This patch adds all the necessary mailbox operations to add support for
the doorbells. Maximum of 32 doorbells are supported on each physical
channel, however the total number of doorbells is restricted to 20
in order to save memory. It can increased if required in future.
Cc: Jassi Brar <[email protected]>
Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/mailbox/arm_mhu.c | 129 ++++++++++++++++++++++++++++++++++++--
1 file changed, 125 insertions(+), 4 deletions(-)
diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c
index c944ca121e9e..ba48d2281dca 100644
--- a/drivers/mailbox/arm_mhu.c
+++ b/drivers/mailbox/arm_mhu.c
@@ -18,6 +18,7 @@
#include <linux/err.h>
#include <linux/interrupt.h>
#include <linux/io.h>
+#include <linux/kernel.h>
#include <linux/mailbox_controller.h>
#include <linux/module.h>
#include <linux/of.h>
@@ -94,6 +95,14 @@ mhu_mbox_to_channel(struct mbox_controller *mbox,
return NULL;
}
+static void mhu_mbox_clear_irq(struct mbox_chan *chan)
+{
+ struct mhu_channel *chan_info = chan->con_priv;
+ void __iomem *base = chan_info->mhu->mlink[chan_info->pchan].rx_reg;
+
+ writel_relaxed(BIT(chan_info->doorbell), base + INTR_CLR_OFS);
+}
+
static unsigned int mhu_mbox_irq_to_pchan_num(struct arm_mhu *mhu, int irq)
{
unsigned int pchan;
@@ -105,6 +114,95 @@ static unsigned int mhu_mbox_irq_to_pchan_num(struct arm_mhu *mhu, int irq)
return pchan;
}
+static struct mbox_chan *mhu_mbox_irq_to_channel(struct arm_mhu *mhu,
+ unsigned int pchan)
+{
+ unsigned long bits;
+ unsigned int doorbell;
+ struct mbox_chan *chan = NULL;
+ struct mbox_controller *mbox = &mhu->mbox;
+ void __iomem *base = mhu->mlink[pchan].rx_reg;
+
+ bits = readl_relaxed(base + INTR_STAT_OFS);
+ if (!bits)
+ /* No IRQs fired in specified physical channel */
+ return NULL;
+
+ /* An IRQ has fired, find the associated channel */
+ for (doorbell = 0; bits; doorbell++) {
+ if (!test_and_clear_bit(doorbell, &bits))
+ continue;
+
+ chan = mhu_mbox_to_channel(mbox, pchan, doorbell);
+ if (chan)
+ break;
+ }
+
+ return chan;
+}
+
+static irqreturn_t mhu_mbox_thread_handler(int irq, void *data)
+{
+ struct mbox_chan *chan;
+ struct arm_mhu *mhu = data;
+ unsigned int pchan = mhu_mbox_irq_to_pchan_num(mhu, irq);
+
+ while (NULL != (chan = mhu_mbox_irq_to_channel(mhu, pchan))) {
+ mbox_chan_received_data(chan, NULL);
+ mhu_mbox_clear_irq(chan);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static bool mhu_doorbell_last_tx_done(struct mbox_chan *chan)
+{
+ struct mhu_channel *chan_info = chan->con_priv;
+ void __iomem *base = chan_info->mhu->mlink[chan_info->pchan].tx_reg;
+
+ if (readl_relaxed(base + INTR_STAT_OFS) & BIT(chan_info->doorbell))
+ return false;
+
+ return true;
+}
+
+static int mhu_doorbell_send_signal(struct mbox_chan *chan)
+{
+ struct mhu_channel *chan_info = chan->con_priv;
+ void __iomem *base = chan_info->mhu->mlink[chan_info->pchan].tx_reg;
+
+ /* Send event to co-processor */
+ writel_relaxed(BIT(chan_info->doorbell), base + INTR_SET_OFS);
+
+ return 0;
+}
+
+static int mhu_doorbell_startup(struct mbox_chan *chan)
+{
+ mhu_mbox_clear_irq(chan);
+ return 0;
+}
+
+static void mhu_doorbell_shutdown(struct mbox_chan *chan)
+{
+ struct mhu_channel *chan_info = chan->con_priv;
+ struct mbox_controller *mbox = &chan_info->mhu->mbox;
+ int i;
+
+ for (i = 0; i < mbox->num_chans; i++)
+ if (chan == &mbox->chans[i])
+ break;
+
+ if (mbox->num_chans == i) {
+ dev_warn(mbox->dev, "Request to free non-existent channel\n");
+ return;
+ }
+
+ /* Reset channel */
+ mhu_mbox_clear_irq(chan);
+ chan->con_priv = NULL;
+}
+
static struct mbox_chan *mhu_mbox_xlate(struct mbox_controller *mbox,
const struct of_phandle_args *spec)
{
@@ -222,16 +320,30 @@ static const struct mbox_chan_ops mhu_ops = {
.last_tx_done = mhu_last_tx_done,
};
+static const struct mbox_chan_ops mhu_doorbell_ops = {
+ .send_signal = mhu_doorbell_send_signal,
+ .startup = mhu_doorbell_startup,
+ .shutdown = mhu_doorbell_shutdown,
+ .last_tx_done = mhu_doorbell_last_tx_done,
+};
+
static const struct mhu_mbox_pdata arm_mhu_pdata = {
.num_pchans = 3,
.num_doorbells = 1,
.support_doorbells = false,
};
+static const struct mhu_mbox_pdata arm_mhu_doorbell_pdata = {
+ .num_pchans = 2, /* Secure can't be used */
+ .num_doorbells = 32,
+ .support_doorbells = true,
+};
+
static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
{
u32 cell_count;
int i, err, max_chans;
+ irq_handler_t handler;
struct arm_mhu *mhu;
struct mbox_chan *chans;
struct mhu_mbox_pdata *pdata;
@@ -251,6 +363,9 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
if (cell_count == 1) {
max_chans = MHU_NUM_PCHANS;
pdata = (struct mhu_mbox_pdata *)&arm_mhu_pdata;
+ } else if (cell_count == 2) {
+ max_chans = MHU_CHAN_MAX;
+ pdata = (struct mhu_mbox_pdata *)&arm_mhu_doorbell_pdata;
} else {
dev_err(dev, "incorrect value of #mbox-cells in %s\n",
np->full_name);
@@ -283,7 +398,6 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
mhu->mbox.dev = dev;
mhu->mbox.chans = chans;
mhu->mbox.num_chans = max_chans;
- mhu->mbox.ops = &mhu_ops;
mhu->mbox.txdone_irq = false;
mhu->mbox.txdone_poll = true;
mhu->mbox.txpoll_period = 1;
@@ -291,6 +405,14 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
mhu->mbox.of_xlate = mhu_mbox_xlate;
amba_set_drvdata(adev, mhu);
+ if (pdata->support_doorbells) {
+ mhu->mbox.ops = &mhu_doorbell_ops;
+ handler = mhu_mbox_thread_handler;
+ } else {
+ mhu->mbox.ops = &mhu_ops;
+ handler = mhu_rx_interrupt;
+ }
+
err = devm_mbox_controller_register(dev, &mhu->mbox);
if (err) {
dev_err(dev, "Failed to register mailboxes %d\n", err);
@@ -308,9 +430,8 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
mhu->mlink[i].rx_reg = mhu->base + mhu_reg[i];
mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + TX_REG_OFFSET;
- err = devm_request_threaded_irq(dev, irq, NULL,
- mhu_rx_interrupt, IRQF_ONESHOT,
- "mhu_link", mhu);
+ err = devm_request_threaded_irq(dev, irq, NULL, handler,
+ IRQF_ONESHOT, "mhu_link", mhu);
if (err) {
dev_err(dev, "Can't claim IRQ %d\n", irq);
mbox_controller_unregister(&mhu->mbox);
--
2.17.1
In order to support doorbells, we need a bit of reword around data
structures that are per-channel. Since the number of doorbells are
not fixed though restricted to maximum of 20, the channel assignment
and initialization is move to xlate function.
This patch also adds the platform data for the existing support of one
channel per physical channel.
Cc: Jassi Brar <[email protected]>
Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/mailbox/arm_mhu.c | 209 ++++++++++++++++++++++++++++++++++----
1 file changed, 187 insertions(+), 22 deletions(-)
diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c
index 98838d5ae108..c944ca121e9e 100644
--- a/drivers/mailbox/arm_mhu.c
+++ b/drivers/mailbox/arm_mhu.c
@@ -20,6 +20,8 @@
#include <linux/io.h>
#include <linux/mailbox_controller.h>
#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
#define INTR_STAT_OFS 0x0
#define INTR_SET_OFS 0x8
@@ -30,7 +32,8 @@
#define MHU_SEC_OFFSET 0x200
#define TX_REG_OFFSET 0x100
-#define MHU_CHANS 3
+#define MHU_NUM_PCHANS 3 /* Secure, Non-Secure High and Low Priority */
+#define MHU_CHAN_MAX 20 /* Max channels to save on unused RAM */
struct mhu_link {
unsigned irq;
@@ -40,53 +43,175 @@ struct mhu_link {
struct arm_mhu {
void __iomem *base;
- struct mhu_link mlink[MHU_CHANS];
- struct mbox_chan chan[MHU_CHANS];
+ struct mhu_link mlink[MHU_NUM_PCHANS];
struct mbox_controller mbox;
+ struct device *dev;
};
+/**
+ * ARM MHU Mailbox platform specific configuration
+ *
+ * @num_pchans: Maximum number of physical channels
+ * @num_doorbells: Maximum number of doorbells per physical channel
+ */
+struct mhu_mbox_pdata {
+ unsigned int num_pchans;
+ unsigned int num_doorbells;
+ bool support_doorbells;
+};
+
+/**
+ * ARM MHU Mailbox allocated channel information
+ *
+ * @mhu: Pointer to parent mailbox device
+ * @pchan: Physical channel within which this doorbell resides in
+ * @doorbell: doorbell number pertaining to this channel
+ */
+struct mhu_channel {
+ struct arm_mhu *mhu;
+ unsigned int pchan;
+ unsigned int doorbell;
+};
+
+static inline struct mbox_chan *
+mhu_mbox_to_channel(struct mbox_controller *mbox,
+ unsigned int pchan, unsigned int doorbell)
+{
+ int i;
+ struct mhu_channel *chan_info;
+
+ for (i = 0; i < mbox->num_chans; i++) {
+ chan_info = mbox->chans[i].con_priv;
+ if (chan_info && chan_info->pchan == pchan &&
+ chan_info->doorbell == doorbell)
+ return &mbox->chans[i];
+ }
+
+ dev_err(mbox->dev,
+ "Channel not registered: physical channel: %d doorbell: %d\n",
+ pchan, doorbell);
+
+ return NULL;
+}
+
+static unsigned int mhu_mbox_irq_to_pchan_num(struct arm_mhu *mhu, int irq)
+{
+ unsigned int pchan;
+ struct mhu_mbox_pdata *pdata = dev_get_platdata(mhu->dev);
+
+ for (pchan = 0; pchan < pdata->num_pchans; pchan++)
+ if (mhu->mlink[pchan].irq == irq)
+ break;
+ return pchan;
+}
+
+static struct mbox_chan *mhu_mbox_xlate(struct mbox_controller *mbox,
+ const struct of_phandle_args *spec)
+{
+ struct arm_mhu *mhu = dev_get_drvdata(mbox->dev);
+ struct mhu_mbox_pdata *pdata = dev_get_platdata(mhu->dev);
+ struct mhu_channel *chan_info;
+ struct mbox_chan *chan = NULL;
+ unsigned int pchan = spec->args[0];
+ unsigned int doorbell = pdata->support_doorbells ? spec->args[1] : 0;
+ int i;
+
+ /* Bounds checking */
+ if (pchan >= pdata->num_pchans || doorbell >= pdata->num_doorbells) {
+ dev_err(mbox->dev,
+ "Invalid channel requested pchan: %d doorbell: %d\n",
+ pchan, doorbell);
+ return ERR_PTR(-EINVAL);
+ }
+
+ for (i = 0; i < mbox->num_chans; i++) {
+ chan_info = mbox->chans[i].con_priv;
+
+ /* Is requested channel free? */
+ if (chan_info &&
+ mbox->dev == chan_info->mhu->dev &&
+ pchan == chan_info->pchan &&
+ doorbell == chan_info->doorbell) {
+ dev_err(mbox->dev, "Channel in use\n");
+ return ERR_PTR(-EBUSY);
+ }
+
+ /*
+ * Find the first free slot, then continue checking
+ * to see if requested channel is in use
+ */
+ if (!chan && !chan_info)
+ chan = &mbox->chans[i];
+ }
+
+ if (!chan) {
+ dev_err(mbox->dev, "No free channels left\n");
+ return ERR_PTR(-EBUSY);
+ }
+
+ chan_info = devm_kzalloc(mbox->dev, sizeof(*chan_info), GFP_KERNEL);
+ if (!chan_info)
+ return ERR_PTR(-ENOMEM);
+
+ chan_info->mhu = mhu;
+ chan_info->pchan = pchan;
+ chan_info->doorbell = doorbell;
+
+ chan->con_priv = chan_info;
+
+ dev_dbg(mbox->dev, "mbox: created channel phys: %d doorbell: %d\n",
+ pchan, doorbell);
+
+ return chan;
+}
+
static irqreturn_t mhu_rx_interrupt(int irq, void *p)
{
- struct mbox_chan *chan = p;
- struct mhu_link *mlink = chan->con_priv;
+ struct arm_mhu *mhu = p;
+ unsigned int pchan = mhu_mbox_irq_to_pchan_num(mhu, irq);
+ struct mbox_chan *chan = mhu_mbox_to_channel(&mhu->mbox, pchan, 0);
+ void __iomem *base = mhu->mlink[pchan].rx_reg;
u32 val;
- val = readl_relaxed(mlink->rx_reg + INTR_STAT_OFS);
+ val = readl_relaxed(base + INTR_STAT_OFS);
if (!val)
return IRQ_NONE;
mbox_chan_received_data(chan, (void *)&val);
- writel_relaxed(val, mlink->rx_reg + INTR_CLR_OFS);
+ writel_relaxed(val, base + INTR_CLR_OFS);
return IRQ_HANDLED;
}
static bool mhu_last_tx_done(struct mbox_chan *chan)
{
- struct mhu_link *mlink = chan->con_priv;
- u32 val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
+ struct mhu_channel *chan_info = chan->con_priv;
+ void __iomem *base = chan_info->mhu->mlink[chan_info->pchan].tx_reg;
+ u32 val = readl_relaxed(base + INTR_STAT_OFS);
return (val == 0);
}
static int mhu_send_data(struct mbox_chan *chan, void *data)
{
- struct mhu_link *mlink = chan->con_priv;
+ struct mhu_channel *chan_info = chan->con_priv;
+ void __iomem *base = chan_info->mhu->mlink[chan_info->pchan].tx_reg;
u32 *arg = data;
- writel_relaxed(*arg, mlink->tx_reg + INTR_SET_OFS);
+ writel_relaxed(*arg, base + INTR_SET_OFS);
return 0;
}
static int mhu_startup(struct mbox_chan *chan)
{
- struct mhu_link *mlink = chan->con_priv;
+ struct mhu_channel *chan_info = chan->con_priv;
+ void __iomem *base = chan_info->mhu->mlink[chan_info->pchan].tx_reg;
u32 val;
- val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
- writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS);
+ val = readl_relaxed(base + INTR_STAT_OFS);
+ writel_relaxed(val, base + INTR_CLR_OFS);
return 0;
}
@@ -97,14 +222,47 @@ static const struct mbox_chan_ops mhu_ops = {
.last_tx_done = mhu_last_tx_done,
};
+static const struct mhu_mbox_pdata arm_mhu_pdata = {
+ .num_pchans = 3,
+ .num_doorbells = 1,
+ .support_doorbells = false,
+};
+
static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
{
- int i, err;
+ u32 cell_count;
+ int i, err, max_chans;
struct arm_mhu *mhu;
+ struct mbox_chan *chans;
+ struct mhu_mbox_pdata *pdata;
struct device *dev = &adev->dev;
- int mhu_reg[MHU_CHANS] = {MHU_LP_OFFSET, MHU_HP_OFFSET, MHU_SEC_OFFSET};
+ struct device_node *np = dev->of_node;
+ int mhu_reg[MHU_NUM_PCHANS] = {
+ MHU_LP_OFFSET, MHU_HP_OFFSET, MHU_SEC_OFFSET,
+ };
+
+ err = of_property_read_u32(np, "#mbox-cells", &cell_count);
+ if (err) {
+ dev_err(dev, "failed to read #mbox-cells in %s\n",
+ np->full_name);
+ return err;
+ }
+
+ if (cell_count == 1) {
+ max_chans = MHU_NUM_PCHANS;
+ pdata = (struct mhu_mbox_pdata *)&arm_mhu_pdata;
+ } else {
+ dev_err(dev, "incorrect value of #mbox-cells in %s\n",
+ np->full_name);
+ return -EINVAL;
+ }
+
+ if (pdata->num_pchans > MHU_NUM_PCHANS) {
+ dev_err(dev, "Number of physical channel can't exceed %d\n",
+ MHU_NUM_PCHANS);
+ return -EINVAL;
+ }
- /* Allocate memory for device */
mhu = devm_kzalloc(dev, sizeof(*mhu), GFP_KERNEL);
if (!mhu)
return -ENOMEM;
@@ -115,14 +273,22 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
return PTR_ERR(mhu->base);
}
+ chans = devm_kcalloc(dev, max_chans, sizeof(*chans), GFP_KERNEL);
+ if (!chans)
+ return -ENOMEM;
+
+ dev->platform_data = pdata;
+
+ mhu->dev = dev;
mhu->mbox.dev = dev;
- mhu->mbox.chans = &mhu->chan[0];
- mhu->mbox.num_chans = MHU_CHANS;
+ mhu->mbox.chans = chans;
+ mhu->mbox.num_chans = max_chans;
mhu->mbox.ops = &mhu_ops;
mhu->mbox.txdone_irq = false;
mhu->mbox.txdone_poll = true;
mhu->mbox.txpoll_period = 1;
+ mhu->mbox.of_xlate = mhu_mbox_xlate;
amba_set_drvdata(adev, mhu);
err = devm_mbox_controller_register(dev, &mhu->mbox);
@@ -131,7 +297,7 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
return err;
}
- for (i = 0; i < MHU_CHANS; i++) {
+ for (i = 0; i < pdata->num_pchans; i++) {
int irq = mhu->mlink[i].irq = adev->irq[i];
if (irq <= 0) {
@@ -139,13 +305,12 @@ static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
continue;
}
- mhu->chan[i].con_priv = &mhu->mlink[i];
mhu->mlink[i].rx_reg = mhu->base + mhu_reg[i];
mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + TX_REG_OFFSET;
err = devm_request_threaded_irq(dev, irq, NULL,
mhu_rx_interrupt, IRQF_ONESHOT,
- "mhu_link", &mhu->chan[i]);
+ "mhu_link", mhu);
if (err) {
dev_err(dev, "Can't claim IRQ %d\n", irq);
mbox_controller_unregister(&mhu->mbox);
--
2.17.1
This patch just re-orders some of the headers includes and also drop
the ones that are unnecessary.
Cc: Jassi Brar <[email protected]>
Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/mailbox/arm_mhu.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c
index 64d85c6a2bdf..747cab1090ff 100644
--- a/drivers/mailbox/arm_mhu.c
+++ b/drivers/mailbox/arm_mhu.c
@@ -13,16 +13,13 @@
* GNU General Public License for more details.
*/
-#include <linux/interrupt.h>
-#include <linux/spinlock.h>
-#include <linux/mutex.h>
-#include <linux/delay.h>
-#include <linux/slab.h>
+#include <linux/amba/bus.h>
+#include <linux/device.h>
#include <linux/err.h>
+#include <linux/interrupt.h>
#include <linux/io.h>
-#include <linux/module.h>
-#include <linux/amba/bus.h>
#include <linux/mailbox_controller.h>
+#include <linux/module.h>
#define INTR_STAT_OFS 0x0
#define INTR_SET_OFS 0x8
--
2.17.1
The ARM MHU has mechanism to assert interrupt signals to facilitate
inter-processor message based communication. It drives the signal using
a 32-bit register, with all 32-bits logically ORed together. It also
enables software to set, clear and check the status of each of the bits
of this register independently. Each bit of the register can be
associated with a type of event that can contribute to raising the
interrupt thereby allowing it to be used as independent doorbells.
Since the first version of this binding can't support doorbells,
this patch extends the existing binding to support them by allowing
"#mbox-cells" to be 2.
Cc: Jassi Brar <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: [email protected]
Signed-off-by: Sudeep Holla <[email protected]>
---
.../devicetree/bindings/mailbox/arm-mhu.txt | 39 ++++++++++++++++++-
1 file changed, 37 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
index 4971f03f0b33..ba659bcc7109 100644
--- a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
+++ b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
@@ -10,6 +10,15 @@ STAT register and the remote clears it after having read the data.
The last channel is specified to be a 'Secure' resource, hence can't be
used by Linux running NS.
+The MHU drives the interrupt signal using a 32-bit register, with all
+32-bits logically ORed together. It provides a set of registers to
+enable software to set, clear and check the status of each of the bits
+of this register independently. The use of 32 bits per interrupt line
+enables software to provide more information about the source of the
+interrupt. For example, each bit of the register can be associated with
+a type of event that can contribute to raising the interrupt. Each of
+the 32-bits can be used as "doorbell" to alert the remote processor.
+
Mailbox Device Node:
====================
@@ -18,13 +27,21 @@ used by Linux running NS.
- compatible: Shall be "arm,mhu" & "arm,primecell"
- reg: Contains the mailbox register address range (base
address and length)
-- #mbox-cells Shall be 1 - the index of the channel needed.
+- #mbox-cells Shall be 1 - the index of the channel needed when
+ not used as set of doorbell bits.
+ Shall be 2 - the index of the channel needed, and
+ the index of the doorbell bit within the channel
+ when used in doorbell mode.
- interrupts: Contains the interrupt information corresponding to
- each of the 3 links of MHU.
+ each of the 3 physical channels of MHU namely low
+ priority non-secure, high priority non-secure and
+ secure channels.
Example:
--------
+1. Controller which doesn't support doorbells
+
mhu: mailbox@2b1f0000 {
#mbox-cells = <1>;
compatible = "arm,mhu", "arm,primecell";
@@ -41,3 +58,21 @@ used by Linux running NS.
reg = <0 0x2e000000 0x4000>;
mboxes = <&mhu 1>; /* HP-NonSecure */
};
+
+2. Controller which supports doorbells
+
+ mhu: mailbox@2b1f0000 {
+ #mbox-cells = <2>;
+ compatible = "arm,mhu", "arm,primecell";
+ reg = <0 0x2b1f0000 0x1000>;
+ interrupts = <0 36 4>, /* LP-NonSecure */
+ <0 35 4>; /* HP-NonSecure */
+ clocks = <&clock 0 2 1>;
+ clock-names = "apb_pclk";
+ };
+
+ mhu_client: scb@2e000000 {
+ compatible = "arm,scpi";
+ reg = <0 0x2e000000 0x200>;
+ mboxes = <&mhu 1 4>; /* HP-NonSecure 5th doorbell bit */
+ };
--
2.17.1
Some mailbox controllers are lack FIFOs or memory to transmit data.
They typically contains single doorbell registers to just signal the
remote. The actually data is transmitted/shared using some shared memory
which is not part of the mailbox.
Such controllers don't need to transmit any data, they just transmit
the signal. In such controllers the data pointer passed to
mbox_send_message is passed to client via it's tx_prepare callback.
Controller doesn't need any data to be passed from the client.
This patch introduce the new API send_signal to support such doorbell/
signal mode in mailbox controllers. This is useful to avoid another
layer of abstraction as typically multiple channels can be multiplexied
into single register.
Cc: Jassi Brar <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Bjorn Andersson <[email protected]>
Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/mailbox/mailbox.c | 11 ++++++++++-
include/linux/mailbox_controller.h | 11 +++++++++++
2 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 38d9df3fb199..e26a079f8223 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -77,7 +77,10 @@ static void msg_submit(struct mbox_chan *chan)
if (chan->cl->tx_prepare)
chan->cl->tx_prepare(chan->cl, data);
/* Try to submit a message to the MBOX controller */
- err = chan->mbox->ops->send_data(chan, data);
+ if (chan->mbox->ops->send_data)
+ err = chan->mbox->ops->send_data(chan, data);
+ else
+ err = chan->mbox->ops->send_signal(chan);
if (!err) {
chan->active_req = data;
chan->msg_count--;
@@ -481,6 +484,12 @@ int mbox_controller_register(struct mbox_controller *mbox)
/* Sanity check */
if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans)
return -EINVAL;
+ /*
+ * A controller can support either doorbell mode or normal message
+ * transmission mode but not both
+ */
+ if (mbox->ops->send_data && mbox->ops->send_signal)
+ return -EINVAL;
if (mbox->txdone_irq)
txdone = TXDONE_BY_IRQ;
diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
index 4994a438444c..b3f547ad782a 100644
--- a/include/linux/mailbox_controller.h
+++ b/include/linux/mailbox_controller.h
@@ -24,6 +24,16 @@ struct mbox_chan;
* transmission of data is reported by the controller via
* mbox_chan_txdone (if it has some TX ACK irq). It must not
* sleep.
+ * @send_signal: The API asks the MBOX controller driver, in atomic
+ * context try to transmit a signal on the bus. Returns 0 if
+ * data is accepted for transmission, -EBUSY while rejecting
+ * if the remote hasn't yet absorbed the last signal sent. Actual
+ * transmission of data must be handled by the client and is
+ * reported by the controller via mbox_chan_txdone (if it has
+ * some TX ACK irq). It must not sleep. Unlike send_data,
+ * send_signal doesn't handle any messages/data. It just sends
+ * notification signal(doorbell) and client needs to prepare all
+ * the data.
* @flush: Called when a client requests transmissions to be blocking but
* the context doesn't allow sleeping. Typically the controller
* will implement a busy loop waiting for the data to flush out.
@@ -49,6 +59,7 @@ struct mbox_chan;
*/
struct mbox_chan_ops {
int (*send_data)(struct mbox_chan *chan, void *data);
+ int (*send_signal)(struct mbox_chan *chan);
int (*flush)(struct mbox_chan *chan, unsigned long timeout);
int (*startup)(struct mbox_chan *chan);
void (*shutdown)(struct mbox_chan *chan);
--
2.17.1
Hi Sudeep,
On Fri, May 31, 2019 at 9:33 AM Sudeep Holla <[email protected]> wrote:
>
> This is my another attempt to extend mailbox framework to support
> doorbell mode mailbox hardware. It also adds doorbell support to ARM
> MHU driver.
>
Nothing has really changed since the last time we discussed many months ago.
MHU remains same, and so are my points.
>
> Brief hardware description about MHU
> ====================================
>
> For each of the interrupt signals, the MHU drives the signal using a 32-bit
> register, with all 32 bits logically ORed together. The MHU provides a set of
> registers to enable software to SET, CLEAR, and check the STATUS of each of
> the bits of this register independently. The use of 32 bits for each interrupt
> line enables software to provide more information about the source of the
> interrupt.
>
"32 bits for each interrupt line" => 32 virtual channels or 32bit
data over one physical channel. And that is how the driver is
currently written.
> For example, each bit of the register can be associated with a type
> of event that can contribute to raising the interrupt.
>
Sure there can be a usecase where each bit is associated with an event
(virtual channel). As you said, this is just one example of how MHU
can be used. There are other ways MHU is used already.
Patch-2/6 looks good though. I will pick that up.
Thanks.
On Fri, May 31, 2019 at 11:21:08AM -0500, Jassi Brar wrote:
> Hi Sudeep,
>
> On Fri, May 31, 2019 at 9:33 AM Sudeep Holla <[email protected]> wrote:
> >
> > This is my another attempt to extend mailbox framework to support
> > doorbell mode mailbox hardware. It also adds doorbell support to ARM
> > MHU driver.
> >
> Nothing has really changed since the last time we discussed many months ago.
> MHU remains same, and so are my points.
>
Yes, I understand your concern.
But as mentioned in the cover letter I did try the suggestions and have
detailed reasoning why that's still an issue. In short I ended up
re-inventing mailbox framework with all the queuing and similar APIs
for this. Worse, we can't even add an extra node for that in DT to
describe that. It can't be simple shim as we need to allow multiple
users to access one physical channel at a time. We have use case
where we can this for CPU DVFS fast switching in scheduler context.
> >
> > Brief hardware description about MHU
> > ====================================
> >
> > For each of the interrupt signals, the MHU drives the signal using a 32-bit
> > register, with all 32 bits logically ORed together. The MHU provides a set of
> > registers to enable software to SET, CLEAR, and check the STATUS of each of
> > the bits of this register independently. The use of 32 bits for each interrupt
> > line enables software to provide more information about the source of the
> > interrupt.
> >
> "32 bits for each interrupt line" => 32 virtual channels or 32bit
> data over one physical channel. And that is how the driver is
> currently written.
>
Yes, I understand.
> > For example, each bit of the register can be associated with a type
> > of event that can contribute to raising the interrupt.
> >
> Sure there can be a usecase where each bit is associated with an event
> (virtual channel). As you said, this is just one example of how MHU
> can be used. There are other ways MHU is used already.
>
Agreed and I am not saying that's incorrect or asking to remove
support for that. Future versions of MHU are making it more complex
and the specification classify the 3 modes of transport protocol in which
the new controller can be configured and used.
The choice is left to platform based on use case to get best/optimal
results. That's the reason I am trying to convince you that we need to
support all those configurations/transport protocols in the Linux
mailbox controller driver. As you mention one use-case of MHU, the word
transfer with 2^32 - 1 options is optimal for IoT use-case where as
doorbell mode is optimal for heavy payloads. The newer versions of
MHU are designed keeping both configurations in mind and the same is
suggested by h/w teams to
various vendors.
Arnd has similar suggestions(something like patch 1) to deal with such
configurations/transport protocols. Please note I am referring to
different transport protocols and not message protocols(SCPI/SCMI fall
under this category)
> Patch-2/6 looks good though. I will pick that up.
>
Thanks.
--
Regards,
Sudeep
On Fri, May 31, 2019 at 05:53:26PM +0100, Sudeep Holla wrote:
> On Fri, May 31, 2019 at 11:21:08AM -0500, Jassi Brar wrote:
> > On Fri, May 31, 2019 at 9:33 AM Sudeep Holla <[email protected]> wrote:
> > > This is my another attempt to extend mailbox framework to support
> > > doorbell mode mailbox hardware. It also adds doorbell support to ARM
> > > MHU driver.
> > Nothing has really changed since the last time we discussed many months ago.
> > MHU remains same, and so are my points.
> Yes, I understand your concern.
> But as mentioned in the cover letter I did try the suggestions and have
> detailed reasoning why that's still an issue. In short I ended up
> re-inventing mailbox framework with all the queuing and similar APIs
> for this. Worse, we can't even add an extra node for that in DT to
> describe that. It can't be simple shim as we need to allow multiple
> users to access one physical channel at a time. We have use case
> where we can this for CPU DVFS fast switching in scheduler context.
Forgive me if I'm missing something here (this is partly based on
conversations from months ago so I may be misremembering things) but is
the issue here specifically the doorbell mode or is it the need to have
partly software defined mailboxes implemented using this hardware? My
understanding is that the hardware is more a component that's intended
to allow potentially multiple more complex mailboxes to be tied to a
single hardware block than a complete mailbox in and of itself. It
feels like the issues with sharing access to the hardware and with the
API for talking to doorbell hardware are getting tied together and
confusing things. But like I say I might be missing something here.
On Fri, May 31, 2019 at 9:33 AM Sudeep Holla <[email protected]> wrote:
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> index 38d9df3fb199..e26a079f8223 100644
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -77,7 +77,10 @@ static void msg_submit(struct mbox_chan *chan)
> if (chan->cl->tx_prepare)
> chan->cl->tx_prepare(chan->cl, data);
> /* Try to submit a message to the MBOX controller */
> - err = chan->mbox->ops->send_data(chan, data);
> + if (chan->mbox->ops->send_data)
> + err = chan->mbox->ops->send_data(chan, data);
> + else
> + err = chan->mbox->ops->send_signal(chan);
> if (!err) {
> chan->active_req = data;
> chan->msg_count--;
>
The 'void *data' parameter in send_data() is controller specific.
The doorbell controllers should simply ignore that.
So signal/doorbell controllers are already supported fine. See
drivers/mailbox/tegra-hsp.c for example.
Thanks.
On Mon, Jun 03, 2019 at 04:51:05PM -0500, Jassi Brar wrote:
> On Fri, May 31, 2019 at 9:33 AM Sudeep Holla <[email protected]> wrote:
>
> > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> > index 38d9df3fb199..e26a079f8223 100644
> > --- a/drivers/mailbox/mailbox.c
> > +++ b/drivers/mailbox/mailbox.c
> > @@ -77,7 +77,10 @@ static void msg_submit(struct mbox_chan *chan)
> > if (chan->cl->tx_prepare)
> > chan->cl->tx_prepare(chan->cl, data);
> > /* Try to submit a message to the MBOX controller */
> > - err = chan->mbox->ops->send_data(chan, data);
> > + if (chan->mbox->ops->send_data)
> > + err = chan->mbox->ops->send_data(chan, data);
> > + else
> > + err = chan->mbox->ops->send_signal(chan);
> > if (!err) {
> > chan->active_req = data;
> > chan->msg_count--;
> >
> The 'void *data' parameter in send_data() is controller specific.
> The doorbell controllers should simply ignore that.
>
> So signal/doorbell controllers are already supported fine. See
> drivers/mailbox/tegra-hsp.c for example.
>
Agreed, I did have that. But then I thought this API makes it even
clearer to the users that no data is expected. I am fine either way.
--
Regards,
Sudeep
On Mon, Jun 03, 2019 at 08:39:46PM +0100, Mark Brown wrote:
> On Fri, May 31, 2019 at 05:53:26PM +0100, Sudeep Holla wrote:
> > On Fri, May 31, 2019 at 11:21:08AM -0500, Jassi Brar wrote:
> > > On Fri, May 31, 2019 at 9:33 AM Sudeep Holla <[email protected]> wrote:
>
> > > > This is my another attempt to extend mailbox framework to support
> > > > doorbell mode mailbox hardware. It also adds doorbell support to ARM
> > > > MHU driver.
>
> > > Nothing has really changed since the last time we discussed many months ago.
> > > MHU remains same, and so are my points.
>
> > Yes, I understand your concern.
>
> > But as mentioned in the cover letter I did try the suggestions and have
> > detailed reasoning why that's still an issue. In short I ended up
> > re-inventing mailbox framework with all the queuing and similar APIs
> > for this. Worse, we can't even add an extra node for that in DT to
> > describe that. It can't be simple shim as we need to allow multiple
> > users to access one physical channel at a time. We have use case
> > where we can this for CPU DVFS fast switching in scheduler context.
>
> Forgive me if I'm missing something here (this is partly based on
> conversations from months ago so I may be misremembering things) but is
> the issue here specifically the doorbell mode or is it the need to have
> partly software defined mailboxes implemented using this hardware?
I can say it's partially both.
1. The hardware is designed keeping in mind multiple transport protocols:
doorbell mode, single word and multiple work(only in newer versions)
Using that hardware capability provides access to multiple channels
to the software.
2. I can also view this as software defined mailboxes if we go by
definition that each channel should have associated dedicated interrupt
as Jassi mentions.
The main idea is that each bit in these 32-bit registers can be written
atomically without the need of read-modify-write enabling software to
implement multiple channels in lock-less way.
> My understanding is that the hardware is more a component that's intended
> to allow potentially multiple more complex mailboxes to be tied to a
> single hardware block than a complete mailbox in and of itself.
Correct.
> It feels like the issues with sharing access to the hardware and with the
> API for talking to doorbell hardware are getting tied together and
> confusing things. But like I say I might be missing something here.
As I tried to simply in my cover letter, I will try to explain in simpler
terms.
1. This version of hardware has 3 blocks(one for secure and 2 non-secure)
Each block has 3 sets of 32-bit registers(SET, CLEAR and STATUS)
SET and CLEAR are write only and STATUS is read-only.
Each block has a dedicated interrupt line.
2. The hardware was designed to cater 2 transport protocols. A single
word transfer(non-zero) or each bit in doorbell mode.
3. The next version extends with each block having larger than 32-bit
window(up to 124 words) allowing it to used it for multiple
word as transport protocol. Mainly for some IoT usecase.
So what I am trying to convey here is MHU controller hardware can be
used choosing one of the different transport protocols available and
that's platform choice based on the use-case.
The driver in the kernel should identify the same from the firmware/DT
and configure it appropriately.
It may get inefficient and sometime impossible to address all use-case
if we stick to one transport protocol in the driver and try to build
an abstraction on top to use in different transport mode.
Hope this clarify things little bit more.
--
Regards,
Sudeep
On Tue, Jun 04, 2019 at 10:44:24AM +0100, Sudeep Holla wrote:
> On Mon, Jun 03, 2019 at 08:39:46PM +0100, Mark Brown wrote:
>
> > It feels like the issues with sharing access to the hardware and with the
> > API for talking to doorbell hardware are getting tied together and
> > confusing things. But like I say I might be missing something here.
...
> So what I am trying to convey here is MHU controller hardware can be
> used choosing one of the different transport protocols available and
> that's platform choice based on the use-case.
> The driver in the kernel should identify the same from the firmware/DT
> and configure it appropriately.
> It may get inefficient and sometime impossible to address all use-case
> if we stick to one transport protocol in the driver and try to build
> an abstraction on top to use in different transport mode.
Right, what I was trying to get at was that it feels like the discussion
is getting wrapped up in the specifics of the MHU rather than
representing this sort of controller with multiple modes in the
framework. It's important for establishing the use case but ultimately
a separate issue.
On Wed, Jun 5, 2019 at 2:46 PM Mark Brown <[email protected]> wrote:
>
> On Tue, Jun 04, 2019 at 10:44:24AM +0100, Sudeep Holla wrote:
> > On Mon, Jun 03, 2019 at 08:39:46PM +0100, Mark Brown wrote:
>
> >
> > > It feels like the issues with sharing access to the hardware and with the
> > > API for talking to doorbell hardware are getting tied together and
> > > confusing things. But like I say I might be missing something here.
>
> ...
>
> > So what I am trying to convey here is MHU controller hardware can be
> > used choosing one of the different transport protocols available and
> > that's platform choice based on the use-case.
>
> > The driver in the kernel should identify the same from the firmware/DT
> > and configure it appropriately.
>
> > It may get inefficient and sometime impossible to address all use-case
> > if we stick to one transport protocol in the driver and try to build
> > an abstraction on top to use in different transport mode.
>
> Right, what I was trying to get at was that it feels like the discussion
> is getting wrapped up in the specifics of the MHU rather than
> representing this sort of controller with multiple modes in the
> framework.
>
Usually when a controller could be used in more than one way, we
implement the more generic usecase. And that's what was done for MHU.
Implementing doorbell scheme would have disallowed mhu platforms that
don't have any shmem between the endpoints. Now such platforms could
use 32bits registers to pass/get data. Meanwhile doorbells could be
emulated in client code.
Also, next version of MHU has many (100?) such 32bit registers per
interrupt. Clearly those are not meant to be seen as 3200 doorbells,
but as message passing windows. (or maybe that is an almost different
controller because of the differences)
BTW, this is not going to be the end of SCMI troubles (I believe
that's what his client is). SCMI will eventually have to be broken up
in layers (protocol and transport) for many legit platforms to use it.
That is mbox_send_message() will have to be replaced by, say,
platform_mbox_send() in drivers/firmware/arm_scmi/driver.c OR the
platforms have to have shmem and each mailbox controller driver (that
could ever be used under scmi) will have to implement "doorbell
emulation" mode. That is the reason I am not letting the way paved for
such emulations.
Thanks
On Thu, Jun 06, 2019 at 10:20:40AM -0500, Jassi Brar wrote:
> On Thu, Jun 6, 2019 at 7:51 AM Sudeep Holla <[email protected]> wrote:
>
> >
> > > BTW, this is not going to be the end of SCMI troubles (I believe
> > > that's what his client is). SCMI will eventually have to be broken up
> > > in layers (protocol and transport) for many legit platforms to use it.
> > > That is mbox_send_message() will have to be replaced by, say,
> > > platform_mbox_send() in drivers/firmware/arm_scmi/driver.c OR the
> > > platforms have to have shmem and each mailbox controller driver (that
> > > could ever be used under scmi) will have to implement "doorbell
> > > emulation" mode. That is the reason I am not letting the way paved for
> > > such emulations.
> > >
> >
> > While I don't dislike or disagree with separate transport in SCMI which
> > I have invested time and realised that I will duplicate mailbox framework
> > at the end.
> >
> Can you please share the code? Or is it no more available?
>
> > So I am against it only because of duplication and extra
> > layer of indirection which has performance impact(we have this seen in
> > sched governor for DVFS).
> >
> I don't see why the overhead should increase noticeably.
>
Simple, if 2 protocols share the same channel, then the requests are
serialised. E.g. if bits 0 and 1 are allocated for protocol#1
and bits 2 and 3 for protocol#2 and protocol#1 has higher latency
requirements like sched-governor DVFS and there are 3-4 pending requests
on protocol#2, then the incoming request for protocol#1 is blocked.
> > So idea wise, it's good and I don't disagree
> > with practically seen performance impact. Hence I thought it's sane to
> > do something I am proposing.
> >
> Please suggest how is SCMI supposed to work on ~15 controllers
> upstream (except tegra-hsp) ?
>
Do you mean we have to implement platform layer to make it work ?
That's not necessary IMO.
> > It also avoids coming up with virtual DT
> > nodes for this layer of abstract which I am completely against.
> >
> I don't see why virtual DT nodes would be needed for platform layer.
So how will 2 or more different users of the same mailbox identify the
bits allocated for them ?
--
Regards,
Sudeep
On Wed, Jun 05, 2019 at 07:51:12PM -0500, Jassi Brar wrote:
> On Wed, Jun 5, 2019 at 2:46 PM Mark Brown <[email protected]> wrote:
> >
> > On Tue, Jun 04, 2019 at 10:44:24AM +0100, Sudeep Holla wrote:
> > > On Mon, Jun 03, 2019 at 08:39:46PM +0100, Mark Brown wrote:
> >
> > >
> > > > It feels like the issues with sharing access to the hardware and with the
> > > > API for talking to doorbell hardware are getting tied together and
> > > > confusing things. But like I say I might be missing something here.
> >
> > ...
> >
> > > So what I am trying to convey here is MHU controller hardware can be
> > > used choosing one of the different transport protocols available and
> > > that's platform choice based on the use-case.
> >
> > > The driver in the kernel should identify the same from the firmware/DT
> > > and configure it appropriately.
> >
> > > It may get inefficient and sometime impossible to address all use-case
> > > if we stick to one transport protocol in the driver and try to build
> > > an abstraction on top to use in different transport mode.
> >
> > Right, what I was trying to get at was that it feels like the discussion
> > is getting wrapped up in the specifics of the MHU rather than
> > representing this sort of controller with multiple modes in the
> > framework.
> >
> Usually when a controller could be used in more than one way, we
> implement the more generic usecase. And that's what was done for MHU.
That's debatable and we have done that so extensively so far.
So what I am saying is to implement different modes not just one so that
as many use-case are addressed.
> Implementing doorbell scheme would have disallowed mhu platforms that
> don't have any shmem between the endpoints. Now such platforms could
> use 32bits registers to pass/get data. Meanwhile doorbells could be
> emulated in client code.
> Also, next version of MHU has many (100?) such 32bit registers per
> interrupt. Clearly those are not meant to be seen as 3200 doorbells,
> but as message passing windows. (or maybe that is an almost different
> controller because of the differences)
>
I disagree. It's configurable and vendors can just choose 2 instead of
100s as you mentioned based on the use-case and needs. So we will still
need the same there.
> BTW, this is not going to be the end of SCMI troubles (I believe
> that's what his client is). SCMI will eventually have to be broken up
> in layers (protocol and transport) for many legit platforms to use it.
> That is mbox_send_message() will have to be replaced by, say,
> platform_mbox_send() in drivers/firmware/arm_scmi/driver.c OR the
> platforms have to have shmem and each mailbox controller driver (that
> could ever be used under scmi) will have to implement "doorbell
> emulation" mode. That is the reason I am not letting the way paved for
> such emulations.
>
While I don't dislike or disagree with separate transport in SCMI which
I have invested time and realised that I will duplicate mailbox framework
at the end. So I am against it only because of duplication and extra
layer of indirection which has performance impact(we have this seen in
sched governor for DVFS). So idea wise, it's good and I don't disagree
with practically seen performance impact. Hence I thought it's sane to
do something I am proposing. It also avoids coming up with virtual DT
nodes for this layer of abstract which I am completely against.
--
Regards,
Sudeep
On Thu, Jun 6, 2019 at 7:51 AM Sudeep Holla <[email protected]> wrote:
>
> > BTW, this is not going to be the end of SCMI troubles (I believe
> > that's what his client is). SCMI will eventually have to be broken up
> > in layers (protocol and transport) for many legit platforms to use it.
> > That is mbox_send_message() will have to be replaced by, say,
> > platform_mbox_send() in drivers/firmware/arm_scmi/driver.c OR the
> > platforms have to have shmem and each mailbox controller driver (that
> > could ever be used under scmi) will have to implement "doorbell
> > emulation" mode. That is the reason I am not letting the way paved for
> > such emulations.
> >
>
> While I don't dislike or disagree with separate transport in SCMI which
> I have invested time and realised that I will duplicate mailbox framework
> at the end.
>
Can you please share the code? Or is it no more available?
> So I am against it only because of duplication and extra
> layer of indirection which has performance impact(we have this seen in
> sched governor for DVFS).
>
I don't see why the overhead should increase noticeably.
> So idea wise, it's good and I don't disagree
> with practically seen performance impact. Hence I thought it's sane to
> do something I am proposing.
>
Please suggest how is SCMI supposed to work on ~15 controllers
upstream (except tegra-hsp) ?
> It also avoids coming up with virtual DT
> nodes for this layer of abstract which I am completely against.
>
I don't see why virtual DT nodes would be needed for platform layer.
On Thu, Jun 06, 2019 at 04:40:45PM +0100, Sudeep Holla wrote:
> On Thu, Jun 06, 2019 at 10:20:40AM -0500, Jassi Brar wrote:
> > On Thu, Jun 6, 2019 at 7:51 AM Sudeep Holla <[email protected]> wrote:
> >
> > >
> > > > BTW, this is not going to be the end of SCMI troubles (I believe
> > > > that's what his client is). SCMI will eventually have to be broken up
> > > > in layers (protocol and transport) for many legit platforms to use it.
> > > > That is mbox_send_message() will have to be replaced by, say,
> > > > platform_mbox_send() in drivers/firmware/arm_scmi/driver.c OR the
> > > > platforms have to have shmem and each mailbox controller driver (that
> > > > could ever be used under scmi) will have to implement "doorbell
> > > > emulation" mode. That is the reason I am not letting the way paved for
> > > > such emulations.
> > > >
> > >
> > > While I don't dislike or disagree with separate transport in SCMI which
> > > I have invested time and realised that I will duplicate mailbox framework
> > > at the end.
> > >
> > Can you please share the code? Or is it no more available?
> >
> > > So I am against it only because of duplication and extra
> > > layer of indirection which has performance impact(we have this seen in
> > > sched governor for DVFS).
> > >
> > I don't see why the overhead should increase noticeably.
> >
>
> Simple, if 2 protocols share the same channel, then the requests are
> serialised. E.g. if bits 0 and 1 are allocated for protocol#1
> and bits 2 and 3 for protocol#2 and protocol#1 has higher latency
> requirements like sched-governor DVFS and there are 3-4 pending requests
> on protocol#2, then the incoming request for protocol#1 is blocked.
>
Any idea on addressing the above with abstraction layer above the driver ?
And the bit allotment without the virtual channel representation in DT.
These 2 are main issues that needs to be resolved.
--
Regards,
Sudeep