[PATCHv7 0/2] Adding driver for Xilinx LogiCORE IP mailbox.

[PATCHv7 0/2] Adding driver for Xilinx LogiCORE IP mailbox.

[Moritz Fischer]

Hi all,

Thanks for hanging in there, hopefully the last round of review ...

This patchset adds mailbox framework integration for the Xilinx LogiCORE IP
mailbox.  The Xilinx LogiCORE IP mailbox is a fpga softcore that allows
interprocessor communication between AXI4 stream / memory mapped
processors.

Cheers,

Moritz

Changes from v6:
- As suggested by Jassi and S?ren use #mbox-cells = <1>

Changes from v5:
- Fixed constness for ops struct
- Removed redundant checks
- Moved ops assignment
- Removed MODULE_ALIAS

Changes from v4:
- Have separate mbox_ops structs for polling / irq mode
- Moved clk handling to startup / shutdown
- Embedded struct mbox_chan in struct xilinx_mbox
- Fixed mbox-cells in devicetree documentation
- Misc stylistic issues

Changes from v3:
- Stylistic changes
- Changed reg size in dts to 0x100

Changes from v2:
- Fixed error condition for IRQ from >= 0 to > 0
- Fixed clock enable
- Cleaned up docs

Changes from v1:
- Added common clock framework support
- Deal with IRQs that happend before driver load,
  since HW will not let us know about them when we enable IRQs

Changes from v0:
- Several stylistic issues
- Dropped superfluous intr_mode member
- Really masking the IRQs on mailbox_shutdown
- No longer using polling by accident in non-IRQ mode
- Swapped doc and driver commits


Moritz Fischer (2):
  dts: Adding docs for Xilinx LogiCORE IP mailbox driver.
  mailbox: Adding driver for Xilinx LogiCORE IP mailbox.

 .../devicetree/bindings/mailbox/xilinx-mailbox.txt |  44 +++
 MAINTAINERS                                        |   7 +
 drivers/mailbox/Kconfig                            |   7 +
 drivers/mailbox/Makefile                           |   2 +
 drivers/mailbox/mailbox-xilinx.c                   | 367 +++++++++++++++++++++
 5 files changed, 427 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/xilinx-mailbox.txt
 create mode 100644 drivers/mailbox/mailbox-xilinx.c

-- 
2.4.3

[PATCHv7 1/2] dts: Adding docs for Xilinx LogiCORE IP mailbox driver.

[Moritz Fischer]

Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
Acked-by: Michal Simek <michal.simek@xilinx.com>
---
 .../devicetree/bindings/mailbox/xilinx-mailbox.txt | 44 ++++++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/xilinx-mailbox.txt

diff --git a/Documentation/devicetree/bindings/mailbox/xilinx-mailbox.txt b/Documentation/devicetree/bindings/mailbox/xilinx-mailbox.txt
new file mode 100644
index 0000000..f9ec46d
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/xilinx-mailbox.txt
@@ -0,0 +1,44 @@
+Xilinx Mailbox Driver
+=====================
+
+Required properties:
+- compatible       : "xlnx,mailbox-2.1".
+- reg              :  physical base address of the mailbox and length of
+                      memory mapped region.
+- #mbox-cells      :  common mailbox binding property to identify the number
+                      of cells required for the mailbox specifier, should be 1
+- clocks           :  phandle to clock provider
+- clock-names      :  must be 'mbox'
+
+Optional properties:
+- interrupt-parent : interrupt source phandle
+- interrupts       : interrupt number, The interrupt specifier format
+                     depends on the interrupt controller parent.
+
+Example:
+	mbox: mailbox at 40400000 {
+		compatible = "xlnx,mailbox-2.1";
+		reg = <0x40400000 0x100>;
+		interrupt-parent = <&intc>;
+		interrupts = <5>;
+		#mbox-cells = <1>;
+		clocks = <&clkc 15>;
+		clock-names = "mbox";
+	};
+
+Mailbox client
+===============
+"mboxes" and the optional "mbox-names" (please see
+Documentation/devicetree/bindings/mailbox/mailbox.txt for details). Each value
+of the mboxes property should contain a phandle to the mailbox controller
+device node and second argument is the channel index. It must be 0 (hardware
+support only one channel). The equivalent "mbox-names" property value can be
+used to give a name to the communication channel to be used by the client user.
+
+Example:
+	mclient0: mclient0 at 400 {
+		compatible = "client-1.0";
+		reg = <0x400 0x10>;
+		mbox-names = "mbox";
+		mboxes = <&mbox 0>;
+	};
-- 
2.4.3

[PATCHv7 1/2] dts: Adding docs for Xilinx LogiCORE IP mailbox driver.

[Sören Brinkmann]

On Tue, 2015-07-14 at 06:00PM -0700, Moritz Fischer wrote:
> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
> Acked-by: Michal Simek <michal.simek@xilinx.com>
Acked-by: S?ren Brinkmann <soren.brinkmann@xilinx.com>

	S?ren

[PATCHv7 2/2] mailbox: Adding driver for Xilinx LogiCORE IP mailbox.

[Moritz Fischer]

The Xilinx LogiCORE IP mailbox is a FPGA core that allows for
interprocessor communication via AXI4 memory mapped / AXI4 stream
interfaces.

It is single channel per core and allows for transmit and receive.

Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
Acked-by: Michal Simek <michal.simek@xilinx.com>
---
 MAINTAINERS                      |   7 +
 drivers/mailbox/Kconfig          |   7 +
 drivers/mailbox/Makefile         |   2 +
 drivers/mailbox/mailbox-xilinx.c | 367 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 383 insertions(+)
 create mode 100644 drivers/mailbox/mailbox-xilinx.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 2d3d55c..bfe2b78 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11320,6 +11320,13 @@ M:	John Linn <John.Linn@xilinx.com>
 S:	Maintained
 F:	drivers/net/ethernet/xilinx/xilinx_axienet*
 
+XILINX MAILBOX DRIVER
+M:	Moritz Fischer <moritz.fischer@ettus.com>
+L:	linux-kernel at vger.kernel.org
+S:	Maintained
+F:	drivers/mailbox/mailbox-xilinx.c
+F:	Documentation/devicetree/bindings/mailbox/xilinx-mailbox.txt
+
 XILINX UARTLITE SERIAL DRIVER
 M:	Peter Korsgaard <jacmet@sunsite.dk>
 L:	linux-serial at vger.kernel.org
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index e269f08..15b3ade 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -70,4 +70,11 @@ config BCM2835_MBOX
 	  the services of the Videocore. Say Y here if you want to use the
 	  BCM2835 Mailbox.
 
+config XILINX_MBOX
+	tristate "Xilinx Mailbox"
+	help
+	  An implementation of the Xilinx Mailbox soft core. It is used
+	  to send message between processors. Say Y here if you want to use the
+	  Xilinx mailbox support.
+
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 8e6d822..1966e26 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -13,3 +13,5 @@ obj-$(CONFIG_PCC)		+= pcc.o
 obj-$(CONFIG_ALTERA_MBOX)	+= mailbox-altera.o
 
 obj-$(CONFIG_BCM2835_MBOX)	+= bcm2835-mailbox.o
+
+obj-$(CONFIG_XILINX_MBOX)	+= mailbox-xilinx.o
diff --git a/drivers/mailbox/mailbox-xilinx.c b/drivers/mailbox/mailbox-xilinx.c
new file mode 100644
index 0000000..27011e6
--- /dev/null
+++ b/drivers/mailbox/mailbox-xilinx.c
@@ -0,0 +1,367 @@
+/*
+ * Copyright (c) 2015, National Instruments Corp. All rights reserved.
+ *
+ * Driver for the Xilinx LogiCORE mailbox IP block
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+/* register offsets */
+#define MAILBOX_REG_WRDATA	0x00
+#define MAILBOX_REG_RDDATA	0x08
+#define MAILBOX_REG_STATUS	0x10
+#define MAILBOX_REG_ERROR	0x14
+#define MAILBOX_REG_SIT	0x18
+#define MAILBOX_REG_RIT	0x1c
+#define MAILBOX_REG_IS	0x20
+#define MAILBOX_REG_IE	0x24
+#define MAILBOX_REG_IP	0x28
+
+/* status register */
+#define STS_RTA	BIT(3)
+#define STS_STA	BIT(2)
+#define STS_FULL	BIT(1)
+#define STS_EMPTY	BIT(0)
+
+/* error register */
+#define ERR_FULL	BIT(1)
+#define ERR_EMPTY	BIT(0)
+
+/* mailbox interrupt status register */
+#define INT_STATUS_ERR	BIT(2)
+#define INT_STATUS_RTI	BIT(1)
+#define INT_STATUS_STI	BIT(0)
+
+/* mailbox interrupt enable register */
+#define INT_ENABLE_ERR	BIT(2)
+#define INT_ENABLE_RTI	BIT(1)
+#define INT_ENABLE_STI	BIT(0)
+
+#define MBOX_POLLING_MS		5	/* polling interval 5ms */
+
+struct xilinx_mbox {
+	int irq;
+	void __iomem *mbox_base;
+	struct clk *clk;
+	struct device *dev;
+	struct mbox_controller controller;
+	struct mbox_chan chan;
+
+	/* if the controller supports only RX polling mode */
+	struct timer_list rxpoll_timer;
+};
+
+static struct xilinx_mbox *mbox_chan_to_xilinx_mbox(struct mbox_chan *chan)
+{
+	return container_of(chan, struct xilinx_mbox, chan);
+}
+
+static inline bool xilinx_mbox_full(struct xilinx_mbox *mbox)
+{
+	u32 status;
+
+	status = readl_relaxed(mbox->mbox_base + MAILBOX_REG_STATUS);
+
+	return status & STS_FULL;
+}
+
+static inline bool xilinx_mbox_pending(struct xilinx_mbox *mbox)
+{
+	u32 status;
+
+	status = readl_relaxed(mbox->mbox_base + MAILBOX_REG_STATUS);
+
+	return !(status & STS_EMPTY);
+}
+
+static void xilinx_mbox_intmask(struct xilinx_mbox *mbox, u32 mask, bool enable)
+{
+	u32 mask_reg;
+
+	mask_reg = readl_relaxed(mbox->mbox_base + MAILBOX_REG_IE);
+	if (enable)
+		mask_reg |= mask;
+	else
+		mask_reg &= ~mask;
+
+	writel_relaxed(mask_reg, mbox->mbox_base + MAILBOX_REG_IE);
+}
+
+static inline void xilinx_mbox_rx_intmask(struct xilinx_mbox *mbox, bool enable)
+{
+	xilinx_mbox_intmask(mbox, INT_ENABLE_RTI, enable);
+}
+
+static inline void xilinx_mbox_tx_intmask(struct xilinx_mbox *mbox, bool enable)
+{
+	xilinx_mbox_intmask(mbox, INT_ENABLE_STI, enable);
+}
+
+static void xilinx_mbox_rx_data(struct mbox_chan *chan)
+{
+	struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
+	u32 data;
+
+	if (xilinx_mbox_pending(mbox)) {
+		data = readl_relaxed(mbox->mbox_base + MAILBOX_REG_RDDATA);
+		mbox_chan_received_data(chan, (void *)data);
+	}
+}
+
+static void xilinx_mbox_poll_rx(unsigned long data)
+{
+	struct mbox_chan *chan = (struct mbox_chan *)data;
+	struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
+
+	xilinx_mbox_rx_data(chan);
+
+	mod_timer(&mbox->rxpoll_timer,
+		  jiffies + msecs_to_jiffies(MBOX_POLLING_MS));
+}
+
+static irqreturn_t xilinx_mbox_interrupt(int irq, void *p)
+{
+	u32 mask;
+	struct mbox_chan *chan = (struct mbox_chan *)p;
+	struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
+
+	mask = readl_relaxed(mbox->mbox_base + MAILBOX_REG_IS);
+	if (mask & INT_STATUS_RTI)
+		xilinx_mbox_rx_data(chan);
+
+	/* mask irqs *before* notifying done, require tx_block=true */
+	if (mask & INT_STATUS_STI) {
+		xilinx_mbox_tx_intmask(mbox, false);
+		mbox_chan_txdone(chan, 0);
+	}
+
+	writel_relaxed(mask, mbox->mbox_base + MAILBOX_REG_IS);
+
+	return IRQ_HANDLED;
+}
+
+static int xilinx_mbox_irq_startup(struct mbox_chan *chan)
+{
+	int ret;
+	struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
+
+	ret = request_irq(mbox->irq, xilinx_mbox_interrupt, 0,
+			  dev_name(mbox->dev), chan);
+	if (unlikely(ret)) {
+		dev_err(mbox->dev,
+			"failed to register mailbox interrupt:%d\n",
+			ret);
+		return ret;
+	}
+
+	/* prep and enable the clock */
+	clk_prepare_enable(mbox->clk);
+
+	xilinx_mbox_rx_intmask(mbox, true);
+
+	/* if fifo was full already, we didn't get an interrupt */
+	while (xilinx_mbox_pending(mbox))
+		xilinx_mbox_rx_data(chan);
+
+	return 0;
+}
+
+static int xilinx_mbox_poll_startup(struct mbox_chan *chan)
+{
+	struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
+
+	clk_prepare_enable(mbox->clk);
+
+	mod_timer(&mbox->rxpoll_timer,
+		  jiffies + msecs_to_jiffies(MBOX_POLLING_MS));
+
+	return 0;
+}
+
+static int xilinx_mbox_irq_send_data(struct mbox_chan *chan, void *data)
+{
+	struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
+	u32 *udata = data;
+
+	if (xilinx_mbox_full(mbox))
+		return -EBUSY;
+
+	/* enable interrupt before send */
+	xilinx_mbox_tx_intmask(mbox, true);
+
+	writel_relaxed(*udata, mbox->mbox_base + MAILBOX_REG_WRDATA);
+
+	return 0;
+}
+
+static int xilinx_mbox_poll_send_data(struct mbox_chan *chan, void *data)
+{
+	struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
+	u32 *udata = data;
+
+	if (!mbox || !data)
+		return -EINVAL;
+
+	if (xilinx_mbox_full(mbox))
+		return -EBUSY;
+
+	writel_relaxed(*udata, mbox->mbox_base + MAILBOX_REG_WRDATA);
+
+	return 0;
+}
+
+static bool xilinx_mbox_last_tx_done(struct mbox_chan *chan)
+{
+	struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
+
+	/* return false if mailbox is full */
+	return !xilinx_mbox_full(mbox);
+}
+
+static bool xilinx_mbox_peek_data(struct mbox_chan *chan)
+{
+	struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
+
+	return xilinx_mbox_pending(mbox);
+}
+
+static void xilinx_mbox_irq_shutdown(struct mbox_chan *chan)
+{
+	struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
+
+	/* mask all interrupts */
+	writel_relaxed(0, mbox->mbox_base + MAILBOX_REG_IE);
+
+	clk_disable_unprepare(mbox->clk);
+
+	free_irq(mbox->irq, chan);
+}
+
+static void xilinx_mbox_poll_shutdown(struct mbox_chan *chan)
+{
+	struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
+
+	del_timer_sync(&mbox->rxpoll_timer);
+
+	clk_disable_unprepare(mbox->clk);
+}
+
+static const struct mbox_chan_ops xilinx_mbox_irq_ops = {
+	.send_data = xilinx_mbox_irq_send_data,
+	.startup = xilinx_mbox_irq_startup,
+	.shutdown = xilinx_mbox_irq_shutdown,
+	.last_tx_done = xilinx_mbox_last_tx_done,
+	.peek_data = xilinx_mbox_peek_data,
+};
+
+static const struct mbox_chan_ops xilinx_mbox_poll_ops = {
+	.send_data = xilinx_mbox_poll_send_data,
+	.startup = xilinx_mbox_poll_startup,
+	.shutdown = xilinx_mbox_poll_shutdown,
+	.last_tx_done = xilinx_mbox_last_tx_done,
+	.peek_data = xilinx_mbox_peek_data,
+};
+
+static int xilinx_mbox_probe(struct platform_device *pdev)
+{
+	struct xilinx_mbox *mbox;
+	struct resource	*regs;
+	int ret;
+
+	mbox = devm_kzalloc(&pdev->dev, sizeof(*mbox), GFP_KERNEL);
+	if (!mbox)
+		return -ENOMEM;
+
+	/* get clk and enable */
+	mbox->clk = devm_clk_get(&pdev->dev, "mbox");
+	if (IS_ERR(mbox->clk)) {
+		dev_err(&pdev->dev, "Couldn't get clk.\n");
+		return PTR_ERR(mbox->clk);
+	}
+
+	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	mbox->mbox_base = devm_ioremap_resource(&pdev->dev, regs);
+	if (IS_ERR(mbox->mbox_base))
+		return PTR_ERR(mbox->mbox_base);
+
+	mbox->irq = platform_get_irq(pdev, 0);
+	/* if irq is present, we can use it, otherwise, poll */
+	if (mbox->irq > 0) {
+		mbox->controller.txdone_irq = true;
+		mbox->controller.ops = &xilinx_mbox_irq_ops;
+	} else {
+		dev_info(&pdev->dev, "IRQ not found, fallback to polling.\n");
+		mbox->controller.txdone_poll = true;
+		mbox->controller.txpoll_period = MBOX_POLLING_MS;
+		mbox->controller.ops = &xilinx_mbox_poll_ops;
+
+		setup_timer(&mbox->rxpoll_timer, xilinx_mbox_poll_rx,
+			    (unsigned long)&mbox->chan);
+	}
+
+	mbox->dev = &pdev->dev;
+
+	/* hardware supports only one channel. */
+	mbox->controller.dev = mbox->dev;
+	mbox->controller.num_chans = 1;
+	mbox->controller.chans = &mbox->chan;
+
+	ret = mbox_controller_register(&mbox->controller);
+	if (ret) {
+		dev_err(&pdev->dev, "Register mailbox failed\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, mbox);
+
+	return 0;
+}
+
+static int xilinx_mbox_remove(struct platform_device *pdev)
+{
+	struct xilinx_mbox *mbox = platform_get_drvdata(pdev);
+
+	mbox_controller_unregister(&mbox->controller);
+
+	return 0;
+}
+
+static const struct of_device_id xilinx_mbox_match[] = {
+	{ .compatible = "xlnx,mailbox-2.1" },
+	{ /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, xilinx_mbox_match);
+
+static struct platform_driver xilinx_mbox_driver = {
+	.probe	= xilinx_mbox_probe,
+	.remove	= xilinx_mbox_remove,
+	.driver	= {
+		.name	= KBUILD_MODNAME,
+		.of_match_table	= xilinx_mbox_match,
+	},
+};
+
+module_platform_driver(xilinx_mbox_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Xilinx mailbox specific functions");
+MODULE_AUTHOR("Moritz Fischer <moritz.fischer@ettus.com>");
-- 
2.4.3

[PATCHv7 2/2] mailbox: Adding driver for Xilinx LogiCORE IP mailbox.

[Jassi Brar]

On Wed, Jul 15, 2015 at 6:30 AM, Moritz Fischer
<moritz.fischer@ettus.com> wrote:

> +
> +static void xilinx_mbox_rx_data(struct mbox_chan *chan)
> +{
> +       struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
> +       u32 data;
> +
> +       if (xilinx_mbox_pending(mbox)) {
> +               data = readl_relaxed(mbox->mbox_base + MAILBOX_REG_RDDATA);
> +               mbox_chan_received_data(chan, (void *)data);
>
If RDDATA is a FIFO, then above seems wrong - you are assuming
messages are always going to be exactly 32-bits for every protocol.
Ideally you should empty the fifo fully, at least when RX has an
interrupt.

> +
> +static int xilinx_mbox_irq_send_data(struct mbox_chan *chan, void *data)
> +{
> +       struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
> +       u32 *udata = data;
> +
> +       if (xilinx_mbox_full(mbox))
> +               return -EBUSY;
>
This check is redundant. last_tx_done already makes sure this is always true.

> +       /* enable interrupt before send */
> +       xilinx_mbox_tx_intmask(mbox, true);
> +
> +       writel_relaxed(*udata, mbox->mbox_base + MAILBOX_REG_WRDATA);
> +
>From status EMPTY and FULL, it seems WRDATA is a FIFO. So here also
you should expect messages to be <= fifo depth. And not assume exactly
32bit messages always.

> +
> +static bool xilinx_mbox_last_tx_done(struct mbox_chan *chan)
> +{
> +       struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
> +
> +       /* return false if mailbox is full */
> +       return !xilinx_mbox_full(mbox);
>
Instead of FULL, I think it should check for stricter EMPTY status ...
mbox api submits only 1 message at a time.

> +
> +static bool xilinx_mbox_peek_data(struct mbox_chan *chan)
> +{
> +       struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
> +
> +       return xilinx_mbox_pending(mbox);
> +}
> +
> +static void xilinx_mbox_irq_shutdown(struct mbox_chan *chan)
> +{
> +       struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
> +
> +       /* mask all interrupts */
> +       writel_relaxed(0, mbox->mbox_base + MAILBOX_REG_IE);
> +
> +       clk_disable_unprepare(mbox->clk);
> +
> +       free_irq(mbox->irq, chan);
> +}
> +
> +static void xilinx_mbox_poll_shutdown(struct mbox_chan *chan)
> +{
> +       struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
> +
> +       del_timer_sync(&mbox->rxpoll_timer);
> +
> +       clk_disable_unprepare(mbox->clk);
> +}
> +
> +static const struct mbox_chan_ops xilinx_mbox_irq_ops = {
> +       .send_data = xilinx_mbox_irq_send_data,
> +       .startup = xilinx_mbox_irq_startup,
> +       .shutdown = xilinx_mbox_irq_shutdown,
> +       .last_tx_done = xilinx_mbox_last_tx_done,
> +       .peek_data = xilinx_mbox_peek_data,
> +};
> +
> +static const struct mbox_chan_ops xilinx_mbox_poll_ops = {
> +       .send_data = xilinx_mbox_poll_send_data,
> +       .startup = xilinx_mbox_poll_startup,
> +       .shutdown = xilinx_mbox_poll_shutdown,
> +       .last_tx_done = xilinx_mbox_last_tx_done,
> +       .peek_data = xilinx_mbox_peek_data,
> +};
> +
> +static int xilinx_mbox_probe(struct platform_device *pdev)
> +{
> +       struct xilinx_mbox *mbox;
> +       struct resource *regs;
> +       int ret;
> +
> +       mbox = devm_kzalloc(&pdev->dev, sizeof(*mbox), GFP_KERNEL);
> +       if (!mbox)
> +               return -ENOMEM;
> +
> +       /* get clk and enable */
> +       mbox->clk = devm_clk_get(&pdev->dev, "mbox");
> +       if (IS_ERR(mbox->clk)) {
> +               dev_err(&pdev->dev, "Couldn't get clk.\n");
> +               return PTR_ERR(mbox->clk);
> +       }
> +
> +       regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +       mbox->mbox_base = devm_ioremap_resource(&pdev->dev, regs);
> +       if (IS_ERR(mbox->mbox_base))
> +               return PTR_ERR(mbox->mbox_base);
> +
> +       mbox->irq = platform_get_irq(pdev, 0);
> +       /* if irq is present, we can use it, otherwise, poll */
> +       if (mbox->irq > 0) {
> +               mbox->controller.txdone_irq = true;
> +               mbox->controller.ops = &xilinx_mbox_irq_ops;
> +       } else {
> +               dev_info(&pdev->dev, "IRQ not found, fallback to polling.\n");
> +               mbox->controller.txdone_poll = true;
> +               mbox->controller.txpoll_period = MBOX_POLLING_MS;
> +               mbox->controller.ops = &xilinx_mbox_poll_ops;
> +
> +               setup_timer(&mbox->rxpoll_timer, xilinx_mbox_poll_rx,
> +                           (unsigned long)&mbox->chan);
>
I believe there is indeed some platform that doesn't have RX-Interrupt?
 If no, please remove this.
 If yes, you may want to get some hint from platform about the size of
messages and do mbox_chan_received_data() only upon reading those many
bytes.

Cheers.

[PATCHv7 2/2] mailbox: Adding driver for Xilinx LogiCORE IP mailbox.

[Moritz Fischer]

Hi Jassi,

thanks for your feedback.

On Mon, Aug 10, 2015 at 1:05 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Wed, Jul 15, 2015 at 6:30 AM, Moritz Fischer
> <moritz.fischer@ettus.com> wrote:
>
>> +
>> +static void xilinx_mbox_rx_data(struct mbox_chan *chan)
>> +{
>> +       struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
>> +       u32 data;
>> +
>> +       if (xilinx_mbox_pending(mbox)) {
>> +               data = readl_relaxed(mbox->mbox_base + MAILBOX_REG_RDDATA);
>> +               mbox_chan_received_data(chan, (void *)data);
>>
> If RDDATA is a FIFO, then above seems wrong - you are assuming
> messages are always going to be exactly 32-bits for every protocol.
> Ideally you should empty the fifo fully, at least when RX has an
> interrupt.

>From my understanding it's hard to tell how much actually is in the fifo,
you can tell if it's full for send direction, or empty for read direction.

Maybe the STI / RTI can be setup in a smart way to work with multiple message
sizes.
>
>> +
>> +static int xilinx_mbox_irq_send_data(struct mbox_chan *chan, void *data)
>> +{
>> +       struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
>> +       u32 *udata = data;
>> +
>> +       if (xilinx_mbox_full(mbox))
>> +               return -EBUSY;
>>
> This check is redundant. last_tx_done already makes sure this is always true.

Good to know. I'll fix it.
>
>> +       /* enable interrupt before send */
>> +       xilinx_mbox_tx_intmask(mbox, true);
>> +
>> +       writel_relaxed(*udata, mbox->mbox_base + MAILBOX_REG_WRDATA);
>> +
> From status EMPTY and FULL, it seems WRDATA is a FIFO. So here also
> you should expect messages to be <= fifo depth. And not assume exactly
> 32bit messages always.

How do I determine the message size? Doesn't
drivers/mailbox/bcm2835-mailbox.c or
mailbox-altera.c make the same assumption?

>
>> +
>> +static bool xilinx_mbox_last_tx_done(struct mbox_chan *chan)
>> +{
>> +       struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
>> +
>> +       /* return false if mailbox is full */
>> +       return !xilinx_mbox_full(mbox);
>>
> Instead of FULL, I think it should check for stricter EMPTY status ...
> mbox api submits only 1 message at a time.

The EMPTY status applies to the receive direction only, I could check
the STI status
bit though I suppose.

[...]
>> +
>> +       mbox->irq = platform_get_irq(pdev, 0);
>> +       /* if irq is present, we can use it, otherwise, poll */
>> +       if (mbox->irq > 0) {
>> +               mbox->controller.txdone_irq = true;
>> +               mbox->controller.ops = &xilinx_mbox_irq_ops;
>> +       } else {
>> +               dev_info(&pdev->dev, "IRQ not found, fallback to polling.\n");
>> +               mbox->controller.txdone_poll = true;
>> +               mbox->controller.txpoll_period = MBOX_POLLING_MS;
>> +               mbox->controller.ops = &xilinx_mbox_poll_ops;
>> +
>> +               setup_timer(&mbox->rxpoll_timer, xilinx_mbox_poll_rx,
>> +                           (unsigned long)&mbox->chan);
>>
> I believe there is indeed some platform that doesn't have RX-Interrupt?
>  If no, please remove this.
>  If yes, you may want to get some hint from platform about the size of
> messages and do mbox_chan_received_data() only upon reading those many
> bytes.

I'd be fine to drop the polling use case for the moment, on my
platform I can wire up the IRQ.
We can always add it back in in a follow up use case.

Thanks,

Moritz

[PATCHv7 2/2] mailbox: Adding driver for Xilinx LogiCORE IP mailbox.

[Jassi Brar]

On 2 December 2015 at 22:56, Moritz Fischer <moritz.fischer@ettus.com> wrote:
> Hi Jassi,
>
> thanks for your feedback.
>
> On Mon, Aug 10, 2015 at 1:05 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>> On Wed, Jul 15, 2015 at 6:30 AM, Moritz Fischer
>> <moritz.fischer@ettus.com> wrote:
>>
>>> +
>>> +static void xilinx_mbox_rx_data(struct mbox_chan *chan)
>>> +{
>>> +       struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
>>> +       u32 data;
>>> +
>>> +       if (xilinx_mbox_pending(mbox)) {
>>> +               data = readl_relaxed(mbox->mbox_base + MAILBOX_REG_RDDATA);
>>> +               mbox_chan_received_data(chan, (void *)data);
>>>
>> If RDDATA is a FIFO, then above seems wrong - you are assuming
>> messages are always going to be exactly 32-bits for every protocol.
>> Ideally you should empty the fifo fully, at least when RX has an
>> interrupt.
>
> From my understanding it's hard to tell how much actually is in the fifo,
> you can tell if it's full for send direction, or empty for read direction.
>
Simply read the whole FIFO and leave it to the client driver to parse
that data according to the protocol it drives.

> Maybe the STI / RTI can be setup in a smart way to work with multiple message
> sizes.
>>
>>> +
>>> +static int xilinx_mbox_irq_send_data(struct mbox_chan *chan, void *data)
>>> +{
>>> +       struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
>>> +       u32 *udata = data;
>>> +
>>> +       if (xilinx_mbox_full(mbox))
>>> +               return -EBUSY;
>>>
>> This check is redundant. last_tx_done already makes sure this is always true.
>
> Good to know. I'll fix it.
>>
>>> +       /* enable interrupt before send */
>>> +       xilinx_mbox_tx_intmask(mbox, true);
>>> +
>>> +       writel_relaxed(*udata, mbox->mbox_base + MAILBOX_REG_WRDATA);
>>> +
>> From status EMPTY and FULL, it seems WRDATA is a FIFO. So here also
>> you should expect messages to be <= fifo depth. And not assume exactly
>> 32bit messages always.
>
> How do I determine the message size?
>
Always expect any write request is exactly the size of FIFO depth.

> Doesn't
> drivers/mailbox/bcm2835-mailbox.c or
>
Well I did point it out
http://lists.infradead.org/pipermail/linux-rpi-kernel/2015-June/001902.html
 ... but developer assumes there will _never_ be need to pass a
message bigger than 32-bits. Sadly overlooking the possibility that
some protocol might have simple, say, 64-bits wide commands and
responses that could avoid using any Shared-Memory at all.

> mailbox-altera.c make the same assumption?
>
There are 2 registers, for CMD and PRT each, that make up 1 message.
It doesn't seem like a fifo.

>>
>>> +
>>> +static bool xilinx_mbox_last_tx_done(struct mbox_chan *chan)
>>> +{
>>> +       struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
>>> +
>>> +       /* return false if mailbox is full */
>>> +       return !xilinx_mbox_full(mbox);
>>>
>> Instead of FULL, I think it should check for stricter EMPTY status ...
>> mbox api submits only 1 message at a time.
>
> The EMPTY status applies to the receive direction only, I could check
> the STI status
> bit though I suppose.
>
I don't know the h/w but you get my idea. So whatever is in the next revision.

> [...]
>>> +
>>> +       mbox->irq = platform_get_irq(pdev, 0);
>>> +       /* if irq is present, we can use it, otherwise, poll */
>>> +       if (mbox->irq > 0) {
>>> +               mbox->controller.txdone_irq = true;
>>> +               mbox->controller.ops = &xilinx_mbox_irq_ops;
>>> +       } else {
>>> +               dev_info(&pdev->dev, "IRQ not found, fallback to polling.\n");
>>> +               mbox->controller.txdone_poll = true;
>>> +               mbox->controller.txpoll_period = MBOX_POLLING_MS;
>>> +               mbox->controller.ops = &xilinx_mbox_poll_ops;
>>> +
>>> +               setup_timer(&mbox->rxpoll_timer, xilinx_mbox_poll_rx,
>>> +                           (unsigned long)&mbox->chan);
>>>
>> I believe there is indeed some platform that doesn't have RX-Interrupt?
>>  If no, please remove this.
>>  If yes, you may want to get some hint from platform about the size of
>> messages and do mbox_chan_received_data() only upon reading those many
>> bytes.
>
> I'd be fine to drop the polling use case for the moment, on my
> platform I can wire up the IRQ.
> We can always add it back in in a follow up use case.
>

Thanks.

[PATCHv7 0/2] Adding driver for Xilinx LogiCORE IP mailbox.

[Moritz Fischer]

Hi Jassi,

just a ping to see if you're waiting on me to fix things or if this is
good to go from your point of view,
and you're just waiting some time to give people more time to review.

Let me know if you want me to resend with Soeren's Acked-By

Cheers,

Moritz

On Tue, Jul 14, 2015 at 6:00 PM, Moritz Fischer
<moritz.fischer@ettus.com> wrote:
> Hi all,
>
> Thanks for hanging in there, hopefully the last round of review ...
>
> This patchset adds mailbox framework integration for the Xilinx LogiCORE IP
> mailbox.  The Xilinx LogiCORE IP mailbox is a fpga softcore that allows
> interprocessor communication between AXI4 stream / memory mapped
> processors.
>
> Cheers,
>
> Moritz
>
> Changes from v6:
> - As suggested by Jassi and S?ren use #mbox-cells = <1>
>
> Changes from v5:
> - Fixed constness for ops struct
> - Removed redundant checks
> - Moved ops assignment
> - Removed MODULE_ALIAS
>
> Changes from v4:
> - Have separate mbox_ops structs for polling / irq mode
> - Moved clk handling to startup / shutdown
> - Embedded struct mbox_chan in struct xilinx_mbox
> - Fixed mbox-cells in devicetree documentation
> - Misc stylistic issues
>
> Changes from v3:
> - Stylistic changes
> - Changed reg size in dts to 0x100
>
> Changes from v2:
> - Fixed error condition for IRQ from >= 0 to > 0
> - Fixed clock enable
> - Cleaned up docs
>
> Changes from v1:
> - Added common clock framework support
> - Deal with IRQs that happend before driver load,
>   since HW will not let us know about them when we enable IRQs
>
> Changes from v0:
> - Several stylistic issues
> - Dropped superfluous intr_mode member
> - Really masking the IRQs on mailbox_shutdown
> - No longer using polling by accident in non-IRQ mode
> - Swapped doc and driver commits
>
>
> Moritz Fischer (2):
>   dts: Adding docs for Xilinx LogiCORE IP mailbox driver.
>   mailbox: Adding driver for Xilinx LogiCORE IP mailbox.
>
>  .../devicetree/bindings/mailbox/xilinx-mailbox.txt |  44 +++
>  MAINTAINERS                                        |   7 +
>  drivers/mailbox/Kconfig                            |   7 +
>  drivers/mailbox/Makefile                           |   2 +
>  drivers/mailbox/mailbox-xilinx.c                   | 367 +++++++++++++++++++++
>  5 files changed, 427 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/xilinx-mailbox.txt
>  create mode 100644 drivers/mailbox/mailbox-xilinx.c
>
> --
> 2.4.3
>