[PATCH 0/3] remoteproc: Introduce wkup_m3_rproc driver

[PATCH 0/3] remoteproc: Introduce wkup_m3_rproc driver

[Dave Gerlach]

Hi,
This patch series adds a wkup_m3_rproc driver for TI AM335x SoCs.
This family of SoCs contains an ARM Cortex M3 coprocessor that is
responsible for low-level power tasks that cannot be handled by
the main ARM Cortex A8 so firmware running from the CM3 can be
used instead. This driver handles loading of the firmware and
reset of the CM3 once it is booted.

This patch was split off from v4 of the am335x suspend series,
found here [1]. I have pushed a branch based on v3.19-rc1
containing all dependencies here [2] for am33xx suspend
for a higher level view of the entire series of patch sets. This
series is required for a coming series "drivers: soc: ti:
Introduce wkup_m3_ipc driver" that boots this rproc driver and
handles the communication layer between the SoC and this remote
processor.

This patch set depends on series "couple of generic remoteproc
enhancements" by Suman Anna found here [3]. The driver expects to
load firmware am335x-pm-firmware.elf from /lib/firmware which is
found here [4].

Regards,
Dave

[1] http://www.spinics.net/lists/linux-omap/msg109331.html
[2] https://github.com/dgerlach/linux-pm/tree/pm-am335x-v3.19-rc1
[3] http://www.spinics.net/lists/arm-kernel/msg362961.html
[4] https://git.ti.com/ti-cm3-pm-firmware/amx3-cm3/commits/next


Dave Gerlach (3):
  ARM: OMAP2+: Use pdata-quirks for wkup_m3 deassert_hardreset
  Documentation: dt: add ti,am3353-wkup-m3 bindings
  remoteproc: wkup_m3: Add wkup_m3 remote proc driver

 .../bindings/remoteproc/wkup_m3_rproc.txt          |  32 ++++
 arch/arm/mach-omap2/pdata-quirks.c                 |  13 ++
 drivers/remoteproc/Kconfig                         |  12 ++
 drivers/remoteproc/Makefile                        |   1 +
 drivers/remoteproc/wkup_m3_rproc.c                 | 175 +++++++++++++++++++++
 include/linux/platform_data/wkup_m3.h              |  23 +++
 6 files changed, 256 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/wkup_m3_rproc.txt
 create mode 100644 drivers/remoteproc/wkup_m3_rproc.c
 create mode 100644 include/linux/platform_data/wkup_m3.h

-- 
2.1.0

[PATCH 1/3] ARM: OMAP2+: Use pdata-quirks for wkup_m3 deassert_hardreset

[Dave Gerlach]

Use pdata-quirks to provide platform data required for reset
of the wkup_m3 during boot.

Signed-off-by: Dave Gerlach <d-gerlach-l0cyMroinI0@public.gmane.org>
---
 arch/arm/mach-omap2/pdata-quirks.c    | 13 +++++++++++++
 include/linux/platform_data/wkup_m3.h | 23 +++++++++++++++++++++++
 2 files changed, 36 insertions(+)
 create mode 100644 include/linux/platform_data/wkup_m3.h

diff --git a/arch/arm/mach-omap2/pdata-quirks.c b/arch/arm/mach-omap2/pdata-quirks.c
index 3d7eee1..b0c5916 100644
--- a/arch/arm/mach-omap2/pdata-quirks.c
+++ b/arch/arm/mach-omap2/pdata-quirks.c
@@ -18,6 +18,7 @@
 
 #include <linux/platform_data/pinctrl-single.h>
 #include <linux/platform_data/iommu-omap.h>
+#include <linux/platform_data/wkup_m3.h>
 
 #include "am35xx.h"
 #include "common.h"
@@ -288,6 +289,14 @@ static void __init omap3_tao3530_legacy_init(void)
 }
 #endif /* CONFIG_ARCH_OMAP3 */
 
+#ifdef CONFIG_SOC_AM33XX
+static struct wkup_m3_platform_data wkup_m3_data = {
+	.reset_name = "wkup_m3",
+	.assert_reset = omap_device_assert_hardreset,
+	.deassert_reset = omap_device_deassert_hardreset,
+};
+#endif
+
 #ifdef CONFIG_ARCH_OMAP4
 static void __init omap4_sdp_legacy_init(void)
 {
@@ -383,6 +392,10 @@ struct of_dev_auxdata omap_auxdata_lookup[] __initdata = {
 	OF_DEV_AUXDATA("ti,am3517-emac", 0x5c000000, "davinci_emac.0",
 		       &am35xx_emac_pdata),
 #endif
+#ifdef CONFIG_SOC_AM33XX
+	OF_DEV_AUXDATA("ti,am3353-wkup-m3", 0x44d00000, "44d00000.wkup_m3",
+		       &wkup_m3_data),
+#endif
 #ifdef CONFIG_ARCH_OMAP4
 	OF_DEV_AUXDATA("ti,omap4-padconf", 0x4a100040, "4a100040.pinmux", &pcs_pdata),
 	OF_DEV_AUXDATA("ti,omap4-padconf", 0x4a31e040, "4a31e040.pinmux", &pcs_pdata),
diff --git a/include/linux/platform_data/wkup_m3.h b/include/linux/platform_data/wkup_m3.h
new file mode 100644
index 0000000..6ee33d7
--- /dev/null
+++ b/include/linux/platform_data/wkup_m3.h
@@ -0,0 +1,23 @@
+/*
+ * omap wkup_m3: platform data
+ *
+ * Copyright (C) 2014 Texas Instruments, Inc.
+ *
+ * Dave Gerlach <d-gerlach-l0cyMroinI0@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _LINUX_PLATFORM_DATA_WKUP_M3_H
+#define _LINUX_PLATFORM_DATA_WKUP_M3_H
+
+struct wkup_m3_platform_data {
+	const char *reset_name;
+
+	int (*assert_reset)(struct platform_device *pdev, const char *name);
+	int (*deassert_reset)(struct platform_device *pdev, const char *name);
+};
+
+#endif /* _LINUX_PLATFORM_DATA_WKUP_M3_H */
-- 
2.1.0

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

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

[Dave Gerlach]

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

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

diff --git a/Documentation/devicetree/bindings/remoteproc/wkup_m3_rproc.txt b/Documentation/devicetree/bindings/remoteproc/wkup_m3_rproc.txt
new file mode 100644
index 0000000..4d64a23
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/wkup_m3_rproc.txt
@@ -0,0 +1,32 @@
+Wakeup M3 Remote Proc 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. The CM3 requires
+a firmware binary to accomplish this. The wkup_m3 remoteproc driver handles
+the loading of the firmware and booting of the CM3.
+
+Wkup M3 Device Node:
+====================
+A wkup_m3 device node is used to represent a wakeup M3 IP instance within
+a SoC.
+
+Required properties:
+--------------------
+- compatible:		Should be "ti,am3353-wkup-m3" for AM33xx SoCs
+- reg:			Contains the wkup_m3 register address ranges for
+			umem and dmem.
+- ti,hwmods:		Name of the hwmod associated with the mailbox
+- ti,no-reset-on-init:	Reset is handled after fw has been loaded, not at
+			init of hwmod.
+
+Example:
+--------
+/* AM33xx */
+wkup_m3: wkup_m3@44d00000 {
+	compatible = "ti,am3353-wkup-m3";
+	reg = <0x44d00000 0x4000	/* M3 UMEM */
+	       0x44d80000 0x2000>;	/* M3 DMEM */
+	ti,hwmods = "wkup_m3";
+	ti,no-reset-on-init;
+};
-- 
2.1.0

[PATCH 3/3] remoteproc: wkup_m3: Add wkup_m3 remote proc driver

[Dave Gerlach]

Add a remoteproc driver to load the firmware for and boot the wkup_m3
present on am33xx. The wkup_m3 is an integrated Cortex M3 that allows
the SoC to enter the lowest possible power state by taking control from
the MPU after it has gone into its own low power state and shutting off
any additional peripherals.

The driver expects a resource table to be present in the wkup_m3
firmware to define the required memory resources needed by the wkup_m3,
at least the data memory so that the firmware can be copied to the proper
place for execution.

Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
---
 drivers/remoteproc/Kconfig         |  12 +++
 drivers/remoteproc/Makefile        |   1 +
 drivers/remoteproc/wkup_m3_rproc.c | 175 +++++++++++++++++++++++++++++++++++++
 3 files changed, 188 insertions(+)
 create mode 100644 drivers/remoteproc/wkup_m3_rproc.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 5e343ba..7fbdb53 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -41,6 +41,18 @@ config STE_MODEM_RPROC
 	  This can be either built-in or a loadable module.
 	  If unsure say N.
 
+config WKUP_M3_RPROC
+	bool "AM33xx wkup-m3 remoteproc support"
+	depends on SOC_AM33XX
+	select REMOTEPROC
+	help
+	  Say y here to support AM33xx wkup-m3.
+
+	  Required for Suspend-to-ram and CPUIdle on AM33xx. Allows for
+	  loading of firmware of CM3 PM coprocessor that is present
+	  on AM33xx family of SoCs
+	  If unsure say N.
+
 config DA8XX_REMOTEPROC
 	tristate "DA8xx/OMAP-L13x remoteproc support"
 	depends on ARCH_DAVINCI_DA8XX
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index ac2ff75..81b04d1 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -9,4 +9,5 @@ remoteproc-y				+= remoteproc_virtio.o
 remoteproc-y				+= remoteproc_elf_loader.o
 obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
 obj-$(CONFIG_STE_MODEM_RPROC)	 	+= ste_modem_rproc.o
+obj-$(CONFIG_WKUP_M3_RPROC)		+= wkup_m3_rproc.o
 obj-$(CONFIG_DA8XX_REMOTEPROC)		+= da8xx_remoteproc.o
diff --git a/drivers/remoteproc/wkup_m3_rproc.c b/drivers/remoteproc/wkup_m3_rproc.c
new file mode 100644
index 0000000..8686ca2
--- /dev/null
+++ b/drivers/remoteproc/wkup_m3_rproc.c
@@ -0,0 +1,175 @@
+/*
+ * AMx3 Wkup M3 Remote Processor 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/interrupt.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/remoteproc.h>
+
+#include <linux/platform_data/wkup_m3.h>
+
+#include "remoteproc_internal.h"
+
+struct wkup_m3_rproc {
+	struct rproc *rproc;
+	struct platform_device *pdev;
+};
+
+static int wkup_m3_rproc_start(struct rproc *rproc)
+{
+	struct wkup_m3_rproc *m3_rproc = rproc->priv;
+	struct platform_device *pdev = m3_rproc->pdev;
+	struct device *dev = &pdev->dev;
+	struct wkup_m3_platform_data *pdata = dev->platform_data;
+	int ret;
+
+	ret = pdata->deassert_reset(pdev, pdata->reset_name);
+	if (ret) {
+		dev_err(dev, "Unable to reset wkup_m3!\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int wkup_m3_rproc_stop(struct rproc *rproc)
+{
+	struct wkup_m3_rproc *m3_rproc = rproc->priv;
+	struct platform_device *pdev = m3_rproc->pdev;
+	struct device *dev = &pdev->dev;
+	struct wkup_m3_platform_data *pdata = dev->platform_data;
+	int ret;
+
+	ret = pdata->assert_reset(pdev, pdata->reset_name);
+	if (ret) {
+		dev_err(dev, "Unable to assert reset of wkup_m3!\n");
+		return -ENODEV;
+	}
+	return 0;
+}
+
+static struct rproc_ops wkup_m3_rproc_ops = {
+	.start		= wkup_m3_rproc_start,
+	.stop		= wkup_m3_rproc_stop,
+};
+
+static const struct of_device_id wkup_m3_rproc_of_match[] = {
+	{
+		.compatible = "ti,am3353-wkup-m3",
+		.data = (void *)"am335x-pm-firmware.elf",
+	},
+	{},
+};
+
+static int wkup_m3_rproc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const char *fw_name;
+	struct wkup_m3_platform_data *pdata = dev->platform_data;
+	struct wkup_m3_rproc *m3_rproc;
+	const struct of_device_id *match;
+	struct rproc *rproc;
+	int ret;
+
+	if (!(pdata && pdata->deassert_reset && pdata->assert_reset &&
+	      pdata->reset_name)) {
+		dev_err(dev, "Platform data missing!\n");
+		return -ENODEV;
+	}
+
+	match = of_match_device(wkup_m3_rproc_of_match, &pdev->dev);
+	if (!match)
+		return -ENODEV;
+	fw_name = (char *)match->data;
+
+	pm_runtime_enable(&pdev->dev);
+
+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (IS_ERR_VALUE(ret)) {
+		dev_err(&pdev->dev, "pm_runtime_get_sync() failed\n");
+		return ret;
+	}
+
+	rproc = rproc_alloc(dev, "wkup_m3", &wkup_m3_rproc_ops,
+			    fw_name, sizeof(*m3_rproc));
+	if (!rproc)
+		return -ENOMEM;
+
+	m3_rproc = rproc->priv;
+	m3_rproc->rproc = rproc;
+	m3_rproc->pdev = pdev;
+
+	dev_set_drvdata(dev, rproc);
+
+	/* Register as a remoteproc device */
+	ret = rproc_add(rproc);
+	if (ret) {
+		dev_err(dev, "rproc_add failed\n");
+		goto err;
+	}
+
+	return 0;
+
+err:
+	rproc_put(rproc);
+	pm_runtime_put_sync(&pdev->dev);
+	return ret;
+}
+
+static int wkup_m3_rproc_remove(struct platform_device *pdev)
+{
+	struct rproc *rproc = platform_get_drvdata(pdev);
+
+	rproc_del(rproc);
+	rproc_put(rproc);
+	pm_runtime_put_sync(&pdev->dev);
+
+	return 0;
+}
+
+static int wkup_m3_rpm_suspend(struct device *dev)
+{
+	return -EBUSY;
+}
+
+static int wkup_m3_rpm_resume(struct device *dev)
+{
+	return 0;
+}
+
+static const struct dev_pm_ops wkup_m3_rproc_pm_ops = {
+	SET_RUNTIME_PM_OPS(wkup_m3_rpm_suspend, wkup_m3_rpm_resume, NULL)
+};
+
+static struct platform_driver wkup_m3_rproc_driver = {
+	.probe = wkup_m3_rproc_probe,
+	.remove = wkup_m3_rproc_remove,
+	.driver = {
+		.name = "wkup_m3",
+		.of_match_table = wkup_m3_rproc_of_match,
+		.pm = &wkup_m3_rproc_pm_ops,
+	},
+};
+
+module_platform_driver(wkup_m3_rproc_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("wkup m3 remote processor control driver");
-- 
2.1.0

Re: [PATCH 3/3] remoteproc: wkup_m3: Add wkup_m3 remote proc driver

[Felipe Balbi]

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

On Fri, Jan 02, 2015 at 01:51:59PM -0600, Dave Gerlach wrote:
> Add a remoteproc driver to load the firmware for and boot the wkup_m3
> present on am33xx. The wkup_m3 is an integrated Cortex M3 that allows
> the SoC to enter the lowest possible power state by taking control from
> the MPU after it has gone into its own low power state and shutting off
> any additional peripherals.
> 
> The driver expects a resource table to be present in the wkup_m3
> firmware to define the required memory resources needed by the wkup_m3,
> at least the data memory so that the firmware can be copied to the proper
> place for execution.
> 
> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
> ---
>  drivers/remoteproc/Kconfig         |  12 +++
>  drivers/remoteproc/Makefile        |   1 +
>  drivers/remoteproc/wkup_m3_rproc.c | 175 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 188 insertions(+)
>  create mode 100644 drivers/remoteproc/wkup_m3_rproc.c
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 5e343ba..7fbdb53 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -41,6 +41,18 @@ config STE_MODEM_RPROC
>  	  This can be either built-in or a loadable module.
>  	  If unsure say N.
>  
> +config WKUP_M3_RPROC
> +	bool "AM33xx wkup-m3 remoteproc support"

it would be nicer if this could be a loadable module.

> +	depends on SOC_AM33XX
> +	select REMOTEPROC
> +	help
> +	  Say y here to support AM33xx wkup-m3.
> +
> +	  Required for Suspend-to-ram and CPUIdle on AM33xx. Allows for
> +	  loading of firmware of CM3 PM coprocessor that is present
> +	  on AM33xx family of SoCs
> +	  If unsure say N.
> +
>  config DA8XX_REMOTEPROC
>  	tristate "DA8xx/OMAP-L13x remoteproc support"
>  	depends on ARCH_DAVINCI_DA8XX
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index ac2ff75..81b04d1 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -9,4 +9,5 @@ remoteproc-y				+= remoteproc_virtio.o
>  remoteproc-y				+= remoteproc_elf_loader.o
>  obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
>  obj-$(CONFIG_STE_MODEM_RPROC)	 	+= ste_modem_rproc.o
> +obj-$(CONFIG_WKUP_M3_RPROC)		+= wkup_m3_rproc.o
>  obj-$(CONFIG_DA8XX_REMOTEPROC)		+= da8xx_remoteproc.o
> diff --git a/drivers/remoteproc/wkup_m3_rproc.c b/drivers/remoteproc/wkup_m3_rproc.c
> new file mode 100644
> index 0000000..8686ca2
> --- /dev/null
> +++ b/drivers/remoteproc/wkup_m3_rproc.c
> @@ -0,0 +1,175 @@
> +/*
> + * AMx3 Wkup M3 Remote Processor 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/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/remoteproc.h>
> +
> +#include <linux/platform_data/wkup_m3.h>
> +
> +#include "remoteproc_internal.h"
> +
> +struct wkup_m3_rproc {
> +	struct rproc *rproc;
> +	struct platform_device *pdev;
> +};
> +
> +static int wkup_m3_rproc_start(struct rproc *rproc)
> +{
> +	struct wkup_m3_rproc *m3_rproc = rproc->priv;
> +	struct platform_device *pdev = m3_rproc->pdev;
> +	struct device *dev = &pdev->dev;
> +	struct wkup_m3_platform_data *pdata = dev->platform_data;
> +	int ret;
> +
> +	ret = pdata->deassert_reset(pdev, pdata->reset_name);

looks like here you should assert, wait, deassert. What if soemthing
else used wkup_m3 before this loads ?

> +	if (ret) {
> +		dev_err(dev, "Unable to reset wkup_m3!\n");
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static int wkup_m3_rproc_stop(struct rproc *rproc)
> +{
> +	struct wkup_m3_rproc *m3_rproc = rproc->priv;
> +	struct platform_device *pdev = m3_rproc->pdev;
> +	struct device *dev = &pdev->dev;
> +	struct wkup_m3_platform_data *pdata = dev->platform_data;
> +	int ret;
> +
> +	ret = pdata->assert_reset(pdev, pdata->reset_name);
> +	if (ret) {
> +		dev_err(dev, "Unable to assert reset of wkup_m3!\n");
> +		return -ENODEV;
> +	}
> +	return 0;
> +}
> +
> +static struct rproc_ops wkup_m3_rproc_ops = {
> +	.start		= wkup_m3_rproc_start,
> +	.stop		= wkup_m3_rproc_stop,
> +};
> +
> +static const struct of_device_id wkup_m3_rproc_of_match[] = {
> +	{
> +		.compatible = "ti,am3353-wkup-m3",
> +		.data = (void *)"am335x-pm-firmware.elf",

do you know of anybody else who might want to different firmware image
name ? Otherwise why pass it as driver_data ?

> +	},
> +	{},
> +};
> +
> +static int wkup_m3_rproc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const char *fw_name;
> +	struct wkup_m3_platform_data *pdata = dev->platform_data;
> +	struct wkup_m3_rproc *m3_rproc;
> +	const struct of_device_id *match;
> +	struct rproc *rproc;
> +	int ret;
> +
> +	if (!(pdata && pdata->deassert_reset && pdata->assert_reset &&
> +	      pdata->reset_name)) {
> +		dev_err(dev, "Platform data missing!\n");
> +		return -ENODEV;
> +	}

if pdata is missing, couldn't you assume the thing has been reset and
try to load anyway ?

> +	match = of_match_device(wkup_m3_rproc_of_match, &pdev->dev);
> +	if (!match)
> +		return -ENODEV;
> +	fw_name = (char *)match->data;
> +
> +	pm_runtime_enable(&pdev->dev);
> +
> +	ret = pm_runtime_get_sync(&pdev->dev);
> +	if (IS_ERR_VALUE(ret)) {
> +		dev_err(&pdev->dev, "pm_runtime_get_sync() failed\n");
> +		return ret;

this is wrong for two reasons:

a) you need to pm_runtime_disable();
b) even if pm_runtime_get*() fails, you _must_ call
	pm_runtime_put_sync();

> +	}
> +
> +	rproc = rproc_alloc(dev, "wkup_m3", &wkup_m3_rproc_ops,
> +			    fw_name, sizeof(*m3_rproc));
> +	if (!rproc)
> +		return -ENOMEM;
> +
> +	m3_rproc = rproc->priv;
> +	m3_rproc->rproc = rproc;
> +	m3_rproc->pdev = pdev;
> +
> +	dev_set_drvdata(dev, rproc);
> +
> +	/* Register as a remoteproc device */
> +	ret = rproc_add(rproc);
> +	if (ret) {
> +		dev_err(dev, "rproc_add failed\n");
> +		goto err;
> +	}
> +
> +	return 0;
> +
> +err:
> +	rproc_put(rproc);
> +	pm_runtime_put_sync(&pdev->dev);

missing pm_runtime_disable();

> +	return ret;
> +}
> +
> +static int wkup_m3_rproc_remove(struct platform_device *pdev)
> +{
> +	struct rproc *rproc = platform_get_drvdata(pdev);
> +
> +	rproc_del(rproc);
> +	rproc_put(rproc);
> +	pm_runtime_put_sync(&pdev->dev);

missing pm_runtime_disable();

> +
> +	return 0;
> +}
> +
> +static int wkup_m3_rpm_suspend(struct device *dev)
> +{
> +	return -EBUSY;
> +}

looks like this is just coping with omap_device bogosity, no ?

> +
> +static int wkup_m3_rpm_resume(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops wkup_m3_rproc_pm_ops = {
> +	SET_RUNTIME_PM_OPS(wkup_m3_rpm_suspend, wkup_m3_rpm_resume, NULL)
> +};
> +
> +static struct platform_driver wkup_m3_rproc_driver = {
> +	.probe = wkup_m3_rproc_probe,
> +	.remove = wkup_m3_rproc_remove,
> +	.driver = {
> +		.name = "wkup_m3",
> +		.of_match_table = wkup_m3_rproc_of_match,
> +		.pm = &wkup_m3_rproc_pm_ops,
> +	},
> +};
> +
> +module_platform_driver(wkup_m3_rproc_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("wkup m3 remote processor control driver");

do you want to add MODULE_AUTHOR() ?

-- 
balbi

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

Re: [PATCH 3/3] remoteproc: wkup_m3: Add wkup_m3 remote proc driver

[Dave Gerlach]

Felipe,
On 01/02/2015 02:04 PM, Felipe Balbi wrote:
> On Fri, Jan 02, 2015 at 01:51:59PM -0600, Dave Gerlach wrote:
>> Add a remoteproc driver to load the firmware for and boot the wkup_m3
>> present on am33xx. The wkup_m3 is an integrated Cortex M3 that allows
>> the SoC to enter the lowest possible power state by taking control from
>> the MPU after it has gone into its own low power state and shutting off
>> any additional peripherals.
>>
>> The driver expects a resource table to be present in the wkup_m3
>> firmware to define the required memory resources needed by the wkup_m3,
>> at least the data memory so that the firmware can be copied to the proper
>> place for execution.
>>
>> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
>> ---
>>  drivers/remoteproc/Kconfig         |  12 +++
>>  drivers/remoteproc/Makefile        |   1 +
>>  drivers/remoteproc/wkup_m3_rproc.c | 175 +++++++++++++++++++++++++++++++++++++
>>  3 files changed, 188 insertions(+)
>>  create mode 100644 drivers/remoteproc/wkup_m3_rproc.c
>>
>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>> index 5e343ba..7fbdb53 100644
>> --- a/drivers/remoteproc/Kconfig
>> +++ b/drivers/remoteproc/Kconfig
>> @@ -41,6 +41,18 @@ config STE_MODEM_RPROC
>>  	  This can be either built-in or a loadable module.
>>  	  If unsure say N.
>>  
>> +config WKUP_M3_RPROC
>> +	bool "AM33xx wkup-m3 remoteproc support"
> 
> it would be nicer if this could be a loadable module.

Do we really want that though? This is required for core PM functionality like
CPUIdle and Suspend/resume, I feel that it should always be built in for am335x.
I had been taking this approach with all of the PM dependencies.

> 
>> +	depends on SOC_AM33XX
>> +	select REMOTEPROC
>> +	help
>> +	  Say y here to support AM33xx wkup-m3.
>> +
>> +	  Required for Suspend-to-ram and CPUIdle on AM33xx. Allows for
>> +	  loading of firmware of CM3 PM coprocessor that is present
>> +	  on AM33xx family of SoCs
>> +	  If unsure say N.
>> +
>>  config DA8XX_REMOTEPROC
>>  	tristate "DA8xx/OMAP-L13x remoteproc support"
>>  	depends on ARCH_DAVINCI_DA8XX
>> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
>> index ac2ff75..81b04d1 100644
>> --- a/drivers/remoteproc/Makefile
>> +++ b/drivers/remoteproc/Makefile
>> @@ -9,4 +9,5 @@ remoteproc-y				+= remoteproc_virtio.o
>>  remoteproc-y				+= remoteproc_elf_loader.o
>>  obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
>>  obj-$(CONFIG_STE_MODEM_RPROC)	 	+= ste_modem_rproc.o
>> +obj-$(CONFIG_WKUP_M3_RPROC)		+= wkup_m3_rproc.o
>>  obj-$(CONFIG_DA8XX_REMOTEPROC)		+= da8xx_remoteproc.o
>> diff --git a/drivers/remoteproc/wkup_m3_rproc.c b/drivers/remoteproc/wkup_m3_rproc.c
>> new file mode 100644
>> index 0000000..8686ca2
>> --- /dev/null
>> +++ b/drivers/remoteproc/wkup_m3_rproc.c
>> @@ -0,0 +1,175 @@
>> +/*
>> + * AMx3 Wkup M3 Remote Processor 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/interrupt.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/remoteproc.h>
>> +
>> +#include <linux/platform_data/wkup_m3.h>
>> +
>> +#include "remoteproc_internal.h"
>> +
>> +struct wkup_m3_rproc {
>> +	struct rproc *rproc;
>> +	struct platform_device *pdev;
>> +};
>> +
>> +static int wkup_m3_rproc_start(struct rproc *rproc)
>> +{
>> +	struct wkup_m3_rproc *m3_rproc = rproc->priv;
>> +	struct platform_device *pdev = m3_rproc->pdev;
>> +	struct device *dev = &pdev->dev;
>> +	struct wkup_m3_platform_data *pdata = dev->platform_data;
>> +	int ret;
>> +
>> +	ret = pdata->deassert_reset(pdev, pdata->reset_name);
> 
> looks like here you should assert, wait, deassert. What if soemthing
> else used wkup_m3 before this loads ?
> 

Hmm, that's unlikely but not impossible, and if the wkup_m3 is not properly
reset after firmware loading it won't boot, which kills all PM on am335x. I'll
look into doing that.

>> +	if (ret) {
>> +		dev_err(dev, "Unable to reset wkup_m3!\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int wkup_m3_rproc_stop(struct rproc *rproc)
>> +{
>> +	struct wkup_m3_rproc *m3_rproc = rproc->priv;
>> +	struct platform_device *pdev = m3_rproc->pdev;
>> +	struct device *dev = &pdev->dev;
>> +	struct wkup_m3_platform_data *pdata = dev->platform_data;
>> +	int ret;
>> +
>> +	ret = pdata->assert_reset(pdev, pdata->reset_name);
>> +	if (ret) {
>> +		dev_err(dev, "Unable to assert reset of wkup_m3!\n");
>> +		return -ENODEV;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static struct rproc_ops wkup_m3_rproc_ops = {
>> +	.start		= wkup_m3_rproc_start,
>> +	.stop		= wkup_m3_rproc_stop,
>> +};
>> +
>> +static const struct of_device_id wkup_m3_rproc_of_match[] = {
>> +	{
>> +		.compatible = "ti,am3353-wkup-m3",
>> +		.data = (void *)"am335x-pm-firmware.elf",
> 
> do you know of anybody else who might want to different firmware image
> name ? Otherwise why pass it as driver_data ?

I suppose we could pass the name in the devicetree. I do not know of any other
users of other firmware but it's probably better to keep things flexible.

> 
>> +	},
>> +	{},
>> +};
>> +
>> +static int wkup_m3_rproc_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	const char *fw_name;
>> +	struct wkup_m3_platform_data *pdata = dev->platform_data;
>> +	struct wkup_m3_rproc *m3_rproc;
>> +	const struct of_device_id *match;
>> +	struct rproc *rproc;
>> +	int ret;
>> +
>> +	if (!(pdata && pdata->deassert_reset && pdata->assert_reset &&
>> +	      pdata->reset_name)) {
>> +		dev_err(dev, "Platform data missing!\n");
>> +		return -ENODEV;
>> +	}
> 
> if pdata is missing, couldn't you assume the thing has been reset and
> try to load anyway ?

Probably not, we MUST reset after loading the firmware as that is what boots the
wkup_m3.

> 
>> +	match = of_match_device(wkup_m3_rproc_of_match, &pdev->dev);
>> +	if (!match)
>> +		return -ENODEV;
>> +	fw_name = (char *)match->data;
>> +
>> +	pm_runtime_enable(&pdev->dev);
>> +
>> +	ret = pm_runtime_get_sync(&pdev->dev);
>> +	if (IS_ERR_VALUE(ret)) {
>> +		dev_err(&pdev->dev, "pm_runtime_get_sync() failed\n");
>> +		return ret;
> 
> this is wrong for two reasons:
> 
> a) you need to pm_runtime_disable();
> b) even if pm_runtime_get*() fails, you _must_ call
> 	pm_runtime_put_sync();

Ok I will fix this and the following pm_runtime issues. Didn't realize you still
had to call put_sync after a failed get_sync.

> 
>> +	}
>> +
>> +	rproc = rproc_alloc(dev, "wkup_m3", &wkup_m3_rproc_ops,
>> +			    fw_name, sizeof(*m3_rproc));
>> +	if (!rproc)
>> +		return -ENOMEM;
>> +
>> +	m3_rproc = rproc->priv;
>> +	m3_rproc->rproc = rproc;
>> +	m3_rproc->pdev = pdev;
>> +
>> +	dev_set_drvdata(dev, rproc);
>> +
>> +	/* Register as a remoteproc device */
>> +	ret = rproc_add(rproc);
>> +	if (ret) {
>> +		dev_err(dev, "rproc_add failed\n");
>> +		goto err;
>> +	}
>> +
>> +	return 0;
>> +
>> +err:
>> +	rproc_put(rproc);
>> +	pm_runtime_put_sync(&pdev->dev);
> 
> missing pm_runtime_disable();
> 
>> +	return ret;
>> +}
>> +
>> +static int wkup_m3_rproc_remove(struct platform_device *pdev)
>> +{
>> +	struct rproc *rproc = platform_get_drvdata(pdev);
>> +
>> +	rproc_del(rproc);
>> +	rproc_put(rproc);
>> +	pm_runtime_put_sync(&pdev->dev);
> 
> missing pm_runtime_disable();
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int wkup_m3_rpm_suspend(struct device *dev)
>> +{
>> +	return -EBUSY;
>> +}
> 
> looks like this is just coping with omap_device bogosity, no ?
>

Yes, without this omap_device shuts down ther wkup_m3 during suspend, which of
course prevents the wkup_m3 from finishing suspend process or waking SoC back
up. Haven't found a better solution for the problem than this.

>> +
>> +static int wkup_m3_rpm_resume(struct device *dev)
>> +{
>> +	return 0;
>> +}
>> +
>> +static const struct dev_pm_ops wkup_m3_rproc_pm_ops = {
>> +	SET_RUNTIME_PM_OPS(wkup_m3_rpm_suspend, wkup_m3_rpm_resume, NULL)
>> +};
>> +
>> +static struct platform_driver wkup_m3_rproc_driver = {
>> +	.probe = wkup_m3_rproc_probe,
>> +	.remove = wkup_m3_rproc_remove,
>> +	.driver = {
>> +		.name = "wkup_m3",
>> +		.of_match_table = wkup_m3_rproc_of_match,
>> +		.pm = &wkup_m3_rproc_pm_ops,
>> +	},
>> +};
>> +
>> +module_platform_driver(wkup_m3_rproc_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("wkup m3 remote processor control driver");
> 
> do you want to add MODULE_AUTHOR() ?
> 

Yes. Thanks for the comments.

Regards,
Dave

Re: [PATCH 3/3] remoteproc: wkup_m3: Add wkup_m3 remote proc driver

[Felipe Balbi]

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

Hi,

On Mon, Jan 05, 2015 at 02:10:14PM -0600, Dave Gerlach wrote:
> >> Add a remoteproc driver to load the firmware for and boot the wkup_m3
> >> present on am33xx. The wkup_m3 is an integrated Cortex M3 that allows
> >> the SoC to enter the lowest possible power state by taking control from
> >> the MPU after it has gone into its own low power state and shutting off
> >> any additional peripherals.
> >>
> >> The driver expects a resource table to be present in the wkup_m3
> >> firmware to define the required memory resources needed by the wkup_m3,
> >> at least the data memory so that the firmware can be copied to the proper
> >> place for execution.
> >>
> >> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
> >> ---
> >>  drivers/remoteproc/Kconfig         |  12 +++
> >>  drivers/remoteproc/Makefile        |   1 +
> >>  drivers/remoteproc/wkup_m3_rproc.c | 175 +++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 188 insertions(+)
> >>  create mode 100644 drivers/remoteproc/wkup_m3_rproc.c
> >>
> >> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> >> index 5e343ba..7fbdb53 100644
> >> --- a/drivers/remoteproc/Kconfig
> >> +++ b/drivers/remoteproc/Kconfig
> >> @@ -41,6 +41,18 @@ config STE_MODEM_RPROC
> >>  	  This can be either built-in or a loadable module.
> >>  	  If unsure say N.
> >>  
> >> +config WKUP_M3_RPROC
> >> +	bool "AM33xx wkup-m3 remoteproc support"
> > 
> > it would be nicer if this could be a loadable module.
> 
> Do we really want that though? This is required for core PM functionality like
> CPUIdle and Suspend/resume, I feel that it should always be built in for am335x.
> I had been taking this approach with all of the PM dependencies.

right, will we have CPUIdle or suspend/resume during boot ?

> >> +static const struct of_device_id wkup_m3_rproc_of_match[] = {
> >> +	{
> >> +		.compatible = "ti,am3353-wkup-m3",
> >> +		.data = (void *)"am335x-pm-firmware.elf",
> > 
> > do you know of anybody else who might want to different firmware image
> > name ? Otherwise why pass it as driver_data ?
> 
> I suppose we could pass the name in the devicetree. I do not know of any other
> users of other firmware but it's probably better to keep things flexible.

or, since this is handled internally to the driver, we can refactor this
name passing later, when we actually need a different firmware depending
on different devices. No ?

> >> +static int wkup_m3_rproc_probe(struct platform_device *pdev)
> >> +{
> >> +	struct device *dev = &pdev->dev;
> >> +	const char *fw_name;
> >> +	struct wkup_m3_platform_data *pdata = dev->platform_data;
> >> +	struct wkup_m3_rproc *m3_rproc;
> >> +	const struct of_device_id *match;
> >> +	struct rproc *rproc;
> >> +	int ret;
> >> +
> >> +	if (!(pdata && pdata->deassert_reset && pdata->assert_reset &&
> >> +	      pdata->reset_name)) {
> >> +		dev_err(dev, "Platform data missing!\n");
> >> +		return -ENODEV;
> >> +	}
> > 
> > if pdata is missing, couldn't you assume the thing has been reset and
> > try to load anyway ?
> 
> Probably not, we MUST reset after loading the firmware as that is what boots the
> wkup_m3.

alright, perhaps add a comment ?

> >> +	match = of_match_device(wkup_m3_rproc_of_match, &pdev->dev);
> >> +	if (!match)
> >> +		return -ENODEV;
> >> +	fw_name = (char *)match->data;
> >> +
> >> +	pm_runtime_enable(&pdev->dev);
> >> +
> >> +	ret = pm_runtime_get_sync(&pdev->dev);
> >> +	if (IS_ERR_VALUE(ret)) {
> >> +		dev_err(&pdev->dev, "pm_runtime_get_sync() failed\n");
> >> +		return ret;
> > 
> > this is wrong for two reasons:
> > 
> > a) you need to pm_runtime_disable();
> > b) even if pm_runtime_get*() fails, you _must_ call
> > 	pm_runtime_put_sync();
> 
> Ok I will fix this and the following pm_runtime issues. Didn't realize you still
> had to call put_sync after a failed get_sync.

alright.

> >> +static int wkup_m3_rpm_suspend(struct device *dev)
> >> +{
> >> +	return -EBUSY;
> >> +}
> > 
> > looks like this is just coping with omap_device bogosity, no ?
> >
> 
> Yes, without this omap_device shuts down ther wkup_m3 during suspend, which of
> course prevents the wkup_m3 from finishing suspend process or waking SoC back
> up. Haven't found a better solution for the problem than this.

Tony, any better for this ? Do we keep this small hack or find a better
way ?

> >> +static int wkup_m3_rpm_resume(struct device *dev)
> >> +{
> >> +	return 0;
> >> +}
> >> +
> >> +static const struct dev_pm_ops wkup_m3_rproc_pm_ops = {
> >> +	SET_RUNTIME_PM_OPS(wkup_m3_rpm_suspend, wkup_m3_rpm_resume, NULL)
> >> +};
> >> +
> >> +static struct platform_driver wkup_m3_rproc_driver = {
> >> +	.probe = wkup_m3_rproc_probe,
> >> +	.remove = wkup_m3_rproc_remove,
> >> +	.driver = {
> >> +		.name = "wkup_m3",
> >> +		.of_match_table = wkup_m3_rproc_of_match,
> >> +		.pm = &wkup_m3_rproc_pm_ops,
> >> +	},
> >> +};
> >> +
> >> +module_platform_driver(wkup_m3_rproc_driver);
> >> +
> >> +MODULE_LICENSE("GPL v2");
> >> +MODULE_DESCRIPTION("wkup m3 remote processor control driver");
> > 
> > do you want to add MODULE_AUTHOR() ?
> > 
> 
> Yes. Thanks for the comments.

np.

-- 
balbi

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

Re: [PATCH 3/3] remoteproc: wkup_m3: Add wkup_m3 remote proc driver

[Tony Lindgren]

* Felipe Balbi <balbi@ti.com> [150105 12:23]:
> On Mon, Jan 05, 2015 at 02:10:14PM -0600, Dave Gerlach wrote:
> > >> +static int wkup_m3_rpm_suspend(struct device *dev)
> > >> +{
> > >> +	return -EBUSY;
> > >> +}
> > > 
> > > looks like this is just coping with omap_device bogosity, no ?
> > >
> > 
> > Yes, without this omap_device shuts down ther wkup_m3 during suspend, which of
> > course prevents the wkup_m3 from finishing suspend process or waking SoC back
> > up. Haven't found a better solution for the problem than this.
> 
> Tony, any better for this ? Do we keep this small hack or find a better
> way ?

Looks OK to me for now, later on we may want to have a flag for
HWMOD_NEVER_IDLE or something similar for wkup_m3_hwmod. But let's
not add more dependencies to this series.

Regards,

Tony