[PATCH v2 0/7] FlexRM driver improvements

[PATCH v2 0/7] FlexRM driver improvements

[Anup Patel]

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

[PATCH v2 1/7] mailbox: bcm-flexrm-mailbox: Set IRQ affinity hint for FlexRM ring IRQs

[Anup Patel]

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 <anup.patel@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Scott Branden <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

[PATCH v2 2/7] mailbox: bcm-flexrm-mailbox: Add debugfs support

[Anup Patel]

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 <anup.patel@broadcom.com>
Reviewed-by: Vikram Prakash <vikram.prakash@broadcom.com>
Reviewed-by: Scott Branden <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

[PATCH v2 3/7] mailbox: bcm-flexrm-mailbox: Fix mask used in CMPL_START_ADDR_VALUE()

[Anup Patel]

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 <anup.patel@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
Cc: stable@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

[PATCH v2 4/7] mailbox: bcm-flexrm-mailbox: Use bitmap instead of IDA

[Anup Patel]

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 <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

[PATCH v2 5/7] mailbox: Make message send queue size dynamic in Linux mailbox

[Anup Patel]

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 <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Reviewed-by: Jonathan Richardson <jonathan.richardson-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Reviewed-by: Scott Branden <scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel

[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.

Signed-off-by: Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Reviewed-by: Scott Branden <scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel

[Jassi Brar]

Hi Anup,

On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel <anup.patel@broadcom.com> wrote:
> 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 <anup.patel@broadcom.com>
> Reviewed-by: Scott Branden <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];
> +       }
>
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

Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel

[Anup Patel]

Hi Jassi,

Sorry for the delayed response...

On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Anup,
>
> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>> 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 <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>> Reviewed-by: Scott Branden <scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>> ---
>>  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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel

[Jassi Brar]

On Mon, Jul 24, 2017 at 9:26 AM, Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
> Hi Jassi,
>
> Sorry for the delayed response...
>
> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Hi Anup,
>>
>> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>> 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 <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>> Reviewed-by: Scott Branden <scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>> ---
>>>  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 ?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel

[Anup Patel]

On Mon, Jul 24, 2017 at 10:06 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Mon, Jul 24, 2017 at 9:26 AM, Anup Patel <anup.patel@broadcom.com> wrote:
>> Hi Jassi,
>>
>> Sorry for the delayed response...
>>
>> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>> Hi Anup,
>>>
>>> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel <anup.patel@broadcom.com> wrote:
>>>> 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 <anup.patel@broadcom.com>
>>>> Reviewed-by: Scott Branden <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];
>>>> +       }
>>>>
>>> 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.

> 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

Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel

[Jassi Brar]

On Tue, Jul 25, 2017 at 11:11 AM, Anup Patel <anup.patel@broadcom.com> wrote:
> On Mon, Jul 24, 2017 at 10:06 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>> On Mon, Jul 24, 2017 at 9:26 AM, Anup Patel <anup.patel@broadcom.com> wrote:
>>> Hi Jassi,
>>>
>>> Sorry for the delayed response...
>>>
>>> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>> Hi Anup,
>>>>
>>>> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel <anup.patel@broadcom.com> wrote:
>>>>> 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 <anup.patel@broadcom.com>
>>>>> Reviewed-by: Scott Branden <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];
>>>>> +       }
>>>>>
>>>> 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.
>
>> 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

Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel

[Anup Patel]

On Tue, Jul 25, 2017 at 9:37 PM, Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Jul 25, 2017 at 11:11 AM, Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>> On Mon, Jul 24, 2017 at 10:06 PM, Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On Mon, Jul 24, 2017 at 9:26 AM, Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>>> Hi Jassi,
>>>>
>>>> Sorry for the delayed response...
>>>>
>>>> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>> Hi Anup,
>>>>>
>>>>> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>>>>> 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 <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>>>>> Reviewed-by: Scott Branden <scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>>>>> ---
>>>>>>  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.
>>
>>> 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.

Do you agree with approach taken by PATCH5 and PATCH6 to
make queue length runtime?

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

Yes, we just need bigger queue length for FlexRM but we
choose 1024 (max limit) to avoid changing it again in future.

Regards,
Anup
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel

[Jassi Brar]

On Thu, Jul 27, 2017 at 9:25 AM, Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
> On Tue, Jul 25, 2017 at 9:37 PM, Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Tue, Jul 25, 2017 at 11:11 AM, Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>> On Mon, Jul 24, 2017 at 10:06 PM, Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> On Mon, Jul 24, 2017 at 9:26 AM, Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>>>> Hi Jassi,
>>>>>
>>>>> Sorry for the delayed response...
>>>>>
>>>>> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>> Hi Anup,
>>>>>>
>>>>>> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>>>>>> 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 <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>>>>>> Reviewed-by: Scott Branden <scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>>>>>> ---
>>>>>>>  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.
>>>
It doesn't seem to be what I suggest. I see two issues in
flexrm_process_completions()
1) It calls mbox_send_message(), which is a big NO for a controller
driver. Why should you have one more message stored outside of
ringbuffer?

2) It calls mbox_chan_received_data()  which is for messages received
from the remote. And not the way to report failed _transmission_, for
which the api calls back mbox_client.tx_done() .  In your client
driver please populate mbox_client.tx_done() and see which message is
reported "sent fine" when.


>>>> 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.
>
> Do you agree with approach taken by PATCH5 and PATCH6 to
> make queue length runtime?
>
I agree that we may have to get the queue length from platform, if
MBOX_TX_QUEUE_LEN is limiting performance. That will be easier on both
of us. However I suspect the right fix for _this_ situation is in
flexrm driver. See above.

>>
>> BTW, is it a practical use case that needs to queue upto 1024
>> requests? Or are you just testing?
>
> Yes, we just need bigger queue length for FlexRM but we
> choose 1024 (max limit) to avoid changing it again in future.
>
How does the client use the api? Does it work in blocking mode i.e, is
tx_block set ? Is it available somewhere I can have a look?

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel

[Anup Patel]

On Thu, Jul 27, 2017 at 10:29 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Thu, Jul 27, 2017 at 9:25 AM, Anup Patel <anup.patel@broadcom.com> wrote:
>> On Tue, Jul 25, 2017 at 9:37 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>> On Tue, Jul 25, 2017 at 11:11 AM, Anup Patel <anup.patel@broadcom.com> wrote:
>>>> On Mon, Jul 24, 2017 at 10:06 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>>> On Mon, Jul 24, 2017 at 9:26 AM, Anup Patel <anup.patel@broadcom.com> wrote:
>>>>>> Hi Jassi,
>>>>>>
>>>>>> Sorry for the delayed response...
>>>>>>
>>>>>> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>>>>> Hi Anup,
>>>>>>>
>>>>>>> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel <anup.patel@broadcom.com> wrote:
>>>>>>>> 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 <anup.patel@broadcom.com>
>>>>>>>> Reviewed-by: Scott Branden <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];
>>>>>>>> +       }
>>>>>>>>
>>>>>>> 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.
>>>>
> It doesn't seem to be what I suggest. I see two issues in
> flexrm_process_completions()
> 1) It calls mbox_send_message(), which is a big NO for a controller
> driver. Why should you have one more message stored outside of
> ringbuffer?

The "last_pending_msg" in each FlexRM ring was added to fit FlexRM
in Mailbox framework.

We don't have any IRQ for TX done so "txdone_irq" out of the question for
FlexRM. We only have completions for both success or failures (IO errors).

This means we have to use "txdone_poll" for FlexRM. For "txdone_poll",
we have to provide last_tx_done() callback. The last_tx_done() callback
is supposed to return true if last send_data() call succeeded.

To implement last_tx_done() in FlexRM driver, we added "last_pending_msg".

When "last_pending_msg" is NULL it means last call to send_data() succeeded
and when "last_pending_msg" is != NULL it means last call to send_data()
did not go through due to lack of space in FlexRM ring. The IRQ worker
of FlexRM ring will automatically queue the message pointed by
"last_pending_message" and clear it. This is why we have mbox_send_message()
call in flexrm_process_completions().

>
> 2) It calls mbox_chan_received_data()  which is for messages received
> from the remote. And not the way to report failed _transmission_, for
> which the api calls back mbox_client.tx_done() .  In your client
> driver please populate mbox_client.tx_done() and see which message is
> reported "sent fine" when.
>
>
>>>>> 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.
>>
>> Do you agree with approach taken by PATCH5 and PATCH6 to
>> make queue length runtime?
>>
> I agree that we may have to get the queue length from platform, if
> MBOX_TX_QUEUE_LEN is limiting performance. That will be easier on both
> of us. However I suspect the right fix for _this_ situation is in
> flexrm driver. See above.

The current implementation is trying to model FlexRM using "txdone_poll"
method and that's why we have dependency on MBOX_TX_QUEUE_LEN

I think what we really need is new method for "txdone" to model ring
manager HW (such as FlexRM). Let's call it "txdone_none".

For "txdone_none", it means there is no "txdone" reporting in HW
and mbox_send_data() should simply return value returned by
send_data() callback. The last_tx_done() callback is not required
for "txdone_none" and MBOX_TX_QUEUE_LEN also has no
effect on "txdone_none". Both blocking and non-blocking clients
are treated same for "txdone_none".

>
>>>
>>> BTW, is it a practical use case that needs to queue upto 1024
>>> requests? Or are you just testing?
>>
>> Yes, we just need bigger queue length for FlexRM but we
>> choose 1024 (max limit) to avoid changing it again in future.
>>
> How does the client use the api? Does it work in blocking mode i.e, is
> tx_block set ? Is it available somewhere I can have a look?

Yes, our mailbox clients are non-blocking.

We have two mailbox clients (already up-streamed):
1. BCM-SBA-RAID located at drivers/dma/bcm-sba-raid.c
2. SPU2 Crypto located at drivers/crypto/bcm/spu2.c

Regards,
Anup

Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel

[Jassi Brar]

On Thu, Jul 27, 2017 at 11:20 AM, Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
> On Thu, Jul 27, 2017 at 10:29 AM, Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

>>>>>>> Sorry for the delayed response...
>>>>>>>
>>>>>>> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>>>> Hi Anup,
>>>>>>>>
>>>>>>>> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>>>>>>>> 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 <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>>>>>>>> Reviewed-by: Scott Branden <scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>>>>>>>> ---
>>>>>>>>>  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.
>>>>>
>> It doesn't seem to be what I suggest. I see two issues in
>> flexrm_process_completions()
>> 1) It calls mbox_send_message(), which is a big NO for a controller
>> driver. Why should you have one more message stored outside of
>> ringbuffer?
>
> The "last_pending_msg" in each FlexRM ring was added to fit FlexRM
> in Mailbox framework.
>
> We don't have any IRQ for TX done so "txdone_irq" out of the question for
> FlexRM. We only have completions for both success or failures (IO errors).
>
> This means we have to use "txdone_poll" for FlexRM. For "txdone_poll",
> we have to provide last_tx_done() callback. The last_tx_done() callback
> is supposed to return true if last send_data() call succeeded.
>
> To implement last_tx_done() in FlexRM driver, we added "last_pending_msg".
>
> When "last_pending_msg" is NULL it means last call to send_data() succeeded
> and when "last_pending_msg" is != NULL it means last call to send_data()
> did not go through due to lack of space in FlexRM ring.
>
It could be simpler.
Since flexrm_send_data() is essentially about putting the message in
the ring-buffer (and not about _transmission_ failures), the
last_tx_done() should simply return true if requests_ida has not all
ids allocated. False otherwise.

>>
>> 2) It calls mbox_chan_received_data()  which is for messages received
>> from the remote. And not the way to report failed _transmission_, for
>> which the api calls back mbox_client.tx_done() .  In your client
>> driver please populate mbox_client.tx_done() and see which message is
>> reported "sent fine" when.
>>
>>
>>>>>> 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.
>>>
>>> Do you agree with approach taken by PATCH5 and PATCH6 to
>>> make queue length runtime?
>>>
>> I agree that we may have to get the queue length from platform, if
>> MBOX_TX_QUEUE_LEN is limiting performance. That will be easier on both
>> of us. However I suspect the right fix for _this_ situation is in
>> flexrm driver. See above.
>
> The current implementation is trying to model FlexRM using "txdone_poll"
> method and that's why we have dependency on MBOX_TX_QUEUE_LEN
>
> I think what we really need is new method for "txdone" to model ring
> manager HW (such as FlexRM). Let's call it "txdone_none".
>
> For "txdone_none", it means there is no "txdone" reporting in HW
> and mbox_send_data() should simply return value returned by
> send_data() callback. The last_tx_done() callback is not required
> for "txdone_none" and MBOX_TX_QUEUE_LEN also has no
> effect on "txdone_none". Both blocking and non-blocking clients
> are treated same for "txdone_none".
>
That is already supported :)

In drivers/dma/bcm-sba-raid.c

sba_send_mbox_request(...)
{
           ......
        req->msg.error = 0;
        ret = mbox_send_message(sba->mchans[mchans_idx], &req->msg);
        if (ret < 0) {
                dev_err(sba->dev, "send message failed with error %d", ret);
                return ret;
        }
        ret = req->msg.error;
        if (ret < 0) {
                dev_err(sba->dev, "message error %d", ret);
                return ret;
        }
          .....
}

Here you _do_ assume that as soon as the mbox_send_message() returns,
the last_tx_done() is true. In other words, this is a case of client
'knows_txdone'.

So ideally you should specify cl->knows_txdone = true during
mbox_request_channel() and have ...

sba_send_mbox_request(...)
{
        ret = mbox_send_message(sba->mchans[mchans_idx], &req->msg);
        if (ret < 0) {
                dev_err(sba->dev, "send message failed with error %d", ret);
                return ret;
        }

        ret = req->msg.error;

       /* Message successfully placed in the ringbuffer, i.e, done */
       mbox_client_txdone(sba->mchans[mchans_idx], ret);

       if (ret < 0) {
                dev_err(sba->dev, "message error %d", ret);
                return ret;
        }

        .....
}

This way MBOX_TX_QUEUE_LEN should be more than enough and pose no bottleneck.


Cheers!
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel

[Anup Patel]

On Thu, Jul 27, 2017 at 5:23 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Thu, Jul 27, 2017 at 11:20 AM, Anup Patel <anup.patel@broadcom.com> wrote:
>> On Thu, Jul 27, 2017 at 10:29 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
>>>>>>>> Sorry for the delayed response...
>>>>>>>>
>>>>>>>> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>>>>>>> Hi Anup,
>>>>>>>>>
>>>>>>>>> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel <anup.patel@broadcom.com> wrote:
>>>>>>>>>> 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 <anup.patel@broadcom.com>
>>>>>>>>>> Reviewed-by: Scott Branden <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];
>>>>>>>>>> +       }
>>>>>>>>>>
>>>>>>>>> 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.
>>>>>>
>>> It doesn't seem to be what I suggest. I see two issues in
>>> flexrm_process_completions()
>>> 1) It calls mbox_send_message(), which is a big NO for a controller
>>> driver. Why should you have one more message stored outside of
>>> ringbuffer?
>>
>> The "last_pending_msg" in each FlexRM ring was added to fit FlexRM
>> in Mailbox framework.
>>
>> We don't have any IRQ for TX done so "txdone_irq" out of the question for
>> FlexRM. We only have completions for both success or failures (IO errors).
>>
>> This means we have to use "txdone_poll" for FlexRM. For "txdone_poll",
>> we have to provide last_tx_done() callback. The last_tx_done() callback
>> is supposed to return true if last send_data() call succeeded.
>>
>> To implement last_tx_done() in FlexRM driver, we added "last_pending_msg".
>>
>> When "last_pending_msg" is NULL it means last call to send_data() succeeded
>> and when "last_pending_msg" is != NULL it means last call to send_data()
>> did not go through due to lack of space in FlexRM ring.
>>
> It could be simpler.
> Since flexrm_send_data() is essentially about putting the message in
> the ring-buffer (and not about _transmission_ failures), the
> last_tx_done() should simply return true if requests_ida has not all
> ids allocated. False otherwise.

It's not that simple because we have two cases in-which
send_data() will fail:
1. It run-out of IDs in requests_ida
2. There is no room in BD queue of FlexRM ring. This because each
brcm_message can be translated into variable number of descriptors.
In fact, using SPU2 crypto client we have one brcm_message translating
into 100's of descriptors. All-in-all few messages (< 1024) can also
fill-up the BD queue of FlexRM ring.

>
>>>
>>> 2) It calls mbox_chan_received_data()  which is for messages received
>>> from the remote. And not the way to report failed _transmission_, for
>>> which the api calls back mbox_client.tx_done() .  In your client
>>> driver please populate mbox_client.tx_done() and see which message is
>>> reported "sent fine" when.
>>>
>>>
>>>>>>> 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.
>>>>
>>>> Do you agree with approach taken by PATCH5 and PATCH6 to
>>>> make queue length runtime?
>>>>
>>> I agree that we may have to get the queue length from platform, if
>>> MBOX_TX_QUEUE_LEN is limiting performance. That will be easier on both
>>> of us. However I suspect the right fix for _this_ situation is in
>>> flexrm driver. See above.
>>
>> The current implementation is trying to model FlexRM using "txdone_poll"
>> method and that's why we have dependency on MBOX_TX_QUEUE_LEN
>>
>> I think what we really need is new method for "txdone" to model ring
>> manager HW (such as FlexRM). Let's call it "txdone_none".
>>
>> For "txdone_none", it means there is no "txdone" reporting in HW
>> and mbox_send_data() should simply return value returned by
>> send_data() callback. The last_tx_done() callback is not required
>> for "txdone_none" and MBOX_TX_QUEUE_LEN also has no
>> effect on "txdone_none". Both blocking and non-blocking clients
>> are treated same for "txdone_none".
>>
> That is already supported :)

If you are referring to "txdone_ack" then this cannot be used here
because for "txdone_ack" we have to call mbox_chan_txdon() API
after writing descriptors in send_data() callback which will cause
dead-lock in tx_tick() called by mbox_chan_txdone().

>
> In drivers/dma/bcm-sba-raid.c
>
> sba_send_mbox_request(...)
> {
>            ......
>         req->msg.error = 0;
>         ret = mbox_send_message(sba->mchans[mchans_idx], &req->msg);
>         if (ret < 0) {
>                 dev_err(sba->dev, "send message failed with error %d", ret);
>                 return ret;
>         }
>         ret = req->msg.error;
>         if (ret < 0) {
>                 dev_err(sba->dev, "message error %d", ret);
>                 return ret;
>         }
>           .....
> }
>
> Here you _do_ assume that as soon as the mbox_send_message() returns,
> the last_tx_done() is true. In other words, this is a case of client
> 'knows_txdone'.
>
> So ideally you should specify cl->knows_txdone = true during
> mbox_request_channel() and have ...
>
> sba_send_mbox_request(...)
> {
>         ret = mbox_send_message(sba->mchans[mchans_idx], &req->msg);
>         if (ret < 0) {
>                 dev_err(sba->dev, "send message failed with error %d", ret);
>                 return ret;
>         }
>
>         ret = req->msg.error;
>
>        /* Message successfully placed in the ringbuffer, i.e, done */
>        mbox_client_txdone(sba->mchans[mchans_idx], ret);
>
>        if (ret < 0) {
>                 dev_err(sba->dev, "message error %d", ret);
>                 return ret;
>         }
>
>         .....
> }
>

I think we need to improve mailbox.c so that
mbox_chan_txdone() can be called from
send_data() callback.

Regards,
Anup

Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel

[Jassi Brar]

On Fri, Jul 28, 2017 at 2:19 PM, Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
> On Thu, Jul 27, 2017 at 5:23 PM, Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Thu, Jul 27, 2017 at 11:20 AM, Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>> On Thu, Jul 27, 2017 at 10:29 AM, Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>>>>>>>>> Sorry for the delayed response...
>>>>>>>>>
>>>>>>>>> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>>>>>> Hi Anup,
>>>>>>>>>>
>>>>>>>>>> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>>>>>>>>>> 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 <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>>>>>>>>>> Reviewed-by: Scott Branden <scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>>>>>>>>>> ---
>>>>>>>>>>>  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.
>>>>>>>
>>>> It doesn't seem to be what I suggest. I see two issues in
>>>> flexrm_process_completions()
>>>> 1) It calls mbox_send_message(), which is a big NO for a controller
>>>> driver. Why should you have one more message stored outside of
>>>> ringbuffer?
>>>
>>> The "last_pending_msg" in each FlexRM ring was added to fit FlexRM
>>> in Mailbox framework.
>>>
>>> We don't have any IRQ for TX done so "txdone_irq" out of the question for
>>> FlexRM. We only have completions for both success or failures (IO errors).
>>>
>>> This means we have to use "txdone_poll" for FlexRM. For "txdone_poll",
>>> we have to provide last_tx_done() callback. The last_tx_done() callback
>>> is supposed to return true if last send_data() call succeeded.
>>>
>>> To implement last_tx_done() in FlexRM driver, we added "last_pending_msg".
>>>
>>> When "last_pending_msg" is NULL it means last call to send_data() succeeded
>>> and when "last_pending_msg" is != NULL it means last call to send_data()
>>> did not go through due to lack of space in FlexRM ring.
>>>
>> It could be simpler.
>> Since flexrm_send_data() is essentially about putting the message in
>> the ring-buffer (and not about _transmission_ failures), the
>> last_tx_done() should simply return true if requests_ida has not all
>> ids allocated. False otherwise.
>
> It's not that simple because we have two cases in-which
> send_data() will fail:
> 1. It run-out of IDs in requests_ida
> 2. There is no room in BD queue of FlexRM ring. This because each
> brcm_message can be translated into variable number of descriptors.
> In fact, using SPU2 crypto client we have one brcm_message translating
> into 100's of descriptors. All-in-all few messages (< 1024) can also
> fill-up the BD queue of FlexRM ring.
>
OK let me put it abstractly... return false if "there is no space for
another message in the ringbuffer", true otherwise.


>>>>
>>>> 2) It calls mbox_chan_received_data()  which is for messages received
>>>> from the remote. And not the way to report failed _transmission_, for
>>>> which the api calls back mbox_client.tx_done() .  In your client
>>>> driver please populate mbox_client.tx_done() and see which message is
>>>> reported "sent fine" when.
>>>>
>>>>
>>>>>>>> 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.
>>>>>
>>>>> Do you agree with approach taken by PATCH5 and PATCH6 to
>>>>> make queue length runtime?
>>>>>
>>>> I agree that we may have to get the queue length from platform, if
>>>> MBOX_TX_QUEUE_LEN is limiting performance. That will be easier on both
>>>> of us. However I suspect the right fix for _this_ situation is in
>>>> flexrm driver. See above.
>>>
>>> The current implementation is trying to model FlexRM using "txdone_poll"
>>> method and that's why we have dependency on MBOX_TX_QUEUE_LEN
>>>
>>> I think what we really need is new method for "txdone" to model ring
>>> manager HW (such as FlexRM). Let's call it "txdone_none".
>>>
>>> For "txdone_none", it means there is no "txdone" reporting in HW
>>> and mbox_send_data() should simply return value returned by
>>> send_data() callback. The last_tx_done() callback is not required
>>> for "txdone_none" and MBOX_TX_QUEUE_LEN also has no
>>> effect on "txdone_none". Both blocking and non-blocking clients
>>> are treated same for "txdone_none".
>>>
>> That is already supported :)
>
> If you are referring to "txdone_ack" then this cannot be used here
> because for "txdone_ack" we have to call mbox_chan_txdon() API
> after writing descriptors in send_data() callback which will cause
> dead-lock in tx_tick() called by mbox_chan_txdone().
>
Did you read my code snippet below?

It's not mbox_chan_txdone(), but mbox_client_txdone() which is called
by the client.

>>
>> In drivers/dma/bcm-sba-raid.c
>>
>> sba_send_mbox_request(...)
>> {
>>            ......
>>         req->msg.error = 0;
>>         ret = mbox_send_message(sba->mchans[mchans_idx], &req->msg);
>>         if (ret < 0) {
>>                 dev_err(sba->dev, "send message failed with error %d", ret);
>>                 return ret;
>>         }
>>         ret = req->msg.error;
>>         if (ret < 0) {
>>                 dev_err(sba->dev, "message error %d", ret);
>>                 return ret;
>>         }
>>           .....
>> }
>>
>> Here you _do_ assume that as soon as the mbox_send_message() returns,
>> the last_tx_done() is true. In other words, this is a case of client
>> 'knows_txdone'.
>>
>> So ideally you should specify cl->knows_txdone = true during
>> mbox_request_channel() and have ...
>>
>> sba_send_mbox_request(...)
>> {
>>         ret = mbox_send_message(sba->mchans[mchans_idx], &req->msg);
>>         if (ret < 0) {
>>                 dev_err(sba->dev, "send message failed with error %d", ret);
>>                 return ret;
>>         }
>>
>>         ret = req->msg.error;
>>
>>        /* Message successfully placed in the ringbuffer, i.e, done */
>>        mbox_client_txdone(sba->mchans[mchans_idx], ret);
>>
>>        if (ret < 0) {
>>                 dev_err(sba->dev, "message error %d", ret);
>>                 return ret;
>>         }
>>
>>         .....
>> }
>>
>
> I think we need to improve mailbox.c so that
> mbox_chan_txdone() can be called from
> send_data() callback.
>
No please. Other clients call mbox_send_message() followed by
mbox_client_txdone(), and they are right. For example,
drivers/firmware/tegra/bpmp.c

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel

[Anup Patel]

On Fri, Jul 28, 2017 at 2:34 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Fri, Jul 28, 2017 at 2:19 PM, Anup Patel <anup.patel@broadcom.com> wrote:
>> On Thu, Jul 27, 2017 at 5:23 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>> On Thu, Jul 27, 2017 at 11:20 AM, Anup Patel <anup.patel@broadcom.com> wrote:
>>>> On Thu, Jul 27, 2017 at 10:29 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>
>>>>>>>>>> Sorry for the delayed response...
>>>>>>>>>>
>>>>>>>>>> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>>>>>>>>> Hi Anup,
>>>>>>>>>>>
>>>>>>>>>>> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel <anup.patel@broadcom.com> wrote:
>>>>>>>>>>>> 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 <anup.patel@broadcom.com>
>>>>>>>>>>>> Reviewed-by: Scott Branden <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];
>>>>>>>>>>>> +       }
>>>>>>>>>>>>
>>>>>>>>>>> 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.
>>>>>>>>
>>>>> It doesn't seem to be what I suggest. I see two issues in
>>>>> flexrm_process_completions()
>>>>> 1) It calls mbox_send_message(), which is a big NO for a controller
>>>>> driver. Why should you have one more message stored outside of
>>>>> ringbuffer?
>>>>
>>>> The "last_pending_msg" in each FlexRM ring was added to fit FlexRM
>>>> in Mailbox framework.
>>>>
>>>> We don't have any IRQ for TX done so "txdone_irq" out of the question for
>>>> FlexRM. We only have completions for both success or failures (IO errors).
>>>>
>>>> This means we have to use "txdone_poll" for FlexRM. For "txdone_poll",
>>>> we have to provide last_tx_done() callback. The last_tx_done() callback
>>>> is supposed to return true if last send_data() call succeeded.
>>>>
>>>> To implement last_tx_done() in FlexRM driver, we added "last_pending_msg".
>>>>
>>>> When "last_pending_msg" is NULL it means last call to send_data() succeeded
>>>> and when "last_pending_msg" is != NULL it means last call to send_data()
>>>> did not go through due to lack of space in FlexRM ring.
>>>>
>>> It could be simpler.
>>> Since flexrm_send_data() is essentially about putting the message in
>>> the ring-buffer (and not about _transmission_ failures), the
>>> last_tx_done() should simply return true if requests_ida has not all
>>> ids allocated. False otherwise.
>>
>> It's not that simple because we have two cases in-which
>> send_data() will fail:
>> 1. It run-out of IDs in requests_ida
>> 2. There is no room in BD queue of FlexRM ring. This because each
>> brcm_message can be translated into variable number of descriptors.
>> In fact, using SPU2 crypto client we have one brcm_message translating
>> into 100's of descriptors. All-in-all few messages (< 1024) can also
>> fill-up the BD queue of FlexRM ring.
>>
> OK let me put it abstractly... return false if "there is no space for
> another message in the ringbuffer", true otherwise.

Let say at time T, there was no space in BD queue. Now at
time T+X when last_tx_done() it is possible that BD queue
has space because FlexRM has processed some more
descriptor.

I think last_tx_done() for "txdone_poll" method will require
some information passing from send_data() callback to
last_tx_done() which is last_pending_msg for FlexRM driver.

Anyways, I plan to try "txdone_ack" method so I will
remove last_tx_done() and last_pending_msg both.
What do you think?

>
>
>>>>>
>>>>> 2) It calls mbox_chan_received_data()  which is for messages received
>>>>> from the remote. And not the way to report failed _transmission_, for
>>>>> which the api calls back mbox_client.tx_done() .  In your client
>>>>> driver please populate mbox_client.tx_done() and see which message is
>>>>> reported "sent fine" when.
>>>>>
>>>>>
>>>>>>>>> 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.
>>>>>>
>>>>>> Do you agree with approach taken by PATCH5 and PATCH6 to
>>>>>> make queue length runtime?
>>>>>>
>>>>> I agree that we may have to get the queue length from platform, if
>>>>> MBOX_TX_QUEUE_LEN is limiting performance. That will be easier on both
>>>>> of us. However I suspect the right fix for _this_ situation is in
>>>>> flexrm driver. See above.
>>>>
>>>> The current implementation is trying to model FlexRM using "txdone_poll"
>>>> method and that's why we have dependency on MBOX_TX_QUEUE_LEN
>>>>
>>>> I think what we really need is new method for "txdone" to model ring
>>>> manager HW (such as FlexRM). Let's call it "txdone_none".
>>>>
>>>> For "txdone_none", it means there is no "txdone" reporting in HW
>>>> and mbox_send_data() should simply return value returned by
>>>> send_data() callback. The last_tx_done() callback is not required
>>>> for "txdone_none" and MBOX_TX_QUEUE_LEN also has no
>>>> effect on "txdone_none". Both blocking and non-blocking clients
>>>> are treated same for "txdone_none".
>>>>
>>> That is already supported :)
>>
>> If you are referring to "txdone_ack" then this cannot be used here
>> because for "txdone_ack" we have to call mbox_chan_txdon() API
>> after writing descriptors in send_data() callback which will cause
>> dead-lock in tx_tick() called by mbox_chan_txdone().
>>
> Did you read my code snippet below?
>
> It's not mbox_chan_txdone(), but mbox_client_txdone() which is called
> by the client.
>
>>>
>>> In drivers/dma/bcm-sba-raid.c
>>>
>>> sba_send_mbox_request(...)
>>> {
>>>            ......
>>>         req->msg.error = 0;
>>>         ret = mbox_send_message(sba->mchans[mchans_idx], &req->msg);
>>>         if (ret < 0) {
>>>                 dev_err(sba->dev, "send message failed with error %d", ret);
>>>                 return ret;
>>>         }
>>>         ret = req->msg.error;
>>>         if (ret < 0) {
>>>                 dev_err(sba->dev, "message error %d", ret);
>>>                 return ret;
>>>         }
>>>           .....
>>> }
>>>
>>> Here you _do_ assume that as soon as the mbox_send_message() returns,
>>> the last_tx_done() is true. In other words, this is a case of client
>>> 'knows_txdone'.
>>>
>>> So ideally you should specify cl->knows_txdone = true during
>>> mbox_request_channel() and have ...
>>>
>>> sba_send_mbox_request(...)
>>> {
>>>         ret = mbox_send_message(sba->mchans[mchans_idx], &req->msg);
>>>         if (ret < 0) {
>>>                 dev_err(sba->dev, "send message failed with error %d", ret);
>>>                 return ret;
>>>         }
>>>
>>>         ret = req->msg.error;
>>>
>>>        /* Message successfully placed in the ringbuffer, i.e, done */
>>>        mbox_client_txdone(sba->mchans[mchans_idx], ret);
>>>
>>>        if (ret < 0) {
>>>                 dev_err(sba->dev, "message error %d", ret);
>>>                 return ret;
>>>         }
>>>
>>>         .....
>>> }
>>>
>>
>> I think we need to improve mailbox.c so that
>> mbox_chan_txdone() can be called from
>> send_data() callback.
>>
> No please. Other clients call mbox_send_message() followed by
> mbox_client_txdone(), and they are right. For example,
> drivers/firmware/tegra/bpmp.c

OK so I got confused between mbox_chan_txdone() and
mbox_client_txdone().

We should do mbox_client_txdone() from mailbox client
when mbox_chan txmethod is ACK.

Regards,
Anup

Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel

[Jassi Brar]

On Fri, Jul 28, 2017 at 3:18 PM, Anup Patel <anup.patel@broadcom.com> wrote:
> On Fri, Jul 28, 2017 at 2:34 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>> On Fri, Jul 28, 2017 at 2:19 PM, Anup Patel <anup.patel@broadcom.com> wrote:
>>> On Thu, Jul 27, 2017 at 5:23 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>> On Thu, Jul 27, 2017 at 11:20 AM, Anup Patel <anup.patel@broadcom.com> wrote:
>>>>> On Thu, Jul 27, 2017 at 10:29 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>>
>>>>>>>>>>> Sorry for the delayed response...
>>>>>>>>>>>
>>>>>>>>>>> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>>>>>>>>>> Hi Anup,
>>>>>>>>>>>>
>>>>>>>>>>>> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel <anup.patel@broadcom.com> wrote:
>>>>>>>>>>>>> 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 <anup.patel@broadcom.com>
>>>>>>>>>>>>> Reviewed-by: Scott Branden <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];
>>>>>>>>>>>>> +       }
>>>>>>>>>>>>>
>>>>>>>>>>>> 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.
>>>>>>>>>
>>>>>> It doesn't seem to be what I suggest. I see two issues in
>>>>>> flexrm_process_completions()
>>>>>> 1) It calls mbox_send_message(), which is a big NO for a controller
>>>>>> driver. Why should you have one more message stored outside of
>>>>>> ringbuffer?
>>>>>
>>>>> The "last_pending_msg" in each FlexRM ring was added to fit FlexRM
>>>>> in Mailbox framework.
>>>>>
>>>>> We don't have any IRQ for TX done so "txdone_irq" out of the question for
>>>>> FlexRM. We only have completions for both success or failures (IO errors).
>>>>>
>>>>> This means we have to use "txdone_poll" for FlexRM. For "txdone_poll",
>>>>> we have to provide last_tx_done() callback. The last_tx_done() callback
>>>>> is supposed to return true if last send_data() call succeeded.
>>>>>
>>>>> To implement last_tx_done() in FlexRM driver, we added "last_pending_msg".
>>>>>
>>>>> When "last_pending_msg" is NULL it means last call to send_data() succeeded
>>>>> and when "last_pending_msg" is != NULL it means last call to send_data()
>>>>> did not go through due to lack of space in FlexRM ring.
>>>>>
>>>> It could be simpler.
>>>> Since flexrm_send_data() is essentially about putting the message in
>>>> the ring-buffer (and not about _transmission_ failures), the
>>>> last_tx_done() should simply return true if requests_ida has not all
>>>> ids allocated. False otherwise.
>>>
>>> It's not that simple because we have two cases in-which
>>> send_data() will fail:
>>> 1. It run-out of IDs in requests_ida
>>> 2. There is no room in BD queue of FlexRM ring. This because each
>>> brcm_message can be translated into variable number of descriptors.
>>> In fact, using SPU2 crypto client we have one brcm_message translating
>>> into 100's of descriptors. All-in-all few messages (< 1024) can also
>>> fill-up the BD queue of FlexRM ring.
>>>
>> OK let me put it abstractly... return false if "there is no space for
>> another message in the ringbuffer", true otherwise.
>
> Let say at time T, there was no space in BD queue. Now at
> time T+X when last_tx_done() it is possible that BD queue
> has space because FlexRM has processed some more
> descriptor.
>
> I think last_tx_done() for "txdone_poll" method will require
> some information passing from send_data() callback to
> last_tx_done() which is last_pending_msg for FlexRM driver.
>
The problem is flexrm_send_data() accepts single as well as batched
messages, so each send_data() can require different spaces. If you
make flexrm_send_data() accept fixed size messages then you can simply
set a flag (say, last_tx_busy) when max possible messages are queued
and unset that flag in flexrm_process_completions().


> Anyways, I plan to try "txdone_ack" method so I will
> remove last_tx_done() and last_pending_msg both.
> What do you think?
>
Sounds good.


>>
>>>>>>
>>>>>> 2) It calls mbox_chan_received_data()  which is for messages received
>>>>>> from the remote. And not the way to report failed _transmission_, for
>>>>>> which the api calls back mbox_client.tx_done() .  In your client
>>>>>> driver please populate mbox_client.tx_done() and see which message is
>>>>>> reported "sent fine" when.
>>>>>>
>>>>>>
>>>>>>>>>> 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.
>>>>>>>
>>>>>>> Do you agree with approach taken by PATCH5 and PATCH6 to
>>>>>>> make queue length runtime?
>>>>>>>
>>>>>> I agree that we may have to get the queue length from platform, if
>>>>>> MBOX_TX_QUEUE_LEN is limiting performance. That will be easier on both
>>>>>> of us. However I suspect the right fix for _this_ situation is in
>>>>>> flexrm driver. See above.
>>>>>
>>>>> The current implementation is trying to model FlexRM using "txdone_poll"
>>>>> method and that's why we have dependency on MBOX_TX_QUEUE_LEN
>>>>>
>>>>> I think what we really need is new method for "txdone" to model ring
>>>>> manager HW (such as FlexRM). Let's call it "txdone_none".
>>>>>
>>>>> For "txdone_none", it means there is no "txdone" reporting in HW
>>>>> and mbox_send_data() should simply return value returned by
>>>>> send_data() callback. The last_tx_done() callback is not required
>>>>> for "txdone_none" and MBOX_TX_QUEUE_LEN also has no
>>>>> effect on "txdone_none". Both blocking and non-blocking clients
>>>>> are treated same for "txdone_none".
>>>>>
>>>> That is already supported :)
>>>
>>> If you are referring to "txdone_ack" then this cannot be used here
>>> because for "txdone_ack" we have to call mbox_chan_txdon() API
>>> after writing descriptors in send_data() callback which will cause
>>> dead-lock in tx_tick() called by mbox_chan_txdone().
>>>
>> Did you read my code snippet below?
>>
>> It's not mbox_chan_txdone(), but mbox_client_txdone() which is called
>> by the client.
>>
>>>>
>>>> In drivers/dma/bcm-sba-raid.c
>>>>
>>>> sba_send_mbox_request(...)
>>>> {
>>>>            ......
>>>>         req->msg.error = 0;
>>>>         ret = mbox_send_message(sba->mchans[mchans_idx], &req->msg);
>>>>         if (ret < 0) {
>>>>                 dev_err(sba->dev, "send message failed with error %d", ret);
>>>>                 return ret;
>>>>         }
>>>>         ret = req->msg.error;
>>>>         if (ret < 0) {
>>>>                 dev_err(sba->dev, "message error %d", ret);
>>>>                 return ret;
>>>>         }
>>>>           .....
>>>> }
>>>>
>>>> Here you _do_ assume that as soon as the mbox_send_message() returns,
>>>> the last_tx_done() is true. In other words, this is a case of client
>>>> 'knows_txdone'.
>>>>
>>>> So ideally you should specify cl->knows_txdone = true during
>>>> mbox_request_channel() and have ...
>>>>
>>>> sba_send_mbox_request(...)
>>>> {
>>>>         ret = mbox_send_message(sba->mchans[mchans_idx], &req->msg);
>>>>         if (ret < 0) {
>>>>                 dev_err(sba->dev, "send message failed with error %d", ret);
>>>>                 return ret;
>>>>         }
>>>>
>>>>         ret = req->msg.error;
>>>>
>>>>        /* Message successfully placed in the ringbuffer, i.e, done */
>>>>        mbox_client_txdone(sba->mchans[mchans_idx], ret);
>>>>
>>>>        if (ret < 0) {
>>>>                 dev_err(sba->dev, "message error %d", ret);
>>>>                 return ret;
>>>>         }
>>>>
>>>>         .....
>>>> }
>>>>
>>>
>>> I think we need to improve mailbox.c so that
>>> mbox_chan_txdone() can be called from
>>> send_data() callback.
>>>
>> No please. Other clients call mbox_send_message() followed by
>> mbox_client_txdone(), and they are right. For example,
>> drivers/firmware/tegra/bpmp.c
>
> OK so I got confused between mbox_chan_txdone() and
> mbox_client_txdone().
>
> We should do mbox_client_txdone() from mailbox client
> when mbox_chan txmethod is ACK.
>
Yes.

Thanks.

[PATCH v2 7/7] arm64: dts: Add FlexRM DT nodes for Stingray

[Anup Patel]

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 <anup.patel@broadcom.com>
Signed-off-by: Raveendra Padasalagi <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: crypto_mbox@00000000 {
+			compatible = "brcm,iproc-flexrm-mbox";
+			reg = <0x00000000 0x200000>;
+			msi-parent = <&gic_its 0x4100>;
+			#mbox-cells = <3>;
+			dma-coherent;
+		};
+
+		raid_mbox: 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