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.
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
Happy hacking,
Moritz
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 | 8 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/mailbox-xilinx.c | 375 +++++++++++++++++++++
5 files changed, 436 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mailbox/xilinx-mailbox.txt
create mode 100644 drivers/mailbox/mailbox-xilinx.c
--
2.4.2
Changes from v4:
- Changed mbox cells property from 1 to 0
- Fixed interrupt property
Changes from v3:
- Changed reg size to 0x100
Changes from v2:
- Addressed Michal's stylistic comments
- Fixed typo in compatible string
Changes from v1:
- Added common clock framework support
Changes from v0:
- Fixed example bindings
Signed-off-by: Moritz Fischer <[email protected]>
Acked-by: Michal Simek <[email protected]>
---
.../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..269c026
--- /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@40400000 {
+ compatible = "xlnx,mailbox-2.1";
+ reg = <0x40400000 0x100>;
+ interrupt-parent = <&intc>;
+ interrupts = <0 32 1>;
+ #mbox-cells = <0>;
+ 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@400 {
+ compatible = "client-1.0";
+ reg = <0x400 0x10>;
+ mbox-names = "mbox";
+ mboxes = <&mbox 0>;
+ };
--
2.4.2
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.
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
- Misc stylistic issues
Changes from v3:
- Stylistic
Changes from v2:
- Fixed error handling for IRQ from >= 0 to > 0
- Fixed error handling for clock enabling
- Addressed Michal's stylistic comments
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
Signed-off-by: Moritz Fischer <[email protected]>
Acked-by: Michal Simek <[email protected]>
mailbox: Address some feedback, make ops const.
Signed-off-by: Moritz Fischer <[email protected]>
mailbox: xilinx-mailbox: Addressed more of Josh's feedback.
Signed-off-by: Moritz Fischer <[email protected]>
---
MAINTAINERS | 7 +
drivers/mailbox/Kconfig | 8 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/mailbox-xilinx.c | 375 +++++++++++++++++++++++++++++++++++++++
4 files changed, 392 insertions(+)
create mode 100644 drivers/mailbox/mailbox-xilinx.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 4f1e79b..cb88b4a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10998,6 +10998,13 @@ M: John Linn <[email protected]>
S: Maintained
F: drivers/net/ethernet/xilinx/xilinx_axienet*
+XILINX MAILBOX DRIVER
+M: Moritz Fischer <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/mailbox/mailbox-xilinx.c
+F: Documentation/devicetree/bindings/mailbox/xilinx-mailbox.txt
+
XILINX UARTLITE SERIAL DRIVER
M: Peter Korsgaard <[email protected]>
L: [email protected]
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 84b0a2d..e11e4b2 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -60,4 +60,12 @@ config ALTERA_MBOX
An implementation of the Altera Mailbox soft core. It is used
to send message between processors. Say Y here if you want to use the
Altera mailbox support.
+
+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 b18201e..d28a028 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -11,3 +11,5 @@ obj-$(CONFIG_OMAP2PLUS_MBOX) += omap-mailbox.o
obj-$(CONFIG_PCC) += pcc.o
obj-$(CONFIG_ALTERA_MBOX) += mailbox-altera.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..9d6364c
--- /dev/null
+++ b/drivers/mailbox/mailbox-xilinx.c
@@ -0,0 +1,375 @@
+/*
+ * 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);
+
+ /* prep and enable the clock */
+ clk_prepare_enable(mbox->clk);
+
+ /* setup polling timer */
+ setup_timer(&mbox->rxpoll_timer, xilinx_mbox_poll_rx,
+ (unsigned long)chan);
+ 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 (!mbox || !data)
+ return -EINVAL;
+
+ 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 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 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;
+ } 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->dev = &pdev->dev;
+
+ /* hardware supports only one channel. */
+ mbox->controller.dev = mbox->dev;
+ mbox->controller.num_chans = 1;
+ mbox->controller.chans = &mbox->chan;
+
+ if (mbox->irq > 0)
+ mbox->controller.ops = &xilinx_mbox_irq_ops;
+ else
+ mbox->controller.ops = &xilinx_mbox_poll_ops;
+
+ 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 <[email protected]>");
+MODULE_ALIAS("platform:xilinx-mailbox");
--
2.4.2
On Tue, 2015-06-23 at 11:00AM -0700, Moritz Fischer wrote:
> Changes from v4:
> - Changed mbox cells property from 1 to 0
> - Fixed interrupt property
>
> Changes from v3:
> - Changed reg size to 0x100
>
> Changes from v2:
> - Addressed Michal's stylistic comments
> - Fixed typo in compatible string
>
> Changes from v1:
> - Added common clock framework support
>
> Changes from v0:
> - Fixed example bindings
>
> Signed-off-by: Moritz Fischer <[email protected]>
> Acked-by: Michal Simek <[email protected]>
> ---
> .../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..269c026
> --- /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
I guess this should say "should be 0" now?
Sören
On Tue, Jun 23, 2015 at 11:05 AM, Sören Brinkmann
<[email protected]> wrote:
> On Tue, 2015-06-23 at 11:00AM -0700, Moritz Fischer wrote:
>> Changes from v4:
>> - Changed mbox cells property from 1 to 0
>> - Fixed interrupt property
>>
>> Changes from v3:
>> - Changed reg size to 0x100
>>
>> Changes from v2:
>> - Addressed Michal's stylistic comments
>> - Fixed typo in compatible string
>>
>> Changes from v1:
>> - Added common clock framework support
>>
>> Changes from v0:
>> - Fixed example bindings
>>
>> Signed-off-by: Moritz Fischer <[email protected]>
>> Acked-by: Michal Simek <[email protected]>
>> ---
>> .../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..269c026
>> --- /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
>
> I guess this should say "should be 0" now?
D'Oh, yeah ...
Moritz
Hey Moritz-
Just a couple more nits, nothing big. Looks pretty clean!
On Tue, Jun 23, 2015 at 11:00:02AM -0700, Moritz Fischer wrote:
> 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.
>
> 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
> - Misc stylistic issues
>
> Changes from v3:
> - Stylistic
>
> Changes from v2:
> - Fixed error handling for IRQ from >= 0 to > 0
> - Fixed error handling for clock enabling
> - Addressed Michal's stylistic comments
>
> 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
>
> Signed-off-by: Moritz Fischer <[email protected]>
> Acked-by: Michal Simek <[email protected]>
>
> mailbox: Address some feedback, make ops const.
>
> Signed-off-by: Moritz Fischer <[email protected]>
>
> mailbox: xilinx-mailbox: Addressed more of Josh's feedback.
>
> Signed-off-by: Moritz Fischer <[email protected]>
Did you intend for this intermediate history to exist? Seems verbose,
doesn't provide any meaningful detail?
[..]
> +++ b/drivers/mailbox/mailbox-xilinx.c
[..]
> +static int xilinx_mbox_poll_startup(struct mbox_chan *chan)
> +{
> + struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan);
> +
> + /* prep and enable the clock */
Nit: Is this comment conveying anything useful?
> + clk_prepare_enable(mbox->clk);
> +
> + /* setup polling timer */
Nit: Same here.
> + setup_timer(&mbox->rxpoll_timer, xilinx_mbox_poll_rx,
> + (unsigned long)chan);
setup_timer() could conceivably be done in probe().
[..]
> +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)
Nit: How could this happen?
> + return -EINVAL;
> +
> + if (xilinx_mbox_full(mbox))
> + return -EBUSY;
> +
> + writel_relaxed(*udata, mbox->mbox_base + MAILBOX_REG_WRDATA);
> +
> + return 0;
> +}
[..]
> +static struct mbox_chan_ops xilinx_mbox_irq_ops = {
const?
> + .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 struct mbox_chan_ops xilinx_mbox_poll_ops = {
const?
> + .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,
> +};
Splitting up the ops seemed to clean things up quite a bit!
> +
> +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;
> + } 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->dev = &pdev->dev;
> +
> + /* hardware supports only one channel. */
> + mbox->controller.dev = mbox->dev;
> + mbox->controller.num_chans = 1;
> + mbox->controller.chans = &mbox->chan;
> +
> + if (mbox->irq > 0)
> + mbox->controller.ops = &xilinx_mbox_irq_ops;
> + else
> + mbox->controller.ops = &xilinx_mbox_poll_ops;
Nit: you're already checking this above, seems like you can just move
the .ops assignment there.
Josh
On 06/23/2015 08:00 PM, Moritz Fischer wrote:
> 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.
>
> 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
> - Misc stylistic issues
>
> Changes from v3:
> - Stylistic
>
> Changes from v2:
> - Fixed error handling for IRQ from >= 0 to > 0
> - Fixed error handling for clock enabling
> - Addressed Michal's stylistic comments
>
> 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
BTW: These change logs shouldn't be the part of commit.
Thanks,
Michal
On Tue, 2015-06-23 at 11:00 -0700, Moritz Fischer wrote:
> +MODULE_ALIAS("platform:xilinx-mailbox");
So I think this MODULE_ALIAS() is only useful if, in short, there's a
corresponding platform_device created. Ie, a platform_device with a
name "xilinx-mailbox" that will fire of a "MODALIAS=platform:xilinx
-mailbox" when it's created.
I couldn't spot such a platform_device. Provided git grep didn't let me
down here: what breaks if this line is dropped?
Thanks,
Paul Bolle
On 06/24/2015 10:36 PM, Paul Bolle wrote:
> On Tue, 2015-06-23 at 11:00 -0700, Moritz Fischer wrote:
>> +MODULE_ALIAS("platform:xilinx-mailbox");
>
> So I think this MODULE_ALIAS() is only useful if, in short, there's a
> corresponding platform_device created. Ie, a platform_device with a
> name "xilinx-mailbox" that will fire of a "MODALIAS=platform:xilinx
> -mailbox" when it's created.
>
> I couldn't spot such a platform_device. Provided git grep didn't let me
> down here: what breaks if this line is dropped?
IRC you don't need to have this platform_device in the kernel present.
Only one thing which should be check is that this driver can be used as
platform device driver.
The only one problematic part is devm_clk_get() and this should be
checked if you can use this as platform driver. From the first look it
looks like that this will break it.
Anyway if Moritz is able to use this a platform driver he can keep this
line there. If not, it should be removed.
Thanks,
Michal
On Thu, 2015-06-25 at 08:55 +0200, Michal Simek wrote:
> On 06/24/2015 10:36 PM, Paul Bolle wrote:
> > On Tue, 2015-06-23 at 11:00 -0700, Moritz Fischer wrote:
> > > +MODULE_ALIAS("platform:xilinx-mailbox");
> >
> > So I think this MODULE_ALIAS() is only useful if, in short, there's
> > a corresponding platform_device created. Ie, a platform_device with
> > a name "xilinx-mailbox" that will fire of a "MODALIAS=platform:xili
> > nx-mailbox" when it's created.
> >
> > I couldn't spot such a platform_device. Provided git grep didn't
> > let me down here: what breaks if this line is dropped?
>
> IRC you don't need to have this platform_device in the kernel
> present. Only one thing which should be check is that this driver can
> be used as platform device driver.
>
> The only one problematic part is devm_clk_get() and this should be
> checked if you can use this as platform driver. From the first look
> it looks like that this will break it.
>
> Anyway if Moritz is able to use this a platform driver he can keep
> this line there. If not, it should be removed.
But, assuming this works as a platform driver, where does the "xilinx
-mailbox" platform device originate?
Paul Bolle
PS Evolution 3.16 is nearly unbearable in its handling of replies to
plain text messages. Doe anyone know how to make it handle them
sensibly?
On 06/25/2015 09:31 AM, Paul Bolle wrote:
> On Thu, 2015-06-25 at 08:55 +0200, Michal Simek wrote:
>> On 06/24/2015 10:36 PM, Paul Bolle wrote:
>>> On Tue, 2015-06-23 at 11:00 -0700, Moritz Fischer wrote:
>>>> +MODULE_ALIAS("platform:xilinx-mailbox");
>>>
>>> So I think this MODULE_ALIAS() is only useful if, in short, there's
>>> a corresponding platform_device created. Ie, a platform_device with
>>> a name "xilinx-mailbox" that will fire of a "MODALIAS=platform:xili
>>> nx-mailbox" when it's created.
>>>
>>> I couldn't spot such a platform_device. Provided git grep didn't
>>> let me down here: what breaks if this line is dropped?
>>
>> IRC you don't need to have this platform_device in the kernel
>> present. Only one thing which should be check is that this driver can
>> be used as platform device driver.
>>
>> The only one problematic part is devm_clk_get() and this should be
>> checked if you can use this as platform driver. From the first look
>> it looks like that this will break it.
>>
>> Anyway if Moritz is able to use this a platform driver he can keep
>> this line there. If not, it should be removed.
>
> But, assuming this works as a platform driver, where does the "xilinx
> -mailbox" platform device originate?
It has to be platform_device somewhere for sure.
In past we had folder in arch/microblaze/platform folder.
Currently you can add this code to for example
arch/microblaze/kernel/platform.c
But as I said I don't think it is really important. There will be a lot
of others drivers in the kernel which can be used as platform drivers
but you are not able to find out platform_device for it.
The important part is that driver can work as is.
Also it is quite common that users create own BSP for their custom
boards but they don't push it to mainline.
Thanks,
Michal
On Thu, 2015-06-25 at 09:47 +0200, Michal Simek wrote:
> It has to be platform_device somewhere for sure.
> In past we had folder in arch/microblaze/platform folder.
> Currently you can add this code to for example
> arch/microblaze/kernel/platform.c
>
> But as I said I don't think it is really important. There will be a
> lot
> of others drivers in the kernel which can be used as platform drivers
> but you are not able to find out platform_device for it.
Because, like probably happens with this driver, the OF infrastructure
makes sure the .probe and .remove functions will eventually called?
> The important part is that driver can work as is.
Sure.
But I was talking about the MODULE_ALIAS() macro. Trivial as it is,
since I still don't see where a "MODALIAS=platform:xilinx-mailbox"
uevent could come from I still don't see the point of this line. As I
asked in my first message: what breaks if this line is dropped?
> Also it is quite common that users create own BSP for their custom
> boards but they don't push it to mainline.
I can't recall what BSP means. Anyhow, why should we care about boards
not pushed into mainline?
Paul Bolle
On 06/25/2015 11:35 AM, Paul Bolle wrote:
> On Thu, 2015-06-25 at 09:47 +0200, Michal Simek wrote:
>> It has to be platform_device somewhere for sure.
>> In past we had folder in arch/microblaze/platform folder.
>> Currently you can add this code to for example
>> arch/microblaze/kernel/platform.c
>>
>> But as I said I don't think it is really important. There will be a
>> lot
>> of others drivers in the kernel which can be used as platform drivers
>> but you are not able to find out platform_device for it.
>
> Because, like probably happens with this driver, the OF infrastructure
> makes sure the .probe and .remove functions will eventually called?
>
>> The important part is that driver can work as is.
>
> Sure.
>
> But I was talking about the MODULE_ALIAS() macro. Trivial as it is,
> since I still don't see where a "MODALIAS=platform:xilinx-mailbox"
> uevent could come from I still don't see the point of this line. As I
> asked in my first message: what breaks if this line is dropped?
TBH probably nothing because all will just use OF because it is just
common for a long time on all xilinx platforms.
>
>> Also it is quite common that users create own BSP for their custom
>> boards but they don't push it to mainline.
>
> I can't recall what BSP means. Anyhow, why should we care about boards
> not pushed into mainline?
I am not the right person to answer this in general. For SoC tree I
maintain I tend to accept these patches if they are useful and
definitely don't want to have support for various platforms. But having
shared drivers is fine.
For this case if Moritz tested OF and platform device probes and both
work I am fine to keep it there. If only OF part is tested then I would
remove this line.
Thanks,
Michal