[PATCH v4 1/5] dt-bindings: Document JZ47xx VPU auxiliary processor

[PATCH v4 1/5] dt-bindings: Document JZ47xx VPU auxiliary processor

[Paul Cercueil]

Inside the Video Processing Unit (VPU) of the recent JZ47xx SoCs from
Ingenic is a second Xburst MIPS CPU very similar to the main core.
This document describes the devicetree bindings for this auxiliary
processor.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v2: Update TCSM0 address in example
    v3: Change node name to 'video-decoder'
    v4: Convert to YAML. I didn't add Rob's Ack on v3 because of that (sorry Rob)

 .../bindings/remoteproc/ingenic,vpu.yaml      | 76 +++++++++++++++++++
 1 file changed, 76 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml

diff --git a/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
new file mode 100644
index 000000000000..9f876d16a5a6
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/remoteproc/ingenic,vpu.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Ingenic Video Processing Unit bindings
+
+description:
+  Inside the Video Processing Unit (VPU) of the recent JZ47xx SoCs from
+  Ingenic is a second Xburst MIPS CPU very similar to the main core.
+  This document describes the devicetree bindings for this auxiliary
+  processor.
+
+maintainers:
+  - Paul Cercueil <paul@crapouillou.net>
+
+properties:
+  compatible:
+    const: ingenic,jz4770-vpu-rproc
+
+  reg:
+    items:
+      - description: aux registers
+      - description: tcsm0 registers
+      - description: tcsm1 registers
+      - description: sram registers
+
+  reg-names:
+    items:
+      - const: aux
+      - const: tcsm0
+      - const: tcsm1
+      - const: sram
+
+  clocks:
+    items:
+      - description: aux clock
+      - description: vpu clock
+
+  clock-names:
+    items:
+      - const: aux
+      - const: vpu
+
+  interrupts:
+    description: VPU hardware interrupt
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - clocks
+  - clock-names
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    vpu: video-decoder@132a0000 {
+      compatible = "ingenic,jz4770-vpu-rproc";
+
+      reg = <0x132a0000 0x20 /* AUX */
+           0x132b0000 0x4000 /* TCSM0 */
+           0x132c0000 0xc000 /* TCSM1 */
+           0x132f0000 0x7000 /* SRAM */
+      >;
+      reg-names = "aux", "tcsm0", "tcsm1", "sram";
+
+      clocks = <&cgu JZ4770_CLK_AUX>, <&cgu JZ4770_CLK_VPU>;
+      clock-names = "aux", "vpu";
+
+      interrupt-parent = <&cpuintc>;
+      interrupts = <3>;
+    };
-- 
2.24.0

[PATCH v4 2/5] remoteproc: Add device-managed variants of rproc_alloc/rproc_add

[Paul Cercueil]

Add API functions devm_rproc_alloc() and devm_rproc_add(), which behave
like rproc_alloc() and rproc_add() respectively, but register their
respective cleanup function to be called on driver detach.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v3: New patch
    v4: No change

 drivers/remoteproc/remoteproc_core.c | 67 ++++++++++++++++++++++++++++
 include/linux/remoteproc.h           |  5 +++
 2 files changed, 72 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 307df98347ba..0a9fc7fdd1c3 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1932,6 +1932,33 @@ int rproc_add(struct rproc *rproc)
 }
 EXPORT_SYMBOL(rproc_add);
 
+static void devm_rproc_remove(void *rproc)
+{
+	rproc_del(rproc);
+}
+
+/**
+ * devm_rproc_add() - resource managed rproc_add()
+ * @dev: the underlying device
+ * @rproc: the remote processor handle to register
+ *
+ * This function performs like rproc_add() but the registered rproc device will
+ * automatically be removed on driver detach.
+ *
+ * Returns 0 on success and an appropriate error code otherwise.
+ */
+int devm_rproc_add(struct device *dev, struct rproc *rproc)
+{
+	int err;
+
+	err = rproc_add(rproc);
+	if (err)
+		return err;
+
+	return devm_add_action_or_reset(dev, devm_rproc_remove, rproc);
+}
+EXPORT_SYMBOL(devm_rproc_add);
+
 /**
  * rproc_type_release() - release a remote processor instance
  * @dev: the rproc's device
@@ -2149,6 +2176,46 @@ int rproc_del(struct rproc *rproc)
 }
 EXPORT_SYMBOL(rproc_del);
 
+static void devm_rproc_free(struct device *dev, void *res)
+{
+	rproc_free(*(struct rproc **)res);
+}
+
+/**
+ * devm_rproc_alloc() - resource managed rproc_alloc()
+ * @dev: the underlying device
+ * @name: name of this remote processor
+ * @ops: platform-specific handlers (mainly start/stop)
+ * @firmware: name of firmware file to load, can be NULL
+ * @len: length of private data needed by the rproc driver (in bytes)
+ *
+ * This function performs like rproc_alloc() but the acuired rproc device will
+ * automatically be released on driver detach.
+ *
+ * On success the new rproc is returned, and on failure, NULL.
+ */
+struct rproc *devm_rproc_alloc(struct device *dev, const char *name,
+			       const struct rproc_ops *ops,
+			       const char *firmware, int len)
+{
+	struct rproc **ptr, *rproc;
+
+	ptr = devres_alloc(devm_rproc_free, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	rproc = rproc_alloc(dev, name, ops, firmware, len);
+	if (rproc) {
+		*ptr = rproc;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return rproc;
+}
+EXPORT_SYMBOL(devm_rproc_alloc);
+
 /**
  * rproc_add_subdev() - add a subdevice to a remoteproc
  * @rproc: rproc handle to add the subdevice to
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 16ad66683ad0..5f201f0c86c3 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -595,6 +595,11 @@ int rproc_add(struct rproc *rproc);
 int rproc_del(struct rproc *rproc);
 void rproc_free(struct rproc *rproc);
 
+struct rproc *devm_rproc_alloc(struct device *dev, const char *name,
+			       const struct rproc_ops *ops,
+			       const char *firmware, int len);
+int devm_rproc_add(struct device *dev, struct rproc *rproc);
+
 void rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem);
 
 struct rproc_mem_entry *
-- 
2.24.0

[PATCH v4 3/5] remoteproc: Add prepare/unprepare callbacks

[Paul Cercueil]

The .prepare() callback is called before the firmware is loaded to
memory. This is useful for instance in the case where some setup is
required for the memory to be accessible.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v2-v4: No change

 drivers/remoteproc/remoteproc_core.c | 16 +++++++++++++++-
 include/linux/remoteproc.h           |  4 ++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 0a9fc7fdd1c3..3ea5f675a148 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1299,11 +1299,19 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
 	struct device *dev = &rproc->dev;
 	int ret;
 
+	if (rproc->ops->prepare) {
+		ret = rproc->ops->prepare(rproc);
+		if (ret) {
+			dev_err(dev, "Failed to prepare rproc: %d\n", ret);
+			return ret;
+		}
+	}
+
 	/* load the ELF segments to memory */
 	ret = rproc_load_segments(rproc, fw);
 	if (ret) {
 		dev_err(dev, "Failed to load program segments: %d\n", ret);
-		return ret;
+		goto unprepare_rproc;
 	}
 
 	/*
@@ -1354,6 +1362,9 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
 	rproc_unprepare_subdevices(rproc);
 reset_table_ptr:
 	rproc->table_ptr = rproc->cached_table;
+unprepare_rproc:
+	if (rproc->ops->unprepare)
+		rproc->ops->unprepare(rproc);
 
 	return ret;
 }
@@ -1483,6 +1494,9 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
 
 	rproc->state = RPROC_OFFLINE;
 
+	if (rproc->ops->unprepare)
+		rproc->ops->unprepare(rproc);
+
 	dev_info(dev, "stopped remote processor %s\n", rproc->name);
 
 	return 0;
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 5f201f0c86c3..a6272d1ba384 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -355,6 +355,8 @@ enum rsc_handling_status {
 
 /**
  * struct rproc_ops - platform-specific device handlers
+ * @prepare:	prepare the device for power up (before the firmware is loaded)
+ * @unprepare:	unprepare the device after it is stopped
  * @start:	power on the device and boot it
  * @stop:	power off the device
  * @kick:	kick a virtqueue (virtqueue id given as a parameter)
@@ -371,6 +373,8 @@ enum rsc_handling_status {
  * @get_boot_addr:	get boot address to entry point specified in firmware
  */
 struct rproc_ops {
+	int (*prepare)(struct rproc *rproc);
+	void (*unprepare)(struct rproc *rproc);
 	int (*start)(struct rproc *rproc);
 	int (*stop)(struct rproc *rproc);
 	void (*kick)(struct rproc *rproc, int vqid);
-- 
2.24.0

[PATCH v4 4/5] remoteproc: ingenic: Added remoteproc driver

[Paul Cercueil]

This driver is used to boot, communicate with and load firmwares to the
MIPS co-processor found in the VPU hardware of the JZ47xx SoCs from
Ingenic.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Signed-off-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>
Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---

Notes:
    v2: Remove exception for always-mapped memories
    v3: - Use clk_bulk API
    	- Move device-managed code to its own patch [3/4]
    	- Move devicetree table right above ingenic_rproc_driver
    	- Removed #ifdef CONFIG_OF around devicetree table
    	- Removed .owner = THIS_MODULE in ingenic_rproc_driver
    	- Removed useless platform_set_drvdata()
    v4: - Add fix reported by Julia
    	- Change Kconfig symbol to INGENIC_VPU_RPROC
    	- Add documentation to struct vpu
    	- disable_irq_nosync() -> disable_irq()

 drivers/remoteproc/Kconfig         |   8 +
 drivers/remoteproc/Makefile        |   1 +
 drivers/remoteproc/ingenic_rproc.c | 247 +++++++++++++++++++++++++++++
 3 files changed, 256 insertions(+)
 create mode 100644 drivers/remoteproc/ingenic_rproc.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 94afdde4bc9f..75284979e1d2 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -214,6 +214,14 @@ config STM32_RPROC
 
 	  This can be either built-in or a loadable module.
 
+config INGENIC_VPU_RPROC
+	tristate "Ingenic JZ47xx VPU remoteproc support"
+	depends on MIPS || COMPILE_TEST
+	help
+	  Say y or m here to support the VPU in the JZ47xx SoCs from Ingenic.
+	  This can be either built-in or a loadable module.
+	  If unsure say N.
+
 endif # REMOTEPROC
 
 endmenu
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 00f09e658cb3..e360edc342f1 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -10,6 +10,7 @@ remoteproc-y				+= remoteproc_sysfs.o
 remoteproc-y				+= remoteproc_virtio.o
 remoteproc-y				+= remoteproc_elf_loader.o
 obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
+obj-$(CONFIG_INGENIC_VPU_RPROC)		+= ingenic_rproc.o
 obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
 obj-$(CONFIG_WKUP_M3_RPROC)		+= wkup_m3_rproc.o
 obj-$(CONFIG_DA8XX_REMOTEPROC)		+= da8xx_remoteproc.o
diff --git a/drivers/remoteproc/ingenic_rproc.c b/drivers/remoteproc/ingenic_rproc.c
new file mode 100644
index 000000000000..0dd2779dfd38
--- /dev/null
+++ b/drivers/remoteproc/ingenic_rproc.c
@@ -0,0 +1,247 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Ingenic JZ47xx remoteproc driver
+ * Copyright 2019, Paul Cercueil <paul@crapouillou.net>
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/remoteproc.h>
+
+#include "remoteproc_internal.h"
+
+#define REG_AUX_CTRL		0x0
+#define REG_AUX_MSG_ACK		0x10
+#define REG_AUX_MSG		0x14
+#define REG_CORE_MSG_ACK	0x18
+#define REG_CORE_MSG		0x1C
+
+#define AUX_CTRL_SLEEP		BIT(31)
+#define AUX_CTRL_MSG_IRQ_EN	BIT(3)
+#define AUX_CTRL_NMI_RESETS	BIT(2)
+#define AUX_CTRL_NMI		BIT(1)
+#define AUX_CTRL_SW_RESET	BIT(0)
+
+struct vpu_mem_map {
+	const char *name;
+	unsigned int da;
+};
+
+struct vpu_mem_info {
+	const struct vpu_mem_map *map;
+	unsigned long len;
+	void __iomem *base;
+};
+
+static const struct vpu_mem_map vpu_mem_map[] = {
+	{ "tcsm0", 0x132b0000 },
+	{ "tcsm1", 0xf4000000 },
+	{ "sram",  0x132f0000 },
+};
+
+/**
+ * struct vpu - Ingenic VPU remoteproc private structure
+ * @irq: interrupt number
+ * @clks: pointers to the VPU and AUX clocks
+ * @mem_info: array of struct vpu_mem_info, which contain the mapping info of
+ *            each of the external memories
+ * @dev: private pointer to the device
+ */
+struct vpu {
+	int irq;
+	struct clk_bulk_data clks[2];
+	void __iomem *aux_base;
+	struct vpu_mem_info mem_info[ARRAY_SIZE(vpu_mem_map)];
+	struct device *dev;
+};
+
+static int ingenic_rproc_prepare(struct rproc *rproc)
+{
+	struct vpu *vpu = rproc->priv;
+	int ret;
+
+	/* The clocks must be enabled for the firmware to be loaded in TCSM */
+	ret = clk_bulk_prepare_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
+	if (ret)
+		dev_err(vpu->dev, "Unable to start clocks: %d", ret);
+
+	return ret;
+}
+
+static void ingenic_rproc_unprepare(struct rproc *rproc)
+{
+	struct vpu *vpu = rproc->priv;
+
+	clk_bulk_disable_unprepare(ARRAY_SIZE(vpu->clks), vpu->clks);
+}
+
+static int ingenic_rproc_start(struct rproc *rproc)
+{
+	struct vpu *vpu = rproc->priv;
+	u32 ctrl;
+
+	enable_irq(vpu->irq);
+
+	/* Reset the AUX and enable message IRQ */
+	ctrl = AUX_CTRL_NMI_RESETS | AUX_CTRL_NMI | AUX_CTRL_MSG_IRQ_EN;
+	writel(ctrl, vpu->aux_base + REG_AUX_CTRL);
+
+	return 0;
+}
+
+static int ingenic_rproc_stop(struct rproc *rproc)
+{
+	struct vpu *vpu = rproc->priv;
+
+	disable_irq(vpu->irq);
+
+	/* Keep AUX in reset mode */
+	writel(AUX_CTRL_SW_RESET, vpu->aux_base + REG_AUX_CTRL);
+
+	return 0;
+}
+
+static void ingenic_rproc_kick(struct rproc *rproc, int vqid)
+{
+	struct vpu *vpu = rproc->priv;
+
+	writel(vqid, vpu->aux_base + REG_CORE_MSG);
+}
+
+static void *ingenic_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
+{
+	struct vpu *vpu = rproc->priv;
+	void __iomem *va = NULL;
+	unsigned int i;
+
+	if (len <= 0)
+		return NULL;
+
+	for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
+		const struct vpu_mem_info *info = &vpu->mem_info[i];
+		const struct vpu_mem_map *map = info->map;
+
+		if (da >= map->da && (da + len) < (map->da + info->len)) {
+			va = info->base + (da - map->da);
+			break;
+		}
+	}
+
+	return (__force void *)va;
+}
+
+static struct rproc_ops ingenic_rproc_ops = {
+	.prepare = ingenic_rproc_prepare,
+	.unprepare = ingenic_rproc_unprepare,
+	.start = ingenic_rproc_start,
+	.stop = ingenic_rproc_stop,
+	.kick = ingenic_rproc_kick,
+	.da_to_va = ingenic_rproc_da_to_va,
+};
+
+static irqreturn_t vpu_interrupt(int irq, void *data)
+{
+	struct rproc *rproc = data;
+	struct vpu *vpu = rproc->priv;
+	u32 vring;
+
+	vring = readl(vpu->aux_base + REG_AUX_MSG);
+
+	/* Ack the interrupt */
+	writel(0, vpu->aux_base + REG_AUX_MSG_ACK);
+
+	return rproc_vq_interrupt(rproc, vring);
+}
+
+static int ingenic_rproc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *mem;
+	struct rproc *rproc;
+	struct vpu *vpu;
+	unsigned int i;
+	int ret;
+
+	rproc = devm_rproc_alloc(dev, "ingenic-vpu",
+				 &ingenic_rproc_ops, NULL, sizeof(*vpu));
+	if (!rproc)
+		return -ENOMEM;
+
+	vpu = rproc->priv;
+	vpu->dev = &pdev->dev;
+
+	mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "aux");
+	vpu->aux_base = devm_ioremap_resource(dev, mem);
+	if (IS_ERR(vpu->aux_base)) {
+		dev_err(dev, "Failed to ioremap");
+		return PTR_ERR(vpu->aux_base);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
+		mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+						   vpu_mem_map[i].name);
+
+		vpu->mem_info[i].base = devm_ioremap_resource(dev, mem);
+		if (IS_ERR(vpu->mem_info[i].base)) {
+			ret = PTR_ERR(vpu->mem_info[i].base);
+			dev_err(dev, "Failed to ioremap");
+			return ret;
+		}
+
+		vpu->mem_info[i].len = resource_size(mem);
+		vpu->mem_info[i].map = &vpu_mem_map[i];
+	}
+
+	vpu->clks[0].id = "vpu";
+	vpu->clks[1].id = "aux";
+
+	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(vpu->clks), vpu->clks);
+	if (ret) {
+		dev_err(dev, "Failed to get clocks");
+		return ret;
+	}
+
+	vpu->irq = platform_get_irq(pdev, 0);
+	if (vpu->irq < 0)
+		return vpu->irq;
+
+	ret = devm_request_irq(dev, vpu->irq, vpu_interrupt, 0, "VPU", rproc);
+	if (ret < 0) {
+		dev_err(dev, "Failed to request IRQ");
+		return ret;
+	}
+
+	disable_irq(vpu->irq);
+
+	ret = devm_rproc_add(dev, rproc);
+	if (ret) {
+		dev_err(dev, "Failed to register remote processor");
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id ingenic_rproc_of_matches[] = {
+	{ .compatible = "ingenic,jz4770-vpu-rproc", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, ingenic_rproc_of_matches);
+
+static struct platform_driver ingenic_rproc_driver = {
+	.probe = ingenic_rproc_probe,
+	.driver = {
+		.name = "ingenic-vpu",
+		.of_match_table = of_match_ptr(ingenic_rproc_of_matches),
+	},
+};
+module_platform_driver(ingenic_rproc_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
+MODULE_DESCRIPTION("Ingenic JZ47xx Remote Processor control driver");
-- 
2.24.0

[PATCH v4 5/5] MAINTAINERS: Add myself as reviewer for Ingenic rproc driver

[Paul Cercueil]

Add myself as the reviewer for the Ingenic VPU remoteproc driver.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v4: New patch

 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index bd5847e802de..d82814ef2bee 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8197,6 +8197,7 @@ F:	drivers/mtd/nand/raw/ingenic/
 F:	drivers/pinctrl/pinctrl-ingenic.c
 F:	drivers/power/supply/ingenic-battery.c
 F:	drivers/pwm/pwm-jz4740.c
+F:	drivers/remoteproc/ingenic_rproc.c
 F:	drivers/rtc/rtc-jz4740.c
 F:	drivers/tty/serial/8250/8250_ingenic.c
 F:	drivers/usb/musb/jz4740.c
-- 
2.24.0

Re: [PATCH v4 2/5] remoteproc: Add device-managed variants of rproc_alloc/rproc_add

[Fabien DESSENNE]

Hi Paul,


Good initiative! See me remarks below.


On 10/12/2019 5:40 PM, Paul Cercueil wrote:
> Add API functions devm_rproc_alloc() and devm_rproc_add(), which behave
> like rproc_alloc() and rproc_add() respectively, but register their
> respective cleanup function to be called on driver detach.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>
> Notes:
>      v3: New patch
>      v4: No change
>
>   drivers/remoteproc/remoteproc_core.c | 67 ++++++++++++++++++++++++++++
>   include/linux/remoteproc.h           |  5 +++
>   2 files changed, 72 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 307df98347ba..0a9fc7fdd1c3 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c


Maybe these devm function shall be defined in a new remoteproc/devres.c 
file. Although it seems to be a common usage I don't know if there is a 
rule for that.


> @@ -1932,6 +1932,33 @@ int rproc_add(struct rproc *rproc)
>   }
>   EXPORT_SYMBOL(rproc_add);
>   
> +static void devm_rproc_remove(void *rproc)
> +{
> +	rproc_del(rproc);
> +}
> +
> +/**
> + * devm_rproc_add() - resource managed rproc_add()
> + * @dev: the underlying device
> + * @rproc: the remote processor handle to register
> + *
> + * This function performs like rproc_add() but the registered rproc device will
> + * automatically be removed on driver detach.
> + *
> + * Returns 0 on success and an appropriate error code otherwise.
> + */
> +int devm_rproc_add(struct device *dev, struct rproc *rproc)
> +{
> +	int err;
> +
> +	err = rproc_add(rproc);
> +	if (err)
> +		return err;
> +
> +	return devm_add_action_or_reset(dev, devm_rproc_remove, rproc);
> +}
> +EXPORT_SYMBOL(devm_rproc_add);
> +
>   /**
>    * rproc_type_release() - release a remote processor instance
>    * @dev: the rproc's device
> @@ -2149,6 +2176,46 @@ int rproc_del(struct rproc *rproc)
>   }
>   EXPORT_SYMBOL(rproc_del);
>   
> +static void devm_rproc_free(struct device *dev, void *res)
> +{
> +	rproc_free(*(struct rproc **)res);
> +}
> +
> +/**
> + * devm_rproc_alloc() - resource managed rproc_alloc()
> + * @dev: the underlying device
> + * @name: name of this remote processor
> + * @ops: platform-specific handlers (mainly start/stop)
> + * @firmware: name of firmware file to load, can be NULL
> + * @len: length of private data needed by the rproc driver (in bytes)
> + *
> + * This function performs like rproc_alloc() but the acuired rproc device will


typo: s/acuired/acquired


> + * automatically be released on driver detach.
> + *
> + * On success the new rproc is returned, and on failure, NULL.
> + */
> +struct rproc *devm_rproc_alloc(struct device *dev, const char *name,
> +			       const struct rproc_ops *ops,
> +			       const char *firmware, int len)
> +{
> +	struct rproc **ptr, *rproc;
> +
> +	ptr = devres_alloc(devm_rproc_free, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	rproc = rproc_alloc(dev, name, ops, firmware, len);
> +	if (rproc) {
> +		*ptr = rproc;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}
> +
> +	return rproc;


Can't you use devm_add_action_or_reset() here too?


> +}
> +EXPORT_SYMBOL(devm_rproc_alloc);
> +
>   /**
>    * rproc_add_subdev() - add a subdevice to a remoteproc
>    * @rproc: rproc handle to add the subdevice to
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 16ad66683ad0..5f201f0c86c3 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -595,6 +595,11 @@ int rproc_add(struct rproc *rproc);
>   int rproc_del(struct rproc *rproc);
>   void rproc_free(struct rproc *rproc);
>   
> +struct rproc *devm_rproc_alloc(struct device *dev, const char *name,
> +			       const struct rproc_ops *ops,
> +			       const char *firmware, int len);
> +int devm_rproc_add(struct device *dev, struct rproc *rproc);
> +
>   void rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem);
>   
>   struct rproc_mem_entry *


BR

Fabien

Re: [PATCH v4 3/5] remoteproc: Add prepare/unprepare callbacks

[Fabien DESSENNE]

Hi Paul


On 10/12/2019 5:40 PM, Paul Cercueil wrote:
> The .prepare() callback is called before the firmware is loaded to
> memory. This is useful for instance in the case where some setup is
> required for the memory to be accessible.


I am trying to figure out what king of 'setup' may be required. From the 
ingenic driver I understand that you need to enable clocks to allow some 
memory access.

Instead of adding this new ops, why not enabling clocks in probe()?

BR

Fabien


>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>
> Notes:
>      v2-v4: No change
>
>   drivers/remoteproc/remoteproc_core.c | 16 +++++++++++++++-
>   include/linux/remoteproc.h           |  4 ++++
>   2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 0a9fc7fdd1c3..3ea5f675a148 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1299,11 +1299,19 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
>   	struct device *dev = &rproc->dev;
>   	int ret;
>   
> +	if (rproc->ops->prepare) {
> +		ret = rproc->ops->prepare(rproc);
> +		if (ret) {
> +			dev_err(dev, "Failed to prepare rproc: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
>   	/* load the ELF segments to memory */
>   	ret = rproc_load_segments(rproc, fw);
>   	if (ret) {
>   		dev_err(dev, "Failed to load program segments: %d\n", ret);
> -		return ret;
> +		goto unprepare_rproc;
>   	}
>   
>   	/*
> @@ -1354,6 +1362,9 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
>   	rproc_unprepare_subdevices(rproc);
>   reset_table_ptr:
>   	rproc->table_ptr = rproc->cached_table;
> +unprepare_rproc:
> +	if (rproc->ops->unprepare)
> +		rproc->ops->unprepare(rproc);
>   
>   	return ret;
>   }
> @@ -1483,6 +1494,9 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
>   
>   	rproc->state = RPROC_OFFLINE;
>   
> +	if (rproc->ops->unprepare)
> +		rproc->ops->unprepare(rproc);
> +
>   	dev_info(dev, "stopped remote processor %s\n", rproc->name);
>   
>   	return 0;
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 5f201f0c86c3..a6272d1ba384 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -355,6 +355,8 @@ enum rsc_handling_status {
>   
>   /**
>    * struct rproc_ops - platform-specific device handlers
> + * @prepare:	prepare the device for power up (before the firmware is loaded)
> + * @unprepare:	unprepare the device after it is stopped
>    * @start:	power on the device and boot it
>    * @stop:	power off the device
>    * @kick:	kick a virtqueue (virtqueue id given as a parameter)
> @@ -371,6 +373,8 @@ enum rsc_handling_status {
>    * @get_boot_addr:	get boot address to entry point specified in firmware
>    */
>   struct rproc_ops {
> +	int (*prepare)(struct rproc *rproc);
> +	void (*unprepare)(struct rproc *rproc);
>   	int (*start)(struct rproc *rproc);
>   	int (*stop)(struct rproc *rproc);
>   	void (*kick)(struct rproc *rproc, int vqid);

Re: [PATCH v4 1/5] dt-bindings: Document JZ47xx VPU auxiliary processor

[Rob Herring]

On Tue, Dec 10, 2019 at 05:40:10PM +0100, Paul Cercueil wrote:
> Inside the Video Processing Unit (VPU) of the recent JZ47xx SoCs from
> Ingenic is a second Xburst MIPS CPU very similar to the main core.
> This document describes the devicetree bindings for this auxiliary
> processor.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> 
> Notes:
>     v2: Update TCSM0 address in example
>     v3: Change node name to 'video-decoder'
>     v4: Convert to YAML. I didn't add Rob's Ack on v3 because of that (sorry Rob)
> 
>  .../bindings/remoteproc/ingenic,vpu.yaml      | 76 +++++++++++++++++++
>  1 file changed, 76 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
> new file mode 100644
> index 000000000000..9f876d16a5a6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/remoteproc/ingenic,vpu.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Ingenic Video Processing Unit bindings
> +
> +description:
> +  Inside the Video Processing Unit (VPU) of the recent JZ47xx SoCs from
> +  Ingenic is a second Xburst MIPS CPU very similar to the main core.
> +  This document describes the devicetree bindings for this auxiliary
> +  processor.
> +
> +maintainers:
> +  - Paul Cercueil <paul@crapouillou.net>
> +
> +properties:
> +  compatible:
> +    const: ingenic,jz4770-vpu-rproc
> +
> +  reg:
> +    items:
> +      - description: aux registers
> +      - description: tcsm0 registers
> +      - description: tcsm1 registers
> +      - description: sram registers
> +
> +  reg-names:
> +    items:
> +      - const: aux
> +      - const: tcsm0
> +      - const: tcsm1
> +      - const: sram
> +
> +  clocks:
> +    items:
> +      - description: aux clock
> +      - description: vpu clock
> +
> +  clock-names:
> +    items:
> +      - const: aux
> +      - const: vpu
> +
> +  interrupts:
> +    description: VPU hardware interrupt
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - clocks
> +  - clock-names
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    vpu: video-decoder@132a0000 {
> +      compatible = "ingenic,jz4770-vpu-rproc";
> +
> +      reg = <0x132a0000 0x20 /* AUX */
> +           0x132b0000 0x4000 /* TCSM0 */
> +           0x132c0000 0xc000 /* TCSM1 */
> +           0x132f0000 0x7000 /* SRAM */
> +      >;
> +      reg-names = "aux", "tcsm0", "tcsm1", "sram";
> +
> +      clocks = <&cgu JZ4770_CLK_AUX>, <&cgu JZ4770_CLK_VPU>;

Examples are built now by 'make dt_binding_check' and this fails. You 
need to add the include for these defines.

Also, the schema should complain after that on 'reg'. You need to <> 
each entry (addr and size).

> +      clock-names = "aux", "vpu";
> +
> +      interrupt-parent = <&cpuintc>;
> +      interrupts = <3>;
> +    };
> -- 
> 2.24.0
> 

Re: [PATCH v4 1/5] dt-bindings: Document JZ47xx VPU auxiliary processor

[Paul Cercueil]

Hi Rob,


Le ven., déc. 13, 2019 at 13:02, Rob Herring <robh@kernel.org> a 
écrit :
> On Tue, Dec 10, 2019 at 05:40:10PM +0100, Paul Cercueil wrote:
>>  Inside the Video Processing Unit (VPU) of the recent JZ47xx SoCs 
>> from
>>  Ingenic is a second Xburst MIPS CPU very similar to the main core.
>>  This document describes the devicetree bindings for this auxiliary
>>  processor.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>> 
>>  Notes:
>>      v2: Update TCSM0 address in example
>>      v3: Change node name to 'video-decoder'
>>      v4: Convert to YAML. I didn't add Rob's Ack on v3 because of 
>> that (sorry Rob)
>> 
>>   .../bindings/remoteproc/ingenic,vpu.yaml      | 76 
>> +++++++++++++++++++
>>   1 file changed, 76 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
>> 
>>  diff --git 
>> a/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml 
>> b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
>>  new file mode 100644
>>  index 000000000000..9f876d16a5a6
>>  --- /dev/null
>>  +++ b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
>>  @@ -0,0 +1,76 @@
>>  +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>  +%YAML 1.2
>>  +---
>>  +$id: "http://devicetree.org/schemas/remoteproc/ingenic,vpu.yaml#"
>>  +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>>  +
>>  +title: Ingenic Video Processing Unit bindings
>>  +
>>  +description:
>>  +  Inside the Video Processing Unit (VPU) of the recent JZ47xx SoCs 
>> from
>>  +  Ingenic is a second Xburst MIPS CPU very similar to the main 
>> core.
>>  +  This document describes the devicetree bindings for this 
>> auxiliary
>>  +  processor.
>>  +
>>  +maintainers:
>>  +  - Paul Cercueil <paul@crapouillou.net>
>>  +
>>  +properties:
>>  +  compatible:
>>  +    const: ingenic,jz4770-vpu-rproc
>>  +
>>  +  reg:
>>  +    items:
>>  +      - description: aux registers
>>  +      - description: tcsm0 registers
>>  +      - description: tcsm1 registers
>>  +      - description: sram registers
>>  +
>>  +  reg-names:
>>  +    items:
>>  +      - const: aux
>>  +      - const: tcsm0
>>  +      - const: tcsm1
>>  +      - const: sram
>>  +
>>  +  clocks:
>>  +    items:
>>  +      - description: aux clock
>>  +      - description: vpu clock
>>  +
>>  +  clock-names:
>>  +    items:
>>  +      - const: aux
>>  +      - const: vpu
>>  +
>>  +  interrupts:
>>  +    description: VPU hardware interrupt
>>  +
>>  +required:
>>  +  - compatible
>>  +  - reg
>>  +  - reg-names
>>  +  - clocks
>>  +  - clock-names
>>  +  - interrupts
>>  +
>>  +additionalProperties: false
>>  +
>>  +examples:
>>  +  - |
>>  +    vpu: video-decoder@132a0000 {
>>  +      compatible = "ingenic,jz4770-vpu-rproc";
>>  +
>>  +      reg = <0x132a0000 0x20 /* AUX */
>>  +           0x132b0000 0x4000 /* TCSM0 */
>>  +           0x132c0000 0xc000 /* TCSM1 */
>>  +           0x132f0000 0x7000 /* SRAM */
>>  +      >;
>>  +      reg-names = "aux", "tcsm0", "tcsm1", "sram";
>>  +
>>  +      clocks = <&cgu JZ4770_CLK_AUX>, <&cgu JZ4770_CLK_VPU>;
> 
> Examples are built now by 'make dt_binding_check' and this fails. You
> need to add the include for these defines.

Strange, "make dt_binding_check" does not complain here about this file 
with your latest dt-schema tool and Linux 5.5-rc1.

> 
> Also, the schema should complain after that on 'reg'. You need to <>
> each entry (addr and size).

Ok.

-Paul

> 
>>  +      clock-names = "aux", "vpu";
>>  +
>>  +      interrupt-parent = <&cpuintc>;
>>  +      interrupts = <3>;
>>  +    };
>>  --
>>  2.24.0
>> 

Re: [PATCH v4 2/5] remoteproc: Add device-managed variants of rproc_alloc/rproc_add

[Paul Cercueil]

Hi Fabien,


Le jeu., déc. 12, 2019 at 09:43, Fabien DESSENNE 
<fabien.dessenne@st.com> a écrit :
> Hi Paul,
> 
> 
> Good initiative! See me remarks below.
> 
> 
> On 10/12/2019 5:40 PM, Paul Cercueil wrote:
>>  Add API functions devm_rproc_alloc() and devm_rproc_add(), which 
>> behave
>>  like rproc_alloc() and rproc_add() respectively, but register their
>>  respective cleanup function to be called on driver detach.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>> 
>>  Notes:
>>       v3: New patch
>>       v4: No change
>> 
>>    drivers/remoteproc/remoteproc_core.c | 67 
>> ++++++++++++++++++++++++++++
>>    include/linux/remoteproc.h           |  5 +++
>>    2 files changed, 72 insertions(+)
>> 
>>  diff --git a/drivers/remoteproc/remoteproc_core.c 
>> b/drivers/remoteproc/remoteproc_core.c
>>  index 307df98347ba..0a9fc7fdd1c3 100644
>>  --- a/drivers/remoteproc/remoteproc_core.c
>>  +++ b/drivers/remoteproc/remoteproc_core.c
> 
> 
> Maybe these devm function shall be defined in a new 
> remoteproc/devres.c
> file. Although it seems to be a common usage I don't know if there is 
> a
> rule for that.

It's not a rule, more like a good practice. I'll do that.


> 
>>  @@ -1932,6 +1932,33 @@ int rproc_add(struct rproc *rproc)
>>    }
>>    EXPORT_SYMBOL(rproc_add);
>> 
>>  +static void devm_rproc_remove(void *rproc)
>>  +{
>>  +	rproc_del(rproc);
>>  +}
>>  +
>>  +/**
>>  + * devm_rproc_add() - resource managed rproc_add()
>>  + * @dev: the underlying device
>>  + * @rproc: the remote processor handle to register
>>  + *
>>  + * This function performs like rproc_add() but the registered 
>> rproc device will
>>  + * automatically be removed on driver detach.
>>  + *
>>  + * Returns 0 on success and an appropriate error code otherwise.
>>  + */
>>  +int devm_rproc_add(struct device *dev, struct rproc *rproc)
>>  +{
>>  +	int err;
>>  +
>>  +	err = rproc_add(rproc);
>>  +	if (err)
>>  +		return err;
>>  +
>>  +	return devm_add_action_or_reset(dev, devm_rproc_remove, rproc);
>>  +}
>>  +EXPORT_SYMBOL(devm_rproc_add);
>>  +
>>    /**
>>     * rproc_type_release() - release a remote processor instance
>>     * @dev: the rproc's device
>>  @@ -2149,6 +2176,46 @@ int rproc_del(struct rproc *rproc)
>>    }
>>    EXPORT_SYMBOL(rproc_del);
>> 
>>  +static void devm_rproc_free(struct device *dev, void *res)
>>  +{
>>  +	rproc_free(*(struct rproc **)res);
>>  +}
>>  +
>>  +/**
>>  + * devm_rproc_alloc() - resource managed rproc_alloc()
>>  + * @dev: the underlying device
>>  + * @name: name of this remote processor
>>  + * @ops: platform-specific handlers (mainly start/stop)
>>  + * @firmware: name of firmware file to load, can be NULL
>>  + * @len: length of private data needed by the rproc driver (in 
>> bytes)
>>  + *
>>  + * This function performs like rproc_alloc() but the acuired rproc 
>> device will
> 
> 
> typo: s/acuired/acquired
> 
> 
>>  + * automatically be released on driver detach.
>>  + *
>>  + * On success the new rproc is returned, and on failure, NULL.
>>  + */
>>  +struct rproc *devm_rproc_alloc(struct device *dev, const char 
>> *name,
>>  +			       const struct rproc_ops *ops,
>>  +			       const char *firmware, int len)
>>  +{
>>  +	struct rproc **ptr, *rproc;
>>  +
>>  +	ptr = devres_alloc(devm_rproc_free, sizeof(*ptr), GFP_KERNEL);
>>  +	if (!ptr)
>>  +		return ERR_PTR(-ENOMEM);
>>  +
>>  +	rproc = rproc_alloc(dev, name, ops, firmware, len);
>>  +	if (rproc) {
>>  +		*ptr = rproc;
>>  +		devres_add(dev, ptr);
>>  +	} else {
>>  +		devres_free(ptr);
>>  +	}
>>  +
>>  +	return rproc;
> 
> 
> Can't you use devm_add_action_or_reset() here too?

Yes, I guess that will make things simpler.

Thanks,
-Paul


> 
>>  +}
>>  +EXPORT_SYMBOL(devm_rproc_alloc);
>>  +
>>    /**
>>     * rproc_add_subdev() - add a subdevice to a remoteproc
>>     * @rproc: rproc handle to add the subdevice to
>>  diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>  index 16ad66683ad0..5f201f0c86c3 100644
>>  --- a/include/linux/remoteproc.h
>>  +++ b/include/linux/remoteproc.h
>>  @@ -595,6 +595,11 @@ int rproc_add(struct rproc *rproc);
>>    int rproc_del(struct rproc *rproc);
>>    void rproc_free(struct rproc *rproc);
>> 
>>  +struct rproc *devm_rproc_alloc(struct device *dev, const char 
>> *name,
>>  +			       const struct rproc_ops *ops,
>>  +			       const char *firmware, int len);
>>  +int devm_rproc_add(struct device *dev, struct rproc *rproc);
>>  +
>>    void rproc_add_carveout(struct rproc *rproc, struct 
>> rproc_mem_entry *mem);
>> 
>>    struct rproc_mem_entry *
> 
> 
> BR
> 
> Fabien

Re: [PATCH v4 3/5] remoteproc: Add prepare/unprepare callbacks

[Paul Cercueil]

Hi Fabien,


Le jeu., déc. 12, 2019 at 10:03, Fabien DESSENNE 
<fabien.dessenne@st.com> a écrit :
> Hi Paul
> 
> 
> On 10/12/2019 5:40 PM, Paul Cercueil wrote:
>>  The .prepare() callback is called before the firmware is loaded to
>>  memory. This is useful for instance in the case where some setup is
>>  required for the memory to be accessible.
> 
> 
> I am trying to figure out what king of 'setup' may be required. From 
> the
> ingenic driver I understand that you need to enable clocks to allow 
> some
> memory access.
> 
> Instead of adding this new ops, why not enabling clocks in probe()?

Enabling the clocks in the probe means that the clocks will be 
unconditionally enabled until the driver is removed, even if the remote 
processor end up being unused. That would be a waste of power.

Cheers,
-Paul


> 
> BR
> 
> Fabien
> 
> 
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>> 
>>  Notes:
>>       v2-v4: No change
>> 
>>    drivers/remoteproc/remoteproc_core.c | 16 +++++++++++++++-
>>    include/linux/remoteproc.h           |  4 ++++
>>    2 files changed, 19 insertions(+), 1 deletion(-)
>> 
>>  diff --git a/drivers/remoteproc/remoteproc_core.c 
>> b/drivers/remoteproc/remoteproc_core.c
>>  index 0a9fc7fdd1c3..3ea5f675a148 100644
>>  --- a/drivers/remoteproc/remoteproc_core.c
>>  +++ b/drivers/remoteproc/remoteproc_core.c
>>  @@ -1299,11 +1299,19 @@ static int rproc_start(struct rproc *rproc, 
>> const struct firmware *fw)
>>    	struct device *dev = &rproc->dev;
>>    	int ret;
>> 
>>  +	if (rproc->ops->prepare) {
>>  +		ret = rproc->ops->prepare(rproc);
>>  +		if (ret) {
>>  +			dev_err(dev, "Failed to prepare rproc: %d\n", ret);
>>  +			return ret;
>>  +		}
>>  +	}
>>  +
>>    	/* load the ELF segments to memory */
>>    	ret = rproc_load_segments(rproc, fw);
>>    	if (ret) {
>>    		dev_err(dev, "Failed to load program segments: %d\n", ret);
>>  -		return ret;
>>  +		goto unprepare_rproc;
>>    	}
>> 
>>    	/*
>>  @@ -1354,6 +1362,9 @@ static int rproc_start(struct rproc *rproc, 
>> const struct firmware *fw)
>>    	rproc_unprepare_subdevices(rproc);
>>    reset_table_ptr:
>>    	rproc->table_ptr = rproc->cached_table;
>>  +unprepare_rproc:
>>  +	if (rproc->ops->unprepare)
>>  +		rproc->ops->unprepare(rproc);
>> 
>>    	return ret;
>>    }
>>  @@ -1483,6 +1494,9 @@ static int rproc_stop(struct rproc *rproc, 
>> bool crashed)
>> 
>>    	rproc->state = RPROC_OFFLINE;
>> 
>>  +	if (rproc->ops->unprepare)
>>  +		rproc->ops->unprepare(rproc);
>>  +
>>    	dev_info(dev, "stopped remote processor %s\n", rproc->name);
>> 
>>    	return 0;
>>  diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>  index 5f201f0c86c3..a6272d1ba384 100644
>>  --- a/include/linux/remoteproc.h
>>  +++ b/include/linux/remoteproc.h
>>  @@ -355,6 +355,8 @@ enum rsc_handling_status {
>> 
>>    /**
>>     * struct rproc_ops - platform-specific device handlers
>>  + * @prepare:	prepare the device for power up (before the firmware 
>> is loaded)
>>  + * @unprepare:	unprepare the device after it is stopped
>>     * @start:	power on the device and boot it
>>     * @stop:	power off the device
>>     * @kick:	kick a virtqueue (virtqueue id given as a parameter)
>>  @@ -371,6 +373,8 @@ enum rsc_handling_status {
>>     * @get_boot_addr:	get boot address to entry point specified in 
>> firmware
>>     */
>>    struct rproc_ops {
>>  +	int (*prepare)(struct rproc *rproc);
>>  +	void (*unprepare)(struct rproc *rproc);
>>    	int (*start)(struct rproc *rproc);
>>    	int (*stop)(struct rproc *rproc);
>>    	void (*kick)(struct rproc *rproc, int vqid);

Re: [PATCH v4 3/5] remoteproc: Add prepare/unprepare callbacks

[Clément Leger]

Hi Paul

----- On 14 Dec, 2019, at 23:30, Paul Cercueil paul@crapouillou.net wrote:

> Hi Fabien,
> 
> 
> Le jeu., déc. 12, 2019 at 10:03, Fabien DESSENNE
> <fabien.dessenne@st.com> a écrit :
>> Hi Paul
>> 
>> 
>> On 10/12/2019 5:40 PM, Paul Cercueil wrote:
>>>  The .prepare() callback is called before the firmware is loaded to
>>>  memory. This is useful for instance in the case where some setup is
>>>  required for the memory to be accessible.
>> 
>> 
>> I am trying to figure out what king of 'setup' may be required. From
>> the
>> ingenic driver I understand that you need to enable clocks to allow
>> some
>> memory access.
>> 
>> Instead of adding this new ops, why not enabling clocks in probe()?
> 
> Enabling the clocks in the probe means that the clocks will be
> unconditionally enabled until the driver is removed, even if the remote
> processor end up being unused. That would be a waste of power.

We have the same kind of "problem" for k1c remoteproc driver (not yet
upstream, depends on new arch). We need to enable clocks to load code
into remote processor memory and currently we do that in probe.
However, as you stated, we would like to enable them as late as possible
(just before loading code) to avoid wasting power unnecessarily. So the
"prepare" callback totally makes sense.

Regards,

Clément

> 
> Cheers,
> -Paul
> 
> 
>> 
>> BR
>> 
>> Fabien
>> 
>> 
>>> 
>>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>  ---
>>> 
>>>  Notes:
>>>       v2-v4: No change
>>> 
>>>    drivers/remoteproc/remoteproc_core.c | 16 +++++++++++++++-
>>>    include/linux/remoteproc.h           |  4 ++++
>>>    2 files changed, 19 insertions(+), 1 deletion(-)
>>> 
>>>  diff --git a/drivers/remoteproc/remoteproc_core.c
>>> b/drivers/remoteproc/remoteproc_core.c
>>>  index 0a9fc7fdd1c3..3ea5f675a148 100644
>>>  --- a/drivers/remoteproc/remoteproc_core.c
>>>  +++ b/drivers/remoteproc/remoteproc_core.c
>>>  @@ -1299,11 +1299,19 @@ static int rproc_start(struct rproc *rproc,
>>> const struct firmware *fw)
>>>    	struct device *dev = &rproc->dev;
>>>    	int ret;
>>> 
>>>  +	if (rproc->ops->prepare) {
>>>  +		ret = rproc->ops->prepare(rproc);
>>>  +		if (ret) {
>>>  +			dev_err(dev, "Failed to prepare rproc: %d\n", ret);
>>>  +			return ret;
>>>  +		}
>>>  +	}
>>>  +
>>>    	/* load the ELF segments to memory */
>>>    	ret = rproc_load_segments(rproc, fw);
>>>    	if (ret) {
>>>    		dev_err(dev, "Failed to load program segments: %d\n", ret);
>>>  -		return ret;
>>>  +		goto unprepare_rproc;
>>>    	}
>>> 
>>>    	/*
>>>  @@ -1354,6 +1362,9 @@ static int rproc_start(struct rproc *rproc,
>>> const struct firmware *fw)
>>>    	rproc_unprepare_subdevices(rproc);
>>>    reset_table_ptr:
>>>    	rproc->table_ptr = rproc->cached_table;
>>>  +unprepare_rproc:
>>>  +	if (rproc->ops->unprepare)
>>>  +		rproc->ops->unprepare(rproc);
>>> 
>>>    	return ret;
>>>    }
>>>  @@ -1483,6 +1494,9 @@ static int rproc_stop(struct rproc *rproc,
>>> bool crashed)
>>> 
>>>    	rproc->state = RPROC_OFFLINE;
>>> 
>>>  +	if (rproc->ops->unprepare)
>>>  +		rproc->ops->unprepare(rproc);
>>>  +
>>>    	dev_info(dev, "stopped remote processor %s\n", rproc->name);
>>> 
>>>    	return 0;
>>>  diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>>  index 5f201f0c86c3..a6272d1ba384 100644
>>>  --- a/include/linux/remoteproc.h
>>>  +++ b/include/linux/remoteproc.h
>>>  @@ -355,6 +355,8 @@ enum rsc_handling_status {
>>> 
>>>    /**
>>>     * struct rproc_ops - platform-specific device handlers
>>>  + * @prepare:	prepare the device for power up (before the firmware
>>> is loaded)
>>>  + * @unprepare:	unprepare the device after it is stopped
>>>     * @start:	power on the device and boot it
>>>     * @stop:	power off the device
>>>     * @kick:	kick a virtqueue (virtqueue id given as a parameter)
>>>  @@ -371,6 +373,8 @@ enum rsc_handling_status {
>>>     * @get_boot_addr:	get boot address to entry point specified in
>>> firmware
>>>     */
>>>    struct rproc_ops {
>>>  +	int (*prepare)(struct rproc *rproc);
>>>  +	void (*unprepare)(struct rproc *rproc);
>>>    	int (*start)(struct rproc *rproc);
>>>    	int (*stop)(struct rproc *rproc);
> >>    	void (*kick)(struct rproc *rproc, int vqid);

Re: [PATCH v4 2/5] remoteproc: Add device-managed variants of rproc_alloc/rproc_add

[Clément Leger]

Hi Paul,

I tested it on our driver and freeing/remove are called correctly on probe error and on module removal.

Tested-By: Clement Leger <cleger@kalray.eu>

Regards,

Clément

----- On 10 Dec, 2019, at 17:40, Paul Cercueil paul@crapouillou.net wrote:

> Add API functions devm_rproc_alloc() and devm_rproc_add(), which behave
> like rproc_alloc() and rproc_add() respectively, but register their
> respective cleanup function to be called on driver detach.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> 
> Notes:
>    v3: New patch
>    v4: No change
> 
> drivers/remoteproc/remoteproc_core.c | 67 ++++++++++++++++++++++++++++
> include/linux/remoteproc.h           |  5 +++
> 2 files changed, 72 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> index 307df98347ba..0a9fc7fdd1c3 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1932,6 +1932,33 @@ int rproc_add(struct rproc *rproc)
> }
> EXPORT_SYMBOL(rproc_add);
> 
> +static void devm_rproc_remove(void *rproc)
> +{
> +	rproc_del(rproc);
> +}
> +
> +/**
> + * devm_rproc_add() - resource managed rproc_add()
> + * @dev: the underlying device
> + * @rproc: the remote processor handle to register
> + *
> + * This function performs like rproc_add() but the registered rproc device will
> + * automatically be removed on driver detach.
> + *
> + * Returns 0 on success and an appropriate error code otherwise.
> + */
> +int devm_rproc_add(struct device *dev, struct rproc *rproc)
> +{
> +	int err;
> +
> +	err = rproc_add(rproc);
> +	if (err)
> +		return err;
> +
> +	return devm_add_action_or_reset(dev, devm_rproc_remove, rproc);
> +}
> +EXPORT_SYMBOL(devm_rproc_add);
> +
> /**
>  * rproc_type_release() - release a remote processor instance
>  * @dev: the rproc's device
> @@ -2149,6 +2176,46 @@ int rproc_del(struct rproc *rproc)
> }
> EXPORT_SYMBOL(rproc_del);
> 
> +static void devm_rproc_free(struct device *dev, void *res)
> +{
> +	rproc_free(*(struct rproc **)res);
> +}
> +
> +/**
> + * devm_rproc_alloc() - resource managed rproc_alloc()
> + * @dev: the underlying device
> + * @name: name of this remote processor
> + * @ops: platform-specific handlers (mainly start/stop)
> + * @firmware: name of firmware file to load, can be NULL
> + * @len: length of private data needed by the rproc driver (in bytes)
> + *
> + * This function performs like rproc_alloc() but the acuired rproc device will
> + * automatically be released on driver detach.
> + *
> + * On success the new rproc is returned, and on failure, NULL.
> + */
> +struct rproc *devm_rproc_alloc(struct device *dev, const char *name,
> +			       const struct rproc_ops *ops,
> +			       const char *firmware, int len)
> +{
> +	struct rproc **ptr, *rproc;
> +
> +	ptr = devres_alloc(devm_rproc_free, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	rproc = rproc_alloc(dev, name, ops, firmware, len);
> +	if (rproc) {
> +		*ptr = rproc;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}
> +
> +	return rproc;
> +}
> +EXPORT_SYMBOL(devm_rproc_alloc);
> +
> /**
>  * rproc_add_subdev() - add a subdevice to a remoteproc
>  * @rproc: rproc handle to add the subdevice to
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 16ad66683ad0..5f201f0c86c3 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -595,6 +595,11 @@ int rproc_add(struct rproc *rproc);
> int rproc_del(struct rproc *rproc);
> void rproc_free(struct rproc *rproc);
> 
> +struct rproc *devm_rproc_alloc(struct device *dev, const char *name,
> +			       const struct rproc_ops *ops,
> +			       const char *firmware, int len);
> +int devm_rproc_add(struct device *dev, struct rproc *rproc);
> +
> void rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem);
> 
> struct rproc_mem_entry *
> --
> 2.24.0

Re: [PATCH v4 2/5] remoteproc: Add device-managed variants of rproc_alloc/rproc_add

[Paul Cercueil]

Hi Clément,


Le lun., déc. 16, 2019 at 11:46, Clément Leger <cleger@kalray.eu> a 
écrit :
> Hi Paul,
> 
> I tested it on our driver and freeing/remove are called correctly on 
> probe error and on module removal.
> 
> Tested-By: Clement Leger <cleger@kalray.eu>

Great, thanks.

Did you also test the prepare/unprepare callbacks? Is it working for 
you?

Thanks,
-Paul


> ----- On 10 Dec, 2019, at 17:40, Paul Cercueil paul@crapouillou.net 
> wrote:
> 
>>  Add API functions devm_rproc_alloc() and devm_rproc_add(), which 
>> behave
>>  like rproc_alloc() and rproc_add() respectively, but register their
>>  respective cleanup function to be called on driver detach.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>> 
>>  Notes:
>>     v3: New patch
>>     v4: No change
>> 
>>  drivers/remoteproc/remoteproc_core.c | 67 
>> ++++++++++++++++++++++++++++
>>  include/linux/remoteproc.h           |  5 +++
>>  2 files changed, 72 insertions(+)
>> 
>>  diff --git a/drivers/remoteproc/remoteproc_core.c
>>  b/drivers/remoteproc/remoteproc_core.c
>>  index 307df98347ba..0a9fc7fdd1c3 100644
>>  --- a/drivers/remoteproc/remoteproc_core.c
>>  +++ b/drivers/remoteproc/remoteproc_core.c
>>  @@ -1932,6 +1932,33 @@ int rproc_add(struct rproc *rproc)
>>  }
>>  EXPORT_SYMBOL(rproc_add);
>> 
>>  +static void devm_rproc_remove(void *rproc)
>>  +{
>>  +	rproc_del(rproc);
>>  +}
>>  +
>>  +/**
>>  + * devm_rproc_add() - resource managed rproc_add()
>>  + * @dev: the underlying device
>>  + * @rproc: the remote processor handle to register
>>  + *
>>  + * This function performs like rproc_add() but the registered 
>> rproc device will
>>  + * automatically be removed on driver detach.
>>  + *
>>  + * Returns 0 on success and an appropriate error code otherwise.
>>  + */
>>  +int devm_rproc_add(struct device *dev, struct rproc *rproc)
>>  +{
>>  +	int err;
>>  +
>>  +	err = rproc_add(rproc);
>>  +	if (err)
>>  +		return err;
>>  +
>>  +	return devm_add_action_or_reset(dev, devm_rproc_remove, rproc);
>>  +}
>>  +EXPORT_SYMBOL(devm_rproc_add);
>>  +
>>  /**
>>   * rproc_type_release() - release a remote processor instance
>>   * @dev: the rproc's device
>>  @@ -2149,6 +2176,46 @@ int rproc_del(struct rproc *rproc)
>>  }
>>  EXPORT_SYMBOL(rproc_del);
>> 
>>  +static void devm_rproc_free(struct device *dev, void *res)
>>  +{
>>  +	rproc_free(*(struct rproc **)res);
>>  +}
>>  +
>>  +/**
>>  + * devm_rproc_alloc() - resource managed rproc_alloc()
>>  + * @dev: the underlying device
>>  + * @name: name of this remote processor
>>  + * @ops: platform-specific handlers (mainly start/stop)
>>  + * @firmware: name of firmware file to load, can be NULL
>>  + * @len: length of private data needed by the rproc driver (in 
>> bytes)
>>  + *
>>  + * This function performs like rproc_alloc() but the acuired rproc 
>> device will
>>  + * automatically be released on driver detach.
>>  + *
>>  + * On success the new rproc is returned, and on failure, NULL.
>>  + */
>>  +struct rproc *devm_rproc_alloc(struct device *dev, const char 
>> *name,
>>  +			       const struct rproc_ops *ops,
>>  +			       const char *firmware, int len)
>>  +{
>>  +	struct rproc **ptr, *rproc;
>>  +
>>  +	ptr = devres_alloc(devm_rproc_free, sizeof(*ptr), GFP_KERNEL);
>>  +	if (!ptr)
>>  +		return ERR_PTR(-ENOMEM);
>>  +
>>  +	rproc = rproc_alloc(dev, name, ops, firmware, len);
>>  +	if (rproc) {
>>  +		*ptr = rproc;
>>  +		devres_add(dev, ptr);
>>  +	} else {
>>  +		devres_free(ptr);
>>  +	}
>>  +
>>  +	return rproc;
>>  +}
>>  +EXPORT_SYMBOL(devm_rproc_alloc);
>>  +
>>  /**
>>   * rproc_add_subdev() - add a subdevice to a remoteproc
>>   * @rproc: rproc handle to add the subdevice to
>>  diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>  index 16ad66683ad0..5f201f0c86c3 100644
>>  --- a/include/linux/remoteproc.h
>>  +++ b/include/linux/remoteproc.h
>>  @@ -595,6 +595,11 @@ int rproc_add(struct rproc *rproc);
>>  int rproc_del(struct rproc *rproc);
>>  void rproc_free(struct rproc *rproc);
>> 
>>  +struct rproc *devm_rproc_alloc(struct device *dev, const char 
>> *name,
>>  +			       const struct rproc_ops *ops,
>>  +			       const char *firmware, int len);
>>  +int devm_rproc_add(struct device *dev, struct rproc *rproc);
>>  +
>>  void rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry 
>> *mem);
>> 
>>  struct rproc_mem_entry *
>>  --
>>  2.24.0

Re: [PATCH v4 3/5] remoteproc: Add prepare/unprepare callbacks

[Clément Leger]

Hi Paul,

I tested your patch and implemented the prepare callback only.
This is working correctly and the callback is correctly called
to enable clocks right before loading code into memory.

Regarding unprepare, I have no usecase for this since clocks
disabling and resetting is done in rproc_stop. Thus, I did not
tested this part. Did you add it to be called after the subdevices
unprepare call ? If not, what is the use case for this ?
ie, what are you doing in unprepare that you don't do in stop ?

Obviously it should work but I can't say I tested it completely !

Regards,

Clément


----- On 16 Dec, 2019, at 09:42, Clément Leger cleger@kalray.eu wrote:

> Hi Paul
> 
> ----- On 14 Dec, 2019, at 23:30, Paul Cercueil paul@crapouillou.net wrote:
> 
>> Hi Fabien,
>> 
>> 
>> Le jeu., déc. 12, 2019 at 10:03, Fabien DESSENNE
>> <fabien.dessenne@st.com> a écrit :
>>> Hi Paul
>>> 
>>> 
>>> On 10/12/2019 5:40 PM, Paul Cercueil wrote:
>>>>  The .prepare() callback is called before the firmware is loaded to
>>>>  memory. This is useful for instance in the case where some setup is
>>>>  required for the memory to be accessible.
>>> 
>>> 
>>> I am trying to figure out what king of 'setup' may be required. From
>>> the
>>> ingenic driver I understand that you need to enable clocks to allow
>>> some
>>> memory access.
>>> 
>>> Instead of adding this new ops, why not enabling clocks in probe()?
>> 
>> Enabling the clocks in the probe means that the clocks will be
>> unconditionally enabled until the driver is removed, even if the remote
>> processor end up being unused. That would be a waste of power.
> 
> We have the same kind of "problem" for k1c remoteproc driver (not yet
> upstream, depends on new arch). We need to enable clocks to load code
> into remote processor memory and currently we do that in probe.
> However, as you stated, we would like to enable them as late as possible
> (just before loading code) to avoid wasting power unnecessarily. So the
> "prepare" callback totally makes sense.
> 
> Regards,
> 
> Clément
> 
>> 
>> Cheers,
>> -Paul
>> 
>> 
>>> 
>>> BR
>>> 
>>> Fabien
>>> 
>>> 
>>>> 
>>>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>>  ---
>>>> 
>>>>  Notes:
>>>>       v2-v4: No change
>>>> 
>>>>    drivers/remoteproc/remoteproc_core.c | 16 +++++++++++++++-
>>>>    include/linux/remoteproc.h           |  4 ++++
>>>>    2 files changed, 19 insertions(+), 1 deletion(-)
>>>> 
>>>>  diff --git a/drivers/remoteproc/remoteproc_core.c
>>>> b/drivers/remoteproc/remoteproc_core.c
>>>>  index 0a9fc7fdd1c3..3ea5f675a148 100644
>>>>  --- a/drivers/remoteproc/remoteproc_core.c
>>>>  +++ b/drivers/remoteproc/remoteproc_core.c
>>>>  @@ -1299,11 +1299,19 @@ static int rproc_start(struct rproc *rproc,
>>>> const struct firmware *fw)
>>>>    	struct device *dev = &rproc->dev;
>>>>    	int ret;
>>>> 
>>>>  +	if (rproc->ops->prepare) {
>>>>  +		ret = rproc->ops->prepare(rproc);
>>>>  +		if (ret) {
>>>>  +			dev_err(dev, "Failed to prepare rproc: %d\n", ret);
>>>>  +			return ret;
>>>>  +		}
>>>>  +	}
>>>>  +
>>>>    	/* load the ELF segments to memory */
>>>>    	ret = rproc_load_segments(rproc, fw);
>>>>    	if (ret) {
>>>>    		dev_err(dev, "Failed to load program segments: %d\n", ret);
>>>>  -		return ret;
>>>>  +		goto unprepare_rproc;
>>>>    	}
>>>> 
>>>>    	/*
>>>>  @@ -1354,6 +1362,9 @@ static int rproc_start(struct rproc *rproc,
>>>> const struct firmware *fw)
>>>>    	rproc_unprepare_subdevices(rproc);
>>>>    reset_table_ptr:
>>>>    	rproc->table_ptr = rproc->cached_table;
>>>>  +unprepare_rproc:
>>>>  +	if (rproc->ops->unprepare)
>>>>  +		rproc->ops->unprepare(rproc);
>>>> 
>>>>    	return ret;
>>>>    }
>>>>  @@ -1483,6 +1494,9 @@ static int rproc_stop(struct rproc *rproc,
>>>> bool crashed)
>>>> 
>>>>    	rproc->state = RPROC_OFFLINE;
>>>> 
>>>>  +	if (rproc->ops->unprepare)
>>>>  +		rproc->ops->unprepare(rproc);
>>>>  +
>>>>    	dev_info(dev, "stopped remote processor %s\n", rproc->name);
>>>> 
>>>>    	return 0;
>>>>  diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>>>  index 5f201f0c86c3..a6272d1ba384 100644
>>>>  --- a/include/linux/remoteproc.h
>>>>  +++ b/include/linux/remoteproc.h
>>>>  @@ -355,6 +355,8 @@ enum rsc_handling_status {
>>>> 
>>>>    /**
>>>>     * struct rproc_ops - platform-specific device handlers
>>>>  + * @prepare:	prepare the device for power up (before the firmware
>>>> is loaded)
>>>>  + * @unprepare:	unprepare the device after it is stopped
>>>>     * @start:	power on the device and boot it
>>>>     * @stop:	power off the device
>>>>     * @kick:	kick a virtqueue (virtqueue id given as a parameter)
>>>>  @@ -371,6 +373,8 @@ enum rsc_handling_status {
>>>>     * @get_boot_addr:	get boot address to entry point specified in
>>>> firmware
>>>>     */
>>>>    struct rproc_ops {
>>>>  +	int (*prepare)(struct rproc *rproc);
>>>>  +	void (*unprepare)(struct rproc *rproc);
>>>>    	int (*start)(struct rproc *rproc);
>>>>    	int (*stop)(struct rproc *rproc);
> > >>    	void (*kick)(struct rproc *rproc, int vqid);

Re: [PATCH v4 3/5] remoteproc: Add prepare/unprepare callbacks

[Fabien DESSENNE]

Hi Paul


On 14/12/2019 11:30 PM, Paul Cercueil wrote:
> Hi Fabien,
>
>
> Le jeu., déc. 12, 2019 at 10:03, Fabien DESSENNE 
> <fabien.dessenne@st.com> a écrit :
>> Hi Paul
>>
>>
>> On 10/12/2019 5:40 PM, Paul Cercueil wrote:
>>>  The .prepare() callback is called before the firmware is loaded to
>>>  memory. This is useful for instance in the case where some setup is
>>>  required for the memory to be accessible.
>>
>>
>> I am trying to figure out what king of 'setup' may be required. From the
>> ingenic driver I understand that you need to enable clocks to allow some
>> memory access.
>>
>> Instead of adding this new ops, why not enabling clocks in probe()?
>
> Enabling the clocks in the probe means that the clocks will be 
> unconditionally enabled until the driver is removed, even if the 
> remote processor end up being unused. That would be a waste of power.


OK I understand.

Nevertheless I think that you may need to call .prepare() from 
rproc_fw_boot() since you may need to access some memories from the 
point rproc_handle_resources() is called (this sets up virtio which is 
used if you have a resource table defining vdev).

And rproc_fw_boot() calls rproc_enable_iommu(), which sounds like 
"prepare memory", so this may be the right place to call .prepare()


BR

Fabien


>
> Cheers,
> -Paul
>
>
>>
>> BR
>>
>> Fabien
>>
>>
>>>
>>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>  ---
>>>
>>>  Notes:
>>>       v2-v4: No change
>>>
>>>    drivers/remoteproc/remoteproc_core.c | 16 +++++++++++++++-
>>>    include/linux/remoteproc.h           |  4 ++++
>>>    2 files changed, 19 insertions(+), 1 deletion(-)
>>>
>>>  diff --git a/drivers/remoteproc/remoteproc_core.c 
>>> b/drivers/remoteproc/remoteproc_core.c
>>>  index 0a9fc7fdd1c3..3ea5f675a148 100644
>>>  --- a/drivers/remoteproc/remoteproc_core.c
>>>  +++ b/drivers/remoteproc/remoteproc_core.c
>>>  @@ -1299,11 +1299,19 @@ static int rproc_start(struct rproc *rproc, 
>>> const struct firmware *fw)
>>>        struct device *dev = &rproc->dev;
>>>        int ret;
>>>
>>>  +    if (rproc->ops->prepare) {
>>>  +        ret = rproc->ops->prepare(rproc);
>>>  +        if (ret) {
>>>  +            dev_err(dev, "Failed to prepare rproc: %d\n", ret);
>>>  +            return ret;
>>>  +        }
>>>  +    }
>>>  +
>>>        /* load the ELF segments to memory */
>>>        ret = rproc_load_segments(rproc, fw);
>>>        if (ret) {
>>>            dev_err(dev, "Failed to load program segments: %d\n", ret);
>>>  -        return ret;
>>>  +        goto unprepare_rproc;
>>>        }
>>>
>>>        /*
>>>  @@ -1354,6 +1362,9 @@ static int rproc_start(struct rproc *rproc, 
>>> const struct firmware *fw)
>>>        rproc_unprepare_subdevices(rproc);
>>>    reset_table_ptr:
>>>        rproc->table_ptr = rproc->cached_table;
>>>  +unprepare_rproc:
>>>  +    if (rproc->ops->unprepare)
>>>  +        rproc->ops->unprepare(rproc);
>>>
>>>        return ret;
>>>    }
>>>  @@ -1483,6 +1494,9 @@ static int rproc_stop(struct rproc *rproc, 
>>> bool crashed)
>>>
>>>        rproc->state = RPROC_OFFLINE;
>>>
>>>  +    if (rproc->ops->unprepare)
>>>  +        rproc->ops->unprepare(rproc);
>>>  +
>>>        dev_info(dev, "stopped remote processor %s\n", rproc->name);
>>>
>>>        return 0;
>>>  diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>>  index 5f201f0c86c3..a6272d1ba384 100644
>>>  --- a/include/linux/remoteproc.h
>>>  +++ b/include/linux/remoteproc.h
>>>  @@ -355,6 +355,8 @@ enum rsc_handling_status {
>>>
>>>    /**
>>>     * struct rproc_ops - platform-specific device handlers
>>>  + * @prepare:    prepare the device for power up (before the 
>>> firmware is loaded)
>>>  + * @unprepare:    unprepare the device after it is stopped
>>>     * @start:    power on the device and boot it
>>>     * @stop:    power off the device
>>>     * @kick:    kick a virtqueue (virtqueue id given as a parameter)
>>>  @@ -371,6 +373,8 @@ enum rsc_handling_status {
>>>     * @get_boot_addr:    get boot address to entry point specified 
>>> in firmware
>>>     */
>>>    struct rproc_ops {
>>>  +    int (*prepare)(struct rproc *rproc);
>>>  +    void (*unprepare)(struct rproc *rproc);
>>>        int (*start)(struct rproc *rproc);
>>>        int (*stop)(struct rproc *rproc);
>>>        void (*kick)(struct rproc *rproc, int vqid);
>
>

Re: [PATCH v4 3/5] remoteproc: Add prepare/unprepare callbacks

[Bjorn Andersson]

On Tue 10 Dec 08:40 PST 2019, Paul Cercueil wrote:

> The .prepare() callback is called before the firmware is loaded to
> memory. This is useful for instance in the case where some setup is
> required for the memory to be accessible.
> 

Would it make sense to somehow tie this prepare/unprepare to the actual
struct rproc_mem_entry that needs the resource enabled?

Regards,
Bjorn

> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> 
> Notes:
>     v2-v4: No change
> 
>  drivers/remoteproc/remoteproc_core.c | 16 +++++++++++++++-
>  include/linux/remoteproc.h           |  4 ++++
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 0a9fc7fdd1c3..3ea5f675a148 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1299,11 +1299,19 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
>  	struct device *dev = &rproc->dev;
>  	int ret;
>  
> +	if (rproc->ops->prepare) {
> +		ret = rproc->ops->prepare(rproc);
> +		if (ret) {
> +			dev_err(dev, "Failed to prepare rproc: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
>  	/* load the ELF segments to memory */
>  	ret = rproc_load_segments(rproc, fw);
>  	if (ret) {
>  		dev_err(dev, "Failed to load program segments: %d\n", ret);
> -		return ret;
> +		goto unprepare_rproc;
>  	}
>  
>  	/*
> @@ -1354,6 +1362,9 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
>  	rproc_unprepare_subdevices(rproc);
>  reset_table_ptr:
>  	rproc->table_ptr = rproc->cached_table;
> +unprepare_rproc:
> +	if (rproc->ops->unprepare)
> +		rproc->ops->unprepare(rproc);
>  
>  	return ret;
>  }
> @@ -1483,6 +1494,9 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
>  
>  	rproc->state = RPROC_OFFLINE;
>  
> +	if (rproc->ops->unprepare)
> +		rproc->ops->unprepare(rproc);
> +
>  	dev_info(dev, "stopped remote processor %s\n", rproc->name);
>  
>  	return 0;
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 5f201f0c86c3..a6272d1ba384 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -355,6 +355,8 @@ enum rsc_handling_status {
>  
>  /**
>   * struct rproc_ops - platform-specific device handlers
> + * @prepare:	prepare the device for power up (before the firmware is loaded)
> + * @unprepare:	unprepare the device after it is stopped
>   * @start:	power on the device and boot it
>   * @stop:	power off the device
>   * @kick:	kick a virtqueue (virtqueue id given as a parameter)
> @@ -371,6 +373,8 @@ enum rsc_handling_status {
>   * @get_boot_addr:	get boot address to entry point specified in firmware
>   */
>  struct rproc_ops {
> +	int (*prepare)(struct rproc *rproc);
> +	void (*unprepare)(struct rproc *rproc);
>  	int (*start)(struct rproc *rproc);
>  	int (*stop)(struct rproc *rproc);
>  	void (*kick)(struct rproc *rproc, int vqid);
> -- 
> 2.24.0
> 

Re: [PATCH v4 3/5] remoteproc: Add prepare/unprepare callbacks

[Paul Cercueil]

Hi Bjorn,


Le sam., déc. 21, 2019 at 12:20, Bjorn Andersson 
<bjorn.andersson@linaro.org> a écrit :
> On Tue 10 Dec 08:40 PST 2019, Paul Cercueil wrote:
> 
>>  The .prepare() callback is called before the firmware is loaded to
>>  memory. This is useful for instance in the case where some setup is
>>  required for the memory to be accessible.
>> 
> 
> Would it make sense to somehow tie this prepare/unprepare to the 
> actual
> struct rproc_mem_entry that needs the resource enabled?

Do you need such granularity?

In my case, the three memories need the same clock to be enabled.

-Paul


> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>> 
>>  Notes:
>>      v2-v4: No change
>> 
>>   drivers/remoteproc/remoteproc_core.c | 16 +++++++++++++++-
>>   include/linux/remoteproc.h           |  4 ++++
>>   2 files changed, 19 insertions(+), 1 deletion(-)
>> 
>>  diff --git a/drivers/remoteproc/remoteproc_core.c 
>> b/drivers/remoteproc/remoteproc_core.c
>>  index 0a9fc7fdd1c3..3ea5f675a148 100644
>>  --- a/drivers/remoteproc/remoteproc_core.c
>>  +++ b/drivers/remoteproc/remoteproc_core.c
>>  @@ -1299,11 +1299,19 @@ static int rproc_start(struct rproc *rproc, 
>> const struct firmware *fw)
>>   	struct device *dev = &rproc->dev;
>>   	int ret;
>> 
>>  +	if (rproc->ops->prepare) {
>>  +		ret = rproc->ops->prepare(rproc);
>>  +		if (ret) {
>>  +			dev_err(dev, "Failed to prepare rproc: %d\n", ret);
>>  +			return ret;
>>  +		}
>>  +	}
>>  +
>>   	/* load the ELF segments to memory */
>>   	ret = rproc_load_segments(rproc, fw);
>>   	if (ret) {
>>   		dev_err(dev, "Failed to load program segments: %d\n", ret);
>>  -		return ret;
>>  +		goto unprepare_rproc;
>>   	}
>> 
>>   	/*
>>  @@ -1354,6 +1362,9 @@ static int rproc_start(struct rproc *rproc, 
>> const struct firmware *fw)
>>   	rproc_unprepare_subdevices(rproc);
>>   reset_table_ptr:
>>   	rproc->table_ptr = rproc->cached_table;
>>  +unprepare_rproc:
>>  +	if (rproc->ops->unprepare)
>>  +		rproc->ops->unprepare(rproc);
>> 
>>   	return ret;
>>   }
>>  @@ -1483,6 +1494,9 @@ static int rproc_stop(struct rproc *rproc, 
>> bool crashed)
>> 
>>   	rproc->state = RPROC_OFFLINE;
>> 
>>  +	if (rproc->ops->unprepare)
>>  +		rproc->ops->unprepare(rproc);
>>  +
>>   	dev_info(dev, "stopped remote processor %s\n", rproc->name);
>> 
>>   	return 0;
>>  diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>  index 5f201f0c86c3..a6272d1ba384 100644
>>  --- a/include/linux/remoteproc.h
>>  +++ b/include/linux/remoteproc.h
>>  @@ -355,6 +355,8 @@ enum rsc_handling_status {
>> 
>>   /**
>>    * struct rproc_ops - platform-specific device handlers
>>  + * @prepare:	prepare the device for power up (before the firmware 
>> is loaded)
>>  + * @unprepare:	unprepare the device after it is stopped
>>    * @start:	power on the device and boot it
>>    * @stop:	power off the device
>>    * @kick:	kick a virtqueue (virtqueue id given as a parameter)
>>  @@ -371,6 +373,8 @@ enum rsc_handling_status {
>>    * @get_boot_addr:	get boot address to entry point specified in 
>> firmware
>>    */
>>   struct rproc_ops {
>>  +	int (*prepare)(struct rproc *rproc);
>>  +	void (*unprepare)(struct rproc *rproc);
>>   	int (*start)(struct rproc *rproc);
>>   	int (*stop)(struct rproc *rproc);
>>   	void (*kick)(struct rproc *rproc, int vqid);
>>  --
>>  2.24.0
>> 

Re: [PATCH v4 2/5] remoteproc: Add device-managed variants of rproc_alloc/rproc_add

[Bjorn Andersson]

On Thu 12 Dec 01:43 PST 2019, Fabien DESSENNE wrote:

> Hi Paul,
> 
> 
> Good initiative! See me remarks below.
> 

I concur!

> 
> On 10/12/2019 5:40 PM, Paul Cercueil wrote:
> > Add API functions devm_rproc_alloc() and devm_rproc_add(), which behave
> > like rproc_alloc() and rproc_add() respectively, but register their
> > respective cleanup function to be called on driver detach.
> >
> > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > ---
> >
> > Notes:
> >      v3: New patch
> >      v4: No change
> >
> >   drivers/remoteproc/remoteproc_core.c | 67 ++++++++++++++++++++++++++++
> >   include/linux/remoteproc.h           |  5 +++
> >   2 files changed, 72 insertions(+)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 307df98347ba..0a9fc7fdd1c3 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> 
> 
> Maybe these devm function shall be defined in a new remoteproc/devres.c 
> file. Although it seems to be a common usage I don't know if there is a 
> rule for that.
> 

Let's keep it in this same file, the devres.c would be tiny.

> 
> > @@ -1932,6 +1932,33 @@ int rproc_add(struct rproc *rproc)
> >   }
> >   EXPORT_SYMBOL(rproc_add);
> >   
> > +static void devm_rproc_remove(void *rproc)
> > +{
> > +	rproc_del(rproc);
> > +}
> > +
> > +/**
> > + * devm_rproc_add() - resource managed rproc_add()
> > + * @dev: the underlying device
> > + * @rproc: the remote processor handle to register
> > + *
> > + * This function performs like rproc_add() but the registered rproc device will
> > + * automatically be removed on driver detach.
> > + *
> > + * Returns 0 on success and an appropriate error code otherwise.
> > + */
> > +int devm_rproc_add(struct device *dev, struct rproc *rproc)
> > +{
> > +	int err;
> > +
> > +	err = rproc_add(rproc);
> > +	if (err)
> > +		return err;
> > +
> > +	return devm_add_action_or_reset(dev, devm_rproc_remove, rproc);
> > +}
> > +EXPORT_SYMBOL(devm_rproc_add);
> > +
> >   /**
> >    * rproc_type_release() - release a remote processor instance
> >    * @dev: the rproc's device
> > @@ -2149,6 +2176,46 @@ int rproc_del(struct rproc *rproc)
> >   }
> >   EXPORT_SYMBOL(rproc_del);
> >   
> > +static void devm_rproc_free(struct device *dev, void *res)
> > +{
> > +	rproc_free(*(struct rproc **)res);
> > +}
> > +
> > +/**
> > + * devm_rproc_alloc() - resource managed rproc_alloc()
> > + * @dev: the underlying device
> > + * @name: name of this remote processor
> > + * @ops: platform-specific handlers (mainly start/stop)
> > + * @firmware: name of firmware file to load, can be NULL
> > + * @len: length of private data needed by the rproc driver (in bytes)
> > + *
> > + * This function performs like rproc_alloc() but the acuired rproc device will
> 
> 
> typo: s/acuired/acquired
> 
> 
> > + * automatically be released on driver detach.
> > + *
> > + * On success the new rproc is returned, and on failure, NULL.
> > + */
> > +struct rproc *devm_rproc_alloc(struct device *dev, const char *name,
> > +			       const struct rproc_ops *ops,
> > +			       const char *firmware, int len)
> > +{
> > +	struct rproc **ptr, *rproc;
> > +
> > +	ptr = devres_alloc(devm_rproc_free, sizeof(*ptr), GFP_KERNEL);
> > +	if (!ptr)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	rproc = rproc_alloc(dev, name, ops, firmware, len);
> > +	if (rproc) {
> > +		*ptr = rproc;
> > +		devres_add(dev, ptr);
> > +	} else {
> > +		devres_free(ptr);
> > +	}
> > +
> > +	return rproc;
> 
> 
> Can't you use devm_add_action_or_reset() here too?
> 

The proposed function matches how everyone else is doing devm_*_alloc(),
so I would like to keep it as is.

Regards,
Bjorn

> 
> > +}
> > +EXPORT_SYMBOL(devm_rproc_alloc);
> > +
> >   /**
> >    * rproc_add_subdev() - add a subdevice to a remoteproc
> >    * @rproc: rproc handle to add the subdevice to
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index 16ad66683ad0..5f201f0c86c3 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -595,6 +595,11 @@ int rproc_add(struct rproc *rproc);
> >   int rproc_del(struct rproc *rproc);
> >   void rproc_free(struct rproc *rproc);
> >   
> > +struct rproc *devm_rproc_alloc(struct device *dev, const char *name,
> > +			       const struct rproc_ops *ops,
> > +			       const char *firmware, int len);
> > +int devm_rproc_add(struct device *dev, struct rproc *rproc);
> > +
> >   void rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem);
> >   
> >   struct rproc_mem_entry *
> 
> 
> BR
> 
> Fabien

Re: [PATCH v4 2/5] remoteproc: Add device-managed variants of rproc_alloc/rproc_add

[Bjorn Andersson]

On Tue 10 Dec 08:40 PST 2019, Paul Cercueil wrote:
[..]
> +/**
> + * devm_rproc_add() - resource managed rproc_add()
> + * @dev: the underlying device
> + * @rproc: the remote processor handle to register
> + *
> + * This function performs like rproc_add() but the registered rproc device will
> + * automatically be removed on driver detach.
> + *
> + * Returns 0 on success and an appropriate error code otherwise.

The kerneldoc format is "Return: foo on bar...". So please update this
to

Return: 0 on success, negative errno on failure

> + */
> +int devm_rproc_add(struct device *dev, struct rproc *rproc)
> +{
[..]
> +/**
> + * devm_rproc_alloc() - resource managed rproc_alloc()
> + * @dev: the underlying device
> + * @name: name of this remote processor
> + * @ops: platform-specific handlers (mainly start/stop)
> + * @firmware: name of firmware file to load, can be NULL
> + * @len: length of private data needed by the rproc driver (in bytes)
> + *
> + * This function performs like rproc_alloc() but the acuired rproc device will
> + * automatically be released on driver detach.
> + *
> + * On success the new rproc is returned, and on failure, NULL.

Return: new rproc instance, NULL on failure

Regards,
Bjorn

Re: [PATCH v4 3/5] remoteproc: Add prepare/unprepare callbacks

[Bjorn Andersson]

On Wed 15 Jan 13:15 PST 2020, Paul Cercueil wrote:

> Hi Bjorn,
> 
> 
> Le sam., déc. 21, 2019 at 12:20, Bjorn Andersson
> <bjorn.andersson@linaro.org> a écrit :
> > On Tue 10 Dec 08:40 PST 2019, Paul Cercueil wrote:
> > 
> > >  The .prepare() callback is called before the firmware is loaded to
> > >  memory. This is useful for instance in the case where some setup is
> > >  required for the memory to be accessible.
> > > 
> > 
> > Would it make sense to somehow tie this prepare/unprepare to the actual
> > struct rproc_mem_entry that needs the resource enabled?
> 
> Do you need such granularity?
> 

I don't have such needs, but given some of the memory structure that
Suman and Loic has been talking about I would expect that such need
exists.

> In my case, the three memories need the same clock to be enabled.
> 

But we can update your driver to associate your one clock with the
memory objects if/when we end up implementing this later.

Regards,
Bjorn

> -Paul
> 
> 
> > 
> > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > >  ---
> > > 
> > >  Notes:
> > >      v2-v4: No change
> > > 
> > >   drivers/remoteproc/remoteproc_core.c | 16 +++++++++++++++-
> > >   include/linux/remoteproc.h           |  4 ++++
> > >   2 files changed, 19 insertions(+), 1 deletion(-)
> > > 
> > >  diff --git a/drivers/remoteproc/remoteproc_core.c
> > > b/drivers/remoteproc/remoteproc_core.c
> > >  index 0a9fc7fdd1c3..3ea5f675a148 100644
> > >  --- a/drivers/remoteproc/remoteproc_core.c
> > >  +++ b/drivers/remoteproc/remoteproc_core.c
> > >  @@ -1299,11 +1299,19 @@ static int rproc_start(struct rproc *rproc,
> > > const struct firmware *fw)
> > >   	struct device *dev = &rproc->dev;
> > >   	int ret;
> > > 
> > >  +	if (rproc->ops->prepare) {
> > >  +		ret = rproc->ops->prepare(rproc);
> > >  +		if (ret) {
> > >  +			dev_err(dev, "Failed to prepare rproc: %d\n", ret);
> > >  +			return ret;
> > >  +		}
> > >  +	}
> > >  +
> > >   	/* load the ELF segments to memory */
> > >   	ret = rproc_load_segments(rproc, fw);
> > >   	if (ret) {
> > >   		dev_err(dev, "Failed to load program segments: %d\n", ret);
> > >  -		return ret;
> > >  +		goto unprepare_rproc;
> > >   	}
> > > 
> > >   	/*
> > >  @@ -1354,6 +1362,9 @@ static int rproc_start(struct rproc *rproc,
> > > const struct firmware *fw)
> > >   	rproc_unprepare_subdevices(rproc);
> > >   reset_table_ptr:
> > >   	rproc->table_ptr = rproc->cached_table;
> > >  +unprepare_rproc:
> > >  +	if (rproc->ops->unprepare)
> > >  +		rproc->ops->unprepare(rproc);
> > > 
> > >   	return ret;
> > >   }
> > >  @@ -1483,6 +1494,9 @@ static int rproc_stop(struct rproc *rproc,
> > > bool crashed)
> > > 
> > >   	rproc->state = RPROC_OFFLINE;
> > > 
> > >  +	if (rproc->ops->unprepare)
> > >  +		rproc->ops->unprepare(rproc);
> > >  +
> > >   	dev_info(dev, "stopped remote processor %s\n", rproc->name);
> > > 
> > >   	return 0;
> > >  diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > >  index 5f201f0c86c3..a6272d1ba384 100644
> > >  --- a/include/linux/remoteproc.h
> > >  +++ b/include/linux/remoteproc.h
> > >  @@ -355,6 +355,8 @@ enum rsc_handling_status {
> > > 
> > >   /**
> > >    * struct rproc_ops - platform-specific device handlers
> > >  + * @prepare:	prepare the device for power up (before the firmware
> > > is loaded)
> > >  + * @unprepare:	unprepare the device after it is stopped
> > >    * @start:	power on the device and boot it
> > >    * @stop:	power off the device
> > >    * @kick:	kick a virtqueue (virtqueue id given as a parameter)
> > >  @@ -371,6 +373,8 @@ enum rsc_handling_status {
> > >    * @get_boot_addr:	get boot address to entry point specified in
> > > firmware
> > >    */
> > >   struct rproc_ops {
> > >  +	int (*prepare)(struct rproc *rproc);
> > >  +	void (*unprepare)(struct rproc *rproc);
> > >   	int (*start)(struct rproc *rproc);
> > >   	int (*stop)(struct rproc *rproc);
> > >   	void (*kick)(struct rproc *rproc, int vqid);
> > >  --
> > >  2.24.0
> > > 
> 
> 

Re: [PATCH v4 3/5] remoteproc: Add prepare/unprepare callbacks

[Arnaud POULIQUEN]

Hi Bjorn,Paul,

On 1/20/20 9:19 PM, Bjorn Andersson wrote:
> On Wed 15 Jan 13:15 PST 2020, Paul Cercueil wrote:
> 
>> Hi Bjorn,
>>
>>
>> Le sam., déc. 21, 2019 at 12:20, Bjorn Andersson
>> <bjorn.andersson@linaro.org> a écrit :
>>> On Tue 10 Dec 08:40 PST 2019, Paul Cercueil wrote:
>>>
>>>>  The .prepare() callback is called before the firmware is loaded to
>>>>  memory. This is useful for instance in the case where some setup is
>>>>  required for the memory to be accessible.
>>>>
>>>
>>> Would it make sense to somehow tie this prepare/unprepare to the actual
>>> struct rproc_mem_entry that needs the resource enabled?
>>
>> Do you need such granularity?
>>
> 
> I don't have such needs, but given some of the memory structure that
> Suman and Loic has been talking about I would expect that such need
> exists
> 
>> In my case, the three memories need the same clock to be enabled.
>>
> 
> But we can update your driver to associate your one clock with the
> memory objects if/when we end up implementing this later.

IMHO, as mention Fabien the prepare/unprepare should probably not in the stop/start
but in rproc_fw_boot and rproc_resource_cleanup. This should cover more usecases (such as power domain) and
avoid a unprepare/prepare on crash recovery.

Nevertheless I'm wondering if it's more like PM runtime that should be implemented here...?

Regards,
Arnaud

> 
> Regards,
> Bjorn
> 
>> -Paul
>>
>>
>>>
>>>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>>  ---
>>>>
>>>>  Notes:
>>>>      v2-v4: No change
>>>>
>>>>   drivers/remoteproc/remoteproc_core.c | 16 +++++++++++++++-
>>>>   include/linux/remoteproc.h           |  4 ++++
>>>>   2 files changed, 19 insertions(+), 1 deletion(-)
>>>>
>>>>  diff --git a/drivers/remoteproc/remoteproc_core.c
>>>> b/drivers/remoteproc/remoteproc_core.c
>>>>  index 0a9fc7fdd1c3..3ea5f675a148 100644
>>>>  --- a/drivers/remoteproc/remoteproc_core.c
>>>>  +++ b/drivers/remoteproc/remoteproc_core.c
>>>>  @@ -1299,11 +1299,19 @@ static int rproc_start(struct rproc *rproc,
>>>> const struct firmware *fw)
>>>>   	struct device *dev = &rproc->dev;
>>>>   	int ret;
>>>>
>>>>  +	if (rproc->ops->prepare) {
>>>>  +		ret = rproc->ops->prepare(rproc);
>>>>  +		if (ret) {
>>>>  +			dev_err(dev, "Failed to prepare rproc: %d\n", ret);
>>>>  +			return ret;
>>>>  +		}
>>>>  +	}
>>>>  +

>>>>   	/* load the ELF segments to memory */
>>>>   	ret = rproc_load_segments(rproc, fw);
>>>>   	if (ret) {
>>>>   		dev_err(dev, "Failed to load program segments: %d\n", ret);
>>>>  -		return ret;
>>>>  +		goto unprepare_rproc;
>>>>   	}
>>>>
>>>>   	/*
>>>>  @@ -1354,6 +1362,9 @@ static int rproc_start(struct rproc *rproc,
>>>> const struct firmware *fw)
>>>>   	rproc_unprepare_subdevices(rproc);
>>>>   reset_table_ptr:
>>>>   	rproc->table_ptr = rproc->cached_table;
>>>>  +unprepare_rproc:
>>>>  +	if (rproc->ops->unprepare)
>>>>  +		rproc->ops->unprepare(rproc);
>>>>
>>>>   	return ret;
>>>>   }
>>>>  @@ -1483,6 +1494,9 @@ static int rproc_stop(struct rproc *rproc,
>>>> bool crashed)
>>>>
>>>>   	rproc->state = RPROC_OFFLINE;
>>>>
>>>>  +	if (rproc->ops->unprepare)
>>>>  +		rproc->ops->unprepare(rproc);
>>>>  +
>>>>   	dev_info(dev, "stopped remote processor %s\n", rproc->name);
>>>>
>>>>   	return 0;
>>>>  diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>>>  index 5f201f0c86c3..a6272d1ba384 100644
>>>>  --- a/include/linux/remoteproc.h
>>>>  +++ b/include/linux/remoteproc.h
>>>>  @@ -355,6 +355,8 @@ enum rsc_handling_status {
>>>>
>>>>   /**
>>>>    * struct rproc_ops - platform-specific device handlers
>>>>  + * @prepare:	prepare the device for power up (before the firmware
>>>> is loaded)
>>>>  + * @unprepare:	unprepare the device after it is stopped
>>>>    * @start:	power on the device and boot it
>>>>    * @stop:	power off the device
>>>>    * @kick:	kick a virtqueue (virtqueue id given as a parameter)
>>>>  @@ -371,6 +373,8 @@ enum rsc_handling_status {
>>>>    * @get_boot_addr:	get boot address to entry point specified in
>>>> firmware
>>>>    */
>>>>   struct rproc_ops {
>>>>  +	int (*prepare)(struct rproc *rproc);
>>>>  +	void (*unprepare)(struct rproc *rproc);
>>>>   	int (*start)(struct rproc *rproc);
>>>>   	int (*stop)(struct rproc *rproc);
>>>>   	void (*kick)(struct rproc *rproc, int vqid);
>>>>  --
>>>>  2.24.0
>>>>
>>
>>