[PATCH 0/3] drivers: soc: ti: Introduce wkup_m3_ipc driver

[PATCH 0/3] drivers: soc: ti: Introduce wkup_m3_ipc driver

[Dave Gerlach]

This series introduces a wkup_m3_ipc driver to handle communication
between the MPU and Cortex M3 present on TI AM335x SoCs. This is
required for much of the PM functionality for AM335x including suspend
support. This was split off from v4 of the am335x suspend series,
discussion that led to the implementation of this driver can be found
with the series here [1].  A previous RFC version of this series can be
found here [2]. The changes from that version are as follows:
 - Remove wake source reporting as it is unnecessary.
 - Use newly introduced rproc_get_by_phandle API to get rproc for
   booting [3].

This series depends on the patch "remoteproc: Introduce
rproc_get_by_phandle API" [3] and the wkup_m3_rproc series found
here [4]. A branch based on 3.19-rc1 containing this series and
all dependencies for the AM33xx suspend series can be found
here [5] for a high level view of what I am using this for.

A small API is exposed to allow the SoC PM code to execute the
specific tasks it needs to in order to enter and exit low power
modes. Communication works the same as it did in the past using the
IPC registers found within the control module, a mailbox module, and
an interrupt coming back from the CM3. All of that, including the
configurations needed for different low power tasks is encapsulated
within this driver.

Regards,
Dave

[1] http://www.spinics.net/lists/linux-omap/msg109331.html
[2] http://www.spinics.net/lists/linux-omap/msg113372.html
[3] http://marc.info/?l=linux-kernel&m=142022798923784&w=2
[4] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg795457.html
[5] https://github.com/dgerlach/linux-pm/tree/pm-am335x-v3.19-rc1

Dave Gerlach (3):
  Documentation: dt: add ti,am3353-wkup-m3-ipc bindings
  soc: ti: Add wkup_m3_ipc driver
  ARM: dts: am33xx: Add wkup_m3_ipc node

 .../devicetree/bindings/soc/ti/wkup_m3_ipc.txt     |  41 ++
 arch/arm/boot/dts/am33xx.dtsi                      |   9 +
 drivers/soc/ti/Kconfig                             |  11 +
 drivers/soc/ti/Makefile                            |   1 +
 drivers/soc/ti/wkup_m3_ipc.c                       | 451 +++++++++++++++++++++
 include/linux/wkup_m3_ipc.h                        |  33 ++
 6 files changed, 546 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/ti/wkup_m3_ipc.txt
 create mode 100644 drivers/soc/ti/wkup_m3_ipc.c
 create mode 100644 include/linux/wkup_m3_ipc.h

-- 
2.1.0

[PATCH 1/3] Documentation: dt: add ti,am3353-wkup-m3-ipc bindings

[Dave Gerlach]

Add the device tree bindings document for ti,am3353-wkup-m3-ipc which
is used by the wkup_m3_ipc driver.

Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
---
 .../devicetree/bindings/soc/ti/wkup_m3_ipc.txt     | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/ti/wkup_m3_ipc.txt

diff --git a/Documentation/devicetree/bindings/soc/ti/wkup_m3_ipc.txt b/Documentation/devicetree/bindings/soc/ti/wkup_m3_ipc.txt
new file mode 100644
index 0000000..ceb6acf
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/ti/wkup_m3_ipc.txt
@@ -0,0 +1,41 @@
+Wakeup M3 IPC Driver
+=====================
+
+TI AMx3 family of devices use a Cortex M3 co-processor to help with various
+low power tasks that cannot be controlled from the MPU like suspend/resume
+and certain deep C-states for CPU Idle. Once the wkup_m3_ipc driver uses the
+wkup_m3_rproc driver to boot the wkup_m3, it handles communication with the
+CM3 using IPC registers present in the SoC's control module and a mailbox.
+The wkup_m3_ipc exposes an API to allow the SoC PM code to execute specific
+PM tasks.
+
+Wkup M3 Device Node:
+====================
+A wkup_m3_ipc device node is used to represent a wakeup M3 IP instance within
+an SoC. The sub-mailboxes are represented as child node of this parent node.
+
+Required properties:
+--------------------
+- compatible:		Should be "ti,am3353-wkup-m3-ipc" for AM33xx SoCs
+- reg:			Contains the wkup_m3 register address ranges for
+			ipc-regs.
+- reg-names:		Name for ipc-regs given above
+- interrupts:		Contains the interrupt information for the wkup_m3
+			interrupt that signals the MPU.
+- ti,rproc:		Phandle to the wkup_m3 rproc node so the IPC driver
+			can boot it.
+- mboxes:		Phandles used by IPC framework to get correct mbox
+			channel for communication. Must point to appropriate
+			mbox_wkupm3 child node.
+
+Example:
+--------
+/* AM33xx */
+wkup_m3_ipc: wkup_m3_ipc@44e11324 {
+	compatible = "ti,am3353-wkup-m3-ipc";
+	reg = <0x44e11324 0x0024>;
+	reg-names = "ipc_regs";
+	interrupts = <78>;
+	ti,rproc = <&wkup_m3>;
+	mboxes = <&mailbox &mbox_wkupm3>;
+};
-- 
2.1.0

[PATCH 2/3] soc: ti: Add wkup_m3_ipc driver

[Dave Gerlach]

Introduce a wkup_m3_ipc driver to handle communication between the MPU
and Cortex M3 wkup_m3 present on am335x.

This driver is responsible for actually booting the wkup_m3_rproc and
also handling all IPC which is done using the IPC registers in the control
module, a mailbox, and a separate interrupt back from the wkup_m3. A small
API is exposed for executing specific power commands, which include
configuring for low power mode, request a transition to a low power mode,
and status info on a previous transition.

Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
---
 drivers/soc/ti/Kconfig       |  11 ++
 drivers/soc/ti/Makefile      |   1 +
 drivers/soc/ti/wkup_m3_ipc.c | 451 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/wkup_m3_ipc.h  |  33 ++++
 4 files changed, 496 insertions(+)
 create mode 100644 drivers/soc/ti/wkup_m3_ipc.c
 create mode 100644 include/linux/wkup_m3_ipc.h

diff --git a/drivers/soc/ti/Kconfig b/drivers/soc/ti/Kconfig
index 7266b21..61cda85 100644
--- a/drivers/soc/ti/Kconfig
+++ b/drivers/soc/ti/Kconfig
@@ -28,4 +28,15 @@ config KEYSTONE_NAVIGATOR_DMA
 
 	  If unsure, say N.
 
+config WKUP_M3_IPC
+	bool "TI AM33XX Wkup-M3 IPC Driver"
+	depends on WKUP_M3_RPROC
+	select MAILBOX
+	select OMAP2PLUS_MBOX
+	help
+	  TI AM33XX has a Cortex M3 to handle low power transitions. This IPC
+	  driver provides the necessary API to communicate and use the wkup m3
+	  for PM features like Suspend/Resume and boots the wkup_m3 using
+	  wkup_m3_rproc driver.
+
 endif # SOC_TI
diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
index 6bed611..b6b8c8b 100644
--- a/drivers/soc/ti/Makefile
+++ b/drivers/soc/ti/Makefile
@@ -3,3 +3,4 @@
 #
 obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS)	+= knav_qmss_queue.o knav_qmss_acc.o
 obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA)	+= knav_dma.o
+obj-$(CONFIG_WKUP_M3_IPC)		+= wkup_m3_ipc.o
diff --git a/drivers/soc/ti/wkup_m3_ipc.c b/drivers/soc/ti/wkup_m3_ipc.c
new file mode 100644
index 0000000..4dcf748
--- /dev/null
+++ b/drivers/soc/ti/wkup_m3_ipc.c
@@ -0,0 +1,451 @@
+/*
+ * AMx3 Wkup M3 IPC driver
+ *
+ * Copyright (C) 2014 Texas Instruments, Inc.
+ *
+ * Dave Gerlach <d-gerlach@ti.com>
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/kthread.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/omap-mailbox.h>
+#include <linux/platform_device.h>
+#include <linux/remoteproc.h>
+#include <linux/suspend.h>
+#include <linux/wkup_m3_ipc.h>
+
+#define AM33XX_CTRL_IPC_REG_COUNT	0x8
+#define AM33XX_CTRL_IPC_REG_OFFSET(m)	(0x4 + 4 * (m))
+
+/* AM33XX M3_TXEV_EOI register */
+#define AM33XX_CONTROL_M3_TXEV_EOI	0x00
+
+#define AM33XX_M3_TXEV_ACK		(0x1 << 0)
+#define AM33XX_M3_TXEV_ENABLE		(0x0 << 0)
+
+#define IPC_CMD_DS0			0x4
+#define IPC_CMD_STANDBY			0xc
+#define IPC_CMD_RESET			0xe
+#define DS_IPC_DEFAULT			0xffffffff
+#define M3_VERSION_UNKNOWN		0x0000ffff
+#define M3_BASELINE_VERSION		0x187
+#define M3_STATUS_RESP_MASK		(0xffff << 16)
+#define M3_FW_VERSION_MASK		0xffff
+
+#define M3_STATE_UNKNOWN		0
+#define M3_STATE_RESET			1
+#define M3_STATE_INITED			2
+#define M3_STATE_MSG_FOR_LP		3
+#define M3_STATE_MSG_FOR_RESET		4
+
+struct wkup_m3_ipc {
+	struct rproc *rproc;
+
+	void __iomem *ipc_mem_base;
+	struct device *dev;
+
+	int mem_type;
+	unsigned long resume_addr;
+	int state;
+
+	struct mbox_client mbox_client;
+	struct mbox_chan *mbox;
+};
+
+static struct wkup_m3_ipc m3_ipc_state;
+
+static DECLARE_COMPLETION(m3_ipc_sync);
+
+static inline void am33xx_txev_eoi(struct wkup_m3_ipc *m3_ipc)
+{
+	writel(AM33XX_M3_TXEV_ACK,
+	       m3_ipc->ipc_mem_base + AM33XX_CONTROL_M3_TXEV_EOI);
+}
+
+static inline void am33xx_txev_enable(struct wkup_m3_ipc *m3_ipc)
+{
+	writel(AM33XX_M3_TXEV_ENABLE,
+	       m3_ipc->ipc_mem_base + AM33XX_CONTROL_M3_TXEV_EOI);
+}
+
+static inline void wkup_m3_ctrl_ipc_write(struct wkup_m3_ipc *m3_ipc,
+					  u32 val, int ipc_reg_num)
+{
+	if (ipc_reg_num < 0 || ipc_reg_num > AM33XX_CTRL_IPC_REG_COUNT)
+		return;
+
+	writel(val, m3_ipc->ipc_mem_base +
+	       AM33XX_CTRL_IPC_REG_OFFSET(ipc_reg_num));
+}
+
+static inline unsigned int wkup_m3_ctrl_ipc_read(struct wkup_m3_ipc *m3_ipc,
+						 int ipc_reg_num)
+{
+	if (ipc_reg_num < 0 || ipc_reg_num > AM33XX_CTRL_IPC_REG_COUNT)
+		return 0;
+
+	return readl(m3_ipc->ipc_mem_base +
+		     AM33XX_CTRL_IPC_REG_OFFSET(ipc_reg_num));
+}
+
+static int wkup_m3_fw_version_read(void)
+{
+	int val;
+
+	val = wkup_m3_ctrl_ipc_read(&m3_ipc_state, 2);
+
+	return val & M3_FW_VERSION_MASK;
+}
+
+static irqreturn_t wkup_m3_txev_handler(int irq, void *ipc_data)
+{
+	struct wkup_m3_ipc *m3_ipc = (struct wkup_m3_ipc *)ipc_data;
+	struct device *dev = m3_ipc->dev;
+	int ver = 0;
+
+	am33xx_txev_eoi(m3_ipc);
+
+	switch (m3_ipc->state) {
+	case M3_STATE_RESET:
+		ver = wkup_m3_fw_version_read();
+
+		if (ver == M3_VERSION_UNKNOWN ||
+		    ver < M3_BASELINE_VERSION) {
+			dev_warn(dev, "CM3 Firmware Version %x not supported\n",
+				 ver);
+		} else {
+			dev_info(dev, "CM3 Firmware Version = 0x%x\n", ver);
+		}
+
+		m3_ipc->state = M3_STATE_INITED;
+		complete(&m3_ipc_sync);
+		break;
+	case M3_STATE_MSG_FOR_RESET:
+		m3_ipc->state = M3_STATE_INITED;
+		complete(&m3_ipc_sync);
+		break;
+	case M3_STATE_MSG_FOR_LP:
+		complete(&m3_ipc_sync);
+		break;
+	case M3_STATE_UNKNOWN:
+		dev_warn(dev, "Unknown CM3 State\n");
+	}
+
+	am33xx_txev_enable(m3_ipc);
+
+	return IRQ_HANDLED;
+}
+
+static void wkup_m3_mbox_callback(struct mbox_client *client, void *data)
+{
+	struct wkup_m3_ipc *m3_ipc = container_of(client, struct wkup_m3_ipc,
+						  mbox_client);
+
+	omap_mbox_disable_irq(m3_ipc->mbox, IRQ_RX);
+}
+
+static int wkup_m3_ping(void)
+{
+	struct device *dev = m3_ipc_state.dev;
+	mbox_msg_t dummy_msg = 0;
+	int ret;
+
+	if (!m3_ipc_state.mbox) {
+		dev_err(m3_ipc_state.dev,
+			"No IPC channel to communicate with wkup_m3!\n");
+		return -EIO;
+	}
+
+	/*
+	 * Write a dummy message to the mailbox in order to trigger the RX
+	 * interrupt to alert the M3 that data is available in the IPC
+	 * registers. We must enable the IRQ here and disable it after in
+	 * the RX callback to avoid multiple interrupts being received
+	 * by the CM3.
+	 */
+	omap_mbox_enable_irq(m3_ipc_state.mbox, IRQ_RX);
+	ret = mbox_send_message(m3_ipc_state.mbox, (void *)dummy_msg);
+	if (ret < 0) {
+		dev_err(dev, "%s: mbox_send_message() failed: %d\n",
+			__func__, ret);
+		return ret;
+	}
+
+	ret = wait_for_completion_timeout(&m3_ipc_sync,
+					  msecs_to_jiffies(500));
+	if (!ret) {
+		dev_err(dev, "MPU<->CM3 sync failure\n");
+		m3_ipc_state.state = M3_STATE_UNKNOWN;
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int wkup_m3_is_available(void)
+{
+	return (m3_ipc_state.state != M3_STATE_RESET);
+}
+
+/* Public functions */
+/**
+ * wkup_m3_set_mem_type - Pass wkup_m3 which type of memory is in use
+ * @mem_type: memory type value read directly from emif
+ *
+ * wkup_m3 must know what memory type is in use to properly suspend
+ * and resume.
+ */
+void wkup_m3_set_mem_type(int mem_type)
+{
+	m3_ipc_state.mem_type = mem_type;
+}
+
+/**
+ * wkup_m3_set_resume_address - Pass wkup_m3 resume address
+ * @addr: Physical address from which resume code should execute
+ */
+void wkup_m3_set_resume_address(void *addr)
+{
+	m3_ipc_state.resume_addr = (unsigned long)addr;
+}
+
+/**
+ * wkup_m3_set_resume_address - Retrieve wkup_m3 status code after suspend
+ *
+ * Returns code representing the status of a low power mode transition.
+ *	0 - Successful transition
+ *	1 - Failure to transition to low power state
+ */
+int wkup_m3_request_pm_status(void)
+{
+	unsigned int i;
+	int val;
+
+	val = wkup_m3_ctrl_ipc_read(&m3_ipc_state, 1);
+
+	i = M3_STATUS_RESP_MASK & val;
+	i >>= __ffs(M3_STATUS_RESP_MASK);
+
+	return i;
+}
+
+/**
+ * wkup_m3_prepare_low_power - Request preparation for transition to
+ *			       low power state
+ * @state: A kernel suspend state to enter, either MEM or STANDBY
+ *
+ * Returns 0 if preparation was successful, otherwise returns error code
+ */
+int wkup_m3_prepare_low_power(int state)
+{
+	struct device *dev = m3_ipc_state.dev;
+	int m3_power_state;
+	int ret = 0;
+
+	if (!wkup_m3_is_available()) {
+		dev_err(dev, "wkup_m3 not available. DeepSleep entry not possible.\n");
+		return -ENODEV;
+	}
+
+	switch (state) {
+	case PM_SUSPEND_MEM:
+		m3_power_state = IPC_CMD_DS0;
+		break;
+	default:
+		return 1;
+	}
+
+	/* Program each required IPC register then write defaults to others */
+	wkup_m3_ctrl_ipc_write(&m3_ipc_state, m3_ipc_state.resume_addr, 0);
+	wkup_m3_ctrl_ipc_write(&m3_ipc_state, m3_power_state, 1);
+	wkup_m3_ctrl_ipc_write(&m3_ipc_state, m3_ipc_state.mem_type, 4);
+
+	wkup_m3_ctrl_ipc_write(&m3_ipc_state, DS_IPC_DEFAULT, 2);
+	wkup_m3_ctrl_ipc_write(&m3_ipc_state, DS_IPC_DEFAULT, 3);
+	wkup_m3_ctrl_ipc_write(&m3_ipc_state, DS_IPC_DEFAULT, 5);
+	wkup_m3_ctrl_ipc_write(&m3_ipc_state, DS_IPC_DEFAULT, 6);
+	wkup_m3_ctrl_ipc_write(&m3_ipc_state, DS_IPC_DEFAULT, 7);
+
+	m3_ipc_state.state = M3_STATE_MSG_FOR_LP;
+
+	ret = wkup_m3_ping();
+	if (ret) {
+		dev_err(dev, "Unable to ping CM3\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * wkup_m3_finish_low_power - Return m3 to reset state
+ *
+ * Returns 0 if reset was successful, otherwise returns error code
+ */
+int wkup_m3_finish_low_power(void)
+{
+	struct device *dev = m3_ipc_state.dev;
+	int ret = 0;
+
+	if (!wkup_m3_is_available())
+		return -ENODEV;
+
+	wkup_m3_ctrl_ipc_write(&m3_ipc_state, IPC_CMD_RESET, 1);
+	wkup_m3_ctrl_ipc_write(&m3_ipc_state, DS_IPC_DEFAULT, 2);
+
+	m3_ipc_state.state = M3_STATE_MSG_FOR_RESET;
+
+	ret = wkup_m3_ping();
+	if (ret) {
+		dev_err(dev, "Unable to ping CM3\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static void wkup_m3_rproc_boot_thread(struct rproc *rproc)
+{
+	struct device *dev = &rproc->dev;
+	int ret;
+
+	wait_for_completion(&rproc->firmware_loading_complete);
+
+	ret = rproc_boot(rproc);
+	if (ret)
+		dev_err(dev, "rproc_boot failed\n");
+
+	do_exit(0);
+}
+
+static int wkup_m3_ipc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int irq, ret;
+	uint32_t rproc_phandle;
+	struct rproc *m3_rproc;
+	struct resource *res;
+	struct task_struct *task;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ipc_regs");
+	m3_ipc_state.ipc_mem_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(m3_ipc_state.ipc_mem_base)) {
+		dev_err(dev, "could not ioremap ipc_mem\n");
+		ret = PTR_ERR(m3_ipc_state.ipc_mem_base);
+		goto err;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	if (!irq) {
+		dev_err(&pdev->dev, "no irq resource\n");
+		ret = -ENXIO;
+		goto err;
+	}
+
+	ret = devm_request_irq(dev, irq, wkup_m3_txev_handler,
+			       IRQF_DISABLED, "wkup_m3_txev", &m3_ipc_state);
+	if (ret) {
+		dev_err(dev, "request_irq failed\n");
+		goto err;
+	}
+
+	m3_ipc_state.mbox_client.dev = dev;
+	m3_ipc_state.mbox_client.tx_done = NULL;
+	m3_ipc_state.mbox_client.rx_callback = wkup_m3_mbox_callback;
+	m3_ipc_state.mbox_client.tx_block = false;
+	m3_ipc_state.mbox_client.knows_txdone = false;
+
+	m3_ipc_state.mbox = mbox_request_channel(&m3_ipc_state.mbox_client, 0);
+	if (IS_ERR(m3_ipc_state.mbox)) {
+		dev_err(dev, "IPC Request for A8->M3 Channel failed! %ld\n",
+			PTR_ERR(m3_ipc_state.mbox));
+		ret = PTR_ERR(m3_ipc_state.mbox);
+		m3_ipc_state.mbox = NULL;
+		return ret;
+	}
+
+	if (of_property_read_u32(dev->of_node, "ti,rproc", &rproc_phandle)) {
+		dev_err(&pdev->dev, "could not get rproc phandle\n");
+		ret = -ENODEV;
+		goto err;
+	}
+
+	m3_rproc = rproc_get_by_phandle(rproc_phandle);
+	if (!m3_rproc) {
+		dev_err(&pdev->dev, "could not get rproc handle\n");
+		ret = -EPROBE_DEFER;
+		goto err;
+	}
+
+	m3_ipc_state.rproc = m3_rproc;
+	m3_ipc_state.dev = dev;
+	m3_ipc_state.state = M3_STATE_RESET;
+
+	/*
+	 * Wait for firmware loading completion in a thread so we
+	 * can boot the wkup_m3 as soon as it's ready without holding
+	 * up kernel boot
+	 */
+	task = kthread_run((void *)wkup_m3_rproc_boot_thread, m3_rproc,
+			   "wkup_m3_rproc_loader");
+
+	if (IS_ERR(task)) {
+		dev_err(dev, "can't create rproc_boot thread\n");
+		goto err_put_rproc;
+	}
+
+	return 0;
+
+err_put_rproc:
+	rproc_put(m3_rproc);
+err:
+	mbox_free_channel(m3_ipc_state.mbox);
+	return ret;
+}
+
+static int wkup_m3_ipc_remove(struct platform_device *pdev)
+{
+	if (m3_ipc_state.mbox)
+		mbox_free_channel(m3_ipc_state.mbox);
+
+	if (m3_ipc_state.rproc) {
+		rproc_shutdown(m3_ipc_state.rproc);
+		rproc_put(m3_ipc_state.rproc);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id wkup_m3_ipc_of_match[] = {
+	{ .compatible = "ti,am3353-wkup-m3-ipc", .data = NULL, },
+	{},
+};
+
+static struct platform_driver wkup_m3_ipc_driver = {
+	.probe = wkup_m3_ipc_probe,
+	.remove = wkup_m3_ipc_remove,
+	.driver = {
+		.name = "wkup_m3_ipc",
+		.of_match_table = wkup_m3_ipc_of_match,
+	},
+};
+
+module_platform_driver(wkup_m3_ipc_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("wkup m3 remote processor ipc driver");
diff --git a/include/linux/wkup_m3_ipc.h b/include/linux/wkup_m3_ipc.h
new file mode 100644
index 0000000..a0c2722
--- /dev/null
+++ b/include/linux/wkup_m3_ipc.h
@@ -0,0 +1,33 @@
+/*
+ * TI Wakeup M3 for AMx3 SoCs Power Management Routines
+ *
+ * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
+ * Dave Gerlach <d-gerlach@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _LINUX_WKUP_M3_H
+#define _LINUX_WKUP_M3_H
+
+#ifdef CONFIG_WKUP_M3_IPC
+void wkup_m3_set_mem_type(int mem_type);
+void wkup_m3_set_resume_address(void *addr);
+int wkup_m3_prepare_low_power(int state);
+int wkup_m3_finish_low_power(void);
+int wkup_m3_request_pm_status(void);
+#else
+static inline void wkup_m3_set_mem_type(int mem_type) { }
+static inline void wkup_m3_set_resume_address(void *addr) { }
+static inline int wkup_m3_prepare_low_power(int state) { return -1; }
+static inline int wkup_m3_finish_low_power(void) { return -1; }
+static inline int wkup_m3_request_pm_status(void) { return -1; }
+#endif /* CONFIG_WKUP_M3_IPC */
+#endif /* _LINUX_WKUP_M3_H */
-- 
2.1.0

Re: [PATCH 2/3] soc: ti: Add wkup_m3_ipc driver

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 17029 bytes --]

On Fri, Jan 02, 2015 at 02:00:16PM -0600, Dave Gerlach wrote:
> Introduce a wkup_m3_ipc driver to handle communication between the MPU
> and Cortex M3 wkup_m3 present on am335x.
> 
> This driver is responsible for actually booting the wkup_m3_rproc and
> also handling all IPC which is done using the IPC registers in the control
> module, a mailbox, and a separate interrupt back from the wkup_m3. A small
> API is exposed for executing specific power commands, which include
> configuring for low power mode, request a transition to a low power mode,
> and status info on a previous transition.
> 
> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
> ---
>  drivers/soc/ti/Kconfig       |  11 ++
>  drivers/soc/ti/Makefile      |   1 +
>  drivers/soc/ti/wkup_m3_ipc.c | 451 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/wkup_m3_ipc.h  |  33 ++++
>  4 files changed, 496 insertions(+)
>  create mode 100644 drivers/soc/ti/wkup_m3_ipc.c
>  create mode 100644 include/linux/wkup_m3_ipc.h
> 
> diff --git a/drivers/soc/ti/Kconfig b/drivers/soc/ti/Kconfig
> index 7266b21..61cda85 100644
> --- a/drivers/soc/ti/Kconfig
> +++ b/drivers/soc/ti/Kconfig
> @@ -28,4 +28,15 @@ config KEYSTONE_NAVIGATOR_DMA
>  
>  	  If unsure, say N.
>  
> +config WKUP_M3_IPC
> +	bool "TI AM33XX Wkup-M3 IPC Driver"

tristate ?

> +	depends on WKUP_M3_RPROC
> +	select MAILBOX
> +	select OMAP2PLUS_MBOX

selects are usually frowned upon.

> +	help
> +	  TI AM33XX has a Cortex M3 to handle low power transitions. This IPC
> +	  driver provides the necessary API to communicate and use the wkup m3
> +	  for PM features like Suspend/Resume and boots the wkup_m3 using
> +	  wkup_m3_rproc driver.
> +
>  endif # SOC_TI
> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
> index 6bed611..b6b8c8b 100644
> --- a/drivers/soc/ti/Makefile
> +++ b/drivers/soc/ti/Makefile
> @@ -3,3 +3,4 @@
>  #
>  obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS)	+= knav_qmss_queue.o knav_qmss_acc.o
>  obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA)	+= knav_dma.o
> +obj-$(CONFIG_WKUP_M3_IPC)		+= wkup_m3_ipc.o
> diff --git a/drivers/soc/ti/wkup_m3_ipc.c b/drivers/soc/ti/wkup_m3_ipc.c
> new file mode 100644
> index 0000000..4dcf748
> --- /dev/null
> +++ b/drivers/soc/ti/wkup_m3_ipc.c
> @@ -0,0 +1,451 @@
> +/*
> + * AMx3 Wkup M3 IPC driver
> + *
> + * Copyright (C) 2014 Texas Instruments, Inc.
> + *
> + * Dave Gerlach <d-gerlach@ti.com>
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/kthread.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/omap-mailbox.h>
> +#include <linux/platform_device.h>
> +#include <linux/remoteproc.h>
> +#include <linux/suspend.h>
> +#include <linux/wkup_m3_ipc.h>
> +
> +#define AM33XX_CTRL_IPC_REG_COUNT	0x8
> +#define AM33XX_CTRL_IPC_REG_OFFSET(m)	(0x4 + 4 * (m))
> +
> +/* AM33XX M3_TXEV_EOI register */
> +#define AM33XX_CONTROL_M3_TXEV_EOI	0x00
> +
> +#define AM33XX_M3_TXEV_ACK		(0x1 << 0)
> +#define AM33XX_M3_TXEV_ENABLE		(0x0 << 0)
> +
> +#define IPC_CMD_DS0			0x4
> +#define IPC_CMD_STANDBY			0xc
> +#define IPC_CMD_RESET			0xe
> +#define DS_IPC_DEFAULT			0xffffffff
> +#define M3_VERSION_UNKNOWN		0x0000ffff
> +#define M3_BASELINE_VERSION		0x187
> +#define M3_STATUS_RESP_MASK		(0xffff << 16)
> +#define M3_FW_VERSION_MASK		0xffff
> +
> +#define M3_STATE_UNKNOWN		0
> +#define M3_STATE_RESET			1
> +#define M3_STATE_INITED			2
> +#define M3_STATE_MSG_FOR_LP		3
> +#define M3_STATE_MSG_FOR_RESET		4
> +
> +struct wkup_m3_ipc {
> +	struct rproc *rproc;
> +
> +	void __iomem *ipc_mem_base;
> +	struct device *dev;
> +
> +	int mem_type;
> +	unsigned long resume_addr;
> +	int state;
> +
> +	struct mbox_client mbox_client;
> +	struct mbox_chan *mbox;
> +};
> +
> +static struct wkup_m3_ipc m3_ipc_state;
> +
> +static DECLARE_COMPLETION(m3_ipc_sync);

either move this inside struct wkup_m3_ipc or make this
DECLARE_COMPLETION_ONSTACK();

> +
> +static inline void am33xx_txev_eoi(struct wkup_m3_ipc *m3_ipc)
> +{
> +	writel(AM33XX_M3_TXEV_ACK,
> +	       m3_ipc->ipc_mem_base + AM33XX_CONTROL_M3_TXEV_EOI);
> +}
> +
> +static inline void am33xx_txev_enable(struct wkup_m3_ipc *m3_ipc)
> +{
> +	writel(AM33XX_M3_TXEV_ENABLE,
> +	       m3_ipc->ipc_mem_base + AM33XX_CONTROL_M3_TXEV_EOI);
> +}
> +
> +static inline void wkup_m3_ctrl_ipc_write(struct wkup_m3_ipc *m3_ipc,
> +					  u32 val, int ipc_reg_num)
> +{
> +	if (ipc_reg_num < 0 || ipc_reg_num > AM33XX_CTRL_IPC_REG_COUNT)
> +		return;

you should probably add a WARN() here as this should never happen. Also,
you might want to remove all "inline" modifiers and let compiler decide
what to inline.

> +
> +	writel(val, m3_ipc->ipc_mem_base +
> +	       AM33XX_CTRL_IPC_REG_OFFSET(ipc_reg_num));
> +}
> +
> +static inline unsigned int wkup_m3_ctrl_ipc_read(struct wkup_m3_ipc *m3_ipc,
> +						 int ipc_reg_num)
> +{
> +	if (ipc_reg_num < 0 || ipc_reg_num > AM33XX_CTRL_IPC_REG_COUNT)
> +		return 0;
> +
> +	return readl(m3_ipc->ipc_mem_base +
> +		     AM33XX_CTRL_IPC_REG_OFFSET(ipc_reg_num));
> +}
> +
> +static int wkup_m3_fw_version_read(void)

you can very easily pass struct wkup_m3_ipc pointer as argument here.

> +{
> +	int val;
> +
> +	val = wkup_m3_ctrl_ipc_read(&m3_ipc_state, 2);
> +
> +	return val & M3_FW_VERSION_MASK;
> +}
> +
> +static irqreturn_t wkup_m3_txev_handler(int irq, void *ipc_data)
> +{
> +	struct wkup_m3_ipc *m3_ipc = (struct wkup_m3_ipc *)ipc_data;

unnecessary cast

> +	struct device *dev = m3_ipc->dev;
> +	int ver = 0;
> +
> +	am33xx_txev_eoi(m3_ipc);
> +
> +	switch (m3_ipc->state) {
> +	case M3_STATE_RESET:
> +		ver = wkup_m3_fw_version_read();
> +
> +		if (ver == M3_VERSION_UNKNOWN ||
> +		    ver < M3_BASELINE_VERSION) {
> +			dev_warn(dev, "CM3 Firmware Version %x not supported\n",
> +				 ver);
> +		} else {
> +			dev_info(dev, "CM3 Firmware Version = 0x%x\n", ver);
> +		}
> +
> +		m3_ipc->state = M3_STATE_INITED;
> +		complete(&m3_ipc_sync);
> +		break;
> +	case M3_STATE_MSG_FOR_RESET:
> +		m3_ipc->state = M3_STATE_INITED;
> +		complete(&m3_ipc_sync);
> +		break;
> +	case M3_STATE_MSG_FOR_LP:
> +		complete(&m3_ipc_sync);
> +		break;
> +	case M3_STATE_UNKNOWN:
> +		dev_warn(dev, "Unknown CM3 State\n");
> +	}
> +
> +	am33xx_txev_enable(m3_ipc);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void wkup_m3_mbox_callback(struct mbox_client *client, void *data)
> +{
> +	struct wkup_m3_ipc *m3_ipc = container_of(client, struct wkup_m3_ipc,
> +						  mbox_client);
> +
> +	omap_mbox_disable_irq(m3_ipc->mbox, IRQ_RX);
> +}
> +
> +static int wkup_m3_ping(void)

same here, pass wkup_m3_ipc pointer as argument.

> +{
> +	struct device *dev = m3_ipc_state.dev;
> +	mbox_msg_t dummy_msg = 0;
> +	int ret;
> +
> +	if (!m3_ipc_state.mbox) {
> +		dev_err(m3_ipc_state.dev,

you already have a local dev pointer, why don't you use it ?

> +			"No IPC channel to communicate with wkup_m3!\n");
> +		return -EIO;
> +	}
> +
> +	/*
> +	 * Write a dummy message to the mailbox in order to trigger the RX
> +	 * interrupt to alert the M3 that data is available in the IPC
> +	 * registers. We must enable the IRQ here and disable it after in
> +	 * the RX callback to avoid multiple interrupts being received
> +	 * by the CM3.
> +	 */
> +	omap_mbox_enable_irq(m3_ipc_state.mbox, IRQ_RX);
> +	ret = mbox_send_message(m3_ipc_state.mbox, (void *)dummy_msg);

unnecessary cast.

> +	if (ret < 0) {
> +		dev_err(dev, "%s: mbox_send_message() failed: %d\n",
> +			__func__, ret);
> +		return ret;
> +	}
> +
> +	ret = wait_for_completion_timeout(&m3_ipc_sync,

yeah, why couldn't you ?

	wait_for_completion_timeout(&m3_ipc_state->complete, [...]

why does it have to be declared on the stack as a global variable ?

> +					  msecs_to_jiffies(500));
> +	if (!ret) {
> +		dev_err(dev, "MPU<->CM3 sync failure\n");
> +		m3_ipc_state.state = M3_STATE_UNKNOWN;
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int wkup_m3_is_available(void)

should pass wkup_m3_ipc pointer as argument.

> +{
> +	return (m3_ipc_state.state != M3_STATE_RESET);
> +}
> +
> +/* Public functions */
> +/**
> + * wkup_m3_set_mem_type - Pass wkup_m3 which type of memory is in use
> + * @mem_type: memory type value read directly from emif
> + *
> + * wkup_m3 must know what memory type is in use to properly suspend
> + * and resume.
> + */
> +void wkup_m3_set_mem_type(int mem_type)
> +{
> +	m3_ipc_state.mem_type = mem_type;
> +}
> +
> +/**
> + * wkup_m3_set_resume_address - Pass wkup_m3 resume address
> + * @addr: Physical address from which resume code should execute
> + */
> +void wkup_m3_set_resume_address(void *addr)
> +{
> +	m3_ipc_state.resume_addr = (unsigned long)addr;
> +}
> +
> +/**
> + * wkup_m3_set_resume_address - Retrieve wkup_m3 status code after suspend
> + *
> + * Returns code representing the status of a low power mode transition.
> + *	0 - Successful transition
> + *	1 - Failure to transition to low power state
> + */
> +int wkup_m3_request_pm_status(void)
> +{
> +	unsigned int i;
> +	int val;
> +
> +	val = wkup_m3_ctrl_ipc_read(&m3_ipc_state, 1);
> +
> +	i = M3_STATUS_RESP_MASK & val;
> +	i >>= __ffs(M3_STATUS_RESP_MASK);
> +
> +	return i;
> +}
> +
> +/**
> + * wkup_m3_prepare_low_power - Request preparation for transition to
> + *			       low power state
> + * @state: A kernel suspend state to enter, either MEM or STANDBY
> + *
> + * Returns 0 if preparation was successful, otherwise returns error code
> + */
> +int wkup_m3_prepare_low_power(int state)
> +{
> +	struct device *dev = m3_ipc_state.dev;
> +	int m3_power_state;
> +	int ret = 0;
> +
> +	if (!wkup_m3_is_available()) {
> +		dev_err(dev, "wkup_m3 not available. DeepSleep entry not possible.\n");
> +		return -ENODEV;
> +	}
> +
> +	switch (state) {
> +	case PM_SUSPEND_MEM:
> +		m3_power_state = IPC_CMD_DS0;
> +		break;
> +	default:
> +		return 1;
> +	}
> +
> +	/* Program each required IPC register then write defaults to others */
> +	wkup_m3_ctrl_ipc_write(&m3_ipc_state, m3_ipc_state.resume_addr, 0);
> +	wkup_m3_ctrl_ipc_write(&m3_ipc_state, m3_power_state, 1);
> +	wkup_m3_ctrl_ipc_write(&m3_ipc_state, m3_ipc_state.mem_type, 4);
> +
> +	wkup_m3_ctrl_ipc_write(&m3_ipc_state, DS_IPC_DEFAULT, 2);
> +	wkup_m3_ctrl_ipc_write(&m3_ipc_state, DS_IPC_DEFAULT, 3);
> +	wkup_m3_ctrl_ipc_write(&m3_ipc_state, DS_IPC_DEFAULT, 5);
> +	wkup_m3_ctrl_ipc_write(&m3_ipc_state, DS_IPC_DEFAULT, 6);
> +	wkup_m3_ctrl_ipc_write(&m3_ipc_state, DS_IPC_DEFAULT, 7);
> +
> +	m3_ipc_state.state = M3_STATE_MSG_FOR_LP;
> +
> +	ret = wkup_m3_ping();
> +	if (ret) {
> +		dev_err(dev, "Unable to ping CM3\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * wkup_m3_finish_low_power - Return m3 to reset state
> + *
> + * Returns 0 if reset was successful, otherwise returns error code
> + */
> +int wkup_m3_finish_low_power(void)
> +{
> +	struct device *dev = m3_ipc_state.dev;
> +	int ret = 0;
> +
> +	if (!wkup_m3_is_available())
> +		return -ENODEV;
> +
> +	wkup_m3_ctrl_ipc_write(&m3_ipc_state, IPC_CMD_RESET, 1);
> +	wkup_m3_ctrl_ipc_write(&m3_ipc_state, DS_IPC_DEFAULT, 2);
> +
> +	m3_ipc_state.state = M3_STATE_MSG_FOR_RESET;
> +
> +	ret = wkup_m3_ping();
> +	if (ret) {
> +		dev_err(dev, "Unable to ping CM3\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}

I'm very much against exposed functions like this, but at least declare
them as EXPORT_SYMBOL_GPL().

> +static void wkup_m3_rproc_boot_thread(struct rproc *rproc)
> +{
> +	struct device *dev = &rproc->dev;
> +	int ret;
> +
> +	wait_for_completion(&rproc->firmware_loading_complete);
> +
> +	ret = rproc_boot(rproc);
> +	if (ret)
> +		dev_err(dev, "rproc_boot failed\n");
> +
> +	do_exit(0);
> +}
> +
> +static int wkup_m3_ipc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	int irq, ret;
> +	uint32_t rproc_phandle;
> +	struct rproc *m3_rproc;
> +	struct resource *res;
> +	struct task_struct *task;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ipc_regs");
> +	m3_ipc_state.ipc_mem_base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(m3_ipc_state.ipc_mem_base)) {
> +		dev_err(dev, "could not ioremap ipc_mem\n");
> +		ret = PTR_ERR(m3_ipc_state.ipc_mem_base);
> +		goto err;
> +	}
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (!irq) {
> +		dev_err(&pdev->dev, "no irq resource\n");
> +		ret = -ENXIO;
> +		goto err;
> +	}
> +
> +	ret = devm_request_irq(dev, irq, wkup_m3_txev_handler,
> +			       IRQF_DISABLED, "wkup_m3_txev", &m3_ipc_state);
> +	if (ret) {
> +		dev_err(dev, "request_irq failed\n");
> +		goto err;
> +	}
> +
> +	m3_ipc_state.mbox_client.dev = dev;
> +	m3_ipc_state.mbox_client.tx_done = NULL;
> +	m3_ipc_state.mbox_client.rx_callback = wkup_m3_mbox_callback;
> +	m3_ipc_state.mbox_client.tx_block = false;
> +	m3_ipc_state.mbox_client.knows_txdone = false;
> +
> +	m3_ipc_state.mbox = mbox_request_channel(&m3_ipc_state.mbox_client, 0);
> +	if (IS_ERR(m3_ipc_state.mbox)) {
> +		dev_err(dev, "IPC Request for A8->M3 Channel failed! %ld\n",
> +			PTR_ERR(m3_ipc_state.mbox));
> +		ret = PTR_ERR(m3_ipc_state.mbox);
> +		m3_ipc_state.mbox = NULL;
> +		return ret;
> +	}
> +
> +	if (of_property_read_u32(dev->of_node, "ti,rproc", &rproc_phandle)) {
> +		dev_err(&pdev->dev, "could not get rproc phandle\n");
> +		ret = -ENODEV;
> +		goto err;
> +	}
> +
> +	m3_rproc = rproc_get_by_phandle(rproc_phandle);
> +	if (!m3_rproc) {
> +		dev_err(&pdev->dev, "could not get rproc handle\n");
> +		ret = -EPROBE_DEFER;
> +		goto err;
> +	}
> +
> +	m3_ipc_state.rproc = m3_rproc;
> +	m3_ipc_state.dev = dev;
> +	m3_ipc_state.state = M3_STATE_RESET;
> +
> +	/*
> +	 * Wait for firmware loading completion in a thread so we
> +	 * can boot the wkup_m3 as soon as it's ready without holding
> +	 * up kernel boot
> +	 */
> +	task = kthread_run((void *)wkup_m3_rproc_boot_thread, m3_rproc,
> +			   "wkup_m3_rproc_loader");

I wonder two things:

1) This thread will only be used during boot up. Do we really need a
thread or would request_firmware_nowait() be enough ?

2) what's the size of the firmware ? Is it really so large that we must
run this as a separate thread ? Meaning, why isn't request_firmware()
enough ? How much time would we be waiting ?

> +
> +	if (IS_ERR(task)) {
> +		dev_err(dev, "can't create rproc_boot thread\n");
> +		goto err_put_rproc;
> +	}
> +
> +	return 0;
> +
> +err_put_rproc:
> +	rproc_put(m3_rproc);
> +err:
> +	mbox_free_channel(m3_ipc_state.mbox);
> +	return ret;
> +}
> +
> +static int wkup_m3_ipc_remove(struct platform_device *pdev)
> +{

	struct wkup_m3_ipc *m3 = platform_get_drvdata(pdev);

> +	if (m3_ipc_state.mbox)

if your request mailbox fails, you're bailing out on probe, this check
is useless.

> +		mbox_free_channel(m3_ipc_state.mbox);
> +
> +	if (m3_ipc_state.rproc) {

likewise.

> +		rproc_shutdown(m3_ipc_state.rproc);
> +		rproc_put(m3_ipc_state.rproc);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id wkup_m3_ipc_of_match[] = {
> +	{ .compatible = "ti,am3353-wkup-m3-ipc", .data = NULL, },

unnnecessary zero initialization.

> +	{},
> +};
> +
> +static struct platform_driver wkup_m3_ipc_driver = {
> +	.probe = wkup_m3_ipc_probe,
> +	.remove = wkup_m3_ipc_remove,
> +	.driver = {
> +		.name = "wkup_m3_ipc",
> +		.of_match_table = wkup_m3_ipc_of_match,
> +	},
> +};
> +
> +module_platform_driver(wkup_m3_ipc_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("wkup m3 remote processor ipc driver");

MODULE_AUTHOR() ??

> diff --git a/include/linux/wkup_m3_ipc.h b/include/linux/wkup_m3_ipc.h
> new file mode 100644
> index 0000000..a0c2722
> --- /dev/null
> +++ b/include/linux/wkup_m3_ipc.h
> @@ -0,0 +1,33 @@
> +/*
> + * TI Wakeup M3 for AMx3 SoCs Power Management Routines
> + *
> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/

actually, it's 2015 already, so you probably want:

* Copyright (C) 2013-2015 Texas Instruments Incorporated - http://www.ti.com/

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/3] soc: ti: Add wkup_m3_ipc driver

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 846 bytes --]

On Fri, Jan 02, 2015 at 02:16:43PM -0600, Felipe Balbi wrote:
> On Fri, Jan 02, 2015 at 02:00:16PM -0600, Dave Gerlach wrote:
> > Introduce a wkup_m3_ipc driver to handle communication between the MPU
> > and Cortex M3 wkup_m3 present on am335x.
> > 
> > This driver is responsible for actually booting the wkup_m3_rproc and
> > also handling all IPC which is done using the IPC registers in the control
> > module, a mailbox, and a separate interrupt back from the wkup_m3. A small
> > API is exposed for executing specific power commands, which include
> > configuring for low power mode, request a transition to a low power mode,
> > and status info on a previous transition.

another comment is that no user of such API is provided, so it's very
difficult to verify that there's no other way of implementing this.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/3] soc: ti: Add wkup_m3_ipc driver

[Dave Gerlach]

Felipe,
On 01/02/2015 02:16 PM, Felipe Balbi wrote:
> On Fri, Jan 02, 2015 at 02:00:16PM -0600, Dave Gerlach wrote:
>> Introduce a wkup_m3_ipc driver to handle communication between the MPU
>> and Cortex M3 wkup_m3 present on am335x.
>>
>> This driver is responsible for actually booting the wkup_m3_rproc and
>> also handling all IPC which is done using the IPC registers in the control
>> module, a mailbox, and a separate interrupt back from the wkup_m3. A small
>> API is exposed for executing specific power commands, which include
>> configuring for low power mode, request a transition to a low power mode,
>> and status info on a previous transition.
>>
>> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
>> ---
>>  drivers/soc/ti/Kconfig       |  11 ++
>>  drivers/soc/ti/Makefile      |   1 +
>>  drivers/soc/ti/wkup_m3_ipc.c | 451 +++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/wkup_m3_ipc.h  |  33 ++++
>>  4 files changed, 496 insertions(+)
>>  create mode 100644 drivers/soc/ti/wkup_m3_ipc.c
>>  create mode 100644 include/linux/wkup_m3_ipc.h
>>
>> diff --git a/drivers/soc/ti/Kconfig b/drivers/soc/ti/Kconfig
>> index 7266b21..61cda85 100644
>> --- a/drivers/soc/ti/Kconfig
>> +++ b/drivers/soc/ti/Kconfig
>> @@ -28,4 +28,15 @@ config KEYSTONE_NAVIGATOR_DMA
>>  
>>  	  If unsure, say N.
>>  
>> +config WKUP_M3_IPC
>> +	bool "TI AM33XX Wkup-M3 IPC Driver"
> 
> tristate ?

If we want to allow this and the rproc driver to be built as modules than we can
change this.

> 
>> +	depends on WKUP_M3_RPROC
>> +	select MAILBOX
>> +	select OMAP2PLUS_MBOX
> 
> selects are usually frowned upon.

Followed example set by OMAP_REMOTEPROC which selects the mailbox, thought that
would be alright.

> 
>> +	help
>> +	  TI AM33XX has a Cortex M3 to handle low power transitions. This IPC
>> +	  driver provides the necessary API to communicate and use the wkup m3
>> +	  for PM features like Suspend/Resume and boots the wkup_m3 using
>> +	  wkup_m3_rproc driver.
>> +
>>  endif # SOC_TI
>> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
>> index 6bed611..b6b8c8b 100644
>> --- a/drivers/soc/ti/Makefile
>> +++ b/drivers/soc/ti/Makefile
>> @@ -3,3 +3,4 @@
>>  #
>>  obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS)	+= knav_qmss_queue.o knav_qmss_acc.o
>>  obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA)	+= knav_dma.o
>> +obj-$(CONFIG_WKUP_M3_IPC)		+= wkup_m3_ipc.o
>> diff --git a/drivers/soc/ti/wkup_m3_ipc.c b/drivers/soc/ti/wkup_m3_ipc.c
>> new file mode 100644
>> index 0000000..4dcf748
>> --- /dev/null
>> +++ b/drivers/soc/ti/wkup_m3_ipc.c
>> @@ -0,0 +1,451 @@
>> +/*
>> + * AMx3 Wkup M3 IPC driver
>> + *
>> + * Copyright (C) 2014 Texas Instruments, Inc.
>> + *
>> + * Dave Gerlach <d-gerlach@ti.com>
>> + *
>> + * 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.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/kernel.h>
>> +#include <linux/kthread.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/mailbox_client.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/omap-mailbox.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/remoteproc.h>
>> +#include <linux/suspend.h>
>> +#include <linux/wkup_m3_ipc.h>
>> +
>> +#define AM33XX_CTRL_IPC_REG_COUNT	0x8
>> +#define AM33XX_CTRL_IPC_REG_OFFSET(m)	(0x4 + 4 * (m))
>> +
>> +/* AM33XX M3_TXEV_EOI register */
>> +#define AM33XX_CONTROL_M3_TXEV_EOI	0x00
>> +
>> +#define AM33XX_M3_TXEV_ACK		(0x1 << 0)
>> +#define AM33XX_M3_TXEV_ENABLE		(0x0 << 0)
>> +
>> +#define IPC_CMD_DS0			0x4
>> +#define IPC_CMD_STANDBY			0xc
>> +#define IPC_CMD_RESET			0xe
>> +#define DS_IPC_DEFAULT			0xffffffff
>> +#define M3_VERSION_UNKNOWN		0x0000ffff
>> +#define M3_BASELINE_VERSION		0x187
>> +#define M3_STATUS_RESP_MASK		(0xffff << 16)
>> +#define M3_FW_VERSION_MASK		0xffff
>> +
>> +#define M3_STATE_UNKNOWN		0
>> +#define M3_STATE_RESET			1
>> +#define M3_STATE_INITED			2
>> +#define M3_STATE_MSG_FOR_LP		3
>> +#define M3_STATE_MSG_FOR_RESET		4
>> +
>> +struct wkup_m3_ipc {
>> +	struct rproc *rproc;
>> +
>> +	void __iomem *ipc_mem_base;
>> +	struct device *dev;
>> +
>> +	int mem_type;
>> +	unsigned long resume_addr;
>> +	int state;
>> +
>> +	struct mbox_client mbox_client;
>> +	struct mbox_chan *mbox;
>> +};
>> +
>> +static struct wkup_m3_ipc m3_ipc_state;
>> +
>> +static DECLARE_COMPLETION(m3_ipc_sync);
> 
> either move this inside struct wkup_m3_ipc or make this
> DECLARE_COMPLETION_ONSTACK();

There is no reason this can't be in the struct, I will move it.

> 
>> +
>> +static inline void am33xx_txev_eoi(struct wkup_m3_ipc *m3_ipc)
>> +{
>> +	writel(AM33XX_M3_TXEV_ACK,
>> +	       m3_ipc->ipc_mem_base + AM33XX_CONTROL_M3_TXEV_EOI);
>> +}
>> +
>> +static inline void am33xx_txev_enable(struct wkup_m3_ipc *m3_ipc)
>> +{
>> +	writel(AM33XX_M3_TXEV_ENABLE,
>> +	       m3_ipc->ipc_mem_base + AM33XX_CONTROL_M3_TXEV_EOI);
>> +}
>> +
>> +static inline void wkup_m3_ctrl_ipc_write(struct wkup_m3_ipc *m3_ipc,
>> +					  u32 val, int ipc_reg_num)
>> +{
>> +	if (ipc_reg_num < 0 || ipc_reg_num > AM33XX_CTRL_IPC_REG_COUNT)
>> +		return;
> 
> you should probably add a WARN() here as this should never happen. Also,
> you might want to remove all "inline" modifiers and let compiler decide
> what to inline.

Good point about the warn, I should switch to that.

> 
>> +
>> +	writel(val, m3_ipc->ipc_mem_base +
>> +	       AM33XX_CTRL_IPC_REG_OFFSET(ipc_reg_num));
>> +}
>> +
>> +static inline unsigned int wkup_m3_ctrl_ipc_read(struct wkup_m3_ipc *m3_ipc,
>> +						 int ipc_reg_num)
>> +{
>> +	if (ipc_reg_num < 0 || ipc_reg_num > AM33XX_CTRL_IPC_REG_COUNT)
>> +		return 0;
>> +
>> +	return readl(m3_ipc->ipc_mem_base +
>> +		     AM33XX_CTRL_IPC_REG_OFFSET(ipc_reg_num));
>> +}
>> +
>> +static int wkup_m3_fw_version_read(void)
> 
> you can very easily pass struct wkup_m3_ipc pointer as argument here.

I can modify all of these static functions to take the wkup_m3_ipc pointer as an
argument.

> 
>> +{
>> +	int val;
>> +
>> +	val = wkup_m3_ctrl_ipc_read(&m3_ipc_state, 2);
>> +
>> +	return val & M3_FW_VERSION_MASK;
>> +}
>> +
>> +static irqreturn_t wkup_m3_txev_handler(int irq, void *ipc_data)
>> +{
>> +	struct wkup_m3_ipc *m3_ipc = (struct wkup_m3_ipc *)ipc_data;
> 
> unnecessary cast

Ok.

> 
>> +	struct device *dev = m3_ipc->dev;
>> +	int ver = 0;
>> +
>> +	am33xx_txev_eoi(m3_ipc);
>> +
>> +	switch (m3_ipc->state) {
>> +	case M3_STATE_RESET:
>> +		ver = wkup_m3_fw_version_read();
>> +
>> +		if (ver == M3_VERSION_UNKNOWN ||
>> +		    ver < M3_BASELINE_VERSION) {
>> +			dev_warn(dev, "CM3 Firmware Version %x not supported\n",
>> +				 ver);
>> +		} else {
>> +			dev_info(dev, "CM3 Firmware Version = 0x%x\n", ver);
>> +		}
>> +
>> +		m3_ipc->state = M3_STATE_INITED;
>> +		complete(&m3_ipc_sync);
>> +		break;
>> +	case M3_STATE_MSG_FOR_RESET:
>> +		m3_ipc->state = M3_STATE_INITED;
>> +		complete(&m3_ipc_sync);
>> +		break;
>> +	case M3_STATE_MSG_FOR_LP:
>> +		complete(&m3_ipc_sync);
>> +		break;
>> +	case M3_STATE_UNKNOWN:
>> +		dev_warn(dev, "Unknown CM3 State\n");
>> +	}
>> +
>> +	am33xx_txev_enable(m3_ipc);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static void wkup_m3_mbox_callback(struct mbox_client *client, void *data)
>> +{
>> +	struct wkup_m3_ipc *m3_ipc = container_of(client, struct wkup_m3_ipc,
>> +						  mbox_client);
>> +
>> +	omap_mbox_disable_irq(m3_ipc->mbox, IRQ_RX);
>> +}
>> +
>> +static int wkup_m3_ping(void)
> 
> same here, pass wkup_m3_ipc pointer as argument.

Yep.

> 
>> +{
>> +	struct device *dev = m3_ipc_state.dev;
>> +	mbox_msg_t dummy_msg = 0;
>> +	int ret;
>> +
>> +	if (!m3_ipc_state.mbox) {
>> +		dev_err(m3_ipc_state.dev,
> 
> you already have a local dev pointer, why don't you use it ?

Thanks for catching this, just a mistake on my part.

> 
>> +			"No IPC channel to communicate with wkup_m3!\n");
>> +		return -EIO;
>> +	}
>> +
>> +	/*
>> +	 * Write a dummy message to the mailbox in order to trigger the RX
>> +	 * interrupt to alert the M3 that data is available in the IPC
>> +	 * registers. We must enable the IRQ here and disable it after in
>> +	 * the RX callback to avoid multiple interrupts being received
>> +	 * by the CM3.
>> +	 */
>> +	omap_mbox_enable_irq(m3_ipc_state.mbox, IRQ_RX);
>> +	ret = mbox_send_message(m3_ipc_state.mbox, (void *)dummy_msg);
> 
> unnecessary cast.

I get a compiler warning without this one, may need it.

> 
>> +	if (ret < 0) {
>> +		dev_err(dev, "%s: mbox_send_message() failed: %d\n",
>> +			__func__, ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = wait_for_completion_timeout(&m3_ipc_sync,
> 
> yeah, why couldn't you ?
> 
> 	wait_for_completion_timeout(&m3_ipc_state->complete, [...]
> 
> why does it have to be declared on the stack as a global variable ?

As I mentioned before, it doesn't and your approach is better.

> 
>> +					  msecs_to_jiffies(500));
>> +	if (!ret) {
>> +		dev_err(dev, "MPU<->CM3 sync failure\n");
>> +		m3_ipc_state.state = M3_STATE_UNKNOWN;
>> +		return -EIO;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int wkup_m3_is_available(void)
> 
> should pass wkup_m3_ipc pointer as argument.

Yep.

> 
>> +{
>> +	return (m3_ipc_state.state != M3_STATE_RESET);
>> +}
>> +
>> +/* Public functions */
>> +/**
>> + * wkup_m3_set_mem_type - Pass wkup_m3 which type of memory is in use
>> + * @mem_type: memory type value read directly from emif
>> + *
>> + * wkup_m3 must know what memory type is in use to properly suspend
>> + * and resume.
>> + */
>> +void wkup_m3_set_mem_type(int mem_type)
>> +{
>> +	m3_ipc_state.mem_type = mem_type;
>> +}
>> +
>> +/**
>> + * wkup_m3_set_resume_address - Pass wkup_m3 resume address
>> + * @addr: Physical address from which resume code should execute
>> + */
>> +void wkup_m3_set_resume_address(void *addr)
>> +{
>> +	m3_ipc_state.resume_addr = (unsigned long)addr;
>> +}
>> +
>> +/**
>> + * wkup_m3_set_resume_address - Retrieve wkup_m3 status code after suspend
>> + *
>> + * Returns code representing the status of a low power mode transition.
>> + *	0 - Successful transition
>> + *	1 - Failure to transition to low power state
>> + */
>> +int wkup_m3_request_pm_status(void)
>> +{
>> +	unsigned int i;
>> +	int val;
>> +
>> +	val = wkup_m3_ctrl_ipc_read(&m3_ipc_state, 1);
>> +
>> +	i = M3_STATUS_RESP_MASK & val;
>> +	i >>= __ffs(M3_STATUS_RESP_MASK);
>> +
>> +	return i;
>> +}
>> +
>> +/**
>> + * wkup_m3_prepare_low_power - Request preparation for transition to
>> + *			       low power state
>> + * @state: A kernel suspend state to enter, either MEM or STANDBY
>> + *
>> + * Returns 0 if preparation was successful, otherwise returns error code
>> + */
>> +int wkup_m3_prepare_low_power(int state)
>> +{
>> +	struct device *dev = m3_ipc_state.dev;
>> +	int m3_power_state;
>> +	int ret = 0;
>> +
>> +	if (!wkup_m3_is_available()) {
>> +		dev_err(dev, "wkup_m3 not available. DeepSleep entry not possible.\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	switch (state) {
>> +	case PM_SUSPEND_MEM:
>> +		m3_power_state = IPC_CMD_DS0;
>> +		break;
>> +	default:
>> +		return 1;
>> +	}
>> +
>> +	/* Program each required IPC register then write defaults to others */
>> +	wkup_m3_ctrl_ipc_write(&m3_ipc_state, m3_ipc_state.resume_addr, 0);
>> +	wkup_m3_ctrl_ipc_write(&m3_ipc_state, m3_power_state, 1);
>> +	wkup_m3_ctrl_ipc_write(&m3_ipc_state, m3_ipc_state.mem_type, 4);
>> +
>> +	wkup_m3_ctrl_ipc_write(&m3_ipc_state, DS_IPC_DEFAULT, 2);
>> +	wkup_m3_ctrl_ipc_write(&m3_ipc_state, DS_IPC_DEFAULT, 3);
>> +	wkup_m3_ctrl_ipc_write(&m3_ipc_state, DS_IPC_DEFAULT, 5);
>> +	wkup_m3_ctrl_ipc_write(&m3_ipc_state, DS_IPC_DEFAULT, 6);
>> +	wkup_m3_ctrl_ipc_write(&m3_ipc_state, DS_IPC_DEFAULT, 7);
>> +
>> +	m3_ipc_state.state = M3_STATE_MSG_FOR_LP;
>> +
>> +	ret = wkup_m3_ping();
>> +	if (ret) {
>> +		dev_err(dev, "Unable to ping CM3\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * wkup_m3_finish_low_power - Return m3 to reset state
>> + *
>> + * Returns 0 if reset was successful, otherwise returns error code
>> + */
>> +int wkup_m3_finish_low_power(void)
>> +{
>> +	struct device *dev = m3_ipc_state.dev;
>> +	int ret = 0;
>> +
>> +	if (!wkup_m3_is_available())
>> +		return -ENODEV;
>> +
>> +	wkup_m3_ctrl_ipc_write(&m3_ipc_state, IPC_CMD_RESET, 1);
>> +	wkup_m3_ctrl_ipc_write(&m3_ipc_state, DS_IPC_DEFAULT, 2);
>> +
>> +	m3_ipc_state.state = M3_STATE_MSG_FOR_RESET;
>> +
>> +	ret = wkup_m3_ping();
>> +	if (ret) {
>> +		dev_err(dev, "Unable to ping CM3\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
> 
> I'm very much against exposed functions like this, but at least declare
> them as EXPORT_SYMBOL_GPL().

Alright, if we do plan to make this buildable as a module then I'll have to add
these anyway.
> 
>> +static void wkup_m3_rproc_boot_thread(struct rproc *rproc)
>> +{
>> +	struct device *dev = &rproc->dev;
>> +	int ret;
>> +
>> +	wait_for_completion(&rproc->firmware_loading_complete);
>> +
>> +	ret = rproc_boot(rproc);
>> +	if (ret)
>> +		dev_err(dev, "rproc_boot failed\n");
>> +
>> +	do_exit(0);
>> +}
>> +
>> +static int wkup_m3_ipc_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	int irq, ret;
>> +	uint32_t rproc_phandle;
>> +	struct rproc *m3_rproc;
>> +	struct resource *res;
>> +	struct task_struct *task;
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ipc_regs");
>> +	m3_ipc_state.ipc_mem_base = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(m3_ipc_state.ipc_mem_base)) {
>> +		dev_err(dev, "could not ioremap ipc_mem\n");
>> +		ret = PTR_ERR(m3_ipc_state.ipc_mem_base);
>> +		goto err;
>> +	}
>> +
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (!irq) {
>> +		dev_err(&pdev->dev, "no irq resource\n");
>> +		ret = -ENXIO;
>> +		goto err;
>> +	}
>> +
>> +	ret = devm_request_irq(dev, irq, wkup_m3_txev_handler,
>> +			       IRQF_DISABLED, "wkup_m3_txev", &m3_ipc_state);
>> +	if (ret) {
>> +		dev_err(dev, "request_irq failed\n");
>> +		goto err;
>> +	}
>> +
>> +	m3_ipc_state.mbox_client.dev = dev;
>> +	m3_ipc_state.mbox_client.tx_done = NULL;
>> +	m3_ipc_state.mbox_client.rx_callback = wkup_m3_mbox_callback;
>> +	m3_ipc_state.mbox_client.tx_block = false;
>> +	m3_ipc_state.mbox_client.knows_txdone = false;
>> +
>> +	m3_ipc_state.mbox = mbox_request_channel(&m3_ipc_state.mbox_client, 0);
>> +	if (IS_ERR(m3_ipc_state.mbox)) {
>> +		dev_err(dev, "IPC Request for A8->M3 Channel failed! %ld\n",
>> +			PTR_ERR(m3_ipc_state.mbox));
>> +		ret = PTR_ERR(m3_ipc_state.mbox);
>> +		m3_ipc_state.mbox = NULL;
>> +		return ret;
>> +	}
>> +
>> +	if (of_property_read_u32(dev->of_node, "ti,rproc", &rproc_phandle)) {
>> +		dev_err(&pdev->dev, "could not get rproc phandle\n");
>> +		ret = -ENODEV;
>> +		goto err;
>> +	}
>> +
>> +	m3_rproc = rproc_get_by_phandle(rproc_phandle);
>> +	if (!m3_rproc) {
>> +		dev_err(&pdev->dev, "could not get rproc handle\n");
>> +		ret = -EPROBE_DEFER;
>> +		goto err;
>> +	}
>> +
>> +	m3_ipc_state.rproc = m3_rproc;
>> +	m3_ipc_state.dev = dev;
>> +	m3_ipc_state.state = M3_STATE_RESET;
>> +
>> +	/*
>> +	 * Wait for firmware loading completion in a thread so we
>> +	 * can boot the wkup_m3 as soon as it's ready without holding
>> +	 * up kernel boot
>> +	 */
>> +	task = kthread_run((void *)wkup_m3_rproc_boot_thread, m3_rproc,
>> +			   "wkup_m3_rproc_loader");
> 
> I wonder two things:
> 
> 1) This thread will only be used during boot up. Do we really need a
> thread or would request_firmware_nowait() be enough ?
> 
> 2) what's the size of the firmware ? Is it really so large that we must
> run this as a separate thread ? Meaning, why isn't request_firmware()
> enough ? How much time would we be waiting ?

The issue here comes from the case where you attempt to load a firmware stored
in the rootfs which is the typical use case for this. Remoteproc core requests
the firmware twice, first with _nowait to load the resource table and then again
without nowait to boot the rproc. rproc_boot requires the resource table to be
loaded. The thread is really to wait for the firmware loaded completion, which
gets set after the resource table has been loaded, to let boot proceed. System
boot will get stuck otherwise as this driver can probe before rootfs is available.

> 
>> +
>> +	if (IS_ERR(task)) {
>> +		dev_err(dev, "can't create rproc_boot thread\n");
>> +		goto err_put_rproc;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_put_rproc:
>> +	rproc_put(m3_rproc);
>> +err:
>> +	mbox_free_channel(m3_ipc_state.mbox);
>> +	return ret;
>> +}
>> +
>> +static int wkup_m3_ipc_remove(struct platform_device *pdev)
>> +{
> 
> 	struct wkup_m3_ipc *m3 = platform_get_drvdata(pdev);
> 
>> +	if (m3_ipc_state.mbox)
> 
> if your request mailbox fails, you're bailing out on probe, this check
> is useless.
> 
>> +		mbox_free_channel(m3_ipc_state.mbox);
>> +
>> +	if (m3_ipc_state.rproc) {
> 
> likewise.

Good points, can drop those.

> 
>> +		rproc_shutdown(m3_ipc_state.rproc);
>> +		rproc_put(m3_ipc_state.rproc);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id wkup_m3_ipc_of_match[] = {
>> +	{ .compatible = "ti,am3353-wkup-m3-ipc", .data = NULL, },
> 
> unnnecessary zero initialization.

Yes, will fix.

> 
>> +	{},
>> +};
>> +
>> +static struct platform_driver wkup_m3_ipc_driver = {
>> +	.probe = wkup_m3_ipc_probe,
>> +	.remove = wkup_m3_ipc_remove,
>> +	.driver = {
>> +		.name = "wkup_m3_ipc",
>> +		.of_match_table = wkup_m3_ipc_of_match,
>> +	},
>> +};
>> +
>> +module_platform_driver(wkup_m3_ipc_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("wkup m3 remote processor ipc driver");
> 
> MODULE_AUTHOR() ??

Yeah I'll add it.

> 
>> diff --git a/include/linux/wkup_m3_ipc.h b/include/linux/wkup_m3_ipc.h
>> new file mode 100644
>> index 0000000..a0c2722
>> --- /dev/null
>> +++ b/include/linux/wkup_m3_ipc.h
>> @@ -0,0 +1,33 @@
>> +/*
>> + * TI Wakeup M3 for AMx3 SoCs Power Management Routines
>> + *
>> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
> 
> actually, it's 2015 already, so you probably want:
> 
> * Copyright (C) 2013-2015 Texas Instruments Incorporated - http://www.ti.com/
> 

Whoops, seems I lost track of time there, thansk for the comments.

Regards,
Dave

Re: [PATCH 2/3] soc: ti: Add wkup_m3_ipc driver

[Tony Lindgren]

* Dave Gerlach <d-gerlach-l0cyMroinI0@public.gmane.org> [150105 14:51]:
> Felipe,
> On 01/02/2015 02:16 PM, Felipe Balbi wrote:
> > On Fri, Jan 02, 2015 at 02:00:16PM -0600, Dave Gerlach wrote:
> >> Introduce a wkup_m3_ipc driver to handle communication between the MPU
> >> and Cortex M3 wkup_m3 present on am335x.
> >>
> >> This driver is responsible for actually booting the wkup_m3_rproc and
> >> also handling all IPC which is done using the IPC registers in the control
> >> module, a mailbox, and a separate interrupt back from the wkup_m3. A small
> >> API is exposed for executing specific power commands, which include
> >> configuring for low power mode, request a transition to a low power mode,
> >> and status info on a previous transition.
> >>
> >> Signed-off-by: Dave Gerlach <d-gerlach-l0cyMroinI0@public.gmane.org>
> >> ---
> >>  drivers/soc/ti/Kconfig       |  11 ++
> >>  drivers/soc/ti/Makefile      |   1 +
> >>  drivers/soc/ti/wkup_m3_ipc.c | 451 +++++++++++++++++++++++++++++++++++++++++++
> >>  include/linux/wkup_m3_ipc.h  |  33 ++++
> >>  4 files changed, 496 insertions(+)
> >>  create mode 100644 drivers/soc/ti/wkup_m3_ipc.c
> >>  create mode 100644 include/linux/wkup_m3_ipc.h
> >>
> >> diff --git a/drivers/soc/ti/Kconfig b/drivers/soc/ti/Kconfig
> >> index 7266b21..61cda85 100644
> >> --- a/drivers/soc/ti/Kconfig
> >> +++ b/drivers/soc/ti/Kconfig
> >> @@ -28,4 +28,15 @@ config KEYSTONE_NAVIGATOR_DMA
> >>  
> >>  	  If unsure, say N.
> >>  
> >> +config WKUP_M3_IPC
> >> +	bool "TI AM33XX Wkup-M3 IPC Driver"
> > 
> > tristate ?
> 
> If we want to allow this and the rproc driver to be built as modules than we can
> change this.

Yes please, the PM is never needed early, and should be optional.
And doing that will make it a lot easier for you to do further work
on your driver ;)

And it will also make it easier to add support for other SoCs as
it seems the same M3 is used at least on am437x.
 
> > 
> >> +	depends on WKUP_M3_RPROC
> >> +	select MAILBOX
> >> +	select OMAP2PLUS_MBOX
> > 
> > selects are usually frowned upon.
> 
> Followed example set by OMAP_REMOTEPROC which selects the mailbox, thought that
> would be alright.

The select should be only done if the option selected is a silen
Kconfig option where it's never selectable by the user. Otherwise
you will errors sooner or later with make randconfig builds as the
dependencies change. Using depends on is better for those cases.

Regards,

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

Re: [PATCH 2/3] soc: ti: Add wkup_m3_ipc driver

[Dave Gerlach]

Tony,
On 01/05/2015 04:51 PM, Tony Lindgren wrote:
> * Dave Gerlach <d-gerlach@ti.com> [150105 14:51]:
>> Felipe,
>> On 01/02/2015 02:16 PM, Felipe Balbi wrote:
>>> On Fri, Jan 02, 2015 at 02:00:16PM -0600, Dave Gerlach wrote:
>>>> Introduce a wkup_m3_ipc driver to handle communication between the MPU
>>>> and Cortex M3 wkup_m3 present on am335x.
>>>>
>>>> This driver is responsible for actually booting the wkup_m3_rproc and
>>>> also handling all IPC which is done using the IPC registers in the control
>>>> module, a mailbox, and a separate interrupt back from the wkup_m3. A small
>>>> API is exposed for executing specific power commands, which include
>>>> configuring for low power mode, request a transition to a low power mode,
>>>> and status info on a previous transition.
>>>>
>>>> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
>>>> ---
>>>>  drivers/soc/ti/Kconfig       |  11 ++
>>>>  drivers/soc/ti/Makefile      |   1 +
>>>>  drivers/soc/ti/wkup_m3_ipc.c | 451 +++++++++++++++++++++++++++++++++++++++++++
>>>>  include/linux/wkup_m3_ipc.h  |  33 ++++
>>>>  4 files changed, 496 insertions(+)
>>>>  create mode 100644 drivers/soc/ti/wkup_m3_ipc.c
>>>>  create mode 100644 include/linux/wkup_m3_ipc.h
>>>>
>>>> diff --git a/drivers/soc/ti/Kconfig b/drivers/soc/ti/Kconfig
>>>> index 7266b21..61cda85 100644
>>>> --- a/drivers/soc/ti/Kconfig
>>>> +++ b/drivers/soc/ti/Kconfig
>>>> @@ -28,4 +28,15 @@ config KEYSTONE_NAVIGATOR_DMA
>>>>  
>>>>  	  If unsure, say N.
>>>>  
>>>> +config WKUP_M3_IPC
>>>> +	bool "TI AM33XX Wkup-M3 IPC Driver"
>>>
>>> tristate ?
>>
>> If we want to allow this and the rproc driver to be built as modules than we can
>> change this.
> 
> Yes please, the PM is never needed early, and should be optional.
> And doing that will make it a lot easier for you to do further work
> on your driver ;)
> 
> And it will also make it easier to add support for other SoCs as
> it seems the same M3 is used at least on am437x.
>  

I can not build the PM code as a module at this time due to many mach-omap
function calls it uses which are not exported, so I need handles to all five
functions in this driver used in pm33xx to call from built-in PM code. Do you
have a preference on how these function handles get passed?

I currently have added a pdata-quirks implementation that passes a function to
the wkup_m3_ipc driver through it's pdata which it then calls at probe to pass a
struct containing all used function pointers to the pm code that were previously
called directly. Is that what you would prefer or something else? I had also
looked at making the struct of function pointers in the driver global and just
picking it up from the pm code with an extern declaration or even putting a bus
notifier in the PM code and watching for the wkup_m3_ipc driver to be bound.

Thought I would see what you prefer and possibly avoid an unnecessary re-spin.

Regards,
Dave

>>>
>>>> +	depends on WKUP_M3_RPROC
>>>> +	select MAILBOX
>>>> +	select OMAP2PLUS_MBOX
>>>
>>> selects are usually frowned upon.
>>
>> Followed example set by OMAP_REMOTEPROC which selects the mailbox, thought that
>> would be alright.
> 
> The select should be only done if the option selected is a silen
> Kconfig option where it's never selectable by the user. Otherwise
> you will errors sooner or later with make randconfig builds as the
> dependencies change. Using depends on is better for those cases.
> 
> Regards,
> 
> Tony 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/3] soc: ti: Add wkup_m3_ipc driver

[Tony Lindgren]

* Dave Gerlach <d-gerlach@ti.com> [150226 12:05]:
> Tony,
> On 01/05/2015 04:51 PM, Tony Lindgren wrote:
> > * Dave Gerlach <d-gerlach@ti.com> [150105 14:51]:
> >> Felipe,
> >> On 01/02/2015 02:16 PM, Felipe Balbi wrote:
> >>> On Fri, Jan 02, 2015 at 02:00:16PM -0600, Dave Gerlach wrote:
> >>>> Introduce a wkup_m3_ipc driver to handle communication between the MPU
> >>>> and Cortex M3 wkup_m3 present on am335x.
> >>>>
> >>>> This driver is responsible for actually booting the wkup_m3_rproc and
> >>>> also handling all IPC which is done using the IPC registers in the control
> >>>> module, a mailbox, and a separate interrupt back from the wkup_m3. A small
> >>>> API is exposed for executing specific power commands, which include
> >>>> configuring for low power mode, request a transition to a low power mode,
> >>>> and status info on a previous transition.
> >>>>
> >>>> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
> >>>> ---
> >>>>  drivers/soc/ti/Kconfig       |  11 ++
> >>>>  drivers/soc/ti/Makefile      |   1 +
> >>>>  drivers/soc/ti/wkup_m3_ipc.c | 451 +++++++++++++++++++++++++++++++++++++++++++
> >>>>  include/linux/wkup_m3_ipc.h  |  33 ++++
> >>>>  4 files changed, 496 insertions(+)
> >>>>  create mode 100644 drivers/soc/ti/wkup_m3_ipc.c
> >>>>  create mode 100644 include/linux/wkup_m3_ipc.h
> >>>>
> >>>> diff --git a/drivers/soc/ti/Kconfig b/drivers/soc/ti/Kconfig
> >>>> index 7266b21..61cda85 100644
> >>>> --- a/drivers/soc/ti/Kconfig
> >>>> +++ b/drivers/soc/ti/Kconfig
> >>>> @@ -28,4 +28,15 @@ config KEYSTONE_NAVIGATOR_DMA
> >>>>  
> >>>>  	  If unsure, say N.
> >>>>  
> >>>> +config WKUP_M3_IPC
> >>>> +	bool "TI AM33XX Wkup-M3 IPC Driver"
> >>>
> >>> tristate ?
> >>
> >> If we want to allow this and the rproc driver to be built as modules than we can
> >> change this.
> > 
> > Yes please, the PM is never needed early, and should be optional.
> > And doing that will make it a lot easier for you to do further work
> > on your driver ;)
> > 
> > And it will also make it easier to add support for other SoCs as
> > it seems the same M3 is used at least on am437x.
> >  
> 
> I can not build the PM code as a module at this time due to many mach-omap
> function calls it uses which are not exported, so I need handles to all five
> functions in this driver used in pm33xx to call from built-in PM code. Do you
> have a preference on how these function handles get passed?

OK, you can pass the function pointers in platform_data. Care to
list those functions? That might allow us to make other omap PM
code into loadable modules in drivers/* :)
 
> I currently have added a pdata-quirks implementation that passes a function to
> the wkup_m3_ipc driver through it's pdata which it then calls at probe to pass a
> struct containing all used function pointers to the pm code that were previously
> called directly. Is that what you would prefer or something else? I had also
> looked at making the struct of function pointers in the driver global and just
> picking it up from the pm code with an extern declaration or even putting a bus
> notifier in the PM code and watching for the wkup_m3_ipc driver to be bound.

Yeah pdata-quirks.c is probably OK for populating the function pointers.
We may have already some place in the PM code to pass it too.
 
> Thought I would see what you prefer and possibly avoid an unnecessary re-spin.

Sounds like we're getting close to getting am335x/am437x PM code working :)

Regards,

Tony

Re: [PATCH 2/3] soc: ti: Add wkup_m3_ipc driver

[Dave Gerlach]

Tony,
On 02/26/2015 04:08 PM, Tony Lindgren wrote:
> * Dave Gerlach <d-gerlach-l0cyMroinI0@public.gmane.org> [150226 12:05]:
>> Tony,
>> On 01/05/2015 04:51 PM, Tony Lindgren wrote:
>>> * Dave Gerlach <d-gerlach-l0cyMroinI0@public.gmane.org> [150105 14:51]:
>>>> Felipe,
>>>> On 01/02/2015 02:16 PM, Felipe Balbi wrote:
>>>>> On Fri, Jan 02, 2015 at 02:00:16PM -0600, Dave Gerlach wrote:
>>>>>> Introduce a wkup_m3_ipc driver to handle communication between the MPU
>>>>>> and Cortex M3 wkup_m3 present on am335x.
>>>>>>
>>>>>> This driver is responsible for actually booting the wkup_m3_rproc and
>>>>>> also handling all IPC which is done using the IPC registers in the control
>>>>>> module, a mailbox, and a separate interrupt back from the wkup_m3. A small
>>>>>> API is exposed for executing specific power commands, which include
>>>>>> configuring for low power mode, request a transition to a low power mode,
>>>>>> and status info on a previous transition.
>>>>>>
>>>>>> Signed-off-by: Dave Gerlach <d-gerlach-l0cyMroinI0@public.gmane.org>
>>>>>> ---
>>>>>>  drivers/soc/ti/Kconfig       |  11 ++
>>>>>>  drivers/soc/ti/Makefile      |   1 +
>>>>>>  drivers/soc/ti/wkup_m3_ipc.c | 451 +++++++++++++++++++++++++++++++++++++++++++
>>>>>>  include/linux/wkup_m3_ipc.h  |  33 ++++
>>>>>>  4 files changed, 496 insertions(+)
>>>>>>  create mode 100644 drivers/soc/ti/wkup_m3_ipc.c
>>>>>>  create mode 100644 include/linux/wkup_m3_ipc.h
>>>>>>
>>>>>> diff --git a/drivers/soc/ti/Kconfig b/drivers/soc/ti/Kconfig
>>>>>> index 7266b21..61cda85 100644
>>>>>> --- a/drivers/soc/ti/Kconfig
>>>>>> +++ b/drivers/soc/ti/Kconfig
>>>>>> @@ -28,4 +28,15 @@ config KEYSTONE_NAVIGATOR_DMA
>>>>>>  
>>>>>>  	  If unsure, say N.
>>>>>>  
>>>>>> +config WKUP_M3_IPC
>>>>>> +	bool "TI AM33XX Wkup-M3 IPC Driver"
>>>>>
>>>>> tristate ?
>>>>
>>>> If we want to allow this and the rproc driver to be built as modules than we can
>>>> change this.
>>>
>>> Yes please, the PM is never needed early, and should be optional.
>>> And doing that will make it a lot easier for you to do further work
>>> on your driver ;)
>>>
>>> And it will also make it easier to add support for other SoCs as
>>> it seems the same M3 is used at least on am437x.
>>>  
>>
>> I can not build the PM code as a module at this time due to many mach-omap
>> function calls it uses which are not exported, so I need handles to all five
>> functions in this driver used in pm33xx to call from built-in PM code. Do you
>> have a preference on how these function handles get passed?
> 
> OK, you can pass the function pointers in platform_data. Care to
> list those functions? That might allow us to make other omap PM
> code into loadable modules in drivers/* :)

Sure, apart from the dependencies I create for myself in other modules that I
introduce I see:

ERROR: "omap_pm_clkdms_setup" [arch/arm/mach-omap2/pm33xx.ko] undefined!
ERROR: "clkdm_for_each" [arch/arm/mach-omap2/pm33xx.ko] undefined!
ERROR: "clkdm_lookup" [arch/arm/mach-omap2/pm33xx.ko] undefined!
ERROR: "pwrdm_lookup" [arch/arm/mach-omap2/pm33xx.ko] undefined!
ERROR: "pwrdm_post_transition" [arch/arm/mach-omap2/pm33xx.ko] undefined!
ERROR: "clkdm_sleep" [arch/arm/mach-omap2/pm33xx.ko] undefined!
ERROR: "clkdm_wakeup" [arch/arm/mach-omap2/pm33xx.ko] undefined!
ERROR: "pwrdm_read_pwrst" [arch/arm/mach-omap2/pm33xx.ko] undefined!
ERROR: "cpu_suspend" [arch/arm/mach-omap2/pm33xx.ko] undefined!
ERROR: "omap_set_pwrdm_state" [arch/arm/mach-omap2/pm33xx.ko] undefined!

So the mach-omap2 clockdomain and power domain functions along with arch arm
cpu_suspend code. I see similar dependencies in the other mach-omap2/pmxxxx.c
code also.

>  
>> I currently have added a pdata-quirks implementation that passes a function to
>> the wkup_m3_ipc driver through it's pdata which it then calls at probe to pass a
>> struct containing all used function pointers to the pm code that were previously
>> called directly. Is that what you would prefer or something else? I had also
>> looked at making the struct of function pointers in the driver global and just
>> picking it up from the pm code with an extern declaration or even putting a bus
>> notifier in the PM code and watching for the wkup_m3_ipc driver to be bound.
> 
> Yeah pdata-quirks.c is probably OK for populating the function pointers.
> We may have already some place in the PM code to pass it too.
>  

Alright, thanks for your input.

>> Thought I would see what you prefer and possibly avoid an unnecessary re-spin.
> 
> Sounds like we're getting close to getting am335x/am437x PM code working :)

Getting there!

Regards,
Dave

> 
> Regards,
> 
> Tony
> 

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

Re: [PATCH 2/3] soc: ti: Add wkup_m3_ipc driver

[Tony Lindgren]

* Dave Gerlach <d-gerlach@ti.com> [150227 09:04]:
> Tony,
> On 02/26/2015 04:08 PM, Tony Lindgren wrote:
> > * Dave Gerlach <d-gerlach@ti.com> [150226 12:05]:
> >> Tony,
> >> On 01/05/2015 04:51 PM, Tony Lindgren wrote:
> >>> * Dave Gerlach <d-gerlach@ti.com> [150105 14:51]:
> >>>> Felipe,
> >>>> On 01/02/2015 02:16 PM, Felipe Balbi wrote:
> >>>>> On Fri, Jan 02, 2015 at 02:00:16PM -0600, Dave Gerlach wrote:
> >>>>>> Introduce a wkup_m3_ipc driver to handle communication between the MPU
> >>>>>> and Cortex M3 wkup_m3 present on am335x.
> >>>>>>
> >>>>>> This driver is responsible for actually booting the wkup_m3_rproc and
> >>>>>> also handling all IPC which is done using the IPC registers in the control
> >>>>>> module, a mailbox, and a separate interrupt back from the wkup_m3. A small
> >>>>>> API is exposed for executing specific power commands, which include
> >>>>>> configuring for low power mode, request a transition to a low power mode,
> >>>>>> and status info on a previous transition.
> >>>>>>
> >>>>>> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
> >>>>>> ---
> >>>>>>  drivers/soc/ti/Kconfig       |  11 ++
> >>>>>>  drivers/soc/ti/Makefile      |   1 +
> >>>>>>  drivers/soc/ti/wkup_m3_ipc.c | 451 +++++++++++++++++++++++++++++++++++++++++++
> >>>>>>  include/linux/wkup_m3_ipc.h  |  33 ++++
> >>>>>>  4 files changed, 496 insertions(+)
> >>>>>>  create mode 100644 drivers/soc/ti/wkup_m3_ipc.c
> >>>>>>  create mode 100644 include/linux/wkup_m3_ipc.h
> >>>>>>
> >>>>>> diff --git a/drivers/soc/ti/Kconfig b/drivers/soc/ti/Kconfig
> >>>>>> index 7266b21..61cda85 100644
> >>>>>> --- a/drivers/soc/ti/Kconfig
> >>>>>> +++ b/drivers/soc/ti/Kconfig
> >>>>>> @@ -28,4 +28,15 @@ config KEYSTONE_NAVIGATOR_DMA
> >>>>>>  
> >>>>>>  	  If unsure, say N.
> >>>>>>  
> >>>>>> +config WKUP_M3_IPC
> >>>>>> +	bool "TI AM33XX Wkup-M3 IPC Driver"
> >>>>>
> >>>>> tristate ?
> >>>>
> >>>> If we want to allow this and the rproc driver to be built as modules than we can
> >>>> change this.
> >>>
> >>> Yes please, the PM is never needed early, and should be optional.
> >>> And doing that will make it a lot easier for you to do further work
> >>> on your driver ;)
> >>>
> >>> And it will also make it easier to add support for other SoCs as
> >>> it seems the same M3 is used at least on am437x.
> >>>  
> >>
> >> I can not build the PM code as a module at this time due to many mach-omap
> >> function calls it uses which are not exported, so I need handles to all five
> >> functions in this driver used in pm33xx to call from built-in PM code. Do you
> >> have a preference on how these function handles get passed?
> > 
> > OK, you can pass the function pointers in platform_data. Care to
> > list those functions? That might allow us to make other omap PM
> > code into loadable modules in drivers/* :)
> 
> Sure, apart from the dependencies I create for myself in other modules that I
> introduce I see:
> 
> ERROR: "omap_pm_clkdms_setup" [arch/arm/mach-omap2/pm33xx.ko] undefined!
> ERROR: "clkdm_for_each" [arch/arm/mach-omap2/pm33xx.ko] undefined!
> ERROR: "clkdm_lookup" [arch/arm/mach-omap2/pm33xx.ko] undefined!
> ERROR: "pwrdm_lookup" [arch/arm/mach-omap2/pm33xx.ko] undefined!
> ERROR: "pwrdm_post_transition" [arch/arm/mach-omap2/pm33xx.ko] undefined!
> ERROR: "clkdm_sleep" [arch/arm/mach-omap2/pm33xx.ko] undefined!
> ERROR: "clkdm_wakeup" [arch/arm/mach-omap2/pm33xx.ko] undefined!
> ERROR: "pwrdm_read_pwrst" [arch/arm/mach-omap2/pm33xx.ko] undefined!
> ERROR: "cpu_suspend" [arch/arm/mach-omap2/pm33xx.ko] undefined!
> ERROR: "omap_set_pwrdm_state" [arch/arm/mach-omap2/pm33xx.ko] undefined!
> 
> So the mach-omap2 clockdomain and power domain functions along with arch arm
> cpu_suspend code. I see similar dependencies in the other mach-omap2/pmxxxx.c
> code also.

OK thanks for the list.

Regards,

Tony

Re: [PATCH 2/3] soc: ti: Add wkup_m3_ipc driver

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2774 bytes --]

Hi,

On Mon, Jan 05, 2015 at 04:49:08PM -0600, Dave Gerlach wrote:
> >> +	/*
> >> +	 * Write a dummy message to the mailbox in order to trigger the RX
> >> +	 * interrupt to alert the M3 that data is available in the IPC
> >> +	 * registers. We must enable the IRQ here and disable it after in
> >> +	 * the RX callback to avoid multiple interrupts being received
> >> +	 * by the CM3.
> >> +	 */
> >> +	omap_mbox_enable_irq(m3_ipc_state.mbox, IRQ_RX);
> >> +	ret = mbox_send_message(m3_ipc_state.mbox, (void *)dummy_msg);
> > 
> > unnecessary cast.
> 
> I get a compiler warning without this one, may need it.

right, try with:

	ret = mbox_send_mesage(m3->mbox, &dummy_msg);

Another option is to just get rid of mbox_msg_t altogether since that's
just a u32 anyway. Then fix omap-mailbox.c::omap_mbox_chan_send_data()
so it knows it's receiving a void *, then it could:

	u32 data = *(u32 *)mssg;

but as Tony said, better not to add more dependencies to this series.

> >> +	m3_rproc = rproc_get_by_phandle(rproc_phandle);
> >> +	if (!m3_rproc) {
> >> +		dev_err(&pdev->dev, "could not get rproc handle\n");
> >> +		ret = -EPROBE_DEFER;
> >> +		goto err;
> >> +	}
> >> +
> >> +	m3_ipc_state.rproc = m3_rproc;
> >> +	m3_ipc_state.dev = dev;
> >> +	m3_ipc_state.state = M3_STATE_RESET;
> >> +
> >> +	/*
> >> +	 * Wait for firmware loading completion in a thread so we
> >> +	 * can boot the wkup_m3 as soon as it's ready without holding
> >> +	 * up kernel boot
> >> +	 */
> >> +	task = kthread_run((void *)wkup_m3_rproc_boot_thread, m3_rproc,
> >> +			   "wkup_m3_rproc_loader");
> > 
> > I wonder two things:
> > 
> > 1) This thread will only be used during boot up. Do we really need a
> > thread or would request_firmware_nowait() be enough ?
> > 
> > 2) what's the size of the firmware ? Is it really so large that we must
> > run this as a separate thread ? Meaning, why isn't request_firmware()
> > enough ? How much time would we be waiting ?
> 
> The issue here comes from the case where you attempt to load a firmware stored
> in the rootfs which is the typical use case for this. Remoteproc core requests
> the firmware twice, first with _nowait to load the resource table and then again

sounds like a bug to me. Or am I missing something ?

> without nowait to boot the rproc. rproc_boot requires the resource table to be
> loaded. The thread is really to wait for the firmware loaded completion, which
> gets set after the resource table has been loaded, to let boot proceed. System
> boot will get stuck otherwise as this driver can probe before rootfs is available.

IMO, that should be fixed in remoteproc, but it probably goes towards
"let's not add more dependencies".

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 3/3] ARM: dts: am33xx: Add wkup_m3_ipc node

[Dave Gerlach]

Add wkup_m3_ipc node for wkup_m3_ipc driver.

Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
---
 arch/arm/boot/dts/am33xx.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index acd3705..1ebb230 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -863,6 +863,15 @@
 			reg = <0x48310000 0x2000>;
 			interrupts = <111>;
 		};
+
+		wkup_m3_ipc: wkup_m3_ipc@44e11324 {
+			compatible = "ti,am3353-wkup-m3-ipc";
+			reg = <0x44e11324 0x0024>;
+			reg-names = "ipc_regs";
+			interrupts = <78>;
+			ti,rproc = <&wkup_m3>;
+			mboxes = <&mailbox &mbox_wkupm3>;
+		};
 	};
 };
 
-- 
2.1.0