Date:	Fri, 2 Jan 2015 14:16:43 -0600
From:	Felipe Balbi <balbi@...com>
To:	Dave Gerlach <d-gerlach@...com>
CC:	<linux-arm-kernel@...ts.infradead.org>,
	<linux-kernel@...r.kernel.org>, <linux-omap@...r.kernel.org>,
	<devicetree@...r.kernel.org>,
	Benoit Cousson <bcousson@...libre.com>,
	Ohad Ben-Cohen <ohad@...ery.com>, Suman Anna <s-anna@...com>,
	Arnd Bergmann <arnd@...db.de>,
	Kevin Hilman <khilman@...aro.org>,
	Tony Lindgren <tony@...mide.com>
Subject: Re: [PATCH 2/3] soc: ti: Add wkup_m3_ipc driver

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@...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@...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

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists