[PATCH v2 0/2] mailbox: introduce STMicroelectronics STM32 IPCC driver

[PATCH v2 0/2] mailbox: introduce STMicroelectronics STM32 IPCC driver

[Fabien Dessenne]

The STMicroelectronics STM32 Inter-Processor Communication Controller
(IPCC) is used for communicating data between two processors.
It provides a non blocking signaling mechanism to post and retrieve
communication data in an atomic way.

Changes since v2:
- update bindings and driver according to Rob's comments:
    - change compatible property to "st,stm32mp1-ipcc"
    - change "st,proc_id" property to "st,proc-id"
    - define all interrupts as mandatory

Fabien Dessenne (2):
  dt-bindings: mailbox: add STMicroelectronics STM32 IPCC binding
  mailbox: add STMicroelectronics STM32 IPCC driver

 .../devicetree/bindings/mailbox/stm32-ipcc.txt     |  47 +++
 drivers/mailbox/Kconfig                            |   8 +
 drivers/mailbox/Makefile                           |   2 +
 drivers/mailbox/stm32-ipcc.c                       | 403 +++++++++++++++++++++
 4 files changed, 460 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/stm32-ipcc.txt
 create mode 100644 drivers/mailbox/stm32-ipcc.c

-- 
2.7.4

[PATCH v2 1/2] dt-bindings: mailbox: add STMicroelectronics STM32 IPCC binding

[Fabien Dessenne]

Add a binding for the STMicroelectronics STM32 IPCC block exposing a
mailbox mechanism between two processors.

Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
---
 .../devicetree/bindings/mailbox/stm32-ipcc.txt     | 47 ++++++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/stm32-ipcc.txt

diff --git a/Documentation/devicetree/bindings/mailbox/stm32-ipcc.txt b/Documentation/devicetree/bindings/mailbox/stm32-ipcc.txt
new file mode 100644
index 0000000..1d2b7fe
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/stm32-ipcc.txt
@@ -0,0 +1,47 @@
+* STMicroelectronics STM32 IPCC (Inter-Processor Communication Controller)
+
+The IPCC block provides a non blocking signaling mechanism to post and
+retrieve messages in an atomic way between two processors.
+It provides the signaling for N bidirectionnal channels. The number of channels
+(N) can be read from a dedicated register.
+
+Required properties:
+- compatible:   Must be "st,stm32mp1-ipcc"
+- reg:          Register address range (base address and length)
+- st,proc-id:   Processor id using the mailbox (0 or 1)
+- clocks:       Input clock
+- interrupt-names: List of names for the interrupts described by the interrupt
+                   property. Must contain the following entries:
+                   - "rx"
+                   - "tx"
+                   - "wakeup"
+- interrupts:   Interrupt specifiers for "rx channel occupied", "tx channel
+                free" and "system wakeup".
+- #mbox-cells:  Number of cells required for the mailbox specifier. Must be 1.
+                The data contained in the mbox specifier of the "mboxes"
+                property in the client node is the mailbox channel index.
+
+Optional properties:
+- wakeup-source: Flag to indicate whether this device can wake up the system
+
+
+
+Example:
+	ipcc: mailbox@4c001000 {
+		compatible = "st,stm32mp1-ipcc";
+		#mbox-cells = <1>;
+		reg = <0x4c001000 0x400>;
+		st,proc-id = <0>;
+		interrupts-extended = <&intc GIC_SPI 100 IRQ_TYPE_NONE>,
+				      <&intc GIC_SPI 101 IRQ_TYPE_NONE>,
+				      <&aiec 62 1>;
+		interrupt-names = "rx", "tx", "wakeup";
+		clocks = <&rcc_clk IPCC>;
+		wakeup-source;
+	}
+
+Client:
+	mbox_test {
+		...
+		mboxes = <&ipcc 0>, <&ipcc 1>;
+	};
-- 
2.7.4

Re: [PATCH v2 1/2] dt-bindings: mailbox: add STMicroelectronics STM32 IPCC binding

[Rob Herring]

On Mon, Mar 12, 2018 at 11:58:26AM +0100, Fabien Dessenne wrote:
> Add a binding for the STMicroelectronics STM32 IPCC block exposing a
> mailbox mechanism between two processors.
> 
> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> ---
>  .../devicetree/bindings/mailbox/stm32-ipcc.txt     | 47 ++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/stm32-ipcc.txt

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v2 2/2] mailbox: add STMicroelectronics STM32 IPCC driver

[Fabien Dessenne]

The STMicroelectronics STM32 Inter-Processor Communication Controller
(IPCC) is used for communicating data between two processors.
It provides a non blocking signaling mechanism to post and retrieve
communication data in an atomic way.

Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
---
 drivers/mailbox/Kconfig      |   8 +
 drivers/mailbox/Makefile     |   2 +
 drivers/mailbox/stm32-ipcc.c | 403 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 413 insertions(+)
 create mode 100644 drivers/mailbox/stm32-ipcc.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index ba2f152..d7581f0 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -171,4 +171,12 @@ config BCM_FLEXRM_MBOX
 	  Mailbox implementation of the Broadcom FlexRM ring manager,
 	  which provides access to various offload engines on Broadcom
 	  SoCs. Say Y here if you want to use the Broadcom FlexRM.
+
+config STM32_IPCC
+	tristate "STM32 IPCC Mailbox"
+	depends on MACH_STM32MP157
+	help
+	  Mailbox implementation for STMicroelectonics STM32 family chips
+	  with hardware for Inter-Processor Communication Controller (IPCC)
+	  between processors. Say Y here if you want to have this support.
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 4896f8d..7ea9654 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -36,3 +36,5 @@ obj-$(CONFIG_BCM_FLEXRM_MBOX)	+= bcm-flexrm-mailbox.o
 obj-$(CONFIG_QCOM_APCS_IPC)	+= qcom-apcs-ipc-mailbox.o
 
 obj-$(CONFIG_TEGRA_HSP_MBOX)	+= tegra-hsp.o
+
+obj-$(CONFIG_STM32_IPCC) 	+= stm32-ipcc.o
diff --git a/drivers/mailbox/stm32-ipcc.c b/drivers/mailbox/stm32-ipcc.c
new file mode 100644
index 0000000..fa3cc59
--- /dev/null
+++ b/drivers/mailbox/stm32-ipcc.c
@@ -0,0 +1,403 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) STMicroelectronics 2018 - All Rights Reserved
+ * Authors: Ludovic Barre <ludovic.barre@st.com> for STMicroelectronics.
+ *          Fabien Dessenne <fabien.dessenne@st.com> for STMicroelectronics.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/pm_wakeirq.h>
+
+#define IPCC_XCR		0x000
+#define XCR_RXOIE		BIT(0)
+#define XCR_TXOIE		BIT(16)
+
+#define IPCC_XMR		0x004
+#define IPCC_XSCR		0x008
+#define IPCC_XTOYSR		0x00c
+
+#define IPCC_PROC_OFFST		0x010
+
+#define IPCC_HWCFGR		0x3f0
+#define IPCFGR_CHAN_MASK	GENMASK(7, 0)
+
+#define IPCC_VER		0x3f4
+#define VER_MINREV_MASK		GENMASK(3, 0)
+#define VER_MAJREV_MASK		GENMASK(7, 4)
+
+#define RX_BIT_MASK		GENMASK(15, 0)
+#define RX_BIT_CHAN(chan)	BIT(chan)
+#define TX_BIT_SHIFT		16
+#define TX_BIT_MASK		GENMASK(31, 16)
+#define TX_BIT_CHAN(chan)	BIT(TX_BIT_SHIFT + (chan))
+
+#define STM32_MAX_PROCS		2
+
+enum {
+	IPCC_IRQ_RX,
+	IPCC_IRQ_TX,
+	IPCC_IRQ_NUM,
+};
+
+struct stm32_ipcc {
+	struct mbox_controller controller;
+	void __iomem *reg_base;
+	void __iomem *reg_proc;
+	struct clk *clk;
+	int irqs[IPCC_IRQ_NUM];
+	int wkp;
+	u32 proc_id;
+	u32 n_chans;
+	u32 xcr;
+	u32 xmr;
+};
+
+static inline void stm32_ipcc_set_bits(void __iomem *reg, u32 mask)
+{
+	writel_relaxed(readl_relaxed(reg) | mask, reg);
+}
+
+static inline void stm32_ipcc_clr_bits(void __iomem *reg, u32 mask)
+{
+	writel_relaxed(readl_relaxed(reg) & ~mask, reg);
+}
+
+static irqreturn_t stm32_ipcc_rx_irq(int irq, void *data)
+{
+	struct stm32_ipcc *ipcc = data;
+	struct device *dev = ipcc->controller.dev;
+	u32 status, mr, tosr, chan;
+	irqreturn_t ret = IRQ_NONE;
+	int proc_offset;
+
+	/* read 'channel occupied' status from other proc */
+	proc_offset = ipcc->proc_id ? -IPCC_PROC_OFFST : IPCC_PROC_OFFST;
+	tosr = readl_relaxed(ipcc->reg_proc + proc_offset + IPCC_XTOYSR);
+	mr = readl_relaxed(ipcc->reg_proc + IPCC_XMR);
+
+	/* search for unmasked 'channel occupied' */
+	status = tosr & FIELD_GET(RX_BIT_MASK, ~mr);
+
+	for (chan = 0; chan < ipcc->n_chans; chan++) {
+		if (!(status & (1 << chan)))
+			continue;
+
+		dev_dbg(dev, "%s: chan:%d rx\n", __func__, chan);
+
+		mbox_chan_received_data(&ipcc->controller.chans[chan], NULL);
+
+		stm32_ipcc_set_bits(ipcc->reg_proc + IPCC_XSCR,
+				    RX_BIT_CHAN(chan));
+
+		ret = IRQ_HANDLED;
+	}
+
+	return ret;
+}
+
+static irqreturn_t stm32_ipcc_tx_irq(int irq, void *data)
+{
+	struct stm32_ipcc *ipcc = data;
+	struct device *dev = ipcc->controller.dev;
+	u32 status, mr, tosr, chan;
+	irqreturn_t ret = IRQ_NONE;
+
+	tosr = readl_relaxed(ipcc->reg_proc + IPCC_XTOYSR);
+	mr = readl_relaxed(ipcc->reg_proc + IPCC_XMR);
+
+	/* search for unmasked 'channel free' */
+	status = ~tosr & FIELD_GET(TX_BIT_MASK, ~mr);
+
+	for (chan = 0; chan < ipcc->n_chans ; chan++) {
+		if (!(status & (1 << chan)))
+			continue;
+
+		dev_dbg(dev, "%s: chan:%d tx\n", __func__, chan);
+
+		/* mask 'tx channel free' interrupt */
+		stm32_ipcc_set_bits(ipcc->reg_proc + IPCC_XMR,
+				    TX_BIT_CHAN(chan));
+
+		mbox_chan_txdone(&ipcc->controller.chans[chan], 0);
+
+		ret = IRQ_HANDLED;
+	}
+
+	return ret;
+}
+
+static int stm32_ipcc_send_data(struct mbox_chan *link, void *data)
+{
+	unsigned int chan = (unsigned int)link->con_priv;
+	struct stm32_ipcc *ipcc = container_of(link->mbox, struct stm32_ipcc,
+					       controller);
+
+	dev_dbg(ipcc->controller.dev, "%s: chan:%d\n", __func__, chan);
+
+	/* set channel n occupied */
+	stm32_ipcc_set_bits(ipcc->reg_proc + IPCC_XSCR, TX_BIT_CHAN(chan));
+
+	/* unmask 'tx channel free' interrupt */
+	stm32_ipcc_clr_bits(ipcc->reg_proc + IPCC_XMR, TX_BIT_CHAN(chan));
+
+	return 0;
+}
+
+static int stm32_ipcc_startup(struct mbox_chan *link)
+{
+	unsigned int chan = (unsigned int)link->con_priv;
+	struct stm32_ipcc *ipcc = container_of(link->mbox, struct stm32_ipcc,
+					       controller);
+	int ret;
+
+	ret = clk_prepare_enable(ipcc->clk);
+	if (ret) {
+		dev_err(ipcc->controller.dev, "can not enable the clock\n");
+		return ret;
+	}
+
+	/* unmask 'rx channel occupied' interrupt */
+	stm32_ipcc_clr_bits(ipcc->reg_proc + IPCC_XMR, RX_BIT_CHAN(chan));
+
+	return 0;
+}
+
+static void stm32_ipcc_shutdown(struct mbox_chan *link)
+{
+	unsigned int chan = (unsigned int)link->con_priv;
+	struct stm32_ipcc *ipcc = container_of(link->mbox, struct stm32_ipcc,
+					       controller);
+
+	/* mask rx/tx interrupt */
+	stm32_ipcc_set_bits(ipcc->reg_proc + IPCC_XMR,
+			    RX_BIT_CHAN(chan) | TX_BIT_CHAN(chan));
+
+	clk_disable_unprepare(ipcc->clk);
+}
+
+static const struct mbox_chan_ops stm32_ipcc_ops = {
+	.send_data	= stm32_ipcc_send_data,
+	.startup	= stm32_ipcc_startup,
+	.shutdown	= stm32_ipcc_shutdown,
+};
+
+static int stm32_ipcc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct stm32_ipcc *ipcc;
+	struct resource *res;
+	unsigned int i;
+	int ret;
+	u32 ip_ver;
+	static const char * const irq_name[] = {"rx", "tx"};
+	irq_handler_t irq_thread[] = {stm32_ipcc_rx_irq, stm32_ipcc_tx_irq};
+
+	if (!np) {
+		dev_err(dev, "No DT found\n");
+		return -ENODEV;
+	}
+
+	ipcc = devm_kzalloc(dev, sizeof(*ipcc), GFP_KERNEL);
+	if (!ipcc)
+		return -ENOMEM;
+
+	/* proc_id */
+	if (of_property_read_u32(np, "st,proc-id", &ipcc->proc_id)) {
+		dev_err(dev, "Missing st,proc-id\n");
+		return -ENODEV;
+	}
+
+	if (ipcc->proc_id >= STM32_MAX_PROCS) {
+		dev_err(dev, "Invalid proc_id (%d)\n", ipcc->proc_id);
+		return -EINVAL;
+	}
+
+	/* regs */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	ipcc->reg_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(ipcc->reg_base))
+		return PTR_ERR(ipcc->reg_base);
+
+	ipcc->reg_proc = ipcc->reg_base + ipcc->proc_id * IPCC_PROC_OFFST;
+
+	/* clock */
+	ipcc->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(ipcc->clk))
+		return PTR_ERR(ipcc->clk);
+
+	ret = clk_prepare_enable(ipcc->clk);
+	if (ret) {
+		dev_err(dev, "can not enable the clock\n");
+		return ret;
+	}
+
+	/* irq */
+	for (i = 0; i < IPCC_IRQ_NUM; i++) {
+		ipcc->irqs[i] = of_irq_get_byname(dev->of_node, irq_name[i]);
+		if (ipcc->irqs[i] < 0) {
+			dev_err(dev, "no IRQ specified %s\n", irq_name[i]);
+			ret = ipcc->irqs[i];
+			goto err_clk;
+		}
+
+		ret = devm_request_threaded_irq(dev, ipcc->irqs[i], NULL,
+						irq_thread[i], IRQF_ONESHOT,
+						dev_name(dev), ipcc);
+		if (ret) {
+			dev_err(dev, "failed to request irq %d (%d)\n", i, ret);
+			goto err_clk;
+		}
+	}
+
+	/* mask and enable rx/tx irq */
+	stm32_ipcc_set_bits(ipcc->reg_proc + IPCC_XMR,
+			    RX_BIT_MASK | TX_BIT_MASK);
+	stm32_ipcc_set_bits(ipcc->reg_proc + IPCC_XCR, XCR_RXOIE | XCR_TXOIE);
+
+	/* wakeup */
+	if (of_property_read_bool(np, "wakeup-source")) {
+		ipcc->wkp = of_irq_get_byname(dev->of_node, "wakeup");
+		if (ipcc->wkp < 0) {
+			dev_err(dev, "could not get wakeup IRQ\n");
+			ret = ipcc->wkp;
+			goto err_clk;
+		}
+
+		device_init_wakeup(dev, true);
+		ret = dev_pm_set_dedicated_wake_irq(dev, ipcc->wkp);
+		if (ret) {
+			dev_err(dev, "Failed to set wake up irq\n");
+			goto err_init_wkp;
+		}
+	} else {
+		device_init_wakeup(dev, false);
+	}
+
+	/* mailbox controller */
+	ipcc->n_chans = readl_relaxed(ipcc->reg_base + IPCC_HWCFGR);
+	ipcc->n_chans &= IPCFGR_CHAN_MASK;
+
+	ipcc->controller.dev = dev;
+	ipcc->controller.txdone_irq = true;
+	ipcc->controller.ops = &stm32_ipcc_ops;
+	ipcc->controller.num_chans = ipcc->n_chans;
+	ipcc->controller.chans = devm_kcalloc(dev, ipcc->controller.num_chans,
+					      sizeof(*ipcc->controller.chans),
+					      GFP_KERNEL);
+	if (!ipcc->controller.chans) {
+		ret = -ENOMEM;
+		goto err_irq_wkp;
+	}
+
+	for (i = 0; i < ipcc->controller.num_chans; i++)
+		ipcc->controller.chans[i].con_priv = (void *)i;
+
+	ret = mbox_controller_register(&ipcc->controller);
+	if (ret)
+		goto err_irq_wkp;
+
+	platform_set_drvdata(pdev, ipcc);
+
+	ip_ver = readl_relaxed(ipcc->reg_base + IPCC_VER);
+
+	dev_info(dev, "ipcc rev:%ld.%ld enabled, %d chans, proc %d\n",
+		 FIELD_GET(VER_MAJREV_MASK, ip_ver),
+		 FIELD_GET(VER_MINREV_MASK, ip_ver),
+		 ipcc->controller.num_chans, ipcc->proc_id);
+
+	clk_disable_unprepare(ipcc->clk);
+	return 0;
+
+err_irq_wkp:
+	if (ipcc->wkp)
+		dev_pm_clear_wake_irq(dev);
+err_init_wkp:
+	device_init_wakeup(dev, false);
+err_clk:
+	clk_disable_unprepare(ipcc->clk);
+	return ret;
+}
+
+static int stm32_ipcc_remove(struct platform_device *pdev)
+{
+	struct stm32_ipcc *ipcc = platform_get_drvdata(pdev);
+
+	mbox_controller_unregister(&ipcc->controller);
+
+	if (ipcc->wkp)
+		dev_pm_clear_wake_irq(&pdev->dev);
+
+	device_init_wakeup(&pdev->dev, false);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static void stm32_ipcc_set_irq_wake(struct device *dev, bool enable)
+{
+	struct stm32_ipcc *ipcc = dev_get_drvdata(dev);
+	unsigned int i;
+
+	if (device_may_wakeup(dev))
+		for (i = 0; i < IPCC_IRQ_NUM; i++)
+			irq_set_irq_wake(ipcc->irqs[i], enable);
+}
+
+static int stm32_ipcc_suspend(struct device *dev)
+{
+	struct stm32_ipcc *ipcc = dev_get_drvdata(dev);
+
+	ipcc->xmr = readl_relaxed(ipcc->reg_proc + IPCC_XMR);
+	ipcc->xcr = readl_relaxed(ipcc->reg_proc + IPCC_XCR);
+
+	stm32_ipcc_set_irq_wake(dev, true);
+
+	return 0;
+}
+
+static int stm32_ipcc_resume(struct device *dev)
+{
+	struct stm32_ipcc *ipcc = dev_get_drvdata(dev);
+
+	stm32_ipcc_set_irq_wake(dev, false);
+
+	writel_relaxed(ipcc->xmr, ipcc->reg_proc + IPCC_XMR);
+	writel_relaxed(ipcc->xcr, ipcc->reg_proc + IPCC_XCR);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(stm32_ipcc_pm_ops,
+			 stm32_ipcc_suspend, stm32_ipcc_resume);
+
+static const struct of_device_id stm32_ipcc_of_match[] = {
+	{ .compatible = "st,stm32mp1-ipcc" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, stm32_ipcc_of_match);
+
+static struct platform_driver stm32_ipcc_driver = {
+	.driver = {
+		.name = "stm32-ipcc",
+		.owner = THIS_MODULE,
+		.pm = &stm32_ipcc_pm_ops,
+		.of_match_table = stm32_ipcc_of_match,
+	},
+	.probe		= stm32_ipcc_probe,
+	.remove		= stm32_ipcc_remove,
+};
+
+module_platform_driver(stm32_ipcc_driver);
+
+MODULE_AUTHOR("Ludovic Barre <ludovic.barre@st.com>");
+MODULE_AUTHOR("Fabien Dessenne <fabien.dessenne@st.com>");
+MODULE_DESCRIPTION("STM32 IPCC driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [v2,2/2] mailbox: add STMicroelectronics STM32 IPCC driver

[Fabien DESSENNE]

Just a gentle ping ... or have I missed out on a reply?


On 12/03/18 11:58, Fabien DESSENNE wrote:
> The STMicroelectronics STM32 Inter-Processor Communication Controller
> (IPCC) is used for communicating data between two processors.
> It provides a non blocking signaling mechanism to post and retrieve
> communication data in an atomic way.
>
> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> ---
>   drivers/mailbox/Kconfig      |   8 +
>   drivers/mailbox/Makefile     |   2 +
>   drivers/mailbox/stm32-ipcc.c | 403 +++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 413 insertions(+)
>   create mode 100644 drivers/mailbox/stm32-ipcc.c
>
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index ba2f152..d7581f0 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -171,4 +171,12 @@ config BCM_FLEXRM_MBOX
>   	  Mailbox implementation of the Broadcom FlexRM ring manager,
>   	  which provides access to various offload engines on Broadcom
>   	  SoCs. Say Y here if you want to use the Broadcom FlexRM.
> +
> +config STM32_IPCC
> +	tristate "STM32 IPCC Mailbox"
> +	depends on MACH_STM32MP157
> +	help
> +	  Mailbox implementation for STMicroelectonics STM32 family chips
> +	  with hardware for Inter-Processor Communication Controller (IPCC)
> +	  between processors. Say Y here if you want to have this support.
>   endif
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index 4896f8d..7ea9654 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -36,3 +36,5 @@ obj-$(CONFIG_BCM_FLEXRM_MBOX)	+= bcm-flexrm-mailbox.o
>   obj-$(CONFIG_QCOM_APCS_IPC)	+= qcom-apcs-ipc-mailbox.o
>   
>   obj-$(CONFIG_TEGRA_HSP_MBOX)	+= tegra-hsp.o
> +
> +obj-$(CONFIG_STM32_IPCC) 	+= stm32-ipcc.o
> diff --git a/drivers/mailbox/stm32-ipcc.c b/drivers/mailbox/stm32-ipcc.c
> new file mode 100644
> index 0000000..fa3cc59
> --- /dev/null
> +++ b/drivers/mailbox/stm32-ipcc.c
> @@ -0,0 +1,403 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) STMicroelectronics 2018 - All Rights Reserved
> + * Authors: Ludovic Barre <ludovic.barre@st.com> for STMicroelectronics.
> + *          Fabien Dessenne <fabien.dessenne@st.com> for STMicroelectronics.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_wakeirq.h>
> +
> +#define IPCC_XCR		0x000
> +#define XCR_RXOIE		BIT(0)
> +#define XCR_TXOIE		BIT(16)
> +
> +#define IPCC_XMR		0x004
> +#define IPCC_XSCR		0x008
> +#define IPCC_XTOYSR		0x00c
> +
> +#define IPCC_PROC_OFFST		0x010
> +
> +#define IPCC_HWCFGR		0x3f0
> +#define IPCFGR_CHAN_MASK	GENMASK(7, 0)
> +
> +#define IPCC_VER		0x3f4
> +#define VER_MINREV_MASK		GENMASK(3, 0)
> +#define VER_MAJREV_MASK		GENMASK(7, 4)
> +
> +#define RX_BIT_MASK		GENMASK(15, 0)
> +#define RX_BIT_CHAN(chan)	BIT(chan)
> +#define TX_BIT_SHIFT		16
> +#define TX_BIT_MASK		GENMASK(31, 16)
> +#define TX_BIT_CHAN(chan)	BIT(TX_BIT_SHIFT + (chan))
> +
> +#define STM32_MAX_PROCS		2
> +
> +enum {
> +	IPCC_IRQ_RX,
> +	IPCC_IRQ_TX,
> +	IPCC_IRQ_NUM,
> +};
> +
> +struct stm32_ipcc {
> +	struct mbox_controller controller;
> +	void __iomem *reg_base;
> +	void __iomem *reg_proc;
> +	struct clk *clk;
> +	int irqs[IPCC_IRQ_NUM];
> +	int wkp;
> +	u32 proc_id;
> +	u32 n_chans;
> +	u32 xcr;
> +	u32 xmr;
> +};
> +
> +static inline void stm32_ipcc_set_bits(void __iomem *reg, u32 mask)
> +{
> +	writel_relaxed(readl_relaxed(reg) | mask, reg);
> +}
> +
> +static inline void stm32_ipcc_clr_bits(void __iomem *reg, u32 mask)
> +{
> +	writel_relaxed(readl_relaxed(reg) & ~mask, reg);
> +}
> +
> +static irqreturn_t stm32_ipcc_rx_irq(int irq, void *data)
> +{
> +	struct stm32_ipcc *ipcc = data;
> +	struct device *dev = ipcc->controller.dev;
> +	u32 status, mr, tosr, chan;
> +	irqreturn_t ret = IRQ_NONE;
> +	int proc_offset;
> +
> +	/* read 'channel occupied' status from other proc */
> +	proc_offset = ipcc->proc_id ? -IPCC_PROC_OFFST : IPCC_PROC_OFFST;
> +	tosr = readl_relaxed(ipcc->reg_proc + proc_offset + IPCC_XTOYSR);
> +	mr = readl_relaxed(ipcc->reg_proc + IPCC_XMR);
> +
> +	/* search for unmasked 'channel occupied' */
> +	status = tosr & FIELD_GET(RX_BIT_MASK, ~mr);
> +
> +	for (chan = 0; chan < ipcc->n_chans; chan++) {
> +		if (!(status & (1 << chan)))
> +			continue;
> +
> +		dev_dbg(dev, "%s: chan:%d rx\n", __func__, chan);
> +
> +		mbox_chan_received_data(&ipcc->controller.chans[chan], NULL);
> +
> +		stm32_ipcc_set_bits(ipcc->reg_proc + IPCC_XSCR,
> +				    RX_BIT_CHAN(chan));
> +
> +		ret = IRQ_HANDLED;
> +	}
> +
> +	return ret;
> +}
> +
> +static irqreturn_t stm32_ipcc_tx_irq(int irq, void *data)
> +{
> +	struct stm32_ipcc *ipcc = data;
> +	struct device *dev = ipcc->controller.dev;
> +	u32 status, mr, tosr, chan;
> +	irqreturn_t ret = IRQ_NONE;
> +
> +	tosr = readl_relaxed(ipcc->reg_proc + IPCC_XTOYSR);
> +	mr = readl_relaxed(ipcc->reg_proc + IPCC_XMR);
> +
> +	/* search for unmasked 'channel free' */
> +	status = ~tosr & FIELD_GET(TX_BIT_MASK, ~mr);
> +
> +	for (chan = 0; chan < ipcc->n_chans ; chan++) {
> +		if (!(status & (1 << chan)))
> +			continue;
> +
> +		dev_dbg(dev, "%s: chan:%d tx\n", __func__, chan);
> +
> +		/* mask 'tx channel free' interrupt */
> +		stm32_ipcc_set_bits(ipcc->reg_proc + IPCC_XMR,
> +				    TX_BIT_CHAN(chan));
> +
> +		mbox_chan_txdone(&ipcc->controller.chans[chan], 0);
> +
> +		ret = IRQ_HANDLED;
> +	}
> +
> +	return ret;
> +}
> +
> +static int stm32_ipcc_send_data(struct mbox_chan *link, void *data)
> +{
> +	unsigned int chan = (unsigned int)link->con_priv;
> +	struct stm32_ipcc *ipcc = container_of(link->mbox, struct stm32_ipcc,
> +					       controller);
> +
> +	dev_dbg(ipcc->controller.dev, "%s: chan:%d\n", __func__, chan);
> +
> +	/* set channel n occupied */
> +	stm32_ipcc_set_bits(ipcc->reg_proc + IPCC_XSCR, TX_BIT_CHAN(chan));
> +
> +	/* unmask 'tx channel free' interrupt */
> +	stm32_ipcc_clr_bits(ipcc->reg_proc + IPCC_XMR, TX_BIT_CHAN(chan));
> +
> +	return 0;
> +}
> +
> +static int stm32_ipcc_startup(struct mbox_chan *link)
> +{
> +	unsigned int chan = (unsigned int)link->con_priv;
> +	struct stm32_ipcc *ipcc = container_of(link->mbox, struct stm32_ipcc,
> +					       controller);
> +	int ret;
> +
> +	ret = clk_prepare_enable(ipcc->clk);
> +	if (ret) {
> +		dev_err(ipcc->controller.dev, "can not enable the clock\n");
> +		return ret;
> +	}
> +
> +	/* unmask 'rx channel occupied' interrupt */
> +	stm32_ipcc_clr_bits(ipcc->reg_proc + IPCC_XMR, RX_BIT_CHAN(chan));
> +
> +	return 0;
> +}
> +
> +static void stm32_ipcc_shutdown(struct mbox_chan *link)
> +{
> +	unsigned int chan = (unsigned int)link->con_priv;
> +	struct stm32_ipcc *ipcc = container_of(link->mbox, struct stm32_ipcc,
> +					       controller);
> +
> +	/* mask rx/tx interrupt */
> +	stm32_ipcc_set_bits(ipcc->reg_proc + IPCC_XMR,
> +			    RX_BIT_CHAN(chan) | TX_BIT_CHAN(chan));
> +
> +	clk_disable_unprepare(ipcc->clk);
> +}
> +
> +static const struct mbox_chan_ops stm32_ipcc_ops = {
> +	.send_data	= stm32_ipcc_send_data,
> +	.startup	= stm32_ipcc_startup,
> +	.shutdown	= stm32_ipcc_shutdown,
> +};
> +
> +static int stm32_ipcc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct stm32_ipcc *ipcc;
> +	struct resource *res;
> +	unsigned int i;
> +	int ret;
> +	u32 ip_ver;
> +	static const char * const irq_name[] = {"rx", "tx"};
> +	irq_handler_t irq_thread[] = {stm32_ipcc_rx_irq, stm32_ipcc_tx_irq};
> +
> +	if (!np) {
> +		dev_err(dev, "No DT found\n");
> +		return -ENODEV;
> +	}
> +
> +	ipcc = devm_kzalloc(dev, sizeof(*ipcc), GFP_KERNEL);
> +	if (!ipcc)
> +		return -ENOMEM;
> +
> +	/* proc_id */
> +	if (of_property_read_u32(np, "st,proc-id", &ipcc->proc_id)) {
> +		dev_err(dev, "Missing st,proc-id\n");
> +		return -ENODEV;
> +	}
> +
> +	if (ipcc->proc_id >= STM32_MAX_PROCS) {
> +		dev_err(dev, "Invalid proc_id (%d)\n", ipcc->proc_id);
> +		return -EINVAL;
> +	}
> +
> +	/* regs */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	ipcc->reg_base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(ipcc->reg_base))
> +		return PTR_ERR(ipcc->reg_base);
> +
> +	ipcc->reg_proc = ipcc->reg_base + ipcc->proc_id * IPCC_PROC_OFFST;
> +
> +	/* clock */
> +	ipcc->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(ipcc->clk))
> +		return PTR_ERR(ipcc->clk);
> +
> +	ret = clk_prepare_enable(ipcc->clk);
> +	if (ret) {
> +		dev_err(dev, "can not enable the clock\n");
> +		return ret;
> +	}
> +
> +	/* irq */
> +	for (i = 0; i < IPCC_IRQ_NUM; i++) {
> +		ipcc->irqs[i] = of_irq_get_byname(dev->of_node, irq_name[i]);
> +		if (ipcc->irqs[i] < 0) {
> +			dev_err(dev, "no IRQ specified %s\n", irq_name[i]);
> +			ret = ipcc->irqs[i];
> +			goto err_clk;
> +		}
> +
> +		ret = devm_request_threaded_irq(dev, ipcc->irqs[i], NULL,
> +						irq_thread[i], IRQF_ONESHOT,
> +						dev_name(dev), ipcc);
> +		if (ret) {
> +			dev_err(dev, "failed to request irq %d (%d)\n", i, ret);
> +			goto err_clk;
> +		}
> +	}
> +
> +	/* mask and enable rx/tx irq */
> +	stm32_ipcc_set_bits(ipcc->reg_proc + IPCC_XMR,
> +			    RX_BIT_MASK | TX_BIT_MASK);
> +	stm32_ipcc_set_bits(ipcc->reg_proc + IPCC_XCR, XCR_RXOIE | XCR_TXOIE);
> +
> +	/* wakeup */
> +	if (of_property_read_bool(np, "wakeup-source")) {
> +		ipcc->wkp = of_irq_get_byname(dev->of_node, "wakeup");
> +		if (ipcc->wkp < 0) {
> +			dev_err(dev, "could not get wakeup IRQ\n");
> +			ret = ipcc->wkp;
> +			goto err_clk;
> +		}
> +
> +		device_init_wakeup(dev, true);
> +		ret = dev_pm_set_dedicated_wake_irq(dev, ipcc->wkp);
> +		if (ret) {
> +			dev_err(dev, "Failed to set wake up irq\n");
> +			goto err_init_wkp;
> +		}
> +	} else {
> +		device_init_wakeup(dev, false);
> +	}
> +
> +	/* mailbox controller */
> +	ipcc->n_chans = readl_relaxed(ipcc->reg_base + IPCC_HWCFGR);
> +	ipcc->n_chans &= IPCFGR_CHAN_MASK;
> +
> +	ipcc->controller.dev = dev;
> +	ipcc->controller.txdone_irq = true;
> +	ipcc->controller.ops = &stm32_ipcc_ops;
> +	ipcc->controller.num_chans = ipcc->n_chans;
> +	ipcc->controller.chans = devm_kcalloc(dev, ipcc->controller.num_chans,
> +					      sizeof(*ipcc->controller.chans),
> +					      GFP_KERNEL);
> +	if (!ipcc->controller.chans) {
> +		ret = -ENOMEM;
> +		goto err_irq_wkp;
> +	}
> +
> +	for (i = 0; i < ipcc->controller.num_chans; i++)
> +		ipcc->controller.chans[i].con_priv = (void *)i;
> +
> +	ret = mbox_controller_register(&ipcc->controller);
> +	if (ret)
> +		goto err_irq_wkp;
> +
> +	platform_set_drvdata(pdev, ipcc);
> +
> +	ip_ver = readl_relaxed(ipcc->reg_base + IPCC_VER);
> +
> +	dev_info(dev, "ipcc rev:%ld.%ld enabled, %d chans, proc %d\n",
> +		 FIELD_GET(VER_MAJREV_MASK, ip_ver),
> +		 FIELD_GET(VER_MINREV_MASK, ip_ver),
> +		 ipcc->controller.num_chans, ipcc->proc_id);
> +
> +	clk_disable_unprepare(ipcc->clk);
> +	return 0;
> +
> +err_irq_wkp:
> +	if (ipcc->wkp)
> +		dev_pm_clear_wake_irq(dev);
> +err_init_wkp:
> +	device_init_wakeup(dev, false);
> +err_clk:
> +	clk_disable_unprepare(ipcc->clk);
> +	return ret;
> +}
> +
> +static int stm32_ipcc_remove(struct platform_device *pdev)
> +{
> +	struct stm32_ipcc *ipcc = platform_get_drvdata(pdev);
> +
> +	mbox_controller_unregister(&ipcc->controller);
> +
> +	if (ipcc->wkp)
> +		dev_pm_clear_wake_irq(&pdev->dev);
> +
> +	device_init_wakeup(&pdev->dev, false);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static void stm32_ipcc_set_irq_wake(struct device *dev, bool enable)
> +{
> +	struct stm32_ipcc *ipcc = dev_get_drvdata(dev);
> +	unsigned int i;
> +
> +	if (device_may_wakeup(dev))
> +		for (i = 0; i < IPCC_IRQ_NUM; i++)
> +			irq_set_irq_wake(ipcc->irqs[i], enable);
> +}
> +
> +static int stm32_ipcc_suspend(struct device *dev)
> +{
> +	struct stm32_ipcc *ipcc = dev_get_drvdata(dev);
> +
> +	ipcc->xmr = readl_relaxed(ipcc->reg_proc + IPCC_XMR);
> +	ipcc->xcr = readl_relaxed(ipcc->reg_proc + IPCC_XCR);
> +
> +	stm32_ipcc_set_irq_wake(dev, true);
> +
> +	return 0;
> +}
> +
> +static int stm32_ipcc_resume(struct device *dev)
> +{
> +	struct stm32_ipcc *ipcc = dev_get_drvdata(dev);
> +
> +	stm32_ipcc_set_irq_wake(dev, false);
> +
> +	writel_relaxed(ipcc->xmr, ipcc->reg_proc + IPCC_XMR);
> +	writel_relaxed(ipcc->xcr, ipcc->reg_proc + IPCC_XCR);
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(stm32_ipcc_pm_ops,
> +			 stm32_ipcc_suspend, stm32_ipcc_resume);
> +
> +static const struct of_device_id stm32_ipcc_of_match[] = {
> +	{ .compatible = "st,stm32mp1-ipcc" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, stm32_ipcc_of_match);
> +
> +static struct platform_driver stm32_ipcc_driver = {
> +	.driver = {
> +		.name = "stm32-ipcc",
> +		.owner = THIS_MODULE,
> +		.pm = &stm32_ipcc_pm_ops,
> +		.of_match_table = stm32_ipcc_of_match,
> +	},
> +	.probe		= stm32_ipcc_probe,
> +	.remove		= stm32_ipcc_remove,
> +};
> +
> +module_platform_driver(stm32_ipcc_driver);
> +
> +MODULE_AUTHOR("Ludovic Barre <ludovic.barre@st.com>");
> +MODULE_AUTHOR("Fabien Dessenne <fabien.dessenne@st.com>");
> +MODULE_DESCRIPTION("STM32 IPCC driver");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH v2 2/2] mailbox: add STMicroelectronics STM32 IPCC driver

[Jassi Brar]

On Mon, Mar 12, 2018 at 4:28 PM, Fabien Dessenne <fabien.dessenne@st.com> wrote:
....
> +
> +       /* irq */
> +       for (i = 0; i < IPCC_IRQ_NUM; i++) {
> +               ipcc->irqs[i] = of_irq_get_byname(dev->of_node, irq_name[i]);
> +               if (ipcc->irqs[i] < 0) {
> +                       dev_err(dev, "no IRQ specified %s\n", irq_name[i]);
> +                       ret = ipcc->irqs[i];
> +                       goto err_clk;
> +               }
> +
> +               ret = devm_request_threaded_irq(dev, ipcc->irqs[i], NULL,
> +                                               irq_thread[i], IRQF_ONESHOT,
> +                                               dev_name(dev), ipcc);
>
In your interrupt handlers you don't do anything that could block.
Threads only adds some delay to your message handling.
So maybe use devm_request_irq() ?

.......
> +
> +static struct platform_driver stm32_ipcc_driver = {
> +       .driver = {
> +               .name = "stm32-ipcc",
> +               .owner = THIS_MODULE,
>
No need of owner here these days.
And also maybe use readl/writel, instead of _relaxed.

Cheers!

Re: [PATCH v2 2/2] mailbox: add STMicroelectronics STM32 IPCC driver

[Fabien DESSENNE]

Hi


On 05/04/18 11:38, Jassi Brar wrote:
> On Mon, Mar 12, 2018 at 4:28 PM, Fabien Dessenne <fabien.dessenne-qxv4g6HH51o@public.gmane.org> wrote:
> ....
>> +
>> +       /* irq */
>> +       for (i = 0; i < IPCC_IRQ_NUM; i++) {
>> +               ipcc->irqs[i] = of_irq_get_byname(dev->of_node, irq_name[i]);
>> +               if (ipcc->irqs[i] < 0) {
>> +                       dev_err(dev, "no IRQ specified %s\n", irq_name[i]);
>> +                       ret = ipcc->irqs[i];
>> +                       goto err_clk;
>> +               }
>> +
>> +               ret = devm_request_threaded_irq(dev, ipcc->irqs[i], NULL,
>> +                                               irq_thread[i], IRQF_ONESHOT,
>> +                                               dev_name(dev), ipcc);
>>
> In your interrupt handlers you don't do anything that could block.
> Threads only adds some delay to your message handling.
> So maybe use devm_request_irq() ?

The interrupt handlers call mbox_chan_received_data() / 
mbox_chan_txdone(), which call in turn client's rx_callback() / 
tx_done() / tx_prepare() which behavior may be unsafe. Hence, using a 
threaded irq here seems to be a good choice.


>
> .......
>> +
>> +static struct platform_driver stm32_ipcc_driver = {
>> +       .driver = {
>> +               .name = "stm32-ipcc",
>> +               .owner = THIS_MODULE,
>>
> No need of owner here these days.

OK, I will suppress it.

> And also maybe use readl/writel, instead of _relaxed.

The IPCC device is exclusively used on ARM. In ARM architecture, the 
ioremap on devices are strictly ordered and uncached.
In that case, using _relaxed avoids an unneeded cache flush, slightly 
improving performance.

>
> Cheers!

Re: [PATCH v2 2/2] mailbox: add STMicroelectronics STM32 IPCC driver

[Jassi Brar]

On Fri, Apr 6, 2018 at 5:59 PM, Fabien DESSENNE <fabien.dessenne@st.com> wrote:
> Hi
>
>
> On 05/04/18 11:38, Jassi Brar wrote:
>> On Mon, Mar 12, 2018 at 4:28 PM, Fabien Dessenne <fabien.dessenne@st.com> wrote:
>> ....
>>> +
>>> +       /* irq */
>>> +       for (i = 0; i < IPCC_IRQ_NUM; i++) {
>>> +               ipcc->irqs[i] = of_irq_get_byname(dev->of_node, irq_name[i]);
>>> +               if (ipcc->irqs[i] < 0) {
>>> +                       dev_err(dev, "no IRQ specified %s\n", irq_name[i]);
>>> +                       ret = ipcc->irqs[i];
>>> +                       goto err_clk;
>>> +               }
>>> +
>>> +               ret = devm_request_threaded_irq(dev, ipcc->irqs[i], NULL,
>>> +                                               irq_thread[i], IRQF_ONESHOT,
>>> +                                               dev_name(dev), ipcc);
>>>
>> In your interrupt handlers you don't do anything that could block.
>> Threads only adds some delay to your message handling.
>> So maybe use devm_request_irq() ?
>
> The interrupt handlers call mbox_chan_received_data() /
> mbox_chan_txdone(), which call in turn client's rx_callback() /
> tx_done() / tx_prepare() which behavior may be unsafe. Hence, using a
> threaded irq here seems to be a good choice.
>
rx_callback() is supposed to be atomic. So was tx_done() but some
platforms needed preparing for the message to be sent. Your client is
not going to be used by other platforms or even over other
controllers, so if your prepare is NULL/atomic, you should assume
tx_done to be atomic and not lose performace. If time comes to fix it,
we'll move prepare() out of the atomic path.


>> .......
>>> +
>>> +static struct platform_driver stm32_ipcc_driver = {
>>> +       .driver = {
>>> +               .name = "stm32-ipcc",
>>> +               .owner = THIS_MODULE,
>>>
>> No need of owner here these days.
>
> OK, I will suppress it.
>
>> And also maybe use readl/writel, instead of _relaxed.
>
> The IPCC device is exclusively used on ARM. In ARM architecture, the
> ioremap on devices are strictly ordered and uncached.
> In that case, using _relaxed avoids an unneeded cache flush, slightly
> improving performance.
>
Its not the portability, but that the impact is negligible in favor of
_relaxed() version when all you do is just program some registers and
not heavy duty i/o. But I am ok either way.  You'd gain far more
performance handling irqs in non-threaded manner :)

Cheers!

Re: [PATCH v2 2/2] mailbox: add STMicroelectronics STM32 IPCC driver

[Fabien DESSENNE]

On 06/04/18 14:56, Jassi Brar wrote:
> On Fri, Apr 6, 2018 at 5:59 PM, Fabien DESSENNE <fabien.dessenne@st.com> wrote:
>> Hi
>>
>>
>> On 05/04/18 11:38, Jassi Brar wrote:
>>> On Mon, Mar 12, 2018 at 4:28 PM, Fabien Dessenne <fabien.dessenne@st.com> wrote:
>>> ....
>>>> +
>>>> +       /* irq */
>>>> +       for (i = 0; i < IPCC_IRQ_NUM; i++) {
>>>> +               ipcc->irqs[i] = of_irq_get_byname(dev->of_node, irq_name[i]);
>>>> +               if (ipcc->irqs[i] < 0) {
>>>> +                       dev_err(dev, "no IRQ specified %s\n", irq_name[i]);
>>>> +                       ret = ipcc->irqs[i];
>>>> +                       goto err_clk;
>>>> +               }
>>>> +
>>>> +               ret = devm_request_threaded_irq(dev, ipcc->irqs[i], NULL,
>>>> +                                               irq_thread[i], IRQF_ONESHOT,
>>>> +                                               dev_name(dev), ipcc);
>>>>
>>> In your interrupt handlers you don't do anything that could block.
>>> Threads only adds some delay to your message handling.
>>> So maybe use devm_request_irq() ?
>> The interrupt handlers call mbox_chan_received_data() /
>> mbox_chan_txdone(), which call in turn client's rx_callback() /
>> tx_done() / tx_prepare() which behavior may be unsafe. Hence, using a
>> threaded irq here seems to be a good choice.
>>
> rx_callback() is supposed to be atomic.

I am worried with this atomic part (and honestly I did not note that the 
callbacks were expected to be)

In my case, remoteproc->virtio->rpmsg is the mailbox client defining the 
rx_callback.
If I follow your suggestion, I shall make this rx_callback Atomic in 
remoteproc (or in virtio or rpmsg). And this does not seem to be so 
simple (add a worker in the middle of somewhere?). Bjorn, feel free to 
comment this part.

An alternate implementation consists in using a threaded IRQ for the 
mailbox interrupt.
This option is not only simple, but also ensures to split bottom & half 
parts at the irq level which is IMHO a general good practice.

I can see that some mailbox clients implement callbacks that are NOT 
atomic and I suspect this is the reason why some mailbox drivers use 
threaded_irq (rockchip mailbox splits the bottom & half parts).

Would it be acceptable to consider the "atomic client callback" as a 
non-strict rule ?

>   So was tx_done() but some
> platforms needed preparing for the message to be sent. Your client is
> not going to be used by other platforms or even over other
> controllers, so if your prepare is NULL/atomic, you should assume
> tx_done to be atomic and not lose performace. If time comes to fix it,
> we'll move prepare() out of the atomic path.
>
>
>>> .......
>>>> +
>>>> +static struct platform_driver stm32_ipcc_driver = {
>>>> +       .driver = {
>>>> +               .name = "stm32-ipcc",
>>>> +               .owner = THIS_MODULE,
>>>>
>>> No need of owner here these days.
>> OK, I will suppress it.
>>
>>> And also maybe use readl/writel, instead of _relaxed.
>> The IPCC device is exclusively used on ARM. In ARM architecture, the
>> ioremap on devices are strictly ordered and uncached.
>> In that case, using _relaxed avoids an unneeded cache flush, slightly
>> improving performance.
>>
> Its not the portability, but that the impact is negligible in favor of
> _relaxed() version when all you do is just program some registers and
> not heavy duty i/o. But I am ok either way.  You'd gain far more
> performance handling irqs in non-threaded manner :)
>
> Cheers!

Re: [PATCH v2 2/2] mailbox: add STMicroelectronics STM32 IPCC driver

[Jassi Brar]

On Fri, Apr 6, 2018 at 8:35 PM, Fabien DESSENNE <fabien.dessenne@st.com> wrote:
>
> On 06/04/18 14:56, Jassi Brar wrote:
>> On Fri, Apr 6, 2018 at 5:59 PM, Fabien DESSENNE <fabien.dessenne@st.com> wrote:
>>> Hi
>>>
>>>
>>> On 05/04/18 11:38, Jassi Brar wrote:
>>>> On Mon, Mar 12, 2018 at 4:28 PM, Fabien Dessenne <fabien.dessenne@st.com> wrote:
>>>> ....
>>>>> +
>>>>> +       /* irq */
>>>>> +       for (i = 0; i < IPCC_IRQ_NUM; i++) {
>>>>> +               ipcc->irqs[i] = of_irq_get_byname(dev->of_node, irq_name[i]);
>>>>> +               if (ipcc->irqs[i] < 0) {
>>>>> +                       dev_err(dev, "no IRQ specified %s\n", irq_name[i]);
>>>>> +                       ret = ipcc->irqs[i];
>>>>> +                       goto err_clk;
>>>>> +               }
>>>>> +
>>>>> +               ret = devm_request_threaded_irq(dev, ipcc->irqs[i], NULL,
>>>>> +                                               irq_thread[i], IRQF_ONESHOT,
>>>>> +                                               dev_name(dev), ipcc);
>>>>>
>>>> In your interrupt handlers you don't do anything that could block.
>>>> Threads only adds some delay to your message handling.
>>>> So maybe use devm_request_irq() ?
>>> The interrupt handlers call mbox_chan_received_data() /
>>> mbox_chan_txdone(), which call in turn client's rx_callback() /
>>> tx_done() / tx_prepare() which behavior may be unsafe. Hence, using a
>>> threaded irq here seems to be a good choice.
>>>
>> rx_callback() is supposed to be atomic.
>
> I am worried with this atomic part (and honestly I did not note that the
> callbacks were expected to be)
>
> In my case, remoteproc->virtio->rpmsg is the mailbox client defining the
> rx_callback.
> If I follow your suggestion, I shall make this rx_callback Atomic in
> remoteproc (or in virtio or rpmsg). And this does not seem to be so
> simple (add a worker in the middle of somewhere?). Bjorn, feel free to
> comment this part.
>
> An alternate implementation consists in using a threaded IRQ for the
> mailbox interrupt.
> This option is not only simple, but also ensures to split bottom & half
> parts at the irq level which is IMHO a general good practice.
>
> I can see that some mailbox clients implement callbacks that are NOT
> atomic and I suspect this is the reason why some mailbox drivers use
> threaded_irq (rockchip mailbox splits the bottom & half parts).
>
> Would it be acceptable to consider the "atomic client callback" as a
> non-strict rule ?
>
Of course you can traverse atomic path from sleepable context (but not
vice-versa).
Please send in the final revision.

Thanks.

Re: [PATCH v2 2/2] mailbox: add STMicroelectronics STM32 IPCC driver

[Fabien DESSENNE]

On 06/04/18 18:20, Jassi Brar wrote:
> On Fri, Apr 6, 2018 at 8:35 PM, Fabien DESSENNE <fabien.dessenne-qxv4g6HH51o@public.gmane.org> wrote:
>> On 06/04/18 14:56, Jassi Brar wrote:
>>> On Fri, Apr 6, 2018 at 5:59 PM, Fabien DESSENNE <fabien.dessenne-qxv4g6HH51o@public.gmane.org> wrote:
>>>> Hi
>>>>
>>>>
>>>> On 05/04/18 11:38, Jassi Brar wrote:
>>>>> On Mon, Mar 12, 2018 at 4:28 PM, Fabien Dessenne <fabien.dessenne-qxv4g6HH51o@public.gmane.org> wrote:
>>>>> ....
>>>>>> +
>>>>>> +       /* irq */
>>>>>> +       for (i = 0; i < IPCC_IRQ_NUM; i++) {
>>>>>> +               ipcc->irqs[i] = of_irq_get_byname(dev->of_node, irq_name[i]);
>>>>>> +               if (ipcc->irqs[i] < 0) {
>>>>>> +                       dev_err(dev, "no IRQ specified %s\n", irq_name[i]);
>>>>>> +                       ret = ipcc->irqs[i];
>>>>>> +                       goto err_clk;
>>>>>> +               }
>>>>>> +
>>>>>> +               ret = devm_request_threaded_irq(dev, ipcc->irqs[i], NULL,
>>>>>> +                                               irq_thread[i], IRQF_ONESHOT,
>>>>>> +                                               dev_name(dev), ipcc);
>>>>>>
>>>>> In your interrupt handlers you don't do anything that could block.
>>>>> Threads only adds some delay to your message handling.
>>>>> So maybe use devm_request_irq() ?
>>>> The interrupt handlers call mbox_chan_received_data() /
>>>> mbox_chan_txdone(), which call in turn client's rx_callback() /
>>>> tx_done() / tx_prepare() which behavior may be unsafe. Hence, using a
>>>> threaded irq here seems to be a good choice.
>>>>
>>> rx_callback() is supposed to be atomic.
>> I am worried with this atomic part (and honestly I did not note that the
>> callbacks were expected to be)
>>
>> In my case, remoteproc->virtio->rpmsg is the mailbox client defining the
>> rx_callback.
>> If I follow your suggestion, I shall make this rx_callback Atomic in
>> remoteproc (or in virtio or rpmsg). And this does not seem to be so
>> simple (add a worker in the middle of somewhere?). Bjorn, feel free to
>> comment this part.
>>
>> An alternate implementation consists in using a threaded IRQ for the
>> mailbox interrupt.
>> This option is not only simple, but also ensures to split bottom & half
>> parts at the irq level which is IMHO a general good practice.
>>
>> I can see that some mailbox clients implement callbacks that are NOT
>> atomic and I suspect this is the reason why some mailbox drivers use
>> threaded_irq (rockchip mailbox splits the bottom & half parts).
>>
>> Would it be acceptable to consider the "atomic client callback" as a
>> non-strict rule ?
>>
> Of course you can traverse atomic path from sleepable context (but not
> vice-versa).

So, to be sure we understand each other, I can use threaded_irq, right?

> Please send in the final revision.
>
> Thanks.

Re: [PATCH v2 2/2] mailbox: add STMicroelectronics STM32 IPCC driver

[Jassi Brar]

On Mon, Apr 9, 2018 at 2:33 PM, Fabien DESSENNE <fabien.dessenne@st.com> wrote:
>
>
> On 06/04/18 18:20, Jassi Brar wrote:
>> On Fri, Apr 6, 2018 at 8:35 PM, Fabien DESSENNE <fabien.dessenne@st.com> wrote:
>>> On 06/04/18 14:56, Jassi Brar wrote:
>>>> On Fri, Apr 6, 2018 at 5:59 PM, Fabien DESSENNE <fabien.dessenne@st.com> wrote:
>>>>> Hi
>>>>>
>>>>>
>>>>> On 05/04/18 11:38, Jassi Brar wrote:
>>>>>> On Mon, Mar 12, 2018 at 4:28 PM, Fabien Dessenne <fabien.dessenne@st.com> wrote:
>>>>>> ....
>>>>>>> +
>>>>>>> +       /* irq */
>>>>>>> +       for (i = 0; i < IPCC_IRQ_NUM; i++) {
>>>>>>> +               ipcc->irqs[i] = of_irq_get_byname(dev->of_node, irq_name[i]);
>>>>>>> +               if (ipcc->irqs[i] < 0) {
>>>>>>> +                       dev_err(dev, "no IRQ specified %s\n", irq_name[i]);
>>>>>>> +                       ret = ipcc->irqs[i];
>>>>>>> +                       goto err_clk;
>>>>>>> +               }
>>>>>>> +
>>>>>>> +               ret = devm_request_threaded_irq(dev, ipcc->irqs[i], NULL,
>>>>>>> +                                               irq_thread[i], IRQF_ONESHOT,
>>>>>>> +                                               dev_name(dev), ipcc);
>>>>>>>
>>>>>> In your interrupt handlers you don't do anything that could block.
>>>>>> Threads only adds some delay to your message handling.
>>>>>> So maybe use devm_request_irq() ?
>>>>> The interrupt handlers call mbox_chan_received_data() /
>>>>> mbox_chan_txdone(), which call in turn client's rx_callback() /
>>>>> tx_done() / tx_prepare() which behavior may be unsafe. Hence, using a
>>>>> threaded irq here seems to be a good choice.
>>>>>
>>>> rx_callback() is supposed to be atomic.
>>> I am worried with this atomic part (and honestly I did not note that the
>>> callbacks were expected to be)
>>>
>>> In my case, remoteproc->virtio->rpmsg is the mailbox client defining the
>>> rx_callback.
>>> If I follow your suggestion, I shall make this rx_callback Atomic in
>>> remoteproc (or in virtio or rpmsg). And this does not seem to be so
>>> simple (add a worker in the middle of somewhere?). Bjorn, feel free to
>>> comment this part.
>>>
>>> An alternate implementation consists in using a threaded IRQ for the
>>> mailbox interrupt.
>>> This option is not only simple, but also ensures to split bottom & half
>>> parts at the irq level which is IMHO a general good practice.
>>>
>>> I can see that some mailbox clients implement callbacks that are NOT
>>> atomic and I suspect this is the reason why some mailbox drivers use
>>> threaded_irq (rockchip mailbox splits the bottom & half parts).
>>>
>>> Would it be acceptable to consider the "atomic client callback" as a
>>> non-strict rule ?
>>>
>> Of course you can traverse atomic path from sleepable context (but not
>> vice-versa).
>
> So, to be sure we understand each other, I can use threaded_irq, right?
>
Yes. Its platform specific driver, I can't dictate the features you want :)