ST's platforms currently support a maximum of 5 Mailboxes, one for
each of the supported co-processors situated on the platform. Each
Mailbox is divided up into 4 instances which consist of 32 channels.
Messages are passed between the application and co-processors using
shared memory areas.
Also included in the set is an example Client which should be generic
enough to use as a testing environment for all Mailbox IPs which use
shared memory to pass messages. With a small change, it should also
be able to test more extravagant implementations too.
Lee Jones (6):
mailbox: dt: Supply bindings for ST's Mailbox IP
mailbox: dt-bindings: Add shared [driver <=> device tree] defines
mailbox: Add support for ST's Mailbox IP
ARM: STi: stih407-family: Add nodes for Mailbox
mailbox: Add generic mechanism for testing Mailbox Controllers
ARM: STi: DT: STiH407: Enable Mailbox testing facility
.../devicetree/bindings/mailbox/sti-mailbox.txt | 53 ++
arch/arm/boot/dts/stih407-family.dtsi | 40 ++
drivers/mailbox/Kconfig | 14 +
drivers/mailbox/Makefile | 4 +
drivers/mailbox/mailbox-sti.c | 562 +++++++++++++++++++++
drivers/mailbox/mailbox-test.c | 210 ++++++++
include/dt-bindings/mailbox/mailbox.h | 14 +
7 files changed, 897 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mailbox/sti-mailbox.txt
create mode 100644 drivers/mailbox/mailbox-sti.c
create mode 100644 drivers/mailbox/mailbox-test.c
create mode 100644 include/dt-bindings/mailbox/mailbox.h
--
1.9.1
Signed-off-by: Lee Jones <[email protected]>
---
.../devicetree/bindings/mailbox/sti-mailbox.txt | 53 ++++++++++++++++++++++
1 file changed, 53 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mailbox/sti-mailbox.txt
diff --git a/Documentation/devicetree/bindings/mailbox/sti-mailbox.txt b/Documentation/devicetree/bindings/mailbox/sti-mailbox.txt
new file mode 100644
index 0000000..3390e3e
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/sti-mailbox.txt
@@ -0,0 +1,53 @@
+ST Microelectronics Mailbox Driver
+
+Each ST Mailbox IP currently consists of 4 instances of 32 channels. Messages
+are passed between Application and Remote processors using shared memory.
+
+Controller
+----------
+
+Required properties:
+- compatible : Should be "st,stih407-mailbox"
+- reg : Offset and length of the device's register set
+- mbox-name : Name of the mailbox
+- #mbox-cells: : Must be 3
+ <&phandle instance channel direction>
+ phandle : Label name of controller
+ instance : Instance number
+ channel : Channel number
+ direction : Data flow direction; MBOX_TX or MBOX_RX
+
+Optional properties
+- interrupts : Contains the IRQ line for a Rx mailbox
+ [NB: The absence of this property denotes Tx only]
+
+Example:
+
+mailbox0: mailbox@0 {
+ compatible = "st,stih407-mailbox";
+ reg = <0x8f00000 0x1000>;
+ interrupts = <GIC_SPI 1 IRQ_TYPE_NONE>;
+ #mbox-cells = <3>;
+ mbox-name = "a9";
+};
+
+Client
+------
+
+Required properties:
+- compatible : Many (See the client docs)
+- reg : Shared (between Application and Remote) memory address
+- mboxes : Standard property to specify a Mailbox (See ./mailbox.txt)
+ Cells must match 'mbox-cells' (See Controller docs above)
+
+Optional properties
+- mbox-names : Name given to channels seen in the 'mboxes' property.
+
+Example:
+
+mailbox_test {
+ compatible = "mailbox_test";
+ reg = <0x[shared_memory_address], [shared_memory_size]>;
+ mboxes = <&mailbox2 0 1 MBOX_TX>, <&mailbox0 2 1 MBOX_RX>;
+ mbox-names = "tx", "rx";
+};
--
1.9.1
This header is currently only used for defines pertaining to data
direction i.e. Rx, Tx or Loopback.
Signed-off-by: Lee Jones <[email protected]>
---
include/dt-bindings/mailbox/mailbox.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)
create mode 100644 include/dt-bindings/mailbox/mailbox.h
diff --git a/include/dt-bindings/mailbox/mailbox.h b/include/dt-bindings/mailbox/mailbox.h
new file mode 100644
index 0000000..82e929a
--- /dev/null
+++ b/include/dt-bindings/mailbox/mailbox.h
@@ -0,0 +1,14 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __MAILBOX_CONTROLLER_DT_BINDINGS_H
+#define __MAILBOX_CONTROLLER_DT_BINDINGS_H
+
+#define MBOX_TX 0x1
+#define MBOX_RX 0x2
+#define MBOX_LOOPBACK (MBOX_RX | MBOX_TX)
+
+#endif /* __MAILBOX_CONTROLLER_DT_BINDINGS_H */
--
1.9.1
ST's platforms currently support a maximum of 5 Mailboxes, one for
each of the supported co-processors situated on the platform. Each
Mailbox is divided up into 4 instances which consist of 32 channels.
Messages are passed between the application and co-processors using
shared memory areas. It is the Client's responsibility to manage
these areas.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/mailbox/Kconfig | 7 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/mailbox-sti.c | 562 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 571 insertions(+)
create mode 100644 drivers/mailbox/mailbox-sti.c
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index e269f08..2cc4c39 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 STI_MBOX
+ tristate "STI Mailbox framework support"
+ depends on ARCH_STI && OF
+ help
+ Mailbox implementation for STMicroelectonics family chips with
+ hardware for interprocessor communication.
+
endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 8e6d822..7cb4766 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_STI_MBOX) += mailbox-sti.o
diff --git a/drivers/mailbox/mailbox-sti.c b/drivers/mailbox/mailbox-sti.c
new file mode 100644
index 0000000..0c82f7b
--- /dev/null
+++ b/drivers/mailbox/mailbox-sti.c
@@ -0,0 +1,562 @@
+/*
+ * STi Mailbox
+ *
+ * Copyright (C) 2015 ST Microelectronics
+ *
+ * Author: Lee Jones <[email protected]> for ST Microelectronics
+ *
+ * Based on the original driver written by;
+ * Alexandre Torgue, Olivier Lebreton and Loic Pallardy
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <dt-bindings/mailbox/mailbox.h>
+
+#include "mailbox.h"
+
+#define STI_MBOX_INST_MAX 4 /* RAM saving: Max supported instances */
+#define STI_MBOX_CHAN_MAX 20 /* RAM saving: Max supported channels */
+
+static DEFINE_SPINLOCK(sti_mbox_chan_lock);
+
+/**
+ * STi Mailbox device data
+ *
+ * An IP Mailbox is currently composed of 4 instances
+ * Each instance is currently composed of 32 channels
+ * This means that we have 128 channels per Mailbox
+ * A channel an be used for TX or RX
+ *
+ * @dev: Device to which it is attached
+ * @mbox: Representation of a communication channel controller
+ * @base: Base address of the register mapping region
+ * @name: Name of the mailbox
+ * @enabled: Local copy of enabled channels
+ */
+struct sti_mbox_device {
+ struct device *dev;
+ struct mbox_controller *mbox;
+ void __iomem *base;
+ const char *name;
+ u32 enabled[STI_MBOX_INST_MAX];
+};
+
+/**
+ * STi Mailbox platform specfic configuration
+ *
+ * @num_inst: Maximum number of instances in one HW Mailbox
+ * @num_chan: Maximum number of channel per instance
+ * @irq_val: Register offset to read interrupt status
+ * @irq_set: Register offset to generate a Tx channel interrupt
+ * @irq_clr: Register offset to clear pending Rx interrupts
+ * @ena_val: Register offset to read enable status
+ * @ena_set: Register offset to enable a channel
+ * @ena_clr: Register offset to disable a channel
+ */
+struct sti_mbox_pdata {
+ unsigned int num_inst;
+ unsigned int num_chan;
+ unsigned int irq_val;
+ unsigned int irq_set;
+ unsigned int irq_clr;
+ unsigned int ena_val;
+ unsigned int ena_set;
+ unsigned int ena_clr;
+};
+
+/**
+ * STi Mailbox allocated channel information
+ *
+ * @mdev: Pointer to parent Mailbox device
+ * @instance: Instance number channel resides in
+ * @channel: Channel number pertaining to this container
+ * @direction: Direction which data will travel in through the channel (Tx/Rx)
+ */
+struct sti_channel {
+ struct sti_mbox_device *mdev;
+ unsigned int instance;
+ unsigned int channel;
+ unsigned int direction;
+};
+
+static inline bool sti_mbox_channel_is_enabled(struct mbox_chan *chan)
+{
+ struct sti_channel *chan_info = chan->con_priv;
+ struct sti_mbox_device *mdev = chan_info->mdev;
+ unsigned int instance = chan_info->instance;
+ unsigned int channel = chan_info->channel;
+
+ return mdev->enabled[instance] & BIT(channel);
+}
+
+static inline
+struct mbox_chan *sti_mbox_to_channel(struct mbox_controller *mbox,
+ unsigned int instance,
+ unsigned int channel)
+{
+ struct sti_channel *chan_info;
+ int i;
+
+ for (i = 0; i < mbox->num_chans; i++) {
+ chan_info = mbox->chans[i].con_priv;
+ if (chan_info &&
+ chan_info->instance == instance &&
+ chan_info->channel == channel)
+ return &mbox->chans[i];
+ }
+
+ dev_err(mbox->dev,
+ "Channel not registered: instance: %d channel: %d\n",
+ instance, channel);
+
+ return NULL;
+}
+
+static void sti_mbox_enable_channel(struct mbox_chan *chan)
+{
+ struct sti_channel *chan_info = chan->con_priv;
+ struct sti_mbox_device *mdev = chan_info->mdev;
+ struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
+ unsigned int instance = chan_info->instance;
+ unsigned int channel = chan_info->channel;
+ unsigned long flags;
+ void __iomem *base;
+
+ base = mdev->base + (instance * sizeof(u32));
+
+ spin_lock_irqsave(&sti_mbox_chan_lock, flags);
+ mdev->enabled[instance] |= BIT(channel);
+ writel_relaxed(BIT(channel), base + pdata->ena_set);
+ spin_unlock_irqrestore(&sti_mbox_chan_lock, flags);
+}
+
+static void sti_mbox_disable_channel(struct mbox_chan *chan)
+{
+ struct sti_channel *chan_info = chan->con_priv;
+ struct sti_mbox_device *mdev = chan_info->mdev;
+ struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
+ unsigned int instance = chan_info->instance;
+ unsigned int channel = chan_info->channel;
+ unsigned long flags;
+ void __iomem *base;
+
+ base = mdev->base + (instance * sizeof(u32));
+
+ spin_lock_irqsave(&sti_mbox_chan_lock, flags);
+ mdev->enabled[instance] &= ~BIT(channel);
+ writel_relaxed(BIT(channel), base + pdata->ena_clr);
+ spin_unlock_irqrestore(&sti_mbox_chan_lock, flags);
+}
+
+static void sti_mbox_clear_irq(struct mbox_chan *chan)
+{
+ struct sti_channel *chan_info = chan->con_priv;
+ struct sti_mbox_device *mdev = chan_info->mdev;
+ struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
+ unsigned int instance = chan_info->instance;
+ unsigned int channel = chan_info->channel;
+ void __iomem *base;
+
+ base = mdev->base + (instance * sizeof(u32));
+
+ writel_relaxed(BIT(channel), base + pdata->irq_clr);
+}
+
+static struct mbox_chan *sti_mbox_irq_to_channel(struct sti_mbox_device *mdev,
+ unsigned int instance)
+{
+ struct mbox_controller *mbox = mdev->mbox;
+ struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
+ struct mbox_chan *chan = NULL;
+ unsigned int channel;
+ unsigned long bits;
+ void __iomem *base;
+
+ base = mdev->base + (instance * sizeof(u32));
+
+ bits = readl_relaxed(base + pdata->irq_val);
+ if (!bits)
+ /* No IRQs fired in specified instance */
+ return NULL;
+
+ /* An IRQ has fired, find the associated channel */
+ for (channel = 0; bits; channel++) {
+ if (!test_and_clear_bit(channel, &bits))
+ continue;
+
+ chan = sti_mbox_to_channel(mbox, instance, channel);
+ if (chan) {
+ dev_dbg(mbox->dev,
+ "IRQ fired on instance: %d channel: %d\n",
+ instance, channel);
+ break;
+ }
+ }
+
+ return chan;
+}
+
+static irqreturn_t sti_mbox_thread_handler(int irq, void *data)
+{
+ struct sti_mbox_device *mdev = data;
+ struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
+ struct mbox_chan *chan;
+ unsigned int instance;
+
+ for (instance = 0; instance < pdata->num_inst; instance++) {
+keep_looking:
+ chan = sti_mbox_irq_to_channel(mdev, instance);
+ if (!chan)
+ continue;
+
+ mbox_chan_received_data(chan, NULL);
+ sti_mbox_clear_irq(chan);
+ sti_mbox_enable_channel(chan);
+ goto keep_looking;
+ }
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t sti_mbox_irq_handler(int irq, void *data)
+{
+ struct sti_mbox_device *mdev = data;
+ struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
+ struct sti_channel *chan_info;
+ struct mbox_chan *chan;
+ unsigned int instance;
+ int ret = IRQ_NONE;
+
+ for (instance = 0; instance < pdata->num_inst; instance++) {
+ chan = sti_mbox_irq_to_channel(mdev, instance);
+ if (!chan)
+ continue;
+ chan_info = chan->con_priv;
+
+ if (!sti_mbox_channel_is_enabled(chan)) {
+ dev_warn(mdev->dev,
+ "Unexpected IRQ: %s\n"
+ " instance: %d: channel: %d [enabled: %x]\n",
+ mdev->name, chan_info->instance,
+ chan_info->channel, mdev->enabled[instance]);
+ ret = IRQ_HANDLED;
+ continue;
+ }
+
+ sti_mbox_disable_channel(chan);
+ ret = IRQ_WAKE_THREAD;
+ }
+
+ if (ret == IRQ_NONE)
+ dev_err(mdev->dev, "Spurious IRQ - was a channel requested?\n");
+
+ return ret;
+}
+
+static bool sti_mbox_tx_is_ready(struct mbox_chan *chan)
+{
+ struct sti_channel *chan_info = chan->con_priv;
+ struct sti_mbox_device *mdev = chan_info->mdev;
+ struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
+ unsigned int instance = chan_info->instance;
+ unsigned int channel = chan_info->channel;
+ void __iomem *base;
+
+ base = mdev->base + (instance * sizeof(u32));
+
+ if (!(chan_info->direction & MBOX_TX))
+ return false;
+
+ if (!(readl_relaxed(base + pdata->ena_val) & BIT(channel))) {
+ dev_dbg(mdev->dev, "Mbox: %s: inst: %d, chan: %d disabled\n",
+ mdev->name, instance, channel);
+ return false;
+ }
+
+ if (readl_relaxed(base + pdata->irq_val) & BIT(channel)) {
+ dev_dbg(mdev->dev, "Mbox: %s: inst: %d, chan: %d not ready\n",
+ mdev->name, instance, channel);
+ return false;
+ }
+
+ return true;
+}
+
+static int sti_mbox_send_data(struct mbox_chan *chan, void *data)
+{
+ struct sti_channel *chan_info = chan->con_priv;
+ struct sti_mbox_device *mdev = chan_info->mdev;
+ struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
+ unsigned int instance = chan_info->instance;
+ unsigned int channel = chan_info->channel;
+ void __iomem *base;
+
+ if (!sti_mbox_tx_is_ready(chan))
+ return -EBUSY;
+
+ base = mdev->base + (instance * sizeof(u32));
+
+ /* Send event to co-processor */
+ writel_relaxed(BIT(channel), base + pdata->irq_set);
+
+ dev_dbg(mdev->dev,
+ "Sent via Mailbox %s: instance: %d channel: %d\n",
+ mdev->name, instance, channel);
+
+ return 0;
+}
+
+static int sti_mbox_startup_chan(struct mbox_chan *chan)
+{
+ sti_mbox_clear_irq(chan);
+ sti_mbox_enable_channel(chan);
+
+ return 0;
+}
+
+static void sti_mbox_shutdown_chan(struct mbox_chan *chan)
+{
+ struct sti_channel *chan_info = chan->con_priv;
+ struct mbox_controller *mbox = chan_info->mdev->mbox;
+ int i;
+
+ for (i = 0; i < mbox->num_chans; i++)
+ if (chan == &mbox->chans[i])
+ break;
+
+ if (mbox->num_chans == i) {
+ dev_warn(mbox->dev, "Request to free non-existent channel\n");
+ return;
+ }
+
+ sti_mbox_disable_channel(chan);
+ sti_mbox_clear_irq(chan);
+
+ /* Reset channel */
+ memset(chan, 0, sizeof(*chan));
+ chan->mbox = mbox;
+ chan->txdone_method = TXDONE_BY_POLL;
+ spin_lock_init(&chan->lock);
+}
+
+static struct mbox_chan *sti_mbox_xlate(struct mbox_controller *mbox,
+ const struct of_phandle_args *spec)
+{
+ struct sti_mbox_device *mdev = dev_get_drvdata(mbox->dev);
+ struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
+ struct sti_channel *chan_info;
+ struct mbox_chan *chan = NULL;
+ unsigned int instance = spec->args[0];
+ unsigned int channel = spec->args[1];
+ unsigned int direction = spec->args[2];
+ int i;
+
+ /* Bounds checking */
+ if (instance >= pdata->num_inst || channel >= pdata->num_chan) {
+ dev_err(mbox->dev,
+ "Invalid channel requested instance: %d channel: %d\n",
+ instance, channel);
+ return NULL;
+ }
+
+ for (i = 0; i < mbox->num_chans; i++) {
+ chan_info = mbox->chans[i].con_priv;
+
+ /* Is requested channel free? */
+ if (direction != MBOX_LOOPBACK &&
+ chan_info &&
+ mbox->dev == chan_info->mdev->dev &&
+ instance == chan_info->instance &&
+ channel == chan_info->channel) {
+ dev_err(mbox->dev, "Channel in use\n");
+ return NULL;
+ }
+
+ /* Find the first free slot */
+ if (!chan && !chan_info)
+ chan = &mbox->chans[i];
+ }
+
+ if (!chan) {
+ dev_err(mbox->dev, "No free channels left\n");
+ return NULL;
+ }
+
+ chan_info = devm_kzalloc(mbox->dev, sizeof(*chan_info), GFP_KERNEL);
+ if (!chan_info)
+ return NULL;
+
+ chan_info->mdev = mdev;
+ chan_info->instance = instance;
+ chan_info->channel = channel;
+ chan_info->direction = direction;
+
+ chan->con_priv = chan_info;
+
+ dev_info(mbox->dev,
+ "Mbox: %s: Created channel:\n"
+ " instance: %d channel: %d direction: %s\n",
+ mdev->name, instance, channel,
+ direction == MBOX_LOOPBACK ? "Loopback" :
+ direction == MBOX_TX ? "Tx" : "Rx");
+
+ return chan;
+}
+
+static struct mbox_chan_ops sti_mbox_ops = {
+ .startup = sti_mbox_startup_chan,
+ .shutdown = sti_mbox_shutdown_chan,
+ .send_data = sti_mbox_send_data,
+ .last_tx_done = sti_mbox_tx_is_ready,
+};
+
+static const struct sti_mbox_pdata mbox_stih407_pdata = {
+ .num_inst = 4,
+ .num_chan = 32,
+ .irq_val = 0x04,
+ .irq_set = 0x24,
+ .irq_clr = 0x44,
+ .ena_val = 0x64,
+ .ena_set = 0x84,
+ .ena_clr = 0xa4,
+};
+
+static const struct of_device_id sti_mailbox_match[] = {
+ {
+ .compatible = "st,stih407-mailbox",
+ .data = (void *)&mbox_stih407_pdata
+ },
+ { }
+};
+
+static int sti_mbox_probe(struct platform_device *pdev)
+{
+ const struct of_device_id *match;
+ struct mbox_controller *mbox;
+ struct sti_mbox_device *mdev;
+ struct device_node *np = pdev->dev.of_node;
+ struct mbox_chan *chans;
+ struct resource *res;
+ int irq;
+ int ret;
+
+ match = of_match_device(sti_mailbox_match, &pdev->dev);
+ if (!match) {
+ dev_err(&pdev->dev, "No configuration found\n");
+ return -ENODEV;
+ }
+ pdev->dev.platform_data = (struct sti_mbox_pdata *) match->data;
+
+ mdev = devm_kzalloc(&pdev->dev, sizeof(*mdev), GFP_KERNEL);
+ if (!mdev)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, mdev);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ mdev->base = devm_ioremap_resource(&pdev->dev, res);
+ if (!mdev->base)
+ return -ENOMEM;
+
+ ret = of_property_read_string(np, "mbox-name", &mdev->name);
+ if (ret)
+ mdev->name = np->full_name;
+
+ mbox = devm_kzalloc(&pdev->dev, sizeof(*mbox), GFP_KERNEL);
+ if (!mbox)
+ return -ENOMEM;
+
+ chans = devm_kzalloc(&pdev->dev,
+ sizeof(*chans) * STI_MBOX_CHAN_MAX, GFP_KERNEL);
+ if (!chans)
+ return -ENOMEM;
+
+ mdev->dev = &pdev->dev;
+ mdev->mbox = mbox;
+
+ /* STi Mailbox does not have a Tx-Done or Tx-Ready IRQ */
+ mbox->txdone_irq = false;
+ mbox->txdone_poll = true;
+ mbox->txpoll_period = 100;
+ mbox->ops = &sti_mbox_ops;
+ mbox->dev = mdev->dev;
+ mbox->of_xlate = sti_mbox_xlate;
+ mbox->chans = chans;
+ mbox->num_chans = STI_MBOX_CHAN_MAX;
+
+ ret = mbox_controller_register(mbox);
+ if (ret)
+ return ret;
+
+ /* It's okay for Tx Mailboxes to not supply IRQs */
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ dev_info(&pdev->dev,
+ "%s: Registered Tx only Mailbox\n", mdev->name);
+ return 0;
+ }
+
+ ret = devm_request_threaded_irq(&pdev->dev, irq,
+ sti_mbox_irq_handler,
+ sti_mbox_thread_handler,
+ IRQF_ONESHOT, mdev->name, mdev);
+ if (ret) {
+ dev_err(&pdev->dev, "Can't claim IRQ %d\n", irq);
+ mbox_controller_unregister(mbox);
+ return -EINVAL;
+ }
+
+ dev_info(&pdev->dev, "%s: Registered Tx/Rx Mailbox\n", mdev->name);
+
+ return 0;
+}
+
+static int sti_mbox_remove(struct platform_device *pdev)
+{
+ struct sti_mbox_device *mdev = platform_get_drvdata(pdev);
+
+ mbox_controller_unregister(mdev->mbox);
+
+ return 0;
+}
+
+static struct platform_driver sti_mbox_driver = {
+ .probe = sti_mbox_probe,
+ .remove = sti_mbox_remove,
+ .driver = {
+ .name = "sti-mailbox",
+ .of_match_table = sti_mailbox_match,
+ },
+};
+
+static int __init sti_mbox_init(void)
+{
+ return platform_driver_register(&sti_mbox_driver);
+}
+
+static void __exit sti_mbox_exit(void)
+{
+ platform_driver_unregister(&sti_mbox_driver);
+}
+
+postcore_initcall(sti_mbox_init);
+module_exit(sti_mbox_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("STMicroelectronics Mailbox Controller");
+MODULE_AUTHOR("Lee Jones <[email protected]");
+MODULE_ALIAS("platform:mailbox-sti");
--
1.9.1
This patch supplies the Mailbox Controller nodes. In order to
request channels, these nodes will be referenced by Mailbox
Client nodes.
Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/boot/dts/stih407-family.dtsi | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index 838b812..c061d11 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -11,6 +11,7 @@
#include <dt-bindings/phy/phy.h>
#include <dt-bindings/reset-controller/stih407-resets.h>
#include <dt-bindings/interrupt-controller/irq-st.h>
+#include <dt-bindings/mailbox/mailbox.h>
/ {
#address-cells = <1>;
#size-cells = <1>;
@@ -565,5 +566,38 @@
<&phy_port2 PHY_TYPE_USB3>;
};
};
+
+ mailbox0: mailbox@0 {
+ compatible = "st,stih407-mailbox";
+ reg = <0x8f00000 0x1000>;
+ interrupts = <GIC_SPI 1 IRQ_TYPE_NONE>;
+ #mbox-cells = <3>;
+ mbox-name = "a9";
+ status = "okay";
+ };
+
+ mailbox1: mailbox@1 {
+ compatible = "st,stih407-mailbox";
+ reg = <0x8f01000 0x1000>;
+ #mbox-cells = <3>;
+ mbox-name = "st231_gp_1";
+ status = "okay";
+ };
+
+ mailbox2: mailbox@2 {
+ compatible = "st,stih407-mailbox";
+ reg = <0x8f02000 0x1000>;
+ #mbox-cells = <3>;
+ mbox-name = "st231_gp_0";
+ status = "okay";
+ };
+
+ mailbox3: mailbox@3 {
+ compatible = "st,stih407-mailbox";
+ reg = <0x8f03000 0x1000>;
+ #mbox-cells = <3>;
+ mbox-name = "st231_audio_video";
+ status = "okay";
+ };
};
};
--
1.9.1
This particular Client implementation uses shared memory in order
to pass messages between Mailbox users; however, it can be easily
hacked to support any type of Controller.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/mailbox/Kconfig | 7 ++
drivers/mailbox/Makefile | 2 +
drivers/mailbox/mailbox-test.c | 210 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 219 insertions(+)
create mode 100644 drivers/mailbox/mailbox-test.c
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 2cc4c39..7720bde 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -77,4 +77,11 @@ config STI_MBOX
Mailbox implementation for STMicroelectonics family chips with
hardware for interprocessor communication.
+config MAILBOX_TEST
+ tristate "Mailbox Test Client"
+ depends on OF
+ help
+ Test client to help with testing new Controller driver
+ implementations.
+
endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 7cb4766..92435ef 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -2,6 +2,8 @@
obj-$(CONFIG_MAILBOX) += mailbox.o
+obj-$(CONFIG_MAILBOX_TEST) += mailbox-test.o
+
obj-$(CONFIG_ARM_MHU) += arm_mhu.o
obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o
diff --git a/drivers/mailbox/mailbox-test.c b/drivers/mailbox/mailbox-test.c
new file mode 100644
index 0000000..10bfe3a
--- /dev/null
+++ b/drivers/mailbox/mailbox-test.c
@@ -0,0 +1,210 @@
+/*
+ * Copyright (C) 2015 ST Microelectronics
+ *
+ * Author: Lee Jones <[email protected]>
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+
+#define MBOX_MAX_MSG_LEN 128
+
+static struct dentry *root_debugfs_dir;
+
+struct mbox_test_device {
+ struct device *dev;
+ struct mbox_chan *tx_channel;
+ struct mbox_chan *rx_channel;
+ void __iomem *mmio;
+
+};
+
+static ssize_t mbox_test_write(struct file *filp,
+ const char __user *userbuf,
+ size_t count, loff_t *ppos)
+{
+ struct mbox_test_device *tdev = filp->private_data;
+ char *message;
+ int ret;
+
+ if (count > MBOX_MAX_MSG_LEN) {
+ dev_err(tdev->dev,
+ "Message length %d greater than max allowed %d\n",
+ count, MBOX_MAX_MSG_LEN);
+ return -EINVAL;
+ }
+
+ message = kzalloc(count, GFP_KERNEL);
+ if (!message)
+ return -ENOMEM;
+
+ ret = copy_from_user(message, userbuf, count);
+ if (ret)
+ return -EFAULT;
+
+ print_hex_dump(KERN_ERR, "Client: Sending: ", DUMP_PREFIX_ADDRESS,
+ 16, 1, message, 16, true);
+
+ ret = mbox_send_message(tdev->tx_channel, message);
+ if (ret < 0)
+ dev_err(tdev->dev, "Failed to send message via mailbox\n");
+
+ kfree(message);
+
+ return ret < 0 ? ret : count;
+}
+
+static const struct file_operations mbox_test_ops = {
+ .write = mbox_test_write,
+ .open = simple_open,
+ .llseek = generic_file_llseek,
+};
+
+static int mbox_test_add_debugfs(struct platform_device *pdev,
+ struct mbox_test_device *tdev)
+{
+ if (!debugfs_initialized())
+ return 0;
+
+ root_debugfs_dir = debugfs_create_dir("mailbox", NULL);
+ if (!root_debugfs_dir) {
+ dev_err(&pdev->dev, "Failed to create Mailbox debugfs\n");
+ return -EINVAL;
+ }
+
+ debugfs_create_file("send-message", 0200, root_debugfs_dir,
+ tdev, &mbox_test_ops);
+
+ return 0;
+}
+
+static void mbox_test_receive_message(struct mbox_client *client, void *message)
+{
+ struct mbox_test_device *tdev = dev_get_drvdata(client->dev);
+
+ if (!tdev->mmio) {
+ dev_info(tdev->dev, "Client: Recived something [read mmio]\n");
+ return;
+ }
+
+ print_hex_dump(KERN_ERR, "Client: from co-proc: ", DUMP_PREFIX_ADDRESS,
+ 16, 1, tdev->mmio, MBOX_MAX_MSG_LEN, true);
+}
+
+static void mbox_test_prepare_message(struct mbox_client *client, void *message)
+{
+ struct mbox_test_device *tdev = dev_get_drvdata(client->dev);
+
+ if (tdev->mmio)
+ memcpy(tdev->mmio, message, MBOX_MAX_MSG_LEN);
+}
+
+static void mbox_test_message_sent(struct mbox_client *client,
+ void *message, int r)
+{
+ if (r)
+ dev_warn(client->dev,
+ "Client: Message could not be sent: %d\n", r);
+ else
+ dev_info(client->dev,
+ "Client: Message sent\n");
+}
+
+static struct mbox_chan *
+mbox_test_request_channel(struct platform_device *pdev, const char *name)
+{
+ struct mbox_client *client;
+
+ client = devm_kzalloc(&pdev->dev, sizeof(*client), GFP_KERNEL);
+ if (!client)
+ return ERR_PTR(-ENOMEM);
+
+ client->dev = &pdev->dev;
+ client->rx_callback = mbox_test_receive_message;
+ client->tx_prepare = mbox_test_prepare_message;
+ client->tx_done = mbox_test_message_sent;
+ client->tx_block = true;
+ client->knows_txdone = false;
+ client->tx_tout = 500;
+
+ return mbox_request_channel_byname(client, name);
+}
+
+static int mbox_test_probe(struct platform_device *pdev)
+{
+ struct mbox_test_device *tdev;
+ struct resource *res;
+ int ret;
+
+ tdev = devm_kzalloc(&pdev->dev, sizeof(*tdev), GFP_KERNEL);
+ if (!tdev)
+ return -ENOMEM;
+
+ /* It's okay for MMIO to be NULL */
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ tdev->mmio = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(tdev->mmio))
+ tdev->mmio = NULL;
+
+ tdev->tx_channel = mbox_test_request_channel(pdev, "tx");
+ if (IS_ERR(tdev->tx_channel))
+ return PTR_ERR(tdev->tx_channel);
+
+ tdev->rx_channel = mbox_test_request_channel(pdev, "rx");
+ if (IS_ERR(tdev->rx_channel))
+ return PTR_ERR(tdev->rx_channel);
+
+ tdev->dev = &pdev->dev;
+ platform_set_drvdata(pdev, tdev);
+
+ ret = mbox_test_add_debugfs(pdev, tdev);
+ if (ret)
+ return ret;
+
+ dev_info(&pdev->dev, "Successfully registered\n");
+
+ return 0;
+}
+
+static int mbox_test_remove(struct platform_device *pdev)
+{
+ struct mbox_test_device *tdev = platform_get_drvdata(pdev);
+
+ debugfs_remove_recursive(root_debugfs_dir);
+
+ mbox_free_channel(tdev->tx_channel);
+ mbox_free_channel(tdev->rx_channel);
+
+ return 0;
+}
+
+static const struct of_device_id mbox_test_match[] = {
+ { .compatible = "mailbox_test" },
+ {},
+};
+
+static struct platform_driver mbox_test_driver = {
+ .driver = {
+ .name = "mailbox_sti_test",
+ .of_match_table = mbox_test_match,
+ },
+ .probe = mbox_test_probe,
+ .remove = mbox_test_remove,
+};
+module_platform_driver(mbox_test_driver);
+
+MODULE_DESCRIPTION("Generic Mailbox Testing Facility");
+MODULE_AUTHOR("Lee Jones <[email protected]");
+MODULE_LICENSE("GPL v2");
--
1.9.1
This patch supplies a Client node to enable the Mailbox testing
facility. It will be used to send and receive messages from any
given co-processor in order to test the STi Mailbox Controller
driver.
Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/boot/dts/stih407-family.dtsi | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index c061d11..df7fe80 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -567,6 +567,12 @@
};
};
+ mailbox_test {
+ compatible = "mailbox_test";
+ mboxes = <&mailbox2 1 31 MBOX_TX>, <&mailbox0 1 31 MBOX_RX>;
+ mbox-names = "tx", "rx";
+ };
+
mailbox0: mailbox@0 {
compatible = "st,stih407-mailbox";
reg = <0x8f00000 0x1000>;
--
1.9.1
On Fri, Jul 17, 2015 at 5:34 PM, Lee Jones <[email protected]> wrote:
> ST's platforms currently support a maximum of 5 Mailboxes, one for
> each of the supported co-processors situated on the platform. Each
> Mailbox is divided up into 4 instances which consist of 32 channels.
> Messages are passed between the application and co-processors using
> shared memory areas. It is the Client's responsibility to manage
> these areas.
>
Thanks. It's a lot better than the old driver. However a few nits as usual :)
> +
> +#define STI_MBOX_INST_MAX 4 /* RAM saving: Max supported instances */
>
Above you say 5 instances. Another u32 doesn't cost much.
> +#define STI_MBOX_CHAN_MAX 20 /* RAM saving: Max supported channels */
> +
This assumption is reasonable.
> +
> +static void sti_mbox_enable_channel(struct mbox_chan *chan)
> +{
> + struct sti_channel *chan_info = chan->con_priv;
> + struct sti_mbox_device *mdev = chan_info->mdev;
> + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
> + unsigned int instance = chan_info->instance;
> + unsigned int channel = chan_info->channel;
> + unsigned long flags;
> + void __iomem *base;
> +
> + base = mdev->base + (instance * sizeof(u32));
> +
Maybe have something simpler like MBOX_BASE(instance)? Or some inline
function to avoid this 5-lines ritual?
> + spin_lock_irqsave(&sti_mbox_chan_lock, flags);
> + mdev->enabled[instance] |= BIT(channel);
> + writel_relaxed(BIT(channel), base + pdata->ena_set);
> + spin_unlock_irqrestore(&sti_mbox_chan_lock, flags);
>
You don't need locking for SET/CLR type registers which are meant for
when they could be accessed by processors that can not share a lock.
So maybe drop the lock here and elsewhere.
However, you need some mechanism to check if you succeeded 'owning'
the channel by reading back what you write to own the channel (not
sure which is that register here). Usually we need that action and
verification when we assign a channel to some user.
> +
> +static int sti_mbox_send_data(struct mbox_chan *chan, void *data)
> +{
> + struct sti_channel *chan_info = chan->con_priv;
> + struct sti_mbox_device *mdev = chan_info->mdev;
> + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
> + unsigned int instance = chan_info->instance;
> + unsigned int channel = chan_info->channel;
> + void __iomem *base;
> +
> + if (!sti_mbox_tx_is_ready(chan))
> + return -EBUSY;
This is the first thing I look out for in every new driver :) this
check is unnecessary.
> +
> +static void sti_mbox_shutdown_chan(struct mbox_chan *chan)
> +{
> + struct sti_channel *chan_info = chan->con_priv;
> + struct mbox_controller *mbox = chan_info->mdev->mbox;
> + int i;
> +
> + for (i = 0; i < mbox->num_chans; i++)
> + if (chan == &mbox->chans[i])
> + break;
> +
> + if (mbox->num_chans == i) {
> + dev_warn(mbox->dev, "Request to free non-existent channel\n");
> + return;
> + }
> +
> + sti_mbox_disable_channel(chan);
> + sti_mbox_clear_irq(chan);
> +
> + /* Reset channel */
> + memset(chan, 0, sizeof(*chan));
> + chan->mbox = mbox;
> + chan->txdone_method = TXDONE_BY_POLL;
>
No please. mbox_chan is owned by the API. At most you could clear con_priv.
> +static struct mbox_chan *sti_mbox_xlate(struct mbox_controller *mbox,
> + const struct of_phandle_args *spec)
> +{
> + struct sti_mbox_device *mdev = dev_get_drvdata(mbox->dev);
> + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
> + struct sti_channel *chan_info;
> + struct mbox_chan *chan = NULL;
> + unsigned int instance = spec->args[0];
> + unsigned int channel = spec->args[1];
> + unsigned int direction = spec->args[2];
> + int i;
> +
> + /* Bounds checking */
> + if (instance >= pdata->num_inst || channel >= pdata->num_chan) {
> + dev_err(mbox->dev,
> + "Invalid channel requested instance: %d channel: %d\n",
> + instance, channel);
> + return NULL;
> + }
> +
> + for (i = 0; i < mbox->num_chans; i++) {
> + chan_info = mbox->chans[i].con_priv;
> +
> + /* Is requested channel free? */
> + if (direction != MBOX_LOOPBACK &&
> + chan_info &&
> + mbox->dev == chan_info->mdev->dev &&
> + instance == chan_info->instance &&
> + channel == chan_info->channel) {
> + dev_err(mbox->dev, "Channel in use\n");
> + return NULL;
> + }
> +
> + /* Find the first free slot */
> + if (!chan && !chan_info)
> + chan = &mbox->chans[i];
shouldn't it break out of loop here?
> + }
> +
Doesn't mbox->chans[i].con_priv need some locking here?
> +
> +static const struct sti_mbox_pdata mbox_stih407_pdata = {
> + .num_inst = 4,
> + .num_chan = 32,
> + .irq_val = 0x04,
> + .irq_set = 0x24,
> + .irq_clr = 0x44,
> + .ena_val = 0x64,
> + .ena_set = 0x84,
> + .ena_clr = 0xa4,
>
Register offsets are parameters of the controller, and also these look
ugly. Please make these #define's
> +
> +static int __init sti_mbox_init(void)
> +{
> + return platform_driver_register(&sti_mbox_driver);
> +}
> +
> +static void __exit sti_mbox_exit(void)
> +{
> + platform_driver_unregister(&sti_mbox_driver);
> +}
> +
> +postcore_initcall(sti_mbox_init);
>
This seems fragile. Shouldn't the users defer probe if they don't get a channel?
cheers!
On Tue, 21 Jul 2015, Jassi Brar wrote:
> On Fri, Jul 17, 2015 at 5:34 PM, Lee Jones <[email protected]> wrote:
> > ST's platforms currently support a maximum of 5 Mailboxes, one for
> > each of the supported co-processors situated on the platform. Each
> > Mailbox is divided up into 4 instances which consist of 32 channels.
> > Messages are passed between the application and co-processors using
> > shared memory areas. It is the Client's responsibility to manage
> > these areas.
> >
> Thanks. It's a lot better than the old driver. However a few nits as usual :)
Never a problem. :)
> > +
> > +#define STI_MBOX_INST_MAX 4 /* RAM saving: Max supported instances */
> >
> Above you say 5 instances. Another u32 doesn't cost much.
4 instances, 5 mailboxes.
> > +#define STI_MBOX_CHAN_MAX 20 /* RAM saving: Max supported channels */
> > +
> This assumption is reasonable.
>
> > +
> > +static void sti_mbox_enable_channel(struct mbox_chan *chan)
> > +{
> > + struct sti_channel *chan_info = chan->con_priv;
> > + struct sti_mbox_device *mdev = chan_info->mdev;
> > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
> > + unsigned int instance = chan_info->instance;
> > + unsigned int channel = chan_info->channel;
> > + unsigned long flags;
> > + void __iomem *base;
> > +
> > + base = mdev->base + (instance * sizeof(u32));
> > +
> Maybe have something simpler like MBOX_BASE(instance)? Or some inline
> function to avoid this 5-lines ritual?
I think some of the functions also make use of the intermediary
pointers, but I'll look into it.
> > + spin_lock_irqsave(&sti_mbox_chan_lock, flags);
> > + mdev->enabled[instance] |= BIT(channel);
> > + writel_relaxed(BIT(channel), base + pdata->ena_set);
> > + spin_unlock_irqrestore(&sti_mbox_chan_lock, flags);
> >
> You don't need locking for SET/CLR type registers which are meant for
> when they could be accessed by processors that can not share a lock.
> So maybe drop the lock here and elsewhere.
Okay.
> However, you need some mechanism to check if you succeeded 'owning'
> the channel by reading back what you write to own the channel (not
> sure which is that register here). Usually we need that action and
> verification when we assign a channel to some user.
I don't think there is a technical reason why it wouldn't succeed. We
don't normally read back every register change me make. Why is this
IP different?
> > +static int sti_mbox_send_data(struct mbox_chan *chan, void *data)
> > +{
> > + struct sti_channel *chan_info = chan->con_priv;
> > + struct sti_mbox_device *mdev = chan_info->mdev;
> > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
> > + unsigned int instance = chan_info->instance;
> > + unsigned int channel = chan_info->channel;
> > + void __iomem *base;
> > +
> > + if (!sti_mbox_tx_is_ready(chan))
> > + return -EBUSY;
> This is the first thing I look out for in every new driver :) this
> check is unnecessary.
In what way? What if the channel is disabled or there is an IRQ
already pending?
> > +static void sti_mbox_shutdown_chan(struct mbox_chan *chan)
> > +{
> > + struct sti_channel *chan_info = chan->con_priv;
> > + struct mbox_controller *mbox = chan_info->mdev->mbox;
> > + int i;
> > +
> > + for (i = 0; i < mbox->num_chans; i++)
> > + if (chan == &mbox->chans[i])
> > + break;
> > +
> > + if (mbox->num_chans == i) {
> > + dev_warn(mbox->dev, "Request to free non-existent channel\n");
> > + return;
> > + }
> > +
> > + sti_mbox_disable_channel(chan);
> > + sti_mbox_clear_irq(chan);
> > +
> > + /* Reset channel */
> > + memset(chan, 0, sizeof(*chan));
> > + chan->mbox = mbox;
> > + chan->txdone_method = TXDONE_BY_POLL;
> >
> No please. mbox_chan is owned by the API. At most you could clear con_priv.
I will look for the API call to reset the channel then.
> > +static struct mbox_chan *sti_mbox_xlate(struct mbox_controller *mbox,
> > + const struct of_phandle_args *spec)
> > +{
> > + struct sti_mbox_device *mdev = dev_get_drvdata(mbox->dev);
> > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
> > + struct sti_channel *chan_info;
> > + struct mbox_chan *chan = NULL;
> > + unsigned int instance = spec->args[0];
> > + unsigned int channel = spec->args[1];
> > + unsigned int direction = spec->args[2];
> > + int i;
> > +
> > + /* Bounds checking */
> > + if (instance >= pdata->num_inst || channel >= pdata->num_chan) {
> > + dev_err(mbox->dev,
> > + "Invalid channel requested instance: %d channel: %d\n",
> > + instance, channel);
> > + return NULL;
> > + }
> > +
> > + for (i = 0; i < mbox->num_chans; i++) {
> > + chan_info = mbox->chans[i].con_priv;
> > +
> > + /* Is requested channel free? */
> > + if (direction != MBOX_LOOPBACK &&
> > + chan_info &&
> > + mbox->dev == chan_info->mdev->dev &&
> > + instance == chan_info->instance &&
> > + channel == chan_info->channel) {
> > + dev_err(mbox->dev, "Channel in use\n");
> > + return NULL;
> > + }
> > +
> > + /* Find the first free slot */
> > + if (!chan && !chan_info)
> > + chan = &mbox->chans[i];
> shouldn't it break out of loop here?
Yes, I guess it should. Good spot.
> > + }
> > +
> Doesn't mbox->chans[i].con_priv need some locking here?
I can add some.
> > +static const struct sti_mbox_pdata mbox_stih407_pdata = {
> > + .num_inst = 4,
> > + .num_chan = 32,
> > + .irq_val = 0x04,
> > + .irq_set = 0x24,
> > + .irq_clr = 0x44,
> > + .ena_val = 0x64,
> > + .ena_set = 0x84,
> > + .ena_clr = 0xa4,
> >
> Register offsets are parameters of the controller
And this is a controller driver? Not sure I get the point.
> and also these look ugly. Please make these #define's
Sure.
> > +static int __init sti_mbox_init(void)
> > +{
> > + return platform_driver_register(&sti_mbox_driver);
> > +}
> > +
> > +static void __exit sti_mbox_exit(void)
> > +{
> > + platform_driver_unregister(&sti_mbox_driver);
> > +}
> > +
> > +postcore_initcall(sti_mbox_init);
> >
> This seems fragile. Shouldn't the users defer probe if they don't get a channel?
I'm not sure why we have to be early. I will investigate.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, Jul 21, 2015 at 8:36 PM, Lee Jones <[email protected]> wrote:
> On Tue, 21 Jul 2015, Jassi Brar wrote:
>
>> However, you need some mechanism to check if you succeeded 'owning'
>> the channel by reading back what you write to own the channel (not
>> sure which is that register here). Usually we need that action and
>> verification when we assign a channel to some user.
>
> I don't think there is a technical reason why it wouldn't succeed. We
> don't normally read back every register change me make. Why is this
> IP different?
>
Not the IP, but register access. SET and CLR type registers work on
individual bits because the processors don't share a lock that
protects register read->modify->write.
Usually there is also some flag that is set to some unique value to
claim ownership of the resource, and if two processors try to claim
simultaneously we need to check if the flag reads back the value we
set to assert claim. There should be some such flag/register but as I
said I don't know if/which. Is there?
>> > +static int sti_mbox_send_data(struct mbox_chan *chan, void *data)
>> > +{
>> > + struct sti_channel *chan_info = chan->con_priv;
>> > + struct sti_mbox_device *mdev = chan_info->mdev;
>> > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
>> > + unsigned int instance = chan_info->instance;
>> > + unsigned int channel = chan_info->channel;
>> > + void __iomem *base;
>> > +
>> > + if (!sti_mbox_tx_is_ready(chan))
>> > + return -EBUSY;
>> This is the first thing I look out for in every new driver :) this
>> check is unnecessary.
>
> In what way? What if the channel is disabled or there is an IRQ
> already pending?
>
API calls send_data() only if last_tx_done() returned true.
>> > +static void sti_mbox_shutdown_chan(struct mbox_chan *chan)
>> > +{
>> > + struct sti_channel *chan_info = chan->con_priv;
>> > + struct mbox_controller *mbox = chan_info->mdev->mbox;
>> > + int i;
>> > +
>> > + for (i = 0; i < mbox->num_chans; i++)
>> > + if (chan == &mbox->chans[i])
>> > + break;
>> > +
>> > + if (mbox->num_chans == i) {
>> > + dev_warn(mbox->dev, "Request to free non-existent channel\n");
>> > + return;
>> > + }
>> > +
>> > + sti_mbox_disable_channel(chan);
>> > + sti_mbox_clear_irq(chan);
>> > +
>> > + /* Reset channel */
>> > + memset(chan, 0, sizeof(*chan));
>> > + chan->mbox = mbox;
>> > + chan->txdone_method = TXDONE_BY_POLL;
>> >
>> No please. mbox_chan is owned by the API. At most you could clear con_priv.
>
> I will look for the API call to reset the channel then.
>
API internally does any needed reset.
>> > +static const struct sti_mbox_pdata mbox_stih407_pdata = {
>> > + .num_inst = 4,
>> > + .num_chan = 32,
>> > + .irq_val = 0x04,
>> > + .irq_set = 0x24,
>> > + .irq_clr = 0x44,
>> > + .ena_val = 0x64,
>> > + .ena_set = 0x84,
>> > + .ena_clr = 0xa4,
>> >
>> Register offsets are parameters of the controller
>
> And this is a controller driver? Not sure I get the point.
>
Platform_data usually carries board/platform specific parameters.
Register offset "properties" are as fixed as the behavior of the
controller, so they should stay inside the code, not come via
platform_data.
On Tue, 21 Jul 2015, Jassi Brar wrote:
> On Tue, Jul 21, 2015 at 8:36 PM, Lee Jones <[email protected]> wrote:
> > On Tue, 21 Jul 2015, Jassi Brar wrote:
> >
> >> However, you need some mechanism to check if you succeeded 'owning'
> >> the channel by reading back what you write to own the channel (not
> >> sure which is that register here). Usually we need that action and
> >> verification when we assign a channel to some user.
> >
> > I don't think there is a technical reason why it wouldn't succeed. We
> > don't normally read back every register change me make. Why is this
> > IP different?
> >
> Not the IP, but register access. SET and CLR type registers work on
> individual bits because the processors don't share a lock that
> protects register read->modify->write.
>
> Usually there is also some flag that is set to some unique value to
> claim ownership of the resource, and if two processors try to claim
> simultaneously we need to check if the flag reads back the value we
> set to assert claim. There should be some such flag/register but as I
> said I don't know if/which. Is there?
The only registers we have available are; read_irq_value,
set_irq_value, clear_irq_value, read_enable_value, set_enable_value
and clear_enable_values.
This controller doesn't claim ownership of anything. It essentially
just turns IRQs on and off.
> >> > +static int sti_mbox_send_data(struct mbox_chan *chan, void *data)
> >> > +{
> >> > + struct sti_channel *chan_info = chan->con_priv;
> >> > + struct sti_mbox_device *mdev = chan_info->mdev;
> >> > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
> >> > + unsigned int instance = chan_info->instance;
> >> > + unsigned int channel = chan_info->channel;
> >> > + void __iomem *base;
> >> > +
> >> > + if (!sti_mbox_tx_is_ready(chan))
> >> > + return -EBUSY;
> >> This is the first thing I look out for in every new driver :) this
> >> check is unnecessary.
> >
> > In what way? What if the channel is disabled or there is an IRQ
> > already pending?
> >
> API calls send_data() only if last_tx_done() returned true.
I know for a fact that the 'catchers' in sti_mbox_tx_is_ready() to
fire, because I have triggered them. I'd really rather keep this
harmless check in.
> >> > +static void sti_mbox_shutdown_chan(struct mbox_chan *chan)
> >> > +{
> >> > + struct sti_channel *chan_info = chan->con_priv;
> >> > + struct mbox_controller *mbox = chan_info->mdev->mbox;
> >> > + int i;
> >> > +
> >> > + for (i = 0; i < mbox->num_chans; i++)
> >> > + if (chan == &mbox->chans[i])
> >> > + break;
> >> > +
> >> > + if (mbox->num_chans == i) {
> >> > + dev_warn(mbox->dev, "Request to free non-existent channel\n");
> >> > + return;
> >> > + }
> >> > +
> >> > + sti_mbox_disable_channel(chan);
> >> > + sti_mbox_clear_irq(chan);
> >> > +
> >> > + /* Reset channel */
> >> > + memset(chan, 0, sizeof(*chan));
> >> > + chan->mbox = mbox;
> >> > + chan->txdone_method = TXDONE_BY_POLL;
> >> >
> >> No please. mbox_chan is owned by the API. At most you could clear con_priv.
> >
> > I will look for the API call to reset the channel then.
> >
> API internally does any needed reset.
Okay, thanks for the clarification, I will remove these lines.
> >> > +static const struct sti_mbox_pdata mbox_stih407_pdata = {
> >> > + .num_inst = 4,
> >> > + .num_chan = 32,
> >> > + .irq_val = 0x04,
> >> > + .irq_set = 0x24,
> >> > + .irq_clr = 0x44,
> >> > + .ena_val = 0x64,
> >> > + .ena_set = 0x84,
> >> > + .ena_clr = 0xa4,
> >> >
> >> Register offsets are parameters of the controller
> >
> > And this is a controller driver? Not sure I get the point.
> >
> Platform_data usually carries board/platform specific parameters.
> Register offset "properties" are as fixed as the behavior of the
> controller, so they should stay inside the code, not come via
> platform_data.
That's not the case for this controller. There is nothing preventing
these values from changing on a new board revisions. After all, this
isn't really a Controller IP per-say.
AFAIK, all current platforms do use this mapping, so I can change it
with a view to changing it back if a different set of offsets appear
in subsequent incarnations. But again, I think it's pretty harmless.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, Jul 21, 2015 at 9:22 PM, Lee Jones <[email protected]> wrote:
> On Tue, 21 Jul 2015, Jassi Brar wrote:
>
>> >
>> >> > +static int sti_mbox_send_data(struct mbox_chan *chan, void *data)
>> >> > +{
>> >> > + struct sti_channel *chan_info = chan->con_priv;
>> >> > + struct sti_mbox_device *mdev = chan_info->mdev;
>> >> > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
>> >> > + unsigned int instance = chan_info->instance;
>> >> > + unsigned int channel = chan_info->channel;
>> >> > + void __iomem *base;
>> >> > +
>> >> > + if (!sti_mbox_tx_is_ready(chan))
>> >> > + return -EBUSY;
>> >> This is the first thing I look out for in every new driver :) this
>> >> check is unnecessary.
>> >
>> > In what way? What if the channel is disabled or there is an IRQ
>> > already pending?
>> >
>> API calls send_data() only if last_tx_done() returned true.
>
> I know for a fact that the 'catchers' in sti_mbox_tx_is_ready() to
> fire, because I have triggered them. I'd really rather keep this
> harmless check in.
>
If you put some printk in send_data() and last_tx_done() you'll see
what I mean :)
>> >> > +static const struct sti_mbox_pdata mbox_stih407_pdata = {
>> >> > + .num_inst = 4,
>> >> > + .num_chan = 32,
>> >> > + .irq_val = 0x04,
>> >> > + .irq_set = 0x24,
>> >> > + .irq_clr = 0x44,
>> >> > + .ena_val = 0x64,
>> >> > + .ena_set = 0x84,
>> >> > + .ena_clr = 0xa4,
>> >> >
>> >> Register offsets are parameters of the controller
>> >
>> > And this is a controller driver? Not sure I get the point.
>> >
>> Platform_data usually carries board/platform specific parameters.
>> Register offset "properties" are as fixed as the behavior of the
>> controller, so they should stay inside the code, not come via
>> platform_data.
>
> That's not the case for this controller. There is nothing preventing
> these values from changing on a new board revisions.
>
Hmm ... interesting! Can't see how enable/disable channel and irq
set/clear could be effected by writing to random, but agreed upon,
location between two processors. There ought to be some controller
listening there? Now I am more interested in knowing this IP :)
thnx.
On Tue, 21 Jul 2015, Jassi Brar wrote:
> On Tue, Jul 21, 2015 at 9:22 PM, Lee Jones <[email protected]> wrote:
> > On Tue, 21 Jul 2015, Jassi Brar wrote:
> >
> >> >
> >> >> > +static int sti_mbox_send_data(struct mbox_chan *chan, void *data)
> >> >> > +{
> >> >> > + struct sti_channel *chan_info = chan->con_priv;
> >> >> > + struct sti_mbox_device *mdev = chan_info->mdev;
> >> >> > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
> >> >> > + unsigned int instance = chan_info->instance;
> >> >> > + unsigned int channel = chan_info->channel;
> >> >> > + void __iomem *base;
> >> >> > +
> >> >> > + if (!sti_mbox_tx_is_ready(chan))
> >> >> > + return -EBUSY;
> >> >> This is the first thing I look out for in every new driver :) this
> >> >> check is unnecessary.
> >> >
> >> > In what way? What if the channel is disabled or there is an IRQ
> >> > already pending?
> >> >
> >> API calls send_data() only if last_tx_done() returned true.
> >
> > I know for a fact that the 'catchers' in sti_mbox_tx_is_ready() to
> > fire, because I have triggered them. I'd really rather keep this
> > harmless check in.
> >
> If you put some printk in send_data() and last_tx_done() you'll see
> what I mean :)
>
> >> >> > +static const struct sti_mbox_pdata mbox_stih407_pdata = {
> >> >> > + .num_inst = 4,
> >> >> > + .num_chan = 32,
> >> >> > + .irq_val = 0x04,
> >> >> > + .irq_set = 0x24,
> >> >> > + .irq_clr = 0x44,
> >> >> > + .ena_val = 0x64,
> >> >> > + .ena_set = 0x84,
> >> >> > + .ena_clr = 0xa4,
> >> >> >
> >> >> Register offsets are parameters of the controller
> >> >
> >> > And this is a controller driver? Not sure I get the point.
> >> >
> >> Platform_data usually carries board/platform specific parameters.
> >> Register offset "properties" are as fixed as the behavior of the
> >> controller, so they should stay inside the code, not come via
> >> platform_data.
> >
> > That's not the case for this controller. There is nothing preventing
> > these values from changing on a new board revisions.
> >
> Hmm ... interesting! Can't see how enable/disable channel and irq
> set/clear could be effected by writing to random, but agreed upon,
> location between two processors. There ought to be some controller
> listening there? Now I am more interested in knowing this IP :)
High level
----------
MB0 MB1 MB2 MB3 MB4
+---------+---------+---------+---------+---------+
INST0 | | | | | |
+---------+---------+---------+---------+---------+
INST1 | | | | | |
+---------+---------+---------+---------+---------+
INST2 | | | | | |
+---------+---------+---------+---------+---------+
INST3 | | | | | |
+---------+---------+---------+---------+---------+
Low level [each box above looks like this)
------------------------------------------
1 32
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
IRQ_VAL | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
IRQ_SET | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
IRQ_CLR | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
ENB_VAL | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
ENB_SET | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
ENB_CLR | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
That's it. That's the entirety of the "IP".
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, Jul 21, 2015 at 11:18 PM, Lee Jones <[email protected]> wrote:
> On Tue, 21 Jul 2015, Jassi Brar wrote:
>
>> On Tue, Jul 21, 2015 at 9:22 PM, Lee Jones <[email protected]> wrote:
>> > On Tue, 21 Jul 2015, Jassi Brar wrote:
>> >
>> >> >
>> >> >> > +static int sti_mbox_send_data(struct mbox_chan *chan, void *data)
>> >> >> > +{
>> >> >> > + struct sti_channel *chan_info = chan->con_priv;
>> >> >> > + struct sti_mbox_device *mdev = chan_info->mdev;
>> >> >> > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
>> >> >> > + unsigned int instance = chan_info->instance;
>> >> >> > + unsigned int channel = chan_info->channel;
>> >> >> > + void __iomem *base;
>> >> >> > +
>> >> >> > + if (!sti_mbox_tx_is_ready(chan))
>> >> >> > + return -EBUSY;
>> >> >> This is the first thing I look out for in every new driver :) this
>> >> >> check is unnecessary.
>> >> >
>> >> > In what way? What if the channel is disabled or there is an IRQ
>> >> > already pending?
>> >> >
>> >> API calls send_data() only if last_tx_done() returned true.
>> >
>> > I know for a fact that the 'catchers' in sti_mbox_tx_is_ready() to
>> > fire, because I have triggered them. I'd really rather keep this
>> > harmless check in.
>> >
>> If you put some printk in send_data() and last_tx_done() you'll see
>> what I mean :)
>>
>> >> >> > +static const struct sti_mbox_pdata mbox_stih407_pdata = {
>> >> >> > + .num_inst = 4,
>> >> >> > + .num_chan = 32,
>> >> >> > + .irq_val = 0x04,
>> >> >> > + .irq_set = 0x24,
>> >> >> > + .irq_clr = 0x44,
>> >> >> > + .ena_val = 0x64,
>> >> >> > + .ena_set = 0x84,
>> >> >> > + .ena_clr = 0xa4,
>> >> >> >
>> >> >> Register offsets are parameters of the controller
>> >> >
>> >> > And this is a controller driver? Not sure I get the point.
>> >> >
>> >> Platform_data usually carries board/platform specific parameters.
>> >> Register offset "properties" are as fixed as the behavior of the
>> >> controller, so they should stay inside the code, not come via
>> >> platform_data.
>> >
>> > That's not the case for this controller. There is nothing preventing
>> > these values from changing on a new board revisions.
>> >
>> Hmm ... interesting! Can't see how enable/disable channel and irq
>> set/clear could be effected by writing to random, but agreed upon,
>> location between two processors. There ought to be some controller
>> listening there? Now I am more interested in knowing this IP :)
>
> High level
> ----------
>
> MB0 MB1 MB2 MB3 MB4
> +---------+---------+---------+---------+---------+
> INST0 | | | | | |
> +---------+---------+---------+---------+---------+
> INST1 | | | | | |
> +---------+---------+---------+---------+---------+
> INST2 | | | | | |
> +---------+---------+---------+---------+---------+
> INST3 | | | | | |
> +---------+---------+---------+---------+---------+
>
> Low level [each box above looks like this)
> ------------------------------------------
>
> 1 32
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> IRQ_VAL | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | |
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> IRQ_SET | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | |
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> IRQ_CLR | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | |
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> ENB_VAL | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | |
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> ENB_SET | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | |
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> ENB_CLR | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | |
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>
> That's it. That's the entirety of the "IP".
>
Thanks for taking time out to draw it. Reading code I did get the idea
that mailbox registers are interleaved rather than usual separate
regions. But that doesn't change anything.
Regardless of the organisation, the registers do have to be at a
particular address... I mean when you set some bit in ENB_SET
'register' there has to be "something" beneath it that triggers the
interrupt. That "something" is the controller, which can't see such
writes to other locations. Right? I mean this is just like any other
device controller which may have register space at different offsets
but relative addresses of registers won't change across platforms.
thanks.
Let's carry on the conversation here, rather than submitting my fixed
up patch. Once agreed, I will re-send.
> On Fri, Jul 17, 2015 at 5:34 PM, Lee Jones <[email protected]> wrote:
> > ST's platforms currently support a maximum of 5 Mailboxes, one for
> > each of the supported co-processors situated on the platform. Each
> > Mailbox is divided up into 4 instances which consist of 32 channels.
> > Messages are passed between the application and co-processors using
> > shared memory areas. It is the Client's responsibility to manage
> > these areas.
> >
> Thanks. It's a lot better than the old driver. However a few nits as usual :)
>
> > +
> > +#define STI_MBOX_INST_MAX 4 /* RAM saving: Max supported instances */
> >
> Above you say 5 instances. Another u32 doesn't cost much.
This should stay the same. There are 4 instances.
> > +#define STI_MBOX_CHAN_MAX 20 /* RAM saving: Max supported channels */
> > +
> This assumption is reasonable.
>
> > +
> > +static void sti_mbox_enable_channel(struct mbox_chan *chan)
> > +{
> > + struct sti_channel *chan_info = chan->con_priv;
> > + struct sti_mbox_device *mdev = chan_info->mdev;
> > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
> > + unsigned int instance = chan_info->instance;
> > + unsigned int channel = chan_info->channel;
> > + unsigned long flags;
> > + void __iomem *base;
> > +
> > + base = mdev->base + (instance * sizeof(u32));
> > +
> Maybe have something simpler like MBOX_BASE(instance)? Or some inline
> function to avoid this 5-lines ritual?
I've checked and we can't do this, as the we need most (all?) of the
intermediary variables too. No ritual just to get the final variable
for instance.
> > + spin_lock_irqsave(&sti_mbox_chan_lock, flags);
> > + mdev->enabled[instance] |= BIT(channel);
> > + writel_relaxed(BIT(channel), base + pdata->ena_set);
> > + spin_unlock_irqrestore(&sti_mbox_chan_lock, flags);
> >
> You don't need locking for SET/CLR type registers which are meant for
> when they could be accessed by processors that can not share a lock.
> So maybe drop the lock here and elsewhere.
>From what I can gather, I think we need this locking. What happens if
we get scheduled between setting the enabled bit in our cache and
actually setting the ena_set bit? We would be out of sync.
> However, you need some mechanism to check if you succeeded 'owning'
> the channel by reading back what you write to own the channel (not
> sure which is that register here). Usually we need that action and
> verification when we assign a channel to some user.
I don't think we need to do that with this driver. All of the
allocation is controlled from within this code base. The channels are
pre-allocated and written into the co-proc's Firmware.
> > +static int sti_mbox_send_data(struct mbox_chan *chan, void *data)
> > +{
> > + struct sti_channel *chan_info = chan->con_priv;
> > + struct sti_mbox_device *mdev = chan_info->mdev;
> > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
> > + unsigned int instance = chan_info->instance;
> > + unsigned int channel = chan_info->channel;
> > + void __iomem *base;
> > +
> > + if (!sti_mbox_tx_is_ready(chan))
> > + return -EBUSY;
> This is the first thing I look out for in every new driver :) this
> check is unnecessary.
I see what you mean now. I will remove this check.
> > +static void sti_mbox_shutdown_chan(struct mbox_chan *chan)
> > +{
> > + struct sti_channel *chan_info = chan->con_priv;
> > + struct mbox_controller *mbox = chan_info->mdev->mbox;
> > + int i;
> > +
> > + for (i = 0; i < mbox->num_chans; i++)
> > + if (chan == &mbox->chans[i])
> > + break;
> > +
> > + if (mbox->num_chans == i) {
> > + dev_warn(mbox->dev, "Request to free non-existent channel\n");
> > + return;
> > + }
> > +
> > + sti_mbox_disable_channel(chan);
> > + sti_mbox_clear_irq(chan);
> > +
> > + /* Reset channel */
> > + memset(chan, 0, sizeof(*chan));
> > + chan->mbox = mbox;
> > + chan->txdone_method = TXDONE_BY_POLL;
> >
> No please. mbox_chan is owned by the API. At most you could clear con_priv.
Removed.
> > +static struct mbox_chan *sti_mbox_xlate(struct mbox_controller *mbox,
> > + const struct of_phandle_args *spec)
> > +{
> > + struct sti_mbox_device *mdev = dev_get_drvdata(mbox->dev);
> > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
> > + struct sti_channel *chan_info;
> > + struct mbox_chan *chan = NULL;
> > + unsigned int instance = spec->args[0];
> > + unsigned int channel = spec->args[1];
> > + unsigned int direction = spec->args[2];
> > + int i;
> > +
> > + /* Bounds checking */
> > + if (instance >= pdata->num_inst || channel >= pdata->num_chan) {
> > + dev_err(mbox->dev,
> > + "Invalid channel requested instance: %d channel: %d\n",
> > + instance, channel);
> > + return NULL;
> > + }
> > +
> > + for (i = 0; i < mbox->num_chans; i++) {
> > + chan_info = mbox->chans[i].con_priv;
> > +
> > + /* Is requested channel free? */
> > + if (direction != MBOX_LOOPBACK &&
> > + chan_info &&
> > + mbox->dev == chan_info->mdev->dev &&
> > + instance == chan_info->instance &&
> > + channel == chan_info->channel) {
> > + dev_err(mbox->dev, "Channel in use\n");
> > + return NULL;
> > + }
> > +
> > + /* Find the first free slot */
> > + if (!chan && !chan_info)
> > + chan = &mbox->chans[i];
> shouldn't it break out of loop here?
I checked that and we don't. We need to check all of the channels do
wee if we have double allocated. If we have, NULL is returned from
above.
I have; however, updated the comment above for clarity.
> > + }
> > +
> Doesn't mbox->chans[i].con_priv need some locking here?
Not that I can see. Would you mind explaining further please?
> > +static const struct sti_mbox_pdata mbox_stih407_pdata = {
> > + .num_inst = 4,
> > + .num_chan = 32,
> > + .irq_val = 0x04,
> > + .irq_set = 0x24,
> > + .irq_clr = 0x44,
> > + .ena_val = 0x64,
> > + .ena_set = 0x84,
> > + .ena_clr = 0xa4,
> >
> Register offsets are parameters of the controller, and also these look
> ugly. Please make these #define's
These are now #defined.
> > +static int __init sti_mbox_init(void)
> > +{
> > + return platform_driver_register(&sti_mbox_driver);
> > +}
> > +
> > +static void __exit sti_mbox_exit(void)
> > +{
> > + platform_driver_unregister(&sti_mbox_driver);
> > +}
> > +
> > +postcore_initcall(sti_mbox_init);
> >
> This seems fragile. Shouldn't the users defer probe if they don't get a channel?
This has been converted to module_platform_driver()
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Thu, Jul 23, 2015 at 1:59 PM, Lee Jones <[email protected]> wrote:
>> On Fri, Jul 17, 2015 at 5:34 PM, Lee Jones <[email protected]> wrote:
>> > +static void sti_mbox_enable_channel(struct mbox_chan *chan)
>> > +{
>> > + struct sti_channel *chan_info = chan->con_priv;
>> > + struct sti_mbox_device *mdev = chan_info->mdev;
>> > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
>> > + unsigned int instance = chan_info->instance;
>> > + unsigned int channel = chan_info->channel;
>> > + unsigned long flags;
>> > + void __iomem *base;
>> > +
>> > + base = mdev->base + (instance * sizeof(u32));
>> > +
>> Maybe have something simpler like MBOX_BASE(instance)? Or some inline
>> function to avoid this 5-lines ritual?
>
> I've checked and we can't do this, as the we need most (all?) of the
> intermediary variables too. No ritual just to get the final variable
> for instance.
>
OK. How about ?
#define MBOX_BASE(m, n) ((m)->base + (n) * 4)
void __iomem *base = MBOX_BASE(mdev, instance);
>> > + spin_lock_irqsave(&sti_mbox_chan_lock, flags);
>> > + mdev->enabled[instance] |= BIT(channel);
>> > + writel_relaxed(BIT(channel), base + pdata->ena_set);
>> > + spin_unlock_irqrestore(&sti_mbox_chan_lock, flags);
>> >
>> You don't need locking for SET/CLR type registers which are meant for
>> when they could be accessed by processors that can not share a lock.
>> So maybe drop the lock here and elsewhere.
>
> From what I can gather, I think we need this locking. What happens if
> we get scheduled between setting the enabled bit in our cache and
> actually setting the ena_set bit? We would be out of sync.
>
IIU what you mean... can't that still happen because of the _relaxed()?
Anyways my point was about set/clr type regs. And you may look at if
channel really needs disabling while interrupts are handled? I think
it shouldn't because either it is going to be a to-fro communication
or random broadcasts without any guarantee of reception. But of course
you would know better your platform.
BTW sti_mbox_channel_is_enabled() also needs to have locking around
enabled[] if you want to keep it. And maybe embed sti_mbox_chan_lock
inside sti_mbox_device.
>> However, you need some mechanism to check if you succeeded 'owning'
>> the channel by reading back what you write to own the channel (not
>> sure which is that register here). Usually we need that action and
>> verification when we assign a channel to some user.
>
> I don't think we need to do that with this driver. All of the
> allocation is controlled from within this code base. The channels are
> pre-allocated and written into the co-proc's Firmware.
>
OK
>> > + }
>> > +
>> Doesn't mbox->chans[i].con_priv need some locking here?
>
> Not that I can see. Would you mind explaining further please?
>
Not anymore, after the clarification that we don't need a 'break' statement.
cheers.
Hi Lee,
On Fri, Jul 17, 2015 at 3:04 PM, Lee Jones <[email protected]> wrote:
> ST's platforms currently support a maximum of 5 Mailboxes, one for
> each of the supported co-processors situated on the platform. Each
> Mailbox is divided up into 4 instances which consist of 32 channels.
> Messages are passed between the application and co-processors using
> shared memory areas. It is the Client's responsibility to manage
> these areas.
>
> Signed-off-by: Lee Jones <[email protected]>
> ---
> drivers/mailbox/Kconfig | 7 +
> drivers/mailbox/Makefile | 2 +
> drivers/mailbox/mailbox-sti.c | 562 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 571 insertions(+)
> create mode 100644 drivers/mailbox/mailbox-sti.c
[..]
> +static irqreturn_t sti_mbox_thread_handler(int irq, void *data)
> +{
> + struct sti_mbox_device *mdev = data;
> + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
> + struct mbox_chan *chan;
> + unsigned int instance;
> +
> + for (instance = 0; instance < pdata->num_inst; instance++) {
> +keep_looking:
> + chan = sti_mbox_irq_to_channel(mdev, instance);
> + if (!chan)
> + continue;
> +
> + mbox_chan_received_data(chan, NULL);
> + sti_mbox_clear_irq(chan);
> + sti_mbox_enable_channel(chan);
> + goto keep_looking;
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t sti_mbox_irq_handler(int irq, void *data)
> +{
> + struct sti_mbox_device *mdev = data;
> + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
> + struct sti_channel *chan_info;
> + struct mbox_chan *chan;
> + unsigned int instance;
> + int ret = IRQ_NONE;
> +
> + for (instance = 0; instance < pdata->num_inst; instance++) {
> + chan = sti_mbox_irq_to_channel(mdev, instance);
> + if (!chan)
> + continue;
> + chan_info = chan->con_priv;
> +
> + if (!sti_mbox_channel_is_enabled(chan)) {
> + dev_warn(mdev->dev,
> + "Unexpected IRQ: %s\n"
> + " instance: %d: channel: %d [enabled: %x]\n",
> + mdev->name, chan_info->instance,
> + chan_info->channel, mdev->enabled[instance]);
> + ret = IRQ_HANDLED;
> + continue;
> + }
> +
> + sti_mbox_disable_channel(chan);
> + ret = IRQ_WAKE_THREAD;
> + }
> +
> + if (ret == IRQ_NONE)
> + dev_err(mdev->dev, "Spurious IRQ - was a channel requested?\n");
> +
> + return ret;
> +}
With such usage of ret variable can it happen that handling of last
but one channel/instance will set ret to IRQ_WAKE_THREAD and at the
same time handling of last channel/instance will set ret to
IRQ_HANDLED during iteration loop and finally generic subsystem will
not wake up thread handler because it will receive IRQ_HANDLED?
Just checking.
[..]
--
Thanks,
Alexey Klimov
On Thu, 23 Jul 2015, Jassi Brar wrote:
> On Thu, Jul 23, 2015 at 1:59 PM, Lee Jones <[email protected]> wrote:
> >> On Fri, Jul 17, 2015 at 5:34 PM, Lee Jones <[email protected]> wrote:
>
> >> > +static void sti_mbox_enable_channel(struct mbox_chan *chan)
> >> > +{
> >> > + struct sti_channel *chan_info = chan->con_priv;
> >> > + struct sti_mbox_device *mdev = chan_info->mdev;
> >> > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
> >> > + unsigned int instance = chan_info->instance;
> >> > + unsigned int channel = chan_info->channel;
> >> > + unsigned long flags;
> >> > + void __iomem *base;
> >> > +
> >> > + base = mdev->base + (instance * sizeof(u32));
> >> > +
> >> Maybe have something simpler like MBOX_BASE(instance)? Or some inline
> >> function to avoid this 5-lines ritual?
> >
> > I've checked and we can't do this, as the we need most (all?) of the
> > intermediary variables too. No ritual just to get the final variable
> > for instance.
> >
> OK. How about ?
> #define MBOX_BASE(m, n) ((m)->base + (n) * 4)
> void __iomem *base = MBOX_BASE(mdev, instance);
Oh, those 5 lines. I thought you meant:
struct sti_channel *chan_info = chan->con_priv;
struct sti_mbox_device *mdev = chan_info->mdev;
unsigned int instance = chan_info->instance;
base = mdev->base + (instance * sizeof(u32));
... which is why I said that the intermediary variables are required.
Well, I 'can' do that, but it seems to be unnecessarily obfuscating
what's going on and doesn't actually save any lines.
It's not a point that I consider arguing over though, so if you want
me to do it, I will. You have the final say here.
> >> > + spin_lock_irqsave(&sti_mbox_chan_lock, flags);
> >> > + mdev->enabled[instance] |= BIT(channel);
> >> > + writel_relaxed(BIT(channel), base + pdata->ena_set);
> >> > + spin_unlock_irqrestore(&sti_mbox_chan_lock, flags);
> >> >
> >> You don't need locking for SET/CLR type registers which are meant for
> >> when they could be accessed by processors that can not share a lock.
> >> So maybe drop the lock here and elsewhere.
> >
> > From what I can gather, I think we need this locking. What happens if
> > we get scheduled between setting the enabled bit in our cache and
> > actually setting the ena_set bit? We would be out of sync.
> >
> IIU what you mean... can't that still happen because of the _relaxed()?
Not sure what you mean. The _relaxed variant merely omit some IO
barriers.
> Anyways my point was about set/clr type regs. And you may look at if
> channel really needs disabling while interrupts are handled? I think
> it shouldn't because either it is going to be a to-fro communication
> or random broadcasts without any guarantee of reception. But of course
> you would know better your platform.
I'd certainly feel more comfortable if we kept this part of the
semantics, as it's how the platform experts at ST originally wrote the
driver, and they know a lot more about the protocols used than I.
> BTW sti_mbox_channel_is_enabled() also needs to have locking around
> enabled[] if you want to keep it.
Done.
> And maybe embed sti_mbox_chan_lock inside sti_mbox_device.
Not sure this is required. I can find >600 instances of others using
spinlocks as static globals.
> >> Doesn't mbox->chans[i].con_priv need some locking here?
> >
> > Not that I can see. Would you mind explaining further please?
> >
> Not anymore, after the clarification that we don't need a 'break' statement.
Great, thanks.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Thu, 23 Jul 2015, Alexey Klimov wrote:
> On Fri, Jul 17, 2015 at 3:04 PM, Lee Jones <[email protected]> wrote:
> > ST's platforms currently support a maximum of 5 Mailboxes, one for
> > each of the supported co-processors situated on the platform. Each
> > Mailbox is divided up into 4 instances which consist of 32 channels.
> > Messages are passed between the application and co-processors using
> > shared memory areas. It is the Client's responsibility to manage
> > these areas.
> >
> > Signed-off-by: Lee Jones <[email protected]>
> > ---
> > drivers/mailbox/Kconfig | 7 +
> > drivers/mailbox/Makefile | 2 +
> > drivers/mailbox/mailbox-sti.c | 562 ++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 571 insertions(+)
> > create mode 100644 drivers/mailbox/mailbox-sti.c
>
> [..]
>
> > +static irqreturn_t sti_mbox_thread_handler(int irq, void *data)
> > +{
> > + struct sti_mbox_device *mdev = data;
> > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
> > + struct mbox_chan *chan;
> > + unsigned int instance;
> > +
> > + for (instance = 0; instance < pdata->num_inst; instance++) {
> > +keep_looking:
> > + chan = sti_mbox_irq_to_channel(mdev, instance);
> > + if (!chan)
> > + continue;
> > +
> > + mbox_chan_received_data(chan, NULL);
> > + sti_mbox_clear_irq(chan);
> > + sti_mbox_enable_channel(chan);
> > + goto keep_looking;
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t sti_mbox_irq_handler(int irq, void *data)
> > +{
> > + struct sti_mbox_device *mdev = data;
> > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
> > + struct sti_channel *chan_info;
> > + struct mbox_chan *chan;
> > + unsigned int instance;
> > + int ret = IRQ_NONE;
> > +
> > + for (instance = 0; instance < pdata->num_inst; instance++) {
> > + chan = sti_mbox_irq_to_channel(mdev, instance);
> > + if (!chan)
> > + continue;
> > + chan_info = chan->con_priv;
> > +
> > + if (!sti_mbox_channel_is_enabled(chan)) {
> > + dev_warn(mdev->dev,
> > + "Unexpected IRQ: %s\n"
> > + " instance: %d: channel: %d [enabled: %x]\n",
> > + mdev->name, chan_info->instance,
> > + chan_info->channel, mdev->enabled[instance]);
> > + ret = IRQ_HANDLED;
> > + continue;
> > + }
> > +
> > + sti_mbox_disable_channel(chan);
> > + ret = IRQ_WAKE_THREAD;
> > + }
> > +
> > + if (ret == IRQ_NONE)
> > + dev_err(mdev->dev, "Spurious IRQ - was a channel requested?\n");
> > +
> > + return ret;
> > +}
>
> With such usage of ret variable can it happen that handling of last
> but one channel/instance will set ret to IRQ_WAKE_THREAD and at the
> same time handling of last channel/instance will set ret to
> IRQ_HANDLED during iteration loop and finally generic subsystem will
> not wake up thread handler because it will receive IRQ_HANDLED?
> Just checking.
Yes, I guess that it theoretically possible. Now fixed.
Thanks for the spot.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Fri, Jul 24, 2015 at 3:06 PM, Lee Jones <[email protected]> wrote:
> On Thu, 23 Jul 2015, Jassi Brar wrote:
>
>> >> > +static void sti_mbox_enable_channel(struct mbox_chan *chan)
>> >> > +{
>> >> > + struct sti_channel *chan_info = chan->con_priv;
>> >> > + struct sti_mbox_device *mdev = chan_info->mdev;
>> >> > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
>> >> > + unsigned int instance = chan_info->instance;
>> >> > + unsigned int channel = chan_info->channel;
>> >> > + unsigned long flags;
>> >> > + void __iomem *base;
>> >> > +
>> >> > + base = mdev->base + (instance * sizeof(u32));
>> >> > +
>> >> Maybe have something simpler like MBOX_BASE(instance)? Or some inline
>> >> function to avoid this 5-lines ritual?
>> >
>> > I've checked and we can't do this, as the we need most (all?) of the
>> > intermediary variables too. No ritual just to get the final variable
>> > for instance.
>> >
>> OK. How about ?
>> #define MBOX_BASE(m, n) ((m)->base + (n) * 4)
>> void __iomem *base = MBOX_BASE(mdev, instance);
>
> Oh, those 5 lines. I thought you meant:
>
> struct sti_channel *chan_info = chan->con_priv;
> struct sti_mbox_device *mdev = chan_info->mdev;
> unsigned int instance = chan_info->instance;
> base = mdev->base + (instance * sizeof(u32));
>
> ... which is why I said that the intermediary variables are required.
>
> Well, I 'can' do that, but it seems to be unnecessarily obfuscating
> what's going on and doesn't actually save any lines.
>
> It's not a point that I consider arguing over though, so if you want
> me to do it, I will. You have the final say here.
>
The macro seems tidier. Just a nit.
>> >> > + spin_lock_irqsave(&sti_mbox_chan_lock, flags);
>> >> > + mdev->enabled[instance] |= BIT(channel);
>> >> > + writel_relaxed(BIT(channel), base + pdata->ena_set);
>> >> > + spin_unlock_irqrestore(&sti_mbox_chan_lock, flags);
>> >> >
>> >> You don't need locking for SET/CLR type registers which are meant for
>> >> when they could be accessed by processors that can not share a lock.
>> >> So maybe drop the lock here and elsewhere.
>> >
>> > From what I can gather, I think we need this locking. What happens if
>> > we get scheduled between setting the enabled bit in our cache and
>> > actually setting the ena_set bit? We would be out of sync.
>> >
>> IIU what you mean... can't that still happen because of the _relaxed()?
>
> Not sure what you mean. The _relaxed variant merely omit some IO
> barriers.
>
By the time you exit the spinlock the write may still haven't been
effected. Maybe use writel() there.
>> And maybe embed sti_mbox_chan_lock inside sti_mbox_device.
>
> Not sure this is required. I can find >600 instances of others using
> spinlocks as static globals.
>
And there should be >600 instances of *static* globals that are
protected by some static spinlock ;)
Here the static sti_mbox_chan_lock protects sti_mbox_device which is
allocated during probe. I hope you agree that the standard practice is
to make the lock a member of the same structure that it protects.
Otherwise it gives the wrong impression that the same lock will be
used for any number of allocated mailbox instances.
cheers.