Discussion:
[PATCH v2 0/7] FlexRM driver improvements
Anup Patel
2017-07-21 06:55:35 UTC
Permalink
This patchset does various improvments to Broadcom FlexRM
mailbox controller driver and also adds FlexRM DT nodes
for Stingray SOC.

The patches are based on Linux-4.13-rc1 and can also be
found at flexrm-imp-v2 branch of
https://github.com/Broadcom/arm64-linux.git

Changes since v1:
- Add one more patch to use bitmap instead of IDA in FlexRM driver

Anup Patel (7):
mailbox: bcm-flexrm-mailbox: Set IRQ affinity hint for FlexRM ring
IRQs
mailbox: bcm-flexrm-mailbox: Add debugfs support
mailbox: bcm-flexrm-mailbox: Fix mask used in CMPL_START_ADDR_VALUE()
mailbox: bcm-flexrm-mailbox: Use bitmap instead of IDA
mailbox: Make message send queue size dynamic in Linux mailbox
mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel
arm64: dts: Add FlexRM DT nodes for Stingray

.../boot/dts/broadcom/stingray/stingray-fs4.dtsi | 54 ++++++
.../arm64/boot/dts/broadcom/stingray/stingray.dtsi | 2 +
drivers/mailbox/bcm-flexrm-mailbox.c | 196 ++++++++++++++++++---
drivers/mailbox/mailbox.c | 15 +-
include/linux/mailbox_controller.h | 5 +-
5 files changed, 246 insertions(+), 26 deletions(-)
create mode 100644 arch/arm64/boot/dts/broadcom/stingray/stingray-fs4.dtsi
--
2.7.4
Anup Patel
2017-07-21 06:55:36 UTC
Permalink
This patch set IRQ affinity hint for FlexRM ring IRQ at time of
enabling ring (i.e. flexrm_startup()). The IRQ affinity hint will
allow FlexRM driver to distribute FlexRM ring IRQs across online
CPUs so that all FlexRM ring IRQs don't land in CPU0 by default.

Signed-off-by: Anup Patel <***@broadcom.com>
Reviewed-by: Ray Jui <***@broadcom.com>
Reviewed-by: Scott Branden <***@broadcom.com>
---
drivers/mailbox/bcm-flexrm-mailbox.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)

diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c b/drivers/mailbox/bcm-flexrm-mailbox.c
index da67882..e8c3666 100644
--- a/drivers/mailbox/bcm-flexrm-mailbox.c
+++ b/drivers/mailbox/bcm-flexrm-mailbox.c
@@ -260,6 +260,7 @@ struct flexrm_ring {
void __iomem *regs;
bool irq_requested;
unsigned int irq;
+ cpumask_t irq_aff_hint;
unsigned int msi_timer_val;
unsigned int msi_count_threshold;
struct ida requests_ida;
@@ -1217,6 +1218,18 @@ static int flexrm_startup(struct mbox_chan *chan)
}
ring->irq_requested = true;

+ /* Set IRQ affinity hint */
+ ring->irq_aff_hint = CPU_MASK_NONE;
+ val = ring->mbox->num_rings;
+ val = (num_online_cpus() < val) ? val / num_online_cpus() : 1;
+ cpumask_set_cpu((ring->num / val) % num_online_cpus(),
+ &ring->irq_aff_hint);
+ ret = irq_set_affinity_hint(ring->irq, &ring->irq_aff_hint);
+ if (ret) {
+ dev_err(ring->mbox->dev, "failed to set IRQ affinity hint\n");
+ goto fail_free_irq;
+ }
+
/* Disable/inactivate ring */
writel_relaxed(0x0, ring->regs + RING_CONTROL);

@@ -1261,6 +1274,9 @@ static int flexrm_startup(struct mbox_chan *chan)

return 0;

+fail_free_irq:
+ free_irq(ring->irq, ring);
+ ring->irq_requested = false;
fail_free_cmpl_memory:
dma_pool_free(ring->mbox->cmpl_pool,
ring->cmpl_base, ring->cmpl_dma_base);
@@ -1314,6 +1330,7 @@ static void flexrm_shutdown(struct mbox_chan *chan)

/* Release IRQ */
if (ring->irq_requested) {
+ irq_set_affinity_hint(ring->irq, NULL);
free_irq(ring->irq, ring);
ring->irq_requested = false;
}
--
2.7.4
Anup Patel
2017-07-21 06:55:37 UTC
Permalink
This patch adds debugfs support to Broadcom FlexRM driver
so that we can see FlexRM ring state when any issue happens.

Signed-off-by: Anup Patel <***@broadcom.com>
Reviewed-by: Vikram Prakash <***@broadcom.com>
Reviewed-by: Scott Branden <***@broadcom.com>
---
drivers/mailbox/bcm-flexrm-mailbox.c | 136 ++++++++++++++++++++++++++++++++++-
1 file changed, 134 insertions(+), 2 deletions(-)

diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c b/drivers/mailbox/bcm-flexrm-mailbox.c
index e8c3666..a0be2ea 100644
--- a/drivers/mailbox/bcm-flexrm-mailbox.c
+++ b/drivers/mailbox/bcm-flexrm-mailbox.c
@@ -17,6 +17,8 @@

#include <asm/barrier.h>
#include <asm/byteorder.h>
+#include <linux/atomic.h>
+#include <linux/debugfs.h>
#include <linux/delay.h>
#include <linux/device.h>
#include <linux/dma-mapping.h>
@@ -270,6 +272,9 @@ struct flexrm_ring {
u32 bd_write_offset;
void *cmpl_base;
dma_addr_t cmpl_dma_base;
+ /* Atomic stats */
+ atomic_t msg_send_count;
+ atomic_t msg_cmpl_count;
/* Protected members */
spinlock_t lock;
struct brcm_message *last_pending_msg;
@@ -283,6 +288,9 @@ struct flexrm_mbox {
struct flexrm_ring *rings;
struct dma_pool *bd_pool;
struct dma_pool *cmpl_pool;
+ struct dentry *root;
+ struct dentry *config;
+ struct dentry *stats;
struct mbox_controller controller;
};

@@ -913,6 +921,62 @@ static void *flexrm_write_descs(struct brcm_message *msg, u32 nhcnt,

/* ====== FlexRM driver helper routines ===== */

+static void flexrm_write_config_in_seqfile(struct flexrm_mbox *mbox,
+ struct seq_file *file)
+{
+ int i;
+ const char *state;
+ struct flexrm_ring *ring;
+
+ seq_printf(file, "%-5s %-9s %-18s %-10s %-18s %-10s\n",
+ "Ring#", "State", "BD_Addr", "BD_Size",
+ "Cmpl_Addr", "Cmpl_Size");
+
+ for (i = 0; i < mbox->num_rings; i++) {
+ ring = &mbox->rings[i];
+ if (readl(ring->regs + RING_CONTROL) &
+ BIT(CONTROL_ACTIVE_SHIFT))
+ state = "active";
+ else
+ state = "inactive";
+ seq_printf(file,
+ "%-5d %-9s 0x%016llx 0x%08x 0x%016llx 0x%08x\n",
+ ring->num, state,
+ (unsigned long long)ring->bd_dma_base,
+ (u32)RING_BD_SIZE,
+ (unsigned long long)ring->cmpl_dma_base,
+ (u32)RING_CMPL_SIZE);
+ }
+}
+
+static void flexrm_write_stats_in_seqfile(struct flexrm_mbox *mbox,
+ struct seq_file *file)
+{
+ int i;
+ u32 val, bd_read_offset;
+ struct flexrm_ring *ring;
+
+ seq_printf(file, "%-5s %-10s %-10s %-10s %-11s %-11s\n",
+ "Ring#", "BD_Read", "BD_Write",
+ "Cmpl_Read", "Submitted", "Completed");
+
+ for (i = 0; i < mbox->num_rings; i++) {
+ ring = &mbox->rings[i];
+ bd_read_offset = readl_relaxed(ring->regs + RING_BD_READ_PTR);
+ val = readl_relaxed(ring->regs + RING_BD_START_ADDR);
+ bd_read_offset *= RING_DESC_SIZE;
+ bd_read_offset += (u32)(BD_START_ADDR_DECODE(val) -
+ ring->bd_dma_base);
+ seq_printf(file, "%-5d 0x%08x 0x%08x 0x%08x %-11d %-11d\n",
+ ring->num,
+ (u32)bd_read_offset,
+ (u32)ring->bd_write_offset,
+ (u32)ring->cmpl_read_offset,
+ (u32)atomic_read(&ring->msg_send_count),
+ (u32)atomic_read(&ring->msg_cmpl_count));
+ }
+}
+
static int flexrm_new_request(struct flexrm_ring *ring,
struct brcm_message *batch_msg,
struct brcm_message *msg)
@@ -1013,6 +1077,9 @@ static int flexrm_new_request(struct flexrm_ring *ring,
/* Save ring BD write offset */
ring->bd_write_offset = (unsigned long)(next - ring->bd_base);

+ /* Increment number of messages sent */
+ atomic_inc_return(&ring->msg_send_count);
+
exit:
/* Update error status in message */
msg->error = ret;
@@ -1106,12 +1173,37 @@ static int flexrm_process_completions(struct flexrm_ring *ring)
mbox_chan_received_data(chan, msg);

/* Increment number of completions processed */
+ atomic_inc_return(&ring->msg_cmpl_count);
count++;
}

return count;
}

+/* ====== FlexRM Debugfs callbacks ====== */
+
+static int flexrm_debugfs_conf_show(struct seq_file *file, void *offset)
+{
+ struct platform_device *pdev = to_platform_device(file->private);
+ struct flexrm_mbox *mbox = platform_get_drvdata(pdev);
+
+ /* Write config in file */
+ flexrm_write_config_in_seqfile(mbox, file);
+
+ return 0;
+}
+
+static int flexrm_debugfs_stats_show(struct seq_file *file, void *offset)
+{
+ struct platform_device *pdev = to_platform_device(file->private);
+ struct flexrm_mbox *mbox = platform_get_drvdata(pdev);
+
+ /* Write stats in file */
+ flexrm_write_stats_in_seqfile(mbox, file);
+
+ return 0;
+}
+
/* ====== FlexRM interrupt handler ===== */

static irqreturn_t flexrm_irq_event(int irq, void *dev_id)
@@ -1272,6 +1364,10 @@ static int flexrm_startup(struct mbox_chan *chan)
val = BIT(CONTROL_ACTIVE_SHIFT);
writel_relaxed(val, ring->regs + RING_CONTROL);

+ /* Reset stats to zero */
+ atomic_set(&ring->msg_send_count, 0);
+ atomic_set(&ring->msg_cmpl_count, 0);
+
return 0;

fail_free_irq:
@@ -1491,6 +1587,8 @@ static int flexrm_mbox_probe(struct platform_device *pdev)
ring->bd_dma_base = 0;
ring->cmpl_base = NULL;
ring->cmpl_dma_base = 0;
+ atomic_set(&ring->msg_send_count, 0);
+ atomic_set(&ring->msg_cmpl_count, 0);
spin_lock_init(&ring->lock);
ring->last_pending_msg = NULL;
ring->cmpl_read_offset = 0;
@@ -1532,6 +1630,36 @@ static int flexrm_mbox_probe(struct platform_device *pdev)
ring->irq = desc->irq;
}

+ /* Check availability of debugfs */
+ if (!debugfs_initialized())
+ goto skip_debugfs;
+
+ /* Create debugfs root entry */
+ mbox->root = debugfs_create_dir(dev_name(mbox->dev), NULL);
+ if (IS_ERR_OR_NULL(mbox->root)) {
+ ret = PTR_ERR_OR_ZERO(mbox->root);
+ goto fail_free_msis;
+ }
+
+ /* Create debugfs config entry */
+ mbox->config = debugfs_create_devm_seqfile(mbox->dev,
+ "config", mbox->root,
+ flexrm_debugfs_conf_show);
+ if (IS_ERR_OR_NULL(mbox->config)) {
+ ret = PTR_ERR_OR_ZERO(mbox->config);
+ goto fail_free_debugfs_root;
+ }
+
+ /* Create debugfs stats entry */
+ mbox->stats = debugfs_create_devm_seqfile(mbox->dev,
+ "stats", mbox->root,
+ flexrm_debugfs_stats_show);
+ if (IS_ERR_OR_NULL(mbox->stats)) {
+ ret = PTR_ERR_OR_ZERO(mbox->stats);
+ goto fail_free_debugfs_root;
+ }
+skip_debugfs:
+
/* Initialize mailbox controller */
mbox->controller.txdone_irq = false;
mbox->controller.txdone_poll = true;
@@ -1544,7 +1672,7 @@ static int flexrm_mbox_probe(struct platform_device *pdev)
sizeof(*mbox->controller.chans), GFP_KERNEL);
if (!mbox->controller.chans) {
ret = -ENOMEM;
- goto fail_free_msis;
+ goto fail_free_debugfs_root;
}
for (index = 0; index < mbox->num_rings; index++)
mbox->controller.chans[index].con_priv = &mbox->rings[index];
@@ -1552,13 +1680,15 @@ static int flexrm_mbox_probe(struct platform_device *pdev)
/* Register mailbox controller */
ret = mbox_controller_register(&mbox->controller);
if (ret)
- goto fail_free_msis;
+ goto fail_free_debugfs_root;

dev_info(dev, "registered flexrm mailbox with %d channels\n",
mbox->controller.num_chans);

return 0;

+fail_free_debugfs_root:
+ debugfs_remove_recursive(mbox->root);
fail_free_msis:
platform_msi_domain_free_irqs(dev);
fail_destroy_cmpl_pool:
@@ -1578,6 +1708,8 @@ static int flexrm_mbox_remove(struct platform_device *pdev)

mbox_controller_unregister(&mbox->controller);

+ debugfs_remove_recursive(mbox->root);
+
platform_msi_domain_free_irqs(dev);

dma_pool_destroy(mbox->cmpl_pool);
--
2.7.4
Anup Patel
2017-07-21 06:55:38 UTC
Permalink
The mask used in CMPL_START_ADDR_VALUE() should be 27bits instead of
26bits. This incorrect mask was causing completion writes to 40bits
physical address fail.

This patch fixes mask used in CMPL_START_ADDR_VALUE() macro.

Fixes: dbc049eee730 ("mailbox: Add driver for Broadcom FlexRM
ring manager")

Signed-off-by: Anup Patel <***@broadcom.com>
Reviewed-by: Ray Jui <***@broadcom.com>
Reviewed-by: Scott Branden <***@broadcom.com>
Cc: ***@vger.kernel.org
---
drivers/mailbox/bcm-flexrm-mailbox.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c b/drivers/mailbox/bcm-flexrm-mailbox.c
index a0be2ea..46ad305 100644
--- a/drivers/mailbox/bcm-flexrm-mailbox.c
+++ b/drivers/mailbox/bcm-flexrm-mailbox.c
@@ -97,7 +97,7 @@

/* Register RING_CMPL_START_ADDR fields */
#define CMPL_START_ADDR_VALUE(pa) \
- ((u32)((((u64)(pa)) >> RING_CMPL_ALIGN_ORDER) & 0x03ffffff))
+ ((u32)((((u64)(pa)) >> RING_CMPL_ALIGN_ORDER) & 0x07ffffff))

/* Register RING_CONTROL fields */
#define CONTROL_MASK_DISABLE_CONTROL 12
--
2.7.4
Anup Patel
2017-07-21 06:55:40 UTC
Permalink
Currently, the message send queue size in Linux mailbox framework
is hard-coded to MBOX_TX_QUEUE_LEN which is defined as 20.

This message send queue can easily overflow if mbox_send_message()
is called for same mailbox channel several times. The size of message
send queue should not be hard-coded in Linux mailbox framework and
instead mailbox controller driver should have a mechanism to specify
message send queue size for each mailbox channel.

This patch makes message send queue size dynamic in Linux mailbox
framework and provides a mechanism to set message send queue size
for each mailbox channel. If mailbox controller driver does not set
message send queue size then we assume the hard-coded value of 20.

Signed-off-by: Anup Patel <***@broadcom.com>
Reviewed-by: Jonathan Richardson <***@broadcom.com>
Reviewed-by: Scott Branden <***@broadcom.com>
---
drivers/mailbox/mailbox.c | 15 ++++++++++++---
include/linux/mailbox_controller.h | 5 +++--
2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 537f4f6..ccc2aea 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -34,7 +34,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
spin_lock_irqsave(&chan->lock, flags);

/* See if there is any space left */
- if (chan->msg_count == MBOX_TX_QUEUE_LEN) {
+ if (chan->msg_count == chan->msg_queue_len) {
spin_unlock_irqrestore(&chan->lock, flags);
return -ENOBUFS;
}
@@ -43,7 +43,7 @@ static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
chan->msg_data[idx] = mssg;
chan->msg_count++;

- if (idx == MBOX_TX_QUEUE_LEN - 1)
+ if (idx == chan->msg_queue_len - 1)
chan->msg_free = 0;
else
chan->msg_free++;
@@ -70,7 +70,7 @@ static void msg_submit(struct mbox_chan *chan)
if (idx >= count)
idx -= count;
else
- idx += MBOX_TX_QUEUE_LEN - count;
+ idx += chan->msg_queue_len - count;

data = chan->msg_data[idx];

@@ -346,6 +346,12 @@ struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
spin_lock_irqsave(&chan->lock, flags);
chan->msg_free = 0;
chan->msg_count = 0;
+ chan->msg_data = kcalloc(chan->msg_queue_len,
+ sizeof(void *), GFP_ATOMIC);
+ if (!chan->msg_data) {
+ spin_unlock_irqrestore(&chan->lock, flags);
+ return ERR_PTR(-ENOMEM);
+ }
chan->active_req = NULL;
chan->cl = cl;
init_completion(&chan->tx_complete);
@@ -420,6 +426,7 @@ void mbox_free_channel(struct mbox_chan *chan)
chan->active_req = NULL;
if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
chan->txdone_method = TXDONE_BY_POLL;
+ kfree(chan->msg_data);

module_put(chan->mbox->dev->driver->owner);
spin_unlock_irqrestore(&chan->lock, flags);
@@ -477,6 +484,8 @@ int mbox_controller_register(struct mbox_controller *mbox)
chan->cl = NULL;
chan->mbox = mbox;
chan->txdone_method = txdone;
+ if (chan->msg_queue_len < MBOX_TX_QUEUE_LEN)
+ chan->msg_queue_len = MBOX_TX_QUEUE_LEN;
spin_lock_init(&chan->lock);
}

diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
index 74deadb..eba3fed 100644
--- a/include/linux/mailbox_controller.h
+++ b/include/linux/mailbox_controller.h
@@ -110,6 +110,7 @@ struct mbox_controller {
* @active_req: Currently active request hook
* @msg_count: No. of mssg currently queued
* @msg_free: Index of next available mssg slot
+ * @msg_queue_len: Max number of mssg which can be queued
* @msg_data: Hook for data packet
* @lock: Serialise access to the channel
* @con_priv: Hook for controller driver to attach private data
@@ -120,8 +121,8 @@ struct mbox_chan {
struct mbox_client *cl;
struct completion tx_complete;
void *active_req;
- unsigned msg_count, msg_free;
- void *msg_data[MBOX_TX_QUEUE_LEN];
+ unsigned int msg_count, msg_free, msg_queue_len;
+ void **msg_data;
spinlock_t lock; /* Serialise access to the channel */
void *con_priv;
};
--
2.7.4
Anup Patel
2017-07-21 06:55:41 UTC
Permalink
The Broadcom FlexRM ring (i.e. mailbox channel) can handle
larger number of messages queued in one FlexRM ring hence
this patch sets msg_queue_len for each mailbox channel to
be same as RING_MAX_REQ_COUNT.

Signed-off-by: Anup Patel <***@broadcom.com>
Reviewed-by: Scott Branden <***@broadcom.com>
---
drivers/mailbox/bcm-flexrm-mailbox.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c b/drivers/mailbox/bcm-flexrm-mailbox.c
index 9873818..20055a0 100644
--- a/drivers/mailbox/bcm-flexrm-mailbox.c
+++ b/drivers/mailbox/bcm-flexrm-mailbox.c
@@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct platform_device *pdev)
ret = -ENOMEM;
goto fail_free_debugfs_root;
}
- for (index = 0; index < mbox->num_rings; index++)
+ for (index = 0; index < mbox->num_rings; index++) {
+ mbox->controller.chans[index].msg_queue_len =
+ RING_MAX_REQ_COUNT;
mbox->controller.chans[index].con_priv = &mbox->rings[index];
+ }

/* Register mailbox controller */
ret = mbox_controller_register(&mbox->controller);
--
2.7.4
Jassi Brar
2017-07-21 15:46:24 UTC
Permalink
Hi Anup,
Post by Anup Patel
The Broadcom FlexRM ring (i.e. mailbox channel) can handle
larger number of messages queued in one FlexRM ring hence
this patch sets msg_queue_len for each mailbox channel to
be same as RING_MAX_REQ_COUNT.
---
drivers/mailbox/bcm-flexrm-mailbox.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c b/drivers/mailbox/bcm-flexrm-mailbox.c
index 9873818..20055a0 100644
--- a/drivers/mailbox/bcm-flexrm-mailbox.c
+++ b/drivers/mailbox/bcm-flexrm-mailbox.c
@@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct platform_device *pdev)
ret = -ENOMEM;
goto fail_free_debugfs_root;
}
- for (index = 0; index < mbox->num_rings; index++)
+ for (index = 0; index < mbox->num_rings; index++) {
+ mbox->controller.chans[index].msg_queue_len =
+ RING_MAX_REQ_COUNT;
mbox->controller.chans[index].con_priv = &mbox->rings[index];
+ }
While writing mailbox.c I wasn't unaware that there is the option to
choose the queue length at runtime.
The idea was to keep the code as simple as possible. I am open to
making it a runtime thing, but first, please help me understand how
that is useful here.

I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024)
elements. Any message submitted to mailbox api can be immediately
written onto the ringbuffer if there is some space.
Is there any mechanism to report back to a client driver, if its
message in ringbuffer failed "to be sent"?
If there isn't any, then I think, in flexrm_last_tx_done() you should
simply return true if there is some space left in the rung-buffer,
false otherwise.

Thanks
Anup Patel
2017-07-24 03:56:30 UTC
Permalink
Hi Jassi,

Sorry for the delayed response...
Post by Jassi Brar
Hi Anup,
Post by Anup Patel
The Broadcom FlexRM ring (i.e. mailbox channel) can handle
larger number of messages queued in one FlexRM ring hence
this patch sets msg_queue_len for each mailbox channel to
be same as RING_MAX_REQ_COUNT.
---
drivers/mailbox/bcm-flexrm-mailbox.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c b/drivers/mailbox/bcm-flexrm-mailbox.c
index 9873818..20055a0 100644
--- a/drivers/mailbox/bcm-flexrm-mailbox.c
+++ b/drivers/mailbox/bcm-flexrm-mailbox.c
@@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct platform_device *pdev)
ret = -ENOMEM;
goto fail_free_debugfs_root;
}
- for (index = 0; index < mbox->num_rings; index++)
+ for (index = 0; index < mbox->num_rings; index++) {
+ mbox->controller.chans[index].msg_queue_len =
+ RING_MAX_REQ_COUNT;
mbox->controller.chans[index].con_priv = &mbox->rings[index];
+ }
While writing mailbox.c I wasn't unaware that there is the option to
choose the queue length at runtime.
The idea was to keep the code as simple as possible. I am open to
making it a runtime thing, but first, please help me understand how
that is useful here.
I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024)
elements. Any message submitted to mailbox api can be immediately
written onto the ringbuffer if there is some space.
Is there any mechanism to report back to a client driver, if its
message in ringbuffer failed "to be sent"?
If there isn't any, then I think, in flexrm_last_tx_done() you should
simply return true if there is some space left in the rung-buffer,
false otherwise.
Yes, we have error code in "struct brcm_message" to report back
errors from send_message. In our mailbox clients, we check
return value of mbox_send_message() and also the error code
in "struct brcm_message".

The flexrm_last_tx_done() will mostly return true when it is able to
write message in the FlexRM ring. It will return false only when
there was no room in FlexRM ring or number of in-flight messages
in FlexRM ring are 1024 (max enteries in completion queue of
FlexRM ring).

We started seeing issues with fixed queue length in mailbox framework
when we stressed one FlexRM ring from multiple CPUs. Instead of simply
increasing MBOX_TX_QUEUE_LEN, it is better to let mailbox controller
driver to choose the queue length because there also Ring Manager
hardware who support variable sized rings.

Regards,
Anup
Jassi Brar
2017-07-24 16:36:39 UTC
Permalink
Post by Anup Patel
Hi Jassi,
Sorry for the delayed response...
Post by Jassi Brar
Hi Anup,
Post by Anup Patel
The Broadcom FlexRM ring (i.e. mailbox channel) can handle
larger number of messages queued in one FlexRM ring hence
this patch sets msg_queue_len for each mailbox channel to
be same as RING_MAX_REQ_COUNT.
---
drivers/mailbox/bcm-flexrm-mailbox.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c b/drivers/mailbox/bcm-flexrm-mailbox.c
index 9873818..20055a0 100644
--- a/drivers/mailbox/bcm-flexrm-mailbox.c
+++ b/drivers/mailbox/bcm-flexrm-mailbox.c
@@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct platform_device *pdev)
ret = -ENOMEM;
goto fail_free_debugfs_root;
}
- for (index = 0; index < mbox->num_rings; index++)
+ for (index = 0; index < mbox->num_rings; index++) {
+ mbox->controller.chans[index].msg_queue_len =
+ RING_MAX_REQ_COUNT;
mbox->controller.chans[index].con_priv = &mbox->rings[index];
+ }
While writing mailbox.c I wasn't unaware that there is the option to
choose the queue length at runtime.
The idea was to keep the code as simple as possible. I am open to
making it a runtime thing, but first, please help me understand how
that is useful here.
I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024)
elements. Any message submitted to mailbox api can be immediately
written onto the ringbuffer if there is some space.
Is there any mechanism to report back to a client driver, if its
message in ringbuffer failed "to be sent"?
If there isn't any, then I think, in flexrm_last_tx_done() you should
simply return true if there is some space left in the rung-buffer,
false otherwise.
Yes, we have error code in "struct brcm_message" to report back
errors from send_message. In our mailbox clients, we check
return value of mbox_send_message() and also the error code
in "struct brcm_message".
I meant after the message has been accepted in the ringbuffer but the
remote failed to receive it.
There seems no such provision. IIANW, then you should be able to
consider every message as "sent successfully" once it is in the ring
buffer i.e, immediately after mbox_send_message() returns 0.
In that case I would think you don't need more than a couple of
entries out of MBOX_TX_QUEUE_LEN ?
Anup Patel
2017-07-25 05:41:21 UTC
Permalink
Post by Jassi Brar
Post by Anup Patel
Hi Jassi,
Sorry for the delayed response...
Post by Jassi Brar
Hi Anup,
Post by Anup Patel
The Broadcom FlexRM ring (i.e. mailbox channel) can handle
larger number of messages queued in one FlexRM ring hence
this patch sets msg_queue_len for each mailbox channel to
be same as RING_MAX_REQ_COUNT.
---
drivers/mailbox/bcm-flexrm-mailbox.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c b/drivers/mailbox/bcm-flexrm-mailbox.c
index 9873818..20055a0 100644
--- a/drivers/mailbox/bcm-flexrm-mailbox.c
+++ b/drivers/mailbox/bcm-flexrm-mailbox.c
@@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct platform_device *pdev)
ret = -ENOMEM;
goto fail_free_debugfs_root;
}
- for (index = 0; index < mbox->num_rings; index++)
+ for (index = 0; index < mbox->num_rings; index++) {
+ mbox->controller.chans[index].msg_queue_len =
+ RING_MAX_REQ_COUNT;
mbox->controller.chans[index].con_priv = &mbox->rings[index];
+ }
While writing mailbox.c I wasn't unaware that there is the option to
choose the queue length at runtime.
The idea was to keep the code as simple as possible. I am open to
making it a runtime thing, but first, please help me understand how
that is useful here.
I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024)
elements. Any message submitted to mailbox api can be immediately
written onto the ringbuffer if there is some space.
Is there any mechanism to report back to a client driver, if its
message in ringbuffer failed "to be sent"?
If there isn't any, then I think, in flexrm_last_tx_done() you should
simply return true if there is some space left in the rung-buffer,
false otherwise.
Yes, we have error code in "struct brcm_message" to report back
errors from send_message. In our mailbox clients, we check
return value of mbox_send_message() and also the error code
in "struct brcm_message".
I meant after the message has been accepted in the ringbuffer but the
remote failed to receive it.
Yes, even this case is handled.

In case of IO errors after message has been put in ring buffer, we get
completion message with error code and mailbox client drivers will
receive back "struct brcm_message" with error set.

You can refer flexrm_process_completions() for more details.
Post by Jassi Brar
There seems no such provision. IIANW, then you should be able to
consider every message as "sent successfully" once it is in the ring
buffer i.e, immediately after mbox_send_message() returns 0.
In that case I would think you don't need more than a couple of
entries out of MBOX_TX_QUEUE_LEN ?
What I am trying to suggest is that we can take upto 1024 messages
in a FlexRM ring but the MBOX_TX_QUEUE_LEN limits us queuing
more messages. This issue manifest easily when multiple CPUs
queues to same FlexRM ring (i.e. same mailbox channel).

Another quick fix is to make MBOX_TX_QUEUE_LEN as 1024 but
it will not be generic fix.

Regards,
Anup
Jassi Brar
2017-07-25 16:07:31 UTC
Permalink
Post by Anup Patel
Post by Jassi Brar
Post by Anup Patel
Hi Jassi,
Sorry for the delayed response...
Post by Jassi Brar
Hi Anup,
Post by Anup Patel
The Broadcom FlexRM ring (i.e. mailbox channel) can handle
larger number of messages queued in one FlexRM ring hence
this patch sets msg_queue_len for each mailbox channel to
be same as RING_MAX_REQ_COUNT.
---
drivers/mailbox/bcm-flexrm-mailbox.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c b/drivers/mailbox/bcm-flexrm-mailbox.c
index 9873818..20055a0 100644
--- a/drivers/mailbox/bcm-flexrm-mailbox.c
+++ b/drivers/mailbox/bcm-flexrm-mailbox.c
@@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct platform_device *pdev)
ret = -ENOMEM;
goto fail_free_debugfs_root;
}
- for (index = 0; index < mbox->num_rings; index++)
+ for (index = 0; index < mbox->num_rings; index++) {
+ mbox->controller.chans[index].msg_queue_len =
+ RING_MAX_REQ_COUNT;
mbox->controller.chans[index].con_priv = &mbox->rings[index];
+ }
While writing mailbox.c I wasn't unaware that there is the option to
choose the queue length at runtime.
The idea was to keep the code as simple as possible. I am open to
making it a runtime thing, but first, please help me understand how
that is useful here.
I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024)
elements. Any message submitted to mailbox api can be immediately
written onto the ringbuffer if there is some space.
Is there any mechanism to report back to a client driver, if its
message in ringbuffer failed "to be sent"?
If there isn't any, then I think, in flexrm_last_tx_done() you should
simply return true if there is some space left in the rung-buffer,
false otherwise.
Yes, we have error code in "struct brcm_message" to report back
errors from send_message. In our mailbox clients, we check
return value of mbox_send_message() and also the error code
in "struct brcm_message".
I meant after the message has been accepted in the ringbuffer but the
remote failed to receive it.
Yes, even this case is handled.
In case of IO errors after message has been put in ring buffer, we get
completion message with error code and mailbox client drivers will
receive back "struct brcm_message" with error set.
You can refer flexrm_process_completions() for more details.
Post by Jassi Brar
There seems no such provision. IIANW, then you should be able to
consider every message as "sent successfully" once it is in the ring
buffer i.e, immediately after mbox_send_message() returns 0.
In that case I would think you don't need more than a couple of
entries out of MBOX_TX_QUEUE_LEN ?
What I am trying to suggest is that we can take upto 1024 messages
in a FlexRM ring but the MBOX_TX_QUEUE_LEN limits us queuing
more messages. This issue manifest easily when multiple CPUs
queues to same FlexRM ring (i.e. same mailbox channel).
OK then, I guess we have to make the queue length a runtime decision.

BTW, is it a practical use case that needs to queue upto 1024
requests? Or are you just testing?

Thanks

Anup Patel
2017-07-21 06:55:39 UTC
Permalink
Currently, we are using IDA library for managing IDs
on a FlexRM ring. The IDA library dynamically allocates
memory for underlying data structures which can cause
potential locking issue when allocating/free IDs from
flexrm_new_request() and flexrm_process_completions().

To tackle this, we replace use of IDA with bitmap for
each FlexRM ring and also protect the bitmap with FlexRM
ring lock.

Signed-off-by: Anup Patel <***@broadcom.com>
---
drivers/mailbox/bcm-flexrm-mailbox.c | 36 +++++++++++++++++++-----------------
1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c b/drivers/mailbox/bcm-flexrm-mailbox.c
index 46ad305..9873818 100644
--- a/drivers/mailbox/bcm-flexrm-mailbox.c
+++ b/drivers/mailbox/bcm-flexrm-mailbox.c
@@ -18,13 +18,13 @@
#include <asm/barrier.h>
#include <asm/byteorder.h>
#include <linux/atomic.h>
+#include <linux/bitmap.h>
#include <linux/debugfs.h>
#include <linux/delay.h>
#include <linux/device.h>
#include <linux/dma-mapping.h>
#include <linux/dmapool.h>
#include <linux/err.h>
-#include <linux/idr.h>
#include <linux/interrupt.h>
#include <linux/kernel.h>
#include <linux/mailbox_controller.h>
@@ -265,7 +265,6 @@ struct flexrm_ring {
cpumask_t irq_aff_hint;
unsigned int msi_timer_val;
unsigned int msi_count_threshold;
- struct ida requests_ida;
struct brcm_message *requests[RING_MAX_REQ_COUNT];
void *bd_base;
dma_addr_t bd_dma_base;
@@ -277,6 +276,7 @@ struct flexrm_ring {
atomic_t msg_cmpl_count;
/* Protected members */
spinlock_t lock;
+ DECLARE_BITMAP(requests_bmap, RING_MAX_REQ_COUNT);
struct brcm_message *last_pending_msg;
u32 cmpl_read_offset;
};
@@ -994,10 +994,10 @@ static int flexrm_new_request(struct flexrm_ring *ring,
msg->error = 0;

/* If no requests possible then save data pointer and goto done. */
- reqid = ida_simple_get(&ring->requests_ida, 0,
- RING_MAX_REQ_COUNT, GFP_KERNEL);
+ spin_lock_irqsave(&ring->lock, flags);
+ reqid = bitmap_find_free_region(ring->requests_bmap,
+ RING_MAX_REQ_COUNT, 0);
if (reqid < 0) {
- spin_lock_irqsave(&ring->lock, flags);
if (batch_msg)
ring->last_pending_msg = batch_msg;
else
@@ -1005,13 +1005,16 @@ static int flexrm_new_request(struct flexrm_ring *ring,
spin_unlock_irqrestore(&ring->lock, flags);
return 0;
}
+ spin_unlock_irqrestore(&ring->lock, flags);
ring->requests[reqid] = msg;

/* Do DMA mappings for the message */
ret = flexrm_dma_map(ring->mbox->dev, msg);
if (ret < 0) {
ring->requests[reqid] = NULL;
- ida_simple_remove(&ring->requests_ida, reqid);
+ spin_lock_irqsave(&ring->lock, flags);
+ bitmap_release_region(ring->requests_bmap, reqid, 0);
+ spin_unlock_irqrestore(&ring->lock, flags);
return ret;
}

@@ -1088,7 +1091,9 @@ static int flexrm_new_request(struct flexrm_ring *ring,
if (exit_cleanup) {
flexrm_dma_unmap(ring->mbox->dev, msg);
ring->requests[reqid] = NULL;
- ida_simple_remove(&ring->requests_ida, reqid);
+ spin_lock_irqsave(&ring->lock, flags);
+ bitmap_release_region(ring->requests_bmap, reqid, 0);
+ spin_unlock_irqrestore(&ring->lock, flags);
}

return ret;
@@ -1163,7 +1168,9 @@ static int flexrm_process_completions(struct flexrm_ring *ring)

/* Release reqid for recycling */
ring->requests[reqid] = NULL;
- ida_simple_remove(&ring->requests_ida, reqid);
+ spin_lock_irqsave(&ring->lock, flags);
+ bitmap_release_region(ring->requests_bmap, reqid, 0);
+ spin_unlock_irqrestore(&ring->lock, flags);

/* Unmap DMA mappings */
flexrm_dma_unmap(ring->mbox->dev, msg);
@@ -1414,7 +1421,6 @@ static void flexrm_shutdown(struct mbox_chan *chan)

/* Release reqid for recycling */
ring->requests[reqid] = NULL;
- ida_simple_remove(&ring->requests_ida, reqid);

/* Unmap DMA mappings */
flexrm_dma_unmap(ring->mbox->dev, msg);
@@ -1424,6 +1430,9 @@ static void flexrm_shutdown(struct mbox_chan *chan)
mbox_chan_received_data(chan, msg);
}

+ /* Clear requests bitmap */
+ bitmap_zero(ring->requests_bmap, RING_MAX_REQ_COUNT);
+
/* Release IRQ */
if (ring->irq_requested) {
irq_set_affinity_hint(ring->irq, NULL);
@@ -1581,7 +1590,6 @@ static int flexrm_mbox_probe(struct platform_device *pdev)
ring->irq_requested = false;
ring->msi_timer_val = MSI_TIMER_VAL_MASK;
ring->msi_count_threshold = 0x1;
- ida_init(&ring->requests_ida);
memset(ring->requests, 0, sizeof(ring->requests));
ring->bd_base = NULL;
ring->bd_dma_base = 0;
@@ -1590,6 +1598,7 @@ static int flexrm_mbox_probe(struct platform_device *pdev)
atomic_set(&ring->msg_send_count, 0);
atomic_set(&ring->msg_cmpl_count, 0);
spin_lock_init(&ring->lock);
+ bitmap_zero(ring->requests_bmap, RING_MAX_REQ_COUNT);
ring->last_pending_msg = NULL;
ring->cmpl_read_offset = 0;
}
@@ -1701,9 +1710,7 @@ static int flexrm_mbox_probe(struct platform_device *pdev)

static int flexrm_mbox_remove(struct platform_device *pdev)
{
- int index;
struct device *dev = &pdev->dev;
- struct flexrm_ring *ring;
struct flexrm_mbox *mbox = platform_get_drvdata(pdev);

mbox_controller_unregister(&mbox->controller);
@@ -1715,11 +1722,6 @@ static int flexrm_mbox_remove(struct platform_device *pdev)
dma_pool_destroy(mbox->cmpl_pool);
dma_pool_destroy(mbox->bd_pool);

- for (index = 0; index < mbox->num_rings; index++) {
- ring = &mbox->rings[index];
- ida_destroy(&ring->requests_ida);
- }
-
return 0;
}
--
2.7.4
Anup Patel
2017-07-21 06:55:42 UTC
Permalink
We have two instances of FlexRM on Stingray. One for SBA RAID
offload engine and another for SPU2 Crypto offload engine.

This patch adds FlexRM mailbox controller DT nodes for Stingray.

Signed-off-by: Anup Patel <***@broadcom.com>
Signed-off-by: Raveendra Padasalagi <***@broadcom.com>
---
.../boot/dts/broadcom/stingray/stingray-fs4.dtsi | 54 ++++++++++++++++++++++
.../arm64/boot/dts/broadcom/stingray/stingray.dtsi | 2 +
2 files changed, 56 insertions(+)
create mode 100644 arch/arm64/boot/dts/broadcom/stingray/stingray-fs4.dtsi

diff --git a/arch/arm64/boot/dts/broadcom/stingray/stingray-fs4.dtsi b/arch/arm64/boot/dts/broadcom/stingray/stingray-fs4.dtsi
new file mode 100644
index 0000000..1f927c4
--- /dev/null
+++ b/arch/arm64/boot/dts/broadcom/stingray/stingray-fs4.dtsi
@@ -0,0 +1,54 @@
+/*
+ * BSD LICENSE
+ *
+ * Copyright(c) 2016-2017 Broadcom. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in
+ * the documentation and/or other materials provided with the
+ * distribution.
+ * * Neither the name of Broadcom nor the names of its
+ * contributors may be used to endorse or promote products derived
+ * from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+ fs4: fs4 {
+ compatible = "simple-bus";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x0 0x67000000 0x00800000>;
+
+ crypto_mbox: ***@00000000 {
+ compatible = "brcm,iproc-flexrm-mbox";
+ reg = <0x00000000 0x200000>;
+ msi-parent = <&gic_its 0x4100>;
+ #mbox-cells = <3>;
+ dma-coherent;
+ };
+
+ raid_mbox: ***@00400000 {
+ compatible = "brcm,iproc-flexrm-mbox";
+ reg = <0x00400000 0x200000>;
+ dma-coherent;
+ msi-parent = <&gic_its 0x4300>;
+ #mbox-cells = <3>;
+ };
+ };
diff --git a/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi b/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi
index 49933cf..a98fbe9a 100644
--- a/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi
+++ b/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi
@@ -261,6 +261,8 @@
};
};

+ #include "stingray-fs4.dtsi"
+
hsls {
compatible = "simple-bus";
#address-cells = <1>;
--
2.7.4
Loading...